fix(canvas): clear sendInFlightRef on WS-push reply path - #2185
Merged
HongmingWang-Rabbit merged 4 commits intoApr 27, 2026
Conversation
Send button + Enter both silently no-op'd after the first agent reply
on runtimes that deliver via WebSocket (claude-code SDK does this per
the comment at ChatTab.tsx:294-298). The visible disabled-state checks
(sending, uploading, agentReachable) were all clean — the freeze came
from a third synchronous reentry guard the button can't see:
if (sendInFlightRef.current) return; // ChatTab.tsx:438
The ref was set true at the start of sendMessage() and only cleared in
.then() / .catch() of the HTTP fall-through and the upload-failure
branch. The WS-push handler in the pendingAgentMsgs effect cleared
`sending` and `sendingFromAPIRef` but left `sendInFlightRef` stuck
true. The HTTP .then() then early-returned at the dedup check (line
513) without touching the ref — only the .catch() early-return path
did. Net result: refresh fixed it because the ref reset on remount.
Two-line fix:
- WS handler: also clear sendInFlightRef when the push delivers the
reply (primary fix; no race window where the ref is stuck while
the user can already type)
- .then() early-return: mirror .catch()'s cleanup as defense in
depth, so neither delivery order leaks the ref
While here: A2AEdge.test.tsx fixture was typed `as never` to dodge
EdgeProps' discriminated-union complaint, which broke spreading at
the call sites with TS2698 ("Spread types may only be created from
object types"). Replaced with `as unknown as ComponentProps<typeof
A2AEdge>` — preserves the original "skip restating every optional
field" intent and keeps a spreadable type.
All 10 A2AEdge tests pass; tsc --noEmit is clean.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
HongmingWang-Rabbit
requested a review
from hongmingwang-moleculeai
as a code owner
April 27, 2026 18:12
…lper Self-review on PR #2185 surfaced a latent race the original fix exposed: the WS-clears-guards path now releases sendInFlightRef immediately, which means a user can fire msg #2 between WS-arrival and HTTP-arrival for msg #1. Without coordination, msg #1's late .then() sees sendingFromAPIRef=true (set by msg #2's send), enters the main body, and runs setSending(false) + appendMessageDeduped against msg #1's response body — clobbering msg #2's in-flight UI state. This race is realistic for claude-code SDK: the comment at line 294-298 already calls WS the "authoritative reply arrived" signal, and the user typically reads-then-types before the trailing HTTP completes. Without the original Send-button freeze "protecting" the race, it surfaces. Two changes: 1. Token-keyed callbacks. sendTokenRef bumps on every sendMessage entry; .then()/.catch() capture the token in closure and bail without touching any flags if a newer send has superseded them. The newer send owns the in-flight guards. 2. releaseSendGuards() helper. The three-clear-guards trio (setSending, sendingFromAPIRef, sendInFlightRef) now lives in one useCallback so the WS handler, .then() success, and .catch() success can't drift apart. A future contributor dropping one of the three would silently re-introduce either the post-WS Send freeze or the stale-callback clobber. Skipped a unit test for this regression — ChatTab has no __tests__ file and a mount test would need WS + zustand + api mocks. The fix is 4 logical lines (token capture + 2 guard checks) and the manual test covers it. Follow-up to add a focused mount test when ChatTab gets its first __tests__ file. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
HongmingWang-Rabbit
enabled auto-merge
April 27, 2026 18:50
Third-pass review caught a fourth WS path I missed. The original fix + the stale-callback follow-up patched 3 sites that release the in-flight guards (pendingAgentMsgs effect, HTTP .then() success, HTTP .catch() success), but the ACTIVITY_LOGGED handler at lines 410-419 also clears `sending` + `sendingFromAPIRef` when the platform logs the workspace's a2a_receive ok/error. It only cleared 2 of the 3 refs — same exact bug class as the original. If THIS path wins the race (a2a_receive activity logged before pendingAgentMsgs delivers the reply text), sendInFlightRef stays stuck true and the next sendMessage() silently no-ops at line 464. Fix: route both branches (ok and error) through releaseSendGuards() so all four sites are now uniform. Updated the helper's docstring to explicitly list all four sites and warn that any future "I saw the reply" path that only clears the natural pair (sending + sendingFromAPIRef) will silently re-introduce the freeze. The disabled-button logic can't see sendInFlightRef so the visible state diverges from the synchronous re-entry guard otherwise. This is exactly the drift `releaseSendGuards()` was supposed to prevent — the helper landed in the prior commit but the activity-log site wasn't migrated to use it. Fixing now closes the gap. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
🔒 Auto-merge disabled — new commit ( |
Both AgentCommsPanel and ChatTab's activity-feed opened raw
`new WebSocket(WS_URL)` instances per mount, with no onclose handler
and no reconnect logic. When the underlying connection dropped — idle
timeout, browser background-tab throttle, network jitter — the per-
panel sockets stayed dead until the panel re-mounted (refresh or
sub-tab unmount/remount). Live agent-comms bubbles and live activity
feed lines silently went missing in the gap, manifesting as "the
delegation didn't show up until I refreshed."
The global ReconnectingSocket in store/socket.ts already owns
reconnect, exponential backoff, health-check, and HTTP fallback poll.
Routing component subscribers through it gives every consumer those
guarantees for free, with one TCP connection per tab instead of N.
Three new pieces:
- store/socket-events.ts: tiny pub/sub bus. emitSocketEvent fan-outs
every decoded WSMessage to the listener Set; subscribeSocketEvents
returns an unsubscribe. A throwing listener is logged and isolated
so it can't break siblings.
- store/socket.ts: ws.onmessage now calls emitSocketEvent(msg) right
after applyEvent(msg), so the store's derived state and component
subscribers stay in lockstep on every event arrival.
- hooks/useSocketEvent.ts: React hook that registers exactly once
per mount, capturing the latest handler in a ref so the closure
sees current state/props without re-subscribing on every render.
Refactored sites:
- AgentCommsPanel: replaced its WebSocket-in-useEffect block with
useSocketEvent. Same parsing logic; the panel no longer opens its
own connection.
- ChatTab activity feed: split the previous useEffect in two — one
seeds the activity log when `sending` flips, the other subscribes
unconditionally and gates work on `sending` inside the handler.
Hooks can't be conditional, so the gate has to live in the body
rather than around the effect.
The ws-close graceful-close helper is no longer needed in either
site; the global socket owns its own teardown.
Tests: 6 new tests for the bus contract (single delivery, fan-out
order, unsubscribe, throwing-listener isolation, no-subscriber emit,
duplicate-subscribe Set semantics). All 27 existing socket tests
still pass.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
🔒 Auto-merge disabled — new commit ( |
HongmingWang-Rabbit
deleted the
fix/canvas-send-button-stuck-after-ws-reply
branch
April 27, 2026 20:21
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Test plan
🤖 Generated with Claude Code