fix(diagnostic): ensure diagnostic request after didOpen#5540
fix(diagnostic): ensure diagnostic request after didOpen#5540fannheyward merged 1 commit intomasterfrom
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5540 +/- ##
==========================================
- Coverage 97.97% 97.91% -0.07%
==========================================
Files 280 280
Lines 27887 27899 +12
Branches 5772 5777 +5
==========================================
- Hits 27321 27316 -5
- Misses 107 113 +6
- Partials 459 470 +11 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Updates the diagnostic pull logic to avoid suppressing the “pull on open” behavior when the client only “knows” about a document via an older version or certain in-flight request states, helping ensure a diagnostic request is issued after didOpen.
Changes:
- Add
tracksSameVersiontoDocumentPullStateTrackerto check whether a tracked pull state matches the current document version. - Add
knowsSameVersiontoDiagnosticRequestorto factor in open request state + version when deciding whether diagnostics are already “known”. - Switch the
didOpendiagnostic trigger guard fromknows(...)toknowsSameVersion(...).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public tracksSameVersion(kind: PullState, document: TextDocument): boolean { | ||
| const key = document.uri.toString() | ||
| const states = kind === PullState.document ? this.documentPullStates : this.workspacePullStates | ||
| const state = states.get(key) | ||
| return state !== undefined && state.pulledVersion === document.version | ||
| } |
There was a problem hiding this comment.
tracksSameVersion/knowsSameVersion build the map key via document.uri.toString(), while the rest of this file consistently uses DocumentOrUri.asKey(...) for openRequests and pull state maps. Using the helper here would avoid subtle key mismatches if URI normalization ever changes and keeps keying logic in one place.
| public knowsSameVersion(kind: PullState, document: TextDocument): boolean { | ||
| const requestState = this.openRequests.get(document.uri.toString()) | ||
| if (requestState === undefined) { | ||
| return this.documentStates.tracksSameVersion(kind, document) | ||
| } | ||
| // A reschedule request is independent of a version so it will trigger | ||
| // on the latest version no matter what. | ||
| if (requestState.state === RequestStateKind.reschedule) { | ||
| return true | ||
| } | ||
| if (requestState.state === RequestStateKind.outDated) { | ||
| return false | ||
| } | ||
| return requestState.version === document.version | ||
| } |
There was a problem hiding this comment.
New version-aware behavior (tracksSameVersion + knowsSameVersion, and the didOpen gating change) isn’t covered by tests. Since there are already tests for DocumentPullStateTracker and DiagnosticFeature under src/__tests__/client/diagnostics.test.ts, it would be good to add cases for: tracked state with same/different pulledVersion, an active request with same/different version, and outDated/reschedule request states, to ensure didOpen triggers (or skips) pulls as intended.
microsoft/vscode-languageserver-node#1723