[No-QA] chore: [Performance] Optimize useFilterFormValues (~16ms per render)#83746
Conversation
|
@jayeshmangwani Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
|
@TaduJR can you provide measurements on the performance improvement please? |
|
We also have several lint issues on SearchPage. A lot of that logic was removed already |
Sure.
Hmm I think they are occurring after merging with main. Let me check |
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.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b28ed1fb66
ℹ️ 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".
|
@TaduJR let's resolve conflicts and mark the PR as ready for review please |
…rmance-OptimizeuseFilterFormValues-16ms-per-render # Conflicts: # src/components/Search/SearchPageHeader/SearchFiltersBar.tsx
…rmance-OptimizeuseFilterFormValues-16ms-per-render
|
|
All good @luacmartins? |
|
@TaduJR can you provide additional metrics for the actual time it takes to run the hook now? |
|
@jayeshmangwani please prioritize this review when you're online |
|
Asked for a reassingment |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / SafariScreen.Recording.2026-03-08.at.18.45.35.mov |
@TaduJR Aren't we implementing the suggested change in your proposal of using |
…rmance-OptimizeuseFilterFormValues-16ms-per-render
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 @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.34-0 🚀
|
|
🚀 Deployed to production by https://github.com/luacmartins in version: 9.3.34-2 🚀
|
Explanation of Change
useFilterFormValuesran in bothSearchPageanduseSearchFiltersBar, creating duplicate Onyx subscriptions and runningbuildFilterFormValuesFromQuerytwice per render. Additionally, everyuseOnyx(ONYXKEYS.COLLECTION.POLICY)call across the search filter hooks subscribed to the entire policy object — triggeringdeepEqualcomparisons on massive objects (employeeList,connections,customUnits,rules, etc.) whenever any policy field changed.Removes the duplicate
useFilterFormValues+useSearchFilterSyncfromSearchPage— keeps them only inuseSearchFiltersBar, the sole consumer.Adds Onyx selectors to all 7 collection subscriptions across 4 hooks, each extracting only the fields actually consumed downstream:
useFilterFormValues:policiesSelector(taxRates only),reportsSelector(reportID only),policyCategoriesSelector(name only),policyTagsSelector(tag names only)useSearchFiltersBar:typeOptionsPoliciesSelector(9 fields for canSendInvoice check)useAdvancedSearchFilters:advancedSearchPoliciesSelector(17 fields for filter visibility)useExportedToFilterOptions:exportedToPoliciesSelector(id, name, connections, exportLayouts)Optimizes
buildFilterFormValuesFromQuery— hoists 4 constant validation Sets to module level, pre-computes 3 dynamic Sets before the filter loop, and replacesgetTagNamesFromTagsListswith direct inlineiteration into Sets for TAG/CATEGORY paths.
Optimizes
getAllTaxRatesNamesAndKeysin PolicyUtils — replaces O(n).includes()deduplication with O(1)Set-based lookup.Fixed Issues
$ #82421
PROPOSAL: #82421 (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
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari