diff --git a/src/components/NumFound/NumFound.tsx b/src/components/NumFound/NumFound.tsx index ee602b32b..24f94fbd5 100644 --- a/src/components/NumFound/NumFound.tsx +++ b/src/components/NumFound/NumFound.tsx @@ -17,6 +17,7 @@ const sanitizeNum = (num: number): string => { export const NumFound = (props: INumFoundProps): ReactElement => { const { count = 0, isLoading } = props; + const searchStatus = useStore((state) => state.searchStatus); if (isLoading) { return ( @@ -35,7 +36,7 @@ export const NumFound = (props: INumFoundProps): ReactElement => { {countString} {' '} - results + results {searchStatus === 'success' ? : null} ); diff --git a/src/components/SearchFacet/FacetList.tsx b/src/components/SearchFacet/FacetList.tsx index 8b5091786..5fc00c6e2 100644 --- a/src/components/SearchFacet/FacetList.tsx +++ b/src/components/SearchFacet/FacetList.tsx @@ -28,7 +28,6 @@ import { PopoverCloseButton, PopoverContent, PopoverHeader, - Skeleton, Spinner, Stack, Text, @@ -122,7 +121,7 @@ export const NodeList = memo( const updateModal = useFacetStore(selectors.updateModal); const depth = getLevelFromKey(prefix) + 1; const expandable = params.hasChildren && (level === 'root' || params.maxDepth > depth); - const { treeData, isFetching, isLoading, isError } = useGetFacetData({ + const { treeData, isFetching, isLoading, isSearchLoading, isError } = useGetFacetData({ ...params, prefix, level, @@ -158,10 +157,10 @@ export const NodeList = memo( ); } - if (isFetching || isLoading) { + if (isFetching || isLoading || isSearchLoading) { return ( -
- +
+
); } else if (treeData?.length === 0) { @@ -298,6 +297,7 @@ export const NodeListModal = (props: INodeListProps) => { treeData, isFetching, isLoading, + isSearchLoading, isError, pagination, handleLoadMore, @@ -313,20 +313,11 @@ export const NodeListModal = (props: INodeListProps) => { sortDir, }); - if (isFetching || isLoading) { + if (isFetching || isLoading || isSearchLoading) { return ( - - - - - - - - - - - - +
+ +
); } else if (isEmpty(treeData)) { return ( diff --git a/src/components/SearchFacet/SearchFacetModal/SearchFacetModal.tsx b/src/components/SearchFacet/SearchFacetModal/SearchFacetModal.tsx index 78345979f..e780ae865 100644 --- a/src/components/SearchFacet/SearchFacetModal/SearchFacetModal.tsx +++ b/src/components/SearchFacet/SearchFacetModal/SearchFacetModal.tsx @@ -35,7 +35,7 @@ import { join, last, map, pipe, pluck, split } from 'ramda'; import { parseAPIError } from '@/utils/common/parseAPIError'; import { FacetItem, FacetLogic } from '../types'; -interface ISearchFacetModalProps extends Omit { +interface ISearchFacetModalProps extends Omit { children: (props: { searchTerm: string }) => ReactNode; } diff --git a/src/components/SearchFacet/YearHistogramSlider.tsx b/src/components/SearchFacet/YearHistogramSlider.tsx index 8fa59ae84..75166ae34 100644 --- a/src/components/SearchFacet/YearHistogramSlider.tsx +++ b/src/components/SearchFacet/YearHistogramSlider.tsx @@ -25,6 +25,7 @@ export interface IYearHistogramSliderProps { const Component = ({ onQueryUpdate, width, height, onExpand, expanded }: IYearHistogramSliderProps) => { const query = useStore((state) => state.latestQuery); + const searchStatus = useStore((state) => state.searchStatus); // query without the year range filter, to show all years on the histogram const cleanedQuery = useMemo(() => { @@ -38,7 +39,7 @@ const Component = ({ onQueryUpdate, width, height, onExpand, expanded }: IYearHi }, [query]); const { data } = useGetSearchFacetCounts(getSearchFacetYearsParams(cleanedQuery), { - enabled: !!cleanedQuery && cleanedQuery.q.trim().length > 0, + enabled: searchStatus === 'success' && !!cleanedQuery && cleanedQuery.q.trim().length > 0, suspense: true, }); diff --git a/src/components/SearchFacet/store/FacetStore.ts b/src/components/SearchFacet/store/FacetStore.ts index 3e573cc65..9d9901c4e 100644 --- a/src/components/SearchFacet/store/FacetStore.ts +++ b/src/components/SearchFacet/store/FacetStore.ts @@ -1,7 +1,7 @@ import { facetConfig } from '@/components/SearchFacet/config'; import { FacetItem, IFacetParams, SearchFacetID } from '@/components/SearchFacet/types'; import { omit, pick, uniq } from 'ramda'; -import { createElement, FC } from 'react'; +import { createElement, FC, ReactNode } from 'react'; import create from 'zustand'; import createContext from 'zustand/context'; import { computeNextSelectionState, createNodes, getSelected } from './helpers'; @@ -128,7 +128,7 @@ const createStore = (preloadedState: Partial) => () => const FacetStoreContext = createContext(); export const useFacetStore = FacetStoreContext.useStore; -export const FacetStoreProvider: FC<{ facetId: SearchFacetID }> = ({ children, facetId }) => { +export const FacetStoreProvider: FC<{ facetId: SearchFacetID; children?: ReactNode }> = ({ children, facetId }) => { const params = pick( [ 'label', diff --git a/src/components/SearchFacet/useGetFacetData.test.ts b/src/components/SearchFacet/useGetFacetData.test.ts index 148fce3a0..ed635788f 100644 --- a/src/components/SearchFacet/useGetFacetData.test.ts +++ b/src/components/SearchFacet/useGetFacetData.test.ts @@ -1,5 +1,7 @@ import { describe, test, expect, vi, TestContext } from 'vitest'; -import { renderHook, waitFor, createServerListenerMocks, urls } from '@/test-utils'; +import { renderHook, waitFor, act, createServerListenerMocks, urls } from '@/test-utils'; +import { useStore } from '@/store'; +import { IUseGetFacetDataProps } from './useGetFacetData'; import { useGetFacetData } from './useGetFacetData'; import { defaultQueryParams } from '@/store/slices/search'; import { FacetField } from '@/api/search/types'; @@ -14,11 +16,16 @@ const defaultProps = { level: 'root' as const, }; +const useCompound = (props: IUseGetFacetDataProps) => ({ + setSearchStatus: useStore((state) => state.setSearchStatus), + facet: useGetFacetData(props), +}); + describe('useGetFacetData', () => { - test('does not fire a request when latestQuery.q is empty', async ({ server }: TestContext) => { + test('does not fire a request when searchStatus is idle', async ({ server }: TestContext) => { const { onRequest } = createServerListenerMocks(server); renderHook(() => useGetFacetData(defaultProps), { - initialStore: { latestQuery: { ...defaultQueryParams, q: '' } }, + initialStore: { searchStatus: 'idle', latestQuery: { ...defaultQueryParams, q: 'star' } }, }); await new Promise((r) => setTimeout(r, 200)); @@ -26,10 +33,10 @@ describe('useGetFacetData', () => { expect(searchRequests).toHaveLength(0); }); - test('does not fire a request when latestQuery.q is whitespace-only', async ({ server }: TestContext) => { + test('does not fire a request when searchStatus is not success (empty)', async ({ server }: TestContext) => { const { onRequest } = createServerListenerMocks(server); renderHook(() => useGetFacetData(defaultProps), { - initialStore: { latestQuery: { ...defaultQueryParams, q: ' ' } }, + initialStore: { searchStatus: 'empty', latestQuery: { ...defaultQueryParams, q: 'star' } }, }); await new Promise((r) => setTimeout(r, 200)); @@ -40,7 +47,7 @@ describe('useGetFacetData', () => { test('fires a request when latestQuery.q is non-empty', async ({ server }: TestContext) => { const { onRequest } = createServerListenerMocks(server); renderHook(() => useGetFacetData(defaultProps), { - initialStore: { latestQuery: { ...defaultQueryParams, q: 'star' } }, + initialStore: { searchStatus: 'success', latestQuery: { ...defaultQueryParams, q: 'star' } }, }); await waitFor(() => { @@ -49,3 +56,119 @@ describe('useGetFacetData', () => { }); }); }); + +describe('useGetFacetData — searchStatus gating', () => { + test('does not fire when searchStatus is loading', async ({ server }: TestContext) => { + const { onRequest } = createServerListenerMocks(server); + + renderHook(() => useCompound(defaultProps), { + initialStore: { + searchStatus: 'loading', + latestQuery: { ...defaultQueryParams, q: 'star' }, + }, + }); + + await new Promise((r) => setTimeout(r, 200)); + const facetRequests = urls(onRequest).filter((u) => u === '/search/query'); + expect(facetRequests).toHaveLength(0); + }); + + test('does not fire when searchStatus is empty', async ({ server }: TestContext) => { + const { onRequest } = createServerListenerMocks(server); + + renderHook(() => useCompound(defaultProps), { + initialStore: { + searchStatus: 'empty', + latestQuery: { ...defaultQueryParams, q: 'star' }, + }, + }); + + await new Promise((r) => setTimeout(r, 200)); + const facetRequests = urls(onRequest).filter((u) => u === '/search/query'); + expect(facetRequests).toHaveLength(0); + }); + + test('does not fire when searchStatus is error', async ({ server }: TestContext) => { + const { onRequest } = createServerListenerMocks(server); + + renderHook(() => useCompound(defaultProps), { + initialStore: { + searchStatus: 'error', + latestQuery: { ...defaultQueryParams, q: 'star' }, + }, + }); + + await new Promise((r) => setTimeout(r, 200)); + const facetRequests = urls(onRequest).filter((u) => u === '/search/query'); + expect(facetRequests).toHaveLength(0); + }); + + test('fires and returns data when searchStatus is success', async ({ server }: TestContext) => { + const { onRequest } = createServerListenerMocks(server); + + const { result } = renderHook(() => useCompound(defaultProps), { + initialStore: { + searchStatus: 'success', + latestQuery: { ...defaultQueryParams, q: 'star' }, + }, + }); + + await waitFor(() => { + const facetRequests = urls(onRequest).filter((u) => u === '/search/query'); + expect(facetRequests.length).toBeGreaterThan(0); + }); + + await waitFor(() => { + expect(result.current.facet.treeData.length).toBeGreaterThan(0); + }); + }); + + test('regression: loading→success transition unblocks fetch and populates data', async ({ server }: TestContext) => { + const { onRequest } = createServerListenerMocks(server); + + const { result } = renderHook(() => useCompound(defaultProps), { + initialStore: { + searchStatus: 'loading', + latestQuery: { ...defaultQueryParams, q: 'star' }, + }, + }); + + await new Promise((r) => setTimeout(r, 200)); + expect(urls(onRequest).filter((u) => u === '/search/query')).toHaveLength(0); + expect(result.current.facet.treeData).toHaveLength(0); + + act(() => { + result.current.setSearchStatus('success'); + }); + + await waitFor(() => { + expect(urls(onRequest).filter((u) => u === '/search/query').length).toBeGreaterThan(0); + }); + + await waitFor(() => { + expect(result.current.facet.treeData.length).toBeGreaterThan(0); + }); + }); + + test('success→loading transition clears treeData synchronously', async ({ server }: TestContext) => { + createServerListenerMocks(server); + + const { result } = renderHook(() => useCompound(defaultProps), { + initialStore: { + searchStatus: 'success', + latestQuery: { ...defaultQueryParams, q: 'star' }, + }, + }); + + await waitFor(() => { + expect(result.current.facet.treeData.length).toBeGreaterThan(0); + }); + + act(() => { + result.current.setSearchStatus('loading'); + }); + + expect(result.current.facet.treeData).toHaveLength(0); + expect(result.current.facet.totalResults).toBe(0); + }); +}); diff --git a/src/components/SearchFacet/useGetFacetData.ts b/src/components/SearchFacet/useGetFacetData.ts index 78168539f..7bb8b3c3e 100644 --- a/src/components/SearchFacet/useGetFacetData.ts +++ b/src/components/SearchFacet/useGetFacetData.ts @@ -36,6 +36,7 @@ const querySelector = (state: AppState) => omit(['fl', 'start', 'rows'], state.l export const useGetFacetData = (props: IUseGetFacetDataProps) => { const searchQuery = useStore(querySelector); + const searchStatus = useStore((state: AppState) => state.searchStatus); const { field, query = '', @@ -59,7 +60,7 @@ export const useGetFacetData = (props: IUseGetFacetDataProps) => { setPagination(calculatePagination({ page: 0, numPerPage: FACET_DEFAULT_LIMIT })); }, [prefix, searchTerm, sortDir]); - const isQueryEnabled = enabled && isNonEmptyString(searchQuery?.q?.trim()); + const isQueryEnabled = enabled && searchStatus === 'success'; // fetch the data const { data, ...result } = useGetSearchFacetJSON( @@ -83,12 +84,12 @@ export const useGetFacetData = (props: IUseGetFacetDataProps) => { }, { enabled: isQueryEnabled, - keepPreviousData: true, }, ); const res = data?.[field]; - const treeData = useMemo(() => formatTreeData(res?.buckets ?? []), [res?.buckets]); + const rawTreeData = useMemo(() => formatTreeData(res?.buckets ?? []), [res?.buckets]); + const treeData = searchStatus === 'success' ? rawTreeData : ([] as FacetItem[]); const identifiers = useMemo( () => @@ -160,12 +161,13 @@ export const useGetFacetData = (props: IUseGetFacetDataProps) => { return { treeData: enhancedTreeData, - totalResults: res?.numBuckets ?? 0, + totalResults: searchStatus === 'success' ? res?.numBuckets ?? 0 : 0, pagination: pagination, handlePrevious, handleLoadMore, handlePageChange, - canLoadMore: res?.numBuckets !== treeData?.length, + canLoadMore: searchStatus === 'success' && res?.numBuckets !== treeData?.length, + isSearchLoading: searchStatus === 'loading', ...result, isLoading: (isQueryEnabled && result.isLoading) || (hasIdentifiers && isLoading), isFetching: result.isFetching || (hasIdentifiers && isFetching), diff --git a/src/pages/search/index.tsx b/src/pages/search/index.tsx index 87f751a0a..fd098c622 100644 --- a/src/pages/search/index.tsx +++ b/src/pages/search/index.tsx @@ -35,7 +35,7 @@ import { VisuallyHidden, } from '@chakra-ui/react'; import { calculateStartIndex } from '@/components/ResultList/Pagination/usePagination'; -import { FormEventHandler, RefObject, useCallback, useEffect, useMemo, useRef, useState } from 'react'; +import { FormEventHandler, RefObject, useCallback, useEffect, useLayoutEffect, useMemo, useRef, useState } from 'react'; import { useIsClient } from '@/lib/useIsClient'; import { useScrollRestoration } from '@/lib/useScrollRestoration'; import { LocalSettings, NumPerPageType } from '@/types'; @@ -82,6 +82,10 @@ const SearchFacets = dynamic( { ssr: false }, ); +// useLayoutEffect triggers an SSR warning on server-rendered pages; use +// useEffect on the server where layout effects are a no-op anyway. +const useIsomorphicLayoutEffect = typeof window !== 'undefined' ? useLayoutEffect : useEffect; + /** * Consolidated selector for search page store values * Using shallow comparison to prevent unnecessary re-renders @@ -97,6 +101,7 @@ const useSearchPageStore = () => setNumPerPage: state.setNumPerPage, setDocs: state.setDocs, clearAllSelected: state.clearAllSelected, + setSearchStatus: state.setSearchStatus, }), shallow, ); @@ -138,6 +143,7 @@ const SearchPage: NextPage = () => { setNumPerPage, setDocs, clearAllSelected: clearSelectedDocs, + setSearchStatus, } = useSearchPageStore(); const { settings } = useSettings({ suspense: false }); @@ -254,16 +260,32 @@ const SearchPage: NextPage = () => { void router.push({ pathname: router.pathname, search }, null, { scroll: false, shallow: true }); }; - // Update the store when we have data - useEffect(() => { - if (data?.response.docs.length > 0) { - setDocs(data.response.docs.map((d) => d.bibcode)); - setQuery(searchParams); - submitQuery(); + // Drive searchStatus and store state based on the main search result. + // useIsomorphicLayoutEffect fires before paint on the client (so facets start + // loading in the same frame results render), but falls back to useEffect on + // the server to avoid the SSR warning React emits for useLayoutEffect. + // Uses isLoading (not isFetching) to avoid disabling facets during + // background refetches of the same query. + useIsomorphicLayoutEffect(() => { + if (isLoading) { + setSearchStatus('loading'); + return; + } + if (isError) { + setSearchStatus('error'); + return; + } + if (isSuccess) { + if (data.response.numFound === 0) { + setSearchStatus('empty'); + } else { + setDocs(data.response.docs.map((d) => d.bibcode)); + setQuery(searchParams); + submitQuery(); + setSearchStatus('success'); + } } - // Note: setDocs, setQuery, submitQuery are stable Zustand actions - // searchParams is derived from router, changes trigger new data fetch - }, [data, setDocs, setQuery, submitQuery, searchParams]); + }, [data, isSuccess, isLoading, isError, setDocs, setQuery, submitQuery, setSearchStatus, searchParams]); // Memoized retry handler for error alert const handleRetry = useCallback(() => { diff --git a/src/store/slices/search.ts b/src/store/slices/search.ts index 09d4b56ef..5bea40eca 100644 --- a/src/store/slices/search.ts +++ b/src/store/slices/search.ts @@ -5,6 +5,8 @@ import { mergeRight } from 'ramda'; import { isNumPerPageType } from '@/utils/common/guards'; import { IADSApiSearchParams } from '@/api/search/types'; +export type SearchStatus = 'idle' | 'loading' | 'success' | 'empty' | 'error'; + export const defaultQueryParams: IADSApiSearchParams = { q: '', fl: [ @@ -38,6 +40,7 @@ export interface ISearchState { showHighlights: boolean; queryAddition: string; clearQueryFlag: boolean; + searchStatus: SearchStatus; } export interface ISearchAction { @@ -50,6 +53,7 @@ export interface ISearchAction { toggleShowHighlights: () => void; setQueryAddition: (queryAddition: string) => void; setClearQueryFlag: (clearQueryFlag: boolean) => void; + setSearchStatus: (status: SearchStatus) => void; } export const searchSlice: StoreSlice = (set) => ({ @@ -63,6 +67,7 @@ export const searchSlice: StoreSlice = (set) => ({ showHighlights: false, queryAddition: null, clearQueryFlag: false, + searchStatus: 'idle' as SearchStatus, setNumPerPage: (numPerPage: NumPerPageType) => set( @@ -86,4 +91,5 @@ export const searchSlice: StoreSlice = (set) => ({ set(({ showHighlights }) => ({ showHighlights: !showHighlights }), false, 'search/toggleShowHighlights'), setQueryAddition: (queryAddition: string) => set(() => ({ queryAddition }), false, 'search/setQueryAddition'), setClearQueryFlag: (clearQueryFlag: boolean) => set(() => ({ clearQueryFlag }), false, 'search/setClearQueryFlag'), + setSearchStatus: (searchStatus: SearchStatus) => set(() => ({ searchStatus }), false, 'search/setSearchStatus'), }); diff --git a/src/test-utils.tsx b/src/test-utils.tsx index 48b3ec213..32eda88e6 100644 --- a/src/test-utils.tsx +++ b/src/test-utils.tsx @@ -1,4 +1,4 @@ -import { AppState, StoreProvider, useCreateStore } from '@/store'; +import { AppState, StoreProvider, createStore } from '@/store'; import { render, renderHook, RenderOptions } from '@testing-library/react'; import { MockedRequest } from 'msw'; import { SetupServerApi } from 'msw/node'; @@ -47,21 +47,26 @@ interface IProviderOptions { storePreset?: 'orcid-authenticated'; } -export const DefaultProviders = ({ children, options }: { - children: ReactElement | ReactNode, - options: IProviderOptions +export const DefaultProviders = ({ + children, + options, +}: { + children: ReactElement | ReactNode; + options: IProviderOptions; }) => { const queryClient = new QueryClient({ defaultOptions: { queries: { retry: false, cacheTime: 0, staleTime: 0 } } }); - const store = isObject(options?.initialStore) ? - options.initialStore : - options?.storePreset ? getStateFromPreset(options.storePreset) : {}; + const store = isObject(options?.initialStore) + ? options.initialStore + : options?.storePreset + ? getStateFromPreset(options.storePreset) + : {}; return ( - + createStore(store)}> {children} @@ -86,8 +91,11 @@ const getStateFromPreset = (preset: IProviderOptions['storePreset']): Partial) => { +const renderComponent = ( + ui: ReactElement, + providerOptions?: IProviderOptions, + options?: Omit, +) => { const result = render(ui, { wrapper: ({ children }) => {children}, ...options, @@ -96,8 +104,11 @@ const renderComponent = (ui: ReactElement, providerOptions?: IProviderOptions, return { user, ...result }; }; -const renderHookComponent = , TProps = Parameters>(hook: Parameters>[0], - providerOptions?: IProviderOptions, options?: Omit>[1], 'wrapper'>) => { +const renderHookComponent = , TProps = Parameters>( + hook: Parameters>[0], + providerOptions?: IProviderOptions, + options?: Omit>[1], 'wrapper'>, +) => { return renderHook(hook, { wrapper: ({ children }) => {children}, ...options,