timothy/ersatztv
1
0
Fork 0
Code Issues 55 Pull Requests 6 Actions 2 Packages Projects Releases 9 Wiki Activity

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

Browse Source 
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>
This commit is contained in:
timothy
2026-07-28 19:00:08 +02:00
co-authored by Claude Opus 5
parent 8523088ceb
commit 017ef988d0
4 changed files with 50 additions and 14 deletions
Download Patch File Download Diff File Expand all files Collapse all files
ErsatzTV.Application/MediaCollections/Queries/GetPlaylistItemsHandler.cs
+4
View File
@@ -55,6 +55,10 @@ public class GetPlaylistItemsHandler(IDbContextFactory<TvContext> dbContextFacto
.Include(i => i.MediaItem)
.ThenInclude(i => (i as Image).ImageMetadata)
.ThenInclude(mm => mm.Artwork)
// RemoteStream is projected by the shared ProjectMediaItemToViewModel switch as of #671;
// without its metadata the name would degrade to "???" here while every sibling type resolves.
.Include(i => i.MediaItem)
.ThenInclude(i => (i as RemoteStream).RemoteStreamMetadata)
.ToListAsync(cancellationToken);
return allItems.Map(Mapper.ProjectToViewModel).ToList();
ErsatzTV.Application/MediaItems/Mapper.cs
+5 -1
View File
@@ -109,7 +109,11 @@ internal static class Mapper
var numbersString = $"e{string.Join('e', episodeNumbers.Map(n => $"{n:00}"))}";
var titlesString = $"{string.Join('/', episodeTitles)}";
return $"{showTitle}s{e.Season?.SeasonNumber ?? 0:00}{numbersString} - {titlesString}";
// "s00" conventionally means Specials, so an unloaded Season must not borrow it — that would
// fabricate plausible-looking real data. Render the season as explicitly unknown instead.
string seasonNumber = e.Season is null ? "??" : $"{e.Season.SeasonNumber:00}";
return $"{showTitle}s{seasonNumber}{numbersString} - {titlesString}";
}
private static string MusicVideoTitle(MusicVideo mv)
ErsatzTV.Tests/Application/MediaCollections/RerunCollectionQueryHandlerTests.cs
+29 -11
View File
@@ -83,6 +83,32 @@ public class RerunCollectionQueryHandlerTests : MediaCollectionHandlerTestBase
AssertSelectionResolved(result.Page[0], collectionType);
}
/// <summary>
/// The exact projected name per type. Pinning the whole string — rather than merely asserting
/// "not a placeholder" — is what makes a missing NESTED include leg visible: dropping
/// Episode → Season → Show still yields the non-placeholder "s00e04 - Selected episode", and
/// dropping MusicVideo → Artist still yields "Selected music video". Both would sail past a
/// looser assertion while having lost real data.
/// </summary>
private static string ExpectedName(CollectionType collectionType) =>
collectionType switch
{
CollectionType.Collection => "Selected collection",
CollectionType.MultiCollection => "Selected multi collection",
CollectionType.SmartCollection => "Selected smart collection",
CollectionType.TelevisionShow => "Selected show (2020)",
CollectionType.TelevisionSeason => "Parent show (2020) - Season 3",
CollectionType.Artist => "Selected artist",
CollectionType.Movie => "Selected movie (2019)",
CollectionType.Episode => "Episode's show - s02e04 - Selected episode",
CollectionType.MusicVideo => "Video's artist - Selected music video",
CollectionType.OtherVideo => "Selected other video",
CollectionType.Song => "Song artist - Selected song",
CollectionType.Image => "Selected image",
CollectionType.RemoteStream => "Selected remote stream",
_ => throw new AssertionException($"No expected name pinned for {collectionType}")
};
/// <summary>
/// Mirrors <c>RerunCollectionController.ProjectToResponseModel</c>, which flattens the tagged
/// union to the single <c>selectedId</c> / <c>selectedName</c> pair the SPA consumes. The id is
@@ -102,16 +128,7 @@ public class RerunCollectionQueryHandlerTests : MediaCollectionHandlerTestBase
?? vm.MediaItem?.Name;
selectedId.ShouldBe(SelectedId, $"{collectionType} lost its selected id");
selectedName.ShouldNotBeNullOrWhiteSpace($"{collectionType} lost its selected name");
// The placeholders the mappers emit when metadata is missing. Asserting merely "not null" would
// pass on "???" — i.e. on a selection whose navigation was never loaded.
selectedName.ShouldNotContain("???", customMessage: $"{collectionType} resolved to a placeholder name");
selectedName.ShouldNotStartWith("[unknown", customMessage: $"{collectionType} resolved to a placeholder");
selectedName.ShouldNotStartWith(
"[unsupported media type",
customMessage: $"{collectionType} fell through the media-item switch");
selectedName.ShouldBe(ExpectedName(collectionType), $"{collectionType} projected the wrong name");
}
private async Task SeedSelection(CollectionType collectionType)
@@ -216,7 +233,8 @@ public class RerunCollectionQueryHandlerTests : MediaCollectionHandlerTestBase
context.Songs.Add(new Song
{
Id = SelectedId,
SongMetadata = [new SongMetadata { Title = "Selected song", Artists = [] }]
SongMetadata =
[new SongMetadata { Title = "Selected song", Artists = ["Song artist"] }]
});
break;
case CollectionType.Image:
docs/decisions/records/api/selection-projection-include-chain.md
+12 -2
View File
@@ -41,8 +41,12 @@ Nine further media-item switches exist (four in `ProgramSchedules.Mapper`, five
`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 full graph, so PlaylistItem was
never broken in production — it shared the latent `RemoteStream` gap and now shares the fix.
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 `"???"`.
**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.
## Verification worth repeating
@@ -51,3 +55,9 @@ the list include chain failed all 13 types with "lost its selected id"; restorin
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.
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.