diff --git a/backend/app/routes/discover.py b/backend/app/routes/discover.py
index 3ff5465d..f029f0e2 100644
--- a/backend/app/routes/discover.py
+++ b/backend/app/routes/discover.py
@@ -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
diff --git a/backend/tests/test_discover.py b/backend/tests/test_discover.py
index a2ecf648..261001b7 100644
--- a/backend/tests/test_discover.py
+++ b/backend/tests/test_discover.py
@@ -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)
diff --git a/frontend/src/App.keymap.contract.test.ts b/frontend/src/App.keymap.contract.test.ts
new file mode 100644
index 00000000..ee26db20
--- /dev/null
+++ b/frontend/src/App.keymap.contract.test.ts
@@ -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")');
+ });
+});
diff --git a/frontend/src/App.tsx b/frontend/src/App.tsx
index 94103800..8d9de728 100644
--- a/frontend/src/App.tsx
+++ b/frontend/src/App.tsx
@@ -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
diff --git a/frontend/src/components/Home.dom.test.tsx b/frontend/src/components/Home.dom.test.tsx
index 9490feb2..73093a2f 100644
--- a/frontend/src/components/Home.dom.test.tsx
+++ b/frontend/src/components/Home.dom.test.tsx
@@ -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");
diff --git a/frontend/src/components/Home.tsx b/frontend/src/components/Home.tsx
index 2958bcdc..b96c12b1 100644
--- a/frontend/src/components/Home.tsx
+++ b/frontend/src/components/Home.tsx
@@ -145,7 +145,9 @@ function Rail({
icon: ,
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(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() {
{budget !== "any" && (
-
diff --git a/frontend/src/components/NavigationProvider.tsx b/frontend/src/components/NavigationProvider.tsx
index 982841a2..2186076f 100644
--- a/frontend/src/components/NavigationProvider.tsx
+++ b/frontend/src/components/NavigationProvider.tsx
@@ -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({
ytSearch: null,
});
const ActionsContext = createContext({
- 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],
);
diff --git a/frontend/src/components/comfortPin.tsx b/frontend/src/components/comfortPin.tsx
index aac38117..0ae04392 100644
--- a/frontend/src/components/comfortPin.tsx
+++ b/frontend/src/components/comfortPin.tsx
@@ -192,6 +192,12 @@ export function useComfortPin(open: boolean): ComfortPin {
label: pinId === null ? t("home.pin.add") : t("home.pin.remove"),
icon: pinId === null ? : ,
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),
},
];
diff --git a/frontend/src/i18n/locales/en/home.json b/frontend/src/i18n/locales/en/home.json
index 7618c452..87edb861 100644
--- a/frontend/src/i18n/locales/en/home.json
+++ b/frontend/src/i18n/locales/en/home.json
@@ -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",
diff --git a/frontend/src/i18n/locales/hu/home.json b/frontend/src/i18n/locales/hu/home.json
index 2a263838..3136e620 100644
--- a/frontend/src/i18n/locales/hu/home.json
+++ b/frontend/src/i18n/locales/hu/home.json
@@ -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",
diff --git a/frontend/src/lib/history.burial.dom.test.ts b/frontend/src/lib/history.burial.dom.test.ts
new file mode 100644
index 00000000..b6e739ed
--- /dev/null
+++ b/frontend/src/lib/history.burial.dom.test.ts
@@ -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();
+ }
+ });
+});
diff --git a/frontend/src/lib/history.dom.test.ts b/frontend/src/lib/history.dom.test.ts
index cced69c0..56309956 100644
--- a/frontend/src/lib/history.dom.test.ts
+++ b/frontend/src/lib/history.dom.test.ts
@@ -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"]);
diff --git a/frontend/src/lib/history.ts b/frontend/src/lib/history.ts
index 692b6e2c..2069d69f 100644
--- a/frontend/src/lib/history.ts
+++ b/frontend/src/lib/history.ts
@@ -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