João Salavisa
b8a09bc5ea
* fix(github): pick allowed merge method so squash-only repos stop 405ing The merge button sent an empty merge_method to GitHub, which defaults to "merge" regardless of repo settings — squash-only / rebase-only repos returned 405 "Merge commits are not allowed on this repository". The backend now resolves an empty method to the first allowed one via a new GetRepoMergeMethods endpoint (5-min cache), and the frontend renders a GitHub-style split button so users can pick among the allowed methods. * fix(github): address PR #999 review feedback - pr-merge-button: extract repo-methods fetch to useRepoMergeMethods hook (Claude bot); hide button while methods are loading so the primary label doesn't flicker (Claude bot); drop e.preventDefault on dropdown items so Radix closes the menu on selection (Greptile P1). - gh_client / pat_client: treat missing allow_* fields as false instead of true so a permission-gated response can't pick a disallowed merge method (cubic P2). - service.MergePR: only overwrite mergeMethod when pickDefaultMergeMethod returns non-empty, so a degenerate all-false config falls through to GitHub's default with the real error (Claude bot). * fix(github): don't hide merge button on lookup failure Round-2 PR feedback (Claude, Greptile, CodeRabbit, cubic): the `if (!methods) return null` guard added in the previous round to avoid label flicker turned a transient repo-methods lookup failure into a permanent UI lockout — the button would stay hidden until unmount/remount. - pr-merge-button: revert the null-guard; treat null as "use backend resolver", render the plain primary button without a dropdown. - use-repo-merge-methods: add a 5-min frontend TTL to match the backend cache so admin merge-strategy flips are visible without a page reload. Catch block now triggers a rerender, and co-waiting consumers attach both fulfilled/rejected handlers to avoid silent rejection swallowing. * test(github): cover useRepoMergeMethods + silence rejection warning Round-3 PR feedback (Claude): - Swallow rejections in the originating fetch chain instead of rethrowing, so the first consumer's promise never escapes the hook as an unhandled rejection. Coalesced consumers already re-render to null, which is the same state a fresh "not loaded" call would show. - Add use-repo-merge-methods.test.ts covering: null-on-missing-params, loading then resolution, cache hit on remount, concurrent-fetch coalescing, retry-on-failure (no cache poisoning, no unhandled rejection), and TTL-expiry refetch via fake timers. * test(github): cover httpGetRepoMergeMethods error branches Round-4 PR feedback (Claude): the new endpoint had only happy-path coverage. Add tests matching httpMergePR's existing shape: - nil client → 503 (ErrNoClient) - GitHubAPIError{404} → 404 - generic upstream error → 500 --------- Co-authored-by: Kandev Agent <agent@kandev.dev>
57 lines
1.8 KiB
Go
57 lines
1.8 KiB
Go
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)
|
|
}
|
|
}
|