Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions src/hooks/useSearchLoadingState.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
11 changes: 7 additions & 4 deletions src/hooks/useSearchPageSetup.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,10 +36,9 @@ function useSearchPageSetup(queryJSON: Readonly<SearchQueryJSON> | 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() {
Expand Down Expand Up @@ -69,12 +68,16 @@ function useSearchPageSetup(queryJSON: Readonly<SearchQueryJSON> | 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();
Expand Down
20 changes: 19 additions & 1 deletion src/libs/SearchUIUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<SearchQueryJSON> | 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;
}
Expand Down Expand Up @@ -6515,6 +6532,7 @@ export {
shouldShowEmptyState,
compareValues,
isSearchDataLoaded,
isSearchPending,
getValidGroupBy,
getTypeOptions,
getSortByOptions,
Expand Down
27 changes: 18 additions & 9 deletions src/libs/actions/Search.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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<OnyxUpdate<typeof ONYXKEYS.COLLECTION.SNAPSHOT>> = 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},
},
},
]
Expand Down Expand Up @@ -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'),
},
Expand Down Expand Up @@ -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, ''));
Expand Down
3 changes: 0 additions & 3 deletions src/pages/Search/SearchPage.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -78,8 +78,6 @@ function SearchPage({route}: SearchPageProps) {
searchResults = lastNonEmptySearchResults;
}

const metadata = searchResults?.search;

useEffect(() => {
if (shouldUseNarrowLayout) {
return;
Expand Down Expand Up @@ -140,7 +138,6 @@ function SearchPage({route}: SearchPageProps) {
{shouldUseNarrowLayout ? (
<SearchPageNarrow
queryJSON={currentSearchQueryJSON}
metadata={metadata}
searchResults={searchResults}
isMobileSelectionModeEnabled={isMobileSelectionModeEnabled}
onSortPressedCallback={onSortPressedCallback}
Expand Down
10 changes: 5 additions & 5 deletions src/pages/Search/SearchPageNarrow/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ import useWindowDimensions from '@hooks/useWindowDimensions';
import {turnOffMobileSelectionMode} from '@libs/actions/MobileSelectionMode';
import Navigation from '@libs/Navigation/Navigation';
import {buildCannedSearchQuery} from '@libs/SearchQueryUtils';
import {isSearchDataLoaded} from '@libs/SearchUIUtils';
import {isSearchDataLoaded, isSearchPending} from '@libs/SearchUIUtils';
import {getPendingSubmitFollowUpAction} from '@libs/telemetry/submitFollowUpAction';

import variables from '@styles/variables';
Expand All @@ -39,7 +39,6 @@ import {search} from '@userActions/Search';
import CONST from '@src/CONST';
import ROUTES from '@src/ROUTES';
import type {SearchResults} from '@src/types/onyx';
import type {SearchResultsInfo} from '@src/types/onyx/SearchResults';

import {useFocusEffect, useNavigation, useRoute} from '@react-navigation/native';
import React, {useCallback, useContext, useEffect, useRef, useState, useTransition} from 'react';
Expand All @@ -55,7 +54,6 @@ const ANIMATION_DURATION_IN_MS = 300;

type SearchPageNarrowProps = {
queryJSON?: SearchQueryJSON;
metadata?: SearchResultsInfo;
searchResults?: SearchResults;
isMobileSelectionModeEnabled: boolean;
onSortPressedCallback: () => void;
Expand All @@ -75,7 +73,6 @@ function SearchPageNarrow({
queryJSON,
searchResults,
isMobileSelectionModeEnabled,
metadata,
onSortPressedCallback,
searchOverlayContent,
onSearchContentReady,
Expand Down Expand Up @@ -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 (
Expand Down
93 changes: 93 additions & 0 deletions tests/unit/Search/SearchUIUtilsTest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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', () => {
Expand Down
5 changes: 5 additions & 0 deletions tests/unit/Search/searchSnapshotStateTest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
});

Expand All @@ -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 () => {
Expand All @@ -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 () => {
Expand Down
Loading