Conversation
🦋 Changeset detectedLatest commit: 3d8bf6c The changes in this PR will be included in the next version bump. This PR includes changesets to release 13 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
b7c1208 to
62e26af
Compare
Merging this PR will not alter performance
Comparing Footnotes
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a new nursery lint rule Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/biome_js_analyze/src/lint/nursery/use_math_min_max.rs`:
- Line 15: Replace the public empty options struct/type UseMathMinMaxOptions
with a simple alias type Options = () in the rule module so the rule uses a unit
options type until real settings exist; remove the now-unnecessary
crates/biome_rule_options::use_math_min_max module and its export from the
options lib (and apply the same change to the other similar rule definitions
referenced around lines 61–65) to avoid extra API surface and codegen churn,
ensuring any references to UseMathMinMaxOptions are updated to Options.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: db2224cf-8313-49bc-9087-d7220efce4f0
⛔ Files ignored due to path filters (9)
crates/biome_configuration/src/analyzer/linter/rules.rsis excluded by!**/rules.rsand included by**crates/biome_configuration/src/generated/linter_options_check.rsis excluded by!**/generated/**,!**/generated/**and included by**crates/biome_diagnostics_categories/src/categories.rsis excluded by!**/categories.rsand included by**crates/biome_js_analyze/tests/specs/nursery/useMathMinMax/invalid.js.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/useMathMinMax/invalid.ts.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/useMathMinMax/valid.js.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/useMathMinMax/valid.ts.snapis excluded by!**/*.snapand included by**packages/@biomejs/backend-jsonrpc/src/workspace.tsis excluded by!**/backend-jsonrpc/src/workspace.tsand included by**packages/@biomejs/biome/configuration_schema.jsonis excluded by!**/configuration_schema.jsonand included by**
📒 Files selected for processing (8)
.changeset/hot-seas-jump.mdcrates/biome_js_analyze/src/lint/nursery/use_math_min_max.rscrates/biome_js_analyze/tests/specs/nursery/useMathMinMax/invalid.jscrates/biome_js_analyze/tests/specs/nursery/useMathMinMax/invalid.tscrates/biome_js_analyze/tests/specs/nursery/useMathMinMax/valid.jscrates/biome_js_analyze/tests/specs/nursery/useMathMinMax/valid.tscrates/biome_rule_options/src/lib.rscrates/biome_rule_options/src/use_math_min_max.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/biome_js_analyze/src/lint/nursery/use_math_min_max.rs`:
- Around line 107-112: The rule currently constructs a hard-coded Math.min/max
callee without checking for a local binding named "Math"; add a semantic guard
that bails out if "Math" is shadowed in the current scope. Before constructing
the AnyJsExpression::JsStaticMemberExpression (the
make::js_identifier_expression(make::js_reference_identifier(make::ident("Math")))
call) or before applying the fix, query the semantic/scope/binding API in this
lint (the function handling the node) to see if an identifier binding for "Math"
exists in the lexical scope and return/skip the rule if it does, ensuring you do
not emit the Math.min/max replacement when a local "Math" is defined.
- Around line 114-117: The current fix strips and discards trivia via
clean_argument(...) and replace_node_discard_trivia(...), which removes inner
comments inside conditionals (e.g., a > /*keep*/ b ? b : a); update the logic in
the use_math_min_max lint to preserve comments by first detecting comments on
the conditional and its operands (check the comparison.left/right and the
ternary/conditional node for any leading/trailing comment trivia) and if any
comments are present, skip performing the replacement; alternatively, if you
want to keep the transform, use a replacement API that preserves trivia (instead
of replace_node_discard_trivia) and ensure you transfer the original node's
trivia when inserting args from clean_argument; apply the same change to the
other affected blocks (the similar code at the regions around the other two
diffs you noted).
- Around line 193-198: The AST equality check using is_node_equal for left/right
vs consequent/alternate is too permissive because it allows matching
non-idempotent expressions (e.g. foo(), i++), so before returning Min/Max ensure
both compared operands are side-effect-free/idempotent: add or call a purity
predicate (e.g. is_pure_expression / is_idempotent) on left, right, consequent,
and alternate and only accept matches when the corresponding nodes are pure;
update the matching logic around the is_node_equal checks in use_math_min_max
(the left_matches_* and right_matches_* logic and the subsequent branch that
returns Min/Max) to short-circuit if any involved expression is not pure, and
apply the same guard to the analogous checks in the 200-218 block.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1fc5045c-5e05-4e98-b853-08587ba9de4b
⛔ Files ignored due to path filters (10)
crates/biome_cli/src/execute/migrate/eslint_any_rule_to_biome.rsis excluded by!**/migrate/eslint_any_rule_to_biome.rsand included by**crates/biome_configuration/src/analyzer/linter/rules.rsis excluded by!**/rules.rsand included by**crates/biome_configuration/src/generated/linter_options_check.rsis excluded by!**/generated/**,!**/generated/**and included by**crates/biome_diagnostics_categories/src/categories.rsis excluded by!**/categories.rsand included by**crates/biome_js_analyze/tests/specs/nursery/useMathMinMax/invalid.js.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/useMathMinMax/invalid.ts.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/useMathMinMax/valid.js.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/useMathMinMax/valid.ts.snapis excluded by!**/*.snapand included by**packages/@biomejs/backend-jsonrpc/src/workspace.tsis excluded by!**/backend-jsonrpc/src/workspace.tsand included by**packages/@biomejs/biome/configuration_schema.jsonis excluded by!**/configuration_schema.jsonand included by**
📒 Files selected for processing (8)
.changeset/hot-seas-jump.mdcrates/biome_js_analyze/src/lint/nursery/use_math_min_max.rscrates/biome_js_analyze/tests/specs/nursery/useMathMinMax/invalid.jscrates/biome_js_analyze/tests/specs/nursery/useMathMinMax/invalid.tscrates/biome_js_analyze/tests/specs/nursery/useMathMinMax/valid.jscrates/biome_js_analyze/tests/specs/nursery/useMathMinMax/valid.tscrates/biome_rule_options/src/lib.rscrates/biome_rule_options/src/use_math_min_max.rs
✅ Files skipped from review due to trivial changes (5)
- .changeset/hot-seas-jump.md
- crates/biome_js_analyze/tests/specs/nursery/useMathMinMax/valid.ts
- crates/biome_js_analyze/tests/specs/nursery/useMathMinMax/valid.js
- crates/biome_js_analyze/tests/specs/nursery/useMathMinMax/invalid.js
- crates/biome_rule_options/src/use_math_min_max.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/biome_rule_options/src/lib.rs
- crates/biome_js_analyze/tests/specs/nursery/useMathMinMax/invalid.ts
ematipico
reviewed
Apr 12, 2026
62e26af to
48a5dca
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
crates/biome_js_analyze/src/lint/nursery/use_math_min_max.rs (2)
210-252:⚠️ Potential issue | 🟠 MajorBranch-only inner comments can still disappear.
This only merges edge trivia from the other occurrence. If the non-selected copy carries an interior comment, for example
a + b > c ? c : a + /* keep */ b,is_node_equalstill matches but the replacement is built fromcondition_expression, so that comment vanishes. Please either skip the action when either matched operand contains interior comments, or preserve the full matched subtree instead of only its edge trivia.Also applies to: 281-313
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_js_analyze/src/lint/nursery/use_math_min_max.rs` around lines 210 - 252, The replacement can drop interior comments (e.g., a + /* keep */ b) because build_argument_expression only preserves edge trivia; modify build_argument_expression (and similarly the other occurrence around lines 281-313) to skip producing a replacement when either matched operand contains interior comments: add a helper (e.g., has_interior_comments) that inspects the operand subtree for non-edge comments (or uses comment_pieces for interior positions) for both condition_expression and branch_expression, and if either returns true, return None instead of constructing argument; keep existing uses of clean_argument, separator_comment_pieces, prepend_trivia_pieces and append_trivia_pieces unchanged.
387-392:⚠️ Potential issue | 🔴 CriticalStill needs a repeatability guard.
is_node_equalwill also matchfoo() > b ? b : foo(),obj.value > b ? b : obj.value, ori++ > b ? b : i++. The ternary may evaluate the chosen side again;Math.min/maxevaluates each operand once. Please require the matched operands and branches to be repeatable/idempotent before returningMinorMax.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_js_analyze/src/lint/nursery/use_math_min_max.rs` around lines 387 - 392, The current equality checks (is_node_equal on left/right vs consequent/alternate) can match expressions that are not repeatable (e.g., foo(), i++), causing semantic change when replacing the ternary with Math.min/Math.max; add a repeatability guard before constructing Min/Max: implement or call a helper (e.g., is_repeatable_expression or is_idempotent_node) and require that the matched nodes left, right, consequent, and alternate (the symbols left, right, consequent, alternate and the equality checks left_matches_consequent/left_matches_alternate/right_matches_consequent/right_matches_alternate) are repeatable/idempotent when determining Min or Max, returning no lint if any involved expression is not repeatable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/biome_js_analyze/src/lint/nursery/use_math_min_max.rs`:
- Around line 429-436: The function has_unsupported_operand currently only
checks for bigint and Date types, allowing direct non-number literals like
strings or booleans to slip through. To fix this, update has_unsupported_operand
to also return true if is_non_number_literal detects a non-number literal in the
expression. This ensures any obvious non-number literals are properly excluded
from Math.min/Math.max usage. Make the same change in the analogous code block
around lines 557-563.
---
Duplicate comments:
In `@crates/biome_js_analyze/src/lint/nursery/use_math_min_max.rs`:
- Around line 210-252: The replacement can drop interior comments (e.g., a + /*
keep */ b) because build_argument_expression only preserves edge trivia; modify
build_argument_expression (and similarly the other occurrence around lines
281-313) to skip producing a replacement when either matched operand contains
interior comments: add a helper (e.g., has_interior_comments) that inspects the
operand subtree for non-edge comments (or uses comment_pieces for interior
positions) for both condition_expression and branch_expression, and if either
returns true, return None instead of constructing argument; keep existing uses
of clean_argument, separator_comment_pieces, prepend_trivia_pieces and
append_trivia_pieces unchanged.
- Around line 387-392: The current equality checks (is_node_equal on left/right
vs consequent/alternate) can match expressions that are not repeatable (e.g.,
foo(), i++), causing semantic change when replacing the ternary with
Math.min/Math.max; add a repeatability guard before constructing Min/Max:
implement or call a helper (e.g., is_repeatable_expression or
is_idempotent_node) and require that the matched nodes left, right, consequent,
and alternate (the symbols left, right, consequent, alternate and the equality
checks
left_matches_consequent/left_matches_alternate/right_matches_consequent/right_matches_alternate)
are repeatable/idempotent when determining Min or Max, returning no lint if any
involved expression is not repeatable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e66fb192-17c6-4d49-9cd2-45af9ea07a7c
⛔ Files ignored due to path filters (10)
crates/biome_cli/src/execute/migrate/eslint_any_rule_to_biome.rsis excluded by!**/migrate/eslint_any_rule_to_biome.rsand included by**crates/biome_configuration/src/analyzer/linter/rules.rsis excluded by!**/rules.rsand included by**crates/biome_configuration/src/generated/linter_options_check.rsis excluded by!**/generated/**,!**/generated/**and included by**crates/biome_diagnostics_categories/src/categories.rsis excluded by!**/categories.rsand included by**crates/biome_js_analyze/tests/specs/nursery/useMathMinMax/invalid.js.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/useMathMinMax/invalid.ts.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/useMathMinMax/valid.js.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/useMathMinMax/valid.ts.snapis excluded by!**/*.snapand included by**packages/@biomejs/backend-jsonrpc/src/workspace.tsis excluded by!**/backend-jsonrpc/src/workspace.tsand included by**packages/@biomejs/biome/configuration_schema.jsonis excluded by!**/configuration_schema.jsonand included by**
📒 Files selected for processing (8)
.changeset/hot-seas-jump.mdcrates/biome_js_analyze/src/lint/nursery/use_math_min_max.rscrates/biome_js_analyze/tests/specs/nursery/useMathMinMax/invalid.jscrates/biome_js_analyze/tests/specs/nursery/useMathMinMax/invalid.tscrates/biome_js_analyze/tests/specs/nursery/useMathMinMax/valid.jscrates/biome_js_analyze/tests/specs/nursery/useMathMinMax/valid.tscrates/biome_rule_options/src/lib.rscrates/biome_rule_options/src/use_math_min_max.rs
✅ Files skipped from review due to trivial changes (6)
- .changeset/hot-seas-jump.md
- crates/biome_rule_options/src/lib.rs
- crates/biome_js_analyze/tests/specs/nursery/useMathMinMax/valid.ts
- crates/biome_js_analyze/tests/specs/nursery/useMathMinMax/valid.js
- crates/biome_js_analyze/tests/specs/nursery/useMathMinMax/invalid.ts
- crates/biome_rule_options/src/use_math_min_max.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/biome_js_analyze/tests/specs/nursery/useMathMinMax/invalid.js
692542f to
85887c3
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/biome_js_analyze/src/lint/nursery/use_math_min_max.rs (1)
479-480: Minor: Use///for doc comment consistency.Other functions in this file use
///doc comments. This one uses//.♻️ Suggested fix
-// Is it a `bigint` literal or a `BigInt()` call? Math.min/max don't work with bigints, so we want to -// avoid suggesting a replacement that would break the code. +/// Is it a `bigint` literal or a `BigInt()` call? Math.min/max don't work with bigints, so we want to +/// avoid suggesting a replacement that would break the code. fn is_bigint_like(expression: &AnyJsExpression) -> bool {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_js_analyze/src/lint/nursery/use_math_min_max.rs` around lines 479 - 480, Replace the plain `//` comment preceding the bigint-check with a `///` doc comment to match the file's doc-comment style; specifically, convert the comment that reads "Is it a `bigint` literal or a `BigInt()` call? Math.min/max don't work with bigints, so we want to avoid suggesting a replacement that would break the code." into a `///` documentation comment above the associated function or block (the comment near the bigint check in use_math_min_max.rs) so it follows the same `///` format used by other functions in this file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@crates/biome_js_analyze/src/lint/nursery/use_math_min_max.rs`:
- Around line 479-480: Replace the plain `//` comment preceding the bigint-check
with a `///` doc comment to match the file's doc-comment style; specifically,
convert the comment that reads "Is it a `bigint` literal or a `BigInt()` call?
Math.min/max don't work with bigints, so we want to avoid suggesting a
replacement that would break the code." into a `///` documentation comment above
the associated function or block (the comment near the bigint check in
use_math_min_max.rs) so it follows the same `///` format used by other functions
in this file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7213e3d0-a21b-48ac-8be9-680ae3cef261
⛔ Files ignored due to path filters (10)
crates/biome_cli/src/execute/migrate/eslint_any_rule_to_biome.rsis excluded by!**/migrate/eslint_any_rule_to_biome.rsand included by**crates/biome_configuration/src/analyzer/linter/rules.rsis excluded by!**/rules.rsand included by**crates/biome_configuration/src/generated/linter_options_check.rsis excluded by!**/generated/**,!**/generated/**and included by**crates/biome_diagnostics_categories/src/categories.rsis excluded by!**/categories.rsand included by**crates/biome_js_analyze/tests/specs/nursery/useMathMinMax/invalid.js.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/useMathMinMax/invalid.ts.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/useMathMinMax/valid.js.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/useMathMinMax/valid.ts.snapis excluded by!**/*.snapand included by**packages/@biomejs/backend-jsonrpc/src/workspace.tsis excluded by!**/backend-jsonrpc/src/workspace.tsand included by**packages/@biomejs/biome/configuration_schema.jsonis excluded by!**/configuration_schema.jsonand included by**
📒 Files selected for processing (8)
.changeset/hot-seas-jump.mdcrates/biome_js_analyze/src/lint/nursery/use_math_min_max.rscrates/biome_js_analyze/tests/specs/nursery/useMathMinMax/invalid.jscrates/biome_js_analyze/tests/specs/nursery/useMathMinMax/invalid.tscrates/biome_js_analyze/tests/specs/nursery/useMathMinMax/valid.jscrates/biome_js_analyze/tests/specs/nursery/useMathMinMax/valid.tscrates/biome_rule_options/src/lib.rscrates/biome_rule_options/src/use_math_min_max.rs
✅ Files skipped from review due to trivial changes (5)
- .changeset/hot-seas-jump.md
- crates/biome_rule_options/src/lib.rs
- crates/biome_rule_options/src/use_math_min_max.rs
- crates/biome_js_analyze/tests/specs/nursery/useMathMinMax/valid.ts
- crates/biome_js_analyze/tests/specs/nursery/useMathMinMax/valid.js
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/biome_js_analyze/tests/specs/nursery/useMathMinMax/invalid.ts
- crates/biome_js_analyze/tests/specs/nursery/useMathMinMax/invalid.js
85887c3 to
3d8bf6c
Compare
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
2 participants
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
This adds
useMathMinMaxwhich is a port of https://github.com/sindresorhus/eslint-plugin-unicorn/blob/main/docs/rules/prefer-math-min-max.mdgenerated by gpt 5.4
Test Plan
snapshots
Docs