fix(e24): review round 2 — a buried history entry, a stale hero, and a spinner that was not ours

Nine of the round's ten surviving findings. The tenth (a failed shelf reorder leaving an order the server rejected on screen) is on ROADMAP.md: its obvious remedy reintroduces the overlapping-move defect the code already documents, so it wants a design, not a review edit. - `navigateOverOverlays` buried the live overlay entries before knowing whether the navigation pushed one. `setPage` no-ops when the page is already current and Home is the default landing page, so Shift+W over a player ON Home marked its entry buried under a push that never happened: the unmount consumed the mark instead of reclaiming the entry, and the next browser Back was a press that did nothing. `setPage` now reports whether it navigated, and one `buryUnder` rule serves both entry points — `closeThenNavigate` takes the same no-op navigation from a palette command for the page you are already on. - The hero could answer a new time budget with the previous one's pick. The chip row resets seed/seen/press but cannot reset an answer that already resolved, and `keepPreviousData` chained through the disabled seed-0 query — so a reroll under "10 perc" showed the film picked for "Bármennyi" while it loaded, and kept it if the request failed. `lastPick` now carries the server's own budget echo, and the placeholder is scoped to the budget. - The reroll button spun and swallowed clicks after mark-watched, bookmark, pin and shelf moves: the pick key sits under the invalidated `discover` prefix on purpose, so raw `isFetching` was true for background refetches. It now means "a press of mine is outstanding" — which still covers the first press, where there is no placeholder. - Failed writes on Home toasted the page's LOAD-failure string over a fully loaded page; they get their own key (HU+EN). The empty-state reroll had no `loading`, so the press was unacknowledged and a second click skipped a tier. `danger` moved into the shared unpin row so the same command is not red on the shelf and neutral in the rail below it. - Two comments claimed things the code does not do: the `exclude` cap guards no "unbounded scan" (it never reaches SQL), and the route test that named the cap sends three keys. Tests: the burial rule gets its own file (it is the one assertion that lets a reconcile issue a real programmatic back()); App's keymap wiring gets a source contract, because dropping it left all 818 tests green; `add_pin`'s IntegrityError arm was reached by nothing. Every new test mutation-checked red, both edges where the predicate has two. Gate: ruff, ruff format, 3x tsc, eslint (0 errors), prettier, knip, vitest 822, pytest 1005.
2026-09-09 00:12:53 +02:00
parent 3709b71a94
commit e4ffd59ca7
13 changed files with 317 additions and 32 deletions
@@ -62,8 +62,13 @@ def get_discover(
    }


-# How many already-shown cards a reroll may carry. The client keeps a rolling window (see
-# `Home.tsx`); the cap is here so a crafted URL cannot turn the exclusion list into an unbounded scan.
+# How many already-shown cards a reroll may carry, matched to the client's own rolling window
+# (`SEEN_WINDOW` in `Home.tsx`). A cheap sanity clamp, and deliberately no more than that: `exclude`
+# never reaches SQL — `hero.pick` uses it only as set membership over an already-bounded candidate
+# pool (the resume rail's `RAIL_LIMIT` rows, the shelf, `SURPRISE_DRAW` surprise draws) — so there is
+# no scan for a crafted URL to unbound, and the set is built in full before this slice anyway. The
+# comment that claimed otherwise was corrected in round 2; the request line's own length is what
+# actually bounds a hostile `exclude`.
 MAX_EXCLUDE = 32


@@ -854,10 +854,15 @@ def test_pinning_a_plex_movie_plays_itself_without_a_cycle(db, as_user):

 def test_the_reroll_endpoint_parses_its_whole_exclusion_list_and_its_press_count(db, as_user):
    """The reroll's behaviour is covered at the domain level; the ROUTE's query-string translation —
-    the comma split, the cap, the `start` passthrough — was not covered at all, so a change there
-    could hand back the same card on every press with the suite green. Excluding all three candidates
-    is what proves the whole list parsed: a delimiter change leaves one unsplit string in the set,
-    and a hero comes back."""
+    the comma split and the `start` passthrough — was not covered at all, so a change there could
+    hand back the same card on every press with the suite green. Excluding all three candidates is
+    what proves the whole list parsed: a delimiter change leaves one unsplit string in the set, and a
+    hero comes back.
+
+    NOT the `MAX_EXCLUDE` clamp, which this sends three keys against a cap of 32 and cannot see. The
+    docstring used to claim it did (round 2). The clamp is deliberately left unpinned: it guards
+    nothing the route would otherwise do — see the constant's own comment — and a test that sent 33
+    keys could only assert that the request still answers, which the comma split already proves."""
    user, _ = _seed_yt(db)
    db.add(
        VideoState(
@@ -887,6 +892,23 @@ def test_pinning_something_that_does_not_exist_is_a_404(db, as_user):
    assert r.status_code == 404


+def test_a_pin_whose_target_vanishes_mid_request_is_a_404_not_a_500(db, as_user, monkeypatch):
+    """`add_pin`'s `except IntegrityError` arm was reached by no test at all, so the 500→404
+    narrowing could be reverted with the whole suite green (round 2). The 404 above comes from the
+    `resolve_ref` PRE-CHECK, a different branch that returns before the INSERT.
+
+    The race the arm exists for — a Plex sync dropping the row between `resolve_ref` confirming it
+    and the INSERT — needs two transactions, but the BRANCH does not: a `resolve_ref` pointing at an
+    id no row carries lets the pre-check pass and trips the same FK on commit, with the re-query
+    finding no winner. The sentinel has to FIT `comfort_pins.video_id` (varchar(16)) — a longer one
+    raises `DataError` on the way in, which this arm does not catch and never meant to. Its sibling arm (a concurrent duplicate, where the re-query DOES find a winner
+    and answers 200) genuinely needs a second connection and stays uncovered."""
+    user, _ = _seed_yt(db)
+    monkeypatch.setattr(pins, "resolve_ref", lambda *a, **k: ("video_id", "gone-mid-req"))
+    r = as_user(user).post("/api/discover/pins", json={"kind": "video", "ref": "vshort"})
+    assert r.status_code == 404
+
+
 def test_another_users_playlist_is_not_pinnable(db, as_user):
    """404, not 403 — a 403 would confirm the id exists to a stranger."""
    user, _ = _seed_yt(db)
@@ -0,0 +1,33 @@
+// The global Shift+W wiring, pinned at its call site.
+//
+// What this is: a source contract, the same idiom as uiBoundary.contract.test.ts — a structural rule
+// asserted directly rather than left to prose. What it is NOT: a behaviour test. The behaviour lives
+// in history.dom.test.ts and history.burial.dom.test.ts, which drive `navigateOverOverlays` against
+// real overlay entries; the end-to-end gesture (Shift+W over an open player, then Back) is a browser
+// check, because jsdom reproduces history ordering only as bookkeeping.
+//
+// It exists because the composition line itself was unpinned: App.tsx is the one module no test
+// renders (main.tsx lazy-imports it and nothing else does), globalKeymap.dom.test.tsx drives the
+// hook with its OWN handler, and history.dom.test.ts drives the helper with its own navigate — so
+// dropping `navigateOverOverlays` from App left all 818 tests green while the user-visible bug came
+// straight back (round 2, verified by running that revert).
+import { readFileSync } from "node:fs";
+import { fileURLToPath } from "node:url";
+
+import { describe, expect, it } from "vitest";
+
+const APP = readFileSync(fileURLToPath(new URL("./App.tsx", import.meta.url)), "utf8");
+
+describe("App · global keymap wiring", () => {
+  it("routes the global home shortcut through navigateOverOverlays", () => {
+    const call = /useGlobalKeymap\(([\s\S]*?)\);/.exec(APP)?.[1];
+    // A rename or a move is a legitimate change — but then this test is the thing that has to be
+    // updated deliberately, which is the whole point of pinning it.
+    expect(
+      call,
+      "App.tsx no longer calls useGlobalKeymap in a shape this test can read",
+    ).toBeTypeOf("string");
+    expect(call).toContain("navigateOverOverlays");
+    expect(call).toContain('setPage("home")');
+  });
+});
@@ -145,7 +145,9 @@ export default function App() {
  // Through `navigateOverOverlays`, because a global key can fire while an overlay is open and does
  // not know it: the page change unmounts the player (it lives inside the module), whose reconcile
  // then reclaimed the entry this push had just made — Shift+W over a playing video closed the
-  // player and snapped straight back to the feed.
+  // player and snapped straight back to the feed. `setPage`'s boolean is load-bearing here, not
+  // decoration: pressed while ALREADY on Home (the default landing page) it navigates nowhere, and
+  // burying the open player's entry under a push that never happened orphaned it — see `buryUnder`.
  useGlobalKeymap(!!meQuery.data, () => navigateOverOverlays(() => setPage("home")));

  // Pin this tab to whatever account it first loaded as. Without this, a tab that never picked
@@ -295,6 +295,60 @@ describe("Home", () => {
    expect(screen.queryByText("Hero v1")).toBeNull();
  });

+  it("never answers a new budget with the previous budget's pick", async () => {
+    // Both halves of the hold outlive the question they answered: the chip row resets seed/seen/press,
+    // but it cannot reset an answer that already RESOLVED, and `keepPreviousData` chains straight
+    // through the disabled seed-0 query the chip click leaves behind. So the first reroll under a new
+    // budget offered the old budget's card — while it loaded, and permanently once it failed, since
+    // `retry: false` makes a failed pick terminal. Under a chip that promises ten minutes, that makes
+    // the page's one control meaningless (round 2).
+    discover.mockResolvedValue({
+      budget: "any",
+      hero: { tier: "resume", card: ytCard("v1", "Hero v1") },
+      continue_watching: [],
+      comfort: [],
+    });
+    discoverPick.mockResolvedValueOnce({
+      budget: "any",
+      hero: { tier: "surprise", card: ytCard("v9", "Long film") },
+    });
+    mount();
+    await screen.findByText("Hero v1");
+    fireEvent.click(screen.getByText("home.hero.reroll"));
+    await screen.findByText("Long film");
+
+    // A different question — whose own answer is already loaded.
+    discover.mockResolvedValue({
+      budget: "short",
+      hero: { tier: "resume", card: ytCard("v2", "Short thing") },
+      continue_watching: [],
+      comfort: [],
+    });
+    fireEvent.click(screen.getByText("home.budget.short"));
+    await screen.findByText("Short thing");
+
+    // Rejected on OUR schedule, as the sibling test above does, so the assertion cannot land while
+    // the query is still pending — that is a different state.
+    let failPick!: (e: Error) => void;
+    discoverPick.mockReturnValueOnce(
+      new Promise((_resolve, reject) => {
+        failPick = reject;
+      }),
+    );
+    fireEvent.click(screen.getByText("home.hero.reroll"));
+    await waitFor(() => expect(discoverPick).toHaveBeenCalledTimes(2));
+    // In flight: the placeholder must not reach across the budget change.
+    expect(screen.queryByText("Long film")).toBeNull();
+
+    await act(async () => {
+      failPick(new Error("boom"));
+      await new Promise((r) => setTimeout(r, 0));
+    });
+    // Failed: this budget's own hero, never the film picked for "any".
+    expect(screen.getByText("Short thing")).toBeTruthy();
+    expect(screen.queryByText("Long film")).toBeNull();
+  });
+
  it("a mutation's invalidation reaches the rerolled hero too", async () => {
    // The reroll has its own query key so pressing the button does not refetch the rails — but under
    // the SAME root, or the prefix every mutation invalidates misses it and a rerolled hero outlives
@@ -315,6 +369,50 @@ describe("Home", () => {
    await waitFor(() => expect(discoverPick).toHaveBeenCalledTimes(2));
  });

+  it("an unrelated mutation does not put the reroll button into a spinner", async () => {
+    // The other edge of the test above. Because the pick key sits under the `["discover"]` prefix,
+    // every mark-watched, bookmark, pin and shelf move on this page refetches it in the BACKGROUND —
+    // so binding the button's `loading` to raw `isFetching` made it spin and, since `ui/Button` turns
+    // `loading` into `disabled`, swallow clicks after actions the user never associated with a
+    // reroll (round 2). A press is outstanding only while THIS key has no answer of its own.
+    const rerollBtn = () =>
+      screen.getByText("home.hero.reroll").closest("button") as HTMLButtonElement;
+    let resolvePick!: (v: unknown) => void;
+    discoverPick.mockReturnValueOnce(
+      new Promise((resolve) => {
+        resolvePick = resolve;
+      }),
+    );
+    const { qc } = mountWithClient();
+    await screen.findByText("Hero v1");
+    fireEvent.click(screen.getByText("home.hero.reroll"));
+    await waitFor(() => expect(discoverPick).toHaveBeenCalledTimes(1));
+
+    // The positive control, and it is the half most easily broken while fixing the other: this is the
+    // FIRST press, so there is no previous pick for the placeholder to hold, and a predicate resting
+    // on `isPlaceholderData` alone would drop the spinner from exactly the press that needs it.
+    expect(rerollBtn().disabled).toBe(true);
+    expect(rerollBtn().getAttribute("aria-busy")).toBe("true");
+
+    await act(async () => {
+      resolvePick({ budget: "any", hero: { tier: "surprise", card: ytCard("v9", "Rerolled") } });
+      await new Promise((r) => setTimeout(r, 0));
+    });
+    await screen.findByText("Rerolled");
+
+    // Never settles, so the assertion lands squarely inside the refetch window rather than after it —
+    // and therefore the invalidation must NOT be awaited: its promise resolves only once every
+    // refetch it started has, which this one never will.
+    discoverPick.mockReturnValue(new Promise(() => {}));
+    act(() => {
+      void qc.invalidateQueries({ queryKey: qk.discover() });
+    });
+    await waitFor(() => expect(discoverPick).toHaveBeenCalledTimes(2));
+
+    expect(rerollBtn().disabled).toBe(false);
+    expect(rerollBtn().getAttribute("aria-busy")).toBeNull();
+  });
+
  it("refreshes the payload the bookmark is read from", async () => {
    mount();
    await screen.findByText("Second thing");
@@ -145,7 +145,9 @@ function Rail({
                icon: <PinOff className="w-4 h-4" />,
                separatorBefore: true,
                // The only row here that removes a curated entry, and it sat in the same colour as
-                // the four harmless reorders. Every other destructive row in the app is `danger`.
+                // the four harmless reorders. `danger` matches the shared unpin row in
+                // `comfortPin.item()`, which is what every OTHER surface (including the hero and the
+                // rail below) composes for the same command — the two must not disagree.
                danger: true,
                // The pin id travels on the shelf card, so this needs no lookup — and unlike every
                // other surface it can never be in the "not known yet" state.
@@ -378,15 +380,41 @@ export default function Home() {
    // has no data for a beat, `hero` falls to null, and the page renders "nothing to suggest yet" —
    // an EMPTY state standing in for a loading one, which is the exact dishonesty the house rules
    // forbid. Measured in the browser (2026-09-06); the button's own spinner is the loading signal.
-    placeholderData: keepPreviousData,
+    //
+    // But only WITHIN a budget. Plain `keepPreviousData` chains through the disabled seed-0 query a
+    // chip click leaves behind, so the first reroll under a new budget held up the card the PREVIOUS
+    // question was answered with — under a chip promising something else (round 2). Returning
+    // undefined instead falls the hero back to this budget's own loaded answer, which is already in
+    // hand; the spinner still covers the wait.
+    placeholderData: (prev) => (prev?.budget === budget ? prev : undefined),
  });
-  // The last pick that actually RESOLVED. Every press is a new query key, so a failed one has no
-  // data of its own and `keepPreviousData` (pending-only) cannot cover it — without this the hero
-  // silently rewound to the card the page loaded with, several presses back, while `core.req`'s own
-  // dialog was explaining the failure. A reroll that fails leaves the answer on screen alone.
-  const [lastPick, setLastPick] = useState<DiscoverPayload["hero"]>(null);
+  // Is a PRESS outstanding? NOT `pickQ.isFetching`. The pick key deliberately sits under the
+  // `["discover"]` prefix so a mutation's invalidation reaches the rerolled hero — which also means
+  // every mark-watched, bookmark, pin/unpin and shelf move on this page refetches it in the
+  // background, and `ui/Button` turns `loading` into `disabled`. So the reroll spun and swallowed
+  // clicks after actions the user never associated with it (round 2). A press is outstanding exactly
+  // while THIS key has no answer of its own: the placeholder is still standing in for the previous
+  // press, or — the FIRST press, which has no previous data to stand in — there is nothing at all.
+  // `isPlaceholderData` alone would drop the spinner from exactly that first press.
+  const rerolling = pickQ.isFetching && (pickQ.isPlaceholderData || pickQ.data === undefined);
+  // The last pick that actually RESOLVED, tagged with the budget it answered. Every press is a new
+  // query key, so a failed one has no data of its own and `keepPreviousData` (pending-only) cannot
+  // cover it — without this the hero silently rewound to the card the page loaded with, several
+  // presses back, while `core.req`'s own dialog was explaining the failure. A reroll that fails
+  // leaves the answer on screen alone.
+  //
+  // The TAG is load-bearing, not bookkeeping: the chip row resets `seed`/`seen`/`press` but cannot
+  // reset an answer that already resolved, so an untagged slot outlived the question it answered —
+  // switch to "10 perc", press reroll, let that request fail (`retry: false`, so it is terminal),
+  // and the two-hour film picked under "Bármennyi" was still the hero, under a chip promising ten
+  // minutes (round 2). Taken from the SERVER's `budget` echo rather than this render's state, so the
+  // tag cannot be minted from a budget the pick never answered.
+  const [lastPick, setLastPick] = useState<{
+    budget: DiscoverBudget;
+    hero: DiscoverPayload["hero"];
+  } | null>(null);
  useEffect(() => {
-    if (pickQ.data !== undefined) setLastPick(pickQ.data.hero);
+    if (pickQ.data !== undefined) setLastPick({ budget: pickQ.data.budget, hero: pickQ.data.hero });
  }, [pickQ.data]);

  const data = q.data;
@@ -401,7 +429,7 @@ export default function Home() {
      ? (data?.hero ?? null)
      : pickQ.data !== undefined
        ? pickQ.data.hero
-        : (lastPick ?? data?.hero ?? null);
+        : ((lastPick?.budget === budget ? lastPick.hero : null) ?? data?.hero ?? null);
  // The hero is the top of the resume rail by construction, so leaving it in the rail below prints
  // the same card twice, a hand's width apart. Matched on source+id rather than on the tier alone:
  // a rerolled hero can be a surprise pick that happens to be nothing to do with this rail.
@@ -429,7 +457,12 @@ export default function Home() {
        .then(() => qc.invalidateQueries({ queryKey: qk.discover() }))
        // Without this the rejection escapes unhandled and the click just does nothing — the
        // feed's equivalents both catch, and the reorder on this same page already does.
-        .catch((e: unknown) => notifyActionError(e, t("home.error")));
+        // A WRITE key, not `home.error`: that string is the page's LOAD failure ("could not load
+        // your suggestions"), and it is what a detail-less rejection actually shows — an offline
+        // click raises a bare fetch TypeError, so there is no server reason to prefer over it. It
+        // would describe a state the user can see is untrue, over a fully populated page whose
+        // invalidations never even ran (they sit inside `.then`).
+        .catch((e: unknown) => notifyActionError(e, t("home.actionFailed")));
    },
    [qc, t],
  );
@@ -446,7 +479,8 @@ export default function Home() {
          // re-sent the same call — the control looked dead.
          qc.invalidateQueries({ queryKey: qk.discover() });
        })
-        .catch((e: unknown) => notifyActionError(e, t("home.error")));
+        // The write key, for the same reason as `onState` above.
+        .catch((e: unknown) => notifyActionError(e, t("home.actionFailed")));
    },
    [qc, t],
  );
@@ -607,7 +641,7 @@ export default function Home() {
          card={hero.card}
          onPlay={onPlay}
          onReroll={reroll}
-          rerolling={pickQ.isFetching}
+          rerolling={rerolling}
        />
      ) : (
        // Honest: nothing in the library fits what was asked. Offering something that does not fit
@@ -621,7 +655,20 @@ export default function Home() {
          </StateMessage>
          {budget !== "any" && (
            <div className="flex justify-center pb-4">
-              <Button variant="outline" size="xs" type="button" onClick={reroll}>
+              {/* The same action as the Hero's own reroll, so it carries the same in-flight signal.
+                  Without it the press was unacknowledged — `hero` stays null for the whole round
+                  trip in this branch, so nothing on the page moved — and a second click spent
+                  another pick AND skipped a tier in the server's `start=` rotation. The icon comes
+                  along because `loading` swaps it for the spinner: with no icon the button would
+                  grow by a spinner's width mid-press. */}
+              <Button
+                variant="outline"
+                size="xs"
+                type="button"
+                icon={<RotateCcw className="h-4 w-4" />}
+                loading={rerolling}
+                onClick={reroll}
+              >
                {t("home.hero.reroll")}
              </Button>
            </div>
@@ -37,7 +37,10 @@ export interface NavigationState {
 }

 export interface NavigationActions {
-  setPage: (next: Page) => void;
+  /** Switch modules. Returns whether it actually navigated — it is a no-op when the requested page
+   *  is already current with no channel page over it, and a caller that buries overlay history
+   *  entries under the push has to know that (see `lib/history.navigateOverOverlays`). */
+  setPage: (next: Page) => boolean;
  openChannel: (id: string, name?: string) => void;
  closeChannel: () => void;
  enterYtSearch: (q: string) => void;
@@ -50,7 +53,7 @@ const StateContext = createContext<NavigationState>({
  ytSearch: null,
 });
 const ActionsContext = createContext<NavigationActions>({
-  setPage: () => {},
+  setPage: () => false,
  openChannel: () => {},
  closeChannel: () => {},
  enterYtSearch: () => {},
@@ -118,7 +121,7 @@ export function NavigationProvider({ children }: { children: ReactNode }) {
    (next: Page) => {
      // A channel page overlays the content column, so a nav click must close it even when the
      // underlying page is unchanged (e.g. "Feed" while a channel opened from the feed).
-      if (next === pageRef.current && !channelViewRef.current) return;
+      if (next === pageRef.current && !channelViewRef.current) return false;
      markDirty();
      setChannelView(null); // leave any open channel page when navigating via the rail
      setPageState(next);
@@ -147,6 +150,7 @@ export function NavigationProvider({ children }: { children: ReactNode }) {
        }),
      );
      notifyNav();
+      return true;
    },
    [markDirty],
  );
@@ -192,6 +192,12 @@ export function useComfortPin(open: boolean): ComfortPin {
          label: pinId === null ? t("home.pin.add") : t("home.pin.remove"),
          icon: pinId === null ? <Pin className="w-4 h-4" /> : <PinOff className="w-4 h-4" />,
          separatorBefore: true,
+          // One row, two states, and only ONE of them is destructive — so `danger` flips with the
+          // label and the icon rather than being decided at a call site. The shelf's own hand-rolled
+          // unpin row on Home got the flag first; leaving this one neutral painted the identical
+          // command (same label, same icon, same `removeComfortPin`) two different colours on one
+          // screen — the hero's menu and the Continue-watching rail come through here (round 2).
+          danger: pinId !== null,
          onSelect: () => toggle(target, title),
        },
      ];
@@ -2,6 +2,7 @@
  "navLabel": "Home",
  "title": "What should I watch?",
  "error": "Could not load your suggestions.",
+  "actionFailed": "Could not save that.",
  "budget": {
    "ask": "How much time do you have?",
    "short": "10 minutes",
@@ -2,6 +2,7 @@
  "navLabel": "Kezdőlap",
  "title": "Mit nézzek most?",
  "error": "Nem sikerült betölteni az ajánlatokat.",
+  "actionFailed": "Nem sikerült elmenteni.",
  "budget": {
    "ask": "Mennyi időd van?",
    "short": "10 perc",
@@ -0,0 +1,43 @@
+// @vitest-environment jsdom
+//
+// ONE test, ONE file, for the same reason CommandPalette.ordering.dom.test.tsx has its own: this is
+// the only assertion in the suite that lets a reconcile issue a REAL programmatic history.back(),
+// and history.ts's overlay bookkeeping is module-global. history.dom.test.ts states its own
+// invariant — every layer there is closed by a genuine Back, so no programmatic back() runs and the
+// shared state stays clean between its tests — and this test would break it.
+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();
+    }
+  });
+});
@@ -96,7 +96,11 @@ describe("closeThenNavigate", () => {
    const order: string[] = [];
    closeThenNavigate(
      () => order.push("close"),
-      () => order.push("navigate"),
+      // 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"]);

@@ -164,6 +164,21 @@ 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, because both take the same no-op navigation.
+ *
+ *  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;
+}
+
 // Push/pop history entries until their count matches the live overlay count. Runs once per tick.
 function reconcileHistory(): void {
  reconcileScheduled = false;
@@ -221,7 +236,7 @@ function ensureOverlayListener(): void {
 *
 *  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: () => void): void {
+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
@@ -233,7 +248,6 @@ export function closeThenNavigate(close: () => void, navigate: () => void): void
    done = true;
    window.removeEventListener("popstate", onPop);
    clearTimeout(fallback);
-    navigate();
    // 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
@@ -241,8 +255,9 @@ export function closeThenNavigate(close: () => void, navigate: () => void): void
    // 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).
-    buriedEntries = historyDepth;
+    // closed them (round 2). `buryUnder` is what skips the mark when the command navigated nowhere
+    // — a palette entry for the page you are already on.
+    buryUnder(navigate);
  };
  const onPop = () => {
    // The same reconcile can issue MORE than one back(); a pushState made between them would be
@@ -268,10 +283,14 @@ export function closeThenNavigate(close: () => void, navigate: () => void): void
 *  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. */
-export function navigateOverOverlays(navigate: () => void): void {
-  buriedEntries = historyDepth;
-  navigate();
+ *  longer pops the page out from under the user.
+ *
+ *  `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);
 }

 /** While the calling component is mounted, Back (or the browser/mouse back button) closes it via