Refactor environment change events to make getEnvironment a pure read and add refreshEnvironment event#1495
Draft
eleanorjboyd wants to merge 3 commits intomicrosoft:mainfrom
Draft
Conversation
… and add refreshEnvironment event Co-authored-by: Copilot <copilot@github.com>
eleanorjboyd
changed the title
eleanorjboyd
added
the
bug
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses issue #1492 where a read-only getEnvironment() call (triggered by project-structure telemetry) could inadvertently mutate the selected interpreter and emit change events, causing the workspace interpreter to “flip” to a different system Python.
Changes:
- Refactors
PythonEnvironmentManagers.getEnvironment()to be a pure read (no cache mutation / no change events) and introducesrefreshEnvironment()for explicit cache refresh + event emission. - Renames and clarifies environment change events to distinguish “any manager changed” vs “active selection changed”.
- Adds unit coverage to ensure reads don’t fire events and to validate
refreshEnvironment()behavior.
Show a summary per file
| File | Description |
|---|---|
| src/features/envManagers.ts | Splits read vs mutation paths (getEnvironment vs refreshEnvironment), updates internal cache naming, and renames emitted events. |
| src/features/pythonApi.ts | Switches manager change handling to use refreshEnvironment() so only the selected manager’s changes propagate to the Python API event. |
| src/internal.api.ts | Updates internal EnvironmentManagers interface event names and adds refreshEnvironment() to the contract. |
| src/extension.ts | Updates listeners to the renamed events for view/status refresh and internal logging. |
| src/features/views/projectView.ts | Updates project view refresh trigger to use the renamed “any manager changed” event. |
| src/test/features/envManagers.unit.test.ts | Adds unit tests covering “pure read” semantics and refreshEnvironment() change-event behavior. |
Copilot's findings
- Files reviewed: 6/6 changed files
- Comments generated: 2
Comment on lines
+70
to
75
| /** 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>(); |
There was a problem hiding this comment.
The new EventEmitters _onDidChangeManagerEnvironment / _onDidChangeActiveEnvironment are created here, but dispose() only disposes _onDidChangeEnvironmentManager, _onDidChangePackageManager, _onDidChangeEnvironments, and _onDidChangePackages. Please also dispose these new emitters (and any corresponding public events) to avoid leaking listeners when PythonEnvironmentManagers is disposed.
Comment on lines
93
to
100
| 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); | ||
| }); |
There was a problem hiding this comment.
The setImmediate(async () => ...) callback can throw/reject and will become an unhandled promise rejection (nothing awaits it). Wrap the async body in a try/catch (and log via traceError) or use void this.envManagers.refreshEnvironment(e.uri).catch(...) to ensure failures are surfaced without crashing the extension host.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fixes #1492