add Vary Cookie #1483
add Vary Cookie #1483
Conversation
WalkthroughModifies VTEX loaders to add conditional HTTP headers for checkout routes in the proxy handler, and enhances cart logging with defensive cookie-based data extraction including orderFormId and cookie count detection. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
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 |
Tagging OptionsShould a new tag be published when this PR is merged?
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
vtex/loaders/proxy.ts
Outdated
| customHeaders: [ | ||
| { | ||
| key: "Vary", | ||
| value: "Cookie, Accept-Encoding", | ||
| }, | ||
| ], |
There was a problem hiding this comment.
🧩 Analysis chain
Verify handler support and assess caching impact.
Adding Vary: Cookie will significantly affect caching behavior:
-
Cache efficiency: Every unique cookie combination will create a separate cache entry, potentially causing dramatic reductions in CDN/cache hit rates and increased storage costs.
-
Handler compatibility: Confirm that
website/handlers/proxy.tssupports thecustomHeadersfield. -
Documentation: Consider documenting why this change is necessary and the expected impact on cache performance.
For VTEX e-commerce sites handling personalized content (checkout, account), this may be intentional, but the broad application to all proxied paths warrants careful consideration.
Run the following script to verify the handler supports customHeaders:
Additionally, please search the web to understand best practices for Vary header usage in e-commerce CDN configurations:
🏁 Script executed:
#!/bin/bash
# Description: Check if website/handlers/proxy.ts supports customHeaders field
# Search for customHeaders in the proxy handler implementation
ast-grep --pattern $'customHeaders'
# Also check for the handler type definition
fd -e ts -e tsx proxy.ts | xargs rg -n "customHeaders" -A 3 -B 3Length of output: 4595
🌐 Web query:
Vary Cookie header CDN caching best practices e-commerce
💡 Result:
Concise best-practices for using Vary/Cookie with CDNs on e‑commerce sites
-
Avoid sending Vary: Cookie for whole Cookie header. it creates one cache key per full Cookie value (huge fragmentation, near-zero hit rate). (peakhour.io)
-
Prefer isolating dynamic fragments (edge-side includes / edge functions) so the HTML shell is cacheable and only small pieces (cart count, “logged in” fragment) are fetched per user. This preserves high cache hit rates. (vodien.com)
-
If you must vary on cookie, vary only on a specific cookie name/value (e.g., currency or locale) by using CDN rules that look up a single cookie, not the entire Cookie header. Many CDNs support “vary by cookie” or custom cache keys via edge config/workers. (peakhour.io)
-
Mark truly user-private pages as Cache-Control: private / no-store (or bypass CDN caching) so session/PII never gets stored in shared caches. Don’t rely on Vary alone to protect private content. (httpwg.org)
-
Know your CDN’s behavior and plan accordingly: some CDNs (or non‑Enterprise tiers) ignore Vary: Cookie for caching decisions or only support limited vary headers — test and use CDN-specific features (page rules, bypass-on-cookie, enterprise cache keys, workers). (simonwillison.net)
-
Use short-lived edge-cache TTLs and cache purging/webhooks for frequently-updated dynamic content; combine with stale-while-revalidate or background refresh where supported to avoid blocking user requests. (blog.cloudflare.com)
-
Test thoroughly (crawl as anonymous, logged-in, crawler UAs) to detect cache bleed (logged-in content served to anon) and validate SEO/crawler behavior. Maintain crawler rules so bots see expected variants. (vodien.com)
Quick implementation checklist
- Don’t emit Vary: Cookie globally.
- Use ESI/edge fragments or edge functions for per-user bits.
- If varying, configure CDN to vary by a single cookie name or custom cache key.
- Mark private responses as private/no-store.
- Verify CDN’s Vary handling in your tier; use page rules/enterprise features if needed.
- Add monitoring/crawls for cache correctness.
If you want, I can:
- Map these recommendations to a specific CDN (Cloudflare, Fastly, Akamai) with exact configuration examples and tests.
Citations:
- 1: https://www.peakhour.io/blog/vary-cache-on-cookie-value/?utm_source=openai
- 2: https://www.vodien.com/learn/cdn-caching-rules-for-seo-friendly-content/?utm_source=openai
- 3: https://www.peakhour.io/blog/vary-cache-on-cookie-value/?utm_source=openai
- 4: https://httpwg.org/specs/rfc7231.html?utm_source=openai
- 5: https://simonwillison.net/2023/Nov/20/cloudflare-does-not-consider-vary-values-in-caching-decisions/?utm_source=openai
- 6: https://blog.cloudflare.com/ecommerce-best-practices/?utm_source=openai
- 7: https://www.vodien.com/learn/cdn-caching-rules-for-seo-friendly-content/?utm_source=openai
Clarify intent and use CDN-specific cookie variance instead of global Vary header.
Handler support confirmed ✓—customHeaders is properly implemented in website/handlers/proxy.ts. However, emitting Vary: Cookie creates one cache key per full Cookie value, resulting in huge fragmentation and near-zero hit rate, which is critical for e-commerce.
Industry best practices recommend:
- If varying on cookie, vary only on a specific cookie name/value via CDN rules or custom cache keys, not the entire Cookie header
- Use edge-side includes or edge functions so the HTML shell remains cacheable with only small user-specific fragments fetched per user
- Mark truly user-private pages as Cache-Control: private/no-store instead of relying on Vary alone
Why is this change needed? Please clarify the business reason—are you addressing a security issue, cache bleed, or something else? The Vary header approach will likely degrade performance significantly on checkout, account, and login paths.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
vtex/utils/cookies.ts (1)
25-27: Clean up extra whitespace.Lines 25 and 27 contain trailing/unnecessary whitespace that should be removed for consistency.
- : cookie; - - if(cookie.name === "checkout.vtex.com" && options?.shouldClearCartCookie) { - + : cookie; + + if (cookie.name === "checkout.vtex.com" && options?.shouldClearCartCookie) {Note: Also add a space after
iffor consistency with JavaScript/TypeScript style conventions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
vtex/loaders/cart.ts(3 hunks)vtex/utils/cookies.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
vtex/loaders/cart.ts (1)
vtex/utils/cookies.ts (1)
proxySetCookie(8-42)
🔇 Additional comments (4)
vtex/utils/cookies.ts (1)
12-14: LGTM: Backward-compatible signature extension.The optional
optionsparameter properly extends the function signature without breaking existing callers.vtex/loaders/cart.ts (3)
28-43: LGTM: Consistent return structure in error paths.The function now returns a structured object
{ shouldClearCartCookie: false }in all early-return paths (missing cookie and JWT parse errors), which makes the function more predictable and easier to consume.
48-74: LGTM: Proper mismatch detection and flag setting.The function correctly returns
{ shouldClearCartCookie: true }when an email mismatch is detected between the cookie and order form, and{ shouldClearCartCookie: false }in the default case. The logging provides good observability for debugging cart mismatches.
110-114: Domain handling in proxySetCookie may prevent cookie clearing.The integration in cart.ts is clean, but the underlying
proxySetCookiefunction has a critical domain handling issue: whentoDomainis not provided, the cookie is set with an empty string domain (vtex/utils/cookies.ts:35), which may prevent the "checkout.vtex.com" cookie from being properly cleared.Additionally, the URL parsing at line 16 (
new URL(toDomain)) lacks error handling and will throw if an invalid URL is passed.Verify that:
- The empty string domain fallback actually clears cookies correctly in the browser
req.urlis always a valid URL in practice (though it should be, as a standard Request property)
vtex/utils/cookies.ts
Outdated
| if(cookie.name === "checkout.vtex.com" && options?.shouldClearCartCookie) { | ||
|
|
||
| setCookie(to, { | ||
| name: "checkout.vtex.com", | ||
| value: "", | ||
| expires: new Date(0), | ||
| maxAge: 0, | ||
| path: "/", | ||
| secure: true, | ||
| httpOnly: true, | ||
| domain: newDomain ? newDomain?.hostname : "", | ||
| }); |
There was a problem hiding this comment.
Fix domain handling when clearing cookie.
When toDomain is not provided (i.e., newDomain is falsy), line 36 sets domain: "", which may fail to clear the cookie. Cookies must be cleared with the same domain they were originally set with.
Consider either:
- Not setting the
domainproperty whennewDomainis falsy (allowing the browser to use the current domain) - Using the original cookie's domain from
cookie.domain
Apply this diff:
- domain: newDomain ? newDomain?.hostname : "",
+ domain: newDomain ? newDomain.hostname : cookie.domain,Additionally, line 36 has redundant optional chaining—if newDomain is truthy, .hostname is guaranteed to exist.
📝 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.
| if(cookie.name === "checkout.vtex.com" && options?.shouldClearCartCookie) { | |
| setCookie(to, { | |
| name: "checkout.vtex.com", | |
| value: "", | |
| expires: new Date(0), | |
| maxAge: 0, | |
| path: "/", | |
| secure: true, | |
| httpOnly: true, | |
| domain: newDomain ? newDomain?.hostname : "", | |
| }); | |
| if(cookie.name === "checkout.vtex.com" && options?.shouldClearCartCookie) { | |
| setCookie(to, { | |
| name: "checkout.vtex.com", | |
| value: "", | |
| expires: new Date(0), | |
| maxAge: 0, | |
| path: "/", | |
| secure: true, | |
| httpOnly: true, | |
| domain: newDomain ? newDomain.hostname : cookie.domain, | |
| }); |
🤖 Prompt for AI Agents
In vtex/utils/cookies.ts around lines 26 to 37, the cookie-clear code sets
domain: "" when newDomain is falsy and uses redundant optional chaining; change
it so the domain is only set when newDomain is provided (use domain:
newDomain.hostname) and otherwise either omit the domain property so the browser
uses the current origin or explicitly use the original cookie.domain value
(cookie.domain) when available; remove the unnecessary optional chaining on
newDomain.hostname.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
vtex/loaders/cart.ts (1)
75-82: NormalizeORDER_FORM_IDS_TO_CLEARvalues to avoid subtle mismatchesThe current parsing:
const orderFormIdsToClear = Deno.env.get("ORDER_FORM_IDS_TO_CLEAR")?.split(","); if (orderFormIdFromRequest && orderFormIdsToClear?.includes(orderFormIdFromRequest)) { return { shouldClearCartCookie: true }; }is fragile if the env var contains spaces or empty entries (e.g.,
"id1, id2"). A small normalization step will make this more robust without changing behavior for well‑formatted values:- const orderFormIdsToClear = Deno.env.get("ORDER_FORM_IDS_TO_CLEAR")?.split(","); + const orderFormIdsToClear = Deno.env.get("ORDER_FORM_IDS_TO_CLEAR") + ?.split(",") + .map((id) => id.trim()) + .filter(Boolean);This ensures that operational misconfigurations (extra spaces, trailing commas) don’t silently prevent cookie clearing when it’s expected.
After adjusting, it’d be good to verify with a local env like
ORDER_FORM_IDS_TO_CLEAR="foo , bar, baz "that only the intended IDs are recognized.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
vtex/loaders/cart.ts(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
vtex/loaders/cart.ts (1)
vtex/utils/cookies.ts (1)
proxySetCookie(8-42)
🔇 Additional comments (3)
vtex/loaders/cart.ts (3)
34-36: Consistent return contract fromlogMismatchedCartlooks goodAlways returning
{ shouldClearCartCookie: boolean }(including on missing/invalid auth cookie and match cases) makes the loader usage straightforward and avoidsundefinedhandling. No issues here; this is a solid shape change.Also applies to: 40-43, 84-85
120-120: WiringshouldClearCartCookiethrough toproxySetCookieis correct; consider testsPlumbing the
{ shouldClearCartCookie }result fromlogMismatchedCartintoproxySetCookiematches the updatedproxySetCookiesignature and cleanly centralizes the cookie‑clearing behavior there. The control flow looks coherent.It would be valuable to add at least one integration/unit test that:
- Forces a mismatch and a known
orderFormIdFromRequestinORDER_FORM_IDS_TO_CLEAR.- Asserts that the outgoing
Set-Cookieforcheckout.vtex.comis cleared as expected.This will guard against regressions in the cookie parsing and env‑driven decision logic.
Also applies to: 123-125
55-58: The review comment is incorrect. The cookie parsing is already correct.The code in
vtex/loaders/cart.tsline 57 correctly extracts theorderFormId:
getCookiesreturns parsed cookie values as aRecord<string, string>. Thecheckout.vtex.comcookie value is formatted as__ofid=<orderFormId>(not JSON or URL-encoded), as evidenced byvtex/utils/orderForm.ts:
- Line 42: validation regex
/^__ofid=([0-9a-fA-F])+$/confirms the format- Line 47: uses identical logic
ofidCookie.split("=")to extract the ID- Line 65:
formatCookiecreates the value as__ofid=${orderFormId}The existing code at line 57 (
split("=").at(1)) correctly extracts the order form ID and will properly match againstORDER_FORM_IDS_TO_CLEARas checked at line 77. The suggested fix would unnecessarily complicate the code and break the current working implementation.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
vtex/loaders/cart.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
vtex/loaders/cart.ts (1)
vtex/utils/cookies.ts (1)
proxySetCookie(8-42)
🔇 Additional comments (4)
vtex/loaders/cart.ts (4)
34-43: LGTM: Consistent return type improves type safety.The early returns now provide a consistent object shape
{ shouldClearCartCookie: boolean }, which improves type safety and makes the function contract clearer.
57-83: Excellent observability improvements.The addition of
hasTwoCookies,OrderFormIdFromRequestin the mismatch logging, and the dedicated warning log when clearing cookies significantly improves debugging capability for cart-related issues.
124-128: LGTM: Clean integration with proxySetCookie.The destructuring and passing of
shouldClearCartCookietoproxySetCookieis implemented correctly and matches the updated signature invtex/utils/cookies.ts.
1-1: Vary header implementation verified and confirmed.The search confirms that the Vary header implementation exists in
vtex/loaders/proxy.ts(lines 80-83). The implementation correctly:
- Checks if the path template includes "/checkout"
- Injects a
Vary: Cookie, Accept-Encodingheader for matching pathsThe code changes are correct and no further action is required.
| const emailFromCookie = jwtPayload?.sub; | ||
| const userIdFromCookie = jwtPayload?.userId; | ||
|
|
||
| const orderFormIdFromRequest = cookies["checkout.vtex.com"]?.split("=").at(1); |
There was a problem hiding this comment.
Validate cookie format before parsing.
The cookie parsing assumes the value contains an "=" separator and takes the part after it. This could fail silently if the cookie format is unexpected (e.g., no "=" present, multiple "=" characters).
Consider adding validation or using a more robust parsing approach:
- const orderFormIdFromRequest = cookies["checkout.vtex.com"]?.split("=").at(1);
+ const cookieValue = cookies["checkout.vtex.com"];
+ const orderFormIdFromRequest = cookieValue?.includes("=")
+ ? cookieValue.split("=").at(1)
+ : cookieValue;Alternatively, document the expected cookie format if this structure is guaranteed by the VTEX platform.
📝 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.
| const orderFormIdFromRequest = cookies["checkout.vtex.com"]?.split("=").at(1); | |
| const cookieValue = cookies["checkout.vtex.com"]; | |
| const orderFormIdFromRequest = cookieValue?.includes("=") | |
| ? cookieValue.split("=").at(1) | |
| : cookieValue; |
🤖 Prompt for AI Agents
In vtex/loaders/cart.ts around line 48, the code blindly splits
cookies["checkout.vtex.com"] on "=" and takes .at(1), which can fail for missing
or malformed values; validate the cookie string first (existence and that it
contains at least one "="), then parse robustly by finding the first "="
(indexOf) and using substring to get everything after it (so multiple "=" are
preserved), trim the result, and fall back to null or a safe default if
validation fails; add a brief comment documenting the expected cookie format.
vtex/loaders/cart.ts
Outdated
| const orderFormIdsToClear = Deno.env.get("ORDER_FORM_IDS_TO_CLEAR")?.split(","); | ||
|
|
||
| if (orderFormIdFromRequest && orderFormIdsToClear?.includes(orderFormIdFromRequest)) { | ||
| logger.warn(`Clearing cart cookie `, { | ||
| orderFormIdFromRequest, | ||
| EmailFromCookie: emailFromCookie, | ||
| EmailFromOrderForm: email, | ||
| }) | ||
| return { shouldClearCartCookie: true }; | ||
| } | ||
|
|
||
| return { shouldClearCartCookie: false }; |
There was a problem hiding this comment.
Add whitespace trimming for environment variable parsing.
The comma-separated ORDER_FORM_IDS_TO_CLEAR environment variable is split without trimming whitespace, which could cause matching failures if the configuration includes spaces (e.g., "id1, id2, id3").
Apply this diff to handle whitespace:
- const orderFormIdsToClear = Deno.env.get("ORDER_FORM_IDS_TO_CLEAR")?.split(",");
+ const orderFormIdsToClear = Deno.env.get("ORDER_FORM_IDS_TO_CLEAR")
+ ?.split(",")
+ .map(id => id.trim())
+ .filter(id => id.length > 0);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
vtex/loaders/cart.ts around lines 76 to 87: the ORDER_FORM_IDS_TO_CLEAR env var
is split without trimming, so values like "id1, id2" won't match; update parsing
to split on commas, map each item to .trim(), and filter out empty strings
(e.g., .split(",").map(s => s.trim()).filter(Boolean)) before using
.includes(orderFormIdFromRequest) so whitespace in the env value won't prevent
matches.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
vtex/loaders/cart.ts (2)
28-28: Trim and sanitizeORDER_FORM_IDS_TO_CLEARbefore matching
ORDER_FORM_IDS_TO_CLEARis split on commas but entries aren’t trimmed, so a config like"id1, id2"will never matchorderFormIdFromRequest(" id2"vs"id2"). It’s safer to normalize once at load time:-const orderFormIdsToClear = Deno.env.get("ORDER_FORM_IDS_TO_CLEAR")?.split(","); +const orderFormIdsToClear = Deno.env.get("ORDER_FORM_IDS_TO_CLEAR") + ?.split(",") + .map((id) => id.trim()) + .filter((id) => id.length > 0);If this list ever grows, you might also consider turning it into a
Setfor faster lookups, but that’s optional at current scale.Also applies to: 78-85
50-65: Harden checkout cookie parsing and avoid leaking sensitive headers in logsTwo things here:
- Checkout cookie parsing is brittle
cookies["checkout.vtex.com"]?.split("=").at(1)assumes:
- The value always contains
"=", and- You only care about the part after the first
"=".A malformed value (no
"=") silently yieldsundefined, and multiple"="lose everything after the second part. A more defensive parse that preserves everything after the first"="is safer:- const orderFormIdFromRequest = cookies["checkout.vtex.com"]?.split("=").at(1); + const rawCheckoutCookie = cookies["checkout.vtex.com"]; + const orderFormIdFromRequest = rawCheckoutCookie && rawCheckoutCookie.includes("=") + ? rawCheckoutCookie.slice(rawCheckoutCookie.indexOf("=") + 1).trim() + : rawCheckoutCookie; + + // Expected format: "id=<orderFormId>" set by VTEX's checkout cookieThis keeps working for the current
"id=<orderFormId>"shape, preserves any additional"="in the value, and degrades gracefully if the format changes.
reqHeaderslogging may leak secrets
reqHeaderscurrently logs almost all request headers, only excluding"cookie"and"cache-control". That will typically includeauthorizationand potentially other credentials/API keys, which is risky from a security/compliance standpoint.Consider expanding the denylist or switching to a small allowlist of headers you genuinely need for debugging, e.g.:
- const headersDenyList = new Set(["cookie", "cache-control"]); + const headersDenyList = new Set([ + "cookie", + "cache-control", + "authorization", + "x-vtex-api-appkey", + "x-vtex-api-apptoken", + ]);Or invert this logic to only log a vetted subset of headers.
Also applies to: 57-75
🧹 Nitpick comments (1)
vtex/loaders/cart.ts (1)
36-45: Confirm that env-driven cart clearing should depend on a valid auth cookieBecause of the early returns:
if (!userFromCookie) { return { shouldClearCartCookie: false }; } const [jwtPayload, error] = safeParseJwt(userFromCookie); if (error) { console.error("Error parsing JWT", error); return { shouldClearCartCookie: false }; }the
ORDER_FORM_IDS_TO_CLEARlogic only runs when aVtexIdclientAutCookie_*is present and the JWT parses correctly. Any requests without that cookie (or with an invalid one) can never trigger the env-based clearing, even if theircheckout.vtex.comorderFormId is in the configured list.If the intention is to clear only for authenticated sessions, this is fine but might merit a short comment. If instead you want to force-clear specific broken carts regardless of login state, consider moving the
orderFormIdFromRequest/orderFormIdsToClearcheck before these early returns, or separating “logging mismatch” from “deciding whether to clear the cart cookie”.Also applies to: 78-88
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
vtex/loaders/cart.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
vtex/loaders/cart.ts (3)
vtex/utils/types.ts (1)
OrderForm(8-43)vtex/mod.ts (1)
AppContext(27-27)vtex/utils/cookies.ts (1)
proxySetCookie(8-42)
🔇 Additional comments (1)
vtex/loaders/cart.ts (1)
124-128: WiringshouldClearCartCookieintoproxySetCookielooks consistentUsing
logMismatchedCartto compute{ shouldClearCartCookie }and passing it through toproxySetCookiecentralizes the cookie-clearing decision and keeps the loader body clean. The control flow and types here look sound given the updatedproxySetCookiesignature.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
vtex/loaders/cart.ts (2)
28-29: Trim whitespace when parsing environment variable.The comma-separated
ORDER_FORM_IDS_TO_CLEARenvironment variable is split without trimming whitespace. This could cause matching failures on line 70 if the configuration includes spaces (e.g.,"id1, id2, id3").
41-41: Validate cookie format before parsing.The cookie parsing assumes the value contains an "=" separator. This could fail silently if the cookie format is unexpected.
🧹 Nitpick comments (1)
vtex/loaders/cart.ts (1)
70-80: Consistent return type improves API contract.The function now consistently returns an object
{ shouldClearCartCookie: boolean }in all code paths, which is a good improvement over the previous implementation that could returnundefined.Minor nitpick: Line 71 has a trailing space in the log message string.
Apply this diff to remove the trailing space:
- logger.warn(`Clearing cart cookie `, { + logger.warn(`Clearing cart cookie`, {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
vtex/loaders/cart.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
vtex/loaders/cart.ts (3)
vtex/utils/types.ts (1)
OrderForm(8-43)vtex/mod.ts (1)
AppContext(27-27)vtex/utils/cookies.ts (1)
proxySetCookie(8-42)
🔇 Additional comments (2)
vtex/loaders/cart.ts (2)
36-36: Good defensive coding practice.Conditionally parsing the JWT only when
userFromCookieexists prevents unnecessary parsing attempts and potential errors. This is more efficient and safer.
116-120: Clean integration of cookie-clearing flow.The destructuring of
shouldClearCartCookieand passing it toproxySetCookieproperly connects the decision logic to the cookie-clearing implementation. The integration is straightforward and correct.
vtex/loaders/cart.ts
Outdated
| ) { | ||
| const headersDenyList = new Set(["cookie", "cache-control"]); | ||
|
|
||
| const hasTwoCookies = req.headers.get("cookie")?.split("checkout.vtex.com")?.length === 3; |
There was a problem hiding this comment.
Fragile duplicate cookie detection logic.
Splitting the entire cookie header on the string "checkout.vtex.com" and checking length === 3 could produce false positives if a cookie value happens to contain this substring. While this is only used for logging, a more robust approach would parse cookies properly and count actual cookie occurrences by name.
🤖 Prompt for AI Agents
In vtex/loaders/cart.ts around line 51, the current check splits the entire
Cookie header on the substring "checkout.vtex.com" and compares the resulting
array length to 3, which is fragile; instead parse the Cookie header into
individual cookies by splitting on ';', trim each pair, split each pair on the
first '=' to get the cookie name, then count how many cookie names exactly equal
(or match the expected cookie name) "checkout.vtex.com" (or the precise cookie
name used) and set hasTwoCookies when that count is >= 2; update the logging to
use this parsed-count value so detection is based on actual cookie names rather
than substring matches.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
vtex/loaders/proxy.ts (1)
80-87: Implementation is correct; caching concerns previously raised remain valid.The conditional logic correctly applies the Vary header only to checkout-related paths. However, the previous review's concerns about CDN cache fragmentation with
Vary: Cookiestill apply here.vtex/utils/cookies.ts (1)
26-39: Domain handling issue from previous review remains unresolved.The previous review correctly identified that setting
domain: ""on line 35 whennewDomainis falsy may fail to clear the cookie. Additionally, line 35 contains redundant optional chaining (newDomain?.hostnamewhennewDomainis already verified as truthy).Apply the suggested fix from the previous review:
- domain: newDomain ? newDomain?.hostname : "", + domain: newDomain ? newDomain.hostname : cookie.domain,vtex/loaders/cart.ts (2)
59-59: Cookie parsing fragility from previous review remains unresolved.The previous review correctly identified that this parsing assumes the cookie value contains an "=" separator, which could fail silently if the format is unexpected.
Apply the suggested fix from the previous review to handle edge cases:
- const orderFormIdFromRequest = cookies["checkout.vtex.com"]?.split("=").at(1); + const cookieValue = cookies["checkout.vtex.com"]; + const orderFormIdFromRequest = cookieValue?.includes("=") + ? cookieValue.split("=").at(1) + : cookieValue;
69-70: Fragile duplicate cookie detection from previous review remains unresolved.The previous review correctly identified that splitting on the substring
"checkout.vtex.com"could produce false positives if any cookie value contains this text.A more robust approach would parse individual cookies by name rather than using substring matching, as suggested in the previous review.
🧹 Nitpick comments (1)
vtex/loaders/cart.ts (1)
89-102: Consider defensive trimming for orderFormIdsToClear matching.The logic correctly checks if the order form ID should be cleared. However, if configuration values in
orderFormIdsToClearcould contain whitespace (e.g., from manual entry), the comparison might fail.Consider adding defensive trimming:
if ( orderFormIdFromRequest && - orderFormIdsToClear?.includes(orderFormIdFromRequest) + orderFormIdsToClear?.map(id => id.trim()).includes(orderFormIdFromRequest.trim()) ) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
vtex/loaders/cart.ts(5 hunks)vtex/loaders/proxy.ts(1 hunks)vtex/utils/cookies.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
vtex/loaders/cart.ts (3)
vtex/mod.ts (2)
Props(43-108)AppContext(27-27)vtex/utils/types.ts (1)
OrderForm(8-43)vtex/utils/cookies.ts (1)
proxySetCookie(8-41)
🔇 Additional comments (4)
vtex/utils/cookies.ts (1)
12-14: LGTM!The optional
optionsparameter is properly typed and integrates well with the downstream usage in the cart loader.vtex/loaders/cart.ts (3)
20-31: LGTM!The Props interface is well-defined with clear documentation for the new configuration options.
41-46: LGTM!The function signature change appropriately reflects the new behavior of returning mismatch status for downstream decision-making.
111-148: LGTM!The loader integration correctly implements the new cart mismatch detection and cookie clearing flow. The conditional logic properly gates the cookie clearing behavior based on both the configuration flag and the detected mismatch.
aka-sacci-ccr
force-pushed
the
add-vary-cookie
branch
from
1f8c16c to
ba21729
Compare
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.