fix(chat): drop empty reasoning parts at the render boundary - #3448
Conversation
#3444 dropped them during stream assembly, which misses every conversation loaded back from storage: those parts were already persisted, and rendering reads `message.parts` straight through `groupPartsInOrder`. Old threads kept showing an empty "Thought process" disclosure. Moved to `groupPartsInOrder`, the single boundary every rendered message crosses, live or restored, whoever produced the parts. AG-UI permits a reasoning message that opens and closes with no content events and leaves the display decision to the consumer, so this is where the decision belongs, and assembly goes back to reporting the stream as it arrived. The skip runs before the text buffer flushes, like the tool-result skip above it, so text either side stays one block instead of splitting a list or paragraph in two. Net effect on the streaming layer is a deletion.
|
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 ignored due to path filters (1)
📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe change preserves empty completed reasoning spans during streaming assembly. Message grouping now filters those spans while retaining streaming, signed, redacted, and non-empty reasoning parts. ChangesReasoning visibility
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Completes #3444, which fixed this in the wrong layer.
The gap
#3444 dropped empty reasoning spans during stream assembly (
buildCurrentParts). Rendering does not go through that. It readsmessage.partsstraight intogroupPartsInOrder, and for a conversation loaded back from storage those parts come from persistence:Empty parts were being persisted before #3444 —
buildCurrentPartsemitted them,handleFinishpassed them toonMessage, storage kept them. So new threads were clean and every existing thread stayed broken.The fix
Moved to
groupPartsInOrder: the single boundary every rendered message crosses, live or restored, whichever producer built the parts.This is also the layer the spec points at. AG-UI permits a reasoning message that opens and closes with no content events, and explicitly leaves it to the consumer to decide whether to display, suppress, or flag the result. Assembly goes back to reporting the stream as it arrived; display is the consumer's call.
So the assembly-layer guard from #3444 is removed rather than kept alongside the new one. Net effect on the streaming layer is a deletion — one predicate at one boundary instead of the same rule in two places.
The skip runs before the text buffer flushes, matching the
tool-resultskip directly above it. Otherwise a list or paragraph spanning the empty part renders as two markdown blocks; there is a test for that, and it fails if the guard is moved after the flush.Carve-outs kept, both tested: a part still
streamingstays (it has yet to say anything), and a redacted or signed part stays (it carries meaning without visible text).Diff
16 lines of production change, net negative in the streaming layer.
isReasoningPartnow narrows to the realChatReasoningPartinstead of a hand-written subset that silently droppedsignatureandredactedData— which is what let the predicate read them.Verification
deno task typecheckclean,deno fmt --checkanddeno lintcleanflushText()fails itSummary by CodeRabbit
Bug Fixes
Documentation