-
-
Notifications
You must be signed in to change notification settings - Fork 723
fix(lsp): load extension settings before configuration on document open #7764
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
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: 44b0f92 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 a synchronous initialization marker and explicit initialization tracking to the LSP session (an Possibly related PRs
Suggested labels
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/biome_lsp/src/handlers/text_document.rs (2)
35-35
: Remove debug statement.This
eprintln!
appears to be leftover debug code and should be removed or converted to proper logging via thetracing
crate.Apply this diff:
- eprintln!("Session id {:?}", session.key);
74-77
: Remove or replace debug output.Similar to line 35, this
eprintln!
should be removed or converted to proper structured logging using thetracing
crate (e.g.,debug!
orinfo!
).Apply this diff:
- eprintln!( - "Loading configuration from text_document {:?}", - &project_path - ); + debug!( + "Loading configuration from text_document {:?}", + &project_path + );
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.changeset/swift-apples-greet.md
(1 hunks)crates/biome_lsp/src/handlers/text_document.rs
(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
crates/biome_*/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place core crates under /crates/biome_*/
Files:
crates/biome_lsp/src/handlers/text_document.rs
**/*.rs
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.rs
: Format Rust files before committing (e.g., viajust f
which formats Rust)
Document rules, assists, and options with inline rustdoc in source
Files:
crates/biome_lsp/src/handlers/text_document.rs
.changeset/*.md
📄 CodeRabbit inference engine (CONTRIBUTING.md)
.changeset/*.md
: In changesets, only use #### or ##### headers; other header levels are not allowed
Changesets should cover user-facing changes only; internal changes do not need changesets
Use past tense for what you did and present tense for current Biome behavior in changesets
When fixing a bug in a changeset, start with an issue link (e.g., “Fixed #1234: …”)
When referencing a rule or assist in a changeset, include a link to its page on the website
Include code blocks in changesets when applicable to illustrate changes
End every sentence in a changeset with a period
Files:
.changeset/swift-apples-greet.md
🔇 Additional comments (1)
crates/biome_lsp/src/handlers/text_document.rs (1)
78-78
: load_extension_settings handles errors internally and is idempotent
Client configuration failures and JSON parse errors are caught and logged without panicking; each invocation safely overwrites the stored settings.
758e841
to
e736d9e
Compare
e736d9e
to
5786194
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
🧹 Nitpick comments (1)
crates/biome_lsp/src/session.rs (1)
630-638
: Use Acquire/Release forinitialized
atomicsApply this diff to explicitly encode the happens-before relationship:
@@ crates/biome_lsp/src/session.rs - self.initialized.store(true, Ordering::Relaxed); + self.initialized.store(true, Ordering::Release); - self.initialized.load(Ordering::Relaxed) + self.initialized.load(Ordering::Acquire)The
RwLock
onextension_settings
makes Relaxed safe, but Release/Acquire is clearer. Confirmed thatdid_open
’shas_initialized()
check already handles thefalse
case correctly.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
crates/biome_lsp/src/handlers/text_document.rs
(1 hunks)crates/biome_lsp/src/server.rs
(1 hunks)crates/biome_lsp/src/session.rs
(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/biome_lsp/src/handlers/text_document.rs
🧰 Additional context used
📓 Path-based instructions (2)
crates/biome_*/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place core crates under /crates/biome_*/
Files:
crates/biome_lsp/src/session.rs
crates/biome_lsp/src/server.rs
**/*.rs
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.rs
: Format Rust files before committing (e.g., viajust f
which formats Rust)
Document rules, assists, and options with inline rustdoc in source
Files:
crates/biome_lsp/src/session.rs
crates/biome_lsp/src/server.rs
🧬 Code graph analysis (1)
crates/biome_lsp/src/session.rs (1)
crates/biome_lsp/src/server.rs (2)
new
(49-61)new
(548-564)
🔇 Additional comments (3)
crates/biome_lsp/src/session.rs (2)
89-90
: Field addition looks good.The
initialized
flag withAtomicBool
is appropriate for tracking the LSP initialization state in a thread-safe manner.
219-219
: Initialization is correct.Properly initialised to
false
since the session hasn't received theinitialized()
notification at construction time.crates/biome_lsp/src/server.rs (1)
317-317
: Correct placement to fix the race condition.Calling
set_initialized()
after loading extension and workspace settings ensures thatdid_open
can reliably check whether initialization has completed, preventing the race condition wheredid_open
runs before settings are loaded.
Summary
Fixed a race condition between
initialized
anddid_open
that caused therequireConfiguration
setting to sometimes be ignored in editor extensions.Related: zed-industries/zed#39803, biomejs/biome-zed#59, #6589, #6287
The Problem
There was a race condition where
did_open
could execute beforeinitialized
completed loading extension settings. This manifested differently in different editors:Zed: The LSP server starts when a file is opened, causing
did_open
to execute before extension settings are loaded frominitialized
, consistently ignoring therequireConfiguration
setting.VS Code: The LSP server starts even without open files. The bug would occur when:
It would work correctly only when opening VS Code without files first (allowing
initialized
to complete), then opening files afterward.The Solution
This fix ensures
load_extension_settings()
is called indid_open
when creating a new project, before loading the Biome configuration file. This guarantees extension settings are available regardless of the timing betweeninitialized
anddid_open
, maintaining consistency with other configuration loading flows.Test Plan
Manual testing in Zed:
require_config_file
setting in Zed's Biome extension settingsrequireConfiguration
setting is respectedManual testing in VS Code:
requireConfiguration
setting in VS Code's Biome extension settingsrequireConfiguration
setting is respected in both scenarios