fix(chat): keep each tool marker under its own card in back-to-back dispatches#3951
Conversation
… dispatches When the model dispatched two Bash tools in quick succession, late ToolProgress chunks for the first tool no longer matched the current toolStreamID, so streamToolOutput fell through to the generic collapse-then-append path. The fresh live block landed at the tail of the transcript under the second tool's card, and the first tool's collapsed ⎿ marker stacked beneath the second card as well — making the two runs visually indistinguishable. The fix threads the slot beginToolRunning recorded for the first dispatch through shellTranscriptIdx. When a late progress (or late result) for that id arrives, streamToolOutput and collapseToolOutput now reuse the recorded slot instead of appending, so each tool's live block and final summary stay directly under its own card regardless of dispatch/progress arrival order. Adds TestConsecutiveToolCallsKeepMarkersUnderOwnCard to lock the behaviour in: it verifies both markers are present and that the first card's marker previews the full output, not just the chunk visible before the second tool took over.
esengine
left a comment
There was a problem hiding this comment.
The slot-threading design is right and the regression test is exactly the lock-in I want — verifying marker position and that the first card's preview carries the post-late-progress output. One real bug before merge, one optional note:
1. The late path renders "-1 lines" for non-shell tools (blocker). In collapseShellSlot, when the late path runs for an id that isn't the active stream (n = -1) and has no shellOutputs entry — which is every non-shell- tool — n stays -1, skips the n == 0 branch, misses the shellOutputs branch, and lands in the else, rendering ⎿ -1 lines. Your own comment says "for other tools we fall back to leaving the slot blank", but the code never does. And this path is reachable: beginToolRunning now records every id in shellTranscriptIdx, so two back-to-back read_file calls where the first result arrives after the second dispatch hit it. Add an explicit if n < 0 { m.transcript[idx] = ""; m.shellTranscriptIdx[id] = idx; return } (or make the comment true some other way) plus a test with a non-shell id.
2. Zero-output tools now leave a blank line (non-blocking). The old active-path collapse truncated the trailing slot when a tool finished with no output; the new code always keeps the slot as "" so a late chunk has somewhere to land. The comment explains the trade-off honestly, and I can live with a blank line — just flagging that it's a deliberate visual change, not an accident.
Fix the -1 lines path and this is good to go — the ping-pong of live state between interleaved streams losing only the preview tail (full output recovered from shellOutputs at collapse) is an acceptable cosmetic cost and correctly bounded.
The back-to-back dispatch fix in 8b1dcd7 records every dispatched id in shellTranscriptIdx, so a late ToolResult for an earlier tool can land in the correct slot. But for non-shell-prefixed tools (e.g. read_file dispatched in parallel) the streaming state belongs to the current id and the shellOutputs accumulator is never populated, so the late path in collapseShellSlot computed n = -1 and rendered the final else branch as '⎿ -1 lines'. The visual result was two negative-count markers stacked at the end of the transcript, one per parallel tool. Guard n < 0 in collapseShellSlot by treating the unknown as zero output: clear the slot rather than fabricate a count. The id stays recorded in shellTranscriptIdx (set by beginToolRunning) so a late ToolProgress that finally arrives for that id can still extend the slot in place. Add TestConsecutiveNonShellToolsDoNotRenderNegativeLineCount, a minimal regression covering the read_file/read_file case the reviewer flagged: two back-to-back dispatches, late ToolResult for the first one. Verified by stashing the chat_tui.go change and rerunning — the test fails with the exact '-1 lines' transcript the user reported.
|
One more for the same revision, found in a second pass: the late branch of |
esengine
left a comment
There was a problem hiding this comment.
The n < 0 guard + regression test address the blocker exactly — clearing the slot instead of fabricating a count is the right call, and the test asserting the literal "-1 lines" absence is a good tripwire. Merging.
Our comments crossed mid-air (you pushed one minute before mine landed): the late branch still returns without setting transcriptDirty, so the rewritten slot waits for the next event to repaint. It's a one-liner — I'll push it as a follow-up so this doesn't need another round. Thanks for the quick turnaround.
Follow-up to #3951: the late-result branch of collapseToolOutput mutated the transcript without setting transcriptDirty, so the rewritten slot waited for the next unrelated event to paint. Set the flag inside collapseShellSlot, covering both callers. Co-authored-by: reasonix <reasonix@deepseek.com>
fix(chat): keep each tool marker under its own card in back-to-back dispatches
When the model dispatched two Bash tools in quick succession, late
ToolProgress chunks for the first tool no longer matched the current
toolStreamID, so streamToolOutput fell through to the generic
collapse-then-append path. The fresh live block landed at the tail of
the transcript under the second tool's card, and the first tool's
collapsed marker stacked beneath the second card as well - making the
two runs visually indistinguishable.
The fix threads the slot beginToolRunning recorded for the first
dispatch through shellTranscriptIdx. When a late progress (or late
result) for that id arrives, streamToolOutput and collapseToolOutput
now reuse the recorded slot instead of appending, so each tool live
block and final summary stay directly under its own card regardless
of dispatch/progress arrival order.
Adds TestConsecutiveToolCallsKeepMarkersUnderOwnCard to lock the
behaviour in: it verifies both markers are present and that the first
card marker previews the full output, not just the chunk visible
before the second tool took over.
Repro before the fix (TranscriptIndexed on by default):
Old transcript: card1, "On branch main-v2" (frozen), "", card2,
"On branch main-v2\nnothing to commit" (stacked marker)
New transcript: card1, full preview, "", card2, marker (each under its own card)