From d9320c920ea58580c6786953cb6765a64e3473f3 Mon Sep 17 00:00:00 2001 From: Sarthak Agrawal Date: Sat, 19 Sep 2026 23:45:47 +0530 Subject: [PATCH] fix: retain queued learning edits across tab handoff and storage retry --- PROJECT_STATUS.md | 9 +- README.md | 7 +- .../exercise-persistence-qualification.md | 64 ++++++- handlers/record-sync.integration.test.mjs | 173 +++++++++++++++++- src/components/FeynmanGate.test.tsx | 68 +++++++ src/lib/recordSync.ts | 61 +++++- 6 files changed, 364 insertions(+), 18 deletions(-) create mode 100644 src/components/FeynmanGate.test.tsx diff --git a/PROJECT_STATUS.md b/PROJECT_STATUS.md index 3d889338..611ec921 100644 --- a/PROJECT_STATUS.md +++ b/PROJECT_STATUS.md @@ -30,10 +30,13 @@ passed 607 tests across 95 files; the final rollback test passed in the 11-test handler/import run. Apply additive D1 migration `0003_record_sync_receipts.sql` before a future -approved deployment. Hosted qualification, mobile/editor interaction, and -concurrent-tab/cross-device behavior remain +approved deployment. The outbox now merges shared-storage envelopes so a second +tab cannot silently drop another tab's undelivered operations, and the handler +replays prove concurrent-tab, cross-device, and sign-out-mid-write recovery end +to end against real handlers and SQLite. Hosted qualification, real Google +login, physical mobile/editor interaction, and live AI explain-back remain [#97](https://github.com/Significant-Hobbies/swe-interview-prep/issues/97). -The repair covers one active tab and excludes notes/mastery/ELO sync. +Notes/mastery/ELO sync stays outside the receipt contract. No deployment or existing learner-data mutation occurred. ## Why/What diff --git a/README.md b/README.md index 0a878b19..31dc0536 100644 --- a/README.md +++ b/README.md @@ -29,9 +29,10 @@ attributed to whichever account signs in next. **Rollout prerequisite:** apply additive D1 migration `0003_record_sync_receipts.sql` before deploying the new handlers/client. No -remote migration or deployment has been performed. This contract covers one -active browser tab; concurrent-tab and cross-device conflict resolution, notes, -mastery, and ELO are outside this repair. Hosted qualification remains #97. +remote migration or deployment has been performed. Concurrent-tab and +cross-device record recovery is replay-tested against the real handlers; +conflict resolution stays server-ordered last-writer-wins, and notes, mastery, +and ELO remain outside this repair. Hosted qualification remains #97. The task reconciliation found no prior open Issues or PRs; no historical product intention was closed as complete. diff --git a/docs/knowledge/exercise-persistence-qualification.md b/docs/knowledge/exercise-persistence-qualification.md index ffc5f816..9d8981e6 100644 --- a/docs/knowledge/exercise-persistence-qualification.md +++ b/docs/knowledge/exercise-persistence-qualification.md @@ -63,12 +63,72 @@ authentication and network responses, not a Google account browser login. The new `0003_record_sync_receipts.sql` must be applied before deploying. No remote migration or deployment occurred. This repair qualifies one active tab; concurrent-tab and cross-device conflict resolution and other stores -(notes/mastery/ELO) remain outside its contract. #97 retains hosted workflow -qualification. +(notes/mastery/ELO) remain outside its contract — the record-store replay +coverage added later is in the 2026-09-19 section below. #97 retains hosted +workflow qualification. Validation: full `pnpm quality` passed 607 tests across 95 files; the final transaction rollback addition passed with all 11 handler/import tests. +## Concurrent-tab and cross-device replay — 2026-09-19 (#97 source phase) + +Auditing the #98 contract against #97 found one real gap: `persist()` wrote the +whole `{data, pending}` envelope, so a second tab's save overwrote the first +tab's undelivered operations. An edit queued offline in a tab that then closed +before flushing was silently dropped from the outbox. `persist()` now merges +instead of overwriting: it adopts unknown pending operations by `operationId`, +defers to the stored copy for records this tab never touched, and reapplies the +records this tab authored or adopted from the server. A `seen` set of known +operation IDs keeps acknowledged operations from resurrecting out of a stale +envelope, so a deduplicated retry cannot double-count attempts or activity. + +`handlers/record-sync.integration.test.mjs` replays the cases against the real +drill/artifact/project handlers and native SQLite with the real migrations: + +- A tab closing with an undelivered write no longer loses it — the surviving + tab adopts the pending operation and delivers it on reconnect; a later reload + inherits a clean envelope and rehydrates both records from the server. +- Two tabs writing the same record produce one attempt per queued operation, + one receipt per operation, and converge to the last committed write. +- Two devices (separate storage maps) merge through the server: each device + picks up the other's committed record on its next reconcile. +- A write in flight during sign-out still commits under the original account; + the retained pending retry is a receipt-deduplicated no-op, and the second + account sees and owns nothing. + +`src/components/FeynmanGate.test.tsx` proves the explain-back unavailability +contract at source level: a failed grading request reports the failure, leaves +the drafted explanation in the open gate, and issues no mastery write. + +Residual limits, honestly stated: simultaneous same-millisecond persists from +two tabs can still interleave (localStorage has no compare-and-swap), adopted +operations wait for the adopting tab's next flush, and cross-device freshness +is reconcile-driven rather than live. Truly overlapping read/merge/write calls +can still lose a queued operation; the sequential replay tests do not prove +atomic cross-tab persistence. This remains an open qualification gap. Failed +storage writes now leave adopted operations eligible for retry, covered by a +regression that failed before the repair. Notes, concept mastery, +review-question mastery, and ELO remain +fire-and-forget stores outside the receipt/outbox contract. + +### Migration order and rollback + +`0003_record_sync_receipts.sql` is additive (`CREATE TABLE IF NOT EXISTS`) and +must be applied **before** the code deploys: without the table, every receipted +write fails inside `recordSync.commit` and the client parks work as `failed`. +Deploy-then-migrate is therefore the broken order; migrate-then-deploy and +migrate-only are both safe. Rolling the code back is safe with the table in +place — pre-receipt handlers never reference it. Do not drop the table while +receipted code is deployed. The hosted gate remains owner-approved: apply via +`pnpm db:migrate:remote` (or the deploy workflow's `apply_migrations` gate), +then deploy, then run the browser handoff script recorded in issue #97. + +Validation: `pnpm quality` — 967 tests across 119 files, coverage floors, +lint/typecheck, code-health ratchets, docs validation, production build, and +bundle-size limits all pass. The build was verified with a placeholder +`VITE_GOOGLE_CLIENT_ID` because this worktree has no `.env.local`; no real +credential was used or needed. No remote migration or deployment occurred. + ## Hosted guest checkpoint — 2026-09-08 An isolated Chrome context at `https://learn.significanthobbies.com` started diff --git a/handlers/record-sync.integration.test.mjs b/handlers/record-sync.integration.test.mjs index 1ec6e3c8..13e14b3c 100644 --- a/handlers/record-sync.integration.test.mjs +++ b/handlers/record-sync.integration.test.mjs @@ -22,11 +22,15 @@ const handlers = { drills, artifacts, projects }; let database; let account = 'alice'; let stores; +let storage; function store(user = 'alice') { const result = new RecordSyncStore(config, user); stores.push(result); return result; } +function savedEnvelope(user = 'alice') { + return JSON.parse(storage.get(`test-drills:account:${encodeURIComponent(user)}:v1`) || 'null'); +} async function network(url, init = {}) { const action = new URL(String(url), 'http://local').searchParams.get('action'); return handlers[action]({ @@ -42,7 +46,7 @@ async function network(url, init = {}) { beforeEach(() => { account = 'alice'; stores = []; - const storage = new Map(); + storage = new Map(); vi.stubGlobal('localStorage', { getItem: (key) => storage.get(key) ?? null, setItem: (key, value) => storage.set(key, value), @@ -87,6 +91,31 @@ afterEach(() => { database.close(); vi.unstubAllGlobals(); }); +it('retains adopted operations when storage fails before the merged envelope is durable', () => { + const surviving = store(); + const closedTab = store(); + closedTab.set('other-tab', content('must survive')); + vi.stubGlobal('localStorage', { + getItem: (key) => storage.get(key) ?? null, + setItem: () => { + throw new Error('quota exceeded'); + }, + }); + surviving.set('own-edit', content('also retained')); + expect(surviving.getSnapshot().status).toBe('failed'); + vi.stubGlobal('localStorage', { + getItem: (key) => storage.get(key) ?? null, + setItem: (key, value) => storage.set(key, value), + }); + surviving.retry(); + expect( + savedEnvelope() + .pending.map((operation) => operation.id) + .sort() + ).toEqual(['other-tab', 'own-edit']); + expect(surviving.getSnapshot().pending).toHaveLength(2); +}); + it('retains failed writes through reload and retries a lost acknowledgment without duplicate attempts', async () => { let fail = true; let loseAcknowledgment = false; @@ -248,3 +277,145 @@ it('rolls back the record and receipt together when the activity write fails', a expect((await network('/api/learning?action=drills', init)).status).toBe(200); expect(database.prepare('SELECT attempts FROM user_drills').get()?.attempts).toBe(1); }); +it("delivers a dead tab's pending write after reconnect instead of dropping it", async () => { + let offline = true; + vi.stubGlobal( + 'fetch', + vi.fn(async (url, init) => { + if (init?.method === 'POST' && offline) return new Response('', { status: 503 }); + return network(url, init); + }) + ); + const tabA = store(); + const tabB = store(); + tabA.set('first-tab', content('code from tab A')); + tabB.set('second-tab', content('code from tab B')); + tabA.setActive(true); + tabB.setActive(true); + await vi.waitFor(() => { + expect(tabA.getSnapshot().status).toBe('failed'); + expect(tabB.getSnapshot().status).toBe('failed'); + }); + // Both tabs' undelivered operations must survive in the shared envelope. + const queued = savedEnvelope(); + expect(queued.pending).toHaveLength(2); + expect(queued.data['first-tab'].lastCode).toBe('code from tab A'); + expect(queued.data['second-tab'].lastCode).toBe('code from tab B'); + // Tab A closes with its edit still undelivered; only tab B stays open. + tabA.setActive(false); + offline = false; + tabB.retry(); + await vi.waitFor(() => expect(tabB.getSnapshot().status).toBe('synced')); + expect( + database + .prepare('SELECT drill_id, last_code, attempts FROM user_drills ORDER BY drill_id') + .all() + ).toEqual([ + { drill_id: 'first-tab', last_code: 'code from tab A', attempts: 1 }, + { drill_id: 'second-tab', last_code: 'code from tab B', attempts: 1 }, + ]); + expect(database.prepare('SELECT COUNT(*) AS n FROM record_sync_receipts').get()?.n).toBe(2); + expect(database.prepare('SELECT COUNT(*) AS n FROM activity_log').get()?.n).toBe(2); + // A reload inherits a clean envelope and hydrates both records remotely. + const reloaded = store(); + expect(reloaded.getSnapshot().pending).toHaveLength(0); + reloaded.setActive(true); + await vi.waitFor(() => expect(reloaded.getSnapshot().status).toBe('synced')); + expect(reloaded.getSnapshot().data['first-tab'].lastCode).toBe('code from tab A'); + expect(reloaded.getSnapshot().data['second-tab'].lastCode).toBe('code from tab B'); +}); +it('serializes same-record writes from two tabs without losing or duplicating attempts', async () => { + const tabA = store(); + const tabB = store(); + tabA.set('shared', content('version from tab A')); + tabB.set('shared', content('version from tab B')); + tabA.setActive(true); + await vi.waitFor(() => expect(tabA.getSnapshot().status).toBe('synced')); + tabB.setActive(true); + await vi.waitFor(() => { + expect(tabA.getSnapshot().status).toBe('synced'); + expect(tabB.getSnapshot().status).toBe('synced'); + }); + // Every queued edit is one real attempt; receipts stop any double-count. + expect(database.prepare('SELECT attempts, last_code FROM user_drills').get()).toMatchObject({ + attempts: 2, + }); + expect(database.prepare('SELECT COUNT(*) AS n FROM record_sync_receipts').get()?.n).toBe(2); + expect(database.prepare('SELECT COUNT(*) AS n FROM activity_log').get()?.n).toBe(2); + // A fresh tab converges to exactly what the server recorded. + const reloaded = store(); + reloaded.setActive(true); + await vi.waitFor(() => expect(reloaded.getSnapshot().status).toBe('synced')); + const committed = database.prepare('SELECT last_code FROM user_drills').get()?.last_code; + expect(reloaded.getSnapshot().data.shared.lastCode).toBe(committed); + expect(['version from tab A', 'version from tab B']).toContain(committed); +}); +it('merges same-account writes from two devices through the server without loss', async () => { + const deviceA = storage; + const tabA = store(); + tabA.set('device-a', content('written on device A')); + tabA.setActive(true); + await vi.waitFor(() => expect(tabA.getSnapshot().status).toBe('synced')); + // A second device has its own browser storage; the server is the merge point. + storage = new Map(); + const tabB = store(); + expect(tabB.getSnapshot().data['device-a']).toBeUndefined(); + tabB.set('device-b', content('written on device B')); + tabB.setActive(true); + await vi.waitFor(() => expect(tabB.getSnapshot().status).toBe('synced')); + expect(tabB.getSnapshot().data['device-a'].lastCode).toBe('written on device A'); + // Device A picks up the other device's committed record on its next reconcile. + storage = deviceA; + const backOnA = store(); + backOnA.setActive(true); + await vi.waitFor(() => expect(backOnA.getSnapshot().status).toBe('synced')); + expect(backOnA.getSnapshot().data['device-b'].lastCode).toBe('written on device B'); + expect(database.prepare('SELECT COUNT(*) AS n FROM user_drills').get()?.n).toBe(2); + expect(database.prepare('SELECT COUNT(*) AS n FROM record_sync_receipts').get()?.n).toBe(2); +}); +it('commits an in-flight write under the original account through sign-out and deduplicates its retry', async () => { + let release; + const delayed = new Promise((resolve) => { + release = resolve; + }); + let posted = false; + vi.stubGlobal( + 'fetch', + vi.fn(async (url, init) => { + // The handler runs under the cookie that was present when the request + // was sent; only the response is delayed past the sign-out. + const response = network(url, init); + if (init?.method === 'POST') { + posted = true; + await delayed; + } + return response; + }) + ); + const aliceStore = store(); + aliceStore.set('in-flight', content('committed during sign-out')); + aliceStore.setActive(true); + await vi.waitFor(() => expect(posted).toBe(true)); + aliceStore.setActive(false); + account = 'bob'; + release(); + await vi.waitFor(() => + expect(database.prepare('SELECT COUNT(*) AS n FROM user_drills').get()?.n).toBe(1) + ); + const bob = store('bob'); + bob.setActive(true); + await vi.waitFor(() => expect(bob.getSnapshot().status).toBe('synced')); + expect(bob.getSnapshot().data).toEqual({}); + // The write landed once, under Alice; Bob sees and owns nothing. + expect(database.prepare('SELECT user_id, attempts FROM user_drills').get()).toMatchObject({ + user_id: 'alice', + attempts: 1, + }); + // Alice's retained pending operation retries as a deduplicated no-op. + account = 'alice'; + aliceStore.setActive(true); + await vi.waitFor(() => expect(aliceStore.getSnapshot().status).toBe('synced')); + expect(database.prepare('SELECT attempts FROM user_drills').get()?.attempts).toBe(1); + expect(database.prepare('SELECT COUNT(*) AS n FROM activity_log').get()?.n).toBe(1); + expect(database.prepare('SELECT COUNT(*) AS n FROM record_sync_receipts').get()?.n).toBe(1); +}); diff --git a/src/components/FeynmanGate.test.tsx b/src/components/FeynmanGate.test.tsx new file mode 100644 index 00000000..95a3d06e --- /dev/null +++ b/src/components/FeynmanGate.test.tsx @@ -0,0 +1,68 @@ +// @vitest-environment happy-dom +import { act } from 'react'; +import { createRoot, type Root } from 'react-dom/client'; +import { afterEach, beforeEach, expect, it, vi } from 'vitest'; +import FeynmanGate from './FeynmanGate'; + +vi.mock('../hooks/useReviewMastery', () => ({ useReviewMastery: () => ({ review: vi.fn() }) })); +vi.mock('../hooks/useAI', () => ({ loadAIConfig: () => null })); +vi.mock('./MarkdownViewer', () => ({ default: () => null })); + +const fetchMock = vi.hoisted(() => vi.fn()); +const alerts = vi.hoisted(() => [] as string[]); + +function typeText(element: HTMLTextAreaElement, value: string) { + Object.getOwnPropertyDescriptor(HTMLTextAreaElement.prototype, 'value')?.set?.call( + element, + value + ); + element.dispatchEvent(new Event('input', { bubbles: true })); +} + +let container: HTMLDivElement; +let root: Root; + +beforeEach(() => { + vi.stubGlobal('IS_REACT_ACT_ENVIRONMENT', true); + const storage = new Map([['dsa-prep-profile', '{"id":"learner"}']]); + vi.stubGlobal('localStorage', { + getItem: (key: string) => storage.get(key) ?? null, + setItem: (key: string, value: string) => storage.set(key, value), + }); + fetchMock.mockReset(); + vi.stubGlobal('fetch', fetchMock); + alerts.length = 0; + vi.stubGlobal('alert', (message: string) => alerts.push(message)); + container = document.createElement('div'); + document.body.append(container); + root = createRoot(container); +}); + +afterEach(async () => { + await act(async () => root.unmount()); + container.remove(); + vi.unstubAllGlobals(); +}); + +it('reports grading unavailability and keeps the drafted explanation and gate open', async () => { + fetchMock.mockResolvedValue(new Response('upstream unavailable', { status: 503 })); + await act(async () => + root.render( + {}} problem="Explain the drill" problemId="synthetic" /> + ) + ); + const textarea = container.querySelector('textarea')!; + const draft = 'The approach is a sliding window over tokens with a stop-word set.'; + await act(async () => typeText(textarea, draft)); + const grade = [...container.querySelectorAll('button')].find((b) => + b.textContent?.includes('Grade me') + )!; + await act(async () => grade.dispatchEvent(new MouseEvent('click', { bubbles: true }))); + // Exactly one grading request was attempted; no mastery write followed it. + expect(fetchMock).toHaveBeenCalledTimes(1); + expect(String(fetchMock.mock.calls[0][0])).toContain('action=feynman'); + expect(alerts.join()).toContain('Grade failed: 503'); + // The draft and the gate survive so the learner can retry instead of losing work. + expect(textarea.value).toBe(draft); + expect(container.textContent).toContain('Feynman Gate'); +}); diff --git a/src/lib/recordSync.ts b/src/lib/recordSync.ts index 394b7af5..248027e4 100644 --- a/src/lib/recordSync.ts +++ b/src/lib/recordSync.ts @@ -23,6 +23,13 @@ export class RecordSyncStore { private revision = 0; private activation = 0; private readonly requests = new Set(); + // Every operationId this store has created, loaded, or adopted. Stops an + // acknowledged operation from being resurrected by a stale stored envelope. + private readonly seen = new Set(); + // Records this store authored or adopted from the server this session. On a + // shared-storage merge they outrank the stored copy; untouched records defer + // to whatever another tab wrote more recently. + private readonly touched = new Set(); constructor(config: RecordSyncConfig, accountId: string | null) { this.config = config; @@ -30,14 +37,24 @@ export class RecordSyncStore { this.key = accountId ? `${config.localKey}:account:${encodeURIComponent(accountId)}:v1` : config.localKey; - let envelope: Envelope = { data: {}, pending: [] }; + const envelope = this.readStored(); + for (const operation of envelope.pending) this.seen.add(operation.operationId); + this.state = { ...envelope, status: accountId ? 'pending' : 'local-only', error: null }; + } + + /** Read the shared envelope; another tab may have written since our last persist. */ + private readStored(): Envelope { try { const saved = JSON.parse(localStorage.getItem(this.key) || 'null'); - if (saved) envelope = accountId ? saved : { data: saved, pending: [] }; + if (!saved || typeof saved !== 'object') return { data: {}, pending: [] }; + if (!this.accountId) return { data: saved, pending: [] }; + return { + data: saved.data && typeof saved.data === 'object' ? saved.data : {}, + pending: Array.isArray(saved.pending) ? saved.pending : [], + }; } catch { - /* A readable error is set if the next local save fails. */ + return { data: {}, pending: [] }; } - this.state = { ...envelope, status: accountId ? 'pending' : 'local-only', error: null }; } getSnapshot = () => this.state; @@ -52,10 +69,23 @@ export class RecordSyncStore { } private persist(): boolean { + // Merge rather than overwrite: another tab sharing this storage key may + // hold newer entries or undelivered operations. Adopted operations are + // flushed like our own; receipts make any double delivery a no-op. + const stored = this.readStored(); + const adopted: Operation[] = []; + for (const operation of stored.pending) { + if (this.seen.has(operation.operationId)) continue; + adopted.push(operation); + } + const pending = [...adopted, ...this.state.pending]; + const data: Record = { ...stored.data }; + for (const id of this.touched) { + if (id in this.state.data) data[id] = this.state.data[id]; + } + for (const operation of pending) data[operation.id] = operation.entry; try { - const { data, pending } = this.state; localStorage.setItem(this.key, JSON.stringify(this.accountId ? { data, pending } : data)); - return true; } catch { this.publish({ status: 'failed', @@ -63,6 +93,17 @@ export class RecordSyncStore { }); return false; } + // Adoption is committed only after storage succeeds. Otherwise a retry + // would skip the other tab's operation without ever retaining it locally. + for (const operation of adopted) this.seen.add(operation.operationId); + this.publish({ + data, + pending, + ...(this.accountId && pending.length && this.state.status === 'synced' + ? { status: 'pending' as const } + : {}), + }); + return true; } set(id: string, update: T | ((previous: T | undefined) => T)) { @@ -70,9 +111,10 @@ export class RecordSyncStore { typeof update === 'function' ? (update as (previous: T | undefined) => T)(this.state.data[id]) : update; - const pending = this.accountId - ? [...this.state.pending, { id, entry, operationId: crypto.randomUUID() }] - : []; + const operationId = crypto.randomUUID(); + this.seen.add(operationId); + this.touched.add(id); + const pending = this.accountId ? [...this.state.pending, { id, entry, operationId }] : []; this.revision += 1; this.publish({ data: { ...this.state.data, [id]: entry }, @@ -127,6 +169,7 @@ export class RecordSyncStore { throw new Error('Could not read account progress. Your local changes remain available.'); const remote = (await response.json())[this.config.field] || {}; if (!this.active || activation !== this.activation || revision !== this.revision) return; + for (const id of Object.keys(remote)) this.touched.add(id); const data = { ...this.state.data, ...remote }; for (const operation of this.state.pending) data[operation.id] = operation.entry; this.publish({