fix: skip auto-redistribution when adding splits with manual edits#87124
fix: skip auto-redistribution when adding splits with manual edits#87124ikevin127 wants to merge 4 commits intoExpensify:mainfrom
Conversation
|
@ShridharGoel 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] |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2026-04-04.at.5.00.33.PM.movAndroid: mWeb ChromeScreen.Recording.2026-04-04.at.5.07.17.PM.moviOS: HybridAppScreen.Recording.2026-04-04.at.5.12.25.PM.moviOS: mWeb SafariScreen.Recording.2026-04-04.at.5.13.41.PM.movMacOS: Chrome / SafariScreen.Recording.2026-04-04.at.4.54.03.PM.mov |
|
@ikevin127 Isn't the auto-distribution wrong here? Screen.Recording.2026-04-04.at.5.10.22.PM.mov |
|
Not sure, is this something that differs from current staging behaviour (meaning it only happens on this PR) ? If yes, could you please provide some minimal steps for how you reached the scenario in the video and the exected / actual result ? Would help with debugging 🙌 Aside: AFAIK we didn't have auto-redistribution on the Edit split page - but that might've been added in the meantime by somebody else. |
|
I think we can't compare with staging since there the total is wrong from the beginning itself (the issue that we are fixing in this PR). So, even after changing one of the newly added splits, the total isn't correct there. Staging video: Screen.Recording.2026-04-04.at.5.36.38.PM.movSteps:
|
|
@ShridharGoel Looked into it, what you're seeing is a previously existing logic bug (verifiable on staging as well). The fix I applied is working somewhat correctly, but after taking a second look at it - it's a bit too broad, blocking any auto-redistributions (keeps amount at 0) whenever any of the other existing splits were previously edited manually - this is wrong given our issue's expected result:
🔄 Reworking the solution to make sure the new split will be |
|
@ShridharGoel Update:
☝️ This works only for the initial addition of new splits, if you adjust some of the initial splits: the issues you mentioned here and here will occur just like on current staging (which is out of scope for this PR). 🟢 Applied reworked solution and adjusted tests. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2e309483da
ℹ️ 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".
src/libs/actions/IOU/Split.ts
Outdated
| // Auto-redistribute amounts for all splits if this is not a distance request | ||
| if (!isDistanceRequest) { | ||
| // Skip redistribution if existing splits already match the total (new split stays at 0) | ||
| if (!isDistanceRequest && !splitsAlreadyMatchTotal) { |
There was a problem hiding this comment.
Limit redistribution skip to manually edited splits
addSplitExpenseField now bypasses redistribution whenever existing splits already sum to the transaction total, but that condition is true for normal unedited splits as well. In a common flow (for example, two unedited 50/50 splits), adding a new split will now keep amounts as 500, 500, 0 instead of rebalancing across all unedited splits, which regresses the expected auto-adjust behavior described in this module and user flow. The skip condition should be tied to manual-edit state, not just sum equality.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct, this would regress the initial split scenario when none of the splits were manually edited yet and we add new splits - will apply fix shortly.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 58e67c50c2
ℹ️ 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".
| // Check if existing splits already sum to the total | ||
| const existingSum = existingSplits.reduce((sum, split) => sum + split.amount, 0); | ||
| const hasManuallyEditedSplits = existingSplits.some((split) => split.isManuallyEdited); | ||
| const splitsAlreadyMatchTotal = Math.abs(existingSum) === Math.abs(total); |
There was a problem hiding this comment.
Compare split totals with sign before skipping redistribution
Using Math.abs(existingSum) === Math.abs(total) treats opposite-signed totals as a match, so a negative transaction can incorrectly skip redistribution when manually edited splits sum to the positive magnitude (for example, +1000 vs -1000). In that case shouldRedistribute becomes false, the added split stays at 0, and the draft remains unbalanced, which then blocks saving with validation errors; this regression is introduced by the new skip condition and can be avoided by checking signed equality instead of absolute equality.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is intended, as using absolute values to check equality between splits sum amount and total transaction amount is the only reliable way that works for both positive and negative amounts ✅
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
Explanation of Change
When a user manually edits split amounts and then adds a new split, the previous logic would auto-redistribute amounts among unedited splits. This caused incorrect calculations when the manually edited amount exceeded the total (e.g., editing a -$10 split to -$20 and adding a new split).
Now, new splits start at $0.00 when manual edits exist, allowing users to fully control the distribution.
Fixed Issues
$ #82455
PROPOSAL:
Tests
-$10.00(negative).-$20.00(more than total).$0.00.-$20.00.Please enter a valid amount before continuing.validation error.Warning
Note that the auto-redistribution issues caused by editing previously existing splits AFTER adding the new ones are out of scope for this PR as mentioned here.
Offline tests
N/A
QA Steps
Same as tests.
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
Screen.Recording.2026-04-03.at.19.30.58.mov