(SP: 3)[Backend][UI] Harden Stripe refund handling & janitor auth; fix Admin Products currency + overflow#120
Conversation
…y with locale-driven currency formatting
… fix admin SALE validation and stabilize tests/cleanup
liudmylasovetovs
requested review from
AM1007 and
ViktorSvertoka
as code owners
✅ Deploy Preview for develop-devlovers ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
📝 WalkthroughWalkthroughThis pull request introduces comprehensive improvements to the shop admin interface and payment processing, including enhanced error handling for refund operations, new JSON validation for product prices, improved Stripe webhook refund handling with better charge resolution, timing-safe string comparison in authentication, currency-aware product listing rendering, and extensive test infrastructure enhancements for better cleanup error visibility. Changes
Sequence Diagram(s)sequenceDiagram
participant Admin as Admin User
participant Refund as RefundButton Component
participant API as Refund API
participant Stripe as Stripe API
participant DB as Database
Admin->>Refund: Click refund button
Refund->>Refund: Set loading state
Refund->>API: POST refund request (try)
alt Network Error
API--x Refund: Network failure
Refund->>Refund: Store NETWORK_ERROR in state
else Success Response
API->>API: Parse JSON response
alt HTTP Error (res.ok false)
API-->>Refund: 400/500 with error code
Refund->>Refund: Display HTTP_<status> error
else HTTP Success
API->>Stripe: Process refund
Stripe-->>API: Refund confirmation
API->>DB: Update order status
API-->>Refund: success: true
Refund->>Refund: Refresh page
end
end
sequenceDiagram
participant Admin as Admin User
participant Form as Product Form
participant API as Products API
participant Validate as Validator
participant DB as Database
Admin->>Form: Submit product with prices
Form->>Form: setIsSubmitting(true)
Form->>API: POST/PATCH with form data
API->>Validate: Parse and validate prices JSON
alt Invalid JSON
Validate-->>API: InvalidPricesJsonError
API-->>Form: 400 {code: INVALID_PRICES_JSON}
Form->>Form: Display error (finally: setIsSubmitting(false))
else Valid JSON
Validate-->>API: Parsed prices
API->>DB: Create/update product
DB-->>API: Success
API-->>Form: 200 OK
Form->>Form: Reset form (finally: setIsSubmitting(false))
end
sequenceDiagram
participant Webhook as Stripe Webhook
participant Handler as Webhook Handler
participant Stripe as Stripe API
participant DB as Database
participant Janitor as Restock Service
Webhook->>Handler: POST charge.refund.updated event
Handler->>Handler: Resolve order from metadata
Handler->>Handler: Extract refundChargeId
alt refund.charge is string
Handler->>Stripe: retrieveCharge(chargeId)
Stripe-->>Handler: Full charge object
Handler->>Handler: Use charge.amount and amount_refunded
else refund.charge is object
Handler->>Handler: Use provided charge object
end
Handler->>Handler: Compute cumulative refunded amount
alt Full refund detected
Handler->>DB: Update order status to refunded
Handler->>Janitor: Trigger restock
Janitor->>DB: Restore inventory
else Partial refund
Handler->>DB: Update metadata, continue monitoring
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
frontend/lib/tests/stripe-webhook-refund-full.test.ts (1)
7-9: AddretrieveChargeto the mock—route.ts imports and calls it.The route handler imports
retrieveChargeat line 6 and calls it at line 632 whenrefund.chargeis a string. The current mock only exportsverifyWebhookSignature, so if any payload sendsrefund.chargeas a string, the handler will crash with "retrieveCharge is not a function". While current test payloads userefund.chargeas an object and avoid this path, the mock should be comprehensive to prevent this class of bugs.Proposed patch
vi.mock('@/lib/psp/stripe', () => ({ verifyWebhookSignature: vi.fn(), + retrieveCharge: vi.fn(), }));frontend/lib/tests/stripe-webhook-paid-status-repair.test.ts (1)
29-64: Usevi.stubEnv()to safely manage environment variables incallWebhook().Lines 52–53 mutate
process.env.STRIPE_SECRET_KEYandprocess.env.STRIPE_WEBHOOK_SECRETwithout restoration, risking cross-test coupling. TheafterEachhook only cleans up database records, not environment variables.Replace the manual mutations with
vi.stubEnv(), which Vitest provides for automatic cleanup:async function callWebhook(params: { eventId: string; pi: string; orderId: string }) { vi.resetModules(); vi.doMock('@/lib/psp/stripe', async () => { const actual = await vi.importActual<any>('@/lib/psp/stripe'); return { ...actual, verifyWebhookSignature: () => ({ id: params.eventId, type: 'payment_intent.succeeded', data: { object: { id: params.pi, amount_received: 2500, currency: 'usd', status: 'succeeded', metadata: { orderId: params.orderId }, latest_charge: null, }, }, }), }; }); vi.stubEnv('STRIPE_SECRET_KEY', 'sk_test_dummy'); vi.stubEnv('STRIPE_WEBHOOK_SECRET', 'whsec_test_dummy'); const { POST } = await import('@/app/api/shop/webhooks/stripe/route'); return POST( new Request('http://localhost/api/shop/webhooks/stripe', { method: 'POST', headers: { 'stripe-signature': 't=0,v1=deadbeef' }, body: JSON.stringify({ id: params.eventId }), }) as any ); }frontend/app/api/shop/webhooks/stripe/route.ts (1)
304-316: LoadstockRestoredandinventoryStatuswhen querying the order; gaterestockOrder()call by these fields to prevent unnecessary invocations on different Stripe events.The current code is protected by
restockOrder()'s internal guards (which checkstockRestoredandinventoryStatus), so a second different Stripe event will not cause double stock increments. However, it will unnecessarily callrestockOrder()again, relying on that function's internal checks rather than preventing the call at the webhook level.Loading and checking these fields explicitly makes intent clear and avoids redundant function calls:
Proposed change
const [order] = await db .select({ id: orders.id, paymentIntentId: orders.paymentIntentId, totalAmountMinor: orders.totalAmountMinor, currency: orders.currency, paymentStatus: orders.paymentStatus, status: orders.status, + stockRestored: orders.stockRestored, + inventoryStatus: orders.inventoryStatus, }) .from(orders) .where(eq(orders.id, resolvedOrderId)) .limit(1);Then after the refund update:
await db .update(orders) .set({ updatedAt: new Date(), paymentStatus: 'refunded', status: 'CANCELED', pspChargeId: charge?.id ?? refundChargeId ?? null, pspPaymentMethod: resolvePaymentMethod(paymentIntent, charge), pspStatusReason: refund?.reason ?? refund?.status ?? 'refunded', pspMetadata: buildPspMetadata({ eventType, paymentIntent, charge: charge ?? undefined, refund, }), }) .where( and(eq(orders.id, order.id), ne(orders.paymentStatus, 'refunded')) ); - await restockOrder(order.id, { reason: 'refunded' }); + if (!order.stockRestored && order.inventoryStatus !== 'released') { + await restockOrder(order.id, { reason: 'refunded' }); + }
🤖 Fix all issues with AI agents
In @frontend/lib/psp/stripe.ts:
- Around line 91-109: retrievePaymentIntent lacks the non-empty ID validation
present in retrieveCharge; update the retrievePaymentIntent function to check
that paymentIntentId is provided and not just whitespace (e.g., if
(!paymentIntentId || paymentIntentId.trim().length === 0) throw new
Error('STRIPE_INVALID_PAYMENT_INTENT_ID')), perform this check after obtaining
paymentsEnabled/stripe and before calling stripe.paymentIntents.retrieve, and
keep the existing try/catch logging and error throw behavior consistent with
retrieveCharge.
In @frontend/lib/tests/checkout-concurrency-stock1.test.ts:
- Around line 16-25: The test imports the route handler POST (checkoutPOST)
before installing the vitest mock for '@/lib/auth', so the real module may be
loaded/cached; move the vi.mock(...) block to appear before the import of
checkoutPOST (i.e., define vi.mock('@/lib/auth', ...) first, then import { POST
as checkoutPOST } from '@/app/api/shop/checkout/route') to ensure the mock is
installed prior to the route handler and its transitive dependencies loading the
real module.
🧹 Nitpick comments (19)
.gitignore (1)
68-68: Redundant backup patterns in gitignore.Lines 68 and 73 (
frontend/.env.bak) are redundant with line 72 (frontend/.env*.bak), which already matches any.env*.bakfile in the frontend directory. Consider removing the exact duplicates to improve clarity.♻️ Suggested cleanup
# Documentation (development only) .claude/ CLAUDE.md frontend/docs/ - frontend/.env.bak # local env backups frontend/.env*.bak - frontend/.env.bakAdditionally, verify that the
tmp/directory ignore pattern mentioned in the PR objectives is included elsewhere in the file or should be added here.Also applies to: 72-73
frontend/lib/tests/restock-sweep-claim.test.ts (1)
66-75: LGTM! Improved cleanup error visibility.The structured error logging in the cleanup catch block improves observability without masking the original test result. The context provided (file, test, step, orderId, idem, error) is helpful for debugging cleanup failures.
Optional: Consider a shared helper for consistency
Since similar cleanup logging patterns appear across multiple test files in this PR, you might consider extracting a shared helper function to ensure consistency:
// test-utils.ts export function logTestCleanupFailure(context: { file: string; test: string; step: string; error: unknown; [key: string]: unknown; }) { console.error('[test cleanup failed]', context); }Then use it across test files:
} catch (error) { - console.error('[test cleanup failed]', { - file: 'restock-sweep-claim.test.ts', - test: 'two concurrent sweeps must not both process the same order', - step: 'delete order by id', - orderId, - idem, - error, - }); + logTestCleanupFailure({ + file: 'restock-sweep-claim.test.ts', + test: 'two concurrent sweeps must not both process the same order', + step: 'delete order by id', + orderId, + idem, + error, + }); }This would make it easier to evolve the logging format (e.g., add timestamps, send to a test logger) consistently across all tests.
frontend/app/[locale]/shop/admin/products/page.tsx (1)
15-21: Consider logging errors in the catch block for debugging.The silent catch loses error details that could help diagnose issues with malformed price data.
🔧 Suggested improvement
function safeFromDbMoney(value: unknown): number | null { try { return fromDbMoney(value); - } catch { + } catch (err) { + console.warn('safeFromDbMoney: failed to convert', value, err); return null; } }frontend/lib/tests/restock-stale-stripe-orphan.test.ts (1)
80-94: Cleanup failure logging is a good upgrade (no longer silently swallowed).
Optional: if you already introduced a shared helper (e.g.,logTestCleanupFailed) elsewhere in the PR, consider using it here too for consistency and to avoid per-test ad-hoc shapes.frontend/scripts/shop-janitor-restock-stale.mjs (1)
17-33: Timeout parsing hardening looks solid; consider stricter numeric validation (and optionally a max clamp).
Number.parseInt()will accept25000abcas25000; if you want to treat that as invalid, validate with something like/^\d+$/before parsing.Proposed tweak (stricter parsing)
const DEFAULT_TIMEOUT_MS = 25_000; const MIN_TIMEOUT_MS = 1_000; const rawTimeout = (process.env.JANITOR_TIMEOUT_MS ?? '').trim(); -const n = Number.parseInt(rawTimeout, 10); +const n = /^\d+$/.test(rawTimeout) ? Number.parseInt(rawTimeout, 10) : Number.NaN; // NaN / '' / abc / 0 / negative -> default const timeoutMs = Number.isFinite(n) && n > 0 ? Math.max(MIN_TIMEOUT_MS, n) : DEFAULT_TIMEOUT_MS;frontend/lib/tests/checkout-concurrency-stock1.test.ts (1)
237-263: Cleanup catch is still silent; recommend logging (without failing the test).
This makes DB pollution much easier to diagnose when something changes.Suggested cleanup logging
} catch { - // best-effort cleanup + console.error('[test cleanup failed]', { + file: 'checkout-concurrency-stock1.test.ts', + test: 'stock=1 two parallel checkouts', + step: 'cleanup', + productId, + }); }frontend/lib/tests/order-items-variants.test.ts (1)
95-120: Good cleanup observability; consider centralizing the logger to keep test output consistent.The structured payload is useful. Since other tests introduce
logTestCleanupFailed(...), consider reusing a shared helper (or importing one) to avoid drift in fields/message across files.frontend/lib/tests/restock-stale-claim-gate.test.ts (1)
74-89: Cleanup logging + explicit timeouts look like a net stability win.If cleanup failures are frequent, consider a shared helper (like
logTestCleanupFailed) to standardize fields across suites.Also applies to: 141-156
frontend/app/api/shop/admin/products/route.ts (1)
21-31: AvoidanyinisInvalidPricesJsonError(easy type-safe guard).This can be a simple discriminant check without casting.
Proposed diff
function isInvalidPricesJsonError( value: SaleRuleViolation | InvalidPricesJsonError | null ): value is InvalidPricesJsonError { - return !!value && (value as any).code === 'INVALID_PRICES_JSON'; + return ( + !!value && + typeof value === 'object' && + 'code' in value && + (value as { code?: string }).code === 'INVALID_PRICES_JSON' + ); }frontend/app/api/shop/admin/products/[id]/route.ts (1)
27-37: Same as the other route: avoidanyinisInvalidPricesJsonError.Keeps this logic safer and easier to refactor later.
frontend/lib/tests/checkout-no-payments.test.ts (1)
36-42: Centralized cleanup logger is a good pattern.If this shows up in many suites, consider moving it to a shared test util to keep the payload schema consistent.
frontend/lib/tests/restock-order-only-once.test.ts (1)
156-179: Consider aligning cleanup error handling with the pattern in restock-stuck-reserving-sweep.test.ts.The cleanup error handling here logs failures but doesn't preserve or rethrow the original test error. In contrast,
restock-stuck-reserving-sweep.test.ts(lines 51, 168-169, 173-185, 187-188) tracksoriginalErrorto ensure:
- If a test fails, the test error is surfaced (not masked by cleanup errors)
- If cleanup fails with no test error, the cleanup error becomes the failure
This approach is more robust and ensures cleanup problems don't go unnoticed when tests pass.
♻️ Align with the originalError pattern
For each test, add error tracking:
it('duplicate failed restock must not increment stock twice and must not change restocked_at', async () => { const orderId = crypto.randomUUID(); const productId = crypto.randomUUID(); const slug = `test-${crypto.randomUUID()}`; const sku = `sku-${crypto.randomUUID().slice(0, 8)}`; const initialStock = 5; const qty = 2; const createdAt = new Date(Date.now() - 2 * 60 * 60 * 1000); const idem = `test-restock-${crypto.randomUUID()}`; + let originalError: unknown = null; try { // ... test body ... + } catch (e) { + originalError = e; } finally { try { await db.delete(orders).where(eq(orders.id, orderId)); } catch (error) { - logCleanupFailed({ - test: 'restockOrder: duplicate failed restock', - orderId, - productId, - step: 'delete orders', - error, - }); + if (originalError) { + logCleanupFailed({ + test: 'restockOrder: duplicate failed restock', + orderId, + productId, + step: 'delete orders', + error, + }); + } else { + originalError = error; + } } try { await db.delete(products).where(eq(products.id, productId)); } catch (error) { - logCleanupFailed({ - test: 'restockOrder: duplicate failed restock', - orderId, - productId, - step: 'delete products', - error, - }); + if (originalError) { + logCleanupFailed({ + test: 'restockOrder: duplicate failed restock', + orderId, + productId, + step: 'delete products', + error, + }); + } else { + originalError = error; + } } } + if (originalError) throw originalError; }, 30_000);Apply the same pattern to the other two tests.
Also applies to: 274-297, 428-451
frontend/lib/tests/stripe-webhook-psp-fields.test.ts (1)
49-108: Consider tracking originalError to prevent cleanup from masking test failures.The cleanup function logs errors per step but doesn't integrate with test error tracking. Similar to the pattern in
restock-stuck-reserving-sweep.test.ts, consider wrapping the test body in a try/catch to captureoriginalError, then check it in cleanup:let originalError: unknown = null; try { // test body } catch (e) { originalError = e; } finally { await cleanup({ orderId, productId, eventId }, originalError); } if (originalError) throw originalError;And update
cleanupto conditionally log vs. throw:async function cleanup(params: {...}, originalError?: unknown) { // ... for each deletion: try { await db.delete(...); } catch (e) { if (originalError) { logTestCleanupFailed({...}, e); } else { throw e; // or assign to originalError if passed by reference } } }This ensures cleanup problems don't go unnoticed when tests pass, and test failures aren't masked by cleanup errors.
frontend/lib/tests/public-product-visibility.test.ts (1)
17-31: Align cleanup error handling with the originalError pattern.The cleanup function logs errors but doesn't coordinate with test error tracking. Following the pattern in
restock-stuck-reserving-sweep.test.ts, consider:
- Adding
let originalError: unknown = null;before the try block- Wrapping test assertions in
try/catchto captureoriginalError- Updating cleanup to throw (or assign) on error only if
!originalError- Re-throwing
originalErrorafter cleanupThis prevents cleanup failures from masking test failures and ensures cleanup problems are surfaced when tests pass.
frontend/lib/tests/stripe-webhook-paid-status-repair.test.ts (2)
66-109: Cleanup logic is OK, but consider supporting multiple artifacts per test.
lastOrderId/lastEventIdonly tracks one pair; if a future edit creates multiple events/orders in one test, leftovers will be silent (only logged). Consider storing an array of cleanup targets instead of singletons.
111-173: Prefer explicit “row exists” assertions overrow!to keep failures actionable.
Beforeexpect(row!.…)(Line 132-134, 170-171), addexpect(row).toBeTruthy()/expect(row).toBeDefined()so failures read as “missing row” rather than a non-null assertion crash.frontend/lib/tests/stripe-webhook-refund-full.test.ts (2)
91-94: Avoid cleanup masking the original failure.
IfcleanupInserted()throws, Vitest will report the afterEach failure and can hide the real assertion that failed. Consider matching the “log cleanup failure” pattern used in the other test file.
355-422: Good coverage for “don’t userefund.amountto decide fullness”, but fallback-sum path isn’t tested.
This scenario setscharge.amount_refunded: 2500(Line 388-389), so it doesn’t validate the “sumcharge.refunds.data[].amount” fallback. Consider adding a variant whereamount_refundedis absent/null to force the fallback branch.frontend/app/api/shop/webhooks/stripe/route.ts (1)
236-266: In-flight duplicate delivery can still double-process beforeprocessedAtis set.
If Stripe delivers the sameevent.idconcurrently, request B can observeprocessedAt=NULLand re-run side effects while request A is still running (Line 251-266). If you’ve seen this in practice, consider a “claim” update (e.g.,processingStartedAt) or a conditional update as a lock (single-writer) before doing side effects.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (42)
.gitignorefrontend/app/[locale]/shop/admin/layout.tsxfrontend/app/[locale]/shop/admin/orders/[id]/RefundButton.tsxfrontend/app/[locale]/shop/admin/orders/[id]/page.tsxfrontend/app/[locale]/shop/admin/page.tsxfrontend/app/[locale]/shop/admin/products/_components/product-form.tsxfrontend/app/[locale]/shop/admin/products/page.tsxfrontend/app/[locale]/shop/checkout/payment/StripePaymentClient.tsxfrontend/app/api/shop/admin/products/[id]/route.tsfrontend/app/api/shop/admin/products/route.tsfrontend/app/api/shop/cart/rehydrate/route.tsfrontend/app/api/shop/checkout/route.tsfrontend/app/api/shop/internal/orders/restock-stale/route.tsfrontend/app/api/shop/webhooks/stripe/route.tsfrontend/components/shop/admin/admin-product-status-toggle.tsxfrontend/components/shop/header/nav-links.tsxfrontend/db/queries/shop/products.tsfrontend/lib/auth/internal-janitor.tsfrontend/lib/logging.tsfrontend/lib/psp/stripe.tsfrontend/lib/services/orders.tsfrontend/lib/services/products.tsfrontend/lib/shop/data.tsfrontend/lib/tests/admin-product-sale-contract.test.tsfrontend/lib/tests/checkout-concurrency-stock1.test.tsfrontend/lib/tests/checkout-currency-policy.test.tsfrontend/lib/tests/checkout-no-payments.test.tsfrontend/lib/tests/order-items-snapshot-immutable.test.tsfrontend/lib/tests/order-items-variants.test.tsfrontend/lib/tests/product-sale-invariant.test.tsfrontend/lib/tests/public-product-visibility.test.tsfrontend/lib/tests/restock-order-only-once.test.tsfrontend/lib/tests/restock-stale-claim-gate.test.tsfrontend/lib/tests/restock-stale-stripe-orphan.test.tsfrontend/lib/tests/restock-stuck-reserving-sweep.test.tsfrontend/lib/tests/restock-sweep-claim.test.tsfrontend/lib/tests/stripe-webhook-paid-status-repair.test.tsfrontend/lib/tests/stripe-webhook-psp-fields.test.tsfrontend/lib/tests/stripe-webhook-refund-full.test.tsfrontend/scripts/shop-janitor-restock-stale.mjsfrontend/tmp/replay-charge-refunded.jsonfrontend/tmp/replay-stripe-webhook.js
💤 Files with no reviewable changes (8)
- frontend/app/api/shop/checkout/route.ts
- frontend/app/api/shop/cart/rehydrate/route.ts
- frontend/lib/shop/data.ts
- frontend/tmp/replay-charge-refunded.json
- frontend/app/[locale]/shop/checkout/payment/StripePaymentClient.tsx
- frontend/tmp/replay-stripe-webhook.js
- frontend/app/api/shop/internal/orders/restock-stale/route.ts
- frontend/app/[locale]/shop/admin/products/_components/product-form.tsx
🧰 Additional context used
🧬 Code graph analysis (7)
frontend/app/[locale]/shop/admin/orders/[id]/RefundButton.tsx (1)
frontend/scripts/shop-janitor-restock-stale.mjs (1)
res(35-45)
frontend/lib/tests/checkout-no-payments.test.ts (2)
frontend/db/index.ts (1)
db(39-39)frontend/db/schema/shop.ts (3)
products(56-105)productPrices(278-332)orders(107-198)
frontend/lib/tests/public-product-visibility.test.ts (2)
frontend/db/index.ts (1)
db(39-39)frontend/db/queries/shop/products.ts (1)
getPublicProductBySlug(248-261)
frontend/db/queries/shop/products.ts (2)
frontend/lib/validation/shop.ts (1)
CurrencyCode(32-32)frontend/lib/shop/currency.ts (1)
CurrencyCode(2-2)
frontend/lib/tests/admin-product-sale-contract.test.ts (2)
frontend/app/api/shop/admin/products/route.ts (1)
POST(79-215)frontend/app/api/shop/admin/products/[id]/route.ts (1)
PATCH(128-282)
frontend/lib/tests/restock-order-only-once.test.ts (2)
frontend/db/index.ts (1)
db(39-39)frontend/db/schema/shop.ts (1)
products(56-105)
frontend/lib/tests/stripe-webhook-paid-status-repair.test.ts (2)
frontend/db/index.ts (1)
db(39-39)frontend/db/schema/shop.ts (2)
stripeEvents(262-276)orders(107-198)
🪛 Biome (2.1.2)
frontend/lib/tests/order-items-snapshot-immutable.test.ts
[error] 215-215: Unsafe usage of 'throw'.
'throw' in 'finally' overwrites the control flow statements inside 'try' and 'catch'.
(lint/correctness/noUnsafeFinally)
🔇 Additional comments (32)
frontend/lib/services/orders.ts (1)
1111-1111: Comment clarification improves readability.The updated comment at line 1111 clearly distinguishes the Stripe/paid-provider orphan handling from the preceding no-payment orphan logic. This clarification helps readers understand why the
reason === 'stale'condition applies specifically to non-none providers. No logic changes; the code behavior remains unchanged and correct.frontend/lib/auth/internal-janitor.ts (1)
5-25: Well-implemented timing-safe comparison.The padding approach correctly mitigates timing attacks by:
- Always executing
crypto.timingSafeEqualon equal-length buffers regardless of input sizes.- Deferring the length check to after the constant-time comparison.
- Handling the edge case where
crypto.timingSafeEqualrequires non-zero length buffers viaMath.max(..., 1).frontend/lib/tests/product-sale-invariant.test.ts (1)
50-50: LGTM! Appropriate timeout extensions for async DB tests.The 30-second timeouts align with the PR's test stability improvements and are reasonable for tests involving multiple database operations and product invariant validation.
Also applies to: 116-116
frontend/db/queries/shop/products.ts (1)
117-117: LGTM! Non-functional comment cleanup.The removal of developer comments has no impact on functionality and aligns with the PR's repo hygiene objectives.
Also applies to: 200-200
frontend/lib/services/products.ts (1)
577-577: LGTM! Comment translation completed.The removal of the Ukrainian phrase aligns with the PR's goal to translate non-English comments. No functional changes.
frontend/lib/tests/checkout-currency-policy.test.ts (1)
248-248: LGTM! Timeout extension for DB and API test.The 30-second timeout is consistent with other test files in this PR and appropriate for this integration test involving database seeding and checkout API validation.
frontend/app/[locale]/shop/admin/orders/[id]/RefundButton.tsx (2)
19-31: Excellent network error handling added.The try/catch block properly captures fetch failures (network errors, DNS issues, etc.) and surfaces them to the user via error state, preventing unhandled promise rejections. The error message extraction with fallback to 'NETWORK_ERROR' is appropriate.
57-66: LGTM! Formatting improvements enhance readability.The multiline conditional for the button title and consolidated error span improve code readability without affecting behavior.
frontend/app/[locale]/shop/admin/layout.tsx (1)
31-31: LGTM!The wider container (
max-w-7xl) with responsive horizontal padding provides consistent layout real estate across admin pages, aligning with the related changes in other admin views.frontend/app/[locale]/shop/admin/page.tsx (1)
6-7: LGTM!Consistent layout adjustment matching the parent layout component.
frontend/app/[locale]/shop/admin/orders/[id]/page.tsx (2)
21-27: LGTM!Removing the
as CurrencyCodecast in favor of the narrow union literal return is cleaner and type-safe—TypeScript can infer that'UAH' | 'USD'satisfiesCurrencyCode.
43-46: LGTM!Formatting-only change;
canRefundlogic is unchanged.frontend/app/[locale]/shop/admin/products/page.tsx (4)
33-55: LGTM!The
leftJoincorrectly fetches prices for the locale's currency, falling back tonullfor products without a price in that currency. The query structure is clean and efficient.
70-106: LGTM!The
table-fixedlayout with explicit column widths (totaling 100%) prevents overflow issues. The column distribution is reasonable for the content types.
153-169: LGTM!Good use of
aria-labelattributes on the status badges for accessibility. The labels clearly convey the state to screen readers.
110-131: No action needed—the conversion logic is correct.The
fromDbMoneyfunction properly handles the major-unit storage format. It receives legacy numeric values like "12.34" (major units) and correctly converts them to canonical cents (1234) viatoCents(), which multiplies by 100. This matches the storage format documented in the migration and page comment, so amounts will display correctly.frontend/components/shop/admin/admin-product-status-toggle.tsx (2)
46-65: LGTM!Good improvements:
min-w-0on the container prevents flex overflow in table cells.- Truncated error with
titletooltip provides a clean UI while preserving the full message on hover.aria-live="polite"ensures screen readers announce errors appropriately.
18-43: LGTM!The toggle flow correctly manages loading state, error reset, and optimistic UI updates. The error handling is appropriate for user feedback.
frontend/lib/logging.ts (2)
1-14: No functional change; prod vs non-prod behavior unchanged.
16-19: Considerconsole.warnfor warnings (if you want correct severity in logs).
[ suggest_optional_refactor]frontend/components/shop/header/nav-links.tsx (1)
31-35: No behavior change here (comment-only).frontend/app/api/shop/admin/products/route.ts (1)
62-77: Nice: explicit 400 for invalidpricesJSON instead of silently passing validation.Error payload is clear and consistent (
code+field).Also applies to: 95-105
frontend/app/api/shop/admin/products/[id]/route.ts (1)
69-85: Good: PATCH now fails fast with a clear 400 whenpricesJSON is invalid.This prevents “silent accept + confusing downstream errors”.
Also applies to: 147-159
frontend/lib/tests/checkout-no-payments.test.ts (1)
99-113: Cleanup paths now emit actionable context instead of warn/silence—nice.This should make DB pollution issues much easier to diagnose in CI.
Also applies to: 129-157, 210-242
frontend/lib/tests/restock-order-only-once.test.ts (1)
25-33: LGTM: Cleanup failure logging utility.The
logCleanupFailedfunction provides structured error logging for test cleanup operations, which will help diagnose issues without masking actual test failures.frontend/lib/tests/restock-stuck-reserving-sweep.test.ts (1)
51-51: Excellent pattern: cleanup errors don't mask test failures.This implementation correctly tracks the original test error and ensures:
- Cleanup failures are logged without hiding the actual test failure
- If no test failure occurred, cleanup errors are surfaced as the failure
This is the gold standard for test cleanup error handling.
Also applies to: 168-185, 187-188
frontend/lib/tests/stripe-webhook-psp-fields.test.ts (1)
29-35: LGTM: Centralized cleanup failure logging.The
logTestCleanupFailedhelper provides consistent, structured logging with file context.frontend/lib/tests/public-product-visibility.test.ts (2)
9-15: LGTM: Cleanup logging utility.Centralized logging with file context is helpful for debugging.
1-128: Approve quote style consistency.The shift from double quotes to single quotes throughout the file improves consistency with the project's style preferences.
frontend/lib/tests/admin-product-sale-contract.test.ts (3)
47-61: LGTM: Support for testing invalid JSON.The addition of
pricesRawallows passing malformed JSON strings directly, enabling validation of early JSON parsing error handling. The conditional logic is correct and preserves backward compatibility.
161-184: LGTM: POST invalid prices JSON validation.This test correctly verifies that malformed prices JSON is caught early (before
parseAdminProductFormis called) and returns a standardized error response with codeINVALID_PRICES_JSON. The mock assertion ensures the validation happens at the right layer.
186-215: LGTM: PATCH invalid prices JSON validation.This test mirrors the POST validation test for the PATCH endpoint, correctly verifying early JSON parsing error handling with the same standardized error contract. The route context with async params is properly handled.
| if (eventType === 'charge.refunded') { | ||
| const effectiveCharge = charge; | ||
| const amt = | ||
| typeof (charge as any).amount === 'number' | ||
| ? (charge as any).amount | ||
| typeof (effectiveCharge as any)?.amount === 'number' | ||
| ? (effectiveCharge as any).amount | ||
| : null; | ||
| const refunded = | ||
| typeof (charge as any).amount_refunded === 'number' | ||
| ? (charge as any).amount_refunded | ||
| typeof (effectiveCharge as any)?.amount_refunded === 'number' | ||
| ? (effectiveCharge as any).amount_refunded | ||
| : null; | ||
|
|
||
| isFullRefund = amt != null && refunded != null && refunded === amt; | ||
| } else if (eventType === 'charge.refund.updated' && refund) { | ||
| const refundAmt = | ||
| typeof (refund as any).amount === 'number' | ||
| ? (refund as any).amount | ||
| // Ensure we have the Charge to compute cumulative refunded correctly. | ||
| let effectiveCharge: Stripe.Charge | undefined; | ||
|
|
||
| if (typeof refund.charge === 'object' && refund.charge) { | ||
| effectiveCharge = refund.charge as Stripe.Charge; | ||
| } else if (typeof refund.charge === 'string' && refund.charge.trim()) { | ||
| // Critical: fetch charge to get full refunds list | ||
| effectiveCharge = await retrieveCharge(refund.charge.trim()); | ||
| } | ||
|
|
||
| const amt = | ||
| typeof (effectiveCharge as any)?.amount === 'number' | ||
| ? (effectiveCharge as any).amount | ||
| : null; | ||
|
|
||
| let cumulativeRefunded: number | null = | ||
| typeof (effectiveCharge as any)?.amount_refunded === 'number' | ||
| ? (effectiveCharge as any).amount_refunded | ||
| : null; | ||
| isFullRefund = | ||
| refundAmt != null && refundAmt === order.totalAmountMinor; | ||
|
|
||
| // Fallback: sum refunds list if present; include current refund if not in list yet | ||
| if ( | ||
| cumulativeRefunded == null && | ||
| Array.isArray((effectiveCharge as any)?.refunds?.data) | ||
| ) { | ||
| const list = (effectiveCharge as any).refunds.data as any[]; | ||
| const sumFromList = list.reduce((sum, r) => { | ||
| const a = typeof r?.amount === 'number' ? r.amount : 0; | ||
| return sum + a; | ||
| }, 0); | ||
|
|
||
| const currentAmt = | ||
| typeof (refund as any).amount === 'number' | ||
| ? (refund as any).amount | ||
| : 0; | ||
|
|
||
| const hasCurrent = list.some(r => r?.id && r.id === refund.id); | ||
|
|
||
| cumulativeRefunded = sumFromList + (hasCurrent ? 0 : currentAmt); | ||
| } | ||
|
|
||
| // If still unknown -> fail to force retry (better than silently ignoring full refund) | ||
| if (amt == null || cumulativeRefunded == null) { | ||
| throw new Error('REFUND_FULLNESS_UNDETERMINED'); | ||
| } | ||
|
|
||
| isFullRefund = cumulativeRefunded === amt; | ||
|
|
||
| // Prefer charge id from effectiveCharge for PSP fields | ||
| if (effectiveCharge?.id) { | ||
| // override local charge variable for downstream pspChargeId/metadata usage | ||
| charge = effectiveCharge; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd frontend && cat -n app/api/shop/webhooks/stripe/route.ts | sed -n '605,685p'Repository: DevLoversTeam/devlovers.net
Length of output: 3726
Add fallback logic to charge.refunded path to sum refunds when amount_refunded is missing.
The charge.refunded path (lines 612–623) determines full refunds by checking amount_refunded directly, but silently defaults to false if that field is missing. The charge.refund.updated path (lines 624–678) already implements a defensive fallback by summing the refunds.data list when amount_refunded is unavailable, and throws an error if fullness cannot be determined.
This inconsistency means that if Stripe ever omits amount_refunded on a charge.refunded event, the full refund will be silently ignored with no error, potentially causing incorrect payment status updates.
Proposed patch
if (eventType === 'charge.refunded') {
const effectiveCharge = charge;
const amt =
typeof (effectiveCharge as any)?.amount === 'number'
? (effectiveCharge as any).amount
: null;
- const refunded =
+ let refunded: number | null =
typeof (effectiveCharge as any)?.amount_refunded === 'number'
? (effectiveCharge as any).amount_refunded
: null;
+
+ if (
+ refunded == null &&
+ Array.isArray((effectiveCharge as any)?.refunds?.data)
+ ) {
+ const list = (effectiveCharge as any).refunds.data as any[];
+ refunded = list.reduce((sum, r) => {
+ const a = typeof r?.amount === 'number' ? r.amount : 0;
+ return sum + a;
+ }, 0);
+ }
isFullRefund = amt != null && refunded != null && refunded === amt;
}📝 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.
| if (eventType === 'charge.refunded') { | |
| const effectiveCharge = charge; | |
| const amt = | |
| typeof (charge as any).amount === 'number' | |
| ? (charge as any).amount | |
| typeof (effectiveCharge as any)?.amount === 'number' | |
| ? (effectiveCharge as any).amount | |
| : null; | |
| const refunded = | |
| typeof (charge as any).amount_refunded === 'number' | |
| ? (charge as any).amount_refunded | |
| typeof (effectiveCharge as any)?.amount_refunded === 'number' | |
| ? (effectiveCharge as any).amount_refunded | |
| : null; | |
| isFullRefund = amt != null && refunded != null && refunded === amt; | |
| } else if (eventType === 'charge.refund.updated' && refund) { | |
| const refundAmt = | |
| typeof (refund as any).amount === 'number' | |
| ? (refund as any).amount | |
| // Ensure we have the Charge to compute cumulative refunded correctly. | |
| let effectiveCharge: Stripe.Charge | undefined; | |
| if (typeof refund.charge === 'object' && refund.charge) { | |
| effectiveCharge = refund.charge as Stripe.Charge; | |
| } else if (typeof refund.charge === 'string' && refund.charge.trim()) { | |
| // Critical: fetch charge to get full refunds list | |
| effectiveCharge = await retrieveCharge(refund.charge.trim()); | |
| } | |
| const amt = | |
| typeof (effectiveCharge as any)?.amount === 'number' | |
| ? (effectiveCharge as any).amount | |
| : null; | |
| let cumulativeRefunded: number | null = | |
| typeof (effectiveCharge as any)?.amount_refunded === 'number' | |
| ? (effectiveCharge as any).amount_refunded | |
| : null; | |
| isFullRefund = | |
| refundAmt != null && refundAmt === order.totalAmountMinor; | |
| // Fallback: sum refunds list if present; include current refund if not in list yet | |
| if ( | |
| cumulativeRefunded == null && | |
| Array.isArray((effectiveCharge as any)?.refunds?.data) | |
| ) { | |
| const list = (effectiveCharge as any).refunds.data as any[]; | |
| const sumFromList = list.reduce((sum, r) => { | |
| const a = typeof r?.amount === 'number' ? r.amount : 0; | |
| return sum + a; | |
| }, 0); | |
| const currentAmt = | |
| typeof (refund as any).amount === 'number' | |
| ? (refund as any).amount | |
| : 0; | |
| const hasCurrent = list.some(r => r?.id && r.id === refund.id); | |
| cumulativeRefunded = sumFromList + (hasCurrent ? 0 : currentAmt); | |
| } | |
| // If still unknown -> fail to force retry (better than silently ignoring full refund) | |
| if (amt == null || cumulativeRefunded == null) { | |
| throw new Error('REFUND_FULLNESS_UNDETERMINED'); | |
| } | |
| isFullRefund = cumulativeRefunded === amt; | |
| // Prefer charge id from effectiveCharge for PSP fields | |
| if (effectiveCharge?.id) { | |
| // override local charge variable for downstream pspChargeId/metadata usage | |
| charge = effectiveCharge; | |
| } | |
| } | |
| if (eventType === 'charge.refunded') { | |
| const effectiveCharge = charge; | |
| const amt = | |
| typeof (effectiveCharge as any)?.amount === 'number' | |
| ? (effectiveCharge as any).amount | |
| : null; | |
| let refunded: number | null = | |
| typeof (effectiveCharge as any)?.amount_refunded === 'number' | |
| ? (effectiveCharge as any).amount_refunded | |
| : null; | |
| if ( | |
| refunded == null && | |
| Array.isArray((effectiveCharge as any)?.refunds?.data) | |
| ) { | |
| const list = (effectiveCharge as any).refunds.data as any[]; | |
| refunded = list.reduce((sum, r) => { | |
| const a = typeof r?.amount === 'number' ? r.amount : 0; | |
| return sum + a; | |
| }, 0); | |
| } | |
| isFullRefund = amt != null && refunded != null && refunded === amt; | |
| } else if (eventType === 'charge.refund.updated' && refund) { | |
| // Ensure we have the Charge to compute cumulative refunded correctly. | |
| let effectiveCharge: Stripe.Charge | undefined; | |
| if (typeof refund.charge === 'object' && refund.charge) { | |
| effectiveCharge = refund.charge as Stripe.Charge; | |
| } else if (typeof refund.charge === 'string' && refund.charge.trim()) { | |
| // Critical: fetch charge to get full refunds list | |
| effectiveCharge = await retrieveCharge(refund.charge.trim()); | |
| } | |
| const amt = | |
| typeof (effectiveCharge as any)?.amount === 'number' | |
| ? (effectiveCharge as any).amount | |
| : null; | |
| let cumulativeRefunded: number | null = | |
| typeof (effectiveCharge as any)?.amount_refunded === 'number' | |
| ? (effectiveCharge as any).amount_refunded | |
| : null; | |
| // Fallback: sum refunds list if present; include current refund if not in list yet | |
| if ( | |
| cumulativeRefunded == null && | |
| Array.isArray((effectiveCharge as any)?.refunds?.data) | |
| ) { | |
| const list = (effectiveCharge as any).refunds.data as any[]; | |
| const sumFromList = list.reduce((sum, r) => { | |
| const a = typeof r?.amount === 'number' ? r.amount : 0; | |
| return sum + a; | |
| }, 0); | |
| const currentAmt = | |
| typeof (refund as any).amount === 'number' | |
| ? (refund as any).amount | |
| : 0; | |
| const hasCurrent = list.some(r => r?.id && r.id === refund.id); | |
| cumulativeRefunded = sumFromList + (hasCurrent ? 0 : currentAmt); | |
| } | |
| // If still unknown -> fail to force retry (better than silently ignoring full refund) | |
| if (amt == null || cumulativeRefunded == null) { | |
| throw new Error('REFUND_FULLNESS_UNDETERMINED'); | |
| } | |
| isFullRefund = cumulativeRefunded === amt; | |
| // Prefer charge id from effectiveCharge for PSP fields | |
| if (effectiveCharge?.id) { | |
| // override local charge variable for downstream pspChargeId/metadata usage | |
| charge = effectiveCharge; | |
| } | |
| } |
| export async function retrieveCharge(chargeId: string): Promise<Stripe.Charge> { | ||
| const { paymentsEnabled } = getStripeEnv(); | ||
| const stripe = getStripeClient(); | ||
|
|
||
| if (!paymentsEnabled || !stripe) { | ||
| throw new Error('STRIPE_DISABLED'); | ||
| } | ||
|
|
||
| if (!chargeId || chargeId.trim().length === 0) { | ||
| throw new Error('STRIPE_INVALID_CHARGE_ID'); | ||
| } | ||
|
|
||
| try { | ||
| return await stripe.charges.retrieve(chargeId); | ||
| } catch (error) { | ||
| logError('Stripe charge retrieval failed', error); | ||
| throw new Error('STRIPE_CHARGE_RETRIEVE_FAILED'); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find package.json and check Stripe version
find . -name "package.json" -type f | head -5Repository: DevLoversTeam/devlovers.net
Length of output: 116
🏁 Script executed:
#!/bin/bash
# Examine the full stripe.ts file to see SDK initialization and both functions
cat -n frontend/lib/psp/stripe.ts | head -120Repository: DevLoversTeam/devlovers.net
Length of output: 4120
🏁 Script executed:
#!/bin/bash
# Search for retrievePaymentIntent function in the file
rg "retrievePaymentIntent" frontend/lib/psp/stripe.ts -A 20Repository: DevLoversTeam/devlovers.net
Length of output: 835
🏁 Script executed:
#!/bin/bash
# Check Stripe version in frontend package.json
grep -A 2 -B 2 '"stripe"' frontend/package.jsonRepository: DevLoversTeam/devlovers.net
Length of output: 182
🌐 Web query:
Stripe Node.js SDK apiVersion parameter format and valid versions - does '2025-11-17.clover' match expected format
💡 Result:
Short answer: Yes — "2025-11-17.clover" matches Stripe's expected apiVersion format.
Evidence:
- Stripe SDK releases show API versions pinned with a date plus a suffix (e.g. "2025-11-17.clover"). [1]
- Stripe docs demonstrate the apiVersion can be a date and use suffixes for preview vs stable (example: "2025-11-17.preview" and note that stable versions use ".clover"). [2]
Sources:
- stripe-node release notes (pinned API version: 2025-11-17.clover). [1]
- Stripe docs (example using "2025-11-17.preview" and explanatory note about ".clover" being stable). [2]
Add non-empty id validation to retrievePaymentIntent() for consistency with retrieveCharge().
retrieveCharge validates that chargeId is non-empty (lines 99–100), but retrievePaymentIntent (lines 71–90) skips this check for paymentIntentId. Both accept user-supplied IDs and should validate them uniformly before calling the Stripe API.
Stripe SDK version 20.0.0 correctly supports the apiVersion format '2025-11-17.clover', so no changes needed there.
🤖 Prompt for AI Agents
In @frontend/lib/psp/stripe.ts around lines 91 - 109, retrievePaymentIntent
lacks the non-empty ID validation present in retrieveCharge; update the
retrievePaymentIntent function to check that paymentIntentId is provided and not
just whitespace (e.g., if (!paymentIntentId || paymentIntentId.trim().length ===
0) throw new Error('STRIPE_INVALID_PAYMENT_INTENT_ID')), perform this check
after obtaining paymentsEnabled/stripe and before calling
stripe.paymentIntents.retrieve, and keep the existing try/catch logging and
error throw behavior consistent with retrieveCharge.
| import { POST as checkoutPOST } from '@/app/api/shop/checkout/route'; | ||
| import { vi } from 'vitest'; | ||
|
|
||
| vi.mock('@/lib/auth', async () => { | ||
| const actual = await vi.importActual<any>('@/lib/auth'); | ||
| return { | ||
| ...actual, | ||
| getCurrentUser: async () => null, // avoid cookies() in vitest | ||
| }; | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check the test file structure and surrounding context
cat -n frontend/lib/tests/checkout-concurrency-stock1.test.ts | head -50Repository: DevLoversTeam/devlovers.net
Length of output: 1653
🏁 Script executed:
# Check the checkout route handler to see if it imports @/lib/auth
cat -n frontend/app/api/shop/checkout/route.ts | head -100Repository: DevLoversTeam/devlovers.net
Length of output: 3388
🏁 Script executed:
# Search for any vitest configuration
find . -name "vitest.config*" -o -name "vite.config*" | head -10Repository: DevLoversTeam/devlovers.net
Length of output: 98
🏁 Script executed:
# Check how @/lib/auth is imported and used across the codebase
rg -n "@/lib/auth" frontend/app/api/shop/checkout/route.ts
rg -n "getCurrentUser" frontend/app/api/shop/checkout/route.tsRepository: DevLoversTeam/devlovers.net
Length of output: 210
🏁 Script executed:
# Check the vitest configuration
cat -n frontend/vitest.config.tsRepository: DevLoversTeam/devlovers.net
Length of output: 636
🏁 Script executed:
# Check if there are any other imports or side effects at the top of the test file
cat -n frontend/lib/tests/checkout-concurrency-stock1.test.ts | head -80Repository: DevLoversTeam/devlovers.net
Length of output: 2736
🏁 Script executed:
# Verify the exact timing and how @/lib/auth is used in the route
rg -n "getCurrentUser" frontend/app/api/shop/checkout/route.ts -A 2 -B 2Repository: DevLoversTeam/devlovers.net
Length of output: 471
🏁 Script executed:
# Check if this test actually passes currently
# Look for any CI configuration or test results
find . -name ".github" -o -name "*.yml" -o -name "*.yaml" | grep -i test | head -5Repository: DevLoversTeam/devlovers.net
Length of output: 53
🏁 Script executed:
# Check if there's a way to run this test and see if the mock is actually working
# Look for package.json test scripts
grep -A 10 '"test"' frontend/package.json 2>/dev/null || grep -A 10 '"test"' package.json 2>/dev/nullRepository: DevLoversTeam/devlovers.net
Length of output: 53
🏁 Script executed:
# Verify: does the route handler use getCurrentUser at module initialization or only at request time?
cat -n frontend/app/api/shop/checkout/route.ts | grep -A 20 "export.*POST"Repository: DevLoversTeam/devlovers.net
Length of output: 739
🏁 Script executed:
# Understand Vitest mock hoisting behavior - check if vi.mock is called at module initialization
# or if it's hoisted. Look at how the test is actually structured.
cat -n frontend/lib/tests/checkout-concurrency-stock1.test.ts | sed -n '1,30p'Repository: DevLoversTeam/devlovers.net
Length of output: 1079
🏁 Script executed:
# Check if there are any other test files that use a similar pattern
rg -l "vi.mock" frontend/lib/tests/ | head -3Repository: DevLoversTeam/devlovers.net
Length of output: 227
🏁 Script executed:
# Check one of those files to see the import order pattern
head -30 frontend/lib/tests/$(rg -l "vi.mock" frontend/lib/tests/ | head -1 | xargs basename)Repository: DevLoversTeam/devlovers.net
Length of output: 877
🏁 Script executed:
# Critical: check if the @/lib/auth module has any side effects at import time
cat -n frontend/lib/auth.ts | head -50Repository: DevLoversTeam/devlovers.net
Length of output: 1517
Move vi.mock() before importing checkoutPOST to ensure the mock is installed before the route handler (and its transitive dependencies) loads @/lib/auth.
Currently, the route is imported on line 16 before the mock on line 19, which risks the real module being cached before the mock takes effect. This contradicts the pattern used elsewhere in the test suite (e.g., stripe-webhook-psp-fields.test.ts), where mocks are always defined first.
Correct pattern (mock first, then import route)
import { describe, it, expect, beforeAll, afterAll } from 'vitest';
import crypto from 'crypto';
import { NextRequest } from 'next/server';
import { eq, inArray } from 'drizzle-orm';
import { db } from '@/db';
import {
products,
productPrices,
orders,
orderItems,
inventoryMoves,
} from '@/db/schema/shop';
-import { POST as checkoutPOST } from '@/app/api/shop/checkout/route';
import { vi } from 'vitest';
vi.mock('@/lib/auth', async () => {
const actual = await vi.importActual<any>('@/lib/auth');
return {
...actual,
getCurrentUser: async () => null, // avoid cookies() in vitest
};
});
+
+import { POST as checkoutPOST } from '@/app/api/shop/checkout/route';📝 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.
| import { POST as checkoutPOST } from '@/app/api/shop/checkout/route'; | |
| import { vi } from 'vitest'; | |
| vi.mock('@/lib/auth', async () => { | |
| const actual = await vi.importActual<any>('@/lib/auth'); | |
| return { | |
| ...actual, | |
| getCurrentUser: async () => null, // avoid cookies() in vitest | |
| }; | |
| }); | |
| import { vi } from 'vitest'; | |
| vi.mock('@/lib/auth', async () => { | |
| const actual = await vi.importActual<any>('@/lib/auth'); | |
| return { | |
| ...actual, | |
| getCurrentUser: async () => null, // avoid cookies() in vitest | |
| }; | |
| }); | |
| import { POST as checkoutPOST } from '@/app/api/shop/checkout/route'; |
🤖 Prompt for AI Agents
In @frontend/lib/tests/checkout-concurrency-stock1.test.ts around lines 16 - 25,
The test imports the route handler POST (checkoutPOST) before installing the
vitest mock for '@/lib/auth', so the real module may be loaded/cached; move the
vi.mock(...) block to appear before the import of checkoutPOST (i.e., define
vi.mock('@/lib/auth', ...) first, then import { POST as checkoutPOST } from
'@/app/api/shop/checkout/route') to ensure the mock is installed prior to the
route handler and its transitive dependencies loading the real module.
| } finally { | ||
| await cleanupByIds({ orderId, productId }); | ||
| try { | ||
| await cleanupByIds({ orderId, productId }); | ||
| } catch (e) { | ||
| console.error('[test cleanup failed]', { orderId, productId }, e); | ||
| throw e; | ||
| } | ||
| } |
There was a problem hiding this comment.
Don’t throw from finally unconditionally—this can mask the real test failure (Biome noUnsafeFinally).
Track the primary error and only fail the test on cleanup failure when the test itself passed.
Proposed diff (preserve original failure)
- try {
+ let primaryError: unknown = null;
+ try {
// Baseline snapshot
const before = await db
.select({
orderId: orderItems.orderId,
@@
// Snapshot MUST remain V1 even after product changes
expect(after[0]).toEqual(before[0]);
- } finally {
- try {
- await cleanupByIds({ orderId, productId });
- } catch (e) {
- console.error('[test cleanup failed]', { orderId, productId }, e);
- throw e;
- }
- }
+ } catch (e) {
+ primaryError = e;
+ throw e;
+ } finally {
+ try {
+ await cleanupByIds({ orderId, productId });
+ } catch (cleanupError) {
+ console.error('[test cleanup failed]', { orderId, productId, cleanupError });
+ // Only fail due to cleanup if the test itself passed.
+ if (!primaryError) throw cleanupError;
+ }
+ }Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Biome (2.1.2)
[error] 215-215: Unsafe usage of 'throw'.
'throw' in 'finally' overwrites the control flow statements inside 'try' and 'catch'.
(lint/correctness/noUnsafeFinally)
2 participants
Description
This PR combines two commits:
Related Issue
Issue: #<issue_number>
Changes
charge.refund.updatedhandling to correctly detect full refunds using cumulative refunded amount (preferscharge.amount_refunded, falls back to summingcharge.refunds.data[].amount), preventing incorrect terminal state and stock side effects.timingSafeEqualwith padded buffers; no early length-based return).JANITOR_TIMEOUT_MSparsing in the janitor runner script to avoidNaN/invalid values causing flaky abort behavior (safe default applied).SALE_ORIGINAL_REQUIRED(ensuresisSubmittingresets).pricesJSON (returns explicit 400 with a clear code).finallycleanup from masking the original test failure (preserves original error; logs cleanup failures).stripe-webhook-paid-status-repair.test.tsto prevent DB pollution.tmp/(and removed from tracking if previously staged).Database Changes (if applicable)
How Has This Been Tested?
Notes:
npx vitest run .\lib\tests\stripe-webhook-paid-status-repair.test.tsnpx vitest run .\lib\tests\stripe-webhook-refund-full.test.tsnpx vitest run .\lib\tests\checkout-currency-policy.test.tsnpx vitest run .\lib\tests\product-sale-invariant.test.tsnpx vitest runnpm run buildScreenshots (if applicable)
Checklist
Before submitting
Reviewers
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Security
✏️ Tip: You can customize this high-level summary in your review settings.