diff --git a/extensions/diffs/src/browser.test.ts b/extensions/diffs/src/browser.test.ts new file mode 100644 index 00000000000..48ff91b4075 --- /dev/null +++ b/extensions/diffs/src/browser.test.ts @@ -0,0 +1,115 @@ +import fs from "node:fs/promises"; +import os from "node:os"; +import path from "node:path"; +import type { OpenClawConfig } from "openclaw/plugin-sdk"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; + +const { launchMock } = vi.hoisted(() => ({ + launchMock: vi.fn(), +})); + +vi.mock("playwright-core", () => ({ + chromium: { + launch: launchMock, + }, +})); + +describe("PlaywrightDiffScreenshotter", () => { + let rootDir: string; + let outputPath: string; + + beforeEach(async () => { + vi.useFakeTimers(); + rootDir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-diffs-browser-")); + outputPath = path.join(rootDir, "preview.png"); + launchMock.mockReset(); + const browserModule = await import("./browser.js"); + await browserModule.resetSharedBrowserStateForTests(); + }); + + afterEach(async () => { + const browserModule = await import("./browser.js"); + await browserModule.resetSharedBrowserStateForTests(); + vi.useRealTimers(); + await fs.rm(rootDir, { recursive: true, force: true }); + }); + + it("reuses the same browser across renders and closes it after the idle window", async () => { + const pages: Array<{ close: ReturnType }> = []; + const browser = createMockBrowser(pages); + launchMock.mockResolvedValue(browser); + const { PlaywrightDiffScreenshotter } = await import("./browser.js"); + + const screenshotter = new PlaywrightDiffScreenshotter({ + config: createConfig(), + browserIdleMs: 1_000, + }); + + await screenshotter.screenshotHtml({ + html: '
', + outputPath, + theme: "dark", + }); + await screenshotter.screenshotHtml({ + html: '
', + outputPath, + theme: "dark", + }); + + expect(launchMock).toHaveBeenCalledTimes(1); + expect(browser.newPage).toHaveBeenCalledTimes(2); + expect(pages).toHaveLength(2); + expect(pages[0]?.close).toHaveBeenCalledTimes(1); + expect(pages[1]?.close).toHaveBeenCalledTimes(1); + + await vi.advanceTimersByTimeAsync(1_000); + expect(browser.close).toHaveBeenCalledTimes(1); + + await screenshotter.screenshotHtml({ + html: '
', + outputPath, + theme: "light", + }); + + expect(launchMock).toHaveBeenCalledTimes(2); + }); +}); + +function createConfig(): OpenClawConfig { + return { + browser: { + executablePath: process.execPath, + }, + } as OpenClawConfig; +} + +function createMockBrowser(pages: Array<{ close: ReturnType }>) { + const browser = { + newPage: vi.fn(async () => { + const page = createMockPage(); + pages.push(page); + return page; + }), + close: vi.fn(async () => {}), + on: vi.fn(), + }; + return browser; +} + +function createMockPage() { + return { + route: vi.fn(async () => {}), + setContent: vi.fn(async () => {}), + waitForFunction: vi.fn(async () => {}), + evaluate: vi.fn(async () => {}), + locator: vi.fn(() => ({ + waitFor: vi.fn(async () => {}), + boundingBox: vi.fn(async () => ({ x: 40, y: 40, width: 640, height: 240 })), + })), + setViewportSize: vi.fn(async () => {}), + screenshot: vi.fn(async ({ path: screenshotPath }: { path: string }) => { + await fs.writeFile(screenshotPath, Buffer.from("png")); + }), + close: vi.fn(async () => {}), + }; +} diff --git a/extensions/diffs/src/browser.ts b/extensions/diffs/src/browser.ts index 0cec2650fa5..e9654dc043f 100644 --- a/extensions/diffs/src/browser.ts +++ b/extensions/diffs/src/browser.ts @@ -6,15 +6,43 @@ import { chromium } from "playwright-core"; import type { DiffTheme } from "./types.js"; import { VIEWER_ASSET_PREFIX, getServedViewerAsset } from "./viewer-assets.js"; +const DEFAULT_BROWSER_IDLE_MS = 30_000; +const SHARED_BROWSER_KEY = "__default__"; + export type DiffScreenshotter = { screenshotHtml(params: { html: string; outputPath: string; theme: DiffTheme }): Promise; }; +type BrowserInstance = Awaited>; + +type BrowserLease = { + browser: BrowserInstance; + release(): Promise; +}; + +type SharedBrowserState = { + browser?: BrowserInstance; + browserPromise: Promise; + idleTimer: ReturnType | null; + key: string; + users: number; +}; + +type ExecutablePathCache = { + key: string; + valuePromise: Promise; +}; + +let sharedBrowserState: SharedBrowserState | null = null; +let executablePathCache: ExecutablePathCache | null = null; + export class PlaywrightDiffScreenshotter implements DiffScreenshotter { private readonly config: OpenClawConfig; + private readonly browserIdleMs: number; - constructor(params: { config: OpenClawConfig }) { + constructor(params: { config: OpenClawConfig; browserIdleMs?: number }) { this.config = params.config; + this.browserIdleMs = params.browserIdleMs ?? DEFAULT_BROWSER_IDLE_MS; } async screenshotHtml(params: { @@ -23,17 +51,14 @@ export class PlaywrightDiffScreenshotter implements DiffScreenshotter { theme: DiffTheme; }): Promise { await fs.mkdir(path.dirname(params.outputPath), { recursive: true }); - const executablePath = await resolveBrowserExecutablePath(this.config); - let browser: Awaited> | undefined; + const lease = await acquireSharedBrowser({ + config: this.config, + idleMs: this.browserIdleMs, + }); + let page: Awaited> | undefined; try { - browser = await chromium.launch({ - headless: true, - ...(executablePath ? { executablePath } : {}), - args: ["--disable-dev-shm-usage", "--disable-gpu"], - }); - - const page = await browser.newPage({ + page = await lease.browser.newPage({ viewport: { width: 1200, height: 900 }, colorScheme: params.theme, }); @@ -113,11 +138,17 @@ export class PlaywrightDiffScreenshotter implements DiffScreenshotter { `Diff image rendering requires a Chromium-compatible browser. Set browser.executablePath or install Chrome/Chromium. ${reason}`, ); } finally { - await browser?.close().catch(() => {}); + await page?.close().catch(() => {}); + await lease.release(); } } } +export async function resetSharedBrowserStateForTests(): Promise { + executablePathCache = null; + await closeSharedBrowser(); +} + function injectBaseHref(html: string): string { if (html.includes(" { + const cacheKey = JSON.stringify({ + configPath: config.browser?.executablePath?.trim() || "", + env: [ + process.env.OPENCLAW_BROWSER_EXECUTABLE_PATH ?? "", + process.env.BROWSER_EXECUTABLE_PATH ?? "", + process.env.PLAYWRIGHT_CHROMIUM_EXECUTABLE_PATH ?? "", + ], + path: process.env.PATH ?? "", + }); + + if (executablePathCache?.key === cacheKey) { + return await executablePathCache.valuePromise; + } + + const valuePromise = resolveBrowserExecutablePathUncached(config).catch((error) => { + if (executablePathCache?.valuePromise === valuePromise) { + executablePathCache = null; + } + throw error; + }); + executablePathCache = { + key: cacheKey, + valuePromise, + }; + return await valuePromise; +} + +async function resolveBrowserExecutablePathUncached( + config: OpenClawConfig, +): Promise { const configPath = config.browser?.executablePath?.trim(); if (configPath) { await assertExecutable(configPath, "browser.executablePath"); @@ -155,6 +216,99 @@ async function resolveBrowserExecutablePath(config: OpenClawConfig): Promise { + const executablePath = await resolveBrowserExecutablePath(params.config); + const desiredKey = executablePath || SHARED_BROWSER_KEY; + if (sharedBrowserState && sharedBrowserState.key !== desiredKey) { + await closeSharedBrowser(); + } + + if (!sharedBrowserState) { + const browserPromise = chromium + .launch({ + headless: true, + ...(executablePath ? { executablePath } : {}), + args: ["--disable-dev-shm-usage", "--disable-gpu"], + }) + .then((browser) => { + if (sharedBrowserState?.browserPromise === browserPromise) { + sharedBrowserState.browser = browser; + browser.on("disconnected", () => { + if (sharedBrowserState?.browser === browser) { + clearIdleTimer(sharedBrowserState); + sharedBrowserState = null; + } + }); + } + return browser; + }) + .catch((error) => { + if (sharedBrowserState?.browserPromise === browserPromise) { + sharedBrowserState = null; + } + throw error; + }); + + sharedBrowserState = { + browserPromise, + idleTimer: null, + key: desiredKey, + users: 0, + }; + } + + clearIdleTimer(sharedBrowserState); + const state = sharedBrowserState; + const browser = await state.browserPromise; + state.users += 1; + + let released = false; + return { + browser, + release: async () => { + if (released) { + return; + } + released = true; + state.users = Math.max(0, state.users - 1); + if (state.users === 0) { + scheduleIdleBrowserClose(state, params.idleMs); + } + }, + }; +} + +function scheduleIdleBrowserClose(state: SharedBrowserState, idleMs: number): void { + clearIdleTimer(state); + state.idleTimer = setTimeout(() => { + if (sharedBrowserState === state && state.users === 0) { + void closeSharedBrowser(); + } + }, idleMs); +} + +function clearIdleTimer(state: SharedBrowserState): void { + if (!state.idleTimer) { + return; + } + clearTimeout(state.idleTimer); + state.idleTimer = null; +} + +async function closeSharedBrowser(): Promise { + const state = sharedBrowserState; + if (!state) { + return; + } + sharedBrowserState = null; + clearIdleTimer(state); + const browser = state.browser ?? (await state.browserPromise.catch(() => null)); + await browser?.close().catch(() => {}); +} + async function collectExecutableCandidates(): Promise { const candidates = new Set(); diff --git a/extensions/diffs/src/render.test.ts b/extensions/diffs/src/render.test.ts index 95dcccd2f56..e9b7f764a9c 100644 --- a/extensions/diffs/src/render.test.ts +++ b/extensions/diffs/src/render.test.ts @@ -22,6 +22,8 @@ describe("renderDiffDocument", () => { expect(rendered.html).toContain("data-openclaw-diff-root"); expect(rendered.html).toContain("src/example.ts"); expect(rendered.html).toContain("/plugins/diffs/assets/viewer.js"); + expect(rendered.imageHtml).not.toContain("/plugins/diffs/assets/viewer.js"); + expect(rendered.imageHtml).toContain('data-openclaw-diffs-ready="true"'); expect(rendered.html).not.toContain("fonts.googleapis.com"); }); diff --git a/extensions/diffs/src/render.ts b/extensions/diffs/src/render.ts index cb682e93feb..a6ebd29d9da 100644 --- a/extensions/diffs/src/render.ts +++ b/extensions/diffs/src/render.ts @@ -171,13 +171,22 @@ function renderDiffCard(payload: DiffViewerPayload): string { `; } +function renderStaticDiffCard(prerenderedHTML: string): string { + return `
+ + + +
`; +} + function buildHtmlDocument(params: { title: string; bodyHtml: string; theme: DiffRenderOptions["presentation"]["theme"]; + runtimeMode: "viewer" | "image"; }): string { return ` - + @@ -258,12 +267,12 @@ function buildHtmlDocument(params: { -
+
${params.bodyHtml}
- + ${params.runtimeMode === "viewer" ? `` : ""} `; } @@ -271,7 +280,7 @@ function buildHtmlDocument(params: { async function renderBeforeAfterDiff( input: Extract, options: DiffRenderOptions, -): Promise<{ bodyHtml: string; fileCount: number }> { +): Promise<{ viewerBodyHtml: string; imageBodyHtml: string; fileCount: number }> { const fileName = resolveBeforeAfterFileName(input); const lang = normalizeSupportedLanguage(input.lang); const oldFile: FileContents = { @@ -292,13 +301,14 @@ async function renderBeforeAfterDiff( }); return { - bodyHtml: renderDiffCard({ + viewerBodyHtml: renderDiffCard({ prerenderedHTML: result.prerenderedHTML, oldFile: result.oldFile, newFile: result.newFile, options: payloadOptions, langs: buildPayloadLanguages({ oldFile: result.oldFile, newFile: result.newFile }), }), + imageBodyHtml: renderStaticDiffCard(result.prerenderedHTML), fileCount: 1, }; } @@ -306,7 +316,7 @@ async function renderBeforeAfterDiff( async function renderPatchDiff( input: Extract, options: DiffRenderOptions, -): Promise<{ bodyHtml: string; fileCount: number }> { +): Promise<{ viewerBodyHtml: string; imageBodyHtml: string; fileCount: number }> { const files = parsePatchFiles(input.patch).flatMap((entry) => entry.files ?? []); if (files.length === 0) { throw new Error("Patch input did not contain any file diffs."); @@ -320,17 +330,21 @@ async function renderPatchDiff( options: payloadOptions, }); - return renderDiffCard({ - prerenderedHTML: result.prerenderedHTML, - fileDiff: result.fileDiff, - options: payloadOptions, - langs: buildPayloadLanguages({ fileDiff: result.fileDiff }), - }); + return { + viewer: renderDiffCard({ + prerenderedHTML: result.prerenderedHTML, + fileDiff: result.fileDiff, + options: payloadOptions, + langs: buildPayloadLanguages({ fileDiff: result.fileDiff }), + }), + image: renderStaticDiffCard(result.prerenderedHTML), + }; }), ); return { - bodyHtml: sections.join("\n"), + viewerBodyHtml: sections.map((section) => section.viewer).join("\n"), + imageBodyHtml: sections.map((section) => section.image).join("\n"), fileCount: files.length, }; } @@ -348,8 +362,15 @@ export async function renderDiffDocument( return { html: buildHtmlDocument({ title, - bodyHtml: rendered.bodyHtml, + bodyHtml: rendered.viewerBodyHtml, theme: options.presentation.theme, + runtimeMode: "viewer", + }), + imageHtml: buildHtmlDocument({ + title, + bodyHtml: rendered.imageBodyHtml, + theme: options.presentation.theme, + runtimeMode: "image", }), title, fileCount: rendered.fileCount, diff --git a/extensions/diffs/src/store.test.ts b/extensions/diffs/src/store.test.ts index 90995bfc02a..d94bc286c7a 100644 --- a/extensions/diffs/src/store.test.ts +++ b/extensions/diffs/src/store.test.ts @@ -61,4 +61,46 @@ describe("DiffArtifactStore", () => { const updated = await store.updateImagePath(artifact.id, imagePath); expect(updated.imagePath).toBe(imagePath); }); + + it("allocates standalone image paths outside artifact metadata", async () => { + const imagePath = store.allocateStandaloneImagePath(); + expect(imagePath).toMatch(/preview\.png$/); + expect(imagePath).toContain(rootDir); + }); + + it("throttles cleanup sweeps across repeated artifact creation", async () => { + vi.useFakeTimers(); + const now = new Date("2026-02-27T16:00:00Z"); + vi.setSystemTime(now); + store = new DiffArtifactStore({ + rootDir, + cleanupIntervalMs: 60_000, + }); + const cleanupSpy = vi.spyOn(store, "cleanupExpired").mockResolvedValue(); + + await store.createArtifact({ + html: "one", + title: "One", + inputKind: "before_after", + fileCount: 1, + }); + await store.createArtifact({ + html: "two", + title: "Two", + inputKind: "before_after", + fileCount: 1, + }); + + expect(cleanupSpy).toHaveBeenCalledTimes(1); + + vi.setSystemTime(new Date(now.getTime() + 61_000)); + await store.createArtifact({ + html: "three", + title: "Three", + inputKind: "before_after", + fileCount: 1, + }); + + expect(cleanupSpy).toHaveBeenCalledTimes(2); + }); }); diff --git a/extensions/diffs/src/store.ts b/extensions/diffs/src/store.ts index 6e55cb2ce0e..b70223c2972 100644 --- a/extensions/diffs/src/store.ts +++ b/extensions/diffs/src/store.ts @@ -7,6 +7,7 @@ import type { DiffArtifactMeta } from "./types.js"; const DEFAULT_TTL_MS = 30 * 60 * 1000; const MAX_TTL_MS = 6 * 60 * 60 * 1000; const SWEEP_FALLBACK_AGE_MS = 24 * 60 * 60 * 1000; +const DEFAULT_CLEANUP_INTERVAL_MS = 5 * 60 * 1000; const VIEWER_PREFIX = "/plugins/diffs/view"; type CreateArtifactParams = { @@ -20,15 +21,21 @@ type CreateArtifactParams = { export class DiffArtifactStore { private readonly rootDir: string; private readonly logger?: PluginLogger; + private readonly cleanupIntervalMs: number; + private cleanupInFlight: Promise | null = null; + private nextCleanupAt = 0; - constructor(params: { rootDir: string; logger?: PluginLogger }) { + constructor(params: { rootDir: string; logger?: PluginLogger; cleanupIntervalMs?: number }) { this.rootDir = params.rootDir; this.logger = params.logger; + this.cleanupIntervalMs = + params.cleanupIntervalMs === undefined + ? DEFAULT_CLEANUP_INTERVAL_MS + : Math.max(0, Math.floor(params.cleanupIntervalMs)); } async createArtifact(params: CreateArtifactParams): Promise { await this.ensureRoot(); - await this.cleanupExpired(); const id = crypto.randomBytes(10).toString("hex"); const token = crypto.randomBytes(24).toString("hex"); @@ -52,6 +59,7 @@ export class DiffArtifactStore { await fs.mkdir(artifactDir, { recursive: true }); await fs.writeFile(htmlPath, params.html, "utf8"); await this.writeMeta(meta); + this.maybeCleanupExpired(); return meta; } @@ -95,6 +103,11 @@ export class DiffArtifactStore { return path.join(this.artifactDir(id), "preview.png"); } + allocateStandaloneImagePath(): string { + const id = crypto.randomBytes(10).toString("hex"); + return path.join(this.artifactDir(id), "preview.png"); + } + async cleanupExpired(): Promise { await this.ensureRoot(); const entries = await fs.readdir(this.rootDir, { withFileTypes: true }).catch(() => []); @@ -129,6 +142,27 @@ export class DiffArtifactStore { await fs.mkdir(this.rootDir, { recursive: true }); } + private maybeCleanupExpired(): void { + const now = Date.now(); + if (this.cleanupInFlight || now < this.nextCleanupAt) { + return; + } + + this.nextCleanupAt = now + this.cleanupIntervalMs; + const cleanupPromise = this.cleanupExpired() + .catch((error) => { + this.nextCleanupAt = 0; + this.logger?.warn(`Failed to clean expired diff artifacts: ${String(error)}`); + }) + .finally(() => { + if (this.cleanupInFlight === cleanupPromise) { + this.cleanupInFlight = null; + } + }); + + this.cleanupInFlight = cleanupPromise; + } + private artifactDir(id: string): string { return path.join(this.rootDir, id); } diff --git a/extensions/diffs/src/tool.test.ts b/extensions/diffs/src/tool.test.ts index b90debd5c53..c8c3751936f 100644 --- a/extensions/diffs/src/tool.test.ts +++ b/extensions/diffs/src/tool.test.ts @@ -41,7 +41,8 @@ describe("diffs tool", () => { it("returns an image artifact in image mode", async () => { const screenshotter = { - screenshotHtml: vi.fn(async ({ outputPath }: { outputPath: string }) => { + screenshotHtml: vi.fn(async ({ html, outputPath }: { html: string; outputPath: string }) => { + expect(html).not.toContain("/plugins/diffs/assets/viewer.js"); await fs.mkdir(path.dirname(outputPath), { recursive: true }); await fs.writeFile(outputPath, Buffer.from("png")); return outputPath; @@ -66,6 +67,7 @@ describe("diffs tool", () => { expect(readTextContent(result, 0)).toContain("Use the `message` tool"); expect(result?.content).toHaveLength(1); expect((result?.details as Record).imagePath).toBeDefined(); + expect((result?.details as Record).viewerUrl).toBeUndefined(); }); it("falls back to view output when both mode cannot render an image", async () => { @@ -142,6 +144,14 @@ describe("diffs tool", () => { }); it("prefers explicit tool params over configured defaults", async () => { + const screenshotter = { + screenshotHtml: vi.fn(async ({ html, outputPath }: { html: string; outputPath: string }) => { + expect(html).not.toContain("/plugins/diffs/assets/viewer.js"); + await fs.mkdir(path.dirname(outputPath), { recursive: true }); + await fs.writeFile(outputPath, Buffer.from("png")); + return outputPath; + }), + }; const tool = createDiffsTool({ api: createApi(), store, @@ -151,6 +161,7 @@ describe("diffs tool", () => { theme: "light", layout: "split", }, + screenshotter, }); const result = await tool.execute?.("tool-6", { @@ -162,6 +173,7 @@ describe("diffs tool", () => { }); expect((result?.details as Record).mode).toBe("both"); + expect(screenshotter.screenshotHtml).toHaveBeenCalledTimes(1); const viewerPath = String((result?.details as Record).viewerPath); const [id] = viewerPath.split("/").filter(Boolean).slice(-2); const html = await store.readHtml(id); diff --git a/extensions/diffs/src/tool.ts b/extensions/diffs/src/tool.ts index 012250beaa0..13779741614 100644 --- a/extensions/diffs/src/tool.ts +++ b/extensions/diffs/src/tool.ts @@ -91,6 +91,39 @@ export function createDiffsTool(params: { expandUnchanged, }); + const screenshotter = + params.screenshotter ?? new PlaywrightDiffScreenshotter({ config: params.api.config }); + + if (mode === "image") { + const imagePath = params.store.allocateStandaloneImagePath(); + await screenshotter.screenshotHtml({ + html: rendered.imageHtml, + outputPath: imagePath, + theme, + }); + const imageStats = await fs.stat(imagePath); + + return { + content: [ + { + type: "text", + text: + `Diff image generated at: ${imagePath}\n` + + "Use the `message` tool with `path` or `filePath` to send the PNG.", + }, + ], + details: { + title: rendered.title, + inputKind: rendered.inputKind, + fileCount: rendered.fileCount, + mode, + imagePath, + path: imagePath, + imageBytes: imageStats.size, + }, + }; + } + const artifact = await params.store.createArtifact({ html: rendered.html, title: rendered.title, @@ -128,13 +161,10 @@ export function createDiffsTool(params: { }; } - const screenshotter = - params.screenshotter ?? new PlaywrightDiffScreenshotter({ config: params.api.config }); - try { const imagePath = params.store.allocateImagePath(artifact.id); await screenshotter.screenshotHtml({ - html: rendered.html, + html: rendered.imageHtml, outputPath: imagePath, theme, }); diff --git a/extensions/diffs/src/types.ts b/extensions/diffs/src/types.ts index cc0183c9335..f627d78679f 100644 --- a/extensions/diffs/src/types.ts +++ b/extensions/diffs/src/types.ts @@ -67,6 +67,7 @@ export type DiffViewerPayload = { export type RenderedDiffDocument = { html: string; + imageHtml: string; title: string; fileCount: number; inputKind: DiffInput["kind"];