From 9c46acc793df3573e6fbcf53ac3e46663494e410 Mon Sep 17 00:00:00 2001 From: Taylor Mullen Date: Thu, 15 May 2025 22:56:03 -0700 Subject: [PATCH] Refactor: Improve UI rendering and address code review comments This commit addresses several code review comments primarily focused on improving the rendering and stability of the CLI UI. Key changes include: - Passing `isPending` and `availableTerminalHeight` props to `MarkdownDisplay` to enable more intelligent rendering of content, especially for pending messages and code blocks. - Adjusting height calculations in `ToolGroupMessage` and `ToolMessage` to more accurately reflect available space. - Refining the logic in `App.tsx` for measuring and utilizing terminal height, including renaming `footerRef` to `mainControlsRef` for clarity. - Ensuring consistent prop drilling for `isPending` and `availableTerminalHeight` through `HistoryItemDisplay`, `GeminiMessage`, and `GeminiMessageContent`. - In `MarkdownDisplay`, when `isPending` is true and content exceeds `availableTerminalHeight`, the code block will now be truncated with a "... generating more ..." message. If there's insufficient space even for the message, a simpler "... code is being written ..." will be shown. --- packages/cli/src/ui/App.tsx | 42 +++++++++------- .../src/ui/components/HistoryItemDisplay.tsx | 16 +++++- .../ui/components/messages/GeminiMessage.tsx | 14 +++++- .../messages/GeminiMessageContent.tsx | 10 +++- .../components/messages/ToolGroupMessage.tsx | 3 +- .../ui/components/messages/ToolMessage.tsx | 9 ++-- packages/cli/src/ui/utils/MarkdownDisplay.tsx | 49 ++++++++++++++++++- 7 files changed, 114 insertions(+), 29 deletions(-) diff --git a/packages/cli/src/ui/App.tsx b/packages/cli/src/ui/App.tsx index b6275491..e607955a 100644 --- a/packages/cli/src/ui/App.tsx +++ b/packages/cli/src/ui/App.tsx @@ -5,8 +5,9 @@ */ import { useCallback, useEffect, useMemo, useState, useRef } from 'react'; -import { Box, DOMElement, measureElement, Static, Text, useStdout } from 'ink'; +import { Box, DOMElement, measureElement, Static, Text } from 'ink'; import { StreamingState, type HistoryItem } from './types.js'; +import { useTerminalSize } from './hooks/useTerminalSize.js'; import { useGeminiStream } from './hooks/useGeminiStream.js'; import { useLoadingIndicator } from './hooks/useLoadingIndicator.js'; import { useThemeCommand } from './hooks/useThemeCommand.js'; @@ -56,8 +57,7 @@ export const App = ({ const [debugMessage, setDebugMessage] = useState(''); const [showHelp, setShowHelp] = useState(false); const [themeError, setThemeError] = useState(null); - const [availableTerminalHeight, setAvailableTerminalHeight] = - useState(0); + const [footerHeight, setFooterHeight] = useState(0); const { isThemeDialogOpen, openThemeDialog, @@ -195,26 +195,24 @@ export const App = ({ // --- Render Logic --- - // Get terminal dimensions - - const { stdout } = useStdout(); - const terminalWidth = stdout?.columns ?? 80; - const terminalHeight = stdout?.rows ?? 24; - const footerRef = useRef(null); + const { rows: terminalHeight, columns: terminalWidth } = useTerminalSize(); + const mainControlsRef = useRef(null); const pendingHistoryItemRef = useRef(null); // Calculate width for suggestions, leave some padding const suggestionsWidth = Math.max(60, Math.floor(terminalWidth * 0.8)); useEffect(() => { - const staticExtraHeight = /* margins and padding */ 3; - const fullFooterMeasurement = measureElement(footerRef.current!); - const fullFooterHeight = fullFooterMeasurement.height; + if (mainControlsRef.current) { + const fullFooterMeasurement = measureElement(mainControlsRef.current); + setFooterHeight(fullFooterMeasurement.height); + } + }, [terminalHeight]); // Re-calculate if terminalHeight changes, as it might affect footer's rendered height. - setAvailableTerminalHeight( - terminalHeight - fullFooterHeight - staticExtraHeight, - ); - }, [terminalHeight]); + const availableTerminalHeight = useMemo(() => { + const staticExtraHeight = /* margins and padding */ 3; + return terminalHeight - footerHeight - staticExtraHeight; + }, [terminalHeight, footerHeight]); useEffect(() => { if (!pendingHistoryItem) { @@ -260,7 +258,14 @@ export const App = ({
, - ...history.map((h) => ), + ...history.map((h) => ( + + )), ]} > {(item) => item} @@ -272,12 +277,13 @@ export const App = ({ // TODO(taehykim): It seems like references to ids aren't necessary in // HistoryItemDisplay. Refactor later. Use a fake id for now. item={{ ...pendingHistoryItem, id: 0 }} + isPending={true} /> )} {showHelp && } - + {startupWarnings.length > 0 && ( = ({ item, availableTerminalHeight, + isPending, }) => ( {/* Render standard message types */} {item.type === 'user' && } - {item.type === 'gemini' && } + {item.type === 'gemini' && ( + + )} {item.type === 'gemini_content' && ( - + )} {item.type === 'info' && } {item.type === 'error' && } diff --git a/packages/cli/src/ui/components/messages/GeminiMessage.tsx b/packages/cli/src/ui/components/messages/GeminiMessage.tsx index b2c816a9..df8d0a87 100644 --- a/packages/cli/src/ui/components/messages/GeminiMessage.tsx +++ b/packages/cli/src/ui/components/messages/GeminiMessage.tsx @@ -11,9 +11,15 @@ import { Colors } from '../../colors.js'; interface GeminiMessageProps { text: string; + isPending: boolean; + availableTerminalHeight: number; } -export const GeminiMessage: React.FC = ({ text }) => { +export const GeminiMessage: React.FC = ({ + text, + isPending, + availableTerminalHeight, +}) => { const prefix = '✦ '; const prefixWidth = prefix.length; @@ -23,7 +29,11 @@ export const GeminiMessage: React.FC = ({ text }) => { {prefix} - + ); diff --git a/packages/cli/src/ui/components/messages/GeminiMessageContent.tsx b/packages/cli/src/ui/components/messages/GeminiMessageContent.tsx index b9b85dc7..da6e468a 100644 --- a/packages/cli/src/ui/components/messages/GeminiMessageContent.tsx +++ b/packages/cli/src/ui/components/messages/GeminiMessageContent.tsx @@ -10,6 +10,8 @@ import { MarkdownDisplay } from '../../utils/MarkdownDisplay.js'; interface GeminiMessageContentProps { text: string; + isPending: boolean; + availableTerminalHeight: number; } /* @@ -20,13 +22,19 @@ interface GeminiMessageContentProps { */ export const GeminiMessageContent: React.FC = ({ text, + isPending, + availableTerminalHeight, }) => { const originalPrefix = '✦ '; const prefixWidth = originalPrefix.length; return ( - + ); }; diff --git a/packages/cli/src/ui/components/messages/ToolGroupMessage.tsx b/packages/cli/src/ui/components/messages/ToolGroupMessage.tsx index 33460405..d0ad1c5f 100644 --- a/packages/cli/src/ui/components/messages/ToolGroupMessage.tsx +++ b/packages/cli/src/ui/components/messages/ToolGroupMessage.tsx @@ -29,7 +29,6 @@ export const ToolGroupMessage: React.FC = ({ const borderColor = hasPending ? Colors.AccentYellow : Colors.SubtleComment; const staticHeight = /* border */ 2 + /* marginBottom */ 1; - availableTerminalHeight -= staticHeight; return ( = ({ resultDisplay={tool.resultDisplay} status={tool.status} confirmationDetails={tool.confirmationDetails} - availableTerminalHeight={availableTerminalHeight} + availableTerminalHeight={availableTerminalHeight - staticHeight} /> {tool.status === ToolCallStatus.Confirming && tool.confirmationDetails && ( diff --git a/packages/cli/src/ui/components/messages/ToolMessage.tsx b/packages/cli/src/ui/components/messages/ToolMessage.tsx index 220578de..091785f2 100644 --- a/packages/cli/src/ui/components/messages/ToolMessage.tsx +++ b/packages/cli/src/ui/components/messages/ToolMessage.tsx @@ -26,7 +26,6 @@ export const ToolMessage: React.FC = ({ const statusIndicatorWidth = 3; const hasResult = resultDisplay && resultDisplay.toString().trim().length > 0; const staticHeight = /* Header */ 1; - availableTerminalHeight -= staticHeight; let displayableResult = resultDisplay; let hiddenLines = 0; @@ -37,7 +36,7 @@ export const ToolMessage: React.FC = ({ const lines = resultDisplay.split('\n'); // Estimate available height for this specific tool message content area // This is a rough estimate; ideally, we'd have a more precise measurement. - const contentHeightEstimate = availableTerminalHeight - 5; // Subtracting lines for tool name, status, padding etc. + const contentHeightEstimate = availableTerminalHeight - staticHeight - 5; // Subtracting lines for tool name, status, padding etc. if (lines.length > contentHeightEstimate && contentHeightEstimate > 0) { displayableResult = lines.slice(0, contentHeightEstimate).join('\n'); hiddenLines = lines.length - contentHeightEstimate; @@ -83,7 +82,11 @@ export const ToolMessage: React.FC = ({ {typeof displayableResult === 'string' && ( - + )} {typeof displayableResult === 'object' && ( diff --git a/packages/cli/src/ui/utils/MarkdownDisplay.tsx b/packages/cli/src/ui/utils/MarkdownDisplay.tsx index 63888a8b..83fb46ee 100644 --- a/packages/cli/src/ui/utils/MarkdownDisplay.tsx +++ b/packages/cli/src/ui/utils/MarkdownDisplay.tsx @@ -11,6 +11,8 @@ import { colorizeCode } from './CodeColorizer.js'; interface MarkdownDisplayProps { text: string; + isPending: boolean; + availableTerminalHeight: number; } // Constants for Markdown parsing and rendering @@ -26,7 +28,11 @@ const CODE_BLOCK_PADDING = 1; const LIST_ITEM_PREFIX_PADDING = 1; const LIST_ITEM_TEXT_FLEX_GROW = 1; -const MarkdownDisplayInternal: React.FC = ({ text }) => { +const MarkdownDisplayInternal: React.FC = ({ + text, + isPending, + availableTerminalHeight, +}) => { if (!text) return <>; const lines = text.split('\n'); @@ -57,6 +63,8 @@ const MarkdownDisplayInternal: React.FC = ({ text }) => { key={key} content={codeBlockContent} lang={codeBlockLang} + isPending={isPending} + availableTerminalHeight={availableTerminalHeight} />, ); inCodeBlock = false; @@ -176,6 +184,8 @@ const MarkdownDisplayInternal: React.FC = ({ text }) => { key="line-eof" content={codeBlockContent} lang={codeBlockLang} + isPending={isPending} + availableTerminalHeight={availableTerminalHeight} />, ); } @@ -317,12 +327,49 @@ const RenderInline = React.memo(RenderInlineInternal); interface RenderCodeBlockProps { content: string[]; lang: string | null; + isPending: boolean; + availableTerminalHeight: number; } const RenderCodeBlockInternal: React.FC = ({ content, lang, + isPending, + availableTerminalHeight, }) => { + const MIN_LINES_FOR_MESSAGE = 1; // Minimum lines to show before the "generating more" message + const RESERVED_LINES = 2; // Lines reserved for the message itself and potential padding + const MAX_CODE_LINES_WHEN_PENDING = Math.max( + 0, + availableTerminalHeight - CODE_BLOCK_PADDING * 2 - RESERVED_LINES, + ); + + if (isPending) { + if (content.length > MAX_CODE_LINES_WHEN_PENDING) { + if (MAX_CODE_LINES_WHEN_PENDING < MIN_LINES_FOR_MESSAGE) { + // Not enough space to even show the message meaningfully + return ( + + + ... code is being written ... + + + ); + } + const truncatedContent = content.slice(0, MAX_CODE_LINES_WHEN_PENDING); + const colorizedTruncatedCode = colorizeCode( + truncatedContent.join('\n'), + lang, + ); + return ( + + {colorizedTruncatedCode} + ... generating more ... + + ); + } + } + const fullContent = content.join('\n'); const colorizedCode = colorizeCode(fullContent, lang);