fix(documents): check that closing a document consumed its history entry

Closing a card-opened document asks the browser to go back, so the entry
the selection pushed is consumed and the reader's next Back reaches the
page before it. A traversal is a request, not a guarantee: one asked for
while another is still settling can be dropped, and nothing raises when
it is. The sheet closes, React stays consistent, and the address bar goes
on naming a document nobody has open, so a reload or a shared link
re-opens the sheet that was just closed.

Pressing Escape straight after the browser's Forward button is exactly
that shape, and it reproduces: ten repeats across both viewports failed
two of twenty with the URL still carrying the id, and none of twenty once
the close verifies itself. The check is guarded by the document id, so a
traversal that landed on a different selection is left alone, and it is
cancelled the moment the sheet re-opens.

The browser test that found it could not say so. It asserted the sheet
was closed through a locator matched on the document TITLE, and the sheet
falls back to an untitled label while its detail read is pending, so the
assertion also passed on an open sheet showing a skeleton, resolved
immediately instead of waiting, and pushed the failure into a URL poll
ten seconds later. It now asserts the dialog role, which is what the
neighbouring deep-link test already does.
This commit is contained in:
Marc-André Bombeck committed 2026-08-07 10:26:25 +02:00
1 parent e2eea55b6e
commit a5b6508961
3 files changed
+110 -2

No files matched your search

+18 -2
View File
@@ -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();
@@ -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");
});
});
@@ -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,