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>
139 lines
3.5 KiB
Go
139 lines
3.5 KiB
Go
package github
|
|
|
|
import (
|
|
"fmt"
|
|
"sync"
|
|
"time"
|
|
|
|
"golang.org/x/sync/singleflight"
|
|
)
|
|
|
|
// Short-TTL cache around GitHub responses. The gh CLI subprocess path plus
|
|
// multi-step status builds make round trips expensive; caching within a brief
|
|
// window keeps preset switching, pagination, and list re-renders snappy.
|
|
const (
|
|
defaultCacheTTL = 30 * time.Second
|
|
defaultCacheMaxSize = 200
|
|
)
|
|
|
|
type ttlEntry struct {
|
|
value any
|
|
expiresAt time.Time
|
|
}
|
|
|
|
// ttlCache is a tiny TTL map guarded by singleflight to coalesce concurrent
|
|
// misses for the same key. When size exceeds the cap, entries with the
|
|
// earliest expiry are dropped — good enough for a sub-minute window.
|
|
type ttlCache struct {
|
|
mu sync.Mutex
|
|
entries map[string]ttlEntry
|
|
sf singleflight.Group
|
|
ttl time.Duration
|
|
maxSize int
|
|
now func() time.Time
|
|
}
|
|
|
|
func newTTLCache() *ttlCache {
|
|
return &ttlCache{
|
|
entries: make(map[string]ttlEntry),
|
|
ttl: defaultCacheTTL,
|
|
maxSize: defaultCacheMaxSize,
|
|
now: time.Now,
|
|
}
|
|
}
|
|
|
|
// 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()
|
|
entry, ok := c.entries[key]
|
|
if !ok {
|
|
return nil, false
|
|
}
|
|
if c.now().After(entry.expiresAt) {
|
|
delete(c.entries, key)
|
|
return nil, false
|
|
}
|
|
return entry.value, true
|
|
}
|
|
|
|
func (c *ttlCache) set(key string, value any) {
|
|
c.mu.Lock()
|
|
defer c.mu.Unlock()
|
|
if len(c.entries) >= c.maxSize {
|
|
c.evictLocked()
|
|
}
|
|
c.entries[key] = ttlEntry{value: value, expiresAt: c.now().Add(c.ttl)}
|
|
}
|
|
|
|
// evictLocked first drops expired entries; if still over the cap, drops the
|
|
// entries with the earliest expiry until back under the limit. Caller must
|
|
// hold c.mu.
|
|
func (c *ttlCache) evictLocked() {
|
|
now := c.now()
|
|
for k, e := range c.entries {
|
|
if now.After(e.expiresAt) {
|
|
delete(c.entries, k)
|
|
}
|
|
}
|
|
if len(c.entries) < c.maxSize {
|
|
return
|
|
}
|
|
var oldestKey string
|
|
var oldestExp time.Time
|
|
for len(c.entries) >= c.maxSize {
|
|
oldestKey = ""
|
|
for k, e := range c.entries {
|
|
if oldestKey == "" || e.expiresAt.Before(oldestExp) {
|
|
oldestKey = k
|
|
oldestExp = e.expiresAt
|
|
}
|
|
}
|
|
if oldestKey == "" {
|
|
return
|
|
}
|
|
delete(c.entries, oldestKey)
|
|
}
|
|
}
|
|
|
|
// doOrFetch returns a cached value when fresh; otherwise runs fetch under a
|
|
// singleflight guard, caches the result, and returns it. Errors are not
|
|
// cached. The returned value is shared — callers must not mutate it.
|
|
func (c *ttlCache) doOrFetch(key string, fetch func() (any, error)) (any, error) {
|
|
if v, ok := c.get(key); ok {
|
|
return v, nil
|
|
}
|
|
v, err, _ := c.sf.Do(key, func() (any, error) {
|
|
if v, ok := c.get(key); ok {
|
|
return v, nil
|
|
}
|
|
v, err := fetch()
|
|
if err != nil {
|
|
return nil, err
|
|
}
|
|
c.set(key, v)
|
|
return v, nil
|
|
})
|
|
return v, err
|
|
}
|
|
|
|
// searchCacheKey composes a cache key with length-prefixed string fields so
|
|
// that user-controllable inputs (e.g. customQuery) cannot collide with other
|
|
// keys by embedding the separator.
|
|
func searchCacheKey(kind, filter, customQuery string, page, perPage int) string {
|
|
return fmt.Sprintf("%d:%s|%d:%s|%d:%s|%d|%d",
|
|
len(kind), kind, len(filter), filter, len(customQuery), customQuery, page, perPage)
|
|
}
|
|
|
|
func prStatusCacheKey(owner, repo string, number int) string {
|
|
return fmt.Sprintf("%s/%s#%d", owner, repo, number)
|
|
}
|