fix(github): teams with access show in the reviewer picker

packages/source-control-github/src/server/GitHubPullRequestApi.test.ts

@@ -4270,42 +4270,70 @@ layer("GitHubPullRequestApi.layer", (it) => {
     }),
   );
 
-  it.effect("reads who may review and who already has in one request", () =>
-    Effect.gen(function* () {
-      mockedExecute.mockReturnValue(
-        Effect.succeed(
-          output(
-            encodeJson({
-              data: {
-                repository: {
-                  assignableUsers: {
-                    pageInfo: { hasNextPage: false },
-                    nodes: ["bilal", "octocat", "hubot"].map((login) => ({
-                      login,
-                      name: null,
-                      avatarUrl: `https://avatars/${login}`,
-                    })),
-                  },
-                  pullRequest: {
-                    author: { __typename: "User", login: "bilal" },
-                    reviewRequests: {
-                      nodes: [
-                        {
-                          requestedReviewer: {
-                            __typename: "User",
-                            login: "octocat",
-                            name: null,
-                            avatarUrl: "https://avatars/octocat",
-                          },
-                        },
-                      ],
-                    },
-                  },
-                },
+  const reviewerTeam = {
+    slug: "web",
+    combinedSlug: "acme/web",
+    name: "Web",
+    avatarUrl: null,
+    repositories: { nodes: [{ nameWithOwner: "acme/web" }] },
+  };
+  const reviewerTeams = (teams: ReadonlyArray<typeof reviewerTeam>) => ({
+    data: { organization: { teams: { pageInfo: { hasNextPage: false }, nodes: teams } } },
+  });
+  const reviewerPeople = (assignable: ReadonlyArray<string>, requested: ReadonlyArray<string>) => ({
+    data: {
+      repository: {
+        assignableUsers: {
+          pageInfo: { hasNextPage: false },
+          nodes: assignable.map((login) => ({
+            login,
+            name: null,
+            avatarUrl: `https://avatars/${login}`,
+          })),
+        },
+        pullRequest: {
+          author: { __typename: "User", login: "bilal" },
+          reviewRequests: {
+            nodes: requested.map((login) => ({
+              requestedReviewer: {
+                __typename: "User",
+                login,
+                name: null,
+                avatarUrl: `https://avatars/${login}`,
               },
-            }),
-          ),
-        ),
+            })),
+          },
+        },
+      },
+    },
+  });
+
+  it.effect.each([
+    {
+      name: "lists teams with access beside the people",
+      teams: reviewerTeams([reviewerTeam]),
+      expected: [
+        ["octocat", true],
+        ["acme/web", false],
+        ["hubot", false],
+      ],
+    },
+    {
+      name: "still lists the people when the token cannot read the organization's teams",
+      teams: {
+        data: null,
+        errors: [{ type: "INSUFFICIENT_SCOPES", message: "Your token needs read:org." }],
+      },
+      expected: [
+        ["octocat", true],
+        ["hubot", false],
+      ],
+    },
+  ])("$name", ({ teams, expected }) =>
+    Effect.gen(function* () {
+      route(
+        ["ReviewerCandidates", reviewerPeople(["bilal", "octocat", "hubot"], ["octocat"])],
+        ["ReviewerTeams", teams],
       );
       const cli = yield* GitHubPullRequestApi.GitHubPullRequestApi;
 
@@ -4316,45 +4344,31 @@ layer("GitHubPullRequestApi.layer", (it) => {
         number: 7,
       });
 
-      assert.strictEqual(mockedExecute.mock.calls.length, 1);
-      expect(varsAt(0)).toMatchObject({ owner: "acme", name: "web", number: 7 });
-      expect(list.candidates.map((candidate) => [candidate.login, candidate.isRequested])).toEqual([
-        ["octocat", true],
-        ["hubot", false],
+      expect(variablesOf("ReviewerCandidates")).toMatchObject([
+        { owner: "acme", name: "web", number: 7 },
       ]);
+      expect(variablesOf("ReviewerTeams")).toMatchObject([{ owner: "acme", name: "web" }]);
+      expect(list.candidates.map((candidate) => [candidate.login, candidate.isRequested])).toEqual(
+        expected,
+      );
     }),
   );
 
   it.effect("asks GitHub to search everyone assignable when the reviewer menu has a query", () =>
     Effect.gen(function* () {
-      mockedExecute.mockReturnValue(
-        Effect.succeed(
-          output(
-            encodeJson({
-              data: {
-                repository: {
-                  assignableUsers: {
-                    pageInfo: { hasNextPage: false },
-                    nodes: [{ login: "octocat", name: null, avatarUrl: "https://avatars/octocat" }],
-                  },
-                  pullRequest: {
-                    author: { __typename: "User", login: "bilal" },
-                    reviewRequests: {
-                      nodes: ["octocat", "unrelated"].map((login) => ({
-                        requestedReviewer: {
-                          __typename: "User",
-                          login,
-                          name: null,
-                          avatarUrl: "https://avatars/user",
-                        },
-                      })),
-                    },
-                  },
-                },
-              },
-            }),
-          ),
-        ),
+      route(
+        ["ReviewerCandidates", reviewerPeople(["octocat"], ["octocat", "unrelated"])],
+        [
+          "ReviewerTeams",
+          reviewerTeams([
+            {
+              ...reviewerTeam,
+              slug: "octonauts",
+              combinedSlug: "acme/octonauts",
+              name: "Octonauts",
+            },
+          ]),
+        ],
       );
       const cli = yield* GitHubPullRequestApi.GitHubPullRequestApi;
 
@@ -4366,9 +4380,15 @@ layer("GitHubPullRequestApi.layer", (it) => {
         query: "octo",
       });
 
-      expect(varsAt(0)).toMatchObject({ owner: "acme", name: "web", number: 7, query: "octo" });
+      expect(variablesOf("ReviewerCandidates")).toMatchObject([
+        { owner: "acme", name: "web", number: 7, query: "octo" },
+      ]);
+      expect(variablesOf("ReviewerTeams")).toMatchObject([
+        { owner: "acme", name: "web", query: "octo" },
+      ]);
       expect(list.candidates.map((candidate) => [candidate.login, candidate.isRequested])).toEqual([
         ["octocat", true],
+        ["acme/octonauts", false],
       ]);
     }),
   );

packages/source-control-github/src/server/GitHubPullRequestApi.ts

@@ -103,6 +103,7 @@ import {
   toPullRequestWatchFingerprint,
   toReviewDismissalsPage,
   toReviewerCandidates,
+  toReviewerTeams,
   toReviewThreadCommentsPage,
   toLaterReviewThreadPage,
   toReviewThreadPage,
@@ -2463,13 +2464,25 @@ export const make = Effect.gen(function* () {
       function (input) {
         const { owner, name } = parseRepositorySelector(input.repository);
         const operation = "listReviewerCandidates";
-        return client
-          .ReviewerCandidates({ owner, name, number: input.number, query: input.query ?? null })
-          .pipe(
-            graphql({ ...input, operation, allowReserve: true }),
-            Effect.map((result) => toReviewerCandidates(result, input.query)),
-            Effect.flatMap(found({ ...input, operation })),
-          );
+        const target = { ...input, operation, allowReserve: true };
+        const query = input.query ?? null;
+        return Effect.all(
+          [
+            client
+              .ReviewerCandidates({ owner, name, number: input.number, query })
+              .pipe(graphql(target)),
+            client.ReviewerTeams({ owner, name, query }).pipe(
+              graphql(target),
+              Effect.map((result) => toReviewerTeams(result, `${owner}/${name}`)),
+              // Teams need `read:org` and an organization owner; people are still worth listing.
+              Effect.orElseSucceed(() => null),
+            ),
+          ],
+          { concurrency: "unbounded" },
+        ).pipe(
+          Effect.map(([people, teams]) => toReviewerCandidates(people, input.query, teams)),
+          Effect.flatMap(found({ ...input, operation })),
+        );
       },
     ),
 

packages/source-control-github/src/server/gitHubPullRequestJson.test.ts

@@ -19,6 +19,7 @@ import {
   toPullRequestFilesViewedPage,
   toPullRequestRows,
   toReviewerCandidates,
+  toReviewerTeams,
   toReviewThreadCommentsPage,
   toCanWrite,
   toReviewThreadPage,
@@ -31,6 +32,7 @@ import {
   PullRequestCore,
   PullRequestFilesViewed,
   ReviewerCandidates,
+  ReviewerTeams,
   ViewerPermissions,
 } from "./pullRequestReads.graphql.ts";
 import { ReviewThreadComments, ReviewThreads } from "./pullRequestReviews.graphql.ts";
@@ -212,6 +214,8 @@ const decodeReviewThreadCommentsJson = (raw: string) =>
 const decodeViewerPermissionsJson = through(ViewerPermissions.result, toViewerPermissions);
 const decodeLabelCandidatesJson = through(LabelCandidates.result, toLabelCandidates);
 const decodeReviewerCandidatesJson = through(ReviewerCandidates.result, toReviewerCandidates);
+const decodeReviewerCandidates = Schema.decodeUnknownSync(ReviewerCandidates.result);
+const decodeReviewerTeams = Schema.decodeSync(ReviewerTeams.result);
 const decodePullRequestFilesViewedJson = through(
   PullRequestFilesViewed.result,
   toPullRequestFilesViewedPage,
@@ -1462,6 +1466,49 @@ describe("reviewer candidate decoding", () => {
       },
     ]);
   });
+
+  it("offers the organization's teams that can reach this repository, each once", () => {
+    const team = (slug: string, repositories: ReadonlyArray<string>) => ({
+      slug,
+      combinedSlug: `acme/${slug}`,
+      name: slug,
+      avatarUrl: null,
+      repositories: { nodes: repositories.map((nameWithOwner) => ({ nameWithOwner })) },
+    });
+    const teams = decodeReviewerTeams({
+      organization: {
+        teams: {
+          pageInfo: { hasNextPage: true },
+          nodes: [
+            team("reviewers", ["acme/web"]),
+            team("frontend", ["ACME/Web"]),
+            team("legacy", ["acme/web-legacy"]),
+          ],
+        },
+      },
+    });
+    const people = JSON.parse(
+      candidatesJson({
+        assignable: [{ login: "octocat" }],
+        requested: [{ slug: "reviewers", combinedSlug: "acme/reviewers", teamName: "Reviewers" }],
+      }),
+    ) as { readonly data: unknown };
+
+    const list = toReviewerCandidates(
+      decodeReviewerCandidates(people.data),
+      undefined,
+      toReviewerTeams(teams, "acme/web"),
+    );
+
+    expect(
+      list?.candidates.map(({ kind, login, isRequested }) => [kind, login, isRequested]),
+    ).toEqual([
+      ["team", "acme/reviewers", true],
+      ["team", "acme/frontend", false],
+      ["user", "octocat", false],
+    ]);
+    expect(list?.truncated).toBe(true);
+  });
 });
 
 describe("reviewer request payload", () => {

packages/source-control-github/src/server/gitHubPullRequestJson.ts

@@ -73,6 +73,7 @@ import type {
   RepositoryReferences,
   ReferenceCandidate,
   ReviewerCandidates,
+  ReviewerTeams,
   ViewerPermissions,
 } from "./pullRequestReads.graphql.ts";
 import type {
@@ -1555,10 +1556,41 @@ export interface GitHubBaseComparison {
   readonly viewerCanUpdate: boolean;
 }
 
+/** Teams with access to `repository` (owner/name). Null when the owner is not an organization. */
+export function toReviewerTeams(
+  result: ReviewerTeams.Result,
+  repository: string,
+): PullRequestReviewerCandidateList | null {
+  const teams = result.organization?.teams;
+  if (teams == null) return null;
+  const wanted = repository.toLowerCase();
+  return {
+    candidates: nodesOf(teams).flatMap((team) => {
+      const slug = trimmed(team.slug);
+      const hasAccess = nodesOf(team.repositories).some(
+        (node) => node.nameWithOwner.toLowerCase() === wanted,
+      );
+      if (slug === null || !hasAccess) return [];
+      return [
+        {
+          id: slug,
+          kind: "team" as const,
+          login: trimmed(team.combinedSlug) ?? slug,
+          name: trimmed(team.name),
+          avatarUrl: trimmed(team.avatarUrl),
+          isRequested: false,
+        },
+      ];
+    }),
+    truncated: teams.pageInfo.hasNextPage,
+  };
+}
+
 /** assignableUsers avoids push/read:org requirements. Keep requested outsiders/apps removable. */
 export function toReviewerCandidates(
   result: ReviewerCandidates.Result,
   query?: string,
+  teams: PullRequestReviewerCandidateList | null = null,
 ): PullRequestReviewerCandidateList | null {
   const repository = result.repository;
   if (repository == null) return null;
@@ -1583,9 +1615,13 @@ export function toReviewerCandidates(
     });
   }
   const hostMatches = new Set<string>();
+  for (const team of teams?.candidates ?? []) {
+    hostMatches.add(`team ${team.id}`);
+    if (!candidates.has(`team ${team.id}`)) candidates.set(`team ${team.id}`, team);
+  }
   for (const node of nodesOf(repository.assignableUsers)) {
     const login = trimmed(node.login);
-    if (login !== null) hostMatches.add(login);
+    if (login !== null) hostMatches.add(`user ${login}`);
     if (login === null || login === author || candidates.has(`user ${login}`)) continue;
     candidates.set(`user ${login}`, {
       id: login,
@@ -1598,9 +1634,9 @@ export function toReviewerCandidates(
   }
   return {
     candidates: [...candidates.values()].filter(
-      (candidate) => !query?.trim() || (candidate.kind === "user" && hostMatches.has(candidate.id)),
+      (candidate) => !query?.trim() || hostMatches.has(`${candidate.kind} ${candidate.id}`),
     ),
-    truncated: repository.assignableUsers.pageInfo.hasNextPage,
+    truncated: repository.assignableUsers.pageInfo.hasNextPage || teams?.truncated === true,
   };
 }