refactor(shell): Send AnsiOutput when ShowColor is false (#8647)

This commit is contained in:
Gal Zahavi
2025-09-18 13:04:46 -07:00
committed by GitHub
parent 899b6f72cb
commit a34e375193
4 changed files with 120 additions and 41 deletions

View File

@@ -58,6 +58,11 @@ export const ToolMessage: React.FC<ToolMessageProps> = ({
ptyId === activeShellPtyId &&
shellFocused;
const isThisShellFocusable =
(name === SHELL_COMMAND_NAME || name === 'Shell') &&
status === ToolCallStatus.Executing &&
config?.getShouldUseNodePtyShell();
const availableHeight = availableTerminalHeight
? Math.max(
availableTerminalHeight - STATIC_HEIGHT - RESERVED_LINE_COUNT,
@@ -90,9 +95,11 @@ export const ToolMessage: React.FC<ToolMessageProps> = ({
description={description}
emphasis={emphasis}
/>
{isThisShellFocused && (
<Box marginLeft={1}>
<Text color={theme.text.accent}>[Focused]</Text>
{isThisShellFocusable && (
<Box marginLeft={1} flexShrink={0}>
<Text color={theme.text.accent}>
{isThisShellFocused ? '(Focused)' : '(ctrl+f to focus)'}
</Text>
</Box>
)}
{emphasis === 'high' && <TrailingIndicator />}
@@ -118,7 +125,7 @@ export const ToolMessage: React.FC<ToolMessageProps> = ({
</Box>
</MaxSizedBox>
) : typeof resultDisplay === 'object' &&
!Array.isArray(resultDisplay) ? (
'fileDiff' in resultDisplay ? (
<DiffRenderer
diffContent={resultDisplay.fileDiff}
filename={resultDisplay.fileName}

View File

@@ -143,6 +143,8 @@ export const useShellCommandProcessor = (
const activeTheme = themeManager.getActiveTheme();
const shellExecutionConfig = {
...config.getShellExecutionConfig(),
terminalWidth,
terminalHeight,
defaultFg: activeTheme.colors.Foreground,
defaultBg: activeTheme.colors.Background,
};
@@ -158,14 +160,14 @@ export const useShellCommandProcessor = (
if (isBinaryStream) break;
// PTY provides the full screen state, so we just replace.
// Child process provides chunks, so we append.
if (
if (config.getShouldUseNodePtyShell()) {
cumulativeStdout = event.chunk;
shouldUpdate = true;
} else if (
typeof event.chunk === 'string' &&
typeof cumulativeStdout === 'string'
) {
cumulativeStdout += event.chunk;
} else {
cumulativeStdout = event.chunk;
shouldUpdate = true;
}
break;
case 'binary_detected':

View File

@@ -10,6 +10,7 @@ import type { Readable } from 'node:stream';
import { type ChildProcess } from 'node:child_process';
import type { ShellOutputEvent } from './shellExecutionService.js';
import { ShellExecutionService } from './shellExecutionService.js';
import type { AnsiOutput } from '../utils/terminalSerializer.js';
// Hoisted Mocks
const mockPtySpawn = vi.hoisted(() => vi.fn());
@@ -54,6 +55,10 @@ vi.mock('../utils/terminalSerializer.js', () => ({
serializeTerminalToObject: mockSerializeTerminalToObject,
}));
const mockProcessKill = vi
.spyOn(process, 'kill')
.mockImplementation(() => true);
const shellExecutionConfig = {
terminalWidth: 80,
terminalHeight: 24,
@@ -61,9 +66,25 @@ const shellExecutionConfig = {
showColor: false,
};
const mockProcessKill = vi
.spyOn(process, 'kill')
.mockImplementation(() => true);
const createExpectedAnsiOutput = (text: string | string[]): AnsiOutput => {
const lines = Array.isArray(text) ? text : text.split('\n');
const expected: AnsiOutput = Array.from(
{ length: shellExecutionConfig.terminalHeight },
(_, i) => [
{
text: expect.stringMatching((lines[i] || '').trim()),
bold: false,
italic: false,
underline: false,
dim: false,
inverse: false,
fg: '',
bg: '',
},
],
);
return expected;
};
describe('ShellExecutionService', () => {
let mockPtyProcess: EventEmitter & {
@@ -77,6 +98,11 @@ describe('ShellExecutionService', () => {
let mockHeadlessTerminal: {
resize: Mock;
scrollLines: Mock;
buffer: {
active: {
viewportY: number;
};
};
};
let onOutputEventMock: Mock<(event: ShellOutputEvent) => void>;
@@ -110,6 +136,11 @@ describe('ShellExecutionService', () => {
mockHeadlessTerminal = {
resize: vi.fn(),
scrollLines: vi.fn(),
buffer: {
active: {
viewportY: 0,
},
},
};
mockPtySpawn.mockReturnValue(mockPtyProcess);
@@ -161,7 +192,7 @@ describe('ShellExecutionService', () => {
expect(onOutputEventMock).toHaveBeenCalledWith({
type: 'data',
chunk: 'file1.txt',
chunk: createExpectedAnsiOutput('file1.txt'),
});
});
@@ -175,7 +206,7 @@ describe('ShellExecutionService', () => {
expect(onOutputEventMock).toHaveBeenCalledWith(
expect.objectContaining({
type: 'data',
chunk: expect.stringContaining('aredword'),
chunk: createExpectedAnsiOutput('aredword'),
}),
);
});
@@ -197,7 +228,7 @@ describe('ShellExecutionService', () => {
expect(onOutputEventMock).toHaveBeenCalledWith(
expect.objectContaining({
chunk: expect.stringMatching(/^\s*$/),
chunk: createExpectedAnsiOutput(''),
}),
);
});
@@ -438,17 +469,44 @@ describe('ShellExecutionService', () => {
);
});
it('should call onOutputEvent with plain string when showColor is false', async () => {
await simulateExecution('ls --color=auto', (pty) => {
pty.onData.mock.calls[0][0]('a\u001b[31mred\u001b[0mword');
pty.onExit.mock.calls[0][0]({ exitCode: 0, signal: null });
});
it('should call onOutputEvent with AnsiOutput when showColor is false', async () => {
await simulateExecution(
'ls --color=auto',
(pty) => {
pty.onData.mock.calls[0][0]('a\u001b[31mred\u001b[0mword');
pty.onExit.mock.calls[0][0]({ exitCode: 0, signal: null });
},
{ ...shellExecutionConfig, showColor: false },
);
const expected = createExpectedAnsiOutput('aredword');
expect(mockSerializeTerminalToObject).not.toHaveBeenCalled();
expect(onOutputEventMock).toHaveBeenCalledWith(
expect.objectContaining({
type: 'data',
chunk: 'aredword',
chunk: expected,
}),
);
});
it('should handle multi-line output correctly when showColor is false', async () => {
await simulateExecution(
'ls --color=auto',
(pty) => {
pty.onData.mock.calls[0][0](
'line 1\n\u001b[32mline 2\u001b[0m\nline 3',
);
pty.onExit.mock.calls[0][0]({ exitCode: 0, signal: null });
},
{ ...shellExecutionConfig, showColor: false },
);
const expected = createExpectedAnsiOutput(['line 1', 'line 2', 'line 3']);
expect(onOutputEventMock).toHaveBeenCalledWith(
expect.objectContaining({
type: 'data',
chunk: expected,
}),
);
});
@@ -541,7 +599,7 @@ describe('ShellExecutionService child_process fallback', () => {
expect(onOutputEventMock).toHaveBeenCalledWith(
expect.objectContaining({
type: 'data',
chunk: expect.stringContaining('aredword'),
chunk: 'aredword',
}),
);
});

View File

@@ -85,17 +85,6 @@ interface ActivePty {
headlessTerminal: pkg.Terminal;
}
const getVisibleText = (terminal: pkg.Terminal): string => {
const buffer = terminal.buffer.active;
const lines: string[] = [];
for (let i = 0; i < terminal.rows; i++) {
const line = buffer.getLine(buffer.viewportY + i);
const lineContent = line ? line.translateToString(true) : '';
lines.push(lineContent);
}
return lines.join('\n').trimEnd();
};
const getFullBufferText = (terminal: pkg.Terminal): string => {
const buffer = terminal.buffer.active;
const lines: string[] = [];
@@ -379,6 +368,7 @@ export class ShellExecutionService {
cols,
rows,
});
headlessTerminal.scrollToTop();
this.activePtys.set(ptyProcess.pid, { ptyProcess, headlessTerminal });
@@ -404,14 +394,33 @@ export class ShellExecutionService {
if (!isStreamingRawContent) {
return;
}
const newOutput = shellExecutionConfig.showColor
? serializeTerminalToObject(headlessTerminal, {
defaultFg: shellExecutionConfig.defaultFg,
defaultBg: shellExecutionConfig.defaultBg,
})
: getVisibleText(headlessTerminal);
// console.log(newOutput)
let newOutput: AnsiOutput;
if (shellExecutionConfig.showColor) {
newOutput = serializeTerminalToObject(headlessTerminal, {
defaultFg: shellExecutionConfig.defaultFg,
defaultBg: shellExecutionConfig.defaultBg,
});
} else {
const buffer = headlessTerminal.buffer.active;
const lines: AnsiOutput = [];
for (let y = 0; y < headlessTerminal.rows; y++) {
const line = buffer.getLine(buffer.viewportY + y);
const lineContent = line ? line.translateToString(true) : '';
lines.push([
{
text: lineContent,
bold: false,
italic: false,
underline: false,
dim: false,
inverse: false,
fg: '',
bg: '',
},
]);
}
newOutput = lines;
}
// Using stringify for a quick deep comparison.
if (JSON.stringify(output) !== JSON.stringify(newOutput)) {
@@ -602,6 +611,9 @@ export class ShellExecutionService {
if (activePty) {
try {
activePty.headlessTerminal.scrollLines(lines);
if (activePty.headlessTerminal.buffer.active.viewportY < 0) {
activePty.headlessTerminal.scrollToTop();
}
} catch (e) {
// Ignore errors if the pty has already exited, which can happen
// due to a race condition between the exit event and this call.