fix(lsp): correctly resolve configurationPath#9323
Merged
Conversation
🦋 Changeset detectedLatest commit: 360b02e 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 |
Contributor
WalkthroughThis PR fixes resolution of user-provided Possibly related PRs
Suggested labels
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)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/biome_lsp/src/server.tests.rs (1)
4829-4857: Optional: add the positive control fortest_onein the multi-root test.You already prove
test_twodisables formatting; adding one quick assertion thattest_onestill formats would make this test harder to fool by global-state regressions.🤖 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 4829 - 4857, Add a positive control that verifies formatting is enabled for the other workspace folder by sending a matching "textDocument/formatting" request for "test_one/document.js" (using the same server.request and DocumentFormattingParams pattern and uri!("test_one/document.js")) and assert the returned Option<Vec<TextEdit>> is Some and contains at least one edit; place this before or after the existing test_two assertion to ensure the multi-root test checks both the disabled case (test_two) and an enabled case (test_one) to catch regressions in global state or workspace resolution.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/biome_lsp/src/session.rs`:
- Around line 725-733: The current logic uses workspace_roots.iter().find(...)
which returns the first matching root and can pick a parent when folders are
nested; change this to select the most specific (deepest/longest) matching root
before resolving config_path. In the block that checks file_path and uses
workspace_roots, replace the find(...) call with logic that filters roots where
file_path.starts_with(root) and then chooses the root with the greatest
depth/length (e.g., longest path string or most path components) and then return
Some(selected_root.join(&config_path)); keep the rest of the early-return
behavior the same.
---
Nitpick comments:
In `@crates/biome_lsp/src/server.tests.rs`:
- Around line 4829-4857: Add a positive control that verifies formatting is
enabled for the other workspace folder by sending a matching
"textDocument/formatting" request for "test_one/document.js" (using the same
server.request and DocumentFormattingParams pattern and
uri!("test_one/document.js")) and assert the returned Option<Vec<TextEdit>> is
Some and contains at least one edit; place this before or after the existing
test_two assertion to ensure the multi-root test checks both the disabled case
(test_two) and an enabled case (test_one) to catch regressions in global state
or workspace resolution.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7e1bb0f6-ac3b-4956-ad47-bc141d6200f5
📒 Files selected for processing (5)
.changeset/weak-boxes-look.mdcrates/biome_lsp/src/handlers/text_document.rscrates/biome_lsp/src/server.rscrates/biome_lsp/src/server.tests.rscrates/biome_lsp/src/session.rs
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/biome_lsp/src/session.rs`:
- Around line 693-696: The docs say load_workspace_settings should try each
workspace folder and use the first one that produces an existing config path,
but the current implementation unconditionally picks the first workspace root
(the code around the workspace root selection at lines ~743–745). Update the
resolver logic used by load_workspace_settings (and the workspace-root selection
path) to, when file_path is None, iterate workspace_folders (or the workspace
roots collection) and check for the existence of the config file in each folder,
returning the first folder whose config path exists instead of always choosing
the first workspace root; adjust the code paths referenced by
load_workspace_settings and the workspace root selection to use that
existence-check fallback.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 72e597ea-53e7-4cad-9024-47ad9179c201
📒 Files selected for processing (1)
crates/biome_lsp/src/session.rs
arendjr
approved these changes
Mar 4, 2026
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
crates/biome_lsp/src/session.rs (1)
693-695:⚠️ Potential issue | 🟠 MajorAvoid first-root roulette when
file_pathis absent.Line 693 says each workspace folder is tried, but Line 741 always picks the first root. In multi-root setups, that can resolve to the wrong config and report a false missing/error state.
🔧 Suggested fix
- /// - When `file_path` is `None` (e.g. from `load_workspace_settings`), each - /// workspace folder is tried in order. The closest path to the `file_path` is used. + /// - When `file_path` is `None` (e.g. from `load_workspace_settings`), workspace + /// folders are checked in order and the first folder where the joined path + /// exists is used. @@ - if let Some(root) = workspace_roots.first() { + if let Some(root) = workspace_roots + .iter() + .find(|root| root.join(&config_path).exists()) + { + return Some(root.join(&config_path)); + } + + if let Some(root) = workspace_roots.first() { return Some(root.join(&config_path)); }Also applies to: 741-743
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_lsp/src/session.rs` around lines 693 - 695, The code currently defaults to the first workspace root when file_path is None, causing wrong config selection in multi-root setups; update the workspace selection logic used by load_workspace_settings (and the code that currently falls back to base_path()) so that when file_path is None you do not unconditionally pick the first root but instead try each registered workspace folder in order and return the first folder that yields a valid config (or the closest match if multiple candidates), and only if none produce a config fall back to the session's base_path(); change the selection site that references file_path and base_path() so it iterates and validates roots rather than always returning the first root.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@crates/biome_lsp/src/session.rs`:
- Around line 693-695: The code currently defaults to the first workspace root
when file_path is None, causing wrong config selection in multi-root setups;
update the workspace selection logic used by load_workspace_settings (and the
code that currently falls back to base_path()) so that when file_path is None
you do not unconditionally pick the first root but instead try each registered
workspace folder in order and return the first folder that yields a valid config
(or the closest match if multiple candidates), and only if none produce a config
fall back to the session's base_path(); change the selection site that
references file_path and base_path() so it iterates and validates roots rather
than always returning the first root.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 22d74b48-a677-49c0-9ab3-1ac15a587801
📒 Files selected for processing (1)
crates/biome_lsp/src/session.rs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes biomejs/biome-vscode#959
Closes #9217
I helped myself with an AI agent. I checked all the code and the comments, which make sense.
The issue was a small regression, and Biome didn't make the path absolute when users provided a relative path.
Test Plan
Added two new tests
Docs