Skip to content
Open
Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
5af0782
Refactor Search component and add loading skeletons
szymonzalarski98 Mar 5, 2026
a419593
Merge remote-tracking branch 'origin/main' into callstack-internal/sz…
szymonzalarski98 Mar 5, 2026
f49723d
Fix mobile-expensify
szymonzalarski98 Mar 5, 2026
33f9840
Refactor search loading state logic and optimize context synchronizat…
szymonzalarski98 Mar 6, 2026
87d6f70
Retrigger prettier
szymonzalarski98 Mar 6, 2026
803f2e6
Merge branch 'main' into callstack-internal/szymonzalarski/search/mov…
szymonzalarski98 Mar 6, 2026
7075730
Retrigger prettier
szymonzalarski98 Mar 6, 2026
c389d60
Fix failing prettier
szymonzalarski98 Mar 9, 2026
e0c5f7d
PR fixes
szymonzalarski98 Mar 10, 2026
8e8480d
Merge branch 'main' into callstack-internal/szymonzalarski/search/mov…
szymonzalarski98 Mar 10, 2026
8067359
Retrigger CI/CD checks
szymonzalarski98 Mar 10, 2026
833048d
Merge branch 'callstack-internal/szymonzalarski/search/move-skeleton-…
szymonzalarski98 Mar 10, 2026
da450a5
Refactor useSearchPageSetup hook to remove unnecessary useRef and use…
szymonzalarski98 Mar 10, 2026
92140a6
Merge remote-tracking branch 'upstream/main' into callstack-internal/…
szymonzalarski98 Mar 11, 2026
89dc6da
Merge remote-tracking branch 'upstream/main' into callstack-internal/…
szymonzalarski98 Mar 12, 2026
6fe3bff
Update Mobile-Expensify to remove it from files changes
szymonzalarski98 Mar 12, 2026
c4ad5e2
Merge branch 'main' into callstack-internal/szymonzalarski/search/mov…
szymonzalarski98 Mar 12, 2026
db145a1
PR fixes
szymonzalarski98 Mar 13, 2026
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
33 changes: 33 additions & 0 deletions src/components/Search/SearchLoadingSkeleton.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
import React from 'react';
import type {StyleProp, ViewStyle} from 'react-native';
import Animated, {FadeIn, FadeOut} from 'react-native-reanimated';
import SearchRowSkeleton from '@components/Skeletons/SearchRowSkeleton';
import useThemeStyles from '@hooks/useThemeStyles';
import {endSpanWithAttributes} from '@libs/telemetry/activeSpans';
import CONST from '@src/CONST';

type SearchLoadingSkeletonProps = {
containerStyle?: StyleProp<ViewStyle>;
};

function SearchLoadingSkeleton({containerStyle}: SearchLoadingSkeletonProps) {
const styles = useThemeStyles();

return (
<Animated.View
entering={FadeIn.duration(CONST.SEARCH.ANIMATION.FADE_DURATION)}
exiting={FadeOut.duration(CONST.SEARCH.ANIMATION.FADE_DURATION)}
style={[styles.flex1]}
onLayout={() => {
endSpanWithAttributes(CONST.TELEMETRY.SPAN_NAVIGATE_TO_REPORTS, {[CONST.TELEMETRY.ATTRIBUTE_IS_WARM]: false});
}}
>
<SearchRowSkeleton
shouldAnimate
containerStyle={containerStyle}
/>
</Animated.View>
);
}

export default SearchLoadingSkeleton;
82 changes: 6 additions & 76 deletions src/components/Search/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import React, {useCallback, useEffect, useMemo, useRef, useState} from 'react';
import type {NativeScrollEvent, NativeSyntheticEvent, StyleProp, ViewStyle} from 'react-native';
import {View} from 'react-native';
import type {OnyxEntry} from 'react-native-onyx';
import Animated, {FadeIn, FadeOut, useAnimatedStyle, useSharedValue, withTiming} from 'react-native-reanimated';
import Animated, {useAnimatedStyle, useSharedValue, withTiming} from 'react-native-reanimated';
import FullPageErrorView from '@components/BlockingViews/FullPageErrorView';
import FullPageOfflineBlockingView from '@components/BlockingViews/FullPageOfflineBlockingView';
import {ModalActions} from '@components/Modal/Global/ModalContext';
Expand Down Expand Up @@ -37,7 +37,7 @@ import useThemeStyles from '@hooks/useThemeStyles';
import {openOldDotLink} from '@libs/actions/Link';
import {turnOffMobileSelectionMode, turnOnMobileSelectionMode} from '@libs/actions/MobileSelectionMode';
import type {TransactionPreviewData} from '@libs/actions/Search';
import {openSearch, setOptimisticDataForTransactionThreadPreview} from '@libs/actions/Search';
import {setOptimisticDataForTransactionThreadPreview} from '@libs/actions/Search';
import {canUseTouchScreen} from '@libs/DeviceCapabilities';
import Log from '@libs/Log';
import isSearchTopmostFullScreenRoute from '@libs/Navigation/helpers/isSearchTopmostFullScreenRoute';
Expand All @@ -52,6 +52,7 @@ import {
getSections,
getSortedSections,
getSuggestedSearches,
getValidGroupBy,
getWideAmountIndicators,
isGroupedItemArray,
isReportActionListItemType,
Expand Down Expand Up @@ -230,16 +231,8 @@ function Search({
const {markReportIDAsExpense} = useWideRHPActions();
const {currentSearchHash, selectedTransactions, shouldTurnOffSelectionMode, lastSearchType, areAllMatchingItemsSelected, shouldResetSearchQuery, shouldUseLiveData} =
useSearchStateContext();
const {
setCurrentSearchHashAndKey,
setCurrentSearchQueryJSON,
setSelectedTransactions,
clearSelectedTransactions,
setShouldShowFiltersBarLoading,
setShouldShowSelectAllMatchingItems,
selectAllMatchingItems,
setShouldResetSearchQuery,
} = useSearchActionsContext();
const {setSelectedTransactions, clearSelectedTransactions, setShouldShowFiltersBarLoading, setShouldShowSelectAllMatchingItems, selectAllMatchingItems, setShouldResetSearchQuery} =
useSearchActionsContext();
const [offset, setOffset] = useState(0);

const [transactions] = useOnyx(ONYXKEYS.COLLECTION.TRANSACTION);
Expand Down Expand Up @@ -305,22 +298,7 @@ function Search({
}
}, [onDEWModalOpen, showConfirmModal, translate]);

const clearTransactionsAndSetHashAndKey = useCallback(() => {
clearSelectedTransactions(hash);
setCurrentSearchHashAndKey(hash, recentSearchHash, searchKey);
setCurrentSearchQueryJSON(queryJSON);
}, [hash, recentSearchHash, searchKey, clearSelectedTransactions, setCurrentSearchHashAndKey, setCurrentSearchQueryJSON, queryJSON]);

useFocusEffect(clearTransactionsAndSetHashAndKey);

useEffect(() => {
clearTransactionsAndSetHashAndKey();

// Trigger once on mount (e.g., on page reload), when RHP is open and screen is not focused
// eslint-disable-next-line react-hooks/exhaustive-deps
}, []);

const validGroupBy = groupBy && Object.values(CONST.SEARCH.GROUP_BY).includes(groupBy) ? groupBy : undefined;
const validGroupBy = getValidGroupBy(groupBy);
const prevValidGroupBy = usePrevious(validGroupBy);
const isSearchResultsEmpty = !searchResults?.data || isSearchResultsEmptyUtil(searchResults, validGroupBy);

Expand Down Expand Up @@ -368,17 +346,6 @@ function Search({
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [isSmallScreenWidth]);

useEffect(() => {
openSearch({includePartiallySetupBankAccounts: true});
}, []);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should also remove the skeleton logic from SearchList since we now do this up the tree

if (shouldShowLoadingState) {
return (
<Animated.View
entering={FadeIn.duration(CONST.SEARCH.ANIMATION.FADE_DURATION)}
exiting={FadeOut.duration(CONST.SEARCH.ANIMATION.FADE_DURATION)}
style={[styles.flex1]}
onLayout={onLayoutSkeleton}
>
<SearchRowSkeleton
shouldAnimate
containerStyle={shouldUseNarrowLayout ? styles.searchListContentContainerStyles : styles.mt3}
/>
</Animated.View>
);
}

useEffect(() => {
if (!prevIsOffline || isOffline) {
return;
}
openSearch({includePartiallySetupBankAccounts: true});
}, [isOffline, prevIsOffline]);

const {newSearchResultKeys, handleSelectionListScroll, newTransactions} = useSearchHighlightAndScroll({
searchResults,
transactions,
Expand Down Expand Up @@ -408,21 +375,6 @@ function Search({
isCardFeedsLoading);
const shouldShowLoadingMoreItems = !shouldShowLoadingState && searchResults?.search?.isLoading && searchResults?.search?.offset > 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we take shouldShowLoadingState here in Search from useSearchLoading hook?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, no - these serve different purposes. useSearchLoadingState gates whether the Search component should mount at all (page-level skeleton vs component). The internal shouldShowLoadingState in Search/index.tsx controls post-mount behavior (loading bar in TopBar, "load more" skeleton, telemetry warm/cold attribute, etc.) and has additional conditions like hasErrors && searchRequestResponseStatusCode === null that are only relevant once Search is mounted. Merging them would conflate two different concerns.


const loadingSkeletonReasonAttributes = useMemo<SkeletonSpanReasonAttributes>(
() => ({
context: 'Search',
isOffline,
isDataLoaded,
isCardFeedsLoading,
isSearchLoading: !!searchResults?.search?.isLoading,
hasEmptyData: Array.isArray(searchResults?.data) && searchResults?.data.length === 0,
hasErrors,
hasPendingResponse: searchRequestResponseStatusCode === null,
shouldUseLiveData,
}),
[isOffline, isDataLoaded, isCardFeedsLoading, searchResults?.search?.isLoading, searchResults?.data, hasErrors, searchRequestResponseStatusCode, shouldUseLiveData],
);

const loadMoreSkeletonReasonAttributes = useMemo<SkeletonSpanReasonAttributes>(
() => ({
context: 'Search.ListFooter',
Expand Down Expand Up @@ -1249,11 +1201,6 @@ function Search({
spanExistedOnMount.current = false;
}, []);

const onLayoutSkeleton = useCallback(() => {
hasHadFirstLayout.current = true;
endSpanWithAttributes(CONST.TELEMETRY.SPAN_NAVIGATE_TO_REPORTS, {[CONST.TELEMETRY.ATTRIBUTE_IS_WARM]: false});
}, []);

const onLayoutChart = useCallback(() => {
hasHadFirstLayout.current = true;
endSpanWithAttributes(CONST.TELEMETRY.SPAN_NAVIGATE_TO_REPORTS, {[CONST.TELEMETRY.ATTRIBUTE_IS_WARM]: true});
Expand All @@ -1274,23 +1221,6 @@ function Search({
}, [shouldShowLoadingState]),
);

if (shouldShowLoadingState) {
return (
<Animated.View
entering={FadeIn.duration(CONST.SEARCH.ANIMATION.FADE_DURATION)}
exiting={FadeOut.duration(CONST.SEARCH.ANIMATION.FADE_DURATION)}
style={[styles.flex1]}
onLayout={onLayoutSkeleton}
>
<SearchRowSkeleton
shouldAnimate
containerStyle={shouldUseNarrowLayout ? styles.searchListContentContainerStyles : styles.mt3}
reasonAttributes={loadingSkeletonReasonAttributes}
/>
</Animated.View>
);
}

if (searchResults === undefined) {
Log.alert('[Search] Undefined search type');
cancelNavigationSpans();
Expand Down
32 changes: 32 additions & 0 deletions src/hooks/useSearchLoadingState.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
import {useSearchStateContext} from '@components/Search/SearchContext';
import type {SearchQueryJSON} from '@components/Search/types';
import {getValidGroupBy, isSearchDataLoaded} from '@libs/SearchUIUtils';
import CONST from '@src/CONST';
import ONYXKEYS from '@src/ONYXKEYS';
import useNetwork from './useNetwork';
import useOnyx from './useOnyx';

/**
* Computes whether the search page should show a loading skeleton
*/
function useSearchLoadingState(queryJSON: SearchQueryJSON | undefined): boolean {
const {isOffline} = useNetwork();
const {shouldUseLiveData, currentSearchResults} = useSearchStateContext();
const [, cardFeedsResult] = useOnyx(ONYXKEYS.COLLECTION.SHARED_NVP_PRIVATE_DOMAIN_MEMBER);

if (shouldUseLiveData || isOffline || !queryJSON) {
return false;
}

const isDataLoaded = isSearchDataLoaded(currentSearchResults, queryJSON);
const isLoadingWithNoData = !!currentSearchResults?.search?.isLoading && Array.isArray(currentSearchResults?.data) && currentSearchResults.data.length === 0;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Base skeleton state on rendered results during sorting

This hook computes loading from currentSearchResults, but during a sort the page intentionally keeps rendering lastNonEmptySearchResults to avoid a full-page flash (see SearchPage's isSorting fallback). After the hash changes, currentSearchResults points to the new empty snapshot, so !isDataLoaded becomes true and the page swaps the existing table for the full skeleton until the response returns, regressing the “no skeleton flash on sort” behavior.

Useful? React with 👍 / 👎.


const validGroupBy = getValidGroupBy(queryJSON.groupBy);
const isCardFeedsLoading = validGroupBy === CONST.SEARCH.GROUP_BY.CARD && cardFeedsResult?.status === 'loading';

const hasNoData = currentSearchResults?.data === undefined;

return (!isDataLoaded && hasNoData) || isLoadingWithNoData || isCardFeedsLoading;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. BTW adding && hasNoData to !isDataLoaded will make !isDataLoaded logic unnecessary because the inequality between searchResult.search.type and queryJson.type (etc) inside isSearchDataLoaded will now have no effect in the logic.
  2. Replacing searchResults with currentSearchResults to calculate isDataLoaded has now made the loading skeleton to be shown when we changing sort.
2026-03-06.22-25-38.mp4

I also think that I see more delay before the loading skeleton is shown compared to staging when changing search presets.

2026-03-06.23-05-38.mp4

Copy link
Contributor Author

@szymonzalarski98 szymonzalarski98 Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both addressed:

  1. You're right that !isDataLoaded is technically redundant when combined with hasNoData - if data === undefined, isDataLoaded is always false. We kept it for readability but the effective condition is just hasNoData. The hook now returns !isDataLoaded && hasNoData.
  2. The sorting regression is fixed - the hook now accepts searchResults from the caller prop (which includes the lastNonEmptySearchResults sorting fallback from SearchPage) instead of reading currentSearchResults from context. During sorting, the fallback keeps data defined, so the skeleton stays hidden.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding (1) I think you need to recheck. Because what I commented about was (!isDataLoaded && hasNoData) but u instead removed || isLoadingWithNoData || isCardFeedsLoading; 😄

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

waiting for your response here @szymonzalarski98

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, I misread your comment initially. Fixed - removed the redundant !isDataLoaded check. The hook now returns just hasNoData (searchResults?.data === undefined), since when data is undefined, isDataLoaded is always false anyway.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@szymonzalarski98 why can't we stick with previous logic to avoid regression unless it an intentional change like we did for error case?
I still see differences with this

// There's a race condition in Onyx which makes it return data from the previous Search, so in addition to checking that the data is loaded
// we also need to check that the searchResults matches the type and status of the current search
const isDataLoaded = shouldUseLiveData || isSearchDataLoaded(searchResults, queryJSON);
const hasErrors = Object.keys(searchResults?.errors ?? {}).length > 0 && !isOffline;
// For to-do searches, we never show loading state since the data is always available locally from Onyx
const shouldShowLoadingState =
!shouldUseLiveData &&
!isOffline &&
(!isDataLoaded ||
(!!searchResults?.search.isLoading && Array.isArray(searchResults?.data) && searchResults?.data.length === 0) ||
(hasErrors && searchRequestResponseStatusCode === null) ||
isCardFeedsLoading);

the way we calculated isDataLoaded didn't only check for existence to avoid race condition as depicted in the comment and also there were two cases you omitted here


            (!!searchResults?.search.isLoading && Array.isArray(searchResults?.data) && searchResults?.data.length === 0) ||

and

isCardFeedsLoading


export default useSearchLoadingState;
69 changes: 69 additions & 0 deletions src/hooks/useSearchPageSetup.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
import {useCallback, useEffect} from 'react';
import {useFocusEffect} from '@react-navigation/native';
import {useSearchActionsContext, useSearchStateContext} from '@components/Search/SearchContext';
import type {SearchQueryJSON} from '@components/Search/types';
import {openSearch, search} from '@libs/actions/Search';
import {getSuggestedSearches, isSearchDataLoaded} from '@libs/SearchUIUtils';
import useCardFeedsForDisplay from './useCardFeedsForDisplay';
import useCurrentUserPersonalDetails from './useCurrentUserPersonalDetails';
import useNetwork from './useNetwork';
import usePrevious from './usePrevious';

/**
* Handles page-level setup for Search that must happen before the Search component mounts:
* - Sets the context hash/key so the Onyx subscription points to the correct snapshot
* - Fires the search() API call so data starts loading alongside the skeleton
* - Fires openSearch() to load bank account data
* - Re-fires openSearch() when coming back online
*/
function useSearchPageSetup(queryJSON: SearchQueryJSON | undefined) {
const {isOffline} = useNetwork();
const prevIsOffline = usePrevious(isOffline);
const {setCurrentSearchHashAndKey, setCurrentSearchQueryJSON, clearSelectedTransactions} = useSearchActionsContext();
const {shouldUseLiveData, currentSearchResults} = useSearchStateContext();
const {accountID} = useCurrentUserPersonalDetails();
const {defaultCardFeed} = useCardFeedsForDisplay();

const suggestedSearches = getSuggestedSearches(accountID, defaultCardFeed?.id);
const hash = queryJSON?.hash;
const recentSearchHash = queryJSON?.recentSearchHash;
const searchKey = recentSearchHash !== undefined ? Object.values(suggestedSearches).find((s) => s.recentSearchHash === recentSearchHash)?.key : undefined;

// useCallback is required here because useFocusEffect (React Navigation external API) compares callback references.
// React Compiler cannot optimize this — it doesn't know useFocusEffect's internal semantics.
const syncContextWithRoute = useCallback(() => {
if (hash === undefined || recentSearchHash === undefined || !queryJSON) {
return;
}
clearSelectedTransactions(hash);
setCurrentSearchHashAndKey(hash, recentSearchHash, searchKey);
setCurrentSearchQueryJSON(queryJSON);
}, [hash, recentSearchHash, searchKey, queryJSON, clearSelectedTransactions, setCurrentSearchHashAndKey, setCurrentSearchQueryJSON]);

useFocusEffect(syncContextWithRoute);

useEffect(syncContextWithRoute, [syncContextWithRoute]);

useEffect(() => {
if (!queryJSON || hash === undefined || shouldUseLiveData || isOffline) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❌ PERF-12 (docs)

useFocusEffect subscribes to navigation focus/blur events internally. When the callback passed to it changes on every render (because syncContextWithRoute is not wrapped in useCallback), useFocusEffect tears down and re-creates its internal useEffect subscription on every render. This causes repeated subscribe/unsubscribe cycles to the navigation event listeners, which is wasteful and could lead to subtle timing bugs.

Wrap syncContextWithRoute in useCallback with the appropriate dependencies, matching how the original code used useCallback for clearTransactionsAndSetHashAndKey:

const syncContextWithRoute = useCallback(() => {
    if (hash === undefined || recentSearchHash === undefined || !queryJSON) {
        return;
    }
    clearSelectedTransactions(hash);
    setCurrentSearchHashAndKey(hash, recentSearchHash, searchKey);
    setCurrentSearchQueryJSON(queryJSON);
}, [hash, recentSearchHash, searchKey, clearSelectedTransactions, setCurrentSearchHashAndKey, setCurrentSearchQueryJSON, queryJSON]);

Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.

return;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❌ PERF-9 (docs)

useEffect(syncContextWithRoute, [syncContextWithRoute]) will fire on every render because syncContextWithRoute is an inline function that creates a new reference each render. This causes clearSelectedTransactions, setCurrentSearchHashAndKey, and setCurrentSearchQueryJSON to be called on every single render of the host component, which is excessive and could cause unnecessary state updates and re-renders throughout the search context consumers.

This is directly related to the missing useCallback on syncContextWithRoute. Once that function is memoized with useCallback, this effect will only fire when the actual dependencies change. However, also consider whether this separate useEffect is even necessary alongside the useFocusEffect -- the original code in Search/index.tsx had a similar pair but with an eslint-disable comment explaining it was for the mount case when the screen is not focused (e.g., page reload with RHP open).


Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.

if (isSearchDataLoaded(currentSearchResults, queryJSON) || currentSearchResults?.search?.isLoading) {
return;
}
search({queryJSON, searchKey, offset: 0, shouldCalculateTotals: false, isLoading: false});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is this search coming from? I thought we were not adding a new logic here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not new logic - it's the same search() call that was previously inside Search/index.tsx (the mount effect that fires the initial API call). We moved it to page level so that the API request starts in parallel with the skeleton rendering, rather than waiting for Search to mount (which requires 14+ useOnyx hooks to initialize first). The guards are identical: skip if data is already loaded or if a request is already in flight (currentSearchResults?.search?.isLoading).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is shouldCalculateTotals passed as false? Doesn't it depend on the searchHash like here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. The page-level search() call is a preliminary request to get data loading before Search mounts. We pass shouldCalculateTotals: false because the initial request focuses on getting data as fast as possible. Search's internal handleSearch effect has retry logic (via shouldRetrySearchWithTotalsOrGroupedRef at line 557-578) - after its first mount, if totals are needed, it re-queries with shouldCalculateTotals: true. This avoids an unnecessary initial totals calculation that would be recalculated anyway. However, if you'd prefer the correct value from the start, I can wire in useSearchShouldCalculateTotals - let me know.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know let @luacmartins defer on this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure why we'd retrigger a Search API command here. Let's just remove this logic and pass the correct value to being with.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The page-level search() call is actually required in this architecture - it's not a duplicate. Here's why:

In baseline, the skeleton was rendered inside Search component. Search always mounted, fired the API call via handleSearch, and showed the skeleton internally while waiting for data.

In this PR, the skeleton is rendered outside Search at the page level. When data === undefined, the page shows the skeleton instead of mounting Search. This means Search's handleSearch effect never fires, the API call never happens, and we get infinite loading.

The page-level search() call breaks this deadlock - it fires the API request while the skeleton is visible, so that when data arrives, Search can mount.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree it's needed, but why do we trigger it with shouldCalculateTotals: false if we know that it should be true given the search query? We should just pass the correct value to begin with

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done - wired in useSearchShouldCalculateTotals(currentSearchKey, hash, true) so the page-level search() call passes the correct value from the start.

}, [hash, searchKey, isOffline, shouldUseLiveData, currentSearchResults, queryJSON]);

useEffect(() => {
openSearch({includePartiallySetupBankAccounts: true});
}, []);

useEffect(() => {
if (!prevIsOffline || isOffline) {
return;
}
openSearch({includePartiallySetupBankAccounts: true});
}, [isOffline, prevIsOffline]);
}

export default useSearchPageSetup;
5 changes: 5 additions & 0 deletions src/libs/SearchUIUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3823,6 +3823,10 @@ function isSearchDataLoaded(searchResults: SearchResults | undefined, queryJSON:
return isDataLoaded;
}

function getValidGroupBy(groupBy: string | undefined): ValueOf<typeof CONST.SEARCH.GROUP_BY> | undefined {
return groupBy && Object.values(CONST.SEARCH.GROUP_BY).includes(groupBy as ValueOf<typeof CONST.SEARCH.GROUP_BY>) ? (groupBy as ValueOf<typeof CONST.SEARCH.GROUP_BY>) : undefined;
}

function getStatusOptions(translate: LocalizedTranslate, type: SearchDataTypes) {
switch (type) {
case CONST.SEARCH.DATA_TYPES.INVOICE:
Expand Down Expand Up @@ -4639,6 +4643,7 @@ export {
shouldShowEmptyState,
compareValues,
isSearchDataLoaded,
getValidGroupBy,
getStatusOptions,
getTypeOptions,
getGroupByOptions,
Expand Down
2 changes: 2 additions & 0 deletions src/pages/Search/SearchPage.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import usePrevious from '@hooks/usePrevious';
import useReceiptScanDrop from '@hooks/useReceiptScanDrop';
import useResponsiveLayout from '@hooks/useResponsiveLayout';
import useSearchFilterSync from '@hooks/useSearchFilterSync';
import useSearchPageSetup from '@hooks/useSearchPageSetup';
import useSearchShouldCalculateTotals from '@hooks/useSearchShouldCalculateTotals';
import useTheme from '@hooks/useTheme';
import useThemeStyles from '@hooks/useThemeStyles';
Expand Down Expand Up @@ -49,6 +50,7 @@ function SearchPage({route}: SearchPageProps) {
const lastNonEmptySearchResults = useRef<SearchResults | undefined>(undefined);

useConfirmReadyToOpenApp();
useSearchPageSetup(queryJSON);

useEffect(() => {
if (!currentSearchResults?.search?.type) {
Expand Down
27 changes: 17 additions & 10 deletions src/pages/Search/SearchPageNarrow.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import ScreenWrapper from '@components/ScreenWrapper';
import {ScrollOffsetContext} from '@components/ScrollOffsetContextProvider';
import Search from '@components/Search';
import {useSearchActionsContext} from '@components/Search/SearchContext';
import SearchLoadingSkeleton from '@components/Search/SearchLoadingSkeleton';
import SearchPageFooter from '@components/Search/SearchPageFooter';
import SearchFiltersBar from '@components/Search/SearchPageHeader/SearchFiltersBar';
import SearchPageHeader from '@components/Search/SearchPageHeader/SearchPageHeader';
Expand All @@ -21,6 +22,7 @@ import useAndroidBackButtonHandler from '@hooks/useAndroidBackButtonHandler';
import useLoadingBarVisibility from '@hooks/useLoadingBarVisibility';
import useLocalize from '@hooks/useLocalize';
import useNetwork from '@hooks/useNetwork';
import useSearchLoadingState from '@hooks/useSearchLoadingState';
import useResponsiveLayout from '@hooks/useResponsiveLayout';
import useScrollEventEmitter from '@hooks/useScrollEventEmitter';
import useStyleUtils from '@hooks/useStyleUtils';
Expand Down Expand Up @@ -55,6 +57,7 @@ type SearchPageNarrowProps = {
};

function SearchPageNarrow({queryJSON, searchResults, isMobileSelectionModeEnabled, metadata, footerData, shouldShowFooter}: SearchPageNarrowProps) {
const shouldShowLoadingSkeleton = useSearchLoadingState(queryJSON);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will using shouldShowLoadingSkeleton in SearchPage and passing it as a prop for both improve the timing even more?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The timing difference would be negligible (one React component level ≈ <1ms). The current approach of calling the hook inside Wide/Narrow was chosen to avoid prop drilling, per the earlier review guidance from @adhorodyski to use named hooks rather than passing computed values as props. Both Wide and Narrow already receive searchResults as a prop, so the hook has everything it needs without additional plumbing.

const {translate} = useLocalize();
const {shouldUseNarrowLayout} = useResponsiveLayout();
const {windowHeight} = useWindowDimensions();
Expand Down Expand Up @@ -238,16 +241,20 @@ function SearchPageNarrow({queryJSON, searchResults, isMobileSelectionModeEnable
)}
{!searchRouterListVisible && (
<View style={[styles.flex1]}>
<Search
searchResults={searchResults}
key={queryJSON.hash}
queryJSON={queryJSON}
onSearchListScroll={scrollHandler}
contentContainerStyle={!isMobileSelectionModeEnabled ? styles.searchListContentContainerStyles : undefined}
handleSearch={handleSearchAction}
isMobileSelectionModeEnabled={isMobileSelectionModeEnabled}
searchRequestResponseStatusCode={searchRequestResponseStatusCode}
/>
{shouldShowLoadingSkeleton ? (
<SearchLoadingSkeleton containerStyle={styles.searchListContentContainerStyles} />
) : (
<Search
searchResults={searchResults}
key={queryJSON.hash}
queryJSON={queryJSON}
onSearchListScroll={scrollHandler}
contentContainerStyle={!isMobileSelectionModeEnabled ? styles.searchListContentContainerStyles : undefined}
handleSearch={handleSearchAction}
isMobileSelectionModeEnabled={isMobileSelectionModeEnabled}
searchRequestResponseStatusCode={searchRequestResponseStatusCode}
/>
)}
</View>
)}
{shouldShowFooter && !searchRouterListVisible && (
Expand Down
Loading
Loading