(SP: 3) [Backend] Shop hardening + service decomposition: refunds (full-only), guarded paymentStatus, checkout variant & SALE validation, structured logging/env docs#125
Conversation
… + services/orders modules
…empty refunds) to prevent webhook regression
…th 400 INVALID_VARIANT + no-side-effects tests
…guard; add legacy coverage + tripwire; silence webhook contract stderr
liudmylasovetovs
requested review from
AM1007 and
ViktorSvertoka
as code owners
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
✅ Deploy Preview for develop-devlovers ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
📝 WalkthroughWalkthroughRefactors product and order services into modular modules, adds a level-based structured logging system, implements idempotent checkout with guarded payment-state transitions and inventory reservation, hardens Stripe webhook refund handling with retry semantics, and expands tests for payment state and variant validation. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant CheckoutAPI as Checkout Route
participant Idempotency as Idempotency Check
participant ProductSvc as Product/Price Service
participant Inventory as Inventory Service
participant OrderDB as Orders DB
participant PaymentState as guardedPaymentStatusUpdate
participant PSP as Payment Provider (Stripe)
Client->>CheckoutAPI: POST /api/checkout (items, idempotencyKey)
CheckoutAPI->>Idempotency: compute/hash idempotency & lookup
alt existing order found
Idempotency-->>CheckoutAPI: return existing order
else new order
CheckoutAPI->>ProductSvc: validate variants, fetch prices
ProductSvc-->>CheckoutAPI: priced items + totals
CheckoutAPI->>OrderDB: insert skeleton order (CREATED)
OrderDB-->>CheckoutAPI: orderId
CheckoutAPI->>Inventory: applyReserveMove (per product)
Inventory-->>CheckoutAPI: reserved (or fail)
CheckoutAPI->>PaymentState: guardedPaymentStatusUpdate -> INVENTORY_RESERVED
PaymentState-->>OrderDB: conditional update (extraWhere guards)
alt payments enabled
CheckoutAPI->>PSP: create payment intent
PSP-->>CheckoutAPI: paymentIntentId
CheckoutAPI->>PaymentState: guardedPaymentStatusUpdate -> requires_payment
else no payments
CheckoutAPI->>PaymentState: guardedPaymentStatusUpdate -> paid
end
CheckoutAPI-->>Client: CheckoutResult (order summary)
end
sequenceDiagram
participant Stripe
participant Webhook as Webhook Handler
participant EventDB as Stripe Events DB
participant OrderDB as Orders DB
participant PaymentState as guardedPaymentStatusUpdate
participant RefundCalc as Refund fullness logic
Stripe->>Webhook: POST event (payment_succeeded/failed/refund)
Webhook->>EventDB: check processedAt / dedupe
alt duplicate
EventDB-->>Webhook: already processed -> ack
else new event
Webhook->>OrderDB: load order by reference
alt refund event
Webhook->>RefundCalc: determine cumulative refunded amount (amount_refunded or sum refunds.list)
RefundCalc-->>Webhook: refundAmount or INSUFFICIENT_DATA
alt insufficient data
Webhook-->>Stripe: 500 REFUND_FULLNESS_UNDETERMINED (retry later)
else amount known
Webhook->>PaymentState: guardedPaymentStatusUpdate (refund path)
PaymentState-->>OrderDB: conditional update result
Webhook->>EventDB: mark processedAt
Webhook-->>Stripe: 200 OK
end
else payment success/failure
Webhook->>PaymentState: guardedPaymentStatusUpdate (success/failure)
PaymentState-->>OrderDB: update result
Webhook->>EventDB: mark processedAt
Webhook-->>Stripe: 200 OK
end
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 12
🤖 Fix all issues with AI agents
In @frontend/lib/services/orders/checkout.ts:
- Around line 740-790: The catch blocks in createOrderWithItems and
reconcileNoPaymentOrder swallow per-item release failures but unconditionally
set stockRestored=true and restockedAt, which can hide partial failures; change
the flow to track whether any applyReleaseMove calls failed (e.g., a boolean
releaseFailed or failedItems list when iterating itemsToReserve), log details,
and only set stockRestored=true and restockedAt when all releases succeeded; if
any release failed, update inventoryStatus (e.g., to 'release_failed' or keep
'release_pending') and record a failure indicator/message
(failureCode/failureMessage or a new column) so janitors can retry, and ensure
the final DB update uses that flag (references: createOrderWithItems,
reconcileNoPaymentOrder, itemsToReserve, applyReleaseMove, stockRestored,
restockedAt, inventoryStatus).
In @frontend/lib/services/orders/refund.ts:
- Around line 12-14: refundOrder currently declares it returns
Promise<OrderSummaryWithMinor> but simply returns the result of getOrderById
which is typed as Promise<OrderDetail>/Promise<OrderSummary>, so the types
mismatch; fix by either updating getOrderById's return type to
Promise<OrderSummaryWithMinor> if it truly returns totalAmountMinor, or modify
refundOrder to call getOrderById then build and return an object that includes
totalAmountMinor (e.g., compute or map the value) so the returned value conforms
to OrderSummaryWithMinor; locate and update the return type on getOrderById or
add the mapping/field construction inside refundOrder accordingly.
In @frontend/lib/services/orders/restock.ts:
- Around line 106-179: The orphan-handling logic misclassifies canceled orders
and skips terminalizing non-none-provider orphans; update the branches that
handle !reservedMoves.length so that when reason === 'canceled' you set
orders.status to 'CANCELED' (not 'INVENTORY_FAILED') and still perform the same
DB update/restock marker and guardedPaymentStatusUpdate call (use the same
extraWhere eq(orders.restockedAt, now) tie) as in the existing canceled-reserved
path; additionally, for non-none providers ensure non-stale reasons like
'canceled' are handled (not just 'stale') by expanding the second branch that
currently checks reason === 'stale' to also handle 'canceled' and apply the same
status and payment transition logic; refer to the update(...) .set({...}) block,
the isNoPayment check, the second if (reason === 'stale') branch, and the
guardedPaymentStatusUpdate(...) call to make these changes.
- Around line 84-255: Early-return when order.stockRestored/order.restockedAt is
set prevents completing payment transitions and split finalization updates allow
updatedAt to move backwards; fix by (1) replacing the initial "if
(order.inventoryStatus==='released' || order.stockRestored || order.restockedAt
!== null) return" with logic that, if stockRestored/restockedAt is already set,
does NOT return immediately but calls guardedPaymentStatusUpdate with
extraWhere: eq(orders.restockedAt, order.restockedAt) to let any missing payment
transition finish, then returns; (2) make the finalize step atomic by merging
the release_pending->finalize sequence into a single conditional update that
sets inventoryStatus='released', stockRestored=true, restockedAt=finalizedAt,
updatedAt=finalizedAt and status (INVENTORY_FAILED/CANCELED) in one DB update
(use finalizedAt for all timestamps) so a crash cannot leave
inventoryStatus='release_pending' or regress updatedAt; and (3) keep existing
claims (tryClaimRestockLease) and applyReleaseMove usage but ensure all updates
use the same finalizedAt timestamp when binding guardedPaymentStatusUpdate
(extraWhere: eq(orders.restockedAt, finalizedAt)).
In @frontend/lib/services/orders/summary.ts:
- Around line 50-144: The code in parseOrderSummary treats item.selectedSize and
item.selectedColor as possibly null/undefined (uses "?? ''"), but the
OrderItemForSummary type declares them as string, causing a type mismatch;
update the OrderItemForSummary definition so selectedSize and selectedColor are
typed as string | null | undefined (or optional fields) to reflect the DB
nullable columns, or alternatively change parseOrderSummary to not assume
nullability (remove the "?? ''" and ensure callers always provide strings).
Modify the OrderItemForSummary type and any affected callers/tests accordingly
and keep parseOrderSummary's normalizedItems logic intact (references:
OrderItemForSummary, parseOrderSummary, selectedSize, selectedColor).
In @frontend/lib/services/products/mutations/create.ts:
- Around line 111-123: The catch block currently only rolls back a created DB
product (createdProductId) but doesn't remove an uploaded Cloudinary image, so
add cleanup: import or implement deleteProductImage from your Cloudinary module
and, inside the catch, if you have an image public id (e.g., imagePublicId
variable or product.imagePublicId available in this scope) call await
deleteProductImage(imagePublicId) in a try/catch and log failures with logError
before/alongside the existing db.delete(products). Ensure deleteProductImage is
imported at the top and that all cleanup errors are caught and logged without
masking the original thrown error which is rethrown at the end.
In @frontend/lib/services/products/mutations/update.ts:
- Around line 206-209: The catch block currently maps any Postgres
unique-violation (code '23505') to SlugConflictError; change the conditional to
also verify the violated constraint name (check (error as { code?: string;
constraint?: string }).constraint) matches your products.slug uniqueness
constraint (or contains 'slug') before throwing SlugConflictError; otherwise
rethrow the original error. Update the code around the existing check that
references '23505' and SlugConflictError so it only throws SlugConflictError
when both code === '23505' and constraint === '<your_slug_constraint_name>' (or
constraint.includes('slug')).
In @frontend/lib/tests/payment-status-tripwire.test.ts:
- Around line 443-464: The test assumes REPO_ROOT uses the frontend working dir
but currently uses process.cwd(), causing path mismatches between walk() output
(e.g., "frontend/lib/...") and WRITER_RULES keys (e.g., "lib/services/..."); fix
by normalizing paths in getAllowed/walk or adjusting REPO_ROOT: set REPO_ROOT =
path.join(process.cwd(), 'frontend') (or alternatively prepend "frontend/" to
every key in WRITER_RULES or strip a leading "frontend/" from rel before lookup)
so getAllowed(rel) receives matching relative paths and the allowlist logic
works as intended.
🧹 Nitpick comments (20)
frontend/lib/tests/stripe-webhook-contract.test.ts (1)
46-60: LGTM: Test correctly validates WEBHOOK_DISABLED scenario.The test properly sets environment variables before importing the route module, and the empty string for
STRIPE_WEBHOOK_SECRETcorrectly triggers the disabled webhook path.Minor style note: The inline comment at line 51 is in Ukrainian ("ключове: після імпорту і саме порожній рядок"). Consider using English for consistency with the rest of the codebase.
frontend/lib/tests/stripe-webhook-refund-full.test.ts (1)
495-586: LGTM: Comprehensive regression test for refund fullness edge case.This test correctly validates the safe failure mode when refund fullness cannot be determined:
- Returns 500 with explicit error code (enables Stripe retry)
- Leaves
processedAtNULL (prevents deduplication)- Preserves order state (no corruption)
- Ensures no side effects (no restock)
- Validates observability (warning logged with reason)
The test coverage aligns with the PR objective to handle this webhook edge case gracefully.
Minor suggestion: Consider mock consistency
The test directly spies on
console.warn(line 521), but the broader PR introduces structured logging facades. IflogWarnis mocked elsewhere in the test suite, consider whether tests should validate the logging facade rather than console directly for consistency.This is purely optional and doesn't affect correctness.
frontend/lib/services/products/mutations/delete.ts (2)
10-18: Consider using a custom error class for PRODUCT_NOT_FOUND.The function throws a plain
Error('PRODUCT_NOT_FOUND'), but the PR introduces custom error types (e.g.,InvalidVariantError). For consistency and better error handling, consider defining aProductNotFoundErrorclass.Suggested approach
export class ProductNotFoundError extends Error { code = 'PRODUCT_NOT_FOUND' as const; constructor(id: string) { super(`Product not found: ${id}`); this.name = 'ProductNotFoundError'; } } // Then throw: throw new ProductNotFoundError(id);This enables type-safe error handling and better error messages.
22-28: LGTM: Best-effort image cleanup with structured logging.The function correctly attempts to clean up the associated Cloudinary image after deletion. Logging failures without throwing ensures the product deletion succeeds even if the external cleanup fails.
Note: This approach can lead to orphaned images in Cloudinary over time if cleanup consistently fails (e.g., network issues, credential problems). Consider implementing a periodic cleanup job to scan for orphaned images or adding this to your operational monitoring. This is not a blocker for the current implementation.
frontend/app/api/shop/internal/orders/restock-stale/route.ts (1)
1-1: Consider removing the file path comment.The comment
//app\api\shop\internal\orders\restock-stale\route.tson line 1 appears to be a leftover from development. It adds maintenance overhead as the path would need updating if the file moves, and modern IDEs already display the file path.🧹 Suggested removal
-//app\api\shop\internal\orders\restock-stale\route.ts import { NextRequest, NextResponse } from 'next/server';frontend/lib/tests/payment-state-machine.helper.test.ts (1)
104-106: Redundant guard after expect assertion.The
if (res.applied) throwon Line 105 is unnecessary since theexpect(res.applied).toBe(false)on Line 104 would already fail the test. Consider removing for cleaner test code.Suggested simplification
expect(res.applied).toBe(false); - if (res.applied) throw new Error('expected not applied'); + // TypeScript narrowing: res is now { applied: false, ... } expect(res.reason).toBe('INVALID_TRANSITION');frontend/lib/services/products/mutations/toggle.ts (2)
17-19: Consider using a custom error class for consistency.The function throws a plain
Errorwith a string identifier. For consistency with the structured error handling introduced elsewhere in this PR (e.g.,InvalidVariantError), consider defining and using a custom error class such asProductNotFoundError.♻️ Example: Custom error class
In
frontend/lib/services/errors.ts:export class ProductNotFoundError extends Error { constructor(productId: string) { super(`Product not found: ${productId}`); this.name = 'ProductNotFoundError'; } }Then use it here:
+import { ProductNotFoundError } from '@/lib/services/errors'; + export async function toggleProductStatus(id: string): Promise<DbProduct> { const [current] = await db .select() .from(products) .where(eq(products.id, id)) .limit(1); if (!current) { - throw new Error('PRODUCT_NOT_FOUND'); + throw new ProductNotFoundError(id); }
10-27: Optional: Optimize to a single query.The function performs two queries: one to check existence and another to update. You could reduce this to a single
UPDATE...RETURNINGquery and check if the result array is empty.♻️ Single-query alternative
export async function toggleProductStatus(id: string): Promise<DbProduct> { - const [current] = await db - .select() - .from(products) - .where(eq(products.id, id)) - .limit(1); - - if (!current) { - throw new Error('PRODUCT_NOT_FOUND'); - } - - const [updated] = await db + const rows = await db + .select({ isActive: products.isActive }) + .from(products) + .where(eq(products.id, id)) + .limit(1); + + if (!rows.length) { + throw new Error('PRODUCT_NOT_FOUND'); + } + + const [updated] = await db .update(products) - .set({ isActive: !current.isActive }) + .set({ isActive: !rows[0].isActive }) .where(eq(products.id, id)) .returning(); return mapRowToProduct(updated); }Alternatively, rely on the
UPDATE...RETURNINGalone:export async function toggleProductStatus(id: string): Promise<DbProduct> { + // Fetch current isActive value first (lightweight) + const [current] = await db + .select({ isActive: products.isActive }) + .from(products) + .where(eq(products.id, id)) + .limit(1); + + if (!current) { + throw new Error('PRODUCT_NOT_FOUND'); + } + const [updated] = await db .update(products) - .set({ isActive: !current.isActive }) + .set({ isActive: !current.isActive }) .where(eq(products.id, id)) .returning(); + if (!updated) { + throw new Error('PRODUCT_NOT_FOUND'); + } + return mapRowToProduct(updated); }(The second approach avoids a race if another process deletes the row between queries.)
frontend/lib/services/orders/refund.ts (1)
36-36: Non-English comment should be translated.The inline comment
замість order.paymentProvideris in Ukrainian. For consistency and maintainability, translate it to English (e.g., "instead of order.paymentProvider").Suggested fix
- paymentProvider: provider, // <-- замість order.paymentProvider + paymentProvider: provider, // resolved provider, not raw order.paymentProviderfrontend/lib/services/orders/payment-intent.ts (1)
20-25: Consider reducing double database fetch.The order is fetched at lines 20-24 for validation, and again at lines 76-80 after the update. Since
guardedPaymentStatusUpdateconfirms the update succeeded, consider using a.returning()clause in the guard's update or accepting the slight staleness risk to avoid the extra round-trip.Also applies to: 76-85
frontend/lib/services/products/slug.ts (1)
22-44: Potential race condition in slug uniqueness check.Between the
selectcheck and the caller'sinsert, another transaction could insert the same slug. The caller should handleSlugConflictErrorfrom the insert's conflict detection (e.g.,onConflictDoNothing) as the authoritative check. The loop here reduces likelihood but doesn't eliminate the race.Consider documenting this behavior or adding a retry at the caller level for the rare case where the insert still conflicts.
frontend/lib/services/products/types.ts (1)
22-22: Duplicate type definition:DbClient.
DbClientis also defined infrontend/lib/services/orders/_shared.ts(per relevant snippets). Consider consolidating into a shared location (e.g.,@/lib/types/db.ts) to maintain a single source of truth.frontend/lib/services/products/mutations/create.ts (2)
23-26: Excessive(input as any)casts reduce type safety.The function accesses numerous properties via
(input as any), which bypasses TypeScript's type checking. Consider strengthening theProductInputtype definition to include all expected fields (slug, title, description, image, badge, category, type, colors, sizes, isActive, isFeatured, stock, sku) to catch type errors at compile time.Also applies to: 45-77
89-108: Consider wrapping in a transaction.The
productsandproductPricesinserts are not in a transaction. While the cleanup logic handles failures, a brief window exists where a product could exist without prices. For admin-only operations this is likely acceptable, but a transaction would provide stronger consistency guarantees.frontend/lib/services/products/admin/queries.ts (1)
19-21: Create aProductNotFoundErrorclass for consistency with the codebase error handling pattern.The codebase uses typed error classes for error handling (e.g.,
OrderNotFoundError,InvalidVariantError,PriceConfigError). Throwing a plainError('PRODUCT_NOT_FOUND')is inconsistent with this established pattern. Create aProductNotFoundErrorinfrontend/lib/services/errors.tsand use it here.Suggested approach
First, add to
frontend/lib/services/errors.ts:export class ProductNotFoundError extends Error { code = 'PRODUCT_NOT_FOUND' as const; constructor(message = 'Product not found.') { super(message); this.name = 'ProductNotFoundError'; } }Then, in
frontend/lib/services/products/admin/queries.ts:+import { ProductNotFoundError } from '../../errors'; + if (!row) { - throw new Error('PRODUCT_NOT_FOUND'); + throw new ProductNotFoundError('Product not found'); }frontend/lib/tests/checkout-no-payments.test.ts (1)
459-678: Variant rejection tests are strong; consider small cleanup for readability/stability.
- Replace mixed-language comments with English for consistency.
- Consider extracting the repeated “count inventory_moves for productId” into a helper (reduces duplication across tests).
frontend/lib/services/products/prices.ts (1)
75-161: Normalization supports legacy shapes well; consider tightening accepted numeric formats if needed.If you don’t want to accept inputs like
"1e3", replaceNumber(trimmed)parsing with a stricter decimal regex (optional, product decision).frontend/lib/shop/data.ts (1)
75-90: Consider falling back to base “unavailable” data when mapping fails (instead ofnot_found).Right now a product can exist (base row present) but appear as
not_foundif the currency-specific row fails validation.Possible adjustment
const dbProduct = await getPublicProductBySlug(slug, currency); if (dbProduct) { const mapped = mapToShopProduct(dbProduct); - if (mapped) return { kind: 'available', product: mapped }; - return { kind: 'not_found' }; + if (mapped) return { kind: 'available', product: mapped }; + // fall through to base lookup (unavailable) if mapping failed }frontend/lib/services/orders/_shared.ts (1)
44-52:requireTotalCentsshould probably also enforce “non-negative int” (not just finite number).
This is an internal invariant helper; tightening it will prevent subtle money bugs.frontend/lib/services/orders/checkout.ts (1)
265-310:parseVariantListsilently treats unknown shapes as “no variants” (returns[]).
If you want misconfigurations to fail closed (esp. admin-driven variants), consider throwing/logging here (or at leastlogWarn) to avoid accepting invalid selections due to bad DB shape.
📜 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 (43)
frontend/app/[locale]/shop/admin/products/_components/product-form.tsxfrontend/app/[locale]/shop/admin/products/page.tsxfrontend/app/[locale]/shop/checkout/payment/StripePaymentClient.tsxfrontend/app/[locale]/shop/checkout/payment/[orderId]/page.tsxfrontend/app/api/shop/cart/rehydrate/route.tsfrontend/app/api/shop/checkout/route.tsfrontend/app/api/shop/internal/orders/restock-stale/route.tsfrontend/app/api/shop/webhooks/stripe/route.tsfrontend/db/index.tsfrontend/lib/cart.tsfrontend/lib/cloudinary.tsfrontend/lib/logging.tsfrontend/lib/services/errors.tsfrontend/lib/services/orders.tsfrontend/lib/services/orders/_shared.tsfrontend/lib/services/orders/checkout.tsfrontend/lib/services/orders/payment-intent.tsfrontend/lib/services/orders/payment-state.tsfrontend/lib/services/orders/refund.tsfrontend/lib/services/orders/restock.tsfrontend/lib/services/orders/summary.tsfrontend/lib/services/orders/sweeps.tsfrontend/lib/services/products.tsfrontend/lib/services/products/admin/queries.tsfrontend/lib/services/products/cart/rehydrate.tsfrontend/lib/services/products/mapping.tsfrontend/lib/services/products/mutations/create.tsfrontend/lib/services/products/mutations/delete.tsfrontend/lib/services/products/mutations/toggle.tsfrontend/lib/services/products/mutations/update.tsfrontend/lib/services/products/prices.tsfrontend/lib/services/products/slug.tsfrontend/lib/services/products/types.tsfrontend/lib/shop/data.tsfrontend/lib/tests/checkout-no-payments.test.tsfrontend/lib/tests/order-items-variants.test.tsfrontend/lib/tests/payment-state-legacy-writers.test.tsfrontend/lib/tests/payment-state-machine.helper.test.tsfrontend/lib/tests/payment-status-tripwire.test.tsfrontend/lib/tests/stripe-webhook-contract.test.tsfrontend/lib/tests/stripe-webhook-paid-status-repair.test.tsfrontend/lib/tests/stripe-webhook-refund-full.test.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/db/index.tsfrontend/project-structure.txt
🧬 Code graph analysis (15)
frontend/lib/services/products/mutations/toggle.ts (2)
frontend/lib/services/products.ts (1)
toggleProductStatus(7-7)frontend/lib/services/products/mapping.ts (1)
mapRowToProduct(5-21)
frontend/lib/services/orders/refund.ts (6)
frontend/lib/types/shop.ts (1)
OrderSummaryWithMinor(32-35)frontend/lib/services/errors.ts (2)
OrderNotFoundError(26-32)InvalidPayloadError(37-43)frontend/lib/services/orders/_shared.ts (1)
resolvePaymentProvider(29-42)frontend/lib/services/orders/payment-state.ts (1)
guardedPaymentStatusUpdate(122-263)frontend/lib/services/orders/restock.ts (1)
restockOrder(56-255)frontend/lib/services/orders/summary.ts (1)
getOrderById(154-164)
frontend/lib/services/products/admin/queries.ts (3)
frontend/lib/services/products/mapping.ts (1)
mapRowToProduct(5-21)frontend/lib/services/products/types.ts (2)
AdminProductPriceRow(3-11)AdminProductsFilter(13-17)frontend/lib/services/products/prices.ts (1)
assertMoneyMinorInt(18-37)
frontend/lib/services/products/mutations/create.ts (5)
frontend/lib/services/products/slug.ts (1)
normalizeSlug(47-57)frontend/lib/services/products/prices.ts (3)
normalizePricesFromInput(75-161)enforceSaleBadgeRequiresOriginal(209-227)requireUsd(163-169)frontend/lib/services/errors.ts (2)
InvalidPayloadError(37-43)SlugConflictError(85-91)frontend/lib/shop/money.ts (1)
toDbMoney(58-60)frontend/lib/services/products/mapping.ts (1)
mapRowToProduct(5-21)
frontend/lib/services/products/types.ts (2)
frontend/lib/services/products.ts (2)
AdminProductPriceRow(18-18)AdminProductsFilter(18-18)frontend/lib/services/orders/_shared.ts (1)
DbClient(25-25)
frontend/app/api/shop/webhooks/stripe/route.ts (2)
frontend/lib/services/orders/payment-state.ts (1)
guardedPaymentStatusUpdate(122-263)frontend/db/schema/shop.ts (1)
orders(107-198)
frontend/lib/services/orders/summary.ts (4)
frontend/lib/services/orders/_shared.ts (4)
OrderRow(13-13)requireMinor(136-146)resolvePaymentProvider(29-42)DbClient(25-25)frontend/lib/types/shop.ts (1)
OrderSummaryWithMinor(32-35)frontend/lib/shop/money.ts (2)
fromDbMoney(51-56)fromCents(44-46)frontend/lib/services/errors.ts (2)
OrderStateInvalidError(93-116)OrderNotFoundError(26-32)
frontend/app/api/shop/checkout/route.ts (1)
frontend/lib/services/errors.ts (1)
InvalidVariantError(45-69)
frontend/lib/services/products/prices.ts (4)
frontend/lib/services/errors.ts (1)
InvalidPayloadError(37-43)frontend/lib/shop/money.ts (1)
toCents(38-42)frontend/lib/services/products/types.ts (1)
NormalizedPriceRow(24-28)frontend/lib/shop/currency.ts (1)
currencyValues(1-1)
frontend/lib/tests/payment-state-legacy-writers.test.ts (1)
frontend/lib/services/orders/payment-state.ts (1)
guardedPaymentStatusUpdate(122-263)
frontend/lib/services/orders/checkout.ts (4)
frontend/lib/services/orders/_shared.ts (4)
resolvePaymentProvider(29-42)aggregateReserveByProductId(78-88)normVariant(20-23)CheckoutItemWithVariant(15-18)frontend/lib/services/orders/summary.ts (2)
getOrderById(154-164)getOrderByIdempotencyKey(173-191)frontend/lib/services/inventory.ts (2)
applyReserveMove(49-116)applyReleaseMove(118-178)frontend/lib/services/orders/payment-state.ts (1)
guardedPaymentStatusUpdate(122-263)
frontend/lib/services/orders/sweeps.ts (1)
frontend/lib/services/orders/restock.ts (1)
restockOrder(56-255)
frontend/lib/tests/stripe-webhook-contract.test.ts (1)
frontend/app/api/shop/webhooks/stripe/route.ts (1)
POST(145-963)
frontend/lib/services/orders/_shared.ts (4)
frontend/lib/types/shop.ts (2)
CheckoutItem(24-24)OrderSummaryWithMinor(32-35)frontend/lib/shop/cart-item-key.ts (1)
createCartItemKey(1-7)frontend/lib/validation/shop.ts (1)
MAX_QUANTITY_PER_LINE(17-17)frontend/lib/services/errors.ts (2)
InvalidPayloadError(37-43)OrderStateInvalidError(93-116)
frontend/lib/tests/stripe-webhook-paid-status-repair.test.ts (1)
frontend/db/schema/shop.ts (2)
stripeEvents(262-276)orders(107-198)
🔇 Additional comments (82)
frontend/app/[locale]/shop/checkout/payment/StripePaymentClient.tsx (2)
25-25: LGTM: Structured logging import added.The import aligns with the centralized logging facade introduced in this PR.
114-117: LGTM: Structured error logging enhances observability.Replacing
console.errorwithlogErrorprovides structured logging with valuable context (orderId) for debugging payment confirmation failures, while preserving user-facing error handling.frontend/lib/tests/stripe-webhook-contract.test.ts (3)
3-13: LGTM: Test logging mock improves test hygiene.Silencing
logErrorin contract tests prevents expected error noise while preserving test assertions. The comment clearly explains the intent.
15-15: LGTM: Console error spy prevents test noise.The spy correctly suppresses expected
console.erroroutput in negative test scenarios without affecting test logic.Also applies to: 36-37
42-43: LGTM: Proper spy cleanup.The spy is correctly restored and nulled in
afterEach.frontend/lib/tests/stripe-webhook-paid-status-repair.test.ts (3)
75-114: LGTM: Robust cleanup with nullable parameters.The cleanup function correctly handles nullable IDs and guards deletions, preventing errors when partial cleanup is needed. The try-catch blocks ensure cleanup failures don't break test execution.
124-124: LGTM: Cleanup trigger correctly broadened.Changing from
&&to||ensures cleanup runs when either ID is present, preventing resource leaks.
132-186: LGTM: Test formatting standardized.The tests are reformatted to single-line declarations with inline timeouts, consistent with other test files in the PR. No functional changes.
frontend/lib/tests/payment-status-tripwire.test.ts (5)
18-53: Configuration and allowlist look well-structured.The allowlist mechanism with per-file, per-operation granularity is a good approach. The
WRITER_RULESmap clearly documents which files are permitted to perform which operations onorders.paymentStatus.One observation: the
norm()function only handles backslash normalization. If paths inWRITER_RULESneed to match against different path separators or casing on various platforms, consider this when extending.
55-71: LGTM!The recursive
walkfunction correctly traverses directories, excludes unwanted folders, and filters for TypeScript files. Synchronous I/O is acceptable in test contexts.
79-199: Balanced brace extraction is well-implemented.The function correctly handles:
- String literals (single, double, template)
- Comments (line and block)
- Template expression nesting (
${...})- Escape sequences
The stack-based approach for mode tracking is appropriate. The comment at line 12 clearly states this is a scoped scan, not an AST parser, which sets proper expectations.
205-385: Top-level key detection handles multiple formats correctly.The function properly detects
paymentStatusas:
- Identifier key:
paymentStatus: ...- Shorthand:
paymentStatus,- Quoted key:
'paymentStatus': ...or"paymentStatus": ...- Computed string literal:
['paymentStatus']: ...The nesting counter ensures only top-level keys are flagged, avoiding false positives from nested objects.
387-441: Writer detection logic is sound.The approach of:
- Finding
.set({/.values({call sites- Looking back to verify
orderstable context- Checking allowlist before flagging
- Extracting and inspecting the object literal
is effective for this tripwire purpose. The 2000-character lookback should handle typical chained method calls.
frontend/lib/services/errors.ts (2)
45-69:InvalidVariantErroris well-designed.The error class includes all relevant context for debugging variant validation failures:
productId: identifies which productfield: specifies whether size or color failedvalue: the invalid value providedallowed: list of valid optionsThis enables informative 400 responses with actionable details for API consumers.
8-15: LGTM!Formatting changes to constructor signatures improve readability with no behavioral changes.
frontend/lib/logging.ts (5)
1-24: Log level configuration is well-implemented.The level resolution logic:
- Checks
LOG_LEVELenv var first- Falls back to
infoin production,debugin development- Validates the level value and defaults appropriately
The weight-based comparison in
shouldLogis a clean approach for level filtering.
26-46: Error shaping is robust.The function handles various error types gracefully:
- Excludes stack traces in production (security/privacy)
- Handles non-Error objects with JSON.stringify fallback
- Catches JSON.stringify failures (e.g., circular references)
48-58: Sensitive key redaction is a good security practice.The regex covers common sensitive patterns. For future consideration, you might extend to include patterns like
credential,session, orprivate_key, but the current set is a solid baseline.
60-82: Emit function produces well-structured log output.The JSON payload format with
ts,level,msg,meta, anderrfields is ideal for log aggregation systems. Output routing to stderr for errors/warnings and stdout for debug/info follows standard conventions.
87-105: Public API is clean and backward-compatible.The
logErrorsignature maintains backward compatibility while adding optionalmetasupport. The newlogWarn,logInfo, andlogDebugfunctions provide a consistent interface across all log levels.frontend/lib/cart.ts (3)
14-14: LGTM!Import added for the new structured logging utility.
89-93: Structured logging improves observability.Replacing
console.warnwithlogWarnenables consistent log formatting and level-based filtering. The error message extraction pattern is correct.
102-106: Consistent application of structured logging.Same pattern applied for cart save failures. Both error handlers now use structured logging while maintaining the graceful failure behavior.
frontend/db/index.ts (2)
4-4: LGTM!Import added for structured logging utility.
17-19: Structured warning with context metadata.The
logWarncall includes the deployment context in metadata, which aids debugging when the preview URL fallback occurs. Good improvement over plain console.warn.frontend/app/[locale]/shop/checkout/payment/[orderId]/page.tsx (2)
11-11: LGTM!Import added for structured error logging.
172-180: Excellent error context for payment debugging.The structured log includes:
orderId: identifies the affected orderexistingPi: shows if this was a retrieve vs create pathphase: pinpoints which operation failed (retrieve/create/setOrderPaymentIntent)This metadata will be invaluable for diagnosing payment initialization failures in production.
frontend/app/api/shop/internal/orders/restock-stale/route.ts (2)
13-13: LGTM!Import added for structured error logging.
425-431: Good correlation metadata for job failures.Including
runIdin the error log enables tracing failures back to specific janitor runs. This is particularly useful for debugging when the endpoint is called by scheduled tasks or external systems.frontend/lib/cloudinary.ts (1)
52-56: Good migration to structured logging.The addition of contextual metadata (
folder,hasResult) will aid debugging upload failures. The error message remains user-friendly while the structured log captures diagnostic details.frontend/app/api/shop/cart/rehydrate/route.ts (2)
19-28: LGTM - Proper quantity normalization.The normalization logic correctly handles string-to-number conversion for quantities, with appropriate guards for empty strings and non-object items.
60-61: Good structured error logging.Replacing
console.errorwithlogErroraligns with the PR's logging standardization objective.frontend/project-structure.txt (1)
333-356: Well-organized service decomposition.The modular structure under
lib/services/ordersandlib/services/productsfollows a clear separation of concerns, with distinct modules for checkout, payment state, refunds, restock, and product mutations.frontend/app/api/shop/webhooks/stripe/route.ts (6)
12-28: Good structured warning helper for refund diagnostics.The
warnRefundFullnessUndeterminedhelper provides comprehensive diagnostic context for debugging refund edge cases, which is valuable given the complexity of the refund fullness determination logic.
437-470: Well-structured guarded payment transition for success.The
guardedPaymentStatusUpdateintegration withextraWhereconditions effectively prevents unnecessary state churn when the order is already consistent. The redundant safety gates (Lines 467-468) are reasonable defensive programming.
490-498: Good addition of refunded status guard.Adding the early return for
refundedstatus prevents invalid transitions and maintains idempotency for out-of-order webhook delivery.
640-706: Robust refund fullness calculation with fail-fast behavior.The fallback from
amount_refundedtorefunds.listsum, combined with explicitREFUND_FULLNESS_UNDETERMINEDerrors when data is insufficient, ensures eventual consistency via Stripe retries rather than silently treating missing data as zero.
923-929: Smart handling of ALREADY_IN_STATE for restocking.Allowing restock when the transition was already applied (
ALREADY_IN_STATE) handles idempotent webhook redelivery gracefully—ensuring stock is restored even if the payment status update was a no-op.
949-958: Correct retry signaling for undetermined refund fullness.Returning 500 without calling
ack()ensuresprocessedAtremains NULL, triggering Stripe retries until the charge data is fully propagated. This is the correct pattern for eventual consistency.frontend/lib/services/orders/payment-state.ts (5)
16-34: Well-designed provider-specific transition matrices.The separation between Stripe and no-payments transition rules is clear. The comment on Line 21-22 explaining why
refundedaccepts broader source states (webhook ordering/retries) is helpful for maintainability.
122-152: Good early rejection for invalid no-payments targets.Hard-rejecting
pending,requires_payment, andrefundedfor provider'none'before touching the DB is efficient and prevents unnecessary queries.
185-202: Consider the TOCTOU gap between update and diagnosis.The atomic
UPDATE ... WHEREcorrectly handles concurrent modifications by failing if conditions aren't met. The subsequentgetCurrentStatefor diagnosis is only for logging/reporting and doesn't affect correctness. The pattern is sound.
228-235: ALREADY_IN_STATE correctly returned without warning.Not logging a warning for same-state scenarios is appropriate—this is expected behavior during idempotent retries, not an error condition.
265-266: Good test exposure pattern.Exporting
__paymentTransitionswith the double-underscore prefix clearly signals it's for testing purposes while keeping the transition logic accessible for verification.frontend/lib/tests/payment-state-machine.helper.test.ts (3)
7-24: Well-structured mock setup for DB operations.The mock pattern with
updateReturningRowsandselectQueueprovides flexible control over test scenarios. The captured payload verification approach is clean.
61-66: Good regression guard for transition matrix.Pinning the expected allowed transitions prevents accidental matrix changes from breaking webhook handling.
118-137: Good coverage for provider 'none' early rejection.Verifying that
updateMockis not called (Line 135) confirms the early-exit behavior before DB interaction, which is the intended optimization.frontend/lib/tests/payment-state-legacy-writers.test.ts (4)
17-49: Comprehensive test order seeding.The
seedOrderhelper creates realistic order data with all required NOT NULL fields, making tests self-contained and avoiding schema-related failures.
63-71: Good cleanup pattern for test isolation.Using
afterEachwith acreatedarray ensures orders are cleaned up even if tests fail, preventing test pollution.
167-185: Valuable tripwire test for direct paymentStatus writes.This regression test scanning for
.set({ ... paymentStatus:patterns is a clever guardrail to catch accidental bypasses ofguardedPaymentStatusUpdate. The 800-char window should cover typical set calls.Note: The regex may miss dynamically constructed sets or have false positives for commented code. Consider supplementing with a linting rule if this becomes a recurring concern.
73-94: Good coverage for Stripe forbidden transitions.Testing
failed -> requires_paymentas an invalid transition path exercises an important edge case in the payment-intent flow.frontend/app/[locale]/shop/admin/products/page.tsx (1)
9-9: LGTM: Clean migration to structured logging.The replacement of
console.warnwithlogWarnprovides better observability while maintaining the same error-handling behavior (returningnullon invalid price data).Also applies to: 26-31
frontend/app/[locale]/shop/admin/products/_components/product-form.tsx (1)
8-8: LGTM: Structured logging with good context.The migration from
console.errortologErroradds valuable context (mode, productId, slug) for debugging admin product form failures while preserving existing error-handling behavior.Also applies to: 348-352
frontend/lib/services/products/mapping.ts (1)
5-21: LGTM: Clean mapping function with proper type normalization.The function correctly transforms database rows to domain objects, handling price conversions via
fromDbMoney/fromCentsand normalizing nullable fields toundefinedfor TypeScript consistency. Since this is used on validated database results (e.g., from.returning()), the approach is sound.frontend/lib/tests/order-items-variants.test.ts (1)
29-35: LGTM: Robust variant normalization testing.The test correctly validates that the system normalizes variant values (size/color) to lowercase, with consistent assertions at both the API response level and the database level. The normalization helper ensures stable comparisons regardless of input casing.
Also applies to: 76-88, 104-109
frontend/lib/services/orders/refund.ts (2)
43-57: LGTM on error handling.The error handling comprehensively covers all
GuardedPaymentUpdateResultoutcomes:ALREADY_IN_STATE(idempotent),INVALID_TRANSITION,PROVIDER_MISMATCH,BLOCKED, andNOT_FOUND. The fallback toOrderNotFoundErrorcorrectly handles theNOT_FOUNDreason.
59-64: LGTM on restock logic.The idempotent handling is correct: restock triggers on successful transition or when already in the refunded state, and the
!order.stockRestoredguard prevents double-restocking.frontend/lib/services/orders/payment-intent.ts (2)
29-54: LGTM on validation logic.The validation sequence is well-structured: provider check → status allowlist → conflict detection → idempotent short-circuit. This prevents invalid state transitions and ensures consistent error messaging.
56-74: LGTM on state transition handling.The
allowSameStateUpdate: truecorrectly allows updatingpaymentIntentIdwhile remaining inrequires_payment. The pre-validation ensures that by the timeguardedPaymentStatusUpdateis called, only unexpected edge cases would fail.frontend/lib/services/products/slug.ts (1)
52-55: LGTM with minor semantic note.
SlugConflictErroris used for both "slug normalization failed" and "slug conflict". While semantically different, both represent "cannot use this slug" from the caller's perspective. This is acceptable for MVP.frontend/app/api/shop/checkout/route.ts (2)
13-13: LGTM on InvalidVariantError integration.The error code is correctly added to
EXPECTED_BUSINESS_ERROR_CODES, the instanceof check is added toisExpectedBusinessError, and the import is properly included.Also applies to: 32-32, 52-52
467-474: LGTM on InvalidVariantError response handling.The 400 response with structured
details(productId, field, value, allowed) provides actionable feedback to clients for invalid variant selections.frontend/lib/services/products/admin/queries.ts (2)
43-59: LGTM on price row mapping.The defensive validation with
assertMoneyMinorIntensures data integrity even if DB drivers return unexpected types. NullableoriginalPriceMinoris handled correctly.
72-93: LGTM on dynamic filtering.The conditional
whereclause correctly handles empty filters by passingundefined, and the filter application forisActive,category, andtypeis clean and correct.frontend/lib/services/products/types.ts (1)
3-11: LGTM on price row types.The distinction between
AdminProductPriceRow(includes legacy fields for rollout) andNormalizedPriceRow(canonical only for internal use) is clear and well-documented.Also applies to: 24-28
frontend/lib/services/products/mutations/create.ts (2)
28-35: LGTM on image upload handling.The upload error is logged and rethrown appropriately. The optional chaining on line 59 (
uploaded?.secureUrl ?? '') is defensive but harmless.Also applies to: 59-59
80-85: LGTM on slug conflict handling.The
onConflictDoNothingcombined with checking for an empty result correctly handles the race condition where another transaction might insert the same slug between the uniqueness check and the insert.frontend/lib/services/products/mutations/update.ts (1)
57-97: SALE invariant enforcement via “final merged” price rows looks solid.Merging DB rows with incoming upserts before enforcing
originalPriceMinorrequirements is the right way to prevent invalid SALE state.frontend/lib/tests/checkout-no-payments.test.ts (1)
160-183: Extending test helper to support per-item variants is a good alignment with checkout payload.frontend/lib/services/products.ts (1)
1-18: Facade re-exports are clean and keep the public API stable.frontend/lib/services/orders.ts (1)
1-12: Orders facade re-exports look clean and consistent with the service decomposition goal.frontend/lib/services/products/prices.ts (1)
18-37:assertMoneyMinorIntcorrectly rejects fractional minor units (no truncation).frontend/lib/shop/data.ts (1)
139-147: Logging invalid DB/shop product payloads with meta is a big observability win.frontend/lib/services/products/cart/rehydrate.ts (2)
69-103: Input validation is already in place via schema.The call site in
frontend/app/api/shop/cart/rehydrate/route.tsvalidates the payload withcartRehydratePayloadSchema.safeParse()before extracting items and callingrehydrateCartItems(). The schema enforcesproductIdas a valid UUID andquantityas an integer within bounds (min(1).max(MAX_QUANTITY_PER_LINE)), with early rejection of invalid payloads. This prevents the risk of unvalidated input reaching the function.
103-143: I don't have a review comment to rewrite. Please provide the review comment within<review_comment>tags, along with the repository context or file information needed for verification.frontend/lib/services/orders/summary.ts (2)
20-49: Good:orderItemSummarySelectionusescoalesceto keep item snapshots stable while falling back to product data.
This is a nice way to support “product changed after order” without breaking summaries.
122-130: Nice invariant check:paymentProvider=nonewithpaymentIntentIdthrows early.frontend/lib/services/orders/_shared.ts (2)
54-76:mergeCheckoutItemsis solid: canonical keying + MAX_QUANTITY_PER_LINE enforced at merge time.
90-125: Good:hashIdempotencyRequestnormalizes + sorts items for stable hashing.frontend/lib/services/orders/sweeps.ts (1)
75-126: Claim gating logic looks correct (expired-or-null leases only) and is re-checked in the UPDATE.
That’s the right pattern to avoid double-claim under concurrency.Also applies to: 201-260, 338-383
frontend/lib/services/orders/checkout.ts (1)
384-514: Idempotency handling is strong: request hashing, conflict detection, and legacy hash backfill are all headed in the right direction.Also applies to: 574-633
frontend/lib/services/orders/restock.ts (1)
23-54: No issue:tryClaimRestockLeaseis correctly scoped as a private helper function.It is used only internally within
restockOrder(line 197) and no external modules import it. The public surface isrestockOrder, which is properly exported viaorders.ts. This is intentional design.Likely an incorrect or invalid review comment.
| // 3.1) Variant validation (fail-fast; no side effects) | ||
| const variantMap = new Map( | ||
| dbProducts.map(p => [ | ||
| p.id, | ||
| { | ||
| allowedSizes: parseVariantList((p as any).sizes), | ||
| allowedColors: parseVariantList((p as any).colors), | ||
| }, | ||
| ]) | ||
| ); | ||
|
|
||
| for (const item of normalizedItems) { | ||
| const cfg = variantMap.get(item.productId); | ||
| if (!cfg) continue; // product existence handled elsewhere | ||
|
|
||
| const selectedSize = normVariant(item.selectedSize ?? ''); | ||
| const selectedColor = normVariant(item.selectedColor ?? ''); | ||
|
|
||
| if (selectedSize) { | ||
| if ( | ||
| cfg.allowedSizes.length === 0 || | ||
| !cfg.allowedSizes.includes(selectedSize) | ||
| ) { | ||
| throw new InvalidVariantError('Invalid size selection.', { | ||
| productId: item.productId, | ||
| field: 'selectedSize', | ||
| value: selectedSize, | ||
| allowed: cfg.allowedSizes, | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| if (selectedColor) { | ||
| if ( | ||
| cfg.allowedColors.length === 0 || | ||
| !cfg.allowedColors.includes(selectedColor) | ||
| ) { | ||
| throw new InvalidVariantError('Invalid color selection.', { | ||
| productId: item.productId, | ||
| field: 'selectedColor', | ||
| value: selectedColor, | ||
| allowed: cfg.allowedColors, | ||
| }); | ||
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Variant validation doesn’t reject “missing” size/color when the product has configured options.
Current logic only rejects when a non-empty selectedSize/selectedColor is invalid (Line 545+ / Line 559+). Per PR objectives, a product with allowedSizes.length > 0 (or colors) should reject empty selection as well.
Proposed fix (treat configured variants as required selections)
for (const item of normalizedItems) {
const cfg = variantMap.get(item.productId);
if (!cfg) continue; // product existence handled elsewhere
const selectedSize = normVariant(item.selectedSize ?? '');
const selectedColor = normVariant(item.selectedColor ?? '');
+ // If product has configured variants, selection is required.
+ if (cfg.allowedSizes.length > 0 && !selectedSize) {
+ throw new InvalidVariantError('Missing size selection.', {
+ productId: item.productId,
+ field: 'selectedSize',
+ value: selectedSize,
+ allowed: cfg.allowedSizes,
+ });
+ }
if (selectedSize) {
if (
cfg.allowedSizes.length === 0 ||
!cfg.allowedSizes.includes(selectedSize)
) {
@@
}
+ if (cfg.allowedColors.length > 0 && !selectedColor) {
+ throw new InvalidVariantError('Missing color selection.', {
+ productId: item.productId,
+ field: 'selectedColor',
+ value: selectedColor,
+ allowed: cfg.allowedColors,
+ });
+ }
if (selectedColor) {
if (
cfg.allowedColors.length === 0 ||
!cfg.allowedColors.includes(selectedColor)
) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // 3.1) Variant validation (fail-fast; no side effects) | |
| const variantMap = new Map( | |
| dbProducts.map(p => [ | |
| p.id, | |
| { | |
| allowedSizes: parseVariantList((p as any).sizes), | |
| allowedColors: parseVariantList((p as any).colors), | |
| }, | |
| ]) | |
| ); | |
| for (const item of normalizedItems) { | |
| const cfg = variantMap.get(item.productId); | |
| if (!cfg) continue; // product existence handled elsewhere | |
| const selectedSize = normVariant(item.selectedSize ?? ''); | |
| const selectedColor = normVariant(item.selectedColor ?? ''); | |
| if (selectedSize) { | |
| if ( | |
| cfg.allowedSizes.length === 0 || | |
| !cfg.allowedSizes.includes(selectedSize) | |
| ) { | |
| throw new InvalidVariantError('Invalid size selection.', { | |
| productId: item.productId, | |
| field: 'selectedSize', | |
| value: selectedSize, | |
| allowed: cfg.allowedSizes, | |
| }); | |
| } | |
| } | |
| if (selectedColor) { | |
| if ( | |
| cfg.allowedColors.length === 0 || | |
| !cfg.allowedColors.includes(selectedColor) | |
| ) { | |
| throw new InvalidVariantError('Invalid color selection.', { | |
| productId: item.productId, | |
| field: 'selectedColor', | |
| value: selectedColor, | |
| allowed: cfg.allowedColors, | |
| }); | |
| } | |
| } | |
| } | |
| // 3.1) Variant validation (fail-fast; no side effects) | |
| const variantMap = new Map( | |
| dbProducts.map(p => [ | |
| p.id, | |
| { | |
| allowedSizes: parseVariantList((p as any).sizes), | |
| allowedColors: parseVariantList((p as any).colors), | |
| }, | |
| ]) | |
| ); | |
| for (const item of normalizedItems) { | |
| const cfg = variantMap.get(item.productId); | |
| if (!cfg) continue; // product existence handled elsewhere | |
| const selectedSize = normVariant(item.selectedSize ?? ''); | |
| const selectedColor = normVariant(item.selectedColor ?? ''); | |
| // If product has configured variants, selection is required. | |
| if (cfg.allowedSizes.length > 0 && !selectedSize) { | |
| throw new InvalidVariantError('Missing size selection.', { | |
| productId: item.productId, | |
| field: 'selectedSize', | |
| value: selectedSize, | |
| allowed: cfg.allowedSizes, | |
| }); | |
| } | |
| if (selectedSize) { | |
| if ( | |
| cfg.allowedSizes.length === 0 || | |
| !cfg.allowedSizes.includes(selectedSize) | |
| ) { | |
| throw new InvalidVariantError('Invalid size selection.', { | |
| productId: item.productId, | |
| field: 'selectedSize', | |
| value: selectedSize, | |
| allowed: cfg.allowedSizes, | |
| }); | |
| } | |
| } | |
| if (cfg.allowedColors.length > 0 && !selectedColor) { | |
| throw new InvalidVariantError('Missing color selection.', { | |
| productId: item.productId, | |
| field: 'selectedColor', | |
| value: selectedColor, | |
| allowed: cfg.allowedColors, | |
| }); | |
| } | |
| if (selectedColor) { | |
| if ( | |
| cfg.allowedColors.length === 0 || | |
| !cfg.allowedColors.includes(selectedColor) | |
| ) { | |
| throw new InvalidVariantError('Invalid color selection.', { | |
| productId: item.productId, | |
| field: 'selectedColor', | |
| value: selectedColor, | |
| allowed: cfg.allowedColors, | |
| }); | |
| } | |
| } | |
| } |
| } catch (e) { | ||
| const failAt = new Date(); | ||
| await db | ||
| .update(orders) | ||
| .set({ inventoryStatus: 'release_pending', updatedAt: failAt }) | ||
| .where(eq(orders.id, orderId)); | ||
|
|
||
| // best-effort release | ||
| for (const it of itemsToReserve) { | ||
| try { | ||
| await applyReleaseMove(orderId, it.productId, it.quantity); | ||
| } catch (releaseErr) { | ||
| logError( | ||
| `[createOrderWithItems] release failed orderId=${orderId} productId=${it.productId} quantity=${it.quantity}`, | ||
| releaseErr | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| const isOos = e instanceof InsufficientStockError; | ||
| await db | ||
| .update(orders) | ||
| .set({ | ||
| status: 'INVENTORY_FAILED', | ||
| inventoryStatus: 'released', | ||
| failureCode: isOos ? 'OUT_OF_STOCK' : 'INTERNAL_ERROR', | ||
| failureMessage: isOos | ||
| ? e.message | ||
| : 'Checkout failed after reservation attempt.', | ||
| stockRestored: true, | ||
| restockedAt: failAt, | ||
| updatedAt: failAt, | ||
| }) | ||
| .where(eq(orders.id, orderId)); | ||
|
|
||
| const payRes = await guardedPaymentStatusUpdate({ | ||
| orderId, | ||
| paymentProvider, | ||
| to: 'failed', | ||
| source: 'checkout', | ||
| }); | ||
|
|
||
| if (!payRes.applied && payRes.reason !== 'ALREADY_IN_STATE') { | ||
| logError( | ||
| `[createOrderWithItems] paymentStatus transition to failed blocked orderId=${orderId} provider=${paymentProvider} reason=${payRes.reason}`, | ||
| new Error('payment_transition_blocked') | ||
| ); | ||
| } | ||
|
|
||
| throw e; | ||
| } |
There was a problem hiding this comment.
Failure paths can mark stockRestored=true / restockedAt!=null even if release partially failed.
Both in createOrderWithItems (Line 747-773) and reconcileNoPaymentOrder (Line 176-201), release errors are swallowed per-item, but the order is still finalized as “stock restored”. That can permanently prevent later janitors from retrying release, while inventory might still be reserved.
Also applies to: 169-218
🤖 Prompt for AI Agents
In @frontend/lib/services/orders/checkout.ts around lines 740 - 790, The catch
blocks in createOrderWithItems and reconcileNoPaymentOrder swallow per-item
release failures but unconditionally set stockRestored=true and restockedAt,
which can hide partial failures; change the flow to track whether any
applyReleaseMove calls failed (e.g., a boolean releaseFailed or failedItems list
when iterating itemsToReserve), log details, and only set stockRestored=true and
restockedAt when all releases succeeded; if any release failed, update
inventoryStatus (e.g., to 'release_failed' or keep 'release_pending') and record
a failure indicator/message (failureCode/failureMessage or a new column) so
janitors can retry, and ensure the final DB update uses that flag (references:
createOrderWithItems, reconcileNoPaymentOrder, itemsToReserve, applyReleaseMove,
stockRestored, restockedAt, inventoryStatus).
| export async function refundOrder( | ||
| orderId: string | ||
| ): Promise<OrderSummaryWithMinor> { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check type definitions and function signatures
echo "=== OrderSummaryWithMinor definition ==="
rg -n "OrderSummaryWithMinor" --type=ts -B2 -A3
echo ""
echo "=== OrderDetail definition ==="
rg -n "type OrderDetail\|interface OrderDetail" --type=ts -B2 -A5
echo ""
echo "=== getOrderById return type ==="
rg -n "function getOrderById\|export.*getOrderById" --type=ts -B1 -A3
echo ""
echo "=== refundOrder function full implementation ==="
rg -n "export async function refundOrder" --type=ts -A20Repository: DevLoversTeam/devlovers.net
Length of output: 8493
🏁 Script executed:
#!/bin/bash
# Find getOrderById implementation
echo "=== getOrderById function definition ==="
rg -n "function getOrderById" --type=ts -B2 -A10
echo ""
echo "=== Complete refundOrder implementation ==="
sed -n '12,70p' frontend/lib/services/orders/refund.tsRepository: DevLoversTeam/devlovers.net
Length of output: 3672
Fix return type annotation mismatch.
refundOrder declares Promise<OrderSummaryWithMinor> but returns getOrderById, which is annotated to return Promise<OrderDetail> (equal to OrderSummary). Since OrderSummaryWithMinor requires the additional totalAmountMinor field, either correct getOrderById's return type annotation from Promise<OrderDetail> to Promise<OrderSummaryWithMinor>, or adjust refundOrder to construct the required type explicitly.
🤖 Prompt for AI Agents
In @frontend/lib/services/orders/refund.ts around lines 12 - 14, refundOrder
currently declares it returns Promise<OrderSummaryWithMinor> but simply returns
the result of getOrderById which is typed as
Promise<OrderDetail>/Promise<OrderSummary>, so the types mismatch; fix by either
updating getOrderById's return type to Promise<OrderSummaryWithMinor> if it
truly returns totalAmountMinor, or modify refundOrder to call getOrderById then
build and return an object that includes totalAmountMinor (e.g., compute or map
the value) so the returned value conforms to OrderSummaryWithMinor; locate and
update the return type on getOrderById or add the mapping/field construction
inside refundOrder accordingly.
| // already released / legacy idempotency | ||
| if ( | ||
| order.inventoryStatus === 'released' || | ||
| order.stockRestored || | ||
| order.restockedAt !== null | ||
| ) | ||
| return; | ||
|
|
||
| // If state says "none" we still may have reserve moves (crash between reserve and status update). | ||
| const reservedMoves = await db | ||
| .select({ | ||
| productId: inventoryMoves.productId, | ||
| quantity: inventoryMoves.quantity, | ||
| }) | ||
| .from(inventoryMoves) | ||
| .where( | ||
| and( | ||
| eq(inventoryMoves.orderId, orderId), | ||
| eq(inventoryMoves.type, 'reserve') | ||
| ) | ||
| ); | ||
|
|
||
| if (!reservedMoves.length) { | ||
| // Nothing was reserved. For no-payments orders this is an "orphan" that must become terminal. | ||
| if ( | ||
| isNoPayment && | ||
| (reason === 'failed' || reason === 'canceled' || reason === 'stale') | ||
| ) { | ||
| const now = new Date(); | ||
|
|
||
| const [touched] = await db | ||
| .update(orders) | ||
| .set({ | ||
| status: 'INVENTORY_FAILED', | ||
| inventoryStatus: 'released', | ||
| failureCode: order.failureCode ?? 'STALE_ORPHAN', | ||
| failureMessage: | ||
| order.failureMessage ?? | ||
| 'Orphan order: no inventory reservation was recorded.', | ||
| stockRestored: true, | ||
| restockedAt: now, | ||
| updatedAt: now, | ||
| }) | ||
| .where(and(eq(orders.id, orderId), eq(orders.stockRestored, false))) | ||
| .returning({ id: orders.id }); | ||
|
|
||
| if (!touched) return; | ||
|
|
||
| // paymentStatus transition only via guard | ||
| await guardedPaymentStatusUpdate({ | ||
| orderId, | ||
| paymentProvider: provider, | ||
| to: 'failed', | ||
| source: transitionSource, | ||
| // tie to this exact finalize marker (prevents races) | ||
| extraWhere: eq(orders.restockedAt, now), | ||
| }); | ||
|
|
||
| return; | ||
| } | ||
|
|
||
| // Stripe (or any non-none provider): stale orphan must become terminal | ||
| if (reason === 'stale') { | ||
| const now = new Date(); | ||
|
|
||
| const [touched] = await db | ||
| .update(orders) | ||
| .set({ | ||
| status: 'INVENTORY_FAILED', | ||
| inventoryStatus: 'released', | ||
| failureCode: order.failureCode ?? 'STALE_ORPHAN', | ||
| failureMessage: | ||
| order.failureMessage ?? | ||
| 'Orphan order: no inventory reservation was recorded.', | ||
| stockRestored: true, | ||
| restockedAt: now, | ||
| updatedAt: now, | ||
| }) | ||
| .where(and(eq(orders.id, orderId), eq(orders.stockRestored, false))) | ||
| .returning({ id: orders.id }); | ||
|
|
||
| if (!touched) return; | ||
|
|
||
| // paymentStatus transition only via guard (provider from DB) | ||
| await guardedPaymentStatusUpdate({ | ||
| orderId, | ||
| paymentProvider: provider, | ||
| to: 'failed', | ||
| source: transitionSource, | ||
| extraWhere: eq(orders.restockedAt, now), | ||
| }); | ||
|
|
||
| return; | ||
| } | ||
|
|
||
| return; | ||
| } | ||
|
|
||
| // safety: paid can only be released via refund | ||
| // IMPORTANT: for payment_provider='none', payment_status='paid' is not a finality signal | ||
| // (forced by DB CHECK). Finality is inventory_status='reserved'. | ||
| if (!isNoPayment && order.paymentStatus === 'paid' && reason !== 'refunded') { | ||
| throw new OrderStateInvalidError( | ||
| `Cannot restock a paid order without refund reason.`, | ||
| { orderId, details: { reason, paymentStatus: order.paymentStatus } } | ||
| ); | ||
| } | ||
| // If we have reserved moves, we must claim a lease to avoid concurrent double-processing. | ||
| // (Actual stock safety is guaranteed by inventory_moves move_key, but lease prevents wasted work | ||
| // and prevents "restocked_at" churn under concurrency.) | ||
| const claimTtlMinutes = options?.claimTtlMinutes ?? 5; | ||
| const workerId = options?.workerId ?? 'restock'; | ||
| if (!options?.alreadyClaimed) { | ||
| const claimed = await tryClaimRestockLease({ | ||
| orderId, | ||
| workerId, | ||
| claimTtlMinutes, | ||
| }); | ||
| if (!claimed) return; // someone else is processing | ||
| } | ||
| const now = new Date(); | ||
|
|
||
| await db | ||
| .update(orders) | ||
| .set({ inventoryStatus: 'release_pending', updatedAt: now }) | ||
| .where(and(eq(orders.id, orderId), ne(orders.inventoryStatus, 'released'))); | ||
|
|
||
| for (const item of reservedMoves) | ||
| await applyReleaseMove(orderId, item.productId, item.quantity); | ||
| // FINALIZE ONCE: only one caller may flip stock_restored/restocked_at | ||
| // If RETURNING is empty => already finalized by another worker (or previous attempt). | ||
| const finalizedAt = new Date(); | ||
| const [finalized] = await db | ||
| .update(orders) | ||
| .set({ | ||
| stockRestored: true, | ||
| restockedAt: finalizedAt, | ||
| updatedAt: finalizedAt, | ||
| }) | ||
| .where(and(eq(orders.id, orderId), eq(orders.stockRestored, false))) | ||
| .returning({ id: orders.id }); | ||
|
|
||
| if (!finalized) return; | ||
|
|
||
| let normalizedStatus: PaymentStatus | undefined; | ||
| if (reason === 'refunded' && !isNoPayment) normalizedStatus = 'refunded'; | ||
| else if (reason === 'failed' || reason === 'canceled' || reason === 'stale') | ||
| normalizedStatus = 'failed'; | ||
|
|
||
| const shouldCancel = reason === 'canceled'; | ||
| const shouldFail = reason === 'failed' || reason === 'stale'; | ||
| await db | ||
| .update(orders) | ||
| .set({ | ||
| inventoryStatus: 'released', | ||
| updatedAt: now, | ||
| ...(shouldFail ? { status: 'INVENTORY_FAILED' } : {}), | ||
| ...(shouldCancel ? { status: 'CANCELED' } : {}), | ||
| }) | ||
| .where(eq(orders.id, orderId)); | ||
|
|
||
| if (normalizedStatus) { | ||
| await guardedPaymentStatusUpdate({ | ||
| orderId, | ||
| paymentProvider: provider, | ||
| to: normalizedStatus, | ||
| source: transitionSource, | ||
| // bind to finalize-once marker | ||
| extraWhere: eq(orders.restockedAt, finalizedAt), | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
Restock finalization is not crash-safe: early-return prevents completing terminal updates (esp. paymentStatus), and updatedAt can move backwards.
Key problems:
- Line 85-90: returning when
stockRestored/restockedAtis set prevents retry from applying a missingguardedPaymentStatusUpdate(e.g., crash after DB finalize, before payment transition). - Line 215-243: finalize is split across multiple updates; a crash after Line 217-225 but before Line 235-254 can leave
inventoryStatus='release_pending'forever (and retried calls will return early). - Line 239:
updatedAt: now(Line 204) happens afterupdatedAt: finalizedAt(Line 221), soupdatedAtcan decrease.
Proposed fix (make finalize atomic + allow crash-retry to finish payment transition)
export async function restockOrder(
orderId: string,
options?: RestockOptions
): Promise<void> {
const reason = options?.reason;
const [order] = await db
.select({
@@
.limit(1);
if (!order) throw new OrderNotFoundError('Order not found');
@@
- // already released / legacy idempotency
- if (
- order.inventoryStatus === 'released' ||
- order.stockRestored ||
- order.restockedAt !== null
- )
- return;
+ // Idempotency: if already finalized, still allow "finish" payment transition on retry.
+ // (e.g., crash after finalize update but before guardedPaymentStatusUpdate)
+ if (order.inventoryStatus === 'released' && order.restockedAt !== null) {
+ const normalizedStatus =
+ reason === 'refunded' && !isNoPayment
+ ? 'refunded'
+ : reason === 'failed' || reason === 'canceled' || reason === 'stale'
+ ? 'failed'
+ : undefined;
+
+ if (normalizedStatus) {
+ await guardedPaymentStatusUpdate({
+ orderId,
+ paymentProvider: provider,
+ to: normalizedStatus,
+ source: transitionSource,
+ extraWhere: eq(orders.restockedAt, order.restockedAt),
+ });
+ }
+ return;
+ }
@@
- const now = new Date();
+ const now = new Date();
await db
.update(orders)
.set({ inventoryStatus: 'release_pending', updatedAt: now })
.where(and(eq(orders.id, orderId), ne(orders.inventoryStatus, 'released')));
for (const item of reservedMoves)
await applyReleaseMove(orderId, item.productId, item.quantity);
- // FINALIZE ONCE: only one caller may flip stock_restored/restocked_at
- // If RETURNING is empty => already finalized by another worker (or previous attempt).
- const finalizedAt = new Date();
+ // FINALIZE ONCE: do all terminal flags in one write to be crash-safe.
+ const finalizedAt = new Date();
+ const shouldCancel = reason === 'canceled';
+ const shouldFail = reason === 'failed' || reason === 'stale';
const [finalized] = await db
.update(orders)
.set({
stockRestored: true,
restockedAt: finalizedAt,
- updatedAt: finalizedAt,
+ inventoryStatus: 'released',
+ ...(shouldFail ? { status: 'INVENTORY_FAILED' } : {}),
+ ...(shouldCancel ? { status: 'CANCELED' } : {}),
+ updatedAt: finalizedAt,
})
.where(and(eq(orders.id, orderId), eq(orders.stockRestored, false)))
.returning({ id: orders.id });
if (!finalized) return;
let normalizedStatus: PaymentStatus | undefined;
if (reason === 'refunded' && !isNoPayment) normalizedStatus = 'refunded';
else if (reason === 'failed' || reason === 'canceled' || reason === 'stale')
normalizedStatus = 'failed';
- const shouldCancel = reason === 'canceled';
- const shouldFail = reason === 'failed' || reason === 'stale';
- await db
- .update(orders)
- .set({
- inventoryStatus: 'released',
- updatedAt: now,
- ...(shouldFail ? { status: 'INVENTORY_FAILED' } : {}),
- ...(shouldCancel ? { status: 'CANCELED' } : {}),
- })
- .where(eq(orders.id, orderId));
-
if (normalizedStatus) {
await guardedPaymentStatusUpdate({
orderId,
paymentProvider: provider,
to: normalizedStatus,
source: transitionSource,
// bind to finalize-once marker
extraWhere: eq(orders.restockedAt, finalizedAt),
});
}
}🤖 Prompt for AI Agents
In @frontend/lib/services/orders/restock.ts around lines 84 - 255, Early-return
when order.stockRestored/order.restockedAt is set prevents completing payment
transitions and split finalization updates allow updatedAt to move backwards;
fix by (1) replacing the initial "if (order.inventoryStatus==='released' ||
order.stockRestored || order.restockedAt !== null) return" with logic that, if
stockRestored/restockedAt is already set, does NOT return immediately but calls
guardedPaymentStatusUpdate with extraWhere: eq(orders.restockedAt,
order.restockedAt) to let any missing payment transition finish, then returns;
(2) make the finalize step atomic by merging the release_pending->finalize
sequence into a single conditional update that sets inventoryStatus='released',
stockRestored=true, restockedAt=finalizedAt, updatedAt=finalizedAt and status
(INVENTORY_FAILED/CANCELED) in one DB update (use finalizedAt for all
timestamps) so a crash cannot leave inventoryStatus='release_pending' or regress
updatedAt; and (3) keep existing claims (tryClaimRestockLease) and
applyReleaseMove usage but ensure all updates use the same finalizedAt timestamp
when binding guardedPaymentStatusUpdate (extraWhere: eq(orders.restockedAt,
finalizedAt)).
| if (!reservedMoves.length) { | ||
| // Nothing was reserved. For no-payments orders this is an "orphan" that must become terminal. | ||
| if ( | ||
| isNoPayment && | ||
| (reason === 'failed' || reason === 'canceled' || reason === 'stale') | ||
| ) { | ||
| const now = new Date(); | ||
|
|
||
| const [touched] = await db | ||
| .update(orders) | ||
| .set({ | ||
| status: 'INVENTORY_FAILED', | ||
| inventoryStatus: 'released', | ||
| failureCode: order.failureCode ?? 'STALE_ORPHAN', | ||
| failureMessage: | ||
| order.failureMessage ?? | ||
| 'Orphan order: no inventory reservation was recorded.', | ||
| stockRestored: true, | ||
| restockedAt: now, | ||
| updatedAt: now, | ||
| }) | ||
| .where(and(eq(orders.id, orderId), eq(orders.stockRestored, false))) | ||
| .returning({ id: orders.id }); | ||
|
|
||
| if (!touched) return; | ||
|
|
||
| // paymentStatus transition only via guard | ||
| await guardedPaymentStatusUpdate({ | ||
| orderId, | ||
| paymentProvider: provider, | ||
| to: 'failed', | ||
| source: transitionSource, | ||
| // tie to this exact finalize marker (prevents races) | ||
| extraWhere: eq(orders.restockedAt, now), | ||
| }); | ||
|
|
||
| return; | ||
| } | ||
|
|
||
| // Stripe (or any non-none provider): stale orphan must become terminal | ||
| if (reason === 'stale') { | ||
| const now = new Date(); | ||
|
|
||
| const [touched] = await db | ||
| .update(orders) | ||
| .set({ | ||
| status: 'INVENTORY_FAILED', | ||
| inventoryStatus: 'released', | ||
| failureCode: order.failureCode ?? 'STALE_ORPHAN', | ||
| failureMessage: | ||
| order.failureMessage ?? | ||
| 'Orphan order: no inventory reservation was recorded.', | ||
| stockRestored: true, | ||
| restockedAt: now, | ||
| updatedAt: now, | ||
| }) | ||
| .where(and(eq(orders.id, orderId), eq(orders.stockRestored, false))) | ||
| .returning({ id: orders.id }); | ||
|
|
||
| if (!touched) return; | ||
|
|
||
| // paymentStatus transition only via guard (provider from DB) | ||
| await guardedPaymentStatusUpdate({ | ||
| orderId, | ||
| paymentProvider: provider, | ||
| to: 'failed', | ||
| source: transitionSource, | ||
| extraWhere: eq(orders.restockedAt, now), | ||
| }); | ||
|
|
||
| return; | ||
| } | ||
|
|
||
| return; |
There was a problem hiding this comment.
reason === 'canceled' “orphan” path sets status: 'INVENTORY_FAILED' (not CANCELED) and Stripe non-stale orphans become no-ops.
Line 108-144 treats canceled as inventory failure for paymentProvider='none', while the reserved-moves path maps canceled to CANCELED (Line 233-242). Also, for non-none providers with !reservedMoves.length, only reason === 'stale' terminalizes (Line 145-177); other reasons silently return (Line 179).
🤖 Prompt for AI Agents
In @frontend/lib/services/orders/restock.ts around lines 106 - 179, The
orphan-handling logic misclassifies canceled orders and skips terminalizing
non-none-provider orphans; update the branches that handle !reservedMoves.length
so that when reason === 'canceled' you set orders.status to 'CANCELED' (not
'INVENTORY_FAILED') and still perform the same DB update/restock marker and
guardedPaymentStatusUpdate call (use the same extraWhere eq(orders.restockedAt,
now) tie) as in the existing canceled-reserved path; additionally, for non-none
providers ensure non-stale reasons like 'canceled' are handled (not just
'stale') by expanding the second branch that currently checks reason === 'stale'
to also handle 'canceled' and apply the same status and payment transition
logic; refer to the update(...) .set({...}) block, the isNoPayment check, the
second if (reason === 'stale') branch, and the guardedPaymentStatusUpdate(...)
call to make these changes.
| } catch (error) { | ||
| if (createdProductId) { | ||
| try { | ||
| await db.delete(products).where(eq(products.id, createdProductId)); | ||
| } catch (cleanupDbError) { | ||
| logError( | ||
| 'Failed to cleanup product after create failure', | ||
| cleanupDbError | ||
| ); | ||
| } | ||
| } | ||
| throw error; | ||
| } |
There was a problem hiding this comment.
Orphaned Cloudinary image on failure.
If an error occurs after the image upload succeeds but before full completion (e.g., during productPrices insertion), the uploaded image remains in Cloudinary as orphaned data. Consider adding cleanup for the uploaded image in the catch block.
Suggested enhancement
} catch (error) {
+ // Attempt to clean up orphaned Cloudinary image
+ if (uploaded?.publicId) {
+ try {
+ await deleteProductImage(uploaded.publicId);
+ } catch (cleanupImageError) {
+ logError('Failed to cleanup orphaned image after create failure', cleanupImageError);
+ }
+ }
+
if (createdProductId) {
try {
await db.delete(products).where(eq(products.id, createdProductId));This would require importing/implementing a deleteProductImage function from the Cloudinary module.
🤖 Prompt for AI Agents
In @frontend/lib/services/products/mutations/create.ts around lines 111 - 123,
The catch block currently only rolls back a created DB product
(createdProductId) but doesn't remove an uploaded Cloudinary image, so add
cleanup: import or implement deleteProductImage from your Cloudinary module and,
inside the catch, if you have an image public id (e.g., imagePublicId variable
or product.imagePublicId available in this scope) call await
deleteProductImage(imagePublicId) in a try/catch and log failures with logError
before/alongside the existing db.delete(products). Ensure deleteProductImage is
imported at the top and that all cleanup errors are caught and logged without
masking the original thrown error which is rethrown at the end.
| let uploaded: { secureUrl: string; publicId: string } | null = null; | ||
|
|
||
| if ((input as any).image instanceof File && (input as any).image.size > 0) { | ||
| try { | ||
| uploaded = await uploadProductImageFromFile((input as any).image); | ||
| } catch (error) { | ||
| logError('Failed to upload replacement image', error); | ||
| throw error; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Next.js 15 File global Server Actions Route Handlers Node runtime instanceof
💡 Result:
Short answer — key points and fixes:
-
Next.js 15: route handlers (app/.../route.ts) use the Web Request/Response APIs and by default run in the runtime you choose for that segment; you can force Node with:
export const runtime = 'nodejs';. [1][2] -
File‑global Server Actions (the "use server" exported functions) are tied to the page/segment runtime where they are used; they can run in edge-like runtimes or Node depending on the segment config. They are not automatically separate Node processes. [2][3]
-
The instanceof problem: when code crosses JS realms (EdgeVM / worker-like runtime vs Node realm), objects have different constructors (different Array/Response/Request prototypes) so instanceof checks can fail. This is a runtime/realm boundary issue, seen in Next/Edge contexts. Workarounds: run the code in the Node runtime (export runtime = 'nodejs'), avoid instanceof for cross‑realm checks (use duck typing or compare .constructor.name or Symbol.toStringTag), or marshal values (serialize/deserialize) across the boundary. [3][4]
-
Practical checks/fixes:
- If you need Node globals / Response instanceof to behave predictably, set runtime = 'nodejs' on the route/page. [1][2]
- If you must support edge/worker runtime, replace instanceof checks with safe alternatives (e.g., typeof, checking properties, or using Response.prototype.isPrototypeOf(obj)). [3]
- Don’t return a Response from API handlers in Node runtime where it’s unsupported by the handler helper — follow the docs for route handler return rules. [5]
Sources:
[1] Next.js docs — Route Handlers (app route) (route handler runtime config).
[2] Next.js 15 release notes / blog (file-global server actions, runtime behavior).
[3] Next.js GitHub / discussions and issue threads on EdgeVM / instanceof differences and server action runtimes.
[4] Community writeups about realm/instanceof behavior in edge/middleware contexts.
[5] Next.js discussion about API handler return/Response behavior.
If you want, tell me which code snippet is failing and I’ll point to the exact change (e.g., add export const runtime = 'nodejs' or rewrite the instanceof check).
🌐 Web query:
JavaScript instanceof behavior when class is undefined ReferenceError
💡 Result:
Short answer
- If the identifier for the "class" is not declared at all, referencing it causes a ReferenceError before instanceof runs.
Example: obj instanceof NotDeclared; // ReferenceError: NotDeclared is not defined. [1] - If the identifier exists but its value is undefined (e.g. let C; or const C = undefined), instanceof executes and throws a TypeError because the right-hand side is not a valid constructor (not callable / has no usable prototype).
Example: let C; obj instanceof C; // TypeError: Right-hand side of 'instanceof' is not callable / invalid operand. [2]
Why
- The JS engine first evaluates both operands. If evaluation of the RHS fails (identifier missing) you get ReferenceError. If it evaluates to a non-callable/non-object, the instanceof runtime check throws a TypeError. See MDN docs on instanceof and the "invalid right-hand side instanceof operand" error. [1][2]
References
[1] MDN — instanceof operator. (developer.mozilla.org)
[2] MDN — TypeError: invalid 'instanceof' operand. (developer.mozilla.org)
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/instanceof?utm_source=openai
- 2: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Errors/invalid_right_hand_side_instanceof_operand?utm_source=openai
🌐 Web query:
Next.js 15 File object polyfill server-side Node runtime
💡 Result:
Short answer: Next.js 15 only exposes the browser/Edge File API in Edge runtimes — the Node.js server runtime does not automatically polyfill global File. To use File server-side in Next.js (Node runtime) you must add a Node polyfill/ponyfill (for example formdata-node or fetch-blob) and assign it to globalThis (or use the library APIs directly).
How to (concise):
- Install a Node implementation, e.g.:
- npm install formdata-node form-data-encoder
- or npm install fetch-blob formdata-polyfill
- In server code (or a server-only entry), expose it:
- import { File } from 'formdata-node'
- globalThis.File = globalThis.File ?? File
- Use that File with FormData/FormDataEncoder or APIs that expect a File.
Why: Next.js documents that the Edge runtime includes web APIs like File/Blob, while the Node runtime does not polyfill all browser globals — you must supply them yourself or use Node libraries that implement the spec. (nextjs.org)
References:
- Next.js Edge runtime docs (shows File as an Edge API). (nextjs.org)
- formdata-node (File/FormData implementation for Node). (githubhelp.com)
If you want, tell me which package manager and runtime file(s) you’re using and I’ll give exact install + code snippets.
Citations:
- 1: https://nextjs.org/docs/app/api-reference/edge?utm_source=openai
- 2: https://nextjs.org/docs/app/api-reference/edge?utm_source=openai
- 3: https://githubhelp.com/octet-stream/form-data?utm_source=openai
🏁 Script executed:
cat -n frontend/lib/services/products/mutations/update.ts | head -80Repository: DevLoversTeam/devlovers.net
Length of output: 3097
🏁 Script executed:
# Check if File is imported or defined anywhere in the file
rg -n "import.*File|declare.*File|globalThis.File" frontend/lib/services/products/mutations/update.tsRepository: DevLoversTeam/devlovers.net
Length of output: 53
🏁 Script executed:
# Check if there's a "use server" directive or other server action markers
rg -n '"use server"|"use client"' frontend/lib/services/products/mutations/update.tsRepository: DevLoversTeam/devlovers.net
Length of output: 53
🏁 Script executed:
# Check the broader context around lines 46-55
sed -n '30,70p' frontend/lib/services/products/mutations/update.tsRepository: DevLoversTeam/devlovers.net
Length of output: 1289
🏁 Script executed:
# Check if this function is exported and where it's used
rg -l "updateProduct" frontend/lib/services/products/mutations/ --type ts --type tsxRepository: DevLoversTeam/devlovers.net
Length of output: 98
🏁 Script executed:
# Check if there's a "use server" at the top of the file (beyond line 80)
head -1 frontend/lib/services/products/mutations/update.tsRepository: DevLoversTeam/devlovers.net
Length of output: 124
🏁 Script executed:
# Check for any server action or route handler that imports/calls this function
rg -l "updateProduct|'use server'" frontend/ --type ts --type tsx | head -20Repository: DevLoversTeam/devlovers.net
Length of output: 98
🏁 Script executed:
# Find files that import updateProduct
rg "from.*update|import.*updateProduct" frontend/ --type tsRepository: DevLoversTeam/devlovers.net
Length of output: 287
🏁 Script executed:
# Check if there's a server action file in the mutations directory
ls -la frontend/lib/services/products/mutations/Repository: DevLoversTeam/devlovers.net
Length of output: 418
🏁 Script executed:
# Look for "use server" in the mutations directory
rg '"use server"' frontend/lib/services/products/Repository: DevLoversTeam/devlovers.net
Length of output: 53
🏁 Script executed:
# Check where updateProduct is called from
rg "updateProduct" frontend/ --type ts -B 2 -A 2Repository: DevLoversTeam/devlovers.net
Length of output: 4871
🏁 Script executed:
# Check if any route handlers or server actions call this
rg '"use server"|export.*async.*function' frontend/app --type ts | head -30Repository: DevLoversTeam/devlovers.net
Length of output: 2921
🏁 Script executed:
# Look for API routes or server actions that might call updateProduct
fd "route.ts|actions.ts|server.ts" frontend/Repository: DevLoversTeam/devlovers.net
Length of output: 1117
🏁 Script executed:
cat -n frontend/app/api/shop/admin/products/[id]/route.ts | head -100Repository: DevLoversTeam/devlovers.net
Length of output: 3670
🏁 Script executed:
# Check specifically for runtime export in this route handler
rg "export const runtime" frontend/app/api/shop/admin/products/[id]/route.tsRepository: DevLoversTeam/devlovers.net
Length of output: 53
🏁 Script executed:
sed -n '100,200p' frontend/app/api/shop/admin/products/[id]/route.tsRepository: DevLoversTeam/devlovers.net
Length of output: 3172
🏁 Script executed:
# Check if there's any global setup or polyfill for File in the project
rg "globalThis.File|File.*=.*import|polyfill.*File" frontend/ --type tsRepository: DevLoversTeam/devlovers.net
Length of output: 53
🏁 Script executed:
# Check if nodejs runtime is explicitly set anywhere in the app
rg "export const runtime = 'nodejs'" frontend/ --type tsRepository: DevLoversTeam/devlovers.net
Length of output: 164
🏁 Script executed:
# Check what the default runtime is for route handlers in this Next.js app
cat frontend/next.config.ts | head -50Repository: DevLoversTeam/devlovers.net
Length of output: 1019
🏁 Script executed:
# Look at the PATCH handler more to see what's passed to updateProduct
sed -n '200,250p' frontend/app/api/shop/admin/products/[id]/route.tsRepository: DevLoversTeam/devlovers.net
Length of output: 1446
🏁 Script executed:
# Check if the test file actually executes this code path
sed -n '1,50p' frontend/lib/tests/product-sale-invariant.test.tsRepository: DevLoversTeam/devlovers.net
Length of output: 1555
🏁 Script executed:
# Verify if the test passes an image to updateProduct
rg "image|updateProduct" frontend/lib/tests/product-sale-invariant.test.ts -A 5 -B 5 | head -80Repository: DevLoversTeam/devlovers.net
Length of output: 1797
Guard File usage to avoid runtime ReferenceError in non-browser/Edge contexts.
(input as any).image instanceof File will throw a ReferenceError if File isn't defined. This is a risk because the route handler calling updateProduct (frontend/app/api/shop/admin/products/[id]/route.ts) does not explicitly set export const runtime = 'nodejs', meaning it may execute in Next.js's Edge runtime, where the File global is not automatically available.
The same vulnerability exists in the route handler itself at the call site (line ~207), so both locations need the guard:
Proposed fix (safe File detection)
- if ((input as any).image instanceof File && (input as any).image.size > 0) {
+ const maybeImage = (input as any).image;
+ const hasFileGlobal = typeof File !== 'undefined';
+ if (hasFileGlobal && maybeImage instanceof File && maybeImage.size > 0) {
try {
- uploaded = await uploadProductImageFromFile((input as any).image);
+ uploaded = await uploadProductImageFromFile(maybeImage);
} catch (error) {
logError('Failed to upload replacement image', error);
throw error;
}
}Also apply the same guard in the route handler at the call site.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let uploaded: { secureUrl: string; publicId: string } | null = null; | |
| if ((input as any).image instanceof File && (input as any).image.size > 0) { | |
| try { | |
| uploaded = await uploadProductImageFromFile((input as any).image); | |
| } catch (error) { | |
| logError('Failed to upload replacement image', error); | |
| throw error; | |
| } | |
| } | |
| let uploaded: { secureUrl: string; publicId: string } | null = null; | |
| const maybeImage = (input as any).image; | |
| const hasFileGlobal = typeof File !== 'undefined'; | |
| if (hasFileGlobal && maybeImage instanceof File && maybeImage.size > 0) { | |
| try { | |
| uploaded = await uploadProductImageFromFile(maybeImage); | |
| } catch (error) { | |
| logError('Failed to upload replacement image', error); | |
| throw error; | |
| } | |
| } |
| try { | ||
| // 1) upsert prices | ||
| if (prices.length) { | ||
| const upsertRows = prices.map(p => { | ||
| const priceMinor = p.priceMinor; | ||
| const originalMinor = p.originalPriceMinor; | ||
|
|
||
| return { | ||
| productId: id, | ||
| currency: p.currency, | ||
| priceMinor, | ||
| originalPriceMinor: originalMinor, | ||
| price: toDbMoney(priceMinor), | ||
| originalPrice: | ||
| originalMinor == null ? null : toDbMoney(originalMinor), | ||
| }; | ||
| }); | ||
|
|
||
| await db | ||
| .insert(productPrices) | ||
| .values(upsertRows) | ||
| .onConflictDoUpdate({ | ||
| target: [productPrices.productId, productPrices.currency], | ||
| set: { | ||
| priceMinor: sql`excluded.price_minor`, | ||
| originalPriceMinor: sql`excluded.original_price_minor`, | ||
| price: sql`excluded.price`, | ||
| originalPrice: sql`excluded.original_price`, | ||
| updatedAt: sql`now()`, | ||
| }, | ||
| }); | ||
| } | ||
|
|
||
| // 2) update products | ||
| const [row] = await db | ||
| .update(products) | ||
| .set(updateData) | ||
| .where(eq(products.id, id)) | ||
| .returning(); | ||
|
|
||
| if (!row) { | ||
| throw new Error('PRODUCT_NOT_FOUND'); | ||
| } | ||
|
|
||
| if (uploaded && existing.imagePublicId) { | ||
| try { | ||
| await destroyProductImage(existing.imagePublicId); | ||
| } catch (cleanupError) { | ||
| logError('Failed to cleanup old image after update', cleanupError); | ||
| } | ||
| } | ||
|
|
||
| return mapRowToProduct(row); | ||
| } catch (error) { | ||
| // IMPORTANT: cleanup new image on failure (price upsert or product update) | ||
| if (uploaded?.publicId) { | ||
| try { | ||
| await destroyProductImage(uploaded.publicId); | ||
| } catch (cleanupError) { | ||
| logError( | ||
| 'Failed to cleanup uploaded image after update failure', | ||
| cleanupError | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| if ((error as { code?: string }).code === '23505') { | ||
| throw new SlugConflictError('Slug already exists.'); | ||
| } | ||
| throw error; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n frontend/lib/services/products/mutations/update.ts | sed -n '140,210p'Repository: DevLoversTeam/devlovers.net
Length of output: 2530
🏁 Script executed:
rg -n --type=ts -C2 '\.transaction\s*\(' frontend/libRepository: DevLoversTeam/devlovers.net
Length of output: 53
🏁 Script executed:
rg -n --type=ts '23505|SlugConflictError' frontend/lib/services/productsRepository: DevLoversTeam/devlovers.net
Length of output: 943
🏁 Script executed:
rg -n --type=ts 'import.*drizzle' frontend/lib | head -20Repository: DevLoversTeam/devlovers.net
Length of output: 1902
🏁 Script executed:
find frontend -name "*.ts" -type f | xargs grep -l "schema\|migration" | head -10Repository: DevLoversTeam/devlovers.net
Length of output: 480
🏁 Script executed:
rg -n "\.transaction|beginTransaction" frontend --type=tsRepository: DevLoversTeam/devlovers.net
Length of output: 53
🏁 Script executed:
find . -name "schema.ts" -o -name "*schema*" -type f | grep -v node_modulesRepository: DevLoversTeam/devlovers.net
Length of output: 99
🏁 Script executed:
cat -n frontend/drizzle/schema.ts | head -200Repository: DevLoversTeam/devlovers.net
Length of output: 10692
🏁 Script executed:
find frontend -name "*.ts" -type f | xargs grep -l "drizzle.*Database\|createConnection\|getDb" | head -5Repository: DevLoversTeam/devlovers.net
Length of output: 53
🏁 Script executed:
rg -n "export const db" frontend/lib --type=tsRepository: DevLoversTeam/devlovers.net
Length of output: 147
🏁 Script executed:
find frontend/db/schema -type f -name "*.ts" 2>/dev/nullRepository: DevLoversTeam/devlovers.net
Length of output: 274
🏁 Script executed:
find frontend/db -type f -name "*.ts" | head -20Repository: DevLoversTeam/devlovers.net
Length of output: 709
🏁 Script executed:
rg -n "export.*db" frontend/db --type=ts | head -20Repository: DevLoversTeam/devlovers.net
Length of output: 138
🏁 Script executed:
cat -n frontend/db/schema/shop.ts | head -250Repository: DevLoversTeam/devlovers.net
Length of output: 9412
🏁 Script executed:
cat -n frontend/db/index.tsRepository: DevLoversTeam/devlovers.net
Length of output: 1419
🏁 Script executed:
cat -n frontend/db/schema/shop.ts | sed -n '250,350p'Repository: DevLoversTeam/devlovers.net
Length of output: 3917
🏁 Script executed:
rg -n "productPrices" frontend/db/schema/shop.ts -A 30Repository: DevLoversTeam/devlovers.net
Length of output: 1551
🏁 Script executed:
rg -n "transaction\|beginTransaction" frontend --type=ts -iRepository: DevLoversTeam/devlovers.net
Length of output: 53
🏁 Script executed:
cat -n frontend/lib/services/products/mutations/update.ts | sed -n '1,50p'Repository: DevLoversTeam/devlovers.net
Length of output: 1897
🏁 Script executed:
rg -n "drizzle-orm/neon" frontend --type=tsRepository: DevLoversTeam/devlovers.net
Length of output: 142
🏁 Script executed:
cat -n frontend/db/index.tsRepository: DevLoversTeam/devlovers.net
Length of output: 1419
🏁 Script executed:
rg -n "neon\|NeonDatabase" frontend --type=ts -C 2Repository: DevLoversTeam/devlovers.net
Length of output: 53
🏁 Script executed:
find frontend -name "package.json" -type f | xargs cat | grep -A 5 "drizzle\|neon"Repository: DevLoversTeam/devlovers.net
Length of output: 908
🏁 Script executed:
cat -n frontend/lib/services/products/mutations/update.ts | sed -n '60,140p'Repository: DevLoversTeam/devlovers.net
Length of output: 3247
🏁 Script executed:
rg -n "updateData\|productPrices.*currency\|slug" frontend/lib/services/products/mutations/update.ts | head -30Repository: DevLoversTeam/devlovers.net
Length of output: 53
🏁 Script executed:
rg -n "neon\|HTTP\|serverless" frontend/db -C 2 --type=tsRepository: DevLoversTeam/devlovers.net
Length of output: 53
🏁 Script executed:
rg -n "transaction\|atomic\|ACID" frontend --type=ts -i | head -20Repository: DevLoversTeam/devlovers.net
Length of output: 53
🏁 Script executed:
cat -n package.json | grep -A 5 -B 5 "drizzle\|neon"Repository: DevLoversTeam/devlovers.net
Length of output: 115
🏁 Script executed:
find . -name "package.json" -type f | head -5Repository: DevLoversTeam/devlovers.net
Length of output: 116
🏁 Script executed:
cat -n ./package.json | grep -A 3 -B 3 "drizzle\|neon"Repository: DevLoversTeam/devlovers.net
Length of output: 117
🏁 Script executed:
cat ./frontend/package.json | grep -A 3 -B 3 "drizzle\|neon"Repository: DevLoversTeam/devlovers.net
Length of output: 1085
🏁 Script executed:
web_search Neon HTTP Drizzle ORM transaction support serverlessRepository: DevLoversTeam/devlovers.net
Length of output: 121
🌐 Web query:
Neon HTTP Drizzle ORM transaction support serverless
💡 Result:
Short answer: You can run non‑interactive (single-call) transactions over Neon’s HTTP/serverless driver, and Drizzle’s neon-http adapter supports those. But serverless HTTP does not support interactive/session transactions (multi-step transactions that require a persistent connection). For interactive transactions use Neon’s WebSocket/serverless client or a pooled TCP driver (or a non‑serverless Neon plan). [1][2][3][4]
Notes and recommendations:
- Drizzle: neon-http adapter is intended for serverless/edge and supports querying and non‑interactive transactions; for session/interactive transactions use the WebSocket serverless driver. [1]
- Neon: the serverless driver exposes two modes — HTTP (good for short-lived single operations and batched non‑interactive transactions) and WebSocket (required for interactive sessions/transactions). [2][3]
- If your app needs multi-statement transactions with conditional logic or open sessions across requests, use the WebSocket driver or a pooled TCP connection (or avoid serverless HTTP). [2][3][4]
Sources:
- Drizzle docs: Connect to Neon (neon-http vs websockets). [1]
- Neon docs: Connect from Drizzle / choosing driver (HTTP vs WebSocket). [2]
- Neon serverless driver docs: HTTP transaction examples and interactive transaction guidance. [3]
- Community note / tutorial warning about serverless drivers and transactions. [4]
Wrap price upsert + product update in a transaction, but note neon-http limitation.
Right now, productPrices can be upserted successfully and then products update can fail, leaving the database in an inconsistent state. However, the proposed transaction fix will not work with your current setup: drizzle-orm/neon-http is a serverless HTTP driver that does not support interactive/session-based transactions. It only supports non-interactive (single-call) transactions.
To fix this atomicity issue, you have a few options:
- Switch to Neon WebSocket driver – Use
drizzle-orm/neon-serverlesswith the WebSocket client instead of HTTP, which supports interactive transactions. - Use a pooled TCP connection – Switch from serverless HTTP to a persistent connection pool (e.g., PgBouncer, Neon's connection pooling).
- Accept the partial-write risk – Document that prices and product updates are not atomic and add compensating logic (e.g., periodic consistency checks).
- Batch as a single operation – Redesign the API to avoid separate statements, though this may not be feasible given the UPSERT + UPDATE pattern.
For now, this remains a major issue because the risk of data inconsistency is real. At minimum, document this limitation and consider prioritizing the driver migration.
| if ((error as { code?: string }).code === '23505') { | ||
| throw new SlugConflictError('Slug already exists.'); | ||
| } | ||
| throw error; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n --type=ts -C2 "code === '23505'" frontend/lib | head -n 50Repository: DevLoversTeam/devlovers.net
Length of output: 912
🏁 Script executed:
cat -n frontend/lib/services/products/mutations/update.ts | sed -n '200,220p'Repository: DevLoversTeam/devlovers.net
Length of output: 415
🏁 Script executed:
rg -n --type=ts "constraint" frontend/lib | head -n 30Repository: DevLoversTeam/devlovers.net
Length of output: 186
🏁 Script executed:
# Check what database library is being used
rg -n "import.*from.*'pg'" frontend/lib | head -20Repository: DevLoversTeam/devlovers.net
Length of output: 53
🏁 Script executed:
# Check for any error interfaces/types
rg -n "interface.*Error\|type.*Error" frontend/lib/services | head -30Repository: DevLoversTeam/devlovers.net
Length of output: 53
🏁 Script executed:
# Look for database error handling patterns
rg -n "constraint" frontend/lib/services -i -A2 -B2 | head -40Repository: DevLoversTeam/devlovers.net
Length of output: 53
🏁 Script executed:
# Check package.json for database dependencies
cat package.json | grep -A5 -B5 "postgres\|pg\|database"Repository: DevLoversTeam/devlovers.net
Length of output: 115
🏁 Script executed:
# Find package.json
find . -name "package.json" -type f 2>/dev/nullRepository: DevLoversTeam/devlovers.net
Length of output: 116
🏁 Script executed:
# Look for database/prisma setup
rg -n "prisma\|db\|database" --type=ts frontend/lib/services/products/mutations/update.ts -A5 -B5 | head -50Repository: DevLoversTeam/devlovers.net
Length of output: 53
🏁 Script executed:
# Find SlugConflictError definition
rg -n "class SlugConflictError\|export.*SlugConflictError" frontend/lib -A10Repository: DevLoversTeam/devlovers.net
Length of output: 53
🏁 Script executed:
# Look for database client initialization or error handling patterns
rg -n "catch.*error" frontend/lib/services/products/mutations/update.ts -B3 -A8Repository: DevLoversTeam/devlovers.net
Length of output: 982
🏁 Script executed:
# Check frontend package.json for dependencies
grep -E "postgres|pg|prisma|drizzle|knex|database" ./frontend/package.jsonRepository: DevLoversTeam/devlovers.net
Length of output: 455
🏁 Script executed:
# Search for SlugConflictError import or definition
rg -n "SlugConflictError" frontend/libRepository: DevLoversTeam/devlovers.net
Length of output: 1070
🏁 Script executed:
# Look at the complete import section of the update.ts file
head -n 30 frontend/lib/services/products/mutations/update.tsRepository: DevLoversTeam/devlovers.net
Length of output: 1027
🏁 Script executed:
# Check if there are any database error type definitions in the codebase
fd -e ts frontend/lib | xargs grep -l "code.*23505\|constraint.*error" 2>/dev/nullRepository: DevLoversTeam/devlovers.net
Length of output: 426
🏁 Script executed:
# Read the SlugConflictError definition
cat -n frontend/lib/services/errors.ts | sed -n '85,95p'Repository: DevLoversTeam/devlovers.net
Length of output: 455
🏁 Script executed:
# Check the create.ts mutation to see if it has same issue
cat -n frontend/lib/services/products/mutations/create.ts | sed -n '75,95p'Repository: DevLoversTeam/devlovers.net
Length of output: 812
🏁 Script executed:
# Search for all 23505 error handling to see the full pattern
rg -n "23505" frontend/lib -B5 -A5Repository: DevLoversTeam/devlovers.net
Length of output: 1858
🏁 Script executed:
# Check if there are any error type definitions or interfaces
rg -n "interface.*Error\|type.*Error" frontend/lib/services/errors.ts -A5Repository: DevLoversTeam/devlovers.net
Length of output: 53
🏁 Script executed:
# Check the database schema to see what unique constraints exist on products table
rg -n "unique\|constraint" frontend --type=ts --type=ts -i | grep -i "product\|slug\|sku" | head -30Repository: DevLoversTeam/devlovers.net
Length of output: 53
🏁 Script executed:
# Look for the products schema definition
fd schema . frontend -type f | head -10Repository: DevLoversTeam/devlovers.net
Length of output: 241
🏁 Script executed:
# Check if there's a db schema file
find frontend -name "*schema*" -type f 2>/dev/null | head -10Repository: DevLoversTeam/devlovers.net
Length of output: 97
🏁 Script executed:
# Read the schema file to see unique constraints
cat -n frontend/drizzle/schema.ts | head -100Repository: DevLoversTeam/devlovers.net
Length of output: 5537
🏁 Script executed:
# Search for products table definition specifically
rg -n "export.*products\|unique" frontend/drizzle/schema.ts -A10 -B2 | head -50Repository: DevLoversTeam/devlovers.net
Length of output: 53
🏁 Script executed:
# Find the canonical schema files
find frontend/db -name "*.ts" -type f 2>/dev/nullRepository: DevLoversTeam/devlovers.net
Length of output: 1110
🏁 Script executed:
# Search for products table in the canonical schema
rg -n "products\|slug.*unique\|sku.*unique" frontend/db -A5 -B2 | head -80Repository: DevLoversTeam/devlovers.net
Length of output: 53
🏁 Script executed:
# Check the shop schema file
cat -n frontend/db/schema/shop.tsRepository: DevLoversTeam/devlovers.net
Length of output: 14488
🏁 Script executed:
# Look for products table with unique constraints
rg -n "pgTable.*products\|unique\|slug\|sku" frontend/db/schema/shop.ts -A20 -B2Repository: DevLoversTeam/devlovers.net
Length of output: 53
🏁 Script executed:
# Read the full try-catch block around lines 193-210
cat -n frontend/lib/services/products/mutations/update.ts | sed -n '175,215p'Repository: DevLoversTeam/devlovers.net
Length of output: 1276
🏁 Script executed:
# Search for productPrices operations in the file
rg -n "productPrices" frontend/lib/services/products/mutations/update.ts -B2 -A2Repository: DevLoversTeam/devlovers.net
Length of output: 942
🏁 Script executed:
# Check if there's any documentation or error handling pattern with constraint field
rg -n "constraint\|\.constraint" frontend -A2 -B2Repository: DevLoversTeam/devlovers.net
Length of output: 53
🏁 Script executed:
# Look for the specific productPrices constraint definition again
cat -n frontend/db/schema/shop.ts | sed -n '305,315p'Repository: DevLoversTeam/devlovers.net
Length of output: 450
🏁 Script executed:
# Verify what the exact constraint name would be for slug
cat -n frontend/db/schema/shop.ts | sed -n '95,97p'Repository: DevLoversTeam/devlovers.net
Length of output: 228
🌐 Web query:
PostgreSQL error object constraint field pg driver Node.js
💡 Result:
Typical Node.js (node-postgres / pg) error object for a PostgreSQL constraint violation includes properties such as:
- name (e.g. 'error')
- severity
- code (SQLSTATE, e.g. '23505' for unique violation)
- message / detail (e.g. "Key (col)=(val) already exists.")
- schema, table, column (column often undefined for multi-column constraints)
- constraint (the constraint name)
- file, line, routine
Example (from real node-postgres output):
{ name: 'error',
severity: 'ERROR',
code: '23505',
detail: 'Key (tipo_documento_id, numero_documento)=(1,11456963) already exists.',
schema: 'public',
table: 'personas',
column: undefined,
constraint: 'ak_documento_personas',
file: 'nbtinsert.c',
line: '424',
routine: '_bt_check_unique' }.[1][2]
Notes:
- Use error.code to detect the type (e.g. '23505' duplicate key).
- error.constraint gives the constraint name; to find affected columns reliably you can query pg_constraint/pg_attribute using that name or parse error.detail when column is needed. [2][3]
Sources:
[1] node-postgres issue showing sample error object.
[2] StackOverflow discussion explaining Detail vs Constraint and why column may be empty.
[3] PostgreSQL docs / error examples (duplicate key 23505).
Check constraint name before mapping 23505 to SlugConflictError.
The current code maps all PostgreSQL unique-violation errors (code 23505) to SlugConflictError without verifying which constraint was violated. While the products table currently has only a slug constraint, future uniqueness constraints (e.g., on SKU) could trigger the same error code and be misreported as slug conflicts.
PostgreSQL errors include a constraint field in the error object, which should be checked. A safer approach:
Suggested fix
- if ((error as { code?: string }).code === '23505') {
- throw new SlugConflictError('Slug already exists.');
- }
+ const e = error as any;
+ if (e?.code === '23505' && typeof e?.constraint === 'string' && e.constraint.includes('slug')) {
+ throw new SlugConflictError('Slug already exists.');
+ }
throw error;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if ((error as { code?: string }).code === '23505') { | |
| throw new SlugConflictError('Slug already exists.'); | |
| } | |
| throw error; | |
| const e = error as any; | |
| if (e?.code === '23505' && typeof e?.constraint === 'string' && e.constraint.includes('slug')) { | |
| throw new SlugConflictError('Slug already exists.'); | |
| } | |
| throw error; |
🤖 Prompt for AI Agents
In @frontend/lib/services/products/mutations/update.ts around lines 206 - 209,
The catch block currently maps any Postgres unique-violation (code '23505') to
SlugConflictError; change the conditional to also verify the violated constraint
name (check (error as { code?: string; constraint?: string }).constraint)
matches your products.slug uniqueness constraint (or contains 'slug') before
throwing SlugConflictError; otherwise rethrow the original error. Update the
code around the existing check that references '23505' and SlugConflictError so
it only throws SlugConflictError when both code === '23505' and constraint ===
'<your_slug_constraint_name>' (or constraint.includes('slug')).
| describe('Task 6: Tripwire — no direct orders.paymentStatus writers outside allowlist', () => { | ||
| it('fails if any file writes orders.paymentStatus via direct .set/.values outside allowlist', () => { | ||
| const files: string[] = []; | ||
| walk(REPO_ROOT, '', files); | ||
|
|
||
| const offenders: string[] = []; | ||
|
|
||
| for (const rel of files) { | ||
| // Skip tests entirely (they contain regex/fixtures that look like writers) | ||
| if (rel.startsWith('lib/tests/')) continue; | ||
| if (rel.endsWith('.test.ts') || rel.endsWith('.test.tsx')) continue; | ||
| if (rel.endsWith('.spec.ts') || rel.endsWith('.spec.tsx')) continue; | ||
|
|
||
| const abs = path.join(REPO_ROOT, rel); | ||
| const src = fs.readFileSync(abs, 'utf8'); | ||
|
|
||
| const allowed = getAllowed(rel); | ||
| if (hasDirectOrdersWriter(src, allowed)) offenders.push(rel); | ||
| } | ||
|
|
||
| expect(offenders).toEqual([]); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n frontend/lib/tests/payment-status-tripwire.test.ts | head -100Repository: DevLoversTeam/devlovers.net
Length of output: 3880
🏁 Script executed:
# Find the REPO_ROOT definition and WRITER_RULES in the test file
rg -n "REPO_ROOT|WRITER_RULES" frontend/lib/tests/payment-status-tripwire.test.ts -B 2 -A 2Repository: DevLoversTeam/devlovers.net
Length of output: 991
🏁 Script executed:
# Check if the mentioned allowlisted files exist
fd -t f "payment-state.ts" frontend/ 2>/dev/null
fd -t f "checkout.ts" frontend/ 2>/dev/nullRepository: DevLoversTeam/devlovers.net
Length of output: 193
🏁 Script executed:
# Look at the walk function and getAllowed to understand path resolution
rg -n "function walk|function getAllowed|const walk|const getAllowed" frontend/lib/tests/payment-status-tripwire.test.ts -A 15Repository: DevLoversTeam/devlovers.net
Length of output: 744
🏁 Script executed:
# Find package.json files to see test configuration
fd -t f "package.json" frontend/ | head -5Repository: DevLoversTeam/devlovers.net
Length of output: 92
🏁 Script executed:
# Check the test script in frontend package.json
cat -n frontend/package.json | grep -A 5 -B 5 '"test"'Repository: DevLoversTeam/devlovers.net
Length of output: 53
🏁 Script executed:
# Check for vitest config
fd -t f "vitest.config.*" frontend/Repository: DevLoversTeam/devlovers.net
Length of output: 96
🏁 Script executed:
# Check project root package.json for workspace/monorepo setup
cat -n package.json | head -50Repository: DevLoversTeam/devlovers.net
Length of output: 115
🏁 Script executed:
# Verify the actual paths that walk() would collect
# by checking what directory structure exists
fd -t d "frontend" --maxdepth 1
fd -t d "lib" frontend/ --maxdepth 1Repository: DevLoversTeam/devlovers.net
Length of output: 94
🏁 Script executed:
# Check vitest config to understand test working directory
cat -n frontend/vitest.config.tsRepository: DevLoversTeam/devlovers.net
Length of output: 636
🏁 Script executed:
# Look at the root of the repo to understand monorepo structure
ls -la | head -20Repository: DevLoversTeam/devlovers.net
Length of output: 1089
🏁 Script executed:
# Check frontend package.json for test scripts
cat frontend/package.json | grep -A 10 '"test"'Repository: DevLoversTeam/devlovers.net
Length of output: 53
🏁 Script executed:
# Check if there's a root package.json with test scripts
cat -n package.json 2>/dev/null | grep -A 10 '"test"' || echo "No root package.json with test script"Repository: DevLoversTeam/devlovers.net
Length of output: 108
🏁 Script executed:
# Verify the walk function behavior by checking what paths it would generate
# starting from different potential REPO_ROOT values
python3 << 'EOF'
import os
import sys
# Simulating path generation from walk function
def norm(p):
return p.replace('\\', '/')
# If REPO_ROOT = frontend directory
repo_root = 'frontend'
rel_base = ''
entry_name = 'lib'
abs_path = os.path.join(repo_root, rel_base, entry_name)
rel_path = norm(os.path.join(rel_base, entry_name))
print(f"If REPO_ROOT=frontend: rel_path would start with: {rel_path}")
# If REPO_ROOT = repo root
repo_root = '.'
rel_base = ''
entry_name = 'frontend'
abs_path = os.path.join(repo_root, rel_base, entry_name)
rel_path = norm(os.path.join(rel_base, entry_name))
print(f"If REPO_ROOT=repo_root: rel_path would start with: {rel_path}")
# WRITER_RULES paths
writer_rules_path = 'lib/services/orders/payment-state.ts'
print(f"\nWRITER_RULES key: {writer_rules_path}")
EOFRepository: DevLoversTeam/devlovers.net
Length of output: 157
🏁 Script executed:
# Look at the frontend package.json to see test command
cat frontend/package.jsonRepository: DevLoversTeam/devlovers.net
Length of output: 2231
🏁 Script executed:
# Check if there's a README documenting how to run tests
head -50 README.mdRepository: DevLoversTeam/devlovers.net
Length of output: 669
Tripwire test design is sound, but verify working directory assumption.
Skipping test files prevents false positives from regex patterns and fixtures. The assertion expect(offenders).toEqual([]) will provide clear output showing which files violate the rule.
However, the test has a critical path mismatch issue: REPO_ROOT = process.cwd() combined with WRITER_RULES keys that are relative to the frontend directory (e.g., lib/services/orders/payment-state.ts without frontend/ prefix) means the test will fail to match allowlisted files if invoked from the repository root. The walk() function would generate paths like frontend/lib/... that won't match the allowlist keys. Ensure this test is always run from the frontend directory, or prepend frontend/ to all WRITER_RULES keys to match the actual walk output.
🤖 Prompt for AI Agents
In @frontend/lib/tests/payment-status-tripwire.test.ts around lines 443 - 464,
The test assumes REPO_ROOT uses the frontend working dir but currently uses
process.cwd(), causing path mismatches between walk() output (e.g.,
"frontend/lib/...") and WRITER_RULES keys (e.g., "lib/services/..."); fix by
normalizing paths in getAllowed/walk or adjusting REPO_ROOT: set REPO_ROOT =
path.join(process.cwd(), 'frontend') (or alternatively prepend "frontend/" to
every key in WRITER_RULES or strip a leading "frontend/" from rel before lookup)
so getAllowed(rel) receives matching relative paths and the allowlist logic
works as intended.
…self-heal on PRICE_CONFIG_ERROR (no crash, no console)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
frontend/app/api/shop/cart/rehydrate/route.ts (1)
79-92: Consider sanitizingrawValuebefore returning in error response.Exposing
rawValuedirectly could leak sensitive or unexpected data from the database to the client. Consider sanitizing or omitting this field in production responses.🔒 Suggested sanitization
if (error instanceof MoneyValueError) { return jsonError( 500, 'PRICE_CONFIG_ERROR', 'Invalid price configuration for one or more products.', { productId: error.productId, field: error.field, - rawValue: error.rawValue, + // Avoid exposing raw DB values to client; log server-side instead } ); }frontend/components/shop/cart-provider.tsx (1)
90-121: Use type guard instead ofas anyfor safer details access.The
(info.details as any)?.productIdpattern bypasses type checking. Consider a type guard for cleaner, safer access.♻️ Suggested improvement
+ const detailsProductId = + info.details && + typeof info.details === 'object' && + 'productId' in info.details && + typeof (info.details as { productId?: unknown }).productId === 'string' + ? (info.details as { productId: string }).productId + : ''; + - const productId = - typeof (info.details as any)?.productId === 'string' - ? String((info.details as any).productId) - : ''; + const productId = detailsProductId;Alternatively, extend
getErrorInfoto return a typeddetailsobject.frontend/lib/cart.ts (1)
222-228: Consider logging JSON parse failures for debugging.
readJsonSafesilently swallows parse errors. While this is intentional for resilience, logging the failure could help diagnose malformed API responses.📝 Suggested enhancement
async function readJsonSafe(response: Response): Promise<unknown> { try { return await response.json(); - } catch { + } catch (err) { + logWarn('json_parse_failed', { + status: response.status, + url: response.url, + message: err instanceof Error ? err.message : String(err), + }); return null; } }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/app/api/shop/cart/rehydrate/route.tsfrontend/components/shop/cart-provider.tsxfrontend/lib/cart.ts
🧰 Additional context used
🧬 Code graph analysis (3)
frontend/lib/cart.ts (2)
frontend/lib/logging.ts (1)
logWarn(95-97)frontend/lib/validation/shop.ts (2)
CartRehydrateItem(436-436)CartClientItem(435-435)
frontend/app/api/shop/cart/rehydrate/route.ts (6)
frontend/lib/validation/shop.ts (1)
cartRehydratePayloadSchema(326-341)frontend/lib/shop/request-locale.ts (1)
resolveLocaleAndCurrency(37-44)frontend/lib/services/products.ts (1)
rehydrateCartItems(16-16)frontend/lib/logging.ts (1)
logError(87-93)frontend/lib/services/errors.ts (2)
PriceConfigError(71-84)InvalidPayloadError(37-43)frontend/db/queries/shop/orders.ts (1)
MoneyValueError(10-22)
frontend/components/shop/cart-provider.tsx (2)
frontend/lib/cart.ts (7)
CartRehydrateError(203-220)Cart(18-18)emptyCart(23-32)CartClientItem(21-21)persistCartItems(97-107)rehydrateCart(230-259)clearStoredCart(269-273)frontend/lib/logging.ts (1)
logWarn(95-97)
🔇 Additional comments (9)
frontend/app/api/shop/cart/rehydrate/route.ts (2)
32-42: LGTM! Clean helper for standardized error responses.The
jsonErrorhelper centralizes JSON error response formatting with consistent structure.
68-77: LGTM! Proper error classification and logging.
PriceConfigErrorcorrectly returns 400 (client-facing contract error) with relevant diagnostic details, and errors are logged vialogErrorbefore handling.frontend/components/shop/cart-provider.tsx (2)
58-74: LGTM! Defensive error normalization.The helper safely extracts error information with sensible defaults when properties are missing or malformed.
96-133: Solid self-healing pattern with appropriate logging.The retry-once approach with fallback to cart clearing is well-designed:
- Removes only the problematic product when identifiable
- Single retry prevents infinite loops
- Clear fallback (empty cart) ensures users aren't stuck
- Comprehensive logging for debugging
One consideration: if
filtered.length === 0after removal, the code still attemptsrehydrateCart([]), which returnsemptyCartper the implementation incart.ts. This is correct behavior.frontend/lib/cart.ts (5)
89-94: LGTM! Structured logging replaces console.warn.Consistent with PR objectives to use a structured logging facade. The error message extraction pattern is appropriately defensive.
Also applies to: 102-106
157-159: LGTM! Standard type guard for record checking.Clean implementation that correctly excludes arrays from the record type.
161-201: Well-designed multi-format error extraction with good backward compatibility.Handles three error response shapes (legacy string, nested object, flat object) gracefully. The trim checks prevent empty strings from being accepted as valid codes/messages.
203-220: LGTM! Clean error class with immutable properties.Using
readonlyon public properties is good practice for error classes.
242-252: LGTM! Robust error handling with fallback message.The fallback message including HTTP status provides useful context when
apiErr.messageis empty.
2 participants
Description
This PR refactors the shop service layer (products + orders) and hardens critical money/stock/payment flows.
It standardizes paymentStatus transitions under a guarded state machine, enforces contract-level validation for SALE pricing and checkout variants (size/color), tightens refund handling to MVP scope (full refunds only), and unifies structured logging + env documentation for production readiness.
Related Issue
Issue: #<issue_number>
Changes
services/productsandservices/ordersinto smaller, focused modules to reduce file size and improve maintainability.charge.refunded/ “full”refund.updated) and marks orders accordingly.amount_refunded+ empty refunds list to prevent webhook misclassification.badge=SALE, enforcesoriginalPriceMinorpresence andoriginalPriceMinor > priceMinoracross existing currency rows.SALE_ORIGINAL_REQUIREDwith stabledetails.rule(required/greater_than_price).400 INVALID_VARIANT.guardedPaymentStatusUpdatewith an explicit transition matrix per provider.docs/env.md+ updated.env.example.console.*usage with a single structured logging facade (lib/logging.ts).Database Changes (if applicable)
How Has This Been Tested?
Commands run (PowerShell):
npx vitest runnpx vitest run .\lib\tests\admin-product-sale-contract.test.tsnpm run buildScreenshots (if applicable)
Checklist
Before submitting
lib/logging.ts)Reviewers
Summary by CodeRabbit
New Features
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.