feat: use prefix instead hard coded admin #595
Conversation
WalkthroughThe changes refactor the application's routing logic to replace hardcoded Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Router
participant App
participant Components
User->>Router: Navigate to /dashboard/...
Router->>App: Request prefix via app.prefix
App-->>Router: Return 'dashboard'
Router->>Components: Render routes using dynamic prefix
Components->>Router: Use app.prefix for navigation and matching
Poem
✨ Finishing Touches
🧪 Generate Unit Tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/client/src/application/Application.tsx (1)
118-120: Consider making the prefix configurable.The new
prefixgetter provides centralized prefix management, which is good. However, the hardcoded 'dashboard' value might need to be configurable for different deployment scenarios.Consider making the prefix configurable through application options:
+export interface ApplicationOptions { + prefix?: string; // ... other options +} get prefix() { - return 'dashboard'; + return this.options.prefix || 'dashboard'; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
packages/client/src/application/Application.tsx(1 hunks)packages/client/src/application/CustomRouterContextProvider.tsx(0 hunks)packages/client/src/application/RouterManager.tsx(1 hunks)packages/client/src/built-in/admin-layout/MenuEditor.tsx(4 hunks)packages/client/src/built-in/admin-layout/components/AddNewButton.tsx(3 hunks)packages/client/src/built-in/dynamic-page/index.tsx(1 hunks)packages/client/src/built-in/index.tsx(3 hunks)packages/client/src/built-in/setting-layout/SettingLayout.tsx(1 hunks)packages/client/src/built-in/user-settings/UserSettingsLayout.tsx(1 hunks)packages/client/src/collection-manager/CollectionHistoryProvider.tsx(3 hunks)
💤 Files with no reviewable changes (1)
- packages/client/src/application/CustomRouterContextProvider.tsx
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: CI
🔇 Additional comments (14)
packages/client/src/built-in/user-settings/UserSettingsLayout.tsx (1)
72-72: LGTM! Fallback path correctly uses dynamic prefix.The change from hardcoded
'/admin'to/${app.prefix}ensures the fallback navigation aligns with the application's configured prefix.packages/client/src/built-in/setting-layout/SettingLayout.tsx (1)
96-96: LGTM! Home button navigation correctly uses dynamic prefix.The navigation path now uses
/${app.prefix}instead of the hardcoded/admin, making the routing configurable.packages/client/src/application/RouterManager.tsx (1)
148-154: LGTM! Simplified routing context management.The removal of
CustomRouterContextProviderwrapper streamlines the JSX structure and reduces unnecessary routing context complexity.packages/client/src/built-in/admin-layout/components/AddNewButton.tsx (3)
8-8: LGTM! Added useApp import for dynamic prefix.The import correctly adds
useApphook to access the application's dynamic prefix.
26-26: LGTM! Initialized app instance for prefix access.Getting the app instance enables access to the dynamic prefix property.
38-38: LGTM! Navigation path uses dynamic prefix.The navigation URL construction now uses
/${app.prefix}/instead of hardcoded/admin/, making the routing configurable.packages/client/src/collection-manager/CollectionHistoryProvider.tsx (3)
6-6: LGTM! Added useApp import for dynamic prefix.The import correctly adds
useApphook to access the application's dynamic prefix.
21-21: LGTM! Initialized app instance for prefix access.Getting the app instance enables access to the dynamic prefix property for admin page detection.
44-44: LGTM! Admin page detection uses dynamic prefix.The pathname check now uses
/${app.prefix}instead of hardcoded/admin, ensuring admin page detection works with the configurable prefix.packages/client/src/built-in/dynamic-page/index.tsx (1)
22-22: LGTM! Route path correctly updated to use dynamic prefix.The change from
/:entry/:name/*to/${this.app.prefix}/:name/*properly implements the dynamic prefix pattern, removing the parameterized entry segment in favor of the app's configured prefix.packages/client/src/built-in/admin-layout/MenuEditor.tsx (2)
6-6: LGTM! Added necessary import for dynamic prefix usage.The
useAppimport is correctly added to support the dynamic prefix functionality throughout the component.
25-87: Excellent refactoring! All hardcoded admin paths replaced consistently.All route matching, navigation, and URL construction have been properly updated to use
/${app.prefix}instead of hardcoded/adminpaths. The changes maintain the existing logic while making the routing dynamic and configurable.packages/client/src/application/Application.tsx (1)
110-116: LGTM! AdminUrl properly refactored to use dynamic prefix.The
adminUrlgetter correctly usesthis.prefixinstead of hardcoded 'admin' strings, providing consistency with the broader refactoring effort.packages/client/src/built-in/index.tsx (1)
349-372: LGTM! Route definitions properly updated to use dynamic prefix.All route paths have been correctly updated to use
${this.app.prefix}instead of hardcoded paths, maintaining consistency with the refactoring effort.
| subApp ? ( | ||
| <Button key="back" onClick={() => (window.location.href = '/admin')}> | ||
| // TODO 提供默认跳转的页面，即到对应页面后，会触发前端跳转 | ||
| <Button key="back" onClick={() => (window.location.href = `/admin`)}> |
There was a problem hiding this comment.
Fix remaining hardcoded admin reference.
There's still a hardcoded /admin reference in the error handling code that should be updated for consistency with the prefix refactoring.
Apply this fix to use the dynamic prefix:
-<Button key="back" onClick={() => (window.location.href = `/admin`)}>
+<Button key="back" onClick={() => (window.location.href = `/${app.prefix}`)}>Note: You'll need to access the app instance in this context. Consider using the useApp() hook similar to other components.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/client/src/built-in/index.tsx at line 110, replace the hardcoded
'/admin' URL with a dynamic prefix by using the useApp() hook to get the current
app instance and its prefix. Update the onClick handler to set
window.location.href to the prefix plus '/admin' instead of the fixed string.
This ensures consistency with the prefix refactoring across the codebase.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
apps/web/src/index.tsx(1 hunks)packages/client/src/application/Application.tsx(3 hunks)packages/client/src/application/Plugin.ts(1 hunks)packages/client/src/application/PluginManager.ts(0 hunks)packages/client/src/application/components/AppComponent.tsx(1 hunks)packages/client/src/hooks/useAdminSchemaUid.ts(1 hunks)packages/sdk/src/APIClient.ts(1 hunks)
💤 Files with no reviewable changes (1)
- packages/client/src/application/PluginManager.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/client/src/application/Application.tsx
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: CI
🔇 Additional comments (5)
apps/web/src/index.tsx (2)
1-1: LGTM: React import is necessary for StrictMode usage.The explicit React import is required for using
React.StrictModecomponent, which aligns with the changes made in the render method.
10-14: Excellent addition of React StrictMode!Wrapping the App component in
React.StrictModeis a great improvement that will:
- Help identify potential problems during development
- Warn about deprecated React features and unsafe lifecycle methods
- Enable additional development-time checks and warnings
This change aligns with React best practices and will improve the development experience without affecting production performance.
packages/client/src/application/components/AppComponent.tsx (1)
24-26: LGTM! Excellent improvement to prevent redundant loading.The conditional guard ensures
app.load()is only called when necessary, preventing redundant operations and potential race conditions. This is a good defensive programming practice.packages/client/src/hooks/useAdminSchemaUid.ts (1)
1-6: LGTM! Good refactoring to use dynamic configuration.Replacing the hardcoded string with
app.indexSchemaaligns perfectly with the PR objective to use dynamic prefixes instead of hardcoded values. The implementation correctly uses theuseApp()hook to access the application context.packages/client/src/application/Plugin.ts (1)
6-9: LGTM! Clean constructor simplification using TypeScript parameter properties.The refactoring leverages TypeScript's parameter property syntax to automatically create class properties, eliminating boilerplate code while maintaining the same functionality. The
protectedaccess modifiers ensure proper encapsulation.
| if (typeof actionName === 'symbol') { | ||
| console.log('-----', _, actionName); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove debug logging and improve symbol handling.
The symbol check is a good defensive practice, but the console.log appears to be debug code that should be removed before production.
if (typeof actionName === 'symbol') {
- console.log('-----', _, actionName);
- return;
+ // Symbols are not valid action names, return undefined to prevent processing
+ return undefined;
}Consider adding a comment explaining why symbols are filtered out, as this helps future maintainers understand the defensive behavior against framework-internal symbol property accesses.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (typeof actionName === 'symbol') { | |
| console.log('-----', _, actionName); | |
| return; | |
| } | |
| if (typeof actionName === 'symbol') { | |
| // Symbols are not valid action names, return undefined to prevent processing | |
| return undefined; | |
| } |
🤖 Prompt for AI Agents
In packages/sdk/src/APIClient.ts around lines 314 to 317, remove the console.log
debug statement inside the symbol type check to clean up production code. Retain
the symbol check to defensively filter out framework-internal symbol property
accesses, and add a clear comment explaining this purpose for future
maintainers.
Summary by CodeRabbit
New Features
Refactor
/adminreferences with the new dynamic prefix throughout the user interface.Chores