Fire onDidChangeEnvironment for global and single-Uri scopes in set()#1458
Open
StellaHuang95 wants to merge 1 commit intomicrosoft:mainfrom
Open
Fire onDidChangeEnvironment for global and single-Uri scopes in set()#1458StellaHuang95 wants to merge 1 commit intomicrosoft:mainfrom
onDidChangeEnvironment for global and single-Uri scopes in set()#1458StellaHuang95 wants to merge 1 commit intomicrosoft:mainfrom
Conversation
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.
Part of #1454
Bug
CondaEnvManager.set()only fires_onDidChangeEnvironmentin theUri[](multi-project) branch. Thescope === undefined(global) andscope instanceof Uri(single project) branches persist the selection and update internal maps, but never fire the change event.The
venvManager.tscorrectly 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'ssetEnvironment():envCommands.ts:220— After creating a conda environment in the "global" (no workspace) context, callsmanager.set(undefined, env)directly.extension.ts:344— When removing a project, callsmanager.set(uri, undefined)directly.envManagers.ts:404and:434— Batch operations that callmanager.set()directly for individual items.When these code paths run, the orchestrator's
setEnvironment()is never involved, so it never fires its own_onDidChangeEnvironmentFilteredevent. The only way listeners can be notified is through the manager-level_onDidChangeEnvironment— which conda doesn't fire.Event chain that breaks
What the orchestrator masks
When the user selects an environment through the normal UI picker, the orchestrator's
setEnvironment()callsmanager.set()and then fires_onDidChangeEnvironmentFilteredindependently. 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):
manager.set(undefined, env)is called atenvCommands.ts:220.Secondary repro (project removal):
manager.set(uri, undefined)is called atextension.ts:344to clear the environment.Fix
scope === undefined): Capture the "before" state, updatethis.globalEnv, persist, then fire_onDidChangeEnvironmentif the environment actually changed.scope instanceof Uri): Capture the "before" state from the map before updating, then fire_onDidChangeEnvironmentif the environment actually changed.Both branches now follow the same pattern as the
Uri[]branch and thevenvManager.tsimplementation.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
before?.envId.id !== checkedEnv?.envId.id), preventing spurious notifications.globalEnvupdate ensuresget(undefined)returns the correct value immediately afterset().Uri[]branch, which was already working correctly.Tests added
condaEnvManager.setEvents.unit.test.ts: 5 test cases covering: