google-gemini / google-gemini/gemini-cli

fix(vscode-ide-companion): comma operator in activate() leaks two Disposables (gemini.diff.accept, onDidChangeWorkspaceFolders)

Open
#27,790 13 comments 0 reactions 0 assignees View on GitHub
area/core effort/medium good first issue kind/bug priority/p2 status/bot-triaged
Dominant language
TypeScript
Stars
107k
Forks
14.6k
Avg merge
2d 3h
Merged PRs (30d)
45

Description

### What happened?

In the VS Code companion extension (`packages/vscode-ide-companion/src/extension.ts`), the `activate()` function pushes its Disposables into `context.subscriptions` in two `context.subscriptions.push(...)` calls. In each call, two registrations are wrapped in an extra pair of parentheses, which turns them into a single **comma expression** instead of separate arguments:

```ts
// group 1
(vscode.commands.registerCommand('gemini.diff.accept', ...),
vscode.commands.registerCommand('gemini.diff.cancel', ...)),

// group 2
(vscode.workspace.onDidChangeWorkspaceFolders(...),
vscode.workspace.onDidGrantWorkspaceTrust(...)),
```

A comma expression evaluates both operands but its value is only the **last** one. So both registrations actually run (the command and listener are created), but only the second Disposable of each group (`gemini.diff.cancel`, `onDidGrantWorkspaceTrust`) is passed to `push()` and tracked in `context.subscriptions`. The `gemini.diff.accept` command Disposable and the `onDidChangeWorkspaceFolders` listener Disposable are never added, so they are never disposed on deactivation.

Consequences:
- On deactivation/reload within the same extension host (e.g. an extension update), the `gemini.diff.accept` command stays registered. Re-activation then throws `command 'gemini.diff.accept' already exists`.
- The `onDidChangeWorkspaceFolders` listener is never torn down, so a stale listener keeps firing `ideServer.syncEnvVars()` against an `IDEServer` that has been stopped.

I reproduced the comma-operator behavior with a small standalone script that uses the exact `push(...)` argument expressions from the source against stub `vscode` functions returning tagged Disposables, and counted what actually lands in `context.subscriptions`:

```
$ node repro.mjs
Disposables actually pushed into context.subscriptions:
- onDidCloseTextDocument
- registerTextDocumentContentProvider
- registerCommand:gemini.diff.cancel
- onDidGrantWorkspaceTrust
- registerCommand:gemini-cli.runGeminiCLI
- registerCommand:gemini-cli.showNotices

Expected 8 disposables; got 6
MISSING (registered but never pushed -> leaked):
- registerCommand:gemini.diff.accept
- onDidChangeWorkspaceFolders
```

### What did you expect to happen?

All eight Disposables created in `activate()` should be added to `context.subscriptions` so they are disposed on deactivation. With the two stray parenthesis pairs removed (each registration its own argument), all eight are pushed:

```
$ node repro-fixed.mjs
Pushed 8 disposables:
- onDidCloseTextDocument
- registerTextDocumentContentProvider
- registerCommand:gemini.diff.accept
- registerCommand:gemini.diff.cancel
- onDidChangeWorkspaceFolders
- onDidGrantWorkspaceTrust
- registerCommand:gemini-cli.runGeminiCLI
- registerCommand:gemini-cli.showNotices
```

Suggested direction: remove the extra `(` / `)` wrapping the two register/listener pairs in `activate()` so each Disposable is a separate argument to `push()`. (Happy to send a PR if this is accepted — it's a two-character-region change. The existing `extension.test.ts` mock returns `undefined` from `registerCommand`/listeners, so a regression test would need those mocks to return a tagged Disposable and then assert `context.subscriptions` contains the `gemini.diff.accept` command and the `onDidChangeWorkspaceFolders` listener.)

### Client information

Client Information

- Affected package: `packages/vscode-ide-companion` (VS Code companion extension), version `0.47.0-nightly` line on `main`.
- Affected symbol: `activate()` in `packages/vscode-ide-companion/src/extension.ts`.
- Platform: platform-independent (JavaScript comma-operator semantics).

### Login information

API key

### Anything else we need to know?

I'd like to work on this :)

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.