chore(web): delete needsNewBubbleRef latch, derive from message tail#31904
Conversation
The chat domain carried a `needsNewBubbleRef: MutableRefObject<boolean>` threaded through 5 hooks + 4 handler files + the StreamHandlerContext to gate whether the next streaming chunk should extend the last assistant bubble or open a fresh one. The same signal is already derivable from `prev[prev.length - 1]` inside `setMessages` updaters: a streaming assistant tail means "extend", anything else means "create". This PR removes the ref and replaces it with a `tailIsStreamingAssistant` derivation inside `stream-message-updaters.ts`. The latch behavior is preserved via two updater changes: * `appendTextDelta` falls through to `createStreamingBubble` when the tail isn't a streaming assistant (was a no-op before). * `finalizeOnIdle` now flips `isStreaming: false` on streaming assistant tails regardless of running tool-call presence. This is what guarantees the next chunk derives "open a new bubble" after daemon-signaled idle — replacing what the latch used to carry. `upsertToolCall` drops its `shouldCreateNewBubble: boolean` parameter entirely; callers no longer thread the decision. Source files: 9 cleaned of `needsNewBubbleRef` / `syncNeedsNewBubbleFromMessages` references. Single remaining mention is an intentional comment in `use-stream-event-handler.ts:281` explaining what was replaced. Tests: rewritten to assert message-array shape rather than ref values, per "test behavior, not implementation". +2 net passing tests; no regressions vs main (same 36 pre-existing failures from unrelated test-isolation/UI suites). Part of the Chat Bubble State Slimming workstream: 1. delete needsNewBubbleRef <- this PR 2. DisplayMessage id+mergedRowIds[] 3. /v1/history vocabulary sweep
![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: 73d64ae72a
ℹ️ 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".
| ctx.setMessages((prev) => { | ||
| const next = upsertToolCall(prev, newToolCall); | ||
| const tail = next[next.length - 1]; | ||
| // Stamp the stable-id ref to the streaming tail (no-op for extends; new | ||
| // id for creates). See parallel logic in handleAssistantTextDelta. |
There was a problem hiding this comment.
handleSubagentSpawned reads currentAssistantStableIdRef.current synchronously to attach a subagent to its parent message, but this change updates that ref only inside the setMessages updater. Because React runs state updaters during its later render flush, a subagent_spawned event that arrives immediately after tool_use_start can see a stale/undefined ref and store the subagent without parentMessageStableId, so it cannot be grouped under the live transcript bubble. Set the ref before calling setMessages (or derive from messagesRef.current synchronously) so parent linkage does not depend on render timing.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
ignoring for now as we will look to clean up this stable id business
| stableId?: string, | ||
| ): DisplayMessage[] { | ||
| const last = prev[prev.length - 1]; | ||
| if (!last || last.role !== "assistant" || !last.isStreaming) return prev; |
There was a problem hiding this comment.
In our conversation focused on simplifying client state, let's add an item to our list for removing isStreaming from messages.
Messages don't have streaming state. they are just the packets of data. the only thing that has streaming state is the assistant itself on any given conversation
| const last = prev[prev.length - 1]; | ||
| if (!last || last.role !== "assistant" || !last.isStreaming) return prev; | ||
| if (!tailIsStreamingAssistant(prev)) { | ||
| return createStreamingBubble(prev, text, messageId, stableId); |
There was a problem hiding this comment.
In our conversation focused on simplifying client state, let's add an item to our list for removing Bubble terminology.
- we don't even use chat bubbles anymore for our assistant messages
- even if we did, that is a rendering concern and not a state management concern
| // `setMessages([])` makes the tail derivation return "create new bubble" | ||
| // for any subsequent stream event — no separate latch needed. |
There was a problem hiding this comment.
delete this comment
| // Suppress per-chunk text_delta noise — only log the first delta of a | ||
| // new bubble. "First delta" is derived from the message ref tail: | ||
| // if the tail isn't a streaming assistant, the next text_delta will | ||
| // open a fresh bubble. This replaces the previous `needsNewBubbleRef` | ||
| // latch with a tail-derivation read. | ||
| const tail = messagesRef.current[messagesRef.current.length - 1]; | ||
| const tailIsStreaming = | ||
| !!tail && tail.role === "assistant" && !!tail.isStreaming; |
There was a problem hiding this comment.
Delete this comment and import our new tailIsStreamingAssistant helper here
| ctx.setMessages((prev) => { | ||
| const next = upsertToolCall(prev, newToolCall); | ||
| const tail = next[next.length - 1]; | ||
| // Stamp the stable-id ref to the streaming tail (no-op for extends; new | ||
| // id for creates). See parallel logic in handleAssistantTextDelta. |
There was a problem hiding this comment.
ignoring for now as we will look to clean up this stable id business
1 participant
Summary
Delete the
needsNewBubbleRef: MutableRefObject<boolean>latch that was threaded through 5 hooks + 4 handler files + theStreamHandlerContextto gate "should the next streaming chunk extend the last assistant bubble or open a fresh one?". The same signal is already derivable fromprev[prev.length - 1]insidesetMessagesupdaters — a streaming assistant tail means "extend", anything else means "create".This is PR 1 of the Chat Bubble State Slimming workstream. Governing principle:
skills/software-engineering/references/frontend-architecture.md— limit bespoke client state when the canonical model can carry the signal.What changed
New derivation helper in
stream-message-updaters.ts:Two updater behavior changes carry what the latch used to carry:
appendTextDeltafalls through tocreateStreamingBubblewhen the tail isn't a streaming assistant (was a no-op before). OptionalstableIdparam for the new bubble.finalizeOnIdleflipsisStreaming: falseon streaming assistant tails regardless of running tool-call presence. This is what guarantees the next chunk derives "open a new bubble" after daemon-signaled idle — replacing what the latch used to carry.upsertToolCalldrops itsshouldCreateNewBubble: booleanparameter; callers no longer thread the decision.Files touched
needsNewBubbleRef/syncNeedsNewBubbleFromMessages/shouldCreateNewBubbleStreamHandlerContextinterface drops the reftest-helpers.tsfactory drops the refuse-stream-event-handler.ts:281explaining what was replacedVerification
bunx tsc --noEmit: 0 errorsbun run lint: cleanbun test src/domains/chat/ src/domains/conversations/: 1321 pass / 36 fail (same 36 fail count asmain; failures are pre-existing test-isolation problems and UI render flakes — none in files I touched).appendTextDelta > uses stableId when creating a new bubblefinalizeOnIdle > flips isStreaming to false on a streaming assistant with no tool calls at allneedsNewBubbleRef|syncNeedsNewBubbleFromMessages|shouldCreateNewBubbleacrossapps/web/src/: ONE result, the explanatory comment.Workstream
Part of [Chat Bubble State Slimming](workstream-command-center record
282e972a-aa59-4a38-ad96-68f119fcc835):DisplayMessageid+daemonMessageIdintoid+mergedRowIds[]with server-emitted merge clusters/v1/historyvocabulary sweepTriggered by 4-bubble fragmentation on conversation
2e44d9af-6f4a-4847-a2e3-576b247b4edf. The class of bug PR 1 + PR 2 together defend against: any reconcile or state-leak event that flipsisStreaming: falseon the wrong row would cause the next tool call / text delta to spawn a fresh bubble and split the turn. With the ref gone and the message-array shape as the sole source of truth, there's no second place for the bubble-creation decision to disagree with itself.