rodin/review-bot
2
1
Fork 0
Code Issues Pull Requests 1 Actions Packages Projects Releases 6 Wiki Activity

fix(github): consolidate review.go and identity.go into reviews.go (#116) #119

Merged
aweiker merged 4 commits from review-bot-issue-116 into feature/github-support 2026-05-14 01:49:57 +00:00
Conversation 27 Commits 4 Files Changed 4
+68 -62
2 changed files with 68 additions and 62 deletions
Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 22b3ce8fef - Show all commits
github/identity.go
-29
View File
@@ -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
}
github/review.go → github/reviews.go
+68 -33
View File
@@ -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
sonnet-review-bot commented 2026-05-13 22:36:59 +00:00
Outdated
Review
Copy Link
Copy Source

[NIT] The pagination loop will always make one extra request (the one that returns 0 results) when the total review count is an exact multiple of reviewsPerPage. This is the standard approach and not a bug, but worth noting that GitHub's Link header could be used to avoid the extra round-trip if this ever becomes performance-sensitive.

**[NIT]** The pagination loop will always make one extra request (the one that returns 0 results) when the total review count is an exact multiple of reviewsPerPage. This is the standard approach and not a bug, but worth noting that GitHub's Link header could be used to avoid the extra round-trip if this ever becomes performance-sensitive.
sonnet-review-bot commented 2026-05-14 01:02:43 +00:00
Review
Copy Link
Copy Source

[NIT] The ListReviews method uses slog.Warn directly (package-level default logger) rather than a logger injected via the Client. This is consistent with the rest of the codebase based on what's visible, so it's fine — just worth noting that it makes the log output untestable without replacing the global logger.

**[NIT]** The `ListReviews` method uses `slog.Warn` directly (package-level default logger) rather than a logger injected via the Client. This is consistent with the rest of the codebase based on what's visible, so it's fine — just worth noting that it makes the log output untestable without replacing the global logger.
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",
sonnet-review-bot commented 2026-05-13 23:03:57 +00:00
Outdated
Review
Copy Link
Copy Source

[NIT] The truncated boolean is only ever set to true inside the loop when page == maxPages (after the short-page guard). The loop condition page <= maxPages combined with the page == maxPages check at the end means the flag is always set on the final iteration when the page was full. This is correct but slightly subtle — a brief inline comment explaining why the outer if page == maxPages fires only when the last page was full (and the earlier break didn't fire) would help future readers. Low impact given the existing comment is close.

**[NIT]** The `truncated` boolean is only ever set to `true` inside the loop when `page == maxPages` (after the short-page guard). The loop condition `page <= maxPages` combined with the `page == maxPages` check at the end means the flag is always set on the final iteration when the page was full. This is correct but slightly subtle — a brief inline comment explaining why the outer `if page == maxPages` fires only when the last page was full (and the earlier `break` didn't fire) would help future readers. Low impact given the existing comment is close.
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)
}
sonnet-review-bot commented 2026-05-14 00:54:08 +00:00
Review
Copy Link
Copy Source

[NIT] The truncation check at if page == maxPages is logically unreachable inside the loop body because the loop condition is page <= maxPages. When page == maxPages and len(responses) == perPage, the loop exits naturally after this iteration anyway (the next iteration would have page = maxPages+1 > maxPages). The truncated = true path is actually reached, but only on the final iteration when the page is full. This is correct behavior but slightly confusing — a comment clarifying 'this is the last iteration and the page was full, so data was likely cut off' would help readers. No functional bug; the logic is sound.

**[NIT]** The truncation check at `if page == maxPages` is logically unreachable inside the loop body because the loop condition is `page <= maxPages`. When `page == maxPages` and `len(responses) == perPage`, the loop exits naturally after this iteration anyway (the next iteration would have `page = maxPages+1 > maxPages`). The `truncated = true` path is actually reached, but only on the final iteration when the page is full. This is correct behavior but slightly confusing — a comment clarifying 'this is the last iteration and the page was full, so data was likely cut off' would help readers. No functional bug; the logic is sound.
if len(responses) == 0 {
break
}
sonnet-review-bot commented 2026-05-13 22:36:59 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] The truncation warning is emitted after appending the last page's reviews, but only when page == maxReviewPages. If the final page happens to be exactly full (len(responses) == reviewsPerPage) and is also the last real page of data, the loop will make one more request that returns 0 results and break cleanly — the warning fires incorrectly in that case. Consider moving the warning outside the loop, after the loop exits, conditioned on page > maxReviewPages or by tracking whether the loop was cut short by the page cap rather than by an empty response.

**[MINOR]** The truncation warning is emitted after appending the last page's reviews, but only when page == maxReviewPages. If the final page happens to be exactly full (len(responses) == reviewsPerPage) and is also the last real page of data, the loop will make one more request that returns 0 results and break cleanly — the warning fires incorrectly in that case. Consider moving the warning outside the loop, after the loop exits, conditioned on `page > maxReviewPages` or by tracking whether the loop was cut short by the page cap rather than by an empty response.
for _, r := range responses {
sonnet-review-bot commented 2026-05-14 01:02:43 +00:00
Review
Copy Link
Copy Source

[NIT] The truncated variable is initialized to false and only set to true in the loop body. Since Go zero-initializes booleans, the explicit false initialization is redundant. var truncated bool would be idiomatic, but this is extremely minor.

**[NIT]** The `truncated` variable is initialized to `false` and only set to `true` in the loop body. Since Go zero-initializes booleans, the explicit `false` initialization is redundant. `var truncated bool` would be idiomatic, but this is extremely minor.
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
1
 @@ -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
}