fix(vtex): gate vtex_segment cookie on isCacheableSegment - #1642
Open
igoramf wants to merge 5 commits into
Open
Conversation
Set-Cookie: vtex_segment was emitted whenever isAnonymous returned false, which includes UTM-only requests. Cloudflare skips caching any response with Set-Cookie, so landing pages carrying utm_source/utm_campaign stopped being CDN-cacheable and the origin dropped from public,max-age=300 to no-store. The gate now mirrors the middleware's own cacheability predicate (isCacheableSegment). Any response the middleware treats as cacheable stays Set-Cookie-free, so UTM-only and non-default sales-channel requests become CDN-cacheable again. Channel continuity still rides on the VTEXSC cookie, and the segment payload/token are untouched (UTM still reaches the VTEX API and the client for GA/pixel). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughUTM cache-key configuration now controls whether UTM fields enter simulation marketing data and whether segment cookies are emitted for cacheable requests. ChangesUTM cache behavior
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
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.
3 issues found across 1 file
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:267">
P1: UTM-only requests now become publicly cacheable here, but `simulation.ts` still treats any UTM as non-anonymous and sends it as `marketingData`; a VTEX promotion keyed by that UTM can therefore produce a campaign price in a response shared with users without the UTM. Requests whose marketing data can affect pricing need to remain out of the public page cache, or those fields need to be excluded from simulation for cacheable requests.</violation>
<violation number="2" location="vtex/utils/segment.ts:267">
P2: Logged-in UTM requests now skip `vtex_segment` even though middleware marks them `no-store`: this gate checks `isCacheableSegment` but not the same auth-cookie predicate used by middleware. The prior `!isAnonymous` path persisted this changed segment, so the gate should also retain the cookie for logged-in requests.</violation>
<violation number="3" location="vtex/utils/segment.ts:267">
P1: This gate removes only `vtex_segment`; a non-default `?sc=...` request still executes the preceding `setCookie(... name: SALES_CHANNEL_COOKIE)` branch and returns `Set-Cookie: VTEXSC`. With the Cloudflare behavior described in this PR, that landing response still bypasses the CDN, so the claimed non-default-channel cacheability fix is incomplete.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| // UTM-only and non-default sales channel) stay Set-Cookie-free and CDN- | ||
| // cacheable. Mirrors the middleware's cacheability check (isCacheableSegment) | ||
| // so the cookie gate and the Cache-Control decision never disagree. | ||
| if (vtex_segment !== token && !isCacheableSegment(ctx)) { |
There was a problem hiding this comment.
P1: UTM-only requests now become publicly cacheable here, but simulation.ts still treats any UTM as non-anonymous and sends it as marketingData; a VTEX promotion keyed by that UTM can therefore produce a campaign price in a response shared with users without the UTM. Requests whose marketing data can affect pricing need to remain out of the public page cache, or those fields need to be excluded from simulation for cacheable requests.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At vtex/utils/segment.ts, line 267:
<comment>UTM-only requests now become publicly cacheable here, but `simulation.ts` still treats any UTM as non-anonymous and sends it as `marketingData`; a VTEX promotion keyed by that UTM can therefore produce a campaign price in a response shared with users without the UTM. Requests whose marketing data can affect pricing need to remain out of the public page cache, or those fields need to be excluded from simulation for cacheable requests.</comment>
<file context>
@@ -260,9 +260,11 @@ export const setSegmentBag = (
+ // UTM-only and non-default sales channel) stay Set-Cookie-free and CDN-
+ // cacheable. Mirrors the middleware's cacheability check (isCacheableSegment)
+ // so the cookie gate and the Cache-Control decision never disagree.
+ if (vtex_segment !== token && !isCacheableSegment(ctx)) {
setCookie(ctx.response.headers, {
value: token,
</file context>
| // UTM-only and non-default sales channel) stay Set-Cookie-free and CDN- | ||
| // cacheable. Mirrors the middleware's cacheability check (isCacheableSegment) | ||
| // so the cookie gate and the Cache-Control decision never disagree. | ||
| if (vtex_segment !== token && !isCacheableSegment(ctx)) { |
There was a problem hiding this comment.
P1: This gate removes only vtex_segment; a non-default ?sc=... request still executes the preceding setCookie(... name: SALES_CHANNEL_COOKIE) branch and returns Set-Cookie: VTEXSC. With the Cloudflare behavior described in this PR, that landing response still bypasses the CDN, so the claimed non-default-channel cacheability fix is incomplete.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At vtex/utils/segment.ts, line 267:
<comment>This gate removes only `vtex_segment`; a non-default `?sc=...` request still executes the preceding `setCookie(... name: SALES_CHANNEL_COOKIE)` branch and returns `Set-Cookie: VTEXSC`. With the Cloudflare behavior described in this PR, that landing response still bypasses the CDN, so the claimed non-default-channel cacheability fix is incomplete.</comment>
<file context>
@@ -260,9 +260,11 @@ export const setSegmentBag = (
+ // UTM-only and non-default sales channel) stay Set-Cookie-free and CDN-
+ // cacheable. Mirrors the middleware's cacheability check (isCacheableSegment)
+ // so the cookie gate and the Cache-Control decision never disagree.
+ if (vtex_segment !== token && !isCacheableSegment(ctx)) {
setCookie(ctx.response.headers, {
value: token,
</file context>
| // UTM-only and non-default sales channel) stay Set-Cookie-free and CDN- | ||
| // cacheable. Mirrors the middleware's cacheability check (isCacheableSegment) | ||
| // so the cookie gate and the Cache-Control decision never disagree. | ||
| if (vtex_segment !== token && !isCacheableSegment(ctx)) { |
There was a problem hiding this comment.
P2: Logged-in UTM requests now skip vtex_segment even though middleware marks them no-store: this gate checks isCacheableSegment but not the same auth-cookie predicate used by middleware. The prior !isAnonymous path persisted this changed segment, so the gate should also retain the cookie for logged-in requests.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At vtex/utils/segment.ts, line 267:
<comment>Logged-in UTM requests now skip `vtex_segment` even though middleware marks them `no-store`: this gate checks `isCacheableSegment` but not the same auth-cookie predicate used by middleware. The prior `!isAnonymous` path persisted this changed segment, so the gate should also retain the cookie for logged-in requests.</comment>
<file context>
@@ -260,9 +260,11 @@ export const setSegmentBag = (
+ // UTM-only and non-default sales channel) stay Set-Cookie-free and CDN-
+ // cacheable. Mirrors the middleware's cacheability check (isCacheableSegment)
+ // so the cookie gate and the Cache-Control decision never disagree.
+ if (vtex_segment !== token && !isCacheableSegment(ctx)) {
setCookie(ctx.response.headers, {
value: token,
</file context>
Suggested change
| if (vtex_segment !== token && !isCacheableSegment(ctx)) { | |
| if ( | |
| vtex_segment !== token && | |
| (!isCacheableSegment(ctx) || | |
| Boolean( | |
| cookies["VtexIdclientAutCookie"] || | |
| cookies[`VtexIdclientAutCookie_${ctx.account}`], | |
| )) | |
| ) { |
UTM can trigger VTEX promotions and change price, so treating UTM-only requests as cacheable is only safe when the store has opted into removeUTMFromCacheKey — the same flag the product loaders already use to strip UTM from their cache key. Without it, dropping the vtex_segment cookie would let the CDN cache one UTM variant's HTML and serve it generically, breaking per-UTM pricing. With the flag on, gate the cookie on isCacheableSegment (ignores UTM); otherwise keep the previous isAnonymous behavior so UTM-carrying requests stay non-cacheable and cookie-bearing. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Align the middleware's Cache-Control decision with the cookie gate and the loader cache-key decision: a UTM-carrying request is only treated as cacheable when the store opts into removeUTMFromCacheKey. Otherwise UTM disqualifies caching, since UTM can trigger VTEX promotions and change price. Channel stays cacheable (CDN varies by VTEXSC), unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The middleware change was unnecessary. With removeUTMFromCacheKey off, a UTM request is already handled: the vtex_segment cookie blocks CDN caching, and the product loaders keep UTM in their cache key so deco's page cache is keyed per-UTM (correct pricing). Forcing no-store instead only threw away that per-UTM caching, contradicting the flag's design. Middleware reverts to main; the cookie gate in segment.ts is the whole fix. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
With the flag on, the page is cached with UTM stripped from the cache key, so a UTM-triggered VTEX promotion computed in the simulation would be cached and served generically. Skip feeding UTM into marketingData in that mode to keep the simulation consistent with the caching contract. Campaigns, priceTables, regionId and channel are still simulated. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
vtex/utils/extensions/simulation.ts (1)
41-44: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer
ifstatements over&&chaining for side effects.Using logical AND (
&&) for side effects without assignment is generally considered a code smell. Using explicitifblocks improves readability and avoids potential linter warnings for unused expressions.♻️ Proposed refactor
- utmInKey && utm_campaign && md.set("utmCampaign", utm_campaign); - utmInKey && utm_source && md.set("utmSource", utm_source); - utmInKey && utmi_campaign && md.set("utmiCampaign", utmi_campaign); - campaigns && md.set("campaigns", [{ id: campaigns }]); + if (utmInKey) { + if (utm_campaign) md.set("utmCampaign", utm_campaign); + if (utm_source) md.set("utmSource", utm_source); + if (utmi_campaign) md.set("utmiCampaign", utmi_campaign); + } + if (campaigns) md.set("campaigns", [{ id: campaigns }]);🤖 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 `@vtex/utils/extensions/simulation.ts` around lines 41 - 44, In the metadata population logic, replace the side-effect-only && expressions for utmInKey/utm_campaign, utm_source, utmi_campaign, and campaigns with explicit if statements. Preserve each existing condition and corresponding md.set call without changing the stored values.
🤖 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.
Nitpick comments:
In `@vtex/utils/extensions/simulation.ts`:
- Around line 41-44: In the metadata population logic, replace the
side-effect-only && expressions for utmInKey/utm_campaign, utm_source,
utmi_campaign, and campaigns with explicit if statements. Preserve each existing
condition and corresponding md.set call without changing the stored values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 257cd8b9-9190-477b-85fa-e15046bf794a
📒 Files selected for processing (2)
vtex/utils/extensions/simulation.tsvtex/utils/segment.ts
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/extensions/simulation.ts">
<violation number="1" location="vtex/utils/extensions/simulation.ts:38">
P2: Enabling `removeUTMFromCacheKey` now drops UTM from every simulation request, so UTM-dependent pricing is ignored even for non-cacheable requests that still call this extension. Preserving UTM for non-cacheable segments and stripping it only for cacheable responses would avoid regressing the existing simulation behavior for no-store responses.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| // cache key. Feeding UTM into the simulation here would let a UTM-triggered | ||
| // promotion change the price and get cached generically, so skip it to keep | ||
| // the simulation consistent with the caching contract. | ||
| const utmInKey = !ctx.advancedConfigs?.removeUTMFromCacheKey; |
There was a problem hiding this comment.
P2: Enabling removeUTMFromCacheKey now drops UTM from every simulation request, so UTM-dependent pricing is ignored even for non-cacheable requests that still call this extension. Preserving UTM for non-cacheable segments and stripping it only for cacheable responses would avoid regressing the existing simulation behavior for no-store responses.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At vtex/utils/extensions/simulation.ts, line 38:
<comment>Enabling `removeUTMFromCacheKey` now drops UTM from every simulation request, so UTM-dependent pricing is ignored even for non-cacheable requests that still call this extension. Preserving UTM for non-cacheable segments and stripping it only for cacheable responses would avoid regressing the existing simulation behavior for no-store responses.</comment>
<file context>
@@ -30,10 +30,17 @@ const doSimulate = (items: {
+ // cache key. Feeding UTM into the simulation here would let a UTM-triggered
+ // promotion change the price and get cached generically, so skip it to keep
+ // the simulation consistent with the caching contract.
+ const utmInKey = !ctx.advancedConfigs?.removeUTMFromCacheKey;
+
const md = new Map<string, unknown>();
</file context>
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
1 participant
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.
Problem
Set-Cookie: vtex_segmentwas emitted wheneverisAnonymous()returnedfalse, andisAnonymoustreats any request carryingutm_source/utm_campaign/utmi_campaignas non-anonymous.vtex_segmentis a non-framework ("foreign") cookie, so deco'sapplyPageCacheDecisionforcesno-storewhenever it is set. Landing pages arriving with UTM params therefore stopped being cacheable — origin dropped frompublic, max-age=300tono-store.Fix
Two changes, both gated on the existing
advancedConfigs.removeUTMFromCacheKeyflag — the same flag the product loaders already use to strip UTM from their cache key (#1517). The flag is the store's explicit contract: "UTM does not affect content/price here."1. Cookie gate (
utils/segment.ts)!isCacheableSegment(ctx)(ignores UTM). UTM-only requests emit no cookie → stay cacheable.!isAnonymous(ctx). UTM-carrying requests stay cookie-bearing →no-store. Identical tomain.2. Price simulation (
utils/extensions/simulation.ts)marketingData. Since the page is cached with UTM stripped from the key, a UTM-triggered promotion computed here would otherwise be cached and served generically. Campaigns / priceTables / regionId / channel are still simulated.The segment payload/token are untouched — UTM still reaches the VTEX API where relevant and stays on the client (querystring/JS) for GA/pixel. Channel continuity rides on
VTEXSC.isAnonymous,isCacheableSegmentand the middleware are unchanged.Behavior
no-store(= main)public, max-age=300, UTM out of cache key & simulationno-storeno-storeno-storeno-storeWith the flag off, behavior is identical to
main. Enabling the flag is the store's assertion that UTM is analytics-only; the simulation gate makes that assertion airtight against a later UTM-driven promotion being cached generically.Summary by CodeRabbit