(SP: 3) [SHOP] Launch readiness hardening after repeat audit #442
Merged
ViktorSvertoka merged 32 commits intodevelopfrom Apr 4, 2026
Merged
(SP: 3) [SHOP] Launch readiness hardening after repeat audit #442ViktorSvertoka merged 32 commits intodevelopfrom
ViktorSvertoka merged 32 commits intodevelopfrom
Conversation
…egression coverage
…/release invariants
…ad-stability guards
…onflict containment
… retry semantics clear
…+ align checkout shipping tests with UAH policy
…ckout (fail-closed)
…2 at route boundary
…closed contract at route boundary
…thout side effects
…ook, and checkout concurrency clusters
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
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds SHOP_SELLER_ADDRESS support; extracts SizeGuideAccordion and surfaces product sizes; adds many Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant CheckoutRoute as POST /api/shop/checkout
participant Validation as Zod Validation
participant CheckoutService as createOrderWithItems
participant LegalSvc as getShopLegalVersions
participant PriceSvc as rehydrateCartItems
participant DB as Database
participant Canonical as writeCanonicalEventWithRetry
Client->>CheckoutRoute: POST checkout payload
CheckoutRoute->>Validation: validate + superRefine (provider/method/currency)
alt invalid
Validation-->>CheckoutRoute: 422 INVALID_PAYLOAD
CheckoutRoute-->>Client: 422
else valid
CheckoutRoute->>CheckoutService: call with parsed payload
CheckoutService->>LegalSvc: fetch canonical terms/privacy
LegalSvc-->>CheckoutService: versions
alt mismatch
CheckoutService-->>CheckoutRoute: throw TERMS/PRIVACY_VERSION_MISMATCH
CheckoutRoute-->>Client: 422
else
CheckoutService->>PriceSvc: validate authoritative priceMinor
alt price error
PriceSvc-->>CheckoutRoute: PRICE_CONFIG_ERROR -> 422
CheckoutRoute-->>Client: 422
else
CheckoutService->>DB: create order + items
DB-->>CheckoutService: order created
CheckoutService->>Canonical: writeCanonicalEventWithRetry(order_created)
Canonical->>DB: write event (retry once on failure)
Canonical-->>CheckoutService: complete
CheckoutService-->>CheckoutRoute: success (201)
CheckoutRoute-->>Client: 201
end
end
end
sequenceDiagram
participant Stripe as Stripe Event
participant StripeRoute as POST /api/shop/webhooks/stripe
participant Apply as applyPaidStateFromPaymentIntent
participant Snapshot as readStripeOrderPaymentSnapshot
participant Resolver as resolveTerminalSuccessConflictSource
participant GuardedUpdate as guardedPaymentStatusUpdate
participant DB as Database
Stripe->>StripeRoute: payment_intent.succeeded
StripeRoute->>Apply: attempt apply paid state
alt applied=true
Apply-->>StripeRoute: success -> ack()
StripeRoute-->>Stripe: 200
else applied=false
Apply->>Snapshot: read latest order snapshot
Snapshot-->>Resolver: analyze for late_success_after_failed/refunded
alt conflict source found
Resolver->>GuardedUpdate: attempt transition to needs_review
alt transition applied
GuardedUpdate-->>StripeRoute: applied=true -> ack()
StripeRoute-->>Stripe: 200
else applied=false
GuardedUpdate-->>StripeRoute: blocked -> release claim
StripeRoute-->>Stripe: 503 (TERMINAL_SUCCESS_CONFLICT_BLOCKED + Retry-After)
end
else no conflict
StripeRoute-->>Stripe: 200 (noop ack)
end
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![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: 2fc774b04f
ℹ️ 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: 9
🧹 Nitpick comments (16)
frontend/lib/tests/shop/order-items-variants.test.ts (1)
37-54: Seed both currency rows in a single insert for more deterministic setup.This currently does two DB writes for one fixture. Consider a single multi-row
values([...])insert so test setup is atomic and slightly faster.Proposed refactor
- await db.insert(productPrices).values({ - id: priceId, - productId, - currency: 'UAH', - priceMinor: 1800, - originalPriceMinor: null, - price: '18.00', - originalPrice: null, - }); - - await db.insert(productPrices).values({ - productId, - currency: 'USD', - priceMinor: 1800, - originalPriceMinor: null, - price: '18.00', - originalPrice: null, - }); + await db.insert(productPrices).values([ + { + id: priceId, + productId, + currency: 'UAH', + priceMinor: 1800, + originalPriceMinor: null, + price: '18.00', + originalPrice: null, + }, + { + productId, + currency: 'USD', + priceMinor: 1800, + originalPriceMinor: null, + price: '18.00', + originalPrice: null, + }, + ]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/tests/shop/order-items-variants.test.ts` around lines 37 - 54, Combine the two separate inserts into a single multi-row insert to make the test seed atomic: replace the two db.insert(productPrices).values(...) calls with one db.insert(productPrices).values([...]) call that includes both rows (one with id: priceId, currency: 'UAH', productId, priceMinor: 1800, etc., and the other with currency: 'USD', productId, priceMinor: 1800, etc.). Keep the same field names and values, preserve the priceId only on the UAH row, and use the existing productPrices and db.insert(...) call to locate the change.frontend/lib/tests/shop/product-sale-invariant.test.ts (3)
23-31: Signature differs from other test files.Unlike the other test files where
originalPriceMinordefaults tonull, this version requires it as a mandatory parameter. This may be intentional for SALE invariant tests whereoriginalPriceMinoris always explicitly provided, but it creates an inconsistency if you later extract these helpers to a shared module.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/tests/shop/product-sale-invariant.test.ts` around lines 23 - 31, The helper function dualCurrencyPrices has a differing signature because originalPriceMinor is required; change its signature in dualCurrencyPrices to make originalPriceMinor optional with a default of null (e.g., originalPriceMinor: number | null = null) so it matches other test helpers and can be safely shared, and update any call sites if they relied on it being mandatory.
83-95: Consider extracting todualCurrencyPriceRowshelper.Other test files (
admin-product-photo-management.test.ts,product-images-contract.test.ts) extract this DB row mapping into adualCurrencyPriceRowshelper. Consider adding the same helper here for consistency.♻️ Suggested refactor
+function dualCurrencyPriceRows( + productId: string, + priceMinor: number, + originalPriceMinor: number | null +) { + return dualCurrencyPrices(priceMinor, originalPriceMinor).map(price => ({ + productId, + currency: price.currency, + priceMinor: price.priceMinor, + originalPriceMinor: price.originalPriceMinor, + price: toDbMoney(price.priceMinor), + originalPrice: + price.originalPriceMinor == null + ? null + : toDbMoney(price.originalPriceMinor), + })); +} + // Then usage becomes: - await db.insert(productPrices).values( - dualCurrencyPrices(1000, 2000).map(price => ({ - productId: p.id, - currency: price.currency, - priceMinor: price.priceMinor, - originalPriceMinor: price.originalPriceMinor, - price: toDbMoney(price.priceMinor), - originalPrice: - price.originalPriceMinor == null - ? null - : toDbMoney(price.originalPriceMinor), - })) - ); + await db.insert(productPrices).values(dualCurrencyPriceRows(p.id, 1000, 2000));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/tests/shop/product-sale-invariant.test.ts` around lines 83 - 95, Extract the DB row mapping used when inserting prices into a reusable helper named dualCurrencyPriceRows and use it in this test instead of inlining the map; specifically, create dualCurrencyPriceRows(dualCurrencyPrices, productId) (or similar) that performs the same mapping using toDbMoney and originalPrice null handling, then replace the inline dualCurrencyPrices(1000, 2000).map(...) passed to productPrices.insert(...) with dualCurrencyPriceRows(dualCurrencyPrices(1000, 2000), p.id) to match the other tests' pattern and keep consistency with productPrices, dualCurrencyPrices, toDbMoney and p.id.
112-119: Assertion verifies at least one matching row.Using
arrayContaining+objectContainingonly verifies that at least one row matches the expected values. Since the test inserts two currency rows (UAH and USD) and the invariant should hold for both, consider a stricter assertion to verify both rows retained their original values.♻️ Stricter assertion suggestion
- expect(rows).toEqual( - expect.arrayContaining([ - expect.objectContaining({ - priceMinor: 1000, - originalPriceMinor: 2000, - }), - ]) - ); + expect(rows).toHaveLength(2); + for (const row of rows) { + expect(row).toMatchObject({ + priceMinor: 1000, + originalPriceMinor: 2000, + }); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/tests/shop/product-sale-invariant.test.ts` around lines 112 - 119, The current assertion only checks that at least one row matches; update the assertion so both currency rows are validated. Replace the arrayContaining+single objectContaining check on the rows variable with a stricter assertion that ensures both rows have priceMinor: 1000 and originalPriceMinor: 2000 — either by asserting rows contains both expected objects (use expect.arrayContaining with two expect.objectContaining entries including a unique key like currency for UAH and USD) or by iterating rows and asserting each row.priceMinor === 1000 and row.originalPriceMinor === 2000; target the existing expect(rows) assertion to make this change.frontend/lib/tests/shop/admin-product-create-atomic-phasec.test.ts (1)
99-107: Consider extracting to a shared test utility.This
dualCurrencyPriceshelper is duplicated across multiple test files (admin-product-photo-management.test.ts,product-images-contract.test.ts,product-sale-invariant.test.ts). Consider extracting it to a shared test utility module to reduce duplication and ensure consistent behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/tests/shop/admin-product-create-atomic-phasec.test.ts` around lines 99 - 107, Extract the duplicated helper function dualCurrencyPrices into a shared test utility module and export it so all tests can import the same implementation; update the tests that currently duplicate it (admin-product-photo-management.test.ts, product-images-contract.test.ts, product-sale-invariant.test.ts, and admin-product-create-atomic-phasec.test.ts) to import dualCurrencyPrices instead of redefining it, preserving the signature (priceMinor: number, originalPriceMinor: number | null = null) and return shape ({ currency: 'UAH'|'USD', priceMinor, originalPriceMinor }) and then remove the duplicate definitions from each test file and run the test suite to verify no behavioral changes.frontend/components/shop/SizeGuideAccordion.tsx (1)
37-41: Consider using index-based keys forfitNotesto handle potential duplicates.Using the note content itself as the key (
key={note}) will cause React warnings and unexpected behavior if two fit notes have identical text. While duplicates may be unlikely, using the index is safer for list items that aren't reordered.Proposed fix
- {sizeGuide.fitNotes.map(note => ( - <li key={note}>{note}</li> + {sizeGuide.fitNotes.map((note, index) => ( + <li key={index}>{note}</li> ))}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/components/shop/SizeGuideAccordion.tsx` around lines 37 - 41, In SizeGuideAccordion where sizeGuide.fitNotes is rendered, avoid using the note text as the React key; change the map to include the index (e.g., map((note, index) => ...) and use the index as the key for the <li>) so duplicate note strings won't produce warnings or unstable renders when rendering the fitNotes list.frontend/lib/tests/shop/checkout-order-created-notification-phase5.test.ts (1)
152-182: Consider extracting shared test helpers to reduce duplication.
loadOrderOutboxRowandrunNotificationWorkerUntilSentare duplicated between this file andstatus-notifications-phase5.test.ts. Consider extracting these to a shared test utility module (e.g.,frontend/lib/tests/shop/helpers/notification-test-utils.ts).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/tests/shop/checkout-order-created-notification-phase5.test.ts` around lines 152 - 182, Extract the duplicated test helpers loadOrderOutboxRow and runNotificationWorkerUntilSent into a shared test utility module (e.g., create notification-test-utils.ts), then import and use them in both checkout-order-created-notification-phase5.test.ts and status-notifications-phase5.test.ts; ensure the exported helpers keep the same signatures (loadOrderOutboxRow(orderId: string) and runNotificationWorkerUntilSent(orderId: string, maxRuns?: number)) and that runNotificationOutboxWorker and db/notificationOutbox dependencies are imported or passed through if needed so tests continue to run unchanged.frontend/lib/tests/shop/status-notifications-phase5.test.ts (1)
140-157: Worker loop has high limit that may cause slow tests or process unrelated data.The
runNotificationWorkerUntilSenthelper useslimit: 5000per worker run, which could process thousands of unrelated outbox rows from other tests. Consider using a smaller limit (e.g., 10-50) focused on the test's order, or filtering within the worker if supported.🔧 Suggested smaller limit
async function runNotificationWorkerUntilSent(orderId: string, maxRuns = 20) { for (let run = 0; run < maxRuns; run += 1) { const row = await loadOrderOutboxRow(orderId); if (row?.status === 'sent') { return row; } await runNotificationOutboxWorker({ runId: `notify-worker-${crypto.randomUUID()}`, - limit: 5000, + limit: 50, leaseSeconds: 120, maxAttempts: 5, baseBackoffSeconds: 5, }); } return loadOrderOutboxRow(orderId); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/tests/shop/status-notifications-phase5.test.ts` around lines 140 - 157, The helper runNotificationWorkerUntilSent is calling runNotificationOutboxWorker with a very high limit (limit: 5000) which can make tests slow and process unrelated outbox rows; change the worker invocation in runNotificationWorkerUntilSent to use a much smaller limit (e.g., 10–50) or the smallest value that still lets your test's order be processed (for example limit: 50), and if runNotificationOutboxWorker supports scoping/filttering, pass the test's orderId or a dedicated filter option instead; update the invocation in runNotificationWorkerUntilSent (and related test helpers) to the new limit/filter so the loop only processes relevant rows and finishes quickly, and keep the loadOrderOutboxRow calls (loadOrderOutboxRow) unchanged for polling results.frontend/lib/services/shop/notifications/projector.ts (1)
48-63: Silent fallback tonew Date()may mask data integrity issues.When
normalizeOccurredAtreceives an invalid or unparseable string, it silently returnsnew Date()(current time). This could hide upstream bugs whereoccurredAtis corrupted or missing, causing notifications to have incorrect timestamps without any observability.Consider logging a warning when falling back to
new Date()so data issues can be detected in production.🔧 Suggested improvement with logging
function normalizeOccurredAt( value: CanonicalOccurredAt | null | undefined ): Date { if (value instanceof Date) { return value; } if (typeof value === 'string') { const parsed = new Date(value); if (!Number.isNaN(parsed.getTime())) { return parsed; } } + // Consider adding structured logging here: + // console.warn('[normalizeOccurredAt] Invalid or missing occurredAt, using current time', { value }); return new Date(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/services/shop/notifications/projector.ts` around lines 48 - 63, normalizeOccurredAt currently silently falls back to new Date() which can hide upstream data issues; update normalizeOccurredAt to log a warning when it must fallback by either accepting an optional logger param (e.g., normalizeOccurredAt(value, logger?) or using an imported logger) and call logger.warn (or console.warn if no logger) including the function name, the invalid input value and that a fallback to current time was used, then still return new Date(); reference the normalizeOccurredAt function to locate the change and ensure the warning includes the original value for observability.frontend/lib/services/shop/shipping/admin-edit.ts (1)
437-449: Consider preserving address-validation precedence here.This now returns before
resolveSnapshotData(), so stale or non-existentcityRef/warehouseRefvalues never hit theINVALID_SHIPPING_ADDRESSpath; every quote-affecting request collapses toSHIPPING_EDIT_REQUIRES_TOTAL_SYNC. If operators still need the more actionable bad-ref error for stale selections, validate first and then fail closed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/services/shop/shipping/admin-edit.ts` around lines 437 - 449, The current logic throws SHIPPING_EDIT_REQUIRES_TOTAL_SYNC when quoteAffectingChange is true before calling resolveSnapshotData, which prevents stale or missing cityRef/warehouseRef from reaching the INVALID_SHIPPING_ADDRESS path; change the flow in the handler that checks quoteAffectingChange so that you first call resolveSnapshotData (passing executor: tx, input: args.shipping, existingSnapshot: state.shipping_address, preserveQuote: true) to run address validation and surface INVALID_SHIPPING_ADDRESS, then after validation fail closed with SHIPPING_EDIT_REQUIRES_TOTAL_SYNC if quoteAffectingChange remains true; reference the quoteAffectingChange check, resolveSnapshotData call, INVALID_SHIPPING_ADDRESS and SHIPPING_EDIT_REQUIRES_TOTAL_SYNC symbols when making the change.frontend/lib/services/shop/admin-order-lifecycle.ts (1)
300-316:writeLifecycleAuditNonBlockingis still synchronous from the caller’s perspective.This makes audit failures fail-open, but it does not remove audit latency from
confirm/cancel/complete: the helper still waits forargs.write()to finish before returning, and every changed call site still awaits it. If the intent is a true non-blocking policy, fire the promise here and attach the logger in.catch(...)instead.Possible change
-async function writeLifecycleAuditNonBlocking(args: { +function writeLifecycleAuditNonBlocking(args: { orderId: string; requestId: string; action: AdminOrderLifecycleAction; write: () => Promise<unknown>; -}): Promise<void> { - try { - await args.write(); - } catch (error) { - logError('admin_order_lifecycle_audit_failed', error, { - orderId: args.orderId, - requestId: args.requestId, - action: args.action, - code: 'ADMIN_AUDIT_FAILED', - }); - } +}): void { + void Promise.resolve() + .then(() => args.write()) + .catch(error => { + logError('admin_order_lifecycle_audit_failed', error, { + orderId: args.orderId, + requestId: args.requestId, + action: args.action, + code: 'ADMIN_AUDIT_FAILED', + }); + }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/services/shop/admin-order-lifecycle.ts` around lines 300 - 316, The helper writeLifecycleAuditNonBlocking currently awaits args.write(), so callers still block; change it to fire-and-forget by invoking args.write() without awaiting and attach a .catch(...) that calls logError with the same metadata (orderId, requestId, action, code). In other words, replace the await pattern in writeLifecycleAuditNonBlocking (and any internal try/catch) with a non-blocking invocation like Promise.resolve(args.write()).catch(error => logError(...)) so audit writes run asynchronously and failures are logged but do not delay confirm/cancel/complete.frontend/lib/tests/shop/admin-order-lifecycle-audit-reliability.test.ts (1)
274-283: Type assertion may mask schema mismatch.The
as anyon theshippingShipmentsinsert suggests the values may not fully match the table schema. Consider aligning the test values with the expected schema types to catch type drift early.Suggested approach
Verify the
shippingShipmentsschema and provide all required fields with correct types rather than usingas any. If fields likecreatedAt,updatedAt, or others have defaults, the insert should still work without the assertion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/tests/shop/admin-order-lifecycle-audit-reliability.test.ts` around lines 274 - 283, The test is using a broad "as any" on the db.insert into shippingShipments which can hide schema mismatches; update the insert call for shippingShipments (the db.insert(...).values(...) invocation) to supply all required columns with the correct types (e.g., ensure id uses crypto.randomUUID(), orderId, provider, status, attemptCount as number, and set nullable fields or defaults like leaseOwner, leaseExpiresAt, nextAttemptAt, createdAt/updatedAt if required by the shippingShipments schema) instead of casting to any so TypeScript validates the payload against the shippingShipments table type.frontend/lib/tests/shop/admin-product-stock-protection.test.ts (1)
101-113: Consider removingas anytype assertions.The
as anyon insert values (lines 113, 163) may hide schema mismatches. If the schema has required fields with defaults, the insert should work without assertions. This helps catch type drift early.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/tests/shop/admin-product-stock-protection.test.ts` around lines 101 - 113, Remove the unsafe "as any" on the object passed to db.insert(products).values and make the literal conform to the products table insert type (e.g., ProductInsert or the generated insert row type) instead; adjust the object by adding any required fields or defaults (or use a proper typed helper) and keep helpers like toDbMoney as-is so the shape matches the DB schema rather than suppressing type errors with "as any".frontend/lib/tests/shop/checkout-inactive-after-cart.test.ts (2)
57-102: Consider usingvi.stubEnvfor consistency with other test files.Other test files in this PR use
vi.stubEnv()withvi.unstubAllEnvs()for environment variable management. The manual save/restore pattern here is functional but more verbose and error-prone.♻️ Suggested refactor using vi.stubEnv
-const __prevRateLimitDisabled = process.env.RATE_LIMIT_DISABLED; -const __prevPaymentsEnabled = process.env.PAYMENTS_ENABLED; -const __prevStripePaymentsEnabled = process.env.STRIPE_PAYMENTS_ENABLED; -const __prevStripeSecret = process.env.STRIPE_SECRET_KEY; -const __prevStripeWebhookSecret = process.env.STRIPE_WEBHOOK_SECRET; - -beforeAll(() => { - process.env.RATE_LIMIT_DISABLED = '1'; - process.env.PAYMENTS_ENABLED = 'true'; - process.env.STRIPE_PAYMENTS_ENABLED = 'true'; - process.env.STRIPE_SECRET_KEY = 'sk_test_checkout_inactive_after_cart'; - process.env.STRIPE_WEBHOOK_SECRET = 'whsec_test_checkout_inactive_after_cart'; -}); - -afterAll(async () => { - // ... cleanup logic - if (__prevRateLimitDisabled === undefined) - delete process.env.RATE_LIMIT_DISABLED; - else process.env.RATE_LIMIT_DISABLED = __prevRateLimitDisabled; - // ... etc -}); +beforeEach(() => { + vi.stubEnv('RATE_LIMIT_DISABLED', '1'); + vi.stubEnv('PAYMENTS_ENABLED', 'true'); + vi.stubEnv('STRIPE_PAYMENTS_ENABLED', 'true'); + vi.stubEnv('STRIPE_SECRET_KEY', 'sk_test_checkout_inactive_after_cart'); + vi.stubEnv('STRIPE_WEBHOOK_SECRET', 'whsec_test_checkout_inactive_after_cart'); +}); + +afterEach(() => { + vi.unstubAllEnvs(); +});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/tests/shop/checkout-inactive-after-cart.test.ts` around lines 57 - 102, Replace the manual save/restore env pattern in this test (the __prevRateLimitDisabled, __prevPaymentsEnabled, __prevStripePaymentsEnabled, __prevStripeSecret, __prevStripeWebhookSecret variables and their restore logic in beforeAll/afterAll) with vi.stubEnv in beforeAll to set RATE_LIMIT_DISABLED, PAYMENTS_ENABLED, STRIPE_PAYMENTS_ENABLED, STRIPE_SECRET_KEY, and STRIPE_WEBHOOK_SECRET, and call vi.unstubAllEnvs() in afterAll (while keeping the DB cleanup logic that references createdProductIds, db.delete(...), inventoryMoves, orderItems, productPrices, and products intact); remove the manual env save/restore variables and branches so env handling matches other tests that use vi.stubEnv/vi.unstubAllEnvs.
113-146: Type casts (as any) on DB inserts suggest schema type mismatches.Consider updating the schema or using proper typing rather than
as anycasts on lines 134 and 146. This could mask type errors during refactoring.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/tests/shop/checkout-inactive-after-cart.test.ts` around lines 113 - 146, The test is masking schema mismatches by using `as any` on the objects passed to `db.insert(products).values(...)` and `db.insert(productPrices).values(...)`; remove the `as any` casts and make the inserted objects conform to your DB schema types (e.g., the product and productPrice table row types or an Insert type), or create typed factory helpers (e.g., createProductRow(productId, now) / createProductPriceRow(productId, now)) that return the correct shape; ensure fields like currency/price types, nullable fields, and IDs match the table definitions so `db.insert(products).values` and `db.insert(productPrices).values` compile without `any`.frontend/lib/services/products/mutations/toggle.ts (1)
60-61: Consider addingfieldtoInvalidPayloadErrortype definition.The
(error as any).fieldpattern suggests the error type doesn't support thefieldproperty. Adding it to the type definition would provide type safety.♻️ Suggested type extension
In
frontend/lib/services/errors.ts:export class InvalidPayloadError extends Error { code: string; details?: Record<string, unknown>; field?: string; // Add field property // ... }Then in this file:
- (error as any).field = 'prices'; + error.field = 'prices';Also applies to: 78-79
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/services/products/mutations/toggle.ts` around lines 60 - 61, The code assigns a dynamic field property to an error ((error as any).field = 'prices') because InvalidPayloadError lacks a typed field property; update the InvalidPayloadError class in frontend/lib/services/errors.ts to include field?: string (alongside existing code/details) so the assignment is type-safe, then replace the (error as any).field usages in toggle.ts (and the similar usage at the other occurrence) by asserting or narrowing the error to InvalidPayloadError where appropriate (e.g., catch (err) { const error = err as InvalidPayloadError; error.field = 'prices'; throw error; }).
🤖 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/app/api/shop/admin/products/`[id]/status/route.ts:
- Around line 199-222: The handler currently defensively casts the caught error
to Record and reads a non-existent field property; replace that with direct
typed access to InvalidPayloadError.code and .details (e.g., use error.code and
error.details) and remove any use of `field` (both in logWarn metadata and in
the noStoreJson response). Update the logWarn call
(admin_product_status_invalid_payload_error) to include baseMeta, productId,
code, and durationMs only, and return noStoreJson with error.message (or
'Invalid product state'), code, and details, keeping the 400 status; ensure you
reference the InvalidPayloadError type, logWarn, and noStoreJson symbols when
making the changes.
In `@frontend/app/api/shop/checkout/route.ts`:
- Around line 1757-1759: The checkout error handler currently only maps
InsufficientStockError to a 422 response; update the branch in route.ts that
returns errorResponse('INSUFFICIENT_STOCK', ...) to also detect business errors
with code === 'OUT_OF_STOCK' (e.g., check error.code === 'OUT_OF_STOCK' or error
instanceof BusinessError && error.code === 'OUT_OF_STOCK') so those throwers are
translated to the same 422/INSUFFICIENT_STOCK response instead of falling
through to 500; keep using the same error.message and status (422).
- Around line 279-280: The current assignment of status using
checkoutUnprocessableStatus(...) ?? shippingErrorStatus(...) short-circuits and
prevents the subsequent PriceConfigError and IdempotencyConflictError branches
from running for Monobank responses; instead, compute or derive status only
after handling those specific Monobank error branches (or reorder the logic) so
that PriceConfigError and IdempotencyConflictError can inspect the raw Monobank
response and restore their structured details before falling back to
checkoutUnprocessableStatus or shippingErrorStatus; update the flow around the
status variable and the branches that reference PriceConfigError and
IdempotencyConflictError so those branches run first (or incorporate Monobank
mapping into the status resolution) while still defaulting to
checkoutUnprocessableStatus/shippingErrorStatus when no structured mapping
applies.
In `@frontend/lib/services/orders/restock.ts`:
- Around line 347-360: When either concurrent-finalize branch detects that
another worker already finalized the cancel restock (the check using
isRestockReasonAlreadyFinalized(latest, reason) in the touched===false branch
and the equivalent check in the other branch around lines 523-535), call
ensureOrderCanceledCanonicalEvent(orderId, reason) (or replay the order_canceled
canonical write) before returning; i.e., before the early return in both places
ensureOrderCanceledCanonicalEvent is invoked with the same orderId and reason so
the canonical event is persisted even if the winning worker dies.
- Around line 152-177: The refunded branch in isRestockReasonAlreadyFinalized is
too broad; restrict it so a refund is only considered already-finalized when the
state explicitly indicates a refund (e.g., state.status === 'REFUNDED' or the
payment status shows 'refunded') in addition to inventoryStatus === 'released'
and state.stockRestored. Update the refunded check in
isRestockReasonAlreadyFinalized to require one of those explicit refund
indicators together with the existing inventory conditions, and ensure the two
"latest" re-queries that populate RestockFinalizeState include
orders.paymentStatus so state.paymentStatus is available for this check.
In `@frontend/lib/services/shop/shipping/shipments-worker.ts`:
- Around line 1185-1231: finalizeShipmentSuccess currently calls markSucceeded
(which releases the lease) and then tries a fallback markFailed that depends on
the lease_owner = runId, so the fallback updates zero rows leaving shipment
marked succeeded while order remains blocked; change the flow so the lease is
not required for the post-success attention update: either (A) delay releasing
the lease by modifying the sequence in finalizeShipmentSuccess so you
validate/apply the order transition (order_updated) before calling
markSucceeded, or (B) introduce and call a new update function (e.g.,
markNeedsAttentionPostSuccess or markFailedPostSuccess) that sets
terminalNeedsAttention/buildFailure for the shipment/order without requiring
lease_owner = runId (or accepts a flag like ignoreLease:true), and use that in
the !marked.order_updated branch instead of markFailed; update callers and tests
accordingly and keep references to finalizeShipmentSuccess, markSucceeded,
markFailed, and buildFailure so reviewers can find the changes.
In `@frontend/lib/tests/shop/checkout-authoritative-price-minor.test.ts`:
- Around line 1-2: The tests call crypto.randomUUID() but never import crypto,
causing a runtime failure; add an import at the top of the test file such as
"import crypto from 'crypto';" (or "import { randomUUID } from 'crypto';" and
update usages to randomUUID()) so crypto.randomUUID() invocations in the
checkout-authoritative-price-minor tests resolve correctly.
- Around line 31-39: The current mockSelectRows provides from() → leftJoin() →
where() but not from() → where(), causing queries from createOrderWithItems to
fail; update mockSelectRows (the dbSelectMock implementation) so from() returns
an object that has both leftJoin and where methods, where leftJoin() returns an
object with where(), and both where implementations resolve to the provided
rows; keep using dbSelectMock.mockImplementationOnce and ensure the async
behavior matches existing tests (used by rehydrateCartItems and
createOrderWithItems).
In `@frontend/lib/tests/shop/test-legal-consent.ts`:
- Line 14: TEST_LEGAL_CONSENT is computed at import time and therefore snapshots
legal versions; stop exporting a precomputed object and export a factory call
instead so tests get fresh values. Replace the cached export
(TEST_LEGAL_CONSENT) with an exported factory function (e.g.,
getTestLegalConsent or export the createTestLegalConsent function directly) and
update call sites to call that function when building the request payload so
each test receives a newly generated consent object.
---
Nitpick comments:
In `@frontend/components/shop/SizeGuideAccordion.tsx`:
- Around line 37-41: In SizeGuideAccordion where sizeGuide.fitNotes is rendered,
avoid using the note text as the React key; change the map to include the index
(e.g., map((note, index) => ...) and use the index as the key for the <li>) so
duplicate note strings won't produce warnings or unstable renders when rendering
the fitNotes list.
In `@frontend/lib/services/products/mutations/toggle.ts`:
- Around line 60-61: The code assigns a dynamic field property to an error
((error as any).field = 'prices') because InvalidPayloadError lacks a typed
field property; update the InvalidPayloadError class in
frontend/lib/services/errors.ts to include field?: string (alongside existing
code/details) so the assignment is type-safe, then replace the (error as
any).field usages in toggle.ts (and the similar usage at the other occurrence)
by asserting or narrowing the error to InvalidPayloadError where appropriate
(e.g., catch (err) { const error = err as InvalidPayloadError; error.field =
'prices'; throw error; }).
In `@frontend/lib/services/shop/admin-order-lifecycle.ts`:
- Around line 300-316: The helper writeLifecycleAuditNonBlocking currently
awaits args.write(), so callers still block; change it to fire-and-forget by
invoking args.write() without awaiting and attach a .catch(...) that calls
logError with the same metadata (orderId, requestId, action, code). In other
words, replace the await pattern in writeLifecycleAuditNonBlocking (and any
internal try/catch) with a non-blocking invocation like
Promise.resolve(args.write()).catch(error => logError(...)) so audit writes run
asynchronously and failures are logged but do not delay confirm/cancel/complete.
In `@frontend/lib/services/shop/notifications/projector.ts`:
- Around line 48-63: normalizeOccurredAt currently silently falls back to new
Date() which can hide upstream data issues; update normalizeOccurredAt to log a
warning when it must fallback by either accepting an optional logger param
(e.g., normalizeOccurredAt(value, logger?) or using an imported logger) and call
logger.warn (or console.warn if no logger) including the function name, the
invalid input value and that a fallback to current time was used, then still
return new Date(); reference the normalizeOccurredAt function to locate the
change and ensure the warning includes the original value for observability.
In `@frontend/lib/services/shop/shipping/admin-edit.ts`:
- Around line 437-449: The current logic throws
SHIPPING_EDIT_REQUIRES_TOTAL_SYNC when quoteAffectingChange is true before
calling resolveSnapshotData, which prevents stale or missing
cityRef/warehouseRef from reaching the INVALID_SHIPPING_ADDRESS path; change the
flow in the handler that checks quoteAffectingChange so that you first call
resolveSnapshotData (passing executor: tx, input: args.shipping,
existingSnapshot: state.shipping_address, preserveQuote: true) to run address
validation and surface INVALID_SHIPPING_ADDRESS, then after validation fail
closed with SHIPPING_EDIT_REQUIRES_TOTAL_SYNC if quoteAffectingChange remains
true; reference the quoteAffectingChange check, resolveSnapshotData call,
INVALID_SHIPPING_ADDRESS and SHIPPING_EDIT_REQUIRES_TOTAL_SYNC symbols when
making the change.
In `@frontend/lib/tests/shop/admin-order-lifecycle-audit-reliability.test.ts`:
- Around line 274-283: The test is using a broad "as any" on the db.insert into
shippingShipments which can hide schema mismatches; update the insert call for
shippingShipments (the db.insert(...).values(...) invocation) to supply all
required columns with the correct types (e.g., ensure id uses
crypto.randomUUID(), orderId, provider, status, attemptCount as number, and set
nullable fields or defaults like leaseOwner, leaseExpiresAt, nextAttemptAt,
createdAt/updatedAt if required by the shippingShipments schema) instead of
casting to any so TypeScript validates the payload against the shippingShipments
table type.
In `@frontend/lib/tests/shop/admin-product-create-atomic-phasec.test.ts`:
- Around line 99-107: Extract the duplicated helper function dualCurrencyPrices
into a shared test utility module and export it so all tests can import the same
implementation; update the tests that currently duplicate it
(admin-product-photo-management.test.ts, product-images-contract.test.ts,
product-sale-invariant.test.ts, and admin-product-create-atomic-phasec.test.ts)
to import dualCurrencyPrices instead of redefining it, preserving the signature
(priceMinor: number, originalPriceMinor: number | null = null) and return shape
({ currency: 'UAH'|'USD', priceMinor, originalPriceMinor }) and then remove the
duplicate definitions from each test file and run the test suite to verify no
behavioral changes.
In `@frontend/lib/tests/shop/admin-product-stock-protection.test.ts`:
- Around line 101-113: Remove the unsafe "as any" on the object passed to
db.insert(products).values and make the literal conform to the products table
insert type (e.g., ProductInsert or the generated insert row type) instead;
adjust the object by adding any required fields or defaults (or use a proper
typed helper) and keep helpers like toDbMoney as-is so the shape matches the DB
schema rather than suppressing type errors with "as any".
In `@frontend/lib/tests/shop/checkout-inactive-after-cart.test.ts`:
- Around line 57-102: Replace the manual save/restore env pattern in this test
(the __prevRateLimitDisabled, __prevPaymentsEnabled,
__prevStripePaymentsEnabled, __prevStripeSecret, __prevStripeWebhookSecret
variables and their restore logic in beforeAll/afterAll) with vi.stubEnv in
beforeAll to set RATE_LIMIT_DISABLED, PAYMENTS_ENABLED, STRIPE_PAYMENTS_ENABLED,
STRIPE_SECRET_KEY, and STRIPE_WEBHOOK_SECRET, and call vi.unstubAllEnvs() in
afterAll (while keeping the DB cleanup logic that references createdProductIds,
db.delete(...), inventoryMoves, orderItems, productPrices, and products intact);
remove the manual env save/restore variables and branches so env handling
matches other tests that use vi.stubEnv/vi.unstubAllEnvs.
- Around line 113-146: The test is masking schema mismatches by using `as any`
on the objects passed to `db.insert(products).values(...)` and
`db.insert(productPrices).values(...)`; remove the `as any` casts and make the
inserted objects conform to your DB schema types (e.g., the product and
productPrice table row types or an Insert type), or create typed factory helpers
(e.g., createProductRow(productId, now) / createProductPriceRow(productId, now))
that return the correct shape; ensure fields like currency/price types, nullable
fields, and IDs match the table definitions so `db.insert(products).values` and
`db.insert(productPrices).values` compile without `any`.
In `@frontend/lib/tests/shop/checkout-order-created-notification-phase5.test.ts`:
- Around line 152-182: Extract the duplicated test helpers loadOrderOutboxRow
and runNotificationWorkerUntilSent into a shared test utility module (e.g.,
create notification-test-utils.ts), then import and use them in both
checkout-order-created-notification-phase5.test.ts and
status-notifications-phase5.test.ts; ensure the exported helpers keep the same
signatures (loadOrderOutboxRow(orderId: string) and
runNotificationWorkerUntilSent(orderId: string, maxRuns?: number)) and that
runNotificationOutboxWorker and db/notificationOutbox dependencies are imported
or passed through if needed so tests continue to run unchanged.
In `@frontend/lib/tests/shop/order-items-variants.test.ts`:
- Around line 37-54: Combine the two separate inserts into a single multi-row
insert to make the test seed atomic: replace the two
db.insert(productPrices).values(...) calls with one
db.insert(productPrices).values([...]) call that includes both rows (one with
id: priceId, currency: 'UAH', productId, priceMinor: 1800, etc., and the other
with currency: 'USD', productId, priceMinor: 1800, etc.). Keep the same field
names and values, preserve the priceId only on the UAH row, and use the existing
productPrices and db.insert(...) call to locate the change.
In `@frontend/lib/tests/shop/product-sale-invariant.test.ts`:
- Around line 23-31: The helper function dualCurrencyPrices has a differing
signature because originalPriceMinor is required; change its signature in
dualCurrencyPrices to make originalPriceMinor optional with a default of null
(e.g., originalPriceMinor: number | null = null) so it matches other test
helpers and can be safely shared, and update any call sites if they relied on it
being mandatory.
- Around line 83-95: Extract the DB row mapping used when inserting prices into
a reusable helper named dualCurrencyPriceRows and use it in this test instead of
inlining the map; specifically, create dualCurrencyPriceRows(dualCurrencyPrices,
productId) (or similar) that performs the same mapping using toDbMoney and
originalPrice null handling, then replace the inline dualCurrencyPrices(1000,
2000).map(...) passed to productPrices.insert(...) with
dualCurrencyPriceRows(dualCurrencyPrices(1000, 2000), p.id) to match the other
tests' pattern and keep consistency with productPrices, dualCurrencyPrices,
toDbMoney and p.id.
- Around line 112-119: The current assertion only checks that at least one row
matches; update the assertion so both currency rows are validated. Replace the
arrayContaining+single objectContaining check on the rows variable with a
stricter assertion that ensures both rows have priceMinor: 1000 and
originalPriceMinor: 2000 — either by asserting rows contains both expected
objects (use expect.arrayContaining with two expect.objectContaining entries
including a unique key like currency for UAH and USD) or by iterating rows and
asserting each row.priceMinor === 1000 and row.originalPriceMinor === 2000;
target the existing expect(rows) assertion to make this change.
In `@frontend/lib/tests/shop/status-notifications-phase5.test.ts`:
- Around line 140-157: The helper runNotificationWorkerUntilSent is calling
runNotificationOutboxWorker with a very high limit (limit: 5000) which can make
tests slow and process unrelated outbox rows; change the worker invocation in
runNotificationWorkerUntilSent to use a much smaller limit (e.g., 10–50) or the
smallest value that still lets your test's order be processed (for example
limit: 50), and if runNotificationOutboxWorker supports scoping/filttering, pass
the test's orderId or a dedicated filter option instead; update the invocation
in runNotificationWorkerUntilSent (and related test helpers) to the new
limit/filter so the loop only processes relevant rows and finishes quickly, and
keep the loadOrderOutboxRow calls (loadOrderOutboxRow) unchanged for polling
results.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e5ec132d-c143-408a-8c13-186ab7407a0e
📒 Files selected for processing (80)
frontend/.env.examplefrontend/app/[locale]/shop/products/[slug]/page.tsxfrontend/app/api/shop/admin/orders/[id]/cancel-payment/route.tsfrontend/app/api/shop/admin/orders/[id]/refund/route.tsfrontend/app/api/shop/admin/products/[id]/status/route.tsfrontend/app/api/shop/checkout/route.tsfrontend/app/api/shop/internal/monobank/janitor/route.tsfrontend/app/api/shop/orders/[id]/payment/init/route.tsfrontend/app/api/shop/orders/[id]/payment/monobank/google-pay/submit/route.tsfrontend/app/api/shop/orders/[id]/payment/monobank/invoice/route.tsfrontend/app/api/shop/webhooks/monobank/route.tsfrontend/app/api/shop/webhooks/stripe/route.tsfrontend/components/shop/AddToCartButton.tsxfrontend/components/shop/SizeGuideAccordion.tsxfrontend/lib/env/index.tsfrontend/lib/env/shop-critical.tsfrontend/lib/legal/public-seller-information.tsfrontend/lib/services/orders/_shared.tsfrontend/lib/services/orders/checkout.tsfrontend/lib/services/orders/restock.tsfrontend/lib/services/products/cart/rehydrate.tsfrontend/lib/services/products/mutations/toggle.tsfrontend/lib/services/products/mutations/update.tsfrontend/lib/services/shop/admin-order-lifecycle.tsfrontend/lib/services/shop/events/write-canonical-event-with-retry.tsfrontend/lib/services/shop/notifications/projector.tsfrontend/lib/services/shop/shipping/admin-actions.tsfrontend/lib/services/shop/shipping/admin-edit.tsfrontend/lib/services/shop/shipping/shipments-worker.tsfrontend/lib/shop/commercial-policy.server.tsfrontend/lib/shop/data.tsfrontend/lib/tests/helpers/makeCheckoutReq.tsfrontend/lib/tests/shop/admin-order-lifecycle-actions.test.tsfrontend/lib/tests/shop/admin-order-lifecycle-audit-reliability.test.tsfrontend/lib/tests/shop/admin-product-activation-validation.test.tsfrontend/lib/tests/shop/admin-product-create-atomic-phasec.test.tsfrontend/lib/tests/shop/admin-product-photo-management.test.tsfrontend/lib/tests/shop/admin-product-stock-protection.test.tsfrontend/lib/tests/shop/admin-shipping-edit-route.test.tsfrontend/lib/tests/shop/admin-shipping-edit.test.tsfrontend/lib/tests/shop/checkout-authoritative-price-minor.test.tsfrontend/lib/tests/shop/checkout-concurrency-stock1.test.tsfrontend/lib/tests/shop/checkout-currency-policy.test.tsfrontend/lib/tests/shop/checkout-inactive-after-cart.test.tsfrontend/lib/tests/shop/checkout-legal-consent-phase4.test.tsfrontend/lib/tests/shop/checkout-monobank-happy-path.test.tsfrontend/lib/tests/shop/checkout-monobank-idempotency-contract.test.tsfrontend/lib/tests/shop/checkout-monobank-parse-validation.test.tsfrontend/lib/tests/shop/checkout-order-created-notification-phase5.test.tsfrontend/lib/tests/shop/checkout-price-change-fail-closed.test.tsfrontend/lib/tests/shop/checkout-rate-limit-policy.test.tsfrontend/lib/tests/shop/checkout-route-stripe-disabled-recovery.test.tsfrontend/lib/tests/shop/checkout-route-validation-contract.test.tsfrontend/lib/tests/shop/checkout-set-payment-intent-reject-contract.test.tsfrontend/lib/tests/shop/checkout-shipping-authoritative-total.test.tsfrontend/lib/tests/shop/checkout-shipping-phase3.test.tsfrontend/lib/tests/shop/checkout-stripe-error-contract.test.tsfrontend/lib/tests/shop/checkout-stripe-payments-disabled.test.tsfrontend/lib/tests/shop/logging-redaction-real-flows.test.tsfrontend/lib/tests/shop/monobank-psp-unavailable.test.tsfrontend/lib/tests/shop/notifications-projector-phase3.test.tsfrontend/lib/tests/shop/order-items-variants.test.tsfrontend/lib/tests/shop/orders-status-ownership.test.tsfrontend/lib/tests/shop/product-images-contract.test.tsfrontend/lib/tests/shop/product-sale-invariant.test.tsfrontend/lib/tests/shop/public-cart-env-contract.test.tsfrontend/lib/tests/shop/public-seller-information-phase4.test.tsfrontend/lib/tests/shop/public-shop-runtime-cache-smoke.test.tsfrontend/lib/tests/shop/returns-policy-alignment-phase6.test.tsfrontend/lib/tests/shop/runtime-explicitness-phase7.test.tsfrontend/lib/tests/shop/shipping-shipments-worker-phase5.test.tsfrontend/lib/tests/shop/shop-critical-env-fail-fast.test.tsfrontend/lib/tests/shop/status-notifications-phase5.test.tsfrontend/lib/tests/shop/stripe-webhook-contract.test.tsfrontend/lib/tests/shop/stripe-webhook-replay-correctness.test.tsfrontend/lib/tests/shop/stripe-webhook-terminal-consistency.test.tsfrontend/lib/tests/shop/test-legal-consent.tsfrontend/messages/en.jsonfrontend/messages/pl.jsonfrontend/messages/uk.json
…cover route behavior, and repair shop tests
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
frontend/lib/tests/shop/status-notifications-phase5.test.ts (2)
381-382: Trivially true assertion on second projector run.
expect(secondProjectorRun.scanned).toBeGreaterThanOrEqual(0)is always satisfied sincescannedis a non-negative count. This is acceptable for test stability (avoiding brittleness), but if the intent is to verify idempotency, consider assertingsecondProjectorRun.insertedequals 0 instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/tests/shop/status-notifications-phase5.test.ts` around lines 381 - 382, The test contains a trivially true assertion: replace or augment expect(secondProjectorRun.scanned).toBeGreaterThanOrEqual(0) with an idempotency check that verifies no new rows were inserted on the second projector run; specifically assert secondProjectorRun.inserted === 0 (or use expect(secondProjectorRun.inserted).toBe(0)) so the test verifies the projector is idempotent while keeping the firstProjectorRun.scanned assertion as-is.
130-157: Helper functions improve test clarity, but are duplicated across test files.
loadOrderOutboxRowandrunNotificationWorkerUntilSentare duplicated incheckout-order-created-notification-phase5.test.ts. Consider extracting these to a shared test utility module (e.g.,frontend/lib/tests/shop/helpers/notification-test-utils.ts) to reduce maintenance burden.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/tests/shop/status-notifications-phase5.test.ts` around lines 130 - 157, The two helper functions loadOrderOutboxRow and runNotificationWorkerUntilSent are duplicated between status-notifications-phase5.test.ts and checkout-order-created-notification-phase5.test.ts; extract them into a new shared test utility module (e.g., frontend/lib/tests/shop/helpers/notification-test-utils.ts) exporting those functions, update both test files to import them from that module, and ensure any dependent symbols used inside (db, notificationOutbox, runNotificationOutboxWorker, crypto) are either imported into the helper module or passed in as parameters so the helpers remain self-contained and compile in both tests.frontend/lib/tests/shop/checkout-order-created-notification-phase5.test.ts (2)
184-198: Orphan cleanup uses raw SQL with hardcoded column names.This cleanup function is effective for preventing test pollution, but relies on raw SQL with hardcoded column/table names. If the schema changes (e.g., column renames), this will silently fail or break. Since this is test-only code, this is acceptable, but consider adding a comment noting the schema dependency.
📝 Suggested comment for clarity
async function cleanupOrphanOrderCreatedArtifacts() { + // Note: Uses raw column names from schema - update if schema changes await db.execute(sql` delete from notification_outbox🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/tests/shop/checkout-order-created-notification-phase5.test.ts` around lines 184 - 198, cleanupOrphanOrderCreatedArtifacts() uses raw SQL with hardcoded table and column names (notification_outbox.template_key, source_domain, order_id and payment_events.event_name, event_source) which makes the test fragile to schema renames; add a concise comment above the function explaining that this is test-only cleanup that intentionally uses raw SQL and depends on current column/table names, citing which columns/tables are used so future maintainers know to update the cleanup if the schema changes.
152-182: Duplicated helper functions—consider shared extraction.These helper functions are identical to those in
status-notifications-phase5.test.ts. Extracting them to a shared module would improve maintainability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/tests/shop/checkout-order-created-notification-phase5.test.ts` around lines 152 - 182, The test file duplicates helper functions loadOrderOutboxRow and runNotificationWorkerUntilSent already present in status-notifications-phase5.test.ts; extract these helpers into a shared test utility module (e.g., tests/helpers/notificationOutbox.ts) and import them in both test files, keeping function signatures (loadOrderOutboxRow, runNotificationWorkerUntilSent) and behavior identical; update the test imports to use the new module and remove the duplicate implementations from frontend/lib/tests/shop/checkout-order-created-notification-phase5.test.ts.frontend/lib/tests/shop/shipping-shipments-worker-phase5.test.ts (1)
1560-1616: Brittle but acceptable approach for testing the race condition.Intercepting
db.executeby parsing SQL text from query chunks is fragile and will break if the query structure changes. However, this is a pragmatic approach for testing the specific race condition where the order transition becomes blocked between shipment update and order update within the same CTE.Consider adding a comment explaining why this approach is used and what query structure it depends on, so future maintainers understand the coupling.
📝 Suggested documentation
+ // NOTE: This mock intercepts the markSucceeded query by matching SQL text patterns. + // It simulates a race where the order's shippingStatus changes between the + // shipment and order CTEs within markSucceeded. If the markSucceeded query + // structure changes, this test will need updating. executeSpy.mockImplementation((async (query: unknown) => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/tests/shop/shipping-shipments-worker-phase5.test.ts` around lines 1560 - 1616, Add a short explanatory comment near the test's db.execute interception (around executeSpy.mockImplementation / originalExecute) that explains this fragile SQL-parsing approach: state that the test simulates a race between the shipment update and order update inside the same CTE, describe which SQL elements the matcher relies on (presence of "update shipping_shipments s", "provider_ref =", "tracking_number =", and the subsequent "update orders" CTE/statement), and warn that changing the query chunking or CTE structure will break the interception and tests; reference executeSpy, originalExecute, db.execute, sql and seed.shipmentId/seed.orderId in the comment so future maintainers can locate and understand the coupling.frontend/lib/tests/shop/admin-shipping-edit.test.ts (1)
332-389: Also assert thatorderShipping.shippingAddressstays unchanged onINVALID_SHIPPING_ADDRESS.This branch exists specifically because validation now happens before persistence. Adding the same snapshot-immutability check used in the 409 test would make that contract explicit too.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/tests/shop/admin-shipping-edit.test.ts` around lines 332 - 389, After asserting the INVALID_SHIPPING_ADDRESS rejection, additionally query the current order shipping snapshot (select orderShipping.shippingAddress for the orderId, or join orders -> orderShipping if your schema uses a separate table) and assert it equals the original seed shipping address (use the seed's shippingAddress / seed.orderShipping.shippingAddress value); this mirrors the immutability check used in the 409 test and ensures applyAdminOrderShippingEdit (the call in this test) does not mutate orderShipping.shippingAddress on validation failure.
🤖 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/app/api/shop/checkout/route.ts`:
- Around line 1852-1862: The branch handling InsufficientStockError only checks
for InsufficientStockError instances and getErrorCode(...) === 'OUT_OF_STOCK',
which lets wrapped business errors with code 'INSUFFICIENT_STOCK' fall through;
update the condition in the same block (where InsufficientStockError,
getErrorCode, errorResponse and getErrorMessage are used) to also treat
getErrorCode(error) === 'INSUFFICIENT_STOCK' as a 422 checkout error (same
errorResponse payload as the other cases) so that
isExpectedBusinessError()/mapMonobankCheckoutError() classified errors are
handled consistently.
In `@frontend/lib/services/orders/checkout.ts`:
- Around line 1292-1323: The code calls resolveCheckoutLegalConsent before
getOrderByIdempotencyKey which causes retries to fail when canonical legal
versions advance; move or defer legal-consent resolution so idempotent-replay
checks run against the stored order first. Concretely, call
getOrderByIdempotencyKey(db, idempotencyKey) and handle the existing-order
branch (assertIdempotencyCompatible, ensureOrderShippingSnapshot,
ensureOrderCreatedCanonicalEvent, return) before invoking
resolveCheckoutLegalConsent/hashIdempotencyRequest, or make preparedLegalConsent
lazy so resolveCheckoutLegalConsent and inclusion in hashIdempotencyRequest only
occur for new-order flows (use resolveCheckoutLegalConsent, preparedShipping,
and hashIdempotencyRequest in the new-order path).
- Around line 575-580: The recipient hashing uses trimmed empty strings for
optional fields which later get collapsed to null by
readShippingRecipientFieldFromSnapshot(), causing mismatched hashes; update the
recipient construction in checkout hashing so that email and comment are first
trimmed and then normalized to null when they are empty (i.e., treat "" or
all-whitespace as null) before computing the request hash—apply this
normalization to args.shipping.recipient.email and
args.shipping.recipient.comment so their hashed representation matches
readShippingRecipientFieldFromSnapshot().
---
Nitpick comments:
In `@frontend/lib/tests/shop/admin-shipping-edit.test.ts`:
- Around line 332-389: After asserting the INVALID_SHIPPING_ADDRESS rejection,
additionally query the current order shipping snapshot (select
orderShipping.shippingAddress for the orderId, or join orders -> orderShipping
if your schema uses a separate table) and assert it equals the original seed
shipping address (use the seed's shippingAddress /
seed.orderShipping.shippingAddress value); this mirrors the immutability check
used in the 409 test and ensures applyAdminOrderShippingEdit (the call in this
test) does not mutate orderShipping.shippingAddress on validation failure.
In `@frontend/lib/tests/shop/checkout-order-created-notification-phase5.test.ts`:
- Around line 184-198: cleanupOrphanOrderCreatedArtifacts() uses raw SQL with
hardcoded table and column names (notification_outbox.template_key,
source_domain, order_id and payment_events.event_name, event_source) which makes
the test fragile to schema renames; add a concise comment above the function
explaining that this is test-only cleanup that intentionally uses raw SQL and
depends on current column/table names, citing which columns/tables are used so
future maintainers know to update the cleanup if the schema changes.
- Around line 152-182: The test file duplicates helper functions
loadOrderOutboxRow and runNotificationWorkerUntilSent already present in
status-notifications-phase5.test.ts; extract these helpers into a shared test
utility module (e.g., tests/helpers/notificationOutbox.ts) and import them in
both test files, keeping function signatures (loadOrderOutboxRow,
runNotificationWorkerUntilSent) and behavior identical; update the test imports
to use the new module and remove the duplicate implementations from
frontend/lib/tests/shop/checkout-order-created-notification-phase5.test.ts.
In `@frontend/lib/tests/shop/shipping-shipments-worker-phase5.test.ts`:
- Around line 1560-1616: Add a short explanatory comment near the test's
db.execute interception (around executeSpy.mockImplementation / originalExecute)
that explains this fragile SQL-parsing approach: state that the test simulates a
race between the shipment update and order update inside the same CTE, describe
which SQL elements the matcher relies on (presence of "update shipping_shipments
s", "provider_ref =", "tracking_number =", and the subsequent "update orders"
CTE/statement), and warn that changing the query chunking or CTE structure will
break the interception and tests; reference executeSpy, originalExecute,
db.execute, sql and seed.shipmentId/seed.orderId in the comment so future
maintainers can locate and understand the coupling.
In `@frontend/lib/tests/shop/status-notifications-phase5.test.ts`:
- Around line 381-382: The test contains a trivially true assertion: replace or
augment expect(secondProjectorRun.scanned).toBeGreaterThanOrEqual(0) with an
idempotency check that verifies no new rows were inserted on the second
projector run; specifically assert secondProjectorRun.inserted === 0 (or use
expect(secondProjectorRun.inserted).toBe(0)) so the test verifies the projector
is idempotent while keeping the firstProjectorRun.scanned assertion as-is.
- Around line 130-157: The two helper functions loadOrderOutboxRow and
runNotificationWorkerUntilSent are duplicated between
status-notifications-phase5.test.ts and
checkout-order-created-notification-phase5.test.ts; extract them into a new
shared test utility module (e.g.,
frontend/lib/tests/shop/helpers/notification-test-utils.ts) exporting those
functions, update both test files to import them from that module, and ensure
any dependent symbols used inside (db, notificationOutbox,
runNotificationOutboxWorker, crypto) are either imported into the helper module
or passed in as parameters so the helpers remain self-contained and compile in
both tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9e037f67-263a-461f-b09e-42c34ed71976
📒 Files selected for processing (38)
frontend/app/api/shop/admin/products/[id]/status/route.tsfrontend/app/api/shop/checkout/route.tsfrontend/lib/services/errors.tsfrontend/lib/services/orders/checkout.tsfrontend/lib/services/orders/restock.tsfrontend/lib/services/products/mutations/toggle.tsfrontend/lib/services/shop/shipping/admin-edit.tsfrontend/lib/services/shop/shipping/shipments-worker.tsfrontend/lib/tests/shop/admin-order-lifecycle-audit-reliability.test.tsfrontend/lib/tests/shop/admin-product-canonical-audit-phase5.test.tsfrontend/lib/tests/shop/admin-product-stock-protection.test.tsfrontend/lib/tests/shop/admin-shipping-edit.test.tsfrontend/lib/tests/shop/checkout-authoritative-price-minor.test.tsfrontend/lib/tests/shop/checkout-concurrency-stock1.test.tsfrontend/lib/tests/shop/checkout-currency-policy.test.tsfrontend/lib/tests/shop/checkout-inactive-after-cart.test.tsfrontend/lib/tests/shop/checkout-monobank-happy-path.test.tsfrontend/lib/tests/shop/checkout-monobank-idempotency-contract.test.tsfrontend/lib/tests/shop/checkout-monobank-parse-validation.test.tsfrontend/lib/tests/shop/checkout-no-payments.test.tsfrontend/lib/tests/shop/checkout-order-created-notification-phase5.test.tsfrontend/lib/tests/shop/checkout-price-change-fail-closed.test.tsfrontend/lib/tests/shop/checkout-rate-limit-policy.test.tsfrontend/lib/tests/shop/checkout-route-stripe-disabled-recovery.test.tsfrontend/lib/tests/shop/checkout-route-validation-contract.test.tsfrontend/lib/tests/shop/checkout-shipping-authoritative-total.test.tsfrontend/lib/tests/shop/checkout-shipping-phase3.test.tsfrontend/lib/tests/shop/checkout-stripe-payments-disabled.test.tsfrontend/lib/tests/shop/logging-redaction-real-flows.test.tsfrontend/lib/tests/shop/monobank-psp-unavailable.test.tsfrontend/lib/tests/shop/order-items-snapshot-immutable.test.tsfrontend/lib/tests/shop/order-items-variants.test.tsfrontend/lib/tests/shop/orders-status-ownership.test.tsfrontend/lib/tests/shop/product-sale-invariant.test.tsfrontend/lib/tests/shop/restock-order-only-once.test.tsfrontend/lib/tests/shop/shipping-shipments-worker-phase5.test.tsfrontend/lib/tests/shop/status-notifications-phase5.test.tsfrontend/lib/tests/shop/test-legal-consent.ts
✅ Files skipped from review due to trivial changes (3)
- frontend/lib/tests/shop/checkout-authoritative-price-minor.test.ts
- frontend/lib/tests/shop/product-sale-invariant.test.ts
- frontend/lib/services/products/mutations/toggle.ts
🚧 Files skipped from review as they are similar to previous changes (13)
- frontend/lib/tests/shop/monobank-psp-unavailable.test.ts
- frontend/lib/tests/shop/checkout-monobank-idempotency-contract.test.ts
- frontend/lib/tests/shop/orders-status-ownership.test.ts
- frontend/lib/tests/shop/logging-redaction-real-flows.test.ts
- frontend/lib/tests/shop/order-items-variants.test.ts
- frontend/lib/tests/shop/checkout-monobank-happy-path.test.ts
- frontend/lib/tests/shop/test-legal-consent.ts
- frontend/lib/tests/shop/checkout-stripe-payments-disabled.test.ts
- frontend/lib/tests/shop/admin-product-stock-protection.test.ts
- frontend/lib/tests/shop/checkout-inactive-after-cart.test.ts
- frontend/lib/tests/shop/checkout-currency-policy.test.ts
- frontend/lib/tests/shop/checkout-monobank-parse-validation.test.ts
- frontend/lib/tests/shop/admin-order-lifecycle-audit-reliability.test.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
frontend/lib/tests/shop/checkout-order-created-notification-phase5.test.ts (1)
152-182: Consider extracting shared test utilities.The
loadOrderOutboxRowandrunNotificationWorkerUntilSenthelpers are duplicated across test files. Consider extracting to a shared test utility module to reduce maintenance burden.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/tests/shop/checkout-order-created-notification-phase5.test.ts` around lines 152 - 182, Extract the duplicated helpers loadOrderOutboxRow and runNotificationWorkerUntilSent into a shared test utility module and update tests to import them: move the logic that queries db and notificationOutbox (using symbols db, notificationOutbox, eq) and the loop that calls runNotificationOutboxWorker into a new helper file (e.g., tests/utils/notificationOutbox.ts), export loadOrderOutboxRow and runNotificationWorkerUntilSent, ensure runNotificationWorkerUntilSent still calls runNotificationOutboxWorker with the same options, and replace the duplicate implementations in this and other test files with imports from the new module.frontend/lib/tests/shop/admin-shipping-edit.test.ts (1)
47-67: Consider type-safe test fixtures.The
as anycasts ondb.insert()calls throughout this file (lines 55, 67, 86, 117, 129) bypass TypeScript's type checking. While this is a common pattern in tests to allow partial inserts, it can hide issues if the schema evolves and required fields change.Consider defining a test fixture type or using a factory function that explicitly handles optional fields with defaults, which would catch schema drift at compile time.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/tests/shop/admin-shipping-edit.test.ts` around lines 47 - 67, The tests use "as any" on db.insert calls for npCities and npWarehouses which bypasses TypeScript checks; replace these casts by introducing explicit test-fixture types or factory functions (e.g., createTestCity, createTestWarehouse) that return correctly typed objects (use Partial<CityRecord> or the ORM's Insert type) with sensible defaults for optional fields, then call db.insert(npCities).values(createTestCity({...})) and db.insert(npWarehouses).values(createTestWarehouse({...})); update other inserts in the file to use the same factories so schema changes are caught at compile time.frontend/lib/tests/shop/shipping-shipments-worker-phase5.test.ts (2)
1560-1621: Fragile SQL interception acknowledged but consider additional safeguards.The inline comment (lines 1560-1564) correctly documents that this interception is tied to the CTE structure. While necessary for testing race conditions, consider adding a safeguard that fails the test explicitly if the interception doesn't match, rather than silently falling through to
originalExecute:💡 Optional: Add explicit assertion that interception occurred
+ let intercepted = false; + executeSpy.mockImplementation((async (query: unknown) => { // ... existing pattern matching ... if ( sqlText.includes('update shipping_shipments s') && sqlText.includes('provider_ref =') && sqlText.includes('tracking_number =') ) { + intercepted = true; // ... existing interception logic ... } return originalExecute(query as any); }) as typeof db.execute); const result = await runShippingShipmentsWorker({...}); + // Fail explicitly if the CTE pattern changed and interception didn't occur + expect(intercepted).toBe(true);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/tests/shop/shipping-shipments-worker-phase5.test.ts` around lines 1560 - 1621, The fragile executeSpy.mockImplementation interception may silently not trigger; add an interception flag (e.g., interceptionOccurred) inside the executeSpy.mockImplementation branch that updates shipping_shipments and orders (the block using originalExecute and seed.shipmentId/seed.orderId) and then assert that interceptionOccurred is true at the end of the test (or throw an error) so the test fails explicitly if the interception never ran; update references around executeSpy.mockImplementation, sqlText, originalExecute, seed.shipmentId and seed.orderId to locate and implement the flag and final assertion.
1122-1137: Consider typed test fixtures to reduceas anyusage.Multiple
as anytype assertions are used when inserting test data (e.g., lines 1137, 1238, 1265, 1392). While acceptable in tests, a typed fixture builder pattern could catch schema drift earlier:// Example: typed event builder function buildInternalCarrierEvent(args: { orderId: string; shipmentId: string; eventName: 'carrier_create_requested_internal' | 'carrier_create_succeeded_internal'; // ... other typed fields }): typeof shippingEvents.$inferInsert { return { ... }; }This is a minor suggestion for maintainability if these patterns expand.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/tests/shop/shipping-shipments-worker-phase5.test.ts` around lines 1122 - 1137, The test uses many unsafe "as any" casts when inserting into shippingEvents (e.g., the insert block that includes orderId, shipmentId, provider, eventName, payload and dedupeKey built by buildCarrierCreateRequestDedupeKeyForTest), which hides schema drift; create a typed fixture/builder function (e.g., buildInternalCarrierEvent or similar) that accepts a strongly-typed argument object matching shippingEvents.$inferInsert (or the table insert type) and returns a correctly typed insert payload, then replace direct insert objects with calls to that builder to remove the "as any" casts and ensure compile-time checks.
🤖 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/app/api/shop/checkout/route.ts`:
- Around line 104-126: The stock-specific mapping for Monobank errors is being
shadowed by the generic "if (code)" handler because
INSUFFICIENT_STOCK/OUT_OF_STOCK were added to
CHECKOUT_UNPROCESSABLE_ERROR_CODES; to fix, move the dedicated stock branch in
mapMonobankCheckoutError (the branch that inspects Monobank stock error payloads
and returns OUT_OF_STOCK/INSUFFICIENT_STOCK) so it executes before the generic
"if (code)" return, ensuring stock normalization runs first; apply the same
reordering to the other occurrence of the same mapping logic referenced later in
mapMonobankCheckoutError (the duplicated block around the 398–416 area).
In `@frontend/lib/services/orders/checkout.ts`:
- Around line 948-952: The snapshot for the repair path is being built with a
fresh consentedAt (requestedLegalConsentSnapshot) which stamps repairs with the
replay time; instead, when performing the grace-window backfill use the original
acceptance time from the existing order record. Change the calls that create the
repair snapshot (the buildPreparedLegalConsentSnapshot invocation that uses
requestedLegalConsentHashRefs/locale/country and the similar call around the
other spot noted) to pass an explicit consentedAt taken from the existing
order's stored consent timestamp (e.g., the order/orderLegalConsents original
consentedAt field) rather than letting the helper generate a new timestamp.
- Around line 944-960: Move the existing-order "fast path" (the call to
getOrderByIdempotencyKey) to run immediately after you compute the idempotency
key/hash (hashIdempotencyRequest) and after replay normalization
(normalizedItems, requestedLegalConsentHashRefs, preparedShipping.hashRefs) but
before any live authoritative validations (catalog/pricing/shipping quote
checks); in practice, split normalization from new-order validation by computing
requestedLegalConsentHashRefs via resolveRequestedCheckoutLegalConsentHashRefs
and building requestedLegalConsentSnapshot, then call hashIdempotencyRequest and
immediately call getOrderByIdempotencyKey to return stored orders for retries,
and only if no stored order exists proceed to run the live validation steps that
reference pricing/shipping/catalog so retries aren’t blocked by transient state.
---
Nitpick comments:
In `@frontend/lib/tests/shop/admin-shipping-edit.test.ts`:
- Around line 47-67: The tests use "as any" on db.insert calls for npCities and
npWarehouses which bypasses TypeScript checks; replace these casts by
introducing explicit test-fixture types or factory functions (e.g.,
createTestCity, createTestWarehouse) that return correctly typed objects (use
Partial<CityRecord> or the ORM's Insert type) with sensible defaults for
optional fields, then call db.insert(npCities).values(createTestCity({...})) and
db.insert(npWarehouses).values(createTestWarehouse({...})); update other inserts
in the file to use the same factories so schema changes are caught at compile
time.
In `@frontend/lib/tests/shop/checkout-order-created-notification-phase5.test.ts`:
- Around line 152-182: Extract the duplicated helpers loadOrderOutboxRow and
runNotificationWorkerUntilSent into a shared test utility module and update
tests to import them: move the logic that queries db and notificationOutbox
(using symbols db, notificationOutbox, eq) and the loop that calls
runNotificationOutboxWorker into a new helper file (e.g.,
tests/utils/notificationOutbox.ts), export loadOrderOutboxRow and
runNotificationWorkerUntilSent, ensure runNotificationWorkerUntilSent still
calls runNotificationOutboxWorker with the same options, and replace the
duplicate implementations in this and other test files with imports from the new
module.
In `@frontend/lib/tests/shop/shipping-shipments-worker-phase5.test.ts`:
- Around line 1560-1621: The fragile executeSpy.mockImplementation interception
may silently not trigger; add an interception flag (e.g., interceptionOccurred)
inside the executeSpy.mockImplementation branch that updates shipping_shipments
and orders (the block using originalExecute and seed.shipmentId/seed.orderId)
and then assert that interceptionOccurred is true at the end of the test (or
throw an error) so the test fails explicitly if the interception never ran;
update references around executeSpy.mockImplementation, sqlText,
originalExecute, seed.shipmentId and seed.orderId to locate and implement the
flag and final assertion.
- Around line 1122-1137: The test uses many unsafe "as any" casts when inserting
into shippingEvents (e.g., the insert block that includes orderId, shipmentId,
provider, eventName, payload and dedupeKey built by
buildCarrierCreateRequestDedupeKeyForTest), which hides schema drift; create a
typed fixture/builder function (e.g., buildInternalCarrierEvent or similar) that
accepts a strongly-typed argument object matching shippingEvents.$inferInsert
(or the table insert type) and returns a correctly typed insert payload, then
replace direct insert objects with calls to that builder to remove the "as any"
casts and ensure compile-time checks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 51d62588-9c7e-4818-b356-f13c580f2087
📒 Files selected for processing (9)
frontend/app/api/shop/checkout/route.tsfrontend/lib/services/orders/checkout.tsfrontend/lib/tests/shop/admin-shipping-edit.test.tsfrontend/lib/tests/shop/checkout-legal-consent-phase4.test.tsfrontend/lib/tests/shop/checkout-order-created-notification-phase5.test.tsfrontend/lib/tests/shop/checkout-route-validation-contract.test.tsfrontend/lib/tests/shop/checkout-shipping-phase3.test.tsfrontend/lib/tests/shop/shipping-shipments-worker-phase5.test.tsfrontend/lib/tests/shop/status-notifications-phase5.test.ts
Comment on lines
948
to
952
| const requestedLegalConsentSnapshot = buildPreparedLegalConsentSnapshot({ | ||
| hashRefs: requestedLegalConsentHashRefs, | ||
| locale, | ||
| country: country ?? null, | ||
| }); |
Contributor
There was a problem hiding this comment.
Don't stamp repaired consent evidence with the replay time.
requestedLegalConsentSnapshot is created with a fresh consentedAt, then reused when the grace-window repair path backfills a missing orderLegalConsents row. A replay therefore persists the repair time, not the original acceptance time, which weakens the legal artifact you're reconstructing. Build that repair snapshot from the existing order's original timestamp instead of reusing the new-order snapshot.
Also applies to: 1013-1017
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/lib/services/orders/checkout.ts` around lines 948 - 952, The
snapshot for the repair path is being built with a fresh consentedAt
(requestedLegalConsentSnapshot) which stamps repairs with the replay time;
instead, when performing the grace-window backfill use the original acceptance
time from the existing order record. Change the calls that create the repair
snapshot (the buildPreparedLegalConsentSnapshot invocation that uses
requestedLegalConsentHashRefs/locale/country and the similar call around the
other spot noted) to pass an explicit consentedAt taken from the existing
order's stored consent timestamp (e.g., the order/orderLegalConsents original
consentedAt field) rather than letting the helper generate a new timestamp.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
frontend/lib/tests/shop/admin-shipping-edit.test.ts (1)
433-517: Consider adding audit log verification for consistency.This test thoroughly verifies fulfillment snapshot coherence but omits the audit log assertion present in the first test (lines 223-235). For consistency across successful edit scenarios, consider adding:
expect(shippingRow?.shippingAddress).toMatchObject({ methodCode: 'NP_WAREHOUSE', quote: { currency: 'UAH', amountMinor: 100, quoteFingerprint: `quote_${seed.orderId}`, }, recipient: { fullName: 'Queue Safe', phone: '+380931112233', email: 'queue@example.com', comment: 'Use the side entrance', }, }); + + const auditRows = await db + .select({ + action: adminAuditLog.action, + }) + .from(adminAuditLog) + .where(eq(adminAuditLog.orderId, seed.orderId)); + + expect(auditRows).toHaveLength(1); + expect(auditRows[0]?.action).toBe('order_admin_action.edit_shipping'); } finally {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/tests/shop/admin-shipping-edit.test.ts` around lines 433 - 517, After the shipping assertions, add an audit-log assertion for the edit triggered by applyAdminOrderShippingEdit: query the admin/audit table for an entry tied to seed.orderId (and the requestId used) showing the shipping/edit action (e.g., 'update_shipping' or equivalent), with actorUserId null and payload/details containing the recipient fields (fullName, phone, email, comment) and the methodCode change to 'NP_WAREHOUSE'; use the same seed.orderId and requestId values from the test to locate the log and assert the audit entry exists and matches the expected change before calling cleanup(seed).frontend/app/api/shop/checkout/route.ts (1)
1353-1355: Type assertion bridges schema optionality.The
as Parameters<...>cast is necessary because the Zod schema allowslegalConsentto be optional (for validation reporting), whilecreateOrderWithItemsexpects it. Ifundefinedpasses through, the service throwsLEGAL_CONSENT_REQUIRED.Consider a runtime guard for clarity:
if (!legalConsent) { return errorResponse('LEGAL_CONSENT_REQUIRED', 'Legal consent is required.', 422); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/api/shop/checkout/route.ts` around lines 1353 - 1355, The code currently casts legalConsent to satisfy createOrderWithItems even though the Zod schema marks it optional, which can let undefined reach the service and cause a LEGAL_CONSENT_REQUIRED error; add an explicit runtime guard before calling createOrderWithItems that checks the legalConsent variable and returns an errorResponse (e.g., using errorResponse('LEGAL_CONSENT_REQUIRED', 'Legal consent is required.', 422)) when it's missing so only a validated, defined legalConsent is passed into createOrderWithItems.frontend/lib/tests/shop/checkout-shipping-phase3.test.ts (1)
115-131: Minor cleanup robustness consideration.The cleanup deletes
orderItemsbyproductId(line 124), but this assumes all test orders use only the seeded product. Consider also deletingorderItemsbyorderIdfor each order in theorderIdsarray to ensure complete cleanup if future tests involve multiple products.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/tests/shop/checkout-shipping-phase3.test.ts` around lines 115 - 131, In cleanupSeedData update the orderItems cleanup to also remove items by orderId so leftover items from orders that reference other products are removed; specifically, in the async function cleanupSeedData add/delete using orderIds (loop or a WHERE IN on orderItems.orderId) in addition to the existing where(eq(orderItems.productId, data.productId)) call, and ensure this runs before deleting orders (references: cleanupSeedData, orderItems, orders, orderIds).frontend/lib/tests/shop/shipping-shipments-worker-phase5.test.ts (1)
219-276: Cover recipient-based hash drift explicitly.
buildCarrierCreatePayloadIdentity()canonicalizes recipient name/phone infrontend/lib/services/shop/shipping/shipments-worker.ts:1-100, but this “authoritative” builder hard-codes those fields and the drift case below only changesselection.warehouseRef. A regression that drops recipient fields from the carrier-create fingerprint would still pass here. Please read the recipient fromshippingAddressand add a sibling drift mutation onrecipient.fullNameorrecipient.phone.♻️ Suggested helper alignment
const selection = shippingAddress?.selection as | Record<string, unknown> | undefined; + const recipient = shippingAddress?.recipient as + | Record<string, unknown> + | undefined; @@ recipient: { cityRef: selection?.cityRef as string, warehouseRef: selection?.warehouseRef as string, addressLine1: null, addressLine2: null, - fullName: 'Test User', - phone: '+380501112233', + fullName: recipient?.fullName as string, + phone: recipient?.phone as string, },Also applies to: 1211-1338
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/tests/shop/shipping-shipments-worker-phase5.test.ts` around lines 219 - 276, The authoritative payload builder buildAuthoritativeNovaPoshtaRequestPayload currently hard-codes recipient.fullName and recipient.phone which masks fingerprint drift tests; change it to read recipient data from the shippingAddress (use shippingAddress.selection and/or shippingAddress.recipient if present) and use those values for recipient.fullName and recipient.phone instead of literals, and update the associated drift mutation in the test to also vary recipient.fullName or recipient.phone (mirror how buildCarrierCreatePayloadIdentity canonicalizes recipient name/phone) so recipient-based hash drift is covered; ensure you reference the recipient fields in the returned object inside buildAuthoritativeNovaPoshtaRequestPayload and add a sibling mutation that toggles fullName or phone when simulating drift.
🤖 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/lib/tests/shop/shipping-shipments-worker-phase5.test.ts`:
- Around line 1100-1103: The test currently only checks unique success keys via
carrierSuccessOutcomeKeys(readInternalCarrierEvents(seed.shipmentId)).size,
which misses duplicate rows; after reading internalEventsAfterReplay, also count
the raw occurrences of the event type "carrier_create_succeeded_internal" (e.g.,
filter internalEventsAfterReplay by event.type or eventName equals
"carrier_create_succeeded_internal") and assert that this raw count equals 1 to
ensure replay did not append a duplicate row.
---
Nitpick comments:
In `@frontend/app/api/shop/checkout/route.ts`:
- Around line 1353-1355: The code currently casts legalConsent to satisfy
createOrderWithItems even though the Zod schema marks it optional, which can let
undefined reach the service and cause a LEGAL_CONSENT_REQUIRED error; add an
explicit runtime guard before calling createOrderWithItems that checks the
legalConsent variable and returns an errorResponse (e.g., using
errorResponse('LEGAL_CONSENT_REQUIRED', 'Legal consent is required.', 422)) when
it's missing so only a validated, defined legalConsent is passed into
createOrderWithItems.
In `@frontend/lib/tests/shop/admin-shipping-edit.test.ts`:
- Around line 433-517: After the shipping assertions, add an audit-log assertion
for the edit triggered by applyAdminOrderShippingEdit: query the admin/audit
table for an entry tied to seed.orderId (and the requestId used) showing the
shipping/edit action (e.g., 'update_shipping' or equivalent), with actorUserId
null and payload/details containing the recipient fields (fullName, phone,
email, comment) and the methodCode change to 'NP_WAREHOUSE'; use the same
seed.orderId and requestId values from the test to locate the log and assert the
audit entry exists and matches the expected change before calling cleanup(seed).
In `@frontend/lib/tests/shop/checkout-shipping-phase3.test.ts`:
- Around line 115-131: In cleanupSeedData update the orderItems cleanup to also
remove items by orderId so leftover items from orders that reference other
products are removed; specifically, in the async function cleanupSeedData
add/delete using orderIds (loop or a WHERE IN on orderItems.orderId) in addition
to the existing where(eq(orderItems.productId, data.productId)) call, and ensure
this runs before deleting orders (references: cleanupSeedData, orderItems,
orders, orderIds).
In `@frontend/lib/tests/shop/shipping-shipments-worker-phase5.test.ts`:
- Around line 219-276: The authoritative payload builder
buildAuthoritativeNovaPoshtaRequestPayload currently hard-codes
recipient.fullName and recipient.phone which masks fingerprint drift tests;
change it to read recipient data from the shippingAddress (use
shippingAddress.selection and/or shippingAddress.recipient if present) and use
those values for recipient.fullName and recipient.phone instead of literals, and
update the associated drift mutation in the test to also vary recipient.fullName
or recipient.phone (mirror how buildCarrierCreatePayloadIdentity canonicalizes
recipient name/phone) so recipient-based hash drift is covered; ensure you
reference the recipient fields in the returned object inside
buildAuthoritativeNovaPoshtaRequestPayload and add a sibling mutation that
toggles fullName or phone when simulating drift.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3dc6486d-2eac-408f-abb2-079e69c20f7f
📒 Files selected for processing (7)
frontend/app/api/shop/checkout/route.tsfrontend/lib/services/orders/checkout.tsfrontend/lib/tests/shop/admin-shipping-edit.test.tsfrontend/lib/tests/shop/checkout-legal-consent-phase4.test.tsfrontend/lib/tests/shop/checkout-route-validation-contract.test.tsfrontend/lib/tests/shop/checkout-shipping-phase3.test.tsfrontend/lib/tests/shop/shipping-shipments-worker-phase5.test.ts
✅ Files skipped from review due to trivial changes (1)
- frontend/lib/tests/shop/checkout-route-validation-contract.test.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
frontend/lib/tests/shop/shipping-shipments-worker-phase5.test.ts (1)
219-280: Avoid re-implementing the carrier-create payload builder in the test.This helper is now a second source of truth for the request shape and defaults. If the worker payload changes, these idempotency specs can start failing because the fixture drifted rather than because behavior regressed. Prefer reusing/exporting the production builder, or at least a shared fixture factory, here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/tests/shop/shipping-shipments-worker-phase5.test.ts` around lines 219 - 280, The test contains a duplicated payload builder (buildAuthoritativeNovaPoshtaRequestPayload) that mirrors production logic; remove this duplication and reuse the single source of truth by importing the production payload builder (or a shared fixture factory) instead of re-implementing it. Replace buildAuthoritativeNovaPoshtaRequestPayload with an import/ call to the production function (or move the current logic into a shared fixture module and import that), keeping the same signature (returns NovaPoshtaCreateTtnInput and accepts Seeded) and ensuring environment defaults remain sourced from the shared implementation so tests follow the real production behavior.
🤖 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/lib/tests/shop/checkout-shipping-phase3.test.ts`:
- Around line 706-708: The test currently deletes npWarehouses with
seed.warehouseRefA inside the test and cleanupSeedData later attempts to delete
it again; update the test/cleanup to avoid redundant deletes by either marking
that seed.warehouseRefA has been removed (e.g., set a flag or push the ref into
a "deletedSeeds" set) so cleanupSeedData skips already-deleted refs, or change
cleanupSeedData to first check existence before calling db.delete (i.e., query
npWarehouses for seed.warehouseRefA and only call db.delete if found); reference
npWarehouses, seed.warehouseRefA, cleanupSeedData and the
db.delete(...).where(eq(npWarehouses.ref, seed.warehouseRefA)) call when
implementing the guard or tracking.
In `@frontend/lib/tests/shop/shipping-shipments-worker-phase5.test.ts`:
- Around line 1455-1500: The two seeded `carrier_create_succeeded_internal` rows
are modeling different intents because they use different `canonicalHash` values
and omit the real `canonicalPayload`; to model a conflict they must share the
same canonical request identity. In the `shippingEvents` insert (the block that
creates two `carrier_create_succeeded_internal` entries and uses
`buildShippingEventDedupeKey`), set the same payload identity for both rows by
adding a `canonicalPayload` field (same object for both) and use the same
`canonicalHash` value in `payload` (and ensure
`payload.providerRef`/`trackingNumber` reflect the same canonical request if
needed); keep `eventRef` distinct if you want separate outcomes but ensure the
canonical fields are identical so the fixture represents two conflicting
outcomes for one intent.
---
Nitpick comments:
In `@frontend/lib/tests/shop/shipping-shipments-worker-phase5.test.ts`:
- Around line 219-280: The test contains a duplicated payload builder
(buildAuthoritativeNovaPoshtaRequestPayload) that mirrors production logic;
remove this duplication and reuse the single source of truth by importing the
production payload builder (or a shared fixture factory) instead of
re-implementing it. Replace buildAuthoritativeNovaPoshtaRequestPayload with an
import/ call to the production function (or move the current logic into a shared
fixture module and import that), keeping the same signature (returns
NovaPoshtaCreateTtnInput and accepts Seeded) and ensuring environment defaults
remain sourced from the shared implementation so tests follow the real
production behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 76ef4631-01f4-40b4-9bfc-fbbec583d759
📒 Files selected for processing (3)
frontend/lib/tests/shop/admin-shipping-edit.test.tsfrontend/lib/tests/shop/checkout-shipping-phase3.test.tsfrontend/lib/tests/shop/shipping-shipments-worker-phase5.test.ts
…nical identity (shipping worker tests)
ViktorSvertoka
approved these changes
Apr 4, 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 finalizes the Shop module hardening for launch readiness based on the repeat flow-by-flow audit (02.04.2026) :contentReference[oaicite:0]{index=0}.
This is an intentionally large, Shop-only integration PR.
The work was executed workstream-by-workstream (verify → fix → test), but is submitted as a single PR because:
The focus is strictly on:
No feature expansion or unrelated refactors are included.
Reviewer guide (how to read this PR)
This PR is large by file count but narrow by domain (Shop module only).
Recommended review order:
Each section below is independent and maps directly to the repeat audit.
Related Issue
Issue: #<issue_number>
Changes
🔴 Critical flows (launch blockers)
Payment correctness (F-04)
failed + succeededmismatch)Inventory integrity (F-08, F-09)
Fulfillment safety (F-06)
needs_attentionnot masked)Shipping consistency (F-07, F-05)
Checkout contract (F-03, F-02)
🔴 Legal & ops safety
Legal compliance (F-12)
SHOP_SELLER_ADDRESS)Ops safety (F-13)
🟡 Reliability (non-critical but required)
Admin operations reliability (F-10)
Customer communications (F-11)
order_createdorder_canceledshipped🟢 Cleanup / polish
Storefront (F-01)
SizeGuideAccordioncomponentKnown limitations (intentional)
These are conscious trade-offs and out of scope for this PR.
Database Changes (if applicable)
How Has This Been Tested?
Test strategy
Flow-based validation aligned with repeat audit:
Narrow, flow-specific test execution (no masking by full suite)
Verified:
All tests executed on local/test DB only (PowerShell flow) per working contract.
Screenshots (if applicable)
Checklist
Before submitting
Reviewers
Summary by CodeRabbit
New Features
Improvements
Documentation
Tests