fix: reload file from disk on save to fix stale diagnostics #8260
Conversation
🦋 Changeset detectedLatest commit: c689419 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 |
ho991217
changed the title
WalkthroughAdds a changelog entry and implements LSP textDocument/didSave handling. Introduces a public Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)**/*.rs📄 CodeRabbit inference engine (CONTRIBUTING.md)
Files:
🧠 Learnings (7)📓 Common learnings📚 Learning: 2025-11-24T18:06:12.048ZApplied to files:
📚 Learning: 2025-11-24T18:05:20.371ZApplied to files:
📚 Learning: 2025-11-24T18:03:52.024ZApplied to files:
📚 Learning: 2025-11-24T18:06:12.048ZApplied to files:
📚 Learning: 2025-11-24T18:06:12.048ZApplied to files:
📚 Learning: 2025-11-24T18:06:12.048ZApplied to files:
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/biome_lsp/src/handlers/text_document.rs (1)
194-239: did_save handler logic looks good; consider adding a regression testThe save handler nicely forces a reindex from disk (close → reopen with
FileContent::FromServer→ refreshDocument→ update diagnostics), which matches the described stale-diagnostics bug and aligns with the existingdid_open/did_changepatterns.Given this is a subtle multi-instance / on-disk synchronisation issue, it would be great (when you have the patience) to back it with an LSP-level regression test in the existing
server.tests.rs/LSP test harness, so we don’t accidentally break this flow in future. Based on learnings, similar workspace behaviours are usually covered with dedicated watcher/LSP tests.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.changeset/wet-keys-enjoy.md(1 hunks)crates/biome_lsp/src/handlers/text_document.rs(1 hunks)crates/biome_lsp/src/server.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/.changeset/*.md
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Changeset descriptions should be user-facing, use past tense for actions taken (e.g., 'Added new feature'), and present tense for Biome behavior (e.g., 'Biome now supports...'). Include issue links, rule links, and code examples where applicable.
Files:
.changeset/wet-keys-enjoy.md
**/*.rs
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.rs: Use the Rustdbg!()macro for debugging output during test execution, and pass the--show-outputflag tocargo testto display debug output.
Use snapshot testing with theinstacrate for testing in Rust projects. Accept or reject snapshots usingcargo insta accept,cargo insta reject, orcargo insta review.
Write doc comments as doc tests in Rust using code blocks with assertions that will be executed during the testing phase.
Use rustdoc inline documentation for rules, assists, and their options. Create corresponding documentation PRs for other documentation updates against thenextbranch of the website repository.
Set theversionmetadata field in linter rule implementations to'next'for newly created rules. Update this field to the new version number when releasing a minor or major version.
Files:
crates/biome_lsp/src/server.rscrates/biome_lsp/src/handlers/text_document.rs
🧠 Learnings (3)
📚 Learning: 2025-11-24T18:03:52.014Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T18:03:52.014Z
Learning: For bugfix/feature PRs visible to Biome toolchain users or affecting published crates, create a changeset using the `just new-changeset` command with appropriate package selection, change type (major/minor/patch), and description.
Applied to files:
.changeset/wet-keys-enjoy.md
📚 Learning: 2025-08-05T14:43:29.581Z
Learnt from: dyc3
Repo: biomejs/biome PR: 7081
File: packages/@biomejs/biome/configuration_schema.json:7765-7781
Timestamp: 2025-08-05T14:43:29.581Z
Learning: The file `packages/biomejs/biome/configuration_schema.json` is auto-generated and should not be manually edited or reviewed for schema issues; any changes should be made at the code generation source.
Applied to files:
.changeset/wet-keys-enjoy.md
📚 Learning: 2025-11-24T18:06:12.017Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_service/CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T18:06:12.017Z
Learning: Applies to crates/biome_service/src/workspace/watcher.tests.rs : Implement watcher tests for workspace methods in watcher.tests.rs and end-to-end tests in LSP tests
Applied to files:
crates/biome_lsp/src/server.rs
🧬 Code graph analysis (2)
crates/biome_lsp/src/server.rs (1)
crates/biome_lsp/src/handlers/text_document.rs (1)
did_save(196-239)
crates/biome_lsp/src/handlers/text_document.rs (2)
crates/biome_lsp/src/server.rs (4)
params(367-371)did_save(408-412)new(49-61)new(580-596)crates/biome_lsp/src/documents.rs (1)
new(15-21)
🔇 Additional comments (1)
crates/biome_lsp/src/server.rs (1)
408-412: did_save wiring matches existing textDocument handlersForwarding to
handlers::text_document::did_saveand swallowing the result with.await.ok()is consistent withdid_open/did_change/did_close, so the integration looks spot on.
ematipico
left a comment
ematipico
left a comment
There was a problem hiding this comment.
Can you please add a test so we make sure that the new function does we expect?
Added at 8670c1f |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
crates/biome_lsp/src/server.tests.rs (1)
4149-4237: Well-structured test for the content-sync mechanism.The test properly validates that
didSavetriggers a reload from disk, which is the core fix for the stale diagnostics issue. The use ofMemoryFileSystemto simulate external edits is appropriate, and the test follows established patterns in this file.One optional consideration: whilst this test verifies content syncing, it doesn't verify that diagnostics are updated post-save. This may be intentional (focusing the unit test on the sync mechanism), but you might want to ensure diagnostic updates are covered in integration tests or add a follow-up assertion here checking that diagnostics reflect the new content.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/biome_lsp/src/server.tests.rs(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.rs: Use the Rustdbg!()macro for debugging output during test execution, and pass the--show-outputflag tocargo testto display debug output.
Use snapshot testing with theinstacrate for testing in Rust projects. Accept or reject snapshots usingcargo insta accept,cargo insta reject, orcargo insta review.
Write doc comments as doc tests in Rust using code blocks with assertions that will be executed during the testing phase.
Use rustdoc inline documentation for rules, assists, and their options. Create corresponding documentation PRs for other documentation updates against thenextbranch of the website repository.
Set theversionmetadata field in linter rule implementations to'next'for newly created rules. Update this field to the new version number when releasing a minor or major version.
Files:
crates/biome_lsp/src/server.tests.rs
🧠 Learnings (4)
📚 Learning: 2025-11-24T18:06:12.017Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_service/CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T18:06:12.017Z
Learning: Applies to crates/biome_service/src/workspace/watcher.tests.rs : Implement watcher tests for workspace methods in watcher.tests.rs and end-to-end tests in LSP tests
Applied to files:
crates/biome_lsp/src/server.tests.rs
📚 Learning: 2025-11-24T18:06:12.017Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_service/CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T18:06:12.017Z
Learning: Applies to crates/biome_service/src/workspace*.rs : Implement the Workspace trait in the Biome Service to manage internal state of projects, including open documents, project layout instances, and module graph instances
Applied to files:
crates/biome_lsp/src/server.tests.rs
📚 Learning: 2025-11-24T18:06:12.017Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_service/CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T18:06:12.017Z
Learning: Applies to crates/biome_service/src/workspace/client.rs : Use WorkspaceClient implementation for creating connections to the daemon and communicating with WorkspaceServer
Applied to files:
crates/biome_lsp/src/server.tests.rs
📚 Learning: 2025-11-24T18:06:12.017Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_service/CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T18:06:12.017Z
Learning: Applies to crates/biome_service/src/workspace/server.rs : Use WorkspaceServer implementation for maintaining workspace state in daemon mode and CLI daemonless mode
Applied to files:
crates/biome_lsp/src/server.tests.rs
🔇 Additional comments (2)
crates/biome_lsp/src/server.tests.rs (2)
34-37: LGTM!Import addition is necessary for the new didSave functionality and properly integrated into the existing import list.
374-383: LGTM!The helper method follows the established pattern of other document lifecycle methods (didOpen, didChange, didClose) and correctly sets
text: Nonefor standard didSave behavior.
crates/biome_lsp/src/server.tests.rs
Outdated
|
|
||
| // #endregion | ||
|
|
||
| #[tokio::test] | ||
| async fn did_save_syncs_content_from_disk() -> Result<()> { | ||
| // ARRANGE: Set up file system with initial content | ||
| const INITIAL_CONTENT: &str = "const a = 1;"; | ||
| const DISK_CONTENT: &str = "const b = 2;"; |
There was a problem hiding this comment.
@ho991217 I would appreciate it if you could look at what the AI is doing. It placed the tests in the wrong section of the file. See the region comments? Please make sure the test is placed before line 4147
There was a problem hiding this comment.
I moved the test to just before the “MONOREPO TESTS” region, which feels more appropriate. 280c3ff
| // Trigger reindexing from disk to sync with saved content | ||
| session.workspace.close_file(CloseFileParams { | ||
| project_key: doc.project_key, | ||
| path: path.clone(), | ||
| })?; | ||
|
|
||
| // Reopen will load from disk | ||
| session.workspace.open_file(OpenFileParams { | ||
| project_key: doc.project_key, | ||
| path: path.clone(), | ||
| content: FileContent::FromServer, | ||
| document_file_source: None, | ||
| persist_node_cache: true, | ||
| })?; |
There was a problem hiding this comment.
Why do we close and open the file? Wouldn't it be better to use change_file?
There was a problem hiding this comment.
We use close_file and open_file instead of change_file to ensure synchronization when multiple LSP instances watch the same file.
Using change_file would only update with the in-memory content, which might not match what's actually on disk.
| // Sync the Document object with the content reloaded from disk | ||
| let fresh_content = session.workspace.get_file_content(GetFileContentParams { | ||
| project_key: doc.project_key, | ||
| path, | ||
| })?; | ||
|
|
||
| session.insert_document( | ||
| url.clone(), | ||
| Document::new(doc.project_key, doc.version, &fresh_content), | ||
| ); |
There was a problem hiding this comment.
As per spec, the notification could have the new text: https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#textDocument_didSave
If so, we should use that text instead
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/biome_lsp/src/handlers/text_document.rs(1 hunks)crates/biome_lsp/src/server.tests.rs(2 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 (1)
**/*.rs
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.rs: Use the Rustdbg!()macro for debugging output during test execution, and pass the--show-outputflag tocargo testto display debug output.
Use snapshot testing with theinstacrate for testing in Rust projects. Accept or reject snapshots usingcargo insta accept,cargo insta reject, orcargo insta review.
Write doc comments as doc tests in Rust using code blocks with assertions that will be executed during the testing phase.
Use rustdoc inline documentation for rules, assists, and their options. Create corresponding documentation PRs for other documentation updates against thenextbranch of the website repository.
Set theversionmetadata field in linter rule implementations to'next'for newly created rules. Update this field to the new version number when releasing a minor or major version.
Files:
crates/biome_lsp/src/server.tests.rs
🧠 Learnings (4)
📚 Learning: 2025-11-24T18:06:12.048Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_service/CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T18:06:12.048Z
Learning: Applies to crates/biome_service/src/workspace/watcher.tests.rs : Implement watcher tests for workspace methods in watcher.tests.rs and end-to-end tests in LSP tests
Applied to files:
crates/biome_lsp/src/server.tests.rs
📚 Learning: 2025-11-24T18:06:12.048Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_service/CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T18:06:12.048Z
Learning: Applies to crates/biome_service/src/workspace*.rs : Implement the Workspace trait in the Biome Service to manage internal state of projects, including open documents, project layout instances, and module graph instances
Applied to files:
crates/biome_lsp/src/server.tests.rs
📚 Learning: 2025-11-24T18:06:12.048Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_service/CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T18:06:12.048Z
Learning: Applies to crates/biome_service/src/workspace/client.rs : Use WorkspaceClient implementation for creating connections to the daemon and communicating with WorkspaceServer
Applied to files:
crates/biome_lsp/src/server.tests.rs
📚 Learning: 2025-11-24T18:06:12.048Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_service/CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T18:06:12.048Z
Learning: Applies to crates/biome_service/src/workspace/server.rs : Use WorkspaceServer implementation for maintaining workspace state in daemon mode and CLI daemonless mode
Applied to files:
crates/biome_lsp/src/server.tests.rs
🧬 Code graph analysis (1)
crates/biome_lsp/src/server.tests.rs (6)
crates/biome_service/src/workspace/client.rs (1)
fs(210-212)crates/biome_lsp/src/session.rs (1)
new(199-223)crates/biome_formatter/src/lib.rs (10)
new(890-892)new(965-967)new(1053-1065)new(1290-1292)new(1340-1342)new(2109-2117)new(2236-2238)default(306-308)default(408-410)default(675-677)crates/biome_fs/src/fs/os.rs (3)
new(33-37)new(416-433)default(41-46)crates/biome_lsp/src/server.rs (2)
default(573-575)new_with_fs(599-611)crates/biome_configuration/src/analyzer/mod.rs (4)
default(86-88)default(132-134)default(363-365)default(1120-1122)
🔇 Additional comments (1)
crates/biome_lsp/src/server.tests.rs (1)
34-34: LGTM!The import is correctly placed and necessary for the new didSave test.
| #[tokio::test] | ||
| async fn did_save_syncs_content_from_text_parameter() -> Result<()> { | ||
| const INITIAL_CONTENT: &str = "const a=1;"; | ||
| const SAVED_CONTENT: &str = "const b=2;"; | ||
|
|
||
| let fs = Arc::new(MemoryFileSystem::default()); | ||
| let file_path = to_utf8_file_path_buf(uri!("document.js")); | ||
| fs.insert(file_path, INITIAL_CONTENT); | ||
|
|
||
| let factory = ServerFactory::new_with_fs(fs.clone()); | ||
| let (service, client) = factory.create().into_inner(); | ||
| let (stream, sink) = client.split(); | ||
| let mut server = Server::new(service); | ||
|
|
||
| let (sender, _) = channel(CHANNEL_BUFFER_SIZE); | ||
| let reader = tokio::spawn(client_handler(stream, sink, sender)); | ||
|
|
||
| server.initialize().await?; | ||
| server.initialized().await?; | ||
|
|
||
| server.open_document(INITIAL_CONTENT).await?; | ||
|
|
||
| // Send didSave with text parameter (as per LSP spec) | ||
| server | ||
| .notify( | ||
| "textDocument/didSave", | ||
| DidSaveTextDocumentParams { | ||
| text_document: TextDocumentIdentifier { | ||
| uri: uri!("document.js"), | ||
| }, | ||
| text: Some(SAVED_CONTENT.to_string()), | ||
| }, | ||
| ) | ||
| .await?; | ||
|
|
||
| // Format the document to verify the content was updated | ||
| let edits: Option<Vec<TextEdit>> = server | ||
| .request( | ||
| "textDocument/formatting", | ||
| "formatting", | ||
| DocumentFormattingParams { | ||
| text_document: TextDocumentIdentifier { | ||
| uri: uri!("document.js"), | ||
| }, | ||
| options: FormattingOptions { | ||
| tab_size: 4, | ||
| insert_spaces: false, | ||
| properties: HashMap::default(), | ||
| trim_trailing_whitespace: None, | ||
| insert_final_newline: None, | ||
| trim_final_newlines: None, | ||
| }, | ||
| work_done_progress_params: WorkDoneProgressParams { | ||
| work_done_token: None, | ||
| }, | ||
| }, | ||
| ) | ||
| .await? | ||
| .context("formatting returned None")?; | ||
|
|
||
| // If content was properly updated to "const b=2;", | ||
| // formatting should add spaces around = and ; | ||
| let edits = edits.context("formatting did not return edits")?; | ||
| assert!(!edits.is_empty(), "Formatting should produce edits for 'const b=2;'"); | ||
|
|
||
| server.close_document().await?; | ||
|
|
||
| server.shutdown().await?; | ||
| reader.abort(); | ||
|
|
||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
Strengthen the test assertion to verify content was actually updated.
The test assertion at line 3963 only checks that formatting produces edits, but both INITIAL_CONTENT and SAVED_CONTENT would produce formatting edits. If the didSave handler fails to update the content, the test would still pass by formatting the old content.
To properly verify that didSave updated the content, check that the formatting edits reference the new variable name 'b' (from SAVED_CONTENT) rather than 'a' (from INITIAL_CONTENT). For example:
// Verify edits are for the saved content (variable 'b', not 'a')
let formatted_text = edits.iter()
.fold(SAVED_CONTENT.to_string(), |text, edit| {
// Apply edits or check edit positions correspond to SAVED_CONTENT structure
text
});
assert!(formatted_text.contains("const b"), "Formatted text should contain variable 'b' from saved content");Alternatively, use get_file_content to verify the document content matches SAVED_CONTENT before formatting, similar to the pattern in change_document_remove_line (lines 2606-2622).
🤖 Prompt for AI Agents
In crates/biome_lsp/src/server.tests.rs around lines 3900 to 3971, the test only
asserts that formatting produced edits but does not confirm the document was
actually updated to SAVED_CONTENT; update the test to verify the saved content
is present before/after formatting. After sending the didSave notification,
either (1) read the file content from the MemoryFileSystem (or use the existing
get_file_content helper pattern around lines 2606-2622) and assert it equals
SAVED_CONTENT, or (2) apply the returned TextEdits to SAVED_CONTENT (or fold
edits starting from SAVED_CONTENT) and assert the resulting text contains "const
b" (or "const b=") to ensure the edits target the saved content rather than the
initial content.
|
On second thought, it’s likely better to update the document from the In parallel, the VS Code extension needs a change to include the saved buffer text in |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
ematipico
left a comment
ematipico
left a comment
There was a problem hiding this comment.
Looks good! Thank you for addressing the issue. Users will be pleased!
3 participants
Summary
Fixed stale diagnostics when the same file is opened in multiple VSCode instances with nested workspaces.
Problem
Closes biomejs/biome-vscode#817
When the same file is opened in multiple VSCode instances (e.g.,
repo/packages/package-a/test.jsonin one instance andpackage-a/test.jsonin another nested workspace):This happens because the LSP server's internal document state could become out of sync with the actual file content on disk when external modifications occur.
Solution
Implemented the
textDocument/didSavehandler to reload file content from disk on save:FileContent::FromServerto force a fresh read from diskDocumentobject with the reloaded contentThis ensures that on every save, the LSP server's state is synchronized with the disk, resolving any stale state issues caused by concurrent edits from multiple editors.
Test Plan
repo/packages/package-a/test.jsonin VSCode instance Apackage-a/test.jsonin VSCode instance B (nested workspace)Docs
No documentation changes required. This is an internal LSP handler fix.