fix(api): preserve orderForm sales channel in validateCart - #3435
Conversation
Avoid wiping cart items after Quick Order by omitting stale session `sc` on the initial orderForm GET and adopting the orderForm SC when the cart etag is stale. Also defer client validateCart until the first validateSession completes.
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe PR coordinates session validation with cart validation and adds VTEX sales-channel synchronization. Existing carts can preserve their sales channel, stale carts can adopt order-form channels, and returned carts can include the adopted channel. ChangesVTEX cart validation
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant SessionValidation
participant CartValidation
participant VTEXOrderForm
participant SalesChannelState
SessionValidation->>CartValidation: complete session validation
CartValidation->>VTEXOrderForm: fetch order form with channel preservation
VTEXOrderForm-->>CartValidation: return order form and sales channel
CartValidation->>SalesChannelState: evaluate channel divergence
SalesChannelState-->>CartValidation: return adopted channel
CartValidation->>VTEXOrderForm: update cart using adopted channel
CartValidation-->>SessionValidation: silently synchronize session channel
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/api/src/platforms/vtex/resolvers/validateCart.ts (1)
381-448: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd resolver-level integration coverage for both sales-channel branches.
Add
*.int.test.tscases that invokevalidateCartand assert the initial Checkout URL, conditional refetch URL, and returned cart state for stale external carts and non-stale session-channel changes. The existingsalesChannelStaleFetchfixture only covers the catalog lookup.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/api/src/platforms/vtex/resolvers/validateCart.ts` around lines 381 - 448, Add resolver-level integration tests invoking validateCart for both stale external carts and non-stale session-channel changes. Assert the initial Checkout request URL, whether the conditional refetch occurs with the expected sales channel, and the returned cart state; do not rely solely on the salesChannelStaleFetch catalog fixture.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/api/test/unit/platforms/vtex/clients/commerce.test.ts`:
- Around line 75-128: Rename the unit test file from commerce.test.ts to
index.test.ts and place it under the commerce directory so it matches the tested
source module commerce/index.ts and the required test-file naming convention.
Preserve the existing orderForm test contents unchanged.
In `@packages/api/test/unit/platforms/vtex/utils/cartSalesChannel.test.ts`:
- Line 45: Update the test around the JSON.parse call to explicitly throw when
result is null, allowing TypeScript to narrow it to a string before parsing;
remove the result as string assertion while preserving the existing object
match.
---
Nitpick comments:
In `@packages/api/src/platforms/vtex/resolvers/validateCart.ts`:
- Around line 381-448: Add resolver-level integration tests invoking
validateCart for both stale external carts and non-stale session-channel
changes. Assert the initial Checkout request URL, whether the conditional
refetch occurs with the expected sales channel, and the returned cart state; do
not rely solely on the salesChannelStaleFetch catalog fixture.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 61dd66c4-c837-4178-b095-8bdcf9e8fbb5
📒 Files selected for processing (8)
packages/api/src/platforms/vtex/clients/commerce/index.tspackages/api/src/platforms/vtex/resolvers/validateCart.tspackages/api/src/platforms/vtex/utils/cartSalesChannel.tspackages/api/test/mocks/ValidateCartMutation.tspackages/api/test/unit/platforms/vtex/clients/commerce.test.tspackages/api/test/unit/platforms/vtex/utils/cartSalesChannel.test.tspackages/core/src/sdk/cart/index.tspackages/core/src/sdk/session/index.ts
Align the unit test path with the commerce/index.ts source module.
Extract SC adoption glue to reduce cognitive complexity, flatten nested template literals in checkout.orderForm, and narrow the cartSalesChannel test without a type assertion.
Extract the session-wait helper so unit tests can exercise immediate, subscribe, race, and timeout paths under the Quality Gate threshold.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/test/sdk/cart/waitForSessionValidated.test.ts`:
- Around line 31-43: Update both Store<boolean> mocks used by
waitForSessionValidated to include the required readInitial method, returning
the mock store’s initial boolean value consistently with read. Keep the existing
subscribe, unsubscribe, and set behavior unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d1bd220b-92d0-4189-84b6-dcda5026716f
📒 Files selected for processing (3)
packages/core/src/sdk/cart/index.tspackages/core/src/sdk/cart/waitForSessionValidated.tspackages/core/test/sdk/cart/waitForSessionValidated.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/core/src/sdk/cart/index.ts
@faststore/api
@faststore/cli
@faststore/components
@faststore/core
@faststore/diagnostics
@faststore/lighthouse
@faststore/sdk
@faststore/ui
commit: |
renatomaurovtex
left a comment
renatomaurovtex
left a comment
There was a problem hiding this comment.
Reviewed the full resolver flow on the head commit, plus the SDK store internals that the new client-side wait relies on. The design is sound: preserve the orderForm's SC on the first GET, let the orderForm win when the cart is externally stale, let the session win otherwise. Verified that the session↔cart import cycle is pre-existing (session/index.ts already imported cartStore) and the new hasValidatedSessionStore import is only dereferenced at call time, so module init order is safe. Also verified the 3s wait is effectively a safety net only: persisted always calls store.set() on IDB hydration and optimistic validates on every set, so validateSession fires on every page load and hasValidatedSessionStore flips quickly.
One scenario I couldn't convince myself is covered:
packages/api/src/platforms/vtex/resolvers/validateCart.ts
🟠 Persistent session↔orderForm SC divergence re-drops the items one request later. The stale-etag pass adopts the orderForm SC only into the request-scoped context (mutateChannelContext), then setOrderFormEtag makes the cart non-stale. If the client session never converges to the orderForm SC — validateSession derives it from Session Manager (store?.channel?.value ?? channel.salesChannel), so this happens whenever the external flow advanced the orderForm but not vtex_session — the next validateCart takes the non-stale path, shouldRefetchOrderFormWithSessionSalesChannel sees session 1 ≠ orderForm 4, and re-fetches with sc=1: Checkout recalculates under the stale session SC and drops the SC-4-only items. That's the original bug, deferred by one request.
Two asks:
- Confirm the Quick Order flow propagates its SC into Session Manager (
store.channel) — if yes, this is a non-issue for SO-641 and the divergence refetch only fires for genuine in-session switches. - Either way, add a repro step to "How to test it": reload twice after returning from Quick Order. The current steps only prove the first validation.
If Session Manager does not converge, consider gating the refetch on an SC change observed in-session (e.g. channel actually mutated since the last validation) instead of any divergence.
🟡 No resolver-level test for the new branches. The pure helpers are well covered, but nothing asserts that validateCart actually issues the second GET with sc on divergence, or that the adopted channel reaches mutateChannelContext on the stale path. A mutations.test.ts case with session SC ≠ orderForm SC (non-stale) asserting the second fetch URL would lock the behavior against refactors.
💬 adoptOrderFormSalesChannelIfNeeded(ctx, orderForm, isStale) is only called inside if (isStale), so the third parameter is always true — the helper's re-check is dead in practice. Fine to keep for the pure-function test surface, but the call site could drop the flag.
packages/core/src/sdk/cart/waitForSessionValidated.ts
The read → subscribe → re-read race handling is correct, and resolving on timeout (rather than rejecting) is the right degradation. Nice test coverage, including the between-read-and-subscribe race.
Verdict: Approved with comments
Blocking (🔴/🟠):
- 🟠 Answer the two-request divergence question above (and add the double-reload repro step) before merge — if Session Manager doesn't carry the external SC, this reintroduces SO-641 one request later.
Non-blocking (🟡/💬):
- 🟡 Resolver-level integration test for the divergence refetch / SC adoption.
- 💬 Redundant
isStaleparam at the adoption call site.
Checks to confirm before merge: pnpm turbo run test --filter=@faststore/api --filter=@faststore/core · build · pnpm size (core/sdk touched)
|
Reviewed the full 1. The new core test file doesn't type-checkBoth hand-rolled Vitest passes because it strips types instead of checking them, and CI stays green because no workflow runs 2. The session likely won't converge to the orderForm SC, so a second
|
Stop refetching Checkout with a lagging session SC on later validations, which wiped Quick Order items one request after the first preserve/adopt. Expose optional StoreOrder.salesChannel when SC was adopted from a stale cart.
Keep Checkout on the orderForm trade policy when session SC lags, return the adopted SC from validateCart, and silently align fs::session so later requests use the correct channel without revalidating the cart mid-flight.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/api/test/integration/mutations.test.ts`:
- Around line 194-298: Move the two validateCart integration tests around the
divergent and stale sales-channel scenarios from mutations.test.ts into a new
validateCart.int.test.ts file. Preserve their setup, mocks, assertions, and
required imports so pnpm test:int discovers them, while keeping
mutations.test.ts for unit tests only.
- Around line 169-180: Update withOrderFormSalesChannel to return the widened
object shape produced after replacing result, rather than claiming the original
generic subtype T; preserve the generic input constraint and merged fields.
Rename the test file from mutations.test.ts to mutations.int.test.ts to match
the integration-test naming convention.
In `@packages/core/src/sdk/session/syncSalesChannelFromOrderForm.ts`:
- Around line 23-29: Update the channel parsing in syncSalesChannelFromOrderForm
so parsed values are accepted only when they are non-null objects; otherwise
reset channel to an empty record before the salesChannel comparison. Add
coverage for valid non-object JSON, including null, and preserve the existing
comparison behavior for valid channel objects.
In `@packages/core/test/sdk/session/syncSalesChannelFromOrderForm.test.ts`:
- Around line 5-16: Update the baseSession fixture to return a complete Session
object by supplying every required Session field, then remove the `as Session`
type assertion. Keep the existing salesChannel, currency, locale, country,
channel, and person values unchanged while relying on TypeScript to validate the
fixture and expose future Session contract changes.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 478e3803-6bde-4b4f-90cf-6afacb846511
⛔ Files ignored due to path filters (4)
packages/api/src/__generated__/schema.tsis excluded by!**/__generated__/**and included bypackages/**packages/api/test/integration/__snapshots__/mutations.test.ts.snapis excluded by!**/*.snapand included bypackages/**packages/core/@generated/gql.tsis excluded by!**/@generated/**,!**/@generated/**and included bypackages/**packages/core/@generated/graphql.tsis excluded by!**/@generated/**,!**/@generated/**and included bypackages/**
📒 Files selected for processing (14)
packages/api/src/platforms/vtex/clients/commerce/index.tspackages/api/src/platforms/vtex/resolvers/validateCart.tspackages/api/src/platforms/vtex/resolvers/validateSession.tspackages/api/src/platforms/vtex/typeDefs/order.graphqlpackages/api/src/platforms/vtex/utils/cartSalesChannel.tspackages/api/test/integration/mutations.test.tspackages/api/test/mocks/ValidateCartMutation.tspackages/api/test/unit/platforms/vtex/utils/cartSalesChannel.test.tspackages/core/src/sdk/cart/index.tspackages/core/src/sdk/cart/waitForSessionValidated.tspackages/core/src/sdk/session/index.tspackages/core/src/sdk/session/syncSalesChannelFromOrderForm.tspackages/core/test/sdk/cart/waitForSessionValidated.test.tspackages/core/test/sdk/session/syncSalesChannelFromOrderForm.test.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/api/test/mocks/ValidateCartMutation.ts
- packages/api/src/platforms/vtex/clients/commerce/index.ts
- packages/core/test/sdk/cart/waitForSessionValidated.test.ts
- packages/core/src/sdk/cart/waitForSessionValidated.ts
- packages/core/src/sdk/session/index.ts
Lower validateCart/validateSession cognitive complexity, guard non-object channel JSON before syncing adopted SC, and add coverage for the new paths.
Extract session helpers into a dedicated util and add unit tests so the validateSession refactor no longer tanks Coverage on New Code.
|
0 New Issues
0 Fixed Issues
0 Accepted Issues
3 participants
What's the purpose of this pull request?
After returning from Quick Order (or similar external checkout flows),
validateCartwas sending a stale sessionscon the orderForm GET. Checkout recalculated the cart under the wrong trade policy and dropped items only available in the orderForm's sales channel (e.g. SC 4). Tracked in SO-641 /#team-faststore.How it works?
API (
validateCart)preserveSalesChannel(omits?sc=) so Checkout keeps the cart's current sales channel.orderForm.salesChannelinto the GraphQL channel context.Core
validateCartuntil the firstvalidateSessioncompletes (3s timeout), reducing the race where cart validates with a stalesession.channel.Before / After
How to test it?
Preview URL (sandbox): https://sfj-119ced8--storeframework.preview.vtex.app
QA store PR: https://github.com/vtex-sites/faststoreqa.store/pull/907
cd packages/api && pnpm vitest run test/unit/platforms/vtex/utils/cartSalesChannel.test.ts test/unit/platforms/vtex/clients/commerce/index.test.tscd packages/core && pnpm vitest run test/sdk/cart/waitForSessionValidated.test.tscd packages/api && pnpm vitest run test/integration/mutations.test.tsvalidateCartmust not wipe them.Starters Deploy Preview
References
Checklist
bug)pnpm-lock.yamlchangesMade with Cursor
Summary by CodeRabbit
Bug Fixes
Tests