fix(lsp): filter organizeImports from fixAll code action#9487
fix(lsp): filter organizeImports from fixAll code action#9487ematipico merged 3 commits intobiomejs:mainfrom
Conversation
When `source.fixAll.biome` is triggered but `source.organizeImports.biome` is not explicitly requested, the fix-all action now skips the organize imports rule. This prevents import sorting on save when the user has set `source.organizeImports.biome` to "explicit" or "never" in their editor settings. Fixes biomejs#9477 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: 48e5175 The changes in this PR will be included in the next version bump. This PR includes changesets to release 13 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughAdds an Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip CodeRabbit can scan for known vulnerabilities in your dependencies using OSV Scanner.OSV Scanner will automatically detect and report security vulnerabilities in your project's dependencies. No additional configuration is required. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/biome_lsp/src/handlers/analysis.rs (1)
33-34: Derive the skip selector fromORGANIZE_IMPORTS_CATEGORY.Right now the kind lives as
ORGANIZE_IMPORTS_CATEGORY, but the skip path re-spells it as"source"/"organizeImports". If those drift, the bug saunters back in. ReusingRuleSelector::from_lsp_filter(...)keeps both paths in lock-step.♻️ Small tidy-up
let mut skip = vec![]; if !include_organize_imports { - skip.push(AnalyzerSelector::Rule(RuleSelector::Rule( - "source", - "organizeImports", - ))); + if let Some(selector) = + RuleSelector::from_lsp_filter(ORGANIZE_IMPORTS_CATEGORY.to_str().as_ref()) + { + skip.push(AnalyzerSelector::Rule(selector)); + } }Also applies to: 302-316, 379-385
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_lsp/src/handlers/analysis.rs` around lines 33 - 34, Replace the hard-coded skip selector strings ("source"/"organizeImports") by deriving the selector from the existing ORGANIZE_IMPORTS_CATEGORY constant using RuleSelector::from_lsp_filter(...); locate the places that construct the skip path (the code that currently spells out "source" / "organizeImports", including the occurrences around the earlier constant and the other two spots noted) and call RuleSelector::from_lsp_filter(ORGANIZE_IMPORTS_CATEGORY.clone().into() / or pass ORGANIZE_IMPORTS_CATEGORY as required by the API) to produce the selector, then use that RuleSelector instead of the literal strings so both paths stay in sync with ORGANIZE_IMPORTS_CATEGORY.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@crates/biome_lsp/src/handlers/analysis.rs`:
- Around line 33-34: Replace the hard-coded skip selector strings
("source"/"organizeImports") by deriving the selector from the existing
ORGANIZE_IMPORTS_CATEGORY constant using RuleSelector::from_lsp_filter(...);
locate the places that construct the skip path (the code that currently spells
out "source" / "organizeImports", including the occurrences around the earlier
constant and the other two spots noted) and call
RuleSelector::from_lsp_filter(ORGANIZE_IMPORTS_CATEGORY.clone().into() / or pass
ORGANIZE_IMPORTS_CATEGORY as required by the API) to produce the selector, then
use that RuleSelector instead of the literal strings so both paths stay in sync
with ORGANIZE_IMPORTS_CATEGORY.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: baaff266-ffc8-4760-bda9-1f3c6d22d8ab
📒 Files selected for processing (1)
crates/biome_lsp/src/handlers/analysis.rs
ematipico
left a comment
ematipico
left a comment
There was a problem hiding this comment.
Can you please add a test or update the existing ones?
.changeset/quiet-foxes-camp.md
Outdated
| "@biomejs/biome": patch | ||
| --- | ||
|
|
||
| Fixed [#9477](https://github.com/biomejs/biome/issues/9477): `source.fixAll.biome` no longer sorts imports when `source.organizeImports.biome` is set to "explicit" or "never" in editor settings. The organize imports action is now excluded from the fix-all pass unless explicitly requested. |
There was a problem hiding this comment.
| Fixed [#9477](https://github.com/biomejs/biome/issues/9477): `source.fixAll.biome` no longer sorts imports when `source.organizeImports.biome` is set to "explicit" or "never" in editor settings. The organize imports action is now excluded from the fix-all pass unless explicitly requested. | |
| Fixed [#9477](https://github.com/biomejs/biome/issues/9477): `source.fixAll.biome` no longer sorts imports when `source.organizeImports.biome` is disabled in editor settings. The organize imports action is now excluded from the fix-all pass unless explicitly requested. |
Different editor settings have different values. e.g. zed uses false
Add test that confirms source.fixAll.biome does not reorder imports when source.organizeImports is not in the code action filter. Also update changeset wording per reviewer feedback.
|
Added test in 48e5175: Also updated the changeset wording per your inline suggestion. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/biome_lsp/src/server.tests.rs (1)
2641-2649: Avoid coupling this check toedits[0]being a full-document rewrite.Line 2646 assumes the first edit contains the whole file text. If fix-all later emits targeted edits, this test can fail even when behaviour is correct.
💡 Suggested tightening
let edits = changes .get(&uri!("document.js")) .context("expected edits for document.js")?; - let new_text = &edits[0].new_text; - assert!( - new_text.starts_with("import { b }"), - "imports should NOT be reordered when organizeImports is not requested, got: {new_text}" - ); + assert!(!edits.is_empty(), "expected at least one fix-all edit"); + + let full_document_edit = edits.iter().find(|edit| { + edit.range.start == Position::new(0, 0) && edit.range.end.line >= 2 + }); + let new_text = full_document_edit + .map(|edit| edit.new_text.as_str()) + .context("expected a full-document edit for stable content assertions")?; + + assert!( + new_text.starts_with("import { b }"), + "imports should NOT be reordered when organizeImports is not requested, got: {new_text}" + ); + assert!( + new_text.contains("=== 0"), + "fix-all should still apply the lint fix, got: {new_text}" + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_lsp/src/server.tests.rs` around lines 2641 - 2649, The test currently assumes edits[0] is a full-document rewrite; instead scan all edits for one that represents the full-document replacement or contains the expected import prefix. Replace the direct index access (edits[0].new_text) with a check like edits.iter().any(|te| te.new_text.starts_with("import { b }")) so the assertion succeeds if any emitted edit is the full-document rewrite (or contains the expected import) and doesn't fail when targeted edits are produced; reference the existing variables action.edit, changes, edits and uri!("document.js") to locate the edit vector to change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@crates/biome_lsp/src/server.tests.rs`:
- Around line 2641-2649: The test currently assumes edits[0] is a full-document
rewrite; instead scan all edits for one that represents the full-document
replacement or contains the expected import prefix. Replace the direct index
access (edits[0].new_text) with a check like edits.iter().any(|te|
te.new_text.starts_with("import { b }")) so the assertion succeeds if any
emitted edit is the full-document rewrite (or contains the expected import) and
doesn't fail when targeted edits are produced; reference the existing variables
action.edit, changes, edits and uri!("document.js") to locate the edit vector to
change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cef711c1-5596-4edc-aff2-b3a2b8cac90e
📒 Files selected for processing (2)
.changeset/quiet-foxes-camp.mdcrates/biome_lsp/src/server.tests.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- .changeset/quiet-foxes-camp.md
|
Thanks for the merge! Glad this cleans up the fixAll behavior. |
2 participants
Summary
When
source.fixAll.biomeruns (e.g. on save), it includes organize imports actions even whensource.organizeImports.biomeis set to "explicit" or "never" in editor settings. This PR filters out the organize imports rule from the fix-all pass when organize imports was not explicitly requested.The fix adds an
include_organize_importsparameter to thefix_all()function. Whensource.organizeImports.biomeis not in the editor's requested code actions, the organize imports rule is added to theskiplist inFixFileParams, preventing it from running as part of fix-all.Fixes #9477
Test Plan
cargo check -p biome_lsppassesbiome_lsptests pass (cargo test -p biome_lsp)AI Assistance
This PR was written with AI assistance (Claude Code).