diff --git a/src/browser/components/AppLoader/AppLoader.tsx b/src/browser/components/AppLoader/AppLoader.tsx index b69423fb37b..df293ff19c0 100644 --- a/src/browser/components/AppLoader/AppLoader.tsx +++ b/src/browser/components/AppLoader/AppLoader.tsx @@ -15,7 +15,7 @@ import { getAppConfigStore } from "../../stores/AppConfigStore"; import { getDraftStore, useDraftStoreReady } from "../../stores/DraftStore"; import { getProvidersConfigStore } from "../../stores/ProvidersConfigStore"; import { ProjectProvider, useProjectContext } from "../../contexts/ProjectContext"; -import { APIProvider, useAPI, type APIClient } from "@/browser/contexts/API"; +import { APIProvider, useAPI, useLiveAPI, type APIClient } from "@/browser/contexts/API"; import { WorkspaceProvider, useWorkspaceContext } from "../../contexts/WorkspaceContext"; import { RouterProvider } from "../../contexts/RouterContext"; import { TerminalRouterProvider } from "../../terminal/TerminalRouterContext"; @@ -165,7 +165,9 @@ function AppLoaderInner() { const workspaceContext = useWorkspaceContext(); const projectContext = useProjectContext(); const apiState = useAPI(); - const api = apiState.api; + // The stores must see "offline": they drop their client and resubscribe once it is back. + // useAPI().api can keep the last client while Xum reconnects (see useLiveAPI). + const api = useLiveAPI(); const workspaceStoreInstance = useWorkspaceStoreRaw(); const gitStatusStore = useGitStatusStoreRaw(); diff --git a/src/browser/contexts/API.test.tsx b/src/browser/contexts/API.test.tsx index d244783e50e..c2c7dfeb1bc 100644 --- a/src/browser/contexts/API.test.tsx +++ b/src/browser/contexts/API.test.tsx @@ -120,10 +120,11 @@ import type { UseAPIResult as _UseAPIResult, APIProvider as APIProviderType } fr const RealAPIModule: { APIProvider: typeof APIProviderType; useAPI: () => _UseAPIResult; + useLiveAPI: () => _UseAPIResult["api"]; useConnectionLatencyMs: () => number | null; } = require("./API?real=1"); /* eslint-enable @typescript-eslint/no-require-imports, @typescript-eslint/no-unsafe-assignment */ -const { APIProvider, useAPI, useConnectionLatencyMs } = RealAPIModule; +const { APIProvider, useAPI, useLiveAPI, useConnectionLatencyMs } = RealAPIModule; type UseAPIResult = _UseAPIResult; // Each observation carries the API context value by reference (apiState) plus the latency @@ -365,6 +366,37 @@ describe("API reconnection", () => { }); }); + // The stores and deferred writes read useLiveAPI(). Until the provider keeps the last client + // in the gap, it must equal useAPI().api in every render, so they see exactly what they saw. + test("useLiveAPI() equals useAPI().api in every render across drops and reconnects", async () => { + const renders: Array<{ status: string; same: boolean; api: unknown }> = []; + function Probe() { + const state = useAPI(); + const live = useLiveAPI(); + renders.push({ status: state.status, same: live === state.api, api: state.api }); + return null; + } + render( + + + + ); + for (let cycle = 0; cycle < 3; cycle++) { + const ws = MockWebSocket.lastInstance(); + await act(async () => { + ws!.simulateOpen(); + await new Promise((r) => setTimeout(r, 10)); + }); + act(() => { + ws!.simulateClose(1006); + }); + await waitFor(() => expect(MockWebSocket.instances.length).toBeGreaterThan(cycle + 1)); + } + expect(renders.filter((r) => r.status === "connected")).not.toHaveLength(0); + expect(renders.filter((r) => r.status === "reconnecting")).not.toHaveLength(0); + expect(renders.filter((r) => !r.same)).toEqual([]); + }); + test("shows auth_required on close with auth error codes (4401)", async () => { const states: string[] = []; diff --git a/src/browser/contexts/API.tsx b/src/browser/contexts/API.tsx index ac8a7f04a25..f306d7dc4bc 100644 --- a/src/browser/contexts/API.tsx +++ b/src/browser/contexts/API.tsx @@ -30,9 +30,12 @@ export type APIState = | { status: "connecting"; api: null; error: null } | { status: "connected"; api: APIClient; error: null } | { status: "degraded"; api: APIClient; error: null } // Connected but the backend answers slowly - | { status: "reconnecting"; api: null; error: null; attempt: number } + // While reconnecting, and in an error after a lost session, `api` can be the last client: the + // App stays mounted, and panes keep what they show. Calls on it then fail fast with + // ConnectionLostError (connectionLink.ts). Code that must know "offline" uses useLiveAPI(). + | { status: "reconnecting"; api: APIClient | null; error: null; attempt: number } | { status: "auth_required"; api: null; error: string | null } - | { status: "error"; api: null; error: string }; + | { status: "error"; api: APIClient | null; error: string }; interface APIStateMethods { authenticate: (token: string) => void; @@ -676,7 +679,8 @@ function ManagedAPIProvider(props: Omit) { return { status: "error", api: null, error: state.error, ...base }; } }, [state, authenticate, retry]); - useAppConfigStoreClient(value.api); + // The store must see "offline" to resubscribe after the reconnect. + useAppConfigStoreClient(liveClientOf(value)); // Always render children - consumers handle their own loading/error states return ( @@ -738,6 +742,22 @@ export const useAPI = (): UseAPIResult => { return context; }; +/** The client while the connection is live (connected or degraded), else null. */ +export function liveClientOf(state: APIState): APIClient | null { + return state.status === "connected" || state.status === "degraded" ? state.api : null; +} + +/** + * The API client only while the connection is live; null while offline. + * + * `useAPI().api` can stay set while Xum reconnects, so the UI keeps what it shows. Code that must + * see "offline" uses this instead: the stores (they resubscribe when it comes back), deferred + * writes that must wait for a live server, and handlers that report "API not connected". + */ +export function useLiveAPI(): APIClient | null { + return liveClientOf(useAPI()); +} + /** * Like {@link useAPI} but returns null instead of throwing when there is no APIProvider. * Use this in best-effort hooks that may render outside an APIProvider (e.g. isolated diff --git a/src/browser/contexts/WorkspaceContext.tsx b/src/browser/contexts/WorkspaceContext.tsx index c4832114369..401561f2c06 100644 --- a/src/browser/contexts/WorkspaceContext.tsx +++ b/src/browser/contexts/WorkspaceContext.tsx @@ -27,7 +27,7 @@ import { import { deleteWorkspaceStorage, migrateWorkspaceStorage } from "@/browser/utils/workspaceStorage"; import { SCRATCH_PROJECT_CONFIG_KEY } from "@/common/constants/scratch"; import { MULTI_PROJECT_CONFIG_KEY } from "@/common/constants/multiProject"; -import { useAPI } from "@/browser/contexts/API"; +import { useAPI, useLiveAPI } from "@/browser/contexts/API"; import { readPersistedState, updatePersistedState } from "@/browser/hooks/usePersistedState"; import { useProjectContext } from "@/browser/contexts/ProjectContext"; import { useWorkspaceStoreRaw } from "@/browser/stores/WorkspaceStore"; @@ -464,7 +464,8 @@ export function WorkspaceProvider(props: WorkspaceProviderProps) { }, [workspaceStore] ); - useDiscardSideChatOnLeave(api, currentWorkspaceId, workspaceMetadata); + // The discard waits for a live server: useAPI().api can keep the last client while offline. + useDiscardSideChatOnLeave(useLiveAPI(), currentWorkspaceId, workspaceMetadata); // The routed creation draft id, for the startup creation-draft storage GC (never collected). const routedDraftIdRef = useRef(null); const [loading, setLoading] = useState(true); diff --git a/src/browser/contexts/connection.testHarness.tsx b/src/browser/contexts/connection.testHarness.tsx new file mode 100644 index 00000000000..655115f6558 --- /dev/null +++ b/src/browser/contexts/connection.testHarness.tsx @@ -0,0 +1,120 @@ +import { act } from "@testing-library/react"; +import type { ClientContext, ClientLink } from "@orpc/client"; +import { useState, type ReactNode } from "react"; +import { APIContext, type APIClient, type UseAPIResult } from "@/browser/contexts/API"; +import { ConnectionLink } from "@/browser/contexts/connectionLink"; +import type { TestApiOverrides } from "@/browser/testUtils"; +import { createClient } from "@/common/orpc/client"; + +/** + * How the API context looks while Xum reconnects. + * - `null-gap` (the provider today): `api` goes from a client to `null`, then to a new client. + * - `kept-gap` (the provider after the switch): `api` stays the last client with status + * "reconnecting"; its calls reject with ConnectionLostError. Then a new client arrives. + * + * Code that passes in both modes is safe today and cannot break when the provider switches. + */ +export type GapMode = "null-gap" | "kept-gap"; +export const GAP_MODES: readonly GapMode[] = ["null-gap", "kept-gap"]; + +type Procedure = (input: unknown, options: unknown) => unknown; + +/** An inner link that runs each call on the matching function of a test override object. */ +function overridesLink(overrides: TestApiOverrides): ClientLink { + return { + // A then() so a missing override or a throwing one rejects, as a real call does. + call: (path, input, options) => + Promise.resolve().then(() => { + let node: unknown = overrides; + for (const key of path) node = (node as Record | undefined)?.[key]; + if (typeof node !== "function") { + throw new Error(`No test override for ${path.join(".")}`); + } + return (node as Procedure)(input, { signal: options.signal, context: options.context }); + }), + }; +} + +const noop = () => undefined; + +function connected(api: APIClient): UseAPIResult { + return { status: "connected", api, error: null, authenticate: noop, retry: noop }; +} + +/** + * A connection the test drops and restores. Each connect builds a real oRPC client over one + * ConnectionLink, as the provider will, from fresh overrides (`makeOverrides()`), so a test can + * make the next server answer differently. + * + * The drop keeps the real order of each mode. Today the provider's state change renders first + * (React handles a socket `close` event as discrete) and the transport rejects the streams after. + * After the switch, the provider detaches the link first, so calls reject before React renders. + */ +export function createConnectionHarness( + mode: GapMode, + makeOverrides: () => TestApiOverrides +) { + const link = new ConnectionLink(); + let client: APIClient | null = null; + let publish: ((value: UseAPIResult) => void) | null = null; + // The value a Wrapper mounted now starts with (a panel reopened after a tab switch). + let latest: UseAPIResult; + let attempt = 0; + + const connect = (): UseAPIResult => { + link.attach(overridesLink(makeOverrides())); + client = createClient(link); + return connected(client); + }; + latest = connect(); + + const setValue = (value: UseAPIResult) => { + latest = value; + if (publish != null) act(() => publish?.(value)); + }; + + function Wrapper(props: { children: ReactNode }) { + const [value, setValue] = useState(() => latest); + publish = setValue; + return {props.children}; + } + + return { + mode, + Wrapper, + /** The client of the current (or last) connection. */ + get client(): APIClient | null { + return client; + }, + /** Lose the connection. In-flight calls and streams reject with ConnectionLostError. */ + async drop(): Promise { + attempt += 1; + const reconnecting = (api: APIClient | null): UseAPIResult => ({ + status: "reconnecting", + api, + error: null, + attempt, + authenticate: noop, + retry: noop, + }); + if (mode === "null-gap") { + setValue(reconnecting(null)); + link.detach(); + } else { + link.detach(); + await act(async () => { + // Let the rejections reach their handlers before React sees the drop. A macrotask runs + // only after every queued microtask, however long each rejection chain is. + await new Promise((resolve) => setTimeout(resolve, 0)); + }); + setValue(reconnecting(client)); + } + }, + /** Connect again: a new client object, as after a real reconnect. */ + reconnect(): void { + setValue(connect()); + }, + }; +} + +export type ConnectionHarness = ReturnType; diff --git a/src/browser/contexts/connectionLink.bench.ts b/src/browser/contexts/connectionLink.bench.ts new file mode 100644 index 00000000000..bf1aac3374f --- /dev/null +++ b/src/browser/contexts/connectionLink.bench.ts @@ -0,0 +1,82 @@ +/** + * Cost the ConnectionLink adds per call and per stream event, against the inner link alone. + * Run: make bench BENCH=connectionLink + * + * Every API call and every stream event (terminal output, chat deltas) goes through the link + * once the provider uses it. Gate for the switch: the wrapper must add less than 1 µs per event. + */ +import { bench, do_not_optimize, summary } from "mitata"; +import type { ClientContext, ClientLink, ClientOptions } from "@orpc/client"; +import { createClient } from "@/common/orpc/client"; +import { ConnectionLink } from "@/browser/contexts/connectionLink"; + +const OPTIONS: ClientOptions = { context: {} }; +const EVENTS = 1000; + +/** A stream that yields EVENTS values with no I/O, so the bench measures only the wrappers. */ +function syntheticStream(): AsyncIteratorObject & + AsyncIterable { + let i = 0; + const stream = { + next: () => + Promise.resolve( + i < EVENTS + ? { done: false as const, value: { n: i++ } } + : { done: true as const, value: undefined } + ), + return: (value?: unknown) => Promise.resolve({ done: true as const, value }), + throw: (error?: unknown) => + Promise.reject(error instanceof Error ? error : new Error(String(error))), + [Symbol.asyncIterator]() { + return stream; + }, + [Symbol.asyncDispose]: () => Promise.resolve(), + }; + return stream; +} + +const callLink: ClientLink = { call: () => Promise.resolve("pong") }; +const streamLink: ClientLink = { call: () => Promise.resolve(syntheticStream()) }; + +function attached(inner: ClientLink): ClientLink { + const link = new ConnectionLink(); + link.attach(inner); + return link; +} + +const wrappedCallLink = attached(callLink); +const wrappedStreamLink = attached(streamLink); + +summary(() => { + bench("one-shot call: inner link", async () => { + do_not_optimize(await callLink.call(["general", "ping"], "x", OPTIONS)); + }); + bench("one-shot call: ConnectionLink", async () => { + do_not_optimize(await wrappedCallLink.call(["general", "ping"], "x", OPTIONS)); + }); +}); + +summary(() => { + bench(`stream of ${EVENTS} events: inner link`, async () => { + const stream = (await streamLink.call( + ["terminal", "attach"], + {}, + OPTIONS + )) as AsyncIterable; + for await (const event of stream) do_not_optimize(event); + }); + bench(`stream of ${EVENTS} events: ConnectionLink`, async () => { + const stream = (await wrappedStreamLink.call( + ["terminal", "attach"], + {}, + OPTIONS + )) as AsyncIterable; + for await (const event of stream) do_not_optimize(event); + }); +}); + +// Runs once per successful connect after the switch: a new client on the shared link. +bench("createClient(link) + first property access", () => { + const client = createClient(wrappedCallLink); + do_not_optimize(client.general.ping); +}); diff --git a/src/browser/contexts/connectionLink.test.ts b/src/browser/contexts/connectionLink.test.ts new file mode 100644 index 00000000000..7ba78d51689 --- /dev/null +++ b/src/browser/contexts/connectionLink.test.ts @@ -0,0 +1,393 @@ +import { describe, expect, test } from "bun:test"; +import type { ClientContext, ClientLink, ClientOptions } from "@orpc/client"; +import { RPCLink as WebSocketLink } from "@orpc/client/websocket"; +import { isAbortError } from "@/browser/utils/isAbortError"; +import { ConnectionLink, isConnectionLostError } from "./connectionLink"; + +type Link = ClientLink; +const NO_OPTIONS: ClientOptions = { context: {} }; + +/** An inner link whose calls the test settles by hand. */ +function controlledLink() { + const calls: Array<{ + path: string[]; + input: unknown; + signal: AbortSignal | undefined; + resolve: (value: unknown) => void; + reject: (error: unknown) => void; + }> = []; + const link: Link = { + call: (path, input, options) => + new Promise((resolve, reject) => { + calls.push({ path, input, signal: options.signal, resolve, reject }); + }), + }; + return { link, calls }; +} + +/** A stream the test feeds by hand; records return(). */ +function controlledStream() { + const pulls: Array<{ + resolve: (r: IteratorResult) => void; + reject: (e: unknown) => void; + }> = []; + let returned = 0; + const stream = { + next: () => + new Promise>((resolve, reject) => { + pulls.push({ resolve, reject }); + }), + return: (value?: unknown) => { + returned += 1; + return Promise.resolve({ done: true as const, value }); + }, + [Symbol.asyncIterator]() { + return stream; + }, + }; + return { stream, pulls, returned: () => returned }; +} + +async function flush() { + for (let i = 0; i < 5; i++) await Promise.resolve(); +} + +function settleState(promise: Promise) { + const state: { status: "pending" | "resolved" | "rejected"; value?: unknown } = { + status: "pending", + }; + promise.then( + (value) => Object.assign(state, { status: "resolved", value }), + (value: unknown) => Object.assign(state, { status: "rejected", value }) + ); + return state; +} + +describe("ConnectionLink", () => { + test("forwards calls to the attached link, and to the next one after a reconnect", async () => { + const link = new ConnectionLink(); + const first = controlledLink(); + link.attach(first.link); + const a = link.call(["general", "ping"], "one", NO_OPTIONS); + expect(first.calls[0]?.path).toEqual(["general", "ping"]); + first.calls[0]?.resolve("pong-1"); + expect(await a).toBe("pong-1"); + + link.detach(); + const second = controlledLink(); + link.attach(second.link); + const b = link.call(["general", "ping"], "two", NO_OPTIONS); + expect(first.calls).toHaveLength(1); + expect(second.calls[0]?.input).toBe("two"); + second.calls[0]?.resolve("pong-2"); + expect(await b).toBe("pong-2"); + }); + + test("a call while nothing is attached rejects at once with ConnectionLostError", async () => { + const link = new ConnectionLink(); + const error = await link + .call(["general", "ping"], undefined, NO_OPTIONS) + .catch((e: unknown) => e); + expect(isConnectionLostError(error)).toBe(true); + // Existing catch sites treat it like today's socket-close abort. + expect(isAbortError(error)).toBe(true); + expect(isConnectionLostError(new DOMException("aborted", "AbortError"))).toBe(false); + }); + + test("detach() rejects pending calls even while the inner link never settles", async () => { + const link = new ConnectionLink(); + const inner = controlledLink(); + link.attach(inner.link); + const state = settleState(link.call(["workspace", "list"], undefined, NO_OPTIONS)); + link.detach(); + await flush(); + expect(state.status).toBe("rejected"); + expect(isConnectionLostError(state.value)).toBe(true); + + // A result that arrives later is dropped: the caller already has its answer. + inner.calls[0]?.resolve("late"); + await flush(); + expect(state.status).toBe("rejected"); + }); + + test("a server cancel on an attached link passes through unchanged", async () => { + const link = new ConnectionLink(); + const inner = controlledLink(); + link.attach(inner.link); + const pending = link + .call(["terminal", "onExit"], undefined, NO_OPTIONS) + .catch((e: unknown) => e); + const cancel = Object.assign(new Error("Server canceled the request"), { name: "AbortError" }); + inner.calls[0]?.reject(cancel); + const error: unknown = await pending; + expect(error).toBe(cancel); + expect(isConnectionLostError(error)).toBe(false); + }); + + test("the caller's abort wins", async () => { + const link = new ConnectionLink(); + link.attach(controlledLink().link); + const controller = new AbortController(); + controller.abort(new DOMException("stop", "AbortError")); + const error = await link + .call(["general", "ping"], undefined, { context: {}, signal: controller.signal }) + .catch((e: unknown) => e); + expect(error).toBe(controller.signal.reason); + expect(isConnectionLostError(error)).toBe(false); + }); + + test("detach() aborts the inner call, so it settles without a close event", async () => { + const link = new ConnectionLink(); + const inner = controlledLink(); + link.attach(inner.link); + const pending = link.call(["workspace", "list"], undefined, NO_OPTIONS).catch(() => undefined); + const innerSignal = inner.calls[0]?.signal; + expect(innerSignal?.aborted).toBe(false); + link.detach(); + await pending; + expect(innerSignal?.aborted).toBe(true); + expect(isConnectionLostError(innerSignal?.reason)).toBe(true); + }); + + test("a caller abort in the same task as detach() keeps the caller's reason", async () => { + const link = new ConnectionLink(); + link.attach(controlledLink().link); + const controller = new AbortController(); + const pending = link + .call(["general", "ping"], undefined, { context: {}, signal: controller.signal }) + .catch((e: unknown) => e); + const reason = new DOMException("user cancelled", "AbortError"); + controller.abort(reason); + link.detach(); + const error: unknown = await pending; + expect(error).toBe(reason); + expect(isConnectionLostError(error)).toBe(false); + }); + + test("a late detach for an old generation leaves the new one attached", async () => { + const link = new ConnectionLink(); + const oldId = link.attach(controlledLink().link); + link.detach(oldId); + const next = controlledLink(); + link.attach(next.link); + // The old socket's close event arrives after the forced reconnect. + link.detach(oldId); + expect(link.isAttached).toBe(true); + const pending = link.call(["general", "ping"], undefined, NO_OPTIONS); + next.calls[0]?.resolve("pong"); + expect(await pending).toBe("pong"); + }); + + test("attach() twice is a programmer error", () => { + const link = new ConnectionLink(); + link.attach(controlledLink().link); + expect(() => link.attach(controlledLink().link)).toThrow(/already attached/); + link.detach(); + link.detach(); // idempotent + expect(link.isAttached).toBe(false); + }); + + describe("streams", () => { + async function openStream() { + const link = new ConnectionLink(); + const inner = controlledLink(); + link.attach(inner.link); + const source = controlledStream(); + const opened = link.call(["terminal", "attach"], undefined, NO_OPTIONS); + inner.calls[0]?.resolve(source.stream); + const iterator = (await opened) as AsyncIterableIterator & + AsyncIteratorObject; + return { link, source, iterator }; + } + + test("values pass through as the same objects, so event meta is kept", async () => { + const { source, iterator } = await openStream(); + const event = { type: "screenState", data: "x" }; + const next = iterator.next(); + source.pulls[0]?.resolve({ done: false, value: event }); + expect((await next).value).toBe(event); + }); + + test("detach() rejects a waiting next() and every later one, and returns the stream", async () => { + const { link, source, iterator } = await openStream(); + const waiting = settleState(iterator.next()); + link.detach(); + await flush(); + expect(waiting.status).toBe("rejected"); + expect(isConnectionLostError(waiting.value)).toBe(true); + expect(source.returned()).toBe(1); + + const later: unknown = await iterator.next().catch((e: unknown) => e); + expect(isConnectionLostError(later)).toBe(true); + // An event from the old socket after the detach is not delivered. + source.pulls[0]?.resolve({ done: false, value: "stale" }); + await flush(); + expect(waiting.status).toBe("rejected"); + }); + + test("a for-await loop ends with ConnectionLostError on detach", async () => { + const { link, source, iterator } = await openStream(); + const seen: unknown[] = []; + const loop = (async () => { + for await (const value of iterator) seen.push(value); + })().catch((e: unknown) => e); + await flush(); + source.pulls[0]?.resolve({ done: false, value: 1 }); + await flush(); + link.detach(); + expect(isConnectionLostError(await loop)).toBe(true); + expect(seen).toEqual([1]); + }); + + test("a completed stream stays done after a later detach", async () => { + const { link, source, iterator } = await openStream(); + const last = iterator.next(); + source.pulls[0]?.resolve({ done: true, value: undefined }); + expect((await last).done).toBe(true); + link.detach(); + expect(await iterator.next()).toEqual({ done: true, value: undefined }); + }); + + test("a pull after a caller abort and a detach keeps the caller's reason", async () => { + const link = new ConnectionLink(); + const inner = controlledLink(); + link.attach(inner.link); + const controller = new AbortController(); + const opened = link.call(["terminal", "attach"], undefined, { + context: {}, + signal: controller.signal, + }); + inner.calls[0]?.resolve(controlledStream().stream); + const iterator = (await opened) as AsyncIteratorObject; + const reason = new DOMException("user closed the view", "AbortError"); + controller.abort(reason); + link.detach(); + expect(await iterator.next().catch((e: unknown) => e)).toBe(reason); + }); + + test("return() reaches the inner stream once", async () => { + const { source, iterator } = await openStream(); + await iterator.return?.(); + await iterator.return?.(); + expect(source.returned()).toBe(1); + expect(await iterator.next()).toEqual({ done: true, value: undefined }); + }); + + // The missing-close case: the inner stream's own cleanup never settles. + async function openHangingStream() { + const link = new ConnectionLink(); + const inner = controlledLink(); + link.attach(inner.link); + const hang = () => new Promise>(() => undefined); + const stream = { + next: hang, + return: hang, + throw: hang, + [Symbol.asyncIterator]() { + return stream; + }, + }; + const opened = link.call(["terminal", "attach"], undefined, NO_OPTIONS); + inner.calls[0]?.resolve(stream); + return { link, iterator: (await opened) as AsyncIteratorObject }; + } + + test("detach() settles a return() whose inner cleanup hangs", async () => { + const { link, iterator } = await openHangingStream(); + const returned = settleState(iterator.return!()); + await flush(); + expect(returned.status).toBe("pending"); + link.detach(); + await flush(); + expect(returned).toEqual({ status: "resolved", value: { done: true, value: undefined } }); + }); + + test("detach() settles a throw() whose inner cleanup hangs", async () => { + const { link, iterator } = await openHangingStream(); + const thrown = settleState(iterator.throw!(new Error("consumer failed"))); + await flush(); + expect(thrown.status).toBe("pending"); + link.detach(); + await flush(); + expect(thrown.status).toBe("rejected"); + expect(isConnectionLostError(thrown.value)).toBe(true); + }); + + test("a stream that opens after detach() is returned, not leaked", async () => { + const link = new ConnectionLink(); + const inner = controlledLink(); + link.attach(inner.link); + const opened = settleState(link.call(["terminal", "attach"], undefined, NO_OPTIONS)); + link.detach(); + const source = controlledStream(); + inner.calls[0]?.resolve(source.stream); + await flush(); + expect(opened.status).toBe("rejected"); + expect(source.returned()).toBe(1); + }); + }); +}); + +/** Just enough of a browser WebSocket for oRPC's WebSocket link. */ +class FakeSocket { + readyState = 1; // OPEN + sent: string[] = []; + private listeners = new Map void>>(); + addEventListener(type: string, listener: (event: unknown) => void) { + this.listeners.set(type, [...(this.listeners.get(type) ?? []), listener]); + } + send(data: string) { + this.sent.push(data); + } + emit(type: string, event: unknown) { + for (const listener of this.listeners.get(type) ?? []) listener(event); + } +} + +async function waitFor(check: () => boolean) { + for (let i = 0; i < 100 && !check(); i++) await new Promise((r) => setTimeout(r, 1)); + expect(check()).toBe(true); +} + +// Pins the oRPC transport facts the link relies on. An oRPC upgrade that changes them fails here. +describe("ConnectionLink over the real oRPC WebSocket link", () => { + function connect() { + const socket = new FakeSocket(); + const inner = new WebSocketLink({ connect: () => socket as unknown as WebSocket }); + const link = new ConnectionLink(); + link.attach(inner); + return { socket, link }; + } + + test("the transport rejects a call with an AbortError when its socket closes", async () => { + const { socket, link } = connect(); + const pending = link.call(["general", "ping"], "x", NO_OPTIONS).catch((e: unknown) => e); + await waitFor(() => socket.sent.length > 0); + socket.emit("close", { code: 1006, reason: "" }); + const error: unknown = await pending; + // Without a detach the generation is live, so the transport's error passes through. + expect(isAbortError(error)).toBe(true); + expect(isConnectionLostError(error)).toBe(false); + }); + + test("detach() first, as the provider's close listener does, gives ConnectionLostError", async () => { + const { socket, link } = connect(); + const pending = link.call(["general", "ping"], "x", NO_OPTIONS).catch((e: unknown) => e); + await waitFor(() => socket.sent.length > 0); + link.detach(); + socket.emit("close", { code: 1006, reason: "" }); + expect(isConnectionLostError(await pending)).toBe(true); + }); + + test("a server cancel on the open socket stays a plain abort", async () => { + const { socket, link } = connect(); + const pending = link.call(["general", "ping"], "x", NO_OPTIONS).catch((e: unknown) => e); + await waitFor(() => socket.sent.length > 0); + const { id } = JSON.parse(socket.sent[0] ?? "{}") as { id: unknown }; + socket.emit("message", { data: JSON.stringify({ id, kind: "cancel" }) }); + const error: unknown = await pending; + expect(isAbortError(error)).toBe(true); + expect(isConnectionLostError(error)).toBe(false); + expect((error as Error).message).toBe("Server canceled the request"); + }); +}); diff --git a/src/browser/contexts/connectionLink.ts b/src/browser/contexts/connectionLink.ts new file mode 100644 index 00000000000..1494a6bbc69 --- /dev/null +++ b/src/browser/contexts/connectionLink.ts @@ -0,0 +1,316 @@ +import type { ClientContext, ClientLink, ClientOptions } from "@orpc/client"; +import { isAsyncIteratorObject } from "@orpc/shared"; + +const CONNECTION_LOST_CODE = "XUM_CONNECTION_LOST"; + +/** + * The connection to the Xum server was lost: the call or stream ran on a socket that closed, or + * no socket was attached when it started. + * + * Named "AbortError" on purpose. It replaces exactly the AbortError that the WebSocket transport + * raises when its socket closes, and every catch site that treats that one as "stop quietly" + * keeps that behavior. Use isConnectionLostError() to tell it apart from a caller abort or a + * server-side cancel, which carry the same name. + */ +export class ConnectionLostError extends Error { + override readonly name = "AbortError" as const; + readonly code = CONNECTION_LOST_CODE; + + constructor(options?: { cause?: unknown }) { + super("Not connected to the Xum server. Reconnecting…", options); + } +} + +export function isConnectionLostError(error: unknown): error is ConnectionLostError { + return ( + error instanceof Error && + (error as { code?: unknown }).code === CONNECTION_LOST_CODE && + error.name === "AbortError" + ); +} + +type Link = ClientLink; + +/** A stream that detach() can end at once, even while its consumer waits on next(). */ +interface LiveIterator { + end(): void; +} + +/** One attached inner link: the calls and streams that started on it. */ +interface Generation { + readonly id: number; + readonly inner: Link; + detached: boolean; + /** Aborted by detach(): settles the inner calls and streams even when no close event comes. */ + readonly abort: AbortController; + readonly pending: Set<(error: ConnectionLostError) => void>; + readonly iterators: Set; +} + +/** + * A ClientLink that forwards each call to the inner link (one per socket) attached now. + * + * The provider owns one ConnectionLink and attaches a new inner link on each successful connect. + * Clients built on it (`createClient(link)`) resolve the link at call time, so a client held in a + * closure keeps working after a reconnect instead of calling into a closed socket, where a new + * call never settles. + * + * Rules: + * - A call made while nothing is attached rejects at once with ConnectionLostError. Calls are + * never queued: a queued write would run later with outdated intent. + * - A rejection becomes ConnectionLostError only when the call's own generation was detached and + * the caller did not abort. A server cancel on an open socket has the same AbortError name, so + * the error name alone cannot tell the two apart. + * - detach() rejects the generation's pending calls and ends its streams itself. A late or + * missing socket `close` event (a forced reconnect, a MessagePort) cannot leave work hanging. + * A result that arrives after detach() is dropped: its caller already got the error. + */ +export class ConnectionLink implements Link { + private current: Generation | null = null; + private nextId = 1; + + get isAttached(): boolean { + return this.current != null; + } + + /** Attach a connected inner link. Returns the new generation number. */ + attach(inner: Link): number { + if (this.current != null) { + throw new Error("ConnectionLink.attach: a link is already attached; detach it first"); + } + const generation: Generation = { + id: this.nextId++, + inner, + detached: false, + abort: new AbortController(), + pending: new Set(), + iterators: new Set(), + }; + this.current = generation; + return generation.id; + } + + /** + * Synchronous and idempotent. With a generation number (from attach()), it detaches only that + * generation: a late `close` event of an old socket must not detach the socket that replaced + * it after a forced reconnect. + */ + detach(generationId?: number): void { + const generation = this.current; + if (generation == null) return; + if (generationId != null && generationId !== generation.id) return; + this.current = null; + generation.detached = true; + const lost = new ConnectionLostError(); + for (const reject of generation.pending) reject(lost); + generation.pending.clear(); + for (const iterator of generation.iterators) iterator.end(); + generation.iterators.clear(); + // The inner link must settle too: a socket that never sends its close event (a half-open + // one, a MessagePort) would otherwise keep every pending request, and the server keeps + // running the abandoned ones. + generation.abort.abort(lost); + } + + call(path: string[], input: unknown, options: ClientOptions): Promise { + if (path.length === 0) { + throw new Error("ConnectionLink.call: empty procedure path"); + } + const signal = options.signal; + // The caller's abort always wins, as with a direct link. + if (signal?.aborted) { + // eslint-disable-next-line @typescript-eslint/prefer-promise-reject-errors -- the caller's own reason, unchanged + return Promise.reject(signal.reason); + } + const generation = this.current; + if (generation == null) { + return Promise.reject(new ConnectionLostError()); + } + const innerSignal = + signal == null ? generation.abort.signal : AbortSignal.any([signal, generation.abort.signal]); + return settleUnlessLost( + generation.inner.call(path, input, { ...options, signal: innerSignal }), + { + waiters: generation.pending, + signal, + mapValue: (output) => + isAsyncIteratorObject(output) + ? new GenerationIterator(output, generation, signal) + : output, + mapError: (error) => toLinkError(error, generation, signal), + // detach() already rejected the caller. A stream that opened late must not leak. + dropLateValue: closeIfStream, + } + ); + } +} + +/** A rejection from a detached generation, unless the caller aborted, means the connection is gone. */ +function toLinkError(error: unknown, generation: Generation, signal: AbortSignal | undefined) { + return generation.detached && !signal?.aborted + ? new ConnectionLostError({ cause: error }) + : error; +} + +function closeIfStream(value: unknown): void { + if (isAsyncIteratorObject(value)) void returnQuietly(value); +} + +/** The inner stream may already be closed: returning it is best effort. */ +async function returnQuietly( + stream: AsyncIteratorObject, + value?: unknown +) { + try { + await stream.return?.(value); + } catch { + // Nothing to do: the stream is gone either way. + } +} + +/** + * Settles with `work`, unless a connection loss is reported first through `waiters` (detach() + * calls every waiter). Used for one-shot calls and for each next() of a stream: in both, the + * caller must get ConnectionLostError at once, even when the inner link never settles. + */ +function settleUnlessLost( + work: Promise, + hooks: { + waiters: Set<(error: ConnectionLostError) => void>; + /** The caller's signal: an abort that is already there wins over the connection loss. */ + signal: AbortSignal | undefined; + mapValue: (value: T) => R; + mapError: (error: unknown) => unknown; + dropLateValue?: (value: T) => void; + } +): Promise { + return new Promise((resolve, reject) => { + let settled = false; + const lose = (error: ConnectionLostError) => { + if (settled) return; + settled = true; + // The caller aborted in the same task, before the inner link's rejection reached us. + // eslint-disable-next-line @typescript-eslint/prefer-promise-reject-errors -- the caller's own reason, unchanged + reject(hooks.signal?.aborted ? hooks.signal.reason : error); + }; + hooks.waiters.add(lose); + work.then( + (value) => { + hooks.waiters.delete(lose); + if (settled) { + hooks.dropLateValue?.(value); + return; + } + settled = true; + resolve(hooks.mapValue(value)); + }, + (error: unknown) => { + hooks.waiters.delete(lose); + if (settled) return; + settled = true; + // eslint-disable-next-line @typescript-eslint/prefer-promise-reject-errors -- the inner error, or ConnectionLostError + reject(hooks.mapError(error)); + } + ); + }); +} + +/** + * A stream of one generation. Values pass through unchanged, so oRPC event meta (attached to the + * value objects) is kept. Once the generation is detached, a waiting next() rejects with + * ConnectionLostError at once, every later next() does too, and the inner stream is returned. + */ +class GenerationIterator implements AsyncIteratorObject, LiveIterator { + private finished = false; + /** Ended by detach(), not by the stream itself: later pulls report the lost connection. */ + private lost = false; + private readonly waiting = new Set<(error: ConnectionLostError) => void>(); + + constructor( + private readonly inner: AsyncIteratorObject, + private readonly generation: Generation, + private readonly signal: AbortSignal | undefined + ) { + generation.iterators.add(this); + } + + /** Called by detach(). */ + end(): void { + const wasFinished = this.finished; + this.finish(); + // Also settles a return() or throw() still waiting for the inner stream's cleanup. + const lost = new ConnectionLostError(); + for (const lose of this.waiting) lose(lost); + this.waiting.clear(); + if (wasFinished) return; + this.lost = true; + void returnQuietly(this.inner); + } + + next(): Promise> { + // A stream that completed (or was returned) stays done, even after a later detach. + // eslint-disable-next-line @typescript-eslint/prefer-promise-reject-errors -- the caller's reason, or ConnectionLostError + if (this.lost) return Promise.reject(this.lossError()); + if (this.finished) return Promise.resolve({ done: true, value: undefined }); + return settleUnlessLost(this.inner.next(), { + waiters: this.waiting, + signal: this.signal, + mapValue: (result) => { + if (result.done) this.finish(); + return result; + }, + mapError: (error) => { + this.finish(); + return toLinkError(error, this.generation, this.signal); + }, + }); + } + + async return(value?: unknown): Promise> { + if (this.finished) return { done: true, value }; + // A lost connection ends the cleanup too: the stream is done for the caller either way. + await this.cleanUp(returnQuietly(this.inner, value)).catch(() => undefined); + return { done: true, value }; + } + + async throw(error?: unknown): Promise> { + if (this.inner.throw == null) { + this.finish(); + throw error; + } + return this.cleanUp(this.inner.throw(error)); + } + + [Symbol.asyncIterator](): this { + return this; + } + + async [Symbol.asyncDispose](): Promise { + await this.return(); + } + + /** The caller's abort wins over the connection loss, as for one-shot calls. */ + private lossError(): unknown { + return this.signal?.aborted ? this.signal.reason : new ConnectionLostError(); + } + + private finish(): void { + this.finished = true; + this.generation.iterators.delete(this); + } + + /** + * Later pulls see a finished stream at once, but the iterator stays in its generation until + * `cleanup` settles: when the socket closed without a close event, the inner cleanup can hang, + * and only detach() can settle it then. + */ + private cleanUp(cleanup: Promise): Promise { + this.finished = true; + return settleUnlessLost(cleanup, { + waiters: this.waiting, + signal: this.signal, + mapValue: (value) => value, + mapError: (error) => toLinkError(error, this.generation, this.signal), + }).finally(() => this.generation.iterators.delete(this)); + } +} diff --git a/src/browser/contexts/subscriptionInventory.test.ts b/src/browser/contexts/subscriptionInventory.test.ts new file mode 100644 index 00000000000..e6f273c4fcb --- /dev/null +++ b/src/browser/contexts/subscriptionInventory.test.ts @@ -0,0 +1,196 @@ +import { describe, expect, test } from "bun:test"; +import { execFileSync } from "node:child_process"; +import { readFileSync } from "node:fs"; +import ts from "typescript"; + +/** + * Every renderer file that opens a server stream (an `eventIterator` procedure) must say how the + * stream starts again after Xum reconnects. A stream dies with its socket. A holder that keeps a + * client outside React deps (a store, a router, a ref) and has no restart rule leaves a dead + * subscription with no error on screen. + * + * A new holder fails this test until its file is listed here with its rule. + */ +type RestartRule = + /** The stream is opened in an effect with `api` in its deps: a new client re-runs it. */ + | "effect-deps" + /** A store fed through setClient(): it drops the client while offline and resubscribes. */ + | "store-live-feed" + /** The terminal router: today the provider builds a new router per client. */ + | "router-per-client" + /** A long-lived holder with its own capped backoff. */ + | "holder-backoff" + /** One operation's progress stream (a clone): it is not restarted, the operation fails. */ + | "operation-stream" + /** Reads the procedure only to check that the server supports it. */ + | "capability-check"; + +const RESTART_RULES: Record = { + "src/browser/App.tsx": "effect-deps", + "src/browser/components/OutputTab/OutputTab.tsx": "effect-deps", + "src/browser/components/PerfFlightRecorder/PerfFlightRecorder.tsx": "effect-deps", + "src/browser/components/ProjectCreateModal/ProjectCreateModal.tsx": "operation-stream", + "src/browser/components/ProjectPage/ProjectPage.tsx": "effect-deps", + "src/browser/components/SshPromptDialog/SshPromptDialog.tsx": "effect-deps", + "src/browser/components/TerminalView/TerminalView.tsx": "effect-deps", + "src/browser/components/TitleBar/TitleBar.tsx": "effect-deps", + "src/browser/components/UpdateRestartOverlay/UpdateRestartOverlay.tsx": "effect-deps", + "src/browser/contexts/ProjectContext.tsx": "effect-deps", + "src/browser/contexts/WorkspaceContext.tsx": "effect-deps", + "src/browser/features/About/AboutDialog.tsx": "effect-deps", + "src/browser/features/ChatInput/index.tsx": "effect-deps", + "src/browser/features/Memory/MemoryBrowser.tsx": "effect-deps", + "src/browser/features/RightSidebar/DevToolsTab/useDevToolsSubscription.ts": "effect-deps", + "src/browser/features/RightSidebar/Workflows/useWorkflowRuns.ts": "effect-deps", + "src/browser/features/Settings/Sections/BackupSection.tsx": "effect-deps", + "src/browser/features/Settings/Sections/GeneralSection.tsx": "effect-deps", + "src/browser/features/Tools/WorkflowRunToolCall.tsx": "capability-check", + "src/browser/features/desktop/useDesktopConnection.ts": "holder-backoff", + "src/browser/hooks/useAutoModelRouting.ts": "effect-deps", + "src/browser/hooks/useClaudeDesign.ts": "effect-deps", + "src/browser/hooks/useComputerUse.ts": "effect-deps", + "src/browser/hooks/useModelFallbacks.ts": "effect-deps", + "src/browser/stores/AppConfigStore.ts": "store-live-feed", + "src/browser/stores/BackgroundBashStore.ts": "store-live-feed", + "src/browser/stores/DraftStore.ts": "store-live-feed", + "src/browser/stores/ProvidersConfigStore.ts": "store-live-feed", + "src/browser/stores/ReviewStateStore.ts": "store-live-feed", + "src/browser/stores/WorkspaceStore.ts": "store-live-feed", + "src/browser/terminal/TerminalSessionRouter.ts": "router-per-client", +}; + +/** + * The code shape each rule needs, so a listed file cannot claim a rule its code does not follow. + * These are structural checks; the reconnect tests of each holder prove the behavior. + */ +const RULE_SHAPES: Record boolean }> = { + "effect-deps": { + describe: "a useEffect with `api` in its dependency list", + matches: (text) => text.includes("useEffect(") && /\[[^\]]*\bapi\b[^\]]*\]\s*\)/.test(text), + }, + "store-live-feed": { + describe: "a setClient(client) method that the app feeds with the live client", + matches: (text) => /\bsetClient\s*\(\s*\w+\s*:/.test(text), + }, + "router-per-client": { + describe: "a router bound to one client (the provider builds one per client)", + matches: (text) => /constructor\(api: APIClient\b/.test(text), + }, + "holder-backoff": { + describe: "a retry with a delay", + matches: (text) => text.includes("setTimeout(") && /backoff|retry/i.test(text), + }, + "operation-stream": { + describe: "one operation's stream (no restart by design)", + matches: () => true, + }, + "capability-check": { + describe: "a typeof check of the procedure", + matches: (text) => /typeof [^\n]*=== "function"/.test(text), + }, +}; + +const SCHEMA_PATH = "src/common/orpc/schemas/api.ts"; + +/** Paths of every procedure whose output is `eventIterator(...)`, e.g. ["terminal", "onExit"]. */ +function streamProcedures(): string[][] { + const source = ts.createSourceFile( + SCHEMA_PATH, + readFileSync(SCHEMA_PATH, "utf8"), + ts.ScriptTarget.Latest, + true + ); + const paths: string[][] = []; + const isStreamOutput = (property: ts.ObjectLiteralElementLike) => + ts.isPropertyAssignment(property) && + property.name.getText(source) === "output" && + ts.isCallExpression(property.initializer) && + property.initializer.expression.getText(source) === "eventIterator"; + const walk = (object: ts.ObjectLiteralExpression, prefix: string[]) => { + for (const property of object.properties) { + if (!ts.isPropertyAssignment(property)) continue; + if (!ts.isObjectLiteralExpression(property.initializer)) continue; + const path = [...prefix, property.name.getText(source)]; + if (property.initializer.properties.some(isStreamOutput)) paths.push(path); + else walk(property.initializer, path); + } + }; + for (const statement of source.statements) { + if (!ts.isVariableStatement(statement)) continue; + for (const declaration of statement.declarationList.declarations) { + if (declaration.initializer && ts.isObjectLiteralExpression(declaration.initializer)) { + walk(declaration.initializer, [declaration.name.getText(source)]); + } + } + } + return paths; +} + +const escape = (text: string) => text.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); + +/** + * Matches a read of the procedure: `api.terminal.onExit`, `api?.config?.onConfigChanged`, across + * line breaks. A procedure whose name is not generic also matches alone, because callers read it + * through a local first (`configApi?.onConfigChanged`). + */ +function accessPattern(path: string[]): RegExp { + const separator = "\\s*\\??\\.\\s*"; + const full = "\\??\\." + "\\s*" + path.map(escape).join(separator) + "\\b"; + const leaf = path[path.length - 1]; + // Names that are also common props or methods (`onChange`, `onExit`) match only as a full path. + const generic = [ + "subscribe", + "attach", + "clone", + "onChange", + "onExit", + "onStatus", + "onOutput", + "onOpenSettings", + ]; + return generic.includes(leaf) + ? new RegExp(full) + : new RegExp(`${full}|\\??\\.\\s*${escape(leaf)}\\b`); +} + +function rendererSources(): string[] { + return execFileSync("git", ["ls-files", "src/browser", "vscode/src/webview"], { + encoding: "utf8", + }) + .split("\n") + .filter((file) => /\.tsx?$/.test(file)) + .filter((file) => !/\.(test|stories|testHarness)\.tsx?$/.test(file)) + .filter((file) => !file.includes("/stories/") && file !== "src/browser/testUtils.ts"); +} + +describe("server stream holders", () => { + const procedures = streamProcedures(); + const patterns = procedures.map(accessPattern); + const holders = rendererSources().filter((file) => { + const text = readFileSync(file, "utf8"); + return patterns.some((pattern) => pattern.test(text)); + }); + + test("the schema scan finds the stream procedures", () => { + // A guard for the scan itself: if it finds nothing, every other check passes vacuously. + expect(procedures.length).toBeGreaterThan(20); + expect(procedures).toContainEqual(["terminal", "onExit"]); + }); + + test("every file that opens a server stream has a restart rule", () => { + const unlisted = holders.filter((file) => !(file in RESTART_RULES)); + expect(unlisted).toEqual([]); + }); + + test("every listed file has the code shape of its rule", () => { + const mismatched = Object.entries(RESTART_RULES) + .filter(([file, rule]) => !RULE_SHAPES[rule].matches(readFileSync(file, "utf8"))) + .map(([file, rule]) => `${file}: "${rule}" needs ${RULE_SHAPES[rule].describe}`); + expect(mismatched).toEqual([]); + }); + + test("every listed file still opens a server stream", () => { + const stale = Object.keys(RESTART_RULES).filter((file) => !holders.includes(file)); + expect(stale).toEqual([]); + }); +}); diff --git a/src/browser/features/RightSidebar/ArtifactsTab/ArtifactsReconnect.test.tsx b/src/browser/features/RightSidebar/ArtifactsTab/ArtifactsReconnect.test.tsx index bb7739b915a..84777ede690 100644 --- a/src/browser/features/RightSidebar/ArtifactsTab/ArtifactsReconnect.test.tsx +++ b/src/browser/features/RightSidebar/ArtifactsTab/ArtifactsReconnect.test.tsx @@ -3,11 +3,16 @@ import "../../../../../tests/ui/dom"; import { afterEach, beforeEach, describe, expect, test } from "bun:test"; import { act, cleanup, fireEvent, render, waitFor } from "@testing-library/react"; -import { useState, type ReactNode } from "react"; +import type { ReactNode } from "react"; import { installDom } from "../../../../../tests/ui/dom"; -import { APIContext, type APIClient, type UseAPIResult } from "@/browser/contexts/API"; +import type { APIClient } from "@/browser/contexts/API"; +import { + createConnectionHarness, + GAP_MODES, + type ConnectionHarness, +} from "@/browser/contexts/connection.testHarness"; import { ThemeProvider } from "@/browser/contexts/ThemeContext"; -import { createTestApiClient, type TestApiOverrides } from "@/browser/testUtils"; +import type { TestApiOverrides } from "@/browser/testUtils"; import type { ArtifactReadResult } from "@/common/orpc/schemas/artifacts"; import { getArtifactKind } from "@/common/utils/artifactKind"; import { ArtifactsPanel } from "./ArtifactsPanel"; @@ -68,8 +73,8 @@ function fail() { return Promise.resolve({ success: false as const, error: failWith ?? "" }); } -/** A fresh client, as each reconnect creates. */ -function createClient(): APIClient { +/** The server's procedures. The harness builds a fresh client on them at each connect. */ +function serverOverrides(): TestApiOverrides { const api: TestApiOverrides = { artifacts: { list: () => { @@ -164,30 +169,16 @@ function createClient(): APIClient { }, }, }; - return createTestApiClient(api); + return api; } -let setConnection: (api: APIClient | null) => void = () => undefined; -const noop = () => undefined; +let harness: ConnectionHarness; -/** The API context as ManagedAPIProvider reports it: connected, or reconnecting with no client. */ function ConnectionWrapper(props: { children: ReactNode }) { - const [api, setApi] = useState(() => createClient()); - setConnection = setApi; - const value: UseAPIResult = - api != null - ? { status: "connected", api, error: null, authenticate: noop, retry: noop } - : { - status: "reconnecting", - api: null, - error: null, - attempt: 1, - authenticate: noop, - retry: noop, - }; + const Connection = harness.Wrapper; return ( - {props.children} + {props.children} ); } @@ -200,14 +191,14 @@ async function flush() { } async function dropConnection() { - act(() => setConnection(null)); + await harness.drop(); await flush(); } /** Reconnects and waits until the panel re-listed through the new client. */ async function reconnect() { const before = listCalls; - act(() => setConnection(createClient())); + harness.reconnect(); await waitFor(() => expect(listCalls).toBeGreaterThan(before)); await flush(); } @@ -228,10 +219,12 @@ function postFromFrame(frame: HTMLIFrameElement, data: unknown) { }); } -describe("Artifacts tab across a dropped connection", () => { +// Every case runs with the provider of today (null-gap) and after the switch (kept-gap). +describe.each([...GAP_MODES])("Artifacts tab across a dropped connection (%s)", (mode) => { let cleanupDom: (() => void) | null = null; beforeEach(() => { + harness = createConnectionHarness(mode, serverOverrides); cleanupDom = installDom(); window.localStorage.clear(); files = { "app.html": ok("app.html", "") }; @@ -334,7 +327,7 @@ describe("Artifacts tab across a dropped connection", () => { }) as Window["postMessage"]; await dropConnection(); - act(() => setConnection(createClient())); + harness.reconnect(); await flush(); expectSameFrame(view.queryByTestId("mcp-app-frame"), frame); // A torn-down host would have told the view to tear down. @@ -394,7 +387,10 @@ describe("Artifacts tab across a dropped connection", () => { expect(savedStates).toEqual([]); // Reopening the tab (connected again) sends the held write before reading the state. - const reopened = render(, { wrapper: ConnectionWrapper }); + harness.reconnect(); + const reopened = render(, { + wrapper: ConnectionWrapper, + }); await reopened.findByTestId("artifact-frame"); await waitFor(() => expect(savedStates).toEqual([{ marker: "zq-offline" }])); await waitFor(() => @@ -450,7 +446,7 @@ describe("Artifacts tab across a dropped connection", () => { expect(view.getByText("Q")).toBeTruthy(); expect(assetReads.length).toBe(reads); - act(() => setConnection(createClient())); + harness.reconnect(); await waitFor(() => expect(assetReads.length).toBe(reads + 1)); await flush(); expect(view.queryByText(/Could not read/)).toBeNull(); @@ -487,7 +483,7 @@ describe("Artifacts tab across a dropped connection", () => { expect(view.queryByText(/Could not read/)).toBeNull(); await dropConnection(); - act(() => setConnection(createClient())); + harness.reconnect(); await waitFor(() => expect(assetReads).toEqual(["data/s.json", "data/s.json"])); expect(await view.findByText("Q")).toBeTruthy(); expect(view.queryByText(/Could not read/)).toBeNull(); diff --git a/src/browser/features/Tools/WorkflowRunToolCall.tsx b/src/browser/features/Tools/WorkflowRunToolCall.tsx index fa5b0e5d45b..0319a35cfdc 100644 --- a/src/browser/features/Tools/WorkflowRunToolCall.tsx +++ b/src/browser/features/Tools/WorkflowRunToolCall.tsx @@ -1644,10 +1644,11 @@ export const WorkflowRunToolCall: React.FC = ({ return; } + const api = apiState.api; let ignore = false; const discover = async () => { try { - const runs = await apiState.api.workflows.listRuns({ workspaceId }); + const runs = await api.workflows.listRuns({ workspaceId }); const foregroundRun = findForegroundWorkflowRun({ runs, args: launchArgs, @@ -1687,10 +1688,11 @@ export const WorkflowRunToolCall: React.FC = ({ return; } + const api = apiState.api; let ignore = false; const discover = async () => { try { - const nextRun = await apiState.api.workflows.getRun({ + const nextRun = await api.workflows.getRun({ workspaceId, runId: exactDiscoveryRunId, }); @@ -1741,10 +1743,11 @@ export const WorkflowRunToolCall: React.FC = ({ return; } + const api = apiState.api; let ignore = false; const refresh = async () => { try { - const nextRun = await apiState.api.workflows.getRun({ + const nextRun = await api.workflows.getRun({ workspaceId: workflowWorkspaceId, runId, }); @@ -1776,10 +1779,11 @@ export const WorkflowRunToolCall: React.FC = ({ return; } + const api = apiState.api; let ignore = false; const refresh = async () => { try { - const nextRun = await apiState.api.workflows.getRun({ + const nextRun = await api.workflows.getRun({ workspaceId: run.workspaceId, runId, }); diff --git a/src/browser/hooks/useDiscardSideChatOnLeave.test.tsx b/src/browser/hooks/useDiscardSideChatOnLeave.test.tsx new file mode 100644 index 00000000000..f6ed6d63875 --- /dev/null +++ b/src/browser/hooks/useDiscardSideChatOnLeave.test.tsx @@ -0,0 +1,65 @@ +import "../../../tests/ui/dom"; + +import { afterEach, beforeEach, describe, expect, test } from "bun:test"; +import { cleanup, render, waitFor } from "@testing-library/react"; +import { installDom } from "../../../tests/ui/dom"; +import { useLiveAPI } from "@/browser/contexts/API"; +import { createConnectionHarness, GAP_MODES } from "@/browser/contexts/connection.testHarness"; +import type { FrontendWorkspaceMetadata } from "@/common/types/workspace"; +import { createWorkspace } from "@/browser/stories/mocks/workspaces"; +import { useDiscardSideChatOnLeave } from "./useDiscardSideChatOnLeave"; + +let removed: string[] = []; + +const serverOverrides = () => ({ + workspace: { + remove: (input: { workspaceId: string }) => { + removed.push(input.workspaceId); + return Promise.resolve({ success: true as const }); + }, + }, +}); + +const metadata: ReadonlyMap = new Map([ + ["main", createWorkspace({ id: "main", name: "main", projectName: "proj" })], + [ + "side", + { + ...createWorkspace({ id: "side", name: "side", projectName: "proj" }), + sideChatParentWorkspaceId: "main", + }, + ], +]); + +/** Wires the hook the way WorkspaceContext does: it gets the live client. */ +function Probe(props: { currentWorkspaceId: string }) { + useDiscardSideChatOnLeave(useLiveAPI(), props.currentWorkspaceId, metadata); + return null; +} + +describe.each([...GAP_MODES])("useDiscardSideChatOnLeave (%s)", (mode) => { + let cleanupDom: (() => void) | null = null; + beforeEach(() => { + cleanupDom = installDom(); + removed = []; + }); + afterEach(() => { + cleanup(); + cleanupDom?.(); + cleanupDom = null; + }); + + test("a side chat left while offline is discarded once the connection is back", async () => { + const harness = createConnectionHarness(mode, serverOverrides); + const view = render(, { wrapper: harness.Wrapper }); + + await harness.drop(); + view.rerender(); + await new Promise((resolve) => setTimeout(resolve, 10)); + // Nothing is sent to a server that is not there: the discard waits. + expect(removed).toEqual([]); + + harness.reconnect(); + await waitFor(() => expect(removed).toEqual(["side"])); + }); +});