Skip to content

Commit cdc768a

Browse files
committed
Add multi-select + right click to add to stack in PRs view
1 parent d88f5c6 commit cdc768a

10 files changed

Lines changed: 380 additions & 3 deletions

‎package.json‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1134,6 +1134,11 @@
11341134
"title": "%command.pr.refreshPullRequest.title%",
11351135
"category": "%command.pull.request.category%"
11361136
},
1137+
{
1138+
"command": "pr.addToStack",
1139+
"title": "%command.pr.addToStack.title%",
1140+
"category": "%command.pull.request.category%"
1141+
},
11371142
{
11381143
"command": "pr.openFileOnGitHub",
11391144
"title": "%command.pr.openFileOnGitHub.title%",
@@ -2251,6 +2256,10 @@
22512256
"command": "pr.openPullRequestOnGitHub",
22522257
"when": "(gitHubOpenRepositoryCount != 0 && github:inReviewMode) && !isSessionsWindow"
22532258
},
2259+
{
2260+
"command": "pr.addToStack",
2261+
"when": "false"
2262+
},
22542263
{
22552264
"command": "pr.openAllDiffs",
22562265
"when": "(gitHubOpenRepositoryCount != 0 && github:inReviewMode) && !isSessionsWindow"
@@ -2977,6 +2986,11 @@
29772986
}
29782987
],
29792988
"view/item/context": [
2989+
{
2990+
"command": "pr.addToStack",
2991+
"when": "(view == pr:github && viewItem =~ /pullrequest.*:stackable/ && github:canAddToStack) && !isSessionsWindow",
2992+
"group": "0_stack@1"
2993+
},
29802994
{
29812995
"command": "pr.pick",
29822996
"when": "(view == pr:github && viewItem =~ /(pullrequest(:local)?:nonactive)/) && !isSessionsWindow",

‎package.nls.json‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -236,6 +236,7 @@
236236
"command.pr.openPullRequestOnGitHub.title": "Open Pull Request on GitHub",
237237
"command.pr.openAllDiffs.title": "Open All Diffs",
238238
"command.pr.refreshPullRequest.title": "Refresh Pull Request",
239+
"command.pr.addToStack.title": "Add to Stack",
239240
"command.pr.openFileOnGitHub.title": "Open File on GitHub",
240241
"command.pr.revealFileInOS.title": "Reveal in File Explorer",
241242
"command.pr.copyCommitHash.title": "Copy Commit Hash",

‎src/common/executeCommands.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ export namespace contexts {
1515
export const LOADING_PRS_TREE = 'github:loadingPrsTree';
1616
export const LOADING_ISSUES_TREE = 'github:loadingIssuesTree';
1717
export const CREATE_PR_PERMISSIONS = 'github:createPrPermissions';
18+
export const CAN_ADD_TO_STACK = 'github:canAddToStack';
1819
export const RESOLVING_CONFLICTS = 'github:resolvingConflicts';
1920
export const PULL_REQUEST_DESCRIPTION_VISIBLE = 'github:pullRequestDescriptionVisible'; // Boolean indicating if the pull request description is visible
2021
export const ACTIVE_COMMENT_HAS_SUGGESTION = 'github:activeCommentHasSuggestion'; // Boolean indicating if the active comment has a suggestion

‎src/github/githubRepository.ts‎

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -879,6 +879,13 @@ export class GitHubRepository extends Disposable {
879879
}
880880

881881
async addPullRequestToStack(candidate: StackCandidate, number: number): Promise<void> {
882+
return this.addPullRequestsToStack(candidate, [number]);
883+
}
884+
885+
async addPullRequestsToStack(candidate: StackCandidate, numbers: number[]): Promise<void> {
886+
if (numbers.length === 0) {
887+
throw new Error('At least one pull request is required to add to a stack.');
888+
}
882889
const { octokit, remote } = await this.ensure();
883890
const params = {
884891
owner: remote.owner,
@@ -890,12 +897,12 @@ export class GitHubRepository extends Disposable {
890897
await octokit.call(() => octokit.api.request('POST /repos/{owner}/{repo}/stacks/{stack_number}/add', {
891898
...params,
892899
stack_number: stackNumber,
893-
pull_requests: [number],
900+
pull_requests: numbers,
894901
}));
895902
} else {
896903
await octokit.call(() => octokit.api.request('POST /repos/{owner}/{repo}/stacks', {
897904
...params,
898-
pull_requests: [candidate.parentPullRequestNumber, number],
905+
pull_requests: [candidate.parentPullRequestNumber, ...numbers],
899906
}));
900907
}
901908
}

‎src/github/pullRequestStack.ts‎

Lines changed: 88 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,88 @@
1+
/*---------------------------------------------------------------------------------------------
2+
* Copyright (c) Microsoft Corporation. All rights reserved.
3+
* Licensed under the MIT License. See License.txt in the project root for license information.
4+
*--------------------------------------------------------------------------------------------*/
5+
6+
import { GithubItemStateEnum } from './interface';
7+
import { PullRequestModel } from './pullRequestModel';
8+
import { compareIgnoreCase } from '../common/utils';
9+
10+
function sameRepository(first: PullRequestModel, second: PullRequestModel): boolean {
11+
return compareIgnoreCase(first.remote.owner, second.remote.owner) === 0
12+
&& compareIgnoreCase(first.remote.repositoryName, second.remote.repositoryName) === 0
13+
&& compareIgnoreCase(first.githubRepository.remote.normalizedHost, second.githubRepository.remote.normalizedHost) === 0;
14+
}
15+
16+
export function isStackablePullRequest(pullRequest: PullRequestModel): boolean {
17+
const { base, head } = pullRequest;
18+
if (pullRequest.state !== GithubItemStateEnum.Open || !head || !base || head.ref === base.ref) {
19+
return false;
20+
}
21+
const repository = pullRequest.githubRepository.remote;
22+
return [base, head].every(ref => compareIgnoreCase(ref.owner, repository.owner) === 0
23+
&& compareIgnoreCase(ref.repositoryCloneUrl.repositoryName, repository.repositoryName) === 0
24+
&& compareIgnoreCase(ref.repositoryCloneUrl.host, repository.gitProtocol.host) === 0);
25+
}
26+
27+
export function orderStackablePullRequests(pullRequests: readonly PullRequestModel[]): PullRequestModel[] | undefined {
28+
if (pullRequests.length < 2 || pullRequests.some(pr => !isStackablePullRequest(pr) || !sameRepository(pr, pullRequests[0]))) {
29+
return;
30+
}
31+
const byHead = new Map<string, PullRequestModel>();
32+
const byBase = new Map<string, PullRequestModel>();
33+
const numbers = new Set<number>();
34+
for (const pr of pullRequests) {
35+
if (byHead.has(pr.head!.ref) || byBase.has(pr.base.ref) || numbers.has(pr.number)) {
36+
return;
37+
}
38+
byHead.set(pr.head!.ref, pr);
39+
byBase.set(pr.base.ref, pr);
40+
numbers.add(pr.number);
41+
}
42+
const bottoms = pullRequests.filter(pr => !byHead.has(pr.base.ref));
43+
if (bottoms.length !== 1) {
44+
return;
45+
}
46+
const ordered: PullRequestModel[] = [];
47+
let current: PullRequestModel | undefined = bottoms[0];
48+
while (current && ordered.length < pullRequests.length) {
49+
ordered.push(current);
50+
current = byBase.get(current.head!.ref);
51+
}
52+
return ordered.length === pullRequests.length && !current ? ordered : undefined;
53+
}
54+
55+
export async function addPullRequestsToStack(pullRequests: readonly PullRequestModel[]): Promise<number[]> {
56+
const initial = orderStackablePullRequests(pullRequests);
57+
if (!initial) {
58+
throw new Error('Select two or more open pull requests whose head and base branches form a chain in the same repository.');
59+
}
60+
const selectedBranches = initial.map(pr => ({ number: pr.number, base: pr.base.ref, head: pr.head!.ref }));
61+
const repository = initial[0].githubRepository;
62+
const refreshed = await Promise.all(initial.map(async pr => {
63+
const current = await repository.getPullRequest(pr.number, 'addPullRequestsToStack');
64+
if (!current) {
65+
throw new Error(`Unable to refresh pull request #${pr.number} before creating a stack.`);
66+
}
67+
return current;
68+
}));
69+
const ordered = orderStackablePullRequests(refreshed);
70+
if (!ordered || ordered.some((pr, index) =>
71+
pr.number !== selectedBranches[index].number
72+
|| pr.base.ref !== selectedBranches[index].base
73+
|| pr.head?.ref !== selectedBranches[index].head)) {
74+
throw new Error('The selected pull request branches have changed. Refresh the view and try again.');
75+
}
76+
const bottom = ordered[0];
77+
const candidate = await repository.getStackCandidate(bottom.head!.ref);
78+
if (!candidate || candidate.parentPullRequestNumber !== bottom.number) {
79+
throw new Error(`Pull request #${bottom.number} is no longer eligible to start or extend a stack.`);
80+
}
81+
for (const pr of ordered.slice(1)) {
82+
if (await pr.getStack()) {
83+
throw new Error(`Pull request #${pr.number} is already in a stack.`);
84+
}
85+
}
86+
await repository.addPullRequestsToStack(candidate, ordered.slice(1).map(pr => pr.number));
87+
return ordered.map(pr => pr.number);
88+
}

‎src/test/github/pullRequestModel.test.ts‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -467,6 +467,24 @@ describe('PullRequestModel', function () {
467467
await repo.addPullRequestToStack(candidate, 796);
468468
});
469469

470+
it('creates a stack from multiple existing pull requests in branch order', async function () {
471+
const candidate = { parentPullRequestNumber: 795, size: 1, url: 'https://github.com/github/test/pull/795' };
472+
repo.queryProvider.expectOctokitRequest(['request'], ['POST /repos/{owner}/{repo}/stacks', {
473+
owner: 'github', repo: 'test', headers: listParams.headers, pull_requests: [795, 796, 797],
474+
}], {});
475+
476+
await repo.addPullRequestsToStack(candidate, [796, 797]);
477+
});
478+
479+
it('extends a stack with multiple existing pull requests in branch order', async function () {
480+
const candidate = { parentPullRequestNumber: 795, stackNumber: 12, size: 2, url: 'https://github.com/github/test/pull/795' };
481+
repo.queryProvider.expectOctokitRequest(['request'], ['POST /repos/{owner}/{repo}/stacks/{stack_number}/add', {
482+
owner: 'github', repo: 'test', headers: listParams.headers, stack_number: 12, pull_requests: [796, 797],
483+
}], {});
484+
485+
await repo.addPullRequestsToStack(candidate, [796, 797]);
486+
});
487+
470488
it('finds the parent from its GraphQL head branch before offering a stack', async function () {
471489
const parent = new GraphQLPullRequestBuilder().build().repository!.pullRequest;
472490
parent.number = 795;
Lines changed: 174 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,174 @@
1+
/*---------------------------------------------------------------------------------------------
2+
* Copyright (c) Microsoft Corporation. All rights reserved.
3+
* Licensed under the MIT License. See License.txt in the project root for license information.
4+
*--------------------------------------------------------------------------------------------*/
5+
6+
import { default as assert } from 'assert';
7+
import { createSandbox, SinonSandbox } from 'sinon';
8+
import { Protocol } from '../../common/protocol';
9+
import { GitHubServerType } from '../../common/authentication';
10+
import { GitHubRemote } from '../../common/remote';
11+
import { CredentialStore } from '../../github/credentials';
12+
import { GithubItemStateEnum } from '../../github/interface';
13+
import { PullRequestModel } from '../../github/pullRequestModel';
14+
import { addPullRequestsToStack, isStackablePullRequest, orderStackablePullRequests } from '../../github/pullRequestStack';
15+
import { convertRESTPullRequestToRawPullRequest } from '../../github/utils';
16+
import { PullRequestBuilder } from '../builders/rest/pullRequestBuilder';
17+
import { getAddToStackConfirmation } from '../../view/prsTreeDataProvider';
18+
import { MockCommandRegistry } from '../mocks/mockCommandRegistry';
19+
import { MockExtensionContext } from '../mocks/mockExtensionContext';
20+
import { MockGitHubRepository } from '../mocks/mockGitHubRepository';
21+
import { MockTelemetry } from '../mocks/mockTelemetry';
22+
23+
describe('Pull request stack selection', function () {
24+
let sinon: SinonSandbox;
25+
let context: MockExtensionContext;
26+
let credentials: CredentialStore;
27+
let repository: MockGitHubRepository;
28+
let remote: GitHubRemote;
29+
let telemetry: MockTelemetry;
30+
31+
beforeEach(function () {
32+
sinon = createSandbox();
33+
MockCommandRegistry.install(sinon);
34+
context = new MockExtensionContext();
35+
telemetry = new MockTelemetry();
36+
credentials = new CredentialStore(telemetry, context);
37+
const url = 'https://github.com/owner/repo';
38+
remote = new GitHubRemote('origin', url, new Protocol(url), GitHubServerType.GitHubDotCom);
39+
repository = new MockGitHubRepository(remote, credentials, telemetry, sinon);
40+
});
41+
42+
afterEach(function () {
43+
repository.dispose();
44+
credentials.dispose();
45+
context.dispose();
46+
sinon.restore();
47+
});
48+
49+
function pullRequest(number: number, base: string, head: string, state: 'open' | 'closed' = 'open', headOwner: string = remote.owner): PullRequestModel {
50+
const rest = new PullRequestBuilder().number(number).state(state)
51+
.base(ref => ref.ref(base))
52+
.head(ref => ref.ref(head)).build();
53+
for (const ref of [rest.base, rest.head]) {
54+
ref.repo.owner.login = remote.owner;
55+
ref.repo.name = remote.repositoryName;
56+
ref.repo.clone_url = `https://github.com/${remote.owner}/${remote.repositoryName}.git`;
57+
}
58+
rest.head.repo.owner.login = headOwner;
59+
rest.head.repo.clone_url = `https://github.com/${headOwner}/${remote.repositoryName}.git`;
60+
return new PullRequestModel(credentials, telemetry, repository, remote, convertRESTPullRequestToRawPullRequest(rest, repository));
61+
}
62+
63+
it('orders a branch chain from the target base toward the top', function () {
64+
const bottom = pullRequest(1, 'main', 'D1');
65+
const middle = pullRequest(2, 'D1', 'D2');
66+
const top = pullRequest(3, 'D2', 'D3');
67+
assert.deepStrictEqual(orderStackablePullRequests([top, bottom, middle]), [bottom, middle, top]);
68+
});
69+
70+
it('distinguishes creating a stack from adding PRs to an existing stack', function () {
71+
const bottom = pullRequest(1, 'main', 'D1');
72+
const middle = pullRequest(2, 'D1', 'D2');
73+
const top = pullRequest(3, 'D2', 'D3');
74+
assert.deepStrictEqual(getAddToStackConfirmation([bottom, middle], {
75+
parentPullRequestNumber: 1, size: 1, url: bottom.html_url,
76+
}), {
77+
message: 'Create a stack with 2 pull requests?',
78+
detail: '#1 New feature\n#2 New feature',
79+
action: 'Create Stack',
80+
});
81+
assert.deepStrictEqual(getAddToStackConfirmation([bottom, middle], {
82+
parentPullRequestNumber: 1, stackNumber: 10, size: 3, url: bottom.html_url,
83+
}), {
84+
message: 'Add 1 pull request to an existing stack?',
85+
detail: 'Adding #2 New feature\nto #1 New feature',
86+
action: 'Add to Stack',
87+
});
88+
assert.deepStrictEqual(getAddToStackConfirmation([bottom, middle, top], {
89+
parentPullRequestNumber: 1, stackNumber: 10, size: 3, url: bottom.html_url,
90+
}), {
91+
message: 'Add 2 pull requests to an existing stack?',
92+
detail: 'Adding #2 New feature\n#3 New feature\nto #1 New feature',
93+
action: 'Add to Stack',
94+
});
95+
});
96+
97+
it('hides the action for unrelated, duplicate, closed and forked PRs', function () {
98+
const bottom = pullRequest(1, 'main', 'D1');
99+
const top = pullRequest(2, 'D1', 'D2');
100+
assert.strictEqual(orderStackablePullRequests([bottom]) === undefined, true, 'one PR is insufficient');
101+
assert.strictEqual(orderStackablePullRequests([bottom, bottom]) === undefined, true, 'duplicate PRs are invalid');
102+
assert.strictEqual(orderStackablePullRequests([bottom, pullRequest(3, 'other', 'D3')]) === undefined, true, 'disconnected branches are invalid');
103+
assert.strictEqual(orderStackablePullRequests([bottom, top, pullRequest(3, 'D1', 'D2')]) === undefined, true, 'duplicate head branches are invalid');
104+
assert.strictEqual(orderStackablePullRequests([bottom, pullRequest(3, 'D1', 'D3', 'closed')]) === undefined, true, 'closed PRs are invalid');
105+
const forked = pullRequest(4, 'D1', 'D4', 'open', 'another');
106+
assert.strictEqual(isStackablePullRequest(forked), false);
107+
assert.strictEqual(orderStackablePullRequests([bottom, forked]) === undefined, true, 'forked heads are invalid');
108+
assert.strictEqual(orderStackablePullRequests([bottom, top])?.map(pr => pr.number).join(','), '1,2');
109+
});
110+
111+
it('creates a new stack with selected PRs in branch order after refreshing them', async function () {
112+
const bottom = pullRequest(1, 'main', 'D1');
113+
const middle = pullRequest(2, 'D1', 'D2');
114+
const top = pullRequest(3, 'D2', 'D3');
115+
const refresh = sinon.stub(repository, 'getPullRequest').callsFake(async number => [bottom, middle, top].find(pr => pr.number === number));
116+
sinon.stub(repository, 'getStackCandidate').resolves({ parentPullRequestNumber: 1, size: 1, url: bottom.html_url });
117+
sinon.stub(middle, 'getStack').resolves(undefined);
118+
sinon.stub(top, 'getStack').resolves(undefined);
119+
const add = sinon.stub(repository, 'addPullRequestsToStack').resolves();
120+
121+
assert.deepStrictEqual(await addPullRequestsToStack([top, bottom, middle]), [1, 2, 3]);
122+
assert(refresh.calledThrice);
123+
assert(add.calledOnceWithExactly({ parentPullRequestNumber: 1, size: 1, url: bottom.html_url }, [2, 3]));
124+
});
125+
126+
it('appends to an existing stack only when the bottom PR is its top', async function () {
127+
const bottom = pullRequest(1, 'main', 'D1');
128+
const top = pullRequest(2, 'D1', 'D2');
129+
sinon.stub(repository, 'getPullRequest').callsFake(async number => [bottom, top].find(pr => pr.number === number));
130+
const candidate = { parentPullRequestNumber: 1, stackNumber: 10, size: 3, url: bottom.html_url };
131+
sinon.stub(repository, 'getStackCandidate').resolves(candidate);
132+
sinon.stub(top, 'getStack').resolves(undefined);
133+
const add = sinon.stub(repository, 'addPullRequestsToStack').resolves();
134+
135+
assert.deepStrictEqual(await addPullRequestsToStack([top, bottom]), [1, 2]);
136+
assert(add.calledOnceWithExactly(candidate, [2]));
137+
});
138+
139+
it('rejects stale branch chains and PRs already in another stack before writing', async function () {
140+
const bottom = pullRequest(1, 'main', 'D1');
141+
const top = pullRequest(2, 'D1', 'D2');
142+
const moved = pullRequest(2, 'other', 'D2');
143+
const refresh = sinon.stub(repository, 'getPullRequest').callsFake(async number => number === 1 ? bottom : moved);
144+
const add = sinon.stub(repository, 'addPullRequestsToStack').resolves();
145+
await assert.rejects(addPullRequestsToStack([top, bottom]), /branches have changed/);
146+
assert(add.notCalled);
147+
148+
refresh.callsFake(async number => number === 1 ? bottom : top);
149+
sinon.stub(repository, 'getStackCandidate').resolves({ parentPullRequestNumber: 1, size: 1, url: bottom.html_url });
150+
sinon.stub(top, 'getStack').resolves({
151+
position: 1, size: 1, base: 'main', pullRequests: [{
152+
position: 1, number: 2, title: top.title, url: top.html_url, head: 'D2',
153+
state: GithubItemStateEnum.Open, isDraft: false, mergeable: top.item.mergeable!,
154+
}],
155+
});
156+
await assert.rejects(addPullRequestsToStack([bottom, top]), /already in a stack/);
157+
assert(add.notCalled);
158+
});
159+
160+
it('rejects a changed head branch when refreshing reuses the selected model', async function () {
161+
const bottom = pullRequest(1, 'main', 'D1');
162+
const top = pullRequest(2, 'D1', 'D2');
163+
sinon.stub(repository, 'getPullRequest').callsFake(async number => {
164+
if (number === top.number) {
165+
top.head!.ref = 'D3';
166+
}
167+
return number === bottom.number ? bottom : top;
168+
});
169+
const add = sinon.stub(repository, 'addPullRequestsToStack').resolves();
170+
171+
await assert.rejects(addPullRequestsToStack([bottom, top]), /branches have changed/);
172+
assert(add.notCalled);
173+
});
174+
});

‎src/test/view/prsTree.test.ts‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,11 +47,13 @@ describe('GitHub Pull Requests view', function () {
4747
let mockNotificationsManager: MockNotificationManager;
4848
let prsTreeModel: PrsTreeModel;
4949
let discoveredRepository: MockGitHubRepository | undefined;
50+
let createTreeView: ReturnType<SinonSandbox['spy']>;
5051

5152
beforeEach(function () {
5253
sinon = createSandbox();
5354
discoveredRepository = undefined;
5455
MockCommandRegistry.install(sinon);
56+
createTreeView = sinon.spy(vscode.window, 'createTreeView');
5557
mockThemeWatcher = new MockThemeWatcher();
5658

5759
context = new MockExtensionContext();
@@ -108,6 +110,13 @@ describe('GitHub Pull Requests view', function () {
108110
assert.strictEqual(rootNodes.length, 0);
109111
});
110112

113+
it('allows selecting multiple pull requests to create a stack', function () {
114+
const tree = createTreeView.getCalls().find(call => call.args[0] === 'pr:github');
115+
assert(tree);
116+
const options = tree.args[1] as { canSelectMany?: boolean };
117+
assert.strictEqual(options.canSelectMany, true);
118+
});
119+
111120
it('has no children when no GitHub remotes are available', async function () {
112121
sinon
113122
.stub(vscode.workspace, 'workspaceFolders')

0 commit comments

Comments
 (0)