Skip to content

Commit da846f3

Browse files
fix: skip shell-integration wait for shells without SI support (fixes #1391) (#1501)
## Summary Fixes #1391 — terminal activation is slow on nushell (and any other shell VS Code doesn't provide shell integration for). When a Python terminal opens in `command` activation mode, `waitForShellIntegration()` is awaited before sending the activation command. For shells VS Code does **not** provide shell integration for — `nu`, `cmd`, `csh`, `tcsh`, `ksh`, `xonsh` — the `onDidChangeTerminalShellIntegration` event never fires, the prompt-pattern heuristic is unreliable, and the wait runs out the full 5 s default timeout (per `getShellIntegrationTimeout()` when `terminal.integrated.shellIntegration.enabled` is true). Once the wait completes, activation falls back to `terminal.sendText` anyway — so the delay is pure overhead and visibly disrupts the user's typing in the new terminal. ## Root cause `waitForShellIntegration()` in [`src/features/terminal/utils.ts`](src/features/terminal/utils.ts) doesn't consult the codebase's existing source of truth for SI-capable shells (`shellIntegrationSupportedShells` in [`src/features/terminal/shells/common/shellUtils.ts`](src/features/terminal/shells/common/shellUtils.ts)). For `nu`, that means it always times out. ## Fix After the existing `terminal.shellIntegration` early-return in `waitForShellIntegration`, identify the shell via `identifyTerminalShell()` and return `false` immediately when the shell type is known and **not** in `shellIntegrationSupportedShells`. Wrapped in `try/catch` so any detection failure (or `'unknown'` result) falls through to the existing `Promise.race` body — strictly additive: best case faster, worst case identical to today. This: - Reuses the codebase's single source of truth (`shellIntegrationSupportedShells`), so adding a new SI-capable shell automatically benefits both this code and `shouldUseProfileActivation`. - Returns the same value (`false`) the function would have eventually returned — semantics unchanged for all callers, only latency differs. - Preserves all existing behavior for supported shells (`pwsh`, `bash`, `gitbash`, `fish`, `zsh`). - Defensive `try/catch` and explicit `'unknown'` check make the change non-regressing even if shell detection fails. For nu users, this turns the ~5 s freeze before `overlay use ...activate.nu` runs into an imperceptible delay. ## Testing ### Manual On `main` with `nu` configured as default profile and `python-envs.terminal.autoActivationType: "command"`: - Open new terminal → ~5 s gap between prompt appearing and activation command running. On this branch, same setup: - Open new terminal → activation command runs immediately after the prompt. - `Python Environments` output channel logs: `Shell 'nu' does not support shell integration; skipping wait.` Verified the fix does **not** alter behavior for `pwsh` / `bash` (they still wait for SI to come up, which they do quickly). ### Automated 8 new unit tests in `Terminal Utils - waitForShellIntegration` cover: - `undefined` terminal → `false` immediately - `terminal.shellIntegration` already set → `true` immediately, no shell detection - `nu` → `false` immediately, no event listeners registered - `cmd` → `false` immediately - `csh` / `tcsh` / `ksh` / `xonsh` → all `false` immediately - `bash` → falls through to event race, resolves `true` when SI event fires - `unknown` shell → falls through to race - `identifyTerminalShell` throws → falls through to race (defensive) ``` npm run lint # clean npm run compile-tests # clean npm run unittest # 992 passing (including the 8 new tests), 2 pending ``` ## Related - #997 — original "Virtual environment activation is slow" tracking issue, where the maintainer explicitly noted: *"In case shell does not support dynamic evals like `nu`, it will fall back to command activation."* This PR makes that command-activation fallback fast. ## Risk Low. Fully covered by tests. The change is gated by `try/catch` and an explicit allow-list, so the worst case (detection fails or returns `'unknown'`) is identical to current behavior.
1 parent 63bc2c2 commit da846f3

2 files changed

Lines changed: 189 additions & 4 deletions

File tree

src/features/terminal/utils.ts

Lines changed: 34 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,13 @@
11
import * as path from 'path';
22
import { Disposable, env, tasks, Terminal, TerminalOptions, Uri } from 'vscode';
33
import { PythonEnvironment, PythonProject, PythonProjectEnvironmentApi, PythonProjectGetterApi } from '../../api';
4+
import { traceVerbose } from '../../common/logging';
45
import { timeout } from '../../common/utils/asyncUtils';
56
import { createSimpleDebounce } from '../../common/utils/debounce';
67
import { onDidChangeTerminalShellIntegration, onDidWriteTerminalData } from '../../common/window.apis';
78
import { getConfiguration, getWorkspaceFolders } from '../../common/workspace.apis';
9+
import { identifyTerminalShell } from '../common/shellDetector';
10+
import { shellIntegrationSupportedShells } from './shells/common/shellUtils';
811

912
export const SHELL_INTEGRATION_TIMEOUT = 500; // 0.5 seconds
1013

@@ -24,10 +27,26 @@ export function getShellIntegrationTimeout(): number {
2427
}
2528

2629
/**
27-
* Three conditions in a Promise.race:
28-
* 1. Timeout based on VS Code's terminal.integrated.shellIntegration.timeout setting
29-
* 2. Shell integration becoming available (window.onDidChangeTerminalShellIntegration event)
30-
* 3. Detection of common prompt patterns in terminal output
30+
* Waits for shell integration to be ready on the given terminal, up to a timeout.
31+
*
32+
* Returns:
33+
* - `true` if shell integration is (or becomes) available.
34+
* - `false` if the timeout is hit, a common prompt pattern is detected, the terminal
35+
* is undefined, or the shell is known not to support shell integration.
36+
*
37+
* Behavior:
38+
* 1. Returns `true` immediately if `terminal.shellIntegration` is already set.
39+
* 2. Returns `false` immediately when the shell type is identified and is NOT in
40+
* {@link shellIntegrationSupportedShells} (e.g. `nu`, `cmd`, `csh`, `tcsh`,
41+
* `ksh`, `xonsh`). VS Code does not provide shell integration for these
42+
* shells, so waiting up to 5s for an event that will never fire only delays
43+
* the fallback `terminal.sendText` activation.
44+
* If shell detection throws or returns `'unknown'`, we fall through to the
45+
* race below to preserve previous behavior.
46+
* 3. Otherwise races three conditions:
47+
* a. Timeout based on VS Code's `terminal.integrated.shellIntegration.timeout` setting.
48+
* b. Shell integration becoming available (`window.onDidChangeTerminalShellIntegration`).
49+
* c. Detection of common prompt patterns in terminal output.
3150
*/
3251
export async function waitForShellIntegration(terminal?: Terminal): Promise<boolean> {
3352
if (!terminal) {
@@ -37,6 +56,17 @@ export async function waitForShellIntegration(terminal?: Terminal): Promise<bool
3756
return true;
3857
}
3958

59+
// Skip the wait for shells that VS Code does not provide shell integration for.
60+
try {
61+
const shellType = identifyTerminalShell(terminal);
62+
if (shellType !== 'unknown' && !shellIntegrationSupportedShells.includes(shellType)) {
63+
traceVerbose(`Shell '${shellType}' does not support shell integration; skipping wait.`);
64+
return false;
65+
}
66+
} catch {
67+
// Detection failed — preserve original behavior by falling through to the race.
68+
}
69+
4070
const timeoutMs = getShellIntegrationTimeout();
4171

4272
const disposables: Disposable[] = [];

src/test/features/terminal/utils.unit.test.ts

Lines changed: 155 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,17 @@
11
import * as assert from 'assert';
22
import * as sinon from 'sinon';
3+
import { Terminal } from 'vscode';
4+
import * as windowApis from '../../../common/window.apis';
35
import * as workspaceApis from '../../../common/workspace.apis';
6+
import * as shellDetector from '../../../features/common/shellDetector';
47
import {
58
ACT_TYPE_COMMAND,
69
ACT_TYPE_OFF,
710
ACT_TYPE_SHELL,
811
AutoActivationType,
912
getAutoActivationType,
1013
shouldActivateInCurrentTerminal,
14+
waitForShellIntegration,
1115
} from '../../../features/terminal/utils';
1216

1317
interface MockWorkspaceConfig {
@@ -545,3 +549,154 @@ suite('Terminal Utils - shouldActivateInCurrentTerminal', () => {
545549
);
546550
});
547551
});
552+
553+
suite('Terminal Utils - waitForShellIntegration', () => {
554+
let mockGetConfiguration: sinon.SinonStub;
555+
let identifyTerminalShellStub: sinon.SinonStub;
556+
let onDidChangeTerminalShellIntegrationStub: sinon.SinonStub;
557+
let onDidWriteTerminalDataStub: sinon.SinonStub;
558+
559+
function setupLongTimeoutConfig() {
560+
// Make the timeout effectively infinite so tests resolve via the listener,
561+
// not the timer. Avoids flakiness while keeping the race code paths exercised.
562+
const config = {
563+
get: sinon.stub(),
564+
inspect: sinon.stub(),
565+
update: sinon.stub(),
566+
};
567+
config.get.withArgs('shellIntegration.timeout').returns(60_000);
568+
config.get.withArgs('shellIntegration.enabled', true).returns(true);
569+
mockGetConfiguration.withArgs('terminal.integrated').returns(config);
570+
}
571+
572+
setup(() => {
573+
mockGetConfiguration = sinon.stub(workspaceApis, 'getConfiguration');
574+
identifyTerminalShellStub = sinon.stub(shellDetector, 'identifyTerminalShell');
575+
onDidChangeTerminalShellIntegrationStub = sinon.stub(windowApis, 'onDidChangeTerminalShellIntegration');
576+
onDidWriteTerminalDataStub = sinon.stub(windowApis, 'onDidWriteTerminalData');
577+
578+
// Default: dispose-only fake event registrations. Tests that need to fire
579+
// events override these via .callsFake.
580+
const fakeDisposable = { dispose: () => undefined };
581+
onDidChangeTerminalShellIntegrationStub.returns(fakeDisposable);
582+
onDidWriteTerminalDataStub.returns(fakeDisposable);
583+
});
584+
585+
teardown(() => {
586+
sinon.restore();
587+
});
588+
589+
test('returns false immediately when terminal is undefined', async () => {
590+
const result = await waitForShellIntegration(undefined);
591+
592+
assert.strictEqual(result, false);
593+
sinon.assert.notCalled(identifyTerminalShellStub);
594+
sinon.assert.notCalled(onDidChangeTerminalShellIntegrationStub);
595+
});
596+
597+
test('returns true immediately when terminal.shellIntegration is already set', async () => {
598+
const terminal = { shellIntegration: {} } as unknown as Terminal;
599+
600+
const result = await waitForShellIntegration(terminal);
601+
602+
assert.strictEqual(result, true);
603+
sinon.assert.notCalled(identifyTerminalShellStub);
604+
sinon.assert.notCalled(onDidChangeTerminalShellIntegrationStub);
605+
});
606+
607+
test('returns false immediately for nu without registering event listeners', async () => {
608+
const terminal = {} as Terminal;
609+
identifyTerminalShellStub.returns('nu');
610+
611+
const result = await waitForShellIntegration(terminal);
612+
613+
assert.strictEqual(result, false);
614+
sinon.assert.calledOnce(identifyTerminalShellStub);
615+
sinon.assert.notCalled(onDidChangeTerminalShellIntegrationStub);
616+
sinon.assert.notCalled(onDidWriteTerminalDataStub);
617+
});
618+
619+
test('returns false immediately for cmd', async () => {
620+
const terminal = {} as Terminal;
621+
identifyTerminalShellStub.returns('cmd');
622+
623+
const result = await waitForShellIntegration(terminal);
624+
625+
assert.strictEqual(result, false);
626+
sinon.assert.notCalled(onDidChangeTerminalShellIntegrationStub);
627+
});
628+
629+
test('returns false immediately for csh / tcsh / ksh / xonsh', async () => {
630+
const unsupported = ['csh', 'tcsh', 'ksh', 'xonsh'];
631+
for (const shell of unsupported) {
632+
identifyTerminalShellStub.resetHistory();
633+
identifyTerminalShellStub.returns(shell);
634+
onDidChangeTerminalShellIntegrationStub.resetHistory();
635+
636+
const result = await waitForShellIntegration({} as Terminal);
637+
638+
assert.strictEqual(result, false, `expected false for shell '${shell}'`);
639+
sinon.assert.notCalled(onDidChangeTerminalShellIntegrationStub);
640+
}
641+
});
642+
643+
test('falls through to event race for bash (supported shell)', async () => {
644+
setupLongTimeoutConfig();
645+
const terminal = {} as Terminal;
646+
identifyTerminalShellStub.returns('bash');
647+
648+
let listenerRef: ((e: { terminal: Terminal }) => void) | undefined;
649+
onDidChangeTerminalShellIntegrationStub.callsFake((listener: (e: { terminal: Terminal }) => void) => {
650+
listenerRef = listener;
651+
return { dispose: () => undefined };
652+
});
653+
654+
const racePromise = waitForShellIntegration(terminal);
655+
// Yield once so the Promise.race body has a chance to register listeners.
656+
await new Promise<void>((r) => setImmediate(r));
657+
assert.ok(listenerRef, 'shell integration listener should be registered');
658+
listenerRef!({ terminal });
659+
660+
const result = await racePromise;
661+
assert.strictEqual(result, true);
662+
sinon.assert.calledOnce(onDidChangeTerminalShellIntegrationStub);
663+
});
664+
665+
test('falls through to event race when shell type is unknown', async () => {
666+
setupLongTimeoutConfig();
667+
const terminal = {} as Terminal;
668+
identifyTerminalShellStub.returns('unknown');
669+
670+
let listenerRef: ((e: { terminal: Terminal }) => void) | undefined;
671+
onDidChangeTerminalShellIntegrationStub.callsFake((listener: (e: { terminal: Terminal }) => void) => {
672+
listenerRef = listener;
673+
return { dispose: () => undefined };
674+
});
675+
676+
const racePromise = waitForShellIntegration(terminal);
677+
await new Promise<void>((r) => setImmediate(r));
678+
listenerRef!({ terminal });
679+
680+
const result = await racePromise;
681+
assert.strictEqual(result, true);
682+
});
683+
684+
test('falls through to event race when identifyTerminalShell throws', async () => {
685+
setupLongTimeoutConfig();
686+
const terminal = {} as Terminal;
687+
identifyTerminalShellStub.throws(new Error('detection failed'));
688+
689+
let listenerRef: ((e: { terminal: Terminal }) => void) | undefined;
690+
onDidChangeTerminalShellIntegrationStub.callsFake((listener: (e: { terminal: Terminal }) => void) => {
691+
listenerRef = listener;
692+
return { dispose: () => undefined };
693+
});
694+
695+
const racePromise = waitForShellIntegration(terminal);
696+
await new Promise<void>((r) => setImmediate(r));
697+
listenerRef!({ terminal });
698+
699+
const result = await racePromise;
700+
assert.strictEqual(result, true, 'should not regress when detection throws');
701+
});
702+
});

0 commit comments

Comments
 (0)