From bf9c5a63476dd15f86d28ed51a20036cfb72fa7f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Fiere?= Date: Sun, 16 Aug 2026 18:50:25 +0200 Subject: [PATCH] fix(web): reload app on bfcache restore so duplicated tabs show fresh data (#2717) --- .../layout/bfcache-restore-reload.spec.ts | 37 ++ apps/web/src/bfcache-restore-reload.test.ts | 224 +++++++++++ apps/web/src/bfcache-restore-reload.ts | 83 ++++ apps/web/src/main.tsx | 2 + .../fix-duplicated-tab-stale-data/plan.md | 110 +++++ .../task-01-reload-on-bfcache-restore.md | 375 ++++++++++++++++++ docs/specs/INDEX.md | 1 + .../fix-duplicated-tab-stale-data/spec.md | 115 ++++++ 8 files changed, 947 insertions(+) create mode 100644 apps/web/e2e/tests/layout/bfcache-restore-reload.spec.ts create mode 100644 apps/web/src/bfcache-restore-reload.test.ts create mode 100644 apps/web/src/bfcache-restore-reload.ts create mode 100644 docs/plans/fix-duplicated-tab-stale-data/plan.md create mode 100644 docs/plans/fix-duplicated-tab-stale-data/task-01-reload-on-bfcache-restore.md create mode 100644 docs/specs/fix-duplicated-tab-stale-data/spec.md diff --git a/apps/web/e2e/tests/layout/bfcache-restore-reload.spec.ts b/apps/web/e2e/tests/layout/bfcache-restore-reload.spec.ts new file mode 100644 index 0000000000..7e8d642b72 --- /dev/null +++ b/apps/web/e2e/tests/layout/bfcache-restore-reload.spec.ts @@ -0,0 +1,37 @@ +import { expect, test } from "../../fixtures/test-base"; + +test.describe("bfcache restore reload", () => { + test("reloads the app when the page is restored from a frozen snapshot", async ({ testPage }) => { + await testPage.goto("/"); + + // A normal load is not a restore: the document must not reload by itself. + expect(await testPage.evaluate(() => performance.getEntriesByType("navigation")[0]?.type)).toBe( + "navigate", + ); + + // Chrome's "Duplicate tab" (and any bfcache restore) brings the page back + // from a frozen snapshot and fires pageshow with persisted=true. The app + // reloads so the no-store boot payload is re-fetched instead of showing + // the frozen state. Arm the causal wait BEFORE dispatching: the frame + // navigation IS the cause, so we wait on it rather than budgeting the + // effect. + const reloaded = testPage.waitForEvent("framenavigated"); + await testPage + .evaluate(() => { + window.dispatchEvent(new PageTransitionEvent("pageshow", { persisted: true })); + }) + .catch(() => undefined); // the reload may tear down the evaluate context + await reloaded; + + // The fresh document reports the reload navigation type. Poll with the + // default assertion timeout; the entry exists as soon as the frame + // commits, and the catch tolerates the mid-navigation context teardown. + await expect + .poll(() => + testPage + .evaluate(() => performance.getEntriesByType("navigation")[0]?.type) + .catch(() => undefined), + ) + .toBe("reload"); + }); +}); diff --git a/apps/web/src/bfcache-restore-reload.test.ts b/apps/web/src/bfcache-restore-reload.test.ts new file mode 100644 index 0000000000..3c6535aa6a --- /dev/null +++ b/apps/web/src/bfcache-restore-reload.test.ts @@ -0,0 +1,224 @@ +import { afterEach, describe, expect, it, vi } from "vitest"; +import { installBfcacheRestoreReload } from "./bfcache-restore-reload"; + +const RESTORE_PROBE_KEY = "kandev.bfcacheRestoreProbe"; + +function createMemoryStorage() { + const values = new Map(); + return { + storage: { + setItem: vi.fn((key: string, value: string) => values.set(key, value)), + }, + value: () => values.get(RESTORE_PROBE_KEY) ?? null, + }; +} + +function createHarness({ + isDebug = () => false, + storage = createMemoryStorage().storage, + navigationType, + injectNavigationType = true, +}: { + isDebug?: () => boolean; + storage?: Pick; + navigationType?: string; + injectNavigationType?: boolean; +} = {}) { + const target = new EventTarget(); + const reload = vi.fn(); + const getNavigationType = vi.fn(() => navigationType); + const cleanup = installBfcacheRestoreReload({ + target, + reload, + isDebug, + getStorage: () => storage, + ...(injectNavigationType ? { getNavigationType } : {}), + }); + + return { + target, + reload, + storage, + getNavigationType, + cleanup, + dispatch(persisted: boolean) { + const event = new Event("pageshow"); + Object.defineProperty(event, "persisted", { value: persisted }); + target.dispatchEvent(event); + return event; + }, + }; +} + +afterEach(() => { + vi.unstubAllGlobals(); +}); + +describe("installBfcacheRestoreReload", () => { + it("reloads when the page is restored from a frozen snapshot (persisted=true)", () => { + const harness = createHarness(); + + harness.dispatch(true); + + expect(harness.reload).toHaveBeenCalledOnce(); + }); + + it("does not reload on a cold back/forward traversal (persisted=false)", () => { + // Regression for the round-1 finding: a history traversal not served + // from bfcache (e.g. an open WebSocket made the no-store page ineligible) + // loads fresh and must not be reloaded a second time, even when the + // navigation entry reports type `back_forward`. The injected + // navigation-type reader returns `back_forward` here, so reintroducing + // the navigation-type reload fallback would fail this test. + const harness = createHarness({ navigationType: "back_forward" }); + + harness.dispatch(false); + + expect(harness.reload).not.toHaveBeenCalled(); + }); + + it("does not reload on a cold back/forward traversal via the default reader (persisted=false)", () => { + // Companion to the injected-seam case: a reintroduced fallback that calls + // the module's DEFAULT readNavigationType (reading global performance) + // must also fail. Stub global performance to report back_forward, install + // without the injected reader, and assert no reload. + vi.stubGlobal("performance", { + // happy-dom's Event constructor calls performance.now(); keep it intact + // while exposing the back_forward navigation entry. + now: () => 0, + getEntriesByType: (type: string) => (type === "navigation" ? [{ type: "back_forward" }] : []), + }); + const harness = createHarness({ injectNavigationType: false }); + + harness.dispatch(false); + + expect(harness.reload).not.toHaveBeenCalled(); + }); + + it("does not reload on a fresh load (persisted=false)", () => { + // navigationType: "navigate" distinguishes this from the manual-refresh + // case below, so a reintroduced fallback keyed on either type fails. + const harness = createHarness({ navigationType: "navigate" }); + + harness.dispatch(false); + + expect(harness.reload).not.toHaveBeenCalled(); + }); + + it("does not reload on a manual refresh (persisted=false)", () => { + const harness = createHarness({ navigationType: "reload" }); + + harness.dispatch(false); + + expect(harness.reload).not.toHaveBeenCalled(); + }); + + it("does not reload when the event carries no persisted flag", () => { + const harness = createHarness(); + + harness.target.dispatchEvent(new Event("pageshow")); + + expect(harness.reload).not.toHaveBeenCalled(); + }); + + it("uninstall removes the listener", () => { + const harness = createHarness(); + + harness.cleanup(); + harness.dispatch(true); + + expect(harness.reload).not.toHaveBeenCalled(); + }); +}); + +describe("installBfcacheRestoreReload debug probe", () => { + it("records restore evidence before reloading in debug builds", () => { + const memory = createMemoryStorage(); + const harness = createHarness({ + isDebug: () => true, + storage: memory.storage, + navigationType: "back_forward", + }); + + harness.dispatch(true); + + expect(harness.reload).toHaveBeenCalledOnce(); + expect(memory.value()).not.toBeNull(); + expect(JSON.parse(memory.value()!)).toMatchObject({ + persisted: true, + navigationType: "back_forward", + }); + expect(Number.isFinite(JSON.parse(memory.value()!).at)).toBe(true); + }); + + it("records a persisted=false duplicate candidate without reloading", () => { + // The merge gate must be able to distinguish "no pageshow at all" from + // "pageshow with persisted=false": a duplicate that restores stale heap + // state while reporting persisted=false (a cold back_forward traversal) + // is recorded for diagnosis but never reloaded. + const memory = createMemoryStorage(); + const harness = createHarness({ + isDebug: () => true, + storage: memory.storage, + navigationType: "back_forward", + }); + + harness.dispatch(false); + + expect(harness.reload).not.toHaveBeenCalled(); + expect(memory.value()).not.toBeNull(); + expect(JSON.parse(memory.value()!)).toMatchObject({ + persisted: false, + navigationType: "back_forward", + }); + }); + + it("does not record non-candidates even in debug builds", () => { + const memory = createMemoryStorage(); + const harness = createHarness({ + isDebug: () => true, + storage: memory.storage, + navigationType: "navigate", + }); + + harness.dispatch(false); + + expect(harness.reload).not.toHaveBeenCalled(); + expect(memory.value()).toBeNull(); + }); + + it("does not record evidence when debug is disabled", () => { + const memory = createMemoryStorage(); + const harness = createHarness({ isDebug: () => false, storage: memory.storage }); + + harness.dispatch(true); + + expect(harness.reload).toHaveBeenCalledOnce(); + expect(memory.value()).toBeNull(); + }); + + it("a failing probe never blocks the reload", () => { + const harness = createHarness({ + isDebug: () => true, + storage: { + setItem: vi.fn(() => { + throw new Error("storage unavailable"); + }), + }, + }); + + expect(() => harness.dispatch(true)).not.toThrow(); + expect(harness.reload).toHaveBeenCalledOnce(); + }); + + it("a throwing debug predicate never blocks the reload", () => { + const harness = createHarness({ + isDebug: () => { + throw new Error("debug flag unavailable"); + }, + }); + + expect(() => harness.dispatch(true)).not.toThrow(); + expect(harness.reload).toHaveBeenCalledOnce(); + }); +}); diff --git a/apps/web/src/bfcache-restore-reload.ts b/apps/web/src/bfcache-restore-reload.ts new file mode 100644 index 0000000000..631a4e660b --- /dev/null +++ b/apps/web/src/bfcache-restore-reload.ts @@ -0,0 +1,83 @@ +const PAGESHOW_EVENT = "pageshow"; +const RESTORE_PROBE_KEY = "kandev.bfcacheRestoreProbe"; + +type RestoreReloadTarget = Pick; + +interface RestoreReloadOptions { + target?: RestoreReloadTarget; + reload?: () => void; + isDebug?: () => boolean; + getStorage?: () => Pick; + getNavigationType?: () => string | undefined; +} + +function readNavigationType(): string | undefined { + try { + const entry = performance.getEntriesByType("navigation")[0]; + return entry ? (entry as PerformanceNavigationTiming).type : undefined; + } catch { + return undefined; + } +} + +/** + * Reloads the page when it is restored from a frozen browser snapshot + * (back/forward cache restore, including Chrome's "Duplicate tab") instead of + * being loaded fresh. A restored page keeps the frozen JS heap and DOM, so + * state can be stale until a real load; the reload re-fetches the no-store + * boot payload and reconnects the WebSocket. + * + * A restore is detected on `pageshow` when `event.persisted` is true. That is + * the only reliable frozen-restore signal: the navigation type + * `back_forward` also covers cold history traversals and session-restored + * tabs, which load fresh and must NOT be reloaded a second time. Fresh loads + * (`navigate`), manual refreshes (`reload`), and in-app SPA routing never + * reload; a reload produces a fresh document and cannot loop. + * + * In debug builds (`window.__KANDEV_DEBUG`), restore evidence is written to + * sessionStorage BEFORE the reload so a native Duplicate-tab run can be + * verified after the fact: the reload destroys the document, and the + * duplicated tab is a separate DevTools target, so the event data must be + * persisted pre-reload. For duplicate/history candidates the actual + * `persisted` value is recorded even when it is false, so the merge gate can + * distinguish "no pageshow at all" from "pageshow with persisted=false". The + * probe is diagnostics only, is skipped outside debug builds, and never + * blocks the reload. + * + * Returns an uninstall function. + */ +export function installBfcacheRestoreReload({ + target = window, + reload = () => window.location.reload(), + isDebug = () => window.__KANDEV_DEBUG === true, + getStorage = () => window.sessionStorage, + getNavigationType = readNavigationType, +}: RestoreReloadOptions = {}): () => void { + const recordProbe = (persisted: boolean) => { + try { + if (!isDebug()) return; + const navigationType = getNavigationType(); + // Only duplicate/history candidates are probed: a frozen restore, or + // any document reached via a `back_forward` traversal. Fresh loads and + // manual refreshes are not candidates and are never recorded. + if (!persisted && navigationType !== "back_forward") return; + getStorage().setItem( + RESTORE_PROBE_KEY, + JSON.stringify({ persisted, navigationType, at: Date.now() }), + ); + } catch { + // Best-effort diagnostics; never block the reload. + } + }; + + const handlePageshow = (event: Event) => { + const persisted = (event as PageTransitionEvent).persisted === true; + recordProbe(persisted); + if (persisted) { + reload(); + } + }; + + target.addEventListener(PAGESHOW_EVENT, handlePageshow); + return () => target.removeEventListener(PAGESHOW_EVENT, handlePageshow); +} diff --git a/apps/web/src/main.tsx b/apps/web/src/main.tsx index 888e1ec6a8..c495ad23ad 100644 --- a/apps/web/src/main.tsx +++ b/apps/web/src/main.tsx @@ -13,10 +13,12 @@ import type { BootPayload } from "./boot-payload"; import { RootErrorBoundary, RouteErrorBoundary } from "./app-error-boundary"; import { SpaRoutes } from "./spa-routes"; import { installVitePreloadRecovery } from "./vite-preload-recovery"; +import { installBfcacheRestoreReload } from "./bfcache-restore-reload"; import { markRenderingEngine } from "@/lib/browser/rendering-engine"; import { applyTitlePrefix } from "@/lib/browser/document-title"; installVitePreloadRecovery(); +installBfcacheRestoreReload(); markRenderingEngine(document.documentElement); const AUTH_ROUTE_PATHS = new Set(["/login", "/setup", "/invite"]); diff --git a/docs/plans/fix-duplicated-tab-stale-data/plan.md b/docs/plans/fix-duplicated-tab-stale-data/plan.md new file mode 100644 index 0000000000..2edb46520a --- /dev/null +++ b/docs/plans/fix-duplicated-tab-stale-data/plan.md @@ -0,0 +1,110 @@ +--- +spec: docs/specs/fix-duplicated-tab-stale-data/spec.md +created: 2026-08-16 +status: in-progress +--- + +# Implementation Plan: Reload on Frozen-Snapshot Restore + +## Overview + +Chrome's Duplicate tab restores the Kandev page from a frozen browser snapshot +(back/forward-style restore; the duplicated tab's navigation type is +`back_forward`) instead of performing a fresh load. The restored page keeps the +frozen JS heap and DOM, so data that changed after the snapshot (e.g., a task +archived since) shows as stale until a manual refresh. + +Root cause chain, with evidence: + +1. Fresh loads are always correct: the SPA shell and the boot-payload paths set + `Cache-Control: no-store` (`apps/backend/internal/webapp/handler.go`, + `dev_handler.go`), and the frontend data fetches pass `cache: "no-store"` + (`apps/web/lib/api/domains/...`, `loadBootPayload` in + `apps/web/src/boot-payload.ts`). A real navigation therefore re-reads + current state. +2. Tab duplicate is not a fresh load. Chromium tracks it as a `back_forward` + navigation (bfcache-dev discussion, + https://groups.google.com/a/chromium.org/g/bfcache-dev/c/Cs5ISWbKhKU), and + Chrome has been rolling out bfcache admission for `Cache-Control: no-store` + pages (https://developer.chrome.com/docs/web-platform/bfcache-ccns). +3. The app has no restore-specific handling: no handler inspects + `pageshow.persisted` (the generic `useForegroundRefresh` hook registers a + `pageshow` listener in `apps/web/hooks/use-foreground-refresh.ts` but does + not distinguish restores from focus/visibility events, and covers only + subsets of surfaces). +4. Verified in real Chrome (headless, via a test page): a bfcache restore + fires `pageshow` with `persisted === true`, and the frozen timers resume. + Platform guidance for this class of bug is to handle `pageshow` with + `event.persisted === true` and refresh or reload the page (web.dev bfcache + article, https://web.dev/articles/bfcache). + +## Fix + +Add a small, testable bootstrap module that reloads the page when it is +restored from a frozen snapshot: + +- `apps/web/src/bfcache-restore-reload.ts` exports + `installBfcacheRestoreReload(options?)` returning an uninstall function. It + listens for `pageshow` and reloads when `event.persisted === true`. That is + the only reliable frozen-restore signal: the navigation type + `back_forward` also covers cold history traversals and session-restored + tabs, which load fresh and must NOT be reloaded a second time (a redundant + reload would discard restored scroll/UI state on the common + click-out-and-back flow). Fresh loads (`navigate`), manual refreshes + (`reload`), and SPA soft navigations never reload. +- The module follows the `apps/web/src/vite-preload-recovery.ts` pattern: + injected `target` and `reload` for testability. +- `apps/web/src/main.tsx` installs it at module scope next to + `installVitePreloadRecovery()`, before React mounts. + +No backend change. No new user-facing copy (no i18n impact). + +## Tests + +Unit tests, `apps/web/src/bfcache-restore-reload.test.ts` (vitest + happy-dom, +mirroring `vite-preload-recovery.test.ts` conventions): + +- `pageshow` with `persisted === true` triggers the reload. +- `pageshow` with `persisted === false` does not reload — including on a cold + `back_forward` traversal and on a manual refresh (regression: the + navigation type must not be treated as a restore signal). +- An event carrying no `persisted` flag does not reload. +- The uninstall function removes the listener. + +E2E test, `apps/web/e2e/tests/layout/bfcache-restore-reload.spec.ts` +(Playwright + real backend fixture, per `apps/web/e2e/README.md`): + +- Load the app; assert the navigation type is `navigate` and no reload fired. +- Dispatch a real `pageshow` event with `persisted === true` from the page + context; retry (navigation-race safe) until the navigation type becomes + `reload` (the app reloaded through its own installed handler). +- The synthetic-signal approach is deliberate: the e2e backend page holds an + open WebSocket, which in current Chrome makes a `no-store` page ineligible + for bfcache on back/forward navigations, so `page.goBack()` would reload + even without the fix (false positive). The browser's restore machinery is + covered by the real-Chrome manual verification below. + +## Verification + +- `cd apps && pnpm --filter @kandev/web test -- --run src/bfcache-restore-reload.test.ts` +- `cd apps/web && pnpm run typecheck` +- `cd apps && pnpm --filter @kandev/web lint` (changed files only) +- E2E: `cd apps/web && pnpm e2e:run tests/layout/bfcache-restore-reload.spec.ts` +- Real-Chrome manual verification (user or implementer, on a non-headless + Chrome): open Kandev, archive a task, right-click the tab → Duplicate. The + duplicated tab must reload and show the task as archived. This run is the + MERGE BLOCKING gate (instrumentation checklist and decision rule in + `task-01-reload-on-bfcache-restore.md`). If a current Chrome version + restores without firing `pageshow` (no persisted signal), record it in the + task Results and follow up with a WS-close-based fallback, gated on that + evidence and subject to the restore-bounding caveat and regression-test + requirement recorded in `task-01-reload-on-bfcache-restore.md` + ("WS-close fallback caveat"). + +## Implementation Waves And Parallel Candidates + +Wave 1: + +- [ ] [Task 01: Reload on bfcache restore](task-01-reload-on-bfcache-restore.md) + +Sequential. Single task; no parallel candidates. diff --git a/docs/plans/fix-duplicated-tab-stale-data/task-01-reload-on-bfcache-restore.md b/docs/plans/fix-duplicated-tab-stale-data/task-01-reload-on-bfcache-restore.md new file mode 100644 index 0000000000..70d7c31d82 --- /dev/null +++ b/docs/plans/fix-duplicated-tab-stale-data/task-01-reload-on-bfcache-restore.md @@ -0,0 +1,375 @@ +--- +id: "01-reload-on-bfcache-restore" +title: "Reload on bfcache restore" +status: in-progress +wave: 1 +depends_on: [] +plan: "plan.md" +spec: "../../specs/fix-duplicated-tab-stale-data/spec.md" +--- + +# Task 01: Reload on BFCache Restore + +## Acceptance + +- `apps/web/src/bfcache-restore-reload.ts` exports `installBfcacheRestoreReload` + which reloads the page on a `pageshow` event when `event.persisted === true` + only (never on the `back_forward` navigation type alone, which also covers + cold history traversals and session-restored tabs), and returns an + uninstall function. +- `apps/web/src/main.tsx` installs it at module scope next to + `installVitePreloadRecovery()`, before React mounts. +- Unit tests cover: persisted restore reloads; persisted=false does not reload + (fresh load, manual refresh, and cold `back_forward` traversal regression); + an event with no persisted flag does not reload; uninstall removes the + listener. +- E2E test dispatches a real persisted `pageshow` in the running app and + asserts the page reloads (navigation type becomes `reload`); a normal load + does not reload. +- No user-facing copy added (no i18n ratchet impact). +- MERGE BLOCKING: a real-Chrome native Duplicate-tab run is performed and + recorded (see "Blocking verification" below). The automated suite proves + the handler fires on `persisted === true`, which is the only event sequence + consistent with the reported symptom (a cold duplicate load re-fetches the + no-store boot payload and would show fresh data; only a frozen restore + preserves the stale view, and frozen restores fire `pageshow` with + `persisted === true`). The native-run record confirms that sequence on the + user's Chrome before merge. + +## Blocking verification (merge gate) + +Run in the user's non-headless Chrome against a debug build of the affected +Kandev (debug mode sets `window.__KANDEV_DEBUG`, which arms the pre-reload +probe in `bfcache-restore-reload.ts`). The duplicated tab is a SEPARATE +DevTools target and the handler reloads synchronously during its initial +`pageshow`, so attaching DevTools afterward cannot observe the restore; the +probe persists the evidence to sessionStorage BEFORE the reload. + +1. In the SOURCE tab's DevTools console, clear any inherited probe from an + earlier run and store the attempt start time in sessionStorage: + `sessionStorage.removeItem("kandev.bfcacheRestoreProbe"); sessionStorage.setItem("kandev.bfcacheProbeStart", String(Date.now()));` + (sessionStorage survives reloads and Chrome Duplicate copies the source + tab's storage — including this helper key — so the duplicated tab can + compare timestamps even though console variables are target-local.) +2. Archive a task, then right-click the Kandev tab → **Duplicate**. +3. In the duplicated tab's DevTools console, read the probe and check it is + newer than this attempt in one self-contained, null-safe expression + (console bindings are not carried over from the source tab): + `((JSON.parse(sessionStorage.getItem("kandev.bfcacheRestoreProbe") ?? "null")?.at ?? -1) >= Number(sessionStorage.getItem("kandev.bfcacheProbeStart")))` + The probe records `{ persisted, navigationType, at }` captured pre-reload; + the comparison must be true for the record to belong to this run (a stale + probe inherited from an earlier restore must not be attributed to it). If + no `pageshow` fired, there is no probe entry and the expression evaluates + false instead of throwing, so the no-probe outcome in step 6 can be + recorded. +4. Confirm the handler reloaded from inside the duplicated tab's console + after the reload settles: + `performance.getEntriesByType("navigation")[0]?.type` must be `"reload"` + (a bfcache restore without the fix would report `"back_forward"`). The + archived task must no longer be listed as active. (The duplicated tab is a + separate DevTools target and reloads synchronously, so source-tab Network + capture cannot observe its document request; the navigation-type and + UI-state checks above are the executable proof.) +5. Expected with this fix: the probe shows `persisted: true`, the duplicated + tab reloads, and the task is shown as archived. +6. If the duplicated tab fires NO `pageshow` (no probe entry) or shows a + probe with `persisted: false` while still displaying stale data, the + frozen-restore assumption is wrong for that Chrome version: record the + observed sequence (including WebSocket close code/timing, if any), then + design a fallback from that evidence — see the WS-close caveat below. Do + not add a fallback without this evidence. + +### WS-close fallback caveat (only if step 6 triggers) + +The navigation type `back_forward` is fixed for the document lifetime, and +`WebSocketClient.handleDisconnect` observes every later unintentional +disconnect, so reloading on "unexpected WS close while the navigation type is +`back_forward`" would also fire after a cold history/session restore followed +by an ordinary network loss — recreating the round-1 false-positive class. +Any WS-based fallback must be bounded to the restore/bootstrap context (e.g. +only within a short window after the restore, or only when the sessionStorage +probe from this handler is present) and must include a regression test that a +routine disconnect after a cold `back_forward` load does NOT reload. + +## Verification + +```bash +cd apps && pnpm --filter @kandev/web test -- --run src/bfcache-restore-reload.test.ts +cd apps/web && pnpm run typecheck +cd apps && pnpm --filter @kandev/web lint +cd apps/web && pnpm e2e:run tests/layout/bfcache-restore-reload.spec.ts +``` + +Manual, on a real (non-headless) Chrome: archive a task, right-click the +Kandev tab → Duplicate; the duplicated tab must reload and show the task as +archived. Record the outcome in Results. + +## Files Likely Touched + +- `apps/web/src/bfcache-restore-reload.ts` (new) +- `apps/web/src/bfcache-restore-reload.test.ts` (new) +- `apps/web/src/main.tsx` (wire install call) +- `apps/web/e2e/tests/layout/bfcache-restore-reload.spec.ts` (new) + +## Dependencies + +None. + +## Parallelism + +Sequential. Single task; no parallel candidates. + +## Inputs + +- Repair spec: `docs/specs/fix-duplicated-tab-stale-data/spec.md`. +- Existing pattern: `apps/web/src/vite-preload-recovery.ts` and its test + (injected `target`/`reload`, defensive storage reads). +- Bootstrap entry: `apps/web/src/main.tsx` (`installVitePreloadRecovery()` + call site). + +## Risks + +- Chrome duplicate-tab restore event delivery may vary by version. Detection + is `pageshow.persisted === true` only; a state-clone restore that fires no + `pageshow` at all is not detectable by this handler. If a real-Chrome + duplicate test shows no reload, follow up with the WS-close fallback + described in the plan (reload on unexpected WS close when the navigation + type is `back_forward`), gated on that evidence; do not add it + speculatively. +- happy-dom lacks `PageTransitionEvent`; tests define the `persisted` flag on + a plain `Event`. The cold-`back_forward` regression is covered twice: the + injected-seam case (harness `navigationType: "back_forward"` through the + injected reader) and the default-reader case (module installed without the + injected reader while global `performance` reports `back_forward`), so a + reintroduced navigation-type fallback fails either way. +- E2E must use the synthetic persisted-`pageshow` signal (see plan): with an + open WebSocket, current Chrome does not bfcache `no-store` pages on + back/forward, so `page.goBack()` would pass trivially. The reload assertion + retries with `expect(...).toPass({ timeout: 15_000 })` (catching the + destroyed execution context mid-navigation); no sleeps. + +## Output Contract + +Report the root-cause evidence summary, files changed, exact tests run +(unit, typecheck, lint, E2E), the real-Chrome duplicate-verification outcome, +task and plan status updates, and any follow-up decision on the WS-close +fallback. + +## Results + +- Added `apps/web/src/bfcache-restore-reload.ts`: a `pageshow` handler that + reloads the page when `event.persisted === true`, with an uninstall + function (pattern: `vite-preload-recovery.ts`). +- Wired `installBfcacheRestoreReload()` into `apps/web/src/main.tsx` at module + scope next to `installVitePreloadRecovery()`, before React mounts. +- Added `apps/web/src/bfcache-restore-reload.test.ts` (13 tests) and + `apps/web/e2e/tests/layout/bfcache-restore-reload.spec.ts`. +- Checks passed: + - `cd apps && pnpm --filter @kandev/web test -- --run src/bfcache-restore-reload.test.ts` + (13/13 tests, current HEAD). + - `cd apps/web && pnpm run typecheck` (clean). + - `cd apps/web && pnpm exec eslint src/bfcache-restore-reload.ts + src/bfcache-restore-reload.test.ts src/main.tsx + e2e/tests/layout/bfcache-restore-reload.spec.ts` (clean). + - `cd apps/web && pnpm e2e:run tests/layout/bfcache-restore-reload.spec.ts` + (1/1; the assertion arms a `framenavigated` causal wait before + dispatching the restore signal, then asserts the reload navigation type + with the default timeout — the earlier `expect(...).toPass({ timeout: + 15_000 })` effect-polling budget was removed per the E2E causal-wait + convention). + +### Adversarial review round 1 (50-luna-review-fix) + +- Finding (P2, accepted): the initial version reloaded on the + `back_forward` navigation type in addition to `persisted === true`, but + `back_forward` identifies every history traversal, not only frozen + restores. A cold back/forward load or session-restored tab (both load + fresh) was therefore reloaded a second time, discarding restored scroll/UI + state on the common click-out-and-back flow. Frozen restores always fire + `pageshow` with `persisted === true`, so the fallback added false positives + without covering any additional restore path. +- Resolution: detection is now `persisted === true` only; the navigation-type + fallback and its tests were removed; a regression test asserts no reload on + a cold `back_forward` traversal. Spec, plan, and this file updated to match. + +- Real-Chrome duplicate-tab verification (non-headless, user's environment) + remains outstanding: archive a task, right-click the tab → Duplicate, expect + the duplicated tab to reload and show the task as archived. If a Chrome + version restores without firing `pageshow` at all (no persisted signal), + add the plan-documented WS-close fallback gated on that evidence; none was + needed for the signals observed here. + +### Adversarial review round 2 (50-luna-review-fix) + +- Finding 1 (minor, accepted): the cold-`back_forward` regression test only + dispatched `persisted=false` without exposing a `back_forward` navigation + entry, so it was behaviorally identical to the fresh-load test and a + reintroduced navigation-type fallback would stay green under happy-dom (no + matching navigation entry). Fixed: the test now stubs + `performance.getEntriesByType("navigation")` to return a `back_forward` + entry before dispatching `persisted=false`, and asserts no reload. +- Finding 2 (nit, accepted): the plan and task still described the removed + navigation-type reader and defensive Navigation Timing reads. Fixed: both + documents now describe only injected `target`/`reload` and the + plain-`Event` persisted test, retaining the separately gated WS-close + fallback. +- No production-code correctness issue found. The real-Chrome duplicate-tab + run remains the outstanding evidence gate for the WS-close fallback + decision. + +### Adversarial review round 3 (50-luna-review-fix) + +- Finding (nit, accepted): the durable plan/task record said the unit + environment is jsdom and the E2E uses `expect.poll` with default timeouts, + but `apps/web/vitest.config.ts` configures happy-dom and the implemented + E2E uses `expect(...).toPass({ timeout: 15_000 })` (the reload teardown + destroys the evaluate context, so the retry swallows those errors; 15s + covers the reload plus a fresh app boot). Fixed: all jsdom references in + the plan package now name happy-dom, and the E2E wait strategy is + documented as `toPass`/15s. +- No production correctness issue found (persisted-only semantics, bootstrap + timing/StrictMode, listener lifecycle, reload-path interaction, option/cast + safety, cold-`back_forward` stub, E2E positive/negative causality). + +### Adversarial review round 4 (50-luna-review-fix) + +- Finding (major, partially accepted): native Chrome Duplicate calls + `contents->Clone()` + `CopyStateFrom(..., true)` + `LoadIfNecessary()`, and + the cited Chromium discussion establishes only a `back_forward` navigation + type, not `persisted=true`; the E2E synthesizes the signal, so all + automated checks could pass while the reported path remains unfixed. +- Response: the premise that native Duplicate is a cold "navigated clone" is + inconsistent with the reported symptom. A cold duplicate load re-fetches + the no-store boot payload (and the tasks/kanban fetches are `no-store`), + so it would show FRESH data; the user's stale view can only come from a + frozen restore (JS heap preserved), and frozen restores fire `pageshow` + with `persisted === true`. `persisted === true` is therefore the correct + signal for the observed bug, and the handler cannot fire on a cold load. + Accepted part: the native-Duplicate event sequence on the user's Chrome + version is unverified, and a Chrome build could in principle clone without + firing `pageshow` at all. That verification is now a MERGE BLOCKING gate + with an instrumentation checklist (pageshow.persisted, navigation type, + boot-payload requests); if the observed sequence contradicts the + assumption, the WS-close fallback is implemented gated on that evidence. +- The real-Chrome duplicate-tab run remains the outstanding evidence gate + and now blocks merge. + +### Adversarial review round 5 (50-luna-review-fix) + +- Finding 1 (major, accepted): the task/plan were marked done/complete while + the merge gate was outstanding. Fixed: both are now `in-progress`; they are + flipped to done/complete only after the native run is recorded. +- Finding 2 (major, accepted): the gate checklist could not capture the + evidence — `window.__KANDEV_DEBUG` is only a boolean, the duplicated tab is + a separate DevTools target, and the synchronous reload destroys the + pre-reload event/navigation data. Fixed: the module now writes a + debug-gated probe (`kandev.bfcacheRestoreProbe`) to sessionStorage BEFORE + reloading, recording `{ persisted: true, navigationType, at }`; the + checklist reads the probe post-duplicate and corroborates with a fresh + boot-payload request. Probe covered by 3 new unit tests (records in debug, + silent in non-debug, never blocks the reload). +- Finding 3 (minor, accepted): the WS-close fallback rule was not + restore-specific — `back_forward` is fixed for the document lifetime and + `handleDisconnect` observes every later unintentional disconnect, which + would recreate the round-1 false-positive class after a cold restore plus + ordinary network loss. Fixed: the fallback is now explicitly caveated (must + be bounded to the restore/bootstrap context, e.g. a short post-restore + window or the presence of this handler's sessionStorage probe, and must + include a routine-disconnect-after-cold-back_forward regression test) and + is only designed after the step-5 evidence. +- Finding 4 (nit, accepted): the plan claimed no `pageshow` handlers existed + anywhere, but `useForegroundRefresh` already registers one. Fixed: the + claim now says no handler inspects `pageshow.persisted`. + +### Adversarial review round 6 (50-luna-review-fix) + +- Finding 1 (major, accepted): the probe could not capture the falsifying + `persisted=false` case — the handler returned before recording and the + payload hard-coded `persisted: true`. Fixed: the handler now records the + ACTUAL `persisted` value for duplicate/history candidates (`persisted === + true`, or any `back_forward`-typed document) before any reload decision, + and the record survives the reload in sessionStorage; a + persisted=false/back_forward record-without-reload unit test was added. +- Finding 2 (minor, accepted): `isDebug()` ran outside the best-effort + guard, so a throwing predicate aborted the handler before `reload()`. + Fixed: the predicate is inside the diagnostic try/catch; a throwing-`isDebug` + test proves the reload still runs. +- Finding 3 (minor, accepted): the gate read the fixed sessionStorage key + without clearing it, and Chrome Duplicate copies the source tab's storage, + so a stale probe could be misattributed. Fixed: the checklist now clears + the key and records `t0` before Duplicate, and requires `at >= t0`. +- Finding 4 (minor, accepted): Results recorded 6/6 for a file now containing + 12 tests. Fixed: re-ran the focused unit command on current HEAD (12/12) + and updated the record. +- Finding 5 (nit, accepted): the spec said the fix writes no client storage, + but debug restores write the sessionStorage probe. Fixed: Persistence + guarantees now document the debug-only diagnostic key, fields, lifetime, + cleanup, and best-effort failure behavior. + +### Adversarial review round 7 (50-luna-review-fix) + +- Finding 1 (minor, accepted): the cold-`back_forward` regression test + stubbed global `performance`, but the harness always injects the + navigation-type reader, so the stub never reached the exercised code path + and a reintroduced fallback reading the injected option would pass. Fixed: + the test now passes `navigationType: "back_forward"` through the harness's + injected reader (the actual seam the module uses) and the ineffective + global stub and its cleanup were removed. +- Finding 2 (minor, accepted): the gate's Network-capture step could not + observe the duplicated tab — it is a separate DevTools target that reloads + synchronously, so source-tab DevTools never records its document request. + Fixed: step 4 now uses executable post-reload proof from the duplicated + tab's own console — `performance.getEntriesByType("navigation")[0]?.type + === "reload"` — plus the archived-task UI check. +- No production-handler correctness defect found. + +### Adversarial review round 8 (50-luna-review-fix) +- Finding 1 (minor, accepted): the cold-`back_forward` regression only + covered a fallback using the injected navigation reader; a fallback calling + the module's default `readNavigationType` (global performance) would stay + green. Fixed: the harness gained `injectNavigationType: false`, and a + companion test installs without the injected reader while stubbing global + `performance` to report `back_forward` (with `performance.now` for + happy-dom); the Risk text now describes both paths. +- Finding 2 (minor, accepted): step 1 declared `const t0` in the source tab's + console, which is target-local and unavailable in the duplicated tab. + Fixed: the start time is stored in sessionStorage + (`kandev.bfcacheProbeStart`, copied to the duplicate) and the duplicated + tab compares with a self-contained null-safe one-liner + (`((JSON.parse(sessionStorage.getItem("kandev.bfcacheRestoreProbe") ?? "null")?.at ?? -1) >= Number(sessionStorage.getItem("kandev.bfcacheProbeStart")))`), + after a follow-up review caught that the first revision referenced an + unbound `at`, and a later review caught that the expression threw on an + absent probe. +- Finding 3 (nit, accepted): the spec's client-storage contract only + mentioned writes before reload, omitting the `persisted === false` + `back_forward` diagnostic writes. Fixed: the contract now states the probe + covers frozen restores AND persisted-false candidates, with only + `persisted === true` causing a reload. +- No production-handler correctness defect found. + +### PR review fixup (CodeRabbit / Claude / Greptile, PR #2717) + +- CodeRabbit P2 (plan.md:108): the Wave-1 checkbox marked Task 01 complete + while the native gate is open. Fixed: unchecked; it stays `[ ]` until the + native result is recorded. +- CodeRabbit P2 (e2e:30): the reload assertion polled the navigation effect + with a hand-picked 15s budget. Fixed: the E2E now arms a + `framenavigated` causal wait before dispatching the restore signal and + asserts the reload navigation type with the default timeout. +- CodeRabbit + Claude (unit tests): "fresh load" and "manual refresh" tests + exercised identical paths. Fixed: they now pass + `navigationType: "navigate"` / `"reload"` through the harness, guarding a + reintroduced fallback keyed on either type. +- CodeRabbit (task-01:61): the gate's probe expression threw on an absent + probe. Fixed: both step 3 and the round-8 record now use a null-safe + expression that evaluates false (no-probe outcome recordable) instead of + throwing. +- CodeRabbit (plan/spec): the spec claimed native Duplicate "still delivers + `persisted`" — unverified. Fixed: the failure-mode bullet and the + Duplicate-tab scenario now mark that outcome pending until the native gate + is recorded; LanguageTool wording nit ("in the meantime") fixed. +- Claude suggestion: spec status `draft` → `building` (INDEX updated). +- Not actionable here: the merge-blocking native-Chrome Duplicate-tab + verification requires the user's non-headless Chrome (right-click → + Duplicate cannot be driven via CDP/Playwright); it remains the documented + gate and stays open in the task/plan records. diff --git a/docs/specs/INDEX.md b/docs/specs/INDEX.md index 0e30e84c00..ff01945f73 100644 --- a/docs/specs/INDEX.md +++ b/docs/specs/INDEX.md @@ -58,6 +58,7 @@ Product-wide capabilities that are not tied to a single feature area. | [bounded-task-status-delivery](platform/bounded-task-status-delivery.md) | approved | | [diagnostic-logging](platform/diagnostic-logging.md) | approved | | [provider-error-recovery](platform/provider-error-recovery.md) | draft | +| [duplicated-tab-stale-data](fix-duplicated-tab-stale-data/spec.md) | building | | [health-endpoint-version](health-endpoint-version/spec.md) | building | | [go-dev-launcher](go-dev-launcher/spec.md) | draft | diff --git a/docs/specs/fix-duplicated-tab-stale-data/spec.md b/docs/specs/fix-duplicated-tab-stale-data/spec.md new file mode 100644 index 0000000000..8a52c99552 --- /dev/null +++ b/docs/specs/fix-duplicated-tab-stale-data/spec.md @@ -0,0 +1,115 @@ +--- +status: building +created: 2026-08-16 +owner: Kandev +--- + +# Reload Kandev when a tab is restored from a frozen browser snapshot + +## Why + +Duplicating a Kandev tab in Chrome restores a frozen snapshot of the page +(back/forward-style restore, not a fresh load; Chromium reports the duplicated +tab's navigation type as `back_forward`). The restored page keeps the JS heap +and DOM as they were when the snapshot was taken, and no boot payload is +re-fetched. + +Kandev's data-freshness model assumes fresh loads: the SPA shell and all data +fetches are `Cache-Control: no-store`, so any real navigation re-reads current +backend state. A restored page bypasses that model entirely. The existing +foreground-refresh hooks (`useForegroundRefresh`) are best-effort, cover only +subsets of surfaces, and do not distinguish restores from normal focus events, +so a frozen restore can show stale data (for example, a task archived after +the snapshot still appears in Active tasks) until the user manually refreshes. + +Chrome has been rolling out back/forward cache (bfcache) admission for +`Cache-Control: no-store` pages since 2024, so restores of this page are +becoming more common. The platform guidance for this exact situation is to +handle `pageshow` with `event.persisted === true` and refresh or reload the +page. + +## What + +- When the Kandev page is restored from a frozen browser snapshot, the app + performs a full reload so state is re-fetched from the backend. +- A restore is detected on the `pageshow` event when `event.persisted === + true`. This is the only reliable frozen-restore signal: the navigation type + `back_forward` also covers cold history traversals and session-restored + tabs, which load fresh and must not be reloaded a second time. +- Normal page loads are unaffected: a fresh navigation, a manual refresh, + a cold back/forward traversal, and in-app SPA routing never trigger the + reload. + +## API surface + +No backend, network, or public contract change. Observable behavior: after +Chrome's Duplicate tab (or a back/forward restore) the page reloads once +instead of showing frozen state. This is the same effect as the user's manual +refresh, automated. + +## Failure modes + +- Restore signals unavailable (no `PageTransitionEvent.persisted`): the + handler degrades to a no-op and stale data can still appear until a manual + refresh. All browsers that implement bfcache deliver `persisted` on the + restore. +- Cold history traversals and session-restored tabs fire `pageshow` with + `persisted` false even though their navigation type is `back_forward`; the + handler ignores them because those pages load fresh and need no reload. +- Reload loop: a reload produces a fresh document whose `pageshow.persisted` + is `false`, so no recursive reload. +- Open WebSocket at freeze time can make the page ineligible for bfcache on + back/forward navigations in some Chrome versions; such navigations reload + normally and are correct without the handler. Genuine restores deliver + `persisted` on `pageshow`. Whether Chrome's native Duplicate-tab path is + such a restore (vs. a state clone that fires no `pageshow` at all) is + pending the native verification gate documented in the plan; the + Duplicate-tab outcome in the scenarios below is marked pending until that + evidence is recorded. + +## Persistence guarantees + +Server state remains the source of truth; the reload re-reads it. The fix +writes no server state. + +Client storage: in debug builds only (`window.__KANDEV_DEBUG`), a +`pageshow` duplicate/history candidate writes a diagnostic record to +sessionStorage under `kandev.bfcacheRestoreProbe` before any reload decision — +`{ persisted, navigationType, at }`. Candidates are frozen restores +(`persisted === true`, which then reload) and `persisted === false` documents +reached via a `back_forward` traversal (diagnostic only, no reload), so a +native Duplicate-tab run can be verified after the reload destroys the +document. The write is best-effort (never blocks the reload), is skipped +outside debug builds, and is scoped to the tab session: it is overwritten on +the next candidate and cleared by the merge-gate checklist before each +attempt. It contains no user data. + +## Scenarios + +- **GIVEN** a Kandev tab showing a task in Active tasks, **WHEN** the task is + archived and the user duplicates the tab in Chrome, **THEN** the duplicated + tab reloads and the task is not shown as active. *(Pending: the native + Duplicate-tab event sequence is the outstanding verification gate; until it + is recorded, this outcome is expected but not yet evidenced.)* +- **GIVEN** a loaded Kandev page, **WHEN** the browser restores it from + bfcache (back/forward navigation), **THEN** the page reloads with fresh + data. +- **GIVEN** a Kandev page loading normally, **WHEN** it loads, **THEN** no + reload is triggered. +- **GIVEN** a Kandev page, **WHEN** the user refreshes it manually, **THEN** + no additional reload is triggered. +- **GIVEN** a Kandev page reached by a cold back/forward traversal (loaded + fresh, `pageshow.persisted` false), **WHEN** it loads, **THEN** no reload + is triggered. + +## Out of scope + +- Incremental state reconciliation after restore. The reload is the + reconciliation; WS reconnect/resubscribe continues to cover session-level + data independently. +- Reloading on tab freeze/resume (`resume` event). Backgrounded-tab resume is + the normal tab-switch flow already handled by the existing + foreground-refresh hooks. +- Changing HTTP caching headers. The shell and data fetches already use + `no-store`; bfcache is not the HTTP cache and cannot be disabled with + headers.