diff --git a/cmd/roborev/tui/queue_test.go b/cmd/roborev/tui/queue_test.go index 8f736989..4701c1b4 100644 --- a/cmd/roborev/tui/queue_test.go +++ b/cmd/roborev/tui/queue_test.go @@ -628,7 +628,7 @@ func TestTUIJobCellsContent(t *testing.T) { ) cells := m.jobCells(job) - // cells order: ref, branch, repo, agent, queued, elapsed, status, closed + // cells order: ref, branch, repo, agent, queued, elapsed, status, pf, closed if !strings.Contains(cells[0], "abc1234") { t.Errorf("Expected ref to contain abc1234, got %q", cells[0]) } @@ -641,6 +641,9 @@ func TestTUIJobCellsContent(t *testing.T) { if cells[6] != "Done" { t.Errorf("Expected status 'Done', got %q", cells[6]) } + if cells[7] != "-" { + t.Errorf("Expected verdict '-', got %q", cells[7]) + } }) t.Run("claude-code normalizes to claude", func(t *testing.T) { @@ -659,11 +662,14 @@ func TestTUIJobCellsContent(t *testing.T) { job.Closed = &handled cells := m.jobCells(job) - if cells[6] != "Pass" { - t.Errorf("Expected status 'Pass', got %q", cells[6]) + if cells[6] != "Done" { + t.Errorf("Expected status 'Done', got %q", cells[6]) } - if cells[7] != "yes" { - t.Errorf("Expected closed 'yes', got %q", cells[7]) + if cells[7] != "P" { + t.Errorf("Expected verdict 'P', got %q", cells[7]) + } + if cells[8] != "yes" { + t.Errorf("Expected closed 'yes', got %q", cells[8]) } }) } @@ -733,6 +739,71 @@ func TestTUIQueueTableRendersWithinWidth(t *testing.T) { } } +func TestStatusColumnAutoWidth(t *testing.T) { + // The Status column should auto-size to the widest status label + // present in the visible jobs, with a floor of 6 (the "Status" + // header width). This saves horizontal space when no wide status + // labels (e.g. "Canceled") are present. + tests := []struct { + name string + statuses []storage.JobStatus + wantWidth int // expected content width (header included) + }{ + {"done only", []storage.JobStatus{storage.JobStatusDone}, 6}, // "Done"=4, header=6 + {"queued only", []storage.JobStatus{storage.JobStatusQueued}, 6}, // "Queued"=6, header=6 + {"running", []storage.JobStatus{storage.JobStatusRunning}, 7}, // "Running"=7 + {"canceled", []storage.JobStatus{storage.JobStatusCanceled}, 8}, // "Canceled"=8 + {"mixed done and error", []storage.JobStatus{storage.JobStatusDone, storage.JobStatusFailed}, 6}, // max("Done"=4,"Error"=5,header=6)=6 + {"mixed done and canceled", []storage.JobStatus{storage.JobStatusDone, storage.JobStatusCanceled}, 8}, // max("Done"=4,"Canceled"=8)=8 + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + m := newModel("http://localhost", withExternalIODisabled()) + m.width = 200 + m.height = 30 + + jobs := make([]storage.ReviewJob, len(tt.statuses)) + for i, s := range tt.statuses { + jobs[i] = makeJob(int64(i+1), withStatus(s), withRef("abc1234"), withRepoName("repo"), withAgent("test")) + } + m.jobs = jobs + m.selectedIdx = 0 + m.selectedJobID = 1 + + output := m.renderQueueView() + lines := strings.Split(output, "\n") + + // Find the header line (contains "Status" and "P/F") + var headerLine string + for _, line := range lines { + stripped := stripTestANSI(line) + if strings.Contains(stripped, "Status") && strings.Contains(stripped, "P/F") { + headerLine = stripped + break + } + } + if headerLine == "" { + t.Fatal("could not find header line with Status and P/F") + } + + statusIdx := strings.Index(headerLine, "Status") + pfIdx := strings.Index(headerLine, "P/F") + if statusIdx < 0 || pfIdx < 0 || pfIdx <= statusIdx { + t.Fatalf("unexpected header layout: %q", headerLine) + } + + // The gap between "Status" start and "P/F" start is + // the column width + inter-column spacing (1 char padding). + gap := pfIdx - statusIdx + gotWidth := gap - 1 // subtract 1 for inter-column spacing + if gotWidth != tt.wantWidth { + t.Errorf("Status column width = %d, want %d (header: %q)", gotWidth, tt.wantWidth, headerLine) + } + }) + } +} + func TestTUIPaginationAppendMode(t *testing.T) { m := newModel("http://localhost", withExternalIODisabled()) @@ -2198,9 +2269,7 @@ func TestTaskColWidthCacheReuse(t *testing.T) { } } -func TestCombinedStatus(t *testing.T) { - strPtr := func(s string) *string { return &s } - +func TestStatusLabel(t *testing.T) { tests := []struct { name string job storage.ReviewJob @@ -2210,60 +2279,73 @@ func TestCombinedStatus(t *testing.T) { {"running", storage.ReviewJob{Status: storage.JobStatusRunning}, "Running"}, {"failed", storage.ReviewJob{Status: storage.JobStatusFailed}, "Error"}, {"canceled", storage.ReviewJob{Status: storage.JobStatusCanceled}, "Canceled"}, - {"done pass", storage.ReviewJob{Status: storage.JobStatusDone, Verdict: strPtr("P")}, "Pass"}, - {"done fail", storage.ReviewJob{Status: storage.JobStatusDone, Verdict: strPtr("F")}, "Fail"}, - {"done unexpected verdict", storage.ReviewJob{Status: storage.JobStatusDone, Verdict: strPtr("X")}, "Fail"}, - {"done nil verdict", storage.ReviewJob{Status: storage.JobStatusDone}, "Done"}, - {"applied pass", storage.ReviewJob{Status: storage.JobStatusApplied, Verdict: strPtr("P")}, "Pass"}, - {"rebased fail", storage.ReviewJob{Status: storage.JobStatusRebased, Verdict: strPtr("F")}, "Fail"}, + {"done", storage.ReviewJob{Status: storage.JobStatusDone}, "Done"}, + {"applied", storage.ReviewJob{Status: storage.JobStatusApplied}, "Done"}, + {"rebased", storage.ReviewJob{Status: storage.JobStatusRebased}, "Done"}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got := combinedStatus(tt.job) + got := statusLabel(tt.job) if got != tt.want { - t.Errorf("combinedStatus() = %q, want %q", got, tt.want) + t.Errorf("statusLabel() = %q, want %q", got, tt.want) } }) } } -func TestCombinedStatusColor(t *testing.T) { - strPtr := func(s string) *string { return &s } - +func TestStatusColor(t *testing.T) { tests := []struct { - name string - job storage.ReviewJob - wantStyle lipgloss.Style + name string + status storage.JobStatus + want lipgloss.TerminalColor }{ - {"queued", storage.ReviewJob{Status: storage.JobStatusQueued}, queuedStyle}, - {"running", storage.ReviewJob{Status: storage.JobStatusRunning}, runningStyle}, - {"failed", storage.ReviewJob{Status: storage.JobStatusFailed}, failedStyle}, - {"canceled", storage.ReviewJob{Status: storage.JobStatusCanceled}, canceledStyle}, - {"done pass", storage.ReviewJob{Status: storage.JobStatusDone, Verdict: strPtr("P")}, passStyle}, - {"done fail", storage.ReviewJob{Status: storage.JobStatusDone, Verdict: strPtr("F")}, failStyle}, - {"done unexpected verdict", storage.ReviewJob{Status: storage.JobStatusDone, Verdict: strPtr("X")}, failStyle}, - {"done nil verdict", storage.ReviewJob{Status: storage.JobStatusDone}, readyStyle}, - {"applied nil verdict", storage.ReviewJob{Status: storage.JobStatusApplied}, readyStyle}, - {"rebased nil verdict", storage.ReviewJob{Status: storage.JobStatusRebased}, readyStyle}, - {"unknown status", storage.ReviewJob{Status: "unknown"}, queuedStyle}, + {"queued", storage.JobStatusQueued, queuedStyle.GetForeground()}, + {"running", storage.JobStatusRunning, runningStyle.GetForeground()}, + {"done", storage.JobStatusDone, doneStyle.GetForeground()}, + {"applied", storage.JobStatusApplied, doneStyle.GetForeground()}, + {"rebased", storage.JobStatusRebased, doneStyle.GetForeground()}, + {"failed", storage.JobStatusFailed, failedStyle.GetForeground()}, + {"canceled", storage.JobStatusCanceled, canceledStyle.GetForeground()}, + {"unknown", storage.JobStatus("unknown"), nil}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got := combinedStatusColor(tt.job) - want := tt.wantStyle.GetForeground() - if got != want { - t.Errorf("combinedStatusColor() = %v, want %v", got, want) + got := statusColor(tt.status) + if got != tt.want { + t.Errorf("statusColor(%q) = %v, want %v", tt.status, got, tt.want) } }) } - // Verify Error (failedStyle) and Fail (failStyle) use different colors - errorColor := failedStyle.GetForeground() - failColor := failStyle.GetForeground() - if errorColor == failColor { - t.Errorf("Error and Fail should have distinct colors, both are %v", errorColor) + // Error (failedStyle/orange) and Fail (failStyle/red) must be distinct + if failedStyle.GetForeground() == failStyle.GetForeground() { + t.Error("Error and Fail should have distinct colors") + } +} + +func TestVerdictColor(t *testing.T) { + strPtr := func(s string) *string { return &s } + + tests := []struct { + name string + verdict *string + want lipgloss.TerminalColor + }{ + {"pass", strPtr("P"), passStyle.GetForeground()}, + {"fail", strPtr("F"), failStyle.GetForeground()}, + {"unexpected", strPtr("X"), failStyle.GetForeground()}, + {"nil", nil, nil}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := verdictColor(tt.verdict) + if got != tt.want { + t.Errorf("verdictColor() = %v, want %v", got, tt.want) + } + }) } } @@ -2339,17 +2421,23 @@ func TestMigrateColumnConfig(t *testing.T) { wantDirty: true, wantColOrder: nil, }, + { + name: "combined status default order resets", + columnOrder: []string{"ref", "branch", "repo", "agent", "queued", "elapsed", "status", "closed"}, + wantDirty: true, + wantColOrder: nil, + }, { name: "custom order preserved", - columnOrder: []string{"repo", "ref", "agent", "status", "queued", "elapsed", "branch", "closed"}, + columnOrder: []string{"repo", "ref", "agent", "status", "pf", "queued", "elapsed", "branch", "closed"}, wantDirty: false, - wantColOrder: []string{"repo", "ref", "agent", "status", "queued", "elapsed", "branch", "closed"}, + wantColOrder: []string{"repo", "ref", "agent", "status", "pf", "queued", "elapsed", "branch", "closed"}, }, { name: "current default order preserved", - columnOrder: []string{"ref", "branch", "repo", "agent", "queued", "elapsed", "status", "closed"}, + columnOrder: []string{"ref", "branch", "repo", "agent", "queued", "elapsed", "status", "pf", "closed"}, wantDirty: false, - wantColOrder: []string{"ref", "branch", "repo", "agent", "queued", "elapsed", "status", "closed"}, + wantColOrder: []string{"ref", "branch", "repo", "agent", "queued", "elapsed", "status", "pf", "closed"}, }, } @@ -2373,6 +2461,30 @@ func TestMigrateColumnConfig(t *testing.T) { } } +func TestParseColumnOrderAppendsMissing(t *testing.T) { + // A custom order saved before the pf column existed should get + // pf appended automatically by resolveColumnOrder. + oldCustom := []string{"repo", "ref", "agent", "status", "queued", "elapsed", "branch", "closed"} + got := parseColumnOrder(oldCustom) + + // Verify existing columns are in the user's order + wantPrefix := []int{colRepo, colRef, colAgent, colStatus, colQueued, colElapsed, colBranch, colHandled} + if !slices.Equal(got[:len(wantPrefix)], wantPrefix) { + t.Errorf("prefix = %v, want %v", got[:len(wantPrefix)], wantPrefix) + } + + // pf must be appended exactly once + pfCount := 0 + for _, c := range got { + if c == colPF { + pfCount++ + } + } + if pfCount != 1 { + t.Errorf("expected pf to appear once, got %d in %v", pfCount, got) + } +} + func TestDefaultColumnOrderDetection(t *testing.T) { // Verify the slices.Equal check that saveColumnOptions uses // to decide whether to persist column order: default order diff --git a/cmd/roborev/tui/render_queue.go b/cmd/roborev/tui/render_queue.go index 83a61249..b9f171c0 100644 --- a/cmd/roborev/tui/render_queue.go +++ b/cmd/roborev/tui/render_queue.go @@ -97,7 +97,8 @@ const ( colAgent // Agent name colQueued // Enqueue timestamp colElapsed // Elapsed time - colStatus // Job status (combined with verdict) + colStatus // Job status + colPF // Pass/Fail verdict colHandled // Done status colCount // total number of columns ) @@ -248,7 +249,7 @@ func (m model) renderQueueView() string { visCols := m.visibleColumns() // Compute per-column max content widths, using cache when data hasn't changed. - allHeaders := [colCount]string{"", "JobID", "Ref", "Branch", "Repo", "Agent", "Queued", "Elapsed", "Status", "Closed"} + allHeaders := [colCount]string{"", "JobID", "Ref", "Branch", "Repo", "Agent", "Queued", "Elapsed", "Status", "P/F", "Closed"} allFullRows := make([][]string, len(visibleJobList)) for i, job := range visibleJobList { cells := m.jobCells(job) @@ -298,9 +299,10 @@ func (m model) renderQueueView() string { fixedWidth := map[int]int{ colSel: 2, colJobID: idWidth, - colStatus: 8, // fits "Canceled" (8), "Running" (7), etc. + colStatus: max(contentWidth[colStatus], 6), // "Status" header = 6, auto-sizes to content colQueued: 12, colElapsed: 8, + colPF: 3, // "P/F" header = 3 colHandled: max(contentWidth[colHandled], 6), // "Closed" header = 6 colAgent: min(max(contentWidth[colAgent], 5), 12), // "Agent" header = 5, cap at 12 } @@ -491,7 +493,13 @@ func (m model) renderQueueView() string { job := windowJobs[row] switch logicalCol { case colStatus: - s = s.Foreground(combinedStatusColor(job)) + if c := statusColor(job.Status); c != nil { + s = s.Foreground(c) + } + case colPF: + if c := verdictColor(job.Verdict); c != nil { + s = s.Foreground(c) + } case colHandled: if job.Closed != nil { if *job.Closed { @@ -575,8 +583,8 @@ func (m model) renderQueueView() string { } // jobCells returns plain text cell values for a job row. -// Order: ref, branch, repo, agent, status, queued, elapsed, handled -// (colRef through colHandled, 8 values). +// Order: ref, branch, repo, agent, queued, elapsed, status, pf, handled +// (colRef through colHandled, 9 values). func (m model) jobCells(job storage.ReviewJob) []string { ref := shortJobRef(job) if !config.IsDefaultReviewType(job.ReviewType) { @@ -606,7 +614,12 @@ func (m model) jobCells(job storage.ReviewJob) []string { } } - status := combinedStatus(job) + status := statusLabel(job) + + verdict := "-" + if job.Verdict != nil { + verdict = *job.Verdict + } handled := "" if job.Closed != nil { @@ -617,13 +630,11 @@ func (m model) jobCells(job storage.ReviewJob) []string { } } - return []string{ref, branch, repo, agentName, enqueued, elapsed, status, handled} + return []string{ref, branch, repo, agentName, enqueued, elapsed, status, verdict, handled} } -// combinedStatus returns a display string that merges job status -// with verdict: Queued, Running, Error, Canceled, Pass, Fail, or -// Done (for task/fix jobs that have no verdict). -func combinedStatus(job storage.ReviewJob) string { +// statusLabel returns a capitalized display label for the job status. +func statusLabel(job storage.ReviewJob) string { switch job.Status { case storage.JobStatusQueued: return "Queued" @@ -635,44 +646,45 @@ func combinedStatus(job storage.ReviewJob) string { return "Canceled" case storage.JobStatusDone, storage.JobStatusApplied, storage.JobStatusRebased: - if job.Verdict != nil { - if *job.Verdict == "P" { - return "Pass" - } - return "Fail" - } return "Done" default: return string(job.Status) } } -// combinedStatusColor returns the foreground color for the -// combined status column based on job state and verdict. -func combinedStatusColor( - job storage.ReviewJob, +// statusColor returns the foreground color for the Status column. +func statusColor( + status storage.JobStatus, ) lipgloss.TerminalColor { - switch job.Status { + switch status { case storage.JobStatusQueued: return queuedStyle.GetForeground() case storage.JobStatusRunning: return runningStyle.GetForeground() + case storage.JobStatusDone, storage.JobStatusApplied, + storage.JobStatusRebased: + return doneStyle.GetForeground() case storage.JobStatusFailed: return failedStyle.GetForeground() case storage.JobStatusCanceled: return canceledStyle.GetForeground() - case storage.JobStatusDone, storage.JobStatusApplied, - storage.JobStatusRebased: - if job.Verdict == nil { - return readyStyle.GetForeground() - } - if *job.Verdict != "P" { - return failStyle.GetForeground() - } - return passStyle.GetForeground() default: - return queuedStyle.GetForeground() + return nil + } +} + +// verdictColor returns the foreground color for the P/F column. +// Returns nil when no color should be applied (nil verdict). +func verdictColor( + verdict *string, +) lipgloss.TerminalColor { + if verdict == nil { + return nil } + if *verdict == "P" { + return passStyle.GetForeground() + } + return failStyle.GetForeground() } // commandLineForJob computes the representative agent command line from job parameters. @@ -722,21 +734,28 @@ func migrateColumnConfig(cfg *config.Config) bool { cfg.HiddenColumns = nil dirty = true } - // Old default order (status before queued) → reset - oldDefault := []string{ - "ref", "branch", "repo", "agent", - "status", "queued", "elapsed", "closed", - } - if slices.Equal(cfg.ColumnOrder, oldDefault) { - cfg.ColumnOrder = nil - dirty = true + // Old default orders → reset + oldDefaults := [][]string{ + // status before queued (pre-rename) + {"ref", "branch", "repo", "agent", + "status", "queued", "elapsed", "closed"}, + // combined status without pf column + {"ref", "branch", "repo", "agent", + "queued", "elapsed", "status", "closed"}, + } + for _, old := range oldDefaults { + if slices.Equal(cfg.ColumnOrder, old) { + cfg.ColumnOrder = nil + dirty = true + break + } } return dirty } // toggleableColumns is the ordered list of columns the user can show/hide. // colSel and colJobID are always visible and not included here. -var toggleableColumns = []int{colRef, colBranch, colRepo, colAgent, colQueued, colElapsed, colStatus, colHandled} +var toggleableColumns = []int{colRef, colBranch, colRepo, colAgent, colQueued, colElapsed, colStatus, colPF, colHandled} // columnNames maps column constants to display names. var columnNames = map[int]string{ @@ -747,6 +766,7 @@ var columnNames = map[int]string{ colStatus: "Status", colQueued: "Queued", colElapsed: "Elapsed", + colPF: "P/F", colHandled: "Closed", } @@ -759,6 +779,7 @@ var columnConfigNames = map[int]string{ colStatus: "status", colQueued: "queued", colElapsed: "elapsed", + colPF: "pf", colHandled: "closed", } diff --git a/cmd/roborev/tui/tui.go b/cmd/roborev/tui/tui.go index 73f96c66..6bdb83fd 100644 --- a/cmd/roborev/tui/tui.go +++ b/cmd/roborev/tui/tui.go @@ -45,13 +45,12 @@ var ( queuedStyle = lipgloss.NewStyle().Foreground(lipgloss.AdaptiveColor{Light: "136", Dark: "226"}) // Yellow/Gold runningStyle = lipgloss.NewStyle().Foreground(lipgloss.AdaptiveColor{Light: "25", Dark: "33"}) // Blue - doneStyle = lipgloss.NewStyle().Foreground(lipgloss.AdaptiveColor{Light: "28", Dark: "46"}) // Green + doneStyle = lipgloss.NewStyle().Foreground(lipgloss.AdaptiveColor{Light: "27", Dark: "39"}) // Blue (completed) failedStyle = lipgloss.NewStyle().Foreground(lipgloss.AdaptiveColor{Light: "166", Dark: "208"}) // Orange (job error) canceledStyle = lipgloss.NewStyle().Foreground(lipgloss.AdaptiveColor{Light: "243", Dark: "245"}) // Gray passStyle = lipgloss.NewStyle().Foreground(lipgloss.AdaptiveColor{Light: "28", Dark: "46"}) // Green failStyle = lipgloss.NewStyle().Foreground(lipgloss.AdaptiveColor{Light: "124", Dark: "196"}) // Red (review found issues) - readyStyle = lipgloss.NewStyle().Foreground(lipgloss.AdaptiveColor{Light: "27", Dark: "39"}) // Blue (done, no verdict) closedStyle = lipgloss.NewStyle().Foreground(lipgloss.AdaptiveColor{Light: "30", Dark: "51"}) // Cyan helpStyle = lipgloss.NewStyle().