(SP: 3) [SHOP] Harden Shop launch readiness: fail-closed checkout, refund containment, and shipping admin guards#405
Merged
ViktorSvertoka merged 10 commits intodevelopfrom Mar 17, 2026
Conversation
… payment contracts
liudmylasovetovs
requested review from
AM1007 and
ViktorSvertoka
as code owners
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Contributor
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughDisables Monobank and return refunds at runtime, adds a Stripe refund reconciliation subsystem, introduces provider-aware checkout selection with fail‑closed behavior, threads refund-containment into shipping eligibility/workers/admin actions, and updates many tests and janitor/admin reconcile routes. Changes
Sequence DiagramssequenceDiagram
participant Client
participant Checkout as Checkout Route
participant Payments as Payments Resolver
participant PSP as PSP (Stripe/Monobank)
participant DB as Database
Client->>Checkout: POST /checkout (provider?, method, locale)
Checkout->>Payments: resolveCheckoutProviderCandidates(requestedProvider, requestedMethod, currency)
Payments-->>Checkout: resolvedProvider(s)
alt Provider available
Checkout->>PSP: create/payment-init (provider-specific)
PSP-->>Checkout: payment ids / errors
Checkout->>DB: create order with paymentProvider & PSP ids
DB-->>Checkout: created order
Checkout-->>Client: 200 OK (order + payment details)
else No provider available
Checkout-->>Client: 503 PSP_UNAVAILABLE
end
sequenceDiagram
participant StripeWebhook as Stripe Webhook
participant Reconciler as Refund Reconciler
participant PSP as Stripe PSP
participant DB as Database
participant Shipping as Shipping pipeline
StripeWebhook->>Reconciler: refund event (refundId, chargeId)
Reconciler->>PSP: retrieveRefund(refundId)
PSP-->>Reconciler: refund status (succeeded/failed/pending)
Reconciler->>DB: load order + PSP metadata (containment snapshot)
alt refund succeeded
Reconciler->>DB: finalizeStripeRefundSuccess (mark refunded, emit payment_event)
Reconciler->>Shipping: restock / cancel shipments
else refund failed/canceled
Reconciler->>DB: restoreStripeRefundFailure (restore paid, replay shipping)
Reconciler->>Shipping: re-queue shipments from containment snapshot
else refund pending / unknown
Reconciler->>DB: mark psp_status_reason = REFUND_REQUESTED, close queued ship work
end
Reconciler-->>StripeWebhook: ack (200)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 42745c1dc2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
frontend/app/api/shop/webhooks/stripe/route.ts (1)
867-887:⚠️ Potential issue | 🟠 MajorKeep omitted-provider retries idempotent across PSP availability changes.
When the client omits
paymentProvider, these lines resolve the rail from current availability and then persist that resolved rail/method throughcreateOrderWithItems(). A UAH checkout can therefore hash asmonobank/monobank_invoiceon the first request andstripe/stripe_cardon a retry after Monobank goes down, so the sameIdempotency-Keyturns intoIDEMPOTENCY_CONFLICTinstead of recovering the original order.Also applies to: 889-891, 989-992, 1176-1178
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/api/shop/webhooks/stripe/route.ts` around lines 867 - 887, When paymentProvider is omitted the code currently recomputes the provider/method on each request (which can flip when PSP availability changes), so compute and freeze the resolved provider/method once at the start (e.g., set a resolvedPaymentProvider and resolvedPaymentMethod variable when first deriving them from the incoming payload and availability lookup) and always pass those frozen values into createOrderWithItems() and any later logic that persists the order; update the branches that handle paymentIntentId/refundObject to use the same frozen resolved values (reference paymentIntentId, refundObject and createOrderWithItems) so retries use the originally resolved rail/method rather than re-resolving.frontend/lib/services/shop/shipping/admin-actions.ts (1)
584-621:⚠️ Potential issue | 🟠 MajorPreserve the no-op fast path for repeated shipping actions.
These new shipment checks run before the existing
shipping_status === 'shipped'/'delivered'early returns. A duplicate admin request can now start failing if the shipment row drifted after the first successful action, even though no transition is being attempted.🐛 Suggested fix
if (args.action === 'mark_shipped') { - assertShipmentSupportsManualFulfillment(state, 'mark_shipped'); - if (state.shipping_status === 'shipped') { await appendAuditEntry({ orderId: args.orderId, entry: { @@ changed: false, action: args.action, }; } + + assertShipmentSupportsManualFulfillment(state, 'mark_shipped'); if (!isShippingStatusTransitionAllowed(state.shipping_status, 'shipped')) { throw new ShippingAdminActionError( 'INVALID_SHIPPING_TRANSITION', 'mark_shipped is allowed only from label_created.', @@ - assertShipmentSupportsManualFulfillment(state, 'mark_delivered'); - if (state.shipping_status === 'delivered') { await appendAuditEntry({ orderId: args.orderId, entry: { @@ changed: false, action: args.action, }; } + + assertShipmentSupportsManualFulfillment(state, 'mark_delivered'); if (!isShippingStatusTransitionAllowed(state.shipping_status, 'delivered')) { throw new ShippingAdminActionError( 'INVALID_SHIPPING_TRANSITION', 'mark_delivered is allowed only from shipped.',Also applies to: 684-720
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/services/shop/shipping/admin-actions.ts` around lines 584 - 621, The new shipment-row validation runs before the existing early-return fast paths causing duplicate admin requests to fail; restore the no-op fast path by moving the checks that validate the shipment row so they occur after the existing shipping_status === 'shipped' / 'delivered' early-return blocks. Concretely, in the handler for args.action === 'mark_shipped' (and the analogous block around lines 684-720), ensure the assertShipmentSupportsManualFulfillment and any shipment-row consistency checks are executed only after you perform the existing shipping_status === 'shipped' audit append (appendAuditEntry / buildShippingAdminAuditDedupe) and return the no-change response, so repeated identical admin requests still hit the preserved fast path.frontend/lib/tests/shop/order-items-snapshot-immutable.test.ts (1)
133-202:⚠️ Potential issue | 🟠 MajorMove the DB teardown to an outer
finally.If checkout fails before Line 207—
503, invalid JSON, missingorderId, etc.—cleanupByIds()never runs and the seeded product/price rows leak into the shared test database. SincecleanupByIds()already tolerates an absentorderId, it should be called from an outerfinally.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/tests/shop/order-items-snapshot-immutable.test.ts` around lines 133 - 202, The test currently only calls cleanupByIds() after the happy-path assertions, so failures before orderId extraction leak seeded rows; refactor the test to wrap the whole operation in a try/finally (outer finally) and call cleanupByIds(productId, priceId, orderId) from that finally block so cleanup always runs even if checkoutPOST(req) throws, res.json() is invalid, or orderId is missing; keep the existing local variables (productId, priceId, idem, orderId) and ensure orderId is passed (or undefined) into cleanupByIds so its tolerant behavior remains intact.frontend/lib/tests/shop/checkout-concurrency-stock1.test.ts (1)
119-296:⚠️ Potential issue | 🟠 MajorAlways clean up seeded checkout data in a
finally.If any assertion before Line 268 fails, this test skips teardown and leaves behind products, inventory moves, and the winning order. That leaks mutable stock state into later tests. Wrap the whole scenario in
try/finallyand keep the existing cleanup inside thefinally.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/tests/shop/checkout-concurrency-stock1.test.ts` around lines 119 - 296, Wrap the entire test body (from after inserting the product/productPrices through the assertions and before the current cleanup) in a try block and move the current cleanup logic (the code that queries orderItems, deletes orderItems, inventoryMoves, productPrices, orders, and products) into a finally block so it always runs; keep the special-case rethrow behavior for CI by rethrowing the caught error after cleanup (or capture the original error and rethrow it in the finally), and ensure references to productId, db.delete(...), inventoryMoves, productPrices, orderItems and orders remain unchanged so teardown reliably cleans seeded data even if assertions fail.frontend/lib/services/shop/shipping/shipments-worker.ts (2)
693-749:⚠️ Potential issue | 🔴 CriticalRe-check refund containment at the success write boundary.
A refund request can land after this eligibility check and before
createInternetDocument()/markSucceeded(). In that window the worker still creates the label and advances the order tolabel_created, because the success path only guards on shipping-state transitions. That leaves containment fail-open for the in-flight case.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/services/shop/shipping/shipments-worker.ts` around lines 693 - 749, Re-check the order refund/containment state immediately before proceeding to create labels: after running evaluateOrderShippingEligibility and before calling createInternetDocument/markSucceeded, fetch the latest order state (or re-run evaluateOrderShippingEligibility against the latest order snapshot) and confirm there is no refund/requested refund; if a refund is now present, throw the appropriate buildFailure instead of creating a label. Update the flow around evaluateOrderShippingEligibility, createInternetDocument, and markSucceeded to perform this final containment check using the same identifying fields on details (e.g., refund status fields) and abort before calling markSucceeded/createInternetDocument when a refund has appeared.
445-450:⚠️ Potential issue | 🟠 MajorDon’t exclude expired
processingrows from reclamation.This filter now blocks stale
processingshipments from being re-claimed after an order becomes refund-contained. Those rows stay stuck inprocessingwith an expired lease forever, because the worker never reachesprocessClaimedShipment()to downgrade them. Keep the eligibility gate forqueued/failed, but let expiredprocessingrows through so recovery still works.Possible fix
- and ${orderShippingEligibilityWhereSql({ - paymentStatusColumn: sql`o.payment_status`, - orderStatusColumn: sql`o.status`, - inventoryStatusColumn: sql`o.inventory_status`, - pspStatusReasonColumn: sql`o.psp_status_reason`, - })} + and ( + s.status = 'processing' + or ${orderShippingEligibilityWhereSql({ + paymentStatusColumn: sql`o.payment_status`, + orderStatusColumn: sql`o.status`, + inventoryStatusColumn: sql`o.inventory_status`, + pspStatusReasonColumn: sql`o.psp_status_reason`, + })} + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/services/shop/shipping/shipments-worker.ts` around lines 445 - 450, The current eligibility filter call to orderShippingEligibilityWhereSql (used in the query that precedes processClaimedShipment) excludes all "processing" rows, which prevents reclaiming stale processing shipments with expired leases; update the logic so that orderShippingEligibilityWhereSql keeps its filtering for queued/failed but does not exclude processing rows whose lease has expired (e.g., add a condition/flag to orderShippingEligibilityWhereSql to allow processing rows when lease_expires_at <= now or when a passed-in allowExpiredProcessing=true), then use that updated call in the query so processClaimedShipment can still be reached to downgrade expired processing rows.
🧹 Nitpick comments (4)
frontend/lib/services/orders/refund.ts (1)
27-32: Consider extracting sharedreadRowshelper.This helper duplicates
readDbRowsinmonobank-webhook.ts(line 154). Consider extracting to a shared utility (e.g.,@/lib/db/utils) to reduce duplication.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/services/orders/refund.ts` around lines 27 - 32, The helper function readRows duplicates logic found in readDbRows (in monobank-webhook.ts); extract this logic into a shared utility (e.g., create a function named readRows in a new module like "@/lib/db/utils") and replace the local readRows and the existing readDbRows with imports from that module; update the files that used the previous implementations (frontend/lib/services/orders/refund.ts and monobank-webhook.ts) to import the shared readRows, ensuring the exported function signature remains generic (readRows<T>(res: unknown): T[]) and behavior unchanged.frontend/app/api/shop/admin/returns/[id]/refund/route.ts (1)
74-79: Make the post-refund branch explicitly unreachable.This handler now depends on
refundReturnRequest()throwing. If that function ever resolves again, the route will silently emitINTERNAL_ERRORinstead of a real success response.♻️ Proposed cleanup
await refundReturnRequest({ returnRequestId: returnRequestIdForLog, actorUserId: typeof admin.id === 'string' ? admin.id : null, requestId, }); - return noStoreJson({ code: 'INTERNAL_ERROR' }, 500); + throw new Error('refundReturnRequest unexpectedly resolved');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/api/shop/admin/returns/`[id]/refund/route.ts around lines 74 - 79, The post-refund branch is currently reachable if refundReturnRequest() ever resolves, causing a silent INTERNAL_ERROR response; after calling refundReturnRequest({ returnRequestId: ..., actorUserId: ..., requestId }), make the fallback explicitly unreachable by throwing an error (e.g., throw new Error('refundReturnRequest resolved unexpectedly')) instead of returning noStoreJson, or replace the noStoreJson fallback with an explicit thrown Response so the route cannot silently succeed; update the code referencing refundReturnRequest and the current noStoreJson fallback accordingly.frontend/lib/tests/shop/stripe-refund-convergence.test.ts (1)
231-247: Make the second-pass expectation strict.Allowing
finalized_successon the second reconciliation weakens the regression for “no double finalization” and can still hide duplicate terminal side effects. Prefer assertingnoophere, or explicitly counting the terminal writes/events as well.♻️ Proposed tightening
const second = await reconcileStripeRefundOrder({ orderId: seeded.orderId, }); - expect(['finalized_success', 'noop']).toContain(second); + expect(second).toBe('noop');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/tests/shop/stripe-refund-convergence.test.ts` around lines 231 - 247, The test currently allows the second call to reconcileStripeRefundOrder({ orderId: seeded.orderId }) to return either 'finalized_success' or 'noop', which weakens the regression; change the assertion to require 'noop' only (i.e., expect the variable named second to strictly equal 'noop') and ensure any terminal side-effects are explicitly asserted (you already check stockRestored/restockedAt and countMoveKey(releaseKey) — keep those assertions to confirm no duplicate terminal writes). Update the assertion around reconcileStripeRefundOrder in stripe-refund-convergence.test.ts accordingly so the second pass must be 'noop'.frontend/lib/tests/shop/admin-shipping-payment-gate.test.ts (1)
122-130: Consider persistingpspStatusReasonon the direct seed path too.Line 129 only writes the new field when
requiresPostInsertBlockedTransitionis true. If this helper later seeds an already-shippable order with some other non-null PSP reason, the row won't match the requested fixture state.♻️ Small fixture-consistency tweak
await db.insert(orders).values({ id: orderId, totalAmountMinor: 1000, totalAmount: toDbMoney(1000), currency: 'USD', paymentProvider: 'stripe', paymentStatus: seedPaymentStatus, status: seedOrderStatus, inventoryStatus: seedInventoryStatus, + pspStatusReason: requiresPostInsertBlockedTransition + ? null + : (args.pspStatusReason ?? null), shippingRequired: true, shippingPayer: 'customer', shippingProvider: 'nova_poshta', shippingMethodCode: 'NP_WAREHOUSE', shippingAmountMinor: null,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/tests/shop/admin-shipping-payment-gate.test.ts` around lines 122 - 130, The new pspStatusReason field is only persisted in the post-insert transition branch; ensure the direct-seed path also writes it so fixtures match expected state by adding pspStatusReason (args.pspStatusReason ?? null) to the .set(...) payload used when !requiresPostInsertBlockedTransition (the same update flow that calls db.update(orders).set(...)). Locate the branch guarded by requiresPostInsertBlockedTransition and mirror the pspStatusReason handling into the other update/insert path so both code paths persist the field consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/app/api/shop/webhooks/stripe/route.ts`:
- Around line 533-538: The replay/fallback branch for handling
payment_intent.succeeded still uses only the old checks (shippingRequired,
shippingProvider, shippingMethodCode, inventoryStatus) and needs the same
orderShippingEligibilityWhereSql gating used in the main enqueue SQL; update the
manual requeue SQL/where clause in the replay path to include the result of
orderShippingEligibilityWhereSql (pass the same column aliases used elsewhere:
paymentStatusColumn: sql`o.payment_status`, orderStatusColumn: sql`o.status`,
inventoryStatusColumn: sql`o.inventory_status`, pspStatusReasonColumn:
sql`o.psp_status_reason`) so applyStripePaidAndQueueShipmentAtomic() and the
replay fallback share identical eligibility logic. Ensure the replay branch that
reopens shipping respects psp_status_reason='REFUND_REQUESTED' by incorporating
that generated SQL fragment into its WHERE condition.
- Around line 1809-1853: The new restore branch for charge.refund.updated is
currently unreachable because the earlier isRefundChargeIdOnly(refund) fast-path
acks those events before we can handle REFUND_REQUESTED orders with
failed/canceled refunds; modify the control flow so events with refund.charge as
an ID still hit the restore logic when the order has pspStatusReason ===
'REFUND_REQUESTED' and refund.status is 'failed' or 'canceled' — either move the
block that calls buildPspMetadata / mergePspMetadata /
restoreStripeRefundFailure / logWebhookEvent / ack() to run before the
isRefundChargeIdOnly(refund) fast-path, or change the
isRefundChargeIdOnly(refund) check to skip acking and allow processing when
order.pspStatusReason === 'REFUND_REQUESTED' and refund.status is
failed/canceled, ensuring restoreStripeRefundFailure is invoked with refund.id,
chargeId (charge?.id ?? refundChargeId ?? null), paymentIntentId, nextMeta, and
then ack() is returned.
In `@frontend/lib/tests/shop/checkout-currency-policy.test.ts`:
- Around line 32-47: The mock for payment-attempts loads the actual module via
vi.importActual before your test hooks run, causing stale env reads; call
resetEnvCache() immediately before invoking vi.importActual (and again after any
env flag changes) so the imported module reads the updated payment flags; apply
this change around the vi.mock block that wraps
ensureStripePaymentIntentForOrder (and replicate the same resetEnvCache()
placement for the other similar vi.mock blocks that affect Stripe/Monobank
cases).
---
Outside diff comments:
In `@frontend/app/api/shop/webhooks/stripe/route.ts`:
- Around line 867-887: When paymentProvider is omitted the code currently
recomputes the provider/method on each request (which can flip when PSP
availability changes), so compute and freeze the resolved provider/method once
at the start (e.g., set a resolvedPaymentProvider and resolvedPaymentMethod
variable when first deriving them from the incoming payload and availability
lookup) and always pass those frozen values into createOrderWithItems() and any
later logic that persists the order; update the branches that handle
paymentIntentId/refundObject to use the same frozen resolved values (reference
paymentIntentId, refundObject and createOrderWithItems) so retries use the
originally resolved rail/method rather than re-resolving.
In `@frontend/lib/services/shop/shipping/admin-actions.ts`:
- Around line 584-621: The new shipment-row validation runs before the existing
early-return fast paths causing duplicate admin requests to fail; restore the
no-op fast path by moving the checks that validate the shipment row so they
occur after the existing shipping_status === 'shipped' / 'delivered'
early-return blocks. Concretely, in the handler for args.action ===
'mark_shipped' (and the analogous block around lines 684-720), ensure the
assertShipmentSupportsManualFulfillment and any shipment-row consistency checks
are executed only after you perform the existing shipping_status === 'shipped'
audit append (appendAuditEntry / buildShippingAdminAuditDedupe) and return the
no-change response, so repeated identical admin requests still hit the preserved
fast path.
In `@frontend/lib/services/shop/shipping/shipments-worker.ts`:
- Around line 693-749: Re-check the order refund/containment state immediately
before proceeding to create labels: after running
evaluateOrderShippingEligibility and before calling
createInternetDocument/markSucceeded, fetch the latest order state (or re-run
evaluateOrderShippingEligibility against the latest order snapshot) and confirm
there is no refund/requested refund; if a refund is now present, throw the
appropriate buildFailure instead of creating a label. Update the flow around
evaluateOrderShippingEligibility, createInternetDocument, and markSucceeded to
perform this final containment check using the same identifying fields on
details (e.g., refund status fields) and abort before calling
markSucceeded/createInternetDocument when a refund has appeared.
- Around line 445-450: The current eligibility filter call to
orderShippingEligibilityWhereSql (used in the query that precedes
processClaimedShipment) excludes all "processing" rows, which prevents
reclaiming stale processing shipments with expired leases; update the logic so
that orderShippingEligibilityWhereSql keeps its filtering for queued/failed but
does not exclude processing rows whose lease has expired (e.g., add a
condition/flag to orderShippingEligibilityWhereSql to allow processing rows when
lease_expires_at <= now or when a passed-in allowExpiredProcessing=true), then
use that updated call in the query so processClaimedShipment can still be
reached to downgrade expired processing rows.
In `@frontend/lib/tests/shop/checkout-concurrency-stock1.test.ts`:
- Around line 119-296: Wrap the entire test body (from after inserting the
product/productPrices through the assertions and before the current cleanup) in
a try block and move the current cleanup logic (the code that queries
orderItems, deletes orderItems, inventoryMoves, productPrices, orders, and
products) into a finally block so it always runs; keep the special-case rethrow
behavior for CI by rethrowing the caught error after cleanup (or capture the
original error and rethrow it in the finally), and ensure references to
productId, db.delete(...), inventoryMoves, productPrices, orderItems and orders
remain unchanged so teardown reliably cleans seeded data even if assertions
fail.
In `@frontend/lib/tests/shop/order-items-snapshot-immutable.test.ts`:
- Around line 133-202: The test currently only calls cleanupByIds() after the
happy-path assertions, so failures before orderId extraction leak seeded rows;
refactor the test to wrap the whole operation in a try/finally (outer finally)
and call cleanupByIds(productId, priceId, orderId) from that finally block so
cleanup always runs even if checkoutPOST(req) throws, res.json() is invalid, or
orderId is missing; keep the existing local variables (productId, priceId, idem,
orderId) and ensure orderId is passed (or undefined) into cleanupByIds so its
tolerant behavior remains intact.
---
Nitpick comments:
In `@frontend/app/api/shop/admin/returns/`[id]/refund/route.ts:
- Around line 74-79: The post-refund branch is currently reachable if
refundReturnRequest() ever resolves, causing a silent INTERNAL_ERROR response;
after calling refundReturnRequest({ returnRequestId: ..., actorUserId: ...,
requestId }), make the fallback explicitly unreachable by throwing an error
(e.g., throw new Error('refundReturnRequest resolved unexpectedly')) instead of
returning noStoreJson, or replace the noStoreJson fallback with an explicit
thrown Response so the route cannot silently succeed; update the code
referencing refundReturnRequest and the current noStoreJson fallback
accordingly.
In `@frontend/lib/services/orders/refund.ts`:
- Around line 27-32: The helper function readRows duplicates logic found in
readDbRows (in monobank-webhook.ts); extract this logic into a shared utility
(e.g., create a function named readRows in a new module like "@/lib/db/utils")
and replace the local readRows and the existing readDbRows with imports from
that module; update the files that used the previous implementations
(frontend/lib/services/orders/refund.ts and monobank-webhook.ts) to import the
shared readRows, ensuring the exported function signature remains generic
(readRows<T>(res: unknown): T[]) and behavior unchanged.
In `@frontend/lib/tests/shop/admin-shipping-payment-gate.test.ts`:
- Around line 122-130: The new pspStatusReason field is only persisted in the
post-insert transition branch; ensure the direct-seed path also writes it so
fixtures match expected state by adding pspStatusReason (args.pspStatusReason ??
null) to the .set(...) payload used when !requiresPostInsertBlockedTransition
(the same update flow that calls db.update(orders).set(...)). Locate the branch
guarded by requiresPostInsertBlockedTransition and mirror the pspStatusReason
handling into the other update/insert path so both code paths persist the field
consistently.
In `@frontend/lib/tests/shop/stripe-refund-convergence.test.ts`:
- Around line 231-247: The test currently allows the second call to
reconcileStripeRefundOrder({ orderId: seeded.orderId }) to return either
'finalized_success' or 'noop', which weakens the regression; change the
assertion to require 'noop' only (i.e., expect the variable named second to
strictly equal 'noop') and ensure any terminal side-effects are explicitly
asserted (you already check stockRestored/restockedAt and
countMoveKey(releaseKey) — keep those assertions to confirm no duplicate
terminal writes). Update the assertion around reconcileStripeRefundOrder in
stripe-refund-convergence.test.ts accordingly so the second pass must be 'noop'.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: da9743a2-a409-498d-9586-cfc5a9bc9a47
📒 Files selected for processing (34)
frontend/app/api/shop/admin/orders/[id]/refund/route.tsfrontend/app/api/shop/admin/orders/reconcile-stale/route.tsfrontend/app/api/shop/admin/returns/[id]/refund/route.tsfrontend/app/api/shop/checkout/route.tsfrontend/app/api/shop/internal/orders/restock-stale/route.tsfrontend/app/api/shop/webhooks/stripe/route.tsfrontend/lib/psp/stripe.tsfrontend/lib/services/orders.tsfrontend/lib/services/orders/checkout.tsfrontend/lib/services/orders/monobank-webhook.tsfrontend/lib/services/orders/refund.tsfrontend/lib/services/orders/stripe-refund-reconciliation.tsfrontend/lib/services/shop/returns.tsfrontend/lib/services/shop/shipping/admin-actions.tsfrontend/lib/services/shop/shipping/eligibility.tsfrontend/lib/services/shop/shipping/shipments-worker.tsfrontend/lib/shop/payments.tsfrontend/lib/tests/shop/admin-shipping-canonical-audit.test.tsfrontend/lib/tests/shop/admin-shipping-payment-gate.test.tsfrontend/lib/tests/shop/admin-shipping-state-sync.test.tsfrontend/lib/tests/shop/admin-stripe-refund-containment.test.tsfrontend/lib/tests/shop/checkout-concurrency-stock1.test.tsfrontend/lib/tests/shop/checkout-currency-policy.test.tsfrontend/lib/tests/shop/checkout-monobank-idempotency-contract.test.tsfrontend/lib/tests/shop/checkout-monobank-parse-validation.test.tsfrontend/lib/tests/shop/checkout-no-payments.test.tsfrontend/lib/tests/shop/checkout-route-stripe-disabled-recovery.test.tsfrontend/lib/tests/shop/monobank-refund-route-f4.test.tsfrontend/lib/tests/shop/order-items-snapshot-immutable.test.tsfrontend/lib/tests/shop/returns-phase4.test.tsfrontend/lib/tests/shop/returns-refund-disabled-for-launch.test.tsfrontend/lib/tests/shop/shipping-eligibility-refund-contained.test.tsfrontend/lib/tests/shop/stripe-refund-convergence.test.tsfrontend/lib/tests/shop/stripe-webhook-refund-full.test.ts
…pping flows, and test cleanup
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
frontend/app/api/shop/webhooks/stripe/route.ts (1)
1052-1075:⚠️ Potential issue | 🟠 MajorAdd an explicit Stripe provider guard before any order mutation.
paymentProvideris selected (Line 1054) but not enforced. Later branches still update byorders.idonly (e.g., Line 1159, Line 1904, Line 1954), so a mis-associated event could mutate non-Stripe orders.🔧 Proposed guard
if (!order) { logWebhookEvent({ eventType, paymentIntentId, paymentStatus, chargeId: bestEffortChargeId, refundId: bestEffortRefundId, requestId, stripeEventId, }); return ack(); } + + if (order.paymentProvider !== 'stripe') { + logWarn('stripe_webhook_provider_mismatch', { + ...eventMeta(), + code: 'PAYMENT_PROVIDER_MISMATCH', + orderId: order.id, + paymentProvider: order.paymentProvider, + paymentIntentId, + }); + return ack(); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/api/shop/webhooks/stripe/route.ts` around lines 1052 - 1075, The selected order row includes paymentProvider but there is no enforcement that it is Stripe before performing updates; add an explicit guard that verifies orders.paymentProvider === 'stripe' for the resolvedOrderId before any mutation (or include it in all update where clauses) so that functions using resolvedOrderId (and update branches that reference orders.id) only mutate Stripe-backed orders; update the selection/check logic around resolvedOrderId and the update statements that touch orders (the branches referenced by orders.id at the later update sites) to either short-circuit when paymentProvider !== 'stripe' or add a where(eq(orders.paymentProvider, 'stripe')) predicate to each update.frontend/lib/services/shop/shipping/shipments-worker.ts (1)
418-454:⚠️ Potential issue | 🟠 MajorMove the eligibility filter ahead of
LIMIT.
candidatesnow picks the firstlimitqueued/failed rows beforeorderShippingEligibilityWhereSql(...)runs. Refund-contained or otherwise blocked shipments stay inqueued/failed, never reachreleased_blocked, and get selected again on every run. Once enough of them accumulate, eligible shipments behind them stop getting claimed.💡 One way to avoid starving eligible shipments
with candidates as ( select s.id, s.order_id, s.status as candidate_status from shipping_shipments s + join orders o on o.id = s.order_id where ( - ( - s.status in ('queued', 'failed') - and (s.next_attempt_at is null or s.next_attempt_at <= now()) - ) - or s.status = 'processing' + s.status = 'processing' + or ( + s.status in ('queued', 'failed') + and (s.next_attempt_at is null or s.next_attempt_at <= now()) + and ${orderShippingEligibilityWhereSql({ + paymentStatusColumn: sql`o.payment_status`, + orderStatusColumn: sql`o.status`, + inventoryStatusColumn: sql`o.inventory_status`, + pspStatusReasonColumn: sql`o.psp_status_reason`, + })} + and ${shippingStatusTransitionWhereSql({ + column: sql`o.shipping_status`, + to: 'creating_label', + allowNullFrom: true, + includeSame: true, + })} + ) )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/services/shop/shipping/shipments-worker.ts` around lines 418 - 454, The candidates CTE is selecting the first args.limit shipments before the orderShippingEligibilityWhereSql filter runs, causing blocked/refund shipments to persist and starve eligible ones; move the eligibility check into the candidates CTE (join orders and apply orderShippingEligibilityWhereSql there) so the WHERE that uses sql`o.payment_status`/`o.status`/`o.inventory_status`/`o.psp_status_reason` runs before the ORDER BY ... LIMIT ${args.limit} and FOR UPDATE SKIP LOCKED, then keep the claimed CTE as the UPDATE that sets status='processing', lease_owner=${args.runId}, lease_expires_at using ${args.leaseSeconds}, and updated_at=now().
♻️ Duplicate comments (2)
frontend/lib/tests/shop/order-items-snapshot-immutable.test.ts (1)
35-42:⚠️ Potential issue | 🟠 MajorReset the env cache before both hoisted
vi.importActual()calls.These factories can still load the real Stripe modules before
beforeAllmutates the payment env, so this test stays import-order dependent. AddresetEnvCache()at the top of each factory, like the currency-policy test already does.Suggested fix
vi.mock('@/lib/env/stripe', async () => { + resetEnvCache(); const actual = await vi.importActual<Record<string, unknown>>('@/lib/env/stripe'); return { ...actual, isPaymentsEnabled: () => true, }; }); vi.mock('@/lib/services/orders/payment-attempts', async () => { + resetEnvCache(); const actual = await vi.importActual<any>( '@/lib/services/orders/payment-attempts' );Also applies to: 44-58
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/tests/shop/order-items-snapshot-immutable.test.ts` around lines 35 - 42, The vi.mock factories that call vi.importActual('@/lib/env/stripe') must call resetEnvCache() first to avoid import-order dependence; update both factories (the one overriding isPaymentsEnabled in this file and the other similar factory around lines 44-58) to invoke resetEnvCache() at the top of each factory before calling vi.importActual so the real Stripe/env module is reloaded after tests mutate the payment env.frontend/lib/tests/shop/checkout-concurrency-stock1.test.ts (1)
26-41:⚠️ Potential issue | 🟠 MajorReset the env cache before importing the real
payment-attemptsmodule.This mock still calls
vi.importActual()before the file's Stripe env is established inbeforeAll, so provider availability can depend on import order. AddresetEnvCache()at the top of the factory.Suggested fix
vi.mock('@/lib/services/orders/payment-attempts', async () => { + resetEnvCache(); const actual = await vi.importActual<any>( '@/lib/services/orders/payment-attempts' );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/tests/shop/checkout-concurrency-stock1.test.ts` around lines 26 - 41, The mock factory for payment-attempts calls vi.importActual(...) before the test env is prepared; to fix it call resetEnvCache() as the first statement inside the vi.mock factory so environment variables are cleared before importing the real module, then proceed to await vi.importActual(...) and override ensureStripePaymentIntentForOrder (the mocked function name) as currently implemented; this ensures the real module sees the correct Stripe env set in beforeAll.
🧹 Nitpick comments (4)
frontend/lib/tests/shop/stripe-refund-convergence.test.ts (2)
159-166: Consider cleaning upinventory_movesrecords in teardown.The
cleanupfunction deletes products, orders, and shipments, butcountMoveKeyqueriesinventory_moveswhich isn't cleaned. While UUIDs make collisions unlikely, orphaned test records could accumulate in shared test databases.🧹 Proposed cleanup addition
async function cleanup(seed: SeededOrder | null) { if (!seed) return; + await db.execute( + sql`DELETE FROM inventory_moves WHERE move_key LIKE ${'%' + seed.orderId + '%'}` + ); await db .delete(shippingShipments) .where(eq(shippingShipments.orderId, seed.orderId)); await db.delete(orders).where(eq(orders.id, seed.orderId)); await db.delete(products).where(eq(products.id, seed.productId)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/tests/shop/stripe-refund-convergence.test.ts` around lines 159 - 166, The cleanup function for tests (async function cleanup) currently deletes from shippingShipments, orders, and products but doesn't remove related inventory_moves rows queried by countMoveKey; update cleanup to also delete from inventory_moves where orderId = seed.orderId OR productId = seed.productId (use the same db.delete(...).where(eq(inventory_moves.orderId, seed.orderId)) and db.delete(...).where(eq(inventory_moves.productId, seed.productId)) pattern or a combined OR condition) so that inventory_moves entries created during tests are removed along with shipments/orders/products.
67-78: Type assertions (as any) bypass schema validation.Using
as anyon insert values (lines 78, 99, 110) silences type errors but could mask issues if the schema changes. Consider using partial type helpers or satisfying the full schema type for test data.Also applies to: 80-99, 101-110
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/tests/shop/stripe-refund-convergence.test.ts` around lines 67 - 78, The test is using a blanket type assertion "as any" when calling db.insert(products).values(...) which bypasses schema/type checks; replace those assertions by constructing test records that satisfy the products table type (or use a Partial/DeepPartial helper or the specific Insert/Insertable type for the products model) so the compiler enforces schema fields and types. Locate the insert calls (db.insert(products).values) and change them to use the appropriate product insert type or a helper that marks optional fields, ensuring required fields like id, title, slug, sku, price, currency, stock, isActive, badge, and imageUrl match types without "as any".frontend/lib/tests/shop/admin-shipping-payment-gate.test.ts (1)
75-75: Consider de-duplicating the refund marker literal.
'REFUND_REQUESTED'appears in multiple places. Using a shared constant/enum would reduce drift risk between test fixtures and production guard logic.Also applies to: 200-200
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/tests/shop/admin-shipping-payment-gate.test.ts` at line 75, The test uses the string literal 'REFUND_REQUESTED' directly (seen in the expression (args.pspStatusReason ?? null) !== 'REFUND_REQUESTED'); extract this status into a shared constant or enum (e.g., RefundStatus.REFUND_REQUESTED) used by both the test fixture and the production guard logic so the comparison uses the shared symbol instead of a hardcoded string; update references in the test (args.pspStatusReason) and any other occurrences (mentioned around lines 200) to import and use that constant to avoid drift.frontend/app/api/shop/admin/returns/[id]/refund/route.ts (1)
74-79: Solid defensive pattern for aPromise<never>function.Since
refundReturnRequestis typed asPromise<never>and always throwsInvalidPayloadErrorwith codeRETURN_REFUND_DISABLED, line 79 acts as a safeguard. If the implementation ever changes to resolve instead of throw, this will fail explicitly rather than silently returning no response.Consider adding a brief comment to explain the intent for future maintainers:
📝 Optional: Add clarifying comment
await refundReturnRequest({ returnRequestId: returnRequestIdForLog, actorUserId: typeof admin.id === 'string' ? admin.id : null, requestId, }); + // refundReturnRequest returns Promise<never> (always throws); guard against future changes throw new Error('refundReturnRequest unexpectedly resolved');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/api/shop/admin/returns/`[id]/refund/route.ts around lines 74 - 79, Add an explanatory comment above the call to refundReturnRequest to document that refundReturnRequest is intentionally typed as Promise<never> and always throws (InvalidPayloadError with code RETURN_REFUND_DISABLED), and that the subsequent throw new Error('refundReturnRequest unexpectedly resolved') is a defensive safeguard to fail loudly if refundReturnRequest ever changes to resolve; reference the refundReturnRequest call and the explicit throw to help future maintainers understand the intent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/lib/services/shop/shipping/shipments-worker.ts`:
- Around line 745-766: There is a TOCTOU gap allowing label creation after a
refund flip; to fix it, reload and re-validate the persisted order state
immediately before making the non-idempotent external call and before calling
markSucceeded: call loadOrderShippingDetails again (or a minimal status-only
read) after computing latestPayload, re-run assertOrderStillShippable or
explicitly check latestDetails.psp_status_reason !== 'REFUND_REQUESTED' and that
latestSnapshot.methodCode === latestDetails.shipping_method_code, and only then
call createInternetDocument and markSucceeded; also add an eligibility predicate
to markSucceeded (or guard its caller) to refuse marking label_created if the
final-state check fails so label_created cannot be recorded when a refund was
requested.
In `@frontend/lib/tests/shop/checkout-concurrency-stock1.test.ts`:
- Around line 295-296: The cleanup catch inside the finally block currently does
"if (process.env.CI) throw err;" which violates noUnsafeFinally; modify the
cleanup to capture the error instead of throwing: in the finally scope replace
the throw with storing the error into a local variable (e.g. let cleanupErr =
null; cleanupErr = err; and optionally log it), and after the entire try/finally
completes (just after the finally), check if cleanupErr is set and then rethrow
it (or throw a wrapped error). Update the block that references process.env.CI
so it only records the error (or logs) and defers throwing until after the
finally completes.
In `@frontend/lib/tests/shop/order-items-snapshot-immutable.test.ts`:
- Around line 295-296: The finally block currently rethrows cleanupError which
violates noUnsafeFinally; capture cleanupError inside finally (using
cleanupByIds and the existing cleanupError variable), remove the throw from the
finally block, and after the try/finally completes check if (!primaryError &&
cleanupError) then throw cleanupError; reference the variables primaryError and
cleanupError and the cleanupByIds call in the test to locate and move the
conditional throw out of the finally.
---
Outside diff comments:
In `@frontend/app/api/shop/webhooks/stripe/route.ts`:
- Around line 1052-1075: The selected order row includes paymentProvider but
there is no enforcement that it is Stripe before performing updates; add an
explicit guard that verifies orders.paymentProvider === 'stripe' for the
resolvedOrderId before any mutation (or include it in all update where clauses)
so that functions using resolvedOrderId (and update branches that reference
orders.id) only mutate Stripe-backed orders; update the selection/check logic
around resolvedOrderId and the update statements that touch orders (the branches
referenced by orders.id at the later update sites) to either short-circuit when
paymentProvider !== 'stripe' or add a where(eq(orders.paymentProvider,
'stripe')) predicate to each update.
In `@frontend/lib/services/shop/shipping/shipments-worker.ts`:
- Around line 418-454: The candidates CTE is selecting the first args.limit
shipments before the orderShippingEligibilityWhereSql filter runs, causing
blocked/refund shipments to persist and starve eligible ones; move the
eligibility check into the candidates CTE (join orders and apply
orderShippingEligibilityWhereSql there) so the WHERE that uses
sql`o.payment_status`/`o.status`/`o.inventory_status`/`o.psp_status_reason` runs
before the ORDER BY ... LIMIT ${args.limit} and FOR UPDATE SKIP LOCKED, then
keep the claimed CTE as the UPDATE that sets status='processing',
lease_owner=${args.runId}, lease_expires_at using ${args.leaseSeconds}, and
updated_at=now().
---
Duplicate comments:
In `@frontend/lib/tests/shop/checkout-concurrency-stock1.test.ts`:
- Around line 26-41: The mock factory for payment-attempts calls
vi.importActual(...) before the test env is prepared; to fix it call
resetEnvCache() as the first statement inside the vi.mock factory so environment
variables are cleared before importing the real module, then proceed to await
vi.importActual(...) and override ensureStripePaymentIntentForOrder (the mocked
function name) as currently implemented; this ensures the real module sees the
correct Stripe env set in beforeAll.
In `@frontend/lib/tests/shop/order-items-snapshot-immutable.test.ts`:
- Around line 35-42: The vi.mock factories that call
vi.importActual('@/lib/env/stripe') must call resetEnvCache() first to avoid
import-order dependence; update both factories (the one overriding
isPaymentsEnabled in this file and the other similar factory around lines 44-58)
to invoke resetEnvCache() at the top of each factory before calling
vi.importActual so the real Stripe/env module is reloaded after tests mutate the
payment env.
---
Nitpick comments:
In `@frontend/app/api/shop/admin/returns/`[id]/refund/route.ts:
- Around line 74-79: Add an explanatory comment above the call to
refundReturnRequest to document that refundReturnRequest is intentionally typed
as Promise<never> and always throws (InvalidPayloadError with code
RETURN_REFUND_DISABLED), and that the subsequent throw new
Error('refundReturnRequest unexpectedly resolved') is a defensive safeguard to
fail loudly if refundReturnRequest ever changes to resolve; reference the
refundReturnRequest call and the explicit throw to help future maintainers
understand the intent.
In `@frontend/lib/tests/shop/admin-shipping-payment-gate.test.ts`:
- Line 75: The test uses the string literal 'REFUND_REQUESTED' directly (seen in
the expression (args.pspStatusReason ?? null) !== 'REFUND_REQUESTED'); extract
this status into a shared constant or enum (e.g., RefundStatus.REFUND_REQUESTED)
used by both the test fixture and the production guard logic so the comparison
uses the shared symbol instead of a hardcoded string; update references in the
test (args.pspStatusReason) and any other occurrences (mentioned around lines
200) to import and use that constant to avoid drift.
In `@frontend/lib/tests/shop/stripe-refund-convergence.test.ts`:
- Around line 159-166: The cleanup function for tests (async function cleanup)
currently deletes from shippingShipments, orders, and products but doesn't
remove related inventory_moves rows queried by countMoveKey; update cleanup to
also delete from inventory_moves where orderId = seed.orderId OR productId =
seed.productId (use the same db.delete(...).where(eq(inventory_moves.orderId,
seed.orderId)) and db.delete(...).where(eq(inventory_moves.productId,
seed.productId)) pattern or a combined OR condition) so that inventory_moves
entries created during tests are removed along with shipments/orders/products.
- Around line 67-78: The test is using a blanket type assertion "as any" when
calling db.insert(products).values(...) which bypasses schema/type checks;
replace those assertions by constructing test records that satisfy the products
table type (or use a Partial/DeepPartial helper or the specific
Insert/Insertable type for the products model) so the compiler enforces schema
fields and types. Locate the insert calls (db.insert(products).values) and
change them to use the appropriate product insert type or a helper that marks
optional fields, ensuring required fields like id, title, slug, sku, price,
currency, stock, isActive, badge, and imageUrl match types without "as any".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d37768cd-08ef-4919-8952-4b5a011db0c9
📒 Files selected for processing (12)
frontend/app/api/shop/admin/returns/[id]/refund/route.tsfrontend/app/api/shop/webhooks/stripe/route.tsfrontend/lib/services/orders/stripe-refund-reconciliation.tsfrontend/lib/services/shop/shipping/admin-actions.tsfrontend/lib/services/shop/shipping/shipments-worker.tsfrontend/lib/tests/shop/admin-shipping-payment-gate.test.tsfrontend/lib/tests/shop/checkout-concurrency-stock1.test.tsfrontend/lib/tests/shop/checkout-currency-policy.test.tsfrontend/lib/tests/shop/order-items-snapshot-immutable.test.tsfrontend/lib/tests/shop/stripe-refund-convergence.test.tsfrontend/lib/tests/shop/stripe-webhook-psp-fields.test.tsfrontend/lib/tests/shop/stripe-webhook-refund-full.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/lib/services/orders/stripe-refund-reconciliation.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
frontend/lib/tests/shop/checkout-concurrency-stock1.test.ts (1)
274-276: Remove redundant catch/rethrow block.Line 274–Line 276 (
catch (err) { throw err; }) adds no behavior and can be dropped to simplify control flow.♻️ Proposed simplification
- } catch (err) { - throw err; - } finally { + } finally {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/tests/shop/checkout-concurrency-stock1.test.ts` around lines 274 - 276, The catch block that simply rethrows the caught error around the try/finally in the test is redundant—remove the entire "catch (err) { throw err; }" block so the try block flows directly into the existing finally. Update the test (the try/finally surrounding the checkout concurrency logic in checkout-concurrency-stock1.test.ts) by deleting that catch clause and ensuring indentation/formatting is adjusted accordingly; no other behavioral changes are needed.frontend/app/api/shop/webhooks/stripe/route.ts (1)
1717-1726: Consider type-safe casting forrestoreStripeRefundFailurecall.The
order as anycast at line 1718 bypasses TypeScript's type checking. While the order object structurally matchesStripeRefundOrderRow(all required fields are fetched at lines 1051-1075), usingas anyloses compile-time safety.If the
orderselect type doesn't matchStripeRefundOrderRowdue to Drizzle's inference, consider defining a shared type or using a more specific assertion to maintain type safety during future refactors.This is a minor ergonomic suggestion; the current code is functionally correct.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/api/shop/webhooks/stripe/route.ts` around lines 1717 - 1726, The call to restoreStripeRefundFailure uses an unsafe cast "order as any" which bypasses TypeScript checking; replace this with a properly-typed assertion or shared type so the compiler verifies the shape (e.g., ensure the variable order is asserted/typed as StripeRefundOrderRow or a shared interface used by restoreStripeRefundFailure) — update the declaration or the select mapping that produces order (the variable named order returned earlier) or change the function signature of restoreStripeRefundFailure to accept the precise StripeRefundOrderRow type so you can pass order without casting.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/app/api/shop/webhooks/stripe/route.ts`:
- Around line 1906-1957: The current charge.refund.updated handler (the block
using buildPspMetadata, mergePspMetadata and updating orders) treats any
refund.status !== 'succeeded' as "waiting_terminal_refund_status"; change the
condition so only non-terminal statuses (e.g., 'pending', 'requires_action')
produce the waiting decision and terminal statuses ('failed', 'canceled') are
handled separately: detect refund.status in ['failed','canceled'] and set a
terminal failure decision (or log a warning and set an appropriate
pspStatusReason/pspMetadata entry such as 'terminal_refund_failure'), and only
use decision 'waiting_terminal_refund_status' for transient statuses; ensure
this logic also accounts for order.pspStatusReason !== 'REFUND_REQUESTED' so
terminal failures do not silently become "waiting".
---
Nitpick comments:
In `@frontend/app/api/shop/webhooks/stripe/route.ts`:
- Around line 1717-1726: The call to restoreStripeRefundFailure uses an unsafe
cast "order as any" which bypasses TypeScript checking; replace this with a
properly-typed assertion or shared type so the compiler verifies the shape
(e.g., ensure the variable order is asserted/typed as StripeRefundOrderRow or a
shared interface used by restoreStripeRefundFailure) — update the declaration or
the select mapping that produces order (the variable named order returned
earlier) or change the function signature of restoreStripeRefundFailure to
accept the precise StripeRefundOrderRow type so you can pass order without
casting.
In `@frontend/lib/tests/shop/checkout-concurrency-stock1.test.ts`:
- Around line 274-276: The catch block that simply rethrows the caught error
around the try/finally in the test is redundant—remove the entire "catch (err) {
throw err; }" block so the try block flows directly into the existing finally.
Update the test (the try/finally surrounding the checkout concurrency logic in
checkout-concurrency-stock1.test.ts) by deleting that catch clause and ensuring
indentation/formatting is adjusted accordingly; no other behavioral changes are
needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9ba4b3d2-6adc-4cdf-9a35-619285f0bbf8
📒 Files selected for processing (4)
frontend/app/api/shop/webhooks/stripe/route.tsfrontend/lib/services/shop/shipping/shipments-worker.tsfrontend/lib/tests/shop/checkout-concurrency-stock1.test.tsfrontend/lib/tests/shop/order-items-snapshot-immutable.test.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 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 `@frontend/lib/tests/shop/checkout-concurrency-stock1.test.ts`:
- Around line 220-229: The test currently only asserts stock-related failure
when failCode is truthy; make the stock-intent assertion unconditional by
deriving a fallback failure indicator from the response body when failCode is
empty. In the test around failCode (in checkout-concurrency-stock1.test.ts),
compute a single string to check (e.g., const failureIndicator = failCode ||
extract code/error from the response body) and then assert that failureIndicator
includes one of ['OUT_OF_STOCK','INSUFFICIENT_STOCK','STOCK','NOT_ENOUGH_STOCK']
using the existing some(...).toBe(true) check so the stock hint is always
enforced even when failCode is empty.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c7d6e580-f2f2-4904-b56e-60f12b7d30a6
📒 Files selected for processing (2)
frontend/app/api/shop/webhooks/stripe/route.tsfrontend/lib/tests/shop/checkout-concurrency-stock1.test.ts
ViktorSvertoka
approved these changes
Mar 17, 2026
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
2 participants
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.
Description
This PR implements the launch-readiness remediation work for the Shop module based on the production audit findings.
The changes focus on closing launch-critical gaps in checkout, refund handling, and shipping operations:
paymentProvider='none'pathThe goal of this PR is to make the Shop safe for production launch with real orders and payments, without expanding scope into post-launch refund features.
Related Issue
Issue: #<issue_number>
Changes
503 PSP_UNAVAILABLEbehavior when no valid payment provider is availablepaymentProvider='none'orders.psp_status_reason = 'REFUND_REQUESTED'as the single refund-requested markermark_shipped/mark_deliveredDatabase Changes (if applicable)
How Has This Been Tested?
Additional local validation:
Screenshots (if applicable)
N/A
Checklist
Before submitting
Reviewers
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores
Tests