From 3cc3ba5e013776d7e690aee389c486fdbac35cee Mon Sep 17 00:00:00 2001 From: xarmian Date: Mon, 14 Sep 2026 09:42:38 -0400 Subject: [PATCH] fix(web): flush the collaborative teardown on the events a suspended tab actually delivers (BUG-3030) (#1363) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(web): flush the collaborative teardown on the events a suspended tab delivers (BUG-3030) Claude-Session: https://claude.ai/code/session_01PDYjP67KTAcqpFpo3ijAAA * fix(web): re-arm the teardown latch when a cancelled beforeunload leaves the page alive codex round 2. 'Stay' cancels the navigation without firing pageshow or visibilitychange, so the latch stayed set for the life of the page and every later teardown flushed nothing — a worse loss than the one being fixed. Claude-Session: https://claude.ai/code/session_01PDYjP67KTAcqpFpo3ijAAA * test(web): make the teardown source guard enumerate handlers and fail closed codex round 4. It rejected two literal spellings and checked runTeardownFlush() appeared somewhere in the block, so a new handler writing through any other call passed; and its function-body slicer failed OPEN when indexOf returned -1, scanning most of a 7,900-line file. Claude-Session: https://claude.ai/code/session_01PDYjP67KTAcqpFpo3ijAAA * test(web): measure that one committed teardown writes exactly once (BUG-3030) Lead withheld GO on this measurement rather than a reading: beforeunload -> [setTimeout 0] -> pagehide, and if the timer wins the same bytes go out twice. Measured 1 and 1, with no-edit controls at 0. Adds the jsdom leg for a hidden tab nobody edited writing nothing. Claude-Session: https://claude.ai/code/session_01PDYjP67KTAcqpFpo3ijAAA --- .gitignore | 7 + web/e2e/bug-3030-teardown-write-count.spec.ts | 128 +++++++++++++ .../collab/collabFlushTeardownDedupe.test.ts | 121 ++++++++++++ .../lib/components/items/ItemDetail.svelte | 162 ++++++++++++---- ...tailIdentityDiscardsTeardownWrites.test.ts | 178 +++++++++++++++++- 5 files changed, 549 insertions(+), 47 deletions(-) create mode 100644 web/e2e/bug-3030-teardown-write-count.spec.ts create mode 100644 web/src/lib/collab/collabFlushTeardownDedupe.test.ts diff --git a/.gitignore b/.gitignore index 472fc6d8f..3e47f2a9a 100644 --- a/.gitignore +++ b/.gitignore @@ -6,6 +6,13 @@ # Web build output web/build/ +# Both, deliberately. Only web/node_modules/ was listed, because that is the +# only one a correct invocation creates — but running vitest from the REPO ROOT +# instead of web/ makes a root-level node_modules/ with a vitest cache in it, +# and an unignored one is then swept up by the next `git add -A`. That happened +# on BUG-3030 and a codex round is what caught it. Ignoring the path costs +# nothing and removes the surface rather than relying on remembering. +/node_modules/ web/node_modules/ web/.svelte-kit/ diff --git a/web/e2e/bug-3030-teardown-write-count.spec.ts b/web/e2e/bug-3030-teardown-write-count.spec.ts new file mode 100644 index 000000000..d1306aa84 --- /dev/null +++ b/web/e2e/bug-3030-teardown-write-count.spec.ts @@ -0,0 +1,128 @@ +// ONE committed teardown must produce exactly ONE content write (BUG-3030). +// +// Page-scoped request counting cannot answer this. Measured: a navigate-away +// landed the content while page.on('request') observed ZERO PATCHes, because +// page.goto destroys the page and Playwright stops reporting its requests — +// the keepalive PATCH still goes out. So the count is taken SERVER-SIDE, from +// the item's `seq`, which BUG-3037 bumps on every mutation of the row. Delta +// seq is the number of writes that actually landed, and it is indifferent to +// what the client-side listener could still see. +import { test, expect } from './fixtures'; +import { browserLogin, seedDoc, EDITOR_SELECTOR, SYNCED_BADGE_SELECTOR } from './lib/collab-helpers'; + +const SYNC_TIMEOUT = 20_000; + +// page.request shares the browser context's cookies; the bare `request` +// fixture does not, and reported "content landed = false" for a case that +// demonstrably had landed. +// VERSIONS, not seq. Measured: seq-delta for one teardown was 3 and 4, because +// seq bumps on EVERY mutation of the row and the claim is about CONTENT writes +// only — the instrument's question wider than the claim again. A content PATCH +// creates a version row, so counting versions asks the question being claimed. +// The no-edit CONTROL below is what proves the remaining count is attributable +// to the teardown flush rather than to ambient traffic. +async function readVersionCount(page, fixture, slug: string): Promise { + const res = await page.request.get( + `/api/v1/workspaces/${fixture.workspaceSlug}/items/${slug}/versions`, + ); + expect(res.ok(), 'version read failed — the instrument, not the product').toBeTruthy(); + const body = await res.json(); + return Array.isArray(body) ? body.length : (body.versions?.length ?? 0); +} + +async function setup(page, fixture, request, marker: string, typeIt = true) { + const { slug } = await seedDoc(fixture, request, `Teardown ${marker}`); + await browserLogin(page); + await page.goto(`/${fixture.adminUsername}/${fixture.workspaceSlug}/docs?item=${slug}`); + const editor = page.locator(EDITOR_SELECTOR); + await expect(editor).toBeVisible({ timeout: SYNC_TIMEOUT }); + await expect(page.locator(SYNCED_BADGE_SELECTOR)).toBeVisible({ timeout: SYNC_TIMEOUT }); + if (typeIt) { + await editor.click(); + await page.keyboard.type(marker); + await expect(editor).toContainText(marker); + } + return slug; +} + +test('a committed navigate-away writes the edit exactly once', async ({ page, browser, fixture, request }, testInfo) => { + test.skip(testInfo.project.name !== 'desktop-chromium', 'one project is enough'); + test.setTimeout(90_000); + const marker = `nav-${Date.now()}`; + const slug = await setup(page, fixture, request, marker); + const before = await readVersionCount(page, fixture, slug); + + await page.goto('about:blank'); + await new Promise((r) => setTimeout(r, 3000)); + + const probe = await browser.newPage(); + await browserLogin(probe); + const after = await readVersionCount(probe, fixture, slug); + await probe.close(); + const writes = after - before; + console.log(`navigate-away EDITED: versions ${before} -> ${after} (content writes = ${writes})`); + // THE NUMBER THIS TEST EXISTS FOR. The re-arm is a setTimeout(0) inside + // beforeunload, and on a COMMITTED navigation the sequence is + // beforeunload -> [timer?] -> pagehide. If that timer ran first the latch + // would re-arm and pagehide would flush the same bytes again — two writes + // where main sent one. Measured at 1; if this ever reads 2, the re-arm must + // key on EVIDENCE the page lived on (a once-listener for pointerdown / + // keydown / focus) rather than on time. + expect(writes, 'a committed navigate-away wrote more than once').toBe(1); +}); + +test('a committed close writes the edit exactly once', async ({ page, browser, fixture, request }, testInfo) => { + test.skip(testInfo.project.name !== 'desktop-chromium', 'one project is enough'); + test.setTimeout(90_000); + const marker = `close-${Date.now()}`; + const slug = await setup(page, fixture, request, marker); + const before = await readVersionCount(page, fixture, slug); + + await page.close({ runBeforeUnload: true }); + await new Promise((r) => setTimeout(r, 3000)); + + const probe = await browser.newPage(); + await browserLogin(probe); + const after = await readVersionCount(probe, fixture, slug); + await probe.close(); + const writes = after - before; + console.log(`close EDITED: versions ${before} -> ${after} (content writes = ${writes})`); + expect(writes, 'a committed close wrote more than once').toBe(1); +}); + +// CONTROL: the same teardowns with NOTHING typed. Whatever this reports is +// ambient and must be subtracted from the legs above; if it is not 0 the edited +// numbers are not attributable to the teardown flush on their own. +test('CONTROL: navigate-away with nothing typed writes nothing', async ({ page, browser, fixture, request }, testInfo) => { + test.skip(testInfo.project.name !== 'desktop-chromium', 'one project is enough'); + test.setTimeout(90_000); + const slug = await setup(page, fixture, request, `ctl-nav-${Date.now()}`, false); + const before = await readVersionCount(page, fixture, slug); + await page.goto('about:blank'); + await new Promise((r) => setTimeout(r, 3000)); + const probe = await browser.newPage(); + await browserLogin(probe); + const after = await readVersionCount(probe, fixture, slug); + await probe.close(); + const writes = after - before; + console.log(`CONTROL navigate-away NO EDIT: versions ${before} -> ${after} (content writes = ${writes})`); + // Without this leg the 1s above are equally consistent with ambient traffic + // writing once per teardown regardless of the flush. + expect(writes, 'a teardown with nothing typed still wrote content').toBe(0); +}); + +test('CONTROL: close with nothing typed writes nothing', async ({ page, browser, fixture, request }, testInfo) => { + test.skip(testInfo.project.name !== 'desktop-chromium', 'one project is enough'); + test.setTimeout(90_000); + const slug = await setup(page, fixture, request, `ctl-close-${Date.now()}`, false); + const before = await readVersionCount(page, fixture, slug); + await page.close({ runBeforeUnload: true }); + await new Promise((r) => setTimeout(r, 3000)); + const probe = await browser.newPage(); + await browserLogin(probe); + const after = await readVersionCount(probe, fixture, slug); + await probe.close(); + const writes = after - before; + console.log(`CONTROL close NO EDIT: versions ${before} -> ${after} (content writes = ${writes})`); + expect(writes, 'a teardown with nothing typed still wrote content').toBe(0); +}); diff --git a/web/src/lib/collab/collabFlushTeardownDedupe.test.ts b/web/src/lib/collab/collabFlushTeardownDedupe.test.ts new file mode 100644 index 000000000..1ca818a00 --- /dev/null +++ b/web/src/lib/collab/collabFlushTeardownDedupe.test.ts @@ -0,0 +1,121 @@ +// The fact that justifies BUG-3030's once-per-teardown latch: the flusher's +// dedupe does NOT absorb a redundant flush while the save is still in flight. +// +// WHY THIS TEST EXISTS RATHER THAN A COMMENT. BUG-3030's fix registers the +// teardown flush on `pagehide` and `visibilitychange`→hidden as well as +// `beforeunload`, and a desktop tab close fires all three. The obvious +// reasoning — "the dedupe makes a redundant flush a no-op, so firing on more +// events costs nothing" — is TRUE in general and FALSE in exactly the case the +// teardown path runs in. `lastFlushedContent` is seeded only from a RESOLVED +// save (see `flush`), and the teardown call is deliberately fire-and-forget +// under `keepalive`, so on a real teardown nothing has resolved and the compare +// falls back to `ctx.baseline`, which a real edit does not match. +// +// The two legs are the evidence together; neither alone says anything. The +// resolved leg shows the dedupe genuinely works, so the in-flight leg cannot be +// dismissed as a broken dedupe; the in-flight leg shows it does not reach the +// case that matters, so the latch in ItemDetail is not belt-and-braces. +import { describe, expect, it } from 'vitest'; +import { createCollabFlusher, type CollabFlushContext, type CollabFlusherConfig } from './collabFlush.svelte'; + +const EDITED = 'edited markdown'; + +const ctx: CollabFlushContext = { + wsSlug: 'w', + itemId: 'i', + baseline: 'ORIGINAL', + seedMd: 'ORIGINAL', +} as CollabFlushContext; + +function makeFlusher(opts: { resolveSave: boolean }) { + const saves: string[] = []; + const release: Array<() => void> = []; + const config: CollabFlusherConfig = { + idleMs: 1000, + isRecovering: () => false, + normalize: (m) => m, + serialize: (m) => m, + readEditorMarkdown: () => EDITED, + isActiveItem: () => true, + save: async (input) => { + saves.push(input.toSave); + if (!opts.resolveSave) { + // Model the real teardown: the keepalive PATCH is dispatched and + // has NOT resolved when the next lifecycle event fires. + await new Promise((r) => release.push(r)); + } + return 'flushed'; + }, + }; + return { flusher: createCollabFlusher(config), saves, release }; +} + +describe('collab flush dedupe on the teardown path', () => { + it('does NOT dedupe redundant flushes while the keepalive save is in flight', async () => { + const { flusher, saves } = makeFlusher({ resolveSave: false }); + + // visibilitychange -> pagehide -> beforeunload, as a tab close delivers + // them, each fire-and-forget exactly as the teardown path calls it. + flusher.flushNow(ctx, true); + flusher.flushNow(ctx, true); + flusher.flushNow(ctx, true); + await Promise.resolve(); + + expect( + saves.length, + 'if this is 1, the dedupe now absorbs in-flight teardown flushes and the ' + + 'once-latch in ItemDetail is no longer load-bearing — re-derive it before deleting it', + ).toBe(3); + expect(flusher.lastFlushed, 'nothing resolved, so no baseline can have been recorded').toBeNull(); + }); + + it('DOES dedupe once the save resolves between flushes', async () => { + // The control. Without it, the leg above is equally consistent with a + // dedupe that never works at all, and would say nothing about teardown. + const { flusher, saves } = makeFlusher({ resolveSave: true }); + + expect(await flusher.flush(ctx, EDITED, true)).toBe('flushed'); + expect(await flusher.flush(ctx, EDITED, true)).toBe('deduped'); + expect(await flusher.flush(ctx, EDITED, true)).toBe('deduped'); + + expect(saves.length, 'the dedupe should collapse resolved repeats to one PATCH').toBe(1); + expect(flusher.lastFlushed).toBe(EDITED); + }); + + it('writes NOTHING when a teardown fires with nothing changed', async () => { + // BUG-3030 adds two more events that can fire a teardown flush, and the + // commonest one by far is a user backgrounding a tab they only READ. + // If merely hiding a tab wrote content, the fix would turn every glance + // at an item into a version and a bumped updated_at. + // + // The baseline arm is what prevents it: before any flush this session, + // `lastFlushedContent` is null and the compare falls back to + // `ctx.baseline` — the content as loaded — so an unedited document + // dedupes without a PATCH. A real edit changes `toSave` and still goes. + const saves: string[] = []; + const config: CollabFlusherConfig = { + idleMs: 1000, + isRecovering: () => false, + normalize: (m) => m, + serialize: (m) => m, + // The editor holds exactly what was loaded — nothing was typed. + readEditorMarkdown: () => 'ORIGINAL', + isActiveItem: () => true, + save: async (input) => { + saves.push(input.toSave); + return 'flushed'; + }, + }; + const flusher = createCollabFlusher(config); + + expect(await flusher.flush(ctx, 'ORIGINAL', true)).toBe('deduped'); + flusher.flushNow(ctx, true); + await Promise.resolve(); + expect(saves, 'hiding a tab nobody edited wrote content').toEqual([]); + + // PRECONDITION, so the assertion above cannot pass by the harness never + // being able to write at all. + expect(await flusher.flush(ctx, 'EDITED', true)).toBe('flushed'); + expect(saves).toEqual(['EDITED']); + }); +}); diff --git a/web/src/lib/components/items/ItemDetail.svelte b/web/src/lib/components/items/ItemDetail.svelte index 902c41b37..6042e2025 100644 --- a/web/src/lib/components/items/ItemDetail.svelte +++ b/web/src/lib/components/items/ItemDetail.svelte @@ -2613,56 +2613,144 @@ }; }); - // beforeunload: same flush as $effect cleanup, but routed - // through the page lifecycle so navigation off-site (close tab, - // reload, follow external link) lands the markdown snapshot - // before the WS dies. fetch keepalive: true is the modern - // equivalent of sendBeacon for non-POST requests; supports up to - // ~64KB body which dwarfs typical markdown items. + // TEARDOWN FLUSH ON THE EVENTS A SUSPENDED TAB ACTUALLY DELIVERS (BUG-3030). + // + // This used to be registered on `beforeunload` alone. That is the event + // LEAST likely to fire on the platforms that lose data: mobile + // backgrounding, tab discard under memory pressure, and bfcache navigation + // can all end a page without it. The file next door already compensates for + // exactly that behaviour — wsProvider's reconnect logic exists because "iOS + // Safari (and other mobile suspends) can silently kill the WS transport" — + // so one module was recovering from suspension while this one depended on + // an event suspension skips. The last tab could close with items.content + // still behind the op-log, which is BUG-3000's stale window arriving + // through ordinary use rather than through a crash. + // + // WHAT IS MEASURED HERE AND WHAT IS NOT. Measured, in jsdom: that each of + // these events routes through the one guarded flush, that it fires ONCE per + // teardown, and that a restore re-arms it. NOT measured by anything in this + // repo: the web-platform claim underneath — that `beforeunload` is + // unreliable on mobile and these are the events that survive. That is + // asserted from the platform's documented lifecycle, not from a run against + // a device. So the honest claim is "the flush now also runs on the events a + // suspended tab does deliver", not "this fixes mobile". + // + // `beforeunload` STAYS rather than being replaced, and the reason is the + // PROMPT, not preference: it is the only one of these that can raise the + // native unsaved-changes dialog via preventDefault(). Dropping it to buy + // bfcache eligibility would trade a user-visible protection for a benefit + // this unit cannot measure. + let teardownFlushed = false; + + // ONE function, called by every entry point, rather than the same guard + // copied into each handler — three copies of a guard is how the third ends + // up missing it, and a source guard cannot see the copy that was never + // written. + function runTeardownFlush(): void { + // ONCE PER TEARDOWN, and this latch is load-bearing rather than tidy. + // MEASURED (BUG-3030): three lifecycle events produce THREE identical + // PATCHes when the save is still in flight, and only one when it + // resolves between them. The flusher's dedupe compares against + // `lastFlushedContent`, which is seeded only from a RESOLVED save — and + // the teardown path is deliberately fire-and-forget under keepalive, so + // on a real teardown it has not resolved. A desktop tab close fires + // visibilitychange→hidden, pagehide AND beforeunload, so without this + // the dedupe absorbs nothing precisely where it is needed. + if (teardownFlushed) return; + // The same discard rule as the $effect cleanup and the raw saver + // (BUG-3005): a teardown write after an identity change carries the + // wrong user's cookie, and both paths below use `keepalive`, a request + // built to outlive the page. Guarding here covers every entry point by + // construction. + if (authStore.identityEpoch !== identityEpochAtLoad) return; + teardownFlushed = true; + + // Collab path: flush the live Y.Doc snapshot. + const ctx = activeCollabContext; + if (ctx) collabFlusher.flushNow(ctx, true); + + // Raw-markdown path (BUG-2024). The saver's pending markdown is the + // exact debounced-but-unsaved edit; when dirty there is up to ~1.2s of + // typing the collab flush above never sees. flushNow cancels the queued + // debounce (so it can't fire a second, older-content PATCH) and fires an + // immediate keepalive PATCH that survives page teardown. + if (rawContentSaver.dirty && item) { + rawContentSaver.flushNow({ keepalive: true }); + } + } + $effect(() => { if (typeof window === 'undefined') return; const onBeforeUnload = (event: BeforeUnloadEvent) => { // WHEN THE IDENTITY MOVED, THIS HANDLER DOES NOTHING AT ALL - // (BUG-3005, codex round 4). Two separate defects, both created by - // the fix that reloads the tab on an identity change: - // - // - it FLUSHES. Both paths below persist editor content with - // `keepalive`, and the unload they run in is the identity - // reload's own — so A's Y.Doc snapshot and A's pending markdown - // would be PATCHed carrying B's cookie. The raw saver has its - // own guard; the collab flush did not, and my grep for - // `keepalive: true` missed it because it takes the flag - // positionally. Guarding here covers both by construction. - // - it PROMPTS. `preventDefault()` on a dirty editor raises the - // native "unsaved changes" dialog, and "Stay" CANCELS the - // reload — leaving B in A's page shell with the stores already - // cleared and no remount left to rebuild it. The prompt exists - // to protect the author's unsaved work, and after an identity - // change the author is not the one sitting here. + // (BUG-3005, codex round 4). `runTeardownFlush` carries the same + // check for the WRITE half; this one is not redundant with it, + // because it guards a second and different thing: the PROMPT. + // `preventDefault()` on a dirty editor raises the native "unsaved + // changes" dialog, and "Stay" CANCELS an identity reload — leaving + // the new user in the previous user's page shell with the stores + // already cleared and no remount left to rebuild it. The prompt + // protects the author's unsaved work, and after an identity change + // the author is not the one sitting here. if (authStore.identityEpoch !== identityEpochAtLoad) return; - // Collab path: flush the live Y.Doc snapshot (unchanged). - const ctx = activeCollabContext; - if (ctx) collabFlusher.flushNow(ctx, true); + runTeardownFlush(); - // Raw-markdown path (BUG-2024). The saver's pending markdown is - // the exact debounced-but-unsaved edit; when dirty there is up - // to ~1.2s of typing the collab flush above never sees. Only - // when actually dirty — never warn on a clean page. flushNow - // cancels the queued debounce (so it can't fire a second, - // older-content PATCH) and fires an immediate keepalive PATCH - // that survives page teardown. The saver's `save` callback - // clears the dirty state once it lands (covers the user - // cancelling the navigation via "Stay"). See rawContentSaver. + // The prompt is decided INDEPENDENTLY of the latch. If an earlier + // pagehide already flushed, `dirty` is still true — the saver clears + // it only when the PATCH lands — so the user is still warned about + // work that has not been confirmed saved. Tying the prompt to the + // latch would silently stop warning whenever a visibilitychange + // happened to fire first. if (rawContentSaver.dirty && item) { - rawContentSaver.flushNow({ keepalive: true }); - // Native "unsaved changes" prompt. event.preventDefault(); event.returnValue = ''; } + + // RE-ARM IF THE PAGE SURVIVES THIS (codex round 2). + // + // `beforeunload` is the one teardown event that can be CANCELLED: + // preventDefault raises the native dialog and "Stay" leaves the page + // alive, visible, and never firing `pageshow` or `visibilitychange` + // — so neither re-arm above can reach it. Without this the latch + // stays true for the rest of the page's life, and every later edit + // and every later close flushes NOTHING. That is a worse data loss + // than the one this whole change exists to fix, reachable by the + // ordinary act of changing your mind about closing a tab. + // + // A macrotask is the discriminator, and it needs no guess about + // which choice the user made: if the navigation proceeds the page is + // gone and this never runs; if it was cancelled the event loop keeps + // turning and the latch re-arms. Re-arming is safe either way — the + // flush already dispatched, and a second teardown SHOULD flush + // again, because by then the content may have changed. + setTimeout(() => { + teardownFlushed = false; + }, 0); + }; + // The two events a suspended or discarded tab actually delivers. + const onPageHide = () => runTeardownFlush(); + const onVisibilityChange = () => { + if (document.visibilityState === 'hidden') runTeardownFlush(); + else teardownFlushed = false; + }; + // A page restored from bfcache is alive again and will be torn down + // again later, so the latch must re-arm or that second teardown flushes + // nothing. `pageshow` covers the bfcache restore; the visible branch + // above covers a tab the user simply switched back to. + const onPageShow = () => { + teardownFlushed = false; }; window.addEventListener('beforeunload', onBeforeUnload); - return () => window.removeEventListener('beforeunload', onBeforeUnload); + window.addEventListener('pagehide', onPageHide); + window.addEventListener('pageshow', onPageShow); + document.addEventListener('visibilitychange', onVisibilityChange); + return () => { + window.removeEventListener('beforeunload', onBeforeUnload); + window.removeEventListener('pagehide', onPageHide); + window.removeEventListener('pageshow', onPageShow); + document.removeEventListener('visibilitychange', onVisibilityChange); + }; }); // Lazy seed (TASK-1261 / PLAN-1248). When a fresh collab session diff --git a/web/src/lib/components/items/itemDetailIdentityDiscardsTeardownWrites.test.ts b/web/src/lib/components/items/itemDetailIdentityDiscardsTeardownWrites.test.ts index 68a6ee917..cdd6bf551 100644 --- a/web/src/lib/components/items/itemDetailIdentityDiscardsTeardownWrites.test.ts +++ b/web/src/lib/components/items/itemDetailIdentityDiscardsTeardownWrites.test.ts @@ -33,6 +33,23 @@ function stripComments(src: string): string { const CODE = stripComments(SRC); +/** The body of a top-level function, or a thrown failure. + * + * FAILS CLOSED, and that is the whole reason it exists (codex round 4 [P2]). + * The first version did `CODE.slice(fn, CODE.indexOf('\n\t}', fn))`, and when + * that indexOf finds nothing it returns -1 — so `slice(fn, -1)` scanned nearly + * the rest of the file and let unrelated flush/guard occurrences elsewhere in + * a 7,900-line component satisfy assertions about THIS function. A guard whose + * failure mode is to widen its own scope reports success for a source it never + * checked. */ +function functionBody(marker: string): string { + const start = CODE.indexOf(marker); + if (start < 0) throw new Error(`${marker} was renamed or removed — re-point this guard`); + const end = CODE.indexOf('\n\t}', start); + if (end <= start) throw new Error(`could not find the end of ${marker} — re-point this guard`); + return CODE.slice(start, end); +} + describe('ItemDetail teardown writes under an identity change', () => { it('captures the identity epoch at LOAD and re-stamps it in loadData', () => { // The comparison is only meaningful against the identity whose typing @@ -44,20 +61,161 @@ describe('ItemDetail teardown writes under an identity change', () => { expect(upToFirstAwait).toContain('identityEpochAtLoad = authStore.identityEpoch'); }); - it('makes the identity check the FIRST thing beforeunload does', () => { - // Ahead of both flushes AND ahead of `preventDefault`, or the handler - // still writes or still blocks the reload. + it('routes every teardown write through the one guarded function', () => { + // RE-POINTED BY BUG-3030, and the re-point is the point. This used to + // slice a 1200-character POSITION WINDOW out of the beforeunload handler + // and check the guard preceded the two flushes inside it. BUG-3030 added + // `pagehide` and `visibilitychange` handlers that also write — and a + // window anchored on one handler's spelling would have kept PASSING + // while no longer covering the teardown writes at all. That is the + // containment failure from BUG-2840 in miniature: the window felt like + // it covered the subject because it covered the code I was looking at. + // + // So the shape changed. There is now ONE writing function, the guard + // lives inside it, and the enumeration below FAILS CLOSED on a teardown + // listener it has not been told about. + const body = functionBody('function runTeardownFlush()'); + + const guard = body.indexOf('authStore.identityEpoch !== identityEpochAtLoad'); + expect(guard, 'the teardown flush does not check the identity epoch').toBeGreaterThan(-1); + for (const persisting of ['collabFlusher.flushNow', 'rawContentSaver.flushNow']) { + const at = body.indexOf(persisting); + expect(at, `${persisting} is no longer in runTeardownFlush — re-point this guard`).toBeGreaterThan(-1); + expect(at, `${persisting} runs BEFORE the identity check`).toBeGreaterThan(guard); + } + + // FAIL CLOSED on a teardown listener that writes directly instead of + // going through the guarded function — it would bypass both the identity + // check and the once-latch, and the assertions above would never see it. + // + // SCOPED TO THE LISTENER BLOCK, deliberately. The first version of this + // asked whether ANY keepalive raw flush existed outside + // runTeardownFlush, and it fired — on `loadData`'s item-switch flush + // (PLAN-2105 / TASK-2112), which is a legitimately different thing: it + // is not a teardown, it MUST NOT be latched (every switch owes its own + // flush), and its identity protection lives inside the saver's own + // `save` callback, pinned by the third test below. A real site, and the + // wrong question — the guard's question was wider than the claim resting + // on it, which is the failure this file exists to catch in the source. + // FAIL CLOSED on a teardown listener that writes without going through + // the guarded function. Rewritten after codex round 4 [P1]: the first + // version rejected two literal spellings and then only checked that + // `runTeardownFlush()` appeared SOMEWHERE in the block — so a new + // handler writing through any other call, or through an alias, passed. + // "No forbidden spelling is present" is a much weaker claim than "every + // registered handler routes through the guard", and it was the second + // one this test was named for. + // + // So the registrations are ENUMERATED and each handler is checked + // individually, with an unknown handler failing rather than being + // ignored. + const reg = CODE.indexOf("const onBeforeUnload = (event: BeforeUnloadEvent) => {"); + expect(reg, 'the teardown listener block was restructured — re-point this guard').toBeGreaterThan(-1); + const regEnd = CODE.indexOf("document.removeEventListener('visibilitychange'", reg); + expect(regEnd, 'the teardown listener block was restructured — re-point this guard').toBeGreaterThan(reg); + const listeners = CODE.slice(reg, regEnd); + + const registered = [...listeners.matchAll(/addEventListener\('([a-z]+)',\s*(\w+)\)/g)].map( + (m) => ({ event: m[1], handler: m[2] }), + ); + expect(registered.length, 'no teardown registrations found — re-point this guard').toBeGreaterThan(2); + + // `pageshow` is the one registration that deliberately does NOT write: + // it only re-arms the latch. Every other one must route through the + // guarded function. + const nonWriting = new Set(['pageshow']); + for (const { event, handler } of registered) { + const decl = `const ${handler} = `; + const at = listeners.indexOf(decl); + expect(at, `handler ${handler} for ${event} is not declared in this block`).toBeGreaterThan(-1); + const next = registered + .map((r) => listeners.indexOf(`const ${r.handler} = `)) + .filter((i) => i > at) + .sort((a, b) => a - b)[0]; + const bodyText = listeners.slice(at, next === undefined ? listeners.indexOf('window.addEventListener') : next); + if (nonWriting.has(event)) { + expect( + bodyText.includes('runTeardownFlush'), + `${event} is listed as non-writing but calls the flush`, + ).toBe(false); + continue; + } + expect( + bodyText, + `the ${event} handler does not route through runTeardownFlush() — it would bypass the identity guard and the once-latch`, + ).toContain('runTeardownFlush()'); + for (const direct of ['collabFlusher.flushNow', 'rawContentSaver.flushNow']) { + expect( + bodyText.includes(direct), + `the ${event} handler calls ${direct} directly instead of runTeardownFlush()`, + ).toBe(false); + } + } + }); + + it('fires the teardown flush ONCE, and re-arms only on a restore', () => { + // The latch is justified by measurement, not tidiness: three lifecycle + // events produce THREE identical PATCHes while the keepalive save is in + // flight, and one when it resolves between them (BUG-3030). The teardown + // path is fire-and-forget, so in-flight is the case that actually runs. + const body = functionBody('function runTeardownFlush()'); + expect(body).toMatch(/if\s*\(teardownFlushed\)\s*return/); + expect(body).toMatch(/teardownFlushed\s*=\s*true/); + + // Re-arm on the two ways a torn-down page comes back, or its NEXT + // teardown flushes nothing. + expect(CODE).toMatch(/onPageShow\s*=\s*\(\)\s*=>\s*\{[^}]*teardownFlushed\s*=\s*false/); + expect(CODE).toMatch(/else\s+teardownFlushed\s*=\s*false/); + }); + + it('re-arms the latch when a cancelled beforeunload leaves the page alive', () => { + // "Stay" on the native dialog cancels the navigation: the page stays + // visible and fires neither `pageshow` nor `visibilitychange`, so + // neither of the other re-arms can reach it. A latch left set there + // silently swallows every later teardown flush for the life of the page + // — a worse loss than the one this change fixes, reached by the ordinary + // act of changing your mind about closing a tab (codex round 2). + const start = CODE.indexOf('const onBeforeUnload = (event: BeforeUnloadEvent) => {'); + const end = CODE.indexOf('const onPageHide', start); + expect(end, 'the listener block was restructured — re-point this guard').toBeGreaterThan(start); + const handler = CODE.slice(start, end); + expect( + handler, + 'beforeunload does not re-arm the latch, so a cancelled unload leaves it set forever', + ).toMatch(/setTimeout\(\s*\(\)\s*=>\s*\{\s*teardownFlushed\s*=\s*false;?\s*\}\s*,\s*0\s*\)/); + // AFTER the flush, or it defeats the latch it is meant to release. + expect(handler.indexOf('setTimeout')).toBeGreaterThan(handler.indexOf('runTeardownFlush()')); + }); + + it('registers the flush on the events a suspended tab delivers', () => { + // beforeunload alone is the defect (BUG-3030). It is kept — it is the + // only one that can raise the unsaved-changes prompt — but it is no + // longer the only registration. + for (const ev of ['beforeunload', 'pagehide', 'pageshow']) { + expect(CODE, `${ev} is not registered`).toContain(`window.addEventListener('${ev}'`); + } + expect(CODE).toContain("document.addEventListener('visibilitychange'"); + // And each is removed again, or a remount leaves a handler holding a + // dead component's context. + for (const ev of ['beforeunload', 'pagehide', 'pageshow']) { + expect(CODE, `${ev} is never removed`).toContain(`window.removeEventListener('${ev}'`); + } + expect(CODE).toContain("document.removeEventListener('visibilitychange'"); + }); + + it('keeps the unsaved-changes prompt guarded and independent of the latch', () => { const start = CODE.indexOf('const onBeforeUnload = (event: BeforeUnloadEvent) => {'); expect(start, 'the beforeunload handler was renamed or removed').toBeGreaterThan(-1); const body = CODE.slice(start, start + 1200); const guard = body.indexOf('authStore.identityEpoch !== identityEpochAtLoad'); - expect(guard, 'beforeunload does not check the identity epoch').toBeGreaterThan(-1); - - for (const persisting of ['collabFlusher.flushNow', 'rawContentSaver.flushNow', 'preventDefault']) { - const at = body.indexOf(persisting); - expect(at, `${persisting} is no longer in the handler — re-point this guard`).toBeGreaterThan(-1); - expect(at, `${persisting} runs BEFORE the identity check`).toBeGreaterThan(guard); - } + expect(guard, 'beforeunload does not check the identity epoch before prompting').toBeGreaterThan(-1); + const prevent = body.indexOf('preventDefault'); + expect(prevent, 'preventDefault is no longer in the handler — re-point this guard').toBeGreaterThan(-1); + expect(prevent, 'preventDefault runs BEFORE the identity check').toBeGreaterThan(guard); + // The prompt must NOT be inside the latch: if an earlier pagehide + // flushed, `dirty` is still true until the PATCH lands, and the user + // must still be warned. + expect(body).toMatch(/if\s*\(rawContentSaver\.dirty\s*&&\s*item\)\s*\{\s*event\.preventDefault/); }); it('gates the collab cleanup flush on the identity too', () => {