Skip to content

Commit 78731e2

Browse files
authored
UX: pill spacing fix + PR review toggle (r key) (#27)
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
1 parent 1f2cb45 commit 78731e2

11 files changed

Lines changed: 120 additions & 43 deletions

File tree

daemon/internal/ctlserver/handler.go

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,8 @@ func (h *Handler) Handle(conn net.Conn) {
7474
h.handleCyclePRAutopilot(conn, req.PRKey)
7575
case "set_merge_method":
7676
h.handleSetMergeMethod(conn, req.PRKey, req.MergeMethod)
77+
case "toggle_review":
78+
h.handleToggleReview(conn, req.PRKey)
7779
}
7880
}
7981
}
@@ -248,6 +250,30 @@ func (h *Handler) handleRemovePR(conn net.Conn, key string) {
248250
writeJSON(conn, ctlResponse{OK: &ok})
249251
}
250252

253+
func (h *Handler) handleToggleReview(conn net.Conn, key string) {
254+
if h.prPoll == nil {
255+
f := false
256+
writeJSON(conn, ctlResponse{OK: &f})
257+
return
258+
}
259+
parts := strings.SplitN(key, "#", 2)
260+
if len(parts) != 2 {
261+
f := false
262+
writeJSON(conn, ctlResponse{OK: &f})
263+
return
264+
}
265+
ownerRepo := strings.SplitN(parts[0], "/", 2)
266+
if len(ownerRepo) != 2 {
267+
f := false
268+
writeJSON(conn, ctlResponse{OK: &f})
269+
return
270+
}
271+
var number int
272+
fmt.Sscanf(parts[1], "%d", &number)
273+
ok := h.prPoll.ToggleReview(ownerRepo[0], ownerRepo[1], number)
274+
writeJSON(conn, ctlResponse{OK: &ok})
275+
}
276+
251277
func writeJSON(conn net.Conn, v any) {
252278
data, _ := json.Marshal(v)
253279
data = append(data, '\n')

daemon/internal/pr/model.go

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -84,7 +84,7 @@ type TrackedPR struct {
8484
MaxHammer int `json:"max_hammer"` // max fix attempts (default 3)
8585
MergeMethod string `json:"merge_method"` // "squash", "merge", "rebase", "aviator", "" = unset
8686
MergeTriggered bool `json:"merge_triggered"` // true once auto-merge has been fired; resets on check regression
87-
RunReview bool `json:"run_review"` // run code-review skill on creation
87+
ReviewEnabled bool `json:"run_review"` // toggle code review on/off (independent of autopilot)
8888

8989
// Agent pipeline state
9090
AgentRunning string `json:"agent_running,omitempty"` // "" | "fix_ci" | "review" | "fix_review"
@@ -204,6 +204,9 @@ func (pr *TrackedPR) IsAgentRunning() bool {
204204

205205
// ShouldReview returns true if the PR should be code-reviewed.
206206
func (pr *TrackedPR) ShouldReview() bool {
207+
if !pr.ReviewEnabled {
208+
return false
209+
}
207210
if pr.AutopilotMode == PROff {
208211
return false
209212
}

daemon/internal/pr/model_test.go

Lines changed: 13 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -425,40 +425,47 @@ func TestIsAgentRunning(t *testing.T) {
425425
// === ShouldReview ===
426426

427427
func TestShouldReview_ChecksPassing(t *testing.T) {
428-
pr := TrackedPR{AutopilotMode: PRAuto, State: StateChecksPassing}
428+
pr := TrackedPR{AutopilotMode: PRAuto, State: StateChecksPassing, ReviewEnabled: true}
429429
if !pr.ShouldReview() {
430-
t.Error("AUTO + checks_passing + no review state should review")
430+
t.Error("AUTO + checks_passing + review enabled should review")
431431
}
432432
}
433433

434434
func TestShouldReview_AutopilotOff(t *testing.T) {
435-
pr := TrackedPR{AutopilotMode: PROff, State: StateChecksPassing}
435+
pr := TrackedPR{AutopilotMode: PROff, State: StateChecksPassing, ReviewEnabled: true}
436436
if pr.ShouldReview() {
437437
t.Error("autopilot off should not review")
438438
}
439439
}
440440

441441
func TestShouldReview_ChecksFailing(t *testing.T) {
442-
pr := TrackedPR{AutopilotMode: PRAuto, State: StateChecksFailing}
442+
pr := TrackedPR{AutopilotMode: PRAuto, State: StateChecksFailing, ReviewEnabled: true}
443443
if pr.ShouldReview() {
444444
t.Error("checks failing should not review")
445445
}
446446
}
447447

448448
func TestShouldReview_AlreadyReviewed(t *testing.T) {
449-
pr := TrackedPR{AutopilotMode: PRAuto, State: StateChecksPassing, ReviewState: "clean"}
449+
pr := TrackedPR{AutopilotMode: PRAuto, State: StateChecksPassing, ReviewState: "clean", ReviewEnabled: true}
450450
if pr.ShouldReview() {
451451
t.Error("already reviewed should not review again")
452452
}
453453
}
454454

455455
func TestShouldReview_Approved(t *testing.T) {
456-
pr := TrackedPR{AutopilotMode: PRAuto, State: StateApproved}
456+
pr := TrackedPR{AutopilotMode: PRAuto, State: StateApproved, ReviewEnabled: true}
457457
if !pr.ShouldReview() {
458458
t.Error("approved state should also trigger review")
459459
}
460460
}
461461

462+
func TestShouldReview_ReviewDisabled(t *testing.T) {
463+
pr := TrackedPR{AutopilotMode: PRAuto, State: StateChecksPassing, ReviewEnabled: false}
464+
if pr.ShouldReview() {
465+
t.Error("review disabled should not review")
466+
}
467+
}
468+
462469
// === ShouldFixReview ===
463470

464471
func TestShouldFixReview_HasIssues(t *testing.T) {

daemon/internal/pr/poller.go

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@ func NewPoller(storePath string, onChange func()) *Poller {
3030
storePath: storePath,
3131
}
3232
p.load()
33+
p.migrateReviewEnabled()
3334
return p
3435
}
3536

@@ -56,6 +57,7 @@ func (p *Poller) Add(owner, repo string, number int) (*TrackedPR, bool) {
5657
AutopilotMode: PRAuto,
5758
Hammer: true,
5859
MaxHammer: 3,
60+
ReviewEnabled: true,
5961
MergeMethod: method,
6062
Timeline: []PREvent{{Time: time.Now(), Icon: "📝", Message: "Added to tracking"}},
6163
}
@@ -167,6 +169,34 @@ func (p *Poller) CycleAutopilot(owner, repo string, number int) string {
167169
return pr.AutopilotMode
168170
}
169171

172+
// ToggleReview flips ReviewEnabled for a PR.
173+
func (p *Poller) ToggleReview(owner, repo string, number int) bool {
174+
key := fmt.Sprintf("%s/%s#%d", owner, repo, number)
175+
p.mu.Lock()
176+
defer p.mu.Unlock()
177+
178+
pr, ok := p.tracked[key]
179+
if !ok {
180+
return false
181+
}
182+
183+
pr.ReviewEnabled = !pr.ReviewEnabled
184+
label := "enabled"
185+
if !pr.ReviewEnabled {
186+
label = "disabled"
187+
}
188+
pr.Timeline = append(pr.Timeline, PREvent{
189+
Time: time.Now(),
190+
Icon: "🔍",
191+
Message: fmt.Sprintf("Code review %s", label),
192+
})
193+
p.save()
194+
if p.onChange != nil {
195+
p.onChange()
196+
}
197+
return true
198+
}
199+
170200
// FailingCount returns how many PRs have failing checks.
171201
func (p *Poller) FailingCount() int {
172202
p.mu.RLock()
@@ -594,6 +624,21 @@ func (p *Poller) load() {
594624
p.tracked = prs
595625
}
596626

627+
// migrateReviewEnabled sets ReviewEnabled=true for non-terminal PRs that
628+
// were persisted before the field existed (zero-value false).
629+
func (p *Poller) migrateReviewEnabled() {
630+
changed := false
631+
for _, pr := range p.tracked {
632+
if !pr.ReviewEnabled && pr.State != StateMerged && pr.State != StateClosed {
633+
pr.ReviewEnabled = true
634+
changed = true
635+
}
636+
}
637+
if changed {
638+
p.save()
639+
}
640+
}
641+
597642
func (p *Poller) save() {
598643
store := pollerStore{
599644
PRs: p.tracked,

daemon/internal/pr/poller_test.go

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,9 @@ func TestAdd_New(t *testing.T) {
5050
if pr.MergeMethod != "" {
5151
t.Errorf("merge_method = %q, want empty (unset for new repo)", pr.MergeMethod)
5252
}
53+
if !pr.ReviewEnabled {
54+
t.Error("ReviewEnabled should default to true")
55+
}
5356
if len(pr.Timeline) != 1 {
5457
t.Errorf("timeline should have 1 event, got %d", len(pr.Timeline))
5558
}

tui/internal/client/client.go

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,7 @@ type TrackedPR struct {
6868
Hammer bool `json:"hammer"`
6969
HammerCount int `json:"hammer_count"`
7070
MergeMethod string `json:"merge_method"`
71+
ReviewEnabled bool `json:"run_review"`
7172
CreatedAt time.Time `json:"created_at"`
7273
Timeline []PREvent `json:"timeline"`
7374

@@ -271,6 +272,12 @@ func (c *Client) CyclePRAutopilot(key string) error {
271272
return err
272273
}
273274

275+
// TogglePRReview toggles code review on/off for a PR.
276+
func (c *Client) TogglePRReview(key string) error {
277+
_, err := c.sendCommand(request{Action: "toggle_review", PRKey: key})
278+
return err
279+
}
280+
274281
// Focus focuses the Ghostty tab for the given session.
275282
func (c *Client) Focus(sessionID string) error {
276283
_, err := c.sendCommand(request{Action: "focus", SessionID: sessionID})

tui/internal/tui/app.go

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -501,6 +501,20 @@ end tell`, tabIdx, tabIdx)
501501
}
502502
}
503503

504+
case "r":
505+
// Toggle code review on/off for selected PR.
506+
if pr := m.selectedPR(); pr != nil {
507+
key := fmt.Sprintf("%s/%s#%d", pr.Owner, pr.Repo, pr.Number)
508+
label := "review enabled"
509+
if pr.ReviewEnabled {
510+
label = "review disabled"
511+
}
512+
return m, func() tea.Msg {
513+
err := m.client.TogglePRReview(key)
514+
return actionResultMsg{action: label, err: err}
515+
}
516+
}
517+
504518
case "m":
505519
// Set merge method for selected PR — daemon handles the actual merge.
506520
if pr := m.selectedPR(); pr != nil {

tui/internal/tui/hints.go

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ func renderHints(queueVisible bool, hasPending bool, isPRSelected bool, width in
2323
// PR-specific hints.
2424
keys = append(keys, hint{"Enter", "open PR"})
2525
keys = append(keys, hint{"a", "autopilot"})
26+
keys = append(keys, hint{"r", "review"})
2627
keys = append(keys, hint{"m", "method"})
2728
keys = append(keys, hint{"+", "add PR"})
2829
keys = append(keys, hint{"-", "remove"})
@@ -107,6 +108,7 @@ func renderHelp(width, height, scrollOffset int) string {
107108
{"", "Pull Requests"},
108109
{"Enter", "Open PR in browser"},
109110
{"a", "Cycle PR autopilot: OFF → AUTO → YOLO"},
111+
{"r", "Toggle code review on/off"},
110112
{"+", "Add PR to tracking (paste URL)"},
111113
{"-", "Remove selected PR"},
112114
{"m", "Set merge method (daemon handles actual merge)"},

tui/internal/tui/pill.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -93,13 +93,13 @@ func isPassiveState(state string) bool {
9393
// pillNameMaxLen returns the max name length based on state and selection.
9494
// Selected pills show full 20-char names.
9595
// Active unselected (running/waiting): 8 chars — visible but compact.
96-
// Passive unselected (idle/dead): 4 chars — minimal footprint.
96+
// Passive unselected (idle/dead): 6 chars — compact but recognizable.
9797
func pillNameMaxLen(state string, selected bool) int {
9898
if selected {
9999
return 20
100100
}
101101
if isPassiveState(state) {
102-
return 4
102+
return 6
103103
}
104104
return 8 // running, waiting, or other active states
105105
}
@@ -140,7 +140,7 @@ func renderPillWithName(s client.Session, displayName string, selected bool, glo
140140
if compact {
141141
// Passive unselected pills: no background, just dim text — lighter visual weight.
142142
style = lipgloss.NewStyle().
143-
Padding(0, 0).
143+
Padding(0, 1).
144144
Foreground(colorDimFg)
145145
} else {
146146
style = lipgloss.NewStyle().

tui/internal/tui/pr_zoom.go

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,9 @@ func renderPRZoom(pr client.TrackedPR, width, height int, scrollOffset int) stri
5050
if pr.Hammer {
5151
line1 += " " + lipgloss.NewStyle().Foreground(colorOrange).Bold(true).Render("🔨")
5252
}
53+
if !pr.ReviewEnabled {
54+
line1 += " " + lipgloss.NewStyle().Foreground(colorDimFg).Render("review off")
55+
}
5356
headerLines = append(headerLines, line1)
5457

5558
// Line 2: branch → base +42 -12 3 commits mergeable

0 commit comments

Comments
 (0)