From a9210fb9fdfddbd13a9d940e174201f146f5deb6 Mon Sep 17 00:00:00 2001 From: Peyton Montei Date: Mon, 11 May 2026 18:13:18 -0400 Subject: [PATCH 01/11] feat(review): drive drill-in detail with viewport, wrap long events MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The drill-in alt-screen (Ctrl+O) previously rendered each agent event as a single terminal-width-truncated line, with manual scroll-position math driving which window of events was visible. Long multi-paragraph AssistantText findings were unreadable. This change reworks the drill-in around bubbles/v2/viewport so: - A new wrapDisplayWidth helper in tui_text.go honors embedded '\n' as paragraph boundaries and wraps each paragraph independently with ansi.Wrap, preserving multi-paragraph review findings instead of collapsing newlines to spaces. - eventLine is replaced by eventLines, returning []string (wrapped). All existing event types (Started/AssistantText/ToolCall/Tokens/Finished/ RunError) still render; AssistantText is the only one that fans out to multiple lines on embedded newlines. - detailView is replaced by detailFrame, a pure-function chrome (header + body + footer) that wraps an already-rendered viewport body and pads to exactly termHeight lines so the alt-screen frame diff doesn't leave ghost rows. - reviewTUIModel gets a viewport.Model field initialized with sane defaults (80x22) so Ctrl+O before any WindowSizeMsg still renders. WindowSizeMsg, Ctrl+O entry, and ←/→ agent-cycling all resize and re-fill the viewport via refreshDetailContent. - Unhandled keys in detail mode are delegated to viewport.Update so PgUp/PgDn/Home/End/↑/↓/mouse wheel reach its internal keymap. - Auto-tail behavior is preserved: refreshDetailContent samples AtBottom() before SetContentLines and re-issues GotoBottom() if the user was tailing, so a scrolled-up user is not yanked back to the bottom by incoming events. Removed (now dead): - detailScroll field on reviewTUIModel - maxDetailScroll() and clampScroll() helpers - buildBodyLines (replaced by the viewport's own visible-line math via SetContentLines) - Old eventLine and detailView (renamed to eventLines and detailFrame) The footer hint shifts from "↑/↓ scroll" to "PgUp/PgDn scroll" to match what reaches the viewport via key delegation. Test changes: - Existing detailView_* tests are rewritten as detailFrame_* tests that drive rendering through the model (newReviewTUIModel + WindowSizeMsg + agentEventMsg + Ctrl+O + View()), since the viewport is now the source of truth for body lines. - Auto-follow tests now assert on viewport.YOffset() / AtBottom() instead of detailScroll. - New TestDetailFrame_WrapsLongAssistantText (load-bearing) pins that a ≥200-char AssistantText on a 40-wide terminal wraps to multiple visible body lines, each fitting termWidth. - New TestDetailFrame_PreservesAssistantTextNewlines pins multi- paragraph text survives intact. - New TestTUIModel_InitializesDetailViewport pins non-zero default viewport dimensions. - New TestTUIModel_DelegatesScrollKeysToViewport pins that PgDn in detail mode advances viewport YOffset (key delegation works). Co-Authored-By: Claude Opus 4.7 (1M context) Entire-Checkpoint: 4e31dbdd1b05 --- cmd/entire/cli/review/tui_detail.go | 126 +++++++------- cmd/entire/cli/review/tui_detail_test.go | 211 +++++++++++++---------- cmd/entire/cli/review/tui_model.go | 147 ++++++++++------ cmd/entire/cli/review/tui_model_test.go | 130 ++++++++------ cmd/entire/cli/review/tui_text.go | 25 +++ 5 files changed, 370 insertions(+), 269 deletions(-) diff --git a/cmd/entire/cli/review/tui_detail.go b/cmd/entire/cli/review/tui_detail.go index 1aab2318e7..04e3cfa799 100644 --- a/cmd/entire/cli/review/tui_detail.go +++ b/cmd/entire/cli/review/tui_detail.go @@ -1,10 +1,11 @@ // Package review — see env.go for package-level rationale. // -// tui_detail.go provides detailView, the pure-function renderer for the -// alt-screen drill-in view. It renders one agent's live event buffer with -// header/footer chrome and pads to exactly termHeight lines so every frame -// has the same line count (avoids ghost rows in the Bubble Tea alt-screen -// frame diff). +// tui_detail.go provides the alt-screen drill-in renderer. The body content is +// produced by [eventLines] (one or more wrapped lines per event) and fed into +// a bubbles/v2/viewport on [reviewTUIModel]; this file's [detailFrame] is the +// pure-function chrome (header + body + footer) that wraps the viewport's +// pre-rendered output and pads to exactly termHeight lines so every frame has +// the same line count (avoids ghost rows in Bubble Tea's alt-screen diff). package review import ( @@ -14,16 +15,22 @@ import ( reviewtypes "github.com/entireio/cli/cmd/entire/cli/review/types" ) -// eventLine converts a single Event to a single display line for the detail -// view. The line is stripped of control sequences and truncated to maxWidth -// display cells. -func eventLine(ev reviewtypes.Event, maxWidth int) string { +// eventLines converts a single Event to one or more display lines for the +// detail view, wrapped to maxWidth display cells. AssistantText preserves +// embedded '\n' as paragraph breaks; other event types render as a single +// sanitized line that wraps only on width overflow. +func eventLines(ev reviewtypes.Event, maxWidth int) []string { + if maxWidth <= 0 { + return nil + } var raw string switch e := ev.(type) { case reviewtypes.Started: raw = "[started]" case reviewtypes.AssistantText: - raw = e.Text + // AssistantText is the only event that can contain meaningful + // multi-line content; wrapDisplayWidth honors embedded newlines. + return wrapDisplayWidth(e.Text, maxWidth) case reviewtypes.ToolCall: raw = fmt.Sprintf("[tool: %s] %s", e.Name, e.Args) case reviewtypes.Tokens: @@ -44,22 +51,34 @@ func eventLine(ev reviewtypes.Event, maxWidth int) string { raw = "[unknown event]" } - return truncateDisplayWidth(sanitizeDisplayText(raw), maxWidth) + return wrapDisplayWidth(raw, maxWidth) } -// detailView renders the alt-screen drill-in for one agent. row is the agentRow -// being inspected. termWidth/termHeight come from WindowSizeMsg. +// buildEventLines returns every wrapped body line for the supplied event +// buffer, in order. The result is suitable for feeding into a viewport via +// SetContent. +func buildEventLines(buffer []reviewtypes.Event, maxWidth int) []string { + if len(buffer) == 0 || maxWidth <= 0 { + return nil + } + out := make([]string, 0, len(buffer)) + for _, ev := range buffer { + out = append(out, eventLines(ev, maxWidth)...) + } + return out +} + +// detailFrame renders the alt-screen drill-in chrome around a body string. The +// body is the viewport's already-rendered view (already clipped to bodyHeight +// lines by the viewport itself). detailFrame adds: // -// Rendering: // 1. Header line: "─── ( events) ─────────────" (filled to termWidth) -// 2. Body: events from row.buffer scrolled to detailScroll, one line each, -// sanitized and truncated to termWidth display cells. -// 3. Footer line: "←/→ switch agent · Esc back · ↑/↓ scroll" +// 2. Body: trimmed/padded to exactly bodyHeight lines, each padded to termWidth. +// 3. Footer line: "←/→ switch agent · Esc back · PgUp/PgDn scroll" // -// CRITICAL: the rendered string is padded to exactly termHeight lines so every -// frame has the same line count. Bubble Tea's alt-screen frame diff leaves ghost -// rows if the line count varies between frames. -func detailView(row agentRow, detailScroll, termWidth, termHeight int) string { +// CRITICAL: the rendered string is exactly termHeight lines. Bubble Tea's +// alt-screen diff leaves ghost rows if the line count varies between frames. +func detailFrame(row agentRow, body string, termWidth, termHeight int) string { if termWidth < 1 { termWidth = 80 } @@ -67,77 +86,50 @@ func detailView(row agentRow, detailScroll, termWidth, termHeight int) string { termHeight = 3 } - // Reserve 1 line for header, 1 for footer; the body fills the rest. bodyHeight := termHeight - 2 if bodyHeight < 0 { bodyHeight = 0 } - // 1. Header line. headerContent := fmt.Sprintf("─── %s (%d events) ", sanitizeDisplayText(row.name), len(row.buffer)) header := padDisplayWidthWith(headerContent, termWidth, "─") - // 2. Body lines. - lines := buildBodyLines(row.buffer, detailScroll, bodyHeight, termWidth) - - // Pad body to exactly bodyHeight lines. - for len(lines) < bodyHeight { - lines = append(lines, strings.Repeat(" ", termWidth)) - } + // Normalize the viewport body to exactly bodyHeight lines, each padded to + // termWidth so frame width is stable. + bodyLines := splitBodyToHeight(body, bodyHeight, termWidth) - // 3. Footer line. - footerText := "←/→ switch agent · Esc back · ↑/↓ scroll" + footerText := "←/→ switch agent · Esc back · PgUp/PgDn scroll" footer := padDisplayWidth(footerText, termWidth) - // Assemble: header + body + footer = termHeight lines total. var b strings.Builder b.WriteString(header) b.WriteString("\n") - for _, line := range lines { + for _, line := range bodyLines { b.WriteString(line) b.WriteString("\n") } b.WriteString(footer) - // No trailing newline after footer — the caller (View) adds its own. - return b.String() } -// buildBodyLines computes the visible body lines for the detail view. -// It takes the full event buffer, a scroll offset, the maximum number of lines -// to show, and the column width. Returns at most bodyHeight lines. -func buildBodyLines(buffer []reviewtypes.Event, scroll, bodyHeight, termWidth int) []string { - if len(buffer) == 0 || bodyHeight <= 0 { +// splitBodyToHeight normalizes a multi-line body string to exactly bodyHeight +// lines, each truncated and padded to termWidth. Lines beyond bodyHeight are +// dropped; missing lines are padded with spaces. +func splitBodyToHeight(body string, bodyHeight, termWidth int) []string { + if bodyHeight <= 0 { return nil } - - // Clamp scroll to valid range. - if scroll < 0 { - scroll = 0 - } - if scroll >= len(buffer) { - scroll = len(buffer) - 1 - } - - // Determine window: scroll is the index of the LAST visible line so the - // user sees the most-recent events when auto-scrolling. Work backwards. - end := scroll + 1 // exclusive upper bound - start := end - bodyHeight - if start < 0 { - start = 0 + var raw []string + if body != "" { + raw = strings.Split(strings.TrimRight(body, "\n"), "\n") } - // Clamp end to buffer length. - if end > len(buffer) { - end = len(buffer) - start = end - bodyHeight - if start < 0 { - start = 0 + lines := make([]string, 0, bodyHeight) + for i := range bodyHeight { + if i < len(raw) { + lines = append(lines, padDisplayWidth(raw[i], termWidth)) + } else { + lines = append(lines, strings.Repeat(" ", termWidth)) } } - - lines := make([]string, 0, end-start) - for i := start; i < end; i++ { - lines = append(lines, eventLine(buffer[i], termWidth)) - } return lines } diff --git a/cmd/entire/cli/review/tui_detail_test.go b/cmd/entire/cli/review/tui_detail_test.go index 0d5245bbb9..010ec3b5eb 100644 --- a/cmd/entire/cli/review/tui_detail_test.go +++ b/cmd/entire/cli/review/tui_detail_test.go @@ -7,6 +7,7 @@ import ( "testing" "unicode/utf8" + tea "charm.land/bubbletea/v2" "github.com/charmbracelet/x/ansi" reviewtypes "github.com/entireio/cli/cmd/entire/cli/review/types" @@ -18,29 +19,40 @@ func countLines(s string) int { if s == "" { return 0 } - // detailView ends without a trailing newline after footer; lines are - // separated by \n. Count \n occurrences + 1 (for the final segment). + // detailFrame ends without a trailing newline after footer; lines are + // separated by \n. Count \n occurrences + 1 (for the final segment). return strings.Count(s, "\n") + 1 } -func makeBuffer(texts ...string) []reviewtypes.Event { - buf := make([]reviewtypes.Event, len(texts)) - for i, t := range texts { - buf[i] = reviewtypes.AssistantText{Text: t} - } - return buf +// renderDetailViaModel builds a reviewTUIModel populated with the supplied +// events, sized to termWidth × termHeight, and returns the rendered detail +// frame. The viewport is the source of truth for body lines, so we exercise +// rendering through the model rather than calling detailFrame in isolation. +func renderDetailViaModel(t *testing.T, name string, buffer []reviewtypes.Event, termWidth, termHeight int) string { + t.Helper() + m := newReviewTUIModel([]string{name}, nil) + updated, _ := m.Update(tea.WindowSizeMsg{Width: termWidth, Height: termHeight}) + m = mustModel(t, updated) + for _, ev := range buffer { + updated, _ := m.Update(agentEventMsg{agent: name, ev: ev}) + m = mustModel(t, updated) + } + // Enter detail mode so refreshDetailContent runs. + updated, _ = m.Update(testCtrlKey('o')) + m = mustModel(t, updated) + return m.View().Content } -func TestDetailView_PadsToTermHeight(t *testing.T) { +func TestDetailFrame_PadsToTermHeight(t *testing.T) { t.Parallel() for _, termHeight := range []int{5, 10, 20, 24} { t.Run("", func(t *testing.T) { t.Parallel() - row := agentRow{ - name: "agent-a", - buffer: makeBuffer("line1", "line2"), - } - out := detailView(row, 0, 80, termHeight) + out := renderDetailViaModel(t, "agent-a", + []reviewtypes.Event{ + reviewtypes.AssistantText{Text: "line1"}, + reviewtypes.AssistantText{Text: "line2"}, + }, 80, termHeight) got := countLines(out) if got != termHeight { t.Errorf("termHeight=%d: expected %d lines, got %d\noutput:\n%s", @@ -50,24 +62,24 @@ func TestDetailView_PadsToTermHeight(t *testing.T) { } } -func TestDetailView_EmptyBuffer_PadsToTermHeight(t *testing.T) { +func TestDetailFrame_EmptyBuffer_PadsToTermHeight(t *testing.T) { t.Parallel() - row := agentRow{name: "agent-a", buffer: nil} termHeight := 10 - out := detailView(row, 0, 80, termHeight) + out := renderDetailViaModel(t, "agent-a", nil, 80, termHeight) got := countLines(out) if got != termHeight { t.Errorf("empty buffer: expected %d lines, got %d", termHeight, got) } } -func TestDetailView_HeaderContainsAgentNameAndCount(t *testing.T) { +func TestDetailFrame_HeaderContainsAgentNameAndCount(t *testing.T) { t.Parallel() - row := agentRow{ - name: "claude-code", - buffer: makeBuffer("a", "b", "c"), - } - out := detailView(row, 0, 80, 10) + out := renderDetailViaModel(t, "claude-code", + []reviewtypes.Event{ + reviewtypes.AssistantText{Text: "a"}, + reviewtypes.AssistantText{Text: "b"}, + reviewtypes.AssistantText{Text: "c"}, + }, 80, 10) firstLine := strings.SplitN(out, "\n", 2)[0] if !strings.Contains(firstLine, "claude-code") { t.Errorf("header missing agent name: %q", firstLine) @@ -80,10 +92,10 @@ func TestDetailView_HeaderContainsAgentNameAndCount(t *testing.T) { } } -func TestDetailView_FooterPresent(t *testing.T) { +func TestDetailFrame_FooterPresent(t *testing.T) { t.Parallel() - row := agentRow{name: "agent-a", buffer: makeBuffer("x")} - out := detailView(row, 0, 80, 8) + out := renderDetailViaModel(t, "agent-a", + []reviewtypes.Event{reviewtypes.AssistantText{Text: "x"}}, 80, 8) lines := strings.Split(out, "\n") lastLine := lines[len(lines)-1] if !strings.Contains(lastLine, "Esc back") { @@ -91,40 +103,18 @@ func TestDetailView_FooterPresent(t *testing.T) { } } -func TestDetailView_LineTruncation_RuneSafe(t *testing.T) { - t.Parallel() - // Use a multi-byte UTF-8 string: each '日' is 3 bytes but 1 rune. - multibyte := strings.Repeat("日", 20) // 20 runes, 60 bytes - row := agentRow{ - name: "agent-a", - buffer: []reviewtypes.Event{reviewtypes.AssistantText{Text: multibyte}}, - } - termWidth := 10 - out := detailView(row, 0, termWidth, 5) - // Each body line must not exceed termWidth runes. - for i, line := range strings.Split(out, "\n") { - runes := utf8.RuneCountInString(line) - if runes > termWidth { - t.Errorf("line %d has %d runes (>%d): %q", i, runes, termWidth, line) - } - } -} - -func TestDetailView_LinesFitTerminalWidth(t *testing.T) { +func TestDetailFrame_LinesFitTerminalWidth(t *testing.T) { t.Parallel() - row := agentRow{ - name: "claude-code-with-a-very-wide-name", - buffer: []reviewtypes.Event{ - reviewtypes.AssistantText{Text: strings.Repeat("界", 20)}, - reviewtypes.ToolCall{Name: "wide", Args: strings.Repeat("🚀", 20)}, - reviewtypes.RunError{Err: errors.New(strings.Repeat("日", 20))}, - }, + buffer := []reviewtypes.Event{ + reviewtypes.AssistantText{Text: strings.Repeat("界", 20)}, + reviewtypes.ToolCall{Name: "wide", Args: strings.Repeat("🚀", 20)}, + reviewtypes.RunError{Err: errors.New(strings.Repeat("日", 20))}, } - for _, width := range []int{1, 2, 5, 10, 20, 40, 80} { + for _, width := range []int{10, 20, 40, 80} { t.Run(fmt.Sprintf("width %d", width), func(t *testing.T) { t.Parallel() - out := detailView(row, 2, width, 8) + out := renderDetailViaModel(t, "claude-code-with-a-very-wide-name", buffer, width, 12) for i, line := range strings.Split(out, "\n") { if got := ansi.StringWidth(line); got > width { t.Fatalf("line %d width = %d, want <= %d:\n%q", i, got, width, line) @@ -134,15 +124,26 @@ func TestDetailView_LinesFitTerminalWidth(t *testing.T) { } } -func TestDetailView_ANSIStripped(t *testing.T) { +func TestDetailFrame_MultibyteRuneSafe(t *testing.T) { + t.Parallel() + multibyte := strings.Repeat("日", 20) // 20 runes, 60 bytes + termWidth := 10 + out := renderDetailViaModel(t, "agent-a", + []reviewtypes.Event{reviewtypes.AssistantText{Text: multibyte}}, termWidth, 8) + for i, line := range strings.Split(out, "\n") { + runes := utf8.RuneCountInString(line) + if runes > termWidth { + t.Errorf("line %d has %d runes (>%d): %q", i, runes, termWidth, line) + } + } +} + +func TestDetailFrame_ANSIStripped(t *testing.T) { t.Parallel() // Include CSI sequences that codex emits (cursor-hide / cursor-show). ansiText := "hello\x1b[?25lworld\x1b[?25h" - row := agentRow{ - name: "agent-a", - buffer: []reviewtypes.Event{reviewtypes.AssistantText{Text: ansiText}}, - } - out := detailView(row, 0, 80, 5) + out := renderDetailViaModel(t, "agent-a", + []reviewtypes.Event{reviewtypes.AssistantText{Text: ansiText}}, 80, 6) if strings.Contains(out, "\x1b") { t.Error("output should have ANSI sequences stripped") } @@ -151,46 +152,74 @@ func TestDetailView_ANSIStripped(t *testing.T) { } } -func TestDetailView_Scrolling_LeadingLinesHidden(t *testing.T) { +func TestDetailFrame_EventTypes_Rendered(t *testing.T) { t.Parallel() - // 5 events; termHeight=6 (1 header + 3 body + 1 footer = 5; we set 6 to leave 4 body lines). - texts := []string{"line0", "line1", "line2", "line3", "line4"} - row := agentRow{name: "agent-a", buffer: makeBuffer(texts...)} + buffer := []reviewtypes.Event{ + reviewtypes.Started{}, + reviewtypes.ToolCall{Name: "read_file", Args: "foo.go"}, + reviewtypes.Tokens{In: 100, Out: 50}, + reviewtypes.Finished{Success: true}, + reviewtypes.RunError{Err: errors.New("oops")}, + } + out := renderDetailViaModel(t, "agent-a", buffer, 120, 10) + checks := []string{"[started]", "[tool: read_file]", "in=100", "[finished: success]", "[error: oops]"} + for _, want := range checks { + if !strings.Contains(out, want) { + t.Errorf("expected %q in output; got:\n%s", want, out) + } + } +} - // scroll=4 (max): shows events 1-4 in body (if bodyHeight=4). - termHeight := 6 - out := detailView(row, 4, 80, termHeight) - if strings.Contains(out, "line0") { - t.Error("line0 should not appear when scrolled to the bottom with 4 body lines") +// TestDetailFrame_WrapsLongAssistantText is load-bearing: a long AssistantText +// must wrap across multiple visible body lines rather than being truncated. +// This is the whole point of switching the drill-in body to a viewport. +func TestDetailFrame_WrapsLongAssistantText(t *testing.T) { + t.Parallel() + // 200+ character AssistantText (space-separated tokens so word wrap can fire). + long := strings.TrimSpace(strings.Repeat("word ", 50)) // 50 words × 5 chars - trailing space ≈ 249 chars + if utf8.RuneCountInString(long) < 200 { + t.Fatalf("test setup: expected >= 200 runes, got %d", utf8.RuneCountInString(long)) } - if !strings.Contains(out, "line4") { - t.Errorf("line4 should appear at scroll=4; output:\n%s", out) + termWidth := 40 + termHeight := 20 // ample body height + out := renderDetailViaModel(t, "agent-a", + []reviewtypes.Event{reviewtypes.AssistantText{Text: long}}, termWidth, termHeight) + + // Every line fits within termWidth. + lines := strings.Split(out, "\n") + for i, line := range lines { + if got := ansi.StringWidth(line); got > termWidth { + t.Fatalf("line %d width = %d, want <= %d:\n%q", i, got, termWidth, line) + } } - // scroll=0: shows first bodyHeight events. - out0 := detailView(row, 0, 80, termHeight) - if !strings.Contains(out0, "line0") { - t.Errorf("line0 should appear at scroll=0; output:\n%s", out0) + // The body must contain more than one line of "word" content. A single + // truncated line would only show one fragment; wrapping should produce + // several body lines beyond the header. + wordLineCount := 0 + for _, line := range lines { + if strings.Contains(line, "word") { + wordLineCount++ + } + } + if wordLineCount < 4 { + t.Errorf("expected long AssistantText to wrap onto multiple visible lines (got %d body lines with 'word'); output:\n%s", + wordLineCount, out) } } -func TestDetailView_EventTypes_Rendered(t *testing.T) { +// TestDetailFrame_PreservesAssistantTextNewlines pins that embedded newlines in +// AssistantText act as paragraph breaks rather than being collapsed to spaces. +// Multi-paragraph review findings must remain readable after the wrap helper +// turns them into multiple body lines. +func TestDetailFrame_PreservesAssistantTextNewlines(t *testing.T) { t.Parallel() - row := agentRow{ - name: "agent-a", - buffer: []reviewtypes.Event{ - reviewtypes.Started{}, - reviewtypes.ToolCall{Name: "read_file", Args: "foo.go"}, - reviewtypes.Tokens{In: 100, Out: 50}, - reviewtypes.Finished{Success: true}, - reviewtypes.RunError{Err: errors.New("oops")}, - }, - } - out := detailView(row, 4, 120, 10) - checks := []string{"[started]", "[tool: read_file]", "in=100", "[finished: success]", "[error: oops]"} - for _, want := range checks { + text := "first paragraph here\nsecond paragraph here\nthird paragraph here" + out := renderDetailViaModel(t, "agent-a", + []reviewtypes.Event{reviewtypes.AssistantText{Text: text}}, 80, 12) + for _, want := range []string{"first paragraph here", "second paragraph here", "third paragraph here"} { if !strings.Contains(out, want) { - t.Errorf("expected %q in output; got:\n%s", want, out) + t.Errorf("expected paragraph %q to survive in detail output; got:\n%s", want, out) } } } diff --git a/cmd/entire/cli/review/tui_model.go b/cmd/entire/cli/review/tui_model.go index 3255beacd0..320e7170e8 100644 --- a/cmd/entire/cli/review/tui_model.go +++ b/cmd/entire/cli/review/tui_model.go @@ -15,6 +15,7 @@ import ( "time" "charm.land/bubbles/v2/spinner" + "charm.land/bubbles/v2/viewport" tea "charm.land/bubbletea/v2" "charm.land/lipgloss/v2" @@ -50,11 +51,14 @@ type tickMsg time.Time // reviewTUIModel is the Bubble Tea model for the review dashboard. type reviewTUIModel struct { - rows []agentRow - rowIdx map[string]int // agent name → row index (O(1) lookup) - detailMode bool - detailIdx int // which agent is shown in drill-in - detailScroll int + rows []agentRow + rowIdx map[string]int // agent name → row index (O(1) lookup) + detailMode bool + detailIdx int // which agent is shown in drill-in + // detail is the pager backing the drill-in body. Width/Height are kept + // in sync with termWidth/termHeight (minus header+footer). Scroll + // position is internal state; AtBottom drives auto-tail. + detail viewport.Model cancel context.CancelFunc cancelOnce *sync.Once @@ -84,14 +88,25 @@ func newReviewTUIModel(agents []string, cancel context.CancelFunc) reviewTUIMode } rowIdx[name] = i } + // Seed viewport with defaults that match termWidth/termHeight so an + // immediate Ctrl+O before any WindowSizeMsg still renders. + const ( + defaultTermWidth = 80 + defaultTermHeight = 24 + ) + vp := viewport.New( + viewport.WithWidth(defaultTermWidth), + viewport.WithHeight(defaultTermHeight-2), + ) return reviewTUIModel{ rows: rows, rowIdx: rowIdx, + detail: vp, cancel: cancel, cancelOnce: &sync.Once{}, spinner: sp, - termWidth: 80, - termHeight: 24, + termWidth: defaultTermWidth, + termHeight: defaultTermHeight, } } @@ -156,7 +171,9 @@ func (m reviewTUIModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) { case tea.WindowSizeMsg: m.termWidth = msg.Width m.termHeight = msg.Height - m = m.clampScroll() + m.detail.SetWidth(m.detailViewportWidth()) + m.detail.SetHeight(m.detailViewportHeight()) + m = m.refreshDetailContent() return m, nil case tea.KeyPressMsg: @@ -165,6 +182,47 @@ func (m reviewTUIModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) { return m, nil } +// detailViewportWidth returns the viewport's width, mirroring termWidth. +func (m reviewTUIModel) detailViewportWidth() int { + if m.termWidth < 1 { + return 1 + } + return m.termWidth +} + +// detailViewportHeight returns the viewport's body height, reserving one line +// for the header and one for the footer. +func (m reviewTUIModel) detailViewportHeight() int { + h := m.termHeight - 2 + if h < 1 { + return 1 + } + return h +} + +// refreshDetailContent re-renders the focused agent's events into the +// viewport. It preserves auto-tail: if the viewport was sitting at the bottom +// (or has no scrollable content), it jumps to the new bottom after the content +// is replaced; otherwise the user's scroll position is left untouched. +// +// reviewTUIModel uses value receivers throughout (matching the Bubble Tea +// idiom of returning an updated tea.Model from Update); the viewport is +// mutated in place on the returned copy and the caller assigns the result +// back. +func (m reviewTUIModel) refreshDetailContent() reviewTUIModel { + if len(m.rows) == 0 || m.detailIdx < 0 || m.detailIdx >= len(m.rows) { + m.detail.SetContent("") + return m + } + wasAtBottom := m.detail.AtBottom() + lines := buildEventLines(m.rows[m.detailIdx].buffer, m.detailViewportWidth()) + m.detail.SetContentLines(lines) + if wasAtBottom { + m.detail.GotoBottom() + } + return m +} + // handleAgentEvent processes an agentEventMsg, updating the relevant row. func (m reviewTUIModel) handleAgentEvent(msg agentEventMsg) (tea.Model, tea.Cmd) { idx, ok := m.rowIdx[msg.agent] @@ -214,15 +272,11 @@ func (m reviewTUIModel) handleAgentEvent(msg agentEventMsg) (tea.Model, tea.Cmd) // No visible state update for tool calls in the dashboard. } - // Auto-follow ONLY when the user is already at the bottom. This lets a - // user scroll up to inspect older events without each new event yanking - // them back to the tail. The pre-append max-scroll was for buffer - // length-1; if detailScroll was at-or-past that, the user was tailing. + // Re-render the focused agent's viewport content when a new event lands + // for it. refreshDetailContent's AtBottom check preserves user scroll if + // they've scrolled up; auto-tails otherwise. if m.detailMode && m.detailIdx == idx { - preAppendMax := len(row.buffer) - 2 // -1 for the just-appended event, -1 for max-index - if preAppendMax < 0 || m.detailScroll >= preAppendMax { - m.detailScroll = m.maxDetailScroll() - } + m = m.refreshDetailContent() } return m, nil @@ -253,7 +307,13 @@ func (m reviewTUIModel) handleKey(msg tea.KeyPressMsg) (tea.Model, tea.Cmd) { if len(m.rows) > 0 && (m.detailIdx < 0 || m.detailIdx >= len(m.rows)) { m.detailIdx = 0 } - m.detailScroll = m.maxDetailScroll() + // Resize viewport in case termWidth/termHeight have changed since + // last detail-mode entry, then load the focused agent's events and + // tail to the bottom. + m.detail.SetWidth(m.detailViewportWidth()) + m.detail.SetHeight(m.detailViewportHeight()) + m = m.refreshDetailContent() + m.detail.GotoBottom() return m, nil case msg.Code == tea.KeyEscape || msg.Code == tea.KeyEsc: @@ -266,63 +326,36 @@ func (m reviewTUIModel) handleKey(msg tea.KeyPressMsg) (tea.Model, tea.Cmd) { case msg.Code == tea.KeyLeft: if m.detailMode && len(m.rows) > 0 { m.detailIdx = (m.detailIdx - 1 + len(m.rows)) % len(m.rows) - m.detailScroll = m.maxDetailScroll() + m = m.refreshDetailContent() + m.detail.GotoBottom() } return m, nil case msg.Code == tea.KeyRight: if m.detailMode && len(m.rows) > 0 { m.detailIdx = (m.detailIdx + 1) % len(m.rows) - m.detailScroll = m.maxDetailScroll() - } - return m, nil - - case msg.Code == tea.KeyUp: - if m.detailMode && m.detailScroll > 0 { - m.detailScroll-- - } - return m, nil - - case msg.Code == tea.KeyDown: - if m.detailMode { - if maxScroll := m.maxDetailScroll(); m.detailScroll < maxScroll { - m.detailScroll++ - } + m = m.refreshDetailContent() + m.detail.GotoBottom() } return m, nil } - return m, nil -} - -// maxDetailScroll returns the largest valid detailScroll value for the current -// agent's buffer (0 when the buffer is empty or no rows exist). -func (m reviewTUIModel) maxDetailScroll() int { - if len(m.rows) == 0 { - return 0 - } - n := len(m.rows[m.detailIdx].buffer) - if n == 0 { - return 0 - } - return n - 1 -} - -// clampScroll returns a copy of m with detailScroll clamped to valid bounds. -// Used after resize or index change. -func (m reviewTUIModel) clampScroll() reviewTUIModel { - maxScroll := m.maxDetailScroll() - if m.detailScroll > maxScroll { - m.detailScroll = maxScroll + // Delegate any unhandled key to the viewport so PgUp/PgDn/Home/End/↑/↓ + // reach its internal keymap. Only meaningful in detail mode — on the + // dashboard the viewport is inert. + if m.detailMode { + var cmd tea.Cmd + m.detail, cmd = m.detail.Update(msg) + return m, cmd } - return m + return m, nil } // View renders the current state. func (m reviewTUIModel) View() tea.View { var content string if m.detailMode && len(m.rows) > 0 { - content = detailView(m.rows[m.detailIdx], m.detailScroll, m.termWidth, m.termHeight) + content = detailFrame(m.rows[m.detailIdx], m.detail.View(), m.termWidth, m.termHeight) } else { content = m.dashboardView() } diff --git a/cmd/entire/cli/review/tui_model_test.go b/cmd/entire/cli/review/tui_model_test.go index bfefc0be16..d41bef6c17 100644 --- a/cmd/entire/cli/review/tui_model_test.go +++ b/cmd/entire/cli/review/tui_model_test.go @@ -232,35 +232,47 @@ func TestTUIModel_LeftRight_CycleDetailIdx(t *testing.T) { } } -func TestTUIModel_UpDown_Scroll(t *testing.T) { +// TestTUIModel_InitializesDetailViewport pins that the viewport widget on the +// model starts with non-zero default dimensions so that an immediate Ctrl+O +// before a WindowSizeMsg still renders without panicking. +func TestTUIModel_InitializesDetailViewport(t *testing.T) { t.Parallel() m := newTestModel([]string{"agent-a"}, func() {}) - m.detailMode = true - // Populate buffer so max scroll > 0. - for range 5 { - m.rows[0].buffer = append(m.rows[0].buffer, reviewtypes.Started{}) + if w := m.detail.Width(); w <= 0 { + t.Errorf("expected detail viewport width > 0, got %d", w) } - m.detailScroll = 4 // at max - - // Down when at max: clamp. - updated, _ := m.Update(testKey(tea.KeyDown)) - m = mustModel(t, updated) - if m.detailScroll != 4 { - t.Errorf("scroll should stay at max on Down; got %d", m.detailScroll) + if h := m.detail.Height(); h <= 0 { + t.Errorf("expected detail viewport height > 0, got %d", h) } +} - // Up: 4 → 3. - updated, _ = m.Update(testKey(tea.KeyUp)) +// TestTUIModel_DelegatesScrollKeysToViewport pins that pager keys (PgDn) in +// detail mode reach the viewport's internal keymap and advance YOffset rather +// than being swallowed by the model's switch. +func TestTUIModel_DelegatesScrollKeysToViewport(t *testing.T) { + t.Parallel() + m := newTestModel([]string{"agent-a"}, func() {}) + // Size the window so the viewport has a sensible height. + updated, _ := m.Update(tea.WindowSizeMsg{Width: 40, Height: 10}) m = mustModel(t, updated) - if m.detailScroll != 3 { - t.Errorf("expected scroll=3 after Up; got %d", m.detailScroll) + // Populate enough content to scroll. AssistantText with long text wraps + // to many viewport lines. + long := strings.Repeat("paragraph of text ", 20) + for range 5 { + updated, _ = m.Update(agentEventMsg{agent: "agent-a", ev: reviewtypes.AssistantText{Text: long}}) + m = mustModel(t, updated) } + // Enter detail mode (auto-tails to bottom). + updated, _ = m.Update(testCtrlKey('o')) + m = mustModel(t, updated) + // Jump to top so PgDn has somewhere to scroll into. + m.detail.GotoTop() + startOffset := m.detail.YOffset() - // Down again: 3 → 4. - updated, _ = m.Update(testKey(tea.KeyDown)) + updated, _ = m.Update(tea.KeyPressMsg{Code: tea.KeyPgDown}) m = mustModel(t, updated) - if m.detailScroll != 4 { - t.Errorf("expected scroll=4 after Down; got %d", m.detailScroll) + if m.detail.YOffset() <= startOffset { + t.Errorf("expected PgDn to advance viewport YOffset beyond %d; got %d", startOffset, m.detail.YOffset()) } } @@ -305,22 +317,27 @@ func TestTUIModel_RunFinishedMsg_AnyKeyQuits(t *testing.T) { } } +// TestTUIModel_AutoFollow_DetailMode pins that when the viewport is sitting +// at the bottom and a new event arrives, the model snaps back to bottom so +// the user keeps seeing the tail. The viewport's AtBottom() drives this. func TestTUIModel_AutoFollow_DetailMode(t *testing.T) { t.Parallel() m := newTestModel([]string{"agent-a"}, func() {}) - m.detailMode = true - m.detailIdx = 0 - m.detailScroll = 0 - - // Send 5 events; model should auto-scroll to bottom each time. - current := m - for i := range 5 { - updated, _ := current.Update(agentEventMsg{agent: "agent-a", ev: reviewtypes.Started{}}) - current = mustModel(t, updated) - wantScroll := i // buffer has i+1 events; max scroll is i - if current.detailScroll != wantScroll { - t.Errorf("event %d: want detailScroll=%d, got %d", i, wantScroll, current.detailScroll) - } + updated, _ := m.Update(tea.WindowSizeMsg{Width: 40, Height: 8}) + m = mustModel(t, updated) + updated, _ = m.Update(testCtrlKey('o')) + m = mustModel(t, updated) + + // Send events that produce content taller than the viewport so a tail + // exists below the visible window. + long := strings.Repeat("review finding ", 10) + for range 6 { + updated, _ = m.Update(agentEventMsg{agent: "agent-a", ev: reviewtypes.AssistantText{Text: long}}) + m = mustModel(t, updated) + } + if !m.detail.AtBottom() { + t.Errorf("expected viewport to track bottom after each event (auto-follow); YOffset=%d, total=%d", + m.detail.YOffset(), m.detail.TotalLineCount()) } } @@ -425,41 +442,46 @@ func assertDashboardFitsWidth(t *testing.T, m reviewTUIModel) { } // TestTUIModel_AutoFollow_PreservesUserScroll pins the contract that new -// agent events should NOT yank the user back to the tail when they have +// agent events must NOT yank the user back to the tail when they have // scrolled up to inspect older events. Auto-follow only re-engages when -// the user is already at the bottom. +// the viewport is already at the bottom. func TestTUIModel_AutoFollow_PreservesUserScroll(t *testing.T) { t.Parallel() m := newReviewTUIModel([]string{"agent-a"}, nil) - m.termHeight = 10 - m.detailMode = true - m.detailIdx = 0 + updated, _ := m.Update(tea.WindowSizeMsg{Width: 40, Height: 10}) + m = mustModel(t, updated) + updated, _ = m.Update(testCtrlKey('o')) + m = mustModel(t, updated) - // Build up a buffer of 20 events. + // Build up enough wrapped content to overflow the viewport several times. + long := strings.Repeat("line of review text ", 10) for range 20 { - updated, _ := m.Update(agentEventMsg{agent: "agent-a", ev: reviewtypes.AssistantText{Text: "line"}}) + updated, _ = m.Update(agentEventMsg{agent: "agent-a", ev: reviewtypes.AssistantText{Text: long}}) m = mustModel(t, updated) } - // Snap to bottom, then scroll up by 5. - m.detailScroll = m.maxDetailScroll() - 5 - scrollBeforeNewEvent := m.detailScroll + // Scroll up away from the bottom. + m.detail.GotoTop() + startOffset := m.detail.YOffset() + if m.detail.AtBottom() { + t.Fatal("test setup: viewport unexpectedly at bottom after GotoTop") + } - // Send another event. The user is NOT at the bottom — scroll should not move. - updated, _ := m.Update(agentEventMsg{agent: "agent-a", ev: reviewtypes.AssistantText{Text: "new line"}}) + // Send another event — the user is NOT at the bottom, so YOffset must not move. + updated, _ = m.Update(agentEventMsg{agent: "agent-a", ev: reviewtypes.AssistantText{Text: long}}) m = mustModel(t, updated) - if m.detailScroll != scrollBeforeNewEvent { - t.Errorf("expected detailScroll to stay at %d (user scrolled up), got %d (auto-follow yanked back)", - scrollBeforeNewEvent, m.detailScroll) + if m.detail.YOffset() != startOffset { + t.Errorf("expected viewport YOffset to stay at %d (user scrolled up); got %d (auto-follow yanked back)", + startOffset, m.detail.YOffset()) } - // Now scroll to bottom and send another event — should auto-follow. - m.detailScroll = m.maxDetailScroll() - updated, _ = m.Update(agentEventMsg{agent: "agent-a", ev: reviewtypes.AssistantText{Text: "another"}}) + // Now jump to bottom and send another event — should auto-follow. + m.detail.GotoBottom() + updated, _ = m.Update(agentEventMsg{agent: "agent-a", ev: reviewtypes.AssistantText{Text: long}}) m = mustModel(t, updated) - if m.detailScroll != m.maxDetailScroll() { - t.Errorf("expected auto-follow to track bottom (got detailScroll=%d, max=%d)", - m.detailScroll, m.maxDetailScroll()) + if !m.detail.AtBottom() { + t.Errorf("expected auto-follow to track bottom after event; YOffset=%d, total=%d", + m.detail.YOffset(), m.detail.TotalLineCount()) } } diff --git a/cmd/entire/cli/review/tui_text.go b/cmd/entire/cli/review/tui_text.go index 36b9ba3023..99c2624dd2 100644 --- a/cmd/entire/cli/review/tui_text.go +++ b/cmd/entire/cli/review/tui_text.go @@ -56,3 +56,28 @@ func truncateDisplayWidth(s string, width int) string { } return ansi.Truncate(s, width, "…") } + +// wrapDisplayWidth wraps s to lines no wider than width display cells. Embedded +// '\n' characters are honored as paragraph boundaries: each paragraph is +// sanitized (ANSI/control stripped) and wrapped independently. A paragraph that +// wraps to nothing still contributes an empty line, preserving blank-line +// structure between paragraphs. +// +// Returns nil for width <= 0 or empty input. +func wrapDisplayWidth(s string, width int) []string { + if width <= 0 || s == "" { + return nil + } + paragraphs := strings.Split(s, "\n") + out := make([]string, 0, len(paragraphs)) + for _, p := range paragraphs { + clean := sanitizeDisplayText(p) + if clean == "" { + out = append(out, "") + continue + } + wrapped := ansi.Wrap(clean, width, "") + out = append(out, strings.Split(wrapped, "\n")...) + } + return out +} From 27fd1baca7c299465d6d3dfba122570910374fe5 Mon Sep 17 00:00:00 2001 From: Peyton Montei Date: Mon, 11 May 2026 19:10:54 -0400 Subject: [PATCH 02/11] feat(review): post-finish access + visible cancellation feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two user-visible fixes to the multi-agent review TUI: 1. Post-finish drill-in is still reachable. Before: any key on the post-finish dashboard quit the TUI, so Ctrl+O on a completed run was swallowed and the user couldn't inspect the final per-agent buffer. Now: only q, Esc, Enter, and Ctrl+C dismiss the post-finish dashboard; every other key (including Ctrl+O) falls through to normal handling. The footer changes from "Press any key to exit." to "Ctrl+O: drill in · q/Esc/Enter: exit" so the available actions are explicit. Detail-mode behavior is unchanged — Esc still returns to the dashboard and viewport keys still scroll. 2. Cancellation is visible while agents drain. Before: pressing Ctrl+C once cancelled both agents via the shared CancelFunc, but the dashboard kept showing the running spinner until the agents fully drained. With no visual feedback, users pressed Ctrl+C a second time to dismiss, which short-circuited the dump. Now: a `cancelling` flag flips to true on the first Ctrl+C alongside the existing `cancelOnce.Do(cancel)`. The dashboard footer becomes "Cancelling agents... · Ctrl+C again: force quit", and any agent still in the running branch of renderRow renders as the literal "cancelling" — distinct from the terminal "— cancel" used for AgentStatusCancelled, and 10 chars so it fits the existing statusWidth column without resizing the table. The first Ctrl+C no longer emits tea.Quit; the TUI stays up so the user can see the indicator while the program awaits the natural runFinishedMsg. A second Ctrl+C while cancelling=true force-quits immediately via tea.Quit. The shared CancelFunc still fires at most once thanks to cancelOnce, so the second press doesn't double-cancel. The single shared CancelFunc mechanism is unchanged — only the visibility/feedback layer was added. Per-agent context cancellation, mouse-wheel scroll, and viewport rendering are out of scope. Co-Authored-By: Claude Opus 4.7 (1M context) Entire-Checkpoint: e3562dc89402 --- cmd/entire/cli/review/tui_model.go | 64 ++++++- cmd/entire/cli/review/tui_model_test.go | 217 ++++++++++++++++++++++-- cmd/entire/cli/review/tui_sink_test.go | 6 +- 3 files changed, 262 insertions(+), 25 deletions(-) diff --git a/cmd/entire/cli/review/tui_model.go b/cmd/entire/cli/review/tui_model.go index 320e7170e8..34c377f6ae 100644 --- a/cmd/entire/cli/review/tui_model.go +++ b/cmd/entire/cli/review/tui_model.go @@ -69,6 +69,13 @@ type reviewTUIModel struct { finished bool summary reviewtypes.RunSummary + + // cancelling tracks whether a Ctrl+C-initiated cancellation is in flight. + // Set true on the first Ctrl+C (in tandem with cancelOnce firing the shared + // CancelFunc); the dashboard switches to a "cancelling" indicator and the + // footer offers a force-quit hint. A second Ctrl+C while cancelling=true + // force-quits without waiting for agents to drain. + cancelling bool } // newReviewTUIModel builds an initial model pre-populated with one row per @@ -284,9 +291,14 @@ func (m reviewTUIModel) handleAgentEvent(msg agentEventMsg) (tea.Model, tea.Cmd) // handleKey processes keyboard input. func (m reviewTUIModel) handleKey(msg tea.KeyPressMsg) (tea.Model, tea.Cmd) { - // Any key after finished dismisses. - if m.finished { - return m, tea.Quit + // Post-finish dashboard: only explicit exit keys dismiss. Any other key + // (including Ctrl+O) falls through to normal handling so the user can + // still drill into completed agent output. Ctrl+C also quits immediately + // — there are no agents left to drain. + if m.finished && !m.detailMode { + if isPostFinishExitKey(msg) || (msg.Code == 'c' && msg.Mod == tea.ModCtrl) { + return m, tea.Quit + } } switch { @@ -295,8 +307,18 @@ func (m reviewTUIModel) handleKey(msg tea.KeyPressMsg) (tea.Model, tea.Cmd) { // In drill-in: Ctrl+C is intentionally ignored; Esc first. return m, nil } + // Second Ctrl+C while a cancellation is already in flight force-quits + // without waiting for agents to drain. The shared CancelFunc is + // guarded by cancelOnce, so the second press doesn't double-cancel. + if m.cancelling { + return m, tea.Quit + } + m.cancelling = true m.cancelOnce.Do(m.cancel) - return m, tea.Quit + // Do NOT quit on the first Ctrl+C: leave the TUI up so the user sees + // the cancelling indicator while agents drain. Natural finish + // (runFinishedMsg) or a second Ctrl+C dismisses the TUI. + return m, nil case msg.Code == 'o' && msg.Mod == tea.ModCtrl: if m.detailMode { @@ -351,6 +373,21 @@ func (m reviewTUIModel) handleKey(msg tea.KeyPressMsg) (tea.Model, tea.Cmd) { return m, nil } +// isPostFinishExitKey reports whether msg is one of the explicit exit keys +// honored on the post-finish dashboard. Ctrl+C is handled separately in the +// normal switch so duplicate-press semantics (cancelling → force-quit) still +// apply; here we only need q/Esc/Enter without modifiers. +func isPostFinishExitKey(msg tea.KeyPressMsg) bool { + if msg.Mod != 0 { + return false + } + switch msg.Code { + case 'q', tea.KeyEnter, tea.KeyEscape: + return true + } + return false +} + // View renders the current state. func (m reviewTUIModel) View() tea.View { var content string @@ -375,10 +412,13 @@ func (m reviewTUIModel) dashboardView() string { } b.WriteString("\n") - if m.finished { + switch { + case m.finished: m.writeDashboardLine(&b, m.countsLine()) - m.writeDashboardLine(&b, "Press any key to exit.") - } else { + m.writeDashboardLine(&b, "Ctrl+O: drill in · q/Esc/Enter: exit") + case m.cancelling: + m.writeDashboardLine(&b, "Cancelling agents... · Ctrl+C again: force quit") + default: m.writeDashboardLine(&b, "Ctrl+O: drill in · Ctrl+C: cancel") } return b.String() @@ -414,9 +454,15 @@ func (m reviewTUIModel) renderRow(row agentRow) string { case reviewtypes.AgentStatusCancelled: statusStr = "— cancel" case reviewtypes.AgentStatusUnknown: - if row.runStart.IsZero() { + switch { + case m.cancelling: + // In-flight cancellation: distinct from the terminal Cancelled + // state ("— cancel") so the user can see that the cancel signal + // has been sent but the agent is still draining. + statusStr = "cancelling" + case row.runStart.IsZero(): statusStr = "queued" - } else { + default: statusStr = m.spinner.View() + " running" } } diff --git a/cmd/entire/cli/review/tui_model_test.go b/cmd/entire/cli/review/tui_model_test.go index d41bef6c17..076d0a545c 100644 --- a/cmd/entire/cli/review/tui_model_test.go +++ b/cmd/entire/cli/review/tui_model_test.go @@ -118,23 +118,28 @@ func TestTUIModel_AgentEvent_RunError(t *testing.T) { } } -func TestTUIModel_KeyCtrlC_NotDetailMode_CancelsAndQuits(t *testing.T) { +func TestTUIModel_KeyCtrlC_NotDetailMode_CancelsAndMarksCancelling(t *testing.T) { t.Parallel() var called atomic.Bool cancel := func() { called.Store(true) } m := newTestModel([]string{"agent-a"}, cancel) - _, cmd := m.Update(testCtrlKey('c')) + updated, cmd := m.Update(testCtrlKey('c')) if !called.Load() { t.Error("expected cancel to be called on Ctrl+C outside detail mode") } - if cmd == nil { - t.Error("expected a quit command to be returned") + m2 := mustModel(t, updated) + if !m2.cancelling { + t.Error("expected m.cancelling=true after first Ctrl+C") } - // Verify it's the quit command by running it. - msg := cmd() - if _, ok := msg.(tea.QuitMsg); !ok { - t.Errorf("expected tea.QuitMsg from Ctrl+C cmd, got %T", msg) + // First Ctrl+C must NOT quit — the TUI stays up so agents can drain + // visibly. Only a second Ctrl+C (or natural finish) dismisses. + if cmd != nil { + if msg := cmd(); msg != nil { + if _, ok := msg.(tea.QuitMsg); ok { + t.Error("first Ctrl+C must NOT send a quit command; it should wait for agents to drain") + } + } } } @@ -297,7 +302,7 @@ func TestTUIModel_TickMsg_ReSchedulesTick(t *testing.T) { } } -func TestTUIModel_RunFinishedMsg_AnyKeyQuits(t *testing.T) { +func TestTUIModel_RunFinishedMsg_MarksFinished(t *testing.T) { t.Parallel() m := newTestModel([]string{"agent-a"}, func() {}) @@ -306,14 +311,198 @@ func TestTUIModel_RunFinishedMsg_AnyKeyQuits(t *testing.T) { if !m2.finished { t.Error("model should be finished after runFinishedMsg") } +} + +// TestTUIModel_PostFinishCtrlOEntersDetailMode pins that Ctrl+O still enters +// drill-in after both agents finish so the user can inspect completed output. +// Previously any-key-quits behavior swallowed Ctrl+O on the post-finish frame. +func TestTUIModel_PostFinishCtrlOEntersDetailMode(t *testing.T) { + t.Parallel() + m := newTestModel([]string{"agent-a", "agent-b"}, func() {}) + updated, _ := m.Update(runFinishedMsg{summary: reviewtypes.RunSummary{}}) + m = mustModel(t, updated) + if !m.finished { + t.Fatal("setup: expected model finished after runFinishedMsg") + } + + updated, cmd := m.Update(testCtrlKey('o')) + m2 := mustModel(t, updated) + if !m2.detailMode { + t.Error("expected Ctrl+O post-finish to enter detail mode") + } + // Ctrl+O must NOT quit post-finish. + if cmd != nil { + if msg := cmd(); msg != nil { + if _, ok := msg.(tea.QuitMsg); ok { + t.Error("Ctrl+O post-finish must not produce a quit command") + } + } + } +} + +// TestTUIModel_PostFinishQuitsOnExplicitKeys pins that q, Esc, Enter, and Ctrl+C +// each produce tea.QuitMsg when finished and in dashboard mode. +func TestTUIModel_PostFinishQuitsOnExplicitKeys(t *testing.T) { + t.Parallel() + cases := []struct { + name string + key tea.KeyPressMsg + }{ + {"q", testKey('q')}, + {"Esc", testKey(tea.KeyEscape)}, + {"Enter", testKey(tea.KeyEnter)}, + {"Ctrl+C", testCtrlKey('c')}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + m := newTestModel([]string{"agent-a"}, func() {}) + updated, _ := m.Update(runFinishedMsg{summary: reviewtypes.RunSummary{}}) + m = mustModel(t, updated) + + _, cmd := m.Update(tc.key) + if cmd == nil { + t.Fatalf("expected a command for explicit-exit key %q post-finish", tc.name) + } + msg := cmd() + if _, ok := msg.(tea.QuitMsg); !ok { + t.Errorf("expected tea.QuitMsg for key %q post-finish, got %T", tc.name, msg) + } + }) + } +} + +// TestTUIModel_PostFinishIgnoresRandomKeys pins that non-exit keys (e.g. 'x', +// arrow keys) do NOT quit when finished on the dashboard. They fall through to +// normal handling instead of the old any-key-quits shortcut. +func TestTUIModel_PostFinishIgnoresRandomKeys(t *testing.T) { + t.Parallel() + cases := []struct { + name string + key tea.KeyPressMsg + }{ + {"x", testKey('x')}, + {"Right", testKey(tea.KeyRight)}, + {"Left", testKey(tea.KeyLeft)}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + m := newTestModel([]string{"agent-a", "agent-b"}, func() {}) + updated, _ := m.Update(runFinishedMsg{summary: reviewtypes.RunSummary{}}) + m = mustModel(t, updated) + + _, cmd := m.Update(tc.key) + if cmd != nil { + if msg := cmd(); msg != nil { + if _, ok := msg.(tea.QuitMsg); ok { + t.Errorf("key %q must not quit post-finish on dashboard", tc.name) + } + } + } + }) + } +} + +// TestTUIModel_PostFinishFooterUsesExplicitExitKeys pins the dashboard footer +// switches from the old "Press any key to exit." prompt to an explicit-keys +// hint that mentions Ctrl+O and the named exit keys. +func TestTUIModel_PostFinishFooterUsesExplicitExitKeys(t *testing.T) { + t.Parallel() + m := newTestModel([]string{"agent-a"}, func() {}) + m.termWidth = 120 + updated, _ := m.Update(runFinishedMsg{summary: reviewtypes.RunSummary{}}) + m = mustModel(t, updated) + + out := m.dashboardView() + if strings.Contains(out, "Press any key to exit.") { + t.Errorf("post-finish footer must not show legacy 'Press any key to exit.' line:\n%s", out) + } + if !strings.Contains(out, "Ctrl+O") { + t.Errorf("post-finish footer should mention Ctrl+O for drill-in:\n%s", out) + } + if !strings.Contains(out, "q/Esc/Enter") { + t.Errorf("post-finish footer should list q/Esc/Enter as exit keys:\n%s", out) + } +} - // Any key should now quit. - _, cmd := m2.Update(testKey(tea.KeyEnter)) +// TestTUIModel_CtrlCMarksAgentsCancelling pins that the first Ctrl+C while +// agents are still running sets m.cancelling and renderRow reflects an +// in-flight cancellation indicator (distinct from the terminal Cancelled +// state). +func TestTUIModel_CtrlCMarksAgentsCancelling(t *testing.T) { + t.Parallel() + m := newTestModel([]string{"agent-a"}, func() {}) + // Stamp runStart so the row is in the running branch of renderRow. + updated, _ := m.Update(agentEventMsg{agent: "agent-a", ev: reviewtypes.Started{}}) + m = mustModel(t, updated) + m.termWidth = 120 + + // Before Ctrl+C the running row should say "running". + if status := m.renderRow(m.rows[0]); !strings.Contains(status, "running") { + t.Fatalf("setup: expected running indicator pre-Ctrl+C; got %q", status) + } + + updated, _ = m.Update(testCtrlKey('c')) + m = mustModel(t, updated) + if !m.cancelling { + t.Fatal("expected m.cancelling=true after first Ctrl+C") + } + + got := m.renderRow(m.rows[0]) + if strings.Contains(got, "running") { + t.Errorf("renderRow should drop the 'running' indicator once cancelling; got %q", got) + } + if !strings.Contains(got, "cancel") { + t.Errorf("renderRow should show a cancelling indicator; got %q", got) + } +} + +// TestTUIModel_FooterDuringCancellation pins that the dashboard footer changes +// while a cancel is in flight (cancelling && !finished) to signal the user +// that draining is in progress and a second Ctrl+C will force quit. +func TestTUIModel_FooterDuringCancellation(t *testing.T) { + t.Parallel() + m := newTestModel([]string{"agent-a"}, func() {}) + m.termWidth = 120 + updated, _ := m.Update(testCtrlKey('c')) + m = mustModel(t, updated) + + out := m.dashboardView() + if !strings.Contains(out, "Cancelling agents") { + t.Errorf("expected footer to announce cancellation in progress; got:\n%s", out) + } + if !strings.Contains(out, "Ctrl+C again") { + t.Errorf("expected footer to mention force-quit hint; got:\n%s", out) + } +} + +// TestTUIModel_SecondCtrlCForceQuits pins that once cancelling is in flight, +// a second Ctrl+C emits tea.QuitMsg immediately rather than waiting for +// agents to drain. +func TestTUIModel_SecondCtrlCForceQuits(t *testing.T) { + t.Parallel() + var count atomic.Int32 + cancel := func() { count.Add(1) } + + m := newTestModel([]string{"agent-a"}, cancel) + updated, _ := m.Update(testCtrlKey('c')) + m = mustModel(t, updated) + if !m.cancelling { + t.Fatal("setup: expected cancelling=true after first Ctrl+C") + } + + _, cmd := m.Update(testCtrlKey('c')) if cmd == nil { - t.Error("expected quit command after finished + any key") + t.Fatal("expected a command from second Ctrl+C") + } + msg := cmd() + if _, ok := msg.(tea.QuitMsg); !ok { + t.Errorf("expected tea.QuitMsg from second Ctrl+C, got %T", msg) } - if msg := cmd(); msg == nil { - t.Error("expected non-nil quit msg") + // Shared CancelFunc still fires at most once thanks to cancelOnce. + if got := count.Load(); got != 1 { + t.Errorf("CancelFunc should fire exactly once across both Ctrl+Cs; got %d", got) } } diff --git a/cmd/entire/cli/review/tui_sink_test.go b/cmd/entire/cli/review/tui_sink_test.go index 374d3ab00d..217a1de2d6 100644 --- a/cmd/entire/cli/review/tui_sink_test.go +++ b/cmd/entire/cli/review/tui_sink_test.go @@ -28,7 +28,9 @@ func finishAndDismissTUI(t *testing.T, sink *TUISink, summary reviewtypes.RunSum case <-done: return case <-ticker.C: - sink.program.Send(tea.KeyPressMsg(tea.Key{Code: 'x', Text: "x"})) + // 'q' is an explicit post-finish exit key. (Any-key-quits was + // removed so the user can still Ctrl+O into completed output.) + sink.program.Send(tea.KeyPressMsg(tea.Key{Code: 'q', Text: "q"})) case <-timeout: t.Fatal("RunFinished() did not return within 10 seconds") } @@ -98,7 +100,7 @@ func TestTUISink_AgentEvent_BeforeStart_IsNoOp(t *testing.T) { } // TestTUISink_RunFinished_EventuallyUnblocks verifies that RunFinished unblocks -// once the finished TUI receives the same any-key dismissal used by a user. +// once the finished TUI receives an explicit exit key (q) like a user would press. func TestTUISink_RunFinished_EventuallyUnblocks(t *testing.T) { t.Parallel() var buf bytes.Buffer From 998ddfd1e71591058a79e6bbf47c2b91f228fdae Mon Sep 17 00:00:00 2001 From: Peyton Montei Date: Mon, 11 May 2026 19:23:53 -0400 Subject: [PATCH 03/11] feat(review): enable mouse-wheel scroll in drill-in viewport MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Commit 1 of this branch swapped the manual drill-in scroll path for bubbles/v2/viewport. The viewport widget handles mouse-wheel events natively, but only if the Bubble Tea program is configured to deliver mouse messages — otherwise wheel input is consumed by the terminal as a plain scroll-region scroll, which the alt-screen ignores. Result: the viewport sat inert under the wheel even though every other interaction worked. Bubble Tea v2 reshaped mouse config from a Program option (`tea.WithMouseCellMotion()` in v1) to a per-view field (`View.MouseMode`). Set `MouseMode = tea.MouseModeCellMotion` on the detail-mode View next to the existing AltScreen toggle. Dashboard mode keeps the default `MouseModeNone` so the summary table doesn't capture mouse selection — wheel scrolling only kicks in once the user enters the drill-in via Ctrl+O. Extended the existing AltScreen-toggle tests to also assert the MouseMode flip; the wheel-handling itself lives in viewport and is covered upstream. Co-Authored-By: Claude Opus 4.7 (1M context) Entire-Checkpoint: 007dbb86f787 --- cmd/entire/cli/review/tui_model.go | 11 +++++++++++ cmd/entire/cli/review/tui_model_test.go | 6 ++++++ 2 files changed, 17 insertions(+) diff --git a/cmd/entire/cli/review/tui_model.go b/cmd/entire/cli/review/tui_model.go index 34c377f6ae..903031dd2d 100644 --- a/cmd/entire/cli/review/tui_model.go +++ b/cmd/entire/cli/review/tui_model.go @@ -389,6 +389,14 @@ func isPostFinishExitKey(msg tea.KeyPressMsg) bool { } // View renders the current state. +// +// In detail mode we enable [tea.MouseModeCellMotion] so the embedded +// [viewport.Model] receives mouse-wheel events natively — the viewport handles +// them as scroll, but only if the Bubble Tea program is configured to deliver +// mouse messages. Bubble Tea v2 expresses that config as a per-view field +// rather than a Program option, so it lives here next to AltScreen. +// Dashboard mode leaves the default [tea.MouseModeNone] in place so normal +// terminal mouse selection still works on the summary table. func (m reviewTUIModel) View() tea.View { var content string if m.detailMode && len(m.rows) > 0 { @@ -398,6 +406,9 @@ func (m reviewTUIModel) View() tea.View { } v := tea.NewView(content) v.AltScreen = m.detailMode + if m.detailMode { + v.MouseMode = tea.MouseModeCellMotion + } return v } diff --git a/cmd/entire/cli/review/tui_model_test.go b/cmd/entire/cli/review/tui_model_test.go index 076d0a545c..9362c30982 100644 --- a/cmd/entire/cli/review/tui_model_test.go +++ b/cmd/entire/cli/review/tui_model_test.go @@ -193,6 +193,9 @@ func TestTUIModel_KeyCtrlO_EntersDrillIn(t *testing.T) { if !m2.View().AltScreen { t.Error("expected View().AltScreen=true in detail mode") } + if got := m2.View().MouseMode; got != tea.MouseModeCellMotion { + t.Errorf("expected View().MouseMode=MouseModeCellMotion in detail mode (so viewport gets wheel events); got %v", got) + } } func TestTUIModel_KeyEsc_ExitsDrillIn(t *testing.T) { @@ -211,6 +214,9 @@ func TestTUIModel_KeyEsc_ExitsDrillIn(t *testing.T) { if m2.View().AltScreen { t.Error("expected View().AltScreen=false outside detail mode") } + if got := m2.View().MouseMode; got != tea.MouseModeNone { + t.Errorf("expected View().MouseMode=MouseModeNone outside detail mode (preserve normal terminal selection on dashboard); got %v", got) + } } func TestTUIModel_LeftRight_CycleDetailIdx(t *testing.T) { From 6fb988b431c2ad32cf675e3f008cef78579168e1 Mon Sep 17 00:00:00 2001 From: Peyton Montei Date: Mon, 11 May 2026 19:32:40 -0400 Subject: [PATCH 04/11] review: update tui_sink doc comments to match new exit-key behavior The RunFinished doc on TUISink still described the pre-Commit-2 behavior ("press any key to dismiss"). Update to reflect the new explicit exit-key handling (q/Esc/Enter/Ctrl+C) with Ctrl+O still available for post-mortem drill-in. Comment-only change, no code behavior touched. Co-Authored-By: Claude Opus 4.7 (1M context) Entire-Checkpoint: a6c21d0fe3b7 --- cmd/entire/cli/review/tui_sink.go | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/cmd/entire/cli/review/tui_sink.go b/cmd/entire/cli/review/tui_sink.go index 77f7d8ff77..bda351469d 100644 --- a/cmd/entire/cli/review/tui_sink.go +++ b/cmd/entire/cli/review/tui_sink.go @@ -120,11 +120,15 @@ func (s *TUISink) AgentEvent(agent string, ev reviewtypes.Event) { // RunFinished (Sink interface): mark the run complete and send the final // summary message. The TUI shows the dashboard one more frame with the -// terminal statuses and waits for the user to press any key to dismiss. +// terminal statuses, then waits for the user to dismiss it. After a run +// completes, dismissal requires an explicit exit key (q/Esc/Enter/Ctrl+C); +// Ctrl+O still drills into agent buffers for post-mortem inspection. Other +// keys are no-ops so users can navigate the completed run without +// accidentally dismissing. // -// IMPORTANT: RunFinished blocks until the user dismisses (presses any key) -// so that post-run sinks (e.g. DumpSink) render their narrative AFTER the -// TUI has exited and the terminal is back in normal mode. +// IMPORTANT: RunFinished blocks until the user dismisses so that post-run +// sinks (e.g. DumpSink) render their narrative AFTER the TUI has exited +// and the terminal is back in normal mode. func (s *TUISink) RunFinished(summary reviewtypes.RunSummary) { s.mu.Lock() if s.finished { @@ -135,8 +139,8 @@ func (s *TUISink) RunFinished(summary reviewtypes.RunSummary) { s.mu.Unlock() s.program.Send(runFinishedMsg{summary: summary}) - // Block until the Bubble Tea program exits (user presses any key after - // seeing the final dashboard, or Ctrl+C was already received and the - // program already quit). + // Block until the Bubble Tea program exits — user pressed an explicit + // exit key (q/Esc/Enter/Ctrl+C) after seeing the final dashboard, or + // Ctrl+C was received during the run and the program already quit. s.Wait() } From c279308b22d7bc77a43ad2f580523023d302b3c1 Mon Sep 17 00:00:00 2001 From: Peyton Montei Date: Mon, 11 May 2026 19:54:09 -0400 Subject: [PATCH 05/11] review: route mouse-wheel events to viewport in detail mode MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Commit 998ddfd1e correctly set View.MouseMode = MouseModeCellMotion on entry to detail mode so the bubbletea Program would receive mouse events from the terminal. But the Update switch only routed tea.KeyPressMsg to handleKey and tea.WindowSizeMsg to a resize handler — mouse events fell through to the catch-all `return m, nil` at the bottom and never reached the viewport. bubbles/v2/viewport handles tea.MouseWheelMsg natively when its MouseWheelEnabled flag is set (true by default), so the fix is just to add a case in the Update switch that forwards mouse messages to m.detail.Update when in detail mode. Includes a regression test (TestTUIModel_DelegatesMouseWheelToViewport) that sends a synthetic tea.MouseWheelMsg{Button: tea.MouseWheelDown} into the model in detail mode and asserts the viewport's YOffset advances. Mirrors the existing keyboard-delegation test pattern. Co-Authored-By: Claude Opus 4.7 (1M context) Entire-Checkpoint: 39b4e9bacbf6 --- cmd/entire/cli/review/tui_model.go | 14 +++++++++++++ cmd/entire/cli/review/tui_model_test.go | 28 +++++++++++++++++++++++++ 2 files changed, 42 insertions(+) diff --git a/cmd/entire/cli/review/tui_model.go b/cmd/entire/cli/review/tui_model.go index 903031dd2d..3940390849 100644 --- a/cmd/entire/cli/review/tui_model.go +++ b/cmd/entire/cli/review/tui_model.go @@ -185,6 +185,20 @@ func (m reviewTUIModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) { case tea.KeyPressMsg: return m.handleKey(msg) + + case tea.MouseWheelMsg, tea.MouseClickMsg, tea.MouseReleaseMsg, tea.MouseMotionMsg: + // Mouse events: only meaningful inside the drill-in viewport, which + // reads tea.MouseWheelMsg natively (see bubbles/v2/viewport.go:696). + // Without this delegation the events arrive at the Program (because + // View.MouseMode = MouseModeCellMotion is set during detail mode) + // but fall through Update unhandled — the user sees no scroll + // response to the wheel. + if m.detailMode { + var cmd tea.Cmd + m.detail, cmd = m.detail.Update(msg) + return m, cmd + } + return m, nil } return m, nil } diff --git a/cmd/entire/cli/review/tui_model_test.go b/cmd/entire/cli/review/tui_model_test.go index 9362c30982..39477d8b01 100644 --- a/cmd/entire/cli/review/tui_model_test.go +++ b/cmd/entire/cli/review/tui_model_test.go @@ -257,6 +257,34 @@ func TestTUIModel_InitializesDetailViewport(t *testing.T) { } } +// TestTUIModel_DelegatesMouseWheelToViewport pins that mouse-wheel events in +// detail mode reach the viewport's MouseWheelMsg handler. The bug this guards +// against: setting View.MouseMode = MouseModeCellMotion on entry to detail +// mode caused the Program to receive mouse events, but the Update switch +// only routed tea.KeyPressMsg — mouse events fell through unhandled and the +// viewport never got a chance to scroll. +func TestTUIModel_DelegatesMouseWheelToViewport(t *testing.T) { + t.Parallel() + m := newTestModel([]string{"agent-a"}, func() {}) + updated, _ := m.Update(tea.WindowSizeMsg{Width: 40, Height: 10}) + m = mustModel(t, updated) + long := strings.Repeat("paragraph of text ", 20) + for range 5 { + updated, _ = m.Update(agentEventMsg{agent: "agent-a", ev: reviewtypes.AssistantText{Text: long}}) + m = mustModel(t, updated) + } + updated, _ = m.Update(testCtrlKey('o')) + m = mustModel(t, updated) + m.detail.GotoTop() + startOffset := m.detail.YOffset() + + updated, _ = m.Update(tea.MouseWheelMsg{Button: tea.MouseWheelDown}) + m = mustModel(t, updated) + if m.detail.YOffset() <= startOffset { + t.Errorf("expected mouse-wheel-down to advance viewport YOffset beyond %d; got %d", startOffset, m.detail.YOffset()) + } +} + // TestTUIModel_DelegatesScrollKeysToViewport pins that pager keys (PgDn) in // detail mode reach the viewport's internal keymap and advance YOffset rather // than being swallowed by the model's switch. From cc9017396d483e31815d348dfbbc74505e37abe8 Mon Sep 17 00:00:00 2001 From: Peyton Montei Date: Mon, 11 May 2026 22:49:46 -0400 Subject: [PATCH 06/11] review: address viewport follow-up feedback Entire-Checkpoint: 21824f232bbf --- cmd/entire/cli/review/tui_detail.go | 2 +- cmd/entire/cli/review/tui_model.go | 20 +++++++++++------ cmd/entire/cli/review/tui_model_test.go | 30 +++++++++++++++++++++++++ 3 files changed, 44 insertions(+), 8 deletions(-) diff --git a/cmd/entire/cli/review/tui_detail.go b/cmd/entire/cli/review/tui_detail.go index 04e3cfa799..2edd2a3fcf 100644 --- a/cmd/entire/cli/review/tui_detail.go +++ b/cmd/entire/cli/review/tui_detail.go @@ -56,7 +56,7 @@ func eventLines(ev reviewtypes.Event, maxWidth int) []string { // buildEventLines returns every wrapped body line for the supplied event // buffer, in order. The result is suitable for feeding into a viewport via -// SetContent. +// SetContentLines. func buildEventLines(buffer []reviewtypes.Event, maxWidth int) []string { if len(buffer) == 0 || maxWidth <= 0 { return nil diff --git a/cmd/entire/cli/review/tui_model.go b/cmd/entire/cli/review/tui_model.go index 3940390849..ef09354f0f 100644 --- a/cmd/entire/cli/review/tui_model.go +++ b/cmd/entire/cli/review/tui_model.go @@ -176,11 +176,12 @@ func (m reviewTUIModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) { return m, spinCmd case tea.WindowSizeMsg: + wasAtBottom := m.detail.AtBottom() m.termWidth = msg.Width m.termHeight = msg.Height m.detail.SetWidth(m.detailViewportWidth()) m.detail.SetHeight(m.detailViewportHeight()) - m = m.refreshDetailContent() + m = m.refreshDetailContentWithAutoTail(wasAtBottom) return m, nil case tea.KeyPressMsg: @@ -188,7 +189,7 @@ func (m reviewTUIModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) { case tea.MouseWheelMsg, tea.MouseClickMsg, tea.MouseReleaseMsg, tea.MouseMotionMsg: // Mouse events: only meaningful inside the drill-in viewport, which - // reads tea.MouseWheelMsg natively (see bubbles/v2/viewport.go:696). + // handles tea.MouseWheelMsg natively. // Without this delegation the events arrive at the Program (because // View.MouseMode = MouseModeCellMotion is set during detail mode) // but fall through Update unhandled — the user sees no scroll @@ -231,11 +232,14 @@ func (m reviewTUIModel) detailViewportHeight() int { // mutated in place on the returned copy and the caller assigns the result // back. func (m reviewTUIModel) refreshDetailContent() reviewTUIModel { + return m.refreshDetailContentWithAutoTail(m.detail.AtBottom()) +} + +func (m reviewTUIModel) refreshDetailContentWithAutoTail(wasAtBottom bool) reviewTUIModel { if len(m.rows) == 0 || m.detailIdx < 0 || m.detailIdx >= len(m.rows) { m.detail.SetContent("") return m } - wasAtBottom := m.detail.AtBottom() lines := buildEventLines(m.rows[m.detailIdx].buffer, m.detailViewportWidth()) m.detail.SetContentLines(lines) if wasAtBottom { @@ -388,15 +392,17 @@ func (m reviewTUIModel) handleKey(msg tea.KeyPressMsg) (tea.Model, tea.Cmd) { } // isPostFinishExitKey reports whether msg is one of the explicit exit keys -// honored on the post-finish dashboard. Ctrl+C is handled separately in the -// normal switch so duplicate-press semantics (cancelling → force-quit) still -// apply; here we only need q/Esc/Enter without modifiers. +// honored on the post-finish dashboard. Ctrl+C is handled by the caller because +// it has a modifier; here we only need q/Esc/Enter without modifiers. func isPostFinishExitKey(msg tea.KeyPressMsg) bool { if msg.Mod != 0 { return false } + if msg.Code == tea.KeyEscape || msg.Code == tea.KeyEsc { + return true + } switch msg.Code { - case 'q', tea.KeyEnter, tea.KeyEscape: + case 'q', tea.KeyEnter: return true } return false diff --git a/cmd/entire/cli/review/tui_model_test.go b/cmd/entire/cli/review/tui_model_test.go index 39477d8b01..4843ed3641 100644 --- a/cmd/entire/cli/review/tui_model_test.go +++ b/cmd/entire/cli/review/tui_model_test.go @@ -384,6 +384,7 @@ func TestTUIModel_PostFinishQuitsOnExplicitKeys(t *testing.T) { }{ {"q", testKey('q')}, {"Esc", testKey(tea.KeyEscape)}, + {"KeyEsc", testKey(tea.KeyEsc)}, {"Enter", testKey(tea.KeyEnter)}, {"Ctrl+C", testCtrlKey('c')}, } @@ -564,6 +565,35 @@ func TestTUIModel_AutoFollow_DetailMode(t *testing.T) { } } +// TestTUIModel_AutoFollow_ResizePreservesBottomWhenTailing pins that a user who +// is tailing the detail viewport remains at the bottom after a resize changes +// wrapping and increases the viewport's maximum scroll offset. +func TestTUIModel_AutoFollow_ResizePreservesBottomWhenTailing(t *testing.T) { + t.Parallel() + m := newTestModel([]string{"agent-a"}, func() {}) + updated, _ := m.Update(tea.WindowSizeMsg{Width: 80, Height: 12}) + m = mustModel(t, updated) + updated, _ = m.Update(testCtrlKey('o')) + m = mustModel(t, updated) + + long := strings.Repeat("resize-sensitive review finding ", 8) + for range 8 { + updated, _ = m.Update(agentEventMsg{agent: "agent-a", ev: reviewtypes.AssistantText{Text: long}}) + m = mustModel(t, updated) + } + m.detail.GotoBottom() + if !m.detail.AtBottom() { + t.Fatal("setup: expected viewport to be at bottom before resize") + } + + updated, _ = m.Update(tea.WindowSizeMsg{Width: 30, Height: 8}) + m = mustModel(t, updated) + if !m.detail.AtBottom() { + t.Errorf("expected viewport to stay at bottom after resize; YOffset=%d, total=%d", + m.detail.YOffset(), m.detail.TotalLineCount()) + } +} + func TestTUIModel_View_DashboardMode(t *testing.T) { t.Parallel() m := newTestModel([]string{"agent-a", "agent-b"}, func() {}) From 2b5e06f853cf24acd29aa6725fafc26ba00ee71f Mon Sep 17 00:00:00 2001 From: Peyton Montei Date: Tue, 12 May 2026 00:21:23 -0400 Subject: [PATCH 07/11] review: align TUI docs and drill-in footer with current keybindings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - tui_sink.go: TUISink doc still described the pre-PR "single Ctrl+C cancels and quits" semantic; update to reflect the two-press flow (first press fires CancelFunc + shows indicator, second press force-quits). - tui_detail.go: drill-in footer listed only PgUp/PgDn even though the viewport's default keymap binds ↑/↓/Home/End as well; expand so users see what actually works. Co-Authored-By: Claude Opus 4.7 (1M context) Entire-Checkpoint: 091517a61c13 --- cmd/entire/cli/review/tui_detail.go | 4 ++-- cmd/entire/cli/review/tui_sink.go | 10 ++++++---- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/cmd/entire/cli/review/tui_detail.go b/cmd/entire/cli/review/tui_detail.go index 2edd2a3fcf..14d2e7a034 100644 --- a/cmd/entire/cli/review/tui_detail.go +++ b/cmd/entire/cli/review/tui_detail.go @@ -74,7 +74,7 @@ func buildEventLines(buffer []reviewtypes.Event, maxWidth int) []string { // // 1. Header line: "─── ( events) ─────────────" (filled to termWidth) // 2. Body: trimmed/padded to exactly bodyHeight lines, each padded to termWidth. -// 3. Footer line: "←/→ switch agent · Esc back · PgUp/PgDn scroll" +// 3. Footer line: "←/→ switch agent · Esc back · scroll: PgUp/PgDn/↑/↓/Home/End" // // CRITICAL: the rendered string is exactly termHeight lines. Bubble Tea's // alt-screen diff leaves ghost rows if the line count varies between frames. @@ -98,7 +98,7 @@ func detailFrame(row agentRow, body string, termWidth, termHeight int) string { // termWidth so frame width is stable. bodyLines := splitBodyToHeight(body, bodyHeight, termWidth) - footerText := "←/→ switch agent · Esc back · PgUp/PgDn scroll" + footerText := "←/→ switch agent · Esc back · scroll: PgUp/PgDn/↑/↓/Home/End" footer := padDisplayWidth(footerText, termWidth) var b strings.Builder diff --git a/cmd/entire/cli/review/tui_sink.go b/cmd/entire/cli/review/tui_sink.go index bda351469d..e57c6d37db 100644 --- a/cmd/entire/cli/review/tui_sink.go +++ b/cmd/entire/cli/review/tui_sink.go @@ -24,10 +24,12 @@ import ( // orchestrator's dispatch goroutine. // // Cancellation: cancel is the same context.CancelFunc that controls the -// orchestrator's run context. KeyCtrlC in the dashboard calls this function -// (gated by a sync.Once in the model). Out-of-TUI SIGINT routes through the -// cobra root's context, which cancels the same function — no parallel -// signal.Notify goroutine is needed here. +// orchestrator's run context. The first KeyCtrlC in the dashboard fires this +// function (guarded by a sync.Once in the model) and switches the dashboard +// to a "Cancelling agents..." indicator while agents drain; a second KeyCtrlC +// force-quits without waiting. Out-of-TUI SIGINT routes through the cobra +// root's context, which cancels the same function — no parallel signal.Notify +// goroutine is needed here. type TUISink struct { program *tea.Program From e22f8a79f55fee0ad6a81c1f6de9deaa9a582fc0 Mon Sep 17 00:00:00 2001 From: Peyton Montei Date: Tue, 12 May 2026 00:29:34 -0400 Subject: [PATCH 08/11] review: tighten Ctrl+C and post-finish key handling around edge cases MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two related fixes to the TUI's cancellation and post-finish dismissal: 1. Ctrl+C with all agents already terminal short-circuits to tea.Quit instead of setting cancelling=true and firing the CancelFunc. The race window between the last agent's Finished event and the orchestrator's runFinishedMsg was flashing "Cancelling agents..." for a user who effectively had nothing left to cancel. New allAgentsTerminal() helper gates the cancelling path on whether any row is still AgentStatusUnknown. 2. q / Enter / Ctrl+C in detail mode AFTER the run finishes now quit directly. Previously they fell through to the viewport (which has no quit binding) and were silently inert — the user had to Esc to the dashboard first, then press an exit key. Esc itself keeps its "back to dashboard" meaning so the post-finish dashboard is still reachable from detail mode. Removes the now-unused isPostFinishExitKey helper in favor of an inlined switch that handles both dashboard and detail-mode post-finish cases together. Three new tests pin the changed behaviors: - TestTUIModel_CtrlCAfterAllRowsTerminalQuitsImmediately - TestTUIModel_PostFinishInDetailMode_ExitKeysQuit (q/Enter/Ctrl+C) - TestTUIModel_PostFinishInDetailMode_EscReturnsToDashboard Co-Authored-By: Claude Opus 4.7 (1M context) Entire-Checkpoint: 216d1bdc0348 --- cmd/entire/cli/review/tui_model.go | 60 ++++++++----- cmd/entire/cli/review/tui_model_test.go | 107 ++++++++++++++++++++++++ 2 files changed, 146 insertions(+), 21 deletions(-) diff --git a/cmd/entire/cli/review/tui_model.go b/cmd/entire/cli/review/tui_model.go index ef09354f0f..eab4b66fe4 100644 --- a/cmd/entire/cli/review/tui_model.go +++ b/cmd/entire/cli/review/tui_model.go @@ -309,13 +309,24 @@ func (m reviewTUIModel) handleAgentEvent(msg agentEventMsg) (tea.Model, tea.Cmd) // handleKey processes keyboard input. func (m reviewTUIModel) handleKey(msg tea.KeyPressMsg) (tea.Model, tea.Cmd) { - // Post-finish dashboard: only explicit exit keys dismiss. Any other key - // (including Ctrl+O) falls through to normal handling so the user can - // still drill into completed agent output. Ctrl+C also quits immediately - // — there are no agents left to drain. - if m.finished && !m.detailMode { - if isPostFinishExitKey(msg) || (msg.Code == 'c' && msg.Mod == tea.ModCtrl) { + // Post-finish: explicit exit keys dismiss the TUI from either the + // dashboard or detail mode. Esc retains its "back to dashboard" meaning + // while drilled in so the user can return to the finished dashboard + // before quitting. Other keys (including Ctrl+O and scroll keys) fall + // through to normal handling so post-mortem inspection still works. + if m.finished { + switch { + case msg.Mod == 0 && msg.Code == 'q': + return m, tea.Quit + case msg.Mod == 0 && msg.Code == tea.KeyEnter: return m, tea.Quit + case msg.Mod == tea.ModCtrl && msg.Code == 'c': + return m, tea.Quit + case msg.Mod == 0 && (msg.Code == tea.KeyEscape || msg.Code == tea.KeyEsc): + if !m.detailMode { + return m, tea.Quit + } + // Detail mode: fall through to main switch where Esc returns to dashboard. } } @@ -325,10 +336,17 @@ func (m reviewTUIModel) handleKey(msg tea.KeyPressMsg) (tea.Model, tea.Cmd) { // In drill-in: Ctrl+C is intentionally ignored; Esc first. return m, nil } - // Second Ctrl+C while a cancellation is already in flight force-quits - // without waiting for agents to drain. The shared CancelFunc is - // guarded by cancelOnce, so the second press doesn't double-cancel. if m.cancelling { + // Second Ctrl+C while a cancellation is already in flight + // force-quits without waiting for agents to drain. cancelOnce + // guards CancelFunc against the duplicate-firing case. + return m, tea.Quit + } + if m.allAgentsTerminal() { + // Race window: every agent emitted a terminal event but + // runFinishedMsg hasn't arrived yet. There's nothing left to + // cancel — quit immediately instead of flashing the + // "Cancelling agents..." footer until the runFinishedMsg lands. return m, tea.Quit } m.cancelling = true @@ -391,21 +409,21 @@ func (m reviewTUIModel) handleKey(msg tea.KeyPressMsg) (tea.Model, tea.Cmd) { return m, nil } -// isPostFinishExitKey reports whether msg is one of the explicit exit keys -// honored on the post-finish dashboard. Ctrl+C is handled by the caller because -// it has a modifier; here we only need q/Esc/Enter without modifiers. -func isPostFinishExitKey(msg tea.KeyPressMsg) bool { - if msg.Mod != 0 { +// allAgentsTerminal reports whether every agent row has reached a terminal +// status. Used to short-circuit Ctrl+C cancellation in the race window between +// the last agent emitting Finished/RunError and runFinishedMsg arriving — at +// that point CancelFunc has nothing to do and the "Cancelling agents..." +// footer would only flash briefly before runFinishedMsg dismisses it anyway. +func (m reviewTUIModel) allAgentsTerminal() bool { + if len(m.rows) == 0 { return false } - if msg.Code == tea.KeyEscape || msg.Code == tea.KeyEsc { - return true - } - switch msg.Code { - case 'q', tea.KeyEnter: - return true + for _, r := range m.rows { + if r.status == reviewtypes.AgentStatusUnknown { + return false + } } - return false + return true } // View renders the current state. diff --git a/cmd/entire/cli/review/tui_model_test.go b/cmd/entire/cli/review/tui_model_test.go index 4843ed3641..15a74075c0 100644 --- a/cmd/entire/cli/review/tui_model_test.go +++ b/cmd/entire/cli/review/tui_model_test.go @@ -796,3 +796,110 @@ func TestTUIModel_RunErrorSticky_FinishedDoesNotFlipToSucceeded(t *testing.T) { t.Errorf("expected Failed to stick (RunError is sticky), got %v", m.rows[0].status) } } + +// TestTUIModel_CtrlCAfterAllRowsTerminalQuitsImmediately pins that Ctrl+C +// during the race window between an agent's terminal event (Finished or +// RunError) and the orchestrator's runFinishedMsg short-circuits to tea.Quit +// instead of flashing the "Cancelling agents..." indicator. The pre-fix +// behavior fired CancelFunc and set m.cancelling=true even though every +// agent had already finished — confusing the user for the brief window +// before the dashboard transitioned to its post-finish state. +func TestTUIModel_CtrlCAfterAllRowsTerminalQuitsImmediately(t *testing.T) { + t.Parallel() + var called atomic.Bool + cancel := func() { called.Store(true) } + m := newTestModel([]string{"agent-a", "agent-b"}, cancel) + + // Drive both rows to a terminal status via events. runFinishedMsg is NOT + // sent — that's the race window we're testing. + updated, _ := m.Update(agentEventMsg{agent: "agent-a", ev: reviewtypes.Finished{Success: true}}) + m = mustModel(t, updated) + updated, _ = m.Update(agentEventMsg{agent: "agent-b", ev: reviewtypes.Finished{Success: true}}) + m = mustModel(t, updated) + if m.finished { + t.Fatal("setup: m.finished should be false (runFinishedMsg not sent)") + } + + updated, cmd := m.Update(testCtrlKey('c')) + m = mustModel(t, updated) + if m.cancelling { + t.Error("Ctrl+C with all rows already terminal must NOT set cancelling=true") + } + if called.Load() { + t.Error("CancelFunc must not fire when there's nothing left to cancel") + } + if cmd == nil { + t.Fatal("expected a quit command when Ctrl+C arrives with all rows terminal") + } + if _, ok := cmd().(tea.QuitMsg); !ok { + t.Errorf("expected tea.QuitMsg, got %T", cmd()) + } +} + +// TestTUIModel_PostFinishInDetailMode_ExitKeysQuit pins that q/Enter/Ctrl+C +// pressed while drilled in AFTER the run finished dismiss the TUI directly +// instead of being swallowed by the viewport (which has no quit binding). +// Pre-fix the user had to Esc out of detail mode first, then press an exit +// key — a two-step dismissal with no on-screen hint that q/Enter were inert. +func TestTUIModel_PostFinishInDetailMode_ExitKeysQuit(t *testing.T) { + t.Parallel() + cases := []struct { + name string + key tea.KeyPressMsg + }{ + {"q", testKey('q')}, + {"Enter", testKey(tea.KeyEnter)}, + {"Ctrl+C", testCtrlKey('c')}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + m := newTestModel([]string{"agent-a"}, func() {}) + updated, _ := m.Update(runFinishedMsg{summary: reviewtypes.RunSummary{}}) + m = mustModel(t, updated) + updated, _ = m.Update(testCtrlKey('o')) + m = mustModel(t, updated) + if !m.detailMode { + t.Fatalf("setup: expected detailMode=true after Ctrl+O") + } + + _, cmd := m.Update(tc.key) + if cmd == nil { + t.Fatalf("expected a command for key %q post-finish in detail mode", tc.name) + } + if _, ok := cmd().(tea.QuitMsg); !ok { + t.Errorf("expected tea.QuitMsg for key %q post-finish in detail mode, got %T", tc.name, cmd()) + } + }) + } +} + +// TestTUIModel_PostFinishInDetailMode_EscReturnsToDashboard pins that Esc +// preserves its "back to dashboard" meaning even after the run finishes, +// rather than quitting outright. The user can then dismiss from the +// dashboard via q/Esc/Enter/Ctrl+C. Regression guard for the post-finish +// detail-mode dismissal change. +func TestTUIModel_PostFinishInDetailMode_EscReturnsToDashboard(t *testing.T) { + t.Parallel() + m := newTestModel([]string{"agent-a"}, func() {}) + updated, _ := m.Update(runFinishedMsg{summary: reviewtypes.RunSummary{}}) + m = mustModel(t, updated) + updated, _ = m.Update(testCtrlKey('o')) + m = mustModel(t, updated) + if !m.detailMode { + t.Fatalf("setup: expected detailMode=true after Ctrl+O") + } + + updated, cmd := m.Update(testKey(tea.KeyEscape)) + m = mustModel(t, updated) + if m.detailMode { + t.Error("Esc post-finish in detail mode must return to dashboard, not quit") + } + if cmd != nil { + if msg := cmd(); msg != nil { + if _, ok := msg.(tea.QuitMsg); ok { + t.Error("Esc post-finish in detail mode must NOT quit; it returns to dashboard") + } + } + } +} From 93be14c4f3ebe3e2f209c648388045bf27b298f1 Mon Sep 17 00:00:00 2001 From: Peyton Montei Date: Tue, 12 May 2026 00:33:27 -0400 Subject: [PATCH 09/11] review: lock mouse-event routing and cancelling-indicator gating with tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two regression-guard tests covering behaviors that are correct in the current implementation but were unpinned: - TestTUIModel_DelegatesNonWheelMouseEventsToViewport asserts that the Update case-arm routing mouse events to the viewport covers Click, Release, and Motion in addition to Wheel. A future refactor narrowing the case-arm to wheel-only would silently break click-drag selection inside the drill-in while still passing the existing wheel test. The viewport does not expose selection state publicly, so the test pins the weaker invariant that the events do not panic and detailMode survives — enough to catch the case-arm-removal regression. - TestTUIModel_CancellingIndicatorOnlyAffectsRunningRows asserts that renderRow only swaps "running" for "cancelling" — rows already in a terminal status keep their own indicator (e.g. "✓ done"). Without this pin a future refactor could surface "cancelling" on Succeeded rows, misleading the user about which agents actually got cancelled. Co-Authored-By: Claude Opus 4.7 (1M context) Entire-Checkpoint: 787efb0b4aa5 --- cmd/entire/cli/review/tui_model_test.go | 81 +++++++++++++++++++++++++ 1 file changed, 81 insertions(+) diff --git a/cmd/entire/cli/review/tui_model_test.go b/cmd/entire/cli/review/tui_model_test.go index 15a74075c0..4e7af14342 100644 --- a/cmd/entire/cli/review/tui_model_test.go +++ b/cmd/entire/cli/review/tui_model_test.go @@ -797,6 +797,87 @@ func TestTUIModel_RunErrorSticky_FinishedDoesNotFlipToSucceeded(t *testing.T) { } } +// TestTUIModel_DelegatesNonWheelMouseEventsToViewport pins that the Update +// case-arm routing mouse events to the viewport covers Click, Release, and +// Motion in addition to Wheel. The viewport's selection support (click-drag +// highlight in terminals that emit cell-motion events) needs the full event +// stream, not just scroll. A future refactor that narrowed the case-arm to +// wheel-only would silently break selection while still passing +// TestTUIModel_DelegatesMouseWheelToViewport. +// +// Coverage limitation: the viewport's internal selection state is not +// exposed via a public getter, so this test asserts the weaker invariant +// that the events do not panic and that detailMode survives. That catches +// the regression where the case-arm is removed entirely (and the events +// fall through Update's catch-all to return m, nil); it does not catch +// finer-grained changes to selection semantics inside the viewport. +func TestTUIModel_DelegatesNonWheelMouseEventsToViewport(t *testing.T) { + t.Parallel() + cases := []struct { + name string + msg tea.Msg + }{ + {"click", tea.MouseClickMsg{}}, + {"release", tea.MouseReleaseMsg{}}, + {"motion", tea.MouseMotionMsg{}}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + m := newTestModel([]string{"agent-a"}, func() {}) + updated, _ := m.Update(tea.WindowSizeMsg{Width: 40, Height: 10}) + m = mustModel(t, updated) + updated, _ = m.Update(testCtrlKey('o')) + m = mustModel(t, updated) + if !m.detailMode { + t.Fatalf("setup: expected detailMode=true after Ctrl+O") + } + + updated, _ = m.Update(tc.msg) + m = mustModel(t, updated) + if !m.detailMode { + t.Errorf("detailMode should remain true after mouse %s event", tc.name) + } + }) + } +} + +// TestTUIModel_CancellingIndicatorOnlyAffectsRunningRows pins that the +// "cancelling" status indicator in renderRow only replaces "running" — rows +// already in a terminal status keep their own indicator. Without this gate +// the post-Ctrl+C frame would briefly show every row as "cancelling" before +// the dashboard transitioned to its post-finish state, including agents +// that had already succeeded. +func TestTUIModel_CancellingIndicatorOnlyAffectsRunningRows(t *testing.T) { + t.Parallel() + m := newTestModel([]string{"agent-a", "agent-b"}, func() {}) + m.termWidth = 120 + // agent-a has already succeeded; agent-b is still running. + updated, _ := m.Update(agentEventMsg{agent: "agent-a", ev: reviewtypes.Started{}}) + m = mustModel(t, updated) + updated, _ = m.Update(agentEventMsg{agent: "agent-a", ev: reviewtypes.Finished{Success: true}}) + m = mustModel(t, updated) + updated, _ = m.Update(agentEventMsg{agent: "agent-b", ev: reviewtypes.Started{}}) + m = mustModel(t, updated) + + // Flip cancelling without going through Ctrl+C so the + // allAgentsTerminal() short-circuit doesn't fire. + m.cancelling = true + + gotA := m.renderRow(m.rows[0]) + if !strings.Contains(gotA, "done") { + t.Errorf("succeeded row should retain ✓ done indicator while cancelling; got %q", gotA) + } + if strings.Contains(gotA, "cancelling") { + t.Errorf("succeeded row must not show 'cancelling' indicator; got %q", gotA) + } + + gotB := m.renderRow(m.rows[1]) + if !strings.Contains(gotB, "cancelling") { + t.Errorf("running row should show cancelling indicator; got %q", gotB) + } +} + // TestTUIModel_CtrlCAfterAllRowsTerminalQuitsImmediately pins that Ctrl+C // during the race window between an agent's terminal event (Finished or // RunError) and the orchestrator's runFinishedMsg short-circuits to tea.Quit From c36e0437371197dbf58d7d67f6ce61fd8d078130 Mon Sep 17 00:00:00 2001 From: Peyton Montei Date: Tue, 12 May 2026 20:28:59 -0400 Subject: [PATCH 10/11] review: force-quit from drill-in during cancel + small TUI cleanups MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four small follow-ups from review of this PR: 1. Reorder the Ctrl+C handler so cancelling beats detailMode. The dashboard footer hint "Ctrl+C again: force quit" was unreachable from inside the drill-in view: detailMode was checked first and silently swallowed the keypress. A user who hits Ctrl+C, drills in to watch a hanging agent, and then wants out had to Esc back to the dashboard first — the exact failure mode the force-quit escape hatch exists for, blocked in the one context where it matters most. The idle "Esc first" guard still applies when cancelling=false. 2. Hoist defaultTermWidth/defaultTermHeight to package level. They were local to newReviewTUIModel and the dashboardWidth() / detailViewportWidth() fallbacks duplicated the 80 literal — except detailViewportWidth() actually fell back to 1, which would collapse the viewport to a single column. Now both functions share the same package-level constant. 3. Resolve the tea.KeyEscape || tea.KeyEsc redundancy. Per ultraviolet/key.go:224, KeyEsc = KeyEscape (literal alias) — the disjunction was dead code. Kept KeyEscape (canonical name). 4. SetContent("") → SetContentLines(nil) on the empty-buffer path. refreshDetailContentWithAutoTail mixed the two viewport APIs in a single function; nil-lines is the symmetric twin to the populated SetContentLines call below it. New test pins the cancelling-beats-detailMode contract: TestTUIModel_SecondCtrlCForceQuits_FromDetailMode Co-Authored-By: Claude Opus 4.7 (1M context) --- cmd/entire/cli/review/tui_model.go | 42 ++++++++++++++++--------- cmd/entire/cli/review/tui_model_test.go | 40 +++++++++++++++++++++++ 2 files changed, 67 insertions(+), 15 deletions(-) diff --git a/cmd/entire/cli/review/tui_model.go b/cmd/entire/cli/review/tui_model.go index eab4b66fe4..e38ad4a1e9 100644 --- a/cmd/entire/cli/review/tui_model.go +++ b/cmd/entire/cli/review/tui_model.go @@ -23,6 +23,16 @@ import ( "github.com/entireio/cli/cmd/entire/cli/stringutil" ) +// Default terminal dimensions used before the first tea.WindowSizeMsg +// arrives. Shared by the constructor and the dashboardWidth / +// detailViewportWidth fallbacks so both views render at the same width when +// termWidth is uninitialized — a 1-cell viewport falls back here would +// collapse to a single column, which is not what we want. +const ( + defaultTermWidth = 80 + defaultTermHeight = 24 +) + // agentRow holds per-agent live state during the TUI run. type agentRow struct { name string @@ -97,10 +107,6 @@ func newReviewTUIModel(agents []string, cancel context.CancelFunc) reviewTUIMode } // Seed viewport with defaults that match termWidth/termHeight so an // immediate Ctrl+O before any WindowSizeMsg still renders. - const ( - defaultTermWidth = 80 - defaultTermHeight = 24 - ) vp := viewport.New( viewport.WithWidth(defaultTermWidth), viewport.WithHeight(defaultTermHeight-2), @@ -207,7 +213,7 @@ func (m reviewTUIModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) { // detailViewportWidth returns the viewport's width, mirroring termWidth. func (m reviewTUIModel) detailViewportWidth() int { if m.termWidth < 1 { - return 1 + return defaultTermWidth } return m.termWidth } @@ -237,7 +243,7 @@ func (m reviewTUIModel) refreshDetailContent() reviewTUIModel { func (m reviewTUIModel) refreshDetailContentWithAutoTail(wasAtBottom bool) reviewTUIModel { if len(m.rows) == 0 || m.detailIdx < 0 || m.detailIdx >= len(m.rows) { - m.detail.SetContent("") + m.detail.SetContentLines(nil) return m } lines := buildEventLines(m.rows[m.detailIdx].buffer, m.detailViewportWidth()) @@ -322,7 +328,7 @@ func (m reviewTUIModel) handleKey(msg tea.KeyPressMsg) (tea.Model, tea.Cmd) { return m, tea.Quit case msg.Mod == tea.ModCtrl && msg.Code == 'c': return m, tea.Quit - case msg.Mod == 0 && (msg.Code == tea.KeyEscape || msg.Code == tea.KeyEsc): + case msg.Mod == 0 && (msg.Code == tea.KeyEscape): if !m.detailMode { return m, tea.Quit } @@ -332,16 +338,22 @@ func (m reviewTUIModel) handleKey(msg tea.KeyPressMsg) (tea.Model, tea.Cmd) { switch { case msg.Code == 'c' && msg.Mod == tea.ModCtrl: - if m.detailMode { - // In drill-in: Ctrl+C is intentionally ignored; Esc first. - return m, nil - } if m.cancelling { // Second Ctrl+C while a cancellation is already in flight - // force-quits without waiting for agents to drain. cancelOnce - // guards CancelFunc against the duplicate-firing case. + // force-quits without waiting for agents to drain. Checked + // before m.detailMode so the force-quit escape hatch works + // from drill-in too — the dashboard footer hint promises this + // path and a user drilled into a hanging agent's buffer is + // exactly when they need force-quit most. cancelOnce guards + // CancelFunc against the duplicate-firing case. return m, tea.Quit } + if m.detailMode { + // Idle drill-in: Ctrl+C is intentionally ignored so the user + // reading content can't accidentally fire a cancel. Esc first + // to return to the dashboard. + return m, nil + } if m.allAgentsTerminal() { // Race window: every agent emitted a terminal event but // runFinishedMsg hasn't arrived yet. There's nothing left to @@ -374,7 +386,7 @@ func (m reviewTUIModel) handleKey(msg tea.KeyPressMsg) (tea.Model, tea.Cmd) { m.detail.GotoBottom() return m, nil - case msg.Code == tea.KeyEscape || msg.Code == tea.KeyEsc: + case msg.Code == tea.KeyEscape: if m.detailMode { m.detailMode = false return m, nil @@ -480,7 +492,7 @@ func (m reviewTUIModel) writeDashboardLine(b *strings.Builder, line string) { func (m reviewTUIModel) dashboardWidth() int { if m.termWidth <= 0 { - return 80 + return defaultTermWidth } return m.termWidth } diff --git a/cmd/entire/cli/review/tui_model_test.go b/cmd/entire/cli/review/tui_model_test.go index 4e7af14342..235257a3be 100644 --- a/cmd/entire/cli/review/tui_model_test.go +++ b/cmd/entire/cli/review/tui_model_test.go @@ -541,6 +541,46 @@ func TestTUIModel_SecondCtrlCForceQuits(t *testing.T) { } } +// TestTUIModel_SecondCtrlCForceQuits_FromDetailMode locks the force-quit +// escape hatch from inside the drill-in view. When cancelling is already +// in flight, the dashboard footer promises "Ctrl+C again: force quit" — +// a user who drilled into a hanging agent's buffer to diagnose the hang +// needs that promise to hold from drill-in too. Without the cancelling- +// before-detailMode precedence in handleKey, Ctrl+C in this state was +// silently swallowed. +func TestTUIModel_SecondCtrlCForceQuits_FromDetailMode(t *testing.T) { + t.Parallel() + var count atomic.Int32 + cancel := func() { count.Add(1) } + + m := newTestModel([]string{"agent-a"}, cancel) + // First Ctrl+C on dashboard initiates cancellation. + updated, _ := m.Update(testCtrlKey('c')) + m = mustModel(t, updated) + if !m.cancelling { + t.Fatal("setup: expected cancelling=true after first Ctrl+C") + } + // Drill in to inspect the hanging agent. + updated, _ = m.Update(testCtrlKey('o')) + m = mustModel(t, updated) + if !m.detailMode { + t.Fatal("setup: expected detailMode=true after Ctrl+O") + } + + // Second Ctrl+C while cancelling AND in drill-in must force-quit. + _, cmd := m.Update(testCtrlKey('c')) + if cmd == nil { + t.Fatal("expected a command from second Ctrl+C in detail mode while cancelling") + } + msg := cmd() + if _, ok := msg.(tea.QuitMsg); !ok { + t.Errorf("expected tea.QuitMsg from second Ctrl+C in detail mode while cancelling, got %T", msg) + } + if got := count.Load(); got != 1 { + t.Errorf("CancelFunc should fire exactly once; got %d", got) + } +} + // TestTUIModel_AutoFollow_DetailMode pins that when the viewport is sitting // at the bottom and a new event arrives, the model snaps back to bottom so // the user keeps seeing the tail. The viewport's AtBottom() drives this. From b5645e7e723b0b8900c014554de9babf471a2af5 Mon Sep 17 00:00:00 2001 From: Peyton Montei Date: Wed, 13 May 2026 16:11:46 -0400 Subject: [PATCH 11/11] review: tighten TUI drill-in edge cases from PR-1184 review Fix three follow-ups surfaced while re-reading the drill-in code: - wrapDisplayWidth: strip trailing newlines so "text\n" yields one line, not a phantom blank tail. Adds tui_text_test.go covering empty input, ANSI/control stripping, paragraph breaks, and the trailing-newline case. - handleKey: check allAgentsTerminal() before the detail-mode Ctrl+C swallow so a user reading a finished agent's buffer can dismiss without pressing Esc first. - splitBodyToHeight docstring: clarify that the bodyHeight truncation path is a defensive guard, since the viewport already clips View() output to Height(). Co-Authored-By: Claude Opus 4.7 (1M context) Entire-Checkpoint: b6f2dac09988 --- cmd/entire/cli/review/tui_detail.go | 6 +- cmd/entire/cli/review/tui_model.go | 15 +++-- cmd/entire/cli/review/tui_model_test.go | 45 +++++++++++++ cmd/entire/cli/review/tui_text.go | 12 +++- cmd/entire/cli/review/tui_text_test.go | 86 +++++++++++++++++++++++++ 5 files changed, 154 insertions(+), 10 deletions(-) create mode 100644 cmd/entire/cli/review/tui_text_test.go diff --git a/cmd/entire/cli/review/tui_detail.go b/cmd/entire/cli/review/tui_detail.go index 14d2e7a034..d5e7e121e3 100644 --- a/cmd/entire/cli/review/tui_detail.go +++ b/cmd/entire/cli/review/tui_detail.go @@ -113,8 +113,10 @@ func detailFrame(row agentRow, body string, termWidth, termHeight int) string { } // splitBodyToHeight normalizes a multi-line body string to exactly bodyHeight -// lines, each truncated and padded to termWidth. Lines beyond bodyHeight are -// dropped; missing lines are padded with spaces. +// lines, each truncated and padded to termWidth. Missing lines are padded +// with spaces. The bodyHeight cap is a defensive guard: viewport.View() +// should already clip to its Height(), so the overflow-truncation path is +// not expected to trigger in normal use. func splitBodyToHeight(body string, bodyHeight, termWidth int) []string { if bodyHeight <= 0 { return nil diff --git a/cmd/entire/cli/review/tui_model.go b/cmd/entire/cli/review/tui_model.go index e597a5ef1b..9c8770715a 100644 --- a/cmd/entire/cli/review/tui_model.go +++ b/cmd/entire/cli/review/tui_model.go @@ -360,19 +360,22 @@ func (m reviewTUIModel) handleKey(msg tea.KeyPressMsg) (tea.Model, tea.Cmd) { // CancelFunc against the duplicate-firing case. return m, tea.Quit } - if m.detailMode { - // Idle drill-in: Ctrl+C is intentionally ignored so the user - // reading content can't accidentally fire a cancel. Esc first - // to return to the dashboard. - return m, nil - } if m.allAgentsTerminal() { // Race window: every agent emitted a terminal event but // runFinishedMsg hasn't arrived yet. There's nothing left to // cancel — quit immediately instead of flashing the // "Cancelling agents..." footer until the runFinishedMsg lands. + // Checked before m.detailMode so the user reading a finished + // agent's buffer doesn't have to press Esc first to dismiss. return m, tea.Quit } + if m.detailMode { + // Idle drill-in with at least one agent still running: Ctrl+C + // is intentionally ignored so the user reading content can't + // accidentally fire a cancel. Esc first to return to the + // dashboard. + return m, nil + } m.cancelling = true m.cancelOnce.Do(m.cancel) // Do NOT quit on the first Ctrl+C: leave the TUI up so the user sees diff --git a/cmd/entire/cli/review/tui_model_test.go b/cmd/entire/cli/review/tui_model_test.go index 9382c245c8..99ff586f61 100644 --- a/cmd/entire/cli/review/tui_model_test.go +++ b/cmd/entire/cli/review/tui_model_test.go @@ -1109,6 +1109,51 @@ func TestTUIModel_CtrlCAfterAllRowsTerminalQuitsImmediately(t *testing.T) { } } +// TestTUIModel_CtrlCInDetailModeAfterAllRowsTerminalQuits pins that Ctrl+C +// pressed inside drill-in during the narrow race window where every agent +// has emitted a terminal event but the orchestrator's runFinishedMsg has +// not arrived yet quits the TUI instead of being swallowed. Without the +// detail-mode gate on allAgentsTerminal(), the user reading a completed +// agent's buffer would have to press Esc first to return to the dashboard +// before Ctrl+C took effect — needlessly two-step when there is nothing +// left to cancel. Companion to +// [TestTUIModel_CtrlCAfterAllRowsTerminalQuitsImmediately] for the +// dashboard-mode case. +func TestTUIModel_CtrlCInDetailModeAfterAllRowsTerminalQuits(t *testing.T) { + t.Parallel() + var called atomic.Bool + cancel := func() { called.Store(true) } + m := newTestModel([]string{"agent-a", "agent-b"}, cancel) + + updated, _ := m.Update(agentEventMsg{agent: "agent-a", ev: reviewtypes.Finished{Success: true}}) + m = mustModel(t, updated) + updated, _ = m.Update(agentEventMsg{agent: "agent-b", ev: reviewtypes.Finished{Success: true}}) + m = mustModel(t, updated) + updated, _ = m.Update(testCtrlKey('o')) + m = mustModel(t, updated) + if !m.detailMode { + t.Fatal("setup: expected detailMode=true after Ctrl+O") + } + if m.finished { + t.Fatal("setup: m.finished should be false (runFinishedMsg not sent)") + } + + updated, cmd := m.Update(testCtrlKey('c')) + m = mustModel(t, updated) + if m.cancelling { + t.Error("Ctrl+C in detail mode with all rows terminal must NOT set cancelling=true") + } + if called.Load() { + t.Error("CancelFunc must not fire when there is nothing left to cancel") + } + if cmd == nil { + t.Fatal("expected a quit command when Ctrl+C arrives in detail mode with all rows terminal") + } + if _, ok := cmd().(tea.QuitMsg); !ok { + t.Errorf("expected tea.QuitMsg, got %T", cmd()) + } +} + // TestTUIModel_PostFinishInDetailMode_ExitKeysQuit pins that q/Enter/Ctrl+C // pressed while drilled in AFTER the run finished dismiss the TUI directly // instead of being swallowed by the viewport (which has no quit binding). diff --git a/cmd/entire/cli/review/tui_text.go b/cmd/entire/cli/review/tui_text.go index 99c2624dd2..243f168bb0 100644 --- a/cmd/entire/cli/review/tui_text.go +++ b/cmd/entire/cli/review/tui_text.go @@ -63,9 +63,17 @@ func truncateDisplayWidth(s string, width int) string { // wraps to nothing still contributes an empty line, preserving blank-line // structure between paragraphs. // -// Returns nil for width <= 0 or empty input. +// Trailing newlines are stripped before splitting so "text\n" yields a single +// line, not a phantom blank tail — matching how splitBodyToHeight trims its +// input. +// +// Returns nil for width <= 0 or input that is empty (or only newlines). func wrapDisplayWidth(s string, width int) []string { - if width <= 0 || s == "" { + if width <= 0 { + return nil + } + s = strings.TrimRight(s, "\n") + if s == "" { return nil } paragraphs := strings.Split(s, "\n") diff --git a/cmd/entire/cli/review/tui_text_test.go b/cmd/entire/cli/review/tui_text_test.go new file mode 100644 index 0000000000..275dccb626 --- /dev/null +++ b/cmd/entire/cli/review/tui_text_test.go @@ -0,0 +1,86 @@ +package review + +import ( + "reflect" + "testing" +) + +func TestWrapDisplayWidth(t *testing.T) { + t.Parallel() + tests := []struct { + name string + in string + width int + want []string + }{ + { + name: "empty input returns nil", + in: "", + width: 80, + want: nil, + }, + { + name: "zero width returns nil", + in: "anything", + width: 0, + want: nil, + }, + { + name: "negative width returns nil", + in: "anything", + width: -10, + want: nil, + }, + { + name: "short single line fits", + in: "hello", + width: 80, + want: []string{"hello"}, + }, + { + name: "long line wraps to width", + in: "aaaa bbbb cccc", + width: 5, + want: []string{"aaaa", "bbbb", "cccc"}, + }, + { + name: "embedded newline preserved as paragraph break", + in: "a\n\nb", + width: 80, + want: []string{"a", "", "b"}, + }, + { + name: "trailing newline does not produce phantom blank line", + in: "text\n", + width: 80, + want: []string{"text"}, + }, + { + name: "multiple trailing newlines collapsed", + in: "text\n\n\n", + width: 80, + want: []string{"text"}, + }, + { + name: "ANSI escape stripped from output", + in: "\x1b[31mred\x1b[0m text", + width: 80, + want: []string{"red text"}, + }, + { + name: "control chars stripped", + in: "a\x07b", + width: 80, + want: []string{"ab"}, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + got := wrapDisplayWidth(tt.in, tt.width) + if !reflect.DeepEqual(got, tt.want) { + t.Errorf("wrapDisplayWidth(%q, %d) = %#v, want %#v", tt.in, tt.width, got, tt.want) + } + }) + } +}