From d7725c274c8ee0d968fef8dbf2e66d5933dd6feb Mon Sep 17 00:00:00 2001 From: Timothy Date: Wed, 5 Aug 2026 19:20:26 +0200 Subject: [PATCH] fix(685): close the second ingress; stop the error path claiming a short query MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Verification 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 #740 --- docs/spa-conventions.md | 14 +++++++++----- web/src/api/pageSizeCallSites.guard.test.ts | 11 +++++++---- web/src/screens/CollectionsScreen.test.tsx | 8 ++++++++ web/src/screens/CollectionsScreen.tsx | 20 +++++++++++++------- 4 files changed, 37 insertions(+), 16 deletions(-) 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