fix(685): close the second ingress; stop the error path claiming a short query

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
2026-08-05 19:20:26 +02:00
parent b6bf94f129
commit d7725c274c
4 changed files with 37 additions and 16 deletions
@@ -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
@@ -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',
@@ -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) ---------- */
@@ -76,7 +76,12 @@ const ADDABLE_TYPES = new Set<LibraryBrowseItem['mediaType']>(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 &ldquo;{submittedQuery}&rdquo;
        </p>
      )}
-      {totalMatches > results.length && (
-        <p aria-live="polite" className="ctv-collections-picker-note">
-          Showing {results.length} of {totalMatches} matches — narrow your search.
-        </p>
-      )}
+      {/* 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). */}
+      <p aria-live="polite" className="ctv-collections-picker-note">
+        {totalMatches > results.length ? `Showing ${results.length} of ${totalMatches} matches — narrow your search.` : ''}
+      </p>
      <div className="ctv-collections-picker-results">
-        {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