feat(phase3-slice2): payout record creation + invoice/receipt endpoint (Milestones 3c + 3d)#12
Merged
Merged
Conversation
…elay attempt policy union type Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…t (Milestones 3c + 3d) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds payouts, invoices, reviews, and disputes domains (types, repositories, in-memory/Postgres implementations), services, controllers, API-client routes, frontend actions/tests, worker log stub, Nest module wiring, and end-to-end smoke-test scripts; PaymentsService capture now creates payouts and generates invoices. Changes
Sequence DiagramsequenceDiagram
participant Client as Client
participant Payments as PaymentsService
participant Payouts as PayoutsService
participant Invoices as InvoicesService
participant PayoutRepo as PayoutRepository
participant InvoiceRepo as InvoiceRepository
Client->>Payments: capturePaymentForBooking(input)
Payments->>Payments: persist payment
Payments->>Payouts: createPayoutForCapture(payment, providerUserId, correlationId)
Payouts->>PayoutRepo: findPayoutByBookingId(bookingId)
alt existing payout
PayoutRepo-->>Payouts: existing payout
else new payout
Payouts->>PayoutRepo: createPayout(input)
PayoutRepo-->>Payouts: new payout
Payouts->>Payouts: emit payout.created event
end
Payouts-->>Payments: payout result
Payments->>Invoices: generateInvoiceForBooking(bookingContext, payment, correlationId)
Invoices->>InvoiceRepo: findInvoiceByBookingId(bookingId)
alt existing invoice
InvoiceRepo-->>Invoices: existing invoice
else new invoice
Invoices->>InvoiceRepo: createInvoice(input)
InvoiceRepo-->>Invoices: new invoice
end
Invoices-->>Payments: invoice result
Payments-->>Client: return captured payment (with ids)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 |
![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: 8
🧹 Nitpick comments (7)
packages/domain/src/index.ts (1)
192-205: Standardize payout event discriminator naming.
PayoutCreatedDomainEventusestypewhile other domain events in this file useeventName(e.g., Line 45, Line 60, Line 148). Aligning this avoids special-case handling in shared event pipelines.♻️ Suggested change
export type PayoutCreatedDomainEvent = { - type: 'payout.created'; + eventName: 'payout.created'; payoutId: string; bookingId: string; providerUserId: string; amountCents: number; currency: string; correlationId: string; occurredAt: string; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/domain/src/index.ts` around lines 192 - 205, The PayoutCreatedDomainEvent currently uses the field name "type" which is inconsistent with other domain events that use "eventName"; change the PayoutCreatedDomainEvent to use eventName: string (value 'payout.created') instead of type, and update the PayoutCreatedWorkerEnvelope type signature if it references the old field, plus any constructors/serializers/deserializers or consumers that create or read PayoutCreatedDomainEvent to read/write eventName; locate symbols PayoutCreatedDomainEvent and PayoutCreatedWorkerEnvelope in the file and rename the property consistently to avoid special-case handling in shared event pipelines.services/platform-api/src/invoices/invoices.service.ts (1)
23-27: Add booking/payment invariant checks before invoice creation.Before generating the invoice, validate that
payment.bookingId,payment.customerUserId, andpayment.providerUserIdmatch the booking context to prevent cross-entity data corruption on bad caller input.♻️ Suggested guard
async generateInvoiceForBooking( booking: BookingContext, payment: PaymentRecord, correlationId: string, ): Promise<InvoiceRecord> { + if ( + payment.bookingId !== booking.bookingId || + payment.customerUserId !== booking.customerUserId || + payment.providerUserId !== booking.providerUserId + ) { + throw new Error('Booking/payment identity mismatch while generating invoice.'); + } + const existing = await this.invoices.findInvoiceByBookingId(booking.bookingId);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/platform-api/src/invoices/invoices.service.ts` around lines 23 - 27, In generateInvoiceForBooking, add precondition guards that verify payment.bookingId === booking.bookingId, payment.customerUserId === booking.customerUserId, and payment.providerUserId === booking.providerUserId (or the equivalent properties on BookingContext) and if any check fails immediately throw a descriptive error (include correlationId and which field mismatched) to prevent creating an invoice for mismatched booking/payment pairs; place these checks at the start of the generateInvoiceForBooking function before any invoice creation logic.services/platform-api/src/invoices/invoices.module.ts (1)
9-14: LGTM with optional cleanup.Module wiring is correct.
InvoicesServiceis properly exported for injection byPaymentsModule.Note: Listing
InMemoryInvoiceRepositorydirectly inprovidersalongsideinvoiceRepositoryProvidermay be redundant if the provider already handles the class binding. This doesn't cause issues but could be simplified by removing the direct class reference if not needed elsewhere.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/platform-api/src/invoices/invoices.module.ts` around lines 9 - 14, InvoicesModule currently lists both InMemoryInvoiceRepository and invoiceRepositoryProvider in providers which is likely redundant; update the module by removing the direct class provider InMemoryInvoiceRepository from the providers array and keep invoiceRepositoryProvider (and InvoicesService) so the invoice binding is provided via the provider token, ensuring InvoicesService remains exported for PaymentsModule injection; verify that no other module or test depends on a direct InMemoryInvoiceRepository class binding before removing it.apps/product-app/src/features/payouts/payout-screen-actions.ts (1)
23-23: Consider runtime validation for API response.The response is cast with
as PayoutRecord[]without runtime validation. If the API ever returns malformed data, this could cause subtle runtime errors downstream.For robustness, consider validating the response shape (e.g., using a schema validation library like Zod) or at minimum checking
Array.isArray(payouts).💡 Minimal defensive check
- const payouts = (await response.json()) as PayoutRecord[]; + const payouts = (await response.json()) as PayoutRecord[]; + if (!Array.isArray(payouts)) { + return { status: 'error', message: 'Invalid response format from server.' }; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/product-app/src/features/payouts/payout-screen-actions.ts` at line 23, The response is being cast directly to PayoutRecord[] which is unsafe; in the code around the line where you call response.json() and assign to payouts, validate the runtime shape before treating it as PayoutRecord[]. Replace the blind cast with a runtime check: ensure Array.isArray(payouts) and verify required fields exist on each item (or use a Zod schema) and handle/throw/log if validation fails; update the code paths that use payouts (the variable named payouts and any functions that consume it) to only proceed after validation or handle the error pathway.services/platform-api/src/payouts/payouts.service.test.ts (1)
64-72: Test name suggests event emission verification, but test only checks payout creation.The test "emits PayoutCreatedDomainEvent breadcrumb on new payout" doesn't actually verify event emission or breadcrumb logging. It only asserts that a payout is created with the expected
bookingId. Consider either:
- Renaming to reflect what's actually tested (e.g., "creates payout for new booking")
- Adding verification of event emission (e.g., by capturing emitted events or spying on the logger)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/platform-api/src/payouts/payouts.service.test.ts` around lines 64 - 72, The test named "emits PayoutCreatedDomainEvent breadcrumb on new payout" currently only verifies payout creation via createService(), makePayment(), and service.createPayoutForCapture but does not assert any event or breadcrumb emission; either rename the test to reflect creation behavior (e.g., "creates payout for new booking") or augment it to assert emission by spying/stubbing the event/logging path used by createPayoutForCapture (for example spy the breadcrumb/logger or event emitter used to emit PayoutCreatedDomainEvent) and add an expect that the emitter/logger was called with the PayoutCreatedDomainEvent/breadcrumb payload after calling service.createPayoutForCapture.services/platform-api/src/payments/payments.service.ts (1)
53-68: Sequential execution of payout and invoice creation.Both operations are awaited sequentially. Since they're independent (invoice doesn't depend on payout), they could be parallelized with
Promise.allfor slightly better latency. However, the current sequential approach is simpler and easier to debug.💡 Optional: Parallelize independent operations
- await this.payoutsService.createPayoutForCapture( - result.payment, - input.providerUserId, - input.correlationId, - ); - - await this.invoicesService.generateInvoiceForBooking( - { - bookingId: input.bookingId, - customerUserId: input.customerUserId, - providerUserId: input.providerUserId, - requestedService: input.requestedService ?? 'General handyman help', - }, - result.payment, - input.correlationId, - ); + await Promise.all([ + this.payoutsService.createPayoutForCapture( + result.payment, + input.providerUserId, + input.correlationId, + ), + this.invoicesService.generateInvoiceForBooking( + { + bookingId: input.bookingId, + customerUserId: input.customerUserId, + providerUserId: input.providerUserId, + requestedService: input.requestedService ?? 'General handyman help', + }, + result.payment, + input.correlationId, + ), + ]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/platform-api/src/payments/payments.service.ts` around lines 53 - 68, The code currently awaits payoutsService.createPayoutForCapture(...) and then invoicesService.generateInvoiceForBooking(...) sequentially which adds unnecessary latency; change to run them in parallel using Promise.all so both payoutsService.createPayoutForCapture(result.payment, input.providerUserId, input.correlationId) and invoicesService.generateInvoiceForBooking({ bookingId: input.bookingId, customerUserId: input.customerUserId, providerUserId: input.providerUserId, requestedService: input.requestedService ?? 'General handyman help' }, result.payment, input.correlationId) are started together and awaited via Promise.all([...]) to preserve behavior but reduce overall wait time.packages/api-client/src/index.ts (1)
263-289: Derive new routes from shared base constants to avoid path drift.These are currently hardcoded while the rest of the file centralizes base paths in
apiRoutes.♻️ Proposed refactor
export const providerPayoutApiRoutes = { - list: () => '/api/v1/providers/me/payouts', - detail: (payoutId: string) => `/api/v1/providers/me/payouts/${payoutId}`, + list: () => `${apiRoutes.providers}/me/payouts`, + detail: (payoutId: string) => `${apiRoutes.providers}/me/payouts/${payoutId}`, } as const; @@ export const bookingInvoiceApiRoutes = { - get: (bookingId: string) => `/api/v1/bookings/${bookingId}/invoice`, + get: (bookingId: string) => `${apiRoutes.bookings}/${bookingId}/invoice`, } as const;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/api-client/src/index.ts` around lines 263 - 289, The hardcoded paths in providerPayoutApiRoutes and bookingInvoiceApiRoutes should be derived from the shared apiRoutes constants to avoid path drift: update providerPayoutApiRoutes (used by createGetMyPayoutsRequest and createGetPayoutDetailRequest) to build URLs from the central apiRoutes provider/base entries (e.g., use apiRoutes.providers.me or equivalent) instead of literal '/api/v1/providers/me/payouts', and update bookingInvoiceApiRoutes (used by createGetBookingInvoiceRequest) to use apiRoutes.bookings or its booking base path (e.g., apiRoutes.bookings.base or apiRoutes.bookings.booking) to construct `/bookings/${bookingId}/invoice`; keep the same function signatures and headers but replace string literals with template construction from the apiRoutes constants.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.agent/plans/phase3-slice2-payouts-invoices-2026-04-01.md:
- Line 113: The Markdown fences triggering MD040 lack language identifiers;
update the three fenced code blocks containing the lines starting with "GET
/api/v1/providers/me/payouts → provider session required", "GET
/api/v1/bookings/:id/invoice → customer or provider session required", and the
block with the commit message "feat(phase3-slice2): payout record creation +
invoice/receipt endpoint (Milestones 3c + 3d)" to use a language token (e.g.,
change ``` to ```text) so the linter passes.
In `@apps/admin-web/next-env.d.ts`:
- Line 3: The import of "./.next/dev/types/routes.d.ts" in next-env.d.ts
references a build output that won't exist and indicates typedRoutes isn't
configured; either remove that import from next-env.d.ts, or enable Next.js
typed routes by setting typedRoutes: true in next.config.js and ensure tsconfig
includes the generated types directory (e.g., .next/types/**/*.ts or use
isolatedDevBuild) so the declaration merging works; update next-env.d.ts only if
you intentionally rely on typed routes—otherwise delete the import line to avoid
referencing non-existent build artifacts.
In
`@services/platform-api/src/invoices/infrastructure/in-memory-invoice.repository.ts`:
- Line 21: The repository is storing caller-owned mutable lineItems by reference
(lineItems: input.lineItems) which can lead to silent mutations; in the
InMemoryInvoiceRepository (e.g., the create/save method where lineItems is set)
replace that reference assignment with a deep copy of the array and its item
objects—use structuredClone if available or map over input.lineItems to create
new plain objects (or JSON deep-clone) so the stored invoice owns its own
immutable copy of lineItems.
In `@services/platform-api/src/invoices/invoices.service.ts`:
- Around line 28-66: The current findInvoiceByBookingId + createInvoice flow is
vulnerable to race conditions (two concurrent calls can both miss and create a
duplicate invoice); change the callsite to use an atomic repository operation
such as invoices.createInvoiceIfAbsent(booking.bookingId, invoicePayload) or
implement createInvoiceIfAbsent in the invoices repository which attempts to
insert and on conflict returns the existing invoice (or enforces a unique
bookingId constraint at persistence layer and returns the existing record on
conflict); replace the existing invoices.findInvoiceByBookingId(...) +
invoices.createInvoice(...) sequence with a single call to the new
createInvoiceIfAbsent and propagate its result.
In `@services/platform-api/src/payments/payments.service.ts`:
- Around line 53-68: The two awaited calls payoutsService.createPayoutForCapture
and invoicesService.generateInvoiceForBooking can produce a partial-success
(payout created but invoice fails); wrap both in a try/catch around the invoice
generation so that after calling this.payoutsService.createPayoutForCapture(...)
you call this.invoicesService.generateInvoiceForBooking(...) inside a try block
and in the catch log a clear, structured error (include result.payment id,
input.correlationId, input.bookingId and the caught error) and return or throw a
well-defined partial-failure indicator (e.g., return a response object with
paymentCaptured: true and invoiceGenerated: false or throw a
PartialFailureError) so callers can treat the operation as “possibly succeeded”
and retry idempotently; reference the methods
payoutsService.createPayoutForCapture and
invoicesService.generateInvoiceForBooking when changing the implementation.
In `@services/platform-api/src/payouts/domain/payout.repository.ts`:
- Around line 15-16: The repository method findPayoutsByProviderUserId returns
an unbounded PayoutRecord[] which prevents implementing a paginated provider
payouts API; change the method signature to accept and return pagination
parameters (for example add limit and cursor/offset arguments or a PagingOptions
type and return a PaginatedResult<PayoutRecord> or { items: PayoutRecord[];
nextCursor?: string; total?: number }) and update all implementations of
findPayoutsByProviderUserId (and any callers) to use those parameters (e.g., add
limit/offset or cursor handling inside the repository implementation and SQL/ORM
queries) so the listing is bounded and supports pagination.
In `@services/platform-api/src/payouts/payouts.service.ts`:
- Around line 21-45: The current read-then-create flow using
this.payouts.findPayoutByBookingId followed by this.payouts.createPayout is
race-prone and can create duplicate payouts under concurrent requests; change to
an atomic create-or-return-existing pattern by attempting the create first (or
using an upsert/INSERT ... ON CONFLICT) inside the payouts.createPayout
implementation and, on unique-constraint violation, re-query with
payouts.findPayoutByBookingId to return the existing record; ensure you still
emit the same logStructuredBreadcrumb entry (with replayed: true) when returning
the existing payout so callers and telemetry behave identically.
- Around line 47-69: The code builds a PayoutCreatedDomainEvent
(payoutCreatedEvent) and logs via logStructuredBreadcrumb but never actually
emits or persists the event; update the code to publish or enqueue
payoutCreatedEvent to your domain event pipeline (for example call the existing
domain event publisher or outbox API used elsewhere — e.g.,
domainEventPublisher.publish(payoutCreatedEvent) or
outbox.enqueue(payoutCreatedEvent)) immediately after constructing
payoutCreatedEvent and before/after logging, ensuring correlationId and
occurredAt are passed through.
---
Nitpick comments:
In `@apps/product-app/src/features/payouts/payout-screen-actions.ts`:
- Line 23: The response is being cast directly to PayoutRecord[] which is
unsafe; in the code around the line where you call response.json() and assign to
payouts, validate the runtime shape before treating it as PayoutRecord[].
Replace the blind cast with a runtime check: ensure Array.isArray(payouts) and
verify required fields exist on each item (or use a Zod schema) and
handle/throw/log if validation fails; update the code paths that use payouts
(the variable named payouts and any functions that consume it) to only proceed
after validation or handle the error pathway.
In `@packages/api-client/src/index.ts`:
- Around line 263-289: The hardcoded paths in providerPayoutApiRoutes and
bookingInvoiceApiRoutes should be derived from the shared apiRoutes constants to
avoid path drift: update providerPayoutApiRoutes (used by
createGetMyPayoutsRequest and createGetPayoutDetailRequest) to build URLs from
the central apiRoutes provider/base entries (e.g., use apiRoutes.providers.me or
equivalent) instead of literal '/api/v1/providers/me/payouts', and update
bookingInvoiceApiRoutes (used by createGetBookingInvoiceRequest) to use
apiRoutes.bookings or its booking base path (e.g., apiRoutes.bookings.base or
apiRoutes.bookings.booking) to construct `/bookings/${bookingId}/invoice`; keep
the same function signatures and headers but replace string literals with
template construction from the apiRoutes constants.
In `@packages/domain/src/index.ts`:
- Around line 192-205: The PayoutCreatedDomainEvent currently uses the field
name "type" which is inconsistent with other domain events that use "eventName";
change the PayoutCreatedDomainEvent to use eventName: string (value
'payout.created') instead of type, and update the PayoutCreatedWorkerEnvelope
type signature if it references the old field, plus any
constructors/serializers/deserializers or consumers that create or read
PayoutCreatedDomainEvent to read/write eventName; locate symbols
PayoutCreatedDomainEvent and PayoutCreatedWorkerEnvelope in the file and rename
the property consistently to avoid special-case handling in shared event
pipelines.
In `@services/platform-api/src/invoices/invoices.module.ts`:
- Around line 9-14: InvoicesModule currently lists both
InMemoryInvoiceRepository and invoiceRepositoryProvider in providers which is
likely redundant; update the module by removing the direct class provider
InMemoryInvoiceRepository from the providers array and keep
invoiceRepositoryProvider (and InvoicesService) so the invoice binding is
provided via the provider token, ensuring InvoicesService remains exported for
PaymentsModule injection; verify that no other module or test depends on a
direct InMemoryInvoiceRepository class binding before removing it.
In `@services/platform-api/src/invoices/invoices.service.ts`:
- Around line 23-27: In generateInvoiceForBooking, add precondition guards that
verify payment.bookingId === booking.bookingId, payment.customerUserId ===
booking.customerUserId, and payment.providerUserId === booking.providerUserId
(or the equivalent properties on BookingContext) and if any check fails
immediately throw a descriptive error (include correlationId and which field
mismatched) to prevent creating an invoice for mismatched booking/payment pairs;
place these checks at the start of the generateInvoiceForBooking function before
any invoice creation logic.
In `@services/platform-api/src/payments/payments.service.ts`:
- Around line 53-68: The code currently awaits
payoutsService.createPayoutForCapture(...) and then
invoicesService.generateInvoiceForBooking(...) sequentially which adds
unnecessary latency; change to run them in parallel using Promise.all so both
payoutsService.createPayoutForCapture(result.payment, input.providerUserId,
input.correlationId) and invoicesService.generateInvoiceForBooking({ bookingId:
input.bookingId, customerUserId: input.customerUserId, providerUserId:
input.providerUserId, requestedService: input.requestedService ?? 'General
handyman help' }, result.payment, input.correlationId) are started together and
awaited via Promise.all([...]) to preserve behavior but reduce overall wait
time.
In `@services/platform-api/src/payouts/payouts.service.test.ts`:
- Around line 64-72: The test named "emits PayoutCreatedDomainEvent breadcrumb
on new payout" currently only verifies payout creation via createService(),
makePayment(), and service.createPayoutForCapture but does not assert any event
or breadcrumb emission; either rename the test to reflect creation behavior
(e.g., "creates payout for new booking") or augment it to assert emission by
spying/stubbing the event/logging path used by createPayoutForCapture (for
example spy the breadcrumb/logger or event emitter used to emit
PayoutCreatedDomainEvent) and add an expect that the emitter/logger was called
with the PayoutCreatedDomainEvent/breadcrumb payload after calling
service.createPayoutForCapture.
🪄 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
Run ID: eb2f1fde-e917-4ca8-a37c-0ca29d8bb9dd
📒 Files selected for processing (32)
.agent/plans/phase3-slice2-payouts-invoices-2026-04-01.mdapps/admin-web/next-env.d.tsapps/product-app/src/features/payouts/payout-screen-actions.test.tsapps/product-app/src/features/payouts/payout-screen-actions.tsapps/product-app/src/features/payouts/payout-state.test.tsapps/product-app/src/features/payouts/payout-state.tspackages/api-client/src/index.tspackages/domain/src/index.tsservices/background-workers/src/workers/payment-captured.worker.tsservices/platform-api/src/app.module.tsservices/platform-api/src/bookings/bookings.service.complete.test.tsservices/platform-api/src/bookings/bookings.service.test.tsservices/platform-api/src/bookings/bookings.service.tsservices/platform-api/src/invoices/domain/invoice.repository.tsservices/platform-api/src/invoices/infrastructure/in-memory-invoice.repository.tsservices/platform-api/src/invoices/infrastructure/invoice-repository.provider.tsservices/platform-api/src/invoices/invoices.controller.tsservices/platform-api/src/invoices/invoices.module.tsservices/platform-api/src/invoices/invoices.service.test.tsservices/platform-api/src/invoices/invoices.service.tsservices/platform-api/src/orchestration/relay-attempt-policy.tsservices/platform-api/src/payments/domain/payment.repository.tsservices/platform-api/src/payments/payments.module.tsservices/platform-api/src/payments/payments.service.test.tsservices/platform-api/src/payments/payments.service.tsservices/platform-api/src/payouts/domain/payout.repository.tsservices/platform-api/src/payouts/infrastructure/in-memory-payout.repository.tsservices/platform-api/src/payouts/infrastructure/payout-repository.provider.tsservices/platform-api/src/payouts/payouts.controller.tsservices/platform-api/src/payouts/payouts.module.tsservices/platform-api/src/payouts/payouts.service.test.tsservices/platform-api/src/payouts/payouts.service.ts
Co-Authored-By: Paperclip <noreply@paperclip.ing>
Co-Authored-By: Paperclip <noreply@paperclip.ing>
Co-Authored-By: Paperclip <noreply@paperclip.ing>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
services/platform-api/src/bookings/bookings.service.ts (1)
489-529:⚠️ Potential issue | 🔴 CriticalAvoid creating payout/invoice before the booking transition is committed.
capturePaymentForBooking()now triggers payout and invoice creation inservices/platform-api/src/payments/payments.service.ts:53-68. IfcompleteAcceptedBooking()then returnsnot-foundortransition-conflict, this method exits with a captured payment and downstream records for a booking that never reachedcompleted. Please move those side effects behind a successful booking transition, or add compensation for the capture/payout/invoice writes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/platform-api/src/bookings/bookings.service.ts` around lines 489 - 529, The payment capture currently creates payout/invoice side effects inside capturePaymentForBooking, which can leave persisted payment & payout records if bookings.completeAcceptedBooking returns not-found or transition-conflict; fix by ensuring side effects occur only after a successful booking transition: either (A) move the call that creates payout/invoice out of capturePaymentForBooking and invoke it only after completeAcceptedBooking returns ok (keep capturePaymentForBooking to only authorize/capture funds), or (B) keep capturePaymentForBooking but implement compensation in bookings.service.ts to call a new paymentsService.rollbackPayment/voidOrRefundPayment when completeAcceptedBooking returns not-found or transition-conflict; reference capturePaymentForBooking and completeAcceptedBooking when making the change.
🧹 Nitpick comments (9)
services/platform-api/src/payouts/infrastructure/postgres-payout.repository.test.ts (2)
6-6: Use domainPayoutStatusin tests to prevent contract drift.The local union can diverge from
@quickwerk/domainand let tests pass against stale status values.Refactor sketch
+import type { PayoutStatus } from '@quickwerk/domain'; import { describe, expect, it } from 'vitest'; @@ -type PayoutStatus = 'pending' | 'processing' | 'settled' | 'failed';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/platform-api/src/payouts/infrastructure/postgres-payout.repository.test.ts` at line 6, Tests define a local union type PayoutStatus which can drift from the canonical enum; replace the local "type PayoutStatus = 'pending' | 'processing' | 'settled' | 'failed';" with an import of PayoutStatus from `@quickwerk/domain` (e.g., import { PayoutStatus } from '@quickwerk/domain') and update any test references to use that imported symbol (and adjust any value assertions to the domain's values) so tests always use the domain contract.
123-146: Add multi-row assertion forcreated_at DESCordering.Current coverage checks only one payout, so it does not verify the repository’s ordering contract for provider payout lists.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/platform-api/src/payouts/infrastructure/postgres-payout.repository.test.ts` around lines 123 - 146, Add a test case in the describe('findPayoutsByProviderUserId') suite that creates multiple PayoutState entries for the same provider with different created_at timestamps (use makePayoutState to produce two or more payouts and set distinct createdAt values), pass them into makePostgresClient (Map keyed by BOOKING_ID variants), call PostgresPayoutRepository.findPayoutsByProviderUserId(PROVIDER_ID), assert the result length matches the number inserted, and assert the array is ordered by created_at DESC (e.g., result[0].createdAt >= result[1].createdAt and so on) to verify ordering behavior.services/platform-api/src/payouts/infrastructure/postgres-payout.repository.ts (1)
77-145: ReusePAYOUT_SELECT_COLUMNSacross all read queries to avoid drift.
findPayoutById,findPayoutsByProviderUserId, andfindPayoutByBookingIdinline the same column list already defined inPAYOUT_SELECT_COLUMNS.Refactor sketch
- `SELECT id::text, - provider_user_id::text, - booking_id::text, - payment_id::text, - amount_cents, - currency, - status, - settlement_ref, - created_at, - settled_at - FROM payouts + `SELECT ${PAYOUT_SELECT_COLUMNS} + FROM payouts WHERE id = $1::uuid LIMIT 1`,Apply the same simplification in the other two read methods.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/platform-api/src/payouts/infrastructure/postgres-payout.repository.ts` around lines 77 - 145, Replace the duplicated inline column lists in the SQL strings of findPayoutById, findPayoutsByProviderUserId, and findPayoutByBookingId with the existing PAYOUT_SELECT_COLUMNS constant to avoid drift; update each query to interpolate or concatenate PAYOUT_SELECT_COLUMNS where the SELECT column block currently appears (keep the same WHERE, ORDER BY, LIMIT clauses and parameters) so all three methods reuse the single source of truth for selected columns.services/platform-api/src/invoices/infrastructure/postgres-invoice.repository.ts (1)
88-107: Consider bulk-inserting line items for better performance.Line items are inserted one-by-one in a loop. For invoices with many line items, this creates N round-trips to the database within the transaction. A single bulk insert would be more efficient.
♻️ Suggested bulk insert approach
if (existingId === invoiceId) { - for (const item of input.lineItems) { - await client.query( - `INSERT INTO invoice_line_items ( - id, - invoice_id, - description, - quantity, - unit_amount_cents, - total_amount_cents - ) VALUES ($1::uuid, $2::uuid, $3, $4, $5, $6)`, - [ - randomUUID(), - invoiceId, - item.description, - item.quantity, - item.unitAmountCents, - item.totalAmountCents, - ], - ); - } + if (input.lineItems.length > 0) { + const values: unknown[] = []; + const placeholders = input.lineItems.map((item, i) => { + const offset = i * 6; + values.push( + randomUUID(), + invoiceId, + item.description, + item.quantity, + item.unitAmountCents, + item.totalAmountCents, + ); + return `($${offset + 1}::uuid, $${offset + 2}::uuid, $${offset + 3}, $${offset + 4}, $${offset + 5}, $${offset + 6})`; + }); + await client.query( + `INSERT INTO invoice_line_items ( + id, invoice_id, description, quantity, unit_amount_cents, total_amount_cents + ) VALUES ${placeholders.join(', ')}`, + values, + ); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/platform-api/src/invoices/infrastructure/postgres-invoice.repository.ts` around lines 88 - 107, The current code inserts invoice_line_items inside a loop causing N DB round-trips; replace the per-item client.query calls with a single bulk INSERT: pre-generate UUIDs for each line item (using randomUUID()), build a flattened values array including invoiceId and each item's fields, construct a single INSERT INTO invoice_line_items (...) VALUES (<placeholders>) with one VALUES row per item (unique parameter indexes for each column), then call client.query once; update the block that currently iterates "for (const item of input.lineItems)" to create the placeholders and values and execute a single client.query using those variables (keep using invoiceId, randomUUID(), and item.description/quantity/unitAmountCents/totalAmountCents).apps/product-app/src/features/booking/review-screen-actions.test.ts (1)
39-45: Strengthen the “without comment” assertion.The test title says “without comment”, but it only checks
status. Consider asserting the request/response handling specific to omittedcommentso this case can catch regressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/product-app/src/features/booking/review-screen-actions.test.ts` around lines 39 - 45, The test only checks status but should also assert handling of an omitted comment; update the 'returns submitted state without comment' test to verify that submitReview sent no comment and that the returned review has no comment field: use the mockFetch/ fetch spy from mockFetch to inspect the request body passed into submitReview (from submitReview's fetch call) and expect it not to contain a "comment" key or to be null/undefined, and also assert the returned review (from makeReviewRecord / result) has comment === undefined (or no comment property). Target symbols: submitReview, mockFetch, makeReviewRecord, and the test's fetch result/spy.scripts/smoke/phase3-slice2-payouts-invoices-smoke.sh (1)
140-150: Consider edge case: numeric extraction may fail for zero values.The
extract_numregex\([0-9][0-9]*\)requires at least one digit but doesn't handle the case whereamountCentscould legitimately be0. While unlikely in this test flow, the pattern\([0-9]*\)or\(-\?[0-9]*\)would be more robust.Optional: Handle zero and negative values
extract_num() { local json="$1" local field="$2" - echo "$json" | sed -n "s/.*\"${field}\":\([0-9][0-9]*\).*/\1/p" | head -1 + echo "$json" | sed -n "s/.*\"${field}\":\(-\?[0-9][0-9]*\).*/\1/p" | head -1 }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/smoke/phase3-slice2-payouts-invoices-smoke.sh` around lines 140 - 150, The extract_num function's regex in extract_num currently requires one or more digits and will miss a value of "0" (and won't handle negatives); update the sed capture in extract_num (the function named extract_num) to allow zero and optionally negative numbers (e.g., use a pattern that permits an optional '-' and zero or more digits) so fields like amountCents="0" are captured correctly and still return the first match as before.services/platform-api/src/reviews/reviews.controller.ts (2)
18-21: Same observation: Consider adding validation decorators.Similar to
SubmitDisputeBodyin the disputes controller, this DTO would benefit from runtime validation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/platform-api/src/reviews/reviews.controller.ts` around lines 18 - 21, Convert the plain TypeScript type SubmitReviewBody into a class DTO with validation decorators (e.g., class SubmitReviewBody { rating; comment }) and apply class-validator rules: validate rating as an integer within allowed bounds (IsInt, Min, Max) and validate comment as an optional string with length constraints (IsOptional, IsString, Length). Update any handlers that consume SubmitReviewBody in reviews.controller.ts to accept the class instance (so Nest's ValidationPipe can run) and ensure the same DTO name (SubmitReviewBody) is used so references remain consistent.
94-116: Public endpoint LGTM, but consider defensive error handling.The public provider reviews endpoint is appropriate for displaying reviews. However,
getProviderReviewsreturns{ ok: true; reviews }unconditionally today, but if the service signature evolves to include error cases, this controller won't handle them. Consider adding defensive handling for consistency:Optional: Defensive error handling
const result = await this.reviewsService.getProviderReviews(providerUserId); + if ('ok' in result && !result.ok) { + throw new HttpException(result.error, result.statusCode); + } return result.reviews;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/platform-api/src/reviews/reviews.controller.ts` around lines 94 - 116, The controller assumes reviewsService.getProviderReviews always succeeds; update ProviderReviewsController.getProviderReviews to handle potential error responses from reviewsService.getProviderReviews by checking the returned shape (e.g., result.ok) and, if false, set an appropriate HTTP status on response (or throw an HttpException) and return an error payload; ensure you still set the correlationIdHeaderName via response.setHeader and return result.reviews only when result.ok is true so callers get consistent success/error behavior.services/platform-api/src/disputes/disputes.controller.ts (1)
18-21: This validation recommendation conflicts with the codebase's established pattern.The
SubmitDisputeBodytype is consistent with how all other controllers in this codebase handle request bodies (e.g.,SubmitReviewBody,SignInRequestBody,CreateBookingRequestBody). The codebase does not useclass-validatororValidationPipeanywhere inservices/platform-api/src/. If runtime validation is desired, it should be applied consistently across all request body types as an architectural change, not selectively to this endpoint.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/platform-api/src/disputes/disputes.controller.ts` around lines 18 - 21, The SubmitDisputeBody should follow the repository's existing pattern of plain TypeScript request-body types rather than introducing class-validator/ValidationPipe; revert any class-validator decorators or ValidationPipe usage in disputes.controller.ts and restore SubmitDisputeBody as a simple type alias (e.g., type SubmitDisputeBody = { category: string; description: string; }) and ensure the controller method uses that type like other controllers (e.g., SubmitReviewBody, SignInRequestBody); if runtime validation is needed, file a separate PR to introduce validation across services/platform-api with consistent patterns rather than changing this single endpoint.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/admin-web/src/features/disputes/dispute-queue-actions.ts`:
- Around line 16-25: The isDisputeRecord type guard is too permissive: tighten
validation in isDisputeRecord to fully validate optional and enum-like fields —
ensure reporterRole is one of 'customer'|'provider', status and category are
checked against the DisputeRecord allowed values (enum or union literals), and
validate resolvedAt and resolutionNote presence/format according to
DisputeRecord (e.g., resolvedAt must be either undefined or an ISO date string
and resolutionNote must be a string when present). Update the guard logic in
isDisputeRecord to explicitly check these fields (use the same enum/union
definitions or helper validators used elsewhere) so malformed payloads cannot be
treated as DisputeRecord.
In `@services/platform-api/src/bookings/bookings.service.ts`:
- Around line 421-461: The current branch that handles bookingToComplete.status
=== 'completed' returns early and skips publishing the capture event, causing
missed downstream events on replay; before returning the successful response
(after obtaining replayedPayment and logging the breadcrumb in this branch),
call the existing publishPaymentCaptured flow with the replayed payment (use the
same signature used at the later path around publishPaymentCaptured) and include
replay metadata (e.g., replayed: true) so the capture event is emitted for
replays; reference getPaymentByBookingId, the booking.status === 'completed'
branch, the breadcrumb log block, and publishPaymentCaptured to locate where to
insert the call.
In `@services/platform-api/src/disputes/disputes.service.ts`:
- Around line 28-60: The current idempotency check using
this.disputes.findByBookingIdAndReporter(...) followed by
this.disputes.save(dispute) is race-prone and can create duplicate disputes
under concurrent requests; change the repository usage to an atomic upsert or
add a unique DB constraint on (bookingId, reporterUserId) and handle conflicts
in the save path. Implement a new repository method like
upsertByBookingAndReporter(dispute) (or modify save to perform INSERT ... ON
CONFLICT DO NOTHING/UPDATE) so the operation returns the existing record when a
conflict occurs, and ensure the service returns that record instead of inserting
a duplicate; keep disputeId generation idempotent by only generating when the
upsert indicates a new row was created.
- Around line 17-60: The submitDispute flow currently only checks session.role
but not whether the caller is actually associated with the booking; before
creating or saving the Dispute in submitDispute, fetch the booking record (use
your booking lookup method, e.g. this.bookings.findById or
this.bookings.getBookingById) and verify that session.userId is a participant on
that booking (for reporterRole 'customer' ensure booking.customerId ===
session.userId; for 'provider' ensure booking.providerId === session.userId); if
the booking is missing or the user is not the booking participant return { ok:
false, statusCode: 403, error: 'Not authorized to submit dispute for this
booking' } and do not call this.disputes.save. Ensure you use the same dispute
flow and logging (e.g. logStructuredBreadcrumb) after the authorization check
passes.
- Around line 20-53: The code currently force-casts the incoming category string
into DisputeRecord['category'] which can persist invalid values; update the
submit flow in disputes.service.ts to validate category before creating the
DisputeRecord: introduce a whitelist check (or type guard) against the
DisputeRecord category union/enum (e.g., via an isValidCategory helper or a Set
of allowed categories), and if validation fails return a descriptive error
response (e.g., { ok: false, statusCode: 400, error: 'Invalid dispute category'
}); only when the check passes assign category to the dispute object instead of
using category as DisputeRecord['category'].
In
`@services/platform-api/src/disputes/infrastructure/dispute-repository.provider.ts`:
- Around line 6-9: The provider disputeRepositoryProvider currently uses
useClass: InMemoryDisputeRepository which causes Nest to create a separate
instance; change it to useExisting: InMemoryDisputeRepository so the
DISPUTE_REPOSITORY token resolves to the same instance as direct
InMemoryDisputeRepository injections (update disputeRepositoryProvider to
useExisting and ensure DisputesModule registers InMemoryDisputeRepository once).
In
`@services/platform-api/src/payouts/infrastructure/postgres-payout.repository.test.ts`:
- Around line 190-216: The mock for the INSERT into payouts incorrectly computes
rowCount after inserting, masking ON CONFLICT DO NOTHING behavior; change the
logic in the test mock (the branch guarded by text.includes('INSERT INTO
payouts')) to first determine const existed = payouts.has(bookingId) (using the
bookingId extracted from values), then only add the new payout to the payouts
Map when existed is false, and return { rows: [], rowCount: existed ? 0 : 1 };
reference the variables/text check used in the diff: text.includes('INSERT INTO
payouts'), values -> bookingId, payouts Map, and PAYOUT_ID.
In
`@services/platform-api/src/payouts/infrastructure/postgres-payout.repository.ts`:
- Around line 39-75: The createPayout implementation uses ON CONFLICT
(booking_id) DO NOTHING and then returns the existing row without checking that
the existing payout's fields match the attempted payload; update createPayout
to, after the SELECT of the existing row (PAYOUT_SELECT_COLUMNS -> row),
validate that row.provider_user_id, row.payment_id, row.amount_cents and
row.currency match input.providerUserId, input.paymentId, input.amountCents and
input.currency (and optionally status if relevant), and if any mismatch occurs
throw a clear error indicating an idempotency conflict so callers know the
existing record is inconsistent with the attempted write; keep the
insert+do-nothing behavior but enforce this payload consistency check before
returning mapPayoutRow(row).
In
`@services/platform-api/src/reviews/infrastructure/review-repository.provider.ts`:
- Around line 6-9: The provider reviewRepositoryProvider currently uses useClass
which creates a separate InMemoryReviewRepository instance; change it to
useExisting: InMemoryReviewRepository (and keep provide: REVIEW_REPOSITORY) so
the REVIEW_REPOSITORY token reuses the already-registered
InMemoryReviewRepository instance, or alternatively remove the direct
InMemoryReviewRepository registration and only register reviewRepositoryProvider
to avoid duplicate instances.
In `@services/platform-api/src/reviews/reviews.service.ts`:
- Around line 90-111: The code constructs a ReviewSubmittedDomainEvent but never
publishes it while the breadcrumb log name suggests emission; update
ReviewsService to either (A) inject and use a domain event publisher (mirror
BookingDomainEventPublisher usage) and call e.g.
reviewDomainEventPublisher.publish(event) immediately after creating the event
(keep the log but rename it to reflect success of publish if you want), or (B)
if you intentionally do not publish, change the logStructuredBreadcrumb event
name from 'review.submitted.domain-event.emit' to something like
'review.submitted.domain-event.logged' to avoid implying emission; locate the
ReviewSubmittedDomainEvent creation, the logStructuredBreadcrumb call, and
follow the pattern used by BookingDomainEventPublisher to implement publishing
if chosen.
---
Outside diff comments:
In `@services/platform-api/src/bookings/bookings.service.ts`:
- Around line 489-529: The payment capture currently creates payout/invoice side
effects inside capturePaymentForBooking, which can leave persisted payment &
payout records if bookings.completeAcceptedBooking returns not-found or
transition-conflict; fix by ensuring side effects occur only after a successful
booking transition: either (A) move the call that creates payout/invoice out of
capturePaymentForBooking and invoke it only after completeAcceptedBooking
returns ok (keep capturePaymentForBooking to only authorize/capture funds), or
(B) keep capturePaymentForBooking but implement compensation in
bookings.service.ts to call a new
paymentsService.rollbackPayment/voidOrRefundPayment when completeAcceptedBooking
returns not-found or transition-conflict; reference capturePaymentForBooking and
completeAcceptedBooking when making the change.
---
Nitpick comments:
In `@apps/product-app/src/features/booking/review-screen-actions.test.ts`:
- Around line 39-45: The test only checks status but should also assert handling
of an omitted comment; update the 'returns submitted state without comment' test
to verify that submitReview sent no comment and that the returned review has no
comment field: use the mockFetch/ fetch spy from mockFetch to inspect the
request body passed into submitReview (from submitReview's fetch call) and
expect it not to contain a "comment" key or to be null/undefined, and also
assert the returned review (from makeReviewRecord / result) has comment ===
undefined (or no comment property). Target symbols: submitReview, mockFetch,
makeReviewRecord, and the test's fetch result/spy.
In `@scripts/smoke/phase3-slice2-payouts-invoices-smoke.sh`:
- Around line 140-150: The extract_num function's regex in extract_num currently
requires one or more digits and will miss a value of "0" (and won't handle
negatives); update the sed capture in extract_num (the function named
extract_num) to allow zero and optionally negative numbers (e.g., use a pattern
that permits an optional '-' and zero or more digits) so fields like
amountCents="0" are captured correctly and still return the first match as
before.
In `@services/platform-api/src/disputes/disputes.controller.ts`:
- Around line 18-21: The SubmitDisputeBody should follow the repository's
existing pattern of plain TypeScript request-body types rather than introducing
class-validator/ValidationPipe; revert any class-validator decorators or
ValidationPipe usage in disputes.controller.ts and restore SubmitDisputeBody as
a simple type alias (e.g., type SubmitDisputeBody = { category: string;
description: string; }) and ensure the controller method uses that type like
other controllers (e.g., SubmitReviewBody, SignInRequestBody); if runtime
validation is needed, file a separate PR to introduce validation across
services/platform-api with consistent patterns rather than changing this single
endpoint.
In
`@services/platform-api/src/invoices/infrastructure/postgres-invoice.repository.ts`:
- Around line 88-107: The current code inserts invoice_line_items inside a loop
causing N DB round-trips; replace the per-item client.query calls with a single
bulk INSERT: pre-generate UUIDs for each line item (using randomUUID()), build a
flattened values array including invoiceId and each item's fields, construct a
single INSERT INTO invoice_line_items (...) VALUES (<placeholders>) with one
VALUES row per item (unique parameter indexes for each column), then call
client.query once; update the block that currently iterates "for (const item of
input.lineItems)" to create the placeholders and values and execute a single
client.query using those variables (keep using invoiceId, randomUUID(), and
item.description/quantity/unitAmountCents/totalAmountCents).
In
`@services/platform-api/src/payouts/infrastructure/postgres-payout.repository.test.ts`:
- Line 6: Tests define a local union type PayoutStatus which can drift from the
canonical enum; replace the local "type PayoutStatus = 'pending' | 'processing'
| 'settled' | 'failed';" with an import of PayoutStatus from `@quickwerk/domain`
(e.g., import { PayoutStatus } from '@quickwerk/domain') and update any test
references to use that imported symbol (and adjust any value assertions to the
domain's values) so tests always use the domain contract.
- Around line 123-146: Add a test case in the
describe('findPayoutsByProviderUserId') suite that creates multiple PayoutState
entries for the same provider with different created_at timestamps (use
makePayoutState to produce two or more payouts and set distinct createdAt
values), pass them into makePostgresClient (Map keyed by BOOKING_ID variants),
call PostgresPayoutRepository.findPayoutsByProviderUserId(PROVIDER_ID), assert
the result length matches the number inserted, and assert the array is ordered
by created_at DESC (e.g., result[0].createdAt >= result[1].createdAt and so on)
to verify ordering behavior.
In
`@services/platform-api/src/payouts/infrastructure/postgres-payout.repository.ts`:
- Around line 77-145: Replace the duplicated inline column lists in the SQL
strings of findPayoutById, findPayoutsByProviderUserId, and
findPayoutByBookingId with the existing PAYOUT_SELECT_COLUMNS constant to avoid
drift; update each query to interpolate or concatenate PAYOUT_SELECT_COLUMNS
where the SELECT column block currently appears (keep the same WHERE, ORDER BY,
LIMIT clauses and parameters) so all three methods reuse the single source of
truth for selected columns.
In `@services/platform-api/src/reviews/reviews.controller.ts`:
- Around line 18-21: Convert the plain TypeScript type SubmitReviewBody into a
class DTO with validation decorators (e.g., class SubmitReviewBody { rating;
comment }) and apply class-validator rules: validate rating as an integer within
allowed bounds (IsInt, Min, Max) and validate comment as an optional string with
length constraints (IsOptional, IsString, Length). Update any handlers that
consume SubmitReviewBody in reviews.controller.ts to accept the class instance
(so Nest's ValidationPipe can run) and ensure the same DTO name
(SubmitReviewBody) is used so references remain consistent.
- Around line 94-116: The controller assumes reviewsService.getProviderReviews
always succeeds; update ProviderReviewsController.getProviderReviews to handle
potential error responses from reviewsService.getProviderReviews by checking the
returned shape (e.g., result.ok) and, if false, set an appropriate HTTP status
on response (or throw an HttpException) and return an error payload; ensure you
still set the correlationIdHeaderName via response.setHeader and return
result.reviews only when result.ok is true so callers get consistent
success/error behavior.
🪄 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
Run ID: 8ef71c0c-72f4-4b81-a2e0-35cb5d4c216b
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (40)
.agent/plans/phase3-completion-2026-04-02.mdapps/admin-web/package.jsonapps/admin-web/src/features/disputes/dispute-queue-actions.test.tsapps/admin-web/src/features/disputes/dispute-queue-actions.tsapps/admin-web/src/features/disputes/dispute-queue-state.test.tsapps/admin-web/src/features/disputes/dispute-queue-state.tsapps/product-app/src/features/booking/review-screen-actions.test.tsapps/product-app/src/features/booking/review-screen-actions.tsapps/product-app/src/features/booking/review-state.test.tsapps/product-app/src/features/booking/review-state.tspackages/api-client/src/index.tspackages/domain/src/index.tsscripts/smoke/phase3-e2e-smoke.shscripts/smoke/phase3-slice2-payouts-invoices-smoke.shservices/platform-api/src/app.module.tsservices/platform-api/src/bookings/bookings.module.tsservices/platform-api/src/bookings/bookings.service.complete.test.tsservices/platform-api/src/bookings/bookings.service.tsservices/platform-api/src/disputes/disputes.controller.tsservices/platform-api/src/disputes/disputes.module.tsservices/platform-api/src/disputes/disputes.service.test.tsservices/platform-api/src/disputes/disputes.service.tsservices/platform-api/src/disputes/domain/dispute.repository.tsservices/platform-api/src/disputes/infrastructure/dispute-repository.provider.tsservices/platform-api/src/disputes/infrastructure/in-memory-dispute.repository.tsservices/platform-api/src/invoices/infrastructure/invoice-repository.provider.tsservices/platform-api/src/invoices/infrastructure/postgres-invoice.repository.test.tsservices/platform-api/src/invoices/infrastructure/postgres-invoice.repository.tsservices/platform-api/src/invoices/invoices.module.tsservices/platform-api/src/payouts/infrastructure/payout-repository.provider.tsservices/platform-api/src/payouts/infrastructure/postgres-payout.repository.test.tsservices/platform-api/src/payouts/infrastructure/postgres-payout.repository.tsservices/platform-api/src/payouts/payouts.module.tsservices/platform-api/src/reviews/domain/review.repository.tsservices/platform-api/src/reviews/infrastructure/in-memory-review.repository.tsservices/platform-api/src/reviews/infrastructure/review-repository.provider.tsservices/platform-api/src/reviews/reviews.controller.tsservices/platform-api/src/reviews/reviews.module.tsservices/platform-api/src/reviews/reviews.service.test.tsservices/platform-api/src/reviews/reviews.service.ts
✅ Files skipped from review due to trivial changes (7)
- apps/admin-web/src/features/disputes/dispute-queue-state.test.ts
- .agent/plans/phase3-completion-2026-04-02.md
- apps/product-app/src/features/booking/review-state.test.ts
- services/platform-api/src/bookings/bookings.service.complete.test.ts
- apps/admin-web/package.json
- apps/product-app/src/features/booking/review-state.ts
- apps/admin-web/src/features/disputes/dispute-queue-state.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- services/platform-api/src/invoices/infrastructure/invoice-repository.provider.ts
- services/platform-api/src/payouts/payouts.module.ts
- services/platform-api/src/app.module.ts
- services/platform-api/src/invoices/invoices.module.ts
- services/platform-api/src/payouts/infrastructure/payout-repository.provider.ts
- packages/api-client/src/index.ts
- packages/domain/src/index.ts
Hardened disputes authorization/validation and repository wiring, fixed payout/invoice idempotency edge cases, and corrected replay event handling/tests. Also cleaned markdown/Next typed-route artifacts and tightened admin dispute payload guards. Co-Authored-By: Paperclip <noreply@paperclip.ing>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
services/platform-api/src/payouts/payouts.service.ts (1)
39-61:⚠️ Potential issue | 🟠 Major
PayoutCreatedDomainEventis still never emitted.This object currently only feeds the breadcrumb payload, so downstream consumers will never observe a real
payout.createdevent. Please publish/enqueue it through the existing event pipeline instead of only logging it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/platform-api/src/payouts/payouts.service.ts` around lines 39 - 61, The PayoutCreatedDomainEvent object (payoutCreatedEvent) is only being logged via logStructuredBreadcrumb and never emitted to the event pipeline; after constructing payoutCreatedEvent, also publish/enqueue it to the service's domain event pipeline (e.g., call the existing event publisher such as eventBus.publish or enqueueDomainEvent) so downstream consumers receive the 'payout.created' event, while keeping the logStructuredBreadcrumb call for observability. Ensure you import/locate and use the project's canonical publisher (the function/class used elsewhere to emit domain events) and await/handle errors from that publish call.
🧹 Nitpick comments (3)
services/platform-api/src/disputes/disputes.service.test.ts (2)
45-79: Reduce duplicated session factory setup.Line 45–79 repeats the same object shape three times. A single role-parameterized helper will keep tests easier to maintain.
♻️ Proposed refactor
-const makeCustomerSession = (userId = 'customer-1') => { - const createdAt = new Date(); - return { - userId, - role: 'customer' as const, - token: `tok-${userId}`, - email: `${userId}@quickwerk.local`, - createdAt: createdAt.toISOString(), - expiresAt: new Date(createdAt.getTime() + 3600000).toISOString(), - }; -}; - -const makeProviderSession = (userId = 'provider-1') => { - const createdAt = new Date(); - return { - userId, - role: 'provider' as const, - token: `tok-${userId}`, - email: `${userId}@quickwerk.local`, - createdAt: createdAt.toISOString(), - expiresAt: new Date(createdAt.getTime() + 3600000).toISOString(), - }; -}; - -const makeOperatorSession = (userId = 'operator-1') => { +const makeSession = <TRole extends 'customer' | 'provider' | 'operator'>( + role: TRole, + userId: string, +) => { const createdAt = new Date(); return { userId, - role: 'operator' as const, + role, token: `tok-${userId}`, email: `${userId}@quickwerk.local`, createdAt: createdAt.toISOString(), expiresAt: new Date(createdAt.getTime() + 3600000).toISOString(), }; }; + +const makeCustomerSession = (userId = 'customer-1') => makeSession('customer', userId); +const makeProviderSession = (userId = 'provider-1') => makeSession('provider', userId); +const makeOperatorSession = (userId = 'operator-1') => makeSession('operator', userId);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/platform-api/src/disputes/disputes.service.test.ts` around lines 45 - 79, The three duplicated factories makeCustomerSession, makeProviderSession and makeOperatorSession all build the same session shape; replace them with a single parameterized helper (e.g., makeSession(role: 'customer'|'provider'|'operator', userId = '<default>')) that constructs createdAt, token, email, expiresAt and role, then update tests to call makeSession('customer', ...), makeSession('provider', ...), or makeSession('operator', ...) instead of the three separate functions so the shape is maintained in one place.
128-152: Strengthen idempotency assertion to lock the dedupe key.The title says replay is keyed by
(bookingId, reporterUserId), but Line 133–146 submits identical category/description twice. Vary the second payload so this test fails if dedupe ever starts depending on category/description.✅ Proposed test tightening
const second = await service.submitDispute( session, booking.bookingId, - 'no-show', - 'Provider did not arrive.', + 'quality', + 'Different text to prove reporter+booking idempotency.', 'corr-3b', ); expect(first.ok).toBe(true); expect(second.ok).toBe(true); if (first.ok && second.ok) { expect(second.dispute.disputeId).toBe(first.dispute.disputeId); + expect(second.dispute.category).toBe(first.dispute.category); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/platform-api/src/disputes/disputes.service.test.ts` around lines 128 - 152, The test currently calls service.submitDispute twice with identical category/description so it won't catch dedupe keys beyond (bookingId, reporterUserId); update the second submitDispute call in the test (within disputes.service.test.ts) to use a different category and/or description (while keeping the same booking.bookingId and same session reporter user) so the assertion on submitDispute idempotency for submitDispute(...) properly verifies deduping is keyed only by (bookingId, reporterUserId) — look for the submitDispute calls created via createService(), makeCustomerSession(), and createCompletedBooking() and change the second call's category/description to a different value.services/platform-api/src/invoices/infrastructure/in-memory-invoice.repository.ts (1)
13-14: Avoid repeated O(n) scans forbookingIdlookups.Both idempotency and booking lookup scan all invoices. A secondary
bookingId -> invoiceIdmap keeps these paths O(1).♻️ Suggested refactor
export class InMemoryInvoiceRepository implements InvoiceRepository { private readonly invoices = new Map<string, InvoiceRecord>(); + private readonly invoiceIdByBookingId = new Map<string, string>(); @@ - const existing = Array.from(this.invoices.values()).find((i) => i.bookingId === input.bookingId); + const existingInvoiceId = this.invoiceIdByBookingId.get(input.bookingId); + const existing = existingInvoiceId ? this.invoices.get(existingInvoiceId) : undefined; @@ this.invoices.set(invoiceId, this.cloneRecord(record)); + this.invoiceIdByBookingId.set(input.bookingId, invoiceId); @@ - const found = Array.from(this.invoices.values()).find((i) => i.bookingId === bookingId); + const invoiceId = this.invoiceIdByBookingId.get(bookingId); + const found = invoiceId ? this.invoices.get(invoiceId) : undefined;Also applies to: 41-42
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/platform-api/src/invoices/infrastructure/in-memory-invoice.repository.ts` around lines 13 - 14, Replace the repeated O(n) scans over this.invoices for bookingId lookups by adding a secondary index Map (e.g., bookingIdIndex: Map<string, string>) that maps bookingId -> invoiceId; update code paths that currently do Array.from(this.invoices.values()).find(i => i.bookingId === input.bookingId) (used for idempotency checks and booking lookups) to first consult bookingIdIndex and then fetch the invoice from this.invoices by id; ensure bookingIdIndex is maintained (set on create, update when bookingId changes, and delete on remove) so all bookingId and idempotency lookups become O(1).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@services/platform-api/src/invoices/infrastructure/in-memory-invoice.repository.ts`:
- Around line 15-16: The repository is returning references to in-memory Invoice
objects (e.g., the places that currently "return existing;" and other
read-return sites) which allows callers to mutate stored state; fix by cloning
on both storage and return: introduce a helper (e.g., private
cloneInvoice(invoice: Invoice): Invoice) that uses structuredClone (or JSON deep
clone fallback) and use it when inserting/updating the internal map/array and
when returning results from all read methods (the methods that currently return
stored instances and the save/upsert methods), so the repository always stores
and emits clones rather than original object references.
In `@services/platform-api/src/payments/payments.service.ts`:
- Around line 60-66: The call to invoicesService.generateInvoiceForBooking is
falling back to a hardcoded 'General handyman help' when input.requestedService
is missing, which can write incorrect invoice descriptions; either make
requestedService required on the incoming payload (validate/throw if
input.requestedService is falsy) or query the real booking/service data before
calling generateInvoiceForBooking (e.g., call bookingsService.getBookingById or
the appropriate booking lookup using input.bookingId to pull the actual service
name) and pass that value into invoicesService.generateInvoiceForBooking instead
of the hardcoded default.
In `@services/platform-api/src/reviews/reviews.service.ts`:
- Around line 126-145: The getBookingReviews method lacks explicit role
validation and therefore lets 'operator' sessions bypass access checks; update
getBookingReviews to mirror submitReview's role guard by rejecting any
session.role that is not 'customer' or 'provider' (e.g., return { ok: false,
statusCode: 403, error: 'You do not have access to this booking.' }) before
comparing booking.customerUserId/providerUserId, referencing the
getBookingReviews method and SessionRole/AuthSession types to locate where to
add the guard.
---
Duplicate comments:
In `@services/platform-api/src/payouts/payouts.service.ts`:
- Around line 39-61: The PayoutCreatedDomainEvent object (payoutCreatedEvent) is
only being logged via logStructuredBreadcrumb and never emitted to the event
pipeline; after constructing payoutCreatedEvent, also publish/enqueue it to the
service's domain event pipeline (e.g., call the existing event publisher such as
eventBus.publish or enqueueDomainEvent) so downstream consumers receive the
'payout.created' event, while keeping the logStructuredBreadcrumb call for
observability. Ensure you import/locate and use the project's canonical
publisher (the function/class used elsewhere to emit domain events) and
await/handle errors from that publish call.
---
Nitpick comments:
In `@services/platform-api/src/disputes/disputes.service.test.ts`:
- Around line 45-79: The three duplicated factories makeCustomerSession,
makeProviderSession and makeOperatorSession all build the same session shape;
replace them with a single parameterized helper (e.g., makeSession(role:
'customer'|'provider'|'operator', userId = '<default>')) that constructs
createdAt, token, email, expiresAt and role, then update tests to call
makeSession('customer', ...), makeSession('provider', ...), or
makeSession('operator', ...) instead of the three separate functions so the
shape is maintained in one place.
- Around line 128-152: The test currently calls service.submitDispute twice with
identical category/description so it won't catch dedupe keys beyond (bookingId,
reporterUserId); update the second submitDispute call in the test (within
disputes.service.test.ts) to use a different category and/or description (while
keeping the same booking.bookingId and same session reporter user) so the
assertion on submitDispute idempotency for submitDispute(...) properly verifies
deduping is keyed only by (bookingId, reporterUserId) — look for the
submitDispute calls created via createService(), makeCustomerSession(), and
createCompletedBooking() and change the second call's category/description to a
different value.
In
`@services/platform-api/src/invoices/infrastructure/in-memory-invoice.repository.ts`:
- Around line 13-14: Replace the repeated O(n) scans over this.invoices for
bookingId lookups by adding a secondary index Map (e.g., bookingIdIndex:
Map<string, string>) that maps bookingId -> invoiceId; update code paths that
currently do Array.from(this.invoices.values()).find(i => i.bookingId ===
input.bookingId) (used for idempotency checks and booking lookups) to first
consult bookingIdIndex and then fetch the invoice from this.invoices by id;
ensure bookingIdIndex is maintained (set on create, update when bookingId
changes, and delete on remove) so all bookingId and idempotency lookups become
O(1).
🪄 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
Run ID: 177b1293-a807-45a7-9bf7-b7dfabff0cd0
📒 Files selected for processing (17)
.agent/plans/phase3-slice2-payouts-invoices-2026-04-01.mdapps/admin-web/next-env.d.tsapps/admin-web/src/features/disputes/dispute-queue-actions.tsservices/platform-api/src/bookings/bookings.service.tsservices/platform-api/src/disputes/disputes.module.tsservices/platform-api/src/disputes/disputes.service.test.tsservices/platform-api/src/disputes/disputes.service.tsservices/platform-api/src/disputes/infrastructure/dispute-repository.provider.tsservices/platform-api/src/invoices/infrastructure/in-memory-invoice.repository.tsservices/platform-api/src/invoices/invoices.service.tsservices/platform-api/src/payments/payments.service.tsservices/platform-api/src/payouts/infrastructure/in-memory-payout.repository.tsservices/platform-api/src/payouts/infrastructure/postgres-payout.repository.test.tsservices/platform-api/src/payouts/infrastructure/postgres-payout.repository.tsservices/platform-api/src/payouts/payouts.service.tsservices/platform-api/src/reviews/infrastructure/review-repository.provider.tsservices/platform-api/src/reviews/reviews.service.ts
✅ Files skipped from review due to trivial changes (2)
- apps/admin-web/next-env.d.ts
- .agent/plans/phase3-slice2-payouts-invoices-2026-04-01.md
🚧 Files skipped from review as they are similar to previous changes (9)
- services/platform-api/src/disputes/disputes.module.ts
- services/platform-api/src/reviews/infrastructure/review-repository.provider.ts
- services/platform-api/src/payouts/infrastructure/postgres-payout.repository.test.ts
- apps/admin-web/src/features/disputes/dispute-queue-actions.ts
- services/platform-api/src/payouts/infrastructure/in-memory-payout.repository.ts
- services/platform-api/src/disputes/infrastructure/dispute-repository.provider.ts
- services/platform-api/src/invoices/invoices.service.ts
- services/platform-api/src/bookings/bookings.service.ts
- services/platform-api/src/disputes/disputes.service.ts
Add clone/index safety for in-memory invoices, require requestedService for payment capture invoice generation, and enforce booking-review role checks with tests. Co-Authored-By: Paperclip <noreply@paperclip.ing>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
services/platform-api/src/reviews/reviews.service.ts (1)
83-83: Consider simplifying comment normalization to avoid redundanttrim()call.The current expression calls
trim()twice when comment is non-empty whitespace.♻️ Suggested simplification
- comment: input.comment?.trim() ? input.comment.trim() : null, + comment: input.comment?.trim() || null,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/platform-api/src/reviews/reviews.service.ts` at line 83, The comment normalization calls trim() twice; change the assignment in the reviews service so you trim once and reuse the result (referencing input.comment and the comment property in the code that sets comment:), e.g. compute a single variable like trimmedComment = input.comment?.trim() and then set comment to trimmedComment ? trimmedComment : null (or use the equivalent one-line expression that trims once and falls back to null).services/platform-api/src/payments/payments.service.ts (1)
69-73: Add payout-failure breadcrumb for post-processing parity.If
createPayoutForCapturethrows, this path exits without a dedicated post-processing failure breadcrumb, unlike the invoice path on Lines 75-99. Add symmetric logging for payout failures to improve incident triage.Proposed fix
- await this.payoutsService.createPayoutForCapture( - result.payment, - input.providerUserId, - input.correlationId, - ); + try { + await this.payoutsService.createPayoutForCapture( + result.payment, + input.providerUserId, + input.correlationId, + ); + } catch (error) { + logStructuredBreadcrumb({ + event: 'payment.capture.post-processing', + correlationId: input.correlationId, + status: 'failed', + details: { + paymentId: result.payment.paymentId, + bookingId: input.bookingId, + reason: 'payout-generation-failed', + error: error instanceof Error ? error.message : String(error), + }, + }); + throw error; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/platform-api/src/payments/payments.service.ts` around lines 69 - 73, The call to payoutsService.createPayoutForCapture lacks the symmetric post-processing failure breadcrumb used by the invoice path; wrap the createPayoutForCapture invocation in a try/catch inside the same method (payments.service.ts), and in the catch block emit the same kind of "post-processing failure" breadcrumb used for invoices (include identifiers like result.payment.id/payment, input.providerUserId, input.correlationId and the caught error), then rethrow the error so behavior stays the same; reference createPayoutForCapture and the surrounding payment post-processing flow to mirror the invoice breadcrumb handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@services/platform-api/src/payments/payments.service.ts`:
- Around line 23-36: The code currently throws a generic Error when
requestedService is missing in the capture flow (see requestedService check and
logStructuredBreadcrumb call); replace the thrown Error with Nest's
BadRequestException to return HTTP 400, import BadRequestException from
`@nestjs/common` at the top of the file, and throw new BadRequestException(`Failed
to capture payment for booking ${input.bookingId}: missing requestedService`) in
place of the existing throw to preserve the message and ensure correct HTTP
semantics.
---
Nitpick comments:
In `@services/platform-api/src/payments/payments.service.ts`:
- Around line 69-73: The call to payoutsService.createPayoutForCapture lacks the
symmetric post-processing failure breadcrumb used by the invoice path; wrap the
createPayoutForCapture invocation in a try/catch inside the same method
(payments.service.ts), and in the catch block emit the same kind of
"post-processing failure" breadcrumb used for invoices (include identifiers like
result.payment.id/payment, input.providerUserId, input.correlationId and the
caught error), then rethrow the error so behavior stays the same; reference
createPayoutForCapture and the surrounding payment post-processing flow to
mirror the invoice breadcrumb handling.
In `@services/platform-api/src/reviews/reviews.service.ts`:
- Line 83: The comment normalization calls trim() twice; change the assignment
in the reviews service so you trim once and reuse the result (referencing
input.comment and the comment property in the code that sets comment:), e.g.
compute a single variable like trimmedComment = input.comment?.trim() and then
set comment to trimmedComment ? trimmedComment : null (or use the equivalent
one-line expression that trims once and falls back to null).
🪄 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
Run ID: fdeec6ea-6f30-4884-b377-285efe951f82
📒 Files selected for processing (6)
services/platform-api/src/invoices/infrastructure/in-memory-invoice.repository.tsservices/platform-api/src/payments/domain/payment.repository.tsservices/platform-api/src/payments/payments.service.test.tsservices/platform-api/src/payments/payments.service.tsservices/platform-api/src/reviews/reviews.service.test.tsservices/platform-api/src/reviews/reviews.service.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- services/platform-api/src/payments/domain/payment.repository.ts
- services/platform-api/src/payments/payments.service.test.ts
- services/platform-api/src/invoices/infrastructure/in-memory-invoice.repository.ts
Comment on lines
+23
to
+36
| if (!requestedService) { | ||
| logStructuredBreadcrumb({ | ||
| event: 'payment.capture.write', | ||
| correlationId: input.correlationId, | ||
| status: 'failed', | ||
| details: { | ||
| bookingId: input.bookingId, | ||
| amountCents: input.amountCents, | ||
| currency: input.currency, | ||
| reason: 'missing-requested-service', | ||
| }, | ||
| }); | ||
| throw new Error(`Failed to capture payment for booking ${input.bookingId}: missing requestedService`); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether generic Error is globally remapped (if yes, severity may be lower).
rg -n --type=ts -C3 'useGlobalFilters|APP_FILTER|ExceptionFilter|@Catch\('
rg -n --type=ts -C3 'catch\s*\(\s*error\s*:\s*Error|instanceof Error|HttpException'Repository: MarkoAtc/QuickWerk
Length of output: 50374
Use BadRequestException for invalid requestedService validation.
Line 35 throws a generic Error, which returns HTTP 500 in NestJS. This is an input-validation failure and should return HTTP 400 (Bad Request). Import and use BadRequestException instead:
-import { Inject, Injectable } from '@nestjs/common';
+import { BadRequestException, Inject, Injectable } from '@nestjs/common';- throw new Error(`Failed to capture payment for booking ${input.bookingId}: missing requestedService`);
+ throw new BadRequestException(
+ `Failed to capture payment for booking ${input.bookingId}: missing requestedService`,
+ );📝 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.
Suggested change
| if (!requestedService) { | |
| logStructuredBreadcrumb({ | |
| event: 'payment.capture.write', | |
| correlationId: input.correlationId, | |
| status: 'failed', | |
| details: { | |
| bookingId: input.bookingId, | |
| amountCents: input.amountCents, | |
| currency: input.currency, | |
| reason: 'missing-requested-service', | |
| }, | |
| }); | |
| throw new Error(`Failed to capture payment for booking ${input.bookingId}: missing requestedService`); | |
| } | |
| if (!requestedService) { | |
| logStructuredBreadcrumb({ | |
| event: 'payment.capture.write', | |
| correlationId: input.correlationId, | |
| status: 'failed', | |
| details: { | |
| bookingId: input.bookingId, | |
| amountCents: input.amountCents, | |
| currency: input.currency, | |
| reason: 'missing-requested-service', | |
| }, | |
| }); | |
| throw new BadRequestException( | |
| `Failed to capture payment for booking ${input.bookingId}: missing requestedService`, | |
| ); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/platform-api/src/payments/payments.service.ts` around lines 23 - 36,
The code currently throws a generic Error when requestedService is missing in
the capture flow (see requestedService check and logStructuredBreadcrumb call);
replace the thrown Error with Nest's BadRequestException to return HTTP 400,
import BadRequestException from `@nestjs/common` at the top of the file, and throw
new BadRequestException(`Failed to capture payment for booking
${input.bookingId}: missing requestedService`) in place of the existing throw to
preserve the message and ensure correct HTTP semantics.
Co-Authored-By: Paperclip <noreply@paperclip.ing>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
services/platform-api/src/payouts/payouts.service.test.ts (1)
64-72: Test name doesn't match test behavior.The test is named "emits PayoutCreatedDomainEvent breadcrumb on new payout" but only verifies the payout was created. It doesn't actually verify that
logStructuredBreadcrumbwas called with the expected event details.Consider either:
- Renaming the test to reflect what it actually tests
- Adding a spy on
logStructuredBreadcrumbto verify the breadcrumb was logged with correct payloadOption 1: Rename to match behavior
- it('emits PayoutCreatedDomainEvent breadcrumb on new payout', async () => { + it('creates payout for a new booking', async () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/platform-api/src/payouts/payouts.service.test.ts` around lines 64 - 72, The test name asserts a breadcrumb emission but the test only verifies payout creation; either rename the test to reflect that it only checks createPayoutForCapture produced a payout (e.g., "creates payout for capture") or add a spy/assertion on logStructuredBreadcrumb to verify it was called with the expected PayoutCreatedDomainEvent payload after calling createPayoutForCapture; locate the test using the function createPayoutForCapture and the current test case, then either change the it(...) description or instrument logStructuredBreadcrumb (mock/spy) and add an expect(spy).toHaveBeenCalledWith(...) for the event details.services/platform-api/src/payouts/payouts.service.ts (1)
37-42:replayedlogic is partially redundant with Postgres conflict validation.The
PostgresPayoutRepository.createPayoutthrows an error if an existing payout has differentproviderUserId,paymentId,amountCents, orcurrency. Therefore, ifcreatePayoutreturns successfully, those fields will always match. Thereplayedflag will only ever betruewhencreatedAtdiffers (indicating an existing payout was returned).Consider simplifying to:
-const replayed = - payout.providerUserId !== providerUserId || - payout.paymentId !== payment.paymentId || - payout.amountCents !== payment.amountCents || - payout.currency !== payment.currency || - payout.createdAt !== createdAt; +const replayed = payout.createdAt !== createdAt;This is still safe because any field mismatch would have thrown an error.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/platform-api/src/payouts/payouts.service.ts` around lines 37 - 42, The replayed flag currently redundantly compares providerUserId, paymentId, amountCents, and currency against payment even though PostgresPayoutRepository.createPayout will throw on any mismatch; change the replayed calculation in payouts.service.ts (where replayed is computed) to only check createdAt inequality (e.g., payout.createdAt !== createdAt) and remove the other comparisons so replayed reflects only the returned existing-payout timestamp difference after createPayout returns successfully.services/platform-api/src/payouts/infrastructure/postgres-payout.repository.ts (1)
121-131: Invalid cursor silently returns empty results.When the cursor ID doesn't exist or belongs to a different provider, the subquery returns
NULL, causing the row-value comparison to fail, returning zero rows. This is likely acceptable behavior (invalid cursors shouldn't return data), but consider whether returning a specific error or the first page might be preferable for API consumers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/platform-api/src/payouts/infrastructure/postgres-payout.repository.ts` around lines 121 - 131, The SQL paging clause using the cursor ($2::uuid) can silently return no rows when the cursor id doesn't exist or belongs to a different provider; update the repository (postgres-payout.repository.ts) to validate the cursor before running the paged query: in the method that builds/executing the query (e.g., listPayouts / findPayouts), add a pre-check that SELECTs the payout by id and provider_user_id and if not found either throw a clear InvalidCursorError (recommended) or treat it as a null cursor to return the first page; alternatively, if you prefer SQL-only handling, replace the NULL-producing subquery with an EXISTS check (e.g., use WHERE NOT EXISTS(...) THEN treat cursor as NULL) so the comparison doesn't silently fail—pick one approach and implement consistent behavior and error type for callers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@services/platform-api/src/payouts/infrastructure/postgres-payout.repository.ts`:
- Around line 121-131: The SQL paging clause using the cursor ($2::uuid) can
silently return no rows when the cursor id doesn't exist or belongs to a
different provider; update the repository (postgres-payout.repository.ts) to
validate the cursor before running the paged query: in the method that
builds/executing the query (e.g., listPayouts / findPayouts), add a pre-check
that SELECTs the payout by id and provider_user_id and if not found either throw
a clear InvalidCursorError (recommended) or treat it as a null cursor to return
the first page; alternatively, if you prefer SQL-only handling, replace the
NULL-producing subquery with an EXISTS check (e.g., use WHERE NOT EXISTS(...)
THEN treat cursor as NULL) so the comparison doesn't silently fail—pick one
approach and implement consistent behavior and error type for callers.
In `@services/platform-api/src/payouts/payouts.service.test.ts`:
- Around line 64-72: The test name asserts a breadcrumb emission but the test
only verifies payout creation; either rename the test to reflect that it only
checks createPayoutForCapture produced a payout (e.g., "creates payout for
capture") or add a spy/assertion on logStructuredBreadcrumb to verify it was
called with the expected PayoutCreatedDomainEvent payload after calling
createPayoutForCapture; locate the test using the function
createPayoutForCapture and the current test case, then either change the it(...)
description or instrument logStructuredBreadcrumb (mock/spy) and add an
expect(spy).toHaveBeenCalledWith(...) for the event details.
In `@services/platform-api/src/payouts/payouts.service.ts`:
- Around line 37-42: The replayed flag currently redundantly compares
providerUserId, paymentId, amountCents, and currency against payment even though
PostgresPayoutRepository.createPayout will throw on any mismatch; change the
replayed calculation in payouts.service.ts (where replayed is computed) to only
check createdAt inequality (e.g., payout.createdAt !== createdAt) and remove the
other comparisons so replayed reflects only the returned existing-payout
timestamp difference after createPayout returns successfully.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2908b8d0-c232-4726-826e-38028cccd13c
📒 Files selected for processing (10)
apps/product-app/src/features/payouts/payout-screen-actions.test.tsapps/product-app/src/features/payouts/payout-screen-actions.tspackages/api-client/src/index.tsservices/platform-api/src/payouts/domain/payout.repository.tsservices/platform-api/src/payouts/infrastructure/in-memory-payout.repository.tsservices/platform-api/src/payouts/infrastructure/postgres-payout.repository.test.tsservices/platform-api/src/payouts/infrastructure/postgres-payout.repository.tsservices/platform-api/src/payouts/payouts.controller.tsservices/platform-api/src/payouts/payouts.service.test.tsservices/platform-api/src/payouts/payouts.service.ts
✅ Files skipped from review due to trivial changes (1)
- services/platform-api/src/payouts/domain/payout.repository.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- apps/product-app/src/features/payouts/payout-screen-actions.test.ts
- apps/product-app/src/features/payouts/payout-screen-actions.ts
- services/platform-api/src/payouts/infrastructure/in-memory-payout.repository.ts
- services/platform-api/src/payouts/payouts.controller.ts
- packages/api-client/src/index.ts
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
1 participant
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.
Summary
Phase 3 Slice 2 closes Milestones 3c and 3d from the Phase 3 Payments, Payouts & Document Workflows plan.
PayoutCreatedDomainEvent; payout list + detail endpoints addedGET /api/v1/bookings/:id/invoiceaddedAlso includes the carry-over fix from
fix/ts-build-crash-payment-relay-types: correctsPaymentRecord.replayedaccessor + relay attempt policy union type.What's in this PR
Domain (
packages/domain)PayoutStatus,PayoutRecord,PayoutCreatedDomainEvent,PayoutCreatedWorkerEnvelopeInvoiceLineItem,InvoiceStatus,InvoiceRecordPayoutsModule (
services/platform-api/src/payouts/)PayoutRepositoryinterface +InMemoryPayoutRepositoryPayoutsService:createPayoutForCapture(idempotent),getMyPayouts,getPayoutByIdPayoutsController:GET /api/v1/providers/me/payouts+GET /api/v1/providers/me/payouts/:idPaymentsService— payout created automatically after payment captureInvoicesModule (
services/platform-api/src/invoices/)InvoiceRepositoryinterface +InMemoryInvoiceRepositoryInvoicesService:generateInvoiceForBooking(idempotent),getInvoiceByBookingIdInvoicesController:GET /api/v1/bookings/:id/invoicePaymentsService— invoice generated after payout creationAPI Client (
packages/api-client)providerPayoutApiRoutes,bookingInvoiceApiRoutescreateGetMyPayoutsRequest,createGetPayoutDetailRequest,createGetBookingInvoiceRequestProduct App (
apps/product-app)payout-state.ts—PayoutLoadStateunion typepayout-screen-actions.ts—loadMyPayoutsactionBackground Workers
payment-captured.worker.ts— extended withpayment.captured.worker.invoice-generation.stubbreadcrumbTest plan
pnpm check— zero type errors across workspacepnpm --filter @quickwerk/platform-api test— 164 passing (7 new payout tests, 7 new invoice tests)pnpm --filter @quickwerk/product-app test— 146 passing (10 new payout state/action tests)pnpm --filter @quickwerk/background-workers test— 26 passingbookings.service.complete.test.ts("idempotent: completing again by same provider returns replayed payment") — exists onmainbefore this branch, tracked separatelyNew endpoints
GET/api/v1/providers/me/payoutsGET/api/v1/providers/me/payouts/:idGET/api/v1/bookings/:id/invoicePhase 3 progress
🤖 Generated with Claude Code
Summary by CodeRabbit