fix(web): isSending honors activeTurnId so thinking indicator survives stale terminal events#32057
Conversation
…s stale terminal events
When a stale 'message_complete' (or 'assistant_activity_state{phase:idle}')
from a previous turn lands during the await window inside 'sendMessage',
'completeTurn' resets phase to idle and clears activeTurnId. The
subsequent 'acceptSend(turnId)' restores activeTurnId but does not touch
phase or lastTerminalReason, leaving the store in an illegal but
observable shape:
phase: 'idle'
activeTurnId: 'turn-NEW'
lastTerminalReason: 'complete'
The old phase-only 'isSending' predicate returned false here, so
'shouldShowThinkingIndicator' failed on its first AND-clause and the
indicator stayed hidden until the first assistant delta arrived.
Treat 'activeTurnId !== null' as authoritative for in-flight state and
keep the phase checks so the post-complete 'queued waiting' state
(phase='queued', activeTurnId=null, pendingQueuedCount>0) still reports
as sending.
Adds regression tests for the illegal shape and the terminal-idle
counterpart, and updates the misaligned 'does NOT call onPollReconciled
when turn is already idle' reconciliation test to match its intent
(genuine terminal idle = activeTurnId null). The illegal-state stuckness
recovery is already exercised by the existing 'stuck sending' rescue
tests above it.
| /** True when we are waiting for the first assistant text delta. */ | ||
| export function isThinking(state: TurnState): boolean { | ||
| return state.phase === "thinking"; | ||
| } |
There was a problem hiding this comment.
Check this out. Based on the comment, we expected the state.phase to be thinking, instead of adding this active turn id null check, which I think would always render the thinking indicator during the lifetime of the assistant response
There was a problem hiding this comment.
Good catch — you're right. The fix belongs in the state machine, not the predicate.
Reverted isSending to phase-only. Moved the recovery into acceptSend (and the matching USER_SEND_ACCEPTED reducer case): when the POST resolves and we're entering the "waiting for first delta" window, restore phase: "thinking" + lastTerminalReason: null if a stale terminal landed between requestSend and acceptSend. Non-terminal phases (e.g. awaiting_user_input from a surface that's still active) are left alone.
This matches the isThinking doc comment exactly — phase IS thinking through that window, so all downstream selectors keep working unchanged.
Pushed as 24c7b2f.
Per Vargas's review: 'we expected state.phase to be thinking' (per isThinking doc comment 'True when we are waiting for the first assistant text delta'). Patching the predicate was the wrong layer — the state machine itself should keep phase=thinking from acceptSend until the first delta. Revert isSending to phase-only. Make acceptSend / USER_SEND_ACCEPTED restore (phase=thinking, lastTerminalReason=null) when phase was clobbered to idle/errored by a stale terminal event during the POST await. Non-terminal phases (e.g. awaiting_user_input from a surface) are left alone.
When a stale
message_complete(orassistant_activity_state{phase:idle}) from a previous turn lands during theawait sendMessageViaStream(...)window insidesendMessage,completeTurn()resetsphase: idleand clearsactiveTurnId. The subsequentacceptSend(turnId)restoresactiveTurnIdbut does not touch phase orlastTerminalReason, leaving the turn store in an illegal but observable shape:The old phase-only
isSendingpredicate returnedfalsehere, soshouldShowThinkingIndicatorfailed on its first AND-clause and the thinking indicator stayed hidden until the first assistant delta arrived._vellumDebug.chat.thinkingIndicator()reportedfailingConditions: ["notSendingAndNotRestoredProcessing"]in exactly this shape in the wild.Fix
Treat
activeTurnId !== nullas authoritative for in-flight state. Keep the existing phase checks so the post-complete "queued waiting" state (phase: "queued",activeTurnId: null,pendingQueuedCount > 0) still reports as sending.Once the first assistant delta arrives,
onTextDeltatransitionsphase → "streaming". The indicator continues to render through the rest of the turn from the streaming/thinking phases as before.Why not fix the stale-terminal race itself?
The deeper fix is to gate
handleMessageComplete/handleAssistantActivityStateon a turn-id match (the daemon already carriesrequestIdon activity-state events; message-complete would need to grow one). That requires daemon coordination and is a separate concern. This PR fixes the symptom at the predicate that all consumers read from.Tests
turn-store.test.ts— three new cases pinning the predicate to the actual invariant: returns true for the illegal shape, false for terminal idle, true for queued-waiting.use-message-reconciliation.test.tsx— updateddoes NOT call onPollReconciled when turn is already idleto match its name (genuine terminal idle hasactiveTurnId: null). The illegal-state recovery is already covered by the dedicated "stuck sending" rescue tests above it.