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.
This commit is contained in:
@@ -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";
|
||||
|
||||
@@ -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";
|
||||
|
||||
+16
-10
@@ -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`. */
|
||||
|
||||
Reference in New Issue
Block a user