From a0955be6813adee3ca8a3ed2154dc8eb8fe30237 Mon Sep 17 00:00:00 2001 From: Ehab Younes Date: Mon, 20 Apr 2026 17:39:47 +0300 Subject: [PATCH 1/6] refactor: extract shared webview IPC helpers with enforced exhaustiveness - Add isIpcCommand/Request, notifyWebview, dispatchCommand/Request, and onWhileVisible in src/webviews/util.ts; migrate tasksPanelProvider and speedtestPanel off their local copies. - Add sendCommand/onNotification in @repo/webview-shared for vanilla webviews; useIpc stays for React. - speedtestPanel calls buildCommandHandlers AND buildRequestHandlers, so adding a new action to SpeedtestApi is a compile error until a handler is wired. - Document the contract and the visibility/theme re-send guarantee in packages/webview-shared/README.md; AGENTS.md and CONTRIBUTING.md point at it. - Expand speedtestPanel tests (payload, visibility/theme re-send, viewJson dispatch, cleanup) and add ipc.test.ts for the new helpers. --- AGENTS.md | 15 ++ CONTRIBUTING.md | 39 ++--- packages/speedtest/src/index.ts | 6 +- packages/webview-shared/README.md | 137 ++++++++++++++++++ packages/webview-shared/src/index.ts | 4 +- packages/webview-shared/src/ipc.ts | 43 ++++++ packages/webview-shared/src/notifications.ts | 20 --- packages/webview-shared/src/react/useIpc.ts | 6 +- .../speedtest/speedtestPanelFactory.ts | 60 ++++---- src/webviews/tasks/tasksPanelProvider.ts | 100 +++---------- src/webviews/util.ts | 128 ++++++++++++++++ test/webview/shared/ipc.test.ts | 120 +++++++++++++++ 12 files changed, 515 insertions(+), 163 deletions(-) create mode 100644 packages/webview-shared/README.md create mode 100644 packages/webview-shared/src/ipc.ts delete mode 100644 packages/webview-shared/src/notifications.ts create mode 100644 test/webview/shared/ipc.test.ts diff --git a/AGENTS.md b/AGENTS.md index 752eb913..97d31775 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -79,6 +79,21 @@ test/ └── mocks/ # Shared test mocks ``` +## Webviews + +When adding or modifying a panel, follow `packages/webview-shared/README.md`. +It is the single source of truth for the IPC contract, exhaustive handler +maps, and the visibility/theme re-send guarantee. + +Non-negotiables: + +- Never hand-roll `window.addEventListener("message", ...)` or + `postMessage({ method, params })`. Use `onNotification` / `sendCommand` + (vanilla) or `useIpc` (React) from `@repo/webview-shared`. +- Extension panels must call **both** `buildCommandHandlers` and + `buildRequestHandlers` (empty `{}` is fine). This gives a compile error + when anyone adds an action to the API without a matching handler. + ## Code Style - TypeScript with strict typing diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index f81a2f06..32b8b5c0 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -74,28 +74,18 @@ that are close to shutting down. ## Webviews -The extension uses React-based webviews for rich UI panels, built with Vite and -organized as a pnpm workspace in `packages/`. +The extension ships rich UI panels as webviews built with Vite, organized as a +pnpm workspace in `packages/`. The canonical guide for building one covers +the IPC contract, exhaustiveness rules, the "no dropped events" guarantee, +and a new-panel checklist. It lives next to the code: -### Project Structure +**[`packages/webview-shared/README.md`](packages/webview-shared/README.md)** -```text -packages/ -├── webview-shared/ # Shared types, React hooks, and Vite config -│ └── extension.d.ts # Types exposed to extension (excludes React) -└── tasks/ # Example webview (copy this for new webviews) - -src/webviews/ -├── util.ts # getWebviewHtml() helper -└── tasks/ # Extension-side provider for tasks panel -``` - -Key patterns: +Existing webviews as references: -- **Type sharing**: Extension imports types from `@repo/webview-shared` via path mapping - to `extension.d.ts`. Webviews import directly from `@repo/webview-shared/react`. -- **Message passing**: Use `postMessage()`/`useMessage()` hooks for communication. -- **Lifecycle**: Dispose event listeners properly (see `TasksPanel.ts` for example). +- `packages/tasks` + `src/webviews/tasks/`: React (uses `useIpc`). +- `packages/speedtest` + `src/webviews/speedtest/`: vanilla TS (uses + `onNotification` / `sendCommand`). ### Development @@ -103,15 +93,8 @@ Key patterns: pnpm watch # Rebuild extension and webviews on changes ``` -Press F5 to launch the Extension Development Host. Use "Developer: Reload Webviews" -to see webview changes. - -### Adding a New Webview - -1. Copy `packages/tasks` to `packages/` and update the package name -2. Create a provider in `src/webviews//` (see `TasksPanel.ts` for reference) -3. Register the view in `package.json` under `contributes.views` -4. Register the provider in `src/extension.ts` +Press F5 to launch the Extension Development Host. Use "Developer: Reload +Webviews" to see webview changes. ## Testing diff --git a/packages/speedtest/src/index.ts b/packages/speedtest/src/index.ts index da5c4f7d..d90879d4 100644 --- a/packages/speedtest/src/index.ts +++ b/packages/speedtest/src/index.ts @@ -1,5 +1,5 @@ import { SpeedtestApi, type SpeedtestResult, toError } from "@repo/shared"; -import { postMessage, subscribeNotification } from "@repo/webview-shared"; +import { onNotification, sendCommand } from "@repo/webview-shared"; import { renderLineChart } from "./chart"; import { @@ -20,11 +20,11 @@ const TOOLTIP_GAP_PX = 32; let cleanup: (() => void) | undefined; function main(): void { - subscribeNotification(SpeedtestApi.data, ({ workspaceId, result }) => { + onNotification(SpeedtestApi.data, ({ workspaceId, result }) => { try { cleanup?.(); cleanup = renderPage(result, workspaceId, () => - postMessage({ method: SpeedtestApi.viewJson.method }), + sendCommand(SpeedtestApi.viewJson), ); } catch (err) { showError(`Failed to render speedtest: ${toError(err).message}`); diff --git a/packages/webview-shared/README.md b/packages/webview-shared/README.md new file mode 100644 index 00000000..2f20e1ce --- /dev/null +++ b/packages/webview-shared/README.md @@ -0,0 +1,137 @@ +# Webview IPC + +Shared primitives for typed, reliable messaging between the extension host +and VS Code webviews. Both sides use the **same `Api` definition object** so +wire formats can't drift and method-name typos are caught at compile time. + +## Message kinds + +```ts +// packages/shared/src/ipc/protocol.ts +defineNotification(method); // extension to webview, fire-and-forget +defineCommand

(method); // webview to extension, fire-and-forget +defineRequest(method); // webview to extension, awaits response +``` + +Define all three together in one `Api` const so both sides import the exact +same method strings: + +```ts +// packages/shared/src/myfeature/api.ts +export const MyFeatureApi = { + data: defineNotification("myfeature/data"), + doThing: defineCommand<{ id: string }>("myfeature/doThing"), + getThings: defineRequest("myfeature/getThings"), +} as const; +``` + +## Extension side (`src/webviews/...`) + +### Sending notifications + +```ts +import { notifyWebview } from "../util"; + +notifyWebview(panel.webview, MyFeatureApi.data, payload); +``` + +### Handling incoming messages (exhaustiveness) + +Every panel **must** build handler maps with `buildCommandHandlers` and +`buildRequestHandlers`, even if it has zero requests today. Both builders +are mapped over the `Api` definition, so adding a new `defineCommand` or +`defineRequest` entry **produces a compile error** at the panel that forgot +to wire a handler. This makes it impossible to ship an API surface that +the extension silently drops. + +```ts +import { buildCommandHandlers, buildRequestHandlers } from "@repo/shared"; +import { + dispatchCommand, + dispatchRequest, + isIpcCommand, + isIpcRequest, +} from "../util"; + +const commandHandlers = buildCommandHandlers(MyFeatureApi, { + doThing: async (p) => { ... }, +}); +// Empty is fine; it still enforces future additions. +const requestHandlers = buildRequestHandlers(MyFeatureApi, { + getThings: async () => this.fetchThings(), +}); + +panel.webview.onDidReceiveMessage((message: unknown) => { + if (isIpcRequest(message)) { + void dispatchRequest(message, requestHandlers, panel.webview, { + logger, + showErrorToUser: (m) => USER_ACTION_METHODS.has(m), + }); + } else if (isIpcCommand(message)) { + void dispatchCommand(message, commandHandlers, { logger }); + } +}); +``` + +**Error semantics** (built into `dispatchCommand` / `dispatchRequest`): + +| | Logs | Response sent? | `showErrorMessage` default | +| ------------- | ------------- | ----------------------------- | -------------------------- | +| Command fails | `logger.warn` | n/a | **yes** (user action) | +| Request fails | `logger.warn` | `success: false` with `error` | **no** (often background) | + +Pass `showErrorToUser: (method) => …` to override per-method. + +## Webview side, vanilla (`@repo/webview-shared`) + +```ts +import { MyFeatureApi } from "@repo/shared"; +import { onNotification, sendCommand } from "@repo/webview-shared"; + +// Subscribe to pushes. +const unsubscribe = onNotification(MyFeatureApi.data, (payload) => { + render(payload); +}); + +// Fire a command. +sendCommand(MyFeatureApi.doThing, { id: "42" }); +``` + +`onNotification` returns an unsubscribe function; call it on cleanup. + +## Webview side, React + +Use `useIpc` (`packages/webview-shared/src/react/useIpc`). Same semantics +plus request/response correlation with timeout and UUID bookkeeping. + +## The "no dropped events" guarantee + +Webview contexts are **destroyed when hidden** unless +`retainContextWhenHidden` is set (costly; avoid). This means the webview's +in-memory state, event listeners, and canvas pixels are lost whenever the +user switches tabs. + +Every webview that pushes state from the extension must therefore **re-send +on these signals**, so the revived webview isn't left empty or stale: + +1. **Visibility change**: `panel.onDidChangeViewState(() => panel.visible && resend())`. + The webview was just re-created from HTML, so no in-memory state remains. +2. **Color theme change**: `vscode.window.onDidChangeActiveColorTheme(() => panel.visible && resend())`. + DOM elements update via CSS vars, but canvas and SVG drawn imperatively + do not: pixels are baked in. The webview must redraw against the new + theme. + +The `onWhileVisible(panel, event, handler)` helper in `src/webviews/util.ts` +wraps both cases. Both disposables must be collected and disposed in +`panel.onDidDispose` so they don't leak when the user closes the tab. + +See `src/webviews/speedtest/speedtestPanel.ts` for a minimal reference. + +## Checklist for a new webview + +1. Define the API in `packages/shared/src//api.ts` and export from `packages/shared/src/index.ts`. +2. Extension side: `buildCommandHandlers` **and** `buildRequestHandlers` (empty `{}` is fine; both are needed for exhaustiveness). +3. Extension side: dispatch through `isIpcRequest` to `dispatchRequest` and `isIpcCommand` to `dispatchCommand`, both with a logger. +4. Extension side: use `onWhileVisible` for `onDidChangeViewState` and `onDidChangeActiveColorTheme`, dispose in `onDidDispose`. +5. Webview side: use `onNotification` / `sendCommand` (vanilla) or `useIpc` (React). Never hand-roll `window.addEventListener("message", ...)`. +6. Tests: verify the panel posts the expected payload shape, re-sends on visibility and theme, and handles incoming commands. diff --git a/packages/webview-shared/src/index.ts b/packages/webview-shared/src/index.ts index 334b27df..01d83d22 100644 --- a/packages/webview-shared/src/index.ts +++ b/packages/webview-shared/src/index.ts @@ -8,5 +8,5 @@ export interface WebviewMessage { // VS Code state API export { getState, setState, postMessage } from "./api"; -// Notification subscription for non-React webviews -export { subscribeNotification } from "./notifications"; +// Typed IPC helpers for vanilla webviews +export { sendCommand, onNotification } from "./ipc"; diff --git a/packages/webview-shared/src/ipc.ts b/packages/webview-shared/src/ipc.ts new file mode 100644 index 00000000..34f171ee --- /dev/null +++ b/packages/webview-shared/src/ipc.ts @@ -0,0 +1,43 @@ +/** + * Typed IPC helpers for vanilla-TS webviews. React webviews should use + * `useIpc` (./react/useIpc), which adds request/response correlation. + */ + +import { postMessage } from "./api"; + +import type { CommandDef, NotificationDef } from "@repo/shared"; + +/** Send a fire-and-forget command to the extension. */ +export function sendCommand

( + def: CommandDef

, + ...args: P extends void ? [] : [params: P] +): void { + postMessage({ + method: def.method, + params: args[0], + }); +} + +/** + * Subscribe to a typed notification from the extension. Returns an + * unsubscribe function; call it on cleanup. Multiple subscribers are + * invoked in registration order. + */ +export function onNotification( + def: NotificationDef, + callback: (data: D) => void, +): () => void { + const handler = (event: MessageEvent) => { + const msg = event.data; + if ( + typeof msg !== "object" || + msg === null || + (msg as { type?: unknown }).type !== def.method + ) { + return; + } + callback((msg as { data: D }).data); + }; + window.addEventListener("message", handler); + return () => window.removeEventListener("message", handler); +} diff --git a/packages/webview-shared/src/notifications.ts b/packages/webview-shared/src/notifications.ts deleted file mode 100644 index e71877b6..00000000 --- a/packages/webview-shared/src/notifications.ts +++ /dev/null @@ -1,20 +0,0 @@ -import type { NotificationDef } from "@repo/shared"; - -/** Subscribe to an extension notification. Returns an unsubscribe function. */ -export function subscribeNotification( - def: NotificationDef, - callback: (data: D) => void, -): () => void { - const handler = (event: MessageEvent) => { - const msg = event.data as { type?: string; data?: D } | undefined; - if (!msg || typeof msg !== "object") { - return; - } - if (msg.type !== def.method || msg.data === undefined) { - return; - } - callback(msg.data); - }; - window.addEventListener("message", handler); - return () => window.removeEventListener("message", handler); -} diff --git a/packages/webview-shared/src/react/useIpc.ts b/packages/webview-shared/src/react/useIpc.ts index 9fe994b3..c6c0d0ed 100644 --- a/packages/webview-shared/src/react/useIpc.ts +++ b/packages/webview-shared/src/react/useIpc.ts @@ -6,7 +6,7 @@ import { useEffect, useRef } from "react"; import { postMessage } from "../api"; -import { subscribeNotification } from "../notifications"; +import { onNotification as subscribe } from "../ipc"; import type { CommandDef, @@ -61,7 +61,7 @@ export function useIpc(options: UseIpcOptions = {}) { }, []); // Request/response correlation lives here. Notifications are routed via - // the shared subscribeNotification helper (see onNotification below). + // the shared onNotification helper (see the method below). useEffect(() => { const handler = (event: MessageEvent) => { const msg = event.data as IpcResponse | undefined; @@ -143,7 +143,7 @@ export function useIpc(options: UseIpcOptions = {}) { definition: NotificationDef, callback: (data: D) => void, ): () => void { - return subscribeNotification(definition, callback); + return subscribe(definition, callback); } return { request, command, onNotification }; diff --git a/src/webviews/speedtest/speedtestPanelFactory.ts b/src/webviews/speedtest/speedtestPanelFactory.ts index 7c5207b3..4713ee79 100644 --- a/src/webviews/speedtest/speedtestPanelFactory.ts +++ b/src/webviews/speedtest/speedtestPanelFactory.ts @@ -2,12 +2,21 @@ import * as vscode from "vscode"; import { buildCommandHandlers, - type SpeedtestData, + buildRequestHandlers, SpeedtestApi, + type SpeedtestData, type SpeedtestResult, } from "@repo/shared"; -import { getWebviewHtml } from "../util"; +import { + dispatchCommand, + dispatchRequest, + getWebviewHtml, + isIpcCommand, + isIpcRequest, + notifyWebview, + onWhileVisible, +} from "../util"; import type { Logger } from "../../logging/logger"; @@ -60,16 +69,10 @@ export class SpeedtestPanelFactory { // or theme change to rehydrate and redraw. const payload: SpeedtestData = { workspaceId, result }; const sendData = () => - panel.webview.postMessage({ - type: SpeedtestApi.data.method, - data: payload, - }); - const sendIfVisible = () => { - if (panel.visible) { - sendData(); - } - }; + notifyWebview(panel.webview, SpeedtestApi.data, payload); + // Both builders emit a compile error if any command or request in the + // API lacks a handler here; the empty `{}` below is still load-bearing. const commandHandlers = buildCommandHandlers(SpeedtestApi, { // Webview signals it's subscribed; safe to push the payload now. ready: () => { @@ -90,23 +93,30 @@ export class SpeedtestPanelFactory { } }, }); + const requestHandlers = buildRequestHandlers(SpeedtestApi, {}); + const logger = this.logger; const disposables: vscode.Disposable[] = [ - panel.onDidChangeViewState(sendIfVisible), - vscode.window.onDidChangeActiveColorTheme(sendIfVisible), - panel.webview.onDidReceiveMessage( - (message: { method: string; params?: unknown }) => { - const handler = commandHandlers[message.method]; - if (handler) { - Promise.resolve(handler(message.params)).catch((err: unknown) => { - this.logger.error( - `Unhandled error in speedtest handler for '${message.method}'`, - err, - ); - }); - } - }, + onWhileVisible(panel, panel.onDidChangeViewState, sendData), + onWhileVisible( + panel, + vscode.window.onDidChangeActiveColorTheme, + sendData, ), + panel.webview.onDidReceiveMessage((message: unknown) => { + if (isIpcRequest(message)) { + void dispatchRequest(message, requestHandlers, panel.webview, { + logger, + }); + } else if (isIpcCommand(message)) { + void dispatchCommand(message, commandHandlers, { logger }); + } else { + logger.warn( + "Ignoring unrecognized speedtest webview message", + message, + ); + } + }), ]; panel.onDidDispose(() => { for (const d of disposables) { diff --git a/src/webviews/tasks/tasksPanelProvider.ts b/src/webviews/tasks/tasksPanelProvider.ts index 6680e857..e7ce3679 100644 --- a/src/webviews/tasks/tasksPanelProvider.ts +++ b/src/webviews/tasks/tasksPanelProvider.ts @@ -12,8 +12,6 @@ import { isStableTask, TasksApi, type CreateTaskParams, - type IpcRequest, - type IpcResponse, type NotificationDef, type TaskDetails, type TaskLogs, @@ -27,10 +25,16 @@ import { streamAgentLogs, streamBuildLogs, } from "../../api/workspace"; -import { toError } from "../../error/errorUtils"; import { type Logger } from "../../logging/logger"; import { vscodeProposed } from "../../vscodeProposed"; -import { getWebviewHtml } from "../util"; +import { + dispatchCommand, + dispatchRequest, + getWebviewHtml, + isIpcCommand, + isIpcRequest, + notifyWebview, +} from "../util"; import type { Preset, @@ -48,31 +52,6 @@ function getTaskBuildUrl(baseUrl: string, task: Task): string { return `${baseUrl}/tasks/${task.owner_name}/${task.id}`; } -/** Check if message is a request (has requestId) */ -function isIpcRequest(msg: unknown): msg is IpcRequest { - return ( - typeof msg === "object" && - msg !== null && - "requestId" in msg && - typeof (msg as IpcRequest).requestId === "string" && - "method" in msg && - typeof (msg as IpcRequest).method === "string" - ); -} - -/** Check if message is a command (has method but no requestId) */ -function isIpcCommand( - msg: unknown, -): msg is { method: string; params?: unknown } { - return ( - typeof msg === "object" && - msg !== null && - !("requestId" in msg) && - "method" in msg && - typeof (msg as { method: string }).method === "string" - ); -} - export class TasksPanelProvider implements vscode.WebviewViewProvider, vscode.Disposable { @@ -183,53 +162,15 @@ export class TasksPanelProvider private async handleMessage(message: unknown): Promise { if (isIpcRequest(message)) { - await this.handleRequest(message); + await dispatchRequest(message, this.requestHandlers, this.view?.webview, { + logger: this.logger, + showErrorToUser: (method) => + TasksPanelProvider.USER_ACTION_METHODS.has(method), + }); } else if (isIpcCommand(message)) { - await this.handleCommand(message); - } - } - - private async handleRequest(message: IpcRequest): Promise { - const { requestId, method, params } = message; - - try { - const handler = this.requestHandlers[method]; - if (!handler) { - throw new Error(`Unknown method: ${method}`); - } - const data = await handler(params); - this.sendResponse({ requestId, method, success: true, data }); - } catch (err) { - const errorMessage = toError(err).message; - this.logger.warn(`Request ${method} failed`, err); - this.sendResponse({ - requestId, - method, - success: false, - error: errorMessage, + await dispatchCommand(message, this.commandHandlers, { + logger: this.logger, }); - if (TasksPanelProvider.USER_ACTION_METHODS.has(method)) { - vscode.window.showErrorMessage(errorMessage); - } - } - } - - private async handleCommand(message: { - method: string; - params?: unknown; - }): Promise { - const { method, params } = message; - - try { - const handler = this.commandHandlers[method]; - if (!handler) { - throw new Error(`Unknown command: ${method}`); - } - await handler(params); - } catch (err) { - const errorMessage = toError(err).message; - this.logger.warn(`Command ${method} failed`, err); - vscode.window.showErrorMessage(errorMessage); } } @@ -581,18 +522,13 @@ export class TasksPanelProvider } } - private sendResponse(response: IpcResponse): void { - this.view?.webview.postMessage(response); - } - private notify( def: NotificationDef, ...args: D extends void ? [] : [data: D] ): void { - this.view?.webview.postMessage({ - type: def.method, - ...(args.length > 0 ? { data: args[0] } : {}), - }); + if (this.view) { + notifyWebview(this.view.webview, def, ...args); + } } dispose(): void { diff --git a/src/webviews/util.ts b/src/webviews/util.ts index 08115f8f..053af95d 100644 --- a/src/webviews/util.ts +++ b/src/webviews/util.ts @@ -1,6 +1,11 @@ import { randomBytes } from "node:crypto"; import * as vscode from "vscode"; +import { toError } from "../error/errorUtils"; +import { type Logger } from "../logging/logger"; + +import type { IpcRequest, IpcResponse, NotificationDef } from "@repo/shared"; + /** * Get the HTML content for a webview. */ @@ -59,3 +64,126 @@ const HTML_ENTITIES: Record = { export function escapeHtml(s: string): string { return s.replace(/[&<>"']/g, (ch) => HTML_ENTITIES[ch]); } + +export function isIpcRequest(msg: unknown): msg is IpcRequest { + return ( + typeof msg === "object" && + msg !== null && + "requestId" in msg && + typeof (msg as IpcRequest).requestId === "string" && + "method" in msg && + typeof (msg as IpcRequest).method === "string" + ); +} + +export function isIpcCommand( + msg: unknown, +): msg is { method: string; params?: unknown } { + return ( + typeof msg === "object" && + msg !== null && + !("requestId" in msg) && + "method" in msg && + typeof (msg as { method: string }).method === "string" + ); +} + +/** Push a typed notification to a webview. */ +export function notifyWebview( + webview: vscode.Webview, + def: NotificationDef, + ...args: D extends void ? [] : [data: D] +): void { + webview.postMessage({ + type: def.method, + ...(args.length > 0 ? { data: args[0] } : {}), + }); +} + +export interface DispatchOptions { + logger: Logger; + /** + * Predicate run after a handler throws. If it returns true the error is + * also shown via `showErrorMessage`. Defaults: commands show, requests + * don't (see `dispatchCommand` / `dispatchRequest`). + */ + showErrorToUser?: (method: string) => boolean; +} + +/** + * Dispatch a fire-and-forget command. On failure, logs a warning and (by + * default) shows the error to the user. + */ +export async function dispatchCommand( + message: { method: string; params?: unknown }, + handlers: Record void | Promise>, + options: DispatchOptions, +): Promise { + const { method, params } = message; + const showToUser = options.showErrorToUser ?? (() => true); + try { + const handler = handlers[method]; + if (!handler) { + throw new Error(`Unknown command: ${method}`); + } + await handler(params); + } catch (err) { + const errorMessage = toError(err).message; + options.logger.warn(`Command ${method} failed`, err); + if (showToUser(method)) { + vscode.window.showErrorMessage(errorMessage); + } + } +} + +/** + * Dispatch a request and post a typed response back to the webview. On + * failure, logs a warning, posts an error response, and (if + * `showErrorToUser(method)` returns true) shows the error to the user; + * default is not to show. + * + * If `webview` is undefined (e.g. the view was disposed mid-flight) the + * response is dropped silently. + */ +export async function dispatchRequest( + message: IpcRequest, + handlers: Record Promise>, + webview: vscode.Webview | undefined, + options: DispatchOptions, +): Promise { + const { requestId, method, params } = message; + const showToUser = options.showErrorToUser ?? (() => false); + const respond = (response: IpcResponse) => { + void webview?.postMessage(response); + }; + try { + const handler = handlers[method]; + if (!handler) { + throw new Error(`Unknown method: ${method}`); + } + const data = await handler(params); + respond({ requestId, method, success: true, data }); + } catch (err) { + const errorMessage = toError(err).message; + options.logger.warn(`Request ${method} failed`, err); + respond({ requestId, method, success: false, error: errorMessage }); + if (showToUser(method)) { + vscode.window.showErrorMessage(errorMessage); + } + } +} + +/** + * Fire `handler` on `event` only while `panel.visible` is true. + */ +export function onWhileVisible( + panel: { readonly visible: boolean }, + event: vscode.Event, + handler: () => void, +): vscode.Disposable { + return event(() => { + if (panel.visible) { + handler(); + } + }); +} diff --git a/test/webview/shared/ipc.test.ts b/test/webview/shared/ipc.test.ts new file mode 100644 index 00000000..32130b06 --- /dev/null +++ b/test/webview/shared/ipc.test.ts @@ -0,0 +1,120 @@ +import { beforeEach, describe, expect, it, vi } from "vitest"; + +import { defineCommand, defineNotification } from "@repo/shared"; +import { onNotification, sendCommand } from "@repo/webview-shared/ipc"; + +interface Sent { + method: string; + params?: unknown; +} + +const sent: Sent[] = []; + +vi.stubGlobal( + "acquireVsCodeApi", + vi.fn(() => ({ + postMessage: (msg: Sent) => sent.push(msg), + getState: () => undefined, + setState: () => undefined, + })), +); + +beforeEach(() => { + sent.length = 0; +}); + +describe("sendCommand", () => { + it("posts {method, params}", () => { + const cmd = defineCommand<{ id: string }>("ns/doThing"); + sendCommand(cmd, { id: "42" }); + + expect(sent).toEqual([{ method: "ns/doThing", params: { id: "42" } }]); + }); + + it("posts without params for void-payload commands", () => { + const cmd = defineCommand("ns/noop"); + sendCommand(cmd); + + expect(sent).toEqual([{ method: "ns/noop", params: undefined }]); + }); +}); + +describe("onNotification", () => { + it("invokes callback only for matching method", () => { + const def = defineNotification<{ count: number }>("ns/updated"); + const cb = vi.fn(); + + const unsubscribe = onNotification(def, cb); + + window.dispatchEvent( + new MessageEvent("message", { + data: { type: "ns/other", data: { count: 1 } }, + }), + ); + expect(cb).not.toHaveBeenCalled(); + + window.dispatchEvent( + new MessageEvent("message", { + data: { type: "ns/updated", data: { count: 7 } }, + }), + ); + expect(cb).toHaveBeenCalledWith({ count: 7 }); + + unsubscribe(); + }); + + it("stops receiving events after unsubscribe", () => { + const def = defineNotification("ns/ping"); + const cb = vi.fn(); + + const unsubscribe = onNotification(def, cb); + unsubscribe(); + + window.dispatchEvent( + new MessageEvent("message", { + data: { type: "ns/ping", data: "hello" }, + }), + ); + + expect(cb).not.toHaveBeenCalled(); + }); + + it("ignores non-object messages", () => { + const def = defineNotification("ns/evt"); + const cb = vi.fn(); + const unsubscribe = onNotification(def, cb); + + window.dispatchEvent(new MessageEvent("message", { data: null })); + window.dispatchEvent(new MessageEvent("message", { data: "string" })); + window.dispatchEvent(new MessageEvent("message", { data: 42 })); + + expect(cb).not.toHaveBeenCalled(); + unsubscribe(); + }); + + it("supports multiple independent subscribers", () => { + const def = defineNotification("ns/count"); + const a = vi.fn(); + const b = vi.fn(); + + const unsubA = onNotification(def, a); + const unsubB = onNotification(def, b); + + window.dispatchEvent( + new MessageEvent("message", { data: { type: "ns/count", data: 1 } }), + ); + + expect(a).toHaveBeenCalledWith(1); + expect(b).toHaveBeenCalledWith(1); + + unsubA(); + window.dispatchEvent( + new MessageEvent("message", { data: { type: "ns/count", data: 2 } }), + ); + + expect(a).toHaveBeenCalledTimes(1); + expect(b).toHaveBeenCalledTimes(2); + + unsubB(); + }); +}); From 23c96b92dc44541e818f7528b741d1832f7974d3 Mon Sep 17 00:00:00 2001 From: Ehab Younes Date: Mon, 20 Apr 2026 18:01:44 +0300 Subject: [PATCH 2/6] refactor: migrate chat panel to shared IPC helpers Define ChatApi in shared and replace the ad-hoc {type, ...} message handling with buildCommandHandlers / buildRequestHandlers plus dispatchCommand / dispatchRequest. Outgoing notifications route through a private notify() wrapper, mirroring tasksPanelProvider. The iframe shim in the inline HTML keeps its own {type, payload} contract with the Coder server, but its shim-to-extension side now speaks the IPC wire format. Split the handler into handleFromIframe / handleFromExtension + a toIframe helper + a showRetry builder so the dispatch reads straight through. Tests updated to the new wire format. --- packages/shared/src/chat/api.ts | 20 ++ packages/shared/src/index.ts | 3 + src/webviews/chat/chatPanelProvider.ts | 215 ++++++++++-------- .../webviews/chat/chatPanelProvider.test.ts | 20 +- 4 files changed, 158 insertions(+), 100 deletions(-) create mode 100644 packages/shared/src/chat/api.ts diff --git a/packages/shared/src/chat/api.ts b/packages/shared/src/chat/api.ts new file mode 100644 index 00000000..47e2b74e --- /dev/null +++ b/packages/shared/src/chat/api.ts @@ -0,0 +1,20 @@ +import { defineCommand, defineNotification } from "../ipc/protocol"; + +/** The chat webview embeds an iframe that speaks to the Coder server. */ +export const ChatApi = { + /** Iframe reports it needs the session token. */ + vscodeReady: defineCommand("coder:vscode-ready"), + /** Iframe reports the chat UI has rendered. */ + chatReady: defineCommand("coder:chat-ready"), + /** Iframe requests an external navigation; same-origin only. */ + navigate: defineCommand<{ url: string }>("coder:navigate"), + + /** Push the current theme into the iframe. */ + setTheme: defineNotification<{ theme: "light" | "dark" }>("coder:set-theme"), + /** Push the session token to bootstrap iframe auth. */ + authBootstrapToken: defineNotification<{ token: string }>( + "coder:auth-bootstrap-token", + ), + /** Signal that auth could not be obtained. */ + authError: defineNotification<{ error: string }>("coder:auth-error"), +} as const; diff --git a/packages/shared/src/index.ts b/packages/shared/src/index.ts index 8f9ccef3..b403822e 100644 --- a/packages/shared/src/index.ts +++ b/packages/shared/src/index.ts @@ -16,3 +16,6 @@ export { type SpeedtestInterval, type SpeedtestResult, } from "./speedtest/api"; + +// Chat API +export { ChatApi } from "./chat/api"; diff --git a/src/webviews/chat/chatPanelProvider.ts b/src/webviews/chat/chatPanelProvider.ts index 3b7f48bd..252291c4 100644 --- a/src/webviews/chat/chatPanelProvider.ts +++ b/src/webviews/chat/chatPanelProvider.ts @@ -1,22 +1,36 @@ import * as vscode from "vscode"; +import { + buildCommandHandlers, + buildRequestHandlers, + ChatApi, + type NotificationDef, +} from "@repo/shared"; + import { type CoderApi } from "../../api/coderApi"; import { type Logger } from "../../logging/logger"; -import { getNonce } from "../util"; +import { + dispatchCommand, + dispatchRequest, + getNonce, + isIpcCommand, + isIpcRequest, + notifyWebview, +} from "../util"; /** * Provides a webview that embeds the Coder agent chat UI. * Authentication flows through postMessage: * * 1. The iframe loads /agents/{id}/embed on the Coder server. - * 2. The embed page detects the user is signed out and sends + * 2. The embed page detects the user is signed out and posts * { type: "coder:vscode-ready" } to window.parent. - * 3. Our webview relays this to the extension host. - * 4. The extension host replies with the session token. - * 5. The webview forwards { type: "coder:vscode-auth-bootstrap" } - * with the token back into the iframe. - * 6. The embed page calls API.setSessionToken(token), re-fetches - * the authenticated user, and renders the chat UI. + * 3. Our shim relays that to the extension as a ChatApi.vscodeReady + * command. + * 4. The extension pushes the session token back with + * ChatApi.authBootstrapToken. + * 5. The shim forwards it into the iframe, which calls + * API.setSessionToken and re-fetches the user. */ export class ChatPanelProvider implements vscode.WebviewViewProvider, vscode.Disposable @@ -28,6 +42,13 @@ export class ChatPanelProvider private chatId: string | undefined; private authRetryTimer: ReturnType | undefined; + private readonly commandHandlers = buildCommandHandlers(ChatApi, { + vscodeReady: () => this.sendAuthToken(), + chatReady: () => this.sendTheme(), + navigate: ({ url }) => this.handleNavigate(url), + }); + private readonly requestHandlers = buildRequestHandlers(ChatApi, {}); + constructor( private readonly client: Pick, private readonly logger: Logger, @@ -41,11 +62,17 @@ export class ChatPanelProvider : "dark"; } + private notify( + def: NotificationDef, + ...args: D extends void ? [] : [data: D] + ): void { + if (this.view) { + notifyWebview(this.view.webview, def, ...args); + } + } + private sendTheme(): void { - this.view?.webview.postMessage({ - type: "coder:set-theme", - theme: this.getTheme(), - }); + this.notify(ChatApi.setTheme, { theme: this.getTheme() }); } /** @@ -75,8 +102,20 @@ export class ChatPanelProvider this.view = webviewView; webviewView.webview.options = { enableScripts: true }; this.disposables.push( - webviewView.webview.onDidReceiveMessage((msg: unknown) => { - this.handleMessage(msg); + webviewView.webview.onDidReceiveMessage((message: unknown) => { + if (isIpcRequest(message)) { + void dispatchRequest( + message, + this.requestHandlers, + webviewView.webview, + { logger: this.logger }, + ); + } else if (isIpcCommand(message)) { + void dispatchCommand(message, this.commandHandlers, { + logger: this.logger, + showErrorToUser: () => false, + }); + } }), vscode.window.onDidChangeActiveColorTheme(() => { this.sendTheme(); @@ -114,38 +153,19 @@ export class ChatPanelProvider webview.html = this.getIframeHtml(embedUrl, coderUrl); } - private handleMessage(message: unknown): void { - if (typeof message !== "object" || message === null) { + private handleNavigate(url: string): void { + const coderUrl = this.client.getHost(); + if (!url || !coderUrl) { return; } - const msg = message as { type?: string; payload?: { url?: string } }; - switch (msg.type) { - case "coder:vscode-ready": - this.sendAuthToken(); - break; - case "coder:chat-ready": - this.sendTheme(); - break; - case "coder:navigate": { - const url = msg.payload?.url; - const coderUrl = this.client.getHost(); - if (url && coderUrl) { - try { - const resolved = new URL(url, coderUrl); - const expected = new URL(coderUrl); - if (resolved.origin === expected.origin) { - void vscode.env.openExternal( - vscode.Uri.parse(resolved.toString()), - ); - } - } catch { - this.logger.warn(`Chat: invalid navigate URL: ${url}`); - } - } - break; + try { + const resolved = new URL(url, coderUrl); + const expected = new URL(coderUrl); + if (resolved.origin === expected.origin) { + void vscode.env.openExternal(vscode.Uri.parse(resolved.toString())); } - default: - break; + } catch { + this.logger.warn(`Chat: invalid navigate URL: ${url}`); } } @@ -178,17 +198,13 @@ export class ChatPanelProvider "Chat iframe requested auth but no session token available " + "after all retries", ); - this.view?.webview.postMessage({ - type: "coder:auth-error", + this.notify(ChatApi.authError, { error: "No session token available. Please sign in and retry.", }); return; } this.logger.info("Chat: forwarding token to iframe"); - this.view?.webview.postMessage({ - type: "coder:auth-bootstrap-token", - token, - }); + this.notify(ChatApi.authBootstrapToken, { token }); } private getIframeHtml(embedUrl: string, allowedOrigin: string): string { @@ -246,53 +262,72 @@ export class ChatPanelProvider status.style.display = 'none'; }); - window.addEventListener('message', (event) => { - const data = event.data; - if (!data || typeof data !== 'object') return; + // Shim sits between two wire formats. The iframe speaks + // { type, payload } (a contract owned by the Coder server). + // The extension speaks the IPC protocol: commands are + // { method, params } and notifications are { type, data }. + // See packages/webview-shared/README.md. + const toIframe = (type, payload) => { + iframe.contentWindow.postMessage({ type, payload }, '${allowedOrigin}'); + }; - if (event.source === iframe.contentWindow) { - if (data.type === 'coder:vscode-ready') { - status.textContent = 'Authenticating…'; - vscode.postMessage({ type: 'coder:vscode-ready' }); - } - if (data.type === 'coder:chat-ready') { - vscode.postMessage({ type: 'coder:chat-ready' }); - } - if (data.type === 'coder:navigate') { - vscode.postMessage(data); - } - return; - } + const showRetry = (error) => { + status.textContent = ''; + status.appendChild(document.createTextNode(error || 'Authentication failed.')); + const btn = document.createElement('button'); + btn.id = 'retry-btn'; + btn.textContent = 'Retry'; + btn.addEventListener('click', () => { + status.textContent = 'Authenticating…'; + vscode.postMessage({ method: 'coder:vscode-ready' }); + }); + status.appendChild(document.createElement('br')); + status.appendChild(btn); + status.style.display = 'block'; + iframe.style.display = 'none'; + }; - if (data.type === 'coder:auth-bootstrap-token') { - status.textContent = 'Signing in…'; - iframe.contentWindow.postMessage({ - type: 'coder:vscode-auth-bootstrap', - payload: { token: data.token }, - }, '${allowedOrigin}'); + const handleFromIframe = (msg) => { + switch (msg.type) { + case 'coder:vscode-ready': + status.textContent = 'Authenticating…'; + vscode.postMessage({ method: 'coder:vscode-ready' }); + return; + case 'coder:chat-ready': + vscode.postMessage({ method: 'coder:chat-ready' }); + return; + case 'coder:navigate': + vscode.postMessage({ + method: 'coder:navigate', + params: { url: msg.payload?.url }, + }); + return; } + }; - if (data.type === 'coder:set-theme') { - iframe.contentWindow.postMessage({ - type: 'coder:set-theme', - payload: { theme: data.theme }, - }, '${allowedOrigin}'); + const handleFromExtension = (msg) => { + const data = msg.data || {}; + switch (msg.type) { + case 'coder:auth-bootstrap-token': + status.textContent = 'Signing in…'; + toIframe('coder:vscode-auth-bootstrap', { token: data.token }); + return; + case 'coder:set-theme': + toIframe('coder:set-theme', { theme: data.theme }); + return; + case 'coder:auth-error': + showRetry(data.error); + return; } + }; - if (data.type === 'coder:auth-error') { - status.textContent = ''; - status.appendChild(document.createTextNode(data.error || 'Authentication failed.')); - const btn = document.createElement('button'); - btn.id = 'retry-btn'; - btn.textContent = 'Retry'; - btn.addEventListener('click', () => { - status.textContent = 'Authenticating…'; - vscode.postMessage({ type: 'coder:vscode-ready' }); - }); - status.appendChild(document.createElement('br')); - status.appendChild(btn); - status.style.display = 'block'; - iframe.style.display = 'none'; + window.addEventListener('message', (event) => { + const msg = event.data; + if (!msg || typeof msg !== 'object') return; + if (event.source === iframe.contentWindow) { + handleFromIframe(msg); + } else { + handleFromExtension(msg); } }); })(); diff --git a/test/unit/webviews/chat/chatPanelProvider.test.ts b/test/unit/webviews/chat/chatPanelProvider.test.ts index 97fa5a88..d93ea01c 100644 --- a/test/unit/webviews/chat/chatPanelProvider.test.ts +++ b/test/unit/webviews/chat/chatPanelProvider.test.ts @@ -92,11 +92,11 @@ describe("ChatPanelProvider", () => { windowMock.__setActiveColorThemeKind(kind); const { sendFromWebview, postMessage } = createHarness(); - sendFromWebview({ type: "coder:chat-ready" }); + sendFromWebview({ method: "coder:chat-ready" }); expect(findPostedMessage(postMessage, "coder:set-theme")).toEqual({ type: "coder:set-theme", - theme: expected, + data: { theme: expected }, }); }); @@ -108,7 +108,7 @@ describe("ChatPanelProvider", () => { expect(postMessage).toHaveBeenCalledWith({ type: "coder:set-theme", - theme: "light", + data: { theme: "light" }, }); }); }); @@ -117,13 +117,13 @@ describe("ChatPanelProvider", () => { it("sends auth token on coder:vscode-ready", () => { const { sendFromWebview, postMessage } = createHarness(); - sendFromWebview({ type: "coder:vscode-ready" }); + sendFromWebview({ method: "coder:vscode-ready" }); expect( findPostedMessage(postMessage, "coder:auth-bootstrap-token"), ).toEqual({ type: "coder:auth-bootstrap-token", - token: "test-token", + data: { token: "test-token" }, }); }); }); @@ -133,8 +133,8 @@ describe("ChatPanelProvider", () => { const { sendFromWebview } = createHarness(); sendFromWebview({ - type: "coder:navigate", - payload: { url: "/templates" }, + method: "coder:navigate", + params: { url: "/templates" }, }); expect(vscode.env.openExternal).toHaveBeenCalledWith( @@ -145,7 +145,7 @@ describe("ChatPanelProvider", () => { it("ignores navigate without url payload", () => { const { sendFromWebview } = createHarness(); - sendFromWebview({ type: "coder:navigate" }); + sendFromWebview({ method: "coder:navigate", params: {} }); expect(vscode.env.openExternal).not.toHaveBeenCalled(); }); @@ -154,8 +154,8 @@ describe("ChatPanelProvider", () => { const { sendFromWebview } = createHarness(); sendFromWebview({ - type: "coder:navigate", - payload: { url: "https://evil.com/steal" }, + method: "coder:navigate", + params: { url: "https://evil.com/steal" }, }); expect(vscode.env.openExternal).not.toHaveBeenCalled(); From f11c7f946778de77474e901227b2208798da15f3 Mon Sep 17 00:00:00 2001 From: Ehab Younes Date: Thu, 30 Apr 2026 15:45:52 +0300 Subject: [PATCH 3/6] refactor: address PR review on webview IPC helpers Split src/webviews/util.ts into dispatch.ts (IPC runtime) and html.ts (scaffolding). Default dispatchCommand to not show errors, matching dispatchRequest, and let panels opt in per method. Tasks panel adds viewInCoder and viewLogs to its user-action set so dialogs still pop for clicked items. Chat shim now uses ?? for data fallback, validates URL before forwarding navigate, and references ChatApi sync requirements in a short comment block. Drop the duplicate notify wrapper in chat and tasks since notifyWebview accepts undefined webview. Rename "Unknown method" to "Unknown request" for symmetry. sendCommand omits params for void commands. Tests cover the dispatch error branches, void notifications, and the auth-error retry path. README tightened and clarified that the visibility resend only matters when retainContextWhenHidden is unset. --- packages/shared/src/chat/api.ts | 5 +- packages/webview-shared/README.md | 113 +++++----- packages/webview-shared/src/ipc.ts | 2 +- src/webviews/chat/chatPanelProvider.ts | 43 ++-- src/webviews/dispatch.ts | 125 +++++++++++ src/webviews/html.ts | 61 ++++++ .../speedtest/speedtestPanelFactory.ts | 4 +- src/webviews/tasks/tasksPanelProvider.ts | 33 ++- src/webviews/util.ts | 189 ---------------- .../webviews/chat/chatPanelProvider.test.ts | 33 ++- test/unit/webviews/dispatch.test.ts | 202 ++++++++++++++++++ .../webviews/{util.test.ts => html.test.ts} | 2 +- .../webviews/tasks/tasksPanelProvider.test.ts | 2 +- test/webview/shared/ipc.test.ts | 17 +- 14 files changed, 531 insertions(+), 300 deletions(-) create mode 100644 src/webviews/dispatch.ts create mode 100644 src/webviews/html.ts delete mode 100644 src/webviews/util.ts create mode 100644 test/unit/webviews/dispatch.test.ts rename test/unit/webviews/{util.test.ts => html.test.ts} (99%) diff --git a/packages/shared/src/chat/api.ts b/packages/shared/src/chat/api.ts index 47e2b74e..4a95fc79 100644 --- a/packages/shared/src/chat/api.ts +++ b/packages/shared/src/chat/api.ts @@ -1,6 +1,9 @@ import { defineCommand, defineNotification } from "../ipc/protocol"; -/** The chat webview embeds an iframe that speaks to the Coder server. */ +/** + * Chat webview API. The inline shim in `chatPanelProvider.ts` mirrors + * these entries; missed updates fail silently at runtime. + */ export const ChatApi = { /** Iframe reports it needs the session token. */ vscodeReady: defineCommand("coder:vscode-ready"), diff --git a/packages/webview-shared/README.md b/packages/webview-shared/README.md index 2f20e1ce..7013fe9c 100644 --- a/packages/webview-shared/README.md +++ b/packages/webview-shared/README.md @@ -1,20 +1,18 @@ # Webview IPC -Shared primitives for typed, reliable messaging between the extension host -and VS Code webviews. Both sides use the **same `Api` definition object** so -wire formats can't drift and method-name typos are caught at compile time. +Typed messaging between the extension and VS Code webviews. Both sides +import the same `Api` definition, so wire formats can't drift and method +typos fail at compile time. -## Message kinds +## Three message kinds ```ts -// packages/shared/src/ipc/protocol.ts defineNotification(method); // extension to webview, fire-and-forget defineCommand

(method); // webview to extension, fire-and-forget defineRequest(method); // webview to extension, awaits response ``` -Define all three together in one `Api` const so both sides import the exact -same method strings: +Define them all in one `Api` so both sides share the strings: ```ts // packages/shared/src/myfeature/api.ts @@ -27,22 +25,18 @@ export const MyFeatureApi = { ## Extension side (`src/webviews/...`) -### Sending notifications +Push notifications: ```ts -import { notifyWebview } from "../util"; +import { notifyWebview } from "../dispatch"; notifyWebview(panel.webview, MyFeatureApi.data, payload); ``` -### Handling incoming messages (exhaustiveness) - -Every panel **must** build handler maps with `buildCommandHandlers` and -`buildRequestHandlers`, even if it has zero requests today. Both builders -are mapped over the `Api` definition, so adding a new `defineCommand` or -`defineRequest` entry **produces a compile error** at the panel that forgot -to wire a handler. This makes it impossible to ship an API surface that -the extension silently drops. +Receive commands and requests. Every panel builds both handler maps (an +empty `{}` is fine). The maps are mapped over the `Api`, so adding a new +`defineCommand` or `defineRequest` produces a compile error in any panel +missing a handler. That's how new methods can't ship and silently drop. ```ts import { buildCommandHandlers, buildRequestHandlers } from "@repo/shared"; @@ -51,12 +45,12 @@ import { dispatchRequest, isIpcCommand, isIpcRequest, -} from "../util"; +} from "../dispatch"; const commandHandlers = buildCommandHandlers(MyFeatureApi, { doThing: async (p) => { ... }, }); -// Empty is fine; it still enforces future additions. +// Empty is fine; it still locks in future additions. const requestHandlers = buildRequestHandlers(MyFeatureApi, { getThings: async () => this.fetchThings(), }); @@ -73,65 +67,76 @@ panel.webview.onDidReceiveMessage((message: unknown) => { }); ``` -**Error semantics** (built into `dispatchCommand` / `dispatchRequest`): +### Error handling -| | Logs | Response sent? | `showErrorMessage` default | -| ------------- | ------------- | ----------------------------- | -------------------------- | -| Command fails | `logger.warn` | n/a | **yes** (user action) | -| Request fails | `logger.warn` | `success: false` with `error` | **no** (often background) | +Both dispatchers log handler failures via `logger.warn`. Requests also +post a `success: false` response so the webview's awaited promise rejects. -Pass `showErrorToUser: (method) => …` to override per-method. +Neither pops a dialog by default. Pass `showErrorToUser: (method) => ...` +to opt in for methods where a silent failure would confuse the user. -## Webview side, vanilla (`@repo/webview-shared`) +## Webview side, vanilla ```ts import { MyFeatureApi } from "@repo/shared"; import { onNotification, sendCommand } from "@repo/webview-shared"; -// Subscribe to pushes. const unsubscribe = onNotification(MyFeatureApi.data, (payload) => { render(payload); }); -// Fire a command. sendCommand(MyFeatureApi.doThing, { id: "42" }); ``` -`onNotification` returns an unsubscribe function; call it on cleanup. +Call the returned unsubscribe function on cleanup. ## Webview side, React -Use `useIpc` (`packages/webview-shared/src/react/useIpc`). Same semantics -plus request/response correlation with timeout and UUID bookkeeping. +Use `useIpc` from `packages/webview-shared/src/react/useIpc`. Same +semantics, plus request/response correlation with timeouts and UUID +bookkeeping. -## The "no dropped events" guarantee +## Re-sending on lifecycle events -Webview contexts are **destroyed when hidden** unless -`retainContextWhenHidden` is set (costly; avoid). This means the webview's -in-memory state, event listeners, and canvas pixels are lost whenever the -user switches tabs. +Webviews lose state in two ways the extension has to compensate for. -Every webview that pushes state from the extension must therefore **re-send -on these signals**, so the revived webview isn't left empty or stale: +**Hidden webviews are destroyed.** Switching tabs discards in-memory state, +listeners, and canvas pixels. Push-driven panels resend when the webview +comes back. Subscribe to `panel.onDidChangeViewState` (`WebviewPanel`) or +`view.onDidChangeVisibility` (`WebviewViewProvider`) and resend on +`visible`. Setting `retainContextWhenHidden` avoids this but is costly, +so we don't. -1. **Visibility change**: `panel.onDidChangeViewState(() => panel.visible && resend())`. - The webview was just re-created from HTML, so no in-memory state remains. -2. **Color theme change**: `vscode.window.onDidChangeActiveColorTheme(() => panel.visible && resend())`. - DOM elements update via CSS vars, but canvas and SVG drawn imperatively - do not: pixels are baked in. The webview must redraw against the new - theme. +**Theme changes don't repaint canvases.** CSS vars update automatically, +but canvas and SVG drawn imperatively bake the theme into pixels. +Subscribe to `vscode.window.onDidChangeActiveColorTheme` and resend so +the webview redraws. This applies regardless of `retainContextWhenHidden`. -The `onWhileVisible(panel, event, handler)` helper in `src/webviews/util.ts` -wraps both cases. Both disposables must be collected and disposed in -`panel.onDidDispose` so they don't leak when the user closes the tab. +`onWhileVisible(panel, event, handler)` in `src/webviews/dispatch.ts` +works with both panel shapes. Collect disposables and clear them in +`onDidDispose`. -See `src/webviews/speedtest/speedtestPanel.ts` for a minimal reference. +See `speedtestPanelFactory.ts` for a `WebviewPanel` example and +`tasksPanelProvider.ts` for a `WebviewViewProvider` example. ## Checklist for a new webview -1. Define the API in `packages/shared/src//api.ts` and export from `packages/shared/src/index.ts`. -2. Extension side: `buildCommandHandlers` **and** `buildRequestHandlers` (empty `{}` is fine; both are needed for exhaustiveness). -3. Extension side: dispatch through `isIpcRequest` to `dispatchRequest` and `isIpcCommand` to `dispatchCommand`, both with a logger. -4. Extension side: use `onWhileVisible` for `onDidChangeViewState` and `onDidChangeActiveColorTheme`, dispose in `onDidDispose`. -5. Webview side: use `onNotification` / `sendCommand` (vanilla) or `useIpc` (React). Never hand-roll `window.addEventListener("message", ...)`. -6. Tests: verify the panel posts the expected payload shape, re-sends on visibility and theme, and handles incoming commands. +1. Register the view in `package.json` under `contributes.views` (sidebar) + or call `vscode.window.createWebviewPanel` (editor tab). +2. Register the provider in `src/extension.ts`. +3. Define the API in `packages/shared/src//api.ts` and export it + from `packages/shared/src/index.ts`. +4. Build both handler maps with `buildCommandHandlers` and + `buildRequestHandlers` (empty `{}` is fine). +5. Dispatch with `isIpcRequest` -> `dispatchRequest` and `isIpcCommand` -> + `dispatchCommand`, both with a logger. +6. Use `onWhileVisible` for `onDidChangeViewState` / + `onDidChangeVisibility` and `onDidChangeActiveColorTheme`, and dispose + in `onDidDispose`. +7. On the webview side, use `onNotification` / `sendCommand` (vanilla) or + `useIpc` (React). Don't hand-roll `window.addEventListener("message", +...)` in webview package code. The one exception is an inline HTML + shim bridging a third-party iframe (see `chatPanelProvider.ts`); keep + it small. +8. Tests: assert the panel posts the expected payload shape, resends on + visibility and theme, and handles incoming commands. diff --git a/packages/webview-shared/src/ipc.ts b/packages/webview-shared/src/ipc.ts index 34f171ee..9aedf5f1 100644 --- a/packages/webview-shared/src/ipc.ts +++ b/packages/webview-shared/src/ipc.ts @@ -14,7 +14,7 @@ export function sendCommand

( ): void { postMessage({ method: def.method, - params: args[0], + ...(args.length > 0 ? { params: args[0] } : {}), }); } diff --git a/src/webviews/chat/chatPanelProvider.ts b/src/webviews/chat/chatPanelProvider.ts index 252291c4..3d1d8d6e 100644 --- a/src/webviews/chat/chatPanelProvider.ts +++ b/src/webviews/chat/chatPanelProvider.ts @@ -4,7 +4,6 @@ import { buildCommandHandlers, buildRequestHandlers, ChatApi, - type NotificationDef, } from "@repo/shared"; import { type CoderApi } from "../../api/coderApi"; @@ -12,11 +11,11 @@ import { type Logger } from "../../logging/logger"; import { dispatchCommand, dispatchRequest, - getNonce, isIpcCommand, isIpcRequest, notifyWebview, -} from "../util"; +} from "../dispatch"; +import { getNonce } from "../html"; /** * Provides a webview that embeds the Coder agent chat UI. @@ -62,17 +61,10 @@ export class ChatPanelProvider : "dark"; } - private notify( - def: NotificationDef, - ...args: D extends void ? [] : [data: D] - ): void { - if (this.view) { - notifyWebview(this.view.webview, def, ...args); - } - } - private sendTheme(): void { - this.notify(ChatApi.setTheme, { theme: this.getTheme() }); + notifyWebview(this.view?.webview, ChatApi.setTheme, { + theme: this.getTheme(), + }); } /** @@ -113,7 +105,6 @@ export class ChatPanelProvider } else if (isIpcCommand(message)) { void dispatchCommand(message, this.commandHandlers, { logger: this.logger, - showErrorToUser: () => false, }); } }), @@ -198,13 +189,13 @@ export class ChatPanelProvider "Chat iframe requested auth but no session token available " + "after all retries", ); - this.notify(ChatApi.authError, { + notifyWebview(this.view?.webview, ChatApi.authError, { error: "No session token available. Please sign in and retry.", }); return; } this.logger.info("Chat: forwarding token to iframe"); - this.notify(ChatApi.authBootstrapToken, { token }); + notifyWebview(this.view?.webview, ChatApi.authBootstrapToken, { token }); } private getIframeHtml(embedUrl: string, allowedOrigin: string): string { @@ -262,11 +253,9 @@ export class ChatPanelProvider status.style.display = 'none'; }); - // Shim sits between two wire formats. The iframe speaks - // { type, payload } (a contract owned by the Coder server). - // The extension speaks the IPC protocol: commands are - // { method, params } and notifications are { type, data }. - // See packages/webview-shared/README.md. + // Bridges the iframe ({ type, payload }, owned by the Coder server) + // and the extension IPC ({ method, params } / { type, data }). + // Cases below must mirror ChatApi; inline JS can't import it. const toIframe = (type, payload) => { iframe.contentWindow.postMessage({ type, payload }, '${allowedOrigin}'); }; @@ -297,16 +286,18 @@ export class ChatPanelProvider vscode.postMessage({ method: 'coder:chat-ready' }); return; case 'coder:navigate': - vscode.postMessage({ - method: 'coder:navigate', - params: { url: msg.payload?.url }, - }); + if (msg.payload?.url) { + vscode.postMessage({ + method: 'coder:navigate', + params: { url: msg.payload.url }, + }); + } return; } }; const handleFromExtension = (msg) => { - const data = msg.data || {}; + const data = msg.data ?? {}; switch (msg.type) { case 'coder:auth-bootstrap-token': status.textContent = 'Signing in…'; diff --git a/src/webviews/dispatch.ts b/src/webviews/dispatch.ts new file mode 100644 index 00000000..3c17d272 --- /dev/null +++ b/src/webviews/dispatch.ts @@ -0,0 +1,125 @@ +import * as vscode from "vscode"; + +import { toError } from "../error/errorUtils"; +import { type Logger } from "../logging/logger"; + +import type { IpcRequest, IpcResponse, NotificationDef } from "@repo/shared"; + +export interface DispatchOptions { + logger: Logger; + /** Returning true shows the handler's error via `showErrorMessage`. */ + showErrorToUser?: (method: string) => boolean; +} + +/** Push a typed notification to a webview. No-op when `webview` is undefined. */ +export function notifyWebview( + webview: vscode.Webview | undefined, + def: NotificationDef, + ...args: D extends void ? [] : [data: D] +): void { + void webview?.postMessage({ + type: def.method, + ...(args.length > 0 ? { data: args[0] } : {}), + }); +} + +/** Dispatch a fire-and-forget command, logging any handler failure. */ +export async function dispatchCommand( + message: { method: string; params?: unknown }, + handlers: Record void | Promise>, + options: DispatchOptions, +): Promise { + const { method, params } = message; + try { + const handler = handlers[method]; + if (!handler) { + throw new Error(`Unknown command: ${method}`); + } + await handler(params); + } catch (err) { + handleDispatchError("Command", method, err, options); + } +} + +/** + * Dispatch a request and post a typed response back. If the handler throws, + * posts a failure response. If `webview` is undefined the response is dropped. + */ +export async function dispatchRequest( + message: IpcRequest, + handlers: Record Promise>, + webview: vscode.Webview | undefined, + options: DispatchOptions, +): Promise { + const { requestId, method, params } = message; + const respond = (response: IpcResponse) => { + void webview?.postMessage(response); + }; + try { + const handler = handlers[method]; + if (!handler) { + throw new Error(`Unknown request: ${method}`); + } + const data = await handler(params); + respond({ requestId, method, success: true, data }); + } catch (err) { + respond({ + requestId, + method, + success: false, + error: toError(err).message, + }); + handleDispatchError("Request", method, err, options); + } +} + +/** Fire `handler` on `event` only while `panel.visible` is true. */ +export function onWhileVisible( + panel: { readonly visible: boolean }, + event: vscode.Event, + handler: () => void, +): vscode.Disposable { + return event(() => { + if (panel.visible) { + handler(); + } + }); +} + +/** Check if message is a request (has requestId). */ +export function isIpcRequest(msg: unknown): msg is IpcRequest { + return ( + typeof msg === "object" && + msg !== null && + "requestId" in msg && + typeof (msg as IpcRequest).requestId === "string" && + "method" in msg && + typeof (msg as IpcRequest).method === "string" + ); +} + +/** Check if message is a command (has method but no requestId). */ +export function isIpcCommand( + msg: unknown, +): msg is { method: string; params?: unknown } { + return ( + typeof msg === "object" && + msg !== null && + !("requestId" in msg) && + "method" in msg && + typeof (msg as { method: string }).method === "string" + ); +} + +function handleDispatchError( + kind: "Command" | "Request", + method: string, + err: unknown, + options: DispatchOptions, +): void { + const message = toError(err).message; + options.logger.warn(`${kind} ${method} failed`, err); + if (options.showErrorToUser?.(method)) { + vscode.window.showErrorMessage(message); + } +} diff --git a/src/webviews/html.ts b/src/webviews/html.ts new file mode 100644 index 00000000..b3216907 --- /dev/null +++ b/src/webviews/html.ts @@ -0,0 +1,61 @@ +import { randomBytes } from "node:crypto"; +import * as vscode from "vscode"; + +/** Get the HTML content for a webview. */ +export function getWebviewHtml( + webview: vscode.Webview, + extensionUri: vscode.Uri, + webviewName: string, + title: string, +): string { + const nonce = getNonce(); + const baseUri = vscode.Uri.joinPath( + extensionUri, + "dist", + "webviews", + webviewName, + ); + const scriptUri = webview.asWebviewUri( + vscode.Uri.joinPath(baseUri, "index.js"), + ); + const styleUri = webview.asWebviewUri( + vscode.Uri.joinPath(baseUri, "index.css"), + ); + + // The vscode-elements library looks for a link element with "vscode-codicon-stylesheet" + // ID to load the codicons font inside its shadow DOM components + return ` + + + + + + ${escapeHtml(title)} + + + +

+ + +`; +} + +export function getNonce(): string { + return randomBytes(16).toString("base64"); +} + +/** + * Escape characters with special meaning in HTML so user-controlled strings + * can be safely interpolated into markup. + */ +export function escapeHtml(s: string): string { + return s.replace(/[&<>"']/g, (ch) => HTML_ENTITIES[ch]); +} + +const HTML_ENTITIES: Record = { + "&": "&", + "<": "<", + ">": ">", + '"': """, + "'": "'", +}; diff --git a/src/webviews/speedtest/speedtestPanelFactory.ts b/src/webviews/speedtest/speedtestPanelFactory.ts index 4713ee79..eadfbf9c 100644 --- a/src/webviews/speedtest/speedtestPanelFactory.ts +++ b/src/webviews/speedtest/speedtestPanelFactory.ts @@ -11,12 +11,12 @@ import { import { dispatchCommand, dispatchRequest, - getWebviewHtml, isIpcCommand, isIpcRequest, notifyWebview, onWhileVisible, -} from "../util"; +} from "../dispatch"; +import { getWebviewHtml } from "../html"; import type { Logger } from "../../logging/logger"; diff --git a/src/webviews/tasks/tasksPanelProvider.ts b/src/webviews/tasks/tasksPanelProvider.ts index e7ce3679..46614665 100644 --- a/src/webviews/tasks/tasksPanelProvider.ts +++ b/src/webviews/tasks/tasksPanelProvider.ts @@ -12,7 +12,6 @@ import { isStableTask, TasksApi, type CreateTaskParams, - type NotificationDef, type TaskDetails, type TaskLogs, type TaskTemplate, @@ -30,11 +29,11 @@ import { vscodeProposed } from "../../vscodeProposed"; import { dispatchCommand, dispatchRequest, - getWebviewHtml, isIpcCommand, isIpcRequest, notifyWebview, -} from "../util"; +} from "../dispatch"; +import { getWebviewHtml } from "../html"; import type { Preset, @@ -57,12 +56,15 @@ export class TasksPanelProvider { public static readonly viewType = "coder.tasksPanel"; + /** Methods whose handler errors pop a dialog; others are logged only. */ private static readonly USER_ACTION_METHODS = new Set([ TasksApi.pauseTask.method, TasksApi.resumeTask.method, TasksApi.deleteTask.method, TasksApi.downloadLogs.method, TasksApi.sendTaskMessage.method, + TasksApi.viewInCoder.method, + TasksApi.viewLogs.method, ]); private view?: vscode.WebviewView; @@ -110,12 +112,12 @@ export class TasksPanelProvider ) {} public showCreateForm(): void { - this.notify(TasksApi.showCreateForm); + notifyWebview(this.view?.webview, TasksApi.showCreateForm); } public refresh(): void { this.cachedLogs = undefined; - this.notify(TasksApi.refresh); + notifyWebview(this.view?.webview, TasksApi.refresh); } resolveWebviewView( @@ -161,15 +163,17 @@ export class TasksPanelProvider } private async handleMessage(message: unknown): Promise { + const showErrorToUser = (method: string) => + TasksPanelProvider.USER_ACTION_METHODS.has(method); if (isIpcRequest(message)) { await dispatchRequest(message, this.requestHandlers, this.view?.webview, { logger: this.logger, - showErrorToUser: (method) => - TasksPanelProvider.USER_ACTION_METHODS.has(method), + showErrorToUser, }); } else if (isIpcCommand(message)) { await dispatchCommand(message, this.commandHandlers, { logger: this.logger, + showErrorToUser, }); } } @@ -364,7 +368,7 @@ export class TasksPanelProvider const clean = stripAnsi(line); // Skip lines that were purely ANSI codes, but keep intentional blank lines. if (line.length > 0 && clean.length === 0) return; - this.notify(TasksApi.workspaceLogsAppend, [clean]); + notifyWebview(this.view?.webview, TasksApi.workspaceLogsAppend, [clean]); }; const onStreamClose = () => { @@ -423,7 +427,7 @@ export class TasksPanelProvider try { const tasks = await this.fetchTasks(); if (tasks !== null) { - this.notify(TasksApi.tasksUpdated, tasks); + notifyWebview(this.view?.webview, TasksApi.tasksUpdated, tasks); } } catch (err) { this.logger.warn("Failed to refresh tasks after action", err); @@ -433,7 +437,7 @@ export class TasksPanelProvider private async refreshAndNotifyTask(taskId: string): Promise { try { const task = await this.client.getTask("me", taskId); - this.notify(TasksApi.taskUpdated, task); + notifyWebview(this.view?.webview, TasksApi.taskUpdated, task); } catch (err) { this.logger.warn("Failed to refresh task after action", err); } @@ -522,15 +526,6 @@ export class TasksPanelProvider } } - private notify( - def: NotificationDef, - ...args: D extends void ? [] : [data: D] - ): void { - if (this.view) { - notifyWebview(this.view.webview, def, ...args); - } - } - dispose(): void { this.buildLogStream.close(); this.agentLogStream.close(); diff --git a/src/webviews/util.ts b/src/webviews/util.ts deleted file mode 100644 index 053af95d..00000000 --- a/src/webviews/util.ts +++ /dev/null @@ -1,189 +0,0 @@ -import { randomBytes } from "node:crypto"; -import * as vscode from "vscode"; - -import { toError } from "../error/errorUtils"; -import { type Logger } from "../logging/logger"; - -import type { IpcRequest, IpcResponse, NotificationDef } from "@repo/shared"; - -/** - * Get the HTML content for a webview. - */ -export function getWebviewHtml( - webview: vscode.Webview, - extensionUri: vscode.Uri, - webviewName: string, - title: string, -): string { - const nonce = getNonce(); - const baseUri = vscode.Uri.joinPath( - extensionUri, - "dist", - "webviews", - webviewName, - ); - const scriptUri = webview.asWebviewUri( - vscode.Uri.joinPath(baseUri, "index.js"), - ); - const styleUri = webview.asWebviewUri( - vscode.Uri.joinPath(baseUri, "index.css"), - ); - - // The vscode-elements library looks for a link element with "vscode-codicon-stylesheet" - // ID to load the codicons font inside its shadow DOM components - return ` - - - - - - ${escapeHtml(title)} - - - -
- - -`; -} - -export function getNonce(): string { - return randomBytes(16).toString("base64"); -} - -const HTML_ENTITIES: Record = { - "&": "&", - "<": "<", - ">": ">", - '"': """, - "'": "'", -}; - -/** Escape characters with special meaning in HTML so user-controlled strings - * can be safely interpolated into markup. */ -export function escapeHtml(s: string): string { - return s.replace(/[&<>"']/g, (ch) => HTML_ENTITIES[ch]); -} - -export function isIpcRequest(msg: unknown): msg is IpcRequest { - return ( - typeof msg === "object" && - msg !== null && - "requestId" in msg && - typeof (msg as IpcRequest).requestId === "string" && - "method" in msg && - typeof (msg as IpcRequest).method === "string" - ); -} - -export function isIpcCommand( - msg: unknown, -): msg is { method: string; params?: unknown } { - return ( - typeof msg === "object" && - msg !== null && - !("requestId" in msg) && - "method" in msg && - typeof (msg as { method: string }).method === "string" - ); -} - -/** Push a typed notification to a webview. */ -export function notifyWebview( - webview: vscode.Webview, - def: NotificationDef, - ...args: D extends void ? [] : [data: D] -): void { - webview.postMessage({ - type: def.method, - ...(args.length > 0 ? { data: args[0] } : {}), - }); -} - -export interface DispatchOptions { - logger: Logger; - /** - * Predicate run after a handler throws. If it returns true the error is - * also shown via `showErrorMessage`. Defaults: commands show, requests - * don't (see `dispatchCommand` / `dispatchRequest`). - */ - showErrorToUser?: (method: string) => boolean; -} - -/** - * Dispatch a fire-and-forget command. On failure, logs a warning and (by - * default) shows the error to the user. - */ -export async function dispatchCommand( - message: { method: string; params?: unknown }, - handlers: Record void | Promise>, - options: DispatchOptions, -): Promise { - const { method, params } = message; - const showToUser = options.showErrorToUser ?? (() => true); - try { - const handler = handlers[method]; - if (!handler) { - throw new Error(`Unknown command: ${method}`); - } - await handler(params); - } catch (err) { - const errorMessage = toError(err).message; - options.logger.warn(`Command ${method} failed`, err); - if (showToUser(method)) { - vscode.window.showErrorMessage(errorMessage); - } - } -} - -/** - * Dispatch a request and post a typed response back to the webview. On - * failure, logs a warning, posts an error response, and (if - * `showErrorToUser(method)` returns true) shows the error to the user; - * default is not to show. - * - * If `webview` is undefined (e.g. the view was disposed mid-flight) the - * response is dropped silently. - */ -export async function dispatchRequest( - message: IpcRequest, - handlers: Record Promise>, - webview: vscode.Webview | undefined, - options: DispatchOptions, -): Promise { - const { requestId, method, params } = message; - const showToUser = options.showErrorToUser ?? (() => false); - const respond = (response: IpcResponse) => { - void webview?.postMessage(response); - }; - try { - const handler = handlers[method]; - if (!handler) { - throw new Error(`Unknown method: ${method}`); - } - const data = await handler(params); - respond({ requestId, method, success: true, data }); - } catch (err) { - const errorMessage = toError(err).message; - options.logger.warn(`Request ${method} failed`, err); - respond({ requestId, method, success: false, error: errorMessage }); - if (showToUser(method)) { - vscode.window.showErrorMessage(errorMessage); - } - } -} - -/** - * Fire `handler` on `event` only while `panel.visible` is true. - */ -export function onWhileVisible( - panel: { readonly visible: boolean }, - event: vscode.Event, - handler: () => void, -): vscode.Disposable { - return event(() => { - if (panel.visible) { - handler(); - } - }); -} diff --git a/test/unit/webviews/chat/chatPanelProvider.test.ts b/test/unit/webviews/chat/chatPanelProvider.test.ts index d93ea01c..e27cd792 100644 --- a/test/unit/webviews/chat/chatPanelProvider.test.ts +++ b/test/unit/webviews/chat/chatPanelProvider.test.ts @@ -16,10 +16,7 @@ interface Harness { html: () => string; } -function createHarness(): Harness { - const client = new MockCoderApi(); - client.setCredentials("https://coder.example.com", "test-token"); - +function createHarnessFor(client: MockCoderApi): Harness { const provider = new ChatPanelProvider(client, createMockLogger()); let handler: ((msg: unknown) => void) | null = null; @@ -62,6 +59,12 @@ function createHarness(): Harness { }; } +function createHarness(): Harness { + const client = new MockCoderApi(); + client.setCredentials("https://coder.example.com", "test-token"); + return createHarnessFor(client); +} + function findPostedMessage( postMessage: ReturnType, type: string, @@ -126,6 +129,28 @@ describe("ChatPanelProvider", () => { data: { token: "test-token" }, }); }); + + it("posts auth-error after exhausting retries when token is missing", () => { + vi.useFakeTimers(); + try { + const client = new MockCoderApi(); + client.setCredentials("https://coder.example.com", undefined); + const { sendFromWebview, postMessage } = createHarnessFor(client); + + sendFromWebview({ method: "coder:vscode-ready" }); + // 5 retries with base 500ms exponential backoff. + vi.advanceTimersByTime(500 + 1000 + 2000 + 4000 + 8000); + + expect(findPostedMessage(postMessage, "coder:auth-error")).toEqual({ + type: "coder:auth-error", + data: { + error: "No session token available. Please sign in and retry.", + }, + }); + } finally { + vi.useRealTimers(); + } + }); }); describe("navigation", () => { diff --git a/test/unit/webviews/dispatch.test.ts b/test/unit/webviews/dispatch.test.ts new file mode 100644 index 00000000..33338196 --- /dev/null +++ b/test/unit/webviews/dispatch.test.ts @@ -0,0 +1,202 @@ +import { beforeEach, describe, expect, it, vi } from "vitest"; +import * as vscode from "vscode"; + +import { + dispatchCommand, + dispatchRequest, + notifyWebview, + onWhileVisible, +} from "@/webviews/dispatch"; + +import { defineNotification } from "@repo/shared"; + +import { createMockLogger } from "../../mocks/testHelpers"; + +const logger = createMockLogger(); +const showError = vi.mocked(vscode.window.showErrorMessage); + +beforeEach(() => { + showError.mockClear(); +}); + +function makeMockWebview() { + const posted: unknown[] = []; + const w: vscode.Webview = { + options: { enableScripts: true, localResourceRoots: [] }, + html: "", + cspSource: "mock-csp", + onDidReceiveMessage: () => ({ dispose: () => undefined }), + postMessage: (msg) => { + posted.push(msg); + return Promise.resolve(true); + }, + asWebviewUri: (uri) => uri, + }; + return { webview: w, posted }; +} + +describe("notifyWebview", () => { + it("posts {type, data} for payload notifications", () => { + const { webview, posted } = makeMockWebview(); + notifyWebview( + webview, + defineNotification<{ count: number }>("ns/updated"), + { count: 7 }, + ); + expect(posted).toEqual([{ type: "ns/updated", data: { count: 7 } }]); + }); + + it("omits the data field for void notifications", () => { + const { webview, posted } = makeMockWebview(); + notifyWebview(webview, defineNotification("ns/refresh")); + expect(posted).toEqual([{ type: "ns/refresh" }]); + }); + + it("is a no-op when webview is undefined", () => { + expect(() => + notifyWebview(undefined, defineNotification<{ x: number }>("ns/evt"), { + x: 1, + }), + ).not.toThrow(); + }); +}); + +describe("dispatchCommand", () => { + it("invokes the matching handler with params", async () => { + const handler = vi.fn(); + await dispatchCommand( + { method: "do", params: { id: 1 } }, + { do: handler }, + { logger }, + ); + expect(handler).toHaveBeenCalledWith({ id: 1 }); + }); + + it("does not show errors by default when a handler throws", async () => { + await dispatchCommand( + { method: "do" }, + { do: vi.fn().mockRejectedValue(new Error("kaboom")) }, + { logger }, + ); + expect(showError).not.toHaveBeenCalled(); + }); + + it("does not show errors by default for unknown commands", async () => { + await dispatchCommand({ method: "missing" }, {}, { logger }); + expect(showError).not.toHaveBeenCalled(); + }); + + it("shows errors when showErrorToUser opts in", async () => { + await dispatchCommand( + { method: "do" }, + { do: vi.fn().mockRejectedValue(new Error("kaboom")) }, + { logger, showErrorToUser: () => true }, + ); + expect(showError).toHaveBeenCalledWith("kaboom"); + }); +}); + +describe("dispatchRequest", () => { + it("posts a success response with the handler's return value", async () => { + const { webview, posted } = makeMockWebview(); + await dispatchRequest( + { requestId: "r1", method: "get", params: { id: 1 } }, + { get: vi.fn().mockResolvedValue({ ok: true }) }, + webview, + { logger }, + ); + expect(posted).toEqual([ + { requestId: "r1", method: "get", success: true, data: { ok: true } }, + ]); + }); + + it("posts a failure response with the handler's error message", async () => { + const { webview, posted } = makeMockWebview(); + await dispatchRequest( + { requestId: "r2", method: "get" }, + { get: vi.fn().mockRejectedValue(new Error("boom")) }, + webview, + { logger }, + ); + expect(posted).toEqual([ + { requestId: "r2", method: "get", success: false, error: "boom" }, + ]); + expect(showError).not.toHaveBeenCalled(); + }); + + it("posts an Unknown request error for missing handlers", async () => { + const { webview, posted } = makeMockWebview(); + await dispatchRequest({ requestId: "r3", method: "missing" }, {}, webview, { + logger, + }); + expect(posted).toEqual([ + { + requestId: "r3", + method: "missing", + success: false, + error: "Unknown request: missing", + }, + ]); + }); + + it("shows errors only for methods that opt in", async () => { + const { webview } = makeMockWebview(); + await dispatchRequest( + { requestId: "r4", method: "delete" }, + { delete: vi.fn().mockRejectedValue(new Error("nope")) }, + webview, + { logger, showErrorToUser: (m) => m === "delete" }, + ); + expect(showError).toHaveBeenCalledWith("nope"); + }); + + it("drops the response silently when the webview is undefined", async () => { + await expect( + dispatchRequest( + { requestId: "r5", method: "get" }, + { get: vi.fn().mockResolvedValue("done") }, + undefined, + { logger }, + ), + ).resolves.toBeUndefined(); + }); +}); + +describe("onWhileVisible", () => { + function makePanel(visible: boolean) { + const listeners = new Set<() => void>(); + const event: vscode.Event = (cb) => { + listeners.add(cb as () => void); + return { dispose: () => listeners.delete(cb as () => void) }; + }; + return { + panel: { visible }, + event, + fire: () => listeners.forEach((l) => l()), + }; + } + + it("fires when the panel is visible", () => { + const { panel, event, fire } = makePanel(true); + const handler = vi.fn(); + onWhileVisible(panel, event, handler); + fire(); + expect(handler).toHaveBeenCalledTimes(1); + }); + + it("skips when the panel is hidden", () => { + const { panel, event, fire } = makePanel(false); + const handler = vi.fn(); + onWhileVisible(panel, event, handler); + fire(); + expect(handler).not.toHaveBeenCalled(); + }); + + it("unsubscribes on dispose", () => { + const { panel, event, fire } = makePanel(true); + const handler = vi.fn(); + onWhileVisible(panel, event, handler).dispose(); + fire(); + expect(handler).not.toHaveBeenCalled(); + }); +}); diff --git a/test/unit/webviews/util.test.ts b/test/unit/webviews/html.test.ts similarity index 99% rename from test/unit/webviews/util.test.ts rename to test/unit/webviews/html.test.ts index ab0ec272..9e0093a4 100644 --- a/test/unit/webviews/util.test.ts +++ b/test/unit/webviews/html.test.ts @@ -1,7 +1,7 @@ import { describe, expect, it } from "vitest"; import * as vscode from "vscode"; -import { escapeHtml, getNonce, getWebviewHtml } from "@/webviews/util"; +import { escapeHtml, getNonce, getWebviewHtml } from "@/webviews/html"; const webview: vscode.Webview = { options: { enableScripts: true, localResourceRoots: [] }, diff --git a/test/unit/webviews/tasks/tasksPanelProvider.test.ts b/test/unit/webviews/tasks/tasksPanelProvider.test.ts index b49b3e8a..ec804ba3 100644 --- a/test/unit/webviews/tasks/tasksPanelProvider.test.ts +++ b/test/unit/webviews/tasks/tasksPanelProvider.test.ts @@ -788,7 +788,7 @@ describe("TasksPanelProvider", () => { const res = await h.request(defineRequest("unknownMethod")); expect(res.success).toBe(false); - expect(res.error).toContain("Unknown method"); + expect(res.error).toContain("Unknown request"); }); it("createTask succeeds even when refreshing the task list fails", async () => { diff --git a/test/webview/shared/ipc.test.ts b/test/webview/shared/ipc.test.ts index 32130b06..7fa9a81c 100644 --- a/test/webview/shared/ipc.test.ts +++ b/test/webview/shared/ipc.test.ts @@ -31,11 +31,11 @@ describe("sendCommand", () => { expect(sent).toEqual([{ method: "ns/doThing", params: { id: "42" } }]); }); - it("posts without params for void-payload commands", () => { + it("omits params for void-payload commands", () => { const cmd = defineCommand("ns/noop"); sendCommand(cmd); - expect(sent).toEqual([{ method: "ns/noop", params: undefined }]); + expect(sent).toEqual([{ method: "ns/noop" }]); }); }); @@ -92,6 +92,19 @@ describe("onNotification", () => { unsubscribe(); }); + it("fires for void notifications (no data field)", () => { + const def = defineNotification("ns/ping"); + const cb = vi.fn(); + const unsubscribe = onNotification(def, cb); + + window.dispatchEvent( + new MessageEvent("message", { data: { type: "ns/ping" } }), + ); + expect(cb).toHaveBeenCalledWith(undefined); + + unsubscribe(); + }); + it("supports multiple independent subscribers", () => { const def = defineNotification("ns/count"); const a = vi.fn(); From b54d26495c254c091b5c301762a6374e93dcc27e Mon Sep 17 00:00:00 2001 From: Ehab Younes Date: Thu, 30 Apr 2026 17:20:56 +0300 Subject: [PATCH 4/6] refactor: move chat shim into a typed bundle Replaces the inline JS shim in chatPanelProvider with a real webview package at packages/chat/. The shim now imports ChatApi and uses sendCommand and subscribeNotifications, so renaming an entry on the API breaks the build at every callsite. Iframe and status div are still pre-rendered in the panel HTML so the iframe loads as soon as the page parses, matching the original code path; the bundle just attaches listeners. Adds subscribeNotifications to webview-shared, an exhaustive notification subscriber driven by a single window message listener and a Map dispatcher. Removes the public onNotification export; useIpc still uses the lower-level subscribeOne for per-call React subscriptions. Speedtest migrates to subscribeNotifications too. Pulls a buildWebviewCsp helper out of html.ts so the chat panel and the standard getWebviewHtml share one CSP source of truth, and adds getWebviewAssetUris for panels that compose their own HTML. README rewritten as a navigation map with a "Where each handler lives" table and pointers to the canonical reference packages. --- packages/chat/package.json | 21 ++ packages/chat/src/css.d.ts | 1 + packages/chat/src/index.css | 39 ++++ packages/chat/src/index.ts | 79 ++++++++ packages/chat/tsconfig.json | 10 + packages/chat/vite.config.ts | 3 + packages/shared/src/chat/api.ts | 5 +- packages/shared/src/ipc/protocol.ts | 9 + packages/speedtest/src/index.ts | 24 +-- packages/webview-shared/README.md | 180 ++++++------------ packages/webview-shared/src/index.ts | 4 +- packages/webview-shared/src/ipc.ts | 47 ++++- packages/webview-shared/src/react/useIpc.ts | 4 +- pnpm-lock.yaml | 19 ++ src/extension.ts | 6 +- src/webviews/chat/chatPanelProvider.ts | 172 ++++------------- src/webviews/html.ts | 53 ++++-- .../webviews/chat/chatPanelProvider.test.ts | 7 +- test/unit/webviews/html.test.ts | 2 +- test/webview/shared/ipc.test.ts | 102 ++++------ 20 files changed, 429 insertions(+), 358 deletions(-) create mode 100644 packages/chat/package.json create mode 100644 packages/chat/src/css.d.ts create mode 100644 packages/chat/src/index.css create mode 100644 packages/chat/src/index.ts create mode 100644 packages/chat/tsconfig.json create mode 100644 packages/chat/vite.config.ts diff --git a/packages/chat/package.json b/packages/chat/package.json new file mode 100644 index 00000000..4669c503 --- /dev/null +++ b/packages/chat/package.json @@ -0,0 +1,21 @@ +{ + "name": "@repo/chat", + "version": "1.0.0", + "description": "Coder chat iframe shim webview", + "private": true, + "type": "module", + "scripts": { + "build": "vite build", + "dev": "vite build --watch", + "typecheck": "tsc --noEmit" + }, + "dependencies": { + "@repo/shared": "workspace:*", + "@repo/webview-shared": "workspace:*" + }, + "devDependencies": { + "@types/vscode-webview": "catalog:", + "typescript": "catalog:", + "vite": "catalog:" + } +} diff --git a/packages/chat/src/css.d.ts b/packages/chat/src/css.d.ts new file mode 100644 index 00000000..cbe652db --- /dev/null +++ b/packages/chat/src/css.d.ts @@ -0,0 +1 @@ +declare module "*.css"; diff --git a/packages/chat/src/index.css b/packages/chat/src/index.css new file mode 100644 index 00000000..14fac54a --- /dev/null +++ b/packages/chat/src/index.css @@ -0,0 +1,39 @@ +html, +body { + margin: 0; + padding: 0; + width: 100%; + height: 100%; + overflow: hidden; + background: var(--vscode-editor-background, #1e1e1e); +} + +iframe { + border: none; + width: 100%; + height: 100%; +} + +#status { + color: var(--vscode-foreground, #ccc); + font-family: var(--vscode-font-family, sans-serif); + font-size: 13px; + padding: 16px; + text-align: center; +} + +#retry-btn { + margin-top: 12px; + padding: 6px 16px; + background: var(--vscode-button-background, #0e639c); + color: var(--vscode-button-foreground, #fff); + border: none; + border-radius: 2px; + cursor: pointer; + font-family: var(--vscode-font-family, sans-serif); + font-size: 13px; +} + +#retry-btn:hover { + background: var(--vscode-button-hoverBackground, #1177bb); +} diff --git a/packages/chat/src/index.ts b/packages/chat/src/index.ts new file mode 100644 index 00000000..4062734b --- /dev/null +++ b/packages/chat/src/index.ts @@ -0,0 +1,79 @@ +import { ChatApi } from "@repo/shared"; +import { sendCommand, subscribeNotifications } from "@repo/webview-shared"; + +import "./index.css"; + +/** + * Chat shim. Bridges the iframe's foreign `{ type, payload }` protocol + * and the extension's `ChatApi`. The iframe and status div are + * pre-rendered in the panel's HTML; the bundle just attaches listeners. + */ + +const iframe = document.getElementById("chat-frame") as HTMLIFrameElement; +const status = document.getElementById("status") as HTMLDivElement; +const allowedOrigin = new URL(iframe.src).origin; + +iframe.addEventListener("load", () => { + iframe.style.display = "block"; + status.style.display = "none"; +}); + +function toIframe(type: string, payload: unknown): void { + iframe.contentWindow?.postMessage({ type, payload }, allowedOrigin); +} + +function showRetry(error: string): void { + status.textContent = ""; + status.appendChild( + document.createTextNode(error || "Authentication failed."), + ); + const btn = document.createElement("button"); + btn.id = "retry-btn"; + btn.textContent = "Retry"; + btn.addEventListener("click", () => { + status.textContent = "Authenticating…"; + sendCommand(ChatApi.vscodeReady); + }); + status.appendChild(document.createElement("br")); + status.appendChild(btn); + status.style.display = "block"; + iframe.style.display = "none"; +} + +// Compile-checked: a new ChatApi notification without a handler fails the build. +subscribeNotifications(ChatApi, { + setTheme: ({ theme }) => toIframe("coder:set-theme", { theme }), + authBootstrapToken: ({ token }) => { + status.textContent = "Signing in…"; + toIframe("coder:vscode-auth-bootstrap", { token }); + }, + authError: ({ error }) => showRetry(error), +}); + +// Iframe -> extension. `msg.type` strings are the foreign Coder protocol; +// every `sendCommand(ChatApi.X)` below is still type-checked. +window.addEventListener("message", (event) => { + if (event.source !== iframe.contentWindow) { + return; + } + if (typeof event.data !== "object" || event.data === null) { + return; + } + const msg = event.data as { type?: string; payload?: { url?: string } }; + switch (msg.type) { + case "coder:vscode-ready": + status.textContent = "Authenticating…"; + sendCommand(ChatApi.vscodeReady); + return; + case "coder:chat-ready": + sendCommand(ChatApi.chatReady); + return; + case "coder:navigate": + if (msg.payload?.url) { + sendCommand(ChatApi.navigate, { url: msg.payload.url }); + } + return; + default: + return; + } +}); diff --git a/packages/chat/tsconfig.json b/packages/chat/tsconfig.json new file mode 100644 index 00000000..e1940bf7 --- /dev/null +++ b/packages/chat/tsconfig.json @@ -0,0 +1,10 @@ +{ + "extends": "../tsconfig.packages.json", + "compilerOptions": { + "paths": { + "@repo/shared": ["../shared/src"], + "@repo/webview-shared": ["../webview-shared/src"] + } + }, + "include": ["src"] +} diff --git a/packages/chat/vite.config.ts b/packages/chat/vite.config.ts new file mode 100644 index 00000000..cb12ddf2 --- /dev/null +++ b/packages/chat/vite.config.ts @@ -0,0 +1,3 @@ +import { createWebviewConfig } from "../webview-shared/createWebviewConfig"; + +export default createWebviewConfig("chat", __dirname); diff --git a/packages/shared/src/chat/api.ts b/packages/shared/src/chat/api.ts index 4a95fc79..2761ada3 100644 --- a/packages/shared/src/chat/api.ts +++ b/packages/shared/src/chat/api.ts @@ -1,9 +1,6 @@ import { defineCommand, defineNotification } from "../ipc/protocol"; -/** - * Chat webview API. The inline shim in `chatPanelProvider.ts` mirrors - * these entries; missed updates fail silently at runtime. - */ +/** Chat webview API. */ export const ChatApi = { /** Iframe reports it needs the session token. */ vscodeReady: defineCommand("coder:vscode-ready"), diff --git a/packages/shared/src/ipc/protocol.ts b/packages/shared/src/ipc/protocol.ts index 98d55deb..b57d1dc3 100644 --- a/packages/shared/src/ipc/protocol.ts +++ b/packages/shared/src/ipc/protocol.ts @@ -96,6 +96,15 @@ export type CommandHandlerMap = { : never; }; +/** Requires a subscriber for every NotificationDef in Api. Compile error if one is missing. */ +export type NotificationHandlerMap = { + [K in keyof Api as Api[K] extends { kind: "notification" } + ? K + : never]: Api[K] extends NotificationDef + ? (data: D) => void + : never; +}; + // --- API hook type --- /** Derives a fully typed hook interface from an API definition object. */ diff --git a/packages/speedtest/src/index.ts b/packages/speedtest/src/index.ts index d90879d4..476f0071 100644 --- a/packages/speedtest/src/index.ts +++ b/packages/speedtest/src/index.ts @@ -1,5 +1,5 @@ import { SpeedtestApi, type SpeedtestResult, toError } from "@repo/shared"; -import { onNotification, sendCommand } from "@repo/webview-shared"; +import { sendCommand, subscribeNotifications } from "@repo/webview-shared"; import { renderLineChart } from "./chart"; import { @@ -20,18 +20,20 @@ const TOOLTIP_GAP_PX = 32; let cleanup: (() => void) | undefined; function main(): void { - onNotification(SpeedtestApi.data, ({ workspaceId, result }) => { - try { - cleanup?.(); - cleanup = renderPage(result, workspaceId, () => - sendCommand(SpeedtestApi.viewJson), - ); - } catch (err) { - showError(`Failed to render speedtest: ${toError(err).message}`); - } + subscribeNotifications(SpeedtestApi, { + data: ({ workspaceId, result }) => { + try { + cleanup?.(); + cleanup = renderPage(result, workspaceId, () => + sendCommand(SpeedtestApi.viewJson), + ); + } catch (err) { + showError(`Failed to render speedtest: ${toError(err).message}`); + } + }, }); // Signal we're subscribed; the extension waits for this before sending. - postMessage({ method: SpeedtestApi.ready.method }); + sendCommand(SpeedtestApi.ready); } function renderPage( diff --git a/packages/webview-shared/README.md b/packages/webview-shared/README.md index 7013fe9c..4756e2ce 100644 --- a/packages/webview-shared/README.md +++ b/packages/webview-shared/README.md @@ -4,139 +4,85 @@ Typed messaging between the extension and VS Code webviews. Both sides import the same `Api` definition, so wire formats can't drift and method typos fail at compile time. -## Three message kinds - -```ts -defineNotification(method); // extension to webview, fire-and-forget -defineCommand

(method); // webview to extension, fire-and-forget -defineRequest(method); // webview to extension, awaits response -``` - -Define them all in one `Api` so both sides share the strings: - -```ts -// packages/shared/src/myfeature/api.ts -export const MyFeatureApi = { - data: defineNotification("myfeature/data"), - doThing: defineCommand<{ id: string }>("myfeature/doThing"), - getThings: defineRequest("myfeature/getThings"), -} as const; -``` - -## Extension side (`src/webviews/...`) - -Push notifications: - -```ts -import { notifyWebview } from "../dispatch"; +This file is a map; the helpers themselves are the source of truth. When +something here looks wrong, trust the linked source. -notifyWebview(panel.webview, MyFeatureApi.data, payload); -``` - -Receive commands and requests. Every panel builds both handler maps (an -empty `{}` is fine). The maps are mapped over the `Api`, so adding a new -`defineCommand` or `defineRequest` produces a compile error in any panel -missing a handler. That's how new methods can't ship and silently drop. - -```ts -import { buildCommandHandlers, buildRequestHandlers } from "@repo/shared"; -import { - dispatchCommand, - dispatchRequest, - isIpcCommand, - isIpcRequest, -} from "../dispatch"; - -const commandHandlers = buildCommandHandlers(MyFeatureApi, { - doThing: async (p) => { ... }, -}); -// Empty is fine; it still locks in future additions. -const requestHandlers = buildRequestHandlers(MyFeatureApi, { - getThings: async () => this.fetchThings(), -}); - -panel.webview.onDidReceiveMessage((message: unknown) => { - if (isIpcRequest(message)) { - void dispatchRequest(message, requestHandlers, panel.webview, { - logger, - showErrorToUser: (m) => USER_ACTION_METHODS.has(m), - }); - } else if (isIpcCommand(message)) { - void dispatchCommand(message, commandHandlers, { logger }); - } -}); -``` +## Three message kinds -### Error handling +Defined in `packages/shared/src/ipc/protocol.ts`: -Both dispatchers log handler failures via `logger.warn`. Requests also -post a `success: false` response so the webview's awaited promise rejects. +- `defineNotification(method)` - extension to webview, fire-and-forget +- `defineCommand

(method)` - webview to extension, fire-and-forget +- `defineRequest(method)` - webview to extension, awaits a response -Neither pops a dialog by default. Pass `showErrorToUser: (method) => ...` -to opt in for methods where a silent failure would confuse the user. +Group them in one `Api` const at `packages/shared/src//api.ts` +(see `chat/api.ts`, `speedtest/api.ts`, `tasks/api.ts`). -## Webview side, vanilla +## Where each handler lives -```ts -import { MyFeatureApi } from "@repo/shared"; -import { onNotification, sendCommand } from "@repo/webview-shared"; +| Direction | Define | Extension side (`src/webviews/...`) | Webview vanilla | Webview React | +| ------------------------------------- | ----------------------- | ------------------------------------------------------ | ----------------------------- | ----------------------------------- | +| extension -> webview push | `defineNotification` | `notifyWebview(view, def, data)` | `subscribeNotifications(api)` | `apiHook.on(cb)` via `useIpc` | +| webview -> extension, fire-and-forget | `defineCommand

` | handler in `buildCommandHandlers` -> `dispatchCommand` | `sendCommand(def, params)` | `apiHook.(params)` | +| webview -> extension, awaits response | `defineRequest` | handler in `buildRequestHandlers` -> `dispatchRequest` | _not exposed; use a command_ | `await apiHook.(params)` | -const unsubscribe = onNotification(MyFeatureApi.data, (payload) => { - render(payload); -}); +Compile-time exhaustiveness fails the build in three places: -sendCommand(MyFeatureApi.doThing, { id: "42" }); -``` +- **Extension**: `buildCommandHandlers(Api, ...)` / `buildRequestHandlers(Api, ...)` (`packages/shared/src/ipc/protocol.ts`). +- **Webview vanilla**: `subscribeNotifications(Api, ...)` (`packages/webview-shared/src/ipc.ts`). +- **Webview React**: `apiHook` is generated from `Api` so every notification has a typed accessor (`buildApiHook` in `packages/shared/src/ipc/protocol.ts`). -Call the returned unsubscribe function on cleanup. +## Reference implementations -## Webview side, React +Use these as the working blueprint when writing a new webview. The +helpers' JSDoc covers their contracts. -Use `useIpc` from `packages/webview-shared/src/react/useIpc`. Same -semantics, plus request/response correlation with timeouts and UUID -bookkeeping. +| Concern | Look at | +| --------------------------------------- | ----------------------------------------------------------- | +| Vanilla webview package | `packages/speedtest/` (or `packages/chat/`) | +| React webview package | `packages/tasks/` | +| Extension panel (`WebviewPanel`) | `src/webviews/speedtest/speedtestPanelFactory.ts` | +| Extension panel (`WebviewViewProvider`) | `src/webviews/tasks/tasksPanelProvider.ts` | +| Iframe-embedding panel | `src/webviews/chat/chatPanelProvider.ts` + `packages/chat/` | +| Vite config helper | `packages/webview-shared/createWebviewConfig.ts` | +| Dispatch / lifecycle helpers | `src/webviews/dispatch.ts` | +| HTML scaffolding | `src/webviews/html.ts` | ## Re-sending on lifecycle events -Webviews lose state in two ways the extension has to compensate for. - -**Hidden webviews are destroyed.** Switching tabs discards in-memory state, -listeners, and canvas pixels. Push-driven panels resend when the webview -comes back. Subscribe to `panel.onDidChangeViewState` (`WebviewPanel`) or -`view.onDidChangeVisibility` (`WebviewViewProvider`) and resend on -`visible`. Setting `retainContextWhenHidden` avoids this but is costly, -so we don't. - -**Theme changes don't repaint canvases.** CSS vars update automatically, -but canvas and SVG drawn imperatively bake the theme into pixels. -Subscribe to `vscode.window.onDidChangeActiveColorTheme` and resend so -the webview redraws. This applies regardless of `retainContextWhenHidden`. +Webviews lose state in two ways the extension has to compensate for: -`onWhileVisible(panel, event, handler)` in `src/webviews/dispatch.ts` -works with both panel shapes. Collect disposables and clear them in -`onDidDispose`. +1. **Hidden webviews are destroyed** unless `retainContextWhenHidden` is + set (costly; we don't). Push panels must resend when the webview + comes back visible. +2. **Theme changes don't repaint canvases** (CSS vars update DOM but + imperative canvas/SVG bake the theme into pixels). Resend on + `vscode.window.onDidChangeActiveColorTheme` regardless of + `retainContextWhenHidden`. -See `speedtestPanelFactory.ts` for a `WebviewPanel` example and -`tasksPanelProvider.ts` for a `WebviewViewProvider` example. +`onWhileVisible` in `src/webviews/dispatch.ts` wraps both. See its JSDoc +and the `disposables` array in `speedtestPanelFactory.ts` for usage. ## Checklist for a new webview -1. Register the view in `package.json` under `contributes.views` (sidebar) - or call `vscode.window.createWebviewPanel` (editor tab). -2. Register the provider in `src/extension.ts`. -3. Define the API in `packages/shared/src//api.ts` and export it - from `packages/shared/src/index.ts`. -4. Build both handler maps with `buildCommandHandlers` and - `buildRequestHandlers` (empty `{}` is fine). -5. Dispatch with `isIpcRequest` -> `dispatchRequest` and `isIpcCommand` -> - `dispatchCommand`, both with a logger. -6. Use `onWhileVisible` for `onDidChangeViewState` / - `onDidChangeVisibility` and `onDidChangeActiveColorTheme`, and dispose - in `onDidDispose`. -7. On the webview side, use `onNotification` / `sendCommand` (vanilla) or - `useIpc` (React). Don't hand-roll `window.addEventListener("message", -...)` in webview package code. The one exception is an inline HTML - shim bridging a third-party iframe (see `chatPanelProvider.ts`); keep - it small. -8. Tests: assert the panel posts the expected payload shape, resends on - visibility and theme, and handles incoming commands. +1. **Shared API**: add `packages/shared/src//api.ts` and + re-export from `packages/shared/src/index.ts`. +2. **Webview package**: copy `packages/speedtest/` (vanilla) or + `packages/tasks/` (React). The `vite.config.ts` is one line. +3. **Webview entry**: subscribe with `subscribeNotifications` (vanilla) + or `useIpc` + the generated `apiHook` (React). Don't hand-roll + `window.addEventListener("message", ...)`. +4. **Extension panel**: register in `package.json` under + `contributes.views` (sidebar) or via `vscode.window.createWebviewPanel` + (editor tab), and wire the provider in `src/extension.ts`. +5. **Extension dispatch**: build both handler maps with + `buildCommandHandlers` / `buildRequestHandlers` (empty `{}` is what + locks in future exhaustiveness), then dispatch with + `isIpcRequest` -> `dispatchRequest` and `isIpcCommand` -> + `dispatchCommand`. Pass `showErrorToUser` for user-initiated methods. +6. **Lifecycle**: use `onWhileVisible` for `onDidChangeViewState` / + `onDidChangeVisibility` and `onDidChangeActiveColorTheme`; dispose in + `onDidDispose`. +7. **Tests**: assert the panel posts the expected payload shape, + resends on visibility and theme, and handles incoming commands and + requests. See `test/unit/webviews/` for patterns. diff --git a/packages/webview-shared/src/index.ts b/packages/webview-shared/src/index.ts index 01d83d22..a78b101d 100644 --- a/packages/webview-shared/src/index.ts +++ b/packages/webview-shared/src/index.ts @@ -8,5 +8,5 @@ export interface WebviewMessage { // VS Code state API export { getState, setState, postMessage } from "./api"; -// Typed IPC helpers for vanilla webviews -export { sendCommand, onNotification } from "./ipc"; +// IPC helpers for vanilla webviews. React webviews use `useIpc` instead. +export { sendCommand, subscribeNotifications } from "./ipc"; diff --git a/packages/webview-shared/src/ipc.ts b/packages/webview-shared/src/ipc.ts index 9aedf5f1..4c748265 100644 --- a/packages/webview-shared/src/ipc.ts +++ b/packages/webview-shared/src/ipc.ts @@ -1,11 +1,12 @@ -/** - * Typed IPC helpers for vanilla-TS webviews. React webviews should use - * `useIpc` (./react/useIpc), which adds request/response correlation. - */ +/** Typed IPC helpers for webviews. React layers `useIpc` for request/response. */ import { postMessage } from "./api"; -import type { CommandDef, NotificationDef } from "@repo/shared"; +import type { + CommandDef, + NotificationDef, + NotificationHandlerMap, +} from "@repo/shared"; /** Send a fire-and-forget command to the extension. */ export function sendCommand

( @@ -19,11 +20,39 @@ export function sendCommand

( } /** - * Subscribe to a typed notification from the extension. Returns an - * unsubscribe function; call it on cleanup. Multiple subscribers are - * invoked in registration order. + * Exhaustively subscribe to every notification on `api`. Compile error + * if any notification lacks a handler. Returns a single unsubscribe. + */ +export function subscribeNotifications< + Api extends Record, +>(api: Api, handlers: NotificationHandlerMap): () => void; +export function subscribeNotifications( + api: Record, + handlers: Record void>, +): () => void { + const byMethod = new Map void>(); + for (const [key, def] of Object.entries(api)) { + if (def.kind === "notification") { + byMethod.set(def.method, handlers[key]); + } + } + const handler = (event: MessageEvent) => { + const msg = event.data; + if (typeof msg !== "object" || msg === null) { + return; + } + const cb = byMethod.get((msg as { type?: string }).type ?? ""); + cb?.((msg as { data: unknown }).data); + }; + window.addEventListener("message", handler); + return () => window.removeEventListener("message", handler); +} + +/** + * Single-notification subscribe. React's `useIpc` uses this for + * `apiHook.on`. Vanilla webviews should use `subscribeNotifications`. */ -export function onNotification( +export function subscribeOne( def: NotificationDef, callback: (data: D) => void, ): () => void { diff --git a/packages/webview-shared/src/react/useIpc.ts b/packages/webview-shared/src/react/useIpc.ts index c6c0d0ed..90952359 100644 --- a/packages/webview-shared/src/react/useIpc.ts +++ b/packages/webview-shared/src/react/useIpc.ts @@ -6,7 +6,7 @@ import { useEffect, useRef } from "react"; import { postMessage } from "../api"; -import { onNotification as subscribe } from "../ipc"; +import { subscribeOne } from "../ipc"; import type { CommandDef, @@ -143,7 +143,7 @@ export function useIpc(options: UseIpcOptions = {}) { definition: NotificationDef, callback: (data: D) => void, ): () => void { - return subscribe(definition, callback); + return subscribeOne(definition, callback); } return { request, command, onNotification }; diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 42887668..efd06625 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -262,6 +262,25 @@ importers: specifier: ^4.1.5 version: 4.1.5(@types/node@22.19.17)(@vitest/coverage-v8@4.1.5)(jsdom@29.1.0)(vite@8.0.10(@types/node@22.19.17)(esbuild@0.28.0)) + packages/chat: + dependencies: + '@repo/shared': + specifier: workspace:* + version: link:../shared + '@repo/webview-shared': + specifier: workspace:* + version: link:../webview-shared + devDependencies: + '@types/vscode-webview': + specifier: 'catalog:' + version: 1.57.5 + typescript: + specifier: 'catalog:' + version: 6.0.3 + vite: + specifier: 'catalog:' + version: 8.0.10(@types/node@24.10.12)(esbuild@0.28.0) + packages/shared: devDependencies: typescript: diff --git a/src/extension.ts b/src/extension.ts index cb38ecb3..c8aad5c1 100644 --- a/src/extension.ts +++ b/src/extension.ts @@ -224,7 +224,11 @@ export async function activate(ctx: vscode.ExtensionContext): Promise { ); // Register Chat embed panel with dependencies - const chatPanelProvider = new ChatPanelProvider(client, output); + const chatPanelProvider = new ChatPanelProvider( + ctx.extensionUri, + client, + output, + ); ctx.subscriptions.push( chatPanelProvider, vscode.window.registerWebviewViewProvider( diff --git a/src/webviews/chat/chatPanelProvider.ts b/src/webviews/chat/chatPanelProvider.ts index 3d1d8d6e..b655e9f8 100644 --- a/src/webviews/chat/chatPanelProvider.ts +++ b/src/webviews/chat/chatPanelProvider.ts @@ -15,21 +15,18 @@ import { isIpcRequest, notifyWebview, } from "../dispatch"; -import { getNonce } from "../html"; +import { buildWebviewCsp, getNonce, getWebviewAssetUris } from "../html"; /** - * Provides a webview that embeds the Coder agent chat UI. - * Authentication flows through postMessage: + * Webview that embeds the Coder agent chat UI inside an iframe. The + * panel's HTML pre-renders the iframe with `src=embedUrl`; the + * `@repo/chat` bundle attaches listeners and bridges the iframe's + * foreign `{ type, payload }` protocol to `ChatApi`. Auth flow: * - * 1. The iframe loads /agents/{id}/embed on the Coder server. - * 2. The embed page detects the user is signed out and posts - * { type: "coder:vscode-ready" } to window.parent. - * 3. Our shim relays that to the extension as a ChatApi.vscodeReady - * command. - * 4. The extension pushes the session token back with - * ChatApi.authBootstrapToken. - * 5. The shim forwards it into the iframe, which calls - * API.setSessionToken and re-fetches the user. + * 1. Iframe loads /agents/{id}/embed and posts `coder:vscode-ready`. + * 2. Bundle forwards as `ChatApi.vscodeReady`. + * 3. Extension responds with `ChatApi.authBootstrapToken`. + * 4. Bundle forwards the token into the iframe. */ export class ChatPanelProvider implements vscode.WebviewViewProvider, vscode.Disposable @@ -49,6 +46,7 @@ export class ChatPanelProvider private readonly requestHandlers = buildRequestHandlers(ChatApi, {}); constructor( + private readonly extensionUri: vscode.Uri, private readonly client: Pick, private readonly logger: Logger, ) {} @@ -92,7 +90,12 @@ export class ChatPanelProvider // duplicates if VS Code re-resolves the view. this.disposeView(); this.view = webviewView; - webviewView.webview.options = { enableScripts: true }; + webviewView.webview.options = { + enableScripts: true, + localResourceRoots: [ + vscode.Uri.joinPath(this.extensionUri, "dist", "webviews", "chat"), + ], + }; this.disposables.push( webviewView.webview.onDidReceiveMessage((message: unknown) => { if (isIpcRequest(message)) { @@ -128,20 +131,13 @@ export class ChatPanelProvider throw new Error("renderView called before resolveWebviewView"); } const webview = this.view.webview; - - if (!this.chatId) { - webview.html = this.getNoAgentHtml(); - return; - } - const coderUrl = this.client.getHost(); - if (!coderUrl) { + if (!this.chatId || !coderUrl) { webview.html = this.getNoAgentHtml(); return; } - const embedUrl = `${coderUrl}/agents/${this.chatId}/embed?theme=${this.getTheme()}`; - webview.html = this.getIframeHtml(embedUrl, coderUrl); + webview.html = this.getEmbedHtml(webview, embedUrl); } private handleNavigate(url: string): void { @@ -198,131 +194,31 @@ export class ChatPanelProvider notifyWebview(this.view?.webview, ChatApi.authBootstrapToken, { token }); } - private getIframeHtml(embedUrl: string, allowedOrigin: string): string { + /** + * Pre-renders the iframe and adds `frame-src` to the CSP. The bundle + * attaches listeners; it doesn't construct the iframe. + */ + private getEmbedHtml(webview: vscode.Webview, embedUrl: string): string { const nonce = getNonce(); - - return /* html */ ` + const frameSrc = new URL(embedUrl).origin; + const { scriptUri, styleUri } = getWebviewAssetUris( + webview, + this.extensionUri, + "chat", + ); + return ` - + Coder Chat - +

Loading chat…
- - + + `; } diff --git a/src/webviews/html.ts b/src/webviews/html.ts index b3216907..5c47ceb5 100644 --- a/src/webviews/html.ts +++ b/src/webviews/html.ts @@ -1,25 +1,56 @@ import { randomBytes } from "node:crypto"; import * as vscode from "vscode"; -/** Get the HTML content for a webview. */ -export function getWebviewHtml( +/** Asset URIs for a webview's bundle. Use directly for custom HTML/CSP; otherwise call `getWebviewHtml`. */ +export function getWebviewAssetUris( webview: vscode.Webview, extensionUri: vscode.Uri, webviewName: string, - title: string, -): string { - const nonce = getNonce(); +): { scriptUri: vscode.Uri; styleUri: vscode.Uri } { const baseUri = vscode.Uri.joinPath( extensionUri, "dist", "webviews", webviewName, ); - const scriptUri = webview.asWebviewUri( - vscode.Uri.joinPath(baseUri, "index.js"), - ); - const styleUri = webview.asWebviewUri( - vscode.Uri.joinPath(baseUri, "index.css"), + return { + scriptUri: webview.asWebviewUri(vscode.Uri.joinPath(baseUri, "index.js")), + styleUri: webview.asWebviewUri(vscode.Uri.joinPath(baseUri, "index.css")), + }; +} + +/** + * Build the webview CSP. Pass `frameSrc` to allow embedding an iframe + * from that origin (used by the chat panel). + */ +export function buildWebviewCsp( + webview: vscode.Webview, + nonce: string, + options?: { frameSrc?: string }, +): string { + const directives = [ + "default-src 'none'", + options?.frameSrc && `frame-src ${options.frameSrc}`, + `script-src 'nonce-${nonce}'`, + `style-src ${webview.cspSource} 'unsafe-inline'`, + `font-src ${webview.cspSource}`, + `img-src ${webview.cspSource} data:`, + ]; + return directives.filter(Boolean).join("; "); +} + +/** Standard webview HTML: mounts the package's bundle into `#root`. */ +export function getWebviewHtml( + webview: vscode.Webview, + extensionUri: vscode.Uri, + webviewName: string, + title: string, +): string { + const nonce = getNonce(); + const { scriptUri, styleUri } = getWebviewAssetUris( + webview, + extensionUri, + webviewName, ); // The vscode-elements library looks for a link element with "vscode-codicon-stylesheet" @@ -28,7 +59,7 @@ export function getWebviewHtml( - + ${escapeHtml(title)} diff --git a/test/unit/webviews/chat/chatPanelProvider.test.ts b/test/unit/webviews/chat/chatPanelProvider.test.ts index e27cd792..586ee24a 100644 --- a/test/unit/webviews/chat/chatPanelProvider.test.ts +++ b/test/unit/webviews/chat/chatPanelProvider.test.ts @@ -17,7 +17,11 @@ interface Harness { } function createHarnessFor(client: MockCoderApi): Harness { - const provider = new ChatPanelProvider(client, createMockLogger()); + const provider = new ChatPanelProvider( + vscode.Uri.file("/ext"), + client, + createMockLogger(), + ); let handler: ((msg: unknown) => void) | null = null; @@ -196,6 +200,7 @@ describe("ChatPanelProvider", () => { expect(html()).toContain( "https://coder.example.com/agents/test-agent-123/embed", ); + expect(html()).toContain("/dist/webviews/chat/index.js"); }); it("focuses the chat panel", () => { diff --git a/test/unit/webviews/html.test.ts b/test/unit/webviews/html.test.ts index 9e0093a4..7eb4dac4 100644 --- a/test/unit/webviews/html.test.ts +++ b/test/unit/webviews/html.test.ts @@ -73,7 +73,7 @@ describe("getWebviewHtml", () => { it("uses the webview's cspSource for style/font/img sources", () => { const html = getWebviewHtml(webview, extensionUri, "speedtest", "ok"); expect(html).toContain( - "style-src mock-csp 'unsafe-inline'; font-src mock-csp; img-src mock-csp data:;", + "style-src mock-csp 'unsafe-inline'; font-src mock-csp; img-src mock-csp data:", ); }); }); diff --git a/test/webview/shared/ipc.test.ts b/test/webview/shared/ipc.test.ts index 7fa9a81c..a50028dc 100644 --- a/test/webview/shared/ipc.test.ts +++ b/test/webview/shared/ipc.test.ts @@ -1,7 +1,7 @@ import { beforeEach, describe, expect, it, vi } from "vitest"; import { defineCommand, defineNotification } from "@repo/shared"; -import { onNotification, sendCommand } from "@repo/webview-shared/ipc"; +import { sendCommand, subscribeNotifications } from "@repo/webview-shared/ipc"; interface Sent { method: string; @@ -27,107 +27,87 @@ describe("sendCommand", () => { it("posts {method, params}", () => { const cmd = defineCommand<{ id: string }>("ns/doThing"); sendCommand(cmd, { id: "42" }); - expect(sent).toEqual([{ method: "ns/doThing", params: { id: "42" } }]); }); it("omits params for void-payload commands", () => { const cmd = defineCommand("ns/noop"); sendCommand(cmd); - expect(sent).toEqual([{ method: "ns/noop" }]); }); }); -describe("onNotification", () => { - it("invokes callback only for matching method", () => { - const def = defineNotification<{ count: number }>("ns/updated"); - const cb = vi.fn(); +describe("subscribeNotifications", () => { + const Api = { + updated: defineNotification<{ count: number }>("ns/updated"), + ping: defineNotification("ns/ping"), + // non-notification entries are ignored. + doThing: defineCommand<{ id: string }>("ns/doThing"), + } as const; - const unsubscribe = onNotification(def, cb); - - window.dispatchEvent( - new MessageEvent("message", { - data: { type: "ns/other", data: { count: 1 } }, - }), - ); - expect(cb).not.toHaveBeenCalled(); + it("invokes the matching handler with typed data", () => { + const updated = vi.fn(); + const ping = vi.fn(); + const unsub = subscribeNotifications(Api, { updated, ping }); window.dispatchEvent( new MessageEvent("message", { data: { type: "ns/updated", data: { count: 7 } }, }), ); - expect(cb).toHaveBeenCalledWith({ count: 7 }); + expect(updated).toHaveBeenCalledWith({ count: 7 }); + expect(ping).not.toHaveBeenCalled(); - unsubscribe(); + unsub(); }); - it("stops receiving events after unsubscribe", () => { - const def = defineNotification("ns/ping"); - const cb = vi.fn(); - - const unsubscribe = onNotification(def, cb); - unsubscribe(); + it("ignores non-matching messages", () => { + const updated = vi.fn(); + const ping = vi.fn(); + const unsub = subscribeNotifications(Api, { updated, ping }); window.dispatchEvent( - new MessageEvent("message", { - data: { type: "ns/ping", data: "hello" }, - }), + new MessageEvent("message", { data: { type: "ns/other" } }), ); - - expect(cb).not.toHaveBeenCalled(); - }); - - it("ignores non-object messages", () => { - const def = defineNotification("ns/evt"); - const cb = vi.fn(); - const unsubscribe = onNotification(def, cb); - window.dispatchEvent(new MessageEvent("message", { data: null })); - window.dispatchEvent(new MessageEvent("message", { data: "string" })); window.dispatchEvent(new MessageEvent("message", { data: 42 })); - expect(cb).not.toHaveBeenCalled(); - unsubscribe(); + expect(updated).not.toHaveBeenCalled(); + expect(ping).not.toHaveBeenCalled(); + + unsub(); }); - it("fires for void notifications (no data field)", () => { - const def = defineNotification("ns/ping"); - const cb = vi.fn(); - const unsubscribe = onNotification(def, cb); + it("fires void notifications with undefined data", () => { + const updated = vi.fn(); + const ping = vi.fn(); + const unsub = subscribeNotifications(Api, { updated, ping }); window.dispatchEvent( new MessageEvent("message", { data: { type: "ns/ping" } }), ); - expect(cb).toHaveBeenCalledWith(undefined); + expect(ping).toHaveBeenCalledWith(undefined); - unsubscribe(); + unsub(); }); - it("supports multiple independent subscribers", () => { - const def = defineNotification("ns/count"); - const a = vi.fn(); - const b = vi.fn(); + it("unsubscribes every handler when the returned function is called", () => { + const updated = vi.fn(); + const ping = vi.fn(); + const unsub = subscribeNotifications(Api, { updated, ping }); - const unsubA = onNotification(def, a); - const unsubB = onNotification(def, b); + unsub(); window.dispatchEvent( - new MessageEvent("message", { data: { type: "ns/count", data: 1 } }), + new MessageEvent("message", { + data: { type: "ns/updated", data: { count: 1 } }, + }), ); - - expect(a).toHaveBeenCalledWith(1); - expect(b).toHaveBeenCalledWith(1); - - unsubA(); window.dispatchEvent( - new MessageEvent("message", { data: { type: "ns/count", data: 2 } }), + new MessageEvent("message", { data: { type: "ns/ping" } }), ); - expect(a).toHaveBeenCalledTimes(1); - expect(b).toHaveBeenCalledTimes(2); - - unsubB(); + expect(updated).not.toHaveBeenCalled(); + expect(ping).not.toHaveBeenCalled(); }); }); From 699e733c8bbe2b327ff7ed6c0f0f3520291ef8bd Mon Sep 17 00:00:00 2001 From: Ehab Younes Date: Thu, 30 Apr 2026 17:39:35 +0300 Subject: [PATCH 5/6] refactor: address remaining PR review on webview IPC helpers DEREM-11: useIpc.command no longer hand-rolls postMessage. Delegates to sendCommand, which also brings in the void-command params-omission behavior so React webviews are consistent with vanilla. DEREM-16: speedtest panel test "ignores unknown message methods" was asserting against a sync throw on an async dispatch, which always passed regardless of behavior. Renamed to reflect the user-facing contract (no error dialog) and asserts on showErrorMessage after the dispatch settles. --- packages/webview-shared/src/react/useIpc.ts | 7 ++----- .../webviews/speedtest/speedtestPanelFactory.test.ts | 9 +++++---- 2 files changed, 7 insertions(+), 9 deletions(-) diff --git a/packages/webview-shared/src/react/useIpc.ts b/packages/webview-shared/src/react/useIpc.ts index 90952359..4b6e4fcd 100644 --- a/packages/webview-shared/src/react/useIpc.ts +++ b/packages/webview-shared/src/react/useIpc.ts @@ -6,7 +6,7 @@ import { useEffect, useRef } from "react"; import { postMessage } from "../api"; -import { subscribeOne } from "../ipc"; +import { sendCommand, subscribeOne } from "../ipc"; import type { CommandDef, @@ -120,10 +120,7 @@ export function useIpc(options: UseIpcOptions = {}) { definition: CommandDef

, ...args: P extends void ? [] : [params: P] ): void { - postMessage({ - method: definition.method, - params: args[0], - }); + sendCommand(definition, ...args); } /** diff --git a/test/unit/webviews/speedtest/speedtestPanelFactory.test.ts b/test/unit/webviews/speedtest/speedtestPanelFactory.test.ts index a5f48672..2f8e5151 100644 --- a/test/unit/webviews/speedtest/speedtestPanelFactory.test.ts +++ b/test/unit/webviews/speedtest/speedtestPanelFactory.test.ts @@ -125,11 +125,12 @@ describe("SpeedtestPanelFactory", () => { }); }); - it("ignores unknown message methods", () => { + it("does not surface an error dialog for unknown command methods", async () => { const { hooks } = openChart(); - expect(() => - hooks.sendFromWebview({ method: "speedtest/bogus" }), - ).not.toThrow(); + hooks.sendFromWebview({ method: "speedtest/bogus" }); + // Dispatch is async; let the rejection settle before asserting. + await Promise.resolve(); + expect(vscode.window.showErrorMessage).not.toHaveBeenCalled(); }); it("stops responding to visibility and theme events after disposal", () => { From fa1e95de8d96b06494c20b6d7135df8589b15084 Mon Sep 17 00:00:00 2001 From: Ehab Younes Date: Thu, 30 Apr 2026 18:37:37 +0300 Subject: [PATCH 6/6] refactor: address remaining PR review on webview IPC helpers DEREM-23: Chat shim now uses a single source-gated message listener. Iframe messages route to the foreign-protocol switch; extension messages dispatch through `buildNotificationRouter` (extracted from `subscribeNotifications`) so the exhaustiveness check is preserved and the dispatch loop is shared. An iframe message whose `type` matches a notification can no longer reach the typed handler. DEREM-24: Direct tests for `isIpcRequest` and `isIpcCommand` covering numeric `requestId`/`method`, missing fields, command vs request disambiguation, and null/non-object inputs. DEREM-25: Tests for `buildWebviewCsp`. Base CSP omits `frame-src`; the option injects it. DEREM-26: `embedUrl` now flows through `escapeHtml` before the iframe `src` interpolation, so a `chatId` containing `"` can't break out of the attribute. DEREM-21: New `test/webview/chat/index.test.ts` covers iframe forwarding (vscodeReady, chatReady, navigate with and without url), extension push (setTheme), source isolation against notification-typed iframe messages, and the auth-error retry flow. DEREM-22: Reworded the `useIpc` comment to describe the role without naming the helper. --- packages/chat/src/index.ts | 144 +++++++++++++------- packages/webview-shared/src/index.ts | 6 +- packages/webview-shared/src/ipc.ts | 39 ++++-- packages/webview-shared/src/react/useIpc.ts | 4 +- src/webviews/chat/chatPanelProvider.ts | 9 +- src/webviews/html.ts | 5 +- test/css.d.ts | 1 + test/unit/webviews/dispatch.test.ts | 46 +++++++ test/unit/webviews/html.test.ts | 24 +++- test/webview/chat/index.test.ts | 117 ++++++++++++++++ 10 files changed, 319 insertions(+), 76 deletions(-) create mode 100644 test/css.d.ts create mode 100644 test/webview/chat/index.test.ts diff --git a/packages/chat/src/index.ts b/packages/chat/src/index.ts index 4062734b..701b7505 100644 --- a/packages/chat/src/index.ts +++ b/packages/chat/src/index.ts @@ -1,65 +1,65 @@ -import { ChatApi } from "@repo/shared"; -import { sendCommand, subscribeNotifications } from "@repo/webview-shared"; +import { ChatApi, type NotificationHandlerMap } from "@repo/shared"; +import { buildNotificationRouter, sendCommand } from "@repo/webview-shared"; import "./index.css"; -/** - * Chat shim. Bridges the iframe's foreign `{ type, payload }` protocol - * and the extension's `ChatApi`. The iframe and status div are - * pre-rendered in the panel's HTML; the bundle just attaches listeners. - */ +/** Chat shim: source-gated bridge between the iframe `{ type, payload }` protocol and `ChatApi`. */ +export function main(): void { + const shim = findShim(); + if (!shim) { + return; + } + revealIframeOnLoad(shim); + listenForMessages(shim); +} -const iframe = document.getElementById("chat-frame") as HTMLIFrameElement; -const status = document.getElementById("status") as HTMLDivElement; -const allowedOrigin = new URL(iframe.src).origin; +interface Shim { + iframe: HTMLIFrameElement; + status: HTMLDivElement; + allowedOrigin: string; +} -iframe.addEventListener("load", () => { - iframe.style.display = "block"; - status.style.display = "none"; -}); +interface IframeMessage { + type?: string; + payload?: { url?: string }; +} -function toIframe(type: string, payload: unknown): void { - iframe.contentWindow?.postMessage({ type, payload }, allowedOrigin); +function findShim(): Shim | null { + const iframe = document.getElementById("chat-frame"); + const status = document.getElementById("status"); + if ( + !(iframe instanceof HTMLIFrameElement) || + !(status instanceof HTMLDivElement) + ) { + return null; + } + return { iframe, status, allowedOrigin: new URL(iframe.src).origin }; } -function showRetry(error: string): void { - status.textContent = ""; - status.appendChild( - document.createTextNode(error || "Authentication failed."), - ); - const btn = document.createElement("button"); - btn.id = "retry-btn"; - btn.textContent = "Retry"; - btn.addEventListener("click", () => { - status.textContent = "Authenticating…"; - sendCommand(ChatApi.vscodeReady); +function revealIframeOnLoad({ iframe, status }: Shim): void { + iframe.addEventListener("load", () => { + iframe.style.display = "block"; + status.style.display = "none"; }); - status.appendChild(document.createElement("br")); - status.appendChild(btn); - status.style.display = "block"; - iframe.style.display = "none"; } -// Compile-checked: a new ChatApi notification without a handler fails the build. -subscribeNotifications(ChatApi, { - setTheme: ({ theme }) => toIframe("coder:set-theme", { theme }), - authBootstrapToken: ({ token }) => { - status.textContent = "Signing in…"; - toIframe("coder:vscode-auth-bootstrap", { token }); - }, - authError: ({ error }) => showRetry(error), -}); +function listenForMessages(shim: Shim): void { + const route = buildNotificationRouter( + ChatApi, + buildNotificationHandlers(shim), + ); + window.addEventListener("message", (event) => { + if (event.source === shim.iframe.contentWindow) { + if (typeof event.data === "object" && event.data !== null) { + handleFromIframe(shim, event.data as IframeMessage); + } + return; + } + route(event.data); + }); +} -// Iframe -> extension. `msg.type` strings are the foreign Coder protocol; -// every `sendCommand(ChatApi.X)` below is still type-checked. -window.addEventListener("message", (event) => { - if (event.source !== iframe.contentWindow) { - return; - } - if (typeof event.data !== "object" || event.data === null) { - return; - } - const msg = event.data as { type?: string; payload?: { url?: string } }; +function handleFromIframe({ status }: Shim, msg: IframeMessage): void { switch (msg.type) { case "coder:vscode-ready": status.textContent = "Authenticating…"; @@ -76,4 +76,46 @@ window.addEventListener("message", (event) => { default: return; } -}); +} + +// Compile-checked: a new ChatApi notification without a handler fails the build. +function buildNotificationHandlers( + shim: Shim, +): NotificationHandlerMap { + return { + setTheme: ({ theme }) => postToIframe(shim, "coder:set-theme", { theme }), + authBootstrapToken: ({ token }) => { + shim.status.textContent = "Signing in…"; + postToIframe(shim, "coder:vscode-auth-bootstrap", { token }); + }, + authError: ({ error }) => showRetry(shim, error), + }; +} + +function postToIframe( + { iframe, allowedOrigin }: Shim, + type: string, + payload: unknown, +): void { + iframe.contentWindow?.postMessage({ type, payload }, allowedOrigin); +} + +function showRetry({ iframe, status }: Shim, error: string): void { + status.textContent = ""; + status.appendChild( + document.createTextNode(error || "Authentication failed."), + ); + const btn = document.createElement("button"); + btn.id = "retry-btn"; + btn.textContent = "Retry"; + btn.addEventListener("click", () => { + status.textContent = "Authenticating…"; + sendCommand(ChatApi.vscodeReady); + }); + status.appendChild(document.createElement("br")); + status.appendChild(btn); + status.style.display = "block"; + iframe.style.display = "none"; +} + +main(); diff --git a/packages/webview-shared/src/index.ts b/packages/webview-shared/src/index.ts index a78b101d..d46be34b 100644 --- a/packages/webview-shared/src/index.ts +++ b/packages/webview-shared/src/index.ts @@ -9,4 +9,8 @@ export interface WebviewMessage { export { getState, setState, postMessage } from "./api"; // IPC helpers for vanilla webviews. React webviews use `useIpc` instead. -export { sendCommand, subscribeNotifications } from "./ipc"; +export { + buildNotificationRouter, + sendCommand, + subscribeNotifications, +} from "./ipc"; diff --git a/packages/webview-shared/src/ipc.ts b/packages/webview-shared/src/ipc.ts index 4c748265..1f2f1445 100644 --- a/packages/webview-shared/src/ipc.ts +++ b/packages/webview-shared/src/ipc.ts @@ -20,38 +20,47 @@ export function sendCommand

( } /** - * Exhaustively subscribe to every notification on `api`. Compile error - * if any notification lacks a handler. Returns a single unsubscribe. + * Build a dispatcher that routes a message to the matching notification + * handler. Compile error if a handler is missing. Use this when composing + * dispatch with other listener logic; otherwise use `subscribeNotifications`. */ -export function subscribeNotifications< +export function buildNotificationRouter< Api extends Record, ->(api: Api, handlers: NotificationHandlerMap): () => void; -export function subscribeNotifications( +>(api: Api, handlers: NotificationHandlerMap): (data: unknown) => void; +export function buildNotificationRouter( api: Record, handlers: Record void>, -): () => void { +): (data: unknown) => void { const byMethod = new Map void>(); for (const [key, def] of Object.entries(api)) { if (def.kind === "notification") { byMethod.set(def.method, handlers[key]); } } - const handler = (event: MessageEvent) => { - const msg = event.data; - if (typeof msg !== "object" || msg === null) { + return (data: unknown) => { + if (typeof data !== "object" || data === null) { return; } - const cb = byMethod.get((msg as { type?: string }).type ?? ""); - cb?.((msg as { data: unknown }).data); + const msg = data as { type?: string; data?: unknown }; + byMethod.get(msg.type ?? "")?.(msg.data); }; +} + +/** Subscribe to every notification on `api`. Compile error if a handler is missing. */ +export function subscribeNotifications< + Api extends Record, +>(api: Api, handlers: NotificationHandlerMap): () => void; +export function subscribeNotifications( + api: Record, + handlers: Record void>, +): () => void { + const route = buildNotificationRouter(api, handlers); + const handler = (event: MessageEvent) => route(event.data); window.addEventListener("message", handler); return () => window.removeEventListener("message", handler); } -/** - * Single-notification subscribe. React's `useIpc` uses this for - * `apiHook.on`. Vanilla webviews should use `subscribeNotifications`. - */ +/** Single-notification subscribe; React's `useIpc` uses this. Vanilla webviews use `subscribeNotifications`. */ export function subscribeOne( def: NotificationDef, callback: (data: D) => void, diff --git a/packages/webview-shared/src/react/useIpc.ts b/packages/webview-shared/src/react/useIpc.ts index 4b6e4fcd..0185fb2b 100644 --- a/packages/webview-shared/src/react/useIpc.ts +++ b/packages/webview-shared/src/react/useIpc.ts @@ -60,8 +60,8 @@ export function useIpc(options: UseIpcOptions = {}) { }; }, []); - // Request/response correlation lives here. Notifications are routed via - // the shared onNotification helper (see the method below). + // Request/response correlation lives here; notifications are handled + // per-subscription below. useEffect(() => { const handler = (event: MessageEvent) => { const msg = event.data as IpcResponse | undefined; diff --git a/src/webviews/chat/chatPanelProvider.ts b/src/webviews/chat/chatPanelProvider.ts index b655e9f8..00bdb81d 100644 --- a/src/webviews/chat/chatPanelProvider.ts +++ b/src/webviews/chat/chatPanelProvider.ts @@ -15,7 +15,12 @@ import { isIpcRequest, notifyWebview, } from "../dispatch"; -import { buildWebviewCsp, getNonce, getWebviewAssetUris } from "../html"; +import { + buildWebviewCsp, + escapeHtml, + getNonce, + getWebviewAssetUris, +} from "../html"; /** * Webview that embeds the Coder agent chat UI inside an iframe. The @@ -217,7 +222,7 @@ export class ChatPanelProvider

Loading chat…
- + `; diff --git a/src/webviews/html.ts b/src/webviews/html.ts index 5c47ceb5..4cb5cab3 100644 --- a/src/webviews/html.ts +++ b/src/webviews/html.ts @@ -19,10 +19,7 @@ export function getWebviewAssetUris( }; } -/** - * Build the webview CSP. Pass `frameSrc` to allow embedding an iframe - * from that origin (used by the chat panel). - */ +/** Build the webview CSP. Pass `frameSrc` to allow embedding an iframe. */ export function buildWebviewCsp( webview: vscode.Webview, nonce: string, diff --git a/test/css.d.ts b/test/css.d.ts new file mode 100644 index 00000000..cbe652db --- /dev/null +++ b/test/css.d.ts @@ -0,0 +1 @@ +declare module "*.css"; diff --git a/test/unit/webviews/dispatch.test.ts b/test/unit/webviews/dispatch.test.ts index 33338196..33eeed92 100644 --- a/test/unit/webviews/dispatch.test.ts +++ b/test/unit/webviews/dispatch.test.ts @@ -4,6 +4,8 @@ import * as vscode from "vscode"; import { dispatchCommand, dispatchRequest, + isIpcCommand, + isIpcRequest, notifyWebview, onWhileVisible, } from "@/webviews/dispatch"; @@ -162,6 +164,50 @@ describe("dispatchRequest", () => { }); }); +describe("isIpcRequest", () => { + it("matches messages with both string requestId and string method", () => { + expect(isIpcRequest({ requestId: "r1", method: "get" })).toBe(true); + }); + + it("rejects non-string requestId", () => { + expect(isIpcRequest({ requestId: 1, method: "get" })).toBe(false); + }); + + it("rejects non-string method", () => { + expect(isIpcRequest({ requestId: "r1", method: 7 })).toBe(false); + }); + + it("rejects messages missing requestId", () => { + expect(isIpcRequest({ method: "get" })).toBe(false); + }); + + it("rejects null and non-objects", () => { + expect(isIpcRequest(null)).toBe(false); + expect(isIpcRequest("string")).toBe(false); + expect(isIpcRequest(undefined)).toBe(false); + }); +}); + +describe("isIpcCommand", () => { + it("matches messages with method but no requestId", () => { + expect(isIpcCommand({ method: "do" })).toBe(true); + expect(isIpcCommand({ method: "do", params: { x: 1 } })).toBe(true); + }); + + it("rejects messages with requestId (those are requests)", () => { + expect(isIpcCommand({ requestId: "r1", method: "do" })).toBe(false); + }); + + it("rejects non-string method", () => { + expect(isIpcCommand({ method: 7 })).toBe(false); + }); + + it("rejects null and non-objects", () => { + expect(isIpcCommand(null)).toBe(false); + expect(isIpcCommand(42)).toBe(false); + }); +}); + describe("onWhileVisible", () => { function makePanel(visible: boolean) { const listeners = new Set<() => void>(); diff --git a/test/unit/webviews/html.test.ts b/test/unit/webviews/html.test.ts index 7eb4dac4..0148ecea 100644 --- a/test/unit/webviews/html.test.ts +++ b/test/unit/webviews/html.test.ts @@ -1,7 +1,12 @@ import { describe, expect, it } from "vitest"; import * as vscode from "vscode"; -import { escapeHtml, getNonce, getWebviewHtml } from "@/webviews/html"; +import { + buildWebviewCsp, + escapeHtml, + getNonce, + getWebviewHtml, +} from "@/webviews/html"; const webview: vscode.Webview = { options: { enableScripts: true, localResourceRoots: [] }, @@ -41,6 +46,23 @@ describe("getNonce", () => { }); }); +describe("buildWebviewCsp", () => { + it("emits a base CSP without frame-src by default", () => { + const csp = buildWebviewCsp(webview, "abc"); + expect(csp).toContain("default-src 'none'"); + expect(csp).toContain("script-src 'nonce-abc'"); + expect(csp).toContain("style-src mock-csp 'unsafe-inline'"); + expect(csp).not.toContain("frame-src"); + }); + + it("inserts frame-src when the option is set", () => { + const csp = buildWebviewCsp(webview, "abc", { + frameSrc: "https://coder.example.com", + }); + expect(csp).toContain("frame-src https://coder.example.com"); + }); +}); + describe("getWebviewHtml", () => { it("escapes the title to prevent HTML injection", () => { const html = getWebviewHtml( diff --git a/test/webview/chat/index.test.ts b/test/webview/chat/index.test.ts new file mode 100644 index 00000000..935be7c1 --- /dev/null +++ b/test/webview/chat/index.test.ts @@ -0,0 +1,117 @@ +import { + afterAll, + beforeAll, + beforeEach, + describe, + expect, + it, + vi, +} from "vitest"; + +import { main } from "../../../packages/chat/src/index"; +import { qs } from "../helpers"; + +const postToExtension = vi.fn(); +const postToIframe = vi.fn(); + +const EMBED_URL = "https://coder.example.com/agents/abc/embed?theme=dark"; +const ALLOWED_ORIGIN = "https://coder.example.com"; + +let iframe: HTMLIFrameElement; + +beforeAll(() => { + vi.stubGlobal( + "acquireVsCodeApi", + vi.fn(() => ({ + postMessage: postToExtension, + getState: vi.fn(), + setState: vi.fn(), + })), + ); + document.body.innerHTML = ` +
Loading chat…
+ + `; + iframe = qs(document, "#chat-frame"); + // Spy on jsdom's real contentWindow.postMessage to avoid fabricating a Window. + vi.spyOn(iframe.contentWindow!, "postMessage").mockImplementation( + postToIframe, + ); + main(); +}); + +afterAll(() => { + vi.unstubAllGlobals(); +}); + +beforeEach(() => { + postToExtension.mockClear(); + postToIframe.mockClear(); +}); + +function fireMessage(data: unknown, fromIframe = false): void { + window.dispatchEvent( + new MessageEvent("message", { + data, + source: fromIframe ? iframe.contentWindow : null, + }), + ); +} + +describe("chat shim", () => { + it("forwards iframe coder:vscode-ready as ChatApi.vscodeReady", () => { + fireMessage({ type: "coder:vscode-ready" }, true); + expect(postToExtension).toHaveBeenCalledWith({ + method: "coder:vscode-ready", + }); + }); + + it("forwards iframe coder:chat-ready as ChatApi.chatReady", () => { + fireMessage({ type: "coder:chat-ready" }, true); + expect(postToExtension).toHaveBeenCalledWith({ + method: "coder:chat-ready", + }); + }); + + it("ignores iframe coder:navigate without a url payload", () => { + fireMessage({ type: "coder:navigate", payload: {} }, true); + expect(postToExtension).not.toHaveBeenCalled(); + }); + + it("forwards iframe coder:navigate with a url payload", () => { + fireMessage( + { type: "coder:navigate", payload: { url: "/templates" } }, + true, + ); + expect(postToExtension).toHaveBeenCalledWith({ + method: "coder:navigate", + params: { url: "/templates" }, + }); + }); + + it("forwards extension setTheme into the iframe", () => { + fireMessage({ type: "coder:set-theme", data: { theme: "light" } }); + expect(postToIframe).toHaveBeenCalledWith( + { type: "coder:set-theme", payload: { theme: "light" } }, + ALLOWED_ORIGIN, + ); + }); + + it("does not dispatch notification handlers for messages from the iframe", () => { + // Source-isolation: a notification-typed iframe message must not reach + // the typed handler (would destructure undefined and throw). + expect(() => + fireMessage({ type: "coder:set-theme", payload: {} }, true), + ).not.toThrow(); + expect(postToIframe).not.toHaveBeenCalled(); + }); + + it("renders a Retry button on auth-error and re-sends vscodeReady", () => { + fireMessage({ type: "coder:auth-error", data: { error: "no token" } }); + const btn = qs(document, "#retry-btn"); + btn.click(); + expect(postToExtension).toHaveBeenCalledWith({ + method: "coder:vscode-ready", + }); + }); +});