fix slugify for price ranges #1434
Conversation
Tagging OptionsShould a new tag be published when this PR is merged?
|
WalkthroughFormatting and whitespace edits across several utilities and HubSpot actions, plus a functional change adding price-range detection to Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as slugify(caller)
participant SL as slugify
rect #E8F6EF
Caller->>SL: provide input string
alt matches price-range (de-<num>-a-<num>)
SL->>SL: remove commas\nreplace separators with '-' \nreplace special chars\nmap chars, preserve decimals\nlowercase and return
else non-price
SL->>SL: apply original replacements\nmap chars\nlowercase and return
end
end
sequenceDiagram
participant Loader as productListingPage loader
participant legacy as legacyFacetToFilter
Loader->>legacy: call legacyFacetToFilter(facet, false)
Note right of legacy #FFF3CD: Previously the second arg could be (name === "Categories")\nNow always `false`, changing Categories handling
legacy-->>Loader: return filter
Loader->>Loader: collect filters and continue processing
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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 |
![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
🧹 Nitpick comments (3)
vtex/utils/slugify.ts (3)
12-14: Remove redundant underscore replacement.Underscore appears in both Line 13's pattern
[·/_:]and Line 14's pattern. The first replacement converts underscore to hyphen, making the second occurrence redundant.Apply this diff:
- .replace(/,(\d+)/g, ".$1") // Convert comma decimals to dot decimals - .replace(/[·/_:]/g, "-") - .replace(/[*+~()'"!:@&\[\]`/ %$#?{}|><=_^]/g, "-") // Remove dot from this regex too + .replace(/,(\d+)/g, ".$1") + .replace(/[·/_:]/g, "-") + .replace(/[*+~()'"!:@&\[\]`/ %$#?{}|><=^]/g, "-") // Removed redundant underscore
21-29: Original behavior correctly preserved.The non-price path maintains the existing logic, which is appropriate for backward compatibility.
Optionally, you could remove the redundant underscore from Line 25's pattern since Line 24 already replaces it.
5-30: Consider adding JSDoc documentation.The function now handles a special case (price ranges with decimal preservation) that would benefit from documentation explaining the Portuguese "de-X-a-Y" format and the rationale for different handling.
Example:
+/** + * Converts a string to a URL-friendly slug. + * + * Special handling for Portuguese price ranges (e.g., "de-400-a-799.99"): + * - Preserves decimal points in numeric values + * - Normalizes comma decimals to dot decimals + * + * @param str - The string to slugify + * @returns The slugified string + * + * @example + * slugify("Café São Paulo") // "cafe-sao-paulo" + * slugify("de-400-a-799.99") // "de-400-a-799.99" + * slugify("de-400-a-799,99") // "de-400-a-799.99" + */ export const slugify = (str: string) => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
google-drive/utils/query.ts(1 hunks)google-drive/utils/types.ts(3 hunks)hubspot/actions/conversations/archiveThread.ts(0 hunks)hubspot/actions/conversations/updateThread.ts(0 hunks)vtex/utils/slugify.ts(1 hunks)
💤 Files with no reviewable changes (2)
- hubspot/actions/conversations/updateThread.ts
- hubspot/actions/conversations/archiveThread.ts
🔇 Additional comments (2)
google-drive/utils/types.ts (1)
127-248: LGTM! JSDoc formatting improvements.The changes add blank lines within JSDoc comments for improved readability. No functional impact.
google-drive/utils/query.ts (1)
3-17: LGTM! JSDoc formatting improvements.The changes add blank lines within the JSDoc comment for improved readability. No functional impact.
| // Check if this looks like a price range (e.g., "de-400-a-799.99") | ||
| const isPriceRange = /de-\d+([,.]?\d+)?-a-\d+([,.]?\d+)?/.test(str); | ||
|
|
There was a problem hiding this comment.
Anchor the regex to prevent false positives.
The current regex uses .test() without anchors, so it matches if the pattern appears anywhere in the string. For example, "product-name-de-400-a-799-other-text" would incorrectly be treated as a price range.
Apply this diff to anchor the regex:
- const isPriceRange = /de-\d+([,.]?\d+)?-a-\d+([,.]?\d+)?/.test(str);
+ const isPriceRange = /^de-\d+([,.]?\d+)?-a-\d+([,.]?\d+)?$/.test(str);📝 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.
| // Check if this looks like a price range (e.g., "de-400-a-799.99") | |
| const isPriceRange = /de-\d+([,.]?\d+)?-a-\d+([,.]?\d+)?/.test(str); | |
| // Check if this looks like a price range (e.g., "de-400-a-799.99") | |
| const isPriceRange = /^de-\d+([,.]?\d+)?-a-\d+([,.]?\d+)?$/.test(str); |
🤖 Prompt for AI Agents
In vtex/utils/slugify.ts around lines 6 to 8, the regex used to detect price
ranges is unanchored and can match substrings anywhere in the input; update the
pattern to anchor it (use ^ and $ or equivalent) so it only matches the entire
string formatted as a price range (e.g., ^de-\d+([,.]?\d+)?-a-\d+([,.]?\d+)?$),
then run tests or sample inputs like "product-de-400-a-799-other" to verify it
no longer yields false positives.
| return str | ||
| .replace(/,/g, "") | ||
| .replace(/[·/_:]/g, "-") // Remove dot from this regex to preserve decimals | ||
| .replace(/[*+~()'"!:@&\[\]`/ %$#?{}|><=_^]/g, "-") // Remove dot from this regex too | ||
| .split("") | ||
| .map((char) => mapped[char] ?? char) | ||
| .join("") | ||
| .toLowerCase(); |
There was a problem hiding this comment.
Critical: Comma decimals are corrupted.
The regex at Line 7 accepts both comma and dot as decimal separators ([,.]?), but Line 12 removes all commas. This means price ranges with comma decimals (e.g., "de-400-a-799,99") become "de-400-a-79999" instead of "de-400-a-799.99".
Apply this diff to convert comma decimals to dots:
- return str
- .replace(/,/g, "")
- .replace(/[·/_:]/g, "-") // Remove dot from this regex to preserve decimals
+ return str
+ .replace(/,(\d+)/g, ".$1") // Convert comma decimals to dot decimals
+ .replace(/[·/_:]/g, "-")📝 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.
| return str | |
| .replace(/,/g, "") | |
| .replace(/[·/_:]/g, "-") // Remove dot from this regex to preserve decimals | |
| .replace(/[*+~()'"!:@&\[\]`/ %$#?{}|><=_^]/g, "-") // Remove dot from this regex too | |
| .split("") | |
| .map((char) => mapped[char] ?? char) | |
| .join("") | |
| .toLowerCase(); | |
| return str | |
| .replace(/,(\d+)/g, ".$1") // Convert comma decimals to dot decimals | |
| .replace(/[·/_:]/g, "-") | |
| .replace(/[*+~()'"!:@&\[\]`/ %$#?{}|><=_^]/g, "-") // Remove dot from this regex too | |
| .split("") | |
| .map((char) => mapped[char] ?? char) | |
| .join("") | |
| .toLowerCase(); |
🤖 Prompt for AI Agents
In vtex/utils/slugify.ts around lines 11 to 18, the current chain removes all
commas which corrupts comma-based decimals (e.g. "799,99" -> "79999"); before
removing generic commas, first convert comma decimals to dots by adding a
replacement that turns digit-comma-digit sequences into digit-dot-digit (e.g.
use a regex like /(\d),(\d+)/g -> '$1.$2'), then continue to remove any
remaining stray commas and the other characters as before so "799,99" becomes
"799.99" and then slugified correctly.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
vtex/loaders/legacy/productListingPage.ts(2 hunks)
🧰 Additional context used
🪛 GitHub Actions: ci
vtex/loaders/legacy/productListingPage.ts
[error] 370-371: Deno fmt check failed. Found 1 not formatted file in 2080 files. Run 'deno fmt' to fix formatting.
🔇 Additional comments (1)
vtex/loaders/legacy/productListingPage.ts (1)
391-391: Confirm intentional change tofullPathfor Categories facets
legacyFacetToFilter’s 8th argument (fullPath) is now alwaysfalse—previously it wasname === "Categories", enabling full-path handling for Categories facets.
• Is disabling fullPath for Categories intentional?
• How does this relate to the PR’s “fix slugify for price ranges”?
•fullPathcontrols whether the filter uses the entire URL segment hierarchy.
| ? getFlatCategories(vtexFacets.CategoriesTrees) | ||
| : {}; | ||
|
|
||
|
|
There was a problem hiding this comment.
Remove extra blank line to fix formatting check.
The Deno formatter detected an extra blank line here that should be removed.
Apply this diff to fix the formatting:
-
const filters = Object.entries({📝 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 filters = Object.entries({ |
🧰 Tools
🪛 GitHub Actions: ci
[error] 370-371: Deno fmt check failed. Found 1 not formatted file in 2080 files. Run 'deno fmt' to fix formatting.
🤖 Prompt for AI Agents
In vtex/loaders/legacy/productListingPage.ts around line 370, there's an extra
blank line that fails the Deno formatter; remove that blank line so the file has
no consecutive empty lines at that location and re-run the formatter (deno fmt)
to verify formatting passes.
Summary by CodeRabbit
Style
Improvements