Fix bulk merge post-merge navigation to open target thread report#83825
Fix bulk merge post-merge navigation to open target thread report#83825marufsharifi wants to merge 23 commits intoExpensify:mainfrom
Conversation
|
PR changes:
|
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@rojiphil, thanks for your patience the pr is ready for your review. The failure unit test doesn't seem related to my changes, btw. thanks. |
|
@rojiphil, kindly bump. thanks. |
|
@marufsharifi There are two issues which needs fix:
Attaching staging and pr version test videos for reference. 83825-pr-issues.mp483825-staging-issues.mp4 |
|
Sorry for the delay, I will try to update this tomorrow. |
|
Sorry for the delay i was OOO, i will update here today. Thanks. |
|
I've fixed the case where we select more than one expense from bulk actions, then merge. But for one expense selected from bulk action, it sometimes doesn't work. I will update here tomorrow. Thanks for your patience. |
- resolve the target transaction thread before starting one-selected report checkbox merge - fall back to creating the transaction thread when thread metadata is not yet hydrated - keep the two-selected report merge path unchanged - add focused hook coverage for report-action and thread-creation overrides
|
This PR fixes the merge destination issues for search-origin expense merges. Root cause
Approach
Result
Tests |
- preserve the prefixed transaction key type in search bulk merge lookup - clean up a spellcheck-only encoded route string in merge tests - use typed report action fixtures in merge utility tests - scope legacy originalMessage and SearchResults casts in hook tests
|
@rojiphil, I've fixed all the issue, could you please take another look when you get a chance? thanks. |
|
@rojiphil, gentle bump. thanks. |
|
Reviewing today |
|
@rojiphil, gentle bump. thanks. |
rojiphil
left a comment
There was a problem hiding this comment.
@marufsharifi Thanks for the PR. Just one query though.
Also, there typecheck and unit test failures. And need conflict resolution too.
Please have a look.
|
|
||
| mergedTransactions[key] = { | ||
| ...onyxTransaction, | ||
| transactionThreadReportID: snapshotTransaction.transactionThreadReportID, |
There was a problem hiding this comment.
Why is there a need to set transactionThreadReportID here again?
|
The different scenarios work well too. 83825-web-chrome-001.mp4 |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a1728068d9
ℹ️ 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".
- make merge thread overrides transaction-aware instead of passing a bare thread ID - only persist the override when it still matches the final kept target after merge target/source selection - prevent same-report split merges from redirecting to a deleted source thread - add targeted coverage for split-over-cash target swap and updated hook override behavior
- align report checkbox merge tests with the actual lowercase bulk action value - use the shared merge action constant instead of a hardcoded "MERGE" string - apply the expected Prettier formatting in MergeTransaction
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
Create an expense in self DM.
Create an expense in workspace chat.
Go to Reports > Expenses.
Select the workspace expense via the checkbox.
Click the dropdown button > Merge.
Select self DM expense > Continue.
Select the second option in each section > Next > Merge expenses.
Verify the transaction thread is open after merging expenses via the bulk option
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
web.mp4