Skip to content

Commit 7eae7d2

Browse files
committed
fix: skip shell-integration wait for shells without SI support (microsoft#1391)
waitForShellIntegration() blocks for up to 5s waiting for an onDidChangeTerminalShellIntegration event that never fires for shells VS Code does not provide shell integration for (nu, cmd, csh, tcsh, ksh, xonsh). The wait is followed by a fallback terminal.sendText() activation that doesn't depend on shell integration anyway, so the delay is pure overhead. After the existing terminal.shellIntegration early-return, identify the shell via identifyTerminalShell() and return false immediately when the type is known and not in shellIntegrationSupportedShells. The check is wrapped in try/catch so any detection failure (or an 'unknown' result) falls through to the existing race, preserving prior behavior. For nu users this turns a ~5s freeze before activation into an imperceptible delay. Tests: - 8 new unit tests covering: nu/cmd/csh/tcsh/ksh/xonsh skip immediately, supported shells fall through to the race, unknown falls through, detection throwing falls through, undefined terminal and pre-existing shellIntegration early-returns. Fixes microsoft#1391
1 parent 1eb7b67 commit 7eae7d2

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)