diff --git a/e2e/documents.spec.ts b/e2e/documents.spec.ts index 0232fd2fc..96d537e0d 100644 --- a/e2e/documents.spec.ts +++ b/e2e/documents.spec.ts @@ -235,6 +235,19 @@ test.describe("document vault", () => { await expect(openButton(page, "MRT Knie")).toBeVisible(); await openButton(page, "MRT Knie").click(); const detail = page.getByRole("dialog", { name: "MRT Knie" }); + // The sheet is CLOSED is a different claim from the sheet is showing this + // document, and only one of them can be made by a name-matched locator. + // + // The sheet takes its accessible name from the document title and falls + // back to "Untitled document" while the detail read is pending, so + // `expect(detail).not.toBeVisible()` also passes on an OPEN sheet that has + // gone back to its skeleton — measured, not theorised: a slow detail read + // reports the named dialog invisible and the unnamed one visible at the + // same instant. That resolves immediately instead of waiting, which is the + // wait this test needs after a history traversal, and the assertion that + // depended on it then reads a URL the app has not finished with. The + // deep-link test above already uses the unnamed role for this reason. + const sheet = page.getByRole("dialog"); await expect .poll(() => new URL(page.url()).searchParams.get("doc")) @@ -246,15 +259,18 @@ test.describe("document vault", () => { expect(new URL(page.url()).searchParams.get("q")).toBe("MRT"); await page.goBack(); - await expect(detail).not.toBeVisible(); + await expect(sheet).not.toBeVisible(); expect(new URL(page.url()).searchParams.get("doc")).toBeNull(); expect(new URL(page.url()).searchParams.get("view")).toBe("compact"); await page.goForward(); + // Both: the sheet is open AND it is this document, so Escape is pressed on + // a settled sheet rather than on one still resolving its title. + await expect(sheet).toBeVisible(); await expect(detail).toBeVisible(); await page.keyboard.press("Escape"); - await expect(detail).not.toBeVisible(); + await expect(sheet).not.toBeVisible(); await expect .poll(() => new URL(page.url()).searchParams.get("doc")) .toBeNull(); diff --git a/src/components/documents/__tests__/documents-view-navigation.test.ts b/src/components/documents/__tests__/documents-view-navigation.test.ts index 7d4dff791..0621f2478 100644 --- a/src/components/documents/__tests__/documents-view-navigation.test.ts +++ b/src/components/documents/__tests__/documents-view-navigation.test.ts @@ -5,6 +5,7 @@ import { closeDocumentSelectionAfterCoachHandoff, documentSelectionHistoryState, documentSelectionHref, + documentSelectionSurvivedClose, withoutDocumentSelectionHref, } from "../documents-view"; @@ -173,4 +174,44 @@ describe("document vault URL selection", () => { expect(history.href).toBe("/documents?episode=ep-1"); }); + + // ── The dropped traversal ──────────────────────────────────────────────── + // + // `history.back()` is a request. A traversal asked for while another is + // still settling can be dropped, and the browser says nothing: the sheet + // closes, React stays consistent, and the address bar keeps naming a + // document nobody has open. Reproduced in the browser suite as a closed + // sheet whose `?doc=` never cleared; this is the predicate the component + // uses to notice it. + + it("reports a selection that survived its own close", () => { + expect( + documentSelectionSurvivedClose("?episode=ep-1&doc=doc-1", "doc-1"), + ).toBe(true); + }); + + it("reports nothing to do once the traversal landed", () => { + expect(documentSelectionSurvivedClose("?episode=ep-1", "doc-1")).toBe( + false, + ); + }); + + it("leaves a DIFFERENT selection alone", () => { + // The traversal landed somewhere that carries its own selection. Stripping + // that would close a sheet this close had nothing to do with. + expect( + documentSelectionSurvivedClose("?episode=ep-1&doc=doc-2", "doc-1"), + ).toBe(false); + }); + + it("strips only the selection when the fallback runs", () => { + // What the component does with a true verdict, on the URL shape that + // produced it: every unrelated parameter survives. + expect( + withoutDocumentSelectionHref( + "/documents", + "episode=ep-1&view=compact&q=MRT&doc=doc-1", + ), + ).toBe("/documents?episode=ep-1&view=compact&q=MRT"); + }); }); diff --git a/src/components/documents/documents-view.tsx b/src/components/documents/documents-view.tsx index ac220790c..e480fc7e3 100644 --- a/src/components/documents/documents-view.tsx +++ b/src/components/documents/documents-view.tsx @@ -120,6 +120,30 @@ export function withoutDocumentSelectionHref( return search ? `${pathname}?${search}` : pathname; } +/** + * How long to wait before checking that `history.back()` actually happened. + * + * Long enough for a traversal to commit on a loaded machine, short enough that + * a reader who closed the sheet does not see the selection linger in the + * address bar. The window is bounded on the other side too: the effect that + * arms this cancels it as soon as the sheet re-opens. + */ +export const CLOSE_TRAVERSAL_GRACE_MS = 250; + +/** + * The selection this close was meant to consume is still in the address bar. + * + * Matched on the id rather than on the presence of `doc`, so a traversal that + * landed somewhere carrying a DIFFERENT selection is not mistaken for a + * dropped one and stripped. + */ +export function documentSelectionSurvivedClose( + search: string, + documentId: string, +): boolean { + return new URLSearchParams(search).get("doc") === documentId; +} + export function closeDocumentSelectionHistoryEntry( history: { readonly state: unknown; @@ -711,6 +735,33 @@ export function DocumentsView() { handedOffDocumentRef.current, coachLaunch?.closeIntent, ); + + // Consuming the pushed entry is a REQUEST to the browser, not a guarantee. + // + // `history.back()` is queued against the session history, and a traversal + // asked for while another is still settling can be dropped — closing the + // sheet immediately after pressing the browser's Forward button is exactly + // that shape. Nothing raises when it happens: the sheet is closed, React + // is consistent, and the URL still names the document. A reload or a + // shared link then re-opens the sheet the reader just closed. + // + // So the request is checked rather than trusted. If the selection is still + // in the address bar shortly afterwards, it is removed the synchronous + // way. The guard is the document id, so a traversal that landed on some + // other selection is left alone, and the cleanup below cancels the check + // the moment the sheet re-opens or the parameter changes — which is what + // makes re-opening within the window safe. + const verify = window.setTimeout(() => { + if (!documentSelectionSurvivedClose(window.location.search, docParam)) { + return; + } + window.history.replaceState( + null, + "", + withoutDocumentSelectionHref(pathname, window.location.search), + ); + }, CLOSE_TRAVERSAL_GRACE_MS); + return () => window.clearTimeout(verify); }, [ authLoading, coachOwnsDocument,