diff --git a/api/queries_pr.go b/api/queries_pr.go index aa584cf5d..fbed0611f 100644 --- a/api/queries_pr.go +++ b/api/queries_pr.go @@ -958,13 +958,16 @@ func (r ReviewerTeam) Slug() string { func (r ReviewerTeam) sealedReviewerCandidate() {} // SuggestedReviewerActors fetches suggested reviewers for a pull request. -// It combines results from: -// - suggestedReviewerActors (10 max) - suggested based on activity -// - repository collaborators (10 max) - all collaborators -// - organization teams (10 max for org repos) - all teams (if owner is an org) -// Results are returned in that order with duplicates removed. +// It combines results from three sources using a cascading quota system: +// - suggestedReviewerActors - suggested based on PR activity (base quota: 5) +// - repository collaborators - all collaborators (base quota: 5 + unfilled from suggestions) +// - organization teams - all teams for org repos (base quota: 5 + unfilled from collaborators) +// +// This ensures we show up to 15 total candidates, with each source filling any +// unfilled quota from the previous source. Results are deduplicated. // Returns the candidates, a MoreResults count, and an error. func SuggestedReviewerActors(client *Client, repo ghrepo.Interface, prID string, query string) ([]ReviewerCandidate, int, error) { + // Fetch 10 from each source to allow cascading quota to fill from available results. // Use a single query that includes organization.teams - if the owner is not an org, // we'll get a "Could not resolve to an Organization" error which we handle gracefully. type responseData struct { @@ -985,7 +988,7 @@ func SuggestedReviewerActors(client *Client, repo ghrepo.Interface, prID string, } `graphql:"... on Bot"` } } - } `graphql:"suggestedReviewerActors(first: 5, query: $query)"` + } `graphql:"suggestedReviewerActors(first: 10, query: $query)"` } `graphql:"... on PullRequest"` } `graphql:"node(id: $id)"` Repository struct { @@ -995,7 +998,7 @@ func SuggestedReviewerActors(client *Client, repo ghrepo.Interface, prID string, Login string Name string } - } `graphql:"collaborators(first: 5, query: $query)"` + } `graphql:"collaborators(first: 10, query: $query)"` } `graphql:"repository(owner: $owner, name: $name)"` Organization struct { Teams struct { @@ -1003,7 +1006,7 @@ func SuggestedReviewerActors(client *Client, repo ghrepo.Interface, prID string, Nodes []struct { Slug string } - } `graphql:"teams(first: 5, query: $query)"` + } `graphql:"teams(first: 10, query: $query)"` } `graphql:"organization(login: $owner)"` } @@ -1022,54 +1025,73 @@ func SuggestedReviewerActors(client *Client, repo ghrepo.Interface, prID string, return nil, 0, err } - ownerName := repo.RepoOwner() + // Build candidates using cascading quota logic: + // Each source has a base quota of 5, plus any unfilled quota from previous sources. + // This ensures we show up to 15 total candidates, filling gaps when earlier sources have fewer. seen := make(map[string]bool) var candidates []ReviewerCandidate + const baseQuota = 5 - // Add suggested reviewers first (excluding author) + // Suggested reviewers (excluding author) + suggestionsAdded := 0 for _, n := range result.Node.PullRequest.SuggestedActors.Nodes { + if suggestionsAdded >= baseQuota { + break + } if n.IsAuthor { continue } var candidate ReviewerCandidate + var login string if n.Reviewer.TypeName == "User" && n.Reviewer.User.Login != "" { - candidate = NewReviewerUser(n.Reviewer.User.Login, n.Reviewer.User.Name) + login = n.Reviewer.User.Login + candidate = NewReviewerUser(login, n.Reviewer.User.Name) } else if n.Reviewer.TypeName == "Bot" && n.Reviewer.Bot.Login != "" { - candidate = NewReviewerBot(n.Reviewer.Bot.Login) + login = n.Reviewer.Bot.Login + candidate = NewReviewerBot(login) } else { continue } - - login := candidate.Login() if !seen[login] { seen[login] = true candidates = append(candidates, candidate) + suggestionsAdded++ } } - // Add collaborators (deduped against suggested) + // Collaborators: quota = base + unfilled from suggestions + collaboratorsQuota := baseQuota + (baseQuota - suggestionsAdded) + collaboratorsAdded := 0 for _, c := range result.Repository.Collaborators.Nodes { + if collaboratorsAdded >= collaboratorsQuota { + break + } if c.Login == "" { continue } - candidate := NewReviewerUser(c.Login, c.Name) - login := candidate.Login() - if !seen[login] { - seen[login] = true - candidates = append(candidates, candidate) + if !seen[c.Login] { + seen[c.Login] = true + candidates = append(candidates, NewReviewerUser(c.Login, c.Name)) + collaboratorsAdded++ } } - // Add teams (will be empty if owner is not an org) + // Teams: quota = base + unfilled from collaborators + teamsQuota := baseQuota + (collaboratorsQuota - collaboratorsAdded) + teamsAdded := 0 + ownerName := repo.RepoOwner() for _, t := range result.Organization.Teams.Nodes { + if teamsAdded >= teamsQuota { + break + } if t.Slug == "" { continue } - candidate := NewReviewerTeam(ownerName, t.Slug) - login := candidate.Login() - if !seen[login] { - seen[login] = true - candidates = append(candidates, candidate) + teamLogin := fmt.Sprintf("%s/%s", ownerName, t.Slug) + if !seen[teamLogin] { + seen[teamLogin] = true + candidates = append(candidates, NewReviewerTeam(ownerName, t.Slug)) + teamsAdded++ } } diff --git a/api/queries_pr_test.go b/api/queries_pr_test.go index 0e2646048..cea2c201b 100644 --- a/api/queries_pr_test.go +++ b/api/queries_pr_test.go @@ -2,6 +2,8 @@ package api import ( "encoding/json" + "fmt" + "strings" "testing" "github.com/cli/cli/v2/internal/ghrepo" @@ -136,3 +138,200 @@ func Test_Logins(t *testing.T) { }) } } + +// mockReviewerResponse generates a GraphQL response for SuggestedReviewerActors tests. +// It creates suggestions (s1, s2...), collaborators (c1, c2...), and teams (team1, team2...). +// totalCollabs and totalTeams set the TotalCount fields (for "more results" calculation). +func mockReviewerResponse(suggestions, collabs, teams, totalCollabs, totalTeams int) string { + var suggestionNodes, collabNodes, teamNodes []string + + for i := 1; i <= suggestions; i++ { + suggestionNodes = append(suggestionNodes, + fmt.Sprintf(`{"isAuthor": false, "reviewer": {"__typename": "User", "login": "s%d", "name": "S%d"}}`, i, i)) + } + for i := 1; i <= collabs; i++ { + collabNodes = append(collabNodes, + fmt.Sprintf(`{"login": "c%d", "name": "C%d"}`, i, i)) + } + for i := 1; i <= teams; i++ { + teamNodes = append(teamNodes, + fmt.Sprintf(`{"slug": "team%d"}`, i)) + } + + return fmt.Sprintf(`{ + "data": { + "node": {"suggestedReviewerActors": {"nodes": [%s]}}, + "repository": {"collaborators": {"totalCount": %d, "nodes": [%s]}}, + "organization": {"teams": {"totalCount": %d, "nodes": [%s]}} + } + }`, strings.Join(suggestionNodes, ","), totalCollabs, strings.Join(collabNodes, ","), + totalTeams, strings.Join(teamNodes, ",")) +} + +func TestSuggestedReviewerActors(t *testing.T) { + tests := []struct { + name string + httpStubs func(*httpmock.Registry) + expectedCount int + expectedLogins []string + expectedMore int + expectError bool + }{ + { + name: "all sources plentiful - 5 each from cascading quota", + httpStubs: func(reg *httpmock.Registry) { + reg.Register( + httpmock.GraphQL(`query SuggestedReviewerActors\b`), + httpmock.StringResponse(mockReviewerResponse(6, 6, 6, 20, 10))) + }, + expectedCount: 15, + expectedLogins: []string{"s1", "s2", "s3", "s4", "s5", "c1", "c2", "c3", "c4", "c5", "OWNER/team1", "OWNER/team2", "OWNER/team3", "OWNER/team4", "OWNER/team5"}, + expectedMore: 30, + }, + { + name: "few suggestions - collaborators fill gap", + httpStubs: func(reg *httpmock.Registry) { + reg.Register( + httpmock.GraphQL(`query SuggestedReviewerActors\b`), + httpmock.StringResponse(mockReviewerResponse(2, 10, 6, 50, 10))) + }, + expectedCount: 15, + expectedLogins: []string{"s1", "s2", "c1", "c2", "c3", "c4", "c5", "c6", "c7", "c8", "OWNER/team1", "OWNER/team2", "OWNER/team3", "OWNER/team4", "OWNER/team5"}, + expectedMore: 60, + }, + { + name: "few suggestions and collaborators - teams fill gap", + httpStubs: func(reg *httpmock.Registry) { + reg.Register( + httpmock.GraphQL(`query SuggestedReviewerActors\b`), + httpmock.StringResponse(mockReviewerResponse(2, 3, 10, 3, 10))) + }, + expectedCount: 15, + expectedLogins: []string{"s1", "s2", "c1", "c2", "c3", "OWNER/team1", "OWNER/team2", "OWNER/team3", "OWNER/team4", "OWNER/team5", "OWNER/team6", "OWNER/team7", "OWNER/team8", "OWNER/team9", "OWNER/team10"}, + expectedMore: 13, + }, + { + name: "no suggestions or collaborators - teams only", + httpStubs: func(reg *httpmock.Registry) { + reg.Register( + httpmock.GraphQL(`query SuggestedReviewerActors\b`), + httpmock.StringResponse(mockReviewerResponse(0, 0, 10, 0, 10))) + }, + expectedCount: 10, // max 15, but only 10 teams available + expectedLogins: []string{"OWNER/team1", "OWNER/team2", "OWNER/team3", "OWNER/team4", "OWNER/team5", "OWNER/team6", "OWNER/team7", "OWNER/team8", "OWNER/team9", "OWNER/team10"}, + expectedMore: 10, + }, + { + name: "author excluded from suggestions", + httpStubs: func(reg *httpmock.Registry) { + // Custom response with author flag + reg.Register( + httpmock.GraphQL(`query SuggestedReviewerActors\b`), + httpmock.StringResponse(`{ + "data": { + "node": {"suggestedReviewerActors": {"nodes": [ + {"isAuthor": true, "reviewer": {"__typename": "User", "login": "author", "name": "Author"}}, + {"isAuthor": false, "reviewer": {"__typename": "User", "login": "s1", "name": "S1"}}, + {"isAuthor": false, "reviewer": {"__typename": "User", "login": "s2", "name": "S2"}} + ]}}, + "repository": {"collaborators": {"totalCount": 5, "nodes": [{"login": "c1", "name": "C1"}]}}, + "organization": {"teams": {"totalCount": 3, "nodes": [{"slug": "team1"}]}} + } + }`)) + }, + expectedCount: 4, + expectedLogins: []string{"s1", "s2", "c1", "OWNER/team1"}, + expectedMore: 8, + }, + { + name: "deduplication across sources", + httpStubs: func(reg *httpmock.Registry) { + // Custom response with duplicate user + reg.Register( + httpmock.GraphQL(`query SuggestedReviewerActors\b`), + httpmock.StringResponse(`{ + "data": { + "node": {"suggestedReviewerActors": {"nodes": [ + {"isAuthor": false, "reviewer": {"__typename": "User", "login": "shareduser", "name": "Shared"}} + ]}}, + "repository": {"collaborators": {"totalCount": 10, "nodes": [ + {"login": "shareduser", "name": "Shared"}, + {"login": "c1", "name": "C1"} + ]}}, + "organization": {"teams": {"totalCount": 5, "nodes": [{"slug": "team1"}]}} + } + }`)) + }, + expectedCount: 3, + expectedLogins: []string{"shareduser", "c1", "OWNER/team1"}, + expectedMore: 15, + }, + { + name: "personal repo - no organization teams", + httpStubs: func(reg *httpmock.Registry) { + reg.Register( + httpmock.GraphQL(`query SuggestedReviewerActors\b`), + httpmock.StringResponse(`{ + "data": { + "node": {"suggestedReviewerActors": {"nodes": [ + {"isAuthor": false, "reviewer": {"__typename": "User", "login": "s1", "name": "S1"}} + ]}}, + "repository": {"collaborators": {"totalCount": 3, "nodes": [{"login": "c1", "name": "C1"}]}}, + "organization": null + }, + "errors": [{"message": "Could not resolve to an Organization with the login of 'OWNER'."}] + }`)) + }, + expectedCount: 2, + expectedLogins: []string{"s1", "c1"}, + expectedMore: 3, + }, + { + name: "bot reviewer included", + httpStubs: func(reg *httpmock.Registry) { + reg.Register( + httpmock.GraphQL(`query SuggestedReviewerActors\b`), + httpmock.StringResponse(`{ + "data": { + "node": {"suggestedReviewerActors": {"nodes": [ + {"isAuthor": false, "reviewer": {"__typename": "Bot", "login": "copilot-pull-request-reviewer"}}, + {"isAuthor": false, "reviewer": {"__typename": "User", "login": "s1", "name": "S1"}} + ]}}, + "repository": {"collaborators": {"totalCount": 5, "nodes": []}}, + "organization": {"teams": {"totalCount": 0, "nodes": []}} + } + }`)) + }, + expectedCount: 2, + expectedLogins: []string{"copilot-pull-request-reviewer", "s1"}, + expectedMore: 5, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + reg := &httpmock.Registry{} + if tt.httpStubs != nil { + tt.httpStubs(reg) + } + + client := newTestClient(reg) + repo, _ := ghrepo.FromFullName("OWNER/REPO") + + candidates, moreResults, err := SuggestedReviewerActors(client, repo, "PR_123", "") + if tt.expectError { + assert.Error(t, err) + return + } + assert.NoError(t, err) + assert.Equal(t, tt.expectedCount, len(candidates), "candidate count mismatch") + assert.Equal(t, tt.expectedMore, moreResults, "moreResults mismatch") + + logins := make([]string, len(candidates)) + for i, c := range candidates { + logins[i] = c.Login() + } + assert.Equal(t, tt.expectedLogins, logins) + }) + } +}