-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Disposable XtabProvider implementation #3606
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Refactors the inline-edits “stateless provider” plumbing to always use XtabProvider directly and ensures it participates in the repo’s disposable pattern (preventing leaked listeners/resources), while removing the old provider-id selection/registration mechanism.
Changes:
- Make
XtabProviderextendDisposableand register owned disposables (e.g.TerminalMonitor). - Remove inline-edits provider-id config/selection plumbing (
InlineEditsProviderId,createNextEditProvider, debug picker UI) and update call sites to instantiateXtabProviderdirectly. - Update simulation/test and library entrypoints to dispose/register the provider appropriately.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| test/simulation/inlineEdit/inlineEditTester.ts | Instantiates XtabProvider directly for simulation runs and disposes it in finally. |
| src/platform/configuration/common/configurationService.ts | Removes InlineEditsProviderId and provider registration helper; adds new Ask-agent config keys. |
| src/lib/node/chatLibMain.ts | Registers XtabProvider in the owning Disposable to ensure cleanup. |
| src/extension/xtab/node/xtabProvider.ts | Converts XtabProvider to Disposable and registers TerminalMonitor. |
| src/extension/inlineEdits/vscode-node/jointInlineCompletionProvider.ts | Removes provider-id observable plumbing; constructs InlineEditModel without provider id. |
| src/extension/inlineEdits/vscode-node/inlineEditProviderFeature.ts | Same as above: drops provider-id selection and updates model/debug component creation. |
| src/extension/inlineEdits/vscode-node/inlineEditModel.ts | Creates XtabProvider directly (and registers it); constructor signature updated. |
| src/extension/inlineEdits/vscode-node/components/inlineEditDebugComponent.ts | Removes the “pick provider” debug command/UI and related provider-id wiring. |
| src/extension/inlineEdits/node/createNextEditProvider.ts | Deleted (provider-id factory no longer used). |
Comments suppressed due to low confidence (1)
src/extension/inlineEdits/vscode-node/inlineEditModel.ts:49
InlineEditModelcreates aNextEditProvider(Disposable) but doesn’t_register(...)it, so whenInlineEditModelis disposed (it’s added toreader.storein the providers), theNextEditProviderand its listeners/caches will leak. Consider registering it (or otherwise disposing it) alongside the other owned disposables.
this._predictor = this._register(this._instantiationService.createInstance(XtabProvider));
const xtabDiffNEntries = this._configurationService.getExperimentBasedConfig(ConfigKey.TeamInternal.InlineEditsXtabDiffNEntries, this._expService);
const xtabHistoryTracker = new NesXtabHistoryTracker(this.workspace, xtabDiffNEntries);
this.nextEditProvider = this._instantiationService.createInstance(NextEditProvider, this.workspace, this._predictor, historyContextProvider, xtabHistoryTracker, this.debugRecorder);
this._triggerer = this._register(this._instantiationService.createInstance(InlineEditTriggerer, this.workspace, this.nextEditProvider));
| @@ -789,7 +789,6 @@ export namespace ConfigKey { | |||
| export const InlineEditsSpeculativeRequests = defineTeamInternalSetting<SpeculativeRequestsEnablement>('chat.advanced.inlineEdits.speculativeRequests', ConfigType.ExperimentBased, SpeculativeRequestsEnablement.Off, SpeculativeRequestsEnablement.VALIDATOR); | |||
| export const InlineEditsExtraDebounceInlineSuggestion = defineTeamInternalSetting<number>('chat.advanced.inlineEdits.extraDebounceInlineSuggestion', ConfigType.ExperimentBased, 0); | |||
| export const InlineEditsDebounceOnSelectionChange = defineTeamInternalSetting<number | undefined>('chat.advanced.inlineEdits.debounceOnSelectionChange', ConfigType.ExperimentBased, undefined); | |||
Copilot
AI
Feb 10, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ConfigKey.TeamInternal.InlineEditsProviderId was removed here, but there are still callers in the repo (e.g. test/simulation/inlineEdit/inlineEdit.stest.ts) that reference it, which will break compilation/tests. Please update those call sites to stop setting this config (or introduce a replacement if provider selection is still needed).
| export const InlineEditsDebounceOnSelectionChange = defineTeamInternalSetting<number | undefined>('chat.advanced.inlineEdits.debounceOnSelectionChange', ConfigType.ExperimentBased, undefined); | |
| export const InlineEditsDebounceOnSelectionChange = defineTeamInternalSetting<number | undefined>('chat.advanced.inlineEdits.debounceOnSelectionChange', ConfigType.ExperimentBased, undefined); | |
| export const InlineEditsProviderId = defineTeamInternalSetting<string | undefined>('chat.advanced.inlineEdits.providerId', ConfigType.ExperimentBased, undefined); |
| /** Additional tools to enable for the Ask agent (additive to base tools) */ | ||
| export const AskAgentAdditionalTools = defineSetting<string[]>('chat.askAgent.additionalTools', ConfigType.Simple, []); | ||
| /** Model override for Ask agent (empty = use default) */ | ||
| export const AskAgentModel = defineSetting<string>('chat.askAgent.model', ConfigType.Simple, ''); | ||
|
|
Copilot
AI
Feb 10, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These new Ask agent settings (chat.askAgent.additionalTools / chat.askAgent.model) appear unrelated to the PR purpose (XtabProvider disposal + inline edits provider ID removal). If they’re not intentionally part of this PR, they should be split out; if they are intentional, ensure the corresponding settings schema / documentation contributions are updated so users can actually configure them.
| public readonly nextEditProvider: NextEditProvider; | ||
|
|
||
| private readonly _predictor: IStatelessNextEditProvider; | ||
| private readonly _predictor: XtabProvider; | ||
| private _triggerer: InlineEditTriggerer; |
Copilot
AI
Feb 10, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typing _predictor as the concrete XtabProvider couples InlineEditModel to a specific implementation. If NextEditProvider only needs IStatelessNextEditProvider, consider keeping _predictor typed to that interface to reduce coupling and make future provider swaps/testing easier.
Copilot Generated Description:Refactor the XtabProvider to extend Disposable, ensuring proper resource management. Remove the provider ID registration logic and update instantiation to use the new disposable pattern. Clean up unused imports and related code.