Merged
Conversation
…istence + async label workflow (#364)
…tics cleanup (#367) * perf(vercel): cut runtime costs via notification, blog cache, and analytics changes * perf(blog): remove server searchParams usage to preserve ISR
… to client-side (#370) * refactor(frontend): remove locale layout dynamic auth and move header auth client-side * fix(frontend): prevent stale auth responses in useAuth and remove redundant dashboard dynamic layout
…abs (#372) * refactor(frontend): remove locale layout dynamic auth and move header auth client-side * fix(frontend): prevent stale auth responses in useAuth and remove redundant dashboard dynamic layout * feat(frontend): sync auth state across tabs via BroadcastChannel
…tars cache (#371) * perf(quiz-flow): move quiz progress to client-side fetch, enable ISR for quizzes page - Move user progress fetch from SSR to client-side API (/api/quiz/progress) - Remove force-dynamic and getCurrentUser() from quizzes page - Add revalidate=300 for ISR caching - Use window.history.replaceState for tab URL sync (avoid Next.js navigation) - Add forceMount to TabsContent to prevent layout shift on tab switch - Fix nested <main> — use <section> inside DynamicGridBackground - Cache GitHub stars count in sessionStorage to avoid refetch + re-animation * perf: replace useRef with useState lazy initializer in GitHubStarButton Fixes React 19 react-hooks/refs ESLint error — useRef.current cannot be read during render. Uses useState(getStoredStars) to capture the sessionStorage value once on mount instead. * fix: stop star icon trembling on hover in GitHubStarButton
…zes page (#373) * perf(quiz-flow): move quiz progress to client-side fetch, enable ISR for quizzes page - Move user progress fetch from SSR to client-side API (/api/quiz/progress) - Remove force-dynamic and getCurrentUser() from quizzes page - Add revalidate=300 for ISR caching - Use window.history.replaceState for tab URL sync (avoid Next.js navigation) - Add forceMount to TabsContent to prevent layout shift on tab switch - Fix nested <main> — use <section> inside DynamicGridBackground - Cache GitHub stars count in sessionStorage to avoid refetch + re-animation * perf: replace useRef with useState lazy initializer in GitHubStarButton Fixes React 19 react-hooks/refs ESLint error — useRef.current cannot be read during render. Uses useState(getStoredStars) to capture the sessionStorage value once on mount instead. * fix: stop star icon trembling on hover in GitHubStarButton * fix: eliminate quiz timer flash on language switch Remove Suspense boundary (loading.tsx) that unmounted QuizContainer during locale navigation. Synchronous session restore via useReducer lazy initializer and correct timer initialization via useState lazy initializer prevent any visible state reset on language switch * fix: replace quiz card layout shift with skeleton grid during progress load
…out polling + add sweep indexes (#375) * (SP: 3) [Backend] add internal janitor (jobs 1-4), claim/lease + runbook (G0-G6) * (SP: 3) [Backend] add provider selector, fix payments gating, i18n checkout errors * Add shop category images to public * (SP: 3) [Shop][Monobank] I1 structured logging: codes + logging safety checks * (SP: 3) [Shop][Monobank] Fail-closed non-browser origin posture for webhook + janitor (ORIGIN_BLOCKED) * (SP: 3) [Shop][Monobank] [Shop][Monobank] J gate: add orders status ownership test and pass all pre-prod invariants * (SP: 3) [Shop][Monobank] review fixes (tests, logging, success UI) * (SP: 1) [Shop][Monobank] Tighten webhook log-code typing; harden DB tests; minor security/log/UI cleanups * (SP: 1) [Shop][Monobank] harden Monobank webhook (origin/PII-safe logs) and remove duplicate sha256 hashing * (SP: 1) [Cart] adding route for user orders to cart page * (SP: 1) [Cart] fix after review cart mpage and adding index for orders * (SP: 1) [Cart] Fix cart orders summary auth rendering and return totalCount for orders badge * (SP: 1) [Cart] remove console.warn from CartPageClient to satisfy monobank logging safety invariant, namespace localStorage cart by user and reset on auth change * (SP: 1) [Cart] rehydrate per cartOwnerId (remove didHydrate coupling) * (SP: 2)[Backend] shop/shipping schema migrations foundation * (SP: 2)[Backend] shop/shipping public routes + np cache + sync * (SP: 2)[Backend] shop/shipping: shipping persistence + currency policy * (SP: 2)[Backend] shop/shipping: webhook apply + psp fields + enqueue shipping * (SP: 2)[Backend] shop/shipping: shipments worker + internal run + np mock * (SP: 2)[Backend] shop/shipping: admin+ui shipping actions * (SP: 2)[Backend] shop/shipping: retention + log sanitizer + metrics * (SP: 1)[Backend] stabilize Monobank janitor (job1/job3) and fix failing apply-outcomes tests * (SP: 1) [db]: add shop shipping core migration * (SP: 1) [FIX] resolve merge artifacts in order details page * (SP: 1) [FIX] apply post-review fixes for shipping and admin flows * (SP: 1) [FIX] align cart shipping imports (localeToCountry + availability reason code) * (SP: 1) [FIX] hard-block checkout when shipping disabled + i18n reason mapping * (SP: 1) [FIX] harden webhook enqueue + shipping worker + NP catalog + cart fail-closed * (SP: 1) [FIX] Initialize shippingMethodsLoading to true to avoid premature checkout. * (SP: 1) [FIX] migration 17 * (SP: 1) [DB] migrarion to testind DB and adjusting tests * (SP: 1)[DB] slow down restock janitor + enforce prod interval floor * (SP: 1) [DB] add order status lite view (opt-in) + instrumentation * (SP: 1) [DB] replace checkout success router.refresh polling with backoff API polling * (SP: 1) [DB] throttle sessions activity heartbeat + use count(*) (PK invariant) * (SP: 1)[DB] enforce production min intervals for internal shipping jobs * (SP: 1) [DB] add minimal partial indexes for orders sweeps + rollout notes * (SP: 1) [DB] refactor sweep claim step to FOR UPDATE SKIP LOCKED batching * (SP: 1)[DB]: slow janitor schedule to every 30 minutes * (SP: 1)[DB] increase polling delays for MonobankRedirectStatus * (SP: 1)[FIX] harden webhooks + fix SSR hydration + janitor/np gates + sweeps refactor * (SP: 1)[FIX] harden shipping enqueue gating + apply NP interval floor
… notifications, consent, returns) (#378) * (SP:3)[SHOP] add canonical payment/shipping/admin audit tables + dedupe helper with flagged atomic dual-write * (SP: 2)[SHOP] add INTL quote flow (request/offer/accept/decline), payment-init gate, and quote expiry/timeout sweeps * (SP: 3)[SHOP] introduce outbox-driven notifications with projector + worker (phase 3) * (SP: 3)[SHOP] add minimal returns/RMA lifecycle with atomic audit + canonical events (phase 4) * (SP: 3)[SHOP] enforce guest status-token lite-only access and audit token usage (phase 5) * (SP: 3)[SHOP] centralize transition guards and enforce across admin/webhook/worker flows (phase 6) :wq n * (SP:3)[SHOP]: enforce DATABASE_URL_LOCAL preflight + deterministic vitest config * (SP:3)[SHOP]: require canonical events in prod (fail-fast) * (SP:3)[SHOP]: implement notifications transport with retries + DLQ * (SP:3)[SHOP]: persist checkout legal consent artifact * (SP:1)[SHOP] add migration 0025 for consent + events + audit + prices * (SP:1)[SHOP] emit shipping_events for shipment worker transitions * (SP:2)[SHOP] audit admin product mutations (deduped) * (SP:2)[SHOP] make Monobank webhook retryable on transient apply failures * (SP:3) [SHOP] enforce guest status-token scopes across order actions * (SP:1) [SHOP] make product_prices the only write authority * (SP:1) [SHOP] add Playwright e2e smoke suite (local DB only) * (SP:3) [SHOP] explicitly reject exchanges (EXCHANGES_NOT_SUPPORTED) * (SP: 3)[FIX] harden audit + workers/tests; fix transitions/restock + webhook perf * (SP: 3)[FIX] harden local-db test safety and tighten shop reliability guards * (SP: 3)[FIX] fail-closed admin audit, refine shipments-worker outcomes/metrics, tighten quote+tests * (SP: 1)[FIX] harden shipments-worker claiming/leases and make audit+quote/test paths resilient * (SP: 1)[FIX] harden quote request errors/logging and sanitize requestId; document best-effort delete audit
ViktorSvertoka
added
documentation
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Contributor
📝 WalkthroughWalkthroughIntroduces Shop 1.0.6 with production-readiness features: legal consent tracking for checkout, end-to-end international quote workflow (request→offer→accept→decline/expire), admin audit logging system, notification outbox infrastructure with worker/projector, returns management with refund/inventory handling, shipping event tracking with deduplication, and enhanced status-token scopes. Adds 8 database migrations, 15+ new API routes, comprehensive validation schemas, and 30+ service-layer functions. Changes
Sequence Diagram(s)sequenceDiagram
participant Customer
participant API as API Route
participant Service as Quote/Order Service
participant DB as Database
participant Event as Event Writer
Customer->>API: POST /orders/[id]/quote/request
API->>Service: requestIntlQuote(orderId)
Service->>DB: Load order, validate intl constraints
Service->>DB: Update order quote_status → 'requested'
Service->>Event: Write shipping event (quote_requested)
Event->>DB: Insert into shippingEvents (dedupe)
Service-->>API: { orderId, quoteStatus: 'requested', changed }
API-->>Customer: 200 JSON response
Customer->>API: POST /admin/orders/[id]/quote/offer
API->>Service: offerIntlQuote(orderId, version, currency, shippingQuoteMinor, expiresAt)
Service->>DB: Load order, validate status → 'requested'
Service->>DB: Create shipping quote (version, status='offered', expiresAt)
Service->>DB: Update order quote_status → 'offered', quote_version, shipping_quote_minor
Service->>Event: Write shipping event (quote_offered)
Event->>DB: Insert into shippingEvents (dedupe)
Service-->>API: { orderId, version, quoteStatus: 'offered', expiresAt, shippingQuoteMinor }
API-->>Customer: 200 JSON response
Customer->>API: POST /orders/[id]/quote/accept
API->>Service: acceptIntlQuote(orderId, version)
Service->>DB: Load order, validate quote offered & not expired
Service->>DB: Reserve inventory for quote items
Service->>DB: Update order quote_status → 'accepted', quote_accepted_at, quote_payment_deadline_at
Service->>DB: Update shipping quote accepted_at
Service->>Event: Write shipping event (quote_accepted)
Event->>DB: Insert into shippingEvents (dedupe)
Service-->>API: { orderId, version, quoteStatus: 'accepted', paymentDeadlineAt, totalAmountMinor }
API-->>Customer: 200 JSON response
sequenceDiagram
participant Customer
participant API as Notification API
participant Projector as Projector Service
participant Worker as Worker Service
participant Transport as Email Transport
participant DB as Database
Projector->>DB: Query shipping_events with status='quote_requested'
Projector->>Projector: Map eventName → template key 'intl_quote_requested'
Projector->>DB: Build dedupe key, check exists in notification_outbox
Projector->>DB: Insert into notification_outbox (status='pending', nextAttemptAt=now)
DB-->>Projector: Row inserted (or conflict ignored)
Projector-->>API: { scanned, inserted, templateCounts }
API->>Worker: runNotificationOutboxWorker({ limit, leaseSeconds, maxAttempts })
Worker->>DB: Claim batch (status='pending', nextAttemptAt ≤ now), set lease_owner=runId, lease_expires_at
Worker->>DB: Load claimed rows
loop For each claimed row
Worker->>DB: Load order, recipient user
Worker->>Transport: renderTemplate(templateKey, orderId, payload)
Transport-->>Worker: { subject, text, html }
Worker->>Transport: sendShopNotificationEmail({ to, subject, text, html })
alt Success
Transport-->>Worker: { sent: true }
Worker->>DB: Mark sent (status='sent', sentAt=now, attemptCount++)
else Transient Error
Transport-->>Worker: { transient: true, code }
Worker->>DB: Mark failed (status='failed', attemptCount++, nextAttemptAt=now+backoff)
else Dead Letter (maxAttempts exceeded)
Worker->>DB: Mark dead_letter (status='dead_letter', deadLetteredAt=now)
end
end
Worker-->>API: { claimed, processed, sent, retried, deadLettered }
sequenceDiagram
participant Customer
participant API as Return API
participant Service as Returns Service
participant DB as Database
participant Audit as Audit Writer
participant Refund as Stripe Refund
Customer->>API: POST /orders/[id]/returns { idempotencyKey, reason, policyRestock }
API->>Service: createReturnRequest(orderId, idempotencyKey, reason, policyRestock)
Service->>DB: Check idempotency key in return_requests
alt Existing idempotent return
Service->>DB: Load existing return + items
Service-->>API: { created: false, request }
else New return
Service->>DB: Load order + items
Service->>Service: Calculate refund_amount_minor from items
Service->>DB: Insert return_requests row (status='requested')
Service->>DB: Insert return_items rows (linked to return + order items)
Service->>DB: Insert shipping_event (return_requested)
Service->>Audit: Write admin audit (action='return_created', target_id=return_id)
Service-->>API: { created: true, request }
end
API-->>Customer: 201 JSON response
Customer->>API: POST /admin/returns/[id]/approve
API->>Service: approveReturnRequest(returnRequestId, actorUserId)
Service->>DB: Load return, validate status='requested'
Service->>DB: Update return status='approved', approved_at, approved_by
Service->>DB: Insert shipping_event (return_approved)
Service->>Audit: Write admin audit (action='return_approved', actor_user_id=actorUserId)
Service-->>API: { success, returnRequest }
API-->>Customer: 200 JSON
Customer->>API: POST /admin/returns/[id]/refund
API->>Service: refundReturnRequest(returnRequestId, actorUserId)
Service->>DB: Load return, validate status='received'
Service->>DB: Validate refund_amount_minor > 0 and PSP='stripe'
Service->>Refund: createRefund(orderId, amount, returnId)
Refund-->>Service: { refundId, status }
Service->>DB: Update return status='refunded', refunded_at, refunded_by, refund_provider_ref
Service->>DB: Insert shipping_event (return_refunded)
Service->>Audit: Write admin audit (action='return_refunded')
Service-->>API: { success, returnRequest, refund }
API-->>Customer: 200 JSON
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
![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: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
frontend/lib/services/products/mutations/update.ts (1)
149-168:⚠️ Potential issue | 🟠 MajorWrap price upsert and product update in a transaction to ensure atomicity.
The price upsert (lines 149-161) and product update (lines 164-168) are separate database operations. If the product update fails after the price upsert succeeds, the database will be left with updated prices but stale product metadata.
🔧 Proposed fix using a transaction
try { - if (prices.length) { - const upsertRows = prices.map(p => { - ... - }); - - await db - .insert(productPrices) - .values(upsertRows) - .onConflictDoUpdate({ - ... - }); - } - - const [row] = await db - .update(products) - .set(updateData) - .where(eq(products.id, id)) - .returning(); + const [row] = await db.transaction(async (tx) => { + if (prices.length) { + const upsertRows = prices.map(p => { + const priceMinor = p.priceMinor; + const originalMinor = p.originalPriceMinor; + + return { + productId: id, + currency: p.currency, + priceMinor, + originalPriceMinor: originalMinor, + price: toDbMoney(priceMinor), + originalPrice: + originalMinor == null ? null : toDbMoney(originalMinor), + }; + }); + + await tx + .insert(productPrices) + .values(upsertRows) + .onConflictDoUpdate({ + target: [productPrices.productId, productPrices.currency], + set: { + priceMinor: sql`excluded.price_minor`, + originalPriceMinor: sql`excluded.original_price_minor`, + price: sql`excluded.price`, + originalPrice: sql`excluded.original_price`, + updatedAt: sql`now()`, + }, + }); + } + + return tx + .update(products) + .set(updateData) + .where(eq(products.id, id)) + .returning(); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/services/products/mutations/update.ts` around lines 149 - 168, The price upsert (using productPrices and upsertRows) and the subsequent product update (using products, updateData and id) must be executed inside a single DB transaction to ensure atomicity; refactor the code to call db.transaction(async (tx) => { ... }) (or the project's transaction helper) and perform the insert(...).onConflictDoUpdate(...) against tx and the update(...).where(...).returning() against tx, then return the resulting row from inside the transaction, ensuring any error causes the transaction to roll back.frontend/app/api/shop/orders/[id]/status/route.ts (1)
211-220:⚠️ Potential issue | 🔴 CriticalRemove stale
responseModebranch — it references an undefined identifier.Lines 211 and 216 use
responseMode, but onlyrequestedResponseModeandeffectiveResponseModeare defined. The code will fail at runtime with a ReferenceError. Additionally, lines 199–208 already handle the 'lite' case correctly usingeffectiveResponseMode, making lines 211–220 redundant.Suggested patch
- if (responseMode === 'lite') { - const liteOrder = await getOrderStatusLiteSummary(orderId); - logInfo('order_status_responded', { - requestId, - orderId, - responseMode, - durationMs: Date.now() - startedAtMs, - }); - return noStoreJson(liteOrder, { status: 200 }); - }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/api/shop/orders/`[id]/status/route.ts around lines 211 - 220, The extra branch references an undefined responseMode and duplicates the already-handled lite path; remove the entire if (responseMode === 'lite') { ... } block (including the call to getOrderStatusLiteSummary, the logInfo('order_status_responded', ...) and the return noStoreJson(...)) so the code relies on the earlier handling that uses requestedResponseMode / effectiveResponseMode; ensure no other references to responseMode remain and that logging and response use the existing effectiveResponseMode flow.frontend/lib/services/orders/checkout.ts (1)
1086-1097:⚠️ Potential issue | 🔴 CriticalMalformed
try/catchblock:catchincorrectly placed afterifstatementThe
catchblock at line 1097 is syntactically invalid. It appears after the closing brace of theifstatement (line 1096), butcatchblocks can only followtryblocks, notifstatements. This will fail TypeScript parsing.The correct structure should consolidate the exception handling into a single
catchblock at line 1113:Suggested fix
try { await ensureOrderLegalConsentSnapshot({ orderId: created.id, snapshot: preparedLegalConsent.snapshot, }); if (preparedShipping.required && preparedShipping.snapshot) { await ensureOrderShippingSnapshot({ orderId: created.id, snapshot: preparedShipping.snapshot, }); - } catch (e) { - // Neon HTTP: no interactive transactions. Do compensating cleanup. - logError( - `[createOrderWithItems] orderShipping snapshot insert failed orderId=${created.id}`, - e - ); - try { - await db.delete(orders).where(eq(orders.id, created.id)); - } catch (cleanupErr) { - logError( - `[createOrderWithItems] cleanup delete failed orderId=${created.id}`, - cleanupErr - ); - } - throw e; } } catch (e) { // Neon HTTP: no interactive transactions. Do compensating cleanup. logError(🤖 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 1086 - 1097, The try/catch is malformed because the closing brace for the try block ends before the if check, placing catch after an if; wrap both calls (ensureOrderLegalConsentSnapshot and the conditional ensureOrderShippingSnapshot) inside the same try and move the catch to directly follow that try block (remove the stray closing brace before the if), so the catch handles exceptions from ensureOrderLegalConsentSnapshot and ensureOrderShippingSnapshot (references: ensureOrderLegalConsentSnapshot, ensureOrderShippingSnapshot, preparedShipping, created.id).
🧹 Nitpick comments (29)
frontend/lib/services/products/mutations/update.ts (1)
43-43: Consider improving type safety by properly typingProductUpdateInput.The
(input as any)pattern is used repeatedly throughout this function (lines 43, 49, 51, 61, 109-127), bypassing TypeScript's type checking. While this is pre-existing code, strengthening theProductUpdateInputtype to include all expected fields would improve compile-time safety and IDE support.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/services/products/mutations/update.ts` at line 43, Define a proper ProductUpdateInput type that lists all fields accessed in updateProduct (e.g., slug, title, description, price, metadata, images, etc.) with correct optional/nullability and types, update the updateProduct mutation signature to accept ProductUpdateInput instead of a loose any, and then remove all (input as any) casts in the function (references: ProductUpdateInput type, updateProduct function) so the code uses input.slug, input.title, input.description, etc., directly and benefits from compile-time checking.frontend/lib/tests/shop/order-payment-init-token-scope-phase7.test.ts (2)
67-79: Avoidas anyin DB fixture inserts.Line 78 drops schema type-safety and can mask future schema drift. Prefer a typed insert payload.
Proposed refactor
async function insertOrder(orderId: string) { - await db.insert(orders).values({ + const row: typeof orders.$inferInsert = { id: orderId, totalAmountMinor: 1000, totalAmount: toDbMoney(1000), currency: 'USD', paymentProvider: 'stripe', paymentStatus: 'pending', status: 'INVENTORY_RESERVED', inventoryStatus: 'reserved', idempotencyKey: crypto.randomUUID(), fulfillmentMode: 'intl', - } as any); + }; + await db.insert(orders).values(row); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/tests/shop/order-payment-init-token-scope-phase7.test.ts` around lines 67 - 79, The test uses an untyped insert (db.insert(orders).values(... as any)) which removes schema safety; replace the cast by constructing a properly typed insert payload for the orders table: import or reference the generated Orders/OrdersInsert type (or the ORM's insert type) and declare the payload typed to that type, then pass that payload to db.insert(orders).values(...). Keep the same fields (id: orderId, totalAmountMinor, totalAmount: toDbMoney(1000), currency, paymentProvider, paymentStatus, status, inventoryStatus, idempotencyKey, fulfillmentMode) but ensure types match the orders schema and remove "as any".
110-111: Make the “forbidden” token explicit instead of default-derived.Line 110 currently depends on
createStatusTokendefault scopes. If defaults evolve, this negative-path test may start failing for the wrong reason. Use an explicit scope set that intentionally excludesorder_payment_init.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/tests/shop/order-payment-init-token-scope-phase7.test.ts` around lines 110 - 111, The test relies on createStatusToken defaults for a negative-path token; change the call to createStatusToken used for the forbidden case (currently assigned to unscopedToken and passed to makeRequest) to pass an explicit scopes list that intentionally does NOT include "order_payment_init" (e.g., an array of valid scopes excluding that permission) and rename the variable to reflect it’s a forbidden token (e.g., forbiddenToken) so the test remains stable if defaults change.frontend/lib/tests/shop/orders-status-ownership.test.ts (1)
397-398: TightenpaymentStatusassertion to reduce false positives.
toBeTruthy()is permissive. On Line 397, asserting type/allowed values makes this test more regression-resistant.Suggested test hardening
- expect((json as any).paymentStatus).toBeTruthy(); + expect(typeof (json as any).paymentStatus).toBe('string'); + expect(['pending', 'processing', 'success', 'failed', 'expired']).toContain( + (json as any).paymentStatus + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/tests/shop/orders-status-ownership.test.ts` around lines 397 - 398, Replace the permissive expect((json as any).paymentStatus).toBeTruthy() with a tighter assertion that verifies the type and allowed values for paymentStatus: assert it's a string and assert it equals one of the expected enum/values used by the app (e.g., "PAID", "PENDING", "FAILED" or your codebase's exact status names) so the test checks both type and membership rather than truthiness; update the assertion in orders-status-ownership.test.ts around the (json as any).paymentStatus check accordingly.frontend/lib/tests/shop/product-prices-write-authority-phase8.test.ts (1)
1-1: Remove unusedcryptoimport.The
cryptomodule is imported but never used in this test file.🧹 Proposed fix
-import crypto from 'node:crypto'; - import { and, eq } from 'drizzle-orm';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/tests/shop/product-prices-write-authority-phase8.test.ts` at line 1, The file imports the unused symbol "crypto" via the line `import crypto from 'node:crypto';` which should be removed to avoid unused-import lint errors; delete that import statement from the test file (product-prices-write-authority-phase8.test.ts) so only used imports remain.frontend/lib/tests/shop/monobank-webhook-retry-classifier.test.ts (1)
22-25: Clarify test name for accuracy.The test name says "known code outside transient whitelist" but
'SOME_UNKNOWN_KNOWN_CODE'is not in either list - it's truly unknown. Consider renaming to better reflect the fail-closed behavior being tested.📝 Suggested rename
- it('known code outside transient whitelist is non-retryable (fail-closed)', () => { + it('unknown code is non-retryable (fail-closed)', () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/tests/shop/monobank-webhook-retry-classifier.test.ts` around lines 22 - 25, Test name is misleading: change the it(...) description for the test that calls isRetryableApplyError with err = { code: 'SOME_UNKNOWN_KNOWN_CODE' } to accurately describe that an unknown code (not in known or transient lists) is treated as non-retryable (fail-closed); update the string in the test (the it(...) invocation) in monobank-webhook-retry-classifier.test.ts to something like "unknown code is non-retryable (fail-closed)" so it matches the assertion and function under test (isRetryableApplyError).frontend/lib/services/shop/notifications/projector.ts (1)
18-36: Optional: Consolidate identical canonical row types.
ShippingCanonicalRowandPaymentCanonicalRoware structurally identical. Consider unifying them into a singleCanonicalEventRowtype.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/services/shop/notifications/projector.ts` around lines 18 - 36, ShippingCanonicalRow and PaymentCanonicalRow are identical; replace both with a single CanonicalEventRow type and update any references to ShippingCanonicalRow and PaymentCanonicalRow to use CanonicalEventRow instead (e.g., change type declarations, function params/returns, and variable annotations that mention ShippingCanonicalRow or PaymentCanonicalRow such as in the projector file). Ensure the new CanonicalEventRow includes the same fields (id, orderId, eventName, eventSource, eventRef, payload, occurredAt) and keep exports/imports consistent.frontend/lib/services/shop/notifications/outbox-worker.ts (1)
123-132: Basic email regex may reject valid addresses.The regex
/^[^\s@]+@[^\s@]+\.[^\s@]+$/rejects valid addresses likeuser@localhostor addresses with quoted local parts. While this is likely acceptable for typical shop use cases, be aware of edge cases.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/services/shop/notifications/outbox-worker.ts` around lines 123 - 132, The current EMAIL_REGEX used by normalizeEmailOrNull is too strict and rejects valid addresses (e.g., user@localhost or quoted local parts); update normalizeEmailOrNull to either (a) replace the regex with a more permissive/standard one that allows localhost and quoted local parts or (b) remove the heavy regex and perform a simple, pragmatic check (e.g., ensure value is a non-empty string, contains a single '@' and no spaces) or (preferred) use a well-tested validator (e.g., validator.isEmail) to validate addresses; adjust the EMAIL_REGEX constant or the validation logic in normalizeEmailOrNull accordingly and keep existing trimming/lowercasing/redacted checks intact.frontend/lib/services/shop/notifications/transport.ts (1)
105-112: Consider classifyingESOCKETas transient.Socket errors (
ESOCKET) are often caused by transient network issues (connection reset, broken pipe) rather than permanent configuration problems. Treating them as permanent prevents retries that might succeed.Proposed fix
const transientCodes = new Set(['ECONNECTION', 'ETIMEDOUT', 'EAI_AGAIN']); const permanentCodes = new Set([ 'EAUTH', 'EENVELOPE', 'EMESSAGE', - 'ESOCKET', 'NOTIFICATION_TRANSPORT_MISCONFIG', ]); + // ESOCKET removed from permanent - socket errors are often transient network issues🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/services/shop/notifications/transport.ts` around lines 105 - 112, The current classification treats 'ESOCKET' as permanent in the permanentCodes Set; move 'ESOCKET' from the permanentCodes Set into the transientCodes Set so socket-related errors are retried—update the Set literals referenced as transientCodes and permanentCodes to ensure 'ESOCKET' appears only in transientCodes (and remove it from permanentCodes) so retry logic will treat it as transient.frontend/lib/services/shop/events/dedupe-key.ts (1)
33-33: Consider using simple string comparison instead oflocaleComparefor deterministic cross-environment hashing.
localeComparebehavior can vary across Node.js versions, OS locales, and ICU configurations. For dedupe keys that must be identical across all environments (e.g., different servers, CI vs production), a locale-independent comparison ensures consistency.Proposed fix
- const keys = Object.keys(source).sort((a, b) => a.localeCompare(b)); + const keys = Object.keys(source).sort();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/services/shop/events/dedupe-key.ts` at line 33, The sort currently uses localeCompare on the keys (const keys = Object.keys(source).sort((a, b) => a.localeCompare(b))) which is non-deterministic across environments; replace the locale-aware comparison with a simple, deterministic string comparator that returns -1/0/1 based on standard lexical ordering (e.g., compare a and b with < and >) so the keys array is ordered identically everywhere; update the sort call on the keys derived from source in dedupe-key.ts accordingly.frontend/app/api/shop/internal/notifications/run/route.ts (1)
61-100: Consider including Zod validation details in the error response.Currently, when validation fails, only a generic
"Invalid payload"message is returned. Includingparsed.error.issues(or a sanitized subset) would help API consumers debug malformed requests without exposing sensitive internals.💡 Proposed enhancement
const parsed = internalNotificationsRunPayloadSchema.safeParse(rawBody); if (!parsed.success) { return noStoreJson( { success: false, code: 'INVALID_PAYLOAD', message: 'Invalid payload', + errors: parsed.error.issues.map(i => ({ + path: i.path.join('.'), + message: i.message, + })), }, requestId, 400 ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/api/shop/internal/notifications/run/route.ts` around lines 61 - 100, When internalNotificationsRunPayloadSchema.safeParse(rawBody) fails in the route handler, return a 400 payload that includes sanitized Zod validation details from parsed.error (e.g. parsed.error.issues mapped to simple {path, message} entries or the formatted errors) instead of the generic "Invalid payload"; update the failure branch where parsed.success is false to call noStoreJson with the enhanced error object (referencing internalNotificationsRunPayloadSchema and parsed) and keep the same status/code fields while limiting or formatting the issues to avoid leaking internals.frontend/lib/services/shop/returns.ts (1)
862-881: Minor: timestamp created at setClause construction differs fromnowin applyTransition.Line 875 creates
new Date()when building thesetClause, butapplyTransition(line 741) creates its ownconst now = new Date()for event/audit timestamps. This could result in a few milliseconds difference betweenapproved_atand the audit log'soccurred_at.♻️ Suggested improvement
Pass a shared timestamp to
applyTransitionor have the transition function handle timestamp fields:export async function approveReturnRequest(args: { returnRequestId: string; actorUserId: string | null; requestId: string; }) { + const now = new Date(); return applyTransition({ returnRequestId: args.returnRequestId, actorUserId: args.actorUserId, requestId: args.requestId, expectedFrom: 'requested', statusTo: 'approved', action: 'return.approve', eventName: 'return_approved', - setClause: sql`approved_at = ${new Date()}, approved_by = ${args.actorUserId}`, + setClause: sql`approved_at = ${now}, approved_by = ${args.actorUserId}`, + occurredAt: now, payload: { returnRequestId: args.returnRequestId, actorUserId: args.actorUserId, }, }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/services/shop/returns.ts` around lines 862 - 881, approveReturnRequest constructs approved_at using new Date(), while applyTransition also creates its own now; compute a single timestamp in approveReturnRequest (e.g., const now = new Date()) and pass it into applyTransition so the setClause uses that same now for approved_at and the transition/event/audit timestamps; update the call site to include the shared timestamp argument and adjust applyTransition to accept and use that parameter for its event/audit occurred_at values and any setClause substitutions.frontend/lib/tests/shop/returns-composite-fk-phaseE.test.ts (1)
10-19: Consider adding explicit type assertions instead ofas anycasts.The
as anycasts on lines 18, 35, and 50 suppress type checking for the insert values. While this works for tests, it could hide missing required fields or type mismatches if the schema changes.♻️ Suggested improvement
Consider defining a partial type or using Drizzle's inference types to maintain some type safety while allowing partial inserts:
- } as any); + } satisfies Partial<typeof orders.$inferInsert>);Or add a comment explaining which required fields are intentionally omitted (relying on DB defaults).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/tests/shop/returns-composite-fk-phaseE.test.ts` around lines 10 - 19, The test uses broad `as any` casts for inserted rows (e.g., in the createOrder function where you call `db.insert(orders).values({...} as any)`), which silences type checking; replace these with an explicit, narrower type such as a Partial/insert type for the orders table (for example define a NewOrder type or use Drizzle's inferred insert type like `InferInsertModel<typeof orders>`/`Partial<...>`), or add a concise comment documenting intentionally omitted DB-default fields; update all similar casts (the other insert calls flagged) to use that explicit type so the compiler still verifies property names and types while allowing omitted fields.frontend/lib/tests/shop/admin-shipping-canonical-audit.test.ts (1)
21-37: Consider using proper types instead ofas anyfor test data.The
as anycast on line 37 bypasses type checking for the order insert. While acceptable for test setup, this could mask issues if the schema changes. Consider defining a test fixture type or using a partial type assertion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/tests/shop/admin-shipping-canonical-audit.test.ts` around lines 21 - 37, The test uses a broad `as any` cast when calling db.insert(orders).values(...) which hides type errors; replace it with a proper typed fixture (e.g. a Partial or Insert type for your orders row) and pass that to db.insert to let the compiler validate fields — locate the insert call using db.insert and orders and build a typed object (using the existing toDbMoney(orderTotal) and orderId/crypto.randomUUID() values) or create a test helper like makeOrderFixture(): Partial<OrderInsert> to avoid `as any`.frontend/lib/tests/helpers/db-safety.ts (1)
39-43: Potential whitespace mismatch in URL comparison.The
requiredLocalvalue is obtained fromprocess.env.SHOP_REQUIRED_DATABASE_URL_LOCAL ?? ''without trimming, butdatabaseUrlLocalis also not trimmed. If either env var has leading/trailing whitespace, the comparison on line 39 might fail unexpectedly.Consider trimming both values for consistent comparison:
♻️ Suggested fix
- const requiredLocal = process.env.SHOP_REQUIRED_DATABASE_URL_LOCAL ?? ''; + const requiredLocal = (process.env.SHOP_REQUIRED_DATABASE_URL_LOCAL ?? '').trim(); - if (strictLocal && requiredLocal && databaseUrlLocal !== requiredLocal) { + if (strictLocal && requiredLocal && databaseUrlLocal.trim() !== requiredLocal) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/tests/helpers/db-safety.ts` around lines 39 - 43, The comparison between databaseUrlLocal and requiredLocal in the strictLocal check can fail due to leading/trailing whitespace; update the code that obtains or uses requiredLocal and databaseUrlLocal (variables named requiredLocal and databaseUrlLocal) to trim both values (e.g., call .trim() when assigning or before comparison) so the conditional if (strictLocal && requiredLocal && databaseUrlLocal !== requiredLocal) uses trimmed strings for a reliable equality check.frontend/lib/shop/status-token.ts (1)
140-163: Type assertions without prior validation could be improved.Lines 154-158 cast
rawPayloadfields to their expected types without validating the actual types first. While the subsequent checks (lines 165-178) catch invalidv,orderId,exp, andiatvalues,nonceis not validated and could be a non-string value.Consider adding explicit type checks before casting, or validating
noncein the existing validation block:♻️ Suggested validation
if (!Number.isFinite(payload.iat) || payload.iat > now + 60) { return { ok: false, reason: 'invalid_iat' }; } + if (typeof payload.nonce !== 'string') { + return { ok: false, reason: 'invalid_payload' }; + } + return { ok: true, payload };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/shop/status-token.ts` around lines 140 - 163, The code casts rawPayload.nonce into payload without validating its type; update the validation so nonce is explicitly checked to be a string (or null/undefined as appropriate) before assigning to payload. In the block that currently validates v, orderId, exp, and iat, add a check for typeof rawPayload.nonce === 'string' (or allow undefined) and only then set payload.nonce from rawPayload.nonce; otherwise return the same invalid_payload error. Reference rawPayload, payload, TokenPayload, and the existing validation block to locate where to add this check.frontend/app/api/shop/admin/products/[id]/status/route.ts (1)
59-61: Consider simplifyingactorUserIdderivation.Since
requireAdminApithrowsAdminUnauthorizedErrorwhen there's no user,adminUseris guaranteed to be truthy at this point. The null-coalescing tonullis defensive but may be unnecessary.That said, keeping it as-is is also acceptable for defensive coding against potential future changes to
requireAdminApi.♻️ Optional simplification
const adminUser = await requireAdminApi(request); - const actorUserId = - adminUser && typeof adminUser.id === 'string' ? adminUser.id : null; + const actorUserId = adminUser.id;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/api/shop/admin/products/`[id]/status/route.ts around lines 59 - 61, The actorUserId computation is overly defensive because requireAdminApi throws on unauthorized, so adminUser is always present; simplify by directly deriving the id (e.g., replace the conditional assignment to actorUserId with a direct extraction from adminUser.id or a typed assertion) using the existing requireAdminApi and actorUserId symbols, or if you prefer to keep a safety check, narrow the type of adminUser first (e.g., ensure adminUser is typed non-nullable) before assigning actorUserId.frontend/app/api/shop/orders/[id]/quote/quote-utils.ts (1)
22-34: Consider adding a brief comment explaining the status code mapping rationale.The conditional logic is correct but may be hard to follow without context. A short comment explaining why certain codes map to 409/410/400 would improve maintainability.
📝 Suggested documentation
+/** + * Maps quote error codes to HTTP status codes: + * - 409 Conflict: state conflicts (already accepted, not offered, etc.) or version mismatches + * - 410 Gone: expired quotes (except during initial request) + * - 400 Bad Request: all other validation errors + */ export function mapQuoteErrorStatus( code: string, mode: QuoteErrorStatusMode ): number {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/api/shop/orders/`[id]/quote/quote-utils.ts around lines 22 - 34, Add a concise comment above the mapQuoteErrorStatus function (or immediately above its main conditional) that explains the mapping rationale: that codes in CONFLICT_CODES or specific version/stock conflicts (CONFLICT_CODES, 'QUOTE_VERSION_CONFLICT' with VERSION_CONFLICT_MODES, and 'QUOTE_STOCK_UNAVAILABLE' when mode === 'accept') represent client/server conflicts and map to HTTP 409, that 'QUOTE_EXPIRED' represents a resource no longer available and maps to 410 (except for request mode), and all other errors default to 400; reference the function name mapQuoteErrorStatus and constants CONFLICT_CODES and VERSION_CONFLICT_MODES in the comment.frontend/lib/services/shop/events/write-shipping-event.ts (1)
23-25: Consider addingdbexecutor option for consistency withwriteAdminAudit.The
writeAdminAuditfunction accepts an optional{ db?: AdminAuditExecutor }parameter for testability and transaction support. This function currently only uses the globaldbinstance.♻️ Add optional executor parameter
+type ShippingEventExecutor = Pick<typeof db, 'insert'>; + export async function writeShippingEvent( - args: WriteShippingEventArgs + args: WriteShippingEventArgs, + options?: { db?: ShippingEventExecutor } ): Promise<{ inserted: boolean; dedupeKey: string; id: string | null }> { + const executor = options?.db ?? db; const dedupeKey = args.dedupeKey ??Then use
executorinstead ofdbin the insert call.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/services/shop/events/write-shipping-event.ts` around lines 23 - 25, Update writeShippingEvent to accept an optional executor parameter (e.g., { db?: ShippingEventExecutor }) similar to writeAdminAudit so tests and transactions can inject a DB executor; change the function signature of writeShippingEvent to include this optional options object, default to using the global db when not provided, and replace direct uses of the global db in the function (the insert call and any selects) with the provided executor variable (e.g., use executor.db or executor.execute) so the same injection pattern as writeAdminAudit is followed.frontend/lib/tests/shop/returns-phase4.test.ts (1)
154-159: Avoid hard-coded shared admin IDs in tests.Using fixed IDs (
admin_1…admin_4) introduces inter-test coupling and can cause flaky behavior when test files run concurrently. Prefer per-test random admin IDs and scoped cleanup.Also applies to: 207-212, 266-271, 328-333, 398-398
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/tests/shop/returns-phase4.test.ts` around lines 154 - 159, Tests use hard-coded admin IDs like 'admin_1' when calling ensureAdmin and approveReturnRequest which can couple tests; change each occurrence (e.g., where ensureAdmin('admin_1') and approveReturnRequest({ actorUserId: 'admin_1', ... }) are used) to generate a unique admin id per test (e.g., const adminId = `admin_${randomUUID()}`) and pass that adminId into ensureAdmin and approveReturnRequest, then ensure any created admin is torn down or scoped to the test (cleanup/afterEach) to avoid cross-test interference.frontend/app/api/shop/admin/returns/[id]/reject/route.ts (1)
18-28: Extract shared admin-returns response helpers to avoid drift.
noStoreJsonandmapInvalidPayloadStatusduplicate logic already present infrontend/app/api/shop/admin/returns/[id]/approve/route.ts. Centralizing these helpers will reduce divergence risk across approve/reject/receive/refund routes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/api/shop/admin/returns/`[id]/reject/route.ts around lines 18 - 28, Extract the duplicated helpers noStoreJson and mapInvalidPayloadStatus into a shared module (e.g., an admin-returns response helpers file) and import them from that module in this route; remove the local definitions of noStoreJson and mapInvalidPayloadStatus in reject route and update usages to call the imported functions so approve/reject/receive/refund routes all reuse the same implementations (ensure the exported helpers maintain the same signatures used by the existing calls).frontend/lib/tests/shop/order-status-token.test.ts (1)
231-302: Test names are now misleading relative to assertions.Line 231 and Line 262 describe attempt-return/prioritization behavior, but the assertions intentionally require
json.attemptto be undefined. Consider renaming these tests to reflect lite-mode response expectations.Suggested rename-only patch
- it('returns attempt when a payment attempt exists', async () => { + it('returns lite payload without attempt even when payment attempts exist', async () => { @@ - it('prefers creating/active attempt over newer non-active attempt', async () => { + it('keeps lite payload stable (no attempt object) regardless of attempt ordering', async () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/tests/shop/order-status-token.test.ts` around lines 231 - 302, The test titles are misleading: the it blocks named "returns attempt when a payment attempt exists" and "prefers creating/active attempt over newer non-active attempt" assert that json.attempt is undefined (lite-mode). Rename those test descriptions to reflect lite-mode/no-attempt behavior (e.g., "returns no attempt (lite-mode) when a payment attempt exists" and "returns no attempt (lite-mode) preferring active attempt over newer non-active attempt") by updating the string arguments to the corresponding it(...) calls in frontend/lib/tests/shop/order-status-token.test.ts.frontend/lib/services/shop/shipping/shipments-worker.ts (3)
475-483: Minor formatting: Inconsistent indentation in SQL join clause.Line 482 has inconsistent indentation compared to the rest of the CTE structure. While this is purely cosmetic, maintaining consistent formatting aids readability in complex SQL queries.
🧹 Fix indentation
select c.id, c.order_id, c.provider, c.status, c.attempt_count from claimed c -join mark_orders mo on mo.order_id = c.order_id + join mark_orders mo on mo.order_id = c.order_id `);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/services/shop/shipping/shipments-worker.ts` around lines 475 - 483, Align the SQL JOIN indentation in the CTE/select block inside shipments-worker.ts so it matches the preceding lines: the "join mark_orders mo on mo.order_id = c.order_id" line should be indented to the same level as "from claimed c" and the other select clauses (maintain consistent two-space indentation used in the CTE) to improve readability of the query.
864-921: Consider consolidating duplicate metric try/catch blocks.The failure path has three nearly identical try/catch blocks for recording metrics (lines 864-882, 884-901, 903-920). While this works correctly, consider extracting a helper to reduce repetition.
♻️ Example helper extraction
function safeRecordShippingMetric(args: Parameters<typeof recordShippingMetric>[0], warnMeta: Record<string, unknown>) { try { recordShippingMetric(args); } catch { logWarn('shipping_shipments_worker_terminal_metric_write_failed', warnMeta); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/services/shop/shipping/shipments-worker.ts` around lines 864 - 921, Duplicate try/catch blocks are used around recordShippingMetric calls (when terminalNeedsAttention is false) which should be consolidated; create a helper like safeRecordShippingMetric that accepts the recordShippingMetric payload and the warn metadata, calls recordShippingMetric inside a try and calls logWarn('shipping_shipments_worker_terminal_metric_write_failed', warnMeta) in the catch, then replace the three repetitive try/catch usages with calls to safeRecordShippingMetric passing the same args (use the existing payloads that include name/source/runId/orderId/shipmentId/code and the warnMeta containing runId/orderId/shipmentId/errorCode/code).
291-293: Minor: SimplifynextAttemptNumberlogic.The inner
Math.max(0, Math.trunc(attemptCount))is redundant since the outerMath.max(1, ...)already ensures the result is at least 1, handling any negative inputs.♻️ Simplified implementation
function nextAttemptNumber(attemptCount: number): number { - return Math.max(1, Math.max(0, Math.trunc(attemptCount)) + 1); + return Math.max(1, Math.trunc(attemptCount) + 1); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/services/shop/shipping/shipments-worker.ts` around lines 291 - 293, The function nextAttemptNumber contains a redundant inner Math.max call; simplify by computing Math.trunc(attemptCount) + 1 and then clamping with Math.max(1, ...). Update nextAttemptNumber(attemptCount: number) to return Math.max(1, Math.trunc(attemptCount) + 1) so negative inputs still produce at least 1 and behavior is unchanged.frontend/lib/services/shop/shipping/admin-actions.ts (3)
156-216: Canonical dual-write branching is well-structured.The function cleanly branches between non-canonical (simple update) and canonical (CTE with audit log insert) paths. The audit log insert uses
on conflict (dedupe_key) do nothingfor idempotency.However, line 210 has a redundant condition:
where ${args.canonical.enabled} = trueSince we're already in the
if (args.canonical.enabled)branch (the function returns early at line 168 otherwise), this SQL condition will always be true.♻️ Remove redundant SQL condition
from updated_order uo - where ${args.canonical.enabled} = true on conflict (dedupe_key) do nothing🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/services/shop/shipping/admin-actions.ts` around lines 156 - 216, The SQL WHERE clause "where ${args.canonical.enabled} = true" inside the inserted_admin_audit CTE is redundant because the outer JS branch already ensures args.canonical.enabled is true; remove that condition from the CTE (in the block that builds the CTEs: updated_order / inserted_admin_audit) so the INSERT SELECT reads directly from updated_order without filtering on ${args.canonical.enabled}, leaving the rest of the query (on conflict, returning id) unchanged.
218-329:requeueShipmentcanonical flow mirrorsappendAuditEntrypattern.Same observation: line 321 has the redundant
where ${args.canonical.enabled} = truecondition within the already-guarded canonical branch.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/services/shop/shipping/admin-actions.ts` around lines 218 - 329, The canonical branch in requeueShipment redundantly checks the same guard inside the inserted_admin_audit WHERE clause; remove the "where ${args.canonical.enabled} = true" condition (or replace it with a no-op true) in the inserted_admin_audit CTE so the insertion relies on the outer canonical branch, leaving the rest of the CTE (insert into admin_audit_log, dedupe logic, and returning) intact.
331-413:updateOrderShippingStatusfollows the same dual-write pattern.The transition guard via
shippingStatusTransitionWhereSqlis correctly applied in both the non-canonical and canonical branches. Same redundant condition at line 406.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/services/shop/shipping/admin-actions.ts` around lines 331 - 413, The code duplicates the shipping-status transition guard by applying shippingStatusTransitionWhereSql in both branches of updateOrderShippingStatus; consolidate the update logic so the transition condition is applied only once: extract the UPDATE ... RETURNING (the logic currently in the non-canonical branch or the updated_order CTE) into a single shared SQL block used by both code paths (or factor into a helper used by both), then have the canonical branch only add the inserted_admin_audit CTE that SELECTs from that shared updated_order; reference shippingStatusTransitionWhereSql, updated_order CTE and args.canonical.enabled/inserted_admin_audit when making the change.frontend/app/api/shop/admin/orders/[id]/quote/offer/route.ts (1)
24-28:noStoreJsonhelper duplicates existing utility.This helper is identical to the one in
frontend/app/api/shop/orders/[id]/quote/quote-utils.ts(see relevant code snippets). Consider importing from the shared utility to reduce duplication.♻️ Import from shared utility
-function noStoreJson(body: unknown, init?: { status?: number }) { - const res = NextResponse.json(body, { status: init?.status ?? 200 }); - res.headers.set('Cache-Control', 'no-store'); - return res; -} +import { noStoreJson } from '@/app/api/shop/orders/[id]/quote/quote-utils';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/api/shop/admin/orders/`[id]/quote/offer/route.ts around lines 24 - 28, The local noStoreJson function duplicates the shared helper; remove the local declaration of noStoreJson in this route and import the shared helper from the existing quote utility (the module that exports noStoreJson from quote-utils) instead; add an import for noStoreJson at the top of the file, delete the duplicate function, and keep all existing call sites unchanged so the route uses the centralized noStoreJson implementation.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (7)
frontend/package-lock.jsonis excluded by!**/package-lock.jsonfrontend/public/icons/aws.svgis excluded by!**/*.svgfrontend/public/icons/azure.svgis excluded by!**/*.svgfrontend/public/icons/devops.svgis excluded by!**/*.svgfrontend/public/icons/django.svgis excluded by!**/*.svgfrontend/public/icons/docker.svgis excluded by!**/*.svgfrontend/public/icons/kubernetes.svgis excluded by!**/*.svg
📒 Files selected for processing (126)
CHANGELOG.mdfrontend/.env.examplefrontend/.gitignorefrontend/app/[locale]/shop/checkout/success/MonobankRedirectStatus.tsxfrontend/app/api/shop/admin/orders/[id]/quote/offer/route.tsfrontend/app/api/shop/admin/products/[id]/route.tsfrontend/app/api/shop/admin/products/[id]/status/route.tsfrontend/app/api/shop/admin/products/route.tsfrontend/app/api/shop/admin/returns/[id]/approve/route.tsfrontend/app/api/shop/admin/returns/[id]/receive/route.tsfrontend/app/api/shop/admin/returns/[id]/refund/route.tsfrontend/app/api/shop/admin/returns/[id]/reject/route.tsfrontend/app/api/shop/checkout/route.tsfrontend/app/api/shop/internal/notifications/run/route.tsfrontend/app/api/shop/internal/orders/restock-stale/route.tsfrontend/app/api/shop/orders/[id]/payment/init/route.tsfrontend/app/api/shop/orders/[id]/quote/accept/route.tsfrontend/app/api/shop/orders/[id]/quote/decline/route.tsfrontend/app/api/shop/orders/[id]/quote/quote-utils.tsfrontend/app/api/shop/orders/[id]/quote/request/route.tsfrontend/app/api/shop/orders/[id]/returns/route.tsfrontend/app/api/shop/orders/[id]/status/route.tsfrontend/app/api/shop/webhooks/monobank/route.tsfrontend/app/api/shop/webhooks/stripe/route.tsfrontend/components/about/HeroSection.tsxfrontend/data/category.tsfrontend/data/categoryStyles.tsfrontend/db/index.tsfrontend/db/schema/shop.tsfrontend/drizzle/0021_solid_sage.sqlfrontend/drizzle/0022_demonic_vapor.sqlfrontend/drizzle/0023_clean_madame_masque.sqlfrontend/drizzle/0024_gigantic_annihilus.sqlfrontend/drizzle/0025_cute_mentor.sqlfrontend/drizzle/0026_gray_stone_men.sqlfrontend/drizzle/meta/0021_snapshot.jsonfrontend/drizzle/meta/0022_snapshot.jsonfrontend/drizzle/meta/0023_snapshot.jsonfrontend/drizzle/meta/0024_snapshot.jsonfrontend/drizzle/meta/0025_snapshot.jsonfrontend/drizzle/meta/0026_snapshot.jsonfrontend/drizzle/meta/_journal.jsonfrontend/lib/about/stats.tsfrontend/lib/env/index.tsfrontend/lib/env/monobank.tsfrontend/lib/env/nova-poshta.tsfrontend/lib/env/shop-canonical-events.tsfrontend/lib/env/shop-intl.tsfrontend/lib/env/shop-legal.tsfrontend/lib/services/orders/_shared.tsfrontend/lib/services/orders/checkout.tsfrontend/lib/services/orders/monobank-retry.tsfrontend/lib/services/orders/monobank-webhook.tsfrontend/lib/services/orders/payment-attempts.tsfrontend/lib/services/orders/restock.tsfrontend/lib/services/products/mutations/create.tsfrontend/lib/services/products/mutations/update.tsfrontend/lib/services/products/slug.tsfrontend/lib/services/products/types.tsfrontend/lib/services/shop/events/dedupe-key.tsfrontend/lib/services/shop/events/write-admin-audit.tsfrontend/lib/services/shop/events/write-payment-event.tsfrontend/lib/services/shop/events/write-shipping-event.tsfrontend/lib/services/shop/notifications/outbox-worker.tsfrontend/lib/services/shop/notifications/projector.tsfrontend/lib/services/shop/notifications/templates.tsfrontend/lib/services/shop/notifications/transport.tsfrontend/lib/services/shop/order-access.tsfrontend/lib/services/shop/quotes.tsfrontend/lib/services/shop/returns.tsfrontend/lib/services/shop/shipping/admin-actions.tsfrontend/lib/services/shop/shipping/shipments-worker.tsfrontend/lib/services/shop/transitions/order-state.tsfrontend/lib/services/shop/transitions/return-state.tsfrontend/lib/services/shop/transitions/shipping-state.tsfrontend/lib/shop/status-token.tsfrontend/lib/shop/url.tsfrontend/lib/tests/helpers/db-safety.tsfrontend/lib/tests/shop/admin-product-audit-dedupe-phase5.test.tsfrontend/lib/tests/shop/admin-product-canonical-audit-phase5.test.tsfrontend/lib/tests/shop/admin-product-create-atomic-phasec.test.tsfrontend/lib/tests/shop/admin-shipping-canonical-audit.test.tsfrontend/lib/tests/shop/canonical-events-env.test.tsfrontend/lib/tests/shop/checkout-legal-consent-phase4.test.tsfrontend/lib/tests/shop/checkout-monobank-idempotency-contract.test.tsfrontend/lib/tests/shop/checkout-shipping-phase3.test.tsfrontend/lib/tests/shop/intl-quote-domain-phase2.test.tsfrontend/lib/tests/shop/monobank-api-methods.test.tsfrontend/lib/tests/shop/monobank-env.test.tsfrontend/lib/tests/shop/monobank-http-client.test.tsfrontend/lib/tests/shop/monobank-payments-disabled.test.tsfrontend/lib/tests/shop/monobank-webhook-apply.test.tsfrontend/lib/tests/shop/monobank-webhook-crypto.test.tsfrontend/lib/tests/shop/monobank-webhook-rate-limit-policy.test.tsfrontend/lib/tests/shop/monobank-webhook-retry-classifier.test.tsfrontend/lib/tests/shop/monobank-webhook-signature-verify.test.tsfrontend/lib/tests/shop/notifications-projector-phase3.test.tsfrontend/lib/tests/shop/notifications-worker-phase3.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-payment-init-intl-gate-phase2.test.tsfrontend/lib/tests/shop/order-payment-init-token-scope-phase7.test.tsfrontend/lib/tests/shop/order-status-token.test.tsfrontend/lib/tests/shop/orders-status-ownership.test.tsfrontend/lib/tests/shop/product-prices-write-authority-phase8.test.tsfrontend/lib/tests/shop/returns-composite-fk-phaseE.test.tsfrontend/lib/tests/shop/returns-phase4.test.tsfrontend/lib/tests/shop/returns-route-phase4.test.tsfrontend/lib/tests/shop/setup.tsfrontend/lib/tests/shop/shipping-internal-retention-route-phase7.test.tsfrontend/lib/tests/shop/shipping-methods-route-p2.test.tsfrontend/lib/tests/shop/shipping-np-cities-route-p2.test.tsfrontend/lib/tests/shop/shipping-np-warehouses-route-p2.test.tsfrontend/lib/tests/shop/shipping-shipments-worker-phase5.test.tsfrontend/lib/tests/shop/shop-url.test.tsfrontend/lib/tests/shop/stripe-webhook-psp-fields.test.tsfrontend/lib/tests/shop/transition-matrix-phase6.test.tsfrontend/lib/types/shop.tsfrontend/lib/validation/shop-notifications.tsfrontend/lib/validation/shop-returns.tsfrontend/lib/validation/shop.tsfrontend/package.jsonfrontend/playwright.config.tsfrontend/tests/e2e/shop-minimal-phase9.spec.tsfrontend/vitest.shop.config.tsstudio/package.json
💤 Files with no reviewable changes (14)
- frontend/lib/tests/shop/shipping-np-cities-route-p2.test.ts
- frontend/lib/tests/shop/monobank-http-client.test.ts
- frontend/lib/tests/shop/checkout-shipping-phase3.test.ts
- frontend/lib/tests/shop/shipping-internal-retention-route-phase7.test.ts
- frontend/lib/tests/shop/monobank-webhook-crypto.test.ts
- frontend/lib/tests/shop/checkout-monobank-idempotency-contract.test.ts
- frontend/lib/tests/shop/shipping-methods-route-p2.test.ts
- frontend/lib/tests/shop/monobank-api-methods.test.ts
- frontend/lib/tests/shop/monobank-webhook-signature-verify.test.ts
- frontend/lib/tests/shop/shipping-np-warehouses-route-p2.test.ts
- frontend/lib/tests/shop/monobank-env.test.ts
- frontend/lib/tests/shop/monobank-payments-disabled.test.ts
- frontend/lib/tests/shop/nova-poshta-client-network-failure.test.ts
- frontend/lib/tests/shop/shop-url.test.ts
Comment on lines
+491
to
+506
| } catch (auditError) { | ||
| logWarn('admin_product_update_audit_failed', { | ||
| ...baseMeta, | ||
| code: 'AUDIT_WRITE_FAILED', | ||
| requestId, | ||
| actorUserId, | ||
| productId: updated.id, | ||
| action: 'product_admin_action.update', | ||
| message: | ||
| auditError instanceof Error | ||
| ? auditError.message | ||
| : String(auditError), | ||
| durationMs: Date.now() - startedAtMs, | ||
| }); | ||
| throw auditError; | ||
| } |
Contributor
There was a problem hiding this comment.
Audit failure causes 500 error even though product update succeeded.
When writeAdminAudit fails (line 491-505), the error is rethrown and eventually returns a 500 response. However, updateProduct at line 455 already succeeded. This creates a confusing user experience:
- Client receives 500 error
- Client may retry the update
- But the product was actually updated
This is inconsistent with the DELETE handler (lines 774-788) which logs audit failures but still returns success.
🔧 Recommended fix
Follow the same pattern as DELETE - log the failure but return success:
try {
await writeAdminAudit({
actorUserId,
action: 'product_admin_action.update',
// ... rest of args
});
} catch (auditError) {
logWarn('admin_product_update_audit_failed', {
...baseMeta,
code: 'AUDIT_WRITE_FAILED',
// ... rest of meta
});
- throw auditError;
+ // Update is complete; keep success response to avoid misleading retries.
+ // Audit failure is logged and should be monitored/alerted separately.
}
return noStoreJson({ success: true, product: updated }, { status: 200 });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/app/api/shop/admin/products/`[id]/route.ts around lines 491 - 506,
The catch block for writeAdminAudit currently rethrows auditError causing a 500
even though updateProduct succeeded; remove the throw and follow the DELETE
handler pattern by logging the audit failure (using logWarn as already done) but
returning a successful response containing the updated product (or appropriate
200/204) after updateProduct completes; specifically, in the route where
updateProduct and writeAdminAudit are called, delete the `throw auditError` and
ensure the function returns the same success payload/status it would when
auditing succeeds so clients see the update as successful.
Comment on lines
+480
to
+497
| const remaining3 = Math.max(0, deadlineMs - Date.now()); | ||
| const processedIntlQuoteExpired = | ||
| remaining3 > 0 | ||
| ? await sweepExpiredOfferedIntlQuotes({ | ||
| batchSize: policy.batchSize, | ||
| now: new Date(), | ||
| }) | ||
| : 0; | ||
|
|
||
| const remaining4 = Math.max(0, deadlineMs - Date.now()); | ||
| const processedIntlQuotePaymentTimeouts = | ||
| remaining4 > 0 | ||
| ? await sweepAcceptedIntlQuotePaymentTimeouts({ | ||
| batchSize: policy.batchSize, | ||
| now: new Date(), | ||
| }) | ||
| : 0; | ||
|
|
Contributor
There was a problem hiding this comment.
New sweep steps bypass the maxRuntimeMs contract.
On Line 482-Line 496, both new sweeps only check remaining > 0 before starting, but each sweep can still run a full batch once entered. That can push request runtime past deadlineMs and weaken the route’s time-budget guarantee.
💡 Proposed direction (route + service contract)
- const remaining3 = Math.max(0, deadlineMs - Date.now());
+ const sweepNow = new Date();
+ const remaining3 = Math.max(0, deadlineMs - Date.now());
const processedIntlQuoteExpired =
remaining3 > 0
? await sweepExpiredOfferedIntlQuotes({
batchSize: policy.batchSize,
- now: new Date(),
+ now: sweepNow,
+ timeBudgetMs: remaining3,
})
: 0;
const remaining4 = Math.max(0, deadlineMs - Date.now());
const processedIntlQuotePaymentTimeouts =
remaining4 > 0
? await sweepAcceptedIntlQuotePaymentTimeouts({
batchSize: policy.batchSize,
- now: new Date(),
+ now: sweepNow,
+ timeBudgetMs: remaining4,
})
: 0;And in frontend/lib/services/shop/quotes.ts, add timeBudgetMs handling so loops stop when budget is exhausted.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/app/api/shop/internal/orders/restock-stale/route.ts` around lines
480 - 497, The two new sweep calls (sweepExpiredOfferedIntlQuotes and
sweepAcceptedIntlQuotePaymentTimeouts) only check remaining time once and then
may run a full batch past deadlineMs; update the route to compute remaining time
before each batch invocation and pass a timeBudgetMs (e.g., Math.max(0,
deadlineMs - Date.now())) into both sweep calls instead of only checking
remaining > 0, and update the corresponding service functions in
frontend/lib/services/shop/quotes.ts to accept a timeBudgetMs parameter and
break their internal loop when the budget is exhausted (stop starting a new
batch if timeBudgetMs <= 0 or would be exceeded). Ensure the route uses
policy.batchSize and now as before but supplies timeBudgetMs, and ensure the
service loop reduces/refreshes the remaining budget between batches so the
maxRuntimeMs contract is honored.
Comment on lines
+56
to
+57
| const blocked = guardBrowserSameOrigin(request); | ||
| if (blocked) return blocked; |
Contributor
There was a problem hiding this comment.
Ensure blocked same-origin responses are explicitly non-cacheable.
At Line 57, the blocked response is returned without setting Cache-Control: no-store, unlike the quote routes. This leaves an inconsistent cache policy for this endpoint’s rejection path.
Suggested patch
const blocked = guardBrowserSameOrigin(request);
- if (blocked) return blocked;
+ if (blocked) {
+ blocked.headers.set('Cache-Control', 'no-store');
+ return blocked;
+ }📝 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
| const blocked = guardBrowserSameOrigin(request); | |
| if (blocked) return blocked; | |
| const blocked = guardBrowserSameOrigin(request); | |
| if (blocked) { | |
| blocked.headers.set('Cache-Control', 'no-store'); | |
| return blocked; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/app/api/shop/orders/`[id]/payment/init/route.ts around lines 56 -
57, The blocked same-origin response returned from the guardBrowserSameOrigin
call is missing a non-cacheable header; update the code path in route.ts where
you check const blocked = guardBrowserSameOrigin(request); if (blocked) return
blocked; to set Cache-Control: no-store on the blocked Response before returning
(e.g., clone or set the header on the Response object returned by
guardBrowserSameOrigin) so the rejection path uses the same non-cacheable policy
as the quote routes.
Comment on lines
+3
to
+4
| "terms_accepted" boolean DEFAULT true NOT NULL, | ||
| "privacy_accepted" boolean DEFAULT true NOT NULL, |
Contributor
There was a problem hiding this comment.
Avoid implicit legal consent via default true.
With defaults set to true, inserts that omit these fields still pass checks. For legal consent records, acceptance should be explicit at write-time.
📜 Proposed fix
- "terms_accepted" boolean DEFAULT true NOT NULL,
- "privacy_accepted" boolean DEFAULT true NOT NULL,
+ "terms_accepted" boolean NOT NULL,
+ "privacy_accepted" boolean NOT NULL,Also applies to: 13-14
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/drizzle/0025_cute_mentor.sql` around lines 3 - 4, Remove the
implicit consent defaults by deleting DEFAULT true for the "terms_accepted" and
"privacy_accepted" columns (and any duplicate occurrences noted at lines 13-14)
so that inserts must explicitly provide these boolean values; keep or enforce
NOT NULL if you want the DB to reject omitted values, or alternatively allow
NULLs if you prefer explicit acceptance represented by true only.
| return { | ||
| webhookMode: parseWebhookMode(rawMode), | ||
| refundEnabled: env.MONO_REFUND_ENABLED === 'true', | ||
| refundEnabled: process.env.MONO_REFUND_ENABLED === 'true', |
Contributor
There was a problem hiding this comment.
Monobank feature flags can misread enabled states.
On Line 26 and Line 98–100, 'true'-only parsing rejects 1. That can silently keep payments/refunds disabled despite explicit enablement via numeric toggles.
Proposed fix
+function parseEnvBool(raw: string | undefined): boolean {
+ const v = (raw ?? '').trim().toLowerCase();
+ return v === 'true' || v === '1';
+}
+
export function getMonobankConfig(): MonobankConfig {
@@
- refundEnabled: process.env.MONO_REFUND_ENABLED === 'true',
+ refundEnabled: parseEnvBool(process.env.MONO_REFUND_ENABLED),
@@
- const paymentsFlag = process.env.PAYMENTS_ENABLED ?? 'false';
+ const paymentsFlag = process.env.PAYMENTS_ENABLED;
const configured = !!token;
- const paymentsEnabled = String(paymentsFlag).trim() === 'true' && configured;
+ const paymentsEnabled = parseEnvBool(paymentsFlag) && configured;Also applies to: 98-100
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/lib/env/monobank.ts` at line 26, The environment flag parsing for
Monobank (e.g., refundEnabled using process.env.MONO_REFUND_ENABLED and the
other flags around lines 98–100) only treats the string 'true' as enabled and
therefore ignores numeric toggles like '1'; update the checks to accept common
truthy representations (at minimum 'true' and '1') or use a small helper like
isEnvTrue to normalize values, then replace direct === 'true' comparisons for
refundEnabled and the flags at the later block with that normalized check
(reference symbols: refundEnabled and the Monobank-related feature flag
variables found near lines 98–100).
Comment on lines
+904
to
+947
| export async function receiveReturnRequest(args: { | ||
| returnRequestId: string; | ||
| actorUserId: string | null; | ||
| requestId: string; | ||
| }) { | ||
| const current = await loadReturnById(args.returnRequestId); | ||
| if (!current) { | ||
| throw returnError('RETURN_NOT_FOUND', 'Return request not found.'); | ||
| } | ||
| if (current.status === 'received') { | ||
| return { changed: false, row: current }; | ||
| } | ||
| if (!isReturnStatusTransitionAllowed(current.status, 'received')) { | ||
| throw returnError( | ||
| 'RETURN_TRANSITION_INVALID', | ||
| `Invalid return transition from ${current.status} to received.`, | ||
| { | ||
| returnRequestId: current.id, | ||
| statusFrom: current.status, | ||
| statusTo: 'received', | ||
| } | ||
| ); | ||
| } | ||
|
|
||
| if (current.policyRestock) { | ||
| await restockReturnItems(current.id, current.orderId); | ||
| } | ||
|
|
||
| return applyTransition({ | ||
| returnRequestId: args.returnRequestId, | ||
| actorUserId: args.actorUserId, | ||
| requestId: args.requestId, | ||
| expectedFrom: 'approved', | ||
| statusTo: 'received', | ||
| action: 'return.receive', | ||
| eventName: 'return_received', | ||
| setClause: sql`received_at = ${new Date()}, received_by = ${args.actorUserId}`, | ||
| payload: { | ||
| returnRequestId: args.returnRequestId, | ||
| actorUserId: args.actorUserId, | ||
| restocked: current.policyRestock, | ||
| }, | ||
| }); | ||
| } |
Contributor
There was a problem hiding this comment.
Race condition: restock operation is not atomic with status transition.
The current flow performs restockReturnItems (line 929) before applyTransition (line 932). If the status changes between these operations (e.g., concurrent request), the inventory is released but the return request remains in approved status.
This creates an inconsistent state where:
- Items are released back to inventory
- But the return is not marked as
received - A retry would fail the transition check but restock was already done
🔧 Recommended approach
Move the restock inside the transition's atomic CTE, or use a transaction wrapper:
export async function receiveReturnRequest(args: {
returnRequestId: string;
actorUserId: string | null;
requestId: string;
}) {
- const current = await loadReturnById(args.returnRequestId);
- if (!current) {
- throw returnError('RETURN_NOT_FOUND', 'Return request not found.');
- }
- if (current.status === 'received') {
- return { changed: false, row: current };
- }
- if (!isReturnStatusTransitionAllowed(current.status, 'received')) {
- throw returnError(
- 'RETURN_TRANSITION_INVALID',
- `Invalid return transition from ${current.status} to received.`,
- {
- returnRequestId: current.id,
- statusFrom: current.status,
- statusTo: 'received',
- }
- );
- }
-
- if (current.policyRestock) {
- await restockReturnItems(current.id, current.orderId);
- }
-
- return applyTransition({
+ // Use a transaction to ensure atomicity
+ return db.transaction(async (tx) => {
+ const current = await loadReturnById(args.returnRequestId);
+ if (!current) {
+ throw returnError('RETURN_NOT_FOUND', 'Return request not found.');
+ }
+ if (current.status === 'received') {
+ return { changed: false, row: current };
+ }
+ // ... validation ...
+
+ const result = await applyTransitionWithTx(tx, {
+ // ... same args ...
+ });
+
+ // Restock AFTER successful status change within same transaction
+ if (current.policyRestock && result.changed) {
+ await restockReturnItemsWithTx(tx, current.id, current.orderId);
+ }
+
+ return result;
+ });Alternatively, integrate restocking into the applyTransition CTE so it's atomic with the status update.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/lib/services/shop/returns.ts` around lines 904 - 947, The restock
operation in receiveReturnRequest is not atomic with the status transition —
move the restocking into the same atomic operation so inventory and status
change succeed or fail together: either call restock logic from inside the
applyTransition CTE (embed the restock SQL/CTE in applyTransition) or execute
restockReturnItems and applyTransition inside a single DB transaction so both
occur atomically; update receiveReturnRequest to remove the pre-transition
restock call and instead invoke the restock as part of applyTransition (or wrap
both in a transaction) and keep the existing isReturnStatusTransitionAllowed
check and expectedFrom/expectedTo semantics.
Comment on lines
+126
to
+137
| async function cleanupSeed(seed: Seed) { | ||
| await db.delete(adminAuditLog).where(eq(adminAuditLog.orderId, seed.orderId)); | ||
| await db | ||
| .delete(shippingEvents) | ||
| .where(eq(shippingEvents.orderId, seed.orderId)); | ||
| await db | ||
| .delete(inventoryMoves) | ||
| .where(eq(inventoryMoves.orderId, seed.orderId)); | ||
| await db.delete(orderItems).where(eq(orderItems.orderId, seed.orderId)); | ||
| await db.delete(orders).where(eq(orders.id, seed.orderId)); | ||
| await db.delete(products).where(eq(products.id, seed.productId)); | ||
| } |
Contributor
There was a problem hiding this comment.
Delete seeded customer users during teardown to keep test state isolated.
cleanupSeed removes order/product artifacts but leaves seed.userId in users, which can accumulate stale rows across repeated runs.
♻️ Proposed fix
async function cleanupSeed(seed: Seed) {
await db.delete(adminAuditLog).where(eq(adminAuditLog.orderId, seed.orderId));
await db
.delete(shippingEvents)
.where(eq(shippingEvents.orderId, seed.orderId));
await db
.delete(inventoryMoves)
.where(eq(inventoryMoves.orderId, seed.orderId));
await db.delete(orderItems).where(eq(orderItems.orderId, seed.orderId));
await db.delete(orders).where(eq(orders.id, seed.orderId));
await db.delete(products).where(eq(products.id, seed.productId));
+ await db.delete(users).where(eq(users.id, seed.userId));
}📝 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
| async function cleanupSeed(seed: Seed) { | |
| await db.delete(adminAuditLog).where(eq(adminAuditLog.orderId, seed.orderId)); | |
| await db | |
| .delete(shippingEvents) | |
| .where(eq(shippingEvents.orderId, seed.orderId)); | |
| await db | |
| .delete(inventoryMoves) | |
| .where(eq(inventoryMoves.orderId, seed.orderId)); | |
| await db.delete(orderItems).where(eq(orderItems.orderId, seed.orderId)); | |
| await db.delete(orders).where(eq(orders.id, seed.orderId)); | |
| await db.delete(products).where(eq(products.id, seed.productId)); | |
| } | |
| async function cleanupSeed(seed: Seed) { | |
| await db.delete(adminAuditLog).where(eq(adminAuditLog.orderId, seed.orderId)); | |
| await db | |
| .delete(shippingEvents) | |
| .where(eq(shippingEvents.orderId, seed.orderId)); | |
| await db | |
| .delete(inventoryMoves) | |
| .where(eq(inventoryMoves.orderId, seed.orderId)); | |
| await db.delete(orderItems).where(eq(orderItems.orderId, seed.orderId)); | |
| await db.delete(orders).where(eq(orders.id, seed.orderId)); | |
| await db.delete(products).where(eq(products.id, seed.productId)); | |
| await db.delete(users).where(eq(users.id, seed.userId)); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/lib/tests/shop/returns-phase4.test.ts` around lines 126 - 137, The
teardown function cleanupSeed currently removes orders, products and related
rows but omits deleting the seeded customer user; add a deletion for the users
table using seed.userId (e.g., call db.delete(users).where(eq(users.id,
seed.userId))) as the final step in cleanupSeed so seeded users are removed
after all dependent rows are deleted to avoid FK issues.
Comment on lines
+5
to
+6
| const REQUIRED_LOCAL_DB_URL = | ||
| 'postgresql://devlovers_local:Gfdtkk43@localhost:5432/devlovers_shop_local_clean?sslmode=disable'; |
Contributor
There was a problem hiding this comment.
Hardcoded database credentials in source code.
The connection string includes username (devlovers_local) and password (Gfdtkk43) directly in the source file. Even for local development databases, embedding credentials in source code is a security risk:
- Credentials end up in version control history
- Developers may inadvertently use similar patterns for production credentials
- CI/CD systems may log or expose these values
🔧 Recommended fix
Use an environment variable reference instead:
-const REQUIRED_LOCAL_DB_URL =
- 'postgresql://devlovers_local:Gfdtkk43@localhost:5432/devlovers_shop_local_clean?sslmode=disable';
+const REQUIRED_LOCAL_DB_URL = process.env.TEST_DATABASE_URL_LOCAL ?? '';
function assertStrictShopLocalDb() {
+ if (!REQUIRED_LOCAL_DB_URL) {
+ throw new Error(
+ '[shop-test-preflight] TEST_DATABASE_URL_LOCAL env var must be set for shop tests.'
+ );
+ }
const appEnv = (process.env.APP_ENV ?? '').trim().toLowerCase();Then document the expected local DSN format in a .env.test.example file (without actual credentials).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/lib/tests/shop/setup.ts` around lines 5 - 6, Replace the hardcoded
DSN in REQUIRED_LOCAL_DB_URL with a read from an environment variable (e.g.,
process.env.TEST_LOCAL_DB_URL or import.meta.env.TEST_LOCAL_DB_URL depending on
runtime) and fail fast or throw a clear error if the env var is missing; also
add a `.env.test.example` entry documenting the expected DSN format
(postgresql://user:pass@host:port/dbname?sslmode=disable) without real
credentials so contributors know how to set TEST_LOCAL_DB_URL locally.
Comment on lines
+391
to
+398
| export const checkoutLegalConsentSchema = z | ||
| .object({ | ||
| termsAccepted: z.boolean(), | ||
| privacyAccepted: z.boolean(), | ||
| termsVersion: checkoutLegalVersionSchema, | ||
| privacyVersion: checkoutLegalVersionSchema, | ||
| }) | ||
| .strict(); |
Contributor
There was a problem hiding this comment.
Validation allows false but DB requires true for consent flags.
checkoutLegalConsentSchema accepts termsAccepted: false and privacyAccepted: false, but the order_legal_consents table has CHECK constraints requiring both to be true (see frontend/db/schema/shop.ts lines 812-819). This mismatch will cause DB insert failures at runtime when users submit with false values.
Consider adding .refine() to enforce true:
Proposed fix
export const checkoutLegalConsentSchema = z
.object({
- termsAccepted: z.boolean(),
- privacyAccepted: z.boolean(),
+ termsAccepted: z.literal(true),
+ privacyAccepted: z.literal(true),
termsVersion: checkoutLegalVersionSchema,
privacyVersion: checkoutLegalVersionSchema,
})
.strict();📝 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
| export const checkoutLegalConsentSchema = z | |
| .object({ | |
| termsAccepted: z.boolean(), | |
| privacyAccepted: z.boolean(), | |
| termsVersion: checkoutLegalVersionSchema, | |
| privacyVersion: checkoutLegalVersionSchema, | |
| }) | |
| .strict(); | |
| export const checkoutLegalConsentSchema = z | |
| .object({ | |
| termsAccepted: z.literal(true), | |
| privacyAccepted: z.literal(true), | |
| termsVersion: checkoutLegalVersionSchema, | |
| privacyVersion: checkoutLegalVersionSchema, | |
| }) | |
| .strict(); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/lib/validation/shop.ts` around lines 391 - 398, The schema
checkoutLegalConsentSchema currently allows termsAccepted and privacyAccepted to
be false which conflicts with the order_legal_consents DB CHECK constraints;
update the schema so both flags must be true (either by changing termsAccepted
and privacyAccepted to z.literal(true) or by adding a .refine() on
checkoutLegalConsentSchema that asserts obj.termsAccepted === true &&
obj.privacyAccepted === true) and include a clear validation error message; keep
checkoutLegalVersionSchema as-is and ensure the validation shape still matches
the DB table constraints.
Comment on lines
+3
to
+5
| const LOCAL_DB_URL = | ||
| 'postgresql://devlovers_local:Gfdtkk43@localhost:5432/devlovers_shop_local_clean?sslmode=disable'; | ||
|
|
Contributor
There was a problem hiding this comment.
Remove hardcoded credentials and secrets from test config.
Line 4 and Line 30-31 embed sensitive values directly in source. Even test secrets should not be committed, and this also increases accidental environment coupling.
🔐 Proposed fix
-const LOCAL_DB_URL =
- 'postgresql://devlovers_local:Gfdtkk43@localhost:5432/devlovers_shop_local_clean?sslmode=disable';
+const LOCAL_DB_URL = process.env.DATABASE_URL_LOCAL?.trim();
+const SHOP_STATUS_TOKEN_SECRET = process.env.SHOP_STATUS_TOKEN_SECRET?.trim();
+
+if (!LOCAL_DB_URL) {
+ throw new Error('DATABASE_URL_LOCAL is required for Playwright E2E.');
+}
+if (!SHOP_STATUS_TOKEN_SECRET) {
+ throw new Error('SHOP_STATUS_TOKEN_SECRET is required for Playwright E2E.');
+}
...
env: {
APP_ENV: 'local',
DATABASE_URL_LOCAL: LOCAL_DB_URL,
DATABASE_URL: '',
SHOP_STRICT_LOCAL_DB: '1',
SHOP_REQUIRED_DATABASE_URL_LOCAL: LOCAL_DB_URL,
- SHOP_STATUS_TOKEN_SECRET:
- 'test_status_token_secret_test_status_token_secret',
+ SHOP_STATUS_TOKEN_SECRET,
NODE_ENV: 'test',
NEXT_TELEMETRY_DISABLED: '1',
},Also applies to: 24-31
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/playwright.config.ts` around lines 3 - 5, The LOCAL_DB_URL constant
contains hardcoded DB credentials; replace its literal value with a runtime
secret source (e.g., read from process.env.LOCAL_DB_URL or a test-specific env
var loaded via dotenv/CI secrets) and remove any other embedded credentials in
this file (refer to occurrences of LOCAL_DB_URL and any other DB/credential
strings) so tests pull credentials from environment/secret storage and fail fast
with a clear error if the env var is missing. Ensure you update any test setup
that referenced the constant to use the new env var and do not commit any
fallback that includes real credentials.
Merged
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
3 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.
Summary by CodeRabbit
New Features
Shop Production Readiness
Performance & Infrastructure