fix(search): keep the target selector inside the set it offers

Round-2 review of cf5c69e235. Making both target selectors controlled closed a
label desync and opened a narrower one, and the deselect path the controlled
value exposed turns out to reach a payout field.

1. A controlled `<Select>` whose value is not among its options keeps showing
   the PREVIOUS option's label. Mantine's value→label sync runs only when the
   value resolves to an option (`Select.mjs`: the `[value, selectedOption]`
   effect takes neither branch otherwise) and the uncontrolled fallback that
   used to clear it is skipped once `value` is passed. The header search reaches
   that state without any user action: its target follows the URL section, and a
   section whose feature flag is off is filtered out of the options — so on
   `/images` with image search disabled the selector read "Models" while the
   provider searched the images index. Uncontrolled it read blank. Both
   selectors now pass `null` when the target is not an offered option, which is
   what blanks it; the offered set is hoisted so the value is clamped to the
   same list the options are built from.

2. `QuickSearchDropdown`'s selector did not set `allowDeselect`, and Mantine
   defaults it to `true` — so a single-option selector is deselectable, and the
   deselect hands `null` to the change handler, which fell back to `models`.
   Callers read the picked entity as the type their `supportedIndexes` names;
   `CosmeticShopItemUpsertForm` passes `['users']` with the selector visible and
   writes the picked id into `meta.paidToUserIds`, which
   `cosmetic-shop.service.ts` splits a price across. `allowDeselect={false}`,
   and the fallback now goes to the first supported index rather than to
   `models`. Pre-existing on both counts; the previous commit fixed the
   initial-state half of the same inconsistency and left the runtime half.

3. Corrections to claims, no behaviour change. The `supportedIndexes` fallback
   comment described an observed defect; enumerating all ten call sites shows
   every one either passes `startingIndex` or supports `models` first, so it is
   a forward guard and now says so. `shouldRefineSearchQuery`'s docblock
   required every input to appear in the calling effect's dependency array,
   which `selectedItem` does not — the rule is real only for a source that
   OUTLIVES the remount, and the sentence now says that and names both cases.

Tests: 22, unchanged in count, 7 still RED with the two components at
bbeffcf71e. Five mutants that survived round 2 now die:

  - the URL-follow sync left in place AND re-added inside the subtree (the
    consolidation that forgets to delete one copy — this restores the reverted
    category pick with every other assertion satisfied)
  - its dependency array emptied, so it stops following navigation
  - `searchErrorState` dropped from the predicate ARGUMENT while left in the
    dependency array, which the dep-array check alone cannot see
  - the selector value unclamped
  - `allowDeselect` removed

Two spellings named in review are accepted as still walkable: an index written
`searchIndexMap['models']` or as an imported constant satisfies the
constant-index check, because a source-text guard cannot follow either. The
comment says what these guards pin — spelling and file order — rather than
implying tree position.

Still not verified: the browser tier does not run on the authoring host, so the
real click path remains unexercised end to end, and finding 1's stale-label
behaviour was established from Mantine's source rather than by rendering it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

src/components/AutocompleteSearch/AutocompleteSearch.tsx

@@ -352,6 +352,17 @@ function AutocompleteSearchContentInner<TKey extends SearchIndexKey>(
     indexName,
   ]);
 
+  // Ensure we disable search targets if they are not enabled. Hoisted because the selector's
+  // value is clamped to this set as well as read from it — the two have to be the same list.
+  const enabledTargets = targetData.filter(
+    ({ value }) =>
+      (features.imageSearch ? true : value !== 'images') &&
+      (features.bounties ? true : value !== 'bounties') &&
+      (features.articles ? true : value !== 'articles') &&
+      (features.toolSearch ? true : value !== 'tools') &&
+      (features.comicSearch ? true : value !== 'comics')
+  );
+
   const focusInput = () => inputRef.current?.focus();
   const blurInput = () => inputRef.current?.blur();
 
@@ -480,7 +491,12 @@ function AutocompleteSearchContentInner<TKey extends SearchIndexKey>(
           // CONTROLLED. Uncontrolled, its displayed label is internal state inside the keyed
           // provider, so a target switch would remount it back to whatever the default said
           // while the search really did move — a selector that lies about what it is searching.
-          value={indexNameProp}
+          //
+          // `null` rather than the target when the target is not an OFFERED option: the URL can
+          // point the search at an index whose feature flag is off, and Mantine leaves a
+          // controlled value it cannot resolve showing the PREVIOUS option's label. Blank is
+          // honest about "none of these"; a stale label is the same lie in a different place.
+          value={enabledTargets.some(({ value }) => value === indexNameProp) ? indexNameProp : null}
           aria-label="Search category"
           classNames={{
             root: classes.targetSelectorRoot,
@@ -493,15 +509,7 @@ function AutocompleteSearchContentInner<TKey extends SearchIndexKey>(
             className: classes.targetSelectorRightSection,
           }}
           maxDropdownHeight={280}
-          // Ensure we disable search targets if they are not enabled
-          data={targetData.filter(
-            ({ value }) =>
-              (features.imageSearch ? true : value !== 'images') &&
-              (features.bounties ? true : value !== 'bounties') &&
-              (features.articles ? true : value !== 'articles') &&
-              (features.toolSearch ? true : value !== 'tools') &&
-              (features.comicSearch ? true : value !== 'comics')
-          )}
+          data={enabledTargets}
           rightSection={<IconChevronDown size={16} color="currentColor" />}
           style={{ flexShrink: 1 }}
           onChange={(v: string | null) => onTargetChange(v as TKey)}

src/components/Search/QuickSearchDropdown.tsx

@@ -143,16 +143,18 @@ export const QuickSearchDropdown = ({
   disableInitialSearch,
   ...props
 }: QuickSearchDropdownProps) => {
-  // Falling back to the first SUPPORTED index rather than to `models`: the selector's options are
-  // `supportedIndexes`, so a caller that supplies `['users']` and no `startingIndex` used to
-  // display "Users" while the provider really targeted `models`. Now that the selector is
-  // controlled by this value, that disagreement would show as an empty selector instead of being
-  // invisible — so the two are made to agree at the source.
-  const [targetIndex, setTargetIndex] = useState<SearchIndexKey>(
-    startingIndex ?? props.supportedIndexes?.[0] ?? 'models'
-  );
+  // The target has to stay inside the set the selector OFFERS, at both ends. A bare `models`
+  // fallback can leave the component searching an index the caller never supported — a caller
+  // that passes `['users']` gets a users picker whose hits are models — and now that the selector
+  // is controlled by this value, a target outside the offered set also blanks the control.
+  //
+  // FORWARD GUARD, not a fix for an observed defect: every current caller either passes
+  // `startingIndex` or supports `models` first, so neither fallback moves any call site today.
+  // The deselect path below is the one that could reach it, and it is closed here too.
+  const fallbackIndex = startingIndex ?? props.supportedIndexes?.[0] ?? 'models';
+  const [targetIndex, setTargetIndex] = useState<SearchIndexKey>(fallbackIndex);
   const handleTargetChange = (value: SearchIndexKey | null) => {
-    setTargetIndex(value ?? 'models');
+    setTargetIndex(value ?? fallbackIndex);
   };
   // Owned above the keyed search provider below, so it outlives the remount an index switch
   // causes.
@@ -292,6 +294,17 @@ function QuickSearchDropdownContent<TIndex extends SearchIndexKey>({
   // request itself. `isSearchStalled` alone leaves the first 300ms looking like a dead input.
   const loading = search.length > 0 && (search !== query || isSearchStalled);
 
+  // Ensure we disable search targets if they are not enabled. Hoisted because the selector's
+  // value is clamped to this set as well as read from it — the two have to be the same list.
+  const enabledTargets = availableIndexes
+    .filter(
+      (value) =>
+        (features.imageSearch ? true : searchIndexMap[value] !== IMAGES_SEARCH_INDEX) &&
+        (features.toolSearch ? true : searchIndexMap[value] !== TOOLS_SEARCH_INDEX) &&
+        (features.articles ? true : value !== 'articles')
+    )
+    .map((index) => ({ label: IndexToLabel[searchIndexMap[index]], value: index }));
+
   return (
     <Group className={classes.wrapper} gap={0} wrap="nowrap">
       {!!showIndexSelect && (
@@ -306,18 +319,19 @@ function QuickSearchDropdownContent<TIndex extends SearchIndexKey>({
           // CONTROLLED. Uncontrolled, its displayed label is internal state inside the keyed
           // provider, so a target switch would remount it back to the first supported index
           // while the search really did move — a selector that lies about what it is searching.
-          value={indexNameProp}
-          // Ensure we disable search targets if they are not enabled
-          data={availableIndexes
-            .filter(
-              (value) =>
-                (features.imageSearch ? true : searchIndexMap[value] !== IMAGES_SEARCH_INDEX) &&
-                (features.toolSearch ? true : searchIndexMap[value] !== TOOLS_SEARCH_INDEX) &&
-                (features.articles ? true : value !== 'articles')
-            )
-            .map((index) => ({ label: IndexToLabel[searchIndexMap[index]], value: index }))}
+          //
+          // `null` rather than the target when the target is not an OFFERED option: Mantine leaves
+          // a controlled value it cannot resolve showing the PREVIOUS option's label, which is the
+          // same lie in a different place. Blank is honest about "none of these".
+          value={enabledTargets.some(({ value }) => value === indexNameProp) ? indexNameProp : null}
+          data={enabledTargets}
           rightSection={<IconChevronDown size={16} color="currentColor" />}
           onChange={(value) => onIndexNameChange(value as TIndex)}
+          // A single-option selector is otherwise DESELECTABLE, and a deselect hands `null` to the
+          // change handler. Callers read the picked entity as the type their `supportedIndexes`
+          // names — one of them writes it into a Buzz payout recipient set — so silently moving
+          // the target to another index is not a display bug.
+          allowDeselect={false}
         />
       )}
       <ClearableAutoComplete

src/components/Search/__tests__/dropdown-index-remount.test.ts

@@ -39,15 +39,33 @@ const INSTANT_SEARCH_ROOTS = {
   'src/components/CollectionSelectModal/CollectionSelectModal.tsx': 'static-index',
 } as const;
 
-/** Every non-test `.tsx` under `src/` that renders an `<InstantSearch>` element, repo-relative. */
+/**
+ * Every non-test `.tsx` under `src/` that renders an `<InstantSearch>` element, repo-relative.
+ *
+ * A full-suite run creates and removes directories under `src/` while this walk is happening, so
+ * an entry can vanish between the listing and the read. Its sibling ledger in this directory
+ * documents that as an OBSERVED hazard — it surfaces as a collection failure, which contributes
+ * zero tests and moves no failure count — so entries that cannot be read are skipped rather than
+ * thrown on.
+ */
 function findInstantSearchRoots(dir = 'src'): string[] {
   const found: string[] = [];
-  for (const entry of readdirSync(path.join(repoRoot, dir), { withFileTypes: true })) {
+  let entries: ReturnType<typeof readdirSync<{ withFileTypes: true }>>;
+  try {
+    entries = readdirSync(path.join(repoRoot, dir), { withFileTypes: true });
+  } catch {
+    return found;
+  }
+  for (const entry of entries) {
     const relPath = `${dir}/${entry.name}`;
     if (entry.isDirectory()) {
       found.push(...findInstantSearchRoots(relPath));
     } else if (entry.name.endsWith('.tsx') && !entry.name.includes('.test.')) {
-      if (read(relPath).includes('<InstantSearch')) found.push(relPath);
+      try {
+        if (read(relPath).includes('<InstantSearch')) found.push(relPath);
+      } catch {
+        continue;
+      }
     }
   }
   return found;
@@ -168,17 +186,34 @@ describe('the dropdown roots carry the typed text across that remount', () => {
     expect(sync).toBeGreaterThan(-1);
     expect(provider).toBeGreaterThan(sync);
 
+    // EXACTLY ONE writer. Hoisting the sync while leaving the old copy in place reintroduces the
+    // whole defect with the assertion above still satisfied — the consolidation-that-forgot-to-
+    // delete shape, which is the likeliest way this comes back.
+    expect([...source.matchAll(/(?:setTargetIndex|onTargetChange)\(searchTarget/g)]).toHaveLength(
+      1
+    );
+
+    // …and it still FOLLOWS navigation. Emptying its dependency array leaves one writer, in the
+    // right place, that only ever runs once.
+    expect(source.slice(sync)).toMatch(
+      /^\s*setTargetIndex\(searchTarget\);\s*\}, \[searchTarget\]\)/
+    );
+
     // …and the selector reads the target rather than holding its own copy of it, which a remount
     // would reset while the search really had moved.
-    expect(source).toContain('value={indexNameProp}');
+    expect(source).toContain('value === indexNameProp) ? indexNameProp : null');
     expect(source).not.toContain('defaultValue={searchTarget}');
   });
 
   it('QuickSearchDropdown drives its index selector from the target it is searching', () => {
     const source = read('src/components/Search/QuickSearchDropdown.tsx');
 
-    expect(source).toContain('value={indexNameProp}');
+    expect(source).toContain('value === indexNameProp) ? indexNameProp : null');
     expect(source).not.toContain('defaultValue={availableIndexes[0]}');
+
+    // A single-option selector is deselectable by default, and the `null` that produces would
+    // move the target off the set the caller supports — for one caller, into a payout path.
+    expect(source).toContain('allowDeselect={false}');
   });
 
   it('AutocompleteSearch re-runs its refine effect when search availability recovers', () => {
@@ -187,10 +222,21 @@ describe('the dropdown roots carry the typed text across that remount', () => {
     // unavailable restores the typed text, returns early, and never refines once the flag clears
     // — a populated box over an empty helper query. Pinned structurally because only a render of
     // the real component could observe it, and that is the browser tier.
-    const source = read('src/components/AutocompleteSearch/AutocompleteSearch.tsx');
+    // Comments stripped first: a dependency array can otherwise satisfy a token search with the
+    // token sitting inside `/* … */`, which is the walk `openingTag` above already guards against.
+    const source = read('src/components/AutocompleteSearch/AutocompleteSearch.tsx')
+      .replace(/\/\*[\s\S]*?\*\//g, '')
+      .replace(/^\s*\/\/.*$/gm, '');
     const deps = source.match(/\}, \[debouncedSearch, query, indexName[^\]]*\]/);
 
-    expect(deps?.[0]).toContain('searchErrorState');
+    expect(deps?.[0] ?? '(no refine dependency array matched)').toContain('searchErrorState');
+
+    // And the flag has to still be PASSED to the predicate. The dependency array alone cannot see
+    // that: dropping it from the argument while leaving the token in the deps refines DURING an
+    // outage with this test still green.
+    expect(source).toContain(
+      'shouldRefineSearchQuery(debouncedSearch, query, !!selectedItem || searchErrorState)'
+    );
   });
 });
 

src/components/Search/useCarriedSearchText.ts

@@ -58,9 +58,12 @@ export function seedCarriedSearchText(carried: string | undefined, refinedQuery:
  * rebuilt helper reports an empty query, so carried text differs from it and gets pushed.
  *
  * @param blocked reasons not to refine at all — a hit was picked from the list, or search is
- *   unavailable. Every input here must appear in the calling effect's dependency array, `blocked`
- *   included: one of its sources outlives the remount, so an effect that cannot re-run when it
- *   clears leaves a populated input over an empty helper query.
+ *   unavailable. 🔴 A source of `blocked` that OUTLIVES the remount must appear in the calling
+ *   effect's dependency array, or a tree that remounted while blocked restores the typed text,
+ *   returns early, and never refines once the block clears — a populated input over an empty
+ *   helper query. `searchErrorState` is such a source (a module-level store) and is listed.
+ *   A source that is per-mount state is reset by the remount and needs no such listing;
+ *   `AutocompleteSearch`'s `selectedItem` is one, and is deliberately not listed.
  */
 export function shouldRefineSearchQuery(
   typed: string,