feat(mobile): add in-app permanent account deletion - #481
Conversation
Greptile SummaryThis PR adds permanent in-app account deletion to the mobile account screen, including server-directed reauthentication and local teardown.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| apps/mobile/src/runtime/cloud/deletion.ts | Implements requirement lookup, reauthentication dispatch, deletion-result classification, and best-effort local teardown; the previously reported transport-response issue is fixed. |
| apps/mobile/src/runtime/cloud/account.ts | Adds authoritative browser reauthentication checks requiring a new session for the same Cloud user. |
| apps/mobile/src/runtime/cloud/idp.ts | Refactors native Apple authentication to return fresh IdP and authorization credentials for deletion while retaining normal sign-in. |
| apps/mobile/src/components/account/delete-account-section.tsx | Adds confirmation, busy-state handling, outcome-specific messaging, and teardown after accepted deletion. |
| apps/mobile/src/runtime/cloud/tests/deletion.test.ts | Covers deletion requirements, reauthentication branches, response classification, transport failure handling, and teardown behavior. |
Sequence Diagram
sequenceDiagram
actor User
participant App as Mobile App
participant Cloud as LinkCode Cloud
participant IdP as Central IdP / Apple
User->>App: Confirm account deletion
App->>Cloud: GET deletion requirements
Cloud-->>App: native or browser
App->>IdP: Reauthenticate
IdP-->>App: Fresh identity proof
App->>Cloud: DELETE account
alt Request fails or is rejected
Cloud-->>App: Failure
App-->>User: Keep local state and show error
else Deletion accepted
Cloud-->>App: completed or pending
App->>App: Clear sessions, enrollment, and tunnel hosts
App-->>User: Show deletion result
end
Reviews (7): Last reviewed commit: "Update apps/mobile/src/runtime/cloud/acc..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
A thrown fetch means no response ever arrived; report it as a retryable failure instead of tearing down local state on a guess.
|
Pushed a fix for the transport-failure P1 (7343154): a thrown Recorded as D-23 in the task packet (gitignored, local only). Two things from that review thread are deliberately deferred, not fixed here:
|
Matches the linkcodehq-side rename (D-24): the field is a provider-agnostic deletion-completion status, not something mobile or linkcodehq should name after a specific provider.
Verification (in-progress)
1
Probe:
2Wait for test-flight and auth+linkcodehq's staging environment. |
|
@copilot please fix the merge conflicts in this pull request. |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
On the browser re-authentication branch, reauthenticateToCloud() proves the Cloud session changed but never proves it still belongs to the same account — so a system browser signed in as a different LinkCode account leads to DELETE /account permanently deleting the wrong one. Details inline on account.ts.
Reviewed changes — full initial review of the mobile-facing third of CODE-292 (7 commits, 12 files) at 74e5b902; the Cloud and IdP halves live in other repos and were not reviewed.
- New deletion client —
runtime/cloud/deletion.tsreads the server-ownednative/browserrequirement, re-authenticates on that branch, issues oneDELETE /account, and maps the result into a four-variant outcome union with per-stage Sentry tagging. - Local teardown —
runAccountDeletionTeardown()runs cloud sign-out, IdP sign-out, and device-enrollment clearing underPromise.allSettled, then removes only tunnel-derived hosts, preserving direct/LAN profiles. - Shared Apple re-auth core —
idp.tsfactors sign-in and deletion re-auth into a privateauthenticateWithAppleNatively(), adds an Applestateround-trip check, requirescredential.authorizationCode, and introducesIdpTokenAcquisitionError. - Browser re-auth —
account.tsgainsreauthenticateToCloud(), which re-runs the OAuth flow and asserts the authoritative session id changed. - Destructive entry point —
DeleteAccountSectionrenders arole="destructive"button in its ownSectionbelow Sign out, behind a confirm alert, with outcome-specific copy inenandzh-cn. - Dev-stack overrides —
EXPO_PUBLIC_CLOUD_URL/EXPO_PUBLIC_IDP_URLnow override the production Cloud and IdP origins;/tasks/is gitignored.
I ran the new tests locally (pnpm vitest run --project mobile apps/mobile/src/runtime/cloud/__tests__): 18 passed, matching the PR body. I also confirmed three things that looked suspicious but are correct, so nobody re-litigates them: the Apple state round-trip is genuinely supported (expo-apple-authentication types it on AppleAuthenticationSignInOptions, and ios/AppleAuthenticationRequest.swift sets request.state and echoes credential?.state), 'tunnelHostId' in host is a sound discriminant against the HostProfile zod union, and the IdpTokenAcquisitionError wrapping does not defeat isAppleSignInCancel because signInAsync is called before the new try block.
⚠️ Nothing gates this client against a Cloud that does not yet serve these endpoints
GET /account/deletion-requirements and DELETE /account land in linkcodehq#51, and the IdP-side revocation in auth#20. This PR has no feature flag, no capability probe, and no version negotiation, so a mobile build cut before those deploy ships a Delete Account button whose only possible outcome is the requirements-stage failure — the user sees the generic "Could not delete your account. Please try again.", and Sentry collects one account_deletion_stage: requirements report per tap. Worth stating the intended merge/release order explicitly, since the App Store review that motivates CODE-292 will be looking at a shipped binary.
Technical details
# Cross-repo rollout ordering for the deletion endpoints
## Affected sites
- `apps/mobile/src/runtime/cloud/deletion.ts:70-82` — the `requirements` read is
unconditional and its only failure mode is a generic `{ kind: 'failed' }`.
- `apps/mobile/src/components/account/delete-account-section.tsx:73-79` — the button is
always rendered whenever the account screen renders its signed-in subtree.
## Required outcome
- A mobile binary can never reach TestFlight/App Store with a Delete Account button that
the deployed Cloud cannot service.
## Open questions for the human
- Is the intended order "merge + deploy `linkcodehq#51` and `auth#20`, then cut the
mobile build", enforced only by process? If so, say so in the PR description so the
release cut is not a judgement call.
- If mobile can ship first, does the button need to hide itself when the requirements
read returns 404 (endpoint absent) as distinct from 5xx (endpoint down)?⚠️ The new comments cite design documents that this PR makes permanently unreachable
Commit 6147b31b gitignores /tasks/, and the PR description confirms tasks/CODE-292/ is "intentionally untracked" — yet the code added here cites it eight times (design.md §3.4, design.md §3.5, D-5, D-7, D-19, D-23, CODE-292 §3.5). client.ts:12 also points at AGENTS.local.md, which does not exist anywhere in the repo. Root AGENTS.md is explicit on both counts — "No CODE-xxx issue references — traceability belongs in commits and PR descriptions" and "Design rationale longer than two lines belongs in the owning AGENTS.md, not inline" — so these should either move into apps/mobile/AGENTS.md or shrink to the constraint they encode.
Technical details
# Inline comments reference untracked design docs and a nonexistent file
## Affected sites
- `apps/mobile/src/runtime/cloud/client.ts:12` — cites `AGENTS.local.md`; `ls` finds no
such file at the repo root or under `apps/mobile`, and `.gitignore` does not mention it.
- `apps/mobile/src/runtime/cloud/deletion.ts:15` — `CODE-292:` prefix on the module doc.
- `apps/mobile/src/runtime/cloud/deletion.ts:100-103` — "D-19's accepted gap".
- `apps/mobile/src/runtime/cloud/deletion.ts:122-127` — "D-23, reversing the original §3.4 call".
- `apps/mobile/src/runtime/cloud/deletion.ts:186` — "design.md §3.5".
- `apps/mobile/src/runtime/cloud/idp.ts:33`, `:120`, `:130` — `CODE-292 D-7`,
`CODE-292 D-5/D-19`, `CODE-292 §3.5`.
- `apps/mobile/src/components/account/delete-account-section.tsx:45` — "design.md §3.4, TN3194".
## Required outcome
- No comment in the tree points at a document a reader cannot open. The constraints
these comments genuinely encode (why teardown is best-effort, why a lost response is
never reported as accepted, why direct hosts survive) stay discoverable.
## Suggested approach
- Keep the durable rationale — it is good rationale — but move it to
`apps/mobile/AGENTS.md`, which already owns this app's traps, and reduce each inline
comment to the one- or two-line constraint. `TN3194` can stay: it is a stable public
Apple technote, unlike `design.md`.
- Drop the bare `D-nn` / `CODE-292` tokens; the commit messages and this PR body already
carry that traceability.ℹ️ The two new EXPO_PUBLIC_* variables are missing from docs/ENVIRONMENT.md
docs/ENVIRONMENT.md tabulates every other build-time mobile variable (EXPO_PUBLIC_SENTRY_DSN, EXPO_PUBLIC_POSTHOG_PROJECT_TOKEN, EXPO_PUBLIC_POSTHOG_HOST), and root AGENTS.md routes "read, add, or override an environment variable" straight at that file. EXPO_PUBLIC_CLOUD_URL and EXPO_PUBLIC_IDP_URL are documented only in env.d.ts JSDoc and a client.ts comment. These two are more load-bearing than the telemetry ones — an accidentally-set value repoints auth and account deletion at another origin — so the reference table is exactly where they belong.
Technical details
# Document EXPO_PUBLIC_CLOUD_URL and EXPO_PUBLIC_IDP_URL
## Affected sites
- `docs/ENVIRONMENT.md` — build-time mobile table (around lines 81-85) lists every other
`EXPO_PUBLIC_*` variable; these two are absent.
- `apps/mobile/src/env.d.ts:14-17` — declares them.
- `apps/mobile/src/runtime/cloud/client.ts:16` and
`apps/mobile/src/runtime/cloud/idp.ts:16` — read them.
## Required outcome
- Both variables appear in the `docs/ENVIRONMENT.md` mobile build-time table, stating
that they are inlined by Metro/EAS, that unset means production, and that they are for
local `svc dev` stacks only.ℹ️ Nitpicks
deleteInProgresswas added to bothen.ts:1287andzh-cn.ts:1251but has no call site —DeleteAccountSectiononly reflectsbusyviadisabled(busy). Either wire it to a progress affordance (the browser branch involves a full round trip to the system browser, so the button silently greying out is thin feedback) or drop both strings.deletion.ts:14-19statesrunAccountDeletionTeardown"is exported separately only so a retry (best-effort, on next launch/foreground) can re-run just that part." The only non-test call site isdelete-account-section.tsx:38; there is no launch or foreground retry, noAppStatelistener, and no persisted pending-teardown flag. Either build the retry or drop the clause, since as written the comment justifies the export with code that does not exist.deletion.ts:146-147says an unparseable success body is "ambiguous in the same way a network failure is" — but commit73431547deliberately made the network-failure branch returnfailed, notpending. The two comments now contradict each other; the honest distinction is that a 2xx was received here, so acceptance is known rather than guessed.
Claude Opus ｜ 𝕏
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
ℹ️ No critical issues — minor suggestions inline.
The blocking finding from my last review is closed, and I verified the mechanism rather than taking the shape of the fix on trust. One cosmetic nit inline on account.ts.
Reviewed changes — the delta since my review at 9f610fe8: one commit, 5274851 fix(mobile): make deletion reauthentication fail safe.
- Fail-safe sign-out before the mismatch throw —
reauthenticateToCloud()now callscloudAuthClient.signOut()and throws the newCloudAccountMismatchErrorwhenuserIdchanges, so the retry the previous copy invited can no longer land on the other account's session. - New terminal
account-mismatchoutcome —deletion.tsmapsCloudAccountMismatchErrorto its own outcome with dedicateden/zh-cncopy, keeping "a different account signed in" distinguishable from a dismissed browser. Not reported to Sentry, which reads as deliberate — a user with two accounts is a normal event. - Response telemetry widened —
>= 500became!== 409, so a 400/403 contract break now produces aresponse-stage event instead of a silent generic failure. Covered by a new 403 test. authorizationCodeguard folded back into the shared core —reauthenticateWithAppleis now a plain alias ofauthenticateWithAppleNatively(), andidp.test.tsis deleted. That test drovesignInAsyncthrough a mock resolvingauthorizationCode: null, a state the real wrapper rejects before returning (node_modules/expo-apple-authentication/build/AppleAuthentication.js:44-46), so it covered a split that no longer exists — no real coverage lost.
On the fix itself: it is fail-safe by construction, not just on the happy path. cloudAuthClient.signOut() clears the local session before any network I/O — node_modules/@better-auth/expo/dist/client.js:410 calls clearSessionCache() from inside the init plugin hook, and clearSessionCache (client.js:270-279) writes "{}" to both the cookie and the local cache and nulls the session atom. So an offline or failing /sign-out request still cannot leave the other account's cookie behind, and readAuthoritativeSession()'s ?disableCookieCache=true means no residual data can make it observe the wrong identity either. The two prior-round informational items — the Android dead end for native accounts and the cross-repo rollout ordering — are unchanged and still open as product/process decisions; I'm not re-raising them here.
Claude Opus ｜ 𝕏
Co-authored-by: pullfrog[bot] <226033991+pullfrog[bot]@users.noreply.github.com>
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — the delta since my review at 5274851: one commit, ea8878bc Update apps/mobile/src/runtime/cloud/account.ts, which is a single line.
- Mismatch sign-out no longer masks its own outcome —
account.ts:70becameawait cloudAuthClient.signOut().catch(noop), so a rejecting sign-out can no longer replaceCloudAccountMismatchErrorwith a raw error.deletion.ts:105'sinstanceofcheck now always matches, and a cross-account browser re-authentication always terminates in theaccount-mismatchcopy ("A different account was signed in…") rather than the generic "Could not confirm it's you. Please try again."
That is the suggestion from my last review applied verbatim, and it holds up on inspection rather than on shape: foxts/noop exports noop as Noop = (...args: any[]) => any (node_modules/foxts/dist/noop/index.d.ts), so it is a valid catch handler and not one of the foxts helpers whose name misleads; it was already imported at account.ts:2, so the diff adds no import. I ran npx eslint apps/mobile/src/runtime/cloud/account.ts (clean) and npx vitest run --project mobile apps/mobile/src/runtime/cloud/__tests__ (2 files, 23 tests passed) at this head. account.test.ts:84's expect(mocks.signOut).toHaveBeenCalledTimes(1) still pins that sign-out happens before the throw, which is the load-bearing half of the guarantee — the swallowed rejection is a best-effort network call whose local effect (clearSessionCache runs inside the init plugin hook, before any I/O) has already landed.
Nothing else in the tree changed, and all six of my earlier inline threads are resolved. The two standing informational items — the Android dead end for accounts the server marks native, and the cross-repo rollout ordering against linkcodehq#51 / auth#20, which the PR body now states as a release gate — are unchanged product and process decisions, not code findings; I'm not re-raising them.
Claude Opus ｜ 𝕏
Cross-repo context
One-third of CODE-292 (App Store Guideline 5.1.1(v) in-app account deletion). This is the mobile-facing surface: destructive entry point, server-directed re-authentication, one delete mutation, and local teardown.
Companion PRs: linkcodehq#51, auth#20. Design, decisions, and the full verification record live in the local CODE-292 task packet.
Rollout order
Merge and deploy
auth#20andlinkcodehq#51before cutting any TestFlight or App Store build from this PR. This ordering is a release gate: the mobile deletion button must not ship until the deployed Cloud servesGET /account/deletion-requirementsandDELETE /accountand the deployed Auth serves the revocation contract.Summary
deleteAccount()first reads the server-ownednative/browserrequirement; device capability is never treated as an account fact. A device that cannot satisfy a required native Apple flow gets an explicit, non-retryable device message.runAccountDeletionTeardown()clears both local authentication states, device enrollment, and tunnel-derived hosts while preserving direct/LAN hosts.Verification
pnpm check:cipasses.pnpm test: 355 files passed, 1 skipped; 3038 tests passed, 1 skipped.CODE292 JourneyandProbesurvive teardown.DELETE /account.Known gaps (non-blocking)