Fix handleEndOfLifetime supersededBy tracking for inline completions#320143
Open
yavanosta wants to merge 1 commit into
Open
Fix handleEndOfLifetime supersededBy tracking for inline completions#320143yavanosta wants to merge 1 commit into
handleEndOfLifetime supersededBy tracking for inline completions#320143yavanosta wants to merge 1 commit into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds per-item pid tracking to inline completion items so end-of-lifetime events can reference the originating provider/id more precisely.
Changes:
- Add
pidtoIdentifiableInlineCompletionto identify the originating provider/id per item. - Populate
pidwhen converting inline completion items on the extension host side. - Update end-of-lifetime event mapping to forward
pidfrom each reason item.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/vs/workbench/api/common/extHostLanguageFeatures.ts | Includes pid on marshalled inline completion items sent to main thread. |
| src/vs/workbench/api/common/extHost.protocol.ts | Extends the cross-thread inline completion item shape with pid. |
| src/vs/workbench/api/browser/mainThreadLanguageFeatures.ts | Adjusts end-of-lifetime event mapping to use per-item pid values. |
Comment on lines
492
to
496
| export interface IdentifiableInlineCompletion extends languages.InlineCompletion { | ||
| pid: number; | ||
| idx: number; | ||
| suggestionId: EditSuggestionId | undefined; | ||
| } |
|
|
||
| if (this._supportsHandleEvents) { | ||
| await this._proxy.$handleInlineCompletionEndOfLifetime(this.handle, completions.pid, item.idx, mapReason(reason, i => ({ pid: completions.pid, idx: i.idx }))); | ||
| await this._proxy.$handleInlineCompletionEndOfLifetime(this.handle, completions.pid, item.idx, mapReason(reason, i => ({ pid: i.pid, idx: i.idx }))); |
| } | ||
|
|
||
| export interface IdentifiableInlineCompletion extends languages.InlineCompletion { | ||
| pid: number; |
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.
Problem
When an inline completion item is superseded by a newer suggestion (e.g., from a subsequent completion request), the extension host provider's
handleEndOfLifetimecallback should receive the superseding completion item viareason.supersededBy. Instead, providers were receiving the old completion item itself (essentially being told the item superseded itself).Root Cause
When the main thread processes
handleEndOfLifetimeinmainThreadLanguageFeatures.ts, it mapsreason.supersededByinto an IPC reference{ pid, idx }. Because individualIdentifiableInlineCompletionitems only trackedidxand notpid,mapReasonincorrectly fell back to usingcompletions.pid:Since
completions.pidrefers to the old completion list being disposed, it paired the old list's PID with the new item's index (0). When this arrived at the extension host, it resolvedthis._references.get(oldPid)?.items[0], incorrectly fetching the old item instead of the new one.Solution
pid: numberto theIdentifiableInlineCompletionDTO interface inextHost.protocol.ts.InlineCompletionAdapter.provideInlineCompletionsinextHostLanguageFeatures.tsto includepidon every completion item DTO sent across the IPC bridge.mapReasoninmainThreadLanguageFeatures.tsto usei.pidinstead ofcompletions.pid.Impact
This change is entirely scoped to the internal Core <-> Extension Host RPC boundary. The public vscode extension API remains 100% unchanged.