TUI - Optimize rendering of long streaming responses - #4037
Conversation
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟡 NEEDS ATTENTION
Lower-confidence findings (not posted inline)
- [medium]
pkg/tui/tui_stream_scrolledup_test.go:59— Racy reset of atomic counters (compositions,writes) while render goroutine may be incrementing them (confidence: weak 52/100) - [medium]
pkg/tui/tui_stream_scrolledup_test.go:63— Sleep-based timing assumptions make render-count assertions flaky on slow CI (confidence: weak 52/100) - [low]
pkg/tui/components/scrollview/scrollview.go:327—composePaddedsilently drops trailing scrollbar lines whensbLinesis longer than the content lines slice (informational) - [low]
pkg/tui/tui_stream_motion_scaling_test.go:103—root.ar.Stop()called afterprogram.Quit()/<-done; if the animation runtime sends into the quit program a goroutine leak or panic may occur (informational) - [low]
pkg/tui/components/message/message.go:219—imageScanTailsliced by raw byte index may produce invalid UTF-8 when the final character is a multi-byte rune (informational)
993f88b to
442f4af
Compare
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟡 NEEDS ATTENTION
4 medium-severity findings in the new caching and deferred-tail logic. No high-severity or security issues detected.
| } | ||
| msg.Content = b.String() | ||
| index := m.deferredTailIndex | ||
| _ = m.views[index].(message.Model).SetMessage(msg) |
There was a problem hiding this comment.
[medium] materializeDeferredTail discards the tea.Cmd returned by SetMessage, silently dropping image-load side-effects
SetMessage returns a tea.Cmd that triggers loadMarkdownImageReferences for any markdown images in the message content. In materializeDeferredTail, this return value is explicitly discarded with _ = m.views[index].(message.Model).SetMessage(msg).
The deferred tail exists precisely because the user scrolled away during streaming — so the final chunks (which may introduce new image references) never went through AppendContent's image scanning path. When the tail is materialized, SetMessage may produce a non-nil image-load command, but that command is dropped. Any inline images that appear only in the deferred portion of the response will remain unresolved.
Suggested fix: capture and return the cmd from materializeDeferredTail, then include it in the command batch at each call site (e.g. materializeDeferredTailForInteraction / FinalizeStream).
| Confidence | Score |
|---|---|
| 🟡 moderate | 67/100 |
| // Stream completion is an exact-content boundary even when the viewport is | ||
| // scrolled above the active response. Reconcile buffered chunks before any | ||
| // queue/export/copy consumer can observe the final message. | ||
| p.messages.FinalizeStream() |
There was a problem hiding this comment.
[medium] FinalizeStream called unconditionally before the sub-agent depth check in handleStreamStopped
p.messages.FinalizeStream() is called at the very top of handleStreamStopped, before p.streamDepth is inspected. FinalizeStream materializes the deferred tail and resets deferredTailIndex to -1.
In a sub-agent delegation, when a sub-agent's StreamStopped arrives while the parent stream is still running (streamDepth > 1 before the decrement), the parent's deferred tail — accumulated because the user has scrolled — is flushed prematurely. After deferredTailIndex is reset to -1, subsequent chunks from the parent stream that arrive while the user remains scrolled away are re-deferred into a fresh deferredTail slice. The chunks that landed before the premature flush were already folded into the message, so the content is not lost, but the flush triggers ScrollToBottom at the top of the streamDepth > 0 return path, forcing an unwanted scroll-to-bottom on the still-active parent response.
Consider guarding FinalizeStream() on p.streamDepth <= 1 (i.e., only finalize for the outermost stopped stream), matching the pattern used for the rest of the cleanup logic below.
| Confidence | Score |
|---|---|
| 🟡 moderate | 57/100 |
| prefix = mv.senderPrefix(msg.Sender) | ||
| } | ||
| headerKey := prefix + header | ||
| if cache.headerKey != headerKey || cache.width != width { |
There was a problem hiding this comment.
[medium] Header cache not invalidated on terminal width change when headerKey is unchanged
In RenderedSegments, the width-change guard at line 377 resets the stable-line cache AND immediately writes the new width into cache.width:
if cache.width != width || !strings.HasPrefix(parts.StablePrefix, cache.stable) {
cache.width, cache.stable = width, "" // cache.width updated HERE
cache.stableLines = nil
}Then the header-line invalidation check at line 395 is:
if cache.headerKey != headerKey || cache.width != width {Because cache.width was just set to width in the block above, the cache.width != width sub-condition is always false after a width change. Header lines are only re-rendered when headerKey also changes. If the sender name and hover state are unchanged, headerKey stays the same, and the cached header lines (rendered at the old width) are served at the new terminal width.
Header lines include the sender prefix styled via lipgloss, which is width-sensitive. A width change without a re-render of headers produces misaligned or truncated header text until the next hover or action change forces a headerKey update.
Suggested fix: add an explicit width check for the header cache, separate from the headerKey check:
if cache.headerKey != headerKey || cache.headerWidth != width {
cache.headerKey = headerKey
cache.headerWidth = width
// rebuild header lines...
}Alternatively, reset cache.headerKey = "" inside the width-change block at line 377–380 so the subsequent check re-fires unconditionally.
| Confidence | Score |
|---|---|
| 🟡 moderate | 67/100 |
| return "" | ||
| } | ||
| m.syncScrollbar() | ||
| if m.NeedsScrollbar() && len(visibleLines) < m.height { |
There was a problem hiding this comment.
[medium] ViewWithPaddedLines only pads to viewport height for NeedsScrollbar, skipping the reserveScrollbarSpace case
ViewWithPaddedLines pads the input slice to m.height only when NeedsScrollbar() is true:
if m.NeedsScrollbar() && len(visibleLines) < m.height {
result := make([]string, m.height)
copy(result, visibleLines)
visibleLines = result
}
return m.composePadded(visibleLines)composePadded's reserveScrollbarSpace branch iterates over whatever length lines has — it does not pad to m.height. When reserveScrollbarSpace is true but NeedsScrollbar() is false (content is shorter than the viewport), ViewWithPaddedLines passes the unpadded, shorter slice to composePadded, which renders fewer rows than the viewport height. This breaks the fixed-height contract the callers rely on and can cause the layout below the message list to shift upward.
The parallel viewWithLines method does not have this gap — it pads in both scrollbar and non-scrollbar cases identically. ViewWithPaddedLines should mirror that behavior:
| if m.NeedsScrollbar() && len(visibleLines) < m.height { | |
| if (m.NeedsScrollbar() || m.reserveScrollbarSpace) && len(visibleLines) < m.height { | |
| result := make([]string, m.height) | |
| copy(result, visibleLines) | |
| visibleLines = result | |
| } |
| Confidence | Score |
|---|---|
| 🟡 moderate | 67/100 |
442f4af to
7c4857b
Compare
Improves cpu usage during streaming of long responses by caching better and not re-rendering the entire response on each new chunk that arrives.
Videos below show an example of the before and after using the same thread
2/3 of the diff are tests in a separate commit
Before
main@2a70168d4
initial ram usage seems lower, but rises and bounces around during streaming
cpu usage rises as the response continues, peaking above 50% in this example
Screencast.From.2026-08-22.20-24-47.webm
After
initial ram usage is higher, but remains mostly flat or goes down a tiny bit during streaming. final usage is not meaningfully higher
cpu usage remains essentially flat around 9-12% in this example for the entire duration of the response
Screencast.From.2026-08-22.20-21-38.webm