Merged
Conversation
|
Merging this PR will not alter performance
Comparing Footnotes
|
a4385e6 to
20bfcdd
Compare
Contributor
WalkthroughThis PR refactors the Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/biome_module_graph/src/module_graph.rs (1)
515-524:⚠️ Potential issue | 🟡 MinorMissing
dynamic_import_pathsinbuild_import_tree_for_htmlCSS imports collection.Other locations (e.g., lines 391-401, 646-650) consistently chain
dynamic_import_paths.values()alongsidestatic_import_paths.values()when collecting CSS imports for HTML modules. This location only chainsstatic_import_paths, which appears to be an oversight.🔧 Proposed fix
let css_imports: Vec<_> = html_info .imported_stylesheets .iter() .chain(html_info.static_import_paths.values()) + .chain(html_info.dynamic_import_paths.values()) .filter_map(|stylesheet_path| { let path = stylesheet_path.as_path()?; self.css_module_info_for_path(path)?; Some(path.to_path_buf()) }) .collect();🤖 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 515 - 524, In build_import_tree_for_html, the css_imports collection only chains html_info.static_import_paths.values() and omits html_info.dynamic_import_paths.values(); update the iterator chain that builds css_imports (the html_info.imported_stylesheets.iter().chain(...)) to also chain html_info.dynamic_import_paths.values(), preserving the existing filter_map call to call self.css_module_info_for_path(path)? and collect the PathBufs so dynamic stylesheet imports are included alongside static ones.
🧹 Nitpick comments (6)
crates/biome_html_analyze/benches/html_analyzer.rs (1)
8-8: Minor style inconsistency between the twoanalyzecalls.The new import at line 8 is used at line 161 (
HtmlAnalyzerServices::default()), but the first call at line 117 still uses the fully qualified pathbiome_html_analyze::HtmlAnalyzerServices::default(). Consider picking one style for consistency.♻️ Optional: use import consistently
Either remove the import and use the fully qualified path at line 161:
- HtmlAnalyzerServices::default(), + biome_html_analyze::HtmlAnalyzerServices::default(),Or update line 117 to use the import (and remove
biome_html_analyze::prefix there).Also applies to: 161-161
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_html_analyze/benches/html_analyzer.rs` at line 8, The two calls to create the analyzer are inconsistent: one uses the fully-qualified biome_html_analyze::HtmlAnalyzerServices::default() and the other uses the imported HtmlAnalyzerServices::default(); pick one style and make them consistent by either removing the use import and using the fully-qualified path in both places or by changing the fully-qualified call to use the imported HtmlAnalyzerServices::default() so both analyze invocations use the same symbol form.crates/biome_module_graph/src/html_module_info/mod.rs (1)
116-124: Minor doc clarity: consider updating the static imports comment.The comment at lines 119-120 states dynamic imports "are ignored for upward-traversal", which remains true. However, now that
dynamic_import_pathsis explicitly tracked (line 123-124), it might be worth adding a brief note explaining the distinction—static imports drive upward traversal, whilst dynamic imports are tracked separately for CSS class resolution.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_module_graph/src/html_module_info/mod.rs` around lines 116 - 124, Update the doc comment for the struct fields to clarify the distinction between static and dynamic imports: modify the comment for static_import_paths to state that static imports (tracked in static_import_paths) drive upward-traversal, whereas dynamic imports are tracked separately in dynamic_import_paths and do not participate in upward-traversal but are still recorded (e.g., for CSS class resolution). Reference the fields static_import_paths and dynamic_import_paths when adding the brief explanatory sentence so readers can quickly see the intended behavior.crates/biome_test_utils/src/lib.rs (1)
1084-1084: Consider impact ofModuleGraphResolutionKind::Moduleson all workspace tests.This change enables module graph resolution for all
analyze_with_workspacecalls, not justnoUndeclaredClassestests. While necessary for project-domain rules, it may increase test execution time for rules that don't require the module graph.If test performance becomes a concern, consider parameterising the resolution kind or using a separate helper for module-graph-dependent tests.
🤖 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` at line 1084, The change unconditionally sets ModuleGraphResolutionKind::Modules for analyze_with_workspace, affecting all workspace tests and possibly slowing ones that don't need module graph resolution; update the test helper to accept a ModuleGraphResolutionKind parameter (defaulting to the previous, cheaper kind) or provide a new helper (e.g., analyze_with_workspace_with_graph) so only tests like noUndeclaredClasses pass ModuleGraphResolutionKind::Modules while others use the lightweight variant; update calls to analyze_with_workspace or replace them with the new helper where module-graph resolution is required.crates/biome_js_analyze/src/lint/nursery/no_undeclared_classes.rs (3)
321-406: Code duplication between semantic and non-semantic extraction.
collect_class_names_from_expression_no_semantic(lines 321-406) andcollect_class_names_from_expression(lines 439-591) share nearly identical logic for handling objects, arrays, and call expressions. Only the identifier handling differs.You could consider extracting the common logic into a shared helper that accepts an optional closure for identifier resolution. That said, the current approach is clear and self-contained — just something to keep an eye on if more expression types are added later.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_js_analyze/src/lint/nursery/no_undeclared_classes.rs` around lines 321 - 406, The two functions collect_class_names_from_expression_no_semantic and collect_class_names_from_expression duplicate handling for CallExpression, ObjectExpression, ArrayExpression, and literals; extract the shared traversal into a helper (e.g., traverse_expression_common) that takes the expression and an optional callback/closure used only for identifier resolution differences, then have both original functions call that helper supplying the appropriate identifier-resolver closure (one that uses semantic model, one that returns None/uses tokens), keeping unique identifier logic out of the common traversal.
119-144: Consider building the import tree outside the loop.The
import_treeis built for each undeclared class (lines 133-137), but the tree structure doesn't change between iterations — it depends only onfile_pathandis_html_like. Building it once before the loop and cloning for eachUndeclaredClasswould avoid redundant traversal.♻️ Proposed optimisation
+ // Build import tree once for diagnostics (only if we have undeclared classes) + let mut import_tree_cache: Option<Option<ImportTreeNode>> = None; + for entry in &class_entries { let found_class = css_steps.iter().any(|step| { step.css_classes .iter() .any(|c| c.text() == entry.name.as_ref()) }); if !found_class { - let import_tree = if is_html_like { - module_graph.build_import_tree_for_html(file_path) - } else { - module_graph.build_import_tree(file_path) - }; + let import_tree = import_tree_cache + .get_or_insert_with(|| { + if is_html_like { + module_graph.build_import_tree_for_html(file_path) + } else { + module_graph.build_import_tree(file_path) + } + }) + .clone(); signals.push(UndeclaredClass { range: entry.range, name: entry.name.clone(), import_tree, }); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_js_analyze/src/lint/nursery/no_undeclared_classes.rs` around lines 119 - 144, The loop over class_entries repeatedly calls module_graph.build_import_tree_for_html or build_import_tree for every undeclared class even though import_tree depends only on file_path and is_html_like; compute the import_tree once before iterating (use the same condition used now with is_html_like) and then clone or reuse that import_tree when constructing each UndeclaredClass pushed to signals, keeping the rest of the logic (found_class check on css_steps and pushing UndeclaredClass with range/name) unchanged.
263-316:run_without_semanticduplicates the core checking loop.The CSS step collection and undeclared class iteration (lines 284-315) mirrors the logic in
run()(lines 107-144). If this pattern grows, consider extracting a shared helper. For now, it's manageable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_js_analyze/src/lint/nursery/no_undeclared_classes.rs` around lines 263 - 316, run_without_semantic duplicates the CSS collection and undeclared-class iteration logic present in run(); extract the shared logic into a helper (e.g., a new function like collect_undeclared_classes_for_file or validate_classes_against_css) that accepts the module_graph, file_path, and the collected entries (Vec<Entry> from collect_class_names_from_expression_no_semantic) and returns Vec<UndeclaredClass>, then call that helper from both run_without_semantic and run() replacing the duplicated block that uses module_graph.traverse_import_tree_for_html_classes, module_graph.build_import_tree_for_html, and the loop that builds signals; ensure the helper references the same types (UndeclaredClass, entry.range, entry.name) and preserves the early-return behavior when css_steps.is_empty().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@crates/biome_module_graph/src/module_graph.rs`:
- Around line 515-524: In build_import_tree_for_html, the css_imports collection
only chains html_info.static_import_paths.values() and omits
html_info.dynamic_import_paths.values(); update the iterator chain that builds
css_imports (the html_info.imported_stylesheets.iter().chain(...)) to also chain
html_info.dynamic_import_paths.values(), preserving the existing filter_map call
to call self.css_module_info_for_path(path)? and collect the PathBufs so dynamic
stylesheet imports are included alongside static ones.
---
Nitpick comments:
In `@crates/biome_html_analyze/benches/html_analyzer.rs`:
- Line 8: The two calls to create the analyzer are inconsistent: one uses the
fully-qualified biome_html_analyze::HtmlAnalyzerServices::default() and the
other uses the imported HtmlAnalyzerServices::default(); pick one style and make
them consistent by either removing the use import and using the fully-qualified
path in both places or by changing the fully-qualified call to use the imported
HtmlAnalyzerServices::default() so both analyze invocations use the same symbol
form.
In `@crates/biome_js_analyze/src/lint/nursery/no_undeclared_classes.rs`:
- Around line 321-406: The two functions
collect_class_names_from_expression_no_semantic and
collect_class_names_from_expression duplicate handling for CallExpression,
ObjectExpression, ArrayExpression, and literals; extract the shared traversal
into a helper (e.g., traverse_expression_common) that takes the expression and
an optional callback/closure used only for identifier resolution differences,
then have both original functions call that helper supplying the appropriate
identifier-resolver closure (one that uses semantic model, one that returns
None/uses tokens), keeping unique identifier logic out of the common traversal.
- Around line 119-144: The loop over class_entries repeatedly calls
module_graph.build_import_tree_for_html or build_import_tree for every
undeclared class even though import_tree depends only on file_path and
is_html_like; compute the import_tree once before iterating (use the same
condition used now with is_html_like) and then clone or reuse that import_tree
when constructing each UndeclaredClass pushed to signals, keeping the rest of
the logic (found_class check on css_steps and pushing UndeclaredClass with
range/name) unchanged.
- Around line 263-316: run_without_semantic duplicates the CSS collection and
undeclared-class iteration logic present in run(); extract the shared logic into
a helper (e.g., a new function like collect_undeclared_classes_for_file or
validate_classes_against_css) that accepts the module_graph, file_path, and the
collected entries (Vec<Entry> from
collect_class_names_from_expression_no_semantic) and returns
Vec<UndeclaredClass>, then call that helper from both run_without_semantic and
run() replacing the duplicated block that uses
module_graph.traverse_import_tree_for_html_classes,
module_graph.build_import_tree_for_html, and the loop that builds signals;
ensure the helper references the same types (UndeclaredClass, entry.range,
entry.name) and preserves the early-return behavior when css_steps.is_empty().
In `@crates/biome_module_graph/src/html_module_info/mod.rs`:
- Around line 116-124: Update the doc comment for the struct fields to clarify
the distinction between static and dynamic imports: modify the comment for
static_import_paths to state that static imports (tracked in
static_import_paths) drive upward-traversal, whereas dynamic imports are tracked
separately in dynamic_import_paths and do not participate in upward-traversal
but are still recorded (e.g., for CSS class resolution). Reference the fields
static_import_paths and dynamic_import_paths when adding the brief explanatory
sentence so readers can quickly see the intended behavior.
In `@crates/biome_test_utils/src/lib.rs`:
- Line 1084: The change unconditionally sets ModuleGraphResolutionKind::Modules
for analyze_with_workspace, affecting all workspace tests and possibly slowing
ones that don't need module graph resolution; update the test helper to accept a
ModuleGraphResolutionKind parameter (defaulting to the previous, cheaper kind)
or provide a new helper (e.g., analyze_with_workspace_with_graph) so only tests
like noUndeclaredClasses pass ModuleGraphResolutionKind::Modules while others
use the lightweight variant; update calls to analyze_with_workspace or replace
them with the new helper where module-graph resolution is required.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2feba897-f9c1-4664-b2d0-70939cc0a7e0
⛔ Files ignored due to path filters (49)
crates/biome_cli/tests/snapshots/main_cases_html/no_undeclared_classes_passes_when_declared.snapis excluded by!**/*.snapand included by**crates/biome_cli/tests/snapshots/main_cases_html/no_undeclared_classes_silent_without_style_info.snapis excluded by!**/*.snapand included by**crates/biome_html_analyze/tests/specs/a11y/noRedundantRoles/astro/invalid.astro.snapis excluded by!**/*.snapand included by**crates/biome_html_analyze/tests/specs/a11y/noRedundantRoles/astro/valid.astro.snapis excluded by!**/*.snapand included by**crates/biome_html_analyze/tests/specs/a11y/noRedundantRoles/invalid.html.snapis excluded by!**/*.snapand included by**crates/biome_html_analyze/tests/specs/a11y/noRedundantRoles/invalidHtmlAamRoleGeneric.html.snapis excluded by!**/*.snapand included by**crates/biome_html_analyze/tests/specs/a11y/noRedundantRoles/svelte/invalid.svelte.snapis excluded by!**/*.snapand included by**crates/biome_html_analyze/tests/specs/a11y/noRedundantRoles/svelte/valid.svelte.snapis excluded by!**/*.snapand included by**crates/biome_html_analyze/tests/specs/a11y/noRedundantRoles/valid.html.snapis excluded by!**/*.snapand included by**crates/biome_html_analyze/tests/specs/a11y/noRedundantRoles/vue/invalid.vue.snapis excluded by!**/*.snapand included by**crates/biome_html_analyze/tests/specs/a11y/noRedundantRoles/vue/valid.vue.snapis excluded by!**/*.snapand included by**crates/biome_html_analyze/tests/specs/a11y/useAnchorContent/astro/invalid.astro.snapis excluded by!**/*.snapand included by**crates/biome_html_analyze/tests/specs/a11y/useAnchorContent/svelte/invalid.svelte.snapis excluded by!**/*.snapand included by**crates/biome_html_analyze/tests/specs/a11y/useAnchorContent/vue/invalid.vue.snapis excluded by!**/*.snapand included by**crates/biome_html_analyze/tests/specs/a11y/useKeyWithMouseEvents/astro/invalid.astro.snapis excluded by!**/*.snapand included by**crates/biome_html_analyze/tests/specs/a11y/useKeyWithMouseEvents/astro/valid.astro.snapis excluded by!**/*.snapand included by**crates/biome_html_analyze/tests/specs/a11y/useKeyWithMouseEvents/svelte/invalid.svelte.snapis excluded by!**/*.snapand included by**crates/biome_html_analyze/tests/specs/a11y/useKeyWithMouseEvents/svelte/valid.svelte.snapis excluded by!**/*.snapand included by**crates/biome_html_analyze/tests/specs/a11y/useKeyWithMouseEvents/vue/invalid.vue.snapis excluded by!**/*.snapand included by**crates/biome_html_analyze/tests/specs/a11y/useKeyWithMouseEvents/vue/valid.vue.snapis excluded by!**/*.snapand included by**crates/biome_html_analyze/tests/specs/nursery/noScriptUrl/invalid.astro.snapis excluded by!**/*.snapand included by**crates/biome_html_analyze/tests/specs/nursery/noScriptUrl/invalid.html.snapis excluded by!**/*.snapand included by**crates/biome_html_analyze/tests/specs/nursery/noScriptUrl/invalid.svelte.snapis excluded by!**/*.snapand included by**crates/biome_html_analyze/tests/specs/nursery/noScriptUrl/invalid.vue.snapis excluded by!**/*.snapand included by**crates/biome_html_analyze/tests/specs/nursery/noSyncScripts/invalid.html.snapis excluded by!**/*.snapand included by**crates/biome_html_analyze/tests/specs/nursery/noUndeclaredClasses/invalid-css-import-astro/invalid.astro.snapis excluded by!**/*.snapand included by**crates/biome_html_analyze/tests/specs/nursery/noUndeclaredClasses/invalid-scoped-styles/invalid.vue.snapis excluded by!**/*.snapand included by**crates/biome_html_analyze/tests/specs/nursery/noUndeclaredClasses/invalid-upward-traversal-astro/App.astro.snapis excluded by!**/*.snapand included by**crates/biome_html_analyze/tests/specs/nursery/noUndeclaredClasses/invalid-upward-traversal-astro/Button.astro.snapis excluded by!**/*.snapand included by**crates/biome_html_analyze/tests/specs/nursery/noUndeclaredClasses/invalid-upward-traversal-astro/Page.astro.snapis excluded by!**/*.snapand included by**crates/biome_html_analyze/tests/specs/nursery/noUndeclaredClasses/invalid-upward-traversal-vue/App.vue.snapis excluded by!**/*.snapand included by**crates/biome_html_analyze/tests/specs/nursery/noUndeclaredClasses/invalid-upward-traversal-vue/Button.vue.snapis excluded by!**/*.snapand included by**crates/biome_html_analyze/tests/specs/nursery/noUndeclaredClasses/invalid-upward-traversal-vue/Page.vue.snapis excluded by!**/*.snapand included by**crates/biome_html_analyze/tests/specs/nursery/noUndeclaredClasses/invalid.html.snapis excluded by!**/*.snapand included by**crates/biome_html_analyze/tests/specs/nursery/noUndeclaredClasses/invalid.vue.snapis excluded by!**/*.snapand included by**crates/biome_html_analyze/tests/specs/nursery/noUndeclaredClasses/valid-css-import-astro/valid.astro.snapis excluded by!**/*.snapand included by**crates/biome_html_analyze/tests/specs/nursery/noUndeclaredClasses/valid-no-style-info/valid.astro.snapis excluded by!**/*.snapand included by**crates/biome_html_analyze/tests/specs/nursery/noUndeclaredClasses/valid-no-style-info/valid.html.snapis excluded by!**/*.snapand included by**crates/biome_html_analyze/tests/specs/nursery/noUndeclaredClasses/valid-scoped-styles/valid.vue.snapis excluded by!**/*.snapand included by**crates/biome_html_analyze/tests/specs/nursery/noUndeclaredClasses/valid.astro.snapis excluded by!**/*.snapand included by**crates/biome_html_analyze/tests/specs/nursery/noUndeclaredClasses/valid.svelte.snapis excluded by!**/*.snapand included by**crates/biome_html_analyze/tests/specs/nursery/noUndeclaredClasses/valid.vue.snapis excluded by!**/*.snapand included by**crates/biome_html_analyze/tests/specs/nursery/useVueValidVCloak/invalid.vue.snapis excluded by!**/*.snapand included by**crates/biome_html_analyze/tests/specs/nursery/useVueValidVElseIf/invalid.html.snapis excluded by!**/*.snapand included by**crates/biome_html_analyze/tests/specs/nursery/useVueValidVElseIf/valid.html.snapis excluded by!**/*.snapand included by**crates/biome_html_analyze/tests/specs/source/noDuplicateClasses/invalid.html.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/noUndeclaredClasses/invalid.jsx.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/noUndeclaredClasses/valid/valid.jsx.snapis excluded by!**/*.snapand included by**packages/@biomejs/backend-jsonrpc/src/workspace.tsis excluded by!**/backend-jsonrpc/src/workspace.tsand included by**
📒 Files selected for processing (42)
crates/biome_cli/tests/cases/html.rscrates/biome_html_analyze/benches/html_analyzer.rscrates/biome_html_analyze/tests/specs/a11y/noRedundantRoles/astro/invalid.astrocrates/biome_html_analyze/tests/specs/a11y/noRedundantRoles/astro/valid.astrocrates/biome_html_analyze/tests/specs/a11y/noRedundantRoles/invalid.htmlcrates/biome_html_analyze/tests/specs/a11y/noRedundantRoles/invalidHtmlAamRoleGeneric.htmlcrates/biome_html_analyze/tests/specs/a11y/noRedundantRoles/svelte/invalid.sveltecrates/biome_html_analyze/tests/specs/a11y/noRedundantRoles/svelte/valid.sveltecrates/biome_html_analyze/tests/specs/a11y/noRedundantRoles/valid.htmlcrates/biome_html_analyze/tests/specs/a11y/noRedundantRoles/vue/invalid.vuecrates/biome_html_analyze/tests/specs/a11y/noRedundantRoles/vue/valid.vuecrates/biome_html_analyze/tests/specs/a11y/useAnchorContent/astro/invalid.astrocrates/biome_html_analyze/tests/specs/a11y/useAnchorContent/svelte/invalid.sveltecrates/biome_html_analyze/tests/specs/a11y/useAnchorContent/vue/invalid.vuecrates/biome_html_analyze/tests/specs/a11y/useKeyWithMouseEvents/vue/invalid.vuecrates/biome_html_analyze/tests/specs/a11y/useKeyWithMouseEvents/vue/valid.vuecrates/biome_html_analyze/tests/specs/nursery/noScriptUrl/invalid.astrocrates/biome_html_analyze/tests/specs/nursery/noScriptUrl/invalid.htmlcrates/biome_html_analyze/tests/specs/nursery/noScriptUrl/invalid.sveltecrates/biome_html_analyze/tests/specs/nursery/noScriptUrl/invalid.vuecrates/biome_html_analyze/tests/specs/nursery/noSyncScripts/invalid.htmlcrates/biome_html_analyze/tests/specs/nursery/noUndeclaredClasses/invalid-css-import-astro/invalid.astrocrates/biome_html_analyze/tests/specs/nursery/noUndeclaredClasses/invalid-css-import-astro/styles.csscrates/biome_html_analyze/tests/specs/nursery/noUndeclaredClasses/invalid.htmlcrates/biome_html_analyze/tests/specs/nursery/noUndeclaredClasses/valid-css-import-astro/styles.csscrates/biome_html_analyze/tests/specs/nursery/noUndeclaredClasses/valid-css-import-astro/valid.astrocrates/biome_html_analyze/tests/specs/nursery/noUndeclaredClasses/valid-no-style-info/valid.astrocrates/biome_html_analyze/tests/specs/nursery/noUndeclaredClasses/valid-no-style-info/valid.htmlcrates/biome_html_analyze/tests/specs/nursery/useVueValidVCloak/invalid.vuecrates/biome_html_analyze/tests/specs/source/noDuplicateClasses/invalid.htmlcrates/biome_js_analyze/src/lint/nursery/no_undeclared_classes.rscrates/biome_js_analyze/tests/specs/nursery/noUndeclaredClasses/invalid.jsxcrates/biome_js_analyze/tests/specs/nursery/noUndeclaredClasses/valid/valid.jsxcrates/biome_js_syntax/src/file_source.rscrates/biome_json_analyze/tests/compat_sortpkg_tests.rscrates/biome_module_graph/src/css_module_info/traverse.rscrates/biome_module_graph/src/html_module_info/mod.rscrates/biome_module_graph/src/html_module_info/visitor.rscrates/biome_module_graph/src/module_graph.rscrates/biome_service/src/embed/types.rscrates/biome_service/src/file_handlers/html.rscrates/biome_test_utils/src/lib.rs
aa38dcf to
c895967
Compare
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/biome_module_graph/src/module_graph.rs (1)
515-524:⚠️ Potential issue | 🟡 MinorMissing
dynamic_import_pathsinbuild_import_tree_for_html.This location only chains
static_import_pathsbut omitsdynamic_import_paths. Elsewhere in this file (e.g., lines 371-372, 395, 472-475), both are chained together. This inconsistency may cause dynamic CSS imports to be missing from the diagnostic import tree.🛠️ Suggested fix
let css_imports: Vec<_> = html_info .imported_stylesheets .iter() .chain(html_info.static_import_paths.values()) + .chain(html_info.dynamic_import_paths.values()) .filter_map(|stylesheet_path| { let path = stylesheet_path.as_path()?; self.css_module_info_for_path(path)?; Some(path.to_path_buf()) }) .collect();🤖 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 515 - 524, In build_import_tree_for_html, the css_imports construction only chains html_info.static_import_paths and thus omits dynamic CSS imports; update the iterator chain to include html_info.dynamic_import_paths (i.e., chain html_info.imported_stylesheets.iter().chain(html_info.static_import_paths.values()).chain(html_info.dynamic_import_paths.values())) so css_module_info_for_path is invoked for dynamic imports as well and dynamic paths are collected into css_imports.crates/biome_test_utils/src/lib.rs (1)
1101-1120:⚠️ Potential issue | 🟠 MajorDuplicate
index_files_for_testcall.Lines 1101-1107 already index all files (the primary test file plus sidecars via
files_to_index). The second call at lines 1109-1120 indexes the exact same set of files again (sidecar_paths+virtual_file_path=files_to_index).This redundant indexing may cause unexpected behaviour or simply waste cycles. One of these blocks should be removed.
🛠️ Suggested fix — remove the duplicate block
workspace.index_files_for_test( project_key, files_to_index.into_iter().map(|path| { let document_file_source = DocumentFileSource::from_well_known(path.as_path(), true); (BiomePath::new(path), document_file_source) }), ); - // Index all files through the internal indexing path so that: - // - Embedded CSS/JS blocks are parsed and stored - // - The module graph is populated with CSS classes and import edges - // This is required for project-domain rules like noUndeclaredClasses. - let mut all_virtual_files = sidecar_paths; - all_virtual_files.push(virtual_file_path.clone()); - let files_with_sources = all_virtual_files.iter().map(|path| { - let biome_path = BiomePath::new(path.clone()); - let doc_source = DocumentFileSource::from_well_known(path.as_path(), true); - (biome_path, doc_source) - }); - workspace.index_files_for_test(project_key, files_with_sources);🤖 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 1101 - 1120, Remove the duplicate indexing call: the second block that builds all_virtual_files from sidecar_paths plus virtual_file_path and then calls workspace.index_files_for_test duplicates the earlier workspace.index_files_for_test over files_to_index; delete the second block (the all_virtual_files/files_with_sources construction and its workspace.index_files_for_test call) so files are only indexed once, keeping the original files_to_index-based call and symbols workspace.index_files_for_test, files_to_index, sidecar_paths, and virtual_file_path intact.
🧹 Nitpick comments (1)
crates/biome_module_graph/src/html_module_info/mod.rs (1)
122-125: Consider expanding the docstring fordynamic_import_paths.The docstring for
static_import_paths(lines 116-121) explains the key/value semantics and notes that dynamic imports are excluded. This new field would benefit from similar detail — e.g., clarifying that these areimport("…")expressions from embedded scripts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_module_graph/src/html_module_info/mod.rs` around lines 122 - 125, The docstring for the new field dynamic_import_paths is too terse; expand it to mirror the explanatory detail given for static_import_paths by clarifying key/value semantics and what qualifies as a dynamic import (e.g., import("...") expressions in embedded scripts), noting that these are resolved paths of modules loaded via dynamic imports and how/when they are populated; update the doc comment above the dynamic_import_paths field (and optionally adjust static_import_paths for parity) to include that information and any differences from static imports.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@crates/biome_module_graph/src/module_graph.rs`:
- Around line 515-524: In build_import_tree_for_html, the css_imports
construction only chains html_info.static_import_paths and thus omits dynamic
CSS imports; update the iterator chain to include html_info.dynamic_import_paths
(i.e., chain
html_info.imported_stylesheets.iter().chain(html_info.static_import_paths.values()).chain(html_info.dynamic_import_paths.values()))
so css_module_info_for_path is invoked for dynamic imports as well and dynamic
paths are collected into css_imports.
In `@crates/biome_test_utils/src/lib.rs`:
- Around line 1101-1120: Remove the duplicate indexing call: the second block
that builds all_virtual_files from sidecar_paths plus virtual_file_path and then
calls workspace.index_files_for_test duplicates the earlier
workspace.index_files_for_test over files_to_index; delete the second block (the
all_virtual_files/files_with_sources construction and its
workspace.index_files_for_test call) so files are only indexed once, keeping the
original files_to_index-based call and symbols workspace.index_files_for_test,
files_to_index, sidecar_paths, and virtual_file_path intact.
---
Nitpick comments:
In `@crates/biome_module_graph/src/html_module_info/mod.rs`:
- Around line 122-125: The docstring for the new field dynamic_import_paths is
too terse; expand it to mirror the explanatory detail given for
static_import_paths by clarifying key/value semantics and what qualifies as a
dynamic import (e.g., import("...") expressions in embedded scripts), noting
that these are resolved paths of modules loaded via dynamic imports and how/when
they are populated; update the doc comment above the dynamic_import_paths field
(and optionally adjust static_import_paths for parity) to include that
information and any differences from static imports.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6a7eb5ae-c49a-475d-8b88-e7c937975142
⛔ Files ignored due to path filters (29)
crates/biome_cli/tests/snapshots/main_cases_html/no_undeclared_classes_passes_when_declared.snapis excluded by!**/*.snapand included by**crates/biome_cli/tests/snapshots/main_cases_html/no_undeclared_classes_silent_without_style_info.snapis excluded by!**/*.snapand included by**crates/biome_html_analyze/tests/specs/a11y/noRedundantRoles/astro/invalid.astro.snapis excluded by!**/*.snapand included by**crates/biome_html_analyze/tests/specs/a11y/noRedundantRoles/invalid.html.snapis excluded by!**/*.snapand included by**crates/biome_html_analyze/tests/specs/a11y/noRedundantRoles/invalidHtmlAamRoleGeneric.html.snapis excluded by!**/*.snapand included by**crates/biome_html_analyze/tests/specs/a11y/noRedundantRoles/svelte/invalid.svelte.snapis excluded by!**/*.snapand included by**crates/biome_html_analyze/tests/specs/a11y/noRedundantRoles/svelte/valid.svelte.snapis excluded by!**/*.snapand included by**crates/biome_html_analyze/tests/specs/a11y/noRedundantRoles/vue/invalid.vue.snapis excluded by!**/*.snapand included by**crates/biome_html_analyze/tests/specs/a11y/useAnchorContent/astro/invalid.astro.snapis excluded by!**/*.snapand included by**crates/biome_html_analyze/tests/specs/a11y/useAnchorContent/svelte/invalid.svelte.snapis excluded by!**/*.snapand included by**crates/biome_html_analyze/tests/specs/a11y/useAnchorContent/vue/invalid.vue.snapis excluded by!**/*.snapand included by**crates/biome_html_analyze/tests/specs/a11y/useKeyWithMouseEvents/vue/invalid.vue.snapis excluded by!**/*.snapand included by**crates/biome_html_analyze/tests/specs/a11y/useKeyWithMouseEvents/vue/valid.vue.snapis excluded by!**/*.snapand included by**crates/biome_html_analyze/tests/specs/nursery/noScriptUrl/invalid.astro.snapis excluded by!**/*.snapand included by**crates/biome_html_analyze/tests/specs/nursery/noScriptUrl/invalid.html.snapis excluded by!**/*.snapand included by**crates/biome_html_analyze/tests/specs/nursery/noScriptUrl/invalid.svelte.snapis excluded by!**/*.snapand included by**crates/biome_html_analyze/tests/specs/nursery/noScriptUrl/invalid.vue.snapis excluded by!**/*.snapand included by**crates/biome_html_analyze/tests/specs/nursery/noSyncScripts/invalid.html.snapis excluded by!**/*.snapand included by**crates/biome_html_analyze/tests/specs/nursery/noUndeclaredClasses/invalid-css-import-astro/invalid.astro.snapis excluded by!**/*.snapand included by**crates/biome_html_analyze/tests/specs/nursery/noUndeclaredClasses/invalid.html.snapis excluded by!**/*.snapand included by**crates/biome_html_analyze/tests/specs/nursery/noUndeclaredClasses/valid-css-import-astro/valid.astro.snapis excluded by!**/*.snapand included by**crates/biome_html_analyze/tests/specs/nursery/noUndeclaredClasses/valid-no-style-info/valid.astro.snapis excluded by!**/*.snapand included by**crates/biome_html_analyze/tests/specs/nursery/noUndeclaredClasses/valid-no-style-info/valid.html.snapis excluded by!**/*.snapand included by**crates/biome_html_analyze/tests/specs/nursery/useVueValidVCloak/invalid.vue.snapis excluded by!**/*.snapand included by**crates/biome_html_analyze/tests/specs/nursery/useVueValidVElseIf/invalid.html.snapis excluded by!**/*.snapand included by**crates/biome_html_analyze/tests/specs/source/noDuplicateClasses/invalid.html.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/noUndeclaredClasses/invalid.jsx.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/noUndeclaredClasses/valid/valid.jsx.snapis excluded by!**/*.snapand included by**packages/@biomejs/backend-jsonrpc/src/workspace.tsis excluded by!**/backend-jsonrpc/src/workspace.tsand included by**
📒 Files selected for processing (37)
crates/biome_cli/tests/cases/html.rscrates/biome_html_analyze/tests/specs/a11y/noRedundantRoles/astro/invalid.astrocrates/biome_html_analyze/tests/specs/a11y/noRedundantRoles/invalid.htmlcrates/biome_html_analyze/tests/specs/a11y/noRedundantRoles/invalidHtmlAamRoleGeneric.htmlcrates/biome_html_analyze/tests/specs/a11y/noRedundantRoles/svelte/invalid.sveltecrates/biome_html_analyze/tests/specs/a11y/noRedundantRoles/svelte/valid.sveltecrates/biome_html_analyze/tests/specs/a11y/noRedundantRoles/vue/invalid.vuecrates/biome_html_analyze/tests/specs/a11y/useAnchorContent/astro/invalid.astrocrates/biome_html_analyze/tests/specs/a11y/useAnchorContent/svelte/invalid.sveltecrates/biome_html_analyze/tests/specs/a11y/useAnchorContent/vue/invalid.vuecrates/biome_html_analyze/tests/specs/a11y/useKeyWithMouseEvents/vue/invalid.vuecrates/biome_html_analyze/tests/specs/a11y/useKeyWithMouseEvents/vue/valid.vuecrates/biome_html_analyze/tests/specs/nursery/noScriptUrl/invalid.astrocrates/biome_html_analyze/tests/specs/nursery/noScriptUrl/invalid.htmlcrates/biome_html_analyze/tests/specs/nursery/noScriptUrl/invalid.sveltecrates/biome_html_analyze/tests/specs/nursery/noScriptUrl/invalid.vuecrates/biome_html_analyze/tests/specs/nursery/noSyncScripts/invalid.htmlcrates/biome_html_analyze/tests/specs/nursery/noUndeclaredClasses/invalid-css-import-astro/invalid.astrocrates/biome_html_analyze/tests/specs/nursery/noUndeclaredClasses/invalid-css-import-astro/styles.csscrates/biome_html_analyze/tests/specs/nursery/noUndeclaredClasses/invalid.htmlcrates/biome_html_analyze/tests/specs/nursery/noUndeclaredClasses/valid-css-import-astro/styles.csscrates/biome_html_analyze/tests/specs/nursery/noUndeclaredClasses/valid-css-import-astro/valid.astrocrates/biome_html_analyze/tests/specs/nursery/noUndeclaredClasses/valid-no-style-info/valid.astrocrates/biome_html_analyze/tests/specs/nursery/noUndeclaredClasses/valid-no-style-info/valid.htmlcrates/biome_html_analyze/tests/specs/nursery/useVueValidVCloak/invalid.vuecrates/biome_html_analyze/tests/specs/source/noDuplicateClasses/invalid.htmlcrates/biome_js_analyze/src/lint/nursery/no_undeclared_classes.rscrates/biome_js_analyze/tests/specs/nursery/noUndeclaredClasses/invalid.jsxcrates/biome_js_analyze/tests/specs/nursery/noUndeclaredClasses/valid/valid.jsxcrates/biome_js_syntax/src/file_source.rscrates/biome_module_graph/src/css_module_info/traverse.rscrates/biome_module_graph/src/html_module_info/mod.rscrates/biome_module_graph/src/html_module_info/visitor.rscrates/biome_module_graph/src/module_graph.rscrates/biome_service/src/embed/types.rscrates/biome_service/src/file_handlers/html.rscrates/biome_test_utils/src/lib.rs
💤 Files with no reviewable changes (1)
- crates/biome_html_analyze/tests/specs/a11y/noRedundantRoles/svelte/valid.svelte
✅ Files skipped from review due to trivial changes (24)
- crates/biome_html_analyze/tests/specs/nursery/noScriptUrl/invalid.astro
- crates/biome_html_analyze/tests/specs/a11y/useAnchorContent/svelte/invalid.svelte
- crates/biome_html_analyze/tests/specs/a11y/useAnchorContent/astro/invalid.astro
- crates/biome_html_analyze/tests/specs/nursery/useVueValidVCloak/invalid.vue
- crates/biome_html_analyze/tests/specs/a11y/noRedundantRoles/astro/invalid.astro
- crates/biome_html_analyze/tests/specs/nursery/noScriptUrl/invalid.svelte
- crates/biome_html_analyze/tests/specs/a11y/noRedundantRoles/invalid.html
- crates/biome_html_analyze/tests/specs/nursery/noUndeclaredClasses/valid-no-style-info/valid.html
- crates/biome_html_analyze/tests/specs/a11y/noRedundantRoles/invalidHtmlAamRoleGeneric.html
- crates/biome_html_analyze/tests/specs/source/noDuplicateClasses/invalid.html
- crates/biome_html_analyze/tests/specs/nursery/noScriptUrl/invalid.vue
- crates/biome_html_analyze/tests/specs/nursery/noSyncScripts/invalid.html
- crates/biome_html_analyze/tests/specs/a11y/useKeyWithMouseEvents/vue/valid.vue
- crates/biome_html_analyze/tests/specs/a11y/noRedundantRoles/svelte/invalid.svelte
- crates/biome_html_analyze/tests/specs/nursery/noUndeclaredClasses/invalid-css-import-astro/styles.css
- crates/biome_html_analyze/tests/specs/a11y/useKeyWithMouseEvents/vue/invalid.vue
- crates/biome_html_analyze/tests/specs/a11y/noRedundantRoles/vue/invalid.vue
- crates/biome_html_analyze/tests/specs/nursery/noUndeclaredClasses/valid-no-style-info/valid.astro
- crates/biome_html_analyze/tests/specs/nursery/noScriptUrl/invalid.html
- crates/biome_cli/tests/cases/html.rs
- crates/biome_html_analyze/tests/specs/nursery/noUndeclaredClasses/valid-css-import-astro/valid.astro
- crates/biome_html_analyze/tests/specs/nursery/noUndeclaredClasses/invalid.html
- crates/biome_html_analyze/tests/specs/nursery/noUndeclaredClasses/invalid-css-import-astro/invalid.astro
- crates/biome_html_analyze/tests/specs/a11y/useAnchorContent/vue/invalid.vue
🚧 Files skipped from review as they are similar to previous changes (6)
- crates/biome_html_analyze/tests/specs/nursery/noUndeclaredClasses/valid-css-import-astro/styles.css
- crates/biome_module_graph/src/css_module_info/traverse.rs
- crates/biome_js_analyze/tests/specs/nursery/noUndeclaredClasses/valid/valid.jsx
- crates/biome_module_graph/src/html_module_info/visitor.rs
- crates/biome_js_analyze/tests/specs/nursery/noUndeclaredClasses/invalid.jsx
- crates/biome_js_analyze/src/lint/nursery/no_undeclared_classes.rs
dyc3
approved these changes
Mar 29, 2026
Member
Author
There was a problem hiding this comment.
I double checked, the source file is there
Comment on lines
+22
to
+25
| /// This rule checks string literals, variable references (resolved through the semantic | ||
| /// model), call expressions like `clsx(...)` / `classnames(...)`, object expression keys, | ||
| /// and array expressions. Dynamic class names that cannot be statically resolved are | ||
| /// silently skipped. |
Contributor
There was a problem hiding this comment.
tailwind users are going to get a bunch of false positives from this rule, at least until we start handling tailwind configs. I would at least leave a note about that.
Member
Author
There was a problem hiding this comment.
Yeah definitely. It's on my to-do list
This was referenced Apr 7, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR improves noUndeclaredClasses on two fronts:
class:listbindingsclassName={someClass}Tests created with an AI agent.
Architecture designed by me and implemented partially by me.
Test Plan
Added many tests. Some CLI tests were moved into the analyze infra.
Docs