From ad7df0d9574495f0c1386a471f63a11c6acba1a5 Mon Sep 17 00:00:00 2001 From: Dave Bauman Date: Tue, 2 Jan 2024 16:27:15 -0500 Subject: [PATCH] fix: Ignore old requests in useSearch hook Previously, search-as-you-type could result in a scenario where the results displayed didn't match the user's search. This occurred because of a bug in the `pending.current` logic that delayed subsequent searches and called `fetchMore()` after the pending query completed. The issue is that `fetchMore()` runs in the closure of the previous search, so it actually just repeated the old search instead of performing a new search with new variables. This PR addresses this issue by removing the `pending.current` check, firing all requests when they come in. When requests complete, they are ignored if they are not the most-recently triggered. Urql doesn't support AbortController, so we can't cancel the in-progress requests. If necessary, we can increase the debounce delay to reduce load. The infinite scroll controller tends to trigger multiple duplicate requests, so these are also being filtered out before sending using similar logic. --- packages/frontend/src/shared/useSearch.ts | 126 +++++++++++++--------- 1 file changed, 78 insertions(+), 48 deletions(-) diff --git a/packages/frontend/src/shared/useSearch.ts b/packages/frontend/src/shared/useSearch.ts index 249279229c..b7f7aaabe0 100644 --- a/packages/frontend/src/shared/useSearch.ts +++ b/packages/frontend/src/shared/useSearch.ts @@ -85,12 +85,45 @@ export interface UseSearchProps { paused?: boolean; } -// https://github.com/typescript-eslint/typescript-eslint/issues/2446 -// type PaginationReturn = [{}, () => void]; +export type SearchResultState = { + data: { + insights: { + suggestedFilters?: AutocompleteResults; + results: SearchResult[]; + }; + }; + error?: CombinedError | string; + fetching: boolean; + from: React.MutableRefObject; + hasMore: boolean; + total: number; +}; + +export type UseSearchReturnType = [SearchResultState, () => Promise]; -export function useSearch({ query, sort, paused = false }: UseSearchProps): any { +/** + * Custom hook for performing a search. + * + * This hook is designed to be used with an infinite scroll component, + * so it returns both the search result state and a function to fetch more results. + * The search results include the aggregated results from all pages that have been + * loaded so far. + * + * Any changes to the query or sort options will reset the infinite scroll state + * back to the first page of results. + * + * If multiple requests are made before the first one finishes, only the latest + * request will be used. Urql does not support cancelling requests, so this + * is the best we can do. + * + * @param {UseSearchProps} options - The search options. + * @param {string} options.query - The search query. + * @param {string} options.sort - The sort option. + * @param {boolean} [options.paused=false] - Whether the search is paused. + * @returns {UseSearchReturnType} - The search result state and a function to fetch more results. + */ +export function useSearch({ query, sort, paused = false }: UseSearchProps): UseSearchReturnType { const [fetching, setFetching] = useState(true); - const loadingNextPage = useRef(false); const [suggestedFilters, setSuggestedFilters] = useState(); const [error, setError] = useState(); @@ -99,7 +132,8 @@ export function useSearch({ query, sort, paused = false }: UseSearchProps): any const size = 20; // Fixed value for now const from = useRef(0); const [results, setResults] = useState([]); - const pending = useRef(false); + + const latestRequest = useRef(); const fetchMore = useCallback(async () => { if (paused) { @@ -107,61 +141,57 @@ export function useSearch({ query, sort, paused = false }: UseSearchProps): any return; } - if (!loadingNextPage.current) { - // console.log(`Loading next page (from = ${from.current})`); - loadingNextPage.current = true; - - const { data: nextPage, error } = await urqlClient - .query(INSIGHTS_QUERY, { - search: { - query: query || '', - sort: sort && [sort], - paging: { - from: from.current, - size - } - } - }) - .toPromise(); - - setError(error); + // Generate a unique ID for this request so we can ignore old responses + const requestId = `r||${query}||${sort}||${from.current}`; + if (latestRequest.current === requestId) { + // Ignore duplicate requests + // The infinite scroll component may trigger multiple requests for the same page + return; + } - if (nextPage != null) { - if (from.current === 0) { - // Replace results with the first page of a new search - setResults(nextPage.insights.results); + latestRequest.current = requestId; - // Only use total and suggested filters from the first page of results - total.current = nextPage.insights.pageInfo.total; - setSuggestedFilters(nextPage.insights.suggestedFilters); - } else { - setResults((existing) => [...existing, ...nextPage.insights.results]); + const { data: nextPage, error } = await urqlClient + .query(INSIGHTS_QUERY, { + search: { + query: query || '', + sort: sort && [sort], + paging: { + from: from.current, + size + } } + }) + .toPromise(); - // console.log('Finished loading page ' + from.current); - from.current += size; - } + if (latestRequest.current !== requestId) { + // Ignore this response, it's for an old request + return; + } + + setError(error); - // Done! - loadingNextPage.current = false; - setFetching(false); + if (nextPage != null) { + if (from.current === 0) { + // Replace results with the first page of a new search + setResults(nextPage.insights.results); - // If a request is pending, process it now - if (pending.current) { - pending.current = false; - fetchMore(); + // Only use total and suggested filters from the first page of results + total.current = nextPage.insights.pageInfo.total; + setSuggestedFilters(nextPage.insights.suggestedFilters); + } else { + setResults((existing) => [...existing, ...nextPage.insights.results]); } - } else { - // If a fetchMore request came while we were loading, pending is set true - // This is here to trigger a subsequent page load to avoid cases where the - // front-end component already triggered fetchMore(), but it was already loading. - pending.current = true; + + from.current += size; } + + // Done! + setFetching(false); }, [paused, query, sort]); useEffect(() => { // Reset the scroll state whenever query/sort changes - // console.log('Resetting infinite scroll'); from.current = 0; setResults([]); setFetching(true);