From 36110cea4325392487d8c6e736b30325959100d4 Mon Sep 17 00:00:00 2001 From: Alex Ross <38270282+alexr00@users.noreply.github.com> Date: Tue, 6 Oct 2026 12:37:49 +0200 Subject: [PATCH 1/2] Improve PR stack refreshing --- src/github/githubRepository.ts | 35 +- src/github/issueModel.ts | 3 + src/github/pullRequestModel.ts | 19 + src/github/pullRequestOverview.ts | 86 +++-- src/test/github/createPRViewProvider.test.ts | 76 ++-- src/test/github/pullRequestModel.test.ts | 136 ++++++- src/test/github/pullRequestOverview.test.ts | 375 ++++++++++++++++--- src/test/view/prsTree.test.ts | 8 +- src/view/prsTreeDataProvider.ts | 8 +- 9 files changed, 594 insertions(+), 152 deletions(-) diff --git a/src/github/githubRepository.ts b/src/github/githubRepository.ts index 9661a8709a..fdf46c270c 100644 --- a/src/github/githubRepository.ts +++ b/src/github/githubRepository.ts @@ -221,6 +221,12 @@ export class GitHubRepository extends Disposable { public readonly onDidAddPullRequest: vscode.Event = this._onDidAddPullRequest.event; private _onDidChangePullRequests: vscode.EventEmitter = this._register(new vscode.EventEmitter()); public readonly onDidChangePullRequests: vscode.Event = this._onDidChangePullRequests.event; + private readonly _onDidChangeStack = this._register(new vscode.EventEmitter()); + public readonly onDidChangeStack = this._onDidChangeStack.event; + + notifyStackChanged(numbers: readonly number[]): void { + this._onDidChangeStack.fire(numbers); + } public get hub(): GitHub { if (this._hub && this.remote.isEnterprise && (!this.authMatchesServer || !this.remote.matchesServerUri(this._hub.serverUri))) { @@ -880,11 +886,11 @@ export class GitHubRepository extends Disposable { return { parentPullRequestNumber: parent.number, stackNumber: stack.number, size: stack.pull_requests.length, url: parent.html_url }; } - async addPullRequestToStack(candidate: StackCandidate, number: number): Promise { + async addPullRequestToStack(candidate: StackCandidate, number: number): Promise { return this.addPullRequestsToStack(candidate, [number]); } - async addPullRequestsToStack(candidate: StackCandidate, numbers: number[]): Promise { + async addPullRequestsToStack(candidate: StackCandidate, numbers: number[]): Promise { if (numbers.length === 0) { throw new Error('At least one pull request is required to add to a stack.'); } @@ -895,18 +901,30 @@ export class GitHubRepository extends Disposable { headers: { 'X-GitHub-Api-Version': '2026-03-10' }, }; const stackNumber = candidate.stackNumber; + let data: unknown; if (stackNumber !== undefined) { - await octokit.call(() => octokit.api.request('POST /repos/{owner}/{repo}/stacks/{stack_number}/add', { + ({ data } = await octokit.call(() => octokit.api.request('POST /repos/{owner}/{repo}/stacks/{stack_number}/add', { ...params, stack_number: stackNumber, pull_requests: numbers, - })); + }))); } else { - await octokit.call(() => octokit.api.request('POST /repos/{owner}/{repo}/stacks', { + ({ data } = await octokit.call(() => octokit.api.request('POST /repos/{owner}/{repo}/stacks', { ...params, pull_requests: [candidate.parentPullRequestNumber, ...numbers], - })); + }))); + } + if (!isObject(data) || !Array.isArray(data.pull_requests) || data.pull_requests.length === 0) { + throw new Error('GitHub returned an invalid result when adding pull requests to a stack.'); } + const members = data.pull_requests.map((pr: unknown) => { + if (!isObject(pr) || typeof pr.number !== 'number') { + throw new Error('GitHub returned an invalid pull request stack entry.'); + } + return pr.number; + }); + this.notifyStackChanged(members); + return members; } async unstackAll(pullRequestNumber: number, expectedPullRequests: readonly number[]): Promise { @@ -935,13 +953,16 @@ export class GitHubRepository extends Disposable { stack_number: stacks[0].number, })); if (result.status === 204) { + this.notifyStackChanged(expectedPullRequests); return []; } if (result.status !== 200 || !isObject(result.data) || !Array.isArray(result.data.pull_requests) || !result.data.pull_requests.every((pr: unknown) => isObject(pr) && typeof pr.number === 'number')) { throw new Error('GitHub returned an invalid result when unstacking pull requests.'); } - return result.data.pull_requests.map((pr: { number: number }) => pr.number); + const remaining = result.data.pull_requests.map((pr: { number: number }) => pr.number); + this.notifyStackChanged(expectedPullRequests); + return remaining; } async canGetProjectsNow(): Promise { diff --git a/src/github/issueModel.ts b/src/github/issueModel.ts index ff6b910ac7..5de5561431 100644 --- a/src/github/issueModel.ts +++ b/src/github/issueModel.ts @@ -42,6 +42,9 @@ export interface IssueChangeEvent { draft?: true; reviewers?: true; base?: true; + head?: true; + mergeability?: true; + mergeQueue?: true; } export class IssueModel extends Disposable { diff --git a/src/github/pullRequestModel.ts b/src/github/pullRequestModel.ts index 94d7cf8378..13c4625f5d 100644 --- a/src/github/pullRequestModel.ts +++ b/src/github/pullRequestModel.ts @@ -298,6 +298,15 @@ export class PullRequestModel extends IssueModel implements IPullRe changes.draft = true; this.isDraft = item.isDraft; } + if (this.head && item.head && (this.head.ref !== item.head.ref || this.head.sha !== item.head.sha)) { + changes.head = true; + } + if (this.item.mergeable !== item.mergeable) { + changes.mergeability = true; + } + if (this.base && item.base && (this.base.ref !== item.base.ref || this.base.sha !== item.base.sha)) { + changes.base = true; + } this.suggestedReviewers = item.suggestedReviewers; this.closingIssues = item.closingIssues ?? []; @@ -315,6 +324,11 @@ export class PullRequestModel extends IssueModel implements IPullRe this.base = new GitHubRef(item.base.ref, item.base!.label, item.base!.sha, item.base!.repo.cloneUrl, item.base.repo.owner, item.base.repo.name, item.base.repo.isInOrganization); } if (item.mergeQueueEntry !== undefined) { + if (this.mergeQueueEntry?.position !== item.mergeQueueEntry?.position + || this.mergeQueueEntry?.state !== item.mergeQueueEntry?.state + || this.mergeQueueEntry?.url !== item.mergeQueueEntry?.url) { + changes.mergeQueue = true; + } this.mergeQueueEntry = item.mergeQueueEntry ?? undefined; } if (item.hasComments !== undefined) { @@ -593,6 +607,7 @@ export class PullRequestModel extends IssueModel implements IPullRe throw new Error('GitHub returned an unknown stack merge result.'); } Logger.debug(`Stack merge for #${this.number}: ${response.status}`, PullRequestModel.ID); + this.githubRepository.notifyStackChanged(stack.pullRequests.map(entry => entry.number)); return response.status; } @@ -2249,9 +2264,13 @@ export class PullRequestModel extends IssueModel implements IPullRe Logger.debug(`Fetch pull request mergeability ${this.number} - done`, PullRequestModel.ID); const mergeability = parseMergeability(data.repository?.pullRequest.mergeable, data.repository?.pullRequest.mergeStateStatus); + const previousMergeability = this.item.mergeable; this.item.mergeable = mergeability; this.conflicts = data.repository?.pullRequest.mergeRequirements?.conditions.find(condition => condition.__typename === 'PullRequestMergeConflictStateCondition')?.conflicts; this.update(this.item); + if (previousMergeability !== mergeability) { + this._onDidChange.fire({ mergeability: true }); + } return { mergeability, conflicts: this.conflicts }; } catch (e) { Logger.error(`Unable to fetch PR Mergeability: ${e}`, PullRequestModel.ID); diff --git a/src/github/pullRequestOverview.ts b/src/github/pullRequestOverview.ts index e2d9d5b3b1..39b4de30ab 100644 --- a/src/github/pullRequestOverview.ts +++ b/src/github/pullRequestOverview.ts @@ -73,6 +73,10 @@ export class PullRequestOverviewPanel extends IssueOverviewPanel | undefined; private _updateSequence = 0; private _previewSequence = 0; + private _stackLoaded = false; + private _stackPullRequestNumbers = new Set(); + private _stackRefreshPending = false; + private _stackRefreshPromise: Promise | undefined; private _resolveCommentThreadQueue: Promise = Promise.resolve(); public static override async createOrShow( @@ -182,13 +186,6 @@ export class PullRequestOverviewPanel extends IssueOverviewPanel { - const panels = numbers - .map(number => this.findPanel(owner, repo, number)) - .filter((panel): panel is PullRequestOverviewPanel => !!panel); - await Promise.all(panels.map(panel => panel.refreshPanel())); - } - /** * Register the webview context-menu commands once globally, * rather than per panel instance. Each command receives the @@ -278,28 +275,52 @@ export class PullRequestOverviewPanel extends IssueOverviewPanel { + if (numbers.includes(this._item.number) || numbers.some(number => this._stackPullRequestNumbers.has(number))) { + void this.refreshStack(); + } + })); + this._prListeners.push(repository.onDidChangePullRequests(changes => { + if (changes.some(({ model, event }) => model.number !== this._item.number + && this._stackPullRequestNumbers.has(model.number) + && (event.state || event.draft || event.title || event.base || event.head || event.mergeability || event.mergeQueue))) { + void this.refreshStack(); + } + })); this._prListeners.push(this._item.onDidChange(e => { if (e.draft) { - const item = this._item; void this.refreshPanel(); - if (areStacksEnabled()) { - void item.getStack().then(stack => { - if (stack) { - return PullRequestOverviewPanel.refreshStackPanels(item.remote.owner, item.remote.repositoryName, - stack.pullRequests.filter(entry => entry.number !== item.number).map(entry => entry.number)); - } - }).catch(error => { - Logger.error(`Failed to refresh pull request stack after draft change: ${formatError(error)}`, PullRequestOverviewPanel.ID); - void vscode.window.showErrorMessage(vscode.l10n.t('Unable to refresh pull request stack: {0}', formatError(error))); - }); - } } else if ((e.state || e.comments) && !this._refreshing && !this._updateItemPromise) { this.refreshPanel(); + } else if (e.title || e.base || e.head || e.mergeability || e.mergeQueue) { + void this.refreshStack(); } })); } } + private refreshStack(): Promise { + if (this.isDisposed || !areStacksEnabled()) { + return Promise.resolve(); + } + this._stackRefreshPending = true; + this._stackRefreshPromise ??= Promise.resolve().then(async () => { + while (this._stackRefreshPending && !this.isDisposed && this._panel.visible && areStacksEnabled()) { + this._stackRefreshPending = false; + if (this._item) { + await this.loadStack(this._item, this._updateSequence); + } + } + }).finally(() => { + this._stackRefreshPromise = undefined; + if (this._stackRefreshPending && !this.isDisposed && this._panel.visible && areStacksEnabled()) { + void this.refreshStack(); + } + }); + return this._stackRefreshPromise; + } + /** * Override to process permalinks with PR-specific logic (including diff links). * Returns undefined if bodyHTML is undefined. @@ -338,6 +359,9 @@ export class PullRequestOverviewPanel extends IssueOverviewPanel { Logger.error(`Failed to update deferred assignable users: ${formatError(error)}`, PullRequestOverviewPanel.ID); }); - let stackLoaded = false; const deferredDataPromise = Promise.all([ measureDeferred('statusChecks', pullRequestModel.getStatusChecks()), reviewRequestsPromise, @@ -612,7 +639,7 @@ export class PullRequestOverviewPanel extends IssueOverviewPanel { stackLoaded = true; }); + void this.refreshStack(); } const timelineStart = performance.now(); void Promise.all([pullRequestModel.getTimelineEvents(), reviewRequestsPromise]).then(async ([latestTimelineEvents, requestedReviewers]) => { @@ -658,10 +685,10 @@ export class PullRequestOverviewPanel extends IssueOverviewPanel void): Promise { + private async loadStack(pullRequestModel: PullRequestModel, updateSequence: number): Promise { try { const stack = await pullRequestModel.getStack(); - if (updateSequence !== this._updateSequence || !areStacksEnabled()) { + if (this.isDisposed || updateSequence !== this._updateSequence || !areStacksEnabled()) { return; } const stackQueueMethod = stack ? await this._folderRepositoryManager.mergeQueueMethodForBranch(stack.base, pullRequestModel.remote.owner, pullRequestModel.remote.repositoryName) : undefined; @@ -676,20 +703,22 @@ export class PullRequestOverviewPanel extends IssueOverviewPanel entry.number)); await this._postMessage({ command: 'pr.update', pullrequest: { stack: linkedStack, stackLoaded: true, + stackLoadError: false, ...(stack ? { mergeQueueMethod: stackQueueMethod } : {}), } satisfies Partial, }); } } catch (error) { Logger.error(`Failed to load pull request stack: ${formatError(error)}`, PullRequestOverviewPanel.ID); - if (updateSequence === this._updateSequence && areStacksEnabled()) { + if (!this.isDisposed && updateSequence === this._updateSequence && areStacksEnabled()) { void this._postMessage({ command: 'pr.update', pullrequest: { stackLoadError: true } satisfies Partial }); } } @@ -1157,8 +1186,6 @@ export class PullRequestOverviewPanel extends IssueOverviewPanel pr.number)); if (remainingPullRequests.length === stack.size) { void vscode.window.showInformationMessage(vscode.l10n.t('No pull requests were unstacked. Merged, queued, or currently merging pull requests remain in the stack.')); } else { @@ -1573,6 +1600,7 @@ export class PullRequestOverviewPanel extends IssueOverviewPanel task({ report: () => undefined }, cancellation.token)); - const candidate: StackCandidate = { parentPullRequestNumber: 795, stackNumber: 12, size: 3, url: 'https://github.com/github/test/pull/795' }; - const getCandidate = sinon.stub(githubRepository, 'getStackCandidate').resolves(candidate); - sinon.stub(folderManager, 'createGitHubRepositoryFromOwnerName').resolves(githubRepository); - sinon.stub(model, 'filesHaveChanges').resolves(false); - repository.expectFetch('origin', 'D4'); - const createdPR = new PullRequestModel(credentials, new MockTelemetry(), githubRepository, githubRepository.remote, - convertRESTPullRequestToRawPullRequest(new PullRequestBuilder().number(796).build(), githubRepository)); - const create = sinon.stub(folderManager, 'createPullRequest').resolves(createdPR); - const addToStack = sinon.stub(githubRepository, 'addPullRequestToStack').resolves(); - sinon.stub(provider, 'postCreate').resolves(); - sinon.stub(provider, '_replyMessage').resolves(); - const throwError = sinon.stub(provider, '_throwError').resolves(); - const done = asPromise(provider.onDone); - - await provider.createForTest({ - command: 'pr.create', req: '1', - args: { - title: 'Fourth change', body: '', owner: 'github', repo: 'test', base: 'D3', - compareOwner: 'github', compareRepo: 'test', compareBranch: 'D4', - draft: false, autoMerge: false, labels: [], projects: [], assignees: [], reviewers: [], - addToStack: true, stackParentPullRequest: 795, stackNumber: 12, - }, + for (const stackNumber of [undefined, 12]) { + it(`${stackNumber === undefined ? 'creates' : 'extends'} a stack without querying or refreshing webviews directly`, async function () { + const cancellation = new vscode.CancellationTokenSource(); + sinon.stub(vscode.window, 'withProgress').callsFake((_options, task) => task({ report: () => undefined }, cancellation.token)); + const candidate: StackCandidate = { parentPullRequestNumber: 795, stackNumber, size: stackNumber === undefined ? 1 : 3, url: 'https://github.com/github/test/pull/795' }; + const getCandidate = sinon.stub(githubRepository, 'getStackCandidate').resolves(candidate); + sinon.stub(folderManager, 'createGitHubRepositoryFromOwnerName').resolves(githubRepository); + sinon.stub(model, 'filesHaveChanges').resolves(false); + repository.expectFetch('origin', 'D4'); + const createdPR = new PullRequestModel(credentials, new MockTelemetry(), githubRepository, githubRepository.remote, + convertRESTPullRequestToRawPullRequest(new PullRequestBuilder().number(796).build(), githubRepository)); + const create = sinon.stub(folderManager, 'createPullRequest').resolves(createdPR); + const numbers = stackNumber === undefined ? [795, 796] : [793, 794, 795, 796]; + const addToStack = sinon.stub(githubRepository, 'addPullRequestToStack').resolves(numbers); + const getStack = sinon.stub(createdPR, 'getStack').rejects(new Error('Unexpected stack refetch')); + sinon.stub(provider, 'postCreate').resolves(); + sinon.stub(provider, '_replyMessage').resolves(); + const throwError = sinon.stub(provider, '_throwError').resolves(); + const done = asPromise(provider.onDone); + + await provider.createForTest({ + command: 'pr.create', req: '1', + args: { + title: 'Fourth change', body: '', owner: 'github', repo: 'test', base: 'D3', + compareOwner: 'github', compareRepo: 'test', compareBranch: 'D4', + draft: false, autoMerge: false, labels: [], projects: [], assignees: [], reviewers: [], + addToStack: true, stackParentPullRequest: 795, stackNumber, + }, + }); + assert.strictEqual(await done, createdPR, throwError.firstCall?.args[1] ?? 'Pull request creation did not complete.'); + assert(getCandidate.calledWithExactly('D3')); + assert(create.calledOnce); + assert(addToStack.calledOnceWithExactly(candidate, 796)); + assert(getStack.notCalled); + sinon.assert.callOrder(getCandidate, create, addToStack); + cancellation.dispose(); }); - const result = await Promise.race([ - done, - new Promise(resolve => setTimeout(() => resolve(undefined), 2000)), - ]); - assert(result === createdPR, throwError.firstCall?.args[1] ?? 'Pull request creation did not complete.'); - assert(getCandidate.calledWithExactly('D3')); - assert(create.calledOnce); - assert(addToStack.calledOnceWithExactly(candidate, 796)); - sinon.assert.callOrder(getCandidate, create, addToStack); - cancellation.dispose(); - }); + } it('does not create a pull request when the selected stack parent has changed', async function () { const cancellation = new vscode.CancellationTokenSource(); @@ -351,6 +351,7 @@ describe('Create pull request stack', function () { sinon.stub(folderManager, 'createPullRequest').resolves(createdPR); const setDetails = sinon.stub(provider, 'postCreate').resolves(); sinon.stub(githubRepository, 'addPullRequestToStack').rejects(new Error('Stack is locked')); + const getStack = sinon.stub(createdPR, 'getStack'); const showError = sinon.stub(vscode.window, 'showErrorMessage').resolves(undefined); const done = asPromise(provider.onDone); @@ -365,6 +366,7 @@ describe('Create pull request stack', function () { }); assert((await done) === createdPR); assert(setDetails.calledOnce); + assert(getStack.notCalled); assert(showError.calledOnce); assert.match(showError.firstCall.args[0], /#796 was created but could not be added to its stack: Stack is locked/); cancellation.dispose(); diff --git a/src/test/github/pullRequestModel.test.ts b/src/test/github/pullRequestModel.test.ts index 98c12c3de5..3fe930b803 100644 --- a/src/test/github/pullRequestModel.test.ts +++ b/src/test/github/pullRequestModel.test.ts @@ -11,7 +11,7 @@ import { GitChangeType, SlimFileChange } from '../../common/file'; import { CredentialStore } from '../../github/credentials'; import { FolderRepositoryManager } from '../../github/folderRepositoryManager'; import { PullRequestModel } from '../../github/pullRequestModel'; -import { GithubItemStateEnum, PullRequestMergeability, PullRequestStack } from '../../github/interface'; +import { GithubItemStateEnum, MergeQueueState, PullRequestMergeability, PullRequestStack } from '../../github/interface'; import { Protocol } from '../../common/protocol'; import { GitHubRemote, Remote } from '../../common/remote'; import { convertRESTPullRequestToRawPullRequest } from '../../github/utils'; @@ -104,6 +104,92 @@ describe('PullRequestModel', function () { assert.strictEqual(open.state, GithubItemStateEnum.Merged); }); + it('emits specific changes when head commits or mergeability change', function () { + const pr = new PullRequestBuilder().number(794).build(); + const model = repo.createOrUpdatePullRequestModel(convertRESTPullRequestToRawPullRequest(pr, repo)); + const changes = sinon.spy(); + repo.onDidChangePullRequests(changes); + + model.update({ ...model.item, head: { ...model.item.head!, sha: 'updated-head' } }); + assert.deepStrictEqual(changes.lastCall.args[0][0].event, { head: true }); + model.update({ ...model.item, mergeable: PullRequestMergeability.NotMergeable }); + assert.deepStrictEqual(changes.lastCall.args[0][0].event, { mergeability: true }); + }); + + it('emits a draft change separately from head and mergeability changes', function () { + const model = repo.createOrUpdatePullRequestModel( + convertRESTPullRequestToRawPullRequest(new PullRequestBuilder().number(794).build(), repo)); + const changes = sinon.spy(); + repo.onDidChangePullRequests(changes); + + model.update({ ...model.item, isDraft: !model.isDraft }); + + assert(changes.calledOnce); + assert.deepStrictEqual(changes.firstCall.args[0][0].event, { draft: true }); + }); + + it('compares merge queue fields explicitly and preserves omitted queue entries', function () { + const model = repo.createOrUpdatePullRequestModel( + convertRESTPullRequestToRawPullRequest(new PullRequestBuilder().number(794).build(), repo)); + const changes = sinon.spy(); + repo.onDidChangePullRequests(changes); + const entry = { position: 1, state: MergeQueueState.Queued, url: 'https://github.com/github/test/queue' }; + + model.update({ ...model.item, mergeQueueEntry: entry }); + assert.deepStrictEqual(changes.lastCall.args[0][0].event, { mergeQueue: true }); + changes.resetHistory(); + model.update({ ...model.item, mergeQueueEntry: { url: entry.url, state: entry.state, position: entry.position } }); + model.update({ ...model.item, mergeQueueEntry: undefined }); + assert(changes.notCalled); + assert.deepStrictEqual(model.mergeQueueEntry, entry); + + for (const updated of [ + { ...entry, position: 2 }, + { ...entry, position: 2, state: MergeQueueState.AwaitingChecks }, + { ...entry, position: 2, state: MergeQueueState.AwaitingChecks, url: `${entry.url}/updated` }, + null, + ]) { + changes.resetHistory(); + model.update({ ...model.item, mergeQueueEntry: updated }); + assert(changes.calledOnce); + assert.deepStrictEqual(changes.firstCall.args[0][0].event, { mergeQueue: true }); + } + assert.strictEqual(model.mergeQueueEntry, undefined); + }); + + it('emits a base change when the base branch or commit changes', function () { + const model = repo.createOrUpdatePullRequestModel( + convertRESTPullRequestToRawPullRequest(new PullRequestBuilder().number(794).build(), repo)); + const changes = sinon.spy(); + repo.onDidChangePullRequests(changes); + const base = model.item.base; + assert(base); + + model.update({ ...model.item, base: { ...base, ref: 'updated-base' } }); + assert(changes.lastCall.args[0][0].event.base); + }); + + it('emits a mergeability change when fetching mergeability changes the model in place', async function () { + const model = repo.createOrUpdatePullRequestModel( + convertRESTPullRequestToRawPullRequest(new PullRequestBuilder().number(794).build(), repo)); + model.item.mergeable = PullRequestMergeability.Mergeable; + const changes = sinon.spy(); + repo.onDidChangePullRequests(changes); + repo.queryProvider.expectGraphQLQuery({ + query: queries.PullRequestMergeability, + variables: { owner: 'github', name: 'test', number: 794 }, + }, { + data: { repository: { pullRequest: { mergeable: 'MERGEABLE', mergeStateStatus: 'BLOCKED' } } }, + loading: false, stale: false, networkStatus: NetworkStatus.ready, + }); + + const result = await model.getMergeability(); + + assert.strictEqual(result.mergeability, PullRequestMergeability.NotMergeable); + assert(changes.calledOnce); + assert.deepStrictEqual(changes.firstCall.args[0][0].event, { mergeability: true }); + }); + describe('getStack', function () { function createModel() { const pr = new PullRequestBuilder().number(794).build(); @@ -232,12 +318,15 @@ describe('PullRequestModel', function () { it('requests a direct stack merge with the selected method and head SHA', async function () { const model = createModel(); + const changed = sinon.spy(); + repo.onDidChangeStack(changed); repo.queryProvider.expectOctokitRequest(['request'], [`PUT ${route}`, requestParams(model)], { status: 'merged', details: { message: 'Merged', sha: 'merge-sha' }, }); assert.strictEqual(await model.mergeStack(new MockRepository(), stack, 'squash', 'direct_merge'), 'merged'); + assert(changed.calledOnceWithExactly([793, 794])); }); it('queues a stack without supplying a direct merge method', async function () { @@ -452,6 +541,8 @@ describe('PullRequestModel', function () { }; it('detects an unstacked parent and creates a new stack with both PRs', async function () { + const changed = sinon.spy(); + repo.onDidChangeStack(changed); sinon.stub(repo, 'getPullRequestForBranch').resolves(parentModel()); repo.queryProvider.expectOctokitRequest(['request'], [listRoute, listParams], []); repo.queryProvider.expectOctokitRequest(['request'], ['POST /repos/{owner}/{repo}/stacks', { @@ -459,30 +550,48 @@ describe('PullRequestModel', function () { repo: 'test', headers: listParams.headers, pull_requests: [795, 796], - }], {}); + }], { pull_requests: [{ number: 795 }, { number: 796 }] }); const candidate = await repo.getStackCandidate('D3'); assert.deepStrictEqual(candidate, { parentPullRequestNumber: 795, size: 1, url: 'https://github.com/github/test/pull/795' }); assert(candidate); - await repo.addPullRequestToStack(candidate, 796); + assert.deepStrictEqual(await repo.addPullRequestToStack(candidate, 796), [795, 796]); + assert(changed.calledOnceWithExactly([795, 796])); }); it('creates a stack from multiple existing pull requests in branch order', async function () { const candidate = { parentPullRequestNumber: 795, size: 1, url: 'https://github.com/github/test/pull/795' }; repo.queryProvider.expectOctokitRequest(['request'], ['POST /repos/{owner}/{repo}/stacks', { owner: 'github', repo: 'test', headers: listParams.headers, pull_requests: [795, 796, 797], - }], {}); + }], { pull_requests: [{ number: 795 }, { number: 796 }, { number: 797 }] }); - await repo.addPullRequestsToStack(candidate, [796, 797]); + assert.deepStrictEqual(await repo.addPullRequestsToStack(candidate, [796, 797]), [795, 796, 797]); }); it('extends a stack with multiple existing pull requests in branch order', async function () { + const changed = sinon.spy(); + repo.onDidChangeStack(changed); const candidate = { parentPullRequestNumber: 795, stackNumber: 12, size: 2, url: 'https://github.com/github/test/pull/795' }; repo.queryProvider.expectOctokitRequest(['request'], ['POST /repos/{owner}/{repo}/stacks/{stack_number}/add', { owner: 'github', repo: 'test', headers: listParams.headers, stack_number: 12, pull_requests: [796, 797], - }], {}); + }], { pull_requests: [{ number: 793 }, { number: 795 }, { number: 796 }, { number: 797 }] }); + + assert.deepStrictEqual(await repo.addPullRequestsToStack(candidate, [796, 797]), [793, 795, 796, 797]); + assert(changed.calledOnceWithExactly([793, 795, 796, 797])); + }); - await repo.addPullRequestsToStack(candidate, [796, 797]); + it('rejects malformed stack mutation responses instead of returning incomplete membership', async function () { + const changed = sinon.spy(); + repo.onDidChangeStack(changed); + const candidate = { parentPullRequestNumber: 795, size: 1, url: 'https://github.com/github/test/pull/795' }; + for (const response of [null, {}, { pull_requests: [] }, { pull_requests: [{ number: 795 }, { number: '796' }] }]) { + repo.queryProvider.expectOctokitRequest(['request'], ['POST /repos/{owner}/{repo}/stacks', { + owner: 'github', repo: 'test', headers: listParams.headers, pull_requests: [795, 796], + }], response); + + await assert.rejects(repo.addPullRequestToStack(candidate, 796), /GitHub returned an invalid/); + } + assert(changed.notCalled); }); it('finds the parent from its GraphQL head branch before offering a stack', async function () { @@ -533,12 +642,12 @@ describe('PullRequestModel', function () { headers: listParams.headers, stack_number: 12, pull_requests: [796], - }], {}); + }], { pull_requests: [{ number: 793 }, { number: 795 }, { number: 796 }] }); const candidate = await repo.getStackCandidate('D3'); assert.deepStrictEqual(candidate, { parentPullRequestNumber: 795, stackNumber: 12, size: 2, url: 'https://github.com/github/test/pull/795' }); assert(candidate); - await repo.addPullRequestToStack(candidate, 796); + assert.deepStrictEqual(await repo.addPullRequestToStack(candidate, 796), [793, 795, 796]); }); it('does not offer a stack when the matching PR is not the top', async function () { @@ -621,15 +730,20 @@ describe('PullRequestModel', function () { const unstackArgs = [unstackRoute, { ...params, stack_number: 12 }]; it('dissolves a stack when GitHub returns 204', async function () { + const changed = sinon.spy(); + repo.onDidChangeStack(changed); repo.queryProvider.expectOctokitRequest(['request'], listArgs, [{ number: 12, pull_requests: [{ number: 794 }, { number: 795 }], }]); repo.queryProvider.expectOctokitRequest(['request'], unstackArgs, undefined, 204); assert.deepStrictEqual(await repo.unstackAll(795, [794, 795]), []); + assert(changed.calledOnceWithExactly([794, 795])); }); it('reports locked PRs remaining after unstacking', async function () { + const changed = sinon.spy(); + repo.onDidChangeStack(changed); repo.queryProvider.expectOctokitRequest(['request'], listArgs, [{ number: 12, pull_requests: [{ number: 794 }, { number: 795 }], }]); @@ -638,6 +752,7 @@ describe('PullRequestModel', function () { }, 200); assert.deepStrictEqual(await repo.unstackAll(795, [794, 795]), [794]); + assert(changed.calledOnceWithExactly([794, 795])); }); it('does not unstack a different or missing stack', async function () { @@ -671,12 +786,15 @@ describe('PullRequestModel', function () { }); it('surfaces an unstack failure rather than reporting success', async function () { + const changed = sinon.spy(); + repo.onDidChangeStack(changed); repo.queryProvider.expectOctokitRequest(['request'], listArgs, [{ number: 12, pull_requests: [{ number: 795 }], }]); repo.queryProvider.expectOctokitError(['request'], unstackArgs, new Error('Stack is locked')); await assert.rejects(repo.unstackAll(795, [795]), /Stack is locked/); + assert(changed.notCalled); }); }); diff --git a/src/test/github/pullRequestOverview.test.ts b/src/test/github/pullRequestOverview.test.ts index f047d9fb4a..c614549ad6 100644 --- a/src/test/github/pullRequestOverview.test.ts +++ b/src/test/github/pullRequestOverview.test.ts @@ -23,7 +23,7 @@ import { GitApiImpl } from '../../api/api1'; import { CredentialStore } from '../../github/credentials'; import { GitHubServerType } from '../../common/authentication'; import { GitHubRemote } from '../../common/remote'; -import { CheckState, GithubItemStateEnum, IAccount, PullRequestMergeability, PullRequestStack } from '../../github/interface'; +import { CheckState, GithubItemStateEnum, IAccount, MergeQueueState, PullRequestMergeability, PullRequestStack } from '../../github/interface'; import { CreatePullRequestHelper } from '../../view/createPullRequestHelper'; import { RepositoriesManager } from '../../github/repositoriesManager'; import { MockThemeWatcher } from '../mocks/mockThemeWatcher'; @@ -354,6 +354,7 @@ describe('PullRequestOverview', function () { let getAssignableUsers: SinonStub, ReturnType>; let getReviewRequests: SinonStub<[], ReturnType>; let getPreview: SinonStub<[number], Promise>; + let getMergeQueueMethod: SinonStub; const preview: PullRequestPreview = { number: 1000, title: 'Preview title', titleHTML: 'Preview title', body: 'Preview description', bodyHTML: '

Preview description

', url: 'https://github.com/aaa/bbb/pull/1000', @@ -374,7 +375,7 @@ describe('PullRequestOverview', function () { sinon.stub(prModel, 'getStatusChecks').resolves([{ state: CheckState.Success, statuses: [] }, null]); sinon.stub(prModel, 'getMergeability').resolves({ mergeability: PullRequestMergeability.Mergeable }); sinon.stub(pullRequestManager, 'getBranchNameForPullRequest').resolves(undefined); - sinon.stub(pullRequestManager, 'mergeQueueMethodForBranch').resolves(undefined); + getMergeQueueMethod = sinon.stub(pullRequestManager, 'mergeQueueMethodForBranch').resolves(undefined); sinon.stub(pullRequestManager, 'isHeadUpToDateWithBase').resolves(true); sinon.stub(pullRequestManager, 'getPreferredEmail').resolves(undefined); sinon.stub(pullRequestManager, 'checkBranchUpToDate').resolves(); @@ -431,6 +432,68 @@ describe('PullRequestOverview', function () { sinon.assert.notCalled(getPreview); }); + it('keeps the stack queue method when deferred PR data arrives after the stack', async function () { + sinon.stub(vscode.env, 'asExternalUri').callsFake(async uri => uri); + sinon.stub(prModel, 'getStack').resolves({ + position: 1, size: 1, base: 'stack-target', + pullRequests: [{ + position: 1, number: prModel.number, title: prModel.title, head: 'feature', + url: prModel.html_url, state: GithubItemStateEnum.Open, isDraft: false, mergeable: PullRequestMergeability.Mergeable, + }], + }); + getMergeQueueMethod.callsFake(async branch => branch === 'stack-target' ? 'squash' : 'merge'); + let releaseReviewRequests!: (value: Awaited>) => void; + getReviewRequests.returns(new Promise(resolve => { releaseReviewRequests = resolve; })); + + await openPanel(); + assert.strictEqual(messages.find(message => message.pullrequest?.stackLoaded)?.pullrequest?.mergeQueueMethod, 'squash'); + + releaseReviewRequests([]); + await new Promise(resolve => setImmediate(resolve)); + + const deferred = messages.find(message => message.pullrequest?.status); + assert(deferred); + assert.strictEqual('mergeQueueMethod' in deferred.pullrequest!, false); + }); + + it('resets stack-loaded state when another full overview update starts', async function () { + const getStack = sinon.stub(prModel, 'getStack').resolves(undefined); + await openPanel(); + const panel = PullRequestOverviewPanel.findPanel(remote.owner, remote.repositoryName, prModel.number)!; + assert.strictEqual((panel as any)._stackLoaded, true); + let releaseStack!: (stack: PullRequestStack | undefined) => void; + getStack.returns(new Promise(resolve => { releaseStack = resolve; })); + getMergeQueueMethod.resolves('merge'); + messages.length = 0; + + await openPanel(); + + assert.strictEqual((panel as any)._stackLoaded, false); + assert.strictEqual(messages.find(message => message.pullrequest?.status)?.pullrequest?.mergeQueueMethod, 'merge'); + releaseStack(undefined); + await new Promise(resolve => setImmediate(resolve)); + assert.strictEqual((panel as any)._stackLoaded, true); + }); + + it('serializes initial stack loads across full overview updates and ignores the obsolete result', async function () { + let releaseStack!: (stack: PullRequestStack | undefined) => void; + const getStack = sinon.stub(prModel, 'getStack').resolves(undefined); + getStack.onFirstCall().returns(new Promise(resolve => { releaseStack = resolve; })); + await openPanel(); + const panel = PullRequestOverviewPanel.findPanel(remote.owner, remote.repositoryName, prModel.number)!; + + await openPanel(); + + assert(getStack.calledOnce); + assert.strictEqual(messages.some(message => message.pullrequest?.stackLoaded), false); + releaseStack(undefined); + await (panel as any)._stackRefreshPromise; + + assert(getStack.calledTwice); + assert.strictEqual(messages.filter(message => message.pullrequest?.stackLoaded).length, 1); + assert.strictEqual((panel as any)._stackLoaded, true); + }); + for (const previewHasStarted of [false, true]) { it(`shows a preview during slow initialization when the model resolves ${previewHasStarted ? 'after' : 'before'} the preview query starts`, async function () { let resolveModel: ((model: PullRequestModel) => void) | undefined; @@ -859,14 +922,13 @@ describe('PullRequestOverview', function () { const externalUri = sinon.stub(vscode.env, 'asExternalUri').callsFake(async uri => uri.with({ scheme: 'test-external' })); const { panel, model, stackQuery } = await createPanel(); sinon.stub(pullRequestManager, 'mergeQueueMethodForBranch').resolves(undefined); - const onLoaded = sinon.spy(); const postMessage = sinon.stub(panel as any, '_postMessage').callsFake(async (message: { pullrequest?: { stackLoaded?: boolean } }) => { if (message.pullrequest?.stackLoaded) { - assert(onLoaded.calledOnce); + assert.strictEqual((panel as any)._stackLoaded, true); } }); - await (panel as any).loadStack(model, (panel as any)._updateSequence, onLoaded); + await (panel as any).loadStack(model, (panel as any)._updateSequence); assert(stackQuery.calledOnce); const update = postMessage.getCalls().find(call => call.args[0].pullrequest?.stackLoaded); @@ -884,13 +946,259 @@ describe('PullRequestOverview', function () { it('ignores results from a stale overview update', async function () { const { panel, model } = await createPanel(); const postMessage = sinon.stub(panel as any, '_postMessage').resolves(); - const onLoaded = sinon.spy(); - await (panel as any).loadStack(model, (panel as any)._updateSequence - 1, onLoaded); + await (panel as any).loadStack(model, (panel as any)._updateSequence - 1); + + assert(postMessage.notCalled); + }); + }); + + describe('stack change events', function () { + async function openStackPanel() { + sinon.stub(vscode.env, 'asExternalUri').callsFake(async uri => uri); + const result = await createPanel(); + sinon.stub(pullRequestManager, 'mergeQueueMethodForBranch').resolves(undefined); + const postMessage = sinon.stub(result.panel as any, '_postMessage').resolves(); + const fullRefresh = sinon.stub(result.panel, 'refreshPanel').resolves(); + const stack = await result.stackQuery(); + assert(stack); + await (result.panel as any).refreshStack(); + result.stackQuery.resetHistory(); + postMessage.resetHistory(); + return { ...result, stack, postMessage, fullRefresh }; + } + + it('updates the membership and badge data when a new PR is added', async function () { + const { panel, stackQuery, stack, postMessage, fullRefresh } = await openStackPanel(); + stackQuery.resolves({ + ...stack, size: 3, + pullRequests: [...stack.pullRequests, { ...stack.pullRequests[1], position: 3, number: 1001, head: 'D3' }], + }); + + repo.notifyStackChanged([999, 1000, 1001]); + await (panel as any)._stackRefreshPromise; + + assert(stackQuery.calledOnce); + assert.strictEqual(postMessage.lastCall.args[0].pullrequest.stack.position, 2); + assert.strictEqual(postMessage.lastCall.args[0].pullrequest.stack.size, 3); + assert.deepStrictEqual(postMessage.lastCall.args[0].pullrequest.stack.pullRequests.map(pr => pr.number), [999, 1000, 1001]); + assert(fullRefresh.notCalled); + }); + + it('detects a new stack for a previously unstacked PR', async function () { + const { panel, stackQuery, stack, postMessage } = await openStackPanel(); + stackQuery.resolves(undefined); + await (panel as any).refreshStack(); + postMessage.resetHistory(); + stackQuery.resolves(stack); + + repo.notifyStackChanged([999, 1000]); + await (panel as any)._stackRefreshPromise; + + assert.strictEqual(postMessage.lastCall.args[0].pullrequest.stack.size, 2); + }); + + it('updates sibling draft status when the repository observes a model change', async function () { + const { panel, stackQuery, stack, postMessage, fullRefresh } = await openStackPanel(); + const siblingItem = { + ...convertRESTPullRequestToRawPullRequest(new PullRequestBuilder().number(999).build(), repo), + isDraft: true, + }; + const sibling = repo.createOrUpdatePullRequestModel(siblingItem); + stackQuery.resolves({ + ...stack, pullRequests: [{ ...stack.pullRequests[0], state: GithubItemStateEnum.Open, isDraft: false }, stack.pullRequests[1]], + }); + + sibling.update({ ...sibling.item, isDraft: false }); + await (panel as any)._stackRefreshPromise; + + assert(stackQuery.calledOnce); + assert.strictEqual(postMessage.lastCall.args[0].pullrequest.stack.pullRequests[0].isDraft, false); + assert(fullRefresh.notCalled); + }); + + for (const change of ['head', 'mergeability', 'merge queue']) { + it(`refreshes stack data when a sibling's ${change} changes`, async function () { + const { panel, stackQuery, fullRefresh } = await openStackPanel(); + const sibling = repo.createOrUpdatePullRequestModel( + convertRESTPullRequestToRawPullRequest(new PullRequestBuilder().number(999).build(), repo)); + + if (change === 'head') { + const head = sibling.item.head; + assert(head); + sibling.update({ ...sibling.item, head: { ...head, sha: 'updated-head' } }); + } else if (change === 'mergeability') { + sibling.update({ ...sibling.item, mergeable: PullRequestMergeability.NotMergeable }); + } else { + sibling.update({ + ...sibling.item, + mergeQueueEntry: { position: 1, state: MergeQueueState.Queued, url: 'https://github.com/aaa/bbb/queue' }, + }); + } + await (panel as any)._stackRefreshPromise; + + assert(stackQuery.calledOnce); + assert(fullRefresh.notCalled); + }); + } + + it('updates sibling state without refreshing the entire displayed PR', async function () { + const { panel, stackQuery, stack, postMessage, fullRefresh } = await openStackPanel(); + const sibling = repo.createOrUpdatePullRequestModel( + convertRESTPullRequestToRawPullRequest(new PullRequestBuilder().number(999).build(), repo)); + stackQuery.resolves({ + ...stack, pullRequests: [{ ...stack.pullRequests[0], state: GithubItemStateEnum.Closed }, stack.pullRequests[1]], + }); + + sibling.update({ ...sibling.item, state: 'closed' }); + await (panel as any)._stackRefreshPromise; + + assert(stackQuery.calledOnce); + assert.strictEqual(postMessage.lastCall.args[0].pullrequest.stack.pullRequests[0].state, GithubItemStateEnum.Closed); + assert(fullRefresh.notCalled); + }); + + it('clears stack data when the stack is dissolved', async function () { + const { panel, stackQuery, postMessage } = await openStackPanel(); + stackQuery.resolves(undefined); - assert(onLoaded.notCalled); + repo.notifyStackChanged([999, 1000]); + await (panel as any)._stackRefreshPromise; + + assert.strictEqual(postMessage.lastCall.args[0].pullrequest.stack, undefined); + assert.strictEqual((panel as any)._stackPullRequestNumbers.size, 0); + }); + + it('ignores changes to unrelated PRs and other repositories', async function () { + const { panel, stackQuery } = await openStackPanel(); + const unrelated = repo.createOrUpdatePullRequestModel( + convertRESTPullRequestToRawPullRequest(new PullRequestBuilder().number(5000).build(), repo)); + const otherRemote = new GitHubRemote('other', 'https://github.com/other/repository', + new Protocol('https://github.com/other/repository'), GitHubServerType.GitHubDotCom); + const other = new MockGitHubRepository(otherRemote, credentialStore, telemetry, sinon); + try { + unrelated.update({ ...unrelated.item, isDraft: true }); + repo.notifyStackChanged([5000]); + other.notifyStackChanged([999, 1000]); + await (panel as any)._stackRefreshPromise; + + assert(stackQuery.notCalled); + } finally { + other.dispose(); + } + }); + + it('coalesces a burst of notifications into one stack load', async function () { + const { panel, stackQuery } = await openStackPanel(); + + repo.notifyStackChanged([999, 1000]); + repo.notifyStackChanged([999, 1000]); + repo.notifyStackChanged([999, 1000]); + await (panel as any)._stackRefreshPromise; + + assert(stackQuery.calledOnce); + }); + + it('loads again when a change arrives during an in-flight refresh', async function () { + const { panel, stackQuery, stack } = await openStackPanel(); + let release!: (value: PullRequestStack) => void; + let started!: () => void; + const loading = new Promise(resolve => { started = resolve; }); + stackQuery.onFirstCall().callsFake(() => { + started(); + return new Promise(resolve => { release = resolve; }); + }); + + repo.notifyStackChanged([999, 1000]); + const refresh = (panel as any)._stackRefreshPromise; + await loading; + repo.notifyStackChanged([999, 1000]); + repo.notifyStackChanged([999, 1000]); + assert(stackQuery.calledOnce); + release(stack); + await refresh; + + assert(stackQuery.calledTwice); + }); + + it('serializes stack requests so newer data is published after the previous request completes', async function () { + const { panel, stackQuery, stack, postMessage } = await openStackPanel(); + let release!: (value: PullRequestStack) => void; + let started!: () => void; + const loading = new Promise(resolve => { started = resolve; }); + stackQuery.onFirstCall().callsFake(() => { + started(); + return new Promise(resolve => { release = resolve; }); + }); + stackQuery.onSecondCall().resolves({ ...stack, base: 'updated-base' }); + const refreshing = (panel as any).refreshStack(); + + await loading; + assert.strictEqual((panel as any).refreshStack(), refreshing); + assert(stackQuery.calledOnce); + release(stack); + await refreshing; + + assert(stackQuery.calledTwice); + assert(postMessage.calledTwice); + assert.strictEqual(postMessage.firstCall.args[0].pullrequest.stack.base, stack.base); + assert.strictEqual(postMessage.lastCall.args[0].pullrequest.stack.base, 'updated-base'); + }); + + it('defers a hidden panel refresh until it becomes visible', async function () { + const { panel, model, stackQuery } = await openStackPanel(); + const webviewPanel = (panel as any)._panel as vscode.WebviewPanel; + let visible = false; + sinon.stub(webviewPanel, 'visible').get(() => visible); + sinon.stub(model, 'getLastUpdateTime').resolves(new Date(0)); + + repo.notifyStackChanged([999, 1000]); + await (panel as any)._stackRefreshPromise; + assert(stackQuery.notCalled); + + visible = true; + (panel as any).onDidChangeViewState({ webviewPanel }); + await (panel as any)._stackRefreshPromise; + assert(stackQuery.calledOnce); + }); + + it('ignores notifications and pending responses after disposal', async function () { + const { panel, model, stackQuery, stack, postMessage } = await openStackPanel(); + let release!: (value: PullRequestStack) => void; + stackQuery.returns(new Promise(resolve => { release = resolve; })); + const loading = (panel as any).loadStack(model, (panel as any)._updateSequence); + + panel.dispose(); + repo.notifyStackChanged([999, 1000]); + release(stack); + await loading; + + assert(stackQuery.calledOnce); assert(postMessage.notCalled); }); + + it('does not load stack data for notifications when stacks are disabled', async function () { + const { panel, stackQuery } = await openStackPanel(); + setStacksEnabled(false); + + repo.notifyStackChanged([999, 1000]); + await (panel as any)._stackRefreshPromise; + + assert(stackQuery.notCalled); + }); + + it('surfaces refresh failures and clears the error after a later successful load', async function () { + const { panel, stackQuery, postMessage } = await openStackPanel(); + stackQuery.onFirstCall().rejects(new Error('Stack is unavailable')); + + repo.notifyStackChanged([999, 1000]); + await (panel as any)._stackRefreshPromise; + assert.strictEqual(postMessage.lastCall.args[0].pullrequest.stackLoadError, true); + + repo.notifyStackChanged([999, 1000]); + await (panel as any)._stackRefreshPromise; + assert.strictEqual(postMessage.lastCall.args[0].pullrequest.stackLoadError, false); + }); }); describe('unstackAll', function () { @@ -901,7 +1209,6 @@ describe('PullRequestOverview', function () { const information = sinon.stub(vscode.window, 'showInformationMessage').resolves(undefined); const unstack = sinon.stub(repo, 'unstackAll').resolves([999]); const reply = sinon.stub(panel as any, '_replyMessage').resolves(); - const refresh = sinon.stub(panel, 'refreshPanel').resolves(); const message = { req: '1', command: 'pr.unstack-all', args: undefined }; await (panel as any).unstackAll(message); @@ -911,10 +1218,9 @@ describe('PullRequestOverview', function () { assert.match((confirm.firstCall.args[1] as vscode.MessageOptions).detail!, /Merged, queued, and currently merging pull requests will remain/); assert(unstack.calledOnceWithExactly(1000, [999, 1000])); sinon.assert.calledWithExactly(reply, message, { cancelled: false, remainingPullRequests: [999] }); - assert(refresh.calledOnce); assert(information.calledOnce); assert.match(information.firstCall.args[0], /1 merged, queued, or currently merging pull requests remain/); - sinon.assert.callOrder(unstack, reply, refresh); + sinon.assert.callOrder(unstack, reply); }); it('keeps the confirmed membership when stack data changes while the modal is open', async function () { @@ -963,53 +1269,6 @@ describe('PullRequestOverview', function () { assert.match(throwError.firstCall.args[1], /stack features are disabled/); }); - it('refreshes other visible PR panels in the unstacked stack', async function () { - const { panel } = await createPanel(); - const siblingModel = new PullRequestModel(credentialStore, telemetry, repo, remote, - convertRESTPullRequestToRawPullRequest(new PullRequestBuilder().number(999).build(), repo)); - await PullRequestOverviewPanel.createOrShow(telemetry, EXTENSION_URI, pullRequestManager, - { owner: remote.owner, repo: remote.repositoryName, number: 999 }, siblingModel); - const sibling = PullRequestOverviewPanel.findPanel(remote.owner, remote.repositoryName, 999)!; - const refreshSibling = sinon.stub(sibling, 'refreshPanel').resolves(); - sinon.stub(panel, 'refreshPanel').resolves(); - sinon.stub(panel as any, '_replyMessage').resolves(); - sinon.stub(vscode.window, 'showWarningMessage').resolves('Unstack all' as never); - sinon.stub(vscode.window, 'showInformationMessage').resolves(undefined); - sinon.stub(repo, 'unstackAll').resolves([]); - - await (panel as any).unstackAll({ req: '5', command: 'pr.unstack-all', args: undefined }); - - assert(refreshSibling.calledOnce); - }); - - it('refreshes the stack entry in other open panels when a PR changes draft state', async function () { - const { panel, model } = await createPanel(); - const siblingModel = new PullRequestModel(credentialStore, telemetry, repo, remote, - convertRESTPullRequestToRawPullRequest(new PullRequestBuilder().number(999).build(), repo)); - await PullRequestOverviewPanel.createOrShow(telemetry, EXTENSION_URI, pullRequestManager, - { owner: remote.owner, repo: remote.repositoryName, number: 999 }, siblingModel); - const sibling = PullRequestOverviewPanel.findPanel(remote.owner, remote.repositoryName, 999)!; - const refreshCurrent = sinon.stub(panel, 'refreshPanel').resolves(); - let finishRefresh: () => void; - const refreshedSibling = new Promise(resolve => { finishRefresh = resolve; }); - const refreshSibling = sinon.stub(sibling, 'refreshPanel').callsFake(async () => finishRefresh()); - - (model as any)._onDidChange.fire({ draft: true }); - await refreshedSibling; - - assert(refreshCurrent.calledOnce); - assert(refreshSibling.calledOnce); - }); - - it('refreshes only the requested open stack panels', async function () { - const { panel } = await createPanel(); - const refresh = sinon.stub(panel, 'refreshPanel').resolves(); - - await PullRequestOverviewPanel.refreshStackPanels(remote.owner, remote.repositoryName, [1000, 999]); - - assert(refresh.calledOnce); - }); - it('does not call the Stacks API when confirmation is cancelled', async function () { const { panel } = await createPanel(); sinon.stub(vscode.window, 'showWarningMessage').resolves(undefined); diff --git a/src/test/view/prsTree.test.ts b/src/test/view/prsTree.test.ts index a1e834bbe4..51abaee187 100644 --- a/src/test/view/prsTree.test.ts +++ b/src/test/view/prsTree.test.ts @@ -22,7 +22,6 @@ import { mockTreeViewWorkbench } from '../mocks/mockTreeViewWorkbench'; import { MockGitHubRepository } from '../mocks/mockGitHubRepository'; import { PullRequestGitHelper } from '../../github/pullRequestGitHelper'; import { PullRequestModel } from '../../github/pullRequestModel'; -import { PullRequestOverviewPanel } from '../../github/pullRequestOverview'; import { convertRESTPullRequestToRawPullRequest, parseGraphQLPullRequest } from '../../github/utils'; import { PullRequestBuilder } from '../builders/rest/pullRequestBuilder'; import { PRNode } from '../../view/treeNodes/pullRequestNode'; @@ -229,7 +228,7 @@ describe('GitHub Pull Requests view', function () { assert.match(showError.firstCall.args[0], /stack features are disabled/); }); - it('refreshes selected and existing stack PR panels after adding from the tree', async function () { + it('adds selected PRs without fetching stack membership to refresh panels', async function () { const url = 'https://github.com/aaa/bbb'; const remote = new GitHubRemote('origin', url, new Protocol(url), GitHubServerType.GitHubDotCom); const repository = new MockGitHubRepository(remote, credentialStore, telemetry, sinon); @@ -245,7 +244,7 @@ describe('GitHub Pull Requests view', function () { { position: 2, number: 1, title: 'Bottom', url, head: 'D1', state: GithubItemStateEnum.Open, isDraft: false, mergeable: PullRequestMergeability.Unknown }, ], }; - sinon.stub(bottom, 'getStack').resolves(existing); + const getStack = sinon.stub(bottom, 'getStack').resolves(existing); sinon.stub(top, 'getStack').resolves(undefined); sinon.stub(repository, 'getStackCandidate').resolves({ parentPullRequestNumber: 1, stackNumber: 10, size: 2, url }); sinon.stub(repository, 'getPullRequest').callsFake(async number => number === 1 ? bottom : top); @@ -253,12 +252,11 @@ describe('GitHub Pull Requests view', function () { const confirm = sinon.stub(vscode.window, 'showInformationMessage'); confirm.onFirstCall().resolves('Add to Stack' as never); confirm.onSecondCall().resolves(undefined); - const refresh = sinon.stub(PullRequestOverviewPanel, 'refreshStackPanels').resolves(); await (provider as any).addSelectedPullRequestsToStack(selected[0], selected); assert(add.calledOnce); - assert(refresh.calledOnceWithExactly(remote.owner, remote.repositoryName, [10, 1, 2])); + assert(getStack.notCalled); } finally { repository.dispose(); } diff --git a/src/view/prsTreeDataProvider.ts b/src/view/prsTreeDataProvider.ts index 03ded633b6..4abfe6f229 100644 --- a/src/view/prsTreeDataProvider.ts +++ b/src/view/prsTreeDataProvider.ts @@ -268,14 +268,8 @@ export class PullRequestsTreeDataProvider extends Disposable implements vscode.T if (approved !== confirmation.action) { return; } - const existingStack = candidate.stackNumber !== undefined ? await bottom.getStack() : undefined; - if (candidate.stackNumber !== undefined && !existingStack) { - throw new Error(`Unable to load the existing stack for pull request #${bottom.number}. Refresh the view and try again.`); - } - const added = await addPullRequestsToStack(ordered, candidate); + await addPullRequestsToStack(ordered, candidate); this.refreshAll(true); - await PullRequestOverviewPanel.refreshStackPanels(bottom.remote.owner, bottom.remote.repositoryName, - [...new Set([...(existingStack?.pullRequests.map(pr => pr.number) ?? []), ...added])]); void vscode.window.showInformationMessage(vscode.l10n.t('Pull requests added to the stack.')); } catch (error) { Logger.error(`Failed to add pull requests to stack: ${formatError(error)}`, PullRequestsTreeDataProvider.name); From 52fb0dd3376c51f5894c7318d96c4ecb487432fd Mon Sep 17 00:00:00 2001 From: Alex Ross <38270282+alexr00@users.noreply.github.com> Date: Wed, 7 Oct 2026 17:45:33 +0200 Subject: [PATCH 2/2] CCR --- src/github/pullRequestOverview.ts | 5 ++-- src/test/github/pullRequestOverview.test.ts | 30 ++++++++++++++++++--- 2 files changed, 30 insertions(+), 5 deletions(-) diff --git a/src/github/pullRequestOverview.ts b/src/github/pullRequestOverview.ts index 22dee95b26..5aaa96a066 100644 --- a/src/github/pullRequestOverview.ts +++ b/src/github/pullRequestOverview.ts @@ -761,7 +761,8 @@ export class PullRequestOverviewPanel extends IssueOverviewPanel ({ @@ -786,7 +787,7 @@ export class PullRequestOverviewPanel extends IssueOverviewPanel, }); } diff --git a/src/test/github/pullRequestOverview.test.ts b/src/test/github/pullRequestOverview.test.ts index 9f59b90a83..7e06f7f63e 100644 --- a/src/test/github/pullRequestOverview.test.ts +++ b/src/test/github/pullRequestOverview.test.ts @@ -24,7 +24,7 @@ import { GitApiImpl } from '../../api/api1'; import { CredentialStore } from '../../github/credentials'; import { GitHubServerType } from '../../common/authentication'; import { GitHubRemote } from '../../common/remote'; -import { CheckState, GithubItemStateEnum, IAccount, MergeQueueState, PullRequestMergeability, PullRequestStack } from '../../github/interface'; +import { CheckState, GithubItemStateEnum, IAccount, MergeMethod, MergeQueueState, PullRequestMergeability, PullRequestStack } from '../../github/interface'; import { CreatePullRequestHelper } from '../../view/createPullRequestHelper'; import { RepositoriesManager } from '../../github/repositoriesManager'; import { MockThemeWatcher } from '../mocks/mockThemeWatcher'; @@ -911,7 +911,7 @@ describe('PullRequestOverview', function () { }); sinon.stub(pullRequestManager, 'getAssignableUsers').resolves({}); const branch = sinon.stub(pullRequestManager, 'getBranchNameForPullRequest').resolves(undefined); - sinon.stub(pullRequestManager, 'mergeQueueMethodForBranch').resolves(undefined); + const queueMethod = sinon.stub(pullRequestManager, 'mergeQueueMethodForBranch').resolves(undefined); sinon.stub(pullRequestManager, 'isHeadUpToDateWithBase').resolves(true); sinon.stub(pullRequestManager, 'getPreferredEmail').resolves(undefined); sinon.stub(pullRequestManager, 'checkBranchUpToDate').resolves(); @@ -919,7 +919,7 @@ describe('PullRequestOverview', function () { const externalUri = sinon.stub(vscode.env, 'asExternalUri').callsFake(async uri => uri); const showError = sinon.stub(vscode.window, 'showErrorMessage').resolves(undefined); const query = sinon.spy(repo, 'query'); - return { models, access, branch, externalUri, showError, query }; + return { models, access, branch, queueMethod, externalUri, showError, query }; } async function createPanel(number = 1000) { @@ -1131,6 +1131,30 @@ describe('PullRequestOverview', function () { assert.strictEqual((panel as any)._stackPullRequestNumbers.size, 0); }); + const queueMethods: (MergeMethod | undefined)[] = [undefined, 'merge']; + for (const method of queueMethods) { + it(`restores the PR base queue setting after unstacking ${method ? 'when it has a queue' : 'when it has no queue'}`, async function () { + const { panel, model, stackQuery, stack, postMessage, queueMethod } = await openStackPanel(); + assert.notStrictEqual(model.base.ref, stack.base); + queueMethod.callsFake(async base => base === stack.base ? 'squash' : method); + repo.notifyStackChanged([999, 1000]); + await (panel as any)._stackRefreshPromise; + assert.strictEqual(postMessage.lastCall.args[0].pullrequest.mergeQueueMethod, 'squash'); + queueMethod.resetHistory(); + postMessage.resetHistory(); + stackQuery.resolves(undefined); + + repo.notifyStackChanged([999, 1000]); + await (panel as any)._stackRefreshPromise; + + assert(queueMethod.calledOnceWithExactly(model.base.ref, remote.owner, remote.repositoryName)); + const update = postMessage.lastCall.args[0].pullrequest; + assert.strictEqual(update.stack, undefined); + assert('mergeQueueMethod' in update); + assert.strictEqual(update.mergeQueueMethod, method); + }); + } + it('ignores changes to unrelated PRs and other repositories', async function () { const { panel, stackQuery } = await openStackPanel(); const unrelated = repo.createOrUpdatePullRequestModel(