fix: clearer unequal-split language and prevent silent save failure#676
Open
mvanhorn wants to merge 4 commits into
Open
fix: clearer unequal-split language and prevent silent save failure#676mvanhorn wants to merge 4 commits into
mvanhorn wants to merge 4 commits into
Conversation
krokosik
requested changes
Jun 21, 2026
Comment on lines
+59
to
+115
| 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})`; | ||
| }, [ |
Collaborator
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 ? ( |
Collaborator
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) |
Collaborator
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
Comment on lines
302
to
+389
| @@ -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; | |||
| } | |||
Collaborator
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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 entirety