diff --git a/frontend/src/components/Home.dom.test.tsx b/frontend/src/components/Home.dom.test.tsx index 73093a2f..26f91987 100644 --- a/frontend/src/components/Home.dom.test.tsx +++ b/frontend/src/components/Home.dom.test.tsx @@ -413,6 +413,78 @@ describe("Home", () => { expect(rerollBtn().getAttribute("aria-busy")).toBeNull(); }); + it("a press that FAILED does not leave the button hostage to the next mutation", async () => { + // Round 3's correction to the case above. A key whose request failed never gets data of its own + // (`retry: false` makes it terminal), so a predicate reading `data === undefined` stayed true + // forever and every later background refetch re-disabled the button — the same defect, one + // branch over. `isFetched` remembers that the key settled, an error counting as settled. + const rerollBtn = () => + screen.getByText("home.hero.reroll").closest("button") as HTMLButtonElement; + let failPick!: (e: Error) => void; + discoverPick.mockReturnValueOnce( + new Promise((_resolve, reject) => { + failPick = reject; + }), + ); + const { qc } = mountWithClient(); + 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 + + // Now an unrelated action refetches that errored key in the background. + 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("keeps the honest empty answer when a press fails after an empty one", async () => { + // The tagged slot is authoritative for its own budget, including when what it holds is "no hero + // at all". Under a `??` chain that read as "nothing remembered" and fell through to the loaded + // payload, so a failed press resurrected the card the page had already excluded as seen and just + // said it had nothing beyond (round 3). + discover.mockResolvedValue({ + budget: "short", + hero: { tier: "resume", card: ytCard("v1", "Hero v1") }, + continue_watching: [], + comfort: [], + }); + mount(); + await screen.findByText("Hero v1"); + fireEvent.click(screen.getByText("home.budget.short")); // the empty-state branch needs a band + await waitFor(() => expect(discover).toHaveBeenLastCalledWith("short", 0)); + + // A press that RESOLVES with nothing left: the honest empty state. + discoverPick.mockResolvedValueOnce({ budget: "short", hero: null }); + fireEvent.click(screen.getByText("home.hero.reroll")); + await screen.findByText("home.hero.emptyBudget"); + expect(screen.queryByText("Hero v1")).toBeNull(); + + // …and a press from there that fails must not undo it. + 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)); + await act(async () => { + failPick(new Error("boom")); + await new Promise((r) => setTimeout(r, 0)); + }); + expect(screen.getByText("home.hero.emptyBudget")).toBeTruthy(); + expect(screen.queryByText("Hero v1")).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 b96c12b1..fa3c17e9 100644 --- a/frontend/src/components/Home.tsx +++ b/frontend/src/components/Home.tsx @@ -393,10 +393,16 @@ export default function Home() { // 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); + // while THIS key has never settled — which is what `isFetched` means (react-query counts both a + // success and an error as settled), and every press mints a fresh seed, so a fresh key. + // + // Round 3 corrected this: `isPlaceholderData || data === undefined` looked equivalent and was not. + // A pick that FAILED leaves its key data-less forever (`retry: false`), so the second disjunct + // stayed true and the next unrelated mutation re-disabled the button — the same defect, surviving + // in the error branch. Guarding on `!isError` does NOT help either: query-core resets a data-less + // query to status "pending" for the duration of a refetch, so the error is invisible exactly when + // it matters. `isFetched` is the only flag that remembers. + const rerolling = pickQ.isFetching && !pickQ.isFetched; // 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 @@ -429,7 +435,15 @@ export default function Home() { ? (data?.hero ?? null) : pickQ.data !== undefined ? pickQ.data.hero - : ((lastPick?.budget === budget ? lastPick.hero : null) ?? data?.hero ?? null); + : // A ternary, not `??`: the tagged slot is AUTHORITATIVE for its own budget, and "the last + // pick for this budget resolved to no hero" is a real answer, not a missing one. Under + // `??` the two collapsed, so a failed press after an empty pick resurrected the card the + // page loaded with — which it had already excluded as seen and just said it had nothing + // beyond (round 3). A mismatched or absent tag still falls through to the loaded hero, + // which is what the FIRST press of a budget needs. + lastPick?.budget === budget + ? lastPick.hero + : (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. diff --git a/frontend/src/components/NavigationProvider.dom.test.tsx b/frontend/src/components/NavigationProvider.dom.test.tsx new file mode 100644 index 00000000..5f2a77ff --- /dev/null +++ b/frontend/src/components/NavigationProvider.dom.test.tsx @@ -0,0 +1,101 @@ +// @vitest-environment jsdom +// +// `setPage`'s no-op contract — the PRODUCER side of the history-burial fix, which nothing rendered +// before this. `setPage` early-returns when the requested page is already current with no channel +// page over it, and reports that by returning `false`; `lib/history.buryUnder` reads the boolean to +// decide whether the overlay entries beneath a navigation are still reclaimable. Get it wrong and +// the round-2 bug is back: Shift+W over the player on Home buries an entry nothing pushed past, and +// the user's next browser Back does nothing. +// +// The type does NOT fence this. `NavigationActions` pins the SHAPE (`=> boolean`), so deleting one +// `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 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`. +vi.mock("react-i18next", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + useTranslation: () => ({ t: (k: string) => k, i18n: { language: "en" } }), + }; +}); + +import { NavigationProvider, useNavigation, useNavigationActions } from "./NavigationProvider"; + +const wrapper = ({ children }: { children: ReactNode }) => ( + {children} +); +const render = () => + renderHook(() => ({ ...useNavigation(), ...useNavigationActions() }), { wrapper }); + +beforeEach(() => { + localStorage.clear(); + sessionStorage.setItem("siftlode.activeAccount", "1"); // account-scoped page memory + window.history.replaceState({ sfPage: "feed" }, "", "/"); +}); + +describe("NavigationProvider · setPage reports whether it navigated", () => { + it("returns true and pushes an entry for a real page change", () => { + const { result } = render(); + const push = vi.spyOn(window.history, "pushState"); + try { + let reported: boolean | undefined; + act(() => { + reported = result.current.setPage("plex"); + }); + expect(reported).toBe(true); + expect(push).toHaveBeenCalledTimes(1); + expect(result.current.page).toBe("plex"); + } finally { + push.mockRestore(); + } + }); + + it("returns false and pushes nothing when it is already that page", () => { + const { result } = render(); + act(() => { + result.current.setPage("plex"); + }); + const push = vi.spyOn(window.history, "pushState"); + try { + let reported: boolean | undefined; + act(() => { + reported = result.current.setPage("plex"); + }); + // Both halves matter: `false` is what `buryUnder` reads, and "pushed nothing" is what makes + // that answer true. A value-only assertion would pass against an inverted pair. + expect(reported).toBe(false); + expect(push).not.toHaveBeenCalled(); + } finally { + push.mockRestore(); + } + }); + + 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. + const { result } = render(); + act(() => { + result.current.openChannel("UC1", "A channel"); + }); + expect(result.current.channelView).not.toBeNull(); + + const push = vi.spyOn(window.history, "pushState"); + try { + let reported: boolean | undefined; + act(() => { + reported = result.current.setPage("feed"); + }); + expect(reported).toBe(true); + expect(push).toHaveBeenCalledTimes(1); + expect(result.current.channelView).toBeNull(); + } finally { + push.mockRestore(); + } + }); +}); diff --git a/frontend/src/lib/history.burial.dom.test.ts b/frontend/src/lib/history.burial.dom.test.ts index b6e739ed..17a4846a 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: 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. +// 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. 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 56309956..02d37d7d 100644 --- a/frontend/src/lib/history.dom.test.ts +++ b/frontend/src/lib/history.dom.test.ts @@ -152,6 +152,37 @@ describe("closeThenNavigate", () => { } }); + // The mirror of the case above, and the OTHER door into `buryUnder` — round 3 found that only + // `navigateOverOverlays` was pinned, so this call site could be reverted to the old unconditional + // burial with the whole suite green. Reachable through the palette: `run: () => setPage(page)` is + // a bare arrow, so `setPage`'s `false` really does arrive here when the chosen module is the one + // already on screen. Nothing was pushed, so the layer underneath is still ours to reclaim. + it("leaves the layer below reclaimable when the command navigated nowhere", async () => { + const back = vi.spyOn(window.history, "back").mockImplementation(() => { + window.dispatchEvent(new PopStateEvent("popstate", { state: window.history.state })); + }); + try { + const player = renderHook(() => useBackToClose(() => {})); + await settle(); + const palette = renderHook(() => useBackToClose(() => {})); + await settle(); + + const navigate = vi.fn(() => false); // setPage's early return: already on that page + await act(async () => { + closeThenNavigate(() => palette.unmount(), navigate); + await Promise.resolve(); + }); + expect(navigate).toHaveBeenCalledTimes(1); + expect(back).toHaveBeenCalledTimes(1); // the palette's own entry + + player.unmount(); + await settle(); + expect(back).toHaveBeenCalledTimes(2); // and the player's, because nothing was pushed over it + } finally { + back.mockRestore(); + } + }); + // Without this the command is not late, it never happens: an overlay closed in the same tick it // opened never pushed an entry, so no reclaim popstate is ever coming. it("navigates anyway when no reclaim popstate ever arrives", () => { diff --git a/frontend/src/lib/history.ts b/frontend/src/lib/history.ts index 2069d69f..267bc151 100644 --- a/frontend/src/lib/history.ts +++ b/frontend/src/lib/history.ts @@ -173,6 +173,16 @@ let listenerInstalled = false; * 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. * + * 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`. + * * 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 { @@ -255,8 +265,9 @@ export function closeThenNavigate(close: () => void, navigate: () => boolean | v // reconcile ate the new page entry). MARK them rather than zeroing the depth: round 1 set // `historyDepth = 0` outright, which also desynced the counter from overlays the navigation does // NOT unmount — About/ReleaseNotes live outside the page column — so one Back both navigated and - // closed them (round 2). `buryUnder` is what skips the mark when the command navigated nowhere - // — a palette entry for the page you are already on. + // closed them (round 2). `buryUnder` skips the mark when the command reports it navigated + // nowhere — which here is only the palette's bare `() => setPage(page)` module rows; see the + // fail-open limit documented on `buryUnder` for the rows that cannot report. buryUnder(navigate); }; const onPop = () => {