Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
90 changes: 89 additions & 1 deletion packages/cli/src/ui/hooks/useInputHistoryStore.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -302,4 +302,92 @@ 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
// setPastSessionMessages inside the setCurrentSessionMessages 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('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']);
});
});
});
33 changes: 20 additions & 13 deletions packages/cli/src/ui/hooks/useInputHistoryStore.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<string[]>;
Expand All @@ -29,6 +29,13 @@ export function useInputHistoryStore(): UseInputHistoryStoreReturn {
>([]);
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<string[]>([]);
const currentRef = useRef<string[]>([]);

/**
* Recalculate the complete input history from past and current sessions.
* Applies the same deduplication logic as the previous implementation.
Expand Down Expand Up @@ -65,6 +72,7 @@ export function useInputHistoryStore(): UseInputHistoryStoreReturn {

try {
const pastMessages = (await logger.getPreviousUserMessages()) || [];
pastRef.current = pastMessages;
setPastSessionMessages(pastMessages); // Store as newest first
recalculateHistory([], pastMessages);
setIsInitialized(true);
Expand All @@ -74,6 +82,7 @@ export function useInputHistoryStore(): UseInputHistoryStoreReturn {
'Failed to initialize input history from logger:',
error,
);
pastRef.current = [];
setPastSessionMessages([]);
recalculateHistory([], []);
setIsInitialized(true);
Expand All @@ -91,19 +100,17 @@ 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 plain state updates. 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;
});
setCurrentSessionMessages(newCurrentSession);
recalculateHistory(
newCurrentSession.slice().reverse(), // Convert to newest first
pastRef.current,
);
Comment on lines +103 to +109

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

_currentSessionMessages is a dead state that is never read or returned by this hook. Since recalculateHistory already updates the inputHistory state, calling setCurrentSessionMessages is redundant and adds unnecessary state dispatch overhead. Please remove this call.

      const newCurrentSession = [...currentRef.current, trimmedInput];
      currentRef.current = newCurrentSession;

      recalculateHistory(
        newCurrentSession.slice().reverse(),
        pastRef.current,
      );

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 19be911. I removed _currentSessionMessages and its setter entirely; currentRef remains the synchronous source for the next update, and recalculateHistory is now the only state dispatch for this path. I also kept the regression coverage that asserts one setInputHistory dispatch per add, so reintroducing a redundant state update in this flow is detectable.

},
[recalculateHistory],
);
Expand Down