feat: JSDoc type annotations for all tree node files#4413
Merged
matthew-dean merged 2 commits intoless:masterfrom Mar 10, 2026
Merged
feat: JSDoc type annotations for all tree node files#4413matthew-dean merged 2 commits intoless:masterfrom
matthew-dean merged 2 commits intoless:masterfrom
Conversation
dosubot
Bot
added
the
size:XXL
📝 WalkthroughWalkthroughConverts many Less AST node implementations from prototype-style to ES6 classes, adds // Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
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 unit tests (beta)
Comment |
matthew-dean
force-pushed
the
feat/jsdoc-classes
branch
from
0c1a282 to
8b9e255
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
packages/less/lib/less/functions/style.js (1)
20-27:⚠️ Potential issue | 🟠 MajorDon't swallow all
style()evaluation failures.This fallback now hides real Less errors too. If
styleExpressionthrows because a Less variable is missing or invalid, the function will quietly returnundefinedand fall back to plain CSS instead of failing. It also suppresses the new zero-argumentArgumenterror. Limit the fallback to the CSS-function case only, and let genuine evaluation errors propagate.🔧 Narrow the fallback instead of catching everything
export default { /** `@param` {...*} args */ style: function(...args) { - try { - return styleExpression.call(this, args); - } catch (e) { - // When style() is used as a CSS function (e.g. `@container` style(--responsive: true)), - // arguments won't be valid Less variables. Return undefined to let the - // parser fall through and treat it as plain CSS. - } + if (args.length === 0 || args[0] instanceof Variable) { + return styleExpression.call(this, args); + } + + // When style() is used as a CSS function (e.g. `@container` style(--responsive: true)), + // the argument is not a Less variable. Return undefined so the call is emitted as CSS. + return; }, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/less/lib/less/functions/style.js` around lines 20 - 27, The current try/catch in style() swallows all errors from styleExpression; narrow it so only the CSS-function case is ignored: inspect the received args in style(...args) and if they look like a raw CSS function invocation (e.g., a single Anonymous/Quoted node or an arg whose .type indicates it’s not a parsed Less expression) return undefined, otherwise rethrow the caught error so genuine Less evaluation errors (including the zero-argument Argument error) propagate; update the catch in style() to only swallow and return undefined for that specific args shape and rethrow for anything else, keeping references to style and styleExpression to locate the change.packages/less/lib/less/tree/ruleset.js (1)
689-708:⚠️ Potential issue | 🟠 MajorThe single-selector branch in
createParenthesis()is inverted.Line 694 checks
elementsToPak.length === 0and then immediately readselementsToPak[0]. The call at Line 950 passes a one-element array, so the directnew Paren(selector)path never runs and single nested selectors are wrapped differently from the original AST.💡 Proposed fix
- if (elementsToPak.length === 0) { + if (elementsToPak.length === 1) { replacementParen = new Paren(elementsToPak[0]); } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/less/lib/less/tree/ruleset.js` around lines 689 - 708, The branch in createParenthesis wrongly treats an empty array as the single-selector case; change the condition to check for a single element (elementsToPak.length === 1) so that when there is exactly one element you do replacementParen = new Paren(elementsToPak[0]), otherwise build insideParent / Selector from elementsToPak as currently implemented; locate this in the createParenthesis function using the symbols elementsToPak, replacementParen, originalElement, Paren, Selector, and Element.packages/less/lib/less/tree/property.js (1)
41-79:⚠️ Potential issue | 🟠 MajorUse the node accessors here and clear
evaluatingon every exit.
Line 77/Line 78still readthis.currentFileInfo.filenameandthis.index, but this class now stores_fileInfo/_indexand the earlier throw path already usesfileInfo()/getIndex(). That means an undefined property reference can raise a secondary error instead of the intendedNameerror, and becausethis.evaluatingis only reset on the success path, any throw betweenLine 41andLine 79leaves the node stuck in the recursive state on the next eval.Proposed fix
this.evaluating = true; - - property = this.find(context.frames, function (/** `@type` {Node} */ frame) { - let v; - const vArr = /** `@type` {Ruleset} */ (frame).property(name); - if (vArr) { - for (let i = 0; i < vArr.length; i++) { - v = vArr[i]; - - vArr[i] = new Declaration(v.name, - v.value, - v.important, - v.merge, - v.index, - v.currentFileInfo, - v.inline, - v.variable - ); - } - mergeRules(vArr); - - v = vArr[vArr.length - 1]; - if (v.important) { - const importantScope = context.importantScope[context.importantScope.length - 1]; - importantScope.important = v.important; - } - v = v.value.eval(context); - return v; - } - }); - if (property) { - this.evaluating = false; - return property; - } else { - throw { type: 'Name', - message: `Property '${name}' is undefined`, - filename: this.currentFileInfo.filename, - index: this.index }; - } + try { + property = this.find(context.frames, function (/** `@type` {Node} */ frame) { + let v; + const vArr = /** `@type` {Ruleset} */ (frame).property(name); + if (vArr) { + for (let i = 0; i < vArr.length; i++) { + v = vArr[i]; + + vArr[i] = new Declaration(v.name, + v.value, + v.important, + v.merge, + v.index, + v.currentFileInfo, + v.inline, + v.variable + ); + } + mergeRules(vArr); + + v = vArr[vArr.length - 1]; + if (v.important) { + const importantScope = context.importantScope[context.importantScope.length - 1]; + importantScope.important = v.important; + } + v = v.value.eval(context); + return v; + } + }); + } finally { + this.evaluating = false; + } + if (property) { + return property; + } + throw { type: 'Name', + message: `Property '${name}' is undefined`, + filename: this.fileInfo().filename, + index: this.getIndex() };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/less/lib/less/tree/property.js` around lines 41 - 79, The node leaves this.evaluating true and uses legacy fields on error paths; update the method (the Property node's eval logic) to always clear this.evaluating on every exit (use a try { ... } finally { this.evaluating = false; }) and replace references to this.currentFileInfo.filename and this.index in the thrown Name error with the node accessors fileInfo().filename and getIndex() (or fileInfo() / getIndex()) so the thrown error uses the new _fileInfo/_index accessors.packages/less/lib/less/tree/call.js (1)
71-109:⚠️ Potential issue | 🟠 MajorRestore calc state in a single
finally.
Line 74exits calc/math mode before the fallback path is decided, while exceptions fromfuncCaller.call()andthis.args.map(...)skip cleanup entirely. Inside nestedcalc(...)evaluation, that can leave the caller with the wrong calc/math state or unwind it too far before the outer call finishes.Proposed fix
- if (funcCaller.isValid()) { - try { - result = funcCaller.call(this.args); - exitCalc(); - } catch (e) { - // eslint-disable-next-line no-prototype-builtins - if (/** `@type` {Record<string, unknown>} */ (e).hasOwnProperty('line') && /** `@type` {Record<string, unknown>} */ (e).hasOwnProperty('column')) { - throw e; - } - throw { - type: /** `@type` {Record<string, string>} */ (e).type || 'Runtime', - message: `Error evaluating function \`${this.name}\`${/** `@type` {Error} */ (e).message ? `: ${/** `@type` {Error} */ (e).message}` : ''}`, - index: this.getIndex(), - filename: this.fileInfo().filename, - line: /** `@type` {Record<string, number>} */ (e).lineNumber, - column: /** `@type` {Record<string, number>} */ (e).columnNumber - }; - } - } - - if (result !== null && result !== undefined) { - // Results that that are not nodes are cast as Anonymous nodes - // Falsy values or booleans are returned as empty nodes - if (!(result instanceof Node)) { - if (!result || result === true) { - result = new Anonymous(null); - } - else { - result = new Anonymous(result.toString()); - } - - } - result._index = this._index; - result._fileInfo = this._fileInfo; - return result; - } - - const args = this.args.map(a => a.eval(context)); - exitCalc(); - - return new Call(this.name, args, this.getIndex(), this.fileInfo()); + try { + if (funcCaller.isValid()) { + try { + result = funcCaller.call(this.args); + } catch (e) { + // eslint-disable-next-line no-prototype-builtins + if (/** `@type` {Record<string, unknown>} */ (e).hasOwnProperty('line') && /** `@type` {Record<string, unknown>} */ (e).hasOwnProperty('column')) { + throw e; + } + throw { + type: /** `@type` {Record<string, string>} */ (e).type || 'Runtime', + message: `Error evaluating function \`${this.name}\`${/** `@type` {Error} */ (e).message ? `: ${/** `@type` {Error} */ (e).message}` : ''}`, + index: this.getIndex(), + filename: this.fileInfo().filename, + line: /** `@type` {Record<string, number>} */ (e).lineNumber, + column: /** `@type` {Record<string, number>} */ (e).columnNumber + }; + } + } + + if (result !== null && result !== undefined) { + // Results that that are not nodes are cast as Anonymous nodes + // Falsy values or booleans are returned as empty nodes + if (!(result instanceof Node)) { + if (!result || result === true) { + result = new Anonymous(null); + } + else { + result = new Anonymous(result.toString()); + } + + } + result._index = this._index; + result._fileInfo = this._fileInfo; + return result; + } + + const args = this.args.map(a => a.eval(context)); + return new Call(this.name, args, this.getIndex(), this.fileInfo()); + } finally { + exitCalc(); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/less/lib/less/tree/call.js` around lines 71 - 109, The calc/math state must be restored in a single finally so exitCalc() always runs; wrap the logic that may early-return or throw (the funcCaller.isValid() branch including the funcCaller.call(this.args) call and the later fallback this.args.map(a => a.eval(context))) in a try { ... } finally { exitCalc(); } and remove the existing exitCalc() calls inside the try and before args evaluation so exitCalc() is invoked exactly once regardless of success, return, or exception; keep the existing behavior around converting non-Node results to Anonymous and preserving _index/_fileInfo.
🧹 Nitpick comments (8)
packages/less/lib/less/tree/value.js (1)
8-20: Minor: Error message doesn't match the accepted parameter type.The JSDoc indicates
valuecan beNode[] | Node, but the error message says "Value requires an array argument". Consider updating the message to reflect that a singleNodeis also accepted.💡 Suggested message update
if (!value) { - throw new Error('Value requires an array argument'); + throw new Error('Value requires a value argument'); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/less/lib/less/tree/value.js` around lines 8 - 20, The constructor for Value currently throws "Value requires an array argument" when value is falsy; update the thrown Error message in the Value class constructor to accurately reflect the accepted parameter types (Node or Node[]) — e.g., mention that a Node or array of Nodes is required — so the message aligns with the JSDoc and the constructor's handling of single Node values.packages/less/benchmark/index.js (1)
29-37: Theless.parsecallback signature is correct. The callback receives four parameters(err, root, imports, options)as shown in line 82 ofpackages/less/lib/less/parse.js, and theParseTreeconstructor correctly acceptsrootandimports.Consider wrapping the
ParseTreeconstruction andtoCSScall in a try-catch block to handle rendering errors gracefully during benchmarking, since any exception thrown bytoCSSwould otherwise propagate uncaught.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/less/benchmark/index.js` around lines 29 - 37, The less.parse callback currently calls new less.ParseTree(root, imports) and tree.toCSS(options) without protection; wrap the ParseTree construction and the tree.toCSS(options) call in a try-catch inside the less.parse callback to catch rendering exceptions, call console.error with the caught error and a short context message, and exit or set a non-zero status (e.g., process.exit(3) or similar) so the benchmark handles rendering errors gracefully instead of letting exceptions propagate.packages/less/lib/less/tree/quoted.js (1)
58-83: Use the realreplace()callback shape here.These patterns each have one capture group. At Line 64 and Line 75, the next argument from
String.prototype.replace()would be the numeric offset, not a second captured name, so the current JSDoc and?? name2fallback are misleading.Suggested cleanup
/** * `@param` {string} _ * `@param` {string} name1 - * `@param` {string} name2 * `@returns` {string} */ - const variableReplacement = function (_, name1, name2) { - const v = new Variable(`@${name1 ?? name2}`, that.getIndex(), that.fileInfo()).eval(context); + const variableReplacement = function (_, name1) { + const v = new Variable(`@${name1}`, that.getIndex(), that.fileInfo()).eval(context); return (v instanceof Quoted) ? /** `@type` {string} */ (v.value) : v.toCSS(context); }; /** * `@param` {string} _ * `@param` {string} name1 - * `@param` {string} name2 * `@returns` {string} */ - const propertyReplacement = function (_, name1, name2) { - const v = new Property(`$${name1 ?? name2}`, that.getIndex(), that.fileInfo()).eval(context); + const propertyReplacement = function (_, name1) { + const v = new Property(`$${name1}`, that.getIndex(), that.fileInfo()).eval(context); return (v instanceof Quoted) ? /** `@type` {string} */ (v.value) : v.toCSS(context); }; /** * `@param` {string} value * `@param` {RegExp} regexp - * `@param` {(substring: string, ...args: string[]) => string} replacementFnc + * `@param` {(substring: string, name: string) => string} replacementFnc * `@returns` {string} */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/less/lib/less/tree/quoted.js` around lines 58 - 83, The replace callback signatures are wrong: String.prototype.replace passes (match, capture1, offset, string...), so there is no second captured name; update variableReplacement and propertyReplacement to accept (match, name) (remove name2 and the "?? name2" fallback), update their JSDoc to reflect a single captured group (e.g., `@param` {string} match, `@param` {string} name), and use that single name when constructing new Variable(`@${name}`...) and new Property(`$${name}`...); keep the rest of the logic (instanceof Quoted check and toCSS) unchanged.packages/less/lib/less/tree/expression.js (1)
55-72: Prefer structural narrow types overExpressionassertions here.
value[0]andreturnValueare not guaranteed to beExpressioninstances on this path —returnValuecan be aDimension, andvalue[0]only needs a couple of optional flags. Using small structural types keeps@ts-checkhonest instead of asserting a narrower runtime shape.♻️ Suggested typing cleanup
- const first = /** `@type` {Expression} */ (value[0]); + const first = /** `@type` {{ parens?: boolean, parensInOp?: boolean }} */ (value[0]); if (first.parens && !first.parensInOp && !context.inCalc) { doubleParen = true; } returnValue = value[0].eval(context); - /** `@type` {Expression} */ (returnValue).noSpacing = - /** `@type` {Expression} */ (returnValue).noSpacing || noSpacing; + /** `@type` {{ noSpacing?: boolean }} */ (returnValue).noSpacing = + /** `@type` {{ noSpacing?: boolean }} */ (returnValue).noSpacing || noSpacing;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/less/lib/less/tree/expression.js` around lines 55 - 72, The code currently asserts value[0] and returnValue as Expression which is too narrow (returnValue can be a Dimension and value[0] only needs a few optional flags and eval), so remove the blanket /** `@type` {Expression} */ casts and introduce small structural JSDoc types (e.g. a MinimalExpr with optional properties parens, parensInOp, noSpacing and an eval(context) method, and a union type for Returnable = MinimalExpr | Dimension). Replace the two Expression assertions around value[0] and returnValue with casts to these small structural types (or use inline object-type JSDoc annotations) and update the final noSpacing assignment to safely access noSpacing on the union (using the structural type) so `@ts-check` validates the shape without over-asserting Expression.packages/less/lib/less/tree/variable.js (1)
2-5: Use a type-only JSDoc import forRuleset.
Rulesetis only used in the cast on Line 45, so this real ESM import adds a runtime module edge to an otherwise annotation-only change. Please switch it to a JSDoc type alias/import unlessruleset.jsis intentionally needed at runtime.💡 Suggested fix
/** `@import` { EvalContext, FileInfo } from './node.js' */ +/** `@typedef` {import('./ruleset.js').default} Ruleset */ import Node from './node.js'; import Call from './call.js'; -import Ruleset from './ruleset.js';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/less/lib/less/tree/variable.js` around lines 2 - 5, The import of Ruleset in variable.js is a runtime ESM import but it's only used as a type in the cast (the cast referencing Ruleset), so remove the runtime import of Ruleset and replace it with a JSDoc type-only import/typedef (e.g. a typedef that imports Ruleset via import('./ruleset.js').Ruleset) so the cast can keep using the Ruleset identifier without creating a runtime module edge; keep the existing cast as-is and remove the "import Ruleset from './ruleset.js';" statement.packages/less/lib/less/tree/atrule.js (1)
49-49: Useconstinstead ofvarfor consistency.The rest of the file uses
let/constdeclarations. Thisvarshould be updated for consistency.- var selectors = (new Selector([], null, null, index, currentFileInfo)).createEmptySelectors(); + const selectors = (new Selector([], null, null, index, currentFileInfo)).createEmptySelectors();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/less/lib/less/tree/atrule.js` at line 49, Replace the function-scoped var declaration with a block-scoped const to match the rest of the file: change the declaration of selectors (the result of (new Selector([], null, null, index, currentFileInfo)).createEmptySelectors()) from var to const so it uses const and preserves immutability while keeping references to Selector and createEmptySelectors unchanged.packages/less/lib/less/tree/unit.js (1)
71-73: Consider extracting the regex to a module-level constant.The
RegExpis recreated on everyisLength()call. While this is a minor performance concern, extracting it as a module-level constant would be slightly more efficient.+const LENGTH_UNITS_REGEX = /^(px|em|ex|ch|rem|in|cm|mm|pc|pt|ex|vw|vh|vmin|vmax)$/i; + class Unit extends Node { // ... isLength() { - return RegExp('^(px|em|ex|ch|rem|in|cm|mm|pc|pt|ex|vw|vh|vmin|vmax)$', 'gi').test(this.toCSS(/** `@type` {import('./node.js').EvalContext} */ ({}))); + return LENGTH_UNITS_REGEX.test(this.toCSS(/** `@type` {import('./node.js').EvalContext} */ ({}))); }Note: The
giflags includeg(global) which is unnecessary for.test()with a single match, andialone suffices for case-insensitivity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/less/lib/less/tree/unit.js` around lines 71 - 73, The isLength() method recreates the RegExp on each call and uses an unnecessary 'g' flag; extract the pattern to a module-level constant (e.g., LENGTH_UNITS) using a single case-insensitive regex (no 'g') and replace the inline RegExp call in isLength() with LENGTH_UNITS.test(this.toCSS(/** `@type` {import('./node.js').EvalContext} */ ({}))); this moves the regex allocation out of the hot path and removes the redundant global flag.packages/less/lib/less/tree/js-eval-node.js (1)
66-71: CastingundefinedtoEvalContextmay mask runtime issues.The
toCSSmethod typically expects a valid context object. While this cast silences the type checker, passingundefinedcould cause runtime issues iftoCSSimplementations access context properties without null checks.Consider using an empty object instead if a minimal context is acceptable:
- return `[${obj.value.map(function (v) { return v.toCSS(/** `@type` {EvalContext} */ (undefined)); }).join(', ')}]`; + return `[${obj.value.map(function (v) { return v.toCSS(/** `@type` {EvalContext} */ ({})); }).join(', ')}]`; } else { - return obj.toCSS(/** `@type` {EvalContext} */ (undefined)); + return obj.toCSS(/** `@type` {EvalContext} */ ({}));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/less/lib/less/tree/js-eval-node.js` around lines 66 - 71, The code is casting undefined to EvalContext when calling obj.toCSS inside the array map and fallback, which can hide runtime errors; update the calls to pass a minimal context object instead of undefined (e.g., an empty object typed as EvalContext or a shared minimalEvalContext) so toCSS receives a real context; modify the two places where toCSS is invoked (the map callback using v.toCSS(...) and the fallback obj.toCSS(...)) to use that minimal context and ensure its shape satisfies any required properties of EvalContext.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/less/lib/less/tree/atrule.js`:
- Line 202: The forEach callback currently uses an expression that returns a
value (rule.merge = false), which Biome flags; change the callback on
rules.forEach to use an explicit block-style function body so it performs the
assignment without returning a value (i.e. replace the concise arrow expression
with a block that sets rule.merge = false); locate the invocation at
rules.forEach(/** `@param` {RulesetLikeNode} rule */ rule => rule.merge = false)
and update it so the callback is a statement-only block to avoid unintended
returns.
In `@packages/less/lib/less/tree/color.js`:
- Around line 18-53: The constructor currently uses "this.alpha = this.alpha ||
(typeof a === 'number' ? a : 1);" which overwrites a parsed alpha of 0; change
the logic in the Color constructor to only assign the default alpha when
this.alpha is undefined (i.e., check typeof this.alpha === 'undefined') and
otherwise keep the parsed value, falling back to the passed parameter a if
provided; refer to the Color constructor, this.alpha, and parameter a when
making the change.
In `@packages/less/lib/less/tree/import.js`:
- Around line 182-183: The evaluated features node (const features =
this.features && this.features.eval(context)) is computed but later branches
still check this.features.value, causing layer(...) to be missed after
evaluation; update the isLayer checks and any subsequent inspections in
Import.prototype methods (the branches handling layer-import vs media-import,
e.g. the blocks referenced around lines 182, 216-229, 255-288) to use the
already-evaluated features variable instead of this.features.value so the
decision uses the post-evaluation AST node; locate uses of this.features.value
in those methods and replace them with checks against features (and adjust
null/undefined handling accordingly).
In `@packages/less/lib/less/tree/mixin-call.js`:
- Around line 226-229: The catch block currently recreates an error object
losing any extra properties from the original Less error (e.g., type); preserve
nested error metadata by merging the original error object (err) into the thrown
object while still setting/overriding message, index (this.getIndex()), filename
(this.fileInfo().filename) and stack; locate the catch in mixin-call.js around
evalCall() where err is defined and change the throw to include the full err
(spread or equivalent) so nested fields like type are retained.
In `@packages/less/lib/less/tree/nested-at-rule.js`:
- Around line 21-37: The typedef NestableAtRuleThis declares evalNested as
returning Ruleset but the implementation can return self (a Node subtype);
update the typedef entry evalNested: (context: EvalContext) => Ruleset to
evalNested: (context: EvalContext) => Node | Ruleset so the signature matches
possible return values (adjust the union type on the evalNested property in the
NestableAtRuleThis typedef).
In `@packages/less/lib/less/tree/ruleset.js`:
- Around line 146-153: The callback passed to Parser.prototype.parseNode
currently ignores the err parameter, causing invalid interpolated selectors to
be silently kept; update the callback used in the new Parser(...).parseNode(...)
call to check for err and throw/propagate it (e.g. if (err) throw err;) before
assigning selectors, mirroring the error handling used in Selector parsing; keep
the existing assignment selectors = utils.flattenArray(result) only when err is
null and result is present.
In `@packages/less/lib/less/tree/selector.js`:
- Around line 76-86: The callback passed to Parser.parseNode inside
getElements() uses a plain function and references this._fileInfo, causing this
to be undefined on callback invocation; fix it by capturing this._fileInfo into
a local variable (e.g. const fileInfo = this._fileInfo) before the parseNode
call and replace the plain function callback with an arrow function (or
otherwise bind the callback) so the error path uses the captured fileInfo when
constructing the LessError; update the callback passed to Parser.parseNode in
getElements() accordingly.
---
Outside diff comments:
In `@packages/less/lib/less/functions/style.js`:
- Around line 20-27: The current try/catch in style() swallows all errors from
styleExpression; narrow it so only the CSS-function case is ignored: inspect the
received args in style(...args) and if they look like a raw CSS function
invocation (e.g., a single Anonymous/Quoted node or an arg whose .type indicates
it’s not a parsed Less expression) return undefined, otherwise rethrow the
caught error so genuine Less evaluation errors (including the zero-argument
Argument error) propagate; update the catch in style() to only swallow and
return undefined for that specific args shape and rethrow for anything else,
keeping references to style and styleExpression to locate the change.
In `@packages/less/lib/less/tree/call.js`:
- Around line 71-109: The calc/math state must be restored in a single finally
so exitCalc() always runs; wrap the logic that may early-return or throw (the
funcCaller.isValid() branch including the funcCaller.call(this.args) call and
the later fallback this.args.map(a => a.eval(context))) in a try { ... } finally
{ exitCalc(); } and remove the existing exitCalc() calls inside the try and
before args evaluation so exitCalc() is invoked exactly once regardless of
success, return, or exception; keep the existing behavior around converting
non-Node results to Anonymous and preserving _index/_fileInfo.
In `@packages/less/lib/less/tree/property.js`:
- Around line 41-79: The node leaves this.evaluating true and uses legacy fields
on error paths; update the method (the Property node's eval logic) to always
clear this.evaluating on every exit (use a try { ... } finally { this.evaluating
= false; }) and replace references to this.currentFileInfo.filename and
this.index in the thrown Name error with the node accessors fileInfo().filename
and getIndex() (or fileInfo() / getIndex()) so the thrown error uses the new
_fileInfo/_index accessors.
In `@packages/less/lib/less/tree/ruleset.js`:
- Around line 689-708: The branch in createParenthesis wrongly treats an empty
array as the single-selector case; change the condition to check for a single
element (elementsToPak.length === 1) so that when there is exactly one element
you do replacementParen = new Paren(elementsToPak[0]), otherwise build
insideParent / Selector from elementsToPak as currently implemented; locate this
in the createParenthesis function using the symbols elementsToPak,
replacementParen, originalElement, Paren, Selector, and Element.
---
Nitpick comments:
In `@packages/less/benchmark/index.js`:
- Around line 29-37: The less.parse callback currently calls new
less.ParseTree(root, imports) and tree.toCSS(options) without protection; wrap
the ParseTree construction and the tree.toCSS(options) call in a try-catch
inside the less.parse callback to catch rendering exceptions, call console.error
with the caught error and a short context message, and exit or set a non-zero
status (e.g., process.exit(3) or similar) so the benchmark handles rendering
errors gracefully instead of letting exceptions propagate.
In `@packages/less/lib/less/tree/atrule.js`:
- Line 49: Replace the function-scoped var declaration with a block-scoped const
to match the rest of the file: change the declaration of selectors (the result
of (new Selector([], null, null, index,
currentFileInfo)).createEmptySelectors()) from var to const so it uses const and
preserves immutability while keeping references to Selector and
createEmptySelectors unchanged.
In `@packages/less/lib/less/tree/expression.js`:
- Around line 55-72: The code currently asserts value[0] and returnValue as
Expression which is too narrow (returnValue can be a Dimension and value[0] only
needs a few optional flags and eval), so remove the blanket /** `@type`
{Expression} */ casts and introduce small structural JSDoc types (e.g. a
MinimalExpr with optional properties parens, parensInOp, noSpacing and an
eval(context) method, and a union type for Returnable = MinimalExpr |
Dimension). Replace the two Expression assertions around value[0] and
returnValue with casts to these small structural types (or use inline
object-type JSDoc annotations) and update the final noSpacing assignment to
safely access noSpacing on the union (using the structural type) so `@ts-check`
validates the shape without over-asserting Expression.
In `@packages/less/lib/less/tree/js-eval-node.js`:
- Around line 66-71: The code is casting undefined to EvalContext when calling
obj.toCSS inside the array map and fallback, which can hide runtime errors;
update the calls to pass a minimal context object instead of undefined (e.g., an
empty object typed as EvalContext or a shared minimalEvalContext) so toCSS
receives a real context; modify the two places where toCSS is invoked (the map
callback using v.toCSS(...) and the fallback obj.toCSS(...)) to use that minimal
context and ensure its shape satisfies any required properties of EvalContext.
In `@packages/less/lib/less/tree/quoted.js`:
- Around line 58-83: The replace callback signatures are wrong:
String.prototype.replace passes (match, capture1, offset, string...), so there
is no second captured name; update variableReplacement and propertyReplacement
to accept (match, name) (remove name2 and the "?? name2" fallback), update their
JSDoc to reflect a single captured group (e.g., `@param` {string} match, `@param`
{string} name), and use that single name when constructing new
Variable(`@${name}`...) and new Property(`$${name}`...); keep the rest of the
logic (instanceof Quoted check and toCSS) unchanged.
In `@packages/less/lib/less/tree/unit.js`:
- Around line 71-73: The isLength() method recreates the RegExp on each call and
uses an unnecessary 'g' flag; extract the pattern to a module-level constant
(e.g., LENGTH_UNITS) using a single case-insensitive regex (no 'g') and replace
the inline RegExp call in isLength() with LENGTH_UNITS.test(this.toCSS(/** `@type`
{import('./node.js').EvalContext} */ ({}))); this moves the regex allocation out
of the hot path and removes the redundant global flag.
In `@packages/less/lib/less/tree/value.js`:
- Around line 8-20: The constructor for Value currently throws "Value requires
an array argument" when value is falsy; update the thrown Error message in the
Value class constructor to accurately reflect the accepted parameter types (Node
or Node[]) — e.g., mention that a Node or array of Nodes is required — so the
message aligns with the JSDoc and the constructor's handling of single Node
values.
In `@packages/less/lib/less/tree/variable.js`:
- Around line 2-5: The import of Ruleset in variable.js is a runtime ESM import
but it's only used as a type in the cast (the cast referencing Ruleset), so
remove the runtime import of Ruleset and replace it with a JSDoc type-only
import/typedef (e.g. a typedef that imports Ruleset via
import('./ruleset.js').Ruleset) so the cast can keep using the Ruleset
identifier without creating a runtime module edge; keep the existing cast as-is
and remove the "import Ruleset from './ruleset.js';" statement.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3f0e3012-372f-4696-b174-0c639140899a
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (56)
.husky/pre-commitpackages/less/benchmark/index.jspackages/less/lib/less-browser/add-default-options.jspackages/less/lib/less-browser/utils.jspackages/less/lib/less-node/fs.jspackages/less/lib/less/deprecation.jspackages/less/lib/less/functions/style.jspackages/less/lib/less/index.jspackages/less/lib/less/parser/parser.jspackages/less/lib/less/transform-tree.jspackages/less/lib/less/tree/anonymous.jspackages/less/lib/less/tree/assignment.jspackages/less/lib/less/tree/atrule-syntax.jspackages/less/lib/less/tree/atrule.jspackages/less/lib/less/tree/attribute.jspackages/less/lib/less/tree/call.jspackages/less/lib/less/tree/color.jspackages/less/lib/less/tree/combinator.jspackages/less/lib/less/tree/comment.jspackages/less/lib/less/tree/condition.jspackages/less/lib/less/tree/container.jspackages/less/lib/less/tree/debug-info.jspackages/less/lib/less/tree/declaration.jspackages/less/lib/less/tree/detached-ruleset.jspackages/less/lib/less/tree/dimension.jspackages/less/lib/less/tree/element.jspackages/less/lib/less/tree/expression.jspackages/less/lib/less/tree/extend.jspackages/less/lib/less/tree/import.jspackages/less/lib/less/tree/index.jspackages/less/lib/less/tree/javascript.jspackages/less/lib/less/tree/js-eval-node.jspackages/less/lib/less/tree/keyword.jspackages/less/lib/less/tree/media.jspackages/less/lib/less/tree/merge-rules.jspackages/less/lib/less/tree/mixin-call.jspackages/less/lib/less/tree/mixin-definition.jspackages/less/lib/less/tree/namespace-value.jspackages/less/lib/less/tree/negative.jspackages/less/lib/less/tree/nested-at-rule.jspackages/less/lib/less/tree/node.jspackages/less/lib/less/tree/operation.jspackages/less/lib/less/tree/paren.jspackages/less/lib/less/tree/property.jspackages/less/lib/less/tree/query-in-parens.jspackages/less/lib/less/tree/quoted.jspackages/less/lib/less/tree/ruleset.jspackages/less/lib/less/tree/selector.jspackages/less/lib/less/tree/unicode-descriptor.jspackages/less/lib/less/tree/unit.jspackages/less/lib/less/tree/url.jspackages/less/lib/less/tree/value.jspackages/less/lib/less/tree/variable-call.jspackages/less/lib/less/tree/variable.jspackages/less/lib/less/visitors/set-tree-visibility-visitor.jspackages/less/package.json
matthew-dean
force-pushed
the
feat/jsdoc-classes
branch
2 times, most recently
from
ffa45da to
7760c01
Compare
Add proper JSDoc type annotations to all 44 files in lib/less/tree/, enabling per-file TypeScript checking via @ts-check. No {*} or {any} casts — all types are derived from reading the actual code. Key changes: - Shared types (EvalContext, CSSOutput, TreeVisitor, FileInfo, VisibilityInfo) defined in node.js - Node.value typed as union: Node | Node[] | string | number | undefined - Node.prototype.parse declared for parser-injected prototype property - Constructor properties explicitly declared with proper types - Inline casts used to narrow union types at usage sites - Widened base class params where subclasses pass different types Also adds typecheck to prepublishOnly and pre-commit hook to catch regressions as more files are annotated toward global checkJs: true. All 139 tests pass, zero TypeScript errors.
matthew-dean
force-pushed
the
feat/jsdoc-classes
branch
from
7760c01 to
d0eb430
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/less/lib/less/tree/variable.js (1)
42-68:⚠️ Potential issue | 🟡 MinorReset
this.evaluatingin afinally.Any exception from
v.value.eval(context)/Call.eval(context)or the undefined-variable branch leaves this node permanently marked as evaluating. Reusing the same AST node after that will false-positive as a recursive definition.🛠️ Suggested fix
this.evaluating = true; - - variable = this.find(context.frames, function (frame) { - const v = /** `@type` {Ruleset} */ (frame).variable(name); - if (v) { - if (v.important) { - const importantScope = context.importantScope[context.importantScope.length - 1]; - importantScope.important = v.important; - } - // If in calc, wrap vars in a function call to cascade evaluate args first - if (context.inCalc) { - return (new Call('_SELF', [v.value], 0, undefined)).eval(context); - } - else { - return v.value.eval(context); - } - } - }); - if (variable) { - this.evaluating = false; - return variable; - } else { - throw { type: 'Name', - message: `variable ${name} is undefined`, - filename: this.fileInfo().filename, - index: this.getIndex() }; - } + try { + variable = this.find(context.frames, function (frame) { + const v = /** `@type` {Ruleset} */ (frame).variable(name); + if (v) { + if (v.important) { + const importantScope = context.importantScope[context.importantScope.length - 1]; + importantScope.important = v.important; + } + // If in calc, wrap vars in a function call to cascade evaluate args first + if (context.inCalc) { + return (new Call('_SELF', [v.value], 0, undefined)).eval(context); + } + return v.value.eval(context); + } + }); + if (variable) { + return variable; + } + throw { type: 'Name', + message: `variable ${name} is undefined`, + filename: this.fileInfo().filename, + index: this.getIndex() }; + } finally { + this.evaluating = false; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/less/lib/less/tree/variable.js` around lines 42 - 68, The code sets this.evaluating = true but only clears it on the success path, so exceptions from v.value.eval(context), Call(...).eval(context), or the throw branch leave the node marked evaluating; wrap the variable lookup and evaluation logic (the find callback and subsequent return/throw) in a try/finally (or ensure a finally-like cleanup) so that this.evaluating is always set back to false, referencing the this.evaluating flag, the find invocation that calls v.value.eval(context) and Call('_SELF', [v.value], 0, undefined).eval(context), and ensure the finally executes before any throw to avoid spurious recursive detection.packages/less/lib/less/tree/ruleset.js (1)
689-710:⚠️ Potential issue | 🟠 MajorFix off-by-one error: condition should check
length === 1, notlength === 0.When
elementsToPak.length === 0, line 695 accesseselementsToPak[0], which is undefined. The condition should be=== 1to handle the single-element case, where that element is passed directly to theParenconstructor. Theelsebranch handles multiple elements by wrapping each in anElementandSelectorstructure.Proposed fix
- if (elementsToPak.length === 0) { + if (elementsToPak.length === 1) { replacementParen = new Paren(elementsToPak[0]); } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/less/lib/less/tree/ruleset.js` around lines 689 - 710, The createParenthesis function has an off-by-one: it checks elementsToPak.length === 0 but then uses elementsToPak[0]; change the condition to elementsToPak.length === 1 so the single-element path constructs replacementParen = new Paren(elementsToPak[0]) correctly, while the else branch continues to wrap multiple items into Element and Selector; update the conditional that currently references elementsToPak.length to === 1 and ensure no other logic assumes a zero-length array there.
♻️ Duplicate comments (4)
packages/less/lib/less/tree/nested-at-rule.js (1)
21-37:⚠️ Potential issue | 🟡 Minor
evalNestedstill advertises the wrong return type.Line 130 returns
self, soNestableAtRuleThis.evalNestedcannot beRuleset-only. Becausepackages/less/lib/less/tree/atrule.js:332reuses this prototype directly, the bad signature leaks beyond this helper.🛠️ Type fix
- * evalNested: (context: EvalContext) => Ruleset, + * evalNested: (context: EvalContext) => Node | Ruleset,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/less/lib/less/tree/nested-at-rule.js` around lines 21 - 37, The JSDoc typedef for NestableAtRuleThis incorrectly declares evalNested as returning only Ruleset while the implementation (and reuse in atrule.js) can return self (a Node) as well; update the typedef in nested-at-rule.js so evalNested: (context: EvalContext) => Node | Ruleset (or the correct union type used elsewhere) to match actual return values and prevent the incorrect narrow signature from leaking to atrule.js and other consumers.packages/less/lib/less/tree/color.js (1)
51-52:⚠️ Potential issue | 🟠 MajorDon't overwrite parsed alpha
0with the default.Line 52 uses
||, so#0000or#00000000parse withalpha = 0at lines 37/47, but then it's immediately replaced with1because0is falsy. This transforms fully transparent hex colors into opaque black.Suggested fix
/** `@type` {number} */ - this.alpha = this.alpha || (typeof a === 'number' ? a : 1); + this.alpha = (this.alpha !== undefined) ? this.alpha : (typeof a === 'number' ? a : 1);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/less/lib/less/tree/color.js` around lines 51 - 52, The alpha assignment in the Color constructor uses a falsy check and overwrites a parsed 0 (fully transparent) with 1; replace the falsy `||` logic on `this.alpha` with an explicit undefined/null check (e.g., use nullish coalescing or test for undefined) so that a parsed numeric 0 is preserved while still defaulting to `a` or 1 when alpha is truly missing; update the line that sets `this.alpha` in color.js (the assignment using `this.alpha || (typeof a === 'number' ? a : 1)`) to use the explicit check.packages/less/lib/less/tree/atrule.js (1)
202-202:⚠️ Potential issue | 🟡 MinorUse block syntax in
forEachcallback to avoid unintended return.The assignment expression
rule.merge = falsereturnsfalse, which Biome flags as an unintended return from aforEachcallback.Suggested fix
- rules.forEach(/** `@param` {RulesetLikeNode} rule */ rule => rule.merge = false); + rules.forEach(/** `@param` {RulesetLikeNode} rule */ rule => { rule.merge = false; });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/less/lib/less/tree/atrule.js` at line 202, The forEach callback currently returns the assignment expression (rules.forEach(rule => rule.merge = false)), which Biome flags as an unintended return; update the callback to use block syntax so it performs the assignment without returning a value (e.g., change the arrow callback to a block-body that sets rule.merge = false) — locate the rules.forEach call in atrule.js where RulesetLikeNode objects are iterated and replace the concise arrow expression with a block-bodied callback that executes rule.merge = false;.packages/less/lib/less/tree/selector.js (1)
76-89:⚠️ Potential issue | 🔴 CriticalFix the callback binding in
getElements().The callback at lines 81-89 uses a plain
functionwiththis._fileInfoin the error path (line 86). WhenparseNode()invokes the callback on parse failures,thisisundefined, causing aTypeErrorinstead of reporting the actual parse error.Suggested fix
if (typeof els === 'string') { const parse = this.parse; + const fileInfo = this._fileInfo; new (/** `@type` {new (...args: unknown[]) => { parseNode: Function }} */ (/** `@type` {unknown} */ (Parser)))(parse.context, parse.importManager, this._fileInfo, this._index).parseNode( els, ['selector'], - function(/** `@type` {{ index: number, message: string } | null} */ err, /** `@type` {Selector[]} */ result) { + (/** `@type` {{ index: number, message: string } | null} */ err, /** `@type` {Selector[]} */ result) => { if (err) { throw new LessError({ index: err.index, message: err.message - }, parse.imports, /** `@type` {string} */ (/** `@type` {FileInfo} */ (this._fileInfo).filename)); + }, parse.imports, /** `@type` {string} */ (fileInfo && fileInfo.filename)); } els = result[0].elements; }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/less/lib/less/tree/selector.js` around lines 76 - 89, The callback passed to Parser.parseNode inside getElements() uses a plain function so `this` is undefined when invoked, causing a TypeError when referencing this._fileInfo; change the callback to a bound function (e.g., use an arrow function or .bind(this)) so it preserves the surrounding `this`, and ensure the error branch constructs LessError with the correct this._fileInfo filename; update the callback passed to parseNode (the anonymous function handling err and result) accordingly to reference this._fileInfo safely.
🧹 Nitpick comments (3)
packages/less/lib/less/tree/property.js (1)
5-5: ImportmergeRulesdirectly instead of reaching throughpluginManager.
packages/less/lib/less/tree/merge-rules.jsalready exports the helper this code needs. Pulling_mergeRulesoffToCSSVisitor.prototypecouplesPropertyto a private visitor implementation and tocontext.pluginManagerbeing populated in every eval context.♻️ Proposed cleanup
import Node from './node.js'; import Declaration from './declaration.js'; +import mergeRules from './merge-rules.js'; import Ruleset from './ruleset.js';- // TODO: shorten this reference - const mergeRules = /** `@type` {{ less: { visitors: { ToCSSVisitor: { prototype: { _mergeRules: (rules: Declaration[]) => void } } } } }} */ (context.pluginManager).less.visitors.ToCSSVisitor.prototype._mergeRules;Also applies to: 32-33
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/less/lib/less/tree/property.js` at line 5, Property currently reaches into a private visitor via context.pluginManager/ToCSSVisitor.prototype._mergeRules to get the merge helper; instead import the exported mergeRules directly from './merge-rules.js' and use that function in Property (and the other occurrences around lines 32-33) to avoid coupling to the private visitor and pluginManager. Replace any usage of ToCSSVisitor.prototype._mergeRules or context.pluginManager._mergeRules with the imported mergeRules and ensure the import is added at the top of the file.packages/less/lib/less/tree/color.js (1)
33-39: PreferforEachovermapwhen not using return values.Using
map()for side effects (populatingthis.rgbandthis.alpha) without returning values triggers a lint warning. Since the mapped values aren't used,forEachis semantically correct.Suggested fix
- /** `@type` {RegExpMatchArray} */ (rgb.match(/.{2}/g)).map(function (c, i) { + /** `@type` {RegExpMatchArray} */ (rgb.match(/.{2}/g)).forEach(function (c, i) { if (i < 3) { self.rgb.push(parseInt(c, 16)); } else { self.alpha = (parseInt(c, 16)) / 255; } });Apply the same change to the short hex branch at lines 43-49.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/less/lib/less/tree/color.js` around lines 33 - 39, The code currently uses Array.prototype.map for side effects when parsing hex RGB: replace the use of .map on the result of rgb.match(/.{2}/g) with .forEach so you don't create an unused array; update the block that pushes into self.rgb and sets self.alpha (the function that calls parseInt(c, 16) and assigns to self.rgb/self.alpha) to use .forEach, and make the identical change in the short-hex branch that currently mirrors this logic (the branch that expands short hex parts and assigns to self.rgb/self.alpha).packages/less/lib/less/tree/ruleset.js (1)
302-305: Redundant length check can be simplified.The condition
importRules.length || importRules.length === 0is always true for any non-negative length. This appears to be checking ifimportRulesis array-like.♻️ Suggested simplification
- if (importRules && (/** `@type` {Node[]} */ (/** `@type` {unknown} */ (importRules)).length || /** `@type` {Node[]} */ (/** `@type` {unknown} */ (importRules)).length === 0)) { + if (importRules && Array.isArray(importRules)) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/less/lib/less/tree/ruleset.js` around lines 302 - 305, The conditional checking importRules is redundant — replace the expression `importRules && (importRules.length || importRules.length === 0)` with a clear array check such as `Array.isArray(importRules)` to ensure importRules is array-like before casting to Node[]; keep the subsequent code using importArr, rules.splice(i, 1, ...importArr) and i += importArr.length - 1 unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/less/lib/less/tree/call.js`:
- Around line 13-18: Update the JSDoc for the Call constructor so the
currentFileInfo parameter allows undefined: change the `@param` annotation on
currentFileInfo in Call (in packages/less/lib/less/tree/call.js) from `@param`
{FileInfo} currentFileInfo to `@param` {FileInfo | undefined} currentFileInfo to
match usages like new Call('_SELF', [v.value], 0, undefined).
In `@packages/less/lib/less/tree/container.js`:
- Line 86: The assignment unconditionally calls .inherit() on
context.frames[0].functionRegistry which can be undefined; change the code in
container.js (the line assigning this.rules[0].functionRegistry) to guard the
call — only call .inherit() if context.frames[0].functionRegistry exists,
otherwise leave functionRegistry undefined or set a safe fallback; reference the
symbols this.rules[0].functionRegistry and context.frames[0].functionRegistry to
locate and update the logic so it matches how mixin-definition.js treats
functionRegistry as optional.
In `@packages/less/lib/less/tree/declaration.js`:
- Around line 116-119: The error wrapper in Declaration evaluation assumes
fileInfo() exists and calls this.fileInfo().filename, which can throw when
currentFileInfo is undefined; update the logic in the Declaration error handling
(the block that uses getIndex() and fileInfo()) to be null-safe: only set
err.filename if this.fileInfo() returns a non-null object (or
this.currentFileInfo is present), and avoid overwriting an existing
err.filename; keep setting err.index via getIndex() as before but guard fileInfo
access to prevent a secondary TypeError.
In `@packages/less/lib/less/tree/media.js`:
- Line 72: The assignment to RulesetLikeNode.functionRegistry should be guarded
to avoid calling .inherit() on an undefined synthetic frame; check that
context.frames[0] and its functionRegistry exist before calling .inherit() and
only assign the result when present (i.e. if (context.frames[0] &&
context.frames[0].functionRegistry) this.rules[0].functionRegistry =
context.frames[0].functionRegistry.inherit()); ensure you use the same
null-check style as Container.eval to match the codebase.
In `@packages/less/lib/less/tree/nested-at-rule.js`:
- Around line 164-172: The permute method's base case returns arr[0] (a Node[])
while the signature promises Node[][]; fix by returning [arr[0]] in the
arr.length === 1 branch so the method always returns Node[][], or alternatively
if you intend a different shape, widen the return type to allow Node[] |
Node[][] and update the related typedef accordingly (prefer the first fix:
change the base case in permute to return an array containing arr[0]).
In `@packages/less/lib/less/tree/property.js`:
- Around line 15-21: The constructor for Property now sets only _index and
_fileInfo but legacy code in eval() still reads this.index and
this.currentFileInfo.filename, causing a TypeError on missing-property lookups;
update the constructor (constructor(name, index, currentFileInfo)) to preserve
compatibility by also assigning this.index = index and this.currentFileInfo =
currentFileInfo (or ensure eval() uses the new _index/_fileInfo fields and
throws the Less Name error there), so either restore the legacy fields or update
eval()’s throw site to reference the new symbols.
In `@packages/less/lib/less/tree/ruleset.js`:
- Around line 930-932: Remove the duplicated JSDoc type annotation immediately
above the resolvedElements declaration: there are two consecutive "/** `@type`
{(Element | Selector)[]} */" comments; delete one so only a single JSDoc comment
precedes the const resolvedElements = [] declaration (refer to the
resolvedElements variable in ruleset.js).
---
Outside diff comments:
In `@packages/less/lib/less/tree/ruleset.js`:
- Around line 689-710: The createParenthesis function has an off-by-one: it
checks elementsToPak.length === 0 but then uses elementsToPak[0]; change the
condition to elementsToPak.length === 1 so the single-element path constructs
replacementParen = new Paren(elementsToPak[0]) correctly, while the else branch
continues to wrap multiple items into Element and Selector; update the
conditional that currently references elementsToPak.length to === 1 and ensure
no other logic assumes a zero-length array there.
In `@packages/less/lib/less/tree/variable.js`:
- Around line 42-68: The code sets this.evaluating = true but only clears it on
the success path, so exceptions from v.value.eval(context),
Call(...).eval(context), or the throw branch leave the node marked evaluating;
wrap the variable lookup and evaluation logic (the find callback and subsequent
return/throw) in a try/finally (or ensure a finally-like cleanup) so that
this.evaluating is always set back to false, referencing the this.evaluating
flag, the find invocation that calls v.value.eval(context) and Call('_SELF',
[v.value], 0, undefined).eval(context), and ensure the finally executes before
any throw to avoid spurious recursive detection.
---
Duplicate comments:
In `@packages/less/lib/less/tree/atrule.js`:
- Line 202: The forEach callback currently returns the assignment expression
(rules.forEach(rule => rule.merge = false)), which Biome flags as an unintended
return; update the callback to use block syntax so it performs the assignment
without returning a value (e.g., change the arrow callback to a block-body that
sets rule.merge = false) — locate the rules.forEach call in atrule.js where
RulesetLikeNode objects are iterated and replace the concise arrow expression
with a block-bodied callback that executes rule.merge = false;.
In `@packages/less/lib/less/tree/color.js`:
- Around line 51-52: The alpha assignment in the Color constructor uses a falsy
check and overwrites a parsed 0 (fully transparent) with 1; replace the falsy
`||` logic on `this.alpha` with an explicit undefined/null check (e.g., use
nullish coalescing or test for undefined) so that a parsed numeric 0 is
preserved while still defaulting to `a` or 1 when alpha is truly missing; update
the line that sets `this.alpha` in color.js (the assignment using `this.alpha ||
(typeof a === 'number' ? a : 1)`) to use the explicit check.
In `@packages/less/lib/less/tree/nested-at-rule.js`:
- Around line 21-37: The JSDoc typedef for NestableAtRuleThis incorrectly
declares evalNested as returning only Ruleset while the implementation (and
reuse in atrule.js) can return self (a Node) as well; update the typedef in
nested-at-rule.js so evalNested: (context: EvalContext) => Node | Ruleset (or
the correct union type used elsewhere) to match actual return values and prevent
the incorrect narrow signature from leaking to atrule.js and other consumers.
In `@packages/less/lib/less/tree/selector.js`:
- Around line 76-89: The callback passed to Parser.parseNode inside
getElements() uses a plain function so `this` is undefined when invoked, causing
a TypeError when referencing this._fileInfo; change the callback to a bound
function (e.g., use an arrow function or .bind(this)) so it preserves the
surrounding `this`, and ensure the error branch constructs LessError with the
correct this._fileInfo filename; update the callback passed to parseNode (the
anonymous function handling err and result) accordingly to reference
this._fileInfo safely.
---
Nitpick comments:
In `@packages/less/lib/less/tree/color.js`:
- Around line 33-39: The code currently uses Array.prototype.map for side
effects when parsing hex RGB: replace the use of .map on the result of
rgb.match(/.{2}/g) with .forEach so you don't create an unused array; update the
block that pushes into self.rgb and sets self.alpha (the function that calls
parseInt(c, 16) and assigns to self.rgb/self.alpha) to use .forEach, and make
the identical change in the short-hex branch that currently mirrors this logic
(the branch that expands short hex parts and assigns to self.rgb/self.alpha).
In `@packages/less/lib/less/tree/property.js`:
- Line 5: Property currently reaches into a private visitor via
context.pluginManager/ToCSSVisitor.prototype._mergeRules to get the merge
helper; instead import the exported mergeRules directly from './merge-rules.js'
and use that function in Property (and the other occurrences around lines 32-33)
to avoid coupling to the private visitor and pluginManager. Replace any usage of
ToCSSVisitor.prototype._mergeRules or context.pluginManager._mergeRules with the
imported mergeRules and ensure the import is added at the top of the file.
In `@packages/less/lib/less/tree/ruleset.js`:
- Around line 302-305: The conditional checking importRules is redundant —
replace the expression `importRules && (importRules.length || importRules.length
=== 0)` with a clear array check such as `Array.isArray(importRules)` to ensure
importRules is array-like before casting to Node[]; keep the subsequent code
using importArr, rules.splice(i, 1, ...importArr) and i += importArr.length - 1
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a5a4b651-5cf4-4bb0-bdaf-03b7b1ab4a78
📒 Files selected for processing (46)
.husky/pre-commitpackages/less/lib/less/tree/anonymous.jspackages/less/lib/less/tree/assignment.jspackages/less/lib/less/tree/atrule-syntax.jspackages/less/lib/less/tree/atrule.jspackages/less/lib/less/tree/attribute.jspackages/less/lib/less/tree/call.jspackages/less/lib/less/tree/color.jspackages/less/lib/less/tree/combinator.jspackages/less/lib/less/tree/comment.jspackages/less/lib/less/tree/condition.jspackages/less/lib/less/tree/container.jspackages/less/lib/less/tree/debug-info.jspackages/less/lib/less/tree/declaration.jspackages/less/lib/less/tree/detached-ruleset.jspackages/less/lib/less/tree/dimension.jspackages/less/lib/less/tree/element.jspackages/less/lib/less/tree/expression.jspackages/less/lib/less/tree/extend.jspackages/less/lib/less/tree/import.jspackages/less/lib/less/tree/index.jspackages/less/lib/less/tree/javascript.jspackages/less/lib/less/tree/js-eval-node.jspackages/less/lib/less/tree/keyword.jspackages/less/lib/less/tree/media.jspackages/less/lib/less/tree/merge-rules.jspackages/less/lib/less/tree/mixin-call.jspackages/less/lib/less/tree/mixin-definition.jspackages/less/lib/less/tree/namespace-value.jspackages/less/lib/less/tree/negative.jspackages/less/lib/less/tree/nested-at-rule.jspackages/less/lib/less/tree/node.jspackages/less/lib/less/tree/operation.jspackages/less/lib/less/tree/paren.jspackages/less/lib/less/tree/property.jspackages/less/lib/less/tree/query-in-parens.jspackages/less/lib/less/tree/quoted.jspackages/less/lib/less/tree/ruleset.jspackages/less/lib/less/tree/selector.jspackages/less/lib/less/tree/unicode-descriptor.jspackages/less/lib/less/tree/unit.jspackages/less/lib/less/tree/url.jspackages/less/lib/less/tree/value.jspackages/less/lib/less/tree/variable-call.jspackages/less/lib/less/tree/variable.jspackages/less/package.json
🚧 Files skipped from review as they are similar to previous changes (13)
- packages/less/lib/less/tree/merge-rules.js
- packages/less/lib/less/tree/index.js
- packages/less/lib/less/tree/assignment.js
- packages/less/lib/less/tree/combinator.js
- packages/less/lib/less/tree/atrule-syntax.js
- packages/less/lib/less/tree/query-in-parens.js
- packages/less/package.json
- .husky/pre-commit
- packages/less/lib/less/tree/js-eval-node.js
- packages/less/lib/less/tree/unicode-descriptor.js
- packages/less/lib/less/tree/anonymous.js
- packages/less/lib/less/tree/url.js
- packages/less/lib/less/tree/operation.js
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/less/lib/less/tree/call.js (1)
54-65:⚠️ Potential issue | 🟠 MajorAlways unwind calc state in a
finallyblock.
exitCalc()is skipped iffuncCaller.call(...)or anya.eval(context)throws, socontext.mathOn/ calc nesting can leak out of this node on error paths.Suggested fix
- if (funcCaller.isValid()) { - try { - result = funcCaller.call(this.args); - exitCalc(); - } catch (e) { + try { + if (funcCaller.isValid()) { + try { + result = funcCaller.call(this.args); + } catch (e) { + // existing normalization logic + throw e; + } + } + + if (result !== null && result !== undefined) { + // existing result normalization logic + return result; + } + + const args = this.args.map(a => a.eval(context)); + return new Call(this.name, args, this.getIndex(), this.fileInfo()); + } finally { + exitCalc(); - } - } - - if (result !== null && result !== undefined) { - // existing result normalization logic - return result; } - - const args = this.args.map(a => a.eval(context)); - exitCalc(); - - return new Call(this.name, args, this.getIndex(), this.fileInfo());Also applies to: 71-109
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/less/lib/less/tree/call.js` around lines 54 - 65, The code currently toggles context.mathOn and calls context.enterCalc() but doesn't guarantee context.exitCalc() and restoration of context.mathOn when evaluation throws; wrap the evaluation sequence that calls a.eval(context) and funcCaller.call(...) in a try/finally so exit logic always runs: move the existing exitCalc() logic into a finally block and ensure it always calls context.exitCalc() when this.calc || context.inCalc and restores context.mathOn to the saved currentMathContext; apply the same try/finally pattern to the related section around lines 71-109 that performs similar enter/exit calls.packages/less/lib/less/tree/mixin-definition.js (1)
128-146:⚠️ Potential issue | 🟠 MajorDon't evaluate named arguments twice.
This branch calls
arg.value.eval(context)once forevaldArguments[j]and again for theDeclaration. That can change behavior for non-idempotent expressions and doubles the work for every named argument.🐛 Reuse the evaluated node
if (!evaldArguments[j] && name === params[j].name) { - evaldArguments[j] = arg.value.eval(context); - frame.prependRule(new Declaration(name, arg.value.eval(context))); + const evaluatedArg = arg.value.eval(context); + evaldArguments[j] = evaluatedArg; + frame.prependRule(new Declaration(name, evaluatedArg)); isNamedFound = true; break; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/less/lib/less/tree/mixin-definition.js` around lines 128 - 146, The code currently calls arg.value.eval(context) twice for named args (once assigning to evaldArguments[j] and again when creating the Declaration), which can double work and break non-idempotent expressions; change the branch in mixin-definition.js to evaluate arg.value once into a local variable (e.g., evaluated = arg.value.eval(context)), assign evaldArguments[j] = evaluated, and use that same evaluated variable when calling frame.prependRule(new Declaration(name, ...)) so the evaluated node is reused and the second eval is removed.
♻️ Duplicate comments (9)
packages/less/lib/less/tree/color.js (1)
51-52:⚠️ Potential issue | 🟠 MajorPreserve parsed alpha
0instead of defaulting it away.Line 52 still uses
||, so a parsed alpha of0from 4/8-digit hex is treated as missing and replaced withaor1. That changes fully transparent colors into opaque ones, which violates the PR’s “no runtime behavior changes” goal.Suggested fix
- this.alpha = this.alpha || (typeof a === 'number' ? a : 1); + this.alpha = this.alpha === undefined ? (typeof a === 'number' ? a : 1) : this.alpha;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/less/lib/less/tree/color.js` around lines 51 - 52, The assignment to this.alpha uses the || operator which treats a parsed alpha of 0 as falsy and overwrites it; update the logic in the constructor/initializer (referencing this.alpha and the local a parameter in color.js) to preserve numeric 0 by using an explicit existence check (e.g. test typeof this.alpha === 'number' or != null) before falling back to a or 1 so fully transparent colors are not coerced to opaque.packages/less/lib/less/tree/call.js (1)
13-18:⚠️ Potential issue | 🟡 MinorAllow
currentFileInfoto be optional in the constructor JSDoc.Some internal
new Call(...)sites passundefinedhere, so this annotation is narrower than the actual API and makes laterfileInfo()usage look safer than it is.#!/bin/bash # Verify current Call constructor usages and look for undefined file info arguments. sed -n '48,58p' packages/less/lib/less/tree/variable.js rg -nP "new\s+Call\s*\(" packages/less/lib/less -C1🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/less/lib/less/tree/call.js` around lines 13 - 18, The JSDoc for the Call constructor incorrectly requires currentFileInfo; update the parameter annotation for currentFileInfo in the Call constructor JSDoc (symbol: Call, param name: currentFileInfo) to allow undefined (e.g. FileInfo|undefined or FileInfo|null) so callers that pass undefined are accurately represented and downstream uses like fileInfo() reflect the possibility of missing data; ensure the JSDoc matches other call sites that pass undefined.packages/less/lib/less/tree/property.js (1)
74-79:⚠️ Potential issue | 🟠 MajorError path references undefined legacy fields.
The error branch at lines 77-78 reads
this.currentFileInfo.filenameandthis.index, but the constructor (lines 15-22) only setsthis._indexandthis._fileInfo. This will throw aTypeErrorinstead of the intended LessNameerror when a property lookup fails.🐛 Fix to use the correct fields
} else { throw { type: 'Name', message: `Property '${name}' is undefined`, - filename: this.currentFileInfo.filename, - index: this.index }; + filename: this.fileInfo().filename, + index: this.getIndex() }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/less/lib/less/tree/property.js` around lines 74 - 79, The error throw in the Property lookup uses legacy fields this.currentFileInfo.filename and this.index which are not defined; update the throw to use the actual instance fields set in the constructor (this._fileInfo.filename and this._index) so the intended Less Name error is thrown correctly from the property lookup in property.js.packages/less/lib/less/tree/declaration.js (1)
115-122:⚠️ Potential issue | 🟡 MinorError handler should guard
fileInfo()access.Line 119 calls
this.fileInfo().filenamewithout checking iffileInfo()returns a valid object. SincecurrentFileInfois optional in the constructor (line 36), this could replace a real evaluation error with aTypeErrorwhen_fileInfois undefined.Note: The
genCSSerror handler (lines 70-71) already uses the safer patternthis._fileInfo && this._fileInfo.filename.🛡️ Apply the same null-safe pattern
catch (e) { const err = /** `@type` {{ index?: number, filename?: string }} */ (e); if (typeof err.index !== 'number') { err.index = this.getIndex(); - err.filename = this.fileInfo().filename; + const fileInfo = this.fileInfo(); + err.filename = fileInfo && fileInfo.filename; } throw e; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/less/lib/less/tree/declaration.js` around lines 115 - 122, The catch block in declaration.js uses this.fileInfo().filename without null-checking and can throw if _fileInfo is undefined; change the error handling in the catch of the relevant function to use a null-safe access (like this._fileInfo && this._fileInfo.filename) when assigning err.filename, and only call this.getIndex() and this._fileInfo.filename if those values exist; update the catch that currently references this.fileInfo() to mirror the safe pattern used in genCSS (lines ~70-71) so a real evaluation error isn't replaced by a TypeError.packages/less/lib/less/tree/nested-at-rule.js (1)
164-184:⚠️ Potential issue | 🟡 Minor
permute()base case returns wrong shape.Line 172 returns
arr[0](aNode[]) but the method signature promisesNode[][]. The double cast/**@type{Node[][]} */ (/**@type{unknown} */ (arr[0]))hides this mismatch at compile time, but callers expectingNode[][]may receive a flat array.🔧 Fix to maintain the Node[][] contract
} else if (arr.length === 1) { - return /** `@type` {Node[][]} */ (/** `@type` {unknown} */ (arr[0])); + return arr[0].map(item => [item]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/less/lib/less/tree/nested-at-rule.js` around lines 164 - 184, The permute() base case returns a Node[] instead of the promised Node[][]; in the branch where arr.length === 1 return an array containing arr[0] (i.e. return [arr[0]]) so the function always returns Node[][], and remove the unnecessary double-cast; keep the existing behavior for the empty and multi-element branches unchanged.packages/less/lib/less/tree/mixin-call.js (1)
226-229:⚠️ Potential issue | 🟡 MinorPreserve nested Less error metadata when rethrowing.
This wrapper keeps the message and stack, but it drops
typeand any other structured fields from inner Less errors. Failures bubbling out ofevalCall()get flattened at the mixin call site.💡 Suggested fix
} catch (e) { - const err = /** `@type` {{ message?: string, stack?: string }} */ (e); - throw { message: err.message, index: this.getIndex(), filename: this.fileInfo().filename, stack: err.stack }; + const err = (e && typeof e === 'object') ? e : { message: String(e) }; + throw { + ...err, + message: /** `@type` {{ message?: string }} */ (err).message ?? String(e), + index: this.getIndex(), + filename: this.fileInfo().filename, + stack: /** `@type` {{ stack?: string }} */ (err).stack + }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/less/lib/less/tree/mixin-call.js` around lines 226 - 229, The catch block in mixin-call.js currently rethrows a new object with only message/stack, dropping other Less error fields; update the rethrow to preserve all properties from the original error (e.g., using Object.assign or object spread) and then set/override index and filename with this.getIndex() and this.fileInfo().filename so nested Less error metadata like type is retained (keep the existing use-site: the catch around evalCall() in the MixinCall/evalCall code path).packages/less/lib/less/tree/ruleset.js (1)
146-153:⚠️ Potential issue | 🟠 MajorPropagate selector reparse failures here.
If interpolation produces an invalid selector, this callback leaves
selectorson the pre-parse AST and evaluation continues. That hides the parse failure instead of surfacing a Less error.💡 Suggested fix
toParseSelectors.join(','), ['selectors'], function(/** `@type` {Error | null} */ err, /** `@type` {Node[]} */ result) { + if (err) { + throw err; + } if (result) { selectors = /** `@type` {Selector[]} */ (utils.flattenArray(result)); } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/less/lib/less/tree/ruleset.js` around lines 146 - 153, The callback passed to Parser.parseNode in ruleset.js ignores the err parameter and leaves selectors unchanged on parse failure; update the anonymous callback in the parseNode call so that if err is non-null it immediately propagates the failure (e.g., throw the error or rethrow as a Less parsing error) instead of silently continuing, otherwise proceed to set selectors = utils.flattenArray(result) as before; ensure this change references the same Parser.new(...).parseNode callback and the selectors variable so invalid selector reparses surface as a Less error.packages/less/lib/less/tree/container.js (1)
86-86:⚠️ Potential issue | 🟠 MajorGuard the inherited function registry here.
The
functionRegistryoncontext.frames[0]may be undefined in synthetic frames (as handled inmixin-definition.js). Calling.inherit()unconditionally can throw.💡 Suggested fix
- this.rules[0].functionRegistry = /** `@type` {RulesetWithExtras} */ (context.frames[0]).functionRegistry.inherit(); + const parentRegistry = /** `@type` {RulesetWithExtras} */ (context.frames[0]).functionRegistry; + this.rules[0].functionRegistry = parentRegistry ? parentRegistry.inherit() : undefined;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/less/lib/less/tree/container.js` at line 86, The assignment to this.rules[0].functionRegistry calls .inherit() on context.frames[0].functionRegistry without checking for existence and can throw for synthetic frames; update the code in container.js to first check whether context.frames[0].functionRegistry is defined and only call .inherit() when present (otherwise set this.rules[0].functionRegistry to a safe fallback such as undefined or a new/empty registry), mirroring the guarding logic used in mixin-definition.js so .inherit() is never invoked on an undefined value.packages/less/lib/less/tree/media.js (1)
72-72:⚠️ Potential issue | 🟠 MajorGuard the inherited function registry here too.
Same issue as
Container.eval:functionRegistryis treated as optional elsewhere in the tree, so.inherit()can throw on undefined.💡 Suggested fix
- /** `@type` {RulesetLikeNode} */ (this.rules[0]).functionRegistry = /** `@type` {RulesetLikeNode} */ (context.frames[0]).functionRegistry.inherit(); + const parentRegistry = /** `@type` {RulesetLikeNode} */ (context.frames[0]).functionRegistry; + /** `@type` {RulesetLikeNode} */ (this.rules[0]).functionRegistry = parentRegistry ? parentRegistry.inherit() : undefined;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/less/lib/less/tree/media.js` at line 72, The assignment to functionRegistry calls .inherit() without checking existence and can throw if context.frames[0].functionRegistry is undefined; update the code in media.js so that when setting /** `@type` {RulesetLikeNode} */ (this.rules[0]).functionRegistry you first check whether context.frames[0].functionRegistry exists (e.g., conditional or optional chaining) and only call .inherit() when present, otherwise leave functionRegistry undefined (matching how Container.eval guards it).
🧹 Nitpick comments (2)
packages/less/lib/less/tree/variable.js (1)
71-82: Consider extracting the sharedfind()helper.The
find(obj, fun)method is identical to the one inPropertyclass. While not blocking, extracting this to a shared utility or the baseNodeclass would reduce duplication.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/less/lib/less/tree/variable.js` around lines 71 - 82, The find(obj, fun) implementation is duplicated in Variable and Property; extract it to a shared helper or the base Node class (e.g., add Node.find(obj, fun) or a util function findInArray) and replace the per-class method with a call to that shared implementation. Update Variable.find and Property.find to delegate to the new Node.find / util function (preserve signature and behavior: iterate obj, call fun with obj[i] and return first truthy result or null). Ensure tests still pass and remove the duplicated method bodies from both classes.packages/less/lib/less/tree/import.js (1)
277-293: Potential null dereference when accessing nested feature value.Line 280 casts
featureValue[0].valuetoNode[]and reassigns tofeatureValue, but there's no guard ensuringfeatureValue[0].valueis actually an array before the subsequentArray.isArray(featureValue)check on line 281. IffeatureValue[0].valueisundefinedor a non-array, the cast silently proceeds and the array check fails gracefully, but this relies on implicit behavior.Consider adding a truthiness check before the cast for clarity:
♻️ Suggested improvement
if (Array.isArray(featureValue) && featureValue.length >= 1) { - featureValue = /** `@type` {Node[]} */ (featureValue[0].value); - if (Array.isArray(featureValue) && featureValue.length >= 2) { + const innerValue = featureValue[0].value; + if (Array.isArray(innerValue) && innerValue.length >= 2) { + featureValue = innerValue; const isLayer = featureValue[0].type === 'Keyword' && featureValue[0].value === 'layer'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/less/lib/less/tree/import.js` around lines 277 - 293, The code may dereference featureValue[0].value without ensuring it exists; before casting/reassigning featureValue to featureValue[0].value in the import handling block (where this.features and featureValue are used), first check that featureValue[0] is truthy and that Array.isArray(featureValue[0].value) (or featureValue[0].value != null && Array.isArray(...)) is true; only then reassign featureValue = featureValue[0].value and proceed to the later length/type checks and Expression creation (references: this.features, featureValue, Expression, this.css).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/less/lib/less/tree/atrule.js`:
- Around line 86-95: The constructor currently calls setParent(selectors, this)
on a detached local so newly-attached child nodes remain unparented; instead
parent the actual child properties: after creating/assigning this.declarations
in the simple-block path call this.setParent(this.declarations, this) (or this
if declarations are nodes), and after you replace this.rules[0].selectors with a
new Selector list call this.setParent(this.rules[0].selectors, this.rules[0]) so
the fresh selector list is parented to its ruleset; keep the existing
this.setParent(this.rules, this) for the rules array itself.
In `@packages/less/lib/less/tree/call.js`:
- Around line 75-86: Replace the unsafe direct hasOwnProperty calls on the
caught error object with safe calls using Object.prototype.hasOwnProperty.call;
specifically in the catch block in packages/less/lib/less/tree/call.js, change
the condition that currently uses (e).hasOwnProperty('line') and
(e).hasOwnProperty('column') to Object.prototype.hasOwnProperty.call(e, 'line')
and Object.prototype.hasOwnProperty.call(e, 'column') so null/undefined or
objects created with Object.create(null) don't throw; leave the rest of the
thrown error construction (references to this.name, this.getIndex(),
this.fileInfo(), and e.message/lineNumber/columnNumber) unchanged.
In `@packages/less/lib/less/tree/node.js`:
- Around line 57-61: Node.accept currently forwards this.value (and other child
fields) directly to TreeVisitor.visit but the new TreeVisitor.visit signature
expects Node values; update Node.accept to guard before calling visitor.visit:
only call visitor.visit(child) when the child is a Node (e.g., check instanceof
Node or child && typeof child.accept === 'function'), call visitor.visitArray
for arrays (use visitor.visitArray(this.value, nonReplacing) when value is an
array), and otherwise leave primitive children (strings, numbers) unchanged;
apply the same guarded logic to the other child-visiting code paths referenced
(e.g., the later block around the other accept usage) so comment.js and similar
subclasses no longer pass primitives into TreeVisitor.visit.
In `@packages/less/lib/less/tree/selector.js`:
- Around line 13-20: The JSDoc for the constructor parameter currently allows
"(Element | Selector)[] | string" even though getElements() always yields
Element[] and the class uses Element-only members; update the JSDoc for the
"elements" (aka "els") parameter to be Element[] (or document that incoming
Selector[] are normalized to Element[]), and make the same narrowing change for
the other related JSDoc block (the second occurrence around lines 68–91) so that
`@ts-check` reflects the actual runtime shape; reference getElements(), Selector
(class), and the constructor/els parameter when making the edits.
---
Outside diff comments:
In `@packages/less/lib/less/tree/call.js`:
- Around line 54-65: The code currently toggles context.mathOn and calls
context.enterCalc() but doesn't guarantee context.exitCalc() and restoration of
context.mathOn when evaluation throws; wrap the evaluation sequence that calls
a.eval(context) and funcCaller.call(...) in a try/finally so exit logic always
runs: move the existing exitCalc() logic into a finally block and ensure it
always calls context.exitCalc() when this.calc || context.inCalc and restores
context.mathOn to the saved currentMathContext; apply the same try/finally
pattern to the related section around lines 71-109 that performs similar
enter/exit calls.
In `@packages/less/lib/less/tree/mixin-definition.js`:
- Around line 128-146: The code currently calls arg.value.eval(context) twice
for named args (once assigning to evaldArguments[j] and again when creating the
Declaration), which can double work and break non-idempotent expressions; change
the branch in mixin-definition.js to evaluate arg.value once into a local
variable (e.g., evaluated = arg.value.eval(context)), assign evaldArguments[j] =
evaluated, and use that same evaluated variable when calling
frame.prependRule(new Declaration(name, ...)) so the evaluated node is reused
and the second eval is removed.
---
Duplicate comments:
In `@packages/less/lib/less/tree/call.js`:
- Around line 13-18: The JSDoc for the Call constructor incorrectly requires
currentFileInfo; update the parameter annotation for currentFileInfo in the Call
constructor JSDoc (symbol: Call, param name: currentFileInfo) to allow undefined
(e.g. FileInfo|undefined or FileInfo|null) so callers that pass undefined are
accurately represented and downstream uses like fileInfo() reflect the
possibility of missing data; ensure the JSDoc matches other call sites that pass
undefined.
In `@packages/less/lib/less/tree/color.js`:
- Around line 51-52: The assignment to this.alpha uses the || operator which
treats a parsed alpha of 0 as falsy and overwrites it; update the logic in the
constructor/initializer (referencing this.alpha and the local a parameter in
color.js) to preserve numeric 0 by using an explicit existence check (e.g. test
typeof this.alpha === 'number' or != null) before falling back to a or 1 so
fully transparent colors are not coerced to opaque.
In `@packages/less/lib/less/tree/container.js`:
- Line 86: The assignment to this.rules[0].functionRegistry calls .inherit() on
context.frames[0].functionRegistry without checking for existence and can throw
for synthetic frames; update the code in container.js to first check whether
context.frames[0].functionRegistry is defined and only call .inherit() when
present (otherwise set this.rules[0].functionRegistry to a safe fallback such as
undefined or a new/empty registry), mirroring the guarding logic used in
mixin-definition.js so .inherit() is never invoked on an undefined value.
In `@packages/less/lib/less/tree/declaration.js`:
- Around line 115-122: The catch block in declaration.js uses
this.fileInfo().filename without null-checking and can throw if _fileInfo is
undefined; change the error handling in the catch of the relevant function to
use a null-safe access (like this._fileInfo && this._fileInfo.filename) when
assigning err.filename, and only call this.getIndex() and
this._fileInfo.filename if those values exist; update the catch that currently
references this.fileInfo() to mirror the safe pattern used in genCSS (lines
~70-71) so a real evaluation error isn't replaced by a TypeError.
In `@packages/less/lib/less/tree/media.js`:
- Line 72: The assignment to functionRegistry calls .inherit() without checking
existence and can throw if context.frames[0].functionRegistry is undefined;
update the code in media.js so that when setting /** `@type` {RulesetLikeNode} */
(this.rules[0]).functionRegistry you first check whether
context.frames[0].functionRegistry exists (e.g., conditional or optional
chaining) and only call .inherit() when present, otherwise leave
functionRegistry undefined (matching how Container.eval guards it).
In `@packages/less/lib/less/tree/mixin-call.js`:
- Around line 226-229: The catch block in mixin-call.js currently rethrows a new
object with only message/stack, dropping other Less error fields; update the
rethrow to preserve all properties from the original error (e.g., using
Object.assign or object spread) and then set/override index and filename with
this.getIndex() and this.fileInfo().filename so nested Less error metadata like
type is retained (keep the existing use-site: the catch around evalCall() in the
MixinCall/evalCall code path).
In `@packages/less/lib/less/tree/nested-at-rule.js`:
- Around line 164-184: The permute() base case returns a Node[] instead of the
promised Node[][]; in the branch where arr.length === 1 return an array
containing arr[0] (i.e. return [arr[0]]) so the function always returns
Node[][], and remove the unnecessary double-cast; keep the existing behavior for
the empty and multi-element branches unchanged.
In `@packages/less/lib/less/tree/property.js`:
- Around line 74-79: The error throw in the Property lookup uses legacy fields
this.currentFileInfo.filename and this.index which are not defined; update the
throw to use the actual instance fields set in the constructor
(this._fileInfo.filename and this._index) so the intended Less Name error is
thrown correctly from the property lookup in property.js.
In `@packages/less/lib/less/tree/ruleset.js`:
- Around line 146-153: The callback passed to Parser.parseNode in ruleset.js
ignores the err parameter and leaves selectors unchanged on parse failure;
update the anonymous callback in the parseNode call so that if err is non-null
it immediately propagates the failure (e.g., throw the error or rethrow as a
Less parsing error) instead of silently continuing, otherwise proceed to set
selectors = utils.flattenArray(result) as before; ensure this change references
the same Parser.new(...).parseNode callback and the selectors variable so
invalid selector reparses surface as a Less error.
---
Nitpick comments:
In `@packages/less/lib/less/tree/import.js`:
- Around line 277-293: The code may dereference featureValue[0].value without
ensuring it exists; before casting/reassigning featureValue to
featureValue[0].value in the import handling block (where this.features and
featureValue are used), first check that featureValue[0] is truthy and that
Array.isArray(featureValue[0].value) (or featureValue[0].value != null &&
Array.isArray(...)) is true; only then reassign featureValue =
featureValue[0].value and proceed to the later length/type checks and Expression
creation (references: this.features, featureValue, Expression, this.css).
In `@packages/less/lib/less/tree/variable.js`:
- Around line 71-82: The find(obj, fun) implementation is duplicated in Variable
and Property; extract it to a shared helper or the base Node class (e.g., add
Node.find(obj, fun) or a util function findInArray) and replace the per-class
method with a call to that shared implementation. Update Variable.find and
Property.find to delegate to the new Node.find / util function (preserve
signature and behavior: iterate obj, call fun with obj[i] and return first
truthy result or null). Ensure tests still pass and remove the duplicated method
bodies from both classes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ce242e2e-442a-47d8-b17c-6afa84bfd635
📒 Files selected for processing (46)
.husky/pre-commitpackages/less/lib/less/tree/anonymous.jspackages/less/lib/less/tree/assignment.jspackages/less/lib/less/tree/atrule-syntax.jspackages/less/lib/less/tree/atrule.jspackages/less/lib/less/tree/attribute.jspackages/less/lib/less/tree/call.jspackages/less/lib/less/tree/color.jspackages/less/lib/less/tree/combinator.jspackages/less/lib/less/tree/comment.jspackages/less/lib/less/tree/condition.jspackages/less/lib/less/tree/container.jspackages/less/lib/less/tree/debug-info.jspackages/less/lib/less/tree/declaration.jspackages/less/lib/less/tree/detached-ruleset.jspackages/less/lib/less/tree/dimension.jspackages/less/lib/less/tree/element.jspackages/less/lib/less/tree/expression.jspackages/less/lib/less/tree/extend.jspackages/less/lib/less/tree/import.jspackages/less/lib/less/tree/index.jspackages/less/lib/less/tree/javascript.jspackages/less/lib/less/tree/js-eval-node.jspackages/less/lib/less/tree/keyword.jspackages/less/lib/less/tree/media.jspackages/less/lib/less/tree/merge-rules.jspackages/less/lib/less/tree/mixin-call.jspackages/less/lib/less/tree/mixin-definition.jspackages/less/lib/less/tree/namespace-value.jspackages/less/lib/less/tree/negative.jspackages/less/lib/less/tree/nested-at-rule.jspackages/less/lib/less/tree/node.jspackages/less/lib/less/tree/operation.jspackages/less/lib/less/tree/paren.jspackages/less/lib/less/tree/property.jspackages/less/lib/less/tree/query-in-parens.jspackages/less/lib/less/tree/quoted.jspackages/less/lib/less/tree/ruleset.jspackages/less/lib/less/tree/selector.jspackages/less/lib/less/tree/unicode-descriptor.jspackages/less/lib/less/tree/unit.jspackages/less/lib/less/tree/url.jspackages/less/lib/less/tree/value.jspackages/less/lib/less/tree/variable-call.jspackages/less/lib/less/tree/variable.jspackages/less/package.json
🚧 Files skipped from review as they are similar to previous changes (12)
- packages/less/lib/less/tree/variable-call.js
- packages/less/lib/less/tree/atrule-syntax.js
- packages/less/lib/less/tree/assignment.js
- packages/less/lib/less/tree/operation.js
- packages/less/lib/less/tree/detached-ruleset.js
- packages/less/package.json
- packages/less/lib/less/tree/paren.js
- packages/less/lib/less/tree/condition.js
- packages/less/lib/less/tree/debug-info.js
- .husky/pre-commit
- packages/less/lib/less/tree/index.js
- packages/less/lib/less/tree/javascript.js
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/less/lib/less/tree/ruleset.js (1)
694-710:⚠️ Potential issue | 🔴 CriticalBug: Accessing
elementsToPak[0]whenelementsToPak.length === 0yieldsundefined.The condition at line 694 checks for an empty array (
elementsToPak.length === 0) but then attempts to accesselementsToPak[0], which will beundefined. This passesundefinedto theParenconstructor, causing a runtime error.The condition should be
=== 1to handle the single-element case (unwrapping it directly), with the else branch handling multiple elements (wrapping them in a Selector). When the array is truly empty, no valid Paren can be constructed.🐛 Proposed fix
function createParenthesis(elementsToPak, originalElement) { /** `@type` {Paren} */ let replacementParen; /** `@type` {number} */ let j; - if (elementsToPak.length === 0) { + if (elementsToPak.length === 1) { replacementParen = new Paren(elementsToPak[0]); } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/less/lib/less/tree/ruleset.js` around lines 694 - 710, The current code creates a Paren with elementsToPak[0] when elementsToPak.length === 0 (producing undefined); change the branching so that if elementsToPak.length === 1 you construct replacementParen = new Paren(elementsToPak[0]), if elementsToPak.length > 1 you build insideParent with Element(...) and do replacementParen = new Paren(new Selector(insideParent)), and if elementsToPak.length === 0 return null (or otherwise bail out) instead of attempting to construct a Paren; references: elementsToPak, Paren, Selector, Element, originalElement.
♻️ Duplicate comments (1)
packages/less/lib/less/tree/ruleset.js (1)
146-154:⚠️ Potential issue | 🟠 MajorHandle parseNode errors in selector reparsing.
The callback at line 149 receives an
errparameter but doesn't handle it. If variable interpolation produces invalid selector syntax, the error is silently ignored and theselectorsarray retains its unevaluated state, allowing malformed selectors to proceed to code generation.Other
parseNodeinvocations in the codebase consistently handle errors:selector.jsthrows, andtransformDeclarationin the same file marks as parsed on error. This callback should follow the same pattern.Proposed fix
new (/** `@type` {new (...args: [EvalContext, object, FileInfo, number]) => { parseNode: Function }} */ (/** `@type` {unknown} */ (Parser)))(context, /** `@type` {{ context: EvalContext, importManager: object }} */ (this.parse).importManager, selectorFileInfo, startingIndex).parseNode( toParseSelectors.join(','), ['selectors'], function(/** `@type` {Error | null} */ err, /** `@type` {Node[]} */ result) { + if (err) { + throw err; + } if (result) { selectors = /** `@type` {Selector[]} */ (utils.flattenArray(result)); } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/less/lib/less/tree/ruleset.js` around lines 146 - 154, The parseNode callback in ruleset.js ignores the err parameter, allowing invalid selectors through; update the anonymous callback passed to Parser(...).parseNode so that if err is truthy it is propagated (throw the error) instead of silently continuing—locate the parseNode call inside the Ruleset reparse branch (the callback that currently sets selectors = utils.flattenArray(result)) and add an early err check that throws the received Error to match selector.js behavior.
🧹 Nitpick comments (2)
packages/less/lib/less/tree/ruleset.js (2)
302-305: Consider usingArray.isArray()for clarity.The condition
(importRules.length || importRules.length === 0)is a verbose way to check if the result is an array. UsingArray.isArray(importRules)would be more idiomatic and clearer.♻️ Suggested simplification
- if (importRules && (/** `@type` {Node[]} */ (/** `@type` {unknown} */ (importRules)).length || /** `@type` {Node[]} */ (/** `@type` {unknown} */ (importRules)).length === 0)) { - const importArr = /** `@type` {Node[]} */ (/** `@type` {unknown} */ (importRules)); + if (importRules && Array.isArray(importRules)) { + const importArr = /** `@type` {Node[]} */ (importRules); rules.splice(i, 1, ...importArr); i += importArr.length - 1;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/less/lib/less/tree/ruleset.js` around lines 302 - 305, The current condition checking importRules uses a verbose length check; update the if statement to use Array.isArray(importRules) for clarity and intent (keep the existing type casts around importRules). Locate the block handling importRules in ruleset.js (the code that does rules.splice(i, 1, ...importArr) and adjusts i by importArr.length - 1) and replace the combined length check with Array.isArray(importRules), ensuring importArr remains the same casted Node[] and the splice and i arithmetic are unchanged.
429-446: Type mismatch:decl.parsedstores boolean but cast toNode.The property
decl.parsedappears to store a boolean value (true) but is cast toNodevia/**@type{Node} */ (/**@type{unknown} */ (true)). This suggests a type definition mismatch in the underlyingDeclarationtype whereparsedshould be typed asbooleanrather thanNode.This is pre-existing behavior and doesn't introduce bugs, but consider correcting the type definition if feasible.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/less/lib/less/tree/ruleset.js` around lines 429 - 446, The code incorrectly casts boolean flags to Node (see decl.parsed assignments and the JSDoc cast /** `@type` {Node} */ (/** `@type` {unknown} */ (true))); update the Declaration type so parsed is boolean (not Node) and replace these casts with boolean-typed annotations or plain assignments (adjust any other occurrences where decl.parsed is treated as a Node), e.g., change the Declaration/Declaration & { parsed?: ... } JSDoc to parsed?: boolean and remove the /** `@type` {Node} */ casts around true in the parseNode callback and the else branch so decl.parsed is assigned a boolean consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/less/lib/less/tree/ruleset.js`:
- Around line 694-710: The current code creates a Paren with elementsToPak[0]
when elementsToPak.length === 0 (producing undefined); change the branching so
that if elementsToPak.length === 1 you construct replacementParen = new
Paren(elementsToPak[0]), if elementsToPak.length > 1 you build insideParent with
Element(...) and do replacementParen = new Paren(new Selector(insideParent)),
and if elementsToPak.length === 0 return null (or otherwise bail out) instead of
attempting to construct a Paren; references: elementsToPak, Paren, Selector,
Element, originalElement.
---
Duplicate comments:
In `@packages/less/lib/less/tree/ruleset.js`:
- Around line 146-154: The parseNode callback in ruleset.js ignores the err
parameter, allowing invalid selectors through; update the anonymous callback
passed to Parser(...).parseNode so that if err is truthy it is propagated (throw
the error) instead of silently continuing—locate the parseNode call inside the
Ruleset reparse branch (the callback that currently sets selectors =
utils.flattenArray(result)) and add an early err check that throws the received
Error to match selector.js behavior.
---
Nitpick comments:
In `@packages/less/lib/less/tree/ruleset.js`:
- Around line 302-305: The current condition checking importRules uses a verbose
length check; update the if statement to use Array.isArray(importRules) for
clarity and intent (keep the existing type casts around importRules). Locate the
block handling importRules in ruleset.js (the code that does rules.splice(i, 1,
...importArr) and adjusts i by importArr.length - 1) and replace the combined
length check with Array.isArray(importRules), ensuring importArr remains the
same casted Node[] and the splice and i arithmetic are unchanged.
- Around line 429-446: The code incorrectly casts boolean flags to Node (see
decl.parsed assignments and the JSDoc cast /** `@type` {Node} */ (/** `@type`
{unknown} */ (true))); update the Declaration type so parsed is boolean (not
Node) and replace these casts with boolean-typed annotations or plain
assignments (adjust any other occurrences where decl.parsed is treated as a
Node), e.g., change the Declaration/Declaration & { parsed?: ... } JSDoc to
parsed?: boolean and remove the /** `@type` {Node} */ casts around true in the
parseNode callback and the else branch so decl.parsed is assigned a boolean
consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 28373fb7-de66-4675-9427-d45bf260a925
📒 Files selected for processing (1)
packages/less/lib/less/tree/ruleset.js
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
1 participant
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.
Summary
@ts-checkand proper JSDoc type annotations to all 44 files inlib/less/tree/tsc --noEmit{*}or{any}casts — all types derived from reading actual codeprepublishOnlyand pre-commit hook to prevent regressionscheckJs: trueglobally in tsconfig.jsonKey changes
EvalContext,CSSOutput,TreeVisitor,FileInfo,VisibilityInfo) defined innode.jsNode.valuetyped as union:Node | Node[] | string | number | undefinedNode.prototype.parsedeclared for parser-injected prototype property (avoids shadowing)Test plan
tsc --noEmitreports zero errorsSummary by CodeRabbit
Refactor
Chores