abbyswag/qwen-code
1
0
Fork 0
mirror of https://github.com/QwenLM/qwen-code.git synced 2025-12-20 08:47:44 +00:00
Code Issues Packages Projects Releases Wiki Activity

Code review comment fixes and some refactors. (#525)

Browse Source 
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).
This commit is contained in:
Jacob Richman
2025-05-24 00:44:17 -07:00
committed by GitHub
parent 1c3d9d7623
commit b4c16d1f56
17 changed files with 1222 additions and 771 deletions
Download Patch File Download Diff File Expand all files Collapse all files

211
packages/cli/src/ui/hooks/useLoadingIndicator.test.ts
View File

@@ -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],
);
act(() => {
vi.advanceTimersByTime(1000);
});
expect(result.current.elapsedTime).toBe(1);
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,
);
});
it('should pause elapsedTime, show specific phrase, and hide spinner when paused', () => {
const { result, rerender } = renderHook(
({ isPaused }) =>
useLoadingIndicator(StreamingState.Responding, isPaused),
{ initialProps: { isPaused: false } },
);
act(() => {
vi.advanceTimersByTime(2000);
});
expect(result.current.elapsedTime).toBe(2);
expect(result.current.shouldShowSpinner).toBe(true);
rerender({ isPaused: true });
expect(result.current.currentLoadingPhrase).toBe(
'Waiting for user confirmation...',
);
expect(result.current.shouldShowSpinner).toBe(false);
// Time should not advance while paused
const timeBeforePauseAdv = result.current.elapsedTime;
act(() => {
vi.advanceTimersByTime(3000);
});
expect(result.current.elapsedTime).toBe(timeBeforePauseAdv);
// 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],
);
act(() => {
vi.advanceTimersByTime(1000);
});
// Elapsed time should resume from where it left off
expect(result.current.elapsedTime).toBe(timeBeforePauseAdv + 1);
});
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.Idle });
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,
);
});
});
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', () => {
const { result, rerender } = renderHook(
({ isPaused }) =>
useLoadingIndicator(StreamingState.Responding, isPaused),
{ initialProps: { isPaused: false } },
it('should reflect values when Responding', () => {
const { result } = renderHook(() =>
useLoadingIndicator(StreamingState.Responding),
);
// Advance to the second witty phrase
// 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);
});
// 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]);
});
// Pause
rerender({ isPaused: true });
it('should show waiting phrase and retain elapsedTime when WaitingForConfirmation', () => {
const { result, rerender } = renderHook(
({ streamingState }) => useLoadingIndicator(streamingState),
{ initialProps: { streamingState: StreamingState.Responding } },
);
act(() => {
vi.advanceTimersByTime(60000);
});
expect(result.current.elapsedTime).toBe(60);
rerender({ streamingState: StreamingState.WaitingForConfirmation });
expect(result.current.currentLoadingPhrase).toBe(
'Waiting for user confirmation...',
);
expect(result.current.elapsedTime).toBe(60); // Elapsed time should be retained
// Timer should not advance further
act(() => {
vi.advanceTimersByTime(2000);
});
expect(result.current.elapsedTime).toBe(60);
});
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(5000); // 5s
});
expect(result.current.elapsedTime).toBe(5);
rerender({ streamingState: StreamingState.WaitingForConfirmation });
expect(result.current.elapsedTime).toBe(5);
expect(result.current.currentLoadingPhrase).toBe(
'Waiting for user confirmation...',
);
// Unpause
rerender({ isPaused: false });
rerender({ streamingState: StreamingState.Responding });
expect(result.current.elapsedTime).toBe(0); // Should reset
expect(result.current.currentLoadingPhrase).toBe(WITTY_LOADING_PHRASES[0]);
act(() => {
vi.advanceTimersByTime(1000);
});
expect(result.current.elapsedTime).toBe(1);
});
it('should reset timer and phrase when streamingState changes from Responding to Idle', () => {
const { result, rerender } = renderHook(
({ streamingState }) => useLoadingIndicator(streamingState),
{ initialProps: { streamingState: StreamingState.Responding } },
);
act(() => {
vi.advanceTimersByTime(10000); // 10s
});
expect(result.current.elapsedTime).toBe(10);
rerender({ streamingState: StreamingState.Idle });
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);
});
});