Open
Conversation
🦋 Changeset detectedLatest commit: 7378af2 The changes in this PR will be included in the next version bump. This PR includes changesets to release 14 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-CLI
Merging this PR will not alter performance
Comparing 
Footnotes
|
ematipico
force-pushed
the
feat/no-unsed-styles
branch
from
cae8a12 to
948166c
Compare
dyc3
reviewed
Feb 20, 2026
ematipico
force-pushed
the
feat/no-unsed-styles
branch
from
948166c to
bfbdd39
Compare
dyc3
reviewed
Feb 20, 2026
crates/biome_html_analyze/tests/specs/nursery/noUndeclaredStyles/valid.vue
Outdated
Show resolved
Hide resolved
crates/biome_html_analyze/tests/specs/nursery/noUndeclaredStyles/valid.vue
Show resolved
Hide resolved
This reverts commit 64ff34b.
ematipico
force-pushed
the
feat/no-unsed-styles
branch
from
23d1725 to
3f8fd5b
Compare
github-actions
bot
added
A-Parser
github-actions
bot
added
A-Diagnostic
ematipico
force-pushed
the
feat/no-unsed-styles
branch
from
23d1725 to
0475565
Compare
ematipico
force-pushed
the
feat/no-unsed-styles
branch
from
d9ab12b to
f4747a7
Compare
ematipico
force-pushed
the
feat/no-unsed-styles
branch
from
4608234 to
aa9a11a
Compare
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds two new lint rules: Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![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: 13
🧹 Nitpick comments (11)
.claude/skills/lint-rule-development/SKILL.md (3)
18-25: Self-contradictory: the emoji ban uses emojis.The "No Emojis" policy lists five prohibitions, each prefixed with ❌ emojis. This undermines the directive and creates ambiguity about whether the ban applies to documentation or only implementation code.
Suggested fix: use plain text instead
**CRITICAL: No Emojis** Emojis are BANNED in all lint rule code: -- ❌ NO emojis in rustdoc comments -- ❌ NO emojis in diagnostic messages -- ❌ NO emojis in code action descriptions -- ❌ NO emojis in test files or test comments -- ❌ NO emojis anywhere in the rule implementation +- NO emojis in rustdoc comments +- NO emojis in diagnostic messages +- NO emojis in code action descriptions +- NO emojis in test files or test comments +- NO emojis anywhere in the rule implementation🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/skills/lint-rule-development/SKILL.md around lines 18 - 25, The section titled "**CRITICAL: No Emojis**" is self-contradictory because it uses ❌ emojis while banning them; update that block by removing all emoji characters (e.g. the ❌ prefixes) and replace them with plain-text indicators such as "NO" or a simple hyphenated bullet list so the prohibition applies clearly to both documentation and implementation; edit the header "**CRITICAL: No Emojis**" and each listed line (the five prohibition lines) to use plain text only and ensure the rule text explicitly states it applies to docs and code to remove ambiguity.
351-353: Inconsistent emoji usage after the declared ban.The supported languages list uses ✅ and ❌ emojis, contradicting the "CRITICAL: No Emojis" policy declared earlier (lines 18-25). Either the ban should apply only to implementation code (not documentation), or the documentation should consistently avoid emojis.
Suggested fix: use text indicators
**Supported languages:** -- ✅ JavaScript/TypeScript/JSX/TSX -- ✅ CSS (module graph automatically populated from `file=` blocks) -- ❌ HTML with `<style>` blocks (requires embedded snippet parsing - not yet implemented) +- Supported: JavaScript/TypeScript/JSX/TSX +- Supported: CSS (module graph automatically populated from `file=` blocks) +- Not supported: HTML with `<style>` blocks (requires embedded snippet parsing - not yet implemented)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/skills/lint-rule-development/SKILL.md around lines 351 - 353, The supported languages list in SKILL.md currently uses Unicode emoji checkmarks and crosses (e.g., the lines starting with the emoji markers for JavaScript/TypeScript/JSX/TSX, CSS, and HTML `<style>` blocks) which violates the "CRITICAL: No Emojis" policy declared earlier (lines 18-25); remove the emoji characters and replace them with plain text indicators such as "Supported", "Not supported", or "Yes"/"No" (for example change the emoji-prefixed list items to "Supported: JavaScript/TypeScript/JSX/TSX", "Supported: CSS...", "Not supported: HTML with <style> blocks") so the document no longer contains emojis but clearly communicates support status.
243-273: Clarify whether magic comments are enforced or conventional.The section describes the purpose of magic comments (documentation, review aid, debugging reference) but doesn't specify whether test tooling validates their presence or correctness. If they're mandatory, document the enforcement mechanism; if they're merely best practice, state that explicitly to manage expectations.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/skills/lint-rule-development/SKILL.md around lines 243 - 273, Update the "IMPORTANT: Magic Comments for Test Expectations" section to explicitly state whether the magic comment lines are enforced by test tooling or are a recommended convention: edit the paragraph under the header "IMPORTANT: Magic Comments for Test Expectations" to either (A) declare them mandatory and describe the enforcement mechanism (e.g., test harness validates first-line magic comments and fails tests without them) or (B) mark them as a best-practice convention and note they are not programmatically validated; reference the section title and the examples (the JavaScript and HTML magic-comment snippets) so reviewers can find and update the existing content without changing examples unless describing validation behavior.crates/biome_test_utils/src/lib.rs (1)
268-295: Consider consolidating withmodule_graph_for_test_file.This new function shares significant logic with
module_graph_for_test_file(lines 246–266). The main difference appears to be the order of JS vs CSS population and thetrue/falseflag toupdate_graph_for_js_paths. A small refactor to extract shared setup could reduce duplication, though it's not blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_test_utils/src/lib.rs` around lines 268 - 295, module_graph_for_css_test_file duplicates setup logic from module_graph_for_test_file; extract the shared parts (creating ModuleGraph, dir/OsFileSystem, collecting css_paths/js_paths and computing css_roots/js_roots via get_css_like_paths_in_dir/get_css_added_paths/get_js_like_paths_in_dir/get_added_js_paths) into a small helper (e.g. build_module_graph_for_test_dir or module_graph_for_test_file_common) that returns the ModuleGraph and the prepared roots, and then call update_graph_for_css_paths and update_graph_for_js_paths from each caller in the desired order and with the appropriate flag (the boolean passed to update_graph_for_js_paths); update module_graph_for_css_test_file and module_graph_for_test_file to use that helper to remove duplication.crates/biome_js_analyze/tests/specs/nursery/noUndeclaredStyles/invalid.jsx (1)
1-7: Minor inconsistency:Buttonfunction is not exported.Looking at the related snippet from
Button.jsx, the function is exported. If the test harness expects to import and analyse the component, consider addingexport.That said, if this test only validates class usage within the file itself, the current form is fine.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_js_analyze/tests/specs/nursery/noUndeclaredStyles/invalid.jsx` around lines 1 - 7, The test's Button component is defined as function Button but not exported, causing inconsistency with the related exported Button.jsx; update the declaration for the function Button to be exported (either add a named export or make it the default export) so the test harness can import/analyze it—modify the symbol function Button to export it accordingly.crates/biome_module_graph/src/css_module_info/traverse.rs (1)
137-141: Recursiveself.next()call could overflow the stack on deep import chains.When CSS steps are found, the iterator recursively calls
self.next()to yield the first item. For very deep import hierarchies with many CSS files at each level, this could theoretically lead to stack overflow.Consider using a loop instead of recursion for the happy path.
♻️ Iterative alternative
if !css_steps.is_empty() { // Create a real iterator from the vector self.current_css_iter = Some(css_steps.into_iter()); - // Recursively call next() to yield the first CSS file - return self.next(); + // Yield the first CSS file from the new iterator + return self.current_css_iter.as_mut().and_then(|iter| iter.next()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_module_graph/src/css_module_info/traverse.rs` around lines 137 - 141, The recursive call to self.next() after setting self.current_css_iter (via Some(css_steps.into_iter())) can overflow on deep import chains; refactor next() to be iterative: after assigning self.current_css_iter, do not call self.next() recursively—instead loop (e.g., while/loop) to pull the first item from self.current_css_iter (using next() on that iterator) and return it or continue the outer loop if the iterator is empty; update the logic around css_steps and Some(css_steps.into_iter()) so the happy path uses the loop to yield items rather than recursion.crates/biome_js_analyze/tests/specs/nursery/noUndeclaredStyles/upward-traversal/Page.jsx (1)
4-4: Minor: Inconsistent semicolon usage.Line 3 ends with a semicolon but line 4 doesn't. Consider adding one for consistency.
-import { Button } from "./Button.jsx" +import { Button } from "./Button.jsx";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_js_analyze/tests/specs/nursery/noUndeclaredStyles/upward-traversal/Page.jsx` at line 4, The import statement "import { Button } from \"./Button.jsx\"" is missing a trailing semicolon; update that import (and any other imports in this file if present) to consistently end with a semicolon so the file uses consistent semicolon style.crates/biome_module_graph/tests/spec_tests.rs (1)
2617-2623: Avoid asserting the exact importer count.transitive_importers_ofmay include intermediate CSS nodes over time; checking for the expected entry points is sufficient and less brittle.🧩 Suggested tweak
- assert_eq!( - consumers.len(), - 2, - "Expected 2 consumers of components.css; got: {consumers:?}" - );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_module_graph/tests/spec_tests.rs` around lines 2617 - 2623, The test currently asserts an exact count on consumers from module_graph.transitive_importers_of(Utf8Path::new("/src/components.css")), which is brittle; change the assertion to verify that the expected entry-point importers are present instead of asserting consumers.len() == 2. Locate the call to transitive_importers_of and the consumers variable in this test and replace the exact-length assertion with checks that the returned collection contains the specific entry-point paths (e.g., the two expected entry modules) — e.g., use membership/subset assertions or iterate consumers to assert those entry paths are found.crates/biome_service/src/workspace/server.tests.rs (1)
921-1035: Prefer opening the importing JSX file explicitly. It makes module‑graph population explicit and avoids relying on scan behaviour. Consider applying this pattern in the JSX‑backednoUnusedStylestests.🔧 Example tweak (apply similarly to the other JSX‑referencing tests)
- workspace - .open_file(OpenFileParams { - project_key, - path: BiomePath::new("/project/styles.css"), - content: FileContent::FromServer, - document_file_source: None, - persist_node_cache: false, - inline_config: None, - }) - .unwrap(); + for path in ["/project/styles.css", "/project/App.jsx"] { + workspace + .open_file(OpenFileParams { + project_key, + path: BiomePath::new(path), + content: FileContent::FromServer, + document_file_source: None, + persist_node_cache: false, + inline_config: None, + }) + .unwrap(); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_service/src/workspace/server.tests.rs` around lines 921 - 1035, The tests (no_unused_styles_passes_when_class_is_referenced_in_jsx and no_unused_styles_reports_only_unreferenced_classes) open only /project/styles.css but not the importing JSX module; explicitly open the importing file (/project/App.jsx) via workspace.open_file (same signature used for styles.css) after scan_project and before pull_diagnostics so the module graph is populated and the noUnusedStyles analyzer can see the JSX reference.crates/biome_html_analyze/src/lib.rs (1)
14-33: Consider reordering: struct definition after imports.The
HtmlAnalyzerServicesstruct definition (lines 15-20) appears before theusestatements (lines 21-33). While this compiles fine, it's unconventional. Consider moving the struct definition after the imports for consistency with Rust conventions.Suggested reordering
pub use crate::services::module_graph::{HtmlModuleGraph, HtmlModuleGraphService}; use crate::suppression_action::HtmlSuppressionAction; - -/// Services available to HTML lint rules. -#[derive(Debug, Default)] -pub struct HtmlAnalyzerServices { - pub module_graph: Option<Arc<ModuleGraph>>, - pub project_layout: Option<Arc<ProjectLayout>>, -} use biome_analyze::{ AnalysisFilter, AnalyzerOptions, AnalyzerSignal, AnalyzerSuppression, ControlFlow, LanguageRoot, MatchQueryParams, MetadataRegistry, RuleAction, RuleRegistry, to_analyzer_suppressions, }; use biome_deserialize::TextRange; use biome_diagnostics::Error; use biome_html_syntax::{HtmlFileSource, HtmlLanguage}; use biome_module_graph::ModuleGraph; use biome_project_layout::ProjectLayout; use biome_suppression::{SuppressionDiagnostic, parse_suppression_comment}; use std::ops::Deref; use std::sync::{Arc, LazyLock}; + +/// Services available to HTML lint rules. +#[derive(Debug, Default)] +pub struct HtmlAnalyzerServices { + pub module_graph: Option<Arc<ModuleGraph>>, + pub project_layout: Option<Arc<ProjectLayout>>, +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_html_analyze/src/lib.rs` around lines 14 - 33, Move the HtmlAnalyzerServices struct definition so it appears after the module imports; specifically, relocate the pub struct HtmlAnalyzerServices { ... } declaration to below the use declarations (which include symbols like AnalysisFilter, AnalyzerOptions, HtmlFileSource, HtmlLanguage, ModuleGraph, ProjectLayout, etc.) so imports precede type definitions and the file follows common Rust ordering conventions.crates/biome_service/src/workspace/server.rs (1)
1031-1075: Remove the duplicate HtmlRoot branch to avoid dead code.
Two identical branches make the second unreachable and easy to miss during edits.🧹 Proposed cleanup
- } else if let Some(html_root) = - SendNode::into_language_root::<HtmlRoot>(root.clone()) - { - // Collect embedded CSS roots from the stored document's snippets. - let embedded_css_roots: Vec<AnyCssRoot> = self - .documents - .pin() - .get(path.as_path()) - .map(|doc| { - doc.embedded_snippets - .iter() - .filter_map(|s| s.as_css_embedded_snippet()) - .map(|s| s.parse.tree()) - .collect() - }) - .unwrap_or_default(); - self.module_graph.update_graph_for_html_paths( - self.fs.as_ref(), - &self.project_layout, - path, - html_root, - &embedded_css_roots, - ) - }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_service/src/workspace/server.rs` around lines 1031 - 1075, There is a duplicated branch using SendNode::into_language_root::<HtmlRoot>(root.clone()) that repeats the same logic (collecting embedded_css_roots via self.documents.pin().get(...).embedded_snippets and calling self.module_graph.update_graph_for_html_paths(...)), making the second branch dead code; remove the redundant duplicate branch so only a single HtmlRoot branch remains, leaving the embedded_css_roots collection and the call to module_graph.update_graph_for_html_paths intact and unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.claude/skills/README.md:
- Around line 16-30: The "No Emojis Policy" section in README.md uses ❌ emoji
bullets which contradict the rule; update the "Universal Coding Standards" /
"CRITICAL: No Emojis Policy" block by removing all emoji characters (e.g., ❌)
and replacing them with plain text markers or ASCII alternatives like "- NO
emojis in source code" so the policy text (the list items) contains no emoji
symbols and the summary/details (e.g., the "Proposed fix" summary) are also
emoji-free.
In `@crates/biome_cli/Cargo.toml`:
- Around line 83-86: The dev-dependencies currently use workspace = true; change
each dev-dependency (biome_css_formatter, biome_css_parser, biome_js_formatter)
to be a path dependency that points at the local crate directory instead of
relying on workspace=true (e.g. replace the workspace = true entry with path =
"<local-crate-dir>" for each crate), ensuring Cargo uses the local source for
dev builds.
In
`@crates/biome_html_analyze/tests/specs/nursery/noUndeclaredStyles/valid.astro`:
- Around line 72-73: The file comment "No style information at all: rule skips
to avoid false positives" is misleading because there is a <style> block (lines
7–31) defining classes and the element uses class "any-class" which is not
defined there; update the HTML comment to accurately state that a <style> block
exists and clarify the rule's behavior (e.g., that the rule skips undefined
classes when external stylesheets or global CSS are expected) so readers
understand why "any-class" being undefined is acceptable in this test.
In `@crates/biome_html_analyze/tests/specs/nursery/noUndeclaredStyles/valid.vue`:
- Around line 41-42: The test comment claiming "No style information at all" is
misleading because this .vue file contains <style> blocks; update the comment to
accurately describe the test (e.g., "Style blocks present; this tests that
.any-class is allowed/treated as valid because ...") or move the test markup
(<div class="any-class">Unstyled section</div>) into the correct valid/invalid
spec file; locate the marker comment and the element with class="any-class" and
either rewrite the comment to reflect the presence of style blocks or relocate
the test case so that the file contents and comment match (adjust surrounding
spec filename/category if you move it).
In
`@crates/biome_js_analyze/tests/specs/nursery/noUndeclaredStyles/upward-traversal/README.md`:
- Around line 31-34: The README wording is inverted: update the sentence
describing where level3.jsx should see classes so it reflects that level3.jsx
imports level2.jsx (not vice versa); change "level2.css (imported by level2.jsx
which imports level3.jsx)" to "level2.css (imported by level2.jsx, which is
imported by level3.jsx)" and similarly ensure the surrounding examples
referencing level3.jsx, level2.jsx and multilevel.jsx consistently show the
import direction.
In `@crates/biome_module_graph/src/css_module_info/traverse.rs`:
- Around line 63-153: The loop in ImportTreeTraversal::next stops at the first
unvisited parent that imports current_path (owing to the final break), skipping
other unvisited parents; replace that break so the inner for-loop continues
scanning all entries and pushes every unvisited parent that imports current_path
(marking visited and pushing each onto self.stack, and creating
self.current_css_iter when CSS steps are found) instead of exiting after the
first one, thereby ensuring multiple parents importing the same child are all
discovered.
In `@crates/biome_module_graph/src/module_graph.rs`:
- Around line 355-373: Remove the non-doctest example block from the
documentation for traverse_import_tree_for_classes: delete the ignored
pseudocode snippet that references external variables like module_graph and
js_path and the surrounding /// ```ignore ... /// ``` lines so the doc comment
only contains the method description and usage notes (keep references to
build_diagnostic_traversal_chain() if present).
- Around line 150-202: The traversal in is_class_used_in_component_tree only
follows js_info.static_import_paths, so classes used in dynamically imported
components are missed; update the loop to iterate both
js_info.static_import_paths and js_info.dynamic_import_paths (same chaining
pattern used in transitive_importers_of) and push each import_path.as_path()
into the queue if present, preserving the existing visited set and
referenced_classes check.
In `@crates/biome_module_graph/tests/snap/mod.rs`:
- Around line 14-21: The matches pattern in is_js_like_extension includes an
unreachable "d.ts" entry because Path::extension() returns "ts" (no dot) for
.d.ts files; remove the "d.ts" alternative from the matches list in the
is_js_like_extension function so that "ts" covers declaration files and
eliminate the dead code.
In `@crates/biome_rule_options/src/no_undeclared_styles.rs`:
- Around line 1-6: Add a rustdoc comment describing the purpose and fields of
the NoUndeclaredStylesOptions options type: document what the
NoUndeclaredStylesOptions struct controls, any default behavior, and note that
it currently has no fields (or list future configurable fields) so consumers
understand its intent; place the doc comment immediately above the pub struct
NoUndeclaredStylesOptions declaration and ensure it follows rustdoc style (///)
so it appears in generated docs and satisfies the guideline to use inline
documentation for rule options.
In `@crates/biome_rule_options/src/no_unused_styles.rs`:
- Around line 1-6: Add an inline rustdoc comment for the NoUnusedStylesOptions
struct describing what this options type controls and any default behavior or
fields (even if empty), e.g., a brief summary sentence above the pub struct
NoUnusedStylesOptions {}; ensure the doc follows crate conventions (/// style)
and mentions that it is used by the "no unused styles" rule and any relevant
feature flags or defaults.
In `@crates/biome_ruledoc_utils/src/lib.rs`:
- Around line 77-84: Replace the two calls to
DocumentFileSource::from_well_known(&path_buf, false) with a single call to
DocumentFileSource::from_path(&path_buf, false) and cache the result in a local
variable (e.g., let source = DocumentFileSource::from_path(&path_buf, false));
then use source.is_javascript_like() to push into js_paths and
source.is_css_like() to push into css_paths so you get extension-based detection
and avoid calling the ctor twice per iteration.
In `@crates/biome_service/src/file_handlers/json.rs`:
- Around line 520-525: In the lint function in json.rs, change the analyzer
options call to pass the LintParams working_directory instead of None: replace
the call to params.settings.analyzer_options::<JsonLanguage>(params.path, None,
¶ms.language, params.suppression_reason.as_deref()) so it passes
params.working_directory for the second argument; this mirrors how code_actions
and other handlers (CSS/GraphQL) call analyzer_options and ensures consistent
path resolution for analyzer_options, JsonLanguage, lint, and LintParams.
---
Nitpick comments:
In @.claude/skills/lint-rule-development/SKILL.md:
- Around line 18-25: The section titled "**CRITICAL: No Emojis**" is
self-contradictory because it uses ❌ emojis while banning them; update that
block by removing all emoji characters (e.g. the ❌ prefixes) and replace them
with plain-text indicators such as "NO" or a simple hyphenated bullet list so
the prohibition applies clearly to both documentation and implementation; edit
the header "**CRITICAL: No Emojis**" and each listed line (the five prohibition
lines) to use plain text only and ensure the rule text explicitly states it
applies to docs and code to remove ambiguity.
- Around line 351-353: The supported languages list in SKILL.md currently uses
Unicode emoji checkmarks and crosses (e.g., the lines starting with the emoji
markers for JavaScript/TypeScript/JSX/TSX, CSS, and HTML `<style>` blocks) which
violates the "CRITICAL: No Emojis" policy declared earlier (lines 18-25); remove
the emoji characters and replace them with plain text indicators such as
"Supported", "Not supported", or "Yes"/"No" (for example change the
emoji-prefixed list items to "Supported: JavaScript/TypeScript/JSX/TSX",
"Supported: CSS...", "Not supported: HTML with <style> blocks") so the document
no longer contains emojis but clearly communicates support status.
- Around line 243-273: Update the "IMPORTANT: Magic Comments for Test
Expectations" section to explicitly state whether the magic comment lines are
enforced by test tooling or are a recommended convention: edit the paragraph
under the header "IMPORTANT: Magic Comments for Test Expectations" to either (A)
declare them mandatory and describe the enforcement mechanism (e.g., test
harness validates first-line magic comments and fails tests without them) or (B)
mark them as a best-practice convention and note they are not programmatically
validated; reference the section title and the examples (the JavaScript and HTML
magic-comment snippets) so reviewers can find and update the existing content
without changing examples unless describing validation behavior.
In `@crates/biome_html_analyze/src/lib.rs`:
- Around line 14-33: Move the HtmlAnalyzerServices struct definition so it
appears after the module imports; specifically, relocate the pub struct
HtmlAnalyzerServices { ... } declaration to below the use declarations (which
include symbols like AnalysisFilter, AnalyzerOptions, HtmlFileSource,
HtmlLanguage, ModuleGraph, ProjectLayout, etc.) so imports precede type
definitions and the file follows common Rust ordering conventions.
In `@crates/biome_js_analyze/tests/specs/nursery/noUndeclaredStyles/invalid.jsx`:
- Around line 1-7: The test's Button component is defined as function Button but
not exported, causing inconsistency with the related exported Button.jsx; update
the declaration for the function Button to be exported (either add a named
export or make it the default export) so the test harness can import/analyze
it—modify the symbol function Button to export it accordingly.
In
`@crates/biome_js_analyze/tests/specs/nursery/noUndeclaredStyles/upward-traversal/Page.jsx`:
- Line 4: The import statement "import { Button } from \"./Button.jsx\"" is
missing a trailing semicolon; update that import (and any other imports in this
file if present) to consistently end with a semicolon so the file uses
consistent semicolon style.
In `@crates/biome_module_graph/src/css_module_info/traverse.rs`:
- Around line 137-141: The recursive call to self.next() after setting
self.current_css_iter (via Some(css_steps.into_iter())) can overflow on deep
import chains; refactor next() to be iterative: after assigning
self.current_css_iter, do not call self.next() recursively—instead loop (e.g.,
while/loop) to pull the first item from self.current_css_iter (using next() on
that iterator) and return it or continue the outer loop if the iterator is
empty; update the logic around css_steps and Some(css_steps.into_iter()) so the
happy path uses the loop to yield items rather than recursion.
In `@crates/biome_module_graph/tests/spec_tests.rs`:
- Around line 2617-2623: The test currently asserts an exact count on consumers
from module_graph.transitive_importers_of(Utf8Path::new("/src/components.css")),
which is brittle; change the assertion to verify that the expected entry-point
importers are present instead of asserting consumers.len() == 2. Locate the call
to transitive_importers_of and the consumers variable in this test and replace
the exact-length assertion with checks that the returned collection contains the
specific entry-point paths (e.g., the two expected entry modules) — e.g., use
membership/subset assertions or iterate consumers to assert those entry paths
are found.
In `@crates/biome_service/src/workspace/server.rs`:
- Around line 1031-1075: There is a duplicated branch using
SendNode::into_language_root::<HtmlRoot>(root.clone()) that repeats the same
logic (collecting embedded_css_roots via
self.documents.pin().get(...).embedded_snippets and calling
self.module_graph.update_graph_for_html_paths(...)), making the second branch
dead code; remove the redundant duplicate branch so only a single HtmlRoot
branch remains, leaving the embedded_css_roots collection and the call to
module_graph.update_graph_for_html_paths intact and unchanged.
In `@crates/biome_service/src/workspace/server.tests.rs`:
- Around line 921-1035: The tests
(no_unused_styles_passes_when_class_is_referenced_in_jsx and
no_unused_styles_reports_only_unreferenced_classes) open only
/project/styles.css but not the importing JSX module; explicitly open the
importing file (/project/App.jsx) via workspace.open_file (same signature used
for styles.css) after scan_project and before pull_diagnostics so the module
graph is populated and the noUnusedStyles analyzer can see the JSX reference.
In `@crates/biome_test_utils/src/lib.rs`:
- Around line 268-295: module_graph_for_css_test_file duplicates setup logic
from module_graph_for_test_file; extract the shared parts (creating ModuleGraph,
dir/OsFileSystem, collecting css_paths/js_paths and computing css_roots/js_roots
via
get_css_like_paths_in_dir/get_css_added_paths/get_js_like_paths_in_dir/get_added_js_paths)
into a small helper (e.g. build_module_graph_for_test_dir or
module_graph_for_test_file_common) that returns the ModuleGraph and the prepared
roots, and then call update_graph_for_css_paths and update_graph_for_js_paths
from each caller in the desired order and with the appropriate flag (the boolean
passed to update_graph_for_js_paths); update module_graph_for_css_test_file and
module_graph_for_test_file to use that helper to remove duplication.
crates/biome_html_analyze/tests/specs/nursery/noUndeclaredStyles/valid.astro
Show resolved
Hide resolved
crates/biome_html_analyze/tests/specs/nursery/noUndeclaredStyles/valid.vue
Outdated
Show resolved
Hide resolved
crates/biome_js_analyze/tests/specs/nursery/noUndeclaredStyles/upward-traversal/README.md
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/biome_module_graph/src/module_graph.rs`:
- Around line 558-628: The current CSS collection only iterates
js_info.static_import_paths (both in the initial direct-import loop and the
parent-JS loop) and thus misses CSS imported via dynamic imports; update these
loops to also iterate js_info.dynamic_import_paths (or chain static and dynamic,
e.g.,
js_info.static_import_paths.values().chain(js_info.dynamic_import_paths.values()))
when resolving import_path and calling css_module_info_for_path, and apply the
same change in any other helpers that gather CSS from a Js(ModuleInfo::Js) so
dynamic CSS imports are included in available_classes and traversal_path.
---
Duplicate comments:
In `@crates/biome_module_graph/src/module_graph.rs`:
- Around line 355-373: The ignored doctest block in the documentation for
traverse_import_tree_for_classes should be removed or converted into a real
doctest: either delete the `ignore` example entirely, or replace it with a
runnable Rust doctest that uses a small, deterministic test fixture (e.g.,
create a temporary JS/CSS module graph or a mocked ModuleGraph) and an assertion
that iterates over module_graph.traverse_import_tree_for_classes(js_path)
checking that a known class appears in the yielded CssClassStep entries; update
docs to reference CssClassStep and js_path appropriately so the doctest compiles
and passes.
- Around line 166-202: The traversal in is_class_used_in_component_tree only
enqueues js_info.static_import_paths so dynamically imported components are
never visited; update the ModuleInfo::Js branch to also iterate over the dynamic
import collection (e.g., js_info.dynamic_import_paths or
js_info.dynamic_imports) and push those resolved paths into the queue the same
way you do for static_import_paths, ensuring dynamic imports are checked for
referenced_classes as well.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@crates/biome_module_graph/src/module_graph.rs`:
- Around line 166-201: The traversal only follows js_info.static_import_paths,
so classes reachable via import() are skipped; update
is_class_used_in_component_tree to also iterate js_info.dynamic_import_paths
when queuing imports (use dynamic_import_paths.as_path() just like static), and
do the same change in the other traversal/collection loops that handle
ModuleInfo::Js (the places that check js_info.referenced_classes and then
iterate static_import_paths) so both static_import_paths and
dynamic_import_paths are traversed when collecting/checking classes.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/biome_ruledoc_utils/src/lib.rs (1)
48-104:⚠️ Potential issue | 🟠 MajorMemoryFileSystem never receives file contents.
get_added_js_paths/get_css_added_paths/get_html_added_pathsread fromfs, but nothing is inserted into it here, so the module graph stays empty. Please insert each file into the in‑memory FS.Proposed fix
for (path, src) in files { let path_buf = Utf8PathBuf::from(path); let biome_path = BiomePath::new(&path_buf); if biome_path.is_manifest() { match biome_path.file_name() { @@ _ => unimplemented!("Unhandled manifest: {biome_path}"), } } else { let document_file_source = DocumentFileSource::from_path(&path_buf, false); match document_file_source { @@ "Unhandled file type: {biome_path}. Add a new branch once the module graph understands new module types" ), } } + fs.insert(path_buf, src); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_ruledoc_utils/src/lib.rs` around lines 48 - 104, The in-memory filesystem (fs) is never populated so get_added_js_paths/get_css_added_paths/get_html_added_paths read nothing; for each (path, src) in files, write the file contents into fs using the same Utf8PathBuf (path_buf) before classifying it and pushing biome_path into js_paths/css_paths/html_paths; place this write inside the for loop (before calling DocumentFileSource::from_path) so ModuleGraph.update_graph_for_* sees file contents when invoked.crates/biome_test_utils/src/lib.rs (1)
48-103:⚠️ Potential issue | 🟠 MajorAdd
.with_working_directory(working_directory)to the success path return value.When configuration loads successfully, you return
L::resolve_analyzer_options(...)directly, which creates a freshAnalyzerOptionsand loses theworking_directoryyou set earlier. This causes configured tests to behave differently from the error path (which returnsoptions.with_configuration(...)and preserves the directory). The fix is simple:Proposed fix
L::resolve_analyzer_options( &settings, &L::lookup_settings(&settings.languages).linter, L::resolve_environment(&settings), &BiomePath::new(input_file), &DocumentFileSource::from_path( input_file, settings.experimental_full_html_support_enabled(), ), None, - ) + ) + .with_working_directory(working_directory)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_test_utils/src/lib.rs` around lines 48 - 103, The success path in create_analyzer_options currently returns the AnalyzerOptions produced by L::resolve_analyzer_options(...) which loses the earlier working_directory set on options; update the success return to preserve the working directory by calling .with_working_directory(working_directory) (or otherwise applying the working directory) to the AnalyzerOptions returned by L::resolve_analyzer_options so both error and success paths include the same working_directory; locate create_analyzer_options, the earlier options variable and the final L::resolve_analyzer_options(...) call and ensure the returned AnalyzerOptions is augmented with .with_working_directory(working_directory).
♻️ Duplicate comments (3)
crates/biome_test_utils/src/lib.rs (1)
299-313:⚠️ Potential issue | 🟠 MajorCSS discovery is limited to well‑known filenames.
from_well_knownignores ordinary.cssfiles, so directories with typical CSS names are skipped. Usefrom_pathto include extension‑based detection.Proposed fix
- DocumentFileSource::from_well_known(&path, false) + DocumentFileSource::from_path(&path, false) .is_css_like()#!/bin/bash # Inspect DocumentFileSource constructors and confirm how from_well_known behaves rg -n "from_well_known|from_path" --type=rs -C2 crates/biome_service🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_test_utils/src/lib.rs` around lines 299 - 313, The get_css_like_paths_in_dir function currently uses DocumentFileSource::from_well_known(&path, false) which only recognizes specific filenames and therefore misses ordinary .css files; change that call to DocumentFileSource::from_path(&path, false) so extension-based detection is included, keeping the surrounding logic (is_css_like().then(|| BiomePath::new(path)).into_iter().collect()) and recursion via get_css_like_paths_in_dir(&path) intact.crates/biome_module_graph/src/css_module_info/traverse.rs (1)
81-143:⚠️ Potential issue | 🟠 MajorTraversal still skips sibling importers when CSS steps are found.
As soon as the first importer with CSS is found,
return self.next()exits the scan, so other unvisited importers ofcurrent_pathnever get pushed. That can miss classes.Proposed fix
while let Some(current_path) = self.stack.pop() { let data = self.module_graph.data(); + let mut pending_css_steps: Vec<CssClassStep> = Vec::new(); // Find ALL unvisited files that import current_path // We need to process all of them, not just the first one for (file_path, module_info) in data.iter() { @@ if imports_current { self.visited.insert(file_path.clone()); // Push this parent onto the stack for further upward traversal self.stack.push(file_path.clone()); // Collect CSS files imported by this parent - let css_steps: Vec<_> = match module_info { + let css_steps: Vec<_> = match module_info { crate::ModuleInfo::Js(js_info) => js_info @@ ModuleInfo::Css(_) => Vec::new(), }; - - if !css_steps.is_empty() { - // Create a real iterator from the vector - self.current_css_iter = Some(css_steps.into_iter()); - // Recursively call next() to yield the first CSS file - return self.next(); - } + pending_css_steps.extend(css_steps); // Continue checking other files that might also import current_path // Don't break - we need to find ALL importers } } + + if !pending_css_steps.is_empty() { + self.current_css_iter = Some(pending_css_steps.into_iter()); + return self.next(); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_module_graph/src/css_module_info/traverse.rs` around lines 81 - 143, The traversal returns early when it finds the first importer with CSS (setting self.current_css_iter then calling return self.next()), which prevents remaining sibling importers from being pushed and scanned; instead, accumulate CSS steps from all matching importers in the for loop (e.g., append into a shared css_steps Vec across matches), continue iterating so every unvisited importer is self.visited and self.stack.push(...)ed, and only after the loop finishes set self.current_css_iter = Some(css_steps.into_iter()) (if non-empty) and then call return self.next(); reference self.visited, self.stack, current_css_iter, CssClassStep, module_info, and module_graph.css_module_info_for_path.crates/biome_module_graph/src/module_graph.rs (1)
372-389:⚠️ Potential issue | 🟠 MajorInclude dynamic CSS imports in traversal/diagnostics. Direct steps and import‑tree builders still ignore dynamic import paths, so CSS loaded via
import()won’t be visible in lazy traversal or diagnostics.🔧 Suggested fix
- js_info - .static_import_paths - .values() - .filter_map(|import_path| { + js_info + .static_import_paths + .values() + .chain(js_info.dynamic_import_paths.values()) + .filter_map(|import_path| { let path = import_path.as_path()?; let css_info = self.css_module_info_for_path(path)?; @@ - root.css_imports = js_info - .static_import_paths - .values() + root.css_imports = js_info + .static_import_paths + .values() + .chain(js_info.dynamic_import_paths.values()) .filter_map(|import_path| { let path = import_path.as_path()?; // Only include CSS files self.css_module_info_for_path(path)?; @@ - crate::ModuleInfo::Js(js_info) => js_info - .static_import_paths - .values() + crate::ModuleInfo::Js(js_info) => js_info + .static_import_paths + .values() + .chain(js_info.dynamic_import_paths.values()) .filter_map(|import_path| { let path = import_path.as_path()?; // Only include CSS files self.css_module_info_for_path(path)?;Also applies to: 441-452, 497-506
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_module_graph/src/module_graph.rs` around lines 372 - 389, The traversal currently only uses js_info.static_import_paths and thus omits CSS referenced by dynamic imports; update traverse_import_tree_for_classes to also iterate js_info.dynamic_import_paths (and any analogous import-tree builders in the same file) and treat each dynamic import like the static ones: resolve path with js_module_info_for_path, call css_module_info_for_path, and push a CssClassStep { css_path: ..., css_classes: ... } for each matching CSS import; repeat the same change for the other import-tree building blocks noted so dynamic import paths are included in diagnostics and lazy traversal.
🧹 Nitpick comments (2)
crates/biome_module_graph/tests/snap/mod.rs (1)
82-124: Avoid panics when formatting fails.A single invalid fixture will currently blow up the snapshot run. Consider falling back to raw source if parsing/formatting fails.
Proposed refactor
if let Ok(file_source) = JsFileSource::try_from(file_name.as_path()) { let tree = parse( source_code.as_str(), file_source, JsParserOptions::default(), ); - let formatted = - format_node(JsFormatOptions::default(), tree.tree().syntax(), false) - .unwrap() - .print() - .unwrap(); - content.push_str(formatted.as_code().trim()); + match format_node(JsFormatOptions::default(), tree.tree().syntax(), false) + .and_then(|f| f.print()) + { + Ok(formatted) => content.push_str(formatted.as_code().trim()), + Err(_) => content.push_str(source_code.trim()), + } } else if let Ok(file_source) = CssFileSource::try_from(file_name.as_path()) { let tree = biome_css_parser::parse_css( source_code.as_str(), file_source, CssParserOptions::default(), ); - let formatted = biome_css_formatter::format_node( - CssFormatOptions::default(), - tree.tree().syntax(), - ) - .unwrap() - .print() - .unwrap(); - content.push_str(formatted.as_code().trim()); + match biome_css_formatter::format_node( + CssFormatOptions::default(), + tree.tree().syntax(), + ) + .and_then(|f| f.print()) + { + Ok(formatted) => content.push_str(formatted.as_code().trim()), + Err(_) => content.push_str(source_code.trim()), + } } else if let Ok(file_source) = HtmlFileSource::try_from(file_name.as_path()) { let tree = biome_html_parser::parse_html( source_code.as_str(), HtmlParseOptions::from(&file_source), ); - let formatted = biome_html_formatter::format_node( - HtmlFormatOptions::default(), - tree.tree().syntax(), - false, - ) - .unwrap() - .print() - .unwrap(); - content.push_str(formatted.as_code().trim()); + match biome_html_formatter::format_node( + HtmlFormatOptions::default(), + tree.tree().syntax(), + false, + ) + .and_then(|f| f.print()) + { + Ok(formatted) => content.push_str(formatted.as_code().trim()), + Err(_) => content.push_str(source_code.trim()), + } } else { content.push_str(source_code.trim()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_module_graph/tests/snap/mod.rs` around lines 82 - 124, The current formatting branches unwrap results (e.g., format_node(...).unwrap().print().unwrap()) which will panic on parse/format errors; update each branch (JsFileSource::try_from -> parse -> format_node, biome_css_parser::parse_css -> biome_css_formatter::format_node, and biome_html_parser::parse_html -> biome_html_formatter::format_node) to handle errors gracefully by mapping the parse/format/print chain into a Result and on any Err fallback to content.push_str(source_code.trim()) instead of panicking; keep using the same unique symbols (JsFileSource::try_from, parse, format_node, biome_css_parser::parse_css, biome_css_formatter::format_node, biome_html_parser::parse_html, biome_html_formatter::format_node, content.push_str) so the change is local to those branches.crates/biome_module_graph/src/module_graph.rs (1)
91-148: Potential O(N²) scan in importer search. Consider a reverse‑index/cached importer map to avoid re‑scanning all modules per BFS step on large graphs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_module_graph/src/module_graph.rs` around lines 91 - 148, transitive_importers_of currently scans self.data for each BFS step causing O(N²) behavior; instead add and use a reverse-index (importers map) to look up importers in O(1). Modify the ModuleGraph type to own a cached FxHashMap<Utf8PathBuf, Vec<Utf8PathBuf>> (e.g. importers_map) that is built/updated whenever module edges are created/removed (or lazily built and pinned from self.data), provide small helper methods like record_importer(importee, importer) and remove_importer(...), and then change transitive_importers_of to query importers_map for current instead of iterating data.iter(); ensure the cache is kept in sync with mutations to ModuleInfo and use the existing pin/locking strategy (data.pin()) or interior mutability to keep thread-safety.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/biome_service/src/file_handlers/html.rs`:
- Around line 1397-1402: The lint call is not passing the new working directory
from LintParams, causing inconsistent diagnostics; update the call to
workspace_settings.analyzer_options::<HtmlLanguage>(...) used in this file (the
analyzer_options invocation for HtmlLanguage) to pass params.working_directory
instead of None so it matches JSON/CSS/GraphQL flows and threads the working
directory through the linting/code-action pipeline.
In `@crates/biome_service/src/file_handlers/javascript.rs`:
- Around line 902-907: The call to
params.settings.analyzer_options::<JsLanguage> is passing None for the
working_directory but should use the LintParams working_directory; update the
call in javascript.rs to pass params.working_directory (or
params.working_directory.as_deref() if it’s an Option) instead of None so
analyzer_options receives the correct working directory context (referencing
analyzer_options::<JsLanguage>, params.working_directory, and LintParams).
---
Outside diff comments:
In `@crates/biome_ruledoc_utils/src/lib.rs`:
- Around line 48-104: The in-memory filesystem (fs) is never populated so
get_added_js_paths/get_css_added_paths/get_html_added_paths read nothing; for
each (path, src) in files, write the file contents into fs using the same
Utf8PathBuf (path_buf) before classifying it and pushing biome_path into
js_paths/css_paths/html_paths; place this write inside the for loop (before
calling DocumentFileSource::from_path) so ModuleGraph.update_graph_for_* sees
file contents when invoked.
In `@crates/biome_test_utils/src/lib.rs`:
- Around line 48-103: The success path in create_analyzer_options currently
returns the AnalyzerOptions produced by L::resolve_analyzer_options(...) which
loses the earlier working_directory set on options; update the success return to
preserve the working directory by calling
.with_working_directory(working_directory) (or otherwise applying the working
directory) to the AnalyzerOptions returned by L::resolve_analyzer_options so
both error and success paths include the same working_directory; locate
create_analyzer_options, the earlier options variable and the final
L::resolve_analyzer_options(...) call and ensure the returned AnalyzerOptions is
augmented with .with_working_directory(working_directory).
---
Duplicate comments:
In `@crates/biome_module_graph/src/css_module_info/traverse.rs`:
- Around line 81-143: The traversal returns early when it finds the first
importer with CSS (setting self.current_css_iter then calling return
self.next()), which prevents remaining sibling importers from being pushed and
scanned; instead, accumulate CSS steps from all matching importers in the for
loop (e.g., append into a shared css_steps Vec across matches), continue
iterating so every unvisited importer is self.visited and
self.stack.push(...)ed, and only after the loop finishes set
self.current_css_iter = Some(css_steps.into_iter()) (if non-empty) and then call
return self.next(); reference self.visited, self.stack, current_css_iter,
CssClassStep, module_info, and module_graph.css_module_info_for_path.
In `@crates/biome_module_graph/src/module_graph.rs`:
- Around line 372-389: The traversal currently only uses
js_info.static_import_paths and thus omits CSS referenced by dynamic imports;
update traverse_import_tree_for_classes to also iterate
js_info.dynamic_import_paths (and any analogous import-tree builders in the same
file) and treat each dynamic import like the static ones: resolve path with
js_module_info_for_path, call css_module_info_for_path, and push a CssClassStep
{ css_path: ..., css_classes: ... } for each matching CSS import; repeat the
same change for the other import-tree building blocks noted so dynamic import
paths are included in diagnostics and lazy traversal.
In `@crates/biome_test_utils/src/lib.rs`:
- Around line 299-313: The get_css_like_paths_in_dir function currently uses
DocumentFileSource::from_well_known(&path, false) which only recognizes specific
filenames and therefore misses ordinary .css files; change that call to
DocumentFileSource::from_path(&path, false) so extension-based detection is
included, keeping the surrounding logic (is_css_like().then(||
BiomePath::new(path)).into_iter().collect()) and recursion via
get_css_like_paths_in_dir(&path) intact.
---
Nitpick comments:
In `@crates/biome_module_graph/src/module_graph.rs`:
- Around line 91-148: transitive_importers_of currently scans self.data for each
BFS step causing O(N²) behavior; instead add and use a reverse-index (importers
map) to look up importers in O(1). Modify the ModuleGraph type to own a cached
FxHashMap<Utf8PathBuf, Vec<Utf8PathBuf>> (e.g. importers_map) that is
built/updated whenever module edges are created/removed (or lazily built and
pinned from self.data), provide small helper methods like
record_importer(importee, importer) and remove_importer(...), and then change
transitive_importers_of to query importers_map for current instead of iterating
data.iter(); ensure the cache is kept in sync with mutations to ModuleInfo and
use the existing pin/locking strategy (data.pin()) or interior mutability to
keep thread-safety.
In `@crates/biome_module_graph/tests/snap/mod.rs`:
- Around line 82-124: The current formatting branches unwrap results (e.g.,
format_node(...).unwrap().print().unwrap()) which will panic on parse/format
errors; update each branch (JsFileSource::try_from -> parse -> format_node,
biome_css_parser::parse_css -> biome_css_formatter::format_node, and
biome_html_parser::parse_html -> biome_html_formatter::format_node) to handle
errors gracefully by mapping the parse/format/print chain into a Result and on
any Err fallback to content.push_str(source_code.trim()) instead of panicking;
keep using the same unique symbols (JsFileSource::try_from, parse, format_node,
biome_css_parser::parse_css, biome_css_formatter::format_node,
biome_html_parser::parse_html, biome_html_formatter::format_node,
content.push_str) so the change is local to those branches.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
Cargo.lockis excluded by!**/*.lockand included by**crates/biome_html_analyze/tests/specs/nursery/noUndeclaredStyles/valid.html.snapis excluded by!**/*.snapand included by**crates/biome_html_analyze/tests/specs/nursery/noUndeclaredStyles/valid.svelte.snapis excluded by!**/*.snapand included by**crates/biome_html_analyze/tests/specs/nursery/noUndeclaredStyles/valid.vue.snapis excluded by!**/*.snapand included by**
📒 Files selected for processing (28)
.claude/skills/README.md.claude/skills/biome-developer/SKILL.md.claude/skills/diagnostics-development/SKILL.md.claude/skills/formatter-development/SKILL.md.claude/skills/lint-rule-development/SKILL.md.claude/skills/parser-development/SKILL.md.claude/skills/prettier-compare/SKILL.md.claude/skills/rule-options/SKILL.md.claude/skills/testing-codegen/SKILL.md.claude/skills/type-inference/SKILL.mdcrates/biome_html_analyze/tests/specs/nursery/noUndeclaredStyles/valid.htmlcrates/biome_html_analyze/tests/specs/nursery/noUndeclaredStyles/valid.sveltecrates/biome_html_analyze/tests/specs/nursery/noUndeclaredStyles/valid.vuecrates/biome_module_graph/Cargo.tomlcrates/biome_module_graph/src/css_module_info/traverse.rscrates/biome_module_graph/src/module_graph.rscrates/biome_module_graph/tests/snap/mod.rscrates/biome_module_graph/tests/spec_tests.rscrates/biome_ruledoc_utils/src/lib.rscrates/biome_service/src/file_handlers/css.rscrates/biome_service/src/file_handlers/graphql.rscrates/biome_service/src/file_handlers/html.rscrates/biome_service/src/file_handlers/javascript.rscrates/biome_service/src/file_handlers/json.rscrates/biome_service/src/file_handlers/mod.rscrates/biome_service/src/workspace/server.rscrates/biome_test_utils/Cargo.tomlcrates/biome_test_utils/src/lib.rs
✅ Files skipped from review due to trivial changes (2)
- .claude/skills/lint-rule-development/SKILL.md
- .claude/skills/README.md
🚧 Files skipped from review as they are similar to previous changes (7)
- .claude/skills/prettier-compare/SKILL.md
- crates/biome_service/src/file_handlers/json.rs
- .claude/skills/diagnostics-development/SKILL.md
- .claude/skills/parser-development/SKILL.md
- crates/biome_html_analyze/tests/specs/nursery/noUndeclaredStyles/valid.svelte
- .claude/skills/testing-codegen/SKILL.md
- crates/biome_service/src/file_handlers/mod.rs
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/biome_ruledoc_utils/src/lib.rs (1)
79-87: Avoid panicking on unsupported file types.
Line 84 currently hard-fails on any non JS/CSS/HTML file. Previously such files were effectively ignored by downstream filters, so this is a behaviour change that could make ruledoc generation brittle if a doc block includes (say) JSON or YAML. Consider a softer fallback that simply skips module-graph population for unsupported types.Proposed tweak (skip unsupported types)
- _ => unimplemented!( - "Unhandled file type: {biome_path}. Add a new branch once the module graph understands new module types" - ), + _ => { + // Unsupported in the module graph for now; skip. + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_ruledoc_utils/src/lib.rs` around lines 79 - 87, The match on DocumentFileSource in the block that populates js_paths/css_paths/html_paths currently panics for any other variant (the unimplemented! at the default arm), which breaks when encountering JSON/YAML; change the default arm to a no-op that skips the file instead of panicking (e.g., log/trace the unsupported type and do nothing) so unsupported types are ignored during module-graph population; update the match on DocumentFileSource (the branch that currently uses unimplemented!) to simply continue/skip pushing biome_path rather than calling unimplemented!, keeping the existing pushes for DocumentFileSource::Js/Css/Html unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@crates/biome_ruledoc_utils/src/lib.rs`:
- Around line 79-87: The match on DocumentFileSource in the block that populates
js_paths/css_paths/html_paths currently panics for any other variant (the
unimplemented! at the default arm), which breaks when encountering JSON/YAML;
change the default arm to a no-op that skips the file instead of panicking
(e.g., log/trace the unsupported type and do nothing) so unsupported types are
ignored during module-graph population; update the match on DocumentFileSource
(the branch that currently uses unimplemented!) to simply continue/skip pushing
biome_path rather than calling unimplemented!, keeping the existing pushes for
DocumentFileSource::Js/Css/Html unchanged.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
crates/biome_cli/tests/snapshots/main_cases_handle_css_files/no_undeclared_styles_shows_checked_import_chain.snapis excluded by!**/*.snapand included by**crates/biome_module_graph/tests/snapshots/test_jsx_imports_css_file.snapis excluded by!**/*.snapand included by**
📒 Files selected for processing (5)
crates/biome_ruledoc_utils/src/lib.rscrates/biome_service/src/file_handlers/html.rscrates/biome_service/src/file_handlers/javascript.rscrates/biome_service/src/file_handlers/mod.rscrates/biome_service/src/workspace/server.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/biome_service/src/file_handlers/javascript.rs
- crates/biome_service/src/file_handlers/mod.rs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Part of #9156
Part of #9158
Warning
This PR is big, and while it could be split into two, they are very closely related in the module graph, as they share some of the data used. I suggest reviewing the PR on Graphite or using the GitHub desktop application.
Note
A lot of this PR was vibe-coded, a LOT of the code was rewritten, tweaked manually by me, and reviewed multiple times. Many tokens were used.
Note
I plan to rename the rules in the next PR. Too many changes already.
What's implemented
Module Graph
A lot of work has been done around the module graph. The module graph now collects classes defined in CSS files with a small visitor. They are tracked in
CssClassReferenceWe visit JS and HTML files, and we detect the
classandclassNameattributes. We save their values in aCssClassDeclaration. Ultimately, this data structure exposes amatchesfunction that determines whether a CSS class is used somewhere.We also visit
@importin CSS files and<style link="">in HTML files, and resolve those files.noUndeclaredStylesThis function has been implemented for JSX and HTML.
For HTML, for now, is solely focused on checking the classes defined in the
<style>tag, and reporting them.For JSX, for now, is solely focused on checking the module graph and checking undeclared CSS classes across multiple trees. Here, I also made sure to create a data structure that can be printed in the diagnostics. I believe this is very powerful and useful. Check this snapshot how it's able to show the different trees that were inspected.
The reason why, for now, they have different logics is mostly that I wanted to experiment in two separate contexts (HTML and snippets VS module graph in a JSX app), and eventually merge the functionality so that we have consistent behaviour across languages.
Doing so, eventually, will allow us to support cases in frameworks where it's possible to define CSS classes globally and scoped.
noUnusedStylesWay simpler: it uses the module graph to read all the classes defined in the modules visited.
What's missing
Test Plan
Many tests added
Docs
I haven't reviewed all the docs yet, but I will definitely reword them in the following PRs. For now I think we can skip it