feat: Add Admin Role Management page (#204)#245
Conversation
- Add AdminRoleManagement page with assign/revoke admin functionality - Implement POST /api/v1/role/create to assign admin role by email - Implement GET /api/v1/role/admins to fetch all admin users - Implement DELETE /api/v1/role/admin/:email to revoke admin access - Add route protection via Protected_Admin component - Add sidebar navigation link with shield icon - Prevent self-deletion of admin access - Include loading states, error handling, and user feedback - Display admin list with name, email, and role information
|
@bharathkumar39293 is attempting to deploy a commit to the code-with-abhishek-kumar's projects Team on Vercel. A member of the Team first needs to authorize it. |
Summary of ChangesHello @bharathkumar39293, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a crucial "Admin Role Management" page, providing administrators with a centralized interface to manage user privileges. The new page facilitates the assignment and revocation of admin roles, displays a comprehensive list of current administrators, and includes essential safeguards like self-deletion protection. It integrates seamlessly into the existing admin dashboard, enhancing the system's administrative capabilities with a user-friendly and robust solution. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new page for admin role management, including functionality to assign, list, and revoke admin roles. The changes are well-structured, with a new route, a sidebar link, and a dedicated component for the page logic. The implementation includes important features like self-deletion protection and user-friendly feedback with loading states and alerts. My review focuses on improving the React implementation within the new AdminRoleManagement component, specifically regarding list rendering, state management for errors, code duplication, and adherence to React Hooks best practices. These suggestions aim to enhance the code's correctness, maintainability, and robustness.
| const [admins, setAdmins] = useState([]); | ||
| const [isLoading, setIsLoading] = useState(false); | ||
| const [isLoadingAdmins, setIsLoadingAdmins] = useState(true); | ||
| const [error, setError] = useState(''); |
There was a problem hiding this comment.
The component uses a single error state for multiple asynchronous operations (fetching admins, assigning a role, and revoking a role). However, the error is only displayed within the 'Assign Admin Role' form (lines 234-236). This can cause confusion. For instance, if fetching the list of admins fails, the error message will incorrectly appear in the assignment form. To provide clearer feedback to the user, consider using separate error states for each operation (e.g., assignError, fetchError, revokeError) and displaying them in their relevant UI sections.
| <tbody> | ||
| {admins.map((admin, index) => ( | ||
| <tr | ||
| key={index} |
There was a problem hiding this comment.
Using the array index as a key for list items is an anti-pattern in React, especially for dynamic lists where items can be added, removed, or reordered. This can lead to unpredictable UI behavior and performance issues. It's recommended to use a stable and unique identifier for each item. In this case, admin.email appears to be a unique identifier and should be used as the key.
| key={index} | |
| key={admin.email} |
| try { | ||
| setIsLoadingAdmins(true); | ||
| setError(''); | ||
| const token = user?.token || localStorage.getItem('token'); |
There was a problem hiding this comment.
The logic to retrieve the authentication token is duplicated across fetchAdmins, handleAssignAdmin, and handleDeleteAdmin. To improve maintainability and adhere to the DRY (Don't Repeat Yourself) principle, consider extracting this into a helper function. This centralizes the token retrieval logic, making future modifications easier.
| } catch (err) { | ||
| setError( | ||
| err?.response?.data?.message || | ||
| err.message || | ||
| 'Failed to fetch admins', | ||
| ); | ||
| Swal.fire({ | ||
| icon: 'error', | ||
| title: 'Error', | ||
| text: | ||
| err?.response?.data?.message || | ||
| err.message || | ||
| 'Failed to fetch admins', | ||
| }); | ||
| } finally { |
There was a problem hiding this comment.
The error handling logic in the catch blocks for fetchAdmins, handleAssignAdmin, and handleDeleteAdmin is nearly identical. This repeated code can be refactored into a single, reusable function. Creating a utility function like handleApiError(err, defaultMessage) that extracts the message and triggers the Swal alert would reduce boilerplate and make the code cleaner and more maintainable.
| useEffect(() => { | ||
| fetchAdmins(); | ||
| }, []); |
There was a problem hiding this comment.
The useEffect hook calls fetchAdmins, but fetchAdmins is not included in the dependency array. Since fetchAdmins is defined within the component body, it is recreated on every render. To follow React hooks best practices and avoid potential stale closure issues, you should wrap fetchAdmins in useCallback and include it in the useEffect dependency array. This ensures fetchAdmins has a stable identity across renders.
|
Here's the code health analysis summary for commits Analysis Summary
|
|
@bharathkumar39293 we Will review this pr after some time |
2 participants
Implemented Features
Added AdminRoleManagement page with:
Assign admin role (POST /api/v1/role/create)
List all admins (GET /api/v1/role/admins)
Revoke admin role (DELETE /api/v1/role/admin/:email)
Added Protected_Admin route guard for access control.
Added sidebar navigation entry (Role Management) with shield icon.
Added self-deletion protection (current admin cannot revoke their own role).
Included loading states, error handling, validation, and success/error alerts.
Auto-refresh admin list after assign/revoke.
Responsive table layout showing name, email, role.
🛠 Other Notes
Follows existing UI/architecture patterns.
Page available at: /admin/role-management.