diff --git a/apps/backend/internal/github/client_helpers.go b/apps/backend/internal/github/client_helpers.go index b2eff70713..1c0073baca 100644 --- a/apps/backend/internal/github/client_helpers.go +++ b/apps/backend/internal/github/client_helpers.go @@ -93,6 +93,7 @@ func getPRStatus(ctx context.Context, c Client, owner, repo string, number int) PR: pr, ReviewState: reviewState, ChecksState: computeOverallCheckStatus(checks), + MergeableState: pr.MergeableState, ReviewCount: len(reviews), PendingReviewCount: pendingReviewCount, }, nil diff --git a/apps/backend/internal/github/constants.go b/apps/backend/internal/github/constants.go index f821664db1..02d7f8d57b 100644 --- a/apps/backend/internal/github/constants.go +++ b/apps/backend/internal/github/constants.go @@ -26,7 +26,14 @@ const ( checkStatusCompleted = "completed" checkStatusPending = "pending" checkStatusSuccess = "success" - checkConclusionFail = "failure" + + checkConclusionSuccess = "success" + checkConclusionFail = "failure" + checkConclusionSkipped = "skipped" + checkConclusionNeutral = "neutral" + checkConclusionCancelled = "cancelled" + checkConclusionTimedOut = "timed_out" + checkConclusionActionRequired = "action_required" ) // Check source identifiers. diff --git a/apps/backend/internal/github/gh_client.go b/apps/backend/internal/github/gh_client.go index d2222a8e92..1ef34d80a9 100644 --- a/apps/backend/internal/github/gh_client.go +++ b/apps/backend/internal/github/gh_client.go @@ -89,24 +89,25 @@ type ghRequestedReviewer struct { // ghPR is the JSON shape returned by gh pr list/view. type ghPR struct { - Number int `json:"number"` - Title string `json:"title"` - URL string `json:"url"` - State string `json:"state"` - Body string `json:"body"` - HeadRefName string `json:"headRefName"` - HeadRefOid string `json:"headRefOid"` - BaseRefName string `json:"baseRefName"` - IsDraft bool `json:"isDraft"` - Mergeable string `json:"mergeable"` - Additions int `json:"additions"` - Deletions int `json:"deletions"` - CreatedAt time.Time `json:"createdAt"` - UpdatedAt time.Time `json:"updatedAt"` - MergedAt string `json:"mergedAt"` - ClosedAt string `json:"closedAt"` - ReviewRequests []ghRequestedReviewer `json:"reviewRequests"` - Author struct { + Number int `json:"number"` + Title string `json:"title"` + URL string `json:"url"` + State string `json:"state"` + Body string `json:"body"` + HeadRefName string `json:"headRefName"` + HeadRefOid string `json:"headRefOid"` + BaseRefName string `json:"baseRefName"` + IsDraft bool `json:"isDraft"` + Mergeable string `json:"mergeable"` + MergeStateStatus string `json:"mergeStateStatus"` + Additions int `json:"additions"` + Deletions int `json:"deletions"` + CreatedAt time.Time `json:"createdAt"` + UpdatedAt time.Time `json:"updatedAt"` + MergedAt string `json:"mergedAt"` + ClosedAt string `json:"closedAt"` + ReviewRequests []ghRequestedReviewer `json:"reviewRequests"` + Author struct { Login string `json:"login"` } `json:"author"` } @@ -114,7 +115,7 @@ type ghPR struct { func (c *GHClient) GetPR(ctx context.Context, owner, repo string, number int) (*PR, error) { out, err := c.run(ctx, "pr", "view", fmt.Sprintf("%d", number), "--repo", fmt.Sprintf("%s/%s", owner, repo), - "--json", "number,title,url,state,body,headRefName,headRefOid,baseRefName,author,isDraft,mergeable,additions,deletions,createdAt,updatedAt,mergedAt,closedAt,reviewRequests") + "--json", "number,title,url,state,body,headRefName,headRefOid,baseRefName,author,isDraft,mergeable,mergeStateStatus,additions,deletions,createdAt,updatedAt,mergedAt,closedAt,reviewRequests") if err != nil { return nil, fmt.Errorf("get PR #%d: %w", number, err) } @@ -130,7 +131,7 @@ func (c *GHClient) FindPRByBranch(ctx context.Context, owner, repo, branch strin "--repo", fmt.Sprintf("%s/%s", owner, repo), "--head", branch, "--state", "open", - "--json", "number,title,url,state,headRefName,headRefOid,baseRefName,author,isDraft,mergeable,additions,deletions,createdAt,updatedAt", + "--json", "number,title,url,state,headRefName,headRefOid,baseRefName,author,isDraft,mergeable,mergeStateStatus,additions,deletions,createdAt,updatedAt", "--limit", "1") if err != nil { return nil, fmt.Errorf("find PR by branch %q: %w", branch, err) @@ -150,7 +151,7 @@ func (c *GHClient) ListAuthoredPRs(ctx context.Context, owner, repo string) ([]* "--repo", fmt.Sprintf("%s/%s", owner, repo), "--author", "@me", "--state", "open", - "--json", "number,title,url,state,headRefName,headRefOid,baseRefName,author,isDraft,mergeable,additions,deletions,createdAt,updatedAt") + "--json", "number,title,url,state,headRefName,headRefOid,baseRefName,author,isDraft,mergeable,mergeStateStatus,additions,deletions,createdAt,updatedAt") if err != nil { return nil, fmt.Errorf("list authored PRs: %w", err) } @@ -511,6 +512,7 @@ func convertGHPR(raw *ghPR, owner, repo string) *PR { RepoName: repo, Draft: raw.IsDraft, Mergeable: raw.Mergeable == "MERGEABLE", + MergeableState: strings.ToLower(raw.MergeStateStatus), Additions: raw.Additions, Deletions: raw.Deletions, RequestedReviewers: convertGHRequestedReviewers(raw.ReviewRequests), diff --git a/apps/backend/internal/github/gh_client_test.go b/apps/backend/internal/github/gh_client_test.go index 771d7a8f63..56c3a1be76 100644 --- a/apps/backend/internal/github/gh_client_test.go +++ b/apps/backend/internal/github/gh_client_test.go @@ -184,6 +184,37 @@ func TestConvertGHPR_NotMergeable(t *testing.T) { } } +func TestConvertGHPR_MergeStateStatus(t *testing.T) { + tests := []struct { + name string + rawEnum string + want string + }{ + {"clean", "CLEAN", "clean"}, + {"blocked", "BLOCKED", "blocked"}, + {"dirty", "DIRTY", "dirty"}, + {"behind", "BEHIND", "behind"}, + {"unknown", "UNKNOWN", "unknown"}, + {"empty", "", ""}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + raw := &ghPR{ + Number: 1, + State: "OPEN", + MergeStateStatus: tt.rawEnum, + Author: struct { + Login string `json:"login"` + }{Login: "alice"}, + } + pr := convertGHPR(raw, "owner", "repo") + if pr.MergeableState != tt.want { + t.Errorf("MergeableState = %q, want %q", pr.MergeableState, tt.want) + } + }) + } +} + func TestConvertGHRequestedReviewers(t *testing.T) { raw := []ghRequestedReviewer{ {TypeName: "User", Login: "alice"}, diff --git a/apps/backend/internal/github/handlers.go b/apps/backend/internal/github/handlers.go index 01b43fdaee..23badc6cb9 100644 --- a/apps/backend/internal/github/handlers.go +++ b/apps/backend/internal/github/handlers.go @@ -25,7 +25,7 @@ func RegisterMockRoutes(router *gin.Engine, svc *Service, log *logger.Logger) { if !ok { return } - ctrl := NewMockController(mock, svc.TestStore(), log) + ctrl := NewMockController(mock, svc.TestStore(), svc.TestEventBus(), log) ctrl.RegisterRoutes(router) log.Info("registered GitHub mock control endpoints") } diff --git a/apps/backend/internal/github/mock_controller.go b/apps/backend/internal/github/mock_controller.go index f8c67df579..18ebb41bfb 100644 --- a/apps/backend/internal/github/mock_controller.go +++ b/apps/backend/internal/github/mock_controller.go @@ -5,21 +5,26 @@ import ( "time" "github.com/gin-gonic/gin" + "go.uber.org/zap" "github.com/kandev/kandev/internal/common/logger" + "github.com/kandev/kandev/internal/events" + "github.com/kandev/kandev/internal/events/bus" ) const defaultPRState = "open" // MockController handles HTTP endpoints for controlling the MockClient in E2E tests. type MockController struct { - mock *MockClient - store *Store + mock *MockClient + store *Store + eventBus bus.EventBus + logger *logger.Logger } // NewMockController creates a new MockController. -func NewMockController(mock *MockClient, store *Store, _ *logger.Logger) *MockController { - return &MockController{mock: mock, store: store} +func NewMockController(mock *MockClient, store *Store, eventBus bus.EventBus, log *logger.Logger) *MockController { + return &MockController{mock: mock, store: store, eventBus: eventBus, logger: log} } // RegisterRoutes registers all mock control HTTP routes. @@ -186,18 +191,21 @@ func (c *MockController) addBranches(ctx *gin.Context) { // associateTaskPR directly creates a github_task_prs record for E2E testing. func (c *MockController) associateTaskPR(ctx *gin.Context) { var req struct { - TaskID string `json:"task_id"` - Owner string `json:"owner"` - Repo string `json:"repo"` - PRNumber int `json:"pr_number"` - PRURL string `json:"pr_url"` - PRTitle string `json:"pr_title"` - HeadBranch string `json:"head_branch"` - BaseBranch string `json:"base_branch"` - AuthorLogin string `json:"author_login"` - State string `json:"state"` - Additions int `json:"additions"` - Deletions int `json:"deletions"` + TaskID string `json:"task_id"` + Owner string `json:"owner"` + Repo string `json:"repo"` + PRNumber int `json:"pr_number"` + PRURL string `json:"pr_url"` + PRTitle string `json:"pr_title"` + HeadBranch string `json:"head_branch"` + BaseBranch string `json:"base_branch"` + AuthorLogin string `json:"author_login"` + State string `json:"state"` + ReviewState string `json:"review_state"` + ChecksState string `json:"checks_state"` + MergeableState string `json:"mergeable_state"` + Additions int `json:"additions"` + Deletions int `json:"deletions"` } if err := ctx.ShouldBindJSON(&req); err != nil { ctx.JSON(http.StatusBadRequest, gin.H{"error": "invalid payload"}) @@ -208,24 +216,35 @@ func (c *MockController) associateTaskPR(ctx *gin.Context) { } now := time.Now().UTC() tp := &TaskPR{ - TaskID: req.TaskID, - Owner: req.Owner, - Repo: req.Repo, - PRNumber: req.PRNumber, - PRURL: req.PRURL, - PRTitle: req.PRTitle, - HeadBranch: req.HeadBranch, - BaseBranch: req.BaseBranch, - AuthorLogin: req.AuthorLogin, - State: req.State, - Additions: req.Additions, - Deletions: req.Deletions, - CreatedAt: now, + TaskID: req.TaskID, + Owner: req.Owner, + Repo: req.Repo, + PRNumber: req.PRNumber, + PRURL: req.PRURL, + PRTitle: req.PRTitle, + HeadBranch: req.HeadBranch, + BaseBranch: req.BaseBranch, + AuthorLogin: req.AuthorLogin, + State: req.State, + ReviewState: req.ReviewState, + ChecksState: req.ChecksState, + MergeableState: req.MergeableState, + Additions: req.Additions, + Deletions: req.Deletions, + CreatedAt: now, } if err := c.store.CreateTaskPR(ctx.Request.Context(), tp); err != nil { ctx.JSON(http.StatusInternalServerError, gin.H{"error": err.Error()}) return } + // Publish the event so the frontend Zustand store picks up the new PR + // without requiring a page reload — mirrors real AssociatePRWithTask. + if c.eventBus != nil { + event := bus.NewEvent(events.GitHubTaskPRUpdated, "github", tp) + if err := c.eventBus.Publish(ctx.Request.Context(), events.GitHubTaskPRUpdated, event); err != nil { + c.logger.Debug("mock: failed to publish task PR updated event", zap.Error(err)) + } + } ctx.JSON(http.StatusCreated, tp) } diff --git a/apps/backend/internal/github/models.go b/apps/backend/internal/github/models.go index 39b1c9b692..623384ec16 100644 --- a/apps/backend/internal/github/models.go +++ b/apps/backend/internal/github/models.go @@ -20,6 +20,7 @@ type PR struct { Body string `json:"body"` Draft bool `json:"draft"` Mergeable bool `json:"mergeable"` + MergeableState string `json:"mergeable_state"` // clean, blocked, behind, dirty, has_hooks, unstable, draft, unknown, "" Additions int `json:"additions"` Deletions int `json:"deletions"` RequestedReviewers []RequestedReviewer `json:"requested_reviewers"` @@ -86,8 +87,9 @@ type PRFeedback struct { // Unlike PRFeedback, it skips comments to reduce API calls. type PRStatus struct { PR *PR `json:"pr"` - ReviewState string `json:"review_state"` // "approved", "changes_requested", "pending", "" - ChecksState string `json:"checks_state"` // "success", "failure", "pending", "" + ReviewState string `json:"review_state"` // "approved", "changes_requested", "pending", "" + ChecksState string `json:"checks_state"` // "success", "failure", "pending", "" + MergeableState string `json:"mergeable_state"` // "clean", "blocked", "behind", "dirty", "has_hooks", "unstable", "draft", "unknown", "" ReviewCount int `json:"review_count"` PendingReviewCount int `json:"pending_review_count"` } @@ -121,9 +123,10 @@ type TaskPR struct { HeadBranch string `json:"head_branch" db:"head_branch"` BaseBranch string `json:"base_branch" db:"base_branch"` AuthorLogin string `json:"author_login" db:"author_login"` - State string `json:"state" db:"state"` // open, closed, merged - ReviewState string `json:"review_state" db:"review_state"` // approved, changes_requested, pending, "" - ChecksState string `json:"checks_state" db:"checks_state"` // success, failure, pending, "" + State string `json:"state" db:"state"` // open, closed, merged + ReviewState string `json:"review_state" db:"review_state"` // approved, changes_requested, pending, "" + ChecksState string `json:"checks_state" db:"checks_state"` // success, failure, pending, "" + MergeableState string `json:"mergeable_state" db:"mergeable_state"` // clean, blocked, behind, dirty, has_hooks, unstable, draft, unknown, "" ReviewCount int `json:"review_count" db:"review_count"` PendingReviewCount int `json:"pending_review_count" db:"pending_review_count"` CommentCount int `json:"comment_count" db:"comment_count"` diff --git a/apps/backend/internal/github/pat_client.go b/apps/backend/internal/github/pat_client.go index 124469dace..6188478307 100644 --- a/apps/backend/internal/github/pat_client.go +++ b/apps/backend/internal/github/pat_client.go @@ -412,6 +412,7 @@ type patPR struct { State string `json:"state"` Draft bool `json:"draft"` Mergeable *bool `json:"mergeable"` + MergeableState string `json:"mergeable_state"` Additions int `json:"additions"` Deletions int `json:"deletions"` CreatedAt time.Time `json:"created_at"` @@ -474,6 +475,7 @@ func convertPatPR(raw *patPR, owner, repo string) *PR { RepoName: repo, Draft: raw.Draft, Mergeable: mergeable, + MergeableState: strings.ToLower(raw.MergeableState), Additions: raw.Additions, Deletions: raw.Deletions, RequestedReviewers: convertPatRequestedReviewers(raw), diff --git a/apps/backend/internal/github/pat_client_test.go b/apps/backend/internal/github/pat_client_test.go index 9c78056bed..8d7fe1e304 100644 --- a/apps/backend/internal/github/pat_client_test.go +++ b/apps/backend/internal/github/pat_client_test.go @@ -125,6 +125,29 @@ func TestConvertPatPR_Mergeable(t *testing.T) { } } +func TestConvertPatPR_MergeableState(t *testing.T) { + raw := &patPR{ + Number: 2, + State: "open", + MergeableState: "CLEAN", // GitHub REST uses lowercase but be defensive + User: struct { + Login string `json:"login"` + }{Login: "alice"}, + Head: struct { + Ref string `json:"ref"` + SHA string `json:"sha"` + }{Ref: "b"}, + Base: struct { + Ref string `json:"ref"` + }{Ref: "main"}, + } + + pr := convertPatPR(raw, "o", "r") + if pr.MergeableState != "clean" { + t.Errorf("expected normalized mergeable_state=clean, got %q", pr.MergeableState) + } +} + func TestConvertPatRequestedReviewers(t *testing.T) { raw := &patPR{ RequestedReviewers: []struct { diff --git a/apps/backend/internal/github/service.go b/apps/backend/internal/github/service.go index 3d5f5058f4..18a3a7df39 100644 --- a/apps/backend/internal/github/service.go +++ b/apps/backend/internal/github/service.go @@ -77,6 +77,11 @@ func (s *Service) TestStore() *Store { return s.store } +// TestEventBus returns the event bus for test/mock use only. +func (s *Service) TestEventBus() bus.EventBus { + return s.eventBus +} + // IsAuthenticated returns whether the service has a working GitHub client. // Returns false when using the NoopClient fallback (authMethod == "none"). func (s *Service) IsAuthenticated() bool { @@ -475,6 +480,7 @@ 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 != status.ReviewCount || tp.PendingReviewCount != status.PendingReviewCount || !timeEqual(tp.MergedAt, status.PR.MergedAt) || @@ -488,6 +494,7 @@ 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 = status.ReviewCount tp.PendingReviewCount = status.PendingReviewCount // CommentCount is no longer updated from polling -- only refreshed on-demand @@ -1152,23 +1159,41 @@ func findLatestCommentTime(comments []PRComment) *time.Time { return latest } +// computeOverallCheckStatus reduces per-check runs to a single PR-level status. +// Mirrors GitHub's own UI: skipped/neutral conclusions are ignored; any failing +// terminal state (failure, timed_out, cancelled, action_required) makes the PR +// failed; non-completed checks keep the PR pending. func computeOverallCheckStatus(checks []CheckRun) string { if len(checks) == 0 { return "" } hasPending := false + hasPassing := false for _, c := range checks { - if c.Status == checkStatusCompleted && c.Conclusion == checkConclusionFail { - return checkConclusionFail - } if c.Status != checkStatusCompleted { hasPending = true + continue + } + switch c.Conclusion { + case checkConclusionFail, checkConclusionTimedOut, + checkConclusionCancelled, checkConclusionActionRequired: + return checkConclusionFail + case checkConclusionSkipped, checkConclusionNeutral: + // ignore — GitHub's UI does + default: + // Treat success and any future unknown terminal conclusion as passing. + // Being permissive preserves the success signal if GitHub introduces + // a new conclusion we haven't mapped yet. + hasPassing = true } } if hasPending { return checkStatusPending } - return checkStatusSuccess + if hasPassing { + return checkStatusSuccess + } + return "" } func computeOverallReviewState(reviews []PRReview) string { diff --git a/apps/backend/internal/github/service_test.go b/apps/backend/internal/github/service_test.go index c3cb081ceb..84e1b0d843 100644 --- a/apps/backend/internal/github/service_test.go +++ b/apps/backend/internal/github/service_test.go @@ -183,6 +183,83 @@ func TestComputeOverallCheckStatus(t *testing.T) { }, "pending", }, + { + // Regression for the "CI pending" badge bug: GitHub shows + // "All checks have passed — 1 skipped, 20 successful" but + // we were returning pending because the skipped runs weren't + // treated as completed-successful. + "AllSkipped mixed with success returns success", + func() []CheckRun { + checks := make([]CheckRun, 21) + for i := 0; i < 20; i++ { + checks[i] = CheckRun{Status: "completed", Conclusion: "success"} + } + checks[20] = CheckRun{Status: "completed", Conclusion: "skipped"} + return checks + }(), + "success", + }, + { + "only skipped returns empty (no signal)", + []CheckRun{ + {Status: "completed", Conclusion: "skipped"}, + {Status: "completed", Conclusion: "skipped"}, + }, + "", + }, + { + "only neutral returns empty (no signal)", + []CheckRun{ + {Status: "completed", Conclusion: "neutral"}, + {Status: "completed", Conclusion: "neutral"}, + }, + "", + }, + { + "unknown future conclusion treated as passing", + []CheckRun{ + {Status: "completed", Conclusion: "stale"}, // hypothetical future enum + }, + "success", + }, + { + "neutral conclusion is ignored like skipped", + []CheckRun{ + {Status: "completed", Conclusion: "success"}, + {Status: "completed", Conclusion: "neutral"}, + }, + "success", + }, + { + "cancelled conclusion counts as failure", + []CheckRun{ + {Status: "completed", Conclusion: "success"}, + {Status: "completed", Conclusion: "cancelled"}, + }, + "failure", + }, + { + "timed_out conclusion counts as failure", + []CheckRun{ + {Status: "completed", Conclusion: "timed_out"}, + }, + "failure", + }, + { + "action_required conclusion counts as failure", + []CheckRun{ + {Status: "completed", Conclusion: "action_required"}, + }, + "failure", + }, + { + "in_progress plus skipped returns pending (skipped ignored)", + []CheckRun{ + {Status: "in_progress"}, + {Status: "completed", Conclusion: "skipped"}, + }, + "pending", + }, } for _, tt := range tests { @@ -619,6 +696,63 @@ func TestSyncTaskPR_SecondIdenticalSyncNoEvent(t *testing.T) { } } +func TestSyncTaskPR_PublishesEventOnMergeableStateChange(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: "Same", + HeadBranch: "feat", + BaseBranch: "main", + State: "open", + Additions: 3, + Deletions: 1, + ReviewState: "approved", + ChecksState: "success", + MergeableState: "blocked", + ReviewCount: 1, + }); err != nil { + t.Fatalf("create task PR: %v", err) + } + + // Only mergeable_state changes (blocked -> clean); everything else identical. + status := &PRStatus{ + PR: &PR{ + Number: 1, + Title: "Same", + State: "open", + Additions: 3, + Deletions: 1, + RepoOwner: "owner", + RepoName: "repo", + }, + ReviewState: "approved", + ChecksState: "success", + MergeableState: "clean", + ReviewCount: 1, + } + + if err := svc.SyncTaskPR(ctx, "t1", status); err != nil { + t.Fatalf("sync: %v", err) + } + if got := eb.publishedCount(); got != 1 { + t.Errorf("expected 1 event after mergeable_state change, got %d", got) + } + + stored, err := store.GetTaskPR(ctx, "t1") + if err != nil { + t.Fatalf("get task PR: %v", err) + } + if stored.MergeableState != "clean" { + t.Errorf("expected stored mergeable_state=clean, got %q", stored.MergeableState) + } +} + func TestTimeEqual(t *testing.T) { now := time.Now() later := now.Add(time.Second) diff --git a/apps/backend/internal/github/store.go b/apps/backend/internal/github/store.go index 491a0cff9f..8f1b160680 100644 --- a/apps/backend/internal/github/store.go +++ b/apps/backend/internal/github/store.go @@ -59,6 +59,7 @@ const createTablesSQL = ` state TEXT NOT NULL DEFAULT 'open', review_state TEXT NOT NULL DEFAULT '', checks_state TEXT NOT NULL DEFAULT '', + mergeable_state TEXT NOT NULL DEFAULT '', review_count INTEGER DEFAULT 0, pending_review_count INTEGER DEFAULT 0, comment_count INTEGER DEFAULT 0, @@ -109,6 +110,7 @@ func (s *Store) initSchema() error { } // Idempotent migrations for existing databases. _, _ = s.db.Exec(`ALTER TABLE github_pr_watches ADD COLUMN last_review_state TEXT DEFAULT ''`) + _, _ = s.db.Exec(`ALTER TABLE github_task_prs ADD COLUMN mergeable_state TEXT NOT NULL DEFAULT ''`) return nil } @@ -211,11 +213,11 @@ func (s *Store) CreateTaskPR(ctx context.Context, tp *TaskPR) error { tp.UpdatedAt = now _, err := s.db.ExecContext(ctx, ` INSERT INTO github_task_prs (id, task_id, owner, repo, pr_number, pr_url, pr_title, head_branch, base_branch, author_login, - state, review_state, checks_state, review_count, pending_review_count, comment_count, additions, deletions, + state, review_state, checks_state, mergeable_state, review_count, pending_review_count, comment_count, additions, deletions, created_at, merged_at, closed_at, last_synced_at, updated_at) - VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)`, + VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)`, tp.ID, tp.TaskID, tp.Owner, tp.Repo, tp.PRNumber, tp.PRURL, tp.PRTitle, tp.HeadBranch, tp.BaseBranch, tp.AuthorLogin, - tp.State, tp.ReviewState, tp.ChecksState, tp.ReviewCount, tp.PendingReviewCount, tp.CommentCount, tp.Additions, tp.Deletions, + tp.State, tp.ReviewState, tp.ChecksState, tp.MergeableState, tp.ReviewCount, tp.PendingReviewCount, tp.CommentCount, tp.Additions, tp.Deletions, tp.CreatedAt, tp.MergedAt, tp.ClosedAt, tp.LastSyncedAt, tp.UpdatedAt) return err } @@ -290,11 +292,11 @@ func (s *Store) ReplaceTaskPR(ctx context.Context, tp *TaskPR) error { } if _, err := tx.ExecContext(ctx, ` INSERT INTO github_task_prs (id, task_id, owner, repo, pr_number, pr_url, pr_title, head_branch, base_branch, author_login, - state, review_state, checks_state, review_count, pending_review_count, comment_count, additions, deletions, + state, review_state, checks_state, mergeable_state, review_count, pending_review_count, comment_count, additions, deletions, created_at, merged_at, closed_at, last_synced_at, updated_at) - VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)`, + VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)`, tp.ID, tp.TaskID, tp.Owner, tp.Repo, tp.PRNumber, tp.PRURL, tp.PRTitle, tp.HeadBranch, tp.BaseBranch, tp.AuthorLogin, - tp.State, tp.ReviewState, tp.ChecksState, tp.ReviewCount, tp.PendingReviewCount, tp.CommentCount, tp.Additions, tp.Deletions, + tp.State, tp.ReviewState, tp.ChecksState, tp.MergeableState, tp.ReviewCount, tp.PendingReviewCount, tp.CommentCount, tp.Additions, tp.Deletions, tp.CreatedAt, tp.MergedAt, tp.ClosedAt, tp.LastSyncedAt, tp.UpdatedAt); err != nil { return err } @@ -305,12 +307,12 @@ func (s *Store) ReplaceTaskPR(ctx context.Context, tp *TaskPR) error { func (s *Store) UpdateTaskPR(ctx context.Context, tp *TaskPR) error { tp.UpdatedAt = time.Now().UTC() _, err := s.db.ExecContext(ctx, ` - UPDATE github_task_prs SET state = ?, review_state = ?, checks_state = ?, + UPDATE github_task_prs SET state = ?, review_state = ?, checks_state = ?, mergeable_state = ?, review_count = ?, pending_review_count = ?, comment_count = ?, additions = ?, deletions = ?, pr_title = ?, merged_at = ?, closed_at = ?, last_synced_at = ?, updated_at = ? WHERE id = ?`, - tp.State, tp.ReviewState, tp.ChecksState, + tp.State, tp.ReviewState, tp.ChecksState, tp.MergeableState, tp.ReviewCount, tp.PendingReviewCount, tp.CommentCount, tp.Additions, tp.Deletions, tp.PRTitle, tp.MergedAt, tp.ClosedAt, tp.LastSyncedAt, tp.UpdatedAt, tp.ID) diff --git a/apps/web/components/github/pr-task-icon.test.ts b/apps/web/components/github/pr-task-icon.test.ts new file mode 100644 index 0000000000..4a87a8b896 --- /dev/null +++ b/apps/web/components/github/pr-task-icon.test.ts @@ -0,0 +1,171 @@ +import { describe, it, expect } from "vitest"; +import { getPRStatusColor, getPRTooltip, isPRReadyToMerge } from "./pr-task-icon"; +import type { TaskPR } from "@/lib/types/github"; + +function makePR(overrides: Partial = {}): TaskPR { + return { + id: "id", + task_id: "task", + owner: "o", + repo: "r", + pr_number: 1, + pr_url: "", + pr_title: "Test PR", + head_branch: "feat", + base_branch: "main", + author_login: "alice", + state: "open", + review_state: "", + checks_state: "", + mergeable_state: "", + review_count: 0, + pending_review_count: 0, + comment_count: 0, + additions: 0, + deletions: 0, + created_at: "", + merged_at: null, + closed_at: null, + last_synced_at: null, + updated_at: "", + ...overrides, + }; +} + +describe("isPRReadyToMerge", () => { + it("is true only when open + approved + success + clean", () => { + expect( + isPRReadyToMerge( + makePR({ + state: "open", + review_state: "approved", + checks_state: "success", + mergeable_state: "clean", + }), + ), + ).toBe(true); + }); + + it("is false when mergeable_state is blocked", () => { + expect( + isPRReadyToMerge( + makePR({ + state: "open", + review_state: "approved", + checks_state: "success", + mergeable_state: "blocked", + }), + ), + ).toBe(false); + }); + + it("is false when state is merged", () => { + expect( + isPRReadyToMerge( + makePR({ + state: "merged", + review_state: "approved", + checks_state: "success", + mergeable_state: "clean", + }), + ), + ).toBe(false); + }); + + it.each(["behind", "dirty", "has_hooks", "unstable", "draft", "unknown", ""] as const)( + "is false when mergeable_state is %s", + (mergeable_state) => { + expect( + isPRReadyToMerge( + makePR({ + state: "open", + review_state: "approved", + checks_state: "success", + mergeable_state, + }), + ), + ).toBe(false); + }, + ); +}); + +describe("getPRStatusColor", () => { + it("returns ready-to-merge color when all conditions are met", () => { + const pr = makePR({ + state: "open", + review_state: "approved", + checks_state: "success", + mergeable_state: "clean", + }); + expect(getPRStatusColor(pr)).toBe("text-emerald-400"); + }); + + it("returns plain green for approved+success but mergeable_state blocked", () => { + const pr = makePR({ + state: "open", + review_state: "approved", + checks_state: "success", + mergeable_state: "blocked", + }); + expect(getPRStatusColor(pr)).toBe("text-green-500"); + }); + + it("returns plain green when mergeable_state is empty (backfilled row)", () => { + const pr = makePR({ + state: "open", + review_state: "approved", + checks_state: "success", + mergeable_state: "", + }); + expect(getPRStatusColor(pr)).toBe("text-green-500"); + }); + + it("returns red for changes_requested regardless of mergeable_state", () => { + const pr = makePR({ + state: "open", + review_state: "changes_requested", + checks_state: "success", + mergeable_state: "clean", + }); + expect(getPRStatusColor(pr)).toBe("text-red-500"); + }); + + it("returns yellow for pending CI", () => { + const pr = makePR({ state: "open", checks_state: "pending" }); + expect(getPRStatusColor(pr)).toBe("text-yellow-500"); + }); + + it("returns purple for merged", () => { + expect(getPRStatusColor(makePR({ state: "merged" }))).toBe("text-purple-500"); + }); +}); + +describe("getPRTooltip", () => { + it("includes 'Ready to merge' when ready", () => { + const pr = makePR({ + state: "open", + review_state: "approved", + checks_state: "success", + mergeable_state: "clean", + }); + expect(getPRTooltip(pr)).toContain("Ready to merge"); + }); + + it("includes 'Mergeable: blocked' when blocked", () => { + const pr = makePR({ + state: "open", + review_state: "approved", + checks_state: "success", + mergeable_state: "blocked", + }); + expect(getPRTooltip(pr)).toContain("Mergeable: blocked"); + expect(getPRTooltip(pr)).not.toContain("Ready to merge"); + }); + + it("omits mergeable when state is empty or unknown", () => { + const empty = makePR({ state: "open", mergeable_state: "" }); + const unknown = makePR({ state: "open", mergeable_state: "unknown" }); + expect(getPRTooltip(empty)).not.toContain("Mergeable:"); + expect(getPRTooltip(unknown)).not.toContain("Mergeable:"); + }); +}); diff --git a/apps/web/components/github/pr-task-icon.tsx b/apps/web/components/github/pr-task-icon.tsx index cf27af16b0..d3a2f8017f 100644 --- a/apps/web/components/github/pr-task-icon.tsx +++ b/apps/web/components/github/pr-task-icon.tsx @@ -6,12 +6,27 @@ import { cn } from "@/lib/utils"; import { useAppStore } from "@/components/state-provider"; import type { TaskPR } from "@/lib/types/github"; +// Requires checks_state === "success" (not just "") so repos with no CI configured +// won't trigger ready-to-merge on mergeable_state=clean alone. Loosen if we start +// supporting CI-less repos. +export function isPRReadyToMerge(pr: TaskPR): boolean { + return ( + pr.state === "open" && + pr.checks_state === "success" && + pr.review_state === "approved" && + pr.mergeable_state === "clean" + ); +} + export function getPRStatusColor(pr: TaskPR): string { if (pr.state === "merged") return "text-purple-500"; if (pr.state === "closed") return "text-red-500"; if (pr.review_state === "changes_requested" || pr.checks_state === "failure") { return "text-red-500"; } + if (isPRReadyToMerge(pr)) { + return "text-emerald-400"; + } if (pr.review_state === "approved" && pr.checks_state === "success") { return "text-green-500"; } @@ -21,11 +36,16 @@ export function getPRStatusColor(pr: TaskPR): string { return "text-muted-foreground"; } -function getPRTooltip(pr: TaskPR): string { +export function getPRTooltip(pr: TaskPR): string { const parts = [`PR #${pr.pr_number}: ${pr.pr_title}`]; if (pr.state !== "open") parts.push(`State: ${pr.state}`); if (pr.review_state) parts.push(`Review: ${pr.review_state}`); if (pr.checks_state) parts.push(`CI: ${pr.checks_state}`); + if (isPRReadyToMerge(pr)) { + parts.push("Ready to merge"); + } else if (pr.mergeable_state && pr.mergeable_state !== "unknown" && pr.state === "open") { + parts.push(`Mergeable: ${pr.mergeable_state}`); + } return parts.join(" | "); } @@ -40,6 +60,7 @@ export function PRTaskIcon({ taskId }: { taskId: string }) { diff --git a/apps/web/components/github/pr-topbar-button.tsx b/apps/web/components/github/pr-topbar-button.tsx index a96ea192a6..74ce4ef58d 100644 --- a/apps/web/components/github/pr-topbar-button.tsx +++ b/apps/web/components/github/pr-topbar-button.tsx @@ -6,7 +6,7 @@ import { Button } from "@kandev/ui/button"; import { Tooltip, TooltipContent, TooltipTrigger } from "@kandev/ui/tooltip"; import { useDockviewStore } from "@/lib/state/dockview-store"; import { useActiveTaskPR, useTaskPR } from "@/hooks/domains/github/use-task-pr"; -import { getPRStatusColor } from "@/components/github/pr-task-icon"; +import { getPRStatusColor, isPRReadyToMerge } from "@/components/github/pr-task-icon"; import { useAppStore } from "@/components/state-provider"; import type { TaskPR } from "@/lib/types/github"; @@ -22,6 +22,9 @@ function PRStatusIcon({ pr }: { pr: TaskPR }) { if (pr.checks_state === "failure" || pr.review_state === "changes_requested") { return ; } + if (isPRReadyToMerge(pr)) { + return ; + } if (pr.checks_state === "success" && pr.review_state === "approved") { return ; } @@ -45,6 +48,8 @@ export const PRTopbarButton = memo(function PRTopbarButton() {