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.
WalkthroughAlt-screen transcript settlement now advances the flush frontier, skips native scrollback emission, and caches settled body items. Rendering reuses the cache and incremental row context, with invalidation for row toggles and coverage through tests and benchmarks. ChangesAlt-screen transcript rendering
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant settleTranscript
participant rebuildAltScreenSettledItems
participant transcriptBodyItems
settleTranscript->>rebuildAltScreenSettledItems: advance flushed frontier
rebuildAltScreenSettledItems->>transcriptBodyItems: rebuild settled prefix descriptors
transcriptBodyItems-->>rebuildAltScreenSettledItems: settled body items
rebuildAltScreenSettledItems-->>settleTranscript: store width and frontier cache
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. |
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
Bug Fixes
Performance