fix: recover Search snapshots stuck on a stale error instead of looping on Try again

src/hooks/useSearchPageSetup.ts

@@ -7,9 +7,10 @@ import {hasDeferredWrite} from '@libs/deferredLayoutWrite';
 import {isSearchDataLoaded, isSearchPending} from '@libs/SearchUIUtils';
 
 import CONST from '@src/CONST';
+import {isEmptyObject} from '@src/types/utils/EmptyObject';
 
 import {useFocusEffect} from '@react-navigation/native';
-import {useEffect} from 'react';
+import {useEffect, useRef} from 'react';
 
 import useNetwork from './useNetwork';
 import usePrevious from './usePrevious';
@@ -43,6 +44,22 @@ function useSearchPageSetup(queryJSON: Readonly<SearchQueryJSON> | undefined) {
     const isSnapshotSearchLoading = !!currentSearchResults?.search?.isLoading;
     const isInitialSearchPending = isSearchPending(currentSearchResults) && (currentSearchResults?.search?.offset ?? 0) === 0;
 
+    // `errors` counts as a resolution in isSearchDataLoaded, so a snapshot left errored by a failed
+    // request looks loaded and the early return below skips the request that would clear those errors.
+    // The Search page then renders its error view with nothing in flight, and only the Try again
+    // button can break out of it. Allow one recovery attempt per hash instead, so a transient failure
+    // heals by itself while a persistent one still settles on the error view rather than looping
+    // from request to failure and back to request.
+    // The Set is scoped to this hook instance, which lives as long as the Search page stays mounted.
+    // Changing the query only swaps route params, and an inactive tab is hidden rather than unmounted,
+    // so a hash that already spent its attempt gets another one only after a real remount.
+    const hashesWithAttemptedRecoveryRef = useRef<Set<number>>(new Set());
+    // The server already judged the query itself malformed, so re-sending it cannot succeed.
+    const isInvalidQuery = currentSearchResults?.search?.responseJsonCode === CONST.JSON_CODE.INVALID_SEARCH_QUERY;
+    // Same emptiness rule as the error view this recovery exists to unblock (Search/index.tsx), so the
+    // two cannot drift into a state where one shows the error and the other refuses to clear it.
+    const hasRecoverableErrors = !isEmptyObject(currentSearchResults?.errors) && !isInvalidQuery;
+
     // Clear selected transactions when navigating to a different search query
     function clearOnHashChange() {
         if (hash === undefined) {
@@ -71,14 +88,24 @@ function useSearchPageSetup(queryJSON: Readonly<SearchQueryJSON> | undefined) {
             lastSavedSearchHash = hash;
         }
 
+        const shouldRecoverFromErrors = hasRecoverableErrors && !hashesWithAttemptedRecoveryRef.current.has(hash);
+
         // A pending initial request may be stale after reload and can be restarted through request deduplication.
         // Pagination must not restart page one.
-        if (isSnapshotDataLoaded && !isInitialSearchPending) {
+        if (isSnapshotDataLoaded && !isInitialSearchPending && !shouldRecoverFromErrors) {
             return;
         }
+
         const shouldSkipWaitForWrites = hasDeferredWrite(CONST.DEFERRED_LAYOUT_WRITE_KEYS.SEARCH);
-        search({queryJSON, searchKey: currentSearchKey, offset: 0, shouldCalculateTotals, isLoading: false, skipWaitForWrites: shouldSkipWaitForWrites});
-    }, [hash, isOffline, shouldUseLiveData, queryJSON, isSnapshotDataLoaded, isSnapshotSearchLoading, isInitialSearchPending, currentSearchKey, shouldCalculateTotals]);
+        const searchRequest = search({queryJSON, searchKey: currentSearchKey, offset: 0, shouldCalculateTotals, isLoading: false, skipWaitForWrites: shouldSkipWaitForWrites});
+
+        // search() returns undefined when it declines to send (API prevention, or an identical request
+        // already in flight) and writes nothing, so `errors` survives. Spending the attempt on those would
+        // leave the query stuck for the rest of the mount, which is the state this recovery exists to avoid.
+        if (shouldRecoverFromErrors && searchRequest) {
+            hashesWithAttemptedRecoveryRef.current.add(hash);
+        }
+    }, [hash, isOffline, shouldUseLiveData, queryJSON, isSnapshotDataLoaded, isSnapshotSearchLoading, isInitialSearchPending, hasRecoverableErrors, currentSearchKey, shouldCalculateTotals]);
 
     useFocusEffect(() => {
         openSearch();

tests/ui/SearchPageTest.tsx

@@ -6,7 +6,6 @@ import {LocaleContextProvider} from '@components/LocaleContextProvider';
 import OnyxListItemProvider from '@components/OnyxListItemProvider';
 import {SearchContextProvider} from '@components/Search/SearchContextProvider';
 import SearchLoadingSkeleton from '@components/Search/SearchLoadingSkeleton';
-import SearchRowSkeleton from '@components/Skeletons/SearchRowSkeleton';
 import {PlaybackContextProvider} from '@components/VideoPlayerContexts/PlaybackContext';
 
 import useNetwork from '@hooks/useNetwork';
@@ -190,7 +189,7 @@ describe('SearchPageNarrow', () => {
         expect(searchInput).toBeTruthy();
     });
 
-    it('does not retry an already failed search snapshot', async () => {
+    it('retries an already failed search snapshot once on a fresh mount', async () => {
         await act(async () => {
             await Onyx.set(`${ONYXKEYS.COLLECTION.SNAPSHOT}${failedQueryJSON?.hash}`, {
                 errors: {error: 'Something went wrong'},
@@ -206,14 +205,16 @@ describe('SearchPageNarrow', () => {
             });
         });
 
-        const renderedPage = renderPage();
+        renderPage();
 
         await act(async () => {
             jest.advanceTimersByTime(0);
         });
 
-        expect(mockSearch).not.toHaveBeenCalled();
-        expect(renderedPage.UNSAFE_queryByType(SearchRowSkeleton)).toBeNull();
+        // Only a request clears the persisted `errors`, so without this one attempt the page renders its
+        // error view on every mount with nothing in flight, recoverable only by tapping Try again. The
+        // attempt means a fresh mount now shows the skeleton briefly instead of the error straight away.
+        expect(mockSearch).toHaveBeenCalledTimes(1);
     });
 
     // Reproduces the reload case: the errored snapshot survives but the in-memory response code does not,

tests/unit/useSearchPageSetupTest.tsx

@@ -0,0 +1,139 @@
+import {renderHook} from '@testing-library/react-native';
+
+import useSearchPageSetup from '@hooks/useSearchPageSetup';
+
+import {buildSearchQueryJSON} from '@libs/SearchQueryUtils';
+import type {SearchKey} from '@libs/SearchUIUtils';
+
+import CONST from '@src/CONST';
+import type SearchResults from '@src/types/onyx/SearchResults';
+
+import type * as ReactNavigation from '@react-navigation/native';
+
+const mockSearch = jest.fn<Promise<undefined> | undefined, unknown[]>();
+let mockSearchResults: SearchResults | undefined;
+// Mutable so a test can move a real effect dependency and force the effect to run again.
+let mockSearchKey: SearchKey | undefined;
+
+jest.mock('@react-navigation/native', () => {
+    const actualNavigation: typeof ReactNavigation = jest.requireActual('@react-navigation/native');
+    return {
+        ...actualNavigation,
+        useFocusEffect: jest.fn(),
+    };
+});
+
+// The return value matters: the hook only spends a recovery attempt when search() actually sends,
+// so the mock has to pass it through rather than swallow it.
+jest.mock('@libs/actions/Search', () => ({
+    search: (...args: unknown[]) => mockSearch(...args),
+    openSearch: jest.fn(),
+}));
+
+jest.mock('@libs/actions/ReportNavigation', () => ({
+    saveLastSearchParams: jest.fn(),
+}));
+
+jest.mock('@hooks/useNetwork', () => () => ({isOffline: false}));
+
+jest.mock('@hooks/useSearchShouldCalculateTotals', () => () => false);
+
+jest.mock('@components/Search/SearchContext', () => ({
+    useSearchResultsContext: () => ({shouldUseLiveData: false, currentSearchResults: mockSearchResults}),
+    useSearchQueryContext: () => ({currentSearchKey: mockSearchKey}),
+    useSearchSelectionActions: () => ({clearSelectedTransactions: jest.fn()}),
+}));
+
+const QUERY = 'type:expense sortBy:date sortOrder:desc';
+const QUERY_B = 'type:expense sortBy:amount sortOrder:asc';
+const queryJSON = buildSearchQueryJSON(QUERY);
+const queryJSONB = buildSearchQueryJSON(QUERY_B);
+
+/** A snapshot left behind by a failed request: `errors` present, no data, and no server verdict. */
+function buildErroredSnapshot(hash: number): SearchResults {
+    // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion
+    return {
+        search: {
+            type: CONST.SEARCH.DATA_TYPES.EXPENSE,
+            hash,
+            isLoading: false,
+            offset: 0,
+            sortBy: CONST.SEARCH.TABLE_COLUMNS.DATE,
+            sortOrder: CONST.SEARCH.SORT_ORDER.DESC,
+            responseJsonCode: 0,
+        },
+        errors: {error: 'Oops... something went wrong'},
+    } as unknown as SearchResults;
+}
+
+describe('useSearchPageSetup', () => {
+    beforeEach(() => {
+        mockSearch.mockClear();
+        // A sent request; search() returns undefined only when it declines to send.
+        mockSearch.mockReturnValue(Promise.resolve(undefined));
+        mockSearchResults = undefined;
+        mockSearchKey = undefined;
+    });
+
+    it('re-requests a query whose snapshot was left errored by a failed request', () => {
+        mockSearchResults = buildErroredSnapshot(queryJSON?.hash ?? 0);
+
+        renderHook(() => useSearchPageSetup(queryJSON));
+
+        // Without this, `errors` counts as a resolution and the page stays pinned to its error view
+        // with nothing in flight, recoverable only by tapping Try again.
+        expect(mockSearch).toHaveBeenCalledTimes(1);
+    });
+
+    it('does not re-request the same hash again when the recovery attempt fails', () => {
+        mockSearchResults = buildErroredSnapshot(queryJSON?.hash ?? 0);
+
+        const {rerender} = renderHook(() => useSearchPageSetup(queryJSON));
+
+        // The recovery attempt failed and wrote `errors` back, so the snapshot still looks recoverable.
+        // Move a real effect dependency as well, otherwise the effect never re-runs and this asserts nothing.
+        mockSearchKey = CONST.SEARCH.SEARCH_KEYS.EXPENSES;
+        rerender({});
+
+        expect(mockSearch).toHaveBeenCalledTimes(1);
+    });
+
+    it('keeps the recovery attempt available when search() declines to send', () => {
+        mockSearchResults = buildErroredSnapshot(queryJSON?.hash ?? 0);
+        // API prevention is on, so search() writes nothing and `errors` survives. Spending the attempt
+        // here would leave the query stuck for the rest of the mount.
+        mockSearch.mockReturnValue(undefined);
+
+        const {rerender} = renderHook(() => useSearchPageSetup(queryJSON));
+
+        mockSearch.mockReturnValue(Promise.resolve(undefined));
+        mockSearchKey = CONST.SEARCH.SEARCH_KEYS.EXPENSES;
+        rerender({});
+
+        expect(mockSearch).toHaveBeenCalledTimes(2);
+    });
+
+    it('does not re-request a query the server rejected as malformed', () => {
+        const snapshot = buildErroredSnapshot(queryJSON?.hash ?? 0);
+        mockSearchResults = {...snapshot, search: {...snapshot.search, responseJsonCode: CONST.JSON_CODE.INVALID_SEARCH_QUERY}};
+
+        renderHook(() => useSearchPageSetup(queryJSON));
+
+        expect(mockSearch).not.toHaveBeenCalled();
+    });
+
+    it('tracks the retry per hash, so returning to an already-retried hash does not retry it again', () => {
+        mockSearchResults = buildErroredSnapshot(queryJSON?.hash ?? 0);
+        const {rerender} = renderHook(({queryJSON: currentQueryJSON}) => useSearchPageSetup(currentQueryJSON), {initialProps: {queryJSON}});
+
+        // A second, independently errored query mounts. It gets its own retry.
+        mockSearchResults = buildErroredSnapshot(queryJSONB?.hash ?? 0);
+        rerender({queryJSON: queryJSONB});
+
+        // Back to the first query within the same mount. It already used its retry, so this must not fire a third call.
+        mockSearchResults = buildErroredSnapshot(queryJSON?.hash ?? 0);
+        rerender({queryJSON});
+
+        expect(mockSearch).toHaveBeenCalledTimes(2);
+    });
+});