From 39c4e8df0a936c4b22ee3e724e80fa93c9d50d4c Mon Sep 17 00:00:00 2001 From: Timothy Date: Mon, 27 Jul 2026 04:09:21 +0200 Subject: [PATCH] =?UTF-8?q?fix(651):=20review=20round=208=20=E2=80=94=20pu?= =?UTF-8?q?t=20the=20selection-id=20predicate=20at=20the=20boundary,=20not?= =?UTF-8?q?=20the=20site?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 7 added the int32 check inside `isSearchPickerOption` — the place the defect was found — which left every other door into editor state open. A malformed successful response carrying `1.5` or `2147483648` still entered `draft` through list-backed options and through the selection restored from the detail read, keeping Save enabled and sending a value the API cannot bind, while the identical value arriving via SearchPicker was correctly rejected. The predicate now lives once, in `web/src/api/selectionId.ts`, and sits on every path by which an id from the wire becomes editor state. The class crosses all three screens, not just the one the finding named, so all three are covered: - RerunCollectionsScreen: `toPickerOptions` (3 list branches) + `draftFromRerun` - PlaylistsScreen: `toPickerOptions` (3 list branches) + `draftFromItem` (4 id fields) - FillerPresetsScreen: `draftFromPreset` (5 id fields) + the collection-family browse options - pickers.tsx: `isSearchPickerOption` now delegates rather than carrying its own copy An unbindable id is treated as ABSENT, never coerced — rounding 1.5 to 1 would submit a DIFFERENT record — so it surfaces as "no selection" with Save disabled and a visible reason; an option that cannot be selected safely is dropped rather than rendered. Five regressions assert zero writes are reachable via each previously-unguarded path. Also corrects two of my own test descriptions, per the review: the padded-ETag test is a regression guard rather than a round-7 defect demonstration (Headers strips outer whitespace before the app sees it), and the late-settlement test guards the abort/race COMPOSITION — what it actually fails is an abort-only implementation whose fetch ignores its signal, which is why its stub ignores `init.signal`. Co-Authored-By: Claude Opus 5 (1M context) --- docs/decisions/README.md | 2 +- .../spa/library-pickers-resolve-by-search.md | 4 +- docs/spa-conventions.md | 11 +++ web/src/api/index.ts | 1 + web/src/api/selectionId.ts | 24 +++++++ web/src/schedules/pickers.tsx | 17 ++--- web/src/screens/FillerPresetsScreen.tsx | 18 +++-- web/src/screens/PlaylistsScreen.tsx | 23 ++++-- .../screens/RerunCollectionsScreen.test.tsx | 72 ++++++++++++++++++- web/src/screens/RerunCollectionsScreen.tsx | 21 ++++-- 10 files changed, 159 insertions(+), 34 deletions(-) create mode 100644 web/src/api/selectionId.ts diff --git a/docs/decisions/README.md b/docs/decisions/README.md index 04c140333..d75159af8 100644 --- a/docs/decisions/README.md +++ b/docs/decisions/README.md @@ -164,7 +164,7 @@ the link for rationale. Superseded/retired history lives in `archive/`. Regenera | `spa.deco-templates-table` | The deco-templates editor also renders its day/deco assignment as a table, extending (not replacing) the templates-editor-table convention. | 2026-07-09 | [link](records/spa/deco-templates-table.md) | | `spa.download-sample-gate` | The SPA disables both Download Media Sample and Download Results while a troubleshooting session is starting/running (Blazor only gated Download Results). | 2026-07-09 | [link](records/spa/download-sample-gate.md) | | `spa.legacy-redirect-matcher` | `LegacyUiRedirects.TryGetRedirect` is a two-tier matcher — an exact `OrdinalIgnoreCase` `Map` (Tier 1) then an ordered segment-template pattern list (Tier 2, first-match-wins) — collision-free by construction, with a guard invariant that no rule may prefix-match `/api`, `/artwork`, `/docs`, `/openapi`, `/iptv`, `/app`, or `/media/sources`. | 2026-07-11 | [link](records/spa/legacy-redirect-matcher.md) | -| `spa.library-pickers-resolve-by-search` | A picker over a media-library table (Episode/Song/Image/Movie/MusicVideo/TelevisionShow/TelevisionSeason/Artist/OtherVideo/RemoteStream) resolves its options by SEARCH — a debounced `SearchPicker` calling `searchLibraryPickerOptions`, which issues at most ONE `getLibraryBrowseItems` request per settled query, bounded to `LIBRARY_PICKER_RESULTS` (25) rows — CLAMPED inside the helper, not merely defaulted — and gated on `LIBRARY_PICKER_MIN_QUERY` (2) characters. It list-loads NOTHING on mount or on a type switch, so there is no truncation to surface and no truncation hint. The typed text is COMPILED (`titleContainsQuery` → `title:**`), never forwarded raw. The current selection renders from the OWNING RECORD, not from the result set (`selectedName` on a rerun collection / playlist item; a single by-id detail read — `getShow`/`getSeason`/`getArtist` — for a filler preset, which stores only the id), and an edit draft is INITIALIZED ONCE from the detail read — never seeded from the list row, never reconciled against a late response — with the form withheld until it lands, the editor failing CLOSED when the response carries no USABLE concurrency token — absent, empty and whitespace-only ETags are ONE case, normalized in one place, so a PUT without `If-Match` is unreachable, and a deadline plus a route back so a hung request cannot strand it. An id NEVER travels without its namespace: search results are cached against `(source, query)` and list-backed options carry the type they were loaded for, so no id from one type can be offered under another. Conflicts are detected at SAVE time via `If-Match` -> 412 -> Reload, and Reload simply drops the draft back to null and re-runs the same initialize-once load, so the form is unmounted while the replacement is in flight; an asynchronously-resolved name is keyed to the id it was resolved for and never overwrites a label naming a different id. The typeahead implements the full ARIA combobox keyboard contract, because it replaces a natively keyboard-operable ``. The other half of the superseded record is UNCHANGED: bounded-by-construction admin lists (collections, multi-collections, smart collections, playlists) still page to completeness via `loadAllPages` and still report `complete`/`hint: incomplete`. Server-side caps are not raised — this is a web-only change. | 2026-07-26 | [link](records/spa/library-pickers-resolve-by-search.md) | | `spa.logs-page-size-local` | The Logs page rows-per-page preference is stored in `window.localStorage` (`ctv-logs-page-size`), not a server `ConfigElement`. | 2026-07-11 | [link](records/spa/logs-page-size-local.md) | | `spa.playback-troubleshoot-poll` | The playback-troubleshooting screen reports FFmpeg completion by polling `GET /api/troubleshoot/playback/status` (~2s) rather than a server push channel. | 2026-07-09 | [link](records/spa/playback-troubleshoot-poll.md) | | `spa.playout-reset-button` | The SPA keeps a single Reset action (server picks the default build mode) and drops Blazor's separate "Schedule reset" button since its capability already exists via the playout's Edit-details flow. | 2026-07-09 | [link](records/spa/playout-reset-button.md) | diff --git a/docs/decisions/records/spa/library-pickers-resolve-by-search.md b/docs/decisions/records/spa/library-pickers-resolve-by-search.md index 711cef00c..c5a6bb3ff 100644 --- a/docs/decisions/records/spa/library-pickers-resolve-by-search.md +++ b/docs/decisions/records/spa/library-pickers-resolve-by-search.md @@ -5,8 +5,8 @@ status: active since: '2026-07-26' supersedes: spa.list-completeness-vs-bounded-pickers@2026-07-26 superseded-by: none -rule: 'A picker over a media-library table (Episode/Song/Image/Movie/MusicVideo/TelevisionShow/TelevisionSeason/Artist/OtherVideo/RemoteStream) resolves its options by SEARCH — a debounced `SearchPicker` calling `searchLibraryPickerOptions`, which issues at most ONE `getLibraryBrowseItems` request per settled query, bounded to `LIBRARY_PICKER_RESULTS` (25) rows — CLAMPED inside the helper, not merely defaulted — and gated on `LIBRARY_PICKER_MIN_QUERY` (2) characters. It list-loads NOTHING on mount or on a type switch, so there is no truncation to surface and no truncation hint. The typed text is COMPILED (`titleContainsQuery` → `title:**`), never forwarded raw. The current selection renders from the OWNING RECORD, not from the result set (`selectedName` on a rerun collection / playlist item; a single by-id detail read — `getShow`/`getSeason`/`getArtist` — for a filler preset, which stores only the id), and an edit draft is INITIALIZED ONCE from the detail read — never seeded from the list row, never reconciled against a late response — with the form withheld until it lands, the editor failing CLOSED when the response carries no USABLE concurrency token — absent, empty and whitespace-only ETags are ONE case, normalized in one place, so a PUT without `If-Match` is unreachable, and a deadline plus a route back so a hung request cannot strand it. An id NEVER travels without its namespace: search results are cached against `(source, query)` and list-backed options carry the type they were loaded for, so no id from one type can be offered under another. Conflicts are detected at SAVE time via `If-Match` -> 412 -> Reload, and Reload simply drops the draft back to null and re-runs the same initialize-once load, so the form is unmounted while the replacement is in flight; an asynchronously-resolved name is keyed to the id it was resolved for and never overwrites a label naming a different id. The typeahead implements the full ARIA combobox keyboard contract, because it replaces a natively keyboard-operable ``. The other half of the superseded record is UNCHANGED: bounded-by-construction admin lists (collections, multi-collections, smart collections, playlists) still page to completeness via `loadAllPages` and still report `complete`/`hint: incomplete`. Server-side caps are not raised — this is a web-only change.' +signals: 'library picker typeahead, SearchPicker, searchLibraryPickerOptions, titleContainsQuery, LIBRARY_PICKER_RESULTS, LIBRARY_PICKER_MIN_QUERY, LIBRARY_PICKER_LUCENE_SPECIALS, compile typed text not raw Lucene, Lucene && || escaping, picker truncation hint removed, loadAllPages Class A, LuceneSearchIndex.Search hitsLimit, useIsMountedRef, aria-activedescendant combobox keyboard, initialize-once draft not hydrate-merge, no list-row seeding, fail closed on a missing or blank ETag, usable concurrency token, cross-type id, id never travels without its namespace, results keyed on (source query), failed search not cached as empty, selection id int32 boundary predicate, isSelectionId, npm run typecheck not tsc --noEmit, stale result set not committable by keyboard OR pointer · paths: `web/src/api/libraryBrowse.ts`, `web/src/schedules/pickers.tsx`, `web/src/hooks.ts`, `web/src/screens/RerunCollectionsScreen.tsx`, `web/src/screens/PlaylistsScreen.tsx`, `web/src/screens/FillerPresetsScreen.tsx`, `web/src/api/paging.ts`, `docs/spa-conventions.md` §3b · issues: #651, #644, #578, #440' mechanics: '`docs/spa-conventions.md` §3b' --- diff --git a/docs/spa-conventions.md b/docs/spa-conventions.md index 4bc63ea0d..6e014681d 100644 --- a/docs/spa-conventions.md +++ b/docs/spa-conventions.md @@ -295,6 +295,17 @@ appear once the widget is asynchronous: storm** on a persistent outage. Keep the two apart: a `resultsFor.ok` flag says whether the held answer is authoritative, and a separate *attempted* key (a ref, so writing it doesn't re-render) suppresses automatic retries until an explicit user action — reopen, focus, or edit — re-arms it. +- **Put a validity predicate at the BOUNDARY the class crosses, not at the site the bug was found.** + An entity-reference id (`selectedId`, `collectionId`, `mediaItemId`, …) is bound by the API as a + 32-bit integer, so `1.5` or `2147483648` renders and commits fine and then fails on write. Such + ids enter editor state through *several* doors — search results, list-backed `