Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -326,6 +326,7 @@ export const make = Effect.fn("makeBitbucketPullRequestProvider")(function* () {
row.reviews.find(
(review) => review.author?.login.toLowerCase() === reviewer.login.toLowerCase(),
)?.reviewState ?? "pending",
avatarUrl: reviewer.avatarUrl,
})),
checks: checks.items,
// Bitbucket compares no branch with its base.
Expand Down
92 changes: 92 additions & 0 deletions apps/server/src/pullRequest/GitHubPullRequestProvider.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -189,6 +189,98 @@ describe("GitHubPullRequestProvider.setReviewerRequest", () => {
);
});

describe("GitHubPullRequestProvider.listChangeRequests", () => {
const listRow = (input: {
readonly number: number;
readonly author: Record<string, unknown>;
}) => ({
number: input.number,
title: `Pull request ${input.number}`,
url: `https://github.example.com/octocat/example-app/pull/${input.number}`,
author: input.author,
headRefName: `feature/${input.number}`,
baseRefName: "main",
state: "OPEN",
mergedAt: null,
isDraft: false,
additions: 1,
deletions: 0,
createdAt: "2026-08-30T10:00:00Z",
updatedAt: "2026-08-31T10:00:00Z",
reviewDecision: "",
reviewRequests: [],
labels: [],
});

it.effect("derives a plain login's picture and looks up the rest in one request", () =>
Effect.gen(function* () {
mockExecute.mockImplementation((input) =>
Effect.succeed(
processOutput(
input.args[0] === "api"
? JSON.stringify({
data: {
nodes: [{ login: "dependabot[bot]", avatarUrl: "https://avatars.example/bot" }],
},
})
: JSON.stringify([
listRow({ number: 1, author: { login: "octocat", is_bot: false, id: "U_1" } }),
listRow({
number: 2,
author: { login: "dependabot[bot]", is_bot: true, id: "BOT_1" },
}),
]),
),
),
);
const provider = yield* GitHubPullRequestProvider.make();

const rows = yield* provider.listChangeRequests({
...repository,
state: "open",
limit: 30,
});

assert.deepStrictEqual(
rows.map((row) => [row.number, row.author?.avatarUrl]),
[
// Derived from the login on the row's own host, so Enterprise works.
[1, "https://github.example.com/octocat.png?size=80"],
[2, "https://avatars.example/bot"],
],
);
// One listing, then one lookup naming only the account it had to ask about.
const lookup = calls()[1];
assert.deepStrictEqual(lookup?.args, ["api", "graphql", "--input", "-"]);
assert.deepStrictEqual(
(JSON.parse(lookup?.stdin ?? "{}") as { variables: Record<string, unknown> }).variables,
{ ids: ["BOT_1"] },
);
expect(calls()).toHaveLength(2);
}).pipe(Effect.provide(layer)),
);

it.effect("asks the host nothing extra when every login speaks for itself", () =>
Effect.gen(function* () {
mockExecute.mockReturnValue(
Effect.succeed(
processOutput(
JSON.stringify([
listRow({ number: 1, author: { login: "octocat", is_bot: false, id: "U_1" } }),
]),
),
),
);
const provider = yield* GitHubPullRequestProvider.make();

const rows = yield* provider.listChangeRequests({ ...repository, state: "open", limit: 30 });

assert.equal(rows[0]?.author?.avatarUrl, "https://github.example.com/octocat.png?size=80");
expect(calls()).toHaveLength(1);
}).pipe(Effect.provide(layer)),
);
});

describe("GitHubPullRequestProvider.listAuthoredChangeRequests", () => {
it.effect("searches the whole host for the viewer's own work, on stdin", () =>
Effect.gen(function* () {
Expand Down
104 changes: 102 additions & 2 deletions apps/server/src/pullRequest/GitHubPullRequestProvider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,13 @@ import type * as Schema from "effect/Schema";
import type {
PullRequestAction,
PullRequestActivity,
PullRequestActor,
PullRequestBaseComparison,
PullRequestCapabilities,
PullRequestComment,
PullRequestListState,
PullRequestMergeMethod,
PullRequestReviewer,
PullRequestUpdateMethod,
} from "@threadlines/contracts";
import { formatSchemaError } from "@threadlines/shared/schemaJson";
Expand All @@ -21,6 +23,12 @@ import {
findAuthenticatedGitHubAccount,
parseGitHubAuthStatus,
} from "../sourceControl/gitHubAuthStatus.ts";
import {
AVATAR_NODES_GRAPHQL_QUERY,
decodeGitHubAvatarNodesJson,
derivedGitHubAvatarUrl,
gitHubHostFromUrl,
} from "./gitHubAvatar.ts";
import {
decodeGitHubPullRequestActivityJson,
decodeGitHubPullRequestDetailJson,
Expand All @@ -43,6 +51,7 @@ import {
encodeGraphQlRequestJson,
gitHubAuthoredSearchQuery,
gitHubReactionContent,
type GitHubGraphQlVariable,
PULL_REQUEST_CONVERSATION_GRAPHQL_QUERY,
PULL_REQUEST_NODE_ID_GRAPHQL_QUERY,
REACTION_SUBJECT_SCOPE_GRAPHQL_QUERY,
Expand Down Expand Up @@ -241,7 +250,7 @@ export const make = Effect.fn("makeGitHubPullRequestProvider")(function* () {
readonly operation: string;
readonly cwd: string;
readonly query: string;
readonly variables: Readonly<Record<string, string | number | boolean | null>>;
readonly variables: Readonly<Record<string, GitHubGraphQlVariable>>;
}) =>
run({
operation: input.operation,
Expand All @@ -254,7 +263,7 @@ export const make = Effect.fn("makeGitHubPullRequestProvider")(function* () {
readonly operation: string;
readonly cwd: string;
readonly query: string;
readonly variables: Readonly<Record<string, string | number | boolean | null>>;
readonly variables: Readonly<Record<string, GitHubGraphQlVariable>>;
readonly decode: (raw: string) => Result.Result<A, Cause.Cause<Schema.SchemaError>>;
}) =>
graphql(input).pipe(
Expand All @@ -266,6 +275,83 @@ export const make = Effect.fn("makeGitHubPullRequestProvider")(function* () {
}),
);

/**
* The pictures for a set of rows, in as few requests as possible. A plain
* login's picture is at a known URL on the host the row is on, so most rows
* cost nothing; the accounts left over (an app account such as
* `dependabot[bot]`) are looked up together in one request by the node id
* `gh pr list --json author` already reports.
*
* A lookup that fails costs the pictures, not the listing: the client draws
* initials wherever a picture is missing.
*/
const withAuthorAvatars = <
A extends {
readonly url: string;
readonly author: PullRequestActor | null;
readonly authorId: string | null;
},
>(input: {
readonly operation: string;
readonly cwd: string;
readonly rows: ReadonlyArray<A>;
}): Effect.Effect<ReadonlyArray<A>, PullRequestProviderError> => {
const derived = (row: A) =>
row.author === null
? null
: (row.author.avatarUrl ??
derivedGitHubAvatarUrl({
host: gitHubHostFromUrl(row.url),
login: row.author.login,
isBot: row.author.isBot,
}));

const ids = new Set(
input.rows.flatMap((row) =>
row.author !== null && row.authorId !== null && derived(row) === null ? [row.authorId] : [],
),
);

const lookup =
ids.size === 0
? Effect.succeed<ReadonlyMap<string, string>>(new Map())
: graphqlRead({
operation: input.operation,
cwd: input.cwd,
query: AVATAR_NODES_GRAPHQL_QUERY,
variables: { ids: [...ids] },
decode: decodeGitHubAvatarNodesJson,
}).pipe(Effect.catch(() => Effect.succeed<ReadonlyMap<string, string>>(new Map())));

return lookup.pipe(
Effect.map((byLogin) =>
input.rows.map((row) => {
const author = row.author;
if (author === null) {
return row;
}
const avatarUrl = derived(row) ?? byLogin.get(author.login.toLowerCase()) ?? null;
return avatarUrl === author.avatarUrl
? row
: { ...row, author: { ...author, avatarUrl } };
}),
),
);
};

/**
* A reviewer's picture, derived from their login. GitHub reports no picture
* with a review request, and a team is not an account the host serves one
* for, so a team keeps none.
*/
const withReviewerAvatar = (host: string | null) => (reviewer: PullRequestReviewer) =>
reviewer.avatarUrl !== null || reviewer.kind !== "user"
? reviewer
: {
...reviewer,
avatarUrl: derivedGitHubAvatarUrl({ host, login: reviewer.login, isBot: false }),
};

/**
* `gh` takes a body by path. In argv it would show up in process listings and
* run into the command length limit.
Expand Down Expand Up @@ -398,6 +484,7 @@ export const make = Effect.fn("makeGitHubPullRequestProvider")(function* () {
? Effect.succeed(decoded.success)
: Effect.fail(decodeError("list", "PR list", decoded.failure));
}),
Effect.flatMap((rows) => withAuthorAvatars({ operation: "list", cwd: input.cwd, rows })),
),

listAuthoredChangeRequests: (input) =>
Expand Down Expand Up @@ -431,6 +518,17 @@ export const make = Effect.fn("makeGitHubPullRequestProvider")(function* () {
? Effect.succeed(decoded.success)
: Effect.fail(decodeError("detail", "pull request", decoded.failure));
}),
Effect.flatMap((row) =>
withAuthorAvatars({ operation: "detail", cwd: input.cwd, rows: [row] }).pipe(
Effect.map((rows) => rows[0] ?? row),
Effect.map((withAvatar) => ({
...withAvatar,
reviewers: withAvatar.reviewers.map(
withReviewerAvatar(gitHubHostFromUrl(withAvatar.url)),
),
})),
),
),
Effect.flatMap((row) =>
graphqlRead({
operation: "detail",
Expand Down Expand Up @@ -498,6 +596,8 @@ export const make = Effect.fn("makeGitHubPullRequestProvider")(function* () {
...comment,
reactions: annotation.reactions,
viewerIsAuthor: annotation.viewerIsAuthor,
// The GraphQL read is the only one that names a picture.
author: annotation.author ?? comment.author,
};
}),
commits: activity.commits,
Expand Down
10 changes: 9 additions & 1 deletion apps/server/src/pullRequest/GitLabPullRequestProvider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -375,13 +375,21 @@ export const make = Effect.fn("makeGitLabPullRequestProvider")(function* () {
kind: "user",
login: reviewer.login,
state: approved.has(reviewer.login.toLowerCase()) ? "approved" : "pending",
avatarUrl: reviewer.avatarUrl,
}));
// Somebody may approve without having been asked, which GitLab
// reports on the approvals endpoint alone.
const listed = new Set(reviewers.map((reviewer) => reviewer.login.toLowerCase()));
for (const login of approvals) {
if (!listed.has(login.toLowerCase())) {
reviewers.push({ id: login, kind: "user", login, state: "approved" });
// The approvals endpoint names no picture with the account.
reviewers.push({
id: login,
kind: "user",
login,
state: "approved",
avatarUrl: null,
});
}
}
return {
Expand Down
12 changes: 12 additions & 0 deletions apps/server/src/pullRequest/PullRequestProvider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,11 @@ export interface ProviderChangeRequest {
readonly labels: ReadonlyArray<PullRequestLabel>;
/** Absent from a host that does not summarise its reviews. */
readonly reviewDecision?: PullRequestReviewDecision;
/**
* Absent from a host a listing cannot ask about it without a request per row.
* `unknown` is the host saying it has not finished checking.
*/
readonly mergeability?: PullRequestMergeability;
/** Absent where there are no checks, or where the listing did not ask for them. */
readonly checksState?: PullRequestChecksState;
}
Expand All @@ -94,6 +99,13 @@ export interface ProviderChangeRequest {
export interface ProviderAuthoredChangeRequest extends ProviderChangeRequest {
/** Host-native repository identity, the same spelling a listing takes. */
readonly repository: string;
/**
* Whether the viewer may push to that repository, where the search says so.
* A workspace row learns the same thing from {@link ProviderRepositoryAccess};
* a search covers repositories nobody here has checked out, so it has to
* carry the answer itself. Absent where the host named no permission.
*/
readonly viewerCanWrite?: boolean;
}

export interface ProviderChangeRequestDetail extends ProviderChangeRequest {
Expand Down
Loading
Loading