fix(tui): cache settled alt-screen transcript - #647
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes the alt-screen flush frontier behavior and introduces a settled-prefix cache so Bubble Tea’s fullscreen (alt-screen) TUI no longer rebuilds transcript body descriptors and cross-row context for the entire history on every redraw. It advances m.flushed in alt-screen mode (while still skipping tea.Println) and reuses cached settled transcript items while keeping the live tail dynamic for hover/selection and streaming state.
Changes:
- Advance the flush frontier in alt-screen mode and cache the settled transcript prefix as
[]transcriptBodyItemto avoid per-frame O(history) rebuilds. - Build
rowContextonly from the unflushed tail and preserve spacing context viaflushedPreviousKind/flushedHavePreviousKind. - Add regression tests and a benchmark for the settled alt-screen path, plus a small allocation avoidance in
buildRowContextfor the frontier-at-tail steady state.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| internal/tui/transcript_selection.go | Adds settled-item reuse path for alt-screen, plus interaction state plumbing so cached item render closures reflect current hover/selection without rebuilding descriptors. |
| internal/tui/transcript_issue561_bench_test.go | Adds a regression benchmark to measure settled alt-screen transcript body item construction at large transcript sizes. |
| internal/tui/rendering.go | Avoids per-frame map allocations by returning an empty rowContext when called with an empty tail slice. |
| internal/tui/model.go | Extends model state to track flush spacing context and store alt-screen settled cache metadata and shared interaction pointer. |
| internal/tui/flush.go | Removes the alt-screen early return; advances the frontier in alt-screen while skipping printing, caches settled prefix items, and narrows buildRowContext to the tail. |
| internal/tui/flush_test.go | Updates alt-screen flush behavior expectations and adds a regression test ensuring settled cache rebuild after expand/collapse toggles. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughAlt-screen transcript settlement now advances the flush frontier, skips native scrollback emission, and caches settled body items. Rendering reuses cached prefixes and incremental context, with invalidation for settled-row changes. Tests and a benchmark cover these paths. ChangesAlt-screen transcript rendering
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Model
participant settleTranscript
participant rebuildAltScreenSettledItems
participant transcriptBodyItems
participant AltScreenView
Model->>settleTranscript: settle transcript rows
settleTranscript->>settleTranscript: advance flushed frontier
settleTranscript->>rebuildAltScreenSettledItems: rebuild settled prefix
rebuildAltScreenSettledItems->>transcriptBodyItems: build cached body items
AltScreenView->>transcriptBodyItems: render transcript
transcriptBodyItems-->>AltScreenView: reuse settled items and build tail
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/tui/transcript_issue561_bench_test.go (1)
21-29: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: guard against a silent cache miss so a future regression doesn't quietly turn this into an O(history) benchmark. A cheap pre-check (e.g. assert
m.altScreenSettledFrontier == m.flushed && m.altScreenSettledWidth == widthbefore the loop) keeps the benchmark measuring the intended fast path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tui/transcript_issue561_bench_test.go` around lines 21 - 29, Ensure BenchmarkIssue561SettledAltScreen verifies the settled cache state before timing: assert that m.altScreenSettledFrontier equals m.flushed and m.altScreenSettledWidth equals width after setup and before the benchmark loop, so cache misses cannot silently change the benchmark to O(history).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/tui/transcript_issue561_bench_test.go`:
- Around line 21-29: Ensure BenchmarkIssue561SettledAltScreen verifies the
settled cache state before timing: assert that m.altScreenSettledFrontier equals
m.flushed and m.altScreenSettledWidth equals width after setup and before the
benchmark loop, so cache misses cannot silently change the benchmark to
O(history).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7bb8825d-3566-40bc-8e2b-74ddc6b163df
📒 Files selected for processing (6)
internal/tui/flush.gointernal/tui/flush_test.gointernal/tui/model.gointernal/tui/rendering.gointernal/tui/transcript_issue561_bench_test.gointernal/tui/transcript_selection.go
There was a problem hiding this comment.
I built and vetted this and read all six changed files, and the caching design is sound invalidation covers new turns, resize, and expand/collapse of settled rows, and the steady-state fast path correctly bypasses the cache whenever a row is still live. The benchmark matches the stated goal.
But I found a regression that I think has to be fixed before this lands: in alt-screen mode the doctor connectivity spinner freezes.
setDoctorStatusRow (command_center.go:212-222) updates an existing status row in place with m.transcript[i] = row and returns. A rowSystem row is immediately settleable (flush.go:61-64), so once the frontier advances past the doctor row it gets snapshotted into altScreenSettledItems. On the next spinner tick (model.go:1986) doctorFrame changes and setDoctorStatusRow writes new text into m.transcript[i] but it never zeroes altScreenSettledWidth, and the rebuild guard in settleTranscript (flush.go:128) only fires on a frontier/width change. With none of those changing on an in-place text update, the rebuild is skipped and the View fast path (transcript_selection.go:249) serves the stale snapshot. I reproduced it: advancing doctorFrame 0 → 2 still renders the frame-0 glyph. The same repro passes on origin/main, where alt-screen re-rendered the tail every frame.
Fix is small: have setDoctorStatusRow invalidate the settled cache when it updates an existing row (zero altScreenSettledWidth on the in-place path, same as toggleTranscriptRow does). Worth checking other in-place mutators of already-settled rows for the same gap while you're there.
Everything else I'd merge as-is. Requesting changes just for this.
|
@gnanam1990 could you take a look here? I requested changes for a regression: in alt-screen mode the doctor connectivity spinner freezes, because setDoctorStatusRow updates a settled row in place without invalidating the settled-prefix cache, so the View fast path serves a stale snapshot. Would be good to have you confirm (or push back) before it's reworked. |
…updates setDoctorStatusRow, setSandboxSetupStatusRow, and setCompactStatusRow all mutate an already-flushed transcript row in place (m.transcript[i] = row) without invalidating the alt-screen settled cache. Since none of frontier, width, or row count change on such an update, settleTranscript's rebuild guard never fires and the View fast path serves the stale cached snapshot - e.g. the doctor connectivity spinner freezes on its first frame. Zero altScreenSettledWidth on the in-place path, mirroring the existing pattern in toggleTranscriptRow, so the next settle rebuilds the cache. Addresses PR Gitlawb#647 review feedback from Vasanthdev2004.
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
The new commit closes the doctor/sandbox/compact spinner gap I flagged earlier, and all four in-place row mutators are covered now. But the invalidation surface is still incomplete — two more paths change settled-row rendering without advancing the frontier, so the cached prefix goes stale. Both are regressions: before this PR, alt-screen re-rendered the whole transcript every frame, so these were always fresh.
First, the FILES selection tint. selectFile (files_panel.go:354) sets m.selectedFile and scrolls to the edit card, but doesn't zero altScreenSettledWidth; the accent border is read from selectedFile (rendering.go:1412 via rowTouchesSelectedFile) and baked into the cached descriptor at rebuild time, so the first click on a FILES row (transcript_selection.go:1394) scrolls to the card but its border doesn't highlight until a resize or new turn. The Esc-clear at model.go:1373 has the same gap in reverse (a cached accent border won't clear). Fix: invalidate the cache in those paths, same if i < m.flushed { m.altScreenSettledWidth = 0 } style as the status-row setters.
Second, collapseRepeatedStatusCard (model.go:2440) drops two rows from the middle of the settled prefix without adjusting m.flushed or invalidating. After a repeated identical swarm_status, the dropped pair was already settled (frontier had stopped at the live new-call), so m.flushed still equals the new transcript length, the fast path fires, and the cache serves the old call+result cards while hiding the new ones — the collapse is defeated. Fix: when the collapse removes rows, decrement m.flushed by the count removed and zero altScreenSettledWidth.
The caching design and the 0-alloc benchmark are solid; it's just the invalidation surface that needs these two paths closed. Requesting changes for those.
|
@Vasanthdev2004 The two remaining cache invalidation issues are fixed in f0f80b6, with regression coverage for FILES selection clearing and repeated status-card collapse. Could you please re-review when you have a chance? |
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Approving — both invalidation gaps are closed. The setSelectedFile helper covers select, the Esc-clear, and the mouse path in one place, and rewinding m.flushed by the removed row count in the collapse path is exactly the fix I had in mind. The two regression tests plus the cache-miss guard in the benchmark are a nice touch.
One non-blocking note for a possible follow-up: setSelectedFile keys the invalidation on lastRowIndexForFile, but rowTouchesSelectedFile tints every settled result row that touched the file — so if a file's most recent edit card is still in the live tail while an older card for the same file is already settled, the older card's tint can go stale until the next rebuild. It's narrow and cosmetic (self-heals on the next frontier advance or resize), so I'm not holding the PR on it.
Thanks for the quick turnarounds on both rounds.
Summary
tea.PrintlnRoot cause
Alt-screen mode returned from
settleTranscriptbefore advancingm.flushed. Since the shipped TUI always uses alt-screen mode, every redraw rebuilt transcript body descriptors and cross-row context for the complete conversation.Impact
Settled alt-screen frames now reuse cached body descriptors while the live tail remains dynamic. On the issue benchmark, a settled 5,000-turn transcript takes about 110 ns/op with 0 B/op and 0 allocations/op on Windows/amd64.
Validation
go test ./... -run '^$'go test ./internal/tui -run '^$' -bench '^BenchmarkIssue561SettledAltScreen$' -benchmem -benchtime=100xCloses #561
Summary by CodeRabbit