From 6695c32aa236a0992e224dda541f2a242705ba3a Mon Sep 17 00:00:00 2001 From: Gal Zahavi <38544478+galz10@users.noreply.github.com> Date: Tue, 30 Sep 2025 13:44:58 -0700 Subject: [PATCH] fix(shell): improve shell output presentation and usability (#8837) --- .../ui/components/messages/ToolMessage.tsx | 33 ++++++++- .../services/shellExecutionService.test.ts | 15 ++++- .../src/services/shellExecutionService.ts | 67 +++++++++++++++++-- packages/core/src/utils/terminalSerializer.ts | 9 +-- 4 files changed, 107 insertions(+), 17 deletions(-) diff --git a/packages/cli/src/ui/components/messages/ToolMessage.tsx b/packages/cli/src/ui/components/messages/ToolMessage.tsx index 817a4dd70c..5f86d0d5e6 100644 --- a/packages/cli/src/ui/components/messages/ToolMessage.tsx +++ b/packages/cli/src/ui/components/messages/ToolMessage.tsx @@ -62,11 +62,42 @@ export const ToolMessage: React.FC = ({ ptyId === activeShellPtyId && embeddedShellFocused; + const [lastUpdateTime, setLastUpdateTime] = React.useState(null); + const [userHasFocused, setUserHasFocused] = React.useState(false); + const [showFocusHint, setShowFocusHint] = React.useState(false); + + React.useEffect(() => { + if (resultDisplay) { + setLastUpdateTime(new Date()); + } + }, [resultDisplay]); + + React.useEffect(() => { + if (!lastUpdateTime) { + return; + } + + const timer = setTimeout(() => { + setShowFocusHint(true); + }, 5000); + + return () => clearTimeout(timer); + }, [lastUpdateTime]); + + React.useEffect(() => { + if (isThisShellFocused) { + setUserHasFocused(true); + } + }, [isThisShellFocused]); + const isThisShellFocusable = (name === SHELL_COMMAND_NAME || name === 'Shell') && status === ToolCallStatus.Executing && config?.getShouldUseNodePtyShell(); + const shouldShowFocusHint = + isThisShellFocusable && (showFocusHint || userHasFocused); + const availableHeight = availableTerminalHeight ? Math.max( availableTerminalHeight - STATIC_HEIGHT - RESERVED_LINE_COUNT, @@ -99,7 +130,7 @@ export const ToolMessage: React.FC = ({ description={description} emphasis={emphasis} /> - {isThisShellFocusable && ( + {shouldShowFocusHint && ( {isThisShellFocused ? '(Focused)' : '(ctrl+f to focus)'} diff --git a/packages/core/src/services/shellExecutionService.test.ts b/packages/core/src/services/shellExecutionService.test.ts index 63e6482bef..4dfc489181 100644 --- a/packages/core/src/services/shellExecutionService.test.ts +++ b/packages/core/src/services/shellExecutionService.test.ts @@ -64,6 +64,7 @@ const shellExecutionConfig = { terminalHeight: 24, pager: 'cat', showColor: false, + disableDynamicLineTrimming: true, }; const createExpectedAnsiOutput = (text: string | string[]): AnsiOutput => { @@ -441,6 +442,7 @@ describe('ShellExecutionService', () => { showColor: true, defaultFg: '#ffffff', defaultBg: '#000000', + disableDynamicLineTrimming: true, }; const mockAnsiOutput = [ [{ text: 'hello', fg: '#ffffff', bg: '#000000' }], @@ -458,7 +460,6 @@ describe('ShellExecutionService', () => { expect(mockSerializeTerminalToObject).toHaveBeenCalledWith( expect.anything(), // The terminal object - { defaultFg: '#ffffff', defaultBg: '#000000' }, ); expect(onOutputEventMock).toHaveBeenCalledWith( @@ -476,7 +477,11 @@ describe('ShellExecutionService', () => { pty.onData.mock.calls[0][0]('a\u001b[31mred\u001b[0mword'); pty.onExit.mock.calls[0][0]({ exitCode: 0, signal: null }); }, - { ...shellExecutionConfig, showColor: false }, + { + ...shellExecutionConfig, + showColor: false, + disableDynamicLineTrimming: true, + }, ); const expected = createExpectedAnsiOutput('aredword'); @@ -498,7 +503,11 @@ describe('ShellExecutionService', () => { ); pty.onExit.mock.calls[0][0]({ exitCode: 0, signal: null }); }, - { ...shellExecutionConfig, showColor: false }, + { + ...shellExecutionConfig, + showColor: false, + disableDynamicLineTrimming: true, + }, ); const expected = createExpectedAnsiOutput(['line 1', 'line 2', 'line 3']); diff --git a/packages/core/src/services/shellExecutionService.ts b/packages/core/src/services/shellExecutionService.ts index 9dbb539e7e..bea11819cf 100644 --- a/packages/core/src/services/shellExecutionService.ts +++ b/packages/core/src/services/shellExecutionService.ts @@ -57,6 +57,8 @@ export interface ShellExecutionConfig { showColor?: boolean; defaultFg?: string; defaultBg?: string; + // Used for testing + disableDynamicLineTrimming?: boolean; } /** @@ -383,6 +385,7 @@ export class ShellExecutionService { const MAX_SNIFF_SIZE = 4096; let sniffedBytes = 0; let isWriting = false; + let hasStartedOutput = false; let renderTimeout: NodeJS.Timeout | null = null; const render = (finalRender = false) => { @@ -394,12 +397,20 @@ export class ShellExecutionService { if (!isStreamingRawContent) { return; } + + if (!shellExecutionConfig.disableDynamicLineTrimming) { + if (!hasStartedOutput) { + const bufferText = getFullBufferText(headlessTerminal); + if (bufferText.trim().length === 0) { + return; + } + hasStartedOutput = true; + } + } + let newOutput: AnsiOutput; if (shellExecutionConfig.showColor) { - newOutput = serializeTerminalToObject(headlessTerminal, { - defaultFg: shellExecutionConfig.defaultFg, - defaultBg: shellExecutionConfig.defaultBg, - }); + newOutput = serializeTerminalToObject(headlessTerminal); } else { const buffer = headlessTerminal.buffer.active; const lines: AnsiOutput = []; @@ -422,12 +433,32 @@ export class ShellExecutionService { newOutput = lines; } + let lastNonEmptyLine = -1; + for (let i = newOutput.length - 1; i >= 0; i--) { + const line = newOutput[i]; + if ( + line + .map((segment) => segment.text) + .join('') + .trim().length > 0 + ) { + lastNonEmptyLine = i; + break; + } + } + + const trimmedOutput = newOutput.slice(0, lastNonEmptyLine + 1); + + const finalOutput = shellExecutionConfig.disableDynamicLineTrimming + ? newOutput + : trimmedOutput; + // Using stringify for a quick deep comparison. - if (JSON.stringify(output) !== JSON.stringify(newOutput)) { - output = newOutput; + if (JSON.stringify(output) !== JSON.stringify(finalOutput)) { + output = finalOutput; onOutputEvent({ type: 'data', - chunk: newOutput, + chunk: finalOutput, }); } }; @@ -569,12 +600,26 @@ export class ShellExecutionService { * @param input The string to write to the terminal. */ static writeToPty(pid: number, input: string): void { + if (!this.isPtyActive(pid)) { + return; + } + const activePty = this.activePtys.get(pid); if (activePty) { activePty.ptyProcess.write(input); } } + static isPtyActive(pid: number): boolean { + try { + // process.kill with signal 0 is a way to check for the existence of a process. + // It doesn't actually send a signal. + return process.kill(pid, 0); + } catch (_) { + return false; + } + } + /** * Resizes the pseudo-terminal (PTY) of a running process. * @@ -583,6 +628,10 @@ export class ShellExecutionService { * @param rows The new number of rows. */ static resizePty(pid: number, cols: number, rows: number): void { + if (!this.isPtyActive(pid)) { + return; + } + const activePty = this.activePtys.get(pid); if (activePty) { try { @@ -607,6 +656,10 @@ export class ShellExecutionService { * @param lines The number of lines to scroll. */ static scrollPty(pid: number, lines: number): void { + if (!this.isPtyActive(pid)) { + return; + } + const activePty = this.activePtys.get(pid); if (activePty) { try { diff --git a/packages/core/src/utils/terminalSerializer.ts b/packages/core/src/utils/terminalSerializer.ts index f3c8eacec0..7bcd2a4ce6 100644 --- a/packages/core/src/utils/terminalSerializer.ts +++ b/packages/core/src/utils/terminalSerializer.ts @@ -131,15 +131,12 @@ class Cell { } } -export function serializeTerminalToObject( - terminal: Terminal, - options?: { defaultFg?: string; defaultBg?: string }, -): AnsiOutput { +export function serializeTerminalToObject(terminal: Terminal): AnsiOutput { const buffer = terminal.buffer.active; const cursorX = buffer.cursorX; const cursorY = buffer.cursorY; - const defaultFg = options?.defaultFg ?? '#ffffff'; - const defaultBg = options?.defaultBg ?? '#000000'; + const defaultFg = ''; + const defaultBg = ''; const result: AnsiOutput = [];