From 3d0ca57a5fcab69668e61a3257793bd727ff9abb Mon Sep 17 00:00:00 2001 From: Rodin Date: Sat, 2 May 2026 12:10:37 -0700 Subject: [PATCH] fix: address review findings - Gate inline comment resolution on successful supersede - Add pagination to ListReviewComments - Add tests for ListReviewComments and ResolveComment --- cmd/review-bot/main.go | 36 ++++++++++++++------------ gitea/client.go | 39 ++++++++++++++++++----------- gitea/client_test.go | 57 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 102 insertions(+), 30 deletions(-) diff --git a/cmd/review-bot/main.go b/cmd/review-bot/main.go index e40c791..db1b68b 100644 --- a/cmd/review-bot/main.go +++ b/cmd/review-bot/main.go @@ -357,31 +357,35 @@ func main() { if existingReview != nil && existingCommentID > 0 { newReviewURL := fmt.Sprintf("%s/%s/%s/pulls/%d#pullrequestreview-%d", strings.TrimRight(*giteaURL, "/"), owner, repoName, prNumber, posted.ID) supersededBody := buildSupersededBody(existingReview.Body, existingReview.CommitID, newReviewURL, sentinel) + supersedeOK := false if err := giteaClient.EditComment(ctx, owner, repoName, existingCommentID, supersededBody); err != nil { slog.Warn("could not mark old review as superseded", "comment_id", existingCommentID, "error", err) } else { slog.Info("marked old review as superseded", "old_state", existingReview.State, "new_review_id", posted.ID, "pr", prNumber) + supersedeOK = true } - // Resolve old review's inline comments (clears unresolved conversations in PR timeline) - oldComments, err := giteaClient.ListReviewComments(ctx, owner, repoName, prNumber, existingReview.ID) - if err != nil { - slog.Warn("could not list old review comments for resolution", "review_id", existingReview.ID, "error", err) - } else { - resolved := 0 - for _, c := range oldComments { - if c.ID == 0 { - continue + // Resolve old review's inline comments only after successful supersede + if supersedeOK { + oldComments, err := giteaClient.ListReviewComments(ctx, owner, repoName, prNumber, existingReview.ID) + if err != nil { + slog.Warn("could not list old review comments for resolution", "review_id", existingReview.ID, "error", err) + } else { + resolved := 0 + for _, c := range oldComments { + if c.ID == 0 { + continue + } + if err := giteaClient.ResolveComment(ctx, owner, repoName, c.ID); err != nil { + slog.Debug("could not resolve inline comment", "comment_id", c.ID, "error", err) + } else { + resolved++ + } } - if err := giteaClient.ResolveComment(ctx, owner, repoName, c.ID); err != nil { - slog.Debug("could not resolve inline comment", "comment_id", c.ID, "error", err) - } else { - resolved++ + if resolved > 0 { + slog.Info("resolved old inline comments", "count", resolved, "pr", prNumber) } } - if resolved > 0 { - slog.Info("resolved old inline comments", "count", resolved, "pr", prNumber) - } } } diff --git a/gitea/client.go b/gitea/client.go index 0461ca2..c56d895 100644 --- a/gitea/client.go +++ b/gitea/client.go @@ -463,22 +463,33 @@ func (c *Client) EditComment(ctx context.Context, owner, repo string, commentID } // ListReviewComments returns the inline comments attached to a specific review. +// Paginates through all pages. func (c *Client) ListReviewComments(ctx context.Context, owner, repo string, prNumber int, reviewID int64) ([]ReviewComment, error) { - reqURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/pulls/%d/reviews/%d/comments", - c.baseURL, - url.PathEscape(owner), - url.PathEscape(repo), - prNumber, - reviewID) - body, err := c.doGet(ctx, reqURL) - if err != nil { - return nil, fmt.Errorf("list review comments: %w", err) + const pageSize = 50 + var all []ReviewComment + for page := 1; ; page++ { + reqURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/pulls/%d/reviews/%d/comments?limit=%d&page=%d", + c.baseURL, + url.PathEscape(owner), + url.PathEscape(repo), + prNumber, + reviewID, + pageSize, + page) + body, err := c.doGet(ctx, reqURL) + if err != nil { + return nil, fmt.Errorf("list review comments (page %d): %w", page, err) + } + var batch []ReviewComment + if err := json.Unmarshal(body, &batch); err != nil { + return nil, fmt.Errorf("parse review comments (page %d): %w", page, err) + } + all = append(all, batch...) + if len(batch) < pageSize { + break + } } - var comments []ReviewComment - if err := json.Unmarshal(body, &comments); err != nil { - return nil, fmt.Errorf("parse review comments: %w", err) - } - return comments, nil + return all, nil } // ResolveComment marks an inline review comment as resolved. diff --git a/gitea/client_test.go b/gitea/client_test.go index 37dcda0..a27c380 100644 --- a/gitea/client_test.go +++ b/gitea/client_test.go @@ -602,3 +602,60 @@ func TestIsNotFound(t *testing.T) { }) } } + +func TestListReviewComments(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + fmt.Fprint(w, `[{"id":100,"path":"main.go","new_position":5,"body":"finding"},{"id":101,"path":"lib.go","new_position":10,"body":"another"}]`) + })) + defer server.Close() + + client := NewClient(server.URL, "test-token") + comments, err := client.ListReviewComments(context.Background(), "owner", "repo", 1, 42) + if err != nil { + t.Fatalf("ListReviewComments() error = %v", err) + } + if len(comments) != 2 { + t.Fatalf("got %d comments, want 2", len(comments)) + } + if comments[0].ID != 100 { + t.Errorf("comments[0].ID = %d, want 100", comments[0].ID) + } + if comments[1].Path != "lib.go" { + t.Errorf("comments[1].Path = %q, want %q", comments[1].Path, "lib.go") + } +} + +func TestResolveComment(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method != http.MethodPost { + t.Errorf("expected POST, got %s", r.Method) + } + expected := "/api/v1/repos/owner/repo/pulls/comments/99/resolve" + if r.URL.Path != expected { + t.Errorf("path = %q, want %q", r.URL.Path, expected) + } + w.WriteHeader(http.StatusOK) + })) + defer server.Close() + + client := NewClient(server.URL, "test-token") + err := client.ResolveComment(context.Background(), "owner", "repo", 99) + if err != nil { + t.Fatalf("ResolveComment() error = %v", err) + } +} + +func TestResolveComment_Error(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusNotFound) + fmt.Fprint(w, "not found") + })) + defer server.Close() + + client := NewClient(server.URL, "test-token") + err := client.ResolveComment(context.Background(), "owner", "repo", 99) + if err == nil { + t.Fatal("expected error for 404 response") + } +}