From 3391fbfaae6934672f23d010c4c74c63f40d2252 Mon Sep 17 00:00:00 2001 From: Dustin Byrne Date: Fri, 11 Oct 2024 14:21:29 -0400 Subject: [PATCH 1/8] fix: Fix an issue when opening sequence diagrams from Navie on Windows --- src/util.ts | 4 +++- test/unit/util.test.ts | 5 +---- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/src/util.ts b/src/util.ts index 4a63f9bf..00d12047 100644 --- a/src/util.ts +++ b/src/util.ts @@ -419,5 +419,7 @@ export async function parseLocation( ) ); } - return vscode.Uri.parse(location); + // Remove any file:// prefix from the URI. Uri.file() will add it back. Otherwise, it'll be interpreted as + // part of the path, perhaps a drive letter. + return vscode.Uri.file(location.replace(/file:\/\//g, '')); } diff --git a/test/unit/util.test.ts b/test/unit/util.test.ts index 4bbc49b8..3d0187df 100644 --- a/test/unit/util.test.ts +++ b/test/unit/util.test.ts @@ -94,10 +94,7 @@ describe('parseLocation', () => { const result = (await parseLocation('C:\\path\\to\\file.rb')) as URI; expect(result instanceof URI).to.be.true; - - // TODO: This may have different behavior on Windows - // i.e. it may be a URI with a drive letter - expect(result.fsPath).to.equal('\\path\\to\\file.rb'); + expect(result.fsPath).to.equal('c:\\path\\to\\file.rb'); }); it('should parse a location with a line number', async () => { From 9fa0f59563659ec6b0f2a41a35d2645dba4aeacd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafa=C5=82=20Rzepecki?= Date: Thu, 10 Oct 2024 15:06:11 +0200 Subject: [PATCH 2/8] feat: Enable Copilot by default if available --- package.json | 3 +- src/extension.ts | 4 +- src/lib/once.ts | 21 ++++ src/services/chatCompletion.ts | 145 +++++++++++++++------- src/services/rpcProcessService.ts | 2 +- test/unit/services/chatCompletion.test.ts | 1 + 6 files changed, 125 insertions(+), 51 deletions(-) create mode 100644 src/lib/once.ts diff --git a/package.json b/package.json index 7bea6c4a..22c5a51f 100644 --- a/package.json +++ b/package.json @@ -309,7 +309,8 @@ }, "appMap.navie.useVSCodeLM": { "type": "boolean", - "description": "Use VSCode language model API for Navie AI if available.\nRequires a recent VSCode version and (currently) GitHub Copilot extension." + "description": "Use GitHub Copilot as Navie backend if available.\nRequires a recent VSCode version and GitHub Copilot extension.", + "default": true }, "appMap.navie.rpcPort": { "type": "number", diff --git a/src/extension.ts b/src/extension.ts index ae91170e..e64bc017 100644 --- a/src/extension.ts +++ b/src/extension.ts @@ -225,7 +225,7 @@ export async function activate(context: vscode.ExtensionContext): Promise warn(`Chat completion server error: ${e}`)); - instance ??= listening; + if (!instance) { + instance = listening; + ChatCompletion.settingsChanged.fire(); + } } get ready(): Promise { @@ -76,8 +80,9 @@ export default class ChatCompletion implements Disposable { get env(): Record { const pref = ChatCompletion.preferredModel; + if (!pref) return {}; - const modelTokenLimit = pref?.maxInputTokens ?? 3926; + const modelTokenLimit = pref.maxInputTokens; const tokenLimitSetting = ExtensionSettings.navieContextTokenLimit; const tokenLimits = [modelTokenLimit, tokenLimitSetting].filter( (limit) => limit && limit > 0 @@ -86,7 +91,8 @@ export default class ChatCompletion implements Disposable { const env: Record = { OPENAI_API_KEY: this.key, OPENAI_BASE_URL: this.url, - APPMAP_NAVIE_MODEL: pref?.family ?? 'gpt-4o', + APPMAP_NAVIE_MODEL: pref.family, + APPMAP_NAVIE_COMPLETION_BACKEND: 'openai', }; if (tokenLimits.length) { @@ -102,16 +108,16 @@ export default class ChatCompletion implements Disposable { return ChatCompletion.models[0]; } - static async refreshModels(): Promise { + static async refreshModels(): Promise { const previousBest = this.preferredModel?.id; ChatCompletion.models = (await vscode.lm.selectChatModels()).sort( (a, b) => b.maxInputTokens - a.maxInputTokens + b.family.localeCompare(a.family) ); - if (this.preferredModel?.id !== previousBest) this.settingsChanged.fire(); + return this.preferredModel?.id !== previousBest; } - static get instance(): Promise { - if (!instance) return Promise.resolve(undefined); + static get instance(): Promise | undefined { + if (!instance) return undefined; return instance; } @@ -201,67 +207,112 @@ export default class ChatCompletion implements Disposable { } async dispose(): Promise { - if ((await instance) === this) instance = undefined; + if ((await instance) === this) { + instance = undefined; + ChatCompletion.settingsChanged.fire(); + } this.server.close(); } private static settingsChanged = new vscode.EventEmitter(); static onSettingsChanged = ChatCompletion.settingsChanged.event; - static initialize(context: ExtensionContext) { - // TODO: make the messages and handling generic for all LM extensions - const hasLM = 'lm' in vscode && 'selectChatModels' in vscode.lm; - - if (ExtensionSettings.useVsCodeLM && checkAvailability()) - context.subscriptions.push(new ChatCompletion()); + static async initialize(context: ExtensionContext) { + if (await this.checkConfiguration(context)) + context.subscriptions.push( + vscode.lm.onDidChangeChatModels(() => this.checkConfiguration(context)) + ); context.subscriptions.push( - vscode.workspace.onDidChangeConfiguration(async (e) => { - if (e.affectsConfiguration('appMap.navie.useVSCodeLM')) { - const instance = await ChatCompletion.instance; - if (!ExtensionSettings.useVsCodeLM && instance) await instance.dispose(); - else if (ExtensionSettings.useVsCodeLM && checkAvailability()) - context.subscriptions.push(new ChatCompletion()); - this.settingsChanged.fire(); - } - }) + vscode.workspace.onDidChangeConfiguration( + (e) => + e.affectsConfiguration('appMap.navie.useVSCodeLM') && + this.checkConfiguration(context, true) + ) ); + } - if (hasLM) { - ChatCompletion.refreshModels(); - vscode.lm.onDidChangeChatModels( - ChatCompletion.refreshModels, - undefined, - context.subscriptions - ); + static async checkConfiguration(context: ExtensionContext, switched = false): Promise { + // TODO: make the messages and handling generic for all LM extensions + const hasLM = 'lm' in vscode && 'selectChatModels' in vscode.lm; + const wantsLM = ExtensionSettings.useVsCodeLM; + + if (!hasLM) { + if (wantsLM) { + if (switched) + vscode.window.showErrorMessage( + 'AppMap: Copilot backend for Navie is enabled, but the LanguageModel API is not available.\nPlease update your VS Code to the latest version.' + ); + else if (once(context, 'no-lm-api-available')) + vscode.window.showInformationMessage( + 'AppMap: Navie can use Copilot, but the LanguageModel API is not available.\nPlease update your VS Code to the latest version if you want to use it.' + ); + } + return hasLM; + } + once.reset(context, 'no-lm-api-available'); + + if (!wantsLM) { + if (instance) { + await instance.then((i) => i.dispose()); + // must have been switched, so show message + vscode.window.showInformationMessage('AppMap: Copilot backend for Navie is disabled.'); + once.reset(context, 'chat-completion-ready'); + once.reset(context, 'chat-completion-no-models'); + } + return hasLM; } - function checkAvailability() { - if (!hasLM) - vscode.window.showErrorMessage( - 'AppMap: VS Code LM backend for Navie is enabled, but the LanguageModel API is not available.\nPlease update your VS Code to the latest version.' + // now it's hasLM and wantsLM + const changed = await this.refreshModels(); + if (this.preferredModel) { + if (!instance) { + context.subscriptions.push(new this()); + await this.instance; + } else if (changed) ChatCompletion.settingsChanged.fire(); + if (switched) + vscode.window.showInformationMessage( + `AppMap: Copilot backend for Navie is enabled, using model: ${this.preferredModel.name}` + ); + else if (once(context, 'chat-completion-ready')) + vscode.window.showInformationMessage( + `AppMap: Copilot backend for Navie is ready. Model: ${this.preferredModel.name}` ); - else if (!vscode.extensions.getExtension('github.copilot')) { + once.reset(context, 'chat-completion-no-models'); + } else { + if (instance) await instance.then((i) => i.dispose()); + if (switched) vscode.window .showErrorMessage( - 'AppMap: VS Code LM backend for Navie is enabled, but the GitHub Copilot extension is not installed.\nPlease install it from the marketplace and reload the window.', + 'AppMap: Copilot backend for Navie is enabled, but no compatible models were found.\nInstall Copilot to continue.', 'Install Copilot' ) - .then((selection) => { - if (selection === 'Install Copilot') { - const odc = vscode.lm.onDidChangeChatModels(() => { - context.subscriptions.push(new ChatCompletion()); - ChatCompletion.settingsChanged.fire(); - odc.dispose(); - }); + .then( + (selection) => + selection === 'Install Copilot' && vscode.commands.executeCommand( 'workbench.extensions.installExtension', 'github.copilot' - ); - } - }); - } else return true; + ) + ); + else if (once(context, 'chat-completion-no-models')) + vscode.window + .showInformationMessage( + 'AppMap: Navie can use Copilot, but no compatible models were found.\nYou can install Copilot to use this feature.', + 'Install Copilot' + ) + .then( + (selection) => + selection === 'Install Copilot' && + vscode.commands.executeCommand( + 'workbench.extensions.installExtension', + 'github.copilot' + ) + ); + once.reset(context, 'chat-completion-ready'); } + + return hasLM; } } diff --git a/src/services/rpcProcessService.ts b/src/services/rpcProcessService.ts index 6d9f88b5..51794897 100644 --- a/src/services/rpcProcessService.ts +++ b/src/services/rpcProcessService.ts @@ -102,7 +102,7 @@ export default class RpcProcessService implements Disposable { } } - private debouncedRestart(): void { + public debouncedRestart(): void { if (this.restarting) this.scheduleRestart(); else { this.debounce = undefined; diff --git a/test/unit/services/chatCompletion.test.ts b/test/unit/services/chatCompletion.test.ts index 224ecff3..ae786fbc 100644 --- a/test/unit/services/chatCompletion.test.ts +++ b/test/unit/services/chatCompletion.test.ts @@ -65,6 +65,7 @@ describe('ChatCompletion', () => { OPENAI_BASE_URL: chatCompletion.url, APPMAP_NAVIE_TOKEN_LIMIT: '325', APPMAP_NAVIE_MODEL: 'test-family', + APPMAP_NAVIE_COMPLETION_BACKEND: 'openai', }); }); From 21744921b9908e228a861e74b2180a98c5d5af5f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafa=C5=82=20Rzepecki?= Date: Sat, 12 Oct 2024 11:08:03 +0200 Subject: [PATCH 3/8] Don't try to download assets if we're using local dev tools --- src/extension.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/extension.ts b/src/extension.ts index e64bc017..6797574d 100644 --- a/src/extension.ts +++ b/src/extension.ts @@ -228,7 +228,10 @@ export async function activate(context: vscode.ExtensionContext): Promise = (async () => { await dependenciesInstalled; From 9a0810da1e960722b6d00befbc9d9ad4424920df Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafa=C5=82=20Rzepecki?= Date: Sat, 12 Oct 2024 11:08:24 +0200 Subject: [PATCH 4/8] fix: Bind chat completion server to localhost It's only used locally anyway, although security-wise it shouldn't matter much since it needs an authorization key to use anyway. As such, this change is mainly in an effort to avoid tripping up Windows firewall dialog. --- src/services/chatCompletion.ts | 10 +++++++--- test/unit/services/chatCompletion.test.ts | 2 +- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/src/services/chatCompletion.ts b/src/services/chatCompletion.ts index 1da5cf71..db519376 100644 --- a/src/services/chatCompletion.ts +++ b/src/services/chatCompletion.ts @@ -30,7 +30,11 @@ let instance: Promise | undefined; export default class ChatCompletion implements Disposable { public readonly server: Server; - constructor(private portNumber = 0, public readonly key = randomKey()) { + constructor( + private portNumber = 0, + public readonly key = randomKey(), + public readonly host = '127.0.0.1' + ) { this.server = createServer(async (req, res) => { try { await this.handleRequest(req, res); @@ -45,7 +49,7 @@ export default class ChatCompletion implements Disposable { res.end(isNativeError(e) && e.message); } }); - this.server.listen(portNumber); + this.server.listen(portNumber, host); const listening = new Promise((resolve, reject) => this.server .on('listening', () => { @@ -75,7 +79,7 @@ export default class ChatCompletion implements Disposable { } get url(): string { - return `http://localhost:${this.port}/vscode/copilot`; + return `http://${this.host}:${this.port}/vscode/copilot`; } get env(): Record { diff --git a/test/unit/services/chatCompletion.test.ts b/test/unit/services/chatCompletion.test.ts index ae786fbc..d810d474 100644 --- a/test/unit/services/chatCompletion.test.ts +++ b/test/unit/services/chatCompletion.test.ts @@ -125,7 +125,7 @@ describe('ChatCompletion', () => { expect(instance).to.equal(chatCompletion); expect(chatCompletion.port).to.be.above(0); - expect(chatCompletion.url).to.match(/^http:\/\/localhost:\d+\/vscode\/copilot$/); + expect(chatCompletion.url).to.match(/^http:\/\/127.0.0.1:\d+\/vscode\/copilot$/); // make an actual HTTP request to the server const res = await get(chatCompletion.url); From 4df4fcc225fdd883877f884d237302a4399da96d Mon Sep 17 00:00:00 2001 From: Dustin Byrne Date: Wed, 16 Oct 2024 13:22:43 -0400 Subject: [PATCH 5/8] feat: Upgrade @appland/components to v4.39.0 --- package.json | 2 +- yarn.lock | 10 +++++----- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/package.json b/package.json index 22c5a51f..30e6f9e3 100644 --- a/package.json +++ b/package.json @@ -706,7 +706,7 @@ "dependencies": { "@appland/appmap": "^3.129.0", "@appland/client": "^1.14.1", - "@appland/components": "^4.38.3", + "@appland/components": "^4.39.0", "@appland/diagrams": "^1.8.0", "@appland/models": "^2.10.2", "@appland/rpc": "^1.15.0", diff --git a/yarn.lock b/yarn.lock index c8162966..482960d8 100644 --- a/yarn.lock +++ b/yarn.lock @@ -182,9 +182,9 @@ __metadata: languageName: node linkType: hard -"@appland/components@npm:^4.38.3": - version: 4.38.3 - resolution: "@appland/components@npm:4.38.3" +"@appland/components@npm:^4.39.0": + version: 4.39.0 + resolution: "@appland/components@npm:4.39.0" dependencies: "@appland/client": ^1.12.0 "@appland/diagrams": ^1.7.0 @@ -210,7 +210,7 @@ __metadata: sql-formatter: ^4.0.2 vue: ^2.7.14 vuex: ^3.6.0 - checksum: 7b9f5529c75859a1a535cdcefa82c69b783fdfe629305d59bfc163c8fe6e36e6c52cb8bd20cf8561930a641b78c7f04ee43ed77e0e9fe511eabbc615037d7f7d + checksum: db2ce60dfa09e22356499a250d8b385fc2737361ce365be9c8a280023cf4d916ddd079b4bad259050b103ac91c4d774b10b634ccb19d52adab679050e8cde3c3 languageName: node linkType: hard @@ -3054,7 +3054,7 @@ __metadata: dependencies: "@appland/appmap": ^3.129.0 "@appland/client": ^1.14.1 - "@appland/components": ^4.38.3 + "@appland/components": ^4.39.0 "@appland/diagrams": ^1.8.0 "@appland/models": ^2.10.2 "@appland/rpc": ^1.15.0 From 1c1cdb312b65fbe227465fdde8c6dac48edb1d7d Mon Sep 17 00:00:00 2001 From: Dustin Byrne Date: Wed, 16 Oct 2024 13:23:02 -0400 Subject: [PATCH 6/8] feat: Language model provider now updates dynamically --- src/services/navieConfigurationService.ts | 9 ++++ src/services/processWatcher.ts | 7 +++ src/services/rpcProcessService.ts | 50 +++++++++++++++++++ src/webviews/chatSearchWebview.ts | 51 +++++++++++++++++--- test/unit/mock/vscode/workspace.ts | 35 +++++++++++++- test/unit/services/rpcProcessService.test.ts | 43 +++++++++++++++++ web/src/chatSearchView.js | 8 +++ 7 files changed, 194 insertions(+), 9 deletions(-) diff --git a/src/services/navieConfigurationService.ts b/src/services/navieConfigurationService.ts index c3844312..a142ae9c 100644 --- a/src/services/navieConfigurationService.ts +++ b/src/services/navieConfigurationService.ts @@ -39,6 +39,15 @@ export default function navieConfigurationService(context: vscode.ExtensionConte ); } +export async function openAIApiKeyEquals( + extensionContext: vscode.ExtensionContext, + key: string | undefined +): Promise { + const { secrets } = extensionContext; + const storedKey = await secrets.get(OPENAI_API_KEY); + return key === storedKey; +} + export async function setOpenAIApiKey( extensionContext: vscode.ExtensionContext, key: string | undefined diff --git a/src/services/processWatcher.ts b/src/services/processWatcher.ts index 5446c6b7..393ac885 100644 --- a/src/services/processWatcher.ts +++ b/src/services/processWatcher.ts @@ -76,6 +76,7 @@ export class ProcessWatcher implements vscode.Disposable { protected _onError: vscode.EventEmitter = new vscode.EventEmitter(); protected _onAbort: vscode.EventEmitter = new vscode.EventEmitter(); + protected _onRestart: vscode.EventEmitter = new vscode.EventEmitter(); protected shouldRun = false; protected hasAborted = false; @@ -101,6 +102,11 @@ export class ProcessWatcher implements vscode.Disposable { return this._onAbort.event; } + // Fired when the process is restarted. + public get onRestart(): vscode.Event { + return this._onRestart.event; + } + public get id(): ProcessId { return this.options.id; } @@ -169,6 +175,7 @@ export class ProcessWatcher implements vscode.Disposable { } async restart(): Promise { + this._onRestart.fire(); await this.stop(); await this.start(); } diff --git a/src/services/rpcProcessService.ts b/src/services/rpcProcessService.ts index 51794897..035d34ab 100644 --- a/src/services/rpcProcessService.ts +++ b/src/services/rpcProcessService.ts @@ -12,6 +12,7 @@ import assert from 'assert'; import { DEBUG_EXCEPTION, Telemetry } from '../telemetry'; import ErrorCode from '../telemetry/definitions/errorCodes'; import AssetService, { AssetIdentifier } from '../assets/assetService'; +import { openAIApiKeyEquals, setOpenAIApiKey } from './navieConfigurationService'; export type RpcConnect = (port: number) => Client; @@ -23,6 +24,14 @@ export interface RpcProcessServiceState { killProcess(): void; } +interface RpcSettings { + useCopilot?: boolean; + openAIApiKey?: string; + + // If an env var is set to undefined, it will be removed from the env. + env?: Record; +} + export default class RpcProcessService implements Disposable { public readonly _onRpcPortChange = new EventEmitter(); public readonly onRpcPortChange = this._onRpcPortChange.event; @@ -72,6 +81,10 @@ export default class RpcProcessService implements Disposable { ); } + get onRestart(): vscode.Event { + return this.processWatcher.onRestart; + } + // Provides some internal state access, primarily for testing purposes. public get state(): RpcProcessServiceState { return { @@ -225,4 +238,41 @@ export default class RpcProcessService implements Disposable { this._onRpcPortChange.dispose(); this.diposables.forEach((d) => d.dispose()); } + + async updateSettings(settings: RpcSettings): Promise { + let shouldRestart = false; + + if (settings.useCopilot !== undefined) { + const config = vscode.workspace.getConfiguration('appMap.navie'); + const key = 'useVSCodeLM'; + + await config.update(key, settings.useCopilot, true); + + const otherSettings = config.inspect(key); + if (otherSettings?.workspaceValue !== undefined) { + await config.update(key, undefined, undefined); + } + + shouldRestart = true; + } + + if (Object.hasOwnProperty.call(settings, 'openAIApiKey')) { + const sameKey = await openAIApiKeyEquals(this.context, settings.openAIApiKey); + if (!sameKey) { + await setOpenAIApiKey(this.context, settings.openAIApiKey); + shouldRestart = true; + } + } + + if (settings.env) { + const env = vscode.workspace.getConfiguration('appMap').get('commandLineEnvironment', {}); + Object.entries(settings.env).forEach(([k, v]) => { + env[k] = v; + }); + await vscode.workspace.getConfiguration('appMap').update('commandLineEnvironment', env, true); + shouldRestart = true; + } + + if (shouldRestart) this.restart(); + } } diff --git a/src/webviews/chatSearchWebview.ts b/src/webviews/chatSearchWebview.ts index 3202f72e..9fbe9a7d 100644 --- a/src/webviews/chatSearchWebview.ts +++ b/src/webviews/chatSearchWebview.ts @@ -44,7 +44,8 @@ export default class ChatSearchWebview { private constructor( private readonly context: vscode.ExtensionContext, private readonly extensionState: ExtensionState, - private readonly dataService: ChatSearchDataService + private readonly dataService: ChatSearchDataService, + private readonly rpcService: RpcProcessService ) { this.filterStore = new FilterStore(context); this.filterStore.onDidChangeFilters((event) => { @@ -171,6 +172,16 @@ export default class ChatSearchWebview { type: 'update', mostRecentAppMaps: appmaps, }); + }), + this.rpcService.onRpcPortChange(() => { + panel.webview.postMessage({ + type: 'navie-restarted', + }); + }), + this.rpcService.onRestart(() => { + panel.webview.postMessage({ + type: 'navie-restarting', + }); }) ); @@ -248,12 +259,36 @@ export default class ChatSearchWebview { case 'select-llm-option': { const { option } = message; - if (option === 'default') { - await vscode.commands.executeCommand('appmap.clearNavieAiSettings'); - } else if (option === 'own-key') { - await vscode.commands.executeCommand('appmap.openAIApiKey.set'); - } else { - console.error(`Unknown option: ${option}`); + switch (option) { + case 'default': + this.rpcService.updateSettings({ + useCopilot: false, + openAIApiKey: '', + env: { + OPENAI_BASE_URL: undefined, + OPENAI_API_KEY: undefined, + AZURE_OPENAI_API_KEY: undefined, + ANTHROPIC_API_KEY: undefined, + }, + }); + break; + + case 'copilot': + this.rpcService.updateSettings({ useCopilot: true }); + break; + + case 'own-key': + this.rpcService.updateSettings({ + useCopilot: false, + openAIApiKey: await vscode.window.showInputBox({ placeHolder: 'OpenAI API Key' }), + env: { + OPENAI_BASE_URL: undefined, + }, + }); + break; + + default: + console.error(`Unknown option: ${option}`); } break; } @@ -304,6 +339,6 @@ export default class ChatSearchWebview { ): ChatSearchWebview { const dataService = new ChatSearchDataService(rpcService, appmaps); - return new ChatSearchWebview(context, extensionState, dataService); + return new ChatSearchWebview(context, extensionState, dataService, rpcService); } } diff --git a/test/unit/mock/vscode/workspace.ts b/test/unit/mock/vscode/workspace.ts index 6bcb2f92..f34a60d5 100644 --- a/test/unit/mock/vscode/workspace.ts +++ b/test/unit/mock/vscode/workspace.ts @@ -35,9 +35,42 @@ export const EVENTS = { onDidChangeConfiguration: listener(), }; +class Configuration extends Map { + get(key: string, defaultValue?: unknown): unknown { + return super.get(key) ?? defaultValue; + } + + inspect(key: string): { workspaceValue?: unknown } | undefined { + return {}; + } + + update(key: string, value: unknown, target?: unknown): Promise { + if (value === undefined || value === null) { + this.delete(key); + } else { + let filteredValue = value; + if (typeof value === 'object') { + // Undefined/null values are deleted + filteredValue = Object.entries(value).reduce((acc, [k, v]) => { + if (v !== undefined && v !== null) acc[k] = v; + return acc; + }, {}); + } + this.set(key, filteredValue); + } + return Promise.resolve(); + } +} + +const configs = new Map(); + export default { fs, - getConfiguration: () => new Map(), + getConfiguration: (key: string) => { + let config = configs.get(key); + if (!config) configs.set(key, (config = new Configuration())); + return config; + }, workspaceFolders: [], onDidChangeConfiguration: EVENTS.onDidChangeConfiguration, onDidChangeWorkspaceFolders: EVENTS.onDidChangeWorkspaceFolders, diff --git a/test/unit/services/rpcProcessService.test.ts b/test/unit/services/rpcProcessService.test.ts index 745e9f86..90011829 100644 --- a/test/unit/services/rpcProcessService.test.ts +++ b/test/unit/services/rpcProcessService.test.ts @@ -12,6 +12,7 @@ import RpcProcessService, { RpcProcessServiceState } from '../../../src/services import MockExtensionContext from '../../mocks/mockExtensionContext'; import EventEmitter from '../mock/vscode/EventEmitter'; import { waitFor } from '../../waitFor'; +import vscode from '../mock/vscode'; chai.use(chaiAsPromised); @@ -189,4 +190,46 @@ describe('RpcProcessService', () => { await waitFor(`Expecting debounced restart`, () => restartSpy.calledOnce); }); }); + + describe('updateSettings', () => { + it('does not restart if no settings are changed', async () => { + const restartSpy = sinon.spy(rpcService, 'restart'); + await rpcService.updateSettings({}); + expect(restartSpy.called).to.be.false; + }); + + it('restarts if the useCopilot setting is changed', async () => { + const restartSpy = sinon.spy(rpcService, 'restart'); + await rpcService.updateSettings({ useCopilot: true }); + expect(restartSpy.called).to.be.true; + }); + + it('restarts if the openAIApiKey setting is changed', async () => { + const restartSpy = sinon.spy(rpcService, 'restart'); + await rpcService.updateSettings({ openAIApiKey: 'new-key' }); + expect(restartSpy.called).to.be.true; + }); + + it('does not restart if the openAIApiKey setting is the same', async () => { + extensionContext.secrets.store('openai.api_key', 'old-key'); + const restartSpy = sinon.spy(rpcService, 'restart'); + await rpcService.updateSettings({ openAIApiKey: 'old-key' }); + expect(restartSpy.called).to.be.false; + }); + + it('restarts if the env setting is changed', async () => { + const restartSpy = sinon.spy(rpcService, 'restart'); + await rpcService.updateSettings({ env: { foo: 'bar' } }); + expect(restartSpy.called).to.be.true; + }); + + it('properly sets env values', async () => { + vscode.workspace + .getConfiguration('appMap') + .update('commandLineEnvironment', { foo: 'bar', bar: 'baz' }, true); + await rpcService.updateSettings({ env: { foo: undefined, baz: 'qux' } }); + const env = vscode.workspace.getConfiguration('appMap').get('commandLineEnvironment'); + expect(env).to.deep.equal({ bar: 'baz', baz: 'qux' }); + }); + }); }); diff --git a/web/src/chatSearchView.js b/web/src/chatSearchView.js index 26686852..5a756489 100644 --- a/web/src/chatSearchView.js +++ b/web/src/chatSearchView.js @@ -84,6 +84,14 @@ export default function mountChatSearchView() { }); }); + messages + .on('navie-restarting', () => { + app.$refs.ui.onNavieRestarting(); + }) + .on('navie-restarted', () => { + app.$refs.ui.loadNavieConfig(); + }); + app.$on('open-install-instructions', () => { vscode.postMessage({ command: 'open-install-instructions' }); }); From ced7c9ad70694bd0989ed7bde577fb367ba9bb8c Mon Sep 17 00:00:00 2001 From: Dustin Byrne Date: Wed, 16 Oct 2024 16:30:53 -0400 Subject: [PATCH 7/8] Rely on configuration events to restart Navie This change removes a global five second debounce on changes to `appMap.commandLineEnvironment`. However, this behavior is not ideal in settings where the configuration is changed programatically along with other configuration which also restarts the service. A 100ms grace period is added to ensure multiple programmatic changes to configuration can be submitted before the process is restarted without triggering a longer debounce timeout. --- src/extension.ts | 3 ++- src/services/rpcProcessService.ts | 28 +++++++++++--------- test/unit/mock/vscode/workspace.ts | 2 +- test/unit/services/rpcProcessService.test.ts | 27 ++++++++++++------- 4 files changed, 35 insertions(+), 25 deletions(-) diff --git a/src/extension.ts b/src/extension.ts index 6797574d..7bf87c6f 100644 --- a/src/extension.ts +++ b/src/extension.ts @@ -246,7 +246,8 @@ export async function activate(context: vscode.ExtensionContext): Promise { - if (e.affectsConfiguration('appMap.commandLineEnvironment')) rpcService.scheduleRestart(); + if (e.affectsConfiguration('appMap.commandLineEnvironment')) + rpcService.debouncedRestart(); }), vscode.commands.registerCommand('appmap.rpc.restart', async () => { await rpcService.restartServer(); diff --git a/src/services/rpcProcessService.ts b/src/services/rpcProcessService.ts index 035d34ab..d861f8ff 100644 --- a/src/services/rpcProcessService.ts +++ b/src/services/rpcProcessService.ts @@ -43,6 +43,7 @@ export default class RpcProcessService implements Disposable { private diposables: Disposable[] = []; private debounce?: NodeJS.Timeout; private restarting = false; + private restartTimeout?: NodeJS.Timeout; public constructor( private readonly context: ExtensionContext, @@ -105,6 +106,7 @@ export default class RpcProcessService implements Disposable { return this.processWatcher.restart(); } + public restartDelay = 100; public debounceTime = 5000; public scheduleRestart() { @@ -117,11 +119,18 @@ export default class RpcProcessService implements Disposable { public debouncedRestart(): void { if (this.restarting) this.scheduleRestart(); - else { - this.debounce = undefined; - this.restarting = true; - this.restart().finally(() => (this.restarting = false)); - } + else if (!this.restartTimeout) { + // Add a small delay before triggering the restart to allow bulk configuration changes to + // propagate without triggering a longer debounce timeout. + this.restartTimeout = setTimeout(() => { + this.debounce = undefined; + this.restarting = true; + this.restartTimeout = undefined; + this.restart().finally(() => (this.restarting = false)); + }, this.restartDelay).unref(); + } /* else { + There's already a pending restart, so we don't need to do anything. + } */ } protected async pushConfiguration() { @@ -240,8 +249,6 @@ export default class RpcProcessService implements Disposable { } async updateSettings(settings: RpcSettings): Promise { - let shouldRestart = false; - if (settings.useCopilot !== undefined) { const config = vscode.workspace.getConfiguration('appMap.navie'); const key = 'useVSCodeLM'; @@ -252,15 +259,13 @@ export default class RpcProcessService implements Disposable { if (otherSettings?.workspaceValue !== undefined) { await config.update(key, undefined, undefined); } - - shouldRestart = true; } if (Object.hasOwnProperty.call(settings, 'openAIApiKey')) { const sameKey = await openAIApiKeyEquals(this.context, settings.openAIApiKey); if (!sameKey) { await setOpenAIApiKey(this.context, settings.openAIApiKey); - shouldRestart = true; + this.debouncedRestart(); } } @@ -270,9 +275,6 @@ export default class RpcProcessService implements Disposable { env[k] = v; }); await vscode.workspace.getConfiguration('appMap').update('commandLineEnvironment', env, true); - shouldRestart = true; } - - if (shouldRestart) this.restart(); } } diff --git a/test/unit/mock/vscode/workspace.ts b/test/unit/mock/vscode/workspace.ts index f34a60d5..dd7f4110 100644 --- a/test/unit/mock/vscode/workspace.ts +++ b/test/unit/mock/vscode/workspace.ts @@ -35,7 +35,7 @@ export const EVENTS = { onDidChangeConfiguration: listener(), }; -class Configuration extends Map { +export class Configuration extends Map { get(key: string, defaultValue?: unknown): unknown { return super.get(key) ?? defaultValue; } diff --git a/test/unit/services/rpcProcessService.test.ts b/test/unit/services/rpcProcessService.test.ts index 90011829..cbe8706c 100644 --- a/test/unit/services/rpcProcessService.test.ts +++ b/test/unit/services/rpcProcessService.test.ts @@ -13,6 +13,7 @@ import MockExtensionContext from '../../mocks/mockExtensionContext'; import EventEmitter from '../mock/vscode/EventEmitter'; import { waitFor } from '../../waitFor'; import vscode from '../mock/vscode'; +import { Configuration } from '../mock/vscode/workspace'; chai.use(chaiAsPromised); @@ -192,35 +193,41 @@ describe('RpcProcessService', () => { }); describe('updateSettings', () => { - it('does not restart if no settings are changed', async () => { - const restartSpy = sinon.spy(rpcService, 'restart'); + afterEach(() => { + sinon.restore(); + }); + + it('does nothing if no settings are changed', async () => { + const updateSpy = sinon.spy(Configuration.prototype, 'update'); + const restartSpy = sinon.spy(rpcService, 'debouncedRestart'); await rpcService.updateSettings({}); expect(restartSpy.called).to.be.false; + expect(updateSpy.called).to.be.false; }); - it('restarts if the useCopilot setting is changed', async () => { - const restartSpy = sinon.spy(rpcService, 'restart'); + it('updates `useVSCodeLM` if the useCopilot setting is changed', async () => { + const updateSpy = sinon.spy(Configuration.prototype, 'update'); await rpcService.updateSettings({ useCopilot: true }); - expect(restartSpy.called).to.be.true; + expect(updateSpy.calledWith('useVSCodeLM', true)).to.be.true; }); it('restarts if the openAIApiKey setting is changed', async () => { - const restartSpy = sinon.spy(rpcService, 'restart'); + const restartSpy = sinon.spy(rpcService, 'debouncedRestart'); await rpcService.updateSettings({ openAIApiKey: 'new-key' }); expect(restartSpy.called).to.be.true; }); it('does not restart if the openAIApiKey setting is the same', async () => { extensionContext.secrets.store('openai.api_key', 'old-key'); - const restartSpy = sinon.spy(rpcService, 'restart'); + const restartSpy = sinon.spy(rpcService, 'debouncedRestart'); await rpcService.updateSettings({ openAIApiKey: 'old-key' }); expect(restartSpy.called).to.be.false; }); - it('restarts if the env setting is changed', async () => { - const restartSpy = sinon.spy(rpcService, 'restart'); + it('updates `commandLineEnvironment` if the env setting is changed', async () => { + const updateSpy = sinon.spy(Configuration.prototype, 'update'); await rpcService.updateSettings({ env: { foo: 'bar' } }); - expect(restartSpy.called).to.be.true; + expect(updateSpy.calledWith('commandLineEnvironment', { foo: 'bar' })).to.be.true; }); it('properly sets env values', async () => { From 8a697e0c16a2fd362122c0fda2f0b8d5fa8676b9 Mon Sep 17 00:00:00 2001 From: Dustin Byrne Date: Wed, 16 Oct 2024 16:36:18 -0400 Subject: [PATCH 8/8] Rename `onRestart` -> `onBeforeRestart` --- src/services/processWatcher.ts | 8 ++++---- src/services/rpcProcessService.ts | 4 ++-- src/webviews/chatSearchWebview.ts | 2 +- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/services/processWatcher.ts b/src/services/processWatcher.ts index 393ac885..8905e64d 100644 --- a/src/services/processWatcher.ts +++ b/src/services/processWatcher.ts @@ -76,7 +76,7 @@ export class ProcessWatcher implements vscode.Disposable { protected _onError: vscode.EventEmitter = new vscode.EventEmitter(); protected _onAbort: vscode.EventEmitter = new vscode.EventEmitter(); - protected _onRestart: vscode.EventEmitter = new vscode.EventEmitter(); + protected _onBeforeRestart: vscode.EventEmitter = new vscode.EventEmitter(); protected shouldRun = false; protected hasAborted = false; @@ -103,8 +103,8 @@ export class ProcessWatcher implements vscode.Disposable { } // Fired when the process is restarted. - public get onRestart(): vscode.Event { - return this._onRestart.event; + public get onBeforeRestart(): vscode.Event { + return this._onBeforeRestart.event; } public get id(): ProcessId { @@ -175,7 +175,7 @@ export class ProcessWatcher implements vscode.Disposable { } async restart(): Promise { - this._onRestart.fire(); + this._onBeforeRestart.fire(); await this.stop(); await this.start(); } diff --git a/src/services/rpcProcessService.ts b/src/services/rpcProcessService.ts index d861f8ff..d03165e9 100644 --- a/src/services/rpcProcessService.ts +++ b/src/services/rpcProcessService.ts @@ -82,8 +82,8 @@ export default class RpcProcessService implements Disposable { ); } - get onRestart(): vscode.Event { - return this.processWatcher.onRestart; + get onBeforeRestart(): vscode.Event { + return this.processWatcher.onBeforeRestart; } // Provides some internal state access, primarily for testing purposes. diff --git a/src/webviews/chatSearchWebview.ts b/src/webviews/chatSearchWebview.ts index 9fbe9a7d..a122e3fa 100644 --- a/src/webviews/chatSearchWebview.ts +++ b/src/webviews/chatSearchWebview.ts @@ -178,7 +178,7 @@ export default class ChatSearchWebview { type: 'navie-restarted', }); }), - this.rpcService.onRestart(() => { + this.rpcService.onBeforeRestart(() => { panel.webview.postMessage({ type: 'navie-restarting', });