feat(489): support Jellyfin mixed-content libraries #493
No Assignees
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: timothy/ersatztv#493
Reference in New Issue
Block a user
Implements plan Tasks 1–5 and 7 for #489. Spec + plan landed in #490.
A Jellyfin library typed
mixedwas dropped byJellyfinApiClient.Project()'s_ => Nonewith no log line — which is why/data/musicnever reached ErsatzTV (#474) and why/data/standupneeded a local-library workaround. Both libraries are genuinely mixed (shows, concert movies and music videos together), so this models that rather than re-typing them at the source.What changed
LibraryMediaKind.Mixed = 8;Project()maps"mixed", absent, empty and whitespaceCollectionTypeonto it."music"and other unknown types still fall through toNone.SynchronizeJellyfinLibraryByIdHandlergains aMixedarm composing the three existing Jellyfin scanners. No new scanner, no DB migration.Mixed(a mixed library legitimately contains shows)._ => Unit.Defaultreplaced with_ => BaseError.New(...). That default returned success for an unhandled kind and stampedLastScanas though a scan had run — exactly how a missingMixedarm would have hidden.Mixedis rejected for local libraries at the API boundary.Segregation falls out of the model: a library is a place (one path ↔ one Jellyfin library ↔ one ErsatzTV library), so music/standup content cannot leak into
Movies/TV Shows.⚠️ Mergeable but NOT fully verifiable yet
No mixed library has been scanned end-to-end. The
Mixedarm invokesJellyfinMusicVideoLibraryScanner, which currently throwsArgumentNullExceptionon its first item (#488, being fixed in parallel). The unit tests prove the dispatch logic; they do not prove ingestion. Plan Task 6 — trace the music-video reconciliation, then live-E2E with a segregation assertion and a second scan for idempotency — is deliberately not in this PR and must run before #489 is closed.Green CI here should not be read as "mixed libraries work."
Verification
check:apiexit 0dotnet format --verify-no-changesNegative controls (every new assertion proven non-vacuous by reverting its fix): projection → 2 of 4 fail; dispatch arm → 3 of 5 fail; local-
Mixedvalidation → 1 fails. Pre-existing tests keep passing in all three.Adversarial review — verdict MERGEABLE, findings fixed
A cold review-only pass verified all four design claims independently against the code, and corrected one premise of mine:
Mixedwas accepted byPOST /api/v1/libraries/local, creating a library with no scanner that would fail and ERROR-log on every scheduler tick forever. The invariant was asserted in prose twice and enforced nowhere; omitting it from the SPA dropdown is not enforcement when machine clients are a stated/api/v1audience.CollectionTypedidn't match thenullpattern and fell through toNone, so on Jellyfin versions that serialize it that way the feature silently no-opped: no library, no log line — the identical symptom this issue exists to fix.IsLeftbut not thatScanCanceledreaches the caller's info-log branch. Folding it into an aggregateBaseErrorwould silently demote a user cancellation to an ERROR log (#410). Now pinned both ways.Correction to my own spec: I flagged
JellyfinMusicVideoLibraryScanner's reconciliation as an untraced cross-deletion risk. The reviewer read all 216 lines — it performs no reconciliation at all. It cannot cross-delete, but for the opposite reason to the one I assumed, and the flip side is that a music video removed from Jellyfin is never removed from ErsatzTV. Documented rather than left as false reassurance.Accepted limitations, recorded in
docs/decisions.mdrather than ignored: scan progress resets once per kind (the JellyfinScanLibrarysignature has noprogressMin/progressMax); and one permanently-failing kind forces the healthy two to rescan every interval — accepted because stampingLastScanon partial success would mask a broken kind.Rollout (after #488)
Re-type the live
Music VideosJellyfin library back tomixed(reverting tonight's provisionalmusicvideoschange), re-enable sync, scan. Local library 14 (Standup) then becomes retirable — its own issue.fixes #489
CI green @
9affd4b9— status, not a merge verdictAll jobs pass: Build & test (.NET), EF migration integrity (SQLite + MySql), Functional E2E, Formatting, API docs in sync, decisions.md append-only, CI image pin. (
Build & push imageskipped, as expected for a PR.)No
Review-verdict:line is being posted yet, on purpose. Two things are true and neither should be papered over:Mixedarm invokesJellyfinMusicVideoLibraryScanner, which is still broken until #488 lands. Green CI here proves the dispatch logic and the projection; it does not prove ingestion works.9affd4b9itself has not been independently reviewed. It contains the fixes the second review prescribed — which is precisely the class of commit the #242 lesson says to re-review, since a prescribed fix is still new code. Issuing a verdict against a head whose diff no reviewer has seen would defeat the point of the H10 convention.So the verdict should be issued against the head that will actually merge — after #488 lands, this branch rebases, and Task 6 runs.
Review chain so far
ec782d29ec782d29+4211db919affd4b99affd4b9Round 2 was worth running: it found that my own non-vacuity assertion was itself vacuous. The
ScanCancelednegative log check matched by string equality, but the regression it claimed to pin produces a different message — so it could never fail. Now a prefix match, and proven non-vacuous by neutralizing the early-return and watching the test fail (which it previously did not).It also confirmed the finding I most wanted checked: M1 is complete, not a half-fix. Every writer of
Library.MediaKindwas enumerated —UpdateLocalLibraryhas noMediaKindfield,MoveLocalLibraryPathnever writes it, andMixed = 8is new so no legacy row can hold it.One unrelated change in this PR, flagged rather than buried
LibrariesScreen.test.tsxfailed on run 889. That is not this branch's doing: the only web change here is one line of generatedv1.d.ts(types, erased at runtime), andLibrariesScreenis untouched. It passes 3/3 locally; CI runs the SPA suite ~8x slower (98s vs 12s) againstwaitFor's 1000ms default. I bumped those two waits per-test rather than raising the global default.Caveat stated honestly: this run passing does not prove the timeout was the cure — a flake can simply not recur. The types-only argument is the real evidence that the failure wasn't mine. If it recurs elsewhere, the per-test bump is the wrong layer and it needs its own issue.
My first hypothesis — that the docs-only PR #490 failed the same way — was wrong; that run's
Build & testsucceeded via the docs-only skip path. Recording that because it would have been a convenient and false explanation.9affd4b9fctocba6d7a156cba6d7a156to39966ec616Round-4 review (Low): 2 x 5s waits plus a CI-slowed render/findByText/ runPollTick prelude comes to ~13-14s against a 15s budget, so the generic vitest-timeout failure mode was made unlikely rather than eliminated. Took the reviewer's suggested 20s budget, which leaves real headroom and keeps waitFor -- not vitest -- as the thing that reports a failure. Also correcting a claim in the previous commit message: CollectionsScreen.test.tsx uses the options-object form it(name, {timeout}, fn), not the trailing-number form; only TrashScreen.test.tsx:172 matches literally. Both are test-level timeouts so the substance was right, but the citation was not. Refs #489Review-verdict: MERGEABLE @
bf48b64ad5Basis for the verdict
CI green overall on
bf48b64a— all 8 jobs pass (image pin, docs reminder, formatting, decisions.md append-only, Build & test .NET, API docs in sync, Functional E2E, EF migration integrity; image push correctly skipped for a PR). Rebased onto currentmain, which includes #488 and #494.Four independent cold review rounds, every one MERGEABLE, each scoped to code the previous round had not seen — per the #242 lesson that a fix commit is new code:
Mixedaccepted byPOST /api/v1/libraries/local, creating an unscannable library that ERROR-logs every scheduler tick; M3 — blank/whitespaceCollectionTypefell through toNone, silently no-oping the whole feature on some Jellyfin versions; L4, nit. All fixed.Mixedfor local create with no note that it 422s. Both fixed.waitFor {timeout:10000}was unreachable (vitest caps the test at 5s), degrading failure output to a generic timeout with no assertion diff — worse than the default it replaced. Fixed with the repo's established test-level-timeout pattern.it(…, 15000)survives a 7s sleep while the bare form dies at 5000ms). One Low on budget margin — taken, hence this head.Round 4 finding nothing substantive is the signal the loop converged. Notably, every defect the reviews found in my own work was in test code, not production code.
Task 6 — live E2E, complete
Full evidence on #489. Summary:
Mixed—"mixed"(marker file) andnull(no marker, which is what the Jellyfin UI produces today). Thenullcase exists on the operator's live server; without the M3 fix it would have been silently dropped, reproducing the exact bug this PR fixes.Standup(music-video arm receives an empty incoming set, movies present) left all 1437 music videos intact and added 146 movies to a single library. #494'sMusicVideo-join design confirmed empirically, not just by argument.Scope honesty
Deliberately Jellyfin-only — local mixed libraries are unsupported by design and
Mixedis rejected at the local-library API. Three accepted limitations are recorded indocs/decisions.mdrather than glossed: per-kind progress resets, one failing kind forcing the healthy kinds to rescan, andJellyfinMusicVideoLibraryScannerperforming no reconciliation (true when written; #494 has since changed this).Side findings filed rather than folded in: #497 (metadata updates dropped for existing music videos) and a reshaped #487.
All
## Done-whenboxes on #489 are ticked against evidence.