Cold review of 572737a29. Findings taken; two are corrections to my own claims.
CORRECTION: "a regression this branch INTRODUCED" was wrong, and I verified the
reviewer's counter-claim against origin/main before accepting it. That handler
already included SongMetadata AND already routed Song there, so
GET /api/v1/playlists/{id}/items was ALREADY a live 500 for a null-Artists song.
This branch only made the same throw reachable on a second path. The record said
so twice; both are fixed, because a wrong explanation outlives a wrong line.
SWEEP: fixing one site left the mirror standing -- Playouts/Mapper.GetDisplayTitle
had the identical unguarded join on a path that also eager-loads SongMetadata, so
it too was live, feeding the playout guide, troubleshooting, media-item info and
channel states. Guarded, with a unit test; reverting it reproduces
ArgumentNullException. LibraryBrowseItemMapper already wrote `Artists ?? []`, so
the nullability was known in-tree and these sites were simply unswept. Filed #691
for the remaining SongVideoGenerator dereferences on the playback path.
Also: documented that the shared matrix is the RERUN predicate used as a superset
for playlists (the playlist write path rejects RemoteStream today); noted the
third, inert consumer ReplacePlaylistItemsHandler; added the new mechanisms to the
record's `mechanics:` field; and trimmed the record under the 60-line prose
ceiling -- it was tipping the corpus p90 above the ceiling and reddening the
calibration test in scripts/tests, which passes again.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
rule:'Every handler that projects an aggregate carrying a tagged-union selection loads it through ONE shared `<Aggregate>QueryExtensions` include chain — `RerunCollectionQueryExtensions.IncludeSelectionDetails()`, joining the existing `ProgramScheduleItemQueryExtensions.IncludeScheduleItemDetails()` — called by the paged-list handler and the by-id handler alike, so the two cannot drift. The media-item flattening switch is likewise ONE shared helper, `MediaCollections.Mapper.ProjectMediaItemToViewModel`, covering all ten selectable media types including `RemoteStream`, whose named projection is `MediaItems.Mapper.ProjectToNamedViewModel` (it cannot be an overload of `ProjectToViewModel(RemoteStream)`, which already exists returning the unrelated `RemoteStreamViewModel`; C# will not overload on return type). That switch NEVER ends in `_ => null`: a null MediaItem is the legitimate not-a-media-item case, while an unrecognized non-null subtype keeps its id and takes a conspicuous `[unsupported media type: X]` name. Fail-soft is deliberate — throwing would fail an entire paged GET over one unreadable row. Finally, every metadata navigation inside `MediaItems.Mapper` is read through `Optional(...).Flatten()` and degrades to the `"???"` placeholder, because those projections are reached from handlers whose include chains differ and a bare `x.Season.Show.ShowMetadata` is a latent 500 on some other caller GET.'
signals:'rerun collection null selection, selectedId null for every row, list badge renders Collection with no name, detail GET 500 on Episode, detail GET 500 on MusicVideo, RemoteStream dropped by the mapper, underscore arrow null fallthrough, AsNoTracking suppresses navigation fixup, eager load missing on paged list, Include after Skip Take, EpisodeTitle NullReferenceException, MusicVideoTitle bare Artist deref, ShowTitle bare Show deref, id only as available as the name, editor silently clears stored selection · paths:`ErsatzTV.Application/MediaCollections/RerunCollectionQueryExtensions.cs`, `ErsatzTV.Application/MediaCollections/Mapper.cs`, `ErsatzTV.Application/MediaItems/Mapper.cs`, `ErsatzTV.Application/MediaCollections/Queries/GetPagedRerunCollectionsHandler.cs`, `ErsatzTV.Application/MediaCollections/Queries/GetRerunCollectionByIdHandler.cs`, `docs/api-conventions.md` §2a · issues:#671, #651, #229'
Applies the `#229` shared-include-chain remedy to the READ path. That record framed the rule as a
write-path concern (project the mutation response through the include chain the GET uses); #671 is
its mirror image, where the GET itself under-loaded.
Applies the `#229` shared-include-chain remedy to the READ path — that record framed it as a
write-path concern (project the mutation response through the GET's chain); this is its mirror image,
where the GET itself under-loaded.
## The coupling that hid the bug
The id and the display name are read off the SAME eager-loaded navigation, so the id is only ever as
available as the name. There is no state where the API knows WHICH item is selected but not what it
is called. That is why the client-side symptom looked like a naming problem: an unlabelled badge.
The real harm is one level down — the selected id is null too, and an editor that round-trips it
silently clears the stored selection. #651 added a client-side merge-instead-of-replace guard that
made this survivable; that guard stays, but it was a client-side patch over a server-side defect.
The corollary is the rule: never let the id and the name share a single point of failure. Hence the
`_ => null` ban — an unrecognized subtype surrenders its NAME, never its ID.
## Why fail-soft rather than a throw
The Done-when asked for the unhandled case to be handled or made loud. A throw is loud but fails an
entire paged GET over one bad row, on a read path. A conspicuous placeholder name is loud in the UI,
greppable in logs, and preserves the id — the thing that actually matters. Recorded because a future
session could reasonably reach for `NotSupportedException` here, as the sibling
`ProgramSchedules.Mapper` item-type switch does; that one dispatches on the ITEM type, an internal
closed set, where a throw is correct.
The id and the display name are read off the SAME navigation, so the id is only ever as available as
the name — the API never knows WHICH item is selected but not what it is called. Hence the symptom
looked like a naming problem (an unlabelled badge) when the real harm is one level down: the selected
id is null too, and an editor that round-trips it clears the stored selection. #651's client-side
merge-instead-of-replace guard made this survivable and stays, but it patched a server defect from
the client. The rule: never let the id and the name share a single point of failure — hence the
`_ => null` ban, where an unrecognized subtype surrenders its NAME, never its ID. Fail-soft, not a
throw, since a throw fails a whole paged GET over one bad row. (`ProgramSchedules.Mapper`'s switch
does throw, correctly — it dispatches on the ITEM type, an internal closed set.)
## Scope deliberately not widened
Nine further media-item switches exist (four in `ProgramSchedules.Mapper`, five in
`Scheduling.Mapper`) and all handle only Show/Season/Artist. That is NOT the same oversight: those
call sites genuinely restrict selection to those three types, and their handlers load a matching
chain. Only the RerunCollection and PlaylistItem switches span the full set, which is why exactly
those two were merged. `GetPlaylistItemsHandler` already loaded the other nine types, so PlaylistItem
was never broken in production; adding the shared `RemoteStream` arm did oblige it to gain the one
matching `RemoteStreamMetadata` include, or that name alone would have degraded to `"???"`.
Nine further media-item switches (`ProgramSchedules.Mapper`×4, `Scheduling.Mapper`×5) handle only
Show/Season/Artist. Not the same oversight: those call sites genuinely restrict selection to those
three and their handlers load a matching chain. Only the RerunCollection and PlaylistItem switches
span the full set, so exactly those two were merged. A THIRD consumer, `ReplacePlaylistItemsHandler`,
projects items whose navigations are never loaded — inert only because the controller discards the
result and re-queries; a dead projection, not a working one.
**Widening a shared switch incurs a debt in every caller loading for it.** Adding an arm is not
free: each consumer must be re-checked against the navigations the new arm dereferences — and
discharged by a TEST, not by inspection, since inspection is the method that produced this bug.
`GetPlaylistItemsHandler` had no handler-level test at all (its controller tests stub the mediator),
so it gained the same 13-type matrix via the shared `SelectionSeedData`.
**Widening a shared switch incurs a debt in every caller loading for it**, discharged by a TEST, not
by inspection — inspection is the method that produced this bug. `GetPlaylistItemsHandler` had no
handler-level test at all (its controller tests stub the mediator), so it gained the same 13-type
matrix via the shared `SelectionSeedData`.
## Adding an include can CREATE a 500
## `Artists` is a nullable PRIMITIVE COLLECTION, and the sweep must follow the field
Eager-loading is not automatically safe. `SongMetadata.Artists` is a nullable EF **primitive
collection** — a JSON array in one column, not a navigation — that `FallbackMetadataProvider` leaves
unassigned whenever a song's tags fail to read, and `string.Join` throws `ArgumentNullException`(not
`NullReferenceException`) on a null sequence. The rerun list previously did not load `SongMetadata`
at all, so the throw was unreachable there; adding the include promoted it to a live 500 that would
have failed the entire page. The guard is `Optional(sm.Artists).Flatten()`, and the empty case is
filtered too so an artist-less song is not prefixed with a bare `" - "`.
`SongMetadata.Artists` is a nullable EF primitive collection — a JSON array in one column, **not a
navigation** — left unassigned by `FallbackMetadataProvider` when a song's tags fail to read, and
`string.Join` throws `ArgumentNullException`, not`NullReferenceException`, on a null sequence. So a
"null navigation" audit misses it and so does a grep for `NullReferenceException`.
The general rule: when you widen an include chain, audit what the newly-reachable projection
dereferences. A "null navigation" audit is not enough — nullable primitive collections throw a
different exception type and will not turn up in a grep for `NullReferenceException`.
Two corrections, because a wrong explanation outlives a wrong line. It was **not** introduced here:
`GetPlaylistItemsHandler` already included `SongMetadata` on `origin/main` and already routed `Song`,
so `GET /api/v1/playlists/{id}/items` was ALREADY a live 500 — this branch only made the same throw
reachable on a second path. And fixing the rerun site alone left the mirror standing: `Playouts/Mapper`
had the identical unguarded join on a path that also eager-loads `SongMetadata`, likewise live, swept
here. `LibraryBrowseItemMapper` already wrote `Artists ?? []`, so the codebase knew.
Filed separately: `SongVideoGenerator` dereferences `Artists.Count`/`.Contains` unguarded on the
playback path. Sweep by FIELD, not by the one call site the issue named.
## Verification worth repeating
Each mechanism was removed in turn and the suite quoted red before the fix was restored: stripping
the list include chain failed all 13 types with "lost its selected id"; restoring the original
four-type by-id chain failed exactly the six the issue named; and reverting only the three bare
dereferences reproduced the `NullReferenceException` for Episode and MusicVideo. A green new test
over a read path proves little until the mechanism it covers has been shown to fail without it.
Every mechanism was removed in turn and the suite quoted red before restoring it: stripping the list
include chain failed all 13 types on "lost its selected id"; the original four-type by-id chain failed
exactly the six the issue named; reverting the bare dereferences reproduced the `NullReferenceException`
for Episode and MusicVideo; and reverting either `Artists` guard reproduced `ArgumentNullException`.
A green new test over a read path proves little until it has been shown to fail without its mechanism.
The per-type name assertion pins the WHOLE expected string, not merely "is not a placeholder". A
cross-family review caught that the looser form could not see a missing NESTED leg: drop
Episode → Season → Show and the projection still reads `s00e04 - Selected episode`, which contains
no placeholder marker and would have passed. Related: an absent Season renders `s??`, never `s00`,
since `s00` conventionally means Specials and would fabricate plausible-looking real data.
The per-type assertion pins the WHOLE expected string, not merely "is not a placeholder", because a
review caught that the looser form cannot see a missing NESTED leg: drop Episode → Season → Show and
the projection still reads `s00e04 - Selected episode`, placeholder-free, and passes. Relatedly an
absent Season renders `s??`, never `s00`, which conventionally means Specials and would fabricate
plausible-looking real data.
Reference in New Issue
Block a user
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.
Claude Opus 5