feature: add permission control to navbar #221
Conversation
|
@potter-sun is attempting to deploy a commit to the Anto Subash's projects Team on Vercel. A member of the Team first needs to authorize it. |
potter-sun
changed the title
There was a problem hiding this comment.
Pull Request Overview
This PR implements permission-based access control for navigation menu items to prevent non-admin users from encountering errors when accessing restricted pages like roles and users.
Key Changes:
- Added permission policies to menu configuration items with automatic filtering based on user permissions
- Integrated permission checking in both desktop and mobile navigation components
- Removed unused import from RoleList component
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/src/config.ts | Added policy fields to AdminMenus configuration with specific permissions for each menu item |
| src/src/components/role/RoleList.tsx | Removed unused RolePermission import |
| src/src/components/navbar/side-nav-bar.tsx | Implemented permission filtering logic for desktop navigation menu |
| src/src/components/navbar/side-nav-bar-mobile.tsx | Implemented permission filtering logic for mobile navigation menu |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Menu without policy or with permission is visible | ||
| return true | ||
| }) | ||
| }, [can]) |
There was a problem hiding this comment.
The useMemo dependency array is incomplete. It should include 'AdminMenus' to recompute when the menu configuration changes. Add AdminMenus to the dependencies: }, [can, AdminMenus])
| }, [can]) | |
| }, [can, AdminMenus]) |
| // Menu without policy or with permission is visible | ||
| return true | ||
| }) | ||
| }, [can]) |
There was a problem hiding this comment.
The useMemo dependency array is incomplete. It should include 'AdminMenus' to recompute when the menu configuration changes. Add AdminMenus to the dependencies: }, [can, AdminMenus])
| }, [can]) | |
| }, [can, AdminMenus]) |
| const pathname = usePathname() | ||
| const { can } = useGrantedPolicies() | ||
| const currentUser = useCurrentUser() | ||
| const isAdmin = currentUser?.roles?.includes(USER_ROLE.ADMIN) ?? false |
There was a problem hiding this comment.
The isAdmin variable is computed but never used in the component logic. It only appears in the JSX for displaying the admin badge. Consider removing it if the badge is purely cosmetic, or use it in the permission logic if admin users should bypass permission checks.
| const isAdmin = currentUser?.roles?.includes(USER_ROLE.ADMIN) ?? false |
| const sessionData = useSession() | ||
| const { can } = useGrantedPolicies() | ||
| const currentUser = useCurrentUser() | ||
| const isAdmin = currentUser?.roles?.includes(USER_ROLE.ADMIN) ?? false |
There was a problem hiding this comment.
The isAdmin variable is computed but never used in the component logic. It only appears in the JSX for displaying the admin badge. Consider removing it if the badge is purely cosmetic, or use it in the permission logic if admin users should bypass permission checks.
| const isAdmin = currentUser?.roles?.includes(USER_ROLE.ADMIN) ?? false |
1 participant
#220
Non-admin users encounter errors when viewing roles and users after logging in.
Should non-admin users have their viewing permissions controlled via the UI?
Fixed by AI