Add support for CLS to setConfigs mid-runtime#5017
Conversation
|
cc @chrmarti for review |
There was a problem hiding this comment.
Pull request overview
This PR adds a mid-runtime configuration update pathway to the ChatLib “CLS” providers, so configuration overrides can be refreshed without requiring an editor reload.
Changes:
- Added
setConfigs(overrides: Map<string, unknown>)toINESProviderandIInlineCompletionsProvider. - Implemented
setConfigsto apply overrides viaglobalConfigRegistry+IConfigurationService.setConfig. - Extended
OverridableConfigurationServiceto supportsetConfigupdates and emit configuration-change events.
Show a summary per file
| File | Description |
|---|---|
| src/platform/configuration/common/defaultsOnlyConfigurationService.ts | Updates setConfig signature to match the IConfigurationService contract (no-op implementation). |
| src/lib/node/chatLibMain.ts | Adds setConfigs APIs to providers and implements runtime override application + configuration change event propagation. |
Copilot's findings
Comments suppressed due to low confidence (1)
src/lib/node/chatLibMain.ts:802
InlineCompletionsProvider.setConfigs()now updates both IConfigurationService and (conditionally) the InMemoryConfigProvider. There aren’t tests validating that both configuration layers stay in sync after mid-runtime updates (e.g., a setting affecting GhostText behavior). Consider adding a unit test that callssetConfigs()and verifies a config-dependent code path uses the new value without recreating the provider.
setConfigs(overrides: Map<string, unknown>) {
for (const [key, value] of overrides) {
const config = globalConfigRegistry.configs.get(`${CopilotConfigPrefix}.${key}`);
if (config) {
this._configurationService.setConfig(config, value);
}
}
if (this._completionsConfigProvider instanceof InMemoryConfigProvider) {
this._completionsConfigProvider.setCopilotSettings(Object.fromEntries(overrides));
}
}
- Files reviewed: 2/2 changed files
- Comments generated: 2
| setConfigs(overrides: Map<string, unknown>) { | ||
| for (const [key, value] of overrides) { | ||
| const config = globalConfigRegistry.configs.get(`${CopilotConfigPrefix}.${key}`); | ||
| if (config) { | ||
| this._configurationService.setConfig(config, value); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
New runtime configuration update behavior is introduced via setConfigs(), but there are no existing tests covering that calling setConfigs() actually changes subsequent provider behavior (or at least emits configuration change events). Adding a targeted unit test in src/lib/vscode-node/test that updates a known config and asserts the provider observes the new value would help prevent regressions.
This issue also appears on line 792 of the same file.
There was a problem hiding this comment.
I can add tests if necessary :)
|
Merged as microsoft/vscode#308723 |
Adds
setConfigstoINESProviderandIInlineCompletionsProviderto ensure that configs can be added. With this, editor reloads don't need to happen for config to refresh.