Refactor convertBulkTrackedExpensesToIOU to accept transactions array…#84046
Conversation
…rigo-revert-81742
melvin-bot
bot
requested review from
ikevin127 and
joekaufmanexpensify
and removed request for
a team
|
@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] |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
No product review needed |
…rigo-revert-81742
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| }, | ||
| [selectedIds], | ||
| ); | ||
| const [selectedTransactions = CONST.EMPTY_OBJECT] = useOnyx(ONYXKEYS.COLLECTION.TRANSACTION, {selector: getSelectedTransactions}); |
There was a problem hiding this comment.
❌ PERF-11 (docs)
The getSelectedTransactions selector on ONYXKEYS.COLLECTION.TRANSACTION returns a Record<string, Transaction> of full Transaction objects. With a selector present, Onyx uses deepEqual (not shallowEqual) to compare outputs on every collection change. Transaction objects have many fields, making this comparison expensive. The selector output is also an intermediate structure the component further reduces via Object.values(selectedTransactions).
Per PERF-11, selectors on collections that return complex objects rather than primitives or small picked-field objects are counterproductive — subscribe without a selector and transform inline.
Suggested fix:
const [allTransactions] = useOnyx(ONYXKEYS.COLLECTION.TRANSACTION);
const selectedTransactions = useMemo(() => {
if (!allTransactions) return CONST.EMPTY_OBJECT;
return Array.from(selectedIds).reduce<Record<string, Transaction>>((acc, id) => {
const txn = allTransactions[`${ONYXKEYS.COLLECTION.TRANSACTION}${id}`];
if (txn) {
acc[`${ONYXKEYS.COLLECTION.TRANSACTION}${txn.transactionID}`] = txn;
}
return acc;
}, {});
}, [allTransactions, selectedIds]);Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
I guess after thoroughly reading https://github.com/Expensify/App/blob/main/.claude/skills/coding-standards/rules/perf-11-optimize-data-selection.md it makes more sense to not use a selector and filter it inline. You can also simplify your filtering to match this example better:
// GOOD: No selector — filter and compute Set inline
const [reportNameValuePairs] = useOnyx(ONYXKEYS.COLLECTION.REPORT_NAME_VALUE_PAIRS);
const archivedReportIds = new Set(
Object.entries(reportNameValuePairs ?? {})
.filter(([, value]) => value?.isArchived)
.map(([key]) => key),
);
There was a problem hiding this comment.
Hm, but I really hate that it's subscribing to the entire transaction collection. @adhorodyski What do you think for this one? My first instinct is to keep using a selector to filter the collection down to just the transactions matching selectedIds.
There was a problem hiding this comment.
The most important part, render isolation is already in place. This means even if we don't write it perfect here, but 'good enough', it's fine because we'll only rerender the button (and not some whole list component).
Do not preoptimize unless it's really a bottleneck to render this component. We could absolutely use a selector here, it's idiomatically correct and it's only deepEqual's problem that it might be slow. No need to rerender this on unrelated changes to transactions (any of them).
There was a problem hiding this comment.
Awesome, thanks for the tips! I think in this case then, let's go ahead and use the selector @parasharrajat so that we don't have to worry about changes to other transactions.
There was a problem hiding this comment.
I made the change, but now the selectedTransaction does not update properly; it seems like an onyx issue. I reported it earlier, and it was resolved, but I can reproduce that issue again.
There is no use in spending time back and forth on this. Can we move ahead without selector as the selector logic has some bug. @tgolen
There was a problem hiding this comment.
For the sake of this PR, sure. Please open an issue detailing the bug so we can be sure to fix it though.
There was a problem hiding this comment.
I started to discuss here https://expensify.slack.com/archives/C08CZDJFJ77/p1772563117231579. There was a confirmation that it is a Onyx issue.
![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: 833cd00223
ℹ️ 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".
| }, | ||
| [selectedIds], | ||
| ); | ||
| const [selectedTransactions = CONST.EMPTY_OBJECT] = useOnyx(ONYXKEYS.COLLECTION.TRANSACTION, {selector: getSelectedTransactions}); |
There was a problem hiding this comment.
In AddUnreportedExpenseFooter, getSelectedTransactions closes over selectedIds, but the useOnyx subscription omits the third-argument dependency list. For collection selectors, that means selection-only UI changes can leave selectedTransactions stale until the transaction collection itself changes, so pressing Confirm can submit an outdated subset via Object.values(selectedTransactions) and move the wrong expenses.
Useful? React with 👍 / 👎.
|
Ready @ikevin127 |
Reviewer Checklist
Screenshots/VideosScreen.Recording.2026-02-16.at.17.09.20.mov |
|
@parasharrajat some import issues |
|
@rlinoz Fixed |
|
These deprecated warnings are coming from main. |
|
@parasharrajat Those warnings should be fixed in For my own note, we are still needing to come to a conclusion on this thread. Looks like Adam is looking at it today. |
|
All resolved. |
|
Responding in the discussion thread on rule-11 |
|
@parasharrajat prettier is failing |
|
we need to wait till Onys version is bumped based on https://expensify.slack.com/archives/C08CZDJFJ77/p1773141164323519?thread_ts=1772563117.231579&cid=C08CZDJFJ77 |
|
Finally ready. All set. |
|
Ready to roll @rlinoz |
|
Merging as there are some PRs depending on this one. |
|
🚧 @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! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/rlinoz in version: 9.3.39-0 🚀
|
|
🚀 Deployed to production by https://github.com/cristipaval in version: 9.3.39-3 🚀
|
7 participants
Explanation of Change
Fixed Issues
$ #66510
$ #83761
$ #83712
PROPOSAL:
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
12.02.2026_20.27.35_REC.mp4
04.03.2026_15.55.37_REC.mp4