Verdict: NOT mergeable as-is — 2 blocker-class defects + 6 substantials + nits. Nothing architectural; all fixable on the branch.
Blocker 1 — the filler feature is simultaneously wrong and dead (3 findings, one root)
The client never sends showFiller=true, and GetFuturePlayoutItemsByIdHandler strips filler server-side by default → the PR's Filler badge/timeline styling can never trigger on real filler.
Meanwhile the backend mapper always emits fillerKind (real items get "None", truthy) → every real media item renders the "Filler" badge in production. Tests pass only because the fixture invents fillerKind: null and 'PreRoll' — values the actually-called endpoint never returns (the #109 fixture-fidelity class again).
The prototype's "Show filler" Switch on the Upcoming card was silently dropped — which is exactly the control that should drive showFiller. Fix as one unit: treat 'None'/null as non-filler; add the prototype's Show filler toggle wired to showFiller=true refetch; fix fixtures to real values; test both toggle states.
Blocker 2 — undefined design token
shell.css: .ctv-playout-now uses var(--ctv-surface-1) — no such token exists (only --ctv-surface, -2, -3) and no fallback → the now-playing hero background silently breaks in all three themes.
SUBSTANTIAL
setActivePlayout triggers the detail/items fetch inside a setState updater — StrictMode double-invokes updaters → duplicate GETs per selection click; updaters must be pure.
No responsive collapse: none of the new playout grids were added to the existing @media (max-width: 980px) block (every other grid in the file is) → fixed 260px rail forces page-level horizontal scroll on narrow viewports.
Prototype's Reset / Schedule-reset actions on the Playout card silently dropped (no per-playout reset endpoint exists, so these should be visible disabled deferrals with a note, per the deferral rule).
The reset-all failure test exercises a scenario the backend cannot produce (controller unconditionally returns 202; no error path) AND the mock ignores its own fixture's status (hardcodes 422) — false confidence. Reframe as a transport-failure test and fix the mock to honor the fixture status.
The "exactly once on mount" test asserts fetchCount('/api/playouts') === 2 — the second call is the Dashboard's pre-existing prefetch; the test doesn't verify route-level isolation. Assert the delta across navigation instead.
Test gaps: selector-rail search/filter completely untested; no paging coverage (client sends no pageNum/pageSize, backend default 100 — needs at least an honest-count/non-crash test when totalCount > page length).
NITS
--text-lg fallback 18px vs real 16px; --font-mono fallback doesn't match the token; mount effect re-implements load() instead of calling it; channel-state poll landing during a selection fetch is overwritten by the captured snapshot (stale ≤30s); no items-loading flag on selection switch (screen flashes empty states — #86 added a spinner for exactly this); now-item matched by title instead of a start≤now<finish window; rail buttons not disabled while reset-all mutates; shared fixture gives list rows fields that don't exist on PlayoutListItemResponseModel.
Verified clean
Polling discipline (state only, 30s; items/list/warnings never polled); stale-selection guard on rapid switching; reset-all confirm + ref guard + scoped refetch; paged shapes (page ?? []); NaN/zero-span timeline math; TZ-safe date parsing; hook ordering + unmount guards; all endpoint paths/methods/shapes vs backend exact (warnings/count bare int confirmed); scheduleKind/playoutMode/buildStatus fixture values exact; no phantom component props; no new deps; no player element; scope = the 5 declared files; a11y (keyboard-operable rail, aria-current, non-color-only warnings badge).
## Fable review — PR #127 (head 1840ef83), read-only, 3-lens subagent review
**Verdict: NOT mergeable as-is — 2 blocker-class defects + 6 substantials + nits. Nothing architectural; all fixable on the branch.**
### Blocker 1 — the filler feature is simultaneously wrong and dead (3 findings, one root)
- The client never sends `showFiller=true`, and `GetFuturePlayoutItemsByIdHandler` strips filler server-side by default → the PR's Filler badge/timeline styling can never trigger on real filler.
- Meanwhile the backend mapper always emits `fillerKind` (real items get `"None"`, truthy) → **every real media item renders the "Filler" badge in production**. Tests pass only because the fixture invents `fillerKind: null` and `'PreRoll'` — values the actually-called endpoint never returns (the #109 fixture-fidelity class again).
- The prototype's "Show filler" Switch on the Upcoming card was silently dropped — which is exactly the control that should drive `showFiller`.
**Fix as one unit**: treat `'None'`/null as non-filler; add the prototype's Show filler toggle wired to `showFiller=true` refetch; fix fixtures to real values; test both toggle states.
### Blocker 2 — undefined design token
`shell.css`: `.ctv-playout-now` uses `var(--ctv-surface-1)` — no such token exists (only `--ctv-surface`, `-2`, `-3`) and no fallback → the now-playing hero background silently breaks in all three themes.
### SUBSTANTIAL
1. `setActivePlayout` triggers the detail/items fetch **inside a setState updater** — StrictMode double-invokes updaters → duplicate GETs per selection click; updaters must be pure.
2. No responsive collapse: none of the new playout grids were added to the existing `@media (max-width: 980px)` block (every other grid in the file is) → fixed 260px rail forces page-level horizontal scroll on narrow viewports.
3. Prototype's Reset / Schedule-reset actions on the Playout card silently dropped (no per-playout reset endpoint exists, so these should be visible disabled deferrals with a note, per the deferral rule).
4. The reset-all failure test exercises a scenario the backend cannot produce (controller unconditionally returns 202; no error path) AND the mock ignores its own fixture's status (hardcodes 422) — false confidence. Reframe as a transport-failure test and fix the mock to honor the fixture status.
5. The "exactly once on mount" test asserts `fetchCount('/api/playouts') === 2` — the second call is the Dashboard's pre-existing prefetch; the test doesn't verify route-level isolation. Assert the delta across navigation instead.
6. Test gaps: selector-rail search/filter completely untested; no paging coverage (client sends no pageNum/pageSize, backend default 100 — needs at least an honest-count/non-crash test when totalCount > page length).
### NITS
`--text-lg` fallback 18px vs real 16px; `--font-mono` fallback doesn't match the token; mount effect re-implements `load()` instead of calling it; channel-state poll landing during a selection fetch is overwritten by the captured snapshot (stale ≤30s); no items-loading flag on selection switch (screen flashes empty states — #86 added a spinner for exactly this); now-item matched by title instead of a start≤now<finish window; rail buttons not disabled while reset-all mutates; shared fixture gives list rows fields that don't exist on `PlayoutListItemResponseModel`.
### Verified clean
Polling discipline (state only, 30s; items/list/warnings never polled); stale-selection guard on rapid switching; reset-all confirm + ref guard + scoped refetch; paged shapes (`page ?? []`); NaN/zero-span timeline math; TZ-safe date parsing; hook ordering + unmount guards; all endpoint paths/methods/shapes vs backend exact (`warnings/count` bare int confirmed); scheduleKind/playoutMode/buildStatus fixture values exact; no phantom component props; no new deps; no player element; scope = the 5 declared files; a11y (keyboard-operable rail, aria-current, non-color-only warnings badge).
- Filler unit: treat fillerKind 'None'/null as non-filler (real items no
longer render the Filler badge); add the prototype's Show filler toggle
wired to ?showFiller=true (the client previously never requested filler,
making the filler UI dead code); fixtures now use real wire values
- Hoist selection fetch out of the setState updater (StrictMode-safe)
via a baseRef snapshot; merge polled channel states functionally
instead of clobbering with the captured snapshot
- Fix undefined --ctv-surface-1 token on the hero (→ --surface-card per
prototype); correct --text-lg fallback; collapse the new playout grids
in the 980px media block (no page-level horizontal scroll)
- Visible deferrals: disabled Reset / Schedule reset buttons with note
(no per-playout reset endpoint yet)
- itemsLoading spinners on selection switch; time-window now-matching
with title fallback; rail disabled while reset-all mutates
- Tests: mount-count delta isolation from the Dashboard prefetch;
reset-all reframed as transport-failure (mock honors fixture status;
backend is unconditionally 202); rail filter test; paging honesty
('shown of total' label); listPlayout() fixture matches the list DTO
(63 tests)
Review: PR #127 findings
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
timothy
merged commit a8dd88ef73 into main2026-07-05 17:50:57 +02:00
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Summary
Implements the ChicoryTV Playouts now/next monitor for #87 as a frontend-only screen.
Prototype to API mapping
GET /api/playoutspaged responseGET /api/channels/stateGET /api/playouts/{id}/itemspaged responseGET /api/playouts/{id}plus list row dataGET /api/playouts/warnings/countPOST /api/playouts/reset-allVerification
cd web && npm cinpm run lintpassednpm run typecheckpassednpm test -- --runpassed: 60 testsnpm run buildpassedTZ=UTC dotnet build ErsatzTV.slnpassed with existing NU1903/nullable-test warningsTZ=UTC dotnet test ErsatzTV.Tests/ErsatzTV.Tests.csprojpassed: 368 testsTZ=UTC dotnet test ErsatzTV.Core.Tests/ErsatzTV.Core.Tests.csprojpassed: 493 passed, 1 skippedDeferred
Closes #87
Fable review — PR #127 (head
1840ef83), read-only, 3-lens subagent reviewVerdict: NOT mergeable as-is — 2 blocker-class defects + 6 substantials + nits. Nothing architectural; all fixable on the branch.
Blocker 1 — the filler feature is simultaneously wrong and dead (3 findings, one root)
showFiller=true, andGetFuturePlayoutItemsByIdHandlerstrips filler server-side by default → the PR's Filler badge/timeline styling can never trigger on real filler.fillerKind(real items get"None", truthy) → every real media item renders the "Filler" badge in production. Tests pass only because the fixture inventsfillerKind: nulland'PreRoll'— values the actually-called endpoint never returns (the #109 fixture-fidelity class again).showFiller.Fix as one unit: treat
'None'/null as non-filler; add the prototype's Show filler toggle wired toshowFiller=truerefetch; fix fixtures to real values; test both toggle states.Blocker 2 — undefined design token
shell.css:.ctv-playout-nowusesvar(--ctv-surface-1)— no such token exists (only--ctv-surface,-2,-3) and no fallback → the now-playing hero background silently breaks in all three themes.SUBSTANTIAL
setActivePlayouttriggers the detail/items fetch inside a setState updater — StrictMode double-invokes updaters → duplicate GETs per selection click; updaters must be pure.@media (max-width: 980px)block (every other grid in the file is) → fixed 260px rail forces page-level horizontal scroll on narrow viewports.fetchCount('/api/playouts') === 2— the second call is the Dashboard's pre-existing prefetch; the test doesn't verify route-level isolation. Assert the delta across navigation instead.NITS
--text-lgfallback 18px vs real 16px;--font-monofallback doesn't match the token; mount effect re-implementsload()instead of calling it; channel-state poll landing during a selection fetch is overwritten by the captured snapshot (stale ≤30s); no items-loading flag on selection switch (screen flashes empty states — #86 added a spinner for exactly this); now-item matched by title instead of a start≤now<finish window; rail buttons not disabled while reset-all mutates; shared fixture gives list rows fields that don't exist onPlayoutListItemResponseModel.Verified clean
Polling discipline (state only, 30s; items/list/warnings never polled); stale-selection guard on rapid switching; reset-all confirm + ref guard + scoped refetch; paged shapes (
page ?? []); NaN/zero-span timeline math; TZ-safe date parsing; hook ordering + unmount guards; all endpoint paths/methods/shapes vs backend exact (warnings/countbare int confirmed); scheduleKind/playoutMode/buildStatus fixture values exact; no phantom component props; no new deps; no player element; scope = the 5 declared files; a11y (keyboard-operable rail, aria-current, non-color-only warnings badge).- Filler unit: treat fillerKind 'None'/null as non-filler (real items no longer render the Filler badge); add the prototype's Show filler toggle wired to ?showFiller=true (the client previously never requested filler, making the filler UI dead code); fixtures now use real wire values - Hoist selection fetch out of the setState updater (StrictMode-safe) via a baseRef snapshot; merge polled channel states functionally instead of clobbering with the captured snapshot - Fix undefined --ctv-surface-1 token on the hero (→ --surface-card per prototype); correct --text-lg fallback; collapse the new playout grids in the 980px media block (no page-level horizontal scroll) - Visible deferrals: disabled Reset / Schedule reset buttons with note (no per-playout reset endpoint yet) - itemsLoading spinners on selection switch; time-window now-matching with title fallback; rail disabled while reset-all mutates - Tests: mount-count delta isolation from the Dashboard prefetch; reset-all reframed as transport-failure (mock honors fixture status; backend is unconditionally 202); rail filter test; paging honesty ('shown of total' label); listPlayout() fixture matches the list DTO (63 tests) Review: PR #127 findings Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>