ENG-174: Payout hold period + Biome updates#272
Merged
Connorbelez merged 4 commits intomainfrom Mar 25, 2026
Merged
Conversation
![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
📝 WalkthroughWalkthroughAdds UTC business-day utilities, a hold-period config for payment methods, persistence of Changes
Sequence DiagramsequenceDiagram
participant Client as Client/Caller
participant CreateDisp as createDispersalEntries
participant Collections as collectionPlanEntries (index)
participant Attempts as collectionAttempts
participant HoldPeriod as holdPeriod
participant Schema as dispersalEntries table
participant Query as getPayoutEligibleEntries
Client->>CreateDisp: createDispersalEntries(settledDate, [paymentMethod])
alt paymentMethod provided
CreateDisp->>HoldPeriod: getHoldPeriod(paymentMethod)
else resolve from collection
CreateDisp->>Collections: query by obligationId (planned|executing|completed)
Collections->>Attempts: fetch recent confirmed attempt by planEntryId
Attempts-->>CreateDisp: return method?
CreateDisp->>HoldPeriod: getHoldPeriod(resolvedMethod)
end
HoldPeriod-->>CreateDisp: holdBusinessDays
CreateDisp->>HoldPeriod: calculatePayoutEligibleDate(settledDate, method)
HoldPeriod-->>CreateDisp: payoutEligibleAfter (YYYY-MM-DD)
CreateDisp->>Schema: insert dispersalEntries with paymentMethod & payoutEligibleAfter
Schema-->>CreateDisp: persisted
Client->>Query: getPayoutEligibleEntries(asOfDate, lenderId?, limit?)
Query->>Schema: by_eligibility index lookup (status, payoutEligibleAfter ≤ asOfDate)
Schema-->>Query: eligible + legacy entries
Query->>Query: filter by lender, aggregate, paginate
Query-->>Client: return entries, totals, byLender
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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 docstrings
🧪 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 |
![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.
Pull request overview
This PR implements ENG-174 “Payout Hold Period Enforcement” across Convex dispersal entries (new business-day/hold-period utilities, schema + query/mutation updates) and also tweaks the Biome configuration include/exclude patterns.
Changes:
- Add
businessDaysutilities + tests andholdPeriodconfig + tests to compute payout eligibility dates. - Extend dispersal status validation/types, add
payoutEligibleAfter+paymentMethodfields and aby_eligibilityindex, and wire eligibility computation intocreateDispersalEntries. - Add
getPayoutEligibleEntriesadmin query for downstream payout scheduling, plus a Biome config update.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| specs/ENG-174/tasks.md | Master task checklist for ENG-174. |
| specs/ENG-174/chunks/manifest.md | Chunk manifest for ENG-174 plan/execution. |
| specs/ENG-174/chunks/chunk-02-schema-backend/tasks.md | Detailed backend/schema task breakdown. |
| specs/ENG-174/chunks/chunk-02-schema-backend/context.md | Context and current-state excerpts for chunk-02 changes. |
| specs/ENG-174/chunks/chunk-01-utilities/tasks.md | Detailed utilities/config + test plan for chunk-01. |
| specs/ENG-174/chunks/chunk-01-utilities/context.md | Design/context for business-day + hold-period utilities. |
| convex/schema.ts | Adds optional payoutEligibleAfter/paymentMethod fields and by_eligibility index to dispersalEntries. |
| convex/lib/businessDays.ts | New strict business-day utility helpers. |
| convex/lib/tests/businessDays.test.ts | Unit tests for businessDays utilities. |
| convex/dispersal/validators.ts | Extends dispersal status validator to a union of states. |
| convex/dispersal/types.ts | Extends DispersalEntry type with status union + new optional fields. |
| convex/dispersal/queries.ts | Adds getPayoutEligibleEntries admin query. |
| convex/dispersal/holdPeriod.ts | Adds hold-period lookup + payout-eligible date calculation. |
| convex/dispersal/createDispersalEntries.ts | Resolves payment method and stores paymentMethod/payoutEligibleAfter on new dispersal entries. |
| convex/dispersal/tests/holdPeriod.test.ts | Unit tests for hold-period behavior. |
| biome.json | Updates Biome file include/exclude patterns. |
💡 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: 1
🧹 Nitpick comments (2)
convex/dispersal/__tests__/holdPeriod.test.ts (1)
32-36: Consider adding a test for weekend input with manual method.The current test verifies that manual (0-day hold) returns the same date, but it doesn't cover the edge case where the input is a weekend. Adding a test would document the expected behavior:
it("manual: weekend input returns weekend (current behavior)", () => { // March 21 2026 is a Saturday expect(calculatePayoutEligibleDate("2026-03-21", "manual")).toBe( "2026-03-21" // or "2026-03-23" if normalized to Monday ); });This would clarify the intended behavior and help catch regressions if the edge case handling changes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/dispersal/__tests__/holdPeriod.test.ts` around lines 32 - 36, Add a unit test covering the weekend edge case for the manual hold method: in the existing holdPeriod.test.ts add a new it block (e.g., "manual: weekend input returns weekend") that calls calculatePayoutEligibleDate with a weekend date (for example "2026-03-21") and asserts the current expected behavior (returns the same date, "2026-03-21"); this documents the intended behavior and catches regressions if weekend normalization is later introduced.convex/dispersal/createDispersalEntries.ts (1)
73-74: Full table scan oncollectionPlanEntriesmay impact performance.The helper scans all
collectionPlanEntriesand checksobligationIds.includes(obligationId)in memory. This is O(n) where n is the total number of collection plan entries.Since
obligationIdsis an array field, it cannot be efficiently indexed. For now, this is acceptable given the fallback to"manual"if no match is found. However, if the table grows large, consider:
- Adding a denormalized lookup table (
collectionPlanEntry_obligations) mappingobligationId → planEntryId- Caching the resolved method on the obligation or attempt when it's known
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/dispersal/createDispersalEntries.ts` around lines 73 - 74, The current code in createDispersalEntries collects all collectionPlanEntries and does an in-memory scan (planEntries / collectionPlanEntries), causing a full-table scan; instead add a denormalized lookup and use it for lookup: introduce a new mapping table (e.g. collectionPlanEntry_obligations) keyed by obligationId → planEntryId, update all code paths that create/update collectionPlanEntries to insert/delete corresponding mapping rows, and change the lookup in createDispersalEntries to query collectionPlanEntry_obligations by obligationId (or read a cached resolvedMethod field on obligation/attempt if you prefer caching) rather than collecting the entire collectionPlanEntries table. Ensure writes keep the mapping in sync so the createDispersalEntries function can perform a single indexed query instead of an O(n) in-memory filter.
🤖 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/lib/businessDays.ts`:
- Around line 42-44: The early-return when days === 0 in
convex/lib/businessDays.ts returns startDate even if it's a weekend; update the
function so that when days === 0 it still normalizes startDate to the next
business day (reusing this module's existing business-day
normalization/advancement logic) instead of returning the raw startDate, so
payoutEligibleAfter never stores a weekend date.
---
Nitpick comments:
In `@convex/dispersal/__tests__/holdPeriod.test.ts`:
- Around line 32-36: Add a unit test covering the weekend edge case for the
manual hold method: in the existing holdPeriod.test.ts add a new it block (e.g.,
"manual: weekend input returns weekend") that calls calculatePayoutEligibleDate
with a weekend date (for example "2026-03-21") and asserts the current expected
behavior (returns the same date, "2026-03-21"); this documents the intended
behavior and catches regressions if weekend normalization is later introduced.
In `@convex/dispersal/createDispersalEntries.ts`:
- Around line 73-74: The current code in createDispersalEntries collects all
collectionPlanEntries and does an in-memory scan (planEntries /
collectionPlanEntries), causing a full-table scan; instead add a denormalized
lookup and use it for lookup: introduce a new mapping table (e.g.
collectionPlanEntry_obligations) keyed by obligationId → planEntryId, update all
code paths that create/update collectionPlanEntries to insert/delete
corresponding mapping rows, and change the lookup in createDispersalEntries to
query collectionPlanEntry_obligations by obligationId (or read a cached
resolvedMethod field on obligation/attempt if you prefer caching) rather than
collecting the entire collectionPlanEntries table. Ensure writes keep the
mapping in sync so the createDispersalEntries function can perform a single
indexed query instead of an O(n) in-memory filter.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6afc65ff-5f7f-41cb-a266-8d73aac49433
📒 Files selected for processing (16)
biome.jsonconvex/dispersal/__tests__/holdPeriod.test.tsconvex/dispersal/createDispersalEntries.tsconvex/dispersal/holdPeriod.tsconvex/dispersal/queries.tsconvex/dispersal/types.tsconvex/dispersal/validators.tsconvex/lib/__tests__/businessDays.test.tsconvex/lib/businessDays.tsconvex/schema.tsspecs/ENG-174/chunks/chunk-01-utilities/context.mdspecs/ENG-174/chunks/chunk-01-utilities/tasks.mdspecs/ENG-174/chunks/chunk-02-schema-backend/context.mdspecs/ENG-174/chunks/chunk-02-schema-backend/tasks.mdspecs/ENG-174/chunks/manifest.mdspecs/ENG-174/tasks.md
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/dispersal/createDispersalEntries.ts`:
- Around line 73-98: The loop over planEntries currently returns entry.method
immediately which can short-circuit and miss a newer confirmed attempt; change
the logic in createDispersalEntries.ts so you do NOT return entry.method inside
the for (const entry of planEntries) loop—instead, when iterating keep looking
for any confirmedWithMethod (from the collectionAttempts query and its
confirmedWithMethod computation) and if found return its method immediately;
otherwise store a fallbackMethod variable when entry.method exists and continue
scanning all entries; after the loop, if no confirmedWithMethod was found return
fallbackMethod (or undefined) so the search isn’t prematurely short-circuited by
entry.method.
In `@convex/lib/businessDays.ts`:
- Around line 80-86: The function countBusinessDaysBetween currently compares
the raw input strings (start >= end) before validating/parsing them, allowing
malformed dates to bypass validation; change it to call parseUTCDate for both
start and end up front (use parseUTCDate(start) and parseUTCDate(end)), validate
the returned Date objects (or check getTime() is finite), then perform the
comparison using the parsed dates/timestamps (e.g., startDate.getTime() >=
endDate.getTime()) and only then return 0 for non-ascending ranges; update
references in the function (countBusinessDaysBetween, parseUTCDate, endMs)
accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0860cb24-12a8-4305-a307-a42b766b231d
📒 Files selected for processing (6)
convex/dispersal/__tests__/holdPeriod.test.tsconvex/dispersal/createDispersalEntries.tsconvex/dispersal/queries.tsconvex/engine/effects/__tests__/obligationAccrual.integration.test.tsconvex/lib/__tests__/businessDays.test.tsconvex/lib/businessDays.ts
💤 Files with no reviewable changes (1)
- convex/engine/effects/tests/obligationAccrual.integration.test.ts
✅ Files skipped from review due to trivial changes (1)
- convex/dispersal/tests/holdPeriod.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- convex/lib/tests/businessDays.test.ts
…lution, business days - resolvePaymentMethodFromCollection: query plan entries via by_status; use confirmed collection attempts with latest _creationTime - getPayoutEligibleEntries: validate asOfDate; use by_eligibility lte for held entries; legacy pending without hold separate; add page aggregates - addBusinessDays(0): normalize weekend start to next Monday - tests: businessDays, holdPeriod; remove dead import breaking typecheck Made-with: Cursor
Connorbelez
force-pushed
the
Connorbelez/eng-174-payout-hold
branch
from
36d8f1a to
0de2f17
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
convex/dispersal/queries.ts (1)
323-383: Query correctly handles legacy entries, but consider documenting the performance characteristics.The implementation correctly splits into two paths:
- Indexed path for entries with
payoutEligibleAfter <= asOfDate- Legacy fallback for entries without
payoutEligibleAfterA few notes on the current approach:
- The legacy query (lines 354-357) uses
by_eligibilitywith onlystatus="pending", which scans all pending entries then filters in-memory. This is necessary because Convex indexes don't support querying for "field is undefined."- The in-memory
lenderIdfilter (lines 344-346, 360-362) is applied after.collect(), which fetches all matching entries before filtering.These are reasonable tradeoffs given index constraints. As the table grows, consider adding a migration to backfill
payoutEligibleAfteron legacy entries to eliminate the dual-query pattern.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/dispersal/queries.ts` around lines 323 - 383, The getPayoutEligibleEntries query splits into an indexed path (pendingPastHold) and a legacy fallback (pendingAll -> eligibleLegacy) which scans all pending entries and applies lenderId filtering in-memory; update the function's docstring or add a top-of-file comment above getPayoutEligibleEntries explaining this performance characteristic (that pendingAll uses the by_eligibility index with only status="pending" and therefore collects all pending rows), note that lenderId filtering happens after .collect(), and add a short recommendation to run a migration to backfill payoutEligibleAfter to avoid the dual-query pattern as table size grows.convex/dispersal/createDispersalEntries.ts (1)
66-118: Payment method resolution is correct but may be costly at scale.The implementation correctly addresses the prior short-circuit issue by scanning all plan entries before returning. However, this approach:
- Queries all
collectionPlanEntrieswith each of three statuses, then filters in-memory for matchingobligationIds(no index exists on the array field)- For each matching entry, issues a separate query to
collectionAttemptsThis is acceptable for current cardinality but worth noting for future optimization if obligations start having many plan entries.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/dispersal/createDispersalEntries.ts` around lines 66 - 118, The current resolvePaymentMethodFromCollection scans all plan entries for three statuses then filters by obligationIds and issues a query per matching entry for collectionAttempts, which is expensive; change the collectionPlanEntries fetch to query the index that checks the obligationIds array (e.g., withIndex "by_obligation" or a new index that .contains("obligationIds", obligationId)) and include status filtering so you only retrieve relevant entries, then collect all matching planEntry._id values and issue a single batched collectionAttempts query (using the "by_plan_entry" index with an IN or .in on planEntryId over the planEntryIds set) to find the latest confirmed attempt, keeping the same selection logic for bestConfirmed and bestPlanMethod in resolvePaymentMethodFromCollection.
🤖 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/dispersal/createDispersalEntries.ts`:
- Around line 66-118: The current resolvePaymentMethodFromCollection scans all
plan entries for three statuses then filters by obligationIds and issues a query
per matching entry for collectionAttempts, which is expensive; change the
collectionPlanEntries fetch to query the index that checks the obligationIds
array (e.g., withIndex "by_obligation" or a new index that
.contains("obligationIds", obligationId)) and include status filtering so you
only retrieve relevant entries, then collect all matching planEntry._id values
and issue a single batched collectionAttempts query (using the "by_plan_entry"
index with an IN or .in on planEntryId over the planEntryIds set) to find the
latest confirmed attempt, keeping the same selection logic for bestConfirmed and
bestPlanMethod in resolvePaymentMethodFromCollection.
In `@convex/dispersal/queries.ts`:
- Around line 323-383: The getPayoutEligibleEntries query splits into an indexed
path (pendingPastHold) and a legacy fallback (pendingAll -> eligibleLegacy)
which scans all pending entries and applies lenderId filtering in-memory; update
the function's docstring or add a top-of-file comment above
getPayoutEligibleEntries explaining this performance characteristic (that
pendingAll uses the by_eligibility index with only status="pending" and
therefore collects all pending rows), note that lenderId filtering happens after
.collect(), and add a short recommendation to run a migration to backfill
payoutEligibleAfter to avoid the dual-query pattern as table size grows.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fee1a405-0d24-4505-96be-348d4f9c5470
📒 Files selected for processing (17)
biome.jsonconvex/dispersal/__tests__/holdPeriod.test.tsconvex/dispersal/createDispersalEntries.tsconvex/dispersal/holdPeriod.tsconvex/dispersal/queries.tsconvex/dispersal/types.tsconvex/dispersal/validators.tsconvex/engine/effects/__tests__/obligationAccrual.integration.test.tsconvex/lib/__tests__/businessDays.test.tsconvex/lib/businessDays.tsconvex/schema.tsspecs/ENG-174/chunks/chunk-01-utilities/context.mdspecs/ENG-174/chunks/chunk-01-utilities/tasks.mdspecs/ENG-174/chunks/chunk-02-schema-backend/context.mdspecs/ENG-174/chunks/chunk-02-schema-backend/tasks.mdspecs/ENG-174/chunks/manifest.mdspecs/ENG-174/tasks.md
💤 Files with no reviewable changes (1)
- convex/engine/effects/tests/obligationAccrual.integration.test.ts
✅ Files skipped from review due to trivial changes (8)
- biome.json
- convex/dispersal/types.ts
- specs/ENG-174/tasks.md
- convex/dispersal/tests/holdPeriod.test.ts
- convex/dispersal/holdPeriod.ts
- specs/ENG-174/chunks/chunk-02-schema-backend/context.md
- convex/lib/tests/businessDays.test.ts
- specs/ENG-174/chunks/chunk-01-utilities/context.md
🚧 Files skipped from review as they are similar to previous changes (3)
- convex/dispersal/validators.ts
- specs/ENG-174/chunks/manifest.md
- convex/lib/businessDays.ts
This was referenced Mar 26, 2026
Merged
Connorbelez
added a commit
that referenced
this pull request
Apr 20, 2026
## ENG-174: Payout hold enforcement Implements payment-method-based payout hold periods (dispersal schema, `createDispersalEntries`, hold-period utilities, `getPayoutEligibleEntries`, and tests) **and** includes Biome configuration updates in `biome.json`. ### Scope - Hold period / `payoutEligibleAfter` / eligibility query - Biome lint/format rule tweaks <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Track payment method and payout-eligible date on dispersal entries * Automatic hold-period calculation per payment method (including default behavior) * New query to list entries eligible for payout as of a given business date * Dispersal status expanded to include "eligible", "disbursed", and "failed" * **Tests** * Added business-day utilities tests (validation, add/count logic, edge cases) * Added hold-period behavior tests * **Documentation** * Added specs and task docs for payout hold-period enforcement and schema changes <!-- 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.
ENG-174: Payout hold enforcement
Implements payment-method-based payout hold periods (dispersal schema,
createDispersalEntries, hold-period utilities,getPayoutEligibleEntries, and tests) and includes Biome configuration updates inbiome.json.Scope
payoutEligibleAfter/ eligibility querySummary by CodeRabbit
New Features
Tests
Documentation