fix(671): resolve rerun-collection selections through one shared include chain #692

Merged

timothy merged 5 commits from fix/671-rerun-collection-selection into main 2026-07-28 20:56:13 +02:00

Conversation 1 Commits 5 Files Changed 14

+834 -63

timothy commented 2026-07-28 20:28:14 +02:00

Owner

Copy Link

Copy Source

fixes #671

The defect

The issue's diagnosis was accurate, with one correction: RemoteStream does have a mapper overload, but it returns RemoteStreamViewModel, an unrelated record. C# will not overload on return type, so there was no named projection for it to fall back on — which is why it fell through _ => null.

The root coupling: the selected id and the display name are read off the same eager-loaded navigation, so the id is only ever as available as the name. That makes an un-included type look like a naming bug (an unlabelled badge) when the real harm is one level down — the selected id is null too, and the editor round-trips it, silently clearing the user's stored selection.

The fix — at the boundary, not the site

• RerunCollectionQueryExtensions.IncludeSelectionDetails() — one include chain called by both handlers, following the existing ProgramScheduleItemQueryExtensions.IncludeScheduleItemDetails() precedent (#229) rather than pasting a 14-line chain twice. #229 framed that rule as a write-path concern; this is its mirror image, where the GET itself under-loaded.
• One media-item switch. It was duplicated verbatim for RerunCollection and PlaylistItem; both now call ProjectMediaItemToViewModel, which handles RemoteStream and never falls through to null — an unknown subtype keeps its id and takes a conspicuous [unsupported media type: X] name. Throwing would fail a whole paged GET over one bad row.
• Null-guards in MediaItems.Mapper, so the next handler with a thinner chain can't reintroduce the 500.

Beyond the issue

Four review rounds (one cross-family via Codex, three cold same-family) surfaced defects the issue did not name:

• SongMetadata.Artists is a nullable EF primitive collection — a JSON array in one column, not a navigation — left unassigned by FallbackMetadataProvider for a song whose tags fail to read. string.Join throws ArgumentNullException on it, so a "null navigation" audit misses it and so does a grep for NullReferenceException. It was already a live 500 on GET /api/v1/playlists/{id}/items (verified against origin/main); eager-loading SongMetadata on the rerun list made the same throw reachable on a second path. Guarded, and swept by FIELD — Playouts/Mapper.GetDisplayTitle had the identical unguarded join on a path that also eager-loads SongMetadata, likewise live, feeding the playout guide, troubleshooting, media-item info and channel states.
• GetDisplayTitle's Song arm interpolated the entity, not the title — {s} (the case Song s pattern variable) instead of {t}. Song has no ToString() override, so a chaptered song rendered as the literal ErsatzTV.Core.Domain.Song (Chapter 3). Pre-existing; fixed because it is one token inside the block this branch already edits, and this PR adds the first-ever tests for that method.
• GetPlaylistItemsHandler had no handler-level test at all (its controller tests stub the mediator), so widening the shared switch there would have been discharged by inspection — the method that produced #671. It now runs the same 13-type matrix.

Verification

Every mechanism was removed in turn and the suite quoted red before restoring it — a green new test over a read path proves little otherwise:

Mutation	Observed
Strip the list include chain	all 13 types fail lost its selected id
Restore the original 4-type by-id chain	exactly the 6 types the issue named fail
Revert the 3 bare dereferences	NullReferenceException (Episode, MusicVideo)
Revert either Artists guard	ArgumentNullException: Value cannot be null (Parameter 'values')
Remove the playlist RemoteStreamMetadata include	playlist RemoteStream fails
Remove the nested Episode → Season → Show leg	Episode fails on the pinned name
Revert the chapter-title fix	renders ErsatzTV.Core.Domain.Song (Chapter 1)

The test matrix is derived from the production predicate (IsSupportedSelectionType), so a newly-supported type joins it automatically and trips a loud default:; a set-equality completeness guard fails on narrowing and widening. Names are pinned in full rather than checked for "not a placeholder", because the looser form cannot see a missing nested include leg.

All 7 test projects green — 4,444 passed, 0 failed. Format gate, decisions validator and scripts/tests (338) clean. No /api/* contract change (behaviour fix only; DTO and OpenAPI unchanged), so no regeneration was required.

Docs

• docs/api-conventions.md → new §2a, "Flattening a tagged-union selection (read path)"
• docs/decisions/records/api/selection-projection-include-chain.md (+ regenerated catalog)

Deferred, filed not folded

• #690 — the paged TotalCount ignores the search query, so the SPA renders empty pages (pre-existing, different axis)
• #691 — SongVideoGenerator dereferences Artists/AlbumArtists unguarded on the playback path, where the right fallback needs thought rather than a mechanical ?? []

🤖 Generated with Claude Code

fixes #671 ## The defect The issue's diagnosis was accurate, with one correction: `RemoteStream` **does** have a mapper overload, but it returns `RemoteStreamViewModel`, an unrelated record. C# will not overload on return type, so there was no named projection for it to fall back on — which is why it fell through `_ => null`. The root coupling: the selected **id** and the display **name** are read off the *same* eager-loaded navigation, so the id is only ever as available as the name. That makes an un-included type look like a naming bug (an unlabelled badge) when the real harm is one level down — the selected **id** is null too, and the editor round-trips it, silently clearing the user's stored selection. ## The fix — at the boundary, not the site - **`RerunCollectionQueryExtensions.IncludeSelectionDetails()`** — one include chain called by *both* handlers, following the existing `ProgramScheduleItemQueryExtensions.IncludeScheduleItemDetails()` precedent (#229) rather than pasting a 14-line chain twice. #229 framed that rule as a write-path concern; this is its mirror image, where the GET itself under-loaded. - **One media-item switch.** It was duplicated *verbatim* for `RerunCollection` and `PlaylistItem`; both now call `ProjectMediaItemToViewModel`, which handles `RemoteStream` and never falls through to null — an unknown subtype keeps its **id** and takes a conspicuous `[unsupported media type: X]` name. Throwing would fail a whole paged GET over one bad row. - **Null-guards in `MediaItems.Mapper`**, so the next handler with a thinner chain can't reintroduce the 500. ## Beyond the issue Four review rounds (one cross-family via Codex, three cold same-family) surfaced defects the issue did not name: - **`SongMetadata.Artists` is a nullable EF *primitive collection*** — a JSON array in one column, not a navigation — left unassigned by `FallbackMetadataProvider` for a song whose tags fail to read. `string.Join` throws `ArgumentNullException` on it, so a "null navigation" audit misses it and so does a grep for `NullReferenceException`. It was **already** a live 500 on `GET /api/v1/playlists/{id}/items` (verified against `origin/main`); eager-loading `SongMetadata` on the rerun list made the same throw reachable on a second path. Guarded, and swept by FIELD — `Playouts/Mapper.GetDisplayTitle` had the identical unguarded join on a path that also eager-loads `SongMetadata`, likewise live, feeding the playout guide, troubleshooting, media-item info and channel states. - **`GetDisplayTitle`'s Song arm interpolated the entity, not the title** — `{s}` (the `case Song s` pattern variable) instead of `{t}`. `Song` has no `ToString()` override, so a chaptered song rendered as the literal `ErsatzTV.Core.Domain.Song (Chapter 3)`. Pre-existing; fixed because it is one token inside the block this branch already edits, and this PR adds the first-ever tests for that method. - **`GetPlaylistItemsHandler` had no handler-level test at all** (its controller tests stub the mediator), so widening the shared switch there would have been discharged by inspection — the method that produced #671. It now runs the same 13-type matrix. ## Verification Every mechanism was removed in turn and the suite quoted **red** before restoring it — a green new test over a read path proves little otherwise: | Mutation | Observed | |---|---| | Strip the list include chain | all 13 types fail `lost its selected id` | | Restore the original 4-type by-id chain | exactly the 6 types the issue named fail | | Revert the 3 bare dereferences | `NullReferenceException` (Episode, MusicVideo) | | Revert either `Artists` guard | `ArgumentNullException: Value cannot be null (Parameter 'values')` | | Remove the playlist `RemoteStreamMetadata` include | playlist RemoteStream fails | | Remove the nested `Episode → Season → Show` leg | Episode fails on the pinned name | | Revert the chapter-title fix | renders `ErsatzTV.Core.Domain.Song (Chapter 1)` | The test matrix is derived from the production predicate (`IsSupportedSelectionType`), so a newly-supported type joins it automatically and trips a loud `default:`; a set-equality completeness guard fails on narrowing *and* widening. Names are pinned in full rather than checked for "not a placeholder", because the looser form cannot see a missing **nested** include leg. **All 7 test projects green — 4,444 passed, 0 failed.** Format gate, decisions validator and `scripts/tests` (338) clean. No `/api/*` contract change (behaviour fix only; DTO and OpenAPI unchanged), so no regeneration was required. ## Docs - `docs/api-conventions.md` → new §2a, "Flattening a tagged-union selection (read path)" - `docs/decisions/records/api/selection-projection-include-chain.md` (+ regenerated catalog) ## Deferred, filed not folded - **#690** — the paged `TotalCount` ignores the search query, so the SPA renders empty pages (pre-existing, different axis) - **#691** — `SongVideoGenerator` dereferences `Artists`/`AlbumArtists` unguarded on the *playback* path, where the right fallback needs thought rather than a mechanical `?? []` 🤖 Generated with [Claude Code](https://claude.com/claude-code)

timothy added 5 commits 2026-07-28 20:28:15 +02:00

fix(671): resolve rerun-collection selections through one shared include chain ... 8523088ceb

The paged list handler eager-loaded nothing, so `ProjectToViewModel` read four
unloaded navigations and every row of every collection type projected a null
selection. Because the selected id and the display name are read off the SAME
navigation, this dropped the id too -- the harm is not an unlabelled badge but
an editor that round-trips a null and clears the user's stored selection.

The by-id handler loaded metadata for only four of the ten selectable media
types: Song/OtherVideo/Image/RemoteStream returned a null-ish selection and
Episode/MusicVideo threw an NRE that surfaced as a 500.

Fixed at the boundary rather than per call site:

- `RerunCollectionQueryExtensions.IncludeSelectionDetails()` is now the single
  include chain, called by both handlers, joining the existing
  `ProgramScheduleItemQueryExtensions.IncludeScheduleItemDetails()` precedent
  (#229). Artwork legs are deliberately omitted -- this projection reads only
  ids and titles.
- The media-item switch was duplicated verbatim for RerunCollection and
  PlaylistItem; both now call one `ProjectMediaItemToViewModel`, which handles
  `RemoteStream` (via a new `ProjectToNamedViewModel`, since the existing
  `ProjectToViewModel(RemoteStream)` returns an unrelated type) and never falls
  through to null -- an unknown subtype keeps its id and takes a conspicuous
  name, because throwing would fail a whole paged GET over one bad row.
- Every metadata navigation in `MediaItems.Mapper` is now read through
  `Optional(...).Flatten()`, so an un-included nav degrades to "???" instead of
  being a latent 500 for whichever caller loads least.

Tests enumerate all 13 supported CollectionTypes for both handlers, with the
matrix derived from `IsSupportedSelectionType` so a newly-supported type joins
it automatically, plus a completeness guard on the set. Each mechanism was
removed in turn and confirmed red first.

fixes #671

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

fix(671): review round 1 -- pin exact names, complete the playlist include ... 017ef988d0

Cross-family (Codex) adversarial review of 8523088ce. All three findings taken:

- The name assertion only checked "not a placeholder", so it could not see a
  missing NESTED include leg: dropping Episode -> Season -> Show still renders
  "s00e04 - Selected episode", which contains no placeholder marker and passed.
  Now every type pins its whole expected string; re-removing that leg fails, as
  verified before restoring it.
- Widening the shared switch with a RemoteStream arm put `GetPlaylistItemsHandler`
  one include short -- it loaded metadata for the other nine types, so playlist
  RemoteStream names alone would have degraded to "???".
- `?? 0` rendered an unloaded Season as "s00", which conventionally means
  Specials and so fabricated plausible-looking real data; it now renders "s??".

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

fix(671): review round 2 -- guard Song.Artists, cover the second consumer ... 572737a29e

Cold independent review of 017ef988d. Three findings taken, one filed out.

The important one is a regression this branch INTRODUCED. `SongMetadata.Artists`
is a nullable EF primitive collection (JSON in one column, not a navigation)
that FallbackMetadataProvider leaves unassigned when a song's tags fail to read,
and `string.Join` throws ArgumentNullException 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
whole page. Confirmed by reverting the guard: ArgumentNullException, parameter
'values'. The file header claiming every member was guarded was false.
The empty case is filtered too, so an artist-less song loses its bare " - ".

Second: `GetPlaylistItemsHandler` had no handler-level test at all (its
controller tests stub the mediator), so the RemoteStream include added last
round was discharged by inspection -- the same method that produced #671. It
now runs the same 13-type matrix via a shared SelectionSeedData; removing the
include fails that matrix.

Third: dropped the dead `(i as Season).SeasonMetadata` include leg -- the Season
projection reads Show.ShowMetadata and the scalar SeasonNumber, never
SeasonMetadata.

Filed #690 for the pre-existing, out-of-scope finding: the paged TotalCount
ignores the search query, so the SPA renders empty pages.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

fix(671): review round 3 -- sweep Artists by FIELD, correct the archaeology ... 34eee753b2

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>

fix(671): review round 4 -- fix the chapter-title entity interpolation ... 2249a806c9

Final cold review of 34eee753b: the commit's own changes were confirmed correct,
but it flagged a real pre-existing bug in the exact block I had just edited, and
I was adding the first-ever tests for that method without covering it.

`Playouts/Mapper.GetDisplayTitle`'s Song arm interpolated `{s}` -- the
`case Song s` ENTITY -- into its chapter-title branch instead of `{t}`, the
composed title. Song has no ToString() override, so a chaptered song rendered as
the literal "ErsatzTV.Core.Domain.Song (Chapter 3)" in the playout guide,
troubleshooting, media-item info and channel states. The sibling MusicVideo and
OtherVideo arms are correct only because they happen to name their lambda `s`.
Pre-existing on main; fixed here because it is one token inside the block this
branch already touches. Two tests pin it; reverting renders the type name.

Also: completed the guard on that arm (`Optional(s.SongMetadata).Flatten()`, the
other half of the sibling pattern I claimed to have copied), added the new
mechanism to the record's `mechanics:`, added the symptom tokens a future session
would actually search for (ArgumentNullException, Artists, primitive collection,
chaptered song) to `signals:`, restored the remedy sentence an earlier trim
dropped, and trimmed to 59 prose lines for margin under the 60-line ceiling.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

timothy commented 2026-07-28 20:55:48 +02:00

Author

Owner

Copy Link

Copy Source

Review-verdict: MERGEABLE @ 2249a80

Four review rounds: one cross-family (Codex) plus three cold same-family, each re-reviewing the prior fix commit. Rounds 1-3 findings all taken; round 4 fixed a pre-existing entity-vs-title interpolation bug it found in the block this branch edits. Round 4's own changes are one token, one guard completion, two tests and doc edits, each verified by removing the mechanism and observing the red rather than by a fifth review round. Every mechanism in the branch was mutation-tested. All 7 test projects green locally (4444 passed); CI green with Build and test at 25m35s, so not a docs-only fast path. Posting as the PR author, and merging.

Review-verdict: MERGEABLE @ 2249a80 Four review rounds: one cross-family (Codex) plus three cold same-family, each re-reviewing the prior fix commit. Rounds 1-3 findings all taken; round 4 fixed a pre-existing entity-vs-title interpolation bug it found in the block this branch edits. Round 4's own changes are one token, one guard completion, two tests and doc edits, each verified by removing the mechanism and observing the red rather than by a fifth review round. Every mechanism in the branch was mutation-tested. All 7 test projects green locally (4444 passed); CI green with Build and test at 25m35s, so not a docs-only fast path. Posting as the PR author, and merging.

timothy merged commit 036bcfc5a0 into main 2026-07-28 20:56:13 +02:00

timothy deleted branch fix/671-rerun-collection-selection 2026-07-28 20:56:14 +02:00

timothy referenced this issue from a commit 2026-07-28 20:56:15 +02:00

Merge pull request 'fix(671): resolve rerun-collection selections through one shared include chain' (#692) from fix/671-rerun-collection-selection into main

timothy referenced this pull request 2026-07-28 20:56:53 +02:00

Rerun collections: the list endpoint returns a null selection for every row, and the detail GET 500s or nulls for five media types #671

Sign in to join this conversation.

Reviewers

No Reviewers

Labels

Clear labels

ad-hoc

One-off / ad-hoc work not tracked by a dedicated issue

api

REST API / HTTP endpoints

bug

Something isn't working

ci-cd

Build, test, deploy pipeline

content

Channel content / schedules / playlists

dependencies

Dependency updates (Renovate)

enhancement

New feature or improvement

frontend

ChicoryTV React SPA frontend

in-progress

Claimed by an active session — do not pick up

jellyfin

Jellyfin tuner / IPTV integration

parked

Excluded from automatic queue pickup; work only when explicitly selected

priority: high

priority: low

priority: medium

review

Adversarial review finding

security

Security / vulnerability fix

No labels

Milestone

No items

No Milestone

Projects

Clear projects

No projects

Assignees

Clear assignees

renovate timothy (Timothy)

No Assignees

1 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: timothy/ersatztv#692