Fix duplicated thinking rows after mid-stream transcript redraws - #631
Closed
rian-dolphin wants to merge 6 commits into
Closed
Fix duplicated thinking rows after mid-stream transcript redraws#631rian-dolphin wants to merge 6 commits into
rian-dolphin wants to merge 6 commits into
Conversation
A full transcript redraw while an assistant message streams (any slash command triggers one via _refresh(), as do Ctrl+T, resize, and theme changes) remounted the provisional thinking item as a durable widget. Finalization only removes widgets tracked as live streaming state, so the remounted row was orphaned and the thinking block or its hidden placeholder appeared twice, permanently. Mark adapter-created provisional items on ChatItem and make _redraw and update_thinking_visibility treat them as live streaming state: their widgets register in _active_message_widgets, the trailing provisional thinking row is rebuilt as a streaming widget seeded from a copied item, and hidden-placeholder runs split when provisional-ness changes so a provisional run never shares a placeholder with durable history. The adapter clears provisional flags at assistant MessageStart and MessageEnd, and cancellation/error paths convert leftovers to durable rows so the next finalization cannot delete an interrupted turn's thinking. Also mount a mid-stream thinking toggle's trailing row above the live assistant widget instead of below the streamed answer text.
The review of the mid-stream fix found a regression: mounting a toggled thinking row above the live assistant widget means the placeholder is no longer the last child, so the child-inspection check reported it as not visible and the next thinking delta mounted a second placeholder. Derive visibility from the window items instead, mirroring _redraw, and drop the now-unused last-child helper.
finish_structured_assistant_message awaits between removing streamed widgets and mounting canonical blocks. A refresh or thinking toggle processed while the coroutine was suspended rendered the already canonical state, and the resumed finalization mounted a second full copy of the message: thinking and answer text both duplicated. This is the whole-message duplication seen live; the earlier commits' fix only covered redraws landing between events. Reset live-widget bookkeeping before the awaits so interleaved callers never observe half-finalized state, detect that a concurrent render already mounted this message's canonical items and let that render win instead of mounting a duplicate, and anchor update_thinking_visibility on the live assistant widget only while it is still mounted so a toggle interleaved with finalization cannot raise MountError.
A new assistant MessageStart without a prior MessageEnd converts the old stream's provisional rows to durable history, but their widgets stayed in the live set, so the next finalization removed the durable thinking row from the transcript. Release the live set at the stream boundary instead. Also track provisional items in their own list so converting them stops scanning the whole transcript.
A replacement MessageStart overwrote assistant_buffer, so the old stream's text had no state row behind its mounted widget and the next redraw dropped it. Flush the buffer into a durable assistant row first, the same policy _flush applies at AgentEnd and tool start.
Contributor
Author
|
Closed in favor of #632, which fixes the same two fault windows (mid-stream redraws and the finalization pause) with the ownership model from Pi: one cumulative active-assistant draft as the only provisional state, one owner for all live tail widgets that every redraw reconstructs, and stream/render generation guards for stale async work. All of this PR's test scenarios pass against #632, and its two strongest ones (a thinking toggle interleaved into a suspended finalization, and interrupted rows surviving the next turn's completion) were ported there as permanent regression tests. The failure analysis here directly informed the follow-up. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The TUI can show one streamed message or thinking messages two times. The copies stay on the screen. In the worst case, the thinking block and the answer text both show two times. The session file is correct in all cases. Only the display is not correct.
The problem occurs when the transcript does a new render while a message streams or completes. These actions start such a render:
/sessionand/model.Example before:
You can see the thinking block duplicated twice here.
Cause
The fault has two windows.
Fix
See the code for the details.
Tests
We wrote the tests first. Four tests caused the first fault on the main branch, through the real event path: a render, Ctrl+T, and
/session, with the thinking shown and hidden. All four failed. Then we applied part one of the fix.Reviews found more edge cases and one regression. A live test found the second window. We added a test for each case before each correction: eleven more tests. They cover interrupted and replaced streams, toggles after text, placeholder survival, and renders in the cleanup pause.
All fifteen tests pass. The full test suite, ruff, and mypy show no errors.
Same example on fix branch:
