# Plan: Fix code-review findings for PR 11592 (session persistence rework)

Source: adversarial code review of PR 11592 (versioned localStorage sessions,
see `versioned-localstorage-sessions.md`). 16 deduped candidates, 2 refuted,
10 confirmed/plausible findings reported. This plan groups them into six
workstreams ordered by severity and shared root cause. No implementation has
started.

## Root-cause summary

The PR makes persistence writes awaited and failable where they were previously
fire-and-forget (`void persisted.write(...)`). Three systemic consequences fell
out of that change and account for most findings:

1. **Inconsistent failure handling at call sites.** Some paths swallow write
   failures silently, some rethrow into callers that treat any rejection as
   total failure, and none distinguish "server operation succeeded but
   persistence failed" from "operation failed."
2. **In-memory state (`_state`) is now only adopted after a successful write**,
   so a storage failure also corrupts the in-memory view - a regression vs
   main, where `_state` was updated before the (swallowed) write.
3. **Persistence now sits inside the awaited token-refresh chain**, so its
   locking behavior can block the request path.

## Findings-to-workstream map

| # | Finding (short) | File anchor | Workstream |
| - | --- | --- | - |
| 1 | writeToStorage rethrow breaks private mode | `src/state/persisted/index.web.ts:221` | A |
| 2 | Indefinite Web Lock can hang token refresh in all tabs | `src/state/persisted/storage-lock.web.ts:21` | B |
| 3 | Stale bundle's token rotation silently not persisted | `src/state/session/index.tsx:119` | C |
| 4 | Failed logout write can resurrect session on cold start | `src/state/session/index.tsx:449` | A |
| 5 | resumeSession no-op resolves as success | `src/state/session/index.tsx:532` | D |
| 6 | resumeSession rejects after switch already committed | `src/state/session/index.tsx:554` | A |
| 7 | Removed empty-page cursor HACKFIX | `src/lib/api/feed/likes.ts:52` | E |
| 8 | Background refresh reorders account list | `src/state/persisted/session-merge.ts:292` | D |
| 9 | try/finally rewrite can strand pending flags | `src/screens/Login/ChooseAccountForm.tsx:62` | F |
| 10 | refreshSession throws despite successful rotation | `src/state/session/index.tsx:688` | A |

## Workstream A: unify persistence-failure semantics (findings 1, 4, 6, 10)

This is the core of the fix. Decide on one policy and apply it everywhere,
rather than patching the four symptomatic sites independently.

### Proposed policy

- **In-memory state always commits first.** A storage write failure must never
  leave `_state` (or the session reducer) behind the truth. Restore the main
  behavior: update `_state` before attempting the write, on both platforms
  (`src/state/persisted/index.web.ts:104`, `src/state/persisted/index.ts:95-96`).
- **Persistence failures are reported, not thrown, on the session paths.**
  Callers of `login` / `createAccount` / `refreshSession` / `resumeSession` /
  `logout` should see success when the server-side operation succeeded. Storage
  failure becomes a logged warning (with a `safeMessage`) plus, where it
  matters, a one-time user-facing signal - not a rejection racing up into
  callers like `ChangeHandleDialog.tsx:176/331` and `Deactivated.tsx:77` that
  cannot distinguish the two.
- **Expected environmental failures stay quiet.** Web private mode /
  quota-exceeded is expected; restore the main-era swallow (with a debug log)
  in `writeToStorage` (`index.web.ts:216-221`) so ~25 fire-and-forget
  `persisted.write()` call sites (e.g. `src/state/preferences/languages.tsx:46`,
  `src/state/shell/onboarding.tsx:40`) do not each produce an unhandled
  rejection.

### Tasks

- [ ] Restore commit-before-write ordering for `_state` on web
      (`index.web.ts:104`) and native (`index.ts:95-96`). This alone fixes the
      "second language toggle loses the first" in-memory regression (finding 1)
      and the logout-resurrection vector (finding 4), since a later successful
      write of any key spreads `{..._state, [key]: value}` from a `_state` that
      already reflects the logout.
- [ ] Stop rethrowing from `writeToStorage` on web for quota/private-mode
      errors; keep rethrow (or structured error) only for unexpected failures.
- [ ] Sweep every `store.dispatch(...)` persistence promise in
      `src/state/session/index.tsx` (lines ~119-139, 371-375, 417-421, 449,
      498, 532-554, 688-693, 732) and make handling uniform per the policy
      above. Today: login/createAccount `.catch()` and continue,
      logout/removeAccount `.catch(() => {})` silently, resumeSession awaits
      uncaught, refreshSession rethrows via `takeSessionChangeError`. All four
      shapes should collapse to one helper, e.g.
      `persistOrWarn(promise, context)`.
- [ ] resumeSession (finding 6): the in-memory switch commits synchronously at
      `index.tsx:114-115`; the awaited write at `index.tsx:554` must not turn a
      committed switch into a caller-visible failure
      (`useAccountSwitcher.ts:49-57` currently shows "Please sign in as
      @handle" and routes to login while `hasSession` is already true).
- [ ] refreshSession (finding 10): once persistence failures no longer throw,
      confirm `ChangeHandleDialog` and `Deactivated` close/succeed correctly.
      If we decide some callers DO need to know about persistence failure,
      expose it as a discriminated result, not a rejection.
- [ ] Audit for remaining unhandled-rejection paths: every `persisted.write()`
      and session `dispatch` call site either awaits with handling or is
      explicitly `void`-ed against a non-throwing promise.

### Open decision

Whether logout should surface persistence failure to the user (it is the one
case where a swallowed failure has a security flavor: tokens remain on disk).
Options: (a) toast "couldn't fully clear stored credentials"; (b) retry harder
then give up silently once `_state` ordering is fixed (the resurrection bug is
gone either way); (c) queue a tombstone re-write on next successful write.
Recommend (b) plus a `logger.error` - the resurrection vector, not the disk
residue, was the real bug, and native disk-full is rare.

## Workstream B: bound the Web Lock so it cannot hang token refresh (finding 2)

`navigator.locks.request(PERSISTED_STORAGE_LOCK, op)` at
`storage-lock.web.ts:21` is exclusive, indefinite, and now sits inside the
awaited chain `fetchHandler -> refresh() -> onUpdated -> dispatch ->
writeSession` (`password-session.js:115-116`, `index.web.ts:121`). A tab paused
in devtools (or hung in the critical section) blocks token refresh - and with
it every authenticated request - in all other tabs.

### Tasks

- [ ] Add an `AbortSignal` timeout to the lock request (a few seconds). On
      timeout, proceed per Workstream A policy: in-memory state is already
      committed; log and skip (or retry once in the background) the persisted
      write. Never let the request path wait indefinitely on another tab.
- [ ] Alternatively/additionally: move the persisted write out of the awaited
      refresh chain entirely - resolve the session promise after the reducer
      dispatch, and let `writeSession` run fire-and-forget behind the lock.
      Evaluate against the cross-tab reconciliation logic before choosing;
      the write ordering guarantees in `session-merge.ts` may depend on
      awaiting.
- [ ] Add a regression test simulating a held lock: second tab's refresh must
      complete (in memory) within the timeout budget.

## Workstream C: persist successful rotations from stale bundles (finding 3)

The reducer's bundle-identity guard (`src/state/session/reducer.ts:84-90`)
returns state unchanged for a mutation from a superseded bundle, so
`needsPersist` stays false and `dispatch` never calls `writeSession`
(`index.tsx:119-139`). But the server has already rotated the refresh token;
bundle disposal is deferred to a post-commit effect (`index.tsx:887-899`), so
account A's `onUpdated` legitimately fires mid-switch. Storage keeps the
consumed refresh token; resuming A after the PDS grace period (~2h) logs the
user out.

### Tasks

- [ ] Decouple "update reducer state" from "persist credentials": a valid
      `refresh` mutation for a known account should reach storage (via the
      session-merge path, which already reconciles by account DID and
      generation) even when the reducer rejects it for bundle identity.
      Concretely: in `onSessionChange` / dispatch, if the reducer returns
      unchanged due to bundle mismatch but the mutation carries newer
      credentials for a stored account, still call `writeSession` with a
      credentials-only merge.
- [ ] Guard against the inverse: a rotation from a stale bundle must not
      clobber NEWER stored credentials for the same account - rely on the
      existing generation comparison in `session-merge.ts` (the "Log rejected
      refresh generations" commit suggests the machinery exists).
- [ ] Test: switch A -> B while A has an in-flight refresh; assert storage ends
      with A's rotated token, reducer stays on B.

## Workstream D: resumeSession divergence and account ordering (findings 5, 8)

### Finding 5: silent no-op resume

`if (!latestStoredAccount?.refreshJwt) return` at `index.tsx:532` resolves as
success without dispatching, so `useAccountSwitcher.ts:38-42` shows "Signed in
as @handle" and fires the `account:loggedIn` metric while the previous account
is still active.

- [ ] Make the no-credentials path throw a typed error (e.g.
      `SessionResumeError('no-stored-credentials')`) so the existing caller
      catch shows "Please sign in as @handle" and routes to login, matching
      main's behavior. Keep the early return only if there is a case where
      silent success is genuinely correct (none identified in review).

### Finding 8: background refresh reorders the account list

`replaceAccount` (`session-merge.ts:292`) unshifts to index 0 and is called by
both `refresh` (line 205) and `login` (line 238) mutations. Cross-tab sync
adopts the order verbatim (`reducer.ts:238-241`), so `AccountList.tsx:54`
reorders under the user's finger during a background refresh in another tab.

- [ ] Preserve list position for `refresh` mutations (replace in place);
      move-to-front only for explicit user actions (login, switch). If
      "current account first" is an invariant elsewhere, derive display order
      in the UI instead of mutating storage order.
- [ ] Test: refresh mutation for account at index 2 leaves it at index 2;
      login mutation still fronts it.

## Workstream E: restore empty-page cursor protection in feed APIs (finding 7)

The removed HACKFIX ("-sfn" comment) in `src/lib/api/feed/likes.ts:52` and
`src/lib/api/feed/custom.ts:99` handled servers returning a cursor with zero
items. The PR's `seenCursors` guard exists only in `MergeFeedAPI`
(`merge.ts:226-236`). `useAutoPagination`'s repeated-cursor guard never matches
an advancing cursor and its 50-attempt failsafe resets whenever `itemCount`
changes; afterward `hasNextPage` stays true and every `onEndReached` refetches
empty pages with a flashing spinner (`PostFeed.tsx:773, 964`).

- [ ] Either restore the empty-page-drops-cursor behavior in `LikesFeedAPI`
      and `CustomFeedAPI`, or lift the `seenCursors` guard into shared code all
      feed APIs use. Prefer the shared guard - it also covers future APIs.
- [ ] Test: mock a feed returning `{items: [], cursor: <advancing>}` pages;
      assert pagination terminates and `hasNextPage` goes false.

## Workstream F: restore finally-guaranteed cleanup (finding 9)

The systematic `try/finally` -> `try/catch` + trailing-statement rewrite (~14
sites) skips cleanup when the catch body itself throws. Confirmed sites:

- `src/screens/Login/ChooseAccountForm.tsx:62` - `setPendingDid(null)` skipped
  if `onSelectAccount(account)` (line 60) throws; the `if (pendingDid) return`
  guard (line 33) then ignores every tap until remount.
- `App.tsx:130` - `setIsReady` skipped -> stuck splash screen.
- `ChangePasswordDialog`, `ExportCarDialog`, `ReportDialog`,
  `useAccountSwitcher` - `setIsProcessing`/`setLoading` flags, with
  `Toast.show` in the catch as the potential thrower.

### Tasks

- [ ] Enumerate all rewritten sites in the PR diff (search the diff for removed
      `finally` blocks) and restore `finally` for every state-flag cleanup.
      Keep `catch` for the error handling; this is `try/catch/finally`, not a
      revert.
- [ ] If any site intentionally moved cleanup out of `finally` (e.g. cleanup
      must NOT run on failure), add a comment stating why.

## Sequencing

1. **A first** - it defines the failure-semantics policy the other session
   workstreams build on, and fixes the two worst user-facing regressions
   (private-mode breakage, logout resurrection).
2. **B and C next** - both touch the dispatch/writeSession chain A refactors;
   doing them after A avoids rework. B's "move write off the refresh chain"
   option interacts with A's helper.
3. **D** after A (finding 5's fix shape depends on A's error-vs-result
   decision for resumeSession).
4. **E and F** are independent of A-D and of each other; can be parallelized
   or done in separate small PRs at any point.

## Verification

- [ ] `pnpm test`, `pnpm typecheck`, `pnpm lint` clean.
- [ ] New unit tests called out in B, C, D, E above.
- [ ] Manual web pass in Safari private mode: toggle preferences (no unhandled
      rejections, both of two consecutive language additions stick), log in,
      change handle (dialog closes), switch accounts, log out.
- [ ] Manual two-tab pass: background refresh in tab B while tab A has the
      account switcher open (no reorder); pause tab B in devtools and confirm
      tab A's requests still complete after token expiry.
- [ ] Native smoke: login, switch, logout, relaunch (no session resurrection).

## Explicitly out of scope (cut or refuted in review)

- uriSize.web helper claim - refuted (blob URLs, not data URIs).
- No-Web-Locks fallback - documented accepted tradeoff.
- Cleanup-level items cut under the finding cap: duplicated reconciliation
  blocks, writeSession fast-path/full-root round-trip, storage-lock
  duplication, `persistWithRetry` vs `retry()`, `messages.po` churn. Revisit
  in a follow-up `/simplify` pass after the correctness fixes land.
- Native `readLatest` lagging the write queue during rapid account switches -
  weakest plausible race, mitigated by the PDS grace period; not planned.