refactor(web): unify turn termination through a single coordinator (LUM-2019)#32469
Conversation
…UM-2019)
Twelve production call sites were each independently responsible for two
store transitions: one terminal turn-store action + one
`removeProcessingConversationId` cleanup on the conversation-store. The
canonical bug was "forget the second call" — exactly what caused the
loading-state regression that needed a separate hot fix.
Replace with a single `endTurn({ conversationId, reason })` action:
- SSE handlers: `handleAssistantActivityState(idle)`,
`handleMessageComplete`, `handleGenerationCancelled`
- Error handlers: `handleStreamError`, `handleConversationErrorEvent`
- Session-error path in `useEventStream`
- Send-message error path, send-finally settle, `handleStopGenerating`
- Secret-prompt cancel, contact-prompt cancel
- Silent-stall rescue in `useMessageReconciliation`
Multi-key cleanup in the send-message catch-all (where retargeted sends
may have left processing markers on multiple conversation ids) stays
inline — `endTurn` is a one-conversation abstraction by design.
Coordinator lives at `src/stores/turn-coordinator.ts`. Required moving
turn-store to a top-level home: `domains/messaging/` was only a
misnamed singleton folder containing turn-store + selectors, so it's
been deleted entirely and the three files are now at `src/stores/`.
Cross-domain allowlist shrunk by 12 files / 18 imports (45/61 → 33/43).
StreamHandlerContext drops `clearProcessingKey` (now unused) and gains
`endTurn`.
Docs:
- STATE_MANAGEMENT.md — updated turn-store location, added a new section
on terminal-cleanup going through `endTurn`.
- CONVENTIONS.md — example tree updated to show turn-store and
turn-coordinator under stores/.
No behavioural change beyond making previously-unpaired terminal
transitions (sites #2, #6, #7 in the audit) defensively clear the
processing key alongside the turn-store action — a strict improvement.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0cf33defc2
ℹ️ 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".
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
… review) The first cut of endTurn cleared `processingConversationIds` unconditionally for every reason. That broke for the `rescued` path: when a stale `.finally()` from a previous send's poll-reconcile fires after the user has already started a new turn in the same conversation, `onPollReconciled(staleTurnId)` correctly no-ops on the turn-store side — but the unconditional processing-key clear would still fire and hide the new in-flight turn's Stop button + sidebar dot. Fix: mirror `onPollReconciled`'s preconditions explicitly in the coordinator. If the rescue is stale (turn already idle, or `rescuedTurnId` doesn't match `activeTurnId`), short-circuit both stores together. The four definitive terminal reasons (complete / cancelled / error / session_error) keep the existing unconditional behavior — they represent terminal events whose meaning is "this turn is over." Two new regression tests cover the rescue-skip scenarios. Also: `handleConversationErrorEvent` now prefers `event.conversationId` over `streamContextRef.current?.conversationId`, matching the same fallback chain used by `handleMessageComplete` and `handleGenerationCancelled`. Race-resilient when a stream teardown clears the ref before the error event fires.
|
@codex review |
|
Codex Review: Didn't find any major issues. 🚀 ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
![vex-assistant-bot[bot]](https://avatars.githubusercontent.com/in/3399303?s=60&v=4){kind=small}
There was a problem hiding this comment.
✦ APPROVE — reviewed at 643f83ee
Value: Eliminates a structural coordination bug that had 12 active instances. endTurn() is a single call where before there were two independent store mutations — and the "forget the second call" failure mode is now architecturally prevented, not just patched.
Full analysis
The coordinator — turn-coordinator.ts ✅
The core logic is sound. endTurn() reads the turn store first, applies the correct terminal action, then conditionally clears the processing key. The rescued pre-check is the critical design decision:
if (args.reason === "rescued") {
const isStaleRescue =
!isSending(turn) ||
(args.rescuedTurnId != null && turn.activeTurnId !== args.rescuedTurnId);
if (isStaleRescue) return;
turn.onPollReconciled(args.rescuedTurnId ?? undefined);
}This is correct. onPollReconciled already self-guards on turnId mismatch and already-idle state — but without the pre-check, the processing-key clear that follows it would fire regardless. The Codex/Devin finding on 0cf33def (both bots flagged the same bug: stale .finally() clearing an in-flight new turn's processing key) was legitimate, and it's fixed at HEAD 643f83ee by this exact pre-check.
Verified in tests: The "rescued with mismatched turnId leaves processing key untouched" test and "rescued when already-idle is a full no-op" test both cover this exact scenario. ✅
Devin's handleConversationErrorEvent finding — addressed at HEAD ✅
Devin flagged that the original refactor used ctx.streamContextRef.current?.conversationId instead of event.conversationId for the error handler. Fixed at HEAD:
ctx.endTurn({
conversationId: event.conversationId ?? ctx.streamContextRef.current?.conversationId,
reason: "error",
});The event.conversationId ?? ctx.streamContextRef.current?.conversationId fallback chain is the same pattern used by handleMessageComplete and handleGenerationCancelled. Consistent. ✅
11 of 12 call sites converted ✅
Every SSE terminal handler, error handler, session-error path, interaction-cancel path, reconciliation rescue, stop-generation — all go through endTurn. The one intentionally left inline (use-send-message catch-all for multi-key cleanup on retargeted sends) has a clear comment explaining why: endTurn is a one-conversation abstraction by design, and the catch-all needs to clean up both the original active key and the resolved key. The comment is accurate and the remaining useTurnStore.getState().onStreamError() + removeProcessingConversationId loop is the only correct shape here.
Bug-fix bonus — 3 previously-missing processing-key clears ✅
Sites #2 (use-send-message post-failure), #6 (secret cancel), and #7 (contact cancel) were calling only the turn-store action with no removeProcessingConversationId. These were latent LUM-1952 clones that hadn't been reported yet. endTurn fixed them as a side-effect of the refactor. The PR body's description of this as "strict improvement, no regression risk" is accurate.
rescued reason — .finally() in use-send-message ✅
The .finally() block at line ~485 is the right shape:
.finally(() => {
if (!isCurrentSendScope(effectiveConversationId)) return;
endTurn({ conversationId: effectiveConversationId, reason: "rescued", rescuedTurnId: turnId });
});The isCurrentSendScope guard is the outer scope check; endTurn's rescued pre-check is the inner turn-id guard. Defense in depth is correct here — stale sends from navigated-away conversations are caught by the outer guard, stale turns replaced by a new send in the same conversation are caught by the inner guard.
Store move (domains/messaging/ → stores/) ✅
turn-store.ts was a singleton with no per-conversation state — it had no business in domains/messaging/ alongside domain-scoped hooks. The move to stores/ is correct per CONVENTIONS.md (stores = module-level singletons). 18-file import codemod, cross-domain allowlist drops 18 imports (12 fewer files touching the old path). Structural improvement.
StreamHandlerContext — clearProcessingKey → endTurn ✅
The interface change correctly drops the clearProcessingKey: (convKey: string) => void field and adds endTurn: (args: EndTurnArgs) => void. The docstring on the new field explains the atomic-two-store contract. use-stream-event-handler.ts wires endTurn directly from the import — no risk of the context object diverging from the module implementation.
Tests ✅
9 unit tests in turn-coordinator.test.ts cover: each terminal reason, the mismatched-rescue guard (active turn not touched), the already-idle rescue guard (processing key not cleared), multi-key isolation (only the supplied conversationId is removed), and null conversationId (turn transitions, processing-key clear skipped). The test for the Codex/Devin-reported bug is explicit and named clearly. Good.
Minor non-blocking: stray blank lines
error-handlers.ts line 9 and message-handlers.ts line 18 each have a blank line added after the import block that produces a double-blank-line. Not functional, linter will catch if configured strictly. No action needed.
Merge gate
- ✅ Vex APPROVED (this review)
- ⏳ Devin + Codex reviewed stale
0cf33def— triggering fresh HEAD reviews below - ✅
bunx tsc --noEmitclean,bun run lintclean, cross-domain allowlist regenerated - ✅ All touched test suites passing per PR body
|
@devin review this PR |
|
@codex review |
|
Codex Review: Didn't find any major issues. 👍 ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
2 participants
Summary
Resolves LUM-2019. Eliminates the 5+ site "forget the second call" coordination smell that LUM-1952 surfaced — and an audit during this PR found it was actually 12 sites, not 5.
The problem
A turn's terminal state is split across two stores:
turn-store.phase— the active turn's lifecycle (one per tab)conversation-store.processingConversationIds— sidebar's view of which conversations are processing (multi-conversation, includes background convos from Slack/Telegram/etc.)Every terminal-event path had to call both stores independently. LUM-1952 happened because the silent-stall rescue path called only the turn-store. Every future terminal-event path was a re-introduction risk.
The fix
Single
endTurn({ conversationId, reason })action atsrc/stores/turn-coordinator.tsthat updates both stores atomically. Every terminal-event path now calls one function.Reasons map 1:1 onto turn-store's terminal actions:
"complete","cancelled","error","session_error","rescued"(the last carries arescuedTurnIdto scopeonPollReconciledto a specific turn).What changed
Coordinator
src/stores/turn-coordinator.ts—endTurn()action with full docstring explaining the split-store rationalesrc/stores/turn-coordinator.test.ts— 9 focused unit tests covering each reason, multi-key isolation, mismatched-rescue-turn guard, and the null-conversationId fallbackCall sites converted (11 of 12)
handleAssistantActivityState(idle),handleMessageComplete,handleGenerationCancelled— SSE terminal handlershandleStreamError,handleConversationErrorEvent— error handlersuse-event-stream.tssession-error pathuse-send-message.tspost-failure, send-finally settle,handleStopGeneratinguse-interaction-actions.tssecret-prompt cancel, contact-prompt canceluse-message-reconciliation.tssilent-stall rescueLeft inline with comment
use-send-message.tscatch-all (multi-key cleanup when retargeted sends touched multiple conversations) —endTurnis a one-conversation abstraction by design; refactoring this site would require a separate multi-key shapeStructural
domains/messaging/{turn-store,turn-selectors,turn-store.test}.ts→src/stores/domains/messaging/(was only a misnamed singleton folder containing turn-store + selectors — no message-store, no message-handlers, nothing else)@/domains/messaging/→@/stores/clearProcessingKey(now unused), gainsendTurnDocs
STATE_MANAGEMENT.md— updated turn-store location, added a new section on terminal cleanup throughendTurnCONVENTIONS.md— example tree shows turn-store + turn-coordinator understores/Cross-domain allowlist
45 files / 61 imports → 33 files / 43 imports (12 fewer files, 18 fewer imports). The "messaging" target is gone entirely from the allowlist.
Behavioural note
The conversions of sites #2 (
use-send-messagepost-failure), #6 (secret cancel), and #7 (contact cancel) add a previously-missingremoveProcessingConversationIdalongside the turn-store transition. These were exactly the same shape of bug as LUM-1952 — terminal turn-store action with no processing-key cleanup — they just hadn't been reported yet. Strict improvement, no regression risk.Test plan
bunx tsc --noEmitcleanbun run lintcleanbun run audit:cross-domainregenerates allowlist with 18 fewer importsturn-coordinator.test.ts(9 tests) covers each reason + edge casesLinear: LUM-2019
Generated by Claude Code