Skip to content

Commit 5655b88

Browse files
committed
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
1 parent 1cee1d0 commit 5655b88

12 files changed

Lines changed: 84 additions & 97 deletions

docs/reference/configuration.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2402,7 +2402,7 @@ conventions and context.
24022402
loaded, allowing you to verify the hierarchy and content being used by the
24032403
AI.
24042404
- See the [Commands documentation](./commands.md#memory) for full details on
2405-
the `/memory` command and its sub-commands (`show` and `refresh`).
2405+
the `/memory` command and its sub-commands (`show` and `reload`).
24062406

24072407
By understanding and utilizing these configuration layers and the hierarchical
24082408
nature of context files, you can effectively manage the AI's memory and tailor

packages/cli/src/ui/AppContainer.tsx

Lines changed: 1 addition & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1757,7 +1757,6 @@ Logging in with Google... Restarting Gemini CLI to continue.
17571757
}
17581758

17591759
const toggleLastTurnTools = () => {
1760-
// If the user manually collapses/expands the view, show the hint and reset the x-second timer.
17611760
triggerExpandHint(true);
17621761

17631762
const targetToolCallIds = getLastTurnToolCallIds(
@@ -1824,13 +1823,7 @@ Logging in with Google... Restarting Gemini CLI to continue.
18241823
) {
18251824
setConstrainHeight(false);
18261825
toggleLastTurnTools();
1827-
1828-
// Force layout refresh after a short delay to allow the terminal layout to settle.
1829-
// Minimize "blank screen" issue after any async subview updates are complete.
1830-
setTimeout(() => {
1831-
refreshStatic();
1832-
}, 250);
1833-
1826+
refreshStatic();
18341827
return true;
18351828
} else if (
18361829
(keyMatchers[Command.FOCUS_SHELL_INPUT](key) ||

packages/cli/src/ui/components/StickyHeader.tsx

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,7 @@ export const StickyHeader: React.FC<StickyHeaderProps> = ({
6767
borderLeft={true}
6868
borderRight={true}
6969
paddingX={1}
70+
paddingBottom={1}
7071
paddingTop={isFirst ? 0 : 1}
7172
>
7273
{children}

packages/cli/src/ui/components/ToolConfirmationQueue.tsx

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -66,9 +66,9 @@ export const ToolConfirmationQueue: React.FC<ToolConfirmationQueueProps> = ({
6666

6767
// ToolConfirmationMessage needs to know the height available for its OWN content.
6868
// We subtract the lines used by the Queue wrapper:
69-
// - 2 lines for the rounded border (top/bottom)
69+
// - 2 lines for the rounded border
7070
// - 2 lines for the Header (text + margin)
71-
// - 2 lines for Tool Identity (text + margin) if shown
71+
// - 2 lines for Tool Identity (text + margin)
7272
const availableContentHeight = constrainHeight
7373
? Math.max(maxHeight - (hideToolIdentity ? 4 : 6), 4)
7474
: undefined;
@@ -83,7 +83,10 @@ export const ToolConfirmationQueue: React.FC<ToolConfirmationQueueProps> = ({
8383
>
8484
<Box flexDirection="column" width={mainAreaWidth - 4}>
8585
{/* Header */}
86-
<Box marginBottom={1} justifyContent="space-between">
86+
<Box
87+
marginBottom={hideToolIdentity ? 0 : 1}
88+
justifyContent="space-between"
89+
>
8790
<Text color={borderColor} bold>
8891
{getConfirmationHeader(tool.confirmationDetails)}
8992
</Text>
@@ -95,7 +98,7 @@ export const ToolConfirmationQueue: React.FC<ToolConfirmationQueueProps> = ({
9598
</Box>
9699

97100
{!hideToolIdentity && (
98-
<Box marginBottom={1}>
101+
<Box>
99102
<ToolStatusIndicator status={tool.status} name={tool.name} />
100103
<ToolInfo
101104
name={tool.name}

packages/cli/src/ui/components/messages/DenseToolMessage.test.tsx

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
/**
22
* @license
3-
* Copyright 2025 Google LLC
3+
* Copyright 2026 Google LLC
44
* SPDX-License-Identifier: Apache-2.0
55
*/
66

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

3637
it('renders correctly for a successful string result', async () => {

packages/cli/src/ui/components/messages/DenseToolMessage.tsx

Lines changed: 40 additions & 52 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
/**
22
* @license
3-
* Copyright 2025 Google LLC
3+
* Copyright 2026 Google LLC
44
* SPDX-License-Identifier: Apache-2.0
55
*/
66

@@ -18,7 +18,11 @@ import {
1818
isListResult,
1919
isReadManyFilesResult,
2020
} from '@google/gemini-cli-core';
21-
import { type IndividualToolCallDisplay, isTodoList } from '../../types.js';
21+
import {
22+
type IndividualToolCallDisplay,
23+
type ToolResultDisplay,
24+
isTodoList,
25+
} from '../../types.js';
2226
import { useAlternateBuffer } from '../../hooks/useAlternateBuffer.js';
2327
import { ToolStatusIndicator } from './ToolShared.js';
2428
import { theme } from '../../semantic-colors.js';
@@ -36,8 +40,13 @@ import { colorizeCode } from '../../utils/CodeColorizer.js';
3640
import { useToolActions } from '../../contexts/ToolActionsContext.js';
3741
import { getFileExtension } from '../../utils/fileUtils.js';
3842

43+
const PAYLOAD_MARGIN_LEFT = 6;
44+
const PAYLOAD_BORDER_CHROME_WIDTH = 4; // paddingX=1 (2 cols) + borders (2 cols)
45+
const PAYLOAD_SCROLL_GUTTER = 4;
46+
const PAYLOAD_MAX_WIDTH = 120 + PAYLOAD_SCROLL_GUTTER;
47+
3948
interface DenseToolMessageProps extends IndividualToolCallDisplay {
40-
terminalWidth?: number;
49+
terminalWidth: number;
4150
availableTerminalHeight?: number;
4251
}
4352

@@ -63,10 +72,6 @@ const hasPayload = (res: unknown): res is PayloadResult => {
6372
return typeof value === 'string';
6473
};
6574

66-
/**
67-
* --- RENDER HELPERS ---
68-
*/
69-
7075
const RenderItemsList: React.FC<{
7176
items?: string[];
7277
maxVisible?: number;
@@ -88,17 +93,13 @@ const RenderItemsList: React.FC<{
8893
);
8994
};
9095

91-
/**
92-
* --- SCENARIO LOGIC (Pure Functions) ---
93-
*/
94-
9596
function getFileOpData(
9697
diff: FileDiff,
9798
status: CoreToolCallStatus,
98-
resultDisplay: unknown,
99-
terminalWidth?: number,
100-
availableTerminalHeight?: number,
101-
isClickable?: boolean,
99+
resultDisplay: ToolResultDisplay | undefined,
100+
terminalWidth: number,
101+
availableTerminalHeight: number | undefined,
102+
isClickable: boolean,
102103
): ViewParts {
103104
const added =
104105
(diff.diffStat?.model_added_lines ?? 0) +
@@ -177,7 +178,7 @@ function getFileOpData(
177178
<DiffRenderer
178179
diffContent={diff.fileDiff}
179180
filename={diff.fileName}
180-
terminalWidth={terminalWidth ? terminalWidth - 6 : 80}
181+
terminalWidth={terminalWidth - PAYLOAD_MARGIN_LEFT}
181182
availableTerminalHeight={availableTerminalHeight}
182183
disableColor={status === CoreToolCallStatus.Cancelled}
183184
/>
@@ -201,26 +202,12 @@ function getReadManyFilesData(result: ReadManyFilesResult): ViewParts {
201202
skippedCount > 0 ? ` (${skippedCount} ignored)` : ''
202203
}`;
203204
const summary = <Text color={theme.text.accent}>{summaryStr}</Text>;
204-
205-
const excludedText =
206-
result.excludes && result.excludes.length > 0
207-
? `Excluded patterns: ${result.excludes.slice(0, 3).join(', ')}${
208-
result.excludes.length > 3 ? '...' : ''
209-
}`
210-
: undefined;
211-
212205
const hasItems = items.length > 0;
213-
const payload =
214-
hasItems || excludedText ? (
215-
<Box flexDirection="column" marginLeft={2}>
216-
{hasItems && <RenderItemsList items={items} maxVisible={maxVisible} />}
217-
{excludedText && (
218-
<Text color={theme.text.secondary} dimColor>
219-
{excludedText}
220-
</Text>
221-
)}
222-
</Box>
223-
) : undefined;
206+
const payload = hasItems ? (
207+
<Box flexDirection="column" marginLeft={2}>
208+
{hasItems && <RenderItemsList items={items} maxVisible={maxVisible} />}
209+
</Box>
210+
) : undefined;
224211

225212
return { description, summary, payload };
226213
}
@@ -309,10 +296,6 @@ function getGenericSuccessData(
309296
return { description, summary, payload };
310297
}
311298

312-
/**
313-
* --- MAIN COMPONENT ---
314-
*/
315-
316299
export const DenseToolMessage: React.FC<DenseToolMessageProps> = (props) => {
317300
const {
318301
callId,
@@ -339,7 +322,7 @@ export const DenseToolMessage: React.FC<DenseToolMessageProps> = (props) => {
339322
const [isFocused, setIsFocused] = useState(false);
340323
const toggleRef = useRef<DOMElement>(null);
341324

342-
// 1. Unified File Data Extraction (Safely bridge resultDisplay and confirmationDetails)
325+
// Unified File Data Extraction (Safely bridge resultDisplay and confirmationDetails)
343326
const diff = useMemo((): FileDiff | undefined => {
344327
if (isFileDiff(resultDisplay)) return resultDisplay;
345328
if (confirmationDetails?.type === 'edit') {
@@ -375,7 +358,7 @@ export const DenseToolMessage: React.FC<DenseToolMessageProps> = (props) => {
375358
isActive: isAlternateBuffer && !!diff,
376359
});
377360

378-
// 2. State-to-View Coordination
361+
// State-to-View Coordination
379362
const viewParts = useMemo((): ViewParts => {
380363
if (diff) {
381364
return getFileOpData(
@@ -459,7 +442,7 @@ export const DenseToolMessage: React.FC<DenseToolMessageProps> = (props) => {
459442
return colorizeCode({
460443
code: addedContent,
461444
language: fileExtension,
462-
maxWidth: terminalWidth ? terminalWidth - 6 : 80,
445+
maxWidth: terminalWidth - PAYLOAD_MARGIN_LEFT,
463446
settings,
464447
disableColor: status === CoreToolCallStatus.Cancelled,
465448
returnLines: true,
@@ -468,7 +451,7 @@ export const DenseToolMessage: React.FC<DenseToolMessageProps> = (props) => {
468451
return renderDiffLines({
469452
parsedLines,
470453
filename: diff.fileName,
471-
terminalWidth: terminalWidth ? terminalWidth - 6 : 80,
454+
terminalWidth: terminalWidth - PAYLOAD_MARGIN_LEFT,
472455
disableColor: status === CoreToolCallStatus.Cancelled,
473456
});
474457
}
@@ -502,7 +485,6 @@ export const DenseToolMessage: React.FC<DenseToolMessageProps> = (props) => {
502485
<Box minHeight={1}>{item}</Box>
503486
);
504487

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

530512
{showPayload && isAlternateBuffer && diffLines.length > 0 && (
531513
<Box
532-
marginLeft={6}
514+
marginLeft={PAYLOAD_MARGIN_LEFT}
533515
marginTop={1}
534516
marginBottom={1}
535517
paddingX={1}
@@ -541,30 +523,36 @@ export const DenseToolMessage: React.FC<DenseToolMessageProps> = (props) => {
541523
borderStyle="round"
542524
borderColor={theme.border.default}
543525
borderDimColor={true}
544-
maxWidth={terminalWidth ? Math.min(124, terminalWidth - 6) : 124}
526+
maxWidth={Math.min(
527+
PAYLOAD_MAX_WIDTH,
528+
terminalWidth - PAYLOAD_MARGIN_LEFT,
529+
)}
545530
>
546531
<ScrollableList
547532
data={diffLines}
548533
renderItem={renderItem}
549534
keyExtractor={keyExtractor}
550535
estimatedItemHeight={() => 1}
551536
hasFocus={isFocused}
552-
width={
553-
// adjustment: 6 margin - 4 padding/border - 4 right-scroll-gutter
554-
terminalWidth ? Math.min(120, terminalWidth - 6 - 4 - 4) : 70
555-
}
537+
width={Math.min(
538+
PAYLOAD_MAX_WIDTH,
539+
terminalWidth -
540+
PAYLOAD_MARGIN_LEFT -
541+
PAYLOAD_BORDER_CHROME_WIDTH -
542+
PAYLOAD_SCROLL_GUTTER,
543+
)}
556544
/>
557545
</Box>
558546
)}
559547

560548
{showPayload && (!isAlternateBuffer || !diff) && viewParts.payload && (
561-
<Box marginLeft={6} marginTop={1} marginBottom={1}>
549+
<Box marginLeft={PAYLOAD_MARGIN_LEFT} marginTop={1} marginBottom={1}>
562550
{viewParts.payload}
563551
</Box>
564552
)}
565553

566554
{showPayload && outputFile && (
567-
<Box marginLeft={6} marginTop={1} marginBottom={1}>
555+
<Box marginLeft={PAYLOAD_MARGIN_LEFT} marginTop={1} marginBottom={1}>
568556
<Text color={theme.text.secondary}>
569557
(Output saved to: {outputFile})
570558
</Text>

packages/cli/src/ui/components/messages/DiffRenderer.tsx

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,6 @@
77
import type React from 'react';
88
import { useMemo } from 'react';
99
import { Box, Text, useIsScreenReaderEnabled } from 'ink';
10-
import crypto from 'node:crypto';
1110
import { colorizeCode, colorizeLine } from '../../utils/CodeColorizer.js';
1211
import { MaxSizedBox } from '../shared/MaxSizedBox.js';
1312
import { theme as semanticTheme } from '../../semantic-colors.js';
@@ -165,9 +164,7 @@ export const DiffRenderer: React.FC<DiffRendererProps> = ({
165164
disableColor,
166165
});
167166
} else {
168-
const key = filename
169-
? `diff-box-${filename}`
170-
: `diff-box-${crypto.createHash('sha1').update(JSON.stringify(parsedLines)).digest('hex')}`;
167+
const key = filename ? `diff-box-${filename}` : undefined;
171168

172169
return (
173170
<MaxSizedBox

packages/cli/src/ui/components/messages/ShellToolMessage.tsx

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -190,7 +190,6 @@ export const ShellToolMessage: React.FC<ShellToolMessageProps> = ({
190190
borderLeft={true}
191191
borderRight={true}
192192
paddingX={1}
193-
paddingTop={1}
194193
flexDirection="column"
195194
>
196195
<ToolResultDisplay

0 commit comments

Comments
 (0)