diff options
| author | Jacob Richman <[email protected]> | 2025-05-24 00:44:17 -0700 |
|---|---|---|
| committer | GitHub <[email protected]> | 2025-05-24 00:44:17 -0700 |
| commit | b4c16d1f56f4e19fffd3d7608b410570f35045f9 (patch) | |
| tree | dfda3634f4d266f5472bd37b78362abebe6a9c9e | |
| parent | 1c3d9d7623ecff0437db0627cace0cbb421b458a (diff) | |
Code review comment fixes and some refactors. (#525)
No intentional different behavior aside for tweaks suggested from the code review of #506 Refactor: Extract console message logic to custom hook
This commit refactors the console message handling from App.tsx into a new custom hook useConsoleMessages.
This change improves the testability of the console message logic and declutters the main App component.
Created useConsoleMessages.ts to encapsulate console message state and update logic.
Updated App.tsx to utilize the new useConsoleMessages hook.
Added unit tests for useConsoleMessages.ts to ensure its functionality.
I deleted and started over on LoadingIndicator.test.tsx as I spent way too much time trying to fix it before just regenerating the tests as the code was easier to write tests for from scratch and the existing tests were not that good (I added them in the previous pull request).
17 files changed, 1190 insertions, 739 deletions
diff --git a/.vscode/launch.json b/.vscode/launch.json index 5a7c6522..b7976fd6 100644 --- a/.vscode/launch.json +++ b/.vscode/launch.json @@ -82,6 +82,46 @@ "console": "integratedTerminal", "internalConsoleOptions": "neverOpen", "skipFiles": ["<node_internals>/**"] + }, + { + "type": "node", + "request": "launch", + "name": "Debug useLoadingIndicator Test (CLI)", + "runtimeExecutable": "npm", + "runtimeArgs": [ + "run", + "test", + "-w", + "packages/cli", + "--", + "--inspect-brk=9229", + "--no-file-parallelism", + "${workspaceFolder}/packages/cli/src/ui/hooks/useLoadingIndicator.test.ts" + ], + "cwd": "${workspaceFolder}", + "console": "integratedTerminal", + "internalConsoleOptions": "neverOpen", + "skipFiles": ["<node_internals>/**"] + }, + { + "type": "node", + "request": "launch", + "name": "Debug LoadingIndicator Test (CLI)", + "runtimeExecutable": "npm", + "runtimeArgs": [ + "run", + "test", + "-w", + "packages/cli", + "--", + "--inspect-brk=9229", + "--no-file-parallelism", + "${workspaceFolder}/packages/cli/src/ui/components/LoadingIndicator.test.tsx" + ], + "cwd": "${workspaceFolder}", + "console": "integratedTerminal", + "internalConsoleOptions": "neverOpen", + "skipFiles": ["<node_internals>/**"] } ] } diff --git a/packages/cli/src/ui/App.tsx b/packages/cli/src/ui/App.tsx index 1453487f..5860e36e 100644 --- a/packages/cli/src/ui/App.tsx +++ b/packages/cli/src/ui/App.tsx @@ -41,6 +41,10 @@ import { useHistory } from './hooks/useHistoryManager.js'; import process from 'node:process'; import { getErrorMessage, type Config } from '@gemini-code/server'; import { useLogger } from './hooks/useLogger.js'; +import { + StreamingContext, + StreamingContextType, +} from './contexts/StreamingContext.js'; interface AppProps { config: Config; @@ -174,19 +178,15 @@ export const App = ({ handleSlashCommand, shellModeActive, ); - const isPausedForConfirmation = useMemo( - () => - pendingHistoryItems.some( - (item) => - item?.type === 'tool_group' && - item.tools.some((tool) => tool.status === 'Confirming'), - ), - [pendingHistoryItems], - ); - const { elapsedTime, currentLoadingPhrase, shouldShowSpinner } = - useLoadingIndicator(streamingState, isPausedForConfirmation); + const { elapsedTime, currentLoadingPhrase } = + useLoadingIndicator(streamingState); const showAutoAcceptIndicator = useAutoAcceptIndicator({ config }); + const streamingContextValue: StreamingContextType = useMemo( + () => ({ streamingState }), + [streamingState], + ); + const handleFinalSubmit = useCallback( (submittedValue: string) => { const trimmedValue = submittedValue.trim(); @@ -278,177 +278,176 @@ export const App = ({ }, [consoleMessages, config]); return ( - <Box flexDirection="column" marginBottom={1} width="90%"> - {/* - * The Static component is an Ink intrinsic in which there can only be 1 per application. - * Because of this restriction we're hacking it slightly by having a 'header' item here to - * ensure that it's statically rendered. - * - * Background on the Static Item: Anything in the Static component is written a single time - * to the console. Think of it like doing a console.log and then never using ANSI codes to - * clear that content ever again. Effectively it has a moving frame that every time new static - * content is set it'll flush content to the terminal and move the area which it's "clearing" - * down a notch. Without Static the area which gets erased and redrawn continuously grows. - */} - <Static - key={staticKey} - items={[ - <Box flexDirection="column" key="header"> - <Header /> - <Tips /> - </Box>, - ...history.map((h) => ( + <StreamingContext.Provider value={streamingContextValue}> + <Box flexDirection="column" marginBottom={1} width="90%"> + {/* + * The Static component is an Ink intrinsic in which there can only be 1 per application. + * Because of this restriction we're hacking it slightly by having a 'header' item here to + * ensure that it's statically rendered. + * + * Background on the Static Item: Anything in the Static component is written a single time + * to the console. Think of it like doing a console.log and then never using ANSI codes to + * clear that content ever again. Effectively it has a moving frame that every time new static + * content is set it'll flush content to the terminal and move the area which it's "clearing" + * down a notch. Without Static the area which gets erased and redrawn continuously grows. + */} + <Static + key={staticKey} + items={[ + <Box flexDirection="column" key="header"> + <Header /> + <Tips /> + </Box>, + ...history.map((h) => ( + <HistoryItemDisplay + availableTerminalHeight={availableTerminalHeight} + key={h.id} + item={h} + isPending={false} + /> + )), + ]} + > + {(item) => item} + </Static> + <Box ref={pendingHistoryItemRef}> + {pendingHistoryItems.map((item, i) => ( <HistoryItemDisplay + key={i} availableTerminalHeight={availableTerminalHeight} - key={h.id} - item={h} - isPending={false} - streamingState={streamingState} + // TODO(taehykim): It seems like references to ids aren't necessary in + // HistoryItemDisplay. Refactor later. Use a fake id for now. + item={{ ...item, id: 0 }} + isPending={true} /> - )), - ]} - > - {(item) => item} - </Static> - <Box ref={pendingHistoryItemRef}> - {pendingHistoryItems.map((item, i) => ( - <HistoryItemDisplay - key={i} - availableTerminalHeight={availableTerminalHeight} - // TODO(taehykim): It seems like references to ids aren't necessary in - // HistoryItemDisplay. Refactor later. Use a fake id for now. - item={{ ...item, id: 0 }} - isPending={true} - streamingState={streamingState} - /> - ))} - </Box> - {showHelp && <Help commands={slashCommands} />} - - <Box flexDirection="column" ref={mainControlsRef}> - {startupWarnings.length > 0 && ( - <Box - borderStyle="round" - borderColor={Colors.AccentYellow} - paddingX={1} - marginY={1} - flexDirection="column" - > - {startupWarnings.map((warning, index) => ( - <Text key={index} color={Colors.AccentYellow}> - {warning} - </Text> - ))} - </Box> - )} + ))} + </Box> + {showHelp && <Help commands={slashCommands} />} - {isThemeDialogOpen ? ( - <Box flexDirection="column"> - {themeError && ( - <Box marginBottom={1}> - <Text color={Colors.AccentRed}>{themeError}</Text> - </Box> - )} - <ThemeDialog - onSelect={handleThemeSelect} - onHighlight={handleThemeHighlight} - settings={settings} - /> - </Box> - ) : ( - <> - <LoadingIndicator - isLoading={streamingState === StreamingState.Responding} - showSpinner={shouldShowSpinner} - currentLoadingPhrase={currentLoadingPhrase} - elapsedTime={elapsedTime} - /> + <Box flexDirection="column" ref={mainControlsRef}> + {startupWarnings.length > 0 && ( <Box - marginTop={1} - display="flex" - justifyContent="space-between" - width="100%" + borderStyle="round" + borderColor={Colors.AccentYellow} + paddingX={1} + marginY={1} + flexDirection="column" > - <Box> - {process.env.GEMINI_SYSTEM_MD && ( - <Text color={Colors.AccentRed}>|⌐■_■| </Text> - )} - {geminiMdFileCount > 0 && ( - <Text color={Colors.SubtleComment}> - Using {geminiMdFileCount} GEMINI.md file - {geminiMdFileCount > 1 ? 's' : ''} - </Text> - )} - </Box> - <Box> - {showAutoAcceptIndicator && !shellModeActive && ( - <AutoAcceptIndicator /> - )} - {shellModeActive && <ShellModeIndicator />} - </Box> + {startupWarnings.map((warning, index) => ( + <Text key={index} color={Colors.AccentYellow}> + {warning} + </Text> + ))} </Box> + )} - {showErrorDetails && ( - <DetailedMessagesDisplay messages={filteredConsoleMessages} /> - )} - - {isInputActive && ( - <InputPrompt - widthFraction={0.9} - onSubmit={handleFinalSubmit} - userMessages={userMessages} - onClearScreen={handleClearScreen} - config={config} - slashCommands={slashCommands} - shellModeActive={shellModeActive} - setShellModeActive={setShellModeActive} + {isThemeDialogOpen ? ( + <Box flexDirection="column"> + {themeError && ( + <Box marginBottom={1}> + <Text color={Colors.AccentRed}>{themeError}</Text> + </Box> + )} + <ThemeDialog + onSelect={handleThemeSelect} + onHighlight={handleThemeHighlight} + settings={settings} + /> + </Box> + ) : ( + <> + <LoadingIndicator + currentLoadingPhrase={currentLoadingPhrase} + elapsedTime={elapsedTime} /> - )} - </> - )} + <Box + marginTop={1} + display="flex" + justifyContent="space-between" + width="100%" + > + <Box> + {process.env.GEMINI_SYSTEM_MD && ( + <Text color={Colors.AccentRed}>|⌐■_■| </Text> + )} + {geminiMdFileCount > 0 && ( + <Text color={Colors.SubtleComment}> + Using {geminiMdFileCount} GEMINI.md file + {geminiMdFileCount > 1 ? 's' : ''} + </Text> + )} + </Box> + <Box> + {showAutoAcceptIndicator && !shellModeActive && ( + <AutoAcceptIndicator /> + )} + {shellModeActive && <ShellModeIndicator />} + </Box> + </Box> - {initError && streamingState !== StreamingState.Responding && ( - <Box - borderStyle="round" - borderColor={Colors.AccentRed} - paddingX={1} - marginBottom={1} - > - {history.find( - (item) => item.type === 'error' && item.text?.includes(initError), - )?.text ? ( - <Text color={Colors.AccentRed}> - { - history.find( - (item) => - item.type === 'error' && item.text?.includes(initError), - )?.text - } - </Text> - ) : ( - <> - <Text color={Colors.AccentRed}> - Initialization Error: {initError} - </Text> + {showErrorDetails && ( + <DetailedMessagesDisplay messages={filteredConsoleMessages} /> + )} + + {isInputActive && ( + <InputPrompt + widthFraction={0.9} + onSubmit={handleFinalSubmit} + userMessages={userMessages} + onClearScreen={handleClearScreen} + config={config} + slashCommands={slashCommands} + shellModeActive={shellModeActive} + setShellModeActive={setShellModeActive} + /> + )} + </> + )} + + {initError && streamingState !== StreamingState.Responding && ( + <Box + borderStyle="round" + borderColor={Colors.AccentRed} + paddingX={1} + marginBottom={1} + > + {history.find( + (item) => + item.type === 'error' && item.text?.includes(initError), + )?.text ? ( <Text color={Colors.AccentRed}> - {' '} - Please check API key and configuration. + { + history.find( + (item) => + item.type === 'error' && item.text?.includes(initError), + )?.text + } </Text> - </> - )} - </Box> - )} + ) : ( + <> + <Text color={Colors.AccentRed}> + Initialization Error: {initError} + </Text> + <Text color={Colors.AccentRed}> + {' '} + Please check API key and configuration. + </Text> + </> + )} + </Box> + )} - <Footer - config={config} - debugMode={config.getDebugMode()} - debugMessage={debugMessage} - cliVersion={cliVersion} - corgiMode={corgiMode} - errorCount={errorCount} - showErrorDetails={showErrorDetails} - /> + <Footer + config={config} + debugMode={config.getDebugMode()} + debugMessage={debugMessage} + cliVersion={cliVersion} + corgiMode={corgiMode} + errorCount={errorCount} + showErrorDetails={showErrorDetails} + /> + </Box> </Box> - </Box> + </StreamingContext.Provider> ); }; diff --git a/packages/cli/src/ui/components/HistoryItemDisplay.tsx b/packages/cli/src/ui/components/HistoryItemDisplay.tsx index 21ffb5a1..39cc5308 100644 --- a/packages/cli/src/ui/components/HistoryItemDisplay.tsx +++ b/packages/cli/src/ui/components/HistoryItemDisplay.tsx @@ -5,7 +5,7 @@ */ import React from 'react'; -import type { HistoryItem, StreamingState } from '../types.js'; +import type { HistoryItem } from '../types.js'; import { UserMessage } from './messages/UserMessage.js'; import { UserShellMessage } from './messages/UserShellMessage.js'; import { GeminiMessage } from './messages/GeminiMessage.js'; @@ -20,14 +20,12 @@ interface HistoryItemDisplayProps { item: HistoryItem; availableTerminalHeight: number; isPending: boolean; - streamingState?: StreamingState; } export const HistoryItemDisplay: React.FC<HistoryItemDisplayProps> = ({ item, availableTerminalHeight, isPending, - streamingState, }) => ( <Box flexDirection="column" key={item.id}> {/* Render standard message types */} @@ -62,7 +60,6 @@ export const HistoryItemDisplay: React.FC<HistoryItemDisplayProps> = ({ toolCalls={item.tools} groupId={item.id} availableTerminalHeight={availableTerminalHeight} - streamingState={streamingState} /> )} </Box> diff --git a/packages/cli/src/ui/components/LoadingIndicator.test.tsx b/packages/cli/src/ui/components/LoadingIndicator.test.tsx index 6a8880d4..a03fb230 100644 --- a/packages/cli/src/ui/components/LoadingIndicator.test.tsx +++ b/packages/cli/src/ui/components/LoadingIndicator.test.tsx @@ -4,91 +4,158 @@ * SPDX-License-Identifier: Apache-2.0 */ +import React from 'react'; import { render } from 'ink-testing-library'; -import { Text } from 'ink'; // Import Text directly from ink +import { Text } from 'ink'; import { LoadingIndicator } from './LoadingIndicator.js'; +import { + StreamingContext, + StreamingContextType, +} from '../contexts/StreamingContext.js'; +import { StreamingState } from '../types.js'; +import { vi } from 'vitest'; +// Mock ink-spinner vi.mock('ink-spinner', () => ({ - default: function MockSpinner() { - return <Text>MockSpinner</Text>; - }, + default: () => <Text>MockSpinner</Text>, })); +const renderWithContext = ( + ui: React.ReactElement, + streamingStateValue: StreamingState, +) => { + const contextValue: StreamingContextType = { + streamingState: streamingStateValue, + }; + return render( + <StreamingContext.Provider value={contextValue}> + {ui} + </StreamingContext.Provider>, + ); +}; + describe('<LoadingIndicator />', () => { - it('should not render when isLoading is false', () => { - const { lastFrame } = render( - <LoadingIndicator - isLoading={false} - showSpinner={true} - currentLoadingPhrase="Loading..." - elapsedTime={0} - />, + const defaultProps = { + currentLoadingPhrase: 'Loading...', + elapsedTime: 5, + }; + + it('should not render when streamingState is Idle', () => { + const { lastFrame } = renderWithContext( + <LoadingIndicator {...defaultProps} />, + StreamingState.Idle, ); expect(lastFrame()).toBe(''); }); - it('should render spinner, phrase, and time when isLoading is true and showSpinner is true', () => { - const phrase = 'Processing data...'; - const time = 5; - const { lastFrame } = render( - <LoadingIndicator - isLoading={true} - showSpinner={true} - currentLoadingPhrase={phrase} - elapsedTime={time} - />, + it('should render spinner, phrase, and time when streamingState is Responding', () => { + const { lastFrame } = renderWithContext( + <LoadingIndicator {...defaultProps} />, + StreamingState.Responding, ); - const output = lastFrame(); - expect(output).toContain(phrase); - expect(output).toContain(`(esc to cancel, ${time}s)`); - // Check for spinner presence by looking for its characteristic characters or structure - // This is a bit fragile as it depends on Spinner's output. - // A more robust way would be to mock Spinner and check if it was rendered. - expect(output).toContain('MockSpinner'); // Check for the mocked spinner text + expect(output).toContain('MockSpinner'); + expect(output).toContain('Loading...'); + expect(output).toContain('(esc to cancel, 5s)'); }); - it('should render phrase and time but no spinner when isLoading is true and showSpinner is false', () => { - const phrase = 'Waiting for input...'; - const time = 10; - const { lastFrame } = render( - <LoadingIndicator - isLoading={true} - showSpinner={false} - currentLoadingPhrase={phrase} - elapsedTime={time} - />, + it('should render phrase and time but no spinner when streamingState is WaitingForConfirmation', () => { + const props = { + currentLoadingPhrase: 'Confirm action', + elapsedTime: 10, + }; + const { lastFrame } = renderWithContext( + <LoadingIndicator {...props} />, + StreamingState.WaitingForConfirmation, ); const output = lastFrame(); - expect(output).toContain(phrase); - expect(output).toContain(`(esc to cancel, ${time}s)`); - // Ensure spinner characters are NOT present expect(output).not.toContain('MockSpinner'); + expect(output).toContain('Confirm action'); + expect(output).not.toContain('(esc to cancel)'); + expect(output).not.toContain(', 10s'); }); it('should display the currentLoadingPhrase correctly', () => { - const specificPhrase = 'Almost there!'; - const { lastFrame } = render( - <LoadingIndicator - isLoading={true} - showSpinner={true} - currentLoadingPhrase={specificPhrase} - elapsedTime={3} - />, + const props = { + currentLoadingPhrase: 'Processing data...', + elapsedTime: 3, + }; + const { lastFrame } = renderWithContext( + <LoadingIndicator {...props} />, + StreamingState.Responding, + ); + expect(lastFrame()).toContain('Processing data...'); + }); + + it('should display the elapsedTime correctly when Responding', () => { + const props = { + currentLoadingPhrase: 'Working...', + elapsedTime: 8, + }; + const { lastFrame } = renderWithContext( + <LoadingIndicator {...props} />, + StreamingState.Responding, + ); + expect(lastFrame()).toContain('(esc to cancel, 8s)'); + }); + + it('should render rightContent when provided', () => { + const rightContent = <Text>Extra Info</Text>; + const { lastFrame } = renderWithContext( + <LoadingIndicator {...defaultProps} rightContent={rightContent} />, + StreamingState.Responding, ); - expect(lastFrame()).toContain(specificPhrase); + expect(lastFrame()).toContain('Extra Info'); }); - it('should display the elapsedTime correctly', () => { - const specificTime = 7; - const { lastFrame } = render( - <LoadingIndicator - isLoading={true} - showSpinner={true} - currentLoadingPhrase="Working..." - elapsedTime={specificTime} - />, + it('should transition correctly between states using rerender', () => { + const { lastFrame, rerender } = renderWithContext( + <LoadingIndicator {...defaultProps} />, + StreamingState.Idle, ); - expect(lastFrame()).toContain(`(esc to cancel, ${specificTime}s)`); + expect(lastFrame()).toBe(''); // Initial: Idle + + // Transition to Responding + rerender( + <StreamingContext.Provider + value={{ streamingState: StreamingState.Responding }} + > + <LoadingIndicator + currentLoadingPhrase="Now Responding" + elapsedTime={2} + /> + </StreamingContext.Provider>, + ); + let output = lastFrame(); + expect(output).toContain('MockSpinner'); + expect(output).toContain('Now Responding'); + expect(output).toContain('(esc to cancel, 2s)'); + + // Transition to WaitingForConfirmation + rerender( + <StreamingContext.Provider + value={{ streamingState: StreamingState.WaitingForConfirmation }} + > + <LoadingIndicator + currentLoadingPhrase="Please Confirm" + elapsedTime={15} + /> + </StreamingContext.Provider>, + ); + output = lastFrame(); + expect(output).not.toContain('MockSpinner'); + expect(output).toContain('Please Confirm'); + expect(output).not.toContain('(esc to cancel)'); + expect(output).not.toContain(', 15s'); + + // Transition back to Idle + rerender( + <StreamingContext.Provider + value={{ streamingState: StreamingState.Idle }} + > + <LoadingIndicator {...defaultProps} /> + </StreamingContext.Provider>, + ); + expect(lastFrame()).toBe(''); }); }); diff --git a/packages/cli/src/ui/components/LoadingIndicator.tsx b/packages/cli/src/ui/components/LoadingIndicator.tsx index b0c24f80..d24b6a56 100644 --- a/packages/cli/src/ui/components/LoadingIndicator.tsx +++ b/packages/cli/src/ui/components/LoadingIndicator.tsx @@ -8,36 +8,38 @@ import React from 'react'; import { Box, Text } from 'ink'; import Spinner from 'ink-spinner'; import { Colors } from '../colors.js'; +import { useStreamingContext } from '../contexts/StreamingContext.js'; +import { StreamingState } from '../types.js'; interface LoadingIndicatorProps { - isLoading: boolean; - showSpinner: boolean; currentLoadingPhrase: string; elapsedTime: number; rightContent?: React.ReactNode; } export const LoadingIndicator: React.FC<LoadingIndicatorProps> = ({ - isLoading, - showSpinner, currentLoadingPhrase, elapsedTime, rightContent, }) => { - if (!isLoading) { + const { streamingState } = useStreamingContext(); + + if (streamingState === StreamingState.Idle) { return null; } return ( <Box marginTop={1} paddingLeft={0}> - {showSpinner && ( + {streamingState === StreamingState.Responding && ( <Box marginRight={1}> <Spinner type="dots" /> </Box> )} <Text color={Colors.AccentPurple}> {currentLoadingPhrase} - {isLoading && ` (esc to cancel, ${elapsedTime}s)`} + {streamingState === StreamingState.WaitingForConfirmation + ? '' + : ` (esc to cancel, ${elapsedTime}s)`} </Text> <Box flexGrow={1}>{/* Spacer */}</Box> {rightContent && <Box>{rightContent}</Box>} diff --git a/packages/cli/src/ui/components/messages/ToolGroupMessage.tsx b/packages/cli/src/ui/components/messages/ToolGroupMessage.tsx index c6c8b874..8bcde3bb 100644 --- a/packages/cli/src/ui/components/messages/ToolGroupMessage.tsx +++ b/packages/cli/src/ui/components/messages/ToolGroupMessage.tsx @@ -6,11 +6,7 @@ import React, { useMemo } from 'react'; import { Box } from 'ink'; -import { - IndividualToolCallDisplay, - StreamingState, - ToolCallStatus, -} from '../../types.js'; +import { IndividualToolCallDisplay, ToolCallStatus } from '../../types.js'; import { ToolMessage } from './ToolMessage.js'; import { ToolConfirmationMessage } from './ToolConfirmationMessage.js'; import { Colors } from '../../colors.js'; @@ -19,14 +15,12 @@ interface ToolGroupMessageProps { groupId: number; toolCalls: IndividualToolCallDisplay[]; availableTerminalHeight: number; - streamingState?: StreamingState; } // Main component renders the border and maps the tools using ToolMessage export const ToolGroupMessage: React.FC<ToolGroupMessageProps> = ({ toolCalls, availableTerminalHeight, - streamingState, }) => { const hasPending = !toolCalls.every( (t) => t.status === ToolCallStatus.Success, @@ -78,7 +72,6 @@ export const ToolGroupMessage: React.FC<ToolGroupMessageProps> = ({ ? 'low' : 'medium' } - streamingState={streamingState} /> </Box> {tool.status === ToolCallStatus.Confirming && diff --git a/packages/cli/src/ui/components/messages/ToolMessage.test.tsx b/packages/cli/src/ui/components/messages/ToolMessage.test.tsx index 1c81cc36..10380ad4 100644 --- a/packages/cli/src/ui/components/messages/ToolMessage.test.tsx +++ b/packages/cli/src/ui/components/messages/ToolMessage.test.tsx @@ -8,6 +8,7 @@ import { render } from 'ink-testing-library'; import { ToolMessage, ToolMessageProps } from './ToolMessage.js'; import { StreamingState, ToolCallStatus } from '../../types.js'; import { Text } from 'ink'; +import { StreamingContext } from '../../contexts/StreamingContext.js'; // Mock child components or utilities if they are complex or have side effects vi.mock('ink-spinner', () => ({ @@ -28,6 +29,17 @@ vi.mock('../../utils/MarkdownDisplay.js', () => ({ }, })); +// Helper to render with context +const renderWithContext = ( + ui: React.ReactElement, + streamingState: StreamingState, +) => + render( + <StreamingContext.Provider value={{ streamingState }}> + {ui} + </StreamingContext.Provider>, + ); + describe('<ToolMessage />', () => { const baseProps: ToolMessageProps = { callId: 'tool-123', @@ -38,11 +50,13 @@ describe('<ToolMessage />', () => { availableTerminalHeight: 20, confirmationDetails: undefined, emphasis: 'medium', - streamingState: StreamingState.Idle, }; it('renders basic tool information', () => { - const { lastFrame } = render(<ToolMessage {...baseProps} />); + const { lastFrame } = renderWithContext( + <ToolMessage {...baseProps} />, + StreamingState.Idle, + ); const output = lastFrame(); expect(output).toContain('✔'); // Success indicator expect(output).toContain('test-tool'); @@ -52,73 +66,72 @@ describe('<ToolMessage />', () => { describe('ToolStatusIndicator rendering', () => { it('shows ✔ for Success status', () => { - const { lastFrame } = render( + const { lastFrame } = renderWithContext( <ToolMessage {...baseProps} status={ToolCallStatus.Success} />, + StreamingState.Idle, ); expect(lastFrame()).toContain('✔'); }); it('shows o for Pending status', () => { - const { lastFrame } = render( + const { lastFrame } = renderWithContext( <ToolMessage {...baseProps} status={ToolCallStatus.Pending} />, + StreamingState.Idle, ); expect(lastFrame()).toContain('o'); }); it('shows ? for Confirming status', () => { - const { lastFrame } = render( + const { lastFrame } = renderWithContext( <ToolMessage {...baseProps} status={ToolCallStatus.Confirming} />, + StreamingState.Idle, ); expect(lastFrame()).toContain('?'); }); it('shows - for Canceled status', () => { - const { lastFrame } = render( + const { lastFrame } = renderWithContext( <ToolMessage {...baseProps} status={ToolCallStatus.Canceled} />, + StreamingState.Idle, ); expect(lastFrame()).toContain('-'); }); it('shows x for Error status', () => { - const { lastFrame } = render( + const { lastFrame } = renderWithContext( <ToolMessage {...baseProps} status={ToolCallStatus.Error} />, + StreamingState.Idle, ); expect(lastFrame()).toContain('x'); }); - it('shows MockSpinner for Executing status when streamingState is Idle', () => { - const { lastFrame } = render( - <ToolMessage - {...baseProps} - status={ToolCallStatus.Executing} - streamingState={StreamingState.Idle} - />, + it('shows paused spiner for Executing status when streamingState is Idle', () => { + const { lastFrame } = renderWithContext( + <ToolMessage {...baseProps} status={ToolCallStatus.Executing} />, + StreamingState.Idle, ); - expect(lastFrame()).toContain('MockSpinner'); + expect(lastFrame()).toContain('⠇'); + expect(lastFrame()).not.toContain('MockSpinner'); expect(lastFrame()).not.toContain('✔'); }); - it('shows MockSpinner for Executing status when streamingState is undefined (default behavior)', () => { - const { lastFrame } = render( + it('shows paused spiner for Executing status when streamingState is WaitingForConfirmation', () => { + const { lastFrame } = renderWithContext( <ToolMessage {...baseProps} status={ToolCallStatus.Executing} />, + StreamingState.WaitingForConfirmation, ); - expect(lastFrame()).toContain('MockSpinner'); + expect(lastFrame()).toContain('⠇'); + expect(lastFrame()).not.toContain('MockSpinner'); expect(lastFrame()).not.toContain('✔'); }); - it('shows ✔ (paused/confirmed look) for Executing status when streamingState is Responding', () => { - // This is the key change from the commit: if the overall app is still responding - // (e.g., waiting for other tool confirmations), an already confirmed and executing tool - // should show a static checkmark to avoid spinner flicker. - const { lastFrame } = render( - <ToolMessage - {...baseProps} - status={ToolCallStatus.Executing} - streamingState={StreamingState.Responding} // Simulate app still responding - />, + it('shows MockSpinner for Executing status when streamingState is Responding', () => { + const { lastFrame } = renderWithContext( + <ToolMessage {...baseProps} status={ToolCallStatus.Executing} />, + StreamingState.Responding, // Simulate app still responding ); - expect(lastFrame()).toContain('✔'); // Should be a checkmark, not spinner - expect(lastFrame()).not.toContain('MockSpinner'); + expect(lastFrame()).toContain('MockSpinner'); + expect(lastFrame()).not.toContain('✔'); }); }); @@ -127,22 +140,25 @@ describe('<ToolMessage />', () => { fileDiff: '--- a/file.txt\n+++ b/file.txt\n@@ -1 +1 @@\n-old\n+new', fileName: 'file.txt', }; - const { lastFrame } = render( + const { lastFrame } = renderWithContext( <ToolMessage {...baseProps} resultDisplay={diffResult} />, + StreamingState.Idle, ); // Check that the output contains the MockDiff content as part of the whole message expect(lastFrame()).toMatch(/MockDiff:--- a\/file\.txt/); }); it('renders emphasis correctly', () => { - const { lastFrame: highEmphasisFrame } = render( + const { lastFrame: highEmphasisFrame } = renderWithContext( <ToolMessage {...baseProps} emphasis="high" />, + StreamingState.Idle, ); // Check for trailing indicator or specific color if applicable (Colors are not easily testable here) expect(highEmphasisFrame()).toContain('←'); // Trailing indicator for high emphasis - const { lastFrame: lowEmphasisFrame } = render( + const { lastFrame: lowEmphasisFrame } = renderWithContext( <ToolMessage {...baseProps} emphasis="low" />, + StreamingState.Idle, ); // For low emphasis, the name and description might be dimmed (check for dimColor if possible) // This is harder to assert directly in text output without color checks. diff --git a/packages/cli/src/ui/components/messages/ToolMessage.tsx b/packages/cli/src/ui/components/messages/ToolMessage.tsx index 49743190..775b4177 100644 --- a/packages/cli/src/ui/components/messages/ToolMessage.tsx +++ b/packages/cli/src/ui/components/messages/ToolMessage.tsx @@ -15,6 +15,7 @@ import { import { DiffRenderer } from './DiffRenderer.js'; import { Colors } from '../../colors.js'; import { MarkdownDisplay } from '../../utils/MarkdownDisplay.js'; +import { useStreamingContext } from '../../contexts/StreamingContext.js'; const STATIC_HEIGHT = 1; const RESERVED_LINE_COUNT = 5; // for tool name, status, padding etc. @@ -25,7 +26,6 @@ export type TextEmphasis = 'high' | 'medium' | 'low'; export interface ToolMessageProps extends IndividualToolCallDisplay { availableTerminalHeight: number; emphasis?: TextEmphasis; - streamingState?: StreamingState; } export const ToolMessage: React.FC<ToolMessageProps> = ({ @@ -35,7 +35,6 @@ export const ToolMessage: React.FC<ToolMessageProps> = ({ status, availableTerminalHeight, emphasis = 'medium', - streamingState, }) => { const contentHeightEstimate = availableTerminalHeight - STATIC_HEIGHT - RESERVED_LINE_COUNT; @@ -63,7 +62,7 @@ export const ToolMessage: React.FC<ToolMessageProps> = ({ <Box paddingX={1} paddingY={0} flexDirection="column"> <Box minHeight={1}> {/* Status Indicator */} - <ToolStatusIndicator status={status} streamingState={streamingState} /> + <ToolStatusIndicator status={status} /> <ToolInfo name={name} status={status} @@ -107,48 +106,42 @@ export const ToolMessage: React.FC<ToolMessageProps> = ({ type ToolStatusIndicatorProps = { status: ToolCallStatus; - streamingState?: StreamingState; }; const ToolStatusIndicator: React.FC<ToolStatusIndicatorProps> = ({ status, - streamingState, -}) => ( - <Box minWidth={STATUS_INDICATOR_WIDTH}> - {status === ToolCallStatus.Pending && ( - <Text color={Colors.AccentGreen}>o</Text> - )} - {status === ToolCallStatus.Executing && - (streamingState === StreamingState.Responding ? ( - // If the tool is responding that means the user has already confirmed - // this tool call, so we can show a checkmark. The call won't complete - // executing until all confirmations are done. Showing a spinner would - // be misleading as the task is not actually executing at the moment - // and also has flickering issues due to Ink rendering limitations. - // If this hack becomes a problem, we can always add an additional prop - // indicating that the tool was indeed confirmed. If the tool was not - // confirmed we could show a paused version of the spinner. - <Text color={Colors.Gray}>✔</Text> - ) : ( - <Spinner type="dots" /> - ))} - {status === ToolCallStatus.Success && ( - <Text color={Colors.AccentGreen}>✔</Text> - )} - {status === ToolCallStatus.Confirming && ( - <Text color={Colors.AccentYellow}>?</Text> - )} - {status === ToolCallStatus.Canceled && ( - <Text color={Colors.AccentYellow} bold> - - - </Text> - )} - {status === ToolCallStatus.Error && ( - <Text color={Colors.AccentRed} bold> - x - </Text> - )} - </Box> -); +}) => { + const { streamingState } = useStreamingContext(); + return ( + <Box minWidth={STATUS_INDICATOR_WIDTH}> + {status === ToolCallStatus.Pending && ( + <Text color={Colors.AccentGreen}>o</Text> + )} + {status === ToolCallStatus.Executing && + (streamingState === StreamingState.Responding ? ( + <Spinner type="dots" /> + ) : ( + // Paused spinner to avoid flicker. + <Text>⠇</Text> + ))} + {status === ToolCallStatus.Success && ( + <Text color={Colors.AccentGreen}>✔</Text> + )} + {status === ToolCallStatus.Confirming && ( + <Text color={Colors.AccentYellow}>?</Text> + )} + {status === ToolCallStatus.Canceled && ( + <Text color={Colors.AccentYellow} bold> + - + </Text> + )} + {status === ToolCallStatus.Error && ( + <Text color={Colors.AccentRed} bold> + x + </Text> + )} + </Box> + ); +}; type ToolInfo = { name: string; diff --git a/packages/cli/src/ui/constants.ts b/packages/cli/src/ui/constants.ts index 5924c7a5..6a0a9375 100644 --- a/packages/cli/src/ui/constants.ts +++ b/packages/cli/src/ui/constants.ts @@ -12,22 +12,4 @@ export const BOX_PADDING_X = 1; export const UI_WIDTH = EstimatedArtWidth + BOX_PADDING_X * 2 + BoxBorderWidth * 2; // ~63 -export const WITTY_LOADING_PHRASES = [ - 'Consulting the digital spirits...', - 'Reticulating splines...', - 'Warming up the AI hamsters...', - 'Asking the magic conch shell...', - 'Generating witty retort...', - 'Polishing the algorithms...', - "Don't rush perfection (or my code)...", - 'Brewing fresh bytes...', - 'Counting electrons...', - 'Engaging cognitive processors...', - 'Checking for syntax errors in the universe...', - 'One moment, optimizing humor...', - 'Shuffling punchlines...', - 'Untangling neural nets...', - 'Compiling brilliance...', -]; -export const PHRASE_CHANGE_INTERVAL_MS = 15000; export const STREAM_DEBOUNCE_MS = 100; diff --git a/packages/cli/src/ui/contexts/StreamingContext.tsx b/packages/cli/src/ui/contexts/StreamingContext.tsx new file mode 100644 index 00000000..6144ed70 --- /dev/null +++ b/packages/cli/src/ui/contexts/StreamingContext.tsx @@ -0,0 +1,26 @@ +/** + * @license + * Copyright 2025 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import React, { createContext } from 'react'; +import { StreamingState } from '../types.js'; + +export interface StreamingContextType { + streamingState: StreamingState; +} + +export const StreamingContext = createContext<StreamingContextType | undefined>( + undefined, +); + +export const useStreamingContext = (): StreamingContextType => { + const context = React.useContext(StreamingContext); + if (context === undefined) { + throw new Error( + 'useStreamingContext must be used within a StreamingContextProvider', + ); + } + return context; +}; diff --git a/packages/cli/src/ui/hooks/useGeminiStream.ts b/packages/cli/src/ui/hooks/useGeminiStream.ts index 45415f39..9dcb005b 100644 --- a/packages/cli/src/ui/hooks/useGeminiStream.ts +++ b/packages/cli/src/ui/hooks/useGeminiStream.ts @@ -42,7 +42,6 @@ import { useToolScheduler, mapToDisplay } from './useToolScheduler.js'; enum StreamProcessingStatus { Completed, - PausedForConfirmation, UserCancelled, Error, } @@ -103,6 +102,21 @@ export const useGeminiStream = ( config, ); + const streamingState = useMemo(() => { + if (toolCalls.some((t) => t.status === 'awaiting_approval')) { + return StreamingState.WaitingForConfirmation; + } + if ( + isResponding || + toolCalls.some( + (t) => t.status === 'executing' || t.status === 'scheduled', + ) + ) { + return StreamingState.Responding; + } + return StreamingState.Idle; + }, [isResponding, toolCalls]); + useEffect(() => { setInitError(null); if (!geminiClientRef.current) { @@ -123,85 +137,100 @@ export const useGeminiStream = ( } }); - const prepareQueryForGemini = async ( - query: PartListUnion, - userMessageTimestamp: number, - signal: AbortSignal, - ): Promise<{ queryToSend: PartListUnion | null; shouldProceed: boolean }> => { - if (typeof query === 'string' && query.trim().length === 0) { - return { queryToSend: null, shouldProceed: false }; - } + const prepareQueryForGemini = useCallback( + async ( + query: PartListUnion, + userMessageTimestamp: number, + signal: AbortSignal, + ): Promise<{ + queryToSend: PartListUnion | null; + shouldProceed: boolean; + }> => { + if (typeof query === 'string' && query.trim().length === 0) { + return { queryToSend: null, shouldProceed: false }; + } - let localQueryToSendToGemini: PartListUnion | null = null; + let localQueryToSendToGemini: PartListUnion | null = null; - if (typeof query === 'string') { - const trimmedQuery = query.trim(); - onDebugMessage(`User query: '${trimmedQuery}'`); - await logger?.logMessage(MessageSenderType.USER, trimmedQuery); + if (typeof query === 'string') { + const trimmedQuery = query.trim(); + onDebugMessage(`User query: '${trimmedQuery}'`); + await logger?.logMessage(MessageSenderType.USER, trimmedQuery); - // Handle UI-only commands first - const slashCommandResult = handleSlashCommand(trimmedQuery); - if (typeof slashCommandResult === 'boolean' && slashCommandResult) { - // Command was handled, and it doesn't require a tool call from here - return { queryToSend: null, shouldProceed: false }; - } else if ( - typeof slashCommandResult === 'object' && - slashCommandResult.shouldScheduleTool - ) { - // Slash command wants to schedule a tool call (e.g., /memory add) - const { toolName, toolArgs } = slashCommandResult; - if (toolName && toolArgs) { - const toolCallRequest: ToolCallRequestInfo = { - callId: `${toolName}-${Date.now()}-${Math.random().toString(16).slice(2)}`, - name: toolName, - args: toolArgs, - }; - schedule([toolCallRequest]); // schedule expects an array or single object + // Handle UI-only commands first + const slashCommandResult = handleSlashCommand(trimmedQuery); + if (typeof slashCommandResult === 'boolean' && slashCommandResult) { + // Command was handled, and it doesn't require a tool call from here + return { queryToSend: null, shouldProceed: false }; + } else if ( + typeof slashCommandResult === 'object' && + slashCommandResult.shouldScheduleTool + ) { + // Slash command wants to schedule a tool call (e.g., /memory add) + const { toolName, toolArgs } = slashCommandResult; + if (toolName && toolArgs) { + const toolCallRequest: ToolCallRequestInfo = { + callId: `${toolName}-${Date.now()}-${Math.random().toString(16).slice(2)}`, + name: toolName, + args: toolArgs, + }; + schedule([toolCallRequest]); // schedule expects an array or single object + } + return { queryToSend: null, shouldProceed: false }; // Handled by scheduling the tool } - return { queryToSend: null, shouldProceed: false }; // Handled by scheduling the tool - } - if (shellModeActive && handleShellCommand(trimmedQuery)) { - return { queryToSend: null, shouldProceed: false }; - } - - // Handle @-commands (which might involve tool calls) - if (isAtCommand(trimmedQuery)) { - const atCommandResult = await handleAtCommand({ - query: trimmedQuery, - config, - addItem, - onDebugMessage, - messageId: userMessageTimestamp, - signal, - }); - if (!atCommandResult.shouldProceed) { + if (shellModeActive && handleShellCommand(trimmedQuery)) { return { queryToSend: null, shouldProceed: false }; } - localQueryToSendToGemini = atCommandResult.processedQuery; + + // Handle @-commands (which might involve tool calls) + if (isAtCommand(trimmedQuery)) { + const atCommandResult = await handleAtCommand({ + query: trimmedQuery, + config, + addItem, + onDebugMessage, + messageId: userMessageTimestamp, + signal, + }); + if (!atCommandResult.shouldProceed) { + return { queryToSend: null, shouldProceed: false }; + } + localQueryToSendToGemini = atCommandResult.processedQuery; + } else { + // Normal query for Gemini + addItem( + { type: MessageType.USER, text: trimmedQuery }, + userMessageTimestamp, + ); + localQueryToSendToGemini = trimmedQuery; + } } else { - // Normal query for Gemini - addItem( - { type: MessageType.USER, text: trimmedQuery }, - userMessageTimestamp, - ); - localQueryToSendToGemini = trimmedQuery; + // It's a function response (PartListUnion that isn't a string) + localQueryToSendToGemini = query; } - } else { - // It's a function response (PartListUnion that isn't a string) - localQueryToSendToGemini = query; - } - if (localQueryToSendToGemini === null) { - onDebugMessage( - 'Query processing resulted in null, not sending to Gemini.', - ); - return { queryToSend: null, shouldProceed: false }; - } - return { queryToSend: localQueryToSendToGemini, shouldProceed: true }; - }; + if (localQueryToSendToGemini === null) { + onDebugMessage( + 'Query processing resulted in null, not sending to Gemini.', + ); + return { queryToSend: null, shouldProceed: false }; + } + return { queryToSend: localQueryToSendToGemini, shouldProceed: true }; + }, + [ + config, + addItem, + onDebugMessage, + handleShellCommand, + handleSlashCommand, + logger, + shellModeActive, + schedule, + ], + ); - const ensureChatSession = async (): Promise<{ + const ensureChatSession = useCallback(async (): Promise<{ client: GeminiClient | null; chat: Chat | null; }> => { @@ -224,7 +253,7 @@ export const useGeminiStream = ( } } return { client: currentClient, chat: chatSessionRef.current }; - }; + }, [addItem]); // --- UI Helper Functions (used by event handlers) --- const updateFunctionResponseUI = ( @@ -285,6 +314,7 @@ export const useGeminiStream = ( history.push({ role: 'model', parts: [functionResponse] }); } updateFunctionResponseUI(responseInfo, status); + if (pendingHistoryItemRef.current) { addItem(pendingHistoryItemRef.current, Date.now()); setPendingHistoryItem(null); @@ -293,138 +323,151 @@ export const useGeminiStream = ( } // --- Stream Event Handlers --- - const handleContentEvent = ( - eventValue: ContentEvent['value'], - currentGeminiMessageBuffer: string, - userMessageTimestamp: number, - ): string => { - let newGeminiMessageBuffer = currentGeminiMessageBuffer + eventValue; - if ( - pendingHistoryItemRef.current?.type !== 'gemini' && - pendingHistoryItemRef.current?.type !== 'gemini_content' - ) { + + const handleContentEvent = useCallback( + ( + eventValue: ContentEvent['value'], + currentGeminiMessageBuffer: string, + userMessageTimestamp: number, + ): string => { + let newGeminiMessageBuffer = currentGeminiMessageBuffer + eventValue; + if ( + pendingHistoryItemRef.current?.type !== 'gemini' && + pendingHistoryItemRef.current?.type !== 'gemini_content' + ) { + if (pendingHistoryItemRef.current) { + addItem(pendingHistoryItemRef.current, userMessageTimestamp); + } + setPendingHistoryItem({ type: 'gemini', text: '' }); + newGeminiMessageBuffer = eventValue; + } + // Split large messages for better rendering performance. Ideally, + // we should maximize the amount of output sent to <Static />. + const splitPoint = findLastSafeSplitPoint(newGeminiMessageBuffer); + if (splitPoint === newGeminiMessageBuffer.length) { + // Update the existing message with accumulated content + setPendingHistoryItem((item) => ({ + type: item?.type as 'gemini' | 'gemini_content', + text: newGeminiMessageBuffer, + })); + } else { + // This indicates that we need to split up this Gemini Message. + // Splitting a message is primarily a performance consideration. There is a + // <Static> component at the root of App.tsx which takes care of rendering + // content statically or dynamically. Everything but the last message is + // treated as static in order to prevent re-rendering an entire message history + // multiple times per-second (as streaming occurs). Prior to this change you'd + // see heavy flickering of the terminal. This ensures that larger messages get + // broken up so that there are more "statically" rendered. + const beforeText = newGeminiMessageBuffer.substring(0, splitPoint); + const afterText = newGeminiMessageBuffer.substring(splitPoint); + addItem( + { + type: pendingHistoryItemRef.current?.type as + | 'gemini' + | 'gemini_content', + text: beforeText, + }, + userMessageTimestamp, + ); + setPendingHistoryItem({ type: 'gemini_content', text: afterText }); + newGeminiMessageBuffer = afterText; + } + return newGeminiMessageBuffer; + }, + [addItem, pendingHistoryItemRef, setPendingHistoryItem], + ); + + const handleUserCancelledEvent = useCallback( + (userMessageTimestamp: number) => { if (pendingHistoryItemRef.current) { - addItem(pendingHistoryItemRef.current, userMessageTimestamp); + if (pendingHistoryItemRef.current.type === 'tool_group') { + const updatedTools = pendingHistoryItemRef.current.tools.map( + (tool) => + tool.status === ToolCallStatus.Pending || + tool.status === ToolCallStatus.Confirming || + tool.status === ToolCallStatus.Executing + ? { ...tool, status: ToolCallStatus.Canceled } + : tool, + ); + const pendingItem: HistoryItemToolGroup = { + ...pendingHistoryItemRef.current, + tools: updatedTools, + }; + addItem(pendingItem, userMessageTimestamp); + } else { + addItem(pendingHistoryItemRef.current, userMessageTimestamp); + } + setPendingHistoryItem(null); } - setPendingHistoryItem({ type: 'gemini', text: '' }); - newGeminiMessageBuffer = eventValue; - } - // Split large messages for better rendering performance. Ideally, - // we should maximize the amount of output sent to <Static />. - const splitPoint = findLastSafeSplitPoint(newGeminiMessageBuffer); - if (splitPoint === newGeminiMessageBuffer.length) { - // Update the existing message with accumulated content - setPendingHistoryItem((item) => ({ - type: item?.type as 'gemini' | 'gemini_content', - text: newGeminiMessageBuffer, - })); - } else { - // This indicates that we need to split up this Gemini Message. - // Splitting a message is primarily a performance consideration. There is a - // <Static> component at the root of App.tsx which takes care of rendering - // content statically or dynamically. Everything but the last message is - // treated as static in order to prevent re-rendering an entire message history - // multiple times per-second (as streaming occurs). Prior to this change you'd - // see heavy flickering of the terminal. This ensures that larger messages get - // broken up so that there are more "statically" rendered. - const beforeText = newGeminiMessageBuffer.substring(0, splitPoint); - const afterText = newGeminiMessageBuffer.substring(splitPoint); addItem( - { - type: pendingHistoryItemRef.current?.type as - | 'gemini' - | 'gemini_content', - text: beforeText, - }, + { type: MessageType.INFO, text: 'User cancelled the request.' }, userMessageTimestamp, ); - setPendingHistoryItem({ type: 'gemini_content', text: afterText }); - newGeminiMessageBuffer = afterText; - } - return newGeminiMessageBuffer; - }; + setIsResponding(false); + cancel(); + }, + [addItem, pendingHistoryItemRef, setPendingHistoryItem, cancel], + ); - const handleUserCancelledEvent = (userMessageTimestamp: number) => { - if (pendingHistoryItemRef.current) { - if (pendingHistoryItemRef.current.type === 'tool_group') { - const updatedTools = pendingHistoryItemRef.current.tools.map((tool) => - tool.status === ToolCallStatus.Pending || - tool.status === ToolCallStatus.Confirming || - tool.status === ToolCallStatus.Executing - ? { ...tool, status: ToolCallStatus.Canceled } - : tool, - ); - const pendingItem: HistoryItemToolGroup = { - ...pendingHistoryItemRef.current, - tools: updatedTools, - }; - addItem(pendingItem, userMessageTimestamp); - } else { + const handleErrorEvent = useCallback( + (eventValue: ErrorEvent['value'], userMessageTimestamp: number) => { + if (pendingHistoryItemRef.current) { addItem(pendingHistoryItemRef.current, userMessageTimestamp); + setPendingHistoryItem(null); } - setPendingHistoryItem(null); - } - addItem( - { type: MessageType.INFO, text: 'User cancelled the request.' }, - userMessageTimestamp, - ); - setIsResponding(false); - cancel(); - }; - - const handleErrorEvent = ( - eventValue: ErrorEvent['value'], - userMessageTimestamp: number, - ) => { - if (pendingHistoryItemRef.current) { - addItem(pendingHistoryItemRef.current, userMessageTimestamp); - setPendingHistoryItem(null); - } - addItem( - { type: MessageType.ERROR, text: `[API Error: ${eventValue.message}]` }, - userMessageTimestamp, - ); - }; + addItem( + { type: MessageType.ERROR, text: `[API Error: ${eventValue.message}]` }, + userMessageTimestamp, + ); + }, + [addItem, pendingHistoryItemRef, setPendingHistoryItem], + ); - const processGeminiStreamEvents = async ( - stream: AsyncIterable<GeminiEvent>, - userMessageTimestamp: number, - ): Promise<StreamProcessingStatus> => { - let geminiMessageBuffer = ''; - const toolCallRequests: ToolCallRequestInfo[] = []; - for await (const event of stream) { - if (event.type === ServerGeminiEventType.Content) { - geminiMessageBuffer = handleContentEvent( - event.value, - geminiMessageBuffer, - userMessageTimestamp, - ); - } else if (event.type === ServerGeminiEventType.ToolCallRequest) { - toolCallRequests.push(event.value); - } else if (event.type === ServerGeminiEventType.UserCancelled) { - handleUserCancelledEvent(userMessageTimestamp); - cancel(); - return StreamProcessingStatus.UserCancelled; - } else if (event.type === ServerGeminiEventType.Error) { - handleErrorEvent(event.value, userMessageTimestamp); - return StreamProcessingStatus.Error; + const processGeminiStreamEvents = useCallback( + async ( + stream: AsyncIterable<GeminiEvent>, + userMessageTimestamp: number, + ): Promise<StreamProcessingStatus> => { + let geminiMessageBuffer = ''; + const toolCallRequests: ToolCallRequestInfo[] = []; + for await (const event of stream) { + if (event.type === ServerGeminiEventType.Content) { + geminiMessageBuffer = handleContentEvent( + event.value, + geminiMessageBuffer, + userMessageTimestamp, + ); + } else if (event.type === ServerGeminiEventType.ToolCallRequest) { + toolCallRequests.push(event.value); + } else if (event.type === ServerGeminiEventType.UserCancelled) { + handleUserCancelledEvent(userMessageTimestamp); + cancel(); + return StreamProcessingStatus.UserCancelled; + } else if (event.type === ServerGeminiEventType.Error) { + handleErrorEvent(event.value, userMessageTimestamp); + return StreamProcessingStatus.Error; + } } - } - schedule(toolCallRequests); - return StreamProcessingStatus.Completed; - }; - - const streamingState: StreamingState = - isResponding || - toolCalls.some( - (t) => t.status === 'awaiting_approval' || t.status === 'executing', - ) - ? StreamingState.Responding - : StreamingState.Idle; + schedule(toolCallRequests); + return StreamProcessingStatus.Completed; + }, + [ + handleContentEvent, + handleUserCancelledEvent, + cancel, + handleErrorEvent, + schedule, + ], + ); const submitQuery = useCallback( async (query: PartListUnion) => { - if (isResponding) return; + if ( + streamingState === StreamingState.Responding || + streamingState === StreamingState.WaitingForConfirmation + ) + return; const userMessageTimestamp = Date.now(); setShowHelp(false); @@ -458,10 +501,7 @@ export const useGeminiStream = ( userMessageTimestamp, ); - if ( - processingStatus === StreamProcessingStatus.PausedForConfirmation || - processingStatus === StreamProcessingStatus.UserCancelled - ) { + if (processingStatus === StreamProcessingStatus.UserCancelled) { return; } @@ -484,19 +524,16 @@ export const useGeminiStream = ( setIsResponding(false); } }, - // eslint-disable-next-line react-hooks/exhaustive-deps [ - isResponding, setShowHelp, - handleSlashCommand, - shellModeActive, - handleShellCommand, - config, addItem, - onDebugMessage, - refreshStatic, setInitError, - logger, + ensureChatSession, + prepareQueryForGemini, + processGeminiStreamEvents, + setPendingHistoryItem, + pendingHistoryItemRef, + streamingState, ], ); diff --git a/packages/cli/src/ui/hooks/useLoadingIndicator.test.ts b/packages/cli/src/ui/hooks/useLoadingIndicator.test.ts index 496e13d3..c20ded88 100644 --- a/packages/cli/src/ui/hooks/useLoadingIndicator.test.ts +++ b/packages/cli/src/ui/hooks/useLoadingIndicator.test.ts @@ -4,161 +4,124 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; -import { renderHook, act } from '@testing-library/react'; +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { act, renderHook } from '@testing-library/react'; import { useLoadingIndicator } from './useLoadingIndicator.js'; import { StreamingState } from '../types.js'; import { WITTY_LOADING_PHRASES, PHRASE_CHANGE_INTERVAL_MS, -} from '../constants.js'; +} from './usePhraseCycler.js'; describe('useLoadingIndicator', () => { beforeEach(() => { vi.useFakeTimers(); + vi.clearAllMocks(); }); afterEach(() => { - vi.restoreAllMocks(); + vi.runOnlyPendingTimers(); + vi.useRealTimers(); // Restore real timers after each test }); - it('should initialize with default values when not responding', () => { + it('should initialize with default values when Idle', () => { const { result } = renderHook(() => - useLoadingIndicator(StreamingState.Idle, false), + useLoadingIndicator(StreamingState.Idle), ); expect(result.current.elapsedTime).toBe(0); expect(result.current.currentLoadingPhrase).toBe(WITTY_LOADING_PHRASES[0]); - expect(result.current.shouldShowSpinner).toBe(true); }); - describe('when streamingState is Responding', () => { - it('should increment elapsedTime and cycle phrases when not paused', () => { - const { result } = renderHook(() => - useLoadingIndicator(StreamingState.Responding, false), - ); - expect(result.current.shouldShowSpinner).toBe(true); - expect(result.current.currentLoadingPhrase).toBe( - WITTY_LOADING_PHRASES[0], - ); + it('should reflect values when Responding', () => { + const { result } = renderHook(() => + useLoadingIndicator(StreamingState.Responding), + ); - act(() => { - vi.advanceTimersByTime(1000); - }); - expect(result.current.elapsedTime).toBe(1); + // Initial state before timers advance + expect(result.current.elapsedTime).toBe(0); + expect(result.current.currentLoadingPhrase).toBe(WITTY_LOADING_PHRASES[0]); - act(() => { - vi.advanceTimersByTime(PHRASE_CHANGE_INTERVAL_MS); - }); - expect(result.current.currentLoadingPhrase).toBe( - WITTY_LOADING_PHRASES[1], - ); - expect(result.current.elapsedTime).toBe( - 1 + PHRASE_CHANGE_INTERVAL_MS / 1000, - ); + act(() => { + vi.advanceTimersByTime(PHRASE_CHANGE_INTERVAL_MS); }); + // Phrase should cycle if PHRASE_CHANGE_INTERVAL_MS has passed + // This depends on the actual implementation of usePhraseCycler + // For simplicity, we'll check it's one of the witty phrases + expect(result.current.currentLoadingPhrase).toBe(WITTY_LOADING_PHRASES[1]); + }); - it('should pause elapsedTime, show specific phrase, and hide spinner when paused', () => { - const { result, rerender } = renderHook( - ({ isPaused }) => - useLoadingIndicator(StreamingState.Responding, isPaused), - { initialProps: { isPaused: false } }, - ); + it('should show waiting phrase and retain elapsedTime when WaitingForConfirmation', () => { + const { result, rerender } = renderHook( + ({ streamingState }) => useLoadingIndicator(streamingState), + { initialProps: { streamingState: StreamingState.Responding } }, + ); - act(() => { - vi.advanceTimersByTime(2000); - }); - expect(result.current.elapsedTime).toBe(2); - expect(result.current.shouldShowSpinner).toBe(true); + act(() => { + vi.advanceTimersByTime(60000); + }); + expect(result.current.elapsedTime).toBe(60); - rerender({ isPaused: true }); + rerender({ streamingState: StreamingState.WaitingForConfirmation }); - expect(result.current.currentLoadingPhrase).toBe( - 'Waiting for user confirmation...', - ); - expect(result.current.shouldShowSpinner).toBe(false); + expect(result.current.currentLoadingPhrase).toBe( + 'Waiting for user confirmation...', + ); + expect(result.current.elapsedTime).toBe(60); // Elapsed time should be retained - // Time should not advance while paused - const timeBeforePauseAdv = result.current.elapsedTime; - act(() => { - vi.advanceTimersByTime(3000); - }); - expect(result.current.elapsedTime).toBe(timeBeforePauseAdv); + // Timer should not advance further + act(() => { + vi.advanceTimersByTime(2000); + }); + expect(result.current.elapsedTime).toBe(60); + }); - // Unpause - rerender({ isPaused: false }); - expect(result.current.shouldShowSpinner).toBe(true); - // Phrase should reset to the beginning of witty phrases - expect(result.current.currentLoadingPhrase).toBe( - WITTY_LOADING_PHRASES[0], - ); + it('should reset elapsedTime and use initial phrase when transitioning from WaitingForConfirmation to Responding', () => { + const { result, rerender } = renderHook( + ({ streamingState }) => useLoadingIndicator(streamingState), + { initialProps: { streamingState: StreamingState.Responding } }, + ); - act(() => { - vi.advanceTimersByTime(1000); - }); - // Elapsed time should resume from where it left off - expect(result.current.elapsedTime).toBe(timeBeforePauseAdv + 1); + act(() => { + vi.advanceTimersByTime(5000); // 5s }); + expect(result.current.elapsedTime).toBe(5); - it('should reset timer and phrase when streamingState changes from Responding to Idle', () => { - const { result, rerender } = renderHook( - ({ streamingState }) => useLoadingIndicator(streamingState, false), - { initialProps: { streamingState: StreamingState.Responding } }, - ); - - act(() => { - vi.advanceTimersByTime(PHRASE_CHANGE_INTERVAL_MS + 1000); - }); - expect(result.current.elapsedTime).toBe( - PHRASE_CHANGE_INTERVAL_MS / 1000 + 1, - ); - expect(result.current.currentLoadingPhrase).toBe( - WITTY_LOADING_PHRASES[1], - ); + rerender({ streamingState: StreamingState.WaitingForConfirmation }); + expect(result.current.elapsedTime).toBe(5); + expect(result.current.currentLoadingPhrase).toBe( + 'Waiting for user confirmation...', + ); - rerender({ streamingState: StreamingState.Idle }); + rerender({ streamingState: StreamingState.Responding }); + expect(result.current.elapsedTime).toBe(0); // Should reset + expect(result.current.currentLoadingPhrase).toBe(WITTY_LOADING_PHRASES[0]); - expect(result.current.elapsedTime).toBe(0); - // When idle, the phrase interval should be cleared, but the last phrase might persist - // until the next "Responding" state. The important part is that the timer is reset. - // Depending on exact timing, it might be the last witty phrase or the first. - // For this test, we'll ensure it's one of them. - expect(WITTY_LOADING_PHRASES).toContain( - result.current.currentLoadingPhrase, - ); + act(() => { + vi.advanceTimersByTime(1000); }); + expect(result.current.elapsedTime).toBe(1); }); - it('should clear intervals on unmount', () => { - const { unmount } = renderHook(() => - useLoadingIndicator(StreamingState.Responding, false), - ); - const clearIntervalSpy = vi.spyOn(global, 'clearInterval'); - unmount(); - // Expecting two intervals (elapsedTime and phraseInterval) to be cleared. - expect(clearIntervalSpy).toHaveBeenCalledTimes(2); - }); - - it('should reset to initial witty phrase when unpaused', () => { + it('should reset timer and phrase when streamingState changes from Responding to Idle', () => { const { result, rerender } = renderHook( - ({ isPaused }) => - useLoadingIndicator(StreamingState.Responding, isPaused), - { initialProps: { isPaused: false } }, + ({ streamingState }) => useLoadingIndicator(streamingState), + { initialProps: { streamingState: StreamingState.Responding } }, ); - // Advance to the second witty phrase act(() => { - vi.advanceTimersByTime(PHRASE_CHANGE_INTERVAL_MS); + vi.advanceTimersByTime(10000); // 10s }); - expect(result.current.currentLoadingPhrase).toBe(WITTY_LOADING_PHRASES[1]); + expect(result.current.elapsedTime).toBe(10); - // Pause - rerender({ isPaused: true }); - expect(result.current.currentLoadingPhrase).toBe( - 'Waiting for user confirmation...', - ); + rerender({ streamingState: StreamingState.Idle }); - // Unpause - rerender({ isPaused: false }); + expect(result.current.elapsedTime).toBe(0); expect(result.current.currentLoadingPhrase).toBe(WITTY_LOADING_PHRASES[0]); + + // Timer should not advance + act(() => { + vi.advanceTimersByTime(2000); + }); + expect(result.current.elapsedTime).toBe(0); }); }); diff --git a/packages/cli/src/ui/hooks/useLoadingIndicator.ts b/packages/cli/src/ui/hooks/useLoadingIndicator.ts index ac75986a..46754ac1 100644 --- a/packages/cli/src/ui/hooks/useLoadingIndicator.ts +++ b/packages/cli/src/ui/hooks/useLoadingIndicator.ts @@ -4,98 +4,54 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { useState, useEffect, useRef } from 'react'; -import { - WITTY_LOADING_PHRASES, - PHRASE_CHANGE_INTERVAL_MS, -} from '../constants.js'; import { StreamingState } from '../types.js'; +import { useTimer } from './useTimer.js'; +import { usePhraseCycler } from './usePhraseCycler.js'; +import { useState, useEffect, useRef } from 'react'; // Added useRef -export const useLoadingIndicator = ( - streamingState: StreamingState, - isPaused: boolean, -) => { - const [elapsedTime, setElapsedTime] = useState(0); - const [currentLoadingPhrase, setCurrentLoadingPhrase] = useState( - WITTY_LOADING_PHRASES[0], - ); - const timerRef = useRef<NodeJS.Timeout | null>(null); - const phraseIntervalRef = useRef<NodeJS.Timeout | null>(null); - const currentPhraseIndexRef = useRef<number>(0); +export const useLoadingIndicator = (streamingState: StreamingState) => { + const [timerResetKey, setTimerResetKey] = useState(0); + const isTimerActive = streamingState === StreamingState.Responding; - const [shouldShowSpinner, setShouldShowSpinner] = useState(true); + const elapsedTimeFromTimer = useTimer(isTimerActive, timerResetKey); - useEffect(() => { - if (streamingState === StreamingState.Responding) { - if (!isPaused) { - if (!timerRef.current) { - // No specific action needed here if timer wasn't running and we are not paused. - // Elapsed time continues from where it left off or starts from 0 if it's a fresh start. - } - if (timerRef.current) clearInterval(timerRef.current); - timerRef.current = setInterval(() => { - setElapsedTime((prevTime) => prevTime + 1); - }, 1000); - } else { - if (timerRef.current) { - clearInterval(timerRef.current); - timerRef.current = null; - } - } - } else { - if (timerRef.current) { - clearInterval(timerRef.current); - timerRef.current = null; - } - setElapsedTime(0); - } + const isPhraseCyclingActive = streamingState === StreamingState.Responding; + const isWaiting = streamingState === StreamingState.WaitingForConfirmation; + const currentLoadingPhrase = usePhraseCycler( + isPhraseCyclingActive, + isWaiting, + ); - return () => { - if (timerRef.current) { - clearInterval(timerRef.current); - timerRef.current = null; - } - }; - }, [streamingState, isPaused]); + const [retainedElapsedTime, setRetainedElapsedTime] = useState(0); + const prevStreamingStateRef = useRef<StreamingState | null>(null); useEffect(() => { - if (streamingState === StreamingState.Responding) { - if (!isPaused) { - setShouldShowSpinner(true); - if (!phraseIntervalRef.current) { - currentPhraseIndexRef.current = 0; - setCurrentLoadingPhrase(WITTY_LOADING_PHRASES[0]); - phraseIntervalRef.current = setInterval(() => { - currentPhraseIndexRef.current = - (currentPhraseIndexRef.current + 1) % - WITTY_LOADING_PHRASES.length; - setCurrentLoadingPhrase( - WITTY_LOADING_PHRASES[currentPhraseIndexRef.current], - ); - }, PHRASE_CHANGE_INTERVAL_MS); - } - } else { - setShouldShowSpinner(false); - setCurrentLoadingPhrase('Waiting for user confirmation...'); - if (phraseIntervalRef.current) { - clearInterval(phraseIntervalRef.current); - phraseIntervalRef.current = null; - } - } - } else { - if (phraseIntervalRef.current) { - clearInterval(phraseIntervalRef.current); - phraseIntervalRef.current = null; - } + if ( + prevStreamingStateRef.current === StreamingState.WaitingForConfirmation && + streamingState === StreamingState.Responding + ) { + setTimerResetKey((prevKey) => prevKey + 1); + setRetainedElapsedTime(0); // Clear retained time when going back to responding + } else if ( + streamingState === StreamingState.Idle && + prevStreamingStateRef.current === StreamingState.Responding + ) { + setTimerResetKey((prevKey) => prevKey + 1); // Reset timer when becoming idle from responding + setRetainedElapsedTime(0); + } else if (streamingState === StreamingState.WaitingForConfirmation) { + // Capture the time when entering WaitingForConfirmation + // elapsedTimeFromTimer will hold the last value from when isTimerActive was true. + setRetainedElapsedTime(elapsedTimeFromTimer); } - return () => { - if (phraseIntervalRef.current) { - clearInterval(phraseIntervalRef.current); - phraseIntervalRef.current = null; - } - }; - }, [streamingState, isPaused]); + prevStreamingStateRef.current = streamingState; + }, [streamingState, elapsedTimeFromTimer]); - return { elapsedTime, currentLoadingPhrase, shouldShowSpinner }; + return { + elapsedTime: + streamingState === StreamingState.WaitingForConfirmation + ? retainedElapsedTime + : elapsedTimeFromTimer, + currentLoadingPhrase, + }; }; diff --git a/packages/cli/src/ui/hooks/usePhraseCycler.test.ts b/packages/cli/src/ui/hooks/usePhraseCycler.test.ts new file mode 100644 index 00000000..f5be12e9 --- /dev/null +++ b/packages/cli/src/ui/hooks/usePhraseCycler.test.ts @@ -0,0 +1,111 @@ +/** + * @license + * Copyright 2025 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { renderHook, act } from '@testing-library/react'; +import { + usePhraseCycler, + WITTY_LOADING_PHRASES, + PHRASE_CHANGE_INTERVAL_MS, +} from './usePhraseCycler.js'; + +describe('usePhraseCycler', () => { + beforeEach(() => { + vi.useFakeTimers(); + }); + + afterEach(() => { + vi.restoreAllMocks(); + }); + + it('should initialize with the first witty phrase when not active and not waiting', () => { + const { result } = renderHook(() => usePhraseCycler(false, false)); + expect(result.current).toBe(WITTY_LOADING_PHRASES[0]); + }); + + it('should show "Waiting for user confirmation..." when isWaiting is true', () => { + const { result, rerender } = renderHook( + ({ isActive, isWaiting }) => usePhraseCycler(isActive, isWaiting), + { initialProps: { isActive: true, isWaiting: false } }, + ); + rerender({ isActive: true, isWaiting: true }); + expect(result.current).toBe('Waiting for user confirmation...'); + }); + + it('should not cycle phrases if isActive is false and not waiting', () => { + const { result } = renderHook(() => usePhraseCycler(false, false)); + act(() => { + vi.advanceTimersByTime(PHRASE_CHANGE_INTERVAL_MS * 2); + }); + expect(result.current).toBe(WITTY_LOADING_PHRASES[0]); + }); + + it('should cycle through witty phrases when isActive is true and not waiting', () => { + const { result } = renderHook(() => usePhraseCycler(true, false)); + expect(result.current).toBe(WITTY_LOADING_PHRASES[0]); + + act(() => { + vi.advanceTimersByTime(PHRASE_CHANGE_INTERVAL_MS); + }); + expect(result.current).toBe(WITTY_LOADING_PHRASES[1]); + + act(() => { + vi.advanceTimersByTime(PHRASE_CHANGE_INTERVAL_MS); + }); + expect(result.current).toBe( + WITTY_LOADING_PHRASES[2 % WITTY_LOADING_PHRASES.length], + ); + }); + + it('should reset to the first phrase when isActive becomes true after being false (and not waiting)', () => { + const { result, rerender } = renderHook( + ({ isActive, isWaiting }) => usePhraseCycler(isActive, isWaiting), + { initialProps: { isActive: false, isWaiting: false } }, + ); + // Cycle to a different phrase + rerender({ isActive: true, isWaiting: false }); + act(() => { + vi.advanceTimersByTime(PHRASE_CHANGE_INTERVAL_MS); + }); + expect(result.current).not.toBe(WITTY_LOADING_PHRASES[0]); + + // Set to inactive + rerender({ isActive: false, isWaiting: false }); + expect(result.current).toBe(WITTY_LOADING_PHRASES[0]); // Should reset to first phrase + + // Set back to active + rerender({ isActive: true, isWaiting: false }); + expect(result.current).toBe(WITTY_LOADING_PHRASES[0]); // Should start with the first phrase + }); + + it('should clear phrase interval on unmount when active', () => { + const { unmount } = renderHook(() => usePhraseCycler(true, false)); + const clearIntervalSpy = vi.spyOn(global, 'clearInterval'); + unmount(); + expect(clearIntervalSpy).toHaveBeenCalledOnce(); + }); + + it('should reset to the first witty phrase when transitioning from waiting to active', () => { + const { result, rerender } = renderHook( + ({ isActive, isWaiting }) => usePhraseCycler(isActive, isWaiting), + { initialProps: { isActive: true, isWaiting: false } }, + ); + + // Cycle to a different phrase + act(() => { + vi.advanceTimersByTime(PHRASE_CHANGE_INTERVAL_MS); + }); + expect(result.current).toBe(WITTY_LOADING_PHRASES[1]); + + // Go to waiting state + rerender({ isActive: false, isWaiting: true }); + expect(result.current).toBe('Waiting for user confirmation...'); + + // Go back to active cycling + rerender({ isActive: true, isWaiting: false }); + expect(result.current).toBe(WITTY_LOADING_PHRASES[0]); + }); +}); diff --git a/packages/cli/src/ui/hooks/usePhraseCycler.ts b/packages/cli/src/ui/hooks/usePhraseCycler.ts new file mode 100644 index 00000000..96bad676 --- /dev/null +++ b/packages/cli/src/ui/hooks/usePhraseCycler.ts @@ -0,0 +1,84 @@ +/** + * @license + * Copyright 2025 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import { useState, useEffect, useRef } from 'react'; + +export const WITTY_LOADING_PHRASES = [ + 'Consulting the digital spirits...', + 'Reticulating splines...', + 'Warming up the AI hamsters...', + 'Asking the magic conch shell...', + 'Generating witty retort...', + 'Polishing the algorithms...', + "Don't rush perfection (or my code)...", + 'Brewing fresh bytes...', + 'Counting electrons...', + 'Engaging cognitive processors...', + 'Checking for syntax errors in the universe...', + 'One moment, optimizing humor...', + 'Shuffling punchlines...', + 'Untangling neural nets...', + 'Compiling brilliance...', +]; + +export const PHRASE_CHANGE_INTERVAL_MS = 15000; + +/** + * Custom hook to manage cycling through loading phrases. + * @param isActive Whether the phrase cycling should be active. + * @param isWaiting Whether to show a specific waiting phrase. + * @returns The current loading phrase. + */ +export const usePhraseCycler = (isActive: boolean, isWaiting: boolean) => { + const [currentLoadingPhrase, setCurrentLoadingPhrase] = useState( + WITTY_LOADING_PHRASES[0], + ); + const phraseIntervalRef = useRef<NodeJS.Timeout | null>(null); + const currentPhraseIndexRef = useRef<number>(0); + + useEffect(() => { + if (isWaiting) { + setCurrentLoadingPhrase('Waiting for user confirmation...'); + if (phraseIntervalRef.current) { + clearInterval(phraseIntervalRef.current); + phraseIntervalRef.current = null; + } + } else if (isActive) { + if (phraseIntervalRef.current) { + clearInterval(phraseIntervalRef.current); + } + // Reset to the first witty phrase when starting to respond + currentPhraseIndexRef.current = 0; + setCurrentLoadingPhrase(WITTY_LOADING_PHRASES[0]); + + phraseIntervalRef.current = setInterval(() => { + currentPhraseIndexRef.current = + (currentPhraseIndexRef.current + 1) % WITTY_LOADING_PHRASES.length; + setCurrentLoadingPhrase( + WITTY_LOADING_PHRASES[currentPhraseIndexRef.current], + ); + }, PHRASE_CHANGE_INTERVAL_MS); + } else { + // Idle or other states, clear the phrase interval + // and reset to the first phrase for next active state. + if (phraseIntervalRef.current) { + clearInterval(phraseIntervalRef.current); + phraseIntervalRef.current = null; + } + setCurrentLoadingPhrase(WITTY_LOADING_PHRASES[0]); + currentPhraseIndexRef.current = 0; + } + + return () => { + if (phraseIntervalRef.current) { + clearInterval(phraseIntervalRef.current); + phraseIntervalRef.current = null; + } + }; + }, [isActive, isWaiting]); + + return currentLoadingPhrase; +}; diff --git a/packages/cli/src/ui/hooks/useTimer.test.ts b/packages/cli/src/ui/hooks/useTimer.test.ts new file mode 100644 index 00000000..20d44d17 --- /dev/null +++ b/packages/cli/src/ui/hooks/useTimer.test.ts @@ -0,0 +1,120 @@ +/** + * @license + * Copyright 2025 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { renderHook, act } from '@testing-library/react'; +import { useTimer } from './useTimer.js'; + +describe('useTimer', () => { + beforeEach(() => { + vi.useFakeTimers(); + }); + + afterEach(() => { + vi.restoreAllMocks(); + }); + + it('should initialize with 0', () => { + const { result } = renderHook(() => useTimer(false, 0)); + expect(result.current).toBe(0); + }); + + it('should not increment time if isActive is false', () => { + const { result } = renderHook(() => useTimer(false, 0)); + act(() => { + vi.advanceTimersByTime(5000); + }); + expect(result.current).toBe(0); + }); + + it('should increment time every second if isActive is true', () => { + const { result } = renderHook(() => useTimer(true, 0)); + act(() => { + vi.advanceTimersByTime(1000); + }); + expect(result.current).toBe(1); + act(() => { + vi.advanceTimersByTime(2000); + }); + expect(result.current).toBe(3); + }); + + it('should reset to 0 and start incrementing when isActive becomes true from false', () => { + const { result, rerender } = renderHook( + ({ isActive, resetKey }) => useTimer(isActive, resetKey), + { initialProps: { isActive: false, resetKey: 0 } }, + ); + expect(result.current).toBe(0); + + rerender({ isActive: true, resetKey: 0 }); + expect(result.current).toBe(0); // Should reset to 0 upon becoming active + + act(() => { + vi.advanceTimersByTime(1000); + }); + expect(result.current).toBe(1); + }); + + it('should reset to 0 when resetKey changes while active', () => { + const { result, rerender } = renderHook( + ({ isActive, resetKey }) => useTimer(isActive, resetKey), + { initialProps: { isActive: true, resetKey: 0 } }, + ); + act(() => { + vi.advanceTimersByTime(3000); // 3s + }); + expect(result.current).toBe(3); + + rerender({ isActive: true, resetKey: 1 }); // Change resetKey + expect(result.current).toBe(0); // Should reset to 0 + + act(() => { + vi.advanceTimersByTime(1000); + }); + expect(result.current).toBe(1); // Starts incrementing from 0 + }); + + it('should be 0 if isActive is false, regardless of resetKey changes', () => { + const { result, rerender } = renderHook( + ({ isActive, resetKey }) => useTimer(isActive, resetKey), + { initialProps: { isActive: false, resetKey: 0 } }, + ); + expect(result.current).toBe(0); + + rerender({ isActive: false, resetKey: 1 }); + expect(result.current).toBe(0); + }); + + it('should clear timer on unmount', () => { + const { unmount } = renderHook(() => useTimer(true, 0)); + const clearIntervalSpy = vi.spyOn(global, 'clearInterval'); + unmount(); + expect(clearIntervalSpy).toHaveBeenCalledOnce(); + }); + + it('should preserve elapsedTime when isActive becomes false, and reset to 0 when it becomes active again', () => { + const { result, rerender } = renderHook( + ({ isActive, resetKey }) => useTimer(isActive, resetKey), + { initialProps: { isActive: true, resetKey: 0 } }, + ); + + act(() => { + vi.advanceTimersByTime(3000); // Advance to 3 seconds + }); + expect(result.current).toBe(3); + + rerender({ isActive: false, resetKey: 0 }); + expect(result.current).toBe(3); // Time should be preserved when timer becomes inactive + + // Now make it active again, it should reset to 0 + rerender({ isActive: true, resetKey: 0 }); + expect(result.current).toBe(0); + act(() => { + vi.advanceTimersByTime(1000); + }); + expect(result.current).toBe(1); + }); +}); diff --git a/packages/cli/src/ui/hooks/useTimer.ts b/packages/cli/src/ui/hooks/useTimer.ts new file mode 100644 index 00000000..53c266dd --- /dev/null +++ b/packages/cli/src/ui/hooks/useTimer.ts @@ -0,0 +1,65 @@ +/** + * @license + * Copyright 2025 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import { useState, useEffect, useRef } from 'react'; + +/** + * Custom hook to manage a timer that increments every second. + * @param isActive Whether the timer should be running. + * @param resetKey A key that, when changed, will reset the timer to 0 and restart the interval. + * @returns The elapsed time in seconds. + */ +export const useTimer = (isActive: boolean, resetKey: unknown) => { + const [elapsedTime, setElapsedTime] = useState(0); + const timerRef = useRef<NodeJS.Timeout | null>(null); + const prevResetKeyRef = useRef(resetKey); + const prevIsActiveRef = useRef(isActive); + + useEffect(() => { + let shouldResetTime = false; + + if (prevResetKeyRef.current !== resetKey) { + shouldResetTime = true; + prevResetKeyRef.current = resetKey; + } + + if (prevIsActiveRef.current === false && isActive) { + // Transitioned from inactive to active + shouldResetTime = true; + } + + if (shouldResetTime) { + setElapsedTime(0); + } + prevIsActiveRef.current = isActive; + + // Manage interval + if (isActive) { + // Clear previous interval unconditionally before starting a new one + // This handles resetKey changes while active, ensuring a fresh interval start. + if (timerRef.current) { + clearInterval(timerRef.current); + } + timerRef.current = setInterval(() => { + setElapsedTime((prev) => prev + 1); + }, 1000); + } else { + if (timerRef.current) { + clearInterval(timerRef.current); + timerRef.current = null; + } + } + + return () => { + if (timerRef.current) { + clearInterval(timerRef.current); + timerRef.current = null; + } + }; + }, [isActive, resetKey]); + + return elapsedTime; +}; |
