Skip to content

Commit ccd8daf

Browse files
Fire onDidChangeEnvironment for global and single-Uri scopes in set() (#1458)
Part of #1454 ## Bug `CondaEnvManager.set()` only fires `_onDidChangeEnvironment` in the `Uri[]` (multi-project) branch. The `scope === undefined` (global) and `scope instanceof Uri` (single project) branches persist the selection and update internal maps, but never fire the change event. The `venvManager.ts` correctly fires the event in all three branches (lines 413, 444, 477), so this is an inconsistency specific to the conda manager. ## Why it's a bug There are code paths that call `manager.set()` **directly**, bypassing the orchestrator's `setEnvironment()`: 1. **`envCommands.ts:220`** — After creating a conda environment in the "global" (no workspace) context, calls `manager.set(undefined, env)` directly. 2. **`extension.ts:344`** — When removing a project, calls `manager.set(uri, undefined)` directly. 3. **`envManagers.ts:404` and `:434`** — Batch operations that call `manager.set()` directly for individual items. When these code paths run, the orchestrator's `setEnvironment()` is never involved, so it never fires its own `_onDidChangeEnvironmentFiltered` event. The **only** way listeners can be notified is through the manager-level `_onDidChangeEnvironment` — which conda doesn't fire. ### Event chain that breaks ``` manager.set(undefined, env) // called directly, not through orchestrator → conda persists to disk ✅ → conda updates in-memory state ✅ → conda fires _onDidChangeEnvironment ❌ (BUG) envManagers._onDidChangeEnvironment // relayed from manager event → never fires → extension.ts:511 updateViewsAndStatus ❌ → projectView.ts:62 tree refresh ❌ pythonApi._onDidChangeEnvironment // driven by _onDidChangeEnvironmentFiltered → never fires → shellStartupActivationVariablesManager ❌ (terminal activation not updated) → 3rd party extensions ❌ ``` ### What the orchestrator masks When the user selects an environment through the normal UI picker, the orchestrator's `setEnvironment()` calls `manager.set()` and then fires `_onDidChangeEnvironmentFiltered` independently. This masks the missing manager event for that specific flow. But the direct-call paths above are not masked. ## Repro steps **Primary repro (global create flow):** 1. Open VS Code with **no workspace folder open**. 2. Run "Python: Create Environment" from the command palette. 3. Select Conda, choose a name and Python version. 4. The environment is created successfully and `manager.set(undefined, env)` is called at `envCommands.ts:220`. 5. **Bug:** The status bar still shows the old/no environment. The project view doesn't update. Terminal activation doesn't change. The UI appears frozen even though the environment was created and set internally. **Secondary repro (project removal):** 1. Open a multi-root workspace with a Python project using a conda environment. 2. Remove the project from the workspace. 3. `manager.set(uri, undefined)` is called at `extension.ts:344` to clear the environment. 4. **Bug:** Listeners don't know the environment was cleared — stale state remains in views and terminal activation. ## Fix 1. **Global scope (`scope === undefined`)**: Capture the "before" state, update `this.globalEnv`, persist, then fire `_onDidChangeEnvironment` if the environment actually changed. 2. **Single Uri scope (`scope instanceof Uri`)**: Capture the "before" state from the map before updating, then fire `_onDidChangeEnvironment` if the environment actually changed. Both branches now follow the same pattern as the `Uri[]` branch and the `venvManager.ts` implementation. Before: https://github.com/user-attachments/assets/5ec01adc-caeb-4db6-9620-deb0662b92f8 After: https://github.com/user-attachments/assets/08baeceb-e9b7-45d4-9e5c-83792641fda6 ## Why this fix is correct - Events are only fired when the environment actually changes (`before?.envId.id !== checkedEnv?.envId.id`), preventing spurious notifications. - The in-memory `globalEnv` update ensures `get(undefined)` returns the correct value immediately after `set()`. - No change to the `Uri[]` branch, which was already working correctly. - The change is purely additive — existing behavior is preserved, missing behavior is added. - For the normal picker flow (through the orchestrator), the manager event is now fired in addition to the orchestrator's event. This is consistent with venvManager and harmless — listeners that receive both just get a redundant but correct notification. ## Tests added - `condaEnvManager.setEvents.unit.test.ts`: 5 test cases covering: - Global scope fires event - Global scope does not fire event when unchanged - Single Uri scope fires event - Single Uri scope does not fire event when unchanged - Clearing an environment (set to undefined) fires event
1 parent 6ec9f33 commit ccd8daf

2 files changed

Lines changed: 158 additions & 0 deletions

File tree

src/managers/conda/condaEnvManager.ts

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -311,7 +311,12 @@ export class CondaEnvManager implements EnvironmentManager, Disposable {
311311
: undefined;
312312

313313
if (scope === undefined) {
314+
const before = this.globalEnv;
315+
this.globalEnv = checkedEnv;
314316
await setCondaForGlobal(checkedEnv?.environmentPath?.fsPath);
317+
if (before?.envId.id !== checkedEnv?.envId.id) {
318+
this._onDidChangeEnvironment.fire({ uri: undefined, old: before, new: checkedEnv });
319+
}
315320
} else if (scope instanceof Uri) {
316321
const folder = this.api.getPythonProject(scope);
317322
const fsPath = folder?.uri?.fsPath ?? scope.fsPath;
@@ -327,12 +332,16 @@ export class CondaEnvManager implements EnvironmentManager, Disposable {
327332
}
328333

329334
const normalizedFsPath = normalizePath(fsPath);
335+
const before = this.fsPathToEnv.get(normalizedFsPath);
330336
if (checkedEnv) {
331337
this.fsPathToEnv.set(normalizedFsPath, checkedEnv);
332338
} else {
333339
this.fsPathToEnv.delete(normalizedFsPath);
334340
}
335341
await setCondaForWorkspace(fsPath, checkedEnv?.environmentPath.fsPath);
342+
if (before?.envId.id !== checkedEnv?.envId.id) {
343+
this._onDidChangeEnvironment.fire({ uri: scope, old: before, new: checkedEnv });
344+
}
336345
}
337346
} else if (Array.isArray(scope) && scope.every((u) => u instanceof Uri)) {
338347
const projects: PythonProject[] = [];
Lines changed: 149 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,149 @@
1+
/* eslint-disable @typescript-eslint/no-explicit-any */
2+
import assert from 'assert';
3+
import * as sinon from 'sinon';
4+
import { Uri } from 'vscode';
5+
import { DidChangeEnvironmentEventArgs, PythonEnvironment, PythonEnvironmentApi, PythonProject } from '../../../api';
6+
import { normalizePath } from '../../../common/utils/pathUtils';
7+
import { PythonEnvironmentImpl } from '../../../internal.api';
8+
import { CondaEnvManager } from '../../../managers/conda/condaEnvManager';
9+
import * as condaUtils from '../../../managers/conda/condaUtils';
10+
import { NativePythonFinder } from '../../../managers/common/nativePythonFinder';
11+
12+
function makeEnv(name: string, envPath: string, version: string = '3.12.0'): PythonEnvironment {
13+
return new PythonEnvironmentImpl(
14+
{ id: `${name}-test`, managerId: 'ms-python.python:conda' },
15+
{
16+
name,
17+
displayName: `${name} (${version})`,
18+
displayPath: envPath,
19+
version,
20+
environmentPath: Uri.file(envPath),
21+
sysPrefix: envPath,
22+
execInfo: {
23+
run: { executable: 'python' },
24+
},
25+
},
26+
);
27+
}
28+
29+
function createManager(apiOverrides?: Partial<PythonEnvironmentApi>): CondaEnvManager {
30+
const api = {
31+
getPythonProject: sinon.stub().returns(undefined),
32+
...apiOverrides,
33+
} as any as PythonEnvironmentApi;
34+
const manager = new CondaEnvManager(
35+
{} as NativePythonFinder,
36+
api,
37+
{ info: sinon.stub(), error: sinon.stub(), warn: sinon.stub() } as any,
38+
);
39+
(manager as any)._initialized = { completed: true, promise: Promise.resolve() };
40+
(manager as any).collection = [];
41+
return manager;
42+
}
43+
44+
suite('CondaEnvManager.set - onDidChangeEnvironment event firing', () => {
45+
let checkNoPythonStub: sinon.SinonStub;
46+
47+
setup(() => {
48+
sinon.stub(condaUtils, 'setCondaForGlobal').resolves();
49+
sinon.stub(condaUtils, 'setCondaForWorkspace').resolves();
50+
checkNoPythonStub = sinon.stub(condaUtils, 'checkForNoPythonCondaEnvironment');
51+
});
52+
53+
teardown(() => {
54+
sinon.restore();
55+
});
56+
57+
test('set(undefined, env) fires onDidChangeEnvironment for global scope', async () => {
58+
const manager = createManager();
59+
const oldEnv = makeEnv('base', '/miniconda3', '3.11.0');
60+
const newEnv = makeEnv('myenv', '/miniconda3/envs/myenv', '3.12.0');
61+
(manager as any).globalEnv = oldEnv;
62+
checkNoPythonStub.resolves(newEnv);
63+
64+
const events: DidChangeEnvironmentEventArgs[] = [];
65+
manager.onDidChangeEnvironment((e) => events.push(e));
66+
67+
await manager.set(undefined, newEnv);
68+
69+
assert.strictEqual(events.length, 1, 'should fire exactly one event');
70+
assert.strictEqual(events[0].uri, undefined, 'uri should be undefined for global scope');
71+
assert.strictEqual(events[0].old, oldEnv);
72+
assert.strictEqual(events[0].new, newEnv);
73+
});
74+
75+
test('set(undefined, env) does not fire event when env is unchanged', async () => {
76+
const manager = createManager();
77+
const env = makeEnv('base', '/miniconda3', '3.11.0');
78+
(manager as any).globalEnv = env;
79+
checkNoPythonStub.resolves(env);
80+
81+
const events: DidChangeEnvironmentEventArgs[] = [];
82+
manager.onDidChangeEnvironment((e) => events.push(e));
83+
84+
await manager.set(undefined, env);
85+
86+
assert.strictEqual(events.length, 0, 'should not fire event when env is unchanged');
87+
});
88+
89+
test('set(Uri, env) fires onDidChangeEnvironment for single Uri scope', async () => {
90+
const projectUri = Uri.file('/workspace/project');
91+
const project = { uri: projectUri, name: 'project' } as PythonProject;
92+
const manager = createManager({
93+
getPythonProject: sinon.stub().returns(project) as any,
94+
});
95+
const newEnv = makeEnv('myenv', '/miniconda3/envs/myenv', '3.12.0');
96+
checkNoPythonStub.resolves(newEnv);
97+
98+
const events: DidChangeEnvironmentEventArgs[] = [];
99+
manager.onDidChangeEnvironment((e) => events.push(e));
100+
101+
await manager.set(projectUri, newEnv);
102+
103+
assert.strictEqual(events.length, 1, 'should fire exactly one event');
104+
assert.strictEqual(events[0].uri, projectUri);
105+
assert.strictEqual(events[0].old, undefined);
106+
assert.strictEqual(events[0].new, newEnv);
107+
});
108+
109+
test('set(Uri, env) does not fire event when env is unchanged', async () => {
110+
const projectUri = Uri.file('/workspace/project');
111+
const project = { uri: projectUri, name: 'project' } as PythonProject;
112+
const manager = createManager({
113+
getPythonProject: sinon.stub().returns(project) as any,
114+
});
115+
const env = makeEnv('myenv', '/miniconda3/envs/myenv', '3.12.0');
116+
checkNoPythonStub.resolves(env);
117+
118+
// Pre-populate the map with the same env
119+
(manager as any).fsPathToEnv.set(normalizePath(projectUri.fsPath), env);
120+
121+
const events: DidChangeEnvironmentEventArgs[] = [];
122+
manager.onDidChangeEnvironment((e) => events.push(e));
123+
124+
await manager.set(projectUri, env);
125+
126+
assert.strictEqual(events.length, 0, 'should not fire event when env is unchanged');
127+
});
128+
129+
test('set(Uri, undefined) fires event when clearing environment', async () => {
130+
const projectUri = Uri.file('/workspace/project');
131+
const project = { uri: projectUri, name: 'project' } as PythonProject;
132+
const manager = createManager({
133+
getPythonProject: sinon.stub().returns(project) as any,
134+
});
135+
const oldEnv = makeEnv('myenv', '/miniconda3/envs/myenv', '3.12.0');
136+
137+
// Pre-populate the map
138+
(manager as any).fsPathToEnv.set(normalizePath(projectUri.fsPath), oldEnv);
139+
140+
const events: DidChangeEnvironmentEventArgs[] = [];
141+
manager.onDidChangeEnvironment((e) => events.push(e));
142+
143+
await manager.set(projectUri, undefined);
144+
145+
assert.strictEqual(events.length, 1, 'should fire event when clearing');
146+
assert.strictEqual(events[0].old, oldEnv);
147+
assert.strictEqual(events[0].new, undefined);
148+
});
149+
});

0 commit comments

Comments
 (0)