diff --git a/src/hooks/useSearchLoadingState.ts b/src/hooks/useSearchLoadingState.ts index d8e927eb0b79..7850242be924 100644 --- a/src/hooks/useSearchLoadingState.ts +++ b/src/hooks/useSearchLoadingState.ts @@ -29,13 +29,16 @@ function useSearchLoadingState(queryJSON: SearchQueryJSON | undefined, searchRes const isCardFeedsLoading = validGroupBy === CONST.SEARCH.GROUP_BY.CARD && cardFeedsResult?.status === 'loading'; const hasErrors = Object.keys(searchResults?.errors ?? {}).length > 0; + // A resolved request stamps `state: loaded` even when it wrote no data. That terminal state hands off to + // Search's own empty view, so a dataless 200 no longer pins the page skeleton on the `data === undefined` check. + const isLoaded = searchResults?.search?.state === CONST.SEARCH.SNAPSHOT_STATE.LOADED; - // Show page-level skeleton when no data has ever arrived for this query, + // Show page-level skeleton when no data has ever arrived for this query and the request has not resolved, // or when card feeds are still loading for card-grouped searches. - // Once data arrives (even empty []), Search mounts and handles its own + // Once data arrives (even empty []) or the request resolves, Search mounts and handles its own // loading/empty states internally via shouldShowLoadingState. // When errors are present, let Search mount so it can render FullPageErrorView. - return (hasNoData && !hasErrors) || isCardFeedsLoading; + return (hasNoData && !hasErrors && !isLoaded) || isCardFeedsLoading; } export default useSearchLoadingState; diff --git a/src/hooks/useSearchPageSetup.ts b/src/hooks/useSearchPageSetup.ts index c57f4d247065..cd586750a4a7 100644 --- a/src/hooks/useSearchPageSetup.ts +++ b/src/hooks/useSearchPageSetup.ts @@ -36,10 +36,9 @@ function useSearchPageSetup(queryJSON: Readonly | undefined) { const hash = queryJSON?.hash; const shouldCalculateTotals = useSearchShouldCalculateTotals(currentSearchKey, hash, true); - // Derived primitives so effects do not depend on the whole snapshot object (new reference every + // Derived primitive so the effect does not depend on the whole snapshot object (new reference every // Onyx merge) while exhaustive-deps still sees every transition that matters for firing search(). const isSnapshotDataLoaded = queryJSON ? isSearchDataLoaded(currentSearchResults, queryJSON) : false; - const isSnapshotSearchLoading = !!currentSearchResults?.search?.isLoading; // Clear selected transactions when navigating to a different search query function clearOnHashChange() { @@ -69,12 +68,16 @@ function useSearchPageSetup(queryJSON: Readonly | undefined) { lastSavedSearchHash = hash; } - if (isSnapshotDataLoaded || isSnapshotSearchLoading) { + // Only skip when the snapshot already holds resolved data for this query. Do not gate on a stored + // loading flag: a reload/crash mid-request strands it and would block the re-fire forever. search() + // dedupes a genuinely in-flight request through its own module-memory registry, which resets on + // reload, so a stranded loading state re-fires and self-heals instead of pinning the skeleton. + if (isSnapshotDataLoaded) { 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, currentSearchKey, shouldCalculateTotals]); + }, [hash, isOffline, shouldUseLiveData, queryJSON, isSnapshotDataLoaded, currentSearchKey, shouldCalculateTotals]); useFocusEffect(() => { openSearch(); diff --git a/src/libs/SearchUIUtils.ts b/src/libs/SearchUIUtils.ts index 8a5919ae4c56..e14f9ab4fbee 100644 --- a/src/libs/SearchUIUtils.ts +++ b/src/libs/SearchUIUtils.ts @@ -4802,8 +4802,25 @@ function shouldShowEmptyState(isDataLoaded: boolean, dataLength: number, type: S return !isDataLoaded || dataLength === 0 || !type || !Object.values(CONST.SEARCH.DATA_TYPES).includes(type); } +/** + * Whether a real search request for this snapshot is currently in flight: the search action stamps + * `state: loading` optimistically and resolves it to `loaded`/`error`, so `loading` means "not yet resolved". + * Prefer this over the legacy `search.isLoading` flag: `state` always reaches a terminal write (success, + * failure or the network-catch failureData), so it cannot get pinned by a dropped response the way the flag can. + */ +function isSearchPending(searchResults: SearchResults | undefined) { + return searchResults?.search?.state === CONST.SEARCH.SNAPSHOT_STATE.LOADING; +} + function isSearchDataLoaded(searchResults: SearchResults | undefined, queryJSON: Readonly | undefined) { - const isDataLoaded = (searchResults?.data != null || searchResults?.errors != null) && searchResults?.search?.type === queryJSON?.type && searchResults.search.hash === queryJSON?.hash; + const state = searchResults?.search?.state; + // A terminal `state` (loaded/error) means the request for this snapshot resolved, even when it wrote no data + // (a dataless 200 that used to pin the skeleton forever). It counts as loaded alongside the original + // data/errors shape check, still guarded by the type/hash match that rejects a stale snapshot Onyx can + // transiently return for the previous query. + const isTerminal = state === CONST.SEARCH.SNAPSHOT_STATE.LOADED || state === CONST.SEARCH.SNAPSHOT_STATE.ERROR; + const hasResolved = searchResults?.data != null || searchResults?.errors != null || isTerminal; + const isDataLoaded = hasResolved && searchResults?.search?.type === queryJSON?.type && searchResults?.search?.hash === queryJSON?.hash; return isDataLoaded; } @@ -6515,6 +6532,7 @@ export { shouldShowEmptyState, compareValues, isSearchDataLoaded, + isSearchPending, getValidGroupBy, getTypeOptions, getSortByOptions, diff --git a/src/libs/actions/Search.ts b/src/libs/actions/Search.ts index 1ba88e362a12..422fc21af1e5 100644 --- a/src/libs/actions/Search.ts +++ b/src/libs/actions/Search.ts @@ -26,6 +26,7 @@ import deferModalPresentationAfterPopoverDismiss from '@libs/deferModalPresentat import {getMicroSecondOnyxErrorWithTranslationKey} from '@libs/ErrorUtils'; import fileDownload from '@libs/fileDownload'; import {getExportFileName} from '@libs/fileDownload/FileUtils'; +import Log from '@libs/Log'; import createDynamicRoute from '@libs/Navigation/helpers/dynamicRoutesUtils/createDynamicRoute'; import isSearchTopmostFullScreenRoute from '@libs/Navigation/helpers/isSearchTopmostFullScreenRoute'; import Navigation, {navigationRef} from '@libs/Navigation/Navigation'; @@ -703,19 +704,19 @@ function getOnyxLoadingData( }, ]; - // successData writes the terminal `loaded` state on any jsonCode 200 resolve. It also stamps `type` so - // responses that do carry data stay consistent with the anti-stale isSearchDataLoaded check (which compares - // type/hash). On a success response without data, isSearchDataLoaded still resolves to false via its own data/errors gate; - // `state` is what marks that case as done once a future PR wires the read side to it. `isLoading` isn't set here - // because finallyData always runs right after and already clears it for isSearchAPI. Empty for the non-search - // callers of this helper so they don't pay for a meaningless `{search: {}}` merge on the SNAPSHOT key. + // successData writes the terminal `loaded` state on any jsonCode 200 resolve, stamping `type` and `hash` so + // the terminal state satisfies the anti-stale isSearchDataLoaded check (which matches type/hash) even on a + // dataless response that carries none of its own. isSearchDataLoaded now treats a terminal state as resolved, + // so such a response resolves to Search's empty view instead of pinning the skeleton on the data-shape check. + // `isLoading` isn't set here because finallyData always runs right after and already clears it for isSearchAPI. + // Empty for the non-search callers of this helper so they don't pay for a meaningless `{search: {}}` merge. const successData: Array> = isSearchRequest ? [ { onyxMethod: Onyx.METHOD.MERGE, key: `${ONYXKEYS.COLLECTION.SNAPSHOT}${hash}`, value: { - search: {state: CONST.SEARCH.SNAPSHOT_STATE.LOADED, type}, + search: {state: CONST.SEARCH.SNAPSHOT_STATE.LOADED, type, hash}, }, }, ] @@ -744,7 +745,7 @@ function getOnyxLoadingData( search: { type, ...(isSearchAPI && {isLoading: false}), - ...(isSearchRequest && {state: CONST.SEARCH.SNAPSHOT_STATE.ERROR}), + ...(isSearchRequest && {state: CONST.SEARCH.SNAPSHOT_STATE.ERROR, hash}), }, errors: getMicroSecondOnyxErrorWithTranslationKey('common.genericErrorMessage'), }, @@ -982,8 +983,16 @@ function search({ const startRequest = () => makeRequestWithSideEffects(READ_COMMANDS.SEARCH, {hash: queryJSON.hash, jsonQuery}, {optimisticData, successData, finallyData, failureData}) .then((result) => { + const response = result?.onyxData?.[0]?.value as OnyxSearchResponse; + + // Observability for the failure mode this read side converts from an infinite skeleton into an empty + // view: a jsonCode 200 that wrote no snapshot data. It used to be invisible (the skeleton just hung), + // so log it to keep the dataless-resolve case queryable now that it silently resolves. + if (result?.jsonCode === 200 && response?.data === undefined) { + Log.info('[Search] loading_terminal_empty', false, {hash: queryJSON.hash, type: queryJSON.type}); + } + if (shouldUpdateLastSearchParams) { - const response = result?.onyxData?.[0]?.value as OnyxSearchResponse; const reports = Object.keys(response?.data ?? {}) .filter((key) => key.startsWith(ONYXKEYS.COLLECTION.REPORT)) .map((key) => key.replace(ONYXKEYS.COLLECTION.REPORT, '')); diff --git a/src/pages/Search/SearchPage.tsx b/src/pages/Search/SearchPage.tsx index 27300f3fd1d9..7aa5c1a35426 100644 --- a/src/pages/Search/SearchPage.tsx +++ b/src/pages/Search/SearchPage.tsx @@ -78,8 +78,6 @@ function SearchPage({route}: SearchPageProps) { searchResults = lastNonEmptySearchResults; } - const metadata = searchResults?.search; - useEffect(() => { if (shouldUseNarrowLayout) { return; @@ -140,7 +138,6 @@ function SearchPage({route}: SearchPageProps) { {shouldUseNarrowLayout ? ( void; @@ -75,7 +73,6 @@ function SearchPageNarrow({ queryJSON, searchResults, isMobileSelectionModeEnabled, - metadata, onSortPressedCallback, searchOverlayContent, onSearchContentReady, @@ -231,7 +228,10 @@ function SearchPageNarrow({ } const isDataLoaded = shouldUseLiveData || isSearchDataLoaded(searchResults, queryJSON); - const shouldShowLoadingState = !isOffline && (!isDataLoaded || !!metadata?.isLoading); + // Show the loading bar while data is missing or a request is genuinely pending. Read the pending state + // from the request lifecycle (`state`) rather than the legacy `isLoading` flag so a dataless resolve or a + // stranded flag cannot keep it up. + const shouldShowLoadingState = !isOffline && (!isDataLoaded || isSearchPending(searchResults)); const contentContainerStyle = !isMobileSelectionModeEnabled ? styles.searchListContentContainerStyles(hasFilterBars) : undefined; return ( diff --git a/tests/unit/Search/SearchUIUtilsTest.ts b/tests/unit/Search/SearchUIUtilsTest.ts index 2447829c1282..ac43adaf479a 100644 --- a/tests/unit/Search/SearchUIUtilsTest.ts +++ b/tests/unit/Search/SearchUIUtilsTest.ts @@ -8349,6 +8349,99 @@ describe('SearchUIUtils', () => { it('should return false when queryJSON is undefined but searchResults has a concrete type and hash', () => { expect(SearchUIUtils.isSearchDataLoaded(makeSearchResults(), undefined)).toBe(false); }); + + it('should return true on a dataless response that reached a terminal loaded state (type and hash match)', () => { + const results = makeSearchResults({ + data: undefined, + errors: undefined, + search: { + hasMoreResults: false, + hasResults: false, + offset: 0, + hash: queryJSON?.hash ?? 0, + isLoading: false, + type: CONST.SEARCH.DATA_TYPES.EXPENSE, + state: CONST.SEARCH.SNAPSHOT_STATE.LOADED, + }, + }); + expect(SearchUIUtils.isSearchDataLoaded(results, queryJSON)).toBe(true); + }); + + it('should return true when the request reached a terminal error state', () => { + const results = makeSearchResults({ + data: undefined, + errors: undefined, + search: { + hasMoreResults: false, + hasResults: false, + offset: 0, + hash: queryJSON?.hash ?? 0, + isLoading: false, + type: CONST.SEARCH.DATA_TYPES.EXPENSE, + state: CONST.SEARCH.SNAPSHOT_STATE.ERROR, + }, + }); + expect(SearchUIUtils.isSearchDataLoaded(results, queryJSON)).toBe(true); + }); + + it('should return false while a request is still loading with no data yet', () => { + const results = makeSearchResults({ + data: undefined, + errors: undefined, + search: { + hasMoreResults: false, + hasResults: false, + offset: 0, + hash: queryJSON?.hash ?? 0, + isLoading: true, + type: CONST.SEARCH.DATA_TYPES.EXPENSE, + state: CONST.SEARCH.SNAPSHOT_STATE.LOADING, + }, + }); + expect(SearchUIUtils.isSearchDataLoaded(results, queryJSON)).toBe(false); + }); + + it('should return false when a terminal state belongs to a stale snapshot (hash mismatch)', () => { + const results = makeSearchResults({ + data: undefined, + errors: undefined, + search: { + hasMoreResults: false, + hasResults: false, + offset: 0, + hash: (queryJSON?.hash ?? 0) + 1, + isLoading: false, + type: CONST.SEARCH.DATA_TYPES.EXPENSE, + state: CONST.SEARCH.SNAPSHOT_STATE.LOADED, + }, + }); + expect(SearchUIUtils.isSearchDataLoaded(results, queryJSON)).toBe(false); + }); + }); + + describe('Test isSearchPending', () => { + const queryJSON = buildSearchQueryJSON('type:expense'); + + function makeSearch(state: OnyxTypes.SearchResults['search']['state']): OnyxTypes.SearchResults { + return { + data: {personalDetailsList: {}}, + search: {hasMoreResults: false, hasResults: true, offset: 0, hash: queryJSON?.hash ?? 0, isLoading: false, type: CONST.SEARCH.DATA_TYPES.EXPENSE, state}, + }; + } + + it('should return true only while the snapshot state is loading', () => { + expect(SearchUIUtils.isSearchPending(makeSearch(CONST.SEARCH.SNAPSHOT_STATE.LOADING))).toBe(true); + }); + + it('should return false for the terminal loaded and error states', () => { + expect(SearchUIUtils.isSearchPending(makeSearch(CONST.SEARCH.SNAPSHOT_STATE.LOADED))).toBe(false); + expect(SearchUIUtils.isSearchPending(makeSearch(CONST.SEARCH.SNAPSHOT_STATE.ERROR))).toBe(false); + }); + + it('should return false when the state is absent or searchResults is undefined', () => { + expect(SearchUIUtils.isSearchPending(makeSearch(undefined))).toBe(false); + expect(SearchUIUtils.isSearchPending(undefined)).toBe(false); + }); }); describe('Test isSearchResultsEmpty', () => { diff --git a/tests/unit/Search/searchSnapshotStateTest.ts b/tests/unit/Search/searchSnapshotStateTest.ts index 05bf7d9f8711..743453bf0de9 100644 --- a/tests/unit/Search/searchSnapshotStateTest.ts +++ b/tests/unit/Search/searchSnapshotStateTest.ts @@ -102,6 +102,7 @@ describe('search snapshot terminal state', () => { const snapshot = await getOnyxValue(`${ONYXKEYS.COLLECTION.SNAPSHOT}${queryJSON.hash}` as const); expect(snapshot?.search?.state).toBe(CONST.SEARCH.SNAPSHOT_STATE.LOADED); + expect(snapshot?.search?.hash).toBe(queryJSON.hash); expect(snapshot?.search?.hasResults).toBe(true); }); @@ -114,6 +115,7 @@ describe('search snapshot terminal state', () => { const snapshot = await getOnyxValue(`${ONYXKEYS.COLLECTION.SNAPSHOT}${queryJSON.hash}` as const); // finallyData runs after failureData; the error state must survive it. expect(snapshot?.search?.state).toBe(CONST.SEARCH.SNAPSHOT_STATE.ERROR); + expect(snapshot?.search?.hash).toBe(queryJSON.hash); }); it('reaches a terminal state when a successful response resolves without any snapshot data', async () => { @@ -125,6 +127,9 @@ describe('search snapshot terminal state', () => { const snapshot = await getOnyxValue(`${ONYXKEYS.COLLECTION.SNAPSHOT}${queryJSON.hash}` as const); expect(snapshot?.search?.state).toBe(CONST.SEARCH.SNAPSHOT_STATE.LOADED); + // The terminal state carries the query hash so the read side can treat it as resolved even though the + // dataless response wrote neither data nor its own search metadata. + expect(snapshot?.search?.hash).toBe(queryJSON.hash); }); it('resolves the snapshot to error when the request promise rejects, instead of staying loading', async () => {