Skip to content

Commit 06ea80a

Browse files
wesmclaude
andauthored
Fix review screen showing worktree branch instead of stored branch (#492)
## Summary - The TUI review screen resolved the branch name dynamically via `git name-rev`, which can return a worktree branch when the same SHA is reachable from multiple refs. The queue view already handled this correctly by preferring the stored `job.Branch`. - `fetchReview()` now checks `review.Job.Branch` first and only falls back to `git.GetBranchName()` when the stored value is empty (for older jobs that predate the Branch column). - Added a regression test for the stored-branch priority. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
1 parent 5d507e3 commit 06ea80a

File tree

4 files changed

+133
-9
lines changed

4 files changed

+133
-9
lines changed

cmd/roborev/tui/fetch.go

Lines changed: 22 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -501,16 +501,33 @@ func (m model) fetchReview(jobID int64) tea.Cmd {
501501

502502
responses := m.loadResponses(jobID, review)
503503

504-
// Compute branch name for single commits (not ranges)
505-
var branchName string
506-
if review.Job != nil && review.Job.RepoPath != "" && !strings.Contains(review.Job.GitRef, "..") {
507-
branchName = git.GetBranchName(review.Job.RepoPath, review.Job.GitRef)
508-
}
504+
branchName := reviewBranchName(review.Job)
509505

510506
return reviewMsg{review: review, responses: responses, jobID: jobID, branchName: branchName}
511507
}
512508
}
513509

510+
// reviewBranchName returns the branch to display on the review screen.
511+
// It prefers the stored job.Branch (set at enqueue time) over a dynamic
512+
// git name-rev lookup, which can be misled by worktree branches
513+
// reachable from the same SHA. Falls back to git lookup only for
514+
// single-commit reviews when the stored branch is empty.
515+
func reviewBranchName(job *storage.ReviewJob) string {
516+
if job == nil {
517+
return ""
518+
}
519+
if job.Branch == branchNone {
520+
return ""
521+
}
522+
if job.Branch != "" {
523+
return job.Branch
524+
}
525+
if job.RepoPath != "" && !strings.Contains(job.GitRef, "..") {
526+
return git.GetBranchName(job.RepoPath, job.GitRef)
527+
}
528+
return ""
529+
}
530+
514531
func (m model) fetchReviewForPrompt(jobID int64) tea.Cmd {
515532
return func() tea.Msg {
516533
review, err := m.loadReview(jobID)

cmd/roborev/tui/review_branch_test.go

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,56 @@ func TestTUIReviewMsgSetsBranchName(t *testing.T) {
3030
assert.Equal(t, "main", m2.currentBranch)
3131
}
3232

33+
func TestReviewBranchName(t *testing.T) {
34+
tests := []struct {
35+
name string
36+
job *storage.ReviewJob
37+
want string
38+
}{
39+
{
40+
name: "nil job",
41+
job: nil,
42+
want: "",
43+
},
44+
{
45+
name: "stored branch preferred over git lookup",
46+
job: &storage.ReviewJob{Branch: "main", GitRef: "abc123", RepoPath: "/tmp/repo"},
47+
want: "main",
48+
},
49+
{
50+
name: "stored branch for range review",
51+
job: &storage.ReviewJob{Branch: "main", GitRef: "abc123..def456"},
52+
want: "main",
53+
},
54+
{
55+
name: "branchNone sentinel treated as empty",
56+
job: &storage.ReviewJob{Branch: "(none)", GitRef: "abc123"},
57+
want: "",
58+
},
59+
{
60+
name: "branchNone with repo path skips git lookup",
61+
job: &storage.ReviewJob{Branch: "(none)", GitRef: "abc123", RepoPath: "/tmp/repo"},
62+
want: "",
63+
},
64+
{
65+
name: "no stored branch and range skips git lookup",
66+
job: &storage.ReviewJob{GitRef: "abc123..def456", RepoPath: "/tmp/repo"},
67+
want: "",
68+
},
69+
{
70+
name: "no stored branch and no repo path",
71+
job: &storage.ReviewJob{GitRef: "abc123"},
72+
want: "",
73+
},
74+
}
75+
for _, tt := range tests {
76+
t.Run(tt.name, func(t *testing.T) {
77+
got := reviewBranchName(tt.job)
78+
assert.Equal(t, tt.want, got)
79+
})
80+
}
81+
}
82+
3383
func TestTUIReviewMsgEmptyBranchForRange(t *testing.T) {
3484
m := newModel("http://localhost", withExternalIODisabled())
3585
m.jobs = []storage.ReviewJob{

internal/storage/reviews.go

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -21,9 +21,10 @@ func (db *DB) GetReviewByJobID(jobID int64) (*Review, error) {
2121
var commitSubject sql.NullString
2222

2323
var verdictBool sql.NullInt64
24+
var branch sql.NullString
2425
err := db.QueryRow(`
2526
SELECT rv.id, rv.job_id, rv.agent, rv.prompt, rv.output, rv.created_at, rv.closed, rv.uuid, rv.verdict_bool,
26-
j.id, j.repo_id, j.commit_id, j.git_ref, j.session_id, j.agent, j.reasoning, j.status, j.enqueued_at,
27+
j.id, j.repo_id, j.commit_id, j.git_ref, j.branch, j.session_id, j.agent, j.reasoning, j.status, j.enqueued_at,
2728
j.started_at, j.finished_at, j.worker_id, j.error, j.model, j.job_type, j.review_type, j.patch_id,
2829
rp.root_path, rp.name, c.subject
2930
FROM reviews rv
@@ -32,7 +33,7 @@ func (db *DB) GetReviewByJobID(jobID int64) (*Review, error) {
3233
LEFT JOIN commits c ON c.id = j.commit_id
3334
WHERE rv.job_id = ?
3435
`, jobID).Scan(&r.ID, &r.JobID, &r.Agent, &r.Prompt, &r.Output, &createdAt, &closed, &reviewUUID, &verdictBool,
35-
&job.ID, &job.RepoID, &commitID, &job.GitRef, &sessionID, &job.Agent, &job.Reasoning, &job.Status, &enqueuedAt,
36+
&job.ID, &job.RepoID, &commitID, &job.GitRef, &branch, &sessionID, &job.Agent, &job.Reasoning, &job.Status, &enqueuedAt,
3637
&startedAt, &finishedAt, &workerID, &errMsg, &model, &jobTypeStr, &reviewTypeStr, &patchIDStr,
3738
&job.RepoPath, &job.RepoName, &commitSubject)
3839
if err != nil {
@@ -50,6 +51,9 @@ func (db *DB) GetReviewByJobID(jobID int64) (*Review, error) {
5051
if commitSubject.Valid {
5152
job.CommitSubject = commitSubject.String
5253
}
54+
if branch.Valid {
55+
job.Branch = branch.String
56+
}
5357
if model.Valid {
5458
job.Model = model.String
5559
}
@@ -109,9 +113,10 @@ func (db *DB) GetReviewByCommitSHA(sha string) (*Review, error) {
109113

110114
// Search by git_ref which contains the SHA for single commits
111115
var verdictBool sql.NullInt64
116+
var branch sql.NullString
112117
err := db.QueryRow(`
113118
SELECT rv.id, rv.job_id, rv.agent, rv.prompt, rv.output, rv.created_at, rv.closed, rv.uuid, rv.verdict_bool,
114-
j.id, j.repo_id, j.commit_id, j.git_ref, j.session_id, j.agent, j.reasoning, j.status, j.enqueued_at,
119+
j.id, j.repo_id, j.commit_id, j.git_ref, j.branch, j.session_id, j.agent, j.reasoning, j.status, j.enqueued_at,
115120
j.started_at, j.finished_at, j.worker_id, j.error, j.model, j.job_type, j.review_type, j.patch_id,
116121
rp.root_path, rp.name, c.subject
117122
FROM reviews rv
@@ -122,7 +127,7 @@ func (db *DB) GetReviewByCommitSHA(sha string) (*Review, error) {
122127
ORDER BY rv.created_at DESC
123128
LIMIT 1
124129
`, sha).Scan(&r.ID, &r.JobID, &r.Agent, &r.Prompt, &r.Output, &createdAt, &closed, &reviewUUID, &verdictBool,
125-
&job.ID, &job.RepoID, &commitID, &job.GitRef, &sessionID, &job.Agent, &job.Reasoning, &job.Status, &enqueuedAt,
130+
&job.ID, &job.RepoID, &commitID, &job.GitRef, &branch, &sessionID, &job.Agent, &job.Reasoning, &job.Status, &enqueuedAt,
126131
&startedAt, &finishedAt, &workerID, &errMsg, &model, &jobTypeStr, &reviewTypeStr, &patchIDStr,
127132
&job.RepoPath, &job.RepoName, &commitSubject)
128133
if err != nil {
@@ -139,6 +144,9 @@ func (db *DB) GetReviewByCommitSHA(sha string) (*Review, error) {
139144
if commitSubject.Valid {
140145
job.CommitSubject = commitSubject.String
141146
}
147+
if branch.Valid {
148+
job.Branch = branch.String
149+
}
142150
if model.Valid {
143151
job.Model = model.String
144152
}

internal/storage/reviews_test.go

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -370,6 +370,55 @@ func createCompletedJobWithOptions(t *testing.T, db *DB, opts EnqueueOpts, outpu
370370
return updatedJob
371371
}
372372

373+
func TestGetReviewByJobIDIncludesBranch(t *testing.T) {
374+
db := openTestDB(t)
375+
defer db.Close()
376+
377+
repo := createRepo(t, db, "/tmp/test-repo")
378+
379+
tests := []struct {
380+
name string
381+
branch string
382+
want string
383+
}{
384+
{"branch populated when set", "main", "main"},
385+
{"branch empty when not set", "", ""},
386+
}
387+
388+
for _, tt := range tests {
389+
t.Run(tt.name, func(t *testing.T) {
390+
job := createCompletedJobWithOptions(t, db, EnqueueOpts{
391+
RepoID: repo.ID,
392+
GitRef: "sha-" + tt.name,
393+
Branch: tt.branch,
394+
}, "output")
395+
396+
review, err := db.GetReviewByJobID(job.ID)
397+
require.NoError(t, err)
398+
require.NotNil(t, review.Job)
399+
assert.Equal(t, tt.want, review.Job.Branch)
400+
})
401+
}
402+
}
403+
404+
func TestGetReviewByCommitSHAIncludesBranch(t *testing.T) {
405+
db := openTestDB(t)
406+
defer db.Close()
407+
408+
repo := createRepo(t, db, "/tmp/test-repo")
409+
410+
job := createCompletedJobWithOptions(t, db, EnqueueOpts{
411+
RepoID: repo.ID,
412+
GitRef: "branch-sha-test",
413+
Branch: "feature/x",
414+
}, "output")
415+
416+
review, err := db.GetReviewByCommitSHA(job.GitRef)
417+
require.NoError(t, err)
418+
require.NotNil(t, review.Job)
419+
assert.Equal(t, "feature/x", review.Job.Branch)
420+
}
421+
373422
// verifyComment helper checks if a comment matches expected values.
374423
func verifyComment(t *testing.T, actual Response, expectedUser, expectedMsg string) {
375424
t.Helper()

0 commit comments

Comments
 (0)