(SP: 3)[SHOP][Wallets][NP] Stripe wallets (Apple/Google Pay) + Monobank Google Pay flow + Nova Poshta city_ref FK fixes#392
Merged
ViktorSvertoka merged 15 commits intodevelopfrom Mar 6, 2026
Conversation
…omain association placeholder
… lock lite status contract
…s pending unknown
…e; harden sync payload encoding
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Contributor
📝 WalkthroughWalkthroughAdds Monobank Google Pay end-to-end: frontend selection/UI and client, server config/submit/invoice routes, Monobank wallet submission service and webhook metadata, payment-method plumbing across checkout and validation, Nova Poshta city/warehouse refactor and DB migrations, plus many tests and i18n updates. Changes
Sequence Diagram(s)sequenceDiagram
actor Browser
participant Cart as CartPageClient
participant ConfigAPI as GET /api/.../google-pay/config
participant GPay as GooglePay JS
participant SubmitAPI as POST /api/.../google-pay/submit
participant WalletSvc as Monobank Wallet Service
participant Monobank as Monobank PSP
participant Return as MonobankReturnStatus
Browser->>Cart: select Monobank Google Pay, place order
Cart->>ConfigAPI: fetch Google Pay paymentDataRequest
ConfigAPI-->>Cart: paymentDataRequest
Cart->>GPay: init button, isReadyToPay → user approves
GPay-->>Cart: gToken (card token)
Cart->>SubmitAPI: POST gToken + idempotency + return URLs
SubmitAPI->>WalletSvc: submitMonobankWalletPayment(...)
WalletSvc->>Monobank: walletPayment request
Monobank-->>WalletSvc: response (invoiceId/redirect/status)
WalletSvc-->>SubmitAPI: submission result (statusToken/outcome)
SubmitAPI-->>Browser: response (redirect or pending + statusToken)
Browser->>Return: open return URL with statusToken
Return->>Return: poll /api/orders/{id}/status?view=lite&statusToken=...
Return-->>Browser: redirect to success or error when terminal
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
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: 04e7f5f83f
ℹ️ 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".
Comment on lines
+220
to
+223
| eq(paymentAttempts.orderId, orderId), | ||
| eq(paymentAttempts.provider, 'monobank'), | ||
| inArray(paymentAttempts.status, [...ACTIVE_ATTEMPT_STATUSES]) | ||
| ) |
There was a problem hiding this comment.
This active-attempt lookup treats any Monobank creating/active attempt as a wallet conflict, including invoice attempts created by checkout, so a monobank_google_pay order can be blocked with MONOBANK_WALLET_CONFLICT before wallet submission starts. In the current flow, checkout still initializes a Monobank invoice attempt first, so guest/user Google Pay submits hit this branch and return 409 instead of calling the PSP. Filter this query to wallet-tagged attempts (or avoid creating invoice attempts for Google Pay orders) so the wallet rail can proceed.
Useful? React with 👍 / 👎.
| const auth = await authorizeOrderMutationAccess({ | ||
| orderId, | ||
| statusToken, | ||
| requiredScope: 'order_payment_init', |
There was a problem hiding this comment.
This endpoint requires order_payment_init, but checkout-generated status tokens are created with the default status_lite scope, and the cart now forwards that token into the Google Pay page. For guest orders (no authenticated user fallback), config requests are rejected with STATUS_TOKEN_SCOPE_FORBIDDEN, so Google Pay cannot initialize at all. Either mint checkout tokens with order_payment_init for this flow or relax the required scope on the config/init path.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 12
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/stripe-webhook-refund-full.test.ts (1)
209-263:⚠️ Potential issue | 🟡 MinorWrap
fetchspy cleanup intry/finallyfor two tests to prevent spy leakage on assertion failures.The
vi.spyOn()calls on lines 209 and 432 restore the spy only on the success path. If an assertion fails beforefetchSpy.mockRestore()runs, the spy leaks into subsequent tests. The fail-closed test (line 266) already usestry/finallycorrectly; apply the same pattern to the other two tests.Pattern
- const fetchSpy = vi.spyOn(globalThis, 'fetch'); - const res = await POST(makeRequest()); + const fetchSpy = vi.spyOn(globalThis, 'fetch'); + try { + const res = await POST(makeRequest()); expect(res.status).toBe(200); - ... - fetchSpy.mockRestore(); + ... + } finally { + fetchSpy.mockRestore(); + }Also applies to: 432-489
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/tests/shop/stripe-webhook-refund-full.test.ts` around lines 209 - 263, The fetch spy created with vi.spyOn(globalThis, 'fetch') in the two tests (the one around the POST(makeRequest()) call and the other at lines ~432-489) must be restored in a finally block to avoid leaking the spy on assertion failures; wrap the part from creating fetchSpy through the POST and subsequent assertions in try { ... } finally { fetchSpy.mockRestore(); } so that fetchSpy.mockRestore() always runs (keep the existing assertions and calls to POST, expect(...), and checks like expect(retrieveChargeMock).not.toHaveBeenCalled()).frontend/lib/services/orders/monobank-webhook.ts (1)
503-522:⚠️ Potential issue | 🟠 MajorClear stale
pspMetadata.walletwhen payment succeeds or fails on non-wallet attempts.Orders can have multiple payment attempts. If a first attempt with wallet metadata (e.g., Google Pay) fails, then a second attempt without wallet metadata succeeds, the
jsonb || patchmerge preserves the stalewalletkey from the old metadata sincepatchdoesn't include it. The paid order remains incorrectly attributed to Google Pay.This affects both the success path (
atomicMarkPaidOrderAndSucceedAttempt) and failure path (atomicFinalizeOrderAndAttempt).Suggested fix
function buildMergedMetaSql( normalized: NormalizedWebhook, walletAttribution: WalletAttribution | null ) { const metadataPatch: Record<string, unknown> = { monobank: { invoiceId: normalized.invoiceId, status: normalized.status, amount: normalized.amount ?? null, ccy: normalized.ccy ?? null, reference: normalized.reference ?? null, }, }; - if (walletAttribution) { - metadataPatch.wallet = walletAttribution; - } - - return sql`coalesce(${orders.pspMetadata}, '{}'::jsonb) || ${JSON.stringify( - metadataPatch - )}::jsonb`; + const baseMeta = walletAttribution + ? sql`jsonb_set( + coalesce(${orders.pspMetadata}, '{}'::jsonb) - 'wallet', + '{wallet}', + ${JSON.stringify(walletAttribution)}::jsonb, + true + )` + : sql`coalesce(${orders.pspMetadata}, '{}'::jsonb) - 'wallet'`; + + return sql`${baseMeta} || ${JSON.stringify(metadataPatch)}::jsonb`; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/services/orders/monobank-webhook.ts` around lines 503 - 522, The merge currently preserves a stale pspMetadata.wallet because jsonb || only overwrites keys present in the RHS; modify buildMergedMetaSql so when walletAttribution is null it first removes the wallet key from orders.pspMetadata before merging. Concretely, keep constructing metadataPatch without wallet when walletAttribution is null, and return SQL that uses (coalesce(${orders.pspMetadata}, '{}'::jsonb) - 'wallet') || ${JSON.stringify(metadataPatch)}::jsonb in that branch; when walletAttribution is present keep the existing coalesce(...) || ${JSON.stringify(metadataPatch)}::jsonb behavior. This will clear stale wallet data for both atomicMarkPaidOrderAndSucceedAttempt and atomicFinalizeOrderAndAttempt flows that call buildMergedMetaSql.
🧹 Nitpick comments (16)
frontend/lib/tests/shop/checkout-shipping-phase3.test.ts (1)
159-161: Keep the unsupported-currency assertion type-aware.Line 159 now passes for any thrown value with a matching
code, including a plain object, so this test no longer protects the domain-error contract for this path. Prefer capturing the rejection once and asserting both the code and that the value is anErroror the expected service error type.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/tests/shop/checkout-shipping-phase3.test.ts` around lines 159 - 161, The test currently uses .rejects.toMatchObject({ code: 'SHIPPING_CURRENCY_UNSUPPORTED' }) which will match any plain object with that shape; change the assertion to capture the rejection and assert both that the thrown value is an Error (or the expected service error type) and that it carries the code 'SHIPPING_CURRENCY_UNSUPPORTED' — e.g. replace the single toMatchObject chain with two assertions like await expect(promise).rejects.toBeInstanceOf(Error) (or ShippingServiceError if that class exists) and await expect(promise).rejects.toHaveProperty('code', 'SHIPPING_CURRENCY_UNSUPPORTED'), or alternatively await promise.catch(e => { expect(e).toBeInstanceOf(Error); expect(e.code).toBe('SHIPPING_CURRENCY_UNSUPPORTED'); }) so the test verifies type and code instead of just shape.frontend/app/api/shop/internal/shipping/np/sync/route.ts (1)
97-99: Use an app-stage flag or platform-specific variable instead ofNODE_ENV.In deployed Next.js apps (including Vercel Preview deployments),
NODE_ENVis set toproductionfor all built deployments because they run in production mode. This makes the conditionNODE_ENV !== 'production'ineffective for distinguishing staging/preview from production environments. UseVERCEL_ENV(if on Vercel) or a dedicated flag likeAPP_ENVto reliably gate debug behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/api/shop/internal/shipping/np/sync/route.ts` around lines 97 - 99, The debug gating currently uses process.env.NODE_ENV in the debugEnabled assignment; replace that check with a platform-aware environment variable (e.g. process.env.VERCEL_ENV === 'preview' || process.env.APP_ENV === 'staging') so debugEnabled reliably distinguishes preview/stage vs production; update the expression that defines debugEnabled (the const debugEnabled and its use of request.headers.get('x-shop-debug')) to combine the chosen env-var check with the existing header check and trim logic so debug is enabled only when running in a non-production app-stage (VERCEL_ENV/APP_ENV) and the x-shop-debug header equals '1'.frontend/lib/tests/shop/stripe-webhook-refund-full.test.ts (1)
291-313: Assert the event stays retryable on this fail-closed branch.This new 500-path only checks order-side effects. Please also assert that the
stripeEventsrow exists andprocessedAtis stillNULL; otherwise this branch can accidentally mark the event handled and break retries without this test catching it.Suggested assertion block
expect(await res.json()).toEqual({ code: 'REFUND_FULLNESS_UNDETERMINED', }); + + const [evt] = await db + .select({ + processedAt: stripeEvents.processedAt, + paymentIntentId: stripeEvents.paymentIntentId, + }) + .from(stripeEvents) + .where(eq(stripeEvents.eventId, eventId)) + .limit(1); + + expect(evt).toBeTruthy(); + expect(evt.processedAt).toBeNull(); + expect(evt.paymentIntentId).toBe(inserted.paymentIntentId); const [row] = await db .select({ paymentStatus: orders.paymentStatus,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/tests/shop/stripe-webhook-refund-full.test.ts` around lines 291 - 313, Add an assertion that the stripeEvents row for this webhook still exists and remains unprocessed (processedAt is NULL) so the event stays retryable; query the stripeEvents table (using the stripeEvents symbol) for the test event id (e.g., use the previously created inserted.eventId or inserted.id as the identifier) and assert the row is found and that its processedAt column is null.frontend/lib/services/shop/shipping/nova-poshta-client.ts (1)
434-460: Rename this API togetWarehousesByCityRef.The implementation no longer accepts a settlement ref: it calls Nova Poshta with
CityRef. Keeping the old name makes the old semantics look valid at every call site and obscures the city-based migration.Suggested rename in this module
-export async function getWarehousesBySettlementRef( - settlementRef: string +export async function getWarehousesByCityRef( + cityRef: string ): Promise<NovaPoshtaWarehouse[]> { @@ const batch = await callNp<WarehouseItem>({ modelName: 'Address', calledMethod: 'getWarehouses', methodProperties: { - CityRef: settlementRef, + CityRef: cityRef, Limit: String(limit), Page: String(page), Language: 'ua', }, });The catalog-side wrappers/args should follow the same rename in the same pass.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/services/shop/shipping/nova-poshta-client.ts` around lines 434 - 460, The function getWarehousesBySettlementRef is misnamed because it uses CityRef in the API request; rename the function to getWarehousesByCityRef and update all local references and exports accordingly (including any wrapper functions and argument names in this module and catalog-side wrappers) so callers clearly reflect city-based semantics; ensure the implementation signature remains the same (settlementRef parameter should be renamed to cityRef where present), and update any tests or imports that reference getWarehousesBySettlementRef and the documented API name to the new getWarehousesByCityRef; verify usages of callNp and the methodProperties.CityRef remain correct.frontend/app/[locale]/shop/checkout/payment/monobank/MonobankGooglePayClient.tsx (1)
190-193: Reuse one idempotency key per in-flight wallet submit.Line 341 generates a brand-new key on every click. If the first POST reaches the server but the client never receives the response, the retry becomes a conflict/new-attempt path instead of an idempotent replay of the original submit.
♻️ Proposed change
const paymentsClientRef = useRef<GooglePayPaymentsClient | null>(null); const buttonHostRef = useRef<HTMLDivElement | null>(null); + const submitIdempotencyKeyRef = useRef<string | null>(null); @@ + submitIdempotencyKeyRef.current ??= generateIdempotencyKey(); const response = await fetch( `/api/shop/orders/${encodeURIComponent( orderId )}/payment/monobank/google-pay/submit${statusTokenQuery}`, { method: 'POST', headers: { 'Content-Type': 'application/json', - 'Idempotency-Key': generateIdempotencyKey(), + 'Idempotency-Key': submitIdempotencyKeyRef.current, }, body: JSON.stringify({ gToken }), } );If this component can ever be reused for a different
orderId, reset the ref whenorderIdchanges.Also applies to: 333-342
frontend/app/api/shop/checkout/route.ts (2)
111-116: DuplicateisMonobankGooglePayEnabledimplementation.This is the same function as in
frontend/lib/services/orders/checkout.ts(lines 744-749). Extract to a shared location like@/lib/env/monobankto avoid drift and ensure consistent behavior.♻️ Proposed extraction
Create in
frontend/lib/env/monobank.ts:export function isMonobankGooglePayEnabled(): boolean { const raw = (process.env.SHOP_MONOBANK_GPAY_ENABLED ?? '') .trim() .toLowerCase(); return raw === 'true' || raw === '1' || raw === 'yes' || raw === 'on'; }Then import from both files:
-function isMonobankGooglePayEnabled(): boolean { - const raw = (process.env.SHOP_MONOBANK_GPAY_ENABLED ?? '') - .trim() - .toLowerCase(); - return raw === 'true' || raw === '1' || raw === 'yes' || raw === 'on'; -} +import { isMonobankGooglePayEnabled } from '@/lib/env/monobank';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/api/shop/checkout/route.ts` around lines 111 - 116, Duplicate implementation of isMonobankGooglePayEnabled should be extracted to a single shared module: create a new module (e.g., export function isMonobankGooglePayEnabled in a new monobank env module) and replace both local implementations by importing that function; update callers in checkout route and orders/checkout to import the shared isMonobankGooglePayEnabled from the new module so behavior is centralized and avoids drift.
651-656: Default method mapping is hardcoded inline.The comment "Explicit default table (locked)" suggests this is intentional, but this duplicates logic that also exists in
resolveDefaultMethodForProviderfrom@/lib/shop/payments. Consider using that function for consistency.♻️ Suggested change
- // Explicit default table (locked): stripe -> stripe_card, monobank -> monobank_invoice. - const defaultMethod: PaymentMethod = - selectedProvider === 'monobank' ? 'monobank_invoice' : 'stripe_card'; - const selectedMethod: PaymentMethod = requestedMethod ?? defaultMethod; + const selectedMethod: PaymentMethod = + requestedMethod ?? resolveDefaultMethodForProvider(selectedProvider, selectedCurrency);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/api/shop/checkout/route.ts` around lines 651 - 656, The default payment method is hardcoded here duplicating logic in resolveDefaultMethodForProvider; replace the inline defaultMethod calculation with a call to resolveDefaultMethodForProvider(selectedProvider) and use its result when requestedMethod is null (affecting selectedMethod), keeping selectedProvider and selectedCurrency logic intact; ensure you import resolveDefaultMethodForProvider from "@/lib/shop/payments" and retain the existing resolveCurrencyFromLocale(locale) usage for selectedCurrency.frontend/app/[locale]/shop/checkout/payment/monobank/[orderId]/page.tsx (1)
40-55: Helper functions duplicated across pages.
getStringParam,parseStatusToken, andshouldClearCartare duplicated fromfrontend/app/[locale]/shop/checkout/return/monobank/page.tsx. Consider extracting these to a shared utility file.♻️ Proposed extraction
Create
frontend/app/[locale]/shop/checkout/_utils/search-params.ts:export type SearchParams = Record<string, string | string[] | undefined>; export function getStringParam(params: SearchParams | undefined, key: string): string { const raw = params?.[key]; if (!raw) return ''; if (Array.isArray(raw)) return raw[0] ?? ''; return raw; } export function parseStatusToken(params: SearchParams | undefined): string | null { const value = getStringParam(params, 'statusToken').trim(); return value.length ? value : null; } export function shouldClearCart(params: SearchParams | undefined): boolean { const value = getStringParam(params, 'clearCart'); return value === '1' || value === 'true'; }Then import in both pages.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/`[locale]/shop/checkout/payment/monobank/[orderId]/page.tsx around lines 40 - 55, Extract the duplicated helpers getStringParam, parseStatusToken, and shouldClearCart into a single shared module (e.g., search-params.ts) and import them from both pages; specifically, move the implementations of getStringParam, parseStatusToken, and shouldClearCart into the new module, export them (and the SearchParams type), then replace the local implementations in page.tsx files with imports of those functions to remove duplication.frontend/lib/tests/shop/monobank-google-pay-submit-route.test.ts (2)
145-166: Polling helper could mask intermittent failures.
waitForCreatingAttemptpolls every 25ms for up to 3 seconds but throws a generic error on timeout. Consider adding diagnostic information about what was found (if anything) to help debug flaky tests.♻️ Suggested improvement
async function waitForCreatingAttempt(orderId: string, timeoutMs = 3_000) { const deadline = Date.now() + timeoutMs; + let lastAttempt: { id: string; status: string } | undefined; while (Date.now() < deadline) { const [attempt] = await db .select({ id: paymentAttempts.id, status: paymentAttempts.status, }) .from(paymentAttempts) .where( and( eq(paymentAttempts.orderId, orderId), eq(paymentAttempts.provider, 'monobank') ) ) .limit(1); - if (attempt && attempt.status === 'creating') return; + lastAttempt = attempt; + if (attempt?.status === 'creating') return; await new Promise(resolve => setTimeout(resolve, 25)); } - throw new Error('Timed out waiting for creating payment attempt'); + throw new Error( + `Timed out waiting for creating payment attempt. Last seen: ${JSON.stringify(lastAttempt)}` + ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/tests/shop/monobank-google-pay-submit-route.test.ts` around lines 145 - 166, The helper waitForCreatingAttempt currently throws a generic timeout error which hides intermittent failures; update the function (waitForCreatingAttempt) to retain the last fetched row (from the paymentAttempts select) and include diagnostic details in the thrown error (e.g., JSON.stringify(lastAttempt), orderId, provider, and timeoutMs) or log them before throwing so tests show what was observed when the wait expired; ensure you reference the same query on paymentAttempts and keep the polling behavior otherwise unchanged.
91-115: Type assertion on insert values may mask schema drift.The
as anycast on line 114 bypasses type checking for the order insert. If the schema changes, this test helper won't catch mismatches at compile time. Consider defining a proper type or using Drizzle's inferred insert type.♻️ Suggested improvement
+import type { InferInsertModel } from 'drizzle-orm'; + +type OrderInsert = InferInsertModel<typeof orders>; + async function insertOrder(args: { id: string; paymentProvider?: 'monobank' | 'stripe'; paymentStatus?: 'pending' | 'requires_payment' | 'paid' | 'failed' | 'refunded'; currency?: 'UAH' | 'USD'; paymentMethod?: 'monobank_google_pay' | 'monobank_invoice' | 'stripe_card'; }) { - await db.insert(orders).values({ + const values: OrderInsert = { id: args.id, totalAmountMinor: 4321, totalAmount: toDbMoney(4321), currency: args.currency ?? 'UAH', paymentProvider: args.paymentProvider ?? 'monobank', paymentStatus: args.paymentStatus ?? 'pending', status: 'INVENTORY_RESERVED', inventoryStatus: 'reserved', idempotencyKey: `idem_${crypto.randomUUID()}`, pspPaymentMethod: args.paymentMethod ?? 'monobank_google_pay', pspMetadata: { checkout: { requestedMethod: args.paymentMethod ?? 'monobank_google_pay', }, }, - } as any); + }; + await db.insert(orders).values(values); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/tests/shop/monobank-google-pay-submit-route.test.ts` around lines 91 - 115, The test helper insertOrder uses a loose "as any" on the db.insert(orders).values(...) payload which hides schema mismatches; replace the cast by constructing a properly typed payload (use Drizzle's inferred insert type for the orders table or an explicit OrderInsert type) and pass that typed object to db.insert instead, making sure fields like totalAmount (toDbMoney(...)), pspPaymentMethod, and pspMetadata.checkout.requestedMethod match the orders schema; update the insertOrder function signature to use the typed payload so the compiler will catch any schema drift.frontend/app/[locale]/shop/checkout/return/monobank/MonobankReturnStatus.tsx (2)
157-163: **Polling continuation logic appears inverted.**The logic at lines 158-161 appears suspicious. After checking that the status is NOT inTERMINAL_NON_PAID(line 151-156) and NOTpaid(line 142-148), the code checks if!PENDING_STATUSES.has(nextStatus.paymentStatus)and continues polling. This means:
- If status IS pending → goes to line 163, continues polling ✓
- If status is NOT pending (and not terminal/paid) → goes to line 159, continues polling ✓
Both paths continue polling, which seems redundant. Consider simplifying.
♻️ Suggested simplification
if (TERMINAL_NON_PAID.has(nextStatus.paymentStatus)) { router.replace( `/shop/checkout/error?orderId=${encodeURIComponent(orderId)}` ); return; } - if (!PENDING_STATUSES.has(nextStatus.paymentStatus)) { - timer = setTimeout(poll, POLL_DELAY_MS); - return; - } - + // Continue polling for any non-terminal state timer = setTimeout(poll, POLL_DELAY_MS);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/`[locale]/shop/checkout/return/monobank/MonobankReturnStatus.tsx around lines 157 - 163, The polling logic currently schedules setTimeout twice and effectively always continues polling; update the poll control in the poll function so it only reschedules when the payment is actually pending: replace the duplicate timer scheduling with a single conditional that checks PENDING_STATUSES.has(nextStatus.paymentStatus) and calls setTimeout(poll, POLL_DELAY_MS) only in that case (leave existing early-return behavior for TERMINAL_NON_PAID and paid checks intact), removing the redundant timer assignment; refer to PENDING_STATUSES, nextStatus.paymentStatus, poll, and POLL_DELAY_MS when making the change.
182-185: Type assertion on translation key.The
as anycast on line 184 bypasses TypeScript's type checking for translation keys. If the translation namespace changes, this won't be caught at compile time.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/`[locale]/shop/checkout/return/monobank/MonobankReturnStatus.tsx around lines 182 - 185, The code bypasses type checking by casting the translation key with "as any" in the statusLabel useMemo; instead update getStatusLabelKey to return the correct translation key type (the union/keyof used by your i18n `t` function) or a typed wrapper (e.g., `getStatusLabelKey(paymentStatus): TranslationKey`) and then remove the "as any" cast so the call becomes `t(getStatusLabelKey(status.paymentStatus))`; ensure the `statusLabel` useMemo signature stays the same and adjust any related types where getStatusLabelKey is defined so TypeScript enforces valid translation keys.frontend/lib/services/orders/checkout.ts (2)
1062-1100: Backfill logic silently swallows update failures.The backfill for
pspPaymentMethodandpspMetadatacatches errors and only logs whenDEBUGis enabled. While this prevents checkout failures from backfill issues, it could mask persistent database problems. Consider logging atlogWarnlevel unconditionally to improve observability.♻️ Suggested improvement
} catch (e) { - if (process.env.DEBUG) { - logWarn('checkout_rejected', { - phase: 'payment_method_backfill', - orderId: row.id, - message: e instanceof Error ? e.message : String(e), - }); - } + logWarn('checkout_payment_method_backfill_failed', { + orderId: row.id, + message: e instanceof Error ? e.message : String(e), + }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/services/orders/checkout.ts` around lines 1062 - 1100, The catch block in the backfill branch (around the db.update(orders).set(...) call guarded by needsMethodBackfill or needsMetadataBackfill) only calls logWarn when process.env.DEBUG is set, which hides update failures; change the catch to always call logWarn('checkout_rejected', { phase: 'payment_method_backfill', orderId: row.id, message: e instanceof Error ? e.message : String(e) }) (remove the DEBUG conditional) so failures are logged at warn level unconditionally; keep the existing error message extraction and do not rethrow unless you want the backfill to fail the checkout.
744-749: Consider extracting shared env parsing logic.
isMonobankGooglePayEnabled()duplicates the same environment variable parsing logic that appears infrontend/app/api/shop/checkout/route.ts(lines 111-116). Consider extracting this to a shared utility in@/lib/env/monobankalongsideisMonobankEnabled()to ensure consistent behavior and reduce duplication.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/services/orders/checkout.ts` around lines 744 - 749, Extract the duplicated env parsing into a single utility (e.g., parseEnvBoolean(envValue: string | undefined): boolean) and replace the inline logic in isMonobankGooglePayEnabled and the other occurrence used by isMonobankEnabled to call this utility; ensure parseEnvBoolean normalizes (trim, toLowerCase) and returns true for 'true','1','yes','on', then update both isMonobankGooglePayEnabled() and isMonobankEnabled() to delegate to parseEnvBoolean(process.env.SHOP_MONOBANK_GPAY_ENABLED) / parseEnvBoolean(process.env.SHOP_MONOBANK_ENABLED) respectively so behavior is consistent and duplication removed.frontend/app/[locale]/shop/cart/CartPageClient.tsx (2)
98-105: Dead code:void args.monobankGooglePayEnabledhas no effect.The
voidexpression silences unused variable warnings but doesn't use the parameter. If the intent is to always default tomonobank_invoiceregardless of Google Pay availability, the parameter should be removed. Otherwise, implement the conditional logic.♻️ Proposed fix
If Google Pay should be the default when enabled:
function resolveDefaultMethodForProvider(args: { provider: CheckoutProvider; monobankGooglePayEnabled: boolean; }): CheckoutPaymentMethod { if (args.provider === 'stripe') return 'stripe_card'; - void args.monobankGooglePayEnabled; - return 'monobank_invoice'; + return args.monobankGooglePayEnabled ? 'monobank_google_pay' : 'monobank_invoice'; }Or if always defaulting to invoice is intentional, remove the unused parameter:
-function resolveDefaultMethodForProvider(args: { - provider: CheckoutProvider; - monobankGooglePayEnabled: boolean; -}): CheckoutPaymentMethod { +function resolveDefaultMethodForProvider( + provider: CheckoutProvider +): CheckoutPaymentMethod { - if (args.provider === 'stripe') return 'stripe_card'; - void args.monobankGooglePayEnabled; + if (provider === 'stripe') return 'stripe_card'; return 'monobank_invoice'; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/`[locale]/shop/cart/CartPageClient.tsx around lines 98 - 105, The function resolveDefaultMethodForProvider currently contains a no-op void args.monobankGooglePayEnabled which causes dead code; either remove monobankGooglePayEnabled from the parameter list and related type references (CheckoutProvider/CheckoutPaymentMethod usage) if the default should always be 'monobank_invoice', or implement the conditional: inside resolveDefaultMethodForProvider check args.monobankGooglePayEnabled when args.provider !== 'stripe' and return the Google Pay default (e.g., 'monobank_google_pay' or the correct CheckoutPaymentMethod enum/value used elsewhere) when true, otherwise return 'monobank_invoice'; also update any call sites and the function signature accordingly to keep types consistent.
1001-1034: Duplicate redirect logic formonobank_google_pay.The redirect to the Monobank Google Pay status page is duplicated: once when
monobankPageUrlexists (Lines 1002-1014) and again when it doesn't (Lines 1019-1034). Both branches perform the same redirect usingstatusToken. This creates dead code since the second block's condition (paymentProvider === 'monobank' && checkoutPaymentMethod === 'monobank_google_pay') withoutmonobankPageUrlwould never be reached if the first block already handles Google Pay withmonobankPageUrl.♻️ Consolidate the Google Pay redirect logic
if (paymentProvider === 'monobank' && monobankPageUrl) { - if (checkoutPaymentMethod === 'monobank_google_pay') { - if (!statusToken) { - setCheckoutError(t('checkout.errors.unexpectedResponse')); - return; - } - - router.push( - `${shopBase}/checkout/payment/monobank/${encodeURIComponent( - orderId - )}?statusToken=${encodeURIComponent(statusToken)}&clearCart=1` - ); - return; - } - window.location.assign(monobankPageUrl); return; } if ( paymentProvider === 'monobank' && checkoutPaymentMethod === 'monobank_google_pay' ) { if (!statusToken) { setCheckoutError(t('checkout.errors.unexpectedResponse')); return; } router.push( `${shopBase}/checkout/payment/monobank/${encodeURIComponent( orderId )}?statusToken=${encodeURIComponent(statusToken)}&clearCart=1` ); return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/`[locale]/shop/cart/CartPageClient.tsx around lines 1001 - 1034, Duplicate redirect logic for Monobank Google Pay exists: consolidate the two branches that handle paymentProvider === 'monobank' && checkoutPaymentMethod === 'monobank_google_pay' into a single conditional that first validates statusToken (call setCheckoutError(t('checkout.errors.unexpectedResponse')) and return if missing) and then redirects using router.push to `${shopBase}/checkout/payment/monobank/${encodeURIComponent(orderId)}?statusToken=${encodeURIComponent(statusToken)}&clearCart=1`; keep the separate window.location.assign(monobankPageUrl) path only for non-Google-Pay flows where monobankPageUrl is present, and remove the duplicated second block to avoid dead code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@frontend/app/`[locale]/shop/checkout/payment/monobank/MonobankGooglePayClient.tsx:
- Around line 113-140: The cached googlePayScriptPromise (used in
loadGooglePayScript) is never cleared on load failure, causing all future calls
to reuse a rejected promise; update the error paths (the existing element
'error' listener and the created script.onerror handler and the Promise
rejection branch) to clear or set googlePayScriptPromise to undefined before
rejecting so subsequent mounts can retry loading GOOGLE_PAY_SCRIPT_SRC; keep the
success path unchanged so the resolved promise remains cached.
In `@frontend/app/api/shop/internal/shipping/np/sync/route.ts`:
- Around line 248-273: The current debug payload built when debugEnabled is true
exposes raw error.message and sensitive deployment/config values (NP_API_BASE,
NP_API_KEY length) in the response; change the assembly of debug in route.ts
(the object built before calling noStoreJson) to never include raw messages or
raw config values: replace error.message with a sanitized field like errorType
or errorClass (use error instanceof NovaPoshtaApiError to set isNovaPoshtaError
and include only error.code/status if non-sensitive), and replace env entries
with an allowlist of non-sensitive booleans/strings (e.g., isNpApiKeyConfigured:
boolean, shopShippingNpEnabled: string|null, appEnv: string|null) while omitting
NP_API_BASE and any API key lengths; keep full error.message/stack logged to
processLogger (or existing logger) but only return the sanitized debug object in
the noStoreJson response.
In
`@frontend/app/api/shop/orders/`[id]/payment/monobank/google-pay/submit/route.ts:
- Around line 19-20: The Monobank Google Pay submit route must mint a portable
status token when the incoming query token is absent so status polling works
outside the original session; update the route handler that builds the Monobank
return URL (the code using toAbsoluteUrl and reading the query token validated
by idempotencyKeySchema/orderIdParamSchema) to create a portable token with type
"status_lite" and purpose "order_payment_init" when no token was supplied,
append that token to the return/status URL, and ensure any existing logic that
reuses the incoming query token falls back to this newly minted token for status
polling and redirects.
In `@frontend/app/api/shop/webhooks/stripe/route.ts`:
- Line 11: The charge.refund.updated handler currently throws
REFUND_FULLNESS_UNDETERMINED when the webhook provides a Refund with charge as a
string (no expanded charge); update the branch in route.ts to either (preferred)
fetch/expand the charge before inspecting fullness (use the extracted
refundChargeId to call Stripe API and set effectiveCharge) or (alternate) treat
an ID-only refund payload as non-fatal by logging a warning and returning a 200
without throwing; modify the code paths that reference effectiveCharge and
REFUND_FULLNESS_UNDETERMINED accordingly (look for variables/identifiers
refundChargeId, effectiveCharge, and the REFUND_FULLNESS_UNDETERMINED error) so
the handler no longer returns HTTP 500 for ID-only refund webhooks.
In `@frontend/drizzle/0029_shop_np_warehouses_city_ref_fk.sql`:
- Around line 1-13: Before adding the new FK constraint
np_warehouses_city_ref_np_cities_ref_fk on table np_warehouses(city_ref),
sanitize existing city_ref values by nulling any refs that do not exist in
np_cities(ref); i.e., run an update to set city_ref = NULL where city_ref IS NOT
NULL AND NOT EXISTS (SELECT 1 FROM np_cities WHERE ref = np_warehouses.city_ref)
and only then add the ALTER TABLE ... ADD CONSTRAINT statement, so the ADD
CONSTRAINT cannot fail due to orphaned city_ref values.
In `@frontend/lib/psp/monobank.ts`:
- Around line 783-809: The walletPayment() parsing currently returns invoiceId:
null when the provider omits it; change walletPayment() to validate invoiceId
after extracting it (from raw.invoiceId) and throw a descriptive Error if
invoiceId is missing/empty so callers can fail fast—this keeps the contract
expected by getInvoiceStatus() and createInvoice() which require a non-null
invoiceId. Locate the invoiceId extraction in monobank.ts (the block assigning
invoiceId, status, redirectUrl and returning the object) and replace the silent
null-return with a runtime check that throws (e.g., "Monobank wallet payment
missing invoiceId") before returning the result.
In `@frontend/lib/services/shop/shipping/nova-poshta-client.ts`:
- Around line 397-409: The code extracts SettlementRef into ref (via
readString(item.Ref)) and stores it in the results, but the subsequent call to
callNp for getWarehouses uses the wrong parameter name (CityRef) — change the
methodProperties key from CityRef to SettlementRef in the getWarehouses call
(the call with modelName: 'Address' and calledMethod: 'getWarehouses'), and
ensure it passes the settlement identifier variable (the same ref/settlementRef
produced from readString(item.Ref)) as the value.
In `@frontend/lib/tests/shop/monobank-api-methods.test.ts`:
- Around line 405-433: The test sets fake timers with vi.useFakeTimers() and
restores them at the end with vi.useRealTimers(), but restoration is not
guaranteed if assertions fail; wrap the test body that advances timers and
asserts (the block using vi.useFakeTimers(), vi.advanceTimersByTimeAsync(25),
the promise p, and the expect checks for PspError/code and fetchMock calls) in a
try/finally so vi.useRealTimers() is always executed; locate the walletPayment
test in monobank-api-methods.test.ts (the it('walletPayment timeout returns
PSP_TIMEOUT without retries' ...) block) and move the vi.useRealTimers() call
into the finally clause to guarantee cleanup.
In `@frontend/lib/tests/shop/order-items-snapshot-immutable.test.ts`:
- Around line 51-54: The test reimplements IP generation in makeTestClientIp;
replace its usage with the standard helper deriveTestIpFromIdemKey from
"@/lib/tests/helpers/ip.ts": remove or delete makeTestClientIp, import
deriveTestIpFromIdemKey, and call deriveTestIpFromIdemKey(idem) (or
deriveTestIpFromIdemKey(seed/idempotency key variable used in this test)
wherever makeTestClientIp was used) so tests use the RFC‑5737 TEST‑NET‑3
addresses consistently.
In `@frontend/messages/pl.json`:
- Around line 467-471: The JSON keys monobankGooglePay, monobankInvoice,
monobankGooglePayHint, monobankGooglePayFallbackHint, and
monobankGooglePayUnavailable contain English strings and must be localized to
Polish (or removed to allow fallback to the default locale) so the checkout and
Monobank flows don't show mixed-language UI; update those values with proper
Polish translations (or omit them so the i18n system falls back) and also apply
the same fix for the other block referenced (keys around lines 732-747) to
ensure all new Monobank/checkout copy is translated consistently.
In `@frontend/messages/uk.json`:
- Around line 467-471: The new Ukrainian locale keys (monobankGooglePay,
monobankInvoice, monobankGooglePayHint, monobankGooglePayFallbackHint,
monobankGooglePayUnavailable) contain English copy; update their values to
proper Ukrainian translations (or remove them to let the default locale be used)
so the checkout and Monobank return flows aren't mixed-language; apply the same
fix to the other untranslated block referenced (the similar keys around the
other section).
In `@frontend/scripts/np-mock-server.mjs`:
- Around line 184-223: The mock NP payload is missing ShortAddress and
ShortAddressRu fields so the warehouse mapping in nova-poshta-client.ts (which
expects ShortAddress/ShortAddressRu) yields null addresses; update each
warehouse object returned by okNp (the two entries with Ref '11111111-...' and
'22222222-...') to include ShortAddress and ShortAddressRu (e.g., mirror Address
and AddressRu or provide a concise variant) so the mock aligns with production
shape.
---
Outside diff comments:
In `@frontend/lib/services/orders/monobank-webhook.ts`:
- Around line 503-522: The merge currently preserves a stale pspMetadata.wallet
because jsonb || only overwrites keys present in the RHS; modify
buildMergedMetaSql so when walletAttribution is null it first removes the wallet
key from orders.pspMetadata before merging. Concretely, keep constructing
metadataPatch without wallet when walletAttribution is null, and return SQL that
uses (coalesce(${orders.pspMetadata}, '{}'::jsonb) - 'wallet') ||
${JSON.stringify(metadataPatch)}::jsonb in that branch; when walletAttribution
is present keep the existing coalesce(...) ||
${JSON.stringify(metadataPatch)}::jsonb behavior. This will clear stale wallet
data for both atomicMarkPaidOrderAndSucceedAttempt and
atomicFinalizeOrderAndAttempt flows that call buildMergedMetaSql.
In `@frontend/lib/tests/shop/stripe-webhook-refund-full.test.ts`:
- Around line 209-263: The fetch spy created with vi.spyOn(globalThis, 'fetch')
in the two tests (the one around the POST(makeRequest()) call and the other at
lines ~432-489) must be restored in a finally block to avoid leaking the spy on
assertion failures; wrap the part from creating fetchSpy through the POST and
subsequent assertions in try { ... } finally { fetchSpy.mockRestore(); } so that
fetchSpy.mockRestore() always runs (keep the existing assertions and calls to
POST, expect(...), and checks like
expect(retrieveChargeMock).not.toHaveBeenCalled()).
---
Nitpick comments:
In `@frontend/app/`[locale]/shop/cart/CartPageClient.tsx:
- Around line 98-105: The function resolveDefaultMethodForProvider currently
contains a no-op void args.monobankGooglePayEnabled which causes dead code;
either remove monobankGooglePayEnabled from the parameter list and related type
references (CheckoutProvider/CheckoutPaymentMethod usage) if the default should
always be 'monobank_invoice', or implement the conditional: inside
resolveDefaultMethodForProvider check args.monobankGooglePayEnabled when
args.provider !== 'stripe' and return the Google Pay default (e.g.,
'monobank_google_pay' or the correct CheckoutPaymentMethod enum/value used
elsewhere) when true, otherwise return 'monobank_invoice'; also update any call
sites and the function signature accordingly to keep types consistent.
- Around line 1001-1034: Duplicate redirect logic for Monobank Google Pay
exists: consolidate the two branches that handle paymentProvider === 'monobank'
&& checkoutPaymentMethod === 'monobank_google_pay' into a single conditional
that first validates statusToken (call
setCheckoutError(t('checkout.errors.unexpectedResponse')) and return if missing)
and then redirects using router.push to
`${shopBase}/checkout/payment/monobank/${encodeURIComponent(orderId)}?statusToken=${encodeURIComponent(statusToken)}&clearCart=1`;
keep the separate window.location.assign(monobankPageUrl) path only for
non-Google-Pay flows where monobankPageUrl is present, and remove the duplicated
second block to avoid dead code.
In `@frontend/app/`[locale]/shop/checkout/payment/monobank/[orderId]/page.tsx:
- Around line 40-55: Extract the duplicated helpers getStringParam,
parseStatusToken, and shouldClearCart into a single shared module (e.g.,
search-params.ts) and import them from both pages; specifically, move the
implementations of getStringParam, parseStatusToken, and shouldClearCart into
the new module, export them (and the SearchParams type), then replace the local
implementations in page.tsx files with imports of those functions to remove
duplication.
In
`@frontend/app/`[locale]/shop/checkout/return/monobank/MonobankReturnStatus.tsx:
- Around line 157-163: The polling logic currently schedules setTimeout twice
and effectively always continues polling; update the poll control in the poll
function so it only reschedules when the payment is actually pending: replace
the duplicate timer scheduling with a single conditional that checks
PENDING_STATUSES.has(nextStatus.paymentStatus) and calls setTimeout(poll,
POLL_DELAY_MS) only in that case (leave existing early-return behavior for
TERMINAL_NON_PAID and paid checks intact), removing the redundant timer
assignment; refer to PENDING_STATUSES, nextStatus.paymentStatus, poll, and
POLL_DELAY_MS when making the change.
- Around line 182-185: The code bypasses type checking by casting the
translation key with "as any" in the statusLabel useMemo; instead update
getStatusLabelKey to return the correct translation key type (the union/keyof
used by your i18n `t` function) or a typed wrapper (e.g.,
`getStatusLabelKey(paymentStatus): TranslationKey`) and then remove the "as any"
cast so the call becomes `t(getStatusLabelKey(status.paymentStatus))`; ensure
the `statusLabel` useMemo signature stays the same and adjust any related types
where getStatusLabelKey is defined so TypeScript enforces valid translation
keys.
In `@frontend/app/api/shop/checkout/route.ts`:
- Around line 111-116: Duplicate implementation of isMonobankGooglePayEnabled
should be extracted to a single shared module: create a new module (e.g., export
function isMonobankGooglePayEnabled in a new monobank env module) and replace
both local implementations by importing that function; update callers in
checkout route and orders/checkout to import the shared
isMonobankGooglePayEnabled from the new module so behavior is centralized and
avoids drift.
- Around line 651-656: The default payment method is hardcoded here duplicating
logic in resolveDefaultMethodForProvider; replace the inline defaultMethod
calculation with a call to resolveDefaultMethodForProvider(selectedProvider) and
use its result when requestedMethod is null (affecting selectedMethod), keeping
selectedProvider and selectedCurrency logic intact; ensure you import
resolveDefaultMethodForProvider from "@/lib/shop/payments" and retain the
existing resolveCurrencyFromLocale(locale) usage for selectedCurrency.
In `@frontend/app/api/shop/internal/shipping/np/sync/route.ts`:
- Around line 97-99: The debug gating currently uses process.env.NODE_ENV in the
debugEnabled assignment; replace that check with a platform-aware environment
variable (e.g. process.env.VERCEL_ENV === 'preview' || process.env.APP_ENV ===
'staging') so debugEnabled reliably distinguishes preview/stage vs production;
update the expression that defines debugEnabled (the const debugEnabled and its
use of request.headers.get('x-shop-debug')) to combine the chosen env-var check
with the existing header check and trim logic so debug is enabled only when
running in a non-production app-stage (VERCEL_ENV/APP_ENV) and the x-shop-debug
header equals '1'.
In `@frontend/lib/services/orders/checkout.ts`:
- Around line 1062-1100: The catch block in the backfill branch (around the
db.update(orders).set(...) call guarded by needsMethodBackfill or
needsMetadataBackfill) only calls logWarn when process.env.DEBUG is set, which
hides update failures; change the catch to always call
logWarn('checkout_rejected', { phase: 'payment_method_backfill', orderId:
row.id, message: e instanceof Error ? e.message : String(e) }) (remove the DEBUG
conditional) so failures are logged at warn level unconditionally; keep the
existing error message extraction and do not rethrow unless you want the
backfill to fail the checkout.
- Around line 744-749: Extract the duplicated env parsing into a single utility
(e.g., parseEnvBoolean(envValue: string | undefined): boolean) and replace the
inline logic in isMonobankGooglePayEnabled and the other occurrence used by
isMonobankEnabled to call this utility; ensure parseEnvBoolean normalizes (trim,
toLowerCase) and returns true for 'true','1','yes','on', then update both
isMonobankGooglePayEnabled() and isMonobankEnabled() to delegate to
parseEnvBoolean(process.env.SHOP_MONOBANK_GPAY_ENABLED) /
parseEnvBoolean(process.env.SHOP_MONOBANK_ENABLED) respectively so behavior is
consistent and duplication removed.
In `@frontend/lib/services/shop/shipping/nova-poshta-client.ts`:
- Around line 434-460: The function getWarehousesBySettlementRef is misnamed
because it uses CityRef in the API request; rename the function to
getWarehousesByCityRef and update all local references and exports accordingly
(including any wrapper functions and argument names in this module and
catalog-side wrappers) so callers clearly reflect city-based semantics; ensure
the implementation signature remains the same (settlementRef parameter should be
renamed to cityRef where present), and update any tests or imports that
reference getWarehousesBySettlementRef and the documented API name to the new
getWarehousesByCityRef; verify usages of callNp and the methodProperties.CityRef
remain correct.
In `@frontend/lib/tests/shop/checkout-shipping-phase3.test.ts`:
- Around line 159-161: The test currently uses .rejects.toMatchObject({ code:
'SHIPPING_CURRENCY_UNSUPPORTED' }) which will match any plain object with that
shape; change the assertion to capture the rejection and assert both that the
thrown value is an Error (or the expected service error type) and that it
carries the code 'SHIPPING_CURRENCY_UNSUPPORTED' — e.g. replace the single
toMatchObject chain with two assertions like await
expect(promise).rejects.toBeInstanceOf(Error) (or ShippingServiceError if that
class exists) and await expect(promise).rejects.toHaveProperty('code',
'SHIPPING_CURRENCY_UNSUPPORTED'), or alternatively await promise.catch(e => {
expect(e).toBeInstanceOf(Error);
expect(e.code).toBe('SHIPPING_CURRENCY_UNSUPPORTED'); }) so the test verifies
type and code instead of just shape.
In `@frontend/lib/tests/shop/monobank-google-pay-submit-route.test.ts`:
- Around line 145-166: The helper waitForCreatingAttempt currently throws a
generic timeout error which hides intermittent failures; update the function
(waitForCreatingAttempt) to retain the last fetched row (from the
paymentAttempts select) and include diagnostic details in the thrown error
(e.g., JSON.stringify(lastAttempt), orderId, provider, and timeoutMs) or log
them before throwing so tests show what was observed when the wait expired;
ensure you reference the same query on paymentAttempts and keep the polling
behavior otherwise unchanged.
- Around line 91-115: The test helper insertOrder uses a loose "as any" on the
db.insert(orders).values(...) payload which hides schema mismatches; replace the
cast by constructing a properly typed payload (use Drizzle's inferred insert
type for the orders table or an explicit OrderInsert type) and pass that typed
object to db.insert instead, making sure fields like totalAmount
(toDbMoney(...)), pspPaymentMethod, and pspMetadata.checkout.requestedMethod
match the orders schema; update the insertOrder function signature to use the
typed payload so the compiler will catch any schema drift.
In `@frontend/lib/tests/shop/stripe-webhook-refund-full.test.ts`:
- Around line 291-313: Add an assertion that the stripeEvents row for this
webhook still exists and remains unprocessed (processedAt is NULL) so the event
stays retryable; query the stripeEvents table (using the stripeEvents symbol)
for the test event id (e.g., use the previously created inserted.eventId or
inserted.id as the identifier) and assert the row is found and that its
processedAt column is null.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3734e522-0919-42fc-9d42-6a96c4f7513f
📒 Files selected for processing (63)
frontend/app/[locale]/shop/cart/CartPageClient.tsxfrontend/app/[locale]/shop/cart/page.tsxfrontend/app/[locale]/shop/checkout/payment/monobank/MonobankGooglePayClient.tsxfrontend/app/[locale]/shop/checkout/payment/monobank/[orderId]/page.tsxfrontend/app/[locale]/shop/checkout/return/monobank/MonobankReturnStatus.tsxfrontend/app/[locale]/shop/checkout/return/monobank/page.tsxfrontend/app/api/shop/checkout/route.tsfrontend/app/api/shop/internal/shipping/np/sync/route.tsfrontend/app/api/shop/orders/[id]/payment/monobank/_shared.tsfrontend/app/api/shop/orders/[id]/payment/monobank/google-pay/config/route.tsfrontend/app/api/shop/orders/[id]/payment/monobank/google-pay/submit/route.tsfrontend/app/api/shop/orders/[id]/payment/monobank/invoice/route.tsfrontend/app/api/shop/orders/[id]/status/route.tsfrontend/app/api/shop/shipping/methods/route.tsfrontend/app/api/shop/webhooks/stripe/route.tsfrontend/components/shop/header/CartButton.tsxfrontend/db/queries/shop/admin-orders.tsfrontend/db/schema/shop.tsfrontend/drizzle.config.tsfrontend/drizzle/0029_shop_np_warehouses_city_ref_fk.sqlfrontend/drizzle/meta/0029_snapshot.jsonfrontend/drizzle/meta/_journal.jsonfrontend/lib/psp/monobank.tsfrontend/lib/psp/stripe.tsfrontend/lib/services/orders/_shared.tsfrontend/lib/services/orders/checkout.tsfrontend/lib/services/orders/monobank-wallet.tsfrontend/lib/services/orders/monobank-webhook.tsfrontend/lib/services/shop/notifications/outbox-worker.tsfrontend/lib/services/shop/quotes.tsfrontend/lib/services/shop/returns.tsfrontend/lib/services/shop/shipping/admin-actions.tsfrontend/lib/services/shop/shipping/inventory-eligibility.tsfrontend/lib/services/shop/shipping/metrics.tsfrontend/lib/services/shop/shipping/nova-poshta-catalog.tsfrontend/lib/services/shop/shipping/nova-poshta-client.tsfrontend/lib/services/shop/transitions/order-state.tsfrontend/lib/shop/payments.tsfrontend/lib/tests/shop/checkout-currency-policy.test.tsfrontend/lib/tests/shop/checkout-monobank-idempotency-contract.test.tsfrontend/lib/tests/shop/checkout-monobank-parse-validation.test.tsfrontend/lib/tests/shop/checkout-shipping-phase3.test.tsfrontend/lib/tests/shop/monobank-api-methods.test.tsfrontend/lib/tests/shop/monobank-google-pay-config-route.test.tsfrontend/lib/tests/shop/monobank-google-pay-submit-route.test.tsfrontend/lib/tests/shop/monobank-janitor-job3.test.tsfrontend/lib/tests/shop/monobank-wallet-service.test.tsfrontend/lib/tests/shop/monobank-webhook-apply.test.tsfrontend/lib/tests/shop/notifications-worker-transport-phase3.test.tsfrontend/lib/tests/shop/nova-poshta-client-network-failure.test.tsfrontend/lib/tests/shop/order-items-snapshot-immutable.test.tsfrontend/lib/tests/shop/order-payment-init-token-scope-phase7.test.tsfrontend/lib/tests/shop/order-status-token.test.tsfrontend/lib/tests/shop/product-prices-write-authority-phase8.test.tsfrontend/lib/tests/shop/shipping-retention-phase7.test.tsfrontend/lib/tests/shop/stripe-webhook-psp-fields.test.tsfrontend/lib/tests/shop/stripe-webhook-refund-full.test.tsfrontend/lib/validation/shop.tsfrontend/messages/en.jsonfrontend/messages/pl.jsonfrontend/messages/uk.jsonfrontend/proxy.tsfrontend/scripts/np-mock-server.mjs
💤 Files with no reviewable changes (3)
- frontend/lib/tests/shop/product-prices-write-authority-phase8.test.ts
- frontend/lib/services/shop/notifications/outbox-worker.ts
- frontend/lib/services/shop/shipping/metrics.ts
frontend/app/[locale]/shop/checkout/payment/monobank/MonobankGooglePayClient.tsx
Outdated
Show resolved
Hide resolved
frontend/app/api/shop/orders/[id]/payment/monobank/google-pay/submit/route.ts
Outdated
Show resolved
Hide resolved
Comment on lines
+1
to
+13
| ALTER TABLE np_warehouses | ||
| DROP CONSTRAINT IF EXISTS np_warehouses_settlement_ref_np_cities_ref_fk; | ||
|
|
||
| DO $$ | ||
| BEGIN | ||
| IF NOT EXISTS ( | ||
| SELECT 1 FROM pg_constraint | ||
| WHERE conname = 'np_warehouses_city_ref_np_cities_ref_fk' | ||
| ) THEN | ||
| ALTER TABLE np_warehouses | ||
| ADD CONSTRAINT np_warehouses_city_ref_np_cities_ref_fk | ||
| FOREIGN KEY (city_ref) REFERENCES np_cities(ref) | ||
| ON DELETE SET NULL; |
Contributor
There was a problem hiding this comment.
Sanitize existing city_ref data before enforcing the FK.
city_ref was previously unconstrained, so one orphaned value is enough to make this ADD CONSTRAINT fail on deploy. Backfill or null invalid refs in the migration before adding the foreign key.
Suggested migration hardening
ALTER TABLE np_warehouses
DROP CONSTRAINT IF EXISTS np_warehouses_settlement_ref_np_cities_ref_fk;
+UPDATE np_warehouses w
+SET city_ref = NULL
+WHERE city_ref IS NOT NULL
+ AND NOT EXISTS (
+ SELECT 1
+ FROM np_cities c
+ WHERE c.ref = w.city_ref
+ );
+
DO $$
BEGIN
IF NOT EXISTS (
SELECT 1 FROM pg_constraint
WHERE conname = 'np_warehouses_city_ref_np_cities_ref_fk'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ALTER TABLE np_warehouses | |
| DROP CONSTRAINT IF EXISTS np_warehouses_settlement_ref_np_cities_ref_fk; | |
| DO $$ | |
| BEGIN | |
| IF NOT EXISTS ( | |
| SELECT 1 FROM pg_constraint | |
| WHERE conname = 'np_warehouses_city_ref_np_cities_ref_fk' | |
| ) THEN | |
| ALTER TABLE np_warehouses | |
| ADD CONSTRAINT np_warehouses_city_ref_np_cities_ref_fk | |
| FOREIGN KEY (city_ref) REFERENCES np_cities(ref) | |
| ON DELETE SET NULL; | |
| ALTER TABLE np_warehouses | |
| DROP CONSTRAINT IF EXISTS np_warehouses_settlement_ref_np_cities_ref_fk; | |
| UPDATE np_warehouses w | |
| SET city_ref = NULL | |
| WHERE city_ref IS NOT NULL | |
| AND NOT EXISTS ( | |
| SELECT 1 | |
| FROM np_cities c | |
| WHERE c.ref = w.city_ref | |
| ); | |
| DO $$ | |
| BEGIN | |
| IF NOT EXISTS ( | |
| SELECT 1 FROM pg_constraint | |
| WHERE conname = 'np_warehouses_city_ref_np_cities_ref_fk' | |
| ) THEN | |
| ALTER TABLE np_warehouses | |
| ADD CONSTRAINT np_warehouses_city_ref_np_cities_ref_fk | |
| FOREIGN KEY (city_ref) REFERENCES np_cities(ref) | |
| ON DELETE SET NULL; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/drizzle/0029_shop_np_warehouses_city_ref_fk.sql` around lines 1 -
13, Before adding the new FK constraint np_warehouses_city_ref_np_cities_ref_fk
on table np_warehouses(city_ref), sanitize existing city_ref values by nulling
any refs that do not exist in np_cities(ref); i.e., run an update to set
city_ref = NULL where city_ref IS NOT NULL AND NOT EXISTS (SELECT 1 FROM
np_cities WHERE ref = np_warehouses.city_ref) and only then add the ALTER TABLE
... ADD CONSTRAINT statement, so the ADD CONSTRAINT cannot fail due to orphaned
city_ref values.
Comment on lines
184
to
223
| return json( | ||
| res, | ||
| 200, | ||
| okNp([ | ||
| { | ||
| Ref: '11111111-1111-1111-1111-111111111111', | ||
| Description: 'Warehouse #1 (mock)', | ||
| CityRef: cityRef, | ||
| SettlementRef: cityRef, | ||
|
|
||
| Number: '1', | ||
| TypeOfWarehouse: 'Warehouse', | ||
|
|
||
| Description: 'Відділення №1 (mock)', | ||
| DescriptionRu: 'Отделение №1 (mock)', | ||
|
|
||
| Address: 'вул. Хрещатик, 1', | ||
| AddressRu: 'ул. Крещатик, 1', | ||
|
|
||
| PostMachine: '0', | ||
| IsPostMachine: false, | ||
| CategoryOfWarehouse: 'Branch', | ||
| }, | ||
| { | ||
| Ref: '22222222-2222-2222-2222-222222222222', | ||
| CityRef: cityRef, | ||
| SettlementRef: cityRef, | ||
|
|
||
| Number: '2', | ||
| TypeOfWarehouse: 'Postomat', | ||
|
|
||
| Description: 'Поштомат №1 (mock)', | ||
| DescriptionRu: 'Почтомат №1 (mock)', | ||
|
|
||
| Address: 'вул. Хрещатик, 2', | ||
| AddressRu: 'ул. Крещатик, 2', | ||
|
|
||
| PostMachine: '1', | ||
| IsPostMachine: true, | ||
| CategoryOfWarehouse: 'Postomat', | ||
| }, |
Contributor
There was a problem hiding this comment.
Expose ShortAddress in the mock payload as well.
frontend/lib/services/shop/shipping/nova-poshta-client.ts still maps warehouses from ShortAddress / ShortAddressRu. With only Address / AddressRu here, mock-backed sync writes address = null, so address search and local UI coverage drift from production.
Suggested mock payload fix
{
Ref: '11111111-1111-1111-1111-111111111111',
CityRef: cityRef,
SettlementRef: cityRef,
@@
Description: 'Відділення №1 (mock)',
DescriptionRu: 'Отделение №1 (mock)',
Address: 'вул. Хрещатик, 1',
AddressRu: 'ул. Крещатик, 1',
+ ShortAddress: 'вул. Хрещатик, 1',
+ ShortAddressRu: 'ул. Крещатик, 1',
@@
{
Ref: '22222222-2222-2222-2222-222222222222',
CityRef: cityRef,
SettlementRef: cityRef,
@@
Description: 'Поштомат №1 (mock)',
DescriptionRu: 'Почтомат №1 (mock)',
Address: 'вул. Хрещатик, 2',
AddressRu: 'ул. Крещатик, 2',
+ ShortAddress: 'вул. Хрещатик, 2',
+ ShortAddressRu: 'ул. Крещатик, 2',🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/scripts/np-mock-server.mjs` around lines 184 - 223, The mock NP
payload is missing ShortAddress and ShortAddressRu fields so the warehouse
mapping in nova-poshta-client.ts (which expects ShortAddress/ShortAddressRu)
yields null addresses; update each warehouse object returned by okNp (the two
entries with Ref '11111111-...' and '22222222-...') to include ShortAddress and
ShortAddressRu (e.g., mirror Address and AddressRu or provide a concise variant)
so the mock aligns with production shape.
…ility, and stabilize tests
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
frontend/lib/services/orders/checkout.ts (1)
875-882:⚠️ Potential issue | 🟠 MajorHonor an explicit
paymentProvider: 'none'.
requestedProvider === 'none'currently falls through to Stripe whenever Stripe is enabled, so an explicit no-PSP checkout still creates a Stripe-backed order and never reaches the no-payment reconciliation path.💡 Suggested fix
const paymentProvider: PaymentProvider = - requestedProvider === 'monobank' - ? 'monobank' - : stripePaymentsEnabled - ? 'stripe' - : 'none'; - const paymentsEnabled = - paymentProvider === 'monobank' ? true : stripePaymentsEnabled; + requestedProvider === 'none' + ? 'none' + : requestedProvider === 'monobank' + ? 'monobank' + : stripePaymentsEnabled + ? 'stripe' + : 'none'; + const paymentsEnabled = paymentProvider !== 'none';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/services/orders/checkout.ts` around lines 875 - 882, The logic computing paymentProvider and paymentsEnabled wrongly treats requestedProvider === 'none' as fallthrough to Stripe; update the selection so paymentProvider is exactly requestedProvider when it is one of 'monobank'|'stripe'|'none' (or explicitly check for 'none') and only choose 'stripe' as a fallback when requestedProvider is not provided and stripePaymentsEnabled is true; also compute paymentsEnabled as paymentProvider !== 'none' && (paymentProvider === 'monobank' || stripePaymentsEnabled) so an explicit requestedProvider === 'none' produces paymentProvider === 'none' and paymentsEnabled === false.frontend/app/[locale]/shop/cart/CartPageClient.tsx (1)
1523-1532:⚠️ Potential issue | 🟡 MinorCollapse the warehouse suggestions after a selection.
This handler keeps
warehouseOptionspopulated, so the dropdown stays open and the follow-up fetch can repopulate it even though a warehouse is already chosen.💡 Minimal fix
onClick={() => { clearCheckoutUiErrors(); setSelectedWarehouseRef(warehouse.ref); setSelectedWarehouseName(warehouse.name); setWarehouseQuery( warehouse.address ? `${warehouse.name} (${warehouse.address})` : warehouse.name ); + setWarehouseOptions([]); }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/`[locale]/shop/cart/CartPageClient.tsx around lines 1523 - 1532, The click handler selects a warehouse but leaves warehouseOptions populated so the suggestions dropdown remains open; update the handler (alongside clearCheckoutUiErrors, setSelectedWarehouseRef, setSelectedWarehouseName, setWarehouseQuery) to collapse the suggestions by clearing the options and/or hiding the list—e.g. call setWarehouseOptions([]) and/or setShowWarehouseOptions(false) after setting the selected values so the dropdown closes and subsequent fetches won't keep it open.
♻️ Duplicate comments (1)
frontend/lib/psp/monobank.ts (1)
342-366:⚠️ Potential issue | 🟡 MinorTighten wallet payload normalization before the PSP call.
cardTokenis validated againsttrim()but the untrimmed value is still sent, and this helper accepts any positiveccyeven though the Monobank integration here is UAH-only elsewhere. Failing fast here avoids avoidable upstream 4xx responses.💡 Minimal fix
export function buildMonobankWalletPayload( args: MonobankWalletPaymentInput ): MonobankWalletPaymentRequest { - if (typeof args.cardToken !== 'string' || !args.cardToken.trim()) { + const cardToken = + typeof args.cardToken === 'string' ? args.cardToken.trim() : ''; + if (!cardToken) { throw new Error('wallet cardToken is required'); } if (!Number.isSafeInteger(args.amountMinor) || args.amountMinor <= 0) { throw new Error('Invalid wallet amount (minor units)'); } - if (!Number.isSafeInteger(args.ccy) || args.ccy <= 0) { - throw new Error('Invalid wallet currency code'); + if (args.ccy !== MONO_CCY) { + throw new Error(`Monobank wallet payments require ccy=${MONO_CCY}`); } const redirectUrl = args.redirectUrl.trim(); const webHookUrl = args.webHookUrl.trim(); if (!redirectUrl || !webHookUrl) { throw new Error('wallet redirectUrl and webHookUrl are required'); } return { - cardToken: args.cardToken, + cardToken, amount: args.amountMinor, ccy: args.ccy, initiationKind: 'client',🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/psp/monobank.ts` around lines 342 - 366, Trim and normalize the wallet payload before returning: compute and use a trimmed cardToken (e.g., const cardToken = args.cardToken.trim()) instead of sending the untrimmed args.cardToken, and tighten currency validation to only accept UAH (require args.ccy === 980 or compare against a MONOBANK_UAH constant) rather than any positive integer; keep the existing amountMinor and URL checks but ensure redirectUrl and webHookUrl are derived from their trimmed values when building the returned object (use cardToken, amount: args.amountMinor, ccy: 980, initiationKind: 'client', redirectUrl, webHookUrl).
🧹 Nitpick comments (5)
frontend/lib/tests/shop/monobank-api-methods.test.ts (1)
304-311: DropinitiationKindfrom the test input.
buildMonobankWalletPayload()already hardcodesinitiationKind: 'client'infrontend/lib/psp/monobank.ts:339-368. Passing it here makes this test less useful, because it no longer proves thatwalletPaymentis enforcing that field itself.Suggested fix
const result = await walletPayment({ cardToken: 'token-value', amountMinor: 1234, ccy: 980, - initiationKind: 'client', redirectUrl: 'https://shop.test/return/monobank', webHookUrl: 'https://shop.test/api/shop/webhooks/monobank', });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/tests/shop/monobank-api-methods.test.ts` around lines 304 - 311, The test is passing initiationKind explicitly which masks enforcement in walletPayment; remove the initiationKind property from the test payload in monobank-api-methods.test.ts so walletPayment/buildMonobankWalletPayload must supply initiationKind itself (see walletPayment and buildMonobankWalletPayload), then run the test to confirm behavior still passes and that initiationKind is set by the code under test.frontend/lib/services/shop/shipping/nova-poshta-client.ts (2)
434-482: Pagination fallback logic is well-designed but could benefit from a comment.The fallback from
LIMIT_PRIMARY=500toLIMIT_FALLBACK=200on empty first-page results is a good defensive pattern for APIs that may reject large page sizes. However, the reasoning isn't immediately obvious.Consider adding a brief comment explaining why this fallback exists (e.g., some NP endpoints may return empty results for large limits).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/services/shop/shipping/nova-poshta-client.ts` around lines 434 - 482, Add a short clarifying comment above the first-page fallback logic in getWarehousesByCityRef explaining why LIMIT_PRIMARY (500) is reduced to LIMIT_FALLBACK (200) when the first page returns empty — note that some Nova Poshta endpoints can return empty results for large page sizes so we retry with a smaller limit by resetting page to 1 and setting firstPageAttempted; reference the LIMIT_PRIMARY, LIMIT_FALLBACK, firstPageAttempted flag and the page reset behavior so the intent is clear to future readers.
419-433: Hardcoded error code may be fragile.The fallback to
searchSettlementsByCityNameis triggered only by the exact error code'20000900746'. While the comment explains this is based on observed behavior, hardcoded error codes can be brittle if Nova Poshta changes their API responses.Consider:
- Adding a constant with a descriptive name for documentation
- Logging when the fallback is triggered for observability
🔧 Suggested improvement
+const NP_CITIES_NOT_FOUND_ERROR_CODE = '20000900746'; + export async function searchSettlements(args: { q: string; page?: number; limit?: number; }): Promise<NovaPoshtaSettlement[]> { try { return await getCitiesByFindByString(args); } catch (err) { - // precise fallback for the exact NP error we saw in debug: - if (err instanceof NovaPoshtaApiError && err.code === '20000900746') { + // Fallback for NP error when getCities finds no matches + if (err instanceof NovaPoshtaApiError && err.code === NP_CITIES_NOT_FOUND_ERROR_CODE) { return await searchSettlementsByCityName(args); } throw err; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/services/shop/shipping/nova-poshta-client.ts` around lines 419 - 433, Replace the hardcoded Nova Poshta error code in searchSettlements with a named constant (e.g. NOVA_POSHTA_FALLBACK_ERROR_CODE = '20000900746') and use that constant in the err.code comparison inside the catch block of searchSettlements; additionally, add an observability log statement (using the existing logger in this module—e.g., processLogger or the module's logger) right before calling searchSettlementsByCityName to record that the fallback path was triggered and include the error details and args.q for context. Ensure the constant is declared near related functions and exported/typed appropriately if used elsewhere, and keep the matching logic to err instanceof NovaPoshtaApiError && err.code === NOVA_POSHTA_FALLBACK_ERROR_CODE.frontend/app/api/shop/internal/shipping/np/sync/route.ts (1)
107-120: Consider defaulting totruewhen both environment variables are unset.When both
APP_ENVandVERCEL_ENVare empty/unset, the function returnsfalse, which treats the environment as production. This is a safe default (fail-closed), but the namingisNonProductionAppStagemight be misleading since an unconfigured environment would be considered "production" by this logic.If this is intentional for safety, a brief comment would clarify the design choice.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/api/shop/internal/shipping/np/sync/route.ts` around lines 107 - 120, The current isNonProductionAppStage function treats an unset APP_ENV and VERCEL_ENV as production (returns false); change it to default to non-production (return true) when both environment variables are empty/unset by checking APP_ENV and VERCEL_ENV (process.env.APP_ENV and process.env.VERCEL_ENV) after trimming/lowering and returning true if both are falsy, otherwise keep the existing logic (APP_ENV !== 'production' && APP_ENV !== 'prod' or VERCEL_ENV !== 'production'); update the function implementation and add a short comment explaining the deliberate default-to-non-production behavior.frontend/app/api/shop/webhooks/stripe/route.ts (1)
298-329: Consider omitting wallet attribution when type is null.Currently
buildStripeWalletAttributionalways returns an object, addingwallet: { provider: 'stripe', type: null, source: 'event' }to metadata even for non-wallet payments. This adds noise to stored data.♻️ Optional: Return null when no wallet detected
function buildStripeWalletAttribution(args: { paymentIntent?: Stripe.PaymentIntent; charge?: Stripe.Charge; -}): StripeWalletAttribution { - return { - provider: 'stripe', - type: resolveStripeWalletType(args.paymentIntent, args.charge), - source: 'event', - }; +}): StripeWalletAttribution | null { + const type = resolveStripeWalletType(args.paymentIntent, args.charge); + if (!type) return null; + return { provider: 'stripe', type, source: 'event' }; }Then at call sites, conditionally include:
extra: { ...(walletAttribution && { wallet: walletAttribution }), // ... }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/api/shop/webhooks/stripe/route.ts` around lines 298 - 329, buildStripeWalletAttribution currently always returns a StripeWalletAttribution object even when resolveStripeWalletType returns null, causing wallet: {type: null} to be stored; change buildStripeWalletAttribution to return StripeWalletAttribution | null (call resolveStripeWalletType and if it returns null, return null), update its signature and any callers to conditionally include the wallet field (e.g., ...(walletAttribution && { wallet: walletAttribution })) so wallet metadata is omitted for non-wallet payments; reference resolveStripeWalletType and buildStripeWalletAttribution when making these changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/app/`[locale]/shop/cart/CartPageClient.tsx:
- Around line 576-580: The code currently treats non-OK responses and
parsed.available === false as "no results" by calling setCityOptions([]);
instead introduce and set a separate lookup-failure state (e.g.,
cityLookupFailed or cityLookupError) when response.ok is false or
parsed.available === false, avoid clearing cityOptions on transient failures,
and update the UI logic that decides "noResults" to only use an empty
cityOptions when the lookup succeeded; apply the same change for the duplicate
logic block referenced around the other occurrence (lines 1449-1456) so both the
setCityOptions([])/parsed.available checks and the UI noResults condition are
aligned.
In
`@frontend/app/`[locale]/shop/checkout/payment/monobank/MonobankGooglePayClient.tsx:
- Around line 370-385: The current branch ignores the API's returnUrl and sends
all non-redirect responses to goToPending(), which loses the minted token and
hides hard submit errors; update the logic in MonobankGooglePayClient (the block
that builds data, redirectUrl and checks response.ok) to: 1) prefer and navigate
to data.returnUrl (if typeof data?.returnUrl === 'string' and non-empty) before
falling back to redirectUrl; 2) only call goToPending() for true pending flows
when the API indicates pending (or when a pending returnUrl is present and
used); and 3) do not route non-ok responses (response.ok === false) to
goToPending—surface an error (throw or navigate to your error handler) so
4xx/5xx failures are not shown as “processing payment.” Ensure you reference and
update the variables/data checks (data, redirectUrl, returnUrl) and the calls to
window.location.assign(...) and goToPending() accordingly.
In
`@frontend/app/`[locale]/shop/checkout/return/monobank/MonobankReturnStatus.tsx:
- Around line 41-46: TERMINAL_NON_PAID currently omits the 'requires_payment'
status causing the return page to poll indefinitely; update the
TERMINAL_NON_PAID Set in MonobankReturnStatus.tsx to include 'requires_payment'
and make the identical change to the other equivalent terminal-status set used
later in the file (the second definition around the return-status handling) so
that orders requiring payment are treated as terminal and trigger the checkout
error/retry flow.
In `@frontend/drizzle/meta/_journal.json`:
- Around line 208-220: The migrations run in the wrong order: tag
"0030_shop_np_warehouses_city_ref_fk_backfill" (data cleanup) is scheduled after
tag "0029_shop_np_warehouses_city_ref_fk" (adds the FK), which causes 0029 to
fail on drifted DBs. Fix by either folding the cleanup SQL into the start of the
"0029_shop_np_warehouses_city_ref_fk" migration (run the orphaned
np_warehouses.city_ref cleanup statements immediately before the ALTER TABLE/add
constraint in migration 0029) or regenerate/reorder the journal so
"0030_shop_np_warehouses_city_ref_fk_backfill" runs before
"0029_shop_np_warehouses_city_ref_fk" (ensure the backfill/DELETE/UPDATE runs
first, then the FK creation). Ensure the migration that adds the FK references
the cleaned state and include idempotent checks in the cleanup to be safe on
repeated runs.
In `@frontend/lib/services/orders/checkout.ts`:
- Around line 1007-1017: existingMethod can default to the provider instead of
using a stored choice in pspMetadata, causing wrong hashing; modify the
existingMethod resolution so that after calling
normalizeStoredPaymentMethod(row.pspPaymentMethod) you also check
row.pspMetadata?.checkout?.requestedMethod (parse/use the stored
checkout.requestedMethod value) and normalize that before falling back to
resolveDefaultMethodForProvider; update the logic around existingMethod (and the
same pattern in the later block 1019-1050) to prefer the stored
pspMetadata.checkout.requestedMethod when present, using the same normalization
routine as normalizeStoredPaymentMethod.
In `@frontend/lib/tests/shop/checkout-shipping-phase3.test.ts`:
- Around line 15-16: The test must assert the specific error class instead of a
generic Error: update the failing assertions that only check `.code` to assert
the thrown error is an instance of InvalidPayloadError (e.g.
expect(err).toBeInstanceOf(InvalidPayloadError)); add or update the import to
bring in InvalidPayloadError if missing, and apply the same change for the
second occurrence mentioned (lines ~159-163) so createOrderWithItems path and
any API handler-specific instanceof InvalidPayloadError handling remain covered.
In `@frontend/lib/tests/shop/monobank-janitor-job1.test.ts`:
- Around line 289-292: The current assertions check that getInvoiceStatusMock
was called at least twice overall and that invoiceId appeared at least once;
tighten this to assert the mock was invoked for this specific invoiceId at least
twice by counting calls where call[0] === invoiceId and expecting that count to
be >= 2 (update the assertions around getInvoiceStatusMock and invoiceId to
compute and assert on that per-invoice call count).
---
Outside diff comments:
In `@frontend/app/`[locale]/shop/cart/CartPageClient.tsx:
- Around line 1523-1532: The click handler selects a warehouse but leaves
warehouseOptions populated so the suggestions dropdown remains open; update the
handler (alongside clearCheckoutUiErrors, setSelectedWarehouseRef,
setSelectedWarehouseName, setWarehouseQuery) to collapse the suggestions by
clearing the options and/or hiding the list—e.g. call setWarehouseOptions([])
and/or setShowWarehouseOptions(false) after setting the selected values so the
dropdown closes and subsequent fetches won't keep it open.
In `@frontend/lib/services/orders/checkout.ts`:
- Around line 875-882: The logic computing paymentProvider and paymentsEnabled
wrongly treats requestedProvider === 'none' as fallthrough to Stripe; update the
selection so paymentProvider is exactly requestedProvider when it is one of
'monobank'|'stripe'|'none' (or explicitly check for 'none') and only choose
'stripe' as a fallback when requestedProvider is not provided and
stripePaymentsEnabled is true; also compute paymentsEnabled as paymentProvider
!== 'none' && (paymentProvider === 'monobank' || stripePaymentsEnabled) so an
explicit requestedProvider === 'none' produces paymentProvider === 'none' and
paymentsEnabled === false.
---
Duplicate comments:
In `@frontend/lib/psp/monobank.ts`:
- Around line 342-366: Trim and normalize the wallet payload before returning:
compute and use a trimmed cardToken (e.g., const cardToken =
args.cardToken.trim()) instead of sending the untrimmed args.cardToken, and
tighten currency validation to only accept UAH (require args.ccy === 980 or
compare against a MONOBANK_UAH constant) rather than any positive integer; keep
the existing amountMinor and URL checks but ensure redirectUrl and webHookUrl
are derived from their trimmed values when building the returned object (use
cardToken, amount: args.amountMinor, ccy: 980, initiationKind: 'client',
redirectUrl, webHookUrl).
---
Nitpick comments:
In `@frontend/app/api/shop/internal/shipping/np/sync/route.ts`:
- Around line 107-120: The current isNonProductionAppStage function treats an
unset APP_ENV and VERCEL_ENV as production (returns false); change it to default
to non-production (return true) when both environment variables are empty/unset
by checking APP_ENV and VERCEL_ENV (process.env.APP_ENV and
process.env.VERCEL_ENV) after trimming/lowering and returning true if both are
falsy, otherwise keep the existing logic (APP_ENV !== 'production' && APP_ENV
!== 'prod' or VERCEL_ENV !== 'production'); update the function implementation
and add a short comment explaining the deliberate default-to-non-production
behavior.
In `@frontend/app/api/shop/webhooks/stripe/route.ts`:
- Around line 298-329: buildStripeWalletAttribution currently always returns a
StripeWalletAttribution object even when resolveStripeWalletType returns null,
causing wallet: {type: null} to be stored; change buildStripeWalletAttribution
to return StripeWalletAttribution | null (call resolveStripeWalletType and if it
returns null, return null), update its signature and any callers to
conditionally include the wallet field (e.g., ...(walletAttribution && { wallet:
walletAttribution })) so wallet metadata is omitted for non-wallet payments;
reference resolveStripeWalletType and buildStripeWalletAttribution when making
these changes.
In `@frontend/lib/services/shop/shipping/nova-poshta-client.ts`:
- Around line 434-482: Add a short clarifying comment above the first-page
fallback logic in getWarehousesByCityRef explaining why LIMIT_PRIMARY (500) is
reduced to LIMIT_FALLBACK (200) when the first page returns empty — note that
some Nova Poshta endpoints can return empty results for large page sizes so we
retry with a smaller limit by resetting page to 1 and setting
firstPageAttempted; reference the LIMIT_PRIMARY, LIMIT_FALLBACK,
firstPageAttempted flag and the page reset behavior so the intent is clear to
future readers.
- Around line 419-433: Replace the hardcoded Nova Poshta error code in
searchSettlements with a named constant (e.g. NOVA_POSHTA_FALLBACK_ERROR_CODE =
'20000900746') and use that constant in the err.code comparison inside the catch
block of searchSettlements; additionally, add an observability log statement
(using the existing logger in this module—e.g., processLogger or the module's
logger) right before calling searchSettlementsByCityName to record that the
fallback path was triggered and include the error details and args.q for
context. Ensure the constant is declared near related functions and
exported/typed appropriately if used elsewhere, and keep the matching logic to
err instanceof NovaPoshtaApiError && err.code ===
NOVA_POSHTA_FALLBACK_ERROR_CODE.
In `@frontend/lib/tests/shop/monobank-api-methods.test.ts`:
- Around line 304-311: The test is passing initiationKind explicitly which masks
enforcement in walletPayment; remove the initiationKind property from the test
payload in monobank-api-methods.test.ts so
walletPayment/buildMonobankWalletPayload must supply initiationKind itself (see
walletPayment and buildMonobankWalletPayload), then run the test to confirm
behavior still passes and that initiationKind is set by the code under test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5eece10d-5566-4aed-b3cf-751ea140e27d
📒 Files selected for processing (26)
frontend/app/[locale]/shop/cart/CartPageClient.tsxfrontend/app/[locale]/shop/checkout/payment/monobank/MonobankGooglePayClient.tsxfrontend/app/[locale]/shop/checkout/return/monobank/MonobankReturnStatus.tsxfrontend/app/api/shop/internal/shipping/np/sync/route.tsfrontend/app/api/shop/orders/[id]/payment/monobank/google-pay/submit/route.tsfrontend/app/api/shop/shipping/np/warehouses/route.tsfrontend/app/api/shop/webhooks/stripe/route.tsfrontend/drizzle/0030_shop_np_warehouses_city_ref_fk_backfill.sqlfrontend/drizzle/meta/0030_snapshot.jsonfrontend/drizzle/meta/_journal.jsonfrontend/lib/psp/monobank.tsfrontend/lib/services/orders/checkout.tsfrontend/lib/services/orders/monobank-webhook.tsfrontend/lib/services/shop/shipping/nova-poshta-catalog.tsfrontend/lib/services/shop/shipping/nova-poshta-client.tsfrontend/lib/tests/shop/checkout-shipping-phase3.test.tsfrontend/lib/tests/shop/monobank-api-methods.test.tsfrontend/lib/tests/shop/monobank-google-pay-submit-route.test.tsfrontend/lib/tests/shop/monobank-janitor-job1.test.tsfrontend/lib/tests/shop/nova-poshta-client-network-failure.test.tsfrontend/lib/tests/shop/order-items-snapshot-immutable.test.tsfrontend/lib/tests/shop/shipping-np-warehouses-route-p2.test.tsfrontend/lib/tests/shop/stripe-webhook-refund-full.test.tsfrontend/messages/en.jsonfrontend/messages/pl.jsonfrontend/messages/uk.json
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/lib/tests/shop/monobank-google-pay-submit-route.test.ts
- frontend/lib/services/orders/monobank-webhook.ts
frontend/app/[locale]/shop/checkout/payment/monobank/MonobankGooglePayClient.tsx
Outdated
Show resolved
Hide resolved
Comment on lines
+41
to
+46
| const TERMINAL_NON_PAID = new Set([ | ||
| 'failed', | ||
| 'refunded', | ||
| 'canceled', | ||
| 'needs_review', | ||
| ]); |
Contributor
There was a problem hiding this comment.
Treat requires_payment as terminal here.
payment_status includes requires_payment, but this set omits it. When the order falls back to a retry-required state, the return page keeps polling forever instead of sending the user to the checkout error/retry flow.
💡 Suggested fix
const TERMINAL_NON_PAID = new Set([
'failed',
'refunded',
+ 'requires_payment',
'canceled',
'needs_review',
]);Also applies to: 157-164
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/app/`[locale]/shop/checkout/return/monobank/MonobankReturnStatus.tsx
around lines 41 - 46, TERMINAL_NON_PAID currently omits the 'requires_payment'
status causing the return page to poll indefinitely; update the
TERMINAL_NON_PAID Set in MonobankReturnStatus.tsx to include 'requires_payment'
and make the identical change to the other equivalent terminal-status set used
later in the file (the second definition around the return-status handling) so
that orders requiring payment are treated as terminal and trigger the checkout
error/retry flow.
Comment on lines
+208
to
+220
| { | ||
| "idx": 29, | ||
| "version": "7", | ||
| "when": 1772756436792, | ||
| "tag": "0029_shop_np_warehouses_city_ref_fk", | ||
| "breakpoints": true | ||
| }, | ||
| { | ||
| "idx": 30, | ||
| "version": "7", | ||
| "when": 1772764398365, | ||
| "tag": "0030_shop_np_warehouses_city_ref_fk_backfill", | ||
| "breakpoints": true |
Contributor
There was a problem hiding this comment.
Backfill runs too late to save broken environments.
This journal schedules 0030_shop_np_warehouses_city_ref_fk_backfill after 0029_shop_np_warehouses_city_ref_fk, but frontend/drizzle/0029_shop_np_warehouses_city_ref_fk.sql:1-16 already tries to add the FK before orphaned np_warehouses.city_ref rows are cleaned up. On any database with drifted data, migration 0029 can fail and block 0030 from ever running. Please fold the cleanup into 0029 or regenerate the sequence so the data repair runs before the FK is created.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/drizzle/meta/_journal.json` around lines 208 - 220, The migrations
run in the wrong order: tag "0030_shop_np_warehouses_city_ref_fk_backfill" (data
cleanup) is scheduled after tag "0029_shop_np_warehouses_city_ref_fk" (adds the
FK), which causes 0029 to fail on drifted DBs. Fix by either folding the cleanup
SQL into the start of the "0029_shop_np_warehouses_city_ref_fk" migration (run
the orphaned np_warehouses.city_ref cleanup statements immediately before the
ALTER TABLE/add constraint in migration 0029) or regenerate/reorder the journal
so "0030_shop_np_warehouses_city_ref_fk_backfill" runs before
"0029_shop_np_warehouses_city_ref_fk" (ensure the backfill/DELETE/UPDATE runs
first, then the FK creation). Ensure the migration that adds the FK references
the cleaned state and include idempotent checks in the cleanup to be safe on
repeated runs.
Comment on lines
+15
to
+16
| import { IdempotencyConflictError } from '@/lib/services/errors'; | ||
| import { createOrderWithItems } from '@/lib/services/orders'; |
Contributor
There was a problem hiding this comment.
Keep the typed error assertion here.
This test now accepts any Error with the right .code, but createOrderWithItems still throws InvalidPayloadError on this path, and the API handler still has special instanceof InvalidPayloadError handling. A regression to a plain Error would now pass unnoticed.
Suggested fix
-import { IdempotencyConflictError } from '@/lib/services/errors';
+import {
+ IdempotencyConflictError,
+ InvalidPayloadError,
+} from '@/lib/services/errors';
…
- await expect(promise).rejects.toBeInstanceOf(Error);
+ await expect(promise).rejects.toBeInstanceOf(InvalidPayloadError);
await expect(promise).rejects.toHaveProperty(
'code',
'SHIPPING_CURRENCY_UNSUPPORTED'
);Also applies to: 159-163
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/lib/tests/shop/checkout-shipping-phase3.test.ts` around lines 15 -
16, The test must assert the specific error class instead of a generic Error:
update the failing assertions that only check `.code` to assert the thrown error
is an instance of InvalidPayloadError (e.g.
expect(err).toBeInstanceOf(InvalidPayloadError)); add or update the import to
bring in InvalidPayloadError if missing, and apply the same change for the
second occurrence mentioned (lines ~159-163) so createOrderWithItems path and
any API handler-specific instanceof InvalidPayloadError handling remain covered.
… and Nova Poshta flows
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
frontend/app/[locale]/shop/checkout/payment/monobank/MonobankGooglePayClient.tsx (1)
347-352: Consider adding user feedback when token extraction fails.When
gTokenis null after Google Pay returns data, the user is silently redirected to pending. While this may be intentional (letting webhook finalize), a brief UI message before redirect could improve UX.💡 Optional: Add transient message before pending redirect
const gToken = extractGToken(paymentData); if (!gToken) { submitIdempotencyKeyRef.current = null; + setUiMessage(t('monobankGooglePay.processingFallback')); goToPending(); return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/`[locale]/shop/checkout/payment/monobank/MonobankGooglePayClient.tsx around lines 347 - 352, When extractGToken(paymentData) returns null you currently reset submitIdempotencyKeyRef.current and call goToPending() silently; change this so you display a brief user-facing message (e.g., via existing showToast/setNotification or a local state like setErrorMessage) indicating token extraction failed and the order is being finalized, then clear submitIdempotencyKeyRef.current and call goToPending() after showing the message (either immediately if using a non-blocking toast or after a short timeout/acknowledgement if using an inline banner). Locate the null-check around extractGToken, update the branch to invoke the app’s notification API or set component state before calling goToPending, and ensure existing behavior of resetting submitIdempotencyKeyRef.current is preserved.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/app/`[locale]/shop/cart/CartPageClient.tsx:
- Around line 98-103: The client-side resolveDefaultMethodForProvider currently
ignores currency and always returns 'monobank_invoice' for Monobank; update the
function signature to accept a currency parameter and implement the same logic
as the server: if provider === 'stripe' return 'stripe_card'; if provider ===
'monobank' return 'monobank_invoice' only when currency === 'UAH' otherwise
return null; ensure callers of resolveDefaultMethodForProvider are updated to
pass the current currency so the client-side selection matches the server-side
resolveDefaultMethodForProvider.
In
`@frontend/app/`[locale]/shop/checkout/payment/monobank/MonobankGooglePayClient.tsx:
- Around line 400-410: The check for a canceled Google Pay flow unnecessarily
calls .toUpperCase() on statusCode; in the catch block inside
MonobankGooglePayClient (where submitIdempotencyKeyRef.current is cleared and
setUiMessage(t('monobankGooglePay.cancelled')) is called), change the condition
to compare statusCode directly to 'CANCELED' (i.e., replace
statusCode.toUpperCase() === 'CANCELED' with statusCode === 'CANCELED') while
keeping the existing defensive extraction of statusCode intact.
---
Nitpick comments:
In
`@frontend/app/`[locale]/shop/checkout/payment/monobank/MonobankGooglePayClient.tsx:
- Around line 347-352: When extractGToken(paymentData) returns null you
currently reset submitIdempotencyKeyRef.current and call goToPending() silently;
change this so you display a brief user-facing message (e.g., via existing
showToast/setNotification or a local state like setErrorMessage) indicating
token extraction failed and the order is being finalized, then clear
submitIdempotencyKeyRef.current and call goToPending() after showing the message
(either immediately if using a non-blocking toast or after a short
timeout/acknowledgement if using an inline banner). Locate the null-check around
extractGToken, update the branch to invoke the app’s notification API or set
component state before calling goToPending, and ensure existing behavior of
resetting submitIdempotencyKeyRef.current is preserved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: df435652-4215-4669-8cc4-35a8a0ec43dc
📒 Files selected for processing (11)
frontend/app/[locale]/shop/cart/CartPageClient.tsxfrontend/app/[locale]/shop/checkout/payment/monobank/MonobankGooglePayClient.tsxfrontend/app/[locale]/shop/checkout/return/monobank/MonobankReturnStatus.tsxfrontend/app/api/shop/internal/shipping/np/sync/route.tsfrontend/app/api/shop/webhooks/stripe/route.tsfrontend/lib/psp/monobank.tsfrontend/lib/services/orders/checkout.tsfrontend/lib/services/shop/shipping/nova-poshta-client.tsfrontend/lib/tests/shop/checkout-shipping-phase3.test.tsfrontend/lib/tests/shop/monobank-api-methods.test.tsfrontend/lib/tests/shop/monobank-janitor-job1.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- frontend/lib/tests/shop/monobank-janitor-job1.test.ts
- frontend/lib/tests/shop/monobank-api-methods.test.ts
- frontend/app/[locale]/shop/checkout/return/monobank/MonobankReturnStatus.tsx
- frontend/app/api/shop/webhooks/stripe/route.ts
frontend/app/[locale]/shop/checkout/payment/monobank/MonobankGooglePayClient.tsx
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
frontend/app/[locale]/shop/cart/CartPageClient.tsx (1)
78-107: Consider importing types and helper frompayments.tsto reduce duplication.
CheckoutPaymentMethodandCheckoutProviderduplicatePaymentMethodandPaymentProviderfromfrontend/lib/shop/payments.ts. Similarly,resolveDefaultMethodForProviderreimplements the same logic exported there (context snippet 2). Importing these would reduce drift risk and keep a single source of truth.♻️ Suggested refactor
+import { + type PaymentMethod, + type PaymentProvider, + resolveDefaultMethodForProvider, +} from '@/lib/shop/payments'; + -type CheckoutProvider = 'stripe' | 'monobank'; -type CheckoutPaymentMethod = - | 'stripe_card' - | 'monobank_invoice' - | 'monobank_google_pay'; +type CheckoutProvider = Exclude<PaymentProvider, 'none'>; +type CheckoutPaymentMethod = PaymentMethod; -function resolveDefaultMethodForProvider(args: { - provider: CheckoutProvider; - currency: string | null | undefined; -}): CheckoutPaymentMethod | null { - if (args.provider === 'stripe') return 'stripe_card'; - if (args.provider === 'monobank') { - return args.currency === 'UAH' ? 'monobank_invoice' : null; - } - return null; -}Then update call sites to use positional arguments:
resolveDefaultMethodForProvider(provider, currency)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/`[locale]/shop/cart/CartPageClient.tsx around lines 78 - 107, This file duplicates types and logic: replace local CheckoutProvider and CheckoutPaymentMethod and the resolveDefaultMethodForProvider implementation by importing the canonical PaymentProvider, PaymentMethod types and the resolveDefaultMethodForProvider (or equivalent) helper from frontend/lib/shop/payments.ts; remove the local resolveDefaultMethodForProvider and Checkout* type declarations, update resolveInitialProvider (if still needed) to use the imported PaymentProvider type, and update all call sites in this module to call the imported helper with the positional arguments (provider, currency) as suggested so the single source of truth in payments.ts is used.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@frontend/app/`[locale]/shop/cart/CartPageClient.tsx:
- Around line 78-107: This file duplicates types and logic: replace local
CheckoutProvider and CheckoutPaymentMethod and the
resolveDefaultMethodForProvider implementation by importing the canonical
PaymentProvider, PaymentMethod types and the resolveDefaultMethodForProvider (or
equivalent) helper from frontend/lib/shop/payments.ts; remove the local
resolveDefaultMethodForProvider and Checkout* type declarations, update
resolveInitialProvider (if still needed) to use the imported PaymentProvider
type, and update all call sites in this module to call the imported helper with
the positional arguments (provider, currency) as suggested so the single source
of truth in payments.ts is used.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5fef13d9-5efb-44b4-babe-b4a7393f88f7
📒 Files selected for processing (5)
frontend/app/[locale]/shop/cart/CartPageClient.tsxfrontend/app/[locale]/shop/checkout/payment/monobank/MonobankGooglePayClient.tsxfrontend/messages/en.jsonfrontend/messages/pl.jsonfrontend/messages/uk.json
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/app/[locale]/shop/checkout/payment/monobank/MonobankGooglePayClient.tsx
ViktorSvertoka
approved these changes
Mar 6, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Wallets + shipping hardening for the Shop module.
This PR adds Stripe wallet support via Payment Element (Apple Pay / Google Pay eligibility surfaces), introduces a Monobank Google Pay payment rail (with invoice fallback and webhook-authoritative finalization), and fixes Nova Poshta schema drift to ensure warehouses reference cities via
city_ref(restoring correct lookup/caching behavior).Related Issue
Issue: #<issue_number>
Changes
cardrail for deterministic wallet surfacing, add wallet attribution, and ensure webhook processing remains deterministic (no extra Stripe fetch).settlement_refFK withcity_refFK for consistent city/warehouse linking.Database Changes (if applicable)
How Has This Been Tested?
Commands (PowerShell):
Screenshots (if applicable)
Checklist
Before submitting
Reviewers
Summary by CodeRabbit
New Features
Improvements