fix: Populate diagnostic fields on send codeAction#5341
Open
apatenotre wants to merge 1 commit into
Open
Conversation
Author
|
@microsoft-github-policy-service agree |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
[LSP] Round-trip
Diagnostic.dataand other fields throughtextDocument/codeActionFixes #5342.
LspCodeActionProvider.provideCodeActionswas sendingcontext.diagnosticsentries containing onlyrange,message, andseverity— droppingcode,source,tags,relatedInformation,codeDescription, anddata. The LSP spec requiresdatato be round-tripped (this client even advertisesdataSupport: true), and real-world servers (e.g. ruff) silently drop quickfix actions when it's missing.Approach
DiagnosticsCache(monaco-lsp-client/src/adapters/DiagnosticsCache.ts) keyed by Monaco URI string.LspDiagnosticsFeaturefrom both push (publishDiagnostics) and pull (textDocument/diagnostic) paths, including the related-documents branch. Entries removed onmodel.onWillDisposeModel.LspConnection.diagnosticsCacheso any feature can read it.LspCodeActionProvider.provideCodeActionslooks up each marker's matching cachedDiagnostic(by range + message in LSP coordinates) and forwards the full object. Markers without a cache hit (set by some other source — e.g. ESLint extension setting markers under a different owner) fall back to a synthesized Diagnostic that still includescode/source/tags/relatedInformationfromIMarker.Cache keying is by Monaco URI (
model.uri.toString()) throughout, to keep cleanup trivial viamonaco.editor.onWillDisposeModelregardless of whether diagnostics came in via push or pull.Verification done locally
In a real Next.js + Monaco app using
monaco-editor0.55.1:textDocument/codeAction.context.diagnosticsentries containing onlyrange,message,severity. Ruff returns 2 source-only actions. Monaco lightbulb shows no quickfixes on theF401unused-import diagnostic.code: "F401",source: "Ruff",data: {…}. Ruff returns 4 actions including 2quickfix.*. "Remove unused import: 'os'" and "Disable for this line" appear in the lightbulb.data) is unchanged; its hover, definition, code actions, and diagnostics all still work.Notes
monaco-lsp-client/currently has no test infrastructure (no test script, no framework dep, notest/folder). Happy to add one if you pick a framework; the assertion shape is: stub the LSP transport to capture outgoingtextDocument/codeActionpayloads, push a Diagnostic withdata/code/sourceviapublishDiagnostics, set a marker matching that range, trigger a code-action request, assert the captured outgoing payload'scontext.diagnostics[0]matches the original verbatim.model.uri.toString()(Monaco URI), not LSP URI, so that cleanup viamonaco.editor.onWillDisposeModelis a one-liner. Both producers translate to Monaco URI before caching.