Conversation
Tagging OptionsShould a new tag be published when this PR is merged?
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughUpdated the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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}
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/loaders/legacy/productListingPage.ts`:
- Around line 454-457: The conditional starting at the if using hasMap,
searchTerm, and cachedSearchTerms is misformatted; run the Deno formatter (deno
fmt) and commit the result so the block around the if (hasMap || (searchTerm &&
!cachedSearchTerms.includes(searchTerm.toLowerCase()))) { ... } and the
following declaration of fq are properly formatted according to project style.
- Around line 452-454: The code uses substring matching to set hasMap (const
hasMap = url.search.includes("map=")), which can falsely detect map when it
appears inside other params and cause cacheKey to be null; change detection to
be key-aware by using the URLSearchParams API (use url.searchParams.has('map'))
so hasMap accurately reflects the presence of the map param; update the
conditional that uses hasMap (the if using hasMap || (searchTerm &&
!cachedSearchTerms.includes(...))) and keep existing references to
ctx.cachedSearchTerms, cachedSearchTerms, and searchTerm unchanged.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: ccc9daec-f7c0-4b2c-bcc4-022c2b05dc5d
📒 Files selected for processing (1)
vtex/loaders/legacy/productListingPage.ts
| const hasMap = url.search.includes("map="); | ||
| const cachedSearchTerms = ctx.cachedSearchTerms ?? []; | ||
| if (searchTerm && !cachedSearchTerms.includes(searchTerm.toLowerCase())) { | ||
| if (hasMap || (searchTerm && !cachedSearchTerms.includes(searchTerm.toLowerCase()))) { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the map-detection implementation in cacheKey and check for raw substring usage.
rg -n -C2 'const hasMap\s*=' vtex/loaders/legacy/productListingPage.ts
rg -n -C2 'search\.includes\("map="\)|searchParams\.has\("map"\)' vtex/loaders/legacy/productListingPage.tsRepository: deco-cx/apps
Length of output: 637
🏁 Script executed:
#!/bin/bash
# Check the broader context: how is 'url' created/typed, and confirm searchParams availability
# Also check if there are other uses of the map parameter to understand the feature context
# Get more context around line 452 to see url object creation
sed -n '440,460p' vtex/loaders/legacy/productListingPage.ts
# Search for how 'url' is defined in this function
rg -n 'function|const url|let url|url\s*=' vtex/loaders/legacy/productListingPage.ts | head -20
# Check if url.searchParams is used elsewhere in the file
rg -n 'url\.searchParams' vtex/loaders/legacy/productListingPage.ts
# Look for any map parameter usage context
rg -n 'map' vtex/loaders/legacy/productListingPage.ts | head -30Repository: deco-cx/apps
Length of output: 3154
🏁 Script executed:
#!/bin/bash
# Check TypeScript context and any type constraints
head -30 vtex/loaders/legacy/productListingPage.ts | grep -E "import|export|interface|type"
# Check if the cacheKey function has explicit type annotations
sed -n '449,455p' vtex/loaders/legacy/productListingPage.tsRepository: deco-cx/apps
Length of output: 926
Use exact map key detection to avoid unintended cache bypass.
Line 452 uses raw substring matching (url.search.includes("map=")), which can incorrectly match map= within other parameter values or names (e.g., utm_campaign=email_map=test), causing cacheKey to return null unexpectedly and bypass caching.
The codebase already uses url.searchParams throughout the file, so the proper solution is to use key-aware detection:
Proposed fix
- const hasMap = url.search.includes("map=");
+ const hasMap = url.searchParams.has("map");📝 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 hasMap = url.search.includes("map="); | |
| const cachedSearchTerms = ctx.cachedSearchTerms ?? []; | |
| if (searchTerm && !cachedSearchTerms.includes(searchTerm.toLowerCase())) { | |
| if (hasMap || (searchTerm && !cachedSearchTerms.includes(searchTerm.toLowerCase()))) { | |
| const hasMap = url.searchParams.has("map"); | |
| const cachedSearchTerms = ctx.cachedSearchTerms ?? []; | |
| if (hasMap || (searchTerm && !cachedSearchTerms.includes(searchTerm.toLowerCase()))) { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@vtex/loaders/legacy/productListingPage.ts` around lines 452 - 454, The code
uses substring matching to set hasMap (const hasMap =
url.search.includes("map=")), which can falsely detect map when it appears
inside other params and cause cacheKey to be null; change detection to be
key-aware by using the URLSearchParams API (use url.searchParams.has('map')) so
hasMap accurately reflects the presence of the map param; update the conditional
that uses hasMap (the if using hasMap || (searchTerm &&
!cachedSearchTerms.includes(...))) and keep existing references to
ctx.cachedSearchTerms, cachedSearchTerms, and searchTerm unchanged.
![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 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/loaders/legacy/productListingPage.ts">
<violation number="1" location="vtex/loaders/legacy/productListingPage.ts:452">
P2: Use searchParams.has("map") instead of a substring match so only the actual `map` query parameter disables caching.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| const url = new URL(props.pageHref || req.url); | ||
|
|
||
| const searchTerm = url.searchParams.get("ft") || url.searchParams.get("q"); | ||
| const hasMap = url.search.includes("map="); |
There was a problem hiding this comment.
P2: Use searchParams.has("map") instead of a substring match so only the actual map query parameter disables caching.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At vtex/loaders/legacy/productListingPage.ts, line 452:
<comment>Use searchParams.has("map") instead of a substring match so only the actual `map` query parameter disables caching.</comment>
<file context>
@@ -449,8 +449,9 @@ export const cacheKey = (props: Props, req: Request, ctx: AppContext) => {
const url = new URL(props.pageHref || req.url);
const searchTerm = url.searchParams.get("ft") || url.searchParams.get("q");
+ const hasMap = url.search.includes("map=");
const cachedSearchTerms = ctx.cachedSearchTerms ?? [];
- if (searchTerm && !cachedSearchTerms.includes(searchTerm.toLowerCase())) {
</file context>
| const hasMap = url.search.includes("map="); | |
| const hasMap = url.searchParams.has("map"); |
1 participant
What is this Contribution About?
Please provide a brief description of the changes or enhancements you are proposing in this pull request.
Issue Link
Please link to the relevant issue that this pull request addresses:
Loom Video
Demonstration Link
Summary by cubic
Skip caching for product listing pages when the URL contains
map=to avoid stale faceted results.cacheKeystill returns null for new search terms not inctx.cachedSearchTerms.Written for commit 07ed5b2. Summary will update on new commits.
Summary by CodeRabbit