Load a preview in the webview first

package.json

@@ -4288,8 +4288,9 @@
 		"compile:web": "webpack --mode development --config-name extension:webworker --config-name webviews",
 		"lint": "eslint --fix --cache . --ext .ts,.tsx",
 		"package": "npx vsce package",
-		"test": "npm run test:preprocess && npm run test:scripts && node ./out/src/test/runTests.js",
+		"test": "npm run test:preprocess && npm run test:scripts && npm run test:webviews && node ./out/src/test/runTests.js",
 		"test:scripts": "mocha \"out/src/test/scripts/**/*.test.js\"",
+		"test:webviews": "node scripts/test-webviews.js",
 		"test:preprocess": "npm run compile:test && npm run test:preprocess-gql && npm run test:preprocess-svg && npm run test:preprocess-fixtures",
 		"browsertest:preprocess": "tsc ./src/test/browser/runTests.ts --outDir ./dist/browser/test --rootDir ./src/test/browser --target es6 --module commonjs",
 		"browsertest": "npm run browsertest:preprocess && node ./dist/browser/test/runTests.js",

scripts/test-webviews.js

@@ -0,0 +1,81 @@
+/*---------------------------------------------------------------------------------------------
+ *  Copyright (c) Microsoft Corporation. All rights reserved.
+ *  Licensed under the MIT License. See License.txt in the project root for license information.
+ *--------------------------------------------------------------------------------------------*/
+
+const path = require('path');
+const glob = require('glob');
+const Mocha = require('mocha');
+const webpack = require('webpack');
+const installJsDomGlobal = require('jsdom-global');
+
+async function main() {
+	const root = path.resolve(__dirname, '..');
+	const tests = glob.sync('webviews/**/test/**/*.test.{ts,tsx}', { cwd: root, absolute: true }).sort();
+	if (!tests.length) {
+		throw new Error('No webview test files found.');
+	}
+
+	const configs = await require('../webpack.config')({ esbuild: true }, { mode: 'development' });
+	const config = configs.find(config => config.name === 'webviews');
+	config.entry = [path.join(root, 'src', 'test', 'webviews', 'setup.ts'), ...tests];
+	config.target = 'node';
+	config.output = { path: path.join(root, 'out', 'webview-tests'), filename: 'index.js' };
+	config.externals = [({ request }, callback) => {
+		if (!request.startsWith('.') && !path.isAbsolute(request)) {
+			callback(null, 'commonjs ' + request);
+		} else {
+			callback();
+		}
+	}];
+
+	await new Promise((resolve, reject) => {
+		const compiler = webpack(config);
+		compiler.run((error, stats) => compiler.close(closeError => {
+			if (error || closeError) {
+				reject(error ?? closeError);
+			} else if (stats.hasErrors()) {
+				reject(new Error(stats.toString('errors-warnings')));
+			} else {
+				if (stats.hasWarnings()) {
+					process.stderr.write(stats.toString('errors-warnings') + '\n');
+				}
+				resolve();
+			}
+		}));
+	});
+
+	const mocha = new Mocha({ ui: 'bdd', color: true, failZero: true });
+	if (process.env.TEST_JUNIT_XML_PATH) {
+		const report = process.env.TEST_JUNIT_XML_PATH;
+		mocha.reporter('mocha-multi-reporters', {
+			reporterEnabled: 'mocha-junit-reporter, spec',
+			mochaJunitReporterReporterOptions: {
+				mochaFile: path.join(path.dirname(report), `webviews-${path.basename(report)}`),
+				suiteTitleSeparatedBy: ' / ',
+				outputs: true,
+			},
+		});
+	}
+
+	const cleanup = installJsDomGlobal('', { pretendToBeVisual: true });
+	// Match JSDOM's APIs rather than exposing Node's MessageChannel to React's scheduler.
+	const messageChannel = global.MessageChannel;
+	global.MessageChannel = window.MessageChannel;
+	try {
+		require('source-map-support').install();
+		mocha.addFile(path.join(config.output.path, config.output.filename));
+		const failures = await new Promise(resolve => mocha.run(resolve));
+		process.exitCode = failures ? 1 : 0;
+	} finally {
+		mocha.dispose();
+		window.close();
+		cleanup();
+		global.MessageChannel = messageChannel;
+	}
+}
+
+main().catch(error => {
+	process.stderr.write(`${error.stack ?? error}\n`);
+	process.exitCode = 1;
+});

src/commands.ts

@@ -565,14 +565,14 @@ export function registerCommands(
 
 		}));
 
-	const resolvePr = async (context: BaseContext | undefined): Promise<{ folderManager: FolderRepositoryManager, pr: PullRequestModel } | undefined> => {
+	const resolvePr = async (context: BaseContext | undefined, loadMode: 'default' | 'overview' = 'default'): Promise<{ folderManager: FolderRepositoryManager, pr: PullRequestModel } | undefined> => {
 		if (!context) {
 			return undefined;
 		}
 
 		const folderManager = folderRepositoryManagerResolver.getManagerForRepository(context.owner, context.repo);
 
-		const pr = await folderManager.resolvePullRequest(context.owner, context.repo, context.number, true);
+		const pr = await folderManager.resolvePullRequest(context.owner, context.repo, context.number, true, loadMode);
 		if (!pr) {
 			return undefined;
 		}
@@ -1102,7 +1102,7 @@ export function registerCommands(
 					repo: argument.pullRequestDetails.repository.name,
 					number: argument.pullRequestDetails.number,
 					preventDefaultContextMenuItems: true,
-				}))?.pr;
+				}, 'overview'))?.pr;
 			} else if (PRChatContextItem.is(argument)) {
 				issueModel = argument.pr;
 			} else if (IssueChatContextItem.is(argument)) {

src/github/externalUriOpener.ts

@@ -9,8 +9,10 @@ import { IssueOverviewPanel } from './issueOverview';
 import { PullRequestOverviewPanel } from './pullRequestOverview';
 import { getGitHubIssueOrPullRequestUriOpenerPriority, openWithDefaultExternalOpener, parseGitHubIssueOrPullRequestUri } from '../common/externalUri';
 import { Disposable } from '../common/lifecycle';
+import Logger from '../common/logger';
 import { OPEN_PULL_LINKS, PR_SETTINGS_NAMESPACE } from '../common/settingKeys';
 import { ITelemetry } from '../common/telemetry';
+import { formatError } from '../common/utils';
 import { EXTENSION_ID } from '../constants';
 
 class GitHubIssueOrPullRequestExternalUriOpener extends Disposable implements vscode.ExternalUriOpener {
@@ -44,21 +46,31 @@ class GitHubIssueOrPullRequestExternalUriOpener extends Disposable implements vs
 
 		const folderRepositoryManager = this._folderRepositoryManagerResolver.getManagerForRepository(identity.owner, identity.repo);
 		if (identity.kind === 'pullRequest') {
-			const pullRequest = await folderRepositoryManager.resolvePullRequest(identity.owner, identity.repo, identity.number, true);
-			if (token.isCancellationRequested) {
-				return;
-			}
-			if (!pullRequest) {
-				await openWithDefaultExternalOpener(openContext.sourceUri);
-				return;
+			const pullRequest = folderRepositoryManager.resolvePullRequest(identity.owner, identity.repo, identity.number, true, 'overview').then(async (pullRequest) => {
+				if (token.isCancellationRequested) {
+					throw new vscode.CancellationError();
+				}
+				if (!pullRequest) {
+					await openWithDefaultExternalOpener(openContext.sourceUri);
+					throw new vscode.CancellationError();
+				}
+				return pullRequest;
+			});
+			// Start the webview while the first repository and PR requests are in flight.
+			try {
+				await PullRequestOverviewPanel.createOrShow(
+					this._telemetry,
+					this._context.extensionUri,
+					folderRepositoryManager,
+					identity,
+					pullRequest,
+				);
+			} catch (error) {
+				if (!(error instanceof vscode.CancellationError)) {
+					Logger.error(`Failed to open pull request: ${formatError(error)}`, 'GitHubIssueOrPullRequestExternalUriOpener');
+					await vscode.window.showErrorMessage(formatError(error));
+				}
 			}
-			await PullRequestOverviewPanel.createOrShow(
-				this._telemetry,
-				this._context.extensionUri,
-				folderRepositoryManager,
-				identity,
-				pullRequest,
-			);
 		} else {
 			const issue = await folderRepositoryManager.resolveIssue(identity.owner, identity.repo, identity.number, true, true);
 			if (token.isCancellationRequested) {

src/github/folderRepositoryManager.ts

@@ -15,6 +15,7 @@ import { CopilotWorkingStatus, GitHubRepository, isRateLimitError, ItemsData, PU
 import { PullRequestState } from './graphql';
 import { IAccount, ILabel, IMilestone, IProject, IPullRequestsPagingOptions, Issue, ITeam, MergeMethod, PRType, PullRequestMergeability, RepoAccessAndMergeMethods, User } from './interface';
 import { IssueModel } from './issueModel';
+import { getErrorCode } from './loggingOctokit';
 import { PullRequestGitHelper, PullRequestMetadata } from './pullRequestGitHelper';
 import { IResolvedPullRequestModel, PullRequestModel } from './pullRequestModel';
 import {
@@ -30,7 +31,7 @@ import {
 import type { Branch, Commit, Repository, UpstreamRef } from '../api/api';
 import { GitApiImpl, GitErrorCodes } from '../api/api1';
 import { GitHubManager } from '../authentication/githubServer';
-import { AuthProvider, GitHubServerType } from '../common/authentication';
+import { AuthProvider, GitHubServerType, isSamlError } from '../common/authentication';
 import { commands, contexts } from '../common/executeCommands';
 import { InMemFileChange, SlimFileChange } from '../common/file';
 import { findLocalRepoRemoteFromGitHubRef } from '../common/githubRef';
@@ -2481,7 +2482,7 @@ export class FolderRepositoryManager extends Disposable {
 
 	//#region Git related APIs
 
-	private async resolveItem(owner: string, repositoryName: string): Promise<GitHubRepository | undefined> {
+	private async resolveItem(owner: string, repositoryName: string, resolveMetadata: boolean = true): Promise<GitHubRepository | undefined> {
 		let githubRepo = this._githubRepositories.find(repo => {
 			const ret =
 				repo.remote.owner.toLowerCase() === owner.toLowerCase() &&
@@ -2492,7 +2493,7 @@ export class FolderRepositoryManager extends Disposable {
 		if (!githubRepo) {
 			Logger.appendLine(`GitHubRepository not found: ${owner}/${repositoryName}`, this.id);
 			// try to create the repository
-			githubRepo = await this.createGitHubRepositoryFromOwnerName(owner, repositoryName);
+			githubRepo = await this.createGitHubRepositoryFromOwnerName(owner, repositoryName, resolveMetadata);
 		}
 		return githubRepo;
 	}
@@ -2510,11 +2511,23 @@ export class FolderRepositoryManager extends Disposable {
 		repositoryName: string,
 		pullRequestNumber: number,
 		useCache: boolean = false,
+		loadMode: 'default' | 'overview' = 'default',
 	): Promise<PullRequestModel | undefined> {
-		const githubRepo = await this.resolveItem(owner, repositoryName);
+		const githubRepo = await this.resolveItem(owner, repositoryName, loadMode !== 'overview');
 		Logger.trace(`Found GitHub repo for pr #${pullRequestNumber}: ${githubRepo ? 'yes' : 'no'}`, this.id);
 		if (githubRepo) {
-			const pr = await githubRepo.getPullRequest(pullRequestNumber, 'FolderRepositoryManager.resolvePullRequest', useCache);
+			const pullRequestPromise = githubRepo.getPullRequest(pullRequestNumber, 'FolderRepositoryManager.resolvePullRequest', useCache, false, loadMode);
+			let pr: PullRequestModel | undefined;
+			if (loadMode === 'overview') {
+				// Both are needed to render the overview, but neither depends on the other.
+				const [accessibleRepository, pullRequest] = await Promise.all([
+					this.validateGitHubRepositoryAccess(githubRepo),
+					pullRequestPromise,
+				]);
+				pr = accessibleRepository ? pullRequest : undefined;
+			} else {
+				pr = await pullRequestPromise;
+			}
 			Logger.trace(`Found GitHub pr repo for pr #${pullRequestNumber}: ${pr ? 'yes' : 'no'}`, this.id);
 			return pr;
 		}
@@ -3067,7 +3080,7 @@ export class FolderRepositoryManager extends Disposable {
 		});
 	}
 
-	async createGitHubRepositoryFromOwnerName(owner: string, repositoryName: string): Promise<GitHubRepository | undefined> {
+	async createGitHubRepositoryFromOwnerName(owner: string, repositoryName: string, resolveMetadata: boolean = true): Promise<GitHubRepository | undefined> {
 		const existing = this.findExistingGitHubRepository({ owner, repositoryName });
 		if (existing) {
 			return existing;
@@ -3080,25 +3093,37 @@ export class FolderRepositoryManager extends Disposable {
 		const gitRemotes = await parseRepositoryRemotesAsync(this.repository);
 		const gitRemote = gitRemotes.find(r => r.owner === owner && r.repositoryName === repositoryName);
 		const uri = gitRemote?.url ?? `https://github.com/${owner}/${repositoryName}`;
-		const repo = await this.createAndAddGitHubRepository(new Remote(gitRemote?.remoteName ?? repositoryName, uri, new Protocol(uri)), this._credentialStore);
+		const repo = await this.createGitHubRepository(new Remote(gitRemote?.remoteName ?? repositoryName, uri, new Protocol(uri)), this._credentialStore, undefined, true);
+		return resolveMetadata ? this.validateGitHubRepositoryAccess(repo) : repo;
+	}
+
+	private async validateGitHubRepositoryAccess(repo: GitHubRepository): Promise<GitHubRepository | undefined> {
+		const { owner, repositoryName } = repo.remote;
 		let reason: string;
 		try {
 			await repo.getMetadata();
 			return repo;
 		} catch (e) {
+			// Only a definitive not-found response should prevent subsequent retries.
+			if (getErrorCode(e) !== '404' || isSamlError(e)) {
+				Logger.warn(`Failed to validate repository ${owner}/${repositoryName}: ${formatError(e)}`, this.id);
+				return undefined;
+			}
 			reason = 'error';
 			Logger.appendLine(`Repository ${owner}/${repositoryName} is not accessible: ${e}`, this.id);
 		}
 		Logger.appendLine(`Repository ${owner}/${repositoryName} is not accessible.`, this.id);
-		this._inaccessibleRepos.add(repoKey);
+		this._inaccessibleRepos.add(`${owner.toLowerCase()}/${repositoryName.toLowerCase()}`);
 		this.removeGitHubRepository(repo.remote);
+		const gitRemotes = await parseRepositoryRemotesAsync(this.repository);
+		const hasLocalRemote = gitRemotes.some(remote => remote.owner === owner && remote.repositoryName === repositoryName);
 		/* __GDPR__
 			"repository.inaccessible" : {
 				"hasLocalRemote" : { "classification": "SystemMetaData", "purpose": "FeatureInsight" },
 				"reason" : { "classification": "SystemMetaData", "purpose": "FeatureInsight" }
 			}
 		*/
-		this.telemetry.sendTelemetryEvent('repository.inaccessible', { hasLocalRemote: (!!gitRemote).toString(), reason });
+		this.telemetry.sendTelemetryEvent('repository.inaccessible', { hasLocalRemote: hasLocalRemote.toString(), reason });
 		return undefined;
 	}
 

src/github/githubRepository.ts

@@ -67,6 +67,7 @@ import {
 	convertRESTPullRequestToRawPullRequest,
 	getAvatarWithEnterpriseFallback,
 	getOverrideBranch,
+	GraphQLAccount,
 	isInCodespaces,
 	parseAccount,
 	parseGraphQLIssue,
@@ -77,6 +78,7 @@ import {
 	parseMilestone,
 	restPaginate,
 } from './utils';
+import { PullRequestPreview } from './views';
 import { StackCandidate } from '../../common/views';
 import { AuthenticationError, AuthProvider, GitHubServerType, isSamlError } from '../common/authentication';
 
@@ -1403,13 +1405,44 @@ export class GitHubRepository extends Disposable {
 		}
 	}
 
-	async getPullRequest(id: number, callerName: string, useCache: boolean = false, silent: boolean = false): Promise<PullRequestModel | undefined> {
+	async getPullRequestPreview(number: number): Promise<PullRequestPreview> {
+		if (!Number.isSafeInteger(number) || number <= 0) {
+			throw new Error(`Invalid pull request number: ${number}`);
+		}
+		const { query, remote, schema } = await this.ensure();
+		type PreviewData = Omit<PullRequestPreview, 'author' | 'base' | 'head'> & {
+			author: GraphQLAccount | null;
+			baseRefName: string;
+			headRefName: string;
+			baseRepository: { owner: { login: string } };
+			headRepository: { owner: { login: string } } | null;
+		};
+		const { data } = await query<{ repository: { pullRequest: PreviewData | null } | null }>({
+			query: schema.PullRequestPreview,
+			variables: { owner: remote.owner, name: remote.repositoryName, number },
+		});
+		if (!data.repository?.pullRequest) {
+			throw new Error(`Unable to load pull request preview for ${remote.owner}/${remote.repositoryName}#${number}`);
+		}
+		// A preview must never populate the shared cache of actionable PR models.
+		const { author, baseRefName, headRefName, baseRepository, headRepository, ...preview } = data.repository.pullRequest;
+		return {
+			...preview,
+			author: parseAccount(author, this),
+			base: `${baseRepository.owner.login}/${remote.repositoryName}:${baseRefName}`,
+			head: headRepository ? `${headRepository.owner.login}/${remote.repositoryName}:${headRefName}` : '',
+		};
+	}
+
+	async getPullRequest(id: number, callerName: string, useCache: boolean = false, silent: boolean = false, loadMode: 'default' | 'overview' = 'default'): Promise<PullRequestModel | undefined> {
 		if (useCache && this._pullRequestModelsByNumber.has(id)) {
 			Logger.debug(`Using cached pull request model for ${id}`, this.id);
 			return this._pullRequestModelsByNumber.get(id)!.model;
 		}
 
-		if (!(await this.isPlausibleItemNumber(id))) {
+		// Explicit overview requests already identify a PR; the max-number lookup is
+		// only useful for speculative references extracted from text.
+		if (!Number.isSafeInteger(id) || id <= 0 || (loadMode === 'default' && !(await this.isPlausibleItemNumber(id)))) {
 			Logger.debug(`Skipping pull request fetch for implausible number ${id} (caller: ${callerName})`, this.id);
 			return;
 		}
@@ -1433,7 +1466,9 @@ export class GitHubRepository extends Disposable {
 
 			Logger.debug(`Fetch pull request ${id} - done`, this.id);
 			const pr = this.createOrUpdatePullRequestModel(await parseGraphQLPullRequest(data.repository.pullRequest, this), silent);
-			await pr.getLastUpdateTime(new Date(pr.item.updatedAt));
+			if (loadMode === 'default') {
+				await pr.getLastUpdateTime(new Date(pr.item.updatedAt));
+			}
 			let repoIds = GitHubRepository._succeededPullRequests.get(id);
 			if (!repoIds) {
 				repoIds = new Set();