Merged
Conversation
Introduced a new /users page for admins to approve or reject pending user signups. Includes GraphQL queries/mutations for pending and rejected users, custom hooks, and a MANAGE_USERS authorization action gated to the ADMIN role. Added "Users" nav link visible only to admins. Co-Authored-By: Claude <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds a user management feature: a Users nav item gated by a new MANAGE_USERS permission, a /users route serving a PendingUsers page, GraphQL queries/mutations for pending/rejected users, hooks to fetch them, and approve/reject actions wired to UI mutations and refetching. Changes
Sequence Diagram(s)sequenceDiagram
participant Admin as Admin (Browser)
participant UI as PendingUsers UI
participant Hook as usePendingUsers / useRejectedUsers
participant GraphQL as GraphQL API
participant DB as Database
Admin->>UI: Click "Approve" / "Reject"
UI->>Hook: call mutation (APPROVE_USER / REJECT_USER)
Hook->>GraphQL: send mutation
GraphQL->>DB: update user status
DB-->>GraphQL: persist result
GraphQL-->>Hook: mutation response
Hook-->>UI: resolve, call refetch()
UI->>Hook: refetch PENDING_USERS / REJECTED_USERS
Hook->>GraphQL: send query (PENDING_USERS / REJECTED_USERS)
GraphQL->>DB: query lists
DB-->>GraphQL: return lists
GraphQL-->>Hook: query response
Hook-->>UI: render updated lists
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/hooks/useRejectedUsers.hook.ts (1)
17-21:refetchreturn type loses the Promise.According to the Apollo Client docs,
useQuery'srefetchhas the signature(variables?: Partial<TVariables>) => Promise<ApolloQueryResult<TData>>. Declaring it as() => voidin the public hook signature is technically valid TypeScript (void widening), but callers can never chain on orawaitthe returned promise if they need to. The same applies tousePendingUsers.♻️ Proposed fix
-import { useQuery } from '@apollo/client'; +import { ApolloQueryResult, useQuery } from '@apollo/client'; ... export function useRejectedUsers(): { loading: boolean; data: { rejectedUsers: RejectedUser[] } | undefined; - refetch: () => void; + refetch: () => Promise<ApolloQueryResult<{ rejectedUsers: RejectedUser[] }>>; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/useRejectedUsers.hook.ts` around lines 17 - 21, The hook signature for useRejectedUsers (and similarly usePendingUsers) declares refetch as () => void which loses the Promise returned by Apollo's useQuery; update the public return type so refetch matches useQuery's signature, e.g. refetch: (variables?: Partial<any>) => Promise<ApolloQueryResult<{ rejectedUsers: RejectedUser[] }>> (import ApolloQueryResult) or use the correct generic TVariables/TData types used by useQuery, ensuring callers can await or chain the returned promise; change the refetch type in the exported function return to the Promise-based signature and mirror the same fix in usePendingUsers.src/hooks/usePendingUsers.hook.ts (1)
12-16: Samerefetch: () => voidnarrowing concern asuseRejectedUsers.For consistency, consider updating this hook's return type to expose the full
Promise<ApolloQueryResult<...>>return just like the suggested fix foruseRejectedUsers.hook.ts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/usePendingUsers.hook.ts` around lines 12 - 16, The hook usePendingUsers currently types refetch as a void-returning function which narrows callers; change its return type so refetch matches the underlying Apollo client's signature (Promise<ApolloQueryResult<{ pendingUsers: PendingUser[] }>>). Update the exported return type for usePendingUsers to expose loading, data: { pendingUsers: PendingUser[] } | undefined, and refetch: (variables?: Partial<Record<string, any>>) => Promise<ApolloQueryResult<{ pendingUsers: PendingUser[] }>> (or the exact ApolloQueryResult generic used elsewhere) so it is consistent with useRejectedUsers and callers can await the query result.src/pages/users/PendingUsers.tsx (1)
15-16: No in-flight guard on action buttons — duplicate mutations possible.The
loadingstate fromuseMutationis discarded (only the mutate function is destructured on Lines 15–16). While a mutation is in flight, all Approve and Reject buttons remain enabled, so a quick double-click fires the same mutation twice.♻️ Proposed fix — capture loading state and disable buttons
- const [approveUser] = useMutation(APPROVE_USER); - const [rejectUser] = useMutation(REJECT_USER); + const [approveUser, { loading: approveLoading }] = useMutation(APPROVE_USER); + const [rejectUser, { loading: rejectLoading }] = useMutation(REJECT_USER);Then pass
disabledto each button:- <Button onClick={() => handleApprove(user.id, refetchPending)}>Approve</Button> - <Button danger onClick={() => handleReject(user.id)}> + <Button disabled={approveLoading || rejectLoading} onClick={() => handleApprove(user.id, refetchPending)}>Approve</Button> + <Button danger disabled={approveLoading || rejectLoading} onClick={() => handleReject(user.id)}>- <Button onClick={() => handleApprove(user.id, refetchRejected)}>Approve</Button> + <Button disabled={approveLoading || rejectLoading} onClick={() => handleApprove(user.id, refetchRejected)}>Approve</Button>Also applies to: 54-55, 96-96
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/users/PendingUsers.tsx` around lines 15 - 16, Capture each mutation's loading flag from useMutation (e.g., const [approveUser, { loading: approving }] = useMutation(APPROVE_USER) and const [rejectUser, { loading: rejecting }] = useMutation(REJECT_USER)) and pass the appropriate disabled prop to the corresponding Approve and Reject buttons in the PendingUsers component so the button is disabled while its mutation is in-flight; if you need per-row locking, derive a per-user in-flight map keyed by user id when invoking approveUser/rejectUser and use that map to disable only that row's buttons.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/app.tsx`:
- Line 66: The /users Route currently uses the generic USE_APP guard (which
allows plain USER) so any authenticated user can reach <PendingUsers />; change
the route to require the MANAGE_USERS permission instead — e.g., replace or wrap
the Route entry for path='/users' (element={<PendingUsers />}) so it is rendered
only under the authorization guard/component that checks for MANAGE_USERS (not
USER/USE_APP), or add an explicit permission check before rendering
<PendingUsers />; ensure the guard/prop you use matches your app's auth helpers
(MANAGE_USERS, USE_APP, Route/Authorized wrapper) so only users with
MANAGE_USERS can access the page.
In `@src/pages/users/PendingUsers.tsx`:
- Around line 18-27: handleApprove and handleReject currently call
approveUser/rejectUser without error handling so network/server failures are
swallowed; wrap each mutation call (approveUser and rejectUser) in try/catch,
await the mutation inside the try, call
refetch()/refetchPending()/refetchRejected() only on success, and in the catch
set/display an error to the admin (e.g., via component state, a toast, or an
alert) using the caught error.message; alternatively, check and handle the error
field returned from the useMutation tuple after the await and surface that to
the user so failures are not silent.
---
Nitpick comments:
In `@src/hooks/usePendingUsers.hook.ts`:
- Around line 12-16: The hook usePendingUsers currently types refetch as a
void-returning function which narrows callers; change its return type so refetch
matches the underlying Apollo client's signature (Promise<ApolloQueryResult<{
pendingUsers: PendingUser[] }>>). Update the exported return type for
usePendingUsers to expose loading, data: { pendingUsers: PendingUser[] } |
undefined, and refetch: (variables?: Partial<Record<string, any>>) =>
Promise<ApolloQueryResult<{ pendingUsers: PendingUser[] }>> (or the exact
ApolloQueryResult generic used elsewhere) so it is consistent with
useRejectedUsers and callers can await the query result.
In `@src/hooks/useRejectedUsers.hook.ts`:
- Around line 17-21: The hook signature for useRejectedUsers (and similarly
usePendingUsers) declares refetch as () => void which loses the Promise returned
by Apollo's useQuery; update the public return type so refetch matches
useQuery's signature, e.g. refetch: (variables?: Partial<any>) =>
Promise<ApolloQueryResult<{ rejectedUsers: RejectedUser[] }>> (import
ApolloQueryResult) or use the correct generic TVariables/TData types used by
useQuery, ensuring callers can await or chain the returned promise; change the
refetch type in the exported function return to the Promise-based signature and
mirror the same fix in usePendingUsers.
In `@src/pages/users/PendingUsers.tsx`:
- Around line 15-16: Capture each mutation's loading flag from useMutation
(e.g., const [approveUser, { loading: approving }] = useMutation(APPROVE_USER)
and const [rejectUser, { loading: rejecting }] = useMutation(REJECT_USER)) and
pass the appropriate disabled prop to the corresponding Approve and Reject
buttons in the PendingUsers component so the button is disabled while its
mutation is in-flight; if you need per-row locking, derive a per-user in-flight
map keyed by user id when invoking approveUser/rejectUser and use that map to
disable only that row's buttons.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/NavigationBar.tsxsrc/app.tsxsrc/hooks/usePendingUsers.hook.tssrc/hooks/useRejectedUsers.hook.tssrc/pages/users/PendingUsers.tsxsrc/queries/user.tssrc/services/authorization.policy.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/app.tsx (1)
40-40: Optional: computecanManageUsersonce to avoid the duplicateuserCanPerformActioncall.
userCanPerformAction(user, 'MANAGE_USERS')is evaluated twice — once for theNavigationBarprop and once for the route guard — each render cycle. Extracting it to a local variable keeps both usages in sync and makes future changes (e.g. memoising) a one-liner.♻️ Proposed refactor
+ const canManageUsers = userCanPerformAction(user, 'MANAGE_USERS'); + return ( <div className='min-h-screen flex flex-col'> <NavigationBar ... - canManageUsers={userCanPerformAction(user, 'MANAGE_USERS')} + canManageUsers={canManageUsers} /> ... - {userCanPerformAction(user, 'MANAGE_USERS') && <Route path='/users' element={<PendingUsers />} />} + {canManageUsers && <Route path='/users' element={<PendingUsers />} />}Also applies to: 66-66
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app.tsx` at line 40, Compute the permission once and reuse it: create a local constant (e.g. const canManageUsers = userCanPerformAction(user, 'MANAGE_USERS')) and replace both places that call userCanPerformAction (the NavigationBar prop canManageUsers and the route guard check around line ~66) to use that constant; if desired later, wrap the computation in useMemo keyed on user to memoize.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/app.tsx`:
- Line 40: Compute the permission once and reuse it: create a local constant
(e.g. const canManageUsers = userCanPerformAction(user, 'MANAGE_USERS')) and
replace both places that call userCanPerformAction (the NavigationBar prop
canManageUsers and the route guard check around line ~66) to use that constant;
if desired later, wrap the computation in useMemo keyed on user to memoize.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Introduced a new /users page for admins to approve or reject
pending user signups. Includes GraphQL queries/mutations for
pending and rejected users, custom hooks, and a MANAGE_USERS
authorization action gated to the ADMIN role. Added "Users"
nav link visible only to admins.
Co-Authored-By: Claude noreply@anthropic.com
Summary by CodeRabbit