Repository navigation
Fix merge expense transaction thread navigation - #96633
marufsharifi wants to merge 59 commits into
Conversation
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
|
@rojiphil, could you please take a look at this. thanks. |
|
@rojiphil, Gentle bump. thanks. |
|
@rojiphil, Friendly bump. thanks. |
|
@marufsharifi Picking this up today |
|
Thanks @marufsharifi for the update. The core approach used here looks like a good improvement over the original PR. The offline issue also seems to have been fixed. Will take a closer look at the code sometime today. 96633-web-chrome-001.mp4 |
|
@rojiphil, Quick bump. thanks. |
|
@rojiphil, quick bump. thanks. |
|
@rojiphil, Friendly bump. thanks. |
|
bumped in slack. |
|
@codex review |
|
@MelvinBot review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d066c8dfb4
ℹ️ 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".
Review — looks solid ✅Approach is sound and matches the reported bug (#83257). The fix has three coordinated parts, all of which check out:
Verification
A couple of non-blocking things worth a look: Observations / questions
Nice test coverage on the new branches (unreported→self-DM attach, and the "IOU action has no This is an automated review. Reply with |
|
@rojiphil, Friendly bump. thanks. |
|
@rojiphil, conflicts resolved again. This PR has been open a while and I'm resolving conflicts almost daily. |
|
@marufsharifi The challenge in this issue is that there are quite a lot of edge scenarios that need to be tested. 96633-issue-1.mp4 |
|
Since you are not able to reproduce this, this must be something to do with some difference in workspace or expenses between our setups. |
@rojiphil I was able to reproduce this with a unit test and found the cause. It's fixed now — |
|
@rojiphil, Quick bump. thanks. |
|
Will check this today. |
|
@codex review |
|
@MelvinBot review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Code reviewThe approach looks right, and CI is green apart from the approval check. I found one change worth making before merge, plus two non-blocking notes. 1. Only create a thread when the target has an IOU action and no thread yetIn DynamicConfirmationPage.tsx:135-156,
Suggested guard: if (targetIOUAction?.childReportID) {
targetTransactionThreadReportIDForMerge = targetIOUAction.childReportID;
} else if (targetIOUAction) {
// createTransactionThreadReport(...)
}2. Non-blocking notes
|
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ 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". |
|
@rojiphil addressed Melvin's latest feedback. Could you check once again? Thanks! |
|
Review in progress |
|
Conflicts resolved |
rojiphil
left a comment
There was a problem hiding this comment.
@marufsharifi Did some investigation regarding the issue mentioned at #96633 (comment).
When the merge destination is a self-DM, the target expense's IOU action can already point at a transaction thread that was never opened, so that report is missing from Onyx. The merge only wrote parentReportID and parentReportActionID onto that key, which left a report with no reportID and no reportActions_ entry, so the right-hand pane waited on OpenReport forever while offline.
Looks like we need to seed a full thread report, a placeholder CREATED action, and a loaded state when that report is missing, and still only updates the parent fields when the thread is already in Onyx.
Please look into this. Thanks.
I have left one more review comment for your attention.
@rojiphil, Thanks for the investigation. I couldn't reproduce it on my side, so I've implemented the fix based on your findings. Could you check and ping me? Thanks! |
|
@marufsharifi Seeding the missing thread report is the right fix for the two-field report_. It still never writes reportActions_ or the loading flags, and the empty-actions skeleton is no longer skipped offline, so Flow 3 can keep showing that skeleton. Please also seed a placeholder CREATED action (removed on success) and set hasOnceLoadedReportActions / isLoadingInitialReportActions: false when the thread report is missing. Treat a stored report with no reportID as missing, and cover the no-snapshot case in the test. |
|
@rojiphil all four addressed, including the no-snapshot test case. Could you check and ping me? Thanks!
|

Explanation of Change
Merge expense now follows the same destination behavior across individual and bulk merge flows.
Before this update, merging expenses from
search/spendcould open the wrong place or leave the RHP loading, especially when the target workspace expense did not already have a transaction thread and the destination was anunreported/self-DMexpense.Now, the app creates the missing optimistic transaction thread when needed, carries that thread through confirmation, and attaches offline optimistic merge data to the correct destination report. This makes post-merge navigation consistent and predictable for
workspace,self-DM,split, and offline merge cases.Fixed Issues
$ #83257
PROPOSAL:
Tests
Flow 1: Workspace expense with self-DM expense
Flow 2: Workspace manual expense with split expense
Flow 3: Offline workspace expense with self-DM expense
Offline tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, 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.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
Flow:1
flow01.mov
Flow:2
flow02.mov
Flow:3
flow03.mov
Android: mWeb Chrome
Flow:1
flow1.mp4
Flow:2
flow2.mp4
Flow:3
flow3.mp4
iOS: Native
Flow:1
flow001.mov
Flow:2
flow002.mov
Flow:3
flow003.mov
iOS: mWeb Safari
Flow:1
flow0001.mov
Flow:2
flow0002.mov
Flow:3
Screen.Recording.2026-07-21.at.8.17.39.PM.mov
MacOS: Chrome / Safari
Flow:1
flow1.mov
Flow:2
flow2.mov
Flow:3
flow3.mov