fix(cli): stop scheduling state updates from inside a state updater - #29211
fix(cli): stop scheduling state updates from inside a state updater#29211linhongyu510 wants to merge 2 commits into
Conversation
useInputHistoryStore.addInput() called setPastSessionMessages() from inside the setCurrentSessionMessages() updater, and ran recalculateHistory() — itself a setState — from that nested updater. The inner updater returned prevPast unchanged purely to piggyback the side effect. React updaters must be pure and may run more than once per update (StrictMode double-invoke, replays under batching). Instrumenting the current code shows one addInput invoking the outer updater twice and the inner updater three times, so recalculateHistory ran three times for a single submit. Derive the next session list before touching state and issue two plain updates, keeping the two lists mirrored in refs so addInput can read them without an updater. The resulting history was already idempotent, so the visible fix is the redundant render pass: one submit under StrictMode goes from 6 renders to 4. The hook's public shape is unchanged, so AppContainer is unaffected.
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 addresses a latent contract violation in 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
|
|
📊 PR Size: size/M
|
There was a problem hiding this comment.
Code Review
This pull request refactors the useInputHistoryStore hook to move state updates outside of updater functions, preventing redundant renders and ensuring compatibility with React's StrictMode double-invocations and batching. It introduces useRef mirrors for past and current message lists to derive state updates upfront, and adds comprehensive tests to verify behavior under StrictMode. The reviewer pointed out that calling setCurrentSessionMessages is redundant since currentSessionMessages is a dead state that is never read or returned, and suggested removing it to avoid unnecessary state dispatch overhead.
| const newCurrentSession = [...currentRef.current, trimmedInput]; | ||
| currentRef.current = newCurrentSession; | ||
|
|
||
| return newCurrentSession; | ||
| }); | ||
| setCurrentSessionMessages(newCurrentSession); | ||
| recalculateHistory( | ||
| newCurrentSession.slice().reverse(), // Convert to newest first | ||
| pastRef.current, | ||
| ); |
There was a problem hiding this comment.
_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,
);Review feedback: _currentSessionMessages was never read or returned, so dispatching to it on every submit was pure overhead. Remove the state pair and keep currentRef as the single source for the running session list. Renders per submit are unchanged at 2 - React batches the two dispatches into one pass - so this removes dead state rather than a render. Adds a test that the per-submit render cost does not grow as submits accumulate, so a reintroduced redundant write would show up.
|
Thanks — verified and applied in 19be911. Confirmed the claim first: One correction on the rationale, so the PR description doesn't overstate the win: renders per submit are unchanged at 2. I measured before and after with a render-counting probe under Added Re-verified after the change:
|
Summary
useInputHistoryStore.addInput()scheduled state updates from inside a stateupdater: it called
setPastSessionMessages()within thesetCurrentSessionMessages()updater, and ranrecalculateHistory()— itself asetState— from that nested updater. React requires updaters to be pure, andmay invoke them more than once per update.
The resulting history was already idempotent, so this fixes a latent contract
violation plus one measurable cost: a redundant render pass on every submit.
Details
The pattern in
packages/cli/src/ui/hooks/useInputHistoryStore.tswas:The inner updater returned
prevPastunchanged purely to piggyback a sideeffect and read
prevPast.Instrumenting the current code (counters inside each updater, rendered under
StrictMode) shows a singleaddInput('hello'):setCurrentSessionMessages)setPastSessionMessages)recalculateHistoryThis change derives the next session list before touching state, then issues
two plain updates. Because
addInputcan no longer readprevCurrent/prevPastfrom an updater, the two lists are mirrored in refs kept in sync at every write
site (
addInputand both branches ofinitializeFromLogger).Scope notes:
inputHistory,addInput,initializeFromLogger)is unchanged, so
AppContainerneeds no changes._currentSessionMessageswas already unread before this change (hence theunderscore prefix). Per review it is now removed entirely, since
addInputwas its only writer;
currentRefis the single source for the runningsession list.
_pastSessionMessagesstill has two writers ininitializeFromLogger, so it stays.Related Issues
Fixes #29046
How to Validate
Expected:
19 passed. The file had 14 tests; this PR adds 5.To confirm the new render-count test is load-bearing, restore only the old
addInputbody (keeping the new tests) and re-run:Measured renders for one submit under
StrictMode: 6 before → 4 after.Note on the follow-up commit: removing the dead
setCurrentSessionMessagesdoes not reduce renders further (still 2 per submit — React batches the two
dispatches into one pass). It removes dead state, not a render pass.
Also run, unchanged by this PR:
Edge cases covered by the added tests: batched submits in one
act(),past-session messages still visible after batched submits, consecutive-duplicate
dedup across batched submits, and per-submit render cost not growing as submits
accumulate (guards a redundant state write from creeping back).
Pre-Merge Checklist