From d88f5c66ff0a159f6997db1846c936602189403c Mon Sep 17 00:00:00 2001 From: Alex Ross <38270282+alexr00@users.noreply.github.com> Date: Thu, 1 Oct 2026 08:24:22 +0200 Subject: [PATCH] Fix "merge stack" button showing when it shouldn't --- src/github/interface.ts | 13 ++++ src/github/pullRequestModel.ts | 4 ++ src/test/github/pullRequestModel.test.ts | 35 ++++++++++ webviews/components/merge.tsx | 6 +- webviews/editorWebview/test/overview.test.tsx | 64 ++++++++++++++++++- 5 files changed, 117 insertions(+), 5 deletions(-) diff --git a/src/github/interface.ts b/src/github/interface.ts index 6ae32f87a5..ac04839e69 100644 --- a/src/github/interface.ts +++ b/src/github/interface.ts @@ -47,6 +47,19 @@ export enum PullRequestMergeability { Behind, } +export function isStackMergeable(stack: PullRequestStack, number: number): boolean { + const current = stack.pullRequests.find(entry => entry.number === number); + if (!current || current.position !== stack.position || current.state !== GithubItemStateEnum.Open + || current.isDraft || current.mergeable !== PullRequestMergeability.Mergeable) { + return false; + } + return stack.pullRequests.every(entry => + entry.position > stack.position || + entry.state === GithubItemStateEnum.Merged || + (entry.state === GithubItemStateEnum.Open && !entry.isDraft && entry.mergeable === PullRequestMergeability.Mergeable) + ); +} + export enum MergeQueueState { AwaitingChecks, Locked, diff --git a/src/github/pullRequestModel.ts b/src/github/pullRequestModel.ts index 3830bd1fb3..94d7cf8378 100644 --- a/src/github/pullRequestModel.ts +++ b/src/github/pullRequestModel.ts @@ -55,6 +55,7 @@ import { IGitTreeItem, IRawFileChange, IRawFileContent, + isStackMergeable, IssueReference, ISuggestedReviewer, ITeam, @@ -533,6 +534,9 @@ export class PullRequestModel extends IssueModel implements IPullRe if (!stack.pullRequests.some(entry => entry.number === this.number && entry.state === GithubItemStateEnum.Open)) { throw new Error(`Pull request #${this.number} is not open in this stack.`); } + if (!isStackMergeable(stack, this.number)) { + throw new Error(`Pull request stack for #${this.number} is not ready to merge.`); + } const headSha = this.head?.sha; if (!headSha) { throw new Error(`Missing head commit for pull request #${this.number}.`); diff --git a/src/test/github/pullRequestModel.test.ts b/src/test/github/pullRequestModel.test.ts index 3e0a401bee..430a8d2bca 100644 --- a/src/test/github/pullRequestModel.test.ts +++ b/src/test/github/pullRequestModel.test.ts @@ -317,6 +317,41 @@ describe('PullRequestModel', function () { const invalidStack: PullRequestStack = { ...stack, pullRequests: [{ ...stack.pullRequests[1], state: GithubItemStateEnum.Merged }] }; await assert.rejects(model.mergeStack(new MockRepository(), invalidStack, 'squash', 'direct_merge'), /not open in this stack/); }); + + it('does not submit a merge when the current or a downstack PR is blocked', async function () { + const model = createModel(); + const blocked = [ + { ...stack, pullRequests: [stack.pullRequests[0], { ...stack.pullRequests[1], mergeable: PullRequestMergeability.NotMergeable }] }, + ...[ + { mergeable: PullRequestMergeability.Conflict }, + { mergeable: PullRequestMergeability.Behind }, + { mergeable: PullRequestMergeability.Unknown }, + { isDraft: true }, + { state: GithubItemStateEnum.Closed }, + ].map(change => ({ + ...stack, + pullRequests: [{ ...stack.pullRequests[0], ...change }, stack.pullRequests[1]], + })), + ]; + + for (const candidate of blocked) { + await assert.rejects(model.mergeStack(new MockRepository(), candidate, 'squash', 'direct_merge'), /not ready to merge/); + } + }); + + it('permits merging when a downstack PR is already merged', async function () { + const model = createModel(); + const withMergedBelow = { + ...stack, + pullRequests: [{ ...stack.pullRequests[0], state: GithubItemStateEnum.Merged, mergeable: PullRequestMergeability.Unknown }, stack.pullRequests[1]], + }; + repo.queryProvider.expectOctokitRequest(['request'], [`PUT ${route}`, requestParams(model)], { + status: 'merged', + details: { message: 'Merged', sha: 'merge-sha' }, + }); + + assert.strictEqual(await model.mergeStack(new MockRepository(), withMergedBelow, 'squash', 'direct_merge'), 'merged'); + }); }); it('returns no stack when the pull request is not stacked', async function () { diff --git a/webviews/components/merge.tsx b/webviews/components/merge.tsx index 4d793c8444..347d2299f7 100644 --- a/webviews/components/merge.tsx +++ b/webviews/components/merge.tsx @@ -24,6 +24,7 @@ import { groupBy } from '../../src/common/utils'; import { CheckState, GithubItemStateEnum, + isStackMergeable, MergeMethod, PullRequestCheckStatus, PullRequestMergeability, @@ -424,8 +425,9 @@ export const PrActions = ({ pr, isSimple }: { pr: PullRequest; isSimple: boolean if (pr.stackMergeStatus === 'enqueued') { return
Pull request stack added to the merge queue.
; } - if (pr.stack && hasWritePermission && !pr.mergeQueueEntry) { - return ; + if (pr.stack) { + return hasWritePermission && !pr.mergeQueueEntry && mergeable === PullRequestMergeability.Mergeable + && isStackMergeable(pr.stack, pr.number) ? : null; } if (!pr.stack && mergeable === PullRequestMergeability.Mergeable && hasWritePermission && !pr.mergeQueueEntry) { diff --git a/webviews/editorWebview/test/overview.test.tsx b/webviews/editorWebview/test/overview.test.tsx index e4cadbab10..b1b88512d2 100644 --- a/webviews/editorWebview/test/overview.test.tsx +++ b/webviews/editorWebview/test/overview.test.tsx @@ -67,7 +67,7 @@ describe('Overview', function () { base: 'main', pullRequests: [ { position: 1, number: 793, title: 'First Change', head: 'D1', url: 'https://example.com/793', state: GithubItemStateEnum.Open, isDraft: false, mergeable: PullRequestMergeability.Mergeable }, - { position: 2, number: 794, title: 'Second Change', head: 'D2', url: 'https://example.com/794', state: GithubItemStateEnum.Open, isDraft: false, mergeable: PullRequestMergeability.NotMergeable }, + { position: 2, number: 794, title: 'Second Change', head: 'D2', url: 'https://example.com/794', state: GithubItemStateEnum.Open, isDraft: false, mergeable: PullRequestMergeability.Mergeable }, { position: 3, number: 795, title: 'Third Change', head: 'D3', url: 'https://example.com/795', state: GithubItemStateEnum.Open, isDraft: true, mergeable: PullRequestMergeability.Mergeable }, ], }).build(); @@ -95,12 +95,12 @@ describe('Overview', function () { ]); assert.deepStrictEqual([...section.querySelectorAll('.stack-entry-readiness')].map(entry => [entry.classList[1], entry.getAttribute('aria-label')]), [ ['waiting', 'Draft pull request cannot be merged'], - ['blocked', 'Merge requirements not met'], + ['ready', 'Ready to merge'], ['ready', 'Ready to merge'], ]); assert.deepStrictEqual([...section.querySelectorAll('.stack-entry-readiness')].map(entry => entry.getAttribute('title')), [ 'Draft pull request cannot be merged', - 'Merge requirements not met', + 'Ready to merge', 'Ready to merge', ]); assert.strictEqual(section.querySelector('.stack-entry-state'), null); @@ -184,6 +184,64 @@ describe('Overview', function () { 'Mergeability is being checked', 'Already merged', ]); + assert.strictEqual(out.container.querySelector('.stack-merge'), null); + }); + + it('hides stack merge when the current or an open downstack PR is not mergeable', function () { + const readyStack = { + position: 2, + size: 3, + base: 'main', + pullRequests: [ + { position: 1, number: 793, title: 'First Change', head: 'D1', url: 'https://example.com/793', state: GithubItemStateEnum.Open, isDraft: false, mergeable: PullRequestMergeability.Mergeable }, + { position: 2, number: 794, title: 'Second Change', head: 'D2', url: 'https://example.com/794', state: GithubItemStateEnum.Open, isDraft: false, mergeable: PullRequestMergeability.Mergeable }, + { position: 3, number: 795, title: 'Third Change', head: 'D3', url: 'https://example.com/795', state: GithubItemStateEnum.Open, isDraft: false, mergeable: PullRequestMergeability.Conflict }, + ], + }; + const blocked = [ + new PullRequestBuilder().number(794).mergeable(PullRequestMergeability.Conflict).stack(readyStack).build(), + ...[ + { mergeable: PullRequestMergeability.Conflict }, + { mergeable: PullRequestMergeability.NotMergeable }, + { mergeable: PullRequestMergeability.Behind }, + { mergeable: PullRequestMergeability.Unknown }, + { isDraft: true }, + { state: GithubItemStateEnum.Closed }, + ].map(change => new PullRequestBuilder().number(794).stack({ + ...readyStack, + pullRequests: [{ ...readyStack.pullRequests[0], ...change }, ...readyStack.pullRequests.slice(1)], + }).build()), + ]; + for (const pr of blocked) { + const out = render( + + + , + ); + assert(out.container.querySelector('#pull-request-stack')); + assert.strictEqual(out.container.querySelector('.stack-merge'), null); + out.unmount(); + } + }); + + it('ignores PRs above and already merged PRs below the current stack merge', function () { + const stack = { + position: 2, + size: 3, + base: 'main', + pullRequests: [ + { position: 1, number: 793, title: 'First Change', head: 'D1', url: 'https://example.com/793', state: GithubItemStateEnum.Merged, isDraft: false, mergeable: PullRequestMergeability.Unknown }, + { position: 2, number: 794, title: 'Second Change', head: 'D2', url: 'https://example.com/794', state: GithubItemStateEnum.Open, isDraft: false, mergeable: PullRequestMergeability.Mergeable }, + { position: 3, number: 795, title: 'Third Change', head: 'D3', url: 'https://example.com/795', state: GithubItemStateEnum.Open, isDraft: false, mergeable: PullRequestMergeability.Conflict }, + ], + }; + const pr = new PullRequestBuilder().number(794).stack(stack).build(); + const out = render( + + + , + ); + assert(out.getByText('Merge stack (1 pull request)')); }); it('does not offer to merge a stack without write permission', function () {