Fix bugs for selfDM splits feature#92055
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.
|
|
@dmkt9 @sobitneupane One of you needs to 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] |
|
@dmkt9 is a reviewer here |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 57e546e7df
ℹ️ 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".
…order for IOURequestStepDistanceRate
@dmkt9 |
|
@ZhenjaHorbach Thanks for the ping! I'll finish my review soon. |
Thank you @dmkt9. Please prioritize this review as it's an important feature |
| await Onyx.merge(snapshotKey, { | ||
| data: { | ||
| [split1SnapshotKey]: split1TransactionForSnapshot, | ||
| [split2SnapshotKey]: split2TransactionForSnapshot, | ||
| }, | ||
| search: {type: CONST.SEARCH.DATA_TYPES.EXPENSE, isLoading: false}, | ||
| } as unknown as SearchResults); |
There was a problem hiding this comment.
| await Onyx.merge(snapshotKey, { | |
| data: { | |
| [split1SnapshotKey]: split1TransactionForSnapshot, | |
| [split2SnapshotKey]: split2TransactionForSnapshot, | |
| }, | |
| search: {type: CONST.SEARCH.DATA_TYPES.EXPENSE, isLoading: false}, | |
| } as unknown as SearchResults); | |
| const snapshotData: SearchResults['data'] = {}; | |
| if (split1TransactionForSnapshot) { | |
| snapshotData[split1SnapshotKey] = split1TransactionForSnapshot; | |
| } | |
| if (split2TransactionForSnapshot) { | |
| snapshotData[split2SnapshotKey] = split2TransactionForSnapshot; | |
| } | |
| await Onyx.merge(snapshotKey, { | |
| data: snapshotData, | |
| search: {type: CONST.SEARCH.DATA_TYPES.EXPENSE, isLoading: false}, | |
| }); |
| await Onyx.merge(snapshotKey, { | ||
| data: { | ||
| [workspaceChildSnapshotKey]: workspaceChildTransaction, | ||
| [selfDMChildSnapshotKey]: selfDMChildTransaction, | ||
| }, | ||
| search: {type: CONST.SEARCH.DATA_TYPES.EXPENSE, isLoading: false}, | ||
| } as unknown as SearchResults); |
There was a problem hiding this comment.
| await Onyx.merge(snapshotKey, { | |
| data: { | |
| [workspaceChildSnapshotKey]: workspaceChildTransaction, | |
| [selfDMChildSnapshotKey]: selfDMChildTransaction, | |
| }, | |
| search: {type: CONST.SEARCH.DATA_TYPES.EXPENSE, isLoading: false}, | |
| } as unknown as SearchResults); | |
| const snapshotData: SearchResults['data'] = {}; | |
| snapshotData[workspaceChildSnapshotKey] = workspaceChildTransaction; | |
| snapshotData[selfDMChildSnapshotKey] = selfDMChildTransaction; | |
| await Onyx.merge(snapshotKey, { | |
| data: snapshotData, | |
| search: {type: CONST.SEARCH.DATA_TYPES.EXPENSE, isLoading: false}, | |
| }); |
| await Onyx.merge(snapshotKey, { | ||
| data: { | ||
| [child1SnapshotKey]: childTransaction1, | ||
| [child2SnapshotKey]: childTransaction2, | ||
| }, | ||
| search: {type: CONST.SEARCH.DATA_TYPES.EXPENSE, isLoading: false}, | ||
| } as unknown as SearchResults); |
There was a problem hiding this comment.
| await Onyx.merge(snapshotKey, { | |
| data: { | |
| [child1SnapshotKey]: childTransaction1, | |
| [child2SnapshotKey]: childTransaction2, | |
| }, | |
| search: {type: CONST.SEARCH.DATA_TYPES.EXPENSE, isLoading: false}, | |
| } as unknown as SearchResults); | |
| const snapshotData: SearchResults['data'] = {}; | |
| snapshotData[child1SnapshotKey] = childTransaction1; | |
| snapshotData[child2SnapshotKey] = childTransaction2; | |
| await Onyx.merge(snapshotKey, { | |
| data: snapshotData, | |
| search: {type: CONST.SEARCH.DATA_TYPES.EXPENSE, isLoading: false}, | |
| }); |
| "../../tests/actions/IOUTest/SplitTest.ts" "@typescript-eslint/no-unsafe-type-assertion" 23 | ||
| "../../tests/actions/IOUTest/TrackExpenseTest.ts" "@typescript-eslint/no-unsafe-type-assertion" 10 |
There was a problem hiding this comment.
| "../../tests/actions/IOUTest/SplitTest.ts" "@typescript-eslint/no-unsafe-type-assertion" 23 | |
| "../../tests/actions/IOUTest/TrackExpenseTest.ts" "@typescript-eslint/no-unsafe-type-assertion" 10 | |
| "../../tests/actions/IOUTest/SplitTest.ts" "@typescript-eslint/no-unsafe-type-assertion" 20 | |
| "../../tests/actions/IOUTest/TrackExpenseTest.ts" "@typescript-eslint/no-unsafe-type-assertion" 2 |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 87ae5689f0
ℹ️ 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".
| const rateFromAnyPolicy = DistanceRequestUtils.getEnabledRateByCustomUnitRateIDFromAnyPolicy(transactionChanges.customUnitRateID, policies); | ||
| if (rateFromAnyPolicy?.rate) { | ||
| updatedMileageRate = rateFromAnyPolicy; |
There was a problem hiding this comment.
Recompute distance units for fallback mileage rates
When the selected customUnitRateID is only found via the new cross-policy fallback and that rate uses a different unit, the transaction has already stored newDistanceUnit from the wrong/missing policy and skipped the quantity conversion above. The later amount calculation then treats the old quantity as if it were in the fallback rate's unit (for example, 10 mi becomes 10 km when selecting a km rate), so the split amount/merchant are wrong until corrected by the server, and remain wrong offline. Resolve the fallback rate before setting/converting comment.customUnit.distanceUnit, or redo that conversion when rateFromAnyPolicy is used.
Useful? React with 👍 / 👎.
| const submitButtons: ActionableItem[] = isSubmit2026BetaEnabled | ||
| ? [ | ||
| prepareSubmitDestinationButton(CONST.IOU.SUBMIT_DESTINATION.FRIEND, 'submitToFriend'), | ||
| prepareSubmitDestinationButton(CONST.IOU.SUBMIT_DESTINATION.EMPLOYER, 'submitToEmployer'), |
There was a problem hiding this comment.
Hide the friend destination for split expenses
For Submit 2026 users with at least one workspace, a self-DM split expense still renders the Submit to a friend button here, but split children are now forced into a workspaces-only recipient picker in IOURequestStepParticipants. Tapping this CTA therefore opens an employer/workspace-only flow under a friend label, and if there is a single/default workspace it can submit to that workspace despite the user choosing the friend destination. Build the submit buttons for split expenses with only the employer/workspace destination.
Useful? React with 👍 / 👎.
|
@MelvinBot review |
|
🤖 Reviewed this PR (8 self-DM split bug fixes, ~1.4k LOC). No blockers — the logic is sound and well-commented, and I traced the two highest-risk changes (the reverse-split trigger and the navigation-ordering rework in Verified correct (highest-risk areas)
Worth a look before merge1. 2. Nits (non-blocking)
All CI checks are currently green. Nice test coverage added ( |
Unfortunately, I can reproduce this bug again. |
|
Bug: It looks like the
2026-07-22.16-11-19.mp4 |
This step still occurs on my side. I remember you couldn't reproduce it on your side, but could you please check again? We've had a few bugs in the past that you couldn't reproduce locally, but QA was able to reproduce them.
|
Can't reproduce |
The same 2026-07-22.11.56.51.mov |
I recorded a video in my earlier comment. |
|
@ZhenjaHorbach Which browser are you using? I'm using Chrome/Edge on Linux, and I can reproduce them consistently. |
Chrome on macOs |
|
Plus |
OK, that makes sense to me as well. |
Also works well for me 2026-07-22.12.04.48.mov |
Did you follow my steps? |
But okay |
About this issue |
Explanation of Change
Fixed Issues
$ #91874
$ #91876
$ #91879
$ #91907
$ #91913
$ #91917
$ #91920
$ #91918
PROPOSAL:
Tests
#91874
Precondition:
Test:
#91876
Precondition:
Test:
#91879
Precondition:
Test:
#91907
Test:
#91913
Precondition:
Test:
#91917
Test:
#91918
Test:
#91920
Precondition:
Account has Self DM.
Test:
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 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: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
91874.mov
91876.mov
91879.mov
91907.mov
91913.mov
91918.mov
91920.mov