From de463253149a66c2b93eda946b90d7bb139f6fa6 Mon Sep 17 00:00:00 2001 From: Alex Ross <38270282+alexr00@users.noreply.github.com> Date: Thu, 8 Oct 2026 17:24:17 +0200 Subject: [PATCH] Update webviews after stack/unstack operation Fixes #9018 --- src/github/activityBarViewProvider.ts | 73 +++-- src/github/pullRequestOverview.ts | 4 +- src/github/views.ts | 6 +- .../github/activityBarViewProvider.test.ts | 296 ++++++++++++++++++ src/test/github/pullRequestOverview.test.ts | 11 +- webviews/editorWebview/test/overview.test.tsx | 52 +++ 6 files changed, 408 insertions(+), 34 deletions(-) create mode 100644 src/test/github/activityBarViewProvider.test.ts diff --git a/src/github/activityBarViewProvider.ts b/src/github/activityBarViewProvider.ts index 9ac08527bc..5b4097e603 100644 --- a/src/github/activityBarViewProvider.ts +++ b/src/github/activityBarViewProvider.ts @@ -29,11 +29,12 @@ export class PullRequestViewProvider extends WebviewViewBase implements vscode.W public override readonly viewType = 'github:activePullRequest'; private _existingReviewers: ReviewState[] = []; private _updatingPromise: Promise | undefined; + private _stackUpdateSequence = 0; constructor( extensionUri: vscode.Uri, private readonly _folderRepositoryManager: FolderRepositoryManager, - private readonly _reviewManager: ReviewManager, + private readonly _reviewManager: Pick, private _item: PullRequestModel, ) { super(extensionUri); @@ -173,6 +174,49 @@ export class PullRequestViewProvider extends WebviewViewBase implements vscode.W } })); this._prDisposables.push(pullRequestModel.onDidChangePendingReviewState(() => this.updatePullRequest(pullRequestModel))); + this._prDisposables.push(pullRequestModel.githubRepository.onDidChangeStack(numbers => { + if (numbers.includes(this._item.number)) { + void this.refreshStack(); + } + })); + } + + public override dispose(): void { + disposeAll(this._prDisposables ?? []); + this._updatePendingVisibility?.dispose(); + super.dispose(); + } + + private async refreshStack(): Promise { + const updateSequence = ++this._stackUpdateSequence; + const pullRequest = this._item; + if (this.isDisposed || !this._view || !areStacksEnabled()) { + return; + } + try { + const stack = await pullRequest.getStack(); + if (this.isDisposed || updateSequence !== this._stackUpdateSequence || !areStacksEnabled()) { + return; + } + const mergeQueueMethod = await this._folderRepositoryManager.mergeQueueMethodForBranch( + stack?.base ?? pullRequest.base.ref, pullRequest.remote.owner, pullRequest.remote.repositoryName); + if (!this.isDisposed && updateSequence === this._stackUpdateSequence && areStacksEnabled()) { + await this._postMessage({ + command: 'pr.update', + pullrequest: { + stack: stack ?? null, + stackLoaded: true, + stackLoadError: false, + mergeQueueMethod: mergeQueueMethod ?? null, + } satisfies Partial, + }); + } + } catch (error) { + Logger.error(`Failed to load active pull request stack: ${formatError(error)}`, PullRequestViewProvider.name); + if (!this.isDisposed && updateSequence === this._stackUpdateSequence && areStacksEnabled()) { + void this._postMessage({ command: 'pr.update', pullrequest: { stackLoadError: true } satisfies Partial }); + } + } } private _updatePendingVisibility: vscode.Disposable | undefined = undefined; @@ -189,6 +233,7 @@ export class PullRequestViewProvider extends WebviewViewBase implements vscode.W } try { + this._stackUpdateSequence++; if (this._view && !this._view.visible) { this._updatePendingVisibility?.dispose(); this._updatePendingVisibility = this._view.onDidChangeVisibility(async () => { @@ -197,7 +242,7 @@ export class PullRequestViewProvider extends WebviewViewBase implements vscode.W }); } - if ((this._prDisposables === undefined) || (pullRequestModel.number !== this._item.number)) { + if ((this._prDisposables === undefined) || !isSamePullRequest) { this.registerPrSpecificListeners(pullRequestModel); } this._item = pullRequestModel; @@ -329,29 +374,7 @@ export class PullRequestViewProvider extends WebviewViewBase implements vscode.W command: 'pr.initialize', pullrequest: context, }); - if (areStacksEnabled()) { - void pullRequest.getStack().then(async stack => { - if (!this._item.equals(pullRequest) || !areStacksEnabled()) { - return; - } - const stackQueueMethod = stack ? await this._folderRepositoryManager.mergeQueueMethodForBranch(stack.base, pullRequest.remote.owner, pullRequest.remote.repositoryName) : undefined; - if (this._item.equals(pullRequest) && areStacksEnabled()) { - this._postMessage({ - command: 'pr.update', - pullrequest: { - stack, - stackLoaded: true, - ...(stack ? { mergeQueueMethod: stackQueueMethod } : {}), - } satisfies Partial, - }); - } - }).catch(error => { - Logger.error(`Failed to load active pull request stack: ${formatError(error)}`, PullRequestViewProvider.name); - if (this._item.equals(pullRequest) && areStacksEnabled()) { - this._postMessage({ command: 'pr.update', pullrequest: { stackLoadError: true } satisfies Partial }); - } - }); - } + void this.refreshStack(); } catch (e) { vscode.window.showErrorMessage(`Error updating active pull request view: ${formatError(e)}`); diff --git a/src/github/pullRequestOverview.ts b/src/github/pullRequestOverview.ts index 4a02b801b0..a5e3261a7b 100644 --- a/src/github/pullRequestOverview.ts +++ b/src/github/pullRequestOverview.ts @@ -782,11 +782,11 @@ export class PullRequestOverviewPanel extends IssueOverviewPanel, }); } diff --git a/src/github/views.ts b/src/github/views.ts index 9795b2ac75..fe4b503ea5 100644 --- a/src/github/views.ts +++ b/src/github/views.ts @@ -80,7 +80,8 @@ export type PullRequestPreview = Pick; export interface PullRequest extends Issue { - stack?: PullRequestStack; + /** Use null to clear a previous value in serialized webview updates. */ + stack?: PullRequestStack | null; canUpdateStack?: boolean; stackLoaded?: boolean; stackLoadError?: boolean; @@ -109,7 +110,8 @@ export interface PullRequest extends Issue { autoMerge?: boolean; allowAutoMerge: boolean; autoMergeMethod?: MergeMethod; - mergeQueueMethod: MergeMethod | undefined; + /** Use null to clear a previous value in serialized webview updates. */ + mergeQueueMethod: MergeMethod | undefined | null; mergeQueueEntry?: { url: string; position: number; diff --git a/src/test/github/activityBarViewProvider.test.ts b/src/test/github/activityBarViewProvider.test.ts new file mode 100644 index 0000000000..08fc2ec436 --- /dev/null +++ b/src/test/github/activityBarViewProvider.test.ts @@ -0,0 +1,296 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. All rights reserved. + * Licensed under the MIT License. See License.txt in the project root for license information. + *--------------------------------------------------------------------------------------------*/ + +import { default as assert } from 'assert'; +import * as vscode from 'vscode'; +import { createSandbox, SinonSandbox, SinonStub } from 'sinon'; +import { GitApiImpl } from '../../api/api1'; +import { GitHubServerType } from '../../common/authentication'; +import Logger from '../../common/logger'; +import { Protocol } from '../../common/protocol'; +import { GitHubRemote } from '../../common/remote'; +import { EXTENSION_ID } from '../../constants'; +import { PullRequestViewProvider } from '../../github/activityBarViewProvider'; +import { CredentialStore } from '../../github/credentials'; +import { FolderRepositoryManager } from '../../github/folderRepositoryManager'; +import { GithubItemStateEnum, MergeMethod, PullRequestMergeability, PullRequestStack } from '../../github/interface'; +import { PullRequestModel } from '../../github/pullRequestModel'; +import { RepositoriesManager } from '../../github/repositoriesManager'; +import { convertRESTPullRequestToRawPullRequest } from '../../github/utils'; +import { PullRequest } from '../../github/views'; +import { CreatePullRequestHelper } from '../../view/createPullRequestHelper'; +import { ReviewManager } from '../../view/reviewManager'; +import { PullRequestBuilder } from '../builders/rest/pullRequestBuilder'; +import { MockCommandRegistry } from '../mocks/mockCommandRegistry'; +import { MockExtensionContext } from '../mocks/mockExtensionContext'; +import { MockGitHubRepository } from '../mocks/mockGitHubRepository'; +import { MockRepository } from '../mocks/mockRepository'; +import { mockStackSetting } from '../mocks/mockStackSetting'; +import { MockTelemetry } from '../mocks/mockTelemetry'; +import { MockThemeWatcher } from '../mocks/mockThemeWatcher'; + +interface ViewMessage { + command: string; + pullrequest?: Partial; +} + +class TestPullRequestViewProvider extends PullRequestViewProvider { + public readonly messages: ViewMessage[] = []; + + public attachView(panel: vscode.WebviewPanel): void { + const visibility = this._register(new vscode.EventEmitter()); + this._view = { + viewType: this.viewType, + webview: panel.webview, + visible: true, + title: '', + onDidDispose: panel.onDidDispose, + onDidChangeVisibility: visibility.event, + show: () => undefined, + }; + } + + protected override async _postMessage(message: ViewMessage): Promise { + this.messages.push(JSON.parse(JSON.stringify(message))); + } +} + +describe('Active pull request stack changes', function () { + let sinon: SinonSandbox; + let context: MockExtensionContext; + let credentials: CredentialStore; + let repositoriesManager: RepositoriesManager; + let folderManager: FolderRepositoryManager; + let repo: MockGitHubRepository; + let provider: TestPullRequestViewProvider; + let models: Map; + let model: PullRequestModel; + let stackQuery: SinonStub<[], Promise>; + let queueMethod: SinonStub; + let setStacksEnabled: (enabled: boolean) => void; + const stack: PullRequestStack = { + position: 2, size: 3, base: 'main', + pullRequests: [999, 1000, 1001].map((number, index) => ({ + position: index + 1, number, title: `Change ${index + 1}`, url: '', head: `D${index + 1}`, + state: GithubItemStateEnum.Open, isDraft: false, mergeable: PullRequestMergeability.Mergeable, + })), + }; + + function createModel(number: number) { + const item = convertRESTPullRequestToRawPullRequest(new PullRequestBuilder().number(number).build(), repo); + const result = new PullRequestModel(credentials, folderManager.telemetry, repo, repo.remote, item); + models.set(number, result); + sinon.stub(result, 'getTimelineEvents').resolves([]); + sinon.stub(result, 'getReviewRequests').resolves([]); + sinon.stub(result, 'canEdit').resolves(true); + sinon.stub(result, 'validateDraftMode').resolves(false); + sinon.stub(result, 'getCoAuthors').resolves([]); + return result; + } + + async function settleUpdates(): Promise { + await new Promise(resolve => setImmediate(resolve)); + } + + async function initialize(): Promise { + await provider.updatePullRequest(model); + await settleUpdates(); + provider.messages.length = 0; + stackQuery.resetHistory(); + } + + function latestStackUpdate(): Partial { + const updates = provider.messages.filter(message => message.command === 'pr.update'); + assert(updates.length > 0); + return updates[updates.length - 1].pullrequest!; + } + + beforeEach(function () { + sinon = createSandbox(); + MockCommandRegistry.install(sinon); + sinon.stub(vscode.window, 'showErrorMessage').callsFake(message => { + assert.fail(message); + }); + setStacksEnabled = mockStackSetting(sinon); + context = new MockExtensionContext(); + context.extensionUri = vscode.extensions.getExtension(EXTENSION_ID)!.extensionUri; + const telemetry = new MockTelemetry(); + credentials = new CredentialStore(telemetry, context); + repositoriesManager = new RepositoriesManager(credentials, telemetry); + folderManager = new FolderRepositoryManager(0, context, new MockRepository(), telemetry, + new GitApiImpl(repositoriesManager), credentials, new CreatePullRequestHelper(), new MockThemeWatcher()); + const url = 'https://github.com/aaa/bbb'; + const remote = new GitHubRemote('origin', url, new Protocol(url), GitHubServerType.GitHubDotCom); + repo = new MockGitHubRepository(remote, credentials, telemetry, sinon); + models = new Map(); + model = createModel(1000); + stackQuery = sinon.stub(model, 'getStack').resolves(stack); + sinon.stub(folderManager, 'resolvePullRequest').callsFake(async (_owner, _repo, number) => models.get(number)); + sinon.stub(folderManager, 'getPullRequestRepositoryAccessAndMergeMethods').resolves({ + hasWritePermission: true, + mergeMethodsAvailability: { merge: true, squash: true, rebase: true }, + viewerCanAutoMerge: false, + }); + sinon.stub(folderManager, 'getBranchNameForPullRequest').resolves(undefined); + sinon.stub(folderManager, 'getPullRequestRepositoryDefaultBranch').resolves('main'); + sinon.stub(folderManager, 'getCurrentUser').resolves(model.author); + queueMethod = sinon.stub(folderManager, 'mergeQueueMethodForBranch').resolves(undefined); + provider = new TestPullRequestViewProvider(context.extensionUri, folderManager, + sinon.createStubInstance(ReviewManager), model); + const panel = vscode.window.createWebviewPanel('testActivePullRequest', '', vscode.ViewColumn.One, {}); + context.subscriptions.push(panel); + provider.attachView(panel); + }); + + afterEach(function () { + provider.dispose(); + for (const item of models.values()) { + item.dispose(); + } + folderManager.dispose(); + repositoriesManager.dispose(); + repo.dispose(); + credentials.dispose(); + context.dispose(); + sinon.restore(); + }); + + it('clears a three-pull-request stack from the sidebar after unstacking', async function () { + await initialize(); + stackQuery.resolves(undefined); + repo.notifyStackChanged([999, 1000, 1001]); + await settleUpdates(); + + assert(stackQuery.calledOnce); + assert.deepStrictEqual(latestStackUpdate(), { + stack: null, stackLoaded: true, stackLoadError: false, mergeQueueMethod: null, + }); + }); + + it('updates the remaining stack after partially unstacking', async function () { + await initialize(); + const remaining = { ...stack, size: 2, pullRequests: stack.pullRequests.slice(0, 2) }; + stackQuery.resolves(remaining); + repo.notifyStackChanged([999, 1000, 1001]); + await settleUpdates(); + + assert.deepStrictEqual(latestStackUpdate().stack, remaining); + }); + + it('detects a stack added to a previously unstacked pull request', async function () { + stackQuery.resolves(undefined); + await initialize(); + stackQuery.resolves(stack); + repo.notifyStackChanged([999, 1000, 1001]); + await settleUpdates(); + + assert.deepStrictEqual(latestStackUpdate().stack, stack); + }); + + for (const method of [undefined, 'merge'] as (MergeMethod | undefined)[]) { + it(`restores the pull request base branch queue method (${method ?? 'none'}) after unstacking`, async function () { + queueMethod.callsFake(async branch => branch === stack.base ? 'squash' : method); + await initialize(); + stackQuery.resolves(undefined); + repo.notifyStackChanged([1000]); + await settleUpdates(); + + assert('mergeQueueMethod' in latestStackUpdate()); + assert.strictEqual(latestStackUpdate().mergeQueueMethod, method ?? null); + assert(queueMethod.lastCall.calledWithExactly(model.base.ref, model.remote.owner, model.remote.repositoryName)); + }); + } + + it('ignores notifications for unrelated pull requests and repositories', async function () { + await initialize(); + const url = 'https://github.com/other/repository'; + const other = new MockGitHubRepository(new GitHubRemote('other', url, new Protocol(url), GitHubServerType.GitHubDotCom), + credentials, folderManager.telemetry, sinon); + try { + repo.notifyStackChanged([2000]); + other.notifyStackChanged([1000]); + await settleUpdates(); + assert(stackQuery.notCalled); + assert.strictEqual(provider.messages.length, 0); + } finally { + other.dispose(); + } + }); + + it('does not load stacks when the feature is disabled', async function () { + setStacksEnabled(false); + await initialize(); + repo.notifyStackChanged([1000]); + await settleUpdates(); + + assert(stackQuery.notCalled); + assert.strictEqual(provider.messages.length, 0); + }); + + it('ignores an older stack load that completes after unstacking', async function () { + let release!: (value: PullRequestStack) => void; + stackQuery.onFirstCall().returns(new Promise(resolve => { release = resolve; })); + await provider.updatePullRequest(model); + assert.strictEqual(stackQuery.callCount, 1); + provider.messages.length = 0; + stackQuery.onSecondCall().resolves(undefined); + repo.notifyStackChanged([1000]); + await settleUpdates(); + assert.strictEqual(stackQuery.callCount, 2); + assert.strictEqual(latestStackUpdate().stack, null); + release(stack); + await settleUpdates(); + + assert.strictEqual(provider.messages.length, 1); + assert.strictEqual(latestStackUpdate().stack, null); + }); + + it('reports stack load failures and clears the error after a successful refresh', async function () { + await initialize(); + const logError = sinon.stub(Logger, 'error'); + stackQuery.rejects(new Error('Stack unavailable')); + repo.notifyStackChanged([1000]); + await settleUpdates(); + assert.strictEqual(latestStackUpdate().stackLoadError, true); + assert(logError.calledOnce); + + stackQuery.resolves(undefined); + repo.notifyStackChanged([1000]); + await settleUpdates(); + assert.strictEqual(latestStackUpdate().stackLoadError, false); + }); + + it('registers stack listeners for the new active pull request', async function () { + await initialize(); + const next = createModel(2000); + const nextStackQuery = sinon.stub(next, 'getStack').resolves(undefined); + await provider.updatePullRequest(next); + await settleUpdates(); + provider.messages.length = 0; + nextStackQuery.resetHistory(); + + repo.notifyStackChanged([1000]); + await settleUpdates(); + assert(nextStackQuery.notCalled); + repo.notifyStackChanged([2000]); + await settleUpdates(); + assert(nextStackQuery.calledOnce); + assert.strictEqual(latestStackUpdate().stack, null); + }); + + it('does not refresh or publish a pending stack load after disposal', async function () { + await initialize(); + let release!: (value: PullRequestStack) => void; + stackQuery.returns(new Promise(resolve => { release = resolve; })); + repo.notifyStackChanged([1000]); + provider.dispose(); + repo.notifyStackChanged([1000]); + release(stack); + await settleUpdates(); + + assert(stackQuery.calledOnce); + assert.strictEqual(provider.messages.length, 0); + }); +}); diff --git a/src/test/github/pullRequestOverview.test.ts b/src/test/github/pullRequestOverview.test.ts index 61138364b3..cfeb3251ed 100644 --- a/src/test/github/pullRequestOverview.test.ts +++ b/src/test/github/pullRequestOverview.test.ts @@ -1184,7 +1184,8 @@ describe('PullRequestOverview', function () { stackQuery.resolves(undefined); repo.notifyStackChanged([999, 1000]); await (panel as any)._stackRefreshPromise; - assert.strictEqual(postMessage.lastCall.args[0].pullrequest.stack, undefined); + const message = JSON.parse(JSON.stringify(postMessage.lastCall.args[0])); + assert.strictEqual(message.pullrequest.stack, null); assert.strictEqual((panel as any)._stackPullRequestNumbers.size, 0); }); @@ -1205,10 +1206,10 @@ describe('PullRequestOverview', function () { 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); + const update = JSON.parse(JSON.stringify(postMessage.lastCall.args[0])).pullrequest; + assert.strictEqual(update.stack, null); assert('mergeQueueMethod' in update); - assert.strictEqual(update.mergeQueueMethod, method); + assert.strictEqual(update.mergeQueueMethod, method ?? null); }); } @@ -1458,7 +1459,7 @@ describe('PullRequestOverview', function () { stackQuery.resolves(undefined); repo.notifyStackChanged([999, 1000]); await (panel as any)._stackRefreshPromise; - assert.strictEqual(postMessage.lastCall.args[0].pullrequest.stack, undefined); + assert.strictEqual(postMessage.lastCall.args[0].pullrequest.stack, null); assert.strictEqual(postMessage.lastCall.args[0].pullrequest.canUpdateStack, false); }); diff --git a/webviews/editorWebview/test/overview.test.tsx b/webviews/editorWebview/test/overview.test.tsx index 65ef93c574..eef717bd02 100644 --- a/webviews/editorWebview/test/overview.test.tsx +++ b/webviews/editorWebview/test/overview.test.tsx @@ -12,8 +12,10 @@ import { createSandbox, SinonSandbox } from 'sinon'; import { PullRequestBuilder } from './builder/pullRequest'; import { CheckState, GithubItemStateEnum, PullRequestCheckStatus, PullRequestMergeability } from '../../../src/github/interface'; +import { Root as ActivityBarRoot } from '../../activityBarView/app'; import { Overview as ActivityBarOverview } from '../../activityBarView/overview'; import { PRContext, default as PullRequestContext } from '../../common/context'; +import { Root as EditorRoot } from '../app'; import { Overview } from '../overview'; describe('Overview', function () { @@ -362,6 +364,56 @@ describe('Overview', function () { assert(out.getByText('Merge Pull Request')); }); + for (const [view, Root, Component] of [ + ['sidebar', ActivityBarRoot, ActivityBarOverview], + ['editor', EditorRoot, Overview], + ] as const) { + for (const mergeQueueMethod of [undefined, 'squash'] as const) { + it(`clears the ${view} stack after a serialized unstack update (${mergeQueueMethod ?? 'no queue'})`, async function () { + const pr = new PullRequestBuilder().number(1001).mergeQueueMethod(mergeQueueMethod).stack({ + position: 3, size: 3, base: 'main', + pullRequests: [999, 1000, 1001].map((number, index) => ({ + position: index + 1, number, title: `Change ${index + 1}`, head: `D${index + 1}`, url: '', + state: GithubItemStateEnum.Open, isDraft: false, mergeable: PullRequestMergeability.Mergeable, + })), + }).build(); + const context = new PRContext(pr); + context.setPR(pr); + const out = render( + + {current => } + , + ); + const stackAction = mergeQueueMethod ? 'Add stack to merge queue' : 'Merge stack (3 pull requests)'; + assert(out.getByText(stackAction)); + if (view === 'editor') { + assert(out.container.querySelector('#pull-request-stack')); + assert(out.container.querySelector('.stack-badge')); + } + + context.handleMessage(JSON.parse(JSON.stringify({ + command: 'pr.update', + pullrequest: { stack: null, stackLoaded: true, stackLoadError: false, mergeQueueMethod: null }, + }))); + + await wait(() => { + assert.strictEqual(out.queryByText(stackAction), null); + assert.strictEqual(out.container.querySelector('#pull-request-stack'), null); + assert.strictEqual(out.container.querySelector('.stack-badge'), null); + assert.strictEqual(context.pr?.stack, null); + assert.strictEqual(context.pr?.mergeQueueMethod, null); + if (view === 'sidebar') { + assert.strictEqual(out.container.querySelector('.select-control input[type="submit"]')?.getAttribute('value'), 'Create Merge Commit'); + assert(out.getByText('Comment')); + assert(out.getByText("Checkout 'main'")); + } else { + assert(out.getByText('Merge Pull Request')); + } + }, { timeout: 500 }); + }); + } + } + it('offers Unstack all for an eligible stack without showing it to users without write permission', function () { const stack = { position: 2, size: 2, base: 'main',