From 22b3ce8fefaa957e10fa7f5c458a0f9e6e675ca9 Mon Sep 17 00:00:00 2001 From: claw Date: Wed, 13 May 2026 15:35:55 -0700 Subject: [PATCH] fix(github): consolidate review.go and identity.go into reviews.go (#116) Remove github/review.go and github/identity.go, replacing them with a consolidated github/reviews.go that: - Uses doJSONRequest for PostReview and DismissReview (cleaner than manual marshal + doRequestWithBody) - Adds paginated ListReviews with per_page=100 and max 100 pages - Consolidates GetAuthenticatedUser and userResponse type (previously duplicated in identity.go) - Preserves all sentinel errors (ErrCannotDeleteSubmittedReview, ErrConflictingCommitIDs), state translation, commit ID validation, and SupersedeReviews This prevents the redeclaration errors that occur when both review.go and reviews.go exist in the same package, as described in issue #116. Closes #116 --- github/identity.go | 29 --------- github/{review.go => reviews.go} | 101 +++++++++++++++++++++---------- 2 files changed, 68 insertions(+), 62 deletions(-) delete mode 100644 github/identity.go rename github/{review.go => reviews.go} (77%) diff --git a/github/identity.go b/github/identity.go deleted file mode 100644 index 46c26a8..0000000 --- a/github/identity.go +++ /dev/null @@ -1,29 +0,0 @@ -package github - -import ( - "context" - "encoding/json" - "fmt" -) - -// userResponse is the GitHub API response for the authenticated user. -type userResponse struct { - Login string `json:"login"` -} - -// GetAuthenticatedUser returns the login of the currently authenticated user. -func (c *Client) GetAuthenticatedUser(ctx context.Context) (string, error) { - reqURL := fmt.Sprintf("%s/user", c.baseURL) - - body, err := c.doGet(ctx, reqURL) - if err != nil { - return "", fmt.Errorf("get authenticated user: %w", err) - } - - var resp userResponse - if err := json.Unmarshal(body, &resp); err != nil { - return "", fmt.Errorf("parse user response: %w", err) - } - - return resp.Login, nil -} diff --git a/github/review.go b/github/reviews.go similarity index 77% rename from github/review.go rename to github/reviews.go index 1d043e5..6a129bd 100644 --- a/github/review.go +++ b/github/reviews.go @@ -5,12 +5,21 @@ import ( "encoding/json" "errors" "fmt" + "log/slog" "net/http" "net/url" "gitea.weiker.me/rodin/review-bot/vcs" ) +const ( + // reviewsPerPage is the number of reviews to fetch per API page. + reviewsPerPage = 100 + // maxReviewPages is the maximum number of pages to paginate through + // when listing reviews. Acts as a safeguard against infinite pagination. + maxReviewPages = 100 +) + // ErrCannotDeleteSubmittedReview is returned when DeleteReview is called on // a review that has already been submitted (APPROVED, REQUEST_CHANGES, COMMENT). // GitHub only allows deletion of PENDING reviews. Callers that need to replace @@ -54,6 +63,11 @@ type dismissReviewRequest struct { Event string `json:"event"` } +// userResponse is the GitHub API response for the authenticated user. +type userResponse struct { + Login string `json:"login"` +} + // translateGitHubReviewState translates a GitHub API review state to the // canonical vcs.Review.State value. func translateGitHubReviewState(state string) string { @@ -117,12 +131,7 @@ func (c *Client) PostReview(ctx context.Context, owner, repo string, number int, }) } - data, err := json.Marshal(payload) - if err != nil { - return nil, fmt.Errorf("marshal review request: %w", err) - } - - body, err := c.doRequestWithBody(ctx, http.MethodPost, reqURL, data) + body, err := c.doJSONRequest(ctx, http.MethodPost, reqURL, payload) if err != nil { return nil, fmt.Errorf("post review: %w", err) } @@ -141,33 +150,51 @@ func (c *Client) PostReview(ctx context.Context, owner, repo string, number int, }, nil } -// ListReviews retrieves all reviews for a pull request. +// ListReviews retrieves all reviews for a pull request with pagination. // GitHub review states are translated to canonical vcs values. func (c *Client) ListReviews(ctx context.Context, owner, repo string, number int) ([]vcs.Review, error) { - reqURL := fmt.Sprintf("%s/repos/%s/%s/pulls/%d/reviews", - c.baseURL, url.PathEscape(owner), url.PathEscape(repo), number) + var allReviews []vcs.Review - body, err := c.doGet(ctx, reqURL) - if err != nil { - return nil, fmt.Errorf("list reviews: %w", err) - } + for page := 1; page <= maxReviewPages; page++ { + reqURL := fmt.Sprintf("%s/repos/%s/%s/pulls/%d/reviews?per_page=%d&page=%d", + c.baseURL, url.PathEscape(owner), url.PathEscape(repo), number, reviewsPerPage, page) - var responses []reviewResponse - if err := json.Unmarshal(body, &responses); err != nil { - return nil, fmt.Errorf("parse reviews response: %w", err) - } + body, err := c.doGet(ctx, reqURL) + if err != nil { + return nil, fmt.Errorf("list reviews page %d: %w", page, err) + } - reviews := make([]vcs.Review, len(responses)) - for i, r := range responses { - reviews[i] = vcs.Review{ - ID: r.ID, - Body: r.Body, - User: vcs.UserInfo{Login: r.User.Login}, - State: translateGitHubReviewState(r.State), - CommitID: r.CommitID, + var responses []reviewResponse + if err := json.Unmarshal(body, &responses); err != nil { + return nil, fmt.Errorf("parse reviews response: %w", err) + } + + if len(responses) == 0 { + break + } + + for _, r := range responses { + allReviews = append(allReviews, vcs.Review{ + ID: r.ID, + Body: r.Body, + User: vcs.UserInfo{Login: r.User.Login}, + State: translateGitHubReviewState(r.State), + CommitID: r.CommitID, + }) + } + + if len(responses) < reviewsPerPage { + break + } + + if page == maxReviewPages { + slog.Warn("ListReviews hit page limit; results may be truncated", + "owner", owner, "repo", repo, "pr", number, + "maxPages", maxReviewPages, "reviewsFetched", len(allReviews)) } } - return reviews, nil + + return allReviews, nil } // DeleteReview deletes a pull request review. @@ -178,7 +205,6 @@ func (c *Client) DeleteReview(ctx context.Context, owner, repo string, number in reqURL := fmt.Sprintf("%s/repos/%s/%s/pulls/%d/reviews/%d", c.baseURL, url.PathEscape(owner), url.PathEscape(repo), number, reviewID) - // nil body: the GitHub DELETE endpoint for reviews requires no request body. _, err := c.doRequestWithBody(ctx, http.MethodDelete, reqURL, nil) if err != nil { var apiErr *APIError @@ -204,12 +230,7 @@ func (c *Client) DismissReview(ctx context.Context, owner, repo string, number i Event: "DISMISS", } - data, err := json.Marshal(payload) - if err != nil { - return fmt.Errorf("marshal dismiss request: %w", err) - } - - _, err = c.doRequestWithBody(ctx, http.MethodPut, reqURL, data) + _, err := c.doJSONRequest(ctx, http.MethodPut, reqURL, payload) if err != nil { return fmt.Errorf("dismiss review: %w", err) } @@ -228,3 +249,17 @@ func (c *Client) SupersedeReviews(ctx context.Context, owner, repo string, prNum } return errors.Join(errs...) } + +// GetAuthenticatedUser returns the login name of the authenticated user. +func (c *Client) GetAuthenticatedUser(ctx context.Context) (string, error) { + reqURL := fmt.Sprintf("%s/user", c.baseURL) + body, err := c.doGet(ctx, reqURL) + if err != nil { + return "", fmt.Errorf("get authenticated user: %w", err) + } + var resp userResponse + if err := json.Unmarshal(body, &resp); err != nil { + return "", fmt.Errorf("parse user response: %w", err) + } + return resp.Login, nil +}