From b4acf75930e767e2e3183d9e20d4125166cfa882 Mon Sep 17 00:00:00 2001 From: Jarrod Whelan <150866123+jwhelangoog@users.noreply.github.com> Date: Thu, 26 Mar 2026 15:43:45 -0700 Subject: [PATCH] refactor(cli): address code review feedback - Abstracted hardcoded padding, margin, and width calculations in `DenseToolMessage` into explicit constants to improve readability. - Made `terminalWidth` a required property on `DenseToolMessageProps` to ensure consistent layout calculations, simplifying internal ternary checks. - Updated `DenseToolMessage` tests to provide the now-required `terminalWidth` prop. - Removed expensive `node:crypto` usage in `DiffRenderer` key generation, opting for a simpler optional key. - Simplified terminal refresh logic in `AppContainer` by removing `setTimeout` from the "show more lines" handler, as it was redundant. - Streamlined `staticHeight` calculation loop in `ToolGroupMessage` to use layout constants. - Removed redundant `height={0}` properties on Box borders in `ToolGroupMessage`. - Simplified `effectiveMaxHeight` assignment in `ToolResultDisplay` by using the pre-calculated `availableHeight` directly. - Restore tool message padding by moving `paddingTop={1}` from `ToolMessage` and `ShellToolMessage` content boxes back to `paddingBottom={1}` in `StickyHeader`. - Restore `ToolConfirmationQueue` layout. - Removed excluded file information from ReadManyFiles tool's compact output --- docs/reference/configuration.md | 2 +- packages/cli/src/ui/AppContainer.tsx | 9 +- .../cli/src/ui/components/StickyHeader.tsx | 1 + .../ui/components/ToolConfirmationQueue.tsx | 11 ++- .../messages/DenseToolMessage.test.tsx | 3 +- .../components/messages/DenseToolMessage.tsx | 92 ++++++++----------- .../ui/components/messages/DiffRenderer.tsx | 5 +- .../components/messages/ShellToolMessage.tsx | 1 - .../components/messages/ToolGroupMessage.tsx | 46 ++++++---- .../ui/components/messages/ToolMessage.tsx | 1 - .../components/messages/ToolResultDisplay.tsx | 7 +- packages/cli/src/ui/types.ts | 3 - 12 files changed, 84 insertions(+), 97 deletions(-) diff --git a/docs/reference/configuration.md b/docs/reference/configuration.md index 17a993327e..2d6ed7d298 100644 --- a/docs/reference/configuration.md +++ b/docs/reference/configuration.md @@ -2402,7 +2402,7 @@ conventions and context. loaded, allowing you to verify the hierarchy and content being used by the AI. - See the [Commands documentation](./commands.md#memory) for full details on - the `/memory` command and its sub-commands (`show` and `refresh`). + the `/memory` command and its sub-commands (`show` and `reload`). By understanding and utilizing these configuration layers and the hierarchical nature of context files, you can effectively manage the AI's memory and tailor diff --git a/packages/cli/src/ui/AppContainer.tsx b/packages/cli/src/ui/AppContainer.tsx index c6259380fb..129bcb8fda 100644 --- a/packages/cli/src/ui/AppContainer.tsx +++ b/packages/cli/src/ui/AppContainer.tsx @@ -1757,7 +1757,6 @@ Logging in with Google... Restarting Gemini CLI to continue. } const toggleLastTurnTools = () => { - // If the user manually collapses/expands the view, show the hint and reset the x-second timer. triggerExpandHint(true); const targetToolCallIds = getLastTurnToolCallIds( @@ -1824,13 +1823,7 @@ Logging in with Google... Restarting Gemini CLI to continue. ) { setConstrainHeight(false); toggleLastTurnTools(); - - // Force layout refresh after a short delay to allow the terminal layout to settle. - // Minimize "blank screen" issue after any async subview updates are complete. - setTimeout(() => { - refreshStatic(); - }, 250); - + refreshStatic(); return true; } else if ( (keyMatchers[Command.FOCUS_SHELL_INPUT](key) || diff --git a/packages/cli/src/ui/components/StickyHeader.tsx b/packages/cli/src/ui/components/StickyHeader.tsx index 9c44c76855..62d5dcd22d 100644 --- a/packages/cli/src/ui/components/StickyHeader.tsx +++ b/packages/cli/src/ui/components/StickyHeader.tsx @@ -67,6 +67,7 @@ export const StickyHeader: React.FC = ({ borderLeft={true} borderRight={true} paddingX={1} + paddingBottom={1} paddingTop={isFirst ? 0 : 1} > {children} diff --git a/packages/cli/src/ui/components/ToolConfirmationQueue.tsx b/packages/cli/src/ui/components/ToolConfirmationQueue.tsx index 1fa34a6641..e5294e9614 100644 --- a/packages/cli/src/ui/components/ToolConfirmationQueue.tsx +++ b/packages/cli/src/ui/components/ToolConfirmationQueue.tsx @@ -66,9 +66,9 @@ export const ToolConfirmationQueue: React.FC = ({ // ToolConfirmationMessage needs to know the height available for its OWN content. // We subtract the lines used by the Queue wrapper: - // - 2 lines for the rounded border (top/bottom) + // - 2 lines for the rounded border // - 2 lines for the Header (text + margin) - // - 2 lines for Tool Identity (text + margin) if shown + // - 2 lines for Tool Identity (text + margin) const availableContentHeight = constrainHeight ? Math.max(maxHeight - (hideToolIdentity ? 4 : 6), 4) : undefined; @@ -83,7 +83,10 @@ export const ToolConfirmationQueue: React.FC = ({ > {/* Header */} - + {getConfirmationHeader(tool.confirmationDetails)} @@ -95,7 +98,7 @@ export const ToolConfirmationQueue: React.FC = ({ {!hideToolIdentity && ( - + { status: CoreToolCallStatus.Success, resultDisplay: 'Success result' as ToolResultDisplay, confirmationDetails: undefined, + terminalWidth: 80, }; it('renders correctly for a successful string result', async () => { diff --git a/packages/cli/src/ui/components/messages/DenseToolMessage.tsx b/packages/cli/src/ui/components/messages/DenseToolMessage.tsx index 60355c72a7..6e81d07931 100644 --- a/packages/cli/src/ui/components/messages/DenseToolMessage.tsx +++ b/packages/cli/src/ui/components/messages/DenseToolMessage.tsx @@ -1,6 +1,6 @@ /** * @license - * Copyright 2025 Google LLC + * Copyright 2026 Google LLC * SPDX-License-Identifier: Apache-2.0 */ @@ -18,7 +18,11 @@ import { isListResult, isReadManyFilesResult, } from '@google/gemini-cli-core'; -import { type IndividualToolCallDisplay, isTodoList } from '../../types.js'; +import { + type IndividualToolCallDisplay, + type ToolResultDisplay, + isTodoList, +} from '../../types.js'; import { useAlternateBuffer } from '../../hooks/useAlternateBuffer.js'; import { ToolStatusIndicator } from './ToolShared.js'; import { theme } from '../../semantic-colors.js'; @@ -36,8 +40,13 @@ import { colorizeCode } from '../../utils/CodeColorizer.js'; import { useToolActions } from '../../contexts/ToolActionsContext.js'; import { getFileExtension } from '../../utils/fileUtils.js'; +const PAYLOAD_MARGIN_LEFT = 6; +const PAYLOAD_BORDER_CHROME_WIDTH = 4; // paddingX=1 (2 cols) + borders (2 cols) +const PAYLOAD_SCROLL_GUTTER = 4; +const PAYLOAD_MAX_WIDTH = 120 + PAYLOAD_SCROLL_GUTTER; + interface DenseToolMessageProps extends IndividualToolCallDisplay { - terminalWidth?: number; + terminalWidth: number; availableTerminalHeight?: number; } @@ -63,10 +72,6 @@ const hasPayload = (res: unknown): res is PayloadResult => { return typeof value === 'string'; }; -/** - * --- RENDER HELPERS --- - */ - const RenderItemsList: React.FC<{ items?: string[]; maxVisible?: number; @@ -88,17 +93,13 @@ const RenderItemsList: React.FC<{ ); }; -/** - * --- SCENARIO LOGIC (Pure Functions) --- - */ - function getFileOpData( diff: FileDiff, status: CoreToolCallStatus, - resultDisplay: unknown, - terminalWidth?: number, - availableTerminalHeight?: number, - isClickable?: boolean, + resultDisplay: ToolResultDisplay | undefined, + terminalWidth: number, + availableTerminalHeight: number | undefined, + isClickable: boolean, ): ViewParts { const added = (diff.diffStat?.model_added_lines ?? 0) + @@ -177,7 +178,7 @@ function getFileOpData( @@ -201,26 +202,12 @@ function getReadManyFilesData(result: ReadManyFilesResult): ViewParts { skippedCount > 0 ? ` (${skippedCount} ignored)` : '' }`; const summary = → {summaryStr}; - - const excludedText = - result.excludes && result.excludes.length > 0 - ? `Excluded patterns: ${result.excludes.slice(0, 3).join(', ')}${ - result.excludes.length > 3 ? '...' : '' - }` - : undefined; - const hasItems = items.length > 0; - const payload = - hasItems || excludedText ? ( - - {hasItems && } - {excludedText && ( - - {excludedText} - - )} - - ) : undefined; + const payload = hasItems ? ( + + {hasItems && } + + ) : undefined; return { description, summary, payload }; } @@ -309,10 +296,6 @@ function getGenericSuccessData( return { description, summary, payload }; } -/** - * --- MAIN COMPONENT --- - */ - export const DenseToolMessage: React.FC = (props) => { const { callId, @@ -339,7 +322,7 @@ export const DenseToolMessage: React.FC = (props) => { const [isFocused, setIsFocused] = useState(false); const toggleRef = useRef(null); - // 1. Unified File Data Extraction (Safely bridge resultDisplay and confirmationDetails) + // Unified File Data Extraction (Safely bridge resultDisplay and confirmationDetails) const diff = useMemo((): FileDiff | undefined => { if (isFileDiff(resultDisplay)) return resultDisplay; if (confirmationDetails?.type === 'edit') { @@ -375,7 +358,7 @@ export const DenseToolMessage: React.FC = (props) => { isActive: isAlternateBuffer && !!diff, }); - // 2. State-to-View Coordination + // State-to-View Coordination const viewParts = useMemo((): ViewParts => { if (diff) { return getFileOpData( @@ -459,7 +442,7 @@ export const DenseToolMessage: React.FC = (props) => { return colorizeCode({ code: addedContent, language: fileExtension, - maxWidth: terminalWidth ? terminalWidth - 6 : 80, + maxWidth: terminalWidth - PAYLOAD_MARGIN_LEFT, settings, disableColor: status === CoreToolCallStatus.Cancelled, returnLines: true, @@ -468,7 +451,7 @@ export const DenseToolMessage: React.FC = (props) => { return renderDiffLines({ parsedLines, filename: diff.fileName, - terminalWidth: terminalWidth ? terminalWidth - 6 : 80, + terminalWidth: terminalWidth - PAYLOAD_MARGIN_LEFT, disableColor: status === CoreToolCallStatus.Cancelled, }); } @@ -502,7 +485,6 @@ export const DenseToolMessage: React.FC = (props) => { {item} ); - // 3. Final Layout return ( @@ -529,7 +511,7 @@ export const DenseToolMessage: React.FC = (props) => { {showPayload && isAlternateBuffer && diffLines.length > 0 && ( = (props) => { borderStyle="round" borderColor={theme.border.default} borderDimColor={true} - maxWidth={terminalWidth ? Math.min(124, terminalWidth - 6) : 124} + maxWidth={Math.min( + PAYLOAD_MAX_WIDTH, + terminalWidth - PAYLOAD_MARGIN_LEFT, + )} > = (props) => { keyExtractor={keyExtractor} estimatedItemHeight={() => 1} hasFocus={isFocused} - width={ - // adjustment: 6 margin - 4 padding/border - 4 right-scroll-gutter - terminalWidth ? Math.min(120, terminalWidth - 6 - 4 - 4) : 70 - } + width={Math.min( + PAYLOAD_MAX_WIDTH, + terminalWidth - + PAYLOAD_MARGIN_LEFT - + PAYLOAD_BORDER_CHROME_WIDTH - + PAYLOAD_SCROLL_GUTTER, + )} /> )} {showPayload && (!isAlternateBuffer || !diff) && viewParts.payload && ( - + {viewParts.payload} )} {showPayload && outputFile && ( - + (Output saved to: {outputFile}) diff --git a/packages/cli/src/ui/components/messages/DiffRenderer.tsx b/packages/cli/src/ui/components/messages/DiffRenderer.tsx index 2a0d5b39c4..ddee2e55df 100644 --- a/packages/cli/src/ui/components/messages/DiffRenderer.tsx +++ b/packages/cli/src/ui/components/messages/DiffRenderer.tsx @@ -7,7 +7,6 @@ import type React from 'react'; import { useMemo } from 'react'; import { Box, Text, useIsScreenReaderEnabled } from 'ink'; -import crypto from 'node:crypto'; import { colorizeCode, colorizeLine } from '../../utils/CodeColorizer.js'; import { MaxSizedBox } from '../shared/MaxSizedBox.js'; import { theme as semanticTheme } from '../../semantic-colors.js'; @@ -165,9 +164,7 @@ export const DiffRenderer: React.FC = ({ disableColor, }); } else { - const key = filename - ? `diff-box-${filename}` - : `diff-box-${crypto.createHash('sha1').update(JSON.stringify(parsedLines)).digest('hex')}`; + const key = filename ? `diff-box-${filename}` : undefined; return ( = ({ borderLeft={true} borderRight={true} paddingX={1} - paddingTop={1} flexDirection="column" > { const res = tool.resultDisplay; if (!res) return false; + // TODO(24053): Usage of type guards makes this class too aware of internals if (isFileDiff(res)) return true; if (tool.confirmationDetails?.type === 'edit') return true; if (isGrepResult(res) && res.matches.length > 0) return true; @@ -217,25 +222,30 @@ export const ToolGroupMessage: React.FC = ({ for (let i = 0; i < groupedTools.length; i++) { const group = groupedTools[i]; const isFirst = i === 0; + const isLast = i === groupedTools.length - 1; const prevGroup = i > 0 ? groupedTools[i - 1] : null; const prevIsCompact = prevGroup && !Array.isArray(prevGroup) && isCompactTool(prevGroup, isCompactModeEnabled); + const nextGroup = !isLast ? groupedTools[i + 1] : null; + const nextIsCompact = + nextGroup && + !Array.isArray(nextGroup) && + isCompactTool(nextGroup, isCompactModeEnabled); + const isAgentGroup = Array.isArray(group); const isCompact = !isAgentGroup && isCompactTool(group, isCompactModeEnabled); + const showClosingBorder = !isCompact && (nextIsCompact || isLast); + if (isFirst) { - height += (borderTopOverride ?? false) ? 1 : 0; - } else if (isCompact && prevIsCompact) { - height += 0; - } else if (isCompact || prevIsCompact) { + height += borderTopOverride ? 1 : 0; + } else if (isCompact !== prevIsCompact) { + // Add a 1-line gap when transitioning between compact and standard tools (or vice versa) height += 1; - } else { - // Gap is provided by StickyHeader's paddingTop=1 - height += 0; } const isFirstProp = !!(isFirst @@ -247,17 +257,15 @@ export const ToolGroupMessage: React.FC = ({ height += 1; // Header height += group.length; // 1 line per agent if (isFirstProp) height += 1; // Top border + if (showClosingBorder) height += 1; // Bottom border } else { if (isCompact) { height += 1; // Base height for compact tool } else { // Static overhead for standard tool header: - // 1 line for header text - // 1 line for dark separator - // 1 line for tool body internal paddingTop=1 - // 1 line for top border (if isFirstProp) OR StickyHeader paddingTop=1 (if !isFirstProp) - height += 3; - height += isFirstProp ? 1 : 1; // Either top border or paddingTop + height += + TOOL_RESULT_STATIC_HEIGHT + + TOOL_RESULT_STANDARD_RESERVED_LINE_COUNT; } } } @@ -317,13 +325,14 @@ export const ToolGroupMessage: React.FC = ({ */ width={terminalWidth} paddingRight={TOOL_MESSAGE_HORIZONTAL_MARGIN} + // When border will be present, add margin of 1 to create spacing from the + // previous message. marginBottom={(borderBottomOverride ?? true) ? 1 : 0} > {visibleToolCalls.length === 0 && isExplicitClosingSlice && borderBottomOverride === true && ( = ({ !isAgentGroup && isCompactTool(group, isCompactModeEnabled); const isTopicToolCall = !isAgentGroup && isTopicTool(group.name); + // When border is present, add margin of 1 to create spacing from the + // previous message. let marginTop = 0; if (isFirst) { marginTop = (borderTopOverride ?? false) ? 1 : 0; @@ -376,7 +387,10 @@ export const ToolGroupMessage: React.FC = ({ } else if (isCompact || prevIsCompact) { marginTop = 1; } else { - // Subsequent standard tools: StickyHeader's paddingTop=1 provides the gap. + // For subsequent standard tools scenarios, the ToolMessage and + // ShellToolMessage components manage their own top spacing by passing + // `isFirst=false` to their internal StickyHeader which then applies + // a paddingTop=1 to create desired gap between standard tool outputs. marginTop = 0; } @@ -406,7 +420,6 @@ export const ToolGroupMessage: React.FC = ({ /> {showClosingBorder && ( = ({ {showClosingBorder && ( = ({ borderLeft={true} borderRight={true} paddingX={1} - paddingTop={1} flexDirection="column" > {status === CoreToolCallStatus.Executing && progress !== undefined && ( diff --git a/packages/cli/src/ui/components/messages/ToolResultDisplay.tsx b/packages/cli/src/ui/components/messages/ToolResultDisplay.tsx index 2b05eb453b..4b51ae8ab8 100644 --- a/packages/cli/src/ui/components/messages/ToolResultDisplay.tsx +++ b/packages/cli/src/ui/components/messages/ToolResultDisplay.tsx @@ -179,11 +179,8 @@ export const ToolResultDisplay: React.FC = ({ // Final render based on session mode if (isAlternateBuffer) { - // If availableTerminalHeight is undefined, we don't have a fixed budget, - // so if maxLines is also undefined, we shouldn't cap the height at all. - const effectiveMaxHeight = - maxLines ?? - (availableTerminalHeight !== undefined ? availableHeight : undefined); + // Use maxLines if provided, otherwise fall back to the calculated available height + const effectiveMaxHeight = maxLines ?? availableHeight; return ( seems to have some issues with typescript's -// type inference e.g. historyItem.type === 'tool_group' isn't auto-inferring that -// 'tools' in historyItem. // Individually exported types extending HistoryItemBase export type HistoryItemWithoutId = | HistoryItemUser