feat(github): add /github page for PRs and issues (#687)
* feat(github): add /github page for PRs and issues Dedicated page under /github lists pull requests and issues across the authenticated user's repos with preset filters, custom GitHub search queries, per-repo narrowing, saved queries, and one-click task launching for any PR or issue. Performance: - Cache user PR/issue search responses for 30s with singleflight to coalesce concurrent misses, so preset switching and pagination avoid upstream round trips. - Replace the per-row PR status fetch (N+1) with a batch endpoint POST /api/v1/github/prs/statuses that fans out concurrently on the backend with a semaphore and caches per-PR. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * style: apply Prettier formatting Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix: address review feedback on PR cache + status batching - Harden searchCacheKey against separator collisions from user-controllable customQuery (length-prefix encoding). - Drop repositories.loaded hydration when inner workspace fetch fails so the client refetches instead of trusting a partial snapshot. - Guard useGitHubSearch with a monotonic request counter to drop stale responses when filter+page changes overlap. - Fix usePRStatuses teardown: depend only on composed key so a new array identity per render doesn't cancel the in-flight batch. - Correct godocs (duplicate, outdated state:open claim). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix: address additional review feedback - Restore state:open for issue watcher: buildIssueSearchQuery no longer injects it as a default, so existing watches were starting to surface closed issues. Inject state:open in buildIssueFilter so only the watcher path is affected; the /github page presets already set their own state qualifiers. - countCheckResults now excludes in-progress checks from the denominator so a `1/3` badge reflects pass rate on completed checks, not a mix of running and failed. The pending state is still carried via ChecksState. - usePRStatuses retries after a transient fetch error (only mark the key as completed once the fetch resolves). This also fixes missing badges under React Strict Mode's intentional double-mount, since the first effect's cleanup fires before the response lands and the guard stays unset for the second mount to retry. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * test(github): add tests for my-github hooks Covers use-committed-query, buildParams from use-github-search, use-saved-presets readStorage validation, use-known-repos recordForKey accumulator semantics, and use-pr-statuses batch fetch lifecycle. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix: address review feedback on search cache and repo accumulator - Clamp perPage before composing the search cache key so identical pages (e.g. perPage=150 and =100) share a cache entry instead of creating duplicates keyed on the raw value. - Add resetKnownReposStore and call it on /github unmount so the module-level repo accumulator doesn't carry across navigations. - Rename use-pr-statuses test to match its assertion (it verifies stale statuses are cleared when the key changes and fetch fails). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * style: apply Prettier formatting Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(web): distinguish blocked from dirty in PR mergeable badge GitHub's blocked state (required reviews/checks/branch protection) is not a merge conflict, so don't label it "Conflicts". Surface it as "Blocked" with the pending tone; keep "Conflicts" for dirty. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix: address remaining /github code review comments - Route PR status + batch through handleSearchError so ErrNoClient surfaces as 503 instead of 500 (matches the search endpoints). - Sort workflow steps by position in the quick-task-launcher default pick, matching the convention in issue-watch-dialog. - Make PresetItem keyboard-accessible: role=button, tabIndex, and Enter/Space handling on the wrapping div (it contains a nested delete button, so it can't itself be a button). - Extract useRepoOptions out of useGitHubPageState to bring the hook back under the 100-line complexity limit. - Honor ctx.Done() when acquiring the PR-status batch semaphore so a cancelled HTTP request releases queued goroutines instead of running each upstream fetch to completion. - Clarify parsePaginationQuery doc: the upper bound (100) is enforced by clampSearchPage in the client layer, not here. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(web): address /github review follow-ups - Merge duplicate @/lib/utils imports in pr-list. - Use rel="noopener noreferrer" on external PR/issue links (codebase convention) and add cursor-pointer per the UI guidelines. - Rewrite onOpenSaveDialog as an explicit if-block so the callback returns void rather than boolean | void. - Use PaginationEllipsis in its own PaginationItem so the ellipsis and the adjacent page link are separate list items instead of visually fused into a single cell. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * refactor(web): derive QuickTaskLauncher dialog state from props Replaces the useEffect+setState anti-pattern with useMemo-derived state driven directly by the payload prop. Drops the eslint suppression that was needed for react-hooks/set-state-in-effect. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> --------- Co-authored-by: Kandev Agent <agent@kandev.dev> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
@@ -70,6 +70,18 @@ type Client interface {
|
||||
// customQuery, when non-empty, replaces the entire generated query.
|
||||
ListIssues(ctx context.Context, filter, customQuery string) ([]*Issue, error)
|
||||
|
||||
// SearchPRs searches for PRs matching the given query.
|
||||
// filter is an optional additional search qualifier (e.g. "author:@me" or "repo:owner/name").
|
||||
// customQuery, when non-empty, replaces the entire generated query.
|
||||
SearchPRs(ctx context.Context, filter, customQuery string) ([]*PR, error)
|
||||
|
||||
// SearchPRsPaged is the paginated variant of SearchPRs. page is 1-indexed;
|
||||
// perPage is clamped to GitHub's 1..100 range by the implementation.
|
||||
SearchPRsPaged(ctx context.Context, filter, customQuery string, page, perPage int) (*PRSearchPage, error)
|
||||
|
||||
// ListIssuesPaged is the paginated variant of ListIssues.
|
||||
ListIssuesPaged(ctx context.Context, filter, customQuery string, page, perPage int) (*IssueSearchPage, error)
|
||||
|
||||
// GetIssueState returns the state of a single issue ("open" or "closed").
|
||||
GetIssueState(ctx context.Context, owner, repo string, number int) (string, error)
|
||||
}
|
||||
|
||||
@@ -89,6 +89,7 @@ func getPRStatus(ctx context.Context, c Client, owner, repo string, number int)
|
||||
checks = []CheckRun{}
|
||||
}
|
||||
reviewState, pendingReviewCount := deriveReviewSyncState(pr, reviews)
|
||||
total, passing := countCheckResults(checks)
|
||||
return &PRStatus{
|
||||
PR: pr,
|
||||
ReviewState: reviewState,
|
||||
@@ -96,9 +97,36 @@ func getPRStatus(ctx context.Context, c Client, owner, repo string, number int)
|
||||
MergeableState: pr.MergeableState,
|
||||
ReviewCount: len(reviews),
|
||||
PendingReviewCount: pendingReviewCount,
|
||||
ChecksTotal: total,
|
||||
ChecksPassing: passing,
|
||||
}, nil
|
||||
}
|
||||
|
||||
// countCheckResults returns (total, passing) counts using the same logic as
|
||||
// computeOverallCheckStatus: skipped/neutral and still-running checks are
|
||||
// excluded from the total so the X/Y badge communicates pass rate rather
|
||||
// than conflating in-flight checks with failures. The pending state is
|
||||
// carried separately via ChecksState.
|
||||
func countCheckResults(checks []CheckRun) (int, int) {
|
||||
total, passing := 0, 0
|
||||
for _, c := range checks {
|
||||
if c.Status != checkStatusCompleted {
|
||||
continue
|
||||
}
|
||||
switch c.Conclusion {
|
||||
case checkConclusionSkipped, checkConclusionNeutral:
|
||||
continue
|
||||
case checkConclusionFail, checkConclusionTimedOut,
|
||||
checkConclusionCancelled, checkConclusionActionRequired:
|
||||
total++
|
||||
default:
|
||||
total++
|
||||
passing++
|
||||
}
|
||||
}
|
||||
return total, passing
|
||||
}
|
||||
|
||||
// convertRawCheckRuns converts raw ghCheckRun structs into the domain CheckRun type.
|
||||
func convertRawCheckRuns(raw []ghCheckRun) []CheckRun {
|
||||
checks := make([]CheckRun, len(raw))
|
||||
@@ -304,18 +332,59 @@ func convertSearchItemToPR(
|
||||
}
|
||||
}
|
||||
|
||||
// clampSearchPage normalizes page and perPage to GitHub's Search API limits:
|
||||
// page >= 1, 1 <= perPage <= 100. Defaults: page=1, perPage=50.
|
||||
func clampSearchPage(page, perPage int) (int, int) {
|
||||
if page < 1 {
|
||||
page = 1
|
||||
}
|
||||
if perPage <= 0 {
|
||||
perPage = 50
|
||||
}
|
||||
if perPage > 100 {
|
||||
perPage = 100
|
||||
}
|
||||
return page, perPage
|
||||
}
|
||||
|
||||
// hasTypeQualifier reports whether the query already pins the result type via
|
||||
// `type:pr`, `type:issue`, `is:pr`, or `is:issue` (case-insensitive).
|
||||
func hasTypeQualifier(query string) bool {
|
||||
lower := strings.ToLower(query)
|
||||
for _, tok := range strings.Fields(lower) {
|
||||
switch tok {
|
||||
case "type:pr", "type:issue", "is:pr", "is:issue":
|
||||
return true
|
||||
}
|
||||
}
|
||||
return false
|
||||
}
|
||||
|
||||
// buildIssueSearchQuery assembles a GitHub search query for issues.
|
||||
// When customQuery is non-empty, it is used verbatim as the entire query.
|
||||
// Otherwise a query is built from optional filter qualifiers.
|
||||
// The `type:issue` qualifier is always injected (unless the caller already
|
||||
// pinned it) because `/search/issues` returns both PRs and issues otherwise.
|
||||
func buildIssueSearchQuery(filter, customQuery string) string {
|
||||
return buildSearchQuery("type:issue", filter, customQuery)
|
||||
}
|
||||
|
||||
// buildPRSearchQuery assembles a GitHub search query for pull requests.
|
||||
// The `type:pr` qualifier is always injected (unless the caller already pinned
|
||||
// it) because `/search/issues` returns both PRs and issues otherwise.
|
||||
func buildPRSearchQuery(filter, customQuery string) string {
|
||||
return buildSearchQuery("type:pr", filter, customQuery)
|
||||
}
|
||||
|
||||
func buildSearchQuery(typeQualifier, filter, customQuery string) string {
|
||||
if customQuery != "" {
|
||||
return customQuery
|
||||
if hasTypeQualifier(customQuery) {
|
||||
return customQuery
|
||||
}
|
||||
return typeQualifier + " " + customQuery
|
||||
}
|
||||
base := "type:issue state:open"
|
||||
if filter != "" {
|
||||
base += " " + filter
|
||||
return typeQualifier + " " + filter
|
||||
}
|
||||
return base
|
||||
return typeQualifier
|
||||
}
|
||||
|
||||
// issueSearchItem is an issue item from the GitHub search API.
|
||||
|
||||
@@ -399,6 +399,55 @@ func TestHasFailingChecks(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
func TestCountCheckResults(t *testing.T) {
|
||||
tests := []struct {
|
||||
name string
|
||||
checks []CheckRun
|
||||
wantTotal, wantPass int
|
||||
}{
|
||||
{
|
||||
name: "empty",
|
||||
wantTotal: 0, wantPass: 0,
|
||||
},
|
||||
{
|
||||
name: "in-progress checks excluded from total",
|
||||
checks: []CheckRun{
|
||||
{Status: checkStatusCompleted, Conclusion: checkConclusionSuccess},
|
||||
{Status: "in_progress", Conclusion: ""},
|
||||
{Status: "queued", Conclusion: ""},
|
||||
},
|
||||
wantTotal: 1, wantPass: 1,
|
||||
},
|
||||
{
|
||||
name: "skipped and neutral are excluded",
|
||||
checks: []CheckRun{
|
||||
{Status: checkStatusCompleted, Conclusion: checkConclusionSuccess},
|
||||
{Status: checkStatusCompleted, Conclusion: checkConclusionSkipped},
|
||||
{Status: checkStatusCompleted, Conclusion: checkConclusionNeutral},
|
||||
},
|
||||
wantTotal: 1, wantPass: 1,
|
||||
},
|
||||
{
|
||||
name: "failures count toward total but not passing",
|
||||
checks: []CheckRun{
|
||||
{Status: checkStatusCompleted, Conclusion: checkConclusionSuccess},
|
||||
{Status: checkStatusCompleted, Conclusion: checkConclusionFail},
|
||||
{Status: checkStatusCompleted, Conclusion: checkConclusionTimedOut},
|
||||
},
|
||||
wantTotal: 3, wantPass: 1,
|
||||
},
|
||||
}
|
||||
for _, tt := range tests {
|
||||
t.Run(tt.name, func(t *testing.T) {
|
||||
total, passing := countCheckResults(tt.checks)
|
||||
if total != tt.wantTotal || passing != tt.wantPass {
|
||||
t.Errorf("countCheckResults = (%d, %d), want (%d, %d)",
|
||||
total, passing, tt.wantTotal, tt.wantPass)
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
func TestHasChangesRequested(t *testing.T) {
|
||||
tests := []struct {
|
||||
name string
|
||||
|
||||
@@ -34,6 +34,8 @@ func (c *Controller) RegisterHTTPRoutes(router *gin.Engine) {
|
||||
|
||||
api.GET("/prs/:owner/:repo/:number", c.httpGetPRFeedback)
|
||||
api.GET("/prs/:owner/:repo/:number/info", c.httpGetPRInfo)
|
||||
api.GET("/prs/:owner/:repo/:number/status", c.httpGetPRStatus)
|
||||
api.POST("/prs/statuses", c.httpGetPRStatusesBatch)
|
||||
api.POST("/prs/:owner/:repo/:number/reviews", c.httpSubmitReview)
|
||||
|
||||
api.GET("/watches/pr", c.httpListPRWatches)
|
||||
@@ -57,6 +59,9 @@ func (c *Controller) RegisterHTTPRoutes(router *gin.Engine) {
|
||||
api.GET("/repos/search", c.httpSearchRepos)
|
||||
api.GET("/repos/:owner/:repo/branches", c.httpListRepoBranches)
|
||||
|
||||
api.GET("/user/prs", c.httpSearchUserPRs)
|
||||
api.GET("/user/issues", c.httpSearchUserIssues)
|
||||
|
||||
api.GET("/stats", c.httpGetStats)
|
||||
}
|
||||
|
||||
@@ -128,6 +133,52 @@ func (c *Controller) httpGetPRFeedback(ctx *gin.Context) {
|
||||
ctx.JSON(http.StatusOK, feedback)
|
||||
}
|
||||
|
||||
func (c *Controller) httpGetPRStatus(ctx *gin.Context) {
|
||||
owner := ctx.Param("owner")
|
||||
repo := ctx.Param("repo")
|
||||
numberStr := ctx.Param("number")
|
||||
number, err := strconv.Atoi(numberStr)
|
||||
if err != nil {
|
||||
ctx.JSON(http.StatusBadRequest, gin.H{"error": "invalid PR number"})
|
||||
return
|
||||
}
|
||||
status, err := c.service.GetPRStatus(ctx.Request.Context(), owner, repo, number)
|
||||
if err != nil {
|
||||
c.handleSearchError(ctx, err)
|
||||
return
|
||||
}
|
||||
ctx.JSON(http.StatusOK, status)
|
||||
}
|
||||
|
||||
// prStatusesBatchMaxRefs caps how many PRs a single batch request can ask for.
|
||||
// The list page defaults to 25 per page and can go up to 100; double that
|
||||
// gives slack without letting a rogue client ask for thousands.
|
||||
const prStatusesBatchMaxRefs = 200
|
||||
|
||||
// httpGetPRStatusesBatch fetches statuses for multiple PRs in one request.
|
||||
// Body: {"refs": [{"owner","repo","number"}, ...]}.
|
||||
// Response: {"statuses": {"<owner>/<repo>#<number>": <PRStatus>}}. PRs that
|
||||
// fail upstream are omitted rather than failing the whole batch.
|
||||
func (c *Controller) httpGetPRStatusesBatch(ctx *gin.Context) {
|
||||
var body struct {
|
||||
Refs []PRRef `json:"refs"`
|
||||
}
|
||||
if err := ctx.ShouldBindJSON(&body); err != nil {
|
||||
ctx.JSON(http.StatusBadRequest, gin.H{"error": "invalid request body"})
|
||||
return
|
||||
}
|
||||
if len(body.Refs) > prStatusesBatchMaxRefs {
|
||||
ctx.JSON(http.StatusBadRequest, gin.H{"error": "too many refs"})
|
||||
return
|
||||
}
|
||||
statuses, err := c.service.GetPRStatusesBatch(ctx.Request.Context(), body.Refs)
|
||||
if err != nil {
|
||||
c.handleSearchError(ctx, err)
|
||||
return
|
||||
}
|
||||
ctx.JSON(http.StatusOK, gin.H{"statuses": statuses})
|
||||
}
|
||||
|
||||
func (c *Controller) httpGetPRInfo(ctx *gin.Context) {
|
||||
owner := ctx.Param("owner")
|
||||
repo := ctx.Param("repo")
|
||||
@@ -344,6 +395,84 @@ func (c *Controller) httpListRepoBranches(ctx *gin.Context) {
|
||||
ctx.JSON(http.StatusOK, gin.H{"branches": branches})
|
||||
}
|
||||
|
||||
// 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`
|
||||
// - page: 1-indexed page (default 1)
|
||||
// - per_page: items per page, clamped to 1..100 (default 50)
|
||||
//
|
||||
// When query is non-empty it is used verbatim; otherwise filter is used.
|
||||
func (c *Controller) httpSearchUserPRs(ctx *gin.Context) {
|
||||
query := ctx.Query("query")
|
||||
filter := ctx.Query("filter")
|
||||
page, perPage := parsePaginationQuery(ctx)
|
||||
result, err := c.service.SearchUserPRsPaged(ctx.Request.Context(), filter, query, page, perPage)
|
||||
if err != nil {
|
||||
c.handleSearchError(ctx, err)
|
||||
return
|
||||
}
|
||||
if result.PRs == nil {
|
||||
result.PRs = []*PR{}
|
||||
}
|
||||
ctx.JSON(http.StatusOK, result)
|
||||
}
|
||||
|
||||
// httpSearchUserIssues mirrors httpSearchUserPRs for issues.
|
||||
func (c *Controller) httpSearchUserIssues(ctx *gin.Context) {
|
||||
query := ctx.Query("query")
|
||||
filter := ctx.Query("filter")
|
||||
page, perPage := parsePaginationQuery(ctx)
|
||||
result, err := c.service.SearchUserIssuesPaged(ctx.Request.Context(), filter, query, page, perPage)
|
||||
if err != nil {
|
||||
c.handleSearchError(ctx, err)
|
||||
return
|
||||
}
|
||||
if result.Issues == nil {
|
||||
result.Issues = []*Issue{}
|
||||
}
|
||||
ctx.JSON(http.StatusOK, result)
|
||||
}
|
||||
|
||||
// parsePaginationQuery reads `page` and `per_page` query params. Missing or
|
||||
// non-positive values fall back to defaults (page=1, perPage=50). The upper
|
||||
// bound for perPage (100) is applied later by clampSearchPage in the client
|
||||
// layer, where it also gates the cache key.
|
||||
func parsePaginationQuery(ctx *gin.Context) (int, int) {
|
||||
page, _ := strconv.Atoi(ctx.Query("page"))
|
||||
perPage, _ := strconv.Atoi(ctx.Query("per_page"))
|
||||
if page < 1 {
|
||||
page = 1
|
||||
}
|
||||
if perPage <= 0 {
|
||||
perPage = 50
|
||||
}
|
||||
return page, perPage
|
||||
}
|
||||
|
||||
// handleSearchError maps client errors to proper HTTP responses.
|
||||
func (c *Controller) handleSearchError(ctx *gin.Context, err error) {
|
||||
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()})
|
||||
}
|
||||
|
||||
func (c *Controller) httpGetStats(ctx *gin.Context) {
|
||||
req := &PRStatsRequest{
|
||||
WorkspaceID: ctx.Query("workspace_id"),
|
||||
|
||||
@@ -35,6 +35,12 @@ func (s *stubClient) FindPRByBranch(context.Context, string, string, string) (*P
|
||||
func (s *stubClient) ListAuthoredPRs(context.Context, string, string) ([]*PR, error) {
|
||||
return nil, nil
|
||||
}
|
||||
func (s *stubClient) SearchPRs(context.Context, string, string) ([]*PR, error) {
|
||||
return nil, nil
|
||||
}
|
||||
func (s *stubClient) SearchPRsPaged(context.Context, string, string, int, int) (*PRSearchPage, error) {
|
||||
return &PRSearchPage{PRs: []*PR{}}, nil
|
||||
}
|
||||
func (s *stubClient) ListReviewRequestedPRs(context.Context, string, string, string) ([]*PR, error) {
|
||||
return nil, nil
|
||||
}
|
||||
@@ -69,6 +75,9 @@ func (s *stubClient) ListRepoBranches(context.Context, string, string) ([]RepoBr
|
||||
func (s *stubClient) ListIssues(context.Context, string, string) ([]*Issue, error) {
|
||||
return nil, nil
|
||||
}
|
||||
func (s *stubClient) ListIssuesPaged(context.Context, string, string, int, int) (*IssueSearchPage, error) {
|
||||
return &IssueSearchPage{Issues: []*Issue{}}, nil
|
||||
}
|
||||
func (s *stubClient) GetIssueState(context.Context, string, string, int) (string, error) {
|
||||
return defaultPRState, nil
|
||||
}
|
||||
|
||||
@@ -171,21 +171,69 @@ func (c *GHClient) ListReviewRequestedPRs(ctx context.Context, scope, filter, cu
|
||||
return c.parseSearchResults(out)
|
||||
}
|
||||
|
||||
func (c *GHClient) SearchPRs(ctx context.Context, filter, customQuery string) ([]*PR, error) {
|
||||
page, err := c.SearchPRsPaged(ctx, filter, customQuery, 1, 50)
|
||||
if err != nil {
|
||||
return nil, err
|
||||
}
|
||||
return page.PRs, nil
|
||||
}
|
||||
|
||||
func (c *GHClient) SearchPRsPaged(ctx context.Context, filter, customQuery string, page, perPage int) (*PRSearchPage, error) {
|
||||
page, perPage = clampSearchPage(page, perPage)
|
||||
query := buildPRSearchQuery(filter, customQuery)
|
||||
out, err := c.run(ctx, "api", "search/issues",
|
||||
"-X", "GET",
|
||||
"-f", "q="+query,
|
||||
"-f", fmt.Sprintf("per_page=%d", perPage),
|
||||
"-f", fmt.Sprintf("page=%d", page),
|
||||
)
|
||||
if err != nil {
|
||||
return nil, fmt.Errorf("search PRs: %w", err)
|
||||
}
|
||||
var result struct {
|
||||
TotalCount int `json:"total_count"`
|
||||
Items json.RawMessage `json:"items"`
|
||||
}
|
||||
if err := json.Unmarshal([]byte(out), &result); err != nil {
|
||||
return nil, fmt.Errorf("parse PR search response: %w", err)
|
||||
}
|
||||
prs, err := c.parseSearchResults(string(result.Items))
|
||||
if err != nil {
|
||||
return nil, err
|
||||
}
|
||||
return &PRSearchPage{PRs: prs, TotalCount: result.TotalCount, Page: page, PerPage: perPage}, nil
|
||||
}
|
||||
|
||||
func (c *GHClient) ListIssues(ctx context.Context, filter, customQuery string) ([]*Issue, error) {
|
||||
page, err := c.ListIssuesPaged(ctx, filter, customQuery, 1, 50)
|
||||
if err != nil {
|
||||
return nil, err
|
||||
}
|
||||
return page.Issues, nil
|
||||
}
|
||||
|
||||
func (c *GHClient) ListIssuesPaged(ctx context.Context, filter, customQuery string, page, perPage int) (*IssueSearchPage, error) {
|
||||
page, perPage = clampSearchPage(page, perPage)
|
||||
query := buildIssueSearchQuery(filter, customQuery)
|
||||
out, err := c.run(ctx, "api", "search/issues",
|
||||
"-X", "GET",
|
||||
"-f", "q="+query,
|
||||
"-f", "per_page=50",
|
||||
"--jq", ".items")
|
||||
"-f", fmt.Sprintf("per_page=%d", perPage),
|
||||
"-f", fmt.Sprintf("page=%d", page),
|
||||
)
|
||||
if err != nil {
|
||||
return nil, fmt.Errorf("list issues: %w", err)
|
||||
}
|
||||
var items []issueSearchItem
|
||||
if err := json.Unmarshal([]byte(out), &items); err != nil {
|
||||
return nil, fmt.Errorf("parse issue search results: %w", err)
|
||||
var result struct {
|
||||
TotalCount int `json:"total_count"`
|
||||
Items []issueSearchItem `json:"items"`
|
||||
}
|
||||
return parseIssueSearchResults(items), nil
|
||||
if err := json.Unmarshal([]byte(out), &result); err != nil {
|
||||
return nil, fmt.Errorf("parse issue search response: %w", err)
|
||||
}
|
||||
issues := parseIssueSearchResults(result.Items)
|
||||
return &IssueSearchPage{Issues: issues, TotalCount: result.TotalCount, Page: page, PerPage: perPage}, nil
|
||||
}
|
||||
|
||||
func (c *GHClient) GetIssueState(ctx context.Context, owner, repo string, number int) (string, error) {
|
||||
|
||||
@@ -137,6 +137,28 @@ func (m *MockClient) ListIssues(context.Context, string, string) ([]*Issue, erro
|
||||
return nil, nil
|
||||
}
|
||||
|
||||
func (m *MockClient) ListIssuesPaged(context.Context, string, string, int, int) (*IssueSearchPage, error) {
|
||||
return &IssueSearchPage{Issues: []*Issue{}, TotalCount: 0, Page: 1, PerPage: 50}, nil
|
||||
}
|
||||
|
||||
func (m *MockClient) SearchPRs(context.Context, string, string) ([]*PR, error) {
|
||||
m.mu.RLock()
|
||||
defer m.mu.RUnlock()
|
||||
result := make([]*PR, 0, len(m.prs))
|
||||
for _, pr := range m.prs {
|
||||
result = append(result, pr)
|
||||
}
|
||||
return result, nil
|
||||
}
|
||||
|
||||
func (m *MockClient) SearchPRsPaged(ctx context.Context, _, _ string, page, perPage int) (*PRSearchPage, error) {
|
||||
prs, err := m.SearchPRs(ctx, "", "")
|
||||
if err != nil {
|
||||
return nil, err
|
||||
}
|
||||
return &PRSearchPage{PRs: prs, TotalCount: len(prs), Page: page, PerPage: perPage}, nil
|
||||
}
|
||||
|
||||
func (m *MockClient) GetIssueState(context.Context, string, string, int) (string, error) {
|
||||
return defaultPRState, nil
|
||||
}
|
||||
|
||||
@@ -92,6 +92,25 @@ type PRStatus struct {
|
||||
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"`
|
||||
ChecksTotal int `json:"checks_total"`
|
||||
ChecksPassing int `json:"checks_passing"`
|
||||
}
|
||||
|
||||
// PRSearchPage is a paginated slice of PR search results, with the total
|
||||
// count reported by the GitHub Search API (capped at 1000).
|
||||
type PRSearchPage struct {
|
||||
PRs []*PR `json:"prs"`
|
||||
TotalCount int `json:"total_count"`
|
||||
Page int `json:"page"`
|
||||
PerPage int `json:"per_page"`
|
||||
}
|
||||
|
||||
// IssueSearchPage is a paginated slice of Issue search results.
|
||||
type IssueSearchPage struct {
|
||||
Issues []*Issue `json:"issues"`
|
||||
TotalCount int `json:"total_count"`
|
||||
Page int `json:"page"`
|
||||
PerPage int `json:"per_page"`
|
||||
}
|
||||
|
||||
// PRWatch tracks active PR monitoring (session → PR).
|
||||
|
||||
@@ -85,6 +85,18 @@ func (c *NoopClient) ListIssues(context.Context, string, string) ([]*Issue, erro
|
||||
return nil, ErrNoClient
|
||||
}
|
||||
|
||||
func (c *NoopClient) ListIssuesPaged(context.Context, string, string, int, int) (*IssueSearchPage, error) {
|
||||
return nil, ErrNoClient
|
||||
}
|
||||
|
||||
func (c *NoopClient) SearchPRs(context.Context, string, string) ([]*PR, error) {
|
||||
return nil, ErrNoClient
|
||||
}
|
||||
|
||||
func (c *NoopClient) SearchPRsPaged(context.Context, string, string, int, int) (*PRSearchPage, error) {
|
||||
return nil, ErrNoClient
|
||||
}
|
||||
|
||||
func (c *NoopClient) GetIssueState(context.Context, string, string, int) (string, error) {
|
||||
return "", ErrNoClient
|
||||
}
|
||||
|
||||
@@ -133,16 +133,59 @@ func (c *PATClient) ListReviewRequestedPRs(ctx context.Context, scope, filter, c
|
||||
return prs, nil
|
||||
}
|
||||
|
||||
func (c *PATClient) SearchPRs(ctx context.Context, filter, customQuery string) ([]*PR, error) {
|
||||
page, err := c.SearchPRsPaged(ctx, filter, customQuery, 1, 50)
|
||||
if err != nil {
|
||||
return nil, err
|
||||
}
|
||||
return page.PRs, nil
|
||||
}
|
||||
|
||||
func (c *PATClient) SearchPRsPaged(ctx context.Context, filter, customQuery string, page, perPage int) (*PRSearchPage, error) {
|
||||
page, perPage = clampSearchPage(page, perPage)
|
||||
query := buildPRSearchQuery(filter, customQuery)
|
||||
var result struct {
|
||||
TotalCount int `json:"total_count"`
|
||||
Items []patSearchItem `json:"items"`
|
||||
}
|
||||
endpoint := fmt.Sprintf("/search/issues?q=%s&per_page=%d&page=%d",
|
||||
url.QueryEscape(query), perPage, page)
|
||||
if err := c.get(ctx, endpoint, &result); err != nil {
|
||||
return nil, fmt.Errorf("search PRs: %w", err)
|
||||
}
|
||||
prs := make([]*PR, len(result.Items))
|
||||
for i, item := range result.Items {
|
||||
prs[i] = convertSearchItemToPR(
|
||||
item.Number, item.Title, item.HTMLURL, item.State,
|
||||
item.User.Login, item.RepositoryURL, item.Draft,
|
||||
item.CreatedAt, item.UpdatedAt,
|
||||
)
|
||||
}
|
||||
return &PRSearchPage{PRs: prs, TotalCount: result.TotalCount, Page: page, PerPage: perPage}, nil
|
||||
}
|
||||
|
||||
func (c *PATClient) ListIssues(ctx context.Context, filter, customQuery string) ([]*Issue, error) {
|
||||
page, err := c.ListIssuesPaged(ctx, filter, customQuery, 1, 50)
|
||||
if err != nil {
|
||||
return nil, err
|
||||
}
|
||||
return page.Issues, nil
|
||||
}
|
||||
|
||||
func (c *PATClient) ListIssuesPaged(ctx context.Context, filter, customQuery string, page, perPage int) (*IssueSearchPage, error) {
|
||||
page, perPage = clampSearchPage(page, perPage)
|
||||
query := buildIssueSearchQuery(filter, customQuery)
|
||||
var result struct {
|
||||
Items []issueSearchItem `json:"items"`
|
||||
TotalCount int `json:"total_count"`
|
||||
Items []issueSearchItem `json:"items"`
|
||||
}
|
||||
endpoint := "/search/issues?q=" + url.QueryEscape(query) + "&per_page=50"
|
||||
endpoint := fmt.Sprintf("/search/issues?q=%s&per_page=%d&page=%d",
|
||||
url.QueryEscape(query), perPage, page)
|
||||
if err := c.get(ctx, endpoint, &result); err != nil {
|
||||
return nil, fmt.Errorf("search issues: %w", err)
|
||||
}
|
||||
return parseIssueSearchResults(result.Items), nil
|
||||
issues := parseIssueSearchResults(result.Items)
|
||||
return &IssueSearchPage{Issues: issues, TotalCount: result.TotalCount, Page: page, PerPage: perPage}, nil
|
||||
}
|
||||
|
||||
func (c *PATClient) GetIssueState(ctx context.Context, owner, repo string, number int) (string, error) {
|
||||
|
||||
@@ -48,17 +48,21 @@ type Service struct {
|
||||
taskSessionChecker TaskSessionChecker
|
||||
syncGroup singleflight.Group
|
||||
taskEventSubs []bus.Subscription
|
||||
searchCache *ttlCache
|
||||
prStatusCache *ttlCache
|
||||
}
|
||||
|
||||
// NewService creates a new GitHub service.
|
||||
func NewService(client Client, authMethod string, secrets SecretProvider, store *Store, eventBus bus.EventBus, log *logger.Logger) *Service {
|
||||
return &Service{
|
||||
client: client,
|
||||
authMethod: authMethod,
|
||||
secrets: secrets,
|
||||
store: store,
|
||||
eventBus: eventBus,
|
||||
logger: log,
|
||||
client: client,
|
||||
authMethod: authMethod,
|
||||
secrets: secrets,
|
||||
store: store,
|
||||
eventBus: eventBus,
|
||||
logger: log,
|
||||
searchCache: newTTLCache(),
|
||||
prStatusCache: newTTLCache(),
|
||||
}
|
||||
}
|
||||
|
||||
@@ -544,6 +548,83 @@ func (s *Service) GetPRFeedback(ctx context.Context, owner, repo string, number
|
||||
return s.client.GetPRFeedback(ctx, owner, repo, number)
|
||||
}
|
||||
|
||||
// GetPRStatus fetches lightweight PR status (review + checks + mergeable).
|
||||
// Cached briefly so repeat loads of the same list (pagination, re-render,
|
||||
// back-navigation) don't refetch. The returned pointer is shared — callers
|
||||
// must not mutate it.
|
||||
func (s *Service) GetPRStatus(ctx context.Context, owner, repo string, number int) (*PRStatus, error) {
|
||||
if s.client == nil {
|
||||
return nil, fmt.Errorf("github client not available")
|
||||
}
|
||||
key := prStatusCacheKey(owner, repo, number)
|
||||
v, err := s.prStatusCache.doOrFetch(key, func() (any, error) {
|
||||
return s.client.GetPRStatus(ctx, owner, repo, number)
|
||||
})
|
||||
if err != nil {
|
||||
return nil, err
|
||||
}
|
||||
return v.(*PRStatus), nil
|
||||
}
|
||||
|
||||
// PRRef identifies a pull request by owner/repo/number.
|
||||
type PRRef struct {
|
||||
Owner string `json:"owner"`
|
||||
Repo string `json:"repo"`
|
||||
Number int `json:"number"`
|
||||
}
|
||||
|
||||
// prStatusBatchConcurrency bounds how many upstream GetPRStatus calls run in
|
||||
// parallel. GitHub has per-hour quotas and per-endpoint concurrency limits;
|
||||
// this is well under both while still collapsing a 25-PR page into a single
|
||||
// short wait on the client.
|
||||
const prStatusBatchConcurrency = 8
|
||||
|
||||
// GetPRStatusesBatch fetches statuses for multiple PRs concurrently, honoring
|
||||
// the per-PR cache. The returned map is keyed by prStatusCacheKey; PRs that
|
||||
// fail to fetch are logged and omitted from the result so one bad repo
|
||||
// doesn't poison the page.
|
||||
func (s *Service) GetPRStatusesBatch(ctx context.Context, refs []PRRef) (map[string]*PRStatus, error) {
|
||||
if s.client == nil {
|
||||
return nil, fmt.Errorf("github client not available")
|
||||
}
|
||||
result := make(map[string]*PRStatus, len(refs))
|
||||
var mu sync.Mutex
|
||||
sem := make(chan struct{}, prStatusBatchConcurrency)
|
||||
var wg sync.WaitGroup
|
||||
for _, ref := range refs {
|
||||
if ref.Owner == "" || ref.Repo == "" || ref.Number <= 0 {
|
||||
continue
|
||||
}
|
||||
wg.Add(1)
|
||||
go func(r PRRef) {
|
||||
defer wg.Done()
|
||||
// Release queued goroutines early when the caller disconnects —
|
||||
// otherwise up to 200 refs queue up serially behind the semaphore
|
||||
// and each still runs its full upstream fetch.
|
||||
select {
|
||||
case sem <- struct{}{}:
|
||||
case <-ctx.Done():
|
||||
return
|
||||
}
|
||||
defer func() { <-sem }()
|
||||
status, err := s.GetPRStatus(ctx, r.Owner, r.Repo, r.Number)
|
||||
if err != nil {
|
||||
s.logger.Debug("batch PR status fetch failed",
|
||||
zap.String("owner", r.Owner),
|
||||
zap.String("repo", r.Repo),
|
||||
zap.Int("number", r.Number),
|
||||
zap.Error(err))
|
||||
return
|
||||
}
|
||||
mu.Lock()
|
||||
result[prStatusCacheKey(r.Owner, r.Repo, r.Number)] = status
|
||||
mu.Unlock()
|
||||
}(ref)
|
||||
}
|
||||
wg.Wait()
|
||||
return result, nil
|
||||
}
|
||||
|
||||
// TriggerPRSync performs an immediate PR status sync for a task.
|
||||
// If the watch has a PR number, it fetches the latest status from GitHub
|
||||
// and syncs it to the TaskPR record. If still searching (pr_number=0),
|
||||
@@ -968,6 +1049,78 @@ func (s *Service) ListRepoBranches(ctx context.Context, owner, repo string) ([]R
|
||||
return s.client.ListRepoBranches(ctx, owner, repo)
|
||||
}
|
||||
|
||||
// SearchUserPRs searches for PRs using a filter or custom query. Unless the
|
||||
// caller already pins a type qualifier, `type:pr` is injected into the
|
||||
// composed query.
|
||||
func (s *Service) SearchUserPRs(ctx context.Context, filter, customQuery string) ([]*PR, error) {
|
||||
if s.client == nil {
|
||||
return nil, fmt.Errorf("github client not available")
|
||||
}
|
||||
return s.client.SearchPRs(ctx, filter, customQuery)
|
||||
}
|
||||
|
||||
// SearchUserIssues searches for issues using a filter or custom query. Unless
|
||||
// the caller already pins a type qualifier, `type:issue` is injected into the
|
||||
// composed query.
|
||||
func (s *Service) SearchUserIssues(ctx context.Context, filter, customQuery string) ([]*Issue, error) {
|
||||
if s.client == nil {
|
||||
return nil, fmt.Errorf("github client not available")
|
||||
}
|
||||
return s.client.ListIssues(ctx, filter, customQuery)
|
||||
}
|
||||
|
||||
// SearchUserPRsPaged is the paginated variant of SearchUserPRs. Results are
|
||||
// cached for a short window (see searchCacheTTL) — callers must not mutate
|
||||
// the returned page, since it is shared across concurrent requests.
|
||||
func (s *Service) SearchUserPRsPaged(ctx context.Context, filter, customQuery string, page, perPage int) (*PRSearchPage, error) {
|
||||
if s.client == nil {
|
||||
return nil, fmt.Errorf("github client not available")
|
||||
}
|
||||
// Clamp before composing the cache key so perPage=150 and perPage=100
|
||||
// share the same cached page instead of creating two entries for
|
||||
// identical results.
|
||||
page, perPage = clampSearchPage(page, perPage)
|
||||
key := searchCacheKey("pr", filter, customQuery, page, perPage)
|
||||
v, err := s.searchCache.doOrFetch(key, func() (any, error) {
|
||||
result, err := s.client.SearchPRsPaged(ctx, filter, customQuery, page, perPage)
|
||||
if err != nil {
|
||||
return nil, err
|
||||
}
|
||||
if result != nil && result.PRs == nil {
|
||||
result.PRs = []*PR{}
|
||||
}
|
||||
return result, nil
|
||||
})
|
||||
if err != nil {
|
||||
return nil, err
|
||||
}
|
||||
return v.(*PRSearchPage), nil
|
||||
}
|
||||
|
||||
// SearchUserIssuesPaged is the paginated variant of SearchUserIssues. See
|
||||
// SearchUserPRsPaged for caching semantics.
|
||||
func (s *Service) SearchUserIssuesPaged(ctx context.Context, filter, customQuery string, page, perPage int) (*IssueSearchPage, error) {
|
||||
if s.client == nil {
|
||||
return nil, fmt.Errorf("github client not available")
|
||||
}
|
||||
page, perPage = clampSearchPage(page, perPage)
|
||||
key := searchCacheKey("issue", filter, customQuery, page, perPage)
|
||||
v, err := s.searchCache.doOrFetch(key, func() (any, error) {
|
||||
result, err := s.client.ListIssuesPaged(ctx, filter, customQuery, page, perPage)
|
||||
if err != nil {
|
||||
return nil, err
|
||||
}
|
||||
if result != nil && result.Issues == nil {
|
||||
result.Issues = []*Issue{}
|
||||
}
|
||||
return result, nil
|
||||
})
|
||||
if err != nil {
|
||||
return nil, err
|
||||
}
|
||||
return v.(*IssueSearchPage), nil
|
||||
}
|
||||
|
||||
// ReserveReviewPRTask atomically claims the dedup slot for a (watch, repo, PR)
|
||||
// tuple before task creation begins. Returns true if this caller won and
|
||||
// should proceed to create the task, false if another caller already holds
|
||||
@@ -1319,9 +1472,12 @@ func (s *Service) fetchIssues(ctx context.Context, watch *IssueWatch) ([]*Issue,
|
||||
return s.fetchIssuesWithRepoFilter(ctx, watch), nil
|
||||
}
|
||||
|
||||
// buildIssueFilter builds the filter qualifier from watch labels.
|
||||
// buildIssueFilter builds the filter qualifier from watch labels. `state:open`
|
||||
// is included because the watcher is only interested in active issues —
|
||||
// buildIssueSearchQuery no longer injects it (the /github page presets
|
||||
// supply their own state qualifiers), so we add it here instead.
|
||||
func (s *Service) buildIssueFilter(watch *IssueWatch) string {
|
||||
var parts []string
|
||||
parts := []string{"state:open"}
|
||||
for _, label := range watch.Labels {
|
||||
if strings.ContainsRune(label, ' ') {
|
||||
parts = append(parts, `label:"`+label+`"`)
|
||||
|
||||
@@ -89,3 +89,35 @@ func TestShouldDeleteIssueTask(t *testing.T) {
|
||||
}
|
||||
})
|
||||
}
|
||||
|
||||
func TestBuildIssueFilter_IncludesStateOpen(t *testing.T) {
|
||||
svc := &Service{}
|
||||
tests := []struct {
|
||||
name string
|
||||
watch *IssueWatch
|
||||
want string
|
||||
}{
|
||||
{
|
||||
name: "no labels",
|
||||
watch: &IssueWatch{},
|
||||
want: "state:open",
|
||||
},
|
||||
{
|
||||
name: "single label",
|
||||
watch: &IssueWatch{Labels: []string{"bug"}},
|
||||
want: "state:open label:bug",
|
||||
},
|
||||
{
|
||||
name: "label with space is quoted",
|
||||
watch: &IssueWatch{Labels: []string{"good first issue"}},
|
||||
want: `state:open label:"good first issue"`,
|
||||
},
|
||||
}
|
||||
for _, tt := range tests {
|
||||
t.Run(tt.name, func(t *testing.T) {
|
||||
if got := svc.buildIssueFilter(tt.watch); got != tt.want {
|
||||
t.Errorf("buildIssueFilter() = %q, want %q", got, tt.want)
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
@@ -0,0 +1,122 @@
|
||||
package github
|
||||
|
||||
import (
|
||||
"context"
|
||||
"fmt"
|
||||
"sync/atomic"
|
||||
"testing"
|
||||
)
|
||||
|
||||
type countingStatusClient struct {
|
||||
*MockClient
|
||||
calls atomic.Int32
|
||||
}
|
||||
|
||||
func (c *countingStatusClient) GetPRStatus(_ context.Context, owner, repo string, number int) (*PRStatus, error) {
|
||||
c.calls.Add(1)
|
||||
return &PRStatus{PR: &PR{RepoOwner: owner, RepoName: repo, Number: number}}, nil
|
||||
}
|
||||
|
||||
func TestService_GetPRStatus_UsesCache(t *testing.T) {
|
||||
client := &countingStatusClient{MockClient: NewMockClient()}
|
||||
svc := newTestService(client)
|
||||
ctx := context.Background()
|
||||
|
||||
for i := 0; i < 3; i++ {
|
||||
if _, err := svc.GetPRStatus(ctx, "o", "r", 1); err != nil {
|
||||
t.Fatalf("call %d: %v", i, err)
|
||||
}
|
||||
}
|
||||
if got := client.calls.Load(); got != 1 {
|
||||
t.Fatalf("expected 1 upstream call, got %d", got)
|
||||
}
|
||||
|
||||
if _, err := svc.GetPRStatus(ctx, "o", "r", 2); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
if got := client.calls.Load(); got != 2 {
|
||||
t.Fatalf("expected new fetch for different number, got %d", got)
|
||||
}
|
||||
}
|
||||
|
||||
func TestService_GetPRStatusesBatch_FetchesAndCaches(t *testing.T) {
|
||||
client := &countingStatusClient{MockClient: NewMockClient()}
|
||||
svc := newTestService(client)
|
||||
ctx := context.Background()
|
||||
|
||||
refs := []PRRef{
|
||||
{Owner: "o", Repo: "r", Number: 1},
|
||||
{Owner: "o", Repo: "r", Number: 2},
|
||||
{Owner: "o", Repo: "r", Number: 3},
|
||||
}
|
||||
statuses, err := svc.GetPRStatusesBatch(ctx, refs)
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
if len(statuses) != 3 {
|
||||
t.Fatalf("expected 3 statuses, got %d", len(statuses))
|
||||
}
|
||||
if got := client.calls.Load(); got != 3 {
|
||||
t.Fatalf("expected 3 upstream calls, got %d", got)
|
||||
}
|
||||
|
||||
// Second batch hits the per-PR cache — no new upstream calls.
|
||||
if _, err := svc.GetPRStatusesBatch(ctx, refs); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
if got := client.calls.Load(); got != 3 {
|
||||
t.Fatalf("expected no new upstream calls on 2nd batch, got %d", got)
|
||||
}
|
||||
}
|
||||
|
||||
func TestService_GetPRStatusesBatch_SkipsInvalidRefs(t *testing.T) {
|
||||
client := &countingStatusClient{MockClient: NewMockClient()}
|
||||
svc := newTestService(client)
|
||||
refs := []PRRef{
|
||||
{Owner: "", Repo: "r", Number: 1},
|
||||
{Owner: "o", Repo: "", Number: 2},
|
||||
{Owner: "o", Repo: "r", Number: 0},
|
||||
{Owner: "o", Repo: "r", Number: 7},
|
||||
}
|
||||
statuses, err := svc.GetPRStatusesBatch(context.Background(), refs)
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
if len(statuses) != 1 {
|
||||
t.Fatalf("expected 1 valid status, got %d", len(statuses))
|
||||
}
|
||||
if _, ok := statuses["o/r#7"]; !ok {
|
||||
t.Fatalf("expected status for valid ref, got keys %v", keys(statuses))
|
||||
}
|
||||
}
|
||||
|
||||
type failingStatusClient struct {
|
||||
*MockClient
|
||||
}
|
||||
|
||||
func (failingStatusClient) GetPRStatus(context.Context, string, string, int) (*PRStatus, error) {
|
||||
return nil, fmt.Errorf("upstream boom")
|
||||
}
|
||||
|
||||
func TestService_GetPRStatusesBatch_OmitsFailedRefs(t *testing.T) {
|
||||
svc := newTestService(failingStatusClient{MockClient: NewMockClient()})
|
||||
refs := []PRRef{
|
||||
{Owner: "o", Repo: "r", Number: 1},
|
||||
{Owner: "o", Repo: "r", Number: 2},
|
||||
}
|
||||
statuses, err := svc.GetPRStatusesBatch(context.Background(), refs)
|
||||
if err != nil {
|
||||
t.Fatalf("unexpected batch error: %v", err)
|
||||
}
|
||||
if len(statuses) != 0 {
|
||||
t.Fatalf("expected empty map when all refs fail, got %d", len(statuses))
|
||||
}
|
||||
}
|
||||
|
||||
func keys[K comparable, V any](m map[K]V) []K {
|
||||
out := make([]K, 0, len(m))
|
||||
for k := range m {
|
||||
out = append(out, k)
|
||||
}
|
||||
return out
|
||||
}
|
||||
@@ -0,0 +1,91 @@
|
||||
package github
|
||||
|
||||
import (
|
||||
"context"
|
||||
"sync/atomic"
|
||||
"testing"
|
||||
|
||||
"github.com/kandev/kandev/internal/common/logger"
|
||||
)
|
||||
|
||||
type countingSearchClient struct {
|
||||
*MockClient
|
||||
prCalls atomic.Int32
|
||||
issueCalls atomic.Int32
|
||||
}
|
||||
|
||||
func (c *countingSearchClient) SearchPRsPaged(ctx context.Context, filter, customQuery string, page, perPage int) (*PRSearchPage, error) {
|
||||
c.prCalls.Add(1)
|
||||
return c.MockClient.SearchPRsPaged(ctx, filter, customQuery, page, perPage)
|
||||
}
|
||||
|
||||
func (c *countingSearchClient) ListIssuesPaged(ctx context.Context, filter, customQuery string, page, perPage int) (*IssueSearchPage, error) {
|
||||
c.issueCalls.Add(1)
|
||||
return c.MockClient.ListIssuesPaged(ctx, filter, customQuery, page, perPage)
|
||||
}
|
||||
|
||||
func newTestService(client Client) *Service {
|
||||
return &Service{
|
||||
client: client,
|
||||
authMethod: AuthMethodPAT,
|
||||
logger: logger.Default(),
|
||||
searchCache: newTTLCache(),
|
||||
prStatusCache: newTTLCache(),
|
||||
}
|
||||
}
|
||||
|
||||
func TestService_SearchUserPRsPaged_UsesCache(t *testing.T) {
|
||||
client := &countingSearchClient{MockClient: NewMockClient()}
|
||||
svc := newTestService(client)
|
||||
ctx := context.Background()
|
||||
|
||||
for i := 0; i < 3; i++ {
|
||||
if _, err := svc.SearchUserPRsPaged(ctx, "", "is:open author:@me", 1, 25); err != nil {
|
||||
t.Fatalf("call %d: %v", i, err)
|
||||
}
|
||||
}
|
||||
if got := client.prCalls.Load(); got != 1 {
|
||||
t.Fatalf("expected 1 upstream PR call, got %d", got)
|
||||
}
|
||||
|
||||
if _, err := svc.SearchUserPRsPaged(ctx, "", "is:open author:@me", 2, 25); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
if got := client.prCalls.Load(); got != 2 {
|
||||
t.Fatalf("expected new fetch for different page, got %d", got)
|
||||
}
|
||||
}
|
||||
|
||||
func TestService_SearchUserIssuesPaged_UsesCache(t *testing.T) {
|
||||
client := &countingSearchClient{MockClient: NewMockClient()}
|
||||
svc := newTestService(client)
|
||||
ctx := context.Background()
|
||||
|
||||
for i := 0; i < 3; i++ {
|
||||
if _, err := svc.SearchUserIssuesPaged(ctx, "", "is:open", 1, 25); err != nil {
|
||||
t.Fatalf("call %d: %v", i, err)
|
||||
}
|
||||
}
|
||||
if got := client.issueCalls.Load(); got != 1 {
|
||||
t.Fatalf("expected 1 upstream issue call, got %d", got)
|
||||
}
|
||||
}
|
||||
|
||||
func TestService_SearchUserPRsPaged_PRsIssuesIsolated(t *testing.T) {
|
||||
client := &countingSearchClient{MockClient: NewMockClient()}
|
||||
svc := newTestService(client)
|
||||
ctx := context.Background()
|
||||
|
||||
if _, err := svc.SearchUserPRsPaged(ctx, "", "q", 1, 25); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
if _, err := svc.SearchUserIssuesPaged(ctx, "", "q", 1, 25); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
if got := client.prCalls.Load(); got != 1 {
|
||||
t.Fatalf("pr calls = %d", got)
|
||||
}
|
||||
if got := client.issueCalls.Load(); got != 1 {
|
||||
t.Fatalf("issue calls = %d", got)
|
||||
}
|
||||
}
|
||||
@@ -0,0 +1,129 @@
|
||||
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,
|
||||
}
|
||||
}
|
||||
|
||||
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)
|
||||
}
|
||||
@@ -0,0 +1,161 @@
|
||||
package github
|
||||
|
||||
import (
|
||||
"errors"
|
||||
"fmt"
|
||||
"sync"
|
||||
"sync/atomic"
|
||||
"testing"
|
||||
"testing/synctest"
|
||||
"time"
|
||||
)
|
||||
|
||||
func TestTTLCache_HitAndMiss(t *testing.T) {
|
||||
cache := newTTLCache()
|
||||
var calls int
|
||||
fetch := func() (any, error) {
|
||||
calls++
|
||||
return "result", nil
|
||||
}
|
||||
for i := 0; i < 3; i++ {
|
||||
v, err := cache.doOrFetch("k", fetch)
|
||||
if err != nil {
|
||||
t.Fatalf("unexpected err: %v", err)
|
||||
}
|
||||
if v != "result" {
|
||||
t.Fatalf("unexpected value: %v", v)
|
||||
}
|
||||
}
|
||||
if calls != 1 {
|
||||
t.Fatalf("expected fetch called once, got %d", calls)
|
||||
}
|
||||
}
|
||||
|
||||
func TestTTLCache_Expiry(t *testing.T) {
|
||||
synctest.Test(t, func(t *testing.T) {
|
||||
cache := newTTLCache()
|
||||
cache.ttl = 30 * time.Second
|
||||
var calls int
|
||||
fetch := func() (any, error) {
|
||||
calls++
|
||||
return calls, nil
|
||||
}
|
||||
if _, err := cache.doOrFetch("k", fetch); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
time.Sleep(29 * time.Second)
|
||||
if _, err := cache.doOrFetch("k", fetch); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
if calls != 1 {
|
||||
t.Fatalf("expected cache hit before TTL; calls=%d", calls)
|
||||
}
|
||||
time.Sleep(2 * time.Second)
|
||||
v, err := cache.doOrFetch("k", fetch)
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
if calls != 2 {
|
||||
t.Fatalf("expected refetch after TTL; calls=%d", calls)
|
||||
}
|
||||
if v != 2 {
|
||||
t.Fatalf("expected fresh value 2, got %v", v)
|
||||
}
|
||||
})
|
||||
}
|
||||
|
||||
func TestTTLCache_ErrorNotCached(t *testing.T) {
|
||||
cache := newTTLCache()
|
||||
var calls int
|
||||
sentinel := errors.New("boom")
|
||||
fetch := func() (any, error) {
|
||||
calls++
|
||||
if calls == 1 {
|
||||
return nil, sentinel
|
||||
}
|
||||
return "ok", nil
|
||||
}
|
||||
if _, err := cache.doOrFetch("k", fetch); !errors.Is(err, sentinel) {
|
||||
t.Fatalf("expected sentinel error, got %v", err)
|
||||
}
|
||||
v, err := cache.doOrFetch("k", fetch)
|
||||
if err != nil {
|
||||
t.Fatalf("unexpected err: %v", err)
|
||||
}
|
||||
if v != "ok" {
|
||||
t.Fatalf("unexpected value: %v", v)
|
||||
}
|
||||
if calls != 2 {
|
||||
t.Fatalf("expected 2 fetch calls (error not cached), got %d", calls)
|
||||
}
|
||||
}
|
||||
|
||||
func TestSearchCacheKey_KeyIsolation(t *testing.T) {
|
||||
if searchCacheKey("pr", "", "is:open", 1, 25) == searchCacheKey("issue", "", "is:open", 1, 25) {
|
||||
t.Fatal("kind should be part of key")
|
||||
}
|
||||
if searchCacheKey("pr", "", "is:open", 1, 25) == searchCacheKey("pr", "", "is:open", 2, 25) {
|
||||
t.Fatal("page should be part of key")
|
||||
}
|
||||
if searchCacheKey("pr", "", "is:open", 1, 25) == searchCacheKey("pr", "", "is:closed", 1, 25) {
|
||||
t.Fatal("customQuery should be part of key")
|
||||
}
|
||||
// A user-controllable customQuery must not be able to collide with
|
||||
// adjacent fields by embedding the separator.
|
||||
collisions := [][2][5]any{
|
||||
{{"pr", "a", "b", 1, 25}, {"pr", "a|b", "", 1, 25}},
|
||||
{{"pr", "", "a|b|1|25", 0, 0}, {"pr", "a", "b", 1, 25}},
|
||||
}
|
||||
for _, c := range collisions {
|
||||
a := searchCacheKey(c[0][0].(string), c[0][1].(string), c[0][2].(string), c[0][3].(int), c[0][4].(int))
|
||||
b := searchCacheKey(c[1][0].(string), c[1][1].(string), c[1][2].(string), c[1][3].(int), c[1][4].(int))
|
||||
if a == b {
|
||||
t.Fatalf("separator collision: %q == %q", a, b)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
func TestTTLCache_Singleflight(t *testing.T) {
|
||||
cache := newTTLCache()
|
||||
var calls atomic.Int32
|
||||
release := make(chan struct{})
|
||||
started := make(chan struct{}, 1)
|
||||
fetch := func() (any, error) {
|
||||
calls.Add(1)
|
||||
select {
|
||||
case started <- struct{}{}:
|
||||
default:
|
||||
}
|
||||
<-release
|
||||
return "v", nil
|
||||
}
|
||||
|
||||
const n = 8
|
||||
var wg sync.WaitGroup
|
||||
wg.Add(n)
|
||||
for i := 0; i < n; i++ {
|
||||
go func() {
|
||||
defer wg.Done()
|
||||
if _, err := cache.doOrFetch("k", fetch); err != nil {
|
||||
t.Errorf("fetch err: %v", err)
|
||||
}
|
||||
}()
|
||||
}
|
||||
<-started
|
||||
close(release)
|
||||
wg.Wait()
|
||||
if got := calls.Load(); got != 1 {
|
||||
t.Fatalf("expected singleflight to coalesce; calls=%d", got)
|
||||
}
|
||||
}
|
||||
|
||||
func TestTTLCache_MaxSizeEviction(t *testing.T) {
|
||||
cache := newTTLCache()
|
||||
cache.maxSize = 3
|
||||
for i := 0; i < 5; i++ {
|
||||
cache.set(fmt.Sprintf("k%d", i), i)
|
||||
}
|
||||
if len(cache.entries) > cache.maxSize {
|
||||
t.Fatalf("cache exceeded maxSize: %d", len(cache.entries))
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user