Conversation
Summary of ChangesHello, 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 robust user management capabilities to the console application, empowering administrators with a dedicated interface to oversee and control user accounts. It includes a new navigation entry, a dynamic user listing page, and a detailed user profile view that facilitates administrative actions like banning and role assignment. The changes also include an improvement to the underlying data table hook for more efficient parameter handling. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
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
|
There was a problem hiding this comment.
Pull request overview
Adds an Admin “Users” management surface to the console using better-auth’s admin client plugin, including routing, navigation, and new pages for listing users and viewing/updating user status.
Changes:
- Enable
better-authadmin plugin in the console auth client and introduce a localUsertype. - Add
/admin/usersroute + sidebar navigation entry, and implement Users page, list view, and detail/action sheet. - Refactor
useServerDataTableto support setting multiple params at once viasetParams.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| console/src/utils/authClient.ts | Enables adminClient() plugin and defines an extended User type used by the admin UI. |
| console/src/routers/index.tsx | Registers the new /admin/users route under the authenticated area. |
| console/src/pages/admin/users.tsx | Adds the top-level Admin Users page wrapper. |
| console/src/pages/admin/list-users.tsx | Implements server-driven user listing table and selection to open a detail sheet. |
| console/src/pages/admin/user-detail-sheet.tsx | Implements user detail display and admin actions (ban/unban, role change). |
| console/src/hooks/useServerDataTable.ts | Refactors param updates to a multi-field setParams helper. |
| console/src/components/common/layout/menu-bar.tsx | Adds “Admin → Users” entry to the sidebar navigation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| { | ||
| path: 'admin', | ||
| children: [ | ||
| { | ||
| path: 'users', | ||
| element: <Users />, | ||
| }, | ||
| ], | ||
| }, |
There was a problem hiding this comment.
The new /admin/users route is only protected by ProtectedRoute (authentication) and does not enforce an admin role. As a result, any logged-in user can reach the admin UI and trigger admin API calls. Consider adding an explicit role-based guard for the admin route subtree (redirect/403) rather than relying on API failures.
| { | ||
| title: 'Admin', | ||
| url: '#', | ||
| items: [ | ||
| { | ||
| title: 'Users', | ||
| icon: <User />, | ||
| url: '/admin/users', | ||
| }, | ||
| ], | ||
| }, |
There was a problem hiding this comment.
The sidebar always shows the new "Admin → Users" navigation item without checking the current user's role. This exposes admin functionality to non-admin users (leading to confusing UX at best, and unnecessary surface area). Consider conditionally rendering this section based on the session user's role or a dedicated permissions check.
![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 user management feature, including a user list page and a user detail view with admin actions, along with new routes and components. A critical security vulnerability has been identified: the new 'Admin' menu section and the /admin/users route lack proper role-based access control (RBAC) on the frontend, making them accessible to any authenticated user and exposing sensitive data. Additionally, the review highlighted areas for improvement in the new components, including suboptimal initial loading state handling in list-users.tsx, incomplete query invalidation in user-detail-sheet.tsx which could lead to stale data, and a minor issue with accessing a user's name for the avatar fallback.
| path: 'admin', | ||
| children: [ | ||
| { | ||
| path: 'users', | ||
| element: <Users />, | ||
| }, | ||
| ], | ||
| }, |
There was a problem hiding this comment.
The '/admin/users' route is protected only by 'ProtectedRoute', which typically only verifies authentication. There is no check for the 'admin' role, allowing any logged-in user to navigate to the user management page. This could lead to the exposure of sensitive user information (PII) and administrative actions to unauthorized users. Access to the '/admin' route and its children should be restricted to users with the 'admin' role, for example by using a specialized 'AdminRoute' component or by passing role requirements to the 'ProtectedRoute'.
| title: 'Admin', | ||
| url: '#', | ||
| items: [ | ||
| { | ||
| title: 'Users', | ||
| icon: <User />, | ||
| url: '/admin/users', | ||
| }, | ||
| ], | ||
| }, |
There was a problem hiding this comment.
The 'Admin' menu section is rendered for all authenticated users without checking if they have the 'admin' role. This exposes administrative functionality in the UI to unauthorized users. While backend enforcement may prevent unauthorized actions, exposing these menu items to non-admin users violates the principle of least privilege and increases the attack surface. It is recommended to conditionally render this section only for users with administrative privileges by checking the user's role from the session.
| }, | ||
| onSuccess: () => { | ||
| toast.success(`User has been ${aUser?.banned ? 'unbanned' : 'banned'}.`); | ||
| return queryClient.invalidateQueries({ queryKey: ['user', aUser?.id] }); |
There was a problem hiding this comment.
After successfully updating a user's status (ban/unban), the list of users on the main page should be updated to reflect this change. Currently, only the specific user query (['user', aUser?.id]) is invalidated. You should also invalidate the users list query to ensure the UI is consistent.
queryClient.invalidateQueries({ queryKey: ['users'] });
return queryClient.invalidateQueries({ queryKey: ['user', aUser?.id] });
| }, | ||
| onSuccess: () => { | ||
| toast.success('User role updated.'); | ||
| return queryClient.invalidateQueries({ queryKey: ['user', aUser?.id] }); |
There was a problem hiding this comment.
After successfully updating a user's role, the list of users on the main page should be updated to reflect this change. Currently, only the specific user query (['user', aUser?.id]) is invalidated. You should also invalidate the users list query to ensure the UI is consistent.
queryClient.invalidateQueries({ queryKey: ['users'] });
return queryClient.invalidateQueries({ queryKey: ['user', aUser?.id] });
| alt={aUser.name} | ||
| /> | ||
| <AvatarFallback className="text-3xl"> | ||
| {aUser.name[0]} |
There was a problem hiding this comment.
Accessing aUser.name[0] might be unsafe if aUser.name is an empty string. While React can render undefined without crashing, it's better to handle this case explicitly to avoid unexpected behavior in the UI and provide a fallback. Using optional chaining ?. and a fallback character is a safer approach.
{aUser.name?.[0] || '?'}
* feat(asset): add tls filter for asset * fix(core): fix asset test * fix(asset): fix based on bot reviews * fix(console): fix small typo in tls tab * fix(console): add missing tls for queryParams in asset context * fix(console): fix tls query hook in dashboard --------- Co-authored-by: Quang Vinh <32523515+l1ttps@users.noreply.github.com>
Bumps [multer](https://github.com/expressjs/multer) from 2.0.2 to 2.1.0. - [Release notes](https://github.com/expressjs/multer/releases) - [Changelog](https://github.com/expressjs/multer/blob/main/CHANGELOG.md) - [Commits](expressjs/multer@v2.0.2...v2.1.0) --- updated-dependencies: - dependency-name: multer dependency-version: 2.1.0 dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
* refactor(targets): combine logic create target in transaction * feat(targets): add bulk target creation endpoint * feat(targets): add bulk creation support
Co-authored-by: Quang Vinh <32523515+l1ttps@users.noreply.github.com>
…nd re-design settings ui (#299) * feat(console): add all workspaces navigation and improve 404 page UI * refactor(layout): extract header into dedicated HeaderBar component * refactor(console): add workspace-aware header layout * refactor(console): convert workspace creation to page and add route protection * refactor(console): update workspaces UI from table to card layout * feat(workspaces): add member and target counts to workspace list * refactor(settings): reorganize settings page with sidebar layout * feat(settings): add API keys management * refactor(settings): improve API key display layout * fix(screenshot-cell): add type assertion for screenshotPath * refactor(workspaces): use workspace selector hook
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| LayoutDashboard, | ||
| SquareTerminal, | ||
| Target, | ||
| User, |
There was a problem hiding this comment.
WARNING: Imported User icon but admin route lacks proper protection
The Admin section with Users route is added to the sidebar menu, but there's no indication that this route is protected by authentication or authorization middleware. Anyone could potentially access /admin/users without proper credentials.
| @@ -48,6 +49,17 @@ export function AppSidebar({ ...props }: React.ComponentProps<typeof Sidebar>) { | |||
| }, | |||
There was a problem hiding this comment.
CRITICAL: Admin section route /admin/users missing authentication protection
The new Admin > Users menu item links to /admin/users but there's no evidence this route is protected by authentication middleware. This could allow unauthenticated access to user management functionality.
| @@ -48,6 +49,17 @@ export function AppSidebar({ ...props }: React.ComponentProps<typeof Sidebar>) { | |||
| }, | |||
| ], | |||
There was a problem hiding this comment.
CRITICAL: Admin section route /admin/users missing authorization check
Even if authenticated, there's no indication that only admin users can access the /admin/users route. Regular users could potentially access user management functionality.
| @@ -48,6 +49,17 @@ export function AppSidebar({ ...props }: React.ComponentProps<typeof Sidebar>) { | |||
| }, | |||
| ], | |||
| }, | |||
There was a problem hiding this comment.
WARNING: Admin section route /admin/users missing role-based validation
The route should validate that the user has the appropriate role (admin) before granting access to user management features.
| }, | ||
| ], | ||
| }, | ||
| { |
There was a problem hiding this comment.
WARNING: Admin section route /admin/users missing permission check
Beyond role validation, there should be specific permission checks for user management actions (view, create, edit, delete users).
| [setParams], | ||
| ), | ||
| setFilter: useCallback( | ||
| (v: string) => { |
There was a problem hiding this comment.
SUGGESTION: setFilter should implement debouncing
For search/filter functionality, consider adding debounce to prevent excessive API calls while user is typing.
| setParam('filter', v); | ||
| } | ||
| if (filter === v) return; | ||
| setParams({ |
There was a problem hiding this comment.
SUGGESTION: Consider resetting page on filter change
When filter changes, it's common practice to reset to page 1 since the filtered results may have different pagination. This is already implemented, which is good.
| const formSchema = z.object({ | ||
| name: z.string().min(1, 'Name is required'), | ||
| email: z.string().min(1, 'Email is required').email('Invalid email address'), | ||
| password: z.string().min(8, 'Password must be at least 8 characters'), |
There was a problem hiding this comment.
SUGGESTION: Add password confirmation field for better UX and security
The password field lacks a confirmation field which is standard practice for user creation to prevent typos. Consider adding a 'confirmPassword' field with validation that it matches the password field.
| <FormItem> | ||
| <FormLabel>Password</FormLabel> | ||
| <FormControl> | ||
| <Input |
There was a problem hiding this comment.
SUGGESTION: Consider showing password strength indicator
For better user experience, consider adding a password strength indicator to help users create stronger passwords.
| <FormControl> | ||
| <Input | ||
| type="password" | ||
| placeholder="Min. 8 characters" |
There was a problem hiding this comment.
SUGGESTION: Add 'show password' toggle for accessibility
Consider adding a toggle to show/hide the password field content for better accessibility and user experience.
Code Review SummaryStatus: 15 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
WARNING
SUGGESTION
Other Observations (not in diff)
Files Reviewed (12 files)
|
No description provided.