ENG-56: implement PaymentMethod interface, ManualPaymentMethod, MockPADMethod, and registry#119
Conversation
…ADMethod, and registry Add pluggable payment rail abstractions using the Strategy pattern with dependency-injected scheduling for testability in Convex's stateless V8 isolates. - PaymentMethod interface with typed InitiateParams/Result, Confirm/Cancel/StatusResult - ManualPaymentMethod: immediate confirmation, no external API - MockPADMethod: simulated PAD with configurable delay/failure and DI scheduler - Two-tier registry: simple getPaymentMethod() + full DI createPaymentMethodRegistry() - 19 tests covering all implementations, config overrides, and ConvexError on unknown method Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughIntroduces a payment method subsystem with a Changes
Sequence DiagramsequenceDiagram
participant Client
participant Registry
participant PaymentMethod
participant Scheduler
participant Database
Client->>Registry: getPaymentMethod("mock_pad")<br/>with scheduleSettlement
Registry->>Registry: buildMethod("mock_pad")
Registry-->>Client: MockPADMethod instance
Client->>PaymentMethod: initiate(params)
PaymentMethod->>PaymentMethod: generate providerRef<br/>calculate shouldFail
PaymentMethod->>Scheduler: scheduleSettlement(delayMs, {providerRef, shouldFail, planEntryId})
Scheduler-->>PaymentMethod: scheduled
PaymentMethod-->>Client: {providerRef, status: "pending"}
Client->>PaymentMethod: getStatus(providerRef)
PaymentMethod->>Database: lookup pending payment
Database-->>PaymentMethod: payment record
PaymentMethod-->>Client: {status: "pending", providerData}
Scheduler->>Database: trigger settlement based on shouldFail
Database-->>Scheduler: settlement complete or failed
Client->>PaymentMethod: confirm(providerRef)
PaymentMethod-->>Client: {settledAt, providerRef}
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes The review involves multiple cohorts of changes: new interface definitions with clear contracts, two payment method implementations with configuration validation and DI patterns, a registry with error handling, and a detailed test suite covering nominal paths and edge cases. The logic density is moderate (validation, config merging, error handling), and the changes span multiple related files requiring consistent understanding of the pattern. Configuration validation and DI injection add some complexity but follow standard patterns. Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![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
This stack of pull requests is managed by Graphite. Learn more about stacking. |
![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
Introduces a pluggable payment-rail abstraction under convex/payments/ using a PaymentMethod strategy interface, with concrete manual and mock_pad implementations and a small registry API (plus a Vitest suite) to support downstream payment scheduling work (ENG-63).
Changes:
- Added
PaymentMethodinterface with typed params/results. - Implemented
ManualPaymentMethod(immediate confirmation) andMockPADMethod(pending + injected scheduler). - Added a two-tier method registry (
getPaymentMethod+ DI-friendlycreatePaymentMethodRegistry) and a dedicated test suite.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| specs/ENG-56/tasks.md | Work plan/checklist for ENG-56. |
| specs/ENG-56/chunks/manifest.md | Chunk manifest for implementation/testing phases. |
| specs/ENG-56/chunks/chunk-01-interface-and-implementations/tasks.md | Chunk 1 task breakdown. |
| specs/ENG-56/chunks/chunk-01-interface-and-implementations/context.md | Chunk 1 design/acceptance context. |
| specs/ENG-56/chunks/chunk-02-tests/tasks.md | Chunk 2 task breakdown. |
| specs/ENG-56/chunks/chunk-02-tests/context.md | Chunk 2 testing/quality gate context. |
| convex/payments/methods/interface.ts | Defines PaymentMethod and typed params/results. |
| convex/payments/methods/manual.ts | Manual rail implementation (immediate confirmation). |
| convex/payments/methods/mockPAD.ts | Mock PAD rail implementation with DI scheduler and config. |
| convex/payments/methods/registry.ts | Two-tier lookup + DI registry factory; unknown method errors. |
| convex/payments/tests/methods.test.ts | Vitest coverage for both methods + registry behaviors. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…niqueness, test coverage - Add runtime validation in MockPADMethod constructor for failureRate [0,1] and delayMs >= 0 - Replace Date.now() with crypto.randomUUID() in providerRef generation for both methods - Add getStatus() test for MockPADMethod and constructor validation tests - Use afterEach(vi.restoreAllMocks) instead of manual cleanup in test blocks 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}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
convex/payments/methods/registry.ts (1)
67-73: Consider failing fast formock_padin the simple registry tier.Returning a scheduler-dependent method from
getPaymentMethod()defers failure toinitiate(). Throwing immediately for"mock_pad"in this path would make misuse more obvious.♻️ Proposed fail-fast guard
export function getPaymentMethod(method: string): PaymentMethod { + if (method === "mock_pad") { + throw new ConvexError( + 'Payment method "mock_pad" requires scheduler injection. ' + + "Use createPaymentMethodRegistry()." + ); + } return buildMethod(method, noopScheduler); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/payments/methods/registry.ts` around lines 67 - 73, getPaymentMethod currently always returns buildMethod(method, noopScheduler), which defers scheduler-dependent errors until initiate(); add a fail-fast guard inside getPaymentMethod to check if method === "mock_pad" and throw a clear Error immediately instead of returning a noop-scheduled method. Locate getPaymentMethod and modify it to throw for "mock_pad" (mentioning the problematic method name) while keeping other paths calling buildMethod(method, noopScheduler); reference buildMethod, noopScheduler and initiate in the error message to guide callers.convex/payments/methods/interface.ts (1)
16-20: Use ConvexId<"...">types for entity references inInitiateParams.
borrowerIdandmortgageIdare currently modeled as plainstring, which loses table-level type safety at call sites. These should be typed asId<"borrowers">andId<"mortgages">respectively to match the schema and align with how references are typed elsewhere in the codebase.For
planEntryId, the corresponding table is not yet defined in the schema; type it asId<"...">once the exact table name is confirmed.♻️ Proposed type-safety refactor
+import type { Id } from "../../_generated/dataModel"; + export interface InitiateParams { /** Amount in cents */ amount: number; - borrowerId: string; + borrowerId: Id<"borrowers">; metadata?: Record<string, unknown>; method: string; - mortgageId: string; - planEntryId: string; + mortgageId: Id<"mortgages">; + planEntryId: string; // Replace with Id<"..."> once exact table name is confirmed. }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/payments/methods/interface.ts` around lines 16 - 20, Update the InitiateParams interface to use Convex's Id generic types for entity references: change borrowerId from string to Id<"borrowers"> and mortgageId from string to Id<"mortgages"> (and set planEntryId to Id<"..."> once the exact table name is known); import the Id type from Convex where this interface is declared so type-checking is consistent with other references in the codebase (look for the InitiateParams interface in this file to apply the changes).convex/payments/__tests__/methods.test.ts (1)
25-31: Use method-specific fixtures to avoid semantic mismatch in MockPAD tests.
sampleParamsis fixed tomethod: "manual"but is also used inMockPADMethodtests. This can hide regressions if method-specific validation is added later. Consider splitting fixtures (manualParamsandmockPadParams) and use"mock_pad"in the MockPAD suite.♻️ Suggested fixture split
-const sampleParams: InitiateParams = { +const manualParams: InitiateParams = { amount: 100_000, // $1000 in cents mortgageId: "mortgage_123", borrowerId: "borrower_456", planEntryId: "entry_789", method: "manual", }; + +const mockPadParams: InitiateParams = { + ...manualParams, + method: "mock_pad", +};Then use
manualParamsinManualPaymentMethodtests andmockPadParamsinMockPADMethodtests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/payments/__tests__/methods.test.ts` around lines 25 - 31, The current shared fixture sampleParams (type InitiateParams) uses method: "manual" but is also used by MockPADMethod tests; split into two explicit fixtures (e.g., manualParams and mockPadParams) where manualParams keeps method: "manual" and mockPadParams sets method: "mock_pad" (and otherwise mirrors required fields like amount, mortgageId, borrowerId, planEntryId); update tests referencing sampleParams to use manualParams in ManualPaymentMethod suites and mockPadParams in MockPADMethod suites to avoid semantic mismatches and catch method-specific validation regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@specs/ENG-56/chunks/chunk-01-interface-and-implementations/context.md`:
- Line 127: The spec text wrongly states that the `convex/payments/` directory
does not exist yet; update or remove that outdated sentence referencing
`convex/payments/` in the Markdown so the document matches the implementation
introduced in this PR, ensuring the spec no longer claims the directory is
missing.
In `@specs/ENG-56/chunks/chunk-02-tests/context.md`:
- Line 76: The documented error string for getPaymentMethod is out of sync with
the runtime: the registry currently throws ConvexError("Unknown payment method:
\"unknown\"") (with quotes). Update the spec line in context.md so the example
matches the implementation (e.g., `getPaymentMethod("unknown")` throws
`ConvexError("Unknown payment method: \"unknown\"")`), or alternatively change
the registry's thrown message in the getPaymentMethod/registry code to remove
the quotes—pick one consistent approach and make sure the example and the thrown
message match exactly.
In `@specs/ENG-56/chunks/manifest.md`:
- Around line 5-6: The manifest rows for chunk-01-interface-and-implementations
and chunk-02-tests are still marked "pending" but the corresponding tasks are
completed; update the manifest entries for
chunk-01-interface-and-implementations and chunk-02-tests to the correct
completed status to match the tasks document and ensure the manifest and tasks
are consistent (also scan for any other ENG-56 chunk rows and align their status
if needed).
---
Nitpick comments:
In `@convex/payments/__tests__/methods.test.ts`:
- Around line 25-31: The current shared fixture sampleParams (type
InitiateParams) uses method: "manual" but is also used by MockPADMethod tests;
split into two explicit fixtures (e.g., manualParams and mockPadParams) where
manualParams keeps method: "manual" and mockPadParams sets method: "mock_pad"
(and otherwise mirrors required fields like amount, mortgageId, borrowerId,
planEntryId); update tests referencing sampleParams to use manualParams in
ManualPaymentMethod suites and mockPadParams in MockPADMethod suites to avoid
semantic mismatches and catch method-specific validation regressions.
In `@convex/payments/methods/interface.ts`:
- Around line 16-20: Update the InitiateParams interface to use Convex's Id
generic types for entity references: change borrowerId from string to
Id<"borrowers"> and mortgageId from string to Id<"mortgages"> (and set
planEntryId to Id<"..."> once the exact table name is known); import the Id type
from Convex where this interface is declared so type-checking is consistent with
other references in the codebase (look for the InitiateParams interface in this
file to apply the changes).
In `@convex/payments/methods/registry.ts`:
- Around line 67-73: getPaymentMethod currently always returns
buildMethod(method, noopScheduler), which defers scheduler-dependent errors
until initiate(); add a fail-fast guard inside getPaymentMethod to check if
method === "mock_pad" and throw a clear Error immediately instead of returning a
noop-scheduled method. Locate getPaymentMethod and modify it to throw for
"mock_pad" (mentioning the problematic method name) while keeping other paths
calling buildMethod(method, noopScheduler); reference buildMethod, noopScheduler
and initiate in the error message to guide callers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f844e87e-da93-4dd6-a423-20cfbb11804a
📒 Files selected for processing (11)
convex/payments/__tests__/methods.test.tsconvex/payments/methods/interface.tsconvex/payments/methods/manual.tsconvex/payments/methods/mockPAD.tsconvex/payments/methods/registry.tsspecs/ENG-56/chunks/chunk-01-interface-and-implementations/context.mdspecs/ENG-56/chunks/chunk-01-interface-and-implementations/tasks.mdspecs/ENG-56/chunks/chunk-02-tests/context.mdspecs/ENG-56/chunks/chunk-02-tests/tasks.mdspecs/ENG-56/chunks/manifest.mdspecs/ENG-56/tasks.md
| - No `any` types — project enforces this | ||
| - File naming: camelCase for files (e.g., `mockPAD.ts`, not `mock-pad.ts`) | ||
| - Directory structure: `convex/payments/methods/` (new directory, needs creating) | ||
| - The `convex/payments/` directory does NOT exist yet — must create it |
There was a problem hiding this comment.
Update stale implementation-state statement.
Line 127 says convex/payments/ does not exist yet, but this PR already introduces that directory and files. Please update/remove this line to avoid spec drift.
Based on learnings: Do not flag Markdown linting/formatting issues in this repository; only raise findings when .md content drifts from implemented architecture.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@specs/ENG-56/chunks/chunk-01-interface-and-implementations/context.md` at
line 127, The spec text wrongly states that the `convex/payments/` directory
does not exist yet; update or remove that outdated sentence referencing
`convex/payments/` in the Markdown so the document matches the implementation
introduced in this PR, ensuring the spec no longer claims the directory is
missing.
| ### Registry | ||
| - `getPaymentMethod("manual")` returns ManualPaymentMethod instance | ||
| - `getPaymentMethod("mock_pad")` returns MockPADMethod instance | ||
| - `getPaymentMethod("unknown")` throws `ConvexError("Unknown payment method: unknown")` |
There was a problem hiding this comment.
Documented unknown-method error string does not match implementation.
This line documents Unknown payment method: unknown, but the registry currently throws with quotes around the method value (Unknown payment method: "unknown"). Please align doc and runtime string to prevent expectation drift.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@specs/ENG-56/chunks/chunk-02-tests/context.md` at line 76, The documented
error string for getPaymentMethod is out of sync with the runtime: the registry
currently throws ConvexError("Unknown payment method: \"unknown\"") (with
quotes). Update the spec line in context.md so the example matches the
implementation (e.g., `getPaymentMethod("unknown")` throws `ConvexError("Unknown
payment method: \"unknown\"")`), or alternatively change the registry's thrown
message in the getPaymentMethod/registry code to remove the quotes—pick one
consistent approach and make sure the example and the thrown message match
exactly.
| | chunk-01-interface-and-implementations | T-001 through T-004 | pending | | ||
| | chunk-02-tests | T-005 through T-006 | pending | |
There was a problem hiding this comment.
Manifest status is stale relative to this PR’s completed state.
Both chunks are marked pending, but specs/ENG-56/tasks.md marks all ENG-56 items complete. This creates tracking drift across planning docs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@specs/ENG-56/chunks/manifest.md` around lines 5 - 6, The manifest rows for
chunk-01-interface-and-implementations and chunk-02-tests are still marked
"pending" but the corresponding tasks are completed; update the manifest entries
for chunk-01-interface-and-implementations and chunk-02-tests to the correct
completed status to match the tasks document and ensure the manifest and tasks
are consistent (also scan for any other ENG-56 chunk rows and align their status
if needed).
Add pluggable payment rail abstractions using the Strategy pattern with
dependency-injected scheduling for testability in Convex's stateless V8 isolates.
Co-Authored-By: Claude Opus 4.6 (1M context) noreply@anthropic.com
Summary by CodeRabbit
New Features
Tests