From bf5b12c863ba50e92ce20a90e0ef2638831cf332 Mon Sep 17 00:00:00 2001 From: Pavel Fadeev Date: Thu, 19 Mar 2026 21:34:19 +0100 Subject: [PATCH] UX: pill spacing fix + PR review toggle (r key) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pill spacing: - Passive pills: Padding(0,0) → Padding(0,1) for breathing room - Passive name length: 4 → 6 chars (recognizable names) - Remove state-group summary from strip (redundant with status bar) PR review toggle: - Repurpose unused RunReview field as ReviewEnabled - ShouldReview() gated by ReviewEnabled - New ToggleReview() method + toggle_review ctl action - r key in TUI toggles review, shows flash - "review off" label in PR zoom header when disabled - Hint + help entry for r key - Migration: existing non-terminal PRs get ReviewEnabled=true --- daemon/internal/ctlserver/handler.go | 26 ++++++++++++++++ daemon/internal/pr/model.go | 5 +++- daemon/internal/pr/model_test.go | 19 ++++++++---- daemon/internal/pr/poller.go | 45 ++++++++++++++++++++++++++++ daemon/internal/pr/poller_test.go | 3 ++ tui/internal/client/client.go | 7 +++++ tui/internal/tui/app.go | 14 +++++++++ tui/internal/tui/hints.go | 2 ++ tui/internal/tui/pill.go | 6 ++-- tui/internal/tui/pr_zoom.go | 3 ++ tui/internal/tui/strip.go | 33 -------------------- 11 files changed, 120 insertions(+), 43 deletions(-) diff --git a/daemon/internal/ctlserver/handler.go b/daemon/internal/ctlserver/handler.go index 93fe59f..9ba4904 100644 --- a/daemon/internal/ctlserver/handler.go +++ b/daemon/internal/ctlserver/handler.go @@ -74,6 +74,8 @@ func (h *Handler) Handle(conn net.Conn) { h.handleCyclePRAutopilot(conn, req.PRKey) case "set_merge_method": h.handleSetMergeMethod(conn, req.PRKey, req.MergeMethod) + case "toggle_review": + h.handleToggleReview(conn, req.PRKey) } } } @@ -248,6 +250,30 @@ func (h *Handler) handleRemovePR(conn net.Conn, key string) { writeJSON(conn, ctlResponse{OK: &ok}) } +func (h *Handler) handleToggleReview(conn net.Conn, key string) { + if h.prPoll == nil { + f := false + writeJSON(conn, ctlResponse{OK: &f}) + return + } + parts := strings.SplitN(key, "#", 2) + if len(parts) != 2 { + f := false + writeJSON(conn, ctlResponse{OK: &f}) + return + } + ownerRepo := strings.SplitN(parts[0], "/", 2) + if len(ownerRepo) != 2 { + f := false + writeJSON(conn, ctlResponse{OK: &f}) + return + } + var number int + fmt.Sscanf(parts[1], "%d", &number) + ok := h.prPoll.ToggleReview(ownerRepo[0], ownerRepo[1], number) + writeJSON(conn, ctlResponse{OK: &ok}) +} + func writeJSON(conn net.Conn, v any) { data, _ := json.Marshal(v) data = append(data, '\n') diff --git a/daemon/internal/pr/model.go b/daemon/internal/pr/model.go index ada826b..7635bab 100644 --- a/daemon/internal/pr/model.go +++ b/daemon/internal/pr/model.go @@ -84,7 +84,7 @@ type TrackedPR struct { MaxHammer int `json:"max_hammer"` // max fix attempts (default 3) MergeMethod string `json:"merge_method"` // "squash", "merge", "rebase", "aviator", "" = unset MergeTriggered bool `json:"merge_triggered"` // true once auto-merge has been fired; resets on check regression - RunReview bool `json:"run_review"` // run code-review skill on creation + ReviewEnabled bool `json:"run_review"` // toggle code review on/off (independent of autopilot) // Agent pipeline state AgentRunning string `json:"agent_running,omitempty"` // "" | "fix_ci" | "review" | "fix_review" @@ -204,6 +204,9 @@ func (pr *TrackedPR) IsAgentRunning() bool { // ShouldReview returns true if the PR should be code-reviewed. func (pr *TrackedPR) ShouldReview() bool { + if !pr.ReviewEnabled { + return false + } if pr.AutopilotMode == PROff { return false } diff --git a/daemon/internal/pr/model_test.go b/daemon/internal/pr/model_test.go index 07b1f88..1e1fd50 100644 --- a/daemon/internal/pr/model_test.go +++ b/daemon/internal/pr/model_test.go @@ -425,40 +425,47 @@ func TestIsAgentRunning(t *testing.T) { // === ShouldReview === func TestShouldReview_ChecksPassing(t *testing.T) { - pr := TrackedPR{AutopilotMode: PRAuto, State: StateChecksPassing} + pr := TrackedPR{AutopilotMode: PRAuto, State: StateChecksPassing, ReviewEnabled: true} if !pr.ShouldReview() { - t.Error("AUTO + checks_passing + no review state should review") + t.Error("AUTO + checks_passing + review enabled should review") } } func TestShouldReview_AutopilotOff(t *testing.T) { - pr := TrackedPR{AutopilotMode: PROff, State: StateChecksPassing} + pr := TrackedPR{AutopilotMode: PROff, State: StateChecksPassing, ReviewEnabled: true} if pr.ShouldReview() { t.Error("autopilot off should not review") } } func TestShouldReview_ChecksFailing(t *testing.T) { - pr := TrackedPR{AutopilotMode: PRAuto, State: StateChecksFailing} + pr := TrackedPR{AutopilotMode: PRAuto, State: StateChecksFailing, ReviewEnabled: true} if pr.ShouldReview() { t.Error("checks failing should not review") } } func TestShouldReview_AlreadyReviewed(t *testing.T) { - pr := TrackedPR{AutopilotMode: PRAuto, State: StateChecksPassing, ReviewState: "clean"} + pr := TrackedPR{AutopilotMode: PRAuto, State: StateChecksPassing, ReviewState: "clean", ReviewEnabled: true} if pr.ShouldReview() { t.Error("already reviewed should not review again") } } func TestShouldReview_Approved(t *testing.T) { - pr := TrackedPR{AutopilotMode: PRAuto, State: StateApproved} + pr := TrackedPR{AutopilotMode: PRAuto, State: StateApproved, ReviewEnabled: true} if !pr.ShouldReview() { t.Error("approved state should also trigger review") } } +func TestShouldReview_ReviewDisabled(t *testing.T) { + pr := TrackedPR{AutopilotMode: PRAuto, State: StateChecksPassing, ReviewEnabled: false} + if pr.ShouldReview() { + t.Error("review disabled should not review") + } +} + // === ShouldFixReview === func TestShouldFixReview_HasIssues(t *testing.T) { diff --git a/daemon/internal/pr/poller.go b/daemon/internal/pr/poller.go index 374a6a4..e7e1d53 100644 --- a/daemon/internal/pr/poller.go +++ b/daemon/internal/pr/poller.go @@ -30,6 +30,7 @@ func NewPoller(storePath string, onChange func()) *Poller { storePath: storePath, } p.load() + p.migrateReviewEnabled() return p } @@ -56,6 +57,7 @@ func (p *Poller) Add(owner, repo string, number int) (*TrackedPR, bool) { AutopilotMode: PRAuto, Hammer: true, MaxHammer: 3, + ReviewEnabled: true, MergeMethod: method, Timeline: []PREvent{{Time: time.Now(), Icon: "📝", Message: "Added to tracking"}}, } @@ -167,6 +169,34 @@ func (p *Poller) CycleAutopilot(owner, repo string, number int) string { return pr.AutopilotMode } +// ToggleReview flips ReviewEnabled for a PR. +func (p *Poller) ToggleReview(owner, repo string, number int) bool { + key := fmt.Sprintf("%s/%s#%d", owner, repo, number) + p.mu.Lock() + defer p.mu.Unlock() + + pr, ok := p.tracked[key] + if !ok { + return false + } + + pr.ReviewEnabled = !pr.ReviewEnabled + label := "enabled" + if !pr.ReviewEnabled { + label = "disabled" + } + pr.Timeline = append(pr.Timeline, PREvent{ + Time: time.Now(), + Icon: "🔍", + Message: fmt.Sprintf("Code review %s", label), + }) + p.save() + if p.onChange != nil { + p.onChange() + } + return true +} + // FailingCount returns how many PRs have failing checks. func (p *Poller) FailingCount() int { p.mu.RLock() @@ -594,6 +624,21 @@ func (p *Poller) load() { p.tracked = prs } +// migrateReviewEnabled sets ReviewEnabled=true for non-terminal PRs that +// were persisted before the field existed (zero-value false). +func (p *Poller) migrateReviewEnabled() { + changed := false + for _, pr := range p.tracked { + if !pr.ReviewEnabled && pr.State != StateMerged && pr.State != StateClosed { + pr.ReviewEnabled = true + changed = true + } + } + if changed { + p.save() + } +} + func (p *Poller) save() { store := pollerStore{ PRs: p.tracked, diff --git a/daemon/internal/pr/poller_test.go b/daemon/internal/pr/poller_test.go index 96d9b9d..ca0d352 100644 --- a/daemon/internal/pr/poller_test.go +++ b/daemon/internal/pr/poller_test.go @@ -50,6 +50,9 @@ func TestAdd_New(t *testing.T) { if pr.MergeMethod != "" { t.Errorf("merge_method = %q, want empty (unset for new repo)", pr.MergeMethod) } + if !pr.ReviewEnabled { + t.Error("ReviewEnabled should default to true") + } if len(pr.Timeline) != 1 { t.Errorf("timeline should have 1 event, got %d", len(pr.Timeline)) } diff --git a/tui/internal/client/client.go b/tui/internal/client/client.go index 69bfc4c..8719eb8 100644 --- a/tui/internal/client/client.go +++ b/tui/internal/client/client.go @@ -68,6 +68,7 @@ type TrackedPR struct { Hammer bool `json:"hammer"` HammerCount int `json:"hammer_count"` MergeMethod string `json:"merge_method"` + ReviewEnabled bool `json:"run_review"` CreatedAt time.Time `json:"created_at"` Timeline []PREvent `json:"timeline"` @@ -271,6 +272,12 @@ func (c *Client) CyclePRAutopilot(key string) error { return err } +// TogglePRReview toggles code review on/off for a PR. +func (c *Client) TogglePRReview(key string) error { + _, err := c.sendCommand(request{Action: "toggle_review", PRKey: key}) + return err +} + // Focus focuses the Ghostty tab for the given session. func (c *Client) Focus(sessionID string) error { _, err := c.sendCommand(request{Action: "focus", SessionID: sessionID}) diff --git a/tui/internal/tui/app.go b/tui/internal/tui/app.go index b65aa1d..3f272fe 100644 --- a/tui/internal/tui/app.go +++ b/tui/internal/tui/app.go @@ -501,6 +501,20 @@ end tell`, tabIdx, tabIdx) } } + case "r": + // Toggle code review on/off for selected PR. + if pr := m.selectedPR(); pr != nil { + key := fmt.Sprintf("%s/%s#%d", pr.Owner, pr.Repo, pr.Number) + label := "review enabled" + if pr.ReviewEnabled { + label = "review disabled" + } + return m, func() tea.Msg { + err := m.client.TogglePRReview(key) + return actionResultMsg{action: label, err: err} + } + } + case "m": // Set merge method for selected PR — daemon handles the actual merge. if pr := m.selectedPR(); pr != nil { diff --git a/tui/internal/tui/hints.go b/tui/internal/tui/hints.go index 519096d..7e9376a 100644 --- a/tui/internal/tui/hints.go +++ b/tui/internal/tui/hints.go @@ -23,6 +23,7 @@ func renderHints(queueVisible bool, hasPending bool, isPRSelected bool, width in // PR-specific hints. keys = append(keys, hint{"Enter", "open PR"}) keys = append(keys, hint{"a", "autopilot"}) + keys = append(keys, hint{"r", "review"}) keys = append(keys, hint{"m", "method"}) keys = append(keys, hint{"+", "add PR"}) keys = append(keys, hint{"-", "remove"}) @@ -107,6 +108,7 @@ func renderHelp(width, height, scrollOffset int) string { {"", "Pull Requests"}, {"Enter", "Open PR in browser"}, {"a", "Cycle PR autopilot: OFF → AUTO → YOLO"}, + {"r", "Toggle code review on/off"}, {"+", "Add PR to tracking (paste URL)"}, {"-", "Remove selected PR"}, {"m", "Set merge method (daemon handles actual merge)"}, diff --git a/tui/internal/tui/pill.go b/tui/internal/tui/pill.go index ceb72ce..359c2cb 100644 --- a/tui/internal/tui/pill.go +++ b/tui/internal/tui/pill.go @@ -93,13 +93,13 @@ func isPassiveState(state string) bool { // pillNameMaxLen returns the max name length based on state and selection. // Selected pills show full 20-char names. // Active unselected (running/waiting): 8 chars — visible but compact. -// Passive unselected (idle/dead): 4 chars — minimal footprint. +// Passive unselected (idle/dead): 6 chars — compact but recognizable. func pillNameMaxLen(state string, selected bool) int { if selected { return 20 } if isPassiveState(state) { - return 4 + return 6 } return 8 // running, waiting, or other active states } @@ -140,7 +140,7 @@ func renderPillWithName(s client.Session, displayName string, selected bool, glo if compact { // Passive unselected pills: no background, just dim text — lighter visual weight. style = lipgloss.NewStyle(). - Padding(0, 0). + Padding(0, 1). Foreground(colorDimFg) } else { style = lipgloss.NewStyle(). diff --git a/tui/internal/tui/pr_zoom.go b/tui/internal/tui/pr_zoom.go index 758a254..4748145 100644 --- a/tui/internal/tui/pr_zoom.go +++ b/tui/internal/tui/pr_zoom.go @@ -50,6 +50,9 @@ func renderPRZoom(pr client.TrackedPR, width, height int, scrollOffset int) stri if pr.Hammer { line1 += " " + lipgloss.NewStyle().Foreground(colorOrange).Bold(true).Render("🔨") } + if !pr.ReviewEnabled { + line1 += " " + lipgloss.NewStyle().Foreground(colorDimFg).Render("review off") + } headerLines = append(headerLines, line1) // Line 2: branch → base +42 -12 3 commits mergeable diff --git a/tui/internal/tui/strip.go b/tui/internal/tui/strip.go index cb274aa..c39a0b2 100644 --- a/tui/internal/tui/strip.go +++ b/tui/internal/tui/strip.go @@ -251,39 +251,6 @@ func renderUnifiedStrip(sessions []client.Session, prs []client.TrackedPR, selec }) } - // Prepend a compact state-group summary when there are many sessions. - // Format: "▶2 ⏸1 ✔5 ●2" — lets user scan state distribution instantly. - // Done after PR processing so we can update selectedIdx and sepBoundary cleanly. - if len(sessions) >= 5 { - counts := map[string]int{} - for _, s := range sessions { - counts[s.State]++ - } - var parts []string - if n := counts["running"]; n > 0 { - parts = append(parts, fmt.Sprintf("\u25b6%d", n)) - } - if n := counts["waiting"]; n > 0 { - parts = append(parts, fmt.Sprintf("\u23f8%d", n)) - } - if n := counts["idle"]; n > 0 { - parts = append(parts, fmt.Sprintf("\u2714%d", n)) - } - if n := counts["dead"]; n > 0 { - parts = append(parts, fmt.Sprintf("\u25cf%d", n)) - } - if len(parts) > 0 { - summaryStr := lipgloss.NewStyle().Foreground(colorDimFg).Render(strings.Join(parts, " ")) - summaryPill := pillEntry{rendered: summaryStr, width: lipgloss.Width(summaryStr)} - // Prepend: shift all indices by 1. - allPills = append([]pillEntry{summaryPill}, allPills...) - if selectedIdx >= 0 { - selectedIdx++ - } - sepBoundary++ // separator now one position further right - } - } - // Fit pills within budget, always including the selected pill. // Strategy: include pills left-to-right until budget exhausted. // If selected pill would be excluded, shift the visible window.