-
Notifications
You must be signed in to change notification settings - Fork 0
feat(#788): minimize stale inline review comments on re-review #820
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -312,11 +312,14 @@ func submitFormalReview(ctx context.Context, client forge.Client, owner, repo st | |
| user, err := client.GetAuthenticatedUser(ctx) | ||
| if err != nil { | ||
| printer.StepInfo("Could not determine authenticated user, skipping stale review cleanup") | ||
| } else if reviews, err := client.ListPullRequestReviews(ctx, owner, repo, pr); err != nil { | ||
| printer.StepInfo("Could not list reviews, skipping stale review cleanup") | ||
| } else { | ||
| dismissStaleRequestChanges(ctx, client, owner, repo, pr, event, user, reviews, printer) | ||
| minimizeStaleReviews(ctx, client, user, reviews, printer) | ||
| if reviews, err := client.ListPullRequestReviews(ctx, owner, repo, pr); err != nil { | ||
| printer.StepInfo("Could not list reviews, skipping stale review cleanup") | ||
| } else { | ||
| dismissStaleRequestChanges(ctx, client, owner, repo, pr, event, user, reviews, printer) | ||
| minimizeStaleReviews(ctx, client, user, reviews, printer) | ||
| } | ||
| minimizeStaleInlineComments(ctx, client, owner, repo, pr, user, printer) | ||
| } | ||
|
|
||
| var diffHunks map[string][][2]int | ||
|
|
@@ -618,6 +621,45 @@ func minimizeStaleReviews(ctx context.Context, client forge.Client, user string, | |
| printer.StepDone("Stale reviews minimized") | ||
| } | ||
|
|
||
| // minimizeStaleInlineComments finds all inline review comments posted | ||
| // by the given user on the PR and minimizes them. This prevents | ||
| // duplicate inline comments from accumulating across re-review runs. | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [low] error-handling-consistency minimizeStaleInlineComments checks forge.IsNotSupported(err) to silently skip on GitLab, but the existing ListPullRequestReviews error handler (line 316) does not distinguish error types. The IsNotSupported check is well-motivated here but the same argument applies to ListPullRequestReviews. Suggested fix: Consider aligning both error handling patterns, either by adding IsNotSupported to ListPullRequestReviews or documenting why the difference is intentional. |
||
| // The sticky comment already preserves full review history, so | ||
| // minimizing prior inline comments loses no information. | ||
| // | ||
| // Errors are non-fatal — failing to minimize stale inline comments | ||
| // should not prevent the new review from being submitted. | ||
| func minimizeStaleInlineComments(ctx context.Context, client forge.Client, owner, repo string, pr int, user string, printer *ui.Printer) { | ||
| comments, err := client.ListPullRequestReviewComments(ctx, owner, repo, pr) | ||
| if err != nil { | ||
| // ErrNotSupported is expected for forges that don't support this | ||
| // operation (e.g., GitLab). Silently skip in that case. | ||
| if !forge.IsNotSupported(err) { | ||
| printer.StepInfo(fmt.Sprintf("Could not list inline comments (%v), skipping stale inline comment cleanup", err)) | ||
| } | ||
| return | ||
| } | ||
|
|
||
| var stale []forge.PullRequestReviewComment | ||
| for _, c := range comments { | ||
| if c.User == user { | ||
| stale = append(stale, c) | ||
| } | ||
| } | ||
|
|
||
| if len(stale) == 0 { | ||
| return | ||
| } | ||
|
|
||
| printer.StepStart(fmt.Sprintf("Minimizing %d stale inline comment(s)", len(stale))) | ||
| for _, c := range stale { | ||
| if err := client.MinimizeComment(ctx, c.NodeID, "OUTDATED"); err != nil { | ||
| printer.StepInfo(fmt.Sprintf("Warning: could not minimize inline comment %s: %v", c.NodeID, err)) | ||
| } | ||
| } | ||
| printer.StepDone("Stale inline comments minimized") | ||
| } | ||
|
|
||
| // sanitizeReviewResult runs the security output pipeline over all | ||
| // user-visible text fields in a ReviewResult. This catches leaked | ||
| // secrets and zero-width–obfuscated tokens before they reach the | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1511,3 +1511,100 @@ func TestBuildFallbackReviewBody(t *testing.T) { | |
| assert.Equal(t, "", body) | ||
| }) | ||
| } | ||
|
|
||
| func TestMinimizeStaleInlineComments_MinimizesOwnComments(t *testing.T) { | ||
| fc := forge.NewFakeClient() | ||
| fc.PRReviewComments = map[string][]forge.PullRequestReviewComment{ | ||
| "acme/repo/1": { | ||
| {ID: 1, NodeID: "PRC_1", User: "fullsend-bot", Path: "a.go", Line: 10, Body: "old finding"}, | ||
| {ID: 2, NodeID: "PRC_2", User: "human-reviewer", Path: "a.go", Line: 12, Body: "human comment"}, | ||
| {ID: 3, NodeID: "PRC_3", User: "fullsend-bot", Path: "b.go", Line: 5, Body: "another old finding"}, | ||
| }, | ||
| } | ||
|
|
||
| printer := ui.New(io.Discard) | ||
| minimizeStaleInlineComments(context.Background(), fc, "acme", "repo", 1, "fullsend-bot", printer) | ||
|
|
||
| require.Len(t, fc.MinimizedComments, 2) | ||
| assert.Equal(t, "PRC_1", fc.MinimizedComments[0].NodeID) | ||
| assert.Equal(t, "OUTDATED", fc.MinimizedComments[0].Reason) | ||
| assert.Equal(t, "PRC_3", fc.MinimizedComments[1].NodeID) | ||
| assert.Equal(t, "OUTDATED", fc.MinimizedComments[1].Reason) | ||
| } | ||
|
|
||
| func TestMinimizeStaleInlineComments_NoComments(t *testing.T) { | ||
| fc := forge.NewFakeClient() | ||
| printer := ui.New(io.Discard) | ||
| minimizeStaleInlineComments(context.Background(), fc, "acme", "repo", 1, "fullsend-bot", printer) | ||
| assert.Empty(t, fc.MinimizedComments) | ||
| } | ||
|
|
||
| func TestMinimizeStaleInlineComments_ListErrorIsNonFatal(t *testing.T) { | ||
| fc := forge.NewFakeClient() | ||
| fc.Errors["ListPullRequestReviewComments"] = fmt.Errorf("API error") | ||
|
|
||
| var out bytes.Buffer | ||
| printer := ui.New(&out) | ||
| minimizeStaleInlineComments(context.Background(), fc, "acme", "repo", 1, "fullsend-bot", printer) | ||
|
|
||
| assert.Empty(t, fc.MinimizedComments) | ||
| assert.Contains(t, out.String(), "Could not list inline comments") | ||
| } | ||
|
|
||
| func TestMinimizeStaleInlineComments_MinimizeErrorIsNonFatal(t *testing.T) { | ||
| fc := forge.NewFakeClient() | ||
| fc.Errors["MinimizeComment"] = fmt.Errorf("GraphQL error") | ||
| fc.PRReviewComments = map[string][]forge.PullRequestReviewComment{ | ||
| "acme/repo/1": { | ||
| {ID: 1, NodeID: "PRC_1", User: "fullsend-bot", Path: "a.go", Line: 10, Body: "old"}, | ||
| {ID: 2, NodeID: "PRC_2", User: "fullsend-bot", Path: "b.go", Line: 5, Body: "old2"}, | ||
| }, | ||
| } | ||
|
|
||
| var out bytes.Buffer | ||
| printer := ui.New(&out) | ||
| minimizeStaleInlineComments(context.Background(), fc, "acme", "repo", 1, "fullsend-bot", printer) | ||
|
|
||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [low] test-adequacy TestMinimizeStaleInlineComments_MinimizeErrorIsNonFatal asserts the warning string appears in output but does not verify both comments were attempted. The FakeClient's error injection returns the same error for every MinimizeComment call without recording it, so MinimizedComments is empty. The test cannot distinguish 'loop continued after first error' from 'loop stopped after first error.' Suggested fix: Assert both node IDs (PRC_1 and PRC_2) appear in the warning output to confirm both minimize attempts were made. |
||
| // MinimizeComment errors should be logged but not stop processing. | ||
| assert.Contains(t, out.String(), "could not minimize inline comment") | ||
| } | ||
|
|
||
| func TestMinimizeStaleInlineComments_NotSupportedIsSilent(t *testing.T) { | ||
| fc := forge.NewFakeClient() | ||
| fc.Errors["ListPullRequestReviewComments"] = forge.ErrNotSupported | ||
|
|
||
| var out bytes.Buffer | ||
| printer := ui.New(&out) | ||
| minimizeStaleInlineComments(context.Background(), fc, "acme", "repo", 1, "fullsend-bot", printer) | ||
|
|
||
| assert.Empty(t, fc.MinimizedComments) | ||
| assert.NotContains(t, out.String(), "Could not list inline comments", | ||
| "ErrNotSupported should be silently skipped") | ||
| } | ||
|
|
||
| func TestSubmitFormalReview_MinimizesStaleInlineComments(t *testing.T) { | ||
| fc := forge.NewFakeClient() | ||
| fc.AuthenticatedUser = "fullsend-bot" | ||
| fc.PRReviews = map[string][]forge.PullRequestReview{ | ||
| "acme/repo/1": { | ||
| {ID: 100, NodeID: "PRR_100", User: "fullsend-bot", State: "COMMENTED", Body: "old review"}, | ||
| }, | ||
| } | ||
| fc.PRReviewComments = map[string][]forge.PullRequestReviewComment{ | ||
| "acme/repo/1": { | ||
| {ID: 1, NodeID: "PRC_1", User: "fullsend-bot", Path: "a.go", Line: 10, Body: "old inline"}, | ||
| {ID: 2, NodeID: "PRC_2", User: "someone-else", Path: "a.go", Line: 12, Body: "human inline"}, | ||
| {ID: 3, NodeID: "PRC_3", User: "fullsend-bot", Path: "b.go", Line: 5, Body: "old inline 2"}, | ||
| }, | ||
| } | ||
|
|
||
| printer := ui.New(io.Discard) | ||
| err := submitFormalReview(context.Background(), fc, "acme", "repo", 1, "approve", "", "", nil, false, printer) | ||
| require.NoError(t, err) | ||
|
|
||
| // Should minimize: 1 stale review body + 2 stale inline comments = 3. | ||
| require.Len(t, fc.MinimizedComments, 3) | ||
| assert.Equal(t, "PRR_100", fc.MinimizedComments[0].NodeID, "review body minimized first") | ||
| assert.Equal(t, "PRC_1", fc.MinimizedComments[1].NodeID, "first inline comment minimized") | ||
| assert.Equal(t, "PRC_3", fc.MinimizedComments[2].NodeID, "second inline comment minimized") | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -214,6 +214,20 @@ type ReviewComment struct { | |
| Body string // comment body (Markdown) | ||
| } | ||
|
|
||
| // PullRequestReviewComment represents an existing inline comment on a | ||
| // pull request diff, as returned by the "List review comments" API. | ||
| // This is distinct from ReviewComment (which is used when *creating* | ||
| // inline comments). PullRequestReviewComment carries metadata needed | ||
| // for lifecycle management (e.g., minimizing stale comments). | ||
| type PullRequestReviewComment struct { | ||
| ID int | ||
| NodeID string | ||
| User string // author login | ||
| Path string | ||
| Line int | ||
| Body string | ||
| } | ||
|
|
||
| // PullRequestFileDiff represents a file changed in a pull request along | ||
| // with its unified diff patch. The patch may be empty for binary files, | ||
| // rename-only changes, or when GitHub truncates large diffs. | ||
|
|
@@ -524,6 +538,11 @@ type Client interface { | |
| // comments, when non-nil, attaches inline diff comments to the review. | ||
| CreatePullRequestReview(ctx context.Context, owner, repo string, number int, event, body, commitSHA string, comments []ReviewComment) error | ||
| ListPullRequestReviews(ctx context.Context, owner, repo string, number int) ([]PullRequestReview, error) | ||
| // ListPullRequestReviewComments returns all inline review comments | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [low] interface-extension The forge.Client interface has been extended with a new required method ListPullRequestReviewComments. This is routine internal interface evolution — the interface is in the internal/ package (not importable externally) and all three in-tree implementations (GitHub, GitLab, fake) are correctly updated. |
||
| // on a pull request (comments attached to specific lines in the diff). | ||
| // Used to identify stale inline comments from prior review runs that | ||
| // should be minimized before posting new ones. | ||
| ListPullRequestReviewComments(ctx context.Context, owner, repo string, number int) ([]PullRequestReviewComment, error) | ||
| DismissPullRequestReview(ctx context.Context, owner, repo string, number, reviewID int, message string) error | ||
|
|
||
| // Change proposal merge | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[low] unnecessary-coupling
minimizeStaleInlineComments is placed inside the else-block gated on ListPullRequestReviews succeeding, but it does not use the reviews variable. If ListPullRequestReviews fails (transient API error, rate limit), inline comment cleanup is silently skipped even though it could proceed independently.
Suggested fix: Move the minimizeStaleInlineComments call outside the reviews-dependent else block, into a separate conditional that only requires user to be non-empty.