T9: persist the user identifier across page loads (browser)

[krassx]

Review-only PR. This change is already on main (the repo works directly on main; see CLAUDE.md). The base branch is the commit it forked from, so the diff below is exactly this task. Merging this PR changes nothing. Review findings become follow-up commits on main. Plan: docs/design/mobile-api-alignment.md.

Mobile parity: iOS setUserIdentifier is persisted across restarts (Android does not).

Browser only: one localStorage entry per app token, restored synchronously at launch before capture starts; the report dialog pre-fills it on the next load. Opt-out persistUserIdentifier (default true). Real-Chromium e2e: set/reload/present, clear/reload/absent, opt-out writes nothing.

Review focus (privacy)

• An email in localStorage is PII at rest; chosen over cookies deliberately (cookies would send it on every request). Check the default-on decision.
• Cross-tab is last-writer-wins at next launch (no live sync). Firefox/Safari with blocked storage unverified. Node/server tiers stay in memory by design.

🤖 Generated with Claude Code

[cursor]

T9 (browser user-identifier persistence) at fa86c3a. First review of this PR.

Intent matches the task: iOS-style persist of setUserIdentifier across page loads, browser-only, one localStorage key per app token, restore synchronously after the client is created and before providers / manifests / client.launch(), public setters (and the report dialog) write through the same store, persistUserIdentifier: false skips the store entirely. I also read createClient / Environment, the report-dialog email path, Electron renderer launchCore, carrier/public-client wrapping, recovery markers, and the existing deviceIdEnv.localStorage test doubles.

Happy path is solid. Two P2s on persistence integrity / privacy of the opt-out; one P3 from reusing the device-id storage seam.

Findings

1. P2 — Failed setItem leaves the previous identifier on disk

Location: packages/browser/src/user-identifier-store.ts:58-64 (wired from launch.ts:848-850)

Problem: write() swallows QuotaExceededError (and any other setItem throw) after the in-memory Environment has already been updated. read() on the next launch still succeeds, so the old value is restored.

Impact: Reports in the session that changed identity are attributed to the new user; after reload they flip back to the previous user. For an email used as request.json.email, that is a cross-session identity mix-up, not just a missed persist.

Scenario: setUserIdentifier('alice@…') persisted. Origin localStorage later hits quota (mobile Safari ~5MB is the usual case). App calls setUserIdentifier('bob@…'); setItem throws; onError fires; getUserIdentifier() returns bob@. Reload restores alice@. This is the normal localStorage failure mode: setItem throws while getItem still works. A first-time write that fails is harmless (next launch sees nothing). A change is not.

Fix: After a failed write of a new value, attempt removeItem so the next launch does not resurrect the stale identity (and/or read-back after setItem). Same idea if clear() fails while getItem still works — rare for removeItem, but the logout path is the privacy-sensitive one.

2. P2 — persistUserIdentifier: false leaves PII at rest and a later default launch restores it

Location: packages/browser/src/launch.ts:622-627 (pinned by user-identifier.launch.test.ts:149-158)

Problem: Opt-out skips creating the store, so it neither restores nor removes an already-stored value. The option docs say "Set false when the identifier is PII you do not want at rest on the device." The implementation leaves that PII in localStorage.

Impact: An app that turns the flag off (privacy review, GDPR, "stop storing email") still has the email on disk. Any later launch that omits the flag (default true) silently restores it onto reports and the dialog pre-fill.

Scenario: Session 1 default-on, setUserIdentifier('ann@example.com'). Session 2 persistUserIdentifier: false — in-memory only, test asserts the stale key is untouched. Session 3 default launch (flag forgotten, or a different bundle) — ann@example.com is back on getUserIdentifier() and request.json.email.

Fix: On opt-out, still construct the store long enough to clear() the existing key (and not read()). If non-destructive opt-out is intentional, the docs/PROGRESS text need to say the value remains at rest and will come back when the flag is omitted.

3. P3 — Identifier store reuses deviceIdEnv.localStorage, so key-blind device-id fakes become a user identifier

Location: packages/browser/src/launch.ts:624

Problem: Production without deviceIdEnv correctly falls through to globalThis.localStorage. Tests that already inject a device-id-only fake (getItem: () => TEST_DEVICE_ID in report-dialog.launch.test.ts, mobile-options.launch.test.ts, launch-lifecycle.test.ts) now answer every key with the device UUID. Restore treats that as a persisted identifier.

Impact: Those launches set getUserIdentifier() to the device UUID. A dialog send that never touches email (report-dialog.launch.test.ts "sends a dialog report…") now uploads request.json.email = a1b2c3d4-…. The test still passes because it does not assert email is absent. Production real localStorage is key-aware, so this is not an end-user bug unless someone injects a similarly key-blind deviceIdEnv.localStorage.

Scenario: Any in-tree browser launch that uses the key-blind stub; I confirmed report-dialog.launch.test.ts still passes after this PR, so the leak is silent.

Fix: Do not reuse the device-id seam. Default to globalThis.localStorage; add a dedicated identifier storage option if tests need injection. At minimum, make the existing fakes key-aware (as testDeviceIdLocalStorage() already is).

Privacy / default-on (not a defect)

Default true matches T9 / iOS and is the right parity call. Residual web-specific risk vs iOS Keychain: XSS and same-origin third-party scripts can read bugsee.user_identifier.<appToken>; the raw app token in the key is enumerable. Cookies were correctly rejected. persist: false (IndexedDB capture) does not disable this, and should not — it never covered device-id either; the new flag is the knob. Cross-tab is last-writer-wins at next launch, as documented; a leftover tab that submits the report dialog will write the old email back after another tab called clearUserIdentifier().

Test notes

Ran pnpm --filter @bugsee/browser exec vitest run src/user-identifier-store.test.ts src/user-identifier.launch.test.ts src/report-dialog.launch.test.ts — 32/32 passed. Did not run the Chromium e2e (instrumentation-tests/test/user-identifier.e2e.ts) in this pass.

Verdict

1. Overall risk: Medium
2. Merge recommendation: Request changes (the two P2s)
3. Most important to fix: stale identity after a failed setItem; opt-out leaving PII at rest (or document that it does)
4. Positives: Restore is actually before providers and client.launch(); every storage touch is try/catch including a throwing localStorage getter; per-token keys; dialog uses the wrapped setters; empty stored value is absent; Node/server correctly stay in-memory; real-Chromium e2e covers set/clear/opt-out write-nothing.

  

_{Sent by Cursor Automation: Bugsee code review}

[cursor]

P2 — Failed setItem leaves the previous identifier on disk.

write is best-effort, but setItem throwing (QuotaExceededError is the realistic case) is not symmetric with read: getItem still works, so the next launch restores the old value after in-memory already moved to the new one.

Scenario: setUserIdentifier('alice@…') persisted. Later setUserIdentifier('bob@…') hits quota; onError fires; getUserIdentifier() is bob@. Reload restores alice@. Reports in the failed session and the next session disagree about who the user is (request.json.email).

First-time write failure is fine (next launch sees nothing). An identity change is the bug.

Fix: On write failure, try removeItem so a stale identity cannot come back (and/or read-back after setItem). Worth doing the same for clear() if removeItem throws while getItem still works — that is the logout path.

[cursor]

P2 — Opt-out does not purge an already-stored identifier.

persistUserIdentifier: false skips creating the store, so it neither restores nor removes. user-identifier.launch.test.ts pins the stale key remaining. The option docs (BugseeOption.PersistUserIdentifier, BugseeLaunchOptions.persistUserIdentifier) say to pass false when the identifier is PII you do not want at rest.

Scenario: Default launch persists ann@example.com. A later launch with persistUserIdentifier: false ignores it for that session. A following default launch (flag omitted) restores ann@example.com onto the Environment, the first report, and the dialog pre-fill.

Fix: On opt-out, open the store only to clear() the existing key (do not read()). If leaving the value is intentional so toggling the flag is non-destructive, the docs need to say it stays at rest and will return when the flag is omitted.

[cursor]

P3 — Reusing deviceIdEnv.localStorage for a different key space.

This is how the new tests inject storage, but several existing launch harnesses already pass a device-id-only fake (getItem: () => TEST_DEVICE_ID in report-dialog.launch.test.ts / mobile-options.launch.test.ts / launch-lifecycle.test.ts). After this change that getItem answers the identifier key too, so launch restores the device UUID as the user identifier.

Scenario: report-dialog.launch.test.ts "sends a dialog report that reaches the collector as code_dialog" never fills email. The field is pre-filled with the UUID; send uploads request.json.email = a1b2c3d4-…. The test still passes (it does not assert email is absent). I re-ran that file; 13 tests green, leak is silent.

Production globalThis.localStorage is key-aware, so this is not an end-user bug unless a caller injects a similarly key-blind deviceIdEnv.localStorage.

Fix: Default identifier storage to globalThis.localStorage; do not share the device-id seam. Or make those fakes key-aware, matching testDeviceIdLocalStorage() in launch.test.ts.

[krassx]

Review findings triaged against current main (this PR branch is a frozen snapshot, so its diff does not show the fixes): Opt-in persistence/removal fixed earlier on main; a failed write now drops the stale stored value (7da7f3e).

The overview PR #46 (head = main) was re-reviewed at ba608c4: no remaining findings, recommendation Approve.

🤖 Generated with Claude Code

[krassx]

Closing instead of merging: this review-only branch is a frozen snapshot that now conflicts with the review base branch after #37/#40/#43/#46 were merged. Its content is already on main, and every finding was addressed there (see the earlier triage comment; #46 was approved at the final review).

🤖 Generated with Claude Code