fix: clearer unequal-split language and prevent silent save failure - #676
fix: clearer unequal-split language and prevent silent save failure#676mvanhorn wants to merge 6 commits into
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.
📝 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
| {!canSplitScreenClosed ? ( | ||
| <p className="text-center text-xs text-red-500">{t('errors.invalid_split')}</p> | ||
| ) : null} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Announce the validation message to assistive technology.
The dynamic error text has no live-region semantics. Screen-reader users may not receive the validation result after they change a split. Add role="alert" to this message.
Proposed fix
- <p className="text-center text-xs text-red-500">{t('errors.invalid_split')}</p>
+ <p role="alert" className="text-center text-xs text-red-500">
+ {t('errors.invalid_split')}
+ </p>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {!canSplitScreenClosed ? ( | |
| <p className="text-center text-xs text-red-500">{t('errors.invalid_split')}</p> | |
| ) : null} | |
| {!canSplitScreenClosed ? ( | |
| <p role="alert" className="text-center text-xs text-red-500"> | |
| {t('errors.invalid_split')} | |
| </p> | |
| ) : null} |
🤖 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/SplitTypeSection.tsx` around lines 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.
|
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 :) |
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