ENG-50: implement confirmation effects (commitReservation, prorateAccrual, updatePaymentSchedule)#115
Merged
Connorbelez merged 3 commits intomainfrom Mar 18, 2026
Merged
Conversation
…rual, updatePaymentSchedule) Implements the three deal-closing confirmation effects that fire when a deal transitions to confirmed (fundsTransfer.onDone → confirmed): - commitReservation: commits the pending ledger reservation for the deal - prorateAccrualBetweenOwners: calculates and writes seller/buyer prorate credit entries based on closing date relative to payment schedule - updatePaymentSchedule: creates a dealReroute record to redirect future dispersals from seller to buyer for the transferred share Adds schema tables (prorateEntries, dealReroutes) and supporting CRUD modules (mortgages/queries, obligations/queries, prorateEntries/*, dealReroutes/*). Replaces placeholder registry entries with real effect handlers. Tests: 9 pass, 6 skipped (convex-test action ctx limitation). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![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
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughImplements three deal-closing confirmation effects (commitReservation, prorateAccrualBetweenOwners, updatePaymentSchedule) with supporting database schema, data access queries/mutations, and comprehensive test coverage. Updates effect registry to wire new implementations. Includes gap analysis documentation against PRD requirements. Changes
Sequence Diagram(s)sequenceDiagram
participant Event as Deal Confirmation Event
participant Engine as Effect Engine
participant DealDB as Deal DB
participant MortDB as Mortgage DB
participant ObligDB as Obligation DB
participant Ledger as Ledger Service
participant Log as Logger
Event->>Engine: trigger dealClosing effects
rect rgb(100, 150, 200, 0.5)
Note over Engine,Log: commitReservation Effect
Engine->>DealDB: fetch deal by ID
DealDB-->>Engine: deal + reservationId
alt reservationId missing
Engine->>Log: graceful exit, no action
else reservationId present
Engine->>Ledger: commitReservation(reservationId, effectiveDate)
Ledger-->>Engine: success / error
Engine->>Log: log outcome
end
end
rect rgb(150, 100, 200, 0.5)
Note over Engine,Log: prorateAccrualBetweenOwners Effect
Engine->>DealDB: fetch deal by ID
DealDB-->>Engine: deal + closingDate
alt deal missing
Engine->>Log: throw DEAL_NOT_FOUND
else closingDate missing
Engine->>Log: graceful exit
else idempotent check passes
Engine->>ObligDB: getSettledBeforeDate(mortgageId)
ObligDB-->>Engine: lastPaymentDate
Engine->>ObligDB: getFirstAfterDate(mortgageId)
ObligDB-->>Engine: nextPaymentDate
Engine->>MortDB: fetch mortgage (interest, principal)
MortDB-->>Engine: mortgage data
Engine->>Engine: calculate dailyRate, sellerDays, buyerDays
Engine->>DealDB: insertProrateEntries (seller + buyer splits)
DealDB-->>Engine: entry IDs
Engine->>Log: log proration summary
end
end
rect rgb(200, 150, 100, 0.5)
Note over Engine,Log: updatePaymentSchedule Effect
Engine->>DealDB: fetch deal by ID
DealDB-->>Engine: deal + closingDate
alt deal missing
Engine->>Log: throw DEAL_NOT_FOUND
else closingDate missing
Engine->>Log: graceful exit
else idempotent check (no existing reroute)
Engine->>DealDB: insertDealReroutes(dealId, fromOwner, toOwner, effectiveAfterDate)
DealDB-->>Engine: reroute ID
Engine->>Log: log creation success
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
Owner
Author
This stack of pull requests is managed by Graphite. Learn more about stacking. |
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.
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
Owner
Author
|
@greptile review |
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.
There was a problem hiding this comment.
Pull request overview
Implements the three confirmation-time effects for deal closing (commit reservation, prorate accrual credits, and reroute future payment dispersals) plus the supporting storage and internal CRUD needed to persist and query the resulting records.
Changes:
- Add new persistence tables for prorate credits and deal payment reroutes, plus internal CRUD modules.
- Implement
commitReservation,prorateAccrualBetweenOwners, andupdatePaymentScheduleeffects and wire them into the effects registry. - Add a Vitest suite covering error/idempotency paths for the new effects (with several happy-path tests currently skipped).
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| specs/ENG-50/tasks.md | Task tracking checklist for ENG-50 implementation/testing. |
| specs/ENG-50/gap-analysis.md | Documents PRD vs implementation deviations and follow-ups. |
| convex/schema.ts | Adds prorateEntries + dealReroutes tables and indexes. |
| convex/prorateEntries/queries.ts | Internal query to fetch prorate entries by deal for idempotency. |
| convex/prorateEntries/mutations.ts | Internal mutation to atomically insert prorate entries. |
| convex/dealReroutes/queries.ts | Internal query to fetch reroute record by deal for idempotency. |
| convex/dealReroutes/mutations.ts | Internal mutation to insert a reroute record. |
| convex/obligations/queries.ts | Internal queries to find last-settled and next obligation around closing date. |
| convex/mortgages/queries.ts | Internal mortgage fetch helper for effects. |
| convex/engine/effects/dealClosingProrate.ts | Implements prorate accrual effect writing prorateEntries. |
| convex/engine/effects/dealClosingPayments.ts | Implements reroute effect writing dealReroutes. |
| convex/engine/effects/dealClosing.ts | Adds commitReservation effect implementation. |
| convex/engine/effects/registry.ts | Replaces placeholders with real effect handlers. |
| convex/deals/tests/effects.test.ts | Adds unit tests for confirmation effects (some skipped due to harness limitations). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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: 5
🤖 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/engine/effects/dealClosingPayments.ts`:
- Around line 58-78: The catch block around the
ctx.runMutation(internal.dealReroutes.mutations.insert, ...) in
updatePaymentSchedule is swallowing failures; instead of just logging, re-throw
(or propagate) the error so the caller knows the reroute write failed. Update
the catch to log the error and then throw the caught error (or remove the
try/catch and let the exception bubble) so that a confirmed deal cannot be left
without the reroute side effect.
- Around line 37-47: The current idempotency pattern uses ctx.runQuery with
internal.dealReroutes.queries.getByDealId followed by a separate insert, which
allows races; make the check+insert atomic by moving dedupe into a single
mutation or enforcing a unique constraint. Replace the read-then-write flow
(ctx.runQuery + subsequent insert into internal.dealReroutes) with a single
atomic operation: either implement a createIfNotExists-style mutation on
internal.dealReroutes that checks dealId and returns existing row if present, or
add a unique constraint on dealId and catch the duplicate-key error on insert to
treat it as a no-op; apply the same change for the other occurrence around lines
59-67 (same symbols: ctx.runQuery, internal.dealReroutes.*) so concurrent
executions cannot create duplicate reroutes.
In `@convex/engine/effects/dealClosingProrate.ts`:
- Around line 175-195: The catch block after calling
ctx.runMutation(internal.prorateEntries.mutations.insertProrateEntries, {
entries }) only logs the error and returns, which hides failures; update the
catch in dealClosingProrate.ts to log the error (keeping the existing
console.error message referencing dealId) and then rethrow the caught error (or
throw a new Error with the original error message) so the caller can observe and
handle the prorate write failure for entries/sellerAmount/buyerAmount.
- Around line 47-57: The idempotency check using
ctx.runQuery(internal.prorateEntries.queries.getByDealId, { dealId }) is
currently outside the transaction that inserts prorate entries, allowing race
conditions; move this existence check into the same mutation/transaction that
performs the insert (or replace the logic with a conditional insert/upsert
within the transaction). Concretely, update the mutation that creates prorate
entries so it first queries getByDealId (or does a SELECT ... FOR UPDATE / an
atomic upsert) inside the same ctx.runMutation/transactional block that calls
the insert logic for dealId, and remove the standalone pre-check (also apply the
same change for the similar check at the second occurrence around lines
referenced 176-179). Ensure you use the same transaction context so only one
concurrent run can create entries for a given dealId.
In `@convex/mortgages/queries.ts`:
- Around line 8-17: getInternalMortgage currently throws
ConvexError("MORTGAGE_NOT_FOUND") when ctx.db.get(mortgageId) returns nothing,
but callers (e.g., the logic in dealClosingProrate.ts) expect a nullable result;
change the handler in getInternalMortgage to return null instead of throwing
when mortgage is falsy (mirroring getInternalDeal), remove the ConvexError
throw, and ensure any type annotations or callers rely on a nullable return
rather than catching an exception.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ef73d027-9395-44b1-a95d-56f3bca0b0f7
📒 Files selected for processing (14)
convex/dealReroutes/mutations.tsconvex/dealReroutes/queries.tsconvex/deals/__tests__/effects.test.tsconvex/engine/effects/dealClosing.tsconvex/engine/effects/dealClosingPayments.tsconvex/engine/effects/dealClosingProrate.tsconvex/engine/effects/registry.tsconvex/mortgages/queries.tsconvex/obligations/queries.tsconvex/prorateEntries/mutations.tsconvex/prorateEntries/queries.tsconvex/schema.tsspecs/ENG-50/gap-analysis.mdspecs/ENG-50/tasks.md
- mortgages/queries: return null instead of throwing (match caller pattern) - obligations/queries: use index range constraints + .first() instead of collect+sort (avoids full-table scan) - dealClosingPayments, dealClosingProrate: re-throw errors after logging so failures propagate to Convex scheduler for retry Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This was referenced Mar 18, 2026
Connorbelez
added a commit
that referenced
this pull request
Apr 20, 2026
…rual, updatePaymentSchedule) (#115) Implements the three deal-closing confirmation effects that fire when a deal transitions to confirmed (fundsTransfer.onDone → confirmed): - commitReservation: commits the pending ledger reservation for the deal - prorateAccrualBetweenOwners: calculates and writes seller/buyer prorate credit entries based on closing date relative to payment schedule - updatePaymentSchedule: creates a dealReroute record to redirect future dispersals from seller to buyer for the transferred share Adds schema tables (prorateEntries, dealReroutes) and supporting CRUD modules (mortgages/queries, obligations/queries, prorateEntries/*, dealReroutes/*). Replaces placeholder registry entries with real effect handlers. Tests: 9 pass, 6 skipped (convex-test action ctx limitation). 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 ## Release Notes * **New Features** * Implemented complete deal closing workflow with three core operations: reservation commitment to lock funds, automated interest proration calculation between seller and buyer, and payment routing configuration for transferred ownership shares. * Added comprehensive database support for tracking prorate entries and payment reroutes. * **Tests** * Added extensive test coverage for deal closing effects including edge cases and idempotency validation. <!-- 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.
Implements the three deal-closing confirmation effects that fire when a deal
transitions to confirmed (fundsTransfer.onDone → confirmed):
credit entries based on closing date relative to payment schedule
dispersals from seller to buyer for the transferred share
Adds schema tables (prorateEntries, dealReroutes) and supporting CRUD modules
(mortgages/queries, obligations/queries, prorateEntries/, dealReroutes/).
Replaces placeholder registry entries with real effect handlers.
Tests: 9 pass, 6 skipped (convex-test action ctx limitation).
Co-Authored-By: Claude Opus 4.6 (1M context) noreply@anthropic.com
Summary by CodeRabbit
Release Notes
New Features
Tests
Documentation