Skip to content

fix(mobile): surface ambiguous deletion transport - #498

Merged
lucas77778 merged 1 commit into
masterfrom
fix/code-292-deletion-transport
Aug 31, 2026
Merged

fix(mobile): surface ambiguous deletion transport#498
lucas77778 merged 1 commit into
masterfrom
fix/code-292-deletion-transport

Conversation

@xiaoland

Copy link
Copy Markdown
Member

Summary

  • return an explicit unknown outcome when DELETE /account produces no HTTP response
  • show neutral copy without claiming success, failure, or safe retry
  • keep local deletion teardown limited to confirmed accepted responses

Follow-up to #481 for CODE-292 D-38.

Verification

  • pnpm check:ci
  • pnpm vitest run --project mobile apps/mobile/src/runtime/cloud/tests/deletion.test.ts
  • pnpm test (3038 passed, 1 skipped, before rebasing the identical patch onto current master)

Copilot AI lite review requested due to automatic review settings August 31, 2026 05:47
@linear-code

linear-code Bot commented Aug 31, 2026

Copy link
Copy Markdown

CODE-292

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@greptile-apps

greptile-apps Bot commented Aug 31, 2026

Copy link
Copy Markdown

Greptile Summary

This PR distinguishes an ambiguous account-deletion transport failure from a confirmed rejection and presents neutral localized messaging without performing unconfirmed teardown.

  • Adds an unknown account-deletion outcome for DELETE requests that reject without an HTTP result.
  • Handles the outcome separately in the mobile account UI.
  • Adds English and Simplified Chinese messaging and updates focused transport-error coverage.

Confidence Score: 5/5

The PR appears safe to merge, with no actionable changed-code defects identified.

The new outcome accurately avoids claiming either rejection or acceptance after the DELETE transport rejects, and the UI continues to reserve destructive local teardown for confirmed accepted responses.

Important Files Changed

Filename Overview
apps/mobile/src/runtime/cloud/deletion.ts Adds an explicit unknown transport outcome while preserving teardown only for confirmed accepted responses.
apps/mobile/src/components/account/delete-account-section.tsx Presents neutral messaging for ambiguous deletion outcomes and returns before local teardown.
apps/mobile/src/runtime/cloud/tests/deletion.test.ts Updates the network-rejection test to assert the unknown outcome and transport-stage reporting.
packages/presentation/i18n/src/locales/en.ts Adds neutral English copy for an unconfirmed deletion request.
packages/presentation/i18n/src/locales/zh-cn.ts Adds corresponding Simplified Chinese copy for an unconfirmed deletion request.

Reviews (1): Last reviewed commit: "fix(mobile): surface ambiguous deletion ..." | Re-trigger Greptile

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ The change itself is right. Two rough edges on the comment and the copy, plus one question about what happens after the alert.

Reviewed changes — full initial review of the single commit 0fe36729 (5 files) at 0fe3672, the CODE-292 D-38 follow-up to #481.

  • New unknown outcomedeletion.ts:27-28 adds a seventh variant to AccountDeletionOutcome, and the catch around DELETE /account (:123-126) now returns it instead of { kind: 'failed' }, so a lost response is no longer reported as a rejected request.
  • Neutral copy, no teardowndelete-account-section.tsx:39-42 alerts deleteUnknown and returns before runAccountDeletionTeardown(), keeping local teardown limited to completed / pending.
  • Rewritten failed doc comment — the same hunk narrows failed to "the server rejected the request".
  • Locale stringsdeleteUnknown added to en.ts and zh-cn.ts, positioned to mirror the union order.

I verified the premise the whole PR rests on, since it is the one thing that would make unknown dead code: better-auth's client builds its $fetch through @better-fetch/fetch's createFetch with no catchAllError (node_modules/better-auth/dist/client/config.mjs:37), and betterFetch's await fetch(context.url, context) is not wrapped in a try/catch (node_modules/@better-fetch/fetch/dist/index.js:600), so a rejected RN fetch really does propagate out of $fetch. The catch is live and the new branch is reachable. I also confirmed the new test is not theatre: reverting :125 back to { kind: 'failed' } fails deletion.test.ts:275 with expected { kind: 'failed' } to deeply equal { kind: 'unknown' }, and 20/20 pass at head (npx vitest run --project mobile apps/mobile/src/runtime/cloud/__tests__/deletion.test.ts). delete-account-section.tsx is the only consumer of the union anywhere in apps/mobile, and the busy flag still resets through the finally on this path, so the new variant leaves no other call site unhandled. This also cleanly resolves the contradiction a prior review flagged on #481, where commit 73431547 had made this branch failed while the neighbouring comment called it ambiguous.

ℹ️ Nothing reconciles the unknown state, and the next tap re-asserts the claim this PR removes

The copy asks the user to check their account status, but if the server did complete the deletion the client stays fully authenticated with device enrollment and tunnel host profiles intact, and nothing re-checks on next launch or foreground. Worse, tapping Delete again re-enters deleteAccount(), where the now-invalid session makes the requirements read fail and returns { kind: 'failed' } — "Could not delete your account. Please try again." That is the exact wrong-confidence message this PR set out to remove, one tap later. The gap predates this PR, so it is a scope question rather than a defect in the diff.

Technical details
# The `unknown` outcome has no recovery path

## Affected sites
- `apps/mobile/src/runtime/cloud/deletion.ts:123-126` — returns `unknown` and stops; nothing
  records that a delete may be in flight.
- `apps/mobile/src/components/account/delete-account-section.tsx:39-42` — terminal alert, then
  `finally { setBusy(false) }` re-enables the button with no changed state behind it.
- `apps/mobile/src/runtime/cloud/deletion.ts:75-82` — the retry path. A session invalidated by a
  server-side deletion makes `requirements.error` truthy, so the second attempt lands on
  `{ kind: 'failed' }` and the generic `deleteFailed` copy.

## Required outcome
- A decision, recorded somewhere durable, on what the client owes the user after an `unknown`.
  Either the ambiguity is genuinely terminal and the copy is the whole answer, or the client
  needs to distinguish "the account is gone" from "the delete failed" on the next attempt.

## Open questions for the human
- Is D-38 scoped to *surfacing* the ambiguity only, with reconciliation tracked separately? If so
  it is worth saying so in the PR body, because the second-tap message is the one a user will
  actually act on.
- Would a 401/404 on the requirements read after a prior `unknown` be enough to distinguish the
  two cases, or does that need a server-side account-status endpoint?

ℹ️ Nitpicks

  • runAccountDeletionTeardown's docstring (deletion.ts:158-159) still enumerates only reauthentication-failed and failed as the kinds it must never be called for. unknown is now the one kind where "the account is still active" is not guaranteed, and a reader taking that list as exhaustive would draw exactly the opposite conclusion from the one this PR encodes. Outside the diff hunk, so noting it here rather than inline.

Pullfrog  | Fix all ➔Fix 👍s ➔View workflow run | Using Claude Opus𝕏

Comment thread apps/mobile/src/runtime/cloud/deletion.ts
Comment thread packages/presentation/i18n/src/locales/en.ts
@xiaoland
xiaoland requested a review from lucas77778 August 31, 2026 11:53

@lucas77778 lucas77778 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@lucas77778
lucas77778 merged commit dee76ee into master Aug 31, 2026
13 checks passed
@lucas77778
lucas77778 deleted the fix/code-292-deletion-transport branch August 31, 2026 11:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants