(SP: 3) [SHOP] harden shipping lifecycle recovery and production-safe provider/logging guards#415
Merged
ViktorSvertoka merged 12 commits intodevelopfrom Mar 25, 2026
Merged
Conversation
liudmylasovetovs
requested review from
AM1007,
LesiaUKR and
ViktorSvertoka
as code owners
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Contributor
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds canonical fulfillment-stage derivation and surfaces it across order APIs, pages, and summaries; introduces provider runtime validations (Stripe, Monobank, Nova Poshta); adds logging redaction utilities; implements transactional ensureQueuedInitialShipment and a recover_initial_shipment admin action; and updates tests, env docs, and translations. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API as Shop API
participant DB
participant Service as Fulfillment Service
Client->>API: GET /api/shop/orders/{id}
API->>DB: SELECT order + orders.status + shipping_status + latestShipmentStatusSql(...) + latestReturnStatusSql(...)
DB-->>API: row with orderStatus, shippingStatus, shipmentStatus, returnStatus
API->>Service: deriveCanonicalFulfillmentStage({orderStatus, shippingStatus, shipmentStatus, returnStatus})
Service-->>API: CanonicalFulfillmentStage
API-->>Client: 200 { order: {..., fulfillmentStage } }
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate 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: 4a5dfce41a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
frontend/lib/validation/shop.ts (1)
40-47: Make the canonical fulfillment stages a single source of truth.These literals are now encoded separately here and in
@/lib/services/shop/fulfillment-stage. Exporting a sharedcanonicalFulfillmentStageValuesconstant and deriving both the Zod enum and TS union from it would remove an easy drift point.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/validation/shop.ts` around lines 40 - 47, Introduce and export a single array constant (e.g., canonicalFulfillmentStageValues) containing the literal stages, then derive the Zod enum and a TypeScript union from that array instead of hardcoding literals: replace the current fulfillmentStageSchema = z.enum([...]) with fulfillmentStageSchema = z.enum(canonicalFulfillmentStageValues) and export a type like FulfillmentStage = (typeof canonicalFulfillmentStageValues)[number]; update the other module that currently duplicates the literals (the one exporting fulfillment-stage values) to import and reuse canonicalFulfillmentStageValues so both the Zod schema (fulfillmentStageSchema) and the service layer share the same single source of truth.frontend/lib/services/orders/summary.ts (1)
442-449: Intentional simplification:getOrderByIdempotencyKeyomits shipment/return signals.This function only passes
orderStatusandshippingStatustoderiveCanonicalFulfillmentStage, omittingshipmentStatusandreturnStatus. Per the derivation logic, whenshipmentStatusandreturnStatusare both null, the function falls through and returns'processing'as the default.This is likely intentional since this function is used during checkout flows before shipments exist. However, if this function could be called for orders that already have shipments/returns, the derived stage would be inaccurate.
Consider adding a brief comment documenting this intentional simplification for future maintainers.
📝 Suggested documentation
export async function getOrderByIdempotencyKey( dbClient: DbClient, key: string ): Promise<OrderSummaryWithMinor | null> { + // Note: This function is primarily used during checkout flows where shipments + // don't exist yet, so we only derive stage from order/shipping status. const [order] = await dbClient🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/services/orders/summary.ts` around lines 442 - 449, Add a brief inline comment at the call site where parseOrderSummary invokes deriveCanonicalFulfillmentStage (the call passing only order.status and order.shippingStatus) explaining that shipmentStatus and returnStatus are intentionally omitted in this simplified path (used by getOrderByIdempotencyKey/checkout flows before shipments/returns exist), and note the caveat that if this function is ever reused for orders with existing shipments/returns the derived stage may be inaccurate and those fields should be included; reference deriveCanonicalFulfillmentStage and getOrderByIdempotencyKey in the comment for clarity.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/lib/env/monobank.ts`:
- Around line 168-179: resolveShopPaymentProvider() currently calls
isMonobankEnabled() without protection, so assertions from
assertMonobankRuntimeConfig() can throw ShopProviderConfigError and crash; wrap
the call to isMonobankEnabled() inside a try-catch in
resolveShopPaymentProvider(), catch ShopProviderConfigError (or any Error) and
treat it as "disabled" (return false / fallthrough to other providers) so
invalid Monobank config no longer causes an unhandled exception; ensure the
catch is narrow enough to recognize configuration validation errors but still
safe (refer to resolveShopPaymentProvider(), isMonobankEnabled(), and
resolveMonobankCheckoutEnabled() to mirror existing error-handling patterns).
In `@frontend/lib/env/stripe.ts`:
- Around line 71-77: The current assertProductionLikeProviderString call for
provider 'stripe' only checks format (requiredPrefix 'whsec_') and can allow
test webhook secrets in production; update the validation so that when running
in production (e.g. process.env.NODE_ENV === 'production' or your existing
production flag) you additionally verify the Stripe API key indicates a live
account (check process.env.STRIPE_API_KEY.startsWith('sk_live_') or an
equivalent "is live" indicator) and throw/log a clear error if a test API key is
present while STRIPE_WEBHOOK_SECRET is set, keeping the existing minLength and
prefix checks and updating the error message to reference STRIPE_WEBHOOK_SECRET
and STRIPE_API_KEY; the check should be implemented alongside or inside
assertProductionLikeProviderString usage for 'stripe' (refer to provider:
'stripe', envName: 'STRIPE_WEBHOOK_SECRET', value: webhookSecret).
In `@frontend/lib/logging.ts`:
- Around line 53-55: The top-level log message is not being redacted: update the
redactJsonStringify function to actually perform redaction on strings and
objects (e.g., detect and mask emails, tokens, phone numbers, and sensitive
keys) rather than just returning JSON.stringify(value), and ensure the message
field passed to the logger is passed through this redactor as well; specifically
modify redactJsonStringify and the places that emit msg (the same call sites
around the existing redactJsonStringify usage at the msg/meta/err emission and
the block referenced at lines 65-70) so msg is sanitized consistently with meta
and err before logging.
In `@frontend/lib/services/orders/payment-intent.ts`:
- Around line 58-65: The updated branch is deriving fulfillmentStage from orders
alone which misses shipment/return-only signals; before calling
deriveCanonicalFulfillmentStage in both the current and updated branches, call
readCanonicalFulfillmentSignals(...) to load signals from
shipping_shipments/return_requests (mirror the getOrderById path), then pass the
returned signals into deriveCanonicalFulfillmentStage; also add
readCanonicalFulfillmentSignals to the import list from
../shop/fulfillment-stage so the function is available where parseOrderSummary
and deriveCanonicalFulfillmentStage are used.
In `@frontend/lib/services/shop/shipping/admin-actions.ts`:
- Around line 505-539: The branch that handles state.shipment_status ===
'queued' currently returns early after appendAuditEntry and thus never calls
ensureQueuedInitialShipment to repair drift; remove the early return and invoke
ensureQueuedInitialShipment (passing the orderId and state.shipment_id /
relevant args) so the order shipping_status can be resynchronized from the
existing queued shipment row, then use the result of ensureQueuedInitialShipment
to determine the returned shippingStatus/shipmentStatus/changed values (preserve
the audit append via appendAuditEntry and keep dedupe logic using
buildShippingAdminAuditDedupe).
In `@frontend/lib/services/shop/shipping/ensure-queued-initial-shipment.ts`:
- Around line 76-80: The conflict handler is requeuing a
concurrently-created/advanced shipment by running "on conflict (order_id) do
update set status = 'queued'...", which can regress live shipments; in the
ensureQueuedInitialShipment helper replace that conflict update with a no-op
conflict policy (e.g., "on conflict (order_id) do nothing") so concurrent
inserts are ignored rather than resetting status, and remove the conditional
update that forces status back to 'queued' in that statement; keep the rest of
the insert/transition logic intact and rely on queued_order_ids for idempotency.
---
Nitpick comments:
In `@frontend/lib/services/orders/summary.ts`:
- Around line 442-449: Add a brief inline comment at the call site where
parseOrderSummary invokes deriveCanonicalFulfillmentStage (the call passing only
order.status and order.shippingStatus) explaining that shipmentStatus and
returnStatus are intentionally omitted in this simplified path (used by
getOrderByIdempotencyKey/checkout flows before shipments/returns exist), and
note the caveat that if this function is ever reused for orders with existing
shipments/returns the derived stage may be inaccurate and those fields should be
included; reference deriveCanonicalFulfillmentStage and getOrderByIdempotencyKey
in the comment for clarity.
In `@frontend/lib/validation/shop.ts`:
- Around line 40-47: Introduce and export a single array constant (e.g.,
canonicalFulfillmentStageValues) containing the literal stages, then derive the
Zod enum and a TypeScript union from that array instead of hardcoding literals:
replace the current fulfillmentStageSchema = z.enum([...]) with
fulfillmentStageSchema = z.enum(canonicalFulfillmentStageValues) and export a
type like FulfillmentStage = (typeof canonicalFulfillmentStageValues)[number];
update the other module that currently duplicates the literals (the one
exporting fulfillment-stage values) to import and reuse
canonicalFulfillmentStageValues so both the Zod schema (fulfillmentStageSchema)
and the service layer share the same single source of truth.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3718592f-af72-4f86-8297-6eef6455e392
📒 Files selected for processing (35)
frontend/app/[locale]/admin/shop/orders/[id]/page.tsxfrontend/app/[locale]/shop/orders/[id]/page.tsxfrontend/app/api/shop/admin/orders/[id]/shipping/route.tsfrontend/app/api/shop/orders/[id]/route.tsfrontend/app/api/shop/shipping/methods/route.tsfrontend/lib/env/monobank.tsfrontend/lib/env/nova-poshta.tsfrontend/lib/env/provider-runtime.tsfrontend/lib/env/stripe.tsfrontend/lib/logging.tsfrontend/lib/services/orders/checkout.tsfrontend/lib/services/orders/monobank-webhook.tsfrontend/lib/services/orders/payment-intent.tsfrontend/lib/services/orders/summary.tsfrontend/lib/services/shop/fulfillment-stage.tsfrontend/lib/services/shop/logging-redaction.tsfrontend/lib/services/shop/shipping/admin-actions.tsfrontend/lib/services/shop/shipping/ensure-queued-initial-shipment.tsfrontend/lib/tests/shop/admin-shipping-canonical-audit.test.tsfrontend/lib/tests/shop/admin-shipping-payment-gate.test.tsfrontend/lib/tests/shop/admin-shipping-state-sync.test.tsfrontend/lib/tests/shop/checkout-shipping-authoritative-total.test.tsfrontend/lib/tests/shop/fulfillment-stage.test.tsfrontend/lib/tests/shop/logging-redaction-generic.test.tsfrontend/lib/tests/shop/logging-redaction-real-flows.test.tsfrontend/lib/tests/shop/order-fulfillment-stage-surfaces.test.tsfrontend/lib/tests/shop/order-status-token.test.tsfrontend/lib/tests/shop/orders-access.test.tsfrontend/lib/tests/shop/orders-status-ownership.test.tsfrontend/lib/tests/shop/provider-env-runtime-safety.test.tsfrontend/lib/tests/shop/shipping-methods-route-p2.test.tsfrontend/lib/validation/shop.tsfrontend/messages/en.jsonfrontend/messages/pl.jsonfrontend/messages/uk.json
frontend/lib/services/shop/shipping/ensure-queued-initial-shipment.ts
Outdated
Show resolved
Hide resolved
…lment stage consistency
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/lib/tests/shop/provider-env-runtime-safety.test.ts`:
- Around line 59-72: Swap the order of calling vi.unstubAllEnvs() and the manual
environment operations in the test hooks: in beforeEach call vi.unstubAllEnvs()
first, then delete process.env keys and resetEnvCache(), and in afterEach call
vi.unstubAllEnvs() before restoreEnv() and resetEnvCache(); update the
beforeEach and afterEach blocks that reference ENV_KEYS, previousEnv,
restoreEnv(), resetEnvCache(), and vi.unstubAllEnvs() accordingly so stubs are
cleared prior to manual env modifications/restoration.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 68d4e640-c318-4819-a6d2-6858851e735c
📒 Files selected for processing (14)
frontend/lib/env/payments.tsfrontend/lib/logging.tsfrontend/lib/services/orders/payment-intent.tsfrontend/lib/services/orders/summary.tsfrontend/lib/services/shop/fulfillment-stage.tsfrontend/lib/services/shop/shipping/admin-actions.tsfrontend/lib/services/shop/shipping/ensure-queued-initial-shipment.tsfrontend/lib/tests/shop/admin-shipping-canonical-audit.test.tsfrontend/lib/tests/shop/admin-shipping-state-sync.test.tsfrontend/lib/tests/shop/fulfillment-stage.test.tsfrontend/lib/tests/shop/logging-redaction-generic.test.tsfrontend/lib/tests/shop/payment-intent-fulfillment-stage.test.tsfrontend/lib/tests/shop/provider-env-runtime-safety.test.tsfrontend/lib/validation/shop.ts
✅ Files skipped from review due to trivial changes (2)
- frontend/lib/services/orders/summary.ts
- frontend/lib/tests/shop/fulfillment-stage.test.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- frontend/lib/services/orders/payment-intent.ts
- frontend/lib/validation/shop.ts
- frontend/lib/tests/shop/logging-redaction-generic.test.ts
- frontend/lib/tests/shop/admin-shipping-canonical-audit.test.ts
- frontend/lib/tests/shop/admin-shipping-state-sync.test.ts
- frontend/lib/services/shop/shipping/ensure-queued-initial-shipment.ts
- frontend/lib/logging.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
frontend/.env.example (3)
68-71: Clarify which payment toggle gates these Stripe requirements.The comment states "Required when Stripe payments are enabled" but doesn't specify which environment variable controls this. Based on the code snippet from
frontend/lib/env/stripe.ts:27,42-43, Stripe credentials are gated byPAYMENTS_ENABLED(line 91), notSTRIPE_PAYMENTS_ENABLED(line 66). Consider updating the comment to explicitly reference the controlling variable.📝 Proposed clarification
-# Required when Stripe payments are enabled. +# Required when PAYMENTS_ENABLED=true (see line 91). # In production-like runtime, invalid or placeholder config must fail closed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/.env.example` around lines 68 - 71, Update the env example comment to reference the actual toggle used in code: state that STRIPE_SECRET_KEY and STRIPE_WEBHOOK_SECRET are required when PAYMENTS_ENABLED is true (not STRIPE_PAYMENTS_ENABLED), so change the comment text near STRIPE_SECRET_KEY/STRIPE_WEBHOOK_SECRET to mention PAYMENTS_ENABLED as the controlling variable and ensure consistency with the logic in frontend/lib/env/stripe.ts which checks PAYMENTS_ENABLED.
80-83: Clarify which payment toggle gates these Monobank requirements.Similar to the Stripe section, the comment states "Required when Monobank checkout/webhooks are enabled" but doesn't specify which environment variable controls this. Based on the code snippet from
frontend/lib/env/monobank.ts:135, Monobank credentials are gated byPAYMENTS_ENABLED(line 91). Consider updating the comment to explicitly reference the controlling variable.📝 Proposed clarification
-# Required when Monobank checkout/webhooks are enabled. +# Required when PAYMENTS_ENABLED=true (see line 91). # In production-like runtime, invalid or placeholder config must fail closed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/.env.example` around lines 80 - 83, Update the env example comment to explicitly state which feature flag controls Monobank being required: mention PAYMENTS_ENABLED as the toggle that gates Monobank checkout/webhooks and therefore requires MONO_MERCHANT_TOKEN and MONO_PUBLIC_KEY; edit the existing comment above MONO_MERCHANT_TOKEN and MONO_PUBLIC_KEY to mirror the Stripe section style and reference PAYMENTS_ENABLED so it's clear when these variables must be non-placeholder.
77-78: Document the default fallback values forMONO_INVOICE_TIMEOUT_MS.The code snippet from
frontend/lib/env/monobank.ts:138-141shows that this variable has fallback defaults: 8000ms in production or 12000ms in development. Consider documenting these defaults to help operators understand the behavior when this variable is not set.📝 Proposed enhancement
-# Optional invoice timeout override in milliseconds. +# Optional invoice timeout override in milliseconds. +# Defaults: 8000ms (production), 12000ms (development). MONO_INVOICE_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 77 - 78, Update the .env.example comment for MONO_INVOICE_TIMEOUT_MS to document its runtime fallbacks: state that when unset the app uses 8000 (ms) in production and 12000 (ms) in development as implemented in the code that reads MONO_INVOICE_TIMEOUT_MS; mention the units (milliseconds) and that these are applied only when the env var is not provided so operators know the default behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@frontend/.env.example`:
- Around line 68-71: Update the env example comment to reference the actual
toggle used in code: state that STRIPE_SECRET_KEY and STRIPE_WEBHOOK_SECRET are
required when PAYMENTS_ENABLED is true (not STRIPE_PAYMENTS_ENABLED), so change
the comment text near STRIPE_SECRET_KEY/STRIPE_WEBHOOK_SECRET to mention
PAYMENTS_ENABLED as the controlling variable and ensure consistency with the
logic in frontend/lib/env/stripe.ts which checks PAYMENTS_ENABLED.
- Around line 80-83: Update the env example comment to explicitly state which
feature flag controls Monobank being required: mention PAYMENTS_ENABLED as the
toggle that gates Monobank checkout/webhooks and therefore requires
MONO_MERCHANT_TOKEN and MONO_PUBLIC_KEY; edit the existing comment above
MONO_MERCHANT_TOKEN and MONO_PUBLIC_KEY to mirror the Stripe section style and
reference PAYMENTS_ENABLED so it's clear when these variables must be
non-placeholder.
- Around line 77-78: Update the .env.example comment for MONO_INVOICE_TIMEOUT_MS
to document its runtime fallbacks: state that when unset the app uses 8000 (ms)
in production and 12000 (ms) in development as implemented in the code that
reads MONO_INVOICE_TIMEOUT_MS; mention the units (milliseconds) and that these
are applied only when the env var is not provided so operators know the default
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 854c04f0-0391-4b36-a8c5-6061edfd03cb
📒 Files selected for processing (2)
frontend/.env.examplefrontend/lib/services/shop/logging-redaction.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/lib/services/shop/logging-redaction.ts
ViktorSvertoka
approved these changes
Mar 25, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR combines two Shop launch-readiness workstreams:
The goal is to make shipping recovery operationally safe, expose one canonical fulfillment lifecycle across Shop order surfaces, and harden runtime/logging behavior for production-like environments.
Related Issue
Issue: #<issue_number>
Changes
fulfillmentStageacross relevant customer/admin order detail and status APIs while keeping existing raw/internal state fields intactDatabase Changes (if applicable)
How Has This Been Tested?
Additional notes:
fulfillmentStageScreenshots (if applicable)
N/A — backend / API / lifecycle / runtime safety / logging hardening only.
Checklist
Before submitting
Reviewers
Summary by CodeRabbit
New Features
Improvements