Refactor environment change events to make getEnvironment a pure read and add refreshEnvironment event

eleanorjboyd · Copilot · eleanorjboyd · commit 63f5c6a33e05 · 2026-04-28T20:12:41.000-07:00
Co-authored-by: Copilot &lt;copilot@github.com&gt;
diff --git a/src/extension.ts b/src/extension.ts
@@ -508,13 +508,13 @@ export async function activate(context: ExtensionContext): Promise<PythonEnviron
         window.onDidChangeActiveTextEditor(async () => {
             updateViewsAndStatus(statusBar, workspaceView, managerView, api);
         }),
-        envManagers.onDidChangeEnvironment(async () => {
+        envManagers.onDidChangeManagerEnvironment(async () => {
             updateViewsAndStatus(statusBar, workspaceView, managerView, api);
         }),
         envManagers.onDidChangeEnvironments(async () => {
             updateViewsAndStatus(statusBar, workspaceView, managerView, api);
         }),
-        envManagers.onDidChangeEnvironmentFiltered(async (e) => {
+        envManagers.onDidChangeActiveEnvironment(async (e) => {
             managerView.environmentChanged(e);
             const location = e.uri?.fsPath ?? 'global';
             traceInfo(
diff --git a/src/features/envManagers.ts b/src/features/envManagers.ts
@@ -51,23 +51,42 @@ function generateId(name: string, extensionId?: string): string {
 export class PythonEnvironmentManagers implements EnvironmentManagers {
     private _environmentManagers: Map<string, InternalEnvironmentManager> = new Map();
     private _packageManagers: Map<string, InternalPackageManager> = new Map();
-    private readonly _previousEnvironments = new Map<string, PythonEnvironment | undefined>();
+
+    /**
+     * The last environment announced as "active" for each scope.
+     * Keyed by project URI string or 'global'.
+     *
+     * Used for: (1) change detection before firing onDidChangeActiveEnvironment,
+     *           (2) fallback manager routing when no explicit setting exists.
+     *
+     * Only mutated by setEnvironment() / setEnvironments() / refreshEnvironment().
+     */
+    private readonly _activeSelection = new Map<string, PythonEnvironment | undefined>();
 
     private _onDidChangeEnvironmentManager = new EventEmitter<DidChangeEnvironmentManagerEventArgs>();
     private _onDidChangePackageManager = new EventEmitter<DidChangePackageManagerEventArgs>();
     private _onDidChangeEnvironments = new EventEmitter<InternalDidChangeEnvironmentsEventArgs>();
-    private _onDidChangeEnvironment = new EventEmitter<DidChangeEnvironmentEventArgs>();
-    private _onDidChangeEnvironmentFiltered = new EventEmitter<DidChangeEnvironmentEventArgs>();
+
+    /** Fires when ANY manager reports a selection change, regardless of whether that manager is selected. */
+    private _onDidChangeManagerEnvironment = new EventEmitter<DidChangeEnvironmentEventArgs>();
+
+    /** Fires when the active (selected) environment for a scope actually changes. */
+    private _onDidChangeActiveEnvironment = new EventEmitter<DidChangeEnvironmentEventArgs>();
     private _onDidChangePackages = new EventEmitter<InternalDidChangePackagesEventArgs>();
 
     public onDidChangeEnvironmentManager: Event<DidChangeEnvironmentManagerEventArgs> =
         this._onDidChangeEnvironmentManager.event;
     public onDidChangePackageManager: Event<DidChangePackageManagerEventArgs> = this._onDidChangePackageManager.event;
     public onDidChangeEnvironments: Event<InternalDidChangeEnvironmentsEventArgs> = this._onDidChangeEnvironments.event;
-    public onDidChangeEnvironment: Event<DidChangeEnvironmentEventArgs> = this._onDidChangeEnvironment.event;
+
+    /** Fires when any registered manager reports a change — even if that manager is not the selected one. */
+    public onDidChangeManagerEnvironment: Event<DidChangeEnvironmentEventArgs> =
+        this._onDidChangeManagerEnvironment.event;
     public onDidChangePackages: Event<InternalDidChangePackagesEventArgs> = this._onDidChangePackages.event;
-    public onDidChangeEnvironmentFiltered: Event<DidChangeEnvironmentEventArgs> =
-        this._onDidChangeEnvironmentFiltered.event;
+
+    /** Fires only when the *selected* manager's environment for a scope actually changes. */
+    public onDidChangeActiveEnvironment: Event<DidChangeEnvironmentEventArgs> =
+        this._onDidChangeActiveEnvironment.event;
 
     constructor(private readonly pm: PythonProjectManager) {}
 
@@ -99,7 +118,7 @@ export class PythonEnvironmentManagers implements EnvironmentManagers {
                     return;
                 }
 
-                setImmediate(() => this._onDidChangeEnvironment.fire(e));
+                setImmediate(() => this._onDidChangeManagerEnvironment.fire(e));
             }),
         );
 
@@ -196,7 +215,7 @@ export class PythonEnvironmentManagers implements EnvironmentManagers {
             // Fall back to cached environment's manager if no user-configured settings
             const project = context ? this.pm.get(context) : undefined;
             const key = project ? project.uri.toString() : 'global';
-            const cachedEnv = this._previousEnvironments.get(key);
+            const cachedEnv = this._activeSelection.get(key);
             if (cachedEnv) {
                 const cachedManager = this._environmentManagers.get(cachedEnv.envId.managerId);
                 if (cachedManager) {
@@ -339,11 +358,11 @@ export class PythonEnvironmentManagers implements EnvironmentManagers {
         }
 
         const key = project ? project.uri.toString() : 'global';
-        const oldEnv = this._previousEnvironments.get(key);
+        const oldEnv = this._activeSelection.get(key);
         if (oldEnv?.envId.id !== environment?.envId.id) {
-            this._previousEnvironments.set(key, environment);
+            this._activeSelection.set(key, environment);
             setImmediate(() =>
-                this._onDidChangeEnvironmentFiltered.fire({ uri: project?.uri, new: environment, old: oldEnv }),
+                this._onDidChangeActiveEnvironment.fire({ uri: project?.uri, new: environment, old: oldEnv }),
             );
         }
     }
@@ -393,9 +412,9 @@ export class PythonEnvironmentManagers implements EnvironmentManagers {
 
                     const project = this.pm.get(uri);
                     const key = project ? project.uri.toString() : 'global';
-                    const oldEnv = this._previousEnvironments.get(key);
+                    const oldEnv = this._activeSelection.get(key);
                     if (oldEnv?.envId.id !== environment?.envId.id) {
-                        this._previousEnvironments.set(key, environment);
+                        this._activeSelection.set(key, environment);
                         events.push({ uri: project?.uri, new: environment, old: oldEnv });
                     }
                 });
@@ -411,9 +430,9 @@ export class PythonEnvironmentManagers implements EnvironmentManagers {
                     });
                 }
 
-                const oldEnv = this._previousEnvironments.get('global');
+                const oldEnv = this._activeSelection.get('global');
                 if (oldEnv?.envId.id !== environment?.envId.id) {
-                    this._previousEnvironments.set('global', environment);
+                    this._activeSelection.set('global', environment);
                     events.push({ uri: undefined, new: environment, old: oldEnv });
                 }
             }
@@ -422,7 +441,7 @@ export class PythonEnvironmentManagers implements EnvironmentManagers {
             if (shouldPersistSettings) {
                 await setAllManagerSettings(settings);
             }
-            setImmediate(() => events.forEach((e) => this._onDidChangeEnvironmentFiltered.fire(e)));
+            setImmediate(() => events.forEach((e) => this._onDidChangeActiveEnvironment.fire(e)));
         } else {
             const promises: Promise<void>[] = [];
             const events: DidChangeEnvironmentEventArgs[] = [];
@@ -439,9 +458,9 @@ export class PythonEnvironmentManagers implements EnvironmentManagers {
                             // events. But it ensures that we always get the latest environment at the time of this call.
                             const newEnv = await manager.get(uri);
                             const key = project ? project.uri.toString() : 'global';
-                            const oldEnv = this._previousEnvironments.get(key);
+                            const oldEnv = this._activeSelection.get(key);
                             if (oldEnv?.envId.id !== newEnv?.envId.id) {
-                                this._previousEnvironments.set(key, newEnv);
+                                this._activeSelection.set(key, newEnv);
                                 events.push({ uri: project?.uri, new: newEnv, old: oldEnv });
                             }
                         };
@@ -457,17 +476,17 @@ export class PythonEnvironmentManagers implements EnvironmentManagers {
                         // Always get the new first, then compare with the old. This has minor impact on the ordering of
                         // events. But it ensures that we always get the latest environment at the time of this call.
                         const newEnv = await manager.get(undefined);
-                        const oldEnv = this._previousEnvironments.get('global');
+                        const oldEnv = this._activeSelection.get('global');
                         if (oldEnv?.envId.id !== newEnv?.envId.id) {
-                            this._previousEnvironments.set('global', newEnv);
+                            this._activeSelection.set('global', newEnv);
                             events.push({ uri: undefined, new: newEnv, old: oldEnv });
                         }
                     };
                     promises.push(setAndAddEvent());
                 }
             }
             await Promise.all(promises);
-            setImmediate(() => events.forEach((e) => this._onDidChangeEnvironmentFiltered.fire(e)));
+            setImmediate(() => events.forEach((e) => this._onDidChangeActiveEnvironment.fire(e)));
         }
     }
 
@@ -503,8 +522,9 @@ export class PythonEnvironmentManagers implements EnvironmentManagers {
     /**
      * Gets the current Python environment for the given scope URI or undefined for 'global'.
      *
-     * This method queries the appropriate environment manager for the latest environment for the scope.
-     * It also updates the internal cache and fires an event if the environment has changed since last check.
+     * This is a pure read: it queries the environment manager but does NOT update the
+     * internal selection cache or fire change events. Selection state is only mutated
+     * through setEnvironment() / setEnvironments() / refreshEnvironment().
      *
      * @param scope The scope to get the environment.
      * @returns The current PythonEnvironment for the scope, or undefined if none is set.
@@ -519,21 +539,35 @@ export class PythonEnvironmentManagers implements EnvironmentManagers {
             return undefined;
         }
 
-        const project = scope ? this.pm.get(scope) : undefined;
+        return manager.get(scope);
+    }
+
+    /**
+     * Refreshes the cached environment for the given scope by querying the selected manager.
+     * If the manager's current environment differs from the cache, updates the cache and
+     * fires onDidChangeActiveEnvironment. Use this when a manager signals that its
+     * selection has changed (e.g., via onDidChangeManagerEnvironment).
+     *
+     * Unlike getEnvironment(), this IS a mutation — it updates internal state.
+     * Unlike setEnvironment(), it does NOT call manager.set() or persist to settings.
+     */
+    async refreshEnvironment(scope: GetEnvironmentScope): Promise<void> {
+        const manager = this.getEnvironmentManager(scope);
+        if (!manager) {
+            return;
+        }
 
-        // Always get the new first, then compare with the old. This has minor impact on the ordering of
-        // events. But it ensures that we always get the latest environment at the time of this call.
+        const project = scope ? this.pm.get(scope) : undefined;
         const newEnv = await manager.get(scope);
 
         const key = project ? project.uri.toString() : 'global';
-        const oldEnv = this._previousEnvironments.get(key);
+        const oldEnv = this._activeSelection.get(key);
         if (oldEnv?.envId.id !== newEnv?.envId.id) {
-            this._previousEnvironments.set(key, newEnv);
+            this._activeSelection.set(key, newEnv);
             setImmediate(() =>
-                this._onDidChangeEnvironmentFiltered.fire({ uri: project?.uri, new: newEnv, old: oldEnv }),
+                this._onDidChangeActiveEnvironment.fire({ uri: project?.uri, new: newEnv, old: oldEnv }),
             );
         }
-        return newEnv;
     }
 
     getProjectEnvManagers(uris: Uri[]): InternalEnvironmentManager[] {
diff --git a/src/features/pythonApi.ts b/src/features/pythonApi.ts
@@ -1,35 +1,40 @@
-import { Uri, Disposable, Event, EventEmitter, Terminal, TaskExecution } from 'vscode';
+import { Disposable, Event, EventEmitter, TaskExecution, Terminal, Uri } from 'vscode';
 import {
-    PythonEnvironmentApi,
-    PythonEnvironment,
-    EnvironmentManager,
-    PackageManager,
+    CreateEnvironmentOptions,
+    CreateEnvironmentScope,
     DidChangeEnvironmentEventArgs,
     DidChangeEnvironmentsEventArgs,
+    DidChangeEnvironmentVariablesEventArgs,
+    DidChangePackagesEventArgs,
     DidChangePythonProjectsEventArgs,
+    EnvironmentManager,
+    GetEnvironmentScope,
     GetEnvironmentsScope,
     Package,
-    PythonEnvironmentInfo,
-    PythonProject,
-    RefreshEnvironmentsScope,
-    DidChangePackagesEventArgs,
-    PythonEnvironmentId,
-    CreateEnvironmentScope,
-    SetEnvironmentScope,
-    GetEnvironmentScope,
-    PackageInfo,
     PackageId,
-    PythonProjectCreator,
-    ResolveEnvironmentContext,
+    PackageInfo,
     PackageManagementOptions,
+    PackageManager,
+    PythonBackgroundRunOptions,
+    PythonEnvironment,
+    PythonEnvironmentApi,
+    PythonEnvironmentId,
+    PythonEnvironmentInfo,
     PythonProcess,
+    PythonProject,
+    PythonProjectCreator,
     PythonTaskExecutionOptions,
-    PythonTerminalExecutionOptions,
-    PythonBackgroundRunOptions,
     PythonTerminalCreateOptions,
-    DidChangeEnvironmentVariablesEventArgs,
-    CreateEnvironmentOptions,
+    PythonTerminalExecutionOptions,
+    RefreshEnvironmentsScope,
+    ResolveEnvironmentContext,
+    SetEnvironmentScope,
 } from '../api';
+import { traceInfo } from '../common/logging';
+import { pickEnvironmentManager } from '../common/pickers/managers';
+import { createDeferred } from '../common/utils/deferred';
+import { checkUri } from '../common/utils/pathUtils';
+import { handlePythonPath } from '../common/utils/pythonPath';
 import {
     EnvironmentManagers,
     InternalEnvironmentManager,
@@ -38,17 +43,12 @@ import {
     PythonPackageImpl,
     PythonProjectManager,
 } from '../internal.api';
-import { createDeferred } from '../common/utils/deferred';
-import { traceInfo } from '../common/logging';
-import { pickEnvironmentManager } from '../common/pickers/managers';
-import { handlePythonPath } from '../common/utils/pythonPath';
-import { TerminalManager } from './terminal/terminalManager';
+import { waitForAllEnvManagers, waitForEnvManager, waitForEnvManagerId } from './common/managerReady';
+import { EnvVarManager } from './execution/envVariableManager';
 import { runAsTask } from './execution/runAsTask';
-import { runInTerminal } from './terminal/runInTerminal';
 import { runInBackground } from './execution/runInBackground';
-import { EnvVarManager } from './execution/envVariableManager';
-import { checkUri } from '../common/utils/pathUtils';
-import { waitForAllEnvManagers, waitForEnvManager, waitForEnvManagerId } from './common/managerReady';
+import { runInTerminal } from './terminal/runInTerminal';
+import { TerminalManager } from './terminal/terminalManager';
 
 class PythonEnvironmentApiImpl implements PythonEnvironmentApi {
     private readonly _onDidChangeEnvironments = new EventEmitter<DidChangeEnvironmentsEventArgs>();
@@ -71,7 +71,7 @@ class PythonEnvironmentApiImpl implements PythonEnvironmentApi {
             this._onDidChangePythonProjects,
             this._onDidChangePackages,
             this._onDidChangeEnvironmentVariables,
-            this.envManagers.onDidChangeEnvironmentFiltered((e) => {
+            this.envManagers.onDidChangeActiveEnvironment((e) => {
                 this._onDidChangeEnvironment.fire(e);
                 const location = e.uri?.fsPath ?? 'global';
                 traceInfo(
@@ -92,13 +92,11 @@ class PythonEnvironmentApiImpl implements PythonEnvironmentApi {
             disposables.push(
                 manager.onDidChangeEnvironment((e) => {
                     setImmediate(async () => {
-                        // This will ensure that we use the right manager and only trigger the event
-                        // if the user selected manager decided to change the environment.
-                        // This ensures that if a unselected manager changes environment and raises events
-                        // we don't trigger the Python API event which can cause issues with the consumers.
-                        // This will trigger onDidChangeEnvironmentFiltered event in envManagers, which the Python
-                        // API listens to, and re-triggers the onDidChangeEnvironment event.
-                        await this.envManagers.getEnvironment(e.uri);
+                        // Refresh the central cache for this scope. This ensures that only the
+                        // *selected* manager's changes propagate (refreshEnvironment checks
+                        // getEnvironmentManager(scope) internally). It updates the cache and
+                        // fires onDidChangeActiveEnvironment, which the Python API listens to.
+                        await this.envManagers.refreshEnvironment(e.uri);
                     });
                 }),
             );
diff --git a/src/features/views/projectView.ts b/src/features/views/projectView.ts
@@ -59,7 +59,7 @@ export class ProjectView implements TreeDataProvider<ProjectTreeItem> {
             this.projectManager.onDidChangeProjects(() => {
                 this.debouncedUpdateProject.trigger();
             }),
-            this.envManagers.onDidChangeEnvironment(() => {
+            this.envManagers.onDidChangeManagerEnvironment(() => {
                 this.debouncedUpdateProject.trigger();
             }),
             this.envManagers.onDidChangeEnvironments(() => {
diff --git a/src/internal.api.ts b/src/internal.api.ts
diff --git a/src/test/features/envManagers.unit.test.ts b/src/test/features/envManagers.unit.test.ts
