From 257fffdcc184de4b33e0181054777115ee15bf09 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Almeida?= <57680069+zeval@users.noreply.github.com> Date: Tue, 4 Aug 2026 08:46:38 +0100 Subject: [PATCH] fix: count all unresolved review threads (#2240) --- apps/backend/internal/github/graphql.go | 421 ++++++++++++----- apps/backend/internal/github/graphql_test.go | 423 +++++++++++++++++- apps/backend/internal/github/poller_test.go | 74 +++ .../plan.md | 165 +++++++ .../task-01-paginate-review-threads.md | 124 +++++ docs/specs/ui/ci-pr-automation.md | 29 ++ 6 files changed, 1115 insertions(+), 121 deletions(-) create mode 100644 docs/plans/ci-popover-review-thread-pagination/plan.md create mode 100644 docs/plans/ci-popover-review-thread-pagination/task-01-paginate-review-threads.md diff --git a/apps/backend/internal/github/graphql.go b/apps/backend/internal/github/graphql.go index 2f05793e5e..6935b4ff84 100644 --- a/apps/backend/internal/github/graphql.go +++ b/apps/backend/internal/github/graphql.go @@ -25,14 +25,19 @@ type GraphQLExecutor interface { const graphQLPRBatchAlias = "pr" // graphQLBatchChunkSize bounds the number of aliased fields per request to -// stay well under GitHub's per-query node-count limit (500). 50 PRs × ~10 -// fields = ~500 nodes, leaving headroom. +// keep initial status queries and their decoded response size manageable. const graphQLBatchChunkSize = 50 +// Each continuation can return 100 review-thread nodes. Five continuations +// keep a follow-up query near 500 connection nodes while preserving batching. +const graphQLReviewThreadContinuationChunkSize = 5 + // graphQLBranchProbeLimit fetches two matches so branch lookup can detect // ambiguous fork heads instead of linking the first arbitrary PR. const graphQLBranchProbeLimit = 2 +const graphQLReviewThreadPageSize = 100 + // reviewNode is one PR review entry from the batched GraphQL query. type reviewNode struct { State string `json:"state"` @@ -49,6 +54,85 @@ type graphQLPRRef struct { Number int } +type graphQLPRRepoGroup struct { + Owner string + Repo string + Refs []graphQLPRRef +} + +func groupGraphQLPRRefs(refs []graphQLPRRef) []graphQLPRRepoGroup { + byKey := make(map[string]*graphQLPRRepoGroup) + keys := make([]string, 0) + for _, ref := range refs { + key := ref.Owner + "/" + ref.Repo + group, ok := byKey[key] + if !ok { + group = &graphQLPRRepoGroup{Owner: ref.Owner, Repo: ref.Repo} + byKey[key] = group + keys = append(keys, key) + } + group.Refs = append(group.Refs, ref) + } + sort.Strings(keys) + + groups := make([]graphQLPRRepoGroup, 0, len(keys)) + for _, key := range keys { + groups = append(groups, *byKey[key]) + } + return groups +} + +type reviewThreadNode struct { + IsResolved bool `json:"isResolved"` +} + +type graphQLPageInfo struct { + HasNextPage bool `json:"hasNextPage"` + EndCursor string `json:"endCursor"` +} + +type reviewThreadConnection struct { + TotalCount int `json:"totalCount"` + Nodes []reviewThreadNode `json:"nodes"` + PageInfo graphQLPageInfo `json:"pageInfo"` +} + +type reviewThreadContinuation struct { + Ref graphQLPRRef + Cursor string + SeenCursors map[string]struct{} + RemainingPages int + Status *PRStatus +} + +func newReviewThreadContinuation( + ref graphQLPRRef, + connection reviewThreadConnection, + status *PRStatus, +) (reviewThreadContinuation, error) { + cursor := connection.PageInfo.EndCursor + if cursor == "" { + return reviewThreadContinuation{}, fmt.Errorf( + "review thread pagination for %s/%s#%d returned an empty cursor", + ref.Owner, ref.Repo, ref.Number, + ) + } + remainingThreads := connection.TotalCount - len(connection.Nodes) + if remainingThreads <= 0 { + return reviewThreadContinuation{}, fmt.Errorf( + "review thread pagination for %s/%s#%d exceeded totalCount %d", + ref.Owner, ref.Repo, ref.Number, connection.TotalCount, + ) + } + return reviewThreadContinuation{ + Ref: ref, + Cursor: cursor, + SeenCursors: map[string]struct{}{cursor: {}}, + RemainingPages: (remainingThreads + graphQLReviewThreadPageSize - 1) / graphQLReviewThreadPageSize, + Status: status, + }, nil +} + // graphQLBranchRef is one entry in a batched branch-lookup request. type graphQLBranchRef struct { Owner string @@ -59,12 +143,16 @@ type graphQLBranchRef struct { // chunkedRefs splits refs into chunks of at most graphQLBatchChunkSize so // callers can keep individual GraphQL queries under the node-count limit. func chunkedRefs[T any](refs []T) [][]T { + return chunkRefs(refs, graphQLBatchChunkSize) +} + +func chunkRefs[T any](refs []T, chunkSize int) [][]T { if len(refs) == 0 { return nil } - out := make([][]T, 0, (len(refs)+graphQLBatchChunkSize-1)/graphQLBatchChunkSize) - for i := 0; i < len(refs); i += graphQLBatchChunkSize { - end := i + graphQLBatchChunkSize + out := make([][]T, 0, (len(refs)+chunkSize-1)/chunkSize) + for i := 0; i < len(refs); i += chunkSize { + end := i + chunkSize if end > len(refs) { end = len(refs) } @@ -99,13 +187,8 @@ type batchedPRResult struct { ReviewRequests struct { TotalCount int `json:"totalCount"` } `json:"reviewRequests"` - ReviewThreads struct { - TotalCount int `json:"totalCount"` - Nodes []struct { - IsResolved bool `json:"isResolved"` - } `json:"nodes"` - } `json:"reviewThreads"` - Commits struct { + ReviewThreads reviewThreadConnection `json:"reviewThreads"` + Commits struct { Nodes []struct { Commit struct { StatusCheckRollup *struct { @@ -218,19 +301,10 @@ func classifyBatchedErrors(errs []graphQLError, aliasToRepo map[string]repoRef) // resolution-failure error's path[0] is always the outer repository // alias ("repo0", "repo1", ...). func aliasMapForPRRefs(refs []graphQLPRRef) map[string]repoRef { - byKey := map[string]repoRef{} - var keys []string - for _, r := range refs { - k := r.Owner + "/" + r.Repo - if _, ok := byKey[k]; !ok { - byKey[k] = repoRef{Owner: r.Owner, Repo: r.Repo} - keys = append(keys, k) - } - } - sort.Strings(keys) - out := make(map[string]repoRef, len(keys)) - for i, k := range keys { - out[fmt.Sprintf("repo%d", i)] = byKey[k] + groups := groupGraphQLPRRefs(refs) + out := make(map[string]repoRef, len(groups)) + for i, group := range groups { + out[fmt.Sprintf("repo%d", i)] = repoRef{Owner: group.Owner, Repo: group.Repo} } return out } @@ -276,34 +350,13 @@ type graphQLRateLimit struct { // The shape mirrors gh pr view fields used by the existing converter so we // can reuse the conversion logic without reshaping callers. func buildBatchedPRQuery(refs []graphQLPRRef) (string, map[string]any) { - // Group refs by (owner, repo) and assign deterministic indices so aliases - // stay stable across runs (helpful for tests and snapshot debugging). - type group struct { - owner string - repo string - numbers []int - } - byKey := map[string]*group{} - keys := []string{} - for _, r := range refs { - key := r.Owner + "/" + r.Repo - g, ok := byKey[key] - if !ok { - g = &group{owner: r.Owner, repo: r.Repo} - byKey[key] = g - keys = append(keys, key) - } - g.numbers = append(g.numbers, r.Number) - } - sort.Strings(keys) - var b strings.Builder b.WriteString("query Batch { ") - for repoIdx, key := range keys { - g := byKey[key] - fmt.Fprintf(&b, `repo%d: repository(owner: %q, name: %q) { `, repoIdx, g.owner, g.repo) - for prIdx, n := range g.numbers { - fmt.Fprintf(&b, `%s%d: pullRequest(number: %d) { %s } `, graphQLPRBatchAlias, prIdx, n, prFieldsBlock()) + for repoIdx, group := range groupGraphQLPRRefs(refs) { + fmt.Fprintf(&b, `repo%d: repository(owner: %q, name: %q) { `, repoIdx, group.Owner, group.Repo) + for prIdx, ref := range group.Refs { + fmt.Fprintf(&b, `%s%d: pullRequest(number: %d) { %s } `, + graphQLPRBatchAlias, prIdx, ref.Number, prFieldsBlock()) } b.WriteString(`} `) } @@ -321,7 +374,8 @@ func prFieldsBlock() string { `author { login } createdAt updatedAt mergedAt closedAt ` + `reviews(last: 100) { nodes { state author { login } submittedAt } } ` + `reviewRequests(first: 0) { totalCount } ` + - `reviewThreads(first: 100) { totalCount nodes { isResolved } } ` + + fmt.Sprintf(`reviewThreads(first: %d) { totalCount nodes { isResolved } pageInfo { hasNextPage endCursor } } `, + graphQLReviewThreadPageSize) + `commits(last: 1) { nodes { commit { statusCheckRollup { state } } } }` } @@ -374,23 +428,12 @@ func convertBatchedPRResult(raw *batchedPRResult, owner, repo string, number int if len(raw.Commits.Nodes) > 0 && raw.Commits.Nodes[0].Commit.StatusCheckRollup != nil { checksState = strings.ToLower(raw.Commits.Nodes[0].Commit.StatusCheckRollup.State) } - // Count unresolved threads from the fetched nodes. When the page is - // capped (totalCount > nodes), fall back to totalCount for the - // resolved-vs-unresolved estimate so the popover doesn't undercount on - // busy PRs. The fallback is conservative: we have no way to tell from - // the truncated page how many of the un-fetched threads are resolved, - // so attribute the unseen tail to "unresolved" — the popover's value - // is meant to be actionable, not exact, and over-reporting is safer - // than silently hiding open feedback. unresolved := 0 for _, t := range raw.ReviewThreads.Nodes { if !t.IsResolved { unresolved++ } } - if total := raw.ReviewThreads.TotalCount; total > len(raw.ReviewThreads.Nodes) { - unresolved += total - len(raw.ReviewThreads.Nodes) - } return &PRStatus{ PR: pr, ReviewState: reviewState, @@ -541,6 +584,7 @@ func runBatchedPRQuery(ctx context.Context, exec GraphQLExecutor, refs []graphQL // repos landed in the negative cache. var allMissing []repoRef var allResidual []graphQLError + var continuations []reviewThreadContinuation for _, chunk := range chunkedRefs(refs) { query, vars := buildBatchedPRQuery(chunk) var resp struct { @@ -557,67 +601,210 @@ func runBatchedPRQuery(ctx context.Context, exec GraphQLExecutor, refs []graphQL // partial results for the ones that resolved; non-resolution errors // still bubble up so the caller falls back to per-watch checks. missing, residual := classifyBatchedErrors(resp.Errors, aliasMapForPRRefs(chunk)) - if err := decodeBatchedPRChunk(chunk, resp.Data, result); err != nil { + chunkContinuations, err := decodeBatchedPRChunk(chunk, resp.Data, result) + if err != nil { return nil, err } + continuations = append(continuations, chunkContinuations...) allMissing = append(allMissing, missing...) allResidual = append(allResidual, residual...) } - if err := wrapBatchedErrors(allMissing, allResidual); err != nil { + return finishBatchedQuery(ctx, exec, result, allMissing, allResidual, continuations) +} + +func finishBatchedQuery( + ctx context.Context, + exec GraphQLExecutor, + result map[string]*PRStatus, + missing []repoRef, + residual []graphQLError, + continuations []reviewThreadContinuation, +) (map[string]*PRStatus, error) { + batchErr := wrapBatchedErrors(missing, residual) + if batchErr != nil && (len(missing) == 0 || len(residual) > 0) { + return nil, batchErr + } + if err := completeReviewThreadContinuations(ctx, exec, continuations); err != nil { + if len(missing) > 0 { + return nil, &batchedMissingReposErr{Repos: missing, Inner: err} + } + return nil, err + } + if batchErr != nil { // When the error is purely "missing repos" across all chunks, // partial Statuses for the other refs are still in `result`; // surface them to the caller alongside the typed error so it // can populate the negative cache without dropping the good data. - if len(allMissing) > 0 && len(allResidual) == 0 { - return result, err - } - return nil, err + return result, batchErr } return result, nil } +func buildReviewThreadPageQuery(continuations []reviewThreadContinuation) string { + var b strings.Builder + b.WriteString("query ReviewThreadPages { ") + for i, continuation := range continuations { + fmt.Fprintf(&b, + `repo%d: repository(owner: %q, name: %q) { pr0: pullRequest(number: %d) { reviewThreads(first: %d, after: %q) { nodes { isResolved } pageInfo { hasNextPage endCursor } } } } `, + i, continuation.Ref.Owner, continuation.Ref.Repo, continuation.Ref.Number, + graphQLReviewThreadPageSize, continuation.Cursor, + ) + } + b.WriteString(`rateLimit { limit remaining resetAt cost } }`) + return b.String() +} + +func completeReviewThreadContinuations( + ctx context.Context, + exec GraphQLExecutor, + continuations []reviewThreadContinuation, +) error { + for len(continuations) > 0 { + next := make([]reviewThreadContinuation, 0, len(continuations)) + for _, chunk := range chunkRefs(continuations, graphQLReviewThreadContinuationChunkSize) { + var resp struct { + Data map[string]json.RawMessage `json:"data"` + Errors []graphQLError `json:"errors"` + } + if err := exec.ExecuteGraphQL(ctx, buildReviewThreadPageQuery(chunk), nil, &resp); err != nil { + return err + } + if err := graphQLErrorsToErr(resp.Errors); err != nil { + return err + } + chunkNext, err := applyReviewThreadPageChunk(chunk, resp.Data) + if err != nil { + return err + } + next = append(next, chunkNext...) + } + continuations = next + } + return nil +} + +func applyReviewThreadPageChunk( + continuations []reviewThreadContinuation, + data map[string]json.RawMessage, +) ([]reviewThreadContinuation, error) { + next := make([]reviewThreadContinuation, 0, len(continuations)) + for i, continuation := range continuations { + page, err := decodeReviewThreadPage(i, data) + if err != nil { + return nil, err + } + continuation, hasNext, err := advanceReviewThreadContinuation(continuation, page) + if err != nil { + return nil, err + } + if hasNext { + next = append(next, continuation) + } + } + return next, nil +} + +func decodeReviewThreadPage(index int, data map[string]json.RawMessage) (reviewThreadConnection, error) { + repoAlias := fmt.Sprintf("repo%d", index) + rawRepo, ok := data[repoAlias] + if !ok || isNullGraphQLValue(rawRepo) { + return reviewThreadConnection{}, fmt.Errorf("review thread response missing repository alias %s", repoAlias) + } + var repoBlock map[string]json.RawMessage + if err := json.Unmarshal(rawRepo, &repoBlock); err != nil { + return reviewThreadConnection{}, fmt.Errorf("decode review thread repo alias %s: %w", repoAlias, err) + } + rawPR, ok := repoBlock["pr0"] + if !ok || isNullGraphQLValue(rawPR) { + return reviewThreadConnection{}, fmt.Errorf("review thread response missing PR alias %s.pr0", repoAlias) + } + var rawPage struct { + ReviewThreads json.RawMessage `json:"reviewThreads"` + } + if err := json.Unmarshal(rawPR, &rawPage); err != nil { + return reviewThreadConnection{}, fmt.Errorf("decode review thread PR alias %s.pr0: %w", repoAlias, err) + } + if isNullGraphQLValue(rawPage.ReviewThreads) { + return reviewThreadConnection{}, fmt.Errorf( + "review thread response missing connection %s.pr0.reviewThreads", + repoAlias, + ) + } + var page reviewThreadConnection + if err := json.Unmarshal(rawPage.ReviewThreads, &page); err != nil { + return reviewThreadConnection{}, fmt.Errorf("decode review thread connection %s.pr0: %w", repoAlias, err) + } + return page, nil +} + +func advanceReviewThreadContinuation( + continuation reviewThreadContinuation, + page reviewThreadConnection, +) (reviewThreadContinuation, bool, error) { + if continuation.RemainingPages <= 0 { + return continuation, false, reviewThreadPageLimitError(continuation.Ref) + } + continuation.RemainingPages-- + for _, thread := range page.Nodes { + if !thread.IsResolved { + continuation.Status.UnresolvedReviewThreads++ + } + } + if !page.PageInfo.HasNextPage { + return continuation, false, nil + } + if continuation.RemainingPages == 0 { + return continuation, false, reviewThreadPageLimitError(continuation.Ref) + } + nextCursor := page.PageInfo.EndCursor + if nextCursor == "" { + return continuation, false, fmt.Errorf( + "review thread pagination for %s/%s#%d returned an empty cursor", + continuation.Ref.Owner, continuation.Ref.Repo, continuation.Ref.Number, + ) + } + if _, seen := continuation.SeenCursors[nextCursor]; seen { + return continuation, false, fmt.Errorf( + "review thread pagination for %s/%s#%d repeated cursor %q", + continuation.Ref.Owner, continuation.Ref.Repo, continuation.Ref.Number, nextCursor, + ) + } + continuation.SeenCursors[nextCursor] = struct{}{} + continuation.Cursor = nextCursor + return continuation, true, nil +} + +func reviewThreadPageLimitError(ref graphQLPRRef) error { + return fmt.Errorf( + "review thread pagination for %s/%s#%d exceeded its page limit", + ref.Owner, ref.Repo, ref.Number, + ) +} + +func isNullGraphQLValue(raw json.RawMessage) bool { + trimmed := bytes.TrimSpace(raw) + return len(trimmed) == 0 || bytes.Equal(trimmed, []byte("null")) +} + // decodeBatchedPRChunk maps the aliased response back to the input refs and // fills in the result map. Refs whose alias is missing or null are skipped // (e.g. PR was deleted upstream); the next poller tick will retry. -func decodeBatchedPRChunk(refs []graphQLPRRef, data map[string]json.RawMessage, result map[string]*PRStatus) error { - type group struct { - idx int - owner string - repo string - prRefs []graphQLPRRef - } - groups := []*group{} - byKey := map[string]*group{} - for _, r := range refs { - k := r.Owner + "/" + r.Repo - g, ok := byKey[k] - if !ok { - g = &group{idx: len(groups), owner: r.Owner, repo: r.Repo} - byKey[k] = g - groups = append(groups, g) - } - g.prRefs = append(g.prRefs, r) - } - keys := make([]string, 0, len(byKey)) - for k := range byKey { - keys = append(keys, k) - } - sort.Strings(keys) - // Reassign group indices in sorted order to match buildBatchedPRQuery. - for i, k := range keys { - byKey[k].idx = i - } - for _, k := range keys { - g := byKey[k] - raw, ok := data[fmt.Sprintf("repo%d", g.idx)] +func decodeBatchedPRChunk( + refs []graphQLPRRef, + data map[string]json.RawMessage, + result map[string]*PRStatus, +) ([]reviewThreadContinuation, error) { + continuations := make([]reviewThreadContinuation, 0) + for repoIdx, group := range groupGraphQLPRRefs(refs) { + raw, ok := data[fmt.Sprintf("repo%d", repoIdx)] if !ok || len(raw) == 0 { continue } repoBlock := map[string]json.RawMessage{} if err := json.Unmarshal(raw, &repoBlock); err != nil { - return fmt.Errorf("decode repo block: %w", err) + return nil, fmt.Errorf("decode repo block: %w", err) } - for prIdx, ref := range g.prRefs { + for prIdx, ref := range group.Refs { alias := fmt.Sprintf("%s%d", graphQLPRBatchAlias, prIdx) rawPR, ok := repoBlock[alias] if !ok || len(rawPR) == 0 || string(rawPR) == "null" { @@ -625,12 +812,22 @@ func decodeBatchedPRChunk(refs []graphQLPRRef, data map[string]json.RawMessage, } var raw batchedPRResult if err := json.Unmarshal(rawPR, &raw); err != nil { - return fmt.Errorf("decode pr alias %s: %w", alias, err) + return nil, fmt.Errorf("decode pr alias %s: %w", alias, err) + } + status := convertBatchedPRResult(&raw, ref.Owner, ref.Repo, ref.Number) + result[prStatusCacheKey(ref.Owner, ref.Repo, ref.Number)] = status + if raw.ReviewThreads.PageInfo.HasNextPage { + continuation, err := newReviewThreadContinuation( + ref, raw.ReviewThreads, status, + ) + if err != nil { + return nil, err + } + continuations = append(continuations, continuation) } - result[prStatusCacheKey(ref.Owner, ref.Repo, ref.Number)] = convertBatchedPRResult(&raw, ref.Owner, ref.Repo, ref.Number) } } - return nil + return continuations, nil } // runBatchedBranchQuery executes the branch-lookup query in chunks and maps @@ -661,16 +858,14 @@ func runBatchedBranchQuery(ctx context.Context, exec GraphQLExecutor, refs []gra allMissing = append(allMissing, missing...) allResidual = append(allResidual, residual...) } - if err := wrapBatchedErrors(allMissing, allResidual); err != nil { - if len(allMissing) > 0 && len(allResidual) == 0 { - return result, err - } - return nil, err - } - return result, nil + return finishBatchedQuery(ctx, exec, result, allMissing, allResidual, nil) } -func decodeBatchedBranchChunk(refs []graphQLBranchRef, data map[string]json.RawMessage, result map[string]*PRStatus) error { +func decodeBatchedBranchChunk( + refs []graphQLBranchRef, + data map[string]json.RawMessage, + result map[string]*PRStatus, +) error { for i, ref := range refs { alias := fmt.Sprintf("b%d", i) raw, ok := data[alias] diff --git a/apps/backend/internal/github/graphql_test.go b/apps/backend/internal/github/graphql_test.go index 2e3407c6a2..0e805bedf4 100644 --- a/apps/backend/internal/github/graphql_test.go +++ b/apps/backend/internal/github/graphql_test.go @@ -4,6 +4,7 @@ import ( "context" "encoding/json" "errors" + "fmt" "strings" "testing" "time" @@ -12,9 +13,10 @@ import ( // stubGraphQLExecutor lets tests inspect/return canned responses without // touching HTTP or the gh CLI. type stubGraphQLExecutor struct { - queries []string - response string - err error + queries []string + response string + responses []string + err error } func (s *stubGraphQLExecutor) ExecuteGraphQL(_ context.Context, query string, _ map[string]any, out any) error { @@ -22,7 +24,123 @@ func (s *stubGraphQLExecutor) ExecuteGraphQL(_ context.Context, query string, _ if s.err != nil { return s.err } - return json.Unmarshal([]byte(s.response), out) + response := s.response + if len(s.responses) > 0 { + response = s.responses[0] + s.responses = s.responses[1:] + } + return json.Unmarshal([]byte(response), out) +} + +func resolvedReviewThreadNodes(count int) []map[string]bool { + nodes := make([]map[string]bool, count) + for i := range nodes { + nodes[i] = map[string]bool{"isResolved": true} + } + return nodes +} + +func mustJSON(t *testing.T, value any) string { + t.Helper() + data, err := json.Marshal(value) + if err != nil { + t.Fatalf("marshal GraphQL fixture: %v", err) + } + return string(data) +} + +func reviewThreadsFixture( + totalCount int, + nodes []map[string]bool, + hasNextPage bool, + endCursor string, +) map[string]any { + return map[string]any{ + "totalCount": totalCount, + "nodes": nodes, + "pageInfo": map[string]any{"hasNextPage": hasNextPage, "endCursor": endCursor}, + } +} + +func batchedPRFixture( + title string, + url string, + totalCount int, + nodes []map[string]bool, + hasNextPage bool, + endCursor string, +) map[string]any { + return map[string]any{ + "state": "OPEN", "title": title, "url": url, + "headRefName": "feat", "baseRefName": "main", "headRefOid": "abc", + "author": map[string]any{"login": "alice"}, + "createdAt": "2026-01-01T00:00:00Z", "updatedAt": "2026-01-02T00:00:00Z", + "reviews": map[string]any{"nodes": []any{}}, + "reviewRequests": map[string]any{"totalCount": 0}, + "reviewThreads": reviewThreadsFixture(totalCount, nodes, hasNextPage, endCursor), + "commits": map[string]any{"nodes": []any{}}, + } +} + +func batchedPRReviewThreadsResponse( + t *testing.T, + totalCount int, + nodes []map[string]bool, + hasNextPage bool, + endCursor string, +) string { + t.Helper() + return mustJSON(t, map[string]any{ + "data": map[string]any{ + "repo0": map[string]any{ + "pr0": batchedPRFixture("Busy PR", "https://x/42", totalCount, nodes, hasNextPage, endCursor), + }, + }, + }) +} + +func reviewThreadPageResponse( + t *testing.T, + nodes []map[string]bool, + hasNextPage bool, + endCursor string, +) string { + t.Helper() + return reviewThreadPagesResponse(t, reviewThreadsFixture(len(nodes), nodes, hasNextPage, endCursor)) +} + +func reviewThreadPagesResponse(t *testing.T, pages ...map[string]any) string { + t.Helper() + data := make(map[string]any, len(pages)) + for i, page := range pages { + data[fmt.Sprintf("repo%d", i)] = map[string]any{ + "pr0": map[string]any{"reviewThreads": page}, + } + } + return mustJSON(t, map[string]any{ + "data": data, + }) +} + +func batchedBranchReviewThreadsResponse( + t *testing.T, + totalCount int, + nodes []map[string]bool, + hasNextPage bool, + endCursor string, +) string { + t.Helper() + node := batchedPRFixture("Busy branch PR", "https://x/7", totalCount, nodes, hasNextPage, endCursor) + node["number"] = 7 + return mustJSON(t, map[string]any{ + "data": map[string]any{ + "b0": map[string]any{ + "pullRequests": map[string]any{ + "nodes": []any{node}, + }, + }, + }, + }) } func TestBuildBatchedPRQuery_GroupsByRepo(t *testing.T) { @@ -102,11 +220,281 @@ func TestRunBatchedPRQuery_DecodesAliasesBackToRefs(t *testing.T) { } } +func TestRunBatchedPRQuery_PaginatesResolvedReviewThreads(t *testing.T) { + exec := &stubGraphQLExecutor{responses: []string{ + batchedPRReviewThreadsResponse(t, 102, resolvedReviewThreadNodes(100), true, "cursor-1"), + reviewThreadPageResponse(t, resolvedReviewThreadNodes(2), false, "cursor-2"), + }} + + got, err := runBatchedPRQuery(context.Background(), exec, []graphQLPRRef{ + {Owner: "o", Repo: "r", Number: 42}, + }) + if err != nil { + t.Fatalf("run: %v", err) + } + + status := got[prStatusCacheKey("o", "r", 42)] + if status.UnresolvedReviewThreads != 0 { + t.Fatalf("UnresolvedReviewThreads = %d, want 0", status.UnresolvedReviewThreads) + } +} + +func TestRunBatchedPRQuery_BatchesReviewThreadContinuations(t *testing.T) { + exec := &stubGraphQLExecutor{responses: []string{ + mustJSON(t, map[string]any{ + "data": map[string]any{ + "repo0": map[string]any{ + "pr0": batchedPRFixture("PR A", "https://x/a/1", 101, resolvedReviewThreadNodes(100), true, "cursor-a"), + }, + "repo1": map[string]any{ + "pr0": batchedPRFixture("PR B", "https://x/b/2", 101, resolvedReviewThreadNodes(100), true, "cursor-b"), + }, + }, + }), + reviewThreadPagesResponse(t, + reviewThreadsFixture(1, []map[string]bool{{"isResolved": false}}, false, "cursor-a2"), + reviewThreadsFixture(1, resolvedReviewThreadNodes(1), false, "cursor-b2"), + ), + }} + + got, err := runBatchedPRQuery(context.Background(), exec, []graphQLPRRef{ + {Owner: "o", Repo: "a", Number: 1}, + {Owner: "o", Repo: "b", Number: 2}, + }) + if err != nil { + t.Fatalf("run: %v", err) + } + if len(exec.queries) != 2 { + t.Fatalf("GraphQL query count = %d, want 2", len(exec.queries)) + } + if !strings.Contains(exec.queries[0], `pageInfo { hasNextPage endCursor }`) { + t.Errorf("initial query does not request review-thread pageInfo: %s", exec.queries[0]) + } + if !strings.Contains(exec.queries[1], `query ReviewThreadPages`) || + !strings.Contains(exec.queries[1], `after: "cursor-a"`) || + !strings.Contains(exec.queries[1], `after: "cursor-b"`) { + t.Errorf("follow-up query does not batch both cursors: %s", exec.queries[1]) + } + if got[prStatusCacheKey("o", "a", 1)].UnresolvedReviewThreads != 1 { + t.Errorf("o/a#1 unresolved count = %d, want 1", got[prStatusCacheKey("o", "a", 1)].UnresolvedReviewThreads) + } + if got[prStatusCacheKey("o", "b", 2)].UnresolvedReviewThreads != 0 { + t.Errorf("o/b#2 unresolved count = %d, want 0", got[prStatusCacheKey("o", "b", 2)].UnresolvedReviewThreads) + } +} + +func TestRunBatchedPRQuery_BoundsReviewThreadContinuationBatchSize(t *testing.T) { + initialData := make(map[string]any, 6) + refs := make([]graphQLPRRef, 6) + pages := make([]map[string]any, 6) + for i := range refs { + repo := fmt.Sprintf("repo-%d", i) + initialData[fmt.Sprintf("repo%d", i)] = map[string]any{ + "pr0": batchedPRFixture( + fmt.Sprintf("PR %d", i), fmt.Sprintf("https://x/%d", i), + 101, resolvedReviewThreadNodes(100), true, fmt.Sprintf("cursor-%d", i), + ), + } + refs[i] = graphQLPRRef{Owner: "o", Repo: repo, Number: i + 1} + pages[i] = reviewThreadsFixture(1, resolvedReviewThreadNodes(1), false, "done") + } + + exec := &stubGraphQLExecutor{responses: []string{ + mustJSON(t, map[string]any{"data": initialData}), + reviewThreadPagesResponse(t, pages...), + reviewThreadPageResponse(t, resolvedReviewThreadNodes(1), false, "done"), + }} + + if _, err := runBatchedPRQuery(context.Background(), exec, refs); err != nil { + t.Fatalf("run: %v", err) + } + if len(exec.queries) != 3 { + t.Fatalf("GraphQL query count = %d, want 3", len(exec.queries)) + } +} + +func TestRunBatchedPRQuery_OneReviewThreadPageSkipsContinuation(t *testing.T) { + exec := &stubGraphQLExecutor{ + response: batchedPRReviewThreadsResponse( + t, 2, []map[string]bool{{"isResolved": true}, {"isResolved": false}}, false, "cursor-1", + ), + } + + got, err := runBatchedPRQuery(context.Background(), exec, []graphQLPRRef{ + {Owner: "o", Repo: "r", Number: 42}, + }) + if err != nil { + t.Fatalf("run: %v", err) + } + if len(exec.queries) != 1 { + t.Fatalf("GraphQL query count = %d, want 1", len(exec.queries)) + } + if got[prStatusCacheKey("o", "r", 42)].UnresolvedReviewThreads != 1 { + t.Fatalf("UnresolvedReviewThreads = %d, want 1", got[prStatusCacheKey("o", "r", 42)].UnresolvedReviewThreads) + } +} + +func TestRunBatchedPRQuery_RejectsRepeatedReviewThreadCursor(t *testing.T) { + exec := &stubGraphQLExecutor{responses: []string{ + batchedPRReviewThreadsResponse(t, 201, resolvedReviewThreadNodes(100), true, "cursor-1"), + reviewThreadPageResponse(t, resolvedReviewThreadNodes(1), true, "cursor-1"), + reviewThreadPageResponse(t, resolvedReviewThreadNodes(1), false, "cursor-2"), + }} + + _, err := runBatchedPRQuery(context.Background(), exec, []graphQLPRRef{ + {Owner: "o", Repo: "r", Number: 42}, + }) + if err == nil { + t.Fatal("expected repeated review-thread cursor to fail") + } + if !strings.Contains(err.Error(), "repeated cursor") { + t.Fatalf("error = %q, want repeated-cursor failure", err) + } +} + +func TestRunBatchedPRQuery_RejectsPagesBeyondInitialTotalCount(t *testing.T) { + exec := &stubGraphQLExecutor{responses: []string{ + batchedPRReviewThreadsResponse(t, 101, resolvedReviewThreadNodes(100), true, "cursor-1"), + reviewThreadPageResponse(t, resolvedReviewThreadNodes(1), true, "cursor-2"), + reviewThreadPageResponse(t, resolvedReviewThreadNodes(1), false, "cursor-3"), + }} + + _, err := runBatchedPRQuery(context.Background(), exec, []graphQLPRRef{ + {Owner: "o", Repo: "r", Number: 42}, + }) + if err == nil { + t.Fatal("expected review-thread pages beyond initial totalCount to fail") + } + if !strings.Contains(err.Error(), "page limit") { + t.Fatalf("error = %q, want page-limit failure", err) + } +} + +func TestRunBatchedPRQuery_PreservesMissingReposOnReviewThreadPaginationFailure(t *testing.T) { + exec := &stubGraphQLExecutor{responses: []string{ + mustJSON(t, map[string]any{ + "data": map[string]any{ + "repo1": map[string]any{ + "pr0": batchedPRFixture( + "Busy PR", "https://x/live/42", 101, + resolvedReviewThreadNodes(100), true, "cursor-1", + ), + }, + }, + "errors": []map[string]any{{ + "message": "Could not resolve to a Repository with the name 'o/dead'.", + "type": "NOT_FOUND", + "path": []string{"repo0"}, + }}, + }), + `{"data": {}}`, + }} + + got, err := runBatchedPRQuery(context.Background(), exec, []graphQLPRRef{ + {Owner: "o", Repo: "dead", Number: 1}, + {Owner: "o", Repo: "live", Number: 42}, + }) + if got != nil { + t.Fatalf("status map = %#v, want nil after incomplete pagination", got) + } + var missingErr *batchedMissingReposErr + if !errors.As(err, &missingErr) { + t.Fatalf("error = %v, want batchedMissingReposErr", err) + } + if len(missingErr.Repos) != 1 || missingErr.Repos[0] != (repoRef{Owner: "o", Repo: "dead"}) { + t.Fatalf("missing repos = %#v, want o/dead", missingErr.Repos) + } + if missingErr.Inner == nil || !strings.Contains(missingErr.Inner.Error(), "missing repository alias") { + t.Fatalf("inner error = %v, want pagination failure", missingErr.Inner) + } +} + +func TestRunBatchedPRQuery_RejectsEmptyReviewThreadCursor(t *testing.T) { + tests := []struct { + name string + responses []string + }{ + { + name: "initial page", + responses: []string{ + batchedPRReviewThreadsResponse(t, 101, resolvedReviewThreadNodes(100), true, ""), + }, + }, + { + name: "continuation page", + responses: []string{ + batchedPRReviewThreadsResponse(t, 201, resolvedReviewThreadNodes(100), true, "cursor-1"), + reviewThreadPageResponse(t, resolvedReviewThreadNodes(100), true, ""), + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + exec := &stubGraphQLExecutor{responses: tt.responses} + got, err := runBatchedPRQuery(context.Background(), exec, []graphQLPRRef{ + {Owner: "o", Repo: "r", Number: 42}, + }) + if err == nil || got != nil { + t.Fatalf("got (%v, %v), want (nil, error)", got, err) + } + if !strings.Contains(err.Error(), "empty cursor") { + t.Fatalf("error = %q, want empty-cursor failure", err) + } + }) + } +} + +func TestRunBatchedPRQuery_PaginatesMultipleReviewThreadRounds(t *testing.T) { + secondPage := append( + []map[string]bool{{"isResolved": false}}, + resolvedReviewThreadNodes(99)..., + ) + exec := &stubGraphQLExecutor{responses: []string{ + batchedPRReviewThreadsResponse(t, 202, resolvedReviewThreadNodes(100), true, "cursor-1"), + reviewThreadPageResponse(t, secondPage, true, "cursor-2"), + reviewThreadPageResponse(t, []map[string]bool{ + {"isResolved": false}, + {"isResolved": true}, + }, false, "cursor-3"), + }} + + got, err := runBatchedPRQuery(context.Background(), exec, []graphQLPRRef{ + {Owner: "o", Repo: "r", Number: 42}, + }) + if err != nil { + t.Fatalf("run: %v", err) + } + if len(exec.queries) != 3 { + t.Fatalf("GraphQL query count = %d, want 3", len(exec.queries)) + } + if got[prStatusCacheKey("o", "r", 42)].UnresolvedReviewThreads != 2 { + t.Fatalf("UnresolvedReviewThreads = %d, want 2", got[prStatusCacheKey("o", "r", 42)].UnresolvedReviewThreads) + } +} + +func TestRunBatchedPRQuery_RejectsNullReviewThreadPage(t *testing.T) { + exec := &stubGraphQLExecutor{responses: []string{ + batchedPRReviewThreadsResponse(t, 101, resolvedReviewThreadNodes(100), true, "cursor-1"), + mustJSON(t, map[string]any{ + "data": map[string]any{ + "repo0": map[string]any{ + "pr0": map[string]any{"reviewThreads": nil}, + }, + }, + }), + }} + + _, err := runBatchedPRQuery(context.Background(), exec, []graphQLPRRef{ + {Owner: "o", Repo: "r", Number: 42}, + }) + if err == nil { + t.Fatal("expected null review-thread page to fail") + } +} + func TestRunBatchedPRQuery_DecodesMultipleReposIntoCorrectKeys(t *testing.T) { - // Two repos in one batch: octo/alpha#1 and octo/beta#9. The decoder sorts - // repo group keys; this test guards against future drift between - // buildBatchedPRQuery's sort and decodeBatchedPRChunk's sort by checking - // each PR lands in its correct key slot. + // Two repos in one batch: check each aliased PR lands in its correct key. exec := &stubGraphQLExecutor{ response: `{ "data": { @@ -231,6 +619,25 @@ func TestRunBatchedBranchQuery_DecodesPRNode(t *testing.T) { } } +func TestRunBatchedBranchQuery_SkipsUnusedReviewThreadPagination(t *testing.T) { + exec := &stubGraphQLExecutor{response: batchedBranchReviewThreadsResponse(t, 101, resolvedReviewThreadNodes(100), true, "cursor-1")} + + got, err := runBatchedBranchQuery(context.Background(), exec, []graphQLBranchRef{ + {Owner: "o", Repo: "r", Branch: "feat"}, + }) + if err != nil { + t.Fatalf("run: %v", err) + } + + status := got[graphqlBranchKey("o", "r", "feat")] + if status == nil || status.PR == nil || status.PR.Number != 7 { + t.Fatalf("branch status = %#v, want discovered PR #7", status) + } + if len(exec.queries) != 1 { + t.Fatalf("GraphQL query count = %d, want 1", len(exec.queries)) + } +} + func TestRunBatchedBranchQuery_EmptyNodesReturnsNoResult(t *testing.T) { exec := &stubGraphQLExecutor{ response: `{ diff --git a/apps/backend/internal/github/poller_test.go b/apps/backend/internal/github/poller_test.go index ce2cdcf804..3f477bfde5 100644 --- a/apps/backend/internal/github/poller_test.go +++ b/apps/backend/internal/github/poller_test.go @@ -645,6 +645,80 @@ func TestTryBatchedPRWatchCheck_NumberedWatch_AppliesStatus(t *testing.T) { } } +func TestTryBatchedPRWatchCheck_PersistsExactReviewThreadCount(t *testing.T) { + poller, _, gh, store := setupBatchedPollerTest(t) + ctx := context.Background() + + gh.prResponses = []string{ + batchedPRReviewThreadsResponse(t, 102, resolvedReviewThreadNodes(100), true, "cursor-1"), + reviewThreadPageResponse(t, resolvedReviewThreadNodes(2), false, "cursor-2"), + } + + watch := &PRWatch{ + SessionID: "s1", TaskID: "t1", Owner: "o", Repo: "r", PRNumber: 42, Branch: "feat", + } + if err := store.CreatePRWatch(ctx, withTestWorkspace(watch)); err != nil { + t.Fatalf("create PR watch: %v", err) + } + if err := store.CreateTaskPR(ctx, &TaskPR{ + TaskID: "t1", Owner: "o", Repo: "r", PRNumber: 42, + PRURL: "https://x/42", PRTitle: "Busy PR", HeadBranch: "feat", BaseBranch: "main", State: "open", + UnresolvedReviewThreads: 154, + }); err != nil { + t.Fatalf("create task PR: %v", err) + } + + if !poller.tryBatchedPRWatchCheck(ctx, []*PRWatch{watch}) { + t.Fatal("expected batched path to succeed") + } + + updated, err := store.GetTaskPR(ctx, "t1") + if err != nil || updated == nil { + t.Fatalf("get task PR: err=%v, pr=%v", err, updated) + } + if updated.UnresolvedReviewThreads != 0 { + t.Fatalf("UnresolvedReviewThreads = %d, want 0", updated.UnresolvedReviewThreads) + } +} + +func TestTryBatchedPRWatchCheck_PreservesReviewThreadCountOnPaginationFailure(t *testing.T) { + poller, _, gh, store := setupBatchedPollerTest(t) + ctx := context.Background() + + gh.prResponses = []string{ + batchedPRReviewThreadsResponse(t, 101, resolvedReviewThreadNodes(100), true, "cursor-1"), + mustJSON(t, map[string]any{ + "data": map[string]any{}, + "errors": []map[string]any{{"message": "review thread page failed"}}, + }), + } + + watch := &PRWatch{ + SessionID: "s1", TaskID: "t1", Owner: "o", Repo: "r", PRNumber: 42, Branch: "feat", + } + if err := store.CreatePRWatch(ctx, withTestWorkspace(watch)); err != nil { + t.Fatalf("create PR watch: %v", err) + } + if err := store.CreateTaskPR(ctx, &TaskPR{ + TaskID: "t1", Owner: "o", Repo: "r", PRNumber: 42, + PRURL: "https://x/42", PRTitle: "Busy PR", HeadBranch: "feat", BaseBranch: "main", State: "open", + UnresolvedReviewThreads: 154, + }); err != nil { + t.Fatalf("create task PR: %v", err) + } + + if poller.tryBatchedPRWatchCheck(ctx, []*PRWatch{watch}) { + t.Fatal("expected batched path to fail") + } + updated, err := store.GetTaskPR(ctx, "t1") + if err != nil || updated == nil { + t.Fatalf("get task PR: err=%v, pr=%v", err, updated) + } + if updated.UnresolvedReviewThreads != 154 { + t.Fatalf("UnresolvedReviewThreads = %d, want preserved value 154", updated.UnresolvedReviewThreads) + } +} + func TestTryBatchedPRWatchCheck_PublishesOnPRFeedbackWatermarkChange(t *testing.T) { poller, _, gh, store := setupBatchedPollerTest(t) ctx := context.Background() diff --git a/docs/plans/ci-popover-review-thread-pagination/plan.md b/docs/plans/ci-popover-review-thread-pagination/plan.md new file mode 100644 index 0000000000..8d537a7f64 --- /dev/null +++ b/docs/plans/ci-popover-review-thread-pagination/plan.md @@ -0,0 +1,165 @@ +--- +spec: docs/specs/ui/ci-pr-automation.md +created: 2026-08-04 +status: complete +--- + +# Implementation Plan: Exact CI Popover Review-Thread Counts + +## Overview + +The lightweight GitHub GraphQL poll currently requests only the first 100 +review threads, then treats every omitted thread as unresolved. This produces +false counts on busy PRs and can incorrectly trigger auto-fix or block +auto-merge. Keep the normal batched query unchanged for PRs that fit in one +page, paginate only truncated review-thread connections, and persist a count +only after every page completes. + +--- + +## Backend + +### GraphQL response contract + +In `apps/backend/internal/github/graphql.go`: + +- Extend the internal `reviewThreads` response shape with + `pageInfo { hasNextPage endCursor }` and request those fields from + `prFieldsBlock`. +- Count only fetched nodes whose `isResolved` value is false. Remove the + `totalCount - len(nodes)` fallback; `totalCount` cannot reveal resolution + state. +- Represent an incomplete connection as an internal continuation containing + PR identity, the destination `PRStatus`, and the next cursor. No public + `PRStatus`, `TaskPR`, database, HTTP, or WebSocket shape changes. + +### Follow-up pagination + +In `apps/backend/internal/github/graphql.go`: + +- Have the numbered-PR decoder collect continuations when a review-thread + connection has another page. Branch lookup only discovers PR metadata and + must not fail because an unused review-thread continuation fails; the next + numbered-watch sync owns the complete count. +- Execute follow-up review-thread page queries in bounded batched rounds, + reusing `GraphQLExecutor` with a dedicated five-PR continuation chunk size. + Each round adds only the unresolved nodes it actually received and advances + through GitHub's cursor until `hasNextPage` is false. +- Include the top-level rate-limit selection in follow-up queries so existing + PAT and `gh` executors continue recording GraphQL quota. +- Reject an absent, empty, or repeated continuation cursor, pagination beyond + the initial `totalCount` page budget, a missing/null response alias, a decode + failure, or any top-level GraphQL error. Return no partially completed status + map on such failure, allowing the existing batched-query fallback to retain + the previous complete count. +- Preserve existing partial-success behavior for repositories that are + independently classified as missing. If another repository's continuation + later fails, retain the typed missing-repository error around that failure so + the caller still populates its negative cache while rejecting all partial + statuses. Issue no follow-up request when all selected PRs fit in their first + page. + +### Persistence and automation + +No new persistence code is required. `Service.SyncTaskPR` already stores +`PRStatus.UnresolvedReviewThreads` only when +`UnresolvedReviewThreadsPopulated` is true, and the CI popover plus automation +read that stored field. Successful pagination therefore replaces a fabricated +count with the exact count; failed pagination must not produce a populated +partial result. + +--- + +## Frontend + +No frontend implementation changes. The existing popover hides +`pr-comments-row` when `unresolved_review_threads` is zero and already has +desktop E2E coverage for that behavior. + +--- + +## Tests + +- **What:** A PR with more than 100 fully resolved threads reports zero, while + unresolved threads on later pages contribute exactly once. + **File:** `apps/backend/internal/github/graphql_test.go`. + **How:** Drive `runBatchedPRQuery` through a sequenced `GraphQLExecutor` and + assert the initial query, continuation query, query count, and final + `UnresolvedReviewThreads` value. +- **What:** Branch discovery returns a selected PR without fetching unused + review-thread continuation pages. + **File:** `apps/backend/internal/github/graphql_test.go`. + **How:** Return a selected branch PR whose review threads have another page, + provide no continuation response, and assert the PR is still discovered with + one GraphQL query. +- **What:** A continuation error or invalid cursor never returns a partial + status map. + **File:** `apps/backend/internal/github/graphql_test.go`. + **How:** Fail the sequenced executor after the first page and assert an error + plus no consumable partial result. +- **What:** A continuation failure does not hide repositories already + classified as unresolvable by the initial batch. + **File:** `apps/backend/internal/github/graphql_test.go`. + **How:** Return a missing-repository GraphQL error beside a busy PR, fail its + continuation, and assert the typed missing-repository wrapper retains the + pagination failure while the status map remains nil. +- **What:** The exact multi-page count reaches the persisted `TaskPR` used by + the popover and automation. + **File:** `apps/backend/internal/github/poller_test.go`. + **How:** Use `setupBatchedPollerTest`, seed a numbered watch and prior nonzero + count, return fully resolved pages, run the real batched poll/apply path, and + assert the stored count becomes zero. + +--- + +## E2E Tests + +- **Scenario:** GIVEN the backend reports zero unresolved review threads, + WHEN the user opens the CI popover, THEN no unresolved-comments row appears. +- **File:** Existing + `apps/web/e2e/tests/pr/pr-topbar-popover.spec.ts` scenario + `comments row hidden when unresolved_review_threads = 0`. +- **What to verify:** Rerun the existing Chromium scenario. No E2E source + change is expected because the defect is in the backend count producer. + +--- + +## Verification Results + +- Exact review-thread pagination is implemented for numbered PRs. Follow-up + pages remain batched, cursor cycles and incomplete responses fail closed, and + no partial count reaches persistence. Branch discovery remains independent + of unused review-thread continuation pages. +- Focused regression tests, the full GitHub backend package, scoped Go lint, + and the existing Chromium popover scenario all pass. Exact commands and + timings are recorded in the task file. + +--- + +## Implementation Waves And Parallel Candidates + +Sequential: + +- [x] [task-01-paginate-review-threads](task-01-paginate-review-threads.md) + +No parallel candidate: query code, unit coverage, persistence coverage, and +the final status field form one tightly coupled backend change. + +--- + +## Risks + +- A malformed or non-advancing GitHub cursor could otherwise loop forever; + cursor validation must fail the sync instead. +- Follow-up pages add GraphQL work only for PRs over 100 review threads. The + implementation must retain batching so a workspace with several busy PRs + does not regress to one subprocess per PR. +- Review-thread state can change while pages are fetched. This fix follows + GitHub connection cursors and accepts the completed traversal as the poll's + snapshot, matching existing eventual-consistency behavior. + +## Out of Scope + +- Renaming the existing UI label from comments to threads. +- Changing PR-watch terminal reset behavior. +- Adding a new persisted completeness column or changing public API payloads. diff --git a/docs/plans/ci-popover-review-thread-pagination/task-01-paginate-review-threads.md b/docs/plans/ci-popover-review-thread-pagination/task-01-paginate-review-threads.md new file mode 100644 index 0000000000..a8b8ae6443 --- /dev/null +++ b/docs/plans/ci-popover-review-thread-pagination/task-01-paginate-review-threads.md @@ -0,0 +1,124 @@ +--- +id: "01-paginate-review-threads" +title: "Paginate review threads" +status: done +wave: 1 +depends_on: [] +plan: "plan.md" +spec: "../../specs/ui/ci-pr-automation.md" +--- + +# Task 01: Paginate review threads + +Implement complete GitHub review-thread pagination for lightweight numbered-PR +GraphQL queries, keep branch discovery independent from unused continuation +pages, then prove the exact count reaches the persisted task PR used by the CI +popover and automation. + +## Acceptance + +1. PRs with more than 100 fully resolved review threads persist zero + unresolved threads; unresolved nodes on later pages are counted exactly. +2. PRs fitting on one page make no follow-up request. Numbered PRs with + continuations use bounded batched page queries; branch discovery returns PR + metadata without depending on unused continuation pages. +3. Missing, invalid, or failed continuation data returns no partial populated + count, preserving the prior complete stored value through existing fallback + behavior. Any missing-repository classifications from the initial batch + remain available to the existing negative cache. + +## Files likely touched + +- `apps/backend/internal/github/graphql.go` +- `apps/backend/internal/github/graphql_test.go` +- `apps/backend/internal/github/poller_test.go` +- `docs/specs/ui/ci-pr-automation.md` +- `docs/plans/ci-popover-review-thread-pagination/plan.md` +- `docs/plans/ci-popover-review-thread-pagination/task-01-paginate-review-threads.md` + +The existing `apps/web/e2e/tests/pr/pr-topbar-popover.spec.ts` is verification +input and should not need modification. + +## Dependencies + +None. + +## Parallelism + +`sequential`. Query orchestration and persistence coverage share the GitHub +test executors and status contract. + +## Inputs + +- `docs/specs/ui/ci-pr-automation.md` requirements for complete counts and + partial-page failure behavior. +- `docs/plans/ci-popover-review-thread-pagination/plan.md` backend design, + risks, and out-of-scope boundaries. +- Existing batching/error patterns in + `apps/backend/internal/github/graphql.go` and + `apps/backend/internal/github/service_pr_watch_batched.go`. + +## Verification + +```bash +cd apps/backend && go test ./internal/github -run 'TestBuildBatchedPRQuery_GroupsByRepo|TestRunBatched(PR|Branch)Query_.*ReviewThread|TestTryBatchedPRWatchCheck_.*ReviewThreadCount' -count=1 -v +cd apps/backend && go test ./internal/github -count=1 +cd apps/backend && golangci-lint run ./internal/github/... +cd apps && pnpm install --frozen-lockfile +cd apps/web && pnpm e2e:run --project chromium tests/pr/pr-topbar-popover.spec.ts -- --grep 'comments row hidden when unresolved_review_threads = 0' +git diff --check +``` + +## Output contract + +Report the cursor/query design, every changed file, exact test and E2E outcomes, +any external GraphQL behavior assumption, cleanup evidence, and blockers or +remaining risks. Mark this task `done`, check it in `plan.md`, and synchronize +`plan.md` verification results only after all commands pass. + +## Results + +- Cursor/query design: the initial batched query requests + `pageInfo { hasNextPage endCursor }`; only connections with another page + become continuations for numbered PRs. Continuations for up to five PRs share + each follow-up query round. Each status counts only returned unresolved + nodes. Empty or repeated cursors, pages beyond the initial `totalCount` + budget, missing/null aliases, decode failures, executor failures, and + top-level GraphQL errors return no status map. +- Branch discovery intentionally skips unused review-thread continuation pages; + after association, the next numbered-watch sync owns the complete count. +- Shared owner/repository grouping now drives initial query aliases, error + aliases, and response decoding. This keeps aliases deterministic and brought + the decoder under the repository complexity limit. +- TDD red evidence: the old logic reported `2` instead of `0` for 102 fully + resolved threads and accepted repeated-cursor and null-page responses. Each + regression turned green after pagination and validation were implemented. +- PR review remediation added a `totalCount`-derived page budget, dedicated + five-PR continuation chunks, empty-cursor and three-page coverage, a precise + null-connection fixture, and a branch-discovery regression. The duplicated + numbered/branch completion tail now shares one helper, and page decoding and + advancement are split into focused helpers to stay within complexity limits. +- A later Codex regression proved that a continuation failure previously hid a + missing-repository classification from the negative cache. The typed missing + error now wraps the pagination failure while returning no partial statuses. +- TDD review-remediation evidence: the combined missing-repository and + continuation-failure test first returned only the pagination error, then + passed after preserving the typed wrapper. +- Focused final regression command passed all pagination-budget, cursor, + multi-round, null-connection, continuation-batching, and branch-discovery + cases in `0.184s`. +- The focused missing-repository preservation regression passed in `0.016s`. +- Full backend package: `go test ./internal/github -count=1` passed in + `13.265s`. +- Scoped lint: `golangci-lint run ./internal/github/...` reported `0 issues`. +- Workspace install: lockfile unchanged; 1,143 packages reused/installed in + `4.8s`. +- Fresh-build Chromium E2E: the existing + `comments row hidden when unresolved_review_threads = 0` scenario passed; + `1 passed (10.3s)` after the production backend and Vite builds. +- External assumption: GitHub's `pageInfo` and opaque cursors define a complete + traversal. Concurrent thread changes are accepted as eventual consistency, + matching the existing poll model. +- Cleanup: the managed E2E runner exited successfully and removed its temporary + fixture workspace. `git diff --check` passes; only the intended source, + regression-test, spec, and plan files remain changed. diff --git a/docs/specs/ui/ci-pr-automation.md b/docs/specs/ui/ci-pr-automation.md index 1183990118..b209751946 100644 --- a/docs/specs/ui/ci-pr-automation.md +++ b/docs/specs/ui/ci-pr-automation.md @@ -136,6 +136,20 @@ Decision: [ADR-0051](../../decisions/0051-pr-agent-notifications-extend-task-pr- every linked PR. Dedupe, last-attempt, review-request, and terminal state are tracked per linked PR. - Kandev checks watched PRs through the existing lightweight PR watch poller, which runs once per minute. Automation wakeups sync the latest lightweight PR state before evaluating gates. When auto-fix is enabled, Kandev fetches full PR feedback so failing checks, requested changes, unresolved threads, and human PR conversation comments can trigger deduped prompts even when the persisted lightweight row was stale. Auto-fix waits until all PR checks have finished before sending or queueing a prompt, so the agent receives the final check set and current comments in one pass. Bot-authored PR conversation comments without failed checks or unresolved review threads are treated as non-actionable status chatter and do not send an agent prompt. +- Lightweight PR status sync counts unresolved review threads across every page + returned by GitHub. A connection's `totalCount` indicates that more threads + exist; it never classifies omitted threads as unresolved. The CI popover, + auto-fix eligibility, and auto-merge readiness consume only a complete + review-thread count. +- If Kandev cannot finish review-thread pagination, it discards the partial + count and follows the existing PR-status sync failure path. A partial page + never replaces the last complete persisted count or becomes fresh automation + input. If the initial batch also identified unresolvable repositories, those + classifications still reach the existing negative cache even when another + repository's continuation fails. +- Branch-only PR discovery associates PR metadata without fetching unused + review-thread continuation pages. Once the watch has a PR number, the next + numbered status sync produces the complete review-thread count. - Saving PR automation options while any option is enabled immediately evaluates the task's current linked PRs instead of waiting for the next PR watch poll. Prompt edits do not reset unchanged checkpoints. @@ -430,6 +444,21 @@ Auto-merge cycle for one task/PR: ## Scenarios - **GIVEN** a task with one open linked PR, **WHEN** the user opens the CI popover above the chat input, **THEN** the popover shows the current CI/review summary and all five automation controls. +- **GIVEN** a linked PR with more than 100 review threads and every thread is + resolved, **WHEN** Kandev completes its lightweight PR status sync, **THEN** + the CI popover shows no unresolved-review row and automation evaluates an + unresolved-thread count of zero. +- **GIVEN** a linked PR whose unresolved review threads span more than one + GitHub page, **WHEN** Kandev completes its lightweight PR status sync, + **THEN** the persisted and displayed count equals the unresolved threads + across all pages. +- **GIVEN** Kandev has a complete persisted review-thread count and GitHub + fails a later pagination request, **WHEN** the lightweight PR status sync + runs, **THEN** no partial or inferred count replaces that complete value. +- **GIVEN** one repository is unresolvable and another repository's + review-thread continuation fails in the same batch, **WHEN** lightweight PR + status sync handles the failure, **THEN** it discards all partial statuses + while still negative-caching the unresolvable repository. - **GIVEN** a user is viewing the CI popover automation controls, **WHEN** they activate the info icon, **THEN** they see help text explaining that Kandev uses the existing 1-minute PR watch checks, fetches full feedback only for candidate PRs, snapshots each auto-fix round, and merges only when readiness gates pass. - **GIVEN** a task with one open linked PR, **WHEN** the user enables `Auto-fix CI & address comments`, **THEN** the setting persists and remains enabled after page reload. - **GIVEN** a task with one open linked PR, **WHEN** the user enables `Auto-merge when ready`, **THEN** the setting persists and remains enabled after page reload.