fix(chat): keep reused text blocks in order and drop empty reasoning spans - #3444
Conversation
…spans Follow-up to #3438, from probing the rest of the streaming assembly for the same defect class. `handleTextStart` overwrote its map entry exactly the way `handleReasoningStart` did, so a content id reused across steps blanked the earlier answer and moved the later one below everything that streamed in between. Reasoning ids collide on every run because providers index them (`reasoning-0`); text ids usually carry a random suffix, which is the only reason this had not been seen. It is reachable today through the local provider, whose text id is `text-${Date.now()}` per doStream call — two steps inside one millisecond reuse it. The renderer would have hidden the damage: `getAnswerPartsForRendering` already drops pre-tool text when a post-tool answer exists, so the screen looks right while the persisted message loses content. Same rule as reasoning now applies: a start on an open block is a replayed start and keeps its text and order, a start after the block closed opens a new one and retires the old. Separately, a reasoning span that opens and closes without content rendered an empty "Thought process" disclosure, which providers produce on any step that did no thinking. Completed empty spans are dropped. Open ones stay, so the thinking affordance still appears while a span is streaming, and redacted or signed spans stay because they carry meaning without visible text.
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe streaming handler now preserves completed text blocks when providers reuse IDs. The parts builder includes those blocks in order and omits empty completed reasoning spans while retaining meaningful and redacted reasoning data. Tests cover these cases. ChangesStreaming block preservation
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Follow-up to #3438. After that landed I probed the rest of the streaming assembly for the same defect class. Two findings, one latent and one live.
handleTextStarthad the identical overwrite bugIt replaced its map entry unconditionally, exactly as
handleReasoningStartdid before #3438. A content id reused across steps blanks the earlier answer and moves the later one below everything that streamed in between:Reasoning ids collide on every run because providers index them (
reasoning-0). Text ids usually carry a random suffix (text-BG3xwKvhoiJGrrdZ), which is the only reason this hadn't surfaced. It is reachable today:src/provider/local/model-runtime-adapter.ts:145buildstext-${Date.now()}perdoStreamcall, so two steps completing inside one millisecond reuse the id.Worth flagging because it would have been hard to spot from the UI:
getAnswerPartsForRenderingalready hides pre-tool text once a post-tool answer exists, so the screen looks correct while the persisted message silently loses content.Same rule as reasoning now applies. A start on a block that is still open is a replayed start and keeps its text and its order; a start after the block closed opens a new block and retires the old one to
closedTextBlocks.Empty reasoning spans rendered an empty "Thought process"
A
reasoning-start/reasoning-endpair with no delta produced{type: "reasoning", text: ""}, and nothing guarded against it — notparts-builder, notgroupPartsInOrder, not theReasoningcomponent, which renders the disclosure trigger regardless of content. Providers open and close the span on any step that did no thinking, so this is common.This predates #3438 (a single empty span already produced it), but #3438 increased exposure: empty spans sharing an id used to collapse into one overwritten slot and now each is retained.
Completed empty spans are dropped. Two deliberate carve-outs, both covered by tests:
Verification
Probes against the built handler, five steps with both ids fully reused:
Nothing lost, nothing reordered, no duplicates.
Suites: agent + chat + react 1390 passed / 0 failed.
deno task typecheckclean,deno fmt --checkanddeno lintclean.Not fixed here
Reusing a tool call id across steps destroys the earlier call the same way. Left alone deliberately: correlating a result to a call requires the id to be unique, so a duplicate id is already ambiguous at the protocol level rather than something the client should paper over.
The AG-UI encoder still emits duplicate reasoning
messageIds across steps.use-chatis now robust to it, but other consumers of that wire are not. Fixing it there is a wire-contract change and wants its own PR.Summary by CodeRabbit