diff --git a/apps/server/src/pullRequest/GitHubPullRequestProvider.test.ts b/apps/server/src/pullRequest/GitHubPullRequestProvider.test.ts index 06d85442..48a3f8eb 100644 --- a/apps/server/src/pullRequest/GitHubPullRequestProvider.test.ts +++ b/apps/server/src/pullRequest/GitHubPullRequestProvider.test.ts @@ -39,11 +39,6 @@ describe("GitHubPullRequestProvider.runAction", () => { input: { action: "update-branch", updateMethod: "rebase" }, args: ["pr", "update-branch", "12", "--repo", "octocat/example-app", "--rebase"], }, - { - name: "disarms auto-merge", - input: { action: "disable-auto-merge" }, - args: ["pr", "merge", "12", "--repo", "octocat/example-app", "--disable-auto"], - }, { name: "deletes the head branch after a merge when asked", input: { action: "merge", mergeMethod: "merge", deleteBranch: true }, @@ -64,11 +59,22 @@ describe("GitHubPullRequestProvider.runAction", () => { ); } - /** The host answers the readiness read with one status and every write with nothing. */ - const hostReports = (status: string) => { + /** + * The host answers the readiness read over GraphQL and every write with + * nothing. `isMergeQueueEnabled` is what the queue case turns on. + */ + const hostReports = (status: string, isMergeQueueEnabled = false) => { mockExecute.mockImplementation((input) => Effect.succeed( - processOutput(input.args[1] === "view" ? JSON.stringify({ mergeStateStatus: status }) : ""), + processOutput( + input.args[0] === "api" + ? JSON.stringify({ + data: { + repository: { pullRequest: { mergeStateStatus: status, isMergeQueueEnabled } }, + }, + }) + : "", + ), ), ); }; @@ -90,7 +96,6 @@ describe("GitHubPullRequestProvider.runAction", () => { }); assert.deepStrictEqual(prCalls(), [ - ["pr", "view", "12", "--repo", "octocat/example-app", "--json", "mergeStateStatus"], // The strategy travels with the standing instruction. ["pr", "merge", "12", "--repo", "octocat/example-app", "--auto", "--squash"], ]); @@ -112,13 +117,77 @@ describe("GitHubPullRequestProvider.runAction", () => { .pipe(Effect.flip); assert.equal(error.detail, "This pull request can merge right now. Use Merge instead."); - // Nothing ran but the read: the merge never reached the host. + // Nothing ran but the GraphQL read: the merge never reached the host. assert.deepStrictEqual( - prCalls().map((args) => args[1]), - ["view"], + calls().map((call) => call.args[0]), + ["api"], ); }).pipe(Effect.provide(layer)), ); + + /** The host answers the standing read with whether the pull request is queued. */ + const hostQueues = (isInMergeQueue: boolean) => { + mockExecute.mockImplementation((input) => + Effect.succeed( + processOutput( + input.args[0] === "api" && String(input.stdin).includes("isInMergeQueue") + ? JSON.stringify({ + data: { repository: { pullRequest: { id: "PR_kwDO123", isInMergeQueue } } }, + }) + : "", + ), + ), + ); + }; + + it.effect("disarms auto-merge through gh while the pull request is not queued", () => + Effect.gen(function* () { + hostQueues(false); + const provider = yield* GitHubPullRequestProvider.make(); + + yield* provider.runAction({ ...repository, number: 12, action: "disable-auto-merge" }); + + assert.deepStrictEqual(prCalls(), [ + ["pr", "merge", "12", "--repo", "octocat/example-app", "--disable-auto"], + ]); + }).pipe(Effect.provide(layer)), + ); + + it.effect("takes a queued pull request out of the merge queue by id instead", () => + Effect.gen(function* () { + hostQueues(true); + const provider = yield* GitHubPullRequestProvider.make(); + + yield* provider.runAction({ ...repository, number: 12, action: "disable-auto-merge" }); + + // gh's own disarm returns without touching a queued pull request, so the + // only write is the dequeue mutation and nothing reaches `gh pr`. + assert.deepStrictEqual(prCalls(), []); + const writes = calls().filter((call) => String(call.stdin).includes("dequeuePullRequest")); + assert.equal(writes.length, 1); + assert.match(String(writes[0]?.stdin), /"id":"PR_kwDO123"/); + }).pipe(Effect.provide(layer)), + ); + + it.effect("arms a green pull request whose base runs a merge queue", () => + Effect.gen(function* () { + hostReports("CLEAN", true); + const provider = yield* GitHubPullRequestProvider.make(); + + yield* provider.runAction({ + ...repository, + number: 12, + action: "enable-auto-merge", + mergeMethod: "squash", + }); + + // Under a queue this call adds the pull request to it, which is the + // whole point, so the refusal must not stand in the way. + assert.deepStrictEqual(prCalls(), [ + ["pr", "merge", "12", "--repo", "octocat/example-app", "--auto", "--squash"], + ]); + }).pipe(Effect.provide(layer)), + ); }); describe("GitHubPullRequestProvider.submitReview", () => { diff --git a/apps/server/src/pullRequest/GitHubPullRequestProvider.ts b/apps/server/src/pullRequest/GitHubPullRequestProvider.ts index bcd8f379..0725e7f3 100644 --- a/apps/server/src/pullRequest/GitHubPullRequestProvider.ts +++ b/apps/server/src/pullRequest/GitHubPullRequestProvider.ts @@ -32,7 +32,6 @@ import { import { decodeGitHubPullRequestActivityJson, decodeGitHubPullRequestDetailJson, - decodeGitHubImmediatelyMergeableJson, decodeGitHubRepositoryJson, GITHUB_PULL_REQUEST_ACTIVITY_FIELDS, GITHUB_PULL_REQUEST_DETAIL_FIELDS, @@ -40,19 +39,25 @@ import { import { ADD_REACTION_GRAPHQL_MUTATION, AUTHORED_PULL_REQUESTS_GRAPHQL_QUERY, - BASE_COMPARISON_GRAPHQL_QUERY, + AUTO_MERGE_READINESS_GRAPHQL_QUERY, buildGitHubReviewerRequestJson, buildGitHubReviewSubmissionJson, decodeGitHubAuthoredPullRequestsJson, - decodeGitHubBaseComparisonJson, + decodeGitHubDetailBaseStateJson, + decodeGitHubImmediatelyMergeableJson, + decodeGitHubMergeQueueStandingJson, decodeGitHubPullRequestConversationJson, decodeGitHubPullRequestNodeIdJson, decodeGitHubReviewerCandidatesJson, decodeGitHubSubjectScopeJson, + DEQUEUE_PULL_REQUEST_GRAPHQL_MUTATION, + DETAIL_BASE_STATE_GRAPHQL_QUERY, encodeGraphQlRequestJson, gitHubAuthoredSearchQuery, gitHubReactionContent, + type GitHubDetailBaseState, type GitHubGraphQlVariable, + MERGE_QUEUE_STANDING_GRAPHQL_QUERY, PULL_REQUEST_CONVERSATION_GRAPHQL_QUERY, PULL_REQUEST_NODE_ID_GRAPHQL_QUERY, REACTION_SUBJECT_SCOPE_GRAPHQL_QUERY, @@ -260,31 +265,42 @@ export const make = Effect.fn("makeGitHubPullRequestProvider")(function* () { stdin: encodeGraphQlRequestJson({ query: input.query, variables: input.variables }), }); + const graphqlRead = (input: { + readonly operation: string; + readonly cwd: string; + readonly query: string; + readonly variables: Readonly>; + readonly decode: (raw: string) => Result.Result>; + }) => + graphql(input).pipe( + Effect.flatMap((output) => { + const decoded = input.decode(output.stdout.trim()); + return Result.isSuccess(decoded) + ? Effect.succeed(decoded.success) + : Effect.fail(decodeError(input.operation, "GraphQL", decoded.failure)); + }), + ); + /** * Fails where GitHub would merge the pull request the moment it is armed. The * refusal is this app's own sentence, since the host never gets asked. + * + * A base guarded by a merge queue is never refused: there the same call adds + * the pull request to the queue rather than merging it, which is exactly what + * arming means under a queue. */ const refuseIfImmediatelyMergeable = ( input: ProviderRepositoryRef & { readonly number: number }, ) => - run({ + graphqlRead({ operation: "runAction", cwd: input.cwd, - args: [ - "pr", - "view", - String(input.number), - ...repositoryArgs(input), - "--json", - "mergeStateStatus", - ], + query: AUTO_MERGE_READINESS_GRAPHQL_QUERY, + variables: graphQlVariables(input), + decode: decodeGitHubImmediatelyMergeableJson, }).pipe( - Effect.flatMap((output) => { - const decoded = decodeGitHubImmediatelyMergeableJson(output.stdout.trim()); - if (!Result.isSuccess(decoded)) { - return Effect.fail(decodeError("runAction", "merge state", decoded.failure)); - } - return decoded.success + Effect.flatMap((immediatelyMergeable) => + immediatelyMergeable ? Effect.fail( new PullRequestProviderError({ provider: PROVIDER_KIND, @@ -293,24 +309,8 @@ export const make = Effect.fn("makeGitHubPullRequestProvider")(function* () { detail: "This pull request can merge right now. Use Merge instead.", }), ) - : Effect.void; - }), - ); - - const graphqlRead = (input: { - readonly operation: string; - readonly cwd: string; - readonly query: string; - readonly variables: Readonly>; - readonly decode: (raw: string) => Result.Result>; - }) => - graphql(input).pipe( - Effect.flatMap((output) => { - const decoded = input.decode(output.stdout.trim()); - return Result.isSuccess(decoded) - ? Effect.succeed(decoded.success) - : Effect.fail(decodeError(input.operation, "GraphQL", decoded.failure)); - }), + : Effect.void, + ), ); /** @@ -571,7 +571,7 @@ export const make = Effect.fn("makeGitHubPullRequestProvider")(function* () { graphqlRead({ operation: "detail", cwd: input.cwd, - query: BASE_COMPARISON_GRAPHQL_QUERY, + query: DETAIL_BASE_STATE_GRAPHQL_QUERY, variables: { ...graphQlVariables(input), headRef: @@ -579,15 +579,17 @@ export const make = Effect.fn("makeGitHubPullRequestProvider")(function* () { ? row.headBranch : `${row.headRepositoryOwnerLogin}:${row.headBranch}`, }, - decode: decodeGitHubBaseComparisonJson, + decode: decodeGitHubDetailBaseStateJson, }).pipe( - // A comparison the host will not make leaves the branch's freshness - // unknown; it is not worth failing a detail the reader can use. - Effect.catch(() => Effect.succeed(null)), - Effect.map((behindBy) => ({ + // A read the host will not answer leaves the branch's freshness + // unknown and says nothing about a queue; it is not worth failing a + // detail the reader can use. + Effect.catch(() => Effect.succeed({ behindBy: null })), + Effect.map((base) => ({ ...row, - baseComparison: toBaseComparison(behindBy), - behindBy, + baseComparison: toBaseComparison(base.behindBy), + behindBy: base.behindBy, + ...(base.mergeQueue === undefined ? {} : { mergeQueue: base.mergeQueue }), })), ), ), @@ -674,9 +676,33 @@ export const make = Effect.fn("makeGitHubPullRequestProvider")(function* () { // `gh pr merge --auto` merges outright when nothing is pending, which is // not what someone arming a merge asked for. The readiness is read first // and a ready pull request is sent back to the Merge button instead. - return input.action === "enable-auto-merge" - ? refuseIfImmediatelyMergeable(input).pipe(Effect.flatMap(action)) - : action(); + if (input.action === "enable-auto-merge") { + return refuseIfImmediatelyMergeable(input).pipe(Effect.flatMap(action)); + } + // `gh pr merge --disable-auto` stops short on a pull request the host has + // already taken into its merge queue and leaves it there, so that case + // is taken out of the queue by name instead. + if (input.action === "disable-auto-merge") { + return graphqlRead({ + operation: "runAction", + cwd: input.cwd, + query: MERGE_QUEUE_STANDING_GRAPHQL_QUERY, + variables: graphQlVariables(input), + decode: decodeGitHubMergeQueueStandingJson, + }).pipe( + Effect.flatMap((queuedId) => + queuedId === null + ? action() + : graphql({ + operation: "runAction", + cwd: input.cwd, + query: DEQUEUE_PULL_REQUEST_GRAPHQL_MUTATION, + variables: { id: queuedId }, + }).pipe(Effect.asVoid), + ), + ); + } + return action(); }, comment: (input) => diff --git a/apps/server/src/pullRequest/PullRequestProvider.ts b/apps/server/src/pullRequest/PullRequestProvider.ts index 665df32d..7a86bbeb 100644 --- a/apps/server/src/pullRequest/PullRequestProvider.ts +++ b/apps/server/src/pullRequest/PullRequestProvider.ts @@ -123,6 +123,12 @@ export interface ProviderChangeRequestDetail extends ProviderChangeRequest { readonly checks: ReadonlyArray; /** Absent where the host does not say whether its rules would take a merge. */ readonly mergeGate?: PullRequestMergeGate; + /** + * The merge queue guarding the base, where the host runs one. Absent where + * the base has no queue, or the host does not say. `position` is null until + * this change request has actually joined the queue. + */ + readonly mergeQueue?: { readonly position: number | null }; readonly baseComparison: PullRequestBaseComparison; /** Null where the host could not compare the branch with its base. */ readonly behindBy: number | null; diff --git a/apps/server/src/pullRequest/PullRequestService.ts b/apps/server/src/pullRequest/PullRequestService.ts index 20875c7a..5310a7ab 100644 --- a/apps/server/src/pullRequest/PullRequestService.ts +++ b/apps/server/src/pullRequest/PullRequestService.ts @@ -467,6 +467,7 @@ function toDetail(input: { checks: row.checks, ...(row.checksState === undefined ? {} : { checksState: row.checksState }), ...(row.mergeGate === undefined ? {} : { mergeGate: row.mergeGate }), + ...(row.mergeQueue === undefined ? {} : { mergeQueue: row.mergeQueue }), viewer: { canWrite: input.repository.canWrite, canReview: viewerKnown && !viewerIsAuthor, diff --git a/apps/server/src/pullRequest/gitHubPullRequestDetail.ts b/apps/server/src/pullRequest/gitHubPullRequestDetail.ts index a33a2bf2..81ed78ae 100644 --- a/apps/server/src/pullRequest/gitHubPullRequestDetail.ts +++ b/apps/server/src/pullRequest/gitHubPullRequestDetail.ts @@ -85,20 +85,6 @@ const GitHubRepositorySchema = Schema.Struct({ default_branch: Schema.optional(Schema.NullOr(Schema.String)), }); -/** - * `gh pr view --json mergeStateStatus`: the one word GitHub sums a pull - * request's readiness into. - */ -const GitHubMergeStateSchema = Schema.Struct({ - mergeStateStatus: Schema.optional(Schema.NullOr(Schema.String)), -}); - -/** - * The states `gh pr merge --auto` merges outright instead of arming, copied - * from the CLI: nothing is pending, so there is nothing to wait for. - */ -const IMMEDIATELY_MERGEABLE_STATES = new Set(["CLEAN", "HAS_HOOKS", "UNSTABLE"]); - /** A `gh pr view` row: everything a list row carries, plus the detail fields. */ export interface GitHubPullRequestDetailRow extends GitHubPullRequestListRow { readonly body: string; @@ -388,7 +374,6 @@ function normalizeCommits( const decodeDetailPayload = decodeJsonResult(GitHubPullRequestDetailRowSchema); const decodeActivityPayload = decodeJsonResult(GitHubPullRequestActivitySchema); const decodeRepositoryPayload = decodeJsonResult(GitHubRepositorySchema); -const decodeMergeStatePayload = decodeJsonResult(GitHubMergeStateSchema); /** * Decodes `gh api repos//` into the viewer's access and the merge @@ -420,22 +405,6 @@ export function decodeGitHubRepositoryJson( }); } -/** - * Whether GitHub would merge the pull request this instant, read from - * `gh pr view --json mergeStateStatus`. A status the host did not name is not - * ready: arming then waits, which is the safe way to be wrong. - */ -export function decodeGitHubImmediatelyMergeableJson( - raw: string, -): Result.Result> { - const payload = decodeMergeStatePayload(raw); - if (!Result.isSuccess(payload)) { - return Result.fail(payload.failure); - } - const status = nonEmptyText(payload.success.mergeStateStatus); - return Result.succeed(status !== null && IMMEDIATELY_MERGEABLE_STATES.has(status)); -} - /** Decodes `gh pr view --json ` into the header the panel renders. */ export function decodeGitHubPullRequestDetailJson( raw: string, diff --git a/apps/server/src/pullRequest/gitHubPullRequestGraphql.test.ts b/apps/server/src/pullRequest/gitHubPullRequestGraphql.test.ts index b15d2eac..7143e785 100644 --- a/apps/server/src/pullRequest/gitHubPullRequestGraphql.test.ts +++ b/apps/server/src/pullRequest/gitHubPullRequestGraphql.test.ts @@ -4,7 +4,8 @@ import * as Result from "effect/Result"; import { decodeGitHubAuthoredPullRequestsJson, - decodeGitHubBaseComparisonJson, + decodeGitHubDetailBaseStateJson, + decodeGitHubImmediatelyMergeableJson, decodeGitHubPullRequestConversationJson, decodeGitHubReviewerCandidatesJson, decodeGitHubSubjectScopeJson, @@ -143,31 +144,70 @@ describe("decodeGitHubPullRequestConversationJson", () => { }); }); -describe("decodeGitHubBaseComparisonJson", () => { - it("counts the commits the base is ahead by", () => { - assert.equal( - decoded( - decodeGitHubBaseComparisonJson( - JSON.stringify({ - data: { repository: { pullRequest: { baseRef: { compare: { behindBy: 7 } } } } }, - }), - ), - "base comparison", - ), - 7, +describe("decodeGitHubDetailBaseStateJson", () => { + const baseState = (pullRequest: unknown) => + decoded( + decodeGitHubDetailBaseStateJson(JSON.stringify({ data: { repository: { pullRequest } } })), + "detail base state", ); + + it("counts the commits the base is ahead by", () => { + assert.deepStrictEqual(baseState({ baseRef: { compare: { behindBy: 7 } } }), { behindBy: 7 }); }); it("reads a comparison the host would not make as unknown", () => { - assert.equal( - decoded( - decodeGitHubBaseComparisonJson( - JSON.stringify({ data: { repository: { pullRequest: { baseRef: null } } } }), - ), - "base comparison", + assert.deepStrictEqual(baseState({ baseRef: null }), { behindBy: null }); + }); + + it("names the queue on a guarded base, and where this pull request sits in it", () => { + assert.deepStrictEqual( + baseState({ + baseRef: { compare: { behindBy: 2 } }, + isMergeQueueEnabled: true, + isInMergeQueue: true, + mergeQueueEntry: { position: 3 }, + }), + { behindBy: 2, mergeQueue: { position: 3 } }, + ); + }); + + it("keeps the queue but no place in it while the pull request has not joined", () => { + assert.deepStrictEqual( + baseState({ + baseRef: { compare: { behindBy: 0 } }, + isMergeQueueEnabled: true, + isInMergeQueue: false, + mergeQueueEntry: null, + }), + { behindBy: 0, mergeQueue: { position: null } }, + ); + }); + + it("says nothing about a queue where the host named no queue fields at all", () => { + assert.deepStrictEqual(baseState({ baseRef: { compare: { behindBy: 1 } } }), { behindBy: 1 }); + }); +}); + +describe("decodeGitHubImmediatelyMergeableJson", () => { + const readiness = (pullRequest: unknown) => + decoded( + decodeGitHubImmediatelyMergeableJson( + JSON.stringify({ data: { repository: { pullRequest } } }), ), - null, + "auto-merge readiness", ); + + it("reports a green pull request on an unguarded base as one that merges now", () => { + assert.equal(readiness({ mergeStateStatus: "CLEAN", isMergeQueueEnabled: false }), true); + assert.equal(readiness({ mergeStateStatus: "BLOCKED", isMergeQueueEnabled: false }), false); + }); + + it("never reports one on a queued base, where arming means joining the queue", () => { + assert.equal(readiness({ mergeStateStatus: "CLEAN", isMergeQueueEnabled: true }), false); + }); + + it("treats a status the host would not name as one that waits", () => { + assert.equal(readiness({}), false); }); }); diff --git a/apps/server/src/pullRequest/gitHubPullRequestGraphql.ts b/apps/server/src/pullRequest/gitHubPullRequestGraphql.ts index 42e4c317..dad70eec 100644 --- a/apps/server/src/pullRequest/gitHubPullRequestGraphql.ts +++ b/apps/server/src/pullRequest/gitHubPullRequestGraphql.ts @@ -78,23 +78,57 @@ export const PULL_REQUEST_CONVERSATION_GRAPHQL_QUERY = `query($owner: String!, $ }`; /** - * How far the head branch trails its base. + * Everything about the base a detail read needs and `gh pr view --json` cannot + * report: how far the head branch trails it, and whether it is guarded by a + * merge queue. One document, so the detail costs one GraphQL request. * - * `mergeStateStatus` is not the answer: GitHub only reports BEHIND where the - * repository requires branches to be current before merging. The comparison - * counts the commits instead, which is the number GitHub's own banner shows. + * `mergeStateStatus` is not the freshness answer: GitHub only reports BEHIND + * where the repository requires branches to be current before merging. The + * comparison counts the commits instead, which is the number GitHub's own + * banner shows. * * `headRef` is qualified `owner:branch` because a branch on a fork has no name * of its own in the base repository. */ -export const BASE_COMPARISON_GRAPHQL_QUERY = `query($owner: String!, $name: String!, $number: Int!, $headRef: String!) { +export const DETAIL_BASE_STATE_GRAPHQL_QUERY = `query($owner: String!, $name: String!, $number: Int!, $headRef: String!) { repository(owner: $owner, name: $name) { pullRequest(number: $number) { baseRef { compare(headRef: $headRef) { behindBy } } + isMergeQueueEnabled + isInMergeQueue + mergeQueueEntry { position } } } }`; +/** + * What arming a standing merge instruction has to know first: whether GitHub + * would take the merge this instant, and whether the base runs a merge queue. + * A queue turns `gh pr merge --auto` on a green pull request into "join the + * queue", which is the point, so only a base without one is refused. + */ +export const AUTO_MERGE_READINESS_GRAPHQL_QUERY = `query($owner: String!, $name: String!, $number: Int!) { + repository(owner: $owner, name: $name) { + pullRequest(number: $number) { mergeStateStatus isMergeQueueEnabled } + } +}`; + +/** + * What disarming has to know first: whether the host has already taken the + * pull request into its merge queue. `gh pr merge --disable-auto` stops short + * on a queued pull request without touching it, so that case is undone by the + * dequeue mutation instead, which is addressed by node id. + */ +export const MERGE_QUEUE_STANDING_GRAPHQL_QUERY = `query($owner: String!, $name: String!, $number: Int!) { + repository(owner: $owner, name: $name) { + pullRequest(number: $number) { id isInMergeQueue } + } +}`; + +export const DEQUEUE_PULL_REQUEST_GRAPHQL_MUTATION = `mutation($id: ID!) { + dequeuePullRequest(input: { id: $id }) { mergeQueueEntry { id } } +}`; + /** Labels and outstanding review requests are short lists; this is room to spare. */ const AUTHORED_CONNECTION_PAGE_SIZE = 20; @@ -486,7 +520,7 @@ export function decodeGitHubPullRequestConversationJson( }); } -const RawBaseComparisonSchema = Schema.Struct({ +const RawDetailBaseStateSchema = Schema.Struct({ data: Schema.Struct({ repository: Schema.NullOr( Schema.Struct({ @@ -504,6 +538,14 @@ const RawBaseComparisonSchema = Schema.Struct({ }), ), ), + isMergeQueueEnabled: Schema.optional(Schema.NullOr(Schema.Boolean)), + isInMergeQueue: Schema.optional(Schema.NullOr(Schema.Boolean)), + /** Null until the pull request actually joins the queue. */ + mergeQueueEntry: Schema.optional( + Schema.NullOr( + Schema.Struct({ position: Schema.optional(Schema.NullOr(Schema.Number)) }), + ), + ), }), ), }), @@ -511,20 +553,119 @@ const RawBaseComparisonSchema = Schema.Struct({ }), }); -const decodeBaseComparison = decodeJsonResult(RawBaseComparisonSchema); +const decodeDetailBaseState = decodeJsonResult(RawDetailBaseStateSchema); + +/** Where the base stands: how far ahead of the head, and what guards it. */ +export interface GitHubDetailBaseState { + /** How many commits the base has that the head does not; null when unanswerable. */ + readonly behindBy: number | null; + /** Present only where the base requires a merge queue. */ + readonly mergeQueue?: { readonly position: number | null }; +} -/** How many commits the base has that the head does not; null when unanswerable. */ -export function decodeGitHubBaseComparisonJson( +/** + * Decodes the one GraphQL read a detail makes about its base. The queue is + * reported only where the base requires one, and its position only once this + * pull request has actually joined it. + */ +export function decodeGitHubDetailBaseStateJson( raw: string, -): Result.Result { - const decoded = decodeBaseComparison(raw); +): Result.Result { + const decoded = decodeDetailBaseState(raw); if (!Result.isSuccess(decoded)) { return Result.fail(decoded.failure); } - const behindBy = decoded.success.data.repository?.pullRequest?.baseRef?.compare?.behindBy; - return Result.succeed( - typeof behindBy === "number" && behindBy >= 0 ? Math.trunc(behindBy) : null, - ); + const pullRequest = decoded.success.data.repository?.pullRequest ?? null; + const behindByRaw = pullRequest?.baseRef?.compare?.behindBy; + const behindBy = + typeof behindByRaw === "number" && behindByRaw >= 0 ? Math.trunc(behindByRaw) : null; + if (pullRequest?.isMergeQueueEnabled !== true) { + return Result.succeed({ behindBy }); + } + const positionRaw = pullRequest.mergeQueueEntry?.position; + const position = + pullRequest.isInMergeQueue === true && typeof positionRaw === "number" && positionRaw > 0 + ? Math.trunc(positionRaw) + : null; + return Result.succeed({ behindBy, mergeQueue: { position } }); +} + +/** + * The states `gh pr merge --auto` merges outright instead of arming, copied + * from the CLI: nothing is pending, so there is nothing to wait for. + */ +const IMMEDIATELY_MERGEABLE_STATES = new Set(["CLEAN", "HAS_HOOKS", "UNSTABLE"]); + +const RawAutoMergeReadinessSchema = Schema.Struct({ + data: Schema.Struct({ + repository: Schema.NullOr( + Schema.Struct({ + pullRequest: Schema.NullOr( + Schema.Struct({ + mergeStateStatus: Schema.optional(Schema.NullOr(Schema.String)), + isMergeQueueEnabled: Schema.optional(Schema.NullOr(Schema.Boolean)), + }), + ), + }), + ), + }), +}); + +const decodeAutoMergeReadiness = decodeJsonResult(RawAutoMergeReadinessSchema); + +/** + * Whether arming this pull request would merge it on the spot instead. True + * only where GitHub would take the merge this instant and the base runs no + * merge queue: under a queue the same call joins the queue, which is what the + * reader asked for. A status the host did not name is not ready, so arming + * then waits, which is the safe way to be wrong. + */ +export function decodeGitHubImmediatelyMergeableJson( + raw: string, +): Result.Result { + const decoded = decodeAutoMergeReadiness(raw); + if (!Result.isSuccess(decoded)) { + return Result.fail(decoded.failure); + } + const pullRequest = decoded.success.data.repository?.pullRequest ?? null; + if (pullRequest?.isMergeQueueEnabled === true) { + return Result.succeed(false); + } + const status = nonEmptyText(pullRequest?.mergeStateStatus)?.toUpperCase() ?? null; + return Result.succeed(status !== null && IMMEDIATELY_MERGEABLE_STATES.has(status)); +} + +const RawMergeQueueStandingSchema = Schema.Struct({ + data: Schema.Struct({ + repository: Schema.NullOr( + Schema.Struct({ + pullRequest: Schema.NullOr( + Schema.Struct({ + id: Schema.optional(Schema.NullOr(Schema.String)), + isInMergeQueue: Schema.optional(Schema.NullOr(Schema.Boolean)), + }), + ), + }), + ), + }), +}); + +const decodeMergeQueueStanding = decodeJsonResult(RawMergeQueueStandingSchema); + +/** + * The node id to dequeue while the pull request sits in a merge queue, or null + * where it does not: a host that names no queue, or no id, has nothing to take + * it out of, and the plain disarm is the right call. + */ +export function decodeGitHubMergeQueueStandingJson( + raw: string, +): Result.Result { + const decoded = decodeMergeQueueStanding(raw); + if (!Result.isSuccess(decoded)) { + return Result.fail(decoded.failure); + } + const pullRequest = decoded.success.data.repository?.pullRequest ?? null; + return Result.succeed(pullRequest?.isInMergeQueue === true ? nonEmptyText(pullRequest.id) : null); } const RawAuthoredNodeSchema = Schema.Struct({ diff --git a/apps/web/src/components/pull-requests/PullRequestDetailPanel.browser.tsx b/apps/web/src/components/pull-requests/PullRequestDetailPanel.browser.tsx index 52a5808f..23a7c83f 100644 --- a/apps/web/src/components/pull-requests/PullRequestDetailPanel.browser.tsx +++ b/apps/web/src/components/pull-requests/PullRequestDetailPanel.browser.tsx @@ -570,6 +570,51 @@ describe("PullRequestDetailPanel", () => { await behind.cleanup(); }); + it("offers one Merge when ready button where the base runs a merge queue", async () => { + const armable = await renderPanel({ + detail: { + viewer: { canWrite: true, canReview: false, canManage: true }, + // Behind its base and blocked on a rule: both are the queue's own work, + // so neither displaces the button nor turns it off. + mergeQueue: { position: null }, + baseComparison: "behind", + behindBy: 3, + mergeGate: "blocked", + }, + }); + + await expect.element(page.getByTestId("pull-request-merge-when-ready")).toBeEnabled(); + expect(page.getByTestId("pull-request-merge").elements()).toHaveLength(0); + expect(page.getByTestId("pull-request-update-branch").elements()).toHaveLength(0); + await userEvent.click(page.getByTestId("pull-request-merge-when-ready")); + + await vi.waitFor(() => { + expect(armable.runAction).toHaveBeenCalledWith({ + ...REFERENCE, + action: "enable-auto-merge", + // The queue picks how it lands; the method goes along and is ignored. + mergeMethod: "squash", + }); + }); + + await armable.cleanup(); + + const queued = await renderPanel({ + detail: { + viewer: { canWrite: true, canReview: false, canManage: true }, + mergeQueue: { position: 2 }, + }, + }); + + // The host owns it from here, so the header states where it stands and + // there is no button offering to start it again. + await expect.element(page.getByText("Queued, 2nd")).toBeVisible(); + expect(page.getByTestId("pull-request-merge-when-ready").elements()).toHaveLength(0); + expect(page.getByTestId("pull-request-merge").elements()).toHaveLength(0); + + await queued.cleanup(); + }); + it("keeps Merge on screen but off while the host's rules refuse it, and says why", async () => { const rendered = await renderPanel({ detail: { diff --git a/apps/web/src/components/pull-requests/PullRequestDetailPanel.tsx b/apps/web/src/components/pull-requests/PullRequestDetailPanel.tsx index c3706210..b70a9002 100644 --- a/apps/web/src/components/pull-requests/PullRequestDetailPanel.tsx +++ b/apps/web/src/components/pull-requests/PullRequestDetailPanel.tsx @@ -102,8 +102,10 @@ import { formatPullRequestBehindLabel, formatPullRequestChecksHeadline, pullRequestBadgeTone, + pullRequestMergeQueueLabel, pullRequestUpdateMethodLabel, resolveDefaultMergeMethod, + resolveMergeWhenReadyBlock, resolvePullRequestMergeBlock, summarizePullRequestChecks, type PullRequestChecksSummary, @@ -609,6 +611,7 @@ function PullRequestDetailHeader({ detail.state === "open" && !detail.isDraft && detail.mergeability === "conflicting" ? `Conflicts with ${detail.baseBranch}` : null; + const queueLabel = pullRequestMergeQueueLabel(detail); const behindLabel = formatPullRequestBehindLabel(detail); const freshness = formatPullRequestBaseFreshness(detail); // Open is the resting state and the glyph already says it; the other three @@ -641,7 +644,13 @@ function PullRequestDetailHeader({ {stateWord ? {stateWord} : null} - {detail.autoMergeEnabled === true ? ( + {/* A base with a merge queue says where the host has taken it, since + the queue, not the standing instruction, is what lands it now. */} + {queueLabel !== null ? ( + + {queueLabel.label} + + ) : detail.mergeQueue === undefined && detail.autoMergeEnabled === true ? ( Auto-merge on ) : null} @@ -1148,37 +1157,82 @@ function usePullRequestActions({ const showClose = canManage && isOpen && allows("close"); const showReopen = canManage && isReopenable && allows("reopen"); const showDraftToggle = canManage && isOpen && allows(detail.isDraft ? "ready" : "draft"); + + // A base guarded by a merge queue: the queue picks the method, brings the + // branch current itself, and refuses a direct merge, so the only thing to + // offer is joining it. The queue's own position says whether it has been. + const queue = detail.mergeQueue ?? null; + const isQueued = queue !== null && queue.position !== null; + // A queued pull request need not carry a standing instruction any more, and + // the same call is what takes it back out, so the way out is offered on the + // queue's own say-so. const showDisableAutoMerge = - canWrite && isOpen && detail.autoMergeEnabled === true && allows("disable-auto-merge"); + canWrite && + isOpen && + allows("disable-auto-merge") && + (detail.autoMergeEnabled === true || isQueued); const showEnableAutoMerge = - canWrite && isOpen && detail.autoMergeEnabled !== true && allows("enable-auto-merge"); + canWrite && + isOpen && + detail.autoMergeEnabled !== true && + allows("enable-auto-merge") && + // Nothing left to arm once the host has taken it into the queue. + !isQueued; + const showMergeWhenReady = queue !== null && showEnableAutoMerge; + // Outside a queue the arming lives in the menu; under one it is the button. + const showEnableAutoMergeInMenu = queue === null && showEnableAutoMerge; const updateMethods = detail.capabilities.updateMethods; const mergeBlock = resolvePullRequestMergeBlock(detail); const mergeDisabled = isRunning || mergeBlock !== null; + // Waiting on checks or on a stale base is the queue's own job, so the only + // things that stop it are the two it cannot fix: a draft, and a conflict. + const mergeWhenReadyBlock = resolveMergeWhenReadyBlock(detail); const defaultMergeMethod = resolveDefaultMergeMethod(detail.mergeMethods, rememberedMergeMethod); const canUpdateBranch = canWrite && isOpen && allows("update-branch"); const isBehind = detail.baseComparison === "behind"; + // Someone may still want the branch current for local testing, so the update + // stays reachable under a queue; it simply must not lead. + const showUpdateBranchInMenu = queue !== null && canUpdateBranch && isBehind; // One primary action for the state the branch is in: work it cannot merge // through first, then bringing it up to date, then the merge itself. The // displaced merge is not lost — it moves into the menu beside the rest. - const primary: "resolve-conflicts" | "update-branch" | "merge" | null = + // Under a queue the last two collapse into one button, and a pull request + // already in the queue has no primary at all: the host owns it now. + const primary: "resolve-conflicts" | "update-branch" | "merge" | "merge-when-ready" | null = isOpen && !detail.isDraft && detail.mergeability === "conflicting" && handoffs?.resolveConflicts ? "resolve-conflicts" - : canUpdateBranch && isBehind && detail.mergeability !== "conflicting" - ? "update-branch" - : showMerge - ? "merge" - : null; - const showMergeInMenu = showMerge && primary !== "merge" && mergeBlock === null; + : queue !== null + ? showMergeWhenReady + ? "merge-when-ready" + : null + : canUpdateBranch && isBehind && detail.mergeability !== "conflicting" + ? "update-branch" + : showMerge + ? "merge" + : null; + // A queue refuses a direct merge, so the merge never reaches the menu either. + const showMergeInMenu = queue === null && showMerge && primary !== "merge" && mergeBlock === null; const hasMenuWrites = - showDraftToggle || showDisableAutoMerge || showEnableAutoMerge || showMergeInMenu; + showDraftToggle || + showDisableAutoMerge || + showEnableAutoMergeInMenu || + showMergeInMenu || + showUpdateBranchInMenu; // The two menu halves: the hand-offs, and the writes that are not buttons. const hasMenu = handoffs !== null || hasMenuWrites; + // Under a queue two writes swap places: the update becomes a menu item and + // arming becomes the primary button. A write with a button of its own says so + // on that button; everything else says so on the line under the header. + const runningOnItsOwnButton = + (runningAction === "enable-auto-merge" && showMergeWhenReady) || + (runningAction === "update-branch" && !showUpdateBranchInMenu); const menuRunningWord = - runningAction !== null && MENU_ACTIONS.has(runningAction) + runningAction !== null && + !runningOnItsOwnButton && + (MENU_ACTIONS.has(runningAction) || runningAction === "update-branch") ? RUNNING_ACTION_WORDS[runningAction] : null; // A hairline between the two halves would be a colour this app does not @@ -1225,6 +1279,20 @@ function usePullRequestActions({ ); + // One button, no method picker and no confirmation: the queue's own setting + // decides how it lands, and joining a queue is undone by leaving it. + const mergeWhenReadyControl = ( + + ); + const updateBranchControl = updateMethods.length > 1 ? ( @@ -1283,6 +1351,18 @@ function usePullRequestActions({ ) : null} {primary === "update-branch" ? updateBranchControl : null} + {primary === "merge-when-ready" ? ( + // The queue fixes a stale base and waits out the checks itself, so + // the only reasons this is off are a draft and a conflict — both + // said on the button rather than by hiding it. + mergeWhenReadyBlock === null ? ( + mergeWhenReadyControl + ) : ( + + {mergeWhenReadyControl} + + ) + ) : null} {primary === "merge" ? ( // A blocked merge stays on screen and says what is in the way: the // fix is on the host, and a vanished button explains nothing. @@ -1363,9 +1443,40 @@ function usePullRequestActions({ ) : null} {(handoffs || showMergeInMenu) && - (showDraftToggle || showDisableAutoMerge || showEnableAutoMerge) ? ( + (showDraftToggle || + showDisableAutoMerge || + showEnableAutoMergeInMenu || + showUpdateBranchInMenu) ? ( ) : null} + {/* Under a queue the update no longer leads, because the queue + builds its own merge on the latest base; it stays here for + anyone who wants the branch current to test it locally. */} + {showUpdateBranchInMenu ? ( + updateMethods.length > 1 ? ( + updateMethods.map((method) => ( + run("update-branch", { updateMethod: method })} + > + {pullRequestUpdateMethodLabel(method, detail.baseBranch)} + + )) + ) : ( + + run( + "update-branch", + updateMethods[0] ? { updateMethod: updateMethods[0] } : undefined, + ) + } + > + Update branch + + ) + ) : null} {showDraftToggle ? ( detail.isDraft ? ( run("ready")}>Mark as ready @@ -1378,10 +1489,14 @@ function usePullRequestActions({ data-testid="pull-request-disable-auto-merge" onClick={() => run("disable-auto-merge")} > - Disable auto-merge + {queue === null + ? "Disable auto-merge" + : isQueued + ? "Leave the merge queue" + : "Cancel merge when ready"} ) : null} - {showEnableAutoMerge ? ( + {showEnableAutoMergeInMenu ? ( run("enable-auto-merge", { mergeMethod: defaultMergeMethod })} @@ -1396,6 +1511,12 @@ function usePullRequestActions({ ); + // Whatever is holding the primary button back, whichever button it is. A + // disabled button cannot be focused, so this is written out as well as + // tucked in the button's own tooltip. + const primaryBlock = + primary === "merge" ? mergeBlock : primary === "merge-when-ready" ? mergeWhenReadyBlock : null; + const notices = ( <> {/* The menu is gone by the time the host answers, so its running action @@ -1410,14 +1531,12 @@ function usePullRequestActions({ > {menuRunningWord}

- {/* 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 ? ( + {primaryBlock !== null ? (

- {mergeBlock} + {primaryBlock}

) : null} {mutation.isError ? ( diff --git a/apps/web/src/components/pull-requests/pullRequests.logic.test.ts b/apps/web/src/components/pull-requests/pullRequests.logic.test.ts index 75503b6a..cf214b9d 100644 --- a/apps/web/src/components/pull-requests/pullRequests.logic.test.ts +++ b/apps/web/src/components/pull-requests/pullRequests.logic.test.ts @@ -39,7 +39,9 @@ import { pullRequestFiltersFromSearch, pullRequestFiltersToSearch, pullRequestLabelColor, + pullRequestMergeQueueLabel, resolveDefaultMergeMethod, + resolveMergeWhenReadyBlock, resolveNeedsYouReason, resolvePullRequestMergeBlock, resolvePullRequestReviewPosition, @@ -1019,6 +1021,53 @@ describe("resolvePullRequestMergeBlock", () => { }); }); +describe("resolveMergeWhenReadyBlock", () => { + it("stops only for what a merge queue cannot fix for itself", () => { + expect(resolveMergeWhenReadyBlock({ mergeability: "mergeable", isDraft: true })).toBe( + "Mark as ready first", + ); + expect(resolveMergeWhenReadyBlock({ mergeability: "conflicting", isDraft: false })).toBe( + "Resolve the conflicts first", + ); + // A stale base and a rule still waiting are the queue's own work: it + // builds its merge on the latest base and waits the checks out there. + expect(resolveMergeWhenReadyBlock({ mergeability: "mergeable", isDraft: false })).toBeNull(); + }); +}); + +describe("pullRequestMergeQueueLabel", () => { + const base = { baseBranch: "main", autoMergeEnabled: false }; + + it("says nothing where the base runs no queue", () => { + expect(pullRequestMergeQueueLabel({ ...base, autoMergeEnabled: true })).toBeNull(); + }); + + it("counts a place in the queue in English, and leaves first place bare", () => { + const at = (position: number) => + pullRequestMergeQueueLabel({ ...base, mergeQueue: { position } })?.label; + expect(at(1)).toBe("Queued"); + expect(at(2)).toBe("Queued, 2nd"); + expect(at(3)).toBe("Queued, 3rd"); + expect(at(4)).toBe("Queued, 4th"); + expect(at(11)).toBe("Queued, 11th"); + expect(at(21)).toBe("Queued, 21st"); + expect(pullRequestMergeQueueLabel({ ...base, mergeQueue: { position: 2 } })?.tooltip).toBe( + "In the merge queue for main", + ); + }); + + it("says what an armed pull request is waiting to do, and nothing when it is not armed", () => { + expect( + pullRequestMergeQueueLabel({ + ...base, + autoMergeEnabled: true, + mergeQueue: { position: null }, + })?.label, + ).toBe("Merge when ready"); + expect(pullRequestMergeQueueLabel({ ...base, mergeQueue: { position: null } })).toBeNull(); + }); +}); + describe("shouldPollPullRequestDetail", () => { const now = Date.parse("2026-09-04T12:00:00.000Z"); const check = (status: PullRequestCheck["status"]) => ({ @@ -1056,6 +1105,18 @@ describe("shouldPollPullRequestDetail", () => { shouldPollPullRequestDetail({ ...justPushed, state: "merged" as const, checks: [] }, now), ).toBe(false); }); + + it("keeps watching a pull request the host has taken into its merge queue", () => { + const inQueue = { ...settled, checks: [check("success")] }; + expect(shouldPollPullRequestDetail({ ...inQueue, mergeQueue: { position: 2 } }, now)).toBe( + true, + ); + // Merely armed under a queue is a settled state: nothing moves until the + // requirements pass, which the checks already say. + expect(shouldPollPullRequestDetail({ ...inQueue, mergeQueue: { position: null } }, now)).toBe( + false, + ); + }); }); describe("handing a review comment to a thread", () => { diff --git a/apps/web/src/components/pull-requests/pullRequests.logic.ts b/apps/web/src/components/pull-requests/pullRequests.logic.ts index 9ddc507f..d7ae7fa4 100644 --- a/apps/web/src/components/pull-requests/pullRequests.logic.ts +++ b/apps/web/src/components/pull-requests/pullRequests.logic.ts @@ -1451,6 +1451,67 @@ export function resolvePullRequestMergeBlock( return null; } +/** + * Why "Merge when ready" is off the table right now, or null when it is + * available. Only the two things a merge queue cannot fix for itself: the queue + * builds its own merge on top of the latest base, so a branch that is behind or + * waiting on a rule is exactly what it is for. + */ +export function resolveMergeWhenReadyBlock( + detail: Pick, +): string | null { + if (detail.isDraft) { + return "Mark as ready first"; + } + if (detail.mergeability === "conflicting") { + return "Resolve the conflicts first"; + } + return null; +} + +/** `1` reads as first and needs no ordinal; the rest are 2nd, 3rd, 4th… */ +function englishOrdinal(value: number): string { + const remainderOfTen = value % 10; + const remainderOfHundred = value % 100; + if (remainderOfTen === 1 && remainderOfHundred !== 11) { + return `${value}st`; + } + if (remainderOfTen === 2 && remainderOfHundred !== 12) { + return `${value}nd`; + } + if (remainderOfTen === 3 && remainderOfHundred !== 13) { + return `${value}rd`; + } + return `${value}th`; +} + +/** + * The header's word for a pull request whose base runs a merge queue, or null + * where there is nothing to say. Queued leads with where it stands, since that + * is the only part that moves; armed says what it is waiting to do. Outside a + * queue this says nothing and the plain auto-merge word stands. + */ +export function pullRequestMergeQueueLabel( + detail: Pick, +): { readonly label: string; readonly tooltip: string } | null { + if (detail.mergeQueue === undefined) { + return null; + } + const position = detail.mergeQueue.position; + if (position !== null) { + return { + label: position <= 1 ? "Queued" : `Queued, ${englishOrdinal(position)}`, + tooltip: `In the merge queue for ${detail.baseBranch}`, + }; + } + return detail.autoMergeEnabled === true + ? { + label: "Merge when ready", + tooltip: "Joins the merge queue once every requirement passes", + } + : 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; @@ -1459,10 +1520,12 @@ export const PULL_REQUEST_FRESH_PUSH_WATCH_MS = 120_000; * 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. + * and would otherwise sit on that answer until the user hit Refresh. A pull + * request sitting in a merge queue is the third: its place in the queue moves + * on its own, and the host lands it without anyone here asking. */ export function shouldPollPullRequestDetail( - detail: Pick, + detail: Pick, now: number, ): boolean { if (detail.checks.some((check) => check.status === "pending")) { @@ -1471,6 +1534,9 @@ export function shouldPollPullRequestDetail( if (detail.state !== "open") { return false; } + if (detail.mergeQueue !== undefined && detail.mergeQueue.position !== null) { + return true; + } const unsettled = detail.checks.length === 0 || detail.mergeability === "unknown"; const updatedAt = Date.parse(detail.updatedAt); return ( diff --git a/packages/contracts/src/pullRequest.ts b/packages/contracts/src/pullRequest.ts index 08da200e..7189baf3 100644 --- a/packages/contracts/src/pullRequest.ts +++ b/packages/contracts/src/pullRequest.ts @@ -423,6 +423,17 @@ export const PullRequestDetail = Schema.Struct({ checksState: Schema.optionalKey(PullRequestChecksState), /** Absent where the host does not say, or has not decided yet after a push. */ mergeGate: Schema.optionalKey(PullRequestMergeGate), + /** + * The base branch's merge queue, where the host has one: the queue decides the + * merge method and brings the branch current itself. Absent where the base has + * no queue, or the host does not say. + */ + mergeQueue: Schema.optionalKey( + Schema.Struct({ + /** Where this pull request stands in the queue, first is 1; null while it is not queued. */ + position: Schema.NullOr(PositiveInt), + }), + ), viewer: PullRequestViewerPermissions, /** The methods the repository allows, in the order merge, squash, rebase. */ mergeMethods: Schema.Array(PullRequestMergeMethod),