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
2026-03-30 16:00:41 -07:00 · 2026-03-26 15:43:45 -07:00
parent 166d3cf23b
commit b4acf75930
12 changed files with 84 additions and 97 deletions
--- 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
--- 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) ||
--- 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<StickyHeaderProps> = ({
      borderLeft={true}
      borderRight={true}
      paddingX={1}
+      paddingBottom={1}
      paddingTop={isFirst ? 0 : 1}
    >
      {children}
--- 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<ToolConfirmationQueueProps> = ({

  // 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<ToolConfirmationQueueProps> = ({
      >
        <Box flexDirection="column" width={mainAreaWidth - 4}>
          {/* Header */}
-          <Box marginBottom={1} justifyContent="space-between">
+          <Box
+            marginBottom={hideToolIdentity ? 0 : 1}
+            justifyContent="space-between"
+          >
            <Text color={borderColor} bold>
              {getConfirmationHeader(tool.confirmationDetails)}
            </Text>
@@ -95,7 +98,7 @@ export const ToolConfirmationQueue: React.FC<ToolConfirmationQueueProps> = ({
          </Box>

          {!hideToolIdentity && (
-            <Box marginBottom={1}>
+            <Box>
              <ToolStatusIndicator status={tool.status} name={tool.name} />
              <ToolInfo
                name={tool.name}
--- a/packages/cli/src/ui/components/messages/DenseToolMessage.test.tsx
+++ b/packages/cli/src/ui/components/messages/DenseToolMessage.test.tsx
@@ -1,6 +1,6 @@
 /**
 * @license
- * Copyright 2025 Google LLC
+ * Copyright 2026 Google LLC
 * SPDX-License-Identifier: Apache-2.0
 */

@@ -31,6 +31,7 @@ describe('DenseToolMessage', () => {
    status: CoreToolCallStatus.Success,
    resultDisplay: 'Success result' as ToolResultDisplay,
    confirmationDetails: undefined,
+    terminalWidth: 80,
  };

  it('renders correctly for a successful string result', async () => {
--- 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(
    <DiffRenderer
      diffContent={diff.fileDiff}
      filename={diff.fileName}
-      terminalWidth={terminalWidth ? terminalWidth - 6 : 80}
+      terminalWidth={terminalWidth - PAYLOAD_MARGIN_LEFT}
      availableTerminalHeight={availableTerminalHeight}
      disableColor={status === CoreToolCallStatus.Cancelled}
    />
@@ -201,26 +202,12 @@ function getReadManyFilesData(result: ReadManyFilesResult): ViewParts {
    skippedCount > 0 ? ` (${skippedCount} ignored)` : ''
  }`;
  const summary = <Text color={theme.text.accent}>→ {summaryStr}</Text>;
-
-  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 ? (
-      <Box flexDirection="column" marginLeft={2}>
-        {hasItems && <RenderItemsList items={items} maxVisible={maxVisible} />}
-        {excludedText && (
-          <Text color={theme.text.secondary} dimColor>
-            {excludedText}
-          </Text>
-        )}
-      </Box>
-    ) : undefined;
+  const payload = hasItems ? (
+    <Box flexDirection="column" marginLeft={2}>
+      {hasItems && <RenderItemsList items={items} maxVisible={maxVisible} />}
+    </Box>
+  ) : undefined;

  return { description, summary, payload };
 }
@@ -309,10 +296,6 @@ function getGenericSuccessData(
  return { description, summary, payload };
 }

-/**
- * --- MAIN COMPONENT ---
- */
-
 export const DenseToolMessage: React.FC<DenseToolMessageProps> = (props) => {
  const {
    callId,
@@ -339,7 +322,7 @@ export const DenseToolMessage: React.FC<DenseToolMessageProps> = (props) => {
  const [isFocused, setIsFocused] = useState(false);
  const toggleRef = useRef<DOMElement>(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<DenseToolMessageProps> = (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<DenseToolMessageProps> = (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<DenseToolMessageProps> = (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<DenseToolMessageProps> = (props) => {
    <Box minHeight={1}>{item}</Box>
  );

-  // 3. Final Layout
  return (
    <Box flexDirection="column">
      <Box marginLeft={2} flexDirection="row" flexWrap="wrap">
@@ -529,7 +511,7 @@ export const DenseToolMessage: React.FC<DenseToolMessageProps> = (props) => {

      {showPayload && isAlternateBuffer && diffLines.length > 0 && (
        <Box
-          marginLeft={6}
+          marginLeft={PAYLOAD_MARGIN_LEFT}
          marginTop={1}
          marginBottom={1}
          paddingX={1}
@@ -541,7 +523,10 @@ export const DenseToolMessage: React.FC<DenseToolMessageProps> = (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,
+          )}
        >
          <ScrollableList
            data={diffLines}
@@ -549,22 +534,25 @@ export const DenseToolMessage: React.FC<DenseToolMessageProps> = (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,
+            )}
          />
        </Box>
      )}

      {showPayload && (!isAlternateBuffer || !diff) && viewParts.payload && (
-        <Box marginLeft={6} marginTop={1} marginBottom={1}>
+        <Box marginLeft={PAYLOAD_MARGIN_LEFT} marginTop={1} marginBottom={1}>
          {viewParts.payload}
        </Box>
      )}

      {showPayload && outputFile && (
-        <Box marginLeft={6} marginTop={1} marginBottom={1}>
+        <Box marginLeft={PAYLOAD_MARGIN_LEFT} marginTop={1} marginBottom={1}>
          <Text color={theme.text.secondary}>
            (Output saved to: {outputFile})
          </Text>
--- 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<DiffRendererProps> = ({
        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 (
        <MaxSizedBox
--- a/packages/cli/src/ui/components/messages/ShellToolMessage.tsx
+++ b/packages/cli/src/ui/components/messages/ShellToolMessage.tsx
@@ -190,7 +190,6 @@ export const ShellToolMessage: React.FC<ShellToolMessageProps> = ({
        borderLeft={true}
        borderRight={true}
        paddingX={1}
-        paddingTop={1}
        flexDirection="column"
      >
        <ToolResultDisplay
--- a/packages/cli/src/ui/components/messages/ToolGroupMessage.tsx
+++ b/packages/cli/src/ui/components/messages/ToolGroupMessage.tsx
@@ -41,6 +41,10 @@ import {
 import { useUIState } from '../../contexts/UIStateContext.js';
 import { getToolGroupBorderAppearance } from '../../utils/borderStyles.js';
 import { useSettings } from '../../contexts/SettingsContext.js';
+import {
+  TOOL_RESULT_STATIC_HEIGHT,
+  TOOL_RESULT_STANDARD_RESERVED_LINE_COUNT,
+} from '../../utils/toolLayoutUtils.js';

 const COMPACT_OUTPUT_ALLOWLIST = new Set([
  EDIT_DISPLAY_NAME,
@@ -74,6 +78,7 @@ export const hasDensePayload = (tool: IndividualToolCallDisplay): boolean => {
  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<ToolGroupMessageProps> = ({
    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<ToolGroupMessageProps> = ({
        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<ToolGroupMessageProps> = ({
    */
      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 && (
          <Box
-            height={0}
            width={contentWidth}
            borderLeft={true}
            borderRight={true}
@@ -368,6 +377,8 @@ export const ToolGroupMessage: React.FC<ToolGroupMessageProps> = ({
          !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<ToolGroupMessageProps> = ({
        } 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<ToolGroupMessageProps> = ({
              />
              {showClosingBorder && (
                <Box
-                  height={0}
                  width={contentWidth}
                  borderLeft={true}
                  borderRight={true}
@@ -475,7 +488,6 @@ export const ToolGroupMessage: React.FC<ToolGroupMessageProps> = ({
            </Box>
            {showClosingBorder && (
              <Box
-                height={0}
                width={contentWidth}
                borderLeft={true}
                borderRight={true}
--- a/packages/cli/src/ui/components/messages/ToolMessage.tsx
+++ b/packages/cli/src/ui/components/messages/ToolMessage.tsx
@@ -119,7 +119,6 @@ export const ToolMessage: React.FC<ToolMessageProps> = ({
        borderLeft={true}
        borderRight={true}
        paddingX={1}
-        paddingTop={1}
        flexDirection="column"
      >
        {status === CoreToolCallStatus.Executing && progress !== undefined && (
--- 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<ToolResultDisplayProps> = ({

    // 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 (
        <Scrollable
--- a/packages/cli/src/ui/types.ts
+++ b/packages/cli/src/ui/types.ts
@@ -370,9 +370,6 @@ export type HistoryItemMcpStatus = HistoryItemBase & {
  showSchema: boolean;
 };

-// Using Omit<HistoryItem, 'id'> 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