From 3b461546e19b14c6885e4b9aee87c3e6a20a0442 Mon Sep 17 00:00:00 2001 From: LHMQ878 Date: Wed, 29 Jul 2026 12:39:29 +0800 Subject: [PATCH 1/2] fix(vscode-ide-companion): track both Disposables in each activate() push Two of the eight registrations in `activate()` were wrapped in parentheses together with the registration that follows them: context.subscriptions.push( ... (vscode.commands.registerCommand('gemini.diff.accept', ...), vscode.commands.registerCommand('gemini.diff.cancel', ...)), ); That is a comma expression, not two arguments. Both operands are evaluated, so both commands do get registered, but the expression's *value* is only the last operand -- so only `gemini.diff.cancel` is ever pushed into `context.subscriptions`. The same applies to the second push, where `onDidChangeWorkspaceFolders` is swallowed by `onDidGrantWorkspaceTrust`. Consequences: - `gemini.diff.accept` stays registered after `deactivate()`. Re-activating in the same extension host throws `command 'gemini.diff.accept' already exists`. - The `onDidChangeWorkspaceFolders` listener outlives the `IDEServer` it closes over and keeps calling `syncEnvVars()` on a stopped server. Removing the stray parentheses makes all eight registrations real arguments to `push()`. The existing tests could not catch this: asserting `expect(vscode.workspace.onDidGrantWorkspaceTrust).toHaveBeenCalled()` passes either way, because a comma expression still evaluates both operands. The new tests therefore make each mocked registration return a tagged Disposable and assert on the *contents* of `context.subscriptions`. Fixes #27790 --- .../src/extension.test.ts | 80 +++++++++++++++++++ .../vscode-ide-companion/src/extension.ts | 8 +- 2 files changed, 84 insertions(+), 4 deletions(-) diff --git a/packages/vscode-ide-companion/src/extension.test.ts b/packages/vscode-ide-companion/src/extension.test.ts index 49b1b76d432..52e07c664d2 100644 --- a/packages/vscode-ide-companion/src/extension.test.ts +++ b/packages/vscode-ide-companion/src/extension.test.ts @@ -131,6 +131,86 @@ describe('activate', () => { expect(vscode.workspace.onDidGrantWorkspaceTrust).toHaveBeenCalled(); }); + describe('disposable tracking', () => { + // Each registration returns a Disposable tagged with what produced it, so + // the assertions below can tell *which* Disposables reached + // context.subscriptions rather than merely that the registration ran. + // + // Asserting the call happened is not enough: a comma expression + // `(regA(), regB())` still evaluates both operands, so both mocks are + // called while only regB's Disposable becomes the argument to push(). + // These tests read context.subscriptions for that reason. + const tagged = (tag: string) => ({ tag, dispose: vi.fn() }); + + beforeEach(() => { + vi.mocked(context.globalState.get).mockReturnValue(true); + vi.mocked(vscode.workspace.onDidCloseTextDocument).mockImplementation( + () => tagged('onDidCloseTextDocument') as never, + ); + vi.mocked( + vscode.workspace.registerTextDocumentContentProvider, + ).mockImplementation( + () => tagged('registerTextDocumentContentProvider') as never, + ); + vi.mocked( + vscode.workspace.onDidChangeWorkspaceFolders, + ).mockImplementation( + () => tagged('onDidChangeWorkspaceFolders') as never, + ); + vi.mocked(vscode.workspace.onDidGrantWorkspaceTrust).mockImplementation( + () => tagged('onDidGrantWorkspaceTrust') as never, + ); + vi.mocked(vscode.commands.registerCommand).mockImplementation( + (command: string) => tagged(`command:${command}`) as never, + ); + }); + + const subscribedTags = () => + context.subscriptions.map((d) => (d as unknown as { tag: string }).tag); + + it('tracks every registration made during activation', async () => { + await activate(context); + + expect(subscribedTags()).toEqual( + expect.arrayContaining([ + 'onDidCloseTextDocument', + 'registerTextDocumentContentProvider', + 'command:gemini.diff.accept', + 'command:gemini.diff.cancel', + 'onDidChangeWorkspaceFolders', + 'onDidGrantWorkspaceTrust', + 'command:gemini-cli.runGeminiCLI', + 'command:gemini-cli.showNotices', + ]), + ); + }); + + it('tracks the gemini.diff.accept command so re-activation does not collide', async () => { + // Left untracked, the command stays registered after deactivation and a + // re-activate in the same extension host throws + // "command 'gemini.diff.accept' already exists". + await activate(context); + + expect(subscribedTags()).toContain('command:gemini.diff.accept'); + }); + + it('tracks the onDidChangeWorkspaceFolders listener so it stops on deactivation', async () => { + // Left untracked, the listener outlives the IDEServer it calls and keeps + // firing syncEnvVars() against a stopped server. + await activate(context); + + expect(subscribedTags()).toContain('onDidChangeWorkspaceFolders'); + }); + + it('pushes one Disposable per registration', async () => { + await activate(context); + + const tags = subscribedTags(); + expect(tags).toHaveLength(new Set(tags).size); + expect(tags).toHaveLength(8); + }); + }); + it('should launch the Gemini CLI when the user clicks the button', async () => { const showInformationMessageMock = vi .mocked(vscode.window.showInformationMessage) diff --git a/packages/vscode-ide-companion/src/extension.ts b/packages/vscode-ide-companion/src/extension.ts index 456ec6e872e..a54c9edf258 100644 --- a/packages/vscode-ide-companion/src/extension.ts +++ b/packages/vscode-ide-companion/src/extension.ts @@ -133,7 +133,7 @@ export async function activate(context: vscode.ExtensionContext) { DIFF_SCHEME, diffContentProvider, ), - (vscode.commands.registerCommand( + vscode.commands.registerCommand( 'gemini.diff.accept', (uri?: vscode.Uri) => { const docUri = uri ?? vscode.window.activeTextEditor?.document.uri; @@ -152,7 +152,7 @@ export async function activate(context: vscode.ExtensionContext) { diffManager.cancelDiff(docUri); } }, - )), + ), ); ideServer = new IDEServer(log, diffManager); @@ -174,14 +174,14 @@ export async function activate(context: vscode.ExtensionContext) { } context.subscriptions.push( - (vscode.workspace.onDidChangeWorkspaceFolders(() => { + vscode.workspace.onDidChangeWorkspaceFolders(() => { // eslint-disable-next-line @typescript-eslint/no-floating-promises ideServer.syncEnvVars(); }), vscode.workspace.onDidGrantWorkspaceTrust(() => { // eslint-disable-next-line @typescript-eslint/no-floating-promises ideServer.syncEnvVars(); - })), + }), vscode.commands.registerCommand('gemini-cli.runGeminiCLI', async () => { const workspaceFolders = vscode.workspace.workspaceFolders; if (!workspaceFolders || workspaceFolders.length === 0) { From a9b0df295a18c500f6b999ac68132853f849d28e Mon Sep 17 00:00:00 2001 From: LHMQ878 <72402929@cityu-dg.edu.cn> Date: Wed, 29 Jul 2026 14:01:35 +0800 Subject: [PATCH 2/2] test(vscode-ide-companion): make subscribedTags tolerate untagged Disposables IDEServer.start pushes its own untagged Disposables onto context.subscriptions. The helper assumed every entry was one of the tagged stubs, so the assertions only hold while start() happens to fail early. Filter to the tagged entries instead. --- packages/vscode-ide-companion/src/extension.test.ts | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/packages/vscode-ide-companion/src/extension.test.ts b/packages/vscode-ide-companion/src/extension.test.ts index 52e07c664d2..a14295ac2d0 100644 --- a/packages/vscode-ide-companion/src/extension.test.ts +++ b/packages/vscode-ide-companion/src/extension.test.ts @@ -165,8 +165,13 @@ describe('activate', () => { ); }); + // Only the stubs above carry a `tag`. IDEServer.start also pushes its own + // untagged Disposables onto context.subscriptions, so filter to the tagged + // ones rather than assuming every entry is one of ours. const subscribedTags = () => - context.subscriptions.map((d) => (d as unknown as { tag: string }).tag); + context.subscriptions + .map((d) => (d as unknown as { tag?: unknown } | undefined)?.tag) + .filter((tag): tag is string => typeof tag === 'string'); it('tracks every registration made during activation', async () => { await activate(context);