diff --git a/backend/internal/review/planner.go b/backend/internal/review/planner.go index a2cbdd6fa5..082eb1b950 100644 --- a/backend/internal/review/planner.go +++ b/backend/internal/review/planner.go @@ -36,7 +36,8 @@ type PRReviewState struct { // review runs. It is pure so the trigger path and API list path share exactly // the same eligibility/status rules. func Plan(prs []domain.PullRequest, runs []domain.ReviewRun) []PRReviewState { - latest := latestRunsByPRAndSHA(runs) + latestForHead := latestRunsByPRAndSHA(runs) + latestForPR := latestRunsByPR(runs) reviews := make([]PRReviewState, 0, len(prs)) for _, pr := range prs { review := PRReviewState{ @@ -48,13 +49,13 @@ func Plan(prs []domain.PullRequest, runs []domain.ReviewRun) []PRReviewState { } if pr.URL == "" || pr.HeadSHA == "" || pr.Draft || pr.Merged || pr.Closed { review.Status = ReviewStateIneligible - if run, ok := latest[review.PRURL+"\x00"+review.TargetSHA]; ok { + if run, ok := latestForPR[review.PRURL]; ok { review.LatestRun = &run } reviews = append(reviews, review) continue } - if run, ok := latest[review.PRURL+"\x00"+review.TargetSHA]; ok { + if run, ok := latestForHead[review.PRURL+"\x00"+review.TargetSHA]; ok { review.LatestRun = &run switch { case run.Status == domain.ReviewRunRunning: @@ -68,6 +69,8 @@ func Plan(prs []domain.PullRequest, runs []domain.ReviewRun) []PRReviewState { default: review.Status = ReviewStateNeedsReview } + } else if run, ok := latestForPR[review.PRURL]; ok { + review.LatestRun = &run } reviews = append(reviews, review) } @@ -80,6 +83,19 @@ func Plan(prs []domain.PullRequest, runs []domain.ReviewRun) []PRReviewState { return reviews } +func latestRunsByPR(runs []domain.ReviewRun) map[string]domain.ReviewRun { + latest := make(map[string]domain.ReviewRun) + for _, run := range runs { + if run.PRURL == "" { + continue + } + if existing, ok := latest[run.PRURL]; !ok || run.CreatedAt.After(existing.CreatedAt) { + latest[run.PRURL] = run + } + } + return latest +} + func latestRunsByPRAndSHA(runs []domain.ReviewRun) map[string]domain.ReviewRun { latest := make(map[string]domain.ReviewRun) for _, run := range runs { diff --git a/backend/internal/review/planner_test.go b/backend/internal/review/planner_test.go index 9b655f2a30..c2a041cada 100644 --- a/backend/internal/review/planner_test.go +++ b/backend/internal/review/planner_test.go @@ -48,6 +48,23 @@ func TestPlanStatuses(t *testing.T) { } } +func TestPlanKeepsLatestRunForPRWhenHeadChanges(t *testing.T) { + runs := []domain.ReviewRun{ + {ID: "older", PRURL: "pr1", TargetSHA: "sha0", Status: domain.ReviewRunComplete, Verdict: domain.VerdictApproved, CreatedAt: time.Unix(1, 0).UTC()}, + {ID: "latest", PRURL: "pr1", TargetSHA: "sha1", Status: domain.ReviewRunDelivered, Verdict: domain.VerdictChangesRequested, CreatedAt: time.Unix(2, 0).UTC()}, + } + got := Plan([]domain.PullRequest{planPR("pr1", 1, "sha2")}, runs) + if len(got) != 1 { + t.Fatalf("review states = %d, want 1", len(got)) + } + if got[0].Status != ReviewStateNeedsReview { + t.Fatalf("status = %s, want %s", got[0].Status, ReviewStateNeedsReview) + } + if got[0].LatestRun == nil || got[0].LatestRun.ID != "latest" { + t.Fatalf("latest run = %+v, want latest", got[0].LatestRun) + } +} + func planPR(url string, n int, sha string) domain.PullRequest { return domain.PullRequest{URL: url, Number: n, HeadSHA: sha} } diff --git a/frontend/src/renderer/components/SessionInspector.tsx b/frontend/src/renderer/components/SessionInspector.tsx index e7ae9d64db..1e1c81dd2f 100644 --- a/frontend/src/renderer/components/SessionInspector.tsx +++ b/frontend/src/renderer/components/SessionInspector.tsx @@ -579,6 +579,18 @@ function sessionReviewVerdict(reviewStates: PRReviewState[]): { if (reviewStates.some((reviewState) => reviewState.latestRun?.status === "failed")) { return { label: "Failed", tone: "danger" }; } + if (reviewStates.some((reviewState) => reviewState.latestRun?.verdict === "changes_requested")) { + return { label: "Changes requested", tone: "danger" }; + } + const reviewsWithVerdicts = reviewStates.filter( + (reviewState) => reviewState.status !== "ineligible" && reviewState.latestRun?.verdict, + ); + if ( + reviewsWithVerdicts.length > 0 && + reviewsWithVerdicts.every((reviewState) => reviewState.latestRun?.verdict === "approved") + ) { + return { label: "Approved", tone: "success" }; + } if (reviewStates.some((reviewState) => reviewState.status === "changes_requested")) { return { label: "Changes requested", tone: "danger" }; } @@ -596,6 +608,12 @@ function reviewVerdict(reviewState: PRReviewState): { if (reviewState.latestRun?.status === "failed") { return { label: "Failed", tone: "danger" }; } + if (reviewState.latestRun?.verdict === "changes_requested") { + return { label: "Changes requested", tone: "danger" }; + } + if (reviewState.latestRun?.verdict === "approved") { + return { label: "Approved", tone: "success" }; + } switch (reviewState.status) { case "running": return { label: "Reviewing...", tone: "running" };