(SP: 3) [Backend] Transactionless checkout + Stripe webhook reliability pack (idempotency, dedupe, restock-once, money invariants)#110
Conversation
…ory ledger, idempotency, and orphan cleanup
…v vars are missing (SP: 1) [Backend] Add Vitest coverage for Orders IDOR access control (/api/shop/orders/[id])
…pected 4xx error logs; remove cart console.error
…ONFIG vs PRICE_DATA errors
…s INVALID_SIGNATURE) and validate end-to-end payment flow
…erification and persist mismatch diagnostics (pspStatusReason/pspMetadata)
…DATE…RETURNING (set status=PAID)
…mpotency for failed/refunded
…ipe webhook (idempotent) + add coverage tests
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. |
📝 WalkthroughWalkthroughAdds canonical minor‑unit money handling, idempotent order creation with inventory reserve/release moves, Stripe payment‑intent client/server integration and webhook deduplication, restock/sweep claim mechanics, DB schema and migration updates, env/config and Cloudinary lazy init changes, UI/payment adjustments, many new tests, and new error/logging utilities. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant CheckoutAPI as Checkout API
participant Orders as Orders Service
participant Inventory as Inventory Service
participant DB as Database
participant Stripe as Stripe
Client->>CheckoutAPI: POST /api/shop/checkout (items, idempotencyKey, locale)
CheckoutAPI->>Orders: createOrderWithItems(items, idempotencyKey, locale)
Orders->>DB: SELECT by idempotency_hash
alt existing order
DB-->>Orders: existing order
Orders-->>CheckoutAPI: return existing order
else new order
Orders->>DB: INSERT order (status=CREATED, idempotency_hash)
Orders->>Inventory: applyReserveMove(orderId, productId, qty)
Inventory->>DB: advisory lock, check/update stock, INSERT inventory_moves (RESERVE)
alt stock sufficient
DB-->>Inventory: applied
Inventory-->>Orders: applied
Orders->>DB: update order (inventoryStatus=reserved)
alt payments enabled
Orders->>Stripe: createPaymentIntent(amountMinor, metadata)
Stripe-->>Orders: client_secret
Orders-->>CheckoutAPI: return order + clientSecret
else
Orders-->>CheckoutAPI: return order (no payments)
end
else insufficient
Inventory-->>Orders: insufficient
Orders->>DB: update order (INVENTORY_FAILED)
Orders-->>CheckoutAPI: 400 insufficient stock
end
end
CheckoutAPI-->>Client: 201 or error
sequenceDiagram
autonumber
participant Stripe as Stripe Webhook
participant WebhookAPI as /api/shop/webhooks/stripe
participant DB as Database
participant Orders as Orders Service
participant Inventory as Inventory Service
Stripe->>WebhookAPI: POST event (id, type, data)
WebhookAPI->>DB: INSERT stripe_events ON CONFLICT DO NOTHING
alt duplicate
DB-->>WebhookAPI: duplicate detected
WebhookAPI-->>Stripe: 200 ACK
else first time
DB-->>WebhookAPI: recorded
WebhookAPI->>DB: lookup order by metadata.orderId
alt payment_intent.succeeded
WebhookAPI->>DB: validate amount & currency vs order.total_amount_minor
alt mismatch
DB-->>WebhookAPI: update psp_status_reason (mismatch)
WebhookAPI-->>Stripe: 200 ACK
else match
DB-->>WebhookAPI: update order (paymentStatus=paid, set psp fields)
WebhookAPI-->>Stripe: 200 ACK
end
else payment_failed/canceled/refund
WebhookAPI->>DB: update order (paymentStatus failed/refunded, set psp fields)
WebhookAPI->>Inventory: applyReleaseMove(orderId, productId, qty)
Inventory-->>DB: INSERT RELEASE move, update stock
WebhookAPI-->>Stripe: 200 ACK
end
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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: 9
Fix all issues with AI Agents 🤖
In @frontend/app/[locale]/shop/checkout/success/page.tsx:
- Around line 56-57: Remove the manual locale prefix from Link hrefs: any Link
instances in this file (e.g., the Link elements currently using
href={`/${locale}/shop/products`}) should use path-only hrefs like
"/shop/products" (and similarly "/shop/cart", "/shop/checkout", etc.) so the
next-intl Link with localePrefix: 'always' can prepend the locale automatically;
update all similar Link usages in this file and the related checkout files to
use locale-agnostic href strings instead of interpolating the locale.
- Around line 117-120: The code uses unnecessary and unsafe casts when computing
totalMinor; since getOrderSummary returns an OrderSummary that includes
totalAmountMinor, replace the conditional with a direct access: set totalMinor =
order.totalAmountMinor (remove the Math.round fallback and all "as any" casts).
Update the line initializing totalMinor (referencing the order variable and
totalAmountMinor field) to use the typed property directly to restore type
safety and match the pattern used in payment page code.
In @frontend/drizzle/0009_p0_inventory_workflow_baseline.sql:
- Around line 24-25: The product_id foreign key currently has no explicit ON
DELETE behavior; update the constraint in the inventory moves table definition
by adding an explicit ON DELETE clause: e.g., change "product_id uuid not null
references products(id)" to "product_id uuid not null references products(id) ON
DELETE CASCADE" to cascade deletions, or if you need to preserve historical
moves, make product_id nullable and use "product_id uuid references products(id)
ON DELETE SET NULL" instead; apply the chosen change to the product_id column
definition in the SQL diff.
In @frontend/lib/cart.ts:
- Around line 124-146: computeSummaryFromItems is summing the wrong field:
replace accumulation of item.lineTotal with the canonical integer cents
item.lineTotalMinor when building totals.totalMinor so
totalAmountMinor/totalAmount are correct; keep using
fromCents(totals.totalMinor) for totalAmount. Also, in the same function
consider validating that all CartRehydrateItem.currency values match (or
explicitly document the assumption) instead of unconditionally taking
items[0]?.currency for totals.currency.
In @frontend/lib/env/cloudinary.ts:
- Around line 30-44: The function getCloudinaryEnvOptional claims in its
docstring to never throw, but the cloudinaryRequiredSchema.parse() call can
throw a ZodError when validation fails. To fix this, replace the parse() method
with safeParse() which returns a result object with success/data properties
instead of throwing. Then check the success property of the result and return
null if validation fails, or extract and return the parsed data if validation
succeeds, maintaining the function's promise to never throw.
In @frontend/lib/psp/stripe.ts:
- Around line 22-25: The Stripe client instantiation sets apiVersion to
"2025-12-15.clover" with an `as any` cast on `_stripe = new Stripe(secretKey, {
apiVersion: "2025-12-15.clover" as any })`; change this to use the SDK-aligned
version `"2025-11-17.clover"` (remove the `as any` cast) OR keep the newer
version but add a clear comment next to the `apiVersion` option documenting why
the override is required, which API changes from 2025-12-15 are relied on, and
why it is safe with stripe-node v20.0.0, so reviewers understand the risk and
rationale.
In @frontend/lib/services/products.ts:
- Around line 504-534: The try block and its contents are mis-indented; fix by
aligning the `try` keyword and its inner block with the surrounding function
scope and existing indentation style: move `try {` to the same indentation level
as the surrounding statements, and re-indent the inner lines (the `if
(prices.length) {`, the mapping to `upsertRows`, and the `await
db.insert(productPrices)...onConflictDoUpdate(...)` chain) so they are
consistently nested under the `try` block; ensure symbols referenced remain
unchanged (prices, upsertRows, productPrices, toDbMoney, db.insert,
onConflictDoUpdate).
In @frontend/lib/tests/admin-api-killswitch.test.ts:
- Around line 95-106: readCodeFromResponse currently calls res.json() and falls
back to res.text(), but res.json() consumes the body so res.text() will fail;
fix by reading the body once and parsing safely: obtain the raw body via
res.clone().text() or text = await res.text() and then try JSON.parse(text) to
extract code/error.code, returning { status, code, raw: text }. Update
readCodeFromResponse to use the single-read approach and extract code from the
parsed JSON (or undefined if parsing fails) while always returning the raw text.
In @frontend/lib/tests/restock-order-only-once.test.ts:
- Around line 263-265: The failing organization is that the it test named
'duplicate refund restock must not increment stock twice and must not change
restocked_at' is placed after the describe('P0-8.4.2', ...) block closes; move
the describe block's closing "}, 30000);" so that the closing brace/timeout call
comes after this it(...) test, thereby enclosing that test inside the describe;
adjust surrounding braces/indentation accordingly so the describe contains all
three tests and retain the existing timeout argument.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
frontend/app/api/shop/checkout/route.ts (1)
488-498: Standardize error codes for MoneyValueError across endpoints.The same error type returns different error codes depending on the endpoint:
PRICE_DATA_ERRORin checkout (line 488) andPRICE_CONFIG_ERRORin cart/rehydrate (line 75). This inconsistency will break clients and monitoring that expect a uniform error code for the same error condition. Choose one error code and apply it consistently across both handlers.frontend/lib/services/orders.ts (1)
96-115: Merging checkout items by productId loses variant information.The
mergeCheckoutItemsfunction uses onlyproductIdas the key, butCheckoutItemincludesselectedSizeandselectedColorfields that distinguish product variants. Items with the same product but different sizes or colors would be incorrectly merged. The cart system already treats variants as separate items usingcreateCartItemKey(productId, selectedSize, selectedColor), so the merge key should include these variant fields to maintain consistency and preserve order details.
🧹 Nitpick comments (26)
frontend/lib/tests/order-items-snapshot-immutable.test.ts (3)
44-54: Avoidas anytype assertion.The
as anycast on line 53 bypasses TypeScript's type checking and could hide incompatibilities with theNextRequestconstructor. Consider properly typing the options object or using a more specific cast if there's a known mismatch between Next.js types and runtime behavior.🔎 Proposed refactor
function makeJsonRequest( url: string, body: unknown, headers: Record<string, string> ) { return new NextRequest(url, { method: 'POST', headers, body: JSON.stringify(body), - } as any); + }); }If TypeScript reports errors, address the specific type mismatch rather than silencing all checks.
126-137: Tighten response validation.Two observations:
Status code check vs. comment mismatch: Line 128's comment states "returns 201 Created on success," but lines 129-130 accept any 2xx status. If 201 is the expected contract, consider checking for it explicitly; otherwise, update the comment to reflect that any 2xx is acceptable.
orderId validation logic: Lines 135-137 check
typeof orderId === 'string'and then throw if orderId is falsy. This is slightly redundant—consider simplifying to a direct truthiness check with a type guard or a more explicit assertion.🔎 Proposed refactor
- // Your checkout returns 201 Created on success. - expect(res.status).toBeGreaterThanOrEqual(200); - expect(res.status).toBeLessThan(300); + // Checkout returns 201 Created on success + expect(res.status).toBe(201); const json = (await res.json()) as CheckoutResponse; expect(json.success).toBe(true); const orderId = json.orderId ?? json.order?.id; - expect(typeof orderId).toBe('string'); - if (!orderId) throw new Error('Missing orderId from checkout response'); + if (!orderId || typeof orderId !== 'string') { + throw new Error('Missing orderId from checkout response'); + }
73-214: Consider multi-currency test coverage.This test validates snapshot immutability for USD-only orders. Given the PR's focus on "locale→currency mapping (uk→UAH, others→USD)" and minor-unit handling across currencies, consider whether snapshot immutability should also be tested for non-USD currencies (e.g., UAH) to ensure the snapshot mechanism works correctly regardless of currency.
If currency-specific snapshot behavior is already covered elsewhere or deemed unnecessary, a brief comment documenting the USD-only scope would help future maintainers.
frontend/lib/tests/public-product-visibility.test.ts (2)
38-38: Minor formatting inconsistency.Extra leading spaces before
price:compared to surrounding lines. While functionally correct, consider normalizing indentation for consistency.🔎 Suggested fix
- price: "10.00", + price: "10.00",
85-85: Minor formatting inconsistency.Extra leading spaces before
price:compared to surrounding lines. While functionally correct, consider normalizing indentation for consistency.🔎 Suggested fix
- price: "19.99", + price: "19.99",frontend/app/[locale]/shop/products/[slug]/page.tsx (1)
23-31: Public visibility gate correctly implements the feature.The gating logic properly enforces that inactive products or products without currency-specific pricing return 404 before any further processing. The implementation aligns with the PR's security objectives.
However, note that this introduces a double-fetch pattern:
getPublicProductBySlug(line 28) followed bygetProductPageData(line 33). While this provides defense-in-depth for access control, it could be optimized ifgetProductPageDatawere refactored to include the public visibility check internally.frontend/lib/tests/admin-api-killswitch.test.ts (1)
148-148: Test name displays[object Object]instead of route name.Using
%swith an object array init.eachwill show[object Object]in test output. Use$namefor property interpolation.🔎 Proposed fix
- it.each(cases)('returns 403 ADMIN_API_DISABLED for all mutating handlers: %s', async (c) => { + it.each(cases)('returns 403 ADMIN_API_DISABLED for all mutating handlers: $name', async (c) => {frontend/lib/tests/checkout-no-payments.test.ts (1)
101-113: Consider logging suppressed cleanup errors for debugging.The empty
catchblocks inbestEffortHardDeleteOrderare acceptable for cleanup purposes, but suppressed errors can make debugging difficult if cleanup unexpectedly fails.🔎 Optional: Log cleanup failures at debug level
async function bestEffortHardDeleteOrder(orderId: string) { // Keep DB reasonably clean in dev. // Use raw SQL because inventory_moves/order_items may not be exported as Drizzle tables. try { await db.execute(sql`delete from inventory_moves where order_id = ${orderId}::uuid`); - } catch {} + } catch (e) { + // Cleanup failure is non-fatal but useful for debugging + console.debug('cleanup: inventory_moves delete failed', e); + } try { await db.execute(sql`delete from order_items where order_id = ${orderId}::uuid`); - } catch {} + } catch (e) { + console.debug('cleanup: order_items delete failed', e); + } try { await db.delete(orders).where(eq(orders.id, orderId)); - } catch {} + } catch (e) { + console.debug('cleanup: orders delete failed', e); + } }frontend/lib/tests/restock-sweep-claim.test.ts (1)
45-62: Solid concurrency test for sweep claim deduplication.The test correctly validates that two concurrent
restockStalePendingOrderscalls process the same order exactly once (a + b === 1).Optional enhancement: Consider adding post-processing assertions (similar to
restock-stale-claim-gate.test.ts) to verify thatstockRestoredistrueandrestockedAtis set after the concurrent run, confirming the order was actually restocked.frontend/lib/tests/restock-stale-claim-gate.test.ts (1)
81-137: Good coverage for expired claim processing.The test validates that orders with an expired claim can be reclaimed and processed.
Optional: For symmetry with the first test, consider adding post-processing assertions to verify
stockRestoredistrueafter the sweep processes the order.frontend/lib/cloudinary.ts (1)
35-43: Redundant call togetCloudinaryEnvRequired().
ensureConfigured()on line 35 already callsgetCloudinaryEnvRequired()internally (line 10). Calling it again on line 36 just to accessuploadFolderis wasteful. Consider caching the env result or returning it fromensureConfigured().🔎 Proposed refactor
+let cachedEnv: ReturnType<typeof getCloudinaryEnvRequired> | null = null; + function ensureConfigured() { if (isConfigured) return; const env = getCloudinaryEnvRequired(); cloudinary.config({ cloud_name: env.cloudName, api_key: env.apiKey, api_secret: env.apiSecret, }); + cachedEnv = env; isConfigured = true; } export async function uploadImage( fileOrBuffer: File | Buffer, options?: { folder?: string } ): Promise<{ url: string; publicId: string }> { ensureConfigured(); - const env = getCloudinaryEnvRequired(); + const env = cachedEnv!; const buffer = await toBuffer(fileOrBuffer); // ...frontend/lib/psp/stripe.ts (1)
110-115: Empty catch swallows all errors, not just signature validation failures.While the comment explains this is expected business behavior, the catch block swallows all errors (including potential unexpected ones like network issues during signature verification). The
constructEventis synchronous and unlikely to throw network errors, but consider logging unexpected error types in debug mode.frontend/lib/tests/stripe-webhook-psp-fields.test.ts (1)
161-161: Consider usingvi.mocked()for cleaner mock typing.The type assertion
(verifyWebhookSignature as unknown as { mockReturnValue: (v: any) => void })is verbose. Vitest providesvi.mocked()for type-safe mock access.🔎 Proposed refactor
- (verifyWebhookSignature as unknown as { mockReturnValue: (v: any) => void }).mockReturnValue(event); + vi.mocked(verifyWebhookSignature).mockReturnValue(event as any);frontend/app/[locale]/shop/checkout/payment/[orderId]/page.tsx (2)
125-147: Excessive type assertions suggest missing type definitions.The repeated
(order as any)casts (lines 125-144) indicate thatOrderSummarytype fromgetOrderSummarymay not includepaymentIntentId,totalAmountMinor,currency, orid. Consider extending the type definition rather than using unsafe casts throughout.Additionally, there are inconsistent indentation issues on lines 133, 138, and 144 (varying space counts).
148-151: Silent error swallowing loses debugging context.While the comment explains the intent (UI shows "Payment cannot be initialized"), silently catching all errors makes debugging difficult. Consider logging the error at a lower level.
🔎 Proposed improvement
} catch (err) { - // Leave clientSecret empty -> UI shows "Payment cannot be initialized" + // Leave clientSecret empty -> UI shows "Payment cannot be initialized" + console.debug('Payment intent initialization failed', err); }frontend/lib/tests/checkout-currency-policy.test.ts (1)
61-71: Consider combining the twobeforeAllblocks.Having two separate
beforeAllblocks (lines 61-65 and 67-71) works but can be confusing. The production check and dynamic import could be in the same block.🔎 Proposed refactor
-beforeAll(() => { +beforeAll(async () => { if (process.env.NODE_ENV === 'production') { throw new Error('Refusing to run DB-mutating tests in production environment.'); } -}); -beforeAll(async () => { // Import route after env + mocks are set const mod = await import('@/app/api/shop/checkout/route'); POST = mod.POST; });frontend/lib/tests/restock-order-only-once.test.ts (2)
183-184: Minor: Inconsistent indentation.Line 184 has inconsistent leading whitespace compared to surrounding lines.
🔎 Proposed fix
price: toDbMoney(1000), -currency: 'USD', + currency: 'USD', createdAt,
152-159: Consider extracting cleanup logic to a shared helper.The cleanup pattern is repeated three times with identical structure. Consider a helper like
cleanupTestEntities(orderId, productId)to reduce duplication.Also applies to: 254-261, 405-412
frontend/lib/services/errors.ts (1)
1-3: Remove commented-out code.The commented
InvalidPayloadErrorandSlugConflictErrorstubs at lines 1-3 and 32-34 are now implemented below. Remove the commented versions to reduce noise.🔎 Proposed fix
-// export class InvalidPayloadError extends Error { -// code = "INVALID_PAYLOAD" as const -// } - export class IdempotencyConflictError extends Error {- -// export class SlugConflictError extends Error { -// code = "SLUG_CONFLICT" as const -// } export class InvalidPayloadError extends Error {Also applies to: 32-34
frontend/lib/services/products.ts (3)
351-426: Product creation with best-effort cleanup looks correct.The flow properly:
- Creates the product with
onConflictDoNothingfor slug collision detection- Inserts price rows after product creation
- Cleans up the product if price insertion fails via best-effort delete
One minor observation: the cleanup on line 416 only deletes the product row. Cloudinary image cleanup for
uploaded?.publicIdis not performed in the catch block if product insert succeeded but price insert failed.🔎 Suggested improvement for image cleanup on failure
} catch (error) { // якщо product_prices впало після створення продукту — прибираємо продукт (best-effort) if (createdProductId) { try { await db.delete(products).where(eq(products.id, createdProductId)); } catch (cleanupDbError) { logError( 'Failed to cleanup product after create failure', cleanupDbError ); } + // Also cleanup uploaded image if product was rolled back + if (uploaded?.publicId) { + try { + await destroyProductImage(uploaded.publicId); + } catch (cleanupImageError) { + logError('Failed to cleanup image after create failure', cleanupImageError); + } + } } throw error; }
612-633: Return type usesunknownfor price fields, reducing type safety.The
priceMinor,originalPriceMinor,price, andoriginalPricefields are typed asunknown. Since these are being read from strongly-typed Drizzle schema columns, they should retain their actual types (number,number | null,string,string | null).🔎 Suggested type improvement
export async function getAdminProductPrices(productId: string): Promise< Array<{ currency: CurrencyCode; - priceMinor: unknown; - originalPriceMinor: unknown; - price: unknown; - originalPrice: unknown; + priceMinor: number; + originalPriceMinor: number | null; + price: string; + originalPrice: string | null; }> > {
635-649: Sameunknowntyping issue ingetAdminProductByIdWithPrices.This follows from the previous comment - the prices array should have proper types for consistency and type safety across the admin API surface.
frontend/lib/services/orders.ts (4)
546-612: Idempotency compatibility check is thorough but has an empty catch.The
assertIdempotencyCompatiblefunction correctly:
- Validates currency match
- Derives hash for legacy orders without stored hash
- Handles failed orders by triggering restock
However, the empty
catch {}on line 593 during hash backfill silently swallows errors. Consider logging this for observability.🔎 Suggested improvement for observability
try { await db .update(orders) .set({ idempotencyRequestHash: derivedExistingHash, updatedAt: new Date(), }) .where(eq(orders.id, row.id)); - } catch {} + } catch (backfillError) { + // Non-critical: hash backfill is best-effort for future strict checks + }
1186-1224: restockStaleNoPaymentOrders lacks lease claiming unlike its sibling.Unlike
restockStalePendingOrders, this function doesn't use the lease claiming pattern. If multiple workers run this sweep concurrently, they could process the same orders redundantly. WhilerestockOrderhas its own idempotency guards, adding lease claiming would reduce wasted work.
1064-1065: Sequential release moves could be parallelized.The release loop processes items sequentially. Since
applyReleaseMoveis idempotent (via move_key), these could be parallelized withPromise.allfor better performance on orders with many items.🔎 Optional parallelization
- for (const item of reservedMoves) - await applyReleaseMove(orderId, item.productId, item.quantity); + await Promise.all( + reservedMoves.map(item => + applyReleaseMove(orderId, item.productId, item.quantity) + ) + );
978-984: Replace raw SQL with Drizzle query builder for type safety.The
inventoryMovestable is exported from the schema (frontend/db/schema/shop.ts) and should be used instead of raw SQL. Replace.from(sql\inventory_moves`)with.from(inventoryMoves)and use the Drizzle column references (inventoryMoves.productId,inventoryMoves.orderId,inventoryMoves.type,inventoryMoves.quantity`) for consistency with the rest of the file and to maintain type safety.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (40)
frontend/app/[locale]/shop/cart/page.tsxfrontend/app/[locale]/shop/checkout/payment/StripePaymentClient.tsxfrontend/app/[locale]/shop/checkout/payment/[orderId]/page.tsxfrontend/app/[locale]/shop/checkout/success/OrderStatusAutoRefresh.tsxfrontend/app/[locale]/shop/checkout/success/page.tsxfrontend/app/[locale]/shop/products/[slug]/page.tsxfrontend/app/api/shop/checkout/route.tsfrontend/app/api/shop/webhooks/stripe/route.tsfrontend/components/shop/product-card.tsxfrontend/db/schema/shop.tsfrontend/drizzle/0009_p0_inventory_workflow_baseline.sqlfrontend/drizzle/0011_add_orders_sweep_claim_index.sqlfrontend/drizzle/meta/0011_snapshot.jsonfrontend/drizzle/meta/_journal.jsonfrontend/lib/cart.tsfrontend/lib/cloudinary.tsfrontend/lib/env/cloudinary.tsfrontend/lib/env/index.tsfrontend/lib/env/stripe.tsfrontend/lib/logging.tsfrontend/lib/psp/stripe.tsfrontend/lib/services/errors.tsfrontend/lib/services/inventory.tsfrontend/lib/services/orders.tsfrontend/lib/services/products.tsfrontend/lib/tests/admin-api-killswitch.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/orders-access.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-sweep-claim.test.tsfrontend/lib/tests/stripe-webhook-contract.test.tsfrontend/lib/tests/stripe-webhook-mismatch.test.tsfrontend/lib/tests/stripe-webhook-psp-fields.test.tsfrontend/lib/validation/shop.tsfrontend/project-structure.txt
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-30T16:01:02.523Z
Learnt from: kryvosheyin
Repo: DevLoversTeam/devlovers.net PR: 88
File: frontend/drizzle/schema.ts:153-153
Timestamp: 2025-12-30T16:01:02.523Z
Learning: The file frontend/drizzle/schema.ts is a legacy file not used by Drizzle or the runtime. The canonical schemas live in frontend/db/schema/* as configured in drizzle-config.ts, and that's what migrations and runtime use.
Applied to files:
frontend/drizzle/meta/0011_snapshot.json
🧬 Code graph analysis (22)
frontend/lib/tests/checkout-no-payments.test.ts (5)
frontend/db/index.ts (1)
db(39-39)frontend/drizzle/schema.ts (1)
currency(7-7)frontend/db/schema/shop.ts (1)
orders(107-198)frontend/lib/services/orders.ts (3)
restockOrder(944-1097)restockOrder(1272-1272)restockStaleNoPaymentOrders(1187-1224)frontend/lib/shop/money.ts (1)
toDbMoney(58-60)
frontend/app/[locale]/shop/checkout/payment/StripePaymentClient.tsx (1)
frontend/lib/shop/currency.ts (1)
formatMoney(115-128)
frontend/lib/tests/restock-stale-claim-gate.test.ts (4)
frontend/db/index.ts (1)
db(39-39)frontend/db/schema/shop.ts (1)
orders(107-198)frontend/lib/shop/money.ts (1)
toDbMoney(58-60)frontend/lib/services/orders.ts (1)
restockStalePendingOrders(1099-1184)
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(252-265)
frontend/lib/tests/orders-access.test.ts (2)
frontend/db/index.ts (1)
db(39-39)frontend/lib/auth.ts (1)
getCurrentUser(95-116)
frontend/lib/tests/order-items-snapshot-immutable.test.ts (2)
frontend/db/index.ts (1)
db(39-39)frontend/db/schema/shop.ts (1)
inventoryMoves(325-354)
frontend/lib/tests/restock-order-only-once.test.ts (5)
frontend/db/index.ts (1)
db(39-39)frontend/db/schema/shop.ts (2)
products(56-105)orders(107-198)frontend/lib/shop/money.ts (1)
toDbMoney(58-60)frontend/lib/services/inventory.ts (1)
applyReserveMove(49-116)frontend/lib/services/orders.ts (2)
restockOrder(944-1097)restockOrder(1272-1272)
frontend/lib/tests/restock-stale-stripe-orphan.test.ts (4)
frontend/db/index.ts (1)
db(39-39)frontend/db/schema/shop.ts (1)
orders(107-198)frontend/lib/shop/money.ts (1)
toDbMoney(58-60)frontend/lib/services/orders.ts (1)
restockStalePendingOrders(1099-1184)
frontend/lib/services/inventory.ts (1)
frontend/db/index.ts (1)
db(39-39)
frontend/app/[locale]/shop/checkout/success/page.tsx (3)
frontend/lib/validation/shop.ts (1)
orderIdParamSchema(371-373)frontend/app/[locale]/shop/checkout/success/OrderStatusAutoRefresh.tsx (1)
OrderStatusAutoRefresh(16-42)frontend/lib/shop/currency.ts (1)
formatMoney(115-128)
frontend/lib/services/products.ts (5)
frontend/db/schema/shop.ts (2)
products(56-105)productPrices(269-323)frontend/lib/shop/money.ts (2)
toDbMoney(58-60)fromCents(44-46)frontend/lib/services/errors.ts (1)
SlugConflictError(54-60)frontend/lib/logging.ts (1)
logError(1-14)frontend/lib/cloudinary.ts (1)
destroyProductImage(77-79)
frontend/app/[locale]/shop/checkout/success/OrderStatusAutoRefresh.tsx (1)
frontend/drizzle/schema.ts (1)
paymentStatus(8-8)
frontend/app/[locale]/shop/products/[slug]/page.tsx (3)
frontend/drizzle/schema.ts (1)
currency(7-7)frontend/lib/shop/currency.ts (1)
resolveCurrencyFromLocale(23-28)frontend/db/queries/shop/products.ts (1)
getPublicProductBySlug(252-265)
frontend/lib/tests/stripe-webhook-mismatch.test.ts (2)
frontend/drizzle/schema.ts (2)
currency(7-7)paymentStatus(8-8)frontend/db/index.ts (1)
db(39-39)
frontend/lib/services/errors.ts (1)
frontend/db/queries/shop/orders.ts (1)
InsufficientStockError(8-8)
frontend/lib/env/stripe.ts (1)
frontend/lib/env/index.ts (2)
getRuntimeEnv(58-62)getClientEnv(52-56)
frontend/lib/cloudinary.ts (1)
frontend/lib/env/cloudinary.ts (1)
getCloudinaryEnvRequired(50-69)
frontend/components/shop/product-card.tsx (1)
frontend/lib/shop/currency.ts (1)
formatMoney(115-128)
frontend/app/[locale]/shop/checkout/payment/[orderId]/page.tsx (3)
frontend/lib/psp/stripe.ts (2)
retrievePaymentIntent(69-88)createPaymentIntent(30-67)frontend/lib/services/orders.ts (1)
setOrderPaymentIntent(841-898)frontend/lib/shop/currency.ts (1)
formatMoney(115-128)
frontend/lib/tests/checkout-currency-policy.test.ts (3)
frontend/app/api/shop/checkout/route.ts (1)
POST(148-503)frontend/db/index.ts (1)
db(39-39)frontend/db/schema/shop.ts (3)
orders(107-198)productPrices(269-323)products(56-105)
frontend/app/api/shop/checkout/route.ts (2)
frontend/lib/services/errors.ts (5)
IdempotencyConflictError(5-13)InvalidPayloadError(35-41)InsufficientStockError(15-21)PriceConfigError(43-53)OrderStateInvalidError(62-85)frontend/lib/logging.ts (2)
logWarn(17-21)logError(1-14)
frontend/lib/tests/restock-sweep-claim.test.ts (4)
frontend/db/index.ts (1)
db(39-39)frontend/db/schema/shop.ts (1)
orders(107-198)frontend/lib/shop/money.ts (1)
toDbMoney(58-60)frontend/lib/services/orders.ts (1)
restockStalePendingOrders(1099-1184)
🪛 LanguageTool
frontend/project-structure.txt
[uncategorized] ~10-~10: The official name of this software platform is spelled with a capital “H”.
Context: ...ts 📁 app 📁 api 📁 auth 📁 github 📁 callback 📄 route....
(GITHUB)
| <Link | ||
| href="/shop/products" | ||
| href={`/${locale}/shop/products`} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check how the i18n Link component handles locale prefixing
rg -n "Link" frontend/i18n/routing.ts -A 10 -B 2
# Check if other pages use the same pattern
rg -n 'href=\{`/\$\{locale\}' --type=tsx --type=ts -C2
# Get the entire routing.ts file to understand the setup
cat -n frontend/i18n/routing.tsRepository: DevLoversTeam/devlovers.net
Length of output: 647
🏁 Script executed:
# Check the checkout success page file
cat -n frontend/app/\[locale\]/shop/checkout/success/page.tsx
# Search for the href pattern more broadly
rg 'href=.*locale.*shop' -C2Repository: DevLoversTeam/devlovers.net
Length of output: 18058
Remove manual locale prefix from Link href attributes.
The next-intl Link component with localePrefix: 'always' automatically prepends the locale to URLs. Using href={/${locale}/shop/products} will cause double-prefixing (/en/en/shop/products). Replace with href="/shop/products" and let the Link component handle locale automatically.
Affects lines 56-57, 62-63, 89-90, 95-96, 178-179, 184-185 in this file, plus similar usage in related checkout files.
🤖 Prompt for AI Agents
In @frontend/app/[locale]/shop/checkout/success/page.tsx around lines 56-57,
Remove the manual locale prefix from Link hrefs: any Link instances in this file
(e.g., the Link elements currently using href={`/${locale}/shop/products`})
should use path-only hrefs like "/shop/products" (and similarly "/shop/cart",
"/shop/checkout", etc.) so the next-intl Link with localePrefix: 'always' can
prepend the locale automatically; update all similar Link usages in this file
and the related checkout files to use locale-agnostic href strings instead of
interpolating the locale.
✅ Addressed in commit b8d6aa8
| order_id uuid not null references orders(id) on delete cascade, | ||
| product_id uuid not null references products(id), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the migration file in full context
cat -n frontend/drizzle/0009_p0_inventory_workflow_baseline.sqlRepository: DevLoversTeam/devlovers.net
Length of output: 8227
🏁 Script executed:
# Search for product deletion logic in the codebase
rg -n "delete.*product|remove.*product|drop.*product" --type ts --type js -A 3 -B 3 | head -100Repository: DevLoversTeam/devlovers.net
Length of output: 7296
🏁 Script executed:
# Look for inventory_moves references to understand its purpose
rg -n "inventory_moves" --type ts --type js -A 2 -B 2 | head -100Repository: DevLoversTeam/devlovers.net
Length of output: 6236
🏁 Script executed:
# Check if there are other migrations that might show patterns for FK constraints
fd "\.sql$" frontend/drizzle/ | head -20Repository: DevLoversTeam/devlovers.net
Length of output: 668
Add explicit ON DELETE behavior for product_id foreign key.
The product_id foreign key lacks an ON DELETE clause and defaults to NO ACTION. This means attempting to delete a product with existing inventory moves will fail. While the codebase handles this through careful deletion ordering in tests, add an explicit constraint to prevent runtime failures: ON DELETE CASCADE if inventory records should follow product lifecycle, ON DELETE SET NULL if they should be preserved as historical records, or explicitly document NO ACTION if preventing product deletion while moves exist is intentional.
🤖 Prompt for AI Agents
In @frontend/drizzle/0009_p0_inventory_workflow_baseline.sql around lines 24-25,
The product_id foreign key currently has no explicit ON DELETE behavior; update
the constraint in the inventory moves table definition by adding an explicit ON
DELETE clause: e.g., change "product_id uuid not null references products(id)"
to "product_id uuid not null references products(id) ON DELETE CASCADE" to
cascade deletions, or if you need to preserve historical moves, make product_id
nullable and use "product_id uuid references products(id) ON DELETE SET NULL"
instead; apply the chosen change to the product_id column definition in the SQL
diff.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 54c1a6dae9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| eq(orders.id, order.id), | ||
| ne(orders.paymentStatus, 'paid'), | ||
| ne(orders.paymentStatus, 'failed'), | ||
| ne(orders.paymentStatus, 'refunded') |
There was a problem hiding this comment.
Stripe can emit payment_intent.payment_failed for an initial attempt and then later emit payment_intent.succeeded if the customer retries with a new payment method; this handler marks the order as failed (and restocks) on the failed event, and then the success update is gated by ne(orders.paymentStatus, 'failed'), so the later success is ignored and the order stays failed even though funds were captured. This can lead to paid orders not being fulfilled and inventory already released to others. Consider allowing the succeeded transition from failed (or only marking failed on terminal cancel/refund events) to keep webhook state consistent.
Useful? React with 👍 / 👎.
…s, minor-unit totals, env parsing, Stripe apiVersion, test robustness, and variant-safe item merging
There was a problem hiding this comment.
Actionable comments posted: 1
Fix all issues with AI Agents 🤖
In @frontend/app/[locale]/shop/checkout/payment/[orderId]/page.tsx:
- Around line 121-151: The code is using unnecessary (order as any) casts and
swallowing errors silently; remove all `(order as any)` assertions and access
fields directly (e.g., use order.paymentIntentId?.trim() ?? '' and order.id,
order.totalAmountMinor, order.currency) inside the payments initialization
block, and in the catch block log the caught error with context (e.g.,
console.error or the project logger) so failures in retrievePaymentIntent,
createPaymentIntent, or setOrderPaymentIntent are observable while keeping
behavior unchanged.
🧹 Nitpick comments (9)
frontend/lib/env/cloudinary.ts (1)
54-72: ConsidersafeParsefor defensive error handling.While the manual checks (lines 56-64) ensure the required env vars are present, using
safeParseon line 66 would be more defensive and ensure onlyCloudinaryDisabledErroris thrown, maintaining a consistent error contract even in unexpected edge cases.🔎 Proposed refactor
if (missing.length) { throw new CloudinaryDisabledError( `Cloudinary is not configured. Missing: ${missing.join(", ")}` ); } - const parsed = cloudinaryRequiredSchema.parse(process.env); + const res = cloudinaryRequiredSchema.safeParse(process.env); + if (!res.success) { + throw new CloudinaryDisabledError( + `Cloudinary configuration is invalid: ${res.error.message}` + ); + } + + const parsed = res.data; return { cloudName: parsed.CLOUDINARY_CLOUD_NAME, apiKey: parsed.CLOUDINARY_API_KEY, apiSecret: parsed.CLOUDINARY_API_SECRET, uploadFolder: parsed.CLOUDINARY_UPLOAD_FOLDER, };frontend/lib/services/products.ts (2)
559-569: Clarify cleanup comment for accuracy.The comment states "product update is not yet guaranteed to have happened," but the price upsert completes before the product update (lines 522-534 precede lines 538-542). The cleanup logic is correct—the uploaded image should be removed on any failure—but the comment could mislead readers.
🔎 Suggested clarification
- // IMPORTANT: цей cleanup валідний, бо product update ще не гарантує що відбувся + // IMPORTANT: cleanup new image on failure (price upsert or product update)
614-635: Consider more specific types instead ofunknownfor price fields.The return type uses
unknownforpriceMinorandoriginalPriceMinor, which forces consumers to validate these values. While this may be intentional for transitional compatibility, usingnumber | nullwould provide better type safety and align with the DB schema expectations.🔎 Proposed type refinement
export async function getAdminProductPrices(productId: string): Promise< Array<{ currency: CurrencyCode; - priceMinor: unknown; - originalPriceMinor: unknown; + priceMinor: number; + originalPriceMinor: number | null; price: unknown; originalPrice: unknown; }>frontend/db/schema/shop.ts (1)
333-362: Callback should return an array for consistency with other table definitions.The
inventoryMovestable's callback returns an object ({ moveKeyUq, orderIdx, ... }), but other tables in this file (e.g.,products,orders,orderItems) return arrays. While both work functionally, using arrays maintains consistency.🔎 Suggested fix for consistency
t => ({ - moveKeyUq: uniqueIndex('inventory_moves_move_key_uq').on(t.moveKey), - orderIdx: index('inventory_moves_order_id_idx').on(t.orderId), - productIdx: index('inventory_moves_product_id_idx').on(t.productId), - qtyCheck: check('inventory_moves_quantity_gt_0', sql`${t.quantity} > 0`), - }) + t => [ + uniqueIndex('inventory_moves_move_key_uq').on(t.moveKey), + index('inventory_moves_order_id_idx').on(t.orderId), + index('inventory_moves_product_id_idx').on(t.productId), + check('inventory_moves_quantity_gt_0', sql`${t.quantity} > 0`), + ] );frontend/lib/tests/order-items-variants.test.ts (1)
78-99: DB query usesas anyfor schema columns that should be typed.Lines 82-83 and 93-95 cast
orderItems.selectedSizeandorderItems.selectedColorasany, suggesting the Drizzle schema inference isn't exposing these columns. This is likely because the columns were added recently and the inferred type hasn't been updated.The schema in
shop.tsdefines these columns, so they should be accessible without casts. Verify the schema exports are correctly regenerated.frontend/lib/services/orders.ts (2)
67-82: Type casts for variant fields should be addressed at schema level.Lines 69-70 cast
orderItems.selectedSizeandorderItems.selectedColorasany. These fields are defined in the schema but the inferred type may not include them. Consider regenerating types or adding explicit type augmentation.
1039-1094: Verify raw SQL table reference matches Drizzle schema.Line 1044 uses raw SQL
sql\inventory_moves`instead of the importedinventoryMoves` table. While functional, this bypasses Drizzle's type safety. Consider using the schema reference for consistency.🔎 Suggested improvement
+import { inventoryMoves } from '@/db/schema'; ... const reservedMoves = await db .select({ - productId: sql<string>`product_id`, - quantity: sql<number>`quantity`, + productId: inventoryMoves.productId, + quantity: inventoryMoves.quantity, }) - .from(sql`inventory_moves`) - .where(and(sql`order_id = ${orderId}::uuid`, sql`type = 'reserve'`)); + .from(inventoryMoves) + .where(and( + eq(inventoryMoves.orderId, orderId), + eq(inventoryMoves.type, 'reserve') + ));frontend/lib/psp/stripe.ts (1)
118-120: Consider logging the original error for debugging.The catch block discards the original error, making it harder to diagnose legitimate signature verification failures (e.g., clock skew, malformed headers). Unlike
createPaymentIntent, this doesn't log before re-throwing.🔎 Proposed fix
- } catch { + } catch (error) { + logError('Stripe webhook signature verification failed', error); throw new Error('STRIPE_INVALID_SIGNATURE'); }frontend/app/api/shop/checkout/route.ts (1)
488-498: Verify error code consistency.
MoneyValueErroris mapped toPRICE_CONFIG_ERRORwith a 500 status, butPriceConfigError(lines 477-482) uses the same code with 400 status. This could confuse clients—same code, different status codes. Consider using a distinct code likePRICE_DATA_ERRORfor the 500 case (corrupted stored data) vsPRICE_CONFIG_ERRORfor the 400 case (missing price configuration).🔎 Proposed fix
if (error instanceof MoneyValueError) { return errorResponse( - 'PRICE_CONFIG_ERROR', + 'PRICE_DATA_ERROR', 'Invalid stored price data for one or more products.', 500, { productId: error.productId, field: error.field, rawValue: error.rawValue, } ); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
frontend/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (18)
frontend/app/[locale]/shop/checkout/payment/StripePaymentClient.tsxfrontend/app/[locale]/shop/checkout/payment/[orderId]/page.tsxfrontend/app/[locale]/shop/checkout/success/page.tsxfrontend/app/api/shop/checkout/route.tsfrontend/db/schema/shop.tsfrontend/drizzle/0012_inventory_moves_product_fk_restrict.sqlfrontend/drizzle/meta/0012_snapshot.jsonfrontend/drizzle/meta/_journal.jsonfrontend/lib/cart.tsfrontend/lib/env/cloudinary.tsfrontend/lib/psp/stripe.tsfrontend/lib/services/orders.tsfrontend/lib/services/products.tsfrontend/lib/shop/cart-item-key.tsfrontend/lib/tests/admin-api-killswitch.test.tsfrontend/lib/tests/order-items-variants.test.tsfrontend/lib/tests/restock-order-only-once.test.tsfrontend/package.json
✅ Files skipped from review due to trivial changes (1)
- frontend/drizzle/meta/0012_snapshot.json
🚧 Files skipped from review as they are similar to previous changes (3)
- frontend/lib/tests/restock-order-only-once.test.ts
- frontend/app/[locale]/shop/checkout/payment/StripePaymentClient.tsx
- frontend/drizzle/meta/_journal.json
🧰 Additional context used
🧬 Code graph analysis (6)
frontend/lib/psp/stripe.ts (2)
frontend/lib/env/stripe.ts (1)
getStripeEnv(17-49)frontend/lib/logging.ts (1)
logError(1-14)
frontend/lib/shop/cart-item-key.ts (1)
frontend/lib/cart.ts (1)
createCartItemKey(49-49)
frontend/app/api/shop/checkout/route.ts (2)
frontend/lib/services/errors.ts (5)
IdempotencyConflictError(5-13)InvalidPayloadError(35-41)InsufficientStockError(15-21)PriceConfigError(43-53)OrderStateInvalidError(62-85)frontend/lib/logging.ts (2)
logWarn(17-21)logError(1-14)
frontend/app/[locale]/shop/checkout/success/page.tsx (3)
frontend/lib/validation/shop.ts (1)
orderIdParamSchema(371-373)frontend/app/[locale]/shop/checkout/success/OrderStatusAutoRefresh.tsx (1)
OrderStatusAutoRefresh(16-42)frontend/lib/shop/currency.ts (1)
formatMoney(115-128)
frontend/app/[locale]/shop/checkout/payment/[orderId]/page.tsx (1)
frontend/lib/psp/stripe.ts (2)
retrievePaymentIntent(71-90)createPaymentIntent(29-69)
frontend/lib/services/orders.ts (7)
frontend/db/schema/shop.ts (1)
orders(107-198)frontend/lib/shop/cart-item-key.ts (1)
createCartItemKey(1-7)frontend/lib/shop/money.ts (3)
fromCents(44-46)sumLineTotals(70-76)toDbMoney(58-60)frontend/db/queries/shop/orders.ts (2)
OrderSummary(33-33)InsufficientStockError(8-8)frontend/lib/services/errors.ts (5)
OrderNotFoundError(23-29)OrderStateInvalidError(62-85)InsufficientStockError(15-21)InvalidPayloadError(35-41)IdempotencyConflictError(5-13)frontend/lib/services/inventory.ts (2)
applyReserveMove(49-116)applyReleaseMove(118-178)frontend/lib/env/stripe.ts (1)
isPaymentsEnabled(51-53)
🔇 Additional comments (39)
frontend/lib/env/cloudinary.ts (3)
3-10: LGTM!The custom error class follows best practices with a typed
codeproperty and proper name assignment.
12-17: LGTM!The schema correctly validates required Cloudinary configuration with appropriate defaults for the upload folder.
30-48: Past concern resolved—function now matches its "never throws" contract.The previous review correctly identified that
.parse()could throw aZodError. The current implementation uses.safeParse()(line 37) and returnsnullon validation failure (line 38), which properly honors the docstring guarantee.frontend/lib/services/products.ts (3)
351-425: Transactionless design trade-off is acceptable.The migration from transactional to sequential inserts (product → prices) introduces a window where the product exists without prices. The best-effort cleanup on failure is appropriate for Neon HTTP compatibility goals stated in the PR objectives.
687-687: LGTM: Dual price representation is consistent.The cart summary correctly maintains both canonical minor units (
totalAmountMinor) and display values (totalAmount) for empty and non-empty carts. This aligns with the PR's minor-unit money handling objectives.Also applies to: 833-840
739-795: Excellent defensive validation for price data.The price derivation logic correctly handles both canonical
priceMinorand legacypricefields with comprehensive validation:
- Critical rejection of fractional minor units (line 762)
- Safe integer range checks (lines 771-776, 789-795)
- Graceful fallback to legacy pricing when needed
The dual representation (canonical
unitPriceMinor/lineTotalMinorand displayunitPrice/lineTotal) is well-structured and consistent.Also applies to: 809-814
frontend/drizzle/0012_inventory_moves_product_fk_restrict.sql (1)
1-6: LGTM! ON DELETE RESTRICT preserves inventory audit trail.The migration correctly enforces referential integrity by preventing product deletion when related
inventory_movesrecords exist. TheDROP IF EXISTSmakes the migration re-runnable.frontend/db/schema/shop.ts (4)
34-54: LGTM! Well-structured enums for the inventory workflow.The enum definitions properly model the order and inventory state machines. The
inventory_move_typeenum withreserve/releasealigns with the ledger-based inventory tracking approach.
144-166: LGTM! Sweep claim fields enable distributed restock coordination.The
sweepClaimedAt,sweepClaimExpiresAt,sweepRunId, andsweepClaimedByfields provide the necessary infrastructure for distributed lease-based restock claim gating, preventing concurrent double-processing.
180-196: LGTM! CHECK constraint enforces PSP field invariants.The
orders_psp_fields_null_when_noneconstraint correctly ensures that PSP-specific fields (pspChargeId,pspPaymentMethod,pspStatusReason) are NULL when provider isnone. The sweep claim index supports efficient stale order queries.
211-237: LGTM! Variant fields with NOT NULL defaults enable reliable upserts.Using
NOT NULL DEFAULT ''forselectedSizeandselectedColorensures the unique indexorder_items_order_variant_uqworks correctly withON CONFLICTclauses, avoiding NULL comparison issues.frontend/lib/shop/cart-item-key.ts (1)
1-7: LGTM! Clean deterministic key generation for variant-aware cart items.The
::delimiter creates a stable composite key forproductId + size + colorcombinations, enabling reliable deduplication and merging across cart operations.frontend/lib/tests/order-items-variants.test.ts (2)
100-116: LGTM! Thorough cleanup with cascade awareness.The cleanup correctly relies on cascade deletes for
order_itemsandinventory_moves, with a best-effort fallback for orphanedproduct_prices. The empty catch block is acceptable here as it's truly best-effort cleanup.
48-63: Remove unnecessaryas anycasts;CheckoutItemalready includes variant fields.The
CheckoutItemtype already definesselectedSizeandselectedColoras optional properties via thecheckoutItemSchema(seefrontend/lib/validation/shop.ts:287-294). Theas anycasts on lines 55 and 62 are unnecessary and should be removed to maintain type safety.Likely an incorrect or invalid review comment.
frontend/lib/tests/admin-api-killswitch.test.ts (2)
95-115: LGTM! Body stream consumption issue resolved.The implementation correctly reads the response body once via
res.text()and then parses JSON, avoiding the "body already read" error that would occur with separateres.json()andres.text()calls.
143-172: LGTM! Comprehensive kill-switch coverage for admin routes.The test properly:
- Mocks production environment with disabled admin API
- Tests all mutation methods (POST, PUT, PATCH, DELETE)
- Verifies 403 response with
ADMIN_API_DISABLEDcode- Covers both static and dynamic routes with appropriate context
frontend/lib/cart.ts (2)
119-154: LGTM! Minor-unit accumulation and currency validation properly implemented.The function now correctly:
- Accumulates
item.lineTotalMinor(canonical integer cents) instead of the display value- Computes
totalAmountviafromCents(totalMinor)for display consistency- Throws on mixed currencies with a clear error message
This addresses the previous review concern about incorrect field accumulation.
22-31: LGTM! Empty cart summary includes canonical totalAmountMinor.The
emptyCartconstant properly initializes bothtotalAmountMinor: 0(canonical) andtotalAmount: 0(display), maintaining consistency with the new money handling approach.frontend/lib/services/orders.ts (9)
43-51: LGTM! Variant normalization ensures consistent key generation.The
normVariantfunction trims whitespace and handles null/undefined, ensuring consistent composite keys across cart merging and idempotency hashing.
147-182: LGTM! Deterministic idempotency hashing with stable canonical form.The
hashIdempotencyRequestfunction:
- Normalizes variants via
normVariant- Sorts items by composite key for determinism
- Includes version, currency, userId, and items in the hash payload
- Uses SHA-256 for collision resistance
This ensures the same logical request always produces the same hash.
301-438: LGTM! reconcileNoPaymentOrder handles crash recovery correctly.The function properly:
- Validates order state before reconciliation
- Aggregates reserves by productId
- Transitions through
reserving→reservedor rolls back toreleased- Uses
applyReleaseMovein a best-effort catch block during failureThe empty catch on line 416 is intentional—release is best-effort during error recovery.
571-665: Idempotency conflict handling is robust.The
assertIdempotencyCompatiblefunction:
- Validates currency match (hard conflict if different)
- Falls back to derived hash for legacy orders without stored hash
- Backfills missing hash for future strict checks
- Re-throws
InsufficientStockErrorfor failed orders with cleanup attemptThis ensures the same idempotency key with different payloads triggers a 409.
840-872: LGTM! Proper rollback on reservation failure.The error handling:
- Sets
inventoryStatus: 'release_pending'immediately- Best-effort releases each reserved item
- Updates order to terminal
INVENTORY_FAILEDstate- Preserves error context in
failureCode/failureMessage- Re-throws original error for caller handling
The empty catch blocks during release are intentional for best-effort cleanup.
972-1003: LGTM! Distributed lease claim with expiration prevents concurrent processing.
tryClaimRestockLeaseatomically claims the restock lease using conditionalUPDATE ... WHERE ... RETURNING:
- Only claims if
stockRestored = false- Respects existing unexpired claims
- Sets
sweepRunIdfor traceabilityThe
or(isNull(...), lt(...))condition correctly handles both unclaimed and expired leases.
1160-1249: LGTM! Batch restock sweep with atomic claim pattern.The
restockStalePendingOrdersfunction:
- Uses a subquery to select candidates deterministically (oldest first)
- Atomically claims a batch with
UPDATE ... WHERE id IN (subquery) ... RETURNING- Re-checks claim gate at UPDATE time to prevent stealing active claims
- Processes claimed orders sequentially with
alreadyClaimed: true- Loops until no more candidates
This pattern safely handles concurrent sweep workers.
1251-1289: LGTM! No-payment stale order cleanup handles orphans.
restockStaleNoPaymentOrdersspecifically targets orders withpaymentProvider = 'none'that didn't complete inventory reservation. It correctly excludes alreadyreservedorreleasedstates and marks orphans as terminal failed.
782-799: Unique constraint verified. The database constraintorder_items_order_variant_uq(in frontend/db/schema/shop.ts) correctly includes all four columns: orderId, productId, selectedSize, and selectedColor. The upsert operation is properly configured.frontend/package.json (1)
48-48: Good practice: exact version pinning for Stripe SDK.Pinning to
20.0.0(removing the^caret) ensures deterministic builds and prevents unintended upgrades that could introduce breaking changes in payment flows. This is particularly important for payment integrations.frontend/lib/psp/stripe.ts (2)
12-27: LGTM: Lazy client caching with key invalidation.The caching pattern correctly handles secretKey rotation by comparing the current key against the cached one. This prevents unnecessary client recreation while ensuring fresh credentials are used when they change.
29-69: LGTM: Payment intent creation with proper validation.Good defensive checks for payments being enabled, valid client, and positive finite amounts. The idempotency key pass-through correctly uses conditional spreading. Error logging before re-throwing preserves diagnostics while exposing clean error codes to callers.
frontend/app/[locale]/shop/checkout/success/page.tsx (3)
10-11: LGTM: Proper dynamic rendering configuration.Setting
dynamic = 'force-dynamic'andrevalidate = 0ensures the page always fetches fresh order data, which is critical for displaying accurate payment status after webhook processing.
126-131: LGTM: Clean implementation addressing prior feedback.Direct access to
order.totalAmountMinor(line 126) and locale-agnostic Link hrefs (e.g.,/shop/products) correctly address the prior review comments about unnecessary type assertions and double locale prefixing.
144-147: Good UX: Informative payment status messaging.The conditional messaging clearly communicates to users whether payment is confirmed or still processing, and the auto-refresh component handles the polling automatically.
frontend/app/[locale]/shop/checkout/payment/[orderId]/page.tsx (2)
60-69: LGTM: Locale-agnostic navigation paths.Links correctly use paths like
/shop/cartand/shop/productswithout manual locale prefixing, allowing the next-intl Link component to handle locale automatically.
204-204: LGTM: Consistent minor-unit money formatting.The
formatMoneycalls correctly useorder.totalAmountMinorwith the currency and locale, ensuring consistent minor-unit handling across the payment flow.frontend/app/api/shop/checkout/route.ts (4)
28-52: LGTM: Clean separation of expected vs unexpected errors.The
EXPECTED_BUSINESS_ERROR_CODESset combined withisExpectedBusinessError()helper enables appropriate logging levels—warnings for expected business rejections (idempotency conflicts, validation failures) and errors for unexpected failures. The instanceof fallback ensures robustness if.codeis missing.
152-161: Good change: Downgrade to warning for expected parse failures.Using
logWarninstead oflogErrorfor JSON parse failures prevents noisy error logs in CI/tests when clients send malformed payloads—a 4xx client error, not a server issue.
241-252: Good addition: Stable 409 response for already-failed orders.Returning a 409 Conflict with
CHECKOUT_FAILEDfor orders that already failed (e.g., due to inventory issues) provides clients with a stable, idempotent response instead of a confusing 500 error.
464-466: LGTM: Idempotency conflict handling.Correctly returns 409 with the error code, message, and details for
IdempotencyConflictError, enabling clients to understand why their idempotent request was rejected (payload mismatch).
…A_ERROR (500) in checkout error contract
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI Agents
In @frontend/app/api/shop/checkout/route.ts:
- Around line 28-34: Remove 'OUT_OF_STOCK' from the
EXPECTED_BUSINESS_ERROR_CODES Set in frontend/app/api/shop/checkout/route.ts
because that code is never returned to the client; update the constant
declaration (EXPECTED_BUSINESS_ERROR_CODES) to exclude 'OUT_OF_STOCK' and leave
the remaining codes intact, relying on the existing instanceof
InsufficientStockError check in the checkout route to handle stock-related
fallback cases and noting orders.ts still storing 'OUT_OF_STOCK' as a DB
failureCode is fine.
🧹 Nitpick comments (6)
frontend/lib/tests/stripe-webhook-psp-fields.test.ts (2)
29-39: Consider removing theas anytype assertion.The type assertion on line 38 bypasses TypeScript's type checking. While common in tests, it could hide type mismatches.
🔎 Suggested improvement
function makeWebhookRequest(rawBody: string) { return new NextRequest('http://localhost:3000/api/shop/webhooks/stripe', { method: 'POST', headers: { 'Content-Type': 'application/json', // route calls verifyWebhookSignature(rawBody, signatureHeader) 'Stripe-Signature': 't=1,v1=test', }, body: rawBody, - } as any); + }); }If this causes type errors, explicitly type the init parameter rather than using
as any.
165-253: Consider verifying the mock was called and order status transitions.The test thoroughly validates PSP field population and idempotency. Two optional enhancements:
- Mock verification: Confirm
verifyWebhookSignaturewas called with expected arguments.- Order status transition: The test checks
paymentStatustransitions to'paid'but doesn't verify iforder.statusshould also transition (e.g., from'INVENTORY_RESERVED'to a final state like'COMPLETED').🔎 Optional enhancements
1. Add mock verification after line 175:
expect(verifyWebhookSignature).toHaveBeenCalledWith({ rawBody, signatureHeader: 't=1,v1=test', });2. If order.status should transition, add assertion after line 194:
// Verify order.status transitions appropriately expect(updated1[0].status).toBe('COMPLETED'); // or expected final statusfrontend/app/api/shop/checkout/route.ts (1)
36-52: Consider usingunknowninstead ofanyfor safer type handling.The
getErrorCodefunction castserrtoanyfor property access. While pragmatic, consider usingunknownwith explicit type guards for better type safety:function getErrorCode(err: unknown): string | null { if (typeof err !== 'object' || err === null) return null; const e = err as { code?: unknown }; return typeof e.code === 'string' ? e.code : null; }The dual checking strategy (code-based + instanceof) in
isExpectedBusinessErrorprovides good defense in depth.frontend/lib/tests/checkout-no-payments.test.ts (1)
85-102: Consider logging suppressed cleanup errors.The empty
catch {}blocks in cleanup functions silently swallow errors. While this is intentional for resilience during test teardown, it can make debugging difficult when cleanup genuinely fails.🔎 Optional: Add debug logging for cleanup failures
async function cleanupIsolatedProduct(productId: string) { // Make sure it won't be visible for any selector. try { await db .update(products) .set({ isActive: false, updatedAt: new Date() } as any) .where(eq(products.id, productId)); - } catch {} + } catch (e) { + // Cleanup failure is non-fatal but useful for debugging + if (process.env.DEBUG) console.warn('cleanupIsolatedProduct: deactivate failed', e); + }frontend/lib/services/orders.ts (2)
855-859: Empty catch blocks in release loops may hide critical failures.The best-effort release loops silently swallow all errors. While this prevents one failed release from blocking others, it could hide systematic issues like database connectivity problems.
🔎 Optional: Log release failures for observability
for (const it of itemsToReserve) { try { await applyReleaseMove(orderId, it.productId, it.quantity); - } catch {} + } catch (releaseErr) { + // Best-effort: log but don't throw to continue releasing other items + console.error(`[restockOrder] release failed for product ${it.productId}:`, releaseErr); + } }Also applies to: 420-424
1263-1301:restockStaleNoPaymentOrdersqueries without atomic claiming, unlikerestockStalePendingOrders.Two concurrent workers could select the same stale order. However,
restockOrderinternally acquires a per-call lease (lines 1122-1128), so actual double-processing is prevented. The inefficiency is limited to wasted DB reads and failed lease attempts.Currently,
restockStaleNoPaymentOrdersis test-only and not wired to any production endpoint. If used in a concurrent context, consider adding atomic claiming for consistency withrestockStalePendingOrders(which uses batch-level claiming at lines 1226-1248), though the current approach is acceptable given the expected low volume of stale no-payment orders.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
frontend/app/[locale]/shop/checkout/payment/[orderId]/page.tsxfrontend/app/api/shop/checkout/route.tsfrontend/components/shop/cart-provider.tsxfrontend/db/schema/shop.tsfrontend/lib/env/cloudinary.tsfrontend/lib/psp/stripe.tsfrontend/lib/services/orders.tsfrontend/lib/services/products.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/stripe-webhook-psp-fields.test.ts
✅ Files skipped from review due to trivial changes (1)
- frontend/components/shop/cart-provider.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/lib/psp/stripe.ts
- frontend/lib/tests/order-items-variants.test.ts
🧰 Additional context used
🧬 Code graph analysis (3)
frontend/lib/tests/checkout-no-payments.test.ts (4)
frontend/db/index.ts (1)
db(39-39)frontend/db/schema/shop.ts (3)
products(56-105)productPrices(277-331)orders(107-198)frontend/lib/shop/money.ts (1)
toDbMoney(58-60)frontend/lib/services/orders.ts (3)
restockOrder(1012-1170)restockOrder(1349-1349)restockStaleNoPaymentOrders(1264-1301)
frontend/lib/tests/stripe-webhook-psp-fields.test.ts (3)
frontend/db/index.ts (1)
db(39-39)frontend/db/schema/shop.ts (5)
stripeEvents(262-275)orderItems(200-260)orders(107-198)productPrices(277-331)products(56-105)frontend/lib/psp/stripe.ts (1)
verifyWebhookSignature(97-122)
frontend/app/api/shop/checkout/route.ts (2)
frontend/lib/services/errors.ts (5)
IdempotencyConflictError(5-13)InvalidPayloadError(35-41)InsufficientStockError(15-21)PriceConfigError(43-53)OrderStateInvalidError(62-85)frontend/lib/logging.ts (2)
logWarn(17-21)logError(1-14)
🔇 Additional comments (45)
frontend/lib/tests/stripe-webhook-psp-fields.test.ts (2)
1-28: LGTM: Clean test setup with appropriate mocking.The partial mock of
@/lib/psp/stripepreserves actual implementations while allowing controlled test behavior for webhook signature verification.
135-163: Verify amount/currency matching is tested per PR requirements.The PR description highlights "amount/currency verification in minor units with diagnostics stored in pspStatusReason/pspMetadata" as a key reliability improvement. This test validates the happy path (matching amount 900 and currency), but doesn't verify that mismatches are rejected.
Consider adding a companion test case that verifies webhook rejection when:
- Event amount doesn't match
order.totalAmountMinor- Event currency doesn't match
order.currencyThis would ensure the webhook's defensive verification logic is covered, especially since the PR emphasizes this as a hardening measure.
frontend/app/[locale]/shop/checkout/payment/[orderId]/page.tsx (7)
1-11: LGTM!Imports are well-organized and appropriate for the Stripe payment intent lifecycle integration.
12-37: LGTM!Helper functions are well-structured:
getOrderIdvalidates with Zod,resolveClientSecretsafely handles array/undefined searchParams, andbuildStatusMessageprovides clear user-facing status text.
80-127: LGTM!The type annotation at line 80 properly types
orderusing the return type ofgetOrderSummary, eliminating the need for type assertions. Error handling correctly differentiatesOrderNotFoundErrorfrom generic failures with appropriate user-facing messages.
135-184: Well-implemented payment intent lifecycle with proper idempotency.This addresses the concerns from previous reviews:
- Direct field access without
as anycasts (line 142, 157-160, 165-166)- Structured error logging with phase tracking and context (lines 172-180)
The idempotency key
pi:${order.id}ensures Stripe returns the same PaymentIntent on retries, and the phase tracking provides good observability for debugging failures in each step.
186-213: LGTM!Correct early return for already-paid orders prevents users from re-entering the payment flow unnecessarily.
62-73: LGTM!Using absolute paths (e.g.,
/shop/cart,/shop/products) with theLinkcomponent from@/i18n/routingis correct—the routing system handles locale prefixing automatically.
250-259:formatMoneycorrectly expects minor units as the first argument.The function signature confirms it accepts
amountMinor: numberand internally converts to major units viaminorToMajor()before formatting. The consistent use oforder.totalAmountMinorthroughout the file aligns with this design.frontend/lib/env/cloudinary.ts (1)
1-79: LGTM! Robust environment configuration with proper error contracts.The past review concern about
getCloudinaryEnvOptionalpotentially throwing has been addressed by usingsafeParse(line 37). The implementation now correctly fulfills its documented contract of never throwing.The dual-accessor pattern is well-designed:
getCloudinaryEnvOptionalsafely returnsnullfor any missing or invalid configurationgetCloudinaryEnvRequiredthrows a typedCloudinaryDisabledErrorwith actionable error messagesThe manual presence checks before schema validation (lines 31-35, 54-57) provide better developer experience than raw Zod errors, while
safeParseensures comprehensive validation including the optionalCLOUDINARY_UPLOAD_FOLDERfield.frontend/app/api/shop/checkout/route.ts (9)
5-5: LGTM! Imports support enhanced error handling.The new imports for
logWarnandIdempotencyConflictErrorcorrectly support the improved error classification and logging strategy introduced in this PR.Also applies to: 7-7
152-156: LGTM! Appropriate use oflogWarnfor expected client errors.The change from
logErrortologWarnfor JSON parsing failures correctly treats these as expected business errors (4xx) rather than internal failures, avoiding unnecessary stack trace noise in logs.
186-188: LGTM! Consistent warning-level logging for validation errors.Appropriately uses
logWarnfor schema validation failures, maintaining consistency with other expected client errors.
241-252: LGTM! Proper handling of pre-failed orders when payments are disabled.This correctly handles the edge case where an order was created but failed (e.g., due to inventory issues) when payments are disabled. Returning 409 with the order ID maintains idempotency contract and provides useful client context.
266-268: LGTM! Enhanced validation prevents inconsistent payment state.The additional check for
paymentIntentIdensures that orders withpaymentProvider: 'none'don't have orphaned payment intents, preventing data inconsistencies.
447-454: LGTM! Proper error classification improves observability.The differentiation between expected business errors (warning-level) and unexpected internal errors (error-level) significantly improves log quality and reduces noise in monitoring systems.
464-466: LGTM! Correct idempotency conflict handling.Properly returns 409 status with detailed context, maintaining the strict idempotency contract described in the PR objectives.
471-473: LGTM! Enhanced diagnostics for order state errors.The additional
field,rawValue, anddetailsfields provide valuable debugging context for internal data integrity issues without exposing sensitive information.
490-491: LGTM! Correctly differentiates price configuration from data corruption errors.This change properly implements the error contract described in the PR objectives:
PRICE_DATA_ERROR(500) for corrupted stored data vs.PRICE_CONFIG_ERROR(400) for missing price configuration.frontend/lib/tests/order-items-snapshot-immutable.test.ts (3)
1-56: LGTM! Test setup and utilities are well-structured.The mocking strategy correctly isolates the test from auth and payment dependencies. The
makeJsonRequesthelper and cleanup function follow established patterns in the test suite.
73-162: Test correctly validates snapshot immutability invariant.The test flow is comprehensive: seed → checkout → capture baseline → mutate source data → verify snapshot unchanged. The assertions on Lines 155-161 properly validate all snapshotted fields before mutation.
163-213: LGTM! Mutation and verification logic is correct.The test aggressively mutates both
productsandproductPricestables, then verifies theorderItemssnapshot remains unchanged viatoEqual. Thefinallyblock ensures cleanup regardless of test outcome.frontend/lib/tests/checkout-no-payments.test.ts (4)
34-83: Test isolation strategy is sound but relies on existing data.The
createIsolatedProductForCurrencyfunction clones an existing active product to satisfy NOT NULL constraints. This is a pragmatic approach for integration tests, but the error message at Line 48-50 clearly communicates the prerequisite.The pattern of creating products as inactive by default (Line 65) and activating only during the test window is a good strategy to prevent interference from parallel tests.
160-277: LGTM! No-payments success path test is thorough.The test validates the complete lifecycle:
- Product activation window minimization (Lines 170-174, 201-205)
- Response-level contract (Lines 207-210)
- DB state verification including
inventoryStatus: 'reserved'as the finality signal (Lines 212-233)- Ledger integrity with exactly one reserve move (Lines 235-243)
- Stock decrement verification (Lines 245-253)
- Restock cleanup restores stock (Lines 255-266)
The 20-second timeout is appropriate for integration tests with DB operations.
279-368: Idempotency test correctly validates contract.The test verifies three critical scenarios:
- Same idempotency key + same payload → returns same order ID (Line 333)
- No double-reserve on replay (Line 337)
- Same key + different payload → 409 conflict (Lines 340-345)
370-436: Orphan cleanup test validates terminal state transition.The test inserts an orphan order (no inventory_moves) with
status: 'CREATED'andinventoryStatus: 'none', then validatesrestockStaleNoPaymentOrdersmarks it terminal withfailureCode: 'STALE_ORPHAN'.Note: The orphan is created with
paymentStatus: 'paid'(Line 380) which is required by the DB CHECK constraint forpaymentProvider: 'none', but the test expectspaymentStatus: 'failed'after sweep (Line 430). This is correct behavior—the sweep transitions it to a terminal failed state.frontend/db/schema/shop.ts (5)
34-54: LGTM! New enums model order and inventory lifecycle correctly.The
orderStatusEnumcaptures the order state machine:CREATED → INVENTORY_RESERVED → PAIDorCREATED → INVENTORY_FAILED → CANCELED. TheinventoryStatusEnumprovides fine-grained tracking of reservation lifecycle for transactionless safety.
144-166: LGTM! Order table extensions support idempotency and distributed sweeps.The new columns enable:
status/inventoryStatus: Explicit state machine trackingfailureCode/failureMessage: Structured error diagnosticsidempotencyRequestHash: Payload fingerprinting for conflict detectionsweepClaimed*fields: Distributed lease mechanism for concurrent sweep workers
180-197: LGTM! CHECK constraints enforce provider-specific invariants.
orders_psp_fields_null_when_nonecorrectly ensures PSP fields are null whenpaymentProvider = 'none'. The updatedorders_payment_status_valid_when_noneconstraint restricts status to('paid','failed')for no-payment orders, preventing invalidpending/requires_paymentstates.The
orders_sweep_claim_expires_idxindex optimizes sweep queries filtering by claim expiration.
200-259: LGTM! Order items support variant selection with composite uniqueness.The
selectedSizeandselectedColorcolumns withNOT NULL DEFAULT ''enable the unique composite indexorder_items_order_variant_uqto work reliably withON CONFLICTclauses. This is a well-known pattern for making nullable-like columns participate in unique constraints.
333-362: LGTM! Inventory moves table design supports idempotent operations.Key design choices:
moveKeyunique index enables idempotent reserve/release viaON CONFLICT DO NOTHINGON DELETE CASCADEonorderIdensures cleanup when orders are deleted- No cascade on
productIdpreserves audit trail even if products are deletedquantity > 0check prevents invalid zero-quantity moves- Separate indices on
orderIdandproductIdoptimize common query patternsfrontend/lib/services/products.ts (6)
43-51: LGTM! AdminProductPriceRow type provides dual representation.The type exposes both canonical minor-unit fields (
priceMinor,originalPriceMinor) and legacy string fields (price,originalPrice) for backward compatibility during rollout.
361-434: LGTM! createProduct implements robust insert-then-cleanup pattern.The implementation:
- Inserts product with
onConflictDoNothing(Line 391)- Checks for null return indicating slug conflict (Lines 394-396)
- Stores
createdProductIdfor cleanup (Line 398)- Inserts prices with canonical and legacy mirrors (Lines 400-419)
- On any error, deletes the product (CASCADE handles prices) at Lines 423-431
This pattern provides atomicity without transactions by using cleanup on failure.
513-565: LGTM! updateProduct uses upsert for prices with proper conflict handling.The
onConflictDoUpdateon[productPrices.productId, productPrices.currency](Line 535) correctly handles both insert and update cases. Thesql\excluded.*`` syntax (Lines 537-541) is the correct Drizzle pattern for referencing the conflicting row's values.The product update happens after price upsert (Lines 546-555), and image cleanup on failure (Lines 567-577) ensures no orphaned Cloudinary assets.
622-655: Defensive runtime validation on priceMinor values.The
assertMoneyMinorIntcalls (Lines 641-651) guard against driver bugs or data corruption where integer columns might return non-integer values. This is appropriately paranoid for financial data.
752-760: PriceConfigError correctly distinguishes missing price configuration.When both
priceMinorandpriceare null (indicating no price row for the requested currency), aPriceConfigErroris thrown with context (productId,currency). This aligns with the PR's error contract differentiatingPRICE_CONFIG_ERROR(400) fromPRICE_DATA_ERROR(500).
846-854: LGTM! Cart summary exposes both canonical and display totals.The
summaryobject now includes:
totalAmountMinor: Canonical integer minor units for calculationstotalAmount: Display value fromfromCents(totalCents)for UIThis dual representation supports the PR's money handling strategy.
frontend/lib/services/orders.ts (8)
1-46: LGTM! Import additions support new inventory and idempotency features.The new imports correctly bring in:
- Inventory operations (
applyReserveMove,applyReleaseMove,inventoryMoves)- Error types (
IdempotencyConflictError)- Variant key generation (
createCartItemKey)
118-152: LGTM! Item merging and aggregation handle variants correctly.
mergeCheckoutItems(Lines 118-140) keys by variant-awarecreateCartItemKey, allowing multiple entries for the same product with different size/color combinations.
aggregateReserveByProductId(Lines 142-152) correctly aggregates quantities byproductIdonly (ignoring variants) because stock is tracked at the product level, not variant level. The deterministic sort ensures consistent reservation order.
154-189: LGTM! Idempotency hash is deterministic and collision-resistant.The hash includes:
- Version field (
v: 1) for future schema evolution- Normalized items sorted by cart key for stability
- Currency and userId for request identity
SHA256 provides sufficient collision resistance for idempotency purposes.
308-445: reconcileNoPaymentOrder handles crash recovery correctly.The function:
- Validates state (Lines 323-358): checks for terminal states, provider consistency
- Sets
inventoryStatus: 'reserving'with guard (Lines 373-382)- Reserves inventory per product (Lines 384-398)
- On success: transitions to
PAID/reserved(Lines 400-412)- On failure: rolls back to
release_pending, releases moves, marksINVENTORY_FAILED(Lines 413-444)The guard at Line 377-381 (
ne(orders.inventoryStatus, 'reserved')) prevents double-reservation on concurrent calls.
578-697: createOrderWithItems entry point handles idempotency correctly.Key flows:
- Currency derived from locale (Line 589)
- Payment status set based on provider (Lines 591-593)
- Items normalized and hashed (Lines 595-602)
assertIdempotencyCompatiblevalidates existing orders (Lines 604-672)- No-payment replay triggers
reconcileNoPaymentOrder(Lines 680-687)The hash backfill at Lines 643-654 is a pragmatic migration strategy for orders created before hash storage.
709-807: Order creation with upsert handles race conditions safely.The flow:
- Insert order skeleton with
status: 'CREATED'(Lines 712-741)- On unique constraint violation, retrieve existing and validate compatibility (Lines 742-764)
- Upsert order items with
onConflictDoUpdate(Lines 767-806)The upsert on
[orderId, productId, selectedSize, selectedColor](Lines 790-795) ensures idempotent item creation even under concurrent retries.
820-883: Inventory reservation with rollback on failure.The reservation loop (Lines 820-833) processes items sequentially. On failure:
- Sets
inventoryStatus: 'release_pending'(Lines 849-852)- Best-effort release for all reserved items (Lines 854-859)
- Marks order as
INVENTORY_FAILEDwith appropriate failure code (Lines 861-876)The sequential processing ensures partial failures can be rolled back cleanly.
1012-1170: restockOrder implements robust distributed restock with lease.Key safety mechanisms:
- Early return if already released (Lines 1038-1043)
- Check for reserved moves to determine work needed (Lines 1046-1057)
- Orphan handling for orders with no moves (Lines 1059-1106)
- Lease claim before processing (Lines 1120-1129)
- Release pending state transition (Lines 1132-1135)
- Atomic finalization with
WHERE stockRestored = false RETURNING(Lines 1142-1152)The guard at Line 1111-1116 prevents restocking paid orders without refund reason, but correctly exempts no-payment orders where
paidis a DB CHECK artifact.
…es and harden error code parsing (unknown + type guard)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
frontend/lib/tests/checkout-no-payments.test.ts (2)
34-83: Consider cleanup on failure increateIsolatedProductForCurrency.If the
db.insert(productPrices)call fails (lines 71-80), the product created at lines 58-68 will be orphaned. This is a minor test hygiene concern.🔎 Suggested improvement
async function createIsolatedProductForCurrency(opts: { currency: 'USD' | 'UAH'; stock: number; }): Promise<{ productId: string }> { const now = new Date(); // Clone a real product row to satisfy NOT NULL columns (schema varies). const [tpl] = await db .select() .from(products) .where(eq(products.isActive as any, true)) .limit(1); if (!tpl) { throw new Error( 'No template product found to clone (need at least 1 active product).' ); } const productId = crypto.randomUUID(); const slug = `t-iso-nopay-${crypto.randomUUID()}`; const sku = `t-iso-nopay-${crypto.randomUUID()}`; // Keep inactive by default to avoid being picked by other tests. await db.insert(products).values({ ...(tpl as any), id: productId, slug, sku, title: `Test ${slug}`, stock: opts.stock, isActive: false, createdAt: now, updatedAt: now, } as any); + try { // Ensure price exists for requested currency (minor + legacy). await db.insert(productPrices).values({ productId, currency: opts.currency, priceMinor: 1000, price: toDbMoney(1000), originalPriceMinor: null, originalPrice: null, createdAt: now, updatedAt: now, } as any); + } catch (e) { + // Cleanup orphaned product on price insert failure + await db.delete(products).where(eq(products.id, productId)).catch(() => {}); + throw e; + } return { productId }; }
150-166: Consider typing the raw SQL result more safely.The
readMovesfunction casts the result withas unknown as MoveRow[], which bypasses type checking. If the SQL column names change, this will fail at runtime without compile-time warnings.🔎 Suggested improvement
async function readMoves(orderId: string): Promise<MoveRow[]> { const res = await db.execute( sql` select product_id as "productId", type, quantity from inventory_moves where order_id = ${orderId}::uuid order by created_at asc ` ); - return (res.rows ?? []) as unknown as MoveRow[]; + return (res.rows ?? []).map(row => ({ + productId: String((row as any).productId ?? ''), + type: String((row as any).type ?? ''), + quantity: Number((row as any).quantity ?? 0), + })); }frontend/lib/services/orders.ts (4)
420-430: Silentconsole.errorin catch block may lose error context.The release loop catches errors but only logs to
console.error. Consider usinglogErrorfor consistency with the rest of the codebase and better observability.🔎 Suggested improvement
+import { logError } from '@/lib/logging'; for (const item of itemsToReserve) { try { await applyReleaseMove(orderId, item.productId, item.quantity); } catch (releaseErr) { - console.error( + logError( '[reconcileNoPaymentOrder] release failed', - { orderId, productId: item.productId, quantity: item.quantity }, releaseErr ); } }
649-660: Empty catch block silently swallows backfill errors.The backfill of
idempotencyRequestHashat lines 651-659 has an empty catch block. While this is intentional (best-effort), consider at least logging at debug level for troubleshooting.🔎 Suggested improvement
try { await db .update(orders) .set({ idempotencyRequestHash: derivedExistingHash, updatedAt: new Date(), }) .where(eq(orders.id, row.id)); - } catch {} + } catch (e) { + // Best-effort backfill; log for debugging if needed + if (process.env.DEBUG) { + console.warn('[assertIdempotencyCompatible] hash backfill failed', { orderId: row.id }, e); + } + }
669-677: Best-effort restock before throwing may mask the original error.At lines 671-673,
restockOrderis called in a try-catch that swallows errors before throwingInsufficientStockError. If the restock fails, the stock remains locked but the error message says "Insufficient stock". Consider logging the restock failure.🔎 Suggested improvement
if (row.paymentStatus === 'failed') { // Best-effort cleanup if inventory was left reserved due to crash. try { await restockOrder(existing.id, { reason: 'failed' }); - } catch {} + } catch (restockErr) { + logError('[assertIdempotencyCompatible] cleanup restock failed', restockErr); + } throw new InsufficientStockError( row.failureMessage ?? 'Insufficient stock.' ); }
860-871: Release loop on failure usesconsole.errorinstead oflogError.Same issue as in
reconcileNoPaymentOrder- consider usinglogErrorfor consistency.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
frontend/app/api/shop/checkout/route.tsfrontend/lib/services/orders.tsfrontend/lib/tests/checkout-no-payments.test.tsfrontend/lib/tests/stripe-webhook-psp-fields.test.ts
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/lib/tests/stripe-webhook-psp-fields.test.ts (3)
frontend/db/index.ts (1)
db(39-39)frontend/db/schema/shop.ts (5)
stripeEvents(262-275)orderItems(200-260)orders(107-198)productPrices(277-331)products(56-105)frontend/lib/psp/stripe.ts (1)
verifyWebhookSignature(97-122)
🔇 Additional comments (26)
frontend/lib/tests/stripe-webhook-psp-fields.test.ts (4)
16-27: LGTM!The mock setup correctly hoists
vi.mockbefore the import ofverifyWebhookSignature, and usesvi.importActualto preserve other module exports while only mocking the webhook signature verification.
29-39: LGTM!Clean helper function for creating webhook requests. The dummy signature is appropriate since
verifyWebhookSignatureis mocked.
41-54: LGTM!The cleanup function correctly respects FK constraints by deleting in the right order:
stripeEvents→orderItems→orders→productPrices→products.
56-264: Thorough test coverage for webhook PSP fields and idempotency.The test properly:
- Seeds all required data with correct FK relationships
- Verifies PSP fields (
pspChargeId,pspPaymentMethod,pspStatusReason,pspMetadata) are populated onpayment_intent.succeeded- Confirms idempotency by resubmitting the same event and verifying no duplicate
stripeEventsrecords- Uses
try/finallyto ensure cleanup runs regardless of test outcomeOne minor observation on line 234: the idempotency response check allows
< 500rather than strictly2xx. This is intentional flexibility since idempotent duplicate handling may return various success codes.frontend/lib/tests/checkout-no-payments.test.ts (4)
11-26: LGTM on the module mocks.The mocks correctly isolate the test environment by disabling payments and preventing
cookies()calls in tests. The use ofvi.importActualto spread actual implementations while overriding specific functions is the right approach.
213-329: Solid test coverage for no-payments success path.The test thoroughly validates:
- API response contract (status, success flag, order shape)
- DB state (paymentProvider, paymentStatus, inventoryStatus, status, currency, totalAmountMinor)
- Ledger entries (single reserve, no releases)
- Stock decrement and restoration via
restockOrderThe try/finally cleanup pattern ensures test artifacts are removed even on failure.
374-380: Verify idempotent return behavior when product is inactive.The second checkout call (lines 375-380) happens after the product is deactivated (lines 369-372). This works because idempotency returns the existing order without re-validating product availability. Worth noting this is intentional behavior for idempotent replays.
422-488: Good orphan cleanup test coverage.The test properly validates the orphan order lifecycle:
- Inserts an orphan with
inventoryStatus: 'none'and no inventory_moves- Runs
restockStaleNoPaymentOrderswitholderThanMinutes: 0- Verifies terminal state:
INVENTORY_FAILED,released,failed,STALE_ORPHANfrontend/app/api/shop/checkout/route.ts (7)
28-53: LGTM on business error classification.The
EXPECTED_BUSINESS_ERROR_CODESset and helper functions provide clean separation between expected business errors (logged as warnings) and unexpected errors (logged as errors). The fallbackinstanceofchecks handle cases where error objects might not have a.codeproperty.
136-147: BOM handling is good but consider additional normalization.The BOM removal is helpful for cross-platform compatibility. Consider also handling other common encoding issues like CRLF or zero-width characters if you've seen them in production.
241-253: Good defensive handling for already-failed orders.Returning a stable 409 for orders that already failed prevents clients from receiving inconsistent 500 errors on idempotent retries. The
CHECKOUT_FAILEDcode withorderIdin details enables client-side debugging.
266-285: Condition logic looks correct but could be simplified.The condition at line 268 checks
!['paid', 'failed'].includes(order.paymentStatus)which is correct given the DB CHECK constraint. The additionalorder.paymentIntentIdcheck ensures no-payment orders don't have Stripe artifacts.
415-446: Restocking on payment intent failure is appropriate.The pattern of attempting to restock on Stripe failure (lines 418-425) prevents inventory from being held indefinitely. The best-effort approach with error logging is correct since the primary error should be surfaced to the client.
489-500: MoneyValueError handling correctly returns 500.The
PRICE_DATA_ERRORcode and 500 status appropriately signal a server-side data integrity issue. IncludingproductId,field, andrawValueprovides good diagnostics without exposing sensitive data.
465-467: Theerror.detailshandling is correct, but consider stricter typing.The
detailsproperty is properly optional and correctly handled with the conditional spread operator inerrorResponse. All instantiations pass plain serializable objects. However, the typeRecord<string, unknown>allows non-serializable values (e.g., Error objects) to slip through, which could serialize unexpectedly withJSON.stringify(). Consider either:
- Adding runtime validation:
details && Object.values(details).some(v => !isSerializable(v))- Narrowing the type to
Record<string, string | number | boolean | null>to prevent non-serializable values by designfrontend/lib/services/orders.ts (11)
55-58:normVariantreturns empty string for falsy input.This is intentional for consistent key generation, but consider documenting that empty string is the canonical form for "no variant".
142-152: Sorting byproductIdensures deterministic reserve order.Sorting aggregated items by
productId(line 151) prevents potential deadlocks when multiple concurrent orders reserve the same products in different orders. Good defensive pattern.
154-189: Idempotency hash implementation is robust.The hash includes:
- Version number (
v: 1) for future schema changes- Currency and userId for full request identity
- Sorted, normalized items for deterministic hashing
Using SHA-256 is appropriate for this use case.
308-451:reconcileNoPaymentOrderhandles crash recovery well.The function correctly:
- Validates order state before reconciliation
- Sets
inventoryStatus: 'reserving'before attempting reserves- Rolls back with releases on failure
- Sets terminal failure state with appropriate codes
One note: the
neconditions at lines 379-381 prevent re-reconciliation of already-processed orders.
771-813: Order items upsert withonConflictDoUpdateis a good idempotency pattern.The upsert on the composite key
(orderId, productId, selectedSize, selectedColor)ensures that retried requests don't create duplicate line items. Theexcluded.*references correctly update to the latest values.
826-839: Sequential reserve loop could be optimized but is safer.Processing reserves sequentially (rather than in parallel) is safer for inventory consistency. The
applyReserveMovefunction handles idempotency viamove_key, so retries are safe.
991-1022: Lease claim mechanism is well-designed.The
tryClaimRestockLeasefunction:
- Uses atomic UPDATE with WHERE conditions
- Checks both unclaimed (
isNull) and expired (lt) claims- Generates unique
sweepRunIdfor traceability- Returns boolean indicating claim success
This prevents concurrent workers from double-processing the same order.
1119-1128: Guard against restocking paid Stripe orders without refund.The condition at line 1123 correctly prevents restocking paid Stripe orders unless
reason === 'refunded'. The!isNoPaymentcheck is necessary because no-payment orders havepaymentStatus: 'paid'by DB constraint but should be restockable for other reasons.
1149-1150: Sequential release loop is correct for idempotent releases.The sequential
forloop ensures releases happen one at a time. Given thatapplyReleaseMoveshould be idempotent (viamove_key), concurrent releases would be safe but sequential is clearer.
1329-1334: Refund validation correctly restricts to Stripe orders.The check at line 1330-1334 prevents refunds on no-payment orders, which is correct since there's no payment to refund.
1275-1313: Function is test-only and not used in production code.This function is only called in test files (
checkout-no-payments.test.ts) and never in production. The "test-only sweep; no batch claiming" comment is accurate and appropriate. The concern about adding claim logic for production concurrency is not applicable since this function has no production usage.Likely an incorrect or invalid review comment.
…d add debug-only logging for best-effort cleanup
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/lib/tests/checkout-no-payments.test.ts (1)
42-100: Verify template product availability and consider type safety.The function clones an existing product to satisfy NOT NULL constraints. While pragmatic, this creates a hidden dependency on the database state (at least one active product must exist).
The multiple
as anytype casts (lines 52, 67, 76, 89) bypass type safety. If the schema changes, these casts could mask issues.Consider:
- Documenting this dependency in test setup/README
- Using explicit type guards or defining a proper type for the insert payload instead of
as anyAlternatively, if test isolation is critical, you could seed a known template product in a global test setup hook.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/lib/services/orders.tsfrontend/lib/tests/checkout-no-payments.test.ts
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/lib/services/orders.ts (10)
frontend/db/schema/shop.ts (3)
orders(107-198)orderItems(200-260)inventoryMoves(333-362)frontend/db/index.ts (1)
db(39-39)frontend/lib/shop/cart-item-key.ts (1)
createCartItemKey(1-7)frontend/lib/shop/money.ts (2)
fromCents(44-46)toDbMoney(58-60)frontend/db/queries/shop/orders.ts (2)
OrderSummary(33-33)InsufficientStockError(8-8)frontend/lib/services/errors.ts (4)
OrderNotFoundError(23-29)OrderStateInvalidError(62-85)InsufficientStockError(15-21)InvalidPayloadError(35-41)frontend/lib/shop/payments.ts (1)
PaymentStatus(9-9)frontend/lib/services/inventory.ts (2)
applyReserveMove(49-116)applyReleaseMove(118-178)frontend/lib/logging.ts (1)
logError(1-14)frontend/lib/env/stripe.ts (1)
isPaymentsEnabled(51-53)
🔇 Additional comments (20)
frontend/lib/tests/checkout-no-payments.test.ts (8)
11-34: LGTM: Test isolation setup is well-structured.The mock setup appropriately isolates the test environment by:
- Silencing expected business warnings
- Forcing no-payments mode for the entire test file
- Avoiding cookies() access in tests by mocking getCurrentUser
102-145: LGTM: Best-effort cleanup is appropriately structured.The sequential cleanup order (deactivate → delete prices → delete product) is correct, and the DEBUG-only logging keeps test output clean while providing observability when needed.
147-165: LGTM: Dynamic import ensures mocks are applied.The dynamic import of the POST handler (line 152) ensures that module-level mocks are established before the route handler is loaded. This is a correct pattern for Vitest module mocking.
169-187: LGTM: Raw SQL query with appropriate type coercion.The manual type coercion handles potential null/undefined values from the raw SQL results. The ordering by
created_atensures deterministic verification of move sequences.
189-231: LGTM: Test cleanup helper with correct deletion order.The deletion sequence (inventory_moves → order_items → orders) respects foreign key constraints and uses raw SQL appropriately for tables not exported in the Drizzle schema.
234-350: LGTM: Comprehensive success path test with proper isolation.This test thoroughly validates:
- API response contract
- DB state consistency
- Inventory ledger correctness (single reserve, no duplicate)
- Stock accounting
- Restock functionality
The try-finally cleanup ensures test artifacts are removed even on failure.
352-441: LGTM: Idempotency contract is thoroughly tested.This test validates the strict idempotency requirements mentioned in the PR objectives:
- Same key + same payload → same order ID (no double-reserve)
- Same key + different payload → 409 conflict
The assertion at line 410 is particularly important for preventing inventory corruption under retries.
443-509: LGTM: Orphan cleanup path test validates stale order handling.This test correctly validates the orphan cleanup behavior described in the PR objectives, ensuring that no-payment orders with no inventory reservation are marked as terminal failures with appropriate metadata.
frontend/lib/services/orders.ts (12)
1-73: LGTM: Imports and type definitions support variant-aware checkout.The additions properly support the variant (size/color) tracking and idempotency features introduced in this PR.
118-140: LGTM: Variant-aware item merging with quantity limits.The merging logic correctly deduplicates items by productId + variant combination and enforces per-line quantity limits.
142-152: LGTM: Product-level aggregation for inventory reservation.Since stock is tracked at the product level (not per-variant), this aggregation correctly consolidates quantities across all variants of the same product.
154-189: LGTM: Stable idempotency hash with variant support.The hash includes all request-sensitive fields (currency, userId, items with variants) in a canonical sorted form. The version field (
v: 1) allows for future hash format evolution.
212-306: LGTM: Order parsing preserves variants and minor-unit pricing.The parsing logic correctly:
- Preserves variant fields (size/color) in item summaries
- Handles migration from legacy decimal to minor-unit pricing
- Validates data integrity with appropriate error types
308-450: LGTM: No-payment reconciliation with correct finality semantics.This function correctly implements the no-payment workflow described in the PR objectives:
- Uses
inventoryStatus='reserved'(notpaymentStatus) as the finality signal (lines 341-347)- Handles partial failures with inventory rollback (lines 413-449)
- Properly marks terminal failure states
The error handling on lines 424-428 uses
logErrorinstead ofconsole.error, aligning with the PR commit message.
609-691: LGTM: Idempotency enforcement with migration path.This function correctly:
- Enforces strict currency compatibility (lines 626-632)
- Handles legacy orders without stored hashes by deriving from persisted state (lines 634-646)
- Performs best-effort backfill for future checks (lines 648-667)
- Cleans up failed orders defensively (lines 676-690)
583-907: LGTM: Comprehensive order creation with idempotency and inventory integration.The refactored flow correctly implements the transactionless checkout described in the PR objectives:
- Currency derived from locale (line 594)
- Strict idempotency with hash verification (lines 609-691, 696)
- Minor-unit pricing throughout (lines 734-735, 797-798)
- Variant fields persisted in order_items (lines 793-794)
- Product-level inventory aggregation (lines 835-837)
- Defensive inventory rollback on failure (lines 873-883)
The
onConflictDoUpdateat lines 808-825 handles race conditions during idempotent retries, ensuring the order_items are updated consistently even if the insert races with an existing row.
1036-1194: LGTM: Restock-once semantics with lease-based concurrency control.This function implements the "restock-once" guarantee mentioned in the PR objectives:
- Checks if already released (lines 1062-1067) for idempotency
- Handles orphan orders (no inventory reserved) by marking them terminal (lines 1083-1130)
- Claims a lease before processing to prevent concurrent restocks (lines 1144-1153)
- Uses
WHERE stockRestored = falseto ensure finalization happens exactly once (lines 1173)The pattern correctly prevents double-restocking even under high concurrency.
1196-1285: LGTM: Batch restock sweep with lease-based work distribution.The sweep implementation correctly:
- Uses atomic claim-and-process pattern (lines 1243-1270)
- Re-validates claim gate at UPDATE time to prevent stealing (lines 1264-1267)
- Processes deterministically (oldest first) for fairness (line 1247)
- Passes
alreadyClaimed: trueto avoid redundant locking (line 1277)
1288-1325: LGTM: No-payment specific restock sweep.This function correctly targets the no-payment flow mentioned in the PR objectives:
- Filters by
paymentProvider='none'(line 1305)- Uses shorter timeout (10 min) appropriate for no-payment orders (line 1292)
- Relies on
restockOrder's internal lease mechanism for concurrency control
1327-1371: LGTM: Refund flow updated for provider-aware logic.The refund function correctly restricts refunds to Stripe orders (lines 1342-1346) and integrates with the new restock workflow.
2 participants
Description
This PR stabilizes the Shop checkout and payments flow under Neon HTTP (no
db.transaction) by moving consistency guarantees to:inventory_moves) for reserve/release,It also includes a hardening sweep across related P0 tasks discovered/validated during the transactionless migration (admin safety, public visibility rules, webhook contract, dedupe, restock-only-once, PSP fields, and order item snapshots).
Related Issue
Issue: #<issue_number>
Changes
Implemented transactionless checkout core (Neon HTTP compatible):
inventory_moves) with deterministic move keysHardened money + currency invariants:
total_amount_minor,*_minorsnapshots)PRICE_CONFIG_ERROR(400); corrupted money/data →PRICE_DATA_ERROR(500)Security + governance hardening:
ENABLE_ADMIN_API(prod-safe gate) with coverageGET /api/shop/orders/[id](IDOR / existence-hiding)Stripe reliability pack (webhook + state machine):
WEBHOOK_DISABLEDvsINVALID_SIGNATUREstripe_events(ON CONFLICT DO NOTHINGearly-exit)pspStatusReason+pspMetadataUPDATE … WHERE … RETURNING:stock_restored/restocked_at)order_itemssnapshots (post-checkout updates to products/prices do not mutate snapshots)Logging/test hygiene:
Database Changes (if applicable)
How Has This Been Tested?
Automated
npm run build(passes)npx vitest run(full suite passes; latest run: 16 test files / 39 tests)Key coverage added/validated (representative):
PRICE_CONFIG_ERROR)Manual smoke (PowerShell + Stripe CLI)
Accept-Language(en/uk) validating currency + minor totalsScreenshots (if applicable)
N/A (backend + API behavior changes)
Checklist
Before submitting
Reviewers
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.