Avoid full transaction collection scans in useRecentlyAddedData - #97783
Conversation
|
@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] |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| } | ||
| return map; | ||
| }, [localTransactions]); | ||
| const [pendingAddTransactions] = useOnyx(ONYXKEYS.COLLECTION.TRANSACTION, {selector: pendingAddTransactionsSelector}); |
There was a problem hiding this comment.
❌ PERF-11 (docs)
This selector filters the entire TRANSACTION collection (described in the code as holding "tens of thousands of entries") into a Transaction[] of full transaction objects. When a useOnyx subscription uses a selector, Onyx runs deepEqual on the selector output on every collection update to decide whether to re-render. Because the output is a large array of full Transaction objects, that deepEqual is expensive and runs on every unrelated change to the collection — this is exactly the "selector filters/maps a collection into an array" anti-pattern, made worse by returning whole objects rather than a narrowed result. It does not actually save the per-render scan the comment claims, since the scan cost simply moves to per-update.
Downstream the result is only used for its IDs (pendingAddTransactions.map((transaction) => transaction.transactionID)). Since localTransactions is already subscribed to (by key), drop the selector and derive the pending-add IDs inline inside the existing useMemo, as the previous code did:
const [localTransactions] = useOnyx(ONYXKEYS.COLLECTION.TRANSACTION);
// ...inside the useMemo:
const pendingAddIDs = Object.values(localTransactions ?? {})
.filter((transaction) => transaction?.pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD)
.map((transaction) => transaction?.transactionID);This keeps the cheap shallowEqual on the raw localTransactions reference and avoids a redundant second subscription plus the costly deepEqual on a large array output.
Reviewed at: 409adef | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
Because the output is a large array of full Transaction objects
it filters only pendingAction === 'ADD' and that's a handful of not-yet synced optimistic creates.
I think it might be a good candidate to improve the PERF-11 rule, as per docs it should not flag such cases:
The selector meaningfully reduces a large dataset to a small result (e.g., a primitive or a few items) by iterating over the subscribed collection itself — the deepEqual cost on a small result is negligible
cc @kacper-mikolajczak
There was a problem hiding this comment.
Hi @TMisiukiewicz thanks for reporting this!
Correct me if I am wrong but it seems like reviewer has a point - wouldn't narrowing down results of the selector to pendingAddIDs directly yield even greater optimisation?
Apart from that, other parts seem to be all good - I wonder if when we'd specified selector to return pendingAddIDs directly, the reviewer would treat it as no-violation.
There was a problem hiding this comment.
Correct me if I am wrong but it seems like reviewer has a point - wouldn't narrowing down results of the selector to pendingAddIDs directly yield even greater optimisation?
yeah that part is correct, I addressed that. However I think the output here was already reduced to a small and cheap result, so IMO it should not flag it 🤔 At the same time I know that it's difficult for the agent to define how big will be the output of the selector
There was a problem hiding this comment.
That's correct, the reviewer can only guesstimate how heavy the data selection is and in this particular example the rule was kind of "extended" to make a remark on selector that could be narrowed down - let's see if reviewer flags it now after applying the change.
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2026-08-04.at.4.06.53.PM.movAndroid: mWeb ChromeiOS: HybridAppScreen.Recording.2026-08-04.at.4.07.30.PM.moviOS: mWeb SafariMacOS: Chrome / Safariweb.mov |
| /** Selecting inside the subscription scans the (very large) collection once per update rather than once per render. */ | ||
| const pendingAddTransactionsSelector = (transactions: OnyxCollection<Transaction>): Transaction[] => | ||
| Object.values(transactions ?? {}).filter((transaction): transaction is Transaction => transaction?.pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD); |
There was a problem hiding this comment.
Same feedback as here: the selector over-returns, defeating its own purpose
const pendingAddTransactionIDsSelector = (transactions: OnyxCollection<Transaction>): string[] =>
Object.values(transactions ?? {})
.filter((transaction) => transaction?.pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD)
.map((transaction) => transaction.transactionID);
If this doesn't help performance gain much, leave as is.
Not blocker.
rlinoz
left a comment
rlinoz
left a comment
There was a problem hiding this comment.
LGTM, not sure if you want to do this https://github.com/Expensify/App/pull/97783/changes#r3713089098?
|
addressed to make it cleaner 👍 |
|
Codex Review: Didn't find any major issues. Chef's kiss. 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". |
|
🤖 Reviewed — this is a clean, behavior-preserving refactor and the performance win is real. LGTM. All CI is green (ESLint, typecheck, React Compiler Compliance, and the dedicated unit + UI test suites), and I confirmed on web that the Home screen and "Recently added" section still render without errors. The core of the change — dropping the three full One nit on the framing, not a blocker: the description says "unchanged pending transactions no longer trigger recomputations," but Verification details
Home screen showing Recently added section with empty state
|
|
🤖 No regressions found. I hunted the two areas where a scan → keyed-lookup refactor typically breaks, and both hold up. 1. The keyed-lookup safety invariant. The refactor replaces "iterate the whole 2. The split-expense edge case (the one the old fallback comment worried about) still works: splitting mutates 3. Offline edits of a pending row aren't stale. The selector only tracks IDs, but the full Live check: created a manual expense from Home and it appeared correctly in "Recently added" (matching merchant/amount/date), no console errors or layout breakage. Verification details
Home "Recently added" empty (before)
New expense appears in "Recently added" (after)
The only follow-up remains the (non-regression) wording nit from my earlier review: the PR description implies recomputations are skipped, but the memo still recomputes on every |
|
🚧 rlinoz has triggered a test Expensify/App build. You can view the workflow run here. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
6 participants
Explanation of Change
Performance fix for the Home Recently added slot. Profiling showed
useRecentlyAddedDataspending between 50 and 200ms, mostly due to repeatedly scanning the entiretransactions_Onyx collection (~46k entries on large customer account) to resolve data for only ~26 snapshot transactions.The fix moves the pending-ADD filtering into a
useOnyxselector, replaces thetransactionID->transactionMap build with directtransactions_<id>lookups, and resolves unconfirmed rows by key instead of filtering the whole collection.Since selector work now happens at the subscription layer and
useOnyxdeep-equal compares selector output, unchanged pending transactions no longer trigger recomputations.Performance improvement:
useRecentlyAddedDatadrops from ~200ms to ~1ms when switching between reports on a customer account.Fixed Issues
$ #97784
PROPOSAL:
Tests
Offline tests
N/A
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
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
Screen.Recording.2026-08-04.at.13.27.03.mov