fix: count all unresolved review threads (#2240)

This commit is contained in:
José Almeida
2026-08-04 08:46:38 +01:00
committed by GitHub
parent 2ee2fa8c66
commit 257fffdcc1
6 changed files with 1115 additions and 121 deletions
+308 -113
View File
@@ -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]
+415 -8
View File
@@ -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: `{
@@ -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()
@@ -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.
@@ -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.
+29
View File
@@ -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.