Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
22 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
377eea4
PR fixes
szymonzalarski98 Mar 16, 2026
cea9a17
PR fix
szymonzalarski98 Mar 17, 2026
c853265
PR fix
szymonzalarski98 Mar 18, 2026
73a6a1b
Merge remote-tracking branch 'origin/main' into callstack-internal/sz…
szymonzalarski98 Mar 18, 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
36 changes: 36 additions & 0 deletions src/components/Search/SearchLoadingSkeleton.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
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 type {SkeletonSpanReasonAttributes} from '@libs/telemetry/useSkeletonSpan';
import CONST from '@src/CONST';

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

function SearchLoadingSkeleton({containerStyle, reasonAttributes}: 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}
reasonAttributes={reasonAttributes}
/>
</Animated.View>
);
}

export default SearchLoadingSkeleton;
57 changes: 4 additions & 53 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 @@ -36,7 +36,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 @@ -50,6 +50,7 @@ import {
getListItem,
getSections,
getSortedSections,
getValidGroupBy,
getWideAmountIndicators,
isGroupedItemArray,
isReportActionListItemType,
Expand Down Expand Up @@ -228,7 +229,6 @@ function Search({
const navigation = useNavigation<PlatformStackNavigationProp<SearchFullscreenNavigatorParamList>>();
const isFocused = useIsFocused();
const {markReportIDAsExpense} = useWideRHPActions();

const {
currentSearchHash,
currentSearchKey,
Expand All @@ -240,7 +240,6 @@ function Search({
shouldUseLiveData,
suggestedSearches,
} = useSearchStateContext();

const {setSelectedTransactions, clearSelectedTransactions, setShouldShowFiltersBarLoading, setShouldShowSelectAllMatchingItems, selectAllMatchingItems, setShouldResetSearchQuery} =
useSearchActionsContext();
const [offset, setOffset] = useState(0);
Expand Down Expand Up @@ -305,7 +304,7 @@ function Search({
}
}, [onDEWModalOpen, showConfirmModal, translate]);

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 @@ -353,17 +352,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 @@ -448,21 +436,6 @@ function Search({

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 @@ -1325,11 +1298,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 @@ -1354,23 +1322,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
35 changes: 35 additions & 0 deletions src/hooks/useSearchLoadingState.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
import {useSearchStateContext} from '@components/Search/SearchContext';
import type {SearchQueryJSON} from '@components/Search/types';
import {getValidGroupBy} from '@libs/SearchUIUtils';
import CONST from '@src/CONST';
import ONYXKEYS from '@src/ONYXKEYS';
import type {SearchResults} from '@src/types/onyx';
import useNetwork from './useNetwork';
import useOnyx from './useOnyx';

/**
* Computes whether the search page should show a loading skeleton.
* Accepts searchResults from the caller (which may include a sorting fallback)
* rather than reading raw context data, so that sorting doesn't trigger a skeleton flash.
*/
function useSearchLoadingState(queryJSON: SearchQueryJSON | undefined, searchResults: SearchResults | undefined): boolean {
const {isOffline} = useNetwork();
const {shouldUseLiveData} = useSearchStateContext();
const [, cardFeedsResult] = useOnyx(ONYXKEYS.COLLECTION.SHARED_NVP_PRIVATE_DOMAIN_MEMBER);

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

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

// Show page-level skeleton when no data has ever arrived for this query,
// or when card feeds are still loading for card-grouped searches.
// Once data arrives (even empty []), Search mounts and handles its own
// loading/empty states internally via shouldShowLoadingState.
return hasNoData || isCardFeedsLoading;
}

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

/**
* Handles page-level setup for Search that must happen before the Search component mounts:
* - Clears selected transactions when the query changes
* - 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 {clearSelectedTransactions} = useSearchActionsContext();
const {shouldUseLiveData, currentSearchResults, currentSearchKey} = useSearchStateContext();

const hash = queryJSON?.hash;
const shouldCalculateTotals = useSearchShouldCalculateTotals(currentSearchKey, hash, true);

// Clear selected transactions when navigating to a different search query
const clearOnHashChange = useCallback(() => {
if (hash === undefined) {
return;
}
clearSelectedTransactions(hash);
}, [hash, clearSelectedTransactions]);

useFocusEffect(clearOnHashChange);

// useEffect supplements useFocusEffect: it handles both the initial mount
// and cases where route params change without a navigation event (e.g. sorting).
useEffect(clearOnHashChange, [clearOnHashChange]);

// Fire search() when the query changes (hash). This runs at the page level so the
// API request starts in parallel with the skeleton, before Search mounts its 14+ useOnyx hooks.
// currentSearchResults is intentionally read but not in deps — search should fire once per
// query change, not re-trigger on every data update from Onyx.
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: currentSearchKey, offset: 0, shouldCalculateTotals, isLoading: false});
}, [hash, isOffline, shouldUseLiveData, queryJSON]);

Check warning on line 53 in src/hooks/useSearchPageSetup.ts

View workflow job for this annotation

GitHub Actions / ESLint check

React Hook useEffect has missing dependencies: 'currentSearchKey', 'currentSearchResults', and 'shouldCalculateTotals'. Either include them or remove the dependency array

Check warning on line 53 in src/hooks/useSearchPageSetup.ts

View workflow job for this annotation

GitHub Actions / Changed files ESLint check

React Hook useEffect has missing dependencies: 'currentSearchKey', 'currentSearchResults', and 'shouldCalculateTotals'. Either include them or remove the dependency array

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 @@ -3939,6 +3939,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 @@ -4779,6 +4783,7 @@ export {
shouldShowEmptyState,
compareValues,
isSearchDataLoaded,
getValidGroupBy,
getStatusOptions,
getTypeOptions,
getGroupByOptions,
Expand Down
61 changes: 38 additions & 23 deletions src/libs/actions/Search.ts
Original file line number Diff line number Diff line change
Expand Up @@ -435,6 +435,11 @@ function openSearchPage({includePartiallySetupBankAccounts}: OpenSearchPageParam
API.read(READ_COMMANDS.OPEN_SEARCH_PAGE, {includePartiallySetupBankAccounts});
}

// Tracks in-flight search requests by hash+offset to prevent duplicate API calls
// when both page-level (useSearchPageSetup) and Search-internal (handleSearch) effects
// fire for the same query. Cleared when the request completes.
const inFlightSearchRequests = new Set<string>();

let shouldPreventSearchAPI = false;
function handlePreventSearchAPI(hash: number | undefined) {
if (typeof hash === 'undefined') {
Expand Down Expand Up @@ -478,6 +483,12 @@ function search({
return;
}

const dedupeKey = `${queryJSON.hash}_${offset ?? 0}`;
if (inFlightSearchRequests.has(dedupeKey)) {
return;
}
inFlightSearchRequests.add(dedupeKey);

const {optimisticData, finallyData, failureData} = getOnyxLoadingData(queryJSON.hash, queryJSON, offset, isOffline, true, shouldCalculateTotals);
const {flatFilters, limit, ...queryJSONWithoutFlatFilters} = queryJSON;
const query = {
Expand All @@ -499,36 +510,40 @@ function search({

return waitForWrites(READ_COMMANDS.SEARCH).then(() => {
// eslint-disable-next-line rulesdir/no-api-side-effects-method
return API.makeRequestWithSideEffects(READ_COMMANDS.SEARCH, {hash: queryJSON.hash, jsonQuery}, {optimisticData, finallyData, failureData}).then((result) => {
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, ''));
if (response?.search?.offset) {
// Indicates that search results are extended from the Report view (with navigation between reports),
// using previous results to enable correct counter behavior.
if (prevReportsLength) {
return API.makeRequestWithSideEffects(READ_COMMANDS.SEARCH, {hash: queryJSON.hash, jsonQuery}, {optimisticData, finallyData, failureData})
.then((result) => {
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, ''));
if (response?.search?.offset) {
// Indicates that search results are extended from the Report view (with navigation between reports),
// using previous results to enable correct counter behavior.
if (prevReportsLength) {
saveLastSearchParams({
queryJSON,
offset,
hasMoreResults: !!response?.search?.hasMoreResults,
previousLengthOfResults: prevReportsLength,
allowPostSearchRecount: false,
});
}
} else {
// Applies to all searches from the Search View
saveLastSearchParams({
queryJSON,
offset,
hasMoreResults: !!response?.search?.hasMoreResults,
previousLengthOfResults: prevReportsLength,
allowPostSearchRecount: false,
previousLengthOfResults: reports.length,
allowPostSearchRecount: true,
});
}
} else {
// Applies to all searches from the Search View
saveLastSearchParams({
queryJSON,
offset,
hasMoreResults: !!response?.search?.hasMoreResults,
previousLengthOfResults: reports.length,
allowPostSearchRecount: true,
});
}

return result?.jsonCode;
});
return result?.jsonCode;
})
.finally(() => {
inFlightSearchRequests.delete(dedupeKey);
});
});
}

Expand Down
Loading
Loading