fix: prevent DB blowups in pricing (USD mirror + product_prices) and enforce PRICE_CONFIG_ERROR, stabilize product pricing validation, multi-currency storage, an…#101
Conversation
…d cart rehydrate price contract
liudmylasovetovs
requested review from
AM1007 and
ViktorSvertoka
as code owners
✅ Deploy Preview for develop-devlovers ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Warning Rate limit exceeded@liudmylasovetovs has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 6 minutes and 35 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughProduct pricing migrated from string major-units to numeric minor-units across services and DB; the product page now forces runtime dynamic rendering; admin PATCH no longer parses or applies price overrides; DB schema gains strict cross-field integrity checks for prices and orders. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant AdminUI as Admin UI
participant AdminAPI as Admin API Route
participant ProductSvc as Product Service
participant DB as Database
Note over AdminUI,AdminAPI: PATCH /api/shop/admin/products/[id]
AdminUI->>AdminAPI: submit formData (may include prices JSON)
AdminAPI->>AdminAPI: parse base admin fields (prices override parsing removed)
AdminAPI->>ProductSvc: updateProduct(parsed.data, images)
ProductSvc->>ProductSvc: normalize/validate priceMinor fields
ProductSvc->>DB: UPDATE product, upsert productPrices with priceMinor/originalPriceMinor
DB-->>ProductSvc: 200 OK
ProductSvc-->>AdminAPI: updated product
AdminAPI-->>AdminUI: 200 OK
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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: 0
🧹 Nitpick comments (4)
frontend/lib/services/products.ts (1)
742-751: Consider simplifying the unit price extraction.The conditional logic with type assertion
(product as any).priceMinoris a defensive pattern, but thepriceMinorfield is already selected in the query (line 690). The fallback tocoercePriceFromDbshould only trigger for legacy data.🔎 Suggested simplification
const unitPriceCents = - typeof (product as any).priceMinor === 'number' && - Number.isFinite((product as any).priceMinor) - ? Math.trunc((product as any).priceMinor) + typeof product.priceMinor === 'number' && + Number.isFinite(product.priceMinor) + ? Math.trunc(product.priceMinor) : toCents( coercePriceFromDb(product.price, { field: 'price', productId: product.id, }) );frontend/app/api/shop/admin/products/[id]/route.ts (2)
22-80: Remove commented-out code.Large blocks of commented-out code (schemas and helper functions) should be deleted rather than kept. If needed later, it can be recovered from version control.
🔎 Proposed cleanup
-// const adminCurrencySchema = z -// .string() -// .transform(v => v.trim().toUpperCase()) -// .pipe(z.enum(['USD', 'UAH'])); - -// function parseMajorToMinor(value: string | number): number { -// const s = String(value).trim().replace(',', '.'); -// -// if (!/^\d+(\.\d{1,2})?$/.test(s)) { -// throw new Error(`Invalid money value: "${value}"`); -// } -// const [whole, frac = ''] = s.split('.'); -// const frac2 = (frac + '00').slice(0, 2); -// const minor = Number(whole) * 100 + Number(frac2); -// if (!Number.isSafeInteger(minor) || minor < 0) { -// throw new Error(`Invalid money value: "${value}"`); -// } -// return minor; -// } - -// const minorRowSchema = z.object({ -// currency: adminCurrencySchema, -// priceMinor: z.preprocess( -// v => (typeof v === 'string' ? Number(v) : v), -// z.number().int().nonnegative() -// ), -// originalPriceMinor: z.preprocess(v => { -// if (v === undefined) return undefined; -// if (v === null) return null; -// return typeof v === 'string' ? Number(v) : v; -// }, z.number().int().nonnegative().nullable().optional()), -// }); - -// const legacyRowSchema = z.object({ -// currency: adminCurrencySchema, -// price: z.preprocess(v => String(v).trim(), z.string().min(1)), -// originalPrice: z.preprocess(v => { -// if (v === undefined) return undefined; -// if (v === null) return null; -// const s = String(v).trim(); -// return s.length ? s : null; -// }, z.string().nullable().optional()), -// }); - -// const adminPriceRowSchema = z -// .union([minorRowSchema, legacyRowSchema]) -// .transform(row => { -// if ('priceMinor' in row) { -// return row; -// } -// return { -// currency: row.currency, -// priceMinor: parseMajorToMinor(row.price), -// originalPriceMinor: -// row.originalPrice == null ? null : parseMajorToMinor(row.originalPrice), -// }; -// }); - -// const adminPricesSchema = z.array(adminPriceRowSchema);
156-162: Type assertionas anybypasses type safety.The
parsed.data as anycast suggests a type mismatch betweenparseAdminProductFormoutput andupdateProductinput. Consider aligning the types or adding a proper type guard.frontend/lib/admin/parseAdminProductForm.ts (1)
246-246: Minor: Remove extra blank line.🔎 Proposed fix
- export function parseAdminProductForm(
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
frontend/app/[locale]/shop/products/[slug]/page.tsxfrontend/app/api/shop/admin/products/[id]/route.tsfrontend/db/schema/shop.tsfrontend/lib/admin/parseAdminProductForm.tsfrontend/lib/services/products.ts
🧰 Additional context used
🧬 Code graph analysis (3)
frontend/app/[locale]/shop/products/[slug]/page.tsx (4)
frontend/app/[locale]/shop/admin/orders/[id]/page.tsx (1)
dynamic(9-9)frontend/app/[locale]/shop/admin/orders/page.tsx (1)
dynamic(7-7)frontend/app/[locale]/layout.tsx (1)
dynamic(15-15)frontend/app/api/shop/orders/[id]/route.ts (1)
dynamic(12-12)
frontend/lib/admin/parseAdminProductForm.ts (1)
frontend/drizzle/schema.ts (1)
currency(7-7)
frontend/lib/services/products.ts (5)
frontend/lib/shop/currency.ts (2)
CurrencyCode(2-2)currencyValues(1-1)frontend/lib/validation/shop.ts (1)
CurrencyCode(32-32)frontend/lib/services/errors.ts (1)
InvalidPayloadError(16-22)frontend/drizzle/schema.ts (1)
currency(7-7)frontend/lib/shop/money.ts (1)
toDbMoney(58-60)
🔇 Additional comments (21)
frontend/lib/services/products.ts (9)
53-57: LGTM!The
NormalizedPriceRowtype correctly represents the canonical minor-unit pricing model withpriceMinoras required integer andoriginalPriceMinoras optional.
138-150: LGTM!The
assertMoneyMinorInthelper properly validates minor units by:
- Coercing string/number inputs
- Checking for finite numbers
- Truncating to integer and validating it's a safe positive integer (≥1)
188-269: Transitional price normalization logic looks solid.The function handles three input paths correctly:
- NEW:
prices[].priceMinor/originalPriceMinor(minor units)- LEGACY:
prices[].price/originalPrice(major strings → converted viatoMoneyMinor)- VERY LEGACY: top-level
price/originalPrice/currencyCurrency validation and duplicate prevention are handled downstream in
validatePriceRows.
271-277: LGTM!The
requireUsdfunction correctly enforces the USD price requirement by checking for both existence and a truthypriceMinorvalue.
279-314: LGTM!The
validatePriceRowsfunction properly enforces:
- No duplicate currencies via
Set- Currency membership in
currencyValuespriceMinormust be a positive safe integeroriginalPriceMinor(when present) must be a safe integer and greater thanpriceMinor
339-414: LGTM!The
createProductfunction correctly:
- Persists USD legacy mirror using
toDbMoney(usd.priceMinor)andtoDbMoney(usd.originalPriceMinor)- Stores canonical
priceMinor/originalPriceMinorinproductPricesalong with legacy mirror columnsThis aligns with the schema constraints requiring mirror consistency.
480-490: LGTM!The USD legacy mirror update correctly uses the new minor-unit fields (
usd.priceMinor,usd.originalPriceMinor) withtoDbMoneyconversion.
504-542: LGTM!The upsert logic for
productPricescorrectly:
- Stores both canonical minor units (
priceMinor,originalPriceMinor)- Maintains legacy mirror columns via
toDbMoneyconversion- Uses update-then-insert pattern to handle existing/new currency rows
728-734: Critical pricing contract enforcement is correctly implemented.The
PriceConfigErroris thrown whenproduct_pricesis missing for the resolved currency, enforcing the no-fallback contract as specified in the PR objectives.frontend/app/[locale]/shop/products/[slug]/page.tsx (2)
8-8: LGTM!Import cleanup is correct—only
getProductPageDatais needed for runtime data fetching.
13-13: Verify the performance impact of switching to dynamic rendering.The change from static generation to
force-dynamicensures fresh pricing data per request, which aligns with the multi-currency pricing requirements. However, this removes the caching benefits of SSG.Ensure this is intentional for product pages, as it may increase server load and TTFB. The pattern is consistent with other pages in the codebase (admin/orders, layout).
frontend/db/schema/shop.ts (4)
75-80: LGTM!The products table constraints correctly enforce:
currency = 'USD'for the legacy mirror fieldprice > 0(positive price requirement)originalPrice IS NULL OR originalPrice > price(discount invariant)These align with the application-level validations in
products.ts.
116-152: LGTM!The orders table changes correctly:
pspMetadatais nownotNullwith default'{}'::jsonb- Mirror consistency check:
totalAmount = totalAmountMinor / 100- Payment status constraint: when
paymentProvider = 'none', status must be'paid'
196-203: LGTM!The orderItems mirror consistency checks correctly enforce:
unitPrice = unitPriceMinor / 100lineTotal = lineTotalMinor / 100
262-273: LGTM!The productPrices constraints correctly enforce:
- Mirror consistency:
price = priceMinor / 100- Null coupling:
originalPriceMinor IS NULL ↔ originalPrice IS NULL- Original price mirror:
originalPrice = originalPriceMinor / 100when not nullThese database-level constraints provide defense-in-depth for the minor-unit pricing model.
frontend/lib/admin/parseAdminProductForm.ts (6)
115-133: LGTM!The
parseMinorInthelper correctly validates:
- Accepts number or numeric string
- Must be finite, integer, and non-negative (≥0)
- Returns
nullfor null/undefined inputs
135-141: LGTM!The
requirePositivePriceMinorhelper enforces the DB constraintpriceMinor > 0for create mode.
148-154: LGTM!Improved error handling returns a structured error object instead of throwing, allowing consistent error propagation.
185-206: LGTM!The canonical minor-unit parsing with legacy fallback is correctly implemented:
- Try
priceMinor(canonical)- Fall back to
price(legacy major → minor conversion)- In create mode, enforce positive price
- In update mode, allow null but reject non-positive values if present
208-230: LGTM!The
originalPriceMinorhandling correctly:
- Prefers canonical
originalPriceMinor- Falls back to legacy
originalPriceconversion- Normalizes empty to
null- Enforces DB invariant:
originalPriceMinor > priceMinorwhen both present
240-243: LGTM!Error handling correctly propagates
ZodErrorinstances and wraps other errors in a structured format.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const n = typeof value === 'number' ? value : Number(value); | ||
| if (!Number.isFinite(n)) { | ||
| throw new InvalidPayloadError(`${field} must be a number.`); | ||
| } | ||
| const i = Math.trunc(n); |
There was a problem hiding this comment.
When a client sends minor-unit pricing, assertMoneyMinorInt truncates fractional values via Math.trunc, so a payload like priceMinor: 199.99 becomes 199 rather than being rejected. This silently undercharges and can desync totals (e.g., cart totals vs. admin intent) if any caller sends non-integer minor units. Consider validating Number.isInteger(n) and rejecting non-integer inputs so bad payloads fail instead of being rounded down.
Useful? React with 👍 / 👎.
…ation to products.ts
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
frontend/lib/services/products.ts (1)
138-150: Fractional minor units are truncated rather than rejected.As previously noted,
Math.truncon line 143 silently rounds down non-integer inputs (e.g.,199.99becomes199), which can cause pricing discrepancies between admin intent and stored values. Consider addingNumber.isInteger(n)check before truncation and rejecting non-integer inputs.
🧹 Nitpick comments (1)
frontend/lib/services/products.ts (1)
742-751: Consider rejecting fractional priceMinor from DB instead of truncating.Line 745 uses
Math.trunc(product.priceMinor)which silently rounds down fractional values. While the DB schema should prevent non-integer minor units, truncating here could mask data integrity issues. Consider validatingNumber.isInteger(product.priceMinor)and throwing an error if fractional values are found.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/app/api/shop/admin/products/[id]/route.tsfrontend/lib/admin/parseAdminProductForm.tsfrontend/lib/services/products.ts
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/lib/admin/parseAdminProductForm.ts (1)
frontend/drizzle/schema.ts (1)
currency(7-7)
🔇 Additional comments (9)
frontend/lib/services/products.ts (4)
188-269: LGTM: Transitional input handling is well-structured.The three-tier fallback (NEW minor units → LEGACY major strings → top-level legacy) correctly supports the migration path while maintaining backward compatibility. Currency validation and format handling are comprehensive.
279-314: LGTM: Comprehensive runtime validation.The validation correctly enforces all invariants: unique currencies, valid currency codes, positive integer minor units, and originalPriceMinor > priceMinor. The runtime guards are appropriate for transitional input that may bypass TypeScript/Zod validation.
316-414: LGTM: Create flow correctly uses minor units.The transaction properly maintains both the USD legacy mirror (lines 351-355) and canonical multi-currency prices (lines 375-394) using minor-unit values. Validation sequence is correct.
416-574: LGTM: Update flow correctly uses minor units with PATCH semantics.The partial-update logic appropriately preserves existing prices when not provided (lines 473-490) and upserts only the currencies included in the request (lines 504-542). Minor-unit handling is consistent.
frontend/app/api/shop/admin/products/[id]/route.ts (1)
64-165: LGTM: Simplified PATCH handler delegates price normalization upstream.Removing inline price parsing/validation from the route and relying on
parseAdminProductFormimproves separation of concerns. The direct spread ofparsed.dataat line 97 is appropriate since validation is complete at that point.frontend/lib/admin/parseAdminProductForm.ts (4)
115-133: LGTM: parseMinorInt correctly validates non-negative integers.The function appropriately validates that the value is a finite integer >= 0. The acceptance of 0 is fine here since
requirePositivePriceMinorand the update-mode validation (lines 199-206) enforce the > 0 constraint where needed.
135-141: LGTM: requirePositivePriceMinor correctly enforces positive price constraint.The validation appropriately rejects null or non-positive values, matching the DB constraint.
143-244: LGTM: parsePricesJsonField handles canonical and legacy pricing correctly.The function properly supports both minor-unit canonical payloads and legacy major-string fallbacks (lines 186-197, 208-218), validates the
originalPriceMinor > priceMinorinvariant (lines 224-230), and correctly differentiates create vs. update validation (lines 199-206). The validation at lines 224-230 appropriately checks the invariant only when both values are present in the payload, as DB constraints and downstream validation will catch any issues with partial updates.
246-330: LGTM: parseAdminProductForm correctly prioritizes canonical prices with legacy fallback.The function appropriately prefers the canonical
pricesJSON payload (line 263), falls back to legacypriceUsd/priceUahfields (lines 269-294), and correctly implements PATCH semantics for update mode (lines 297-302). The final payload construction (lines 304-318) conditionally includes prices only when available.
…nor as safe integer in cart rehydrate
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/lib/services/products.ts (1)
212-230: MissingoriginalPriceMinor > priceMinorinvariant check in the NEW path.The LEGACY path enforces
originalPrice > priceviatoMoneyMinorNullableduring normalization, but the NEW path (lines 218-227) only validates thatoriginalPriceMinoris a valid integer without checking the invariant. WhilevalidatePriceRows(line 314) catches this later, enforcing it here would provide earlier, more contextual error messages and maintain consistency between paths.🔎 Proposed fix
// NEW path: minor units if (p?.priceMinor != null) { const priceMinor = assertMoneyMinorInt( p.priceMinor, `${currency} price` ); const originalPriceMinor = p.originalPriceMinor == null ? null : (() => { const v = assertMoneyMinorInt( p.originalPriceMinor, `${currency} originalPrice` ); + if (v <= priceMinor) { + throw new InvalidPayloadError( + `${currency} originalPrice must be > price.` + ); + } return v; })(); return { currency, priceMinor, originalPriceMinor }; }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/lib/services/products.ts
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/lib/services/products.ts (5)
frontend/lib/shop/currency.ts (2)
CurrencyCode(2-2)currencyValues(1-1)frontend/lib/services/errors.ts (2)
InvalidPayloadError(16-22)PriceConfigError(24-34)frontend/drizzle/schema.ts (1)
currency(7-7)frontend/lib/shop/money.ts (2)
toDbMoney(58-60)toCents(38-42)frontend/db/queries/shop/orders.ts (1)
coercePriceFromDb(35-53)
🔇 Additional comments (7)
frontend/lib/services/products.ts (7)
53-57: LGTM!The
NormalizedPriceRowtype correctly models minor-unit pricing withpriceMinoras a required number andoriginalPriceMinoras nullable for optional sale pricing.
138-157: LGTM! Fractional minor-unit rejection implemented correctly.The validation now properly rejects non-integer minor units via
Number.isInteger(n)check (line 146) instead of truncating, which addresses the previous review feedback. The validation order is correct: finite → integer → safe integer and positive.
278-284: LGTM!The falsy check
!usd?.priceMinoris safe here becausevalidatePriceRowsis called beforerequireUsdincreateProduct, ensuringpriceMinor >= 1.
286-321: LGTM!Comprehensive validation covering all pricing invariants: duplicate currencies, valid currency codes, positive safe integer for
priceMinor, and theoriginalPriceMinor > priceMinorconstraint. The runtime guards provide defense-in-depth against bypassed upstream validation.
358-401: LGTM!The transaction correctly inserts both the canonical
priceMinor/originalPriceMinorvalues and the legacytoDbMoneyrepresentations, maintaining backward compatibility while supporting the new minor-unit model.
489-548: LGTM!The upsert pattern correctly handles partial price updates, updating only the currencies provided in the request. The dual-write to both canonical (
priceMinor) and legacy (price) columns maintains backward compatibility.
749-789: LGTM! Robust DB validation with defense-in-depth.The cart rehydration now enforces strict integer validation on
priceMinorfrom the database, throwingPriceConfigErrorwith full context (productId,currency) for invalid data. The fallback to the legacypricecolumn (lines 773-781) provides transitional compatibility for products not yet migrated to the canonicalpriceMinorfield. The final safety check (lines 784-789) ensures consistency regardless of the source.Consider adding a TODO or logging when the fallback path is taken to track migration progress:
} else { // Fallback to legacy money column (string/decimal), still validated via coercePriceFromDb + // TODO: Remove fallback after full migration to priceMinor unitPriceCents = toCents( coercePriceFromDb(product.price, { field: 'price', productId: product.id, }) ); }
fix: prevent DB blowups in pricing (USD mirror + product_prices) and enforce PRICE_CONFIG_ERROR, stabilize product pricing validation, multi-currency storage, an…
2 participants
Description
This PR fixes production-blocking issues in shop product pricing and cart rehydrate behavior.
Key outcomes:
frontend/lib/services/products.ts(TS error:Cannot find name 'p').prices[]payloads in minor units (priceMinor,originalPriceMinor) and legacy major string prices (price,originalPrice), normalizing to minor units.originalPrice > pricewhen present).products.price/originalPrice/currencyto satisfy existing constraints and keep backward compatibility, while storing source-of-truth prices inproduct_prices.product_pricesonly for currencies provided in the request.product_pricesis missing for the resolved currency.422 PRICE_CONFIG_ERRORwith{ productId, currency }details.Related Issue
Issue: #<issue_number>
Changes
frontend/lib/services/products.tsto remove invalidpreference and correctly validate per-price rows.product_pricesas canonical store and keepsproducts.*as USD legacy mirror.product_pricesfor the resolved currency and returnPRICE_CONFIG_ERRORwhen missing (no implicit conversion/fallbacks).Database Changes (if applicable)
Notes:
product_pricesis the canonical price source for cart rehydrate; missing currency config is treated as a contract error (422).How Has This Been Tested?
Manual checks:
product_prices.product_pricescorrectly; legacy USD mirror remained consistent.Accept-Language: ukreturns items inUAHwhen UAH price exists.Accept-Language: enreturns items inUSDwhen USD price exists.422 {"code":"PRICE_CONFIG_ERROR","message":"Price not configured for currency.","details":{"productId":"...","currency":"..."}}Screenshots (if applicable)
N/A (API/service changes)
Checklist
Before submitting
Reviewers
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.