🐛 Load descriptions before the first overview render

src/github/pullRequestOverview.ts

@@ -555,20 +555,43 @@ export class PullRequestOverviewPanel extends IssueOverviewPanel<PullRequestMode
 		this._item = pullRequestModel;
 		this._diffLinkHashMapPromise = undefined;
 		const updateSequence = ++this._updateSequence;
+		const overviewIdentity = this._identity;
+		// An earlier load must not overwrite a closed panel or a newer request.
+		const isStale = () => 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<string, number>();
 			const measure = async <T>(name: string, promise: Promise<T>): Promise<T> => {
 				const start = performance.now();
@@ -592,6 +615,9 @@ export class PullRequestOverviewPanel extends IssueOverviewPanel<PullRequestMode
 				currentUser,
 				viewerCanEdit,
 			] = await updatingPromise;
+			if (isStale()) {
+				return;
+			}
 			const pullRequest = pullRequestModel;
 			const timelineEvents = cachedTimelineEvents;
 			const dataLoadDuration = performance.now() - updateStart;
@@ -631,6 +657,9 @@ export class PullRequestOverviewPanel extends IssueOverviewPanel<PullRequestMode
 				closingIssuesPromise,
 				this.getInitialStack(pullRequest),
 			]);
+			if (isStale()) {
+				return;
+			}
 			const contextDuration = performance.now() - contextStart;
 			this._canUpdateStackAccess = baseContext.hasWritePermission;
 			// Reloading the pull request keeps the recommendation that is shown, so that the page does not move,
@@ -810,7 +839,9 @@ export class PullRequestOverviewPanel extends IssueOverviewPanel<PullRequestMode
 				this._folderRepositoryManager.checkBranchUpToDate(pullRequest, true);
 			}
 		} catch (e) {
-			vscode.window.showErrorMessage(`Error updating pull request description: ${formatError(e)}`);
+			if (!isStale()) {
+				vscode.window.showErrorMessage(`Error updating pull request description: ${formatError(e)}`);
+			}
 		}
 	}
 

src/test/github/pullRequestOverview.test.ts

@@ -623,6 +623,7 @@ describe('PullRequestOverview', function () {
 			it(`initializes with the latest checkout state when ${initiallyCheckedOut ? 'leaving' : 'entering'} review mode during loading`, async function () {
 				const model = new PullRequestModel(credentialStore, telemetry, repo, remote,
 					convertRESTPullRequestToRawPullRequest(new PullRequestBuilder().number(1000).build(), repo));
+				model.bodyHTML = '<p>Please merge thx</p>';
 				const identity = { owner: remote.owner, repo: remote.repositoryName, number: model.number };
 				pullRequestManager.activePullRequest = initiallyCheckedOut ? model : undefined;
 				sinon.stub(pullRequestManager, 'getCurrentUser').resolves(model.author);
@@ -789,6 +790,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 = '<p>Please merge thx</p>';
+			updatedModel.bodyHTML = '<p>Please merge thx</p>';
 			const identity = { owner: firstModel.remote.owner, repo: firstModel.remote.repositoryName, number: firstModel.number };
 			let releaseInitialization: (defaultBranch: string) => void;
 			const blockedInitialization = new Promise<string>(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 = '<p>Please merge thx</p>';
+			updatedModel.bodyHTML = '<p>Please merge thx</p>';
 			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 = '<p>Please merge thx</p>';
 			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 = '<p>Please merge thx</p>';
+				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('<p>Actual GitHub Markdown</p>'));
+					});
+				});
+			});
+			const fetchedModel = await repo.getPullRequest(prModel.number, 'description hydration regression', false, false, 'overview');
+			assert(fetchedModel);
+			assert.strictEqual(fetchedModel.bodyHTML, '<p>Actual GitHub Markdown</p>');
+			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,
+				'<p>Actual GitHub Markdown</p>');
+		});
+
+		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 = '<p>Late description</p>';
+			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 = '<p>Old description</p>';
+			prModel.bodyHTML = '<p>New description</p>';
+			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, '<p>New description</p>');
+		});
+
+		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;