Skip to content

feat(web-react): interleave assistant text and tool chips via ordered message segments#140

Merged
Tjemmmic merged 5 commits into
mainfrom
feat/web-react-interleaved-segments
Jun 24, 2026
Merged

feat(web-react): interleave assistant text and tool chips via ordered message segments#140
Tjemmmic merged 5 commits into
mainfrom
feat/web-react-interleaved-segments

Conversation

@Tjemmmic

Copy link
Copy Markdown
Contributor

Overview

ChatMessages renders an assistant turn as one header + one markdown body + a tool-chip group after it. A turn that reasons, calls a tool, reasons again, calls another tool renders as one concatenated text blob with every tool chip collected at the bottom — the chronological flow is lost.

Change

Add an optional ordered segments array to ChatUiMessage:

  • ChatMessageSegment = { kind: 'text'; content } | { kind: 'tool'; call }.
  • When a message carries non-empty segments, the assistant bubble renders them in order — interleaving text runs and tool cards under the single existing header (and reasoning box). Only the trailing text run of a live turn smooth-types and shows the caret.
  • When segments is absent, rendering is unchanged: the existing content body + toolCalls group. Fully backward-compatible.

This is additive — no existing field changes meaning, and producers that don't segment a turn render exactly as before.

Tests

src/web-react/chat-messages-segments.test.tsx: renders ChatMessages and asserts (a) segmented turns render text/tool/text in DOM order, (b) unsegmented messages still fold to content-then-tools, (c) an empty segments array falls back to content.

pnpm test (web-react) green; pnpm build (DTS typecheck) green.

@Tjemmmic

Tjemmmic commented Jun 23, 2026

Copy link
Copy Markdown
Contributor Author

🤖 AI Code Review (ensemble)

Resolution (fa2f9e2): 2 addressed · 1 not an issue.

Summary

The PR adds ordered segments to ChatUiMessage to interleave assistant text and tool chips chronologically, rendering them via a dedicated SegmentText component while preserving a legacy fallback. The implementation is conceptually sound, but contains a UX regression where the streaming caret vanishes if a tool call is the final segment during a live stream, alongside minor stability and duplication issues.

Issues Found

3 total — 0 P1 (blocking) · 1 P2 (should fix) · 2 P3 (nice to have)

⚠️ P2 — Streaming caret lost when the last segment is a tool callNot an issue

Note: No regression: the legacy path also suppresses the caret whenever toolCalls are present, and during a trailing tool call the running tool card's animated pulse (plus the composer indicator in sandbox-ui hosts) already signals work in progress. The suggested fix would paint a typing caret on an already-finished text run while a later tool runs, misrepresenting that text as still streaming.

  • File: src/web-react/index.tsx:589-602
  • Problem: In the segmented branch, showCaret and streaming are only passed to SegmentText when seg.kind === 'text'. If the trailing segment is a tool call, the tool branch renders without a caret, and no preceding text run gets showCaret=true. This results in no typing indicator showing while a tool call is in-flight and the turn is still streaming, a visible UX regression vs. the legacy path.
  • Fix: Compute the index of the last text run independently of isLast (e.g., const lastTextIndex = segments.reduce((acc, s, i) => s.kind === 'text' ? i : acc, -1)). Pass streaming={streaming && i === lastTextIndex} to SegmentText. Alternatively, pass a streaming flag into ToolCallCard if the indicator should append to in-progress tool chips.

ℹ️ P3 — Tool segment keys rely on unenforced uniquenessAddressed

Note: Tool keys are now namespaced as tool-<call.id> (fa2f9e2), so they can't collide with the text-<i> keys; call ids are unique within a turn (deduped by call id upstream).

  • File: src/web-react/index.tsx:604
  • Problem: key={seg.call.id} assumes tool call IDs are strictly unique within a message's segments. The type ChatToolCallInfo does not enforce this, and streaming producers could theoretically re-emit updates resulting in duplicate IDs. This risks React key-collision warnings and state corruption.
  • Fix: Use a composite or index-augmented key consistent with the text-segment approach: key={tool-${i}-${seg.call.id}}.

ℹ️ P3 — Duplicate text-block markup across segmented and legacy branchesAddressed

Note: Extracted a shared StreamingCaret component used by both the segmented and legacy branches (fa2f9e2), so the streaming cue can't diverge.

  • File: src/web-react/index.tsx:479-486 and 582-588
  • Problem: The wrapper div classes and the caret span markup are duplicated between SegmentText and the legacy fallback branch. A future change to the caret's animation or styling will likely miss one branch, causing the segmented and legacy paths to visually diverge.
  • Fix: Extract the caret span into a small shared component (e.g., StreamingCaret) and consume it in both SegmentText and the legacy branch.

✅ APPROVE

The core implementation correctly introduces segmented interleaving and safely falls back to legacy behavior. Reviewer A identified a valid P2 UX regression regarding the streaming caret disappearing before a trailing tool call. Since this is the only P2 issue, it does not trigger a CHANGES_REQUESTED mandate, but it should be addressed to ensure streaming visual consistency. The minor stability and duplication concerns are tracked as P3s.

Quick Reference

  • P2: Streaming caret lost when the last segment is a tool call
  • P3: Tool segment keys rely on unenforced uniqueness
  • P3: Duplicate text-block markup across segmented and legacy branches

Synthesized by Sokuza AI from multiple independent reviewers

@Tjemmmic

Copy link
Copy Markdown
Contributor Author

🤖 AI Code Review (ensemble)

Summary

The PR adds ordered message segments to ChatMessages, enabling assistant text and tool chips to interleave chronologically. The implementation is solid with good test coverage for segmented rendering, fallback behavior, and empty segments. Two P2 issues were identified: one regarding streaming caret suppression when the last segment is a tool call, and another about potential key collisions between text and tool segments.

Issues Found

2 total — 0 P1 (blocking) · 2 P2 (should fix) · 0 P3 (nice to have)

⚠️ P2 — Streaming caret suppressed when the last segment is a tool call

  • File: src/web-react/index.tsx:595-598
  • Problem: In the segmented render path, showCaret is only set for the last segment when seg.kind === 'text'. When the last segment is a tool call, no caret is shown even though streaming is true, making the UI appear frozen during tool execution. The fallback path shows the caret during streaming when there are no tool calls, but the segmented path has no equivalent for trailing tool calls.
  • Fix: If a trailing caret is desired during tool execution, render it after the ToolCallCard when streaming && isLast && seg.kind === 'tool'. At minimum, confirm whether this is the intended UX rather than an oversight, given the inconsistency with the non-segmented path.

⚠️ P2 — Segment list keys can collide between text segments and tool calls

  • File: src/web-react/index.tsx:584-598
  • Problem: Text segments use generated keys like text-0, while tool segments use raw seg.call.id. Tool call IDs are external data not constrained here, so a call ID such as text-0 will collide with the first text segment key in the same sibling list. Duplicate React keys can cause incorrect reconciliation during streaming, preserving the wrong useSmoothText state or reusing a component instance for the wrong segment.
  • Fix: Namespace tool segment keys and include the segment index for uniqueness, e.g., key={tool-${i}-${seg.call.id}}. This ensures keys remain unique even if a producer emits duplicate tool IDs.

✅ APPROVE

The core interleaving logic is sound and well-tested. Two P2 issues were identified: one UX inconsistency around the streaming caret on trailing tool segments, and one key collision edge case. Neither is blocking, but both should be addressed before or shortly after merge. The decision is APPROVE since there are no P1 issues and fewer than three P2s.

Quick Reference

  • P2: Streaming caret suppressed when the last segment is a tool call
  • P2: Segment list keys can collide between text segments and tool calls

Synthesized by Sokuza AI from multiple independent reviewers

@Tjemmmic

Tjemmmic commented Jun 23, 2026

Copy link
Copy Markdown
Contributor Author

🤖 AI Code Review (ensemble)

Resolution (9bc77a6): 3 addressed.

Summary

The PR adds optional interleaved text/tool segments for assistant messages, rendering them in emission order with a smooth text effect for streaming, while preserving the legacy fallback. The implementation is largely solid and well-tested, but it has a few user-visible edge cases: an empty text segment can render a blank gap, the streaming caret can disappear when a tool call is the last segment, and tool calls in msg.toolCalls silently vanish when segments are present.

Issues Found

3 total — 0 P1 (blocking) · 3 P2 (should fix) · 0 P3 (nice to have)

⚠️ P2 — Empty text segment renders a full-height empty divAddressed

Note: SegmentText now early-returns null for whitespace-only content (after its hooks, so it stays rules-of-hooks safe), so an empty run paints no gap.

  • File: src/web-react/index.tsx:517-525
  • Problem: SegmentText unconditionally renders <div className="text-base leading-[1.75]">. If a producer emits a text segment with empty or whitespace-only content (e.g., a placeholder between tools), this creates a visible vertical gap in the layout.
  • Fix: Early-return null in SegmentText when content is empty or whitespace-only: if (!content.trim()) return null.

⚠️ P2 — Streaming caret never appears when the last segment is a tool callAddressed

Note: When the turn is streaming and the last segment is a tool, a trailing StreamingCaret now renders after the run (the running tool's own pulse still covers the in-flight case).

  • File: src/web-react/index.tsx:603-608
  • Problem: In the segmented branch, showCaret evaluates to true only for the last segment if it is text. If the message is actively streaming and the final segment is a ToolCallCard, the caret is hidden because tool cards do not render StreamingCaret. This results in a missing streaming indicator during the gap before the next text segment arrives.
  • Fix: If the message is streaming and the last segment is a tool, render a trailing StreamingCaret after the ToolCallCard or accept the gap explicitly.

⚠️ P2 — Tool chips silently dropped when segments lack tool callsAddressed

Note: SegmentedBody now also renders any msg.toolCalls whose ids aren't already present in segments (deduped), so a partially-migrated producer can't drop a tool chip. Covered by new tests.

  • File: src/web-react/index.tsx:591-616
  • Problem: When msg.segments is present and non-empty, the code exclusively renders the segments and ignores msg.toolCalls. If a producer sends text-only segments but still populates msg.toolCalls (e.g., during a partial migration), those tool calls silently disappear. The type definition allows both fields to exist simultaneously.
  • Fix: Deduplicate by rendering trailing msg.toolCalls that are not already present in segments, or document and enforce upstream that segments fully replaces toolCalls.

❌ CHANGES REQUESTEDResolved — no blocking concerns remain

Status: every blocking item is resolved in code (with tests); any not-an-issue item is explained above.

While the core feature works as intended and is well-tested, there are exactly three P2 issues representing distinct user-visible edge cases: rendering gaps from empty segments, a missing streaming caret, and silent data loss for tool calls. Per the decision rules, three or more P2s require changes requested.

Quick Reference

  • P2: Empty text segment renders a full-height empty div
  • P2: Streaming caret never appears when the last segment is a tool call
  • P2: Tool chips silently dropped when segments lack tool calls

Synthesized by Sokuza AI from multiple independent reviewers

@Tjemmmic

Tjemmmic commented Jun 23, 2026

Copy link
Copy Markdown
Contributor Author

🤖 AI Code Review (ensemble)

Resolution (ae29525): 4 addressed.

Summary

PR adds interleaved rendering of assistant text and tool-call chips via an ordered segments field, with legacy fallback when absent. Tests cover ordering, dedup, orphans, and empty segments. Overall a clean, well-tested addition, but a user-facing regression in the reasoning panel state justifies requesting changes before merge.

Issues Found

4 total — 0 P1 (blocking) · 3 P2 (should fix) · 1 P3 (nice to have)

⚠️ P2 — Segmented answers leave the reasoning panel stuck in Thinking stateAddressed

Note: The reasoning box now keys off a derived hasAnswerText (true when content is non-empty OR any text segment has content), not just msg.content. A segmented message with content: '' collapses the reasoning box and stops showing "Thinking…" once an answer segment exists. New regression test added.

  • File: src/web-react/index.tsx:L606-L645
  • Problem: AssistantMessageImpl still derives answer presence from the smoothed legacy msg.content value. The new segmented path allows messages with content: '' and all answer text in segments (as the added tests do), so a segmented assistant message with reasoning will keep <details open={!content}> open and continue showing Thinking… even after answer segments are visible or the turn has completed. That makes completed responses look like they are still thinking.
  • Fix: Derive the reasoning/open state from the rendered answer source, not only msg.content. For segmented messages, treat a non-empty text segment as answer text when deciding whether to start/stop thinking and whether the details should remain open. Add a regression test covering a segmented message with reasoning, content: '', and a non-empty text segment.

⚠️ P2 — Empty/whitespace text segments render null, losing the streaming caretAddressed

Note: SegmentText returns null for an empty run only when it is NOT the live trailing run; when it is (showCaret), it still renders the caret, so a turn ending on an empty streaming text run isn't left without a cue.

  • File: src/web-react/index.tsx:496-503
  • Problem: In SegmentText, when content.trim() is falsy the component returns null. If the last segment is an empty text run (e.g., { kind: 'text', content: ' ' }), the component returns null after the streaming && i === lastIndex caret logic is already computed, silently losing the streaming caret. The fallback caret at the bottom of SegmentedBody only fires for a trailing tool segment, not a trailing empty text segment.
  • Fix: Move the empty-content check before hook calls, or explicitly render the caret even when content is empty: if (!content.trim() && !(showCaret && streaming)) return null. Alternatively, check segments[lastIndex]?.kind and content emptiness in SegmentedBody before delegating the caret to the child.

⚠️ P2 — SegmentText caret condition text && may suppress caret during smooth-text animationAddressed

Note: The caret now gates on showCaret alone (not the smoothed text, which is '' on the first frame), so it's steady from the start instead of flickering.

  • File: src/web-react/index.tsx:500
  • Problem: {showCaret && text && <StreamingCaret />} gates on the smoothed text value, which starts as an empty string at the beginning of a streaming turn (before the smooth-text tick has ramped up) even when content is non-empty. This could cause a brief flicker where the caret disappears on the first render frame. The original legacy code gated on content && (the raw prop), not the smoothed value.
  • Fix: Gate on the raw content prop instead of the smoothed text: {showCaret && content && <StreamingCaret />}.

ℹ️ P3 — Streaming caret on trailing tool segment has no test coverageAddressed

Note: Added a test that renders a streaming turn whose last segment is a tool and asserts the trailing caret (the only aria-hidden pulsing span) is present.

  • File: src/web-react/index.tsx:572
  • Problem: SegmentedBody renders an extra <StreamingCaret /> when streaming && segments[lastIndex]?.kind === 'tool', but no test exercises this path. If this JSX is malformed or the condition is wrong, it would go undetected.
  • Fix: Add a test case where the message is streaming and the last segment is a tool call, asserting the caret element is present in the DOM.

❌ CHANGES REQUESTEDResolved — no blocking concerns remain

Status: every blocking item is resolved in code (with tests) or shown to be a false positive (see notes).

No P1 issues, but three P2s tip it over the threshold. The most serious is the reasoning panel regression flagged by Reviewer C: the <details open={!content}> logic still keys off msg.content, so segmented answers with content: '' and reasoning will look stuck in Thinking state. Reviewer A's caret concerns are visually cosmetic but easy fixes. Recommend addressing at least the P2s before merge.

Quick Reference

  • P2: Segmented answers leave the reasoning panel stuck in Thinking state
  • P2: Empty/whitespace text segments render null, losing the streaming caret
  • P2: SegmentText caret condition text && may suppress caret during smooth-text animation
  • P3: Streaming caret on trailing tool segment has no test coverage

Synthesized by Sokuza AI from multiple independent reviewers

@Tjemmmic

Tjemmmic commented Jun 23, 2026

Copy link
Copy Markdown
Contributor Author

🤖 AI Code Review (ensemble)

Resolution: 3 acknowledged — valid non-blocking notes on the approved change; the current behavior is correct for the streaming/append-only model (see notes).

Summary

This PR adds ordered message segments to interleave assistant text and tool chips chronologically, with a clean fallback to the legacy content + toolCalls layout. The implementation is solid and well-tested, but the index-based React key for segments and the leftover tool call rendering order introduce fragility under certain producer patterns.

Issues Found

3 total — 0 P1 (blocking) · 2 P2 (should fix) · 1 P3 (nice to have)

⚠️ P2 — Segment index used as React key is not stable when segments are mutated ℹ️ Acknowledged

Note: The index key is intentional and correct for this model: segments are append-only within a turn (the platform adapter rebuilds them from the ordered transcript, which only grows), so index i maps to the same logical run across renders and keeps each SegmentText's smooth-text state. A content-hash key would be worse — it would change every streaming frame as the run grows, remounting the component and resetting the reveal. Documented as append-only in the code.

  • File: src/web-react/index.tsx:558-559
  • Problem: The comment claims "Segments only ever append within a turn," but if a producer updates a text segment's content in place or removes a segment, the index-based key text-${i} will cause React to reuse the wrong SegmentText instance. Since SegmentText holds useSmoothText streaming state, a stale instance would display incorrect or jumpy text. This is an unenforceable assumption that creates silent corruption if violated.
  • Fix: Use a stable identifier as the key. If segments don't carry IDs, derive one from content hash or require producers to provide one. Alternatively, add a runtime assertion or type-level constraint documenting that segments must be append-only.

⚠️ P2 — Leftover tool calls always render after all segments, breaking chronological order ℹ️ Acknowledged

Note: Only reachable if a producer sets BOTH segments and orphan toolCalls — which the platform adapter never does (it emits one or the other). toolCalls is a flat list with no position relative to segments, so appending the orphans is the only order-preserving-where-possible option; the alternative (round 2's concern) was silently dropping them, which is worse. A no-op for every real producer.

  • File: src/web-react/index.tsx:566-567
  • Problem: When a partially-migrated producer sets both segments and toolCalls, the leftoverToolCalls (those not represented in segments) are rendered after the entire segments array. This defeats the purpose of interleaving: orphaned tools appear at the very end regardless of when they were emitted relative to text segments. This is a silent semantic regression for mixed-source messages.
  • Fix: At minimum, document this limitation explicitly in a comment and surface a console warning during development so producers know to fully migrate. Better, attempt to interleave leftover tool calls based on a heuristic or position info if available.

ℹ️ P3 — SegmentText memo effectiveness depends on renderBody stability from parent ℹ️ Acknowledged

Note: renderBody is already stabilized: ChatMessages wraps it in useMemo(... , [renderMarkdown]), and the platform passes a useCallback-stable renderMarkdown. So the per-segment memo holds in practice.

  • File: src/web-react/index.tsx:505
  • Problem: useMemo(() => renderBody(text), [renderBody, text]) depends on renderBody. If the parent recreates renderBody on every render (e.g., an inline arrow function), this memo will recompute every frame, defeating its purpose. The parent AssistantMessageImpl receives renderBody from props, so its stability depends on the consumer with no enforced contract.
  • Fix: Verify that callers of AssistantMessageImpl memoize renderBody via useCallback. Alternatively, wrap the memo to be resilient to unstable references by using a ref to hold the latest renderBody.

✅ APPROVE

No P1 issues found and only 2 P2 issues, so this meets the APPROVE threshold. The P2s are architectural robustness concerns about assumptions (append-only segments, renderBody stability) that hold for the current implementation but are not enforced. They should be addressed before final merge but are acceptable for a draft PR. The change is well-tested with focused coverage for ordering, fallback, duplicate suppression, empty segments, reasoning state, and streaming caret behavior.

Quick Reference

  • P2: Segment index used as React key is not stable when segments are mutated
  • P2: Leftover tool calls always render after all segments, breaking chronological order
  • P3: SegmentText memo effectiveness depends on renderBody stability from parent

Synthesized by Sokuza AI from multiple independent reviewers

@Tjemmmic Tjemmmic marked this pull request as ready for review June 24, 2026 00:56

@tangletools tangletools left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Auto-approved PR — ae295258

Blanket team auto-approval is enabled for this reviewer service.
The full PR reviewer audit still runs separately and will publish findings if it detects issues.

tangletools · auto-approval · reason: blanket_auto_approve · 2026-06-24T00:56:36Z

@Tjemmmic Tjemmmic merged commit f7ad460 into main Jun 24, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants