Avoid using selector returning Sets #82104
Conversation
melvin-bot
bot
requested review from
eh2077 and
mkhutornyi
and removed request for
a team and
eh2077
|
@mkhutornyi 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
heyjennahay
and removed request for
a team
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
src/hooks/useCardFeedsForDisplay.ts
Outdated
| const [policies] = useOnyx(ONYXKEYS.COLLECTION.POLICY, {canBeMissing: true}); | ||
|
|
||
| const cardFeedsByPolicy = getCardFeedsForDisplayPerPolicy(allFeeds, translate); | ||
| const eligiblePoliciesIDs = getEligiblePolicyIDs(policies); |
There was a problem hiding this comment.
❌ PERF-6 (docs)
The eligiblePoliciesIDs value is computed on every render by calling getEligiblePolicyIDs(policies). Since this computation involves iterating over all policies and creating a new Set each time, it should be memoized to avoid unnecessary recalculation.
Suggested fix:
const eligiblePoliciesIDs = useMemo(() => getEligiblePolicyIDs(policies), [policies]);This ensures the Set is only recreated when the policies object reference changes, not on every render.
Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
False alarm. This component is react compilable so no need useMemo
src/hooks/useCardFeedsForDisplay.ts
Outdated
| selector: eligiblePoliciesSelector, | ||
| canBeMissing: true, | ||
| }); | ||
| const [policies] = useOnyx(ONYXKEYS.COLLECTION.POLICY, {canBeMissing: true}); |
There was a problem hiding this comment.
❌ PERF-11 (docs)
This change causes unnecessary re-renders. By removing the selector and fetching the entire policies collection, the component now re-renders whenever ANY policy changes (even unrelated ones like name updates, member changes, etc.).
The original issue: The selector returned a Set, and useOnyx uses deepEqual for comparison, which is extremely slow for large Sets.
Better fix: Use a selector that returns an array instead of a Set, since arrays are compared efficiently by deepEqual:
const eligiblePoliciesSelector = (policies: OnyxCollection<Policy>): string[] => {
return Object.values(policies ?? {}).reduce((policiesIDs, policy) => {
if (isPaidGroupPolicy(policy) && policy?.areCompanyCardsEnabled) {
policiesIDs.push(policy.id);
}
return policiesIDs;
}, [] as string[]);
};
// Then use it:
const [eligiblePolicyIDs] = useOnyx(ONYXKEYS.COLLECTION.POLICY, {
selector: eligiblePoliciesSelector,
canBeMissing: true,
});
// Convert to Set only if needed for lookups:
const eligiblePoliciesIDs = useMemo(() => new Set(eligiblePolicyIDs), [eligiblePolicyIDs]);This approach:
- Keeps the efficient selector pattern (only re-renders when eligible policies change)
- Avoids the slow Set comparison in
deepEqual - Only creates the Set when needed for O(1) lookups
Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / SafariScreen.Recording.2026-02-11.at.12.37.39.pm.mov |
![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: 34cddf3c79
ℹ️ 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/useCardFeedsForDisplay.ts
Outdated
| selector: eligiblePoliciesSelector, | ||
| canBeMissing: true, | ||
| }); | ||
| const [policies] = useOnyx(ONYXKEYS.COLLECTION.POLICY, {canBeMissing: true}); |
There was a problem hiding this comment.
Subscribing to ONYXKEYS.COLLECTION.POLICY without a selector now causes this hook to update on any policy-field change, not just eligibility changes, and useCardFeedsForDisplay is consumed in search flows that re-render frequently. For large accounts, this means repeated full-policy scans and feed sorting during normal UI activity (e.g., typing), which regresses responsiveness compared with the prior selector-scoped subscription. Please keep the subscription narrowed to an eligibility-derived value (using a selector output that is cheap to compare, such as an array of IDs) to avoid this production performance hit.
Useful? React with 👍 / 👎.
mkhutornyi
left a comment
mkhutornyi
left a comment
There was a problem hiding this comment.
Looks good but curious why we can't fix this in Onyx level. Or if it's expected for Set collection, can't we change it to object or array?
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.
|
2 participants
Explanation of Change
The
eligiblePoliciesSelectorwas being called repeatedly, iterating over all policies to create a Set on each call. TheuseOnyxhook usesdeepEqualto compare the old and newSetvalues, and this comparison is extremely slow for largeSetstructures. Since this selector was invoked frequently, it caused significant performance degradation.A quick fix for this is to avoid using selector and filter the policies in place.
The same above fix applied also in
useArchivedReportsIdSetby avoiding selector returningSettype dataFixed Issues
$77176
PROPOSAL:
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