From 2e601ed6b71cf2c9ca8977649281567a6440adb0 Mon Sep 17 00:00:00 2001 From: Dallin Romney Date: Fri, 1 May 2026 11:12:20 +0800 Subject: [PATCH] feat(github): show total page count in pagination logs Parses rel="last" from the Link header, and for the cursor-paginated issues listing falls back to open_issues_count from the prior GetRepo response. Logs read "page 3/7 fetched count=100 accumulated=300" when a total is known and remain "page 3 ..." otherwise. --- internal/github/client.go | 58 ++++++++++++++++++----- internal/github/client_test.go | 85 +++++++++++++++++++++++++++++++++- internal/syncer/syncer.go | 21 +++++++-- internal/syncer/syncer_test.go | 27 +++++++++++ 4 files changed, 175 insertions(+), 16 deletions(-) diff --git a/internal/github/client.go b/internal/github/client.go index cacbc5e..6e35455 100644 --- a/internal/github/client.go +++ b/internal/github/client.go @@ -31,9 +31,10 @@ type Options struct { } type ListIssuesOptions struct { - State string - Since string - Limit int + State string + Since string + Limit int + ExpectedTotal int } type RequestError struct { @@ -113,33 +114,37 @@ func (c *Client) ListRepositoryIssues(ctx context.Context, owner, repo string, o values.Set("since", options.Since) } path := fmt.Sprintf("/repos/%s/%s/issues?%s", pathEscape(owner), pathEscape(repo), values.Encode()) - return c.paginate(ctx, path, options.Limit, reporter) + return c.paginate(ctx, path, options.Limit, options.ExpectedTotal, reporter) } func (c *Client) ListIssueComments(ctx context.Context, owner, repo string, number int, reporter Reporter) ([]map[string]any, error) { path := fmt.Sprintf("/repos/%s/%s/issues/%d/comments?per_page=100", pathEscape(owner), pathEscape(repo), number) - return c.paginate(ctx, path, 0, reporter) + return c.paginate(ctx, path, 0, 0, reporter) } func (c *Client) ListPullReviews(ctx context.Context, owner, repo string, number int, reporter Reporter) ([]map[string]any, error) { path := fmt.Sprintf("/repos/%s/%s/pulls/%d/reviews?per_page=100", pathEscape(owner), pathEscape(repo), number) - return c.paginate(ctx, path, 0, reporter) + return c.paginate(ctx, path, 0, 0, reporter) } func (c *Client) ListPullReviewComments(ctx context.Context, owner, repo string, number int, reporter Reporter) ([]map[string]any, error) { path := fmt.Sprintf("/repos/%s/%s/pulls/%d/comments?per_page=100", pathEscape(owner), pathEscape(repo), number) - return c.paginate(ctx, path, 0, reporter) + return c.paginate(ctx, path, 0, 0, reporter) } func (c *Client) ListPullFiles(ctx context.Context, owner, repo string, number int, reporter Reporter) ([]map[string]any, error) { path := fmt.Sprintf("/repos/%s/%s/pulls/%d/files?per_page=100", pathEscape(owner), pathEscape(repo), number) - return c.paginate(ctx, path, 0, reporter) + return c.paginate(ctx, path, 0, 0, reporter) } -func (c *Client) paginate(ctx context.Context, firstPath string, limit int, reporter Reporter) ([]map[string]any, error) { +func (c *Client) paginate(ctx context.Context, firstPath string, limit int, expectedItems int, reporter Reporter) ([]map[string]any, error) { var out []map[string]any nextPath := firstPath page := 0 + totalPages := 0 + if expectedItems > 0 { + totalPages = (expectedItems + 99) / 100 + } for nextPath != "" { page++ var rows []map[string]any @@ -156,11 +161,22 @@ func (c *Client) paginate(ctx context.Context, firstPath string, limit int, repo rows = rows[:limit-len(out)] } out = append(out, rows...) - reporter.Printf("[github] page %d fetched count=%d accumulated=%d", page, len(rows), len(out)) + linkHeader := resp.Header.Get("Link") + if last := lastPage(linkHeader); last > totalPages { + totalPages = last + } + if totalPages > 0 && page > totalPages { + totalPages = page + } + if totalPages > 0 { + reporter.Printf("[github] page %d/%d fetched count=%d accumulated=%d", page, totalPages, len(rows), len(out)) + } else { + reporter.Printf("[github] page %d fetched count=%d accumulated=%d", page, len(rows), len(out)) + } if limit > 0 && len(out) >= limit { break } - nextPath = nextPage(resp.Header.Get("Link")) + nextPath = nextPage(linkHeader) if nextPath != "" && c.pageDelay > 0 { select { case <-ctx.Done(): @@ -231,6 +247,26 @@ func nextPage(linkHeader string) string { return "" } +func lastPage(linkHeader string) int { + for _, part := range strings.Split(linkHeader, ",") { + sections := strings.Split(part, ";") + if len(sections) < 2 { + continue + } + if strings.TrimSpace(sections[1]) != `rel="last"` { + continue + } + rawURL := strings.Trim(strings.TrimSpace(sections[0]), "<>") + parsed, err := url.Parse(rawURL) + if err != nil { + return 0 + } + page, _ := strconv.Atoi(parsed.Query().Get("page")) + return page + } + return 0 +} + func pathEscape(value string) string { return url.PathEscape(value) } diff --git a/internal/github/client_test.go b/internal/github/client_test.go index 5824ae5..0f65c8f 100644 --- a/internal/github/client_test.go +++ b/internal/github/client_test.go @@ -16,7 +16,7 @@ func TestListRepositoryIssuesPaginatesAndLimits(t *testing.T) { } switch r.URL.Query().Get("page") { case "": - w.Header().Set("Link", `<`+serverURL(r)+`?page=2>; rel="next"`) + w.Header().Set("Link", `<`+serverURL(r)+`?page=2>; rel="next", <`+serverURL(r)+`?page=2>; rel="last"`) _ = json.NewEncoder(w).Encode([]map[string]any{{"number": 1}, {"number": 2}}) case "2": _ = json.NewEncoder(w).Encode([]map[string]any{{"number": 3}}) @@ -26,8 +26,10 @@ func TestListRepositoryIssuesPaginatesAndLimits(t *testing.T) { })) defer server.Close() + var messages []string + reporter := Reporter(func(message string) { messages = append(messages, message) }) client := New(Options{Token: "token", BaseURL: server.URL, PageDelay: -1}) - rows, err := client.ListRepositoryIssues(context.Background(), "openclaw", "gitcrawl", ListIssuesOptions{Limit: 3}, nil) + rows, err := client.ListRepositoryIssues(context.Background(), "openclaw", "gitcrawl", ListIssuesOptions{Limit: 3}, reporter) if err != nil { t.Fatalf("list issues: %v", err) } @@ -37,6 +39,76 @@ func TestListRepositoryIssuesPaginatesAndLimits(t *testing.T) { if intValue(rows[2]["number"]) != 3 { t.Fatalf("last number: %#v", rows[2]["number"]) } + joined := strings.Join(messages, "\n") + if !strings.Contains(joined, "page 1/2 fetched") || !strings.Contains(joined, "page 2/2 fetched") { + t.Fatalf("expected page X/Y log lines, got:\n%s", joined) + } +} + +func TestListRepositoryIssuesUsesExpectedTotalWhenNoLastLink(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Query().Get("page") { + case "": + // Cursor-style pagination: only "next", no "last". + w.Header().Set("Link", `<`+serverURL(r)+`?page=2>; rel="next"`) + rows := make([]map[string]any, 100) + for i := range rows { + rows[i] = map[string]any{"number": i + 1} + } + _ = json.NewEncoder(w).Encode(rows) + case "2": + rows := make([]map[string]any, 50) + for i := range rows { + rows[i] = map[string]any{"number": 100 + i + 1} + } + _ = json.NewEncoder(w).Encode(rows) + default: + t.Fatalf("unexpected page: %s", r.URL.RawQuery) + } + })) + defer server.Close() + + var messages []string + reporter := Reporter(func(message string) { messages = append(messages, message) }) + client := New(Options{BaseURL: server.URL, PageDelay: -1}) + rows, err := client.ListRepositoryIssues(context.Background(), "openclaw", "gitcrawl", ListIssuesOptions{ExpectedTotal: 150}, reporter) + if err != nil { + t.Fatalf("list issues: %v", err) + } + if len(rows) != 150 { + t.Fatalf("rows: got %d want 150", len(rows)) + } + joined := strings.Join(messages, "\n") + if !strings.Contains(joined, "page 1/2 fetched") || !strings.Contains(joined, "page 2/2 fetched") { + t.Fatalf("expected page X/Y log lines from hint, got:\n%s", joined) + } +} + +func TestPaginateRaisesTotalWhenActualExceedsHint(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Query().Get("page") { + case "": + w.Header().Set("Link", `<`+serverURL(r)+`?page=2>; rel="next"`) + _ = json.NewEncoder(w).Encode([]map[string]any{{"number": 1}}) + case "2": + _ = json.NewEncoder(w).Encode([]map[string]any{{"number": 2}}) + default: + t.Fatalf("unexpected page: %s", r.URL.RawQuery) + } + })) + defer server.Close() + + var messages []string + reporter := Reporter(func(message string) { messages = append(messages, message) }) + client := New(Options{BaseURL: server.URL, PageDelay: -1}) + // Hint underestimates (1 page) but the API actually returns 2. + if _, err := client.ListRepositoryIssues(context.Background(), "o", "r", ListIssuesOptions{ExpectedTotal: 1}, reporter); err != nil { + t.Fatalf("list: %v", err) + } + joined := strings.Join(messages, "\n") + if !strings.Contains(joined, "page 2/2 fetched") { + t.Fatalf("expected total to be raised to actual page count, got:\n%s", joined) + } } func TestRequestErrorIncludesStatus(t *testing.T) { @@ -127,6 +199,15 @@ func TestNextPageAndReporterBranches(t *testing.T) { if got := nextPage(`; rel="last"`); got != "" { t.Fatalf("bad next page = %q", got) } + if got := lastPage(header); got != 9 { + t.Fatalf("last page = %d", got) + } + if got := lastPage(`; rel="next"`); got != 0 { + t.Fatalf("last page without rel=last = %d", got) + } + if got := lastPage(`<%zz>; rel="last"`); got != 0 { + t.Fatalf("last page bad url = %d", got) + } var messages []string Reporter(func(message string) { messages = append(messages, message) }).Printf("hello %d", 1) if len(messages) != 1 || messages[0] != "hello 1" { diff --git a/internal/syncer/syncer.go b/internal/syncer/syncer.go index aa7d056..5cf54e2 100644 --- a/internal/syncer/syncer.go +++ b/internal/syncer/syncer.go @@ -103,9 +103,10 @@ func (s *Syncer) Sync(ctx context.Context, options Options) (Stats, error) { } else { var err error rows, err = s.client.ListRepositoryIssues(ctx, options.Owner, options.Repo, gh.ListIssuesOptions{ - State: state, - Since: since, - Limit: options.Limit, + State: state, + Since: since, + Limit: options.Limit, + ExpectedTotal: expectedIssueTotal(repoRaw, state, since, options.Limit), }, options.Reporter) if err != nil { return Stats{}, err @@ -435,6 +436,20 @@ func jsonID(value any) string { } } +func expectedIssueTotal(repoRaw map[string]any, state, since string, limit int) int { + if state != "open" || since != "" { + return 0 + } + count := intValue(repoRaw["open_issues_count"]) + if count <= 0 { + return 0 + } + if limit > 0 && limit < count { + return limit + } + return count +} + func intValue(value any) int { switch typed := value.(type) { case float64: diff --git a/internal/syncer/syncer_test.go b/internal/syncer/syncer_test.go index 89cb692..30af37a 100644 --- a/internal/syncer/syncer_test.go +++ b/internal/syncer/syncer_test.go @@ -455,6 +455,33 @@ func TestSyncOpenSinceAppliesClosedOverlapSweep(t *testing.T) { } } +func TestExpectedIssueTotal(t *testing.T) { + cases := []struct { + name string + repo map[string]any + state string + since string + limit int + want int + }{ + {name: "open no filters", repo: map[string]any{"open_issues_count": float64(666)}, state: "open", want: 666}, + {name: "open with limit below count", repo: map[string]any{"open_issues_count": float64(666)}, state: "open", limit: 100, want: 100}, + {name: "open with limit above count", repo: map[string]any{"open_issues_count": float64(50)}, state: "open", limit: 200, want: 50}, + {name: "open with since", repo: map[string]any{"open_issues_count": float64(666)}, state: "open", since: "2026-04-26T00:00:00Z", want: 0}, + {name: "closed state", repo: map[string]any{"open_issues_count": float64(666)}, state: "closed", want: 0}, + {name: "all state", repo: map[string]any{"open_issues_count": float64(666)}, state: "all", want: 0}, + {name: "missing count", repo: map[string]any{}, state: "open", want: 0}, + {name: "zero count", repo: map[string]any{"open_issues_count": float64(0)}, state: "open", want: 0}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if got := expectedIssueTotal(tc.repo, tc.state, tc.since, tc.limit); got != tc.want { + t.Fatalf("expectedIssueTotal = %d, want %d", got, tc.want) + } + }) + } +} + func TestMappingHelperBranches(t *testing.T) { if got := jsonID("abc"); got != "abc" { t.Fatalf("string json id = %q", got)