fix(lsp): reparse open documents after settings load - #11276
Conversation
🦋 Changeset detectedLatest commit: 3ead319 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 |
✅ Organic activityNo automation signals detected in the analyzed events. This is an automated analysis by AgentScan |
WalkthroughWorkspace settings updates now reparse eligible open documents that contain embedded languages. Embedded capability lookup uses the supplied Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/biome_service/src/workspace/server.rs`:
- Around line 643-655: The update loop around open_file_internal must not commit
a stale snapshot after the document collection is released. Revalidate that the
document still has the captured version and content before committing, or
serialize the reparse with change_file so a newer client edit cannot be
overwritten; preserve the newer version and text when they differ. Add a
regression test that interleaves a settings update with change_file and verifies
the newer edit remains effective.
In `@crates/biome_service/src/workspace/server.tests.rs`:
- Around line 218-225: Update format_astro_after_enabling_full_html_support to
assert the complete formatted Astro document, including both --- frontmatter
delimiters, the formatted JavaScript declaration, and the <p>{foo}</p> body; do
not rely only on the body fragment so removal of frontmatter fences fails the
regression test.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a3973a63-7d14-4bcb-a84f-d8fdf9006083
📒 Files selected for processing (3)
.changeset/fix-embedded-lsp-formatting.mdcrates/biome_service/src/workspace/server.rscrates/biome_service/src/workspace/server.tests.rs
| for (path, content, version, document_file_source) in updates { | ||
| self.open_file_internal( | ||
| OpenFileReason::ClientRequest, | ||
| OpenFileParams { | ||
| project_key, | ||
| path: path.into(), | ||
| content: FileContent::FromClient { content, version }, | ||
| document_file_source: Some(document_file_source), | ||
| persist_node_cache: true, | ||
| inline_config: None, | ||
| editor_features: None, | ||
| }, | ||
| )?; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Prevent a stale reparse from overwriting a newer client edit.
These lines reopen a document snapshot after line 641 releases the document collection. If change_file accepts version N + 1 in that interval, this call parses version N and open_file_internal can store the old content and reduce the recorded version through its min merge logic. Formatting and diagnostics can then use stale text.
Make the reparse commit conditional on the same document version and content, or serialise it with client changes. Add a regression test that interleaves a settings update with change_file.
As per coding guidelines, bug fixes require a reproducing regression test.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/biome_service/src/workspace/server.rs` around lines 643 - 655, The
update loop around open_file_internal must not commit a stale snapshot after the
document collection is released. Revalidate that the document still has the
captured version and content before committing, or serialize the reparse with
change_file so a newer client edit cannot be overwritten; preserve the newer
version and text when they differ. Add a regression test that interleaves a
settings update with change_file and verifies the newer edit remains effective.
Source: Coding guidelines
| #[test] | ||
| fn format_astro_after_enabling_full_html_support() { | ||
| assert_format_after_enabling_full_html_support( | ||
| "index.astro", | ||
| "astro", | ||
| "---\nconst foo= 1;\n---\n<p>{foo}</p>\n", | ||
| "<p>{foo}</p>", | ||
| ); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Assert that Astro frontmatter fences remain present.
The helper only checks formatted JavaScript and <p>{foo}</p>. Output with both --- delimiters removed still passes. Assert the frontmatter delimiters and body structure, or assert the complete formatted document.
As per coding guidelines, bug fixes require a reproducing regression test.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/biome_service/src/workspace/server.tests.rs` around lines 218 - 225,
Update format_astro_after_enabling_full_html_support to assert the complete
formatted Astro document, including both --- frontmatter delimiters, the
formatted JavaScript declaration, and the <p>{foo}</p> body; do not rely only on
the body fragment so removal of frontmatter fences fails the regression test.
Source: Coding guidelines
Merging this PR will not alter performance
Comparing 
Footnotes
|
ematipico
left a comment
ematipico
left a comment
There was a problem hiding this comment.
This is an LSP issue, not a workspace issue. With this solution, the CLI is also paying the price for the synchronization solution by reparsing documents that are stored in the workspace. Plus you introduced a side effect now. update_settings now doesn't update settings anymore, it does more.
No, I think the solution belongs to LSP, and the solution doesn't belong to snippets exclusively (maybe it's evident with snippets?). Simply, there's a situation where our internal data are stale because a document is read before we could read the settings.
|
Thinking more, the proposed solution shouldn't even use As for the bug, the reproduction is even easier: open Vue file, then open the configuration and enable full support, switch back to the Vue file. Diagnostics and actions should be out of sync. That's because our parsing strategy changes based on settings, so not even salsa can save us unless we make settings/options a computed product of salsa. For the solution, I would take a different approach: when we finish loading the settings (Biome and extension), before pulling the diagnostics, launch a |
dyc3
changed the title
25ee2fe to
3ead319
Compare
2 participants
Summary
implemented by gpt 5.6 sol
fixes #11275
This is the class of bug that salsa will hopefully help us avoid (when/if we integrate it more).
Here's my clanker's abbreviated explanation of the bug:
The bug required this sequence:
The central problem was that configuration and parsed state could disagree:
Current settings: full HTML support enabled
Stored source: legacy JavaScript/Astro
Stored tree: script/frontmatter only
Before the fix,
update_settingsinstalled the new settings and invalidated the analyzer cache, but left the parsed document untouched. The updated method now reparses affected documents atcrates/biome_service/src/workspace/server.rs:2584.Test Plan
added tests
Docs