SCIX-871 fix: gate SortStats and YearHistogramSlider on searchStatus#861
Closed
thostetler wants to merge 5 commits intoadsabs:masterfrom
Closed
Conversation
SCIX-871 test: add searchStatus facet gating integration tests SCIX-871 feat: gate facets on searchStatus to prevent stale data after empty/failed searches SCIX-871 fix: drive searchStatus from search page to gate facets SCIX-871 perf: use useLayoutEffect to start facets in same frame as search results
SCIX-871 fix: use varied two-column skeleton rows in facets during search loading SCIX-871 fix: use spinner instead of skeleton rows in facets during search loading
- Replace useLayoutEffect with useIsomorphicLayoutEffect (falls back to useEffect on the server) to eliminate the React SSR warning that fires when useLayoutEffect is used on a server-rendered page. - Move setQuery/submitQuery inside the numFound > 0 branch so empty-result searches do not open a Sentry performance span that can never close (providers.tsx only closes spans when docs.current is non-empty).
NumFound's SortStats sub-component and YearHistogramSlider both read latestQuery directly and fire API requests even when the main search has errored, showing stale data from the previous query. Gate both on searchStatus === 'success' so they stay quiet until the search completes successfully.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #861 +/- ##
========================================
- Coverage 62.7% 62.6% -0.0%
========================================
Files 323 323
Lines 38011 38031 +20
Branches 1723 1720 -3
========================================
- Hits 23802 23794 -8
- Misses 14169 14197 +28
Partials 40 40
🚀 New features to boost your workflow:
|
Member
Author
|
Folded into #860. |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces a centralized searchStatus in the Zustand search slice and uses it to prevent secondary “dependent” UI (citation sort stats + year histogram + facet queries) from firing API requests when the primary search is not in a good/successful state, avoiding stale requests against latestQuery.
Changes:
- Add
searchStatus(idle/loading/success/empty/error) to the global search store and drive it from the primary search query lifecycle on/search. - Gate rendering/fetching for
NumFound’sSortStatsandYearHistogramSliderbehindsearchStatus === 'success'. - Gate facet data fetching behind
searchStatusand add tests around the new gating behavior; adjust facet loading UI to account for primary-search loading.
Risk summary
Medium risk: changes affect search-page state semantics and can indirectly influence when/if facet requests fire and what store state is considered “current”.
Findings (priority order)
medium
-
Facet queries can re-enable for whitespace/empty
q- Impact: Facet requests may fire for invalid or effectively-empty queries (e.g., whitespace-only), potentially causing unnecessary/invalid
/search/querycalls and inconsistent UI behavior. - Location:
src/components/SearchFacet/useGetFacetData.ts:63 - Minimal fix: Include a non-empty/trimmed
qguard inisQueryEnabled(in addition tosearchStatus === 'success'). - Confidence: high
- Impact: Facet requests may fire for invalid or effectively-empty queries (e.g., whitespace-only), potentially causing unnecessary/invalid
-
Empty-results path leaves stale
docs.currentin store- Impact: When a search succeeds with
numFound === 0, the UI indicates empty results butstate.docs.currentcan still contain bibcodes from the prior successful search, leaking stale state into selection/telemetry/other store consumers. - Location:
src/pages/search/index.tsx:279-286 - Minimal fix: In the
numFound === 0branch, clear current docs in the store (e.g.,setDocs([])), and ensure related selection flags are consistent. - Confidence: medium
- Impact: When a search succeeds with
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/test-utils.tsx | Switch test store setup to createStore to avoid Zustand singleton leakage across tests; refactor formatting. |
| src/store/slices/search.ts | Add SearchStatus, searchStatus state, and setSearchStatus action to the search slice. |
| src/pages/search/index.tsx | Drive searchStatus from primary search query lifecycle via an isomorphic layout effect. |
| src/components/SearchFacet/YearHistogramSlider.tsx | Gate histogram facet fetch on searchStatus === 'success'. |
| src/components/SearchFacet/useGetFacetData.ts | Gate facet fetch + exposed facet state on searchStatus; add isSearchLoading. |
| src/components/SearchFacet/useGetFacetData.test.ts | Update/add tests validating facet fetch gating by searchStatus. |
| src/components/SearchFacet/store/FacetStore.ts | Adjust provider props typing (explicit children?: ReactNode). |
| src/components/SearchFacet/SearchFacetModal/SearchFacetModal.tsx | Fix prop typing collision by omitting children from inherited props. |
| src/components/SearchFacet/FacetList.tsx | Update facet loading UI to reflect primary-search loading and simplify loader rendering. |
| src/components/NumFound/NumFound.tsx | Gate SortStats rendering on searchStatus === 'success' to prevent stale stats requests. |
| }, [prefix, searchTerm, sortDir]); | ||
|
|
||
| const isQueryEnabled = enabled && isNonEmptyString(searchQuery?.q?.trim()); | ||
| const isQueryEnabled = enabled && searchStatus === 'success'; |
Comment on lines
+279
to
+286
| if (data.response.numFound === 0) { | ||
| setSearchStatus('empty'); | ||
| } else { | ||
| setDocs(data.response.docs.map((d) => d.bibcode)); | ||
| setQuery(searchParams); | ||
| submitQuery(); | ||
| setSearchStatus('success'); | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
NumFound's SortStats sub-component and YearHistogramSlider both read latestQuery
directly and fire API requests when the main search has errored, showing stale
citation stats and histogram data from the previous query.
citation stats call never fires on stale latestQuery
so histogram data is not fetched against a stale query