-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
fix(lsp): prefer most specific project root #9441
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
ematipico
merged 5 commits into
biomejs:main
from
soconnor-seeq:fix/lsp-most-specific-project-root
Mar 30, 2026
+260
−88
Merged
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
c922650
fix(lsp): prefer most specific project root
soconnor-seeq 2519dc2
addresses coderabbitai comment
soconnor-seeq 7d2ba5f
PR feedback addressed
soconnor-seeq a186da5
More PR feedback addressed
soconnor-seeq 8d66d5c
Calls resolve_configuration_path once in did_open flow
soconnor-seeq File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| "@biomejs/biome": patch | ||
| --- | ||
|
|
||
| Fixed [#1630](https://github.com/biomejs/biome/issues/1630): LSP project selection now prefers the most specific project root in nested workspaces. |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,9 +5,9 @@ use crate::{documents::Document, session::Session}; | |
| use biome_configuration::ConfigurationPathHint; | ||
| use biome_service::workspace::{ | ||
| ChangeFileParams, CloseFileParams, DocumentFileSource, FeaturesBuilder, FileContent, | ||
| GetFileContentParams, IgnoreKind, OpenFileParams, PathIsIgnoredParams, | ||
| GetFileContentParams, IgnoreKind, OpenFileParams, PathIsIgnoredParams, ProjectKey, | ||
| }; | ||
| use camino::Utf8PathBuf; | ||
| use camino::{Utf8Path, Utf8PathBuf}; | ||
| use std::sync::Arc; | ||
| use tower_lsp_server::ls_types as lsp; | ||
| use tracing::{debug, error, field, info, trace}; | ||
|
|
@@ -31,86 +31,13 @@ pub(crate) async fn did_open( | |
| let language_hint = DocumentFileSource::from_language_id(¶ms.text_document.language_id); | ||
|
|
||
| let path = session.file_path(&url)?; | ||
| let file_path = path.to_path_buf(); | ||
| let config_path = session.resolve_configuration_path(Some(&file_path)); | ||
|
|
||
| let project_key = match session.project_for_path(&path) { | ||
| Some(project_key) => project_key, | ||
| None => { | ||
| info!("No open project for path: {path:?}. Opening new project."); | ||
|
|
||
| session.set_configuration_status(ConfigurationStatus::Loading); | ||
| if !session.has_initialized() { | ||
| session.load_extension_settings(None).await; | ||
| } | ||
|
|
||
| let status = if let Some(config_path) = session.resolve_configuration_path(Some(&path)) | ||
| { | ||
| info!( | ||
| "Loading user configuration from text_document {:?}", | ||
| &config_path | ||
| ); | ||
| session | ||
| .load_biome_configuration_file(config_path, false) | ||
| .await | ||
| } else { | ||
| let project_path = path | ||
| .parent() | ||
| .map(|parent| parent.to_path_buf()) | ||
| .unwrap_or_default(); | ||
| info!("Loading configuration from text_document {}", &project_path); | ||
| // First check if the current file belongs to any registered workspace folder. | ||
| // If so, return that folder; otherwise, use the folder computed by did_open. | ||
| let project_path = if let Some(workspace_folders) = session.get_workspace_folders() | ||
| { | ||
| if let Some(ws_root) = workspace_folders | ||
| .iter() | ||
| .filter_map(|folder| { | ||
| folder.uri.to_file_path().map(|p| { | ||
| Utf8PathBuf::from_path_buf(p.to_path_buf()) | ||
| .expect("To have a valid UTF-8 path") | ||
| }) | ||
| }) | ||
| .find(|ws| project_path.starts_with(ws)) | ||
| { | ||
| ws_root | ||
| } else { | ||
| project_path.clone() | ||
| } | ||
| } else if let Some(base_path) = session.base_path() { | ||
| if project_path.starts_with(&base_path) { | ||
| base_path | ||
| } else { | ||
| project_path.clone() | ||
| } | ||
| } else { | ||
| project_path | ||
| }; | ||
|
|
||
| session | ||
| .load_biome_configuration_file( | ||
| ConfigurationPathHint::FromLsp(project_path), | ||
| false, | ||
| ) | ||
| .await | ||
| }; | ||
|
|
||
| session.set_configuration_status(status); | ||
|
|
||
| if status.is_loaded() { | ||
| match session.project_for_path(&path) { | ||
| Some(project_key) => project_key, | ||
|
|
||
| None => { | ||
| error!("Could not find project for {path}"); | ||
|
|
||
| return Ok(()); | ||
| } | ||
| } | ||
| } else { | ||
| error!("Configuration could not be loaded for {path}"); | ||
|
|
||
| return Ok(()); | ||
| } | ||
| } | ||
| let Some(project_key) = | ||
| ensure_project_for_opened_document(session, &path, config_path.as_ref()).await | ||
| else { | ||
| return Ok(()); | ||
| }; | ||
|
|
||
| let is_ignored = session | ||
|
|
@@ -148,6 +75,108 @@ pub(crate) async fn did_open( | |
| Ok(()) | ||
| } | ||
|
|
||
| /// Ensure the file is associated with a project and the correct configuration | ||
| /// is loaded before opening the document. | ||
| async fn ensure_project_for_opened_document( | ||
| session: &Arc<Session>, | ||
| path: &Utf8Path, | ||
| config_path: Option<&ConfigurationPathHint>, | ||
| ) -> Option<ProjectKey> { | ||
| let is_relative_config_path = session | ||
| .get_settings_configuration_path() | ||
| .is_some_and(|config_path| !config_path.is_absolute()); | ||
|
|
||
| let mut load_status = None; | ||
|
|
||
| if let Some(project_key) = session.project_for_path(path) { | ||
| if is_relative_config_path { | ||
| // Absolute configurationPath is global; only relative needs per-file resolution. | ||
| // Use the per-file resolved configurationPath so each workspace folder | ||
| // uses its own config, even when a project is already open. | ||
| if let Some(resolved_path) = config_path { | ||
| session.set_configuration_status(ConfigurationStatus::Loading); | ||
| let status = session | ||
| .load_biome_configuration_file(resolved_path.clone(), false) | ||
| .await; | ||
| load_status = Some(status); | ||
| } | ||
| } | ||
| if load_status.is_none() { | ||
| return Some(project_key); | ||
| } | ||
| } else { | ||
| info!("No open project for path: {path:?}. Opening new project."); | ||
| } | ||
|
|
||
| if load_status.is_none() { | ||
| session.set_configuration_status(ConfigurationStatus::Loading); | ||
| if !session.has_initialized() { | ||
| session.load_extension_settings(None).await; | ||
| } | ||
| load_status = Some(load_from_workspace_root_for_path(session, path, config_path).await); | ||
| } | ||
| let status = load_status.expect("load_status should be set"); | ||
|
|
||
| session.set_configuration_status(status); | ||
|
|
||
| if status.is_loaded() { | ||
| session.project_for_path(path).or_else(|| { | ||
| error!("Could not find project for {path}"); | ||
| None | ||
| }) | ||
| } else { | ||
| error!("Configuration could not be loaded for {path}"); | ||
| None | ||
| } | ||
| } | ||
|
|
||
| /// Load configuration anchored to the workspace root that contains `path`. | ||
| async fn load_from_workspace_root_for_path( | ||
| session: &Arc<Session>, | ||
| path: &Utf8Path, | ||
| config_path: Option<&ConfigurationPathHint>, | ||
| ) -> ConfigurationStatus { | ||
| if let Some(path) = config_path { | ||
| info!("Loading user configuration from text_document {:?}", &path); | ||
| return session | ||
| .load_biome_configuration_file(path.clone(), false) | ||
| .await; | ||
| } | ||
|
|
||
| let project_path = path | ||
| .parent() | ||
| .map(|parent| parent.to_path_buf()) | ||
| .unwrap_or_default(); | ||
| info!("Loading configuration from text_document {}", &project_path); | ||
| let project_path = resolve_workspace_base_path(session, &project_path); | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We now call |
||
|
|
||
| session | ||
| .load_biome_configuration_file(ConfigurationPathHint::FromLsp(project_path), false) | ||
| .await | ||
| } | ||
|
|
||
| fn resolve_workspace_base_path(session: &Session, project_path: &Utf8Path) -> Utf8PathBuf { | ||
| let workspace_base = || { | ||
| let workspace_folders = session.get_workspace_folders()?; | ||
| workspace_folders | ||
| .iter() | ||
| .filter_map(|folder| { | ||
| folder.uri.to_file_path().map(|p| { | ||
| Utf8PathBuf::from_path_buf(p.to_path_buf()).expect("To have a valid UTF-8 path") | ||
| }) | ||
| }) | ||
| .filter(|ws| project_path.starts_with(ws)) | ||
| .max_by_key(|ws| ws.as_str().len()) | ||
| }; | ||
| workspace_base() | ||
| .or_else(|| { | ||
| session | ||
| .base_path() | ||
| .and_then(|base_path| project_path.starts_with(&base_path).then_some(base_path)) | ||
| }) | ||
| .unwrap_or_else(|| project_path.to_path_buf()) | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
| } | ||
|
|
||
| /// Handler for `textDocument/didChange` LSP notification | ||
| #[tracing::instrument(level = "debug", skip_all, fields(url = field::display(¶ms.text_document.uri.as_str()), version = params.text_document.version), err)] | ||
| pub(crate) async fn did_change( | ||
|
|
||
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
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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was moved / split up into into helper functions (
load_project_for_open,load_status_for_open,load_from_workspace_base, etc.). Because the new logic for choosing the most specific project root would have madedid_openeven more nested and harder to reason about. The refactor keeps behaviour clear and testable.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And yet no tests were added ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ha great point. I trusted AI with that summary of the change — apologies. I did get it to try and add tests but I think that code requires too much setup to be tested in more of a unit-test-style. Which means this refactor is mainly done for non-test reasons — just to reduce the nesting / indentation of that function as it was getting big.