diff --git a/apps/review-desktop/code-oss/src/vs/review/browser/parts/canvas/reviewCanvasPart.ts b/apps/review-desktop/code-oss/src/vs/review/browser/parts/canvas/reviewCanvasPart.ts index 7739d1350..de5aba0ec 100644 --- a/apps/review-desktop/code-oss/src/vs/review/browser/parts/canvas/reviewCanvasPart.ts +++ b/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 { if (!(this.input instanceof ReviewCanvasEditorInput) || this.input.target.kind !== "api") return; this.readyInput = undefined; diff --git a/apps/review-desktop/scripts/e2e/TESTING.md b/apps/review-desktop/scripts/e2e/TESTING.md index fbb75eeba..7a7be589b 100644 --- a/apps/review-desktop/scripts/e2e/TESTING.md +++ b/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 diff --git a/apps/review-desktop/scripts/e2e/journeys/remote-host.mjs b/apps/review-desktop/scripts/e2e/journeys/remote-host.mjs index c7530f55a..7f0c1fc92 100644 --- a/apps/review-desktop/scripts/e2e/journeys/remote-host.mjs +++ b/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( diff --git a/apps/review-desktop/scripts/e2e/journeys/remote-install.mjs b/apps/review-desktop/scripts/e2e/journeys/remote-install.mjs index 52d339074..a381a2fba 100644 --- a/apps/review-desktop/scripts/e2e/journeys/remote-install.mjs +++ b/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", diff --git a/packages/review-protocol/src/contracts.ts b/packages/review-protocol/src/contracts.ts index 7f660e289..d58b7ea64 100644 --- a/packages/review-protocol/src/contracts.ts +++ b/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; + retry(alias: string): Promise; + install(alias: string): Promise; + openSettings(): Promise; +} + 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; restoreReview?(uuid: string): Promise; - hostStates?(): Promise; + 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; diff --git a/packages/review-protocol/src/index.ts b/packages/review-protocol/src/index.ts index 0d171294f..d720cfe1b 100644 --- a/packages/review-protocol/src/index.ts +++ b/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, diff --git a/packages/review-protocol/src/review-api-client.ts b/packages/review-protocol/src/review-api-client.ts index 3a3b00f11..a6c85591f 100644 --- a/packages/review-protocol/src/review-api-client.ts +++ b/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]; diff --git a/packages/review-protocol/src/review-host-status.ts b/packages/review-protocol/src/review-host-status.ts new file mode 100644 index 000000000..da35e70aa --- /dev/null +++ b/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 & { + 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 = { + 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, +): ReviewHostStatus { + const { label, ...rest } = STATUS[host.state]; + const words = label(host.alias); + + return { + ...rest, + label: words, + sentence: host.detail ?? (words.endsWith("…") ? words : `${words}.`), + }; +} diff --git a/packages/review/app/src/api-canvas.test.tsx b/packages/review/app/src/api-canvas.test.tsx index ffcddc1f8..7804078e9 100644 --- a/packages/review/app/src/api-canvas.test.tsx +++ b/packages/review/app/src/api-canvas.test.tsx @@ -1292,6 +1292,173 @@ it("gives a review on another machine no tutorial controls, even a tutorial revi expect(tutorial).not.toHaveBeenCalledWith(true); }); +it("shows a lost remote host as a top-bar chip that retries the host and the stream", async () => { + const review = await command({ + type: "create", + title: "Remote", + target: { kind: "commits", ...pins }, + }); + + const app = new Hono().route("/reviews-api", createReviewApi(store)); + app.get("/reviews-api/:id/commits", (context) => context.json([])); + let down = false; + let watches = 0; + let cut: (() => void) | undefined; + + const bridge = testReviewBridge( + {}, + { + request: async (url, init) => { + if (!new URL(String(url)).pathname.endsWith("/watch")) + return app.request(url, init); + watches++; + + if (down) + return Response.json( + { ok: false, error: "wb-a is offline: it did not answer." }, + { status: 503 }, + ); + const response = await app.request(url, init); + + return new Response( + response.body!.pipeThrough( + new TransformStream({ + start: (controller) => { + cut = () => + controller.error( + new Error("wb-a is offline: it did not answer."), + ); + }, + }), + ), + response, + ); + }, + }, + ); + + const retry = vi.fn<(alias: string) => Promise>(async () => { + down = false; + }); + + const remoteHosts = { + states: async () => [ + down + ? { + alias: "wb-a", + state: "offline" as const, + detail: "wb-a is offline: it did not answer.", + } + : { alias: "wb-a", state: "online" as const }, + ], + retry, + install: async () => {}, + openSettings: async () => {}, + }; + + const container = document.createElement("div"); + document.body.append(container); + await act(async () => { + canvas = mount(container, { + kind: "api", + reviewId: review.reviewId, + host: "wb-a", + remoteHosts, + bridge, + setSourceView: () => {}, + }); + }); + await act(() => + vi.waitFor(() => + expect(container.querySelector("h1")?.textContent).toBe("Remote"), + ), + ); + + down = true; + await act(async () => cut?.()); + + const chip = await vi.waitFor(() => { + const found = container.querySelector( + "button.connection-chip", + ); + + expect(found?.textContent).toBe("wb-a offline"); + + return found!; + }); + + expect(chip.title).toBe( + "Click to retry\nwb-a is offline: it did not answer.", + ); + + const before = watches; + await act(async () => chip.click()); + expect(retry).toHaveBeenCalledWith("wb-a"); + await act(() => + vi.waitFor(() => { + expect(container.querySelector(".connection-chip")).toBeNull(); + }), + ); + // The stream came back at once, not after its backoff. + expect(watches).toBe(before + 1); +}); + +it("shows why a remote review has not loaded, with its host's next step", async () => { + const install = vi.fn<(alias: string) => Promise>(async () => {}); + + const container = document.createElement("div"); + document.body.append(container); + await act(async () => { + canvas = mount(container, { + kind: "api", + reviewId: "11111111-1111-4111-8111-111111111111", + host: "wb-a", + remoteHosts: { + states: async () => [ + { + alias: "wb-a", + state: "incompatible", + detail: "wb-a runs Whiteboard 0.1.0; this Desktop runs 0.2.0.", + }, + ], + retry: async () => {}, + install, + openSettings: async () => {}, + }, + bridge: testReviewBridge( + {}, + { + request: async () => + Response.json( + { ok: false, error: "wb-a runs Whiteboard 0.1.0." }, + { status: 503 }, + ), + }, + ), + }); + }); + + const waiting = await vi.waitFor(() => { + const found = container.querySelector(".host-waiting"); + + expect(found?.querySelector("h2")?.textContent).toBe( + "wb-a needs an update", + ); + + return found!; + }); + + expect(waiting.textContent).toContain( + "wb-a runs Whiteboard 0.1.0; this Desktop runs 0.2.0.", + ); + await act(async () => + [...waiting.querySelectorAll("button")] + .find((button) => button.textContent === "Install")! + .click(), + ); + expect(install).toHaveBeenCalledWith("wb-a"); +}); + async function mountPeekReview(content: { host?: string; available?: { sourceWindows: boolean }; diff --git a/packages/review/app/src/api-canvas.tsx b/packages/review/app/src/api-canvas.tsx index 9c344f1ff..84e03918f 100644 --- a/packages/review/app/src/api-canvas.tsx +++ b/packages/review/app/src/api-canvas.tsx @@ -40,6 +40,7 @@ import { } from "./authoring-cursor"; import { SaveMarkdown } from "./blocks"; import { CanvasQueryProvider } from "./canvas-query"; +import { ConnectionContext, HostWaiting } from "./connection-chip"; import { DisplayedReviewVersionContext } from "./displayed-review-version-context"; import { DrawQueueProvider } from "./draw-queue-provider"; import { @@ -112,11 +113,13 @@ export function ApiCanvas({ const sourceRef = useRef<{ key: string; version: number }>(undefined); const sourceVersion = sourceRef.current?.key; const [error, setError] = useState(); + const [lost, setLost] = useState(); useEffect(() => { const abort = new AbortController(); const loader = createDocumentLoader(client); setData(undefined); setTrashedAt(null); + setLost(undefined); setActivity(undefined); setCursor(null); setLensCursor(null); @@ -175,6 +178,7 @@ export function ApiCanvas({ content.reviewId, abort.signal, async (snapshot) => { + setLost(undefined); setActivity(snapshot.activity); setCursor((current) => nextCursor(current, cursorMemory, snapshot)); setLensCursor((current) => @@ -237,9 +241,7 @@ export function ApiCanvas({ return; } - setError( - `Connection lost. Reconnecting… ${cause instanceof Error ? cause.message : ""}`, - ); + setLost(cause instanceof Error ? cause.message : ""); }, ); })(); @@ -467,16 +469,39 @@ export function ApiCanvas({ ); + const { host, remoteHosts } = content; + + const connection = useMemo( + () => + lost === undefined + ? undefined + : { + host, + hosts: remoteHosts, + detail: lost, + reconnect: () => client.reconnect(), + }, + [lost, host, remoteHosts, client], + ); + // Loads are near-instant, so stay blank until there is data or an error. // Both branches root the same query provider, so its cache outlives a load. + // Without data there is no top bar for the connection chip. if (!data) return ( {trashBanner} - {error !== undefined && ( + {error === undefined && host && remoteHosts ? ( + client.reconnect()} + lost={lost} + /> + ) : (error ?? lost) !== undefined ? ( <>

- {error} + {error ?? `Connection lost. Reconnecting… ${lost}`}

{version !== undefined && ( )} - )} + ) : null} ); @@ -525,41 +550,43 @@ export function ApiCanvas({

)} {trashBanner} - - + - - - -
- -
- - - + + +
+ +
+ + + + - - + + diff --git a/packages/review/app/src/authoring-activity.tsx b/packages/review/app/src/authoring-activity.tsx index 3edaf32c8..3ae0b096c 100644 --- a/packages/review/app/src/authoring-activity.tsx +++ b/packages/review/app/src/authoring-activity.tsx @@ -7,6 +7,7 @@ import { useContext, useState } from "react"; import { agentColor } from "./agent-colors"; import { AuthoringActivityContext } from "./authoring-activity-context"; import { surfaceOf } from "./authoring-cursor"; +import { ConnectionChip, ConnectionContext } from "./connection-chip"; import { AuthoringCursorContext, LensCursorContext, @@ -35,11 +36,14 @@ export function AuthoringActivityBadge({ onLocate?(view: "review" | "diff"): void; }) { const activity = useContext(AuthoringActivityContext); + const lost = useContext(ConnectionContext) !== undefined; const tooltip = useTooltip( "Activity updates stopped. This does not mean the agent finished.", ); + if (lost) return ; + if (!activity) return null; // The class is a marker for tests. diff --git a/packages/review/app/src/connection-chip.tsx b/packages/review/app/src/connection-chip.tsx new file mode 100644 index 000000000..f9d32da44 --- /dev/null +++ b/packages/review/app/src/connection-chip.tsx @@ -0,0 +1,239 @@ +import { fontSize, fontWeight, motion, radius } from "@canvas/scale.stylex"; +import { Button } from "@canvas/ui/button"; +import { + type ReviewHostStatus, + type ReviewRemoteHostActions, + reviewHostStatus, +} from "@dev.fast/review-protocol"; +import * as stylex from "@stylexjs/stylex"; +import { createContext, useContext } from "react"; + +import { RefreshIcon } from "./icons"; +import { + ACTION_WORDS, + useHostAction, + useRemoteHostState, +} from "./remote-host-state"; +import { withClass } from "./stylex-props"; +import { tokens } from "./tokens.stylex"; +import { useTooltip } from "./use-tooltip"; + +export interface LostConnection { + host?: string; + hosts?: ReviewRemoteHostActions; + detail: string; + /** Reopen the review stream after a retry. */ + reconnect(): void; +} + +export const ConnectionContext = createContext( + undefined, +); + +/** Shown while the review stream is down; on a remote review it offers the host's next step. */ +export function ConnectionChip() { + const connection = useContext(ConnectionContext); + const { host, hosts } = connection ?? {}; + const state = useRemoteHostState(hosts, host, connection !== undefined); + + const status = + state && state.state !== "online" ? reviewHostStatus(state) : undefined; + + const action = status ? status.action : host && hosts ? "retry" : undefined; + + const { run, pending, failure } = useHostAction( + hosts, + host, + connection?.reconnect, + ); + + const tooltip = useTooltip( + action ? `Click to ${ACTION_WORDS[action].toLowerCase()}` : "Reconnecting…", + { detail: failure ?? status?.sentence ?? connection?.detail }, + ); + + if (!connection) return null; + + const label = + status?.label ?? (host ? `${host} disconnected` : "Disconnected"); + + const spinning = pending || status?.busy; + + if (!action) + return ( + + {label} + {spinning ? ( + + ) : null} + + ); + + return ( + + ); +} + +/** A host's next step as a button; `failure` says why the last one failed. */ +export function useHostActionButton( + hosts: ReviewRemoteHostActions | undefined, + alias: string | undefined, + action: ReviewHostStatus["action"], + retried?: () => void, +) { + const { run, pending, failure } = useHostAction(hosts, alias, retried); + + const button = + action && hosts ? ( + + ) : null; + + return { failure, button }; +} + +/** A remote review that has not loaded: what its host is doing, and the next step. */ +export function HostWaiting({ + host, + hosts, + reconnect, + lost, +}: { + host: string; + hosts: ReviewRemoteHostActions; + reconnect(): void; + lost?: string; +}) { + const state = useRemoteHostState(hosts, host, true); + + const status = + state && state.state !== "online" ? reviewHostStatus(state) : undefined; + + const { failure, button } = useHostActionButton( + hosts, + host, + status?.action, + reconnect, + ); + + if (!status) + return lost === undefined ? null : ( +

+ Connection lost. Reconnecting… {lost} +

+ ); + + return ( +
+

{status.label}

+

+ {failure ?? status.sentence} +

+ {button} +
+ ); +} + +const REDUCED = "@media (prefers-reduced-motion: reduce)"; + +const spin = stylex.keyframes({ + to: { transform: "rotate(360deg)" }, +}); + +const styles = stylex.create({ + chip: { + display: "inline-flex", + flex: "0 1 auto", + minWidth: 0, + maxWidth: "320px", + alignItems: "center", + gap: "6px", + height: tokens.chromeControlHeight, + marginInline: "4px", + padding: "0 10px", + borderWidth: "1px", + borderStyle: "solid", + borderColor: tokens.warningOutline, + borderRadius: radius.pill, + backgroundColor: tokens.warningWash, + color: tokens.changeModified, + font: `${fontWeight.semibold} ${fontSize.small} ${tokens.fontMono}`, + whiteSpace: "nowrap", + }, + // A long alias ends in an ellipsis; the tooltip has it all. + text: { + minWidth: 0, + overflow: "hidden", + textOverflow: "ellipsis", + }, + button: { + paddingInlineEnd: "7px", + cursor: { default: "pointer", ":disabled": "progress" }, + filter: { default: null, ":hover:not(:disabled)": "brightness(1.15)" }, + outline: { + default: null, + ":focus-visible": `1px solid ${tokens.changeModified}`, + }, + outlineOffset: { default: null, ":focus-visible": "1px" }, + }, + waiting: { + display: "flex", + flexDirection: "column", + alignItems: "flex-start", + gap: "12px", + maxWidth: "72ch", + margin: "32px auto", + padding: "0 24px", + font: `${fontSize.reading}/1.6 ${tokens.fontDisplay}`, + }, + waitingLabel: { + margin: 0, + color: tokens.changeModified, + font: `${fontWeight.semibold} ${fontSize.reading} ${tokens.fontMono}`, + }, + waitingSentence: { + margin: 0, + color: tokens.inkMuted, + }, + action: { + textDecorationLine: "underline", + textUnderlineOffset: "2px", + }, + icon: { + flexShrink: 0, + width: "13px", + height: "13px", + }, + spinning: { + animationName: { default: spin, [REDUCED]: "none" }, + animationDuration: { default: motion.pulse, [REDUCED]: motion.instant }, + animationTimingFunction: "linear", + animationIterationCount: "infinite", + }, +}); diff --git a/packages/review/app/src/desktop-entry.tsx b/packages/review/app/src/desktop-entry.tsx index 27c8be728..f57499ab7 100644 --- a/packages/review/app/src/desktop-entry.tsx +++ b/packages/review/app/src/desktop-entry.tsx @@ -129,7 +129,7 @@ function Home({ onRestore={ restoreReview ? (review) => restoreReview(review.reviewId) : undefined } - hostStates={content.hostStates} + remoteHosts={content.remoteHosts} onboarding={content.onboarding} onOpenTutorial={content.openTutorial} onOpenSettings={content.openSettings} diff --git a/packages/review/app/src/remote-host-state.ts b/packages/review/app/src/remote-host-state.ts new file mode 100644 index 000000000..7c37d9b94 --- /dev/null +++ b/packages/review/app/src/remote-host-state.ts @@ -0,0 +1,118 @@ +import type { + ReviewGatewayHostState, + ReviewHostStatus, + ReviewRemoteHostActions, +} from "@dev.fast/review-protocol"; +import { type RefObject, useEffect, useState } from "react"; + +const STATES_EVERY_MS = 3000; + +export function useRemoteHostStates( + hosts: Pick | undefined, + watch = true, +) { + const [states, setStates] = useState(); + + useEffect(() => { + if (!hosts || !watch) return; + let live = true; + let reading = false; + + const read = () => { + if (document.hidden || reading) return; + reading = true; + void hosts + .states() + .then((next) => { + if (live) setStates(next); + }) + .catch(() => undefined) + .finally(() => { + reading = false; + }); + }; + + read(); + const timer = setInterval(read, STATES_EVERY_MS); + + return () => { + live = false; + clearInterval(timer); + }; + }, [hosts, watch]); + + return watch ? states : undefined; +} + +export function useRemoteHostState( + hosts: ReviewRemoteHostActions | undefined, + alias: string | undefined, + watch: boolean, +) { + return useRemoteHostStates(alias ? hosts : undefined, watch)?.find( + (host) => host.alias === alias, + ); +} + +export const ACTION_WORDS: Record< + NonNullable, + string +> = { retry: "Retry", install: "Install", settings: "Open Settings" }; + +/** Runs a host's next step; Settings opens scrolled to Remote hosts. */ +export function useHostAction( + hosts: + | (Pick & + Partial>) + | undefined, + alias: string | undefined, + retried?: () => void, +) { + const [pending, setPending] = useState(false); + const [failure, setFailure] = useState(); + + const run = (action: NonNullable) => { + if (!hosts || !alias) return; + setPending(true); + setFailure(undefined); + + const step = + action === "retry" + ? hosts.retry(alias).then(retried) + : action === "install" + ? hosts.install(alias) + : Promise.resolve(hosts.openSettings?.()).then(revealRemoteHosts); + + void step + .catch((error: Error) => setFailure(error.message)) + .finally(() => setPending(false)); + }; + + return { run, pending, failure }; +} + +// Every canvas tab shares this module, so a review can ask the Settings tab, +// open or about to mount, to scroll to Remote hosts. +let requested = false; + +const reveals = new EventTarget(); + +function revealRemoteHosts() { + requested = true; + reveals.dispatchEvent(new Event("reveal")); +} + +export function useSettingsReveal(ref: RefObject) { + useEffect(() => { + const reveal = () => { + if (!requested || !ref.current) return; + requested = false; + ref.current.scrollIntoView({ block: "start" }); + }; + + reveal(); + reveals.addEventListener("reveal", reveal); + + return () => reveals.removeEventListener("reveal", reveal); + }, [ref]); +} diff --git a/packages/review/app/src/remote-hosts-section.browser.test.tsx b/packages/review/app/src/remote-hosts-section.browser.test.tsx index 3f7e5d709..b4a78eb8b 100644 --- a/packages/review/app/src/remote-hosts-section.browser.test.tsx +++ b/packages/review/app/src/remote-hosts-section.browser.test.tsx @@ -221,9 +221,9 @@ test("lists an online host's language groups after its language features, each w expect(rows()[1]).not.toContain("rust"); }); -test("offers Retry to a host that failed to authenticate, is unreachable, not installed or unsupported", async () => { +test("offers Retry to a host that failed to authenticate, is offline or unreachable, not installed or unsupported", async () => { const hosts = remoteHosts( - ["devbox", "box2", "box3", "box4", "box5"], + ["devbox", "box2", "box3", "box4", "box5", "box6"], [ { alias: "devbox", state: "auth-failed", detail: "Permission denied" }, { alias: "box2", state: "unreachable", detail: "timed out" }, @@ -234,6 +234,7 @@ test("offers Retry to a host that failed to authenticate, is unreachable, not in detail: "This host runs glibc 2.31; Whiteboard needs 2.34 or newer.", }, { alias: "box5", state: "installing" }, + { alias: "box6", state: "offline" }, ], ); @@ -243,7 +244,6 @@ test("offers Retry to a host that failed to authenticate, is unreachable, not in page.getByRole("button", { name: "Retry devbox" }).elements(), ).toHaveLength(1), ); - expect(rows()[3]).toContain("unsupported"); expect(rows()[3]).toContain("glibc 2.31"); expect(rows()[4]).toContain("installing"); expect( @@ -251,7 +251,13 @@ test("offers Retry to a host that failed to authenticate, is unreachable, not in .getByRole("button", { name: /^Retry / }) .elements() .map((button) => button.getAttribute("aria-label")), - ).toEqual(["Retry devbox", "Retry box2", "Retry box3", "Retry box4"]); + ).toEqual([ + "Retry devbox", + "Retry box2", + "Retry box3", + "Retry box4", + "Retry box6", + ]); await page.getByRole("button", { name: "Retry box2" }).click(); expect(hosts.retry).toHaveBeenCalledWith("box2"); await page.getByRole("button", { name: "Retry box3" }).click(); @@ -292,9 +298,7 @@ test("shows the install step, offers Install to a declined host, and Retry after await render(hosts); await vi.waitFor(() => - expect(rows()[0]).toContain( - "installing · Installing the Whiteboard package.", - ), + expect(rows()[0]).toContain("Installing the Whiteboard package."), ); expect( page @@ -480,13 +484,9 @@ test("a failed host's detail copies on click; a healthy one does not", async () ), ); - await vi.waitFor(() => - expect(rows()[1]).toContain("unreachable · ssh: timed out"), - ); + await vi.waitFor(() => expect(rows()[1]).toContain("ssh: timed out")); expect(page.getByRole("button", { name: "online" }).query()).toBeNull(); - await page - .getByRole("button", { name: "unreachable · ssh: timed out" }) - .click(); - expect(writeText).toHaveBeenCalledWith("unreachable · ssh: timed out"); + await page.getByRole("button", { name: "ssh: timed out" }).click(); + expect(writeText).toHaveBeenCalledWith("ssh: timed out"); }); diff --git a/packages/review/app/src/remote-hosts-section.tsx b/packages/review/app/src/remote-hosts-section.tsx index 375bfb079..de5bc1659 100644 --- a/packages/review/app/src/remote-hosts-section.tsx +++ b/packages/review/app/src/remote-hosts-section.tsx @@ -3,40 +3,38 @@ import { Checkbox } from "@canvas/ui/checkbox"; import { TextField } from "@canvas/ui/text-field"; import { REVIEW_CLI_INSTALL_TARGET_LABELS, - type ReviewGatewayHostState, type ReviewRemoteAgent, type ReviewRemoteHostsSettings, + reviewHostStatus, } from "@dev.fast/review-protocol"; import * as stylex from "@stylexjs/stylex"; -import { useEffect, useState } from "react"; +import { useEffect, useRef, useState } from "react"; import { CopyableText } from "./copy-text"; +import { + ACTION_WORDS, + useHostAction, + useRemoteHostStates, + useSettingsReveal, +} from "./remote-host-state"; import { settingsStyles as styles } from "./settings-styles"; import { tokens } from "./tokens.stylex"; -const STATES_EVERY_MS = 3000; - -const RETRIED = new Set([ - "auth-failed", - "incompatible", - "not-installed", - "unreachable", - "unsupported", -]); - export function RemoteHostsSection({ hosts, }: { hosts: ReviewRemoteHostsSettings; }) { const [configured, setConfigured] = useState(hosts.configured); - const [states, setStates] = useState(); + const states = useRemoteHostStates(hosts); const [suggestions, setSuggestions] = useState([]); const [alias, setAlias] = useState(""); const [busy, setBusy] = useState(false); const [error, setError] = useState(); const [removing, setRemoving] = useState(); const [uninstall, setUninstall] = useState(false); + const section = useRef(null); + useSettingsReveal(section); useEffect(() => { void hosts @@ -45,33 +43,6 @@ export function RemoteHostsSection({ .catch(() => undefined); }, [hosts]); - useEffect(() => { - let live = true; - let reading = false; - - const read = () => { - if (document.hidden || reading) return; - reading = true; - void hosts - .states() - .then((next) => { - if (live) setStates(next); - }) - .catch(() => undefined) - .finally(() => { - reading = false; - }); - }; - - read(); - const timer = setInterval(read, STATES_EVERY_MS); - - return () => { - live = false; - clearInterval(timer); - }; - }, [hosts]); - const save = async (next: string[]) => { setBusy(true); setError(undefined); @@ -113,10 +84,15 @@ export function RemoteHostsSection({ }; return ( -
+

Remote hosts

{configured.map((name) => { const state = states?.find((host) => host.alias === name); + const failed = !!state && !!reviewHostStatus(state).action; return (
@@ -124,8 +100,13 @@ export function RemoteHostsSection({ {name} {states ? ( ) : null} {state?.state === "online" ? ( @@ -162,22 +143,10 @@ export function RemoteHostsSection({
{state?.declined ? ( - + ) : null} - {state && RETRIED.has(state.state) ? ( - + {failed ? ( + ) : null} + + ); +} + function Detail({ text, failed, diff --git a/packages/review/app/src/review-home-styles.ts b/packages/review/app/src/review-home-styles.ts index 7b7d7e02c..e1b8be461 100644 --- a/packages/review/app/src/review-home-styles.ts +++ b/packages/review/app/src/review-home-styles.ts @@ -316,6 +316,12 @@ export const styles = stylex.create({ textAlign: "left", }, unavailableRow: { opacity: 0.55 }, + hostNotice: { + display: "flex", + alignItems: "center", + flexWrap: "wrap", + gap: "12px", + }, time: { color: tokens.inkFaint, fontVariantNumeric: "tabular-nums", diff --git a/packages/review/app/src/review-home-view.browser.test.tsx b/packages/review/app/src/review-home-view.browser.test.tsx index fc45a138b..82dde9b03 100644 --- a/packages/review/app/src/review-home-view.browser.test.tsx +++ b/packages/review/app/src/review-home-view.browser.test.tsx @@ -1,6 +1,7 @@ import type { ReviewApiSummary, ReviewGatewayHostState, + ReviewRemoteHostActions, } from "@dev.fast/review-protocol"; import { act } from "react"; import { type Root, createRoot } from "react-dom/client"; @@ -905,23 +906,23 @@ describe("Home interactions", () => { expect(titles()).toEqual(["One"]); }); it.each([ - ["offline", "offline"], - ["connecting", "connecting"], - ["not-installed", "not installed"], + ["offline", "offline", "devbox offline"], + ["connecting", "connecting", "Connecting to devbox…"], + ["not-installed", "not installed", "Whiteboard not installed on devbox"], ] as const)( "draws a %s host's review as unavailable, and opening it shows the host's detail", - async (hostState, words) => { + async (hostState, words, label) => { const onOpen = vi.fn<(review: ReviewApiSummary) => void>(); await render([remote("Remote", { hostState })], { onOpen, - hostStates: async () => [ + remoteHosts: hostsOf(async () => [ { alias: "devbox", state: hostState, detail: "devbox is offline: it did not answer within 3 seconds.", }, - ], + ]), }); const row = container.querySelector("[data-home-row]")!; @@ -933,27 +934,31 @@ describe("Home interactions", () => { button("Open Remote") .querySelector("span[title]")! .getAttribute("title"), - ).toBe(`devbox is ${words}`); + ).toBe(label); await click("Open Remote"); expect(onOpen).not.toHaveBeenCalled(); - expect(alert()).toBe( - "devbox is offline: it did not answer within 3 seconds.", + await vi.waitFor(() => + expect(alert()).toContain( + "devbox is offline: it did not answer within 3 seconds.", + ), ); }, ); - it("says only the state while the host's detail loads or when it has none", async () => { + it("says the state and offers its next step while the host's detail loads or when it has none", async () => { const answer = Promise.withResolvers(); await render([remote("Remote", { hostState: "offline" })], { - hostStates: () => answer.promise, + remoteHosts: hostsOf(() => answer.promise), }); await click("Open Remote"); - expect(alert()).toBe("devbox is offline."); + expect(alert()).toContain("devbox offline."); + expect(button("Retry")).toBeTruthy(); await act(async () => answer.resolve([{ alias: "devbox", state: "offline" }]), ); - expect(alert()).toBe("devbox is offline."); + expect(alert()).toContain("devbox offline."); + expect(button("Retry")).toBeTruthy(); }); - it("keeps the message of the last unavailable review opened, and drops it when the list changes", async () => { + it("keeps the message of the last unavailable review opened, and opens that review once its host is back", async () => { const answers = [ Promise.withResolvers(), Promise.withResolvers(), @@ -972,14 +977,18 @@ describe("Home interactions", () => { { alias: "other", state: "offline" as const, detail: "other detail" }, ]; - await render(reviews, { hostStates }); + const onOpen = vi.fn<(review: ReviewApiSummary) => void>(); + const remoteHosts = hostsOf(hostStates); + await render(reviews, { onOpen, remoteHosts }); await click("Open One"); await click("Open Two"); await act(async () => answers[1]!.resolve(states)); await act(async () => answers[0]!.resolve(states)); - expect(alert()).toBe("other detail"); - await render([{ ...reviews[1]!, hostState: "online" }], { hostStates }); + expect(alert()).toBe("other detailRetry"); + const back = { ...reviews[1]!, hostState: "online" as const }; + await render([reviews[0]!, back], { onOpen, remoteHosts }); expect(alert()).toBeUndefined(); + expect(onOpen).toHaveBeenCalledExactlyOnceWith(back); }); it("draws an online remote review as available", async () => { const onOpen = vi.fn<(review: ReviewApiSummary) => void>(); @@ -994,6 +1003,17 @@ describe("Home interactions", () => { expect(onOpen).toHaveBeenCalledWith(review); }); + function hostsOf( + states: () => Promise, + ): ReviewRemoteHostActions { + return { + states, + retry: async () => {}, + install: async () => {}, + openSettings: async () => {}, + }; + } + function alert() { return container.querySelector('[role="alert"]')?.textContent; } diff --git a/packages/review/app/src/review-home-view.tsx b/packages/review/app/src/review-home-view.tsx index 9c130962d..e3441c26b 100644 --- a/packages/review/app/src/review-home-view.tsx +++ b/packages/review/app/src/review-home-view.tsx @@ -2,8 +2,9 @@ import { REVIEW_DISCORD_URL, type ReviewApiSummary, type ReviewCanvasOnboarding, - type ReviewGatewayHostState, + type ReviewRemoteHostActions, homeSessions, + reviewHostStatus, } from "@dev.fast/review-protocol"; import { fuzzySegments } from "@review/fuzzy-match"; import * as stylex from "@stylexjs/stylex"; @@ -19,6 +20,7 @@ import { useState, } from "react"; +import { useHostActionButton } from "./connection-chip"; import { CopyableText, copyText } from "./copy-text"; import { countLabel, formatRelativeTime } from "./format"; import { homeStyles } from "./home-styles"; @@ -41,6 +43,7 @@ import { } from "./icons"; import { homeGroupMarker, homeRowMarker } from "./markers.stylex"; import { useNotify } from "./notify"; +import { useRemoteHostState } from "./remote-host-state"; import type { ReviewFindHost } from "./review-find"; import { HomeFilterPicker, @@ -88,7 +91,7 @@ interface ReviewHomeProps { onDelete?(review: ReviewApiSummary): Promise; onTrash?(review: ReviewApiSummary): Promise; onRestore?(review: ReviewApiSummary): Promise; - hostStates?(): Promise; + remoteHosts?: ReviewRemoteHostActions; onboarding?: ReviewCanvasOnboarding; onOpenTutorial?(): void; onOpenSettings?(): void; @@ -138,7 +141,7 @@ export function ReviewHome({ onDelete, onTrash, onRestore, - hostStates, + remoteHosts, onboarding, onOpenTutorial, onOpenSettings, @@ -182,33 +185,29 @@ export function ReviewHome({ const [busy, setBusy] = useState(false); const busyRef = useRef(false); const [error, setError] = useState(""); - const [hostMessage, setHostMessage] = useState(); - const hostMessageGeneration = useRef(0); + // The unavailable review last opened; it opens once its host is back. + const [waiting, setWaiting] = useState(); useEffect(() => { - hostMessageGeneration.current++; - setHostMessage(undefined); - }, [reviews]); - - const open = useCallback( - async (review: ReviewApiSummary) => { - const generation = ++hostMessageGeneration.current; + if (!waiting) return; - if (!unavailable(review)) { - setHostMessage(undefined); - onOpen(review); + const current = reviews.find( + (review) => review.reviewId === waiting.reviewId, + ); - return; - } + if (current && unavailable(current)) return setWaiting(current); + setWaiting(undefined); - setHostMessage(`${review.host} is ${hostStateWords(review)}.`); - const states = await hostStates?.().catch(() => undefined); - const detail = states?.find((host) => host.alias === review.host)?.detail; + if (current) onOpen(current); + }, [reviews]); - if (detail && generation === hostMessageGeneration.current) - setHostMessage(detail); + const open = useCallback( + (review: ReviewApiSummary) => { + if (unavailable(review)) return setWaiting(review); + setWaiting(undefined); + onOpen(review); }, - [onOpen, hostStates], + [onOpen], ); const [notice, setNotice] = useState(null); @@ -1064,10 +1063,12 @@ export function ReviewHome({ ) : null} {error &&

{error}

} - {hostMessage ? ( -

- -

+ {waiting?.host && waiting.hostState ? ( + ) : null} {!visible.length && ( - {hostStateWords(review)} + {hostStatus(review).status} ); @@ -1542,8 +1543,32 @@ function unavailable(review: ReviewApiSummary): boolean { return review.hostState !== undefined && review.hostState !== "online"; } -function hostStateWords(review: ReviewApiSummary): string { - return review.hostState?.replace("-", " ") ?? ""; +function hostStatus(review: ReviewApiSummary) { + return reviewHostStatus({ + alias: review.host ?? "", + state: review.hostState ?? "online", + }); +} + +function HostNotice({ + alias, + state, + hosts, +}: { + alias: string; + state: NonNullable; + hosts?: ReviewRemoteHostActions; +}) { + const live = useRemoteHostState(hosts, alias, true); + const status = reviewHostStatus(live ?? { alias, state }); + const { failure, button } = useHostActionButton(hosts, alias, status.action); + + return ( +

+ + {button} +

+ ); } function PullRequestPill({ diff --git a/packages/review/app/src/shell-styles.ts b/packages/review/app/src/shell-styles.ts index 3909a8f7d..530acf27f 100644 --- a/packages/review/app/src/shell-styles.ts +++ b/packages/review/app/src/shell-styles.ts @@ -247,9 +247,10 @@ export const shellStyles = stylex.create({ }, }, // The scratchpad has no source tree or pins to set apart, so no rule. + // Shrinks before the action row scrolls, so its badges end in an ellipsis. topbarContext: { display: "flex", - flex: "0 0 auto", + flex: "0 1 auto", minWidth: 0, alignItems: "center", marginRight: "auto", @@ -266,6 +267,7 @@ export const shellStyles = stylex.create({ // the viewport: an open side panel narrows the column without resizing // the window. In a narrow column this ghost Button drops its label. openSourceTree: { + flexShrink: 0, justifyContent: { default: null, [narrowContent]: "center" }, width: { default: null, [narrowContent]: tokens.chromeControlHeight }, padding: { default: "0 10px", [narrowContent]: 0 }, diff --git a/packages/review/src/server/review-gateway-streams.ts b/packages/review/src/server/review-gateway-streams.ts index 1edcb128b..2e0b1a7c3 100644 --- a/packages/review/src/server/review-gateway-streams.ts +++ b/packages/review/src/server/review-gateway-streams.ts @@ -5,6 +5,7 @@ import { type ReviewApiSummary, type ReviewGatewayHostState, parseJsonText, + reviewHostStatus, } from "@dev.fast/review-protocol"; import { coverageModeSchema } from "@review/review-api/review-progress.js"; import { z } from "zod"; @@ -47,7 +48,7 @@ export type Located = | undefined; export const downDetail = (down: ReviewGatewayHostState) => - down.detail ?? `${down.alias} is ${down.state}.`; + reviewHostStatus(down).sentence; const watchPath = (subscriptions: object[]) => `/reviews-api/watch?subscriptions=${encodeURIComponent(JSON.stringify(subscriptions))}`; diff --git a/packages/review/src/server/review-gateway-transport.ts b/packages/review/src/server/review-gateway-transport.ts index ea47c81ab..6ec8da389 100644 --- a/packages/review/src/server/review-gateway-transport.ts +++ b/packages/review/src/server/review-gateway-transport.ts @@ -182,6 +182,7 @@ const ERROR_WORDS = new Map([ ["ECONNREFUSED", "it refused the connection"], ["ECONNRESET", "it closed the connection"], ["UND_ERR_SOCKET", "it closed the connection"], + ["EPIPE", "it closed the connection"], ["ETIMEDOUT", "it did not answer"], ["UND_ERR_HEADERS_TIMEOUT", NO_ANSWER], ]); @@ -199,5 +200,5 @@ export function errorCode(cause: unknown): string { export function errorText(cause: unknown): string { const code = errorCode(cause); - return ERROR_WORDS.get(code) ?? code; + return ERROR_WORDS.get(code) ?? `it failed (${code})`; } diff --git a/packages/review/src/server/review-gateway.ts b/packages/review/src/server/review-gateway.ts index 877e06560..55a5026ab 100644 --- a/packages/review/src/server/review-gateway.ts +++ b/packages/review/src/server/review-gateway.ts @@ -266,7 +266,7 @@ export function createReviewGateway(input: { return answer(remote.alias, timedOut ? 504 : 502, { ok: false, - error: `${remote.alias} did not answer: ${reason}.`, + error: `Could not reach ${remote.alias}: ${reason}.`, }); }