diff --git a/src/github/pullRequestOverview.ts b/src/github/pullRequestOverview.ts index 610972116d..3c635f13c2 100644 --- a/src/github/pullRequestOverview.ts +++ b/src/github/pullRequestOverview.ts @@ -555,20 +555,43 @@ export class PullRequestOverviewPanel extends IssueOverviewPanel this.isDisposed || updateSequence !== this._updateSequence + || overviewIdentity !== this._identity || this._pendingUpdate !== undefined; this._stackLoaded = false; this._isBranchUpToDateWithBase = undefined; const branchStatusSequence = ++this._branchStatusSequence; const updateStart = performance.now(); this._stackPrefetch = undefined; - if (this._panel.visible) { - // The stack only needs the pull request, so it loads while the rest of the page does, and is shown once the page is. - const data = this.fetchStackData(pullRequestModel); - // A failure is reported when the stack is shown. - data.catch(() => undefined); - this._stackPrefetch = { updateSequence, data }; - } try { + // PR list models may contain Markdown without rendered description HTML. + // Resolve it before the initial webview context is sent. + if (pullRequestModel.body?.trim() && !pullRequestModel.bodyHTML?.trim()) { + const resolvedModel = await this.resolveModel({ + owner: pullRequestModel.remote.owner, + repo: pullRequestModel.remote.repositoryName, + number: pullRequestModel.number, + }); + if (isStale()) { + return; + } + if (!resolvedModel) { + throw new Error(`Failed to resolve Pull Request #${pullRequestModel.number} in ${pullRequestModel.remote.owner}/${pullRequestModel.remote.repositoryName}`); + } + pullRequestModel = resolvedModel; + this._item = resolvedModel; + } + + if (this._panel.visible) { + // The stack only needs the pull request, so it loads while the rest of the page does, and is shown once the page is. + const data = this.fetchStackData(pullRequestModel); + // A failure is reported when the stack is shown. + data.catch(() => undefined); + this._stackPrefetch = { updateSequence, data }; + } + const dataTimings = new Map(); const measure = async (name: string, promise: Promise): Promise => { const start = performance.now(); @@ -592,6 +615,9 @@ export class PullRequestOverviewPanel extends IssueOverviewPanel void; const blockedInitialization = new Promise(resolve => releaseInitialization = resolve); @@ -818,6 +821,8 @@ describe('PullRequestOverview', function () { const firstModel = new PullRequestModel(credentialStore, telemetry, repo, remote, firstItem); const updatedItem = convertRESTPullRequestToRawPullRequest(new PullRequestBuilder().number(1000).title('Updated title').build(), repo); const updatedModel = new PullRequestModel(credentialStore, telemetry, repo, remote, updatedItem); + firstModel.bodyHTML = '

Please merge thx

'; + updatedModel.bodyHTML = '

Please merge thx

'; const identity = { owner: firstModel.remote.owner, repo: firstModel.remote.repositoryName, number: firstModel.number }; const staleEvents: TimelineEvent[] = []; let resolveTimeline: (events: TimelineEvent[]) => void; @@ -931,6 +936,7 @@ describe('PullRequestOverview', function () { beforeEach(function () { const prItem = convertRESTPullRequestToRawPullRequest(new PullRequestBuilder().number(1000).build(), repo); + prItem.bodyHTML = '

Please merge thx

'; prModel = new PullRequestModel(credentialStore, telemetry, repo, remote, prItem); getRepository = sinon.stub(pullRequestManager, 'createGitHubRepositoryFromOwnerName').resolves(repo); getPreview = sinon.stub(repo, 'getPullRequestPreview').resolves(preview); @@ -986,6 +992,109 @@ describe('PullRequestOverview', function () { previewLoadingTests(() => ({ model: prModel, preview, getPreview, getRepository, getRepositoryAccess, getAssignableUsers, openPanel, messages, webviewPanel })); + for (const bodyHTML of [undefined, '', ' ']) { + it(`loads missing description HTML before initialization (${JSON.stringify(bodyHTML)})`, async function () { + prModel.bodyHTML = bodyHTML; + let resolveDescription!: (model: PullRequestModel) => void; + const resolveModel = sinon.stub(pullRequestManager, 'resolvePullRequest') + .returns(new Promise(resolve => resolveDescription = resolve)); + const opening = openPanel(); + await new Promise(resolve => setImmediate(resolve)); + assert.strictEqual(messages.some(message => message.command === 'pr.initialize'), false); + sinon.assert.calledOnce(resolveModel); + sinon.assert.calledWithExactly(resolveModel, remote.owner, remote.repositoryName, prModel.number, false, 'overview'); + prModel.bodyHTML = '

Please merge thx

'; + resolveDescription(prModel); + await opening; + assert.strictEqual(messages.find(message => message.command === 'pr.initialize')?.pullrequest?.bodyHTML, prModel.bodyHTML); + }); + } + + it('initializes with description HTML parsed from a GitHub GraphQL response', async function () { + prModel.bodyHTML = undefined; + repo.addGraphQLPullRequest(builder => { + builder.pullRequest(response => { + response.repository(repository => { + repository.pullRequest(pullRequest => pullRequest.number(prModel.number) + .body('Actual GitHub Markdown').bodyHTML('

Actual GitHub Markdown

')); + }); + }); + }); + const fetchedModel = await repo.getPullRequest(prModel.number, 'description hydration regression', false, false, 'overview'); + assert(fetchedModel); + assert.strictEqual(fetchedModel.bodyHTML, '

Actual GitHub Markdown

'); + sinon.stub(fetchedModel, 'canEdit').resolves(true); + sinon.stub(fetchedModel, 'getReviewRequests').resolves([]); + sinon.stub(fetchedModel, 'getTimelineEvents').resolves([]); + sinon.stub(fetchedModel, 'validateDraftMode').resolves(false); + sinon.stub(fetchedModel, 'getStatusChecks').resolves([{ state: CheckState.Success, statuses: [] }, null]); + sinon.stub(fetchedModel, 'getMergeability').resolves({ mergeability: PullRequestMergeability.Mergeable }); + const resolveModel = sinon.stub(pullRequestManager, 'resolvePullRequest').resolves(fetchedModel); + await openPanel(); + sinon.assert.calledOnce(resolveModel); + assert.strictEqual(messages.find(message => message.command === 'pr.initialize')?.pullrequest?.bodyHTML, + '

Actual GitHub Markdown

'); + }); + + it('ignores description lookups that finish after the overview closes', async function () { + prModel.bodyHTML = undefined; + let resolveDescription!: (model: PullRequestModel) => void; + sinon.stub(pullRequestManager, 'resolvePullRequest') + .returns(new Promise(resolve => resolveDescription = resolve)); + const opening = openPanel(); + await new Promise(resolve => setImmediate(resolve)); + webviewPanel.dispose(); + prModel.bodyHTML = '

Late description

'; + resolveDescription(prModel); + await opening; + assert.strictEqual(messages.some(message => message.command === 'pr.initialize'), false); + }); + + it('does not overwrite a newer overview when an older description lookup finishes', async function () { + prModel.bodyHTML = undefined; + let resolveDescription!: (model: PullRequestModel) => void; + const resolveModel = sinon.stub(pullRequestManager, 'resolvePullRequest') + .returns(new Promise(resolve => resolveDescription = resolve)); + const firstOpening = openPanel(); + await new Promise(resolve => setImmediate(resolve)); + sinon.assert.calledOnce(resolveModel); + const staleModel = new PullRequestModel(credentialStore, telemetry, repo, remote, prModel.item); + staleModel.bodyHTML = '

Old description

'; + prModel.bodyHTML = '

New description

'; + const secondOpening = openPanel(); + resolveDescription(staleModel); + await Promise.all([firstOpening, secondOpening]); + const initializations = messages.filter(message => message.command === 'pr.initialize'); + assert.strictEqual(initializations.length, 1); + assert.strictEqual(initializations[0].pullrequest?.bodyHTML, '

New description

'); + }); + + it('does not resolve a description that already has HTML', async function () { + const resolveModel = sinon.stub(pullRequestManager, 'resolvePullRequest'); + await openPanel(); + sinon.assert.notCalled(resolveModel); + assert.strictEqual(messages.find(message => message.command === 'pr.initialize')?.pullrequest?.bodyHTML, prModel.bodyHTML); + }); + + it('does not resolve an empty description', async function () { + prModel.body = ''; + prModel.bodyHTML = undefined; + const resolveModel = sinon.stub(pullRequestManager, 'resolvePullRequest'); + await openPanel(); + sinon.assert.notCalled(resolveModel); + assert.ok(messages.some(message => message.command === 'pr.initialize')); + }); + + it('reports a description lookup failure instead of initializing a blank body', async function () { + prModel.bodyHTML = undefined; + sinon.stub(pullRequestManager, 'resolvePullRequest').rejects(new Error('Description lookup failed')); + const showError = sinon.stub(vscode.window, 'showErrorMessage').resolves(undefined); + await openPanel(); + assert.strictEqual(showError.firstCall.args[0], 'Error updating pull request description: Description lookup failed'); + assert.strictEqual(messages.some(message => message.command === 'pr.initialize'), false); + }); + + it('loads the stack while the rest of the page loads, and shows it after the page', async function () { const getStack = sinon.stub(prModel, 'getStack').resolves(undefined); let releaseAccess!: () => void;