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).

1 files changed, 74 insertions, 111 deletions

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);
   });
 });