(SP:3) [SHOP] harden checkout, wallet flows, shipping coupling, and local smoke coverage#399
Merged
Conversation
…omain association placeholder
… lock lite status contract
…s pending unknown
…e; harden sync payload encoding
…ility, and stabilize tests
… and Nova Poshta flows
…ne payment option card sizing, fix import linter
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
|
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:
📝 WalkthroughWalkthroughAdded status-token support and access-controlled checkout summaries across API, payment clients, and pages; refactored cart UI into step/card components with new UI tokens; introduced shipping eligibility checks and pipeline shutdown; replaced Monobank webhook verifier with a detailed result type; expanded tests and capability/env gating. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant CheckoutAPI as Checkout API
participant OrderAccess as Order Access
participant PSP as Payment Provider
participant Browser as Browser/Redirect
participant SuccessPage as Success Page
Client->>CheckoutAPI: POST /api/shop/checkout (items, paymentProvider, maybe statusToken)
CheckoutAPI->>OrderAccess: authorizeOrderMutationAccess(orderId?, statusToken?, requiredScope)
OrderAccess-->>CheckoutAPI: {ok}|{code: STATUS_TOKEN_*}
alt authorized
CheckoutAPI->>PSP: create payment (intent/invoice)
PSP-->>CheckoutAPI: intentId / pageUrl
CheckoutAPI->>CheckoutAPI: createCheckoutStatusToken(scopes)
CheckoutAPI-->>Client: 200 with statusToken and payment data
else status token required or unauthorized
CheckoutAPI-->>Client: 4xx with code
end
Browser->>PSP: complete payment (success_url includes statusToken)
PSP-->>Browser: redirect to success_url
Browser->>SuccessPage: GET /success?orderId..&statusToken=...
SuccessPage->>OrderAccess: getCheckoutSuccessOrderSummary(orderId, statusToken)
OrderAccess-->>SuccessPage: {ok:true, order}
SuccessPage-->>Client: render success
sequenceDiagram
participant Worker as Shipments Worker
participant DB as Database
participant Elig as Eligibility Service
participant Pipeline as Shipping Pipeline
Worker->>DB: claim queued shipments (SQL uses order/payment/inventory predicate)
DB-->>Worker: shipments
Worker->>Elig: evaluateOrderShippingEligibility(payment_status, order_status, inventory_status)
Elig-->>Worker: {ok:true} or {ok:false, reason}
alt eligible
Worker->>DB: process shipment (create label)
else ineligible
Worker->>Pipeline: closeShippingPipelineForOrder(orderId, reason)
Pipeline->>DB: mark shipments needs_attention, clear leases, maybe cancel order shipping
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
|
![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: 10
🧹 Nitpick comments (8)
frontend/tests/e2e/shop-checkout-local.spec.ts (2)
8-10: Consider using environment variables for local DB credentials.Hardcoding credentials (even for local development) in source code is a maintenance smell. While the strict environment guards mitigate risk, extracting the DSN entirely to an environment variable would be cleaner and avoid accidental credential exposure in git history.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/tests/e2e/shop-checkout-local.spec.ts` around lines 8 - 10, Replace the hardcoded DSN in REQUIRED_LOCAL_DB_URL with a value read from an environment variable (e.g., process.env.REQUIRED_LOCAL_DB_URL or process.env.LOCAL_DB_URL) and keep ALLOWED_LOCAL_DB_HOSTS as-is; validate the env var at startup and throw a clear error if missing so tests fail fast, and optionally provide the existing literal as a non-committed default only when a specific DEV_FALLBACK flag is set to avoid embedding credentials in source history.
439-446: Note:.reverse()mutates the original array.This is acceptable here since cleanup buckets are local to each test and only used once in the
finallyblock. Just noting for awareness in case bucket reuse is considered in the future.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/tests/e2e/shop-checkout-local.spec.ts` around lines 439 - 446, The code in cleanupSeededData calls bucket.orderIds.reverse(), which mutates the original array; to avoid unintended side effects if the CleanupBucket is reused later, operate on a copy instead—e.g., iterate over [...bucket.orderIds].reverse() or bucket.orderIds.slice().reverse() inside cleanupSeededData so the original bucket.orderIds remains unchanged; reference cleanupSeededData and bucket.orderIds (type CleanupBucket) when applying this change.frontend/lib/tests/shop/stripe-payment-client-return-route.test.ts (1)
25-34: Consider more precise assertions for the success route.The test uses
toContain()which verifies substring presence but doesn't validate the full URL structure. Consider using a more precise assertion to ensure the URL format is exactly as expected.💡 Example of stricter assertion
it('keeps succeeded intents routed to checkout success', () => { const route = nextRouteForPaymentResult({ orderId: ORDER_ID, statusToken: STATUS_TOKEN, status: 'succeeded', }); - expect(route).toContain('/shop/checkout/success?orderId='); - expect(route).toContain(`statusToken=${STATUS_TOKEN}`); + expect(route).toBe( + `/shop/checkout/success?orderId=${encodeURIComponent(ORDER_ID)}&statusToken=${STATUS_TOKEN}` + ); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/tests/shop/stripe-payment-client-return-route.test.ts` around lines 25 - 34, The test for nextRouteForPaymentResult currently uses toContain() and should instead assert the exact URL structure: construct the expected URL using ORDER_ID and STATUS_TOKEN (or parse the returned route with URL and assert pathname equals '/shop/checkout/success' and search params orderId and statusToken match exactly) and replace the two toContain assertions with a single precise assertion (e.g., expect(route).toBe(expectedUrl) or assert URL pathname and query params) referencing nextRouteForPaymentResult, ORDER_ID and STATUS_TOKEN to locate the code to change.frontend/app/[locale]/shop/cart/capabilities.ts (1)
30-35: Make the Google Pay resolver self-contained.This helper returns
truefromSHOP_MONOBANK_GPAY_ENABLEDalone, even whenPAYMENTS_ENABLEDis off orisMonobankEnabled()would disable Monobank entirely. Since this is an exported capability helper, it should probably represent the effective availability on its own and gate throughresolveMonobankCheckoutEnabled()first.♻️ Minimal hardening
export function resolveMonobankGooglePayEnabled(): boolean { + if (!resolveMonobankCheckoutEnabled()) return false; + const raw = (process.env.SHOP_MONOBANK_GPAY_ENABLED ?? '') .trim() .toLowerCase(); return raw === 'true' || raw === '1' || raw === 'yes' || raw === 'on'; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/`[locale]/shop/cart/capabilities.ts around lines 30 - 35, The function resolveMonobankGooglePayEnabled should be self-contained and only return true when Monobank checkout is effectively allowed; update it to first call and respect resolveMonobankCheckoutEnabled() (or isMonobankEnabled() if that’s the canonical gate) and immediately return false if that gate is disabled, then fall back to parsing SHOP_MONOBANK_GPAY_ENABLED as it currently does; ensure you reference and call the existing resolveMonobankCheckoutEnabled (or isMonobankEnabled) helper within resolveMonobankGooglePayEnabled so the exported capability reflects effective availability.frontend/lib/tests/shop/checkout-no-payments.test.ts (1)
265-298: Also verify the 503 path leaves inventory untouched.This currently proves no order row is persisted, but it would still pass if stock was reserved/decremented before the PSP-unavailable response. Add a stock and/or
inventory_movesassertion here so the fail-closed path is covered end-to-end.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/tests/shop/checkout-no-payments.test.ts` around lines 265 - 298, The test currently verifies no order row is created but doesn't assert inventory was not reserved; update the 'Explicit Stripe request fails closed when Stripe is unavailable' test (the one that calls postCheckout with paymentProvider: 'stripe') to also fetch and assert the product's stock remains unchanged and that no inventory_moves were recorded for that product/idempotency key; after the existing check that no orders row exists, query products (e.g., select stock from products where id = productId) and assert stock equals the original stock returned by createIsolatedProductForCurrency, and query inventory_moves (or the inventory_moves table) for entries referencing productId (and/or the idemKey) and assert none exist before finally calling cleanupIsolatedProduct.frontend/lib/services/orders/restock.ts (1)
122-127: Shipping pipeline closed before idempotency check.
closeShippingPipelineForOrderis called before the early return at line 138 that checks if the order is already restocked (order.stockRestored || order.restockedAt !== null). This means repeated restock calls will attempt to close the shipping pipeline multiple times for the same order.While
closeShippingPipelineForOrderis likely idempotent, consider moving this call after the early return check to avoid unnecessary work on already-restocked orders.♻️ Move shipping closure after idempotency check
if (!order) throw new OrderNotFoundError('Order not found'); - if (reason) { - await closeShippingPipelineForOrder({ - orderId, - reason: `payment_terminal:${reason}`, - }); - } - const isNoPayment = order.paymentProvider === 'none'; const provider = resolvePaymentProvider(order); const transitionSource = options?.alreadyClaimed ? 'janitor' : 'system'; if ( order.inventoryStatus === 'released' || order.stockRestored || order.restockedAt !== null ) return; + if (reason) { + await closeShippingPipelineForOrder({ + orderId, + reason: `payment_terminal:${reason}`, + }); + } + const reservedMoves = await db🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/services/orders/restock.ts` around lines 122 - 127, The call to closeShippingPipelineForOrder is currently executed before the idempotency guard (the check on order.stockRestored || order.restockedAt !== null), causing repeated restock attempts to close the shipping pipeline unnecessarily; move the await closeShippingPipelineForOrder({...}) call to after the early return/idempotency check (i.e., only invoke closeShippingPipelineForOrder when the function proceeds past the restocked check) and ensure it still uses the same reason formatting (`reason: \`payment_terminal:${reason}\``) so repeated calls exit early without performing shipping closure.frontend/lib/services/orders/summary.ts (1)
196-203: Defensive fallback for edge case.The check
access.code === 'OK' ? 'FORBIDDEN' : access.codehandles an unlikely edge case where authorization fails but the code is still 'OK'. While defensive, consider whether this state is actually possible inauthorizeOrderMutationAccess. If not, this could be simplified.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/services/orders/summary.ts` around lines 196 - 203, The ternary fallback handling in the access check is defensive and should be simplified: inside the failure branch where you currently compute const code = access.code === 'OK' ? 'FORBIDDEN' : access.code, remove the special-case and return the actual access.code directly (or inline it in the returned object) because authorizeOrderMutationAccess should not return 'OK' when authorized is false; update the failure return in the function that contains this snippet (the access handling in frontend/lib/services/orders/summary.ts, the branch that processes authorizeOrderMutationAccess results) to use code: access.code (or simply omit the temporary variable).frontend/app/api/shop/checkout/route.ts (1)
956-970: Consider logging more context on token creation failure.The error handling for status token creation is good, but the catch block swallows the error after logging. If
statusTokenisnull, downstream flows may behave unexpectedly.💡 Suggestion: Add a warning log level or return early for critical flows
If a null
statusTokenwould cause issues in certain flows (e.g., Monobank invoice which requiresstatusTokenperbuildMonobankCheckoutResponse), consider whether to fail the checkout rather than proceed with a null token.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/api/shop/checkout/route.ts` around lines 956 - 970, The catch for createCheckoutStatusToken only logs the error and returns null, which can let downstream flows (e.g., buildMonobankCheckoutResponse) run with a missing statusToken; update the error handling in the statusToken IIFE to log richer context (include orderMeta, order.id, order.paymentProvider and the error stack) and then fail fast for critical flows — either throw a specific Error or return an HTTP error/early response when statusToken is required (instead of returning null); reference createCheckoutStatusToken, statusToken, logError, orderMeta and buildMonobankCheckoutResponse when making this change so callers that require the token do not proceed with a null value.
🤖 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/`[locale]/shop/cart/CartPageClient.tsx:
- Around line 1119-1121: The current fallback link logic only uses
createdOrderId (built via statusTokenQuery) which breaks resuming
tokenized/external provider flows; modify the flow that computes the
continuation target to persist a provider-specific recovery target (e.g.,
paymentRecoveryUrl or recoveryTarget) alongside createdOrderId when preparing
the server response or client state, then update places that build
statusTokenQuery and render the "Not redirected?" links (references:
statusTokenQuery, createdOrderId, and the link render blocks around lines noted)
to use that recoveryTarget when present instead of the generic /shop/orders/{id}
URL so the fallback resumes the correct provider flow. Ensure the recoveryTarget
is URL-encoded when appended and falls back to the order-details link only if
recoveryTarget is absent.
- Around line 1123-1129: The current redirect logic in CartPageClient.tsx uses
paymentProvider, clientSecret and router.push to clear the cart even when Stripe
initialization is incomplete; update the logic so that if paymentProvider ===
'stripe' but clientSecret is falsy you do NOT perform the generic success
redirect (the router.push that includes clearCart=1) — instead treat it as a
failure: surface an error (or redirect to a payment-error route) and do not
clear the cart or proceed to the generic success path. Apply the same guard to
the duplicate Stripe redirect block around the other occurrence (the block near
lines 1161-1165) so both paymentProvider/clientSecret checks strictly require a
usable clientSecret before calling router.push with clearCart=1.
- Around line 257-295: SelectableCard currently renders a button alongside a
hidden radio which creates two focusable controls and duplicate updates; change
SelectableCard to render a <label> wrapper instead of a <button>, move the radio
input inside that label (or associate via htmlFor/id) so the label+input act as
one control, remove the separate visible button element, keep the same className
logic (use SHOP_CART_SELECTABLE_CARD* constants) on the label, wire selection
state to the radio's checked and onChange (or forward onClick to change handler
on the input), and preserve the visual checkmark span and disabled styling;
update any handlers that relied on the button (e.g., onClick) to operate via the
radio input change within SelectableCard.
In `@frontend/app/`[locale]/shop/checkout/payment/StripePaymentClient.tsx:
- Around line 93-100: The error URL currently drops statusToken while the
success URL uses tokenSuffix; update the failure/error path construction to
include the same tokenSuffix so the statusToken is preserved on error/retry
flows. Locate where tokenSuffix is composed and where failure (and any other
error/decline redirect code such as in confirmPayment catch paths) and change
buildInAppPath(`/checkout/error?orderId=${id}`) to incorporate tokenSuffix
(e.g., include ${tokenSuffix}) so both success and failure links carry the
statusToken for token-only guest checkouts.
In `@frontend/lib/env/stripe.ts`:
- Around line 71-85: isPaymentsEnabled currently ignores the STRIPE rail flag
unless callers set respectStripePaymentsFlag: true; change isPaymentsEnabled to
consult isStripeRailEnabledByFlags() by default (return false when the rail is
disabled) and invert the option to an explicit opt-out (e.g.,
options.ignoreStripeRail or options.respectStripePaymentsFlag defaulting to
true) so existing callers see the rail-controlled capability; add a separate
low-level helper (e.g., getRawStripeEnv or isRawPaymentsEnabled) that reads only
getStripeEnv().paymentsEnabled for the rare callers that must bypass the rail.
In `@frontend/lib/services/orders/monobank-janitor.ts`:
- Around line 468-476: The predicate currently allows rows with pa.status =
'creating' to match Job2 even when RECONCILABLE_INVOICE_ID_SQL is not null;
update the SQL in both claimJob2Attempts and
atomicCancelOrderAndFailCreatingAttempt so the shared predicate requires
RECONCILABLE_INVOICE_ID_SQL IS NULL for both statuses, and only apply
UNKNOWN_WALLET_SUBMIT_OUTCOME_SQL to further gate 'active' rows (i.e., require
(pa.status = 'creating' OR (pa.status = 'active' AND
UNKNOWN_WALLET_SUBMIT_OUTCOME_SQL)) while ensuring RECONCILABLE_INVOICE_ID_SQL
IS NULL is common to both branches); mirror this corrected predicate in the
other occurrences noted (around the other ranges) so selection and transition
logic stay aligned.
In `@frontend/messages/pl.json`:
- Around line 453-456: The Polish translation for the JSON key
"warehouse.description" contains a grammatical error; update the value for
"warehouse.description" from "Odbiór z oddziale Nova Poshta" to the correct
genitive form "Odbiór z oddziału Nova Poshta" so the preposition "z" is followed
by the genitive case.
In `@frontend/playwright.config.ts`:
- Around line 20-40: The Playwright smoke config currently sets
reuseExistingServer: true which lets Playwright attach to any existing process
on port 3100 and skip the provided command and env block; change
reuseExistingServer to false (or remove it) so Playwright always runs the
configured command ('command') with the intended env flags (the env object
including APP_ENV, PAYMENTS_ENABLED, SHOP_SHIPPING_ENABLED, APP_ORIGIN, etc.) on
port 3100, ensuring the webServer.env flags are applied deterministically.
- Around line 27-29: The test env uses a whitespace sentinel for DATABASE_URL
which fails getServerEnv() zod validation (z.string().url()); update the env in
the Playwright config so DATABASE_URL is an empty string ('' ) instead of ' ' so
getServerEnv()/getRedisClient() won't throw; keep SHOP_STRICT_LOCAL_DB and
SHOP_REQUIRED_DATABASE_URL_LOCAL as-is so the strict-local-db guard still
applies.
In `@frontend/tests/e2e/shop-checkout-local.spec.ts`:
- Around line 604-624: The test currently falls back to creating a statusToken
via createStatusToken when statusTokenFromRedirect is null, which can mask
redirect regressions; update the logic around
statusTokenFromRedirect/statusToken so that if statusTokenFromRedirect is null
the test fails (or at minimum logs a clear error) before attempting the manual
createStatusToken and page.goto — e.g., assert or throw with context including
createdOrderId and currentPaymentUrl when statusTokenFromRedirect is missing,
then only use createStatusToken as an explicit, logged fallback path if you
intentionally want to continue; reference the variables statusTokenFromRedirect,
statusToken, createStatusToken and the page.goto call to locate where to add the
assertion/throw or warning.
---
Nitpick comments:
In `@frontend/app/`[locale]/shop/cart/capabilities.ts:
- Around line 30-35: The function resolveMonobankGooglePayEnabled should be
self-contained and only return true when Monobank checkout is effectively
allowed; update it to first call and respect resolveMonobankCheckoutEnabled()
(or isMonobankEnabled() if that’s the canonical gate) and immediately return
false if that gate is disabled, then fall back to parsing
SHOP_MONOBANK_GPAY_ENABLED as it currently does; ensure you reference and call
the existing resolveMonobankCheckoutEnabled (or isMonobankEnabled) helper within
resolveMonobankGooglePayEnabled so the exported capability reflects effective
availability.
In `@frontend/app/api/shop/checkout/route.ts`:
- Around line 956-970: The catch for createCheckoutStatusToken only logs the
error and returns null, which can let downstream flows (e.g.,
buildMonobankCheckoutResponse) run with a missing statusToken; update the error
handling in the statusToken IIFE to log richer context (include orderMeta,
order.id, order.paymentProvider and the error stack) and then fail fast for
critical flows — either throw a specific Error or return an HTTP error/early
response when statusToken is required (instead of returning null); reference
createCheckoutStatusToken, statusToken, logError, orderMeta and
buildMonobankCheckoutResponse when making this change so callers that require
the token do not proceed with a null value.
In `@frontend/lib/services/orders/restock.ts`:
- Around line 122-127: The call to closeShippingPipelineForOrder is currently
executed before the idempotency guard (the check on order.stockRestored ||
order.restockedAt !== null), causing repeated restock attempts to close the
shipping pipeline unnecessarily; move the await
closeShippingPipelineForOrder({...}) call to after the early return/idempotency
check (i.e., only invoke closeShippingPipelineForOrder when the function
proceeds past the restocked check) and ensure it still uses the same reason
formatting (`reason: \`payment_terminal:${reason}\``) so repeated calls exit
early without performing shipping closure.
In `@frontend/lib/services/orders/summary.ts`:
- Around line 196-203: The ternary fallback handling in the access check is
defensive and should be simplified: inside the failure branch where you
currently compute const code = access.code === 'OK' ? 'FORBIDDEN' : access.code,
remove the special-case and return the actual access.code directly (or inline it
in the returned object) because authorizeOrderMutationAccess should not return
'OK' when authorized is false; update the failure return in the function that
contains this snippet (the access handling in
frontend/lib/services/orders/summary.ts, the branch that processes
authorizeOrderMutationAccess results) to use code: access.code (or simply omit
the temporary variable).
In `@frontend/lib/tests/shop/checkout-no-payments.test.ts`:
- Around line 265-298: The test currently verifies no order row is created but
doesn't assert inventory was not reserved; update the 'Explicit Stripe request
fails closed when Stripe is unavailable' test (the one that calls postCheckout
with paymentProvider: 'stripe') to also fetch and assert the product's stock
remains unchanged and that no inventory_moves were recorded for that
product/idempotency key; after the existing check that no orders row exists,
query products (e.g., select stock from products where id = productId) and
assert stock equals the original stock returned by
createIsolatedProductForCurrency, and query inventory_moves (or the
inventory_moves table) for entries referencing productId (and/or the idemKey)
and assert none exist before finally calling cleanupIsolatedProduct.
In `@frontend/lib/tests/shop/stripe-payment-client-return-route.test.ts`:
- Around line 25-34: The test for nextRouteForPaymentResult currently uses
toContain() and should instead assert the exact URL structure: construct the
expected URL using ORDER_ID and STATUS_TOKEN (or parse the returned route with
URL and assert pathname equals '/shop/checkout/success' and search params
orderId and statusToken match exactly) and replace the two toContain assertions
with a single precise assertion (e.g., expect(route).toBe(expectedUrl) or assert
URL pathname and query params) referencing nextRouteForPaymentResult, ORDER_ID
and STATUS_TOKEN to locate the code to change.
In `@frontend/tests/e2e/shop-checkout-local.spec.ts`:
- Around line 8-10: Replace the hardcoded DSN in REQUIRED_LOCAL_DB_URL with a
value read from an environment variable (e.g., process.env.REQUIRED_LOCAL_DB_URL
or process.env.LOCAL_DB_URL) and keep ALLOWED_LOCAL_DB_HOSTS as-is; validate the
env var at startup and throw a clear error if missing so tests fail fast, and
optionally provide the existing literal as a non-committed default only when a
specific DEV_FALLBACK flag is set to avoid embedding credentials in source
history.
- Around line 439-446: The code in cleanupSeededData calls
bucket.orderIds.reverse(), which mutates the original array; to avoid unintended
side effects if the CleanupBucket is reused later, operate on a copy
instead—e.g., iterate over [...bucket.orderIds].reverse() or
bucket.orderIds.slice().reverse() inside cleanupSeededData so the original
bucket.orderIds remains unchanged; reference cleanupSeededData and
bucket.orderIds (type CleanupBucket) when applying this change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b22597e6-5b46-4b8f-ba9d-4067499e8d02
📒 Files selected for processing (58)
frontend/app/[locale]/shop/cart/CartPageClient.tsxfrontend/app/[locale]/shop/cart/capabilities.tsfrontend/app/[locale]/shop/cart/page.tsxfrontend/app/[locale]/shop/checkout/error/page.tsxfrontend/app/[locale]/shop/checkout/payment/StripePaymentClient.tsxfrontend/app/[locale]/shop/checkout/payment/[orderId]/page.tsxfrontend/app/[locale]/shop/checkout/payment/monobank/[orderId]/page.tsxfrontend/app/[locale]/shop/checkout/return/monobank/MonobankReturnStatus.tsxfrontend/app/[locale]/shop/checkout/success/page.tsxfrontend/app/api/shop/checkout/route.tsfrontend/app/api/shop/webhooks/monobank/route.tsfrontend/app/api/shop/webhooks/stripe/route.tsfrontend/instrumentation.tsfrontend/lib/env/stripe.tsfrontend/lib/psp/monobank.tsfrontend/lib/services/orders.tsfrontend/lib/services/orders/monobank-janitor.tsfrontend/lib/services/orders/monobank-wallet.tsfrontend/lib/services/orders/monobank-webhook.tsfrontend/lib/services/orders/restock.tsfrontend/lib/services/orders/summary.tsfrontend/lib/services/shop/shipping/admin-actions.tsfrontend/lib/services/shop/shipping/eligibility.tsfrontend/lib/services/shop/shipping/pipeline-shutdown.tsfrontend/lib/services/shop/shipping/shipments-worker.tsfrontend/lib/shop/ui-classes.tsfrontend/lib/tests/shop/admin-shipping-canonical-audit.test.tsfrontend/lib/tests/shop/admin-shipping-payment-gate.test.tsfrontend/lib/tests/shop/cart-stripe-capability-alignment.test.tsfrontend/lib/tests/shop/checkout-monobank-parse-validation.test.tsfrontend/lib/tests/shop/checkout-monobank-payment-page-access.test.tsfrontend/lib/tests/shop/checkout-no-payments.test.tsfrontend/lib/tests/shop/checkout-payment-page-access.test.tsfrontend/lib/tests/shop/checkout-shipping-phase3.test.tsfrontend/lib/tests/shop/checkout-stripe-payments-disabled.test.tsfrontend/lib/tests/shop/checkout-success-page-access.test.tsfrontend/lib/tests/shop/monobank-google-pay-config-route.test.tsfrontend/lib/tests/shop/monobank-google-pay-submit-route.test.tsfrontend/lib/tests/shop/monobank-invoice-route-scope.test.tsfrontend/lib/tests/shop/monobank-janitor-job1.test.tsfrontend/lib/tests/shop/monobank-janitor-job2.test.tsfrontend/lib/tests/shop/monobank-wallet-service.test.tsfrontend/lib/tests/shop/monobank-webhook-apply.test.tsfrontend/lib/tests/shop/monobank-webhook-logging-safety.test.tsfrontend/lib/tests/shop/monobank-webhook-mode.test.tsfrontend/lib/tests/shop/monobank-webhook-origin-posture.test.tsfrontend/lib/tests/shop/monobank-webhook-rate-limit-policy.test.tsfrontend/lib/tests/shop/monobank-webhook-rate-limit-scope.test.tsfrontend/lib/tests/shop/monobank-webhook-route-f2.test.tsfrontend/lib/tests/shop/monobank-webhook-signature-verify.test.tsfrontend/lib/tests/shop/shipping-shipments-worker-phase5.test.tsfrontend/lib/tests/shop/stripe-payment-client-return-route.test.tsfrontend/lib/tests/shop/stripe-webhook-refund-full.test.tsfrontend/messages/en.jsonfrontend/messages/pl.jsonfrontend/messages/uk.jsonfrontend/playwright.config.tsfrontend/tests/e2e/shop-checkout-local.spec.ts
![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: 13f8ec0d2a
ℹ️ 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".
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
frontend/playwright.config.ts (1)
27-27:⚠️ Potential issue | 🔴 CriticalDATABASE_URL empty string will fail Zod validation — guard check happens too late.
The empty string
''cannot pass theDATABASE_URL: z.string().url()validator infrontend/lib/env/index.ts(line 10). Zod validation runs eagerly on module load (line 91), before theSHOP_STRICT_LOCAL_DBguard infrontend/db/index.ts(lines 18–42) ever executes. The guard check does not intercept before validation; it cannot run until the module loads successfully. An empty string is not a valid URL and will fail validation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/playwright.config.ts` at line 27, The test Playwright config sets DATABASE_URL to an empty string which fails the z.string().url() validation in frontend/lib/env/index.ts (the z.string().url() schema) before the SHOP_STRICT_LOCAL_DB guard in frontend/db/index.ts can run; update the Playwright config to supply a valid URL or omit the key so validation passes (e.g., replace DATABASE_URL: '' with a valid placeholder URL like 'http://localhost:5432' or remove the entry), ensuring the environment value satisfies the z.string().url() validator and allows the guard logic in SHOP_STRICT_LOCAL_DB to operate later.frontend/app/[locale]/shop/cart/CartPageClient.tsx (1)
578-593:⚠️ Potential issue | 🟠 MajorReject unknown shipping method codes before they reach state.
The current
methodCodeOkcheck accepts any non-empty string and then casts it toCheckoutDeliveryMethodCode. If/api/shop/shipping/methodsever returns a typo or unsupported code, the UI will still render/select it and only fail later in checkout payload building. Validate against the known codes here and hard-block the response instead of casting blindly.Suggested fix
+ const VALID_METHOD_CODES = new Set<CheckoutDeliveryMethodCode>([ + 'NP_WAREHOUSE', + 'NP_LOCKER', + 'NP_COURIER', + ]); + for (const item of methodsRaw) { @@ - const methodCodeOk = - typeof m.methodCode === 'string' && m.methodCode.trim().length > 0; + const methodCode = + typeof m.methodCode === 'string' ? m.methodCode.trim() : ''; + const methodCodeOk = VALID_METHOD_CODES.has( + methodCode as CheckoutDeliveryMethodCode + ); @@ methods.push({ provider: 'nova_poshta', - methodCode: m.methodCode as CheckoutDeliveryMethodCode, + methodCode: methodCode as CheckoutDeliveryMethodCode, title: String(m.title), }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/`[locale]/shop/cart/CartPageClient.tsx around lines 578 - 593, The code currently accepts any non-empty string for m.methodCode and casts it to CheckoutDeliveryMethodCode; change this to validate against the canonical set of delivery codes (e.g. use Object.values(CheckoutDeliveryMethodCode) or an isValidDeliveryMethodCode type guard) before pushing into methods so unknown/typo codes trigger hardBlock() instead of being blindly cast; update the validation around methodCodeOk (and/or add a new check) to verify m.methodCode is one of the allowed CheckoutDeliveryMethodCode values and call hardBlock() when it is not.frontend/lib/services/orders/monobank-janitor.ts (1)
545-577:⚠️ Potential issue | 🔴 CriticalLock the attempt row before canceling the order.
updated_orderonly acquiresACCESS SHAREon thepayment_attemptstable viaFROM(does not lock individual rows). If a concurrent transaction modifies the attempt row afterupdated_ordercommits but beforeupdated_attemptevaluates its predicate,updated_attemptwill re-check the stale predicate against the new row version underREAD COMMITTEDisolation and skip the update if the predicate is now false. This leaves the orderCANCELEDwhile the payment attempt remains live/reconcilable, creating an inconsistent state.Add an explicit lock to select both rows before the updates: introduce an initial CTE with
SELECT ... FOR UPDATE OF pa, ocontaining the full stale predicate, then drive bothupdated_orderandupdated_attemptfrom that locked candidate. This ensures the predicate is evaluated once on rows held under exclusive lock, preventing the race condition.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/services/orders/monobank-janitor.ts` around lines 545 - 577, The race comes from updated_order reading payment_attempts via FROM but not locking rows, so add an initial CTE (e.g., locked_candidate) that SELECTs the payment_attempts (pa) and orders (o) matching the full stale predicate and the same join conditions, using SELECT ... FOR UPDATE OF pa, o to acquire row locks; then change updated_order and updated_attempt to source their rows FROM locked_candidate (instead of directly from payment_attempts or re-evaluating the predicate) so both updates operate on the same locked candidate rows and the stale predicate is only evaluated once under lock.
🧹 Nitpick comments (3)
frontend/playwright.config.ts (1)
32-40: Consider settingMONO_API_BASEto prevent accidental production API calls.Per the context in
frontend/lib/env/monobank.ts, whenMONO_API_BASEis not set, it defaults to'https://api.monobank.ua'(production). WithPAYMENTS_ENABLED: 'true'and a fakeMONO_MERCHANT_TOKEN, any code path that inadvertently makes a real Monobank API call would hit production (and fail with auth errors, but still make network requests).For a truly local/deterministic smoke suite, consider adding an invalid or mock base URL to guarantee no external traffic:
🛡️ Suggested addition
MONO_MERCHANT_TOKEN: 'e2e_local_monobank_token', + MONO_API_BASE: 'http://localhost:9999', // invalid endpoint to block real API calls SHOP_MONOBANK_GPAY_ENABLED: 'true',🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/playwright.config.ts` around lines 32 - 40, Add an explicit MONO_API_BASE env var to the Playwright test environment so the suite cannot accidentally call production; update the env block where PAYMENTS_ENABLED and MONO_MERCHANT_TOKEN are set (in frontend/playwright.config.ts) to include MONO_API_BASE set to a local/mocked URL (e.g., an invalid or localhost URL) so the code in frontend/lib/env/monobank.ts will use that base instead of the default production API.frontend/tests/e2e/shop-checkout-local.spec.ts (1)
719-720: This assertion is not isolating the currency-gating behavior.The test never completes the required shipping fields, so
placeOrderButtonis already disabled for an unrelated reason. If you want this regression to prove Monobank/USD gating, fill the shipping form first or assert a payment-specific disabled/help state instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/tests/e2e/shop-checkout-local.spec.ts` around lines 719 - 720, The test currently checks placeOrderButton (const placeOrderButton = page.locator('button[aria-busy]')) while leaving required shipping fields empty, so the button is disabled for shipping validation rather than Monobank/USD gating; to fix, ensure the shipping form is completed before asserting currency gating (call the existing shipping-fill helper or fill required fields like address/zip/phone) or instead target a payment-specific UI element (e.g., the payment method selector, a payment-submit button, or the currency-gating help/tooltip element) and assert its disabled state or help text related to Monobank/USD; update the test to use the shipping helper or a more specific locator rather than the generic placeOrderButton locator.frontend/app/[locale]/shop/checkout/payment/StripePaymentClient.tsx (1)
93-100: Centralize checkout result URL construction.The same
orderId/statusTokenquery assembly now exists in several places. A small helper returning{ success, failure }would make future route changes much harder to desync.♻️ Proposed refactor
+function buildCheckoutResultPaths(params: { + orderId: string; + statusToken: string | null; +}) { + const id = encodeURIComponent(params.orderId); + const tokenSuffix = params.statusToken + ? `&statusToken=${encodeURIComponent(params.statusToken)}` + : ''; + + return { + success: buildInAppPath(`/checkout/success?orderId=${id}${tokenSuffix}`), + failure: buildInAppPath(`/checkout/error?orderId=${id}${tokenSuffix}`), + }; +} + export function nextRouteForPaymentResult(params: { orderId: string; statusToken: string | null; status?: string | null; }) { - const { orderId, status, statusToken } = params; - const id = encodeURIComponent(orderId); - const tokenSuffix = statusToken - ? `&statusToken=${encodeURIComponent(statusToken)}` - : ''; - - const success = buildInAppPath( - `/checkout/success?orderId=${id}${tokenSuffix}` - ); - const failure = buildInAppPath(`/checkout/error?orderId=${id}${tokenSuffix}`); + const { status } = params; + const { success, failure } = buildCheckoutResultPaths(params);- try { - const id = encodeURIComponent(orderId); - const tokenSuffix = statusToken - ? `&statusToken=${encodeURIComponent(statusToken)}` - : ''; - - const inAppSuccess = buildInAppPath( - `/checkout/success?orderId=${id}${tokenSuffix}` - ); - const inAppFailure = buildInAppPath( - `/checkout/error?orderId=${id}${tokenSuffix}` - ); + const { success: inAppSuccess, failure: inAppFailure } = + buildCheckoutResultPaths({ orderId, statusToken }); + + try { ... } catch (error) { logError('stripe_payment_confirm_failed', error, { orderId }); setErrorMessage('We couldn’t confirm your payment. Please try again.'); - router.push( - buildInAppPath(`/checkout/error?orderId=${id}${tokenSuffix}`) - ); + router.push(inAppFailure); }Also applies to: 186-195, 219-227, 303-305
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/`[locale]/shop/checkout/payment/StripePaymentClient.tsx around lines 93 - 100, Extract the repeated query-string assembly into a single helper (e.g., getCheckoutResultUrls or buildCheckoutResultPaths) that accepts orderId and optional statusToken and returns an object { success, failure } with both URLs already wrapped via buildInAppPath; replace the local tokenSuffix/success/failure logic in StripePaymentClient (the const tokenSuffix, success, failure) and the other duplicated sites in this file with calls to that helper so all checkout result URL construction is centralized and consistent.
🤖 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/`[locale]/shop/cart/CartPageClient.tsx:
- Around line 73-77: The ShippingWarehouse type is too narrow and drops the
locker flag, so preserve the original payload shape (include isPostMachine:
boolean and any other warehouse metadata) where the /np/warehouses response is
normalized (e.g., in the fetch/normalize logic that currently casts to
ShippingWarehouse) and avoid raw casts; then filter the warehouse list by
isPostMachine when the delivery method equals NP_LOCKER before calling the
setter that stores warehouses in state (look for ShippingWarehouse,
isPostMachine, NP_LOCKER, and the state updater like setShippingWarehouses or
similar in CartPageClient.tsx) so locker-only options are presented and regular
branches are excluded from the UI.
In `@frontend/app/`[locale]/shop/checkout/payment/[orderId]/page.tsx:
- Around line 238-255: Guard this route to Stripe-only orders by checking the
order's payment provider before running any Stripe-specific logic: early-return
the existing not-found shell (or redirect Monobank orders to their payment page)
if the order.provider/payment method is not "stripe". Specifically, add a check
using the retrieved order (order or order.paymentProvider) immediately after
order is loaded and before calling isStripePaymentsEnabled, getStripeEnv,
resolveClientSecret, canInitializeStripeForPaymentStatus,
ensureStripePaymentIntentForOrder, or rendering StripePaymentClient; when the
provider isn't Stripe, return the not-found response or perform the Monobank
redirect and avoid initializing or rendering any Stripe code.
In `@frontend/app/api/shop/checkout/route.ts`:
- Around line 740-760: When requestedProvider === 'stripe' but
stripeCheckoutAvailable is false, the code currently immediately returns a 503
and skips any idempotent recovery; change this to first attempt to resolve an
existing order using the idempotency flow (e.g. call/createOrderWithItems() or
the same idempotency lookup used elsewhere with the incoming Idempotency-Key)
and if that returns an existing order return that successful result, otherwise
proceed to logWarn('checkout_stripe_payments_disabled', {...baseMeta, code:
'PAYMENTS_DISABLED'}) and return the errorResponse('PSP_UNAVAILABLE', 'Payment
provider unavailable.', 503); keep the existing symbols stripeCheckoutAvailable,
requestedProvider, createOrderWithItems(), logWarn and errorResponse to locate
and implement the fix.
- Around line 991-1023: If createCheckoutStatusToken (statusToken) fails while
statusTokenRequired is true and the order was just created (result.isNew),
perform compensation before returning the error: call the order
cancellation/cleanup routine used elsewhere (e.g., cancelOrder or
releaseOrderReservation) to mark the order as cancelled and restore
inventory/stock, include the order id and reason in the log, and handle/log any
errors from that compensation action; after successful (or attempted)
compensation return the existing errorResponse('CHECKOUT_FAILED', ...) as
before. Ensure you locate this logic around the statusToken/statusTokenRequired
check and use the same cancellation function and inventory-restoration helpers
the codebase uses so side-effects are reverted when createCheckoutStatusToken
fails.
In `@frontend/lib/services/orders/monobank-janitor.ts`:
- Around line 49-57: The predicate in JOB2_STALE_ATTEMPT_PREDICATE_SQL treats
pa.provider_payment_intent_id strictly as null, so rows with
provider_payment_intent_id = '' are excluded; change the check to treat empty
string as missing (e.g., use NULLIF(pa.provider_payment_intent_id, '') is null
or an equivalent SQL expression) so that pa.provider_payment_intent_id = '' is
handled the same as null; update the predicate referencing
pa.provider_payment_intent_id inside JOB2_STALE_ATTEMPT_PREDICATE_SQL
accordingly to match how RECONCILABLE_INVOICE_ID_SQL collapses '' to null.
---
Outside diff comments:
In `@frontend/app/`[locale]/shop/cart/CartPageClient.tsx:
- Around line 578-593: The code currently accepts any non-empty string for
m.methodCode and casts it to CheckoutDeliveryMethodCode; change this to validate
against the canonical set of delivery codes (e.g. use
Object.values(CheckoutDeliveryMethodCode) or an isValidDeliveryMethodCode type
guard) before pushing into methods so unknown/typo codes trigger hardBlock()
instead of being blindly cast; update the validation around methodCodeOk (and/or
add a new check) to verify m.methodCode is one of the allowed
CheckoutDeliveryMethodCode values and call hardBlock() when it is not.
In `@frontend/lib/services/orders/monobank-janitor.ts`:
- Around line 545-577: The race comes from updated_order reading
payment_attempts via FROM but not locking rows, so add an initial CTE (e.g.,
locked_candidate) that SELECTs the payment_attempts (pa) and orders (o) matching
the full stale predicate and the same join conditions, using SELECT ... FOR
UPDATE OF pa, o to acquire row locks; then change updated_order and
updated_attempt to source their rows FROM locked_candidate (instead of directly
from payment_attempts or re-evaluating the predicate) so both updates operate on
the same locked candidate rows and the stale predicate is only evaluated once
under lock.
In `@frontend/playwright.config.ts`:
- Line 27: The test Playwright config sets DATABASE_URL to an empty string which
fails the z.string().url() validation in frontend/lib/env/index.ts (the
z.string().url() schema) before the SHOP_STRICT_LOCAL_DB guard in
frontend/db/index.ts can run; update the Playwright config to supply a valid URL
or omit the key so validation passes (e.g., replace DATABASE_URL: '' with a
valid placeholder URL like 'http://localhost:5432' or remove the entry),
ensuring the environment value satisfies the z.string().url() validator and
allows the guard logic in SHOP_STRICT_LOCAL_DB to operate later.
---
Nitpick comments:
In `@frontend/app/`[locale]/shop/checkout/payment/StripePaymentClient.tsx:
- Around line 93-100: Extract the repeated query-string assembly into a single
helper (e.g., getCheckoutResultUrls or buildCheckoutResultPaths) that accepts
orderId and optional statusToken and returns an object { success, failure } with
both URLs already wrapped via buildInAppPath; replace the local
tokenSuffix/success/failure logic in StripePaymentClient (the const tokenSuffix,
success, failure) and the other duplicated sites in this file with calls to that
helper so all checkout result URL construction is centralized and consistent.
In `@frontend/playwright.config.ts`:
- Around line 32-40: Add an explicit MONO_API_BASE env var to the Playwright
test environment so the suite cannot accidentally call production; update the
env block where PAYMENTS_ENABLED and MONO_MERCHANT_TOKEN are set (in
frontend/playwright.config.ts) to include MONO_API_BASE set to a local/mocked
URL (e.g., an invalid or localhost URL) so the code in
frontend/lib/env/monobank.ts will use that base instead of the default
production API.
In `@frontend/tests/e2e/shop-checkout-local.spec.ts`:
- Around line 719-720: The test currently checks placeOrderButton (const
placeOrderButton = page.locator('button[aria-busy]')) while leaving required
shipping fields empty, so the button is disabled for shipping validation rather
than Monobank/USD gating; to fix, ensure the shipping form is completed before
asserting currency gating (call the existing shipping-fill helper or fill
required fields like address/zip/phone) or instead target a payment-specific UI
element (e.g., the payment method selector, a payment-submit button, or the
currency-gating help/tooltip element) and assert its disabled state or help text
related to Monobank/USD; update the test to use the shipping helper or a more
specific locator rather than the generic placeOrderButton locator.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f18f9fa8-785c-48f7-a4f1-e76c48ff655e
📒 Files selected for processing (17)
frontend/app/[locale]/shop/cart/CartPageClient.tsxfrontend/app/[locale]/shop/cart/capabilities.tsfrontend/app/[locale]/shop/checkout/payment/StripePaymentClient.tsxfrontend/app/[locale]/shop/checkout/payment/[orderId]/page.tsxfrontend/app/api/shop/checkout/route.tsfrontend/lib/env/stripe.tsfrontend/lib/services/orders/monobank-janitor.tsfrontend/lib/services/orders/restock.tsfrontend/lib/services/orders/summary.tsfrontend/lib/tests/shop/cart-stripe-capability-alignment.test.tsfrontend/lib/tests/shop/checkout-no-payments.test.tsfrontend/lib/tests/shop/checkout-payment-page-access.test.tsfrontend/lib/tests/shop/stripe-payment-client-return-route.test.tsfrontend/messages/pl.jsonfrontend/playwright.config.tsfrontend/tests/e2e/shop-checkout-local.spec.tsfrontend/tests/e2e/shop-minimal-phase9.spec.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- frontend/app/[locale]/shop/cart/capabilities.ts
- frontend/lib/services/orders/restock.ts
- frontend/lib/tests/shop/stripe-payment-client-return-route.test.ts
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/[locale]/shop/cart/CartPageClient.tsx (2)
1159-1168:⚠️ Potential issue | 🟠 MajorValidate external recovery URLs before navigating to them.
pageUrlis trusted as any non-empty string, then passed towindow.location.assignand rendered back as an external link. Reject unexpected schemes here; otherwise a bad upstream value can turn this into script execution or an off-flow redirect.Suggested fix
+function normalizeExternalRecoveryUrl(value: unknown): string | null { + if (typeof value !== 'string' || value.trim().length === 0) return null; + + try { + const url = new URL(value); + return url.protocol === 'https:' ? url.toString() : null; + } catch { + return null; + } +} + @@ - const monobankPageUrl: string | null = - typeof data.pageUrl === 'string' && data.pageUrl.trim().length > 0 - ? data.pageUrl - : null; + const monobankPageUrl = normalizeExternalRecoveryUrl(data.pageUrl);Also applies to: 1220-1221, 1263-1265
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/`[locale]/shop/cart/CartPageClient.tsx around lines 1159 - 1168, monobankPageUrl is currently accepted as any non-empty string from data.pageUrl and later used in window.location.assign and rendered as an external link; validate and sanitize it by attempting to construct a URL object from data.pageUrl and only accept it when the URL parsing succeeds and url.protocol is 'http:' or 'https:' (reject other schemes and malformed input), trimming whitespace and rejecting control characters; if validation fails, set monobankPageUrl to null. Apply the same validation logic where pageUrl is read/used (references: monobankPageUrl, data.pageUrl, and the window.location.assign / external link rendering sites around the blocks noted at lines ~1220-1221 and ~1263-1265).
1198-1234:⚠️ Potential issue | 🟠 MajorFail closed when a Monobank checkout does not come back as Monobank.
If the user explicitly chose Monobank and the response is missing
paymentProvider === 'monobank', this still falls through to/checkout/success?clearCart=1. That clears the cart after a failed wallet init.Suggested fix
- if (paymentProvider === 'monobank') { + if (selectedProvider === 'monobank') { + if (paymentProvider !== 'monobank') { + setCheckoutError(t('checkout.errors.unexpectedResponse')); + return; + } + if (checkoutPaymentMethod === 'monobank_google_pay') { if (!statusToken) { setCheckoutError(t('checkout.errors.unexpectedResponse')); return; } @@ setPaymentRecoveryUrl(monobankPageUrl); window.location.assign(monobankPageUrl); return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/`[locale]/shop/cart/CartPageClient.tsx around lines 1198 - 1234, When the user selected a Monobank method (check checkoutPaymentMethod for values starting with 'monobank' such as 'monobank' or 'monobank_google_pay') but the response's paymentProvider !== 'monobank', the code currently falls through to the success route and clears the cart; change this to fail closed: add an early guard that if checkoutPaymentMethod indicates Monobank but paymentProvider !== 'monobank' then call setCheckoutError(t('checkout.errors.unexpectedResponse')) and return (do not set paymentRecoveryUrl or call router.push/window.location.assign), preserving the cart; update the block that handles monobank_google_pay/statusToken and the subsequent monobankPageUrl branch to only execute when paymentProvider === 'monobank'.
🧹 Nitpick comments (5)
frontend/lib/tests/shop/checkout-route-stripe-disabled-recovery.test.ts (2)
132-135: Minor: Redundant mock resets aftervi.clearAllMocks().
vi.clearAllMocks()on line 116 already clears call history. ThemockReset()calls here additionally reset implementations, but those are immediately overwritten by the default implementations set in lines 118-130 during the nextbeforeEachrun.These lines are harmless but unnecessary since no implementation state persists across tests anyway.
♻️ Optional: Remove redundant resets
mockReadPositiveIntEnv.mockImplementation( (_name: string, fallback: number) => fallback ); - - mockCreateOrderWithItems.mockReset(); - mockFindExistingCheckoutOrderByIdempotencyKey.mockReset(); - mockRestockOrder.mockReset(); - mockEnsureStripePaymentIntentForOrder.mockReset(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/tests/shop/checkout-route-stripe-disabled-recovery.test.ts` around lines 132 - 135, The four mockReset() calls (mockCreateOrderWithItems, mockFindExistingCheckoutOrderByIdempotencyKey, mockRestockOrder, mockEnsureStripePaymentIntentForOrder) are redundant because vi.clearAllMocks() already clears call history and the tests reassign implementations in the next beforeEach; remove those mockReset() lines to simplify the test setup and avoid unnecessary resets while leaving vi.clearAllMocks() and the subsequent default mock implementations intact.
240-242: Remove unnecessaryas anytype cast.The other test cases call
POST(makeRequest(...))without a type assertion (lines 161, 184, 209). Thisas anyis inconsistent and could mask type errors. SincemakeRequestreturnsNextRequest, which is the expected parameter type, the cast is unnecessary.♻️ Proposed fix
const response = await POST( - makeRequest({ paymentProvider: 'stripe' }) as any + makeRequest({ paymentProvider: 'stripe' }) );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/tests/shop/checkout-route-stripe-disabled-recovery.test.ts` around lines 240 - 242, The test unnecessarily casts the makeRequest result with "as any" when calling POST; remove the "as any" type assertion and pass the NextRequest directly (i.e., call POST(makeRequest({ paymentProvider: 'stripe' })) ) so it matches the other tests; if TypeScript then complains, ensure makeRequest's return type is NextRequest or adjust makeRequest's signature rather than adding a cast—refer to the POST function and makeRequest helper to locate the call to update.frontend/app/api/shop/checkout/route.ts (2)
1080-1091: DuplicateorderMetain log context.Line 1088 nests
orderMetainside the log payload that already spreads...orderMeta, resulting in redundant data.♻️ Remove duplicate field
logError('checkout_status_token_create_failed', error, { ...orderMeta, code: 'STATUS_TOKEN_CREATE_FAILED', statusTokenRequired, tokenScopes: resolveCheckoutTokenScopes({ paymentProvider: order.paymentProvider, paymentStatus: order.paymentStatus, }), - orderMeta, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/api/shop/checkout/route.ts` around lines 1080 - 1091, The log payload passed to logError in the checkout status token creation block duplicates orderMeta (it spreads ...orderMeta and then includes orderMeta again). Update the payload passed to logError (the call that includes code 'STATUS_TOKEN_CREATE_FAILED' and tokenScopes from resolveCheckoutTokenScopes) by removing the redundant orderMeta property so only the spread ...orderMeta remains.
355-382: Type assertions bypass field validation.
extractRecoveredCheckoutOrdercasts values toCheckoutOrderShapewithout validating that required fields (id,currency,totalAmount, etc.) actually exist. IffindExistingCheckoutOrderByIdempotencyKeyreturns a malformed object, this could cause runtime errors downstream.Consider adding minimal field validation:
♻️ Optional: Add minimal field validation
function extractRecoveredCheckoutOrder( value: unknown ): CheckoutOrderShape | null { if (!value || typeof value !== 'object') return null; + const candidate = + 'order' in value && value.order && typeof value.order === 'object' + ? (value.order as Record<string, unknown>) + : 'id' in value + ? (value as Record<string, unknown>) + : null; + + if (!candidate || typeof candidate.id !== 'string') return null; + + return candidate as CheckoutOrderShape; - if ('order' in value && value.order && typeof value.order === 'object') { - return value.order as CheckoutOrderShape; - } - - if ('id' in value) { - return value as CheckoutOrderShape; - } - - return null; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/api/shop/checkout/route.ts` around lines 355 - 382, extractRecoveredCheckoutOrder currently uses type assertions that can let malformed objects through; update it to perform explicit runtime checks on required fields before casting: when handling value.order (in extractRecoveredCheckoutOrder) and when handling a top-level object with 'id', verify presence and types of required properties (e.g., id as string, currency as string, totalAmount or totalCents as number, and any other mandatory fields your CheckoutOrderShape requires) and only then return the object as CheckoutOrderShape, otherwise return null; ensure buildRecoveredCheckoutResult continues to safely handle optional fields like paymentIntentId (already normalized to null) and won’t assume other fields exist.frontend/tests/e2e/shop-checkout-local.spec.ts (1)
844-849: Replace the fixed sleep with an event-driven “still pending” check.
waitForTimeout(3_500)makes this smoke test depend on the current polling cadence. That can both flake and miss a redirect that happens just after the sleep. Prefer stubbing/pinning the status response topendingand asserting the URL stays on the return route without a hard delay.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/tests/e2e/shop-checkout-local.spec.ts` around lines 844 - 849, Replace the fixed sleep by stubbing/pinning the backend status response and asserting the URL remains on the return route: use Playwright's page.route or page.waitForResponse to intercept the status/check endpoint used by the checkout flow, return a persistent "pending" response for that route, then assert page.getByText('Payment pending') is visible and use expect(page).toHaveURL(/\/en\/shop\/checkout\/return\/monobank/) to verify no redirect occurs; remove the await page.waitForTimeout(3_500) and ensure the route stub is registered before the checkout flow that polls status starts.
🤖 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/`[locale]/shop/cart/CartPageClient.tsx:
- Around line 1416-1429: The header wrapper is using SHOP_CART_SECTION_HEADER
(which only defines typography) causing the header layout to lose its
flex/padding/border; change the wrapper div class to the cart header layout
class used by the orders card (i.e., the layout wrapper class used elsewhere for
card headers) and move SHOP_CART_SECTION_HEADER so it only wraps the h2 text
(apply SHOP_CART_SECTION_HEADER to the h2 element), and make the same change for
the other occurrences mentioned (around the symbols SHOP_CART_CARD and
SHOP_CART_SECTION_HEADER at the other sections).
- Around line 297-330: SelectableCard currently doesn't show a visible keyboard
focus because the hidden radio gets focus while the label lacks :focus-within
styles; update SelectableCard to add a focus-visible/focus-within styling class
to the label (e.g. append a new SHOP_CART_SELECTABLE_CARD_FOCUSED or utility
classes like "focus-within:ring-2 focus-within:ring-primary
focus-within:outline-none") so when the inner radio receives focus the label
shows a clear focus ring/outline; modify the className composition in
SelectableCard (the label element) to include that focus class alongside
SHOP_CART_SELECTABLE_CARD and the existing selected/disabled classes.
In `@frontend/tests/e2e/shop-checkout-local.spec.ts`:
- Around line 456-479: The cleanupSeededData helper currently swallows all
errors from cleanupOrder, cleanupProduct, and cleanupCity, which can hide FK or
side-effect failures; change it to surface failures by collecting or rethrowing
errors instead of no-ops: inside cleanupSeededData (and its loops over
CleanupBucket.orderIds, .productIds, .cityRefs) capture any thrown error from
cleanupOrder/cleanupProduct/cleanupCity, attach context (operation and id/ref),
push into an errors array, and after all attempts if errors.length > 0 throw a
single aggregated Error (or rethrow the first) so test runs fail when deletions
do not succeed. Ensure references to the functions cleanupSeededData,
cleanupOrder, cleanupProduct, cleanupCity and the CleanupBucket data are used so
the fix is applied to the correct locations.
---
Outside diff comments:
In `@frontend/app/`[locale]/shop/cart/CartPageClient.tsx:
- Around line 1159-1168: monobankPageUrl is currently accepted as any non-empty
string from data.pageUrl and later used in window.location.assign and rendered
as an external link; validate and sanitize it by attempting to construct a URL
object from data.pageUrl and only accept it when the URL parsing succeeds and
url.protocol is 'http:' or 'https:' (reject other schemes and malformed input),
trimming whitespace and rejecting control characters; if validation fails, set
monobankPageUrl to null. Apply the same validation logic where pageUrl is
read/used (references: monobankPageUrl, data.pageUrl, and the
window.location.assign / external link rendering sites around the blocks noted
at lines ~1220-1221 and ~1263-1265).
- Around line 1198-1234: When the user selected a Monobank method (check
checkoutPaymentMethod for values starting with 'monobank' such as 'monobank' or
'monobank_google_pay') but the response's paymentProvider !== 'monobank', the
code currently falls through to the success route and clears the cart; change
this to fail closed: add an early guard that if checkoutPaymentMethod indicates
Monobank but paymentProvider !== 'monobank' then call
setCheckoutError(t('checkout.errors.unexpectedResponse')) and return (do not set
paymentRecoveryUrl or call router.push/window.location.assign), preserving the
cart; update the block that handles monobank_google_pay/statusToken and the
subsequent monobankPageUrl branch to only execute when paymentProvider ===
'monobank'.
---
Nitpick comments:
In `@frontend/app/api/shop/checkout/route.ts`:
- Around line 1080-1091: The log payload passed to logError in the checkout
status token creation block duplicates orderMeta (it spreads ...orderMeta and
then includes orderMeta again). Update the payload passed to logError (the call
that includes code 'STATUS_TOKEN_CREATE_FAILED' and tokenScopes from
resolveCheckoutTokenScopes) by removing the redundant orderMeta property so only
the spread ...orderMeta remains.
- Around line 355-382: extractRecoveredCheckoutOrder currently uses type
assertions that can let malformed objects through; update it to perform explicit
runtime checks on required fields before casting: when handling value.order (in
extractRecoveredCheckoutOrder) and when handling a top-level object with 'id',
verify presence and types of required properties (e.g., id as string, currency
as string, totalAmount or totalCents as number, and any other mandatory fields
your CheckoutOrderShape requires) and only then return the object as
CheckoutOrderShape, otherwise return null; ensure buildRecoveredCheckoutResult
continues to safely handle optional fields like paymentIntentId (already
normalized to null) and won’t assume other fields exist.
In `@frontend/lib/tests/shop/checkout-route-stripe-disabled-recovery.test.ts`:
- Around line 132-135: The four mockReset() calls (mockCreateOrderWithItems,
mockFindExistingCheckoutOrderByIdempotencyKey, mockRestockOrder,
mockEnsureStripePaymentIntentForOrder) are redundant because vi.clearAllMocks()
already clears call history and the tests reassign implementations in the next
beforeEach; remove those mockReset() lines to simplify the test setup and avoid
unnecessary resets while leaving vi.clearAllMocks() and the subsequent default
mock implementations intact.
- Around line 240-242: The test unnecessarily casts the makeRequest result with
"as any" when calling POST; remove the "as any" type assertion and pass the
NextRequest directly (i.e., call POST(makeRequest({ paymentProvider: 'stripe'
})) ) so it matches the other tests; if TypeScript then complains, ensure
makeRequest's return type is NextRequest or adjust makeRequest's signature
rather than adding a cast—refer to the POST function and makeRequest helper to
locate the call to update.
In `@frontend/tests/e2e/shop-checkout-local.spec.ts`:
- Around line 844-849: Replace the fixed sleep by stubbing/pinning the backend
status response and asserting the URL remains on the return route: use
Playwright's page.route or page.waitForResponse to intercept the status/check
endpoint used by the checkout flow, return a persistent "pending" response for
that route, then assert page.getByText('Payment pending') is visible and use
expect(page).toHaveURL(/\/en\/shop\/checkout\/return\/monobank/) to verify no
redirect occurs; remove the await page.waitForTimeout(3_500) and ensure the
route stub is registered before the checkout flow that polls status starts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b2224c66-5ce1-4e68-8d2c-68e3f8ff35cf
📒 Files selected for processing (9)
frontend/app/[locale]/shop/cart/CartPageClient.tsxfrontend/app/[locale]/shop/checkout/payment/[orderId]/page.tsxfrontend/app/api/shop/checkout/route.tsfrontend/lib/services/orders.tsfrontend/lib/services/orders/checkout.tsfrontend/lib/services/orders/monobank-janitor.tsfrontend/lib/tests/shop/checkout-route-stripe-disabled-recovery.test.tsfrontend/playwright.config.tsfrontend/tests/e2e/shop-checkout-local.spec.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/playwright.config.ts
- frontend/lib/services/orders.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
frontend/app/[locale]/shop/cart/CartPageClient.tsx (1)
1252-1260: Consider converting unreachable fallback to fail-closed.Since
CheckoutProvideris typed as'stripe' | 'monobank', this fallback is type-theoretically unreachable. However, it still performs a success redirect withclearCart=1, which contradicts the PR's fail-closed hardening goal.For defense-in-depth, consider replacing this with an error state:
🛡️ Suggested fail-closed fallback
- const paymentsDisabledFlag = - paymentProvider !== 'stripe' || !clientSecret - ? '&paymentsDisabled=true' - : ''; - - router.push( - `${shopBase}/checkout/success?orderId=${encodeURIComponent( - orderId - )}&clearCart=1${paymentsDisabledFlag}${statusTokenQuery}` - ); + // Unreachable with current types, but fail closed for safety + setCheckoutError(t('checkout.errors.unexpectedResponse'));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/`[locale]/shop/cart/CartPageClient.tsx around lines 1252 - 1260, The current fallback path (where CheckoutProvider is neither 'stripe' nor 'monobank') still performs a success redirect using paymentsDisabledFlag and router.push, which undermines the fail-closed goal; update the logic in CartPageClient.tsx (the branch that computes paymentsDisabledFlag and calls router.push) so that the unreachable default case does not redirect to a success page—instead surface an error: log the unexpected CheckoutProvider value (include the provider value and any relevant context), set an explicit error state or navigate to a dedicated error/failure route, and ensure no clearCart=1 success redirect occurs; reference the CheckoutProvider type, paymentsDisabledFlag computation, and the router.push call when making this change.frontend/tests/e2e/shop-checkout-local.spec.ts (1)
625-627: Avoid presentation-coupled selectors for the total assertions.
aside .text-2xl.font-boldanddd.nth(1)are tightly coupled to the current checkout card markup, so harmless UI refactors will break this smoke suite. Prefer a stable accessible locator or a dedicated test id for the summary total rows.Also applies to: 661-666
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/tests/e2e/shop-checkout-local.spec.ts` around lines 625 - 627, The selectors in the assertions (page.locator('aside .text-2xl.font-bold') and the use of dd.nth(1)) are brittle and tied to presentation; change the test to use a stable accessible locator or a dedicated test id instead: update the checkout summary markup to add a data-testid (e.g. data-testid="summary-total" or data-testid="order-total") on the total element and then replace page.locator('aside .text-2xl.font-bold').first().innerText() with page.getByTestId('summary-total').innerText() (or, if you prefer accessibility, use page.getByRole/getByLabel with the visible label like page.getByRole('heading', { name: /total/i })). Do the same replacement for the dd.nth(1) usage so tests rely on getByTestId/getByRole rather than CSS presentation classes.frontend/app/api/shop/checkout/route.ts (1)
355-368: Avoid hardcoding a second runtime copy of payment providers/statuses.
normalizeRecoveredCheckoutOrder()now depends on these local allow-lists, so the nextPaymentStatusorPaymentProvideraddition will silently make valid recovered orders look malformed and turn recovery into a 503. Prefer deriving these guards from the shared payments runtime constants/schema instead of duplicating the union here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/api/shop/checkout/route.ts` around lines 355 - 368, The local allow-lists VALID_CHECKOUT_PAYMENT_PROVIDERS and VALID_CHECKOUT_PAYMENT_STATUSES cause duplication and can go out of sync with the canonical payment schema; update normalizeRecoveredCheckoutOrder to derive its guards from the shared payments runtime constants/schema (the canonical export that defines PaymentProvider/PaymentStatus) instead of the hardcoded Sets, e.g. import the runtime arrays/enums or schema validators used elsewhere in the payments module and use those to validate provider/status values; ensure the new validation preserves the same behavior (treat unknown values as invalid for recovery) and replace references to VALID_CHECKOUT_PAYMENT_PROVIDERS and VALID_CHECKOUT_PAYMENT_STATUSES accordingly.
🤖 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/checkout/route.ts`:
- Around line 1102-1127: The recovery branch that calls
findExistingCheckoutOrderByIdempotencyKey -> extractRecoveredCheckoutOrder ->
buildRecoveredCheckoutResult bypasses idempotency/conflict checks and returns a
fresh status token without validating against the current request; change this
to either (A) re-run the recovered order through the same idempotency contract
used by createOrderWithItems (i.e., call createOrderWithItems with the original
request payload and idempotencyKey so it performs currency, items, locale,
shipping, legalConsent and request-hash checks) or (B) add explicit validation
after extracting recoveredOrder: compare recoveredOrder.items, currency, locale,
country, shipping address/method, user identity and legalConsentVersion against
the incoming request and reject with an idempotency conflict (or 409/appropriate
error) if any mismatch; update the branch that currently builds
recoveredCheckoutResult to perform one of these paths before returning the
recovered order.
In `@frontend/tests/e2e/shop-checkout-local.spec.ts`:
- Around line 674-677: The teardown in the finally blocks (calling
stopActivePageEffects(page) and cleanupSeededData(bucket)) can throw and
overwrite an earlier test failure; change each finally to preserve the primary
error by storing any thrown error from the test body (e.g., let primaryError)
then run teardown inside a try/catch: if teardown throws, attach or log the
teardown error as supplemental (e.g., add as property or use console.error) but
rethrow the original primaryError (or rethrow the teardown only if there was no
primaryError). Update the finally blocks that call stopActivePageEffects and
cleanupSeededData (and the other similar finallys at the ranges mentioned) to
follow this pattern so cleanup errors don’t replace test failures.
---
Nitpick comments:
In `@frontend/app/`[locale]/shop/cart/CartPageClient.tsx:
- Around line 1252-1260: The current fallback path (where CheckoutProvider is
neither 'stripe' nor 'monobank') still performs a success redirect using
paymentsDisabledFlag and router.push, which undermines the fail-closed goal;
update the logic in CartPageClient.tsx (the branch that computes
paymentsDisabledFlag and calls router.push) so that the unreachable default case
does not redirect to a success page—instead surface an error: log the unexpected
CheckoutProvider value (include the provider value and any relevant context),
set an explicit error state or navigate to a dedicated error/failure route, and
ensure no clearCart=1 success redirect occurs; reference the CheckoutProvider
type, paymentsDisabledFlag computation, and the router.push call when making
this change.
In `@frontend/app/api/shop/checkout/route.ts`:
- Around line 355-368: The local allow-lists VALID_CHECKOUT_PAYMENT_PROVIDERS
and VALID_CHECKOUT_PAYMENT_STATUSES cause duplication and can go out of sync
with the canonical payment schema; update normalizeRecoveredCheckoutOrder to
derive its guards from the shared payments runtime constants/schema (the
canonical export that defines PaymentProvider/PaymentStatus) instead of the
hardcoded Sets, e.g. import the runtime arrays/enums or schema validators used
elsewhere in the payments module and use those to validate provider/status
values; ensure the new validation preserves the same behavior (treat unknown
values as invalid for recovery) and replace references to
VALID_CHECKOUT_PAYMENT_PROVIDERS and VALID_CHECKOUT_PAYMENT_STATUSES
accordingly.
In `@frontend/tests/e2e/shop-checkout-local.spec.ts`:
- Around line 625-627: The selectors in the assertions (page.locator('aside
.text-2xl.font-bold') and the use of dd.nth(1)) are brittle and tied to
presentation; change the test to use a stable accessible locator or a dedicated
test id instead: update the checkout summary markup to add a data-testid (e.g.
data-testid="summary-total" or data-testid="order-total") on the total element
and then replace page.locator('aside .text-2xl.font-bold').first().innerText()
with page.getByTestId('summary-total').innerText() (or, if you prefer
accessibility, use page.getByRole/getByLabel with the visible label like
page.getByRole('heading', { name: /total/i })). Do the same replacement for the
dd.nth(1) usage so tests rely on getByTestId/getByRole rather than CSS
presentation classes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f3af6cff-29af-495a-afd8-f2654ca8946f
📒 Files selected for processing (4)
frontend/app/[locale]/shop/cart/CartPageClient.tsxfrontend/app/api/shop/checkout/route.tsfrontend/lib/tests/shop/checkout-route-stripe-disabled-recovery.test.tsfrontend/tests/e2e/shop-checkout-local.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/lib/tests/shop/checkout-route-stripe-disabled-recovery.test.ts
…abilize local shop e2e
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/app/api/shop/checkout/route.ts (1)
848-878:⚠️ Potential issue | 🟠 MajorDefaulted Stripe checkouts still bypass the fail-closed path.
When the payload omits
paymentProvider/paymentMethod,selectedProviderstill defaults to Stripe, butstripeRequestedButUnavailablestaysfalsebecause it only looks at the raw request fields. With Stripe disabled, that falls through tocheckoutPaymentProvider: 'none'instead of returning the intended 503/recovery response. Base the unavailable check on the resolved provider (selectedProvider === 'stripe') rather than only explicit raw fields.Suggested direction
- const stripeExplicitlyRequested = - requestedProvider === 'stripe' || requestedMethod === 'stripe_card'; - - const stripeRequestedButUnavailable = - stripeExplicitlyRequested && !stripeCheckoutAvailable; + const stripeRequested = selectedProvider === 'stripe'; + + const stripeRequestedButUnavailable = + stripeRequested && !stripeCheckoutAvailable;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/api/shop/checkout/route.ts` around lines 848 - 878, The current logic determines stripeRequestedButUnavailable from raw request fields (requestedProvider/requestedMethod) which misses the case when the provider defaults to Stripe (selectedProvider) and thus can bypass the fail-closed path; update the check so it uses the resolved provider: compute stripeRequestedButUnavailable as (selectedProvider === 'stripe') && !stripeCheckoutAvailable (instead of using requestedProvider/requestedMethod), ensuring checkoutPaymentProvider falls back to 'none' and triggers the intended 503/recovery behavior when Stripe is unavailable.
♻️ Duplicate comments (1)
frontend/app/[locale]/shop/cart/CartPageClient.tsx (1)
1191-1202:⚠️ Potential issue | 🟠 MajorKeep the returned token on the generic recovery fallback.
We still only persist
createdOrderIdfor the non-provider-specific fallback. If checkout returnsorderId+statusTokenbut we never setpaymentRecoveryUrl(for example, an incomplete provider init), the rendered/shop/orders/{id}fallback drops the only guest auth context and can become a dead recovery path. Persist the token alongside the id and append it here, or suppress the generic fallback unless a valid recovery target exists.Suggested direction
const [createdOrderId, setCreatedOrderId] = useState<string | null>(null); + const [createdOrderStatusToken, setCreatedOrderStatusToken] = useState< + string | null + >(null); const [paymentRecoveryUrl, setPaymentRecoveryUrl] = useState<string | null>( null ); @@ setCheckoutError(null); setDeliveryUiError(null); setCreatedOrderId(null); + setCreatedOrderStatusToken(null); setPaymentRecoveryUrl(null); @@ const statusToken: string | null = typeof data.statusToken === 'string' && data.statusToken.trim().length > 0 ? data.statusToken : null; const orderId = String(data.orderId); setCreatedOrderId(orderId); + setCreatedOrderStatusToken(statusToken); @@ const orderDetailsHref = createdOrderId - ? `/shop/orders/${encodeURIComponent(createdOrderId)}` + ? `/shop/orders/${encodeURIComponent(createdOrderId)}${ + createdOrderStatusToken + ? `?statusToken=${encodeURIComponent(createdOrderStatusToken)}` + : '' + }` : null;#!/bin/bash set -euo pipefail printf '\n== Token usage in order/payment routes ==\n' rg -n -C3 'statusToken|order_payment_init|status_lite' frontend/app frontend/lib printf '\n== Recovery-link construction in CartPageClient ==\n' sed -n '1188,1316p' "frontend/app/[locale]/shop/cart/CartPageClient.tsx"Expected result: either the order-details route already accepts the token and this fallback should carry it, or it does not, in which case the raw
/shop/orders/{id}fallback should be suppressed for guest recovery flows.Also applies to: 1308-1316
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/`[locale]/shop/cart/CartPageClient.tsx around lines 1191 - 1202, The generic recovery fallback currently only persists createdOrderId (via setCreatedOrderId) and builds statusTokenQuery locally but may drop the guest auth token; update the flow to also persist the statusToken when present (e.g., store alongside createdOrderId or set a paymentRecoveryToken variable) and ensure paymentRecoveryUrl or the fallback link includes the encoded statusToken (use statusTokenQuery when constructing the fallback), or if no valid token/target exists, suppress the generic /shop/orders/{id} fallback entirely; change references in CartPageClient where statusToken, statusTokenQuery, createdOrderId, setCreatedOrderId and paymentRecoveryUrl are used so the persisted recovery data includes both id and token or the fallback is hidden.
🧹 Nitpick comments (1)
frontend/tests/e2e/shop-checkout-local.spec.ts (1)
262-268:insertOrderallows payment statuses the schema cannot store.This helper currently permits
'refunded'and'needs_review', butfrontend/db/schema/shop.ts:28-31only definespayment_statusaspending | requires_payment | paid | failed. That makes the test helper’s API misleading and will turn any future use of those extra states into a runtime insert failure. Narrow this union to the real schema values or derive it from the shared payment-status type.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/tests/e2e/shop-checkout-local.spec.ts` around lines 262 - 268, The test helper insertOrder currently types its paymentStatus union to include 'refunded' and 'needs_review' which are not accepted by the DB schema; update the helper so its paymentStatus type matches the real schema (only 'pending' | 'requires_payment' | 'paid' | 'failed') or, better, import/derive the shared payment-status type used in frontend/db/schema/shop.ts (e.g., the payment_status type) and use that in insertOrder to prevent invalid inserts at compile time.
🤖 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/`[locale]/shop/checkout/payment/[orderId]/page.tsx:
- Around line 265-273: The current guard around initializing a Stripe
PaymentIntent prevents ensureStripePaymentIntentForOrder() from running when
order.paymentStatus === 'failed', leaving StripePaymentClient rendered without a
clientSecret; update the condition so failed orders can trigger initialization
(either by updating canInitializeStripeForPaymentStatus to treat 'failed' as
retryable or by extending the if-check to include order.paymentStatus ===
'failed') so that ensureStripePaymentIntentForOrder() runs and supplies a fresh
clientSecret before rendering StripePaymentClient; reference
shouldInitStripePaymentIntent, canInitializeStripeForPaymentStatus,
ensureStripePaymentIntentForOrder, StripePaymentClient, order.paymentStatus,
paymentsEnabled, publishableKey, and clientSecret when making the change.
---
Outside diff comments:
In `@frontend/app/api/shop/checkout/route.ts`:
- Around line 848-878: The current logic determines
stripeRequestedButUnavailable from raw request fields
(requestedProvider/requestedMethod) which misses the case when the provider
defaults to Stripe (selectedProvider) and thus can bypass the fail-closed path;
update the check so it uses the resolved provider: compute
stripeRequestedButUnavailable as (selectedProvider === 'stripe') &&
!stripeCheckoutAvailable (instead of using requestedProvider/requestedMethod),
ensuring checkoutPaymentProvider falls back to 'none' and triggers the intended
503/recovery behavior when Stripe is unavailable.
---
Duplicate comments:
In `@frontend/app/`[locale]/shop/cart/CartPageClient.tsx:
- Around line 1191-1202: The generic recovery fallback currently only persists
createdOrderId (via setCreatedOrderId) and builds statusTokenQuery locally but
may drop the guest auth token; update the flow to also persist the statusToken
when present (e.g., store alongside createdOrderId or set a paymentRecoveryToken
variable) and ensure paymentRecoveryUrl or the fallback link includes the
encoded statusToken (use statusTokenQuery when constructing the fallback), or if
no valid token/target exists, suppress the generic /shop/orders/{id} fallback
entirely; change references in CartPageClient where statusToken,
statusTokenQuery, createdOrderId, setCreatedOrderId and paymentRecoveryUrl are
used so the persisted recovery data includes both id and token or the fallback
is hidden.
---
Nitpick comments:
In `@frontend/tests/e2e/shop-checkout-local.spec.ts`:
- Around line 262-268: The test helper insertOrder currently types its
paymentStatus union to include 'refunded' and 'needs_review' which are not
accepted by the DB schema; update the helper so its paymentStatus type matches
the real schema (only 'pending' | 'requires_payment' | 'paid' | 'failed') or,
better, import/derive the shared payment-status type used in
frontend/db/schema/shop.ts (e.g., the payment_status type) and use that in
insertOrder to prevent invalid inserts at compile time.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 28586eb8-eff1-4502-b591-af1b85a34199
📒 Files selected for processing (5)
frontend/app/[locale]/shop/cart/CartPageClient.tsxfrontend/app/[locale]/shop/checkout/payment/[orderId]/page.tsxfrontend/app/api/shop/checkout/route.tsfrontend/lib/tests/shop/checkout-route-stripe-disabled-recovery.test.tsfrontend/tests/e2e/shop-checkout-local.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/lib/tests/shop/checkout-route-stripe-disabled-recovery.test.ts
… stabilize checkout tests
ViktorSvertoka
approved these changes
Mar 12, 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
Hardening pass for the Shop checkout/payment/shipping flow before merch rollout.
This PR closes the reliability gaps found during audit and brings the Shop module to a conditionally production-ready state at the application layer.
The work focused on fail-closed payment behavior, strict shipping/payment coupling, terminal payment shutdown handling, access control for payment pages, Monobank wallet reliability, deterministic validation coverage, wallet readiness hardening, and stable local Playwright smoke coverage.
Batch 6 (DB guardrails / migrations) is intentionally not included in this PR and remains a separate follow-up hardening step.
Related Issue
Issue: #<issue_number>
Changes
Hardened checkout to fail closed for explicit Stripe requests:
stripe -> nonefallbackEnforced shipping/payment coupling:
Closed shipping pipeline on terminal negative payment transitions:
Added missing shipping validation coverage:
Hardened payment page access:
orderIdorder_payment_initstatus_liteAligned checkout token scopes for payment-init flows:
Hardened Monobank webhook / wallet reliability:
Hardened Stripe capability/UX consistency:
Added local Playwright smoke coverage:
Performed read-only wallet readiness audit and prepared a strict pre-release manual smoke runbook:
Database Changes (if applicable)
How Has This Been Tested?
Additional local verification performed:
tests/e2e/shop-checkout-local.spec.ts)npm run buildAPP_ENV=localDATABASE_URL_LOCAL=devlovers_shop_local_cleanDATABASE_URL=' 'SHOP_STRICT_LOCAL_DB=1Screenshots (if applicable)
Manual smoke runbook and real wallet/device verification are separate follow-up steps.
Add screenshots/recordings from real-device wallet validation if available.
Checklist
Before submitting
Reviewers
Summary by CodeRabbit
New Features
Improvements
UI/UX Updates