Refactor search functionality to improve loading state handling #83917
There are no files selected for viewing
| 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; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,33 @@ | ||
| import {useSearchStateContext} from '@components/Search/SearchContext'; | ||
| import type {SearchQueryJSON} from '@components/Search/types'; | ||
| import {isSearchDataLoaded} from '@libs/SearchUIUtils'; | ||
| import type {SearchResults} from '@src/types/onyx'; | ||
| import useNetwork from './useNetwork'; | ||
|
|
||
| /** | ||
| * 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. | ||
| * | ||
| * Note: This hook intentionally does NOT check isCardFeedsLoading. Card feed loading is handled | ||
| * internally by the Search component's shouldShowLoadingState — blocking Search from mounting | ||
| * would prevent the API call from firing and create a deadlock. | ||
| */ | ||
| function useSearchLoadingState(queryJSON: SearchQueryJSON | undefined, searchResults: SearchResults | undefined): boolean { | ||
| const {isOffline} = useNetwork(); | ||
| const {shouldUseLiveData} = useSearchStateContext(); | ||
|
|
||
| if (shouldUseLiveData || isOffline || !queryJSON) { | ||
| return false; | ||
| } | ||
|
|
||
| const isDataLoaded = isSearchDataLoaded(searchResults, queryJSON); | ||
| const hasNoData = searchResults?.data === undefined; | ||
|
|
||
| // Show page-level skeleton ONLY when no data has ever arrived for this query. | ||
| // Once data arrives (even empty []), Search mounts and handles its own | ||
| // loading/empty states internally via shouldShowLoadingState. | ||
| return !isDataLoaded && hasNoData; | ||
| } | ||
|
|
||
| export default useSearchLoadingState; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,84 @@ | ||
| import {useFocusEffect} from '@react-navigation/native'; | ||
| import {useCallback, useEffect, useLayoutEffect, useRef} from 'react'; | ||
| 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; | ||
|
|
||
| // Ref to read currentSearchResults in the search effect without adding it to deps. | ||
| // This prevents re-triggering when data arrives — search should fire once per query change. | ||
| const currentSearchResultsRef = useRef(currentSearchResults); | ||
| useLayoutEffect(() => { | ||
| currentSearchResultsRef.current = currentSearchResults; | ||
| }, [currentSearchResults]); | ||
|
|
||
| // 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 supplements useFocusEffect: it handles both the initial mount | ||
| // and cases where route params change without a navigation event (e.g. sorting). | ||
| useEffect(syncContextWithRoute, [syncContextWithRoute]); | ||
szymonzalarski98 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
|
||
| // Fire search() when the query changes (hash/searchKey). This runs at the page level so the | ||
| // API request starts in parallel with the skeleton, before Search mounts its 14+ useOnyx hooks. | ||
| // Uses a ref for currentSearchResults to avoid re-triggering on every data update. | ||
szymonzalarski98 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| // The search() action has an internal guard (isLoading check) that prevents duplicate calls, | ||
| // so if Search's internal handleSearch effect also fires, only one API request will proceed. | ||
| useEffect(() => { | ||
| if (!queryJSON || hash === undefined || shouldUseLiveData || isOffline) { | ||
|
Contributor
There was a problem hiding this comment. ❌ PERF-12 (docs)
Wrap 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; | ||
| } | ||
|
Contributor
There was a problem hiding this comment. ❌ PERF-9 (docs)
This is directly related to the missing Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency. |
||
| const results = currentSearchResultsRef.current; | ||
| if (isSearchDataLoaded(results, queryJSON) || results?.search?.isLoading) { | ||
| return; | ||
| } | ||
| search({queryJSON, searchKey, offset: 0, shouldCalculateTotals: false, isLoading: false}); | ||
|
||
| }, [hash, searchKey, isOffline, shouldUseLiveData, queryJSON]); | ||
|
|
||
| useEffect(() => { | ||
| openSearch({includePartiallySetupBankAccounts: true}); | ||
| }, []); | ||
|
|
||
| useEffect(() => { | ||
| if (!prevIsOffline || isOffline) { | ||
| return; | ||
| } | ||
| openSearch({includePartiallySetupBankAccounts: true}); | ||
| }, [isOffline, prevIsOffline]); | ||
| } | ||
|
|
||
| export default useSearchPageSetup; | ||
There was a problem hiding this comment.
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
App/src/components/Search/index.tsx
Lines 1210 to 1224 in eed56d0