From e89cf5d86e434fd4064ebfe3d0309b9ab388df2f Mon Sep 17 00:00:00 2001 From: Jacob Richman Date: Sat, 7 Mar 2026 11:31:09 -0800 Subject: [PATCH] fix(cli): correct shell height reporting (#21492) --- packages/cli/src/ui/AppContainer.test.tsx | 56 ----------------- packages/cli/src/ui/AppContainer.tsx | 26 -------- .../messages/ShellToolMessage.test.tsx | 8 +-- .../components/messages/ShellToolMessage.tsx | 62 ++++++++++++++++--- .../ShellToolMessage.test.tsx.snap | 21 ++----- packages/cli/src/ui/utils/toolLayoutUtils.ts | 15 ++++- 6 files changed, 74 insertions(+), 114 deletions(-) diff --git a/packages/cli/src/ui/AppContainer.test.tsx b/packages/cli/src/ui/AppContainer.test.tsx index 6cf81ca316..0b6eaa037b 100644 --- a/packages/cli/src/ui/AppContainer.test.tsx +++ b/packages/cli/src/ui/AppContainer.test.tsx @@ -232,10 +232,7 @@ import { useInputHistoryStore } from './hooks/useInputHistoryStore.js'; import { useKeypress, type Key } from './hooks/useKeypress.js'; import * as useKeypressModule from './hooks/useKeypress.js'; import { useSuspend } from './hooks/useSuspend.js'; -import { measureElement } from 'ink'; -import { useTerminalSize } from './hooks/useTerminalSize.js'; import { - ShellExecutionService, writeToStdout, enableMouseEvents, disableMouseEvents, @@ -2197,35 +2194,6 @@ describe('AppContainer State Management', () => { }); }); - describe('Terminal Height Calculation', () => { - const mockedMeasureElement = measureElement as Mock; - const mockedUseTerminalSize = useTerminalSize as Mock; - - it('should prevent terminal height from being less than 1', async () => { - const resizePtySpy = vi.spyOn(ShellExecutionService, 'resizePty'); - // Arrange: Simulate a small terminal and a large footer - mockedUseTerminalSize.mockReturnValue({ columns: 80, rows: 5 }); - mockedMeasureElement.mockReturnValue({ width: 80, height: 10 }); // Footer is taller than the screen - - mockedUseGeminiStream.mockReturnValue({ - ...DEFAULT_GEMINI_STREAM_MOCK, - activePtyId: 'some-id', - }); - - let unmount: () => void; - await act(async () => { - const result = renderAppContainer(); - unmount = result.unmount; - }); - await waitFor(() => expect(resizePtySpy).toHaveBeenCalled()); - const lastCall = - resizePtySpy.mock.calls[resizePtySpy.mock.calls.length - 1]; - // Check the height argument specifically - expect(lastCall[2]).toBe(1); - unmount!(); - }); - }); - describe('Keyboard Input Handling (CTRL+C / CTRL+D)', () => { let mockHandleSlashCommand: Mock; let mockCancelOngoingRequest: Mock; @@ -3141,30 +3109,6 @@ describe('AppContainer State Management', () => { }); }); - describe('Shell Interaction', () => { - it('should not crash if resizing the pty fails', async () => { - const resizePtySpy = vi - .spyOn(ShellExecutionService, 'resizePty') - .mockImplementation(() => { - throw new Error('Cannot resize a pty that has already exited'); - }); - - mockedUseGeminiStream.mockReturnValue({ - ...DEFAULT_GEMINI_STREAM_MOCK, - activePtyId: 'some-pty-id', // Make sure activePtyId is set - }); - - // The main assertion is that the render does not throw. - let unmount: () => void; - await act(async () => { - const result = renderAppContainer(); - unmount = result.unmount; - }); - - await waitFor(() => expect(resizePtySpy).toHaveBeenCalled()); - unmount!(); - }); - }); describe('Banner Text', () => { it('should render placeholder banner text for USE_GEMINI auth type', async () => { const config = makeFakeConfig(); diff --git a/packages/cli/src/ui/AppContainer.tsx b/packages/cli/src/ui/AppContainer.tsx index 30ebe221f0..965a63db43 100644 --- a/packages/cli/src/ui/AppContainer.tsx +++ b/packages/cli/src/ui/AppContainer.tsx @@ -1420,32 +1420,6 @@ Logging in with Google... Restarting Gemini CLI to continue. const initialPromptSubmitted = useRef(false); const geminiClient = config.getGeminiClient(); - useEffect(() => { - if (activePtyId) { - try { - ShellExecutionService.resizePty( - activePtyId, - Math.floor(terminalWidth * SHELL_WIDTH_FRACTION), - Math.max( - Math.floor(availableTerminalHeight - SHELL_HEIGHT_PADDING), - 1, - ), - ); - } catch (e) { - // This can happen in a race condition where the pty exits - // right before we try to resize it. - if ( - !( - e instanceof Error && - e.message.includes('Cannot resize a pty that has already exited') - ) - ) { - throw e; - } - } - } - }, [terminalWidth, availableTerminalHeight, activePtyId]); - useEffect(() => { if ( initialPrompt && diff --git a/packages/cli/src/ui/components/messages/ShellToolMessage.test.tsx b/packages/cli/src/ui/components/messages/ShellToolMessage.test.tsx index 76b8f95ce7..40e5a7e781 100644 --- a/packages/cli/src/ui/components/messages/ShellToolMessage.test.tsx +++ b/packages/cli/src/ui/components/messages/ShellToolMessage.test.tsx @@ -195,7 +195,7 @@ describe('', () => { [ 'uses ACTIVE_SHELL_MAX_LINES when availableTerminalHeight is large', 100, - ACTIVE_SHELL_MAX_LINES, + ACTIVE_SHELL_MAX_LINES - 3, false, ], [ @@ -207,7 +207,7 @@ describe('', () => { [ 'defaults to ACTIVE_SHELL_MAX_LINES in alternate buffer when availableTerminalHeight is undefined', undefined, - ACTIVE_SHELL_MAX_LINES, + ACTIVE_SHELL_MAX_LINES - 3, false, ], ])('%s', async (_, availableTerminalHeight, expectedMaxLines, focused) => { @@ -301,8 +301,8 @@ describe('', () => { await waitUntilReady(); await waitFor(() => { const frame = lastFrame(); - // Should still be constrained to ACTIVE_SHELL_MAX_LINES (15) because isExpandable is false - expect(frame.match(/Line \d+/g)?.length).toBe(15); + // Should still be constrained to 12 (15 - 3) because isExpandable is false + expect(frame.match(/Line \d+/g)?.length).toBe(12); }); expect(lastFrame()).toMatchSnapshot(); unmount(); diff --git a/packages/cli/src/ui/components/messages/ShellToolMessage.tsx b/packages/cli/src/ui/components/messages/ShellToolMessage.tsx index 3a0cdb702e..f34aa08bfb 100644 --- a/packages/cli/src/ui/components/messages/ShellToolMessage.tsx +++ b/packages/cli/src/ui/components/messages/ShellToolMessage.tsx @@ -24,8 +24,16 @@ import type { ToolMessageProps } from './ToolMessage.js'; import { ACTIVE_SHELL_MAX_LINES } from '../../constants.js'; import { useAlternateBuffer } from '../../hooks/useAlternateBuffer.js'; import { useUIState } from '../../contexts/UIStateContext.js'; -import { type Config } from '@google/gemini-cli-core'; -import { calculateShellMaxLines } from '../../utils/toolLayoutUtils.js'; +import { + type Config, + ShellExecutionService, + CoreToolCallStatus, +} from '@google/gemini-cli-core'; +import { + calculateShellMaxLines, + calculateToolContentMaxLines, + SHELL_CONTENT_OVERHEAD, +} from '../../utils/toolLayoutUtils.js'; export interface ShellToolMessageProps extends ToolMessageProps { config?: Config; @@ -78,6 +86,47 @@ export const ShellToolMessage: React.FC = ({ embeddedShellFocused, ); + const maxLines = calculateShellMaxLines({ + status, + isAlternateBuffer, + isThisShellFocused, + availableTerminalHeight, + constrainHeight, + isExpandable, + }); + + const availableHeight = calculateToolContentMaxLines({ + availableTerminalHeight, + isAlternateBuffer, + maxLinesLimit: maxLines, + }); + + React.useEffect(() => { + const isExecuting = status === CoreToolCallStatus.Executing; + if (isExecuting && ptyId) { + try { + const childWidth = terminalWidth - 4; // account for padding and borders + const finalHeight = + availableHeight ?? ACTIVE_SHELL_MAX_LINES - SHELL_CONTENT_OVERHEAD; + + ShellExecutionService.resizePty( + ptyId, + Math.max(1, childWidth), + Math.max(1, finalHeight), + ); + } catch (e) { + if ( + !( + e instanceof Error && + e.message.includes('Cannot resize a pty that has already exited') + ) + ) { + throw e; + } + } + } + }, [ptyId, status, terminalWidth, availableHeight]); + const { setEmbeddedShellFocused } = useUIActions(); const wasFocusedRef = React.useRef(false); @@ -166,14 +215,7 @@ export const ShellToolMessage: React.FC = ({ terminalWidth={terminalWidth} renderOutputAsMarkdown={renderOutputAsMarkdown} hasFocus={isThisShellFocused} - maxLines={calculateShellMaxLines({ - status, - isAlternateBuffer, - isThisShellFocused, - availableTerminalHeight, - constrainHeight, - isExpandable, - })} + maxLines={maxLines} /> {isThisShellFocused && config && ( > Height Constraints > defaults to ACTIVE_SHELL_MA "╭──────────────────────────────────────────────────────────────────────────────╮ │ ⊶ Shell Command A shell command │ │ │ -│ Line 86 │ -│ Line 87 │ -│ Line 88 │ │ Line 89 │ │ Line 90 │ │ Line 91 │ @@ -16,8 +13,8 @@ exports[` > Height Constraints > defaults to ACTIVE_SHELL_MA │ Line 95 │ │ Line 96 │ │ Line 97 │ -│ Line 98 ▄ │ -│ Line 99 █ │ +│ Line 98 │ +│ Line 99 ▄ │ │ Line 100 █ │ " `; @@ -148,9 +145,6 @@ exports[` > Height Constraints > stays constrained in altern "╭──────────────────────────────────────────────────────────────────────────────╮ │ ✓ Shell Command A shell command │ │ │ -│ Line 86 │ -│ Line 87 │ -│ Line 88 │ │ Line 89 │ │ Line 90 │ │ Line 91 │ @@ -160,8 +154,8 @@ exports[` > Height Constraints > stays constrained in altern │ Line 95 │ │ Line 96 │ │ Line 97 │ -│ Line 98 ▄ │ -│ Line 99 █ │ +│ Line 98 │ +│ Line 99 ▄ │ │ Line 100 █ │ " `; @@ -170,9 +164,6 @@ exports[` > Height Constraints > uses ACTIVE_SHELL_MAX_LINES "╭──────────────────────────────────────────────────────────────────────────────╮ │ ⊶ Shell Command A shell command │ │ │ -│ Line 86 │ -│ Line 87 │ -│ Line 88 │ │ Line 89 │ │ Line 90 │ │ Line 91 │ @@ -182,8 +173,8 @@ exports[` > Height Constraints > uses ACTIVE_SHELL_MAX_LINES │ Line 95 │ │ Line 96 │ │ Line 97 │ -│ Line 98 ▄ │ -│ Line 99 █ │ +│ Line 98 │ +│ Line 99 ▄ │ │ Line 100 █ │ " `; diff --git a/packages/cli/src/ui/utils/toolLayoutUtils.ts b/packages/cli/src/ui/utils/toolLayoutUtils.ts index 8f619901f6..6ba1b85c5e 100644 --- a/packages/cli/src/ui/utils/toolLayoutUtils.ts +++ b/packages/cli/src/ui/utils/toolLayoutUtils.ts @@ -20,6 +20,13 @@ export const TOOL_RESULT_ASB_RESERVED_LINE_COUNT = 6; export const TOOL_RESULT_STANDARD_RESERVED_LINE_COUNT = 2; export const TOOL_RESULT_MIN_LINES_SHOWN = 2; +/** + * The vertical space (in lines) consumed by the shell UI elements + * (1 line for the shell title/header and 2 lines for the top and bottom borders). + */ +export const SHELL_CONTENT_OVERHEAD = + TOOL_RESULT_STATIC_HEIGHT + TOOL_RESULT_STANDARD_RESERVED_LINE_COUNT; + /** * Calculates the final height available for the content of a tool result display. * @@ -88,7 +95,9 @@ export function calculateShellMaxLines(options: { // 2. Handle cases where height is unknown (Standard mode history). if (availableTerminalHeight === undefined) { - return isAlternateBuffer ? ACTIVE_SHELL_MAX_LINES : undefined; + return isAlternateBuffer + ? ACTIVE_SHELL_MAX_LINES - SHELL_CONTENT_OVERHEAD + : undefined; } const maxLinesBasedOnHeight = Math.max(1, availableTerminalHeight - 2); @@ -103,8 +112,8 @@ export function calculateShellMaxLines(options: { // 4. Fall back to process-based constants. const isExecuting = status === CoreToolCallStatus.Executing; const shellMaxLinesLimit = isExecuting - ? ACTIVE_SHELL_MAX_LINES - : COMPLETED_SHELL_MAX_LINES; + ? ACTIVE_SHELL_MAX_LINES - SHELL_CONTENT_OVERHEAD + : COMPLETED_SHELL_MAX_LINES - SHELL_CONTENT_OVERHEAD; return Math.min(maxLinesBasedOnHeight, shellMaxLinesLimit); }