Fix #4460: parse comparison/range syntax in container style() queries#4461
Fix #4460: parse comparison/range syntax in container style() queries#4461dweep-js wants to merge 2 commits into
Conversation
…ries The mediaFeature lookahead regex only matched a bare identifier before a comparison operator (=, >, <, >=, <=), so it failed whenever the operand was a function call, e.g. var(--n) or calc(6/2). Widened the regex to also match a single level of balanced parens before the operator. Added regression tests covering: @container style(var(--n) = 3) @container style(calc(6 / 2) = var(--n)) @container style(var(--size) > 1lh)
📝 WalkthroughWalkthroughThe parser now recognizes more ChangesContainer Style Query Parsing
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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.
🧹 Nitpick comments (1)
packages/less/lib/less/parser/parser.js (1)
1903-1903: 🎯 Functional Correctness | 🔵 TrivialLookahead only supports one level of paren nesting.
The new pattern
(?:[^()]|\([^()]*\))*\s*([<>]=|<=|>=|[<>]|=)correctly handles single-level function calls likevar(--n)andcalc(6 / 2), matching the PR's test cases. However,\([^()]*\)cannot match a nested group (e.g.calc((1 + 1) / 2) = var(--n)), so doubly-nested operands would fail this lookahead and fall through to theelsebranch — likely reproducing the original "Missing closing ')'" error (issue#4460) for that narrower case.Since function-call operands with internal parens (nested
calc(),min()/max()insidecalc(), etc.) are plausible in real container queries, consider whether the lookahead should support arbitrary nesting depth (e.g., a small recursive/manual balanced-paren scan) rather than exactly one level.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/less/lib/less/parser/parser.js` at line 1903, The lookahead in parserInput.$re only handles one level of parentheses, so nested container-query operands can still miss the comparison operator and fall through incorrectly. Update the queryInParens handling in parser.js to support balanced/nested paren scanning instead of the current single-level regex, keeping the logic in the same parser branch that checks syntaxOptions.queryInParens before deciding between the query path and the fallback.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/less/lib/less/parser/parser.js`:
- Line 1903: The lookahead in parserInput.$re only handles one level of
parentheses, so nested container-query operands can still miss the comparison
operator and fall through incorrectly. Update the queryInParens handling in
parser.js to support balanced/nested paren scanning instead of the current
single-level regex, keeping the logic in the same parser branch that checks
syntaxOptions.queryInParens before deciding between the query path and the
fallback.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bb970b68-1f4e-4d26-9333-2c74b64688cc
📒 Files selected for processing (3)
packages/less/lib/less/parser/parser.jspackages/test-data/tests-unit/container/container.csspackages/test-data/tests-unit/container/container.less
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/less/lib/less/parser/parser.js (1)
1903-1903: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winLookahead only handles one level of nested parens — deeper nesting still fails.
The broadened regex
(?:[^()]|\([^()]*\))*\s*([<>]=|<=|>=|[<>]|=)correctly handlesvar(--n) = 3andcalc(6 / 2) = var(--n), but a doubly-nested operand likecalc(var(--x) + 1)ormin(var(--a), var(--b))still can't be matched, since\([^()]*\)disallows inner parens. Such inputs would fall through to the non-comparison branch and likely reproduce the same "Missing closing ')'" failure this PR fixes, just one level deeper.Since the PR explicitly scopes the fix to "a single level of balanced parentheses," this may be an accepted limitation, but it's worth confirming whether nested
calc()/min()expressions are a realistic near-term need (they're common in container queries), given the fix could be extended to arbitrary nesting depth via a small counting-based scan instead of a fixed-depth regex.♻️ Example of a depth-agnostic balanced-paren scan
- if (!p && syntaxOptions.queryInParens && parserInput.$re(/^(?:[^()]|\([^()]*\))*\s*([<>]=|<=|>=|[<>]|=)/)) { + if (!p && syntaxOptions.queryInParens && hasComparisonOperatorAhead()) {where
hasComparisonOperatorAhead()scans forward tracking paren depth and only tests for the operator when depth is 0.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/less/lib/less/parser/parser.js` at line 1903, The lookahead in parserInput.$re within parser.js only supports one level of balanced parentheses, so deeper nested expressions like calc() or min() still miss the comparison branch. Update the query-in-parens detection used in the parser logic around syntaxOptions.queryInParens to handle arbitrary nested balanced parentheses instead of the current fixed-depth regex, ideally by scanning ahead with a paren-depth counter and checking for the comparison operator only at depth 0. Keep the existing behavior for simple cases like var(--n) and calc(6 / 2) while ensuring nested operands are matched correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/less/lib/less/parser/parser.js`:
- Line 1903: The lookahead in parserInput.$re within parser.js only supports one
level of balanced parentheses, so deeper nested expressions like calc() or min()
still miss the comparison branch. Update the query-in-parens detection used in
the parser logic around syntaxOptions.queryInParens to handle arbitrary nested
balanced parentheses instead of the current fixed-depth regex, ideally by
scanning ahead with a paren-depth counter and checking for the comparison
operator only at depth 0. Keep the existing behavior for simple cases like
var(--n) and calc(6 / 2) while ensuring nested operands are matched correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d96e7a3b-941e-45a2-93be-32fa6a37566b
📒 Files selected for processing (1)
packages/less/lib/less/parser/parser.js
Fixes #4460
What:
mediaFeature's lookahead regex for range/comparison syntax inside@container/@mediaparentheses only matched a bare lowercaseidentifier before the operator (
=,>,<,>=,<=). It failedwhenever the left-hand operand was a function call, such as
var(--n)orcalc(6/2), because parentheses aren't in thecharacter class the regex used for its lookahead. This caused a
"Missing closing ')'" parse error for CSS container style queries
using range syntax with custom-property/calc operands, e.g.:
Why:
This is valid CSS per the container queries spec, and the existing
comparison-parsing machinery (
condition()/atomicCondition()/operand()) already supports function-call operands viaentities.call()— the only broken piece was the lookahead regexdeciding whether to even attempt the comparison branch. Widening it
to tolerate a single level of balanced parens (
(?:[^()]|\([^()]*\))*)before the operator fixes all three cases in the issue without
touching any other parsing path.
Checklist:
Summary by CodeRabbit
Bug Fixes
Tests
@container style(...)queries using equality and greater-than comparisons, including cases withcalc(...)expressions and custom properties.