-
-
Notifications
You must be signed in to change notification settings - Fork 713
fix(lsp): internal project handling #7377
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
Conversation
🦋 Changeset detectedLatest commit: a5a0375 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 |
WalkthroughThe PR updates the Biome LSP configuration handling: did_open now relies on session.configuration_status, project registration reuses existing projects, config-file detection uses suffix matching, nested settings avoid an extra clone, diagnostic instrumentation logging was simplified, and Document now derives Debug. Two changeset files add patch-release notes for fixes related to monorepo diagnostics and nested-config recomputation. Assessment against linked issues
Out-of-scope changes
Suggested reviewers
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
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.
Actionable comments posted: 1
🧹 Nitpick comments (5)
.changeset/curly-tables-stare.md (1)
1-5
: Tidy up the changeset phrasing and add a link.Reads well and follows frontmatter, but consider:
- Link the fixed issue for traceability.
- Minor wording: “monorepo setting” → “monorepo setup”.
Example:
-Fixed a bug where the Biome Language Server didn't correctly compute the diagnostics of a monorepo setting, caused by an incorrect handling of the project status. +Fixed a bug where the Biome Language Server didn't correctly compute diagnostics in a monorepo setup due to incorrect handling of the project status. See #7371..changeset/spicy-things-bathe.md (1)
1-5
: LGTM, consistent with guidelines.Frontmatter, tense, and link look right. If you want consistency with the other changeset, mirror the phrasing (“in a monorepo setup”).
crates/biome_service/src/projects.rs (1)
268-277
: Avoid unnecessary clones in set_nested_settings.You can move
path
andsettings
into the map since they’re owned params, saving two clones.- let mut nested_settings = data.nested_settings.clone(); - nested_settings.insert(path.clone(), settings.clone()); + let mut nested_settings = data.nested_settings.clone(); + nested_settings.insert(path, settings);crates/biome_lsp/src/session.rs (1)
468-472
: Preserve tracing field or attach it to the log event.The span declares
diagnostic_count
but it’s no longer recorded. Either record the field or attach it to the info! call.- info!("Diagnostics sent to the client {}", diagnostics.len()); + tracing::Span::current().record("diagnostic_count", diagnostics.len()); + info!(diagnostic_count = diagnostics.len(), "Diagnostics sent to the client");crates/biome_lsp/src/server.rs (1)
357-360
: Suffix match for config detection looks right; consider broadening.editorconfig
watch scopeThe switch to
ends_with
fixes nested config detection. One follow‑up: our registered watches only track root.editorconfig
per folder; with this broader detection, we’ll never reach this branch for nested.editorconfig
changes. Consider registering"**/.editorconfig"
for workspace folders so nested editorconfigs also trigger a refresh.- FileSystemWatcher { - glob_pattern: GlobPattern::Relative(RelativePattern { - pattern: ".editorconfig".to_string(), - base_uri: OneOf::Left(folder.clone()), - }), - kind: Some(WatchKind::all()), - }, + FileSystemWatcher { + glob_pattern: GlobPattern::Relative(RelativePattern { + pattern: "**/.editorconfig".to_string(), + base_uri: OneOf::Left(folder.clone()), + }), + kind: Some(WatchKind::all()), + },
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
.changeset/curly-tables-stare.md
(1 hunks).changeset/spicy-things-bathe.md
(1 hunks)crates/biome_lsp/src/documents.rs
(1 hunks)crates/biome_lsp/src/handlers/text_document.rs
(2 hunks)crates/biome_lsp/src/server.rs
(1 hunks)crates/biome_lsp/src/session.rs
(2 hunks)crates/biome_service/src/projects.rs
(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
.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/spicy-things-bathe.md
.changeset/curly-tables-stare.md
**/*.{rs,toml}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Format Rust and TOML files before committing (use
just f
/just format
).
Files:
crates/biome_lsp/src/documents.rs
crates/biome_lsp/src/server.rs
crates/biome_lsp/src/session.rs
crates/biome_service/src/projects.rs
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/documents.rs
crates/biome_lsp/src/server.rs
crates/biome_lsp/src/session.rs
crates/biome_service/src/projects.rs
crates/biome_lsp/src/handlers/text_document.rs
🧠 Learnings (6)
📚 Learning: 2025-08-11T11:46:05.836Z
Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_diagnostics/CONTRIBUTING.md:0-0
Timestamp: 2025-08-11T11:46:05.836Z
Learning: Applies to crates/biome_diagnostics/**/*.rs : Types implementing Diagnostic must also implement Debug (e.g., use #[derive(Debug, Diagnostic)])
Applied to files:
crates/biome_lsp/src/documents.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/watcher.tests.rs : Place watcher tests for workspace methods in src/workspace/watcher.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/../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_watcher.rs : Keep workspace state in sync with the filesystem using WorkspaceWatcher; only active in daemon mode
Applied to files:
crates/biome_lsp/src/server.rs
📚 Learning: 2025-08-11T11:46:05.836Z
Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_diagnostics/CONTRIBUTING.md:0-0
Timestamp: 2025-08-11T11:46:05.836Z
Learning: Applies to crates/biome_diagnostics/**/*.rs : Use helper advice types from biome_diagnostics::v2 (CodeFrameAdvice, CommandAdvice, DiffAdvice, LogAdvice) when suitable
Applied to files:
crates/biome_lsp/src/session.rs
📚 Learning: 2025-08-11T11:46:05.836Z
Learnt from: CR
PR: biomejs/biome#0
File: crates/biome_diagnostics/CONTRIBUTING.md:0-0
Timestamp: 2025-08-11T11:46:05.836Z
Learning: Applies to crates/biome_diagnostics/**/*.rs : Specify category and severity using #[diagnostic(...)] on the type or derive them from fields
Applied to files:
crates/biome_lsp/src/session.rs
🧬 Code graph analysis (1)
crates/biome_lsp/src/server.rs (1)
crates/biome_fs/src/fs.rs (1)
file_names
(32-34)
⏰ 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). (8)
- GitHub Check: Test Node.js API
- GitHub Check: Test (depot-windows-2022-16)
- GitHub Check: Documentation
- GitHub Check: Check Dependencies
- GitHub Check: Lint project (depot-windows-2022)
- GitHub Check: Lint project (depot-ubuntu-24.04-arm-16)
- GitHub Check: Test (depot-ubuntu-24.04-arm-16)
- GitHub Check: autofix
🔇 Additional comments (3)
crates/biome_lsp/src/documents.rs (1)
7-12
: Deriving Debug is a good addition.Enables better tracing/logging without changing semantics. No objections.
crates/biome_lsp/src/session.rs (1)
784-821
: Guard re-scans to avoid duplicate watchers
insert_and_scan_project
always callsworkspace.scan_project(..., watch: true)
, which unconditionally registers file watchers. To prevent double-watching on an existing project, only invoke it when the project is newly opened (e.g. via anis_new
flag), or use a non-watching rescan API if available. Verify whetherworkspace.scan_project
deduplicates watchers—if it doesn’t, wrap the call inif is_new { … }
.crates/biome_lsp/src/handlers/text_document.rs (1)
11-11
: Import tweak is fine
field
is needed by the tracing attributes below. All good.
ec336fb
to
a5a0375
Compare
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.
Actionable comments posted: 0
♻️ Duplicate comments (1)
crates/biome_lsp/src/handlers/text_document.rs (1)
34-50
: Don’t drop didOpen while configuration/project are unresolved — defer itEarly-returning here orphans the document (no tracking, didChange becomes a no-op, diagnostics never appear). This was flagged earlier and still applies.
Minimal changes to demote log levels and explicitly defer the open for replay after config load:
let status = session.configuration_status(); let project_key = if status.is_loaded() { match session.project_for_path(&path) { Some(project_key) => project_key, None => { - error!("Could not find project for {path}"); + debug!("Deferring didOpen: project not resolved yet for {path}"); + // enqueue for replay once project mapping stabilises + session.defer_did_open(url.clone(), version, content.clone(), language_hint); return Ok(()); } } } else { if status.is_plugin_error() { session.client.show_message(MessageType::WARNING, "The plugin loading has failed. Biome will report only parsing errors until the file is fixed or its usage is disabled.").await; } - error!("Configuration could not be loaded for {path}"); + debug!("Deferring didOpen: configuration not loaded for {path}"); + // enqueue for replay after configuration becomes loaded + session.defer_did_open(url.clone(), version, content.clone(), language_hint); return Ok(()); };Follow-up (recommended):
- Implement Session::defer_did_open + a flush triggered when configuration_status transitions to loaded.
- Alternatively, await a short watch/notify before deferring (to cover race at startup).
Happy to draft the queue + flush plumbing in Session if you like.
#!/bin/bash # Evidence gathering: ensure no existing replay/queue already handles didOpen before config load. set -euo pipefail # Look for any queue/defer/replay of didOpen rg -n -C2 -g 'crates/**' -P '(queue|defer|replay).*(didOpen|did_open)|didOpen.*(queue|defer|replay)' # Who reacts to configuration becoming loaded? rg -n -C2 -g 'crates/**' -P 'configuration_status\(\)|is_loaded\(\)|on_configuration_loaded' # Check if anything reattaches documents after config load rg -n -C2 -g 'crates/**' -P 'insert_document\(|open_file\('
🧹 Nitpick comments (2)
crates/biome_lsp/src/handlers/text_document.rs (2)
12-12
: Use warn! for transient, non-fatal conditions (and import it)Deferrals aren’t errors. Bring in warn! so you can right-size log levels below.
-use tracing::{debug, error, field}; +use tracing::{debug, warn, error, field};
45-47
: Avoid warning spam; tweak wordingGuard this message so it’s shown once per session, not per file. Small copy tweak reads better.
- session.client.show_message(MessageType::WARNING, "The plugin loading has failed. Biome will report only parsing errors until the file is fixed or its usage is disabled.").await; + session.client.show_message( + MessageType::WARNING, + "Plugin loading failed. Biome will only report parsing errors until the plugin is fixed or disabled in your configuration." + ).await;Consider tracking a shown_once flag on Session to de-duplicate.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
.changeset/curly-tables-stare.md
(1 hunks).changeset/spicy-things-bathe.md
(1 hunks)crates/biome_lsp/src/documents.rs
(1 hunks)crates/biome_lsp/src/handlers/text_document.rs
(2 hunks)crates/biome_lsp/src/server.rs
(1 hunks)crates/biome_lsp/src/session.rs
(2 hunks)crates/biome_service/src/projects.rs
(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .changeset/curly-tables-stare.md
🚧 Files skipped from review as they are similar to previous changes (5)
- crates/biome_lsp/src/documents.rs
- .changeset/spicy-things-bathe.md
- crates/biome_service/src/projects.rs
- crates/biome_lsp/src/server.rs
- crates/biome_lsp/src/session.rs
🧰 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
⏰ 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). (8)
- GitHub Check: Documentation
- GitHub Check: Test (depot-ubuntu-24.04-arm-16)
- GitHub Check: Test (depot-windows-2022-16)
- GitHub Check: Lint project (depot-windows-2022)
- GitHub Check: Check Dependencies
- GitHub Check: Lint project (depot-ubuntu-24.04-arm-16)
- GitHub Check: autofix
- GitHub Check: Test Node.js API
🔇 Additional comments (1)
crates/biome_lsp/src/handlers/text_document.rs (1)
11-11
: Import for MessageType is spot onNeeded for the warning surfaced to clients. All good.
Summary
Closes #7371
There were two bugs:
text_document
is called by the client, causing the creation of a new project. However, in the meantime, a call to load the configuration file is issued, causing the creation of two projects. Since the configuration status is atomic, I decided to reduce the scope oftext_document
, and pull diagnostics only when the configuration file is loaded. It seems to work.There's only small hiccup, which I couldn't fix. I'll check it later. When a configuration file is updated, we eventually
update_all_diagnostics
to update the diagnostics of the opened files. For some reason, the callworkspace.pull_diagnostics
doesn't pull the updated diagnostics. If you then attempt to modify the file, the correct diagnostics are pulled. Very weird. Requires more testing.Test Plan
Heavily manual test using the reproduction.
Docs