Diffs: skip unused render targets (#57909)

Merged via squash. Prepared head SHA: 9972f3029f Co-authored-by: gumadeiras <5599352+gumadeiras@users.noreply.github.com> Co-authored-by: gumadeiras <5599352+gumadeiras@users.noreply.github.com> Reviewed-by: @gumadeiras
2026-03-30 16:21:08 -04:00 · 2026-03-30 16:21:08 -04:00 · 07900facf6
parent 30a1690323
commit 07900facf6
7 changed files with 371 additions and 100 deletions
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@ -25,6 +25,7 @@ Docs: https://docs.openclaw.ai
 - Flows/tasks: add a minimal SQLite-backed flow registry plus task-to-flow linkage scaffolding, so orchestrated work can start gaining a first-class parent record without changing current task delivery behavior.
 - Flows/tasks: route one-task ACP and subagent updates through a parent flow owner context, so detached work can emerge back through the intended parent thread/session instead of speaking only as a raw child task.
 - Matrix/history: add optional room history context for Matrix group triggers via `channels.matrix.historyLimit`, with per-agent watermarks and retry-safe snapshots so failed trigger retries do not drift into newer room messages. (#57022) thanks @chain710.
+- Diffs: skip unused viewer-versus-file SSR preload work so `diffs` view-only and file-only runs do less render work while keeping mode outputs aligned. (#57909) thanks @gumadeiras.

 ### Fixes

--- a/extensions/diffs/src/config.test.ts
+++ b/extensions/diffs/src/config.test.ts
@ -287,7 +287,8 @@ describe("renderDiffDocument", () => {
      },
    );

-    const loaderSrc = rendered.html.match(/<script type="module" src="([^"]+)"><\/script>/)?.[1];
+    const html = rendered.html ?? "";
+    const loaderSrc = html.match(/<script type="module" src="([^"]+)"><\/script>/)?.[1];
    expect(loaderSrc).toBe("../../assets/viewer.js");
    expect(
      new URL(loaderSrc ?? "", "https://example.com/openclaw/plugins/diffs/view/id/token").pathname,
--- a/extensions/diffs/src/render-target.test.ts
+++ b/extensions/diffs/src/render-target.test.ts
@ -0,0 +1,92 @@
+import { beforeEach, describe, expect, it, vi } from "vitest";
+
+const { preloadFileDiffMock, preloadMultiFileDiffMock } = vi.hoisted(() => ({
+  preloadFileDiffMock: vi.fn(async ({ fileDiff }: { fileDiff: unknown }) => ({
+    prerenderedHTML: "<div>mock diff</div>",
+    fileDiff,
+  })),
+  preloadMultiFileDiffMock: vi.fn(
+    async ({ oldFile, newFile }: { oldFile: unknown; newFile: unknown }) => ({
+      prerenderedHTML: "<div>mock diff</div>",
+      oldFile,
+      newFile,
+    }),
+  ),
+}));
+
+vi.mock("@pierre/diffs/ssr", () => ({
+  preloadFileDiff: preloadFileDiffMock,
+  preloadMultiFileDiff: preloadMultiFileDiffMock,
+}));
+
+import { DEFAULT_DIFFS_TOOL_DEFAULTS, resolveDiffImageRenderOptions } from "./config.js";
+import { renderDiffDocument } from "./render.js";
+
+function createRenderOptions() {
+  return {
+    presentation: DEFAULT_DIFFS_TOOL_DEFAULTS,
+    image: resolveDiffImageRenderOptions({ defaults: DEFAULT_DIFFS_TOOL_DEFAULTS }),
+    expandUnchanged: false,
+  };
+}
+
+describe("renderDiffDocument render targets", () => {
+  beforeEach(() => {
+    preloadFileDiffMock.mockClear();
+    preloadMultiFileDiffMock.mockClear();
+  });
+
+  it("renders only the viewer variant for before/after viewer mode", async () => {
+    const rendered = await renderDiffDocument(
+      {
+        kind: "before_after",
+        before: "one\n",
+        after: "two\n",
+      },
+      createRenderOptions(),
+      "viewer",
+    );
+
+    expect(rendered.html).toContain("mock diff");
+    expect(rendered.imageHtml).toBeUndefined();
+    expect(preloadMultiFileDiffMock).toHaveBeenCalledTimes(1);
+  });
+
+  it("renders both variants for before/after both mode", async () => {
+    const rendered = await renderDiffDocument(
+      {
+        kind: "before_after",
+        before: "one\n",
+        after: "two\n",
+      },
+      createRenderOptions(),
+      "both",
+    );
+
+    expect(rendered.html).toContain("mock diff");
+    expect(rendered.imageHtml).toContain("mock diff");
+    expect(preloadMultiFileDiffMock).toHaveBeenCalledTimes(2);
+  });
+
+  it("renders only the image variant for patch image mode", async () => {
+    const rendered = await renderDiffDocument(
+      {
+        kind: "patch",
+        patch: [
+          "diff --git a/a.ts b/a.ts",
+          "--- a/a.ts",
+          "+++ b/a.ts",
+          "@@ -1 +1 @@",
+          "-a",
+          "+b",
+        ].join("\n"),
+      },
+      createRenderOptions(),
+      "image",
+    );
+
+    expect(rendered.html).toBeUndefined();
+    expect(rendered.imageHtml).toContain("mock diff");
+    expect(preloadFileDiffMock).toHaveBeenCalledTimes(1);
+  });
+});
--- a/extensions/diffs/src/render.ts
+++ b/extensions/diffs/src/render.ts
@ -5,6 +5,7 @@ import { ensurePierreThemesRegistered } from "./pierre-themes.js";
 import type {
  DiffInput,
  DiffRenderOptions,
+  DiffRenderTarget,
  DiffViewerOptions,
  DiffViewerPayload,
  RenderedDiffDocument,
@ -151,13 +152,25 @@ function buildImageRenderOptions(options: DiffRenderOptions): DiffRenderOptions
  };
 }

-function buildRenderVariants(options: DiffRenderOptions): {
-  viewerOptions: DiffViewerOptions;
-  imageOptions: DiffViewerOptions;
+function shouldRenderViewer(target: DiffRenderTarget): boolean {
+  return target === "viewer" || target === "both";
+}
+
+function shouldRenderImage(target: DiffRenderTarget): boolean {
+  return target === "image" || target === "both";
+}
+
+function buildRenderVariants(params: { options: DiffRenderOptions; target: DiffRenderTarget }): {
+  viewerOptions?: DiffViewerOptions;
+  imageOptions?: DiffViewerOptions;
 } {
  return {
-    viewerOptions: buildDiffOptions(options),
-    imageOptions: buildDiffOptions(buildImageRenderOptions(options)),
+    ...(shouldRenderViewer(params.target)
+      ? { viewerOptions: buildDiffOptions(params.options) }
+      : {}),
+    ...(shouldRenderImage(params.target)
+      ? { imageOptions: buildDiffOptions(buildImageRenderOptions(params.options)) }
+      : {}),
  };
 }

@ -302,34 +315,37 @@ function buildHtmlDocument(params: {
 }

 type RenderedSection = {
-  viewer: string;
-  image: string;
+  viewer?: string;
+  image?: string;
 };

 function buildRenderedSection(params: {
-  viewerPayload: DiffViewerPayload;
-  imagePayload: DiffViewerPayload;
+  viewerPayload?: DiffViewerPayload;
+  imagePayload?: DiffViewerPayload;
 }): RenderedSection {
  return {
-    viewer: renderDiffCard(params.viewerPayload),
-    image: renderDiffCard(params.imagePayload),
+    ...(params.viewerPayload ? { viewer: renderDiffCard(params.viewerPayload) } : {}),
+    ...(params.imagePayload ? { image: renderDiffCard(params.imagePayload) } : {}),
  };
 }

 function buildRenderedBodies(sections: ReadonlyArray<RenderedSection>): {
-  viewerBodyHtml: string;
-  imageBodyHtml: string;
+  viewerBodyHtml?: string;
+  imageBodyHtml?: string;
 } {
+  const viewerSections = sections.flatMap((section) => (section.viewer ? [section.viewer] : []));
+  const imageSections = sections.flatMap((section) => (section.image ? [section.image] : []));
  return {
-    viewerBodyHtml: sections.map((section) => section.viewer).join("\n"),
-    imageBodyHtml: sections.map((section) => section.image).join("\n"),
+    ...(viewerSections.length > 0 ? { viewerBodyHtml: viewerSections.join("\n") } : {}),
+    ...(imageSections.length > 0 ? { imageBodyHtml: imageSections.join("\n") } : {}),
  };
 }

 async function renderBeforeAfterDiff(
  input: Extract<DiffInput, { kind: "before_after" }>,
  options: DiffRenderOptions,
-): Promise<{ viewerBodyHtml: string; imageBodyHtml: string; fileCount: number }> {
+  target: DiffRenderTarget,
+): Promise<{ viewerBodyHtml?: string; imageBodyHtml?: string; fileCount: number }> {
  ensurePierreThemesRegistered();

  const fileName = resolveBeforeAfterFileName(input);
@ -344,40 +360,52 @@ async function renderBeforeAfterDiff(
    contents: input.after,
    ...(lang ? { lang } : {}),
  };
-  const { viewerOptions, imageOptions } = buildRenderVariants(options);
+  const { viewerOptions, imageOptions } = buildRenderVariants({ options, target });
  const [viewerResult, imageResult] = await Promise.all([
-    preloadMultiFileDiffWithFallback({
-      oldFile,
-      newFile,
-      options: viewerOptions,
-    }),
-    preloadMultiFileDiffWithFallback({
-      oldFile,
-      newFile,
-      options: imageOptions,
-    }),
+    viewerOptions
+      ? preloadMultiFileDiffWithFallback({
+          oldFile,
+          newFile,
+          options: viewerOptions,
+        })
+      : Promise.resolve(undefined),
+    imageOptions
+      ? preloadMultiFileDiffWithFallback({
+          oldFile,
+          newFile,
+          options: imageOptions,
+        })
+      : Promise.resolve(undefined),
  ]);
  const section = buildRenderedSection({
-    viewerPayload: {
-      prerenderedHTML: viewerResult.prerenderedHTML,
-      oldFile: viewerResult.oldFile,
-      newFile: viewerResult.newFile,
-      options: viewerOptions,
-      langs: buildPayloadLanguages({
-        oldFile: viewerResult.oldFile,
-        newFile: viewerResult.newFile,
-      }),
-    },
-    imagePayload: {
-      prerenderedHTML: imageResult.prerenderedHTML,
-      oldFile: imageResult.oldFile,
-      newFile: imageResult.newFile,
-      options: imageOptions,
-      langs: buildPayloadLanguages({
-        oldFile: imageResult.oldFile,
-        newFile: imageResult.newFile,
-      }),
-    },
+    ...(viewerResult && viewerOptions
+      ? {
+          viewerPayload: {
+            prerenderedHTML: viewerResult.prerenderedHTML,
+            oldFile: viewerResult.oldFile,
+            newFile: viewerResult.newFile,
+            options: viewerOptions,
+            langs: buildPayloadLanguages({
+              oldFile: viewerResult.oldFile,
+              newFile: viewerResult.newFile,
+            }),
+          },
+        }
+      : {}),
+    ...(imageResult && imageOptions
+      ? {
+          imagePayload: {
+            prerenderedHTML: imageResult.prerenderedHTML,
+            oldFile: imageResult.oldFile,
+            newFile: imageResult.newFile,
+            options: imageOptions,
+            langs: buildPayloadLanguages({
+              oldFile: imageResult.oldFile,
+              newFile: imageResult.newFile,
+            }),
+          },
+        }
+      : {}),
  });

  return {
@ -389,7 +417,8 @@ async function renderBeforeAfterDiff(
 async function renderPatchDiff(
  input: Extract<DiffInput, { kind: "patch" }>,
  options: DiffRenderOptions,
-): Promise<{ viewerBodyHtml: string; imageBodyHtml: string; fileCount: number }> {
+  target: DiffRenderTarget,
+): Promise<{ viewerBodyHtml?: string; imageBodyHtml?: string; fileCount: number }> {
  ensurePierreThemesRegistered();

  const files = parsePatchFiles(input.patch).flatMap((entry) => entry.files ?? []);
@ -408,33 +437,45 @@ async function renderPatchDiff(
    throw new Error(`Patch input is too large to render (max ${MAX_PATCH_TOTAL_LINES} lines).`);
  }

-  const { viewerOptions, imageOptions } = buildRenderVariants(options);
+  const { viewerOptions, imageOptions } = buildRenderVariants({ options, target });
  const sections = await Promise.all(
    files.map(async (fileDiff) => {
      const [viewerResult, imageResult] = await Promise.all([
-        preloadFileDiffWithFallback({
-          fileDiff,
-          options: viewerOptions,
-        }),
-        preloadFileDiffWithFallback({
-          fileDiff,
-          options: imageOptions,
-        }),
+        viewerOptions
+          ? preloadFileDiffWithFallback({
+              fileDiff,
+              options: viewerOptions,
+            })
+          : Promise.resolve(undefined),
+        imageOptions
+          ? preloadFileDiffWithFallback({
+              fileDiff,
+              options: imageOptions,
+            })
+          : Promise.resolve(undefined),
      ]);

      return buildRenderedSection({
-        viewerPayload: {
-          prerenderedHTML: viewerResult.prerenderedHTML,
-          fileDiff: viewerResult.fileDiff,
-          options: viewerOptions,
-          langs: buildPayloadLanguages({ fileDiff: viewerResult.fileDiff }),
-        },
-        imagePayload: {
-          prerenderedHTML: imageResult.prerenderedHTML,
-          fileDiff: imageResult.fileDiff,
-          options: imageOptions,
-          langs: buildPayloadLanguages({ fileDiff: imageResult.fileDiff }),
-        },
+        ...(viewerResult && viewerOptions
+          ? {
+              viewerPayload: {
+                prerenderedHTML: viewerResult.prerenderedHTML,
+                fileDiff: viewerResult.fileDiff,
+                options: viewerOptions,
+                langs: buildPayloadLanguages({ fileDiff: viewerResult.fileDiff }),
+              },
+            }
+          : {}),
+        ...(imageResult && imageOptions
+          ? {
+              imagePayload: {
+                prerenderedHTML: imageResult.prerenderedHTML,
+                fileDiff: imageResult.fileDiff,
+                options: imageOptions,
+                langs: buildPayloadLanguages({ fileDiff: imageResult.fileDiff }),
+              },
+            }
+          : {}),
      });
    }),
  );
@ -448,28 +489,37 @@ async function renderPatchDiff(
 export async function renderDiffDocument(
  input: DiffInput,
  options: DiffRenderOptions,
+  target: DiffRenderTarget = "both",
 ): Promise<RenderedDiffDocument> {
  const title = buildDiffTitle(input);
  const rendered =
    input.kind === "before_after"
-      ? await renderBeforeAfterDiff(input, options)
-      : await renderPatchDiff(input, options);
+      ? await renderBeforeAfterDiff(input, options, target)
+      : await renderPatchDiff(input, options, target);

  return {
-    html: buildHtmlDocument({
-      title,
-      bodyHtml: rendered.viewerBodyHtml,
-      theme: options.presentation.theme,
-      imageMaxWidth: options.image.maxWidth,
-      runtimeMode: "viewer",
-    }),
-    imageHtml: buildHtmlDocument({
-      title,
-      bodyHtml: rendered.imageBodyHtml,
-      theme: options.presentation.theme,
-      imageMaxWidth: options.image.maxWidth,
-      runtimeMode: "image",
-    }),
+    ...(rendered.viewerBodyHtml
+      ? {
+          html: buildHtmlDocument({
+            title,
+            bodyHtml: rendered.viewerBodyHtml,
+            theme: options.presentation.theme,
+            imageMaxWidth: options.image.maxWidth,
+            runtimeMode: "viewer",
+          }),
+        }
+      : {}),
+    ...(rendered.imageBodyHtml
+      ? {
+          imageHtml: buildHtmlDocument({
+            title,
+            bodyHtml: rendered.imageBodyHtml,
+            theme: options.presentation.theme,
+            imageMaxWidth: options.image.maxWidth,
+            runtimeMode: "image",
+          }),
+        }
+      : {}),
    title,
    fileCount: rendered.fileCount,
    inputKind: input.kind,
--- a/extensions/diffs/src/tool-render-output.test.ts
+++ b/extensions/diffs/src/tool-render-output.test.ts
@ -0,0 +1,99 @@
+import fs from "node:fs/promises";
+import path from "node:path";
+import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
+import { createTestPluginApi } from "../../../test/helpers/plugins/plugin-api.js";
+import type { OpenClawPluginApi } from "../api.js";
+import type { DiffScreenshotter } from "./browser.js";
+import { DEFAULT_DIFFS_TOOL_DEFAULTS } from "./config.js";
+import { createDiffStoreHarness } from "./test-helpers.js";
+
+const { renderDiffDocumentMock } = vi.hoisted(() => ({
+  renderDiffDocumentMock: vi.fn(),
+}));
+
+vi.mock("./render.js", () => ({
+  renderDiffDocument: renderDiffDocumentMock,
+}));
+
+describe("diffs tool rendered output guards", () => {
+  let cleanupRootDir: () => Promise<void>;
+  let store: Awaited<ReturnType<typeof createDiffStoreHarness>>["store"];
+
+  beforeEach(async () => {
+    vi.resetModules();
+    renderDiffDocumentMock.mockReset();
+    ({ store, cleanup: cleanupRootDir } = await createDiffStoreHarness(
+      "openclaw-diffs-tool-render-output-",
+    ));
+  });
+
+  afterEach(async () => {
+    await cleanupRootDir();
+  });
+
+  it("accepts empty string image html for file output", async () => {
+    renderDiffDocumentMock.mockResolvedValue({
+      title: "Text diff",
+      fileCount: 1,
+      inputKind: "before_after",
+      imageHtml: "",
+    });
+
+    const { createDiffsTool } = await import("./tool.js");
+    const screenshotter = createPngScreenshotter({
+      assertHtml: (html) => {
+        expect(html).toBe("");
+      },
+    });
+
+    const tool = createDiffsTool({
+      api: createApi(),
+      store,
+      defaults: DEFAULT_DIFFS_TOOL_DEFAULTS,
+      screenshotter,
+    });
+
+    const result = await tool.execute?.("tool-empty-image-html", {
+      before: "one\n",
+      after: "two\n",
+      mode: "file",
+    });
+
+    expect(screenshotter.screenshotHtml).toHaveBeenCalledTimes(1);
+    expect((result?.details as Record<string, unknown>).filePath).toEqual(expect.any(String));
+  });
+});
+
+function createApi(): OpenClawPluginApi {
+  return createTestPluginApi({
+    id: "diffs",
+    name: "Diffs",
+    description: "Diffs",
+    source: "test",
+    config: {
+      gateway: {
+        port: 18789,
+        bind: "loopback",
+      },
+    },
+    runtime: {} as OpenClawPluginApi["runtime"],
+  }) as OpenClawPluginApi;
+}
+
+function createPngScreenshotter(
+  params: {
+    assertHtml?: (html: string) => void;
+  } = {},
+): DiffScreenshotter {
+  const screenshotHtml: DiffScreenshotter["screenshotHtml"] = vi.fn(
+    async ({ html, outputPath }: { html: string; outputPath: string }) => {
+      params.assertHtml?.(html);
+      await fs.mkdir(path.dirname(outputPath), { recursive: true });
+      await fs.writeFile(outputPath, Buffer.from("png"));
+      return outputPath;
+    },
+  );
+  return {
+    screenshotHtml,
+  };
+}
--- a/extensions/diffs/src/tool.ts
+++ b/extensions/diffs/src/tool.ts
@ -5,7 +5,12 @@ import { PlaywrightDiffScreenshotter, type DiffScreenshotter } from "./browser.j
 import { resolveDiffImageRenderOptions } from "./config.js";
 import { renderDiffDocument } from "./render.js";
 import type { DiffArtifactStore } from "./store.js";
-import type { DiffArtifactContext, DiffRenderOptions, DiffToolDefaults } from "./types.js";
+import type {
+  DiffArtifactContext,
+  DiffRenderOptions,
+  DiffRenderTarget,
+  DiffToolDefaults,
+} from "./types.js";
 import {
  DIFF_IMAGE_QUALITY_PRESETS,
  DIFF_LAYOUTS,
@ -164,16 +169,21 @@ export function createDiffsTool(params: {
        fileScale: toolParams.fileScale ?? toolParams.imageScale,
        fileMaxWidth: toolParams.fileMaxWidth ?? toolParams.imageMaxWidth,
      });
+      const renderTarget = resolveRenderTarget(mode);

-      const rendered = await renderDiffDocument(input, {
-        presentation: {
-          ...params.defaults,
-          layout,
-          theme,
+      const rendered = await renderDiffDocument(
+        input,
+        {
+          presentation: {
+            ...params.defaults,
+            layout,
+            theme,
+          },
+          image,
+          expandUnchanged,
        },
-        image,
-        expandUnchanged,
-      });
+        renderTarget,
+      );

      const screenshotter =
        params.screenshotter ?? new PlaywrightDiffScreenshotter({ config: params.api.config });
@ -182,7 +192,7 @@ export function createDiffsTool(params: {
        const artifactFile = await renderDiffArtifactFile({
          screenshotter,
          store: params.store,
-          html: rendered.imageHtml,
+          html: requireRenderedHtml(rendered.imageHtml, "image"),
          theme,
          image,
          ttlMs,
@ -216,7 +226,7 @@ export function createDiffsTool(params: {
      }

      const artifact = await params.store.createArtifact({
-        html: rendered.html,
+        html: requireRenderedHtml(rendered.html, "viewer"),
        title: rendered.title,
        inputKind: rendered.inputKind,
        fileCount: rendered.fileCount,
@ -259,7 +269,7 @@ export function createDiffsTool(params: {
          screenshotter,
          store: params.store,
          artifactId: artifact.id,
-          html: rendered.imageHtml,
+          html: requireRenderedHtml(rendered.imageHtml, "image"),
          theme,
          image,
        });
@ -320,6 +330,23 @@ function isArtifactOnlyMode(mode: DiffMode): mode is "image" | "file" {
  return mode === "image" || mode === "file";
 }

+function resolveRenderTarget(mode: DiffMode): DiffRenderTarget {
+  if (mode === "view") {
+    return "viewer";
+  }
+  if (isArtifactOnlyMode(mode)) {
+    return "image";
+  }
+  return "both";
+}
+
+function requireRenderedHtml(html: string | undefined, target: DiffRenderTarget): string {
+  if (html !== undefined) {
+    return html;
+  }
+  throw new Error(`Missing ${target} render output.`);
+}
+
 function buildArtifactDetails(params: {
  baseDetails: Record<string, unknown>;
  artifactFile: { path: string; bytes: number };
--- a/extensions/diffs/src/types.ts
+++ b/extensions/diffs/src/types.ts
@ -13,6 +13,7 @@ export type DiffTheme = (typeof DIFF_THEMES)[number];
 export type DiffIndicators = (typeof DIFF_INDICATORS)[number];
 export type DiffImageQualityPreset = (typeof DIFF_IMAGE_QUALITY_PRESETS)[number];
 export type DiffOutputFormat = (typeof DIFF_OUTPUT_FORMATS)[number];
+export type DiffRenderTarget = "viewer" | "image" | "both";

 export type DiffPresentationDefaults = {
  fontFamily: string;
@ -92,8 +93,8 @@ export type DiffViewerPayload = {
 };

 export type RenderedDiffDocument = {
-  html: string;
-  imageHtml: string;
+  html?: string;
+  imageHtml?: string;
  title: string;
  fileCount: number;
  inputKind: DiffInput["kind"];