Skip to content

Commit 52ba097

Browse files
wesmclaude
andauthored
Restore separate P/F verdict column in TUI queue (#427)
## Summary - Restore the P/F verdict column between Status and Closed in the TUI queue table, reverting the combined status/verdict approach from #412 which was confusing - Status column now shows job state only (Queued/Running/Done/Error/Canceled) - P/F column shows verdict (P/F/-) with green/red coloring - Change Done status color from green to blue so it's visually distinct from Pass - Auto-size Status column width based on visible content (saves 2 chars when no Canceled jobs present) - Extract `statusColor`/`verdictColor` into testable helpers; return nil for unknown statuses instead of defaulting to queued color - Add `migrateColumnConfig` entries to reset old default orders (both pre-rename and combined-status layouts) - Add `TestStatusColor`, `TestVerdictColor`, `TestParseColumnOrderAppendsMissing`, `TestStatusColumnAutoWidth` tests ## Test plan - [x] All existing TUI tests pass - [x] Full test suite passes (`go test ./...`) - [x] `go vet` and `go fmt` clean - [x] New tests cover status/verdict color mapping, legacy column order migration, and auto-width sizing 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
1 parent e9909f5 commit 52ba097

File tree

3 files changed

+222
-90
lines changed

3 files changed

+222
-90
lines changed

cmd/roborev/tui/queue_test.go

Lines changed: 158 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -628,7 +628,7 @@ func TestTUIJobCellsContent(t *testing.T) {
628628
)
629629
cells := m.jobCells(job)
630630

631-
// cells order: ref, branch, repo, agent, queued, elapsed, status, closed
631+
// cells order: ref, branch, repo, agent, queued, elapsed, status, pf, closed
632632
if !strings.Contains(cells[0], "abc1234") {
633633
t.Errorf("Expected ref to contain abc1234, got %q", cells[0])
634634
}
@@ -641,6 +641,9 @@ func TestTUIJobCellsContent(t *testing.T) {
641641
if cells[6] != "Done" {
642642
t.Errorf("Expected status 'Done', got %q", cells[6])
643643
}
644+
if cells[7] != "-" {
645+
t.Errorf("Expected verdict '-', got %q", cells[7])
646+
}
644647
})
645648

646649
t.Run("claude-code normalizes to claude", func(t *testing.T) {
@@ -659,11 +662,14 @@ func TestTUIJobCellsContent(t *testing.T) {
659662
job.Closed = &handled
660663

661664
cells := m.jobCells(job)
662-
if cells[6] != "Pass" {
663-
t.Errorf("Expected status 'Pass', got %q", cells[6])
665+
if cells[6] != "Done" {
666+
t.Errorf("Expected status 'Done', got %q", cells[6])
664667
}
665-
if cells[7] != "yes" {
666-
t.Errorf("Expected closed 'yes', got %q", cells[7])
668+
if cells[7] != "P" {
669+
t.Errorf("Expected verdict 'P', got %q", cells[7])
670+
}
671+
if cells[8] != "yes" {
672+
t.Errorf("Expected closed 'yes', got %q", cells[8])
667673
}
668674
})
669675
}
@@ -733,6 +739,71 @@ func TestTUIQueueTableRendersWithinWidth(t *testing.T) {
733739
}
734740
}
735741

742+
func TestStatusColumnAutoWidth(t *testing.T) {
743+
// The Status column should auto-size to the widest status label
744+
// present in the visible jobs, with a floor of 6 (the "Status"
745+
// header width). This saves horizontal space when no wide status
746+
// labels (e.g. "Canceled") are present.
747+
tests := []struct {
748+
name string
749+
statuses []storage.JobStatus
750+
wantWidth int // expected content width (header included)
751+
}{
752+
{"done only", []storage.JobStatus{storage.JobStatusDone}, 6}, // "Done"=4, header=6
753+
{"queued only", []storage.JobStatus{storage.JobStatusQueued}, 6}, // "Queued"=6, header=6
754+
{"running", []storage.JobStatus{storage.JobStatusRunning}, 7}, // "Running"=7
755+
{"canceled", []storage.JobStatus{storage.JobStatusCanceled}, 8}, // "Canceled"=8
756+
{"mixed done and error", []storage.JobStatus{storage.JobStatusDone, storage.JobStatusFailed}, 6}, // max("Done"=4,"Error"=5,header=6)=6
757+
{"mixed done and canceled", []storage.JobStatus{storage.JobStatusDone, storage.JobStatusCanceled}, 8}, // max("Done"=4,"Canceled"=8)=8
758+
}
759+
760+
for _, tt := range tests {
761+
t.Run(tt.name, func(t *testing.T) {
762+
m := newModel("http://localhost", withExternalIODisabled())
763+
m.width = 200
764+
m.height = 30
765+
766+
jobs := make([]storage.ReviewJob, len(tt.statuses))
767+
for i, s := range tt.statuses {
768+
jobs[i] = makeJob(int64(i+1), withStatus(s), withRef("abc1234"), withRepoName("repo"), withAgent("test"))
769+
}
770+
m.jobs = jobs
771+
m.selectedIdx = 0
772+
m.selectedJobID = 1
773+
774+
output := m.renderQueueView()
775+
lines := strings.Split(output, "\n")
776+
777+
// Find the header line (contains "Status" and "P/F")
778+
var headerLine string
779+
for _, line := range lines {
780+
stripped := stripTestANSI(line)
781+
if strings.Contains(stripped, "Status") && strings.Contains(stripped, "P/F") {
782+
headerLine = stripped
783+
break
784+
}
785+
}
786+
if headerLine == "" {
787+
t.Fatal("could not find header line with Status and P/F")
788+
}
789+
790+
statusIdx := strings.Index(headerLine, "Status")
791+
pfIdx := strings.Index(headerLine, "P/F")
792+
if statusIdx < 0 || pfIdx < 0 || pfIdx <= statusIdx {
793+
t.Fatalf("unexpected header layout: %q", headerLine)
794+
}
795+
796+
// The gap between "Status" start and "P/F" start is
797+
// the column width + inter-column spacing (1 char padding).
798+
gap := pfIdx - statusIdx
799+
gotWidth := gap - 1 // subtract 1 for inter-column spacing
800+
if gotWidth != tt.wantWidth {
801+
t.Errorf("Status column width = %d, want %d (header: %q)", gotWidth, tt.wantWidth, headerLine)
802+
}
803+
})
804+
}
805+
}
806+
736807
func TestTUIPaginationAppendMode(t *testing.T) {
737808
m := newModel("http://localhost", withExternalIODisabled())
738809

@@ -2198,9 +2269,7 @@ func TestTaskColWidthCacheReuse(t *testing.T) {
21982269
}
21992270
}
22002271

2201-
func TestCombinedStatus(t *testing.T) {
2202-
strPtr := func(s string) *string { return &s }
2203-
2272+
func TestStatusLabel(t *testing.T) {
22042273
tests := []struct {
22052274
name string
22062275
job storage.ReviewJob
@@ -2210,60 +2279,73 @@ func TestCombinedStatus(t *testing.T) {
22102279
{"running", storage.ReviewJob{Status: storage.JobStatusRunning}, "Running"},
22112280
{"failed", storage.ReviewJob{Status: storage.JobStatusFailed}, "Error"},
22122281
{"canceled", storage.ReviewJob{Status: storage.JobStatusCanceled}, "Canceled"},
2213-
{"done pass", storage.ReviewJob{Status: storage.JobStatusDone, Verdict: strPtr("P")}, "Pass"},
2214-
{"done fail", storage.ReviewJob{Status: storage.JobStatusDone, Verdict: strPtr("F")}, "Fail"},
2215-
{"done unexpected verdict", storage.ReviewJob{Status: storage.JobStatusDone, Verdict: strPtr("X")}, "Fail"},
2216-
{"done nil verdict", storage.ReviewJob{Status: storage.JobStatusDone}, "Done"},
2217-
{"applied pass", storage.ReviewJob{Status: storage.JobStatusApplied, Verdict: strPtr("P")}, "Pass"},
2218-
{"rebased fail", storage.ReviewJob{Status: storage.JobStatusRebased, Verdict: strPtr("F")}, "Fail"},
2282+
{"done", storage.ReviewJob{Status: storage.JobStatusDone}, "Done"},
2283+
{"applied", storage.ReviewJob{Status: storage.JobStatusApplied}, "Done"},
2284+
{"rebased", storage.ReviewJob{Status: storage.JobStatusRebased}, "Done"},
22192285
}
22202286

22212287
for _, tt := range tests {
22222288
t.Run(tt.name, func(t *testing.T) {
2223-
got := combinedStatus(tt.job)
2289+
got := statusLabel(tt.job)
22242290
if got != tt.want {
2225-
t.Errorf("combinedStatus() = %q, want %q", got, tt.want)
2291+
t.Errorf("statusLabel() = %q, want %q", got, tt.want)
22262292
}
22272293
})
22282294
}
22292295
}
22302296

2231-
func TestCombinedStatusColor(t *testing.T) {
2232-
strPtr := func(s string) *string { return &s }
2233-
2297+
func TestStatusColor(t *testing.T) {
22342298
tests := []struct {
2235-
name string
2236-
job storage.ReviewJob
2237-
wantStyle lipgloss.Style
2299+
name string
2300+
status storage.JobStatus
2301+
want lipgloss.TerminalColor
22382302
}{
2239-
{"queued", storage.ReviewJob{Status: storage.JobStatusQueued}, queuedStyle},
2240-
{"running", storage.ReviewJob{Status: storage.JobStatusRunning}, runningStyle},
2241-
{"failed", storage.ReviewJob{Status: storage.JobStatusFailed}, failedStyle},
2242-
{"canceled", storage.ReviewJob{Status: storage.JobStatusCanceled}, canceledStyle},
2243-
{"done pass", storage.ReviewJob{Status: storage.JobStatusDone, Verdict: strPtr("P")}, passStyle},
2244-
{"done fail", storage.ReviewJob{Status: storage.JobStatusDone, Verdict: strPtr("F")}, failStyle},
2245-
{"done unexpected verdict", storage.ReviewJob{Status: storage.JobStatusDone, Verdict: strPtr("X")}, failStyle},
2246-
{"done nil verdict", storage.ReviewJob{Status: storage.JobStatusDone}, readyStyle},
2247-
{"applied nil verdict", storage.ReviewJob{Status: storage.JobStatusApplied}, readyStyle},
2248-
{"rebased nil verdict", storage.ReviewJob{Status: storage.JobStatusRebased}, readyStyle},
2249-
{"unknown status", storage.ReviewJob{Status: "unknown"}, queuedStyle},
2303+
{"queued", storage.JobStatusQueued, queuedStyle.GetForeground()},
2304+
{"running", storage.JobStatusRunning, runningStyle.GetForeground()},
2305+
{"done", storage.JobStatusDone, doneStyle.GetForeground()},
2306+
{"applied", storage.JobStatusApplied, doneStyle.GetForeground()},
2307+
{"rebased", storage.JobStatusRebased, doneStyle.GetForeground()},
2308+
{"failed", storage.JobStatusFailed, failedStyle.GetForeground()},
2309+
{"canceled", storage.JobStatusCanceled, canceledStyle.GetForeground()},
2310+
{"unknown", storage.JobStatus("unknown"), nil},
22502311
}
22512312

22522313
for _, tt := range tests {
22532314
t.Run(tt.name, func(t *testing.T) {
2254-
got := combinedStatusColor(tt.job)
2255-
want := tt.wantStyle.GetForeground()
2256-
if got != want {
2257-
t.Errorf("combinedStatusColor() = %v, want %v", got, want)
2315+
got := statusColor(tt.status)
2316+
if got != tt.want {
2317+
t.Errorf("statusColor(%q) = %v, want %v", tt.status, got, tt.want)
22582318
}
22592319
})
22602320
}
22612321

2262-
// Verify Error (failedStyle) and Fail (failStyle) use different colors
2263-
errorColor := failedStyle.GetForeground()
2264-
failColor := failStyle.GetForeground()
2265-
if errorColor == failColor {
2266-
t.Errorf("Error and Fail should have distinct colors, both are %v", errorColor)
2322+
// Error (failedStyle/orange) and Fail (failStyle/red) must be distinct
2323+
if failedStyle.GetForeground() == failStyle.GetForeground() {
2324+
t.Error("Error and Fail should have distinct colors")
2325+
}
2326+
}
2327+
2328+
func TestVerdictColor(t *testing.T) {
2329+
strPtr := func(s string) *string { return &s }
2330+
2331+
tests := []struct {
2332+
name string
2333+
verdict *string
2334+
want lipgloss.TerminalColor
2335+
}{
2336+
{"pass", strPtr("P"), passStyle.GetForeground()},
2337+
{"fail", strPtr("F"), failStyle.GetForeground()},
2338+
{"unexpected", strPtr("X"), failStyle.GetForeground()},
2339+
{"nil", nil, nil},
2340+
}
2341+
2342+
for _, tt := range tests {
2343+
t.Run(tt.name, func(t *testing.T) {
2344+
got := verdictColor(tt.verdict)
2345+
if got != tt.want {
2346+
t.Errorf("verdictColor() = %v, want %v", got, tt.want)
2347+
}
2348+
})
22672349
}
22682350
}
22692351

@@ -2339,17 +2421,23 @@ func TestMigrateColumnConfig(t *testing.T) {
23392421
wantDirty: true,
23402422
wantColOrder: nil,
23412423
},
2424+
{
2425+
name: "combined status default order resets",
2426+
columnOrder: []string{"ref", "branch", "repo", "agent", "queued", "elapsed", "status", "closed"},
2427+
wantDirty: true,
2428+
wantColOrder: nil,
2429+
},
23422430
{
23432431
name: "custom order preserved",
2344-
columnOrder: []string{"repo", "ref", "agent", "status", "queued", "elapsed", "branch", "closed"},
2432+
columnOrder: []string{"repo", "ref", "agent", "status", "pf", "queued", "elapsed", "branch", "closed"},
23452433
wantDirty: false,
2346-
wantColOrder: []string{"repo", "ref", "agent", "status", "queued", "elapsed", "branch", "closed"},
2434+
wantColOrder: []string{"repo", "ref", "agent", "status", "pf", "queued", "elapsed", "branch", "closed"},
23472435
},
23482436
{
23492437
name: "current default order preserved",
2350-
columnOrder: []string{"ref", "branch", "repo", "agent", "queued", "elapsed", "status", "closed"},
2438+
columnOrder: []string{"ref", "branch", "repo", "agent", "queued", "elapsed", "status", "pf", "closed"},
23512439
wantDirty: false,
2352-
wantColOrder: []string{"ref", "branch", "repo", "agent", "queued", "elapsed", "status", "closed"},
2440+
wantColOrder: []string{"ref", "branch", "repo", "agent", "queued", "elapsed", "status", "pf", "closed"},
23532441
},
23542442
}
23552443

@@ -2373,6 +2461,30 @@ func TestMigrateColumnConfig(t *testing.T) {
23732461
}
23742462
}
23752463

2464+
func TestParseColumnOrderAppendsMissing(t *testing.T) {
2465+
// A custom order saved before the pf column existed should get
2466+
// pf appended automatically by resolveColumnOrder.
2467+
oldCustom := []string{"repo", "ref", "agent", "status", "queued", "elapsed", "branch", "closed"}
2468+
got := parseColumnOrder(oldCustom)
2469+
2470+
// Verify existing columns are in the user's order
2471+
wantPrefix := []int{colRepo, colRef, colAgent, colStatus, colQueued, colElapsed, colBranch, colHandled}
2472+
if !slices.Equal(got[:len(wantPrefix)], wantPrefix) {
2473+
t.Errorf("prefix = %v, want %v", got[:len(wantPrefix)], wantPrefix)
2474+
}
2475+
2476+
// pf must be appended exactly once
2477+
pfCount := 0
2478+
for _, c := range got {
2479+
if c == colPF {
2480+
pfCount++
2481+
}
2482+
}
2483+
if pfCount != 1 {
2484+
t.Errorf("expected pf to appear once, got %d in %v", pfCount, got)
2485+
}
2486+
}
2487+
23762488
func TestDefaultColumnOrderDetection(t *testing.T) {
23772489
// Verify the slices.Equal check that saveColumnOptions uses
23782490
// to decide whether to persist column order: default order

0 commit comments

Comments
 (0)