fix(documents): do not check a close that deliberately kept its selection

Maximizing the Coach drawer hands the conversation to /coach and carries
?doc= with it, so browser-Back can rebuild the sheet. That is the one
close-shaped transition whose whole point is to LEAVE the selection where
it is, and the close path has always returned early for it.

The dropped-traversal check added beside that close did not know. It saw
a closed sheet and a selection still in the address bar, which is exactly
the state it exists to repair, and it repaired away the return URL a
quarter of a second after the close had deliberately preserved it.
Whether it won that race was a question about how fast the machine
navigated, so it surfaced as one flaky run on the narrow viewport rather
than as a broken feature.

Both sides read one named predicate now, and the check is skipped when
the close kept the selection on purpose. Proved by driving the grace
window to zero, which is the worst case the timer can have: before, the
maximize never even reached /coach, three attempts out of three; after,
it passes there and the whole documents family passes with it. Twenty
consecutive runs of that family on the narrow viewport are clean.
This commit is contained in:
Marc-André Bombeck committed 2026-08-07 13:15:23 +02:00
1 parent 9849a369a1
commit f49defbc34
2 files changed
+116 -1

No files matched your search

@@ -3,6 +3,7 @@ import { describe, expect, it } from "vitest";
import {
closeDocumentSelectionHistoryEntry,
closeDocumentSelectionAfterCoachHandoff,
coachHandoffRetainsSelection,
documentSelectionHistoryState,
documentSelectionHref,
documentSelectionSurvivedClose,
@@ -214,4 +215,83 @@ describe("document vault URL selection", () => {
),
).toBe("/documents?episode=ep-1&view=compact&q=MRT");
});
// ── The close and the check that follows it must agree ───────────────────
//
// Maximizing the Coach drawer is the one close-shaped transition that keeps
// `?doc=`: it is the return URL that lets browser-Back reconstruct the
// sheet. The dropped-traversal check added beside the close does not know
// that on its own — it sees a closed sheet and a selection still in the
// address bar, which is exactly the state it was written to repair, and it
// would repair away the way back a few hundred milliseconds later.
//
// Whether it won that race was a question about how fast the machine
// navigated to `/coach`, which is why it surfaced as one flaky mobile run.
// Both sides read the same predicate now, and these cases are what stop them
// drifting apart again.
it("keeps the selection when the drawer is being maximized", () => {
expect(coachHandoffRetainsSelection("doc-1", "doc-1", "navigate")).toBe(
true,
);
});
it("does not keep it for a dismissed drawer", () => {
expect(coachHandoffRetainsSelection("doc-1", "doc-1", "dismiss")).toBe(
false,
);
});
it("does not keep it for an ordinary close with no handoff", () => {
expect(coachHandoffRetainsSelection("doc-1", null, undefined)).toBe(false);
});
it("does not keep it when the handoff names a different document", () => {
expect(coachHandoffRetainsSelection("doc-1", "doc-2", "navigate")).toBe(
false,
);
});
it("agrees with what the close actually did, in both directions", () => {
// The property that matters, stated against the close itself rather than
// against the predicate alone: retained means the URL still carries the
// selection, and not-retained means it does not.
const retained = new MemoryHistory(["/today", "/documents?episode=ep-1"]);
retained.pushState(
documentSelectionHistoryState("doc-1"),
"",
"/documents?episode=ep-1&doc=doc-1",
);
closeDocumentSelectionAfterCoachHandoff(
retained,
"/documents",
"episode=ep-1&doc=doc-1",
"doc-1",
"doc-1",
"navigate",
);
expect(coachHandoffRetainsSelection("doc-1", "doc-1", "navigate")).toBe(
true,
);
expect(retained.href).toBe("/documents?episode=ep-1&doc=doc-1");
const closed = new MemoryHistory(["/today", "/documents?episode=ep-1"]);
closed.pushState(
documentSelectionHistoryState("doc-1"),
"",
"/documents?episode=ep-1&doc=doc-1",
);
closeDocumentSelectionAfterCoachHandoff(
closed,
"/documents",
"episode=ep-1&doc=doc-1",
"doc-1",
"doc-1",
"dismiss",
);
expect(coachHandoffRetainsSelection("doc-1", "doc-1", "dismiss")).toBe(
false,
);
expect(closed.href).toBe("/documents?episode=ep-1");
});
});
+36 -1
View File
@@ -180,6 +180,27 @@ export function closeDocumentSelectionHistoryEntry(
);
}
/**
* The selection is being KEPT on purpose, as the way back.
*
* Maximizing the Coach drawer hands the conversation to `/coach` and carries
* `?doc=` with it, so browser-Back can reconstruct the sheet. That is the one
* close-shaped transition that must leave the parameter exactly where it is.
*
* Named, and used by both the close and the check that follows it, because
* those two disagreeing is a bug with no symptom until it has one: the check
* would strip the return URL a few hundred milliseconds after the close
* deliberately preserved it, and whether it won that race was a question about
* how fast the machine navigated.
*/
export function coachHandoffRetainsSelection(
documentId: string,
handedOffDocumentId: string | null,
closeIntent: CoachCloseIntent | null | undefined,
): boolean {
return handedOffDocumentId === documentId && closeIntent === "navigate";
}
export function closeDocumentSelectionAfterCoachHandoff(
history: Parameters<typeof closeDocumentSelectionHistoryEntry>[0],
pathname: string,
@@ -188,7 +209,9 @@ export function closeDocumentSelectionAfterCoachHandoff(
handedOffDocumentId: string | null,
closeIntent: CoachCloseIntent | null | undefined,
): string | null {
if (handedOffDocumentId === documentId && closeIntent === "navigate") {
if (
coachHandoffRetainsSelection(documentId, handedOffDocumentId, closeIntent)
) {
return handedOffDocumentId;
}
@@ -727,6 +750,14 @@ export function DocumentsView() {
handedOffDocumentRef.current = docParam;
return;
}
// Read before the call below reassigns the ref: the same inputs the close
// itself decides on.
const retainedAsReturnUrl = coachHandoffRetainsSelection(
docParam,
handedOffDocumentRef.current,
coachLaunch?.closeIntent,
);
handedOffDocumentRef.current = closeDocumentSelectionAfterCoachHandoff(
window.history,
pathname,
@@ -736,6 +767,10 @@ export function DocumentsView() {
coachLaunch?.closeIntent,
);
// Nothing was closed: the selection is the way back from `/coach` and is
// meant to stay. Checking for it here would delete it.
if (retainedAsReturnUrl) return;
// Consuming the pushed entry is a REQUEST to the browser, not a guarantee.
//
// `history.back()` is queued against the session history, and a traversal