(SP: 3)[Shop][Monobank] UAH-only checkout + webhook verify/apply + status token + admin refund/cancel (feature-gated, Stripe untouched)#302
Conversation
… in in-app routes and Stripe return_url
…ing, webhook + tests)
…, and signed status token
…elper + ops docs (Stripe unchanged)
…t dedupe + claim/lease TTL, paid terminal, mismatch→needs_review)
… payment-state + fix test cleanup
✅ Deploy Preview for develop-devlovers ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughImplements comprehensive Monobank payment provider integration as a parallel payment system to Stripe. Introduces PSP adapter layer, invoice/webhook/refund/cancellation services, database tables for Monobank events and refunds, admin API routes for cancel-payment, new payment status types, and extensive test coverage across the payment orchestration stack. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Checkout API
participant Order Service
participant Monobank PSP
participant Database
participant Webhook Route
Client->>Checkout API: POST /api/shop/checkout (monobank provider)
Checkout API->>Order Service: createOrderWithItems (paymentProvider: 'monobank')
Order Service->>Database: Insert order (currency: UAH, status: pending)
Order Service->>Database: Insert payment attempt (status: creating)
Checkout API->>Monobank PSP: createMonobankAttemptAndInvoice
Monobank PSP->>Database: Store invoice reference in attempt
Monobank PSP->>Monobank PSP: Build invoice payload (basket validation)
Monobank PSP->>Monobank PSP: Request invoice creation (timeout handling)
Monobank PSP-->>Checkout API: Return invoiceId, pageUrl
Checkout API-->>Client: 201 (invoiceId, pageUrl, checkoutUrl)
Note over Client,Monobank PSP: Customer completes payment
Monobank PSP->>Webhook Route: POST /api/shop/webhooks/monobank (signed payload)
Webhook Route->>Webhook Route: Verify signature (cached public key)
Webhook Route->>Webhook Route: Parse payload, compute SHA256
Webhook Route->>Database: Check deduplication (eventKey, rawSha256)
Webhook Route->>Order Service: applyMonoWebhookEvent
Order Service->>Database: Load order, payment attempt
Order Service->>Database: Validate amount/currency match
Order Service->>Database: Update order status (pending → paid)
Order Service->>Database: Update attempt status (creating → succeeded)
Order Service->>Database: Record monobankEvent (appliedResult: applied)
Webhook Route-->>Monobank PSP: 200 OK (deduped or applied)
sequenceDiagram
participant Admin
participant Refund API
participant Refund Service
participant Monobank PSP
participant Database
Admin->>Refund API: POST /api/shop/admin/orders/[id]/refund (admin auth + CSRF)
Refund API->>Database: Check payment provider (monobank?)
Refund API->>Refund API: Enforce refund-enabled flag (MONO_REFUND_ENABLED)
Refund API->>Refund Service: requestMonobankFullRefund
Refund Service->>Database: Load order, validate UAH/paid status
Refund Service->>Database: Check for existing refund (deduplication)
Refund Service->>Database: Locate Monobank invoice ID from attempts
Refund Service->>Database: Insert refund record (status: requested)
Refund Service->>Monobank PSP: cancelInvoicePayment
Monobank PSP->>Monobank PSP: Build cancel request (timeout handling)
Monobank PSP-->>Refund Service: Cancel result (success/failure)
Refund Service->>Database: Update refund status (requested → success/failure)
Refund Service->>Database: Mark order for restock if applicable
Refund API-->>Admin: 200 (refund details, deduped flag)
sequenceDiagram
participant Admin
participant Cancel-Payment API
participant Cancel Service
participant Monobank PSP
participant Database
Admin->>Cancel-Payment API: POST /api/shop/admin/orders/[id]/cancel-payment (admin auth + CSRF)
Cancel-Payment API->>Cancel Service: cancelMonobankUnpaidPayment
Cancel Service->>Database: Load order, validate monobank provider
Cancel Service->>Database: Check order paid status (must be unpaid)
Cancel Service->>Database: Insert/claim cancel record (leader election via INSTANCE_ID)
alt Leader
Cancel Service->>Database: Locate Monobank invoice ID
Cancel Service->>Monobank PSP: removeInvoice
Monobank PSP-->>Cancel Service: Removal result
Cancel Service->>Database: Update cancel status (processing → success/failure)
Cancel Service->>Database: Update order status (PENDING → CANCELED)
Cancel Service->>Database: Trigger inventory restock
else Follower (Deduped)
Cancel Service->>Database: Wait for leader's result (polling)
Cancel Service-->>Cancel-Payment API: Return existing cancel result
end
Cancel-Payment API-->>Admin: 200 (order details, cancel status, deduped flag)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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: add4b83cfe
ℹ️ 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".
| const paymentsEnabled = | ||
| (process.env.PAYMENTS_ENABLED ?? '').trim() === 'true'; | ||
| const stripePaymentsEnabled = | ||
| (process.env.STRIPE_PAYMENTS_ENABLED ?? '').trim() === 'true'; |
There was a problem hiding this comment.
When STRIPE_PAYMENTS_ENABLED is unset/false but PAYMENTS_ENABLED plus Stripe secrets are valid, createOrderWithItems still creates paymentProvider='stripe' orders while this route disables stripePaymentFlow, so checkout returns success with no clientSecret and the order remains pending with reserved stock. This config drift is introduced here because provider selection and payment-init now read different flags; use the same enablement source (or a compatibility fallback) for both paths.
Useful? React with 👍 / 👎.
| return { appliedResult: 'applied', restockReason, restockOrderId }; | ||
| } | ||
|
|
||
| return { appliedResult, restockReason, restockOrderId }; |
There was a problem hiding this comment.
If Monobank sends any status outside the handled set (success, processing, created, failure, expired, reversed), execution reaches this return without writing appliedAt/appliedResult on monobank_events. The row remains effectively un-applied after claim expiry and can be reclaimed/reprocessed on retries even though the function reports an applied result, which breaks dedupe/observability for unexpected provider statuses.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
frontend/lib/tests/shop/admin-api-killswitch.test.ts (1)
157-172:⚠️ Potential issue | 🟡 MinorUpdate context to use async params matching Next.js 15+ handler signatures.
The test passes
{ params: { id: c.id } }synchronously (line 168), but all route handlers are typed withparams: Promise<{ id: string }>and explicitly await the params (e.g.,const rawParams = await context.params;). While awaiting a non-Promise value technically returns the value, the test does not properly simulate the async params behavior expected in Next.js 15+ route handlers.Change line 168 to:
const ctx = { params: Promise.resolve({ id: c.id }) };This ensures the test mirrors production behavior where params is an async Promise.
frontend/app/api/shop/checkout/route.ts (1)
774-893: 🛠️ Refactor suggestion | 🟠 MajorNear-identical Monobank checkout block duplicated for existing vs. new orders.
Lines 774–825 (
!result.isNew) and 842–893 (new order) share ~50 lines of identical logic: lazy import → status token →createMonobankAttemptAndInvoice→ amount-mismatch check →buildMonobankCheckoutResponse. The only difference is the HTTP status (200 vs 201).Extract a helper to eliminate the duplication:
Sketch of extracted helper
+async function handleMonobankPaymentFlow(args: { + order: CheckoutOrderShape; + itemCount: number; + totalCents: number; + requestId: string; + orderMeta: Record<string, unknown>; + httpStatus: 200 | 201; +}) { + logInfo('monobank_lazy_import_invoked', { + requestId: args.requestId, + orderId: args.order.id, + }); + + const { createMonobankAttemptAndInvoice } = + await import('@/lib/services/orders/monobank'); + const statusToken = createStatusToken({ orderId: args.order.id }); + + const monobankAttempt = await createMonobankAttemptAndInvoice({ + orderId: args.order.id, + statusToken, + requestId: args.requestId, + }); + + if (args.totalCents !== monobankAttempt.totalAmountMinor) { + logError('checkout_mono_amount_mismatch', new Error('Monobank amount mismatch'), { + ...args.orderMeta, + code: 'MONO_AMOUNT_MISMATCH', + totalCents: args.totalCents, + totalAmountMinor: monobankAttempt.totalAmountMinor, + }); + return errorResponse('CHECKOUT_FAILED', 'Unable to process checkout.', 500); + } + + return buildMonobankCheckoutResponse({ + order: args.order, + itemCount: args.itemCount, + status: args.httpStatus, + attemptId: monobankAttempt.attemptId, + pageUrl: monobankAttempt.pageUrl, + currency: monobankAttempt.currency, + totalAmountMinor: monobankAttempt.totalAmountMinor, + }); +}
🤖 Fix all issues with AI agents
In `@frontend/docs/monobank-config.md`:
- Around line 47-48: Remove the stray trailing "T" at the end of the sentence
"Canonical names are `MONO_*`." in monobank-config.md; edit that line to read
cleanly (e.g., "Canonical names are `MONO_*`.") so the stray character is
deleted and the sentence is not truncated.
In `@frontend/drizzle/manual/0006_monobank_uah_only.sql`:
- Around line 44-50: The CHECK constraint payment_attempts_mono_currency_uah on
table payment_attempts allows NULL currency for monobank rows because NULL makes
the boolean expression pass; update the constraint to require a non-NULL UAH for
monobank rows by replacing the condition with: provider <> 'monobank' OR
(currency IS NOT NULL AND currency = 'UAH'), so any row with provider='monobank'
must have currency set and equal to 'UAH'.
- Around line 1-57: This manual migration (0006_monobank_uah_only.sql)
duplicates schema changes already managed by the auto migrations (e.g., adding
columns currency and expected_amount_minor, constraints
payment_attempts_provider_check, payment_attempts_status_check,
payment_attempts_expected_amount_minor_non_negative,
payment_attempts_mono_currency_uah, orders_payment_provider_valid and the unique
index payment_attempts_order_provider_active_unique) but will not be run by
drizzle-kit migrate; either delete this file to avoid conflicting manual
execution, or clearly mark it as documentation-only (move to a docs/ folder and
add a README) or document and implement a dedicated custom runner for it (and
remove overlapping changes from the auto migrations), so pick one approach and
remove the redundancy/conflict.
In `@frontend/lib/services/orders/monobank-refund.ts`:
- Around line 248-268: The refund retry path updates the DB row to 'requested'
(monobankRefunds update returning reconciled -> retried) before checking
orderRow.paymentStatus, which can throw invalid('REFUND_ORDER_NOT_PAID') and
leave an orphaned 'requested' record; move the payment status guard to run
before performing the update (i.e., check orderRow.paymentStatus !== 'paid'
immediately after order validations or inside the existing-refund branch before
the update to reconciled) so the update to monobankRefunds only happens when the
order is refundable and avoid writing 'requested' for non-payable orders.
In `@frontend/lib/services/orders/monobank-webhook.ts`:
- Around line 759-771: The restockOrder call (restockOrder) runs outside any
transaction and its failure is only logged via logError, leaving the event's
appliedResult as 'applied' so retries won't detect the problem; modify the catch
to also persist a marker on the payment/event record (e.g., set appliedResult =
'applied_with_issue' or add appliedErrorCode) for the event tied to
parsed.normalized.invoiceId and args.requestId so failures are surfaced and
retry/repair logic can act; ensure the update to the event uses the same event
update function you already have in this module (or create one if missing) and
include the error message/stack from the caught error when writing
appliedErrorCode/detail, keeping the logError call as well.
- Around line 327-329: The code assigns const tx = db but then performs multiple
independent updates (paymentAttempts, orders, monobankEvents) which is
misleading and can leave partial state; replace this by calling
db.transaction(...) and run all the multi-table mutations inside the transaction
callback (using the returned tx handle for queries/updates) so they commit
atomically and rollback on failure; if you intentionally do not make it
transactional, at minimum rename tx to db to avoid implying transactional
semantics — target the block that constructs MonobankApplyOutcome and the places
that reference tx/db for updates to paymentAttempts, orders and monobankEvents.
In `@frontend/lib/services/orders/monobank.ts`:
- Around line 424-437: The code unconditionally cancels the order when an old
"creating" attempt lacks pageUrl; instead only mark the attempt failed and allow
new attempts: stop calling deps.cancelOrderAndRelease(...) and avoid throwing a
terminal PspUnavailableError that cancels the order; keep the
deps.markAttemptFailed(...) for existing.id and return or throw a non-terminal
error that permits retry/new attempt creation (or simply return so higher-level
code can create a new attempt), referencing the existing symbols
markAttemptFailed, existing.id, cancelOrderAndRelease, args.orderId, and
PspUnavailableError and the stale window constant CREATING_STALE_MS to locate
the logic to change.
In `@frontend/lib/shop/url.ts`:
- Around line 38-46: The error message thrown when converting a caller-supplied
absolute URL is misleading; update the check inside the absolute-URL branch
(where toUrl(trimmed, 'absolute URL') is called and getRuntimeEnv().NODE_ENV is
checked) so the thrown error clearly refers to the provided absolute URL rather
than "Shop base URL" — e.g., mention "provided URL must use https in production"
or include the input value (trimmed) in the message. Keep the same validation
logic using toUrl and getRuntimeEnv(), only change the error text to reference
the caller-supplied URL.
In `@frontend/lib/tests/shop/checkout-monobank-idempotency-contract.test.ts`:
- Around line 105-148: The helper createIsolatedProduct currently fails if no
active product template exists (it selects from products where isActive and
throws 'No template product found to clone.'); update createIsolatedProduct to
detect missing tpl and create/insert a minimal template product record (using
products and productPrices) before cloning so tests run on clean DBs, or
alternatively add explicit test setup that seeds a known active template
product; ensure you use the same fields the function expects
(id/slug/sku/title/stock/isActive/createdAt/updatedAt) and reuse toDbMoney when
inserting productPrices so downstream inserts succeed.
In `@frontend/lib/tests/shop/monobank-merchant-paym-info.test.ts`:
- Line 50: Replace the double-encoded "Оплата Ð·Ð°Ð¼Ð¾Ð²Ð»ÐµÐ½Ð½Ñ 123"
values used for the destination property in the error-path tests with the
correctly encoded Unicode string "Оплата замовлення 123" (the same value used in
the valid test). Locate the destination occurrences in the monobank merchant
paym info tests (search for the key name destination and the mojibake text) and
update them; also ensure the test file is saved with UTF-8 encoding to prevent
future double-encoding issues.
In `@frontend/lib/tests/shop/monobank-refund-disabled.test.ts`:
- Around line 11-20: Update the mocked error classes so their `code` values
match the real implementations: change AdminUnauthorizedError.code from
'ADMIN_UNAUTHORIZED' to 'UNAUTHORIZED' and AdminForbiddenError.code from
'ADMIN_FORBIDDEN' to 'FORBIDDEN' in the vi.mock block that defines
requireAdminApi and those error classes (leave AdminApiDisabledError as-is);
this ensures tests that check error.code (e.g., paths exercising requireAdminApi
error handling) behave the same as the real classes.
🧹 Nitpick comments (62)
frontend/lib/tests/shop/payment-state-legacy-writers.test.ts (1)
103-111: Template literal${…}interpolations are not handled by the brace matcher.When
inTemplateis true, the parser only looks for\\and the closing backtick. Braces inside${expr}interpolations (e.g.,`prefix_${fn({ a: 1 })}`) will leak into the depth counter, potentially returning a wrong match position. This could causehasDirectPaymentStatusWriterto miss a real offender or produce a false positive.For the current use-case (scanning for
.set({ paymentStatus })in service files) the risk is low, but worth noting. If you want to harden it:Sketch: track interpolation nesting
if (inTemplate) { if (ch === '\\') { i++; continue; } - if (ch === '`') inTemplate = false; - + if (ch === '`') { + inTemplate = false; + continue; + } + if (ch === '$' && next === '{') { + // Entering interpolation – push state or just let the + // brace-depth tracker handle it by falling through. + i++; // skip '{' + depth++; + inTemplate = false; // exit template mode; the closing '}' will + // decrement depth and we re-enter template mode + // NOTE: full correctness requires a state stack; this is a + // pragmatic simplification. + continue; + } continue; }A fully correct parser would use a stack of lexer states, but that's likely overkill here.
frontend/lib/services/orders/payment-state.ts (2)
120-147: Consider a set-based check as the blocklist grows.The condition on lines 122–125 now checks four statuses via chained
===comparisons. ASet-based lookup would be a bit more maintainable if more statuses are added later.♻️ Optional: set-based blocklist
+const DISALLOWED_FOR_NONE = new Set<PaymentStatus>([ + 'pending', + 'requires_payment', + 'refunded', + 'needs_review', +]); + export async function guardedPaymentStatusUpdate( args: GuardedPaymentUpdateArgs ): Promise<GuardedPaymentUpdateResult> { const { orderId, paymentProvider, to, source, eventId, note } = args; if ( paymentProvider === 'none' && - (to === 'pending' || - to === 'requires_payment' || - to === 'refunded' || - to === 'needs_review') + DISALLOWED_FOR_NONE.has(to) ) {
42-47:allowedFromroutes all non-'none'providers toALLOWED_FROM_STRIPE.Both Stripe and Monobank currently use identical state transition rules. If Monobank ever requires different semantics, the dispatch logic here will need per-provider handling.
frontend/.env.example (1)
76-81: Add a brief comment forSHOP_STATUS_TOKEN_SECRET.Other variables in the file have descriptive comments.
SHOP_STATUS_TOKEN_SECRETis a secret used for signing status tokens (to prevent IDOR per the PR objectives), but there's no inline guidance on its purpose or how to generate a suitable value.Suggested improvement
# --- Shop # Optional canonical base URL override for shop links/webhooks. # Used to build Monobank redirect/webhook URLs. Fallback: APP_ORIGIN, then NEXT_PUBLIC_SITE_URL. # In production, must be https. SHOP_BASE_URL= +# Secret used to sign ownership status tokens (prevents IDOR on order status). +# Generate with: openssl rand -base64 32 SHOP_STATUS_TOKEN_SECRET=frontend/docs/monobank-config.md (1)
108-110: Note:applymode doesn't persist events before applying.Line 110 correctly documents that
applymode does not store events. This means if the apply step fails mid-way (e.g., DB error after signature verification passes), the event is lost with no record for retry or audit. Consider a follow-up to persist events inapplymode as well (persist-first, then apply), which would align with the "persist-first dedupe" pattern described in the PR objectives.frontend/lib/tests/shop/shop-url.test.ts (1)
1-76: Well-structured tests with good coverage of the priority chain and production HTTPS enforcement.One minor gap: there's no test for the error path when none of the three URL env vars are set (
resolveShopBaseUrlthrows in that case per the implementation). Consider adding a negative test for completeness.frontend/lib/services/orders/attempt-idempotency.ts (1)
1-7: Theproviderparameter is redundant — it only accepts the literal'stripe'.Since the type is constrained to
'stripe', the parameter can never vary and always produces the samepi:stripe:prefix. This adds call-site ceremony (callers must pass'stripe') without flexibility. Consider dropping the parameter and hardcoding the prefix, matching the Monobank function's simpler signature.Suggested simplification
-export function buildStripeAttemptIdempotencyKey( - provider: 'stripe', - orderId: string, - attemptNo: number -): string { - return `pi:${provider}:${orderId}:${attemptNo}`; +export function buildStripeAttemptIdempotencyKey( + orderId: string, + attemptNo: number +): string { + return `pi:stripe:${orderId}:${attemptNo}`; }frontend/lib/tests/shop/checkout-currency-policy.test.ts (1)
6-6: Minor formatting: missing space after comma.-import { orderItems,orders, productPrices, products } from '@/db/schema'; +import { orderItems, orders, productPrices, products } from '@/db/schema';frontend/lib/tests/shop/monobank-payments-disabled.test.ts (2)
40-42: Dead code:DATABASE_URLguard is a no-op.The condition
!process.env.DATABASE_URL && __prevDatabaseUrlcan never be true —__prevDatabaseUrlwas captured fromprocess.env.DATABASE_URL, so if__prevDatabaseUrlis truthy,process.env.DATABASE_URLwas already set (and hasn't been cleared). This block never executes.
70-110: Template spread may carry stale/unintended fields.
...(tpl as any)copies all columns from the template product (including any future columns). If the schema adds columns with constraints (e.g., unique indexes), this could cause unexpected failures. Consider explicitly picking only the fields you need from the template, or at minimum document the intent.frontend/db/schema/shop.ts (1)
541-542:bigintwithmode: 'number'is safe here but inconsistent withintegerelsewhere.
expectedAmountMinorusesbigintwhiletotalAmountMinoronordersandunitPriceMinor/lineTotalMinoronorder_itemsuseinteger. Similarly,monobankRefunds.amountMinorusesbigint. This type inconsistency could cause subtle comparison issues in application code. Consider documenting the rationale or aligning the types.frontend/app/[locale]/shop/orders/[id]/page.tsx (1)
32-41: Consistent type derivation from DB schema.Both
OrderPaymentStatusandOrderPaymentProviderare now inferred from the schema, keeping the page types aligned with the DB. Note that the API route (frontend/app/api/shop/orders/[id]/route.ts) still usespaymentProvider: stringin itsOrderDetailResponse— consider aligning it withOrderPaymentProviderfor consistency.frontend/lib/services/orders/checkout.ts (1)
375-387: DuplicatedrequestedProvider === 'monobank'check.
isMonobankRequestedis computed on line 375, but line 381 re-checksrequestedProvider === 'monobank'instead of reusing it. Minor readability nit.♻️ Reuse the flag
const paymentProvider: PaymentProvider = - requestedProvider === 'monobank' + isMonobankRequested ? 'monobank' : stripePaymentsEnabled ? 'stripe' : 'none';frontend/lib/services/orders/monobank-cancel-payment.ts (1)
250-301: IfrestockOrderfails, cancel record stays in'processing'indefinitely.In
finalizeProcessingCancel, ifrestockOrderthrows (line 263→272), the cancel record remains in'processing'state. Any subsequent request will re-enterfinalizeProcessingCanceland retry the restock, which is fine ifrestockOrderis idempotent. However, there's no TTL or max-retry guard — a permanently failing restock will cause every admin retry to hit the same error path without ever transitioning to'failure'.Consider adding a fallback that marks the cancel as
'failure'if restocking repeatedly fails, or add a timeout/attempt-count guard on the'processing'state.frontend/lib/tests/shop/monobank-webhook-apply.test.ts (1)
30-61: Test helperinsertOrderAndAttemptusesas anycasts for DB inserts.This is common in test code to avoid specifying every defaulted column, but it means the test won't catch schema drift (e.g., a new required column without a default). Acceptable for now.
frontend/lib/shop/status-token.ts (1)
13-20: Consider enforcing a minimum length forSHOP_STATUS_TOKEN_SECRET.
getSecret()only checks for a non-empty string. A very short secret (e.g.,"x") would pass but provides negligible HMAC security. Consider adding a minimum-length check (e.g., ≥ 32 characters) to fail early on misconfiguration.🛡️ Proposed guard
function getSecret(): string { const raw = process.env.SHOP_STATUS_TOKEN_SECRET ?? ''; const trimmed = raw.trim(); if (!trimmed) { throw new Error('SHOP_STATUS_TOKEN_SECRET is not configured'); } + if (trimmed.length < 32) { + throw new Error('SHOP_STATUS_TOKEN_SECRET is too short (min 32 chars)'); + } return trimmed; }frontend/app/api/shop/orders/[id]/status/route.ts (1)
49-49:statusTokenin query params will appear in server/proxy access logs.The token is read from
request.nextUrl.searchParams, meaning it's part of the URL. Short-lived tokens (45 min TTL) mitigate the risk, but if access logs are retained long-term or proxied through third-party CDNs, consider noting this in operational docs. AnAuthorizationheader alternative would avoid URL logging but is harder for client-side redirect flows.frontend/app/api/shop/webhooks/monobank/route.ts (3)
22-26:noStoreJsonhelper is duplicated across route files.This same helper exists in
frontend/app/api/shop/orders/[id]/status/route.ts(lines 22-26). Consider extracting to a shared utility (e.g.,@/lib/api/response). Low priority since each route file is self-contained.
57-70: Webhook mode resolution: initial parse on line 57 is a dead assignment whenMONO_WEBHOOK_MODEis unset.When
process.env.MONO_WEBHOOK_MODEisundefined, line 57-59 parses it to'apply', but lines 60-70 immediately override it. This is functionally correct but the redundant initial parse is slightly confusing. A clearer structure would check the env var first.♻️ Clearer mode resolution
- let webhookMode: WebhookMode = parseWebhookMode( - process.env.MONO_WEBHOOK_MODE - ); - if (!process.env.MONO_WEBHOOK_MODE) { + let webhookMode: WebhookMode; + if (process.env.MONO_WEBHOOK_MODE) { + webhookMode = parseWebhookMode(process.env.MONO_WEBHOOK_MODE); + } else { try { webhookMode = parseWebhookMode(getMonobankConfig().webhookMode); } catch (error) { logError('monobank_webhook_mode_invalid', error, { ...baseMeta, code: 'MONO_WEBHOOK_MODE_INVALID', }); webhookMode = 'apply'; } }
104-111: Invalid signature silently returns 200 — consider monitoring for signature failure spikes.Returning 200 on invalid signatures is correct to prevent PSP retries, and the
logWarnprovides observability. Ensure an alert is configured onMONO_SIG_INVALIDevents, as a spike could indicate key rotation issues or a spoofed webhook attack.frontend/drizzle/0007_outstanding_shocker.sql (1)
1-4: Partial unique index recreation is correct but duplicates the manual migration.This drizzle-generated migration performs the same
DROP/CREATEofpayment_attempts_order_provider_active_uniqueasmanual/0006_monobank_uah_only.sql(lines 52–57). Both useIF EXISTS/IF NOT EXISTSso double execution is safe, but the duplication may cause confusion during future maintenance.frontend/lib/tests/shop/monobank-adapter.test.ts (2)
3-4: Merge duplicate imports from the same module.Proposed fix
-import { buildMonobankInvoicePayload } from '@/lib/psp/monobank'; -import { MONO_CCY } from '@/lib/psp/monobank'; +import { buildMonobankInvoicePayload, MONO_CCY } from '@/lib/psp/monobank';
7-32: Consider asserting more payload fields in the success case.The test only verifies
ccyandpaymentType, butbuildMonobankInvoicePayloadalso setsamount,redirectUrl,webHookUrl, andmerchantPaymInfo. Asserting at leastamountand the URLs would catch regressions in field mapping (e.g., the source useswebhookUrlbut the output key iswebHookUrl).frontend/lib/services/orders/summary.ts (1)
162-166:OrderAttemptSummarytype is not exported.The function
getOrderAttemptSummaryis exported but its return typeOrderAttemptSummaryis not. Consumers that need to reference this type explicitly (e.g., for variable annotations) won't be able to import it. If this is intentional (relying on type inference), consider adding a brief comment; otherwise, addexport.Suggested fix
-type OrderAttemptSummary = { +export type OrderAttemptSummary = {frontend/lib/env/payments.ts (1)
5-9: Monobank silently takes precedence when both providers are enabled.If both
isMonobankEnabled()andisStripeEnabled()return true, only'monobank'is returned. This is likely intentional for the feature-gate rollout, but worth a brief inline comment to prevent future confusion when both might be enabled simultaneously.frontend/lib/services/orders/monobank/merchant-paym-info.ts (1)
11-28:safeLineTotalMinorsilently returns 0 for invalid inputs — verify downstream invariant.Returning 0 instead of throwing means a corrupt item silently contributes nothing to the basket total. This works only if
buildMonoMerchantPaymInfoFromSnapshotalways validates the basket sum againstexpectedAmountMinor(triggeringMONO_BASKET_SUM_MISMATCH). If that downstream check were ever relaxed, silent zeroes could slip through.This is fine as-is given current guarantees, but consider adding a brief comment documenting the reliance on the downstream sum check.
frontend/lib/tests/shop/monobank-merchant-paym-info.test.ts (1)
9-18: Consider using Vitest'sexpect().toThrow()for cleaner error assertions.The
expectCodehelper works but iffn()unexpectedly doesn't throw, thethrow new Error('expected error')would fail thetoBeInstanceOf(MonoMerchantPaymInfoError)check with a confusing message. Vitest's built-inexpect(fn).toThrow()combined with a custom matcher or try/catch withexpect.fail()would make failures clearer.frontend/lib/tests/shop/monobank-webhook-crypto.test.ts (1)
33-39:makeResponsemissing requiredResponseproperties could break if implementation changes.The mock returns only
{ok, status, text}. If thefetchWebhookPubKeyimplementation ever accesses.json(),.headers, or otherResponseproperties, these tests would fail with an unhelpful error. Theas anycast on line 123/157 hides this.This is acceptable for now but worth noting — consider using
new Response(body, { status: 200 })for a more robust mock.frontend/lib/tests/shop/checkout-monobank-parse-validation.test.ts (1)
94-135: Mock order'spaymentProvider: 'none'sidesteps the Monobank invoice flow — intentional but worth a comment.The mock returns
paymentProvider: 'none'(line 103), which causes the route to skip Monobank invoice creation entirely. This makes the test purely a parse/validation check (as the name suggests), but a reader might expect the Monobank flow to be exercised. A brief inline comment clarifying that the mock intentionally avoids the PSP call would improve clarity.frontend/lib/tests/shop/monobank-env.test.ts (1)
67-67: Nit: extra leading space on Line 67.This
itblock has an extra space of indentation compared to the other test cases (Lines 43, 53, 61).- it('returns token when MONO_MERCHANT_TOKEN is set', () => { + it('returns token when MONO_MERCHANT_TOKEN is set', () => {frontend/lib/tests/shop/monobank-http-client.test.ts (2)
60-85: Fake timers restoration is not failure-safe.If the test throws before Line 84,
vi.useRealTimers()is never called, potentially leaving fake timers active for subsequent tests. Wrapping intry/finallywould be more robust.♻️ Proposed fix
it('maps timeout to PSP_TIMEOUT', async () => { vi.useFakeTimers(); - const fetchMock = vi.fn(() => new Promise<Response>(() => {})); - globalThis.fetch = fetchMock as any; - - const { fetchWebhookPubKey, PspError } = await import('@/lib/psp/monobank'); - - const p = fetchWebhookPubKey().then( - () => null, - e => e - ); - - await vi.advanceTimersByTimeAsync(25); - const error = await p; - - expect(error).toBeInstanceOf(PspError); - const err = error as InstanceType<typeof PspError>; - expect(err.code).toBe('PSP_TIMEOUT'); - expect(err.safeMeta).toMatchObject({ - endpoint: '/api/merchant/pubkey', - method: 'GET', - timeoutMs: 25, - }); - - vi.useRealTimers(); + try { + const fetchMock = vi.fn(() => new Promise<Response>(() => {})); + globalThis.fetch = fetchMock as any; + + const { fetchWebhookPubKey, PspError } = await import('@/lib/psp/monobank'); + + const p = fetchWebhookPubKey().then( + () => null, + e => e + ); + + await vi.advanceTimersByTimeAsync(25); + const error = await p; + + expect(error).toBeInstanceOf(PspError); + const err = error as InstanceType<typeof PspError>; + expect(err.code).toBe('PSP_TIMEOUT'); + expect(err.safeMeta).toMatchObject({ + endpoint: '/api/merchant/pubkey', + method: 'GET', + timeoutMs: 25, + }); + } finally { + vi.useRealTimers(); + } });
5-37: Env helpers andmakeResponseare duplicated across Monobank test files.
rememberEnv,restoreEnv,makeResponse,ENV_KEYS, andoriginalFetchare nearly identical inmonobank-http-client.test.tsandmonobank-api-methods.test.ts. Consider extracting them to a shared test helper (e.g.,@/lib/tests/helpers/monobank-test-env.ts) to reduce maintenance burden.frontend/docs/payments/monobank/F0-report.md (1)
23-23: Nested backticks in Markdown render incorrectly.The template literal
`checkout:${checkoutSubject}`on Line 23 will not render properly because of the nested backticks. Use double backticks or a code block instead:-- Checkout key format: ``checkout:${checkoutSubject}`` in `frontend/app/api/shop/checkout/route.ts` +- Checkout key format: `` `checkout:${checkoutSubject}` `` in `frontend/app/api/shop/checkout/route.ts`frontend/lib/tests/shop/monobank-webhook-mode.test.ts (1)
104-141: Test description "drops events without applying or storing" is misleading — event is persisted.The test name says "without applying or storing," but lines 129–138 assert that a
monobankEventsrow is written withappliedResult = 'dropped'. This aligns with the PR's "persist-first" design, so the test logic is correct — it's just the description that is inaccurate. Consider rewording to something like"drop mode: persists event as dropped, no order/attempt state changes".frontend/lib/tests/shop/monobank-attempt-invoice.test.ts (1)
188-215: Consider asserting cleanup behavior on persist failure.The PSP timeout test (line 160) asserts that
markAttemptFailedandcancelOrderAndReleaseare called on PSP failure. This test only asserts the error type propagates but doesn't verify whether compensation/cleanup deps are invoked whenfinalizeAttemptWithInvoicefails after a successful PSP call. If the implementation does (or should) clean up in this case, add assertions here; if not, a comment explaining why would help future readers.frontend/lib/tests/shop/monobank-psp-unavailable.test.ts (3)
50-52: Dead code:DATABASE_URLguard is a no-op.
__prevDatabaseUrlis captured fromprocess.env.DATABASE_URLon line 40. AtbeforeAlltime, ifDATABASE_URLis set, then!process.env.DATABASE_URLis false and the branch is skipped. If it's not set,__prevDatabaseUrlisundefined, so&& __prevDatabaseUrlis false. Either way, this block never executes.Proposed fix — remove the dead guard
- if (!process.env.DATABASE_URL && __prevDatabaseUrl) { - process.env.DATABASE_URL = __prevDatabaseUrl; - }
7-7: Nit: missing whitespace in import.-import { orders, paymentAttempts,productPrices, products } from '@/db/schema'; +import { orders, paymentAttempts, productPrices, products } from '@/db/schema';
197-201: Fragile type assertion on raw SQL result.The cast
as unknown as { rows?: Array<{ type: unknown }> }assumes a specific driver response shape. If the Drizzle/pg driver changes its return format, this will silently produce empty results rather than failing. Consider using a typed query through the schema ifinventory_movesis available, or at minimum add an assertion thatmoves.rowsis defined.frontend/lib/tests/shop/monobank-refund-route-f4.test.ts (1)
77-93: Minor:inventoryStatus: 'released'for a paid order is semantically unexpected.Paid orders typically have
inventoryStatus: 'reserved'. While the refund route may not validate this, using a more realistic value ('reserved') would make the test setup less confusing to future readers.frontend/app/api/shop/admin/orders/[id]/refund/route.ts (2)
93-141: Duplicate condition check:targetOrder?.paymentProvider === 'monobank'evaluated twice.Lines 99 and 116 both check the same condition. These can be merged into a single
ifblock for clarity.♻️ Combine into a single Monobank block
if (targetOrder?.paymentProvider === 'monobank') { const { refundEnabled } = getMonobankConfig(); if (!refundEnabled) { logWarn('admin_orders_refund_disabled', { ...baseMeta, code: 'REFUND_DISABLED', orderId: orderIdForLog, durationMs: Date.now() - startedAtMs, }); return noStoreJson( { code: 'REFUND_DISABLED', message: 'Refunds are disabled.' }, { status: 409 } ); } - } - if (targetOrder?.paymentProvider === 'monobank') { const { requestMonobankFullRefund } = await import( '@/lib/services/orders/monobank-refund' ); const result = await requestMonobankFullRefund({ orderId: orderIdForLog, requestId, }); const orderSummary = orderSummarySchema.parse(result.order); return noStoreJson({ success: true, order: { ...orderSummary, createdAt: orderSummary.createdAt instanceof Date ? orderSummary.createdAt.toISOString() : String(orderSummary.createdAt), }, refund: { ...result.refund, deduped: result.deduped, }, }); }
93-97: No guard whentargetOrderis null (order not found in DB).If the order ID doesn't exist,
targetOrderisundefinedand the code falls through to the genericrefundOrdercall on line 143, which will presumably throwOrderNotFoundError. This works, but it means a pointless service call. Consider returning 404 early if!targetOrderto short-circuit and keep the error path explicit.Optional early return
const [targetOrder] = await db .select({ paymentProvider: orders.paymentProvider }) .from(orders) .where(eq(orders.id, orderIdForLog)) .limit(1); + + if (!targetOrder) { + return noStoreJson( + { code: 'ORDER_NOT_FOUND', message: 'Order not found.' }, + { status: 404 } + ); + }frontend/lib/services/orders/monobank-refund.ts (3)
278-342: Significant duplication of reconciliation/retry logic.Lines 296–342 (the
onConflictDoNothingconflict-recovery path) nearly mirror lines 226–261 (thegetExistingRefundpath). Both perform: fetch existing →reconcileSuccessFromOrder→ check deduped → check retryable → update to'requested'.Extract a shared helper (e.g.
reconcileOrRetryExisting(refund, orderId)) to remove the duplicated ~45 lines and reduce the surface area for divergence.
139-167:reconcileSuccessFromOrderlacks an optimistic-concurrency guard on the UPDATE.The update at line 157 unconditionally sets
status = 'success'— it doesn't constrain on the current status (e.g.,AND status <> 'success'). If two concurrent calls reconcile the same refund, both will write to the same row. This is benign here (idempotent write), so flagging just for awareness.
352-380: PSP failure sets refund to'failure'but does not propagate the original error details.When
cancelInvoicePaymentthrows, the error's message/details are not persisted to the refund row (only the status flips to'failure'). Consider storing the error code/message on the refund row (if the schema supports it) to aid debugging failed refunds without relying solely on log search.frontend/lib/tests/shop/monobank-cancel-payment-route-f5.test.ts (1)
92-113: Product currency'USD'vs order currency'UAH'.
insertProductWithReservedStockcreates a product withcurrency: 'USD'(line 101), whileinsertOrderalways usescurrency: 'UAH'(line 127). This doesn't affect the cancel-payment flow under test, but it could confuse future readers or cause issues if product-currency validation is added later. Consider using'UAH'for consistency.frontend/app/api/shop/admin/orders/[id]/cancel-payment/route.ts (1)
109-122:dedupedis exposed at two levels in the response.
result.cancelis spread at line 119, so if it already contains adedupedproperty, the response will havecancel.dedupedand the top-leveldedupedat line 121. This is harmless but redundant — consider removing the top-leveldedupedor omitting it from the spread to keep one canonical location.frontend/drizzle/0008_wide_zzzax.sql (1)
15-31:amount_minorinmonobank_refundsis nullable — considerNOT NULL.Line 22 defines
amount_minorasbigintwithout aNOT NULLconstraint. The refund service always supplies a validated positive integer (seemonobank-refund.tsline 288), so aNULLvalue would indicate a data integrity issue. AddingNOT NULLwould make the schema defensive against partial inserts from other code paths.frontend/app/api/shop/checkout/route.ts (3)
391-399:monobankRequestHintparses provider from body a second time.Lines 392–398 parse
paymentProvider/providerfrom the body to setmonobankRequestHint. Lines 457–478 perform the same parse again forrequestedProvider. Consider consolidating to a single parse pass, or moving the hint detection after the provider parse at line 463.
487-490: Directprocess.envreads for payment feature flags.
paymentsEnabledandstripePaymentsEnabledare parsed inline fromprocess.envrather than using the shared helpers (areShopPaymentsEnabled, etc.) introduced elsewhere in this PR. This duplicates env-parsing logic and risks divergence if the helpers evolve (e.g., adding caching or fallback behavior).
48-67: Provider parsing andisMonoAliasaccept'mono'but reject it as'invalid'— intentional but worth a comment.
parseRequestedProviderreturns'invalid'for'mono', andisMonoAliascatches it separately to return a 422. This two-step dance is correct but non-obvious. A brief inline comment explaining that'mono'is a known alias intentionally rejected to force the canonical'monobank'name would help future readers.frontend/lib/services/orders/monobank-webhook.ts (4)
471-471: Untypedto: anyparameter intransitionPaymentStatusThe
toparameter is typedany, losing type-safety on the payment status value being passed toguardedPaymentStatusUpdate. Use the actual payment status type (or at least a union of the valid values) to catch invalid transitions at compile time.Proposed fix
- const transitionPaymentStatus = async (to: any) => { + const transitionPaymentStatus = async ( + to: 'paid' | 'failed' | 'refunded' | 'needs_review' + ) => {
185-193: Raw SQL result shape depends on driver — fragile castThe
db.execute(sql...)result is cast to{ rows?: Array<{ id?: string }> }viaas unknown as. If the Drizzle driver changes its return shape (e.g., Neon Serverless returns rows directly, while node-postgres wraps them), this silently breaks. The same pattern appears inclaimMonobankEvent(Line 204) and the attempt/order lookups further down.Consider using Drizzle's query-builder API (
.select().from(monobankEvents).where(...)) for type-safe results, or at least add a runtime guard (e.g., assertArray.isArray(result.rows)) before accessing.rows.
39-40: UUID regex excludes v6/v7/v8 UUIDsThe regex only accepts UUID versions 1–5 (
[1-5]). UUIDv7 (timestamp-based, increasingly common) would fail this check, and a validreferencefield containing one would be silently treated as non-UUID. If Monobank or your own system ever emits v7 UUIDs, the reference-based attempt lookup path would be skipped.Broader UUID pattern
-const UUID_RE = - /^[0-9a-f]{8}-[0-9a-f]{4}-[1-5][0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/i; +const UUID_RE = + /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i;
519-519: Repeated'monobank' as anycastsMultiple
.where(eq(orders.paymentProvider, 'monobank' as any))casts indicate a type mismatch between the Drizzle column type and the string literal. This is a recurring code smell across four locations. Consider adding'monobank'to thePaymentProvidertype union (or using the schema's inferred type) so the cast is unnecessary.Also applies to: 590-590, 643-643, 724-724
frontend/lib/env/monobank.ts (3)
54-64:parseTimeoutMsandparsePositiveIntare identical implementations.These two functions have the exact same body. Consolidate into a single helper to avoid the duplication.
Proposed fix
-function parseTimeoutMs(raw: string | undefined, fallback: number): number { - const v = raw ? Number.parseInt(raw, 10) : NaN; - if (!Number.isFinite(v) || v <= 0) return fallback; - return v; -} - -function parsePositiveInt(raw: string | undefined, fallback: number): number { - const v = raw ? Number.parseInt(raw, 10) : NaN; - if (!Number.isFinite(v) || v <= 0) return fallback; - return v; -} +function parsePositiveInt(raw: string | undefined, fallback: number): number { + const v = raw ? Number.parseInt(raw, 10) : NaN; + if (!Number.isFinite(v) || v <= 0) return fallback; + return v; +}Then replace the
parseTimeoutMscall on Line 101 withparsePositiveInt.
125-127:isMonobankEnabled()semantics may confuse callers.
isMonobankEnabled()returnstrueif a token is present, regardless of whetherPAYMENTS_ENABLEDis'true'. Meanwhile,getMonobankEnv().paymentsEnabledrequires both. Callers checkingisMonobankEnabled()to guard payment operations could bypass the payments-enabled gate.Consider aligning semantics — either rename to
isMonobankConfigured()or also check the payments flag.
24-24: Redundant directprocess.envread bypasses Zod validation.
process.env.MONO_WEBHOOK_MODEis read before the Zod-validatedenv.MONO_WEBHOOK_MODE. SincegetServerEnv()already parses and defaults this variable (including the.default('apply')in the schema), readingprocess.envdirectly could yield an un-validated value. If the intent is to allow hot-reload without cache, consider usingresetEnvCache()instead.Proposed simplification
- const rawMode = process.env.MONO_WEBHOOK_MODE ?? env.MONO_WEBHOOK_MODE; + const rawMode = env.MONO_WEBHOOK_MODE;frontend/lib/services/orders/monobank.ts (2)
647-656: Extra DB round-trip forattemptNumberthat was already known.
createCreatingAttemptcomputesnext(the attempt number) but it's buried in the returned row. ThencreateMonobankAttemptAndInvoicedoes a separateSELECTto retrieve it. IfcreateMonoAttemptAndInvoicereturned the attempt number (it's in the inserted row), the extra query could be avoided.
228-372:finalizeAttemptWithInvoicealways throws after retry exhaustion — even on partial success.After the retry loop is exhausted (Line 323), the best-effort metadata save (Lines 330–352) may succeed (marking
finalizePending: trueon the attempt). But Line 369 unconditionally throwsPspInvoicePersistError. This is fine as fail-closed semantics — the webhook will reconcile — but the thrown error doesn't distinguish "partial save succeeded" from "total failure + invoice cancelled" (Lines 353–367). Callers may want to know the difference for logging/monitoring. Consider including a flag in the error metadata.frontend/lib/psp/monobank.ts (3)
569-571: Dead code:MONO_CURRENCYis a compile-time constant'UAH'.
MONO_CURRENCYis declared as'UAH' as conston Line 8, so the conditionMONO_CURRENCY !== 'UAH'is alwaysfalseand this branch can never execute.
233-287:buildMonobankInvoicePayloadandbuildMonobankInvoicePayloadFromInputshare most validation logic.Both functions validate
amountMinor, extract and validatemerchantPaymInfo, parsebasketOrder, and build the finalMonobankInvoiceCreateRequest. The main differences are the input type, thereferencecross-check, andvalidity. Consider extracting the shared validation into a common internal helper to reduce duplication.Also applies to: 496-558
16-32:PspError.causebypasses standard Error cause chain.Setting
this.causeas a direct property shadows the standardErrorcause mechanism (ES2022). Consider passingcausevia the options bag to integrate with native error introspection:Proposed fix
constructor( code: PspErrorCode, message: string, safeMeta?: Record<string, unknown>, cause?: unknown ) { - super(message); + super(message, { cause }); this.code = code; this.safeMeta = safeMeta; - this.cause = cause; }
| -- 6) Monobank-only invariant: currency must be UAH | ||
| ALTER TABLE payment_attempts | ||
| DROP CONSTRAINT IF EXISTS payment_attempts_mono_currency_uah; | ||
|
|
||
| ALTER TABLE payment_attempts | ||
| ADD CONSTRAINT payment_attempts_mono_currency_uah | ||
| CHECK (provider <> 'monobank' OR currency = 'UAH'); |
There was a problem hiding this comment.
Monobank rows with NULL currency pass this CHECK — constraint doesn't enforce UAH as intended.
In SQL, CHECK (provider <> 'monobank' OR currency = 'UAH') evaluates to FALSE OR NULL → NULL when provider = 'monobank' and currency IS NULL. A NULL result is treated as "not violated" by CHECK, so a Monobank payment attempt with no currency set would silently pass.
Add a NOT NULL requirement for Monobank rows:
Proposed fix
ALTER TABLE payment_attempts
ADD CONSTRAINT payment_attempts_mono_currency_uah
- CHECK (provider <> 'monobank' OR currency = 'UAH');
+ CHECK (provider <> 'monobank' OR currency = 'UAH');\n
+ALTER TABLE payment_attempts
+ DROP CONSTRAINT IF EXISTS payment_attempts_mono_currency_not_null;
+
+ALTER TABLE payment_attempts
+ ADD CONSTRAINT payment_attempts_mono_currency_not_null
+ CHECK (provider <> 'monobank' OR currency IS NOT NULL);Or combine into a single constraint:
CHECK (provider <> 'monobank' OR (currency IS NOT NULL AND currency = 'UAH'))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| -- 6) Monobank-only invariant: currency must be UAH | |
| ALTER TABLE payment_attempts | |
| DROP CONSTRAINT IF EXISTS payment_attempts_mono_currency_uah; | |
| ALTER TABLE payment_attempts | |
| ADD CONSTRAINT payment_attempts_mono_currency_uah | |
| CHECK (provider <> 'monobank' OR currency = 'UAH'); | |
| -- 6) Monobank-only invariant: currency must be UAH | |
| ALTER TABLE payment_attempts | |
| DROP CONSTRAINT IF EXISTS payment_attempts_mono_currency_uah; | |
| ALTER TABLE payment_attempts | |
| ADD CONSTRAINT payment_attempts_mono_currency_uah | |
| CHECK (provider <> 'monobank' OR (currency IS NOT NULL AND currency = 'UAH')); |
🤖 Prompt for AI Agents
In `@frontend/drizzle/manual/0006_monobank_uah_only.sql` around lines 44 - 50, The
CHECK constraint payment_attempts_mono_currency_uah on table payment_attempts
allows NULL currency for monobank rows because NULL makes the boolean expression
pass; update the constraint to require a non-NULL UAH for monobank rows by
replacing the condition with: provider <> 'monobank' OR (currency IS NOT NULL
AND currency = 'UAH'), so any row with provider='monobank' must have currency
set and equal to 'UAH'.
| const { appliedResult, restockOrderId, restockReason } = | ||
| await (async (): Promise<MonobankApplyOutcome> => { | ||
| const tx = db; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
head -n 340 frontend/lib/services/orders/monobank-webhook.ts | tail -n 25Repository: DevLoversTeam/devlovers.net
Length of output: 803
🏁 Script executed:
rg -n 'db\.transaction' --type=ts -C2 frontend/lib/services/orders/Repository: DevLoversTeam/devlovers.net
Length of output: 53
🏁 Script executed:
rg -n 'db\.transaction' --type=ts -C2Repository: DevLoversTeam/devlovers.net
Length of output: 53
🏁 Script executed:
rg -n 'transaction' --type=ts frontend/lib/Repository: DevLoversTeam/devlovers.net
Length of output: 420
🏁 Script executed:
fd '*.ts' frontend/ -exec rg -l 'db\.' {} \; | head -20Repository: DevLoversTeam/devlovers.net
Length of output: 551
🏁 Script executed:
sed -n '327,450p' frontend/lib/services/orders/monobank-webhook.tsRepository: DevLoversTeam/devlovers.net
Length of output: 4436
🏁 Script executed:
rg -n 'const db|export.*db' --type=ts frontend/lib/ -B2 -A2Repository: DevLoversTeam/devlovers.net
Length of output: 3370
🏁 Script executed:
cat frontend/lib/tests/shop/monobank-tx2-compensation.test.ts | head -50Repository: DevLoversTeam/devlovers.net
Length of output: 1228
🏁 Script executed:
rg -n '\.transaction\(' --type=ts frontend/lib/ -B3 -A3Repository: DevLoversTeam/devlovers.net
Length of output: 53
🏁 Script executed:
find frontend -name "*.ts" -path "*db*" -type f | grep -E "(index|init|client|database)" | head -20Repository: DevLoversTeam/devlovers.net
Length of output: 119
🏁 Script executed:
cat frontend/db/index.ts 2>/dev/null || find frontend -name "index.ts" -exec grep -l "drizzle\|postgres\|neon" {} \;Repository: DevLoversTeam/devlovers.net
Length of output: 1283
🏁 Script executed:
find . -name "*.ts" -exec grep -l "from '@/db'" {} \; | head -5Repository: DevLoversTeam/devlovers.net
Length of output: 3119
🏁 Script executed:
sed -n '1,30p' frontend/lib/services/orders/monobank-webhook.tsRepository: DevLoversTeam/devlovers.net
Length of output: 841
🏁 Script executed:
sed -n '249,780p' frontend/lib/services/orders/monobank-webhook.ts | grep -n "await\|transaction"Repository: DevLoversTeam/devlovers.net
Length of output: 1142
🏁 Script executed:
rg -n 'db\.transaction' frontend/lib/services/orders/ -A5Repository: DevLoversTeam/devlovers.net
Length of output: 53
🏁 Script executed:
grep -n "transaction" frontend/lib/services/orders/payment-state.tsRepository: DevLoversTeam/devlovers.net
Length of output: 53
🏁 Script executed:
cat frontend/lib/services/orders/payment-state.ts | head -80Repository: DevLoversTeam/devlovers.net
Length of output: 1960
🏁 Script executed:
rg -n 'transaction' frontend/lib/services/orders/payment-state.ts -B2 -A2Repository: DevLoversTeam/devlovers.net
Length of output: 53
🏁 Script executed:
sed -n '600,650p' frontend/lib/services/orders/monobank-webhook.tsRepository: DevLoversTeam/devlovers.net
Length of output: 1621
🏁 Script executed:
sed -n '690,760p' frontend/lib/services/orders/monobank-webhook.ts | head -40Repository: DevLoversTeam/devlovers.net
Length of output: 1281
🏁 Script executed:
cat frontend/lib/services/orders/monobank-webhook.ts | wc -lRepository: DevLoversTeam/devlovers.net
Length of output: 74
🏁 Script executed:
sed -n '750,805p' frontend/lib/services/orders/monobank-webhook.tsRepository: DevLoversTeam/devlovers.net
Length of output: 1444
🏁 Script executed:
git log --oneline -20 frontend/lib/services/orders/monobank-webhook.ts 2>/dev/null | head -10Repository: DevLoversTeam/devlovers.net
Length of output: 108
🏁 Script executed:
rg -n 'transaction' frontend/lib/services/orders/ --type=ts -B1 -A1Repository: DevLoversTeam/devlovers.net
Length of output: 53
Non-transactional multi-step state mutation labeled as tx
const tx = db is misleading — it reads like a transaction handle, but all subsequent updates (to paymentAttempts, orders, and monobankEvents) run as independent statements. If any statement fails after a prior one succeeds, the webhook leaves the system in a partially-applied state (e.g., order marked PAID but paymentAttempts still active, or vice versa).
Drizzle's db.transaction(...) is available in this codebase (as evidenced by test mocks). Wrap the critical multi-table mutations in db.transaction(...) to ensure atomicity, or at minimum rename tx to db to avoid implying transactional guarantees.
Minimal rename if you defer the real transaction:
- const tx = db;
+ // NOTE: not a true DB transaction – each update is independent
+ const conn = db;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const { appliedResult, restockOrderId, restockReason } = | |
| await (async (): Promise<MonobankApplyOutcome> => { | |
| const tx = db; | |
| const { appliedResult, restockOrderId, restockReason } = | |
| await (async (): Promise<MonobankApplyOutcome> => { | |
| // NOTE: not a true DB transaction – each update is independent | |
| const conn = db; |
🤖 Prompt for AI Agents
In `@frontend/lib/services/orders/monobank-webhook.ts` around lines 327 - 329, The
code assigns const tx = db but then performs multiple independent updates
(paymentAttempts, orders, monobankEvents) which is misleading and can leave
partial state; replace this by calling db.transaction(...) and run all the
multi-table mutations inside the transaction callback (using the returned tx
handle for queries/updates) so they commit atomically and rollback on failure;
if you intentionally do not make it transactional, at minimum rename tx to db to
avoid implying transactional semantics — target the block that constructs
MonobankApplyOutcome and the places that reference tx/db for updates to
paymentAttempts, orders and monobankEvents.
frontend/lib/tests/shop/checkout-monobank-idempotency-contract.test.ts
Outdated
Show resolved
Hide resolved
…rror texts/codes, and harden DB cleanup
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Fix all issues with AI agents
In `@frontend/app/api/shop/checkout/route.ts`:
- Around line 564-567: stripePaymentsEnabled currently only checks
STRIPE_PAYMENTS_ENABLED which causes silent failures; update its initializer to
fall back to PAYMENTS_ENABLED (e.g., set stripePaymentsEnabled to check
STRIPE_PAYMENTS_ENABLED then PAYMENTS_ENABLED) so Stripe honors the general
payments flag, and also add a defensive check inside stripePaymentFlow to log
and return a clear error when stripePaymentsEnabled is not true to prevent
returning clientSecret:null; reference paymentsEnabled, stripePaymentsEnabled,
and stripePaymentFlow when making these changes.
In `@frontend/docs/monobank-config.md`:
- Around line 108-110: The documentation incorrectly states that "apply" mode
does not store events; in monobank-webhook.ts the insertEvent() call is executed
unconditionally before the mode check, so apply mode does persist events then
applies them. Update the docs (the table rows for `apply`) to reflect that
`apply` verifies signature, stores the event (via insertEvent()), and then
applies order/payment updates; reference the insertEvent() call and the
mode-check logic in monobank-webhook.ts to ensure the wording matches the actual
behavior.
In `@frontend/lib/services/orders/monobank-refund.ts`:
- Around line 309-316: The current logic in the refund handler treats an
internal idempotency anomaly as a PSP outage by throwing PspUnavailableError
when inserted[0] is falsy and getExistingRefund(extRef) also returns nothing;
change this to throw an InvalidPayloadError (or similar) with a specific code
like 'REFUND_CONFLICT' and include context (orderId, requestId, extRef) so
operators won't misinterpret it as a PSP failure—update the branch that checks
inserted[0] and calls getExistingRefund(extRef) to throw new
InvalidPayloadError('Refund idempotency conflict.', { code: 'REFUND_CONFLICT',
orderId: args.orderId, requestId: args.requestId, extRef }) instead of
PspUnavailableError.
In `@frontend/lib/services/orders/monobank-webhook.ts`:
- Around line 277-279: applyMonoWebhookEvent currently falls back to hashing a
UTF-8 string when args.rawSha256 is missing, creating a fragile divergence from
handleMonobankWebhook which hashes the raw buffer; remove that fallback and
require rawSha256 to be supplied by callers (e.g., handleMonobankWebhook) by
replacing the conditional compute with an assertion/throw if args.rawSha256 is
undefined, referencing applyMonoWebhookEvent and rawSha256 so callers must
compute the SHA256 from the rawBody buffer (as done in handleMonobankWebhook)
before calling; this keeps a single canonical SHA256 computation path and
surfaces errors early.
In `@frontend/lib/services/orders/monobank.ts`:
- Around line 330-372: The fallback currently updates paymentAttempts metadata
(nextAttemptMeta) with finalizePending: true then always throws
PspInvoicePersistError, which leaves a live Monobank invoice when the DB write
succeeded; change the happy-fallback path in the function (the block that
updates paymentAttempts using db.update(paymentAttempts).set(...)) to not
immediately throw but instead return the payment link/invoice info (e.g.,
args.pageUrl and args.invoiceId) so the caller can present the pageUrl to the
customer and rely on webhook reconciliation; keep the existing catch branch that
logs via logError and calls cancelMonobankInvoice and cancelOrderAndRelease
before throwing PspInvoicePersistError, and ensure identifiers paymentAttempts,
nextAttemptMeta, cancelMonobankInvoice, cancelOrderAndRelease, logError, and
PspInvoicePersistError are used as currently to locate the code to change.
- Around line 194-211: markAttemptFailed currently overwrites the
paymentAttempts.metadata column with only args.meta, losing prior fields; change
the update to merge args.meta into the existing metadata instead (e.g., read
existing metadata and shallow-merge with args.meta then write back, or use a
JSONB concat expression in the update so metadata = metadata || <args.meta>),
keeping other fields like pageUrl/invoiceId/requestId intact; update the
db.update call in markAttemptFailed (and reference paymentAttempts.metadata) to
perform a merge rather than replacement and ensure null/undefined existing
metadata is handled.
- Around line 213-226: The cancelOrderAndRelease function currently
unconditionally updates orders to CANCELED and then calls restockOrder, which
risks TOCTOU if the order status changed concurrently; modify
cancelOrderAndRelease to perform an optimistic-concurrency update by adding a
WHERE clause that restricts the update to only cancellable statuses (e.g.,
'PENDING' or whatever the expected pre-cancel state) and check the update
result/rowCount — only call restockOrder(orderId, ...) when the update actually
affected a row; reference the function cancelOrderAndRelease and the subsequent
restockOrder call when making this change.
In `@frontend/lib/tests/shop/checkout-monobank-idempotency-contract.test.ts`:
- Around line 70-72: The conditional restoring of process.env.DATABASE_URL is
dead because __prevDatabaseUrl was captured earlier and will be undefined when
DATABASE_URL was missing; update the guard to check for undefined explicitly or
remove the block: either delete the entire if-block, or change the condition to
typeof __prevDatabaseUrl !== 'undefined' && !process.env.DATABASE_URL (or simply
if (__prevDatabaseUrl !== undefined) process.env.DATABASE_URL =
__prevDatabaseUrl) so the restore runs only when a previous value was actually
captured; locate the reference to __prevDatabaseUrl and process.env.DATABASE_URL
in the test teardown code to apply the change.
- Around line 392-499: The test leaves an orphaned order when assertions after
parsing res1 throw because orderId remains null; after calling postCheckout and
parsing the first response (res1 -> json1), assign orderId = json1.orderId
immediately (right after const json1 = await res1.json()) so the finally block
can always call cleanupOrder(orderId); keep all subsequent assertions unchanged
and ensure orderId is a string before using it in DB queries (references:
orderId variable, res1/json1, postCheckout, cleanupOrder).
🧹 Nitpick comments (15)
frontend/lib/tests/shop/monobank-attempt-lifecycle-d.test.ts (2)
59-67: Consider asserting call arguments, not just call counts.The test verifies that each dependency was called the expected number of times, but doesn't assert what was passed. For example, you could verify that
markAttemptFailedreceived'attempt-old', thatcreateMonobankInvoicereceived the correct amount/currency fromreadMonobankInvoiceParams, and thatfinalizeAttemptWithInvoicewas called with'attempt-new'and'inv_1'. Without argument assertions, regressions that pass the wrong attempt ID or invoice params through the pipeline won't be caught.Example additions
expect(deps.markAttemptFailed).toHaveBeenCalledTimes(1); + expect(deps.markAttemptFailed).toHaveBeenCalledWith( + expect.objectContaining({ attemptId: 'attempt-old' }), + ); expect(deps.cancelOrderAndRelease).not.toHaveBeenCalled(); expect(deps.createCreatingAttempt).toHaveBeenCalledTimes(1); expect(deps.createMonobankInvoice).toHaveBeenCalledTimes(1); expect(deps.finalizeAttemptWithInvoice).toHaveBeenCalledTimes(1); + expect(deps.finalizeAttemptWithInvoice).toHaveBeenCalledWith( + expect.objectContaining({ attemptId: 'attempt-new', invoiceId: 'inv_1' }), + );(Adjust the expected shapes to match the actual call signatures of
createMonoAttemptAndInvoiceImpl.)
10-50:as anysilently hides missing or mistyped deps — consider usingsatisfiesor a partial type.Casting
depsasanymeans the test won't break at compile time if the real dependency contract changes (e.g., a new required dep is added or one is renamed). If you have a type for the deps parameter, usingsatisfies Partial<DepsType>or a typed mock helper would give you compile-time feedback when the contract drifts.frontend/lib/tests/shop/checkout-monobank-idempotency-contract.test.ts (2)
330-339: Cleanup deletes are not wrapped in a transaction.If any intermediate
DELETEfails (e.g., theorder_itemsdelete), the subsequentpaymentAttemptsandordersdeletes are skipped, leaving the DB in a partially cleaned state that can cause flaky failures in later test runs. Wrapping these indb.transaction(…)would make cleanup atomic.The same applies to
cleanupProductbelow.♻️ Suggested transactional cleanup
async function cleanupOrder(orderId: string) { + await db.transaction(async (tx) => { - await db.execute( + await tx.execute( sql`delete from inventory_moves where order_id = ${orderId}::uuid` ); - await db.execute( + await tx.execute( sql`delete from order_items where order_id = ${orderId}::uuid` ); - await db.delete(paymentAttempts).where(eq(paymentAttempts.orderId, orderId)); - await db.delete(orders).where(eq(orders.id, orderId)); + await tx.delete(paymentAttempts).where(eq(paymentAttempts.orderId, orderId)); + await tx.delete(orders).where(eq(orders.id, orderId)); + }); }
144-289: Schema-introspection seeder is heavyweight but addresses the clean-DB concern.This solves the prior review feedback about tests failing on a clean database. The trade-off is significant complexity and coupling to
information_schemainternals. If theproductstable schema is relatively stable, a simpler hardcodedINSERT(with the known required columns) would be easier to maintain and debug. Consider simplifying if this becomes a maintenance burden.frontend/docs/monobank-config.md (1)
204-204: Empty "Notes / follow‑ups" section.The heading at the end of the document has no content. Either add planned follow-up items or remove the heading to avoid looking like an incomplete doc.
frontend/lib/tests/shop/monobank-psp-unavailable.test.ts (2)
50-52: Dead code: the DATABASE_URL guard is always a no-op.
__prevDatabaseUrlis captured fromprocess.env.DATABASE_URLon line 40. IfDATABASE_URLis unset at capture time,__prevDatabaseUrlisundefined, making the&&condition false. If it's already set, the!process.env.DATABASE_URLcheck is false. The conditional never executes.- if (!process.env.DATABASE_URL && __prevDatabaseUrl) { - process.env.DATABASE_URL = __prevDatabaseUrl; - }
133-146: Silentcatch {}in cleanup helpers may mask test infrastructure issues.Empty catch blocks in
cleanupProductsuppress all errors. If cleanup fails due to a schema change or connection issue, the test will pass but leave orphaned data. Consider logging a warning or at minimum adding a comment explaining why errors are swallowed.frontend/lib/services/orders/monobank-refund.ts (2)
232-274: Substantial duplication of reconciliation/retry logic.The "existing refund" path (lines 232–274) and the "insert conflict" path (lines 309–362) perform nearly identical sequences:
reconcileSuccessFromOrder→isDedupedRefundStatuscheck →isRetryableRefundStatuscheck →paymentStatusguard → update to'requested'. Extracting this into a shared helper (e.g.,reconcileAndRetryIfEligible(refundRow, orderRow)) would reduce duplication and the risk of the two paths diverging over time.Also applies to: 309-362
145-173:reconcileSuccessFromOrderupdates status unconditionally — consider adding a WHERE guard.Line 169 updates the refund row to
'success'without constraining on the current status. If this function is ever called outside the current dedup paths (e.g., from a future admin tool), it could overwrite'needs_review'or'failure'to'success'unexpectedly. Adding.where(and(eq(monobankRefunds.id, ...), inArray(monobankRefunds.status, ['requested', 'processing'])))would make it defensive.frontend/app/api/shop/checkout/route.ts (1)
468-476: Provider parsing happens twice — acceptable but worth noting.The
monobankRequestHint(line 468) pre-parses the provider before idempotency key validation, while lines 534–556 do the full parse. This duplication exists because the idempotency key error format depends on knowing the provider. The two-pass approach is correct for this use case.Also applies to: 534-556
frontend/lib/services/orders/monobank-webhook.ts (1)
41-42: UUID regex rejects v6–v8 UUIDs.
UUID_REonly accepts version digits[1-5]. UUID v6 and v7 (RFC 9562, ratified 2024) are increasingly common and would be rejected here. Since this regex validatesreferencefrom Monobank webhooks — which this system generates ascrypto.randomUUID()(v4) — this is fine today, but worth noting if the reference format ever changes.frontend/lib/services/orders/monobank.ts (4)
259-321: Retry loop catches all errors, including non-transient onesThe
try/catchat Line 318 swallows every error type indiscriminately—including "Payment attempt not found" (Line 268/288) and "Order not found" (Line 298/314), which are deterministic failures that will never succeed on retry. This wastes a retry cycle and delays the final error.Consider only retrying on transient/DB-connection errors and re-throwing deterministic failures immediately.
482-523: Snapshot validation failure cancels the order and re-throws the raw errorWhen
buildMonoMerchantPaymInfoFromSnapshotthrows (Line 494), the order is cancelled (Line 517) and the original error is re-thrown (Line 522). This rawMonobankMerchantPaymInfoErrormay not map to a well-defined HTTP status in the API layer, potentially leaking internal details to the client.Consider wrapping it in an
InvalidPayloadErroror similar domain error before re-throwing, consistent with how PSP failures are wrapped inPspUnavailableErrorat Line 581.
42-57: Consider using drizzle'sinArrayinstead of raw SQL for the status filterThe raw SQL fragment at Line 52 works but bypasses drizzle's type-safe query builder. Using
inArray(paymentAttempts.status, ['creating', 'active'])would be more idiomatic and benefit from compile-time checks.Proposed fix
+import { and, eq, inArray, sql } from 'drizzle-orm'; // ... async function getActiveAttempt( orderId: string ): Promise<PaymentAttemptRow | null> { const rows = await db .select() .from(paymentAttempts) .where( and( eq(paymentAttempts.orderId, orderId), eq(paymentAttempts.provider, 'monobank'), - sql`${paymentAttempts.status} in ('creating','active')` + inArray(paymentAttempts.status, ['creating', 'active']) ) ) .limit(1); return rows[0] ?? null; }
631-634: Test-only export exposes internal implementation details
__test__exportscreateMonoAttemptAndInvoiceImplandfinalizeAttemptWithInvoicefor testing. This is a common pattern, but consider gating it behindprocess.env.NODE_ENV !== 'production'or using a dedicated test-utilities module to avoid leaking internals in production bundles.
| try { | ||
| const [attemptRow] = await db | ||
| .select({ metadata: paymentAttempts.metadata }) | ||
| .from(paymentAttempts) | ||
| .where(eq(paymentAttempts.id, args.attemptId)) | ||
| .limit(1); | ||
|
|
||
| const nextAttemptMeta = { | ||
| ...asObj(attemptRow?.metadata), | ||
| pageUrl: args.pageUrl, | ||
| invoiceId: args.invoiceId, | ||
| requestId: args.requestId, | ||
| finalizePending: true, | ||
| }; | ||
|
|
||
| await db | ||
| .update(paymentAttempts) | ||
| .set({ | ||
| providerPaymentIntentId: args.invoiceId, | ||
| metadata: nextAttemptMeta, | ||
| updatedAt: new Date(), | ||
| }) | ||
| .where(eq(paymentAttempts.id, args.attemptId)); | ||
| } catch (error) { | ||
| logError('monobank_invoice_persist_failed', error, { | ||
| orderId: args.orderId, | ||
| attemptId: args.attemptId, | ||
| invoiceId: args.invoiceId, | ||
| requestId: args.requestId, | ||
| }); | ||
|
|
||
| await cancelMonobankInvoice(args.invoiceId); | ||
| await cancelOrderAndRelease(args.orderId, 'Invoice persistence failed.'); | ||
|
|
||
| throw new PspInvoicePersistError('Invoice persistence failed.', { | ||
| orderId: args.orderId, | ||
| }); | ||
| } | ||
|
|
||
| throw new PspInvoicePersistError('Invoice persistence failed.', { | ||
| orderId: args.orderId, | ||
| }); | ||
| } |
There was a problem hiding this comment.
After retry exhaustion, invoice remains alive at Monobank while order is in limbo
When the main retry loop (Lines 259–321) is exhausted, the fallback (Lines 330–367) persists finalizePending: true on the attempt and then throws PspInvoicePersistError at Line 369. However, the Monobank invoice was already created and is still payable. The customer could complete payment via the pageUrl, but the order won't reflect it until the webhook arrives and reconciles.
If the fallback metadata write succeeds (Line 345–352), consider either:
- Returning the
pageUrlto the caller so the customer can still pay (the webhook will eventually reconcile), or - Cancelling the Monobank invoice in this path as well (like the inner catch at Line 361 does).
Currently the happy-fallback path (metadata saved successfully) and the failure-fallback path (metadata save failed) diverge: one leaves the invoice alive, the other cancels it—but both throw the same error to the caller.
🤖 Prompt for AI Agents
In `@frontend/lib/services/orders/monobank.ts` around lines 330 - 372, The
fallback currently updates paymentAttempts metadata (nextAttemptMeta) with
finalizePending: true then always throws PspInvoicePersistError, which leaves a
live Monobank invoice when the DB write succeeded; change the happy-fallback
path in the function (the block that updates paymentAttempts using
db.update(paymentAttempts).set(...)) to not immediately throw but instead return
the payment link/invoice info (e.g., args.pageUrl and args.invoiceId) so the
caller can present the pageUrl to the customer and rely on webhook
reconciliation; keep the existing catch branch that logs via logError and calls
cancelMonobankInvoice and cancelOrderAndRelease before throwing
PspInvoicePersistError, and ensure identifiers paymentAttempts, nextAttemptMeta,
cancelMonobankInvoice, cancelOrderAndRelease, logError, and
PspInvoicePersistError are used as currently to locate the code to change.
frontend/lib/tests/shop/checkout-monobank-idempotency-contract.test.ts
Outdated
Show resolved
Hide resolved
…lback; cleanup tests & docs
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In `@frontend/docs/monobank-config.md`:
- Around line 199-202: The closing Markdown code-fence for the PowerShell
snippet is indented and has extra spacing, which can break rendering; locate the
block that starts with "Invoke-RestMethod -Method Post" and replace the indented
closing backticks with a left-aligned, three-backtick fence (```) with no
leading spaces so the opening and closing fences match; ensure there are exactly
three backticks and no additional characters on that line.
In `@frontend/lib/services/orders/monobank-webhook.ts`:
- Around line 41-42: The UUID_RE currently restricts the version nibble to
[1-5], causing UUIDv6/v7/v8 to be rejected; update the regex used by UUID_RE so
the version nibble accepts any hex digit (e.g., replace the [1-5] class with a
hex class like [0-9a-f]) while keeping the rest of the pattern (including the
variant class [89ab]) intact so UUIDv7 references from Monobank are matched and
referenceAttemptId is populated.
- Around line 844-846: Unrecognized parsed.normalized.status values currently
fall through and return appliedResult without updating the monobankEvents
appliedAt, allowing reclaims; add a final catch-all branch after the existing
status handlers in the monobank webhook handler (the block that inspects
parsed.normalized.status and sets
appliedResult/restockReason/restockOrderId/attemptId) that sets appliedResult to
something like 'applied_noop' and performs the same monobankEvents update that
writes appliedAt (i.e., invoke the existing update path used for other terminal
statuses) so the event row is marked applied and won’t be re-processed.
In `@frontend/lib/services/orders/monobank.ts`:
- Around line 633-651: The catch block around deps.cancelOrderAndRelease
currently throws the cancelError directly, which prevents the original
PspUnavailableError from propagating; change the handler in the catch for
cancelOrderAndRelease to always throw (or rethrow) a PspUnavailableError
(optionally wrapping the cancelError as a cause or adding it to metadata) so
callers expecting PspUnavailableError still receive it—update the catch in the
function that calls deps.cancelOrderAndRelease to log the cancelError via
logError('monobank_cancel_order_failed', ...) and then throw new
PspUnavailableError('Monobank invoice unavailable.', { orderId: args.orderId,
requestId: args.requestId, attemptId: attempt.id, cause: cancelError }) or
equivalent wrapping so the original error is preserved but the thrown type is
PspUnavailableError.
- Around line 365-376: The fallback block has an indentation mismatch: align the
`if (updatedAttempt[0]?.id)` line and its block to the same 4-space indentation
used throughout the surrounding `try` block and function; specifically locate
the `updatedAttempt` check and
`logWarn('monobank_invoice_persist_partial_attempt_only', { ... })` call and
re-indent them to match the surrounding code style so that `if
(updatedAttempt[0]?.id)` and its log call are nested consistently under the
enclosing scope where `paymentAttempts`, `args.attemptId`, and `invoiceId` are
used.
In `@frontend/lib/tests/shop/monobank-finalize-fallback.test.ts`:
- Line 44: The test title for the case in monobank-finalize-fallback.test.ts is
misleading: update the it(...) description so it accurately states that the
function rejects with PspInvoicePersistError but still persists the payment
attempt and cancels the invoice; keep the assertions intact (the
.rejects.toBeInstanceOf(PspInvoicePersistError) and checks that payment_attempts
persist and invoice is cancelled) and reference the existing test block (the
it(...) function starting "does not throw if order persist fails but
payment_attempts persist succeeds") when renaming to something like "rejects
with PspInvoicePersistError but still persists payment_attempt and cancels
invoice".
In `@frontend/lib/tests/shop/monobank-tx2-compensation.test.ts`:
- Around line 19-25: The mock makeUpdateQuery returns a plain object with a then
property which creates a thenable and triggers the noThenProperty lint and async
ordering issues; replace it with a real Promise so it is safely awaitable by
creating Promise.resolve(rows) (e.g., const p = Promise.resolve(rows)) and
attach the returning method via Object.assign(p, { returning: async () => rows
}) so makeUpdateQuery still supports .returning() but is a genuine Promise
rather than a thenable.
🧹 Nitpick comments (12)
frontend/lib/tests/shop/stripe-webhook-mismatch.test.ts (1)
11-11: Consider a note about theDbRows<T>type cast fragility.The
DbRows<T>type assertion ondb.execute()results relies on the raw query return shape. If Drizzle'sexecutereturn type changes across versions, these casts would silently produce incorrect types. This is low-risk for test code, but worth being aware of.frontend/docs/monobank-config.md (1)
206-206: Improve clarity of the "not no-op" phrasing.The phrase "is not 'no-op'" is grammatically awkward. Consider rewording for better readability.
✍️ Suggested rewording
-- `MONO_WEBHOOK_MODE=drop` is not "no-op": webhook signature is still verified and the event is still persisted (insertEvent); only state transitions are skipped. +- `MONO_WEBHOOK_MODE=drop` is not a no-op: webhook signature is still verified and the event is still persisted (insertEvent); only state transitions are skipped.Or more explicitly:
-- `MONO_WEBHOOK_MODE=drop` is not "no-op": webhook signature is still verified and the event is still persisted (insertEvent); only state transitions are skipped. +- `MONO_WEBHOOK_MODE=drop` still performs signature verification and persists the event (insertEvent); only state transitions are skipped.frontend/lib/tests/shop/monobank-psp-unavailable.test.ts (1)
34-71: Consider extracting env save/restore into a reusable helper.The pattern of saving six env vars, overriding them in
beforeAll, and conditionally restoring them inafterAllis repeated across multiple Monobank test files. A smallwithEnvOverrides(overrides)helper returning save/restore functions would reduce boilerplate and the risk of forgetting a variable.Not blocking — the current approach is correct and robust.
frontend/lib/services/orders/monobank-refund.ts (2)
295-373: Substantial code duplication between the existing-refund path and the insert-conflict path.The block at lines 328–368 (inside the
!inserted[0]conflict branch) duplicates the reconciliation logic from lines 237–277 (theif (existing)branch) almost verbatim:reconcileSuccessFromOrder→ checkisDedupedRefundStatus→ checkisRetryableRefundStatus→ checkpaymentStatus→ update to'requested'.Extract a shared helper like
resolveOrRetryRefund(refund, orderId, orderRow)to eliminate this duplication. This would reduce the risk of the two paths diverging silently if future changes update only one.
375-380: Defensive guard usesPspUnavailableErrorfor what is an internal logic error.If
refundRowForPspis stillnullat this point, it means a bug in the control flow above — not a PSP issue. Consider using a genericErroror a dedicated internal error type to make this distinction clear in monitoring/alerting.frontend/lib/services/orders/monobank.ts (1)
730-739: Extra DB query forattemptNumbercould be avoided.
createMonoAttemptAndInvoice(viacreateCreatingAttempt) already creates the attempt and has access to the attempt number. Consider threadingattemptNumberthrough the return value ofcreateMonoAttemptAndInvoiceto avoid this additional query.frontend/lib/tests/shop/monobank-finalize-fallback.test.ts (2)
75-83: Spy targetsdb.updatebroadly — fragile if production code order changes.The spy intercepts all
db.update(table)calls and selectively throws only whentable === orders. This works today but is order-dependent: if the production code ever callsdb.update(orders)for a different column before the payment-attempt update, the test silently exercises a different path. Consider adding an assertion or counter that verifies the spy was invoked exactly the expected number of times for each table to guard against future regressions.
38-41: Cleanup relies onfinally— good; but considerafterEachfor resilience.The
finallyblock ensures cleanup on failure, which is solid. However, if the test framework itself has an error before enteringtry, the records would leak. UsingafterEachwith theorderIdcaptured at describe scope would be slightly more resilient, though this is a minor nit for a sequential test.Also applies to: 117-120
frontend/app/api/shop/checkout/route.ts (4)
69-90: Server-authoritative money sourcing — good security posture.Stripping client-supplied money fields for Monobank prevents price manipulation. The
voidstatements to suppress unused-variable warnings work but are unconventional. An underscore-prefix destructuring (_amount, etc.) or a single// eslint-disable-next-linewould be more idiomatic if your linter supports it.
468-476: Provider is parsed from the body twice — consider extracting once.
parseRequestedProviderruns on the raw body both at line 472 (formonobankRequestHint) and again at line 540 (for routing). I understand the hint is needed early for error normalization, but you could parse once and reuse:Sketch
+ // ── parse provider once, before idempotency checks ── + let requestedProvider: CheckoutRequestedProvider | null = null; + let monobankRequestHint = false; + let payloadForValidation: unknown = body; + + if (body && typeof body === 'object' && !Array.isArray(body)) { + const { paymentProvider, provider, ...rest } = body as Record<string, unknown>; + const rawProvider = paymentProvider ?? provider; + const parsedProvider = parseRequestedProvider(rawProvider); + + monobankRequestHint = + parsedProvider === 'monobank' || + (parsedProvider === 'invalid' && isMonoAlias(rawProvider)); + + if (parsedProvider === 'invalid') { + // defer error response until after idempotency checks + } else { + requestedProvider = parsedProvider; + } + payloadForValidation = rest; + }This avoids re-destructuring the body and re-invoking the parser, though the current approach is functionally correct.
Also applies to: 531-556
486-492: Consider extracting provider-aware error formatting to reduce branching.There are five
if (selectedProvider === 'monobank') { return errorResponse('INVALID_REQUEST', …) }blocks interleaved with the existing Stripe error paths. Each returns a normalizedINVALID_REQUESTcode while the Stripe path returns more specific codes (MISSING_IDEMPOTENCY_KEY,INVALID_PAYLOAD,USER_MISMATCH, etc.).This works correctly but increases cyclomatic complexity. A possible consolidation:
function checkoutErrorResponse( selectedProvider: CheckoutRequestedProvider, stripeCode: string, message: string, status: number, details?: unknown ) { const code = selectedProvider === 'monobank' ? 'INVALID_REQUEST' : stripeCode; return errorResponse(code, message, status, details); }This would collapse each pair of if/return into a single call. Entirely optional — the current approach is explicit and easy to grep.
Also applies to: 507-514, 614-627, 672-678, 693-699
762-768: Unsafe cast onorder.paymentProvider— pre-existing but worth noting.The
as unknown as 'stripe' | 'monobank' | 'none'cast on line 762 has no runtime validation. An unexpected DB value would silently bypass both provider branches and return a generic response withclientSecret: null. This is pre-existing behavior, but with Monobank now added, the surface area for confusion grows. A runtime guard (or exhaustive switch with adefaultlog) would make unexpected values observable.Sketch
- const orderProvider = order.paymentProvider as unknown as - | 'stripe' - | 'monobank' - | 'none'; - - const stripePaymentFlow = orderProvider === 'stripe'; - const monobankPaymentFlow = orderProvider === 'monobank'; + const orderProvider = order.paymentProvider; + const stripePaymentFlow = orderProvider === 'stripe'; + const monobankPaymentFlow = orderProvider === 'monobank'; + + if (!stripePaymentFlow && !monobankPaymentFlow && orderProvider !== 'none') { + logError('checkout_unknown_provider', new Error('Unknown payment provider'), { + ...orderMeta, + provider: orderProvider, + }); + }
frontend/docs/monobank-config.md
Outdated
| Invoke-RestMethod -Method Post ` | ||
| -Uri "https://<host>/api/shop/admin/orders/<orderId>/refund" ` | ||
| -Headers @{ "Origin" = "https://<host>" } | ||
| ``` |
There was a problem hiding this comment.
Fix code block formatting at line 202.
The closing backticks at line 202 have incorrect indentation/spacing, which may cause rendering issues. The backticks should be flush left without extra spaces.
📝 Proposed fix
-Headers @{ "Origin" = "https://<host>" }
- ```
+```📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Invoke-RestMethod -Method Post ` | |
| -Uri "https://<host>/api/shop/admin/orders/<orderId>/refund" ` | |
| -Headers @{ "Origin" = "https://<host>" } | |
| ``` | |
| Invoke-RestMethod -Method Post ` | |
| -Uri "https://<host>/api/shop/admin/orders/<orderId>/refund" ` | |
| -Headers @{ "Origin" = "https://<host>" } |
🤖 Prompt for AI Agents
In `@frontend/docs/monobank-config.md` around lines 199 - 202, The closing
Markdown code-fence for the PowerShell snippet is indented and has extra
spacing, which can break rendering; locate the block that starts with
"Invoke-RestMethod -Method Post" and replace the indented closing backticks with
a left-aligned, three-backtick fence (```) with no leading spaces so the opening
and closing fences match; ensure there are exactly three backticks and no
additional characters on that line.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@frontend/lib/services/orders/monobank.ts`:
- Around line 582-588: The unguarded call to deps.cancelOrderAndRelease in the
buildMonoMerchantPaymInfoFromSnapshot error path can throw and replace the
original validation error; wrap the call to
deps.cancelOrderAndRelease(args.orderId, 'Monobank snapshot validation failed.')
in a try/catch so that if cancelOrderAndRelease throws you do not lose the
original "error" from buildMonoMerchantPaymInfoFromSnapshot — e.g., catch any
error from deps.cancelOrderAndRelease, log or attach it as secondary info, and
then rethrow the original validation error (or throw a composite error that
preserves both) so callers still receive the original validation failure.
🧹 Nitpick comments (4)
frontend/lib/services/orders/monobank.ts (2)
741-750: Extra DB round-trip to fetchattemptNumberthat's already known
createMonoAttemptAndInvoiceImplalready has theattemptrow (which containsattemptNumber) but doesn't include it in its return type. Adding it there would eliminate this additional query.
48-53: Consider using drizzle'sinArrayinstead of raw SQL for the status filterThis works but bypasses drizzle's type checking. The same pattern appears at line 233 in
cancelOrderAndRelease.- sql`${paymentAttempts.status} in ('creating','active')` + inArray(paymentAttempts.status, ['creating', 'active'])frontend/lib/services/orders/monobank-webhook.ts (2)
197-205: Raw SQL result typing relies on runtime shape assumption.The
as unknown as { rows?: Array<{ id?: string }> }cast assumes the Neon driver's response shape. If the driver changes its result format, this would silently returnnull. This pattern is used consistently throughout the file, so it's a known trade-off with raw SQL in drizzle.
347-865: Consider extracting the IIFE to a named helper.The ~500-line IIFE makes the function hard to navigate. Extracting it to a named function (e.g.,
resolveWebhookOutcome) would improve readability and make stack traces more informative, without changing behavior.
| if (orderRow.payment_status === 'paid' && status !== 'success') { | ||
| appliedResult = 'applied_noop'; | ||
| await dbx | ||
| .update(monobankEvents) | ||
| .set({ | ||
| appliedAt: now, | ||
| appliedResult, | ||
| attemptId: attemptRow.id, | ||
| orderId: orderRow.id, | ||
| }) | ||
| .where(eq(monobankEvents.id, eventId)); | ||
|
|
||
| return { | ||
| appliedResult, | ||
| restockReason, | ||
| restockOrderId, | ||
| attemptId, | ||
| orderId, | ||
| }; | ||
| } |
There was a problem hiding this comment.
Paid-terminal guard silently drops reversal and failure webhooks for paid orders.
When orderRow.payment_status === 'paid' and Monobank sends status === 'reversed' (or 'failure'/'expired'), this block returns applied_noop and the reversal handler at lines 768–843 is never reached. The order stays paid despite the payment being reversed at the PSP level.
The guard should only block non-terminal/informational statuses (processing, created) and duplicate success, while allowing reversed, failure, and expired to fall through to their respective handlers.
Proposed fix
- if (orderRow.payment_status === 'paid' && status !== 'success') {
+ if (
+ orderRow.payment_status === 'paid' &&
+ status !== 'success' &&
+ status !== 'reversed' &&
+ status !== 'failure' &&
+ status !== 'expired'
+ ) {
appliedResult = 'applied_noop';| await deps.cancelOrderAndRelease( | ||
| args.orderId, | ||
| 'Monobank snapshot validation failed.' | ||
| ); | ||
|
|
||
| throw error; | ||
| } |
There was a problem hiding this comment.
Unguarded cancelOrderAndRelease swallows the original validation error if it throws
The invoice-creation failure path (lines 634–647) correctly wraps cancelOrderAndRelease in try/catch, but this snapshot-validation failure path does not. If cancelOrderAndRelease throws (e.g., DB transient error), the original error from buildMonoMerchantPaymInfoFromSnapshot is lost and the caller receives an unrelated exception.
Proposed fix
- await deps.cancelOrderAndRelease(
- args.orderId,
- 'Monobank snapshot validation failed.'
- );
-
- throw error;
+ try {
+ await deps.cancelOrderAndRelease(
+ args.orderId,
+ 'Monobank snapshot validation failed.'
+ );
+ } catch (cancelErr) {
+ logError('monobank_cancel_order_failed', cancelErr, {
+ orderId: args.orderId,
+ attemptId: attempt.id,
+ requestId: args.requestId,
+ });
+ }
+
+ throw error;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| await deps.cancelOrderAndRelease( | |
| args.orderId, | |
| 'Monobank snapshot validation failed.' | |
| ); | |
| throw error; | |
| } | |
| try { | |
| await deps.cancelOrderAndRelease( | |
| args.orderId, | |
| 'Monobank snapshot validation failed.' | |
| ); | |
| } catch (cancelErr) { | |
| logError('monobank_cancel_order_failed', cancelErr, { | |
| orderId: args.orderId, | |
| attemptId: attempt.id, | |
| requestId: args.requestId, | |
| }); | |
| } | |
| throw error; | |
| } |
🤖 Prompt for AI Agents
In `@frontend/lib/services/orders/monobank.ts` around lines 582 - 588, The
unguarded call to deps.cancelOrderAndRelease in the
buildMonoMerchantPaymInfoFromSnapshot error path can throw and replace the
original validation error; wrap the call to
deps.cancelOrderAndRelease(args.orderId, 'Monobank snapshot validation failed.')
in a try/catch so that if cancelOrderAndRelease throws you do not lose the
original "error" from buildMonoMerchantPaymInfoFromSnapshot — e.g., catch any
error from deps.cancelOrderAndRelease, log or attach it as secondary info, and
then rethrow the original validation error (or throw a composite error that
preserves both) so callers still receive the original validation failure.
2 participants
Description
This PR adds the Monobank acquiring flow to the Shop in a production-safe, feature-gated way, while preserving existing Stripe behavior. The Monobank path is server-authoritative (UAH minor units only), fail-closed on PSP errors, and uses idempotency to prevent duplicate invoices/attempts. It also introduces an isolated Monobank webhook route with signature verification and exactly-once apply semantics, plus secure status access via signed tokens and admin-only operations for refund and unpaid invoice cancellation.
Related Issue
Issue: #<issue_number>
Changes
Database Changes (if applicable)
How Has This Been Tested?
Notes:
npm run buildpasses with Monobank env vars unset (Stripe path unaffected).Screenshots (if applicable)
N/A (backend + API changes)
Checklist
Before submitting
Reviewers
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes