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', () => {