(SP: 3) [SHOP] fix launch scope decisions and harden checkout totals fail-closed#414
Merged
ViktorSvertoka merged 5 commits intodevelopfrom Mar 24, 2026
Merged
(SP: 3) [SHOP] fix launch scope decisions and harden checkout totals fail-closed#414ViktorSvertoka merged 5 commits intodevelopfrom
ViktorSvertoka merged 5 commits intodevelopfrom
Conversation
liudmylasovetovs
requested review from
AM1007,
LesiaUKR and
ViktorSvertoka
as code owners
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Contributor
📝 WalkthroughWalkthroughAdds deterministic pricing and shipping quote fingerprinting and strict fail-closed validation to checkout; introduces authoritative shipping quote resolution, pricing fingerprint generation/validation, new error codes and tests, updates cart/checkout payloads, and adds related documentation and tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client (Browser)
participant CartService as Cart Service (rehydrateCartItems)
participant ShippingAPI as Shipping Methods API
participant CheckoutAPI as Checkout API (/api/shop/checkout)
participant OrderService as Order Service (createOrderWithItems)
participant DB as Database (orders)
Note over Client,DB: Authoritative pricing & shipping fingerprint flow
Client->>CartService: rehydrateCartItems(items, currency)
CartService-->>Client: CartSummary { pricingFingerprint }
Client->>ShippingAPI: GET /api/shop/shipping/methods?currency=UAH
ShippingAPI->>ShippingAPI: resolveCheckoutShippingQuote(env)
ShippingAPI-->>Client: methods[] with amountMinor & quoteFingerprint
Client->>CheckoutAPI: POST /api/shop/checkout { pricingFingerprint, shippingQuoteFingerprint, ... }
CheckoutAPI->>CheckoutAPI: early-check unsupported discount fields -> 400 (if any)
CheckoutAPI->>OrderService: createOrderWithItems(..., requirePricingFingerprint=true, requireShippingQuoteFingerprint=true)
OrderService->>OrderService: recompute pricingFingerprint (authoritative)
OrderService->>OrderService: resolveCheckoutShippingQuote (authoritative)
OrderService->>OrderService: validate fingerprints -> 409 on mismatch
OrderService->>DB: INSERT order (itemsSubtotalMinor, shippingAmountMinor, totalAmountMinor)
DB-->>OrderService: order created
OrderService-->>CheckoutAPI: order
CheckoutAPI-->>Client: 201 { success:true, order }
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 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📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5422f34092
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/lib/cart.ts (1)
169-205:⚠️ Potential issue | 🟡 Minor
buildCartFromItemsis unused; remove it or document its purpose.This exported function (and
computeSummaryFromItemswhich it calls) are not referenced anywhere in the codebase. The actual pattern for cart initialization isrehydrateCart, which fetches the cart summary (includingpricingFingerprint) from the server API. IfbuildCartFromItemswere ever used, it would fail checkout since it hardcodespricingFingerprint: undefined, but since it has zero call sites, it appears to be dead code.Either remove both functions or add a comment explaining the intended use case (e.g., if this is a utility for external consumers or future use).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/cart.ts` around lines 169 - 205, The exported helper functions buildCartFromItems and computeSummaryFromItems appear to be dead/unsafe (no call sites and computeSummaryFromItems hardcodes pricingFingerprint: undefined which would break checkout); either remove both exports or clearly document their intended purpose and limitations (e.g., note they are offline/local-only helpers, warn about missing pricingFingerprint and mixed-currency checks) and/or update buildCartFromItems to populate pricingFingerprint correctly if you intend it to be used by app code; locate these by the function names buildCartFromItems and computeSummaryFromItems and either delete them or add a top-of-function comment and adjust the implementation to return a real pricingFingerprint or delegate to rehydrateCart when a server-validated summary is required.
🧹 Nitpick comments (4)
frontend/lib/services/shop/shipping/checkout-quote.ts (1)
43-54: Minor: Redundant negative check after digit-only regex.The check
parsed < 0on line 51 is unreachable since/^\d+$/only matches non-negative integer strings. This is harmless but could be simplified.♻️ Optional simplification
const parsed = Number.parseInt(trimmed, 10); - if (!Number.isSafeInteger(parsed) || parsed < 0) return null; + if (!Number.isSafeInteger(parsed)) return null;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/services/shop/shipping/checkout-quote.ts` around lines 43 - 54, The function readNonNegativeIntEnv contains an unnecessary runtime check for parsed < 0 because the /^\d+$/ regex already guarantees only non-negative digits; remove the redundant parsed < 0 condition from the safety checks (inside readNonNegativeIntEnv where parsed is validated with Number.isSafeInteger) so validation relies on the regex and Number.isSafeInteger only, keeping behavior identical but simplifying the logic.frontend/lib/tests/shop/checkout-price-change-fail-closed.test.ts (2)
312-314: Minor: RedundantString()conversion.The
typeof json.order?.id === 'string'check already confirms the value is a string, making theString()conversion redundant.✨ Simplify the conditional
- const orderId = - typeof json.order?.id === 'string' ? String(json.order.id) : null; + const orderId = + typeof json.order?.id === 'string' ? json.order.id : null;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/tests/shop/checkout-price-change-fail-closed.test.ts` around lines 312 - 314, The conditional converting json.order?.id to a string is redundant: when typeof json.order?.id === 'string' is true you can use the value directly. Change the assignment of orderId (the const orderId variable) to return json.order.id when the type check passes instead of wrapping it with String(), e.g. use json.order.id in the true branch (or cast if needed) and keep null otherwise so downstream expect(orderId).toBeTruthy() still receives the original string or null.
134-173: Potential price inconsistency betweenproducts.priceandproductPrices.priceMinor.The
products.pricefield is hardcoded to'9.00'(line 145) whileproductPrices.priceMinoruses the parameter. If a test passes a differentpriceMinorvalue, these would be inconsistent. Currently all tests use900(matching $9.00), so this works, but it could cause confusion for future test variations.✨ Proposed fix to derive product.price from priceMinor
.values({ slug, title: 'Checkout price change test product', description: null, imageUrl: 'https://example.com/checkout-price-change.png', imagePublicId: null, - price: '9.00', + price: (priceMinor / 100).toFixed(2), originalPrice: null, currency: 'USD',🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/tests/shop/checkout-price-change-fail-closed.test.ts` around lines 134 - 173, The products insertion in seedProduct hardcodes products.price to '9.00' which can diverge from the productPrices.priceMinor parameter; change the products insert in seedProduct to compute price from the priceMinor argument (e.g., set products.price = (priceMinor / 100).toFixed(2)) so products and productPrices remain consistent when inserting into products and productPrices; update any related fields (originalPrice if needed) in the same seedProduct function to use the same derived value.frontend/lib/tests/shop/checkout-shipping-authoritative-total.test.ts (1)
167-234: Minor: Type assertions (as any) bypass schema validation.The
as anyassertions on lines 191, 201, 211, and 223 bypass TypeScript's schema validation. While acceptable in tests, consider using proper typed inserts to catch schema drift during compilation. This is a low-priority improvement.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/tests/shop/checkout-shipping-authoritative-total.test.ts` around lines 167 - 234, The test helper seedShippingCheckoutData uses as any on db.insert calls which silences TypeScript schema checks; replace the unsafe assertions by importing/using the proper table row types or DTOs and pass strongly-typed objects to db.insert for products, productPrices, npCities, and npWarehouses (e.g., use ProductRow/ProductPriceRow/NPCCityRow/NPWWarehouseRow types or the ORM's Insert<T> types) so the insert calls in seedShippingCheckoutData remain type-safe and will catch schema drift at compile time while still populating createdProductIds, createdCityRefs, and createdWarehouseRefs as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/docs/shop/checkout-notifications-contract.md`:
- Around line 27-30: Remove the stray CMS/citation artifact
`:contentReference[oaicite:2]{index=2}` from the sentence "Notification flows
depend on a reliable recipient..." in checkout-notifications-contract.md so the
sentence reads cleanly (e.g., "If a guest order is created without email, the
notification pipeline can fail or dead-letter because there is no guaranteed
recipient identity."). Ensure no other `:contentReference[...]` tokens remain in
the document.
---
Outside diff comments:
In `@frontend/lib/cart.ts`:
- Around line 169-205: The exported helper functions buildCartFromItems and
computeSummaryFromItems appear to be dead/unsafe (no call sites and
computeSummaryFromItems hardcodes pricingFingerprint: undefined which would
break checkout); either remove both exports or clearly document their intended
purpose and limitations (e.g., note they are offline/local-only helpers, warn
about missing pricingFingerprint and mixed-currency checks) and/or update
buildCartFromItems to populate pricingFingerprint correctly if you intend it to
be used by app code; locate these by the function names buildCartFromItems and
computeSummaryFromItems and either delete them or add a top-of-function comment
and adjust the implementation to return a real pricingFingerprint or delegate to
rehydrateCart when a server-validated summary is required.
---
Nitpick comments:
In `@frontend/lib/services/shop/shipping/checkout-quote.ts`:
- Around line 43-54: The function readNonNegativeIntEnv contains an unnecessary
runtime check for parsed < 0 because the /^\d+$/ regex already guarantees only
non-negative digits; remove the redundant parsed < 0 condition from the safety
checks (inside readNonNegativeIntEnv where parsed is validated with
Number.isSafeInteger) so validation relies on the regex and Number.isSafeInteger
only, keeping behavior identical but simplifying the logic.
In `@frontend/lib/tests/shop/checkout-price-change-fail-closed.test.ts`:
- Around line 312-314: The conditional converting json.order?.id to a string is
redundant: when typeof json.order?.id === 'string' is true you can use the value
directly. Change the assignment of orderId (the const orderId variable) to
return json.order.id when the type check passes instead of wrapping it with
String(), e.g. use json.order.id in the true branch (or cast if needed) and keep
null otherwise so downstream expect(orderId).toBeTruthy() still receives the
original string or null.
- Around line 134-173: The products insertion in seedProduct hardcodes
products.price to '9.00' which can diverge from the productPrices.priceMinor
parameter; change the products insert in seedProduct to compute price from the
priceMinor argument (e.g., set products.price = (priceMinor / 100).toFixed(2))
so products and productPrices remain consistent when inserting into products and
productPrices; update any related fields (originalPrice if needed) in the same
seedProduct function to use the same derived value.
In `@frontend/lib/tests/shop/checkout-shipping-authoritative-total.test.ts`:
- Around line 167-234: The test helper seedShippingCheckoutData uses as any on
db.insert calls which silences TypeScript schema checks; replace the unsafe
assertions by importing/using the proper table row types or DTOs and pass
strongly-typed objects to db.insert for products, productPrices, npCities, and
npWarehouses (e.g., use ProductRow/ProductPriceRow/NPCCityRow/NPWWarehouseRow
types or the ORM's Insert<T> types) so the insert calls in
seedShippingCheckoutData remain type-safe and will catch schema drift at compile
time while still populating createdProductIds, createdCityRefs, and
createdWarehouseRefs as before.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f904de93-a79d-434a-99e9-d19f4155a12d
📒 Files selected for processing (21)
frontend/app/[locale]/shop/cart/CartPageClient.tsxfrontend/app/api/shop/checkout/route.tsfrontend/app/api/shop/shipping/methods/route.tsfrontend/docs/payments/monobank/E0-gap-report.mdfrontend/docs/payments/monobank/F0-report.mdfrontend/docs/shop/checkout-notifications-contract.mdfrontend/docs/shop/launch-scope-decisions.mdfrontend/docs/shop/legal-merchant-identity-content.mdfrontend/docs/shop/payments-runbook.mdfrontend/lib/cart.tsfrontend/lib/services/orders/checkout.tsfrontend/lib/services/products/cart/rehydrate.tsfrontend/lib/services/shop/shipping/checkout-quote.tsfrontend/lib/shop/checkout-pricing.tsfrontend/lib/tests/shop/cart-rehydrate-variant-sanitize.test.tsfrontend/lib/tests/shop/checkout-currency-policy.test.tsfrontend/lib/tests/shop/checkout-price-change-fail-closed.test.tsfrontend/lib/tests/shop/checkout-shipping-authoritative-total.test.tsfrontend/lib/tests/shop/checkout-shipping-phase3.test.tsfrontend/lib/tests/shop/shipping-methods-route-p2.test.tsfrontend/lib/validation/shop.ts
💤 Files with no reviewable changes (2)
- frontend/docs/payments/monobank/F0-report.md
- frontend/docs/payments/monobank/E0-gap-report.md
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
frontend/lib/tests/shop/checkout-shipping-authoritative-total.test.ts (1)
50-79: Note:resetEnvCache()called at mock factory time.Line 64 calls
resetEnvCache()during the mock factory execution (module load time). This works becausebeforeEachalso callsresetEnvCache()after stubbing env vars. However, this placement is slightly unusual—consider moving it tobeforeAllif the intent is test-time initialization rather than module-load-time.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/tests/shop/checkout-shipping-authoritative-total.test.ts` around lines 50 - 79, The call to resetEnvCache() is happening inside the vi.mock factory (module-load time) which is unusual; remove the resetEnvCache() invocation from the mock factory that defines ensureStripePaymentIntentForOrder and instead call resetEnvCache() in test runtime setup (e.g., in a beforeAll() or beforeEach() block depending on desired scope) so environment cache is reset during test initialization rather than during module import.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@frontend/lib/tests/shop/checkout-shipping-authoritative-total.test.ts`:
- Around line 50-79: The call to resetEnvCache() is happening inside the vi.mock
factory (module-load time) which is unusual; remove the resetEnvCache()
invocation from the mock factory that defines ensureStripePaymentIntentForOrder
and instead call resetEnvCache() in test runtime setup (e.g., in a beforeAll()
or beforeEach() block depending on desired scope) so environment cache is reset
during test initialization rather than during module import.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: dcc54f25-963c-4f96-9a00-f9e5956fc66c
📒 Files selected for processing (5)
frontend/docs/shop/checkout-notifications-contract.mdfrontend/lib/cart.tsfrontend/lib/services/shop/shipping/checkout-quote.tsfrontend/lib/tests/shop/checkout-price-change-fail-closed.test.tsfrontend/lib/tests/shop/checkout-shipping-authoritative-total.test.ts
✅ Files skipped from review due to trivial changes (3)
- frontend/docs/shop/checkout-notifications-contract.md
- frontend/lib/tests/shop/checkout-price-change-fail-closed.test.ts
- frontend/lib/services/shop/shipping/checkout-quote.ts
ViktorSvertoka
approved these changes
Mar 24, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR combines two launch-readiness blocks for the Shop module:
The goal is to remove ambiguity before launch and prevent stale or client-controlled monetary inputs from silently creating incorrect orders.
Related Issue
Issue: #<issue_number>
Changes
Database Changes (if applicable)
How Has This Been Tested?
Additional notes:
Screenshots (if applicable)
N/A — docs + backend / checkout contract hardening only.
Checklist
Before submitting
Reviewers
Summary by CodeRabbit
New Features
Documentation