From 793fd0d59b511f1dfeb44628c97ab0d43903b33f Mon Sep 17 00:00:00 2001 From: Vellum Apollo Bot Date: Tue, 26 May 2026 11:01:10 +0000 Subject: [PATCH 1/2] fix(web): isSending honors activeTurnId so thinking indicator survives 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. --- .../hooks/use-message-reconciliation.test.tsx | 9 +++- .../src/domains/messaging/turn-store.test.ts | 48 +++++++++++++++++++ apps/web/src/domains/messaging/turn-store.ts | 16 ++++++- 3 files changed, 70 insertions(+), 3 deletions(-) diff --git a/apps/web/src/domains/chat/hooks/use-message-reconciliation.test.tsx b/apps/web/src/domains/chat/hooks/use-message-reconciliation.test.tsx index d848a24402d..cb59ce4a4f7 100644 --- a/apps/web/src/domains/chat/hooks/use-message-reconciliation.test.tsx +++ b/apps/web/src/domains/chat/hooks/use-message-reconciliation.test.tsx @@ -393,12 +393,17 @@ describe("reconcileActiveConversation", () => { { id: "m1", role: "user", content: "Hello" }, { id: "m2", role: "assistant", content: "Response" }, ]; + // Genuinely-terminal idle: phase=idle AND activeTurnId=null. The rescue + // path must stay quiet here. (A `phase: "idle"` state with a non-null + // activeTurnId is the illegal post-stale-terminal shape that + // `isSending` now treats as in-flight — covered by the dedicated + // "stuck sending" tests above.) const idleTurnState: TurnState = { phase: "idle", pendingQueuedCount: 0, activeToolCallCount: 0, - activeTurnId: "turn-42", - lastTerminalReason: null, + activeTurnId: null, + lastTerminalReason: "complete", statusText: null, liveWebActivity: {}, autoRoutedProfileLabel: null, diff --git a/apps/web/src/domains/messaging/turn-store.test.ts b/apps/web/src/domains/messaging/turn-store.test.ts index 56d9d64bc12..d8196589922 100644 --- a/apps/web/src/domains/messaging/turn-store.test.ts +++ b/apps/web/src/domains/messaging/turn-store.test.ts @@ -62,6 +62,54 @@ describe("INITIAL_TURN_STATE", () => { }); }); +// --------------------------------------------------------------------------- +// isSending — illegal-state robustness +// --------------------------------------------------------------------------- + +describe("isSending", () => { + test("returns true whenever activeTurnId is set, even if phase is idle", () => { + // Repro of the state observed in the wild: + // `requestSend(turn-NEW)` runs (phase=thinking, activeTurnId=turn-NEW) + // → stale `completeTurn` from the previous turn fires during the + // `await sendMessageViaStream(...)` window + // (phase=idle, activeTurnId=null, lastTerminalReason=complete) + // → `acceptSend(turn-NEW)` re-sets activeTurnId + // (phase=idle, activeTurnId=turn-NEW, lastTerminalReason=complete) + // Before the fix, isSending returned false here and the thinking + // indicator stayed hidden until the first assistant delta arrived. + const illegalAfterStaleTerminal: TurnState = { + ...INITIAL_TURN_STATE, + phase: "idle", + activeTurnId: "turn-1", + lastTerminalReason: "complete", + }; + expect(isSending(illegalAfterStaleTerminal)).toBe(true); + }); + + test("returns false for terminal idle with no active turn", () => { + const idle: TurnState = { + ...INITIAL_TURN_STATE, + phase: "idle", + activeTurnId: null, + lastTerminalReason: "complete", + }; + expect(isSending(idle)).toBe(false); + }); + + test("returns true for queued waiting state (post-complete, pendingQueuedCount>0)", () => { + // After `completeTurn` with queued messages remaining: phase=queued + // and activeTurnId=null until the next turn starts. + const queuedWaiting: TurnState = { + ...INITIAL_TURN_STATE, + phase: "queued", + activeTurnId: null, + pendingQueuedCount: 1, + lastTerminalReason: "complete", + }; + expect(isSending(queuedWaiting)).toBe(true); + }); +}); + // --------------------------------------------------------------------------- // USER_SEND_REQUESTED // --------------------------------------------------------------------------- diff --git a/apps/web/src/domains/messaging/turn-store.ts b/apps/web/src/domains/messaging/turn-store.ts index e26b36b58d2..233c631cf44 100644 --- a/apps/web/src/domains/messaging/turn-store.ts +++ b/apps/web/src/domains/messaging/turn-store.ts @@ -84,9 +84,23 @@ export const INITIAL_TURN_STATE: TurnState = { // Derived helpers // --------------------------------------------------------------------------- -/** True when the turn is actively processing (not idle/errored). */ +/** True when a turn is in flight. + * + * `activeTurnId` is the authoritative signal: it is set synchronously by + * `requestSend` / `acceptSend` and cleared by every terminal handler. The + * phase checks remain so that the post-complete "queued waiting" state + * (phase=`queued`, activeTurnId=`null`, pendingQueuedCount>0) still + * reports as sending. + * + * Including `activeTurnId !== null` also makes the predicate robust to a + * stale terminal event slipping in between `requestSend` and `acceptSend` + * (which would otherwise leave the store in an illegal phase=`idle` + + * activeTurnId=non-null shape until the first assistant delta arrived, + * suppressing the thinking indicator). + */ export function isSending(state: TurnState): boolean { return ( + state.activeTurnId !== null || state.phase === "queued" || state.phase === "thinking" || state.phase === "streaming" || From 24c7b2f4654c6f23ded5b06c560dc5da3402646f Mon Sep 17 00:00:00 2001 From: Vellum Apollo Bot Date: Tue, 26 May 2026 12:18:37 +0000 Subject: [PATCH 2/2] Address review: fix in acceptSend instead of isSending predicate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../hooks/use-message-reconciliation.test.tsx | 9 +- .../src/domains/messaging/turn-store.test.ts | 109 ++++++++++-------- apps/web/src/domains/messaging/turn-store.ts | 48 +++++--- 3 files changed, 92 insertions(+), 74 deletions(-) diff --git a/apps/web/src/domains/chat/hooks/use-message-reconciliation.test.tsx b/apps/web/src/domains/chat/hooks/use-message-reconciliation.test.tsx index cb59ce4a4f7..d848a24402d 100644 --- a/apps/web/src/domains/chat/hooks/use-message-reconciliation.test.tsx +++ b/apps/web/src/domains/chat/hooks/use-message-reconciliation.test.tsx @@ -393,17 +393,12 @@ describe("reconcileActiveConversation", () => { { id: "m1", role: "user", content: "Hello" }, { id: "m2", role: "assistant", content: "Response" }, ]; - // Genuinely-terminal idle: phase=idle AND activeTurnId=null. The rescue - // path must stay quiet here. (A `phase: "idle"` state with a non-null - // activeTurnId is the illegal post-stale-terminal shape that - // `isSending` now treats as in-flight — covered by the dedicated - // "stuck sending" tests above.) const idleTurnState: TurnState = { phase: "idle", pendingQueuedCount: 0, activeToolCallCount: 0, - activeTurnId: null, - lastTerminalReason: "complete", + activeTurnId: "turn-42", + lastTerminalReason: null, statusText: null, liveWebActivity: {}, autoRoutedProfileLabel: null, diff --git a/apps/web/src/domains/messaging/turn-store.test.ts b/apps/web/src/domains/messaging/turn-store.test.ts index d8196589922..a4dcd85f2ba 100644 --- a/apps/web/src/domains/messaging/turn-store.test.ts +++ b/apps/web/src/domains/messaging/turn-store.test.ts @@ -62,54 +62,6 @@ describe("INITIAL_TURN_STATE", () => { }); }); -// --------------------------------------------------------------------------- -// isSending — illegal-state robustness -// --------------------------------------------------------------------------- - -describe("isSending", () => { - test("returns true whenever activeTurnId is set, even if phase is idle", () => { - // Repro of the state observed in the wild: - // `requestSend(turn-NEW)` runs (phase=thinking, activeTurnId=turn-NEW) - // → stale `completeTurn` from the previous turn fires during the - // `await sendMessageViaStream(...)` window - // (phase=idle, activeTurnId=null, lastTerminalReason=complete) - // → `acceptSend(turn-NEW)` re-sets activeTurnId - // (phase=idle, activeTurnId=turn-NEW, lastTerminalReason=complete) - // Before the fix, isSending returned false here and the thinking - // indicator stayed hidden until the first assistant delta arrived. - const illegalAfterStaleTerminal: TurnState = { - ...INITIAL_TURN_STATE, - phase: "idle", - activeTurnId: "turn-1", - lastTerminalReason: "complete", - }; - expect(isSending(illegalAfterStaleTerminal)).toBe(true); - }); - - test("returns false for terminal idle with no active turn", () => { - const idle: TurnState = { - ...INITIAL_TURN_STATE, - phase: "idle", - activeTurnId: null, - lastTerminalReason: "complete", - }; - expect(isSending(idle)).toBe(false); - }); - - test("returns true for queued waiting state (post-complete, pendingQueuedCount>0)", () => { - // After `completeTurn` with queued messages remaining: phase=queued - // and activeTurnId=null until the next turn starts. - const queuedWaiting: TurnState = { - ...INITIAL_TURN_STATE, - phase: "queued", - activeTurnId: null, - pendingQueuedCount: 1, - lastTerminalReason: "complete", - }; - expect(isSending(queuedWaiting)).toBe(true); - }); -}); - // --------------------------------------------------------------------------- // USER_SEND_REQUESTED // --------------------------------------------------------------------------- @@ -156,7 +108,7 @@ describe("USER_SEND_REQUESTED", () => { // --------------------------------------------------------------------------- describe("USER_SEND_ACCEPTED", () => { - test("sets turnId without changing phase", () => { + test("sets turnId without changing phase when already thinking", () => { const thinking = turnReducer(INITIAL_TURN_STATE, { type: "USER_SEND_REQUESTED", turnId: "temp", @@ -168,6 +120,65 @@ describe("USER_SEND_ACCEPTED", () => { expect(state.phase).toBe("thinking"); expect(state.activeTurnId).toBe("real-turn-id"); }); + + test("restores thinking phase when a stale terminal landed between request and accept", () => { + // Wild repro: + // USER_SEND_REQUESTED(turn-NEW) + // → phase=thinking, activeTurnId=turn-NEW, lastTerminalReason=null + // ← stale MESSAGE_COMPLETE from a previous turn arrives during + // the POST await + // → phase=idle, activeTurnId=null, lastTerminalReason=complete + // USER_SEND_ACCEPTED(turn-NEW) + // → must restore the in-flight shape (phase=thinking, + // lastTerminalReason=null) so the thinking indicator stays + // visible until the first assistant delta arrives. + const clobbered: TurnState = { + ...INITIAL_TURN_STATE, + phase: "idle", + activeTurnId: null, + lastTerminalReason: "complete", + }; + const state = turnReducer(clobbered, { + type: "USER_SEND_ACCEPTED", + turnId: "turn-NEW", + }); + expect(state.phase).toBe("thinking"); + expect(state.activeTurnId).toBe("turn-NEW"); + expect(state.lastTerminalReason).toBeNull(); + expect(isSending(state)).toBe(true); + expect(isThinking(state)).toBe(true); + }); + + test("restores thinking phase from errored as well", () => { + const erroredState: TurnState = { + ...INITIAL_TURN_STATE, + phase: "errored", + activeTurnId: null, + lastTerminalReason: "error", + }; + const state = turnReducer(erroredState, { + type: "USER_SEND_ACCEPTED", + turnId: "turn-NEW", + }); + expect(state.phase).toBe("thinking"); + expect(state.lastTerminalReason).toBeNull(); + }); + + test("leaves non-terminal phases alone (e.g. awaiting_user_input from a surface)", () => { + // If the user already replied to a prompt that put us in awaiting_user_input + // and the next send is being accepted, do not regress the phase to "thinking". + const awaiting: TurnState = { + ...INITIAL_TURN_STATE, + phase: "awaiting_user_input", + activeTurnId: "turn-OLD", + }; + const state = turnReducer(awaiting, { + type: "USER_SEND_ACCEPTED", + turnId: "turn-NEW", + }); + expect(state.phase).toBe("awaiting_user_input"); + expect(state.activeTurnId).toBe("turn-NEW"); + }); }); // --------------------------------------------------------------------------- diff --git a/apps/web/src/domains/messaging/turn-store.ts b/apps/web/src/domains/messaging/turn-store.ts index 233c631cf44..eb9821f6e80 100644 --- a/apps/web/src/domains/messaging/turn-store.ts +++ b/apps/web/src/domains/messaging/turn-store.ts @@ -84,23 +84,9 @@ export const INITIAL_TURN_STATE: TurnState = { // Derived helpers // --------------------------------------------------------------------------- -/** True when a turn is in flight. - * - * `activeTurnId` is the authoritative signal: it is set synchronously by - * `requestSend` / `acceptSend` and cleared by every terminal handler. The - * phase checks remain so that the post-complete "queued waiting" state - * (phase=`queued`, activeTurnId=`null`, pendingQueuedCount>0) still - * reports as sending. - * - * Including `activeTurnId !== null` also makes the predicate robust to a - * stale terminal event slipping in between `requestSend` and `acceptSend` - * (which would otherwise leave the store in an illegal phase=`idle` + - * activeTurnId=non-null shape until the first assistant delta arrived, - * suppressing the thinking indicator). - */ +/** True when the turn is actively processing (not idle/errored). */ export function isSending(state: TurnState): boolean { return ( - state.activeTurnId !== null || state.phase === "queued" || state.phase === "thinking" || state.phase === "streaming" || @@ -328,7 +314,23 @@ const useTurnStoreBase = create()((set, get) => ({ statusText: null, })), - acceptSend: (turnId) => set({ activeTurnId: turnId }), + acceptSend: (turnId) => + set((s) => { + // The send POST has resolved — we are now waiting for the first + // assistant text delta. Phase must be "thinking" for that window + // (see `isThinking` doc comment). If a stale terminal event from + // a previous turn slipped in between `requestSend` and here during + // the POST await, phase was clobbered to "idle"/"errored" with + // `lastTerminalReason` populated. Restore the in-flight shape so + // the thinking indicator stays visible until the first delta. + const phaseIsTerminal = s.phase === "idle" || s.phase === "errored"; + return { + activeTurnId: turnId, + ...(phaseIsTerminal + ? { phase: "thinking" as const, lastTerminalReason: null } + : null), + }; + }), // ----- Streaming ----- @@ -617,8 +619,18 @@ export function turnReducer(state: TurnState, event: DomainEvent): TurnState { statusText: null, }; - case "USER_SEND_ACCEPTED": - return { ...state, activeTurnId: event.turnId }; + case "USER_SEND_ACCEPTED": { + // See `acceptSend` action for rationale. + const phaseIsTerminal = + state.phase === "idle" || state.phase === "errored"; + return { + ...state, + activeTurnId: event.turnId, + ...(phaseIsTerminal + ? { phase: "thinking" as const, lastTerminalReason: null } + : null), + }; + } case "ASSISTANT_TEXT_DELTA": if (state.phase === "idle" || state.phase === "errored") {