diff --git a/frontend/src/components/Home.dom.test.tsx b/frontend/src/components/Home.dom.test.tsx index 26f91987..c19f8d3e 100644 --- a/frontend/src/components/Home.dom.test.tsx +++ b/frontend/src/components/Home.dom.test.tsx @@ -430,11 +430,11 @@ describe("Home", () => { await screen.findByText("Hero v1"); fireEvent.click(screen.getByText("home.hero.reroll")); await waitFor(() => expect(discoverPick).toHaveBeenCalledTimes(1)); - await act(async () => { - failPick(new Error("boom")); - await new Promise((r) => setTimeout(r, 0)); - }); - expect(rerollBtn().disabled).toBe(false); // the failure itself releases the button + // `waitFor`, not a fixed tick: react-query hands the rejection to the observer through + // `notifyManager`'s own `setTimeout(0)`, registered AFTER any timer this test schedules, so + // "await one macrotask then assert" races it. Round 4 measured that race as an intermittent red. + failPick(new Error("boom")); + await waitFor(() => expect(rerollBtn().disabled).toBe(false)); // the failure releases the button // Now an unrelated action refetches that errored key in the background. discoverPick.mockReturnValue(new Promise(() => {})); @@ -475,12 +475,18 @@ describe("Home", () => { failPick = reject; }), ); + const rerollBtn = () => + screen.getByText("home.hero.reroll").closest("button") as HTMLButtonElement; fireEvent.click(screen.getByText("home.hero.reroll")); await waitFor(() => expect(discoverPick).toHaveBeenCalledTimes(2)); - await act(async () => { - failPick(new Error("boom")); - await new Promise((r) => setTimeout(r, 0)); - }); + expect(rerollBtn().disabled).toBe(true); // the press is outstanding + + failPick(new Error("boom")); + // Wait for a signal that the REJECTION has landed, not for a fixed tick: react-query notifies + // the observer through its own `setTimeout(0)`, and the empty state is already on screen while + // the press is in flight — so asserting it straight away would pass with or without the fix. + // The button releasing is that signal (`isFetched` flips once the key settles, error included). + await waitFor(() => expect(rerollBtn().disabled).toBe(false)); expect(screen.getByText("home.hero.emptyBudget")).toBeTruthy(); expect(screen.queryByText("Hero v1")).toBeNull(); }); diff --git a/frontend/src/components/NavigationProvider.dom.test.tsx b/frontend/src/components/NavigationProvider.dom.test.tsx index 5f2a77ff..dc6e38a8 100644 --- a/frontend/src/components/NavigationProvider.dom.test.tsx +++ b/frontend/src/components/NavigationProvider.dom.test.tsx @@ -11,12 +11,13 @@ // `return` fails tsc — but round 3 measured that swapping the two values (early return `true`, tail // `false`) passes typecheck and all 822 tests. Hence assertions on the value AND on the effects the // early return skips, which is what the boolean actually stands for. -import { describe, it, expect, beforeEach, vi } from "vitest"; -import { renderHook, act } from "@testing-library/react"; +import { describe, it, expect, afterEach, beforeEach, vi } from "vitest"; +import { renderHook, act, cleanup } from "@testing-library/react"; import type { ReactNode } from "react"; -// Partial: the provider only wants `t` for the document title, but the i18n module this pulls in -// initialises against the real `initReactI18next`. +// Partial: the provider reads `t` AND `i18n.language` (both feed the document-title effect), and the +// import graph pulls in `src/i18n`, which initialises against the real `initReactI18next` — so a +// wholesale mock throws before any test runs. vi.mock("react-i18next", async (importOriginal) => { const actual = await importOriginal(); return { @@ -38,6 +39,9 @@ beforeEach(() => { sessionStorage.setItem("siftlode.activeAccount", "1"); // account-scoped page memory window.history.replaceState({ sfPage: "feed" }, "", "/"); }); +// Not automatic: vitest runs without `globals`, so RTL never registers its own afterEach — leaving a +// provider (and its popstate listener) mounted for the next case. Same reason Home.dom.test.tsx does. +afterEach(cleanup); describe("NavigationProvider · setPage reports whether it navigated", () => { it("returns true and pushes an entry for a real page change", () => { @@ -77,9 +81,15 @@ describe("NavigationProvider · setPage reports whether it navigated", () => { }); it("returns true even for the current page when a channel page is over it", () => { - // The nav rail's "Feed" while a channel opened from the feed is a real navigation: it closes the - // channel page. The `&& !channelViewRef.current` half of the guard is what makes it one. + // The nav rail's own module while a channel page is open over it is a real navigation: it closes + // the channel page. The `&& !channelViewRef.current` half of the guard is what makes it one. const { result } = render(); + // The page under test must be the one that is ALREADY current, or the guard's first conjunct is + // false and this never reaches the clause it is named for — the mistake round 4 caught here: a + // bare URL with cleared storage lands on "home" (navigationInit's E24 default), not "feed", so + // asking for "feed" was just an ordinary page change and deleting the clause left it green. + const current = result.current.page; + expect(current).toBe("home"); act(() => { result.current.openChannel("UC1", "A channel"); }); @@ -89,7 +99,7 @@ describe("NavigationProvider · setPage reports whether it navigated", () => { try { let reported: boolean | undefined; act(() => { - reported = result.current.setPage("feed"); + reported = result.current.setPage(current); }); expect(reported).toBe(true); expect(push).toHaveBeenCalledTimes(1); diff --git a/frontend/src/components/ui/CommandPalette.ordering.dom.test.tsx b/frontend/src/components/ui/CommandPalette.ordering.dom.test.tsx index 6313e4b3..81cf801e 100644 --- a/frontend/src/components/ui/CommandPalette.ordering.dom.test.tsx +++ b/frontend/src/components/ui/CommandPalette.ordering.dom.test.tsx @@ -1,8 +1,10 @@ // @vitest-environment jsdom // // ONE test, ONE file, on purpose. This is the close→reclaim→run ordering contract of the palette -// (see closeThenNavigate), and it is the only assertion in the suite that drives a real -// useBackToClose entry through a real programmatic history.back(). +// (see closeThenNavigate), and it ASSERTS on a real useBackToClose entry driven through a real +// programmatic history.back() — which is why it cannot share module state with anything. (It is not +// the only place in the suite where an unstubbed back() fires; history.burial.dom.test.ts is the +// other file that asserts on one, and any test rendering a real Modal can let one go.) // // It cannot live in CommandPalette.dom.test.tsx. jsdom gives a test FILE a single session history, // and history.ts's overlay bookkeeping is module-global, so the sibling tests there leave an diff --git a/frontend/src/lib/history.burial.dom.test.ts b/frontend/src/lib/history.burial.dom.test.ts index 17a4846a..8b951b0c 100644 --- a/frontend/src/lib/history.burial.dom.test.ts +++ b/frontend/src/lib/history.burial.dom.test.ts @@ -1,10 +1,10 @@ // @vitest-environment jsdom // -// ONE test, ONE file, for the same reason CommandPalette.ordering.dom.test.tsx has its own — it is -// the other assertion in the suite that lets a reconcile issue a REAL programmatic history.back() -// (unstubbed), 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 or a stubbed back(), so nothing real -// fires and the shared state stays clean between its tests — and this test would break it. +// ONE test, ONE file, for the same reason CommandPalette.ordering.dom.test.tsx has its own: it +// ASSERTS on a reconcile issuing a REAL, unstubbed programmatic history.back(), and history.ts's +// overlay bookkeeping is module-global. history.dom.test.ts keeps its shared state clean by never +// leaving a real back() in flight — its layers close by a genuine Back, a stubbed back(), or a +// consumed burial mark — and this test would break that. import { describe, it, expect, vi } from "vitest"; import { renderHook, act } from "@testing-library/react"; import { navigateOverOverlays, useBackToClose } from "./history"; diff --git a/frontend/src/lib/history.dom.test.ts b/frontend/src/lib/history.dom.test.ts index 02d37d7d..4cf0c926 100644 --- a/frontend/src/lib/history.dom.test.ts +++ b/frontend/src/lib/history.dom.test.ts @@ -7,8 +7,12 @@ // A GENUINE user Back is simulated by dispatching `popstate` directly: the real-Back branch in // history.ts does NOT call history.back() (it only decrements its own historyDepth to match the // entry the browser already dropped), so a raw popstate reproduces it deterministically without -// depending on jsdom's async back() semantics. Each test closes its layers via genuine Backs, so -// no reconcile-driven history.back() runs and the shared module state stays clean between tests. +// depending on jsdom's async back() semantics. The invariant that keeps the module-global +// bookkeeping clean between tests here is NOT "only genuine Backs" — two closeThenNavigate cases +// deliberately drive a reconcile into history.back() — it is that no REAL back() is ever left in +// flight: a layer is closed by a genuine Back, by a STUBBED back() that lands its own popstate, or +// by an unmount whose burial mark is consumed. A test that needs an unstubbed one belongs in its own +// file (see history.burial.dom.test.ts). import { describe, it, expect, vi, afterEach } from "vitest"; import { renderHook, act } from "@testing-library/react"; import { closeThenNavigate, navigateOverOverlays, useBackToClose, useBackToExit } from "./history"; diff --git a/frontend/src/lib/history.ts b/frontend/src/lib/history.ts index 267bc151..f9b03510 100644 --- a/frontend/src/lib/history.ts +++ b/frontend/src/lib/history.ts @@ -171,17 +171,23 @@ let listenerInstalled = false; * 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. + * (round 2). One rule for both entry points below — but read the next paragraph before trusting it + * at the `closeThenNavigate` one, where most callers cannot report at all. * - * It fails OPEN, and that is a deliberate limit rather than an oversight: only a literal `false` - * skips the mark, so a `navigate` that reports nothing is assumed to have pushed — which is what - * the code did unconditionally before, so nothing regressed by adding this. The gap it leaves is - * `closeThenNavigate`'s palette callers: `CommandItem.run` is typed `() => void`, so only the rows - * written as a bare `() => setPage(page)` forward the boolean at all, while the block-bodied and - * non-navigating rows (surprise, playlists, theme, the cheat sheet) still mark a live sibling - * overlay's entry buried under a push that never happened. Closing that overlay by its own control - * then costs one dead browser-Back press. Deferred, with the fix (widen `CommandItem.run` and have - * each row report) on ROADMAP.md — this rule is complete only for `navigateOverOverlays`. + * It fails OPEN: only a literal `false` skips the mark, so a `navigate` that reports nothing is + * assumed to have pushed. That is what the code did unconditionally before, so nothing regressed by + * adding the rule — it just does not yet reach everywhere. Today only `navigateOverOverlays` + * (App's Shift+W → `setPage`) is fully covered. Of `closeThenNavigate`'s callers, only a palette + * row written as a bare `() => setPage(page)` forwards the boolean: `CommandItem.run` is typed + * `() => void`, so the block-bodied and non-navigating rows report nothing — and neither does + * `openChannel`, which has its OWN no-push branch (it `replaceState`s when the entry already + * carries `_chan`) and is reached both from a palette row and from PlayerModal's channel-name jump. + * Each of those still marks a live sibling overlay's entry buried under a push that never happened, + * costing one dead browser-Back press when that overlay is closed by its own control. + * + * Closing it is three changes, not one — widen `CommandItem.run` to `() => boolean | void`, return + * the verdict from the block-bodied rows, and give `openChannel` the boolean `setPage` now has — so + * it is deferred to the project backlog rather than bolted on here. * * Marking AFTER the call is safe: `navigate()` is synchronous and only `reconcileHistory` (a * microtask) and the popstate listener ever move `historyDepth`. */