Refactor search functionality to improve loading state handling#83917
Conversation
| @@ -141,6 +160,10 @@ function SearchPageNarrow({queryJSON, searchResults, isMobileSelectionModeEnable | |||
| return () => removeRouteKey(route.key); | |||
| }, [addRouteKey, removeRouteKey, route.key, searchRouterListVisible]); | |||
|
|
|||
| const onLayoutSkeleton = () => { | |||
| endSpan(CONST.TELEMETRY.SPAN_ON_LAYOUT_SKELETON_REPORTS); | |||
There was a problem hiding this comment.
We changed this span. We now track a single span. We should also remove it from the search page, since we
App/src/components/Search/index.tsx
Lines 1187 to 1190 in eed56d0
| useEffect(() => { | ||
| openSearch({includePartiallySetupBankAccounts: true}); | ||
| }, []); | ||
|
|
There was a problem hiding this comment.
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
src/pages/Search/SearchPage.tsx
Outdated
|
|
||
| useConfirmReadyToOpenApp(); | ||
|
|
||
| // Set the search context hash at the page level so the Onyx subscription | ||
| // in SearchContext points to the correct snapshot key even before Search mounts. | ||
| useEffect(() => { |
There was a problem hiding this comment.
these should go into a named hook(s), do not inline effects
src/pages/Search/SearchPage.tsx
Outdated
| confirmPayment={stableOnBulkPaySelected} | ||
| latestBankItems={latestBankItems} | ||
| shouldShowFooter={shouldShowFooter} | ||
| shouldShowLoadingState={shouldShowLoadingState} |
There was a problem hiding this comment.
this should be flagged by the AI reviewer, but for the sake of it - let's compose this skeleton in. there's no need to prop drill anything here.
src/pages/Search/SearchPage.tsx
Outdated
| isMobileSelectionModeEnabled={isMobileSelectionModeEnabled} | ||
| shouldShowLoadingState={shouldShowLoadingState} |
There was a problem hiding this comment.
same here, do not drill this property down since it's just a UI lever. let's think how we can group these components better inside/alongside each other so the loading state can be easily declared (which you proved calculating its state there).
src/pages/Search/SearchPageWide.tsx
Outdated
| PDFValidationComponent, | ||
| ErrorModal, | ||
| shouldShowFooter, | ||
| shouldShowLoadingState, |
There was a problem hiding this comment.
commented across the PR on this prop drilling violation of the review rules (see .claude/skills/coding-standards)
szymonzalarski98
force-pushed
the
callstack-internal/szymonzalarski/search/move-skeleton-to-top-level-search-page
branch
from
f076b0e to
5af0782
Compare
…ymonzalarski/search/move-skeleton-to-top-level-search-page # Conflicts: # src/components/Search/index.tsx # src/pages/Search/SearchPage.tsx
melvin-bot
bot
requested review from
JmillsExpensify
and removed request for
a team
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f49723dbb9
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
src/hooks/useSearchLoadingState.ts
Outdated
| const isDataLoaded = isSearchDataLoaded(currentSearchResults, queryJSON); | ||
| const isLoadingWithNoData = !!currentSearchResults?.search?.isLoading && Array.isArray(currentSearchResults?.data) && currentSearchResults.data.length === 0; |
There was a problem hiding this comment.
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 👍 / 👎.
src/hooks/useSearchPageSetup.ts
Outdated
| import useNetwork from './useNetwork'; | ||
| import usePrevious from './usePrevious'; | ||
|
|
||
| let didOpenSearch = false; |
There was a problem hiding this comment.
Using a module-level didOpenSearch flag means openSearch() runs only once per JS runtime, not once per Search page mount. After logout/login or account switching without restarting the app, this effect will be skipped and search bootstrap data (including bank-account payload requested by openSearch) will not be refreshed unless connectivity toggles, which can leave search-dependent data stale or missing for the new session.
Useful? React with 👍 / 👎.
|
@luacmartins @adhorodyski Hey, I've updated the PR |
|
Does this need c+ review @luacmartins ? |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| import usePrevious from './usePrevious'; | ||
|
|
||
| let didOpenSearch = false; | ||
|
|
There was a problem hiding this comment.
❌ PERF-12 (docs)
The module-level let didOpenSearch = false flag is never reset. Once set to true, it persists for the lifetime of the JavaScript context. This means if a user logs out and logs back in, openSearch() will never fire again because the flag remains true. Unlike the original code in Search/index.tsx which called openSearch() in a straightforward useEffect([], []) on every mount of the Search component, this module-level guard permanently prevents re-execution.
Consider resetting the flag when the hook's host component unmounts, or use a useRef instead of a module-level variable so the guard is scoped to the component instance:
const didOpenSearch = useRef(false);
useEffect(() => {
if (didOpenSearch.current) {
return;
}
didOpenSearch.current = true;
openSearch({includePartiallySetupBankAccounts: true});
}, []);Alternatively, if a module-level flag is intentional, add cleanup to reset it on unmount:
useEffect(() => {
return () => { didOpenSearch = false; };
}, []);Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| useEffect(syncContextWithRoute, [syncContextWithRoute]); | ||
|
|
||
| useEffect(() => { | ||
| if (!queryJSON || hash === undefined || shouldUseLiveData || isOffline) { |
There was a problem hiding this comment.
❌ 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.
| useEffect(() => { | ||
| if (!queryJSON || hash === undefined || shouldUseLiveData || isOffline) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
❌ 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.
|
@FitseTLT yes! Please prioritize this review when you're online |
JmillsExpensify
left a comment
JmillsExpensify
left a comment
There was a problem hiding this comment.
No product review required.
on it |
|
Waiting for your responses on some comments @luacmartins @szymonzalarski98 |
|
Replied |
|
Hey, PR is updated and I've replied to comments |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppbranch.mp4Android: mWeb Chrome2026-03-18.15-55-40.mp4iOS: HybridAppiOS: mWeb Safari2026-03-18.19-09-23.mp4MacOS: Chrome / Safari2026-03-18.18-50-53.mp4 |
|
Left with comments @szymonzalarski98 |
Hey, thank you. I've adressed both comments and answered |
|
@FitseTLT it seems like all pending comments are addressed. Let me know if we missed anything! |
|
what about #83917 (comment) @szymonzalarski98 ? |
|
@FitseTLT PR is updated, comments adressed |
|
Conflicts @szymonzalarski98 |
|
REG: I am seeing a double Search call when I change any filter which doesn't occur on staging 2026-03-17.20-35-33.mp4 |
…ymonzalarski/search/move-skeleton-to-top-level-search-page
|
@FitseTLT thank you for the review, could you please check it once again, I've created a PR and the issue is fixed |
|
@FitseTLT thank you! @luacmartins could you please have a look and we are ready to merge :D |
|
🚧 @luacmartins has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/luacmartins in version: 9.3.41-0 🚀
Bundle Size Analysis (Sentry): |
|
🚀 Deployed to production by https://github.com/cristipaval in version: 9.3.41-4 🚀
|
6 participants
Speeds up skeleton rendering on the Reports page by lifting the search context setup (setCurrentSearchHashAndKey, setCurrentSearchQueryJSON) and openSearch() from the Search component to a new useSearchPageSetup hook called at the SearchPage level.
Previously, when navigating to Reports, the Onyx snapshot subscription hash was set inside Search via useFocusEffect, meaning the context had to wait for Search to mount and its effects to run before the correct snapshot was available. This added an extra render cycle before shouldShowLoadingState could be computed correctly and the skeleton could appear.
Now, useSearchPageSetup sets the context hash at the SearchPage level before Search mounts, so when Search initializes its hooks, the Onyx subscription already points to the correct snapshot. This eliminates the extra render cycle and allows the skeleton to appear immediately on first render.
Key changes:
New useSearchPageSetup hook (src/hooks/useSearchPageSetup.ts): Extracted from Search — handles setCurrentSearchHashAndKey, setCurrentSearchQueryJSON, clearSelectedTransactions (via useFocusEffect + mount effect), openSearch() on mount, and openSearch() on reconnect.
SearchPage.tsx: Added useSearchPageSetup(queryJSON) call.
Search/index.tsx: Removed clearTransactionsAndSetHashAndKey + its useFocusEffect + mount effect, removed openSearch() mount and reconnect effects (all moved to the hook). Skeleton rendering and all other logic unchanged.
SearchPageWide.tsx / SearchPageNarrow.tsx: No changes — Search still renders unconditionally and handles skeleton internally.
Fixed Issues
$ #83342
PROPOSAL:
Proposal: Lift Search Skeleton and API Initialization to SearchPage
Background: The Reports page is organized as a hierarchy starting with
SearchPage, which acts as a router to eitherSearchPageWideorSearchPageNarrow. These sub-pages mount theSearchcomponent, which serves as the primary data-fetching and display layer. TheSearchcomponent manages approximately 14 Onyx subscriptions, includingCOLLECTION.TRANSACTIONandCOLLECTION.POLICY, to populate the reports list.Problem: When a user navigates to the Reports page with no cached Onyx data, if the
Searchcomponent must fully initialize its ~14 Onyx subscriptions and associated hooks before the skeleton can render, then there is an observable 374ms window where the screen is blank, causing users to perceive the application as frozen.Solution: We will move the
SearchRowSkeletonand the initialopenSearch()API call to theSearchPagelevel. This decoupling ensures that the user receives instant visual feedback (the skeleton) the moment the route is matched, rather than waiting for the heavy data-subscription layer to mount and initialize.Measurable Impact:
Tests
Offline tests
QA Steps
Same as Tests above — the change is purely in rendering timing, no new user-facing features. Focus on verifying there are no regressions in:
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari