dreampiler/kandev
1
0
Fork 0
Code Issues 43 Pull Requests Actions 6 Packages Projects Releases Wiki Activity

fix(github): treat skipped checks as passing and add ready-to-merge status (#616)

Browse Source
This commit is contained in:
Carlos Florêncio
2026-04-17 16:33:43 +01:00
committed by GitHub
parent 6cbf050fa1
commit dc827a25d6
18 changed files with 751 additions and 71 deletions
Download Patch File Download Diff File Expand all files Collapse all files
apps/backend/internal/github/client_helpers.go
+1
View File
@@ -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
apps/backend/internal/github/constants.go
+8 -1
View File
@@ -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.
apps/backend/internal/github/gh_client.go
+23 -21
View File
@@ -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),
apps/backend/internal/github/gh_client_test.go
+31
View File
@@ -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"},
apps/backend/internal/github/handlers.go
+1 -1
View File
@@ -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")
}
apps/backend/internal/github/mock_controller.go
+48 -29
View File
@@ -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)
}
apps/backend/internal/github/models.go
+8 -5
View File
@@ -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"`
apps/backend/internal/github/pat_client.go
+2
View File
@@ -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),
apps/backend/internal/github/pat_client_test.go
+23
View File
@@ -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 {
apps/backend/internal/github/service.go
+29 -4
View File
@@ -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 {
apps/backend/internal/github/service_test.go
+134
View File
@@ -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)
apps/backend/internal/github/store.go
+10 -8
View File
@@ -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)
apps/web/components/github/pr-task-icon.test.ts
+171
View File
@@ -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> = {}): 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:");
});
});
apps/web/components/github/pr-task-icon.tsx
+22 -1
View File
@@ -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 }) {
<span
data-testid={`pr-task-icon-${taskId}`}
data-pr-state={pr.state}
data-pr-ready-to-merge={isPRReadyToMerge(pr) ? "true" : "false"}
className={cn("inline-flex items-center shrink-0", getPRStatusColor(pr))}
>
<IconGitPullRequest className="h-3.5 w-3.5" />
apps/web/components/github/pr-topbar-button.tsx
+6 -1
View File
@@ -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 <IconX className="h-3 w-3 text-red-500" />;
}
if (isPRReadyToMerge(pr)) {
return <IconCheck className="h-3 w-3 text-emerald-400" />;
}
if (pr.checks_state === "success" && pr.review_state === "approved") {
return <IconCheck className="h-3 w-3 text-green-500" />;
}
@@ -45,6 +48,8 @@ export const PRTopbarButton = memo(function PRTopbarButton() {
<TooltipTrigger asChild>
<Button
data-testid="pr-topbar-button"
data-pr-state={pr.state}
data-pr-ready-to-merge={isPRReadyToMerge(pr) ? "true" : "false"}
size="sm"
variant="outline"
className="cursor-pointer gap-1.5 px-2"
apps/web/e2e/helpers/api-client.ts
+5
View File
@@ -23,6 +23,8 @@ export type MockPR = {
url?: string;
body?: string;
draft?: boolean;
mergeable?: boolean;
mergeable_state?: string;
additions?: number;
deletions?: number;
merged_at?: string;
@@ -622,6 +624,9 @@ export class ApiClient {
base_branch: string;
author_login: string;
state?: string;
review_state?: string;
checks_state?: string;
mergeable_state?: string;
additions?: number;
deletions?: number;
}): Promise<void> {
apps/web/e2e/tests/pr/pr-status-badge.spec.ts
+217
View File
@@ -0,0 +1,217 @@
import { test, expect } from "../../fixtures/test-base";
import { KanbanPage } from "../../pages/kanban-page";
import { SessionPage } from "../../pages/session-page";
import type { ApiClient } from "../../helpers/api-client";
async function seedBadgeTest(
apiClient: ApiClient,
workspaceId: string,
agentProfileId: string,
repositoryId: string,
title: string,
) {
const workflow = await apiClient.createWorkflow(workspaceId, `${title} Workflow`);
const inboxStep = await apiClient.createWorkflowStep(workflow.id, "Inbox", 0);
const workingStep = await apiClient.createWorkflowStep(workflow.id, "Working", 1);
const doneStep = await apiClient.createWorkflowStep(workflow.id, "Done", 2);
await apiClient.updateWorkflowStep(workingStep.id, {
prompt: 'e2e:message("done")\n{{task_prompt}}',
events: {
on_enter: [{ type: "auto_start_agent" }],
on_turn_complete: [{ type: "move_to_step", config: { step_id: doneStep.id } }],
},
});
await apiClient.saveUserSettings({
workspace_id: workspaceId,
workflow_filter_id: workflow.id,
enable_preview_on_click: false,
});
await apiClient.mockGitHubReset();
await apiClient.mockGitHubSetUser("test-user");
const task = await apiClient.createTask(workspaceId, title, {
workflow_id: workflow.id,
workflow_step_id: inboxStep.id,
agent_profile_id: agentProfileId,
repository_ids: [repositoryId],
});
return { workflow, inboxStep, workingStep, doneStep, task };
}
test.describe("PR status badge", () => {
/**
* Regression for the "CI pending" bug: GitHub reports all checks passed
* (one skipped, many successful). We used to compute "pending" because
* skipped checks weren't classified explicitly. The badge must now show
* the success colour.
*/
test("renders success when all checks passed with some skipped", async ({
testPage,
apiClient,
seedData,
}) => {
test.setTimeout(120_000);
const { workflow, workingStep, doneStep, task } = await seedBadgeTest(
apiClient,
seedData.workspaceId,
seedData.agentProfileId,
seedData.repositoryId,
"CI Skipped Task",
);
const kanban = new KanbanPage(testPage);
await kanban.goto();
await apiClient.moveTask(task.id, workflow.id, workingStep.id);
// Seed task PR directly with checks_state=success (post-fix behaviour
// when 20 success + 1 skipped checks flow through computeOverallCheckStatus).
// Associating after moveTask is intentional: the task may already reach Done
// before this call, and the mock controller's github.task_pr.updated event
// is what refreshes the badge on the already-rendered kanban card. Don't
// reorder before moveTask without preserving that event flow.
await apiClient.mockGitHubAssociateTaskPR({
task_id: task.id,
owner: "testorg",
repo: "testrepo",
pr_number: 101,
pr_url: "https://github.com/testorg/testrepo/pull/101",
pr_title: "Skipped checks",
head_branch: "feat/skipped",
base_branch: "main",
author_login: "test-user",
state: "open",
checks_state: "success",
});
await expect(kanban.taskCardInColumn("CI Skipped Task", doneStep.id)).toBeVisible({
timeout: 45_000,
});
const icon = testPage.getByTestId(`pr-task-icon-${task.id}`);
await expect(icon).toBeVisible({ timeout: 15_000 });
// No reviews, so ready-to-merge must be false; badge should not be yellow.
await expect(icon).toHaveAttribute("data-pr-ready-to-merge", "false");
await expect(icon).not.toHaveClass(/text-yellow-500/);
// Open the task to verify topbar button mirrors the state.
await kanban.taskCardInColumn("CI Skipped Task", doneStep.id).click();
await expect(testPage).toHaveURL(/\/[st]\//, { timeout: 15_000 });
const session = new SessionPage(testPage);
await session.waitForLoad();
await expect(session.prTopbarButton()).toBeVisible({ timeout: 15_000 });
await expect(session.prTopbarButton()).toHaveAttribute("data-pr-ready-to-merge", "false");
});
/**
* When reviewers approved, CI passes, and GitHub's mergeable_state is clean,
* the badge must show the ready-to-merge state so the user knows the PR
* is ready to merge.
*/
test("renders ready-to-merge when approved + clean + checks pass", async ({
testPage,
apiClient,
seedData,
}) => {
test.setTimeout(120_000);
const { workflow, workingStep, doneStep, task } = await seedBadgeTest(
apiClient,
seedData.workspaceId,
seedData.agentProfileId,
seedData.repositoryId,
"Ready To Merge Task",
);
const kanban = new KanbanPage(testPage);
await kanban.goto();
await apiClient.moveTask(task.id, workflow.id, workingStep.id);
await apiClient.mockGitHubAssociateTaskPR({
task_id: task.id,
owner: "testorg",
repo: "testrepo",
pr_number: 102,
pr_url: "https://github.com/testorg/testrepo/pull/102",
pr_title: "Ready to ship",
head_branch: "feat/ready",
base_branch: "main",
author_login: "test-user",
state: "open",
review_state: "approved",
checks_state: "success",
mergeable_state: "clean",
});
await expect(kanban.taskCardInColumn("Ready To Merge Task", doneStep.id)).toBeVisible({
timeout: 45_000,
});
const icon = testPage.getByTestId(`pr-task-icon-${task.id}`);
await expect(icon).toBeVisible({ timeout: 15_000 });
await expect(icon).toHaveAttribute("data-pr-ready-to-merge", "true");
await kanban.taskCardInColumn("Ready To Merge Task", doneStep.id).click();
await expect(testPage).toHaveURL(/\/[st]\//, { timeout: 15_000 });
const session = new SessionPage(testPage);
await session.waitForLoad();
await expect(session.prTopbarButton()).toBeVisible({ timeout: 15_000 });
await expect(session.prTopbarButton()).toHaveAttribute("data-pr-ready-to-merge", "true");
});
/**
* Guard against false positives: reviewers approved and CI is green, but
* GitHub reports mergeable_state=blocked (e.g., CODEOWNERS not satisfied).
* Badge must stay at the plain approved-success state, not ready-to-merge.
*/
test("does not render ready-to-merge when mergeable_state is blocked", async ({
testPage,
apiClient,
seedData,
}) => {
test.setTimeout(120_000);
const { workflow, workingStep, doneStep, task } = await seedBadgeTest(
apiClient,
seedData.workspaceId,
seedData.agentProfileId,
seedData.repositoryId,
"Blocked Task",
);
const kanban = new KanbanPage(testPage);
await kanban.goto();
await apiClient.moveTask(task.id, workflow.id, workingStep.id);
await apiClient.mockGitHubAssociateTaskPR({
task_id: task.id,
owner: "testorg",
repo: "testrepo",
pr_number: 103,
pr_url: "https://github.com/testorg/testrepo/pull/103",
pr_title: "Blocked by CODEOWNERS",
head_branch: "feat/blocked",
base_branch: "main",
author_login: "test-user",
state: "open",
review_state: "approved",
checks_state: "success",
mergeable_state: "blocked",
});
await expect(kanban.taskCardInColumn("Blocked Task", doneStep.id)).toBeVisible({
timeout: 45_000,
});
const icon = testPage.getByTestId(`pr-task-icon-${task.id}`);
await expect(icon).toBeVisible({ timeout: 15_000 });
await expect(icon).toHaveAttribute("data-pr-ready-to-merge", "false");
// Plain-green approved state, not the ready-to-merge emerald.
await expect(icon).not.toHaveClass(/text-emerald-400/);
});
});
apps/web/lib/types/github.ts
+12
View File
@@ -86,6 +86,17 @@ export type PRFeedback = {
has_issues: boolean;
};
export type MergeableState =
| "clean"
| "blocked"
| "behind"
| "dirty"
| "has_hooks"
| "unstable"
| "draft"
| "unknown"
| "";
export type TaskPR = {
id: string;
task_id: string;
@@ -100,6 +111,7 @@ export type TaskPR = {
state: "open" | "closed" | "merged";
review_state: "approved" | "changes_requested" | "pending" | "";
checks_state: "success" | "failure" | "pending" | "";
mergeable_state: MergeableState;
review_count: number;
pending_review_count: number;
comment_count: number;