feat(chat): convert turn state from useReducer to Zustand store with createSelectors#31144
Conversation
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ead273a17d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
||
| const [messages, setMessages] = useState<DisplayMessage[]>([]); | ||
| const [turnState, dispatchTurn] = useReducer(turnReducer, INITIAL_TURN_STATE); | ||
| const dispatchTurn = useTurnStore((s) => s.dispatch); |
There was a problem hiding this comment.
Scope turn state to a chat page instance
Switching from useReducer to the module-level useTurnStore makes turn state global for the entire app lifecycle, so it no longer resets when ChatPage unmounts/remounts. If a user navigates away mid-turn (or another ChatPage instance mounts), the next page can inherit stale phase/activeTurnId and show the wrong sending/stop state. The prior reducer-based state was instance-local and avoided this bleed-through.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — fixed in c5892ff. Added a useEffect in ChatPage that resets the store to INITIAL_TURN_STATE on mount, so navigating away mid-turn and returning starts with a clean slate (matching the prior useReducer instance-local behavior).
| const originalDispatch = useTurnStore.getState().dispatch; | ||
| useTurnStore.setState({ | ||
| dispatch: (event: DomainEvent) => { | ||
| dispatchedEvents.push(event); | ||
| originalDispatch(event); | ||
| }, |
There was a problem hiding this comment.
Restore store dispatch instead of nesting test wrappers
Each createHarness() call wraps useTurnStore.dispatch around the current dispatch implementation, but never restores the base action. Because the store is singleton state, wrappers stack across tests and every dispatch triggers multiple wrapper layers, inflating dispatchedEvents and making assertions order-dependent/flaky. Resetting dispatch to a known base implementation per test avoids cumulative side effects.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — fixed in c5892ff. Replaced the wrapping approach with a self-contained dispatch spy that sets state AND captures events without referencing any previous dispatch function. Each createHarness() call now installs a fresh dispatch, eliminating the stacking issue.
Testing ResultsRan unit tests on the PR branch to verify the Zustand store integration. Zustand Store Integration (26/26 pass)Tests exercise Pure Reducer Regression (114/114 pass)Confirms TypeScript CompilationZero new TS errors in any file touched by this PR. All pre-existing errors are from Not Tested
|
…rilling - Replace turnReducer-based dispatch with 25 named Zustand actions - Action naming: on* for SSE events, imperative for user/system actions - Rename reconcilePoll → onPollReconciled (event-reaction naming) - Remove turnState/dispatchTurn/turnStateRef prop drilling from ChatRouteContent, ChatComposer, and all hooks - Components read directly from useTurnStore(selector) - Stream handlers use ctx.turnActions.<action>() pattern - Keep turnReducer as exported pure function for test coverage - Add turn-store conventions to CONVENTIONS.md Closes LUM-1623 Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>
…ctions Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>
c5892ff to
029266c
Compare
There was a problem hiding this comment.
✦ APPROVE
Significant improvement. dispatchTurn + turnStateRef are threaded through at least 10 hooks and the entire route content component — eliminating that prop drilling while replacing dispatch({ type: "..." }) with named action calls is a net gain in both ergonomics and debuggability. getState() replacing MutableRefObject<TurnState> is the correct Zustand pattern for non-React reads: synchronous, never stale, no ref-sync overhead. The pure reducer is preserved in the file for test isolation, which is the right call.
One real issue: ChatRouteContent calls useTurnStore() bare
Line 513:
const turnState = useTurnStore();This subscribes to the entire store — all 6 state fields plus all 24 action functions. set() returns a new store object on every update, so ChatRouteContent re-renders on every onTextDelta(), onToolUseStart(), onActivityThinking() — 50ms cadence during streaming. That directly contradicts the PR's stated goal ("Selector-based subscriptions let each consumer re-render only when its slice changes — critical during streaming").
The store already has the right hook for this use case:
export function useTurnState(): TurnState {
return useTurnStore(useShallow((s) => ({
phase: s.phase,
pendingQueuedCount: s.pendingQueuedCount,
activeToolCallCount: s.activeToolCallCount,
activeTurnId: s.activeTurnId,
lastTerminalReason: s.lastTerminalReason,
statusText: s.statusText,
})));
}The fix in ChatRouteContent is one word:
// Before
const turnState = useTurnStore();
// After
const turnState = useTurnState();useTurnState() re-renders only when one of the 6 state fields changes — action function references never cause a re-render. Every other consumer in this PR does this correctly: hooks call useTurnStore.getState() for non-React reads; anything that needs to subscribe reactively should use useTurnState().
Secondary: useEffect vs useLayoutEffect for the mount reset
In ChatPage:
useEffect(() => {
useTurnStore.setState(INITIAL_TURN_STATE);
}, []);useEffect runs after the browser paints. If there's stale state (e.g. phase: "streaming") from a previous ChatPage session, the first paint frame renders with that stale state before the reset fires. useLayoutEffect runs after DOM commit but before paint, preventing the visible flash. Low-stakes in practice (state should usually be idle between navigations), but the lifecycle is subtler than it looks.
Everything else is solid
useTurnState()anduseTurnActions()both correctly useuseShallow— action references are stable souseTurnActions()won't re-render ✅- Named actions (
requestSend,cancelGeneration,onStreamError) map cleanly to call sites; theon*vs imperative naming convention documented in CONVENTIONS is clear ✅ isStale()guard on event handlers that shouldn't fire on an idle/errored store — defensive and correct ✅turnStateReffully retired — 5 fewer refs to maintain, state reads are synchronous and always fresh ✅- Reducer preserved for test isolation — pure function, no Zustand involved, confirms every state transition ✅
- Test helpers updated with full
TurnActionsmock usingsatisfies TurnActionsconstraint — catches shape drift ✅ - Codex P1 (store lifetime, stale state on remount) addressed via
useEffectreset ✅ - Codex P2 (test dispatch stacking) addressed ✅
Bot review note: Both Devin and Codex reviewed the pre-rebase commit ead273a17d, which no longer exists in the branch. The current HEAD is the squashed 369f9acf68 + merge-main 029266ce2d. Their P1/P2 findings are addressed in the current code, but they haven't seen the final squashed form — specifically the useTurnStore() bare call above.
|
All three findings from the bot review addressed in 1. Bare Additionally narrowed the selector function signatures throughout:
2. 3. Merge conflict markers in All 146 tests pass (114 reducer + 26 reconciliation + 6 chat-store). |
Resolved conflicts in 10 files where both branches removed their respective dispatch prop-threading: - Our branch removed dispatchInteraction (interaction Zustand store) - Main's #31144 removed dispatchTurn (turn Zustand store) Resolution: remove both dispatchers, use both Zustand stores directly. Also incorporates main's #31194 (organization store migration) and Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai> #31110 (OpenAPI spec sync).
Prompt / plan
Convert the turn state machine from
useReducer+ prop-drilling to a Zustand store with direct named actions andcreateSelectorsatomic selectors. Eliminates all prop-drilling ofturnState,dispatchTurn, andturnStateRefacross the component tree.Why needed:
useReducerrequired prop-drilling state and dispatch through 3+ layers (ChatPage→ChatRouteContent→ChatComposer→ hooks)MutableRefObject<TurnState>(turnStateRef) was a workaround for synchronous reads during ~50ms streaming cadence —useTurnStore.getState()replaces this pattern nativelyWhat changed:
domains/messaging/turn-store.ts— 25 direct named actions replacingdispatch({ type: ... })patternon*for SSE-event reactions (onTextDelta,onStreamError,onPollReconciled), imperative for user/system actions (requestSend,cancelGeneration,resetTurn)createSelectors—useTurnStore.use.phase(),useTurnStore.use.statusText(), etc.useShallow— every subscriber uses per-field atomic selectorsisSending/isThinkingaccept{ phase },shouldShowThinkingIndicatoraccepts{ phase, activeToolCallCount },isSendDisabled/shouldShowAssistantBubbletake onlyUIContext(unused state param removed)ChatPageusesuseLayoutEffectfor store reset (prevents stale state flash between navigations)dispatchTurnremoved fromChatStore— turn state is a separate domainSafety:
turnReduceris preserved as an export — all 114 existing reducer tests run against it unchangedCONVENTIONS.md updates:
createSelectors+.use.field()is the default;useShallowis explicitly wrong for stores owned in this repolib/inside domains conventionAGENTS.mdReferences:
Test plan
tsc --noEmit: cleanLink to Devin session: https://app.devin.ai/sessions/d20746d75d8f4d688baddacd8f87b0ac
Requested by: @ashleeradka