fix(72): name the filter for the one fault it can prove (review)
Build ErsatzTV Image / Docs update reminder (pull_request) Successful in 13s
Build ErsatzTV Image / decisions.md append-only (pull_request) Successful in 15s
Build ErsatzTV Image / Build & test (.NET) (pull_request) Successful in 9m36s
Build ErsatzTV Image / EF migration integrity (SQLite + MySql) (pull_request) Successful in 11m7s
Build ErsatzTV Image / Build & push image (amd64) (pull_request) Has been skipped
Build ErsatzTV Image / API docs in sync (OpenAPI + endpoint index) (pull_request) Failing after 15m13s
Build ErsatzTV Image / Functional E2E (curl contracts) (pull_request) Failing after 20m13s
Build ErsatzTV Image / Formatting (changed .cs conform to .editorconfig) (pull_request) Has been cancelled
Build ErsatzTV Image / Docs update reminder (pull_request) Successful in 13s
Build ErsatzTV Image / decisions.md append-only (pull_request) Successful in 15s
Build ErsatzTV Image / Build & test (.NET) (pull_request) Successful in 9m36s
Build ErsatzTV Image / EF migration integrity (SQLite + MySql) (pull_request) Successful in 11m7s
Build ErsatzTV Image / Build & push image (amd64) (pull_request) Has been skipped
Build ErsatzTV Image / API docs in sync (OpenAPI + endpoint index) (pull_request) Failing after 15m13s
Build ErsatzTV Image / Functional E2E (curl contracts) (pull_request) Failing after 20m13s
Build ErsatzTV Image / Formatting (changed .cs conform to .editorconfig) (pull_request) Has been cancelled
Adversarial review, Medium: the "Problems" filter over-promised. The badge is
honestly named ("No playout"), but the filter claimed a taxonomy that does not
exist — decisions.md enumerates three fault classes this deliberately does NOT
compute (empty schedule behind a playout, broken source, origin). A user whose
Classic schedule is empty would read "Problems 0" as "lineup healthy" and ship
a dead channel. A false all-clear is worse than no affordance.
Renamed the filter to "No playout" so badge and filter both name exactly the
one fault the API can prove, leaving "Problems" free for when the taxonomy
behind it actually exists. Rationale recorded at the predicate so the next
person doesn't "improve" the label back.
Also from the review:
- The zero-playout test's comment claimed include coverage it does not provide
(it passes with or without the include — 0 == 0). Re-stated as what it is: a
mapper boundary check. Its two siblings are the include coverage.
- UpdateChannelHandler:179 is a fourth call site the "shared by all three"
framing excluded. Harmless (the controller discards the view model and
re-projects through GetChannelByIdForApi), but a trap: its query lacks the
MirrorSourceChannel include, so swapping in the shared helper would report 0
for a working mirror. Documented that the fix there is a QUERY change.
- Mirrored the rename into the design-system prototype.
The rename collided the badge and filter labels, so the screen tests now scope
the badge assertion to the table (spa-conventions §6). This also fixed a real
weakness: the mirror test's unscoped queryByText would have matched the filter
button and asserted nothing.
Review note: the reviewer's strongest hypothesis — that PUT and GET could
disagree on a mirror channel's count — was investigated and does NOT hold.
Refs #72
This commit is contained in:
@@ -176,6 +176,13 @@ public class UpdateChannelHandler(
|
||||
await workerChannel.WriteAsync(new RefreshChannelData(c.Number), CancellationToken.None);
|
||||
}
|
||||
|
||||
// Deliberately NOT Mapper.GetPlayoutsCount: this handler's query (see Handle) doesn't include
|
||||
// MirrorSourceChannel, so the shared helper would read that navigation as null and return the
|
||||
// same own-playouts-only count anyway — with a false air of Mirror-awareness. Harmless today
|
||||
// because ChannelController discards this view model and re-projects through
|
||||
// GetChannelByIdForApi, so this count never reaches the wire. If you ever return it directly,
|
||||
// fix the QUERY first (add the MirrorSourceChannel ThenInclude) — swapping in the helper alone
|
||||
// would report 0 playouts for a working mirror channel.
|
||||
return ProjectToViewModel(c, c.Playouts?.Count ?? 0);
|
||||
}
|
||||
|
||||
|
||||
@@ -120,7 +120,10 @@ public class ChannelRepositoryPlayoutIncludeTests
|
||||
[Test]
|
||||
public async Task GetChannel_Should_Report_Zero_For_A_Channel_With_No_Playouts()
|
||||
{
|
||||
// The "will never play" signal #72 renders — must stay 0 rather than becoming vacuously non-zero.
|
||||
// Boundary check on the mapper only — this one passes with or WITHOUT the Playouts include
|
||||
// (0 == 0 either way), so unlike its two siblings it is NOT include coverage. It earns its
|
||||
// place by pinning that the "will never play" signal stays 0 and never drifts vacuously
|
||||
// non-zero, which would silently retire the badge.
|
||||
int profileId = await SeedFFmpegProfile();
|
||||
int channelId = await SeedChannel(
|
||||
new Channel(Guid.NewGuid())
|
||||
|
||||
@@ -31,7 +31,7 @@
|
||||
let rows = D.channels;
|
||||
if (view === "onair") rows = rows.filter((c) => c.live);
|
||||
else if (view === "disabled") rows = rows.filter((c) => !c.enabled);
|
||||
else if (view === "problems") rows = rows.filter((c) => c.playouts === 0);
|
||||
else if (view === "noplayout") rows = rows.filter((c) => c.playouts === 0);
|
||||
|
||||
// group, preserving first-seen order
|
||||
const groups = [];
|
||||
@@ -44,11 +44,11 @@
|
||||
const toggleAll = () => { if (allOn) setSel({}); else { const n = {}; rows.forEach((c) => (n[c.num] = true)); setSel(n); } };
|
||||
const toggleOne = (num) => setSel((s) => ({ ...s, [num]: !s[num] }));
|
||||
|
||||
const counts = { all: D.channels.length, onair: D.channels.filter((c) => c.live).length, disabled: D.channels.filter((c) => !c.enabled).length, problems: D.channels.filter((c) => c.playouts === 0).length };
|
||||
const counts = { all: D.channels.length, onair: D.channels.filter((c) => c.live).length, disabled: D.channels.filter((c) => !c.enabled).length, noplayout: D.channels.filter((c) => c.playouts === 0).length };
|
||||
|
||||
const Segmented = () => (
|
||||
<div style={{ display: "inline-flex", gap: 2, padding: 2, borderRadius: "var(--radius-sm)", background: "var(--ctv-bg-sunken)", border: "1px solid var(--border-hairline)" }}>
|
||||
{[{ v: "all", l: "All", c: counts.all }, { v: "onair", l: "On air", c: counts.onair }, { v: "disabled", l: "Disabled", c: counts.disabled }, { v: "problems", l: "Problems", c: counts.problems }].map((o) => {
|
||||
{[{ v: "all", l: "All", c: counts.all }, { v: "onair", l: "On air", c: counts.onair }, { v: "disabled", l: "Disabled", c: counts.disabled }, { v: "noplayout", l: "No playout", c: counts.noplayout }].map((o) => {
|
||||
const on = o.v === view;
|
||||
return (
|
||||
<button key={o.v} type="button" onClick={() => setView(o.v)}
|
||||
|
||||
@@ -1,4 +1,4 @@
|
||||
import { cleanup, fireEvent, render, screen, waitFor } from '@testing-library/react';
|
||||
import { cleanup, fireEvent, render, screen, waitFor, within } from '@testing-library/react';
|
||||
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
|
||||
import { ChannelsScreen } from './ChannelsScreen';
|
||||
|
||||
@@ -189,12 +189,13 @@ describe('ChannelsScreen — load + render', () => {
|
||||
|
||||
render(<ChannelsScreen />);
|
||||
|
||||
expect(await screen.findByRole('table', { name: 'Channels lineup' })).toBeInTheDocument();
|
||||
// Visible text, not a title-only affordance.
|
||||
expect(screen.getByText('No playout')).toBeInTheDocument();
|
||||
expect(screen.getByRole('button', { name: 'Problems 1' })).toBeInTheDocument();
|
||||
const table = await screen.findByRole('table', { name: 'Channels lineup' });
|
||||
// Scoped to the table: the filter button carries the same label, and an unscoped getByText
|
||||
// matches both (spa-conventions §6). Visible text, not a title-only affordance.
|
||||
expect(within(table).getByText('No playout')).toBeInTheDocument();
|
||||
expect(screen.getByRole('button', { name: 'No playout 1' })).toBeInTheDocument();
|
||||
|
||||
fireEvent.click(screen.getByRole('button', { name: 'Problems 1' }));
|
||||
fireEvent.click(screen.getByRole('button', { name: 'No playout 1' }));
|
||||
|
||||
expect(screen.getByText('Broken')).toBeInTheDocument();
|
||||
expect(screen.queryByText('Healthy')).not.toBeInTheDocument();
|
||||
@@ -209,9 +210,11 @@ describe('ChannelsScreen — load + render', () => {
|
||||
|
||||
render(<ChannelsScreen />);
|
||||
|
||||
expect(await screen.findByRole('table', { name: 'Channels lineup' })).toBeInTheDocument();
|
||||
expect(screen.queryByText('No playout')).not.toBeInTheDocument();
|
||||
expect(screen.getByRole('button', { name: 'Problems 0' })).toBeInTheDocument();
|
||||
const table = await screen.findByRole('table', { name: 'Channels lineup' });
|
||||
// Scoped: the filter button always renders this label, so an unscoped query would find it
|
||||
// even when no row is flagged, making the assertion meaningless.
|
||||
expect(within(table).queryByText('No playout')).not.toBeInTheDocument();
|
||||
expect(screen.getByRole('button', { name: 'No playout 0' })).toBeInTheDocument();
|
||||
});
|
||||
|
||||
it('polls only channel state after the initial load', async () => {
|
||||
|
||||
@@ -27,13 +27,18 @@ import {
|
||||
import { navigateToPath } from '../routing';
|
||||
import { usePrimaryAction } from '../primaryAction';
|
||||
|
||||
type ChannelViewFilter = 'all' | 'onair' | 'disabled' | 'problems';
|
||||
type ChannelViewFilter = 'all' | 'onair' | 'disabled' | 'noplayout';
|
||||
|
||||
// #72: a channel with no playout can never play — the dominant "misconfigured lineup" case, and the
|
||||
// only per-channel failure the API can answer cheaply today. A mirror channel relays its source's
|
||||
// playouts, and the server already folds those into playoutCount, so mirrors are not false-flagged.
|
||||
// Deliberately NOT covered here: an empty schedule behind an existing playout, and broken/missing
|
||||
// sources — see #72 for why both are deferred rather than approximated.
|
||||
//
|
||||
// Deliberately NOT covered: an empty schedule behind an existing playout, and broken/missing sources
|
||||
// (see decisions.md 2026-07-17 for why approximating either would be wrong today). Because those
|
||||
// fault classes are real but invisible here, the badge AND the filter are both named for the single
|
||||
// fault this can actually prove — "No playout", never "Problems". A "Problems 0" reading on a lineup
|
||||
// that holds a dead empty-schedule channel is a false all-clear, which is worse than no affordance.
|
||||
// Do not broaden the label until the taxonomy behind it exists.
|
||||
function willNeverPlay(channel: ChannelSummary): boolean {
|
||||
return channel.playoutCount === 0;
|
||||
}
|
||||
@@ -202,7 +207,7 @@ export function ChannelsScreen() {
|
||||
const statesById = stateByChannelId(query.data.channelStates);
|
||||
const onAirCount = query.data.channelStates.filter((state) => state.onAir).length;
|
||||
const disabledCount = channels.filter((channel) => !channel.isEnabled).length;
|
||||
const problemCount = channels.filter(willNeverPlay).length;
|
||||
const noPlayoutCount = channels.filter(willNeverPlay).length;
|
||||
const visibleChannels = channels.filter((channel) => {
|
||||
if (filter === 'onair') {
|
||||
return statesById.get(channel.id)?.onAir === true;
|
||||
@@ -212,7 +217,7 @@ export function ChannelsScreen() {
|
||||
return !channel.isEnabled;
|
||||
}
|
||||
|
||||
if (filter === 'problems') {
|
||||
if (filter === 'noplayout') {
|
||||
return willNeverPlay(channel);
|
||||
}
|
||||
|
||||
@@ -410,7 +415,7 @@ export function ChannelsScreen() {
|
||||
<button type="button" aria-pressed={filter === 'all'} onClick={() => changeFilter('all')}>All <code>{channels.length}</code></button>
|
||||
<button type="button" aria-pressed={filter === 'onair'} onClick={() => changeFilter('onair')}>On air <code>{onAirCount}</code></button>
|
||||
<button type="button" aria-pressed={filter === 'disabled'} onClick={() => changeFilter('disabled')}>Disabled <code>{disabledCount}</code></button>
|
||||
<button type="button" aria-pressed={filter === 'problems'} onClick={() => changeFilter('problems')}>Problems <code>{problemCount}</code></button>
|
||||
<button type="button" aria-pressed={filter === 'noplayout'} onClick={() => changeFilter('noplayout')}>No playout <code>{noPlayoutCount}</code></button>
|
||||
</div>
|
||||
<span className="ctv-channels-spacer" />
|
||||
<span className="ctv-channels-live"><StatusDot status="live" size={7} /><code>{onAirCount}</code> on air</span>
|
||||
|
||||
Reference in New Issue
Block a user