From 7eae7d2a421435d0428170a360bdf66e65d4c741 Mon Sep 17 00:00:00 2001 From: Stella Huang Date: Fri, 1 May 2026 09:49:49 -0700 Subject: [PATCH] fix: skip shell-integration wait for shells without SI support (#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 #1391 --- src/features/terminal/utils.ts | 38 ++++- src/test/features/terminal/utils.unit.test.ts | 155 ++++++++++++++++++ 2 files changed, 189 insertions(+), 4 deletions(-) diff --git a/src/features/terminal/utils.ts b/src/features/terminal/utils.ts index 54f1a2ca..6cb95b59 100644 --- a/src/features/terminal/utils.ts +++ b/src/features/terminal/utils.ts @@ -1,10 +1,13 @@ import * as path from 'path'; import { Disposable, env, tasks, Terminal, TerminalOptions, Uri } from 'vscode'; import { PythonEnvironment, PythonProject, PythonProjectEnvironmentApi, PythonProjectGetterApi } from '../../api'; +import { traceVerbose } from '../../common/logging'; import { timeout } from '../../common/utils/asyncUtils'; import { createSimpleDebounce } from '../../common/utils/debounce'; import { onDidChangeTerminalShellIntegration, onDidWriteTerminalData } from '../../common/window.apis'; import { getConfiguration, getWorkspaceFolders } from '../../common/workspace.apis'; +import { identifyTerminalShell } from '../common/shellDetector'; +import { shellIntegrationSupportedShells } from './shells/common/shellUtils'; export const SHELL_INTEGRATION_TIMEOUT = 500; // 0.5 seconds @@ -24,10 +27,26 @@ export function getShellIntegrationTimeout(): number { } /** - * Three conditions in a Promise.race: - * 1. Timeout based on VS Code's terminal.integrated.shellIntegration.timeout setting - * 2. Shell integration becoming available (window.onDidChangeTerminalShellIntegration event) - * 3. Detection of common prompt patterns in terminal output + * Waits for shell integration to be ready on the given terminal, up to a timeout. + * + * Returns: + * - `true` if shell integration is (or becomes) available. + * - `false` if the timeout is hit, a common prompt pattern is detected, the terminal + * is undefined, or the shell is known not to support shell integration. + * + * Behavior: + * 1. Returns `true` immediately if `terminal.shellIntegration` is already set. + * 2. Returns `false` immediately when the shell type is identified and is NOT in + * {@link shellIntegrationSupportedShells} (e.g. `nu`, `cmd`, `csh`, `tcsh`, + * `ksh`, `xonsh`). VS Code does not provide shell integration for these + * shells, so waiting up to 5s for an event that will never fire only delays + * the fallback `terminal.sendText` activation. + * If shell detection throws or returns `'unknown'`, we fall through to the + * race below to preserve previous behavior. + * 3. Otherwise races three conditions: + * a. Timeout based on VS Code's `terminal.integrated.shellIntegration.timeout` setting. + * b. Shell integration becoming available (`window.onDidChangeTerminalShellIntegration`). + * c. Detection of common prompt patterns in terminal output. */ export async function waitForShellIntegration(terminal?: Terminal): Promise { if (!terminal) { @@ -37,6 +56,17 @@ export async function waitForShellIntegration(terminal?: Terminal): Promise { ); }); }); + +suite('Terminal Utils - waitForShellIntegration', () => { + let mockGetConfiguration: sinon.SinonStub; + let identifyTerminalShellStub: sinon.SinonStub; + let onDidChangeTerminalShellIntegrationStub: sinon.SinonStub; + let onDidWriteTerminalDataStub: sinon.SinonStub; + + function setupLongTimeoutConfig() { + // Make the timeout effectively infinite so tests resolve via the listener, + // not the timer. Avoids flakiness while keeping the race code paths exercised. + const config = { + get: sinon.stub(), + inspect: sinon.stub(), + update: sinon.stub(), + }; + config.get.withArgs('shellIntegration.timeout').returns(60_000); + config.get.withArgs('shellIntegration.enabled', true).returns(true); + mockGetConfiguration.withArgs('terminal.integrated').returns(config); + } + + setup(() => { + mockGetConfiguration = sinon.stub(workspaceApis, 'getConfiguration'); + identifyTerminalShellStub = sinon.stub(shellDetector, 'identifyTerminalShell'); + onDidChangeTerminalShellIntegrationStub = sinon.stub(windowApis, 'onDidChangeTerminalShellIntegration'); + onDidWriteTerminalDataStub = sinon.stub(windowApis, 'onDidWriteTerminalData'); + + // Default: dispose-only fake event registrations. Tests that need to fire + // events override these via .callsFake. + const fakeDisposable = { dispose: () => undefined }; + onDidChangeTerminalShellIntegrationStub.returns(fakeDisposable); + onDidWriteTerminalDataStub.returns(fakeDisposable); + }); + + teardown(() => { + sinon.restore(); + }); + + test('returns false immediately when terminal is undefined', async () => { + const result = await waitForShellIntegration(undefined); + + assert.strictEqual(result, false); + sinon.assert.notCalled(identifyTerminalShellStub); + sinon.assert.notCalled(onDidChangeTerminalShellIntegrationStub); + }); + + test('returns true immediately when terminal.shellIntegration is already set', async () => { + const terminal = { shellIntegration: {} } as unknown as Terminal; + + const result = await waitForShellIntegration(terminal); + + assert.strictEqual(result, true); + sinon.assert.notCalled(identifyTerminalShellStub); + sinon.assert.notCalled(onDidChangeTerminalShellIntegrationStub); + }); + + test('returns false immediately for nu without registering event listeners', async () => { + const terminal = {} as Terminal; + identifyTerminalShellStub.returns('nu'); + + const result = await waitForShellIntegration(terminal); + + assert.strictEqual(result, false); + sinon.assert.calledOnce(identifyTerminalShellStub); + sinon.assert.notCalled(onDidChangeTerminalShellIntegrationStub); + sinon.assert.notCalled(onDidWriteTerminalDataStub); + }); + + test('returns false immediately for cmd', async () => { + const terminal = {} as Terminal; + identifyTerminalShellStub.returns('cmd'); + + const result = await waitForShellIntegration(terminal); + + assert.strictEqual(result, false); + sinon.assert.notCalled(onDidChangeTerminalShellIntegrationStub); + }); + + test('returns false immediately for csh / tcsh / ksh / xonsh', async () => { + const unsupported = ['csh', 'tcsh', 'ksh', 'xonsh']; + for (const shell of unsupported) { + identifyTerminalShellStub.resetHistory(); + identifyTerminalShellStub.returns(shell); + onDidChangeTerminalShellIntegrationStub.resetHistory(); + + const result = await waitForShellIntegration({} as Terminal); + + assert.strictEqual(result, false, `expected false for shell '${shell}'`); + sinon.assert.notCalled(onDidChangeTerminalShellIntegrationStub); + } + }); + + test('falls through to event race for bash (supported shell)', async () => { + setupLongTimeoutConfig(); + const terminal = {} as Terminal; + identifyTerminalShellStub.returns('bash'); + + let listenerRef: ((e: { terminal: Terminal }) => void) | undefined; + onDidChangeTerminalShellIntegrationStub.callsFake((listener: (e: { terminal: Terminal }) => void) => { + listenerRef = listener; + return { dispose: () => undefined }; + }); + + const racePromise = waitForShellIntegration(terminal); + // Yield once so the Promise.race body has a chance to register listeners. + await new Promise((r) => setImmediate(r)); + assert.ok(listenerRef, 'shell integration listener should be registered'); + listenerRef!({ terminal }); + + const result = await racePromise; + assert.strictEqual(result, true); + sinon.assert.calledOnce(onDidChangeTerminalShellIntegrationStub); + }); + + test('falls through to event race when shell type is unknown', async () => { + setupLongTimeoutConfig(); + const terminal = {} as Terminal; + identifyTerminalShellStub.returns('unknown'); + + let listenerRef: ((e: { terminal: Terminal }) => void) | undefined; + onDidChangeTerminalShellIntegrationStub.callsFake((listener: (e: { terminal: Terminal }) => void) => { + listenerRef = listener; + return { dispose: () => undefined }; + }); + + const racePromise = waitForShellIntegration(terminal); + await new Promise((r) => setImmediate(r)); + listenerRef!({ terminal }); + + const result = await racePromise; + assert.strictEqual(result, true); + }); + + test('falls through to event race when identifyTerminalShell throws', async () => { + setupLongTimeoutConfig(); + const terminal = {} as Terminal; + identifyTerminalShellStub.throws(new Error('detection failed')); + + let listenerRef: ((e: { terminal: Terminal }) => void) | undefined; + onDidChangeTerminalShellIntegrationStub.callsFake((listener: (e: { terminal: Terminal }) => void) => { + listenerRef = listener; + return { dispose: () => undefined }; + }); + + const racePromise = waitForShellIntegration(terminal); + await new Promise((r) => setImmediate(r)); + listenerRef!({ terminal }); + + const result = await racePromise; + assert.strictEqual(result, true, 'should not regress when detection throws'); + }); +});