feat(profile): Delete My Account button + flow#2288
Conversation
Self-service account deletion (V1). A stroke-styled button in Settings
opens a scary confirmation modal; on confirm it POSTs to /users/me/delete
(disables the account + starts the 30-day data-deletion clock), shows the
'deleted within 30 days' notice, then clears the local session and signs
the user out — they can no longer log back in (login is blocked server-side).
Fires delete_account_{initiated,confirmed,failed} PostHog events.
…tton - move requestDeletion onto usersApi (it's a /users endpoint) and drop the one-method accountApi service - collapse isOpen+phase into a single modalState enum (kills the impossible closed+done state), extract the duplicated modal-lock condition, drop the unused catch binding
The /settings route is an orphan (nothing in the app links to it), so the delete button was effectively invisible. Move it under 'Log out' on the Profile screen — the real account surface users reach via the home avatar — and revert the dead /settings edit.
Swap the tiny alert icon for the animated Peanut mascots — sad peanut on
the 'are you sure?' confirm step, crying peanut on the goodbye step — and
warm up the copy/CTAs ('Never mind, I'll stay' / 'Goodbye'). Keeps the
honest bits: permanent, disabled now, data wiped within 30 days.
Drop the stroke button chrome for a plain underlined red text link, a softer/standard treatment for a destructive secondary action.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reached
More reviews will be available in 12 minutes and 52 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughAdds account-deletion support across analytics, API handling, modal UI, profile integration, and username validation. ChangesAccount deletion flow
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
Code-analysis diffPainscore total: 5764.4 → 5772.67 (+8.27) 🆕 New findings (9)
✅ Resolved (7)
📈 Painscore deltas (top movers)
|
🧪 UI test report — ✅ all greenSuites
📊 Coverage (unit)
⏱ 10 slowest test cases
|
Emojis aren't part of the design system.
ValidatedUsernameWrapper validated via HEAD, which still reports a deleted user's username as existing (kept that way to reserve the name). Switch the recipient validator to GET — that route now 404s for deactivated accounts — so the public profile + /send + /request render the 'we don't know any @username' view for deleted users. Signup's availability check is a separate HEAD call, so username reuse stays blocked.
|
@coderabbitai full review |
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/lib/validation/recipient.ts (1)
85-88: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winCollapse the username validation and lookup into one fetch.
After this GET succeeds, Line 47 still performs another user lookup, so the happy path now does two sequential network calls. Since this method only returns a boolean, the payload from the first GET is thrown away. Consider reusing the fetched user here or letting
getByUsernamedrive the validation path directly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/validation/recipient.ts` around lines 85 - 88, The username validation path is making two sequential network calls because verifyPeanutUsername fetches the user and the happy path still does a separate lookup afterward. Update verifyPeanutUsername and the related getByUsername flow in recipient.ts so the initial serverFetch result is reused for validation instead of discarded, or have getByUsername perform the validation directly, keeping the method behavior boolean-only without the extra fetch.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/validation/recipient.ts`:
- Line 87: The username lookup in recipient validation is building a URL path
segment directly from raw input, which can be altered by characters like /, ? or
%. Update the serverFetch call in recipient handling to encode the username path
segment with encodeURIComponent before interpolating it. Use the recipient
lookup branch and the username variable as the key place to make this change so
arbitrary input is treated as a single path segment.
---
Nitpick comments:
In `@src/lib/validation/recipient.ts`:
- Around line 85-88: The username validation path is making two sequential
network calls because verifyPeanutUsername fetches the user and the happy path
still does a separate lookup afterward. Update verifyPeanutUsername and the
related getByUsername flow in recipient.ts so the initial serverFetch result is
reused for validation instead of discarded, or have getByUsername perform the
validation directly, keeping the method behavior boolean-only without the extra
fetch.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a165d025-baa2-412f-bef0-7659cbc31e06
📒 Files selected for processing (1)
src/lib/validation/recipient.ts
CodeRabbit: guard the username path segment against /, ?, % in arbitrary input.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/Settings/DeleteAccountButton.tsx`:
- Around line 53-59: The DeleteAccountButton flow can trap the user in a
non-dismissible “done” modal if logout fails because finish() calls logoutUser({
skipBackendCall: true }) without guaranteeing navigation and lockModal blocks
dismissal whenever modalState is 'done'. Update the DeleteAccountButton logic so
a failed logout path can recover, either by not locking the modal until logout
succeeds or by clearing/rewinding modalState when logoutUser reports an error;
use finish(), lockModal, and the authContext logoutUser behavior as the key
places to adjust.
- Around line 37-49: Add a synchronous re-entry guard to confirmDelete so it
exits immediately if a deletion is already in progress, instead of relying only
on setIsSubmitting in DeleteAccountButton. Check the current submitting state at
the start of confirmDelete before calling posthog.capture or
usersApi.requestDeletion, and return early on a repeated click to prevent
duplicate DELETE /users/me/delete requests and duplicate
ANALYTICS_EVENTS.DELETE_ACCOUNT_CONFIRMED events. Keep the existing
try/catch/finally flow unchanged otherwise.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f270f33b-d5a5-4422-8a34-9834f14d36d1
📒 Files selected for processing (6)
src/components/Profile/index.tsxsrc/components/Settings/DeleteAccountButton.tsxsrc/constants/analytics.consts.tssrc/lib/validation/recipient.tssrc/services/__tests__/users.test.tssrc/services/users.ts
| const confirmDelete = async () => { | ||
| setIsSubmitting(true) | ||
| posthog.capture(ANALYTICS_EVENTS.DELETE_ACCOUNT_CONFIRMED) | ||
| try { | ||
| await usersApi.requestDeletion() | ||
| setModalState('done') | ||
| } catch { | ||
| posthog.capture(ANALYTICS_EVENTS.DELETE_ACCOUNT_FAILED) | ||
| toast.error('Could not delete your account. Please try again.') | ||
| } finally { | ||
| setIsSubmitting(false) | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Add a synchronous re-entry guard to confirmDelete().
Line 38 only schedules the disabled state for the next render, so a fast double-click can still fire POST /users/me/delete twice and emit duplicate delete_account_confirmed events. Guard the handler itself before sending the request.
Suggested fix
const confirmDelete = async () => {
+ if (isSubmitting) return
setIsSubmitting(true)
posthog.capture(ANALYTICS_EVENTS.DELETE_ACCOUNT_CONFIRMED)
try {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const confirmDelete = async () => { | |
| setIsSubmitting(true) | |
| posthog.capture(ANALYTICS_EVENTS.DELETE_ACCOUNT_CONFIRMED) | |
| try { | |
| await usersApi.requestDeletion() | |
| setModalState('done') | |
| } catch { | |
| posthog.capture(ANALYTICS_EVENTS.DELETE_ACCOUNT_FAILED) | |
| toast.error('Could not delete your account. Please try again.') | |
| } finally { | |
| setIsSubmitting(false) | |
| } | |
| } | |
| const confirmDelete = async () => { | |
| if (isSubmitting) return | |
| setIsSubmitting(true) | |
| posthog.capture(ANALYTICS_EVENTS.DELETE_ACCOUNT_CONFIRMED) | |
| try { | |
| await usersApi.requestDeletion() | |
| setModalState('done') | |
| } catch { | |
| posthog.capture(ANALYTICS_EVENTS.DELETE_ACCOUNT_FAILED) | |
| toast.error('Could not delete your account. Please try again.') | |
| } finally { | |
| setIsSubmitting(false) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/Settings/DeleteAccountButton.tsx` around lines 37 - 49, Add a
synchronous re-entry guard to confirmDelete so it exits immediately if a
deletion is already in progress, instead of relying only on setIsSubmitting in
DeleteAccountButton. Check the current submitting state at the start of
confirmDelete before calling posthog.capture or usersApi.requestDeletion, and
return early on a repeated click to prevent duplicate DELETE /users/me/delete
requests and duplicate ANALYTICS_EVENTS.DELETE_ACCOUNT_CONFIRMED events. Keep
the existing try/catch/finally flow unchanged otherwise.
| const finish = () => { | ||
| logoutUser({ skipBackendCall: true }) | ||
| } | ||
|
|
||
| // Once submitting (or on the final notice) the modal can't be dismissed — | ||
| // the user must complete the flow through the CTA. | ||
| const lockModal = isSubmitting || modalState === 'done' |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
The locked “done” state has no recovery path if logout fails.
logoutUser({ skipBackendCall: true }) is not guaranteed to redirect; src/context/authContext.tsx:241-274 catches failures and only shows a toast. Because lockModal stays true for every 'done' state, a logout error leaves the user stuck behind a non-dismissible modal with the same failing CTA.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/Settings/DeleteAccountButton.tsx` around lines 53 - 59, The
DeleteAccountButton flow can trap the user in a non-dismissible “done” modal if
logout fails because finish() calls logoutUser({ skipBackendCall: true })
without guaranteeing navigation and lockModal blocks dismissal whenever
modalState is 'done'. Update the DeleteAccountButton logic so a failed logout
path can recover, either by not locking the modal until logout succeeds or by
clearing/rewinding modalState when logoutUser reports an error; use finish(),
lockModal, and the authContext logoutUser behavior as the key places to adjust.
|
@coderabbitai review in depth. Evaluate:
|
QA testing guide — Delete My Account (V1)Setup: a logged-in test account (note its username). Needs peanut-api-ts#1067 deployed. Happy path
Cancel path
Invisibility (check from a second account or logged-out browser)
Guardrails
Reactivation (ops/support)
Out of scope (V1): actual 30-day data purge (future), and an existing session's token still works until it expires (re-login is what's blocked). |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== Changed files =="
gh pr view 2288 --json files --jq '.files[].path'
echo
echo "== Outline: src/components/Settings/DeleteAccountButton.tsx =="
ast-grep outline src/components/Settings/DeleteAccountButton.tsx --view expanded || true
echo
echo "== src/components/Settings/DeleteAccountButton.tsx =="
sed -n '1,220p' src/components/Settings/DeleteAccountButton.tsx
echo
echo "== src/components/Profile/index.tsx =="
sed -n '1,220p' src/components/Profile/index.tsx
echo
echo "== src/services/users.ts =="
sed -n '1,220p' src/services/users.ts
echo
echo "== src/services/__tests__/users.test.ts =="
sed -n '1,220p' src/services/__tests__/users.test.ts
echo
echo "== src/lib/validation/recipient.ts =="
sed -n '1,220p' src/lib/validation/recipient.ts
echo
echo "== src/constants/analytics.consts.ts =="
sed -n '1,220p' src/constants/analytics.consts.ts
echo
echo "== Search logoutUser definition/usages =="
rg -n -C3 '\blogoutUser\b' src
echo
echo "== Search ActionModal definition/usages =="
rg -n -C2 'ActionModal' src/components src/hooks src/lib src
echo
echo "== Search requestDeletion / usersApi patterns =="
rg -n -C3 '\brequestDeletion\b|\bserverFetch\s*\(' src/services src
echo
echo "== Search verifyPeanutUsername usage =="
rg -n -C3 '\bverifyPeanutUsername\s*\(' srcLength of output: 222427
Overall assessmentRisk of new bugs: moderate
Main concernIn
That means username resolution now becomes GET + GET to the same Why this matters
RecommendationConsider collapsing this into a single fetch:
That would reduce latency and avoid duplicate API traffic. Delete-account flow reviewFor Good
Residual risk
So the delete feature is probably okay, but its behavioral coverage is thinner than the recipient-validation impact area. Performance impact
Bottom lineIf I had to rank the risks in this PR:
So I’d call this safe-ish for the account deletion feature, but with a meaningful performance/DRY regression in recipient validation that’s worth fixing before merge. |
…tests CodeRabbit in-depth review follow-ups: - validateAndResolveRecipient no longer does verifyPeanutUsername (GET) then getByUsername (GET) on the same resource — one getByUsername now both validates (404 = invalid/deleted) and resolves, removing a duplicate round-trip on the send/request hot path. - add DeleteAccountButton state-machine tests: confirm->success->done->logout, error toast on failure, and cancel.
|
Thanks @coderabbitai. Addressed in 711deaa:
Note: @coderabbitai review |
|
Your note about I’ll review the updated changes now. ✏️ Learnings added
✅ Action performedReview finished.
|
|
@coderabbitai re-review in depth. Evaluate:
|
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== PR metadata =="
gh pr view 2288 --json number,title,baseRefName,headRefName,files,commits --jq '{
number, title, baseRefName, headRefName,
files: [.files[].path],
commits: [.commits[].oid[0:10]]
}'
echo
echo "== Changed files =="
gh pr view 2288 --json files --jq '.files[].path'
echo
echo "== Latest diff summary =="
git diff --stat "$(gh pr view 2288 --json baseRefName --jq .baseRefName)"...HEAD
echo
echo "== Find recipient-validation symbols =="
rg -n -C3 '\bvalidateAndResolveRecipient\b|\bverifyPeanutUsername\b|\bgetByUsername\b|\bValidatedUsernameWrapper\b|\bPublicProfile\b' src
echo
echo "== Outline: src/lib/validation/recipient.ts =="
ast-grep outline src/lib/validation/recipient.ts --view expanded || true
echo
echo "== src/lib/validation/recipient.ts =="
sed -n '1,240p' src/lib/validation/recipient.ts
echo
echo "== Locate validateAndResolveRecipient implementation =="
rg -n -C5 '\bvalidateAndResolveRecipient\b' src
echo
echo "== Outline: src/components/Settings/DeleteAccountButton.tsx =="
ast-grep outline src/components/Settings/DeleteAccountButton.tsx --view expanded || true
echo
echo "== src/components/Settings/DeleteAccountButton.tsx =="
sed -n '1,260p' src/components/Settings/DeleteAccountButton.tsx
echo
echo "== Find DeleteAccountButton tests =="
rg -n -C3 'DeleteAccountButton|requestDeletion|logoutUser|toast' src --iglob '*test*' --iglob '*spec*'
echo
echo "== src/services/__tests__/users.test.ts =="
sed -n '1,240p' src/services/__tests__/users.test.ts
echo
echo "== Any component tests near DeleteAccountButton =="
fd -i 'DeleteAccountButton' src
echo
echo "== src/services/users.ts =="
sed -n '1,220p' src/services/users.ts
echo
echo "== src/components/Profile/index.tsx =="
sed -n '1,220p' src/components/Profile/index.tsx
echo
echo "== logoutUser definition/usages =="
rg -n -C4 '\blogoutUser\b' src
echo
echo "== ActionModal definition/usages =="
rg -n -C3 '\bActionModal\b' src/components src/hooks src/lib srcLength of output: 3457 🏁 Script executed: #!/bin/bash
set -u
echo "== PR diff (file headers only) =="
gh pr diff 2288 --patch | sed -n '1,260p' || true
echo
echo "== Changed files =="
gh pr view 2288 --json files --jq '.files[].path' || true
echo
echo "== Outline: src/lib/validation/recipient.ts =="
ast-grep outline src/lib/validation/recipient.ts --view expanded || true
echo
echo "== src/lib/validation/recipient.ts =="
sed -n '1,260p' src/lib/validation/recipient.ts || true
echo
echo "== Locate validateAndResolveRecipient implementation/usages =="
rg -n -C5 '\bvalidateAndResolveRecipient\b' src || true
echo
echo "== Search verifyPeanutUsername/getByUsername call sites =="
rg -n -C3 '\bverifyPeanutUsername\s*\(|\bgetByUsername\s*\(' src || true
echo
echo "== Outline: src/components/Settings/DeleteAccountButton.tsx =="
ast-grep outline src/components/Settings/DeleteAccountButton.tsx --view expanded || true
echo
echo "== src/components/Settings/DeleteAccountButton.tsx =="
sed -n '1,280p' src/components/Settings/DeleteAccountButton.tsx || true
echo
echo "== src/components/Settings/__tests__/DeleteAccountButton.test.tsx =="
sed -n '1,320p' src/components/Settings/__tests__/DeleteAccountButton.test.tsx || true
echo
echo "== src/services/users.ts =="
sed -n '1,220p' src/services/users.ts || true
echo
echo "== src/services/__tests__/users.test.ts =="
sed -n '1,240p' src/services/__tests__/users.test.ts || true
echo
echo "== src/components/Profile/index.tsx =="
sed -n '1,240p' src/components/Profile/index.tsx || true
echo
echo "== logoutUser definition/usages =="
rg -n -C4 '\blogoutUser\b' src || true
echo
echo "== ActionModal definition/usages =="
rg -n -C4 '\bActionModal\b' src || trueLength of output: 166448
Overall assessmentThis looks materially better than the previous revision.
What improved1. Hot-path duplicate username fetch is fixedIn That removes the main risk I called out before:
So on the recipient-validation side, this is now a real improvement, not just a neutral change. 2. Delete flow state handling is cleaner
That’s a nice cleanup:
3. Service placement is more coherentMoving deletion onto 4. UI coverage is much better
That meaningfully reduces risk around the new feature. Residual riskI only see one notable remaining gap, and it’s relatively small: Modal lock behavior still isn’t truly testedThe test file mocks
So while the component code looks right, the tests currently validate the state machine, not the real modal-dismissal mechanics. I’d classify that as:
Performance / latencyNet result: better than before
So I no longer see a meaningful perf concern in this PR. Code quality / DRYCurrent state looks good:
I don’t see a new DRY regression in the updated patch. Bottom lineI’d now call this PR:
The only remaining caution I’d keep in mind is the test gap around actual modal non-dismissability, but I would not treat that as merge-blocking from what I can see here. ✅ Action performedFull review finished. |
CodeRabbit re-review nit: the modal mock now surfaces preventClose + hideModalCloseButton as data-attrs, and a new test asserts the modal is unlocked on confirm and locked during submit and on the done step.
1 participant
What & why
Frontend for self-service Delete My Account (TASK-20092). Pairs with peanut-api-ts#1067 (backend endpoint + login guard). V1 disables the account and shows a "deleted within 30 days" notice.
Changes
usersApi.requestDeletion()(src/services/users.ts) →POST /users/me/delete.DeleteAccountButton(src/components/Settings/): a red underlined text link (text-error) that opens a two-stepActionModal:/settingsis an orphan route).delete_account_{initiated,confirmed,failed}.Tests
Service test
src/services/__tests__/users.test.ts(happy path + error). Local: typecheck clean, jest green (the one failing suite —countryCurrencyMappingflag-asset coverage — is a pre-existing env artifact:public/flags/*.svgare build-generated, unrelated to this PR).Cross-repo
Requires peanut-api-ts#1067 deployed for the endpoint + login block.
🤖 Generated with Claude Code