Skip to content

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

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
t3dotgg merged 1 commit into from
Oct 14, 2026
Merged

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

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
16 changes: 15 additions & 1 deletion packages/source-control-github/src/server/GitHubPullRequestApi.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3691,7 +3691,14 @@ layer("GitHubPullRequestApi.layer", (it) => {
reviewRequests: {
nodes: [
{ requestedReviewer: { login: "reviewer" } },
{ requestedReviewer: { slug: "maintainers", name: "Maintainers" } },
{
requestedReviewer: {
slug: "maintainers",
combinedSlug: "octo/maintainers",
name: "Maintainers",
avatarUrl: "https://avatars.githubusercontent.com/t/1",
},
},
],
},
labels: { nodes: [{ name: "bug", color: "ff0000" }] },
Expand Down Expand Up @@ -3740,6 +3747,13 @@ layer("GitHubPullRequestApi.layer", (it) => {
expect(detail.comparison).toBeNull();
expect(detail.reviewRequestLogins).toEqual(["reviewer"]);
expect(detail.hasTeamReviewRequest).toBe(true);
expect(detail.reviewRequestTeams).toEqual([
{
login: "octo/maintainers",
name: "Maintainers",
avatarUrl: "https://avatars.githubusercontent.com/t/1",
},
]);
expect(detail.labels).toEqual([{ name: "bug", color: "ff0000" }]);
expect(detail.checks).toHaveLength(2);
expect(detail.checksState).toBe("failing");
Expand Down
1 change: 1 addition & 0 deletions packages/source-control-github/src/server/GitHubPullRequestProvider.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import { gitHubViewerPermissions, loginAvatarUrl, make } from "./GitHubPullReque
import type { GitHubReviewThreadComments } from "./gitHubPullRequestJson.ts";

const coreFields = {
reviewRequestTeams: [],
checksTruncated: false,
comparison: { behindBy: 0, viewerCanUpdate: true },
viewerAccess: {
Expand Down
13 changes: 8 additions & 5 deletions packages/source-control-github/src/server/GitHubPullRequestProvider.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -380,11 +380,14 @@ export const make = Effect.gen(function* () {
Effect.map((pullRequest): ProviderChangeRequestDetail => ({
...pullRequest,
author: withAvatar(pullRequest.author, new Map<string, string>(), input.host),
reviewers: pullRequest.reviewRequestLogins.map((login) => ({
login,
name: null,
avatarUrl: null,
})),
reviewers: [
...pullRequest.reviewRequestLogins.map((login) => ({
login,
name: null,
avatarUrl: null,
})),
...pullRequest.reviewRequestTeams,
],
mergeCapabilities: pullRequest.viewerAccess.mergeCapabilities,
viewerPermissions: gitHubViewerPermissions({
...pullRequest.viewerAccess,
Expand Down
42 changes: 36 additions & 6 deletions packages/source-control-github/src/server/gitHubPullRequestJson.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -720,15 +720,44 @@ describe("review thread decoding", () => {
expect(result.reviewers).toHaveLength(1);
});

it("skips a team request, which names nobody to show", () => {
it("lists a requested team by org/slug, apart from a user who shares its slug", () => {
const result = expectSuccess(
decodeReviewThreadsJson(
reviewJson({
// Recorded from pingdotgg/t3code#12329, where CODEOWNERS asked the team.
requested: [
{
combinedSlug: "pingdotgg/ping-devs",
name: "Ping Devs",
avatarUrl: "https://avatars.githubusercontent.com/t/6692390?s=400&v=4",
},
],
reviewed: [
{ __typename: "User", login: "ping-devs", avatarUrl: "https://avatars/u.png" },
],
}),
),
);

expect(result.reviewers).toEqual([
{
login: "pingdotgg/ping-devs",
name: "Ping Devs",
avatarUrl: "https://avatars.githubusercontent.com/t/6692390?s=400&v=4",
},
{ login: "ping-devs", name: null, avatarUrl: "https://avatars/u.png" },
]);
});

it("skips a request whose reviewer GitHub does not show", () => {
const result = expectSuccess(decodeReviewThreadsJson(reviewJson({ requested: [null] })));

expect(result.reviewers).toEqual([]);
});

it("keeps the conversation when a request is from a team, which has no login", () => {
// GraphQL answers with an empty object for a union member the query has no fragment for.
// Failing on it would take the whole response down, comments included.
it("keeps the conversation when a reviewer has neither a login nor a team slug", () => {
// GraphQL answers with an empty object for a union member the query has no fragment for,
// such as a mannequin. Failing on it would take the whole response down, comments included.
const result = expectSuccess(
decodeReviewThreadsJson(
reviewJson({ requested: [{}, { login: "julius", avatarUrl: "https://avatars/j.png" }] }),
Expand Down Expand Up @@ -1466,15 +1495,16 @@ describe("reviewer candidate decoding", () => {
decodeReviewerCandidatesJson(
candidatesJson({
assignable: [{ login: "octocat" }],
requested: [{ slug: "reviewers", name: "Reviewers" }],
requested: [{ slug: "reviewers", combinedSlug: "acme/reviewers", name: "Reviewers" }],
}),
),
);
expect(list.candidates).toEqual([
{
// Asked by slug, shown by the same `org/slug` as its reviewer row.
id: "reviewers",
kind: "team",
login: "reviewers",
login: "acme/reviewers",
name: "Reviewers",
avatarUrl: null,
isRequested: true,
Expand Down
63 changes: 46 additions & 17 deletions packages/source-control-github/src/server/gitHubPullRequestJson.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,9 +46,9 @@ const RawActorSchema = Schema.Struct({
__typename: Schema.optional(Schema.String),
is_bot: Schema.optional(Schema.Boolean),
/**
* Optional because a review can be requested from a team or a mannequin, which the query has
* no fragment for and GraphQL answers with an empty object. A reviewer with no login names
* nobody to show, and must not fail the response the conversation travels in.
* Optional because a requested team goes by its slug instead, and a mannequin, which no query
* has a fragment for, arrives as an empty object. A reviewer with neither names nobody to show,
* and must not fail the response the conversation travels in.
*/
login: Schema.optional(Schema.String),
/** The node id, which is how a listing's authors are resolved to avatars in one request. */
Expand All @@ -58,6 +58,16 @@ const RawActorSchema = Schema.Struct({
avatarUrl: Schema.optional(Schema.NullOr(Schema.String)),
});

/**
* A team answers with a slug where a user answers with a login. `combinedSlug` is the team's
* `org/slug`, which is what a reviewer row shows for it.
*/
const RawRequestedReviewerSchema = Schema.Struct({
...RawActorSchema.fields,
slug: Schema.optional(Schema.NullOr(Schema.String)),
combinedSlug: Schema.optional(Schema.NullOr(Schema.String)),
});

const RawLabelSchema = Schema.Struct({
name: Schema.String,
color: Schema.optional(Schema.NullOr(Schema.String)),
Expand All @@ -66,7 +76,9 @@ const RawLabelSchema = Schema.Struct({
const RawReviewRequestSchema = Schema.Struct({
login: Schema.optional(Schema.NullOr(Schema.String)),
slug: Schema.optional(Schema.NullOr(Schema.String)),
combinedSlug: Schema.optional(Schema.NullOr(Schema.String)),
name: Schema.optional(Schema.NullOr(Schema.String)),
avatarUrl: Schema.optional(Schema.NullOr(Schema.String)),
});

/** One reviewer's most recent review: the state is all the verdict needs, the author is for who. */
Expand Down Expand Up @@ -618,8 +630,8 @@ const RawReviewThreadsSchema = Schema.Struct({
Schema.Struct({
nodes: Schema.Array(
Schema.Struct({
// Null for a team, which is a request nobody in particular owns.
requestedReviewer: Schema.optional(Schema.NullOr(RawActorSchema)),
// Null where GitHub does not name who was asked.
requestedReviewer: Schema.optional(Schema.NullOr(RawRequestedReviewerSchema)),
}),
),
}),
Expand Down Expand Up @@ -835,7 +847,7 @@ export const pullRequestCoreGraphQlQuery = (host: string) => {
viewerCanUpdate viewerDidAuthor viewerCanUpdateBranch
baseRef { compare(headRef: $headRef) { behindBy } }
reviewRequests(first: 100) {
nodes { requestedReviewer { ... on User { login name } ... on Bot { login } ... on Team { slug name } } }
nodes { requestedReviewer { ... on User { login name } ... on Bot { login } ... on Team { slug combinedSlug name avatarUrl } } }
}
labels(first: 100) { nodes { name color } }
commits(last: 1) {
Expand Down Expand Up @@ -1006,8 +1018,8 @@ function pullRequestRowSelection(includeStacks: boolean, includeStats: boolean):
*
* Reviewers come from here rather than from `gh pr view --json reviewRequests` for two reasons:
* that field holds only requests still outstanding, so anyone who has already reviewed drops off
* it, and neither it nor any other `gh` JSON field carries an avatar. A reviewer can be a person
* or an app, and both are asked for by name because they are different GraphQL types.
* it, and neither it nor any other `gh` JSON field carries an avatar. A reviewer can be a person,
* an app, or a team, and each is asked for by name because they are different GraphQL types.
*
* `viewerCanUpdate` and `viewerDidAuthor` ride along here for the same reason: they belong to the
* pull request this query is already standing on, so what the reader may do with it arrives with
Expand Down Expand Up @@ -1052,6 +1064,7 @@ export const REVIEW_THREADS_GRAPHQL_QUERY = `query($owner: String!, $name: Strin
requestedReviewer {
... on User { login name avatarUrl }
... on Bot { __typename login avatarUrl }
... on Team { combinedSlug name avatarUrl }
}
}
}
Expand Down Expand Up @@ -1413,6 +1426,22 @@ function toActor(raw: Schema.Schema.Type<typeof RawActorSchema> | null | undefin
};
}

/**
* A requested team as a reviewer row. It goes by `org/slug`, the handle GitHub mentions it by.
* No GitHub login contains a slash, so a team never shares a row or a verdict with a user.
*/
function toRequestedTeam(
raw:
| Pick<Schema.Schema.Type<typeof RawReviewRequestSchema>, "combinedSlug" | "name" | "avatarUrl">
| null
| undefined,
): PullRequestActor | null {
const login = trimmed(raw?.combinedSlug);
return login === null
? null
: { login, name: trimmed(raw?.name), avatarUrl: trimmed(raw?.avatarUrl) };
}

function toCommitActor(
raw: NonNullable<Schema.Schema.Type<typeof RawCommitSchema>["authors"]>[number],
): PullRequestActor | null {
Expand Down Expand Up @@ -2302,6 +2331,8 @@ export function decodePullRequestWatchFingerprintsJson(
}

export interface GitHubPullRequestCore extends GitHubPullRequestDetail {
/** Teams asked for a review. `reviewRequestLogins` holds the people. */
readonly reviewRequestTeams: ReadonlyArray<PullRequestActor>;
readonly viewerAccess: GitHubViewerAccess & GitHubRepositoryAccess;
readonly comparison: GitHubBaseComparison | null;
readonly checksTruncated: boolean;
Expand All @@ -2316,6 +2347,9 @@ export function decodePullRequestCoreJson(
const pr = repository.pullRequest;
const contexts = pr.commits.nodes[0]?.commit.statusCheckRollup?.contexts;
return Result.succeed({
reviewRequestTeams: pr.reviewRequests.nodes.flatMap(
({ requestedReviewer }) => toRequestedTeam(requestedReviewer) ?? [],
),
...toDetail({
...pr,
reviewRequests: pr.reviewRequests.nodes.flatMap(({ requestedReviewer }) =>
Expand Down Expand Up @@ -2772,7 +2806,7 @@ export function decodeReviewThreadsJson(
...(pullRequest.reviewRequests?.nodes ?? []).map((node) => node.requestedReviewer),
...(pullRequest.latestReviews?.nodes ?? []).map((node) => node.author),
]) {
const actor = toActor(raw);
const actor = toActor(raw) ?? toRequestedTeam(raw);
// Keyed by login, so someone who was asked and then answered appears once.
if (actor !== null && !reviewers.has(actor.login)) reviewers.set(actor.login, actor);
}
Expand Down Expand Up @@ -2932,7 +2966,7 @@ export const REVIEWER_CANDIDATES_GRAPHQL_QUERY = `query($owner: String!, $name:
nodes {
requestedReviewer {
... on User { login name avatarUrl }
... on Team { slug name avatarUrl }
... on Team { slug combinedSlug name avatarUrl }
... on Bot { login avatarUrl }
}
}
Expand All @@ -2941,12 +2975,6 @@ export const REVIEWER_CANDIDATES_GRAPHQL_QUERY = `query($owner: String!, $name:
}
}`;

/** A team answers with a slug where a user answers with a login, and nothing else differs. */
const RawRequestedReviewerSchema = Schema.Struct({
...RawActorSchema.fields,
slug: Schema.optional(Schema.NullOr(Schema.String)),
});

const RawReviewerCandidatesSchema = Schema.Struct({
data: Schema.Struct({
repository: Schema.Struct({
Expand Down Expand Up @@ -3004,7 +3032,8 @@ export function decodeReviewerCandidatesJson(
candidates.set(`${slug === null ? "user" : "team"} ${id}`, {
id,
kind: slug === null ? "user" : "team",
login: id,
// The same `org/slug` the reviewer row shows, so taking the request back finds that row.
login: slug === null ? id : (trimmed(raw?.combinedSlug) ?? id),
name: trimmed(raw?.name),
avatarUrl: trimmed(raw?.avatarUrl),
isRequested: true,
Expand Down
Loading