ENG-160: Add lender payable balance and integration tests#256
Merged
Connorbelez merged 7 commits intomainfrom Mar 23, 2026
Merged
ENG-160: Add lender payable balance and integration tests#256Connorbelez merged 7 commits intomainfrom
Connorbelez merged 7 commits intomainfrom
Conversation
Core lender payable journaling was already implemented via postSettlementAllocation (called from createDispersalEntries). This adds dedicated test coverage for the remaining gaps: - getLenderPayableBalance query edge cases (5 tests): no payables, single mortgage, multi-mortgage aggregate, net balance after partial payout, per-lender independence - Multi-lender dispersal E2E integration (7 tests): multi-lender entry creation, shared postingGroupId, unique idempotencyKeys, sum traceability (payables + fee = settlement), REQ-241 traceability fields, idempotent replay, zero-fee allocation Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Connorbelez has reached the 50-review limit for trial accounts. To continue receiving code reviews, upgrade your plan.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @Connorbelez, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
Contributor
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (13)
📝 WalkthroughWalkthroughAdded three Vitest test suites validating cash-ledger lender-payable balances, dispersal entry creation/idempotency, and servicing-fee recognition across multiple lenders and obligation types. Changes
Sequence Diagram(s)(omitted — changes are test additions and do not introduce new runtime control flow requiring visualization) Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
Adds new Vitest coverage around cash-ledger lender payables, focusing on multi-lender settlement allocation behavior and intended edge cases for lender payable balance retrieval.
Changes:
- Adds an E2E-style integration test suite that runs
createDispersalEntriesand inspects resulting cash-ledger journal entries/accounts. - Adds a new test suite intended to cover
getLenderPayableBalanceedge cases (no payables, multi-mortgage aggregation, partial payout netting, per-lender independence).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| convex/payments/cashLedger/tests/lenderPayableIntegration.test.ts | New integration tests validating multi-lender settlement allocation journaling (postingGroupId/idempotency/traceability/sum invariants). |
| convex/payments/cashLedger/tests/lenderPayableBalance.test.ts | New tests intended to validate lender payable balance behavior across several scenarios. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Core servicing fee recognition was already implemented within postSettlementAllocation (shared with ENG-160 lender payable flow). This adds dedicated test coverage for ENG-161 acceptance criteria: - SERVICING_FEE_RECOGNIZED entry posted per allocation - Fee shares postingGroupId with lender payable entries - Fee entry carries full traceability (mortgageId, obligationId, borrowerId) - SERVICING_REVENUE cumulative balance grows across multiple allocations - Zero-fee allocations (non-interest) produce no fee entry - Fee debits CONTROL:ALLOCATION and credits SERVICING_REVENUE Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@convex/payments/cashLedger/__tests__/lenderPayableBalance.test.ts`:
- Around line 38-45: The tests are rebuilding the expected value by querying
cash_ledger_accounts and summing getCashAccountBalance instead of exercising the
query under test; replace those manual aggregations with direct calls to the
query function getLenderPayableBalance (using the same lender id, e.g.,
seeded.lenderAId) and assert its return value, ensuring you still cover cases by
seeding accounts and comparing getLenderPayableBalance(...) to the known
expected BigInt values (or to the manual sum only when used to compute an
expected value once per test setup), and remove the redundant
ctx.db.query(...).withIndex("by_lender", ...) + reduce logic so the tests
exercise the index/filter/return logic in getLenderPayableBalance itself.
In `@convex/payments/cashLedger/__tests__/lenderPayableIntegration.test.ts`:
- Around line 164-179: The current test uses
ctx.db.query("cash_ledger_journal_entries").withIndex("by_posting_group", q =>
q.eq("postingGroupId", expectedGroupId)) then checks entries.every(e =>
e.postingGroupId === expectedGroupId), which is vacuous; instead assert exact
counts of entryTypes within that group so mis-grouped entries don't disappear
silently—after loading entries for expectedGroupId (using expectedGroupId and
obligationId), count occurrences of each entryType (e.g.,
"LENDER_PAYABLE_CREATED", "SERVICING_FEE_RECOGNIZED") from entryTypes and add
explicit expect assertions for the exact expected counts for the seeded
regular_interest case.
- Around line 118-121: The test is using a non-existent index "by_family" on
cash_ledger_accounts; change the query to use the existing composite index
"by_family_and_mortgage" and add the mortgage equality clause. Replace
withIndex("by_family", (q) => q.eq("family", "LENDER_PAYABLE")) by
withIndex("by_family_and_mortgage", (q) => q.eq("family",
"LENDER_PAYABLE").eq("mortgage", <existingMortgageIdentifier>)) where
<existingMortgageIdentifier> is the mortgage id/reference variable already in
scope in the test (e.g., mortgage.id or mortgageRef).
In `@convex/payments/cashLedger/__tests__/servicingFeeRecognition.test.ts`:
- Around line 324-340: The test currently allows a zero-balance
SERVICING_REVENUE account by collecting revenueAccounts and asserting their
total balance is 0n; change this to assert that no SERVICING_REVENUE account
exists for the mortgage instead. Locate the revenueAccounts query
(ctx.db.query("cash_ledger_accounts").withIndex("by_family_and_mortgage", q =>
q.eq("family", "SERVICING_REVENUE").eq("mortgageId", seeded.mortgageId))) and
replace the balance-summing/assertion using getCashAccountBalance with a strict
existence check (e.g., expect(revenueAccounts).toHaveLength(0) / assert
revenueAccounts.length === 0) so the test fails if any SERVICING_REVENUE account
is created eagerly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 32942469-f292-4a68-b8e1-023da1215f5e
📒 Files selected for processing (3)
convex/payments/cashLedger/__tests__/lenderPayableBalance.test.tsconvex/payments/cashLedger/__tests__/lenderPayableIntegration.test.tsconvex/payments/cashLedger/__tests__/servicingFeeRecognition.test.ts
Each test now calls internalGetLenderPayableBalance._handler() and asserts on the returned numeric value, ensuring the real query implementation is exercised. The existing direct account-level assertions are kept as a secondary check on the underlying state. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace weak inequality assertions with deterministic checks: - 60/40 split: verify each lender's balance is within 1-cent rounding tolerance of the exact proportional share (accounts for pro-rata largest-remainder rounding) - Posting group: assert exactly 3 entries (2 LENDER_PAYABLE_CREATED + 1 SERVICING_FEE_RECOGNIZED) instead of >= 2 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Use by_family_and_mortgage index (more precise) for LENDER_PAYABLE query - Assert no SERVICING_REVENUE account exists on zero-fee path instead of allowing a zero-balance account (catches eager account creation bugs) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
convex/payments/cashLedger/__tests__/servicingFeeRecognition.test.ts (2)
199-205: Consider strengthening theborrowerIdassertion for consistency.Other traceability fields are asserted against exact expected values, but
borrowerIdonly checks existence. Matching the pattern would catch regressions where the wrong borrower is associated with the fee entry.♻️ Suggested change
expect(feeEntry.mortgageId).toBe(seeded.mortgageId); expect(feeEntry.obligationId).toBe(obligationId); - expect(feeEntry.borrowerId).toBeDefined(); + expect(feeEntry.borrowerId).toBe(seeded.borrowerId); expect(feeEntry.postingGroupId).toBe(`allocation:${obligationId}`);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/payments/cashLedger/__tests__/servicingFeeRecognition.test.ts` around lines 199 - 205, The test currently only checks feeEntry.borrowerId is defined; change it to assert the exact expected borrower by replacing that check with expect(feeEntry.borrowerId).toBe(seeded.borrowerId) (or the appropriate seeded field) so the test verifies the feeEntry.borrowerId equals the seeded borrower value alongside the other strict assertions (references: feeEntry, seeded, borrowerId, obligationId).
283-286: Minor: Redundant assertion.If line 285 passes (
balanceAfterSecond === balanceAfterFirst * 2n), line 286 is necessarily true (givenbalanceAfterFirst > 0nfrom line 254). Consider removing it or keeping it only as documentation of intent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/payments/cashLedger/__tests__/servicingFeeRecognition.test.ts` around lines 283 - 286, The two assertions on cumulative balances are redundant: if expect(balanceAfterSecond).toBe(balanceAfterFirst * 2n) in the test already validates exact doubling, the subsequent expect(balanceAfterSecond).toBeGreaterThan(balanceAfterFirst) is unnecessary; remove the redundant expect call referencing balanceAfterSecond and balanceAfterFirst (or replace the equality check with the greater-than check if you intended only to assert an increase) in servicingFeeRecognition.test.ts to keep the test concise.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@convex/payments/cashLedger/__tests__/servicingFeeRecognition.test.ts`:
- Around line 199-205: The test currently only checks feeEntry.borrowerId is
defined; change it to assert the exact expected borrower by replacing that check
with expect(feeEntry.borrowerId).toBe(seeded.borrowerId) (or the appropriate
seeded field) so the test verifies the feeEntry.borrowerId equals the seeded
borrower value alongside the other strict assertions (references: feeEntry,
seeded, borrowerId, obligationId).
- Around line 283-286: The two assertions on cumulative balances are redundant:
if expect(balanceAfterSecond).toBe(balanceAfterFirst * 2n) in the test already
validates exact doubling, the subsequent
expect(balanceAfterSecond).toBeGreaterThan(balanceAfterFirst) is unnecessary;
remove the redundant expect call referencing balanceAfterSecond and
balanceAfterFirst (or replace the equality check with the greater-than check if
you intended only to assert an increase) in servicingFeeRecognition.test.ts to
keep the test concise.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c8dd6201-7785-4893-a4b5-bb5b7603a098
📒 Files selected for processing (3)
convex/payments/cashLedger/__tests__/lenderPayableBalance.test.tsconvex/payments/cashLedger/__tests__/lenderPayableIntegration.test.tsconvex/payments/cashLedger/__tests__/servicingFeeRecognition.test.ts
✅ Files skipped from review due to trivial changes (2)
- convex/payments/cashLedger/tests/lenderPayableBalance.test.ts
- convex/payments/cashLedger/tests/lenderPayableIntegration.test.ts
- seedUnappliedCashAccount patched cumulativeDebits instead of cumulativeCredits for the credit-normal UNAPPLIED_CASH family, giving it a negative balance and failing the balance guard - matchesSpec treated subaccount:undefined as "match any" instead of "match accounts with no subaccount", causing CONTROL to match CONTROL:ACCRUAL erroneously Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
import.meta.glob is a Vite-only API that crashes in Convex's V8 runtime. Moved it out of the shared testUtils.ts into each .test.ts consumer, passing modules as a parameter to createHarness(). This unblocks `bunx convex codegen` which was failing with "import.meta unsupported". Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This was referenced Mar 23, 2026
Merged
Merged
Merged
Connorbelez
added a commit
that referenced
this pull request
Apr 20, 2026
Core lender payable journaling was already implemented via postSettlementAllocation (called from createDispersalEntries). This adds dedicated test coverage for the remaining gaps: - getLenderPayableBalance query edge cases (5 tests): no payables, single mortgage, multi-mortgage aggregate, net balance after partial payout, per-lender independence - Multi-lender dispersal E2E integration (7 tests): multi-lender entry creation, shared postingGroupId, unique idempotencyKeys, sum traceability (payables + fee = settlement), REQ-241 traceability fields, idempotent replay, zero-fee allocation Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Tests** * Added unit tests covering lender payable balance computations (no-payable, single mortgage, multiple mortgages, payout effects, and per-lender isolation). * Added end-to-end integration tests for settlement allocation that validate lender payable creation, pro‑rata splitting, idempotency, single posting group behavior, unique idempotency keys, and full amount reconciliation. * Added tests for servicing fee recognition (fee creation, traceability, account directionality, zero‑fee handling, and cumulative balance behavior). <!-- end of auto-generated comment: release notes by coderabbit.ai -->
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
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.
Core lender payable journaling was already implemented via
postSettlementAllocation (called from createDispersalEntries).
This adds dedicated test coverage for the remaining gaps:
getLenderPayableBalance query edge cases (5 tests):
no payables, single mortgage, multi-mortgage aggregate,
net balance after partial payout, per-lender independence
Multi-lender dispersal E2E integration (7 tests):
multi-lender entry creation, shared postingGroupId,
unique idempotencyKeys, sum traceability (payables + fee = settlement),
REQ-241 traceability fields, idempotent replay, zero-fee allocation
Co-Authored-By: Claude Opus 4.6 (1M context) noreply@anthropic.com
Summary by CodeRabbit