Skip to content

Fix re-review detection and improve PR list pagination #196

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.

Sign up for GitHub

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

Merged
C-Hipple merged 1 commit into from
Jul 29, 2026
Merged

Fix re-review detection and improve PR list pagination #196

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 13 additions & 1 deletion docs/filters.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,12 +13,24 @@ Each workflow can use the available filters to customize which PRs are included.
* `FilterCIFailing` - Only include PRs with failing CI/Actions
* `FilterStale` - PRs with no activity for more than 3 days
* `FilterNotStale` - PRs with activity within the last 3 days
* `FilterWaitingOnMe` - PRs where you are a requested reviewer and need to act
* `FilterWaitingOnMe` - PRs that need action from you (see below)
* `FilterWaitingOnAuthor` - PRs where you were the last to act and are waiting on the author
* `FilterByLabel:<label_name>` - Only include PRs with the specified label (e.g., `FilterByLabel:bug`)
* `FilterByAuthor:<username>` - Only include PRs authored by the specified user
* `FilterExcludeAuthor:<username>` - Exclude PRs authored by the specified user

## FilterWaitingOnMe

A PR is included when any of the following is true:

1. **You have an open review request.** GitHub clears a review request as soon as you submit a review, and re-requesting a review puts you back in the PR's requested reviewers — so being listed there means the request is outstanding, no matter how recently you reviewed or commented. This is what makes re-review requests show up.
2. **Your most recent review was dismissed.** Stale-review dismissal and CODEOWNERS re-requests throw away your approval *without* adding you back to the requested reviewers, so this is the only signal that you owe a re-review.
3. **You have unresponded comments.** You participated in a review thread (or the PR conversation) and someone else replied after you.

`FilterWaitingOnAuthor` is the complement: it excludes PRs where you have an open request or a dismissed review, so a PR never lands in both sections.

Only *personal* review requests count. If your review is requested through a team, use the `Teams` field described below to build a section for it.

## Team-Based Filtering

You can filter PRs by team reviewers by adding a `Teams` field to your workflow configuration. When `Teams` is specified, only PRs where one of those teams is requested as a reviewer will be included. Each workflow can specify its own list of teams, allowing different workflows to target different teams.
Expand Down
174 changes: 136 additions & 38 deletions git_tools/git_tools.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,18 @@ type PRFilter func([]*github.PullRequest) []*github.PullRequest

func GetPRs(client *github.Client, state string, owner string, repo string) ([]*github.PullRequest, error) {
per_page := 100
options := github.PullRequestListOptions{State: state, ListOptions: github.ListOptions{PerPage: per_page, Page: 1}}
// Sort by recent activity, not creation date. We only ever look at the first
// max_additional_calls+1 pages, so on a repo with more open PRs than that
// window the ordering decides what is even eligible for filtering. A PR that
// just had a review re-requested has a fresh updated_at but may have been
// created months ago, and under the default created-desc ordering it fell off
// the end of the window and never reached the filters at all.
options := github.PullRequestListOptions{
State: state,
Sort: "updated",
Direction: "desc",
ListOptions: github.ListOptions{PerPage: per_page, Page: 1},
}
// Use make so a successful fetch with 0 results returns a non-nil empty slice.
// Callers use nil to detect fetch failure vs success (nil == failed, non-nil == succeeded).
prs := make([]*github.PullRequest, 0)
Expand Down Expand Up @@ -857,6 +868,11 @@ type InteractionState struct {
LastOthersTime time.Time
LastCommitTime time.Time
HasUnrespondedComments bool
// MyReviewDismissed is true when the most recent review I submitted has been
// dismissed (stale-review dismissal, a CODEOWNERS re-request, or a manual
// dismissal). GitHub does not put me back in RequestedReviewers in that case,
// so this is the only signal that I owe a re-review.
MyReviewDismissed bool
}

func CalculateInteractionState(myLogin string, pr *github.PullRequest, reviews []*github.PullRequestReview, reviewComments []*github.PullRequestComment, issueComments []*github.IssueComment) InteractionState {
Expand All @@ -867,11 +883,15 @@ func CalculateInteractionState(myLogin string, pr *github.PullRequest, reviews [
}

// Fetch Reviews
var myLatestReview *github.PullRequestReview
for _, r := range reviews {
if r == nil || r.SubmittedAt == nil || r.User == nil || r.User.Login == nil {
continue
}
if strings.EqualFold(*r.User.Login, myLogin) {
if myLatestReview == nil || r.SubmittedAt.After(myLatestReview.SubmittedAt.Time) {
myLatestReview = r
}
if r.SubmittedAt.After(state.LastMeTime) {
state.LastMeTime = r.SubmittedAt.Time
}
Expand All @@ -881,6 +901,7 @@ func CalculateInteractionState(myLogin string, pr *github.PullRequest, reviews [
}
}
}
state.MyReviewDismissed = myLatestReview != nil && myLatestReview.GetState() == "DISMISSED"

// Fetch Review Comments
// Group comments by their "thread"
Expand Down Expand Up @@ -942,6 +963,56 @@ func CalculateInteractionState(myLogin string, pr *github.PullRequest, reviews [
return state
}

// interactionPageSize / interactionMaxPages bound the paginated fetches behind
// GetInteractionState. GitHub caps a PR's commit list at 250 entries, and a PR
// with more than 500 comments is far past the point where any of this matters,
// so five pages is plenty while keeping the worst case at five calls per list.
const (
interactionPageSize = 100
interactionMaxPages = 5
)

// listAllPages walks the paginated endpoint fetch until it runs out of pages
// (or hits interactionMaxPages), returning everything it collected. On error it
// returns the pages gathered so far alongside the error, so callers can still
// work with a partial history rather than falling back to nothing.
func listAllPages[T any](fetch func(opts *github.ListOptions) ([]T, *github.Response, error)) ([]T, error) {
var all []T
opts := &github.ListOptions{PerPage: interactionPageSize, Page: 1}
for i := 0; i < interactionMaxPages; i++ {
page, resp, err := fetch(opts)
if err != nil {
return all, err
}
all = append(all, page...)
if resp == nil || resp.NextPage == 0 {
break
}
opts.Page = resp.NextPage
}
return all, nil
}

// latestCommitTime returns the newest commit timestamp in commits. It scans for
// the maximum rather than trusting the last element: pagination and rebases both
// hand back lists whose final entry is not the most recent commit, and an
// under-reported "last push" time makes a PR look like I have already responded
// to it.
func latestCommitTime(commits []*github.RepositoryCommit) time.Time {
var latest time.Time
for _, c := range commits {
if c == nil || c.Commit == nil {
continue
}
for _, ts := range []*github.CommitAuthor{c.Commit.Committer, c.Commit.Author} {
if ts != nil && ts.Date != nil && ts.Date.After(latest) {
latest = ts.Date.Time
}
}
}
return latest
}

func GetInteractionState(owner, repo string, pr *github.PullRequest) InteractionState {
if pr == nil || pr.Number == nil {
return InteractionState{}
Expand Down Expand Up @@ -969,9 +1040,17 @@ func GetInteractionState(owner, repo string, pr *github.PullRequest) Interaction

resultChan := make(chan result, 4)

// Every list below is paginated. GitHub's default page size is 30, and all of
// these endpoints return oldest-first, so a single unpaginated call on an
// active PR hands back only the *start* of its history: my latest review, the
// reply that is waiting on me, and the newest commit all fall off the end.
// That silently skews every timestamp this state is built from.

// Fetch Reviews
go func() {
reviews, _, err := client.PullRequests.ListReviews(ctx, owner, repo, *pr.Number, nil)
reviews, err := listAllPages(func(opts *github.ListOptions) ([]*github.PullRequestReview, *github.Response, error) {
return client.PullRequests.ListReviews(ctx, owner, repo, *pr.Number, opts)
})
if err != nil {
slog.Warn("Error listing reviews", "owner", owner, "repo", repo, "number", *pr.Number, "error", err)
}
Expand All @@ -980,7 +1059,10 @@ func GetInteractionState(owner, repo string, pr *github.PullRequest) Interaction

// Fetch Review Comments
go func() {
reviewComments, _, err := client.PullRequests.ListComments(ctx, owner, repo, *pr.Number, nil)
reviewComments, err := listAllPages(func(opts *github.ListOptions) ([]*github.PullRequestComment, *github.Response, error) {
return client.PullRequests.ListComments(ctx, owner, repo, *pr.Number,
&github.PullRequestListCommentsOptions{ListOptions: *opts})
})
if err != nil {
slog.Warn("Error listing review comments", "owner", owner, "repo", repo, "number", *pr.Number, "error", err)
}
Expand All @@ -989,7 +1071,10 @@ func GetInteractionState(owner, repo string, pr *github.PullRequest) Interaction

// Fetch Issue Comments
go func() {
issueComments, _, err := client.Issues.ListComments(ctx, owner, repo, *pr.Number, nil)
issueComments, err := listAllPages(func(opts *github.ListOptions) ([]*github.IssueComment, *github.Response, error) {
return client.Issues.ListComments(ctx, owner, repo, *pr.Number,
&github.IssueListCommentsOptions{ListOptions: *opts})
})
if err != nil {
slog.Warn("Error listing issue comments", "owner", owner, "repo", repo, "number", *pr.Number, "error", err)
}
Expand All @@ -998,7 +1083,9 @@ func GetInteractionState(owner, repo string, pr *github.PullRequest) Interaction

// Fetch Commits
go func() {
commits, _, err := client.PullRequests.ListCommits(ctx, owner, repo, *pr.Number, nil)
commits, err := listAllPages(func(opts *github.ListOptions) ([]*github.RepositoryCommit, *github.Response, error) {
return client.PullRequests.ListCommits(ctx, owner, repo, *pr.Number, opts)
})
if err != nil {
slog.Warn("Error listing commits", "owner", owner, "repo", repo, "number", *pr.Number, "error", err)
}
Expand Down Expand Up @@ -1034,12 +1121,7 @@ func GetInteractionState(owner, repo string, pr *github.PullRequest) Interaction
state := CalculateInteractionState(myLogin, pr, reviews, reviewComments, issueComments)

// Set LastCommitTime from commits
if len(commits) > 0 {
latestCommit := commits[len(commits)-1]
if latestCommit.Commit != nil && latestCommit.Commit.Committer != nil && latestCommit.Commit.Committer.Date != nil {
state.LastCommitTime = latestCommit.Commit.Committer.Date.Time
}
}
state.LastCommitTime = latestCommitTime(commits)

// Cache the result even if there were errors (with shorter TTL)
// This prevents retry storms
Expand Down Expand Up @@ -1079,6 +1161,19 @@ func GetMyTeams() []string {
return slugs
}

// reviewRequestedFrom reports whether login has an open personal review request
// on the PR. GitHub logins are case-insensitive, so the comparison uses
// EqualFold: a casing mismatch between the configured GithubUsername and the
// login returned by the API would otherwise match nobody.
func reviewRequestedFrom(pr *github.PullRequest, login string) bool {
for _, r := range pr.RequestedReviewers {
if r != nil && r.Login != nil && strings.EqualFold(*r.Login, login) {
return true
}
}
return false
}

func FilterWaitingOnMe(prs []*github.PullRequest) []*github.PullRequest {
myLogin := config.C().GithubUsername

Expand All @@ -1102,34 +1197,29 @@ func FilterWaitingOnMe(prs []*github.PullRequest) []*github.PullRequest {

state := GetInteractionState(*pr.Base.Repo.Owner.Login, *pr.Base.Repo.Name, pr)

// Is Personally Requested?
isRequested := false
for _, r := range pr.RequestedReviewers {
// GitHub logins are case-insensitive, so compare with EqualFold:
// a casing mismatch between the configured GithubUsername and the
// login returned by the API would otherwise make isRequested
// always false and the filter would match nothing.
if r != nil && r.Login != nil && strings.EqualFold(*r.Login, myLogin) {
isRequested = true
break
}
}
isRequested := reviewRequestedFrom(pr, myLogin)

// A PR is "Waiting on me" if:
// 1. I am individually requested AND I haven't acted since the last push or last other action.
// 2. OR I have unresponded comments.
shouldFilter := false

if isRequested {
actedSinceOthers := !state.LastMeTime.IsZero() && state.LastMeTime.After(state.LastOthersTime) && state.LastMeTime.After(state.LastCommitTime)
if !actedSinceOthers {
shouldFilter = true
}
}

if state.HasUnrespondedComments {
shouldFilter = true
}
// 1. I have a pending review request on it, OR
// 2. my last review was dismissed (so I owe a re-review), OR
// 3. I have unresponded comments.
//
// Case 1 is deliberately unconditional. GitHub clears a review request
// the moment the requested reviewer submits a review, and "re-request
// review" puts them back into RequestedReviewers — so a login sitting in
// RequestedReviewers *now* always means an outstanding request, however
// recently that person reviewed or commented. This used to be gated on
// "have I acted since the last push / last other action", which hid
// exactly the re-review case: the prior review is newer than both the
// last commit and the last comment, so every re-request was filtered out.
shouldFilter := isRequested || state.MyReviewDismissed || state.HasUnrespondedComments

slog.Debug("FilterWaitingOnMe",
"repo", *pr.Base.Repo.Name, "number", *pr.Number, "included", shouldFilter,
"requested", isRequested, "review_dismissed", state.MyReviewDismissed,
"unresponded_comments", state.HasUnrespondedComments,
"last_me", state.LastMeTime, "last_others", state.LastOthersTime,
"last_commit", state.LastCommitTime)

resultChan <- prResult{pr: pr, shouldFilter: shouldFilter}
}(pr)
Expand All @@ -1153,6 +1243,8 @@ func FilterWaitingOnMe(prs []*github.PullRequest) []*github.PullRequest {
}

func FilterWaitingOnAuthor(prs []*github.PullRequest) []*github.PullRequest {
myLogin := config.C().GithubUsername

// Process PRs concurrently to avoid sequential API calls
type prResult struct {
pr *github.PullRequest
Expand All @@ -1174,7 +1266,13 @@ func FilterWaitingOnAuthor(prs []*github.PullRequest) []*github.PullRequest {
state := GetInteractionState(*pr.Base.Repo.Owner.Login, *pr.Base.Repo.Name, pr)
actedSinceOthers := !state.LastMeTime.IsZero() && state.LastMeTime.After(state.LastOthersTime) && state.LastMeTime.After(state.LastCommitTime)

resultChan <- prResult{pr: pr, shouldFilter: actedSinceOthers}
// An open review request on me — or a dismissal of my last review —
// outranks the timestamps: the ball is back in my court, not the
// author's. Without this a re-requested PR would show up in both the
// "Waiting on Me" and "Waiting on Author" sections.
owedByMe := reviewRequestedFrom(pr, myLogin) || state.MyReviewDismissed

resultChan <- prResult{pr: pr, shouldFilter: actedSinceOthers && !owedByMe}
}(pr)
}

Expand Down
Loading
Loading