Commit Graph
3 Commits
Author SHA1 Message Date
peter 92c305bab9 fix(nav): a navigation closes every overlay before it pushes
The overlay history stack let a navigation land ABOVE open overlays and
"buried" their entries with a counter. Measured in Chrome: Shift+W over
About navigated under the still-open dialog, the next Back both undid the
navigation and closed About, and the Back after that did nothing; Shift+W
on Home with the player open did nothing at all. The counter could also be
spent by a different overlay closing first, and callers that could not
report whether they pushed were assumed to have.

setPage and openChannel now close every open overlay and push only once
the history is back in sync (one entry per live overlay, no programmatic
back() in flight, no reconcile queued), so nothing is ever buried and the
whole mechanism goes: navigateOverOverlays, closeThenNavigate, buryUnder,
setPage's boolean, and the three test files that pinned them. The palette
closes only itself before running a row, so a theme picked over the player
keeps the player. Shift+W on Home with the player open now closes it
(owner decision 2026-09-09).

The wait is on state, not on "a popstate": the old 250 ms fallback could
run with back()s still in flight. An overlay that refuses to close within
1.5 s drops the command instead of being navigated out from under.
2026-09-13 00:17:09 +02:00
peter d832bb19ce fix(e24): review round 4 — a test that pinned nothing, and prose still ahead of the code
Round 4 reviewed round 3's commit. Nothing in production behaviour was wrong; what was wrong
was the round's own test and its comments, which is the failure class this loop keeps
finding.

- The new NavigationProvider case named after the `&& !channelViewRef.current` clause never
  reached it. With storage cleared and a bare URL the provider lands on "home" (E24's own
  default), not "feed", so asking for "feed" was an ordinary page change and the guard's
  first conjunct short-circuited. Deleting the clause left all 828 tests green — a dead nav
  control (the rail's own module stops closing an open channel page) with no red anywhere.
  The test now asserts the precondition and calls `setPage` with the page that IS current.
- Both new Home cases rejected a promise and then asserted after one fixed macrotask.
  React-query notifies through `notifyManager`'s own `setTimeout(0)`, registered after any
  timer the test schedules, so the assertion raced it; two reviewers measured intermittent
  reds. They poll for a signal that the rejection LANDED instead — the reroll button
  releasing, which is also the honest one for the empty-answer case, where the empty state is
  already on screen while the press is in flight and asserting it early would pass either way.
- `buryUnder`'s comment still opened with "one rule for both entry points" ten lines above the
  paragraph saying the rule is complete only for one of them, listed the non-reporting palette
  rows without `openChannel` — which has its own `replaceState` no-push branch and is reached
  from PlayerModal too — and pointed at a ROADMAP.md this repository does not contain.
- `history.dom.test.ts`'s header claimed its layers close by genuine Backs only, which two of
  its own tests contradict; and `CommandPalette.ordering.dom.test.tsx` still claimed to be
  "the only assertion in the suite" doing what the file corrected in round 3 also does. Round
  3's message said that pair was handled and only one side had been.

The provider test was re-mutation-checked (dropping the clause now goes red), and both
hardened Home tests still go red against their own reverts. Seven consecutive full runs green.
Gate: ruff, ruff format, 3x tsc, eslint (0 errors), prettier, knip, vitest 828, pytest 1005.
2026-09-09 01:02:22 +02:00
peter 59efc8f2e8 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.
2026-09-09 00:39:37 +02:00