feat: allow configuration of staticRroues #4746
Conversation
|
WalkthroughThe changes in this pull request primarily focus on updating the routing guard functionality across multiple files in the web application. The key modification involves replacing the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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
🧹 Outside diff range and nitpick comments (9)
apps/web-antd/src/router/routes/index.ts (3)
13-13: Add implementation instructions in the comment.The commented import for static routes would benefit from additional documentation explaining how to implement static routes, similar to the dynamic routes pattern.
-// const staticRouteFiles = import.meta.glob('./static/**/*.ts', { eager: true }); +// Static routes can be enabled by: +// 1. Uncomment this line +// 2. Create a 'static' folder under 'routes' +// 3. Add your static route files following the pattern in 'modules' +// const staticRouteFiles = import.meta.glob('./static/**/*.ts', { eager: true });
18-19: Consider using English for consistency.While the Chinese comment is clear, using English would maintain consistency with the codebase and improve international collaboration.
-/** 外部路由列表，访问这些页面可以不需要Layout，可能用于内嵌在别的系统(不会显示在菜单中) */ +/** External route list - pages that don't require Layout, possibly for embedding in other systems (won't show in menu) */
35-35: Consider documenting route priority.The order of routes in
accessRoutesaffects route matching priority. Consider adding a comment explaining why dynamic routes come before static routes.-const accessRoutes = [...dynamicRoutes, ...staticRoutes]; +// Dynamic routes take precedence over static routes to allow for permission-based overrides +const accessRoutes = [...dynamicRoutes, ...staticRoutes];apps/web-ele/src/router/routes/index.ts (1)
13-13: LGTM! Consider enhancing documentation.The commented import follows the established pattern for route modules. However, since this is a new feature for configuring static routes, consider adding a code comment explaining the expected file structure and naming conventions for static route files.
-// const staticRouteFiles = import.meta.glob('./static/**/*.ts', { eager: true }); +// Static route files should be placed in the ./static/ directory +// Example: ./static/about.ts, ./static/landing.ts +// const staticRouteFiles = import.meta.glob('./static/**/*.ts', { eager: true });apps/web-naive/src/router/routes/index.ts (1)
Line range hint
1-36: Consider enhancing the routing architecture documentation.While the current implementation provides a good foundation for static routes, consider adding:
- A README.md in the routes directory explaining:
- The purpose and usage of each route type
- How to configure static routes
- Examples of valid route configurations
- TypeScript interfaces for route configurations
- Route validation utilities
Would you like me to help create this documentation structure?
playground/src/router/routes/index.ts (1)
13-13: Consider translating the Chinese comment to English for consistency.The commented code follows the established pattern, but the comment should be in English to maintain consistency and accessibility for all contributors.
-// const staticRouteFiles = import.meta.glob('./static/**/*.ts', { eager: true }); +// Static route files can be enabled by uncommenting and creating the directory +// const staticRouteFiles = import.meta.glob('./static/**/*.ts', { eager: true });playground/src/router/guard.ts (1)
Line range hint
89-112: Consider enhancing route type safety and separation.The current implementation combines static and dynamic routes in the access control flow. Consider these architectural improvements:
- Add type definitions to distinguish between static and dynamic routes
- Implement separate handling logic for static routes that don't require permission checks
- Add runtime validation to ensure static routes aren't accidentally modified
This will improve maintainability and reduce the risk of security misconfiguration.
apps/web-naive/src/router/guard.ts (1)
107-107: LGTM! Consider enhancing documentation.The change to use
accessRoutesmaintains proper access control while supporting both static and dynamic routes. The implementation correctly handles authorization by checking user roles before generating accessible menus and routes.Consider adding a code comment explaining the distinction between static and dynamic routes in the
accessRoutesconfiguration, as this would help future maintainers understand the routing architecture better.// 生成菜单和路由 const { accessibleMenus, accessibleRoutes } = await generateAccess({ roles: userRoles, router, - // 则会在菜单中显示，但是访问会被重定向到403 + // Routes (both static and dynamic) will be shown in the menu, + // but access will be redirected to 403 if user lacks permission routes: accessRoutes, });apps/web-ele/src/router/guard.ts (1)
Line range hint
89-114: Consider adding documentation for static vs dynamic routes.While the implementation is solid, it would be helpful to add JSDoc comments explaining:
- The distinction between static and dynamic routes
- How the route generation process handles each type
- The expected structure of
accessRoutesThis would improve maintainability and help other developers understand the routing system better.
Example documentation:
/** * Generates accessible routes and menus based on user roles. * @param {Object} params - Generation parameters * @param {string[]} params.roles - User role identifiers * @param {Router} params.router - Vue Router instance * @param {Route[]} params.routes - Combined static and dynamic routes * Static routes: Pre-defined routes that don't change * Dynamic routes: Routes generated based on user permissions * @returns {Promise<{accessibleMenus: Menu[], accessibleRoutes: Route[]}>} */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
- apps/web-antd/src/router/guard.ts (2 hunks)
- apps/web-antd/src/router/routes/index.ts (2 hunks)
- apps/web-ele/src/router/guard.ts (2 hunks)
- apps/web-ele/src/router/routes/index.ts (2 hunks)
- apps/web-naive/src/router/guard.ts (2 hunks)
- apps/web-naive/src/router/routes/index.ts (2 hunks)
- playground/src/router/guard.ts (2 hunks)
- playground/src/router/routes/index.ts (2 hunks)
- pnpm-workspace.yaml (6 hunks)
🔇 Additional comments (19)
apps/web-antd/src/router/routes/index.ts (2)
36-36: Verify the removal of dynamicRoutes export.Removing
dynamicRoutesfrom exports might be a breaking change if other parts of the application depend on it directly.✅ Verification successful
The previous search might have failed due to the alias pattern. Let's search more broadly to find any usage of dynamicRoutes and imports from the router module to verify if this is indeed a breaking change.
Removing dynamicRoutes export is safe
The
dynamicRoutesvariable is only used internally within the routes/index.ts file to constructaccessRoutes(via[...dynamicRoutes, ...staticRoutes]). SinceaccessRoutesis now exported and contains the dynamic routes, removing the directdynamicRoutesexport is safe as there are no direct imports or usage ofdynamicRoutesfound in the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for direct usage of dynamicRoutes import rg "import.*dynamicRoutes.*from.*['\"]@/router/routes['\"]" -g '!*.{md,json}'Length of output: 83
Script:
#!/bin/bash # Search for any usage of dynamicRoutes in the codebase rg "dynamicRoutes" -A 2 -B 2 # Also search for potential aliased imports that might use @/router/routes rg "from ['\"]@/router" -A 1 -B 1Length of output: 3811
20-22: Clarify the static routes implementation approach.The static routes implementation differs from the dynamic routes pattern:
- It uses an empty array instead of
mergeRouteModules- The comment should be in English for consistency
- The relationship between the commented import and empty array initialization needs clarification
Consider one of these approaches:
-/** 不需要权限的菜单列表（会显示在菜单中） */ -// const staticRoutes: RouteRecordRaw[] = mergeRouteModules(staticRouteFiles); -const staticRoutes: RouteRecordRaw[] = []; +/** Static routes that don't require permissions (will show in menu) */ +const staticRoutes: RouteRecordRaw[] = process.env.VITE_USE_STATIC_ROUTES + ? mergeRouteModules(staticRouteFiles) + : [];apps/web-ele/src/router/routes/index.ts (3)
18-21: LGTM! Clear documentation of route types.The comments effectively distinguish between external routes (no Layout, not in menu) and static routes (in menu, no permissions), making the routing system's behavior more transparent.
35-36: Verify guard system compatibility with accessRoutes.The combination of dynamic and static routes into
accessRouteslooks good. However, since this affects the routing guard system, please ensure all guard implementations are updated accordingly.#!/bin/bash # Check if all guard files are updated to use accessRoutes echo "Checking guard implementations..." rg -A 5 "dynamicRoutes" --type ts src/router/guard
22-22: Verify if default empty static routes is intended.While initializing
staticRoutesas an empty array is a safe default, please verify if this aligns with the feature requirements for configuring static routes. Should there be any default static routes provided out of the box?apps/web-naive/src/router/routes/index.ts (2)
35-36: Verify the usage ofaccessRoutesacross the application.The combination of dynamic and static routes in
accessRouteslooks correct, but we should verify that this new export is properly utilized throughout the application.Let's check the usage of accessRoutes:
#!/bin/bash # Description: Verify the usage of accessRoutes across the application # Search for imports of accessRoutes rg "import.*\{.*accessRoutes.*\}" -g "*.{ts,vue}" # Search for direct usage of accessRoutes rg "\baccessRoutes\b" -g "*.{ts,vue}"
13-13: Verify if static route files implementation is complete.The commented import suggests that static route files should be placed in a
staticdirectory, but the implementation appears incomplete. This might not fully satisfy the PR's objective of "allowing configuration of static routes".Let's check if the static route directory and files exist:
✅ Verification successful
Static routes implementation is intentionally commented out and optional
The code review concern can be resolved. The commented import and related code is intentionally left as optional functionality with clear Chinese comments indicating "you can uncomment if needed" (有需要可以自行打开注释). The implementation is complete as:
- Static routes array is properly initialized as empty
const staticRoutes: RouteRecordRaw[] = []- It's correctly integrated into the
accessRoutesexport- The commented code serves as a template for users who want to enable static routes by creating the directory and uncommenting the code
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence of static route files and directory structure # Check if the static routes directory exists fd -t d "static" -p "router/routes" # Look for any .ts files in static routes directories fd -e ts . -p "router/routes/static"Length of output: 2345
Script:
#!/bin/bash # Let's check the complete implementation context around the commented line # Check the content of the routes file cat "apps/web-naive/src/router/routes/index.ts" # Also check if there are any other route-related files in the router directory fd -e ts . -p "router/routes"Length of output: 1403
playground/src/router/routes/index.ts (3)
35-35: Verify route precedence in the combined accessRoutes.The order of route concatenation (dynamic routes before static routes) might affect route matching precedence. Please ensure this order aligns with the intended routing behavior.
#!/bin/bash # Description: Search for route registration and matching logic # Expected: Find route handling code to verify precedence importance echo "Searching for route registration logic..." rg -A 5 "useRouter|createRouter|addRoute" playground/src/
37-37: Breaking Change: Verify all imports using dynamicRoutes.The export signature has changed from
dynamicRoutestoaccessRoutes. This is a breaking change that requires updates in all files importingdynamicRoutes.#!/bin/bash # Description: Find all imports of dynamicRoutes that need updating # Expected: List of files that need to be updated echo "Searching for dynamicRoutes imports..." rg "import.*dynamicRoutes.*from.*routes" playground/src/
18-22: Implementation looks good, verify static routes directory structure.The comments clearly distinguish between different route types, and the empty
staticRoutesarray provides a foundation for future static route configurations.Let's verify the directory structure for consistency:
✅ Verification successful
Route structure is properly organized and consistent with the codebase design
The route directory structure is well-organized with:
- Core routes in
core.ts- Dynamic route modules in
modules/directory containing dashboard, demos, examples, and vben routes- Empty static routes array aligns with the codebase design as all routes are either core routes or dynamic module routes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if the routes directory structure is consistent # Expected: Should show modules/ directory and potentially static/ directory echo "Checking route directory structure..." ls -la playground/src/router/Length of output: 239
Script:
#!/bin/bash # Let's try a more comprehensive approach using fd to find files and directories echo "Checking route-related files and directories..." fd . playground/src/router/ --type f --type d # Let's also check for any route-related imports echo -e "\nChecking route-related imports..." rg "import.*Route" playground/src/router/ -A 2 # Check for route configuration patterns echo -e "\nChecking route configurations..." ast-grep --pattern 'const $_ : RouteRecordRaw[] = $_'Length of output: 123327
playground/src/router/guard.ts (2)
106-106: Review the permission handling for static routes.The
generateAccessfunction now processes both dynamic and static routes throughaccessRoutes. Ensure that static routes are properly handled in the permission checking logic.Let's verify the route handling implementation:
#!/bin/bash # Description: Verify the route handling implementation # Test 1: Check the generateAccess function implementation ast-grep --pattern 'function generateAccess($_) { $$$ }' # Test 2: Check how static routes are defined and handled rg -A 5 "staticRoutes.*=" "playground/src/router/"
11-11: Verify the import change impact.The change from
dynamicRoutestoaccessRoutesreflects the architectural shift to support both dynamic and static routes.Let's verify the export exists and maintains backward compatibility:
✅ Verification successful
Let me gather more information to verify the import changes.
Import change is correctly implemented
The verification shows that:
accessRoutesis properly exported fromplayground/src/router/routes/index.ts- It combines both
dynamicRoutesandstaticRoutesas intended:const accessRoutes = [...dynamicRoutes, ...staticRoutes]- The import in
guard.tscorrectly references this new combined routes array- The architectural change maintains backward compatibility while adding support for static routes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the exports from routes module # Test 1: Check if accessRoutes is properly exported rg -l "export.*accessRoutes" "playground/src/router/routes/" # Test 2: Check if there are any remaining references to dynamicRoutes rg "dynamicRoutes" "playground/src/router/"Length of output: 6689
Script:
#!/bin/bash # Check if accessRoutes is exported and its implementation rg -A 5 'export.*accessRoutes' playground/src/router/routes/ # Check the implementation of routes/index.ts to understand the routes structure rg -A 10 'const routes.*=' playground/src/router/routes/Length of output: 3955
apps/web-naive/src/router/guard.ts (1)
11-11: Verify consistent usage ofaccessRoutesacross the codebase.The change from
dynamicRoutestoaccessRoutesappears to be part of a larger architectural change. Let's verify that this change is consistently applied across all router configurations.✅ Verification successful
The change from
dynamicRoutestoaccessRoutesis correctly implemented across the codebaseThe verification shows a consistent pattern across all applications (web-naive, web-antd, web-ele, playground):
dynamicRoutesis only used internally within route configuration files (routes/index.ts) to constructaccessRoutesaccessRoutesis correctly imported and used in all guard files (guard.ts)- The architecture follows a clear pattern where
accessRoutes = [...dynamicRoutes, ...staticRoutes]🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining references to dynamicRoutes and verify accessRoutes usage # Test 1: Check for any remaining references to dynamicRoutes echo "Checking for remaining dynamicRoutes references..." rg "dynamicRoutes" -g "!*.{md,json,lock}" # Test 2: Verify accessRoutes usage in router configurations echo "Verifying accessRoutes usage in router files..." rg "accessRoutes" --type ts -g "**/router/*"Length of output: 1802
apps/web-antd/src/router/guard.ts (3)
Line range hint
40-122: Implementation maintains security and follows best practices.The changes to support static routes are well-integrated into the existing access control system. The implementation:
- Preserves role-based access control
- Maintains proper access token validation
- Keeps the clean separation of concerns in route processing
108-108: Ensure static routes maintain proper access control.The inclusion of static routes in
accessRoutespassed togenerateAccessis good for security as it ensures all routes go through access control. However, please ensure:
- Static routes that should be publicly accessible are properly marked
- Documentation is updated to reflect the new route configuration options
#!/bin/bash # Description: Check for route configuration documentation and security markers # Look for documentation updates fd README.md --exec grep -l "route" {} \; fd "docs" --exec grep -l "route" {} \; # Check for security-related route configurations ast-grep --pattern 'meta: { $$$ ignoreAccess: true, $$$ }'
11-11: Verify the route configuration structure change.The switch from
dynamicRoutestoaccessRoutesindicates a significant change in route management. Let's verify the structure of these routes to ensure backward compatibility.✅ Verification successful
Let me try different commands to verify the route configuration changes.
Based on the search results, I can now provide a final response:
The route configuration change is consistent and properly implemented.
The switch from
dynamicRoutestoaccessRoutesis a well-structured change whereaccessRoutesis composed of both dynamic and static routes:const accessRoutes = [...dynamicRoutes, ...staticRoutes]. This maintains backward compatibility as it still includes all dynamic routes while adding support for static routes, making it a safe enhancement to the routing system.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the structure and usage of route configurations # Check the route configuration structure ast-grep --pattern 'export const accessRoutes = $$$' # Check for any remaining references to dynamicRoutes rg 'dynamicRoutes' -lLength of output: 192
Script:
#!/bin/bash # Search for route configuration files and their content rg -l "routes\.(ts|js)" --type-add 'routes:*.{ts,js}' # Check the definition and structure of accessRoutes rg "export const accessRoutes" -A 5 # Look for any route-related type definitions rg "type.*Route" -A 3 # Check for any migration or changes related to dynamicRoutes rg "dynamicRoutes" -C 3Length of output: 22762
apps/web-ele/src/router/guard.ts (2)
108-108: LGTM! Proper integration with access control system.The
generateAccessfunction correctly utilizesaccessRoutesto generate accessible menus and routes based on user roles, maintaining proper access control.
11-11: Verify the import renaming impact across the codebase.The renaming from
dynamicRoutestoaccessRoutesbetter reflects its purpose of handling both static and dynamic routes.Let's verify this change is consistent across the codebase:
✅ Verification successful
The import change is safe and consistently implemented
Based on the verification results:
- All guard files (
apps/web-*/src/router/guard.ts) consistently use the newaccessRoutesimport- The usage pattern is uniform across all implementations, where
accessRoutesis used in the same context for route configuration- No mixed usage of old and new naming was found in the guard files
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining references to dynamicRoutes # and verify consistent usage of accessRoutes echo "Checking for any remaining references to dynamicRoutes:" rg "dynamicRoutes" -l echo "\nVerifying consistent usage of accessRoutes:" rg "accessRoutes" -lLength of output: 873
Script:
#!/bin/bash # Let's check the actual content of route files to understand the usage # and potential conflicts between dynamicRoutes and accessRoutes echo "Checking content of route files for dynamicRoutes and accessRoutes definitions:" for file in $(rg -l "dynamicRoutes|accessRoutes" "**/routes/index.ts"); do echo "\n=== $file ===" rg "export.*(dynamicRoutes|accessRoutes)" -A 2 "$file" done echo "\nChecking guard files for usage patterns:" for file in $(rg -l "dynamicRoutes|accessRoutes" "**/guard.ts"); do echo "\n=== $file ===" rg "(dynamicRoutes|accessRoutes)" -A 2 "$file" doneLength of output: 485
Script:
#!/bin/bash # Let's try with fd to find the correct paths first, then examine the content echo "Finding route and guard files:" fd -t f "routes/index.ts|guard.ts" echo "\nChecking content of route files:" fd -t f "routes/index.ts" --exec rg "export.*(dynamicRoutes|accessRoutes)" -A 2 {} echo "\nChecking guard files:" fd -t f "guard.ts" --exec rg "(dynamicRoutes|accessRoutes)" -A 2 {}Length of output: 1111
Script:
#!/bin/bash # Let's check the routes/index.ts files specifically for both terms # to understand the transition and ensure consistency echo "Checking routes/index.ts files content:" fd -t f "routes/index.ts" --exec sh -c 'echo "\n=== {} ==="; cat {}' \;Length of output: 161
pnpm-workspace.yaml (1)
24-25: LGTM! Dependency updates follow semantic versioning.All version bumps are either minor or patch updates, maintaining backward compatibility:
- Minor updates (^9.0.3 → ^9.1.0, etc.) add functionality in a backward-compatible manner
- Patch updates (^4.14.4 → ^4.14.6, etc.) include backward-compatible bug fixes
Let's verify the changes don't introduce breaking changes for the routing feature:
Also applies to: 33-33, 45-45, 52-52, 101-101, 122-122, 124-124, 138-138, 159-159, 167-167, 175-177
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/@core/ui-kit/form-ui/src/form-render/form.vue (1)
Line range hint
84-133: Consider documenting the formFieldProps precedence order.The implementation correctly merges
formFieldPropsfrom multiple sources (global config, common config, and schema-specific config). Consider adding a comment to document the precedence order of these configurations to help future maintainers understand the override behavior.Add a comment like this before the computed property:
/** * Computes the final schema with merged configurations. * formFieldProps precedence order (highest to lowest): * 1. Individual schema formFieldProps * 2. Common component config * 3. Global common config */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- packages/@core/ui-kit/form-ui/src/form-render/form.vue (1 hunks)
🔇 Additional comments (1)
packages/@core/ui-kit/form-ui/src/form-render/form.vue (1)
84-87: Well-structured type definition enhancement!The updated return type properly separates
formFieldPropswhile maintaining type safety through the use of TypeScript'sOmitutility. This change provides better type checking and prevents potential property conflicts.
* feat: allow configuration of staticRroues * chore: update deps * chore: typo
Description
close #4665
Type of change
Please delete options that are not relevant.
pnpm-lock.yamlunless you introduce a new test example.Checklist
pnpm run docs:devcommand.pnpm test.feat:,fix:,perf:,docs:, orchore:.Summary by CodeRabbit
Release Notes
New Features
staticRoutesfor routes that do not require permissions.accessRoutesfor better route management based on user roles.Bug Fixes
Chores
Enhancements