From a96ff934eacdedfea16ae91a0e63858b15c4b593 Mon Sep 17 00:00:00 2001 From: Jacob Richman Date: Fri, 23 May 2025 09:40:01 -0700 Subject: [PATCH] Fix bug updating the cursor after navigating history. (#507) --- .../cli/src/ui/components/InputPrompt.tsx | 10 +- .../cli/src/ui/hooks/useInputHistory.test.ts | 261 ++++++++++++++++++ packages/cli/src/ui/hooks/useInputHistory.ts | 14 +- 3 files changed, 269 insertions(+), 16 deletions(-) create mode 100644 packages/cli/src/ui/hooks/useInputHistory.test.ts diff --git a/packages/cli/src/ui/components/InputPrompt.tsx b/packages/cli/src/ui/components/InputPrompt.tsx index bc514464..730aed48 100644 --- a/packages/cli/src/ui/components/InputPrompt.tsx +++ b/packages/cli/src/ui/components/InputPrompt.tsx @@ -81,20 +81,12 @@ export const InputPrompt: React.FC = ({ [onSubmit, buffer, resetCompletionState], ); - const onChangeAndMoveCursor = useCallback( - (newValue: string) => { - buffer.setText(newValue); - buffer.move('end'); - }, - [buffer], - ); - const inputHistory = useInputHistory({ userMessages, onSubmit: handleSubmitAndClear, isActive: !completion.showSuggestions, currentQuery: buffer.text, - onChangeAndMoveCursor, + onChange: buffer.setText, }); const completionSuggestions = completion.suggestions; diff --git a/packages/cli/src/ui/hooks/useInputHistory.test.ts b/packages/cli/src/ui/hooks/useInputHistory.test.ts new file mode 100644 index 00000000..8d10c376 --- /dev/null +++ b/packages/cli/src/ui/hooks/useInputHistory.test.ts @@ -0,0 +1,261 @@ +/** + * @license + * Copyright 2025 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import { act, renderHook } from '@testing-library/react'; +import { useInputHistory } from './useInputHistory.js'; + +describe('useInputHistory', () => { + const mockOnSubmit = vi.fn(); + const mockOnChange = vi.fn(); + + beforeEach(() => { + vi.clearAllMocks(); + }); + + const userMessages = ['message 1', 'message 2', 'message 3']; + + it('should initialize with historyIndex -1 and empty originalQueryBeforeNav', () => { + const { result } = renderHook(() => + useInputHistory({ + userMessages: [], + onSubmit: mockOnSubmit, + isActive: true, + currentQuery: '', + onChange: mockOnChange, + }), + ); + + // Internal state is not directly testable, but we can infer from behavior. + // Attempting to navigate down should do nothing if historyIndex is -1. + act(() => { + result.current.navigateDown(); + }); + expect(mockOnChange).not.toHaveBeenCalled(); + }); + + describe('handleSubmit', () => { + it('should call onSubmit with trimmed value and reset history', () => { + const { result } = renderHook(() => + useInputHistory({ + userMessages, + onSubmit: mockOnSubmit, + isActive: true, + currentQuery: ' test query ', + onChange: mockOnChange, + }), + ); + + act(() => { + result.current.handleSubmit(' submit value '); + }); + + expect(mockOnSubmit).toHaveBeenCalledWith('submit value'); + // Check if history is reset (e.g., by trying to navigate down) + act(() => { + result.current.navigateDown(); + }); + expect(mockOnChange).not.toHaveBeenCalled(); + }); + + it('should not call onSubmit if value is empty after trimming', () => { + const { result } = renderHook(() => + useInputHistory({ + userMessages, + onSubmit: mockOnSubmit, + isActive: true, + currentQuery: '', + onChange: mockOnChange, + }), + ); + + act(() => { + result.current.handleSubmit(' '); + }); + + expect(mockOnSubmit).not.toHaveBeenCalled(); + }); + }); + + describe('navigateUp', () => { + it('should not navigate if isActive is false', () => { + const { result } = renderHook(() => + useInputHistory({ + userMessages, + onSubmit: mockOnSubmit, + isActive: false, + currentQuery: 'current', + onChange: mockOnChange, + }), + ); + act(() => { + const navigated = result.current.navigateUp(); + expect(navigated).toBe(false); + }); + expect(mockOnChange).not.toHaveBeenCalled(); + }); + + it('should not navigate if userMessages is empty', () => { + const { result } = renderHook(() => + useInputHistory({ + userMessages: [], + onSubmit: mockOnSubmit, + isActive: true, + currentQuery: 'current', + onChange: mockOnChange, + }), + ); + act(() => { + const navigated = result.current.navigateUp(); + expect(navigated).toBe(false); + }); + expect(mockOnChange).not.toHaveBeenCalled(); + }); + + it('should call onChange with the last message when navigating up from initial state', () => { + const currentQuery = 'current query'; + const { result } = renderHook(() => + useInputHistory({ + userMessages, + onSubmit: mockOnSubmit, + isActive: true, + currentQuery, + onChange: mockOnChange, + }), + ); + + act(() => { + result.current.navigateUp(); + }); + + expect(mockOnChange).toHaveBeenCalledWith(userMessages[2]); // Last message + }); + + it('should store currentQuery as originalQueryBeforeNav on first navigateUp', () => { + const currentQuery = 'original user input'; + const { result } = renderHook(() => + useInputHistory({ + userMessages, + onSubmit: mockOnSubmit, + isActive: true, + currentQuery, + onChange: mockOnChange, + }), + ); + + act(() => { + result.current.navigateUp(); // historyIndex becomes 0 + }); + expect(mockOnChange).toHaveBeenCalledWith(userMessages[2]); + + // Navigate down to restore original query + act(() => { + result.current.navigateDown(); // historyIndex becomes -1 + }); + expect(mockOnChange).toHaveBeenCalledWith(currentQuery); + }); + + it('should navigate through history messages on subsequent navigateUp calls', () => { + const { result } = renderHook(() => + useInputHistory({ + userMessages, + onSubmit: mockOnSubmit, + isActive: true, + currentQuery: '', + onChange: mockOnChange, + }), + ); + + act(() => { + result.current.navigateUp(); // Navigates to 'message 3' + }); + expect(mockOnChange).toHaveBeenCalledWith(userMessages[2]); + + act(() => { + result.current.navigateUp(); // Navigates to 'message 2' + }); + expect(mockOnChange).toHaveBeenCalledWith(userMessages[1]); + + act(() => { + result.current.navigateUp(); // Navigates to 'message 1' + }); + expect(mockOnChange).toHaveBeenCalledWith(userMessages[0]); + }); + }); + + describe('navigateDown', () => { + it('should not navigate if isActive is false', () => { + const initialProps = { + userMessages, + onSubmit: mockOnSubmit, + isActive: true, // Start active to allow setup navigation + currentQuery: 'current', + onChange: mockOnChange, + }; + const { result, rerender } = renderHook( + (props) => useInputHistory(props), + { + initialProps, + }, + ); + + // First navigate up to have something in history + act(() => { + result.current.navigateUp(); + }); + mockOnChange.mockClear(); // Clear calls from setup + + // Set isActive to false for the actual test + rerender({ ...initialProps, isActive: false }); + + act(() => { + const navigated = result.current.navigateDown(); + expect(navigated).toBe(false); + }); + expect(mockOnChange).not.toHaveBeenCalled(); + }); + + it('should not navigate if historyIndex is -1 (not in history navigation)', () => { + const { result } = renderHook(() => + useInputHistory({ + userMessages, + onSubmit: mockOnSubmit, + isActive: true, + currentQuery: 'current', + onChange: mockOnChange, + }), + ); + act(() => { + const navigated = result.current.navigateDown(); + expect(navigated).toBe(false); + }); + expect(mockOnChange).not.toHaveBeenCalled(); + }); + + it('should restore originalQueryBeforeNav when navigating down to initial state', () => { + const originalQuery = 'my original input'; + const { result } = renderHook(() => + useInputHistory({ + userMessages, + onSubmit: mockOnSubmit, + isActive: true, + currentQuery: originalQuery, + onChange: mockOnChange, + }), + ); + + act(() => { + result.current.navigateUp(); // Navigates to 'message 3', stores 'originalQuery' + }); + expect(mockOnChange).toHaveBeenCalledWith(userMessages[2]); + mockOnChange.mockClear(); + + act(() => { + result.current.navigateDown(); // Navigates back to original query + }); + expect(mockOnChange).toHaveBeenCalledWith(originalQuery); + }); + }); +}); diff --git a/packages/cli/src/ui/hooks/useInputHistory.ts b/packages/cli/src/ui/hooks/useInputHistory.ts index 90947662..8225d4fc 100644 --- a/packages/cli/src/ui/hooks/useInputHistory.ts +++ b/packages/cli/src/ui/hooks/useInputHistory.ts @@ -11,7 +11,7 @@ interface UseInputHistoryProps { onSubmit: (value: string) => void; isActive: boolean; currentQuery: string; // Renamed from query to avoid confusion - onChangeAndMoveCursor: (value: string) => void; + onChange: (value: string) => void; } interface UseInputHistoryReturn { @@ -25,7 +25,7 @@ export function useInputHistory({ onSubmit, isActive, currentQuery, - onChangeAndMoveCursor: setQueryAndMoveCursor, + onChange, }: UseInputHistoryProps): UseInputHistoryReturn { const [historyIndex, setHistoryIndex] = useState(-1); const [originalQueryBeforeNav, setOriginalQueryBeforeNav] = @@ -65,14 +65,14 @@ export function useInputHistory({ if (nextIndex !== historyIndex) { setHistoryIndex(nextIndex); const newValue = userMessages[userMessages.length - 1 - nextIndex]; - setQueryAndMoveCursor(newValue); // Call the prop passed from parent + onChange(newValue); return true; } return false; }, [ historyIndex, setHistoryIndex, - setQueryAndMoveCursor, + onChange, userMessages, isActive, currentQuery, // Use currentQuery from props @@ -88,17 +88,17 @@ export function useInputHistory({ if (nextIndex === -1) { // Reached the end of history navigation, restore original query - setQueryAndMoveCursor(originalQueryBeforeNav); + onChange(originalQueryBeforeNav); } else { const newValue = userMessages[userMessages.length - 1 - nextIndex]; - setQueryAndMoveCursor(newValue); + onChange(newValue); } return true; }, [ historyIndex, setHistoryIndex, originalQueryBeforeNav, - setQueryAndMoveCursor, + onChange, userMessages, isActive, ]);