-
Notifications
You must be signed in to change notification settings - Fork 20
transform searchTerm to lower case #1476
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Tagging OptionsShould a new tag be published when this PR is merged?
|
WalkthroughThe pull request applies case-insensitive search term handling across product loader cache mechanisms. Search terms are lowercased before cache membership checks in five loader files spanning both intelligentSearch and legacy modules, ensuring cache hits regardless of query term casing. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Notes for review:
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
♻️ Duplicate comments (4)
vtex/loaders/intelligentSearch/productListingPage.ts (1)
437-437: LGTM: Consistent case-insensitive cache lookup.This change aligns with the same pattern applied across all loader files in this PR, enabling case-insensitive search term caching.
vtex/loaders/legacy/productList.ts (1)
294-294: LGTM: Consistent case-insensitive cache lookup.This change aligns with the same pattern applied across all loader files in this PR. The ctx.isInvoke check remains appropriately unchanged.
vtex/loaders/legacy/productListingPage.ts (1)
449-449: LGTM: Consistent case-insensitive cache lookup.This change aligns with the same pattern applied across all loader files in this PR, enabling case-insensitive search term caching.
vtex/loaders/intelligentSearch/productList.ts (1)
321-321: LGTM: Consistent case-insensitive cache lookup.This change aligns with the same pattern applied across all loader files in this PR. The ctx.isInvoke check remains appropriately unchanged.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
vtex/loaders/intelligentSearch/productList.ts(1 hunks)vtex/loaders/intelligentSearch/productListingPage.ts(1 hunks)vtex/loaders/legacy/productList.ts(1 hunks)vtex/loaders/legacy/productListingPage.ts(1 hunks)vtex/loaders/legacy/relatedProductsLoader.ts(1 hunks)
| const searchTerm = url.searchParams.get("ft") || url.searchParams.get("q"); | ||
| const cachedSearchTerms = ctx.cachedSearchTerms ?? []; | ||
| if (searchTerm && !cachedSearchTerms.includes(searchTerm)) { | ||
| if (searchTerm && !cachedSearchTerms.includes(searchTerm.toLowerCase())) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
Verify that cachedSearchTerms contains lowercase values.
The change correctly lowercases the searchTerm before checking inclusion in cachedSearchTerms. However, for this to work as intended, the cachedSearchTerms array must also contain only lowercase values. If cachedSearchTerms contains mixed-case terms (e.g., "Shoes"), this change could cause cache misses when the user searches for "shoes".
Run the following script to verify how cachedSearchTerms is populated and whether values are normalized to lowercase:
🏁 Script executed:
#!/bin/bash
# Search for where cachedSearchTerms is assigned or populated
rg -nP '\bcachedSearchTerms\s*[:=]' -A 3 -B 1
# Search for where ctx.cachedSearchTerms is set
rg -nP 'ctx\.cachedSearchTerms\s*=' -A 3 -B 1
# Look for any toLowerCase() normalization when populating cachedSearchTerms
rg -nP 'cachedSearchTerms.*toLowerCase|toLowerCase.*cachedSearchTerms' -C 2Length of output: 5201
Fix case-sensitivity mismatch in searchTerm comparison across all loaders.
The verification confirms your concern is valid. cachedSearchTerms is populated in vtex/mod.ts:183-186 without lowercasing (search.term), but multiple loaders lowercase the searchTerm during comparison (searchTerm.toLowerCase()). This causes cache misses when cached terms contain uppercase letters.
This pattern exists in:
vtex/loaders/legacy/relatedProductsLoader.ts:164vtex/loaders/legacy/productListingPage.ts:449vtex/loaders/legacy/productList.ts:294vtex/loaders/intelligentSearch/productListingPage.ts:437vtex/loaders/intelligentSearch/productList.ts:321
Normalize cachedSearchTerms to lowercase in vtex/mod.ts during population, or remove the .toLowerCase() call from all comparison checks to ensure consistency.
🤖 Prompt for AI Agents
In vtex/loaders/legacy/relatedProductsLoader.ts around line 164 the comparison
uses searchTerm.toLowerCase() against cachedSearchTerms which are populated
without lowercasing in vtex/mod.ts (lines ~183-186), causing cache misses; fix
by normalizing cachedSearchTerms to lowercase when you populate them in
vtex/mod.ts (i.e., push search.term.toLowerCase()), or alternatively remove the
.toLowerCase() from comparisons in all affected loaders
(vtex/loaders/legacy/relatedProductsLoader.ts:164,
vtex/loaders/legacy/productListingPage.ts:449,
vtex/loaders/legacy/productList.ts:294,
vtex/loaders/intelligentSearch/productListingPage.ts:437,
vtex/loaders/intelligentSearch/productList.ts:321) so both sides are consistent.
Summary by CodeRabbit
Release Notes