diff --git a/apps/backend/internal/github/client.go b/apps/backend/internal/github/client.go index 2a6883ba6a..7b0e042356 100644 --- a/apps/backend/internal/github/client.go +++ b/apps/backend/internal/github/client.go @@ -68,6 +68,9 @@ type Client interface { // ListRepoBranches lists branches for a repository. ListRepoBranches(ctx context.Context, owner, repo string) ([]RepoBranch, error) + // GetRepoMergeMethods reports which merge methods a repository allows. + GetRepoMergeMethods(ctx context.Context, owner, repo string) (RepoMergeMethods, error) + // ListIssues searches for open issues (not PRs) matching the given query. // filter is an optional additional search qualifier (e.g. "repo:owner/name" or "label:bug"). // customQuery, when non-empty, replaces the entire generated query. diff --git a/apps/backend/internal/github/controller.go b/apps/backend/internal/github/controller.go index 9ecc96c01b..e2d550a91a 100644 --- a/apps/backend/internal/github/controller.go +++ b/apps/backend/internal/github/controller.go @@ -66,6 +66,7 @@ func (c *Controller) RegisterHTTPRoutes(router *gin.Engine) { api.GET("/orgs", c.httpListUserOrgs) api.GET("/repos/search", c.httpSearchRepos) api.GET("/repos/:owner/:repo/branches", c.httpListRepoBranches) + api.GET("/repos/:owner/:repo/merge-methods", c.httpGetRepoMergeMethods) api.GET("/user/prs", c.httpSearchUserPRs) api.GET("/user/issues", c.httpSearchUserIssues) @@ -525,6 +526,36 @@ func (c *Controller) httpListRepoBranches(ctx *gin.Context) { ctx.JSON(http.StatusOK, gin.H{"branches": branches}) } +func (c *Controller) httpGetRepoMergeMethods(ctx *gin.Context) { + owner := ctx.Param("owner") + repo := ctx.Param("repo") + methods, err := c.service.GetRepoMergeMethods(ctx.Request.Context(), owner, repo) + if err != nil { + if errors.Is(err, ErrNoClient) { + ctx.JSON(http.StatusServiceUnavailable, gin.H{ + "error": "GitHub is not configured. Install the gh CLI and run 'gh auth login', or add a GITHUB_TOKEN secret.", + "code": "github_not_configured", + }) + return + } + status := http.StatusInternalServerError + var apiErr *GitHubAPIError + if errors.As(err, &apiErr) { + switch apiErr.StatusCode { + case http.StatusNotFound: + status = http.StatusNotFound + case http.StatusUnauthorized: + status = http.StatusUnauthorized + case http.StatusForbidden: + status = http.StatusForbidden + } + } + ctx.JSON(status, gin.H{"error": err.Error()}) + return + } + ctx.JSON(http.StatusOK, methods) +} + // httpSearchUserPRs searches for pull requests. Accepts: // - query: the complete GitHub search query (e.g. "is:pr author:@me state:open") // - filter: additional qualifiers appended to the default `type:pr` diff --git a/apps/backend/internal/github/controller_test.go b/apps/backend/internal/github/controller_test.go index 0afe8b2ad3..205efc891c 100644 --- a/apps/backend/internal/github/controller_test.go +++ b/apps/backend/internal/github/controller_test.go @@ -17,8 +17,9 @@ import ( // stubClient implements Client with no-op defaults; override fields as needed. type stubClient struct { - getPRFunc func(ctx context.Context, owner, repo string, number int) (*PR, error) - mergePRFn func(ctx context.Context, owner, repo string, number int, mergeMethod string) error + getPRFunc func(ctx context.Context, owner, repo string, number int) (*PR, error) + mergePRFn func(ctx context.Context, owner, repo string, number int, mergeMethod string) error + getRepoMergeMethodsFn func() (RepoMergeMethods, error) } func (s *stubClient) IsAuthenticated(context.Context) (bool, error) { return true, nil } @@ -80,6 +81,12 @@ func (s *stubClient) MergePR(ctx context.Context, owner, repo string, number int func (s *stubClient) ListRepoBranches(context.Context, string, string) ([]RepoBranch, error) { return nil, nil } +func (s *stubClient) GetRepoMergeMethods(context.Context, string, string) (RepoMergeMethods, error) { + if s.getRepoMergeMethodsFn != nil { + return s.getRepoMergeMethodsFn() + } + return RepoMergeMethods{Merge: true, Squash: true, Rebase: true}, nil +} func (s *stubClient) ListIssues(context.Context, string, string) ([]*Issue, error) { return nil, nil } @@ -217,13 +224,82 @@ func TestHttpMergePR_InvalidMethod(t *testing.T) { } } -func TestHttpMergePR_EmptyBody_UsesDefaultMethod(t *testing.T) { +func TestHttpMergePR_EmptyBody_ResolvesToAllowedMethod(t *testing.T) { + // Empty merge_method must NOT propagate to GitHub — that would let + // GitHub default to "merge" and 405 on squash-only repos. The service + // should resolve to the first allowed method instead. var gotMethod string sc := &stubClient{ mergePRFn: func(_ context.Context, _, _ string, _ int, mergeMethod string) error { gotMethod = mergeMethod return nil }, + getRepoMergeMethodsFn: func() (RepoMergeMethods, error) { + // Squash-only repo (the case that surfaced this bug). + return RepoMergeMethods{Squash: true}, nil + }, + } + router, _ := setupControllerTest(sc) + + req := httptest.NewRequest(http.MethodPut, "/api/v1/github/prs/acme/widget/42/merge", nil) + w := httptest.NewRecorder() + router.ServeHTTP(w, req) + + if w.Code != http.StatusOK { + t.Fatalf("expected 200, got %d: %s", w.Code, w.Body.String()) + } + if gotMethod != "squash" { + t.Errorf("expected merge_method=squash, got %q", gotMethod) + } +} + +func TestHttpMergePR_ExplicitMethod_Passthrough(t *testing.T) { + // When the user picks a method from the dropdown, the service must + // NOT second-guess it via the repo lookup. + var gotMethod string + var lookupCalls int + sc := &stubClient{ + mergePRFn: func(_ context.Context, _, _ string, _ int, mergeMethod string) error { + gotMethod = mergeMethod + return nil + }, + getRepoMergeMethodsFn: func() (RepoMergeMethods, error) { + lookupCalls++ + return RepoMergeMethods{Merge: true}, nil + }, + } + router, _ := setupControllerTest(sc) + + body := bytes.NewBufferString(`{"merge_method":"rebase"}`) + req := httptest.NewRequest(http.MethodPut, "/api/v1/github/prs/acme/widget/42/merge", body) + req.Header.Set("Content-Type", "application/json") + w := httptest.NewRecorder() + router.ServeHTTP(w, req) + + if w.Code != http.StatusOK { + t.Fatalf("expected 200, got %d: %s", w.Code, w.Body.String()) + } + if gotMethod != "rebase" { + t.Errorf("expected merge_method=rebase, got %q", gotMethod) + } + if lookupCalls != 0 { + t.Errorf("expected no merge-methods lookup for explicit pin, got %d", lookupCalls) + } +} + +func TestHttpMergePR_EmptyBody_LookupFails_FallsBackToGitHubDefault(t *testing.T) { + // If the repo lookup itself errors, we still attempt the merge with an + // empty method and let GitHub surface whatever error it surfaces — better + // than refusing to merge because of an unrelated lookup failure. + var gotMethod string + sc := &stubClient{ + mergePRFn: func(_ context.Context, _, _ string, _ int, mergeMethod string) error { + gotMethod = mergeMethod + return nil + }, + getRepoMergeMethodsFn: func() (RepoMergeMethods, error) { + return RepoMergeMethods{}, fmt.Errorf("rate limited") + }, } router, _ := setupControllerTest(sc) @@ -235,7 +311,84 @@ func TestHttpMergePR_EmptyBody_UsesDefaultMethod(t *testing.T) { t.Fatalf("expected 200, got %d: %s", w.Code, w.Body.String()) } if gotMethod != "" { - t.Errorf("expected empty merge_method, got %q", gotMethod) + t.Errorf("expected empty merge_method (fall back to GitHub default), got %q", gotMethod) + } +} + +func TestHttpGetRepoMergeMethods_OK(t *testing.T) { + sc := &stubClient{ + getRepoMergeMethodsFn: func() (RepoMergeMethods, error) { + return RepoMergeMethods{Squash: true, Rebase: true}, nil + }, + } + router, _ := setupControllerTest(sc) + + req := httptest.NewRequest(http.MethodGet, "/api/v1/github/repos/acme/widget/merge-methods", nil) + w := httptest.NewRecorder() + router.ServeHTTP(w, req) + + if w.Code != http.StatusOK { + t.Fatalf("expected 200, got %d: %s", w.Code, w.Body.String()) + } + var got RepoMergeMethods + if err := json.NewDecoder(w.Body).Decode(&got); err != nil { + t.Fatalf("decode: %v", err) + } + want := RepoMergeMethods{Squash: true, Rebase: true} + if got != want { + t.Errorf("got %+v, want %+v", got, want) + } +} + +func TestHttpGetRepoMergeMethods_NoClient_Returns503(t *testing.T) { + gin.SetMode(gin.TestMode) + log := newControllerTestLogger() + // nil client triggers ErrNoClient via Service.GetRepoMergeMethods. + svc := NewService(nil, "none", nil, nil, nil, log) + ctrl := NewController(svc, log) + router := gin.New() + ctrl.RegisterHTTPRoutes(router) + + req := httptest.NewRequest(http.MethodGet, "/api/v1/github/repos/acme/widget/merge-methods", nil) + w := httptest.NewRecorder() + router.ServeHTTP(w, req) + + if w.Code != http.StatusServiceUnavailable { + t.Fatalf("expected 503, got %d: %s", w.Code, w.Body.String()) + } +} + +func TestHttpGetRepoMergeMethods_NotFound_Returns404(t *testing.T) { + sc := &stubClient{ + getRepoMergeMethodsFn: func() (RepoMergeMethods, error) { + return RepoMergeMethods{}, &GitHubAPIError{StatusCode: http.StatusNotFound, Endpoint: "/repos/acme/widget"} + }, + } + router, _ := setupControllerTest(sc) + + req := httptest.NewRequest(http.MethodGet, "/api/v1/github/repos/acme/widget/merge-methods", nil) + w := httptest.NewRecorder() + router.ServeHTTP(w, req) + + if w.Code != http.StatusNotFound { + t.Fatalf("expected 404, got %d: %s", w.Code, w.Body.String()) + } +} + +func TestHttpGetRepoMergeMethods_OtherError_Returns500(t *testing.T) { + sc := &stubClient{ + getRepoMergeMethodsFn: func() (RepoMergeMethods, error) { + return RepoMergeMethods{}, fmt.Errorf("unexpected upstream failure") + }, + } + router, _ := setupControllerTest(sc) + + req := httptest.NewRequest(http.MethodGet, "/api/v1/github/repos/acme/widget/merge-methods", nil) + w := httptest.NewRecorder() + router.ServeHTTP(w, req) + + if w.Code != http.StatusInternalServerError { + t.Fatalf("expected 500, got %d: %s", w.Code, w.Body.String()) } } diff --git a/apps/backend/internal/github/gh_client.go b/apps/backend/internal/github/gh_client.go index d889eed302..3c8f22a118 100644 --- a/apps/backend/internal/github/gh_client.go +++ b/apps/backend/internal/github/gh_client.go @@ -646,6 +646,32 @@ func ghMergeStatusCode(err error) (int, bool) { return 0, false } +func (c *GHClient) GetRepoMergeMethods(ctx context.Context, owner, repo string) (RepoMergeMethods, error) { + out, err := c.run(ctx, "api", fmt.Sprintf("repos/%s/%s", owner, repo)) + if err != nil { + return RepoMergeMethods{}, fmt.Errorf("get repo merge methods: %w", err) + } + var raw struct { + AllowMergeCommit *bool `json:"allow_merge_commit"` + AllowSquashMerge *bool `json:"allow_squash_merge"` + AllowRebaseMerge *bool `json:"allow_rebase_merge"` + } + if err := json.Unmarshal([]byte(out), &raw); err != nil { + return RepoMergeMethods{}, fmt.Errorf("parse repo: %w", err) + } + // Conservative read: missing field → false. A permission-gated response + // that omits allow_* would otherwise let us pick a disallowed method + // (e.g. "merge" on a rebase-only repo), reproducing the 405 this fix is + // designed to prevent. Callers that fall back to GitHub's default on an + // empty pick get a meaningful error instead of a wrong-method 405. + allowed := func(p *bool) bool { return p != nil && *p } + return RepoMergeMethods{ + Merge: allowed(raw.AllowMergeCommit), + Squash: allowed(raw.AllowSquashMerge), + Rebase: allowed(raw.AllowRebaseMerge), + }, nil +} + func (c *GHClient) ListRepoBranches(ctx context.Context, owner, repo string) ([]RepoBranch, error) { out, err := c.run(ctx, "api", fmt.Sprintf("repos/%s/%s/branches", owner, repo), diff --git a/apps/backend/internal/github/mock_client.go b/apps/backend/internal/github/mock_client.go index 986256aed3..882051d898 100644 --- a/apps/backend/internal/github/mock_client.go +++ b/apps/backend/internal/github/mock_client.go @@ -73,6 +73,7 @@ type MockClient struct { commits map[prKey][]PRCommitInfo submittedReviews []submittedReview mergedPRs []mergedPR + mergeMethods map[repoKey]RepoMergeMethods } // NewMockClient creates a new MockClient with default values. @@ -89,6 +90,7 @@ func NewMockClient() *MockClient { checks: make(map[checkKey][]CheckRun), files: make(map[prKey][]PRFile), commits: make(map[prKey][]PRCommitInfo), + mergeMethods: make(map[repoKey]RepoMergeMethods), } } @@ -275,6 +277,25 @@ func (m *MockClient) SubmitReview(_ context.Context, owner, repo string, number return nil } +func (m *MockClient) GetRepoMergeMethods(_ context.Context, owner, repo string) (RepoMergeMethods, error) { + m.mu.RLock() + defer m.mu.RUnlock() + if methods, ok := m.mergeMethods[repoKey{owner, repo}]; ok { + return methods, nil + } + // Default to all three allowed so existing e2e fixtures don't have to + // seed merge settings just to exercise the merge button. + return RepoMergeMethods{Merge: true, Squash: true, Rebase: true}, nil +} + +// SetRepoMergeMethods overrides the allowed merge methods for a repo. +// Used by e2e fixtures to exercise the squash-only / rebase-only paths. +func (m *MockClient) SetRepoMergeMethods(owner, repo string, methods RepoMergeMethods) { + m.mu.Lock() + defer m.mu.Unlock() + m.mergeMethods[repoKey{owner, repo}] = methods +} + func (m *MockClient) MergePR(_ context.Context, owner, repo string, number int, mergeMethod string) error { m.mu.Lock() defer m.mu.Unlock() @@ -429,6 +450,7 @@ func (m *MockClient) Reset() { m.commits = make(map[prKey][]PRCommitInfo) m.submittedReviews = nil m.mergedPRs = nil + m.mergeMethods = make(map[repoKey]RepoMergeMethods) } // SubmittedReviews returns all recorded SubmitReview calls. diff --git a/apps/backend/internal/github/models.go b/apps/backend/internal/github/models.go index c90f2c40c5..6a31a91df5 100644 --- a/apps/backend/internal/github/models.go +++ b/apps/backend/internal/github/models.go @@ -296,6 +296,14 @@ type RepoBranch struct { Name string `json:"name"` } +// RepoMergeMethods reports which merge methods a repository allows. Mirrors +// the allow_*_merge booleans from GET /repos/{owner}/{repo}. +type RepoMergeMethods struct { + Merge bool `json:"merge"` + Squash bool `json:"squash"` + Rebase bool `json:"rebase"` +} + // GitHubStatus represents GitHub connection status. type GitHubStatus struct { Authenticated bool `json:"authenticated"` diff --git a/apps/backend/internal/github/noop_client.go b/apps/backend/internal/github/noop_client.go index e15d87c44d..793a77f485 100644 --- a/apps/backend/internal/github/noop_client.go +++ b/apps/backend/internal/github/noop_client.go @@ -77,6 +77,10 @@ func (c *NoopClient) ListRepoBranches(context.Context, string, string) ([]RepoBr return nil, ErrNoClient } +func (c *NoopClient) GetRepoMergeMethods(context.Context, string, string) (RepoMergeMethods, error) { + return RepoMergeMethods{}, ErrNoClient +} + func (c *NoopClient) SubmitReview(context.Context, string, string, int, string, string) error { return ErrNoClient } diff --git a/apps/backend/internal/github/pat_client.go b/apps/backend/internal/github/pat_client.go index de2f692579..68925261bd 100644 --- a/apps/backend/internal/github/pat_client.go +++ b/apps/backend/internal/github/pat_client.go @@ -440,6 +440,27 @@ func (c *PATClient) ListRepoBranches(ctx context.Context, owner, repo string) ([ return branches, nil } +func (c *PATClient) GetRepoMergeMethods(ctx context.Context, owner, repo string) (RepoMergeMethods, error) { + var raw struct { + AllowMergeCommit *bool `json:"allow_merge_commit"` + AllowSquashMerge *bool `json:"allow_squash_merge"` + AllowRebaseMerge *bool `json:"allow_rebase_merge"` + } + if err := c.get(ctx, fmt.Sprintf("/repos/%s/%s", owner, repo), &raw); err != nil { + return RepoMergeMethods{}, fmt.Errorf("get repo merge methods: %w", err) + } + // Conservative read: missing field → false. A permission-gated response + // that omits allow_* would otherwise let us pick a disallowed method + // (e.g. "merge" on a rebase-only repo), reproducing the 405 this fix is + // designed to prevent. + allowed := func(p *bool) bool { return p != nil && *p } + return RepoMergeMethods{ + Merge: allowed(raw.AllowMergeCommit), + Squash: allowed(raw.AllowSquashMerge), + Rebase: allowed(raw.AllowRebaseMerge), + }, nil +} + func (c *PATClient) SubmitReview(ctx context.Context, owner, repo string, number int, event, body string) error { endpoint := fmt.Sprintf("/repos/%s/%s/pulls/%d/reviews", owner, repo, number) payload := map[string]string{"event": event} diff --git a/apps/backend/internal/github/service.go b/apps/backend/internal/github/service.go index 5954b957b1..b6debd6507 100644 --- a/apps/backend/internal/github/service.go +++ b/apps/backend/internal/github/service.go @@ -108,6 +108,7 @@ type Service struct { taskEventSubs []bus.Subscription searchCache *ttlCache prStatusCache *ttlCache + mergeMethodsCache *ttlCache protectionCache *branchProtectionCache rateTracker *RateTracker @@ -129,6 +130,7 @@ func NewService(client Client, authMethod string, secrets SecretProvider, store logger: log, searchCache: newTTLCache(), prStatusCache: newTTLCache(), + mergeMethodsCache: newMergeMethodsCache(), protectionCache: newBranchProtectionCache(), rateTracker: NewRateTracker(eventBus, log), cleanupFailureCounts: make(map[string]int), @@ -432,16 +434,70 @@ func (s *Service) SubmitReview(ctx context.Context, owner, repo string, number i } // MergePR merges a pull request. mergeMethod is one of "merge", "squash", -// "rebase"; an empty string lets GitHub apply the repo's default. The caller -// is expected to refresh PR feedback after a successful merge — the background -// poller will catch the merged state on its next pass. +// "rebase"; an empty string asks the service to pick the first method the +// repo allows. The caller is expected to refresh PR feedback after a +// successful merge — the background poller will catch the merged state on +// its next pass. func (s *Service) MergePR(ctx context.Context, owner, repo string, number int, mergeMethod string) error { if s.client == nil { return ErrNoClient } + if mergeMethod == "" { + // Resolve to an allowed method up-front so we don't rely on GitHub's + // "default to merge" behavior, which 405s on repos that disallow + // merge commits (squash-only / rebase-only). Best-effort: if the + // lookup fails (or — degenerate config — reports no method allowed), + // fall back to GitHub's default and surface its error rather than + // blocking the merge attempt. + if methods, err := s.GetRepoMergeMethods(ctx, owner, repo); err == nil { + if pick := pickDefaultMergeMethod(methods); pick != "" { + mergeMethod = pick + } + } + } return s.client.MergePR(ctx, owner, repo, number, mergeMethod) } +// GetRepoMergeMethods returns the merge methods a repo allows, cached for +// a few minutes since repo settings rarely change. +func (s *Service) GetRepoMergeMethods(ctx context.Context, owner, repo string) (RepoMergeMethods, error) { + if s.client == nil { + return RepoMergeMethods{}, ErrNoClient + } + key := owner + "/" + repo + v, err := s.mergeMethodsCache.doOrFetch(key, func() (any, error) { + return s.client.GetRepoMergeMethods(ctx, owner, repo) + }) + if err != nil { + return RepoMergeMethods{}, err + } + return v.(RepoMergeMethods), nil +} + +// Merge method identifiers accepted by GitHub's pulls/{number}/merge endpoint +// and used throughout the merge resolution paths. +const ( + mergeMethodMerge = "merge" + mergeMethodSquash = "squash" + mergeMethodRebase = "rebase" +) + +// pickDefaultMergeMethod picks the merge method to use when the caller +// didn't pin one. Prefers squash (matches the convention most repos in +// this codebase follow) and falls back to merge, then rebase. +func pickDefaultMergeMethod(m RepoMergeMethods) string { + switch { + case m.Squash: + return mergeMethodSquash + case m.Merge: + return mergeMethodMerge + case m.Rebase: + return mergeMethodRebase + default: + return "" + } +} + // --- PR Watch operations --- // CreatePRWatch creates a new PR watch for a (session, repository) pair. diff --git a/apps/backend/internal/github/service_merge_methods_test.go b/apps/backend/internal/github/service_merge_methods_test.go new file mode 100644 index 0000000000..0a2e99fda7 --- /dev/null +++ b/apps/backend/internal/github/service_merge_methods_test.go @@ -0,0 +1,56 @@ +package github + +import ( + "context" + "testing" + + "github.com/kandev/kandev/internal/common/logger" +) + +func TestPickDefaultMergeMethod(t *testing.T) { + tests := []struct { + name string + methods RepoMergeMethods + want string + }{ + {"squash preferred over merge", RepoMergeMethods{Merge: true, Squash: true, Rebase: true}, "squash"}, + {"squash-only repo", RepoMergeMethods{Squash: true}, "squash"}, + {"merge when squash disabled", RepoMergeMethods{Merge: true, Rebase: true}, "merge"}, + {"rebase-only repo", RepoMergeMethods{Rebase: true}, "rebase"}, + {"none allowed → empty string", RepoMergeMethods{}, ""}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := pickDefaultMergeMethod(tt.methods); got != tt.want { + t.Errorf("got %q, want %q", got, tt.want) + } + }) + } +} + +func TestService_GetRepoMergeMethods_Cached(t *testing.T) { + mc := NewMockClient() + mc.SetRepoMergeMethods("acme", "widget", RepoMergeMethods{Squash: true}) + log, _ := logger.NewLogger(logger.LoggingConfig{Level: "error", Format: "json"}) + svc := NewService(mc, AuthMethodPAT, nil, nil, nil, log) + + // Two consecutive calls — cache must serve the second one without re-hitting + // the client. We can't directly count calls into MockClient, but seeding a + // different value after the first call lets us prove the cache won. + first, err := svc.GetRepoMergeMethods(context.Background(), "acme", "widget") + if err != nil { + t.Fatalf("first: %v", err) + } + if !first.Squash || first.Merge || first.Rebase { + t.Errorf("first: got %+v, want squash-only", first) + } + + mc.SetRepoMergeMethods("acme", "widget", RepoMergeMethods{Merge: true, Rebase: true}) + second, err := svc.GetRepoMergeMethods(context.Background(), "acme", "widget") + if err != nil { + t.Fatalf("second: %v", err) + } + if second != first { + t.Errorf("expected cached result %+v, got %+v", first, second) + } +} diff --git a/apps/backend/internal/github/ttl_cache.go b/apps/backend/internal/github/ttl_cache.go index 41f02cdc38..20822e7f94 100644 --- a/apps/backend/internal/github/ttl_cache.go +++ b/apps/backend/internal/github/ttl_cache.go @@ -42,6 +42,15 @@ func newTTLCache() *ttlCache { } } +// newMergeMethodsCache uses a longer TTL than the default search/status +// caches: repo merge settings rarely change, so a 5-minute window cuts the +// per-PR-view API calls without making "I just toggled squash" feel stuck. +func newMergeMethodsCache() *ttlCache { + c := newTTLCache() + c.ttl = 5 * time.Minute + return c +} + func (c *ttlCache) get(key string) (any, bool) { c.mu.Lock() defer c.mu.Unlock() diff --git a/apps/web/components/github/pr-merge-button.tsx b/apps/web/components/github/pr-merge-button.tsx index b9a3661097..92eb2c04b7 100644 --- a/apps/web/components/github/pr-merge-button.tsx +++ b/apps/web/components/github/pr-merge-button.tsx @@ -1,18 +1,27 @@ "use client"; import { useEffect, useState } from "react"; -import { IconGitMerge } from "@tabler/icons-react"; +import { IconChevronDown, IconGitMerge } from "@tabler/icons-react"; import { Button } from "@kandev/ui/button"; +import { + DropdownMenu, + DropdownMenuContent, + DropdownMenuItem, + DropdownMenuTrigger, +} from "@kandev/ui/dropdown-menu"; import { useToast } from "@/components/toast-provider"; +import { useRepoMergeMethods } from "@/hooks/domains/github/use-repo-merge-methods"; import { mergePR } from "@/lib/api/domains/github-api"; -import type { TaskPR } from "@/lib/types/github"; +import type { MergeMethod, TaskPR } from "@/lib/types/github"; import { isPRReadyToMerge } from "./pr-task-icon"; // Renders nothing unless the PR is fully green (CI success + mergeable + -// approval or no-review-needed). Shared by the PR detail panel header and -// the topbar hover popover so a "ready" PR can be merged from either -// surface. `compact` switches to the smaller pill variant used inside the -// dense popover. +// approval or no-review-needed). When `useRepoMergeMethods` hasn't yet +// returned the repo's allowed methods (loading, failure, or expired cache) +// the button still renders — clicks fall through to the backend resolver. +// Shared by the PR detail panel header and the topbar hover popover so a +// "ready" PR can be merged from either surface. `compact` switches to the +// smaller pill variant used inside the dense popover. export function PRMergeButton({ taskPR, onMerged, @@ -28,6 +37,7 @@ export function PRMergeButton({ // state="merged" — otherwise the button briefly re-enables during the async // refresh window and a double-click would hit the GitHub API again. const [merged, setMerged] = useState(false); + const methods = useRepoMergeMethods(taskPR.owner, taskPR.repo); // If the same component instance ever renders a different PR (e.g. the user // switches the active task while the panel/popover stays mounted), the @@ -39,12 +49,19 @@ export function PRMergeButton({ }, [taskPR.id]); if (merged || !isPRReadyToMerge(taskPR)) return null; + // `methods` may be null on first render, on lookup failure, or after the + // 5-minute cache window. We still render the button — clicking with no + // method routes through the backend's GetRepoMergeMethods resolver, so + // the merge succeeds either way. The trade-off is a brief label flicker + // from "Merge PR" → "Squash and merge" on first load; locking the user + // out of merging on a transient fetch failure is the worse alternative. + const allowed = allowedMethods(methods); + const primary = pickPrimaryMethod(allowed); - const handleMerge = async (e: React.MouseEvent) => { - e.stopPropagation(); + const runMerge = async (method?: MergeMethod) => { setMerging(true); try { - await mergePR(taskPR.owner, taskPR.repo, taskPR.pr_number); + await mergePR(taskPR.owner, taskPR.repo, taskPR.pr_number, method); setMerged(true); toast({ description: "PR merged", variant: "success" }); onMerged?.(); @@ -59,31 +76,125 @@ export function PRMergeButton({ } }; - if (compact) { - return ( - - ); - } + const handlePrimary = (e: React.MouseEvent) => { + e.stopPropagation(); + void runMerge(primary); + }; return ( + m !== primary)} + onPickMethod={(m) => void runMerge(m)} + /> + ); +} + +function MergeButtonShell({ + compact, + label, + disabled, + onPrimaryClick, + extraMethods, + onPickMethod, +}: { + compact: boolean; + label: string; + disabled: boolean; + onPrimaryClick: (e: React.MouseEvent) => void; + extraMethods: MergeMethod[]; + onPickMethod: (m: MergeMethod) => void; +}) { + const showDropdown = extraMethods.length > 0; + const primaryBtn = compact ? ( + + ) : ( ); + + if (!showDropdown) { + return compact ? primaryBtn : {primaryBtn}; + } + + return ( + + {primaryBtn} + + + + + + {extraMethods.map((m) => ( + onPickMethod(m)}> + {mergeLabel(m)} + + ))} + + + + ); +} + +function mergeLabel(method?: MergeMethod): string { + switch (method) { + case "squash": + return "Squash and merge"; + case "rebase": + return "Rebase and merge"; + case "merge": + return "Create a merge commit"; + default: + return "Merge PR"; + } +} + +// Returns the methods the repo allows, in the order we prefer to surface +// them (squash → merge → rebase, matching GitHub's own button defaults). +// When the lookup hasn't resolved (or failed), returns an empty array so +// the button still renders without a dropdown and the backend's resolver +// picks the method at merge time. +function allowedMethods( + methods: { merge: boolean; squash: boolean; rebase: boolean } | null, +): MergeMethod[] { + if (!methods) return []; + const order: MergeMethod[] = ["squash", "merge", "rebase"]; + return order.filter((m) => methods[m]); +} + +function pickPrimaryMethod(allowed: MergeMethod[]): MergeMethod | undefined { + return allowed[0]; } diff --git a/apps/web/hooks/domains/github/use-repo-merge-methods.test.ts b/apps/web/hooks/domains/github/use-repo-merge-methods.test.ts new file mode 100644 index 0000000000..9d8fc670cf --- /dev/null +++ b/apps/web/hooks/domains/github/use-repo-merge-methods.test.ts @@ -0,0 +1,134 @@ +import { afterEach, describe, expect, it, vi } from "vitest"; +import { act, cleanup, renderHook, waitFor } from "@testing-library/react"; + +const getRepoMergeMethodsMock = vi.fn(); + +vi.mock("@/lib/api/domains/github-api", () => ({ + getRepoMergeMethods: (...args: unknown[]) => getRepoMergeMethodsMock(...args), +})); + +// Module-level cache survives across tests in the same module — give each +// test a unique owner so the cache state from one doesn't leak into the next. +let repoCounter = 0; +function uniqueRepo() { + repoCounter += 1; + return { owner: `owner-${repoCounter}`, repo: `repo-${repoCounter}` }; +} + +// Import after mocks so the hook picks up the mocked module. +import { useRepoMergeMethods } from "./use-repo-merge-methods"; + +afterEach(() => { + cleanup(); + getRepoMergeMethodsMock.mockReset(); + vi.useRealTimers(); +}); + +describe("useRepoMergeMethods", () => { + it("returns null when owner or repo are missing", () => { + const { result } = renderHook(() => useRepoMergeMethods(null, null)); + expect(result.current).toBeNull(); + expect(getRepoMergeMethodsMock).not.toHaveBeenCalled(); + }); + + it("returns null while in flight, then the resolved value after settling", async () => { + const { owner, repo } = uniqueRepo(); + const methods = { merge: false, squash: true, rebase: false }; + getRepoMergeMethodsMock.mockResolvedValueOnce(methods); + + const { result } = renderHook(() => useRepoMergeMethods(owner, repo)); + expect(result.current).toBeNull(); + + await waitFor(() => expect(result.current).toEqual(methods)); + expect(getRepoMergeMethodsMock).toHaveBeenCalledTimes(1); + expect(getRepoMergeMethodsMock).toHaveBeenCalledWith(owner, repo); + }); + + it("returns the cached value on subsequent mounts without refetching", async () => { + const { owner, repo } = uniqueRepo(); + const methods = { merge: true, squash: false, rebase: false }; + getRepoMergeMethodsMock.mockResolvedValueOnce(methods); + + const first = renderHook(() => useRepoMergeMethods(owner, repo)); + await waitFor(() => expect(first.result.current).toEqual(methods)); + first.unmount(); + + const second = renderHook(() => useRepoMergeMethods(owner, repo)); + // Cache hit: synchronously returns the stored value on first render. + expect(second.result.current).toEqual(methods); + expect(getRepoMergeMethodsMock).toHaveBeenCalledTimes(1); + }); + + it("coalesces concurrent fetches for the same repo into a single request", async () => { + const { owner, repo } = uniqueRepo(); + const methods = { merge: true, squash: true, rebase: true }; + getRepoMergeMethodsMock.mockResolvedValueOnce(methods); + + const first = renderHook(() => useRepoMergeMethods(owner, repo)); + const second = renderHook(() => useRepoMergeMethods(owner, repo)); + + await waitFor(() => { + expect(first.result.current).toEqual(methods); + expect(second.result.current).toEqual(methods); + }); + // Only one upstream call despite two consumers mounting back-to-back. + expect(getRepoMergeMethodsMock).toHaveBeenCalledTimes(1); + }); + + it("returns null on fetch failure without throwing or poisoning the cache", async () => { + const { owner, repo } = uniqueRepo(); + getRepoMergeMethodsMock + .mockRejectedValueOnce(new Error("network down")) + .mockResolvedValueOnce({ merge: false, squash: true, rebase: false }); + + // Catch unhandled rejections so a regression here (rejection escaping + // the hook) fails this test instead of leaking into the test runner. + const unhandled: unknown[] = []; + const onRejection = (event: PromiseRejectionEvent) => { + unhandled.push(event.reason); + event.preventDefault(); + }; + window.addEventListener("unhandledrejection", onRejection); + + const first = renderHook(() => useRepoMergeMethods(owner, repo)); + await waitFor(() => expect(getRepoMergeMethodsMock).toHaveBeenCalledTimes(1)); + expect(first.result.current).toBeNull(); + first.unmount(); + + // Failed fetch must NOT have cached null — the next mount retries. + const second = renderHook(() => useRepoMergeMethods(owner, repo)); + await waitFor(() => expect(getRepoMergeMethodsMock).toHaveBeenCalledTimes(2)); + await waitFor(() => + expect(second.result.current).toEqual({ merge: false, squash: true, rebase: false }), + ); + + window.removeEventListener("unhandledrejection", onRejection); + expect(unhandled).toEqual([]); + }); + + it("refetches after the cache entry expires", async () => { + const { owner, repo } = uniqueRepo(); + const first = { merge: false, squash: true, rebase: false }; + const second = { merge: true, squash: false, rebase: true }; + getRepoMergeMethodsMock.mockResolvedValueOnce(first).mockResolvedValueOnce(second); + + vi.useFakeTimers({ shouldAdvanceTime: true }); + const start = new Date("2026-01-01T00:00:00Z"); + vi.setSystemTime(start); + + const firstMount = renderHook(() => useRepoMergeMethods(owner, repo)); + await waitFor(() => expect(firstMount.result.current).toEqual(first)); + firstMount.unmount(); + + // Move past the 5-minute TTL. + act(() => { + vi.setSystemTime(new Date(start.getTime() + 6 * 60 * 1000)); + }); + + const secondMount = renderHook(() => useRepoMergeMethods(owner, repo)); + // Expired entry should be evicted on read → null while refetch is in flight. + expect(secondMount.result.current).toBeNull(); + await waitFor(() => expect(secondMount.result.current).toEqual(second)); + expect(getRepoMergeMethodsMock).toHaveBeenCalledTimes(2); + }); +}); diff --git a/apps/web/hooks/domains/github/use-repo-merge-methods.ts b/apps/web/hooks/domains/github/use-repo-merge-methods.ts new file mode 100644 index 0000000000..8a43425cf0 --- /dev/null +++ b/apps/web/hooks/domains/github/use-repo-merge-methods.ts @@ -0,0 +1,82 @@ +"use client"; + +import { useEffect, useReducer } from "react"; +import { getRepoMergeMethods } from "@/lib/api/domains/github-api"; +import type { RepoMergeMethods } from "@/lib/types/github"; + +// Module-level cache: merge settings are repo-scoped and rarely change, and +// the backend already TTL-caches them. Coalescing concurrent fetches here +// keeps multiple merge buttons on the same page from triggering duplicate +// round-trips before the backend cache warms. Match the backend's 5-minute +// window so an admin flipping a merge strategy mid-session is reflected +// without a full page reload. +const CACHE_TTL_MS = 5 * 60 * 1000; + +type CachedEntry = { value: RepoMergeMethods; expiresAt: number }; +const completed = new Map(); +// The pending promise is only used to coalesce concurrent fetches — consumers +// re-read from `completed` after it settles, so the resolved value is irrelevant. +// Typing it as `void` lets the chain swallow rejections cleanly (no rethrow, +// no unhandled-rejection warnings in the browser console). +const pending = new Map>(); + +function repoKey(owner: string, repo: string) { + return `${owner}/${repo}`; +} + +function readFresh(key: string): RepoMergeMethods | null { + const entry = completed.get(key); + if (!entry) return null; + if (entry.expiresAt <= Date.now()) { + completed.delete(key); + return null; + } + return entry.value; +} + +/** + * Fetches the merge methods a repository allows so callers can populate the + * merge-method dropdown without sending a request to GitHub that 405s on + * squash-only / rebase-only repos. + * + * Returns `null` while the fetch is in flight, on failure, or when + * owner/repo are missing. Callers should treat `null` as "use the backend + * resolver" — never as "hide the merge UI", because a transient lookup + * failure would otherwise lock the user out of merging. + */ +export function useRepoMergeMethods( + owner: string | null, + repo: string | null, +): RepoMergeMethods | null { + const [, rerender] = useReducer((c: number) => c + 1, 0); + + useEffect(() => { + if (!owner || !repo) return; + const key = repoKey(owner, repo); + if (readFresh(key)) return; + const existing = pending.get(key); + if (existing) { + void existing.then(rerender); + return; + } + const promise = getRepoMergeMethods(owner, repo) + .then((result) => { + completed.set(key, { value: result, expiresAt: Date.now() + CACHE_TTL_MS }); + pending.delete(key); + rerender(); + }) + .catch(() => { + // Don't poison the cache on transient failures — the next mount or + // render will retry. Swallow the rejection here so the originating + // consumer's promise doesn't surface as an unhandled rejection in + // the browser console; co-waiting consumers re-render and read + // null, which behaves identically to a fresh "not-loaded" state. + pending.delete(key); + rerender(); + }); + pending.set(key, promise); + }, [owner, repo]); + + if (!owner || !repo) return null; + return readFresh(repoKey(owner, repo)); +} diff --git a/apps/web/lib/api/domains/github-api.ts b/apps/web/lib/api/domains/github-api.ts index 797a80dab1..989d4501e3 100644 --- a/apps/web/lib/api/domains/github-api.ts +++ b/apps/web/lib/api/domains/github-api.ts @@ -25,6 +25,8 @@ import type { GitHubActionPresets, UpdateGitHubActionPresetsRequest, CleanupTasksResponse, + MergeMethod, + RepoMergeMethods, } from "@/lib/types/github"; // Status @@ -135,12 +137,14 @@ export async function submitPRReview( ); } -// Merge a pull request. Omit mergeMethod to use the repo's default. +// Merge a pull request. Omit mergeMethod to let the backend pick the first +// method the repo allows (avoids GitHub's "default to merge commit" 405 on +// squash-only / rebase-only repos). export async function mergePR( owner: string, repo: string, number: number, - mergeMethod?: "merge" | "squash" | "rebase", + mergeMethod?: MergeMethod, ) { return fetchJson<{ merged: boolean }>(`/api/v1/github/prs/${owner}/${repo}/${number}/merge`, { init: { @@ -150,6 +154,20 @@ export async function mergePR( }); } +// Fetch the merge methods a repository allows (allow_merge_commit / +// allow_squash_merge / allow_rebase_merge). Used by the merge button to +// hide disallowed options and avoid 405s. +export async function getRepoMergeMethods( + owner: string, + repo: string, + options?: ApiRequestOptions, +) { + return fetchJson( + `/api/v1/github/repos/${encodeURIComponent(owner)}/${encodeURIComponent(repo)}/merge-methods`, + options, + ); +} + // PR watches export async function listPRWatches(options?: ApiRequestOptions) { return fetchJson("/api/v1/github/watches/pr", options); diff --git a/apps/web/lib/types/github.ts b/apps/web/lib/types/github.ts index f37387c9ab..00fe8d6fe5 100644 --- a/apps/web/lib/types/github.ts +++ b/apps/web/lib/types/github.ts @@ -123,6 +123,14 @@ export type GitHubPRStatus = { checks_passing: number; }; +export type MergeMethod = "merge" | "squash" | "rebase"; + +export type RepoMergeMethods = { + merge: boolean; + squash: boolean; + rebase: boolean; +}; + export type MergeableState = | "clean" | "blocked"