fix: clearer unequal-split language and prevent silent save failure - #676
Conversation
| const { t, displayName, getCurrencyHelpersCached } = useTranslationWithUtils(); | ||
| const splitValidationMessage = t( | ||
| 'expense_details.add_expense_details.split_type_section.validation.invalid_split', | ||
| ); | ||
| const splitDescription = React.useMemo(() => { | ||
| const splitEquallyText = t( | ||
| 'expense_details.add_expense_details.split_type_section.split_equally', | ||
| ); | ||
|
|
||
| if (SplitType.EQUAL !== splitType) { | ||
| return t('expense_details.add_expense_details.split_type_section.split_unequally'); | ||
| } | ||
|
|
||
| if (!paidBy || !currentUser) { | ||
| return splitEquallyText; | ||
| } | ||
|
|
||
| const selectedParticipants = participants.filter((participant) => { | ||
| const share = splitShares[participant.id]?.[SplitType.EQUAL]; | ||
| return share === undefined || 0n !== share; | ||
| }); | ||
|
|
||
| if (0 === selectedParticipants.length) { | ||
| return splitValidationMessage; | ||
| } | ||
|
|
||
| const splitParticipant = selectedParticipants[0]; | ||
| if (1 === selectedParticipants.length && splitParticipant) { | ||
| if (splitParticipant.id === paidBy.id) { | ||
| return t('expense_details.add_expense_details.split_type_section.direction.no_money_flow'); | ||
| } | ||
|
|
||
| const debtor = isNegative ? paidBy : splitParticipant; | ||
| const payer = isNegative ? splitParticipant : paidBy; | ||
| const debtorName = displayName(debtor, currentUser.id); | ||
| const payerName = displayName(payer, currentUser.id); | ||
|
|
||
| if (payer.id === currentUser.id) { | ||
| return t('expense_details.add_expense_details.split_type_section.direction.owes_you', { | ||
| debtor: debtorName, | ||
| }); | ||
| } | ||
|
|
||
| if (debtor.id === currentUser.id) { | ||
| return t('expense_details.add_expense_details.split_type_section.direction.you_owe', { | ||
| payer: payerName, | ||
| }); | ||
| } | ||
|
|
||
| return t('expense_details.add_expense_details.split_type_section.direction.owes_payer', { | ||
| debtor: debtorName, | ||
| payer: payerName, | ||
| }); | ||
| } | ||
|
|
||
| return `${splitEquallyText} (${selectedParticipants.length})`; | ||
| }, [ |
There was a problem hiding this comment.
There is already a util function encapsulating this behavior generateSplitDescription. I would prefer for such extended logic to be kept out of the already huge AddExpensePage file.
| </Button> | ||
| </SplitExpenseForm> | ||
| </div> | ||
| {!isExpenseSettled ? ( |
There was a problem hiding this comment.
The validation should already be handled in splitTypeSection. There should be no way of finding oneself outside of that input with an invalid state, so this logic is redundant, please remove it along with the toast.
| ...p, | ||
| amount: 0n === getSplitShare(p) ? 0n : amount / BigInt(totalParticipants), | ||
| amount: | ||
| 0 === totalParticipants || 0n === getSplitShare(p) |
There was a problem hiding this comment.
See how totalParticipants is calculated. It's the amount of non-zero splitshares, so this check is completely redundant and complicates the most sensitivie part of the code
| @@ -350,7 +351,14 @@ export function calculateParticipantSplit( | |||
|
|
|||
| if (canSplitScreenClosed) { | |||
| let penniesLeft = updatedParticipants.reduce((acc, p) => acc + (p.amount ?? 0n), 0n); | |||
| const participantsToPick = updatedParticipants.filter((p) => p.amount); | |||
| const roundedToZeroParticipants = | |||
| SplitType.EQUAL === splitType | |||
| ? updatedParticipants.filter((p) => 0n === (p.amount ?? 0n) && 0n !== getSplitShare(p)) | |||
| : []; | |||
| const participantsToPick = | |||
| 0 < roundedToZeroParticipants.length | |||
| ? roundedToZeroParticipants | |||
| : updatedParticipants.filter((p) => p.amount); | |||
| const seed = | |||
| cyrb128( | |||
| `${participantsToPick | |||
| @@ -371,6 +379,14 @@ export function calculateParticipantSplit( | |||
| } | |||
| } | |||
| } | |||
|
|
|||
| if ( | |||
| canSplitScreenClosed && | |||
| 1 < participants.length && | |||
| updatedParticipants.every((p) => 0n === (p.amount ?? 0n)) | |||
| ) { | |||
| canSplitScreenClosed = false; | |||
| } | |||
There was a problem hiding this comment.
Care to explain why do you need to refactor such a huge chunk of the core logic? AFAIK all you want is to check an additional edge case, where all the participant.amount fields are zero.
Reworks the unequal-split wording and guard per the inline review. Lint, type-check, 141 tests, and a production build verified.
|
Pushed the requested changes in caa4f29. Lint, type-check, the full 141-test suite, and a production build all pass. Ready for another look. |
| } | ||
| }, | ||
| "validation": { | ||
| "invalid_split": "Adjust who owes this expense before saving." |
Per review: the split validation message belongs with the other error strings rather than nested under expense_details.add_expense_details.split_type_section.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughChangesThe expense split flow now validates zero-amount equal splits before closure, shows a localized error, submits unsettled expenses directly, and generates direction-specific debt descriptions. Expense split flow
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/AddExpense/AddExpensePage.tsx (1)
107-112: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winBlock invalid submission from the header Save button.
The header Save button at Lines 289-295 remains enabled when
isExpenseSettledis false. It callsaddExpense, which now checks onlypaidBy. An invalid self-only split can therefore reachaddOrEditExpense.Disable the header Save button when
!isExpenseSettled.Proposed fix
disabled={ - addExpenseMutation.isPending || !amount || '' === description || isFileUploading + addExpenseMutation.isPending || + !amount || + '' === description || + isFileUploading || + !isExpenseSettled }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/AddExpense/AddExpensePage.tsx` around lines 107 - 112, Disable the header Save button in the AddExpensePage component when isExpenseSettled is false, while preserving its existing disabled conditions. Ensure the button cannot invoke addExpense for invalid self-only splits before reaching addOrEditExpense.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/AddExpense/SplitTypeSection.tsx`:
- Around line 307-309: Add role="alert" to the invalid_split validation message
rendered by SplitTypeSection so assistive technology announces dynamic
validation updates, while preserving the existing conditional rendering and
styling.
---
Outside diff comments:
In `@src/components/AddExpense/AddExpensePage.tsx`:
- Around line 107-112: Disable the header Save button in the AddExpensePage
component when isExpenseSettled is false, while preserving its existing disabled
conditions. Ensure the button cannot invoke addExpense for invalid self-only
splits before reaching addOrEditExpense.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a3269a40-eb30-403b-a7d2-f8820594fcfe
📒 Files selected for processing (6)
public/locales/en/common.jsonsrc/components/AddExpense/AddExpensePage.tsxsrc/components/AddExpense/SplitTypeSection.tsxsrc/store/addStore.tssrc/tests/addStore.test.tssrc/utils/strings.ts
|
Moved it to |
|
One more thing that could be added to this PR is disabling the Save button when the expense is not properly settled. We are good to go then :) |
The Save button in the expense header stayed enabled when the shares did not add up, unlike the bottom action which already guarded on canSplitScreenClosed. Add the same isExpenseSettled guard, and mark the invalid-split message as an alert so it is announced when it appears.
|
Save is now disabled while the split is unsettled, using the same |
2 participants
Description
Scopes the fix for #645 to the two things @krokosik was receptive to in the thread: the silent save failure and the ambiguous "paid for/by you" wording.
When a split left one person owed the full amount, the secondary screen's Save could no-op and the expense would silently disappear. This adds a blocking validation guard in
addStoreso Save is prevented with an inline message instead of discarding the expense, and rewords the directional labels so the direction of money is explicit. Split-definition semantics are unchanged — this only clarifies labels and adds the missing guard, staying inside the scope @krokosik agreed to (no rework of the core split modal to show money flows).Fixes #645
Demo
Non-visual validation + wording change, covered by unit tests.
src/tests/addStore.test.tsadds cases for the one-person-owed split'scanSplitScreenClosed/validity flag and the adjustment-exceeds-total guard; the previously-vanishing expense now blocks Save with a validation error.SKIP_ENV_VALIDATION=true pnpm testpasses (49/49). No jest config change is needed — CI'scheck.ymlalready exportsSKIP_ENV_VALIDATION.Checklist
CONTRIBUTING.mdin its entiretySummary by CodeRabbit
New Features
Bug Fixes
Tests