Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1058 lines
86 KiB
Markdown
1058 lines
86 KiB
Markdown
# Decisions — append-only log
|
||
|
||
Purpose: why the codebase does what it does, so agents don't "fix" an established convention or
|
||
relitigate a settled call. Append new entries at the bottom in date order (plus a TOC line in the
|
||
Index); **update this doc in the same PR that changes any fact below** (or that establishes a new
|
||
convention worth recording).
|
||
|
||
**Append-only, enforced (ersatztv#303 H9).** A commit or PR that *deletes or modifies* an existing
|
||
line is blocked by the Husky `commit-msg` hook and the CI `decisions-guard` job; insertions anywhere
|
||
are always allowed. The block is lifted only by the **`[decisions-edit]`** token in the commit
|
||
message, for the two legitimate reasons to touch history:
|
||
- **Fix a factual error** in a past entry.
|
||
- **Supersede a reversed decision** — add the new dated entry, then prepend a
|
||
`> **Superseded YYYY-MM by <new entry title>.**` banner to the old entry and tag its Index line
|
||
`(superseded)`. Keep the old entry — *why we changed our mind* is the point; never silently rewrite it.
|
||
|
||
**Consolidation** (prune/merge superseded entries, refresh the Index) is primarily a step in the
|
||
release checklist (`docs/ci-cd.md` → Versioning & releases), done at each release/milestone with
|
||
`[decisions-edit]` — not ad hoc mid-arc. A **size floor** backstops it between releases: the
|
||
`decisions-guard` CI job warns (non-blocking) once this file exceeds **1800 lines** — the point where
|
||
it no longer fits one default 2000-line agent Read — so append-only can't grow past what an agent can
|
||
read in a pass. The metric is the file's read cost (line count), not the entry count. Together these
|
||
keep append-only from accreting stale, contradictory, or unreadably-large history.
|
||
|
||
---
|
||
|
||
## Index
|
||
|
||
Decisions are split between the chronological log **in this file** and four **topic files** under
|
||
`docs/decisions/` — large same-topic clusters extracted at the v26.9.0 consolidation (full
|
||
rationale preserved). Check the relevant topic file below for its subject; otherwise scan the
|
||
in-file entries.
|
||
|
||
**Topic files:**
|
||
|
||
- [`decisions/optimistic-concurrency.md`](decisions/optimistic-concurrency.md) — ETag / If-Match / `Version` optimistic concurrency (#253, #259, #265, #269). Mechanics: `api-conventions.md` §7a–§7c.
|
||
- [`decisions/api-auth-security.md`](decisions/api-auth-security.md) — API/SPA auth & security posture (#197 bundles + #279 headers, #206, #283, #292, #295, #301, #319, #330).
|
||
- [`decisions/release-ci-governance.md`](decisions/release-ci-governance.md) — release / CI / merge governance (#303 H3/H4/H5/H6/H9/H10, #311, #314, #315, #335).
|
||
- [`decisions/spa-modularization.md`](decisions/spa-modularization.md) — App.tsx screen/shell extraction epic #243 (#244, #245, #247).
|
||
|
||
**In this file:**
|
||
|
||
- [2026-06 — REST API wraps existing MediatR handlers 1:1, no service layer](#2026-06--rest-api-wraps-existing-mediatr-handlers-11-no-service-layer)
|
||
- [2026-06 — UI rebuild is a React SPA (ChicoryTV) on the REST API, not a Blazor reskin](#2026-06--ui-rebuild-is-a-react-spa-chicorytv-on-the-rest-api-not-a-blazor-reskin)
|
||
- [2026-07 — Response DTOs live in `ErsatzTV.Core/Api`, file-scoped `#nullable enable`](#2026-07--response-dtos-live-in-ersatztvcoreapi-file-scoped-nullable-enable)
|
||
- [2026-07 — PUT-replace list endpoints derive `Index` from array order; alternate-schedules last row = catch-all default](#2026-07--put-replace-list-endpoints-derive-index-from-array-order-alternate-schedules-last-row--catch-all-default)
|
||
- [2026-07 — Templates editor in the SPA is a table, not Blazor's drag-calendar](#2026-07--templates-editor-in-the-spa-is-a-table-not-blazors-drag-calendar)
|
||
- [2026-07-07 — API artwork contract: rooted URLs produced server-side](#2026-07-07--api-artwork-contract-rooted-urls-produced-server-side)
|
||
- [2026-07-07 — Decode-style endpoints take a row id and look up server-side](#2026-07-07--decode-style-endpoints-take-a-row-id-and-look-up-server-side)
|
||
- [2026-07-07 — Season/episode/music-video drill-in via `parentId`, not new child-listing endpoints](#2026-07-07--seasonepisodemusic-video-drill-in-via-parentid-not-new-child-listing-endpoints)
|
||
- [2026-07-07 — Convention docs read at session start, updated in-PR](#2026-07-07--convention-docs-read-at-session-start-updated-in-pr)
|
||
- [2026-07-09 — Playback-troubleshooting completion feedback: poll status, no push channel](#2026-07-09--playback-troubleshooting-completion-feedback-poll-status-no-push-channel)
|
||
- [2026-07-09 — datetime-local instead of Chronic natural-language start parsing](#2026-07-09--datetime-local-instead-of-chronic-natural-language-start-parsing)
|
||
- [2026-07-09 — SPA gates Download Media Sample while a session is active](#2026-07-09--spa-gates-download-media-sample-while-a-session-is-active)
|
||
- [2026-07-09 — OpenAPI spec mirrors the runtime Newtonsoft serializer (#198)](#2026-07-09--openapi-spec-mirrors-the-runtime-newtonsoft-serializer-198)
|
||
- [2026-07-09 — YAML playout validator: paste-textarea instead of a server file path](#2026-07-09--yaml-playout-validator-paste-textarea-instead-of-a-server-file-path)
|
||
- [2026-07-09 — Channel numbers: prompt-driven sequential renumber instead of drag-to-reorder](#2026-07-09--channel-numbers-prompt-driven-sequential-renumber-instead-of-drag-to-reorder)
|
||
- [2026-07-09 — "Table, not calendar" convention also covers the deco-templates editor](#2026-07-09--table-not-calendar-convention-also-covers-the-deco-templates-editor)
|
||
- [2026-07-11 — Trash "See all" reuses library-browse paging; search stays capped per kind (#213)](#2026-07-11--trash-see-all-reuses-library-browse-paging-search-stays-capped-per-kind-213)
|
||
- [2026-07-11 — Logs page-size is a client-local preference, not a server ConfigElement](#2026-07-11--logs-page-size-is-a-client-local-preference-not-a-server-configelement)
|
||
- [2026-07-11 — Logs column sorting: allow-listed `sortField`/`sortDirection` on `GET /api/logs`](#2026-07-11--logs-column-sorting-allow-listed-sortfieldsortdirection-on-get-apilogs)
|
||
- [2026-07-09 — Per-playout "Schedule reset" button dropped; Reset uses the server-default build mode](#2026-07-09--per-playout-schedule-reset-button-dropped-reset-uses-the-server-default-build-mode)
|
||
- [2026-07-09 — Collection custom order: move up/down buttons, any-kind collections](#2026-07-09--collection-custom-order-move-updown-buttons-any-kind-collections)
|
||
- [2026-07-10 — Shared "Add to…" layer lives in `web/src/media/addTo/`; select-mode is an explicit toggle](#2026-07-10--shared-add-to-layer-lives-in-websrcmediaaddto-select-mode-is-an-explicit-toggle)
|
||
- [2026-07-10 — Schedule-item GET returns a flat, non-polymorphic DTO (`ScheduleItemResponseModel`)](#2026-07-10--schedule-item-get-returns-a-flat-non-polymorphic-dto-scheduleitemresponsemodel)
|
||
- [2026-07-10 — Playout API mutations return 409 while the build lock is held (#215)](#2026-07-10--playout-api-mutations-return-409-while-the-build-lock-is-held-215)
|
||
- [2026-07-11 — Schedules SPA editor: draft/explicit-Save over instant-persist; Copy includes multi/smart/rerun; shuffled-GET normalization preserved](#2026-07-11--schedules-spa-editor-draftexplicit-save-over-instant-persist-copy-includes-multismartrerun-shuffled-get-normalization-preserved)
|
||
- [2026-07-11 — Channel editor: bare-create entry point + external-logo mutual exclusion (#212)](#2026-07-11--channel-editor-bare-create-entry-point--external-logo-mutual-exclusion-212)
|
||
- [2026-07-11 — Queue state lives in the pinned Gitea tracker (#237), not in the handoff file](#2026-07-11--queue-state-lives-in-the-pinned-gitea-tracker-237-not-in-the-handoff-file)
|
||
- [2026-07-11 — EntityLocker: atomic flags + single-owner release discipline, no owner tokens (#231)](#2026-07-11--entitylocker-atomic-flags--single-owner-release-discipline-no-owner-tokens-231)
|
||
- [2026-07-11 — Media-source management REST write API + SPA (#202)](#2026-07-11--media-source-management-rest-write-api--spa-202)
|
||
- [2026-07-11 — Legacy→SPA redirect matcher: exact map + ordered segment-template patterns (#204)](#2026-07-11--legacyspa-redirect-matcher-exact-map--ordered-segment-template-patterns-204)
|
||
- [2026-07-11 — Pre-removal Blazor rollback tag `blazor-final` (#205)](#2026-07-11--pre-removal-blazor-rollback-tag-blazor-final-205)
|
||
- [2026-07-11 — Async-op API contract normalization + playout build observability + F9 scan endpoints (#235)](#2026-07-11--async-op-api-contract-normalization--playout-build-observability--f9-scan-endpoints-235)
|
||
- [2026-07-11 — Post-commit side effects run on `CancellationToken.None` (generalized from #251 to #254)](#2026-07-11--post-commit-side-effects-run-on-cancellationtokennone-generalized-from-251-to-254)
|
||
- [2026-07-12 — External-collections scans get an authoritative status surface (#271); the SPA timeout is retired](#2026-07-12--external-collections-scans-get-an-authoritative-status-surface-271-the-spa-timeout-is-retired)
|
||
- [2026-07-11 — Blazor Server UI removed (#91 phase b)](#2026-07-11--blazor-server-ui-removed-91-phase-b)
|
||
- [2026-07-12 — Live-E2E is a required step for API write-path handler changes (#303)](#2026-07-12--live-e2e-is-a-required-step-for-api-write-path-handler-changes-303)
|
||
- [2026-07-12 — TopBar primary-action button: wire creates, drop the rest (#238)](#2026-07-12--topbar-primary-action-button-wire-creates-drop-the-rest-238)
|
||
- [2026-07-13 — API versioning: the whole `/api` surface is mounted at `/api/v1`, additive-only after freeze (#286)](#2026-07-13--api-versioning-the-whole-api-surface-is-mounted-at-apiv1-additive-only-after-freeze-286)
|
||
- [2026-07-13 — Scheduling API hardening: null-name 500s, duplicate template items, unreachable 404 (#172)](#2026-07-13--scheduling-api-hardening-null-name-500s-duplicate-template-items-unreachable-404-172)
|
||
- [2026-07-16 — Functional-E2E CI harness: advisory curl-contract job over an app booted from source (#299)](#2026-07-16--functional-e2e-ci-harness-advisory-curl-contract-job-over-an-app-booted-from-source-299)
|
||
- [2026-07-16 — Optional advertised IPTV base URL (`iptv.base_url`) resolved centrally in the two generators (#340)](#2026-07-16--optional-advertised-iptv-base-url-iptvbase_url-resolved-centrally-in-the-two-generators-340)
|
||
- [2026-07-16 — Auto-tuning enumerates via EF, persists via SmartCollection; additive coexistence (#69)](#2026-07-16--auto-tuning-enumerates-via-ef-persists-via-smartcollection-additive-coexistence-69)
|
||
- [2026-07-16 — Per-playout reshuffle = scoped Reset build; seed surfaced (#71)](#2026-07-16--per-playout-reshuffle--scoped-reset-build-seed-surfaced-71)
|
||
|
||
---
|
||
|
||
## 2026-06 — REST API wraps existing MediatR handlers 1:1, no service layer
|
||
|
||
The REST API (#2, `docs/rest-api.md`) is thin controllers over the existing MediatR
|
||
Create/Update/Delete handlers — no new service/business-logic layer was introduced, since nearly
|
||
every handler already returns `Either<BaseError, T>`, which maps cleanly to HTTP status codes.
|
||
Latent handler bugs (missing existence checks, `KeyNotFoundException` risk, etc.) are fixed **at
|
||
the handler**, converting what would have 500'd into a proper 404/422 — not papered over in the
|
||
controller. Established across the #2a–#2e gap-issue PRs. Deep FK ids nested inside item-list
|
||
request bodies (e.g. a schedule item's `CollectionId`) are deliberately **not** existence-checked at
|
||
that depth, to avoid N+1 validation queries — precedent set by the schedules endpoints (#172); see
|
||
`docs/api-conventions.md` §3 for the up-to-date statement of this rule.
|
||
|
||
## 2026-06 — UI rebuild is a React SPA (ChicoryTV) on the REST API, not a Blazor reskin
|
||
|
||
#59 committed to a full SPA rebuild rather than reskinning Blazor Server pages. Blazor removal is
|
||
split into two phases under #91: **(a)** root-flip (SPA becomes `/`) + legacy-route redirects —
|
||
DONE, merged via PR #148 (`ErsatzTV/LegacyUiRedirects.cs`, `feat/91-cutover` → main). **(b)** full
|
||
Blazor removal — gated on every route having an SPA equivalent; tracked route-by-route in
|
||
`docs/blazor-route-parity.md`.
|
||
|
||
## 2026-07 — Response DTOs live in `ErsatzTV.Core/Api`, file-scoped `#nullable enable`
|
||
|
||
New REST response DTOs go in `ErsatzTV.Core/Api/<Domain>/*ResponseModel.cs` and mirror the shape of
|
||
the corresponding Application-layer ViewModel — controllers never expose VM types directly. Because
|
||
`ErsatzTV.Core.csproj` sets `<Nullable>disable</Nullable>` project-wide, any response-model file
|
||
with an optional member needs its own `#nullable enable` pragma at the top (most already have one).
|
||
`ErsatzTV.Application` has no nullable context at all — do not add `?` annotations to types living
|
||
there; that's a Core/Api-layer-only convention. Full detail: `docs/api-conventions.md` §2.
|
||
|
||
## 2026-07 — PUT-replace list endpoints derive `Index` from array order; alternate-schedules last row = catch-all default
|
||
|
||
For "replace the whole list" endpoints (PUT over a collection — schedule items, template items,
|
||
etc.), the item's `Index` is derived from its position in the request array, not from a
|
||
client-supplied index/order field — established by `ReplaceScheduleItemsRequest.ToCommand`
|
||
(`Items.Select((item, index) => item.ToReplaceCommand(index))`). Separately, `ProgramScheduleAlternate`
|
||
and `PlayoutTemplate` rows (both `IAlternateScheduleItem`) are evaluated in `Index` order,
|
||
first-match-wins; the convention is to place the least-conditional (or unconditional) row **last**
|
||
so it acts as the catch-all default. Established by the alternate-schedules work (PR #179,
|
||
`AlternateScheduleSelector.cs`).
|
||
|
||
## 2026-07 — Templates editor in the SPA is a table, not Blazor's drag-calendar
|
||
|
||
The legacy Blazor `TemplateEditor.razor` used a drag-and-drop day-grid calendar UI. The SPA
|
||
equivalent (`/app/templates/{id}`, PR #173) renders the same day/block assignment as a table
|
||
instead. This is an accepted, deliberate parity deviation — don't "fix" it to match Blazor's
|
||
interaction model without discussing it first.
|
||
|
||
## 2026-07-07 — API artwork contract: rooted URLs produced server-side
|
||
|
||
API response DTOs return artwork as rooted, directly-usable URLs (`/artwork/posters/...`,
|
||
`/artwork/thumbnails/...`, `/artwork/fanart/...`), plus passthrough for absolute `http(s)://` URLs
|
||
and Jellyfin/Emby proxy variants. Established by PR #181
|
||
(`ErsatzTV.Application/LibraryBrowse/Queries/GetLibraryBrowseItemsHandler.cs`, private `Artwork(...)`
|
||
helper — comment: *"Returns a rooted, directly-usable artwork URL for the SPA's `<img src>`... the
|
||
SPA [needs it pre-rooted]"*), then generalized into the reusable `ApiArtwork` helper
|
||
(`ErsatzTV.Core/Api/ApiArtwork.cs`, PR #183). Root cause: the SPA has no `<base href>`, unlike
|
||
Blazor, so relative artwork paths that worked for Blazor pages 404 in the SPA. Do **not** reuse the
|
||
Application-layer Mappers used by Blazor (e.g. `MediaCards`/`Television` mappers) for new API
|
||
DTOs — those still return old Blazor-convention relative paths; map from the domain/VM directly and
|
||
root the path via `ApiArtwork`.
|
||
|
||
## 2026-07-07 — Decode-style endpoints take a row id and look up server-side
|
||
|
||
Endpoints that decode/expand opaque stored state accept a database row id and resolve server-side,
|
||
rather than accepting client-supplied serialized state to decode. Established by
|
||
`GET /api/playouts/history/{id}` (`PlayoutController.GetHistoryDetails`, PR #182) — the row's raw
|
||
JSON (`Key`/`Details`) is decoded server-side into `PlayoutHistoryDetailsResponseModel`, the client
|
||
never round-trips the raw payload itself.
|
||
|
||
## 2026-07-07 — Season/episode/music-video drill-in via `parentId`, not new child-listing endpoints
|
||
|
||
Rather than adding dedicated child-listing endpoints per media kind (e.g. "list episodes of a
|
||
season"), the library-browse endpoint takes an optional `parentId` query param and the SPA drills
|
||
in by re-querying with it. Established across PRs #181/#183 (library-picker season drill-in, then
|
||
media-detail's season/episode/artist/music-video browsing). Avoids a combinatorial explosion of
|
||
per-kind child endpoints.
|
||
|
||
## 2026-07-07 — Convention docs read at session start, updated in-PR
|
||
|
||
`docs/api-conventions.md`, `docs/spa-conventions.md`, `docs/e2e-local.md`,
|
||
`docs/blazor-route-parity.md`, `docs/domain-model.md`, `docs/decisions.md`, and `docs/README.md`
|
||
are the standing reference set every ChicoryTV session should read before starting work, and each
|
||
one carries an explicit "update this doc in the same PR" rule rather than deferring doc updates to
|
||
a follow-up. These docs **replace per-session recon** — an agent reads the index
|
||
(`docs/README.md`) and the relevant convention doc instead of re-deriving conventions from the code
|
||
each time it starts API/SPA/E2E/parity work. A testing map and a generated-endpoint index are
|
||
tracked as still-to-come under #185. Drafting this doc set also surfaced a drift in
|
||
`ApiControllerSecurityTests.cs`'s hardcoded controller registry (several controllers under
|
||
`ErsatzTV/Controllers/Api/` are missing from it — see `docs/api-conventions.md` §6) — tracked as a
|
||
follow-up under #184 rather than fixed inline, since it's a pre-existing gap, not something this
|
||
doc-drafting pass caused.
|
||
|
||
## 2026-07-09 — Playback-troubleshooting completion feedback: poll status, no push channel
|
||
|
||
The SPA playback-troubleshooting screen (`PlaybackTroubleshootingScreen.tsx`, #145) reports FFmpeg
|
||
completion by **polling `GET /api/troubleshoot/playback/status` every ~2s** while a session is
|
||
running (plus one poll on mount so a session started elsewhere still gates Play), rather than a
|
||
server push. The status endpoint returns `{ state, exitCode, speed, logs }`; the screen captures the
|
||
running→completed/failed transition in local component state and surfaces a completion notice
|
||
(success on exit 0, warning otherwise) — the SPA equivalent of the Blazor page's MediatR
|
||
`ICourier`/`ISnackbar` `PlaybackTroubleshootingCompletedNotification`. Chosen over SignalR/SSE
|
||
because the SPA has **no push channel** and troubleshooting sessions are short and user-initiated, so
|
||
a lightweight poll (started on Play, stopped on settle/unmount) is simpler than standing up a new
|
||
real-time transport. Speed thresholds and the "(Speed: Nx)" badge colors are copied verbatim from the
|
||
Blazor `GetSpeedClass` (red <0.9, green >1.1, amber otherwise).
|
||
|
||
## 2026-07-09 — datetime-local instead of Chronic natural-language start parsing
|
||
|
||
The channel-mode "Date and Time" input in the SPA playback-troubleshooting screen uses a native
|
||
`<input type="datetime-local">`, a **deliberate deviation** from the Blazor page, which parsed a
|
||
free-text field with `Chronic.Core.Parser` (natural language like "yesterday at 8pm"). The SPA has no
|
||
Chronic dependency and a picker is unambiguous; the selected local datetime is sent to
|
||
`playback.m3u8` as an ISO-8601 `start` param via `new Date(value).toISOString()`, which the
|
||
controller binds to `DateTimeOffset?` exactly as the Blazor round-trip (`"o"`) format did.
|
||
|
||
## 2026-07-09 — SPA gates Download Media Sample while a session is active
|
||
|
||
Minor intentional deviation: the SPA playback-troubleshooting screen disables **Download Media
|
||
Sample** (alongside Download Results) while a troubleshooting session is starting/running; Blazor
|
||
only gated Download Results. Both downloads compete with the live transcode for I/O and the sample
|
||
archiver reads the same media file, so gating both during a session is strictly safer and costs
|
||
nothing (sessions are short).
|
||
|
||
## 2026-07-09 — OpenAPI spec mirrors the runtime Newtonsoft serializer (#198)
|
||
|
||
The generated OpenAPI document is made to follow the **runtime** JSON contract, not the reverse. Runtime
|
||
`/api/*` responses are serialized by Newtonsoft via `CustomContractResolver`/`CustomNamingStrategy`
|
||
(camelCase + a `FFmpegProfileId`→`ffmpegProfileId` special case + `[JsonProperty]` overrides such as
|
||
`ChannelResponseModel.FFmpegProfile`→`ffmpegProfile`), while `Microsoft.AspNetCore.OpenApi` generates the
|
||
spec from System.Text.Json metadata, whose camelCase drifted (`fFmpegProfileId`, `fFmpegProfile`). That
|
||
drift fed the SPA the wrong key. Rather than hand-patch the spec or change the wire format (breaking clients),
|
||
we added `NewtonsoftSchemaNamingTransformer` — an OpenAPI schema transformer registered on all three
|
||
documents that renames each schema property through the *same* Newtonsoft contract resolver the runtime uses,
|
||
so the spec matches the wire format by construction. A contract test
|
||
(`OpenApiSerializerContractTests`) serializes representative DTOs through the real runtime settings and pins
|
||
the spec property sets to them. Decision: **the wire format is the source of truth; the spec follows it via the
|
||
real contract resolver.** This also fixed a latent SPA bug (the channel-list "FFmpeg profile" column read
|
||
`fFmpegProfile` and always showed "Unassigned"). Issue #198.
|
||
|
||
## 2026-07-09 — YAML playout validator: paste-textarea instead of a server file path
|
||
|
||
The legacy Blazor YAML playout validator took a **server-side file path** (read directly off the
|
||
container's filesystem). The SPA's `YamlValidatorScreen.tsx` instead uses a paste `<textarea>` — a
|
||
deliberate deviation, not an oversight. The SPA runs entirely client-side against `/api/*` and has
|
||
no access to the server's filesystem, so a file-path field would either need a new
|
||
filesystem-browsing endpoint or silently fail; pasting the YAML directly is simpler and matches how
|
||
every other SPA editor already round-trips content through the API instead of the disk.
|
||
|
||
## 2026-07-09 — Channel numbers: prompt-driven sequential renumber instead of drag-to-reorder
|
||
|
||
The legacy Blazor channel list let you drag-and-drop rows to reorder channel numbers. The SPA
|
||
(`App.tsx`) instead offers a "Renumber" action that walks the list and asks for each channel's new
|
||
number via a sequence of native `prompt()` calls. Deliberate deviation: drag-to-reorder needs a
|
||
dedicated drag library and a bespoke reorder-persistence endpoint; a sequential prompt reuses the
|
||
existing per-channel update call and needs no new UI dependency. Revisit only if channel counts grow
|
||
large enough that prompt-per-channel becomes tedious.
|
||
|
||
## 2026-07-09 — "Table, not calendar" convention also covers the deco-templates editor
|
||
|
||
Extends the 2026-07 "Templates editor in the SPA is a table, not Blazor's drag-calendar" entry
|
||
above (not editing that entry — this generalizes it): the deco-templates editor
|
||
(`DecoTemplatesScreen.tsx`) follows the same convention, rendering its day/deco assignment as a
|
||
table rather than reproducing Blazor's drag-and-drop calendar grid. Same rationale, same
|
||
accepted-deviation status — don't "fix" either editor to match Blazor's interaction model without
|
||
discussing it first.
|
||
|
||
## 2026-07-11 — Trash "See all" reuses library-browse paging; search stays capped per kind (#213)
|
||
|
||
`GET /api/v1/search` still returns at most 100 items per media kind, which is the cheap first page for
|
||
the common case. For an overflowing kind, the SPA's "See all N …" action pages `GET
|
||
/api/v1/library/browse` with `query=state:FileNotFound`, `mediaType`, `pageNum`, and `pageSize=100`, then
|
||
appends the results client-side. This reuses the same `GetLibraryBrowseItems` query behind search,
|
||
adds no API surface, and only pays for follow-up requests when a kind exceeds the first-page cap.
|
||
|
||
## 2026-07-11 — Logs page-size is a client-local preference, not a server ConfigElement
|
||
|
||
The legacy Blazor Logs page persisted the user's chosen rows-per-page via
|
||
`ConfigElementKey.LogsPageSize` (`SaveConfigElementByKey`/`GetConfigElementByKey`), a
|
||
per-server-instance setting stored in the DB. `LogsScreen.tsx` instead persists it to
|
||
`window.localStorage` under `ctv-logs-page-size` (same wrapped-`Storage` pattern as
|
||
`designSystem.ts`'s theme preference: try/catch getter, validated against the known option set,
|
||
falls back to a default) and restores it on mount. Deliberate deviation: this is a per-browser UI
|
||
preference, not server/business state — no other client should see or be affected by it, so there
|
||
is no reason to round-trip it through the API and grow a new `/api/*` surface (or reuse the
|
||
generic config-element endpoints) just to store a page-size number. Follows the existing SPA
|
||
localStorage convention (`designSystem.ts` theme, `auth.ts` token) rather than introducing a new
|
||
persistence mechanism.
|
||
|
||
## 2026-07-11 — Logs column sorting: allow-listed `sortField`/`sortDirection` on `GET /api/logs`
|
||
|
||
Parity for `Logs.razor`'s `MudTableSortLabel` columns (Timestamp, Level — Message was never
|
||
sortable in Blazor either). `LogsController.GetLogs` adds `sortField` (`timestamp` | `level`,
|
||
default `timestamp`) and `sortDirection` (`asc` | `desc`, default `desc`) query params, normalized
|
||
server-side the same way `pageNum`/`pageSize` are clamped rather than rejected with a 422: an
|
||
unrecognized `sortField` silently falls back to `timestamp`, an unrecognized `sortDirection` falls
|
||
back to `desc` — the pre-existing default behavior (newest-first) is unreachable to break via a bad
|
||
query string. `LogsScreen.tsx` renders the two sortable headers as buttons with a chevron
|
||
indicating the active field/direction; clicking the active column toggles direction, clicking the
|
||
other column switches to it ascending.
|
||
|
||
## 2026-07-09 — Per-playout "Schedule reset" button dropped; Reset uses the server-default build mode
|
||
|
||
Blazor's playouts page had both a per-playout **Reset** and a separate **Schedule Reset** control
|
||
(setting the daily rebuild time). The SPA keeps Reset — `POST
|
||
/api/channels/{channelNumber}/playout/reset` with no `mode` param, so the server picks the same
|
||
default Blazor used (Classic → Refresh, everything else → Reset) — but drops the dedicated
|
||
"Schedule reset" button: the daily rebuild time is already editable through the playout's
|
||
Edit-details flow, so a second entry point would duplicate an existing capability. Deliberate
|
||
deviation, not a lost capability. Issue #210.
|
||
|
||
## 2026-07-09 — Collection custom order: move up/down buttons, any-kind collections
|
||
|
||
Blazor reordered collection items with SortableJS drag-and-drop and only enabled custom ordering
|
||
for movies-only collections. The SPA (`CollectionsScreen.tsx`) uses per-row **Move up / Move
|
||
down** buttons in an explicit reorder mode instead of drag (no new drag dependency; the mode loads
|
||
ALL items first because `PUT /api/collections/{id}/custom-order` replaces the whole order from
|
||
array position — submitting a partial page would scramble the rest), and does **not** replicate
|
||
the movies-only gate: the API and the playback-side `CustomOrderCollectionEnumerator` sort by
|
||
`CustomIndex` regardless of item kind, so the SPA offers reorder for any manual collection with
|
||
custom order enabled. Issue #211.
|
||
|
||
## 2026-07-10 — Shared "Add to…" layer lives in `web/src/media/addTo/`; select-mode is an explicit toggle
|
||
|
||
The media mutation surface (#208/#209) is built on one reusable component group,
|
||
`web/src/media/addTo/` (media-domain components, like `MediaPosterCard` — not generic
|
||
`components/`): `AddToCollectionDialog` (existing-collection Select + inline "(New collection)"
|
||
create, Blazor `AddToCollectionDialog.razor` parity), `AddToPlaylistDialog` (group → playlist
|
||
Selects, no inline create), `AddToScheduleDialog` (schedule Select; payload replicates Blazor's
|
||
`AddProgramScheduleItem.ForMediaItem` defaults — see `addTo/scheduleItem.ts`),
|
||
`SaveAsSmartCollectionDialog`, and `AddToMenu` (the drop-in popover for cards/detail pages via
|
||
`MediaPosterCard`'s `actions` slot). New screens wanting add-to affordances use this layer —
|
||
don't build screen-local pickers. Two deliberate deviations from Blazor, applied consistently on
|
||
the search and browse screens: **(1) multi-select is an explicit screen-level "Select" toggle**
|
||
(off = cards open, on = cards select) rather than Blazor's always-on corner-select, because
|
||
`MediaPosterCard`'s select handler takes over the card's single click gesture; **(2) the
|
||
per-card menu offers collection/playlist for a single item of any kind, plus schedule only for
|
||
shows/seasons/artists** — collection/playlist is a superset of Blazor's per-card collection-only
|
||
menu, while the schedule target is gated to exactly the kinds Blazor's
|
||
`AddProgramScheduleItem.ForMediaItem` call sites offer, because the server validator
|
||
(`ProgramScheduleItemCommandBase.CollectionTypeMustBeValid`) accepts only the
|
||
TelevisionShow/TelevisionSeason/Artist per-media-item CollectionTypes and 422s the rest.
|
||
"Add All" (query-wide) mirrors Blazor's two-step: materialize ids
|
||
via `GET /api/search/all-items`, then reuse the id-list add endpoints — no query-based add
|
||
command exists server-side. Issues #208/#209.
|
||
|
||
## 2026-07-10 — Schedule-item GET returns a flat, non-polymorphic DTO (`ScheduleItemResponseModel`)
|
||
|
||
`GET/POST/PUT /api/schedules/{id}/items` return `ScheduleItemResponseModel` /
|
||
`ScheduleItemsResponseModel` (`ErsatzTV.Core/Api/Scheduling/`), **not** the Application-layer
|
||
`ProgramScheduleItemViewModel` hierarchy (One/Flood/Multiple/Duration subtypes). The polymorphic VM
|
||
only described its base shape in OpenAPI, so the SPA couldn't see the subtype fields (issue #126).
|
||
The flat DTO promotes every subtype field to a nullable top-level member — `multipleMode`,
|
||
`multipleCount` (renamed from the VM's `Count`), `playoutDuration`, `tailMode`,
|
||
`discardToFillAttempts` — mapped by pattern-matching the concrete VM in
|
||
`ScheduleItemResponseMapper` (`ErsatzTV.Application/ProgramSchedules/`). Its **mutation fields are
|
||
named 1:1 with `ScheduleItemRequest`** so a GET maps losslessly back to a PUT/POST
|
||
(`ScheduleItemResponseRoundTripTests` is the release gate proving the fixed point). It also carries
|
||
picker-hydration fields the editor needs: `collectionName`/`smartCollectionName`/…/`playlistName`,
|
||
`playlistGroupId` (to preselect the playlist's group), per-filler names, `watermarks` /
|
||
`graphicsElements` as `NamedIdResponseModel` lists, the computed `name`, and `durationEstimate`.
|
||
`GetProgramScheduleItemsHandler.EnforceProperties` still rewrites StartType→Dynamic, Flood→One and
|
||
Playlist/Rerun→PlaybackOrder None when `ShuffleScheduleItems` is on — that lossy normalization is
|
||
deliberate and lives on the read side (documented + tested). New shared `NamedIdResponseModel`
|
||
(`ErsatzTV.Core/Api/`) is the generic `{id, name}` embed for API responses. Issues #126/#207/#212.
|
||
|
||
## 2026-07-10 — Playout API mutations return 409 while the build lock is held (#215)
|
||
|
||
Blazor disabled per-playout Reset/Erase/Delete/Edit while a `BuildPlayout` was in flight
|
||
(`EntityLocker.IsPlayoutLocked`, `Playouts.razor` + per-kind editors); the REST API had no
|
||
equivalent, so a client could race an in-flight build with a destructive `ExecuteDelete` and leave
|
||
a half-built playout. Adversarial-reviewer#18 promoted this to a #91-phase-(b) removal gate: after
|
||
Blazor is deleted the invariant would vanish entirely.
|
||
|
||
Decision: enforce the invariant **server-side** on the API rather than re-implementing a live push
|
||
channel. `PlayoutController` and `ChannelController` inject `IEntityLocker`; every id-keyed mutation
|
||
— `PUT /api/playouts/{id}`, `.../deco`, `.../alternate-schedules`, `.../templates`,
|
||
`POST .../erase-items`, `.../erase-items-and-history`, `DELETE /api/playouts/{id}`, and
|
||
`POST /api/channels/{channelNumber}/playout/reset` — checks `IsPlayoutLocked(id)` first and returns
|
||
**409 Conflict** (`ApiResults.ConflictProblem`, new shared helper mirroring `NotFoundProblem`) while
|
||
the build lock is held. The PUTs are gated too (not just the destructive ops): the target invariant
|
||
is "no mutation during a build", matching Blazor's edit-disable.
|
||
|
||
- **The guard is advisory check-then-act, not mutual exclusion** — same posture as Blazor's disabled
|
||
buttons. A `BuildPlayout` already sitting in the worker queue can take the lock a few milliseconds
|
||
after the check passes, so the original race is *narrowed*, not eliminated; consequences remain
|
||
self-healing (the next rebuild corrects a half-mutated playout). True prevention — having each
|
||
mutation acquire the playout lock for its duration — was deliberately not taken: `LockPlayout`
|
||
publishes `PlayoutUpdatedNotification` (UI churn per mutation) and would make mutations block
|
||
builds, a semantics change out of scope for restoring Blazor parity.
|
||
|
||
- **`reset-all` is deliberately NOT gated** — it stays 202. `ResetAllPlayoutsHandler` already
|
||
*silently skips* locked playouts, which matches Blazor and the handler semantics; a fire-and-forget
|
||
bulk enqueue always accepts.
|
||
- **SPA mirrors the lock via data, not a push channel** — `PlayoutListItemResponseModel` gains an
|
||
`IsLocked` bool (set from `IsPlayoutLocked` in the controller's list projection). The playouts
|
||
screen disables Reset/Erase/Erase-and-history/Delete for a locked row and shows a "Building…"
|
||
Badge; on a 409 from any mutation it surfaces the error and calls `query.refresh()` so the row
|
||
picks up the flag. No new polling was added (the existing 30s channel-state poll is unchanged).
|
||
|
||
Precedent for the 409 shape: `TraktController` (left as-is with its own private `ConflictProblem()`
|
||
to keep the diff small). Convention recorded in `api-conventions.md` §3a.
|
||
|
||
## 2026-07-11 — Schedules SPA editor: draft/explicit-Save over instant-persist; Copy includes multi/smart/rerun; shuffled-GET normalization preserved
|
||
|
||
The ChicoryTV schedules editor (`web/src/screens/SchedulesScreen.tsx` + `web/src/schedules/`,
|
||
issue #207) rebuilds the schedule-item lineup to full mutation parity with the legacy Blazor editor.
|
||
Three deliberate decisions:
|
||
|
||
**(a) Draft model with one explicit Save, replacing instant-persist.** All add/edit/copy/remove/
|
||
reorder mutate a **local draft list only**; a single **Save** issues one
|
||
`PUT /api/schedules/{id}/items` (the replace endpoint). This is intentional because that PUT is
|
||
**destructive server-side** — it deletes and recreates every item row (new ids) and triggers playout
|
||
rebuilds — so batching edits into one flush (vs. the old per-action POST/DELETE/PUT) minimizes churn
|
||
and gives the user a Discard/dirty affordance. On 422/network error the draft is kept and the error
|
||
surfaced; on success the draft is replaced with the server response. A **Discard** action and a dirty
|
||
guard (native `confirm()` on schedule-switch + in-app nav via `navigationGuard.ts`, plus a
|
||
`beforeunload` listener) protect the draft. See `docs/spa-conventions.md` §8. This retires the old
|
||
inline `ScheduleScreen` in `App.tsx` and its instant-persist add/delete/reorder tests.
|
||
|
||
**(b) Copy item deep-copies ALL source references, including multi/smart/rerun collections.** Blazor's
|
||
`CopyItem` omitted the multi-collection / smart-collection / rerun-collection references when
|
||
duplicating an item (copying only the plain collection/media-item/playlist refs) — a latent bug. The
|
||
SPA's `copyDraftItem` (`web/src/schedules/itemRules.ts`) copies every source field + display name, so
|
||
copying a MultiCollection/SmartCollection/Rerun item preserves its source. Deliberate deviation
|
||
fixing the Blazor omission.
|
||
|
||
**(c) The shuffled-schedule GET normalization (`EnforceProperties`) is preserved lossiness, matching
|
||
Blazor.** When a schedule has `ShuffleScheduleItems`, `GET .../items` still rewrites startType→Dynamic,
|
||
Flood→One, and Playlist/Rerun playbackOrder→None (and zeroes discardToFillAttempts for non-random
|
||
Duration items). The SPA does **not** fight this — it hides the Fixed start type and Flood playout
|
||
mode from the option lists (and disables the reorder arrows) for shuffled schedules, mirroring Blazor,
|
||
so a GET→edit→PUT round-trip stays consistent with the server's read-side normalization. Issue #207.
|
||
|
||
## 2026-07-11 — Channel editor: bare-create entry point + external-logo mutual exclusion (#212)
|
||
|
||
Two Blazor-parity decisions closing the channel-editor gaps (`ChannelEditor.razor` +
|
||
`ChannelEditViewModel`):
|
||
|
||
1. **Bare-channel create lives on the channels list, not a form-first route.** Blazor's
|
||
`/channels` (no `Id`) is a full add form; the SPA instead adds a "New blank channel" action next
|
||
to "Add Channel" on `ChannelsScreen` (`web/src/App.tsx`) that POSTs `CreateChannelRequest` with
|
||
Blazor's computed add-mode defaults (`(max existing int-parsed channel number) + 1`, `name: "New
|
||
Channel"`, `group: "ErsatzTV"`, `ffmpegProfileId` = `GET /api/settings/ffmpeg`'s
|
||
`defaultFFmpegProfileId`, `streamingMode: "TransportStreamHybrid"`, `isEnabled`/`showInEpg: true`,
|
||
every other field at its C# `default(T)` — see `ChannelEditor.razor`'s `else` branch for the
|
||
source of truth) directly, then navigates to `/app/edit-channel/{id}` for the rest of the fields.
|
||
This is a deliberate equivalent, not a parity gap: it reuses the existing full editor instead of
|
||
duplicating its ~20 fields into a second form. "Add Channel" (`/app/new-channel`, the
|
||
library-to-lineup `ChannelBuilder` flow) is unrelated and untouched.
|
||
2. **External logo URL wins over an uploaded logo, mirroring
|
||
`ChannelEditViewModel.ToUpdate/ToCreate`.** The channel's logo is `ArtworkContentTypeModel {
|
||
path, contentType, isExternalUrl }`; on hydration, `isExternalUrl: true` populates a separate
|
||
"External logo URL" field and the uploaded-logo draft state is treated as empty (`EMPTY_LOGO =
|
||
{ path: '', contentType: '' }` in `ChannelEditScreen.tsx`) so the two never disagree. On submit,
|
||
a non-blank URL always wins: `logo: { path: url, contentType: '' }`, exactly matching
|
||
`ExternalLogoUrl`'s precedence in the C# view model. Uploading a file clears the URL field (the
|
||
last-set field wins, Blazor parity via `UploadLogo`'s `_model.ExternalLogoUrl = null`). The URL
|
||
is validated as http(s) client-side before save is enabled (Blazor has no equivalent validation;
|
||
added because the field is free text with no server-side format check surfaced to the SPA).
|
||
|
||
Also landed with #212: `preferredAudioLanguageCode`/`preferredSubtitleLanguageCode`,
|
||
`musicVideoCreditsTemplate`, and `streamSelector` moved from free-text `Input`s to `Select`s fed by
|
||
`GET /api/languages` / `/api/channels/music-video-credits-templates` /
|
||
`/api/channels/stream-selectors`. Each keeps the channel's currently-stored value selectable even if
|
||
it's absent from the reference list (`optionsKeepingCurrent` in `ChannelEditScreen.tsx`) so loading
|
||
an existing channel never silently changes the value out from under an unmanaged language code or a
|
||
template/selector file removed from disk since save.
|
||
|
||
## 2026-07-11 — Queue state lives in the pinned Gitea tracker (#237), not in the handoff file
|
||
|
||
With multiple sessions/agents working the repo in parallel, the old protocol — every session
|
||
wholesale-rewrites `docs/handoffs/chicorytv-issue-queue.md` on main (session state + queue +
|
||
next-session prompt) — became a last-writer-wins race. New protocol: **volatile queue state
|
||
moved to Gitea**, which is concurrency-safe by construction. Pinned tracker issue **#237**
|
||
holds the goal + ordered arc in its body (edited rarely, only on arc changes, re-read before
|
||
edit) and an append-only session-comment log (fixed template: Closed / Filed / Triage /
|
||
Arc change / Recommended next). Milestone `Blazor removal (#91 phase b)` + the `review` and
|
||
`in-progress` labels are the machine-queryable view. Sessions **claim** an issue before working
|
||
it (`in-progress` label + claim comment; the tiny read→claim race window is accepted, later
|
||
claimant backs off; stale claims — no commits/comments ~48h — may be taken over with a comment).
|
||
Every new issue gets an explicit end-of-session triage verdict — gate-blocker (milestone + arc
|
||
slot) or backlog (label only) — so review findings adjust the queue only through that step and
|
||
the arc doesn't drift. The handoff file keeps only the **static kickoff prompt** and the
|
||
**append-only Lessons lore** (per-session prompts are gone; task context lives in issue bodies).
|
||
|
||
## 2026-07-11 — EntityLocker: atomic flags + single-owner release discipline, no owner tokens (#231)
|
||
|
||
`EntityLocker` (process-wide singleton, `ErsatzTV.Infrastructure/Locking/EntityLocker.cs`) is the
|
||
advisory "operation on entity X is in progress" mutex layer. Adversarial review (#20/F5, →#231)
|
||
found three defects: six plain-`bool` flags with a non-atomic check-then-set (two threads could both
|
||
acquire and both return `true`), tokenless `Unlock*` letting any caller release another owner's lock
|
||
(e.g. `BuildPlayoutHandler` ignored `LockPlayout`'s return then unconditionally unlocked in
|
||
`finally`), and one batch taking a single `LockLibrary` released after the *first* of two enqueued
|
||
scan units (so the second ran unlocked).
|
||
|
||
**Model chosen: tokenless atomic flag + documented single-owner release discipline.** The six bools
|
||
became `int`s guarded by `Interlocked.CompareExchange` (0/1), so `Lock*`'s `bool` return is now a
|
||
reliable "this call won the transition" signal and the change event fires exactly once per
|
||
transition. The contract (XML-doc'd on `IEntityLocker`): a `true` from `Lock*` confers ownership of
|
||
exactly one release — performed either in the acquiring scope (`finally`, gated on the captured
|
||
bool) or by the single designated releaser the acquirer hands off to (the consumer of the message
|
||
enqueued while holding the lock, with a compensating unlock if the enqueue throws — the established
|
||
`TraktController.EnqueueWithTraktLock` / scheduler `unlock: last` tail pattern). Callers must never
|
||
release a lock they did not acquire. `Unlock*` on an already-unlocked slot returns `false`, fires no
|
||
event, and logs a Warning — the loud tripwire for double-release bugs; it does not throw or
|
||
`Debug.Assert`, because an advisory flag must stay safe to release in `finally` paths. The three
|
||
`ConcurrentDictionary`-backed kinds (Library/Playout/RemoteMediaSource) were already atomic
|
||
(`TryAdd`/`TryRemove`) and kept their semantics (the redundant `ContainsKey` pre-checks were dropped
|
||
as tidy-up); the interface signature is unchanged across its ~40 call sites.
|
||
|
||
**Rejected:** owner tokens/leases — the acquirer and releaser for Library/Trakt/Plex/Collections
|
||
locks are different code correlated only by entity id across in-memory `Channel<T>` queues, so a
|
||
token would have to travel inside ~8 background-request message types for a defect that discipline
|
||
plus the now-trustworthy atomic return value already prevents. Counted/reentrant locks — wrong
|
||
semantics: these are exclusive in-progress flags; two holders is the failure mode, not a feature.
|
||
Call-site fixes this model prescribes land separately: #232 (scan lifecycle — enqueue only after a
|
||
successful lock, compensating unlock on enqueue failure, one release per acquisition in batches) and
|
||
#234 (BuildPlayout/subtitles gate their `finally` unlock on the captured acquire result).
|
||
|
||
## 2026-07-11 — Media-source management REST write API + SPA (#202)
|
||
|
||
Replaced the Blazor `/media/sources/{local,plex,jellyfin,emby}/...` pages (14 routes) with SPA
|
||
screens under `/app/libraries/*` over new write controllers (`LocalLibrariesController`,
|
||
`Plex|Jellyfin|EmbyMediaSourcesController`), wrapping existing MediatR commands 1:1 (no new
|
||
commands, no DB migration). Full design + adversarial-review reconciliation:
|
||
`docs/handoffs/` session record and issue #202. The design surfaced and fixed several pre-existing
|
||
Application/Infrastructure bugs newly reachable from a programmatic client; each is recorded here
|
||
because it changes documented behavior, not just adds a route.
|
||
|
||
**Secure `apiKey` contract (Jellyfin/Emby connection).** The connection GET
|
||
(`RemoteConnectionResponseModel`) returns `{ address, hasApiKey }` — the stored key **never**
|
||
leaves the server, closing a leak where the old design would have served the raw key from an
|
||
unauthenticated GET under any-origin CORS. On the connection PUT, a blank/omitted `apiKey` means
|
||
*retain the existing key*; a non-blank value sets a new one; the key is **required on first
|
||
connect** (no existing secret) → 422. Rationale: GETs aren't behind `X-Api-Key`
|
||
(`ApiKeyAuthorizationFilter` only guards mutating verbs), so a secret-bearing GET is a real
|
||
exposure regardless of how obscure the route is. **Stated explicitly as an input to #197** (the
|
||
planned read-side-auth review for secret-bearing GETs) — #197 should treat "does any GET return a
|
||
credential" as one of its checks, not just this one instance.
|
||
|
||
**Three list-replace identity contracts, not one uniform one.** An earlier draft assumed a single
|
||
"`Id<1`=add / missing=delete / id-preserved" contract across all three PUT-replace families; source
|
||
inspection proved that false for two of them:
|
||
- *Remote library sync preferences* (`PUT .../{id}/libraries`) — the command carries no source id
|
||
and the handler toggles only the ids present in the body; a row **absent** from the request is
|
||
left untouched, not deleted (libraries are sync-discovered, never created via this PUT, so there
|
||
are no `Id=0` adds either). The controller validates the submitted id set against
|
||
`Get{Family}LibrariesBySourceId(id)` (422 on any id not owned by the route's source — closes a
|
||
cross-source hole). Identity is **not stable across a disable**: `Disable{Family}LibrarySync`
|
||
removes and re-adds the row with a fresh id, so the SPA keys its draft to `(name, mediaKind)`,
|
||
never to `Id`, and refetches after every save (the PUT returns the reloaded list).
|
||
- *Path replacements* (`PUT .../{id}/path-replacements`) — id-based (existing `Id`=update,
|
||
`Id<1`=add, absent=delete) as documented, **but** the repo UPDATE SQL had no source-id predicate
|
||
(`WHERE Id = @id`, no `AND {Family}MediaSourceId = @id`), so a PUT to source A could silently
|
||
overwrite source B's row with the same numeric id. Fixed with a handler-level ownership guard
|
||
(reject any incoming positive id not in this source's current set → 422, no partial mutation)
|
||
**and** the repo SQL predicate itself (defense-in-depth for any other caller of that repo method).
|
||
- *Local library paths* (`PUT /api/libraries/local/{id}`) — identity is the **normalized path
|
||
string** (full path, trailing-separator/case-insensitive), not `Id`; `Id` in the request is
|
||
advisory. Renaming a path is delete-old+add-new under the hood (its `LibraryPath.Id` changes).
|
||
Kept as-is (matches the entrenched, tested Blazor behavior and how the SPA edits by value); not
|
||
rewritten to id-based identity, which would be a bigger, riskier change out of #202's scope.
|
||
|
||
**Plex pin-flow as REST: poll until the lock releases, exception-safe non-handoff unlock.** The
|
||
SPA polls `GET /api/media-sources/plex` rather than a per-pin status resource (no pin-addressable
|
||
server state exists to expose; SSE/push was already rejected, 2026-07-09). Polling contract is
|
||
`isLocked && !isAuthorized` = waiting on the user; `isLocked && isAuthorized` = finalizing
|
||
(discovering servers — **do not** stop here, the server list is still empty);
|
||
`!isLocked && isAuthorized` = success; `!isLocked && !isAuthorized` = timed out/abandoned. This
|
||
required fixing a **latent lock-leak bug**: `TryCompletePlexPinFlowHandler` threw
|
||
`OperationCanceledException` on its 2-minute timeout instead of returning `false`, and nothing
|
||
unlocked on that path — an abandoned sign-in wedged the Plex lock until restart or manual sign-out.
|
||
Fix releases `UnlockPlex()` on the timeout-throw, a poll-exception, and an enqueue-exception — but
|
||
**deliberately not** in an unconditional `finally`: on success the lock is handed off to
|
||
`SynchronizePlexMediaSources`, the sole releaser after server discovery; a blanket `finally` would
|
||
double-release and release *before* discovery completes, re-opening the same race the fix closes.
|
||
This is the same non-owner-token discipline as the #231 `EntityLocker` model (2026-07-11 entry
|
||
above), applied to the pin-flow's handoff-vs-terminal distinction specifically.
|
||
|
||
**404 comes from the controller pre-check, not the handler.** `Apply`/`ToEitherAsync` both
|
||
`.Join()` errors, which flattens any `NotFoundError` inside a joined `Validation` down to a plain
|
||
422. So every id-taking endpoint's real 404 is a controller-side pre-check
|
||
(`Get...ById(id)`-is-`None` → `ApiResults.NotFoundProblem`, the `TemplateController.DeleteGroup`
|
||
pattern), not a handler-level conversion — converting the joined validators to `NotFoundError`
|
||
would be dead code, since the join discards the distinction anyway. This is check-then-act (a
|
||
delete racing between the pre-check and the command falls through to the handler's own 422, not a
|
||
404); accepted and tested against the actual runtime error type rather than a hoped-for handler 404.
|
||
|
||
**"Scan All" dropped, not implemented.** The disabled SPA header button on the libraries hub was
|
||
speculative UI with **no Blazor equivalent** (`Libraries.razor` only ever supported per-library
|
||
scan). Removed rather than backed with a new bulk-scan endpoint; per-library scan (#232) and the
|
||
new per-source refresh-libraries endpoints (P8/J9/E9) cover the real capability set.
|
||
|
||
**App-owned popstate for guarded sub-path routes.** `LocalLibraryEditScreen` and the other
|
||
`/app/libraries/*` editors are the first screens to both register a dirty-navigation guard *and*
|
||
track their own sub-path pathname — the combination `spa-conventions.md` §8 had flagged as
|
||
unvalidated. React commits child passive effects before parent ones, so a sub-path wrapper that
|
||
self-registers `popstate` would fire (and switch sub-screen) *before* App's guard-restore listener
|
||
could veto. Resolution: App owns `popstate` centrally for the `libraries` route and only pushes an
|
||
approved sub-path down to the wrapper (which no longer self-registers `popstate`); on a vetoed pop
|
||
App re-pushes the pre-pop URL and the wrapper never sees the rejected path. This is scoped to the
|
||
`libraries` route only (gated on `activeRoute === 'libraries'`) so unguarded sub-path routes
|
||
(Playouts, Media) stay byte-identical. See `spa-conventions.md` §2/§8 for the updated exemplar
|
||
list and the resolved caveat text.
|
||
|
||
## 2026-07-11 — Legacy→SPA redirect matcher: exact map + ordered segment-template patterns (#204)
|
||
|
||
`LegacyUiRedirects.TryGetRedirect` grew from a single exact-path dictionary to a **two-tier matcher**
|
||
behind the unchanged `(PathString, out string)` signature. **Tier 1** is the existing
|
||
`OrdinalIgnoreCase` `Map` (now 52 entries — the parameterless (A)/(B) routes plus the (E-base) browse
|
||
roots whose *targets* carry `?kind=…`). **Tier 2** is an ordered `IReadOnlyList<PatternRule>` of 36
|
||
segment-template rules ((C)/(C2)/(D)/(E-page)), consulted only on a Tier-1 miss; declaration order is
|
||
match order (first-match-wins).
|
||
|
||
Template tokens are minimal: `{id}` matches a **strict positive integer**
|
||
(`int.TryParse(seg, NumberStyles.None, InvariantCulture, out id) && id > 0` — rejects signs,
|
||
whitespace, separators, `0`, negatives, and overflow like `999999999999`; the raw segment text,
|
||
e.g. `007`, is substituted, not re-formatted); `{any}` matches any non-empty segment and is dropped
|
||
(only `/playouts/add/{any}`); everything else is a literal compared `OrdinalIgnoreCase`. The request
|
||
path is split with `StringSplitOptions.None` and **empty segments are rejected** (load-bearing: so
|
||
`/channels//5` cannot match `/channels/{id}`); templates themselves use `RemoveEmptyEntries`. The
|
||
existing single-trailing-slash normalization runs before both tiers, so `/channels/5/` matches.
|
||
|
||
The set is **collision-free by construction** — exact-before-pattern plus strict numeric `{id}` means
|
||
no two tiers/rules can match the same path. **Guard invariant** (comment + `Map`-keys meta-test): no
|
||
Tier-1 key or Tier-2 template may begin with `/api`, `/artwork`, `/docs`, `/openapi`, `/iptv`, `/app`,
|
||
or `/media/sources`; rules are always full, specific templates — **never prefix wildcards** (a bare
|
||
`/media/{any}` rule is forbidden). The blazor branch does not prefix-guard `/api|/artwork|/docs|
|
||
/openapi`, so the matcher's specificity is part of their protection.
|
||
|
||
**Query-string merge**: the incoming request query is now merged into the target via a new public
|
||
`AppendQueryString(target, QueryString)` helper (one-line Startup change:
|
||
`context.Request.PathBase + LegacyUiRedirects.AppendQueryString(target, context.Request.QueryString)`).
|
||
A target that already carries `?` (the `?kind=…` browse roots) is `&`-joined instead of producing a
|
||
malformed double `?`; plain targets keep verbatim-append behavior byte-for-byte. A duplicated key
|
||
after a merge (`?kind=movies` + incoming `?kind=shows`) is first-wins in the SPA
|
||
(`URLSearchParams.get` returns the first value) — acceptable. Extracting the merge into
|
||
`LegacyUiRedirects` keeps it unit-testable without a TestServer while preserving the PathBase
|
||
re-application invariant the Startup source-text test protects.
|
||
|
||
**Rejected**: regex pairs (harder to audit for the `/api`/`/artwork` greediness invariant, noisier
|
||
tests, no benefit — every parameterized route here is "fixed segments + one variable segment");
|
||
ASP.NET `TemplateMatcher`/`RouteMatcher` (pulls routing machinery into a static helper for 36 rules);
|
||
a single unified rule list (loses the O(1) dictionary hit for the ~52 exact routes that dominate real
|
||
traffic). No `/api` change, no OpenAPI regen, no SPA change.
|
||
|
||
## 2026-07-11 — Pre-removal Blazor rollback tag `blazor-final` (#205)
|
||
|
||
Removal-gate item #205: the removal PR deletes both the Blazor reference implementation and the
|
||
`/system/health` escape hatch, so a post-deletion parity gap would otherwise be an archaeology exercise
|
||
(guessing which release tag still matches `main` minus Blazor). Decision + procedure, to run as the **first
|
||
action of the Step 2 deletion PR merge** (not before — `main` moves until then):
|
||
|
||
1. On the `main` commit **immediately preceding** the removal merge (the last commit that still contains
|
||
`ErsatzTV/Pages/**`), cut an annotated tag and push it:
|
||
`git tag -a blazor-final -m "Last commit with the legacy Blazor Server UI (pre-#91-phase-b removal)"`
|
||
then `git push origin blazor-final`. The tag name is **`blazor-final`** (not `v*`) so it does **not**
|
||
trigger the `v*` prod-release build in `.gitea/workflows/docker-build.yml`.
|
||
2. **Restore path** (if a gap surfaces post-removal): `git checkout blazor-final` → `docker build -f
|
||
docker/Dockerfile -t ersatztv:blazor-final .` → pin the **test** container to that image while the gap is
|
||
fixed forward on `main`. Alternatively `git revert` the single deletion merge commit (keep the deletion as
|
||
one squash/merge commit specifically to make this a one-liner).
|
||
3. Document the tag + restore path in the removal PR body; update this entry with the tag's commit sha when
|
||
cut.
|
||
|
||
Not cut this session — `main` still carries Blazor and will advance before the removal PR.
|
||
|
||
## 2026-07-11 — Async-op API contract normalization + playout build observability + F9 scan endpoints (#235)
|
||
|
||
Reviewer#20 F7/F8/F9. Normalizes the queue-triggering `/api/*` endpoints onto one contract, closes the two
|
||
F9 `Libraries.razor` parity gaps, and hardens the Trakt batch-lock lifecycle. Much of the F8 surface was
|
||
**already normalized** by #232 (library scan → `QueueLibraryScanResult` 202/404/409/422) and #215 (per-id
|
||
playout mutations + reset → 409 lock guard) — this issue finished the remaining outliers.
|
||
|
||
**Normalized async-op contract** (queue-triggering endpoints): **202 Accepted** = work queued; **404
|
||
ProblemDetails** = entity missing (controller pre-check); **409 ProblemDetails** = lock held (the running
|
||
job, or a mutation racing it — §3a/§3b); **422 ProblemDetails** = domain precondition (sync disabled /
|
||
unsupported / start failed). Trakt was the reference implementation. Changes made:
|
||
- `MaintenanceController.EmptyTrash` — error path **500 text/plain → 404/422 ProblemDetails** (`ToErrorResult`).
|
||
- `MaintenanceController.CleanArtwork` — silent **200 → 202** (fire-and-forget enqueue). No SPA consumer.
|
||
- `LibrariesController.ScanShow` — conflated **400 `{error}` → 202/404/409/422** via a new
|
||
`QueueShowScanResult` enum (6 outcomes incl. an honest `ScanFailed`→422, distinct from `Unsupported`).
|
||
- `ChannelController.ResetPlayout` — **200 → 202** (queue-triggering; 404/409 unchanged).
|
||
- `PlayoutController.ResetAll` — **202 (no body) → 202 + `ResetAllPlayoutsResponseModel`** reporting
|
||
`queuedPlayoutIds` / `skippedLocked` / `skippedUnsupported` (replaces the silent skip; still 202, still
|
||
skips locked/ExternalJson by design per §3a — now it *reports* what it skipped).
|
||
- `TroubleshootController.TroubleshootPlayback` — bare body-less `NotFound()` → **404/422 ProblemDetails**
|
||
with distinguishing detail. **Status codes the SPA HLS player depends on were preserved** — verified
|
||
`HlsPlayer.tsx` never branches on this endpoint's status (playback state comes from the separate
|
||
`/api/troubleshoot/playback/status` poll); only the error *body* was enriched.
|
||
|
||
**Playout build observability**: the list endpoint (`GET /api/playouts`) already stamped `isLocked` +
|
||
`BuildStatus` on `PlayoutListItemResponseModel` (#215); this issue adds **`isLocked` to the single-playout
|
||
`GET /api/playouts/{id}`** (`PlayoutResponseModel`), so the detail poll surface carries the §3a lock flag
|
||
too. No dedicated `GET /api/playouts/{id}/status` push channel was added — the flag on the existing GETs is
|
||
the HTTP-observable substitute for Blazor's live lock event, matching the `GET /api/trakt/status` precedent.
|
||
|
||
**F9 parity endpoints** (the `Libraries.razor` deletion gate — #202 did NOT close these):
|
||
- **Deep scan**: `POST /api/libraries/{id}/scan` gains `?deep=false`, threaded through
|
||
`QueueLibraryScanByLibraryId(LibraryId, DeepScan=false)` into `ForceSynchronize{Plex,Jellyfin,Emby}LibraryById(id, deep)`
|
||
(was hardcoded `false`). Non-breaking: existing callers omit it.
|
||
- **External-collections scan**: new `POST /api/media-sources/{plex|jellyfin|emby}/{id}/scan-collections?deep=false`
|
||
on the three #202 media-source controllers, dispatching `Synchronize{X}Collections(id, ForceScan:true, deep)`.
|
||
Each pre-checks source existence (404), acquires the per-source **collections** lock (`Lock{X}Collections()` —
|
||
the lock *is* the running scan, so a false = **409**), then enqueues and returns 202; the controller
|
||
compensating-unlocks in a `catch` if the enqueue throws (§3b), and `ScannerService` releases in its `finally`.
|
||
Thin SPA clients shipped (`scanLibrary(id, deep)`, `scanCollections`); **the SPA deep-scan / collections
|
||
buttons are the removal PR's remaining parity work** (parity doc §5).
|
||
|
||
**F7 Trakt batch-lock leak fix**: the global Trakt lock was released only when the *terminal* batch message
|
||
(`Unlock: true`) was processed; a `WorkerService` shutdown/cancellation before that message leaked the lock
|
||
permanently (subsequent Trakt ops 409 until restart — same class as #231/#233/#234). Fix: `WorkerService`
|
||
now releases the Trakt lock in a `finally` on read-loop exit if still held. Non-vacuous regression test proven
|
||
against an inverted-condition control.
|
||
|
||
**Accepted-by-design** (per the issue's decision-record ask): the worker's channels are **unbounded** and
|
||
there is **no shutdown drain** — messages still queued at process exit are dropped. This is acceptable because
|
||
the entity locks are **in-memory singletons that die with the process**, so a dropped message can't strand a
|
||
lock across restarts (the F7 `finally` covers the *within-process* shutdown-break leak, which is the only way
|
||
a lock outlives its batch while the process keeps running). Adding a bounded-channel backpressure / graceful
|
||
drain is out of scope and would not fix a correctness bug.
|
||
|
||
## 2026-07-11 — Post-commit side effects run on `CancellationToken.None` (generalized from #251 to #254)
|
||
|
||
Audit #22 (adversarial-reviewer) found ~20 command handlers threading the request `cancellationToken`
|
||
into work that runs **after** `SaveChangesAsync` commits — the post-commit `WriteAsync` enqueue that
|
||
rebuilds/refreshes the affected entity, `mediator.Publish`, search-index reindex, cache refresh. A late
|
||
HTTP-client disconnect cancels that token, so the *already-committed* mutation throws on the way out
|
||
**and silently drops its side effect** (the playout rebuild is never queued → the persisted edit never
|
||
takes visible effect until a manual Reset). #251 fixed this for the deco handlers; #254 generalizes the
|
||
policy across the codebase.
|
||
|
||
**Decision.** Once a mutation has committed, the *entire* compensating side effect — enqueues, publishes,
|
||
reindexes, cache refreshes, and any post-commit lookup that **gates** one of those enqueues — runs on
|
||
`CancellationToken.None`. The commit is the point of no return; past it the side effect must not be
|
||
half-abortable. Full convention + the two boundaries in `docs/api-conventions.md` §7b.
|
||
|
||
**Scope of the #254 sweep (this PR).** Swept the single-`SaveChanges` handlers under `MediaCollections/`,
|
||
`ProgramSchedules/`, `Playouts/`, `Channels/` (20 handlers). Deliberately **excluded**:
|
||
- **`BuildPlayoutHandler`** — a background/worker handler; its token is the worker shutdown token, not a
|
||
client-disconnect token, so its downstream enqueues *correctly* honor cancellation.
|
||
- **`UpdateFFmpegSettingsHandler` + the two `Configuration/` settings handlers** — they commit via several
|
||
sequential `IConfigElementRepository.Upsert` calls with an interleaved enqueue; "when is it committed"
|
||
is a partial-commit-under-cancellation question broader than the clean single-`SaveChanges` F4 pattern.
|
||
Left for a separate follow-up.
|
||
- **Response-projection reloads** (`ReplaceProgramScheduleItemsHandler` / `AddProgramScheduleItemHandler`
|
||
post-commit graph reload that builds the *returned* view model) keep the request token — a cancelled
|
||
response after a durable commit + `None`-enqueue loses nothing.
|
||
- Handlers a no-token `WriteAsync()` already makes behaviorally correct (`default` == `None`) were left
|
||
alone (explicit-`None` there is cosmetic).
|
||
|
||
Also folded in the two other #254 items on the same handlers: the channel-guide `{number}.xml` delete in
|
||
`DeleteChannelHandler`/`DeletePlayoutHandler` now routes through `IFileSystem.File.Delete` (observable
|
||
under `MockFileSystem`) **before** the commit (a post-commit delete orphans the xml on a crash; the guide
|
||
xml is regenerable on demand, so a pre-commit delete is the safe ordering), and
|
||
`ReplacePlayoutAlternateScheduleItemsHandler` now rejects an empty item list in the handler (not only at
|
||
the controller pre-guard) so a direct caller can't trip the `Max()`-on-empty crash.
|
||
|
||
**Coordination note for #253 PR2–PR4.** Those PRs add `Version++` (pre-commit) to the mutating handlers of
|
||
the versioned aggregates — several of which this sweep also touched (post-commit token, a different line
|
||
region). Low git-conflict risk, but merge `main` in and expect to see the `CancellationToken.None`
|
||
convention already present on the post-commit enqueues.
|
||
|
||
## 2026-07-12 — External-collections scans get an authoritative status surface (#271); the SPA timeout is retired
|
||
|
||
**External Collections rows stay client-derived.** The SPA derives them from `GET
|
||
/api/v1/media-sources`: remote sources expose only sync-enabled libraries, so a non-empty `libraries` list is
|
||
equivalent to Blazor's `Libraries.Any(ShouldSyncItems)` filter. No second listing endpoint or fetch is needed.
|
||
|
||
The first scan-button implementation used a bounded optimistic timeout because collections locks had no HTTP
|
||
mirror. #271 replaces that temporary ceiling with the authoritative status contract below.
|
||
|
||
**Decision 1 — one family-global status endpoint, reading `IEntityLocker`.** New
|
||
`GET /api/v1/media-sources/collections-scan-status` (`MediaSourcesController` → `GetCollectionsScanStatus`
|
||
handler) returns one `CollectionsScanStatusResponseModel { family }` entry per media-source family
|
||
(`plex`/`jellyfin`/`emby`) whose collections lock is currently held, and only for active ones — the direct
|
||
counterpart to `GET /api/v1/libraries/scan-status`. It reads `IEntityLocker.Are{X}CollectionsLocked()` (the lock
|
||
*is* the running scan — the scan-collections controllers acquire it before enqueueing and the scanner releases
|
||
it on completion), analogous to how the library endpoint reads `IScannerProxyService.GetActiveScans()`. Two
|
||
deliberate shape differences from libraries: (a) **family-global, not per-source** — the collections lock takes
|
||
no source id (`LockPlexCollections()`), so an entry means *every* source of that family is scanning, matching
|
||
Blazor's all-rows-disabled behavior (per the PR #272 review note); (b) **no percent** — collections scans
|
||
expose only a boolean lock, not progress.
|
||
|
||
**Decision 2 — the SPA reconciles authoritatively; the fixed timeout is removed.** `useCollectionsScan` now
|
||
polls the new endpoint (seeding on mount, so a scan already running when the screen opens disables the buttons
|
||
immediately — the old timeout couldn't) and reconciles optimistic pending against the active-family set using
|
||
the **same `pruneGraceExpiredPending` grace-tick helper** the library hook uses (now generic over the pending
|
||
key type). A row shows "Scanning" when its family is in the active set **or** it has a still-in-grace optimistic
|
||
pending key. The grace window is kept (not the old wholesale timeout) to absorb the click→observed-active lag and
|
||
the fast-scan-between-polls race — the same bounded-pending discipline #232/#230 established for library scans.
|
||
`COLLECTIONS_PENDING_TIMEOUT_MS` is gone.
|
||
|
||
## 2026-07-11 — Blazor Server UI removed (#91 phase b)
|
||
|
||
The #91 phase (b) removal PR deletes the legacy Blazor Server UI now that the ChicoryTV SPA has parity
|
||
(all gates cleared: #145, #151/#152/#153/#155, #202, #207, #212/#213, #235 F9). The SPA is the only UI.
|
||
|
||
**Deleted.** `ErsatzTV/Pages/**` (all `.razor`, incl. `_Host.cshtml`, `FragmentNavigationBase.cs`,
|
||
`MultiSelectBase.cs`), `ErsatzTV/Shared/**` (all `.razor` + `_Favicons.cshtml`), `ErsatzTV/ViewModels/**`
|
||
(39 Blazor edit-form VMs), `ErsatzTV/Validators/**` (10 Blazor edit-VM FluentValidation validators),
|
||
`App.razor`, `_Imports.razor`, `ErsatzTV/Locals/Shared/**` + `ErsatzTV/Locals/Pages/**` (Blazor
|
||
localization resx — `ErsatzTV/Locals/Resources.*` is KEPT), `ErsatzTV/wwwroot/css/**` (site.css),
|
||
`ErsatzTV/wwwroot/lib/**` (jquery, jqueryui, sortablejs, hls, media-chrome, roboto), `libman.json`, and
|
||
`ErsatzTV.Tests/Pages/MultiSelectBaseTests.cs`.
|
||
|
||
**9 packages pruned** (from both `Directory.Packages.props` and `ErsatzTV/ErsatzTV.csproj`; each verified
|
||
to have zero remaining consumers after the Blazor deletion): **MudBlazor**, **Heron.MudCalendar**,
|
||
**Blazored.FluentValidation**, **BlazorSortable** — unambiguous Blazor UI; **MediatR.Courier.DependencyInjection**
|
||
— `ICourier` was consumed only by the deleted pages, and the app's `mediator.Publish` notification path is
|
||
plain MediatR (unaffected by the `AddCourier` removal); **Markdig**, **HtmlSanitizer**, **Chronic.Core**,
|
||
**NaturalSort.Extension** — verified zero non-Blazor consumers post-deletion.
|
||
|
||
**Startup surgical reduction.** Removed `AddRazorPages` (+`AuthorizeFolder("/")`), `AddServerSideBlazor`,
|
||
`AddMudServices`, `AddSortable`, `AddCourier`, the Blazor-attached `UseAuthentication`/`UseAuthorization`,
|
||
`MapBlazorHub`, and `MapFallbackToPage("/_Host")`. The former "blazor" `MapWhen` branch (lambda param
|
||
renamed `blazor`→`legacy`) is KEPT — it still co-hosts `MapControllers()`, `/docs` (Scalar), dev
|
||
`MapOpenApi()`, and the `LegacyUiRedirects` middleware. `MapFallbackToPage("/_Host")` is REPLACED by a
|
||
catch-all `endpoints.MapFallback(...)` that 302-redirects any unmatched path to `PathBase + "/app"`
|
||
EXCEPT paths under `/api`, `/artwork`, `/docs`, `/openapi` (those get a genuine 404, per #204's design).
|
||
**KEPT** (not removed): the OIDC/JWT/API-key SERVICE registrations (inert unless configured; real auth is
|
||
#197), `ConditionalIptvAuthorizeFilter` (`/iptv/*`), and `ApiKeyAuthorizationFilter` (mutating `/api/*`) —
|
||
per the #206 auth-posture sign-off (deleting the Blazor page challenged nothing beyond phase (a)).
|
||
|
||
**LegacyUiRedirects.** Added redirects for all 14 `/media/sources/*` routes → their `/app/libraries/*`
|
||
SPA screens (7 Tier-1 exact + 7 Tier-2 `{id}` patterns; the last Section-2 rows in
|
||
`blazor-route-parity.md`), and LIFTED the #204-era `/media/sources` forbidden-prefix guard (its Blazor
|
||
pages were replaced by #202's SPA screens). The forbidden-prefix guard now covers only `/api`, `/artwork`,
|
||
`/docs`, `/openapi`, `/iptv`, `/app`.
|
||
|
||
Also removed the now-dead ersatztv#25 razor-Sonar `<NoWarn>S6966;S3267;…</NoWarn>` line from
|
||
`ErsatzTV.csproj` — those Sonar rules only needed suppression in `.razor` `@code`; on `.cs` they run at
|
||
`suggestion` via `.editorconfig`, so removal is safe (closes part of #25's burn-down).
|
||
|
||
**Rollback.** The tag `blazor-final` was cut on the pre-removal `main` commit as the first step (see the
|
||
2026-07-11 "Pre-removal Blazor rollback tag `blazor-final` (#205)" entry above for the exact command +
|
||
restore path). Not a `v*` tag → no prod release build.
|
||
|
||
## 2026-07-12 — Live-E2E is a required step for API write-path handler changes (#303)
|
||
|
||
**A PR that changes an API write-path handler (a `POST`/`PUT`/`DELETE` `/api/*` command that mutates
|
||
state and reloads it through the read path) MUST include a live-E2E pass** — driving the real endpoint
|
||
or its SPA screen and confirming the mutation round-trips through a subsequent read — not only unit /
|
||
characterization tests. Rationale: this class has a **correlated blind spot** unit and characterization
|
||
tests share. A green fixed-point test passed while the write-path returned a production 500 because the
|
||
handler returned a *lazy* LanguageExt `Map` the test never enumerated (#229; reload-through-read-path
|
||
mechanics in `api-conventions.md` §7); the failure surfaces only when the result is materialised, which
|
||
the SPA does and the test did not. Live driving is the only reliable net for it.
|
||
|
||
Non-write-path (pure-SPA/read-only) and docs PRs don't need it. The requirement is auditable, not
|
||
silent: the PR/close comment states that live-E2E ran, or — for a non-write-path change — that it
|
||
wasn't required (the same stated-exemption discipline as the review skip rubric). Recipe +
|
||
"When live-E2E is required": `docs/e2e-local.md`. This formalizes the #229 lore bullet ("live E2E
|
||
remains the only net for this class") into a standing convention.
|
||
|
||
## 2026-07-12 — TopBar primary-action button: wire creates, drop the rest (#238)
|
||
|
||
The shell TopBar rendered a prominent top-right primary-action button (Plus icon) for **every** screen, but
|
||
only `SchedulesScreen` had ever subscribed to its `ctv:primary-action` event — so on every other screen the
|
||
button was **dead** (either a labelled no-op like "Save Changes"/"Add Channel", or, for the ~10 routes whose
|
||
`primaryAction` was `''`, a bare labelless "+"; the button rendered unconditionally). Issue #238 (from a #229
|
||
live-E2E finding, pre-existing since the TopBar's introduction).
|
||
|
||
**Decision — the Plus-icon button is a "create new item" affordance; keep+wire it only where that fits:**
|
||
|
||
- **TopBar renders the button only when the active route declares a non-empty `primaryAction`** (was:
|
||
unconditional). This alone removes every empty-`''` dead "+".
|
||
- **WIRE** (8 list screens with a single unambiguous create flow) via a shared `usePrimaryAction(routeId,
|
||
handler)` hook (`web/src/primaryAction.ts`), each delegating to the same top-level create handler its in-body
|
||
control uses (`navigateToPath('/app/new-channel')`, `setEditing({kind:'new'})`, `navigateToPath('…/add')`,
|
||
etc.): channels, schedules (refactored onto the hook), multiCollections, rerunCollections, traktLists,
|
||
fillerPresets, ffmpegProfiles, watermarks.
|
||
- **DROP** (`primaryAction: ''`, no button) everywhere else, for one of four reasons: (1) the action isn't a
|
||
create so the "+" is wrong and a correct in-body control already exists — editChannel (savebar), settings
|
||
(savebar), guide (Jump to now), playouts (Reset all), logs/troubleshooting/blockPlayoutTroubleshooting
|
||
(Refresh), playbackTroubleshooting (Play), yamlValidator (Validate); (2) ambiguous — collections (two create
|
||
types behind tabs); (3) a silent no-op — builder ("Create Channel" is disabled until the form is valid),
|
||
playlists (needs a group first); (4) semantically misplaced — dashboard (status page), libraries ("Scan" is
|
||
per-library-row, no global target).
|
||
|
||
**Why not wire everything** (the SchedulesScreen precedent): the recon found every screen already carries a
|
||
correct, disabled-state-aware, context-aware in-body control, and several banner actions are unreachable
|
||
without refactoring handlers out from under early returns, or would render a silent no-op — reintroducing the
|
||
very dead-button class #238 fixes. Wiring only the single-create-flow screens gives one explainable rule
|
||
("primary button = create a new item on a list screen") and needs no risky refactors.
|
||
|
||
**Also fixed here:** the `apiKey` route's `primaryAction: 'Save key'` + its description/comment were stale
|
||
post-#295 (the screen displays the machine key + changes the local password; there is nothing to "save") —
|
||
relabelled to reflect the #295 reality. Convention + hook documented in `spa-conventions.md` §10. The
|
||
implicit route-label ⟺ screen-subscription coupling is the kind of thing #247 (shell extraction) will
|
||
formalize; #238 keeps it a documented convention guarded by a data-driven `App.test.tsx` test over the URL-navigating
|
||
create screens (a typo'd route id → the banner navigates nowhere → red). Refs #238 #247.
|
||
## 2026-07-13 — API versioning: the whole `/api` surface is mounted at `/api/v1`, additive-only after freeze (#286)
|
||
|
||
The #197 cold review's C1 **BLOCKER**: `/api/*` was entirely unversioned (`info.version` was cosmetic), so the
|
||
first breaking change would silently break the SPA and any external/MCP client with no negotiation path. This is
|
||
the Phase-2 contract-freeze gate — versioning can't be added compatibly *after* the contract ossifies, so it
|
||
lands before freeze.
|
||
|
||
**What changed.** Every route under `/api` was swept to `/api/v1` — all 251 controller route attributes, the ~24
|
||
`Location`-header literals, the scanner callback URL (`CallLibraryScannerHandler.GetBaseUrl`), and the
|
||
`Startup` request-log path literal. This is **uniform**: the machine JSON API, the browser-session auth surface
|
||
(`/api/v1/auth/*`, still `IgnoreApi`), the internal loopback callbacks (`/api/v1/scan/*`) and the scripted-build
|
||
surface (`/api/v1/scripted/*`) are all versioned, so there is no unversioned corner and the compat rewrite needs
|
||
no exclusion list. The OpenAPI `v1.json` (160 paths), `endpoint-index.md`, and the SPA (945 request literals +
|
||
its test mocks, incl. regex/positional URL parsers) were regenerated/swept in lockstep. **No wire-DTO or
|
||
status-code change** — only the path prefix moved.
|
||
|
||
**Legacy compat = an in-pipeline rewrite, NOT a redirect** (`ApiVersionRewriteMiddleware`, sequenced before
|
||
`UseRouting` in the API branch). A legacy caller hitting an unversioned `/api/foo` has its request *path*
|
||
rewritten to `/api/v1/foo` and continues in-pipeline — method, body, auth headers and query string all survive,
|
||
so curl / the future MCP server / bookmarked URLs keep working with no round-trip (a 307/308 redirect would have
|
||
been fragile for non-GET + custom-header clients). Rewritten (legacy) responses carry RFC 8594 `Deprecation: true`
|
||
+ `Link: </docs>; rel="deprecation"`, and a `Sunset` header when `Api:LegacyRoutesSunset` is configured. An
|
||
already-versioned path (`/api/v1/*`) passes through untouched; a future `/api/v2/*` is **not** forced back to v1
|
||
(the middleware only fills in a *missing* version).
|
||
|
||
**Freeze semantics (owner decisions):** once shipped, `/api/v1` is **additive-only** — new endpoints/optional
|
||
fields are fine; renaming/removing/retyping an existing one requires a new `/api/v2`, never an in-place break.
|
||
The legacy-rewrite compat shim has a **2-release sunset window** (owner-chosen) before removal; the actual removal
|
||
is a tracked Phase-3 follow-up, not this PR. Existing pre-freeze warts (e.g. channel `{id}` vs `{channelNumber}`,
|
||
the synthesized negative-id "(none)" group rows) are frozen as-is per their own prior decisions.
|
||
|
||
**Route-convention standardization (#286, owner-requested).** The leading-slash inconsistency (238 absolute
|
||
`"/api/…"` method routes vs 13 relative `"api/…"`) is resolved: the standard is a **leading-slash absolute route
|
||
on each method's `[Http*]` attribute, no class-level `[Route]`** — except the two controllers where many actions
|
||
share a parametrized prefix (`ScannerController` `{scanId}`, `ScriptedScheduleController` `{buildId}`, ~40
|
||
methods), which keep a leading-slash absolute **class** `[Route("/api/v1/…")]` with relative method segments (the
|
||
right tool for a shared prefix). Enforced by `ApiRouteVersioningTests`: it reflects over every `[ApiController]`
|
||
in `Controllers.Api`, computes each action's *effective* route (ASP.NET's class+method combination rule), and
|
||
asserts it matches `^/api/v\d+/` — so a new controller that drifts (relative or unversioned) fails CI, the
|
||
"fix-it-while-you're-in-the-file" gate the `dotnet format` rules use. Browser-nav endpoints deliberately outside
|
||
`/api` (e.g. `GET /auth/oidc/login`) are out of scope for the test. Docs: `api-conventions.md` §1/§9. Refs #286 #197.
|
||
|
||
## 2026-07-13 — Scheduling API hardening: null-name 500s, duplicate template items, unreachable 404 (#172)
|
||
|
||
Cleared the still-live findings from issue #172 (consolidated non-blocking nits from the #144 S1/S2
|
||
reviews). Most of the 2026-07-07 list had already been ratified deliberate (§8 "(none)" synthesized
|
||
rows; §3b deep-FK non-existence-check) or fixed since (the unauthenticated `/api/logs` +
|
||
`/api/troubleshoot/info` GETs now carry `[RequiresAuthentication]` per §9; the Trakt matched-items link
|
||
points at the live `/app/search`; `GET /api/search` already fans out via `Task.WhenAll`). Three were
|
||
genuinely live:
|
||
|
||
- **Null/empty `name` → 500 (10 handlers).** Create + Replace/Update handlers for Block, Template,
|
||
DecoTemplate, Deco (8, all genuine 500s), plus `UpdateFFmpegProfile` (genuine 500; `CreateFFmpegProfile`
|
||
was already guarded) and `CreatePlaylist` (its DTO coalesces `null`→`""`, so an empty-name persist, not a
|
||
500) all did `if (request.Name.Length > 50)` on a client-nullable `string Name` → unhandled
|
||
`NullReferenceException`. Fixed to `if (string.IsNullOrWhiteSpace(request.Name) || request.Name.Length >
|
||
50)` — kills the NRE, and also rejects empty/whitespace names (matching the group-create handlers'
|
||
`NotEmpty` behavior, closing a latent "block/template named ''" gap). Chose the one-line guard over
|
||
refactoring each handler onto the `NotEmpty`/`NotLongerThan` combinator to keep the blast radius tiny and
|
||
preserve each handler's existing error message + 422 mapping. Convention captured in `api-conventions.md`
|
||
§3b handler-hardening checklist.
|
||
- **Exact-duplicate template items bypassed overlap validation.** `ReplaceTemplateItemsHandler`'s O(n²)
|
||
overlap loop skipped on `item == otherItem`, but `BlockTemplateItem` is a `record`, so two value-identical
|
||
items (same BlockId + StartTime → same computed EndTime) were value-equal and skipped — both persisted
|
||
unvalidated. Switched to index-based iteration (`i != j`) so identical items at distinct positions are
|
||
compared and register as a (self-)intersection → rejected 422. (The SPA's index-based check already caught
|
||
this client-side; it was an API-only gap.)
|
||
- **Unreachable 404 on create-group actions.** `POST /api/blocks/groups` and `POST /api/templates/groups`
|
||
declared `[ProducesResponseType(ProblemDetails, 404)]` copied from precedent, but a create has no parent
|
||
lookup that can 404 (only 201/422). Trimmed — OpenAPI spec regenerated.
|
||
|
||
Deliberately **not** fixed (documented as accepted): the §8 "(none)" synthetic rows, the §3b deep-FK
|
||
non-existence-check, and the missing `Name=` on `PlayoutController` Create/Delete/Update (moot — the
|
||
"v1"-doc `OperationIdOpenApiTransformer` (#197 Bundle C) already synthesizes stable operationIds for
|
||
`Name=`-less ops, and adding `Name=` would risk renaming generated SPA client methods). Refs #172 #197.
|
||
|
||
## 2026-07-16 — Functional-E2E CI harness: advisory curl-contract job over an app booted from source (#299)
|
||
|
||
The manual live-E2E curl flows sessions had been re-running by hand (and leaving only as PR/issue
|
||
comments) are now a CI regression net. Two decisions shaped it:
|
||
|
||
**Boot from source + `dotnet run`, not the built image.** The only "E2E" in CI before this was the
|
||
smoke test in the `build` job, which runs against the *pushed* image — so it exists only on `main`/`v*`
|
||
(the image isn't built on PRs) and would test a stale image, not the PR's code. To gate PRs on the PR's
|
||
own code, the `functional-e2e` job builds the SPA + solution and launches `dotnet ErsatzTV.dll` via the
|
||
same `scripts/e2e-local.sh` used locally (parameterized with `ETV_BUILD_CONFIG=Release`). The assertions
|
||
live in `scripts/e2e-functional.sh`, so the identical harness runs by hand and in CI — which is the
|
||
point of the issue (stop re-deriving the flows each session).
|
||
|
||
**Advisory, not blocking — separate job, not a `build` dependency, not a required check.** Per the
|
||
issue's "a functional-E2E flake must not block the unit-test gate." A boot-the-app job has more moving
|
||
parts (background process, port, readiness wait) than a pure unit test, so it starts advisory and gets
|
||
promoted to a required check / `build` dependency once proven reliable — the same staged rollout the
|
||
`migrations` job used. SQLite is the default provider, so it needs no DB service container.
|
||
|
||
**Scope is curl-only and deterministic; the racy/interactive flows are explicitly deferred.** The first
|
||
cut asserts the legacy→SPA redirect sweep (+ `/api`/`/artwork` never-redirect exemption), the
|
||
auth/CSRF/security-stamp flow, the library-scan status contract (404/202/`scan-status`), and the
|
||
`If-Match`/412 round-trip — all exercisable without seeded media, ffmpeg-transcode, or a browser (an
|
||
empty local library still enqueues `202`; an empty collection drives the concurrency editor). The 409
|
||
"already-scanning" re-trigger (needs a long-running scan to be non-racy), the playout-build lock 409,
|
||
and the genuinely UI-interactive Playwright flows are deferred as #299 follow-ups rather than shipped
|
||
flaky. Assertions were written against a real running instance, not the source — which caught that
|
||
`/artwork/*` returns `400` (not the `404` a static read suggested); extend the harness the same way.
|
||
|
||
## 2026-07-16 — Optional advertised IPTV base URL (`iptv.base_url`) resolved centrally in the two generators (#340)
|
||
|
||
ErsatzTV's absolute IPTV URLs (M3U stream/logo/guide URLs, XMLTV `<icon>`/artwork) were always
|
||
derived from the incoming request's `Scheme`/`Host`/`PathBase`, so any client that fetched with a
|
||
host downstream consumers can't resolve (the historical Gitea #1 `localhost:8409` symptom) baked
|
||
that host into the output. #340 adds an **optional** advertised base URL to pin those URLs to a
|
||
fixed public origin. Several deliberate decisions shaped it:
|
||
|
||
**(a) Resolve the override centrally in the two generation handlers, via a pure Core helper — not
|
||
in the controller.** A new `ErsatzTV.Core/Iptv/AdvertisedBaseUrl.cs` exposes `TryParse(string) →
|
||
Option<(scheme, host, baseUrl)>` and `Resolve(configured, requestScheme, requestHost,
|
||
requestBaseUrl)`. `GetChannelPlaylistHandler` (M3U, via `ChannelPlaylist`) and
|
||
`GetChannelGuideHandler` (both XMLTV `{RequestBase}` substitution sites) call `Resolve` and use its
|
||
result instead of the raw request values. Keeping the logic in a pure, allocation-free Core helper
|
||
(not the thin controller) keeps controllers dumb, makes the parse/validate/resolve rules unit-testable
|
||
in isolation, and — because the fallback path returns the exact request-derived values — leaves the
|
||
M3U/XMLTV golden tests (`ChannelPlaylistGoldenTests`, `ChannelGuideGoldenTests`) untouched.
|
||
|
||
**(b) Validation rules, with blank/invalid → request-derived so "unset" is byte-identical.**
|
||
`TryParse` accepts only an **absolute http(s)** URL with **no credentials, query, or fragment**; it
|
||
**preserves the port and any path prefix** and **normalizes a trailing slash** off. Anything failing
|
||
these rules → `None`, and `Resolve` then falls back to the request-derived scheme/host/base. So an
|
||
unset or malformed value produces output byte-for-byte identical to today's request-derived behaviour
|
||
— the override is strictly opt-in and can never silently corrupt the default path.
|
||
|
||
**(c) A NEW `iptv` settings group, not folded under `xmltv`.** The base URL affects **both** the M3U
|
||
playlist and the XMLTV guide, so it does not belong under the existing XMLTV-only settings. `GET`/`PUT
|
||
/api/v1/settings/iptv` on `SettingsController` (tier `[RequiresAuthentication]`) with body `{ baseUrl }`,
|
||
backed by `GetIptvSettings`/`UpdateIptvSettings` handlers + `IptvSettingsViewModel` /
|
||
`IptvSettingsResponseModel` / `UpdateIptvSettingsRequest`, and a new "IPTV" section on the SPA Settings
|
||
screen. Follows the existing settings GET/PUT pattern — no new API convention.
|
||
|
||
**(d) Blank clears the key; non-blank malformed → 422.** A blank/whitespace `baseUrl` on PUT
|
||
**deletes** the `ConfigElement` (reverting to request-derived); a non-blank value that fails
|
||
`AdvertisedBaseUrl.TryParse` is rejected with **422** rather than being silently stored and ignored at
|
||
generation time — the error surfaces at the point of configuration.
|
||
|
||
**(e) Scoped to M3U + XMLTV, deliberately NOT HDHomeRun.** #340 covers only the M3U playlist and XMLTV
|
||
guide generators. The HDHomeRun lineup URLs still echo the request host; extending the override there
|
||
was explicitly out of scope.
|
||
|
||
**(f) Distinct from `ETV_BASE_URL`.** The `ETV_BASE_URL` environment variable only sets the ASP.NET
|
||
Core `PathBase` (request routing/prefix); it does not advertise a scheme+host. `iptv.base_url` is the
|
||
separate, DB-stored (`ConfigElementKey.IptvBaseUrl`, **no EF migration**) advertised origin for IPTV
|
||
output. See `docs/m3u-xmltv.md` → "IPTV base URL (#340)".
|
||
|
||
## 2026-07-16 — Auto-tuning enumerates via EF, persists via SmartCollection; additive coexistence (#69)
|
||
|
||
Auto-tuning (#69) generates channels from library metadata (TV Show / TV Genre / Movie Genre).
|
||
Enumeration for the preview uses EF distinct+count queries (exact counts drive the min-items
|
||
threshold and preview display); each created channel is backed by a newly-created **SmartCollection**
|
||
(live Lucene query) so channels keep tracking the library as it grows. Query authorship is
|
||
server-side only — the client passes `{axis, value}`, never a Lucene string. Coexistence is additive:
|
||
the batch gets a reserved starting channel number (skipping taken numbers), a proposed channel whose
|
||
name already exists is flagged and de-selected by default, and existing channels are never mutated.
|
||
Bulk create loops the #63 `CreateChannelFromLineup` primitive via `ISender` and returns a per-channel
|
||
Created/Skipped/Failed outcome. Known MVP limitation: the generated SmartCollection is named after the
|
||
channel; a name collision with an existing SmartCollection surfaces as a per-channel Failed outcome.
|
||
|
||
## 2026-07-16 — Per-playout reshuffle = scoped Reset build; seed surfaced (#71)
|
||
|
||
- The shuffle seed (`Playout.Seed`) + per-collection `CollectionEnumeratorState` already persist a stable
|
||
shuffled order across rebuilds. #71's real gap was a **user-triggered per-playout reshuffle** (a Classic
|
||
playout's seed is otherwise only reseeded by a full Reset, and `reset-all` uses `Refresh` for Classic, so
|
||
it never reseeds Classic) plus visibility.
|
||
- `POST /api/v1/playouts/{id}/reshuffle` enqueues `BuildPlayout(id, PlayoutBuildMode.Reset)` — Reset already
|
||
reseeds + clears anchors/rerun-history + rebuilds. Named `/reshuffle` (not `/reset`) to (a) match user
|
||
intent and (b) avoid the "reset-one reseeds Classic while reset-all refreshes Classic" naming clash. It is
|
||
deliberately more aggressive than `reset-all` for Classic: an explicit single-channel action rolls a new
|
||
order; the bulk action stays non-disruptive.
|
||
- `Playout.Seed` is surfaced on the playout list + detail DTOs so the SPA can show it — its purpose is
|
||
**visible confirmation** (the seed changes after a reshuffle).
|