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
3 changes: 3 additions & 0 deletions apps/server/src/pullRequest/PullRequestProvider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import {
type PullRequestLabel,
type PullRequestListState,
type PullRequestMergeability,
type PullRequestMergeGate,
type PullRequestMergeMethod,
type PullRequestReactionContent,
type PullRequestReviewCommentDraft,
Expand Down Expand Up @@ -116,6 +117,8 @@ export interface ProviderChangeRequestDetail extends ProviderChangeRequest {
readonly closedAt: string | null;
readonly reviewers: ReadonlyArray<PullRequestReviewer>;
readonly checks: ReadonlyArray<PullRequestCheck>;
/** Absent where the host does not say whether its rules would take a merge. */
readonly mergeGate?: PullRequestMergeGate;
readonly baseComparison: PullRequestBaseComparison;
/** Null where the host could not compare the branch with its base. */
readonly behindBy: number | null;
Expand Down
1 change: 1 addition & 0 deletions apps/server/src/pullRequest/PullRequestService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -464,6 +464,7 @@ function toDetail(input: {
labels: row.labels,
checks: row.checks,
...(row.checksState === undefined ? {} : { checksState: row.checksState }),
...(row.mergeGate === undefined ? {} : { mergeGate: row.mergeGate }),
viewer: {
canWrite: input.repository.canWrite,
canReview: viewerKnown && !viewerIsAuthor,
Expand Down
8 changes: 8 additions & 0 deletions apps/server/src/pullRequest/gitHubPullRequestDetail.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,14 @@ function decodeActivity(payload: Record<string, unknown>) {
}

describe("decodeGitHubPullRequestDetailJson", () => {
it("reads the host's merge gate the way gh does, and says nothing while it is undecided", () => {
assert.strictEqual(decodeDetail({ mergeStateStatus: "BLOCKED" }).mergeGate, "blocked");
assert.strictEqual(decodeDetail({ mergeStateStatus: "BEHIND" }).mergeGate, "behind");
assert.strictEqual(decodeDetail({ mergeStateStatus: "UNSTABLE" }).mergeGate, "clear");
assert.strictEqual(decodeDetail({ mergeStateStatus: "UNKNOWN" }).mergeGate, undefined);
assert.strictEqual(decodeDetail({}).mergeGate, undefined);
});

it("lists a re-requested reviewer as pending, keeps a verdict over a later comment, and never the author", () => {
const detail = decodeDetail({
author: { login: "octocat", is_bot: false },
Expand Down
30 changes: 30 additions & 0 deletions apps/server/src/pullRequest/gitHubPullRequestDetail.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import {
type PullRequestComment,
type PullRequestCommit,
type PullRequestMergeability,
type PullRequestMergeGate,
type PullRequestMergeMethod,
type PullRequestReviewer,
type PullRequestReviewState,
Expand Down Expand Up @@ -40,6 +41,9 @@ export const GITHUB_PULL_REQUEST_DETAIL_FIELDS = [
"closedAt",
"reviews",
"autoMergeRequest",
// What `gh pr merge` itself consults before it tries: the host's verdict on
// whether its rules would take the merge.
"mergeStateStatus",
// Qualifies the head branch as `owner:branch`, which is the only name a
// branch on a fork has in the base repository.
"headRepositoryOwner",
Expand Down Expand Up @@ -87,6 +91,8 @@ export interface GitHubPullRequestDetailRow extends GitHubPullRequestListRow {
readonly closedAt: string | null;
readonly reviewers: ReadonlyArray<PullRequestReviewer>;
readonly checks: ReadonlyArray<PullRequestCheck>;
/** Absent while the host is still deciding after a push, or on a host too old to say. */
readonly mergeGate?: PullRequestMergeGate;
/** Null on a host too old to report an auto-merge instruction at all. */
readonly autoMergeEnabled: boolean | null;
/** Qualifies the head branch when it lives on a fork. */
Expand Down Expand Up @@ -137,6 +143,7 @@ const GitHubPullRequestDetailRowSchema = Schema.Struct({
reviews: Schema.optional(Schema.NullOr(Schema.Array(GitHubReviewSchema))),
/** An object while auto-merge is armed, null once it is not, absent on an older CLI. */
autoMergeRequest: Schema.optional(Schema.NullOr(Schema.Struct({}))),
mergeStateStatus: Schema.optional(Schema.NullOr(Schema.String)),
headRepositoryOwner: Schema.optional(
Schema.NullOr(Schema.Struct({ login: Schema.optional(Schema.NullOr(Schema.String)) })),
),
Expand Down Expand Up @@ -223,6 +230,27 @@ function normalizeReviewers(input: {
}

/** One row per check. A repeated name is a re-run, so the last one wins. */
/**
* GitHub's `mergeStateStatus` as the gate the client renders. `BLOCKED` and
* `BEHIND` are the two `gh pr merge` refuses on without `--admin`; `DIRTY` is a
* conflict and already told through `mergeability`, and `UNKNOWN` is a host
* that has not finished deciding, so both say nothing here.
*/
function normalizeMergeGate(value: string | null | undefined): PullRequestMergeGate | undefined {
switch (value?.trim().toUpperCase()) {
case "BLOCKED":
return "blocked";
case "BEHIND":
return "behind";
case "CLEAN":
case "HAS_HOOKS":
case "UNSTABLE":
return "clear";
default:
return undefined;
}
}

function normalizeChecks(
checks: ReadonlyArray<Schema.Schema.Type<typeof GitHubStatusCheckSchema>> | null | undefined,
): ReadonlyArray<PullRequestCheck> {
Expand Down Expand Up @@ -384,6 +412,7 @@ export function decodeGitHubPullRequestDetailJson(

const row = payload.success;
const base = normalizeGitHubPullRequestListRow(row);
const mergeGate = normalizeMergeGate(row.mergeStateStatus);
return Result.succeed({
...base,
body: row.body ?? "",
Expand All @@ -399,6 +428,7 @@ export function decodeGitHubPullRequestDetailJson(
reviews: row.reviews ?? [],
}),
checks: normalizeChecks(row.statusCheckRollup),
...(mergeGate === undefined ? {} : { mergeGate }),
// A CLI too old for the field leaves it absent, which is "the host did not
// say" rather than "auto-merge is off".
autoMergeEnabled: row.autoMergeRequest === undefined ? null : row.autoMergeRequest !== null,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -570,6 +570,28 @@ describe("PullRequestDetailPanel", () => {
await behind.cleanup();
});

it("keeps Merge on screen but off while the host's rules refuse it, and says why", async () => {
const rendered = await renderPanel({
detail: {
viewer: { canWrite: true, canReview: false, canManage: true },
mergeGate: "blocked",
checks: [
{ name: "build", status: "success", description: null, url: null },
{ name: "lint", status: "pending", description: null, url: null },
{ name: "test", status: "pending", description: null, url: null },
],
checksState: "pending",
},
});

await expect.element(page.getByTestId("pull-request-merge")).toBeDisabled();
await expect
.element(page.getByTestId("pull-request-merge-block"))
.toHaveTextContent("Waiting on 2 checks");

await rendered.cleanup();
});

it("asks someone for a review from the reviewers row", async () => {
const rendered = await renderPanel({
detail: { viewer: { canWrite: true, canReview: false, canManage: true } },
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1413,7 +1413,12 @@ function usePullRequestActions({
{/* A disabled button cannot be focused, so the reason it is disabled is
written out as well as tucked in its tooltip. */}
{primary === "merge" && mergeBlock !== null ? (
<p className="mt-1 text-right text-xs text-muted-foreground/60">{mergeBlock}</p>
<p
className="mt-1 text-right text-xs text-muted-foreground/60"
data-testid="pull-request-merge-block"
>
{mergeBlock}
</p>
) : null}
{mutation.isError ? (
<p className="mt-1.5 break-words text-right text-xs text-destructive">
Expand Down
86 changes: 86 additions & 0 deletions apps/web/src/components/pull-requests/pullRequests.logic.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import {
EnvironmentId,
ProjectId,
ThreadId,
type PullRequestCheck,
type PullRequestComment,
type PullRequestListEntry,
type PullRequestListResult,
Expand Down Expand Up @@ -44,6 +45,7 @@ import {
sortPullRequests,
summarizePullRequestChecks,
resolveThreadPullRequest,
shouldPollPullRequestDetail,
type PullRequestDiffFile,
type PullRequestEntry,
type PullRequestFilters,
Expand Down Expand Up @@ -968,6 +970,90 @@ describe("resolvePullRequestMergeBlock", () => {
);
expect(resolvePullRequestMergeBlock({ mergeability: "unknown", isDraft: false })).toBeNull();
});

it("says what the host's own rules are waiting on", () => {
const check = (status: PullRequestCheck["status"]) => ({
name: status,
status,
description: null,
url: null,
});
const blocked = {
mergeability: "mergeable" as const,
isDraft: false,
mergeGate: "blocked" as const,
};

expect(
resolvePullRequestMergeBlock({
...blocked,
checks: [check("pending"), check("pending"), check("failure")],
}),
).toBe("Waiting on 2 checks");
expect(resolvePullRequestMergeBlock({ ...blocked, checks: [check("pending")] })).toBe(
"Waiting on 1 check",
);
expect(
resolvePullRequestMergeBlock({ ...blocked, checks: [check("failure"), check("success")] }),
).toBe("A check failed");
expect(
resolvePullRequestMergeBlock({
...blocked,
checks: [check("success")],
reviewDecision: "review-required",
}),
).toBe("Needs an approving review");
expect(resolvePullRequestMergeBlock({ ...blocked, checks: [] })).toBe(
"Blocked by branch rules",
);
expect(resolvePullRequestMergeBlock({ ...blocked, mergeGate: "behind" })).toBe(
"Update the branch first",
);
// A running check on its own is not a block: without a rule requiring it,
// the host merges anyway.
expect(
resolvePullRequestMergeBlock({ ...blocked, mergeGate: "clear", checks: [check("pending")] }),
).toBeNull();
});
});

describe("shouldPollPullRequestDetail", () => {
const now = Date.parse("2026-09-04T12:00:00.000Z");
const check = (status: PullRequestCheck["status"]) => ({
name: status,
status,
description: null,
url: null,
});
const settled = {
state: "open" as const,
mergeability: "mergeable" as const,
updatedAt: "2026-09-04T11:00:00.000Z",
};

it("polls while a check runs, and for a while after a push before any check exists", () => {
expect(shouldPollPullRequestDetail({ ...settled, checks: [check("pending")] }, now)).toBe(true);
expect(shouldPollPullRequestDetail({ ...settled, checks: [check("success")] }, now)).toBe(
false,
);
expect(shouldPollPullRequestDetail({ ...settled, checks: [] }, now)).toBe(false);

const justPushed = { ...settled, updatedAt: "2026-09-04T11:59:30.000Z" };
expect(shouldPollPullRequestDetail({ ...justPushed, checks: [] }, now)).toBe(true);
expect(
shouldPollPullRequestDetail(
{ ...justPushed, mergeability: "unknown", checks: [check("success")] },
now,
),
).toBe(true);
// Checks that have arrived and settled end the watch early.
expect(shouldPollPullRequestDetail({ ...justPushed, checks: [check("success")] }, now)).toBe(
false,
);
expect(
shouldPollPullRequestDetail({ ...justPushed, state: "merged" as const, checks: [] }, now),
).toBe(false);
});
});

describe("handing a review comment to a thread", () => {
Expand Down
59 changes: 55 additions & 4 deletions apps/web/src/components/pull-requests/pullRequests.logic.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1354,22 +1354,73 @@ export function resolveDefaultMergeMethod(
}

/**
* Why merging is off the table right now, or null when it is available. Both
* answers are things the user fixes elsewhere, so the button stays visible and
* says what is in the way rather than disappearing.
* Why merging is off the table right now, or null when it is available. Every
* answer is something the user fixes elsewhere or waits out, so the button
* stays visible and says what is in the way rather than disappearing. The
* host's own gate comes last: a draft or a conflict is the more useful thing
* to say when both hold.
*/
export function resolvePullRequestMergeBlock(
detail: Pick<PullRequestDetail, "mergeability" | "isDraft">,
detail: Pick<PullRequestDetail, "mergeability" | "isDraft"> &
Partial<Pick<PullRequestDetail, "mergeGate" | "checks" | "reviewDecision">>,
): string | null {
if (detail.isDraft) {
return "Mark as ready first";
}
if (detail.mergeability === "conflicting") {
return "Resolve the conflicts first";
}
if (detail.mergeGate === "behind") {
return "Update the branch first";
}
if (detail.mergeGate === "blocked") {
// The host does not say which rule refused, so this reads the most likely
// one off what it did say: running checks, then failed ones, then reviews.
const { pending, failing } = summarizePullRequestChecks(detail.checks ?? []);
if (pending > 0) {
return pending === 1 ? "Waiting on 1 check" : `Waiting on ${pending} checks`;
}
if (failing > 0) {
return failing === 1 ? "A check failed" : "Checks failed";
}
if (
detail.reviewDecision === "review-required" ||
detail.reviewDecision === "changes-requested"
) {
return "Needs an approving review";
}
return "Blocked by branch rules";
}
return null;
}

/** How long after a push the header waits for the host to register the new commit's checks. */
export const PULL_REQUEST_FRESH_PUSH_WATCH_MS = 120_000;

/**
* Whether the header should keep re-reading itself. A check still running is
* the plain case. The other is the quiet moment right after a push (an update
* from the base, a new commit) when the host has the commit but has not queued
* its checks or decided whether it merges: a read then shows no checks at all,
* and would otherwise sit on that answer until the user hit Refresh.
*/
export function shouldPollPullRequestDetail(
detail: Pick<PullRequestDetail, "state" | "checks" | "mergeability" | "updatedAt">,
now: number,
): boolean {
if (detail.checks.some((check) => check.status === "pending")) {
return true;
}
if (detail.state !== "open") {
return false;
}
const unsettled = detail.checks.length === 0 || detail.mergeability === "unknown";
const updatedAt = Date.parse(detail.updatedAt);
return (
unsettled && Number.isFinite(updatedAt) && now - updatedAt < PULL_REQUEST_FRESH_PUSH_WATCH_MS
);
}

/**
* The prompt a review comment becomes when it is handed to the thread working
* the branch. The comment is quoted rather than restated, so the agent reads
Expand Down
12 changes: 6 additions & 6 deletions apps/web/src/lib/pullRequestsReactQuery.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import { useMemo } from "react";
import { resolveEnvironmentOptionLabel } from "~/components/BranchToolbar.logic";
import {
mergePullRequestListResults,
shouldPollPullRequestDetail,
type PullRequestEntry,
type PullRequestProjectFailure,
} from "~/components/pull-requests/pullRequests.logic";
Expand Down Expand Up @@ -84,10 +85,11 @@ function readPayload(input: PullRequestReadInput) {
}

/**
* The detail reads never poll: a pull request is a document the user reads,
* and the panel's own Refresh is the one thing that re-runs `gh`.
* The pace the header keeps itself current at while there is something to wait
* for: a running check, or a fresh push whose checks the host has not queued
* yet. The rest of the time it is a document the user reads, and the panel's
* own Refresh is the one thing that re-runs `gh`.
*/
/** While a check is still running the header keeps itself current at this pace. */
export const PULL_REQUEST_CHECKS_POLL_INTERVAL_MS = 20_000;

export function pullRequestDetailQueryOptions(input: PullRequestReadInput) {
Expand All @@ -101,10 +103,8 @@ export function pullRequestDetailQueryOptions(input: PullRequestReadInput) {
ensureEnvironmentApi(input.environmentId).pullRequests.detail(readPayload(input)),
staleTime: PULL_REQUEST_READ_STALE_TIME_MS,
refetchOnWindowFocus: false,
// A run in progress is the one moment the reader is watching the checks,
// so the header polls until every check has settled, then goes quiet.
refetchInterval: (query) =>
query.state.data?.checks.some((check) => check.status === "pending")
query.state.data !== undefined && shouldPollPullRequestDetail(query.state.data, Date.now())
? PULL_REQUEST_CHECKS_POLL_INTERVAL_MS
: false,
refetchIntervalInBackground: false,
Expand Down
Loading
Loading