Show a remote host's state and next step on its reviews

apps/review-desktop/code-oss/src/vs/review/browser/parts/canvas/reviewCanvasPart.ts

@@ -71,6 +71,7 @@ import type {
 	ReviewDocumentWidthChoice,
 	ReviewKeymapChoice,
 	ReviewReadyNotificationChoice,
+	ReviewRemoteHostActions,
 	ReviewRuntimeConfig,
 	ReviewSurfaceEvent,
 	ReviewTheme,
@@ -459,6 +460,7 @@ export class ReviewCanvasEditorPane extends EditorPane {
 						kind: "api",
 						reviewId,
 						...remoteEntry(this.apiCatalog.reviews.find((review) => review.reviewId === reviewId)),
+						remoteHosts: this.remoteHostActions(),
 						structuralDiffEnabled: this.currentStructuralDiffEnabled(),
 						softwareMapEnabled: this.currentSoftwareMapEnabled(),
 						documentWidth: this.currentDocumentWidth(),
@@ -574,7 +576,7 @@ export class ReviewCanvasEditorPane extends EditorPane {
 							this.reviewTelemetryService.capture("review_restored", { via: "home" });
 							return this.apiCatalog.lifecycle(uuid, "active");
 						},
-						hostStates: () => this.desktopConnection.readRemoteHosts(),
+						remoteHosts: this.remoteHostActions(),
 						openSourceTree: (uuid) => {
 							const api = this.apiCatalog.reviews.find((review) => review.reviewId === uuid);
 							if (api) {
@@ -642,6 +644,17 @@ export class ReviewCanvasEditorPane extends EditorPane {
 		}
 	}
 
+	private remoteHostActions(): ReviewRemoteHostActions {
+		return {
+			states: () => this.desktopConnection.readRemoteHosts(),
+			retry: (alias) => this.desktopConnection.retryRemoteHost(alias),
+			install: (alias) => this.desktopConnection.installRemoteHost(alias),
+			openSettings: async () => {
+				await this.tabsService.openSettings(true);
+			},
+		};
+	}
+
 	async refreshComparison(): Promise<void> {
 		if (!(this.input instanceof ReviewCanvasEditorInput) || this.input.target.kind !== "api") return;
 		this.readyInput = undefined;

apps/review-desktop/scripts/e2e/TESTING.md

@@ -123,7 +123,7 @@ key, and the journey adds `wb-test-a` in Settings as a user would. It checks,
 on the DOM and on the page's requests: the host goes `online`; Home lists the
 container's review as `wb-test-a: wbrepo`; the document, a code peek, the Diff
 view and the structural diff load; an edit and a `session_create` on the
-remote reach the window; a killed `ssh` master shows "Connection lost" and
+remote reach the window; a killed `ssh` master shows a chip in the top bar and
 recovers without a reload; a stopped server is `offline` within 15 s while a
 laptop review still opens; another package version is `incompatible` with the
 install command in Settings; and removing the host takes its reviews out of

apps/review-desktop/scripts/e2e/journeys/remote-host.mjs

@@ -454,17 +454,13 @@ async function journey(ctx, page, until) {
   await tab(title).click();
   await canvas.getByRole("heading", { name: title }).waitFor();
   await page.evaluate(() => {
-    const seen = (window.__remoteHostBanner = []);
+    const seen = (window.__remoteHostChip = []);
 
     new MutationObserver(() => {
-      for (const status of document.querySelectorAll(
-        ".review-canvas-root [role=status]",
+      for (const chip of document.querySelectorAll(
+        ".review-canvas-root .connection-chip",
       ))
-        if (
-          status.textContent.startsWith("Connection lost") &&
-          !seen.includes(status.textContent)
-        )
-          seen.push(status.textContent);
+        if (!seen.includes(chip.textContent)) seen.push(chip.textContent);
     }).observe(document.body, {
       subtree: true,
       childList: true,
@@ -477,8 +473,8 @@ async function journey(ctx, page, until) {
   assert.ok(master, "no ssh master for the host");
   process.kill(master, "SIGKILL");
   await until(
-    () => page.evaluate(() => window.__remoteHostBanner.length > 0),
-    "the Connection lost banner",
+    () => page.evaluate(() => window.__remoteHostChip.length > 0),
+    "the disconnected chip",
     30000,
   );
   await until(
@@ -488,12 +484,8 @@ async function journey(ctx, page, until) {
     60000,
   );
   await until(
-    async () =>
-      (await canvas
-        .locator("[role=status]")
-        .filter({ hasText: "Connection lost" })
-        .count()) === 0,
-    "the banner to clear",
+    async () => (await canvas.locator(".connection-chip").count()) === 0,
+    "the chip to clear",
   );
   await remoteApi("session_edit", {
     sessionId: reviewId,
@@ -505,7 +497,7 @@ async function journey(ctx, page, until) {
   await canvas.getByText("After the reconnect.").waitFor({ timeout: 30000 });
   assert.equal(await page.evaluate(() => performance.timeOrigin), timeOrigin);
   ctx.check(
-    `6. a killed ssh master showed "${await page.evaluate(() => window.__remoteHostBanner[0])}" and the review recovered in the same page`,
+    `6. a killed ssh master showed "${await page.evaluate(() => window.__remoteHostChip[0])}" in the top bar and the review recovered in the same page`,
   );
 
   // 7. A hung server: offline within 15 s, and a laptop review still opens at once.
@@ -546,8 +538,11 @@ async function journey(ctx, page, until) {
 
     section = settings.getByRole("region", { name: "Remote hosts" });
     await until(
-      async () => /incompatible/.test(await hostRow().innerText()),
-      "the Settings row to read incompatible",
+      async () =>
+        /runs Whiteboard .+; this Desktop runs/.test(
+          await hostRow().innerText(),
+        ),
+      "the Settings row to name both versions",
     );
     assert.match(
       await hostRow().locator("code").innerText(),
@@ -558,7 +553,7 @@ async function journey(ctx, page, until) {
     for (const text of [title, second]) {
       await row(text).waitFor();
       assert.equal(await row(text).getAttribute("data-unavailable"), "");
-      assert.match(await row(text).innerText(), /incompatible/);
+      assert.match(await row(text).innerText(), /needs an update/);
     }
 
     ctx.check(

apps/review-desktop/scripts/e2e/journeys/remote-install.mjs

@@ -511,17 +511,14 @@ async function steps(ctx, until, watch, timings, manifestPath) {
   assert.doesNotMatch(first.text, /\/home\//, "the question names a path");
 
   const settingsSteps = (await pageRecord(ctx)).rows[fresh].filter((text) =>
-    text.startsWith("installing · "),
+    STEP_ORDER.some((step) => text.startsWith(step)),
   );
 
   assert.ok(
     settingsSteps.length >= 2,
     `Settings showed ${settingsSteps.length} install step(s): ${settingsSteps}`,
   );
-  assertStepOrder(
-    settingsSteps.map((text) => text.slice("installing · ".length)),
-    "Settings",
-  );
+  assertStepOrder(settingsSteps, "Settings");
   await onRemote(
     fresh,
     `test -x ${ROOT}/versions/${version}/node_modules/@dev.fast/diffr-linux-x64/diffr`,
@@ -563,7 +560,7 @@ async function steps(ctx, until, watch, timings, manifestPath) {
   );
   await until(
     async () =>
-      /unsupported · .*glibc 2\.31/.test(
+      /glibc 2\.31; Whiteboard needs 2\.34/.test(
         await hostRow(oldSection, old).innerText(),
       ),
     "the Settings row to read unsupported",

packages/review-protocol/src/contracts.ts

@@ -598,6 +598,14 @@ export interface ReviewCanvasSettingsContent {
   remoteHosts: ReviewRemoteHostsSettings;
 }
 
+/** What a review or Home can do about the host a review lives on. */
+export interface ReviewRemoteHostActions {
+  states(): Promise<ReviewGatewayHostState[]>;
+  retry(alias: string): Promise<void>;
+  install(alias: string): Promise<void>;
+  openSettings(): Promise<void>;
+}
+
 export interface ReviewRemoteHostsSettings {
   enabled: boolean;
   configured: string[];
@@ -764,6 +772,7 @@ export type ReviewCanvasContent =
       version?: number;
       host?: string;
       available?: ReviewApiSummary["available"];
+      remoteHosts?: ReviewRemoteHostActions;
       bridge: ReviewCanvasBridge;
       setTitle?(title: string): void;
       setSourceView?(
@@ -798,7 +807,7 @@ export type ReviewCanvasContent =
       // Boards in Trash can be restored for 30 days or deleted.
       deleteReview?(uuid: string): Promise<void>;
       restoreReview?(uuid: string): Promise<void>;
-      hostStates?(): Promise<ReviewGatewayHostState[]>;
+      remoteHosts?: ReviewRemoteHostActions;
       // Opens the review and pins its read-only source tree open. Absent when
       // the host cannot show the tree.
       openSourceTree?(uuid: string): void;

packages/review-protocol/src/index.ts

@@ -56,6 +56,8 @@ export * from "./contracts.js";
 
 export * from "./review-api-client.js";
 
+export * from "./review-host-status.js";
+
 export {
   type ByCommitEntry,
   type ReviewAgentTraceEvent,

packages/review-protocol/src/review-api-client.ts

@@ -206,11 +206,7 @@ export class ReviewApiClient {
     if (!connections)
       liveConnections.set(this.request, (connections = new Map()));
 
-    const key = JSON.stringify([
-      this.connection.serverUrl,
-      this.connection.token,
-    ]);
-
+    const key = this.liveKey();
     let live = connections.get(key);
 
     if (!live) {
@@ -230,6 +226,13 @@ export class ReviewApiClient {
       disconnected,
     );
   }
+  /** Reopen the shared stream now rather than after its backoff. */
+  reconnect() {
+    liveConnections.get(this.request)?.get(this.liveKey())?.reconnect();
+  }
+  private liveKey() {
+    return JSON.stringify([this.connection.serverUrl, this.connection.token]);
+  }
 }
 
 type Result =
@@ -342,7 +345,7 @@ class LiveConnection {
     }, restartDelayMs);
   }
 
-  private reconnect() {
+  reconnect() {
     this.abort.abort();
     this.abort = new AbortController();
     const listeners = [...this.listeners];

packages/review-protocol/src/review-host-status.ts

@@ -0,0 +1,76 @@
+import type { ReviewGatewayHostState } from "./contracts.js";
+
+export interface ReviewHostStatus {
+  /** Short words for a chip or a list row, without the host name. */
+  status: string;
+  /** The same with the host name, for a heading. */
+  label: string;
+  sentence: string;
+  /** What helps: none while the host is busy or online. */
+  action?: "retry" | "install" | "settings";
+  busy?: true;
+}
+
+type Wording = Pick<ReviewHostStatus, "action" | "busy"> & {
+  status: string;
+  label(alias: string): string;
+};
+
+const OFFLINE: Wording = {
+  status: "offline",
+  label: (alias) => `${alias} offline`,
+  action: "retry",
+};
+
+const UNUSABLE: Wording = {
+  status: "can't be used",
+  label: (alias) => `${alias} can't be used`,
+  action: "settings",
+};
+
+const STATUS: Record<ReviewGatewayHostState["state"], Wording> = {
+  online: { status: "online", label: (alias) => alias },
+  connecting: {
+    status: "connecting",
+    label: (alias) => `Connecting to ${alias}…`,
+    busy: true,
+  },
+  installing: {
+    status: "installing",
+    label: (alias) => `Installing on ${alias}…`,
+    busy: true,
+  },
+  offline: OFFLINE,
+  unreachable: OFFLINE,
+  incompatible: {
+    status: "needs an update",
+    label: (alias) => `${alias} needs an update`,
+    action: "install",
+  },
+  "not-installed": {
+    status: "not installed",
+    label: (alias) => `Whiteboard not installed on ${alias}`,
+    action: "install",
+  },
+  "auth-failed": {
+    status: "can't sign in",
+    label: (alias) => `Can't sign in to ${alias}`,
+    action: "settings",
+  },
+  unsupported: UNUSABLE,
+  duplicate: UNUSABLE,
+};
+
+/** One wording and one next step per host state, shared by every surface. */
+export function reviewHostStatus(
+  host: Pick<ReviewGatewayHostState, "alias" | "state" | "detail">,
+): ReviewHostStatus {
+  const { label, ...rest } = STATUS[host.state];
+  const words = label(host.alias);
+
+  return {
+    ...rest,
+    label: words,
+    sentence: host.detail ?? (words.endsWith("…") ? words : `${words}.`),
+  };
+}