diff --git a/docs/spa-conventions.md b/docs/spa-conventions.md index a1261376e..41e72f174 100644 --- a/docs/spa-conventions.md +++ b/docs/spa-conventions.md @@ -199,11 +199,15 @@ the #644 follow-up got Class A right and Class B only half right): match count — this is a bulk multi-select add, where "add the 40 matching episodes" is a first-class use, so `AddItemsDialog` sums each kind's `totalCount` and renders a `Showing N of M matches` hint once it exceeds the rendered rows (finding 4 — an earlier revision of this - bullet called the truncation nothing left to hint at). **Put the gate in the single search - sink, not at each caller**: `AddItemsDialog` has two entry paths (blank form submit, kind-chip - click), and #685's first fix duplicated the check at one of them, which measurably tested - nothing — deleting the duplicate reddened no test, while deleting the sink's gate reddened both - paths. **Outstanding on this screen**: `AddItemsDialog` still lacks the monotonic `seqRef` + bullet called the truncation nothing left to hint at). **The gate's home is the shared HELPER, + not the screen — however single-sink the screen's own function looks.** #685 got this wrong + twice in a row, and the second time is the instructive one: the check sat inside `runSearch`, + which genuinely IS the one sink both entry paths route through, so it read as correct. It was + still a duplicate of the helper's gate, and the two masked each other — with both present, + deleting EITHER left the whole suite green, so the min-query boundary test pinned nothing. + Measured after removing the screen's copy: deleting the helper's gate alone reddens that test + and three more. A guard you cannot redden is not a guard, and "it's the single sink" is not + evidence that it is the only one. **Outstanding on this screen**: `AddItemsDialog` still lacks the monotonic `seqRef` stale-response guard and `useIsMountedRef()` — the same class of guard "Debounced typeaheads" below mandates there, applied to a debounced-while-typing fetch; `AddItemsDialog` is an explicit Search-button submission, not a typeahead, so that mandate doesn't reach it directly, but the diff --git a/web/src/api/pageSizeCallSites.guard.test.ts b/web/src/api/pageSizeCallSites.guard.test.ts index bdc597b3f..d729f0e3a 100644 --- a/web/src/api/pageSizeCallSites.guard.test.ts +++ b/web/src/api/pageSizeCallSites.guard.test.ts @@ -187,10 +187,13 @@ const REGISTRY: RegistryEntry[] = [ "real match count below what totalCount reports, and AddItemsDialog now sums each kind's " + "totalCount and renders a 'Showing N of M' hint when it exceeds the rendered rows. The only " + 'remaining client-side filter in AddItemsDialog (ADDABLE_TYPES.has(item.mediaType)) is inert, ' + - 'not a silent drop, and this is now enforced by the type system rather than by convention: ' + - "MediaKindFilter is derived from ADDABLE_TYPE_LIST (`(typeof ADDABLE_TYPE_LIST)[number]`), so " + - 'adding a non-addable kind to MEDIA_KIND_FILTERS is a compile error, not a silently-possible ' + - 'mismatch.' + 'not a silent drop, and this is now enforced by the type system rather than by convention on ' + + 'BOTH ingress paths into the searched kinds — MediaKindFilter (the explicit-chip path) and ' + + 'DEFAULT_SEARCH_KINDS (the `all` fan-out) are each derived from ADDABLE_TYPE_LIST via ' + + '`(typeof ADDABLE_TYPE_LIST)[number]`, so adding a non-addable kind to either is a compile ' + + 'error. Enforcing only the first was the #685 round-3 review finding: the hint sums ' + + 'PRE-filter totalCounts against POST-filter rows, so one unenforced ingress is enough to ' + + 'overstate it with every row of that kind dropped.' }, { file: 'api/paging.ts', diff --git a/web/src/screens/CollectionsScreen.test.tsx b/web/src/screens/CollectionsScreen.test.tsx index 831ce49fd..237919112 100644 --- a/web/src/screens/CollectionsScreen.test.tsx +++ b/web/src/screens/CollectionsScreen.test.tsx @@ -641,6 +641,14 @@ describe('CollectionsScreen', () => { }); expect(within(dialog).queryByText(/Showing .* of .* matches/)).not.toBeInTheDocument(); expect(fetchMock).toHaveBeenCalled(); + + // A failed search must NOT fall into the min-query guidance branch: the user typed a valid + // 2-character query and got a 500, so telling them to "type at least 2 characters" states + // something false about what they just did. The error banner is the whole message (#685 + // review round 3 — that branch conflated "nothing searched yet" with "the last search failed"). + expect( + within(dialog).queryByText(`Type at least ${LIBRARY_PICKER_MIN_QUERY} characters to search.`) + ).not.toBeInTheDocument(); }); /* ---------- add-items search bounds (#685) ---------- */ diff --git a/web/src/screens/CollectionsScreen.tsx b/web/src/screens/CollectionsScreen.tsx index a43412187..0165ace4b 100644 --- a/web/src/screens/CollectionsScreen.tsx +++ b/web/src/screens/CollectionsScreen.tsx @@ -76,7 +76,12 @@ const ADDABLE_TYPES = new Set(ADDABLE_TYPE_LIST) // The default fan-out excludes seasons so a multi-season show doesn't flood the // results with per-season rows (issue #180); seasons (and the other narrower kinds // added for #211) stay reachable via the explicit media-kind filter below. -const DEFAULT_SEARCH_KINDS: LibraryBrowseItem['mediaType'][] = ['Movie', 'TelevisionShow', 'Artist']; +// Derived from ADDABLE_TYPE_LIST for the same reason MediaKindFilter is: this is the SECOND +// ingress into `kinds` (the `all` branch of runSearch), and the truncation hint sums pre-filter +// totalCounts against post-ADDABLE_TYPES rows. A non-addable kind here would overstate the hint +// with every one of its rows dropped, and typing it as the whole mediaType union let that through +// (#685 review round 3 — one of two ingress paths was enforced, which is not an invariant). +const DEFAULT_SEARCH_KINDS: (typeof ADDABLE_TYPE_LIST)[number][] = ['Movie', 'TelevisionShow', 'Artist']; type MediaKindFilter = 'all' | (typeof ADDABLE_TYPE_LIST)[number]; @@ -390,13 +395,14 @@ function AddItemsDialog({ Results for “{submittedQuery}”

)} - {totalMatches > results.length && ( -

- Showing {results.length} of {totalMatches} matches — narrow your search. -

- )} + {/* The live region is mounted UNCONDITIONALLY with the condition inside it: most screen + readers only announce mutations to a region that already existed, so creating the region + and its text in the same commit announces nothing (#685 review round 3). */} +

+ {totalMatches > results.length ? `Showing ${results.length} of ${totalMatches} matches — narrow your search.` : ''} +

- {results.length === 0 && submittedQuery === '' ? ( + {results.length === 0 && submittedQuery === '' && !error ? ( // Keyed to submittedQuery, not the live input: without it, backspacing the query back // below the min-query length after a successful search wiped the rendered rows AND their // checkmarks while `selected` (and the Add button's count) still held them (#685 review