fix(lint): useHookAtTopLevel should not report issues unrelated to the nested hook call
#7406
Conversation
…the nested hook call
🦋 Changeset detectedLatest commit: 4e81bd2 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 |
WalkthroughIntroduces a boolean field Assessment against linked issues
Suggested labels
Suggested reviewers
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
github-actions
bot
added
A-Linter
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
.changeset/poor-horses-smash.md (1)
5-6: Tighten the changeset wording and add a minimal example.This helps users understand past behaviour vs. current behaviour at a glance.
Apply:
- Fixed #6393 issue which reported excessive diagnostics in [useHookAtTopLevel](https://biomejs.dev/linter/rules/use-hook-at-top-level/) lint rule. + Fixed an issue (#6393) where the [useHookAtTopLevel](https://biomejs.dev/linter/rules/use-hook-at-top-level/) rule reported excessive diagnostics for nested hook calls. + + The rule now reports only the offending top-level call site, not sub-hooks of composite hooks. + + ```js + // Before: reported twice (useFoo and useBar). + function useFoo() { return useBar(); } + function Component() { + if (cond) useFoo(); + } + // After: reported once at the call to useFoo(). + ```crates/biome_js_analyze/src/lint/correctness/use_hook_at_top_level.rs (3)
495-506: Reduce shadowing for readability.Avoid reusing the long is_enclosed_in_component_or_hook identifier locally; introduce a short, distinct name to make the control flow easier to parse.
Apply:
- let is_enclosed_in_component_or_hook = is_enclosed_in_component_or_hook - || enclosing_function.is_react_component_or_hook(); + let enclosed = is_enclosed_in_component_or_hook + || enclosing_function.is_react_component_or_hook(); if let AnyJsFunctionOrMethod::AnyJsFunction(function) = enclosing_function && let Some(calls_iter) = function.all_calls(model) { for call in calls_iter { calls.push(CallPath { call: call.tree(), path: path.clone(), - is_enclosed_in_component_or_hook, + is_enclosed_in_component_or_hook: enclosed, }); } }
510-517: Comment vs. behaviour mismatch.The comment says “mark them”, but we only skip reporting (continue) and don’t mutate the flag here. Tweak the comment to match the actual behaviour.
Apply:
- // We should not report problems which are already reported for different - // components or hooks. At the same time, those calls should still be tracked - // to detect recursive calls, so we keep those call paths but mark - // them with "is_enclosed_in_component_or_hook". + // Avoid duplicate diagnostics if this path already passed through + // a component/hook. We still keep previously enqueued paths to + // allow recursion detection elsewhere.
457-463: Sanity check: closure captures root call intentionally.get_hook_name_range closes over the root call; emitted diagnostics will always anchor to the root hook under analysis, not intermediate calls. If that’s intentional (it appears to be), we’re good; otherwise, capture the current call from the loop.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (3)
crates/biome_js_analyze/tests/specs/correctness/useHookAtTopLevel/invalid.js.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/correctness/useHookAtTopLevel/invalid.ts.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/correctness/useHookAtTopLevel/invalidCompositeHook.js.snapis excluded by!**/*.snapand included by**
📒 Files selected for processing (5)
.changeset/poor-horses-smash.md(1 hunks)crates/biome_js_analyze/src/lint/correctness/use_hook_at_top_level.rs(3 hunks)crates/biome_js_analyze/tests/specs/correctness/useHookAtTopLevel/invalid.js(2 hunks)crates/biome_js_analyze/tests/specs/correctness/useHookAtTopLevel/invalid.ts(1 hunks)crates/biome_js_analyze/tests/specs/correctness/useHookAtTopLevel/invalidCompositeHook.js(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
.changeset/*.md
📄 CodeRabbit inference engine (CONTRIBUTING.md)
.changeset/*.md: Create changesets withjust new-changeset; store them in.changeset/with correct frontmatter (package keys and change type).
In changeset descriptions, follow content conventions: user-facing changes only; past tense for what you did; present tense for current behavior; link issues for fixes; link rules/assists; include representative code blocks; end every sentence with a period.
When adding headers in a changeset, only use #### or ##### levels.
Files:
.changeset/poor-horses-smash.md
crates/biome_*_{syntax,parser,formatter,analyze,factory,semantic}/**
📄 CodeRabbit inference engine (CLAUDE.md)
Maintain the per-language crate structure: biome_{lang}_{syntax,parser,formatter,analyze,factory,semantic}
Files:
crates/biome_js_analyze/tests/specs/correctness/useHookAtTopLevel/invalidCompositeHook.jscrates/biome_js_analyze/src/lint/correctness/use_hook_at_top_level.rscrates/biome_js_analyze/tests/specs/correctness/useHookAtTopLevel/invalid.tscrates/biome_js_analyze/tests/specs/correctness/useHookAtTopLevel/invalid.js
crates/biome_*/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place core crates under /crates/biome_*/
Files:
crates/biome_js_analyze/tests/specs/correctness/useHookAtTopLevel/invalidCompositeHook.jscrates/biome_js_analyze/src/lint/correctness/use_hook_at_top_level.rscrates/biome_js_analyze/tests/specs/correctness/useHookAtTopLevel/invalid.tscrates/biome_js_analyze/tests/specs/correctness/useHookAtTopLevel/invalid.js
**/tests/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place test files under a tests/ directory in each crate
Files:
crates/biome_js_analyze/tests/specs/correctness/useHookAtTopLevel/invalidCompositeHook.jscrates/biome_js_analyze/tests/specs/correctness/useHookAtTopLevel/invalid.tscrates/biome_js_analyze/tests/specs/correctness/useHookAtTopLevel/invalid.js
**/*.{rs,toml}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Format Rust and TOML files before committing (use
just f/just format).
Files:
crates/biome_js_analyze/src/lint/correctness/use_hook_at_top_level.rs
🧬 Code graph analysis (1)
crates/biome_js_analyze/tests/specs/correctness/useHookAtTopLevel/invalid.js (1)
crates/biome_js_analyze/tests/specs/correctness/useHookAtTopLevel/valid.js (2)
Component7(93-96)a(99-101)
⏰ 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). (24)
- GitHub Check: Documentation
- GitHub Check: Bench (biome_html_formatter)
- 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: Lint project (depot-ubuntu-24.04-arm-16)
- GitHub Check: Bench (biome_css_parser)
- GitHub Check: Bench (biome_json_parser)
- GitHub Check: Bench (biome_package)
- GitHub Check: Bench (biome_configuration)
- GitHub Check: Bench (biome_json_formatter)
- GitHub Check: Bench (biome_json_analyze)
- GitHub Check: Bench (biome_module_graph)
- GitHub Check: Bench (biome_html_parser)
- GitHub Check: Bench (biome_js_analyze)
- GitHub Check: Bench (biome_css_analyze)
- GitHub Check: Bench (biome_js_formatter)
- GitHub Check: Bench (biome_graphql_parser)
- GitHub Check: Bench (biome_graphql_formatter)
- GitHub Check: Bench (biome_css_formatter)
- GitHub Check: Bench (biome_js_parser)
- GitHub Check: autofix
- GitHub Check: Test Node.js API
🔇 Additional comments (4)
crates/biome_js_analyze/tests/specs/correctness/useHookAtTopLevel/invalid.ts (1)
1-1: Rename clarifies intent and test case.Switching from Component1() to useHook() better models a composite hook invocation driving this rule. Looks good.
Also applies to: 7-7
crates/biome_js_analyze/tests/specs/correctness/useHookAtTopLevel/invalid.js (1)
71-78: Good rename: tests now exercise a real “hook-like” callee.Renaming Component6 → useHook6 avoids accidental component semantics and matches the rule’s intent.
crates/biome_js_analyze/tests/specs/correctness/useHookAtTopLevel/invalidCompositeHook.js (1)
1-11: Nice coverage for composite hooks.This fixture succinctly proves we emit a single diagnostic at the call site into the composite hook.
crates/biome_js_analyze/src/lint/correctness/use_hook_at_top_level.rs (1)
423-427: Tracking enclosure state is the right lever.Adding is_enclosed_in_component_or_hook to CallPath is a minimal, local fix that prevents duplicate diagnostics across component/hook boundaries without widening public APIs.
CodSpeed Performance ReportMerging #7406 will not alter performanceComparing Summary
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
.changeset/poor-horses-smash.md (2)
5-6: Tighten wording: “issue” → “bug”.Slightly more precise and formal phrasing.
Apply this diff:
-Fixed an issue (#6393) where the [useHookAtTopLevel](https://biomejs.dev/linter/rules/use-hook-at-top-level/) rule reported excessive diagnostics for nested hook calls. +Fixed a bug (#6393) in the [useHookAtTopLevel](https://biomejs.dev/linter/rules/use-hook-at-top-level/) rule that caused excessive diagnostics for nested hook calls.
7-8: Minor style tweak for clarity.“of composite hooks” reads fine, but “within composite hooks” is a touch clearer.
-The rule now reports only the offending top-level call site, not sub-hooks of composite hooks. +The rule now reports only the offending top-level call site, not sub-hooks within composite hooks.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
.changeset/poor-horses-smash.md(1 hunks)crates/biome_js_analyze/src/lint/correctness/use_hook_at_top_level.rs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/biome_js_analyze/src/lint/correctness/use_hook_at_top_level.rs
🧰 Additional context used
📓 Path-based instructions (1)
.changeset/*.md
📄 CodeRabbit inference engine (CONTRIBUTING.md)
.changeset/*.md: Create changesets withjust new-changeset; store them in.changeset/with correct frontmatter (package keys and change type).
In changeset descriptions, follow content conventions: user-facing changes only; past tense for what you did; present tense for current behavior; link issues for fixes; link rules/assists; include representative code blocks; end every sentence with a period.
When adding headers in a changeset, only use #### or ##### levels.
Files:
.changeset/poor-horses-smash.md
🪛 LanguageTool
.changeset/poor-horses-smash.md
[style] ~4-~4: Consider using a different verb for a more formal wording.
Context: --- "@biomejs/biome": patch --- Fixed an issue (#6393) where the [useHookAtTo...
(FIX_RESOLVE)
[uncategorized] ~7-~7: “of” seems less likely than “or” (‘either … or’).
Context: ...ding top-level call site, not sub-hooks of composite hooks. ```js // Before: repo...
(AI_HYDRA_LEO_CP_OF_OR)
⏰ 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). (24)
- 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-ubuntu-24.04-arm-16)
- GitHub Check: Lint project (depot-windows-2022)
- GitHub Check: Documentation
- GitHub Check: Bench (biome_configuration)
- GitHub Check: Bench (biome_graphql_parser)
- GitHub Check: Bench (biome_html_parser)
- GitHub Check: Bench (biome_package)
- GitHub Check: Bench (biome_html_formatter)
- GitHub Check: Test Node.js API
- GitHub Check: Bench (biome_css_formatter)
- GitHub Check: Bench (biome_graphql_formatter)
- GitHub Check: Bench (biome_module_graph)
- GitHub Check: Bench (biome_js_analyze)
- GitHub Check: Bench (biome_json_formatter)
- GitHub Check: Bench (biome_json_analyze)
- GitHub Check: Bench (biome_css_analyze)
- GitHub Check: Bench (biome_js_parser)
- GitHub Check: Bench (biome_json_parser)
- GitHub Check: Bench (biome_js_formatter)
- GitHub Check: Bench (biome_css_parser)
- GitHub Check: autofix
🔇 Additional comments (2)
.changeset/poor-horses-smash.md (2)
1-4: Frontmatter looks correct.Package key and patch bump are properly declared.
9-16: Example is clear and minimal.The before/after illustrates the change well; no action needed.
…the nested hook call (biomejs#7406) Co-authored-by: dyc3 <[email protected]> Co-authored-by: ematipico <[email protected]>
Summary
useHookAtTopLevelwas tracing each call through all nested hook / component calls. This is incorrect, since it should only report the place where a hook was incorrectly called, not all sub-hooks of that composite hook.Test Plan
Test is included. Incorrect tests were fixed.
Docs
Changeset is included.
Closes #6393.