From 22ea05e35b6f2fcfc57d703810d8fcf6b9edb218 Mon Sep 17 00:00:00 2001 From: Jarrod Whelan Date: Tue, 10 Feb 2026 20:40:12 -0800 Subject: [PATCH] feat(cli): refactor dense tool output to modular summary+payload pattern Introduces a modular architecture for compact tool output in the CLI. - Separates single-line summaries from multiline payloads across all tools. - Implements specialized handlers for file operations and ReadManyFiles. - Passes terminal dimensions to ensure accurate diff rendering in dense mode. - Ensures state persistence for rejected operations in history. - Includes unit tests for all new layouts, result types, and terminal states. --- .../messages/DenseToolMessage.test.tsx | 201 +++++++++- .../components/messages/DenseToolMessage.tsx | 349 +++++++++++++++--- .../components/messages/ToolGroupMessage.tsx | 9 +- .../messages/ToolResultDisplay.test.tsx | 52 +++ packages/cli/src/ui/hooks/toolMapping.ts | 3 + packages/cli/src/ui/hooks/useGeminiStream.ts | 23 +- packages/cli/src/ui/types.ts | 1 + 7 files changed, 577 insertions(+), 61 deletions(-) diff --git a/packages/cli/src/ui/components/messages/DenseToolMessage.test.tsx b/packages/cli/src/ui/components/messages/DenseToolMessage.test.tsx index 404a8630c6..f255a1a690 100644 --- a/packages/cli/src/ui/components/messages/DenseToolMessage.test.tsx +++ b/packages/cli/src/ui/components/messages/DenseToolMessage.test.tsx @@ -7,8 +7,12 @@ import { describe, it, expect } from 'vitest'; import { renderWithProviders } from '../../../test-utils/render.js'; import { DenseToolMessage } from './DenseToolMessage.js'; -import type { ToolResultDisplay } from '../../types.js'; import { ToolCallStatus } from '../../types.js'; +import type { + FileDiff, + SerializableConfirmationDetails, + ToolResultDisplay, +} from '../../types.js'; describe('DenseToolMessage', () => { const defaultProps = { @@ -49,19 +53,208 @@ describe('DenseToolMessage', () => { expect(output).toContain('→ Line 1 Line 2'); }); - it('renders correctly for file diff results', () => { + it('renders correctly for file diff results with stats', () => { const diffResult = { - fileDiff: 'diff content', + fileDiff: '@@ -1,1 +1,1 @@\n-old line\n+diff content', fileName: 'test.ts', filePath: '/path/to/test.ts', originalContent: 'old content', newContent: 'new content', + diffStat: { + user_added_lines: 5, + user_removed_lines: 2, + user_added_chars: 50, + user_removed_chars: 20, + model_added_lines: 10, + model_removed_lines: 4, + model_added_chars: 100, + model_removed_chars: 40, + }, }; const { lastFrame } = renderWithProviders( , ); const output = lastFrame(); - expect(output).toContain('→ Diff applied to test.ts'); + expect(output).toContain('test.ts (+15, -6) → Accepted'); + expect(output).toContain('diff content'); + }); + + it('renders correctly for Edit tool using confirmationDetails', () => { + const confirmationDetails: SerializableConfirmationDetails = { + type: 'edit', + title: 'Confirm Edit', + fileName: 'styles.scss', + filePath: '/path/to/styles.scss', + fileDiff: + '@@ -1,1 +1,1 @@\n-body { color: blue; }\n+body { color: red; }', + originalContent: 'body { color: blue; }', + newContent: 'body { color: red; }', + }; + const { lastFrame } = renderWithProviders( + , + ); + const output = lastFrame(); + expect(output).toContain('Edit'); + expect(output).toContain('styles.scss'); + expect(output).toContain('→ Confirming'); + expect(output).toContain('body { color: red; }'); + }); + + it('renders correctly for Rejected Edit tool', () => { + const diffResult: FileDiff = { + fileDiff: '@@ -1,1 +1,1 @@\n-old line\n+new line', + fileName: 'styles.scss', + filePath: '/path/to/styles.scss', + originalContent: 'old line', + newContent: 'new line', + }; + const { lastFrame } = renderWithProviders( + , + ); + const output = lastFrame(); + expect(output).toContain('Edit'); + expect(output).toContain('styles.scss'); + expect(output).toContain('→ Rejected'); + expect(output).toContain('- old line'); + expect(output).toContain('+ new line'); + }); + + it('renders correctly for WriteFile tool', () => { + const diffResult: FileDiff = { + fileDiff: '@@ -1,1 +1,1 @@\n-old content\n+new content', + fileName: 'config.json', + filePath: '/path/to/config.json', + originalContent: 'old content', + newContent: 'new content', + diffStat: { + user_added_lines: 1, + user_removed_lines: 1, + user_added_chars: 0, + user_removed_chars: 0, + model_added_lines: 0, + model_removed_lines: 0, + model_added_chars: 0, + model_removed_chars: 0, + }, + }; + const { lastFrame } = renderWithProviders( + , + ); + const output = lastFrame(); + expect(output).toContain('WriteFile'); + expect(output).toContain('config.json (+1, -1)'); + expect(output).toContain('→ Accepted'); + expect(output).toContain('+ new content'); + }); + + it('renders correctly for Rejected WriteFile tool', () => { + const diffResult: FileDiff = { + fileDiff: '@@ -1,1 +1,1 @@\n-old content\n+new content', + fileName: 'config.json', + filePath: '/path/to/config.json', + originalContent: 'old content', + newContent: 'new content', + }; + const { lastFrame } = renderWithProviders( + , + ); + const output = lastFrame(); + expect(output).toContain('WriteFile'); + expect(output).toContain('config.json'); + expect(output).toContain('→ Rejected'); + expect(output).toContain('- old content'); + expect(output).toContain('+ new content'); + }); + + it('renders correctly for Errored Edit tool', () => { + const diffResult: FileDiff = { + fileDiff: '@@ -1,1 +1,1 @@\n-old line\n+new line', + fileName: 'styles.scss', + filePath: '/path/to/styles.scss', + originalContent: 'old line', + newContent: 'new line', + }; + const { lastFrame } = renderWithProviders( + , + ); + const output = lastFrame(); + expect(output).toContain('Edit'); + expect(output).toContain('styles.scss'); + expect(output).toContain('→ Failed'); + }); + + it('renders correctly for grep results', () => { + const grepResult = { + summary: 'Found 2 matches', + matches: [ + { filePath: 'file1.ts', lineNumber: 10, line: 'match 1' }, + { filePath: 'file2.ts', lineNumber: 20, line: 'match 2' }, + ], + }; + const { lastFrame } = renderWithProviders( + , + ); + const output = lastFrame(); + expect(output).toContain('→ Found 2 matches'); + expect(output).toContain('file1.ts:10: match 1'); + expect(output).toContain('file2.ts:20: match 2'); + }); + + it('renders correctly for ls results', () => { + const lsResult = { + summary: 'Listed 2 files', + files: ['file1.ts', 'dir1'], + }; + const { lastFrame } = renderWithProviders( + , + ); + const output = lastFrame(); + expect(output).toContain('→ Listed 2 files'); + expect(output).toContain('file1.ts'); + expect(output).toContain('dir1'); + }); + + it('renders correctly for ReadManyFiles results', () => { + const rmfResult = { + summary: 'Read 3 file(s)', + files: ['file1.ts', 'file2.ts', 'file3.ts'], + skipped: [{ path: 'skipped.bin', reason: 'binary' }], + }; + const { lastFrame } = renderWithProviders( + , + ); + const output = lastFrame(); + expect(output).toContain('→ Read 3 file(s)'); + expect(output).toContain('file1.ts'); + expect(output).toContain('file2.ts'); + expect(output).toContain('file3.ts'); + expect(output).toContain('(1 skipped)'); }); it('renders correctly for todo updates', () => { diff --git a/packages/cli/src/ui/components/messages/DenseToolMessage.tsx b/packages/cli/src/ui/components/messages/DenseToolMessage.tsx index f1d713ead0..67af65af63 100644 --- a/packages/cli/src/ui/components/messages/DenseToolMessage.tsx +++ b/packages/cli/src/ui/components/messages/DenseToolMessage.tsx @@ -5,60 +5,326 @@ */ import type React from 'react'; +import { useMemo } from 'react'; import { Box, Text } from 'ink'; import { ToolCallStatus } from '../../types.js'; -import type { IndividualToolCallDisplay } from '../../types.js'; +import type { + IndividualToolCallDisplay, + FileDiff, + GrepResult, + ListDirectoryResult, + ReadManyFilesResult, +} from '../../types.js'; import { ToolStatusIndicator } from './ToolShared.js'; import { theme } from '../../semantic-colors.js'; +import { DiffRenderer } from './DiffRenderer.js'; -type DenseToolMessageProps = IndividualToolCallDisplay; - -interface FileDiffResult { - fileDiff: string; - fileName: string; +interface DenseToolMessageProps extends IndividualToolCallDisplay { + terminalWidth?: number; + availableTerminalHeight?: number; } -export const DenseToolMessage: React.FC = ({ - name, - description, - status, - resultDisplay, - outputFile, -}) => { - let denseResult: string | undefined; +interface ViewParts { + description?: string; + summary?: React.ReactNode; + payload?: React.ReactNode; +} - if (status === ToolCallStatus.Success && resultDisplay) { - if (typeof resultDisplay === 'string') { - const flattened = resultDisplay.replace(/\n/g, ' ').trim(); - denseResult = - flattened.length > 120 ? flattened.slice(0, 117) + '...' : flattened; - } else if (typeof resultDisplay === 'object') { - if ('fileDiff' in resultDisplay) { - denseResult = `Diff applied to ${(resultDisplay as FileDiffResult).fileName}`; - } else if ('todos' in resultDisplay) { - denseResult = 'Todos updated'; - } else { - // Fallback for AnsiOutput or other objects - denseResult = 'Output received'; - } - } +/** + * --- TYPE GUARDS --- + */ +const isFileDiff = (res: unknown): res is FileDiff => + typeof res === 'object' && res !== null && 'fileDiff' in res; + +const isGrepResult = (res: unknown): res is GrepResult => + typeof res === 'object' && + res !== null && + 'matches' in res && + 'summary' in res; + +const isListResult = ( + res: unknown, +): res is ListDirectoryResult | ReadManyFilesResult => + typeof res === 'object' && res !== null && 'files' in res && 'summary' in res; + +const hasPayload = ( + res: unknown, +): res is { summary: string; payload: string } => + typeof res === 'object' && + res !== null && + 'payload' in res && + 'summary' in res; + +const isTodoList = (res: unknown): res is { todos: unknown[] } => + typeof res === 'object' && res !== null && 'todos' in res; + +/** + * --- RENDER HELPERS --- + */ + +const RenderItemsList: React.FC<{ + items: string[]; + maxVisible?: number; +}> = ({ items, maxVisible = 20 }) => ( + + {items.slice(0, maxVisible).map((item, i) => ( + + {item} + + ))} + {items.length > maxVisible && ( + + ... and {items.length - maxVisible} more + + )} + +); + +/** + * --- SCENARIO LOGIC (Pure Functions) --- + */ + +function getFileOpData( + diff: FileDiff, + status: ToolCallStatus, + resultDisplay: unknown, + terminalWidth?: number, + availableTerminalHeight?: number, +): ViewParts { + const added = + (diff.diffStat?.model_added_lines ?? 0) + + (diff.diffStat?.user_added_lines ?? 0); + const removed = + (diff.diffStat?.model_removed_lines ?? 0) + + (diff.diffStat?.user_removed_lines ?? 0); + const stats = diff.diffStat ? ` (+${added}, -${removed})` : ''; + + const description = `${diff.fileName}${stats}`; + let decision = ''; + let decisionColor = theme.text.secondary; + + if ( + status === ToolCallStatus.Success || + status === ToolCallStatus.Executing + ) { + decision = 'Accepted'; + decisionColor = theme.text.accent; + } else if (status === ToolCallStatus.Canceled) { + decision = 'Rejected'; + decisionColor = theme.text.primary; + } else if (status === ToolCallStatus.Confirming) { + decision = 'Confirming'; } else if (status === ToolCallStatus.Error) { - if (typeof resultDisplay === 'string') { - const flattened = resultDisplay.replace(/\n/g, ' ').trim(); - denseResult = - flattened.length > 120 ? flattened.slice(0, 117) + '...' : flattened; - } else { - denseResult = 'Failed'; - } + decision = typeof resultDisplay === 'string' ? resultDisplay : 'Failed'; + decisionColor = theme.text.accent; } + const summary = decision ? ( + + → {decision.replace(/\n/g, ' ')} + + ) : undefined; + + const payload = ( + + ); + + return { description, summary, payload }; +} + +function getListResultData( + result: ListDirectoryResult | ReadManyFilesResult, + toolName: string, + originalDescription?: string, +): ViewParts { + let description = originalDescription; + const items: string[] = result.files; + const maxVisible = 20; + + // Enhance with ReadManyFiles specific data if present + const rmf = result as ReadManyFilesResult; + if (toolName === 'ReadManyFiles' && rmf.include) { + const includePatterns = rmf.include.join(', '); + description = `Attempting to read files from ${includePatterns}`; + } + + const summary = → {result.summary}; + + const skippedCount = rmf.skipped?.length ?? 0; + const skippedText = + skippedCount > 0 ? `(${skippedCount} skipped)` : undefined; + + const excludedText = + rmf.excludes && rmf.excludes.length > 0 + ? `Excluded patterns: ${rmf.excludes.slice(0, 3).join(', ')}${rmf.excludes.length > 3 ? '...' : ''}` + : undefined; + + const payload = ( + + + {skippedText && ( + + {skippedText} + + )} + {excludedText && ( + + {excludedText} + + )} + + ); + + return { description, summary, payload }; +} + +function getGenericSuccessData( + resultDisplay: unknown, + originalDescription?: string, +): ViewParts { + let summary: React.ReactNode; + let payload: React.ReactNode; + + if (typeof resultDisplay === 'string') { + const flattened = resultDisplay.replace(/\n/g, ' ').trim(); + summary = ( + + → {flattened.length > 120 ? flattened.slice(0, 117) + '...' : flattened} + + ); + } else if (isGrepResult(resultDisplay)) { + summary = → {resultDisplay.summary}; + payload = ( + + `${m.filePath}:${m.lineNumber}: ${m.line.trim()}`, + )} + maxVisible={10} + /> + + ); + } else if (isTodoList(resultDisplay)) { + summary = ( + + → Todos updated + + ); + } else if (hasPayload(resultDisplay)) { + summary = → {resultDisplay.summary}; + payload = ( + + {resultDisplay.payload} + + ); + } else { + summary = ( + + → Output received + + ); + } + + return { description: originalDescription, summary, payload }; +} + +/** + * --- MAIN COMPONENT --- + */ + +export const DenseToolMessage: React.FC = (props) => { + const { + name, + status, + resultDisplay, + confirmationDetails, + outputFile, + terminalWidth, + availableTerminalHeight, + description: originalDescription, + } = props; + + // 1. Unified File Data Extraction (Safely bridge resultDisplay and confirmationDetails) + const diff = useMemo((): FileDiff | undefined => { + if (isFileDiff(resultDisplay)) return resultDisplay; + if (confirmationDetails?.type === 'edit') { + return { + fileName: confirmationDetails.fileName, + fileDiff: confirmationDetails.fileDiff, + filePath: confirmationDetails.filePath, + originalContent: confirmationDetails.originalContent, + newContent: confirmationDetails.newContent, + diffStat: confirmationDetails.diffStat, + }; + } + return undefined; + }, [resultDisplay, confirmationDetails]); + + // 2. State-to-View Coordination + const viewParts = useMemo((): ViewParts => { + if (diff) { + return getFileOpData( + diff, + status, + resultDisplay, + terminalWidth, + availableTerminalHeight, + ); + } + if (isListResult(resultDisplay)) { + return getListResultData(resultDisplay, name, originalDescription); + } + if (status === ToolCallStatus.Success && resultDisplay) { + return getGenericSuccessData(resultDisplay, originalDescription); + } + if (status === ToolCallStatus.Error) { + const text = + typeof resultDisplay === 'string' + ? resultDisplay.replace(/\n/g, ' ') + : 'Failed'; + const errorSummary = ( + + → {text.length > 120 ? text.slice(0, 117) + '...' : text} + + ); + return { + description: originalDescription, + summary: errorSummary, + payload: undefined, + }; + } + + return { + description: originalDescription, + summary: undefined, + payload: undefined, + }; + }, [ + diff, + status, + resultDisplay, + name, + terminalWidth, + availableTerminalHeight, + originalDescription, + ]); + + const { description, summary, payload } = viewParts; + + // 3. Final Layout return ( - + - {name} + {name}{' '} @@ -66,14 +332,13 @@ export const DenseToolMessage: React.FC = ({ {description} - {denseResult && ( + {summary && ( - - → {denseResult} - + {summary} )} + {payload && {payload}} {outputFile && ( diff --git a/packages/cli/src/ui/components/messages/ToolGroupMessage.tsx b/packages/cli/src/ui/components/messages/ToolGroupMessage.tsx index 339dc5ca0e..5fa6a6f34f 100644 --- a/packages/cli/src/ui/components/messages/ToolGroupMessage.tsx +++ b/packages/cli/src/ui/components/messages/ToolGroupMessage.tsx @@ -191,7 +191,14 @@ export const ToolGroupMessage: React.FC = ({ tool.status !== ToolCallStatus.Confirming; if (useDenseView) { - return ; + return ( + + ); } const commonProps = { diff --git a/packages/cli/src/ui/components/messages/ToolResultDisplay.test.tsx b/packages/cli/src/ui/components/messages/ToolResultDisplay.test.tsx index 797e405b62..b5f150ad8f 100644 --- a/packages/cli/src/ui/components/messages/ToolResultDisplay.test.tsx +++ b/packages/cli/src/ui/components/messages/ToolResultDisplay.test.tsx @@ -301,4 +301,56 @@ describe('ToolResultDisplay', () => { expect(output).not.toContain('Line 1'); expect(output).toContain('Line 50'); }); + + it('renders GrepResult as summary string', () => { + const grepResult = { + summary: 'Found 5 matches', + matches: [{ filePath: 'test.ts', lineNumber: 1, line: 'code' }], + }; + const { lastFrame } = render( + , + ); + const output = lastFrame(); + expect(output).toContain('Found 5 matches'); + expect(output).not.toContain('filePath'); // Should not render the array content + expect(output).not.toContain('code'); + }); + + it('renders ListDirectoryResult as summary string', () => { + const lsResult = { + summary: 'Listed 10 files', + files: ['some-file.txt'], + }; + const { lastFrame } = render( + , + ); + const output = lastFrame(); + expect(output).toContain('Listed 10 files'); + expect(output).not.toContain('some-file.txt'); // Should not render the array content + }); + + it('renders ReadManyFilesResult as summary string', () => { + const rmfResult = { + summary: 'Read 20 files', + files: ['f1.txt', 'f2.txt'], + }; + const { lastFrame } = render( + , + ); + const output = lastFrame(); + expect(output).toContain('Read 20 files'); + expect(output).not.toContain('f1.txt'); + }); }); diff --git a/packages/cli/src/ui/hooks/toolMapping.ts b/packages/cli/src/ui/hooks/toolMapping.ts index e83fb583bf..58965d4948 100644 --- a/packages/cli/src/ui/hooks/toolMapping.ts +++ b/packages/cli/src/ui/hooks/toolMapping.ts @@ -92,6 +92,9 @@ export function mapToDisplay( case 'error': case 'cancelled': resultDisplay = call.response.resultDisplay; + // Preserve confirmation details so we can still show what was being proposed + // if it was an edit/write-file that was rejected. + confirmationDetails = call.confirmationDetails; break; case 'awaiting_approval': correlationId = call.correlationId; diff --git a/packages/cli/src/ui/hooks/useGeminiStream.ts b/packages/cli/src/ui/hooks/useGeminiStream.ts index 6f96dd4ef3..c919c7923c 100644 --- a/packages/cli/src/ui/hooks/useGeminiStream.ts +++ b/packages/cli/src/ui/hooks/useGeminiStream.ts @@ -64,7 +64,6 @@ import { findLastSafeSplitPoint } from '../utils/markdownUtilities.js'; import { useStateAndRef } from './useStateAndRef.js'; import type { UseHistoryManagerReturn } from './useHistoryManager.js'; import { useLogger } from './useLogger.js'; -import { SHELL_COMMAND_NAME } from '../constants.js'; import { mapToDisplay as mapTrackedToolCallsToDisplay } from './toolMapping.js'; import { useToolScheduler, @@ -555,23 +554,19 @@ export const useGeminiStream = ( cancelAllToolCalls(abortControllerRef.current.signal); if (pendingHistoryItemRef.current) { - const isShellCommand = - pendingHistoryItemRef.current.type === 'tool_group' && - pendingHistoryItemRef.current.tools.some( - (t) => t.name === SHELL_COMMAND_NAME, - ); - - // If it is a shell command, we update the status to Canceled and clear the output - // to avoid artifacts, then add it to history immediately. - if (isShellCommand) { - // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion - const toolGroup = pendingHistoryItemRef.current as HistoryItemToolGroup; + if (pendingHistoryItemRef.current.type === 'tool_group') { + // Mark all in-progress tools as Canceled when the turn is cancelled. + + const toolGroup = pendingHistoryItemRef.current; const updatedTools = toolGroup.tools.map((tool) => { - if (tool.name === SHELL_COMMAND_NAME) { + if ( + tool.status === ToolCallStatus.Pending || + tool.status === ToolCallStatus.Confirming || + tool.status === ToolCallStatus.Executing + ) { return { ...tool, status: ToolCallStatus.Canceled, - resultDisplay: tool.resultDisplay, }; } return tool; diff --git a/packages/cli/src/ui/types.ts b/packages/cli/src/ui/types.ts index ef89ff8b37..143ff112f4 100644 --- a/packages/cli/src/ui/types.ts +++ b/packages/cli/src/ui/types.ts @@ -31,6 +31,7 @@ export type { ListDirectoryResult, ReadManyFilesResult, FileDiff, + SerializableConfirmationDetails, }; export enum AuthState {