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
41 changes: 2 additions & 39 deletions apps/server/src/pullRequest/GitHubPullRequestCli.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@ import {
decodePullRequestListJson,
decodePullRequestSearchJson,
decodePullRequestStatsJson,
decodeRepositoryAccessJson,
decodeReviewerCandidatesJson,
decodeReviewThreadCommentsJson,
decodeReviewThreadsJson,
Expand All @@ -40,7 +39,6 @@ import {
PULL_REQUEST_ACTIVITY_JSON_FIELDS,
PULL_REQUEST_DETAIL_JSON_FIELDS,
PULL_REQUEST_LIST_JSON_FIELDS,
REPOSITORY_ACCESS_JSON_FIELDS,
RESOLVE_REVIEW_THREAD_GRAPHQL_MUTATION,
REVIEWER_CANDIDATES_GRAPHQL_QUERY,
REVIEW_THREAD_COMMENTS_GRAPHQL_QUERY,
Expand Down Expand Up @@ -316,20 +314,13 @@ export class GitHubPullRequestCli extends Context.Service<
readonly ids: ReadonlyArray<string>;
}) => Effect.Effect<ReadonlyMap<string, string>, GitHubPullRequestCliError>;

/** One `gh repo view`, which answers what the repository allows and where the viewer stands. */
readonly getRepositoryAccess: (input: {
readonly cwd: string;
readonly repository: string;
readonly host: string;
}) => Effect.Effect<GitHubRepositoryAccess, GitHubPullRequestCliError>;

/** The viewer's standing on its own, for deciding a write without reading the whole detail. */
/** One GraphQL read for the repository's merge settings and the viewer's standing. */
readonly getViewerAccess: (input: {
readonly cwd: string;
readonly repository: string;
readonly host: string;
readonly number: number;
}) => Effect.Effect<GitHubViewerAccess, GitHubPullRequestCliError>;
}) => Effect.Effect<GitHubViewerAccess & GitHubRepositoryAccess, GitHubPullRequestCliError>;

/** Who this pull request may be sent to, and who it has already been sent to. */
readonly listReviewerCandidates: (input: {
Expand Down Expand Up @@ -1165,34 +1156,6 @@ export const make = Effect.gen(function* () {
);
},

getRepositoryAccess: (input) =>
github
.execute({
cwd: input.cwd,
args: [
"repo",
"view",
`${input.host}/${input.repository}`,
"--json",
REPOSITORY_ACCESS_JSON_FIELDS,
],
})
.pipe(
Effect.flatMap((result) => {
const decoded = decodeRepositoryAccessJson(result.stdout.trim());
return Result.isSuccess(decoded)
? Effect.succeed(decoded.success)
: Effect.fail(
new GitHubPullRequestReadError({
command: "gh",
cwd: input.cwd,
operation: "getRepositoryAccess",
cause: decoded.failure,
}),
);
}),
),

getViewerAccess: (input) => {
const { owner, name } = parseRepositorySelector(input.repository);
return graphqlRead({
Expand Down
84 changes: 83 additions & 1 deletion apps/server/src/pullRequest/GitHubPullRequestProvider.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,10 @@ import * as Layer from "effect/Layer";
import { GitHubCli } from "../git/Services/GitHubCli.ts";
import * as GitHubPullRequestCli from "./GitHubPullRequestCli.ts";
import * as GitHubPullRequestProvider from "./GitHubPullRequestProvider.ts";
import type { GitHubPullRequestActivity } from "./gitHubPullRequestJson.ts";
import type {
GitHubPullRequestActivity,
GitHubPullRequestDetail,
} from "./gitHubPullRequestJson.ts";

const unusedGitHubCli = Layer.succeed(GitHubCli, {
execute: () => Effect.die("GitHubCli.execute should not be called"),
Expand Down Expand Up @@ -103,3 +106,82 @@ it.effect("reports a strict comment-count lower bound when review-thread loading
assert.strictEqual(activity.commentCount, 2);
}),
);

const pullRequestDetail: GitHubPullRequestDetail = {
authorId: null,
number: 42,
title: "Review me",
url: "https://github.com/acme/web/pull/42",
author: null,
headBranch: "feature/review",
baseBranch: "main",
state: "open",
isDraft: false,
mergeability: "mergeable",
additions: 1,
deletions: 0,
createdAt: "2026-08-10T00:00:00Z",
updatedAt: "2026-08-10T01:00:00Z",
reviewRequestLogins: ["bilal"],
hasTeamReviewRequest: false,
labels: [],
body: "Please review.",
changedFiles: 2,
mergedAt: null,
closedAt: null,
checks: [],
};

it.effect("reads merge settings and viewer standing from one combined access read", () =>
Effect.gen(function* () {
let viewerAccessCalls = 0;
const provider = yield* GitHubPullRequestProvider.make.pipe(
Effect.provide(
Layer.mergeAll(
unusedGitHubCli,
Layer.mock(GitHubPullRequestCli.GitHubPullRequestCli)({
getPullRequestDetail: () => Effect.succeed(pullRequestDetail),
// No getRepositoryAccess mock: a third `gh` read for the merge settings dies here.
getViewerAccess: () => {
viewerAccessCalls += 1;
return Effect.succeed({
mergeCapabilities: { merge: true, squash: true, rebase: false },
canWrite: true,
canUpdate: true,
didAuthor: false,
});
},
}),
),
),
);

const detail = yield* provider.getChangeRequest({
cwd: "/workspace/web",
repository: "acme/web",
host: "github.com",
number: 42,
});

assert.strictEqual(viewerAccessCalls, 1);
assert.deepStrictEqual(detail.mergeCapabilities, {
merge: true,
squash: true,
rebase: false,
});
assert.deepStrictEqual(detail.reviewers, [{ login: "bilal", name: null, avatarUrl: null }]);
// WRITE grants merge; authorship is false so every verdict stays available.
assert.deepStrictEqual(detail.viewerPermissions.actions, [
"merge",
"ready",
"draft",
"close",
"reopen",
]);
assert.deepStrictEqual(detail.viewerPermissions.verdicts, [
"comment",
"approve",
"request-changes",
]);
}),
);
16 changes: 6 additions & 10 deletions apps/server/src/pullRequest/GitHubPullRequestProvider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -209,27 +209,23 @@ export const make = Effect.gen(function* () {
Effect.all(
[
cli.getPullRequestDetail(input),
cli.getRepositoryAccess({
cwd: input.cwd,
repository: input.repository,
host: input.host,
}),
// A small permissions query replaces the deeply paginated review-thread walk on the
// core path. Writes ask again immediately before mutating, so this is presentation.
// One GraphQL read for the repository's merge settings and the viewer's standing,
// replacing the `gh repo view` plus a second permissions query. Writes ask again
// immediately before mutating, so this is presentation.
cli.getViewerAccess(input),
],
{ concurrency: 3 },
{ concurrency: 2 },
).pipe(
Effect.mapError(fail("getChangeRequest")),
Effect.map(
([pullRequest, repository, viewerAccess]): ProviderChangeRequestDetail => ({
([pullRequest, viewerAccess]): ProviderChangeRequestDetail => ({
...pullRequest,
reviewers: pullRequest.reviewRequestLogins.map((login) => ({
login,
name: null,
avatarUrl: null,
})),
mergeCapabilities: repository.mergeCapabilities,
mergeCapabilities: viewerAccess.mergeCapabilities,
viewerPermissions: gitHubViewerPermissions(viewerAccess),
}),
),
Expand Down
35 changes: 35 additions & 0 deletions apps/server/src/pullRequest/gitHubPullRequestJson.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,9 @@ import { describe, expect, it } from "vitest";

import {
decodePullRequestSearchJson,
decodeViewerPermissionsJson,
pullRequestSearchGraphQlQuery,
VIEWER_PERMISSIONS_GRAPHQL_QUERY,
} from "./gitHubPullRequestJson.ts";

describe("GitHub pull request search JSON", () => {
Expand Down Expand Up @@ -46,3 +48,36 @@ describe("GitHub pull request search JSON", () => {
expect(pullRequestSearchGraphQlQuery(1)).toContain("... on Team { slug }");
});
});

describe("GitHub viewer permissions JSON", () => {
it("decodes merge settings and viewer standing from one combined read", () => {
const decoded = decodeViewerPermissionsJson(
JSON.stringify({
data: {
repository: {
mergeCommitAllowed: true,
squashMergeAllowed: false,
rebaseMergeAllowed: true,
viewerPermission: "WRITE",
pullRequest: { viewerCanUpdate: true, viewerDidAuthor: false },
},
},
}),
);

expect(Result.isSuccess(decoded)).toBe(true);
if (!Result.isSuccess(decoded)) return;
expect(decoded.success.mergeCapabilities).toEqual({
merge: true,
squash: false,
rebase: true,
});
expect(decoded.success.canWrite).toBe(true);
expect(decoded.success.canUpdate).toBe(true);
expect(decoded.success.didAuthor).toBe(false);
// The combined query carries the merge settings, so no `gh repo view` is needed.
expect(VIEWER_PERMISSIONS_GRAPHQL_QUERY).toContain("mergeCommitAllowed");
expect(VIEWER_PERMISSIONS_GRAPHQL_QUERY).toContain("squashMergeAllowed");
expect(VIEWER_PERMISSIONS_GRAPHQL_QUERY).toContain("rebaseMergeAllowed");
});
});
47 changes: 18 additions & 29 deletions apps/server/src/pullRequest/gitHubPullRequestJson.ts
Original file line number Diff line number Diff line change
Expand Up @@ -621,12 +621,10 @@ export function buildReviewSubmissionJson(input: {
}

/**
* `viewerPermission` rides along with the merge settings rather than being asked for on its own:
* `gh repo view --json` serves both out of the same GraphQL repository object, so the viewer's
* standing on the repository costs no request of its own.
* What one access read answers: what the repository allows, and where the viewer stands.
* Served by the combined viewer-permissions GraphQL query, so the merge settings cost no
* request of their own.
*/
export const REPOSITORY_ACCESS_JSON_FIELDS =
"mergeCommitAllowed,squashMergeAllowed,rebaseMergeAllowed,viewerPermission";

export interface GitHubPullRequestListItem {
/** The author's node id, kept so a batch can resolve the avatar the listing does not carry. */
Expand Down Expand Up @@ -945,7 +943,6 @@ const decodeStats = decodeJsonResult(RawStatsSchema);
const decodeDetail = decodeJsonResult(RawDetailSchema);
const decodeActivity = decodeJsonResult(RawActivitySchema);
const decodeFileEntry = Schema.decodeUnknownExit(RawPullRequestFileSchema);
const decodeRepositoryAccess = decodeJsonResult(RawRepositoryAccessSchema);
const decodeReviewThreads = decodeJsonResult(RawReviewThreadsSchema);
const decodeReviewThreadComments = decodeJsonResult(RawReviewThreadCommentsSchema);

Expand Down Expand Up @@ -1340,22 +1337,6 @@ function toCanWrite(viewerPermission: string | null | undefined): boolean {
}
}

export function decodeRepositoryAccessJson(
raw: string,
): Result.Result<GitHubRepositoryAccess, DecodeFailure> {
const decoded = decodeRepositoryAccess(raw);
return Result.isSuccess(decoded)
? Result.succeed({
mergeCapabilities: {
merge: decoded.success.mergeCommitAllowed,
squash: decoded.success.squashMergeAllowed,
rebase: decoded.success.rebaseMergeAllowed,
},
canWrite: toCanWrite(decoded.success.viewerPermission),
})
: Result.fail(decoded.failure);
}

/**
* Who a review may be asked of, and who it has already been asked of, in one read.
*
Expand Down Expand Up @@ -1512,22 +1493,25 @@ export interface GitHubViewerAccess {
}

/**
* The viewer's standing, asked on its own. Only the write path needs this: reading a pull request
* already carries the same three fields on calls it was making anyway, and this exists so that a
* merge or a close is decided by what GitHub says now rather than by what the page was told when
* it loaded.
* The viewer's standing and the repository's merge settings in one read. Only the write path
* needs this on its own: reading a pull request carries the same fields on a call it was making
* anyway, and this exists so that a merge or a close is decided by what GitHub says now rather
* than by what the page was told when it loaded.
*
* Asked together, so a response missing any of the merge settings fails rather than defaulting
* open: guessing `true` would offer a merge method the repository forbids.
*/
export const VIEWER_PERMISSIONS_GRAPHQL_QUERY = `query($owner: String!, $name: String!, $number: Int!) {
repository(owner: $owner, name: $name) {
viewerPermission
mergeCommitAllowed squashMergeAllowed rebaseMergeAllowed viewerPermission
pullRequest(number: $number) { viewerCanUpdate viewerDidAuthor }
}
}`;

const RawViewerPermissionsSchema = Schema.Struct({
data: Schema.Struct({
repository: Schema.Struct({
viewerPermission: Schema.optional(Schema.NullOr(Schema.String)),
...RawRepositoryAccessSchema.fields,
/** Null for a number that names no pull request the viewer can see. */
pullRequest: Schema.NullOr(RawViewerFieldsSchema),
}),
Expand All @@ -1538,13 +1522,18 @@ const decodeViewerPermissions = decodeJsonResult(RawViewerPermissionsSchema);

export function decodeViewerPermissionsJson(
raw: string,
): Result.Result<GitHubViewerAccess, DecodeFailure> {
): Result.Result<GitHubViewerAccess & GitHubRepositoryAccess, DecodeFailure> {
const decoded = decodeViewerPermissions(raw);
if (!Result.isSuccess(decoded)) {
return Result.fail(decoded.failure);
}
const repository = decoded.success.data.repository;
return Result.succeed({
mergeCapabilities: {
merge: repository.mergeCommitAllowed,
squash: repository.squashMergeAllowed,
rebase: repository.rebaseMergeAllowed,
},
canWrite: toCanWrite(repository.viewerPermission),
...toPullRequestViewerFields(repository.pullRequest),
});
Expand Down
26 changes: 26 additions & 0 deletions apps/web/src/components/pullRequest/PullRequestRow.browser.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -62,4 +62,30 @@ describe("PullRequestRow", () => {
await row.click();
expect(onSelect).toHaveBeenCalled();
});

it("warms the detail on hover and keyboard focus", async () => {
const onSelect = vi.fn();
const onHoverStart = vi.fn();
const onHoverEnd = vi.fn();
const onFocusRow = vi.fn();
await render(
<PullRequestRow
entry={entry}
selected={false}
onSelect={onSelect}
onHoverStart={onHoverStart}
onHoverEnd={onHoverEnd}
onFocusRow={onFocusRow}
/>,
);

const row = page.getByRole("button", { name: /improve pull request navigation/i });
await row.hover();
expect(onHoverStart).toHaveBeenCalledWith(entry);

// Keyboard focus is intentional on its own: focusing the row warms the detail.
document.querySelector("button")?.focus();
expect(onFocusRow).toHaveBeenCalledWith(entry);
expect(onSelect).not.toHaveBeenCalled();
});
});
Loading
Loading