fix(cli): keep useInputHistoryStore state updaters pure - #29098
Conversation
…i#29046) addInput() scheduled setPastSessionMessages() and recalculateHistory() inside the setCurrentSessionMessages() updater, so side effects ran inside an updater function (double-invoked under StrictMode, once per queued update under batching). Compute the recalculation in an effect keyed to the committed session state instead, and reduce addInput() to a pure functional update.
|
📊 PR Size: size/M
|
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request improves the stability and predictability of the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request refactors the useInputHistoryStore hook to handle rapid, batched inputs correctly and adds a corresponding test case. However, the implementation introduces a useEffect hook to synchronize and recalculate the derived inputHistory state. The reviewer correctly identified this as an anti-pattern that can lead to double rendering, terminal flickering in the Ink CLI framework, and state inconsistency. It is highly recommended to simplify the hook by deriving inputHistory directly during render using useMemo instead.
| useEffect(() => { | ||
| recalculateHistory( | ||
| currentSessionMessages.slice().reverse(), // Convert to newest first | ||
| pastSessionMessages, | ||
| ); | ||
| }, [currentSessionMessages, pastSessionMessages, recalculateHistory]); |
There was a problem hiding this comment.
Anti-pattern: Synchronizing State with useEffect (Derived State)
Using useEffect to synchronize state (inputHistory) when other states (currentSessionMessages or pastSessionMessages) change is a known React anti-pattern.
Why this is a High-Severity Issue:
- Double Rendering & CLI Performance: Updating state inside
useEffectforces React to perform an immediate second render pass. In a CLI application using Ink, extra render passes can cause terminal flickering, cursor glitches, or unnecessary redraw overhead. - State Inconsistency: During the first render pass,
inputHistorywill contain stale data whilecurrentSessionMessageshas already updated. Any component relying on both will see inconsistent state. - Unnecessary Complexity: It requires maintaining three separate states (
inputHistory,currentSessionMessages,pastSessionMessages), auseEffect, and arecalculateHistorycallback.
Recommended Solution:
Derive inputHistory directly during render using useMemo. This guarantees that inputHistory is always in sync, avoids the extra render pass, and simplifies the hook significantly.
Since some of the code to be removed (like the recalculateHistory definition and the initial useState for inputHistory) lies outside the current diff hunks, you will need to manually apply these changes across the file:
- Remove the
inputHistorystate:// Remove this line: // const [inputHistory, setInputHistory] = useState<string[]>([]);
- Remove the
recalculateHistorycallback entirely. - Remove the
useEffectblock. - Add the
useMemoderivation:const inputHistory = useMemo(() => { const currentSession = currentSessionMessages.toReversed(); // Node 20+ supports toReversed() const combinedMessages = [...currentSession, ...pastSessionMessages]; const deduplicatedMessages: string[] = []; if (combinedMessages.length > 0) { deduplicatedMessages.push(combinedMessages[0]); for (let i = 1; i < combinedMessages.length; i++) { if (combinedMessages[i] !== combinedMessages[i - 1]) { deduplicatedMessages.push(combinedMessages[i]); } } } return deduplicatedMessages.reverse(); }, [currentSessionMessages, pastSessionMessages]);
…via effect Address gemini-code-assist review: synchronizing inputHistory state in a useEffect is the derived-state anti-pattern (extra render pass, stale intermediate state, terminal flicker under Ink). Extract the deduplication into a pure computeInputHistory() helper and derive inputHistory with useMemo during render.
|
Good catch — agreed. Refactored in 6b3e357: |
TLDR
useInputHistoryStore.addInput()scheduledsetPastSessionMessages()and the side-effectingrecalculateHistory()(which itself callssetInputHistory()) inside thesetCurrentSessionMessages()updater function. React state updaters must be pure — they may be double-invoked under StrictMode and re-invoked under batching, so the nested work ran multiple times per submit.What changed
recalculateHistory()now runs in auseEffectkeyed to the committedcurrentSessionMessages/pastSessionMessagesvalues, so it executes exactly once per committed change (as suggested in bug(cli): impure state updater in useInputHistoryStore schedules nested setState inside another setState updater #29046).addInput()is reduced to a single pure functional update — no nestedsetStatecalls, no side effects inside updaters.initializeFromLogger()no longer callsrecalculateHistory()directly; the effect recomputes when the past-session messages commit._currentSessionMessages/_pastSessionMessagesbindings since they are now read by the effect.Behavior is unchanged (the old pattern was idempotent); this removes the timing-dependent, unsupported pattern so it cannot break under concurrent features.
Testing
addInput()calls in a singleact()and asserts they compose in submission order.useInputHistoryStore.test.tspass; lint, prettier and typecheck are clean.Checklist