From e4ffd59ca78f48fa050ea5a07caf38246b67b511 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Wed, 9 Sep 2026 00:12:53 +0200 Subject: [PATCH] =?UTF-8?q?fix(e24):=20review=20round=202=20=E2=80=94=20a?= =?UTF-8?q?=20buried=20history=20entry,=20a=20stale=20hero,=20and=20a=20sp?= =?UTF-8?q?inner=20that=20was=20not=20ours?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- backend/app/routes/discover.py | 9 +- backend/tests/test_discover.py | 30 +++++- frontend/src/App.keymap.contract.test.ts | 33 +++++++ frontend/src/App.tsx | 4 +- frontend/src/components/Home.dom.test.tsx | 98 +++++++++++++++++++ frontend/src/components/Home.tsx | 73 +++++++++++--- .../src/components/NavigationProvider.tsx | 10 +- frontend/src/components/comfortPin.tsx | 6 ++ frontend/src/i18n/locales/en/home.json | 1 + frontend/src/i18n/locales/hu/home.json | 1 + frontend/src/lib/history.burial.dom.test.ts | 43 ++++++++ frontend/src/lib/history.dom.test.ts | 6 +- frontend/src/lib/history.ts | 35 +++++-- 13 files changed, 317 insertions(+), 32 deletions(-) create mode 100644 frontend/src/App.keymap.contract.test.ts create mode 100644 frontend/src/lib/history.burial.dom.test.ts 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