fix(lsp): project loading #7418
Conversation
🦋 Changeset detectedLatest commit: b873922 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 |
WalkthroughRefactors LSP project/configuration resolution: did_open now attempts on-demand project detection and lazy configuration loading; Session tracks and can update workspace folders, adds in-flight deduplication for configuration loads via broadcast channels, and propagates reload/force flags through load and scan flows. Several Session method signatures changed to accept reload/force and return owned workspace folder data. ConfigurationPathHint gains Eq/Hash, a #[default] None variant and a to_path_buf helper. Server handlers updated to call load_workspace_settings with explicit flags. Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related PRs
Suggested labels
Suggested reviewers
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ 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). (21)
✨ Finishing Touches
🧪 Generate unit tests
Comment |
github-actions
bot
added
A-Project
ematipico
force-pushed
the
fix/lsp-regression
branch
from
defc911 to
615f23b
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/biome_lsp/src/server.rs (1)
342-368: Support multi-root workspaces in did_change_watched_files.
The current logic usessession.base_path()(backed byinitialize_params.root_uri), so in multi-root setups (where onlyworkspace_foldersis set)base_path()returnsNoneand changes in additional roots are ignored. Simplify by matching on the file’s name only, since the LSP already scopes watched‐file events per folder:- for file_path in file_paths { - let base_path = self.session.base_path(); - if let Some(base_path) = base_path { - let possible_biome_json = file_path.strip_prefix(&base_path); - if let Ok(watched_file) = possible_biome_json - && (ConfigName::file_names() - .iter() - .any(|file_name| watched_file.ends_with(file_name)) - || watched_file.ends_with(".editorconfig")) - { - self.session.load_extension_settings().await; - self.session.load_workspace_settings(true).await; - self.setup_capabilities().await; - self.session.update_all_diagnostics().await; - break; - } - } - } + for file_path in file_paths { + if let Some(name) = file_path.file_name().and_then(|s| s.to_str()) { + if ConfigName::file_names().iter().any(|f| name == *f) + || name == ".editorconfig" + { + self.session.load_extension_settings().await; + self.session.load_workspace_settings(true).await; + self.setup_capabilities().await; + self.session.update_all_diagnostics().await; + break; + } + } + }Please verify in a multi-root workspace (e.g. with both
folderAandfolderBconfigured) that changing anybiome.jsonor.editorconfigtriggers a reload.
🧹 Nitpick comments (8)
.changeset/rich-beds-clap.md (1)
1-5: Tighten the changeset with present‑tense behaviour and watch-trigger details.Front‑matter looks good. To align with guidelines (present tense for current behaviour; representative example; every sentence ends with a period), suggest expanding the body to mention the re‑indexing on config changes and the thread‑aware loading.
Apply:
--- "@biomejs/biome": patch --- -Fixed [#7411](https://github.com/biomejs/biome/issues/7411). The Biome Language Server had a regression where opening an editor with a file already open wouldn't load the project settings correctly. +#### Language Server. + +Fixed [#7411](https://github.com/biomejs/biome/issues/7411). Configuration loading is now thread‑aware within a session, avoiding duplicate reads when concurrent requests arrive. When a watched configuration file (for example, `biome.json`/`biome.jsonc` or `.editorconfig`) changes, the server re‑indexes the project and reloads settings. Opening an editor with a file already open now loads the correct project settings, restoring diagnostics, code actions, and formatting. + +```jsonc +// biome.json +{ + "formatter": { "enabled": true } +} +```crates/biome_configuration/src/lib.rs (1)
647-654: Add convenience predicates for symmetry.You already exposed
is_from_user/is_from_lsp. Addingis_noneandis_from_workspacehelps call‑sites avoid matches and keeps the API tidy.impl ConfigurationPathHint { pub const fn is_from_user(&self) -> bool { matches!(self, Self::FromUser(_)) } pub const fn is_from_lsp(&self) -> bool { matches!(self, Self::FromLsp(_)) } + pub const fn is_none(&self) -> bool { + matches!(self, Self::None) + } + pub const fn is_from_workspace(&self) -> bool { + matches!(self, Self::FromWorkspace(_)) + } }crates/biome_lsp/src/server.rs (2)
312-327: Minor log tweak for accuracy.The message names only
biome.json; we also supportbiome.jsoncand.editorconfig.- info!("Attempting to load the configuration from 'biome.json' file"); + info!("Loading workspace configuration (biome.json/biome.jsonc, .editorconfig).");
410-412: Workspace folder updates: confirm removal semantics.You close projects for
removed, then callupdate_workspace_folderswith onlyadded. Sanity‑check thatupdate_workspace_foldersdoesn’t also expect the full set (added + retained) to avoid stale internal state, and that a subsequentload_workspace_settings(true)correctly re‑indexes only active folders.Happy to add/adjust an integration test in
server.tests.rsthat adds/removes folders and asserts diagnostics/settings reflect the active set—shout if you want me to draft it.crates/biome_lsp/src/handlers/text_document.rs (2)
47-48: Typo in comment (“Ifo so”)Minor polish.
- // Ifo so, we return the project folder, otherwise we use the folder provided by the function did_open + // If so, we return the project folder; otherwise we use the folder provided by did_open.
35-35: Use tracing instead ofeprintln!Prefer structured logs; these
eprintln!calls will bypass the configured logger.- eprintln!("Session id {:?}", session.key); + debug!("Session id {:?}", session.key);- eprintln!( - "Loading configuration from text_document {:?}", - &project_path - ); + debug!("Loading configuration from text_document {:?}", &project_path);Also applies to: 78-83
crates/biome_lsp/src/session.rs (2)
369-371: Nit: message typo inexpectTiny fix, keeps logs tidy.
- Some(path) => Utf8PathBuf::from_path_buf(path.as_ref().to_path_buf()) - .expect("To to have a UTF-8 path"), + Some(path) => Utf8PathBuf::from_path_buf(path.as_ref().to_path_buf()) + .expect("To have a UTF-8 path"),
726-732: Param naming mismatch (reloadvsforce)
load_biome_configuration_file_internaltakesforceand you passreload. Consider renamingreloadtoforce_scan(or similar) across both functions to reflect intent and avoid confusion.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.changeset/rich-beds-clap.md(1 hunks)crates/biome_configuration/src/lib.rs(1 hunks)crates/biome_lsp/src/handlers/text_document.rs(2 hunks)crates/biome_lsp/src/server.rs(3 hunks)crates/biome_lsp/src/session.rs(14 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
.changeset/*.md
📄 CodeRabbit inference engine (CONTRIBUTING.md)
.changeset/*.md: Create changesets withjust new-changeset; store them in.changeset/with correct frontmatter (package keys and change type).
In changeset descriptions, follow content conventions: user-facing changes only; past tense for what you did; present tense for current behavior; link issues for fixes; link rules/assists; include representative code blocks; end every sentence with a period.
When adding headers in a changeset, only use #### or ##### levels.
Files:
.changeset/rich-beds-clap.md
**/*.{rs,toml}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Format Rust and TOML files before committing (use
just f/just format).
Files:
crates/biome_configuration/src/lib.rscrates/biome_lsp/src/server.rscrates/biome_lsp/src/handlers/text_document.rscrates/biome_lsp/src/session.rs
crates/biome_*/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place core crates under /crates/biome_*/
Files:
crates/biome_configuration/src/lib.rscrates/biome_lsp/src/server.rscrates/biome_lsp/src/handlers/text_document.rscrates/biome_lsp/src/session.rs
crates/biome_configuration/src/**
📄 CodeRabbit inference engine (CLAUDE.md)
Keep configuration sources under biome_configuration/src/
Files:
crates/biome_configuration/src/lib.rs
🧠 Learnings (4)
📚 Learning: 2025-08-11T11:53:15.299Z
Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_service/CONTRIBUTING.md:0-0
Timestamp: 2025-08-11T11:53:15.299Z
Learning: Applies to crates/biome_service/../biome_lsp/src/server.tests.rs : Keep end-to-end LSP tests in biome_lsp’s server.tests.rs
Applied to files:
crates/biome_lsp/src/server.rs
📚 Learning: 2025-08-11T11:53:15.299Z
Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_service/CONTRIBUTING.md:0-0
Timestamp: 2025-08-11T11:53:15.299Z
Learning: Applies to crates/biome_service/src/workspace/server.rs : Use WorkspaceServer (src/workspace/server.rs) to maintain workspace state in daemon and CLI daemonless modes
Applied to files:
crates/biome_lsp/src/server.rs
📚 Learning: 2025-08-11T11:53:15.299Z
Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_service/CONTRIBUTING.md:0-0
Timestamp: 2025-08-11T11:53:15.299Z
Learning: Applies to crates/biome_service/src/workspace/client.rs : Use WorkspaceClient (src/workspace/client.rs) to connect to the daemon and communicate with WorkspaceServer
Applied to files:
crates/biome_lsp/src/session.rs
📚 Learning: 2025-08-11T11:53:15.299Z
Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_service/CONTRIBUTING.md:0-0
Timestamp: 2025-08-11T11:53:15.299Z
Learning: Applies to crates/biome_service/src/workspace.rs : Implement the Workspace trait in src/workspace.rs
Applied to files:
crates/biome_lsp/src/session.rs
🧬 Code graph analysis (2)
crates/biome_lsp/src/server.rs (1)
crates/biome_lsp/src/server.tests.rs (1)
initialized(258-260)
crates/biome_lsp/src/handlers/text_document.rs (2)
crates/biome_lsp/src/utils.rs (1)
apply_document_changes(361-412)crates/biome_lsp/src/session.rs (1)
base_path(604-619)
⏰ 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). (25)
- GitHub Check: Test (depot-windows-2022-16)
- GitHub Check: End-to-end tests
- GitHub Check: Test (depot-ubuntu-24.04-arm-16)
- GitHub Check: Documentation
- GitHub Check: Lint project (depot-windows-2022)
- GitHub Check: Lint project (depot-ubuntu-24.04-arm-16)
- GitHub Check: Check Dependencies
- GitHub Check: Test Node.js API
- GitHub Check: Bench (biome_graphql_parser)
- GitHub Check: autofix
- GitHub Check: Bench (biome_package)
- GitHub Check: Bench (biome_configuration)
- GitHub Check: Bench (biome_css_analyze)
- GitHub Check: Bench (biome_module_graph)
- GitHub Check: Bench (biome_html_formatter)
- GitHub Check: Bench (biome_json_analyze)
- GitHub Check: Bench (biome_css_formatter)
- GitHub Check: Bench (biome_css_parser)
- GitHub Check: Bench (biome_graphql_formatter)
- GitHub Check: Bench (biome_json_formatter)
- GitHub Check: Bench (biome_html_parser)
- GitHub Check: Bench (biome_js_parser)
- GitHub Check: Bench (biome_json_parser)
- GitHub Check: Bench (biome_js_formatter)
- GitHub Check: Bench (biome_js_analyze)
🔇 Additional comments (1)
crates/biome_configuration/src/lib.rs (1)
608-629: Deriving Eq/Hash and marking the default variant is spot on.This unblocks use as a hashed key and clarifies defaults without runtime boilerplate. No functional concerns from me.
CodSpeed Performance ReportMerging #7418 will not alter performanceComparing Summary
|
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
crates/biome_lsp/src/handlers/text_document.rs (1)
46-63: Workspace-folder resolution fix: good catch.Switching to filter_map + find removes the previous short-circuit bug with find_map and correctly selects the matching workspace root.
🧹 Nitpick comments (4)
crates/biome_lsp/src/handlers/text_document.rs (4)
51-56: Avoid panic on non‑UTF‑8 workspace URIs.Expecting UTF‑8 here can crash the server; prefer graceful skip.
Apply:
- .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_map(|folder| { + folder + .uri + .to_file_path() + .and_then(|p| Utf8PathBuf::from_path_buf(p.to_path_buf()).ok()) + })
35-35: Swap eprintln! for tracing.Use tracing to keep logs structured and configurable.
Apply:
- eprintln!("Session id {:?}", session.key); + debug!(session_id = ?session.key, "did_open");
74-78: Also replace this eprintln! with tracing.Keeps output tidy and consistent.
Apply:
- eprintln!( - "Loading configuration from text_document {:?}", - &project_path - ); + debug!(project_path = ?project_path, "Loading configuration from text_document");
85-98: Proceed-on-success, bail-on-failure flow reads well.Only suggestion (optional): consider emitting a client.logMessage at info/debug level on failure to aid users when config load fails silently.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/biome_lsp/src/handlers/text_document.rs(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{rs,toml}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Format Rust and TOML files before committing (use
just f/just format).
Files:
crates/biome_lsp/src/handlers/text_document.rs
crates/biome_*/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place core crates under /crates/biome_*/
Files:
crates/biome_lsp/src/handlers/text_document.rs
🧠 Learnings (1)
📚 Learning: 2025-08-11T11:53:15.299Z
Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_service/CONTRIBUTING.md:0-0
Timestamp: 2025-08-11T11:53:15.299Z
Learning: Applies to crates/biome_service/src/workspace.rs : Implement the Workspace trait in src/workspace.rs
Applied to files:
crates/biome_lsp/src/handlers/text_document.rs
🧬 Code graph analysis (1)
crates/biome_lsp/src/handlers/text_document.rs (2)
crates/biome_lsp/src/utils.rs (1)
apply_document_changes(361-412)crates/biome_lsp/src/session.rs (1)
base_path(604-619)
🪛 GitHub Actions: Pull request Node.js
crates/biome_lsp/src/handlers/text_document.rs
[error] 64-64: Rust compile error E0308: 'if' and 'else' have incompatible types (expected '()' found 'Utf8PathBuf') in crates/biome_lsp/src/handlers/text_document.rs:64:20. Hint: consider returning the local binding 'base_path'. Command: cargo build -p biome_cli --release exited with code 101.
⏰ 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). (20)
- GitHub Check: Bench (biome_css_formatter)
- GitHub Check: Bench (biome_json_analyze)
- GitHub Check: Bench (biome_html_formatter)
- GitHub Check: Bench (biome_module_graph)
- GitHub Check: Bench (biome_json_formatter)
- GitHub Check: Bench (biome_graphql_parser)
- GitHub Check: Bench (biome_configuration)
- GitHub Check: Bench (biome_html_parser)
- GitHub Check: Bench (biome_css_analyze)
- GitHub Check: Bench (biome_package)
- GitHub Check: Bench (biome_graphql_formatter)
- GitHub Check: Bench (biome_js_analyze)
- GitHub Check: Bench (biome_css_parser)
- GitHub Check: Bench (biome_json_parser)
- GitHub Check: Bench (biome_js_parser)
- GitHub Check: Bench (biome_js_formatter)
- GitHub Check: Documentation
- GitHub Check: Lint project (depot-windows-2022)
- GitHub Check: Check Dependencies
- GitHub Check: autofix
🔇 Additional comments (1)
crates/biome_lsp/src/handlers/text_document.rs (1)
2-2: Imports look spot-on for the new lazy load and folder resolution flow.Nothing to change here; these are the right additions.
Also applies to: 5-5, 10-13
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Summary
Closes #7411
This PR makes the loading of configuration files "thread-aware". Unfortunately, the previous fix didn't work out as intended. In this PR, the loading of the configuration files uses a locking mechanism, so when two tasks arrive, the last one waits for the previous one, and uses the same value that was resolved by the first task. The loading operations are locked using the session ID, to avoid conflicts inside the same project.
Second, we enforce the scanning of the project when a
biome.jsonfile changes (precisely, when a watched file changes). The bug was caused by the fact that when the watcher is enabled, the project isn't re-indexed. This causes the project settings not to be updated.We now pass a
forceboolean up until when we callsca_project. When a watched file changes, the settings are now correctly reloaded.Test Plan
Here's a video that shows how diagnostics are correctly updated upon configuration updates
Screen.Recording.2025-09-06.at.13.59.36.mov
Docs