Resolve and configure Source windows for remote hosts

.github/workflows/review-desktop-ci.yml

@@ -190,10 +190,13 @@ jobs:
           pnpm -r --filter '!@dev.fast/review-desktop' --filter '!./diffr/**' --workspace-concurrency=1 --if-present typecheck
 
       # Typecheck prepared the shared tutorial assets before concurrent pretests.
+      # Desktop's tests build a remote tarball, which starves concurrent browser
+      # tests of CPU until Chromium drops their module fetches; run them last.
       - name: Test
         run: |
           node --test "scripts/**/*.test.mjs"
-          pnpm -r --filter '!./diffr/**' --no-sort --workspace-concurrency=3 --if-present test
+          pnpm -r --filter '!./diffr/**' --filter '!@dev.fast/review-desktop' --no-sort --workspace-concurrency=3 --if-present test
+          pnpm --filter @dev.fast/review-desktop test
 
       - name: Upload browser test diagnostics
         if: failure()

apps/review-desktop/UPSTREAM

@@ -372,6 +372,10 @@ one of these entries:
   fatal either: it ends that connection only. Isolation is unconditional, so a
   remote Source window gets no upstream reload dialog; it reconnects through
   the fork's resolver and shows its host state in the status bar.
+- `src/vs/platform/window/common/window.ts` and
+  `src/vs/platform/native/electron-main/nativeHostMainService.ts`: the
+  `openWindow` option `reviewReferencesToShow` runs Find References in the
+  opened window.
 - `extensions/package.json` and the extension `package.json`/`package-lock.json`
   files: `@vscode/extension-telemetry` is hoisted to the shared extensions
   install (github keeps its own ^1.0.0 copy; the shared copy is ^0.9.8). The

apps/review-desktop/code-oss/src/vs/review/electron-main/remote/reviewRemoteHosts.test.ts

@@ -12,7 +12,7 @@ import test from "node:test";
 import type { ReviewGatewayHost } from "../../common/reviewProtocol.js";
 import { attachOutput, COMMIT, detectOutput, FAKE_SERVER_ID, fakeClock, fakeSsh, flush, healthServer, until, type FakeRemote } from "./test/fakeSsh.js";
 import type { ReviewRemoteInstallFlow } from "./reviewRemoteHost.js";
-import { ReviewRemoteHosts } from "./reviewRemoteHosts.js";
+import { closeRemoteHostWindows, ReviewRemoteHosts, type ReviewRemoteHostsOptions } from "./reviewRemoteHosts.js";
 import { openRemoteInstallConsent } from "./reviewRemoteInstallConsent.js";
 import type { SshPromptRequest } from "./reviewSshAskpass.js";
 import { reviewSshInstancePrefix } from "./reviewSshCommand.js";
@@ -23,6 +23,7 @@ async function managerFor(
 	answer?: string,
 	before?: (directory: string) => Promise<void>,
 	install?: ReviewRemoteInstallFlow,
+	removed?: ReviewRemoteHostsOptions["removed"],
 ) {
 	const port = await healthServer(t);
 	const directory = await mkdtemp(join(tmpdir(), "wb-hosts-"));
@@ -48,6 +49,7 @@ async function managerFor(
 		clock,
 		timeouts: { poll: 1 },
 		install,
+		removed,
 	});
 	t.after(async () => {
 		await manager.dispose();
@@ -98,6 +100,39 @@ test("removing a host from the setting closes its connection", async (t) => {
 	assert.ok(ssh.master("wb-test-b")!.alive);
 });
 
+test("removing a machine's last alias closes the windows bound to it, and no other window", async (t) => {
+	const closed: string[] = [];
+	const windows = [`whiteboard+${FAKE_SERVER_ID}`, "whiteboard+wb-test-b-server", "ssh-remote+x", undefined].map((remoteAuthority) => ({
+		remoteAuthority,
+		close: () => closed.push(String(remoteAuthority)),
+	}));
+	const b = { attach: { code: 0, stdout: attachOutput(41234, "remote-token", { serverId: "wb-test-b-server" }) } };
+	const { manager, sentUntil } = await managerFor(t, { "wb-test-a": {}, "wb-test-a2": {}, "wb-test-b": b }, undefined, undefined, undefined, (serverId) =>
+		closeRemoteHostWindows({ getWindows: () => windows }, serverId),
+	);
+
+	manager.update(true, ["wb-test-a", "wb-test-a2", "wb-test-b"]);
+	await sentUntil((hosts) => hosts.length === 3 && hosts.every((h) => h.endpoint));
+	manager.update(true, ["wb-test-a", "wb-test-b"]);
+	assert.deepEqual(closed, []);
+	manager.update(true, ["wb-test-b"]);
+	assert.deepEqual(closed, [`whiteboard+${FAKE_SERVER_ID}`]);
+});
+
+test("a machine whose entry lost its id while unreachable is known through its alias until that alias leaves the setting", async (t) => {
+	const { manager, sentUntil } = await managerFor(t, { "wb-test-a": {} });
+	manager.update(true, ["wb-test-a"]);
+	await sentUntil((hosts) => byAlias(hosts, "wb-test-a")?.endpoint !== undefined);
+
+	const unreachable = [{ alias: "wb-test-a", state: "unreachable" as const }];
+	assert.deepEqual(manager.hostState(FAKE_SERVER_ID, unreachable), { alias: "wb-test-a", state: "unreachable" });
+	assert.deepEqual(manager.hostState(FAKE_SERVER_ID, []), { alias: "wb-test-a", state: "connecting" });
+	assert.equal(manager.hostState("other-machine", unreachable), undefined);
+
+	manager.update(true, []);
+	assert.equal(manager.hostState(FAKE_SERVER_ID, unreachable), undefined);
+});
+
 test("an alias added again while its old host closes starts only after the old master exited", async (t) => {
 	const { manager, ssh, sentUntil } = await managerFor(t, { "wb-test-a": { exitDelayMs: 150 } });
 

apps/review-desktop/code-oss/src/vs/review/electron-main/remote/reviewRemoteHosts.ts

@@ -18,6 +18,7 @@ import {
 	type SpawnSsh,
 } from "./reviewRemoteHost.js";
 import type { ReviewSshAskpass, SshPromptRequest } from "./reviewSshAskpass.js";
+import { reviewRemoteAuthority } from "../../services/remote/reviewRemoteAuthority.js";
 import { uninstallRemote } from "./reviewRemoteUninstall.js";
 import {
 	prepareSshControlDirectory,
@@ -48,6 +49,26 @@ export interface ReviewRemoteHostsOptions {
 	readonly clock?: ReviewRemoteClock;
 	readonly timeouts?: ReviewRemoteHostOptions["timeouts"];
 	readonly install?: ReviewRemoteInstallFlow;
+	/** No host in the setting serves this machine any more. */
+	removed?(serverId: string): void;
+}
+
+/** The gateway's state of the machine `serverId`; `configured` is the alias that served it. */
+export function remoteHostState(
+	serverId: string,
+	states: readonly ReviewGatewayHostState[],
+	configured?: string,
+): ({ alias?: string } & Pick<ReviewGatewayHostState, "state" | "detail">) | undefined {
+	const own = states.filter((state) => state.serverId === serverId);
+	const best = own.find((state) => state.state === "online") ?? own.find((state) => state.state === "connecting") ?? own[0];
+	if (best) return best;
+	if (configured) return states.find((state) => state.alias === configured) ?? { alias: configured, state: "connecting" };
+	return states.some((state) => state.serverId === undefined && state.state === "connecting") ? { state: "connecting" } : undefined;
+}
+
+export function closeRemoteHostWindows(windows: { getWindows(): readonly { readonly remoteAuthority?: string; close(): void }[] }, serverId: string): void {
+	const authority = reviewRemoteAuthority(serverId);
+	if (authority) for (const window of windows.getWindows()) if (window.remoteAuthority === authority) window.close();
 }
 
 export function freeLoopbackPort(): Promise<number> {
@@ -110,6 +131,7 @@ export class ReviewRemoteHosts {
 			this.hosts.delete(alias);
 			this.options.log(`${alias}: removed from the setting; closing its connection.`);
 			this.closing.set(host, host.dispose().finally(() => this.closing.delete(host)));
+			this.forget(host);
 		}
 		this.order = wanted;
 		this.refused.clear();
@@ -183,6 +205,7 @@ export class ReviewRemoteHosts {
 		if (host && this.hosts.get(alias) === host) {
 			this.hosts.delete(alias);
 			this.removed.add(alias);
+			this.forget(host);
 			this.publish();
 		}
 		if (!host) return;
@@ -203,6 +226,12 @@ export class ReviewRemoteHosts {
 		return online && this.hosts.get(online.alias)?.languageEndpoint(serverId);
 	}
 
+	/** The state of the machine `serverId`, through the alias in the setting that served it this session. */
+	hostState(serverId: string, states: readonly ReviewGatewayHostState[]): ReturnType<typeof remoteHostState> {
+		const configured = [...this.hosts.values()].find((host) => host.serverId === serverId)?.state.alias;
+		return remoteHostState(serverId, states, configured);
+	}
+
 	resume(): void {
 		for (const host of this.hosts.values()) void host.resume();
 	}
@@ -239,6 +268,11 @@ export class ReviewRemoteHosts {
 		});
 	}
 
+	private forget(host: ReviewRemoteHost): void {
+		const { serverId } = host;
+		if (serverId && ![...this.hosts.values()].some((other) => other.serverId === serverId)) this.options.removed?.(serverId);
+	}
+
 	private askpass(): Promise<ReviewSshAskpass> {
 		return (this.prepared ??= (async () => {
 			await prepareSshControlDirectory(this.options.controlDirectory);

apps/review-desktop/code-oss/src/vs/review/electron-main/reviewDesktopChannel.ts

@@ -57,6 +57,10 @@ export class ReviewDesktopChannel implements IServerChannel {
       if (typeof arg !== "string") return undefined as T;
       return (await this.host.getRemoteLanguageEndpoint(arg)) as T;
     }
+    if (command === "getRemoteHostState") {
+      if (typeof arg !== "string") return undefined as T;
+      return (await this.host.getRemoteHostState(arg)) as T;
+    }
     if (command === "retryRemoteHost") {
       if (typeof arg === "string") this.host.retryRemoteHost(arg);
       return undefined as T;

apps/review-desktop/code-oss/src/vs/review/electron-main/reviewDesktopHost.ts

@@ -9,6 +9,7 @@ import { Disposable, toDisposable } from "../../base/common/lifecycle.js";
 import { join } from "../../base/common/path.js";
 import { IConfigurationService } from "../../platform/configuration/common/configuration.js";
 import { IEnvironmentMainService } from "../../platform/environment/electron-main/environmentMainService.js";
+import { IInstantiationService } from "../../platform/instantiation/common/instantiation.js";
 import {
   ILifecycleMainService,
   LifecycleMainPhase,
@@ -20,6 +21,7 @@ import { IApplicationStorageMainService } from "../../platform/storage/electron-
 import { NullTelemetryService } from "../../platform/telemetry/common/telemetryUtils.js";
 import { IUpdateService } from "../../platform/update/common/update.js";
 import { UtilityProcess } from "../../platform/utilityProcess/electron-main/utilityProcess.js";
+import { IWindowsMainService } from "../../platform/windows/electron-main/windows.js";
 import type { ReviewDesktopConnection } from "../common/reviewDesktopBootstrap.js";
 import type {
   ReviewGatewayHostState,
@@ -52,7 +54,7 @@ import type {
   ReviewRemoteInstallFlow,
   ReviewRemoteInstallMode,
 } from "./remote/reviewRemoteHost.js";
-import { ReviewRemoteHosts } from "./remote/reviewRemoteHosts.js";
+import { closeRemoteHostWindows, ReviewRemoteHosts } from "./remote/reviewRemoteHosts.js";
 import {
   openRemoteInstallConsent,
   reviewRemoteInstallConsentPath,
@@ -100,6 +102,8 @@ export class ReviewDesktopHost extends Disposable {
     @IUpdateService private readonly updateService: IUpdateService,
     @IApplicationStorageMainService
     private readonly applicationStorageMainService: IApplicationStorageMainService,
+    @IInstantiationService
+    private readonly instantiationService: IInstantiationService,
   ) {
     super();
     let resolvedEnvironment: Promise<NodeJS.ProcessEnv> | undefined;
@@ -255,17 +259,23 @@ export class ReviewDesktopHost extends Disposable {
 
   async getRemoteLanguageEndpoint(serverId: string) {
     const manager = this.remoteHosts;
-    if (!manager) return undefined;
+    const states = manager && (await this.remoteHostStates());
+    return states && manager.languageEndpoint(serverId, states).catch(() => undefined);
+  }
+
+  async getRemoteHostState(serverId: string) {
+    const manager = this.remoteHosts;
+    const states = manager && (await this.remoteHostStates());
+    return states && manager.hostState(serverId, states);
+  }
+
+  private async remoteHostStates(): Promise<ReviewGatewayHostState[] | undefined> {
     try {
       const { url, token } = await this.whenConnected();
       const response = await fetch(new URL("/remote-hosts", url), {
         headers: { "x-review-token": token },
       });
-      if (!response.ok) return undefined;
-      return await manager.languageEndpoint(
-        serverId,
-        (await response.json()) as ReviewGatewayHostState[],
-      );
+      return response.ok ? ((await response.json()) as ReviewGatewayHostState[]) : undefined;
     } catch {
       return undefined;
     }
@@ -354,6 +364,10 @@ export class ReviewDesktopHost extends Disposable {
       send: (hosts) => this.supervisor.setRemoteHosts(hosts),
       log: (message) => this.logService.info(`[Remote hosts] ${message}`),
       install: this.remoteInstallFlow(),
+      removed: (serverId) =>
+        this.instantiationService.invokeFunction((accessor) =>
+          closeRemoteHostWindows(accessor.get(IWindowsMainService), serverId),
+        ),
     });
     this.remoteHosts = manager;
     const update = () => {

apps/review-desktop/code-oss/src/vs/review/navigator.desktop.main.ts

@@ -47,6 +47,19 @@ import { ILanguageFeaturesService } from '../editor/common/services/languageFeat
 import { SymbolNavigationAnchor } from '../editor/contrib/gotoSymbol/browser/goToCommands.js';
 import { CommandsRegistry, ICommandService } from '../platform/commands/common/commands.js';
 import { IEditorService } from '../workbench/services/editor/common/editorService.js';
+import { NativeExtensionService } from '../workbench/services/extensions/electron-browser/nativeExtensionService.js';
+import { IExtensionService } from '../workbench/services/extensions/common/extensions.js';
+import { IRemoteAuthorityResolverService, type ResolverResult } from '../platform/remote/common/remoteAuthorityResolver.js';
+import { IMainProcessService } from '../platform/ipc/common/mainProcessService.js';
+import { REVIEW_DESKTOP_CHANNEL } from './common/reviewDesktopBootstrap.js';
+import { isReviewRemoteAuthority, reviewRemoteServerId } from './services/remote/reviewRemoteAuthority.js';
+import { reloadWhenOnline, reviewWindowAuthorityResolver, reviewWindowHosts } from './services/remote/reviewWindowAuthorityResolver.js';
+import { IHostService } from '../workbench/services/host/browser/host.js';
+import { FilesConfigurationService, IFilesConfigurationService } from '../workbench/services/filesConfiguration/common/filesConfigurationService.js';
+import type { IBaseFileStat } from '../platform/files/common/files.js';
+import { isEqual } from '../base/common/resources.js';
+import type { IMarkdownString } from '../base/common/htmlContent.js';
+import type { IWorkspaceContextService } from '../platform/workspace/common/workspace.js';
 
 class NavigatorDefaults {
 	constructor(@IStorageService storage: IStorageService) {
@@ -60,6 +73,45 @@ class NavigatorDefaults {
 
 registerWorkbenchContribution2('review.navigator.defaults', NavigatorDefaults, WorkbenchPhase.BlockStartup);
 
+class NavigatorExtensionService extends NativeExtensionService {
+	private readonly windowHosts = this._instantiationService.invokeFunction((accessor) => reviewWindowHosts(accessor.get(IMainProcessService).getChannel(REVIEW_DESKTOP_CHANNEL)));
+	protected override readonly _remoteAuthorityResolverService = this._instantiationService.invokeFunction((accessor) =>
+		reviewWindowAuthorityResolver(accessor.get(IRemoteAuthorityResolverService), this.windowHosts));
+	private retrying = false;
+
+	protected override async _resolveAuthority(remoteAuthority: string): Promise<ResolverResult> {
+		if (!isReviewRemoteAuthority(remoteAuthority)) return super._resolveAuthority(remoteAuthority);
+		try {
+			return await this._remoteAuthorityResolverService.resolveAuthority(remoteAuthority);
+		} catch (error) {
+			if (!this.retrying) {
+				this.retrying = true;
+				const host = this._instantiationService.invokeFunction((accessor) => accessor.get(IHostService));
+				this._register(reloadWhenOnline(this.windowHosts, reviewRemoteServerId(remoteAuthority), () => void host.reload()));
+			}
+			throw error;
+		}
+	}
+}
+
+registerSingleton(IExtensionService, NavigatorExtensionService, InstantiationType.Eager);
+
+/** Every file in a Source window's folders is read-only, whatever a folder's settings say. */
+class NavigatorFilesConfigurationService extends FilesConfigurationService {
+	private readonly workspace: IWorkspaceContextService;
+
+	constructor(...args: ConstructorParameters<typeof FilesConfigurationService>) {
+		super(...args);
+		this.workspace = args[2];
+	}
+
+	override isReadonly(resource: URI, stat?: IBaseFileStat): boolean | IMarkdownString {
+		return super.isReadonly(resource, stat) || (!!this.workspace.getWorkspaceFolder(resource) && !isEqual(resource, this.workspace.getWorkspace().configuration ?? undefined));
+	}
+}
+
+registerSingleton(IFilesConfigurationService, NavigatorFilesConfigurationService, InstantiationType.Eager);
+
 CommandsRegistry.registerCommand('review.action.showReferencesInSource', async (accessor, resource: string, lineNumber: number, column: number) => {
 	const editorService = accessor.get(IEditorService);
 	const references = accessor.get(ILanguageFeaturesService).referenceProvider;

apps/review-desktop/code-oss/src/vs/review/services/remote/reviewRemoteAuthority.ts

@@ -6,14 +6,26 @@
 import { Schemas } from "../../../base/common/network.js";
 import type { URI } from "../../../base/common/uri.js";
 
+const PREFIX = "whiteboard+";
+
 export function reviewRemoteAuthority(serverId: string): string | undefined {
-	return /^[0-9a-z-]+$/i.test(serverId) ? `whiteboard+${serverId.toLowerCase()}` : undefined;
+	return /^[0-9a-z-]+$/i.test(serverId) ? `${PREFIX}${serverId.toLowerCase()}` : undefined;
+}
+
+/** The server id in a `reviewRemoteAuthority`. */
+export function reviewRemoteServerId(authority: string): string {
+	return authority.slice(PREFIX.length);
 }
 
 export function ownsRemoteResource(authority: string, resource: URI): boolean {
 	return resource.scheme === Schemas.vscodeRemote && resource.authority.toLowerCase() === authority;
 }
 
+/** Exactly the lower-case `whiteboard+<serverId>` that `reviewRemoteAuthority` makes. */
+export function isReviewRemoteAuthority(authority: string | null | undefined): authority is string {
+	return !!authority && /^whiteboard\+[0-9a-z-]+$/.test(authority);
+}
+
 export function override<T extends object>(base: T, members: Partial<T>): T {
 	return new Proxy(base, {
 		get(target, key) {