Fix bulk expense merge opening self DM instead of transaction thread#89360
Fix bulk expense merge opening self DM instead of transaction thread#89360marufsharifi wants to merge 15 commits into
Conversation
melvin-bot
Bot
requested review from
heyjennahay and
rojiphil
and removed request for
a team
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![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: fdc20abd97
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (routeName !== SCREENS.RIGHT_MODAL.SEARCH_REPORT) { | ||
| return false; |
There was a problem hiding this comment.
isSearchOriginForMerge returns false for every route except SCREENS.RIGHT_MODAL.SEARCH_REPORT, so merges initiated from SCREENS.RIGHT_MODAL.SEARCH_MONEY_REQUEST_REPORT are now classified as non-search. In that flow we lose the isOnSearch flag and fall back to the non-search post-merge navigation path, which can reopen/dismiss to the wrong destination instead of the search transaction context.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@rojiphil, this makes sense, but I couldn't reproduce this. Are you able to reproduce this? thanks.
There was a problem hiding this comment.
This theoretically seems like a legit concern
There was a problem hiding this comment.
isSearchOriginForMerge is called in a component that only mounts under MoneyRequestHeader. However, MoneyRequestHeader shows with route.name of SearchMoneyRequestReport only for transaction thread report within SearchMoneyRequestReportPage. And SearchMoneyRequestReportPage always displays expense report and not transaction thread report. So, theoretically, the scenario looks unreachable with the current navigation.
|
@rojiphil, gentle bump. thanks. |
|
@rojiphil, could you please take a look at this pr? thanks. |
|
@marufsharifi Due to multiple PRs here, it's becoming a little messy here. |
@rojiphil, sorry, I've added the flow in the PR author checklist. Thanks. |
|
@rojiphil, merged the main and resolved the conflict; it is ready for your review. thanks. |
rojiphil
left a comment
rojiphil
left a comment
There was a problem hiding this comment.
@marufsharifi The previous PR review comments are still pending. Anyway, I have copied it here so that we do not miss.
Also, regarding the concern mentioned here, can you please share the test steps that would fail if we do not include those changes? I would ilke to explore if we can completely avoid going through all the transactions again.
- delete getTargetTransactionThreadReportIDForSearchSelection from MergeTransactionUtils - update MergeTransactionUtilsTest to cover getTargetTransactionThreadReportIDForSelection directly - keep behavior unchanged while simplifying the helper surface
@rojiphil, I've updated the PR checklist regarding this. |
|
@rojiphil, I've addressed your comment. Could you please take another look when you get a chance? thanks. |
rojiphil
left a comment
rojiphil
left a comment
There was a problem hiding this comment.
Thanks @marufsharifi for addressing the concern regarding getTargetTransactionThreadReportIDForSearchSelection.
Regarding the other concern, I am still not clear. I have left a comment below. Please have a look.
|
|
||
| mergedTransactions[key] = { | ||
| ...onyxTransaction, | ||
| transactionThreadReportID: snapshotTransaction.transactionThreadReportID, |
There was a problem hiding this comment.
regarding the concern mentioned #83825 (comment), can you please share the test steps that would fail if we do not include those changes.
@rojiphil, I've updated the PR checklist regarding this.
Sorry. I did not understand the statement. What I wanted is to explore is whether we can avoid going through all the transactions again. Are you saying that the test steps in PR checklist would fail if we do not include this change?
There was a problem hiding this comment.
@rojiphil, Yes, the second flow in the PR checklist will fail without this change.
There was a problem hiding this comment.
Sorry. I still seem to be missing something. I just tested flow-2 without these changes and it seemed to work. What exactly are we fixing here?
89360-test-001.mp4
There was a problem hiding this comment.
Thanks, this helped clarify the concern.
After rechecking this flow, I’m no longer able to reproduce the stale-overlap case that this fallback loop in useAllTransactions() was intended to protect. Since I couldn’t validate the issue with a reliable repro anymore, I removed the loop and the related test coverage that was specifically asserting that behavior.
At this point, I think it’s better to avoid carrying forward a defensive merge path that we can’t currently justify with a reproducible UI scenario. If we later find a concrete repro showing that search snapshot data is still observably fresher than the overlapping Onyx transaction in this flow, we can add back a targeted fix with clearer evidence.
|
@rojiphil, friendly bump. thanks. |
|
@rojiphil, gentle bump. thanks. |
|
@marufsharifi Will come back to this soon. Sorry for the delay. |
|
Thanks for the merge @marufsharifi . Reviewing this today |
|
@rojiphil, could you please check this PR? I repeatedly encounter conflicts and type failures due to recent code updates. thanks. |
|
Reviewing. Will share updates today |
rojiphil
left a comment
rojiphil
left a comment
There was a problem hiding this comment.
@marufsharifi Continuing to my earlier comment, I still am unclear about the need of the change. I have a left a comment there. Please have a look. Thanks.
|
|
||
| mergedTransactions[key] = { | ||
| ...onyxTransaction, | ||
| transactionThreadReportID: snapshotTransaction.transactionThreadReportID, |
There was a problem hiding this comment.
Sorry. I still seem to be missing something. I just tested flow-2 without these changes and it seemed to work. What exactly are we fixing here?
89360-test-001.mp4
- stop preserving transactionThreadReportID from the search snapshot in useAllTransactions - let overlapping Onyx transactions fully override search snapshot transactions - keep the merge/search flow aligned with the simplified data merge behavior
|
@rojiphil, gentle bump. thanks. |
Reviewer Checklist
Screenshots/VideosAndroid: HybridApp89360-android-hybrid-001.mp4Android: mWeb Chrome89360-mweb-chrome-001.mp4iOS: HybridApp89360-ios-hybrid-001.mp4iOS: mWeb Safari89360-mweb-safari-001.mp4MacOS: Chrome / Safari89360-web-chrome-001.mp4 |
rojiphil
left a comment
rojiphil
left a comment
There was a problem hiding this comment.
@marufsharifi Changes LGTM. Just a small NAB change though. Thanks.
|
@rojiphil, I've addressed your feedback. Could you please take another look when you get a chance? thanks. |
rojiphil
left a comment
rojiphil
left a comment
There was a problem hiding this comment.
Thanks @marufsharifi for the changes.
@MonilBhavsar Changes LGTM.
Over to you. Thanks
MonilBhavsar
left a comment
MonilBhavsar
left a comment
There was a problem hiding this comment.
Commented here #89360 (comment)
@MonilBhavsar That appears theoretical to me but we can include @marufsharifi Can you please include this change as well? |
|
👍 Thanks for looking! |
3 participants
Explanation of Change
Bulk expense merge now follows the same destination behavior as individual merge.
Before this update, bulk merge from
Reports > Expensescould open the wrong place (for example, self-DM) after merge because the app sometimes lost the intended transaction-thread destination during the multi-step merge flow.Now, the app determines the correct target transaction thread earlier in the merge process and carries that destination through to confirmation. That makes post-merge navigation consistent and predictable across flows, especially when merging workspace and
self-DMexpenses from search.follow-up of #79766
Fixed Issues
$ #83257
PROPOSAL:
Tests
Flow 1:
Flow 2:
Offline tests
Same as Tests.
QA Steps
Same as Tests.
// TODO: These must be filled out, or the issue title must include "[No QA]."
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
Android: Native
android.native.mp4
Android: mWeb Chrome
android.web.mp4
iOS: Native
Screen.Recording.2026-03-06.at.5.49.31.PM.mov
iOS: mWeb Safari
Screen.Recording.2026-03-06.at.5.52.45.PM.mov
MacOS: Chrome / Safari
Flow 1:
web.mp4
Flow 2:
Screen.Recording.2026-05-07.at.1.21.19.PM.mov