Deduplicate vtex cookie #1480
Deduplicate vtex cookie #1480
Conversation
Signed-off-by: Marcos Candeia <[email protected]>
Tagging OptionsShould a new tag be published when this PR is merged?
|
|
Warning Rate limit exceeded@mcandeia has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 23 minutes and 16 seconds before requesting another review. ⌛ 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. 📒 Files selected for processing (1)
WalkthroughAdds bag-based propagation of VTEX Cart/OrderForm IDs and cookie handling: new helpers extract and safely set the VTEX checkout cookie; the cart loader reads an orderFormId from the bag, augments request cookies, issues orderForm requests with the augmented cookie, and persists the response-derived cookie/orderFormId back into the bag. Changes
Sequence Diagram(s)sequenceDiagram
participant Req as Incoming Request
participant Loader as Cart Loader
participant Bag
participant Utils as Cookie Utils
participant API as OrderForm API
Note over Loader,Bag: Pre-request — read bag and compute cookie
Req->>Loader: invoke loader
Loader->>Bag: getOrderFormIdFromBag()
Bag-->>Loader: orderFormId?
Loader->>Utils: getCheckoutVtexCookie(request.headers)
Utils-->>Loader: currentCookie?
Loader->>Utils: safelySetCheckoutVtexCookie(currentCookie, orderFormId)
Utils-->>Loader: augmentedCookie
Loader->>API: fetch orderForm (with augmentedCookie header)
API-->>Loader: orderForm response (+ set-cookie)
Loader->>Bag: setOrderFormIdInBag(response-derived cookie / orderFormId)
Loader-->>Req: return cart response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
Signed-off-by: Marcos Candeia <[email protected]>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
vtex/utils/orderForm.ts (1)
7-17: Clarify the value passed tosafelySetCheckoutVtexCookieand handle empty cookie stringsThe helper works, but two details are a bit misleading:
At the call site in
vtex/loaders/cart.ts, the second argument is the full checkout cookie value returned bygetCheckoutVtexCookie(e.g."__ofid=…"), not a bare orderFormId. Naming the parameterorderFormIdcan easily confuse future callers into passing just the ID. Either:
- rename the parameter to something like
checkoutCookieValue, or- change the helper to add the
__ofid=prefix internally and pass only the ID from the caller.When
cookieStringis empty,safelySetCheckoutVtexCookiereturns a value starting with"; ". It’s generally safer to special‑case the empty string and return just"${VTEX_CHECKOUT_COOKIE}=${value}"in that case.vtex/utils/segment.ts (1)
9-10: Bag key naming doesn’t match the value that’s actually stored (ID vs cookie value)The bag entry is named
ORDER_FORM_IDand exposed asgetOrderFormIdFromBag/setOrderFormIdInBag, but invtex/loaders/cart.tsyou’re storing the full checkout cookie value returned bygetCheckoutVtexCookie(response.headers)(e.g."__ofid=…") rather than a bare orderFormId.That’s fine functionally, but the mismatch between name and content can be confusing when reading code across files. Consider either:
- Renaming the symbol and helpers to something like
CHECKOUT_COOKIE_VALUE, or- Actually storing the parsed orderFormId (e.g. splitting
"__ofid=<id>"once here) so the name matches the content.Also applies to: 71-78
vtex/loaders/cart.ts (1)
24-30: Consider reusing the augmented cookie for the marketingData attachment callFor the initial
POST /api/checkout/pub/orderFormyou correctly build:const withOrderFormIdCookie = orderFormId ? safelySetCheckoutVtexCookie(cookie, orderFormId) : cookie; … { headers: { cookie: withOrderFormIdCookie } }However, the marketingData attachment request still uses the original
cookievalue in its headers:headers: { accept: "application/json", "content-type": "application/json", cookie, },If the goal of this PR is to consistently reuse the VTEX checkout cookie (and avoid re-creating order forms) across all checkout-related calls within a request, you may want to pass
withOrderFormIdCookiehere as well so the attachment call benefits from the same deduplicated cookie value.If VTEX only relies on the
orderFormIdpath param for this endpoint, the current behaviour is functionally fine; this would just be a consistency/defensive improvement.Also applies to: 77-91
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
vtex/loaders/cart.ts(3 hunks)vtex/utils/orderForm.ts(1 hunks)vtex/utils/segment.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
vtex/utils/segment.ts (2)
vtex/mod.ts (1)
AppContext(27-27)files/mod.ts (1)
AppContext(16-16)
vtex/utils/orderForm.ts (1)
vtex/utils/openapi/orders.openapi.gen.ts (1)
Headers(6147-6149)
vtex/loaders/cart.ts (1)
vtex/utils/orderForm.ts (2)
safelySetCheckoutVtexCookie(12-17)getCheckoutVtexCookie(7-10)
| const { cookie } = parseCookie(req.headers); | ||
| const segment = getSegmentFromBag(ctx); | ||
|
|
||
| const response = await vcsDeprecated["POST /api/checkout/pub/orderForm"]( | ||
| const maybeOrderFormId = getCheckoutVtexCookieFromBag(ctx); | ||
| const orderFormId = maybeOrderFormId ? await maybeOrderFormId : undefined; | ||
| const withOrderFormIdCookie = orderFormId ? safelySetCheckoutVtexCookie(cookie, orderFormId) : cookie; | ||
| const responsePromise = vcsDeprecated["POST /api/checkout/pub/orderForm"]( | ||
| { sc: segment?.payload?.channel }, | ||
| { headers: { cookie } }, | ||
| { headers: { cookie: withOrderFormIdCookie } }, | ||
| ); | ||
|
|
||
| setCheckoutVtexCookieInBag(ctx, responsePromise.then((response) => getCheckoutVtexCookie(response.headers))); | ||
|
|
||
| const response = await responsePromise; | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
Verify that getCheckoutVtexCookie(response.headers) can actually see the checkout cookie
The bag-based dedup pattern around responsePromise looks good, but this line is a bit suspicious in terms of HTTP semantics:
setCheckoutVtexCookieInBag(
ctx,
responsePromise.then((response) => getCheckoutVtexCookie(response.headers)),
);getCheckoutVtexCookie is implemented via getCookies(headers), which typically inspects the cookie header. For HTTP responses, servers usually send cookies via Set-Cookie headers, and clients do not normally expose a cookie header on responses.
Unless your vcsDeprecated client or VTEX proxy explicitly populates a cookie header in response.headers, this will likely always resolve to undefined, meaning:
- The bag will store
Promise<undefined>. orderFormIdwill remainundefinedin subsequent calls, so the dedup logic viasafelySetCheckoutVtexCookienever kicks in.
You may need to instead parse the Set-Cookie headers (e.g. via a getSetCookies-style helper) to extract checkout.vtex.com from the response and store that.
Please double-check an actual VTEX response (e.g. via logging response.headers) to confirm whether a cookie header is present or if you should switch to parsing set-cookie instead.
🏁 Script executed:
#!/bin/bash
# Search for getCheckoutVtexCookie implementation
echo "=== Searching for getCheckoutVtexCookie implementation ==="
rg -n "getCheckoutVtexCookie\s*=" --type=ts -B2 -A5
echo ""
echo "=== Searching for getCookies implementation ==="
rg -n "getCookies\s*=" --type=ts -B2 -A5
echo ""
echo "=== Searching for safelySetCheckoutVtexCookie implementation ==="
rg -n "safelySetCheckoutVtexCookie\s*=" --type=ts -B2 -A5
echo ""
echo "=== Searching where getCheckoutVtexCookie is called/used ==="
rg -n "getCheckoutVtexCookie" --type=ts -C2
echo ""
echo "=== Checking cart.ts imports ==="
head -15 vtex/loaders/cart.tsLength of output: 3794
🏁 Script executed:
#!/bin/bash
# Search for where withOrderFormIdCookie is used
echo "=== Searching for withOrderFormIdCookie usage ==="
rg -n "withOrderFormIdCookie" --type=ts -B2 -A2
echo ""
echo "=== Searching for lines around 77-91 in cart.ts (attachment call) ==="
sed -n '75,95p' vtex/loaders/cart.tsLength of output: 1377
🏁 Script executed:
#!/bin/bash
# Search for Set-Cookie handling in the codebase
echo "=== Searching for Set-Cookie or getSetCookies ==="
rg -n "Set-Cookie|getSetCookies|set-cookie" --type=ts -C2Length of output: 7887
Fix getCheckoutVtexCookie to parse Set-Cookie headers from responses instead of Cookie headers
Your concern is confirmed: getCheckoutVtexCookie(response.headers) on line 32 has a fundamental issue.
The function uses getCookies(), which parses the REQUEST cookie header. However, API responses contain Set-Cookie headers, not cookie headers. This means the function will always return undefined when called on a response.
The codebase already has the correct utility available: getSetCookies() from the standard library (used in vtex/utils/cookies.ts and authentication actions). You need to either:
- Add a response-specific version of
getCheckoutVtexCookiethat usesgetSetCookies(), or - Use
getSetCookies()directly when handling the response.
Without this fix, the bag stores Promise<undefined> and the dedup logic never activates.
🤖 Prompt for AI Agents
In vtex/loaders/cart.ts around lines 22 to 35,
getCheckoutVtexCookie(response.headers) is incorrectly using a request-cookie
parser and thus returns undefined for API responses; change this to parse the
response Set-Cookie headers by using getSetCookies(response.headers) (either
call getSetCookies() directly here and extract the checkout cookie the same way
getCheckoutVtexCookie does, or implement a response-specific
getCheckoutVtexCookie that uses getSetCookies), then pass the extracted cookie
value into setCheckoutVtexCookieInBag so the bag stores the real cookie instead
of Promise<undefined>.
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/orderForm.ts (2)
safelySetCheckoutVtexCookie(12-17)getCheckoutVtexCookie(7-10)
🔇 Additional comments (1)
vtex/loaders/cart.ts (1)
77-91: LGTM! Correctly applies the updated cookie to the attachment request.The marketing data attachment request now consistently uses
withOrderFormIdCookie(line 88), which includes the orderFormId from the bag when available. This ensures both the initial orderForm request and the subsequent attachment request use the same cookie value, maintaining consistency throughout the request flow.
vtex/loaders/cart.ts
Outdated
| import { getSegmentFromBag } from "../utils/segment.ts"; | ||
| import { getCheckoutVtexCookie, hasDifferentMarketingData, parseCookie } from "../utils/orderForm.ts"; | ||
|
|
||
| import { getOrderFormIdFromBag as getCheckoutVtexCookieFromBag, getSegmentFromBag, setOrderFormIdInBag as setCheckoutVtexCookieInBag } from "../utils/segment.ts"; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove confusing import aliases that misrepresent return types.
The aliases getOrderFormIdFromBag as getCheckoutVtexCookieFromBag and setOrderFormIdInBag as setCheckoutVtexCookieInBag are misleading. Based on the naming and AI summary, these functions work with orderFormId values, not full cookie strings. The aliases suggest they return/set cookies, which creates confusion about their actual behavior.
Apply this diff to use the original function names:
-import { getOrderFormIdFromBag as getCheckoutVtexCookieFromBag, getSegmentFromBag, setOrderFormIdInBag as setCheckoutVtexCookieInBag } from "../utils/segment.ts";
+import { getOrderFormIdFromBag, getSegmentFromBag, setOrderFormIdInBag } from "../utils/segment.ts";And update the usage accordingly:
- const maybeOrderFormId = getCheckoutVtexCookieFromBag(ctx);
+ const maybeOrderFormId = getOrderFormIdFromBag(ctx);- setCheckoutVtexCookieInBag(ctx, responsePromise.then((response) => getCheckoutVtexCookie(response.headers)));
+ setOrderFormIdInBag(ctx, responsePromise.then((response) => getCheckoutVtexCookie(response.headers)));Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In vtex/loaders/cart.ts around line 5, the import aliases
getCheckoutVtexCookieFromBag and setCheckoutVtexCookieInBag are misleading
because the functions actually get/set orderFormId values; change the import to
use the original names getOrderFormIdFromBag and setOrderFormIdInBag, then
update every usage in this file to call getOrderFormIdFromBag and
setOrderFormIdInBag (rename variables/params where necessary) so the names
reflect they operate on orderFormId values rather than cookies.
Signed-off-by: Marcos Candeia <[email protected]>
Signed-off-by: Marcos Candeia <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
vtex/loaders/cart.ts (1)
9-13: Remove misleading import aliases.As flagged in a previous review, these aliases are confusing:
getOrderFormIdFromBag as getCheckoutVtexCookieFromBagsetOrderFormIdInBag as setCheckoutVtexCookieInBagThe functions actually store and retrieve the full cookie value (e.g.,
__ofid=abc123), not just the orderFormId and not a complete cookie string. The aliases suggest they work with "checkout VTEX cookies" when they actually work with cookie values.Based on past review comments.
Apply this diff to use the original function names and update variable names to reflect they hold cookie values:
-import { - getOrderFormIdFromBag as getCheckoutVtexCookieFromBag, - getSegmentFromBag, - setOrderFormIdInBag as setCheckoutVtexCookieInBag, -} from "../utils/segment.ts"; +import { + getOrderFormIdFromBag, + getSegmentFromBag, + setOrderFormIdInBag, +} from "../utils/segment.ts";Then update usage throughout the file:
- Line 32:
const maybeOrderFormId = getCheckoutVtexCookieFromBag(ctx);→const maybeCookieValue = getOrderFormIdFromBag(ctx);- Line 33:
const orderFormId = maybeOrderFormId ? await maybeOrderFormId : undefined;→const cookieValue = maybeCookieValue ? await maybeCookieValue : undefined;- Line 34:
const withOrderFormIdCookie = orderFormId→const withOrderFormIdCookie = cookieValue- Line 35:
? safelySetCheckoutVtexCookie(cookie, orderFormId)→? safelySetCheckoutVtexCookie(cookie, cookieValue)- Line 42:
setCheckoutVtexCookieInBag(→setOrderFormIdInBag(
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
vtex/loaders/cart.ts(3 hunks)vtex/utils/orderForm.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
vtex/utils/orderForm.ts (1)
vtex/utils/openapi/orders.openapi.gen.ts (1)
Headers(6147-6149)
vtex/loaders/cart.ts (1)
vtex/utils/orderForm.ts (2)
safelySetCheckoutVtexCookie(12-20)getCheckoutVtexCookie(7-10)
🔇 Additional comments (4)
vtex/utils/orderForm.ts (2)
1-1: LGTM! Correct import for parsing response cookies.The addition of
getSetCookiesis appropriate for parsingSet-Cookieheaders from HTTP responses, which is the correct approach compared togetCookies(used for requestCookieheaders).
7-10: LGTM! Correctly parses Set-Cookie headers from responses.The function properly uses
getSetCookiesto extract the VTEX checkout cookie from response headers. This is the correct approach for reading cookies from HTTP responses.vtex/loaders/cart.ts (2)
37-47: LGTM! Cookie extraction and bag storage correctly implemented.The flow properly:
- Uses the augmented cookie in the orderForm request
- Extracts the checkout cookie from response
Set-Cookieheaders viagetCheckoutVtexCookie(which now correctly usesgetSetCookies)- Stores the Promise in the bag for lazy evaluation in subsequent requests
- Awaits the response for processing
The async bag pattern (storing the Promise, awaiting when needed) is a valid approach for deduplication.
84-104: LGTM! Consistent cookie usage across requests.The marketingData attachment request correctly reuses
withOrderFormIdCookie(line 101), ensuring both the initial orderForm request and any subsequent attachment requests use the same augmented cookie. This maintains consistency within a single loader invocation.
| export const safelySetCheckoutVtexCookie = ( | ||
| cookieString: string, | ||
| orderFormId: string, | ||
| ) => { | ||
| if (cookieString.includes(VTEX_CHECKOUT_COOKIE)) { | ||
| return cookieString; | ||
| } | ||
| return `${cookieString}; ${VTEX_CHECKOUT_COOKIE}=${orderFormId}`; | ||
| }; |
There was a problem hiding this comment.
Rename misleading parameter and strengthen deduplication logic.
The function has several issues:
-
Misleading parameter name: The parameter
orderFormIdactually receives the full cookie value (e.g.,__ofid=abc123), not just the ID portion. This creates confusion throughout the codebase. -
Brittle deduplication check: The
includes(VTEX_CHECKOUT_COOKIE)check on line 16 could produce false positives if another cookie's value happens to contain the substring"checkout.vtex.com". It should parse cookie name-value pairs properly. -
Missing edge case handling: The concatenation on line 19 doesn't handle cases where
cookieStringis empty or already ends with a semicolon, potentially producing malformed strings like"; checkout.vtex.com=..."or"...; ; checkout.vtex.com=...".
Consider this improved implementation:
export const safelySetCheckoutVtexCookie = (
cookieString: string,
- orderFormId: string,
+ cookieValue: string,
) => {
- if (cookieString.includes(VTEX_CHECKOUT_COOKIE)) {
+ // Parse existing cookies to check if VTEX_CHECKOUT_COOKIE is already set
+ const existingCookies = cookieString.split(';').map(c => c.trim());
+ const hasCookie = existingCookies.some(c =>
+ c.startsWith(`${VTEX_CHECKOUT_COOKIE}=`)
+ );
+
+ if (hasCookie) {
return cookieString;
}
- return `${cookieString}; ${VTEX_CHECKOUT_COOKIE}=${orderFormId}`;
+
+ const prefix = cookieString.trim() ? `${cookieString}; ` : '';
+ return `${prefix}${VTEX_CHECKOUT_COOKIE}=${cookieValue}`;
};📝 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.
| export const safelySetCheckoutVtexCookie = ( | |
| cookieString: string, | |
| orderFormId: string, | |
| ) => { | |
| if (cookieString.includes(VTEX_CHECKOUT_COOKIE)) { | |
| return cookieString; | |
| } | |
| return `${cookieString}; ${VTEX_CHECKOUT_COOKIE}=${orderFormId}`; | |
| }; | |
| export const safelySetCheckoutVtexCookie = ( | |
| cookieString: string, | |
| cookieValue: string, | |
| ) => { | |
| // Parse existing cookies to check if VTEX_CHECKOUT_COOKIE is already set | |
| const existingCookies = cookieString.split(';').map(c => c.trim()); | |
| const hasCookie = existingCookies.some(c => | |
| c.startsWith(`${VTEX_CHECKOUT_COOKIE}=`) | |
| ); | |
| if (hasCookie) { | |
| return cookieString; | |
| } | |
| const prefix = cookieString.trim() ? `${cookieString}; ` : ''; | |
| return `${prefix}${VTEX_CHECKOUT_COOKIE}=${cookieValue}`; | |
| }; |
🤖 Prompt for AI Agents
In vtex/utils/orderForm.ts around lines 12 to 20, rename the misleading
parameter orderFormId to something like checkoutCookiePair (since it contains
the full "name=value" cookie pair), replace the brittle
cookieString.includes(VTEX_CHECKOUT_COOKIE) check with proper parsing: split
cookieString by ';', trim entries, parse each entry into name and value and
detect an existing cookie whose name equals VTEX_CHECKOUT_COOKIE to deduplicate
correctly, and handle edge cases when cookieString is empty or has trailing
semicolons by normalizing/trimming before concatenation; finally, append the
checkoutCookiePair using a single "; " separator only when needed (no leading
semicolon or duplicate semicolons).
Signed-off-by: Marcos Candeia <[email protected]>
Summary by CodeRabbit