Make Switching to the Search Page Faster#74108
Conversation
…eports_v2 # Conflicts: # src/libs/Navigation/AppNavigator/usePreloadFullScreenNavigators.ts
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| }); | ||
|
|
||
| try { | ||
| debugger; |
There was a problem hiding this comment.
Please remove this debugger statement before merging. Debugger statements should not be committed to the codebase as they can cause the application to pause execution in production.
|
LGTM |
. Thank you for your hard work!
|
🚧 @mountiny has triggered a test Expensify/App build. You can view the workflow run here. |
This comment has been minimized.
This comment has been minimized.
|
I know this is still a draft but for me no Search API call is made |
sumo-slonik
force-pushed
the
feature/kuba-nowakowski/preload_reports_v2
branch
from
428772a to
0401f08
Compare
# Conflicts: # src/libs/Navigation/AppNavigator/usePreloadFullScreenNavigators.ts
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
🚧 @mountiny 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, Desktop, and Web. Happy testing! 🧪🧪
|
| return; | ||
| } | ||
| handleSearchAction({queryJSON, searchKey, offset: 0, shouldCalculateTotals}); | ||
| // eslint-disable-next-line react-compiler/react-compiler |
There was a problem hiding this comment.
I know it's not yet enforced anywhere, but we really shouldn't integrate any new code that disables the compiler, especially so high up in the components' tree. Can you rethink how this can be implemented following the rules of React?
There was a problem hiding this comment.
I agree that we should follow that approach but in this particular case I don’t think it will make much difference because the SearchPage already has a lot of issues with the React compiler even without this useEffect, which means it isn’t being compiled anyway. So adding or removing this useEffect won’t really change that situation.
There was a problem hiding this comment.
After testing, I think I’ll do it the way you suggested and make sure it doesn’t conflict with the compiler.
| // Currently, only the Account and Workspaces tabs are preloaded. The remaining tabs will be supported soon. | ||
| const TABS_TO_PRELOAD = [NAVIGATION_TABS.SETTINGS, NAVIGATION_TABS.WORKSPACES]; | ||
| // Currently, only the Inbox, Workspaces, Account tabs are preloaded. The remaining tabs will be supported soon. | ||
| const TABS_TO_PRELOAD = [NAVIGATION_TABS.HOME, NAVIGATION_TABS.SEARCH, NAVIGATION_TABS.WORKSPACES, NAVIGATION_TABS.SETTINGS]; |
There was a problem hiding this comment.
From the PR, this is the only place where we actually turn on the preloading logic. Given that, why can't it be integrated on it's own as a separate change?
There was a problem hiding this comment.
I'd like to call out the docs for navigation.preload:
Preloading a screen means that the screen will be rendered in the background. All the components in the screen will be mounted and the useEffect hooks will be called.
As much as it comes with a good visual effect in isolated testing, at scale this can behave much like a heavy React Context - kill the general JS performance by triggering a lot of the logic in the background, in a much less predictable way. I see (and like) the direction of another part of this PR where we disable the tree when it's not needed, but I'd really emphasize on the importance of performance testing with large accounts before we integrate preloading for this heavy screen (and others).
Search just does a lot of the heavy lifting, and doing this heavy work at a different time (also instantiating effects) can potentially cause us harm, one that is super hard to track, debug and fix.
Preloading itself might yield good results when we want to keep a cheap screen in the memory, but is not a general optimization strategy for really complex (and alive) screens.
There was a problem hiding this comment.
The changes I added were also required for enabling preloading by splitting the SearchPage into smaller components, it became easier to preload only the skeletons. Thanks to that, the entire heavy screen with all search results is not kept in memory, only the lightweight skeletons are.
During preloading, useEffect and useOnyx hooks are still executed, but the actual search is not triggered since it only runs when isFocused is true. This approach helps slightly reduce the issue you mentioned regarding keeping a heavy screen in memory.
Additionally, together with @WojtekBoman, we’re planning to adjust the preloading logic so that not all screens are preloaded at once. We want to spread this over time to optimize performance, but that will require a separate PR.
Given all that, I think keeping preloading here is still worth it despite the trade-offs involved.
| import {openOldDotLink} from '@userActions/Link'; | ||
| import CONST from '@src/CONST'; | ||
|
|
||
| type SearchModalsWrapperProps = { |
There was a problem hiding this comment.
Looking at this interface and the amount of responsibilities within the component, this is the very opposite of writing composable React. This will only grow in size, is error prone and hard to maintain from the very start. Can you explain why it has to be implemented (and why this way) for the preloading?
There was a problem hiding this comment.
It’s not required specifically because of preloading the main reason was that these components were repeatedly used in SearchPage, which had over 1000 lines of code. I figured that any opportunity to make it a bit lighter and more modular was worth taking.
There was a problem hiding this comment.
If you feel that this wrapper is unnecessary, I can remove it, but in my opinion, it simplifies the structure of the SearchPage itself, which is already huge.
There was a problem hiding this comment.
Honestly I think this is something that we'll eventually aim to avoid, so I'd go ahead and remove it. If the structure it complex, it's definitely not a viable solution to create a giga wrapper that is a prop sink for every piece of functionality that lives in there.
Let's focus on the gains coming from not rendering parts of the UI - the rest we can solve later by better composing the screens.
|
|
||
| const {accountID} = useCurrentUserPersonalDetails(); | ||
| const {defaultCardFeed} = useCardFeedsForDisplay(); | ||
| const suggestedSearches = useMemo(() => getSuggestedSearches(accountID, defaultCardFeed?.id), [defaultCardFeed?.id, accountID]); |
There was a problem hiding this comment.
My Chrome tab couldn't record a performance profile for me when working with this branch on the exfy-perf@callstack.com account (Applause workspace). I can't tell for sure, but might this be due to these calculations?
There was a problem hiding this comment.
FYI I had the same by only preloading the Search screen, no additional changes (for this account).
There was a problem hiding this comment.
I think the difficulty in observing this in the profiler may be due to the calculations performed during preloading. That’s why I chose to include a screen recording rather than rely solely on metrics , even if the metrics don’t show a significant difference, the user experience is noticeably improved.
There was a problem hiding this comment.
I need to do some research, but it’s possible that this is due to the time required by passive effects.
| shouldShowFooter={shouldShowFooter} | ||
| /> | ||
| )} | ||
| {(!shouldUseNarrowLayout || isMobileSelectionModeEnabled) && ( |
There was a problem hiding this comment.
This references my previous comment. I see a lot of the changes in the PR could be decoupled from the preloading itself - it looks like much of the performance gain might be coming from disabling a part of the React tree for the initial load. Maybe it can be integrated separately?
There was a problem hiding this comment.
I agree that some of the changes aren’t directly related to preloading, but splitting this into two separate PRs might not be the best idea either. Some of these changes were aimed at fixing bugs that appeared during preloading, such as blank flickering screens or results not being displayed, and at the same time, refactoring this component was necessary to properly store the skeletons as a preloaded screen.
There was a problem hiding this comment.
@adhorodyski I’ll double-check with more tests, but it seems that decomposition brings most of the benefit, while preload adds less and might not be worth including. Removing preload bugs solutions also improves results, so decomposition alone already looks good. I’ll share detailed results tomorrow.
Fixed! |
I don't see the offline indicator on the wide screen. Could you please check again? |
Yes, it’s not there, but I did it on purpose because it seems to me that on the main wide screen we don’t add it either, so I assumed this is working as designed. |
If that doesn't appear, then it's a bug. 
|
It’s already fixed. You were right it wasn’t showing up for me because of the data I had cached locally. After clearing the cache, it displayed correctly on main. It should be all good now. |
|
@linhvovan29546 how is this looking? |
Only one minor UI bug left |
|
I’ve tested across all platforms. Only one minor UI bug remains. I’ll re-review once the conflict is resolved |
|
I think I’ve fixed everything. If you think it’s all good now, I’ll move on to resolving the conflicts. |
|
Yeah please resolve the conflicts |
# Conflicts: # src/pages/Search/SearchPage.tsx # src/pages/Search/SearchPageNarrow.tsx
|
Reviewing now. |
| lastPaymentMethods, | ||
| selectedReportIDs, | ||
| selectedTransactionReportIDs, | ||
| queryJSON, | ||
| selectedPolicyIDs, | ||
| policies, | ||
| integrationsExportTemplates, | ||
| csvExportLayouts, | ||
| clearSelectedTransactions, | ||
| lastPaymentMethods, | ||
| beginExportWithTemplate, | ||
| bulkPayButtonOptions, | ||
| onBulkPaySelected, | ||
| theme.icon, | ||
| styles.colorMuted, | ||
| styles.fontWeightNormal, | ||
| styles.textWrap, | ||
| beginExportWithTemplate, | ||
| integrationsExportTemplates, | ||
| csvExportLayouts, | ||
| policies, | ||
| bulkPayButtonOptions, | ||
| onBulkPaySelected, | ||
| selectedPolicyIDs, | ||
| selectedReportIDs, | ||
| selectedTransactionReportIDs, |
There was a problem hiding this comment.
In future, it would be great to avoid any changes like this that just increase the diff of the pr and make it harder to review
| queueExportSearchWithTemplate({ | ||
| templateName, | ||
| templateType, | ||
| jsonQuery: JSON.stringify(queryJSON), | ||
| reportIDList: [], | ||
| transactionIDList: [], | ||
| policyID, | ||
| }); |
There was a problem hiding this comment.
here as well as far as I can see is no change, maybe if this was a small PR, then feel free to make style changes, but in large prs like this one, it is counter productive 🙌
| queryJSON?: SearchQueryJSON; | ||
| searchResults: OnyxEntry<SearchResults>; | ||
| searchRequestResponseStatusCode: number | null; | ||
| isMobileSelectionModeEnabled: boolean; | ||
| headerButtonsOptions: Array<DropdownOption<SearchHeaderOptionValue>>; | ||
| footerData: { | ||
| count: number | undefined; | ||
| total: number | undefined; | ||
| currency: string | undefined; | ||
| }; | ||
| selectedPolicyIDs: Array<string | undefined>; | ||
| selectedTransactionReportIDs: string[]; | ||
| selectedReportIDs: string[]; | ||
| latestBankItems?: BankAccountMenuItem[]; | ||
| onBulkPaySelected: (paymentMethod?: PaymentMethodType) => void; | ||
| handleSearchAction: (value: SearchParams | string) => void; | ||
| onSortPressedCallback: () => void; | ||
| scrollHandler: (event: NativeSyntheticEvent<NativeScrollEvent>) => void; | ||
| initScanRequest: (e: DragEvent) => void; | ||
| PDFValidationComponent: React.ReactNode; | ||
| ErrorModal: React.ReactNode; | ||
| shouldShowFooter: boolean; |
There was a problem hiding this comment.
All of these should have docs, can you add them in a follow up please?
There was a problem hiding this comment.
I’ll take care of it once I’m done with the inbox.
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.2.63-0 🚀
|
|
🚀 Deployed to production by https://github.com/marcaaron in version: 9.2.63-8 🚀
|
| shouldShowLink={false} | ||
| > | ||
| {!!queryJSON && ( | ||
| <> |
There was a problem hiding this comment.
After this PR we accidentally missed DragAndDropProvider, which led to issue #76986. We've applied the fix here
6 participants
Explanation of Change
Thanks to cleaning up the structure of the search page component, we were able to speed up the time it takes to switch to the Search tab. Below are metrics from several platforms as an example of the time gains:
Desktop web : 2.1s → 1.1s
iOS native : 6,04s → 4.5s
This shows that we’re managing to save roughly half of the time previously needed to switch tabs on the web, and also more than 1 second on the native platform.
dekompozycja.mp4
Fixed Issues
$ #74613
PROPOSAL:
Tests
Go to the Search tab.
Check whether the results are displayed correctly.
Make sure you can correctly open all the filters
Verify you can select multiple expense / reports and open the options for bulk actions
Offline tests
unnesesary
QA Steps
Same as test
// TODO: These must be filled out, or the issue title must include "[No QA]."
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand 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
Screen.Recording.2025-11-14.at.15.55.21.mov
iOS: Native
Screen.Recording.2025-11-14.at.15.15.02.mov
iOS: mWeb Safari
Screen.Recording.2025-11-14.at.15.04.23.mov
MacOS: Chrome / Safari
dekompozycja.1.mov
MacOS: Desktop
Screen.Recording.2025-11-14.at.16.07.07.mov