fix(nav): a navigation closes every overlay before it pushes
The overlay history stack let a navigation land ABOVE open overlays and "buried" their entries with a counter. Measured in Chrome: Shift+W over About navigated under the still-open dialog, the next Back both undid the navigation and closed About, and the Back after that did nothing; Shift+W on Home with the player open did nothing at all. The counter could also be spent by a different overlay closing first, and callers that could not report whether they pushed were assumed to have. setPage and openChannel now close every open overlay and push only once the history is back in sync (one entry per live overlay, no programmatic back() in flight, no reconcile queued), so nothing is ever buried and the whole mechanism goes: navigateOverOverlays, closeThenNavigate, buryUnder, setPage's boolean, and the three test files that pinned them. The palette closes only itself before running a row, so a theme picked over the player keeps the player. Shift+W on Home with the player open now closes it (owner decision 2026-09-09). The wait is on state, not on "a popstate": the old 250 ms fallback could run with back()s still in flight. An overlay that refuses to close within 1.5 s drops the command instead of being navigated out from under.
This commit is contained in:
@@ -1,43 +0,0 @@
|
||||
// @vitest-environment jsdom
|
||||
//
|
||||
// ONE test, ONE file, for the same reason CommandPalette.ordering.dom.test.tsx has its own: it
|
||||
// ASSERTS on a reconcile issuing a REAL, unstubbed programmatic history.back(), and history.ts's
|
||||
// overlay bookkeeping is module-global. history.dom.test.ts keeps its shared state clean by never
|
||||
// leaving a real back() in flight — its layers close by a genuine Back, a stubbed back(), or a
|
||||
// consumed burial mark — and this test would break that.
|
||||
import { describe, it, expect, vi } from "vitest";
|
||||
import { renderHook, act } from "@testing-library/react";
|
||||
import { navigateOverOverlays, useBackToClose } from "./history";
|
||||
|
||||
const settle = () =>
|
||||
act(async () => {
|
||||
await Promise.resolve();
|
||||
});
|
||||
|
||||
describe("navigateOverOverlays · burying a navigation that never happened", () => {
|
||||
// The round-2 defect. `navigateOverOverlays` marked every live overlay entry buried BEFORE it knew
|
||||
// whether the navigation pushed anything, and `setPage` early-returns when the requested page is
|
||||
// already current. Home is the default landing page, so Shift+W pressed while already on Home with
|
||||
// the player open buried the player's entry under a push that never happened: its unmount then
|
||||
// consumed the mark instead of reclaiming the entry with history.back(), leaving an orphan. The
|
||||
// user's next browser Back popped onto an identical `sfPage` with an empty overlay stack — a press
|
||||
// that changed nothing on screen, one more per play → Shift+W → close cycle.
|
||||
it("leaves the overlay's entry reclaimable when the navigation reports it pushed nothing", async () => {
|
||||
const overlay = renderHook(() => useBackToClose(() => {}));
|
||||
await settle();
|
||||
const back = vi.spyOn(window.history, "back");
|
||||
try {
|
||||
// What `setPage` returns from its early return: asked for the page we are already on.
|
||||
const navigate = vi.fn(() => false);
|
||||
act(() => navigateOverOverlays(navigate));
|
||||
expect(navigate).toHaveBeenCalledTimes(1);
|
||||
|
||||
overlay.unmount();
|
||||
await settle();
|
||||
// Nothing was pushed over it, so the entry is still ours — reclaimed, not silently orphaned.
|
||||
expect(back).toHaveBeenCalledTimes(1);
|
||||
} finally {
|
||||
back.mockRestore();
|
||||
}
|
||||
});
|
||||
});
|
||||
@@ -1,32 +1,55 @@
|
||||
// @vitest-environment jsdom
|
||||
//
|
||||
// Component-test lane pilot: the useBackToExit "back-ladder". This is the first jsdom/renderHook
|
||||
// test in the repo (the pure-node lane can't touch window.history / popstate). It exercises the
|
||||
// module-level overlay stack in history.ts through its only observable effect — which onExit fires.
|
||||
// The overlay history stack in history.ts, driven through its observable effects: which close/exit
|
||||
// callback fires, how many programmatic back()s go out, and when a command finally runs.
|
||||
//
|
||||
// A GENUINE user Back is simulated by dispatching `popstate` directly: the real-Back branch in
|
||||
// history.ts does NOT call history.back() (it only decrements its own historyDepth to match the
|
||||
// entry the browser already dropped), so a raw popstate reproduces it deterministically without
|
||||
// depending on jsdom's async back() semantics. The invariant that keeps the module-global
|
||||
// bookkeeping clean between tests here is NOT "only genuine Backs" — two closeThenNavigate cases
|
||||
// deliberately drive a reconcile into history.back() — it is that no REAL back() is ever left in
|
||||
// flight: a layer is closed by a genuine Back, by a STUBBED back() that lands its own popstate, or
|
||||
// by an unmount whose burial mark is consumed. A test that needs an unstubbed one belongs in its own
|
||||
// file (see history.burial.dom.test.ts).
|
||||
// A GENUINE user Back is simulated by dispatching `popstate` directly: the real-Back branch does NOT
|
||||
// call history.back() (it only decrements its own historyDepth to match the entry the browser already
|
||||
// dropped), so a raw popstate reproduces it deterministically. A PROGRAMMATIC back() — a reconcile
|
||||
// reclaiming a closed overlay's entry — is stubbed to land its own popstate, so no real jsdom back()
|
||||
// is ever left in flight. The bookkeeping is module-global: every test must leave the stack, the
|
||||
// depth and the in-flight count at zero for the next one.
|
||||
import { describe, it, expect, vi, afterEach } from "vitest";
|
||||
import { renderHook, act } from "@testing-library/react";
|
||||
import { closeThenNavigate, navigateOverOverlays, useBackToClose, useBackToExit } from "./history";
|
||||
import {
|
||||
closeOverlaysThenNavigate,
|
||||
closeTopOverlayThenRun,
|
||||
useBackToClose,
|
||||
useBackToExit,
|
||||
} from "./history";
|
||||
|
||||
// Let the mount effect's coalesced reconcile microtask settle.
|
||||
const settle = () =>
|
||||
act(async () => {
|
||||
await Promise.resolve();
|
||||
});
|
||||
// Past `afterClosing`'s hop onto a fresh task (a setTimeout 0).
|
||||
const tick = () =>
|
||||
act(async () => {
|
||||
await new Promise((r) => setTimeout(r, 0));
|
||||
});
|
||||
// A genuine user Back (browser already dropped the entry; popstate arrives with no pending back()).
|
||||
const userBack = () =>
|
||||
act(() => {
|
||||
window.dispatchEvent(new PopStateEvent("popstate", { state: window.history.state }));
|
||||
});
|
||||
// A programmatic back() that lands its popstate at once — the browser does it 1–13 ms later.
|
||||
const stubBack = () =>
|
||||
vi.spyOn(window.history, "back").mockImplementation(() => {
|
||||
window.dispatchEvent(new PopStateEvent("popstate", { state: window.history.state }));
|
||||
});
|
||||
/** A Back-closable layer whose close really unmounts it, as a closing dialog does. */
|
||||
function mountLayer(onClose: () => void) {
|
||||
const holder: { unmount?: () => void } = {};
|
||||
const r = renderHook(() =>
|
||||
useBackToClose(() => {
|
||||
onClose();
|
||||
holder.unmount?.();
|
||||
}),
|
||||
);
|
||||
holder.unmount = r.unmount;
|
||||
return r;
|
||||
}
|
||||
|
||||
afterEach(() => {
|
||||
vi.clearAllMocks();
|
||||
@@ -92,137 +115,179 @@ describe("useBackToExit back-ladder", () => {
|
||||
});
|
||||
});
|
||||
|
||||
describe("closeThenNavigate", () => {
|
||||
// The bug it exists for: an overlay's unmount reclaims its Back-to-close entry with a programmatic
|
||||
// history.back(). A pushState made before that lands is undone by it, so a dialog command that
|
||||
// navigates does nothing but close. PlayerModal hit it once; the command palette hit it again.
|
||||
it("closes first and navigates only after the reclaim lands", () => {
|
||||
const order: string[] = [];
|
||||
closeThenNavigate(
|
||||
() => order.push("close"),
|
||||
// Braced: `navigate` reports whether it PUSHED (see `buryUnder`), so a bare `push` returning
|
||||
// an array length is a type error rather than an accidental "yes".
|
||||
() => {
|
||||
order.push("navigate");
|
||||
},
|
||||
);
|
||||
expect(order).toEqual(["close"]);
|
||||
|
||||
window.dispatchEvent(new PopStateEvent("popstate"));
|
||||
expect(order).toEqual(["close", "navigate"]);
|
||||
});
|
||||
|
||||
it("navigates exactly once, however many pops follow", () => {
|
||||
describe("closeOverlaysThenNavigate", () => {
|
||||
// Every in-app navigation goes through it (setPage, openChannel). The trap: closing an overlay
|
||||
// reclaims its Back-to-close entry with a programmatic back(), and a pushState made before that
|
||||
// lands is undone by it — so the command would do nothing but close.
|
||||
it("navigates synchronously when no overlay is open", () => {
|
||||
const navigate = vi.fn();
|
||||
closeThenNavigate(() => {}, navigate);
|
||||
window.dispatchEvent(new PopStateEvent("popstate"));
|
||||
window.dispatchEvent(new PopStateEvent("popstate"));
|
||||
closeOverlaysThenNavigate(navigate);
|
||||
expect(navigate).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
|
||||
// E10 review round 1. Waiting for ONE reclaim is not enough when a SECOND Back-closable layer is
|
||||
// still mounted and the navigation is what unmounts it: that layer's own reconcile fired another
|
||||
// history.back(), which popped the entry the navigation had just pushed and bounced the user back
|
||||
// where they started (palette opened over the player → "go to Channels" landed on the feed).
|
||||
//
|
||||
// NOTE this one may NOT use the raw-popstate shortcut the rest of the file uses: a synthetic
|
||||
// popstate is read as a GENUINE Back, which pops the lower layer off the stack and decrements
|
||||
// historyDepth by itself — so the test would pass with or without the fix. The bug only appears
|
||||
// on the programmatic path, so back() is stubbed to land its own popstate instead.
|
||||
it("adopts the entries of layers the navigation itself unmounts", async () => {
|
||||
const back = vi.spyOn(window.history, "back").mockImplementation(() => {
|
||||
window.dispatchEvent(new PopStateEvent("popstate", { state: window.history.state }));
|
||||
});
|
||||
it("closes every open overlay, topmost first, and navigates only once all their entries are back", async () => {
|
||||
const back = stubBack();
|
||||
try {
|
||||
const player = renderHook(() => useBackToClose(() => {}));
|
||||
const closed: string[] = [];
|
||||
mountLayer(() => closed.push("player"));
|
||||
await settle();
|
||||
const palette = renderHook(() => useBackToClose(() => {}));
|
||||
mountLayer(() => closed.push("palette"));
|
||||
await settle();
|
||||
expect(back).not.toHaveBeenCalled(); // both layers hold an entry; nothing reclaimed yet
|
||||
|
||||
const navigate = vi.fn(() => window.history.pushState({ sfPage: "channels" }, ""));
|
||||
await act(async () => {
|
||||
closeThenNavigate(() => palette.unmount(), navigate);
|
||||
await Promise.resolve(); // let the palette's own reclaim reconcile run
|
||||
});
|
||||
expect(navigate).toHaveBeenCalledTimes(1);
|
||||
expect(back).toHaveBeenCalledTimes(1); // the palette's entry, and only that
|
||||
act(() => closeOverlaysThenNavigate(navigate));
|
||||
expect(closed).toEqual(["palette", "player"]);
|
||||
expect(navigate).not.toHaveBeenCalled();
|
||||
|
||||
player.unmount(); // …which is what the navigation does to the layer below
|
||||
await settle();
|
||||
expect(back).toHaveBeenCalledTimes(1); // must NOT back over the entry navigate just pushed
|
||||
await settle(); // the reconcile reclaims both entries in one go
|
||||
expect(back).toHaveBeenCalledTimes(2);
|
||||
// Not inside the popstate dispatch that settled it — a later listener would still apply the
|
||||
// OLD entry's page over the push.
|
||||
expect(navigate).not.toHaveBeenCalled();
|
||||
|
||||
await tick();
|
||||
expect(navigate).toHaveBeenCalledTimes(1);
|
||||
await tick();
|
||||
expect(back).toHaveBeenCalledTimes(2); // nothing reclaims over the entry navigate pushed
|
||||
expect(window.history.state).toMatchObject({ sfPage: "channels" });
|
||||
} finally {
|
||||
back.mockRestore();
|
||||
}
|
||||
});
|
||||
|
||||
// The mirror of the case above, and the OTHER door into `buryUnder` — round 3 found that only
|
||||
// `navigateOverOverlays` was pinned, so this call site could be reverted to the old unconditional
|
||||
// burial with the whole suite green. Reachable through the palette: `run: () => setPage(page)` is
|
||||
// a bare arrow, so `setPage`'s `false` really does arrive here when the chosen module is the one
|
||||
// already on screen. Nothing was pushed, so the layer underneath is still ours to reclaim.
|
||||
it("leaves the layer below reclaimable when the command navigated nowhere", async () => {
|
||||
// The old helper ran after "a popstate" OR a 250 ms fallback, whichever came first — so a reclaim
|
||||
// slower than the fallback undid the navigation. It must wait for the history, not the clock.
|
||||
it("waits for a back() still in flight instead of pushing under it", async () => {
|
||||
let land: (() => void) | undefined;
|
||||
const back = vi.spyOn(window.history, "back").mockImplementation(() => {
|
||||
window.dispatchEvent(new PopStateEvent("popstate", { state: window.history.state }));
|
||||
land = () =>
|
||||
window.dispatchEvent(new PopStateEvent("popstate", { state: window.history.state }));
|
||||
});
|
||||
try {
|
||||
const player = renderHook(() => useBackToClose(() => {}));
|
||||
mountLayer(() => {});
|
||||
await settle();
|
||||
const palette = renderHook(() => useBackToClose(() => {}));
|
||||
const navigate = vi.fn();
|
||||
act(() => closeOverlaysThenNavigate(navigate));
|
||||
await settle();
|
||||
expect(back).toHaveBeenCalledTimes(1);
|
||||
|
||||
const navigate = vi.fn(() => false); // setPage's early return: already on that page
|
||||
await act(async () => {
|
||||
closeThenNavigate(() => palette.unmount(), navigate);
|
||||
await Promise.resolve();
|
||||
await new Promise((r) => setTimeout(r, 300)); // past where the old fallback fired
|
||||
});
|
||||
expect(navigate).toHaveBeenCalledTimes(1);
|
||||
expect(back).toHaveBeenCalledTimes(1); // the palette's own entry
|
||||
expect(navigate).not.toHaveBeenCalled();
|
||||
|
||||
player.unmount();
|
||||
await settle();
|
||||
expect(back).toHaveBeenCalledTimes(2); // and the player's, because nothing was pushed over it
|
||||
act(() => land?.());
|
||||
await tick();
|
||||
expect(navigate).toHaveBeenCalledTimes(1);
|
||||
} finally {
|
||||
back.mockRestore();
|
||||
}
|
||||
});
|
||||
|
||||
// Without this the command is not late, it never happens: an overlay closed in the same tick it
|
||||
// opened never pushed an entry, so no reclaim popstate is ever coming.
|
||||
it("navigates anyway when no reclaim popstate ever arrives", () => {
|
||||
// No such overlay exists today, but a future "discard your changes?" guard must not have the page
|
||||
// switched out from under it: dropping the command is the honest outcome.
|
||||
it("drops the command when an overlay does not actually close", async () => {
|
||||
const back = stubBack();
|
||||
const stubborn = renderHook(() => useBackToClose(() => {}));
|
||||
await settle();
|
||||
vi.useFakeTimers();
|
||||
try {
|
||||
const navigate = vi.fn();
|
||||
closeThenNavigate(() => {}, navigate);
|
||||
act(() => closeOverlaysThenNavigate(navigate));
|
||||
act(() => {
|
||||
vi.advanceTimersByTime(2000);
|
||||
});
|
||||
expect(navigate).not.toHaveBeenCalled();
|
||||
vi.advanceTimersByTime(300);
|
||||
expect(navigate).toHaveBeenCalledTimes(1);
|
||||
} finally {
|
||||
vi.useRealTimers();
|
||||
stubborn.unmount();
|
||||
await settle();
|
||||
expect(back).toHaveBeenCalledTimes(1); // its entry is still reclaimed on the way out
|
||||
back.mockRestore();
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
describe("navigateOverOverlays", () => {
|
||||
// The global-shortcut half of the same trap `closeThenNavigate` solves. Shift+W does not know an
|
||||
// overlay is open and cannot close it — it just navigates, and the page change unmounts the player
|
||||
// (which lives inside the module column). That unmount's reconcile then reclaimed the entry the
|
||||
// navigation had only just pushed, so the player closed and the page snapped straight back.
|
||||
it("a navigation made from outside an open overlay survives that overlay's unmount", async () => {
|
||||
const overlay = renderHook(() => useBackToClose(() => {}));
|
||||
await settle();
|
||||
const back = vi.spyOn(window.history, "back");
|
||||
// Shift+W pressed on Home with the player open: setPage finds nothing to push. The player must
|
||||
// still close (owner decision 2026-09-09), and its entry must be reclaimed rather than orphaned —
|
||||
// an orphan is a later browser Back that changes nothing on screen.
|
||||
it("closes the overlay and reclaims its entry when the navigation turns out to go nowhere", async () => {
|
||||
const back = stubBack();
|
||||
try {
|
||||
const navigate = vi.fn(() => window.history.pushState({ sfPage: "home" }, ""));
|
||||
act(() => navigateOverOverlays(navigate));
|
||||
expect(navigate).toHaveBeenCalledTimes(1);
|
||||
|
||||
overlay.unmount(); // what the page change does to the player
|
||||
const closed = vi.fn();
|
||||
mountLayer(closed);
|
||||
await settle();
|
||||
// The mark is consumed instead: no back(), so the entry `navigate` pushed is still current.
|
||||
const navigate = vi.fn(); // setPage's early return: already on that page
|
||||
act(() => closeOverlaysThenNavigate(navigate));
|
||||
await settle();
|
||||
await tick();
|
||||
expect(closed).toHaveBeenCalledTimes(1);
|
||||
expect(navigate).toHaveBeenCalledTimes(1);
|
||||
expect(back).toHaveBeenCalledTimes(1);
|
||||
} finally {
|
||||
back.mockRestore();
|
||||
}
|
||||
});
|
||||
|
||||
// An overlay closed in the tick it opened never pushed an entry, so no popstate is coming at all.
|
||||
it("navigates when the overlay closes before its entry was ever pushed", async () => {
|
||||
const back = stubBack();
|
||||
try {
|
||||
mountLayer(() => {}); // no settle: its reconcile is still queued
|
||||
const navigate = vi.fn();
|
||||
act(() => closeOverlaysThenNavigate(navigate));
|
||||
await settle();
|
||||
await tick();
|
||||
expect(navigate).toHaveBeenCalledTimes(1);
|
||||
expect(back).not.toHaveBeenCalled();
|
||||
} finally {
|
||||
back.mockRestore();
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
describe("closeTopOverlayThenRun", () => {
|
||||
// The palette's rows. Only the palette closes: a theme picked over the player must leave it open.
|
||||
it("closes only the top overlay and runs once its entry is back, leaving the one below", async () => {
|
||||
const back = stubBack();
|
||||
try {
|
||||
const playerClosed = vi.fn();
|
||||
mountLayer(playerClosed);
|
||||
await settle();
|
||||
const palette = renderHook(() => useBackToClose(() => {}));
|
||||
await settle();
|
||||
|
||||
const run = vi.fn();
|
||||
act(() => closeTopOverlayThenRun(() => palette.unmount(), run));
|
||||
expect(run).not.toHaveBeenCalled();
|
||||
await settle();
|
||||
await tick();
|
||||
expect(run).toHaveBeenCalledTimes(1);
|
||||
expect(back).toHaveBeenCalledTimes(1); // the palette's entry, and only that
|
||||
expect(playerClosed).not.toHaveBeenCalled();
|
||||
|
||||
await userBack(); // teardown: the player closes by a genuine Back
|
||||
expect(playerClosed).toHaveBeenCalledTimes(1);
|
||||
await settle();
|
||||
} finally {
|
||||
back.mockRestore();
|
||||
}
|
||||
});
|
||||
|
||||
it("runs exactly once, however many popstates follow", async () => {
|
||||
const back = stubBack();
|
||||
try {
|
||||
const palette = renderHook(() => useBackToClose(() => {}));
|
||||
await settle();
|
||||
const run = vi.fn();
|
||||
act(() => closeTopOverlayThenRun(() => palette.unmount(), run));
|
||||
await settle();
|
||||
await tick();
|
||||
await userBack();
|
||||
await userBack();
|
||||
await tick();
|
||||
expect(run).toHaveBeenCalledTimes(1);
|
||||
} finally {
|
||||
back.mockRestore();
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
+93
-105
@@ -150,6 +150,15 @@ export function useHistorySubview<T>(
|
||||
// listener mistook the leaving modal's back() for a genuine user Back and closed itself, and
|
||||
// (b) a mangled history where the pointer ended up BEHIND the surviving entry, so a later browser
|
||||
// Back walked off the app entirely instead of closing the modal. Coalescing removes both.
|
||||
//
|
||||
// A navigation never lands UNDER an open overlay: `setPage` and `openChannel` close every overlay
|
||||
// first and push only once their entries are back (`closeOverlaysThenNavigate`). The design before
|
||||
// E11 let the push land above the overlays and "buried" their entries with a counter, and that
|
||||
// counter was a whole family of bugs — a burial spent by a different overlay closing first, a
|
||||
// navigation that could not report whether it pushed and was assumed to have, one browser Back that
|
||||
// both undid the navigation AND closed the dialog above it, and the dead Back press after that (the
|
||||
// last two measured in Chrome, E11 S1). With nothing ever buried, the stack and the history agree by
|
||||
// construction.
|
||||
type Overlay = { token: number; cb: () => void };
|
||||
const overlayStack: Overlay[] = [];
|
||||
let overlayCounter = 0;
|
||||
@@ -157,64 +166,25 @@ let overlayCounter = 0;
|
||||
let historyDepth = 0;
|
||||
// Programmatic history.back() calls still awaiting their popstate — each must be ignored.
|
||||
let pendingProgrammatic = 0;
|
||||
// Overlay entries a navigation has PUSHED PAST. They are still real history entries and their
|
||||
// overlays may still be mounted, but they are no longer on top, so reclaiming one with history.back()
|
||||
// would pop the navigation instead. Reconcile consumes them silently. See closeThenNavigate.
|
||||
let buriedEntries = 0;
|
||||
let reconcileScheduled = false;
|
||||
let listenerInstalled = false;
|
||||
|
||||
/** Run a navigation and mark the live overlay entries buried under it — but ONLY if it really
|
||||
* pushed an entry. A navigation that reports `false` pushed nothing, so its overlays are still on
|
||||
* top and must stay reclaimable: `setPage` early-returns when the requested page is already
|
||||
* current, and Home is the default landing page, so "Shift+W while already on Home with the player
|
||||
* open" navigated nowhere yet still buried the player's entry. The player's unmount then consumed
|
||||
* the mark instead of issuing history.back(), orphaning its browser entry — and the next Back
|
||||
* popped onto an identical `sfPage` with an empty overlay stack, i.e. a press that did nothing
|
||||
* (round 2). One rule for both entry points below — but read the next paragraph before trusting it
|
||||
* at the `closeThenNavigate` one, where most callers cannot report at all.
|
||||
*
|
||||
* It fails OPEN: only a literal `false` skips the mark, so a `navigate` that reports nothing is
|
||||
* assumed to have pushed. That is what the code did unconditionally before, so nothing regressed by
|
||||
* adding the rule — it just does not yet reach everywhere. Today only `navigateOverOverlays`
|
||||
* (App's Shift+W → `setPage`) is fully covered. Of `closeThenNavigate`'s callers, only a palette
|
||||
* row written as a bare `() => setPage(page)` forwards the boolean: `CommandItem.run` is typed
|
||||
* `() => void`, so the block-bodied and non-navigating rows report nothing — and neither does
|
||||
* `openChannel`, which has its OWN no-push branch (it `replaceState`s when the entry already
|
||||
* carries `_chan`) and is reached both from a palette row and from PlayerModal's channel-name jump.
|
||||
* Each of those still marks a live sibling overlay's entry buried under a push that never happened,
|
||||
* costing one dead browser-Back press when that overlay is closed by its own control.
|
||||
*
|
||||
* Closing it is three changes, not one — widen `CommandItem.run` to `() => boolean | void`, return
|
||||
* the verdict from the block-bodied rows, and give `openChannel` the boolean `setPage` now has — so
|
||||
* it is deferred to the project backlog rather than bolted on here.
|
||||
*
|
||||
* Marking AFTER the call is safe: `navigate()` is synchronous and only `reconcileHistory` (a
|
||||
* microtask) and the popstate listener ever move `historyDepth`. */
|
||||
function buryUnder(navigate: () => boolean | void): void {
|
||||
if (navigate() !== false) buriedEntries = historyDepth;
|
||||
}
|
||||
// Re-checked after every reconcile — see `afterClosing`.
|
||||
const syncWaiters = new Set<() => void>();
|
||||
|
||||
// Push/pop history entries until their count matches the live overlay count. Runs once per tick.
|
||||
function reconcileHistory(): void {
|
||||
reconcileScheduled = false;
|
||||
const want = overlayStack.length;
|
||||
// A genuine user Back decrements historyDepth on its own, so a burial can outlive the entries it
|
||||
// described; never claim more buried entries than there are entries.
|
||||
if (buriedEntries > historyDepth) buriedEntries = historyDepth;
|
||||
while (historyDepth < want) {
|
||||
historyDepth++;
|
||||
window.history.pushState({ ...window.history.state, _ov: ++overlayCounter }, "");
|
||||
}
|
||||
while (historyDepth > want && buriedEntries > 0) {
|
||||
historyDepth--; // buried: the browser entry stays, but it is not ours to reclaim
|
||||
buriedEntries--;
|
||||
}
|
||||
while (historyDepth > want) {
|
||||
historyDepth--;
|
||||
pendingProgrammatic++;
|
||||
window.history.back();
|
||||
}
|
||||
syncWaiters.forEach((cb) => cb());
|
||||
}
|
||||
|
||||
function scheduleReconcile(): void {
|
||||
@@ -240,74 +210,92 @@ function ensureOverlayListener(): void {
|
||||
});
|
||||
}
|
||||
|
||||
/** Close an overlay that holds a `useBackToClose` entry and THEN navigate — in that order, waiting
|
||||
* for the browser to confirm it.
|
||||
*
|
||||
* The trap: closing unmounts the overlay, whose cleanup reconciles the history by issuing a
|
||||
* programmatic `history.back()`. A `pushState` made before that lands (i.e. anything `setPage` /
|
||||
* `openChannel` does synchronously) is simply undone by it, so the navigation silently does
|
||||
* nothing. PlayerModal hit this first — clicking a video's channel name only closed the player —
|
||||
* and the command palette hit it again the day it was written, which is why the fix lives here
|
||||
* instead of being re-derived at each call site.
|
||||
*
|
||||
* Only call this from INSIDE an overlay that uses `useBackToClose`: the popstate we wait for is
|
||||
* that overlay's own entry being reclaimed, and without one it never arrives. */
|
||||
export function closeThenNavigate(close: () => void, navigate: () => boolean | void): void {
|
||||
let done = false;
|
||||
// Armed BEFORE the close, so it is a plain const and there is no declaration order to reason about.
|
||||
// Degrades honestly if no popstate is coming at all — an overlay closed in the same tick it opened
|
||||
// never pushed an entry, so there is nothing to reclaim and the command would hang forever.
|
||||
// Navigating a beat late beats a control that silently does nothing.
|
||||
const fallback = setTimeout(() => run(), 250);
|
||||
const run = () => {
|
||||
if (done) return;
|
||||
done = true;
|
||||
window.removeEventListener("popstate", onPop);
|
||||
clearTimeout(fallback);
|
||||
// Every overlay entry still counted is now BURIED under the entry the navigation just pushed, so
|
||||
// a later unmount must NOT reclaim it with history.back() — that pops the navigation instead and
|
||||
// silently bounces the user back where they started (a palette opened over the player: closing
|
||||
// the palette landed its popstate, the navigation unmounted the player too, and the player's
|
||||
// reconcile ate the new page entry). MARK them rather than zeroing the depth: round 1 set
|
||||
// `historyDepth = 0` outright, which also desynced the counter from overlays the navigation does
|
||||
// NOT unmount — About/ReleaseNotes live outside the page column — so one Back both navigated and
|
||||
// closed them (round 2). `buryUnder` skips the mark when the command reports it navigated
|
||||
// nowhere — which here is only the palette's bare `() => setPage(page)` module rows; see the
|
||||
// fail-open limit documented on `buryUnder` for the rows that cannot report.
|
||||
buryUnder(navigate);
|
||||
};
|
||||
const onPop = () => {
|
||||
// The same reconcile can issue MORE than one back(); a pushState made between them would be
|
||||
// popped by the next. Wait for the last one to land.
|
||||
if (pendingProgrammatic > 0) return;
|
||||
run();
|
||||
};
|
||||
window.addEventListener("popstate", onPop);
|
||||
close();
|
||||
/** The history holds exactly one entry per live overlay and no programmatic back() is still in
|
||||
* flight — so a pushState made now cannot be undone by one. */
|
||||
function historyInSync(): boolean {
|
||||
return historyDepth === overlayStack.length && pendingProgrammatic === 0 && !reconcileScheduled;
|
||||
}
|
||||
|
||||
/** Navigate from OUTSIDE any overlay — a global keyboard shortcut — when one may be open.
|
||||
// How long an overlay gets to actually close before its command is dropped. Closing is one React
|
||||
// render (measured back()→popstate: 1–13 ms in Chrome); nothing in the app refuses a close today,
|
||||
// but a future "discard your changes?" guard must not be navigated out from under.
|
||||
const CLOSE_TIMEOUT_MS = 1500;
|
||||
|
||||
/** Close `targets` (via `close`), then `run` once they have unmounted AND the history has caught up.
|
||||
*
|
||||
* `closeThenNavigate` is for a control that lives INSIDE the overlay and can close it, and it waits
|
||||
* for that overlay's own entry to come back. A global key has neither: it does not know which
|
||||
* overlay is open, or whether one is at all. It just navigates, and the page change unmounts
|
||||
* whatever the module column was holding — whose reconcile then reclaims the entry the navigation
|
||||
* had just pushed. Shift+W over an open player closed the player and snapped back to the page it
|
||||
* started on, which reads as a dead shortcut.
|
||||
* The trap: closing unmounts an overlay, whose cleanup reconciles by issuing a programmatic
|
||||
* `history.back()`. A pushState made before that lands is undone by it, so a command that navigates
|
||||
* silently does nothing but close. PlayerModal hit it first; the command palette the day it was
|
||||
* written.
|
||||
*
|
||||
* Same fix as `closeThenNavigate`, one step shorter: bury the live overlay entries FIRST, so a
|
||||
* later unmount consumes the marks instead of issuing `history.back()`. There is no popstate to
|
||||
* wait for, because nothing is being closed on purpose. An overlay the navigation does NOT unmount
|
||||
* (the palette and the cheat sheet live above the page column) keeps its entry buried too — the
|
||||
* same trade `closeThenNavigate` already makes, and the reason closing the palette afterwards no
|
||||
* longer pops the page out from under the user.
|
||||
* It waits for a STATE, not an event. The helper before E11 waited for "a popstate" plus a 250 ms
|
||||
* fallback — but one reconcile can issue several back()s, an overlay closed in the tick it opened
|
||||
* issues none, and the fallback then ran with back()s still in flight. Here the reconcile and every
|
||||
* popstate re-check one predicate.
|
||||
*
|
||||
* `navigate` reports whether it actually pushed — see `buryUnder`, which is why `setPage` returns a
|
||||
* boolean rather than this guessing from `history.length` (a `back()` followed by a `pushState`
|
||||
* leaves it unchanged) or from the entry's `sfScroll` id (`openChannel` pushes without minting
|
||||
* one). */
|
||||
export function navigateOverOverlays(navigate: () => boolean | void): void {
|
||||
buryUnder(navigate);
|
||||
* `run` goes out on a fresh task, never inside the popstate dispatch that settled it: listeners
|
||||
* registered before ours (NavigationProvider's) have read that event already, but ones after it
|
||||
* have not, and a page applied from the OLD entry after our push would undo the navigation. */
|
||||
function afterClosing(targets: readonly Overlay[], close: () => void, run: () => void): void {
|
||||
let done = false;
|
||||
let queued = false;
|
||||
const gone = () => targets.every((o) => !overlayStack.includes(o));
|
||||
const ready = () => !done && gone() && historyInSync();
|
||||
const stop = () => {
|
||||
done = true;
|
||||
syncWaiters.delete(check);
|
||||
window.removeEventListener("popstate", check);
|
||||
clearTimeout(timeout);
|
||||
};
|
||||
function check(): void {
|
||||
if (queued || !ready()) return;
|
||||
queued = true;
|
||||
setTimeout(() => {
|
||||
queued = false;
|
||||
if (!ready()) return;
|
||||
stop();
|
||||
run();
|
||||
}, 0);
|
||||
}
|
||||
const timeout = setTimeout(() => {
|
||||
if (done) return;
|
||||
stop();
|
||||
// Still mounted: the overlay refused to close, so drop the command rather than navigate under
|
||||
// it. Gone but out of sync: a back() never reported back — run late rather than never.
|
||||
if (gone()) run();
|
||||
}, CLOSE_TIMEOUT_MS);
|
||||
syncWaiters.add(check);
|
||||
window.addEventListener("popstate", check);
|
||||
close();
|
||||
check();
|
||||
}
|
||||
|
||||
/** Navigate over whatever overlays are open: close them all (topmost first, the order Back peels
|
||||
* them in), then navigate once their entries are back. Synchronous when nothing is open — the
|
||||
* ordinary rail click.
|
||||
*
|
||||
* `setPage` and `openChannel` call it themselves, so no call site can forget it: a global shortcut,
|
||||
* a toast's "View" clicked inside the player, a palette row, the player's channel link. Closing is
|
||||
* what a page change already did to the module-owned overlays (it unmounts them); for the ones above
|
||||
* the page column (About, the cheat sheet, the palette) it means the destination is what the user
|
||||
* sees, not a dialog still standing over it. A navigation that turns out to go nowhere — Shift+W on
|
||||
* Home with the player open — still closes, so the key always ends with the answer on screen (owner
|
||||
* decision 2026-09-09). */
|
||||
export function closeOverlaysThenNavigate(navigate: () => void): void {
|
||||
if (overlayStack.length === 0 && historyInSync()) {
|
||||
navigate();
|
||||
return;
|
||||
}
|
||||
const targets = [...overlayStack];
|
||||
afterClosing(targets, () => [...targets].reverse().forEach((o) => o.cb()), navigate);
|
||||
}
|
||||
|
||||
/** Close the overlay a control lives in, then run its command once that overlay's entry is back —
|
||||
* the command palette's rows. Overlays underneath are left alone: a theme picked over the player
|
||||
* must not close the player, and a row that navigates closes it through `setPage` anyway. */
|
||||
export function closeTopOverlayThenRun(close: () => void, run: () => void): void {
|
||||
const top = overlayStack[overlayStack.length - 1];
|
||||
afterClosing(top ? [top] : [], close, run);
|
||||
}
|
||||
|
||||
/** While the calling component is mounted, Back (or the browser/mouse back button) closes it via
|
||||
|
||||
@@ -1,40 +0,0 @@
|
||||
// @vitest-environment jsdom
|
||||
//
|
||||
// `navigateTo` is what a toast's action calls ("Download added — View"), and that click happens
|
||||
// INSIDE an overlay's toast host. The page change unmounts the page-owned overlay (the player), whose
|
||||
// Back-to-close cleanup then reclaimed the history entry the navigation had just pushed — so the user
|
||||
// landed back where they started with the player closed instead of on the Downloads page
|
||||
// (deep-review round 4). Same trap, and the same fix, as the global Shift+W shortcut.
|
||||
//
|
||||
// Its own file: `lib/history`'s depth bookkeeping is module state.
|
||||
import { describe, it, expect, vi } from "vitest";
|
||||
import { renderHook, act } from "@testing-library/react";
|
||||
import { useBackToClose } from "./history";
|
||||
import { navigateTo, setNavigator } from "./nav";
|
||||
|
||||
const settle = () =>
|
||||
act(async () => {
|
||||
await Promise.resolve();
|
||||
});
|
||||
|
||||
describe("navigateTo from inside an overlay", () => {
|
||||
it("buries the overlay's entry, so closing it does not pop the page just opened", async () => {
|
||||
const overlay = renderHook(() => useBackToClose(() => {}));
|
||||
await settle();
|
||||
const back = vi.spyOn(window.history, "back");
|
||||
try {
|
||||
setNavigator(() => {
|
||||
window.history.pushState({ sfPage: "downloads" }, "");
|
||||
return true; // as App's setPage reports: it really pushed
|
||||
});
|
||||
act(() => navigateTo("downloads"));
|
||||
overlay.unmount(); // the page change unmounts the player
|
||||
await settle();
|
||||
expect(back).not.toHaveBeenCalled();
|
||||
expect(window.history.state).toMatchObject({ sfPage: "downloads" });
|
||||
} finally {
|
||||
back.mockRestore();
|
||||
setNavigator(() => {});
|
||||
}
|
||||
});
|
||||
});
|
||||
+9
-10
@@ -1,21 +1,20 @@
|
||||
import { navigateOverOverlays } from "./history";
|
||||
import type { Page } from "./urlState";
|
||||
|
||||
// Tiny decoupled navigator so deep components (a feed card's download toast, the Download
|
||||
// Center) can switch pages without threading setPage through the whole tree. App registers
|
||||
// its setPage on mount; navigateTo is a no-op until then.
|
||||
let _navigate: ((p: Page) => boolean | void) | null = null;
|
||||
// Center) can switch pages without threading setPage through the whole tree. NavigationProvider
|
||||
// registers its setPage on mount; navigateTo is a no-op until then.
|
||||
let _navigate: ((p: Page) => void) | null = null;
|
||||
|
||||
export function setNavigator(fn: (p: Page) => boolean | void): void {
|
||||
export function setNavigator(fn: (p: Page) => void): void {
|
||||
_navigate = fn;
|
||||
}
|
||||
|
||||
/** Over whatever overlay is open: the "View" on a "Download added" toast is clicked INSIDE the player
|
||||
* (its toast host), and the page change unmounts the page-owned player, whose Back-to-close cleanup
|
||||
* then popped the entry the navigation had just pushed — the user landed back on the feed with the
|
||||
* player gone (deep-review round 4). Same trap, same fix as Shift+W (see `navigateOverOverlays`). */
|
||||
/** The "View" on a "Download added" toast is clicked INSIDE the player (its toast host). That is
|
||||
* safe because `setPage` closes every open overlay before it pushes (see
|
||||
* `lib/history.closeOverlaysThenNavigate`) — without it the player's Back-to-close cleanup popped the
|
||||
* entry the navigation had just pushed (deep-review round 4). */
|
||||
export function navigateTo(p: Page): void {
|
||||
navigateOverOverlays(() => _navigate?.(p));
|
||||
_navigate?.(p);
|
||||
}
|
||||
|
||||
/** Should this click be handled IN-APP, or left to the browser?
|
||||
|
||||
Reference in New Issue
Block a user