feat(lint): added new rule no-leaked-render from eslint-react
#8171
Conversation
…eaked-conditional-rendering
…eaked-conditional-rendering
🦋 Changeset detectedLatest commit: ce85fe2 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 |
github-actions
bot
added
A-Project
CodSpeed Performance ReportMerging #8171 will not alter performanceComparing Summary
Footnotes
|
This comment has been minimized.
This comment has been minimized.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
.changeset/calm-shrimps-study.md (1)
1-5: Clarify rule naming in the changesetTo avoid mixing the ESLint and Biome names, consider something like:
-Fixed [#7659](https://github.com/biomejs/biome/issues/7659): Added the new rule [`jsx-no-leaked-render`](https://biomejs.dev/linter/rules/no-leaked-render) from ESLint +Fixed [#7659](https://github.com/biomejs/biome/issues/7659): Added the new rule [`no-leaked-render`](https://biomejs.dev/linter/rules/no-leaked-render), ported from ESLint’s `jsx-no-leaked-render`Keeps the origin clear whilst matching the Biome rule slug.
crates/biome_js_analyze/tests/specs/nursery/noLeakedRender/coerce/invalid.options.json (1)
1-16: Coerce/invalid options are correct; consider adding the known edge-case fixtureThe configuration itself looks good (
validStrategies: ["coerce"], levelerror).Given you called out the
checked={isIndeterminate ? false : isChecked}case as a known gap for the coerce strategy, it might be worth adding that as an extra invalid JSX example in the coerce suite once the implementation handles it, so we lock in coverage.crates/biome_rule_options/src/no_leaked_render.rs (1)
3-15: Options wiring looks consistent with other rulesThe
NoLeakedRenderOptionsstruct andMergeimpl are straightforward:valid_strategiescleanly overrides when provided, and the serde configuration matches the expectedvalidStrategiesshape in tests. Only minor thought: if a user ever passes an emptyvalidStrategiesarray, it will effectively disable both strategies rather than falling back to the default pair, which is probably fine but worth being aware of.crates/biome_js_analyze/src/lint/nursery/no_leaked_render.rs (2)
168-244: Logical&&handling mostly matches the intended coerce semanticsThe
JsLogicalExpressionbranch correctly:
- Restricts the rule to
&&.- Treats unary/call/binary left sides (and nested logicals of those) as “coerce-safe”.
- Special-cases identifiers initialised to
true/falsevia the semantic model, which handles boolean flags likeisOpen1/isOpen2in the fixtures.- Leaves classic leaky cases (
count && title,array.length && <List />, etc.) to fall through and emit diagnostics.This aligns well with the tests in
invalid.jsx,coerce/valid.jsx,coerce/invalid.jsx, andternary/invalid.jsx.One small observation: the
is_literal && left.to_trimmed_text().is_empty()check looks like it may never be true in practice (string/number literals will still have non-empty source text, including quotes), so that branch is probably redundant. Not a blocker, but you might want to either remove it or adjust it to whatever literal special-case you originally had in mind.
310-325: Nested logical-expression helper is conservative (by design)
get_is_coerce_valid_nested_logical_expressiononly treats nested logical expressions as coerce-safe when both sides are unary/call/binary expressions (possibly parenthesised). That’s enough to cover cases like(!display || display === DISPLAY.WELCOME) && <span>foo</span>from the fixtures, while deliberately treating mixed identifier conditions as unsafe.If, in future, you decide to allow more complex but still boolean-only conditions (e.g.
isLoading || hasError) without diagnostics, this helper is a good place to extend that logic.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (11)
crates/biome_configuration/src/analyzer/linter/rules.rsis excluded by!**/rules.rsand included by**crates/biome_diagnostics_categories/src/categories.rsis excluded by!**/categories.rsand included by**crates/biome_js_analyze/src/lint/nursery.rsis excluded by!**/nursery.rsand included by**crates/biome_js_analyze/tests/specs/nursery/noLeakedRender/coerce/invalid.jsx.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/noLeakedRender/coerce/valid.jsx.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/noLeakedRender/invalid.jsx.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/noLeakedRender/ternary/invalid.jsx.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/noLeakedRender/ternary/valid.jsx.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/noLeakedRender/valid.jsx.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 (14)
.changeset/calm-shrimps-study.md(1 hunks)crates/biome_js_analyze/src/lint/nursery/no_leaked_render.rs(1 hunks)crates/biome_js_analyze/tests/specs/nursery/noLeakedRender/coerce/invalid.jsx(1 hunks)crates/biome_js_analyze/tests/specs/nursery/noLeakedRender/coerce/invalid.options.json(1 hunks)crates/biome_js_analyze/tests/specs/nursery/noLeakedRender/coerce/valid.jsx(1 hunks)crates/biome_js_analyze/tests/specs/nursery/noLeakedRender/coerce/valid.options.json(1 hunks)crates/biome_js_analyze/tests/specs/nursery/noLeakedRender/invalid.jsx(1 hunks)crates/biome_js_analyze/tests/specs/nursery/noLeakedRender/ternary/invalid.jsx(1 hunks)crates/biome_js_analyze/tests/specs/nursery/noLeakedRender/ternary/invalid.options.json(1 hunks)crates/biome_js_analyze/tests/specs/nursery/noLeakedRender/ternary/valid.jsx(1 hunks)crates/biome_js_analyze/tests/specs/nursery/noLeakedRender/ternary/valid.options.json(1 hunks)crates/biome_js_analyze/tests/specs/nursery/noLeakedRender/valid.jsx(1 hunks)crates/biome_rule_options/src/lib.rs(1 hunks)crates/biome_rule_options/src/no_leaked_render.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-05T14:43:29.581Z
Learnt from: dyc3
Repo: biomejs/biome PR: 7081
File: packages/@biomejs/biome/configuration_schema.json:7765-7781
Timestamp: 2025-08-05T14:43:29.581Z
Learning: The file `packages/biomejs/biome/configuration_schema.json` is auto-generated and should not be manually edited or reviewed for schema issues; any changes should be made at the code generation source.
Applied to files:
crates/biome_js_analyze/tests/specs/nursery/noLeakedRender/coerce/valid.options.jsoncrates/biome_js_analyze/tests/specs/nursery/noLeakedRender/coerce/invalid.options.jsoncrates/biome_js_analyze/tests/specs/nursery/noLeakedRender/ternary/valid.options.jsoncrates/biome_js_analyze/tests/specs/nursery/noLeakedRender/ternary/invalid.options.json
🧬 Code graph analysis (7)
crates/biome_js_analyze/tests/specs/nursery/noLeakedRender/coerce/valid.jsx (4)
crates/biome_js_analyze/tests/specs/nursery/noLeakedRender/invalid.jsx (6)
Component1(13-15)Component2(17-19)Component3(21-23)Component4(25-29)Component5(31-33)Component6(35-37)crates/biome_js_analyze/tests/specs/nursery/noLeakedRender/coerce/invalid.jsx (6)
Component1(3-5)Component2(7-9)Component3(11-13)Component4(15-19)Component5(21-23)Component6(25-27)crates/biome_js_analyze/tests/specs/nursery/noLeakedRender/ternary/valid.jsx (1)
Component1(3-5)crates/biome_js_analyze/tests/specs/nursery/noLeakedRender/valid.jsx (8)
Component1(3-5)Component2(7-9)Component3(11-13)Component4(15-17)isOpen1(56-56)Component5(19-21)isOpen2(61-61)Component6(23-25)
crates/biome_js_analyze/src/lint/nursery/no_leaked_render.rs (1)
crates/biome_js_analyze/src/services/semantic_class.rs (1)
semantic(43-45)
crates/biome_js_analyze/tests/specs/nursery/noLeakedRender/valid.jsx (3)
crates/biome_js_analyze/tests/specs/nursery/noLeakedRender/invalid.jsx (6)
Component1(13-15)Component2(17-19)Component3(21-23)Component4(25-29)Component5(31-33)Component6(35-37)crates/biome_js_analyze/tests/specs/nursery/noLeakedRender/coerce/valid.jsx (8)
Component1(3-5)Component2(7-14)Component3(16-25)Component4(27-29)Component5(32-34)Component6(37-39)isOpen1(31-31)isOpen2(36-36)crates/biome_js_analyze/tests/specs/nursery/noLeakedRender/ternary/valid.jsx (1)
Component1(3-5)
crates/biome_js_analyze/tests/specs/nursery/noLeakedRender/coerce/invalid.jsx (2)
crates/biome_js_analyze/tests/specs/nursery/noLeakedRender/coerce/valid.jsx (6)
Component1(3-5)Component2(7-14)Component3(16-25)Component4(27-29)Component5(32-34)Component6(37-39)crates/biome_js_analyze/tests/specs/nursery/noLeakedRender/valid.jsx (7)
Component1(3-5)Component2(7-9)Component3(11-13)Component4(15-17)Component5(19-21)Component6(23-25)Component7(27-29)
crates/biome_js_analyze/tests/specs/nursery/noLeakedRender/invalid.jsx (4)
crates/biome_js_analyze/tests/specs/nursery/noLeakedRender/coerce/invalid.jsx (7)
Component1(3-5)Component2(7-9)Component3(11-13)Component4(15-19)Component5(21-23)Component6(25-27)MyComponent2(50-52)crates/biome_js_analyze/tests/specs/nursery/noLeakedRender/coerce/valid.jsx (6)
Component1(3-5)Component2(7-14)Component3(16-25)Component4(27-29)Component5(32-34)Component6(37-39)crates/biome_js_analyze/tests/specs/nursery/noLeakedRender/ternary/invalid.jsx (6)
Component1(1-3)Component2(5-7)Component3(9-11)Component4(13-17)Component5(19-21)Component6(23-25)crates/biome_js_analyze/tests/specs/nursery/noLeakedRender/ternary/valid.jsx (1)
Component1(3-5)
crates/biome_js_analyze/tests/specs/nursery/noLeakedRender/ternary/valid.jsx (3)
crates/biome_js_analyze/tests/specs/nursery/noLeakedRender/invalid.jsx (1)
Component1(13-15)crates/biome_js_analyze/tests/specs/nursery/noLeakedRender/coerce/valid.jsx (1)
Component1(3-5)crates/biome_js_analyze/tests/specs/nursery/noLeakedRender/ternary/invalid.jsx (1)
Component1(1-3)
crates/biome_js_analyze/tests/specs/nursery/noLeakedRender/ternary/invalid.jsx (3)
crates/biome_js_analyze/tests/specs/nursery/noLeakedRender/coerce/valid.jsx (6)
Component1(3-5)Component2(7-14)Component3(16-25)Component4(27-29)Component5(32-34)Component6(37-39)crates/biome_js_analyze/tests/specs/nursery/noLeakedRender/ternary/valid.jsx (1)
Component1(3-5)crates/biome_js_analyze/tests/specs/nursery/noLeakedRender/valid.jsx (12)
Component1(3-5)Component2(7-9)Component3(11-13)Component4(15-17)Component5(19-21)Component6(23-25)Component7(27-29)Component8(31-33)Component9(35-37)Component10(39-41)Component11(43-50)Component12(52-54)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: autofix
- GitHub Check: Bench (biome_js_analyze)
- GitHub Check: Check JS Files
- GitHub Check: Documentation
- GitHub Check: Bench (biome_js_formatter)
- GitHub Check: Bench (biome_js_parser)
- GitHub Check: End-to-end tests
- GitHub Check: Bench (biome_configuration)
- GitHub Check: Lint project (depot-ubuntu-24.04-arm-16)
- GitHub Check: Test (depot-ubuntu-24.04-arm-16)
- GitHub Check: Test (depot-windows-2022-16)
- GitHub Check: Check Dependencies
- GitHub Check: Lint project (depot-windows-2022)
- GitHub Check: Test Node.js API
🔇 Additional comments (12)
crates/biome_rule_options/src/lib.rs (1)
123-123: New rule options module export looks consistent
pub mod no_leaked_render;fits the existing naming and sort order for rule option modules; no issues from this side.crates/biome_js_analyze/tests/specs/nursery/noLeakedRender/coerce/valid.options.json (1)
1-16: Config for coerce/valid matches expected shapeSchema reference, rule path, level, and
validStrategies: ["coerce"]all look consistent with the rest of the suite.crates/biome_js_analyze/tests/specs/nursery/noLeakedRender/ternary/valid.jsx (1)
1-5: Good minimal valid ternary exampleThis nicely captures the “use a ternary, not
&&” pattern the rule recommends; fine as a focused green-path fixture.crates/biome_js_analyze/tests/specs/nursery/noLeakedRender/ternary/valid.options.json (1)
1-16: Ternary/valid options are consistent with coerce variantRule path, level, and
validStrategies: ["ternary"]are all in line with the rest of the specs.crates/biome_js_analyze/tests/specs/nursery/noLeakedRender/ternary/invalid.options.json (1)
1-16: Ternary/invalid options wiring looks fineIdentical structure to the valid config, just paired with the red-path fixtures as expected.
crates/biome_js_analyze/tests/specs/nursery/noLeakedRender/coerce/valid.jsx (1)
1-39: Coerce/valid fixtures exercise a good spread of safe patternsThese components cover the key “coerced to boolean” shapes (double negation, boolean comparisons, compound conditions, and boolean props) without leaking raw counts or lengths, which is exactly what we want under the coerce strategy. Looks solid.
crates/biome_js_analyze/tests/specs/nursery/noLeakedRender/valid.jsx (1)
3-64: Valid fixture set looks comprehensiveThis covers the main “safe” patterns (explicit coercion, comparisons, ternaries, boolean flags) the rule is meant to allow, including the Popover
opencases; nothing here looks like it should trigger diagnostics under the default strategies.crates/biome_js_analyze/tests/specs/nursery/noLeakedRender/coerce/invalid.jsx (1)
78-81: Nice edge case around numeric “open” flagsUsing
const isOpen = 0;here is a neat way to ensure the rule distinguishes boolean flags from numeric falsy values when guarding a prop. This should stay invalid under coerce-only strategy and matches the design of the Popover tests incoerce/valid.jsx.crates/biome_js_analyze/tests/specs/nursery/noLeakedRender/invalid.jsx (1)
3-53: Good baseline invalid coverage for default optionsThese fixtures neatly capture the core leaked-render cases (falsy primitives, length checks, nested collections, and fragment-wrapped blocks) that should be reported when both strategies are enabled. They line up well with the examples in the rule docs.
crates/biome_js_analyze/tests/specs/nursery/noLeakedRender/ternary/invalid.jsx (1)
1-50: Fixtures clearly express “ternary-only” expectationsThis file does a good job of asserting that all
&&-based boolean coercion patterns (including!!count, comparisons, and compound conditions) remain invalid whenvalidStrategiesis['ternary']. It pairs well withternary/valid.jsxand with the coerce-specific fixtures.crates/biome_js_analyze/src/lint/nursery/no_leaked_render.rs (2)
141-156: Semantic lookup helper is a nice touch
get_variable_from_contextwalking ancestor scopes to find bindings and then checking boolean literals is a solid way to avoid false positives for flag-style variables (as used in the Popover examples). This is a good balance between correctness and complexity for a lint rule.
245-264: Proposal oversimplifies ternary handling; risks breaking valid coerce fixturesYou're correct that
MyComponent3(checked={isIndeterminate ? false : isChecked}) isn't currently being flagged—its alternateisCheckeddoesn't match the literal-only check. However, the proposed fix (flagging all ternaries when"ternary"isn't in strategies) would also flagComponent4incoerce/valid.jsx(count ? <List /> : <EmptyList />), which is marked as valid under["coerce"].The actual distinction likely hinges on ternary context:
- Ternaries in JSX attributes (MyComponent3): intended as invalid under coerce
- Ternaries conditionally rendering JSX (Component4): intended as valid
Before merging this change, please:
- Run the full
noLeakedRendertest suite with the proposed fix applied- Confirm
MyComponent3now reports a diagnostic- Verify
coerce/valid.jsx(especiallyComponent4and Component2) remain diagnostics-free- If Component4 starts failing, refine the logic to distinguish attribute ternaries from JSX-rendering ternaries
crates/biome_js_analyze/tests/specs/nursery/noLeakedRender/coerce/invalid.jsx
Outdated
Show resolved
Hide resolved
|
These are some solid feedbacks. I will work on them tomorrow :) |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
crates/biome_js_analyze/src/lint/nursery/no_leaked_render.rs (2)
15-121: Rule docs andvalidStrategiessection still inconsistent and have minor formatting issuesThe doc block is clear overall, but the earlier concerns are still present:
- It calls ternary alternates like
null,undefined, orfalse“problematic”, yet immediately showsuser ? <Profile /> : nullas a valid pattern without mentioning thevalidStrategiesinteraction.- The
validStrategiessection focuses on how fixes transform code (“will transform…”, “transforms the binary expression…”) instead of clearly stating that it controls which syntactic guard patterns are treated as valid and which are reported, per the upstream rule.- Typo + formatting:
"ternay"→"ternary"in thevalidStrategiesdescription.- The stray closing code fence at Lines 118–119 will break rendered docs.
I’d recommend tightening this section to explicitly document:
- Default: flags unsafe
&&guards; treats both coerce-style conditions and ternary guards as valid when their strategies are enabled.- With
["coerce"]: ternary guards likecount ? title : nullare reported.- With
["ternary"]: coerce-style&&guards like!!count && titleare reported.And fix the typo and remove the extra ``` fence. The semantics section of the upstream
react/jsx-no-leaked-renderdocs is a good model to mirror.Confirm from the latest `eslint-plugin-react` documentation for `jsx-no-leaked-render` how `validStrategies` affects which patterns are considered valid vs. reported (beyond just controlling autofix behavior).
265-305: Diagnostic messages: spacing/capitalization/punctuation still need a quick polishThe diagnostics are helpful, but the earlier cosmetic issues remain:
"').React"should be"'). React"."boolean.Use"should be"boolean. Use"."This happens When"should be"This happens when"."null,undefined"should be"null, undefined".- The second note should end with a period for consistency.
Concretely:
- "JavaScript's && operator returns the left value when it's falsy (e.g., 0, NaN, '').React will render that value, causing unexpected UI output." + "JavaScript's && operator returns the left value when it's falsy (e.g., 0, NaN, ''). React will render that value, causing unexpected UI output." @@ - "Make sure the condition is explicitly boolean.Use !!value, value > 0, or a ternary expression." + "Make sure the condition is explicitly boolean. Use !!value, value > 0, or a ternary expression." @@ - "This happens When you use ternary operators in JSX with alternate values like null,undefined, or false" + "This happens when you use ternary operators in JSX with alternate values like null, undefined, or false." @@ - "Replace with a safe alternate value like an empty string or another JSX element" + "Replace it with a safe alternate value like an empty string or another JSX element."
🧹 Nitpick comments (1)
crates/biome_js_analyze/src/lint/nursery/no_leaked_render.rs (1)
132-153: Helper and defaults are reasonable; potential future guard on emptyvalidStrategies
COERCE_STRATEGY,TERNARY_STRATEGY,DEFAULT_VALID_STRATEGIES, andget_variable_from_contextare implemented cleanly and do what they need to (find a binding and check if it’s initialized with a literal boolean).One small edge case to keep in mind: if users configure
"validStrategies": [], the code will treat both strategies as disabled (contrary to the docs’ “at least 1 is required” note) and will effectively warn on all&&and relevant ternaries. If you want to enforce “non-empty” here rather than elsewhere, you could normalize an empty vector back toDEFAULT_VALID_STRATEGIES.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
crates/biome_configuration/src/generated/domain_selector.rsis excluded by!**/generated/**
📒 Files selected for processing (2)
crates/biome_cli/src/execute/migrate/eslint_any_rule_to_biome.rs(1 hunks)crates/biome_js_analyze/src/lint/nursery/no_leaked_render.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
crates/biome_cli/src/execute/migrate/eslint_any_rule_to_biome.rs (2)
packages/@biomejs/backend-jsonrpc/src/workspace.ts (1)
Nursery(1857-2107)crates/biome_configuration/src/analyzer/linter/rules.rs (1)
group(866-1251)
crates/biome_js_analyze/src/lint/nursery/no_leaked_render.rs (1)
packages/@biomejs/backend-jsonrpc/src/workspace.ts (2)
RuleDomain(976-984)NoLeakedRenderOptions(6249-6251)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Bench (biome_configuration)
- GitHub Check: Bench (biome_js_analyze)
- GitHub Check: autofix
- GitHub Check: Test Node.js API
- GitHub Check: Check JS Files
- GitHub Check: Test (depot-windows-2022-16)
- GitHub Check: Bench (biome_js_formatter)
- GitHub Check: Bench (biome_js_parser)
- GitHub Check: Test (depot-ubuntu-24.04-arm-16)
- GitHub Check: Check Dependencies
🔇 Additional comments (6)
crates/biome_cli/src/execute/migrate/eslint_any_rule_to_biome.rs (1)
2512-2523: LGTM! Migration logic correctly implemented.The migration path for
react/no-leaked-render→nursery.no_leaked_renderfollows the established pattern perfectly. The nursery check, group initialization, rule access, and level setting all match the conventions used throughout this file.Note: Since this is a generated file (line 1), ensure the source codegen has been properly updated to persist these changes.
crates/biome_js_analyze/src/lint/nursery/no_leaked_render.rs (5)
120-129: Rule metadata looks consistent with other React lint rules
version: "next",name: "noLeakedRender",language: "jsx",domains: &[RuleDomain::React], and theRuleSource::EslintReact("no-leaked-render")mapping all look correct and aligned with the upstream rule naming.
155-163: Query union and Rule type wiring look correctThe
declare_node_union!forQuery = JsLogicalExpression | JsConditionalExpressionand theRuleassociated types (Semantic<Query>,State = bool,Signals = Option<bool>,Options = NoLeakedRenderOptions) are wired correctly for the rule engine and match the intended scope of this lint.
165-241: Logical&&handling matches intended “coerce” strategy behaviorThe
JsLogicalExpressionbranch:
- Correctly filters to
&&only.- Respects
validStrategiesby only applying the coerce checks when"coerce"is present.- Treats unary/call/binary expressions and recursively nested logical expressions as “coerced”/safe.
- Treats identifiers initialized to literal
true/falseas safe.- Leaves genuinely unsafe guards as diagnostics.
This broadly aligns with the upstream rule’s intent of requiring either explicit coercion or ternary guards to avoid leaking falsy values like
0orNaN.No functional issues spotted here.
307-323: Nested logical-expression helper is correct and termination-safe
get_is_coerce_valid_nested_logical_expressioncorrectly:
- Recurses into nested
JsLogicalExpressionandJsParenthesizedExpression.- Treats unary/call/binary expressions as valid “coerce” leaves.
- Returns
falsefor everything else, ensuring the recursion always terminates.This lines up with the logic in the
&&branch and doesn’t present correctness or performance issues.
242-263: Ternary analysis misses problematic values in the consequent branch (known bug) and is a bit asymmetricIn the
JsConditionalExpressionbranch:
- The rule short-circuits when
"ternary"is invalid_strategies, which is correct for the default behavior.- When
"ternary"is not allowed (e.g.validStrategies: ["coerce"]), it only inspectsexpr.alternate()for"null" | "undefined" | "false".This means cases like the one called out in the PR body:
const MyComponent3 = () => { return <Something checked={isIndeterminate ? false : isChecked} />; };are never reported, because the “problematic” sentinel (
false) lives in the consequent, not the alternate. More generally, anytest ? false | null | undefined : valuepattern in JSX will be silently accepted under["coerce"], which diverges from the expectations for this port.A minimal fix is to treat both branches symmetrically and consider them problematic whenever they’re one of the sentinel values and not a JSX element, e.g.:
- let alternate = expr.alternate().ok()?; - let is_problematic_alternate = TERNARY_INVALID_ALTERNATE_VALUES - .iter() - .any(|&s| alternate.to_trimmed_text() == s); - - let is_jsx_element_alt = matches!(alternate, AnyJsExpression::JsxTagExpression(_)); - - if !is_problematic_alternate || is_jsx_element_alt { - return None; - } - - return Some(true); + let consequent = expr.consequent().ok()?; + let alternate = expr.alternate().ok()?; + + let is_problematic_branch = |expr: &AnyJsExpression| { + // JSX elements are always considered safe fallbacks. + if matches!(expr, AnyJsExpression::JsxTagExpression(_)) { + return false; + } + let text = expr.to_trimmed_text(); + TERNARY_INVALID_ALTERNATE_VALUES + .iter() + .any(|&sentinel| text == sentinel) + }; + + let has_problematic_branch = + is_problematic_branch(&consequent) || is_problematic_branch(&alternate); + + if !has_problematic_branch { + return None; + } + + Some(true)This should make
validStrategies: ["coerce"]flag bothcount ? title : null(as in the upstream docs) and attribute patterns likeisIndeterminate ? false : isChecked, while still allowing ternaries that return a genuine fallback value.
dyc3
left a comment
dyc3
left a comment
There was a problem hiding this comment.
This will be a great rule to have!
…eaked-conditional-rendering
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
crates/biome_js_analyze/src/lint/nursery/no_leaked_render.rs (2)
90-119:validStrategiesdocs: fix typo and stray code fenceThe
validStrategiessection still has the “ternay” typo and an extra ``` fence at Lines 118–119, which will break the rendered docs. While you’re here, it’d be good to tighten the wording so it matches what the rule actually does and the upstream rule’s intent.
291-296: Tidy up diagnostic note grammar / spacingMinor copy polish to make the diagnostics read cleanly:
- "JavaScript's && operator returns the left value when it's falsy (e.g., 0, NaN, '').React will render that value, causing unexpected UI output." + "JavaScript's && operator returns the left value when it's falsy (e.g., 0, NaN, ''). React will render that value, causing unexpected UI output." @@ - "Make sure the condition is explicitly boolean.Use !!value, value > 0, or a ternary expression." + "Make sure the condition is explicitly boolean. Use !!value, value > 0, or a ternary expression." @@ - "This happens When you use ternary operators in JSX with alternate values like null,undefined, or false" + "This happens when you use ternary operators in JSX with alternate values like null, undefined, or false." @@ - "Replace with a safe alternate value like an empty string or another JSX element" + "Replace it with a safe alternate value like an empty string or another JSX element."Also applies to: 308-313
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
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_diagnostics_categories/src/categories.rsis excluded by!**/categories.rsand included by**crates/biome_js_analyze/tests/specs/nursery/noLeakedRender/coerce/invalid.jsx.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 (2)
crates/biome_js_analyze/src/lint/nursery/no_leaked_render.rs(1 hunks)crates/biome_rule_options/src/lib.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/biome_rule_options/src/lib.rs
🧰 Additional context used
🧬 Code graph analysis (1)
crates/biome_js_analyze/src/lint/nursery/no_leaked_render.rs (1)
crates/biome_js_analyze/src/services/semantic_class.rs (1)
semantic(43-45)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: Bench (biome_configuration)
- GitHub Check: Check Dependencies
- GitHub Check: Documentation
- GitHub Check: Test (depot-windows-2022-16)
- GitHub Check: Test (depot-ubuntu-24.04-arm-16)
- GitHub Check: End-to-end tests
- GitHub Check: Check JS Files
- GitHub Check: Bench (biome_js_parser)
- GitHub Check: autofix
- GitHub Check: Test Node.js API
- GitHub Check: Bench (biome_js_analyze)
- GitHub Check: Bench (biome_js_formatter)
ematipico
left a comment
ematipico
left a comment
There was a problem hiding this comment.
Cheers! I found some code that we should optimise. Particuarly, we should remove the use of the recursion
- added docstrings - removed unnecessary functions and replaced with inline code
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
crates/biome_js_analyze/src/lint/nursery/no_leaked_render.rs (1)
245-245: Missing space after full stop in diagnostic note.Line 245:
"boolean.Use"should be"boolean. Use"(add a space after the period).This was flagged in a previous review but appears to still be present in the code.
Apply this diff:
- "Make sure the condition is explicitly boolean.Use !!value, value > 0, or a ternary expression." + "Make sure the condition is explicitly boolean. Use !!value, value > 0, or a ternary expression."
🧹 Nitpick comments (4)
crates/biome_js_analyze/src/lint/nursery/no_leaked_render.rs (4)
207-210: Unreachable literal check or unclear intent.At line 208, you check if a literal's trimmed text is empty and return
Noneif so. However, it's unclear what AST literal nodes would have empty text representation—most literals (0,null,"", etc.) have non-empty text. This either handles an edge case that needs a comment, or is dead code that can be removed.Could you clarify the intent or add a comment explaining which literals this guards against?
178-205: Consider reducing nesting withif letchains or extracting a helper.The semantic model resolution at lines 178-205 uses a long chain of
and_thencalls with nested closures. This is functional but hard to scan. Rust'sif letchains (or a small helper function) would flatten the control flow and make the boolean literal check more readable.For example:
if let Some(binding) = model.binding(&name) && let Some(declaration) = binding.tree().declaration() && let AnyJsBindingDeclaration::JsVariableDeclarator(declarator) = declaration && let Some(initializer) = declarator.initializer() && let Ok(expr) = initializer.expression() && let AnyJsExpression::AnyJsLiteralExpression(literal) = expr && let Ok(token) = literal.value_token() && matches!(token.text_trimmed(), "true" | "false") { return None; }Non-blocking, but improves clarity.
216-221: Clarify ternary alternate logic with a comment or variable.Lines 216-219 define two booleans and then return
Noneif!is_alternate_identifier || is_jsx_element_alt. The double-negative (!is_alternate_identifier OR ...) makes it slightly harder to reason about at a glance.Consider either:
- Adding a comment explaining we only flag identifiers that aren't JSX elements, or
- Inverting to
if is_alternate_identifier && !is_jsx_element_alt { Some(true) } else { None }to make the intent clearer.
270-276: Add a docstring to explain the function's purpose.As noted in previous review feedback, helper functions should have docstrings explaining their business logic. Please add a doc comment describing that this function checks whether a node appears within a JSX expression context (as a child, attribute value, or tag expression) by walking the ancestor chain.
Based on past feedback from maintainer.
Example:
/// Returns true if the given node is used within a JSX expression context. /// /// This checks whether the node appears as: /// - A JSX expression child (e.g., `{expr}` inside JSX) /// - A JSX expression attribute value (e.g., `attr={expr}`) /// - A JSX tag expression fn is_inside_jsx_expression(node: &JsSyntaxNode) -> bool {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.changeset/calm-shrimps-study.md(1 hunks)crates/biome_js_analyze/src/lint/nursery/no_leaked_render.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .changeset/calm-shrimps-study.md
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-21T01:10:53.036Z
Learnt from: dyc3
Repo: biomejs/biome PR: 8171
File: crates/biome_js_analyze/src/lint/nursery/no_leaked_render.rs:125-137
Timestamp: 2025-11-21T01:10:53.036Z
Learning: In the Biome codebase, each lint rule has its own options type declaration (e.g., `type Options = RuleNameOptions`) as part of the codegen process, even if the options struct is empty or unused. This is standard practice and should not be flagged as an issue.
Applied to files:
crates/biome_js_analyze/src/lint/nursery/no_leaked_render.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: Bench (biome_js_formatter)
- GitHub Check: Bench (biome_js_parser)
- GitHub Check: Bench (biome_js_analyze)
- GitHub Check: autofix
- GitHub Check: Bench (biome_configuration)
- GitHub Check: Lint project (depot-windows-2022)
- GitHub Check: Test (depot-ubuntu-24.04-arm-16)
- GitHub Check: Test (depot-windows-2022-16)
- GitHub Check: Lint project (depot-ubuntu-24.04-arm-16)
- GitHub Check: Documentation
- GitHub Check: Check Dependencies
- GitHub Check: Test Node.js API
- GitHub Check: Check JS Files
| "This happens when you use ternary operators in JSX with alternate values that could be variables" | ||
| }) | ||
| .note(markup! { | ||
| "Replace with a safe alternate value like an empty string , null or another JSX element" |
There was a problem hiding this comment.
Fix spacing and punctuation in diagnostic note.
Line 262 has two minor issues:
- Extra space before comma:
"string , null"should be"string, null" - Missing terminal punctuation at the end
Apply this diff:
- "Replace with a safe alternate value like an empty string , null or another JSX element"
+ "Replace with a safe alternate value like an empty string, null, or another JSX element."📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "Replace with a safe alternate value like an empty string , null or another JSX element" | |
| "Replace with a safe alternate value like an empty string, null, or another JSX element." |
🤖 Prompt for AI Agents
In crates/biome_js_analyze/src/lint/nursery/no_leaked_render.rs around line 262,
fix the diagnostic note string by removing the extra space before the comma and
adding terminal punctuation: change `"Replace with a safe alternate value like
an empty string , null or another JSX element"` to `"Replace with a safe
alternate value like an empty string, null, or another JSX element."` so spacing
and punctuation are correct.
ematipico
left a comment
ematipico
left a comment
There was a problem hiding this comment.
Awesome thank you! I think we're good. I left one minor improvement
| @@ -0,0 +1,70 @@ | |||
| // Invalid cases with default options (both 'coerce' and 'ternary' strategies) | |||
There was a problem hiding this comment.
| // Invalid cases with default options (both 'coerce' and 'ternary' strategies) | |
| // should generate diagnostics | |
| // Invalid cases with default options (both 'coerce' and 'ternary' strategies) |
FYI If you accept, this will fail the snapshot
There was a problem hiding this comment.
I have removed my old comment and replaced with should generate diagnostics
| AnyJsExpression::JsLogicalExpression(expr) => { | ||
| let left = expr.left().ok()?; | ||
| let right = expr.right().ok()?; | ||
| stack.push(left); | ||
| stack.push(right); | ||
| } | ||
| AnyJsExpression::JsParenthesizedExpression(expr) => { | ||
| stack.push(expr.expression().ok()?); | ||
| } |
There was a problem hiding this comment.
One last thing. If left, right and expression() are AnyJsExpression, I suggest you go call omit_parentheses(), so we remove the parentheses.
In fact, we have a few tests with binary expressions that contain parentheses. Can we add more of them? Let's get creative and add absurd cases, e.g.:
const Component1 = ({ count, title }) => {
return <div>{(((((count))))) && ((title))}</div>;
};There was a problem hiding this comment.
I tried adding the code you suggested, but parentheses were automatically omitted by the formatter, haha
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
crates/biome_js_analyze/tests/specs/nursery/noLeakedRender/invalid.jsx (1)
1-2: Consider a more descriptive comment.The comment could specify that these are invalid cases for the
noLeakedRenderrule, helping future contributors understand the test's purpose.-// should generate diagnostics +// Invalid cases: should generate noLeakedRender diagnostics
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
crates/biome_js_analyze/tests/specs/nursery/noLeakedRender/invalid.jsx.snapis excluded by!**/*.snapand included by**
📒 Files selected for processing (1)
crates/biome_js_analyze/tests/specs/nursery/noLeakedRender/invalid.jsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
crates/biome_js_analyze/tests/specs/nursery/noLeakedRender/invalid.jsx (1)
crates/biome_js_analyze/tests/specs/nursery/noLeakedRender/valid.jsx (8)
Component1(2-4)Component2(6-8)Component3(10-12)Component4(14-16)Component5(18-20)Component6(22-24)isOpen1(66-66)Component7(26-28)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: Check JS Files
- GitHub Check: Bench (biome_js_analyze)
- GitHub Check: Bench (biome_js_parser)
- GitHub Check: Bench (biome_js_formatter)
- GitHub Check: Lint project (depot-ubuntu-24.04-arm-16)
- GitHub Check: Documentation
- GitHub Check: Test (depot-ubuntu-24.04-arm-16)
- GitHub Check: Check Dependencies
- GitHub Check: Test (depot-windows-2022-16)
- GitHub Check: Lint project (depot-windows-2022)
- GitHub Check: Bench (biome_configuration)
- GitHub Check: Test Node.js API
- GitHub Check: autofix
🔇 Additional comments (3)
crates/biome_js_analyze/tests/specs/nursery/noLeakedRender/invalid.jsx (3)
3-11: LGTM!Clean test of literal falsy values that would leak into the DOM.
13-37: LGTM!These components properly demonstrate leaked render patterns with props.
59-65: The unbound variables are intentional test fixtures, not defects.The entire test file deliberately uses unbound identifiers (
someCondition,maybeObject,elements, etc.) to trigger the rule's diagnostics. MyComponent4–5 use ternary patterns whilst others use AND logic, but mixing strategies within a single test file is acceptable—they all target the same rule with different code patterns. No fixes required; the code is fit for purpose.Likely an incorrect or invalid review comment.
| const MyComponent1 = () => { | ||
| return ( | ||
| <> | ||
| {someCondition && ( | ||
| <div> | ||
| <p>hello</p> | ||
| </div> | ||
| )} | ||
| </> | ||
| ); | ||
| }; | ||
|
|
||
| const MyComponent2 = () => { | ||
| return <>{someCondition && <SomeComponent prop1={val1} prop2={val2} />}</>; | ||
| }; | ||
|
|
||
| const MyComponent3 = () => { | ||
| return <div>{maybeObject && (isFoo ? <Aaa /> : <Bbb />)}</div>; | ||
| }; |
There was a problem hiding this comment.
Unbound identifiers prevent semantic testing.
someCondition, val1, val2, maybeObject, and isFoo are all undefined. Based on the past review of Component7, unbound identifiers cause the rule to diagnose the wrong issue (undefined reference) rather than testing the intended leaked-render pattern. Define these as constants or props to properly exercise semantic lookup.
+// Define test values to exercise semantic patterns
+const someCondition = 0; // or other falsy value
+const val1 = '';
+const val2 = NaN;
+const maybeObject = null;
+const isFoo = false;
+
const MyComponent1 = () => {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const MyComponent1 = () => { | |
| return ( | |
| <> | |
| {someCondition && ( | |
| <div> | |
| <p>hello</p> | |
| </div> | |
| )} | |
| </> | |
| ); | |
| }; | |
| const MyComponent2 = () => { | |
| return <>{someCondition && <SomeComponent prop1={val1} prop2={val2} />}</>; | |
| }; | |
| const MyComponent3 = () => { | |
| return <div>{maybeObject && (isFoo ? <Aaa /> : <Bbb />)}</div>; | |
| }; | |
| // Define test values to exercise semantic patterns | |
| const someCondition = 0; // or other falsy value | |
| const val1 = ''; | |
| const val2 = NaN; | |
| const maybeObject = null; | |
| const isFoo = false; | |
| const MyComponent1 = () => { | |
| return ( | |
| <> | |
| {someCondition && ( | |
| <div> | |
| <p>hello</p> | |
| </div> | |
| )} | |
| </> | |
| ); | |
| }; | |
| const MyComponent2 = () => { | |
| return <>{someCondition && <SomeComponent prop1={val1} prop2={val2} />}</>; | |
| }; | |
| const MyComponent3 = () => { | |
| return <div>{maybeObject && (isFoo ? <Aaa /> : <Bbb />)}</div>; | |
| }; |
🤖 Prompt for AI Agents
In crates/biome_js_analyze/tests/specs/nursery/noLeakedRender/invalid.jsx around
lines 39 to 57, unbound identifiers (someCondition, val1, val2, maybeObject,
isFoo) cause the test to fail semantic lookup and produce undefined-reference
diagnostics; fix by defining those identifiers (e.g., add const declarations at
the top of the test file or accept them as component props and pass mock values)
so the components compile and the leaked-render rule is exercised — add minimal
stable definitions (booleans/values or mocked objects) before the components to
ensure semantic testing targets the intended leaked-render pattern.
| const isOpen1 = 0; | ||
| const Component7 = () => { | ||
| return <Popover open={isOpen1 && items.length > 0} />; | ||
| }; |
There was a problem hiding this comment.
Unbound items identifier.
Whilst the isOpen1 binding is now correct, items remains undefined. This prevents the rule from testing the intended semantic pattern (constant zero in a logical AND). Define items to complete the fix from the previous review.
const isOpen1 = 0;
+const items = [1, 2, 3]; // or appropriate test data
const Component7 = () => {
return <Popover open={isOpen1 && items.length > 0} />;
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const isOpen1 = 0; | |
| const Component7 = () => { | |
| return <Popover open={isOpen1 && items.length > 0} />; | |
| }; | |
| const isOpen1 = 0; | |
| const items = [1, 2, 3]; // or appropriate test data | |
| const Component7 = () => { | |
| return <Popover open={isOpen1 && items.length > 0} />; | |
| }; |
🤖 Prompt for AI Agents
In crates/biome_js_analyze/tests/specs/nursery/noLeakedRender/invalid.jsx around
lines 67-70, the JSX uses an unbound identifier `items`; add a local binding for
`items` (for example an array or whatever shape the tests expect) declared
before Component7 so `items` is in scope when evaluating `isOpen1 &&
items.length > 0`, keeping the test focused on the constant-zero logical AND
case.
- Added two test case with parentheses - Added method to remove parentheses
…eaked-conditional-rendering
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
crates/biome_js_analyze/src/lint/nursery/no_leaked_render.rs (2)
258-258: Fix spacing and add terminal punctuation.Line 258 has an extra space before the comma (
"string , null") and is missing a full stop at the end.Apply this diff:
- "Replace with a safe alternate value like an empty string , null or another JSX element" + "Replace with a safe alternate value like an empty string, null, or another JSX element."
241-241: Missing space after full stop in diagnostic message.Line 241 has
"boolean.Use"which should be"boolean. Use"for proper spacing.Apply this diff:
- "Make sure the condition is explicitly boolean.Use !!value, value > 0, or a ternary expression." + "Make sure the condition is explicitly boolean. Use !!value, value > 0, or a ternary expression."
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
crates/biome_js_analyze/tests/specs/nursery/noLeakedRender/invalid.jsx.snapis excluded by!**/*.snapand included by**
📒 Files selected for processing (2)
crates/biome_js_analyze/src/lint/nursery/no_leaked_render.rs(1 hunks)crates/biome_js_analyze/tests/specs/nursery/noLeakedRender/invalid.jsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/biome_js_analyze/tests/specs/nursery/noLeakedRender/invalid.jsx
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-10-25T07:22:18.540Z
Learnt from: ematipico
Repo: biomejs/biome PR: 7852
File: crates/biome_css_parser/src/syntax/property/mod.rs:161-168
Timestamp: 2025-10-25T07:22:18.540Z
Learning: In the Biome CSS parser, lexer token emission should not be gated behind parser options like `is_tailwind_directives_enabled()`. The lexer must emit correct tokens regardless of parser options to enable accurate diagnostics and error messages when the syntax is used incorrectly.
Applied to files:
crates/biome_js_analyze/src/lint/nursery/no_leaked_render.rs
📚 Learning: 2025-11-21T01:10:53.036Z
Learnt from: dyc3
Repo: biomejs/biome PR: 8171
File: crates/biome_js_analyze/src/lint/nursery/no_leaked_render.rs:125-137
Timestamp: 2025-11-21T01:10:53.036Z
Learning: In the Biome codebase, each lint rule has its own options type declaration (e.g., `type Options = RuleNameOptions`) as part of the codegen process, even if the options struct is empty or unused. This is standard practice and should not be flagged as an issue.
Applied to files:
crates/biome_js_analyze/src/lint/nursery/no_leaked_render.rs
|
@ematipico i have added new test cases with nested parentheses, and also used |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
crates/biome_js_analyze/src/lint/nursery/no_leaked_render.rs (1)
238-242: Tidy spacing and punctuation in diagnostic notesTwo tiny text nits still sneaked through:
- Missing space after the full stop:
"boolean.Use".- Extra space before the comma and slightly awkward list:
"empty string , null or another JSX element.".A small tweak makes the messages read more naturally:
- .note(markup! { - "Make sure the condition is explicitly boolean.Use !!value, value > 0, or a ternary expression." - }) + .note(markup! { + "Make sure the condition is explicitly boolean. Use !!value, value > 0, or a ternary expression." + }) @@ - .note(markup! { - "Replace with a safe alternate value like an empty string , null or another JSX element." - }) + .note(markup! { + "Replace with a safe alternate value like an empty string, null, or another JSX element." + })Also applies to: 254-259
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/biome_js_analyze/src/lint/nursery/no_leaked_render.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-10-25T07:22:18.540Z
Learnt from: ematipico
Repo: biomejs/biome PR: 7852
File: crates/biome_css_parser/src/syntax/property/mod.rs:161-168
Timestamp: 2025-10-25T07:22:18.540Z
Learning: In the Biome CSS parser, lexer token emission should not be gated behind parser options like `is_tailwind_directives_enabled()`. The lexer must emit correct tokens regardless of parser options to enable accurate diagnostics and error messages when the syntax is used incorrectly.
Applied to files:
crates/biome_js_analyze/src/lint/nursery/no_leaked_render.rs
📚 Learning: 2025-11-21T01:10:53.036Z
Learnt from: dyc3
Repo: biomejs/biome PR: 8171
File: crates/biome_js_analyze/src/lint/nursery/no_leaked_render.rs:125-137
Timestamp: 2025-11-21T01:10:53.036Z
Learning: In the Biome codebase, each lint rule has its own options type declaration (e.g., `type Options = RuleNameOptions`) as part of the codegen process, even if the options struct is empty or unused. This is standard practice and should not be flagged as an issue.
Applied to files:
crates/biome_js_analyze/src/lint/nursery/no_leaked_render.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Check JS Files
- GitHub Check: Test Node.js API
- GitHub Check: autofix
- GitHub Check: Bench (biome_js_formatter)
- GitHub Check: Bench (biome_js_analyze)
- GitHub Check: Bench (biome_js_parser)
- GitHub Check: Bench (biome_configuration)
🔇 Additional comments (1)
crates/biome_js_analyze/src/lint/nursery/no_leaked_render.rs (1)
16-96: Rule definition and docs look solidNice, the rule metadata and examples line up with the implemented behaviour (JSX‑only, React domain,
.inspired()rather than.same()with ESLint). The invalid/valid snippets read clearly and match what the linter is looking for.
| let left = exp.left().ok()?; | ||
|
|
||
| let is_left_hand_side_safe = matches!( | ||
| left, | ||
| AnyJsExpression::JsUnaryExpression(_) | ||
| | AnyJsExpression::JsCallExpression(_) | ||
| | AnyJsExpression::JsBinaryExpression(_) | ||
| ); | ||
|
|
||
| if is_left_hand_side_safe { | ||
| return None; | ||
| } | ||
|
|
||
| let mut is_nested_left_hand_side_safe = false; | ||
|
|
||
| let mut stack = vec![left.clone()]; | ||
|
|
||
| // Traverse the expression tree iteratively using a stack | ||
| // This allows us to check nested expressions without recursion | ||
| while let Some(current) = stack.pop() { | ||
| match current { | ||
| AnyJsExpression::JsLogicalExpression(expr) => { | ||
| let left = expr.left().ok()?.omit_parentheses(); | ||
| let right = expr.right().ok()?.omit_parentheses(); | ||
| stack.push(left); | ||
| stack.push(right); | ||
| } | ||
| AnyJsExpression::JsParenthesizedExpression(expr) => { | ||
| stack.push(expr.expression().ok()?.omit_parentheses()); | ||
| } | ||
| // If we find expressions that coerce to boolean (unary, call, binary), | ||
| // then the entire expression is considered safe | ||
| AnyJsExpression::JsUnaryExpression(_) | ||
| | AnyJsExpression::JsCallExpression(_) | ||
| | AnyJsExpression::JsBinaryExpression(_) => { | ||
| is_nested_left_hand_side_safe = true; | ||
| break; | ||
| } | ||
| _ => {} | ||
| } | ||
| } | ||
|
|
||
| if is_nested_left_hand_side_safe { | ||
| return None; | ||
| } | ||
|
|
||
| if let AnyJsExpression::JsIdentifierExpression(ident) = &left { | ||
| let name = ident.name().ok()?; | ||
|
|
||
| // Use the semantic model to resolve the variable binding and check | ||
| // if it's initialized with a boolean literal. This allows us to | ||
| // handle cases like: | ||
| // let isOpen = false; // This is safe | ||
| // return <div>{isOpen && <Content />}</div>; // This should pass | ||
| if let Some(binding) = model.binding(&name) | ||
| && binding | ||
| .tree() | ||
| .declaration() | ||
| .and_then(|declaration| { | ||
| if let AnyJsBindingDeclaration::JsVariableDeclarator(declarator) = | ||
| declaration | ||
| { | ||
| Some(declarator) | ||
| } else { | ||
| None | ||
| } | ||
| }) | ||
| .and_then(|declarator| declarator.initializer()) | ||
| .and_then(|initializer| initializer.expression().ok()) | ||
| .and_then(|expr| { | ||
| if let AnyJsExpression::AnyJsLiteralExpression(literal) = expr { | ||
| Some(literal) | ||
| } else { | ||
| None | ||
| } | ||
| }) | ||
| .and_then(|literal| literal.value_token().ok()) | ||
| .is_some_and(|token| matches!(token.text_trimmed(), "true" | "false")) | ||
| { | ||
| return None; | ||
| } | ||
| } | ||
|
|
||
| let is_literal = matches!(left, AnyJsExpression::AnyJsLiteralExpression(_)); | ||
| if is_literal && left.to_trimmed_text().is_empty() { | ||
| return None; | ||
| } | ||
|
|
There was a problem hiding this comment.
Normalise the left‑hand side with omit_parentheses() to avoid false positives
At the moment left is taken as‑is from the AST, so cases like:
const isOpen = false;
return <div>{(isOpen) && <Content />}</div>;will be treated differently from isOpen && <Content /> even though they’re semantically identical. The same applies to parenthesised literals.
You can fix this by stripping parentheses once up front:
- let left = exp.left().ok()?;
+ let left = exp.left().ok()?.omit_parentheses();This will also make the identifier, literal, and is_literal checks behave consistently for expressions wrapped in extra brackets.
…ejs#8171) Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> Co-authored-by: Emanuele Stoppa <[email protected]>
|
This is an amazing new rule to have 💯 thank you so much! |
4 participants
Yes, i used claude and chatgpt to help me with
I only used them as assistant and not for writing the feature on their own.
Summary
This PR ports the
no-leaked-renderrule from Eslint ReactFixes: #7659
Test Plan
I have added test cases based on validStrategies. Test cases for
coerceis inside coerce folder andternaryis inside ternary folder.But there is one code that is not throwing diagnositcs, and am still looking into it. It should throw error when the validStrategy is
["coerce"]Docs