Skip to content

Commit 28447ad

Browse files
committed
fix: prevent duplicate setting error dialogs when global scope is deferred
When a workspace folder resolves, setting errors are now bundled into the deferred global-scope background handler so notifyUserOfSettingErrors is called once with the combined workspace + global errors, not twice. Addresses review feedback from StellaHuang95 on PR microsoft#1456.
1 parent aab1027 commit 28447ad

2 files changed

Lines changed: 47 additions & 6 deletions

File tree

src/features/interpreterSelection.ts

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -376,23 +376,25 @@ export async function applyInitialEnvironmentSelection(
376376

377377
if (workspaceFolderResolved) {
378378
// Defer global scope so it doesn't block post-selection startup.
379+
// Bundle workspace errors into the background handler so we notify once (not twice).
379380
traceInfo('[interpreterSelection] Workspace env resolved, deferring global scope to background');
381+
const deferredWorkspaceErrors = [...allErrors];
380382
resolveGlobalScope()
381383
.then(async (globalErrors) => {
382-
if (globalErrors.length > 0) {
383-
await notifyUserOfSettingErrors(globalErrors);
384+
const combined = [...deferredWorkspaceErrors, ...globalErrors];
385+
if (combined.length > 0) {
386+
await notifyUserOfSettingErrors(combined);
384387
}
385388
})
386389
.catch((err) => traceError(`[interpreterSelection] Background global scope resolution failed: ${err}`));
387390
} else {
388391
// No workspace folder resolved — global scope is the primary fallback, must await.
389392
const globalErrors = await resolveGlobalScope();
390393
allErrors.push(...globalErrors);
391-
}
392394

393-
// Notify user if any settings could not be applied (workspace + global when awaited)
394-
if (allErrors.length > 0) {
395-
await notifyUserOfSettingErrors(allErrors);
395+
if (allErrors.length > 0) {
396+
await notifyUserOfSettingErrors(allErrors);
397+
}
396398
}
397399

398400
// Duration measures blocking time only (excludes deferred global scope).

src/test/features/interpreterSelection.unit.test.ts

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -811,6 +811,15 @@ suite('Interpreter Selection - applyInitialEnvironmentSelection', () => {
811811

812812
const showWarnStub = sandbox.stub(windowApis, 'showWarningMessage').resolves(undefined);
813813

814+
// Wait for the deferred background handler (notification is bundled with global scope)
815+
let resolveGlobalDone!: () => void;
816+
const globalDone = new Promise<void>((resolve) => {
817+
resolveGlobalDone = resolve;
818+
});
819+
mockEnvManagers.setEnvironments.callsFake(async () => {
820+
resolveGlobalDone();
821+
});
822+
814823
await applyInitialEnvironmentSelection(
815824
mockEnvManagers as unknown as EnvironmentManagers,
816825
mockProjectManager as unknown as PythonProjectManager,
@@ -821,6 +830,10 @@ suite('Interpreter Selection - applyInitialEnvironmentSelection', () => {
821830
// Should still set the environment (falls through to auto-discovery)
822831
assert.ok(mockEnvManagers.setEnvironment.called, 'setEnvironment should be called via fallback');
823832

833+
// Wait for background global scope + notification handler
834+
await globalDone;
835+
await new Promise<void>((resolve) => process.nextTick(resolve));
836+
824837
// Should show a warning about the unregistered manager
825838
assert.ok(showWarnStub.called, 'showWarningMessage should be called for unregistered pythonProjects manager');
826839
});
@@ -838,6 +851,15 @@ suite('Interpreter Selection - applyInitialEnvironmentSelection', () => {
838851

839852
const showWarnStub = sandbox.stub(windowApis, 'showWarningMessage').resolves(undefined);
840853

854+
// Wait for the deferred background handler (notification is bundled with global scope)
855+
let resolveGlobalDone!: () => void;
856+
const globalDone = new Promise<void>((resolve) => {
857+
resolveGlobalDone = resolve;
858+
});
859+
mockEnvManagers.setEnvironments.callsFake(async () => {
860+
resolveGlobalDone();
861+
});
862+
841863
await applyInitialEnvironmentSelection(
842864
mockEnvManagers as unknown as EnvironmentManagers,
843865
mockProjectManager as unknown as PythonProjectManager,
@@ -848,6 +870,10 @@ suite('Interpreter Selection - applyInitialEnvironmentSelection', () => {
848870
// Should still set the environment (falls through to auto-discovery)
849871
assert.ok(mockEnvManagers.setEnvironment.called, 'setEnvironment should be called via fallback');
850872

873+
// Wait for background global scope + notification handler
874+
await globalDone;
875+
await new Promise<void>((resolve) => process.nextTick(resolve));
876+
851877
// Should show a warning about the unregistered manager
852878
assert.ok(showWarnStub.called, 'showWarningMessage should be called for unregistered defaultEnvManager');
853879
});
@@ -955,13 +981,26 @@ suite('Interpreter Selection - applyInitialEnvironmentSelection', () => {
955981

956982
const showWarnStub = sandbox.stub(windowApis, 'showWarningMessage').resolves(undefined);
957983

984+
// Wait for the deferred background handler (notification is bundled with global scope)
985+
let resolveGlobalDone!: () => void;
986+
const globalDone = new Promise<void>((resolve) => {
987+
resolveGlobalDone = resolve;
988+
});
989+
mockEnvManagers.setEnvironments.callsFake(async () => {
990+
resolveGlobalDone();
991+
});
992+
958993
await applyInitialEnvironmentSelection(
959994
mockEnvManagers as unknown as EnvironmentManagers,
960995
mockProjectManager as unknown as PythonProjectManager,
961996
mockNativeFinder as unknown as NativePythonFinder,
962997
mockApi as unknown as PythonEnvironmentApi,
963998
);
964999

1000+
// Wait for background global scope + notification handler
1001+
await globalDone;
1002+
await new Promise<void>((resolve) => process.nextTick(resolve));
1003+
9651004
assert.ok(showWarnStub.called, 'showWarningMessage should be called for unresolvable defaultInterpreterPath');
9661005
const warningMessage = showWarnStub.firstCall.args[0] as string;
9671006
assert.ok(warningMessage.includes('/nonexistent/python'), 'Warning message should include the configured path');

0 commit comments

Comments
 (0)