[NoQA] Optimize CardUtils card filtering to avoid copying large card lists#95272
Conversation
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.
|
# Conflicts: # src/libs/CardUtils.ts
|
🚧 puneetlath has triggered a test Expensify/App build. You can view the workflow run here. |
This comment has been minimized.
This comment has been minimized.
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep it up! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
|
@codex review |
|
Codex Review: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
There was a problem hiding this comment.
Can you add unit tests for these?
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / SafariScreen.Recording.2026-07-08.at.9.25.15.PM.mov |
eh2077
left a comment
There was a problem hiding this comment.
looks good! the refactoring brings better memory efficiency and the functionality remains unchanged.
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 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! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.4.32-0 🚀
|
|
🤖 Help site review: no changes required I reviewed the changes in this PR against the help site articles under Why: This is a purely internal code refactor with no user-facing impact:
The PR is titled Since there are no user-facing changes, I did not create a draft help site PR. |
|
🚀 Deployed to production by https://github.com/grgia in version: 9.4.32-3 🚀
Bundle Size Analysis (Sentry): |
Explanation of Change
Refactors the workspace card-filtering helpers in
CardUtils.tsto reduce peak memory usage when a card feed holds a large number of cards.What this PR does:
cardList-skip rule and iterate the card map in place (no intermediate copy):hasAssignedCardMatching(cardsList, predicate)— short-circuiting boolean scan (replaces the{cardList, ...rest}+Object.values().some()pattern).forEachAssignedCard(cardsList, callback)— side-effect/collect iteration (replaces{cardList, ...rest}+Object.values()when building a result).filterAllInactiveCardsnow accepts the fullWorkspaceCardsListand filters it in a single pass, skippingcardListinternally (or passing it through when the newincludeCardListflag is set), writing surviving cards by reference into one result object — nofilterObjectcopy. The per-card "keep" rule is extracted into a reusableisActiveCard(card, includeDeactivated)predicate.filterInactiveCards/filterInactiveCardsForWorkspacedelegate straight tofilterAllInactiveCardsinstead of rest-spreading + re-merging.getAllCardsForWorkspacefilters each matching feed straight into the accumulator, dropping the per-feed intermediate copy.getCardsByCardholderNameandgetFilteredCardListcollect viaforEachAssignedCardinstead of copying the map first.isSmartLimitEnabled,hasPendingExpensifyCardAction,hasVirtualExpensifyCardMissingPersonalDetails,isCardAlreadyAssigned, and thehasIssuedExpensifyCardSelectorselector usehasAssignedCardMatching(keeping their early-exit behavior).CONST.COMPANY_CARD.CARD_LISTfor thecardListkey literal so the skip is spelled consistently.No behavior change — the same cards are kept/dropped/matched as before; only the allocation pattern changed.
Fixed Issues
$ #93916
PROPOSAL:
Tests
No behavior change, we just need to confirm all works the same way as before.
Company cards
Expensify card
Offline tests
Same, as in Tests section
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
Same, as in Tests section
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand 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
MacOS: Chrome / Safari
ExpensifyCards.mp4
CompanyCards.mp4
NewCardIssue.mp4