feat(js_analyze): implement react leaked rules#10175
Draft
Netail wants to merge 3 commits into
Draft
Conversation
🦋 Changeset detectedLatest commit: 51dbcf3 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-Linter
Merging this PR will not alter performance
Comparing 
Footnotes
|
Netail
force-pushed
the
feat/react-leaked-rules
branch
3 times, most recently
from
33dcfeb to
2871ff4
Compare
Netail
force-pushed
the
feat/react-leaked-rules
branch
from
2871ff4 to
0112fc8
Compare
Contributor
WalkthroughThis pull request adds five new nursery-level lint rules to Biome that enforce proper cleanup of asynchronous and observer APIs in React components: Possibly Related PRs
Suggested Reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Review rate limit: 4/5 reviews remaining, refill in 12 minutes. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (1)
.changeset/no-react-leaked-intersection-observer.md (1)
5-5: ⚡ Quick winConsider “reports” instead of “enforces” for accuracy.
This phrasing may imply error-level enforcement; for nursery rules, default severity is informational unless configured otherwise, so “reports” is usually clearer for users.
Based on learnings: when a lint rule does not explicitly set severity in
declare_lint_rule!, the default isSeverity::Information.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.changeset/no-react-leaked-intersection-observer.md at line 5, Change the wording in the changelog entry for the new nursery rule noReactLeakedIntersectionObserver from “enforces” to “reports” to accurately reflect that nursery rules default to informational severity; mention that unless the rule sets a severity in declare_lint_rule! it will be reported at Severity::Information so users understand the rule is informational by default.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.changeset/no-react-leaked-resize-observer.md:
- Line 5: Update the changeset wording to match the implemented rule
noReactLeakedResizeObserver by stating that cleanup must call either disconnect
or unobserve on the ResizeObserver (not only disconnect), and rephrase the
message to be user-facing and concise (e.g., "Ensure ResizeObservers in React
components/hooks are cleaned up with disconnect or unobserve to avoid memory
leaks"). Also ensure the changeset follows the project's guideline of focusing
only on user-facing changes and remove implementation detail language.
In @.changeset/no-react-leaked-timeout.md:
- Line 5: The changelog sentence overstates the rule scope; update the line
referencing the new nursery rule noReactLeakedTimeout so it explicitly says the
rule enforces that every setTimeout inside useEffect callbacks (in components or
custom hooks) must have a corresponding clearTimeout in the effect cleanup,
rather than saying “every setTimeout in a React component or custom hook.” Edit
the .changeset/no-react-leaked-timeout.md diff text to replace the broader
phrase with “inside useEffect callbacks” and keep the link and rationale intact.
In `@crates/biome_js_analyze/src/lint/nursery/no_react_leaked_event_listener.rs`:
- Around line 79-125: The scan is too broad: local_add_entries currently
collects addEventListener calls from all descendants of the effect callback and
remove_entries collects removes from any effect in the same owner scope, causing
false positives/negatives. Limit the add collection to the current effect’s
executed body (not nested helper declarations) by scanning only the callback’s
executed body/statements and returned cleanup by parsing the callback’s return
expression (the cleanup function) and scanning its descendants for
EventListenerMethod::Remove entries; replace the cross-effect loop that builds
remove_entries (root.descendants()/is_effect_call/enclosing_owner_scope_range)
with a parse of the current effect’s returned cleanup function and use
extract_event_listener_entry on that AST subtree, keeping identifiers
effect_callback, extract_event_listener_entry, EventListenerMethod::Add/Remove,
local_add_entries, and remove_entries to locate the changes.
In
`@crates/biome_js_analyze/src/lint/nursery/no_react_leaked_intersection_observer.rs`:
- Around line 100-149: The analysis is currently global per effect: the loop
over observer_ranges replays all observe/unobserve/disconnect calls for every
observer, so calls for observerA can incorrectly clear observerB; scope the
analysis to only calls that target the specific observer represented by
observer_range by filtering callback_node.descendants() (and observer_call_kind
results) to the matching observer identifier/target for that observer_range
before counting; update the logic in the observer_ranges loop (references:
observer_ranges, callback_node.descendants(), observer_call_kind,
is_dynamic_observe_call, observe_count/unobserve_count, has_disconnect, and
NoReactLeakedIntersectionObserverState) to compute counts per-observer and to
correlate returned cleanup with the matching observer; finally add regression
snapshot tests for multi-observer effects and wrong-target cleanup under
tests/specs/{group}/{rule}/ as required.
In `@crates/biome_js_analyze/src/lint/nursery/no_react_leaked_interval.rs`:
- Around line 158-199: The helper has_clear_interval_in_cleanup currently scans
the whole effect callback; restrict it to only inspect clearInterval calls
inside the returned cleanup function: locate JsReturnStatement nodes inside
callback_node, get each return's argument expression, and if that argument is a
JsFunctionExpression or JsArrowFunctionExpression (or a parenthesized wrapper),
descend into that function's body and run the existing clearInterval detection
logic there (handle arrow-expression bodies and block bodies). Keep the same
var_name matching behavior, and add a snapshot regression test "cleared outside
cleanup" under tests/specs/{group}/{rule}/ for this rule.
In `@crates/biome_js_analyze/src/lint/nursery/no_react_leaked_resize_observer.rs`:
- Around line 78-149: The analysis currently saves only observer_ranges and
re-scans the entire callback for each observer which allows one observer's
disconnect or unrelated objects to affect others; instead capture the observer's
bound identifier/variable declarator when you find the new ResizeObserver (use
the JsVariableDeclarator ancestor of is_new_resize_observer/new expression) and
store that binding (e.g., the JsIdentifier or the declarator node) alongside its
range; then when scanning calls use observer_call_kind/JsCallExpression but
additionally verify the call's receiver resolves to that specific binding
(compare the member expression/object node or resolved binding node) and only
count observe/unobserve/disconnect for that bound observer; return
NoReactLeakedResizeObserverState with the stored observer binding/range when
that specific observer fails the checks.
- Around line 208-219: observer_call_kind currently only strips parentheses then
tries as_js_static_member_expression, so casted forms like (observer.observe as
any)(...) are missed; update observer_call_kind to repeatedly unwrap TS/JS "as"
casts and parenthesized expressions from the callee (after the initial
omit_parentheses()) until you reach a JsStaticMemberExpression, then extract
member.member().value_token().text_trimmed() as before (i.e. keep the match on
"observe"/"unobserve"/"disconnect"); refer to the observer_call_kind function,
the callee variable, and the member_name extraction when making this change.
In `@crates/biome_js_analyze/src/lint/nursery/no_react_leaked_timeout.rs`:
- Around line 158-199: The function has_clear_timeout_in_cleanup currently scans
all descendants of the effect callback (callback_node), which can wrongly pick
up clearTimeout calls outside the returned cleanup; change it to locate the
actual returned cleanup function/body and only search that node for
JsCallExpression with callee name "clearTimeout" (e.g., inspect return
statements or arrow-function concise bodies returned by the callback, unwrap the
returned function expression before calling descendants()), keep the existing
arg-name matching/unwrapping logic (unwrap_typescript_expression) when var_name
is provided, and add a snapshot spec fixture under
tests/specs/nursery/no_react_leaked_timeout/ demonstrating a case where
clearTimeout is called outside the cleanup (so the rule still triggers) to
prevent regressions.
In `@crates/biome_js_analyze/src/react.rs`:
- Around line 359-373: is_effect_call currently treats any member named
useEffect/useLayoutEffect/useInsertionEffect as a React hook; change it to only
match genuine React hooks by rejecting member expressions and resolving the
callee binding: first ensure the call's callee is an Identifier (instead of
using callee.get_callee_member_name()), then resolve that identifier's
binding/definition via your semantic resolver/scope (the same resolver used
elsewhere in the crate) and only return true if the identifier name is one of
"useEffect" | "useLayoutEffect" | "useInsertionEffect" and its resolved binding
comes from the React import (e.g., a named import from "react" or a local
binding that originates from that import); leave calls like foo.useEffect(...)
and other non-React bindings unmatched.
---
Nitpick comments:
In @.changeset/no-react-leaked-intersection-observer.md:
- Line 5: Change the wording in the changelog entry for the new nursery rule
noReactLeakedIntersectionObserver from “enforces” to “reports” to accurately
reflect that nursery rules default to informational severity; mention that
unless the rule sets a severity in declare_lint_rule! it will be reported at
Severity::Information so users understand the rule is informational by default.
🪄 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: d2f11dd5-ef94-4f47-a701-b35994c6eff1
⛔ Files ignored due to path filters (17)
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/domain_selector.rsis excluded by!**/generated/**,!**/generated/**and 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/noReactLeakedEventListener/invalid.tsx.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/noReactLeakedEventListener/valid.tsx.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/noReactLeakedIntersectionObserver/invalid.tsx.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/noReactLeakedIntersectionObserver/valid.tsx.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/noReactLeakedInterval/invalid.tsx.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/noReactLeakedInterval/valid.tsx.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/noReactLeakedResizeObserver/invalid.tsx.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/noReactLeakedResizeObserver/valid.tsx.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/noReactLeakedTimeout/invalid.tsx.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/noReactLeakedTimeout/valid.tsx.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 (30)
.changeset/no-react-leaked-event-listener.md.changeset/no-react-leaked-intersection-observer.md.changeset/no-react-leaked-interval.md.changeset/no-react-leaked-resize-observer.md.changeset/no-react-leaked-timeout.mdcrates/biome_analyze/src/rule.rscrates/biome_js_analyze/src/lib.rscrates/biome_js_analyze/src/lint/nursery/no_react_leaked_event_listener.rscrates/biome_js_analyze/src/lint/nursery/no_react_leaked_intersection_observer.rscrates/biome_js_analyze/src/lint/nursery/no_react_leaked_interval.rscrates/biome_js_analyze/src/lint/nursery/no_react_leaked_resize_observer.rscrates/biome_js_analyze/src/lint/nursery/no_react_leaked_timeout.rscrates/biome_js_analyze/src/react.rscrates/biome_js_analyze/src/typescript.rscrates/biome_js_analyze/tests/specs/nursery/noReactLeakedEventListener/invalid.tsxcrates/biome_js_analyze/tests/specs/nursery/noReactLeakedEventListener/valid.tsxcrates/biome_js_analyze/tests/specs/nursery/noReactLeakedIntersectionObserver/invalid.tsxcrates/biome_js_analyze/tests/specs/nursery/noReactLeakedIntersectionObserver/valid.tsxcrates/biome_js_analyze/tests/specs/nursery/noReactLeakedInterval/invalid.tsxcrates/biome_js_analyze/tests/specs/nursery/noReactLeakedInterval/valid.tsxcrates/biome_js_analyze/tests/specs/nursery/noReactLeakedResizeObserver/invalid.tsxcrates/biome_js_analyze/tests/specs/nursery/noReactLeakedResizeObserver/valid.tsxcrates/biome_js_analyze/tests/specs/nursery/noReactLeakedTimeout/invalid.tsxcrates/biome_js_analyze/tests/specs/nursery/noReactLeakedTimeout/valid.tsxcrates/biome_rule_options/src/lib.rscrates/biome_rule_options/src/no_react_leaked_event_listener.rscrates/biome_rule_options/src/no_react_leaked_intersection_observer.rscrates/biome_rule_options/src/no_react_leaked_interval.rscrates/biome_rule_options/src/no_react_leaked_resize_observer.rscrates/biome_rule_options/src/no_react_leaked_timeout.rs
| "@biomejs/biome": patch | ||
| --- | ||
|
|
||
| Added the new nursery rule [`noReactLeakedResizeObserver`](https://biomejs.dev/linter/rules/no-react-leaked-resize-observer) rule. This rule enforces that every `ResizeObserver` in a React component or custom hook has a corresponding `disconnect` in the cleanup function of `useEffect`. Forgetting to disconnect an observer can lead to memory leaks and unexpected behavior after component unmounts or dependency changes. |
Contributor
There was a problem hiding this comment.
Changeset wording is stricter than the implemented rule.
The text says cleanup must include disconnect, but the rule also accepts matching unobserve cleanup. Please align wording with actual behaviour.
Suggested wording
-Added the new nursery rule [`noReactLeakedResizeObserver`](https://biomejs.dev/linter/rules/no-react-leaked-resize-observer) rule. This rule enforces that every `ResizeObserver` in a React component or custom hook has a corresponding `disconnect` in the cleanup function of `useEffect`. Forgetting to disconnect an observer can lead to memory leaks and unexpected behavior after component unmounts or dependency changes.
+Added the new nursery rule [`noReactLeakedResizeObserver`](https://biomejs.dev/linter/rules/no-react-leaked-resize-observer). This rule enforces that every `ResizeObserver` in a React component or custom hook is properly cleaned up in `useEffect` (for example via `disconnect`, or equivalent `unobserve` cleanup). Missing cleanup can lead to memory leaks and unexpected behaviour after unmounts or dependency changes.As per coding guidelines, "Write changesets with focus on user-facing changes only".
📝 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.
Suggested change
| Added the new nursery rule [`noReactLeakedResizeObserver`](https://biomejs.dev/linter/rules/no-react-leaked-resize-observer) rule. This rule enforces that every `ResizeObserver` in a React component or custom hook has a corresponding `disconnect` in the cleanup function of `useEffect`. Forgetting to disconnect an observer can lead to memory leaks and unexpected behavior after component unmounts or dependency changes. | |
| Added the new nursery rule [`noReactLeakedResizeObserver`](https://biomejs.dev/linter/rules/no-react-leaked-resize-observer). This rule enforces that every `ResizeObserver` in a React component or custom hook is properly cleaned up in `useEffect` (for example via `disconnect`, or equivalent `unobserve` cleanup). Missing cleanup can lead to memory leaks and unexpected behaviour after unmounts or dependency changes. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.changeset/no-react-leaked-resize-observer.md at line 5, Update the
changeset wording to match the implemented rule noReactLeakedResizeObserver by
stating that cleanup must call either disconnect or unobserve on the
ResizeObserver (not only disconnect), and rephrase the message to be user-facing
and concise (e.g., "Ensure ResizeObservers in React components/hooks are cleaned
up with disconnect or unobserve to avoid memory leaks"). Also ensure the
changeset follows the project's guideline of focusing only on user-facing
changes and remove implementation detail language.
| "@biomejs/biome": patch | ||
| --- | ||
|
|
||
| Added the new nursery rule [`noReactLeakedTimeout`](https://biomejs.dev/linter/rules/no-react-leaked-timeout) rule. This rule enforces that every `setTimeout` in a React component or custom hook has a corresponding `clearTimeout` in the cleanup function of `useEffect`. Forgetting to clear a timeout can lead to memory leaks and unexpected callback executions after component unmounts or dependency changes. |
Contributor
There was a problem hiding this comment.
Tighten the changelog wording to useEffect.
The rule only analyses timers created inside effect callbacks, so “every setTimeout in a React component or custom hook” overstates the behaviour a bit. Wording it around useEffect will spare users a surprise.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.changeset/no-react-leaked-timeout.md at line 5, The changelog sentence
overstates the rule scope; update the line referencing the new nursery rule
noReactLeakedTimeout so it explicitly says the rule enforces that every
setTimeout inside useEffect callbacks (in components or custom hooks) must have
a corresponding clearTimeout in the effect cleanup, rather than saying “every
setTimeout in a React component or custom hook.” Edit the
.changeset/no-react-leaked-timeout.md diff text to replace the broader phrase
with “inside useEffect callbacks” and keep the link and rationale intact.
Comment on lines
+79
to
+125
| let callback = effect_callback(query_call)?; | ||
| let local_bindings = collect_bindings(&callback); | ||
| let local_add_entries: Vec<_> = callback | ||
| .syntax() | ||
| .descendants() | ||
| .filter_map(JsCallExpression::cast) | ||
| .filter_map(|call| extract_event_listener_entry(&call, &local_bindings)) | ||
| .filter(|entry| entry.kind == EventListenerMethod::Add && !entry.has_signal) | ||
| .collect(); | ||
|
|
||
| if local_add_entries.is_empty() { | ||
| return None; | ||
| } | ||
|
|
||
| let root = query_call.syntax().ancestors().last()?; | ||
|
|
||
| let mut remove_entries = Vec::new(); | ||
| let query_owner_scope = enclosing_owner_scope_range(query_call); | ||
|
|
||
| for call in root.descendants().filter_map(JsCallExpression::cast) { | ||
| if !is_effect_call(&call) { | ||
| continue; | ||
| } | ||
|
|
||
| if enclosing_owner_scope_range(&call) != query_owner_scope { | ||
| continue; | ||
| } | ||
|
|
||
| let Some(callback) = effect_callback(&call) else { | ||
| continue; | ||
| }; | ||
|
|
||
| let bindings = collect_bindings(&callback); | ||
| for nested_call in callback | ||
| .syntax() | ||
| .descendants() | ||
| .filter_map(JsCallExpression::cast) | ||
| { | ||
| let Some(entry) = extract_event_listener_entry(&nested_call, &bindings) else { | ||
| continue; | ||
| }; | ||
|
|
||
| if entry.kind == EventListenerMethod::Remove { | ||
| remove_entries.push(entry); | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Limit the match to this effect’s executed body and returned cleanup.
The current descendant scans pull addEventListener calls from nested helper functions and removeEventListener calls from any useEffect in the same owner scope. That means a never-called helper can be flagged, while a sibling effect or nested callback can accidentally satisfy the cleanup. Please only collect adds from the current effect body and removes from its returned cleanup function.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/biome_js_analyze/src/lint/nursery/no_react_leaked_event_listener.rs`
around lines 79 - 125, The scan is too broad: local_add_entries currently
collects addEventListener calls from all descendants of the effect callback and
remove_entries collects removes from any effect in the same owner scope, causing
false positives/negatives. Limit the add collection to the current effect’s
executed body (not nested helper declarations) by scanning only the callback’s
executed body/statements and returned cleanup by parsing the callback’s return
expression (the cleanup function) and scanning its descendants for
EventListenerMethod::Remove entries; replace the cross-effect loop that builds
remove_entries (root.descendants()/is_effect_call/enclosing_owner_scope_range)
with a parse of the current effect’s returned cleanup function and use
extract_event_listener_entry on that AST subtree, keeping identifiers
effect_callback, extract_event_listener_entry, EventListenerMethod::Add/Remove,
local_add_entries, and remove_entries to locate the changes.
Comment on lines
+100
to
+149
| for observer_range in observer_ranges { | ||
| let mut has_disconnect = false; | ||
| let mut has_dynamic_observe = false; | ||
| let mut observe_count = 0usize; | ||
| let mut unobserve_count = 0usize; | ||
|
|
||
| for call in callback_node | ||
| .descendants() | ||
| .filter_map(JsCallExpression::cast) | ||
| { | ||
| let Some(method_name) = observer_call_kind(&call) else { | ||
| continue; | ||
| }; | ||
|
|
||
| match method_name { | ||
| "disconnect" => { | ||
| has_disconnect = true; | ||
| } | ||
| "observe" => { | ||
| observe_count += 1; | ||
|
|
||
| if is_dynamic_observe_call(&call, callback_node) { | ||
| has_dynamic_observe = true; | ||
| } | ||
| } | ||
| "unobserve" => { | ||
| unobserve_count += 1; | ||
| } | ||
| _ => {} | ||
| } | ||
| } | ||
|
|
||
| if has_disconnect { | ||
| continue; | ||
| } | ||
|
|
||
| if has_dynamic_observe { | ||
| return Some(NoReactLeakedIntersectionObserverState { | ||
| range: observer_range, | ||
| kind: IntersectionObserverLeakKind::ExpectedDisconnectInControlFlow, | ||
| }); | ||
| } | ||
|
|
||
| if observe_count == 0 || observe_count > unobserve_count { | ||
| return Some(NoReactLeakedIntersectionObserverState { | ||
| range: observer_range, | ||
| kind: IntersectionObserverLeakKind::ExpectedDisconnectOrUnobserveInCleanup, | ||
| }); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Scope the analysis to the returned cleanup and the matching observer.
This loop replays every observe / unobserve / disconnect under the whole effect for each observer_range, so one disconnect() on observerA, a wrong-target unobserve(), or even a setup-time call can make observerB look clean. That will miss real leaks in multi-observer effects and mismatched cleanups.
Please add regression fixtures for multiple observers and wrong-target cleanup when you tighten this. As per coding guidelines, lint rules require snapshot tests in tests/specs/{group}/{rule}/.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@crates/biome_js_analyze/src/lint/nursery/no_react_leaked_intersection_observer.rs`
around lines 100 - 149, The analysis is currently global per effect: the loop
over observer_ranges replays all observe/unobserve/disconnect calls for every
observer, so calls for observerA can incorrectly clear observerB; scope the
analysis to only calls that target the specific observer represented by
observer_range by filtering callback_node.descendants() (and observer_call_kind
results) to the matching observer identifier/target for that observer_range
before counting; update the logic in the observer_ranges loop (references:
observer_ranges, callback_node.descendants(), observer_call_kind,
is_dynamic_observe_call, observe_count/unobserve_count, has_disconnect, and
NoReactLeakedIntersectionObserverState) to compute counts per-observer and to
correlate returned cleanup with the matching observer; finally add regression
snapshot tests for multi-observer effects and wrong-target cleanup under
tests/specs/{group}/{rule}/ as required.
Comment on lines
+158
to
+199
| fn has_clear_interval_in_cleanup( | ||
| callback_node: &biome_js_syntax::JsSyntaxNode, | ||
| var_name: Option<&str>, | ||
| ) -> bool { | ||
| for call in callback_node | ||
| .descendants() | ||
| .filter_map(JsCallExpression::cast) | ||
| { | ||
| let Ok(callee) = call.callee() else { | ||
| continue; | ||
| }; | ||
|
|
||
| let Some(name) = callee.get_callee_member_name() else { | ||
| continue; | ||
| }; | ||
|
|
||
| if name.text_trimmed() != "clearInterval" { | ||
| continue; | ||
| } | ||
|
|
||
| // If no var_name specified, any clearInterval is good enough (for bare setInterval) | ||
| if var_name.is_none() { | ||
| return true; | ||
| } | ||
|
|
||
| // Check if the clearInterval argument matches the variable name | ||
| if let Ok(args) = call.arguments() { | ||
| let [arg] = args.get_arguments_by_index([0]); | ||
| if let Some(arg_option) = arg | ||
| && let Some(expr) = arg_option.as_any_js_expression() | ||
| { | ||
| let inner = unwrap_typescript_expression(expr.clone()); | ||
| let arg_text = inner.to_trimmed_string(); | ||
| if arg_text == var_name.unwrap_or("") { | ||
| return true; | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| false | ||
| } |
Contributor
There was a problem hiding this comment.
Only count clearInterval calls from the returned cleanup.
This helper scans the whole effect body, so a clearInterval(intervalId) in setup code or another nested callback suppresses the diagnostic even if the effect never returns a cleanup. That undercuts the rule’s advertised contract.
Please add a regression fixture for “cleared outside cleanup” when you patch this. As per coding guidelines, lint rules require snapshot tests in tests/specs/{group}/{rule}/.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/biome_js_analyze/src/lint/nursery/no_react_leaked_interval.rs` around
lines 158 - 199, The helper has_clear_interval_in_cleanup currently scans the
whole effect callback; restrict it to only inspect clearInterval calls inside
the returned cleanup function: locate JsReturnStatement nodes inside
callback_node, get each return's argument expression, and if that argument is a
JsFunctionExpression or JsArrowFunctionExpression (or a parenthesized wrapper),
descend into that function's body and run the existing clearInterval detection
logic there (handle arrow-expression bodies and block bodies). Keep the same
var_name matching behavior, and add a snapshot regression test "cleared outside
cleanup" under tests/specs/{group}/{rule}/ for this rule.
Comment on lines
+78
to
+149
| let mut observer_ranges = Vec::new(); | ||
| for new_expr in callback_node | ||
| .descendants() | ||
| .filter_map(JsNewExpression::cast) | ||
| { | ||
| if !is_new_resize_observer(&new_expr) { | ||
| continue; | ||
| } | ||
|
|
||
| if !new_expr | ||
| .syntax() | ||
| .ancestors() | ||
| .any(|ancestor| JsVariableDeclarator::can_cast(ancestor.kind())) | ||
| { | ||
| return Some(NoReactLeakedResizeObserverState { | ||
| range: new_expr.range(), | ||
| kind: ResizeObserverLeakKind::UnexpectedFloatingInstance, | ||
| }); | ||
| } | ||
|
|
||
| observer_ranges.push(new_expr.range()); | ||
| } | ||
|
|
||
| for observer_range in observer_ranges { | ||
| let mut has_disconnect = false; | ||
| let mut has_dynamic_observe = false; | ||
| let mut observe_count = 0usize; | ||
| let mut unobserve_count = 0usize; | ||
|
|
||
| for call in callback_node | ||
| .descendants() | ||
| .filter_map(JsCallExpression::cast) | ||
| { | ||
| let Some(method_name) = observer_call_kind(&call) else { | ||
| continue; | ||
| }; | ||
|
|
||
| match method_name { | ||
| "disconnect" => { | ||
| has_disconnect = true; | ||
| } | ||
| "observe" => { | ||
| observe_count += 1; | ||
|
|
||
| if is_dynamic_observe_call(&call, callback_node) { | ||
| has_dynamic_observe = true; | ||
| } | ||
| } | ||
| "unobserve" => { | ||
| unobserve_count += 1; | ||
| } | ||
| _ => {} | ||
| } | ||
| } | ||
|
|
||
| if has_disconnect { | ||
| continue; | ||
| } | ||
|
|
||
| if has_dynamic_observe { | ||
| return Some(NoReactLeakedResizeObserverState { | ||
| range: observer_range, | ||
| kind: ResizeObserverLeakKind::ExpectedDisconnectInControlFlow, | ||
| }); | ||
| } | ||
|
|
||
| if observe_count == 0 || observe_count > unobserve_count { | ||
| return Some(NoReactLeakedResizeObserverState { | ||
| range: observer_range, | ||
| kind: ResizeObserverLeakKind::ExpectedDisconnectOrUnobserveInCleanup, | ||
| }); | ||
| } |
Contributor
There was a problem hiding this comment.
Track cleanup per observer, not per effect.
This pass stores only the TextRange, then re-counts every observe, unobserve, and disconnect call in the whole callback for each observer. One observer’s disconnect() can therefore pardon a leaking sibling, and an unrelated object with the same method names will skew the result too. Please carry the bound identifier through the analysis and only match calls on that observer.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/biome_js_analyze/src/lint/nursery/no_react_leaked_resize_observer.rs`
around lines 78 - 149, The analysis currently saves only observer_ranges and
re-scans the entire callback for each observer which allows one observer's
disconnect or unrelated objects to affect others; instead capture the observer's
bound identifier/variable declarator when you find the new ResizeObserver (use
the JsVariableDeclarator ancestor of is_new_resize_observer/new expression) and
store that binding (e.g., the JsIdentifier or the declarator node) alongside its
range; then when scanning calls use observer_call_kind/JsCallExpression but
additionally verify the call's receiver resolves to that specific binding
(compare the member expression/object node or resolved binding node) and only
count observe/unobserve/disconnect for that bound observer; return
NoReactLeakedResizeObserverState with the stored observer binding/range when
that specific observer fails the checks.
Comment on lines
+208
to
+219
| fn observer_call_kind(call: &JsCallExpression) -> Option<&'static str> { | ||
| let callee = call.callee().ok()?.omit_parentheses(); | ||
| let member = callee.as_js_static_member_expression()?; | ||
|
|
||
| let member_name = member.member().ok()?.value_token().ok()?; | ||
| match member_name.text_trimmed() { | ||
| "observe" => Some("observe"), | ||
| "unobserve" => Some("unobserve"), | ||
| "disconnect" => Some("disconnect"), | ||
| _ => None, | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Unwrap casted observer methods before matching them.
observer_call_kind() only sees plain observer.observe(...) / observer.disconnect(). Casted forms like (observer.observe as any)(node) are invisible here, so a valid TS-wrapped cleanup would still be reported. A tiny unwrap would save a lot of grief.
Possible fix
-use crate::react::{effect_callback, is_effect_call};
+use crate::{
+ react::{effect_callback, is_effect_call},
+ typescript::unwrap_typescript_expression,
+};
...
fn observer_call_kind(call: &JsCallExpression) -> Option<&'static str> {
- let callee = call.callee().ok()?.omit_parentheses();
+ let callee = unwrap_typescript_expression(call.callee().ok()?.omit_parentheses());
let member = callee.as_js_static_member_expression()?;📝 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.
Suggested change
| fn observer_call_kind(call: &JsCallExpression) -> Option<&'static str> { | |
| let callee = call.callee().ok()?.omit_parentheses(); | |
| let member = callee.as_js_static_member_expression()?; | |
| let member_name = member.member().ok()?.value_token().ok()?; | |
| match member_name.text_trimmed() { | |
| "observe" => Some("observe"), | |
| "unobserve" => Some("unobserve"), | |
| "disconnect" => Some("disconnect"), | |
| _ => None, | |
| } | |
| } | |
| use crate::{ | |
| react::{effect_callback, is_effect_call}, | |
| typescript::unwrap_typescript_expression, | |
| }; | |
| fn observer_call_kind(call: &JsCallExpression) -> Option<&'static str> { | |
| let callee = unwrap_typescript_expression(call.callee().ok()?.omit_parentheses()); | |
| let member = callee.as_js_static_member_expression()?; | |
| let member_name = member.member().ok()?.value_token().ok()?; | |
| match member_name.text_trimmed() { | |
| "observe" => Some("observe"), | |
| "unobserve" => Some("unobserve"), | |
| "disconnect" => Some("disconnect"), | |
| _ => None, | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/biome_js_analyze/src/lint/nursery/no_react_leaked_resize_observer.rs`
around lines 208 - 219, observer_call_kind currently only strips parentheses
then tries as_js_static_member_expression, so casted forms like
(observer.observe as any)(...) are missed; update observer_call_kind to
repeatedly unwrap TS/JS "as" casts and parenthesized expressions from the callee
(after the initial omit_parentheses()) until you reach a
JsStaticMemberExpression, then extract
member.member().value_token().text_trimmed() as before (i.e. keep the match on
"observe"/"unobserve"/"disconnect"); refer to the observer_call_kind function,
the callee variable, and the member_name extraction when making this change.
Comment on lines
+158
to
+199
| fn has_clear_timeout_in_cleanup( | ||
| callback_node: &biome_js_syntax::JsSyntaxNode, | ||
| var_name: Option<&str>, | ||
| ) -> bool { | ||
| for call in callback_node | ||
| .descendants() | ||
| .filter_map(JsCallExpression::cast) | ||
| { | ||
| let Ok(callee) = call.callee() else { | ||
| continue; | ||
| }; | ||
|
|
||
| let Some(name) = callee.get_callee_member_name() else { | ||
| continue; | ||
| }; | ||
|
|
||
| if name.text_trimmed() != "clearTimeout" { | ||
| continue; | ||
| } | ||
|
|
||
| // If no var_name specified, any clearTimeout is good enough (for bare setTimeout) | ||
| if var_name.is_none() { | ||
| return true; | ||
| } | ||
|
|
||
| // Check if the clearTimeout argument matches the variable name | ||
| if let Ok(args) = call.arguments() { | ||
| let [arg] = args.get_arguments_by_index([0]); | ||
| if let Some(arg_option) = arg | ||
| && let Some(expr) = arg_option.as_any_js_expression() | ||
| { | ||
| // Strip TS type assertions (e.g. `timeoutId as number`) to get the plain name | ||
| let inner = unwrap_typescript_expression(expr.clone()); | ||
| let arg_text = inner.to_trimmed_string(); | ||
| if arg_text == var_name.unwrap_or("") { | ||
| return true; | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| false |
Contributor
There was a problem hiding this comment.
Only treat clearTimeout inside the returned cleanup as valid.
has_clear_timeout_in_cleanup walks every descendant of the effect callback, not the function returned from it. A clearTimeout(timeoutId) in setup code or another nested callback will therefore silence the diagnostic even when the effect returns no cleanup at all.
Please add a fixture where the timer is cleared outside cleanup so this stays nailed down. As per coding guidelines, lint rules require snapshot tests in tests/specs/{group}/{rule}/.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/biome_js_analyze/src/lint/nursery/no_react_leaked_timeout.rs` around
lines 158 - 199, The function has_clear_timeout_in_cleanup currently scans all
descendants of the effect callback (callback_node), which can wrongly pick up
clearTimeout calls outside the returned cleanup; change it to locate the actual
returned cleanup function/body and only search that node for JsCallExpression
with callee name "clearTimeout" (e.g., inspect return statements or
arrow-function concise bodies returned by the callback, unwrap the returned
function expression before calling descendants()), keep the existing arg-name
matching/unwrapping logic (unwrap_typescript_expression) when var_name is
provided, and add a snapshot spec fixture under
tests/specs/nursery/no_react_leaked_timeout/ demonstrating a case where
clearTimeout is called outside the cleanup (so the rule still triggers) to
prevent regressions.
Comment on lines
+359
to
+373
| pub(crate) fn is_effect_call(call: &JsCallExpression) -> bool { | ||
| let Ok(callee) = call.callee() else { | ||
| return false; | ||
| }; | ||
|
|
||
| let Some(name) = callee.get_callee_member_name() else { | ||
| return false; | ||
| }; | ||
|
|
||
| let name = name.text_trimmed().to_string(); | ||
| matches!( | ||
| name.as_str(), | ||
| "useEffect" | "useLayoutEffect" | "useInsertionEffect" | ||
| ) | ||
| } |
Contributor
There was a problem hiding this comment.
is_effect_call currently over-matches non-React APIs.
Matching only the callee member name means calls like foo.useEffect(...) are treated as React effect hooks, which can produce false positives in the leaked-resource rules. Please gate this with React-aware resolution (or equivalent binding checks) rather than name-only matching.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/biome_js_analyze/src/react.rs` around lines 359 - 373, is_effect_call
currently treats any member named useEffect/useLayoutEffect/useInsertionEffect
as a React hook; change it to only match genuine React hooks by rejecting member
expressions and resolving the callee binding: first ensure the call's callee is
an Identifier (instead of using callee.get_callee_member_name()), then resolve
that identifier's binding/definition via your semantic resolver/scope (the same
resolver used elsewhere in the crate) and only return true if the identifier
name is one of "useEffect" | "useLayoutEffect" | "useInsertionEffect" and its
resolved binding comes from the React import (e.g., a named import from "react"
or a local binding that originates from that import); leave calls like
foo.useEffect(...) and other non-React bindings unmatched.
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
5 new rules for the React domain to prevent memory leaks (4 ported over from react xyz, 1 biome exclusive based on those)
AI disclosure: Generated with the help of co-pilot
Test Plan
Added unit tests
Docs