From ed69d26e870ecb28003d8dd318bcdae572cd4804 Mon Sep 17 00:00:00 2001 From: Rodin Date: Sat, 2 May 2026 11:43:53 -0700 Subject: [PATCH] fix: post new review first, then supersede old with link Changes the order of operations: 1. POST new review (gets non-stale badge immediately) 2. PATCH old review with superseded message linking to the new one This gives the superseded comment a clickable link to the current review, making navigation between review iterations easy. buildSupersededBody now accepts a newReviewURL parameter. --- cmd/review-bot/main.go | 42 +++++++++++++++++++++++-------------- cmd/review-bot/main_test.go | 10 ++++++--- 2 files changed, 33 insertions(+), 19 deletions(-) diff --git a/cmd/review-bot/main.go b/cmd/review-bot/main.go index 6e92a6e..9f68b01 100644 --- a/cmd/review-bot/main.go +++ b/cmd/review-bot/main.go @@ -316,29 +316,26 @@ func main() { } // --- Review update strategy --- - // Always post a fresh review on the current commit. If a previous review - // exists, mark it as superseded (preserves old threads as navigable history). - // Never skip posting — the fresh review must land on HEAD to clear stale badges. + // 1. POST new review first (gets non-stale approval badge on HEAD) + // 2. Then supersede old review with link to the new one + // Order matters: post first so we have the new review's URL for the supersede message. + var existingReview *gitea.Review + var existingCommentID int64 if *reviewerName != "" { existingReviews, err := giteaClient.ListReviews(ctx, owner, repoName, prNumber) if err != nil { slog.Warn("could not list existing reviews", "pr", prNumber, "error", err) } else { - // In shared-token mode, skip superseding to avoid clobbering sibling reviews. sharedToken := hasSharedToken(existingReviews, sentinel) if !sharedToken { - existing := findOwnReview(existingReviews, sentinel) - if existing != nil { - commentID, err := giteaClient.GetTimelineReviewCommentID(ctx, owner, repoName, prNumber, sentinel) + existingReview = findOwnReview(existingReviews, sentinel) + if existingReview != nil { + cid, err := giteaClient.GetTimelineReviewCommentID(ctx, owner, repoName, prNumber, sentinel) if err != nil { slog.Warn("could not find old review comment ID for supersede", "error", err) + existingReview = nil // can't supersede without comment ID } else { - supersededBody := buildSupersededBody(existing.Body, existing.CommitID, sentinel) - if err := giteaClient.EditComment(ctx, owner, repoName, commentID, supersededBody); err != nil { - slog.Warn("could not mark old review as superseded", "comment_id", commentID, "error", err) - } else { - slog.Info("marked old review as superseded", "old_state", existing.State, "pr", prNumber) - } + existingCommentID = cid } } } else { @@ -347,7 +344,7 @@ func main() { } } - // POST new review (first run, or state transition fallthrough) + // POST new review slog.Info("posting review", "event", event, "pr", prNumber) posted, err := giteaClient.PostReview(ctx, owner, repoName, prNumber, event, reviewBody, inlineComments) if err != nil { @@ -356,6 +353,17 @@ func main() { } slog.Info("review posted", "review_id", posted.ID, "user", posted.User.Login, "pr", prNumber) + // Supersede old review with link to the new one + if existingReview != nil && existingCommentID > 0 { + newReviewURL := fmt.Sprintf("%s/%s/%s/pulls/%d#pullrequestreview-%d", *giteaURL, owner, repoName, prNumber, posted.ID) + supersededBody := buildSupersededBody(existingReview.Body, existingReview.CommitID, newReviewURL, sentinel) + 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) + } + } + } // fetchFileContext fetches the full content of modified files from the PR branch. @@ -514,14 +522,16 @@ func validateReviewerName(name string) error { // buildSupersededBody creates the body for a superseded review: struck-through banner // with collapsed original content and the commit it was evaluated against. -func buildSupersededBody(originalBody, commitSHA, sentinel string) string { +func buildSupersededBody(originalBody, commitSHA, newReviewURL, sentinel string) string { shortSHA := commitSHA if len(shortSHA) > 8 { shortSHA = shortSHA[:8] } var sb strings.Builder sb.WriteString("~~Original review~~\n\n") - sb.WriteString("**Superseded** \u2014 see current review for up-to-date findings.\n\n") + sb.WriteString("**Superseded** \u2014 [see current review](") + sb.WriteString(newReviewURL) + sb.WriteString(") for up-to-date findings.\n\n") if shortSHA != "" { sb.WriteString("
Previous findings (commit ") sb.WriteString(shortSHA) diff --git a/cmd/review-bot/main_test.go b/cmd/review-bot/main_test.go index d812ea2..c822883 100644 --- a/cmd/review-bot/main_test.go +++ b/cmd/review-bot/main_test.go @@ -60,17 +60,21 @@ func makeReview(id int64, login, state string, stale bool, body string) gitea.Re func TestBuildSupersededBody(t *testing.T) { original := "# Review\n\nLooks good.\n\n" sentinel := "" + newURL := "https://gitea.example.com/owner/repo/pulls/1#pullrequestreview-99" - result := buildSupersededBody(original, "abcdef1234567890", sentinel) + result := buildSupersededBody(original, "abcdef1234567890", newURL, sentinel) // Should contain the struck-through banner if !strings.Contains(result, "~~Original review~~") { t.Error("missing struck-through banner") } - // Should contain superseded notice + // Should contain superseded notice with link if !strings.Contains(result, "**Superseded**") { t.Error("missing superseded notice") } + if !strings.Contains(result, "[see current review]("+newURL+")") { + t.Error("missing link to new review") + } // Should contain collapsed original if !strings.Contains(result, "
") { t.Error("missing details/collapse") @@ -95,7 +99,7 @@ func TestBuildSupersededBody(t *testing.T) { func TestBuildSupersededBodyShortSHA(t *testing.T) { // Short SHA should pass through without panic - result := buildSupersededBody("body", "abc", "") + result := buildSupersededBody("body", "abc", "https://example.com/review", "") if !strings.Contains(result, "abc") { t.Error("short SHA not preserved") } -- 2.47.3