fix: harden cloud balance integrity - #709
Conversation
📝 WalkthroughWalkthroughThe change adds settlement allocation persistence, transactional balance reconciliation, participant normalization, multi-currency balance summaries, updated expense flows, and unit/integration test coverage. Group and friend balance mutations now use serializable transactions with retry handling. ChangesSettlement and balance flow
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant splitService
participant runBalanceTransaction
participant balanceProjection
participant Database
Client->>splitService: create or edit settlement expense
splitService->>runBalanceTransaction: execute balance operation
runBalanceTransaction->>Database: begin serializable transaction
splitService->>balanceProjection: capture or restore allocations
balanceProjection->>Database: update group balances and allocations
splitService->>Database: persist expense and participant balances
Database-->>Client: return expense result
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
package.jsonParsing error: ESLint was configured to run on Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (7)
src/tests/splits.test.ts (2)
20-26: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe PERCENTAGE fixture depends on floating-point luck.
33.33 + 33.33 + 33.34is compared against 100 with strict equality insidecalculateParticipantSplit, so this case passes only if the accumulation happens to land exactly on 100. Adding a 4-participant or uneven-percentage case here would surface the fragility flagged onsrc/store/addStore.tsLine 338.🤖 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/tests/splits.test.ts` around lines 20 - 26, The PERCENTAGE case in the cases fixture relies on strict floating-point accumulation equaling 100. Update the test fixture to include a four-participant or uneven-percentage scenario that exposes this precision issue, while preserving coverage of calculateParticipantSplit and the existing split types.
129-150: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSeed
canSplitScreenClosed: falseso Line 149 actually proves something.
setStateleavescanSplitScreenClosedat its store default oftrue, so that assertion would pass even ifsetSplitTypenever recomputed it. The store is also global and never reset, so this state leaks into any test added after it.💚 Suggested change
useAddExpenseStore.setState({ amount: 10, paidBy: payer, splitType: SplitType.EQUAL, + canSplitScreenClosed: false, participants: [🤖 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/tests/splits.test.ts` around lines 129 - 150, Update the test setup for setSplitType in the “reconstructs percentage shares…” case to explicitly initialize canSplitScreenClosed to false, ensuring the assertion verifies that setSplitType recomputes it to true. Reset or otherwise isolate the global useAddExpenseStore state after this test so its seeded state cannot leak into subsequent tests.src/tests/balanceProjection.integration.test.ts (2)
157-160: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffSix independent scenarios in one
test()under a name that describes only the first.Partial settlement, metadata-only edit, deletion, legacy-settlement rejection, departed-member handling, and group-deletion handling all share one test body, so the first failure hides the rest and the failure message won't say which scenario broke. Consider
describe+ subtests over the shared fixture.🤖 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/tests/balanceProjection.integration.test.ts` around lines 157 - 160, Split the single test body currently named “restores group balances when a completed personal settlement becomes partial” into six independently named subtests, covering partial settlement, metadata-only edit, deletion, legacy-settlement rejection, departed-member handling, and group-deletion handling. Use a surrounding describe or shared fixture setup so each subtest runs independently and reports its specific scenario on failure.
151-153: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCleanup only runs on success, so a failing assertion leaks fixtures into the test database.
Every
assertabove this block can abort the test and skip the teardown, leaving users, groups, expenses, and allocations behind for every subsequent run.t.after()(ortry/finally) makes teardown unconditional. Same applies to Lines 511-513.💚 Suggested change
- async () => { + async (t) => { + t.after(async () => { + await db.group.deleteMany({ where: { id: { in: [groupOne.id, groupTwo.id] } } }); + await db.user.deleteMany({ where: { id: { in: [userA.id, userB.id, userC.id] } } }); + await db.$disconnect(); + });(hoisting the ids so they are in scope, and dropping the trailing teardown block)
🤖 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/tests/balanceProjection.integration.test.ts` around lines 151 - 153, Make teardown unconditional in the affected integration tests by moving the cleanup currently following the assertions into a t.after() hook or try/finally block. Hoist the created fixture IDs so cleanup can access them even when assertions fail, and apply the same change to the teardown near the second referenced block; preserve deletion of expenses, allocations, groups, users, and database disconnection.src/store/addStore.ts (1)
257-309: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCent-conservation pass reads well; one subtlety worth a comment.
participantsToAdjustis captured from the pre-inversion amounts, so it means "participants with a non-zero share" rather than "participants with a non-zero final amount". That is the right set for spreading rounding cents, but it is easy to misread two blocks later. A one-line comment would save the next reader the trace.The
Number.isFinite(imbalance)check at Line 288 is already covered by the guard at Line 269 — harmless, but redundant.🤖 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/store/addStore.ts` around lines 257 - 309, The cent-conservation logic needs clarification and minor cleanup. Add a concise comment by participantsToAdjust explaining that it intentionally uses pre-inversion non-zero shares as the correction set, then remove the redundant Number.isFinite(imbalance) guard inside the canSplitScreenClosed block while preserving the earlier finite-amount validation.src/server/api/services/splitService.ts (1)
482-483: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
operationsis inferred asany[], soresult[0] as Expense | undefinedis unchecked.Awaiting the expense creation directly gives you a real
Expenseand drops the cast, while the balance upserts can still go throughPromise.all.♻️ Suggested change
- const result = await Promise.all(operations); - return result[0] as Expense | undefined; + const [createdExpense] = await Promise.all([expenseCreate, ...operations]); + return createdExpense;(with
expenseCreateheld in its own variable instead of being pushed intooperations)🤖 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/server/api/services/splitService.ts` around lines 482 - 483, Update the operation flow near the expense creation and result return so the expense creation promise is stored separately and awaited directly, yielding a typed Expense without casting. Keep only the balance upsert promises in operations and continue awaiting them with Promise.all before returning the created expense.src/server/api/services/balanceTransaction.ts (1)
12-14: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSet explicit
maxWait/timeoutheresrc/server/db.tsdoesn’t configuretransactionOptions, so this interactive transaction still inherits Prisma’s 2s/5s defaults and can fail withP2028under load. If the retry loop stays, add jitter to the backoff to avoid synchronized retries.♻️ Suggested change
return await db.$transaction(operation, { isolationLevel: Prisma.TransactionIsolationLevel.Serializable, + maxWait: 5_000, + timeout: 15_000, });🤖 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/server/api/services/balanceTransaction.ts` around lines 12 - 14, Update the transaction invocation in the balance transaction service to set explicit maxWait and timeout values suitable for this operation, avoiding Prisma’s default limits. If the surrounding retry loop remains, add randomized jitter to its backoff while preserving the existing retry behavior.
🤖 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/pages/add.tsx`:
- Around line 79-87: Handle zero-value SHARE expenses before the edit flow calls
calculateSplitShareBasedOnAmount. Update the surrounding
addStore/share-reconstruction logic so zero amounts use the existing zero-share
behavior before any division occurs, preventing splitShare from becoming NaN and
preserving valid participant values.
In `@src/server/api/services/balanceProjection.ts`:
- Around line 115-143: Change the cleanup in the balance projection flow to
delete only the settlement allocations included in restorableAllocations, rather
than every row matching settlementExpenseId. Preserve allocations excluded by
the membership filter so their provenance remains available for later
reconciliation, and update the affected integration-test expectation
accordingly.
In `@src/server/api/services/splitService.ts`:
- Around line 538-543: The group-expense deletion branch in the delete flow must
gain the same concurrency protection as deletePersonalExpense. Update the group
path around deletePersonalExpense to use runBalanceTransaction, re-read the
expense inside the serializable transaction, and reject or no-op when deletedAt
is already set before reversing balances or marking the expense deleted.
In `@src/store/addStore.ts`:
- Around line 338-339: Update the percentage calculation in
calculateParticipantSplit so independently rounded participant shares cannot
leave the total invalid for a multi-participant split; assign the rounding
residual to the payer, ensuring the resulting shares sum to exactly 100 while
preserving the existing percentage values for other participants.
In `@src/utils/splits.ts`:
- Around line 8-22: Update the participant normalization and validation flow
around storedParticipants to reject any non-finite converted amount, including
NaN and Infinity, before payer, balance, or transaction processing occurs. Use
the existing validation error pattern and preserve the current handling of valid
finite amounts.
---
Nitpick comments:
In `@src/server/api/services/balanceTransaction.ts`:
- Around line 12-14: Update the transaction invocation in the balance
transaction service to set explicit maxWait and timeout values suitable for this
operation, avoiding Prisma’s default limits. If the surrounding retry loop
remains, add randomized jitter to its backoff while preserving the existing
retry behavior.
In `@src/server/api/services/splitService.ts`:
- Around line 482-483: Update the operation flow near the expense creation and
result return so the expense creation promise is stored separately and awaited
directly, yielding a typed Expense without casting. Keep only the balance upsert
promises in operations and continue awaiting them with Promise.all before
returning the created expense.
In `@src/store/addStore.ts`:
- Around line 257-309: The cent-conservation logic needs clarification and minor
cleanup. Add a concise comment by participantsToAdjust explaining that it
intentionally uses pre-inversion non-zero shares as the correction set, then
remove the redundant Number.isFinite(imbalance) guard inside the
canSplitScreenClosed block while preserving the earlier finite-amount
validation.
In `@src/tests/balanceProjection.integration.test.ts`:
- Around line 157-160: Split the single test body currently named “restores
group balances when a completed personal settlement becomes partial” into six
independently named subtests, covering partial settlement, metadata-only edit,
deletion, legacy-settlement rejection, departed-member handling, and
group-deletion handling. Use a surrounding describe or shared fixture setup so
each subtest runs independently and reports its specific scenario on failure.
- Around line 151-153: Make teardown unconditional in the affected integration
tests by moving the cleanup currently following the assertions into a t.after()
hook or try/finally block. Hoist the created fixture IDs so cleanup can access
them even when assertions fail, and apply the same change to the teardown near
the second referenced block; preserve deletion of expenses, allocations, groups,
users, and database disconnection.
In `@src/tests/splits.test.ts`:
- Around line 20-26: The PERCENTAGE case in the cases fixture relies on strict
floating-point accumulation equaling 100. Update the test fixture to include a
four-participant or uneven-percentage scenario that exposes this precision
issue, while preserving coverage of calculateParticipantSplit and the existing
split types.
- Around line 129-150: Update the test setup for setSplitType in the
“reconstructs percentage shares…” case to explicitly initialize
canSplitScreenClosed to false, ensuring the assertion verifies that setSplitType
recomputes it to true. Reset or otherwise isolate the global useAddExpenseStore
state after this test so its seeded state cannot leak into subsequent tests.
🪄 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: c4919379-5ee8-49af-8d5e-56638d0263bc
📒 Files selected for processing (16)
package.jsonprisma/migrations/20260726000000_add_settlement_allocations/migration.sqlprisma/schema.prismasrc/pages/add.tsxsrc/pages/groups.tsxsrc/server/api/routers/group.tssrc/server/api/routers/user.tssrc/server/api/services/balanceProjection.tssrc/server/api/services/balanceTransaction.tssrc/server/api/services/splitService.tssrc/store/addStore.tssrc/tests/balanceProjection.integration.test.tssrc/tests/balances.test.tssrc/tests/splits.test.tssrc/utils/balances.tssrc/utils/splits.ts
| const participants = calculateSplitShareBasedOnAmount( | ||
| amount, | ||
| expenseQuery.data.expenseParticipants.map((participant) => ({ | ||
| ...participant.user, | ||
| amount: toFixedNumber(participant.amount), | ||
| })), | ||
| expenseQuery.data.splitType, | ||
| calculateSplitShareBasedOnAmount( | ||
| toFixedNumber(expenseQuery.data.amount), | ||
| expenseQuery.data.expenseParticipants.map((ep) => ({ | ||
| ...ep.user, | ||
| amount: toFixedNumber(ep.amount), | ||
| })), | ||
| expenseQuery.data.splitType, | ||
| expenseQuery.data.paidByUser, | ||
| ), | ||
| expenseQuery.data.paidByUser, | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Handle zero-value SHARE expenses before reconstructing shares.
Line 79 can hydrate a zero-amount SHARE expense. calculateSplitShareBasedOnAmount divides by amount before its zero guard, so splitShare becomes NaN and the edit flow cannot produce valid participant values.
Proposed fix in src/store/addStore.ts
case SplitType.SHARE:
+ if (amount === 0) {
+ return participants.map((participant) => ({
+ ...participant,
+ splitShare: 0,
+ }));
+ }
+
const shares = participants.map((p) =>🤖 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/pages/add.tsx` around lines 79 - 87, Handle zero-value SHARE expenses
before the edit flow calls calculateSplitShareBasedOnAmount. Update the
surrounding addStore/share-reconstruction logic so zero amounts use the existing
zero-share behavior before any division occurs, preventing splitShare from
becoming NaN and preserving valid participant values.
| const restorableAllocations = allocations.filter( | ||
| (allocation) => | ||
| currentMemberships.has(`${allocation.groupId}:${allocation.userId}`) && | ||
| currentMemberships.has(`${allocation.groupId}:${allocation.friendId}`), | ||
| ); | ||
|
|
||
| await Promise.all( | ||
| restorableAllocations.map((allocation) => | ||
| tx.groupBalance.upsert({ | ||
| where: { | ||
| groupId_currency_firendId_userId: { | ||
| groupId: allocation.groupId, | ||
| currency: allocation.currency, | ||
| userId: allocation.userId, | ||
| firendId: allocation.friendId, | ||
| }, | ||
| }, | ||
| create: { | ||
| groupId: allocation.groupId, | ||
| currency: allocation.currency, | ||
| userId: allocation.userId, | ||
| firendId: allocation.friendId, | ||
| amount: allocation.amount, | ||
| }, | ||
| update: { amount: { increment: allocation.amount } }, | ||
| }), | ||
| ), | ||
| ); | ||
| await tx.settlementAllocation.deleteMany({ where: { settlementExpenseId } }); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
deleteMany drops allocations that were never restored, losing both the money and the provenance.
restorableAllocations filters out allocations whose participants are no longer group members (or whose group was deleted), but Line 143 deletes every allocation for the expense. The result: the group balance stays zeroed, the overall friend balance is restored, and the record explaining the discrepancy is gone — so the group ledger can never be reconciled, even if the user rejoins. This contradicts the schema comment on groupId about deleting a group keeping settlement provenance.
Deleting only what was actually restored keeps the unresolved rows auditable (and reapplies them if membership is later regained).
🛡️ Proposed fix
await Promise.all(
restorableAllocations.map((allocation) =>
tx.groupBalance.upsert({
@@
),
);
- await tx.settlementAllocation.deleteMany({ where: { settlementExpenseId } });
+ await Promise.all(
+ restorableAllocations.map((allocation) =>
+ tx.settlementAllocation.delete({
+ where: {
+ settlementExpenseId_groupId_currency_friendId_userId: {
+ settlementExpenseId,
+ groupId: allocation.groupId,
+ currency: allocation.currency,
+ friendId: allocation.friendId,
+ userId: allocation.userId,
+ },
+ },
+ }),
+ ),
+ );Note this changes the expectation asserted in src/tests/balanceProjection.integration.test.ts Line 509 (detachedAllocations.length === 0).
📝 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.
| const restorableAllocations = allocations.filter( | |
| (allocation) => | |
| currentMemberships.has(`${allocation.groupId}:${allocation.userId}`) && | |
| currentMemberships.has(`${allocation.groupId}:${allocation.friendId}`), | |
| ); | |
| await Promise.all( | |
| restorableAllocations.map((allocation) => | |
| tx.groupBalance.upsert({ | |
| where: { | |
| groupId_currency_firendId_userId: { | |
| groupId: allocation.groupId, | |
| currency: allocation.currency, | |
| userId: allocation.userId, | |
| firendId: allocation.friendId, | |
| }, | |
| }, | |
| create: { | |
| groupId: allocation.groupId, | |
| currency: allocation.currency, | |
| userId: allocation.userId, | |
| firendId: allocation.friendId, | |
| amount: allocation.amount, | |
| }, | |
| update: { amount: { increment: allocation.amount } }, | |
| }), | |
| ), | |
| ); | |
| await tx.settlementAllocation.deleteMany({ where: { settlementExpenseId } }); | |
| const restorableAllocations = allocations.filter( | |
| (allocation) => | |
| currentMemberships.has(`${allocation.groupId}:${allocation.userId}`) && | |
| currentMemberships.has(`${allocation.groupId}:${allocation.friendId}`), | |
| ); | |
| await Promise.all( | |
| restorableAllocations.map((allocation) => | |
| tx.groupBalance.upsert({ | |
| where: { | |
| groupId_currency_firendId_userId: { | |
| groupId: allocation.groupId, | |
| currency: allocation.currency, | |
| userId: allocation.userId, | |
| firendId: allocation.friendId, | |
| }, | |
| }, | |
| create: { | |
| groupId: allocation.groupId, | |
| currency: allocation.currency, | |
| userId: allocation.userId, | |
| firendId: allocation.friendId, | |
| amount: allocation.amount, | |
| }, | |
| update: { amount: { increment: allocation.amount } }, | |
| }), | |
| ), | |
| ); | |
| await Promise.all( | |
| restorableAllocations.map((allocation) => | |
| tx.settlementAllocation.delete({ | |
| where: { | |
| settlementExpenseId_groupId_currency_friendId_userId: { | |
| settlementExpenseId, | |
| groupId: allocation.groupId, | |
| currency: allocation.currency, | |
| friendId: allocation.friendId, | |
| userId: allocation.userId, | |
| }, | |
| }, | |
| }), | |
| ), | |
| ); |
🤖 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/server/api/services/balanceProjection.ts` around lines 115 - 143, Change
the cleanup in the balance projection flow to delete only the settlement
allocations included in restorableAllocations, rather than every row matching
settlementExpenseId. Preserve allocations excluded by the membership filter so
their provenance remains available for later reconciliation, and update the
affected integration-test expectation accordingly.
| if (null === expense.groupId) { | ||
| await deletePersonalExpense(expenseId, deletedBy); | ||
| sendExpensePushNotification(expenseId).catch(console.error); | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Group deletions lost the deletedAt guard that the personal path just gained.
deletePersonalExpense re-reads the expense inside a serializable transaction and rejects an already-deleted expense (Line 303). The group branch below still reads expense outside any transaction at Line 523 and never checks expense.deletedAt, so two concurrent (or simply repeated) delete calls on a group expense reverse the balances twice and leave the group ledger permanently wrong.
Since the personal path is now hardened, this asymmetry is the weakest link in the delete flow. Moving the group branch onto runBalanceTransaction with the same re-read + deletedAt check would close it.
🤖 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/server/api/services/splitService.ts` around lines 538 - 543, The
group-expense deletion branch in the delete flow must gain the same concurrency
protection as deletePersonalExpense. Update the group path around
deletePersonalExpense to use runBalanceTransaction, re-read the expense inside
the serializable transaction, and reject or no-op when deletedAt is already set
before reversing balances or marking the expense deleted.
| ? Math.round((Math.abs(p.amount ?? 0) / amount) * 10000) / 100 | ||
| : Math.round((Math.abs(amount - (p.amount ?? 0)) / amount) * 10000) / 100, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Rounding percentages to 2 dp can leave the split screen unclosable after a split-type switch.
calculateParticipantSplit's PERCENTAGE branch gates on 100 - sum(splitShare) === 0 (Line 226) — an exact float comparison. The shares produced here are rounded independently per participant, so for a 3-way split of 10 the reconstruction gives 33.3 / 33.3 / 33.4, whose sum is subject to binary floating-point error and may not compare exactly equal to 100. The user then flips EQUAL → PERCENTAGE on a perfectly valid expense and gets stuck with the split screen refusing to close.
Two options: assign the residual to the payer so the shares sum to exactly 100 by construction, or relax the gate to an epsilon comparison as the EXACT branch already does.
The existing test only covers the 2-participant 50/50 case, which cannot expose this.
🐛 Suggested epsilon gate (companion fix, outside this range)
- canSplitScreenClosed =
- 100 - participants.reduce((acc, p) => acc + (p.splitShare ?? 0), 0) === 0;
+ canSplitScreenClosed =
+ Math.abs(100 - participants.reduce((acc, p) => acc + (p.splitShare ?? 0), 0)) < 0.01;🤖 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/store/addStore.ts` around lines 338 - 339, Update the percentage
calculation in calculateParticipantSplit so independently rounded participant
shares cannot leave the total invalid for a multi-participant split; assign the
rounding residual to the payer, ensuring the resulting shares sum to exactly 100
while preserving the existing percentage values for other participants.
| const storedParticipants = participants.map((participant) => ({ | ||
| userId: participant.userId, | ||
| amount: toInteger(participant.amount), | ||
| })); | ||
| const payer = storedParticipants.find((participant) => participant.userId === paidBy); | ||
| const uniqueParticipantIds = new Set(storedParticipants.map((participant) => participant.userId)); | ||
|
|
||
| if (!payer || uniqueParticipantIds.size !== storedParticipants.length) { | ||
| throw new Error('Expense participants must include the payer exactly once'); | ||
| } | ||
|
|
||
| const imbalance = storedParticipants.reduce((sum, participant) => sum + participant.amount, 0); | ||
| if (storedParticipants.length < Math.abs(imbalance)) { | ||
| throw new Error('Participant amounts must balance to zero'); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
NaN amounts bypass every validation in this function.
toInteger(NaN) is NaN, and each subsequent check silently accepts it:
storedParticipants.length < Math.abs(NaN)→false, so the balance guard does not fire.payer.amount -= NaN→NaN.- In
hasInvalidParticipant,NaN < 0,storedAmount < NaN, and0 < NaNare allfalse.
So a non-finite participant amount (or a non-finite amount) flows straight into expenseParticipant.create / balance increments inside the transaction, where it fails as an opaque Prisma error — or, worse, corrupts a balance. An explicit finiteness check up front is the cheap fix. (Note Infinity happens to throw the "must balance to zero" message, which is also misleading.)
🛡️ Proposed fix
) => {
+ if (!Number.isFinite(amount) || participants.some((p) => !Number.isFinite(p.amount))) {
+ throw new Error('Expense amounts must be finite numbers');
+ }
+
const storedParticipants = participants.map((participant) => ({
userId: participant.userId,
amount: toInteger(participant.amount),
}));📝 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.
| const storedParticipants = participants.map((participant) => ({ | |
| userId: participant.userId, | |
| amount: toInteger(participant.amount), | |
| })); | |
| const payer = storedParticipants.find((participant) => participant.userId === paidBy); | |
| const uniqueParticipantIds = new Set(storedParticipants.map((participant) => participant.userId)); | |
| if (!payer || uniqueParticipantIds.size !== storedParticipants.length) { | |
| throw new Error('Expense participants must include the payer exactly once'); | |
| } | |
| const imbalance = storedParticipants.reduce((sum, participant) => sum + participant.amount, 0); | |
| if (storedParticipants.length < Math.abs(imbalance)) { | |
| throw new Error('Participant amounts must balance to zero'); | |
| } | |
| if (!Number.isFinite(amount) || participants.some((p) => !Number.isFinite(p.amount))) { | |
| throw new Error('Expense amounts must be finite numbers'); | |
| } | |
| const storedParticipants = participants.map((participant) => ({ | |
| userId: participant.userId, | |
| amount: toInteger(participant.amount), | |
| })); | |
| const payer = storedParticipants.find((participant) => participant.userId === paidBy); | |
| const uniqueParticipantIds = new Set(storedParticipants.map((participant) => participant.userId)); | |
| if (!payer || uniqueParticipantIds.size !== storedParticipants.length) { | |
| throw new Error('Expense participants must include the payer exactly once'); | |
| } | |
| const imbalance = storedParticipants.reduce((sum, participant) => sum + participant.amount, 0); | |
| if (storedParticipants.length < Math.abs(imbalance)) { | |
| throw new Error('Participant amounts must balance to zero'); | |
| } |
🤖 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/utils/splits.ts` around lines 8 - 22, Update the participant
normalization and validation flow around storedParticipants to reject any
non-finite converted amount, including NaN and Infinity, before payer, balance,
or transaction processing occurs. Use the existing validation error pattern and
preserve the current handling of valid finite amounts.
1 participant
Summary
Settlement projection
An exact non-group
SETTLEMENTnow snapshots every affected directionalGroupBalancerow intoSettlementAllocationbefore clearing it. The allocation is linked to the settlement expense and contains the exact amount needed to reverse the projection.Balance-changing settlement operations, group expense creation/editing, group leave/delete, and friend deletion use serializable transactions with bounded retries. Group expense participants are revalidated inside the transaction so lifecycle checks cannot race allocation restoration or create inaccessible debt after a member leaves.
Other integrity fixes
SETTLEMENTfrom group spending totals.Verification
pnpm test: 23 unit tests pass; 2 PostgreSQL tests skip unlessTEST_DATABASE_URLis set.pnpm exec tsc --noEmit: pass.SKIP_ENV_VALIDATION=1 pnpm lint: pass with existing branch warnings only.SKIP_ENV_VALIDATION=1 pnpm build: pass.git diff --check: pass.No historical balance repair is included. The migration is additive and preserves legacy settlement provenance conservatively.
Summary by CodeRabbit