fix(685): AddItemsDialog resolves by search instead of windowing the whole media-library type #741
No Assignees
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: timothy/ersatztv#741
Reference in New Issue
Block a user
fixes #685
AddItemsDialog.runSearchwas reachable with an empty query two ways — a blank form submit, and a kind-chip click which called it immediately — andgetLibraryBrowseItemsomits a falsyquery. So each path degraded into an unfiltered browse of the whole media-library type (first 50 rows, per kind) presented as the answer, with nothing surfacing the truncation.merged.slice(0, 50)separately dropped up to 100 of 150 fetched rows even for a real query.All ten
ADDABLE_TYPE_LISTentries arespa-conventions.md§3b Class B media-library types.Shape of the fix
The dialog is multi-select (checkbox rows, many items added at once, fanned out over several kinds), so §3b's
SearchPicker— single-select — does not fit. It takes §3b's constraints instead, and they live in a shared helper rather than at the call site:searchLibraryBrowseItems(web/src/api/libraryBrowse.ts), a sibling ofsearchLibraryPickerOptions, owns theLIBRARY_PICKER_MIN_QUERYgate, theLIBRARY_PICKER_RESULTSclamp and thetitleContainsQuerycompile. It returns fullLibraryBrowseItemrows plustotalCount(the{id, name}shape loses themediaTypethattoAddItemsRequestneeds).merged.slice(0, 50)is removed rather than hinted at; the per-kind cap that can still truncate is surfaced asShowing N of M matches.Results for "<query>"label keyed to the settled query, so rows are never shown without saying which search produced them.MediaKindFilter,DEFAULT_SEARCH_KINDS) are derived fromADDABLE_TYPE_LIST, making the hint's pre-filter/post-filter arithmetic a compile-time property.Two corrections to the issue body
ADDABLE_TYPESincludes collection-family types needing a truncation hint. It does not — all ten entries are Class B media types. Done-when box 4 has no subject on this screen.SearchPicker, which is single-select and cannot serve this dialog.Review
Four independent adversarial review rounds (cold brief, isolated worktree). Round 2 returned BLOCKED: the helper's bound was dead code to the suite — deleting its clamp left 1119/1119 green while the registry note claimed callers "cannot skip the bound" — and a duplicated gate in the screen masked the helper's, so the boundary test pinned nothing. Both fixed and re-verified by mutation. Rounds 4 and 5 confirmed no new defects.
Every gate and guard here has at least one test that reddens when that gate alone is removed; each was verified by mutating the single clause.
Verification
npm run typecheckclean ·npm run lintclean · 1199/1199 tests green, on currentorigin/main.No live-E2E: pure-SPA change, no API or server-state effect.
Docs
docs/spa-conventions.md§3b — records that multi-select pickers satisfy §3b withoutSearchPicker, corrects a parent paragraph that contradicted the new hint, and carries the lesson that a gate's home is the shared helper however single-sink the screen's own function looks.Follow-up filed: #740 (pre-existing missing
seqRef/useIsMountedRefguards on this screen, explicitly not introduced here).Independent review round 2. Verdict was MERGEABLE with no blockers; this takes the two recommended fixes plus the structural one it listed as a follow-up. - The bound was caller discipline, not code: getLibraryBrowseItems does not clamp pageSize, so the bound was only the constant this one call site chose to pass, and §3b is explicit that a bound a caller can exceed is not a bound. New searchLibraryBrowseItems in libraryBrowse.ts owns the min-query gate, the pageSize clamp and the titleContainsQuery compile, returning full LibraryBrowseItem rows plus totalCount (searchLibraryPickerOptions' {id,name} shape loses the mediaType that toAddItemsRequest needs). runSearch keeps one early return, for the spinner only, and no longer re-implements the gate. - The min-query guidance was keyed to the LIVE input, so backspacing below the gate after a search wiped the rendered rows and their checkmarks while `selected` and the Add button still counted them. Keyed to results.length too. - "Nothing left to hint at" was false: each kind is still capped at LIBRARY_PICKER_RESULTS and totalCount was never read. This is a bulk multi-select add, so the cap is surfaced — per-kind totalCounts are summed and rendered as "Showing N of M matches" once it exceeds the rendered rows. The registry note and the §3b bullet are corrected to stop claiming otherwise. - Gate boundary tested at 1 character (§3b: inclusive endpoints, or a > for >= slip passes the whole suite). - The guard test's deviation loop iterates an empty list now, so it gains one bidirectional assertion that is non-vacuous: the set carrying an `issue` field must equal the set classified 'deviation'. - The §3b bullet no longer reads as a conformance certificate: AddItemsDialog still lacks the seqRef and useIsMountedRef guards §3b mandates. That defect is PRE-EXISTING, not introduced here, and is tracked in #740. refs #740Verification round returned MERGEABLE with all three blocking findings resolved by measurement. These are its four remaining items. - DEFAULT_SEARCH_KINDS was the SECOND ingress into the searched kinds and was not derived, so the previous commit's "enforced by the type system" claim held for one of two paths. A non-addable kind there typechecked clean and would have overstated the hint with every one of its rows dropped — the exact defect the derivation exists to prevent. Now derived; verified by mutation that adding 'Collection' to it is a compile error. - The error path fell into the min-query guidance branch, so a valid 2-character query that got a 500 told the user to type at least 2 characters. That branch conflated "nothing searched yet" with "the last search failed". Newly introduced by the previous commit's error-path reset; now gated on !error and pinned by a test. - The aria-live region was mounted conditionally, creating the region and its text in one commit — which most screen readers do not announce. It is now mounted unconditionally with the condition inside. - The §3b lesson mis-stated where the duplicate gate lived: it was inside runSearch, the genuine single sink, NOT at one of the callers — so the rule as written ("put the gate in the single sink, not at each caller") described the revision that was rejected. Reworded to the actual lesson: the gate's home is the shared helper, and "it's the single sink" is not evidence it is the only guard. That misreading is why #685 got this wrong twice. Declined again, with reasons: the NaN pageSize edge (faithful to the sibling helper), the clamp test's unpinned lower bound (same), and the registry's disclosed same-identity substitution gap. refs #740Review-verdict: MERGEABLE @
7be4265Four independent adversarial rounds (cold brief, isolated worktree, Opus). Round 2 BLOCKED: helper bound was dead code to the suite (deleting the clamp left 1119/1119 green) and a duplicated screen gate masked the helper's, so the boundary test pinned nothing. Both fixed, re-verified by independent mutation. Rounds 4 and 5 confirmed no new defects; all severity bands empty. typecheck+lint clean, 1199/1199.