Optimize defaultExpensifyCardSelector by removing translate dependency#84366
Conversation
Extract getExpensifyCardFeedsForDisplay from getCardFeedsForDisplay so the selector no longer needs the translate parameter. This eliminates the inline closure in useSearchTypeMenuSections that was causing unnecessary re-renders on every render cycle. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@ZhenjaHorbach 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] |
|
All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
|
Conflicts |
|
And even if there's no UI changes, we need to record videos to confirm that nothing is broken |
| * This is extracted from getCardFeedsForDisplay so it can be called independently | ||
| * (e.g. from selectors that only need Expensify Card feeds). | ||
| */ | ||
| function getExpensifyCardFeedsForDisplay(allCards: CardList | undefined): CardFeedsForDisplay { |
There was a problem hiding this comment.
Let's add tests for this util
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Thanks for the review! I've added unit tests for getExpensifyCardFeedsForDisplay in the latest push. It covers empty/undefined inputs, deduplication, mixed card lists, and equivalence with getCardFeedsForDisplay. Regarding the video recordings, since this is a pure logic refactor (selector internals + removing an unused translate dependency) with no UI or rendering changes, would web (Chrome/Safari) and mWeb recordings be sufficient? Happy to set up native environments if needed, just wanted to check first. |
web (Chrome/Safari) and mWeb will be enough |
Screen.Recording.2026-03-06.at.3.51.36.AM.movScreen.Recording.2026-03-06.at.3.48.08.AM.2.mov |
Explanation of Change
defaultExpensifyCardSelector(~13ms per render) was causing unnecessary re-renders inuseSearchTypeMenuSectionsdue to two compounding issues:Unstable selector reference: The selector was wrapped in an inline closure capturing
translate, creating a new function reference on every render. This causeduseOnyxto re-evaluate the selector on every render cycle — even when the underlying card data hadn't changed.Unnecessary work inside the selector: The selector called
getCardFeedsForDisplay({}, allCards, translate), where the first argument{}meant the entire first loop (includinggetOriginalCompanyFeeds) was dead work. Thetranslateparameter was never actually invoked in this code path since the name is always hardcoded asCONST.EXPENSIFY_CARD.BANK. Additionally,Object.values(cards)?.at(0)allocated a full array just to take the first element.Changes made:
getExpensifyCardFeedsForDisplayinCardFeedUtils.ts— a focused function that only iterates cards looking for Expensify Card bank entries, without needingtranslateor processing company card feeds.getCardFeedsForDisplaynow delegates to this function internally viaObject.assign, eliminating code duplication.defaultExpensifyCardSelectorinCard.ts— calls the extracted function directly (notranslateparam), usesfor...ofearly return instead ofObject.values().at(0).useSearchTypeMenuSections.ts— the stable module-level selector is passed directly touseOnyx, eliminating the unstable reference and removing theuseLocalizedependency.CardTest.tsto reflect the new selector signature.Reassure benchmark results (head-to-head, old vs new on same machine):
The dominant performance win is the elimination of per-render re-evaluation (not captured by
measureFunction) — the unstable closure previously forceduseOnyxto re-run the selector on every render ofuseSearchTypeMenuSections, which is mounted in the always-visibleNavigationTabBar.Fixed Issues
$ #82425
PROPOSAL: #82425 (comment)
Tests
Offline tests
undefined/empty card lists gracefullyQA 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
N/A — no UI changes. This is a pure performance optimization (selector refactor).
Android: Native
N/A — no UI changes
Android: mWeb Chrome
N/A — no UI changes
iOS: Native
N/A — no UI changes
iOS: mWeb Safari
N/A — no UI changes
MacOS: Chrome / Safari
N/A — no UI changes