google-gemini / google-gemini/gemini-cli
fix(vscode-ide-companion): comma operator in activate() leaks two Disposables (gemini.diff.accept, onDidChangeWorkspaceFolders)
- 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
Assessment
This issue has not been assessed yet.