diff --git a/src/components/documents/__tests__/documents-view-navigation.test.ts b/src/components/documents/__tests__/documents-view-navigation.test.ts index 0621f2478..ab96a78a8 100644 --- a/src/components/documents/__tests__/documents-view-navigation.test.ts +++ b/src/components/documents/__tests__/documents-view-navigation.test.ts @@ -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"); + }); }); diff --git a/src/components/documents/documents-view.tsx b/src/components/documents/documents-view.tsx index e480fc7e3..dbb049ba4 100644 --- a/src/components/documents/documents-view.tsx +++ b/src/components/documents/documents-view.tsx @@ -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[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