diff --git a/src/CONST/index.ts b/src/CONST/index.ts index 7d37ca62ac30..3267dccd46e5 100644 --- a/src/CONST/index.ts +++ b/src/CONST/index.ts @@ -6558,6 +6558,13 @@ const CONST = { TRIP: 'trip', CHAT: 'chat', }, + // Terminal lifecycle state of a search snapshot's most recent request. Written by the search action lifecycle + // (loading on request start, loaded/error on resolve) so the snapshot always has an explicit, mutually exclusive state. + SNAPSHOT_STATE: { + LOADING: 'loading', + LOADED: 'loaded', + ERROR: 'error', + }, ACTION_FILTERS: { SUBMIT: 'submit', APPROVE: 'approve', diff --git a/src/libs/actions/Search.ts b/src/libs/actions/Search.ts index a4c17be3fcfd..1ba88e362a12 100644 --- a/src/libs/actions/Search.ts +++ b/src/libs/actions/Search.ts @@ -673,6 +673,14 @@ function getOnyxLoadingData( shouldCalculateTotals?: boolean, ): OnyxData { const shouldClearTotals = isSearchAPI && shouldCalculateTotals === false && offset === 0; + + // `search.state` tracks the lifecycle of a real search request (identified by its queryJSON): it starts as + // `loading` optimistically and is resolved to `loaded`/`error` by successData/failureData. handlePreventSearchAPI + // reuses this helper as a UI-only loading toggle with no query and no success/failure step, so it must stay out of + // the state machine — otherwise it would strand `state: loading` with no terminal write to clear it. + const isSearchRequest = isSearchAPI && !!queryJSON; + const type = queryJSON?.type; + const optimisticData: Array> = [ { onyxMethod: Onyx.METHOD.MERGE, @@ -680,6 +688,7 @@ function getOnyxLoadingData( value: { search: { ...(isSearchAPI && {isLoading: true}), + ...(isSearchRequest && {state: CONST.SEARCH.SNAPSHOT_STATE.LOADING}), ...(offset !== undefined ? {offset} : {}), ...(shouldClearTotals ? {count: null, total: null, currency: null} : {}), }, @@ -694,6 +703,26 @@ 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. + const successData: Array> = isSearchRequest + ? [ + { + onyxMethod: Onyx.METHOD.MERGE, + key: `${ONYXKEYS.COLLECTION.SNAPSHOT}${hash}`, + value: { + search: {state: CONST.SEARCH.SNAPSHOT_STATE.LOADED, type}, + }, + }, + ] + : []; + + // finallyData runs after successData/failureData regardless of jsonCode, so it deliberately does NOT write `state`: + // doing so would clobber the `error` terminal set by failureData. The terminal state is owned by success/failure. const finallyData: Array> = [ { onyxMethod: Onyx.METHOD.MERGE, @@ -713,15 +742,16 @@ function getOnyxLoadingData( value: { ...(isOffline ? {} : {data: null}), search: { - type: queryJSON?.type, + type, ...(isSearchAPI && {isLoading: false}), + ...(isSearchRequest && {state: CONST.SEARCH.SNAPSHOT_STATE.ERROR}), }, errors: getMicroSecondOnyxErrorWithTranslationKey('common.genericErrorMessage'), }, }, ]; - return {optimisticData, finallyData, failureData}; + return {optimisticData, successData, finallyData, failureData}; } function saveSearch({queryJSON, newName}: {queryJSON: Readonly; newName?: string}) { @@ -918,7 +948,7 @@ function search({ } inFlightSearchRequests.add(dedupeKey); - const {optimisticData, finallyData, failureData} = getOnyxLoadingData(queryJSON.hash, queryJSON, offset, isOffline, true, shouldCalculateTotals); + const {optimisticData, successData, finallyData, failureData} = getOnyxLoadingData(queryJSON.hash, queryJSON, offset, isOffline, true, shouldCalculateTotals); const {flatFilters, limit, ...queryJSONWithoutFlatFilters} = queryJSON; const backendQueryJSON = shouldUseBackendDateSortFallback(queryJSON.sortBy) ? { @@ -950,7 +980,7 @@ function search({ } const startRequest = () => - makeRequestWithSideEffects(READ_COMMANDS.SEARCH, {hash: queryJSON.hash, jsonQuery}, {optimisticData, finallyData, failureData}) + makeRequestWithSideEffects(READ_COMMANDS.SEARCH, {hash: queryJSON.hash, jsonQuery}, {optimisticData, successData, finallyData, failureData}) .then((result) => { if (shouldUpdateLastSearchParams) { const response = result?.onyxData?.[0]?.value as OnyxSearchResponse; @@ -983,6 +1013,14 @@ function search({ return result?.jsonCode; }) + .catch((error) => { + // A network-level rejection (no HTTP response at all, e.g. offline/timeout) never reaches + // SaveResponseInOnyx, so nothing else applies failureData for it. Apply it here so the snapshot + // still reaches a terminal `error` state instead of being stranded in `loading`, then re-throw so + // this still rejects for any caller relying on that. + Onyx.update(failureData ?? []); + throw error; + }) .finally(() => { inFlightSearchRequests.delete(dedupeKey); }); diff --git a/src/types/onyx/SearchResults.ts b/src/types/onyx/SearchResults.ts index 21e13cd39db3..e4d5cc7a9159 100644 --- a/src/types/onyx/SearchResults.ts +++ b/src/types/onyx/SearchResults.ts @@ -59,6 +59,18 @@ type SearchResultsInfo = { /** Whether the search results are currently loading */ isLoading: boolean; + /** Explicit terminal lifecycle state of the most recent search request for this snapshot. + * Optional because snapshots persisted before this field existed (and snapshots written by + * non-search actions) may not carry it. + * + * Nothing reads this field yet. The existing isLoading/type/status-based loading and error gates + * migrate to read it in a follow-up PR. + * + * Residual limitation: if the app is killed or reloaded mid-request, no cleanup runs, so `loading` can + * still be stranded on disk. The future read side must treat a `loading` state with no in-flight request + * as stale. */ + state?: ValueOf; + /** The number of results */ count?: number; diff --git a/tests/unit/Search/searchSnapshotStateTest.ts b/tests/unit/Search/searchSnapshotStateTest.ts new file mode 100644 index 000000000000..05bf7d9f8711 --- /dev/null +++ b/tests/unit/Search/searchSnapshotStateTest.ts @@ -0,0 +1,142 @@ +import {search} from '@libs/actions/Search'; +import {makeRequestWithSideEffects, waitForWrites} from '@libs/API'; +import {buildSearchQueryJSON} from '@libs/SearchQueryUtils'; + +import CONST from '@src/CONST'; +import ONYXKEYS from '@src/ONYXKEYS'; + +import type {OnyxUpdate} from 'react-native-onyx'; + +import Onyx from 'react-native-onyx'; + +import getOnyxValue from '../../utils/getOnyxValue'; +import waitForBatchedUpdates from '../../utils/waitForBatchedUpdates'; + +jest.mock('@libs/API', () => ({ + makeRequestWithSideEffects: jest.fn(), + waitForWrites: jest.fn(), + write: jest.fn(), + read: jest.fn(), +})); + +// A jsonCode that is not 200, so the request resolves down the failure branch of applyHTTPSOnyxUpdates. +const FAILURE_JSON_CODE = 500; + +function getQueryJSON() { + const queryJSON = buildSearchQueryJSON(''); + if (!queryJSON) { + throw new Error('Query JSON should be defined for test setup'); + } + + return queryJSON; +} + +/** Read the {optimisticData, successData, failureData, finallyData} that search() handed to the API layer. */ +function getCapturedSearchOnyxData(): NonNullable[2]> { + const lastCall = jest.mocked(makeRequestWithSideEffects).mock.calls.at(-1); + if (!lastCall) { + throw new Error('makeRequestWithSideEffects was not called'); + } + return lastCall[2] ?? {}; +} + +/** + * Replays a captured search request the way applyHTTPSOnyxUpdates does: optimisticData first, then any server + * onyxData, then successData (jsonCode 200) or failureData (non-200), and finallyData last regardless of jsonCode. + * This lets us assert the terminal snapshot state that the real API application order would produce. + */ +async function simulateResolvedRequest({jsonCode, serverOnyxData}: {jsonCode: number; serverOnyxData?: Array>}) { + const {optimisticData, successData, failureData, finallyData} = getCapturedSearchOnyxData(); + + await Onyx.update(optimisticData ?? []); + if (serverOnyxData) { + await Onyx.update(serverOnyxData); + } + if (jsonCode === CONST.JSON_CODE.SUCCESS) { + await Onyx.update(successData ?? []); + } else { + await Onyx.update(failureData ?? []); + } + await Onyx.update(finallyData ?? []); + await waitForBatchedUpdates(); +} + +describe('search snapshot terminal state', () => { + beforeAll(() => { + Onyx.init({keys: ONYXKEYS}); + }); + + beforeEach(async () => { + jest.clearAllMocks(); + jest.mocked(waitForWrites).mockResolvedValue(undefined); + jest.mocked(makeRequestWithSideEffects).mockResolvedValue(undefined); + await Onyx.clear(); + }); + + it('marks the snapshot loading optimistically while a request is in flight', async () => { + const queryJSON = getQueryJSON(); + + await search({queryJSON, searchKey: CONST.SEARCH.SEARCH_KEYS.EXPENSES, offset: 0, isLoading: false}); + const {optimisticData} = getCapturedSearchOnyxData(); + await Onyx.update(optimisticData ?? []); + await waitForBatchedUpdates(); + + const snapshot = await getOnyxValue(`${ONYXKEYS.COLLECTION.SNAPSHOT}${queryJSON.hash}` as const); + expect(snapshot?.search?.state).toBe(CONST.SEARCH.SNAPSHOT_STATE.LOADING); + }); + + it('resolves the snapshot to loaded on a successful response', async () => { + const queryJSON = getQueryJSON(); + + await search({queryJSON, searchKey: CONST.SEARCH.SEARCH_KEYS.EXPENSES, offset: 0, isLoading: false}); + await simulateResolvedRequest({ + jsonCode: CONST.JSON_CODE.SUCCESS, + serverOnyxData: [ + { + onyxMethod: Onyx.METHOD.MERGE, + key: `${ONYXKEYS.COLLECTION.SNAPSHOT}${queryJSON.hash}`, + value: {search: {hasResults: true}}, + }, + ], + }); + + const snapshot = await getOnyxValue(`${ONYXKEYS.COLLECTION.SNAPSHOT}${queryJSON.hash}` as const); + expect(snapshot?.search?.state).toBe(CONST.SEARCH.SNAPSHOT_STATE.LOADED); + expect(snapshot?.search?.hasResults).toBe(true); + }); + + it('resolves the snapshot to error on a failed response', async () => { + const queryJSON = getQueryJSON(); + + await search({queryJSON, searchKey: CONST.SEARCH.SEARCH_KEYS.EXPENSES, offset: 0, isLoading: false}); + await simulateResolvedRequest({jsonCode: FAILURE_JSON_CODE}); + + 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); + }); + + it('reaches a terminal state when a successful response resolves without any snapshot data', async () => { + const queryJSON = getQueryJSON(); + + await search({queryJSON, searchKey: CONST.SEARCH.SEARCH_KEYS.EXPENSES, offset: 0, isLoading: false}); + // The bug scenario: a 200 response that writes no server onyxData at all. + await simulateResolvedRequest({jsonCode: CONST.JSON_CODE.SUCCESS}); + + const snapshot = await getOnyxValue(`${ONYXKEYS.COLLECTION.SNAPSHOT}${queryJSON.hash}` as const); + expect(snapshot?.search?.state).toBe(CONST.SEARCH.SNAPSHOT_STATE.LOADED); + }); + + it('resolves the snapshot to error when the request promise rejects, instead of staying loading', async () => { + const queryJSON = getQueryJSON(); + jest.mocked(makeRequestWithSideEffects).mockRejectedValueOnce(new Error('Network request failed')); + + // The failure class this field exists to eliminate: no HTTP response at all (offline/timeout), so + // nothing in the API layer ever applies failureData for it unless search() catches the rejection itself. + await expect(search({queryJSON, searchKey: CONST.SEARCH.SEARCH_KEYS.EXPENSES, offset: 0, isLoading: false})).rejects.toThrow(); + await waitForBatchedUpdates(); + + const snapshot = await getOnyxValue(`${ONYXKEYS.COLLECTION.SNAPSHOT}${queryJSON.hash}` as const); + expect(snapshot?.search?.state).toBe(CONST.SEARCH.SNAPSHOT_STATE.ERROR); + }); +});