diff --git a/packages/cli/src/ui/hooks/useInputHistoryStore.test.ts b/packages/cli/src/ui/hooks/useInputHistoryStore.test.ts index 842009594d8..1d803ea6c02 100644 --- a/packages/cli/src/ui/hooks/useInputHistoryStore.test.ts +++ b/packages/cli/src/ui/hooks/useInputHistoryStore.test.ts @@ -4,7 +4,7 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { act } from 'react'; +import { act, StrictMode } from 'react'; import { renderHook } from '../../test-utils/render.js'; import { vi, describe, it, expect, beforeEach } from 'vitest'; import { useInputHistoryStore } from './useInputHistoryStore.js'; @@ -302,4 +302,121 @@ describe('useInputHistoryStore', () => { ]); }); }); + + describe('state updates stay outside updater functions', () => { + // React may call an updater more than once for a single update (StrictMode + // double-invoke, replays under batching). addInput used to nest a + // setPastSessionMessages call inside the current-session updater and run + // recalculateHistory - itself a setState - from there. The resulting + // history happened to be idempotent, so the tests below pin both the + // history and the render count, which is what the nesting actually cost. + + it('does not schedule extra renders per submit under StrictMode', async () => { + const renders: string[][] = []; + const { result } = await renderHook( + () => { + const store = useInputHistoryStore(); + renders.push(store.inputHistory); + return store; + }, + { wrapper: StrictMode as never }, + ); + + const before = renders.length; + act(() => { + result.current.addInput('only once'); + }); + const rendersForOneSubmit = renders.length - before; + + expect(result.current.inputHistory).toEqual(['only once']); + // The nested-updater version queued a redundant past-messages update on + // top of the history update, costing an extra render pass per submit. + expect(rendersForOneSubmit).toBeLessThanOrEqual(2); + }); + + it('does not grow the render cost as submits accumulate', async () => { + // addInput only writes the history state now. Should a redundant + // per-submit state write reappear, the extra dispatch shows up here. + const renders: string[][] = []; + const { result } = await renderHook( + () => { + const store = useInputHistoryStore(); + renders.push(store.inputHistory); + return store; + }, + { wrapper: StrictMode as never }, + ); + + const first = renders.length; + act(() => { + result.current.addInput('one'); + }); + const costOfFirst = renders.length - first; + + const second = renders.length; + act(() => { + result.current.addInput('two'); + }); + const costOfSecond = renders.length - second; + + expect(result.current.inputHistory).toEqual(['one', 'two']); + expect(costOfSecond).toBeLessThanOrEqual(costOfFirst); + }); + + it('keeps every submit when several are batched into one update', async () => { + const { result } = await renderHook(() => useInputHistoryStore(), { + wrapper: StrictMode as never, + }); + + // Same act() means all three updates are queued together, which is what + // makes an updater-nested update fire once per queued outer update. + act(() => { + result.current.addInput('a'); + result.current.addInput('b'); + result.current.addInput('c'); + }); + + expect(result.current.inputHistory).toEqual(['a', 'b', 'c']); + }); + + it('keeps past messages visible after batched submits', async () => { + const mockLogger = { + getPreviousUserMessages: vi.fn().mockResolvedValue(['past2', 'past1']), + }; + + const { result } = await renderHook(() => useInputHistoryStore(), { + wrapper: StrictMode as never, + }); + + await act(async () => { + await result.current.initializeFromLogger(mockLogger); + }); + + act(() => { + result.current.addInput('new1'); + result.current.addInput('new2'); + }); + + expect(result.current.inputHistory).toEqual([ + 'past1', + 'past2', + 'new1', + 'new2', + ]); + }); + + it('still deduplicates consecutive duplicates across batched submits', async () => { + const { result } = await renderHook(() => useInputHistoryStore(), { + wrapper: StrictMode as never, + }); + + act(() => { + result.current.addInput('same'); + result.current.addInput('same'); + result.current.addInput('other'); + }); + + expect(result.current.inputHistory).toEqual(['same', 'other']); + }); + }); }); diff --git a/packages/cli/src/ui/hooks/useInputHistoryStore.ts b/packages/cli/src/ui/hooks/useInputHistoryStore.ts index ff53f2ba959..5058ab1be00 100644 --- a/packages/cli/src/ui/hooks/useInputHistoryStore.ts +++ b/packages/cli/src/ui/hooks/useInputHistoryStore.ts @@ -5,7 +5,7 @@ */ import { debugLogger } from '@google/gemini-cli-core'; -import { useState, useCallback } from 'react'; +import { useState, useCallback, useRef } from 'react'; interface Logger { getPreviousUserMessages(): Promise; @@ -24,11 +24,15 @@ export interface UseInputHistoryStoreReturn { export function useInputHistoryStore(): UseInputHistoryStoreReturn { const [inputHistory, setInputHistory] = useState([]); const [_pastSessionMessages, setPastSessionMessages] = useState([]); - const [_currentSessionMessages, setCurrentSessionMessages] = useState< - string[] - >([]); const [isInitialized, setIsInitialized] = useState(false); + // Mirrors of the two message lists, read by addInput so it can derive the + // next history without reading state from inside an updater. State updaters + // must be pure and may run more than once (StrictMode double-invoke, replays + // under batching), so they cannot be used to sequence dependent updates. + const pastRef = useRef([]); + const currentRef = useRef([]); + /** * Recalculate the complete input history from past and current sessions. * Applies the same deduplication logic as the previous implementation. @@ -65,6 +69,7 @@ export function useInputHistoryStore(): UseInputHistoryStoreReturn { try { const pastMessages = (await logger.getPreviousUserMessages()) || []; + pastRef.current = pastMessages; setPastSessionMessages(pastMessages); // Store as newest first recalculateHistory([], pastMessages); setIsInitialized(true); @@ -74,6 +79,7 @@ export function useInputHistoryStore(): UseInputHistoryStoreReturn { 'Failed to initialize input history from logger:', error, ); + pastRef.current = []; setPastSessionMessages([]); recalculateHistory([], []); setIsInitialized(true); @@ -91,19 +97,16 @@ export function useInputHistoryStore(): UseInputHistoryStoreReturn { const trimmedInput = input.trim(); if (!trimmedInput) return; // Filter empty/whitespace-only inputs - setCurrentSessionMessages((prevCurrent) => { - const newCurrentSession = [...prevCurrent, trimmedInput]; - - setPastSessionMessages((prevPast) => { - recalculateHistory( - newCurrentSession.slice().reverse(), // Convert to newest first - prevPast, - ); - return prevPast; // No change to past messages - }); + // Derive everything up front, then issue a plain state update. Keeping the + // derivation out of the updaters means a replayed or double-invoked + // updater cannot run recalculateHistory (itself a setState) again. + const newCurrentSession = [...currentRef.current, trimmedInput]; + currentRef.current = newCurrentSession; - return newCurrentSession; - }); + recalculateHistory( + newCurrentSession.slice().reverse(), // Convert to newest first + pastRef.current, + ); }, [recalculateHistory], );