14b5fe2e312b8c5804603dcd82ef4063d19c6271
3
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
|
|
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. |
||
|
|
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. |
||
|
|
e4ffd59ca7 |
fix(e24): review round 2 — a buried history entry, a stale hero, and a spinner that was not ours
Nine of the round's ten surviving findings. The tenth (a failed shelf reorder leaving an order the server rejected on screen) is on ROADMAP.md: its obvious remedy reintroduces the overlapping-move defect the code already documents, so it wants a design, not a review edit. - `navigateOverOverlays` buried the live overlay entries before knowing whether the navigation pushed one. `setPage` no-ops when the page is already current and Home is the default landing page, so Shift+W over a player ON Home marked its entry buried under a push that never happened: the unmount consumed the mark instead of reclaiming the entry, and the next browser Back was a press that did nothing. `setPage` now reports whether it navigated, and one `buryUnder` rule serves both entry points — `closeThenNavigate` takes the same no-op navigation from a palette command for the page you are already on. - The hero could answer a new time budget with the previous one's pick. The chip row resets seed/seen/press but cannot reset an answer that already resolved, and `keepPreviousData` chained through the disabled seed-0 query — so a reroll under "10 perc" showed the film picked for "Bármennyi" while it loaded, and kept it if the request failed. `lastPick` now carries the server's own budget echo, and the placeholder is scoped to the budget. - The reroll button spun and swallowed clicks after mark-watched, bookmark, pin and shelf moves: the pick key sits under the invalidated `discover` prefix on purpose, so raw `isFetching` was true for background refetches. It now means "a press of mine is outstanding" — which still covers the first press, where there is no placeholder. - Failed writes on Home toasted the page's LOAD-failure string over a fully loaded page; they get their own key (HU+EN). The empty-state reroll had no `loading`, so the press was unacknowledged and a second click skipped a tier. `danger` moved into the shared unpin row so the same command is not red on the shelf and neutral in the rail below it. - Two comments claimed things the code does not do: the `exclude` cap guards no "unbounded scan" (it never reaches SQL), and the route test that named the cap sends three keys. Tests: the burial rule gets its own file (it is the one assertion that lets a reconcile issue a real programmatic back()); App's keymap wiring gets a source contract, because dropping it left all 818 tests green; `add_pin`'s IntegrityError arm was reached by nothing. Every new test mutation-checked red, both edges where the predicate has two. Gate: ruff, ruff format, 3x tsc, eslint (0 errors), prettier, knip, vitest 822, pytest 1005. |