(SP: 3) [Backend] add Nova Poshta shipping foundation + checkout persistence + async label workflow#364
Conversation
…ebhook + janitor (ORIGIN_BLOCKED)
…wnership test and pass all pre-prod invariants
…ests; minor security/log/UI cleanups
…s) and remove duplicate sha256 hashing
…lCount for orders badge
…obank logging safety invariant, namespace localStorage cart by user and reset on auth change
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
✅ Deploy Preview for develop-devlovers ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
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 end-to-end Nova Poshta shipping: DB schema/migrations, NP client and cache, shipping tables/workers/retention, admin shipping actions with CSRF, cart/checkout shipping UI and validation, API routes (public/admin/internal), logging/metrics/sanitizers, env updates, and extensive tests. Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(200,220,255,0.5)
participant Browser as Client
participant Cart as CartPageClient
participant ShippingAPI as /api/shop/shipping/*
participant CheckoutAPI as /api/shop/checkout
participant DB as Database
end
Browser->>Cart: load checkout page
Cart->>ShippingAPI: GET /methods or /np/cities (q)
ShippingAPI->>DB: check flags/cache
alt cache miss
ShippingAPI->>NPAPI: Nova Poshta client search
ShippingAPI->>DB: upsert cities/warehouses
end
ShippingAPI-->>Cart: return methods/cities
Browser->>Cart: submit checkout with shipping
Cart->>CheckoutAPI: POST /api/shop/checkout (shipping payload)
CheckoutAPI->>DB: validate refs, persist order + order_shipping snapshot
CheckoutAPI-->>Browser: order created
sequenceDiagram
rect rgba(220,255,220,0.5)
participant Provider as Payment Provider
participant WebhookAPI as /api/shop/webhooks/*
participant DB as Database
participant Worker as Shipments Worker
participant NPAPI as Nova Poshta API
end
Provider->>WebhookAPI: payment succeeded webhook
WebhookAPI->>DB: load order + inventoryStatus
alt inventory committed
WebhookAPI->>DB: set order.shippingStatus=queued, insert shipping_shipments
else not committed
WebhookAPI->>DB: set order.shippingStatus=pending
end
Worker->>DB: claim queued shipment
Worker->>DB: load order_shipping snapshot
Worker->>NPAPI: createInternetDocument (TTN)
alt success
Worker->>DB: mark shipment succeeded, update order.trackingNumber/status
Worker->>DB: record metric succeeded
else transient error
Worker->>DB: increment attemptCount, schedule retry, record metric retries
else final failure
Worker->>DB: mark needs_attention, record metric failed
end
sequenceDiagram
rect rgba(255,230,200,0.5)
participant AdminUI as Admin
participant AdminAPI as /api/shop/admin/orders/[id]/shipping
participant Auth as Auth/CSRF
participant DB as Database
end
AdminUI->>AdminAPI: POST action (retry|mark_shipped|mark_delivered) + CSRF
AdminAPI->>Auth: validate CSRF & admin auth
AdminAPI->>DB: load order & shipment state
AdminAPI->>DB: validate transition and apply updates (audit psp_metadata)
alt retry_label_creation
AdminAPI->>DB: reset shipment & requeue
else mark_shipped/mark_delivered
AdminAPI->>DB: update shipment & order shippingStatus
end
AdminAPI-->>AdminUI: return result
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c77e1912fd
ℹ️ 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".
| await markSucceeded({ | ||
| shipmentId: args.claim.id, | ||
| runId: args.runId, | ||
| providerRef: created.providerRef, | ||
| trackingNumber: created.trackingNumber, |
There was a problem hiding this comment.
processClaimedShipment calls markSucceeded but ignores its return value, even though markSucceeded only updates rows when lease_owner = runId. If the NP API call takes longer than leaseSeconds, another worker can reclaim the shipment after lease expiry; in that case this update becomes a no-op, yet this run still records a succeeded metric and returns 'succeeded', leaving DB state unadvanced and risking duplicate label creation on retries.
Useful? React with 👍 / 👎.
| <main className="mx-auto max-w-6xl px-4 py-8" aria-labelledby="order-title"> | ||
| <header className="flex items-start justify-between gap-4"> |
There was a problem hiding this comment.
A second <main> tree starts immediately after the first one begins, and large parts of the page are duplicated. This renders duplicate controls/content and repeats landmark IDs like order-title/items-title, which breaks page structure and makes the admin order detail view confusing and error-prone to use.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
frontend/lib/services/orders/monobank-janitor.ts (2)
332-347:⚠️ Potential issue | 🟡 Minor
toJob3CandidateRowdoes not map the newraw_sha256field.
Job3CandidateRownow includesraw_sha256: string | null(line 57), buttoJob3CandidateRowomits it from the returned object. The TypeScript compiler should flag this as a type error.This function is currently dead code (only called from the now-unused
claimJob3Events), so it has no runtime impact, but it will cause a compile error.Proposed fix
function toJob3CandidateRow(row: MonobankEventRow): Job3CandidateRow { return { id: row.id, invoice_id: typeof row.invoice_id === 'string' ? row.invoice_id : (row.invoice_id ?? null), attempt_id: typeof row.attempt_id === 'string' ? row.attempt_id : (row.attempt_id ?? null), provider_modified_at: row.provider_modified_at, received_at: row.received_at, raw_payload: row.raw_payload, + raw_sha256: typeof row.raw_sha256 === 'string' ? row.raw_sha256 : null, }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/services/orders/monobank-janitor.ts` around lines 332 - 347, The function toJob3CandidateRow is missing the new raw_sha256 field from Job3CandidateRow; update toJob3CandidateRow to include raw_sha256 by reading row.raw_sha256 from the MonobankEventRow and mapping it the same way as invoice_id/attempt_id (e.g., return the string when typeof row.raw_sha256 === 'string' otherwise use (row.raw_sha256 ?? null)) so the returned object matches the Job3CandidateRow type and the TypeScript error is resolved.
349-381: 🛠️ Refactor suggestion | 🟠 MajorRemove dead code:
claimJob3Events,markJob3EventFailed,toJob3CandidateRow, andreleaseEventLeaseThese functions are not called anywhere in the codebase. They depend on
claimNextMonobankEvent,MonobankEventRow, andJOB3_EVENT_ERROR_CODEwhich are also unreachable. Removing them will reduce maintenance burden and eliminate unused code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/services/orders/monobank-janitor.ts` around lines 349 - 381, Remove the unused dead code by deleting the functions claimJob3Events, markJob3EventFailed, toJob3CandidateRow, and releaseEventLease from monobank-janitor.ts, and also remove any now-unreferenced constants/types they depend on (claimNextMonobankEvent, MonobankEventRow, JOB3_EVENT_ERROR_CODE) if those are not used elsewhere; ensure imports or references to these symbols are cleaned up so there are no unused-import/type lint errors and run tests/TS build to confirm nothing else depends on them.frontend/app/[locale]/admin/shop/orders/[id]/page.tsx (1)
170-280:⚠️ Potential issue | 🔴 CriticalCritical: Duplicate JSX blocks will cause build/render failure.
The file contains both old and new versions of multiple JSX blocks, resulting in malformed JSX. Biome confirms 9 parse errors (unclosed
<header>,<div>,<dl>,<article>,<section>,<table>, etc.). Examples of duplication:
- Lines 170–179 (new
<main>/<header>) and 180–189 (old identical block)- Lines 191–197 (new Back link
<div>) and 198–204 (old identical block)- Lines 223–227 (new
<dl>) and 228–232 (old identical block)- Lines 461–488 (new
<thead>) and 489–516 (old identical block)- Lines 562–573 and 549–561 (duplicate closing tags)
Remove the duplicate (old) blocks so only one version of each JSX section remains. The file will not compile in its current state.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/`[locale]/admin/shop/orders/[id]/page.tsx around lines 170 - 280, The file contains duplicated JSX blocks (e.g., the duplicated <main> / <header> block, the repeated Back link <div>, the repeated <dl> Summary section, duplicated <thead> and duplicated closing tags) which breaks the JSX parse; open frontend/app/[locale]/admin/shop/orders/[id]/page.tsx, locate the duplicated sections around the Order header, the Back Link block, the Summary <dl> (where order.paymentStatus, totalFormatted, paymentProvider, paymentIntentId, idempotencyKey are rendered) and the table <thead>, and remove the older/duplicate copies so each JSX element (main, header, div, dl, article, section, thead, etc.) appears exactly once and remains properly nested/closed (keeping the version that includes RefundButton, order, and totalFormatted). Ensure no leftover duplicate closing tags remain and the component compiles.
🧹 Nitpick comments (28)
frontend/package.json (1)
37-40: Align@tiptap/coreversion floor with the other TipTap packages.
@tiptap/core@3.20.0is the current latest on npm. The three bumped packages now declare^3.20.0, but@tiptap/coreis left at^3.19.0. While npm's semver resolution will typically install 3.20.0 for^3.19.0, the declared minimum is inconsistent and may produce peer-dependency warnings in strict package managers (e.g., pnpm) if the updated packages' peer specs require^3.20.0. The 3.20.0 release notably fixed a runtime error when initializingCodeBlockLowlightby switching to a named export, preventingextend is not a functionerrors caused by ESM/CJS interop issues.♻️ Proposed fix
- "@tiptap/core": "^3.19.0", + "@tiptap/core": "^3.20.0", "@tiptap/extension-code-block-lowlight": "^3.20.0", "@tiptap/react": "^3.20.0", "@tiptap/starter-kit": "^3.20.0",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/package.json` around lines 37 - 40, Update the `@tiptap/core` dependency in package.json from "^3.19.0" to "^3.20.0" so it matches the other TipTap packages (`@tiptap/extension-code-block-lowlight`, `@tiptap/react`, `@tiptap/starter-kit`) and avoids peer-resolution/runtime issues; after editing the dependency entry for "@tiptap/core" run your package manager install (npm/yarn/pnpm) to refresh lockfiles and verify no peer-dependency warnings.frontend/lib/tests/shop/shipping-shipments-worker-phase5.test.ts (1)
145-151: Consider DRYing the worker invocation options.The same
runShippingShipmentsWorkerargument block is repeated across tests. A tiny helper reduces repetition and keeps defaults consistent.♻️ Suggested refactor
+const baseWorkerArgs = { + limit: 10, + leaseSeconds: 120, + baseBackoffSeconds: 10, +}; + +async function runWorker( + overrides: Partial<Parameters<typeof runShippingShipmentsWorker>[0]> = {} +) { + return runShippingShipmentsWorker({ + runId: crypto.randomUUID(), + maxAttempts: 5, + ...baseWorkerArgs, + ...overrides, + }); +}- const result = await runShippingShipmentsWorker({ - runId: crypto.randomUUID(), - limit: 10, - leaseSeconds: 120, - maxAttempts: 5, - baseBackoffSeconds: 10, - }); + const result = await runWorker();- const result = await runShippingShipmentsWorker({ - runId: crypto.randomUUID(), - limit: 10, - leaseSeconds: 120, - maxAttempts: 5, - baseBackoffSeconds: 10, - }); + const result = await runWorker();- const result = await runShippingShipmentsWorker({ - runId: crypto.randomUUID(), - limit: 10, - leaseSeconds: 120, - maxAttempts: 3, - baseBackoffSeconds: 10, - }); + const result = await runWorker({ maxAttempts: 3 });Also applies to: 207-213, 262-268
🤖 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 145 - 151, The repeated runShippingShipmentsWorker argument block should be extracted into a small helper to DRY the tests: add a function (e.g., makeRunShippingOpts or defaultRunShippingOpts) that returns the common object { runId: crypto.randomUUID(), limit: 10, leaseSeconds: 120, maxAttempts: 5, baseBackoffSeconds: 10 } and update each test invocation of runShippingShipmentsWorker to use that helper with object spread to override values when needed (e.g., runShippingShipmentsWorker({ ...defaultRunShippingOpts(), runId: crypto.randomUUID() })). Locate usages around the existing runShippingShipmentsWorker calls (current repeats at lines near the shown block and also at the other two occurrences) and replace them to keep defaults consistent.frontend/scripts/seed-np-local.ps1 (1)
24-25: Prefer Write-Output/Write-Information over Write-Host.
Write-Host can’t be redirected or captured cleanly and is flagged by PSScriptAnalyzer.Suggested adjustment
- Write-Host "Seeding NP local catalog from: $SeedPath" + Write-Information "Seeding NP local catalog from: $SeedPath"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/scripts/seed-np-local.ps1` around lines 24 - 25, Replace the non-redirectable Write-Host call with a redirectable cmdlet: use Write-Output (or Write-Information if you want a different stream) for the message that includes $SeedPath; update the line that currently calls Write-Host "Seeding NP local catalog from: $SeedPath" to use Write-Output (or Write-Information) so the message can be captured by redirection and PSScriptAnalyzer will not flag it.frontend/lib/services/shop/shipping/inventory-eligibility.ts (1)
24-28: Refactor to accept typed SQL expression instead of raw string.
The current API acceptscolumnReference: stringand usessql.raw(), which bypasses parameterization. Drizzle's recommended pattern is to interpolate Drizzle column/table objects directly intosql\``, which automatically escapes them as identifiers. This type-safe approach prevents accidental SQL injection if the function is called with untrusted input.Suggested refactor
-export function inventoryCommittedForShippingSql(columnReference: string): SQL { - return sql`${sql.raw(columnReference)} in (${sql.join( +export function inventoryCommittedForShippingSql(columnReference: SQL): SQL { + return sql`${columnReference} in (${sql.join( INVENTORY_COMMITTED_FOR_SHIPPING.map(value => sql`${value}`), sql`, ` )})`; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/services/shop/shipping/inventory-eligibility.ts` around lines 24 - 28, Change inventoryCommittedForShippingSql to accept a typed Drizzle SQL expression instead of a raw string: replace the parameter type from string to the appropriate SQL expression type (e.g., SQL or the Drizzle column type used in your codebase) and stop using sql.raw. Interpolate the passed expression directly into the template (e.g., sql`${columnReference}`) and keep returning SQL; retain the use of INVENTORY_COMMITTED_FOR_SHIPPING.map(...) for the values. Update any callers to pass the Drizzle column/table expression rather than a raw string.frontend/lib/services/orders/_shared.ts (1)
69-96: Prefer normalizing empty strings toundefinedto avoid leaking empty values downstream.
This keeps semantics consistent with “absent” vs “present but empty” and avoids persisting empty strings.Suggested adjustment
- const selectedSize = - normVariant(item.selectedSize) || - optionValue(normalizedOptions, ['size', 'selectedsize']); - const selectedColor = - normVariant(item.selectedColor) || - optionValue(normalizedOptions, ['color', 'selectedcolor']); + const selectedSizeRaw = + normVariant(item.selectedSize) || + optionValue(normalizedOptions, ['size', 'selectedsize']); + const selectedColorRaw = + normVariant(item.selectedColor) || + optionValue(normalizedOptions, ['color', 'selectedcolor']); + const selectedSize = selectedSizeRaw || undefined; + const selectedColor = selectedColorRaw || undefined; - const variantKey = normVariant(item.variantKey); + const variantKey = normVariant(item.variantKey) || undefined;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/services/orders/_shared.ts` around lines 69 - 96, normalizeCheckoutItem currently lets empty strings flow through (for selectedSize, selectedColor, variantKey and option values) which can leak empty values downstream; update the function to treat empty-string results as undefined by wrapping/normalizing outputs from normVariant and optionValue (or use a small helper like normalizeEmpty) so selectedSize, selectedColor, variantKey and any size/color entries placed into options become undefined instead of "". Call normVariant on item.variantKey and on optionValue results and ensure when building the options object you only spread size/color when the normalized value is truthy (not an empty string).frontend/lib/tests/shop/shipping-internal-np-sync-route-p2.test.ts (2)
20-28: Consider callingresetEnvCache()inbeforeEachfor consistency with sibling tests.The retention route test (
shipping-internal-retention-route-phase7.test.ts) imports and callsresetEnvCache()before each test to ensure cached env values are flushed aftervi.unstubAllEnvs()+ re-stubbing. This test file omits it. If more test cases are added later (e.g., testing the happy path with different flag combinations), stale cached env values could cause flaky results.Suggested change
+import { resetEnvCache } from '@/lib/env'; + // ... describe('internal shipping np sync route (phase 2)', () => { beforeEach(() => { vi.clearAllMocks(); vi.unstubAllEnvs(); + resetEnvCache(); vi.stubEnv('INTERNAL_JANITOR_SECRET', 'test-secret'); vi.stubEnv('SHOP_SHIPPING_ENABLED', 'true'); vi.stubEnv('SHOP_SHIPPING_NP_ENABLED', 'true'); vi.stubEnv('SHOP_SHIPPING_SYNC_ENABLED', 'true'); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/tests/shop/shipping-internal-np-sync-route-p2.test.ts` around lines 20 - 28, The beforeEach setup in the test suite should call resetEnvCache() after vi.unstubAllEnvs() to flush any cached environment lookups; update the beforeEach in the describe block (the beforeEach function that currently calls vi.clearAllMocks(), vi.unstubAllEnvs(), and vi.stubEnv(...)) to invoke resetEnvCache() immediately after vi.unstubAllEnvs() so subsequent vi.stubEnv calls are applied to a clean env cache and avoid flakiness.
30-51: Test coverage is minimal — only the unauthorized path is exercised.The single test case validates the 401 response when the internal secret is missing, which is a good guard-rail test. However, there's no coverage for the happy path, feature-flag-disabled path, invalid payload, or rate-limiting behavior of this route. Consider adding at least a feature-disabled test (returns 200 with
success: false, code: 'FEATURE_DISABLED') and a valid-sync test to confirm end-to-end wiring.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/tests/shop/shipping-internal-np-sync-route-p2.test.ts` around lines 30 - 51, Add tests in shipping-internal-np-sync-route-p2.test.ts to cover the happy and feature-disabled paths: create one test that stubs the feature-flag to disabled (the same setup that the route's POST handler checks) and asserts the response is 200 with JSON { success: false, code: 'FEATURE_DISABLED' } and that dbExecuteMock is not called, and another test that sends a valid POST to the POST handler with the internal secret header present and a valid payload and then asserts a 200 success response (or the expected success payload) and that dbExecuteMock was called with the expected params; reference the existing POST test harness and dbExecuteMock to reuse mocks and request construction.frontend/app/api/shop/admin/orders/[id]/route.ts (1)
50-56:maskEmailexposes the full domain — verify this is intentional.For
"ivan@example.com",maskEmailproduces"i***n@example.com", exposing the complete domain. This is common practice (domains are less sensitive), but worth confirming it aligns with your privacy policy, especially for custom/corporate email domains that could identify an organization.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/api/shop/admin/orders/`[id]/route.ts around lines 50 - 56, maskEmail currently reveals the full domain (maskEmail) — confirm this is intentional per privacy policy or change the implementation to also obfuscate the domain; specifically update the maskEmail function to either (a) leave as-is after a PR note confirming policy alignment, or (b) when masking the domain replace the domain part with a masked string (e.g., keep only the TLD or replace everything before/after the dot with ***) so "ivan@example.com" no longer exposes "example.com", and update any callers/tests that expect the old output accordingly.frontend/scripts/seed-np-local.sql (1)
1-3: Consider adding inline comments with decoded city/warehouse names for readability.The hex-encoded values are opaque. A short comment next to each record (e.g.,
-- Kyiv Local Test,-- Lviv Local Test) would help developers quickly understand the seed data without decoding hex manually.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/scripts/seed-np-local.sql` around lines 1 - 3, The seed file uses hex-encoded Cyrillic values (note the comment "Cyrillic values are injected via hex->UTF8" and the SET client_encoding = 'UTF8') which are hard to read; update each INSERT/record line in seed-np-local.sql to append a short inline comment with the decoded city/warehouse names (for example "-- Kyiv Local Test", "-- Lviv Local Test") so developers can understand entries at a glance without decoding the hex; keep comments brief and placed immediately after the hex value(s) for each record.frontend/lib/tests/shop/nova-poshta-client-network-failure.test.ts (1)
33-51: Consider using Vitest'srejectsmatcher for more idiomatic async error assertions.The try/catch + sentinel throw pattern works, but Vitest provides
expect(...).rejectswhich is more concise and avoids the risk of the sentinel being silently swallowed if the assertion logic itself throws.For example:
await expect(searchSettlements({ q: 'kyiv', page: 1, limit: 10 })) .rejects.toThrow(NovaPoshtaApiError);That said, the current approach does allow richer multi-property assertions on the caught error. If you want to keep the detailed checks, the existing pattern is acceptable.
Also applies to: 53-71
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/tests/shop/nova-poshta-client-network-failure.test.ts` around lines 33 - 51, Replace the try/catch + sentinel pattern with Vitest's async rejects matchers: call the promise once (e.g., const p = searchSettlements({ q: 'kyiv', page: 1, limit: 10 })), then use await expect(p).rejects.toBeInstanceOf(NovaPoshtaApiError) and await expect(p).rejects.toMatchObject({ code: 'NP_FETCH_FAILED', status: 0, message: 'fetch failed' }); keep the existing fetch mock (fetchMock) assertion with expect(fetchMock).toHaveBeenCalledTimes(1), and assert the cause by awaiting p.catch(e => expect((e as NovaPoshtaApiError & { cause?: unknown }).cause).toBe(fetchError)); apply the same change to the other test block that covers lines 53-71.frontend/app/api/shop/internal/shipping/shipments/run/route.ts (1)
27-44: Consider extracting shared internal-route helpers.
normalizeDate,noStoreJson,retryAfterSeconds, andacquireJobSlotare duplicated verbatim between this file andfrontend/app/api/shop/internal/shipping/np/sync/route.ts(and likely the retention route). Extracting them into a shared module (e.g.,@/lib/services/shop/shipping/internal-route-utils) would reduce duplication and make future changes easier to apply consistently.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/api/shop/internal/shipping/shipments/run/route.ts` around lines 27 - 44, The four helper functions normalizeDate, noStoreJson, retryAfterSeconds and acquireJobSlot are duplicated across internal shipping routes; extract them into a single shared module (e.g., create a new module like "@/lib/services/shop/shipping/internal-route-utils") and replace the local definitions in route.ts and the other files with imports from that module; ensure the shared module exports these named functions and that noStoreJson still sets the same headers (Cache-Control and X-Request-Id) and retryAfterSeconds and normalizeDate preserve their existing behavior and types so callers (including acquireJobSlot) continue to work unchanged.frontend/lib/services/shop/shipping/admin-actions.ts (2)
114-131:LIMIT 1on the shipments join may pick an arbitrary row if multiple shipments exist.
loadShippingStatejoinsshipping_shipmentsand takesLIMIT 1. If an order ever has multiple shipment records (e.g., a retry created a second row), the returnedshipment_id/shipment_statusmay not correspond to the most relevant one. Consider addingORDER BY s.created_at DESCbefore theLIMIT 1to ensure the latest shipment is used.♻️ Suggested fix
from orders o left join shipping_shipments s on s.order_id = o.id where o.id = ${orderId}::uuid + order by s.created_at desc nulls last limit 1🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/services/shop/shipping/admin-actions.ts` around lines 114 - 131, The query in loadShippingState returns an arbitrary shipment when multiple shipping_shipments rows exist; update the SQL in the sql`...` block inside loadShippingState to order shipments by newest first (e.g., add "ORDER BY s.created_at DESC" — or "ORDER BY s.created_at DESC NULLS LAST" if created_at can be NULL) before the "LIMIT 1" so the returned shipment_id/shipment_status in ShippingStateRow corresponds to the latest shipment record.
359-421: Add an explicit guard formark_deliveredaction.After the
retry_label_creationandmark_shippedbranches return, the remaining code implicitly handlesmark_deliveredwithout checkingargs.action. While TypeScript's union type currently guarantees only three values, this is fragile — adding a newShippingAdminActionvariant would silently fall into the delivery path.♻️ Suggested fix
+ if (args.action !== 'mark_delivered') { + throw new ShippingAdminActionError( + 'UNSUPPORTED_ACTION', + `Unsupported shipping admin action: ${args.action}`, + 400 + ); + } + if (state.shipping_status === 'delivered') {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/services/shop/shipping/admin-actions.ts` around lines 359 - 421, The code currently assumes any fall-through path is the mark_delivered action; add an explicit guard that checks args.action === 'mark_delivered' before running the delivery logic (the block that calls appendAuditEntry, updateOrderShippingStatus, loadShipmentStatus, and returns the delivered response). If args.action is not 'mark_delivered', throw a ShippingAdminActionError (e.g., 'UNSUPPORTED_SHIPPING_ACTION' or similar) so new ShippingAdminAction variants can't silently be handled as delivery; update the existing error messages accordingly and keep usage of updateOrderShippingStatus, appendAuditEntry, and loadShipmentStatus unchanged inside the guarded branch.frontend/app/[locale]/shop/cart/CartPageClient.tsx (1)
145-178: Consider extracting shipping state into a custom hook oruseReducer.There are 15+ new
useStatecalls for shipping-related state (methods, city query/options/selection, warehouse query/options/selection, recipient fields, delivery error). Extracting this into auseShippingForm()custom hook or collapsing related state into auseReducerwould significantly reduce complexity and improve testability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/`[locale]/shop/cart/CartPageClient.tsx around lines 145 - 178, The component currently contains many related shipping state variables (shippingMethods, shippingMethodsLoading, shippingAvailable, shippingReasonCode, selectedShippingMethod, cityQuery, cityOptions, selectedCityRef, selectedCityName, citiesLoading, warehouseQuery, warehouseOptions, selectedWarehouseRef, selectedWarehouseName, warehousesLoading, courierAddressLine1/2, recipientName/Phone/Email/Comment, deliveryUiError) which should be collapsed into a single custom hook (e.g. useShippingForm) or a useReducer-based state slice; implement useShippingForm to own all these pieces and expose state and updater handlers (getShippingState, setShippingState / specific setters like setCityQuery, selectCity, setWarehouseQuery, selectWarehouse, setRecipientField, clearDeliveryError, fetchShippingMethods) so the CartPageClient.tsx replaces the 15+ useState calls with a single hook call and uses the returned handlers; ensure the hook encapsulates async loading flags and error handling, keeps component API unchanged where possible, and add unit tests for the hook to validate state transitions (selectCity/selectWarehouse, fetchShippingMethods success/failure, recipient field updates).frontend/lib/services/shop/shipping/checkout-payload.ts (2)
73-78: Third copy of locale→country inference.
countryFromLocaleis the third implementation of the same'uk' → 'UA'mapping (also inavailability.tsasinferCountryFromLocaleandcheckout.tsasmapLocaleToCountry). All three normalize, split on[-_], and check for'uk'.This file is likely client-importable (no
'server-only'), so it can't import from server modules — but a shared utility in a common/shared location would prevent drift across the three implementations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/services/shop/shipping/checkout-payload.ts` around lines 73 - 78, countryFromLocale duplicates logic already present as inferCountryFromLocale and mapLocaleToCountry; extract the locale→country mapping into a single shared utility (e.g., a new client-safe module under a common/shared location) and replace the three implementations to import and call that function; ensure the new function preserves normalization/splitting behavior (handles trimOrNull semantics or inline equivalent) and the specific mapping ('uk' -> 'UA') and verify the module is client-importable so frontend code can safely import it instead of keeping three copies.
6-12:ShippingUnavailableReasonCodeincludes'OK', which is not an "unavailable" reason.The type name suggests it represents reasons for unavailability, but
'OK'and'INTERNAL_ERROR'don't fit that semantic. Consider renaming toShippingAvailabilityReasonCode(matchingavailability.ts) orShippingReasonCodeto better reflect the full range of values.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/services/shop/shipping/checkout-payload.ts` around lines 6 - 12, The type name ShippingUnavailableReasonCode is misleading because it includes non-unavailable values like 'OK' and 'INTERNAL_ERROR'; rename the type to a name reflecting both available and unavailable states (e.g., ShippingAvailabilityReasonCode or ShippingReasonCode) and update all references/imports/usages in this module and across the codebase (search for ShippingUnavailableReasonCode) to the new name, making sure any related files such as availability.ts that use the same enum-like values are aligned to the chosen name.frontend/db/schema/shop.ts (1)
160-168: Consider a CHECK constraint for shipping field consistency on orders.All shipping columns are nullable, which is correct for orders that don't require shipping. However, there's no DB-level guard ensuring that when
shipping_required = true, the essential fields (shipping_provider,shipping_method_code,shipping_status) are also populated, and conversely that whenshipping_requiredis false/null, no shipping fields are set.This is not blocking — the application layer enforces this today — but a
CHECKconstraint would provide defense-in-depth consistent with the pattern already used in this table (e.g.,orders_psp_fields_null_when_none).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/db/schema/shop.ts` around lines 160 - 168, Add a DB-level CHECK constraint on the orders schema to enforce shipping field consistency: when shippingRequired (shipping_required) is true, require shippingProvider (shipping_provider), shippingMethodCode (shipping_method_code) and shippingStatus (shipping_status) to be non-null, and when shippingRequired is false or null ensure those shipping fields are all null; implement this as a named CHECK similar to orders_psp_fields_null_when_none so it lives alongside existing constraints and validates shippingRequired, shipping_provider, shipping_method_code, and shipping_status together.frontend/lib/services/shop/shipping/availability.ts (2)
49-54: Unreachable fallback oncurrency
input.currencyis typed asCurrencyCode(non-nullable, non-optional), so the?? resolveCurrencyFromLocale(locale)branch on line 54 can never execute. This is dead code that might mislead readers into thinking currency can be absent.If the intent is to defensively handle untyped callers, consider widening the input type to
CurrencyCode | null | undefinedand documenting it; otherwise, drop the fallback.Proposed fix
- const currency = input.currency ?? resolveCurrencyFromLocale(locale); + const currency = input.currency;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/services/shop/shipping/availability.ts` around lines 49 - 54, The fallback for currency is dead code because input.currency is typed non-nullable CurrencyCode; update resolveShippingAvailability to stop using the nullish fallback — replace "const currency = input.currency ?? resolveCurrencyFromLocale(locale);" with "const currency = input.currency;" (or alternatively widen the ShippingAvailabilityInput currency type to CurrencyCode | null | undefined if you intend to accept missing currencies and keep resolveCurrencyFromLocale). Ensure changes are applied in the resolveShippingAvailability function and adjust types or remove the resolveCurrencyFromLocale call accordingly.
42-47: Duplicate locale→country inference logic
inferCountryFromLocalehere duplicatesmapLocaleToCountryinfrontend/lib/services/orders/checkout.ts(lines 327-331). Both split on[-_], lower-case, and map'uk'→'UA'. Consider extracting a shared helper to avoid drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/services/shop/shipping/availability.ts` around lines 42 - 47, The function inferCountryFromLocale duplicates logic from mapLocaleToCountry; extract a single shared helper (e.g., localeToCountry or mapLocaleToCountry) into a common utility module and have both inferCountryFromLocale and the checkout usage import and call that helper, preserving the behavior (split on /[-_]/, toLowerCase, map 'uk' -> 'UA' and return null otherwise); update imports in frontend/lib/services/shop/shipping/availability.ts and frontend/lib/services/orders/checkout.ts to use the shared function and run tests to ensure behavior is unchanged.frontend/lib/services/shop/shipping/shipments-worker.ts (3)
121-135: All non-HTTP NP API errors are classified as transient — may cause unnecessary retries for permanent failures.Lines 130–134: any
NovaPoshtaApiErrorthat isn'tNP_HTTP_ERRORorNP_INVALID_JSONfalls through to the default branch, which setstransient: true. However, some NP API error codes indicate permanent failures (e.g., invalid address, recipient data errors). These will be retried up tomaxAttemptsbefore finally being markedneeds_attention.Consider checking for known permanent NP error codes (e.g., validation-related
errorCodesfrom the NP envelope) and marking those astransient: falsefor faster escalation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/services/shop/shipping/shipments-worker.ts` around lines 121 - 135, The current NovaPoshtaApiError handler in the shipments worker treats all non-HTTP/invalid-json NP errors as transient; update the NovaPoshtaApiError branch (the code that returns { code: error.code || 'NP_API_ERROR', message: ..., transient: true }) to detect known permanent NP error indicators (e.g., validation/recipient/address error codes found on the error object like error.errorCodes, error.errors or error.data?.errorCodes) and set transient: false for those codes; implement a small set/list of permanent NP error codes and check membership before returning, so permanent failures return transient: false and others keep transient: true.
573-617: Worker processes claims sequentially — acceptable for now but may become a bottleneck.The
forloop on line 587 processes each claimed shipment serially. With the NP API call in each iteration, throughput is bounded by NP response latency × claim count. If the batch size grows, considerPromise.allSettledwith a concurrency limiter, though the current sequential approach is simpler and avoids overwhelming the NP API.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/services/shop/shipping/shipments-worker.ts` around lines 573 - 617, The loop in runShippingShipmentsWorker processes claimed shipments sequentially via awaiting processClaimedShipment for each claim, which limits throughput; change this to process claims in parallel with bounded concurrency (e.g., use Promise.allSettled combined with a concurrency limiter like p-limit or a simple worker-pool) when calling processClaimedShipment for each item returned by claimQueuedShipmentsForProcessing, collect all outcomes from the settled promises and then aggregate processed/succeeded/retried/needsAttention counts the same way before logging and returning the result to preserve behavior while increasing throughput.
268-270: Hardcoded brand name in shipment description.
"DevLovers order ${args.order.order_id}"embeds a brand name. Consider pulling this from config or an environment variable for portability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/services/shop/shipping/shipments-worker.ts` around lines 268 - 270, The shipment payload is using a hardcoded brand in the description ("DevLovers order ${args.order.order_id}"); update the description to read the brand from configuration/environment instead (e.g., use config.brandName or process.env.BRAND_NAME) so it becomes `${brand} order ${args.order.order_id}`; modify the code that builds the description field in shipments-worker.ts (the description property) to use that config value and provide a sensible default fallback if the config key is missing.frontend/lib/services/orders/checkout.ts (2)
711-746: Complex idempotency hash reconstruction with shipping — consider documenting the derivation logic.The hash derivation for existing orders (lines 711–746) reconstructs a normalized representation of the order, including the new shipping refs, from disparate sources (order row, order items, and the shipping snapshot). The
optionsobject construction (lines 719–726) with conditional spread and repeatednormVariantcalls is difficult to follow.This is correct but brittle — any change to how
hashIdempotencyRequestnormalizes its input must be mirrored here. Consider adding a comment or extracting aderiveExistingOrderHash(row, items, shipping)helper to isolate this logic.🤖 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 711 - 746, The idempotency hash reconstruction in the derivedExistingHash computation is complex and brittle; extract this logic into a dedicated helper (e.g., deriveExistingOrderHash) that accepts row, existing/items, existingCityRef, existingWarehouseRef and returns the normalized payload currently passed to hashIdempotencyRequest, reusing normVariant and resolvePaymentProvider inside the helper and centralizing the conditional options/size/color spreads and shipping branch; replace the inline block in the derivedExistingHash assignment with a call to that helper to improve readability and make future changes to hashIdempotencyRequest normalization easier to maintain.
650-652: Fix the type mismatch causing unsafe casting.
mergeCheckoutItemsreturnsCheckoutItem[]by its type signature but internally buildsCheckoutItemWithVariantobjects. This forces the caller to cast at line 652. SincenormalizeCheckoutItemrequiresCheckoutItemWithVariant, changemergeCheckoutItemsto returnCheckoutItemWithVariant[]to eliminate the unsafe cast:Type-safe fix
-export function mergeCheckoutItems(items: CheckoutItem[]): CheckoutItem[] { +export function mergeCheckoutItems(items: CheckoutItem[]): CheckoutItemWithVariant[] {Then in checkout.ts:650-652, remove the cast:
const normalizedItems = mergeCheckoutItems( items - ).map(item => normalizeCheckoutItem(item as CheckoutItemWithVariant)); + ).map(item => normalizeCheckoutItem(item));This also eliminates the duplicate cast already present in
mergeCheckoutItemsline 142.🤖 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 650 - 652, mergeCheckoutItems is typed to return CheckoutItem[] but actually builds CheckoutItemWithVariant objects, forcing unsafe casts where it's used; update the mergeCheckoutItems function signature to return CheckoutItemWithVariant[] and remove the redundant casts inside mergeCheckoutItems (the duplicate cast mentioned) and at its call site in checkout.ts where you currently cast to CheckoutItemWithVariant before calling normalizeCheckoutItem; this makes normalizeCheckoutItem(item: CheckoutItemWithVariant) type-safe without any manual casting.frontend/lib/services/shop/shipping/nova-poshta-client.ts (1)
279-285:isPostMachinerelies on string heuristics in category/type descriptions.If Nova Poshta changes their
CategoryOfWarehouseorTypeOfWarehousetext (e.g., uses Ukrainian-language descriptions), this check will silently stop detecting parcel lockers. Consider also checking forPostFinancetype or the Ukrainian equivalents (поштомат), or logging when neither category nor type matches known patterns so drift is observable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/services/shop/shipping/nova-poshta-client.ts` around lines 279 - 285, The isPostMachine heuristic in nova-poshta-client.ts currently only checks English substrings from readString(item.CategoryOfWarehouse) and readString(item.TypeOfWarehouse), which will break if Nova Poshta uses other terms; update isPostMachine to match against a localized set of patterns (e.g., include 'поштомат', 'поштомат', 'postfinance', 'postomat', 'parcel locker', etc.) by constructing a normalized array of known tokens and testing category/type membership, and add a lightweight diagnostic log (or telemetry hit) when neither field matches any known pattern so drift is observable; keep references to isPostMachine, CategoryOfWarehouse, TypeOfWarehouse, and readString to locate the change.frontend/lib/services/shop/shipping/nova-poshta-catalog.ts (2)
101-125: Avoid full-table scans whenqis blank/whitespace.
Ifqis empty, the query becomesILIKE '%%', which can scan the full active table. Consider short-circuiting to an empty result (or a dedicated “popular cities” path).♻️ Suggested guard
export async function findCachedCities(args: { q: string; limit: number; }): Promise<ShippingCity[]> { - const like = `%${args.q}%`; + const q = args.q.trim(); + if (!q) return []; + const like = `%${q}%`; const res = await db.execute<CityRow>(sql`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/services/shop/shipping/nova-poshta-catalog.ts` around lines 101 - 125, The findCachedCities function currently runs an ILIKE '%...%' query even when args.q is empty/whitespace causing full-table scans; add a guard at the start of findCachedCities to trim args.q and if it's empty return an empty ShippingCity[] (or route to a dedicated “popular cities” path) instead of executing the SQL against np_cities; keep the rest of the logic (mapCityRow, ordering, limit) unchanged and ensure the check uses args.q.trim() to catch whitespace-only inputs.
355-389: Consider avoiding refresh whenqmisses but cache is already populated.
As written, anyqwith zero matches will trigger a full NP fetch for the settlement, even if the local cache already contains warehouses. You could skip refresh when a settlement already has cached entries, and only refresh on true cache misses.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/services/shop/shipping/nova-poshta-catalog.ts` around lines 355 - 389, The function findWarehousesWithCacheOnMiss currently triggers cache refresh for any q that returns zero results even when the settlement cache is already populated; change the logic to avoid calling cacheWarehousesBySettlement when the settlement already has cached entries. Specifically, before calling cacheWarehousesBySettlement, call findCachedWarehouses for the same settlement with no q (or q undefined) and limit 1 to detect if any cached entries exist; only invoke cacheWarehousesBySettlement when that check returns zero (true cache miss). If the settlement has cached entries but the q search returned empty, return the empty result slice without refreshing; keep using the same identifiers (findWarehousesWithCacheOnMiss, findCachedWarehouses, cacheWarehousesBySettlement, runId) and preserve the existing error logging behavior when a refresh is actually attempted.frontend/drizzle/0017_shop_shipping_core.sql (1)
60-67: Consider defaults/backfill to avoid tri‑state ambiguity.
Adding nullableshipping_required/shipping_statuscan leave legacy rows in an undefined state. If the app treats null as false/pending, consider backfilling and setting defaults (e.g.,shipping_required = false,shipping_status = 'pending').🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/drizzle/0017_shop_shipping_core.sql` around lines 60 - 67, The migration adds nullable shipping columns that will leave existing rows ambiguous; update the 0017_shop_shipping_core.sql migration to backfill and set sensible defaults for the new fields on the "orders" table: set "shipping_required" default to false and backfill existing rows to false, set "shipping_status" default to 'pending' and backfill existing rows to 'pending' (and consider making them NOT NULL), and similarly consider default 0 for "shipping_amount_minor" if appropriate; perform the UPDATE/backfill before altering constraints so legacy rows are deterministic and then alter the columns to include DEFAULT and NOT NULL as needed.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
frontend/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (64)
frontend/.env.examplefrontend/app/[locale]/admin/shop/orders/[id]/RefundButton.tsxfrontend/app/[locale]/admin/shop/orders/[id]/ShippingActions.tsxfrontend/app/[locale]/admin/shop/orders/[id]/page.tsxfrontend/app/[locale]/shop/cart/CartPageClient.tsxfrontend/app/[locale]/shop/orders/[id]/page.tsxfrontend/app/api/shop/admin/orders/[id]/route.tsfrontend/app/api/shop/admin/orders/[id]/shipping/route.tsfrontend/app/api/shop/checkout/route.tsfrontend/app/api/shop/internal/shipping/np/sync/route.tsfrontend/app/api/shop/internal/shipping/retention/run/route.tsfrontend/app/api/shop/internal/shipping/shipments/run/route.tsfrontend/app/api/shop/orders/[id]/route.tsfrontend/app/api/shop/shipping/methods/route.tsfrontend/app/api/shop/shipping/np/cities/route.tsfrontend/app/api/shop/shipping/np/warehouses/route.tsfrontend/app/api/shop/webhooks/stripe/route.tsfrontend/db/queries/shop/admin-orders.tsfrontend/db/schema/shop.tsfrontend/drizzle/0017_shop_shipping_core.sqlfrontend/drizzle/meta/0017_snapshot.jsonfrontend/drizzle/meta/_journal.jsonfrontend/lib/env/index.tsfrontend/lib/env/nova-poshta.tsfrontend/lib/services/orders/_shared.tsfrontend/lib/services/orders/checkout.tsfrontend/lib/services/orders/monobank-janitor.tsfrontend/lib/services/orders/monobank-webhook.tsfrontend/lib/services/orders/payment-attempts.tsfrontend/lib/services/shop/shipping/admin-actions.tsfrontend/lib/services/shop/shipping/availability.tsfrontend/lib/services/shop/shipping/checkout-payload.tsfrontend/lib/services/shop/shipping/inventory-eligibility.tsfrontend/lib/services/shop/shipping/log-sanitizer.tsfrontend/lib/services/shop/shipping/metrics.tsfrontend/lib/services/shop/shipping/nova-poshta-catalog.tsfrontend/lib/services/shop/shipping/nova-poshta-client.tsfrontend/lib/services/shop/shipping/retention.tsfrontend/lib/services/shop/shipping/shipments-worker.tsfrontend/lib/tests/shop/admin-api-killswitch.test.tsfrontend/lib/tests/shop/admin-csrf-contract.test.tsfrontend/lib/tests/shop/checkout-currency-policy.test.tsfrontend/lib/tests/shop/checkout-shipping-phase3.test.tsfrontend/lib/tests/shop/monobank-janitor-job3.test.tsfrontend/lib/tests/shop/monobank-webhook-apply-outcomes.test.tsfrontend/lib/tests/shop/monobank-webhook-apply.test.tsfrontend/lib/tests/shop/nova-poshta-client-network-failure.test.tsfrontend/lib/tests/shop/shipping-checkout-payload-phase6.test.tsfrontend/lib/tests/shop/shipping-internal-np-sync-route-p2.test.tsfrontend/lib/tests/shop/shipping-internal-retention-route-phase7.test.tsfrontend/lib/tests/shop/shipping-log-sanitizer-phase7.test.tsfrontend/lib/tests/shop/shipping-methods-route-p2.test.tsfrontend/lib/tests/shop/shipping-np-cities-route-p2.test.tsfrontend/lib/tests/shop/shipping-np-warehouses-route-p2.test.tsfrontend/lib/tests/shop/shipping-retention-phase7.test.tsfrontend/lib/tests/shop/shipping-shipments-worker-phase5.test.tsfrontend/lib/tests/shop/stripe-webhook-psp-fields.test.tsfrontend/lib/types/shop.tsfrontend/lib/validation/shop-shipping.tsfrontend/lib/validation/shop.tsfrontend/package.jsonfrontend/scripts/np-mock-server.mjsfrontend/scripts/seed-np-local.ps1frontend/scripts/seed-np-local.sql
| const snapshot = await readStripePaymentIntentParams(orderId); | ||
| await db | ||
| .update(paymentAttempts) | ||
| .set({ | ||
| currency: snapshot.currency, | ||
| expectedAmountMinor: snapshot.amountMinor, | ||
| updatedAt: new Date(), | ||
| }) | ||
| .where(eq(paymentAttempts.id, nextAttempt.id)); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's find the file and understand its structure
find . -name "payment-attempts.ts" -type fRepository: DevLoversTeam/devlovers.net
Length of output: 121
🏁 Script executed:
#!/bin/bash
# Look at the function ensureStripePaymentIntentForOrder and surrounding context
rg -n "ensureStripePaymentIntentForOrder" -t ts --max-count=20Repository: DevLoversTeam/devlovers.net
Length of output: 1761
🏁 Script executed:
#!/bin/bash
# Find the implementation of readStripePaymentIntentParams
rg -n "readStripePaymentIntentParams" -t ts -A 10 --max-count=30Repository: DevLoversTeam/devlovers.net
Length of output: 6860
🏁 Script executed:
#!/bin/bash
# Search for OrderStateInvalidError to understand when it's thrown
rg -n "OrderStateInvalidError" -t ts -B 2 -A 2Repository: DevLoversTeam/devlovers.net
Length of output: 12076
🏁 Script executed:
#!/bin/bash
# Let's look at the full readStripePaymentIntentParams function to understand when it throws
sed -n '90,120p' frontend/lib/services/orders/payment-intent.tsRepository: DevLoversTeam/devlovers.net
Length of output: 1058
🏁 Script executed:
#!/bin/bash
# Look at the ensureStripePaymentIntentForOrder function and understand its structure
sed -n '187,320p' frontend/lib/services/orders/payment-attempts.tsRepository: DevLoversTeam/devlovers.net
Length of output: 3888
🏁 Script executed:
#!/bin/bash
# Check the first call site in checkout/route.ts
sed -n '800,850p' frontend/app/api/shop/checkout/route.tsRepository: DevLoversTeam/devlovers.net
Length of output: 1712
🏁 Script executed:
#!/bin/bash
# Check the second call site in checkout/route.ts
sed -n '950,1000p' frontend/app/api/shop/checkout/route.tsRepository: DevLoversTeam/devlovers.net
Length of output: 1470
🏁 Script executed:
#!/bin/bash
# Check the call site in payment page
sed -n '230,250p' frontend/app/\[locale\]/shop/checkout/payment/\[orderId\]/page.tsxRepository: DevLoversTeam/devlovers.net
Length of output: 702
🏁 Script executed:
#!/bin/bash
# Let's verify what happens at line 299 with the try-catch to confirm the pattern
sed -n '295,320p' frontend/lib/services/orders/payment-attempts.tsRepository: DevLoversTeam/devlovers.net
Length of output: 728
🏁 Script executed:
#!/bin/bash
# Check if there are any explicit paymentStatus checks in the function
grep -n "paymentStatus" frontend/lib/services/orders/payment-attempts.tsRepository: DevLoversTeam/devlovers.net
Length of output: 53
🏁 Script executed:
#!/bin/bash
# Let's check the try-catch error handling at line 299 to see if it handles OrderStateInvalidError specifically
sed -n '295,340p' frontend/lib/services/orders/payment-attempts.tsRepository: DevLoversTeam/devlovers.net
Length of output: 1208
Guard snapshot backfill against non-payable order states.
readStripePaymentIntentParams throws OrderStateInvalidError when paymentStatus is not in ['pending', 'requires_payment']. At lines 241 and 279, this call lacks error handling; if the order transitions to a non-payable state (e.g., already paid), the function will fail even though a PaymentIntent already exists. Line 299 includes a try-catch, creating inconsistency. Consider catching OrderStateInvalidError at lines 241 and 279 to allow the function to return the existing PI, matching the error handling pattern at line 299.
Potential fix (example)
- if (!attempt.currency || attempt.expectedAmountMinor === null) {
- const snapshot = await readStripePaymentIntentParams(orderId);
- await db
- .update(paymentAttempts)
- .set({
- currency: snapshot.currency,
- expectedAmountMinor: snapshot.amountMinor,
- updatedAt: new Date(),
- })
- .where(eq(paymentAttempts.id, attempt.id));
- }
+ if (!attempt.currency || attempt.expectedAmountMinor === null) {
+ try {
+ const snapshot = await readStripePaymentIntentParams(orderId);
+ await db
+ .update(paymentAttempts)
+ .set({
+ currency: snapshot.currency,
+ expectedAmountMinor: snapshot.amountMinor,
+ updatedAt: new Date(),
+ })
+ .where(eq(paymentAttempts.id, attempt.id));
+ } catch (error) {
+ if (!(error instanceof OrderStateInvalidError)) throw error;
+ }
+ }Also applies to: 278-287, 299-307
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/lib/services/orders/payment-attempts.ts` around lines 241 - 249,
Guard the call to readStripePaymentIntentParams against OrderStateInvalidError:
wrap the calls used when backfilling snapshot data (the invocation that feeds
the update on paymentAttempts and the other call before creating/returning a
PaymentIntent) in a try/catch that specifically catches OrderStateInvalidError,
and on that error reuse/return the existing PaymentIntent (instead of throwing)
so the update of paymentAttempts (the .update().set({...}) where
eq(paymentAttempts.id, nextAttempt.id)) proceeds when the order is in a
non-payable state; keep other exceptions propagated.
| export async function cacheWarehousesBySettlement(args: { | ||
| settlementRef: string; | ||
| runId: string; | ||
| }): Promise<{ upserted: number; deactivated: number }> { | ||
| const warehouses = await getWarehousesBySettlementRef(args.settlementRef); | ||
| await upsertWarehouses(warehouses, args.runId); | ||
| await deactivateMissingWarehouses({ | ||
| settlementRef: args.settlementRef, | ||
| keepRefs: warehouses.map(x => x.ref), | ||
| runId: args.runId, | ||
| }); | ||
| return { | ||
| upserted: warehouses.length, | ||
| deactivated: 0, | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find the file and examine the deactivateMissingWarehouses function
find . -name "nova-poshta-catalog.ts" -type fRepository: DevLoversTeam/devlovers.net
Length of output: 131
🏁 Script executed:
# Look at the file to see the full implementation
head -400 frontend/lib/services/shop/shipping/nova-poshta-catalog.ts | tail -150Repository: DevLoversTeam/devlovers.net
Length of output: 3912
🏁 Script executed:
# Search for the deactivateMissingWarehouses function definition
rg -A 30 "async function deactivateMissingWarehouses" frontend/lib/services/shop/shipping/nova-poshta-catalog.tsRepository: DevLoversTeam/devlovers.net
Length of output: 835
🏁 Script executed:
# Check db.execute usage patterns and imports
head -50 frontend/lib/services/shop/shipping/nova-poshta-catalog.tsRepository: DevLoversTeam/devlovers.net
Length of output: 1028
🏁 Script executed:
# Find the db module
find . -path "*/@/db*" -o -path "*/db.ts" -o -path "*/db.js" | grep -E "(db\.ts|db\.js|db/index)" | head -10Repository: DevLoversTeam/devlovers.net
Length of output: 53
🏁 Script executed:
# Search for db.execute usage patterns to see if anyone captures the result
rg "db\.execute" --type ts --type tsx -B 2 -A 2 | head -80Repository: DevLoversTeam/devlovers.net
Length of output: 98
🏁 Script executed:
# Check the db definition
find . -name "db.ts" -o -name "db.js" | xargs cat 2>/dev/null | head -100Repository: DevLoversTeam/devlovers.net
Length of output: 53
🏁 Script executed:
# Search for where db is exported from
rg "export.*db" --type ts -l | head -10Repository: DevLoversTeam/devlovers.net
Length of output: 277
🏁 Script executed:
# Try to find db-related files
fd "db" --type f | grep -E "\.(ts|js)$" | head -20Repository: DevLoversTeam/devlovers.net
Length of output: 110
🏁 Script executed:
# Search for db.execute more broadly to see result patterns
rg "db\.execute" -A 3 -B 1 | head -100Repository: DevLoversTeam/devlovers.net
Length of output: 7026
🏁 Script executed:
# Look for db module initialization/export
rg "from.*@/db" -A 2 -B 2 | head -50Repository: DevLoversTeam/devlovers.net
Length of output: 3216
🏁 Script executed:
# Check the db module to understand db.execute return type
cat frontend/db/index.tsRepository: DevLoversTeam/devlovers.net
Length of output: 1283
🏁 Script executed:
# Look at how db.execute results are checked in inventory.ts for more context
rg -B 5 -A 10 "unexpected db.execute result shape" frontend/lib/services/inventory.tsRepository: DevLoversTeam/devlovers.net
Length of output: 499
🏁 Script executed:
# Look at a concrete example of db.execute result usage with type checking
rg -B 3 -A 10 "const res = await db.execute" frontend/lib/services/inventory.ts | head -50Repository: DevLoversTeam/devlovers.net
Length of output: 862
🏁 Script executed:
# Check for rowCount usage in the codebase
rg "rowCount" --type ts | head -20Repository: DevLoversTeam/devlovers.net
Length of output: 53
🏁 Script executed:
# Look at drizzle-orm execute documentation or type definitions by checking imports
rg "drizzle-orm" frontend/db/index.ts -A 5Repository: DevLoversTeam/devlovers.net
Length of output: 355
🏁 Script executed:
# Search for how UPDATE queries handle result count elsewhere in the codebase
rg "UPDATE.*SET.*WHERE" -A 5 | grep -E "(rowCount|\.rows|rows\.length)" | head -20Repository: DevLoversTeam/devlovers.net
Length of output: 53
🏁 Script executed:
# Check the shipments-worker to see how db.execute results are used with type generics
rg -B 2 -A 15 "const res = await db.execute<.*UpdatedRow" frontend/lib/services/shop/shipping/shipments-worker.tsRepository: DevLoversTeam/devlovers.net
Length of output: 53
🏁 Script executed:
# Look at a complete example from retention.ts
rg -B 3 -A 20 "const res = await db.execute<UpdatedRow" frontend/lib/services/shop/shipping/retention.tsRepository: DevLoversTeam/devlovers.net
Length of output: 1079
🏁 Script executed:
# Check what properties the result object has by looking at complete usage
cat frontend/lib/services/shop/shipping/retention.ts | head -150Repository: DevLoversTeam/devlovers.net
Length of output: 2379
🏁 Script executed:
# Check if there's any rowCount usage pattern in drizzle with UPDATE operations
rg "rowCount|rows\.length|\.count" frontend/lib/services --type ts | grep -E "(UPDATE|execute)" | head -20Repository: DevLoversTeam/devlovers.net
Length of output: 53
🏁 Script executed:
# Look for RETURNING usage in UPDATE queries
rg "UPDATE.*RETURNING" frontend/lib/services --type ts -A 2 | head -40Repository: DevLoversTeam/devlovers.net
Length of output: 53
🏁 Script executed:
# Check the exact result type when executing UPDATEs without type parameter
rg "await db.execute\(sql" frontend/lib/services/shop/shipping/nova-poshta-catalog.ts -B 2 -A 8Repository: DevLoversTeam/devlovers.net
Length of output: 981
🏁 Script executed:
# Check the full shipments-worker to see complete patterns with UPDATE and RETURNING
cat frontend/lib/services/shop/shipping/shipments-worker.ts | head -200Repository: DevLoversTeam/devlovers.net
Length of output: 5321
🏁 Script executed:
# Search for UPDATE queries with RETURNING in the entire codebase
rg "UPDATE.*RETURNING" frontend --type ts -B 2 -A 5 | head -80Repository: DevLoversTeam/devlovers.net
Length of output: 53
🏁 Script executed:
# Check drizzle documentation or usage by looking at how UPDATE results are structured
# Try checking neon HTTP driver behavior with UPDATE
rg "UPDATE" frontend/lib/services/shop/shipping/nova-poshta-catalog.ts -B 1 -A 10 | grep -E "(UPDATE|const|return|rowCount|rows|\.rows)"Repository: DevLoversTeam/devlovers.net
Length of output: 247
🏁 Script executed:
# Search for any error handling or result inspection after db.execute
rg "await db.execute" frontend/lib/services -A 5 | grep -E "(rows|rowCount|length|status)" | head -30Repository: DevLoversTeam/devlovers.net
Length of output: 1710
🏁 Script executed:
# Check package.json to see drizzle-orm version
cat package.json | grep -A 5 -B 5 "drizzle-orm"Repository: DevLoversTeam/devlovers.net
Length of output: 115
🏁 Script executed:
# Find package.json in the repository
find . -name "package.json" -type f | head -5Repository: DevLoversTeam/devlovers.net
Length of output: 116
🏁 Script executed:
# Check one of those package.json files for drizzle-orm version
head -80 frontend/package.json | grep -A 2 -B 2 "drizzle"Repository: DevLoversTeam/devlovers.net
Length of output: 548
🏁 Script executed:
# Look at monobank-webhook.ts which seems to handle result.rows
rg -B 5 -A 10 "as unknown as.*rows" frontend/lib/services/orders/monobank-webhook.tsRepository: DevLoversTeam/devlovers.net
Length of output: 1878
deactivated is always reported as 0.
The core issue is valid—the return value is hardcoded instead of reflecting actual deactivation activity. However, the proposed fix using rowCount will not work with drizzle-orm; db.execute() returns { rows?: Array<T> }, not a result object with rowCount.
To fix this, add a RETURNING clause to both UPDATE queries in deactivateMissingWarehouses, capture the returned rows (which represent deactivated records), and return their count to the caller:
🛠️ Corrected fix
-async function deactivateMissingWarehouses(args: {
+async function deactivateMissingWarehouses(args: {
settlementRef: string;
keepRefs: string[];
runId: string;
-}) {
+}): Promise<number> {
const keepRefs = args.keepRefs.filter(x => x.trim().length > 0);
if (!keepRefs.length) {
- await db.execute(sql`
+ const res = await db.execute<{ ref: string }>(sql`
UPDATE np_warehouses
SET
is_active = false,
last_sync_run_id = ${args.runId}::uuid,
updated_at = now()
WHERE settlement_ref = ${args.settlementRef}
+ RETURNING ref
`);
- return;
+ return Array.isArray(res) ? res.length : res.rows?.length ?? 0;
}
const refs = keepRefs.map(ref => sql`${ref}`);
- await db.execute(sql`
+ const res = await db.execute<{ ref: string }>(sql`
UPDATE np_warehouses
SET
is_active = false,
last_sync_run_id = ${args.runId}::uuid,
updated_at = now()
WHERE settlement_ref = ${args.settlementRef}
AND ref NOT IN (${sql.join(refs, sql`, `)})
+ RETURNING ref
`);
+ return Array.isArray(res) ? res.length : res.rows?.length ?? 0;
}
export async function cacheWarehousesBySettlement(args: {
settlementRef: string;
runId: string;
}): Promise<{ upserted: number; deactivated: number }> {
const warehouses = await getWarehousesBySettlementRef(args.settlementRef);
await upsertWarehouses(warehouses, args.runId);
- await deactivateMissingWarehouses({
+ const deactivated = await deactivateMissingWarehouses({
settlementRef: args.settlementRef,
keepRefs: warehouses.map(x => x.ref),
runId: args.runId,
});
return {
upserted: warehouses.length,
- deactivated: 0,
+ deactivated,
};
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/lib/services/shop/shipping/nova-poshta-catalog.ts` around lines 307
- 321, cacheWarehousesBySettlement currently returns deactivated: 0 because
deactivateMissingWarehouses is not returning the actual count; update
deactivateMissingWarehouses so its UPDATE queries include a RETURNING clause,
capture the returned rows from db.execute (note drizzle-orm returns { rows?:
Array<T> }), count those returned rows as the number of deactivated records,
have deactivateMissingWarehouses return that count, and then change
cacheWarehousesBySettlement to return deactivated: <value returned from
deactivateMissingWarehouses> instead of the hardcoded 0; ensure upsertWarehouses
and getWarehousesBySettlementRef usages remain unchanged.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/lib/services/orders/monobank-janitor.ts (1)
831-864:⚠️ Potential issue | 🔴 CriticalJob3 can enter an infinite loop if event hashes change during reconstruction, not due to insert conflicts.
The analysis of the deadlock requires deeper examination than the conflict behavior alone.
When Job3 reads stored events with
applied_at IS NULL, it reconstructs therawBodyviaJSON.stringify(toPayloadRecord(eventRow.raw_payload)). If this reconstructed JSON differs in any way from the originally stored payload (e.g., key ordering, whitespace, null handling), the computed SHA256 will not match the storedeventRow.raw_sha256.Job3 then calls
applyMonoWebhookEventwitheventKey: rawSha256(the new computed hash). InsideinsertEvent, the conflict is resolved as follows:
- The insert fails on the
raw_sha256unique constraint (the new hash is unique, not a conflict here)- But when trying to match by the reconstructed
eventKey, the fallback querywhere event_key = newHash or raw_sha256 = newHashfinds nothing—the stored row has the original hashesinsertEventreturns{ eventId: null, deduped: true }applyMonoWebhookEventreturns{ appliedResult: 'deduped', eventId: null }without settingapplied_at- The event remains a Job3 candidate forever
Additionally, if
eventRow.raw_sha256is invalid or malformed (despite schema constraints), the fallback tosha256Utf8(rawBody)produces a new hash, and the same deadlock occurs.Recommendation: Either store and re-use the original
raw_sha256during apply without reconstruction, or ensureinsertEventperforms an upsert (returning the existing row's ID on conflict) so business logic can proceed on re-applied stored events.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/services/orders/monobank-janitor.ts` around lines 831 - 864, The code recomputes rawBody/rawSha256 from toPayloadRecord(eventRow.raw_payload) and passes eventKey: rawSha256 to applyMonoWebhookEvent, which can change the hash and cause Job3 rows to never get marked applied; fix by using the original stored hash when available (use eventRow.raw_sha256 if it matches the /^[0-9a-f]{64}$/i test) instead of always recomputing, or modify insertEvent (the conflict resolution path) to perform an upsert/RETURNING that returns the existing event id on conflict so applyMonoWebhookEvent can mark applied; adjust the call site in monobank-janitor (around readDryRunJob3Candidates, toPayloadRecord, sha256Utf8, applyMonoWebhookEvent) or update insertEvent’s conflict handling accordingly to ensure existing rows are identified and applied_at gets set.
♻️ Duplicate comments (1)
frontend/lib/services/orders/monobank-janitor.ts (1)
331-340:clearAppliedEventClaimis now defined — past concern resolved.The implementation is correct: the
AND applied_at IS NOT NULLguard ensures claims are only cleared after the event has been applied, making spurious no-op calls (e.g.,applied_nooppath) safe. Omitting arunIdfilter is appropriate here since you want any applied-event claim cleared regardless of which instance set it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/services/orders/monobank-janitor.ts` around lines 331 - 340, clearAppliedEventClaim is implemented correctly: it should leave the applied-at guard and no runId filter so any instance's claim for an already-applied event is cleared; no code changes required to the clearAppliedEventClaim function (the update in clearAppliedEventClaim that sets claimed_at, claim_expires_at, claimed_by to null where id = ${args.eventId}::uuid and applied_at is not null is the intended behavior).
🧹 Nitpick comments (3)
frontend/app/[locale]/admin/shop/orders/[id]/page.tsx (2)
29-29:noStore()is redundant alongsidedynamic = 'force-dynamic'.
export const dynamic = 'force-dynamic'already forces dynamic rendering and disables the full-route cache on every request; callingnoStore()inside the component body has no additional effect.♻️ Proposed fix
- import { unstable_noStore as noStore } from 'next/cache'; ... export default async function OrderDetailPage(...) { - noStore();Also applies to: 120-120
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/`[locale]/admin/shop/orders/[id]/page.tsx at line 29, The file redundantly calls noStore() inside the component while also exporting export const dynamic = 'force-dynamic'; remove the noStore() calls (and any unused noStore import) and rely on export const dynamic = 'force-dynamic' to enforce dynamic rendering—search for noStore() usages in this module (e.g., the component body near the top and the repeated occurrence later) and delete them so only the dynamic export remains.
200-200: Currency fallback silently maps any non-UAH value to'USD'.If the schema ever gains a third currency (e.g.
EUR), this ternary will silently format it as USD, producing wrong amounts with no runtime warning.♻️ Proposed fix — narrow via the inferred schema union
- const currency: CurrencyCode = order.currency === 'UAH' ? 'UAH' : 'USD'; + const SUPPORTED_CURRENCIES = new Set<CurrencyCode>(['UAH', 'USD']); + const currency: CurrencyCode = SUPPORTED_CURRENCIES.has(order.currency as CurrencyCode) + ? (order.currency as CurrencyCode) + : 'USD';Better still, if
OrderCurrencyandCurrencyCodeshare the same union, cast directly and let TypeScript enforce exhaustiveness.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/`[locale]/admin/shop/orders/[id]/page.tsx at line 200, The current ternary mapping for currency (`const currency: CurrencyCode = order.currency === 'UAH' ? 'UAH' : 'USD';`) silently maps any unknown `order.currency` to 'USD'; instead, use the schema's union types (e.g. `OrderCurrency` and `CurrencyCode`) and perform a direct typed assignment or an exhaustive switch so TypeScript enforces validity: cast `order.currency` to `CurrencyCode` (or validate via a switch over possible `OrderCurrency` values and throw for unexpected cases) so new currencies like 'EUR' fail at compile-time or surface a runtime error rather than being mis-mapped.frontend/lib/services/orders/monobank-janitor.ts (1)
831-834:readDryRunJob3Candidatesused in the live processing path — misleading name.Unlike Job1/Job2, which use distinct claim-based functions for their live paths, Job3's live path reuses the dry-run read function. The name
readDryRunJob3Candidatesimplies it is only safe/intended for dry-run inspection, which will confuse future readers or refactorers.Consider renaming to something like
readJob3Candidates(used by both paths) orreadUnclaimedJob3Candidatesto accurately reflect that it reads unclaimed events without acquiring a lease.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/services/orders/monobank-janitor.ts` around lines 831 - 834, The helper name readDryRunJob3Candidates is misleading because it's used in the live processing path; rename the function to a neutral name (e.g. readJob3Candidates or readUnclaimedJob3Candidates) and update all call sites (including the live processing path in monobank-janitor.ts and any tests/imports) so the behavior (reading unclaimed Job3 events without acquiring a lease) remains unchanged; update the exported identifier, any JSDoc/comments referencing "dry run", and imports/exports so compilation and runtime linkage are preserved.
🤖 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/`[locale]/admin/shop/orders/[id]/page.tsx:
- Around line 127-132: The redirect currently builds returnTo as
`/${locale}/shop/orders/${id}` which points to the non-admin route; update the
redirect call so the encoded returnTo includes the `/admin` segment (i.e.,
`/${locale}/admin/shop/orders/${id}`) using the existing variables `locale` and
`id` in the same `redirect(...)` invocation so after login the user returns to
the admin order page; adjust the encodeURIComponent argument only and keep the
surrounding redirect logic unchanged.
- Around line 276-288: The two hardcoded labels around order.shippingStatus and
order.trackingNumber must be replaced with localized strings (e.g.
t('shippingStatus') and t('trackingNumber')) and the visible item labels "Qty:",
"Unit:", and "Line:" in the items list should use the same translation keys as
their sr-only counterparts (t('quantity'), t('unitPrice'), t('lineTotal')) so
the page is fully localized; update the JSX where order.shippingStatus and
order.trackingNumber are rendered and the item list label nodes to call the
existing t(...) translator used elsewhere in this component.
- Around line 141-198: The 404 signal from notFound() is being swallowed by the
surrounding try/catch; split the logic so DB I/O is guarded but the empty-rows
check lives outside the DB catch. Specifically, wrap only the db.select(...)
call (the code that assigns rows from db.select in the try) in a try/catch that
uses logError and throws ORDER_DETAIL_LOAD_FAILED on DB errors, then immediately
after that try do if (rows.length === 0) notFound(); keep the mapping/assembly
that uses toOrderItem inside its own try (or the original try if you prefer) so
genuine toOrderItem failures remain caught by logError; reference these symbols:
db.select, rows, notFound(), toOrderItem, and logError.
In `@frontend/lib/services/orders/monobank-janitor.ts`:
- Around line 866-874: The code increments applied/noop then calls
clearAppliedEventClaim inside the main try, so if clearAppliedEventClaim throws
the outer catch will also increment failed and double-count the event; fix by
isolating clearAppliedEventClaim in its own try/catch after you update
applied/noop: after the existing if (isAppliedResult(result.appliedResult)) {
applied += 1 } else { noop += 1 } block, wrap the clearAppliedEventClaim({
eventId: result.eventId }) call in a separate try { await
clearAppliedEventClaim(...) } catch (err) { /* log or warn about claim release
failure for result.eventId but do NOT increment failed or rethrow */ } so
failures to clear the claim don't mark the event as failed in the main counters
(reference: isAppliedResult, applied, noop, failed, result.eventId,
clearAppliedEventClaim).
---
Outside diff comments:
In `@frontend/lib/services/orders/monobank-janitor.ts`:
- Around line 831-864: The code recomputes rawBody/rawSha256 from
toPayloadRecord(eventRow.raw_payload) and passes eventKey: rawSha256 to
applyMonoWebhookEvent, which can change the hash and cause Job3 rows to never
get marked applied; fix by using the original stored hash when available (use
eventRow.raw_sha256 if it matches the /^[0-9a-f]{64}$/i test) instead of always
recomputing, or modify insertEvent (the conflict resolution path) to perform an
upsert/RETURNING that returns the existing event id on conflict so
applyMonoWebhookEvent can mark applied; adjust the call site in monobank-janitor
(around readDryRunJob3Candidates, toPayloadRecord, sha256Utf8,
applyMonoWebhookEvent) or update insertEvent’s conflict handling accordingly to
ensure existing rows are identified and applied_at gets set.
---
Duplicate comments:
In `@frontend/lib/services/orders/monobank-janitor.ts`:
- Around line 331-340: clearAppliedEventClaim is implemented correctly: it
should leave the applied-at guard and no runId filter so any instance's claim
for an already-applied event is cleared; no code changes required to the
clearAppliedEventClaim function (the update in clearAppliedEventClaim that sets
claimed_at, claim_expires_at, claimed_by to null where id =
${args.eventId}::uuid and applied_at is not null is the intended behavior).
---
Nitpick comments:
In `@frontend/app/`[locale]/admin/shop/orders/[id]/page.tsx:
- Line 29: The file redundantly calls noStore() inside the component while also
exporting export const dynamic = 'force-dynamic'; remove the noStore() calls
(and any unused noStore import) and rely on export const dynamic =
'force-dynamic' to enforce dynamic rendering—search for noStore() usages in this
module (e.g., the component body near the top and the repeated occurrence later)
and delete them so only the dynamic export remains.
- Line 200: The current ternary mapping for currency (`const currency:
CurrencyCode = order.currency === 'UAH' ? 'UAH' : 'USD';`) silently maps any
unknown `order.currency` to 'USD'; instead, use the schema's union types (e.g.
`OrderCurrency` and `CurrencyCode`) and perform a direct typed assignment or an
exhaustive switch so TypeScript enforces validity: cast `order.currency` to
`CurrencyCode` (or validate via a switch over possible `OrderCurrency` values
and throw for unexpected cases) so new currencies like 'EUR' fail at
compile-time or surface a runtime error rather than being mis-mapped.
In `@frontend/lib/services/orders/monobank-janitor.ts`:
- Around line 831-834: The helper name readDryRunJob3Candidates is misleading
because it's used in the live processing path; rename the function to a neutral
name (e.g. readJob3Candidates or readUnclaimedJob3Candidates) and update all
call sites (including the live processing path in monobank-janitor.ts and any
tests/imports) so the behavior (reading unclaimed Job3 events without acquiring
a lease) remains unchanged; update the exported identifier, any JSDoc/comments
referencing "dry run", and imports/exports so compilation and runtime linkage
are preserved.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/app/[locale]/admin/shop/orders/[id]/page.tsxfrontend/lib/services/orders/monobank-janitor.ts
| <div> | ||
| <dt className="text-muted-foreground text-xs">Shipping status</dt> | ||
| <dd className="text-sm font-medium"> | ||
| {order.shippingStatus ?? '-'} | ||
| </dd> | ||
| </div> | ||
|
|
||
| <div> | ||
| <dt className="text-muted-foreground text-xs">Tracking number</dt> | ||
| <dd className="text-sm font-medium break-all"> | ||
| {order.trackingNumber ?? '-'} | ||
| </dd> | ||
| </div> |
There was a problem hiding this comment.
"Shipping status" and "Tracking number" are hardcoded English — missing translation keys.
Every other label in the summary section uses t(...), but these two are bare string literals. Given that this page serves a localized audience via next-intl, they need translation keys too.
♻️ Proposed fix
- <dt className="text-muted-foreground text-xs">Shipping status</dt>
+ <dt className="text-muted-foreground text-xs">{t('shippingStatus')}</dt>
...
- <dt className="text-muted-foreground text-xs">Tracking number</dt>
+ <dt className="text-muted-foreground text-xs">{t('trackingNumber')}</dt>The same applies to the inline "Qty:", "Unit:", and "Line:" visible labels in the items list (lines 383, 388, 395); their sr-only counterparts already use t('quantity'), t('unitPrice'), and t('lineTotal').
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <div> | |
| <dt className="text-muted-foreground text-xs">Shipping status</dt> | |
| <dd className="text-sm font-medium"> | |
| {order.shippingStatus ?? '-'} | |
| </dd> | |
| </div> | |
| <div> | |
| <dt className="text-muted-foreground text-xs">Tracking number</dt> | |
| <dd className="text-sm font-medium break-all"> | |
| {order.trackingNumber ?? '-'} | |
| </dd> | |
| </div> | |
| <div> | |
| <dt className="text-muted-foreground text-xs">{t('shippingStatus')}</dt> | |
| <dd className="text-sm font-medium"> | |
| {order.shippingStatus ?? '-'} | |
| </dd> | |
| </div> | |
| <div> | |
| <dt className="text-muted-foreground text-xs">{t('trackingNumber')}</dt> | |
| <dd className="text-sm font-medium break-all"> | |
| {order.trackingNumber ?? '-'} | |
| </dd> | |
| </div> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/app/`[locale]/admin/shop/orders/[id]/page.tsx around lines 276 -
288, The two hardcoded labels around order.shippingStatus and
order.trackingNumber must be replaced with localized strings (e.g.
t('shippingStatus') and t('trackingNumber')) and the visible item labels "Qty:",
"Unit:", and "Line:" in the items list should use the same translation keys as
their sr-only counterparts (t('quantity'), t('unitPrice'), t('lineTotal')) so
the page is fully localized; update the JSX where order.shippingStatus and
order.trackingNumber are rendered and the item list label nodes to call the
existing t(...) translator used elsewhere in this component.
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/app/[locale]/shop/orders/[id]/page.tsx (1)
141-198:⚠️ Potential issue | 🔴 Critical
notFound()insidetryis swallowed — users will see a 500 instead of 404 for missing orders.
notFound()on line 180 throws a Next.js sentinel error, which gets caught by thecatchon line 195 and re-thrown asError('ORDER_DETAIL_LOAD_FAILED'). This produces a 500 error page instead of a proper 404. The same bug was identified and fixed in the admin page (admin/shop/orders/[id]/page.tsx) using an IIFE pattern, but this user-facing page still has it.🐛 Proposed fix — hoist notFound() out of try/catch
+ let rows: Awaited<ReturnType<typeof db.select>>; // adjust type as needed try { const whereClause = isAdmin ? eq(orders.id, parsed.data.id) : and(eq(orders.id, parsed.data.id), eq(orders.userId, user.id)); - const rows = await db + rows = await db .select({ ... }) .from(orders) .leftJoin(orderItems, eq(orderItems.orderId, orders.id)) .where(whereClause) .orderBy(orderItems.id); - - if (rows.length === 0) notFound(); - - const base = rows[0]!.order; - ... - order = { ... }; } catch (error) { logError('User order detail page failed', error); throw new Error('ORDER_DETAIL_LOAD_FAILED'); } + if (rows.length === 0) notFound(); + + try { + const base = rows[0]!.order; + const items = rows.map(r => toOrderItem(r.item)).filter(...); + order = { ...base, ... items }; + } catch (error) { + logError('User order detail page failed', error); + throw new Error('ORDER_DETAIL_LOAD_FAILED'); + }Or replicate the IIFE pattern already used in the admin page.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/`[locale]/shop/orders/[id]/page.tsx around lines 141 - 198, The try/catch is currently catching Next.js's notFound() sentinel and converting it into a 500; move the notFound() call outside the try block (or wrap the DB logic in an IIFE like the admin page) so the sentinel can propagate. Concretely, inside the code that queries rows (the block using db.select(...).from(orders)... and the variable rows), ensure you perform the rows.length === 0 check and call notFound() before the outer catch that logs via logError('User order detail page failed', error) and throws Error('ORDER_DETAIL_LOAD_FAILED'); keep the rest of order assembly (base, items, toOrderItem, createdAt/updatedAt/restockedAt conversion) inside the try so only real errors are caught.
♻️ Duplicate comments (6)
frontend/lib/services/shop/shipping/nova-poshta-catalog.ts (1)
310-325:deactivatedis still hardcoded to0.This was flagged in a previous review.
deactivateMissingWarehousesdoesn't return the count of rows it deactivated, socacheWarehousesBySettlementalways reportsdeactivated: 0regardless of how many warehouses were actually marked inactive.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/services/shop/shipping/nova-poshta-catalog.ts` around lines 310 - 325, The function cacheWarehousesBySettlement currently hardcodes deactivated: 0; change it to return the actual number of deactivated rows by having deactivateMissingWarehouses return its deactivated count (e.g., Promise<number> or {deactivated:number}) and await that result here; update cacheWarehousesBySettlement to call getWarehousesBySettlementRef, upsertWarehouses, then const deactivated = await deactivateMissingWarehouses({...}) and return { upserted: warehouses.length, deactivated }; modify the deactivateMissingWarehouses implementation/signature to compute and return the count of rows it marks inactive so the value is accurate.frontend/lib/services/shop/shipping/nova-poshta-client.ts (2)
439-442:⚠️ Potential issue | 🟠 Major
addressLine2is still silently dropped whenaddressLine1is present.The
||chain meansRecipientAddressNameonly getsaddressLine1when truthy;addressLine2(apartment, building, etc.) is never concatenated. For courier delivery, the full address typically requires both lines.🐛 Proposed fix
- RecipientAddressName: - input.recipient.addressLine1 || - input.recipient.addressLine2 || - undefined, + RecipientAddressName: + [input.recipient.addressLine1, input.recipient.addressLine2] + .filter(Boolean) + .join(', ') || undefined,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/services/shop/shipping/nova-poshta-client.ts` around lines 439 - 442, RecipientAddressName currently uses an || chain that drops addressLine2 when addressLine1 is present; update the assignment for RecipientAddressName (the object property where input.recipient is used) to join input.recipient.addressLine1 and input.recipient.addressLine2 (e.g., concatenate non-empty parts with a space or comma, trim final string) and fall back to undefined if both are empty so the courier receives the full combined address line.
237-267:⚠️ Potential issue | 🟠 Major
callNpstill lacks a fetch timeout.If the NP API hangs, this
fetchcall will block indefinitely (or until the runtime's default timeout). This was flagged in a previous review and remains unaddressed.Add an
AbortControllerwith a reasonable timeout:⏱️ Proposed fix
+ const controller = new AbortController(); + const timeout = setTimeout(() => controller.abort(), 15_000); + let response: Response; try { response = await fetch(config.apiBaseUrl, { method: 'POST', headers: { ... }, body: JSON.stringify({ ... }), cache: 'no-store', + signal: controller.signal, }); } catch (error) { + clearTimeout(timeout); + if (error instanceof DOMException && error.name === 'AbortError') { + throw new NovaPoshtaApiError('NP_TIMEOUT', 'NP request timed out', 504); + } throw new NovaPoshtaApiError('NP_FETCH_FAILED', 'fetch failed', 0, { cause: error, }); + } finally { + clearTimeout(timeout); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/services/shop/shipping/nova-poshta-client.ts` around lines 237 - 267, callNp lacks a fetch timeout and can hang; wrap the fetch in an AbortController with a reasonable timeout (e.g., 5–15s): create an AbortController before calling fetch, pass controller.signal to fetch, set a timer (via setTimeout) that calls controller.abort() after the timeout, and clear the timer after a successful response; update the catch branch to treat AbortError distinctly when throwing NovaPoshtaApiError. Ensure changes are made inside the callNp function around the fetch call and reference the existing NovaPoshtaApiError and response handling logic.frontend/lib/services/orders/monobank-janitor.ts (1)
361-370: Past review concerns addressed:clearAppliedEventClaimis now defined and isolated.The function exists (lines 361–370) and its call site (lines 912–930) is wrapped in a dedicated
try/catchinside the main processing block, so a failure here cannot corrupt theapplied/failedcounters.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/services/orders/monobank-janitor.ts` around lines 361 - 370, clearAppliedEventClaim is correctly defined and isolated; no code change required—just verify the call site in the main processing block that invokes clearAppliedEventClaim is wrapped in its dedicated try/catch so failures cannot affect the applied/failed counters (the function name clearAppliedEventClaim and its invocation in the main processing loop should be used to locate both definitions).frontend/app/[locale]/admin/shop/orders/[id]/page.tsx (1)
282-294:⚠️ Potential issue | 🟡 Minor"Shipping status" and "Tracking number" are still hardcoded English — translation keys exist but aren't used.
The
pl.json(and presumablyen.json/uk.json) already defineshippingStatusandtrackingNumberundershop.orders.detail, and this page loads translations from that namespace (getTranslations('shop.orders.detail')). Replace the bare strings:- <dt className="text-muted-foreground text-xs">Shipping status</dt> + <dt className="text-muted-foreground text-xs">{t('shippingStatus')}</dt> ... - <dt className="text-muted-foreground text-xs">Tracking number</dt> + <dt className="text-muted-foreground text-xs">{t('trackingNumber')}</dt>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/`[locale]/admin/shop/orders/[id]/page.tsx around lines 282 - 294, The dt labels "Shipping status" and "Tracking number" are hardcoded; use the loaded translations from getTranslations('shop.orders.detail') instead. Replace the literal strings in the two <dt> elements with the translation keys t('shippingStatus') and t('trackingNumber') (keeping the surrounding markup and classes intact) so the page uses the locale-specific strings from the shop.orders.detail namespace; ensure t is the translation function returned where getTranslations was called and used in this component.frontend/lib/services/shop/shipping/shipments-worker.ts (1)
256-265:⚠️ Potential issue | 🟠 MajorEnforce Nova Poshta minimum declared cost.
declaredCostUahFromMinorcan return values below Nova Poshta’s minimum declared cost, which will cause TTN creation to fail. Clamp to the documented minimum after rounding.🛠️ Suggested fix
- return Math.floor((Math.trunc(totalMinor) + 50) / 100); + const rounded = Math.floor((Math.trunc(totalMinor) + 50) / 100); + return Math.max(300, rounded);Nova Poshta InternetDocument API minimum declared cost (Cost) value🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/services/shop/shipping/shipments-worker.ts` around lines 256 - 265, The function declaredCostUahFromMinor can produce values below Nova Poshta’s minimum declared cost; update it to clamp the final rounded result to a documented minimum (e.g., introduce a constant like MIN_NOVA_POSHTA_DECLARED_COST_UAH) after the existing rounding logic and before returning, so return Math.max(roundedValue, MIN_NOVA_POSHTA_DECLARED_COST_UAH); keep the existing input validation and error (buildFailure) unchanged and ensure the new constant is clearly named and documented for future reference.
🧹 Nitpick comments (13)
frontend/.env.example (4)
105-107: Swap webhook instance ID order to satisfy dotenv-linter.Place
STRIPE_WEBHOOK_INSTANCE_IDbeforeWEBHOOK_INSTANCE_ID.♻️ Proposed reordering
-WEBHOOK_INSTANCE_ID= -STRIPE_WEBHOOK_INSTANCE_ID= +STRIPE_WEBHOOK_INSTANCE_ID= +WEBHOOK_INSTANCE_ID=🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/.env.example` around lines 105 - 107, Reorder the two environment variable declarations so STRIPE_WEBHOOK_INSTANCE_ID appears before WEBHOOK_INSTANCE_ID in frontend/.env.example to satisfy dotenv-linter; locate the block containing WEBHOOK_INSTANCE_ID and STRIPE_WEBHOOK_INSTANCE_ID and swap their lines so STRIPE_WEBHOOK_INSTANCE_ID is listed first.
66-69: Reorder Monobank webhook keys to match dotenv-linter rules.Place
MONO_REFUND_ENABLEDfirst, thenMONO_WEBHOOK_CLAIM_TTL_MS, thenMONO_WEBHOOK_MODE.♻️ Proposed reordering
-# Optional webhook/runtime tuning (defaults in code if omitted) -MONO_WEBHOOK_MODE= -MONO_WEBHOOK_CLAIM_TTL_MS= -MONO_REFUND_ENABLED=0 +# Optional webhook/runtime tuning (defaults in code if omitted) +MONO_REFUND_ENABLED=0 +MONO_WEBHOOK_CLAIM_TTL_MS= +MONO_WEBHOOK_MODE=🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/.env.example` around lines 66 - 69, Reorder the Monobank env keys in .env.example to satisfy dotenv-linter: place MONO_REFUND_ENABLED first, then MONO_WEBHOOK_CLAIM_TTL_MS, and finally MONO_WEBHOOK_MODE; update the block containing MONO_WEBHOOK_MODE, MONO_WEBHOOK_CLAIM_TTL_MS, and MONO_REFUND_ENABLED so the variables appear in the new order and preserve their values/comments.
48-55: Fix env key order in Stripe block to satisfy dotenv-linter.Reorder to place
STRIPE_MODEbeforeSTRIPE_PAYMENTS_ENABLEDper lint rule.♻️ Proposed reordering
NEXT_PUBLIC_STRIPE_PUBLISHABLE_KEY= -STRIPE_PAYMENTS_ENABLED= # Options: test, live (defaults to test in development, live in production) STRIPE_MODE= +STRIPE_PAYMENTS_ENABLED= STRIPE_SECRET_KEY= STRIPE_WEBHOOK_SECRET=🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/.env.example` around lines 48 - 55, The Stripe env block violates dotenv-linter ordering; swap the lines so STRIPE_MODE appears before STRIPE_PAYMENTS_ENABLED. Edit the frontend/.env.example block containing NEXT_PUBLIC_STRIPE_PUBLISHABLE_KEY, STRIPE_PAYMENTS_ENABLED, STRIPE_MODE, STRIPE_SECRET_KEY, STRIPE_WEBHOOK_SECRET and move the STRIPE_MODE entry to precede STRIPE_PAYMENTS_ENABLED so the keys read NEXT_PUBLIC_STRIPE_PUBLISHABLE_KEY, STRIPE_MODE, STRIPE_PAYMENTS_ENABLED, STRIPE_SECRET_KEY, STRIPE_WEBHOOK_SECRET to satisfy the linter.
81-93: Reorder NP_ keys to satisfy dotenv-linter ordering rules.*Several NP keys are out of order per lint rules; reorder as below.
♻️ Proposed reordering
-NP_API_KEY= -# Optional if code has a default; set explicitly in production for clarity -NP_API_BASE= -NP_SENDER_REF= -NP_SENDER_CONTACT_REF= -NP_SENDER_ADDRESS_REF= -NP_SENDER_CITY_REF= -NP_SENDER_PHONE= +# Optional if code has a default; set explicitly in production for clarity +NP_API_BASE= +NP_API_KEY= +NP_SENDER_ADDRESS_REF= +NP_SENDER_CITY_REF= +NP_SENDER_CONTACT_REF= +NP_SENDER_PHONE= +NP_SENDER_REF= @@ -# Optional tuning (override only if needed; otherwise code defaults apply) -NP_TIMEOUT_MS= -NP_MAX_RETRIES= -NP_RETRY_DELAY_MS= +# Optional tuning (override only if needed; otherwise code defaults apply) +NP_MAX_RETRIES= +NP_RETRY_DELAY_MS= +NP_TIMEOUT_MS=🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/.env.example` around lines 81 - 93, The NP_ environment variable block in .env.example is not in the dotenv-linter expected order; reorder the NP_* keys into alphabetical order so the linter passes. Specifically locate the NP_* entries (NP_API_KEY, NP_API_BASE, NP_SENDER_REF, NP_SENDER_CONTACT_REF, NP_SENDER_ADDRESS_REF, NP_SENDER_CITY_REF, NP_SENDER_PHONE, NP_TIMEOUT_MS, NP_MAX_RETRIES, NP_RETRY_DELAY_MS) and sort them alphabetically (e.g., NP_API_BASE before NP_API_KEY, NP_MAX_RETRIES before NP_RETRY_DELAY_MS, etc.), keeping the same comments and blank lines semantics. Ensure no duplicates or spacing changes beyond reordering.frontend/lib/services/orders/monobank-webhook.ts (1)
476-569: Keepshipping_statusupdates aligned with DB eligibility to avoid drift.
Becauseshipping_statusis set from a precomputed flag, it can diverge from actual eligibility if order fields change between the read and the atomic update. Consider gating the status update on the same DB-side predicates used for enqueueing.♻️ Suggested adjustment to keep status in sync
update orders set status = 'PAID', - shipping_status = case - when ${args.enqueueShipment} then 'queued'::shipping_status - else shipping_status - end, + shipping_status = case + when ${args.enqueueShipment} + and shipping_required = true + and shipping_provider = 'nova_poshta' + and shipping_method_code is not null + and ${inventoryCommittedForShippingSql(sql`orders.inventory_status`)} + then 'queued'::shipping_status + else shipping_status + end,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/services/orders/monobank-webhook.ts` around lines 476 - 569, The update currently sets shipping_status based on the precomputed boolean args.enqueueShipment which can drift from DB-side eligibility; inside atomicMarkPaidOrderAndSucceedAttempt update the orders.shipping_status using the same database predicates as the eligible_for_enqueue CTE (i.e. payment_status = 'paid', shipping_required = true, shipping_provider = 'nova_poshta', shipping_method_code IS NOT NULL, and the inventoryCommittedForShippingSql(...) predicate against updated_order.inventory_status) so the CASE in the orders UPDATE (in updated_order) mirrors the eligible_for_enqueue logic and keeps shipping_status consistent with inserted_shipment/eligible_for_enqueue and the inserted_shipment CTE.frontend/lib/services/shop/shipping/nova-poshta-catalog.ts (1)
163-206: Bulk upsert has no batch-size guard.
upsertCitiesbuilds a singleINSERT ... VALUESstatement for all rows. While NP API responses are typically small (~20-50 settlements), if this is ever called with a larger dataset, the generated SQL could exceed driver/connection limits.Consider chunking if the input size might grow, or add a comment noting the expected upper bound.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/services/shop/shipping/nova-poshta-catalog.ts` around lines 163 - 206, upsertCities currently builds one giant INSERT VALUES block (values, sql.join, db.execute) which can exceed SQL/driver limits for large inputs; modify upsertCities to process rows in bounded batches (e.g., chunk size 200–500) and perform the same INSERT ... ON CONFLICT call per chunk inside a loop, reusing the same VALUES/sql templates and runId, or if you intend to rely on small responses, add a clear comment near upsertCities explaining the expected upper bound and why batching is unnecessary.frontend/lib/services/orders/_shared.ts (1)
148-170: Minor: double normalization per item inmergeCheckoutItems.Each item is normalized via
normalizeCheckoutItemon line 154, thencheckoutItemMergeKey(line 155) normalizes the same item again internally. Consider passing the already-normalized item to a key-builder that skips re-normalization.Also, line 162 uses
item.quantity(the original) instead ofit.quantity(the normalized copy). They're equivalent since normalization preserves quantity, but usingit.quantitywould be more consistent with the rest of the loop body.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/services/orders/_shared.ts` around lines 148 - 170, mergeCheckoutItems currently normalizes each item with normalizeCheckoutItem then calls checkoutItemMergeKey which re-normalizes the same item; to fix, call checkoutItemMergeKey with the already-normalized variable (it) so the key builder doesn't re-run normalization, and when computing mergedQty use it.quantity (the normalized copy) instead of item.quantity; keep the rest of the logic (map, MAX_QUANTITY_PER_LINE check, InvalidPayloadError throw) unchanged.frontend/lib/services/orders/monobank-janitor.ts (1)
860-942: Job3 processes unclaimed candidates without per-event leasing — relies on idempotent apply.
readUnclaimedJob3Candidatesfilters onapplied_at is nulland expired/nullclaim_expires_at, but there is no atomic claim step beforeapplyMonoWebhookEvent. Concurrent janitor runs could select and attempt to apply the same event. This is safe only ifapplyMonoWebhookEventis fully idempotent (which appears to be the case from themode: 'apply'path).Worth noting: under high concurrency, this can lead to duplicate work and redundant
clearAppliedEventClaimcalls. If this becomes a concern, addingFOR UPDATE SKIP LOCKEDsemantics to the read query would eliminate the overlap.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/services/orders/monobank-janitor.ts` around lines 860 - 942, The loop reads candidates via readUnclaimedJob3Candidates then applies each with applyMonoWebhookEvent without an atomic per-event claim, allowing concurrent janitor runs to process the same event; fix by making the candidate selection atomic—either change readUnclaimedJob3Candidates to SELECT ... FOR UPDATE SKIP LOCKED (or use a transaction that updates claim_expires_at/claimer and returns the rows) so rows are locked/marked exclusively before calling applyMonoWebhookEvent, or add an immediate atomic update-by-id claim step before applyMonoWebhookEvent that returns whether the claim succeeded and skip processing if not; ensure clearAppliedEventClaim logic still runs only for successfully claimed/applied events (use eventId + claim ownership) to avoid redundant clear attempts.frontend/lib/services/shop/shipping/nova-poshta-client.ts (1)
360-413: Implement pagination for warehouse fetching or document the 500-warehouse limit.The function uses a hardcoded
Limit: 500andPage: 1, fetching only the first page of results. If a settlement (especially large cities like Kyiv or Kharkiv) has more than 500 warehouses, they will be silently omitted. Consider either looping through all pages until results are exhausted (similar to howsearchSettlementshandles pagination) or add a comment explaining why 500 is sufficient for all Ukrainian settlements.frontend/db/schema/shop.ts (3)
594-625: Shipment worker table is well-designed for lease-based processing.The
(status, nextAttemptAt)index andleaseExpiresAtindex align well with the claim/retry pattern described in the PR. The unique constraint onorderIdcleanly prevents duplicate shipment rows.Minor: consider adding a
CHECK(attempt_count >= 0)for consistency with other non-negative guards in this schema (e.g., stock, quantity, amounts).Optional defensive constraint
table => [ uniqueIndex('shipping_shipments_order_id_uq').on(table.orderId), index('shipping_shipments_queue_idx').on(table.status, table.nextAttemptAt), index('shipping_shipments_lease_idx').on(table.leaseExpiresAt), index('shipping_shipments_provider_ref_idx').on(table.providerRef), + check('shipping_shipments_attempt_count_non_negative', sql`${table.attemptCount} >= 0`), ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/db/schema/shop.ts` around lines 594 - 625, Add a non-negative check constraint for attemptCount on the shippingShipments table: ensure the schema definition for shippingShipments includes a CHECK(attempt_count >= 0) (either on the attemptCount column or as a table-level constraint) so attemptCount (field name attemptCount / DB column attempt_count) cannot be negative; update the table definition where shippingShipments is declared to include this constraint alongside the existing indexes and unique index.
240-261: Shipping invariant constraints look solid, butshippingPayeris not guarded.The bidirectional CHECK pair correctly enforces that
shippingProvider,shippingMethodCode, andshippingStatusmust all be present (or all absent) based onshippingRequired. However,shippingPayeris omitted from both checks — if shipping is required, it can still beNULL, which may lead to ambiguity about who pays for shipping.Is this intentional (e.g., payer is resolved later in the flow)? If it should always be set when shipping is required, consider adding it to the
orders_shipping_present_when_required_chkconstraint.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/db/schema/shop.ts` around lines 240 - 261, The CHECK pair for shipping fields enforces presence/absence of shippingProvider, shippingMethodCode, and shippingStatus based on shippingRequired but omits shippingPayer, allowing NULL payer when shippingRequired is true; update the 'orders_shipping_present_when_required_chk' (and symmetrically 'orders_shipping_null_when_not_required_chk' if you want strict parity) to include ${table.shippingPayer} in the AND conditions so that when shippingRequired is true shippingPayer IS NOT NULL (and when shippingRequired is TRUE in the null-check ensure shippingPayer IS NULL) — modify the check() calls around 'orders_shipping_present_when_required_chk' and 'orders_shipping_null_when_not_required_chk' to add ${table.shippingPayer} to the grouped predicates.
627-650: NP cities catalog table looks good.The schema captures the essential NP settlement data and the
lastSyncRunIdpattern enables efficient staleness detection during sync runs.One consideration: if city autocomplete will use
LIKE 'prefix%'queries, the default B-tree operator class onnameUawon't accelerate prefix searches unless the database collation isCorPOSIX. You could addtext_pattern_opsto the index, but given the catalog size (~30k Ukrainian settlements), this is unlikely to be a bottleneck.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/db/schema/shop.ts` around lines 627 - 650, The npCities table's index np_cities_active_name_idx currently uses the default B-tree operator class which won't help prefix LIKE searches on nameUa in non-C collations; update the index definition on npCities (np_cities_active_name_idx) so the nameUa/name_ua column uses the text_pattern_ops operator class (or create a separate index with text_pattern_ops on nameUa) to accelerate prefix queries like 'prefix%'. Ensure the change targets the index construction in the pgTable call that references table.isActive and table.nameUa.frontend/app/api/shop/webhooks/stripe/route.ts (1)
1007-1028: No log/warning whenapplyResult.appliedisfalseIf the atomic CTE's WHERE conditions reject the row (e.g., order is already fully paid, or stock has been restored),
appliedisfalseand the handler silently continues tomarkStripeAttemptFinalandlogWebhookEvent. Adding a structured warn log here makes idempotent replays and unexpected duplicate deliveries visible in the observability pipeline.💡 Suggested addition
+ if (!applyResult.applied) { + logWarn('stripe_webhook_paid_apply_noop', { + ...eventMeta(), + code: 'PAID_APPLY_NOOP', + orderId: order.id, + paymentStatus: order.paymentStatus, + }); + } if (applyResult.shipmentQueued) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/api/shop/webhooks/stripe/route.ts` around lines 1007 - 1028, When applyStripePaidAndQueueShipmentAtomic returns with applyResult.applied === false, add a structured warning log before proceeding to markStripeAttemptFinal/logWebhookEvent so duplicate or idempotent webhook replays are visible; locate the call to applyStripePaidAndQueueShipmentAtomic and insert a warning using the existing logger (include order.id, requestId, pspChargeId/pspPaymentMethod, and reason like "applyResult.applied=false") and only then continue to call markStripeAttemptFinal and logWebhookEvent; also ensure the warn is emitted even when applyResult.shipmentQueued is false so suppressed applies are observable.
🤖 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/.env.example`:
- Line 164: The file ends without a trailing newline which fails dotenv-linter;
add a single ending blank line by ensuring the last line (GROQ_API_KEY=) is
followed by a newline character so the file terminates with an empty line.
In `@frontend/app/`[locale]/shop/cart/CartPageClient.tsx:
- Around line 147-151: The checkout can be triggered before shipping
availability resolves because shippingMethodsLoading is initialized false;
initialize shippingMethodsLoading to true (useState<ShippingMethod[]>(...?) ->
useState(true)) so the UI treats shipping as loading until the first fetch
completes, and ensure any checkout gating logic (e.g., canPlaceOrder) checks
shippingMethodsLoading or shippingAvailable before allowing submission; update
usage of setShippingMethodsLoading in your fetch/shipping methods (e.g., where
you call fetchShippingMethods or loadShippingOptions) to set false when the
initial load finishes or to set shippingAvailable/shippingReasonCode
accordingly.
In `@frontend/app/api/shop/webhooks/stripe/route.ts`:
- Around line 421-436: The update currently sets shipping_status = 'queued'
based on the pre-computed shouldAttemptEnqueue flag (see shouldAttemptEnqueue
and updated_order), which can become stale and create an orphaned queued status
without a shipping_shipments row; change the SQL so shipping_status is derived
from the actual insertion result (inserted_shipment) instead of the pre-read
flag: remove the CASE branch that uses ${shouldAttemptEnqueue} from the
updated_order SET clause and add a follow-up CTE that updates
orders.shipping_status to 'queued' only when inserted_shipment exists (and keep
eligible_for_enqueue / inventoryCommittedForShippingSql checks in the insertion
CTE), ensuring shipping_status is tied to the real inserted_shipment outcome.
In `@frontend/db/schema/shop.ts`:
- Around line 160-168: shippingAmountMinor lacks a non-negative CHECK constraint
which allows invalid negative shipping costs; add a CHECK on shippingAmountMinor
to enforce >= 0 (mirror the pattern used by
orders_total_amount_minor_non_negative) and name it e.g.
shipping_amount_minor_non_negative so it sits alongside the existing order
checks in the schema; update the schema entry for shippingAmountMinor and the
table-level constraints block where other checks (like
orders_total_amount_minor_non_negative) are declared.
In `@frontend/lib/services/shop/shipping/checkout-payload.ts`:
- Around line 75-163: The functions shippingUnavailableMessage and
buildCheckoutShippingPayload are returning hardcoded English strings for
user-facing errors; change them to return error keys (or accept a translator
function) so localization occurs in the UI. Specifically, update
buildCheckoutShippingPayload to return only code and a new messageKey (or remove
message text) for all failure branches (codes like 'SHIPPING_UNAVAILABLE',
'MISSING_SHIPPING_METHOD', 'MISSING_SHIPPING_CITY',
'MISSING_SHIPPING_WAREHOUSE', 'MISSING_SHIPPING_ADDRESS',
'MISSING_RECIPIENT_NAME', 'INVALID_RECIPIENT_PHONE'), and refactor
shippingUnavailableMessage to return a key (e.g.
'shipping.unavailable.shop_disabled' etc.) or accept a translator param
t(messageKey) to produce localized text; ensure callers in the UI map these
messageKey/code values to t(...) for display.
In `@frontend/messages/pl.json`:
- Around line 419-422: The Polish locale's recipientPhone placeholder uses the
Poland code (+48) but should match the Ukraine format used in en.json/uk.json;
update the "recipientPhone" -> "placeholder" value to use the Ukrainian example
(e.g. "+380XXXXXXXXX або 0XXXXXXXXX" or simply "+380XXXXXXXXX") so it aligns
with Nova Poshta requirements and other locales.
---
Outside diff comments:
In `@frontend/app/`[locale]/shop/orders/[id]/page.tsx:
- Around line 141-198: The try/catch is currently catching Next.js's notFound()
sentinel and converting it into a 500; move the notFound() call outside the try
block (or wrap the DB logic in an IIFE like the admin page) so the sentinel can
propagate. Concretely, inside the code that queries rows (the block using
db.select(...).from(orders)... and the variable rows), ensure you perform the
rows.length === 0 check and call notFound() before the outer catch that logs via
logError('User order detail page failed', error) and throws
Error('ORDER_DETAIL_LOAD_FAILED'); keep the rest of order assembly (base, items,
toOrderItem, createdAt/updatedAt/restockedAt conversion) inside the try so only
real errors are caught.
---
Duplicate comments:
In `@frontend/app/`[locale]/admin/shop/orders/[id]/page.tsx:
- Around line 282-294: The dt labels "Shipping status" and "Tracking number" are
hardcoded; use the loaded translations from
getTranslations('shop.orders.detail') instead. Replace the literal strings in
the two <dt> elements with the translation keys t('shippingStatus') and
t('trackingNumber') (keeping the surrounding markup and classes intact) so the
page uses the locale-specific strings from the shop.orders.detail namespace;
ensure t is the translation function returned where getTranslations was called
and used in this component.
In `@frontend/lib/services/orders/monobank-janitor.ts`:
- Around line 361-370: clearAppliedEventClaim is correctly defined and isolated;
no code change required—just verify the call site in the main processing block
that invokes clearAppliedEventClaim is wrapped in its dedicated try/catch so
failures cannot affect the applied/failed counters (the function name
clearAppliedEventClaim and its invocation in the main processing loop should be
used to locate both definitions).
In `@frontend/lib/services/shop/shipping/nova-poshta-catalog.ts`:
- Around line 310-325: The function cacheWarehousesBySettlement currently
hardcodes deactivated: 0; change it to return the actual number of deactivated
rows by having deactivateMissingWarehouses return its deactivated count (e.g.,
Promise<number> or {deactivated:number}) and await that result here; update
cacheWarehousesBySettlement to call getWarehousesBySettlementRef,
upsertWarehouses, then const deactivated = await
deactivateMissingWarehouses({...}) and return { upserted: warehouses.length,
deactivated }; modify the deactivateMissingWarehouses implementation/signature
to compute and return the count of rows it marks inactive so the value is
accurate.
In `@frontend/lib/services/shop/shipping/nova-poshta-client.ts`:
- Around line 439-442: RecipientAddressName currently uses an || chain that
drops addressLine2 when addressLine1 is present; update the assignment for
RecipientAddressName (the object property where input.recipient is used) to join
input.recipient.addressLine1 and input.recipient.addressLine2 (e.g., concatenate
non-empty parts with a space or comma, trim final string) and fall back to
undefined if both are empty so the courier receives the full combined address
line.
- Around line 237-267: callNp lacks a fetch timeout and can hang; wrap the fetch
in an AbortController with a reasonable timeout (e.g., 5–15s): create an
AbortController before calling fetch, pass controller.signal to fetch, set a
timer (via setTimeout) that calls controller.abort() after the timeout, and
clear the timer after a successful response; update the catch branch to treat
AbortError distinctly when throwing NovaPoshtaApiError. Ensure changes are made
inside the callNp function around the fetch call and reference the existing
NovaPoshtaApiError and response handling logic.
In `@frontend/lib/services/shop/shipping/shipments-worker.ts`:
- Around line 256-265: The function declaredCostUahFromMinor can produce values
below Nova Poshta’s minimum declared cost; update it to clamp the final rounded
result to a documented minimum (e.g., introduce a constant like
MIN_NOVA_POSHTA_DECLARED_COST_UAH) after the existing rounding logic and before
returning, so return Math.max(roundedValue, MIN_NOVA_POSHTA_DECLARED_COST_UAH);
keep the existing input validation and error (buildFailure) unchanged and ensure
the new constant is clearly named and documented for future reference.
---
Nitpick comments:
In `@frontend/.env.example`:
- Around line 105-107: Reorder the two environment variable declarations so
STRIPE_WEBHOOK_INSTANCE_ID appears before WEBHOOK_INSTANCE_ID in
frontend/.env.example to satisfy dotenv-linter; locate the block containing
WEBHOOK_INSTANCE_ID and STRIPE_WEBHOOK_INSTANCE_ID and swap their lines so
STRIPE_WEBHOOK_INSTANCE_ID is listed first.
- Around line 66-69: Reorder the Monobank env keys in .env.example to satisfy
dotenv-linter: place MONO_REFUND_ENABLED first, then MONO_WEBHOOK_CLAIM_TTL_MS,
and finally MONO_WEBHOOK_MODE; update the block containing MONO_WEBHOOK_MODE,
MONO_WEBHOOK_CLAIM_TTL_MS, and MONO_REFUND_ENABLED so the variables appear in
the new order and preserve their values/comments.
- Around line 48-55: The Stripe env block violates dotenv-linter ordering; swap
the lines so STRIPE_MODE appears before STRIPE_PAYMENTS_ENABLED. Edit the
frontend/.env.example block containing NEXT_PUBLIC_STRIPE_PUBLISHABLE_KEY,
STRIPE_PAYMENTS_ENABLED, STRIPE_MODE, STRIPE_SECRET_KEY, STRIPE_WEBHOOK_SECRET
and move the STRIPE_MODE entry to precede STRIPE_PAYMENTS_ENABLED so the keys
read NEXT_PUBLIC_STRIPE_PUBLISHABLE_KEY, STRIPE_MODE, STRIPE_PAYMENTS_ENABLED,
STRIPE_SECRET_KEY, STRIPE_WEBHOOK_SECRET to satisfy the linter.
- Around line 81-93: The NP_ environment variable block in .env.example is not
in the dotenv-linter expected order; reorder the NP_* keys into alphabetical
order so the linter passes. Specifically locate the NP_* entries (NP_API_KEY,
NP_API_BASE, NP_SENDER_REF, NP_SENDER_CONTACT_REF, NP_SENDER_ADDRESS_REF,
NP_SENDER_CITY_REF, NP_SENDER_PHONE, NP_TIMEOUT_MS, NP_MAX_RETRIES,
NP_RETRY_DELAY_MS) and sort them alphabetically (e.g., NP_API_BASE before
NP_API_KEY, NP_MAX_RETRIES before NP_RETRY_DELAY_MS, etc.), keeping the same
comments and blank lines semantics. Ensure no duplicates or spacing changes
beyond reordering.
In `@frontend/app/api/shop/webhooks/stripe/route.ts`:
- Around line 1007-1028: When applyStripePaidAndQueueShipmentAtomic returns with
applyResult.applied === false, add a structured warning log before proceeding to
markStripeAttemptFinal/logWebhookEvent so duplicate or idempotent webhook
replays are visible; locate the call to applyStripePaidAndQueueShipmentAtomic
and insert a warning using the existing logger (include order.id, requestId,
pspChargeId/pspPaymentMethod, and reason like "applyResult.applied=false") and
only then continue to call markStripeAttemptFinal and logWebhookEvent; also
ensure the warn is emitted even when applyResult.shipmentQueued is false so
suppressed applies are observable.
In `@frontend/db/schema/shop.ts`:
- Around line 594-625: Add a non-negative check constraint for attemptCount on
the shippingShipments table: ensure the schema definition for shippingShipments
includes a CHECK(attempt_count >= 0) (either on the attemptCount column or as a
table-level constraint) so attemptCount (field name attemptCount / DB column
attempt_count) cannot be negative; update the table definition where
shippingShipments is declared to include this constraint alongside the existing
indexes and unique index.
- Around line 240-261: The CHECK pair for shipping fields enforces
presence/absence of shippingProvider, shippingMethodCode, and shippingStatus
based on shippingRequired but omits shippingPayer, allowing NULL payer when
shippingRequired is true; update the 'orders_shipping_present_when_required_chk'
(and symmetrically 'orders_shipping_null_when_not_required_chk' if you want
strict parity) to include ${table.shippingPayer} in the AND conditions so that
when shippingRequired is true shippingPayer IS NOT NULL (and when
shippingRequired is TRUE in the null-check ensure shippingPayer IS NULL) —
modify the check() calls around 'orders_shipping_present_when_required_chk' and
'orders_shipping_null_when_not_required_chk' to add ${table.shippingPayer} to
the grouped predicates.
- Around line 627-650: The npCities table's index np_cities_active_name_idx
currently uses the default B-tree operator class which won't help prefix LIKE
searches on nameUa in non-C collations; update the index definition on npCities
(np_cities_active_name_idx) so the nameUa/name_ua column uses the
text_pattern_ops operator class (or create a separate index with
text_pattern_ops on nameUa) to accelerate prefix queries like 'prefix%'. Ensure
the change targets the index construction in the pgTable call that references
table.isActive and table.nameUa.
In `@frontend/lib/services/orders/_shared.ts`:
- Around line 148-170: mergeCheckoutItems currently normalizes each item with
normalizeCheckoutItem then calls checkoutItemMergeKey which re-normalizes the
same item; to fix, call checkoutItemMergeKey with the already-normalized
variable (it) so the key builder doesn't re-run normalization, and when
computing mergedQty use it.quantity (the normalized copy) instead of
item.quantity; keep the rest of the logic (map, MAX_QUANTITY_PER_LINE check,
InvalidPayloadError throw) unchanged.
In `@frontend/lib/services/orders/monobank-janitor.ts`:
- Around line 860-942: The loop reads candidates via readUnclaimedJob3Candidates
then applies each with applyMonoWebhookEvent without an atomic per-event claim,
allowing concurrent janitor runs to process the same event; fix by making the
candidate selection atomic—either change readUnclaimedJob3Candidates to SELECT
... FOR UPDATE SKIP LOCKED (or use a transaction that updates
claim_expires_at/claimer and returns the rows) so rows are locked/marked
exclusively before calling applyMonoWebhookEvent, or add an immediate atomic
update-by-id claim step before applyMonoWebhookEvent that returns whether the
claim succeeded and skip processing if not; ensure clearAppliedEventClaim logic
still runs only for successfully claimed/applied events (use eventId + claim
ownership) to avoid redundant clear attempts.
In `@frontend/lib/services/orders/monobank-webhook.ts`:
- Around line 476-569: The update currently sets shipping_status based on the
precomputed boolean args.enqueueShipment which can drift from DB-side
eligibility; inside atomicMarkPaidOrderAndSucceedAttempt update the
orders.shipping_status using the same database predicates as the
eligible_for_enqueue CTE (i.e. payment_status = 'paid', shipping_required =
true, shipping_provider = 'nova_poshta', shipping_method_code IS NOT NULL, and
the inventoryCommittedForShippingSql(...) predicate against
updated_order.inventory_status) so the CASE in the orders UPDATE (in
updated_order) mirrors the eligible_for_enqueue logic and keeps shipping_status
consistent with inserted_shipment/eligible_for_enqueue and the inserted_shipment
CTE.
In `@frontend/lib/services/shop/shipping/nova-poshta-catalog.ts`:
- Around line 163-206: upsertCities currently builds one giant INSERT VALUES
block (values, sql.join, db.execute) which can exceed SQL/driver limits for
large inputs; modify upsertCities to process rows in bounded batches (e.g.,
chunk size 200–500) and perform the same INSERT ... ON CONFLICT call per chunk
inside a loop, reusing the same VALUES/sql templates and runId, or if you intend
to rely on small responses, add a clear comment near upsertCities explaining the
expected upper bound and why batching is unnecessary.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
frontend/.env.examplefrontend/app/[locale]/admin/shop/orders/[id]/page.tsxfrontend/app/[locale]/shop/cart/CartPageClient.tsxfrontend/app/[locale]/shop/orders/[id]/page.tsxfrontend/app/api/shop/webhooks/stripe/route.tsfrontend/db/schema/shop.tsfrontend/drizzle/0018_shop_orders_shipping_invariants.sqlfrontend/drizzle/meta/0018_snapshot.jsonfrontend/drizzle/meta/_journal.jsonfrontend/lib/services/orders/_shared.tsfrontend/lib/services/orders/checkout.tsfrontend/lib/services/orders/monobank-janitor.tsfrontend/lib/services/orders/monobank-webhook.tsfrontend/lib/services/shop/shipping/admin-actions.tsfrontend/lib/services/shop/shipping/availability.tsfrontend/lib/services/shop/shipping/checkout-payload.tsfrontend/lib/services/shop/shipping/inventory-eligibility.tsfrontend/lib/services/shop/shipping/nova-poshta-catalog.tsfrontend/lib/services/shop/shipping/nova-poshta-client.tsfrontend/lib/services/shop/shipping/shipments-worker.tsfrontend/lib/shop/locale.tsfrontend/messages/en.jsonfrontend/messages/pl.jsonfrontend/messages/uk.json
✅ Files skipped from review due to trivial changes (1)
- frontend/messages/uk.json
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/drizzle/meta/_journal.json
- frontend/lib/services/shop/shipping/inventory-eligibility.ts
…lity reason code)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
frontend/app/[locale]/shop/cart/CartPageClient.tsx (1)
147-153:⚠️ Potential issue | 🟡 MinorInitialize
shippingMethodsLoadingto true to avoid premature checkout.Line 148 starts it as
false, socanPlaceOrdercan go true before the first shipping-methods fetch completes.🔧 Suggested fix
- const [shippingMethodsLoading, setShippingMethodsLoading] = useState(false); + const [shippingMethodsLoading, setShippingMethodsLoading] = useState(true);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/`[locale]/shop/cart/CartPageClient.tsx around lines 147 - 153, The shippingMethodsLoading state is initialized false which allows canPlaceOrder to become true before the first fetch finishes; change the initializer of shippingMethodsLoading in CartPageClient (the useState call named shippingMethodsLoading/setShippingMethodsLoading) to true and ensure any existing fetchShippingMethods logic sets setShippingMethodsLoading(false) on both success and error so loading reflects the actual fetch lifecycle.
🤖 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/`[locale]/shop/cart/CartPageClient.tsx:
- Around line 1141-1160: The warehouse search input with id
"shipping-warehouse-search" lacks an accessible label; add one so screen readers
can identify it: either render a <label htmlFor="shipping-warehouse-search">
(use a visually-hidden CSS class if you don't want it visible) with localized
text from t('delivery.warehouse.label') or add an aria-label/aria-labelledby
that uses the same localized string. Update the JSX near the input (the block
using warehouseQuery, setWarehouseQuery, setSelectedWarehouseRef,
setSelectedWarehouseName, and selectedCityRef) to include the label approach you
choose.
---
Duplicate comments:
In `@frontend/app/`[locale]/shop/cart/CartPageClient.tsx:
- Around line 147-153: The shippingMethodsLoading state is initialized false
which allows canPlaceOrder to become true before the first fetch finishes;
change the initializer of shippingMethodsLoading in CartPageClient (the useState
call named shippingMethodsLoading/setShippingMethodsLoading) to true and ensure
any existing fetchShippingMethods logic sets setShippingMethodsLoading(false) on
both success and error so loading reflects the actual fetch lifecycle.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/app/[locale]/shop/cart/CartPageClient.tsxfrontend/lib/shop/locale.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/lib/shop/locale.ts
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/lib/services/orders/monobank-webhook.ts (1)
489-540:⚠️ Potential issue | 🟠 MajorDuplicate eligibility conditions create a DB consistency hazard.
The 6-condition gate (Lines 489–498
CASEand Lines 529–540eligible_for_enqueue) is copy-pasted. If they ever diverge — a common outcome during future edits — the order row will haveshipping_status = 'queued'with no correspondingshipping_shipmentsrecord, or vice versa.The safer pattern is to make
eligible_for_enqueuethe single source of truth and derive theshipping_statusupdate from it:🛡️ Proposed fix — derive shipping_status from eligible_for_enqueue rather than duplicating conditions
updated_order as ( update orders set status = 'PAID', - shipping_status = case - when ${args.enqueueShipment} - and payment_status = 'paid' - and shipping_required = true - and shipping_provider = 'nova_poshta' - and shipping_method_code is not null - and ${inventoryCommittedForShippingSql(sql`orders.inventory_status`)} - then 'queued'::shipping_status - else shipping_status - end, psp_charge_id = ${args.invoiceId}, psp_metadata = ${args.mergedMetaSql}, updated_at = ${args.now} where id = ${args.orderId}::uuid and payment_provider = 'monobank' and exists ( select 1 from payment_attempts where id = ${args.attemptId}::uuid ) returning id, payment_status, inventory_status, shipping_required, shipping_provider, shipping_method_code ), ... eligible_for_enqueue as ( select id from updated_order where ${args.enqueueShipment} and payment_status = 'paid' and shipping_required = true and shipping_provider = 'nova_poshta' and shipping_method_code is not null and ${inventoryCommittedForShippingSql( sql`updated_order.inventory_status` )} ), inserted_shipment as ( insert into shipping_shipments (...) select id, 'nova_poshta', 'queued', 0, ${args.now}, ${args.now} from eligible_for_enqueue on conflict (order_id) do nothing returning order_id ), + shipping_status_update as ( + update orders + set shipping_status = 'queued'::shipping_status, + updated_at = ${args.now} + where id in (select order_id from inserted_shipment) + returning id + ) select (select id from updated_order) as order_id, (select id from updated_attempt) as attempt_id, (select count(*)::int from inserted_shipment) as inserted_shipment_count🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/services/orders/monobank-webhook.ts` around lines 489 - 540, The update duplicates the 6-condition gate in the CASE and in eligible_for_enqueue; make eligible_for_enqueue the single source of truth and derive shipping_status from it by changing the CASE in the orders update (the CTE producing updated_order) to set shipping_status = 'queued' only when the row exists in eligible_for_enqueue (e.g. replace the six-condition expression that uses args.enqueueShipment and inventoryCommittedForShippingSql(sql`orders.inventory_status`) with an existence test against the eligible_for_enqueue CTE for that order id), leaving eligible_for_enqueue unchanged (it still uses args.enqueueShipment and inventoryCommittedForShippingSql(sql`updated_order.inventory_status`) so the logic is centralized).
♻️ Duplicate comments (5)
frontend/app/[locale]/admin/shop/orders/[id]/page.tsx (1)
392-408:⚠️ Potential issue | 🟡 MinorHardcoded
"Qty:","Unit:","Line:"visible labels — i18n incomplete.The screen-reader
<dt>counterparts already uset('quantity'),t('unitPrice'), andt('lineTotal'), but the visible<dd>prefixes are bare English string literals. These need localized short-form keys (e.g.t('qtyShort'),t('unitShort'),t('lineShort')) or the existing keys if a short form exists.♻️ Proposed fix
-<dd className="text-sm">Qty: {it.quantity}</dd> +<dd className="text-sm">{t('qtyShort')}: {it.quantity}</dd>-<dd className="text-muted-foreground text-sm"> - Unit:{' '} - {safeFormatMoneyMajor(it.unitPrice, currency, locale)} -</dd> +<dd className="text-muted-foreground text-sm"> + {t('unitShort')}:{' '} + {safeFormatMoneyMajor(it.unitPrice, currency, locale)} +</dd>-<dd className="text-sm font-medium"> - Line:{' '} - {safeFormatMoneyMajor(it.lineTotal, currency, locale)} -</dd> +<dd className="text-sm font-medium"> + {t('lineShort')}:{' '} + {safeFormatMoneyMajor(it.lineTotal, currency, locale)} +</dd>Add the corresponding keys to the
shop.orders.detailtranslation namespace.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/`[locale]/admin/shop/orders/[id]/page.tsx around lines 392 - 408, Replace the hardcoded visible prefixes "Qty:", "Unit:", and "Line:" inside the dd elements that render quantities and prices with localized keys (e.g. use t('qtyShort'), t('unitShort'), t('lineShort') where the current dd nodes use "Qty:", "Unit:", "Line:"); update the corresponding translation namespace (shop.orders.detail) to include these short-form keys; ensure the dt elements remain using t('quantity'), t('unitPrice'), t('lineTotal') and that safeFormatMoneyMajor is still used to format amounts.frontend/lib/services/orders/monobank-janitor.ts (2)
375-388:clearAppliedEventClaimresolved and correctly guarded.The function is now defined (resolving the prior undefined-symbol issue) and the
and applied_at is not nullguard on line 386 is correctly defensive: ifpersistEventOutcomenever persistedapplied_at, the claim silently expires rather than being spuriously cleared. The ownership check (claimed_by = ${args.runId}) further prevents cross-run interference.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/services/orders/monobank-janitor.ts` around lines 375 - 388, Define the previously-missing function clearAppliedEventClaim to update monobank_events and clear claimed_at, claim_expires_at, and claimed_by only for the given eventId/runId when applied_at is not null; ensure the SQL uses the arguments eventId and runId (as in the function signature) and preserves the defensive guard "and applied_at is not null" plus the ownership check "and claimed_by = ${args.runId}" so claims are only cleared for events that were actually applied and only by the owning run.
912-936: Double-counting fix confirmed —clearAppliedEventClaimisolated from the main error path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/services/orders/monobank-janitor.ts` around lines 912 - 936, The change correctly isolates clearAppliedEventClaim from the main applied/noop counting path; ensure the call to clearAppliedEventClaim remains outside the logic that increments applied and noop (see isAppliedResult(result.appliedResult), applied, noop) so that any errors from clearAppliedEventClaim do not affect counts, and keep the catch logging via logWarn('internal_monobank_janitor_job3_clear_claim_failed', { ...args.baseMeta, code: 'JANITOR_JOB3_CLEAR_CLAIM_FAILED', runId: args.runId, eventId: eventRow.id, invoiceId: eventRow.invoice_id, attemptId: eventRow.attempt_id, error: clearError instanceof Error ? clearError.message : String(clearError) }) unchanged.frontend/lib/services/shop/shipping/nova-poshta-catalog.ts (1)
279-309:⚠️ Potential issue | 🟠 Major
deactivatedcount still depends onrowCount, which is adapter-shape fragile.Lines 289-290 and 306-307 assume
db.execute()exposesrowCount. If that shape isn’t provided by your drizzle driver,deactivatedstays0even when rows are updated.🐛 Proposed fix (use RETURNING rows as the count source)
async function deactivateMissingWarehouses(args: { settlementRef: string; keepRefs: string[]; runId: string; }): Promise<number> { @@ if (!keepRefs.length) { - const res = await db.execute(sql` + const res = await db.execute<{ ref: string }>(sql` UPDATE np_warehouses SET is_active = false, last_sync_run_id = ${args.runId}::uuid, updated_at = now() WHERE settlement_ref = ${args.settlementRef} AND is_active = true + RETURNING ref `); - - const rowCount = - typeof (res as any)?.rowCount === 'number' ? (res as any).rowCount : 0; - - return rowCount; + return Array.isArray((res as any).rows) ? (res as any).rows.length : 0; } @@ - const res = await db.execute(sql` + const res = await db.execute<{ ref: string }>(sql` UPDATE np_warehouses SET is_active = false, last_sync_run_id = ${args.runId}::uuid, updated_at = now() WHERE settlement_ref = ${args.settlementRef} AND is_active = true AND ref NOT IN (${sql.join(refs, sql`, `)}) + RETURNING ref `); - - const rowCount = - typeof (res as any)?.rowCount === 'number' ? (res as any).rowCount : 0; - - return rowCount; + return Array.isArray((res as any).rows) ? (res as any).rows.length : 0; }Run this to verify result-shape assumptions before finalizing:
#!/bin/bash set -euo pipefail echo "=== drizzle version in frontend/package.json ===" rg -n '"drizzle-orm"' frontend/package.json echo echo "=== DB adapter implementation (inspect execute wiring) ===" cat frontend/db/index.ts echo echo "=== rowCount usage vs rows usage around shipping services ===" rg -n -C2 'rowCount|\.rows' frontend/lib/services/shop/shippingExpected: if
rowCountis not part of your adapter result contract, this function should switch toRETURNING-based counting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/services/shop/shipping/nova-poshta-catalog.ts` around lines 279 - 309, The deactivated count logic in the block using db.execute (inside the function that builds refs from keepRefs and the earlier single-update branch) relies on a driver-specific res.rowCount shape; instead modify both UPDATE statements (the single-branch and the NOT IN branch that uses sql.join(refs, sql`, `)) to append a RETURNING clause (e.g. RETURNING id or ref) and then derive deactivated from the returned rows length (inspect res.rows or the adapter's returned array shape) rather than referencing (res as any).rowCount; update references to args.runId, args.settlementRef, keepRefs, db.execute and sql.join accordingly so both paths use the RETURNING-based count.frontend/.env.example (1)
80-90:⚠️ Potential issue | 🟠 MajorRequired Nova Poshta sender keys in the example are mismatched/missing.
Line 85 documents
NP_SENDER_ADDRESS_REF, but config validation expectsNP_SENDER_WAREHOUSE_REF, and requiredNP_SENDER_NAMEis missing. With shipping enabled, this can fail fast withNovaPoshtaConfigError.🐛 Proposed fix
# Required when shipping is enabled (SHOP_SHIPPING_ENABLED=1 and SHOP_SHIPPING_NP_ENABLED=1). # If shipping is enabled without required NP config, app throws NovaPoshtaConfigError at runtime. NP_API_KEY= # Optional if code has a default; set explicitly in production for clarity NP_API_BASE= -NP_SENDER_ADDRESS_REF= +NP_SENDER_WAREHOUSE_REF= NP_SENDER_CITY_REF= NP_SENDER_CONTACT_REF= +NP_SENDER_NAME= NP_SENDER_PHONE= NP_SENDER_REF= +NP_SENDER_EDRPOU=🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/.env.example` around lines 80 - 90, Update the example environment variables to match the config validation: replace the incorrect NP_SENDER_ADDRESS_REF with NP_SENDER_WAREHOUSE_REF and add the missing required NP_SENDER_NAME entry; ensure NP_API_KEY, NP_API_BASE, NP_SENDER_WAREHOUSE_REF, NP_SENDER_CITY_REF, NP_SENDER_CONTACT_REF, NP_SENDER_PHONE, NP_SENDER_REF and the new NP_SENDER_NAME are present in the example so NovaPoshtaConfigError isn’t triggered when shipping is enabled.
🧹 Nitpick comments (6)
frontend/drizzle/0019_p2_shop_invariants.sql (1)
33-40: Consider the cumulative scan cost on largeorderstables.Three consecutive
VALIDATE CONSTRAINTcalls againstorderseach perform a full sequential scan. On a modest-sized table this is fine, but ifordersgrows large, you may want to combine the three order constraints into a single scan by adding them in oneALTER TABLE ... ADD CONSTRAINT ... ADD CONSTRAINT ...statement (PostgreSQL 12+ supports multiple sub-commands per ALTER). Alternatively, just be aware of the migration runtime.Not a blocker — the current approach is correct and safe.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/drizzle/0019_p2_shop_invariants.sql` around lines 33 - 40, Multiple VALIDATE CONSTRAINT commands on "orders" cause repeated full-table scans; combine the three validations into a single ALTER TABLE "orders" statement that lists the constraints orders_shipping_amount_minor_non_negative_chk, orders_shipping_payer_null_when_not_required_chk, and orders_shipping_payer_present_when_required_chk as multiple sub-commands in one ALTER to perform a single scan (or leave as-is if longer runtime is acceptable) — update the migration to use one ALTER TABLE "orders" ... VALIDATE CONSTRAINT ... VALIDATE CONSTRAINT ... sequence instead of three separate ALTERs.frontend/app/[locale]/admin/shop/orders/[id]/page.tsx (2)
5-5:noStore()is redundant alongsidedynamic = 'force-dynamic'.
dynamic = 'force-dynamic'at module level already opts the entire route out of caching. Theunstable_noStoreimport and itsnoStore()call inside the component body have no additional effect.♻️ Proposed cleanup
-import { unstable_noStore as noStore } from 'next/cache';noStore(); - const { locale, id } = await params;(Remove the
noStore()call at line 120 and the now-unused import.)Also applies to: 120-120
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/`[locale]/admin/shop/orders/[id]/page.tsx at line 5, Remove the redundant unstable_noStore import and noStore() call since the module already sets dynamic = 'force-dynamic'; specifically delete the import statement "import { unstable_noStore as noStore } from 'next/cache';" and remove the noStore() invocation inside the page component (the default export in page.tsx), leaving dynamic = 'force-dynamic' as the single mechanism opting out of caching.
305-349:{isAdmin && ...}guards are alwaystrueand can be removed.
notFound()is called on line 138 when!isAdmin, so every code path reaching the JSX already guaranteesisAdmin === true. The three conditional blocks add noise without any runtime effect.♻️ Proposed cleanup (example for first block; apply similarly to the other two)
-{isAdmin && ( - <div> - <dt className="text-muted-foreground text-xs">{t('provider')}</dt> - <dd className="text-sm">{String(order.paymentProvider)}</dd> - </div> -)} +<div> + <dt className="text-muted-foreground text-xs">{t('provider')}</dt> + <dd className="text-sm">{String(order.paymentProvider)}</dd> +</div>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/`[locale]/admin/shop/orders/[id]/page.tsx around lines 305 - 349, The three JSX blocks are wrapped in redundant {isAdmin && ...} checks because notFound() already prevents non-admins from reaching this render; remove the conditional guards and render the dl blocks unconditionally. Specifically, update the sections using isAdmin around order.paymentProvider, paymentIntentId/idempotencyKey, and stockRestored/restockedFormatted so they render directly (keep the dt/dd contents and classNames intact) and delete the {isAdmin && ( ... )} wrappers; no other logic changes required.frontend/lib/services/orders/monobank-janitor.ts (1)
937-947: No early claim release on failure — consider the retry-latency implication.Unlike Job1/Job2, which call
releaseAttemptLeasein afinallyblock so a failed row is immediately eligible for the next run, Job3 leaves the claim locked untilclaim_expires_at. This means a transiently-failing event is retried only after the fullleaseSecondswindow (15 s–30 min depending onMONO_JANITOR_LEASE_SECONDS), rather than on the next janitor invocation.If fast recovery from transient errors (e.g., DB blip) is important for Job3, add an explicit release on the error path:
♻️ Optional: release claim on failure to align with Job1/Job2 pattern
} catch (error) { failed += 1; logError('internal_monobank_janitor_job3_event_failed', error, { ...args.baseMeta, code: 'JANITOR_JOB3_EVENT_FAILED', runId: args.runId, eventId: eventRow.id, invoiceId: eventRow.invoice_id, attemptId: eventRow.attempt_id, }); + // Release claim so the event can be retried on the next run + // rather than waiting for lease expiry. + try { + await db.execute(sql` + update monobank_events + set claimed_at = null, + claim_expires_at = null, + claimed_by = null + where id = ${eventRow.id}::uuid + and claimed_by = ${args.runId} + and applied_at is null + `); + } catch (_releaseErr) { + // best-effort + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/services/orders/monobank-janitor.ts` around lines 937 - 947, Job3 currently leaves the claim locked on error, causing retries to wait until claim_expires_at; mirror Job1/Job2 behavior by calling releaseAttemptLease(...) when handling failures in the catch block for Job3 (the same attempt identifiers used for logging: eventRow.id, eventRow.attempt_id, args.runId, etc.), or move the releaseAttemptLease call into a finally block around the try/catch in monobank-janitor.ts so that failed rows are released immediately instead of waiting for MONO_JANITOR_LEASE_SECONDS.frontend/lib/services/shop/shipping/nova-poshta-catalog.ts (1)
214-269: Chunk warehouse upserts to avoid oversized SQL payloads.
upsertWarehouses()sends all rows in a singleINSERT ... VALUES. Large settlements can produce very large statements/parameter counts and hurt reliability.♻️ Proposed refactor
async function upsertWarehouses(rows: NovaPoshtaWarehouse[], runId: string) { if (!rows.length) return; - - const values = rows.map(item => { - return sql`( - ${item.ref}, - ${item.cityRef}, - ${item.settlementRef}, - ${item.number}, - ${item.type}, - ${item.name}, - ${item.nameRu}, - ${item.address}, - ${item.addressRu}, - ${item.isPostMachine}, - true, - ${runId}::uuid, - now(), - now() - )`; - }); - - await db.execute(sql` + const CHUNK_SIZE = 300; + for (let i = 0; i < rows.length; i += CHUNK_SIZE) { + const chunk = rows.slice(i, i + CHUNK_SIZE); + const values = chunk.map(item => sql`( + ${item.ref}, + ${item.cityRef}, + ${item.settlementRef}, + ${item.number}, + ${item.type}, + ${item.name}, + ${item.nameRu}, + ${item.address}, + ${item.addressRu}, + ${item.isPostMachine}, + true, + ${runId}::uuid, + now(), + now() + )`); + + await db.execute(sql` INSERT INTO np_warehouses ( ref, city_ref, @@ ON CONFLICT (ref) DO UPDATE SET city_ref = EXCLUDED.city_ref, @@ is_active = true, last_sync_run_id = EXCLUDED.last_sync_run_id, updated_at = now() - `); + `); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/services/shop/shipping/nova-poshta-catalog.ts` around lines 214 - 269, upsertWarehouses currently builds one giant INSERT ... VALUES for all rows which can exceed DB parameter limits; modify upsertWarehouses to chunk rows (e.g., 500–1000 items per batch) and run the existing INSERT ... ON CONFLICT logic for each chunk in a loop: split the incoming rows array into batches, map each batch to its values array (same mapping logic currently using sql`(...)`), call db.execute with sql.join(values, sql`, `) per batch, and await each execution; keep the early return when rows.length is zero and reuse the same column list and ON CONFLICT update block for each chunk to preserve behavior.frontend/lib/services/shop/shipping/shipments-worker.ts (1)
518-530: Validate snapshot method against persistedshipping_method_code.You currently only check presence of
shipping_method_code. A mismatch between DB column and snapshotmethodCodecan slip through and build NP payload from inconsistent data.♻️ Proposed guard
if (!details.shipping_method_code) { throw buildFailure( 'SHIPPING_METHOD_MISSING', 'Shipping method is missing.', false ); } const parsedSnapshot = parseSnapshot(details.shipping_address); + if (parsedSnapshot.methodCode !== details.shipping_method_code) { + throw buildFailure( + 'SHIPPING_METHOD_MISMATCH', + 'Shipping method does not match persisted order method.', + false + ); + } + const payload = toNpPayload({ order: details, snapshot: parsedSnapshot, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/services/shop/shipping/shipments-worker.ts` around lines 518 - 530, The code only checks that details.shipping_method_code exists but does not ensure it matches the parsed snapshot, so parseSnapshot(details.shipping_address) may contain a different methodCode; update the guard to parse the snapshot first (parseSnapshot), compare parsedSnapshot.methodCode (or equivalent property) to details.shipping_method_code, and if they differ throw a failure (e.g., SHIPPING_METHOD_MISMATCH) before calling toNpPayload(order: details, snapshot: parsedSnapshot) so the NP payload is built only from consistent data.
🤖 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/.env.example`:
- Around line 82-84: The dotenv-linter warning is due to ordering of the NP
keys; move the NP_API_BASE declaration so it appears before NP_API_KEY in
frontend/.env.example (ensure the comment about optional/defaults remains with
NP_API_BASE if needed) so the two entries read NP_API_BASE then NP_API_KEY; no
other changes to values or comments are required.
In `@frontend/app/`[locale]/shop/cart/CartPageClient.tsx:
- Around line 293-299: The current parsing of the /shipping/methods response may
silently treat malformed HTTP 200 payloads as available=false but leave
shippingUnavailableHardBlock false; update the validation in CartPageClient
(variables available, reasonCode, methods) to fail closed: explicitly verify
types for data.available (boolean), data.reasonCode (matches
ShippingAvailabilityReasonCode) and data.methods (Array of ShippingMethod), and
if any check fails set available = false and set a hard block flag
(shippingUnavailableHardBlock = true) and populate reasonCode to a safe failure
code (or non-null sentinel) so checkout cannot be re-enabled on malformed
success responses; locate and modify the parsing logic around available,
reasonCode and methods to add these guards.
In `@frontend/app/api/shop/webhooks/stripe/route.ts`:
- Around line 421-427: The enqueue eligibility is currently decided using
pre-read booleans (e.g., shouldAttemptEnqueue /
args.paymentBecamePaidInThisApply and
args.shippingRequired/shippingProvider/shippingMethodCode) which can be stale;
instead compute eligible_for_enqueue from the DB-returned updated_order state
(e.g., use updated_order.payment_status / updated_order.shipping_required /
updated_order.shipping_provider / updated_order.shipping_method_code and call
isInventoryCommittedForShipping(updated_order.inventory_status)) at the point
just before generating the SQL/queue action so the decision reflects the
persisted state; apply the same change to the second occurrence referenced
around the block at 454-465 so both enqueue checks derive eligibility from
updated_order rather than args.
In `@frontend/lib/services/shop/shipping/shipments-worker.ts`:
- Around line 534-550: markSucceeded() can return null when the lease was lost;
update the try block to check the result of markSucceeded (the call to
markSucceeded with shipmentId: args.claim.id, runId: args.runId, providerRef:
created.providerRef, trackingNumber: created.trackingNumber) and if it returns
null treat this as a lease-loss outcome: do not call recordShippingMetric(...)
with name:'succeeded' nor return 'succeeded'—instead record/return an
appropriate lease-loss status (or throw) so the run is not misreported; place
this conditional immediately after the await markSucceeded(...) and before the
recordShippingMetric and return statements.
---
Outside diff comments:
In `@frontend/lib/services/orders/monobank-webhook.ts`:
- Around line 489-540: The update duplicates the 6-condition gate in the CASE
and in eligible_for_enqueue; make eligible_for_enqueue the single source of
truth and derive shipping_status from it by changing the CASE in the orders
update (the CTE producing updated_order) to set shipping_status = 'queued' only
when the row exists in eligible_for_enqueue (e.g. replace the six-condition
expression that uses args.enqueueShipment and
inventoryCommittedForShippingSql(sql`orders.inventory_status`) with an existence
test against the eligible_for_enqueue CTE for that order id), leaving
eligible_for_enqueue unchanged (it still uses args.enqueueShipment and
inventoryCommittedForShippingSql(sql`updated_order.inventory_status`) so the
logic is centralized).
---
Duplicate comments:
In `@frontend/.env.example`:
- Around line 80-90: Update the example environment variables to match the
config validation: replace the incorrect NP_SENDER_ADDRESS_REF with
NP_SENDER_WAREHOUSE_REF and add the missing required NP_SENDER_NAME entry;
ensure NP_API_KEY, NP_API_BASE, NP_SENDER_WAREHOUSE_REF, NP_SENDER_CITY_REF,
NP_SENDER_CONTACT_REF, NP_SENDER_PHONE, NP_SENDER_REF and the new NP_SENDER_NAME
are present in the example so NovaPoshtaConfigError isn’t triggered when
shipping is enabled.
In `@frontend/app/`[locale]/admin/shop/orders/[id]/page.tsx:
- Around line 392-408: Replace the hardcoded visible prefixes "Qty:", "Unit:",
and "Line:" inside the dd elements that render quantities and prices with
localized keys (e.g. use t('qtyShort'), t('unitShort'), t('lineShort') where the
current dd nodes use "Qty:", "Unit:", "Line:"); update the corresponding
translation namespace (shop.orders.detail) to include these short-form keys;
ensure the dt elements remain using t('quantity'), t('unitPrice'),
t('lineTotal') and that safeFormatMoneyMajor is still used to format amounts.
In `@frontend/lib/services/orders/monobank-janitor.ts`:
- Around line 375-388: Define the previously-missing function
clearAppliedEventClaim to update monobank_events and clear claimed_at,
claim_expires_at, and claimed_by only for the given eventId/runId when
applied_at is not null; ensure the SQL uses the arguments eventId and runId (as
in the function signature) and preserves the defensive guard "and applied_at is
not null" plus the ownership check "and claimed_by = ${args.runId}" so claims
are only cleared for events that were actually applied and only by the owning
run.
- Around line 912-936: The change correctly isolates clearAppliedEventClaim from
the main applied/noop counting path; ensure the call to clearAppliedEventClaim
remains outside the logic that increments applied and noop (see
isAppliedResult(result.appliedResult), applied, noop) so that any errors from
clearAppliedEventClaim do not affect counts, and keep the catch logging via
logWarn('internal_monobank_janitor_job3_clear_claim_failed', { ...args.baseMeta,
code: 'JANITOR_JOB3_CLEAR_CLAIM_FAILED', runId: args.runId, eventId:
eventRow.id, invoiceId: eventRow.invoice_id, attemptId: eventRow.attempt_id,
error: clearError instanceof Error ? clearError.message : String(clearError) })
unchanged.
In `@frontend/lib/services/shop/shipping/nova-poshta-catalog.ts`:
- Around line 279-309: The deactivated count logic in the block using db.execute
(inside the function that builds refs from keepRefs and the earlier
single-update branch) relies on a driver-specific res.rowCount shape; instead
modify both UPDATE statements (the single-branch and the NOT IN branch that uses
sql.join(refs, sql`, `)) to append a RETURNING clause (e.g. RETURNING id or ref)
and then derive deactivated from the returned rows length (inspect res.rows or
the adapter's returned array shape) rather than referencing (res as
any).rowCount; update references to args.runId, args.settlementRef, keepRefs,
db.execute and sql.join accordingly so both paths use the RETURNING-based count.
---
Nitpick comments:
In `@frontend/app/`[locale]/admin/shop/orders/[id]/page.tsx:
- Line 5: Remove the redundant unstable_noStore import and noStore() call since
the module already sets dynamic = 'force-dynamic'; specifically delete the
import statement "import { unstable_noStore as noStore } from 'next/cache';" and
remove the noStore() invocation inside the page component (the default export in
page.tsx), leaving dynamic = 'force-dynamic' as the single mechanism opting out
of caching.
- Around line 305-349: The three JSX blocks are wrapped in redundant {isAdmin &&
...} checks because notFound() already prevents non-admins from reaching this
render; remove the conditional guards and render the dl blocks unconditionally.
Specifically, update the sections using isAdmin around order.paymentProvider,
paymentIntentId/idempotencyKey, and stockRestored/restockedFormatted so they
render directly (keep the dt/dd contents and classNames intact) and delete the
{isAdmin && ( ... )} wrappers; no other logic changes required.
In `@frontend/drizzle/0019_p2_shop_invariants.sql`:
- Around line 33-40: Multiple VALIDATE CONSTRAINT commands on "orders" cause
repeated full-table scans; combine the three validations into a single ALTER
TABLE "orders" statement that lists the constraints
orders_shipping_amount_minor_non_negative_chk,
orders_shipping_payer_null_when_not_required_chk, and
orders_shipping_payer_present_when_required_chk as multiple sub-commands in one
ALTER to perform a single scan (or leave as-is if longer runtime is acceptable)
— update the migration to use one ALTER TABLE "orders" ... VALIDATE CONSTRAINT
... VALIDATE CONSTRAINT ... sequence instead of three separate ALTERs.
In `@frontend/lib/services/orders/monobank-janitor.ts`:
- Around line 937-947: Job3 currently leaves the claim locked on error, causing
retries to wait until claim_expires_at; mirror Job1/Job2 behavior by calling
releaseAttemptLease(...) when handling failures in the catch block for Job3 (the
same attempt identifiers used for logging: eventRow.id, eventRow.attempt_id,
args.runId, etc.), or move the releaseAttemptLease call into a finally block
around the try/catch in monobank-janitor.ts so that failed rows are released
immediately instead of waiting for MONO_JANITOR_LEASE_SECONDS.
In `@frontend/lib/services/shop/shipping/nova-poshta-catalog.ts`:
- Around line 214-269: upsertWarehouses currently builds one giant INSERT ...
VALUES for all rows which can exceed DB parameter limits; modify
upsertWarehouses to chunk rows (e.g., 500–1000 items per batch) and run the
existing INSERT ... ON CONFLICT logic for each chunk in a loop: split the
incoming rows array into batches, map each batch to its values array (same
mapping logic currently using sql`(...)`), call db.execute with sql.join(values,
sql`, `) per batch, and await each execution; keep the early return when
rows.length is zero and reuse the same column list and ON CONFLICT update block
for each chunk to preserve behavior.
In `@frontend/lib/services/shop/shipping/shipments-worker.ts`:
- Around line 518-530: The code only checks that details.shipping_method_code
exists but does not ensure it matches the parsed snapshot, so
parseSnapshot(details.shipping_address) may contain a different methodCode;
update the guard to parse the snapshot first (parseSnapshot), compare
parsedSnapshot.methodCode (or equivalent property) to
details.shipping_method_code, and if they differ throw a failure (e.g.,
SHIPPING_METHOD_MISMATCH) before calling toNpPayload(order: details, snapshot:
parsedSnapshot) so the NP payload is built only from consistent data.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
frontend/.env.examplefrontend/app/[locale]/admin/shop/orders/[id]/page.tsxfrontend/app/[locale]/shop/cart/CartPageClient.tsxfrontend/app/[locale]/shop/orders/[id]/page.tsxfrontend/app/api/shop/webhooks/stripe/route.tsfrontend/db/schema/shop.tsfrontend/drizzle/0019_p2_shop_invariants.sqlfrontend/drizzle/meta/0019_snapshot.jsonfrontend/drizzle/meta/_journal.jsonfrontend/lib/services/orders/_shared.tsfrontend/lib/services/orders/monobank-janitor.tsfrontend/lib/services/orders/monobank-webhook.tsfrontend/lib/services/shop/shipping/checkout-payload.tsfrontend/lib/services/shop/shipping/nova-poshta-catalog.tsfrontend/lib/services/shop/shipping/nova-poshta-client.tsfrontend/lib/services/shop/shipping/shipments-worker.tsfrontend/messages/en.jsonfrontend/messages/pl.jsonfrontend/messages/uk.json
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/db/schema/shop.ts
- frontend/lib/services/shop/shipping/checkout-payload.ts
frontend/.env.example
Outdated
| NP_API_KEY= | ||
| # Optional if code has a default; set explicitly in production for clarity | ||
| NP_API_BASE= |
There was a problem hiding this comment.
Resolve dotenv key ordering warning in the NP section.
dotenv-linter warns that NP_API_BASE should be declared before NP_API_KEY (Line 84 vs Line 82). Reordering will clear the warning.
🧰 Tools
🪛 dotenv-linter (4.0.0)
[warning] 84-84: [UnorderedKey] The NP_API_BASE key should go before the NP_API_KEY key
(UnorderedKey)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/.env.example` around lines 82 - 84, The dotenv-linter warning is due
to ordering of the NP keys; move the NP_API_BASE declaration so it appears
before NP_API_KEY in frontend/.env.example (ensure the comment about
optional/defaults remains with NP_API_BASE if needed) so the two entries read
NP_API_BASE then NP_API_KEY; no other changes to values or comments are
required.
| const available = data?.available === true; | ||
| const reasonCode = (data?.reasonCode ?? | ||
| null) as ShippingAvailabilityReasonCode | null; | ||
| const methods = Array.isArray(data?.methods) | ||
| ? (data.methods as ShippingMethod[]) | ||
| : []; | ||
|
|
There was a problem hiding this comment.
Fail closed on invalid /shipping/methods success payloads.
If the endpoint returns HTTP 200 but malformed/partial JSON, available becomes false and reasonCode stays null. That leaves shippingUnavailableHardBlock false and can re-enable checkout without a valid shipping decision path.
🛠️ Suggested fix
- const available = data?.available === true;
- const reasonCode = (data?.reasonCode ??
- null) as ShippingAvailabilityReasonCode | null;
- const methods = Array.isArray(data?.methods)
- ? (data.methods as ShippingMethod[])
- : [];
+ if (!data || typeof data !== 'object' || typeof data.available !== 'boolean') {
+ if (!cancelled) {
+ setShippingAvailable(false);
+ setShippingReasonCode('INTERNAL_ERROR');
+ setShippingMethods([]);
+ }
+ return;
+ }
+
+ const available = data.available;
+ const reasonCode =
+ typeof data.reasonCode === 'string'
+ ? (data.reasonCode as ShippingAvailabilityReasonCode)
+ : null;
+ const methods = Array.isArray(data.methods)
+ ? (data.methods as ShippingMethod[])
+ : [];
+
+ if (!available && !reasonCode) {
+ if (!cancelled) {
+ setShippingAvailable(false);
+ setShippingReasonCode('INTERNAL_ERROR');
+ setShippingMethods([]);
+ }
+ return;
+ }Also applies to: 803-808
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/app/`[locale]/shop/cart/CartPageClient.tsx around lines 293 - 299,
The current parsing of the /shipping/methods response may silently treat
malformed HTTP 200 payloads as available=false but leave
shippingUnavailableHardBlock false; update the validation in CartPageClient
(variables available, reasonCode, methods) to fail closed: explicitly verify
types for data.available (boolean), data.reasonCode (matches
ShippingAvailabilityReasonCode) and data.methods (Array of ShippingMethod), and
if any check fails set available = false and set a hard block flag
(shippingUnavailableHardBlock = true) and populate reasonCode to a safe failure
code (or non-null sentinel) so checkout cannot be re-enabled on malformed
success responses; locate and modify the parsing logic around available,
reasonCode and methods to add these guards.
| const shouldAttemptEnqueue = | ||
| args.paymentBecamePaidInThisApply && | ||
| args.shippingRequired === true && | ||
| args.shippingProvider === 'nova_poshta' && | ||
| Boolean(args.shippingMethodCode) && | ||
| isInventoryCommittedForShipping(args.inventoryStatus); | ||
|
|
There was a problem hiding this comment.
Derive enqueue eligibility from DB state, not pre-read fields.
eligible_for_enqueue is hard-gated by an app-computed boolean built from pre-read order fields. If those fields become eligible between read and SQL execution, enqueue is skipped even though updated_order reflects eligible state. This can leave a paid order without a queued shipment.
Suggested fix
- const shouldAttemptEnqueue =
- args.paymentBecamePaidInThisApply &&
- args.shippingRequired === true &&
- args.shippingProvider === 'nova_poshta' &&
- Boolean(args.shippingMethodCode) &&
- isInventoryCommittedForShipping(args.inventoryStatus);
+ const shouldAttemptEnqueue = args.paymentBecamePaidInThisApply;
@@
eligible_for_enqueue as (
select id
from updated_order
where ${shouldAttemptEnqueue}
and payment_status = 'paid'
and shipping_required = true
and shipping_provider = 'nova_poshta'
and shipping_method_code is not null
and ${inventoryCommittedForShippingSql(
sql`updated_order.inventory_status`
)}Also applies to: 454-465
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/app/api/shop/webhooks/stripe/route.ts` around lines 421 - 427, The
enqueue eligibility is currently decided using pre-read booleans (e.g.,
shouldAttemptEnqueue / args.paymentBecamePaidInThisApply and
args.shippingRequired/shippingProvider/shippingMethodCode) which can be stale;
instead compute eligible_for_enqueue from the DB-returned updated_order state
(e.g., use updated_order.payment_status / updated_order.shipping_required /
updated_order.shipping_provider / updated_order.shipping_method_code and call
isInventoryCommittedForShipping(updated_order.inventory_status)) at the point
just before generating the SQL/queue action so the decision reflects the
persisted state; apply the same change to the second occurrence referenced
around the block at 454-465 so both enqueue checks derive eligibility from
updated_order rather than args.
| await markSucceeded({ | ||
| shipmentId: args.claim.id, | ||
| runId: args.runId, | ||
| providerRef: created.providerRef, | ||
| trackingNumber: created.trackingNumber, | ||
| }); | ||
|
|
||
| recordShippingMetric({ | ||
| name: 'succeeded', | ||
| source: 'shipments_worker', | ||
| runId: args.runId, | ||
| orderId: args.claim.order_id, | ||
| shipmentId: args.claim.id, | ||
| }); | ||
|
|
||
| return 'succeeded'; | ||
| } catch (error) { |
There was a problem hiding this comment.
Handle lease-loss CAS misses before reporting success.
markSucceeded() can return null when the lease was lost (lease_owner mismatch), but the code still records "succeeded" and returns success. That hides contention/race outcomes and can misreport run stats.
🐛 Proposed fix
- await markSucceeded({
+ const marked = await markSucceeded({
shipmentId: args.claim.id,
runId: args.runId,
providerRef: created.providerRef,
trackingNumber: created.trackingNumber,
});
+
+ if (!marked) {
+ logWarn('shipping_shipments_worker_lease_lost', {
+ runId: args.runId,
+ shipmentId: args.claim.id,
+ orderId: args.claim.order_id,
+ code: 'SHIPMENT_LEASE_LOST',
+ });
+ return 'retried';
+ }
recordShippingMetric({
name: 'succeeded',📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| await markSucceeded({ | |
| shipmentId: args.claim.id, | |
| runId: args.runId, | |
| providerRef: created.providerRef, | |
| trackingNumber: created.trackingNumber, | |
| }); | |
| recordShippingMetric({ | |
| name: 'succeeded', | |
| source: 'shipments_worker', | |
| runId: args.runId, | |
| orderId: args.claim.order_id, | |
| shipmentId: args.claim.id, | |
| }); | |
| return 'succeeded'; | |
| } catch (error) { | |
| const marked = await markSucceeded({ | |
| shipmentId: args.claim.id, | |
| runId: args.runId, | |
| providerRef: created.providerRef, | |
| trackingNumber: created.trackingNumber, | |
| }); | |
| if (!marked) { | |
| logWarn('shipping_shipments_worker_lease_lost', { | |
| runId: args.runId, | |
| shipmentId: args.claim.id, | |
| orderId: args.claim.order_id, | |
| code: 'SHIPMENT_LEASE_LOST', | |
| }); | |
| return 'retried'; | |
| } | |
| recordShippingMetric({ | |
| name: 'succeeded', | |
| source: 'shipments_worker', | |
| runId: args.runId, | |
| orderId: args.claim.order_id, | |
| shipmentId: args.claim.id, | |
| }); | |
| return 'succeeded'; | |
| } catch (error) { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/lib/services/shop/shipping/shipments-worker.ts` around lines 534 -
550, markSucceeded() can return null when the lease was lost; update the try
block to check the result of markSucceeded (the call to markSucceeded with
shipmentId: args.claim.id, runId: args.runId, providerRef: created.providerRef,
trackingNumber: created.trackingNumber) and if it returns null treat this as a
lease-loss outcome: do not call recordShippingMetric(...) with name:'succeeded'
nor return 'succeeded'—instead record/return an appropriate lease-loss status
(or throw) so the run is not misreported; place this conditional immediately
after the await markSucceeded(...) and before the recordShippingMetric and
return statements.
… cart fail-closed
* (SP: 3) [Backend] add Nova Poshta shipping foundation + checkout persistence + async label workflow (#364) * (SP: 2) [Frontend] Reduce Vercel variable costs via caching and analytics cleanup (#367) * perf(vercel): cut runtime costs via notification, blog cache, and analytics changes * perf(blog): remove server searchParams usage to preserve ISR * fix(build): align Netlify Node version and remove SpeedInsights import * chore(release): bump version to 1.0.4 --------- Co-authored-by: Liudmyla Sovetovs <milkaegik@gmail.com>
2 participants
Description
This PR delivers a production-grade, end-to-end shipping foundation for the Shop module (Nova Poshta, warehouse/locker delivery). It introduces DB-backed shipping state, Nova Poshta catalog caching, checkout shipping persistence, payment → shipment enqueue correctness, an internal worker to create shipment labels asynchronously, admin/UI shipping actions, and PII-safe sanitization + retention.
Related Issue
Issue: #<issue_number>
Changes
Phase 1 — Schema + migrations foundation
order_shipping,shipping_shipments,np_cities,np_warehouses, shipping enums, and nullable shipping fields onorders.NOT VALID), not in Drizzlecheck()declarations.drizzle-kitoutput.Phase 2 — Public shipping API + NP catalog cache
Phase 3 — Checkout contract + shipping persistence
ordersshipping fields +order_shipping.shipping_address(PII snapshot).Phase 4 — Payments correctness: enqueue shipment when PAID applied
Phase 5 — Worker: create label (TTN) async
Phase 6 — UI + Admin shipping actions
Phase 7 — Sanitizer + retention
order_shipping.shipping_addressafter the retention window for delivered/cancelled orders.Database Changes (if applicable)
How Has This Been Tested?
Notes:
DATABASE_URL_LOCAL, sanityselect 1).drizzle.__drizzle_migrations; shipping constraints created asNOT VALIDper policy; no manual edits to Drizzle meta beyonddrizzle-kitoutput.available:false; feature ON returns NP methods; local-first catalog serves cached items even when NP is down; cache-miss + NP down returns degradedNP_UNAVAILABLEwithno-store.orders+ PII snapshot inorder_shipping; idempotency excludes PII and includes NP refs.shipping_shipmentsonce; repeated webhook does not duplicate shipments.Relevant test runs included:
lib/tests/shop/shipping-methods-route-p2.test.tslib/tests/shop/shipping-np-cities-route-p2.test.tslib/tests/shop/shipping-np-warehouses-route-p2.test.tslib/tests/shop/shipping-internal-np-sync-route-p2.test.tslib/tests/shop/nova-poshta-client-network-failure.test.tslib/tests/shop/checkout-shipping-phase3.test.tslib/tests/shop/checkout-currency-policy.test.tslib/tests/shop/stripe-webhook-psp-fields.test.tslib/tests/shop/monobank-webhook-apply.test.tslib/tests/shop/shipping-shipments-worker-phase5.test.tslib/tests/shop/admin-csrf-contract.test.tslib/tests/shop/admin-api-killswitch.test.tslib/tests/shop/shipping-checkout-payload-phase6.test.tslib/tests/shop/shipping-log-sanitizer-phase7.test.tslib/tests/shop/shipping-retention-phase7.test.tslib/tests/shop/shipping-internal-retention-route-phase7.test.tsScreenshots (if applicable)
Checklist
Before submitting
Reviewers
Summary by CodeRabbit
New Features
Updates
Tests