feat(biome_js_analyze): custom jsxFactory and jsxFragmentFactory#7847
feat(biome_js_analyze): custom jsxFactory and jsxFragmentFactory#7847ematipico merged 6 commits intobiomejs:nextfrom
Conversation
🦋 Changeset detectedLatest commit: b79854e 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 |
|
Caution Review failedThe pull request is closed. WalkthroughAdds support for custom JSX factory identifiers throughout the analyzer: new options Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/biome_service/src/settings.rs (1)
1796-1819: Propagate factory settings in overrides tooOverrides currently merge only jsx_runtime; jsx_factory/jsx_fragment_factory are dropped, which can surprise users overriding per‑path settings.
Consider:
- language_setting.environment.jsx_runtime = - conf.jsx_runtime.or(parent_settings.environment.jsx_runtime); + language_setting.environment.jsx_runtime = + conf.jsx_runtime.or(parent_settings.environment.jsx_runtime); + language_setting.environment.jsx_factory = + conf.jsx_factory.or_else(|| parent_settings.environment.jsx_factory.clone()); + language_setting.environment.jsx_fragment_factory = + conf.jsx_fragment_factory.or_else(|| parent_settings.environment.jsx_fragment_factory.clone());
🧹 Nitpick comments (9)
crates/biome_js_analyze/src/react.rs (1)
338-345: Consider validating import source.Unlike
is_global_react_import(which checks declaration type and import source), this function only checks the binding name. This might match local variables with the same name as the factory.If the JSX factory configuration expects imports from specific modules (e.g., 'preact'), consider adding source validation similar to
is_global_react_import.crates/biome_configuration/src/javascript/mod.rs (1)
48-60: JSX factory fields: good addition; consider normalising inputsTwo minor polish items:
- Trim whitespace on values at read time to avoid config nits.
- Clarify in rustdoc that these should be base identifiers (before any dot), since downstream code often expects that.
Happy path as-is; these are non-blocking.
crates/biome_js_analyze/tests/spec_tests.rs (1)
182-200: Avoid double tsconfig lookup and trim identifiers
- Query tsconfig once and reuse for factory + fragment.
- Trim the identifiers to be safe.
Example tweak:
- if options.jsx_runtime() == Some(JsxRuntime::ReactClassic) { - if options.jsx_factory().is_none() { - let factory = project_layout.query_tsconfig_for_path(input_file, |tsconfig| { - tsconfig.jsx_factory_identifier().map(|s| s.to_string()) - }).flatten(); - options.set_jsx_factory(factory.map(|s| s.into())); - } - if options.jsx_fragment_factory().is_none() { - let fragment_factory = project_layout.query_tsconfig_for_path(input_file, |tsconfig| { - tsconfig.jsx_fragment_factory_identifier().map(|s| s.to_string()) - }).flatten(); - options.set_jsx_fragment_factory(fragment_factory.map(|s| s.into())); - } - } + if options.jsx_runtime() == Some(JsxRuntime::ReactClassic) { + if options.jsx_factory().is_none() || options.jsx_fragment_factory().is_none() { + if let Some(tsconfig) = project_layout.find_tsconfig_for_path(input_file) { + if options.jsx_factory().is_none() { + let f = tsconfig.jsx_factory_identifier().map(|s| s.trim().to_string()); + options.set_jsx_factory(f.map(Into::into)); + } + if options.jsx_fragment_factory().is_none() { + let ff = tsconfig.jsx_fragment_factory_identifier().map(|s| s.trim().to_string()); + options.set_jsx_fragment_factory(ff.map(Into::into)); + } + } + } + }crates/biome_js_analyze/src/lint/correctness/no_unused_imports.rs (1)
626-642: Nice exemption for custom JSX factories; consider sharing helperLogic is sound and fixes the false positives. To DRY with use_import_type, extract a shared helper (e.g. react::is_jsx_factory_binding(binding, ctx)) so both rules stay in lockstep when this grows.
crates/biome_analyze/src/context.rs (1)
164-172: Doc nudge: clarify expected shape of identifiersPlease mention these are base identifiers (pre‑dot), e.g. “React” not “React.createElement”, matching what services/options pass in. Keeps invariants obvious for rule authors.
crates/biome_service/src/file_handlers/javascript.rs (1)
346-359: Minor normalisation: trim and gate by runtimeTwo tiny tweaks:
- Trim before split to avoid stray spaces.
- Optionally only forward factories when runtime is ReactClassic (purely to reduce surprise).
Example:
- let jsx_factory = environment + let jsx_factory = (matches!(jsx_runtime, biome_analyze::options::JsxRuntime::ReactClassic)) + .then_some(environment) .and_then(|env| env.jsx_factory.as_ref()) - .and_then(|factory| factory.split('.').next()) + .map(|s| s.trim()) + .and_then(|factory| factory.split('.').next()) .map(|s| s.into()); - let jsx_fragment_factory = environment + let jsx_fragment_factory = (matches!(jsx_runtime, biome_analyze::options::JsxRuntime::ReactClassic)) + .then_some(environment) .and_then(|env| env.jsx_fragment_factory.as_ref()) - .and_then(|factory| factory.split('.').next()) + .map(|s| s.trim()) + .and_then(|factory| factory.split('.').next()) .map(|s| s.into());crates/biome_package/src/node_js_package/tsconfig_json.rs (1)
120-136: Trim before splitting to be tolerant of whitespaceA tiny guard improves resilience:
- pub fn jsx_factory_identifier(&self) -> Option<&str> { - self.compiler_options - .jsx_factory - .as_ref() - .map(|factory| factory.split('.').next().unwrap_or(factory.as_str())) - } + pub fn jsx_factory_identifier(&self) -> Option<&str> { + self.compiler_options.jsx_factory.as_deref().map(|factory| { + let f = factory.trim(); + f.split('.').next().unwrap_or(f) + }) + }Apply the same to jsx_fragment_factory_identifier.
crates/biome_js_analyze/src/lint/style/use_import_type.rs (1)
1101-1129: Centraliseis_jsx_factory_bindingto avoid driftGreat extraction. Consider moving this to
crate::react(public helper) so both this rule andnoUnusedImportsshare one source of truth for JSX‑factory recognition. Less chance of future divergence.crates/biome_analyze/src/options.rs (1)
179-193: Consider adding rustdoc and reordering methods.Two suggestions to improve maintainability:
Missing rustdoc comments: Per the coding guidelines, "Document rules, assists, and options with inline rustdoc in source". These public API methods should have doc comments explaining their purpose and return values.
Method ordering: Grouping each field's getter and setter together would improve readability. Current order (jsx_factory getter → factory setter → fragment setter → fragment getter) is a bit unusual.
Example improvement:
+ /// Returns the configured JSX factory identifier, if any (e.g., "h" for Preact). pub fn jsx_factory(&self) -> Option<&str> { self.configuration.jsx_factory.as_deref() } + /// Sets the JSX factory identifier for the classic JSX runtime. pub fn set_jsx_factory(&mut self, jsx_factory: Option<Box<str>>) { self.configuration.jsx_factory = jsx_factory; } - pub fn set_jsx_fragment_factory(&mut self, jsx_fragment_factory: Option<Box<str>>) { - self.configuration.jsx_fragment_factory = jsx_fragment_factory; - } - + /// Returns the configured JSX fragment factory identifier, if any (e.g., "Fragment"). pub fn jsx_fragment_factory(&self) -> Option<&str> { self.configuration.jsx_fragment_factory.as_deref() } + + /// Sets the JSX fragment factory identifier for the classic JSX runtime. + pub fn set_jsx_fragment_factory(&mut self, jsx_fragment_factory: Option<Box<str>>) { + self.configuration.jsx_fragment_factory = jsx_fragment_factory; + }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
crates/biome_js_analyze/tests/specs/correctness/noUnusedImports/jsxFactoryPreact.js.snapis excluded by!**/*.snapand included by**
📒 Files selected for processing (17)
crates/biome_analyze/src/context.rs(4 hunks)crates/biome_analyze/src/options.rs(3 hunks)crates/biome_analyze/src/registry.rs(2 hunks)crates/biome_analyze/src/signals.rs(3 hunks)crates/biome_configuration/src/javascript/mod.rs(1 hunks)crates/biome_js_analyze/src/lint/correctness/no_unused_imports.rs(2 hunks)crates/biome_js_analyze/src/lint/style/use_import_type.rs(6 hunks)crates/biome_js_analyze/src/react.rs(1 hunks)crates/biome_js_analyze/tests/quick_test.rs(1 hunks)crates/biome_js_analyze/tests/spec_tests.rs(1 hunks)crates/biome_js_analyze/tests/specs/correctness/noUnusedImports/jsxFactoryPreact.js(1 hunks)crates/biome_js_analyze/tests/specs/correctness/noUnusedImports/jsxFactoryPreact.options.json(1 hunks)crates/biome_js_analyze/tests/specs/correctness/noUnusedImports/jsxFactoryPreact.tsconfig.json(1 hunks)crates/biome_package/src/node_js_package/tsconfig_json.rs(2 hunks)crates/biome_service/src/file_handlers/javascript.rs(2 hunks)crates/biome_service/src/file_handlers/mod.rs(1 hunks)crates/biome_service/src/settings.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
crates/biome_*/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place core crates under /crates/biome_*/
Files:
crates/biome_analyze/src/registry.rscrates/biome_js_analyze/src/react.rscrates/biome_service/src/settings.rscrates/biome_js_analyze/tests/specs/correctness/noUnusedImports/jsxFactoryPreact.tsconfig.jsoncrates/biome_js_analyze/src/lint/correctness/no_unused_imports.rscrates/biome_analyze/src/signals.rscrates/biome_configuration/src/javascript/mod.rscrates/biome_package/src/node_js_package/tsconfig_json.rscrates/biome_js_analyze/tests/specs/correctness/noUnusedImports/jsxFactoryPreact.jscrates/biome_service/src/file_handlers/mod.rscrates/biome_js_analyze/tests/specs/correctness/noUnusedImports/jsxFactoryPreact.options.jsoncrates/biome_js_analyze/tests/spec_tests.rscrates/biome_js_analyze/tests/quick_test.rscrates/biome_analyze/src/options.rscrates/biome_analyze/src/context.rscrates/biome_service/src/file_handlers/javascript.rscrates/biome_js_analyze/src/lint/style/use_import_type.rs
**/*.rs
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.rs: Format Rust files before committing (e.g., viajust fwhich formats Rust)
Document rules, assists, and options with inline rustdoc in source
Files:
crates/biome_analyze/src/registry.rscrates/biome_js_analyze/src/react.rscrates/biome_service/src/settings.rscrates/biome_js_analyze/src/lint/correctness/no_unused_imports.rscrates/biome_analyze/src/signals.rscrates/biome_configuration/src/javascript/mod.rscrates/biome_package/src/node_js_package/tsconfig_json.rscrates/biome_service/src/file_handlers/mod.rscrates/biome_js_analyze/tests/spec_tests.rscrates/biome_js_analyze/tests/quick_test.rscrates/biome_analyze/src/options.rscrates/biome_analyze/src/context.rscrates/biome_service/src/file_handlers/javascript.rscrates/biome_js_analyze/src/lint/style/use_import_type.rs
crates/biome_*_{syntax,parser,formatter,analyze,factory,semantic}/**
📄 CodeRabbit inference engine (CLAUDE.md)
Maintain the per-language crate structure: biome_{lang}_{syntax,parser,formatter,analyze,factory,semantic}
Files:
crates/biome_js_analyze/src/react.rscrates/biome_js_analyze/tests/specs/correctness/noUnusedImports/jsxFactoryPreact.tsconfig.jsoncrates/biome_js_analyze/src/lint/correctness/no_unused_imports.rscrates/biome_js_analyze/tests/specs/correctness/noUnusedImports/jsxFactoryPreact.jscrates/biome_js_analyze/tests/specs/correctness/noUnusedImports/jsxFactoryPreact.options.jsoncrates/biome_js_analyze/tests/spec_tests.rscrates/biome_js_analyze/tests/quick_test.rscrates/biome_js_analyze/src/lint/style/use_import_type.rs
**/tests/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place test files under a tests/ directory in each crate
Files:
crates/biome_js_analyze/tests/specs/correctness/noUnusedImports/jsxFactoryPreact.tsconfig.jsoncrates/biome_js_analyze/tests/specs/correctness/noUnusedImports/jsxFactoryPreact.jscrates/biome_js_analyze/tests/specs/correctness/noUnusedImports/jsxFactoryPreact.options.jsoncrates/biome_js_analyze/tests/spec_tests.rscrates/biome_js_analyze/tests/quick_test.rs
crates/biome_configuration/src/**
📄 CodeRabbit inference engine (CLAUDE.md)
Keep configuration sources under biome_configuration/src/
Files:
crates/biome_configuration/src/javascript/mod.rs
🧠 Learnings (3)
📚 Learning: 2025-10-15T09:20:19.139Z
Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-10-15T09:20:19.139Z
Learning: Applies to crates/biome_analyze/**/lib/src/{lint,assist}/**/*.rs : Retrieve rule options via ctx.options() inside rules; do not manually handle overrides/extends resolution
Applied to files:
crates/biome_analyze/src/registry.rs
📚 Learning: 2025-10-15T09:20:19.139Z
Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-10-15T09:20:19.139Z
Learning: Applies to crates/biome_analyze/**/lib/src/{lint,assist}/**/*.rs : Set the language field in declare_lint_rule! to the most specific applicable language (js, jsx, ts, or tsx)
Applied to files:
crates/biome_service/src/settings.rscrates/biome_service/src/file_handlers/mod.rs
📚 Learning: 2025-10-15T09:20:19.139Z
Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-10-15T09:20:19.139Z
Learning: Applies to crates/biome_analyze/**/tests/quick_test.rs : Use the quick test at biome_js_analyze/tests/quick_test.rs by un-ignoring and adjusting SOURCE and RuleFilter for ad-hoc checks
Applied to files:
crates/biome_js_analyze/tests/quick_test.rs
🧬 Code graph analysis (10)
crates/biome_analyze/src/registry.rs (2)
crates/biome_analyze/src/context.rs (2)
jsx_factory(165-167)jsx_fragment_factory(170-172)crates/biome_analyze/src/options.rs (2)
jsx_factory(179-181)jsx_fragment_factory(191-193)
crates/biome_service/src/settings.rs (2)
crates/biome_analyze/src/context.rs (3)
jsx_runtime(160-162)jsx_factory(165-167)jsx_fragment_factory(170-172)crates/biome_analyze/src/options.rs (3)
jsx_runtime(175-177)jsx_factory(179-181)jsx_fragment_factory(191-193)
crates/biome_js_analyze/src/lint/correctness/no_unused_imports.rs (2)
crates/biome_js_analyze/src/react.rs (2)
is_global_react_import(305-336)is_jsx_factory_import(341-345)crates/biome_analyze/src/context.rs (2)
jsx_factory(165-167)jsx_fragment_factory(170-172)
crates/biome_service/src/file_handlers/mod.rs (3)
crates/biome_service/src/settings.rs (1)
analyzer_options(1119-1139)crates/biome_analyze/src/context.rs (1)
options(155-157)packages/@biomejs/backend-jsonrpc/src/workspace.ts (1)
JsxRuntime(701-701)
crates/biome_js_analyze/tests/spec_tests.rs (2)
packages/@biomejs/backend-jsonrpc/src/workspace.ts (1)
JsxRuntime(701-701)crates/biome_js_analyze/src/services/module_graph.rs (1)
project_layout(23-25)
crates/biome_js_analyze/tests/quick_test.rs (5)
packages/@biomejs/backend-jsonrpc/src/workspace.ts (4)
JsFileSource(9483-9492)TextRange(9327-9327)JsxRuntime(701-701)Severity(9308-9308)crates/biome_js_syntax/src/file_source.rs (1)
jsx(168-170)crates/biome_analyze/src/context.rs (3)
file_path(187-189)new(33-65)options(155-157)crates/biome_js_analyze/src/lib.rs (1)
analyze(188-209)crates/biome_diagnostics/src/lib.rs (1)
print_diagnostic_to_string(85-103)
crates/biome_analyze/src/options.rs (1)
crates/biome_analyze/src/context.rs (2)
jsx_factory(165-167)jsx_fragment_factory(170-172)
crates/biome_analyze/src/context.rs (1)
crates/biome_analyze/src/options.rs (2)
jsx_factory(179-181)jsx_fragment_factory(191-193)
crates/biome_service/src/file_handlers/javascript.rs (3)
packages/@biomejs/backend-jsonrpc/src/workspace.ts (1)
JsxRuntime(701-701)crates/biome_analyze/src/context.rs (3)
jsx_runtime(160-162)jsx_factory(165-167)jsx_fragment_factory(170-172)crates/biome_analyze/src/options.rs (4)
jsx_runtime(175-177)jsx_factory(179-181)jsx_fragment_factory(191-193)configuration(199-201)
crates/biome_js_analyze/src/lint/style/use_import_type.rs (3)
crates/biome_js_analyze/src/react.rs (2)
is_global_react_import(305-336)is_jsx_factory_import(341-345)packages/@biomejs/backend-jsonrpc/src/workspace.ts (1)
JsxRuntime(701-701)crates/biome_analyze/src/context.rs (2)
jsx_factory(165-167)jsx_fragment_factory(170-172)
🔇 Additional comments (13)
crates/biome_service/src/file_handlers/mod.rs (1)
1630-1647: Well-structured tsconfig integration.The logic correctly queries tsconfig.json for JSX factory settings only when needed (ReactClassic runtime, factories not already set). The defensive checks and use of
flatten()are appropriate.crates/biome_analyze/src/registry.rs (2)
409-410: LGTM.Consistent with the existing pattern for extracting options.
424-425: LGTM.Correctly threads JSX factory options through to RuleContext.
crates/biome_js_analyze/tests/specs/correctness/noUnusedImports/jsxFactoryPreact.js (1)
1-10: Good test fixture for Preact JSX support.The test case appropriately demonstrates custom JSX factory usage with Preact's
handFragment.crates/biome_js_analyze/tests/specs/correctness/noUnusedImports/jsxFactoryPreact.options.json (1)
1-6: LGTM.Correctly configures ReactClassic runtime for the test scenario.
crates/biome_js_analyze/tests/specs/correctness/noUnusedImports/jsxFactoryPreact.tsconfig.json (1)
1-8: LGTM.Standard Preact configuration demonstrating the feature.
crates/biome_analyze/src/signals.rs (3)
373-374: LGTM.Correctly propagates JSX factory options to RuleContext in the diagnostic path.
412-413: LGTM.Consistent with the diagnostic path.
479-480: LGTM.Completes the JSX factory propagation across all execution paths.
crates/biome_js_analyze/tests/quick_test.rs (1)
99-157: Solid test coverage for JSX factory support.The test clearly demonstrates that custom JSX factories (h, Fragment) are correctly recognised as used when configured via ReactClassic runtime options.
crates/biome_service/src/settings.rs (1)
500-510: Environment wiring for JSX factories looks rightThe environment now carries jsx_runtime + factory fields; aligns with downstream usage. No blockers here.
crates/biome_analyze/src/options.rs (2)
78-82: LGTM!The field declarations are well-commented and use appropriate types for optional JSX factory configuration.
104-112: LGTM!The builder methods follow the established pattern and enable fluent configuration chaining.
ematipico
left a comment
There was a problem hiding this comment.
That's great, thank you! I left some comments, however the whole logic seems solid :)
ematipico
left a comment
There was a problem hiding this comment.
Ah, remember to:
- point this PR to
next - create a changeset
https://github.com/biomejs/biome?tab=contributing-ov-file#creating-pull-requests
CodSpeed Performance ReportMerging #7847 will not alter performanceComparing Summary
Footnotes
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
crates/biome_service/src/file_handlers/mod.rs (1)
1637-1653: Minor allocs: avoid intermediate String if the setters accept SmolStr/Cow.You’re creating a fresh String per analysis via to_string(). If set_jsx_factory/set_jsx_fragment_factory accept SmolStr or Cow<'static, str>, you can map directly from &str and skip the extra hop. If not, feel free to ignore — this was raised before and deemed acceptable.
Run to confirm the setter signatures:
#!/bin/bash # Inspect setter types so we can decide if we can map &str -> SmolStr/Cow directly. rg -nP -C2 'set_jsx_(fragment_)?factory\s*\(' --type=rust
🧹 Nitpick comments (4)
crates/biome_js_analyze/tests/quick_test.rs (3)
100-100: Consider clarifying the test name.The name
test_jsx_factory_from_tsconfigmight suggest the test reads from an actual tsconfig file, when it actually validates the analyzer behaviour after JSX factory options are set programmatically (simulating what would happen when tsconfig is parsed). The current name is acceptable but could be more precise.
99-157: Consider adding a negative test case.The test validates that with JSX factory configuration, imports are correctly preserved. For a more robust test, consider adding a companion test that proves WITHOUT the configuration, the same imports WOULD be flagged as unused by
noUnusedImports. This would demonstrate the configuration actually changes behaviour.
99-157: Edge cases to consider for future tests.Whilst the current test covers the primary use case well, additional test cases could be valuable:
- Only
jsx_factoryset (no fragment factory)- JSX factory configured but import uses a different identifier
- Non-ReactClassic runtime with factory configuration
These can be addressed in follow-up work if needed.
crates/biome_service/src/file_handlers/mod.rs (1)
1636-1655: Avoid double tsconfig queries; fetch both identifiers in one go.Call query_tsconfig_for_path once and pull both factory and fragment to reduce overhead (IO/cache lookup/locks). Keeps the hot path lean.
Apply this diff within the current block:
- if analyzer_options.jsx_factory().is_none() { - let factory = self - .project_layout - .query_tsconfig_for_path(path, |tsconfig| { - tsconfig.jsx_factory_identifier().map(|s| s.to_string()) - }) - .flatten(); - analyzer_options.set_jsx_factory(factory.map(|s| s.into())); - } - if analyzer_options.jsx_fragment_factory().is_none() { - let fragment_factory = self - .project_layout - .query_tsconfig_for_path(path, |tsconfig| { - tsconfig - .jsx_fragment_factory_identifier() - .map(|s| s.to_string()) - }) - .flatten(); - analyzer_options.set_jsx_fragment_factory(fragment_factory.map(|s| s.into())); - } + let need_factory = analyzer_options.jsx_factory().is_none(); + let need_fragment = analyzer_options.jsx_fragment_factory().is_none(); + if need_factory || need_fragment { + if let Some((factory, fragment)) = self.project_layout.query_tsconfig_for_path(path, |tsconfig| { + ( + tsconfig.jsx_factory_identifier().map(|s| s.to_string()), + tsconfig.jsx_fragment_factory_identifier().map(|s| s.to_string()), + ) + }) { + if need_factory { + analyzer_options.set_jsx_factory(factory.map(|s| s.into())); + } + if need_fragment { + analyzer_options.set_jsx_fragment_factory(fragment.map(|s| s.into())); + } + } + }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
packages/@biomejs/backend-jsonrpc/src/workspace.tsis excluded by!**/backend-jsonrpc/src/workspace.tsand included by**packages/@biomejs/biome/configuration_schema.jsonis excluded by!**/configuration_schema.jsonand included by**
📒 Files selected for processing (4)
crates/biome_js_analyze/src/lint/style/use_import_type.rs(6 hunks)crates/biome_js_analyze/tests/quick_test.rs(1 hunks)crates/biome_js_analyze/tests/spec_tests.rs(1 hunks)crates/biome_service/src/file_handlers/mod.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/biome_js_analyze/src/lint/style/use_import_type.rs
- crates/biome_js_analyze/tests/spec_tests.rs
🧰 Additional context used
📓 Path-based instructions (4)
crates/biome_*_{syntax,parser,formatter,analyze,factory,semantic}/**
📄 CodeRabbit inference engine (CLAUDE.md)
Maintain the per-language crate structure: biome_{lang}_{syntax,parser,formatter,analyze,factory,semantic}
Files:
crates/biome_js_analyze/tests/quick_test.rs
crates/biome_*/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place core crates under /crates/biome_*/
Files:
crates/biome_js_analyze/tests/quick_test.rscrates/biome_service/src/file_handlers/mod.rs
**/tests/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place test files under a tests/ directory in each crate
Files:
crates/biome_js_analyze/tests/quick_test.rs
**/*.rs
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.rs: Format Rust files before committing (e.g., viajust fwhich formats Rust)
Document rules, assists, and options with inline rustdoc in source
Files:
crates/biome_js_analyze/tests/quick_test.rscrates/biome_service/src/file_handlers/mod.rs
🧠 Learnings (2)
📚 Learning: 2025-10-24T21:24:58.631Z
Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-10-24T21:24:58.631Z
Learning: Applies to crates/biome_analyze/crates/*_analyze/tests/quick_test.rs : Quick test lives in biome_js_analyze/tests/quick_test.rs; unignore #[ignore] and set rule filter and SOURCE for ad-hoc runs
Applied to files:
crates/biome_js_analyze/tests/quick_test.rs
📚 Learning: 2025-10-24T21:24:58.631Z
Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-10-24T21:24:58.631Z
Learning: Applies to crates/biome_analyze/crates/*_analyze/**/src/**/lint/**/*.rs : Set the language field in declare_lint_rule! to the most appropriate dialect (js/jsx/ts/tsx)
Applied to files:
crates/biome_service/src/file_handlers/mod.rs
🧬 Code graph analysis (2)
crates/biome_js_analyze/tests/quick_test.rs (4)
packages/@biomejs/backend-jsonrpc/src/workspace.ts (3)
JsFileSource(9491-9500)TextRange(9335-9335)JsxRuntime(709-709)crates/biome_js_syntax/src/file_source.rs (1)
jsx(168-170)crates/biome_analyze/src/context.rs (3)
file_path(187-189)new(33-65)options(155-157)crates/biome_js_analyze/src/lib.rs (1)
analyze(188-209)
crates/biome_service/src/file_handlers/mod.rs (2)
crates/biome_service/src/settings.rs (1)
analyzer_options(1119-1139)packages/@biomejs/backend-jsonrpc/src/workspace.ts (1)
JsxRuntime(709-709)
🔇 Additional comments (1)
crates/biome_service/src/file_handlers/mod.rs (1)
1630-1657: Good guardrails and non-overriding defaults.Nice: only read tsconfig when jsx_runtime is ReactClassic and existing options are unset. This preserves explicit config and fixes the false positives.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
crates/biome_package/src/node_js_package/tsconfig_json.rs (1)
350-365: Consider renaming this test.The test name suggests it verifies that normalization happens during deserialization (not on every access), but it only validates correctness by calling the accessor multiple times. Both approaches (normalize once vs normalize on every call) would pass this test. Consider renaming to
test_jsx_factory_repeated_accessor adding a comment clarifying it's a stress test rather than an efficiency verification.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
crates/biome_cli/tests/snapshots/main_cases_handle_astro_files/format_astro_with_typescript_script_tag.snapis excluded by!**/*.snapand included by**crates/biome_package/tests/tsconfig/valid/tsconfig.valid.baseUrl.json.snapis excluded by!**/*.snapand included by**crates/biome_package/tests/tsconfig/valid/tsconfig.valid.paths.json.snapis excluded by!**/*.snapand included by**
📒 Files selected for processing (9)
.changeset/eight-eels-pull.md(1 hunks).changeset/ten-hoops-unite.md(1 hunks)crates/biome_cli/tests/cases/handle_astro_files.rs(2 hunks)crates/biome_js_analyze/tests/quick_test.rs(1 hunks)crates/biome_package/src/node_js_package/tsconfig_json.rs(3 hunks)crates/biome_package/tests/tsconfig/valid/tsconfig.valid.baseUrl.json.snap.new(1 hunks)crates/biome_package/tests/tsconfig/valid/tsconfig.valid.paths.json.snap.new(1 hunks)crates/biome_service/src/file_handlers/html.rs(1 hunks)crates/biome_service/src/settings.rs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- crates/biome_package/tests/tsconfig/valid/tsconfig.valid.baseUrl.json.snap.new
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/biome_service/src/settings.rs
- crates/biome_js_analyze/tests/quick_test.rs
🧰 Additional context used
📓 Path-based instructions (4)
.changeset/*.md
📄 CodeRabbit inference engine (CONTRIBUTING.md)
.changeset/*.md: In changesets, only use #### or ##### headers; other header levels are not allowed
Changesets should cover user-facing changes only; internal changes do not need changesets
Use past tense for what you did and present tense for current Biome behavior in changesets
When fixing a bug in a changeset, start with an issue link (e.g., “Fixed #1234: …”)
When referencing a rule or assist in a changeset, include a link to its page on the website
Include code blocks in changesets when applicable to illustrate changes
End every sentence in a changeset with a period
Files:
.changeset/ten-hoops-unite.md.changeset/eight-eels-pull.md
crates/biome_*/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place core crates under /crates/biome_*/
Files:
crates/biome_package/src/node_js_package/tsconfig_json.rscrates/biome_package/tests/tsconfig/valid/tsconfig.valid.paths.json.snap.newcrates/biome_service/src/file_handlers/html.rscrates/biome_cli/tests/cases/handle_astro_files.rs
**/*.rs
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.rs: Format Rust files before committing (e.g., viajust fwhich formats Rust)
Document rules, assists, and options with inline rustdoc in source
Files:
crates/biome_package/src/node_js_package/tsconfig_json.rscrates/biome_service/src/file_handlers/html.rscrates/biome_cli/tests/cases/handle_astro_files.rs
**/tests/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place test files under a tests/ directory in each crate
Files:
crates/biome_package/tests/tsconfig/valid/tsconfig.valid.paths.json.snap.newcrates/biome_cli/tests/cases/handle_astro_files.rs
🧬 Code graph analysis (2)
crates/biome_package/src/node_js_package/tsconfig_json.rs (2)
crates/biome_analyze/src/context.rs (1)
new(33-65)crates/biome_package/src/node_js_package/package_json.rs (3)
deserialize(128-168)deserialize(229-235)deserialize(326-336)
crates/biome_cli/tests/cases/handle_astro_files.rs (2)
crates/biome_fs/src/fs/memory.rs (1)
default(37-49)crates/biome_cli/tests/main.rs (1)
run_cli(332-347)
🔇 Additional comments (9)
.changeset/ten-hoops-unite.md (1)
1-5: LGTM!The changeset follows all guidelines: references the issue, uses appropriate tense, and ends with a period.
.changeset/eight-eels-pull.md (1)
1-6: LGTM!The changeset clearly describes the new behavior and follows all guidelines.
crates/biome_service/src/file_handlers/html.rs (1)
491-492: LGTM!The Astro branch follows the established pattern for Svelte and Vue, correctly creating a TypeScript file source with Astro embedding kind.
crates/biome_cli/tests/cases/handle_astro_files.rs (2)
59-72: LGTM!The test constant appropriately represents an Astro file with TypeScript type annotations in a script tag.
638-669: LGTM!The test follows the established pattern and properly validates TypeScript script tag formatting in Astro files.
crates/biome_package/tests/tsconfig/valid/tsconfig.valid.paths.json.snap.new (1)
1-45: LGTM!The snapshot correctly validates the TsConfigJson structure with the new optional JSX factory fields.
crates/biome_package/src/node_js_package/tsconfig_json.rs (3)
120-128: LGTM!The accessors cleanly expose the normalized JSX factory identifiers.
151-163: LGTM!The new fields are well-documented and use the JsxFactoryIdentifier type for automatic normalization.
193-231: LGTM!The implementation correctly normalizes during deserialization (addressing the previous review concern), making the operation efficient. The Deref implementation provides ergonomic access.
Based on learnings
db4c897 to
73a45a9
Compare
|
@ematipico should I rebase on "next" as well? Too many extra changes showed up on "main" based branch |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
crates/biome_package/src/node_js_package/tsconfig_json.rs (1)
193-205: Bold move normalising at deserialisation — this addresses the earlier concern about repeated string work and the added tests are spot on. LGTM.
🧹 Nitpick comments (6)
crates/biome_package/src/node_js_package/tsconfig_json.rs (2)
216-231: Trim whitespace during deserialisation; simplify split handling.
Prevents “ React.createElement ”-style values from leaking spaces and removes the redundant unwrap_or.Apply this diff:
- let full_name = String::deserialize(ctx, value, name)?; - // Extract the base identifier (everything before the first dot) - let base_identifier = full_name - .split('.') - .next() - .unwrap_or(&full_name) - .to_string(); + let full_name = String::deserialize(ctx, value, name)?; + let trimmed = full_name.trim(); + // Extract the base identifier (everything before the first dot) + let base_identifier = trimmed + .split('.') + .next() + .unwrap_or("") + .to_string(); Some(Self(base_identifier))
233-366: Add tests for whitespace and empty-string inputs.
Covers edge cases and guards regressions.Apply this diff:
@@ fn test_jsx_factory_efficiency() { @@ } + + #[test] + fn test_jsx_factory_whitespace_and_empty() { + let json = r#"{ + "compilerOptions": { + "jsxFactory": " React.createElement ", + "jsxFragmentFactory": " Fragment " + } + }"#; + let (tsconfig, _) = TsConfigJson::parse(Utf8Path::new("/test/tsconfig.json"), json); + assert_eq!(tsconfig.jsx_factory_identifier(), Some("React")); + assert_eq!(tsconfig.jsx_fragment_factory_identifier(), Some("Fragment")); + + let json_empty = r#"{ + "compilerOptions": { + "jsxFactory": "", + "jsxFragmentFactory": "" + } + }"#; + let (tsconfig_empty, _) = + TsConfigJson::parse(Utf8Path::new("/test/tsconfig.json"), json_empty); + assert_eq!(tsconfig_empty.jsx_factory_identifier(), None); + assert_eq!(tsconfig_empty.jsx_fragment_factory_identifier(), None); + }.changeset/eight-eels-pull.md (1)
1-6: Add issue links and a rule link per changeset guidelines.
Start with “Fixed #...”, and reference the rule page.Apply this diff:
--- "@biomejs/backend-jsonrpc": patch "@biomejs/biome": patch --- -Biome now respects jsxFactory and jsxFragmentFactory settings from tsconfig.json when using the classic JSX runtime, preventing false positive noUnusedImports errors for custom JSX libraries like Preact. +Fixed #6438 and #3682: Biome respects jsxFactory and jsxFragmentFactory from tsconfig.json when using the classic JSX runtime. +This prevents false-positive correctness/noUnusedImports diagnostics for custom JSX libraries (e.g., Preact). See https://biomejs.dev/lint/rules/no-unused-imports/.As per coding guidelines.
crates/biome_js_analyze/tests/quick_test.rs (1)
99-156: Solid targeted test; exercises ReactClassic with custom factory/fragment and no diagnostics.Consider an extra case using a namespace/default import (e.g., React + React.Fragment) to cover the classic React path too.
crates/biome_js_analyze/src/lint/style/use_import_type.rs (1)
1100-1129: Factor the helper into crate::react and update the rule docs.
Multiple rules now need this predicate; moving it avoids drift and makes testing easier. Also amend the rule’s docstring to note exceptions for custom jsxFactory/jsxFragmentFactory, not only React globals.Happy to extract a shared helper and update docs in a follow-up PR.
crates/biome_analyze/src/context.rs (1)
164-172: Accessors correctly implemented.The methods properly expose the optional JSX factory identifiers. Documentation is adequate, though you might consider elaborating slightly on when these are
None(e.g., "ReturnsNoneunless custom factories are configured in tsconfig.json") to help rule implementers.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
crates/biome_js_analyze/tests/specs/correctness/noUnusedImports/jsxFactoryPreact.js.snapis excluded by!**/*.snapand included by**crates/biome_package/tests/tsconfig/valid/tsconfig.valid.baseUrl.json.snapis excluded by!**/*.snapand included by**crates/biome_package/tests/tsconfig/valid/tsconfig.valid.paths.json.snapis excluded by!**/*.snapand included by**packages/@biomejs/backend-jsonrpc/src/workspace.tsis excluded by!**/backend-jsonrpc/src/workspace.tsand included by**packages/@biomejs/biome/configuration_schema.jsonis excluded by!**/configuration_schema.jsonand included by**
📒 Files selected for processing (18)
.changeset/eight-eels-pull.md(1 hunks)crates/biome_analyze/src/context.rs(4 hunks)crates/biome_analyze/src/options.rs(3 hunks)crates/biome_analyze/src/registry.rs(2 hunks)crates/biome_analyze/src/signals.rs(3 hunks)crates/biome_js_analyze/src/lint/correctness/no_unused_imports.rs(2 hunks)crates/biome_js_analyze/src/lint/style/use_import_type.rs(6 hunks)crates/biome_js_analyze/src/react.rs(1 hunks)crates/biome_js_analyze/tests/quick_test.rs(1 hunks)crates/biome_js_analyze/tests/spec_tests.rs(1 hunks)crates/biome_js_analyze/tests/specs/correctness/noUnusedImports/jsxFactoryPreact.js(1 hunks)crates/biome_js_analyze/tests/specs/correctness/noUnusedImports/jsxFactoryPreact.options.json(1 hunks)crates/biome_js_analyze/tests/specs/correctness/noUnusedImports/jsxFactoryPreact.tsconfig.json(1 hunks)crates/biome_package/src/node_js_package/tsconfig_json.rs(3 hunks)crates/biome_package/tests/tsconfig/valid/tsconfig.valid.baseUrl.json.snap.new(1 hunks)crates/biome_package/tests/tsconfig/valid/tsconfig.valid.paths.json.snap.new(1 hunks)crates/biome_service/src/file_handlers/mod.rs(1 hunks)crates/biome_service/src/settings.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (11)
- crates/biome_package/tests/tsconfig/valid/tsconfig.valid.baseUrl.json.snap.new
- crates/biome_js_analyze/src/react.rs
- crates/biome_js_analyze/tests/specs/correctness/noUnusedImports/jsxFactoryPreact.tsconfig.json
- crates/biome_service/src/settings.rs
- crates/biome_service/src/file_handlers/mod.rs
- crates/biome_js_analyze/src/lint/correctness/no_unused_imports.rs
- crates/biome_package/tests/tsconfig/valid/tsconfig.valid.paths.json.snap.new
- crates/biome_analyze/src/options.rs
- crates/biome_analyze/src/registry.rs
- crates/biome_js_analyze/tests/spec_tests.rs
- crates/biome_js_analyze/tests/specs/correctness/noUnusedImports/jsxFactoryPreact.js
🧰 Additional context used
📓 Path-based instructions (5)
.changeset/*.md
📄 CodeRabbit inference engine (CONTRIBUTING.md)
.changeset/*.md: In changesets, only use #### or ##### headers; other header levels are not allowed
Changesets should cover user-facing changes only; internal changes do not need changesets
Use past tense for what you did and present tense for current Biome behavior in changesets
When fixing a bug in a changeset, start with an issue link (e.g., “Fixed #1234: …”)
When referencing a rule or assist in a changeset, include a link to its page on the website
Include code blocks in changesets when applicable to illustrate changes
End every sentence in a changeset with a period
Files:
.changeset/eight-eels-pull.md
crates/biome_*_{syntax,parser,formatter,analyze,factory,semantic}/**
📄 CodeRabbit inference engine (CLAUDE.md)
Maintain the per-language crate structure: biome_{lang}_{syntax,parser,formatter,analyze,factory,semantic}
Files:
crates/biome_js_analyze/tests/specs/correctness/noUnusedImports/jsxFactoryPreact.options.jsoncrates/biome_js_analyze/tests/quick_test.rscrates/biome_js_analyze/src/lint/style/use_import_type.rs
crates/biome_*/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place core crates under /crates/biome_*/
Files:
crates/biome_js_analyze/tests/specs/correctness/noUnusedImports/jsxFactoryPreact.options.jsoncrates/biome_js_analyze/tests/quick_test.rscrates/biome_analyze/src/signals.rscrates/biome_package/src/node_js_package/tsconfig_json.rscrates/biome_js_analyze/src/lint/style/use_import_type.rscrates/biome_analyze/src/context.rs
**/tests/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place test files under a tests/ directory in each crate
Files:
crates/biome_js_analyze/tests/specs/correctness/noUnusedImports/jsxFactoryPreact.options.jsoncrates/biome_js_analyze/tests/quick_test.rs
**/*.rs
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.rs: Format Rust files before committing (e.g., viajust fwhich formats Rust)
Document rules, assists, and options with inline rustdoc in source
Files:
crates/biome_js_analyze/tests/quick_test.rscrates/biome_analyze/src/signals.rscrates/biome_package/src/node_js_package/tsconfig_json.rscrates/biome_js_analyze/src/lint/style/use_import_type.rscrates/biome_analyze/src/context.rs
🧠 Learnings (2)
📚 Learning: 2025-10-24T21:24:58.631Z
Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-10-24T21:24:58.631Z
Learning: Applies to crates/biome_analyze/crates/*_analyze/tests/specs/**/*.jsonc : .jsonc snapshot test files must contain an array of code strings and are interpreted as script (no ESM import/export)
Applied to files:
crates/biome_js_analyze/tests/specs/correctness/noUnusedImports/jsxFactoryPreact.options.json
📚 Learning: 2025-10-24T21:24:58.631Z
Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-10-24T21:24:58.631Z
Learning: Applies to crates/biome_analyze/crates/*_analyze/tests/quick_test.rs : Quick test lives in biome_js_analyze/tests/quick_test.rs; unignore #[ignore] and set rule filter and SOURCE for ad-hoc runs
Applied to files:
crates/biome_js_analyze/tests/quick_test.rs
🧬 Code graph analysis (4)
crates/biome_js_analyze/tests/quick_test.rs (3)
crates/biome_js_syntax/src/file_source.rs (1)
jsx(168-170)crates/biome_analyze/src/context.rs (3)
file_path(187-189)new(33-65)options(155-157)crates/biome_js_analyze/src/lib.rs (1)
analyze(188-209)
crates/biome_package/src/node_js_package/tsconfig_json.rs (2)
crates/biome_analyze/src/context.rs (1)
new(33-65)crates/biome_analyze/src/registry.rs (2)
new(298-300)new(382-461)
crates/biome_js_analyze/src/lint/style/use_import_type.rs (4)
crates/biome_js_analyze/src/react.rs (2)
is_global_react_import(305-336)is_jsx_factory_import(341-345)packages/@biomejs/backend-jsonrpc/src/workspace.ts (1)
JsxRuntime(709-709)crates/biome_analyze/src/context.rs (2)
jsx_factory(165-167)jsx_fragment_factory(170-172)crates/biome_analyze/src/options.rs (2)
jsx_factory(179-181)jsx_fragment_factory(191-193)
crates/biome_analyze/src/context.rs (1)
crates/biome_analyze/src/options.rs (2)
jsx_factory(179-181)jsx_fragment_factory(191-193)
🔇 Additional comments (5)
crates/biome_js_analyze/tests/specs/correctness/noUnusedImports/jsxFactoryPreact.options.json (1)
1-6: Looks good. Correct runtime for the spec.crates/biome_js_analyze/src/lint/style/use_import_type.rs (1)
195-199: Nice consolidation via is_jsx_factory_binding — fewer branches, clearer intent, same behaviour under ReactClassic.Also applies to: 251-253, 277-279, 343-345
crates/biome_analyze/src/signals.rs (1)
372-376: All RuleContext::new call sites correctly updated.Verification confirms all four RuleContext::new instantiations (three in signals.rs, one in registry.rs) now properly pass jsx_factory and jsx_fragment_factory with consistent parameter ordering.
The optional refactor suggestion to extract a helper remains valid for reducing duplication, but the current changes are correct and complete.
crates/biome_analyze/src/context.rs (2)
23-24: LGTM – JSX factory fields correctly added.The fields use the appropriate lifetime
'aand are sensibly positioned alongside other JSX configuration.
33-65: Constructor properly extended.The new parameters are correctly threaded through to field initialization, maintaining consistency with the existing parameter-heavy signature.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
crates/biome_js_analyze/tests/specs/correctness/noUnusedImports/jsxFactoryPreact.js.snapis excluded by!**/*.snapand included by**crates/biome_package/tests/tsconfig/valid/tsconfig.valid.baseUrl.json.snapis excluded by!**/*.snapand included by**crates/biome_package/tests/tsconfig/valid/tsconfig.valid.paths.json.snapis excluded by!**/*.snapand included by**packages/@biomejs/backend-jsonrpc/src/workspace.tsis excluded by!**/backend-jsonrpc/src/workspace.tsand included by**packages/@biomejs/biome/configuration_schema.jsonis excluded by!**/configuration_schema.jsonand included by**
📒 Files selected for processing (18)
.changeset/eight-eels-pull.md(1 hunks)crates/biome_analyze/src/context.rs(4 hunks)crates/biome_analyze/src/options.rs(3 hunks)crates/biome_analyze/src/registry.rs(2 hunks)crates/biome_analyze/src/signals.rs(3 hunks)crates/biome_js_analyze/src/lint/correctness/no_unused_imports.rs(2 hunks)crates/biome_js_analyze/src/lint/style/use_import_type.rs(6 hunks)crates/biome_js_analyze/src/react.rs(1 hunks)crates/biome_js_analyze/tests/quick_test.rs(1 hunks)crates/biome_js_analyze/tests/spec_tests.rs(1 hunks)crates/biome_js_analyze/tests/specs/correctness/noUnusedImports/jsxFactoryPreact.js(1 hunks)crates/biome_js_analyze/tests/specs/correctness/noUnusedImports/jsxFactoryPreact.options.json(1 hunks)crates/biome_js_analyze/tests/specs/correctness/noUnusedImports/jsxFactoryPreact.tsconfig.json(1 hunks)crates/biome_package/src/node_js_package/tsconfig_json.rs(3 hunks)crates/biome_package/tests/tsconfig/valid/tsconfig.valid.baseUrl.json.snap.new(1 hunks)crates/biome_package/tests/tsconfig/valid/tsconfig.valid.paths.json.snap.new(1 hunks)crates/biome_service/src/file_handlers/mod.rs(1 hunks)crates/biome_service/src/settings.rs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- crates/biome_package/tests/tsconfig/valid/tsconfig.valid.baseUrl.json.snap.new
🚧 Files skipped from review as they are similar to previous changes (11)
- crates/biome_js_analyze/tests/specs/correctness/noUnusedImports/jsxFactoryPreact.options.json
- crates/biome_service/src/file_handlers/mod.rs
- crates/biome_service/src/settings.rs
- crates/biome_js_analyze/src/react.rs
- crates/biome_js_analyze/tests/quick_test.rs
- crates/biome_analyze/src/signals.rs
- crates/biome_analyze/src/options.rs
- crates/biome_js_analyze/src/lint/correctness/no_unused_imports.rs
- crates/biome_js_analyze/tests/spec_tests.rs
- crates/biome_analyze/src/registry.rs
- crates/biome_js_analyze/tests/specs/correctness/noUnusedImports/jsxFactoryPreact.js
🧰 Additional context used
📓 Path-based instructions (5)
.changeset/*.md
📄 CodeRabbit inference engine (CONTRIBUTING.md)
.changeset/*.md: In changesets, only use #### or ##### headers; other header levels are not allowed
Changesets should cover user-facing changes only; internal changes do not need changesets
Use past tense for what you did and present tense for current Biome behavior in changesets
When fixing a bug in a changeset, start with an issue link (e.g., “Fixed #1234: …”)
When referencing a rule or assist in a changeset, include a link to its page on the website
Include code blocks in changesets when applicable to illustrate changes
End every sentence in a changeset with a period
Files:
.changeset/eight-eels-pull.md
crates/biome_*_{syntax,parser,formatter,analyze,factory,semantic}/**
📄 CodeRabbit inference engine (CLAUDE.md)
Maintain the per-language crate structure: biome_{lang}_{syntax,parser,formatter,analyze,factory,semantic}
Files:
crates/biome_js_analyze/tests/specs/correctness/noUnusedImports/jsxFactoryPreact.tsconfig.jsoncrates/biome_js_analyze/src/lint/style/use_import_type.rs
crates/biome_*/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place core crates under /crates/biome_*/
Files:
crates/biome_js_analyze/tests/specs/correctness/noUnusedImports/jsxFactoryPreact.tsconfig.jsoncrates/biome_package/src/node_js_package/tsconfig_json.rscrates/biome_js_analyze/src/lint/style/use_import_type.rscrates/biome_package/tests/tsconfig/valid/tsconfig.valid.paths.json.snap.newcrates/biome_analyze/src/context.rs
**/tests/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place test files under a tests/ directory in each crate
Files:
crates/biome_js_analyze/tests/specs/correctness/noUnusedImports/jsxFactoryPreact.tsconfig.jsoncrates/biome_package/tests/tsconfig/valid/tsconfig.valid.paths.json.snap.new
**/*.rs
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.rs: Format Rust files before committing (e.g., viajust fwhich formats Rust)
Document rules, assists, and options with inline rustdoc in source
Files:
crates/biome_package/src/node_js_package/tsconfig_json.rscrates/biome_js_analyze/src/lint/style/use_import_type.rscrates/biome_analyze/src/context.rs
🧬 Code graph analysis (3)
crates/biome_package/src/node_js_package/tsconfig_json.rs (1)
crates/biome_package/src/node_js_package/package_json.rs (3)
deserialize(128-168)deserialize(229-235)deserialize(326-336)
crates/biome_js_analyze/src/lint/style/use_import_type.rs (4)
crates/biome_js_analyze/src/react.rs (2)
is_global_react_import(305-336)is_jsx_factory_import(341-345)packages/@biomejs/backend-jsonrpc/src/workspace.ts (1)
JsxRuntime(709-709)crates/biome_analyze/src/context.rs (2)
jsx_factory(165-167)jsx_fragment_factory(170-172)crates/biome_analyze/src/options.rs (2)
jsx_factory(179-181)jsx_fragment_factory(191-193)
crates/biome_analyze/src/context.rs (1)
crates/biome_analyze/src/options.rs (2)
jsx_factory(179-181)jsx_fragment_factory(191-193)
🔇 Additional comments (12)
crates/biome_js_analyze/tests/specs/correctness/noUnusedImports/jsxFactoryPreact.tsconfig.json (1)
1-8: LGTM!Valid tsconfig test fixture for Preact-style JSX configuration.
crates/biome_analyze/src/context.rs (3)
23-24: LGTM!Fields are appropriately typed and follow the existing pattern.
44-45: LGTM!Constructor correctly accepts and assigns the new parameters.
Also applies to: 61-62
164-172: LGTM!Accessors are well-documented and follow established conventions.
crates/biome_js_analyze/src/lint/style/use_import_type.rs (2)
1100-1129: LGTM!Helper function consolidates JSX factory detection logic cleanly. The early return for non-ReactClassic runtime is efficient, and the three-check pattern (standard React, custom factory, custom fragment) is clear and correct.
195-195: LGTM!Consistent use of the helper across all relevant call sites eliminates duplication and improves maintainability.
Also applies to: 251-251, 277-277, 343-343
crates/biome_package/src/node_js_package/tsconfig_json.rs (5)
120-128: LGTM!The accessors are correctly implemented. Empty identifier filtering is handled during deserialization (lines 230-234), which is more efficient than filtering on every accessor call.
151-163: LGTM!Fields are well-documented with clear explanation of normalization behaviour. The rename attributes correctly handle TypeScript's camelCase convention.
193-238: LGTM!The deserialization logic correctly extracts the base identifier, trims whitespace, and filters out empty/unusable values. Normalising during deserialization is efficient, and the
Derefimplementation provides ergonomic access.
244-372: LGTM!Comprehensive test coverage for basic functionality including normalisation, simple identifiers, namespaced identifiers, missing fields, deeply nested paths, and efficiency verification.
374-468: LGTM!Excellent edge case coverage including empty strings, whitespace-only, dot-only, and surrounding whitespace scenarios. These tests verify the filtering behaviour that prevents "configured but unusable" states.
crates/biome_package/tests/tsconfig/valid/tsconfig.valid.paths.json.snap.new (1)
1-45: LGTM!Snapshot correctly documents the new
jsx_factoryandjsx_fragment_factoryfields in theCompilerOptionsstructure. TheNonevalues are expected since the test input doesn't configure these options.
567a82d to
8c256e5
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
crates/biome_package/src/node_js_package/tsconfig_json.rs (1)
120-128: Empty identifier filtering is correctly handled upstream.The previous review comment suggested filtering empty strings here, but this is now handled more efficiently during deserialization (lines 228-230), where
JsxFactoryIdentifier::deserializereturnsNonefor empty or whitespace-only values. This ensures the accessors never returnSome(""), making additional checks here unnecessary.
🧹 Nitpick comments (2)
crates/biome_js_analyze/src/lint/style/use_import_type.rs (1)
1100-1129: Nice refactoring!Extracting the repeated JSX factory detection logic into a helper function is a good application of DRY principles. The function is well-structured and clearly documents its purpose.
Minor suggestion: Consider adding a rustdoc comment to explain when this function returns true, e.g.:
/// Returns `true` if the binding is a JSX factory or fragment factory import /// when using the ReactClassic runtime. This includes: /// - Standard React imports (e.g., `import React from 'react'`) /// - Custom JSX factory imports matching `ctx.jsx_factory()` /// - Custom JSX fragment factory imports matching `ctx.jsx_fragment_factory()` fn is_jsx_factory_binding(crates/biome_package/src/node_js_package/tsconfig_json.rs (1)
216-234: Minor: Unreachableunwrap_oron line 224.The
unwrap_or(&full_name)is unnecessary sincesplit('.').next()always returnsSomefor any string input (even empty strings yieldSome("")). The fallback will never be used.Apply this diff to simplify:
- let base_identifier = full_name.split('.').next().unwrap_or(&full_name).trim(); + let base_identifier = full_name.split('.').next().unwrap().trim();Otherwise, the deserialization logic is sound—it correctly handles all edge cases (empty, whitespace, dots, nested identifiers) and normalises once for efficiency.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
crates/biome_js_analyze/tests/specs/correctness/noUnusedImports/jsxFactoryPreact.js.snapis excluded by!**/*.snapand included by**crates/biome_package/tests/tsconfig/valid/tsconfig.valid.baseUrl.json.snapis excluded by!**/*.snapand included by**crates/biome_package/tests/tsconfig/valid/tsconfig.valid.paths.json.snapis excluded by!**/*.snapand included by**
📒 Files selected for processing (18)
.changeset/eight-eels-pull.md(1 hunks)crates/biome_analyze/src/context.rs(4 hunks)crates/biome_analyze/src/options.rs(3 hunks)crates/biome_analyze/src/registry.rs(2 hunks)crates/biome_analyze/src/signals.rs(3 hunks)crates/biome_js_analyze/src/lint/correctness/no_unused_imports.rs(2 hunks)crates/biome_js_analyze/src/lint/style/use_import_type.rs(6 hunks)crates/biome_js_analyze/src/react.rs(1 hunks)crates/biome_js_analyze/tests/quick_test.rs(1 hunks)crates/biome_js_analyze/tests/spec_tests.rs(1 hunks)crates/biome_js_analyze/tests/specs/correctness/noUnusedImports/jsxFactoryPreact.js(1 hunks)crates/biome_js_analyze/tests/specs/correctness/noUnusedImports/jsxFactoryPreact.options.json(1 hunks)crates/biome_js_analyze/tests/specs/correctness/noUnusedImports/jsxFactoryPreact.tsconfig.json(1 hunks)crates/biome_package/src/node_js_package/tsconfig_json.rs(3 hunks)crates/biome_package/tests/tsconfig/valid/tsconfig.valid.baseUrl.json.snap.new(1 hunks)crates/biome_package/tests/tsconfig/valid/tsconfig.valid.paths.json.snap.new(1 hunks)crates/biome_service/src/file_handlers/mod.rs(1 hunks)crates/biome_service/src/settings.rs(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- crates/biome_package/tests/tsconfig/valid/tsconfig.valid.baseUrl.json.snap.new
- crates/biome_package/tests/tsconfig/valid/tsconfig.valid.paths.json.snap.new
🚧 Files skipped from review as they are similar to previous changes (8)
- crates/biome_js_analyze/tests/spec_tests.rs
- crates/biome_service/src/file_handlers/mod.rs
- crates/biome_analyze/src/registry.rs
- crates/biome_analyze/src/context.rs
- crates/biome_js_analyze/tests/quick_test.rs
- crates/biome_js_analyze/tests/specs/correctness/noUnusedImports/jsxFactoryPreact.js
- crates/biome_js_analyze/tests/specs/correctness/noUnusedImports/jsxFactoryPreact.options.json
- crates/biome_analyze/src/options.rs
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{rs,toml}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Format Rust and TOML files before committing (e.g., via
just f)
Files:
crates/biome_js_analyze/src/react.rscrates/biome_js_analyze/src/lint/correctness/no_unused_imports.rscrates/biome_service/src/settings.rscrates/biome_js_analyze/src/lint/style/use_import_type.rscrates/biome_package/src/node_js_package/tsconfig_json.rscrates/biome_analyze/src/signals.rs
**/*.rs
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Document rules, assists, and their options with inline rustdoc in the Rust source
Files:
crates/biome_js_analyze/src/react.rscrates/biome_js_analyze/src/lint/correctness/no_unused_imports.rscrates/biome_service/src/settings.rscrates/biome_js_analyze/src/lint/style/use_import_type.rscrates/biome_package/src/node_js_package/tsconfig_json.rscrates/biome_analyze/src/signals.rs
.changeset/*.md
📄 CodeRabbit inference engine (CONTRIBUTING.md)
.changeset/*.md: In changeset files, only use #### or ##### headers
Changesets should describe user-facing changes; internal-only changes do not need changesets
Use past tense for what you did in the changeset description and present tense for current behavior
For bug fixes, start the changeset description with a link to the issue (e.g., Fixed #1234: ...)
When referencing a rule or assist in a changeset, include a link to the rule/assist page on the website
Include a code block in the changeset when applicable to illustrate the change
End every sentence in a changeset with a full stop (.)
Files:
.changeset/eight-eels-pull.md
🧠 Learnings (1)
📚 Learning: 2025-10-24T21:24:58.631Z
Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-10-24T21:24:58.631Z
Learning: Applies to crates/biome_analyze/crates/*_analyze/**/src/**/lint/**/*.rs : Set the language field in declare_lint_rule! to the most appropriate dialect (js/jsx/ts/tsx)
Applied to files:
crates/biome_service/src/settings.rs
🧬 Code graph analysis (5)
crates/biome_js_analyze/src/react.rs (1)
crates/biome_js_analyze/src/react/components.rs (3)
name(428-435)name(465-489)name(531-546)
crates/biome_js_analyze/src/lint/correctness/no_unused_imports.rs (2)
crates/biome_js_analyze/src/react.rs (2)
is_global_react_import(305-336)is_jsx_factory_import(341-345)crates/biome_analyze/src/context.rs (2)
jsx_factory(165-167)jsx_fragment_factory(170-172)
crates/biome_service/src/settings.rs (2)
crates/biome_analyze/src/context.rs (1)
jsx_runtime(160-162)crates/biome_analyze/src/options.rs (1)
jsx_runtime(175-177)
crates/biome_js_analyze/src/lint/style/use_import_type.rs (4)
crates/biome_js_analyze/src/react.rs (2)
is_global_react_import(305-336)is_jsx_factory_import(341-345)packages/@biomejs/backend-jsonrpc/src/workspace.ts (1)
JsxRuntime(701-701)crates/biome_analyze/src/context.rs (2)
jsx_factory(165-167)jsx_fragment_factory(170-172)crates/biome_analyze/src/options.rs (2)
jsx_factory(179-181)jsx_fragment_factory(191-193)
crates/biome_package/src/node_js_package/tsconfig_json.rs (2)
crates/biome_analyze/src/context.rs (1)
new(33-65)crates/biome_analyze/src/registry.rs (2)
new(298-300)new(382-461)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Check Dependencies
- GitHub Check: Bench (biome_js_formatter)
- GitHub Check: Bench (biome_js_parser)
- GitHub Check: Bench (biome_js_analyze)
- GitHub Check: autofix
🔇 Additional comments (14)
crates/biome_js_analyze/tests/specs/correctness/noUnusedImports/jsxFactoryPreact.tsconfig.json (1)
1-7: Test fixture looks good.The tsconfig.json is correctly configured for testing the Preact scenario with custom
jsxFactory("h") andjsxFragmentFactory("Fragment"). This aligns perfectly with the PR objectives for supporting custom JSX factories..changeset/eight-eels-pull.md (1)
6-6: LGTM!The changeset follows coding guidelines: includes issue links, rule documentation link, and proper formatting. Well done addressing the previous feedback.
crates/biome_js_analyze/src/react.rs (1)
338-345: Verify if name-only matching is sufficient for JSX factory detection.Unlike
is_global_react_import(lines 304-336), which validates the import source and declaration type, this function only checks the binding name. This could incorrectly treat a local variable namedhas a JSX factory import.Consider whether this is intentional (e.g., trusting tsconfig configuration) or if additional validation is needed to confirm the binding is actually an import.
crates/biome_service/src/settings.rs (1)
500-502: LGTM!Good change to preserve other environment fields rather than replacing the entire environment object. This allows jsx_factory and jsx_fragment_factory to coexist with jsx_runtime.
crates/biome_js_analyze/src/lint/correctness/no_unused_imports.rs (1)
626-642: LGTM!The logic clearly exempts custom JSX factory imports when using the classic runtime. The three-part check (standard React, jsx_factory, jsx_fragment_factory) is straightforward and correct.
crates/biome_analyze/src/signals.rs (3)
362-377: LGTM!Consistent threading of jsx_factory and jsx_fragment_factory parameters through the diagnostic context.
401-416: LGTM!Consistent threading of jsx_factory and jsx_fragment_factory parameters through the action context.
468-483: LGTM!Consistent threading of jsx_factory and jsx_fragment_factory parameters through the transformation context.
crates/biome_js_analyze/src/lint/style/use_import_type.rs (4)
195-199: LGTM!Good refactoring to use the centralised helper function.
251-253: LGTM!Consistent use of the helper function for namespace imports.
277-279: LGTM!Consistent use of the helper function for default imports.
343-345: LGTM!Consistent use of the helper function for namespace clause imports.
crates/biome_package/src/node_js_package/tsconfig_json.rs (2)
151-163: LGTM!The field declarations are properly documented with clear examples of the normalization behaviour, and the
#[deserializable(rename = ...)]attributes correctly map to TypeScript's camelCase naming.
236-465: Excellent test coverage!The test suite comprehensively covers normalization behaviour (simple, namespaced, deeply nested) and all relevant edge cases (empty strings, whitespace, dots). The efficiency test (lines 352-368) confirms that normalization happens during deserialization rather than on every access, addressing the reviewer's earlier concern about performance.
Co-authored-by: Naoki Ikeguchi <[email protected]> Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> Co-authored-by: Emanuele Stoppa <[email protected]>
Closes #6438
Closes #3682
Summary
Problem: When using alternative JSX libraries (Preact, Stencil, or custom JSX implementations) with the classic JSX runtime, Biome incorrectly reports custom JSX factory functions as unused imports.
Root cause: Biome only recognized the hardcoded identifiers when jsxRuntime is set to "reactClassic". It didn't read or respect the jsxFactory and jsxFragmentFactory settings from tsconfig.json.
Changeset
Added support for custom JSX factory functions when using the classic React runtime.
Test Plan
Tests added into "biome/crates/biome_js_analyze/tests/specs/correctness/noUnusedImports"