diff --git a/apps/backend/internal/github/service_pr_watch.go b/apps/backend/internal/github/service_pr_watch.go index 044b0eca9e..45ccc2d549 100644 --- a/apps/backend/internal/github/service_pr_watch.go +++ b/apps/backend/internal/github/service_pr_watch.go @@ -530,23 +530,14 @@ func (s *Service) findTaskPRForStatus(ctx context.Context, taskID string, pr *PR return nil, nil } -// SyncTaskPR updates a TaskPR record with the latest PR status. Multi-repo: -// the row is found by (task_id, owner, repo, pr_number) since the same -// task can have several PRs; the legacy GetTaskPR(taskID) "first match" -// would cross repos and silently update the wrong row. -// It only publishes a github.task_pr.updated event when data actually changed, -// preventing feedback loops with frontend sync handlers. -// -//nolint:cyclop // sequential field-by-field "populated/preserve" reconciliation; splitting helpers hides intent -func (s *Service) SyncTaskPR(ctx context.Context, taskID string, status *PRStatus) error { - if status == nil || status.PR == nil { - return fmt.Errorf("sync task PR: missing PR data for task %s", taskID) - } - tp, err := s.findTaskPRForStatus(ctx, taskID, status.PR) - if err != nil || tp == nil { - return err - } +type taskPRSyncState struct { + checksTotal, checksPassing int + unresolved, reviewCount, pendingReviewCount int + requiredReviews *int + baseBranch, mergeableState string +} +func (s *Service) prepareTaskPRSyncState(ctx context.Context, tp *TaskPR, status *PRStatus) taskPRSyncState { // Some sync paths (notably the batched GraphQL poller) don't populate // ChecksTotal / ChecksPassing — they only carry the rollup state. The // caller sets status.ChecksPopulated=true when it actually counted @@ -591,6 +582,37 @@ func (s *Service) SyncTaskPR(ctx context.Context, taskID string, status *PRStatu } else if fetched := s.fetchRequiredReviews(ctx, tp.Owner, tp.Repo, nextBaseBranch); fetched != nil { nextRequiredReviews = fetched } + // GitHub reports draft status separately from mergeStateStatus and can + // return CLEAN for a draft PR. Persist the effective blocker so every + // TaskPR consumer agrees that drafts are not ready to merge. + nextMergeableState := status.MergeableState + if status.PR.Draft { + nextMergeableState = "draft" + } + return taskPRSyncState{ + checksTotal: nextChecksTotal, checksPassing: nextChecksPassing, + unresolved: nextUnresolved, reviewCount: nextReviewCount, pendingReviewCount: nextPendingReviewCount, + requiredReviews: nextRequiredReviews, baseBranch: nextBaseBranch, mergeableState: nextMergeableState, + } +} + +// SyncTaskPR updates a TaskPR record with the latest PR status. Multi-repo: +// the row is found by (task_id, owner, repo, pr_number) since the same +// task can have several PRs; the legacy GetTaskPR(taskID) "first match" +// would cross repos and silently update the wrong row. +// It only publishes a github.task_pr.updated event when data actually changed, +// preventing feedback loops with frontend sync handlers. +// +//nolint:cyclop // sequential field-by-field reconciliation keeps update intent clear +func (s *Service) SyncTaskPR(ctx context.Context, taskID string, status *PRStatus) error { + if status == nil || status.PR == nil { + return fmt.Errorf("sync task PR: missing PR data for task %s", taskID) + } + tp, err := s.findTaskPRForStatus(ctx, taskID, status.PR) + if err != nil || tp == nil { + return err + } + next := s.prepareTaskPRSyncState(ctx, tp, status) changed := tp.State != status.PR.State || tp.PRTitle != status.PR.Title || @@ -598,14 +620,14 @@ func (s *Service) SyncTaskPR(ctx context.Context, taskID string, status *PRStatu tp.Deletions != status.PR.Deletions || tp.ReviewState != status.ReviewState || tp.ChecksState != status.ChecksState || - tp.MergeableState != status.MergeableState || - tp.ReviewCount != nextReviewCount || - tp.PendingReviewCount != nextPendingReviewCount || - !intPtrEqual(tp.RequiredReviews, nextRequiredReviews) || - tp.ChecksTotal != nextChecksTotal || - tp.ChecksPassing != nextChecksPassing || - tp.UnresolvedReviewThreads != nextUnresolved || - tp.BaseBranch != nextBaseBranch || + tp.MergeableState != next.mergeableState || + tp.ReviewCount != next.reviewCount || + tp.PendingReviewCount != next.pendingReviewCount || + !intPtrEqual(tp.RequiredReviews, next.requiredReviews) || + tp.ChecksTotal != next.checksTotal || + tp.ChecksPassing != next.checksPassing || + tp.UnresolvedReviewThreads != next.unresolved || + tp.BaseBranch != next.baseBranch || !timeEqual(tp.MergedAt, status.PR.MergedAt) || !timeEqual(tp.ClosedAt, status.PR.ClosedAt) @@ -617,14 +639,14 @@ func (s *Service) SyncTaskPR(ctx context.Context, taskID string, status *PRStatu tp.ClosedAt = status.PR.ClosedAt tp.ReviewState = status.ReviewState tp.ChecksState = status.ChecksState - tp.MergeableState = status.MergeableState - tp.ReviewCount = nextReviewCount - tp.PendingReviewCount = nextPendingReviewCount - tp.RequiredReviews = nextRequiredReviews - tp.ChecksTotal = nextChecksTotal - tp.ChecksPassing = nextChecksPassing - tp.UnresolvedReviewThreads = nextUnresolved - tp.BaseBranch = nextBaseBranch + tp.MergeableState = next.mergeableState + tp.ReviewCount = next.reviewCount + tp.PendingReviewCount = next.pendingReviewCount + tp.RequiredReviews = next.requiredReviews + tp.ChecksTotal = next.checksTotal + tp.ChecksPassing = next.checksPassing + tp.UnresolvedReviewThreads = next.unresolved + tp.BaseBranch = next.baseBranch // CommentCount is no longer updated from polling -- only refreshed on-demand now := time.Now().UTC() tp.LastSyncedAt = &now diff --git a/apps/backend/internal/github/service_test.go b/apps/backend/internal/github/service_test.go index 05612ad68e..a929695612 100644 --- a/apps/backend/internal/github/service_test.go +++ b/apps/backend/internal/github/service_test.go @@ -927,6 +927,56 @@ func TestSyncTaskPR_PublishesEventOnMergeableStateChange(t *testing.T) { } } +func TestSyncTaskPR_DraftOverridesCleanMergeableState(t *testing.T) { + svc, store, eb := setupSyncTest(t) + ctx := context.Background() + + if err := store.CreateTaskPR(ctx, &TaskPR{ + TaskID: "t1", + Owner: "owner", + Repo: "repo", + PRNumber: 1, + PRURL: "https://github.com/owner/repo/pull/1", + PRTitle: "Draft change", + HeadBranch: "feat", + BaseBranch: "main", + State: "open", + ChecksState: "success", + MergeableState: "clean", + }); err != nil { + t.Fatalf("create task PR: %v", err) + } + + status := &PRStatus{ + PR: &PR{ + Number: 1, + Title: "Draft change", + State: "open", + Draft: true, + RepoOwner: "owner", + RepoName: "repo", + MergeableState: "clean", + }, + ChecksState: "success", + MergeableState: "clean", + } + + if err := svc.SyncTaskPR(ctx, "t1", status); err != nil { + t.Fatalf("sync: %v", err) + } + if got := eb.publishedCount(); got != 1 { + t.Fatalf("expected draft normalization to publish an update, got %d events", got) + } + + stored, err := store.GetTaskPR(ctx, "t1") + if err != nil { + t.Fatalf("get task PR: %v", err) + } + if stored.MergeableState != "draft" { + t.Fatalf("stored mergeable_state = %q, want draft", stored.MergeableState) + } +} + // TestSyncTaskPR_ChecksPopulated_PreservesOnLightweightSync verifies the // "preserve when batch sync didn't populate" path: a PRStatus with // ChecksPopulated=false must NOT clobber the persisted ChecksTotal / diff --git a/apps/web/components/github/pr-status-chip.test.tsx b/apps/web/components/github/pr-status-chip.test.tsx index e01d03cf06..bd8b2943ef 100644 --- a/apps/web/components/github/pr-status-chip.test.tsx +++ b/apps/web/components/github/pr-status-chip.test.tsx @@ -286,6 +286,22 @@ describe("PRStatusChip mobile branch", () => { expect(screen.getByTestId(CHIP_TESTID).getAttribute(ATTR_STATUS)).toBe("failed"); }); + it("identifies a draft PR with passing CI as draft", () => { + renderWithStore( + { + taskPRs: { + byTaskId: { + "task-1": [makePR({ mergeable_state: "draft" })], + }, + }, + }, + , + ); + const chip = screen.getByTestId(CHIP_TESTID); + expect(chip.getAttribute(ATTR_STATUS)).toBe("draft"); + expect(chip.getAttribute(ATTR_READY_TO_MERGE)).toBe("false"); + }); + it("shows automation badges on the mobile chip trigger", () => { renderWithStore( { diff --git a/apps/web/components/github/pr-status-chip.tsx b/apps/web/components/github/pr-status-chip.tsx index 48f6cab8c5..ff3482ccf1 100644 --- a/apps/web/components/github/pr-status-chip.tsx +++ b/apps/web/components/github/pr-status-chip.tsx @@ -31,6 +31,7 @@ import { MultiPRCIPopover } from "@/components/github/multi-pr-ci-popover"; import { hasPRChecksInProgressForDisplay, hasPRChecksPassedForDisplay, + isPRDraft, isPRAwaitingReview, isPRReadyToMerge, isPRWaitingOnBranchProtection, @@ -52,6 +53,7 @@ type ChipStatus = | "conflict" | "blocked" | "behind" + | "draft" | "waiting" | "in_progress" | "neutral"; @@ -68,6 +70,7 @@ function chipStatus(pr: TaskPR): ChipStatus { // getPRStatusColor + PRStatusIcon (dirty = red, behind = amber). if (pr.mergeable_state === "dirty") return "conflict"; if (pr.mergeable_state === "behind") return "behind"; + if (isPRDraft(pr)) return "draft"; // Pending checks / pending review must beat checks_state === "success" so a // PR with all checks green but reviewers still outstanding renders as // in-progress, not passed. Without this order, the chip flips to green the @@ -92,6 +95,7 @@ const CHIP_STATUS_RANK: Record = { conflict: 5, blocked: 4, behind: 3, + draft: 0.5, in_progress: 2, waiting: 1.5, passed: 1, @@ -555,6 +559,8 @@ function ChipStatusGlyph({ status }: { status: ChipStatus }) { return