feat(651): library-browse pickers resolve by search, not a 100-row window #678
No Assignees
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: timothy/ersatztv#678
Reference in New Issue
Block a user
fixes #651
Three pickers (
RerunCollections,Playlists,FillerPresets) populated a native<select>from a bounded 100-rowgetLibraryBrowseItemspage over media-library tables — Episode/Song/Image/Movie/MusicVideo, the largest in an install. #644 made the truncation visible; it stayed unusable. Paging to completeness would have been worse (~200 serial requests, each costlier than the last, feeding tens of thousands of<option>nodes).They now resolve by search.
SearchPickeris shared,titleContainsQuerymoved intolibraryBrowse.tsso one escaping implementation serves every caller, and bounds live in the helper rather than in caller discipline: one request per settled query,pageSize25, no request under 2 characters. Selecting a media-library type now issues zero requests.Also folds in three of #578's four items (fetch-on-focus, a shared is-mounted guard, the LCG divisor). #578 stays open — its backend item is PR #676.
What the reviews changed
Eight adversarial rounds. The design that shipped is not the one that started, and the reversals are the substance:
A late detail response must not be reconciled against user edits. Rounds 1–4 each fixed one reconciliation defect and the next round found its sibling: a field-wise merge paired one selection's id with another's name; conflict "Reload" kept the selection it promised to discard;
identityConflictsboth missed a conflict and manufactured a false one that left Save able to force-write. Round 5 deleted the layer (−249 lines:touchedRef,hydrateDraft,hydrateIdentity,identityConflicts,replaceDraft,replacePending).The justification is a fact worth recording:
GetPagedRerunCollectionsHandlerhas 0.Include()s,GetRerunCollectionByIdHandlerhas 8 + 6 ThenInclude, and both project through the same mapper — so the list response is a strict subset of the detail one. The round-1 guarantee that started the whole layer was protecting a value that only exists in test fixtures; in production it is null for every row (#671).FillerPresetsandPlaylistsalready did initialize-once. RerunCollections was the outlier, which is why every finding landed there.Now: the draft is initialized once from the detail read, the form is withheld until it lands, the editor fails closed when the response carries no usable ETag, and conflicts are detected at save time via
If-Match→ 412 ��� Reload.An id never travels without its namespace.
SearchPickertakes a requiredsource; foreign-source results are hidden outright, not dimmed. The int32 selection-id predicate lives once inselectionId.tsand sits on every ingress across all three screens — an unbindable id is treated as absent, never coerced (rounding1.5→1would submit a different record).Other fixes found by review:
&/|were missing from the Lucene escape set (pre-existing in Auto-Tune, fixed at the shared helper); a failed search retried forever; a malformed 2xx body crashed onresults.length; a never-settling search or detail load had no deadline; keyboard operability (ArrowUp/Down/Enter/Escape per the ARIA APG) — the native<select>it replaced was fully keyboard-operable, so its absence was a regression.Testing
1034 → 1123. Every new test was mutation-verified: the mechanism removed, the failure observed, the mechanism restored. Two standards came out of this branch and are now in §3b — verify an invariant on every screen it names before writing it down, and assert zero writes only after attempting one.
Eight vacuous-test shapes were found and fixed along the way, each passing for a different wrong reason: asserting
console.errorReact 19 no longer emits; a stale text-node handle React mutates in place; a duplicated LCG formula; clicking a disabled button; a test claiming id 9 but firing id 5; mocks omitting an ETag and thereby exercising the force-write path; a stub ignoring its abort signal; and atoHaveLength(0)on writes in a test that never attempted one.Verification
npm run lintclean ·npm run typecheck(tsc -b) clean ·npx vitest run110 files / 1123 tests ·decisions_validate.pyOK ·scripts/tests325 passed. No.cstouched.Verified independently of the implementer: mutating
>=to>inselectionId.tsfails the endpoint test (that predicate is now the single point of failure for eleven call sites).Record
spa.library-pickers-resolve-by-searchsupersedesspa.list-completeness-vs-bounded-pickers. Follow-ups: #670, #671, #677.Round 4's through-line: rounds 2-3 made HYDRATION treat {collectionType, selectedId, selectedName} as one value, but the fix was applied to the structure that was named rather than to every structure that carries an id. Three more instances of the same class, plus two independent findings. HIGH-1 + HIGH-2 (one change, per the structural directive): every result, option and cached result set now carries its SOURCE, and identity is compared as (type, id). - `SearchPicker` takes a REQUIRED `source` prop and caches results against (source, query), not query text. Keying on text let the round-3 re-query guard SUPPRESS the new source's request and leave the previous namespace's hit clickable under the new label — a Collection id stored as a SmartCollection id. Results from another source are now hidden outright rather than dimmed: they are not stale, they are wrong. Required rather than defaulted, because a default would silently opt every caller out. - `pickerFor` tags list-backed options with the type they were loaded for, on both RerunCollectionsScreen and PlaylistsScreen, so the previous type's rows stop being selectable the instant the active type changes rather than lingering through the replacement load. HIGH-3: a touched identity contradicting the server's type is a CONFLICT. Pinning the user's edit was right; adopting the response's newest ETag alongside it authorized a Save that silently overwrote the collaborator's type change with no 412. The conflict dialog is raised and the stale ETag kept, so even a forced Save 412s. With round 3's Reload defect this is the second cross-user lost update, so it is recorded as a category: never install a save-authorizing ETag over a local edit the server contradicts. MEDIUM-4: the editor is inert while a Reload is pending. The dialog closes immediately, so an edit typed before the replacement landed was silently erased along with the touched set that protected it. MEDIUM-5: cached search provenance records `ok`, so a transient 500 is retried instead of being cached as an authoritative "No matches" that reopening can never clear. Also: `npx tsc --noEmit` typechecks NOTHING in web/ — the root tsconfig is solution-style ("files": [] + references), so it resolves to zero inputs and exits 0. The real gate, and what CI runs, is `npm run typecheck` (tsc -b). Verified by planting a deliberate type error: --noEmit stayed green, -b caught it. Running the real gate surfaced four genuine errors in tests written earlier this branch (a missing required prop and three `never has no call signatures` from closure-assigned mock variables), fixed with the repo's existing holder-object pattern rather than casts. Recorded in spa-conventions so the next session doesn't repeat it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>HIGH: the fail-closed gate rejected `null` but not the adjacent values. `Headers.get('ETag')` returns `''` for an empty or whitespace-only header, which PASSED the gate and produced an editable draft; `updateRerunCollection`'s `ifMatch ? … : undefined` then dropped the empty string as falsy and sent no `If-Match`, silently overwriting a collaborator — the exact class the gate exists to make unreachable, reached through the value next door. Absent, empty and whitespace are now one case ("no usable concurrency token"), normalized by a single `usableEtag` helper that returns the TRIMMED token or null, so `etagRef` can only ever hold something that will actually be sent. Tested across four blank shapes asserting zero PUTs are reachable, plus a padded ETag that must be trimmed and USED rather than dropped. MEDIUM: the deadline abandoned the wait without cancelling the work, so each Retry stacked another live connection. It now aborts via an AbortSignal (threaded through `getRerunCollectionWithMeta`) AND clears its timer on settlement and unmount. Both halves are kept deliberately: aborting cancels the work, while the rejected race stops the UI waiting even if the abort never propagates — cancellation and giving-up are not the same guarantee, which the late-settlement test proves by using a stub that ignores its signal. MEDIUM: `Number.isFinite` accepted ids the API cannot bind — `1.5` and values outside int32 rendered, committed through `onSelect`, and would fail server-side on `selectedId`. Validated as an int32 integer. MEDIUM: a malformed or failed page was reported as "No matches", telling the user the library is empty when the request actually failed and giving no hint that reopening retries. Failures now surface as a distinct alert. MEDIUM: `spa-conventions.md` still mandated the deleted "never let a refresh clear an id it failed to name" guard and said "the client guard stays" — contradicting the initialize-once bullet 20 lines below it. Rewritten to state that the guard is gone and must not be rebuilt, with the reason (it only ever preserved a list-seeded value that is null in production). Grepping the DELETED TERMS across all docs — the lesson from round 6's stale `rule:` — also caught two stale `signals:` tokens on the record that the rule fix had missed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>The review's headline finding was in my prose, not my code: spa-conventions and the round-8 commit both claimed an unbindable id "surfaces as 'no selection' with Save disabled" and that regressions "assert zero writes are reachable". True on RerunCollections and FillerPresets. False on PlaylistsScreen in all three respects — `draftFromItem` nulled the id but KEPT `selectedName`, so the row read "Cool Movie" over a null draft; Save had no selection check; and clicking it did issue the PUT with `mediaItemId: null`. Only the server's `ReplacePlaylistItemsHandler` 422 stood there, and the DB would have persisted it (`PlaylistItemConfiguration` marks all four FKs `IsRequired(false)`). Rather than weaken the claim, made it true: a dropped id now clears its label, and Save is gated on every item having a selection, with a visible count as the reason. `playlistGroupId` was the same class on the same screen — seeded from the wire into `AddPlaylistDialog`, re-parsed with a bare `Number()`, and POSTed as an entity reference — so "every path by which an id from the wire becomes editor state" was not literally true. Now filtered from the group options and normalized on submit. Added `selectionId.test.ts`. The predicate had become the single point of failure for eleven call sites across three screens while being exercised only indirectly; nothing pinned the inclusive endpoints, so a `>` for `>=` slip passed the entire suite. Verified by mutating each comparison. Also documented why `0` and negatives are accepted — the contract is bindability, not existence — because every other id check in this repo uses `id > 0` and the next reader would otherwise "fix" the inconsistency. Two of my assertions were vacuous, the eighth of that shape on this branch: one clicked a button it had just asserted disabled (a restatement of `toBeDisabled()`), and one asserted a POST count on a path that never attempted a save. The first is deleted; the second now actually attempts the write, which makes it fail against the unguarded parent. Corrected claim: all five round-8 regressions do fail against their parent, but on their load-bearing assertions (`getByText('A selection is required')`, `queryByText('Bogus Collection')`) — not on the write-count ones, which were passengers. Follow-up filed as #677 (ScheduleItemInspector's unguarded ingresses; list-backed pickers dropping malformed options silently). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>Review-verdict: MERGEABLE @
66c8500Nine cold review rounds. Final delta (3 one-liners: an unconditional toHaveLength(0), verb pluralisation, a comment reword) verified directly by me against the prior BLOCKED review's exact asks. NOTE: the last two passes were SAME-FAMILY (cold, isolated, no implementation role) because Codex quota is exhausted - a real reduction in independence. One intermittent full-suite red under a 20x machine slowdown did not reproduce in 5 subsequent clean runs; attributed to contention, not proven. All CI green on this head.