From b4c16d1f56f4e19fffd3d7608b410570f35045f9 Mon Sep 17 00:00:00 2001 From: Jacob Richman Date: Sat, 24 May 2025 00:44:17 -0700 Subject: 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). --- .../src/ui/components/LoadingIndicator.test.tsx | 189 ++++++++++++++------- 1 file changed, 128 insertions(+), 61 deletions(-) (limited to 'packages/cli/src/ui/components/LoadingIndicator.test.tsx') 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 MockSpinner; - }, + default: () => MockSpinner, })); +const renderWithContext = ( + ui: React.ReactElement, + streamingStateValue: StreamingState, +) => { + const contextValue: StreamingContextType = { + streamingState: streamingStateValue, + }; + return render( + + {ui} + , + ); +}; + describe('', () => { - it('should not render when isLoading is false', () => { - const { lastFrame } = render( - , + const defaultProps = { + currentLoadingPhrase: 'Loading...', + elapsedTime: 5, + }; + + it('should not render when streamingState is Idle', () => { + const { lastFrame } = renderWithContext( + , + 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( - , + it('should render spinner, phrase, and time when streamingState is Responding', () => { + const { lastFrame } = renderWithContext( + , + 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( - , + it('should render phrase and time but no spinner when streamingState is WaitingForConfirmation', () => { + const props = { + currentLoadingPhrase: 'Confirm action', + elapsedTime: 10, + }; + const { lastFrame } = renderWithContext( + , + 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( - , + const props = { + currentLoadingPhrase: 'Processing data...', + elapsedTime: 3, + }; + const { lastFrame } = renderWithContext( + , + StreamingState.Responding, + ); + expect(lastFrame()).toContain('Processing data...'); + }); + + it('should display the elapsedTime correctly when Responding', () => { + const props = { + currentLoadingPhrase: 'Working...', + elapsedTime: 8, + }; + const { lastFrame } = renderWithContext( + , + StreamingState.Responding, + ); + expect(lastFrame()).toContain('(esc to cancel, 8s)'); + }); + + it('should render rightContent when provided', () => { + const rightContent = Extra Info; + const { lastFrame } = renderWithContext( + , + StreamingState.Responding, ); - expect(lastFrame()).toContain(specificPhrase); + expect(lastFrame()).toContain('Extra Info'); }); - it('should display the elapsedTime correctly', () => { - const specificTime = 7; - const { lastFrame } = render( - , + it('should transition correctly between states using rerender', () => { + const { lastFrame, rerender } = renderWithContext( + , + StreamingState.Idle, ); - expect(lastFrame()).toContain(`(esc to cancel, ${specificTime}s)`); + expect(lastFrame()).toBe(''); // Initial: Idle + + // Transition to Responding + rerender( + + + , + ); + let output = lastFrame(); + expect(output).toContain('MockSpinner'); + expect(output).toContain('Now Responding'); + expect(output).toContain('(esc to cancel, 2s)'); + + // Transition to WaitingForConfirmation + rerender( + + + , + ); + 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( + + + , + ); + expect(lastFrame()).toBe(''); }); }); -- cgit v1.2.3