perf: Improvements to opening reports tab#82393
Conversation
…eWorkspaceList calls
Use useActiveAdminPoliciesCount for length checks and only sort policies when the iteration block is actually reached.
melvin-bot
bot
requested review from
heyjennahay and
hungvu193
and removed request for
a team
|
@hungvu193 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] |
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.
|
![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: 1905bb4ec9
ℹ️ 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".
| searchTerm: '', | ||
| localeCompare, | ||
| }); | ||
| const {typeFiltersKeys, workspaces, shouldShowWorkspaceSearchInput} = useAdvancedSearchFilters(); |
There was a problem hiding this comment.
what does shouldShowWorkspaceSearchInput have to do with filters? looks like a piece that could be abstracted away.
There was a problem hiding this comment.
Suprisingly it has a lot to do with filters
It displays if there is more than 12 items to display 😬
| import {useMemo} from 'react'; | ||
| import {sortPoliciesByName} from '@libs/PolicyUtils'; | ||
| import useActiveAdminPolicies from './useActiveAdminPolicies'; | ||
| import useLocalize from './useLocalize'; | ||
|
|
||
| function useSortedActiveAdminPolicies() { | ||
| const {localeCompare} = useLocalize(); | ||
| const activeAdminPolicies = useActiveAdminPolicies(); | ||
| const sortedActiveAdminPolicies = useMemo(() => sortPoliciesByName(activeAdminPolicies, localeCompare), [activeAdminPolicies, localeCompare]); | ||
|
|
||
| return sortedActiveAdminPolicies; | ||
| } | ||
|
|
||
| export default useSortedActiveAdminPolicies; |
There was a problem hiding this comment.
I think we can remove this hook, instead adding a new prop to useActiveAdminPolicies called shouldShortPolicyName and return sortedActiveAdminPolicies if shouldShortPolicyName is true.
There was a problem hiding this comment.
If we do it this way, it would violate CLEAN-REACT-PATTERNS-1 rule
There was a problem hiding this comment.
Oops. Thanks for noticing me that 🤕 . All good then!
|
@TMisiukiewicz Since you optimized |
|
@hungvu193 good call 👍 I updated the component to reflect the same changes |
Cool. I'm testing. Btw could you update this one to the PR description along with the metrics? |
|
@hungvu193 done ✅ |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / SafariScreen.Recording.2026-02-13.at.17.46.25.mov |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 268f430c5d
ℹ️ 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".
| ); | ||
| return adminPolicies; | ||
| }; | ||
| const activeAdminPoliciesSelector = (policies: OnyxCollection<Policy>, currentUserAccountLogin: string) => getActiveAdminWorkspaces(policies, currentUserAccountLogin); |
There was a problem hiding this comment.
activeAdminPoliciesSelector now calls getActiveAdminWorkspaces, which reads isOfflineNetworkStore() internally, but this selector is consumed through useOnyx subscriptions that only depend on policy/login updates (for example useActiveAdminPolicies passes [login]). That means connectivity changes do not trigger recomputation, so the admin-policy list can become stale across online/offline transitions (especially for pending-delete visibility controlled by shouldShowPolicy) until a separate policy update happens, leading to incorrect workspace/payment options.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I would say we do not need to recompute when that changes
| ); | ||
| return adminPolicies; | ||
| }; | ||
| const activeAdminPoliciesSelector = (policies: OnyxCollection<Policy>, currentUserAccountLogin: string) => getActiveAdminWorkspaces(policies, currentUserAccountLogin); |
There was a problem hiding this comment.
I would say we do not need to recompute when that changes
|
|
||
| function useActiveAdminPolicies() { | ||
| const {login} = useCurrentUserPersonalDetails(); | ||
| const selector = useCallback((policies: OnyxCollection<Policy>) => activeAdminPoliciesSelector(policies, login ?? ''), [login]); |
There was a problem hiding this comment.
Do you need ot use the useCallback here because of the rule we have for the selectors? Otherwise this should not be needed because of the react-compiler, right?
There was a problem hiding this comment.
exactly, not sure how to address it properly at this moment - for already compiling components we could skip the useCallback, but for those that do not compile we still need it. I believe at some point we should be able to drop this rule
|
🚧 @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, 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. |
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.3.20-0 🚀
|
|
🚀 Deployed to production by https://github.com/mountiny in version: 9.3.20-6 🚀
|
|
🚀 Deployed to production by https://github.com/mountiny in version: 9.3.20-6 🚀
|
5 participants
Explanation of Change
A couple of improvements that should speed up opening Report screen for a customer with 5k policies. Shipping them as a single PR as they interop between two components:
SearchFiltersBarandSearchPageSearchFiltersBarwas independently callinguseWorkspaceList()with the same parameters thatuseAdvancedSearchFiltersalready computes internally. This caused the full workspace list (policy filtering, section building, search input threshold check) to run twice on every render. NowuseAdvancedSearchFiltersexposes its already-computedworkspacesandshouldShowWorkspaceSearchInput, eliminating the duplicate work.Similarly,
AdvancedSearchFiltershad its ownuseWorkspaceList()call — also removed in favor of the shared hook.SearchPageandSearchFiltersBarwere callinggetActiveAdminWorkspaces()synchronously on every render, iterating over the entire policy collection to filter admin policies and then sorting the result. This has been replaced withuseActiveAdminPolicies— a dedicated hook backed by an Onyx selector that only recomputes when the underlying policy data changes, benefiting from Onyx's built-in memoization.useBulkPayOptionsandSettlementButtonuseBulkPayOptionspreviously received a pre-sortedactiveAdminPoliciesarray as a prop, meaning the sort ran eagerly on every render regardless of whether the sorted list was needed. The actualsortPoliciesByName()call only executes inside theifblock that iterates over policies, which is the only place ordering matters. Same thing applies now toSettlementButtonwhich follows the same logic.Performance results:
SearchAdvancedFiltersBar: 746ms -> 464ms (37% improvement)SearchPage: 415ms -> 76ms (81% improvement)SettlementButton- did not find a way to do proper measurement quickly, but singlesortoperation on the customers data was ~150ms so now it's being fired only when necessaryFixed Issues
$ #80560
PROPOSAL:
Tests
Offline tests
n/a
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
Same as tests
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