fix: [Performance] Move search bulk actions logic to a custom hook#83476
fix: [Performance] Move search bulk actions logic to a custom hook#83476luacmartins merged 14 commits intoExpensify:mainfrom
Conversation
melvin-bot
bot
requested review from
Pujan92 and
trjExpensify
and removed request for
a team
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![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: 5686e69e08
ℹ️ 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".
| <DecisionModal | ||
| title={translate('common.downloadFailedTitle')} | ||
| prompt={emptyReportsCount ? translate('common.downloadFailedEmptyReportDescription', {count: emptyReportsCount}) : translate('common.downloadFailedDescription')} |
There was a problem hiding this comment.
The download-failure dialog is now scoped to SearchBulkActionsButton, which is conditionally rendered only while selected items exist; however, the basic export path still calls clearSelectedTransactions() immediately after firing exportSearchItemsToCSV, and its error callback sets this modal state later. In that flow, the button component is unmounted before the callback runs, so users don't see the failure feedback for export errors (the state update targets an unmounted subtree). This was previously safe when the modal lived at page scope.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@MelvinBot Could you apply this feedback by making the change directly in this PR?
There was a problem hiding this comment.
Applied this fix in 5e63ca5.
Root cause: handleBasicExport called clearSelectedTransactions() synchronously right after exportSearchItemsToCSV(). Since SearchBulkActionsButton is conditionally rendered based on selected items, this unmounts the component before the async onDownloadFailed callback can fire — so the error modal state update targets an unmounted subtree.
Fix (2 files):
src/libs/actions/Search.ts—exportSearchItemsToCSVnow returns thefileDownloadpromise instead of discarding it.src/hooks/useSearchBulkActions.ts—handleBasicExportnowawaits the export. AdidFailflag is set inside the error callback; selections are only cleared whendidFailisfalse. On failure the component stays mounted and the error modal is visible to the user.
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
|
@truph01 let's fix the lint error, conflicts and address the comments above |
…il download completes The download-failure dialog is scoped to SearchBulkActionsButton which unmounts when selected items are cleared. The basic export path called clearSelectedTransactions() synchronously after exportSearchItemsToCSV(), so the error callback's state update targeted an unmounted component. Now exportSearchItemsToCSV returns its fileDownload promise and handleBasicExport awaits it, only clearing selections on success. On failure the component stays mounted and users see the error modal. Co-authored-by: truph01 <truph01@users.noreply.github.com>
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppSimulator.Screen.Recording.-.iPhone.15.Pro.-.2026-02-26.at.18.16.12.moviOS: mWeb SafariSimulator.Screen.Recording.-.iPhone.15.Pro.-.2026-02-26.at.18.25.37.movMacOS: Chrome / SafariScreen.Recording.2026-02-26.at.17.54.40.mov |
|
PR doesn’t need product input as a performance PR. Unassigning and unsubscribing myself. |
|
@codex Please review this PR with the following special focus: This PR extracts bulk action logic from When reviewing, please verify the following:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5e63ca52fa
ℹ️ 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/useSearchBulkActions.ts
Outdated
| if (selectedTransactionsKeys.length < 3 && currentSearchResults?.search.type !== CONST.SEARCH.DATA_TYPES.EXPENSE_REPORT && currentSearchResults?.data) { | ||
| const {transactions: searchedTransactions, reports, policies: transactionPolicies} = getTransactionsAndReportsFromSearch(currentSearchResults, selectedTransactionsKeys); |
There was a problem hiding this comment.
The merge-action gate now depends on currentSearchResults only, but the pre-refactor logic in SearchPage.tsx used the searchResults fallback (including lastNonEmptySearchResults) while sorting. When sorting causes currentSearchResults.data to be temporarily unset, this condition fails and the merge option disappears for the same selected transactions until results reload, which is a regression in bulk-action availability.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@MelvinBot Could you apply this feedback?
There was a problem hiding this comment.
Applied this fix in efcfc54.
The merge-action gate was using currentSearchResults directly, which becomes temporarily undefined during sorting/loading. The original SearchPage.tsx used a searchResults fallback variable backed by lastNonEmptySearchResults.
Fix: Added the same pattern inside the hook:
- A
lastNonEmptySearchResultsRefthat caches results whenevercurrentSearchResults.datais available. - A local
searchResultsvariable that falls back to the cached ref whencurrentSearchResults.datais temporarily unset. - The merge option logic now uses
searchResultsinstead ofcurrentSearchResults, matching the original behavior.
…lback The merge-action gate now uses a cached searchResults fallback instead of currentSearchResults directly. When sorting causes data to be temporarily unset, the hook falls back to lastNonEmptySearchResults (matching the original SearchPage.tsx pattern) so the merge option remains available for the same selected transactions. Co-authored-by: truph01 <truph01@users.noreply.github.com>
|
@truph01 we have conflicts again |
|
@luacmartins All good now! |
|
🚧 @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! 🧪🧪
|
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
@truph01 @Pujan92 did you measure the performance of these changes? We cant see any notes about that, just to confirm this actually meaningfully improved the metric cc @adhorodyski |
|
I didn't measure the performance metric here but based on the issue description, I thought it made sense to split the logic of bulk action dropdown button and the computation part in separate component & hook. Though I should highlight that the performance might not be improved as earlier also we are returning early here(while deriving headerButtonsOptions). |
|
🚀 Deployed to staging by https://github.com/luacmartins in version: 9.3.27-0 🚀
|
|
@mountiny I had measured the performance of delaying calling this hook to the selection component when I proposed the issue. That being said, I realize that I didn't include the performance gain in my proposal. I'll do that next time! |
|
Sounds great, thank you! I thin the PR author should be the main one to measure it and test unless they ask someone to do that for them to confirm cc @truph01 |
|
🚀 Deployed to production by https://github.com/blimpich in version: 9.3.27-8 🚀
|
7 participants
Explanation of Change
useSearchBulkActionshook, move all bulk action logic inside the hook.SearchBulkActionsButton, conditionally rendering it if any items are selected.SearchBulkActionsButtonwill then call theuseSearchBulkActionshook to setup the available actionsFixed Issues
$ #83281
PROPOSAL: #83281 (comment)
Tests
Offline tests
QA Steps
// 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
Screen.Recording.2026-02-26.at.20.13.54.mov
Android: mWeb Chrome
Screen.Recording.2026-02-26.at.20.15.43.mov
iOS: Native
Screen.Recording.2026-02-26.at.20.09.46.mov
iOS: mWeb Safari
Screen.Recording.2026-02-26.at.20.07.50.mov
MacOS: Chrome / Safari
Screen.Recording.2026-02-26.at.02.56.04.mov