Repository navigation
T9: persist the user identifier across page loads (browser) #45
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,118 @@ | ||
| import { describe, expect, it, vi } from 'vitest'; | ||
| import { | ||
| createUserIdentifierStore, | ||
| USER_IDENTIFIER_KEY_PREFIX, | ||
| type UserIdentifierStorage, | ||
| } from './user-identifier-store'; | ||
|
|
||
| function memoryStorage(initial: Record<string, string> = {}): UserIdentifierStorage & { | ||
| data: Map<string, string>; | ||
| } { | ||
| const data = new Map(Object.entries(initial)); | ||
| return { | ||
| data, | ||
| getItem: (k) => data.get(k) ?? null, | ||
| setItem: (k, v) => { | ||
| data.set(k, v); | ||
| }, | ||
| removeItem: (k) => { | ||
| data.delete(k); | ||
| }, | ||
| }; | ||
| } | ||
|
|
||
| const boom = (): never => { | ||
| throw new Error('SecurityError'); | ||
| }; | ||
| const throwingStorage: UserIdentifierStorage = { | ||
| getItem: boom, | ||
| setItem: boom, | ||
| removeItem: boom, | ||
| }; | ||
|
|
||
| describe('createUserIdentifierStore', () => { | ||
| it('round-trips an identifier under a per-token key', () => { | ||
| const storage = memoryStorage(); | ||
| const store = createUserIdentifierStore('tok-a', { storage }); | ||
| store.write('a@b.c'); | ||
| expect([...storage.data.entries()]).toEqual([[`${USER_IDENTIFIER_KEY_PREFIX}tok-a`, 'a@b.c']]); | ||
| expect(store.read()).toBe('a@b.c'); | ||
| }); | ||
|
|
||
| it('isolates app tokens from each other', () => { | ||
| const storage = memoryStorage(); | ||
| createUserIdentifierStore('tok-a', { storage }).write('one'); | ||
| expect(createUserIdentifierStore('tok-b', { storage }).read()).toBeNull(); | ||
| }); | ||
|
|
||
| it('clear removes the stored value', () => { | ||
| const storage = memoryStorage(); | ||
| const store = createUserIdentifierStore('t', { storage }); | ||
| store.write('x'); | ||
| store.clear(); | ||
| expect(storage.data.size).toBe(0); | ||
| expect(store.read()).toBeNull(); | ||
| }); | ||
|
|
||
| it('treats an empty stored value as absent', () => { | ||
| const storage = memoryStorage({ [`${USER_IDENTIFIER_KEY_PREFIX}t`]: '' }); | ||
| expect(createUserIdentifierStore('t', { storage }).read()).toBeNull(); | ||
| }); | ||
|
|
||
| it('never throws on a blocked storage; each failure goes to onError', () => { | ||
| const onError = vi.fn(); | ||
| const store = createUserIdentifierStore('t', { storage: throwingStorage, onError }); | ||
| expect(store.read()).toBeNull(); | ||
| expect(() => store.write('x')).not.toThrow(); | ||
| expect(() => store.clear()).not.toThrow(); | ||
| expect(onError).toHaveBeenCalledTimes(3); | ||
| }); | ||
|
|
||
| it('tolerates failures with no onError configured', () => { | ||
| const store = createUserIdentifierStore('t', { storage: throwingStorage }); | ||
| expect(store.read()).toBeNull(); | ||
| expect(() => store.write('x')).not.toThrow(); | ||
| expect(() => store.clear()).not.toThrow(); | ||
| }); | ||
|
|
||
| it('is inert when there is no storage at all', () => { | ||
| const store = createUserIdentifierStore('t', { storage: null }); | ||
| expect(store.read()).toBeNull(); | ||
| expect(() => store.write('x')).not.toThrow(); | ||
| expect(() => store.clear()).not.toThrow(); | ||
| }); | ||
|
|
||
| it('falls back to globalThis.localStorage by default', () => { | ||
| const storage = memoryStorage(); | ||
| vi.stubGlobal('localStorage', storage); | ||
| try { | ||
| createUserIdentifierStore('t').write('g'); | ||
| expect(storage.data.get(`${USER_IDENTIFIER_KEY_PREFIX}t`)).toBe('g'); | ||
| } finally { | ||
| vi.unstubAllGlobals(); | ||
| } | ||
| }); | ||
|
|
||
| it('is inert when the host has no localStorage global', () => { | ||
| vi.stubGlobal('localStorage', undefined); | ||
| try { | ||
| expect(createUserIdentifierStore('t').read()).toBeNull(); | ||
| } finally { | ||
| vi.unstubAllGlobals(); | ||
| } | ||
| }); | ||
|
|
||
| it('survives a throwing localStorage getter (blocked cookies)', () => { | ||
| const onError = vi.fn(); | ||
| const original = Object.getOwnPropertyDescriptor(globalThis, 'localStorage'); | ||
| Object.defineProperty(globalThis, 'localStorage', { get: boom, configurable: true }); | ||
| try { | ||
| const store = createUserIdentifierStore('t', { onError }); | ||
| expect(store.read()).toBeNull(); | ||
| expect(onError).toHaveBeenCalledTimes(1); | ||
| } finally { | ||
| if (original !== undefined) Object.defineProperty(globalThis, 'localStorage', original); | ||
| else Reflect.deleteProperty(globalThis, 'localStorage'); | ||
| } | ||
| }); | ||
| }); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,73 @@ | ||
| // Durable storage for the user identifier (iOS persists `setUserIdentifier` across launches; web parity). | ||
| // | ||
| // Deliberately NARROWER than the device-id machinery (device-id.ts), which mirrors one value across | ||
| // localStorage + cookie + Cookie Store + IndexedDB + Cache API so it survives a "clear site data". An | ||
| // identifier is the app's PII (often an email): it must live in exactly ONE first-party store, never in a | ||
| // cookie (cookies are sent to the server on every request), and clearing site data SHOULD forget it. So: | ||
| // per-app-token localStorage only, last-writer-wins across tabs, read once at launch. | ||
| // | ||
| // Every access is wrapped. A blocked/throwing storage (private mode, quota, disabled cookies) reports | ||
| // through `onError` and degrades to the in-memory Environment value, which is always authoritative. | ||
|
|
||
| export const USER_IDENTIFIER_KEY_PREFIX = 'bugsee.user_identifier.'; | ||
|
|
||
| /** The localStorage subset used. Injectable for tests. */ | ||
| export interface UserIdentifierStorage { | ||
| getItem(key: string): string | null; | ||
| setItem(key: string, value: string): void; | ||
| removeItem(key: string): void; | ||
| } | ||
|
|
||
| export interface UserIdentifierStore { | ||
| /** The persisted identifier, or null when absent / unreadable. Never throws. */ | ||
| read(): string | null; | ||
| /** Persist `id`. Never throws. */ | ||
| write(id: string): void; | ||
| /** Remove the persisted identifier. Never throws. */ | ||
| clear(): void; | ||
| } | ||
|
|
||
| export interface UserIdentifierStoreOptions { | ||
| /** Storage seam. `undefined` → `globalThis.localStorage`; `null` → no storage (inert store). */ | ||
| storage?: UserIdentifierStorage | null; | ||
| onError?: (error: unknown) => void; | ||
| } | ||
|
|
||
| export function createUserIdentifierStore( | ||
| appToken: string, | ||
| options: UserIdentifierStoreOptions = {}, | ||
| ): UserIdentifierStore { | ||
| const key = `${USER_IDENTIFIER_KEY_PREFIX}${appToken}`; | ||
| const report = (error: unknown): void => options.onError?.(error); | ||
| // Resolved lazily and inside try: merely touching `globalThis.localStorage` throws SecurityError when | ||
| // storage is blocked. | ||
| const resolve = (): UserIdentifierStorage | null => | ||
| options.storage === undefined | ||
| ? ((globalThis as { localStorage?: UserIdentifierStorage }).localStorage ?? null) | ||
| : options.storage; | ||
| return { | ||
| read() { | ||
| try { | ||
| const value = resolve()?.getItem(key); | ||
| return typeof value === 'string' && value !== '' ? value : null; | ||
| } catch (error) { | ||
| report(error); | ||
| return null; | ||
| } | ||
| }, | ||
| write(id) { | ||
| try { | ||
| resolve()?.setItem(key, id); | ||
|
Comment on lines
+58
to
+60
There was a problem hiding this comment. P2 — Failed
Scenario: First-time write failure is fine (next launch sees nothing). An identity change is the bug. Fix: On |
||
| } catch (error) { | ||
| report(error); | ||
| } | ||
| }, | ||
| clear() { | ||
| try { | ||
| resolve()?.removeItem(key); | ||
| } catch (error) { | ||
| report(error); | ||
| } | ||
| }, | ||
| }; | ||
| } | ||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P3 — Reusing
deviceIdEnv.localStoragefor 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_IDinreport-dialog.launch.test.ts/mobile-options.launch.test.ts/launch-lifecycle.test.ts). After this change thatgetItemanswers 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 uploadsrequest.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.localStorageis key-aware, so this is not an end-user bug unless a caller injects a similarly key-blinddeviceIdEnv.localStorage.Fix: Default identifier storage to
globalThis.localStorage; do not share the device-id seam. Or make those fakes key-aware, matchingtestDeviceIdLocalStorage()inlaunch.test.ts.