Repository navigation
fix(vtex): cache bounded legacy map= PLP shapes, keep deep facet crawl uncached - #1687
Draft
pedrofrxncx wants to merge 2 commits into
Draft
pedrofrxncx wants to merge 2 commits into
pedrofrxncx wants to merge 2 commits into
Conversation
#1570 made cacheKey return null for any URL containing "map=", to avoid stale faceted results. A null key makes deco mark the whole page no-store, so every legacy ?map= URL (brand links /<brand>?map=b, category+brand /a/b?map=c,b, static-filter facet links) is uncacheable at the CDN. The key already varies on map. What it was missing are two inputs the loader reads straight from the URL: fmap, which drives the facets query (and which static-filter facet links set alongside map), and PS, the page size. Add both to the key (only when present, so existing keys are unchanged) and drop the map bail-out. Uncached search terms still return null.
Contributor
Tagging OptionsShould a new tag be published when this PR is merged?
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Docstring Coverage |  |
No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 2… |
| Linked Issues check | |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
| Title check | |
The title clearly identifies the main change: bounded caching for legacy map= PLP shapes while keeping deep facet crawls uncached. |
| Description check | |
The description clearly explains the motivation, data, implementation, tests, validation status, and remaining work. It does not include the template's Issue Link, Loom Video, or Demonstration Link se… |
✨ Finishing Touches: 💡 1
-
📝 Generate docstrings
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
🛠️ Fix failing CI checks: 💡
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 help to get the list of available commands.
![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. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. 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:453">
P2: Removing the map bypass lets a PLP with `props.ft` cache without checking that term against `cachedSearchTerms`; the loader uses `props.ft` as its effective full-text term. Apply the allowlist check to the effective term so unlisted searches remain uncached.</violation>
</file>
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
| hasMap || | ||
| (searchTerm && !cachedSearchTerms.includes(searchTerm.toLowerCase())) | ||
| ) { | ||
| if (searchTerm && !cachedSearchTerms.includes(searchTerm.toLowerCase())) { |
There was a problem hiding this comment.
P2: Removing the map bypass lets a PLP with props.ft cache without checking that term against cachedSearchTerms; the loader uses props.ft as its effective full-text term. Apply the allowlist check to the effective term so unlisted searches remain uncached.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At vtex/loaders/legacy/productListingPage.ts, line 453:
<comment>Removing the map bypass lets a PLP with `props.ft` cache without checking that term against `cachedSearchTerms`; the loader uses `props.ft` as its effective full-text term. Apply the allowlist check to the effective term so unlisted searches remain uncached.</comment>
<file context>
@@ -449,12 +449,8 @@ export const cacheKey = (props: Props, req: Request, ctx: AppContext) => {
- hasMap ||
- (searchTerm && !cachedSearchTerms.includes(searchTerm.toLowerCase()))
- ) {
+ if (searchTerm && !cachedSearchTerms.includes(searchTerm.toLowerCase())) {
return null;
}
</file context>
Suggested change
| if (searchTerm && !cachedSearchTerms.includes(searchTerm.toLowerCase())) { | |
| const effectiveSearchTerm = props.ft || searchTerm; | |
| if ( | |
| effectiveSearchTerm && | |
| !cachedSearchTerms.includes(effectiveSearchTerm.toLowerCase()) | |
| ) { |
pedrofrxncx
marked this pull request as draft
October 14, 2026 17:01
Deep facet combinations, spec filters, price ranges and ft text segments are bot crawl space with ~1.0 reuse; caching them evicts hot entries. Cache only up to 2 c/b map segments that match the path.
pedrofrxncx
changed the title
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
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.
Why
#1570 made the legacy PLP
cacheKeyreturnnullfor any URL containingmap=. deco treats anullkey as no-store, so every legacy?map=page bypasses the CDN, including the clean ones like brand links (/<brand>?map=b) and category+brand pages (/a/b?map=c,b).The reason for #1570 (thanks @guitavano) was that bots crawl endless filter combinations. Each one is a one-off URL, and caching them pushed hot entries out of the loader cache. This PR keeps that protection and only re-enables caching for shapes with bounded cardinality.
Data (miess, Cloudflare, last 24h, GET
?map=pages)priceFrom,ft, or map not matching the pathc/b, matching the pathq/ftsearch paramSo caching the deep combinations buys nothing and only churns the cache. The bounded shapes repeat.
Change
map=URLs get a key only when the map has ≤2 parts, every part iscorb, and the part count matches the path segment count. Everything else still returnsnull, same as today.fmapandPSto the key, only when present. The loader reads both from the URL:fmapdrives the facets query andPSthe page size. Keys for URLs without them are byte-identical to before.cachedSearchTermsstill returnnull.Tests
productListingPage_test.ts:/brand?map=band/cat/brand?map=c,bget keys, and different maps get different keysc,c,c),specificationFilter_*,priceFrom,ft, and mismatched map/path staynullfmap/PSvary the keymap/fmap/PSexactly match the current outputnulldeno fmt --check,deno lintanddeno check vtex/mod.tsare clean.Not done yet
Draft until it's validated on a miess-01 preview pinned to this commit, compared against production on the same URLs.
The deep facet crawl (44k req/day, ~1.9 GB) stays uncached. Reducing that is a robots.txt / bot-rule decision, not a cache one.