fix(tui): first frame after detach is never empty and never O(visible rows) (#1753) - #1774
Open
asheshgoplani wants to merge 2 commits into
Open
fix(tui): first frame after detach is never empty and never O(visible rows) (#1753)#1774asheshgoplani wants to merge 2 commits into
asheshgoplani wants to merge 2 commits into
Conversation
…isible rows) (#1753) Two residual causes of the post-Ctrl+Q black screen at large fleet sizes, both surviving #1764: EMPTY first frame - remoteAttachCmd and remoteCreateAndAttachCmd cleared isAttaching only in the ExecCallback, which Bubble Tea runs on its own goroutine after the loop resumes — the first View() raced it and rendered "". Both now clear via onExit inside Run(), while the loop is still parked, like attachCmd and attachWindowCmd. - the double-click attach site pre-set isAttaching before attachSession, which can return nil (no tmux session) — the flag then stayed true and View() was suppressed forever. The pre-set is gone; attachSession owns the flag. - belt: View() now re-serves the last rendered frame instead of "" while isAttaching is set, so even a path that reaches the first post-resume View with the flag still set shows the list, not black. While the renderer is live the frame is identical to the screen, so nothing is written (Bubble Tea #431 stays fixed). BLOCKED first frame (the felt ~1s on a ~57-session deck, reproducing with a large group expanded and vanishing when collapsed) - per-row labels (GetAutoName x2 + GetAutoNameDescription) took Instance.mu per visible row while background UpdateStatus holds that mutex as a writer across tmux subprocess calls (Exists probe 2s cap, DetectTool capture 3s cap); Go's RWMutex queues new readers behind a waiting writer, so the first frame stalled for seconds, scaling with visible row count. Labels now ride the lock-free render snapshot (title/autoName/autoNameDesc), for the overview rows and the session switcher both. - processStatusUpdate refreshed EVERY visible row per pass; with a big group expanded that is an unbudgeted burst of Instance write locks. Visible rows now cycle a dedicated round-robin cursor with a fixed per-pass budget, idle rows with an unchanged cached-activity fingerprint cost zero, and the off-screen batch is explicitly off-screen-only. - group expand is a first-class trigger: newly visible rows render instantly from the snapshot and a budgeted refresh request is queued — never a synchronous whole-group burst. Collapse queues nothing. Tests: one first-View-non-empty regression per attach flavor (session, window, remote attach, remote create-and-attach, reloading branch), the stuck-flag route, the View fallback, a source-level guard that every tea.Exec site wires onExit and that renderSessionItem stays lock-free, plus budget/cycling/expand-trigger coverage for processStatusUpdate. Refs #1753. Coordinates with #1765 (refresh architecture v2): this change is confined to processStatusUpdate's batching and the render snapshot; the sweep redesign in #1765 supersedes neither the flag wiring nor the lock-free labels, and the visible-budget round-robin here is the incremental form of the budget that design calls for. Committed by Ashesh Goplani
|
👋 Thanks for opening this — a couple of intake fields would help the maintainer's validation pipeline (and any reviewer) understand it faster. Nothing here blocks you and nothing is closing: this is a heads-up, and editing the description re-runs this check automatically.
The full field-level contract is in .github/INTAKE.md. AI-authored PRs are welcome here with equal standing. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe UI renders session labels from snapshots, budgets visible-row status refreshes after group expansion, preserves the last frame during attach, and clears attachment state across local and remote command paths. Regression tests cover rendering, refresh budgeting, and attach failures. ChangesUI rendering and attach flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Home
participant TeaExec
participant AttachCommand
participant View
Home->>TeaExec: execute attach command with onExit
TeaExec->>AttachCommand: run or exit command
AttachCommand->>Home: clear isAttaching via onExit
Home->>View: render during attach
View-->>Home: return lastRenderedFrame
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 7✅ Passed checks (7 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
…e source guard The guard scans the function body for banned Instance.mu getters with a plain substring match; a comment explaining the folded auto-name guard still spelled the old call out verbatim. Reword the comment — the guard stays strict. Committed by Ashesh Goplani
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/ui/home.go (1)
4015-4024: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winUse a thread-safe Title accessor in the render snapshot path.
refreshSessionRenderSnapshotcopies shared instances while only holdingh.instancesMu;Instance.Titleis then mutated withouti.muby title syncs,SetField(FieldTitle), and title-reapplied pending changes. Readinginst.Titledirectly creates a race with those writers, just as the neighboringautoName/autoNameDescfields were fixed by using_ThreadSafegetters. Useinst.GetTitleThreadSafe()in bothrefreshSessionRenderSnapshotand the fallback snapshot.🤖 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/ui/home.go` around lines 4015 - 4024, The render snapshot currently reads inst.Title without synchronization, racing with title updates. In refreshSessionRenderSnapshot and its fallback snapshot, replace direct Title access with inst.GetTitleThreadSafe(), matching the existing thread-safe accessors for autoName and autoNameDesc.
🤖 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.
Inline comments:
In `@internal/ui/issue1753_blackscreen_test.go`:
- Around line 329-386: Add an explicit comment or t.Skip-style note to
TestIssue1753_VisibleRowRefreshIsBudgeted,
TestIssue1753_GroupExpandIsBudgetedTrigger, and
TestIssue1753_RowLabelsRenderLockFree stating that RemoteSessionInfo rows are
polled through h.remoteSessions and are intentionally out of scope for these
local session tests, matching family 1’s documented remote-coverage precedent.
---
Outside diff comments:
In `@internal/ui/home.go`:
- Around line 4015-4024: The render snapshot currently reads inst.Title without
synchronization, racing with title updates. In refreshSessionRenderSnapshot and
its fallback snapshot, replace direct Title access with
inst.GetTitleThreadSafe(), matching the existing thread-safe accessors for
autoName and autoNameDesc.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d0de7139-3455-4cbe-ac96-b14a95f1f8a3
📒 Files selected for processing (3)
internal/ui/home.gointernal/ui/issue1753_blackscreen_test.gointernal/ui/session_switcher.go
Comment on lines
+329
to
+386
| // TestIssue1753_VisibleRowRefreshIsBudgeted: with a large group expanded the | ||
| // visible set approaches fleet size, so "refresh every visible row per pass" | ||
| // degenerates into the very burst the off-screen batching exists to prevent — | ||
| // each UpdateStatus takes the Instance write lock, sometimes across tmux | ||
| // subprocess calls. Visible rows must be budgeted round-robin instead. | ||
| func TestIssue1753_VisibleRowRefreshIsBudgeted(t *testing.T) { | ||
| const fleet = 30 | ||
| instances := make([]*session.Instance, 0, fleet) | ||
| ids := make([]string, 0, fleet) | ||
| for i := 0; i < fleet; i++ { | ||
| // StatusRunning with no tmux session: UpdateStatus flips it to a | ||
| // terminated status, so "status changed" counts UpdateStatus calls. | ||
| inst := &session.Instance{ | ||
| ID: fmt.Sprintf("v-%d", i), | ||
| Title: fmt.Sprintf("v-%d", i), | ||
| Tool: "shell", | ||
| Status: session.StatusRunning, | ||
| } | ||
| instances = append(instances, inst) | ||
| ids = append(ids, inst.ID) | ||
| } | ||
| h := newTestHomeWithItems(200, 50, nil) | ||
| h.instances = instances | ||
|
|
||
| req := statusUpdateRequest{viewOffset: 0, visibleHeight: fleet, flatItemIDs: ids} | ||
| h.processStatusUpdate(req) | ||
|
|
||
| changed := 0 | ||
| for _, inst := range instances { | ||
| if inst.GetStatusThreadSafe() != session.StatusRunning { | ||
| changed++ | ||
| } | ||
| } | ||
| // visibleStatusBatchSize(4) + the off-screen batch(2) is the absolute upper | ||
| // bound of UpdateStatus calls in one pass; all 30 rows are visible here so | ||
| // the off-screen batch has nothing to do. | ||
| if changed > 6 { | ||
| t.Fatalf("one processStatusUpdate pass refreshed %d of %d visible rows: the visible-row "+ | ||
| "burst is back — with a large group expanded this is an unbudgeted storm of "+ | ||
| "Instance write locks right when the user expects a frame (#1753)", changed, fleet) | ||
| } | ||
| if changed == 0 { | ||
| t.Fatal("processStatusUpdate refreshed no visible rows at all: the budget must " + | ||
| "amortize, not starve (#1753)") | ||
| } | ||
|
|
||
| // The round-robin must CYCLE: repeated passes eventually cover every row. | ||
| // 12 passes x 4-row budget > 30 rows even with slack for skips. | ||
| for pass := 0; pass < 12; pass++ { | ||
| h.processStatusUpdate(req) | ||
| } | ||
| for _, inst := range instances { | ||
| if inst.GetStatusThreadSafe() == session.StatusRunning { | ||
| t.Fatalf("row %s never refreshed across repeated passes: the visible round-robin "+ | ||
| "does not cycle (#1753)", inst.ID) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Family 2 regression tests don't address RemoteSession per path instructions.
TestIssue1753_VisibleRowRefreshIsBudgeted and TestIssue1753_GroupExpandIsBudgetedTrigger (and TestIssue1753_RowLabelsRenderLockFree above) exercise only local *session.Instance sessions. Visible-row refresh and group-expand triggering are plausibly inapplicable to RemoteSessionInfo rows (they're polled through a separate h.remoteSessions mechanism), similar to the established fork-dispatch precedent, but that's currently implicit rather than documented.
Add a short comment or t.Skip-style note explaining why RemoteSession is out of scope for these specific tests, consistent with family 1's explicit remote coverage.
As per path instructions: "When a PR changes TUI behavior in internal/ui/ or cmd/agent-deck/, verify it also handles RemoteSession or include an explicit t.Skip documenting why."
Also applies to: 391-446
🤖 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/ui/issue1753_blackscreen_test.go` around lines 329 - 386, Add an
explicit comment or t.Skip-style note to
TestIssue1753_VisibleRowRefreshIsBudgeted,
TestIssue1753_GroupExpandIsBudgetedTrigger, and
TestIssue1753_RowLabelsRenderLockFree stating that RemoteSessionInfo rows are
polled through h.remoteSessions and are intentionally out of scope for these
local session tests, matching family 1’s documented remote-coverage precedent.
Source: Path instructions
10 tasks
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.
What problem does this solve?
On the maintainer's live fleet (~57 agent sessions, two TUIs), Ctrl+Q on main tip (post-#1764) still showed ~1s of black screen before the list reappeared. The key diagnostic on #1753: it reproduces when a large group is expanded and goes away when it is collapsed.
Root causes (enumerated)
A. Routes where the first View() after
tea.Execreturns could be EMPTY (View()returns""whileisAttachingis set):attachCmd)Run()via onExit (#1764) ✅attachWindowCmd)attachCmdvia E)remoteAttachCmd)""Run()remoteCreateAndAttachCmd)Run()isAttachingbeforeattachSession, which can return nil → flag stuck true → permanent black screenattachSessionowns the flagstatusUpdateMsgView()re-serves the last rendered frame instead of""— identical content while the renderer is live, so nothing is written and Bubble Tea #431 stays fixedB. Routes where the first frame is BLOCKED (the felt ~1s, scaling with visible rows):
renderSessionItemreadGetAutoName()(twice) +GetAutoNameDescription()per visible row — each anInstance.muRLock — while backgroundUpdateStatusholds that mutex as a writer across tmux subprocess calls (Existsprobe 2s cap at instance.go:4213/4246,DetectTool→capture 3s cap at :4574,show-environmentat :4621). Go's RWMutex queues new readers behind a waiting writer, so one mid-sweep session stalls the whole frame; more visible rows ⇒ more chances to hit a locked one. Exactly why an expanded group reproduces it and two TUIs (double sweep pressure, slower tmux server) make it worse. Row labels now ride the lock-free render snapshot; the session switcher rows too.processStatusUpdaterefreshed every visible row per pass. With a large group expanded, visible ≈ fleet, so the visible-fast/offscreen-skip split degenerated into an unbudgeted storm of Instance write locks (the new requirement recorded on perf(tui): full-screen MaxWidth pass makes View() 51% of CPU — laggy switching with ~55 sessions #1753). Visible rows now cycle a dedicated round-robin cursor with a fixed budget (4/pass), idle rows with an unchanged cached-activity fingerprint cost zero, and the off-screen batch is explicitly off-screen-only.Measurements (sandbox)
Isolated HOME/profile/tmux socket, 60 synthetic
--cmd sleepsessions, TUI driven inside the isolated tmux at 220x50, frames sampled viacapture-paneand classified ATTACHED/BLANK/TUI:As with #1764, the sandbox's idle
sleeppanes cannot reproduce the live fleet's contention (their UpdateStatus never reaches the under-lock subprocess paths, so pre-fix blank is already small); the sandbox proves the fix regresses nothing and that blank stays bounded at ~1 renderer frame (~10ms — the alt-screen erase→first-flush gap, the floor of this architecture). The lock-contention leg is pinned by code-path tests and source guards instead: an empty first frame is now structurally unreachable, and no keypress→first-frame path scales with visible rows.Tests (all fail without their fix)
attachSessionnil-return) routetea.Execsite wiresonExit;Run()of all four command types defers it;renderSessionItemnever touchesInstance.mugettersprocessStatusUpdate: visible budget upper bound, round-robin cycling coverage, group-expand queues a budgeted request / collapse does not, no synchronous refresh on expandCoordination with #1765
Deliberately non-conflicting: this PR touches
processStatusUpdatebatching, the render snapshot fields, and attach-flag wiring — not the sweep architecture. The visible-row budget here is the incremental form of the per-tick budget the refresh-architecture-v2 design calls for; when #1765 lands, its event-driven sweep composes with (and can subsume) Step 1's budget, while the flag wiring and lock-free labels remain necessary regardless.Refs #1753.
Summary by CodeRabbit
Bug Fixes
Performance
Tests