authorgravatar for me@t3.ggTheo Browne <me@t3.gg> 2026-10-14 03:53:40-07:00
committergravatar for noreply@github.comGitHub <noreply@github.com> 2026-10-14 03:53:40-07:00
log6c142934188fa83bcc1d9cb44b425ced82d66468
tree52509d92f79976be3fb4bc1f8e77268a58f21f5e
parent90ca6a775843d226937b396e0af18e9664448508
signature Signed by PGP key B5690EEEBB952194

fix(github): requested teams now show in pull request reviewers (#19375)

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>

5 files changed, 106 insertions(+), 29 deletions(-)

packages/source-control-github/src/server/GitHubPullRequestApi.test.ts+15-1
...@@ -3691,7 +3691,14 @@ layer("GitHubPullRequestApi.layer", (it) => {...@@ -3691,7 +3691,14 @@ layer("GitHubPullRequestApi.layer", (it) => {
3691 reviewRequests: {3691 reviewRequests: {
3692 nodes: [3692 nodes: [
3693 { requestedReviewer: { login: "reviewer" } },3693 { requestedReviewer: { login: "reviewer" } },
3694 { requestedReviewer: { slug: "maintainers", name: "Maintainers" } },3694 {
3695 requestedReviewer: {
3696 slug: "maintainers",
3697 combinedSlug: "octo/maintainers",
3698 name: "Maintainers",
3699 avatarUrl: "https://avatars.githubusercontent.com/t/1",
3700 },
3701 },
3695 ],3702 ],
3696 },3703 },
3697 labels: { nodes: [{ name: "bug", color: "ff0000" }] },3704 labels: { nodes: [{ name: "bug", color: "ff0000" }] },
...@@ -3740,6 +3747,13 @@ layer("GitHubPullRequestApi.layer", (it) => {...@@ -3740,6 +3747,13 @@ layer("GitHubPullRequestApi.layer", (it) => {
3740 expect(detail.comparison).toBeNull();3747 expect(detail.comparison).toBeNull();
3741 expect(detail.reviewRequestLogins).toEqual(["reviewer"]);3748 expect(detail.reviewRequestLogins).toEqual(["reviewer"]);
3742 expect(detail.hasTeamReviewRequest).toBe(true);3749 expect(detail.hasTeamReviewRequest).toBe(true);
3750 expect(detail.reviewRequestTeams).toEqual([
3751 {
3752 login: "octo/maintainers",
3753 name: "Maintainers",
3754 avatarUrl: "https://avatars.githubusercontent.com/t/1",
3755 },
3756 ]);
3743 expect(detail.labels).toEqual([{ name: "bug", color: "ff0000" }]);3757 expect(detail.labels).toEqual([{ name: "bug", color: "ff0000" }]);
3744 expect(detail.checks).toHaveLength(2);3758 expect(detail.checks).toHaveLength(2);
3745 expect(detail.checksState).toBe("failing");3759 expect(detail.checksState).toBe("failing");
packages/source-control-github/src/server/GitHubPullRequestProvider.test.ts+1
...@@ -14,6 +14,7 @@ import { gitHubViewerPermissions, loginAvatarUrl, make } from "./GitHubPullReque...@@ -14,6 +14,7 @@ import { gitHubViewerPermissions, loginAvatarUrl, make } from "./GitHubPullReque
14import type { GitHubReviewThreadComments } from "./gitHubPullRequestJson.ts";14import type { GitHubReviewThreadComments } from "./gitHubPullRequestJson.ts";
1515
16const coreFields = {16const coreFields = {
17 reviewRequestTeams: [],
17 checksTruncated: false,18 checksTruncated: false,
18 comparison: { behindBy: 0, viewerCanUpdate: true },19 comparison: { behindBy: 0, viewerCanUpdate: true },
19 viewerAccess: {20 viewerAccess: {
packages/source-control-github/src/server/GitHubPullRequestProvider.ts+8-5
...@@ -380,11 +380,14 @@ export const make = Effect.gen(function* () {...@@ -380,11 +380,14 @@ export const make = Effect.gen(function* () {
380 Effect.map((pullRequest): ProviderChangeRequestDetail => ({380 Effect.map((pullRequest): ProviderChangeRequestDetail => ({
381 ...pullRequest,381 ...pullRequest,
382 author: withAvatar(pullRequest.author, new Map<string, string>(), input.host),382 author: withAvatar(pullRequest.author, new Map<string, string>(), input.host),
383 reviewers: pullRequest.reviewRequestLogins.map((login) => ({383 reviewers: [
384 login,384 ...pullRequest.reviewRequestLogins.map((login) => ({
385 name: null,385 login,
386 avatarUrl: null,386 name: null,
387 })),387 avatarUrl: null,
388 })),
389 ...pullRequest.reviewRequestTeams,
390 ],
388 mergeCapabilities: pullRequest.viewerAccess.mergeCapabilities,391 mergeCapabilities: pullRequest.viewerAccess.mergeCapabilities,
389 viewerPermissions: gitHubViewerPermissions({392 viewerPermissions: gitHubViewerPermissions({
390 ...pullRequest.viewerAccess,393 ...pullRequest.viewerAccess,
packages/source-control-github/src/server/gitHubPullRequestJson.test.ts+36-6
...@@ -720,15 +720,44 @@ describe("review thread decoding", () => {...@@ -720,15 +720,44 @@ describe("review thread decoding", () => {
720 expect(result.reviewers).toHaveLength(1);720 expect(result.reviewers).toHaveLength(1);
721 });721 });
722722
723 it("skips a team request, which names nobody to show", () => {723 it("lists a requested team by org/slug, apart from a user who shares its slug", () => {
724 const result = expectSuccess(
725 decodeReviewThreadsJson(
726 reviewJson({
727 // Recorded from pingdotgg/t3code#12329, where CODEOWNERS asked the team.
728 requested: [
729 {
730 combinedSlug: "pingdotgg/ping-devs",
731 name: "Ping Devs",
732 avatarUrl: "https://avatars.githubusercontent.com/t/6692390?s=400&v=4",
733 },
734 ],
735 reviewed: [
736 { __typename: "User", login: "ping-devs", avatarUrl: "https://avatars/u.png" },
737 ],
738 }),
739 ),
740 );
741
742 expect(result.reviewers).toEqual([
743 {
744 login: "pingdotgg/ping-devs",
745 name: "Ping Devs",
746 avatarUrl: "https://avatars.githubusercontent.com/t/6692390?s=400&v=4",
747 },
748 { login: "ping-devs", name: null, avatarUrl: "https://avatars/u.png" },
749 ]);
750 });
751
752 it("skips a request whose reviewer GitHub does not show", () => {
724 const result = expectSuccess(decodeReviewThreadsJson(reviewJson({ requested: [null] })));753 const result = expectSuccess(decodeReviewThreadsJson(reviewJson({ requested: [null] })));
725754
726 expect(result.reviewers).toEqual([]);755 expect(result.reviewers).toEqual([]);
727 });756 });
728757
729 it("keeps the conversation when a request is from a team, which has no login", () => {758 it("keeps the conversation when a reviewer has neither a login nor a team slug", () => {
730 // GraphQL answers with an empty object for a union member the query has no fragment for.759 // GraphQL answers with an empty object for a union member the query has no fragment for,
731 // Failing on it would take the whole response down, comments included.760 // such as a mannequin. Failing on it would take the whole response down, comments included.
732 const result = expectSuccess(761 const result = expectSuccess(
733 decodeReviewThreadsJson(762 decodeReviewThreadsJson(
734 reviewJson({ requested: [{}, { login: "julius", avatarUrl: "https://avatars/j.png" }] }),763 reviewJson({ requested: [{}, { login: "julius", avatarUrl: "https://avatars/j.png" }] }),
...@@ -1466,15 +1495,16 @@ describe("reviewer candidate decoding", () => {...@@ -1466,15 +1495,16 @@ describe("reviewer candidate decoding", () => {
1466 decodeReviewerCandidatesJson(1495 decodeReviewerCandidatesJson(
1467 candidatesJson({1496 candidatesJson({
1468 assignable: [{ login: "octocat" }],1497 assignable: [{ login: "octocat" }],
1469 requested: [{ slug: "reviewers", name: "Reviewers" }],1498 requested: [{ slug: "reviewers", combinedSlug: "acme/reviewers", name: "Reviewers" }],
1470 }),1499 }),
1471 ),1500 ),
1472 );1501 );
1473 expect(list.candidates).toEqual([1502 expect(list.candidates).toEqual([
1474 {1503 {
1504 // Asked by slug, shown by the same `org/slug` as its reviewer row.
1475 id: "reviewers",1505 id: "reviewers",
1476 kind: "team",1506 kind: "team",
1477 login: "reviewers",1507 login: "acme/reviewers",
1478 name: "Reviewers",1508 name: "Reviewers",
1479 avatarUrl: null,1509 avatarUrl: null,
1480 isRequested: true,1510 isRequested: true,
packages/source-control-github/src/server/gitHubPullRequestJson.ts+46-17
...@@ -46,9 +46,9 @@ const RawActorSchema = Schema.Struct({...@@ -46,9 +46,9 @@ const RawActorSchema = Schema.Struct({
46 __typename: Schema.optional(Schema.String),46 __typename: Schema.optional(Schema.String),
47 is_bot: Schema.optional(Schema.Boolean),47 is_bot: Schema.optional(Schema.Boolean),
48 /**48 /**
49 * Optional because a review can be requested from a team or a mannequin, which the query has49 * Optional because a requested team goes by its slug instead, and a mannequin, which no query
50 * no fragment for and GraphQL answers with an empty object. A reviewer with no login names50 * has a fragment for, arrives as an empty object. A reviewer with neither names nobody to show,
51 * nobody to show, and must not fail the response the conversation travels in.51 * and must not fail the response the conversation travels in.
52 */52 */
53 login: Schema.optional(Schema.String),53 login: Schema.optional(Schema.String),
54 /** The node id, which is how a listing's authors are resolved to avatars in one request. */54 /** The node id, which is how a listing's authors are resolved to avatars in one request. */
...@@ -58,6 +58,16 @@ const RawActorSchema = Schema.Struct({...@@ -58,6 +58,16 @@ const RawActorSchema = Schema.Struct({
58 avatarUrl: Schema.optional(Schema.NullOr(Schema.String)),58 avatarUrl: Schema.optional(Schema.NullOr(Schema.String)),
59});59});
6060
61/**
62 * A team answers with a slug where a user answers with a login. `combinedSlug` is the team's
63 * `org/slug`, which is what a reviewer row shows for it.
64 */
65const RawRequestedReviewerSchema = Schema.Struct({
66 ...RawActorSchema.fields,
67 slug: Schema.optional(Schema.NullOr(Schema.String)),
68 combinedSlug: Schema.optional(Schema.NullOr(Schema.String)),
69});
70
61const RawLabelSchema = Schema.Struct({71const RawLabelSchema = Schema.Struct({
62 name: Schema.String,72 name: Schema.String,
63 color: Schema.optional(Schema.NullOr(Schema.String)),73 color: Schema.optional(Schema.NullOr(Schema.String)),
...@@ -66,7 +76,9 @@ const RawLabelSchema = Schema.Struct({...@@ -66,7 +76,9 @@ const RawLabelSchema = Schema.Struct({
66const RawReviewRequestSchema = Schema.Struct({76const RawReviewRequestSchema = Schema.Struct({
67 login: Schema.optional(Schema.NullOr(Schema.String)),77 login: Schema.optional(Schema.NullOr(Schema.String)),
68 slug: Schema.optional(Schema.NullOr(Schema.String)),78 slug: Schema.optional(Schema.NullOr(Schema.String)),
79 combinedSlug: Schema.optional(Schema.NullOr(Schema.String)),
69 name: Schema.optional(Schema.NullOr(Schema.String)),80 name: Schema.optional(Schema.NullOr(Schema.String)),
81 avatarUrl: Schema.optional(Schema.NullOr(Schema.String)),
70});82});
7183
72/** One reviewer's most recent review: the state is all the verdict needs, the author is for who. */84/** One reviewer's most recent review: the state is all the verdict needs, the author is for who. */
...@@ -618,8 +630,8 @@ const RawReviewThreadsSchema = Schema.Struct({...@@ -618,8 +630,8 @@ const RawReviewThreadsSchema = Schema.Struct({
618 Schema.Struct({630 Schema.Struct({
619 nodes: Schema.Array(631 nodes: Schema.Array(
620 Schema.Struct({632 Schema.Struct({
621 // Null for a team, which is a request nobody in particular owns.633 // Null where GitHub does not name who was asked.
622 requestedReviewer: Schema.optional(Schema.NullOr(RawActorSchema)),634 requestedReviewer: Schema.optional(Schema.NullOr(RawRequestedReviewerSchema)),
623 }),635 }),
624 ),636 ),
625 }),637 }),
...@@ -835,7 +847,7 @@ export const pullRequestCoreGraphQlQuery = (host: string) => {...@@ -835,7 +847,7 @@ export const pullRequestCoreGraphQlQuery = (host: string) => {
835 viewerCanUpdate viewerDidAuthor viewerCanUpdateBranch847 viewerCanUpdate viewerDidAuthor viewerCanUpdateBranch
836 baseRef { compare(headRef: $headRef) { behindBy } }848 baseRef { compare(headRef: $headRef) { behindBy } }
837 reviewRequests(first: 100) {849 reviewRequests(first: 100) {
838 nodes { requestedReviewer { ... on User { login name } ... on Bot { login } ... on Team { slug name } } }850 nodes { requestedReviewer { ... on User { login name } ... on Bot { login } ... on Team { slug combinedSlug name avatarUrl } } }
839 }851 }
840 labels(first: 100) { nodes { name color } }852 labels(first: 100) { nodes { name color } }
841 commits(last: 1) {853 commits(last: 1) {
...@@ -1006,8 +1018,8 @@ function pullRequestRowSelection(includeStacks: boolean, includeStats: boolean):...@@ -1006,8 +1018,8 @@ function pullRequestRowSelection(includeStacks: boolean, includeStats: boolean):
1006 *1018 *
1007 * Reviewers come from here rather than from `gh pr view --json reviewRequests` for two reasons:1019 * Reviewers come from here rather than from `gh pr view --json reviewRequests` for two reasons:
1008 * that field holds only requests still outstanding, so anyone who has already reviewed drops off1020 * that field holds only requests still outstanding, so anyone who has already reviewed drops off
1009 * it, and neither it nor any other `gh` JSON field carries an avatar. A reviewer can be a person1021 * it, and neither it nor any other `gh` JSON field carries an avatar. A reviewer can be a person,
1010 * or an app, and both are asked for by name because they are different GraphQL types.1022 * an app, or a team, and each is asked for by name because they are different GraphQL types.
1011 *1023 *
1012 * `viewerCanUpdate` and `viewerDidAuthor` ride along here for the same reason: they belong to the1024 * `viewerCanUpdate` and `viewerDidAuthor` ride along here for the same reason: they belong to the
1013 * pull request this query is already standing on, so what the reader may do with it arrives with1025 * pull request this query is already standing on, so what the reader may do with it arrives with
...@@ -1052,6 +1064,7 @@ export const REVIEW_THREADS_GRAPHQL_QUERY = `query($owner: String!, $name: Strin...@@ -1052,6 +1064,7 @@ export const REVIEW_THREADS_GRAPHQL_QUERY = `query($owner: String!, $name: Strin
1052 requestedReviewer {1064 requestedReviewer {
1053 ... on User { login name avatarUrl }1065 ... on User { login name avatarUrl }
1054 ... on Bot { __typename login avatarUrl }1066 ... on Bot { __typename login avatarUrl }
1067 ... on Team { combinedSlug name avatarUrl }
1055 }1068 }
1056 }1069 }
1057 }1070 }
...@@ -1413,6 +1426,22 @@ function toActor(raw: Schema.Schema.Type<typeof RawActorSchema> | null | undefin...@@ -1413,6 +1426,22 @@ function toActor(raw: Schema.Schema.Type<typeof RawActorSchema> | null | undefin
1413 };1426 };
1414}1427}
14151428
1429/**
1430 * A requested team as a reviewer row. It goes by `org/slug`, the handle GitHub mentions it by.
1431 * No GitHub login contains a slash, so a team never shares a row or a verdict with a user.
1432 */
1433function toRequestedTeam(
1434 raw:
1435 | Pick<Schema.Schema.Type<typeof RawReviewRequestSchema>, "combinedSlug" | "name" | "avatarUrl">
1436 | null
1437 | undefined,
1438): PullRequestActor | null {
1439 const login = trimmed(raw?.combinedSlug);
1440 return login === null
1441 ? null
1442 : { login, name: trimmed(raw?.name), avatarUrl: trimmed(raw?.avatarUrl) };
1443}
1444
1416function toCommitActor(1445function toCommitActor(
1417 raw: NonNullable<Schema.Schema.Type<typeof RawCommitSchema>["authors"]>[number],1446 raw: NonNullable<Schema.Schema.Type<typeof RawCommitSchema>["authors"]>[number],
1418): PullRequestActor | null {1447): PullRequestActor | null {
...@@ -2302,6 +2331,8 @@ export function decodePullRequestWatchFingerprintsJson(...@@ -2302,6 +2331,8 @@ export function decodePullRequestWatchFingerprintsJson(
2302}2331}
23032332
2304export interface GitHubPullRequestCore extends GitHubPullRequestDetail {2333export interface GitHubPullRequestCore extends GitHubPullRequestDetail {
2334 /** Teams asked for a review. `reviewRequestLogins` holds the people. */
2335 readonly reviewRequestTeams: ReadonlyArray<PullRequestActor>;
2305 readonly viewerAccess: GitHubViewerAccess & GitHubRepositoryAccess;2336 readonly viewerAccess: GitHubViewerAccess & GitHubRepositoryAccess;
2306 readonly comparison: GitHubBaseComparison | null;2337 readonly comparison: GitHubBaseComparison | null;
2307 readonly checksTruncated: boolean;2338 readonly checksTruncated: boolean;
...@@ -2316,6 +2347,9 @@ export function decodePullRequestCoreJson(...@@ -2316,6 +2347,9 @@ export function decodePullRequestCoreJson(
2316 const pr = repository.pullRequest;2347 const pr = repository.pullRequest;
2317 const contexts = pr.commits.nodes[0]?.commit.statusCheckRollup?.contexts;2348 const contexts = pr.commits.nodes[0]?.commit.statusCheckRollup?.contexts;
2318 return Result.succeed({2349 return Result.succeed({
2350 reviewRequestTeams: pr.reviewRequests.nodes.flatMap(
2351 ({ requestedReviewer }) => toRequestedTeam(requestedReviewer) ?? [],
2352 ),
2319 ...toDetail({2353 ...toDetail({
2320 ...pr,2354 ...pr,
2321 reviewRequests: pr.reviewRequests.nodes.flatMap(({ requestedReviewer }) =>2355 reviewRequests: pr.reviewRequests.nodes.flatMap(({ requestedReviewer }) =>
...@@ -2772,7 +2806,7 @@ export function decodeReviewThreadsJson(...@@ -2772,7 +2806,7 @@ export function decodeReviewThreadsJson(
2772 ...(pullRequest.reviewRequests?.nodes ?? []).map((node) => node.requestedReviewer),2806 ...(pullRequest.reviewRequests?.nodes ?? []).map((node) => node.requestedReviewer),
2773 ...(pullRequest.latestReviews?.nodes ?? []).map((node) => node.author),2807 ...(pullRequest.latestReviews?.nodes ?? []).map((node) => node.author),
2774 ]) {2808 ]) {
2775 const actor = toActor(raw);2809 const actor = toActor(raw) ?? toRequestedTeam(raw);
2776 // Keyed by login, so someone who was asked and then answered appears once.2810 // Keyed by login, so someone who was asked and then answered appears once.
2777 if (actor !== null && !reviewers.has(actor.login)) reviewers.set(actor.login, actor);2811 if (actor !== null && !reviewers.has(actor.login)) reviewers.set(actor.login, actor);
2778 }2812 }
...@@ -2932,7 +2966,7 @@ export const REVIEWER_CANDIDATES_GRAPHQL_QUERY = `query($owner: String!, $name:...@@ -2932,7 +2966,7 @@ export const REVIEWER_CANDIDATES_GRAPHQL_QUERY = `query($owner: String!, $name:
2932 nodes {2966 nodes {
2933 requestedReviewer {2967 requestedReviewer {
2934 ... on User { login name avatarUrl }2968 ... on User { login name avatarUrl }
2935 ... on Team { slug name avatarUrl }2969 ... on Team { slug combinedSlug name avatarUrl }
2936 ... on Bot { login avatarUrl }2970 ... on Bot { login avatarUrl }
2937 }2971 }
2938 }2972 }
...@@ -2941,12 +2975,6 @@ export const REVIEWER_CANDIDATES_GRAPHQL_QUERY = `query($owner: String!, $name:...@@ -2941,12 +2975,6 @@ export const REVIEWER_CANDIDATES_GRAPHQL_QUERY = `query($owner: String!, $name:
2941 }2975 }
2942}`;2976}`;
29432977
2944/** A team answers with a slug where a user answers with a login, and nothing else differs. */
2945const RawRequestedReviewerSchema = Schema.Struct({
2946 ...RawActorSchema.fields,
2947 slug: Schema.optional(Schema.NullOr(Schema.String)),
2948});
2949
2950const RawReviewerCandidatesSchema = Schema.Struct({2978const RawReviewerCandidatesSchema = Schema.Struct({
2951 data: Schema.Struct({2979 data: Schema.Struct({
2952 repository: Schema.Struct({2980 repository: Schema.Struct({
...@@ -3004,7 +3032,8 @@ export function decodeReviewerCandidatesJson(...@@ -3004,7 +3032,8 @@ export function decodeReviewerCandidatesJson(
3004 candidates.set(`${slug === null ? "user" : "team"} ${id}`, {3032 candidates.set(`${slug === null ? "user" : "team"} ${id}`, {
3005 id,3033 id,
3006 kind: slug === null ? "user" : "team",3034 kind: slug === null ? "user" : "team",
3007 login: id,3035 // The same `org/slug` the reviewer row shows, so taking the request back finds that row.
3036 login: slug === null ? id : (trimmed(raw?.combinedSlug) ?? id),
3008 name: trimmed(raw?.name),3037 name: trimmed(raw?.name),
3009 avatarUrl: trimmed(raw?.avatarUrl),3038 avatarUrl: trimmed(raw?.avatarUrl),
3010 isRequested: true,3039 isRequested: true,