fix(chat): #2713 flush pending render before segment reset at tool/interim

[b3nw]

Closes #2713 — live assistant text can truncate at tool-call segment boundaries during streaming.

Before _resetAssistantSegment() in the tool and interim_assistant SSE handlers, synchronously flush any pending rAF render work so tokens that arrived during the 66ms throttle window are written to the DOM before assistantBody is cleared. Without this flush, the pending _doRender callback fires after assistantBody is null and skips the write silently, causing the tail of the pre-tool segment to disappear from the live view.

The fix uses the existing _cancelAnimationFramePendingStreamRender() helper and writes via smd/renderMd/esc (same paths as _doRender) only when _renderPending is true — normal cases where the rAF has already fired are unaffected.

Completed transcripts were never affected (renderMessages rebuilds from the full assistantText accumulator on done).

[nesquena-hermes]

Summary

Reading the patch at static/messages.js:1423-1437 and the matching block at 1486-1502 against the rAF throttling logic on origin/master (static/messages.js:1313-1372), this is a targeted fix for a real bug: _scheduleRender() queues a rAF callback that captures assistantBody from closure scope, and _resetAssistantSegment() nulls that out (messages.js:1289-1294) — so the captured _doRender writes get silently dropped because of the if(assistantBody) guard at line 1335. Calling _cancelAnimationFramePendingStreamRender() plus a synchronous flush before the reset is the right shape.

Code reference

Before the fix, the rAF closure body at static/messages.js:1334-1339 was:

if(assistantBody){
  const displayText = segmentStart===0
    ? parsed.displayText
    : _stripXmlToolCalls(assistantText.slice(segmentStart));
  if(_shouldUseStreamFade()){
    ...

When the tool listener fires inside the 66ms throttle window (line 1325-1326), it goes straight to _resetAssistantSegment() which sets assistantBody=null. The queued rAF then sees assistantBody falsy and the partial-segment tail is lost from the live DOM. The completed transcript is unaffected because renderMessages() rebuilds from assistantText, which matches what the PR description claims.

Diagnosis

The fix block in both event handlers reads:

if(assistantBody&&_renderPending){
  _cancelAnimationFramePendingStreamRender();
  const displayText=segmentStart===0
    ? _parseStreamState().displayText
    : _stripXmlToolCalls(assistantText.slice(segmentStart));
  if(_smdParser){
    _smdWrite(displayText);
  } else if(renderMd){
    assistantBody.innerHTML=renderMd(displayText);
  } else {
    assistantBody.innerHTML=esc(displayText);
  }
}

Three things look correct:

1. The guard assistantBody && _renderPending means the no-pending case skips entirely — zero overhead on the hot path when rAF already fired.
2. The displayText computation mirrors _doRender at line 1336-1338 exactly, including the segmentStart===0 think-tag-vs-tool-strip distinction. Good — easy place to silently diverge otherwise.
3. The smd-vs-renderMd-vs-esc cascade matches the _doRender cascade at line 1347-1365.

Concerns

Stream-fade path not exercised. The real _doRender calls _renderStreamingFadeMarkdown(displayText) via _shouldUseStreamFade() at line 1339-1346 to drive the per-token fade animation. The flush path bypasses that and writes directly. That's probably fine because we're about to seal the segment anyway, but worth confirming: when stream-fade is active and a tool fires mid-fade, will the pre-tool tail render without fade animation? If users see that as a visual glitch it's worth gating the flush behind !_shouldUseStreamFade() or routing through _renderStreamingFadeMarkdown with a "force-complete" flag. Easy to defer to a follow-up if no one's complained.

_parseStreamState() is called twice in the interim_assistant path — once in the new flush block (computed from assistantText.slice(segmentStart) semantics, but only the displayText form), and again at the existing parsed=_parseStreamState() ~30 lines up. Not a correctness issue, just a perf nit: on long sessions the regex-driven think-tag stripping in _stripXmlToolCalls() (messages.js:849-864) isn't free. Cache once.

Code duplication. The two blocks are byte-identical. Worth extracting _flushPendingSegmentRender() (4 lines + 8-line body) and calling it from both listeners. Especially since the tool listener has an additional _freshSegment=true; _smdEndParser(); step that the interim handler doesn't, which makes it easy to drift over time.

Test plan

No automated coverage was added. The PR comment claims "Completed transcripts were never affected" — I agree from reading the code (the persisted message comes from assistantText, not DOM). But the live-render bug itself is hard to assert without a browser or a DOM-level harness. At minimum, please add a tests/test_issue2713_* regression that uses the same read('static/messages.js') AST-grep style as tests/test_streaming_race_fix.py (which already covers _wireSSE invariants) to pin:

• The tool listener body contains _cancelAnimationFramePendingStreamRender().
• It contains _renderPending as a guard.
• Both flush blocks reference both _smdWrite and renderMd/esc as fallback.

That way a future refactor of _scheduleRender() can't silently re-break this.

Otherwise this looks correct. The CHANGELOG entry is present.

[b3nw]

@nesquena-hermes addressed concerns as requested

[nesquena-hermes]

Summary

Re-read the latest HEAD (5655d0ff) against origin/master. Both review asks landed:

1. Helper extraction: _flushPendingSegmentRender() now lives at static/messages.js:1288-1301 as a single function. Both call sites (tool handler at line 1440, interim_assistant at line 1491) reduce to a single one-line invocation. The drift risk between the two byte-identical blocks I flagged in the first review is gone.
2. Test coverage: tests/test_issue2713_streaming_segment_flush.py (178 lines) uses the same read('static/messages.js') static-analysis pattern as test_streaming_race_fix.py. The class breakdown is exactly the shape I asked for.

Code reference

The extracted helper at static/messages.js:1288-1301:

function _flushPendingSegmentRender(){
  if(!assistantBody||!_renderPending) return;
  _cancelAnimationFramePendingStreamRender();
  const displayText=segmentStart===0
    ? _parseStreamState().displayText
    : _stripXmlToolCalls(assistantText.slice(segmentStart));
  if(_smdParser){
    _smdWrite(displayText);
  } else if(renderMd){
    assistantBody.innerHTML=renderMd(displayText);
  } else {
    assistantBody.innerHTML=esc(displayText);
  }
}

This is a closure over assistantBody, _renderPending, segmentStart, assistantText, and _smdParser from attachLiveStream() scope, so the call sites stay clean and the guard logic is single-sourced. Good.

The test invariants pin the three semantically meaningful properties: existence (test_flush_helper_declared), guards (test_flush_helper_guards_on_assistant_body + test_flush_helper_guards_on_render_pending), rAF cancellation (test_flush_helper_cancels_pending_raf), render cascade (_smdWrite/renderMd/esc fallback tests), and crucially the ordering invariant in both SSE handler tests:

flush_pos = fn.index("_flushPendingSegmentRender()")
reset_pos = fn.index("_resetAssistantSegment()", flush_pos)
assert flush_pos < reset_pos

If a future refactor moves the flush call below the reset, those two ordering tests fail — which is exactly the regression class the original bug was in.

Notes

The two deferred items from the first review remain:

• Stream-fade bypass. _flushPendingSegmentRender writes directly via _smdWrite/renderMd/esc rather than routing through _renderStreamingFadeMarkdown(). When _shouldUseStreamFade() returns true and a tool fires mid-fade, the pre-tool tail still won't get a fade animation. This is an aesthetic micro-glitch on a segment that's about to be sealed anyway, so it's fine to defer; just noting it's not addressed in this PR.
• _parseStreamState() call duplication in the interim_assistant path. Still computed once in the flush and once at the existing parsed=_parseStreamState(). Pure perf nit, defer.

Neither blocks the merge.

Verdict

LGTM. The helper extraction is the right shape, and the tests pin the contract well enough that the rAF/segment-reset ordering can't silently regress. CHANGELOG entry is present at the right severity (live-streaming display only, no transcript impact).

[nesquena-hermes]

Shipped in v0.51.122 via release/stage-batch4 (#2815). The _flushPendingSegmentRender helper is in. Closes #2713.