Refactor: isolate getAllTaxRates from ONYXKEYS.COLLECTION.POLICY Onyx.connect data#75024
Merged
Merged
Conversation
melvin-bot
Bot
requested review from
ikevin127 and
trjExpensify
and removed request for
a team
November 13, 2025 07:59
|
@ikevin127 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] |
melvin-bot
Bot
requested review from
a team and
tgolen
and removed request for
a team
November 13, 2025 07:59
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| return []; | ||
| } | ||
| return policyIDs.map((policyID) => policies?.[`${ONYXKEYS.COLLECTION.POLICY}${policyID}`]).filter((policy) => !!policy); | ||
| }, [policyIDs, policies]); |
Contributor
There was a problem hiding this comment.
❌ PERF-6 (docs)
Passing the entire policies collection as a dependency causes this useMemo to re-execute whenever any property of any policy changes, even if the policies referenced by policyIDs haven't changed. This is unnecessary since you're only accessing specific policies by their IDs.
Suggested fix:
Instead of depending on the entire policies object, create a selector or extract only the specific policies needed:
const selectedPolicies = useMemo(() => {
if (policyIDs.length === 0) {
return [];
}
return policyIDs.map((policyID) => {
const policy = policies?.[`${ONYXKEYS.COLLECTION.POLICY}${policyID}`];
return policy ? {id: policy.id, taxRates: policy.taxRates} : null;
}).filter((policy) => !!policy);
}, [policyIDs, ...policyIDs.map(id => policies?.[`${ONYXKEYS.COLLECTION.POLICY}${id}`])]);However, this may be complex. Consider using a useOnyx selector approach for the specific policy IDs instead.
| const [currentUserLogin] = useOnyx(ONYXKEYS.SESSION, {selector: emailSelector, canBeMissing: false}); | ||
| const [currentUserAccountID = -1] = useOnyx(ONYXKEYS.SESSION, {selector: accountIDSelector, canBeMissing: false}); | ||
|
|
||
| const taxRates = getAllTaxRates(policies); |
Contributor
There was a problem hiding this comment.
❌ PERF-4 (docs)
The taxRates object is not memoized but is used as a dependency in the taxAutocompleteList useMemo (line 291). Since getAllTaxRates() returns a new object on every render, the taxAutocompleteList will be recalculated on every render, defeating the purpose of memoization.
Suggested fix:
const taxRates = useMemo(() => getAllTaxRates(policies), [policies]);This ensures referential stability and allows the downstream useMemo to work correctly.
Contributor
Author
There was a problem hiding this comment.
Is it really necessary as React compiler should do this automatically?
Contributor
|
PR doesn’t need product input as a refactor PR. Unassigning and unsubscribing myself. |
trjExpensify
removed their request for review
November 13, 2025 22:10
Contributor
Reviewer Checklist
Screenshots/VideosScreen.Recording.2025-11-13.at.15.46.27.mov |
ikevin127
reviewed
Nov 13, 2025
Contributor
|
@shubham1206agra Found this issue by generating testing steps with AI - noting that the issue is present on staging as well - but it is related to the logic we're working on in this PR: Precondition:
issue.mov |
Contributor
|
✅ Reviewed - checklist completed. Awaiting for comments to be addressed before approving 💯 |
Contributor
Author
@ikevin127 This behavior looks as expected. |
ikevin127
approved these changes
Nov 17, 2025
tgolen
requested changes
Nov 17, 2025
tgolen
left a comment
tgolen
left a comment
Contributor
There was a problem hiding this comment.
Can you please see if updating this branch with main will fix the ESLint errors? And if it doesn't, then I think either the imports should be fixed, or at least have the ESLint error disabled on those lines.
tgolen
approved these changes
Nov 18, 2025
57 tasks
Contributor
|
🚀 Deployed to staging by https://github.com/tgolen in version: 9.2.61-0 🚀
|
Contributor
|
🚀 Deployed to production by https://github.com/grgia in version: 9.2.61-5 🚀
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Explanation of Change
Fixed Issues
$ #66397
Tests
Offline tests
Same as Tests
QA Steps
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
Screen.Recording.2025-11-12.at.10.32.36.PM.mov