feat: enable CDN cache for all sales channels#1562
Merged
Conversation
Contributor
Tagging OptionsShould a new tag be published when this PR is merged?
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 2 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="vtex/utils/segment.ts">
<violation number="1" location="vtex/utils/segment.ts:89">
P1: Removing `channel` from `isCacheableSegment` also disables `VTEXSC` cookie persistence for `?sc` requests, which can break sales-channel continuity across page navigation.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRemoved middleware write of Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Middleware
participant SegmentUtil
participant CDN_Browser
Client->>Middleware: Incoming HTTP request
Middleware->>SegmentUtil: evaluate segment (isCacheableSegment, token, segmentFromRequest)
SegmentUtil-->>Middleware: { cacheable, token, segmentFromRequest.channel? }
alt cacheable == false
Middleware->>CDN_Browser: set Cache-Control: no-store, no-cache, must-revalidate
Middleware->>Middleware: mark PAGE_DIRTY_KEY
else cacheable == true
Middleware->>CDN_Browser: no-store header omitted (allow caching)
end
alt segmentFromRequest.channel exists
Middleware->>CDN_Browser: Set `VTEXSC` cookie
end
alt token differs from incoming cookie
Middleware->>CDN_Browser: Set `vtex_segment` cookie
end
Middleware->>Client: continue via ctx.next!()
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@vtex/utils/segment.ts`:
- Around line 74-94: The code uses isCacheableSegment(...) to decide both
cacheability and whether to write VTEXSC/vtex_segment in setSegmentBag(...),
causing sales-channel (?sc) to be omitted when channel no longer makes a segment
non-cacheable; split persistence from cacheability by adding a separate
predicate (e.g., shouldPersistSegment or shouldPersistChannel) and update
setSegmentBag(...) to write the VTEXSC/vtex_segment cookie when the new
persistence predicate indicates the sales-channel (channel/SC) must be stored,
while leaving isCacheableSegment(...) unchanged for cache-control decisions;
modify setSegmentBag(...) to call the new predicate for persistence and keep
existing cache logic using isCacheableSegment(...).
- Around line 74-94: isCacheableSegment currently can return true for
non-default channels when channelPrivacy is unknown, allowing a private sales
channel to be cached on first hit; update isCacheableSegment to treat any
non-default channel as uncacheable until privacy is resolved (i.e., if
payload.channel exists and payload.channel !== 'default' and
payload.channelPrivacy is null/undefined then return false), and ensure this
check applies in both the removeUTMFromCacheKey branch and the default branch so
setSegmentBag/buildSegmentFromRequest cannot cache partial segment payloads
before channelPrivacy is known.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 87f85f10-7300-4920-b070-98ad6005e342
📒 Files selected for processing (2)
vtex/middleware.tsvtex/utils/segment.ts
- Remove isDefautSalesChannel restriction from isCacheableSegment so non-default channels are cacheable (CDN varies by vtex_segment/VTEXSC) - Always persist VTEXSC cookie for ?sc=X requests regardless of cacheability, ensuring sales channel continuity across navigation - Remove unused PAGE_CACHE_ALLOWED_KEY import from vtex middleware Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
vtex/utils/segment.ts (1)
64-68:⚠️ Potential issue | 🟡 MinorStale docstring: code no longer delegates to
isAnonymous.The comment states "By default, uses isAnonymous" but the implementation now inlines the check and omits the
channelcondition. Update the comment to reflect the current behavior.📝 Suggested docstring update
/** * Checks if the segment is cacheable for CDN purposes. - * By default, uses isAnonymous (UTMs affect cacheability because prices - * can vary by utm_source). With removeUTMFromCacheKey, UTMs are ignored - * (opt-in for stores that don't vary prices by UTM). + * Caching is allowed when the segment has no campaigns, priceTables, or regionId, + * and channelPrivacy is not "private". With removeUTMFromCacheKey disabled (default), + * UTM parameters also block caching (since prices can vary by utm_source). */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vtex/utils/segment.ts` around lines 64 - 68, Update the stale docstring for the cacheability check in vtex/utils/segment.ts: remove the claim that the code "uses isAnonymous" and instead describe the current inlined behavior (the function now directly examines the visitor/session anonymity and no longer applies any channel condition), and keep the note about removeUTMFromCacheKey opting out of UTM-based variability; reference the existing symbol names isAnonymous and removeUTMFromCacheKey in the comment to make the change clear.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@vtex/utils/segment.ts`:
- Around line 64-68: Update the stale docstring for the cacheability check in
vtex/utils/segment.ts: remove the claim that the code "uses isAnonymous" and
instead describe the current inlined behavior (the function now directly
examines the visitor/session anonymity and no longer applies any channel
condition), and keep the note about removeUTMFromCacheKey opting out of
UTM-based variability; reference the existing symbol names isAnonymous and
removeUTMFromCacheKey in the comment to make the change clear.
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="vtex/utils/segment.ts">
<violation number="1" location="vtex/utils/segment.ts:275">
P2: Setting VTEXSC unconditionally will still attach Set-Cookie on otherwise cacheable responses, which can prevent CDN caching. Guard this cookie with `isCacheableSegment(ctx)` (or move it into the existing cacheability block) so cacheable responses stay cookie-free.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
…s unknown Non-default sales channels are only cacheable when channelPrivacy is explicitly "public". If channelPrivacy is null/undefined (e.g. cold request), skip CDN cache to avoid serving a private channel response on first hit.
Cloudflare does not cache responses with Set-Cookie headers. Guard both VTEXSC and vtex_segment cookies inside the !isCacheableSegment block so cacheable responses stay cookie-free and the CDN can cache them. Navigation continuity is preserved: the first cold request for a non-default channel is always non-cacheable (channelPrivacy unknown), so cookies are set there and carried by the browser on subsequent requests.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
vtex/utils/segment.ts (2)
287-307:⚠️ Potential issue | 🟠 MajorBase cookie persistence on the final
no-storedecision.
vtex/middleware.tsstill marks authenticated requests asno-storewithisCacheableSegment(ctx) && !isLoggedIn. This branch only checks the segment predicate, so a logged-in?sc=request on a public channel still skipsVTEXSC/vtex_segment, which drops the selected channel on the next request unless the URL keepssc.💡 Pass the authenticated no-store signal into `setSegmentBag()`
-export const setSegmentBag = ( +export const setSegmentBag = ( cookies: Record<string, string>, req: Request, ctx: AppContext, + persistCookies = false, ) => { ... - if (!isCacheableSegment(ctx)) { + if (persistCookies || !isCacheableSegment(ctx)) {- if (!segment) { - setSegmentBag(cookies, req, ctx); - } - const isLoggedIn = Boolean( cookies[VTEX_ID_CLIENT_COOKIE] || cookies[`${VTEX_ID_CLIENT_COOKIE}_${ctx.account}`], ); + + if (!segment) { + setSegmentBag(cookies, req, ctx, isLoggedIn); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vtex/utils/segment.ts` around lines 287 - 307, The cookie persistence logic in setSegmentBag currently uses isCacheableSegment(ctx) alone and can drop VTEXSC/vtex_segment for authenticated requests that are later marked no-store; instead, change setSegmentBag to accept the final "no-store" decision (or a boolean like shouldNoStore/isAuthenticatedNoStore) from vtex/middleware.ts and base the cookie branch on that value rather than calling isCacheableSegment(ctx) directly. Concretely: update the setSegmentBag signature to accept a noStore flag, pass the computed no-store value from vtex/middleware.ts into setSegmentBag, and replace the if (!isCacheableSegment(ctx)) checks with if (!noStore) (or the inverse logic you choose) while keeping references to setCookie, SALES_CHANNEL_COOKIE, and SEGMENT_COOKIE_NAME unchanged.
74-84:⚠️ Potential issue | 🔴 CriticalClear inherited
channelPrivacywhen?scswitches channels.This branch now treats
"public"as sufficient to cache a non-default channel, butsetSegmentBag()only overrideschannel; it keepschannelPrivacyfromctx.defaultSegment/vtex_segment. A shopper moving from a public channel to?sc=2can therefore reuse the old"public"flag and make the first response for channel 2 cacheable again.💡 One safe way to drop stale privacy on channel switches
const segmentFromRequest = buildSegmentFromRequest(req); + const baseSegment = { + channel: ctx.salesChannel, + ...DEFAULT_SEGMENT, + ...ctx.defaultSegment, + ...segmentFromCookie, + ...segmentFromSalesChannelCookie, + }; const locale = { ...(ctx.defaultSegment?.countryCode && { countryCode: ctx.defaultSegment.countryCode, }), ...(ctx.defaultSegment?.cultureInfo && { cultureInfo: ctx.defaultSegment.cultureInfo, }), }; const segment = { - channel: ctx.salesChannel, - ...DEFAULT_SEGMENT, - ...ctx.defaultSegment, - ...segmentFromCookie, - ...segmentFromSalesChannelCookie, + ...baseSegment, + ...(segmentFromRequest.channel && + segmentFromRequest.channel !== baseSegment.channel + ? { channelPrivacy: undefined } + : {}), ...segmentFromRequest, ...locale, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vtex/utils/segment.ts` around lines 74 - 84, When a shopper switches channels (payload.channel set via setSegmentBag/?sc) we must not inherit a stale channelPrivacy; update the logic that handles segment updates so that when payload.channel is present and differs from the active/default sales channel you clear/reset channelPrivacy (e.g., delete payload.channelPrivacy or set it to undefined) before caching checks. Concretely, in the code paths around setSegmentBag and where you evaluate payload.channel and isDefautSalesChannel(ctx, channel), ensure you explicitly remove or reset payload.channelPrivacy so a previous "public" value cannot be reused for the new channel.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@vtex/utils/segment.ts`:
- Around line 287-307: The cookie persistence logic in setSegmentBag currently
uses isCacheableSegment(ctx) alone and can drop VTEXSC/vtex_segment for
authenticated requests that are later marked no-store; instead, change
setSegmentBag to accept the final "no-store" decision (or a boolean like
shouldNoStore/isAuthenticatedNoStore) from vtex/middleware.ts and base the
cookie branch on that value rather than calling isCacheableSegment(ctx)
directly. Concretely: update the setSegmentBag signature to accept a noStore
flag, pass the computed no-store value from vtex/middleware.ts into
setSegmentBag, and replace the if (!isCacheableSegment(ctx)) checks with if
(!noStore) (or the inverse logic you choose) while keeping references to
setCookie, SALES_CHANNEL_COOKIE, and SEGMENT_COOKIE_NAME unchanged.
- Around line 74-84: When a shopper switches channels (payload.channel set via
setSegmentBag/?sc) we must not inherit a stale channelPrivacy; update the logic
that handles segment updates so that when payload.channel is present and differs
from the active/default sales channel you clear/reset channelPrivacy (e.g.,
delete payload.channelPrivacy or set it to undefined) before caching checks.
Concretely, in the code paths around setSegmentBag and where you evaluate
payload.channel and isDefautSalesChannel(ctx, channel), ensure you explicitly
remove or reset payload.channelPrivacy so a previous "public" value cannot be
reused for the new channel.
Remove channel restriction from isCacheableSegment — all non-private channels are now cacheable. Always set vtex_segment and VTEXSC on every response so the CDN (Cloudflare) can vary its cache key by them and serve the correct bucket per sales channel. Requires deco-cx/deco#XXXX (allowlist vtex_segment/VTEXSC in middleware).
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
vtex/utils/segment.ts (1)
64-70:⚠️ Potential issue | 🔴 CriticalRequire explicit public privacy before caching non-default channels.
A cold
?sc=<non-default>request only haspayload.channelat this point. UntilchannelPrivacyis resolved, this helper returnstrue, sovtex/middleware.tsleaves the response public. That makes the first hit of a private sales channel eligible for CDN storage.🔒 Suggested guard
export const isCacheableSegment = (ctx: AppContext) => { const payload = getSegmentFromBag(ctx)?.payload; - if (payload?.channelPrivacy === "private") return false; - if (!payload) return true; - const { campaigns, priceTables, regionId } = payload; + const { channel, channelPrivacy, campaigns, priceTables, regionId } = payload; + const defaultChannel = ctx.salesChannel ?? + ctx.defaultSegment?.channel ?? + DEFAULT_SEGMENT.channel; + if (channel && channel !== defaultChannel && channelPrivacy !== "public") { + return false; + } return !campaigns && !priceTables && !regionId; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vtex/utils/segment.ts` around lines 64 - 70, isCacheableSegment currently treats segments with an unresolved channelPrivacy as cacheable (returning true), which allows a private channel to be served publicly; update isCacheableSegment (and the payload checks after getSegmentFromBag) so that if payload.channel is present you only allow caching when payload.channelPrivacy === "public" (otherwise return false), while keeping the existing fallback logic for default/no-channel segments and the existing checks for campaigns, priceTables, and regionId.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@vtex/utils/segment.ts`:
- Around line 263-273: The code currently sets vtex_segment based on the full
serialized token (token) which includes utm/utmi fields and thus fragments the
CDN cache; update the logic in the block that checks vtex_segment !== token to
instead compute a UTM-stripped representation (e.g., call or implement
getSegmentCacheKeyWithoutUTM(token) or use serialize() variant that excludes
utm_*/utmi_*), compare the existing cookie value against that UTM-less cache
key, and write that UTM-less key to the cookie (still using SEGMENT_COOKIE_NAME,
path "/", secure and httpOnly) so marketing params no longer cause edge cache
fragmentation; ensure references to serialize() and isCacheableSegment() are
preserved where needed and only the value written/compared is the non-UTM
segment cache key.
---
Duplicate comments:
In `@vtex/utils/segment.ts`:
- Around line 64-70: isCacheableSegment currently treats segments with an
unresolved channelPrivacy as cacheable (returning true), which allows a private
channel to be served publicly; update isCacheableSegment (and the payload checks
after getSegmentFromBag) so that if payload.channel is present you only allow
caching when payload.channelPrivacy === "public" (otherwise return false), while
keeping the existing fallback logic for default/no-channel segments and the
existing checks for campaigns, priceTables, and regionId.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
guitavano
approved these changes
Apr 1, 2026
…uestInit Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
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.
Summary
isDefautSalesChannelrestriction fromisCacheableSegment, enabling CDN cache for non-default sales channels (previously only the default channel was cacheable)isCacheableSegmentinline instead of delegating toisAnonymous, keeping campaigns/priceTables/regionId as the only blockersPAGE_CACHE_ALLOWED_KEYimport from VTEX middleware (no consumer in deco runtime)Test plan
?sc=X) getCache-Control: publicheaderschannelPrivacy=privatestill getno-storeheadersno-storeheadersno-storeheaders🤖 Generated with Claude Code
Summary by cubic
Enable CDN caching for all sales channels using cookie-based vary. Removed the default-channel restriction and UTM checks; always set
VTEXSCandvtex_segmentso Cloudflare serves the right cache per channel.New Features
channelPrivacyis "private".VTEXSC(when?sc=) and updatevtex_segment; removedPAGE_CACHE_ALLOWED_KEYand its import from@deco/deco/blocks.Refactors
@deco/decoRequestInitwith localDecoRequestInitin HTTP utils.Written for commit 3fdf7ae. Summary will update on new commits.
Summary by CodeRabbit