fix(e24): review round 3 — the holes round 2's own fixes left

Round 3 reviewed the round-2 commit across six dimensions. Fifteen findings, five survived
three adversarial refuters each; three died on 2+ refutations (a keyboard-focus claim that
is a property of `ui/Button` at all six of its `loading` call sites, a contract-test
false-pass that needs a deliberate revert plus a self-incriminating comment, and a
MAX_EXCLUDE coupling whose slice is unreachable from the real client).

- `rerolling` was still wrong for a FAILED press. `data === undefined` is true forever for a
  key whose request failed (`retry: false`), so the next unrelated mutation re-disabled the
  reroll button — the same defect round 2 reported, one branch over. `!isFetched` is the
  flag that remembers a key settled; `!isError` would not have worked, because query-core
  resets a data-less query to "pending" for the duration of a refetch, hiding the error
  exactly when it matters.
- The hero fallback used `??`, which cannot tell "this budget's last pick resolved to NO
  hero" from "nothing remembered". So a press that failed after an empty one resurrected the
  card the page had already excluded as seen and just said it had nothing beyond. The tagged
  slot is now authoritative for its own budget.
- `setPage`'s boolean — the input the whole burial rule reads — was asserted by nothing.
  The type fences the SHAPE, so deleting a `return` fails tsc, but swapping the two values
  passed typecheck and all 822 tests. NavigationProvider gets its first render test, on the
  value AND on the effects the early return skips.
- `closeThenNavigate`'s door into `buryUnder` was equally unpinned: reverting that one call
  site to the old unconditional burial left the suite green. It has the mirror case now.
- The comment claiming one rule serves both entry points was overstated. `buryUnder` fails
  OPEN, and `CommandItem.run` is typed `() => void`, so only the palette's bare
  `() => setPage(page)` rows forward the boolean at all. Nothing regressed — that path buried
  unconditionally before — but the prose said more than the code does, so it now says what is
  true and points at the deferred fix. Same for a "the only assertion in the suite" claim
  that the file it cites as precedent also makes.

Every new test mutation-checked red for the right reason, both edges where there are two.
Gate: ruff, ruff format, 3x tsc, eslint (0 errors), prettier, knip, vitest 828, pytest 1005.

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");

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.

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<typeof import("react-i18next")>();
+  return {
+    ...actual,
+    useTranslation: () => ({ t: (k: string) => k, i18n: { language: "en" } }),
+  };
+});
+
+import { NavigationProvider, useNavigation, useNavigationActions } from "./NavigationProvider";
+
+const wrapper = ({ children }: { children: ReactNode }) => (
+  <NavigationProvider>{children}</NavigationProvider>
+);
+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();
+    }
+  });
+});

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";

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", () => {

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 = () => {