mirror of
https://github.com/civitai/civitai.git
synced 2026-09-20 22:08:18 +08:00
fix(search): keep the chosen category when the search provider is rebuilt
Review follow-up to 19adaeb56e. That commit keyed both dropdown
`<InstantSearch>` roots on their index, which is what stops a search firing
with the previous index's parameters — and the remount it introduces reached
two pieces of state nobody had to think about before, because nothing in these
trees had ever unmounted.
1. The header category selector stopped working, on every page.
`AutocompleteSearchContentInner` carried
useEffect(() => {
if (indexNameProp !== searchTarget) onTargetChange(searchTarget);
}, [searchTarget]);
where `searchTarget` comes from `usePathname()`, not from the pick. With no
remount that effect ran only on navigation. Once the provider is keyed,
choosing a category remounts the subtree — and a mount runs every effect
regardless of its dependency array — so the effect read the URL's section,
found it different from the pick, and put the target back. Measured against
a real react-instantsearch tree: user on /models picking Images settled on
`models`, via two mounts and two searches, one of them against an index the
user was immediately bounced off. `searchTarget` is `models` on every page
whose first path segment is not a search target, so this was not an edge.
The sync now lives in `AutocompleteSearch`, above the keyed boundary, for
the same reason the typed-text carrier does: it has to observe navigation
without being restarted by a target switch.
2. Both target selectors were uncontrolled, so their displayed label was state
inside the remounting subtree. A switch reset the label to the default while
the search really had moved — a control that lies about what it is
searching. Both now read the target they are searching.
`QuickSearchDropdown` also defaulted its target to `models` while its
selector offered `supportedIndexes`, so a caller passing `['users']` and no
`startingIndex` showed "Users" over a models search. Invisible while the
selector held its own value; an empty selector once it is controlled. The
default is now the first supported index, which makes the two agree at the
source.
3. `AutocompleteSearch`'s refine effect returns early on `searchErrorState`,
which reads a module-level store and therefore SURVIVES the remount. It was
not in the effect's dependencies, so a tree that remounted while search was
unavailable restored the typed text, returned early, and never refined when
the flag cleared — a populated input over an empty helper query until the
next keystroke. Added to the dependencies.
Both components' refine decisions now go through one `shouldRefineSearchQuery`
predicate rather than two hand-written conditions that had already drifted
apart.
Also: the guard test added earlier on this branch stated, as the reason the
guard exists, that the dropdown roots "cannot" be keyed because remounting
would clear the typed query. This branch falsifies that, and its exclusion rule
("has no key={indexName}") no longer discriminates now that every root is keyed.
Comments only — no assertion changed.
Tests: 22 in `dropdown-index-remount.test.ts`, 7 of them RED with the two
components at 19adaeb56e's parent and green at HEAD. The harness now models the
helper as per-mount state and drives the shipped predicate, so the claim that a
remount RE-RUNS the search is observed (a second refine with the carried text)
rather than asserted by a test name. Added: a negative control that the carrier
is per-instance rather than module-scope, and the blocked/recovered pair.
Mutation sweep, source restored by digest after each and a green re-run after
the batch: 10 mutants, 10 killed, each by an assertion naming its own behaviour.
Two of them — pinning either root's index to a constant — SURVIVED the first
round, because the ledger checked the expression in the tag while both roots
hoist it into a local; the check now follows a bare identifier to its
declaration, and both then die. That pair is the reason this commit's ledger is
worth more than the previous one's.
Not verified: the browser tier does not run on the authoring host, so the real
click path is still unexercised. Finding 1 was reproduced by a subagent against
a real react-instantsearch tree in a scratch harness, not by a test in this
repo; what ships here for it is a structural guard.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -27,7 +27,10 @@ import { InstantSearch, useInstantSearch, useSearchBox } from 'react-instantsear
|
||||
import { ClearableAutoComplete } from '~/components/ClearableAutoComplete/ClearableAutoComplete';
|
||||
import { slugit } from '~/utils/string-helpers';
|
||||
import { autocompleteSearchClient } from '~/components/Search/autocomplete.client';
|
||||
import { useCarriedSearchText } from '~/components/Search/useCarriedSearchText';
|
||||
import {
|
||||
shouldRefineSearchQuery,
|
||||
useCarriedSearchText,
|
||||
} from '~/components/Search/useCarriedSearchText';
|
||||
import { quoteMeiliValue } from '~/components/Search/meili-filter';
|
||||
import { useAutocompleteAvailabilityStore } from '~/components/Search/search-availability.store';
|
||||
import { ModelSearchItem } from '~/components/AutocompleteSearch/renderItems/models';
|
||||
@@ -96,6 +99,18 @@ export const AutocompleteSearch = forwardRef<{ focus: () => void }, Props>(({ ..
|
||||
// causes.
|
||||
const carriedSearchText = useRef('');
|
||||
|
||||
// Follow the section the user navigates to. This has to live ABOVE the keyed provider for the
|
||||
// same reason the carrier does: inside it, the effect would run again on the mount that a
|
||||
// target switch causes, read a section the user has not navigated to, and immediately revert
|
||||
// their pick — so the category selector would only ever "work" when it picked what the URL
|
||||
// already said.
|
||||
const pathname = usePathname();
|
||||
const currentSection = pathname.split('/')[1] || 'models';
|
||||
const searchTarget = targetData.find((t) => t.value === currentSection)?.value ?? 'models';
|
||||
useEffect(() => {
|
||||
setTargetIndex(searchTarget);
|
||||
}, [searchTarget]);
|
||||
|
||||
const isModels = targetIndex === 'models';
|
||||
const isImages = targetIndex === 'images';
|
||||
const supportsPoi = ['models', 'images'].includes(targetIndex);
|
||||
@@ -174,9 +189,6 @@ function AutocompleteSearchContentInner<TKey extends SearchIndexKey>(
|
||||
const isMobile = useIsMobile();
|
||||
const features = useFeatureFlags();
|
||||
const inputRef = useRef<HTMLInputElement>(null);
|
||||
const pathname = usePathname();
|
||||
const currentSection = pathname.split('/')[1] || 'models';
|
||||
const searchTarget = targetData.find((t) => t.value === currentSection)?.value ?? 'models';
|
||||
const domainColor = useDomainColor();
|
||||
|
||||
const { status } = useInstantSearch({
|
||||
@@ -410,7 +422,8 @@ function AutocompleteSearchContentInner<TKey extends SearchIndexKey>(
|
||||
useEffect(() => {
|
||||
// Only set the query when the debounced search changes
|
||||
// and user didn't select from the list
|
||||
if (debouncedSearch === query || selectedItem || searchErrorState) return;
|
||||
if (!shouldRefineSearchQuery(debouncedSearch, query, !!selectedItem || searchErrorState))
|
||||
return;
|
||||
|
||||
// Check if the query is an AIR
|
||||
const air = checkAIR(indexName, debouncedSearch);
|
||||
@@ -424,22 +437,18 @@ function AutocompleteSearchContentInner<TKey extends SearchIndexKey>(
|
||||
|
||||
setQuery(cleanedSearch);
|
||||
setQueryFilters(filters);
|
||||
// `searchErrorState` is a module-level store, so it is the one input here that SURVIVES the
|
||||
// remount an index switch causes. Without it in the deps, a tree that remounted while search
|
||||
// was unavailable restores the typed text, returns early, and then never refines when the
|
||||
// flag clears — the box reads as populated while the fresh helper's query is still empty.
|
||||
// eslint-disable-next-line react-hooks/exhaustive-deps
|
||||
}, [debouncedSearch, query, indexName]);
|
||||
}, [debouncedSearch, query, indexName, searchErrorState]);
|
||||
|
||||
// Clear selected item after search changes
|
||||
useEffect(() => {
|
||||
setSelectedItem(null);
|
||||
}, [debouncedSearch]);
|
||||
|
||||
// Change index target when search target changes
|
||||
useEffect(() => {
|
||||
if (indexNameProp !== searchTarget) {
|
||||
onTargetChange(searchTarget as TKey);
|
||||
}
|
||||
// eslint-disable-next-line react-hooks/exhaustive-deps
|
||||
}, [searchTarget]);
|
||||
|
||||
const processHitUrl = (hit: Hit) => {
|
||||
switch (indexName) {
|
||||
case 'articles':
|
||||
@@ -468,7 +477,10 @@ function AutocompleteSearchContentInner<TKey extends SearchIndexKey>(
|
||||
/>
|
||||
<Group className={classes.wrapper} gap={0} wrap="nowrap">
|
||||
<Select
|
||||
key={pathname}
|
||||
// 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}
|
||||
aria-label="Search category"
|
||||
classNames={{
|
||||
root: classes.targetSelectorRoot,
|
||||
@@ -481,7 +493,6 @@ function AutocompleteSearchContentInner<TKey extends SearchIndexKey>(
|
||||
className: classes.targetSelectorRightSection,
|
||||
}}
|
||||
maxDropdownHeight={280}
|
||||
defaultValue={searchTarget}
|
||||
// Ensure we disable search targets if they are not enabled
|
||||
data={targetData.filter(
|
||||
({ value }) =>
|
||||
|
||||
@@ -25,7 +25,10 @@ import type { ShowcaseItemSchema } from '~/server/schema/user-profile.schema';
|
||||
import { paired } from '~/utils/type-guards';
|
||||
import { searchClient } from '~/components/Search/search.client';
|
||||
import { quickSearchClient } from '~/components/Search/quick-search.client';
|
||||
import { useCarriedSearchText } from '~/components/Search/useCarriedSearchText';
|
||||
import {
|
||||
shouldRefineSearchQuery,
|
||||
useCarriedSearchText,
|
||||
} from '~/components/Search/useCarriedSearchText';
|
||||
import { BrowsingLevelFilter } from './CustomSearchComponents';
|
||||
import { ToolSearchItem } from '~/components/AutocompleteSearch/renderItems/tools';
|
||||
import { ComicsSearchItem } from '~/components/AutocompleteSearch/renderItems/comics';
|
||||
@@ -140,7 +143,14 @@ export const QuickSearchDropdown = ({
|
||||
disableInitialSearch,
|
||||
...props
|
||||
}: QuickSearchDropdownProps) => {
|
||||
const [targetIndex, setTargetIndex] = useState<SearchIndexKey>(startingIndex ?? 'models');
|
||||
// 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'
|
||||
);
|
||||
const handleTargetChange = (value: SearchIndexKey | null) => {
|
||||
setTargetIndex(value ?? 'models');
|
||||
};
|
||||
@@ -272,7 +282,7 @@ function QuickSearchDropdownContent<TIndex extends SearchIndexKey>({
|
||||
useEffect(() => {
|
||||
// Only set the query when the debounced search changes
|
||||
// and user didn't select from the list
|
||||
if (debouncedSearch === query) return;
|
||||
if (!shouldRefineSearchQuery(debouncedSearch, query)) return;
|
||||
|
||||
setQuery(debouncedSearch);
|
||||
// eslint-disable-next-line react-hooks/exhaustive-deps
|
||||
@@ -293,7 +303,10 @@ function QuickSearchDropdownContent<TIndex extends SearchIndexKey>({
|
||||
section: classes.targetSelectorRightSection,
|
||||
}}
|
||||
maxDropdownHeight={280}
|
||||
defaultValue={availableIndexes[0]}
|
||||
// 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(
|
||||
|
||||
@@ -8,6 +8,7 @@ import { afterEach, describe, expect, it } from 'vitest';
|
||||
|
||||
import {
|
||||
seedCarriedSearchText,
|
||||
shouldRefineSearchQuery,
|
||||
useCarriedSearchText,
|
||||
} from '~/components/Search/useCarriedSearchText';
|
||||
|
||||
@@ -69,6 +70,21 @@ function propExpression(tag: string, prop: string): string | null {
|
||||
return match ? match[1].trim() : null;
|
||||
}
|
||||
|
||||
/**
|
||||
* Follow a bare identifier to its local `const` initializer, so the checks below see what the
|
||||
* index really IS rather than what it is spelled. A root that hoists the expression into a local
|
||||
* (both dropdowns do) would otherwise satisfy any check on the tag no matter what the local held
|
||||
* — a `const x = searchIndexMap.models` mutant survives the whole file without this.
|
||||
*
|
||||
* An identifier with no local `const` is a prop or a piece of state (`SearchLayout`'s `indexName`
|
||||
* is a prop), which is dynamic by construction; it is returned unchanged and passes.
|
||||
*/
|
||||
function resolveIndexExpression(source: string, expression: string): string {
|
||||
if (!/^[A-Za-z_$][\w$]*$/.test(expression)) return expression;
|
||||
const declaration = source.match(new RegExp(`\\bconst ${expression}\\s*=\\s*([^;\\n]+)`));
|
||||
return declaration ? declaration[1].trim() : expression;
|
||||
}
|
||||
|
||||
describe('the InstantSearch roots', () => {
|
||||
it('is the set this ledger accounts for', () => {
|
||||
// Derived from the tree, so the ledger fails when the population grows OR shrinks — a new
|
||||
@@ -87,7 +103,8 @@ describe('the InstantSearch roots', () => {
|
||||
}
|
||||
|
||||
it(`${relPath} keys its provider on the very expression it passes as indexName`, () => {
|
||||
const tag = openingTag(read(relPath));
|
||||
const source = read(relPath);
|
||||
const tag = openingTag(source);
|
||||
const indexName = propExpression(tag, 'indexName');
|
||||
const key = propExpression(tag, 'key');
|
||||
|
||||
@@ -95,14 +112,26 @@ describe('the InstantSearch roots', () => {
|
||||
// they can disagree and the provider survives a switch it was supposed to be rebuilt for.
|
||||
expect(indexName).toBeTruthy();
|
||||
expect(key).toBe(indexName);
|
||||
|
||||
// And the index has to TRACK something — the inverse of the `static-index` branch's check,
|
||||
// so the two policies are mutually exclusive. Without it, pinning the index to one constant
|
||||
// leaves key and index still agreeing while the target selector stops switching index at
|
||||
// all in production, and every assertion above stays green.
|
||||
const resolved = resolveIndexExpression(source, indexName as string);
|
||||
expect(resolved).not.toMatch(/^searchIndexMap\.[A-Za-z]+$/);
|
||||
expect(resolved).not.toMatch(/^['"`]/);
|
||||
});
|
||||
}
|
||||
});
|
||||
|
||||
describe('the dropdown roots carry the typed text across that remount', () => {
|
||||
// Keying the provider remounts the subtree the typed text lives in, so each of these two has to
|
||||
// hold that text ABOVE the provider. Asserted structurally because the components themselves are
|
||||
// browser-tier; the carry mechanism's behaviour is exercised further down.
|
||||
// hold that text ABOVE the provider. These are SPELLING-AND-FILE-ORDER checks, not tree-position
|
||||
// ones: they pin that the declaration is written before the provider in the same file and that
|
||||
// the ref is threaded and read by name. A rename, or moving the content component above the
|
||||
// provider in the file, breaks them for a non-defect. They are the only coverage available here
|
||||
// — the components are browser-tier, and the node project collects `.test.ts` only. The carry
|
||||
// MECHANISM's behaviour is exercised further down, against a real remount.
|
||||
const dropdowns = [
|
||||
'src/components/AutocompleteSearch/AutocompleteSearch.tsx',
|
||||
'src/components/Search/QuickSearchDropdown.tsx',
|
||||
@@ -120,11 +149,49 @@ describe('the dropdown roots carry the typed text across that remount', () => {
|
||||
expect(source).toContain('carriedSearchText={carriedSearchText}');
|
||||
expect(source).toContain('useCarriedSearchText(carriedSearchText, query)');
|
||||
|
||||
// The shape this replaced. State seeded from the helper's own query comes back EMPTY on a
|
||||
// remount, which is the regression a re-introduced `useState(query)` would be.
|
||||
expect(source).not.toContain('useState(query)');
|
||||
// Both refine decisions go through the one predicate, so there is a single place where
|
||||
// "does this tree still owe its text to the helper" is decided.
|
||||
expect(source).toContain('shouldRefineSearchQuery(');
|
||||
});
|
||||
}
|
||||
|
||||
it('AutocompleteSearch follows the URL section from ABOVE the keyed provider', () => {
|
||||
// MEASURED REGRESSION, not a hypothetical. This sync used to live inside the subtree with
|
||||
// `[searchTarget]` deps, and `searchTarget` comes from the pathname rather than from the pick.
|
||||
// A mount runs every effect, so once the provider is keyed, choosing a category remounts the
|
||||
// subtree, the effect sees the URL's section instead of the pick, and reverts it — the header
|
||||
// category selector then only ever "works" when it picks what the URL already said.
|
||||
const source = read('src/components/AutocompleteSearch/AutocompleteSearch.tsx');
|
||||
|
||||
const sync = source.indexOf('setTargetIndex(searchTarget)');
|
||||
const provider = source.indexOf('<InstantSearch');
|
||||
expect(sync).toBeGreaterThan(-1);
|
||||
expect(provider).toBeGreaterThan(sync);
|
||||
|
||||
// …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).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).not.toContain('defaultValue={availableIndexes[0]}');
|
||||
});
|
||||
|
||||
it('AutocompleteSearch re-runs its refine effect when search availability recovers', () => {
|
||||
// `searchErrorState` reads a module-level store, so it is the one input to that effect which
|
||||
// SURVIVES the remount. Missing from the deps, a tree that remounted while search was
|
||||
// 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');
|
||||
const deps = source.match(/\}, \[debouncedSearch, query, indexName[^\]]*\]/);
|
||||
|
||||
expect(deps?.[0]).toContain('searchErrorState');
|
||||
});
|
||||
});
|
||||
|
||||
const roots: Root[] = [];
|
||||
@@ -134,15 +201,22 @@ afterEach(async () => {
|
||||
});
|
||||
|
||||
type Harness = {
|
||||
render: (indexName: string, refinedQuery: string) => Promise<void>;
|
||||
render: (
|
||||
indexName: string,
|
||||
options?: { helperQuery?: string; blocked?: boolean }
|
||||
) => Promise<void>;
|
||||
text: () => string;
|
||||
type: (value: string) => Promise<void>;
|
||||
refinedWith: () => string[];
|
||||
};
|
||||
|
||||
/**
|
||||
* A parent that owns the carrier and a child keyed on the index name — the arrangement both
|
||||
* dropdowns now use. `carried: false` is the negative control: the same tree with the text seeded
|
||||
* from the helper's query instead, which is what these components did before.
|
||||
* The arrangement both dropdowns now use, with the search helper modelled by the smallest thing
|
||||
* that can be wrong about it: per-mount state that a remount resets to its initial value, plus the
|
||||
* components' own refine effect — the REAL `shouldRefineSearchQuery`, not a restatement of it.
|
||||
*
|
||||
* `carried: false` is the negative control: the same tree with the text seeded from the helper's
|
||||
* query instead, which is what these components did before.
|
||||
*/
|
||||
function mount(carried: boolean): Harness {
|
||||
const container = document.body.appendChild(document.createElement('div'));
|
||||
@@ -150,86 +224,146 @@ function mount(carried: boolean): Harness {
|
||||
roots.push(root);
|
||||
|
||||
let write: ((value: string) => void) | null = null;
|
||||
const refined: string[] = [];
|
||||
|
||||
function Child({
|
||||
carriedRef,
|
||||
refinedQuery,
|
||||
helperQuery,
|
||||
blocked,
|
||||
}: {
|
||||
carriedRef: React.MutableRefObject<string>;
|
||||
refinedQuery: string;
|
||||
helperQuery: string;
|
||||
blocked: boolean;
|
||||
}) {
|
||||
const viaCarrier = useCarriedSearchText(carriedRef, refinedQuery);
|
||||
const viaState = React.useState(refinedQuery);
|
||||
const viaCarrier = useCarriedSearchText(carriedRef, helperQuery);
|
||||
const viaState = React.useState(helperQuery);
|
||||
const [text, setText] = carried ? viaCarrier : viaState;
|
||||
write = setText;
|
||||
|
||||
// The helper's own query. Per-mount, so a keyed remount hands the child a rebuilt helper
|
||||
// reporting whatever it was constructed with — `''` in production.
|
||||
const [refinedQuery, setRefinedQuery] = React.useState(helperQuery);
|
||||
React.useEffect(() => {
|
||||
if (!shouldRefineSearchQuery(text, refinedQuery, blocked)) return;
|
||||
refined.push(text);
|
||||
setRefinedQuery(text);
|
||||
}, [text, refinedQuery, blocked]);
|
||||
|
||||
return React.createElement('span', null, text);
|
||||
}
|
||||
|
||||
function Parent({ indexName, refinedQuery }: { indexName: string; refinedQuery: string }) {
|
||||
function Parent({
|
||||
indexName,
|
||||
helperQuery,
|
||||
blocked,
|
||||
}: {
|
||||
indexName: string;
|
||||
helperQuery: string;
|
||||
blocked: boolean;
|
||||
}) {
|
||||
const carriedRef = React.useRef('');
|
||||
return React.createElement(Child, { key: indexName, carriedRef, refinedQuery });
|
||||
return React.createElement(Child, { key: indexName, carriedRef, helperQuery, blocked });
|
||||
}
|
||||
|
||||
return {
|
||||
render: async (indexName, refinedQuery) => {
|
||||
await act(async () => root.render(React.createElement(Parent, { indexName, refinedQuery })));
|
||||
render: async (indexName, { helperQuery = '', blocked = false } = {}) => {
|
||||
await act(async () =>
|
||||
root.render(React.createElement(Parent, { indexName, helperQuery, blocked }))
|
||||
);
|
||||
},
|
||||
text: () => container.textContent ?? '',
|
||||
type: async (value) => {
|
||||
await act(async () => write?.(value));
|
||||
},
|
||||
refinedWith: () => [...refined],
|
||||
};
|
||||
}
|
||||
|
||||
describe('useCarriedSearchText', () => {
|
||||
it('keeps the typed text when the index changes and the provider is rebuilt', async () => {
|
||||
const harness = mount(true);
|
||||
await harness.render('models_v9', '');
|
||||
await harness.render('models_v9');
|
||||
await harness.type('dreamshaper');
|
||||
expect(harness.text()).toBe('dreamshaper');
|
||||
|
||||
await harness.render('articles_v6', '');
|
||||
await harness.render('articles_v6');
|
||||
|
||||
expect(harness.text()).toBe('dreamshaper');
|
||||
});
|
||||
|
||||
it('leaves that text differing from the rebuilt helper query, so the search is re-run', async () => {
|
||||
// The refine effect in each component is `if (debouncedSearch === query) return;`. A rebuilt
|
||||
// helper reports an empty query, so this inequality is what makes it fire on the new index
|
||||
// instead of the input merely re-displaying the old text.
|
||||
it('pushes that text into the rebuilt helper, so the search runs again on the new index', async () => {
|
||||
// The point of the seed. A rebuilt helper reports an empty query, so the carried text differs
|
||||
// from it and the refine effect fires a SECOND time — the search is re-run rather than the
|
||||
// input merely re-displaying the old text.
|
||||
const harness = mount(true);
|
||||
await harness.render('models_v9', '');
|
||||
await harness.render('models_v9');
|
||||
await harness.type('dreamshaper');
|
||||
await harness.render('articles_v6', '');
|
||||
expect(harness.refinedWith()).toEqual(['dreamshaper']);
|
||||
|
||||
expect(harness.text()).not.toBe('');
|
||||
await harness.render('articles_v6');
|
||||
|
||||
expect(harness.refinedWith()).toEqual(['dreamshaper', 'dreamshaper']);
|
||||
});
|
||||
|
||||
it('NEGATIVE CONTROL — the same remount drops the text without the carrier', async () => {
|
||||
// Proves the remount in the test above is real. Without this, a harness that silently never
|
||||
// remounted would pass every assertion above while asserting nothing.
|
||||
it('NEGATIVE CONTROL — the same remount drops the text, and refines nothing, without the carrier', async () => {
|
||||
// Proves the remount in the tests above is real: without it `viaState` would still hold the
|
||||
// text and this would fail. Read it together with the two tests above — this one shows the
|
||||
// CHILD was rebuilt, and those show the parent's ref survived that rebuild. Neither claim
|
||||
// stands alone.
|
||||
const harness = mount(false);
|
||||
await harness.render('models_v9', '');
|
||||
await harness.render('models_v9');
|
||||
await harness.type('dreamshaper');
|
||||
expect(harness.text()).toBe('dreamshaper');
|
||||
|
||||
await harness.render('articles_v6', '');
|
||||
await harness.render('articles_v6');
|
||||
|
||||
expect(harness.text()).toBe('');
|
||||
expect(harness.refinedWith()).toEqual(['dreamshaper']);
|
||||
});
|
||||
|
||||
it('does not refine while search is unavailable, and refines once it recovers', async () => {
|
||||
// `blocked` stands for `searchErrorState`, which reads a module-level store and therefore
|
||||
// SURVIVES the remount. It has to be an input the effect can re-run on: a tree that remounted
|
||||
// while blocked restores the text, refines nothing, and would otherwise sit on a populated
|
||||
// input over an empty helper query until the next keystroke.
|
||||
const harness = mount(true);
|
||||
await harness.render('models_v9');
|
||||
await harness.type('dreamshaper');
|
||||
await harness.render('articles_v6', { blocked: true });
|
||||
expect(harness.text()).toBe('dreamshaper');
|
||||
expect(harness.refinedWith()).toEqual(['dreamshaper']);
|
||||
|
||||
await harness.render('articles_v6', { blocked: false });
|
||||
|
||||
expect(harness.refinedWith()).toEqual(['dreamshaper', 'dreamshaper']);
|
||||
});
|
||||
|
||||
it('seeds a first mount from the helper query, since nothing has been typed yet', async () => {
|
||||
const harness = mount(true);
|
||||
await harness.render('models_v9', 'restored-from-url');
|
||||
await harness.render('models_v9', { helperQuery: 'restored-from-url' });
|
||||
expect(harness.text()).toBe('restored-from-url');
|
||||
});
|
||||
|
||||
it('carries a cleared input as cleared, not as the helper query', async () => {
|
||||
it('gives each carrier its own text — two search surfaces on one page do not share', async () => {
|
||||
// A module-scope slot instead of the passed ref would pass every test above while making the
|
||||
// header search and a dropdown on the same page overwrite each other.
|
||||
const typedInto = mount(true);
|
||||
const untouched = mount(true);
|
||||
await typedInto.render('models_v9');
|
||||
await untouched.render('models_v9');
|
||||
|
||||
await typedInto.type('dreamshaper');
|
||||
await untouched.render('articles_v6');
|
||||
|
||||
expect(untouched.text()).toBe('');
|
||||
});
|
||||
|
||||
it('an emptied input falls back to the helper query on the next mount', async () => {
|
||||
const harness = mount(true);
|
||||
await harness.render('models_v9', '');
|
||||
await harness.render('models_v9');
|
||||
await harness.type('dreamshaper');
|
||||
await harness.type('');
|
||||
await harness.render('articles_v6', 'restored-from-url');
|
||||
await harness.render('articles_v6', { helperQuery: 'restored-from-url' });
|
||||
|
||||
// An empty carrier falls back to the helper query — the first-mount behaviour above. That is
|
||||
// the documented precedence, pinned here so a change to it is a decision rather than a drift.
|
||||
@@ -237,6 +371,20 @@ describe('useCarriedSearchText', () => {
|
||||
});
|
||||
});
|
||||
|
||||
describe('shouldRefineSearchQuery', () => {
|
||||
it('refines when the typed text differs from the helper query', () => {
|
||||
expect(shouldRefineSearchQuery('dreamshaper', '')).toBe(true);
|
||||
});
|
||||
|
||||
it('does not refine when the helper already holds that text', () => {
|
||||
expect(shouldRefineSearchQuery('dreamshaper', 'dreamshaper')).toBe(false);
|
||||
});
|
||||
|
||||
it('does not refine while blocked, however far apart the two are', () => {
|
||||
expect(shouldRefineSearchQuery('dreamshaper', '', true)).toBe(false);
|
||||
});
|
||||
});
|
||||
|
||||
describe('seedCarriedSearchText', () => {
|
||||
it('prefers carried text over the helper query', () => {
|
||||
expect(seedCarriedSearchText('dreamshaper', 'restored-from-url')).toBe('dreamshaper');
|
||||
|
||||
@@ -7,11 +7,19 @@ import { IMAGES_SEARCH_INDEX } from '~/server/common/constants';
|
||||
/**
|
||||
* The defect this pins, on the SHIPPED clients rather than on the guard in isolation.
|
||||
*
|
||||
* `<InstantSearch>` in the dropdown surfaces carries no `key={indexName}` — it cannot, because
|
||||
* remounting would clear the query the user is typing — so a target switch can issue a search
|
||||
* with the previous target's `<Configure filters>`. Measured in production as a run of backend
|
||||
* 400s: a models filter set arriving at the images index (`user.id`, `availability`), and `poi`
|
||||
* at indexes that never intend it.
|
||||
* An `<InstantSearch>` that is UPDATED rather than rebuilt on a target switch can issue a search
|
||||
* with the previous target's `<Configure filters>`: react-instantsearch sets the new index and
|
||||
* searches in its render body, before the children that own `filters` re-render. Measured in
|
||||
* production as a run of backend 400s: a models filter set arriving at the images index
|
||||
* (`user.id`, `availability`), and `poi` at indexes that never intend it.
|
||||
*
|
||||
* The dropdown surfaces now also carry `key={indexName}`, so they are rebuilt and there is no
|
||||
* stale filter set left to send — see `dropdown-index-remount.test.ts`, and `useCarriedSearchText`
|
||||
* for how the typed query survives that remount. (An earlier revision of this header said the
|
||||
* dropdowns *could not* be keyed, because remounting would clear what the user is typing. That is
|
||||
* what the carrier fixes.) This guard stays as the request-level backstop: it does not depend on
|
||||
* any component keeping its key, and it covers any future path that assembles a filter set for one
|
||||
* index and sends it to another.
|
||||
*
|
||||
* The assertion that matters is the NEGATIVE one — the doomed request must not be SENT. A test
|
||||
* that only checked the response shape would pass on the pre-change code, which already degrades
|
||||
@@ -83,8 +91,11 @@ const request = (filters: string) => [
|
||||
];
|
||||
|
||||
/**
|
||||
* Every browser search client whose `<InstantSearch>` takes its `indexName` from component
|
||||
* state and has no `key={indexName}` — i.e. every surface the swap can leak on.
|
||||
* Every browser search client built by the shared factory — i.e. every surface whose
|
||||
* `<InstantSearch>` takes its `indexName` from component state, and which therefore assembles a
|
||||
* filter set per target. Their roots are keyed as well, so in practice the stale set is never
|
||||
* built; this asserts the backstop independently of that, because a client is guarded or not
|
||||
* regardless of what any component does with it.
|
||||
*/
|
||||
const GUARDED_CLIENTS: [label: string, load: () => Promise<{ client: unknown }>][] = [
|
||||
[
|
||||
@@ -141,7 +152,13 @@ describe.each(GUARDED_CLIENTS)('%s', (_label, load) => {
|
||||
*/
|
||||
const GUARDED_CLIENT_FACTORY = 'src/components/Search/search-client-factory.ts';
|
||||
|
||||
/** Construction sites that do not need the guard, each with the reason asserted below. */
|
||||
/**
|
||||
* Construction sites that do not need the guard, each with the reason asserted below.
|
||||
*
|
||||
* The discriminator is that these build their own client instead of going through the factory —
|
||||
* NOT that they are the only keyed roots. Every dropdown root is keyed too; these two are listed
|
||||
* because they never reach `createSearchClient`, and each still has to say why it cannot leak.
|
||||
*/
|
||||
const EXCLUDED_CLIENT_SITES: Record<string, { reason: string; mustMatch: RegExp }> = {
|
||||
// Remounts on every index change, so the helper can never carry the previous index's filters.
|
||||
'src/components/Search/SearchLayout.tsx': {
|
||||
|
||||
@@ -49,3 +49,23 @@ export function useCarriedSearchText(
|
||||
export function seedCarriedSearchText(carried: string | undefined, refinedQuery: string): string {
|
||||
return carried ? carried : refinedQuery;
|
||||
}
|
||||
|
||||
/**
|
||||
* Whether the mounted tree still owes its text to the search helper — the decision both dropdowns'
|
||||
* "push the text into the helper" effects make.
|
||||
*
|
||||
* This is what turns a remount into a re-RUN of the search rather than a re-display of the text: a
|
||||
* 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.
|
||||
*/
|
||||
export function shouldRefineSearchQuery(
|
||||
typed: string,
|
||||
refinedQuery: string,
|
||||
blocked = false
|
||||
): boolean {
|
||||
return !blocked && typed !== refinedQuery;
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user