feat(mobile): add in-app permanent account deletion - #481
Conversation
Greptile SummaryThe PR adds permanent account deletion to mobile account settings, including provider-specific reauthentication, server response classification, and local account teardown.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| apps/mobile/src/runtime/cloud/deletion.ts | Implements deletion requirements, reauthentication dispatch, conservative transport-error handling, response classification, and best-effort local teardown. |
| apps/mobile/src/components/account/delete-account-section.tsx | Adds the confirmed destructive account-deletion UI and maps deletion outcomes to localized user feedback. |
| apps/mobile/src/runtime/cloud/account.ts | Adds browser reauthentication that verifies the sign-in flow replaced the prior cloud session. |
| apps/mobile/src/runtime/cloud/idp.ts | Refactors native Apple authentication to return fresh identity and authorization credentials for deletion while preserving sign-in behavior. |
| apps/mobile/src/runtime/cloud/tests/deletion.test.ts | Covers deletion branching, error classification, Apple credentials, transport failure handling, and idempotent teardown. |
Sequence Diagram
sequenceDiagram
actor User
participant UI as Account settings
participant Auth as Reauthentication
participant Cloud as Cloud API
participant Local as Local device state
User->>UI: Confirm account deletion
UI->>Cloud: Read deletion requirements
Cloud-->>UI: Native Apple or browser
UI->>Auth: Reauthenticate
Auth-->>UI: Fresh identity proof/session
UI->>Cloud: DELETE /account
alt Request rejected or transport fails
Cloud-->>UI: Failed
UI-->>User: Show retryable failure
else Deletion pending or completed
Cloud-->>UI: Accepted outcome
UI->>Local: Sign out and clear account-scoped state
UI-->>User: Show pending or completed result
end
Reviews (4): Last reviewed commit: "fix(mobile): drive deletion reauthentica..." | 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 ｜ 𝕏
| const freshSessionSchema = z.object({ | ||
| session: z.object({ id: z.string().min(1) }), | ||
| }); | ||
|
|
||
| async function getAuthoritativeSessionId(): Promise<string> { | ||
| const { data, error } = await cloudAuthClient.$fetch<unknown>( | ||
| `${CLOUD_URL}/auth/get-session?disableCookieCache=true`, | ||
| {}, | ||
| ); | ||
| if (error) throw new Error(`session read failed (${error.status})`); | ||
| return freshSessionSchema.parse(data).session.id; | ||
| } | ||
|
|
||
| export async function reauthenticateToCloud(): Promise<void> { | ||
| const previousSessionId = await getAuthoritativeSessionId(); | ||
| await signInToCloud(); | ||
| const currentSessionId = await getAuthoritativeSessionId(); | ||
| if (currentSessionId === previousSessionId) { | ||
| throw new Error('browser re-authentication did not create a fresh session'); | ||
| } | ||
| } |
There was a problem hiding this comment.
reauthenticateToCloud proves the session changed, not that it still names the same account. signInToCloud() drives a full OAuth flow through the system browser; if that browser's IdP cookie belongs to a different LinkCode account (account switch, shared device, a second account already signed in to Safari), a fresh session is minted for that account, currentSessionId !== previousSessionId passes, and deleteAccount then issues DELETE /account on the new session — permanently deleting an account the user never saw on screen. The server-side freshness check cannot close this: freshness is about session age, not identity.
The fix is cheap because the data is already in the response you are discarding — better-auth's get-session returns { session, user }, and freshSessionSchema parses only session.id.
Technical details
# Browser re-authentication does not bind the new session to the original identity
## Affected sites
- `apps/mobile/src/runtime/cloud/account.ts:42-44` — `freshSessionSchema` parses
`session.id` and drops `user`.
- `apps/mobile/src/runtime/cloud/account.ts:55-62` — `reauthenticateToCloud` compares only
session ids.
- `apps/mobile/src/runtime/cloud/deletion.ts:98-119` — the browser branch goes straight
from a resolved `reauthenticateToCloud()` to `DELETE ${CLOUD_URL}/account`, with no
identity check in between.
## Evidence that `user` is available
`node_modules/better-auth/dist/api/routes/session.d.mts` types the `get-session`
response as `{ session: Session<...>; user: User<...> } | null` — `user.id` is present in
every 200 response and is simply not parsed today.
## Required outcome
- A completed browser re-authentication that lands on a *different* account must fail as
`reauthentication-failed` and must never reach `DELETE /account`.
- The existing freshness assertion stays: a cancelled flow that reuses the old session
must still fail.
## Suggested approach
Parse `user.id` alongside `session.id` and assert both properties — the session id
changed **and** the user id did not. The outcome union's own doc comment at
`deletion.ts:26-27` already advertises "wrong account" as a `reauthentication-failed`
case, so this makes the type honest.
## Note on tests
`apps/mobile/src/runtime/cloud/__tests__/account.test.ts` mocks `$fetch` as
`{ data: { session: { id: 'old-session' } } }`; widening the schema means those fixtures
need a `user` object. Worth adding a third case that pins the new behaviour: same fresh
session id, *different* `user.id`, expect a rejection — that case fails against the
current implementation, which is the point.| const freshSessionSchema = z.object({ | |
| session: z.object({ id: z.string().min(1) }), | |
| }); | |
| async function getAuthoritativeSessionId(): Promise<string> { | |
| const { data, error } = await cloudAuthClient.$fetch<unknown>( | |
| `${CLOUD_URL}/auth/get-session?disableCookieCache=true`, | |
| {}, | |
| ); | |
| if (error) throw new Error(`session read failed (${error.status})`); | |
| return freshSessionSchema.parse(data).session.id; | |
| } | |
| export async function reauthenticateToCloud(): Promise<void> { | |
| const previousSessionId = await getAuthoritativeSessionId(); | |
| await signInToCloud(); | |
| const currentSessionId = await getAuthoritativeSessionId(); | |
| if (currentSessionId === previousSessionId) { | |
| throw new Error('browser re-authentication did not create a fresh session'); | |
| } | |
| } | |
| const freshSessionSchema = z.object({ | |
| session: z.object({ id: z.string().min(1) }), | |
| user: z.object({ id: z.string().min(1) }), | |
| }); | |
| async function readAuthoritativeSession(): Promise<{ sessionId: string; userId: string }> { | |
| const { data, error } = await cloudAuthClient.$fetch<unknown>( | |
| `${CLOUD_URL}/auth/get-session?disableCookieCache=true`, | |
| {}, | |
| ); | |
| if (error) throw new Error(`session read failed (${error.status})`); | |
| const { session, user } = freshSessionSchema.parse(data); | |
| return { sessionId: session.id, userId: user.id }; | |
| } | |
| export async function reauthenticateToCloud(): Promise<void> { | |
| const before = await readAuthoritativeSession(); | |
| await signInToCloud(); | |
| const after = await readAuthoritativeSession(); | |
| if (after.sessionId === before.sessionId) { | |
| throw new Error('browser re-authentication did not create a fresh session'); | |
| } | |
| if (after.userId !== before.userId) { | |
| throw new Error('browser re-authentication signed in a different account'); | |
| } | |
| } |
| if (!credential.authorizationCode) { | ||
| throw new Error('Apple sign-in returned no authorization code'); | ||
| } |
There was a problem hiding this comment.
This guard sits in the core that signInWithApple also uses, but sign-in never reads authorizationCode — only the deletion flow needs it. Apple types the field optional (Data? in Swift, string | null in the Expo binding) and there are documented cases where it comes back nil next to a perfectly valid identityToken (e.g. a request combining ASAuthorizationAppleIDProvider with ASAuthorizationPasswordProvider). So this converts a value sign-in discards into a hard sign-in failure. Move the requirement to the caller that actually depends on it.
Technical details
# Narrow the authorizationCode precondition to the deletion flow
## Affected sites
- `apps/mobile/src/runtime/cloud/idp.ts:67-69` — throws for all callers of
`authenticateWithAppleNatively`.
- `apps/mobile/src/runtime/cloud/idp.ts:105-117` — `signInWithApple` destructures only
`idpToken`; `authorizationCode` is unused on this path.
- `apps/mobile/src/runtime/cloud/idp.ts:125` — `reauthenticateWithApple` is a bare alias,
so it has no place of its own to assert the stronger precondition today.
## Required outcome
- A successful Apple authorization with a null `authorizationCode` still completes
sign-in, exactly as it did before this PR.
- The deletion flow still refuses to proceed without an `authorizationCode`, since Apple
revocation cannot happen without it.
## Suggested approach
- Type the shared core's return as `authorizationCode: string | null` and drop the guard
from it.
- Replace the `reauthenticateWithApple = authenticateWithAppleNatively` alias with a thin
wrapper that performs the non-null assertion and returns the narrowed type. That also
gives the two flows a place to diverge later (they already want different scopes).
- `deleteAccount`'s `native` branch already treats a throw as `reauthentication-failed`
and tags it, so the failure classification needs no change.
## Reference
Apple's `ASAuthorizationAppleIDCredential.authorizationCode` is declared optional:
<https://developer.apple.com/documentation/authenticationservices/asauthorizationappleidcredential/authorizationcode>| return { kind: 'failed' }; | ||
| } | ||
|
|
||
| if (method === 'native') { |
There was a problem hiding this comment.
When the server answers native, this branch calls reauthenticateWithApple() with no check that the device can actually perform an Apple authorization. sign-in.tsx:28 gates its Apple button on AppleAuthentication.isAvailableAsync() precisely because that is not guaranteed, and app.json configures Android as a target where signInAsync is unavailable outright. An Apple-linked account opened on Android therefore gets a permanent dead end: "Could not confirm it's you. Please try again." on every attempt, plus one account_deletion_stage: native-provider Sentry report each time. Letting the server own the requirement is the right call — but the client still needs a defined answer for "required method is impossible here".
Technical details
# No handling for a server-required native re-auth the device cannot perform
## Affected sites
- `apps/mobile/src/runtime/cloud/deletion.ts:84-97` — the `native` branch, entered purely
on the server's say-so.
- `apps/mobile/src/components/account/delete-account-section.tsx:27-29` — collapses this
into `deleteReauthenticationFailed` ("Please try again"), which is misleading: retrying
cannot succeed.
- `apps/mobile/src/app/sign-in.tsx:28` — the existing `isAvailableAsync()` precedent.
## Required outcome
- A user whose account requires native re-authentication on a device that cannot do it is
told something true and actionable, rather than being invited to retry forever.
- No Sentry report is filed for this case; it is a known device limitation, not an error,
in the same spirit as the existing Apple-cancel suppression.
## Suggested approach
- Probe `AppleAuthentication.isAvailableAsync()` before entering the branch and return a
distinct outcome (e.g. `{ kind: 'failed', code: 'DELETION_REQUIRES_APPLE_DEVICE' }`) so
`failureMessage` can carry copy naming the real constraint.
- Alternatively hide the button entirely when the requirements read says `native` and the
device cannot satisfy it — but that needs the requirements read to happen on mount
rather than on tap, which is a larger change.
## Open questions for the human
- Is Android in scope for CODE-292 at all? Guideline 5.1.1(v) is App Store only, so if
Android deletion is explicitly deferred, a tracked issue plus honest copy may be the
right resolution rather than code.
- Can the Cloud's `deletion-requirements` response ever include a `browser` fallback for
an Apple-linked account, which would let the client degrade instead of dead-ending?| const parsed = deletionResponseSchema.safeParse(response.data); | ||
| if (!parsed.success) { | ||
| // The server accepted the request but the response shape is unreadable — | ||
| // ambiguous in the same way a network failure is: assume accepted. | ||
| return { kind: 'pending' }; | ||
| } |
There was a problem hiding this comment.
Every other failure path in this function calls reportFailure, but the two paths that indicate the client and server have drifted apart are silent. An unreadable 2xx body is exactly the signal you want in Sentry — it means the deletion response contract changed under you — and the user is meanwhile told "3–5 business days" for a deletion that most likely completed instantly. The non-401 error branch above (line 138) has the same gap: a genuine pre-PONR 5xx is indistinguishable from an expected 409 business rejection and neither is reported.
Technical details
# Report schema drift and server errors on the deletion endpoint
## Affected sites
- `apps/mobile/src/runtime/cloud/deletion.ts:144-149` — `safeParse` failure returns
`{ kind: 'pending' }` with no `reportFailure` call.
- `apps/mobile/src/runtime/cloud/deletion.ts:135-141` — every non-401 status returns
`failed`; a 5xx is reported nowhere, while a 409 correctly should not be.
## Required outcome
- A `DELETE /account` response body that does not match `deletionResponseSchema` produces
a Sentry event, because it means the wire contract between this client and
`linkcodehq` has diverged on a compliance-critical endpoint.
- A 5xx from `DELETE /account` produces a Sentry event; a 409 carrying a known biz code
continues not to, since that is a designed outcome rather than a fault.
## Suggested approach
The `AccountDeletionFailureStage` union already has room for this — either reuse
`'transport'` or add a `'response'` stage, and gate the error-status report on
`status >= 500`. `parsed.error` from the `safeParse` result is the natural payload.
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 intentionally untracked
tasks/CODE-292/.Summary
deleteAccount()first reads the server-ownednative/browserrequirement; device capability is never treated as an account fact.runAccountDeletionTeardown()clears both local authentication states, device enrollment, and tunnel-derived hosts while preserving direct/LAN hosts.Verification
pnpm check:cipasses.CODE292 JourneyandProbesurvive teardown.DELETE /account.Known gaps (non-blocking)