Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
WalkthroughRefactors user deletion by extracting GraphQL documents and UI pieces into separate files, adds an error boundary and confirmation dialog, centralizes mutation error handling, updates Apollo SSE link/header handling and adds createErrorLink, and tweaks navigation selection and tests. Changes
Sequence Diagram(s)sequenceDiagram
participant AdminUI as Admin UI
participant UserDelete as UserDelete Component
participant Apollo as Apollo Client (SSELink)
participant GraphQL as GraphQL Server / API
Note over AdminUI,GraphQL: User deletion flow (check -> confirm -> subscription updates)
AdminUI->>UserDelete: enter user identifier & click "Check"
UserDelete->>Apollo: mutation UserDeleteCheck(idType,id,...)
Apollo->>GraphQL: HTTP mutation request
GraphQL-->>Apollo: check response (counts, items to delete/transfer)
Apollo-->>UserDelete: return check result
UserDelete->>AdminUI: show results and open Confirm Dialog
AdminUI->>UserDelete: click "Delete Permanently"
UserDelete->>Apollo: mutation UserDeleteJourneysConfirm(userId)
Apollo->>GraphQL: execute confirm mutation
GraphQL-->>Apollo: confirm ack
UserDelete->>Apollo: subscribe UserDeleteConfirmSubscription(...) via SSELink
Apollo->>GraphQL: open SSE subscription (headers via factory, query via print)
GraphQL-->>Apollo: stream logs/updates (logs, done, success)
Apollo-->>UserDelete: forward streamed updates
UserDelete->>AdminUI: render logs/status until done
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 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 docstrings
🧪 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 |
csiyang
changed the base branch from
main
to
siyangcao/nes-1454-deleteuser-backend
Contributor
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
|
I see you added the "on stage" label, I'll get this merged to the stage branch! |
Contributor
|
Merge conflict attempting to merge this into stage. Please fix manually. |
Contributor
|
Merge conflict attempting to merge this into stage. Please fix manually. |
csiyang
commented
Apr 14, 2026
Contributor
Author
csiyang
left a comment
csiyang
left a comment
There was a problem hiding this comment.
Review summary
Conventions loaded: .claude/rules/frontend/apps.md, .claude/rules/backend/apis.md, apps/journeys-admin/AGENTS.md
Critical: None
Concern (1 item):
apps/journeys-admin/src/components/UserDelete/UserDelete.tsx:198—<Suspense>has nofallbackprop. During the initial GET_ME query,useSuspenseQuerywill suspend the component and render nothing (blank area). Should show a loading indicator.
Nit:
- Lines 551 and 609:
onClick={handleCheck}andonClick={handleConfirmDelete}pass async functions directly as event handlers, while theonKeyDownhandler on line 543 correctly usesvoid handleCheck(). Both onClick handlers should use the same pattern:onClick={() => void handleCheck()}/onClick={() => void handleConfirmDelete()}.
Positive:
- Clean 4-step delete flow with clear separation between the check phase and the delete phase
- Step 1 logs are preserved even when Step 2 fails — good UX
- Trust boundary is explicitly documented with a comment in
userDeleteConfirm.ts - Server-side superAdmin guard in
getServerSideProps(defense in depth alongside the client-side check) - Lazy
isSuperAdmininauthScopes.tsavoids an extra DB roundtrip on every authenticated request - Good error recovery — both ApolloError and unknown errors are handled in the catch blocks
confirmLoadingcorrectly covers the gap between clicking Delete and the subscription starting- Accessibility attributes (
aria-label,aria-describedby) on form inputs
csiyang
commented
Apr 14, 2026
Contributor
Author
csiyang
left a comment
csiyang
left a comment
There was a problem hiding this comment.
Round 2 review — all findings resolved
All items from the previous review pass:
- Suspense fallback ✅ —
<Suspense fallback={<CircularProgress />}>added at line 199;CircularProgressimported from@mui/material/CircularProgress - Async onClick handlers ✅ — both
handleCheckandhandleConfirmDeletenow use the() => void fn()pattern, consistent with theonKeyDownhandler
No new issues introduced by the fix commit.
Critical: None
Concern: None
Overall: Ready to merge.
Contributor
|
The latest updates on your projects.
|
Contributor
|
The latest updates on your projects.
|
Contributor
|
The latest updates on your projects.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/journeys-admin/src/components/UserDelete/UserDeleteErrorBoundary.tsx (1)
42-46: Extract a named props type for the fallback component.The inline
{ error: Error | null }shape makes this the only component in the file that skips the repo’s{ComponentName}Propsconvention. Pulling it intoUserDeleteErrorFallbackPropskeeps the new component aligned with the rest ofjourneys-admin.As per coding guidelines, "
**/*.{ts,tsx}: Define a type if possible" and "Props interfaces must follow naming convention {ComponentName}Props".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/journeys-admin/src/components/UserDelete/UserDeleteErrorBoundary.tsx` around lines 42 - 46, Create a named props type for the fallback component by extracting the inline `{ error: Error | null }` into a new type called UserDeleteErrorFallbackProps and update the UserDeleteErrorFallback signature to accept `props: UserDeleteErrorFallbackProps`; ensure the new type is exported or defined adjacent to UserDeleteErrorFallback so the file follows the {ComponentName}Props convention used across the repo and keeps the component consistent with other types in journeys-admin.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/journeys-admin/src/components/UserDelete/UserDelete.tsx`:
- Around line 397-404: The ARIA attributes currently passed into the MUI
slotProps.input in the UserDelete component are not applied to the native
<input>; update the slotProps to use slotProps.htmlInput instead so aria-label
and aria-describedby reach the actual DOM node: locate the slotProps block in
the UserDelete component (where idType === UserDeleteIdType.email ? t('User
email to delete') : t('Database ID to delete') is used) and move the
'aria-label' and 'aria-describedby' properties under slotProps.htmlInput
(preserving the conditional value and key names), ensuring accessibility
attributes are applied to the underlying input element.
---
Nitpick comments:
In `@apps/journeys-admin/src/components/UserDelete/UserDeleteErrorBoundary.tsx`:
- Around line 42-46: Create a named props type for the fallback component by
extracting the inline `{ error: Error | null }` into a new type called
UserDeleteErrorFallbackProps and update the UserDeleteErrorFallback signature to
accept `props: UserDeleteErrorFallbackProps`; ensure the new type is exported or
defined adjacent to UserDeleteErrorFallback so the file follows the
{ComponentName}Props convention used across the repo and keeps the component
consistent with other types in journeys-admin.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: da15a207-e630-4c6d-9898-212bb5c38db7
📒 Files selected for processing (8)
apps/journeys-admin/pages/users/verify.tsxapps/journeys-admin/src/components/UserDelete/UserDelete.spec.tsxapps/journeys-admin/src/components/UserDelete/UserDelete.tsxapps/journeys-admin/src/components/UserDelete/UserDeleteConfirmDialog.tsxapps/journeys-admin/src/components/UserDelete/UserDeleteErrorBoundary.tsxapps/journeys-admin/src/libs/apolloClient/apolloClient.test.tsapps/journeys-admin/src/libs/apolloClient/apolloClient.tslibs/locales/en/apps-journeys-admin.json
✅ Files skipped from review due to trivial changes (1)
- libs/locales/en/apps-journeys-admin.json
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/journeys-admin/src/components/UserDelete/UserDelete.spec.tsx
- apps/journeys-admin/src/libs/apolloClient/apolloClient.test.ts
- apps/journeys-admin/src/components/UserDelete/UserDeleteConfirmDialog.tsx
- apps/journeys-admin/src/libs/apolloClient/apolloClient.ts
… type
- UserDelete.tsx: change slotProps.input → slotProps.htmlInput so that
aria-label and aria-describedby reach the underlying <input> DOM node
in MUI v7; slotProps.input targets the MUI wrapper component, not the
native element, so screen readers were not receiving these attributes
(CodeRabbit major accessibility finding)
- UserDeleteErrorBoundary.tsx: extract inline { error: Error | null }
type into named UserDeleteErrorFallbackProps interface, consistent
with the repo's {ComponentName}Props convention (CodeRabbit nitpick)
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
slotProps.htmlInput applies aria-label directly to the DOM <input>,
which takes precedence over the associated <label> text for accessible
name computation. Tests that queried getByRole('textbox', { name: 'User
Email' }) now get no match because the accessible name is the aria-label
value 'User email to delete'. Update all three occurrences.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
mikeallisonJS
approved these changes
Apr 15, 2026
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
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
3 participants
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.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor