Skip to content

Commit 2ded26a

Browse files
steipetesteipete
authored
fix(diffs): share SSR preloads and repair language-pack hydration (#100487)
* fix(diffs): share SSR preloads and repair language-pack hydration Render viewer and file documents from a single @pierre/diffs SSR preload per file (mode=both previously ran the full diff+highlight pipeline twice; 651ms -> 303ms on an 8-file patch), apply the file-mode font bump as a document-level override, and keep hydration payloads variant-faithful. Fix the language-pack runtime downgrading pack-only languages to plain text at hydration by defining a per-target build flag and forwarding it to payload normalization. Also: case-insensitive language hints, identical before/after short-circuit with details.changed, patch input failures classified as tool input errors, canonical config values now win over deprecated aliases, hash-pinned viewer runtime served immutable, truthful browser-vs-render errors, timing-safe artifact token compare, unref idle browser timer. * docs(changelog): link diffs rendering entry to PR * test(diffs): narrow manifest validation results before value access * test(tooling): allowlist diffs viewer-client define suppression
1 parent f20159e commit 2ded26a

25 files changed

Lines changed: 470 additions & 171 deletions

‎CHANGELOG.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ Docs: https://docs.openclaw.ai
1717

1818
### Fixes
1919

20+
- **Diffs rendering:** render viewer and image output from one SSR preload, preserve language-pack highlighting through hydration, normalize language hints case-insensitively, skip identical before/after inputs with an explicit `changed` result, report truthful file-render and input errors, cache hash-pinned viewer runtimes, and prefer canonical file settings over stale aliases. (#100487)
2021
- **Remote browser reliability:** bound persistent Playwright tab enumeration by the existing remote CDP timeout budget and retire timed-out connection attempts so late completions cannot restore a stuck connection. (#80147, #58968) Thanks @HemantSudarshan and @KeaneYan.
2122
- **Control UI approval prompts:** keep stale resolve failures and busy-state cleanup from leaking across newer approvals or Gateway reconnects. (#98394) Thanks @haruaiclone-droid.
2223
- **Agent empty replies:** surface a visible failure when a completed interactive turn has no deliverable reply, including queued follow-ups, while preserving explicit silence, pending continuations, and committed side effects. (#100456) Thanks @mushuiyu886.

‎docs/tools/diffs.md‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -171,8 +171,11 @@ Without the pack, unsupported languages still render as readable plain text. See
171171

172172
## Output details contract
173173

174+
All successful results include `changed`: identical before/after input returns `false` without creating an artifact; rendered results return `true`.
175+
174176
<AccordionGroup>
175177
<Accordion title="Viewer fields (view and both modes)">
178+
- `changed`
176179
- `artifactId`
177180
- `viewerUrl`
178181
- `viewerPath`
@@ -185,6 +188,7 @@ Without the pack, unsupported languages still render as readable plain text. See
185188

186189
</Accordion>
187190
<Accordion title="File fields (file and both modes)">
191+
- `changed`
188192
- `artifactId`
189193
- `expiresAt`
190194
- `filePath`

‎extensions/diffs-language-pack/src/plugin.ts‎

Lines changed: 14 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,9 @@
11
// Diffs Language Pack plugin module implements plugin behavior.
22
import type { IncomingMessage, ServerResponse } from "node:http";
33
import type { OpenClawPluginApi } from "../api.js";
4-
import { VIEWER_ASSET_PREFIX, getServedViewerAsset } from "./viewer-assets.js";
4+
import { VIEWER_ASSET_PREFIX, VIEWER_RUNTIME_PATH, getServedViewerAsset } from "./viewer-assets.js";
5+
6+
const IMMUTABLE_ASSET_CACHE_CONTROL = "public, max-age=31536000, immutable";
57

68
export function registerDiffsLanguagePackPlugin(api: OpenClawPluginApi): void {
79
api.registerHttpRoute({
@@ -30,7 +32,11 @@ function createDiffsLanguagePackHttpHandler() {
3032
}
3133

3234
res.statusCode = 200;
33-
setSharedHeaders(res, asset.contentType);
35+
setSharedHeaders(
36+
res,
37+
asset.contentType,
38+
parsed.pathname === VIEWER_RUNTIME_PATH ? IMMUTABLE_ASSET_CACHE_CONTROL : undefined,
39+
);
3440
if (req.method === "HEAD") {
3541
res.end();
3642
} else {
@@ -57,8 +63,12 @@ function respondText(res: ServerResponse, statusCode: number, body: string): voi
5763
res.end(body);
5864
}
5965

60-
function setSharedHeaders(res: ServerResponse, contentType: string): void {
61-
res.setHeader("cache-control", "no-store, max-age=0");
66+
function setSharedHeaders(
67+
res: ServerResponse,
68+
contentType: string,
69+
cacheControl = "no-store, max-age=0",
70+
): void {
71+
res.setHeader("cache-control", cacheControl);
6272
res.setHeader("content-type", contentType);
6373
res.setHeader("x-content-type-options", "nosniff");
6474
res.setHeader("referrer-policy", "no-referrer");

‎extensions/diffs/README.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ It gives agents one tool, `diffs`, that can:
2020

2121
The tool can return:
2222

23+
- `details.changed`: `false` when before/after inputs are identical and no artifact was rendered; `true` for rendered results
2324
- `details.viewerUrl`: a gateway URL that can be opened in the canvas
2425
- `details.filePath`: a local rendered artifact path when file rendering is requested
2526
- `details.fileFormat`: the rendered file format (`png` or `pdf`)

‎extensions/diffs/openclaw.plugin.json‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -145,8 +145,7 @@
145145
},
146146
"fileFormat": {
147147
"type": "string",
148-
"enum": ["png", "pdf"],
149-
"default": "png"
148+
"enum": ["png", "pdf"]
150149
},
151150
"format": {
152151
"type": "string",
@@ -155,8 +154,7 @@
155154
},
156155
"fileQuality": {
157156
"type": "string",
158-
"enum": ["standard", "hq", "print"],
159-
"default": "standard"
157+
"enum": ["standard", "hq", "print"]
160158
},
161159
"fileScale": {
162160
"type": "number",

‎extensions/diffs/skills/diffs/SKILL.md‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,8 @@ When you need to show edits as a real diff, prefer the `diffs` tool instead of w
77

88
The `diffs` tool accepts either `before` + `after` text, or a unified `patch` string.
99

10+
Check `details.changed`: identical before/after input returns `false` without creating an artifact; rendered results return `true`.
11+
1012
Use `mode=view` when you want an interactive gateway-hosted viewer. After the tool returns, use `details.viewerUrl` with the canvas tool via `canvas present` or `canvas navigate`.
1113
If the deployment uses a loopback trusted proxy (for example Tailscale Serve with `gateway.trustedProxies` including `127.0.0.1`), raw loopback viewer requests can fail closed without forwarded client-IP headers. In that topology, prefer `mode=file` / `mode=both`, or use a configured `viewerBaseUrl` / explicit proxy/public `baseUrl` when you need a shareable viewer URL.
1214

‎extensions/diffs/src/browser.test.ts‎

Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -208,6 +208,81 @@ describe("PlaywrightDiffScreenshotter", () => {
208208
expect(pages).toHaveLength(1);
209209
expect(pages[0]?.screenshot).toHaveBeenCalledTimes(0);
210210
});
211+
212+
it("wraps browser launch failures with Chromium installation guidance", async () => {
213+
launchMock.mockRejectedValue(new Error("launch failed"));
214+
const screenshotter = new PlaywrightDiffScreenshotter({
215+
config: createConfig(),
216+
browserIdleMs: 1_000,
217+
});
218+
219+
await expect(
220+
screenshotter.screenshotHtml({
221+
html: '<html><head></head><body><main class="oc-frame"></main></body></html>',
222+
outputPath,
223+
theme: "dark",
224+
image: {
225+
format: "png",
226+
qualityPreset: "standard",
227+
scale: 2,
228+
maxWidth: 960,
229+
maxPixels: 8_000_000,
230+
},
231+
}),
232+
).rejects.toThrow("requires a Chromium-compatible browser");
233+
});
234+
235+
it("wraps new-page failures with Chromium installation guidance", async () => {
236+
const browser = createMockBrowser([]);
237+
browser.newPage.mockRejectedValue(new Error("page creation failed"));
238+
launchMock.mockResolvedValue(browser);
239+
const screenshotter = new PlaywrightDiffScreenshotter({
240+
config: createConfig(),
241+
browserIdleMs: 1_000,
242+
});
243+
244+
await expect(
245+
screenshotter.screenshotHtml({
246+
html: '<html><head></head><body><main class="oc-frame"></main></body></html>',
247+
outputPath,
248+
theme: "dark",
249+
image: {
250+
format: "png",
251+
qualityPreset: "standard",
252+
scale: 2,
253+
maxWidth: 960,
254+
maxPixels: 8_000_000,
255+
},
256+
}),
257+
).rejects.toThrow("requires a Chromium-compatible browser");
258+
});
259+
260+
it("preserves render errors after a browser page has opened", async () => {
261+
const browser = createMockBrowser([]);
262+
const page = createMockPage();
263+
page.waitForFunction.mockRejectedValue(new Error("hydration timeout"));
264+
browser.newPage.mockResolvedValue(page);
265+
launchMock.mockResolvedValue(browser);
266+
const screenshotter = new PlaywrightDiffScreenshotter({
267+
config: createConfig(),
268+
browserIdleMs: 1_000,
269+
});
270+
271+
await expect(
272+
screenshotter.screenshotHtml({
273+
html: '<html><head></head><body><main class="oc-frame"></main></body></html>',
274+
outputPath,
275+
theme: "dark",
276+
image: {
277+
format: "png",
278+
qualityPreset: "standard",
279+
scale: 2,
280+
maxWidth: 960,
281+
maxPixels: 8_000_000,
282+
},
283+
}),
284+
).rejects.toThrow("hydration timeout");
285+
});
211286
});
212287

213288
describe("diffs plugin registration", () => {
@@ -428,6 +503,7 @@ describe("diffs plugin registration", () => {
428503
[
429504
"When you need to show edits as a real diff, prefer the `diffs` tool instead of writing a manual summary.",
430505
"It accepts either `before` + `after` text or a unified `patch`.",
506+
"Check `details.changed`: identical before/after input returns `false` without creating an artifact; rendered results return `true`.",
431507
"`mode=view` returns `details.viewerUrl` for canvas use; `mode=file` returns `details.filePath`; `mode=both` returns both.",
432508
"If you need to send the rendered file, use the `message` tool with `path` or `filePath`.",
433509
"Include `path` when you know the filename, and omit presentation overrides unless needed.",

‎extensions/diffs/src/browser.ts‎

Lines changed: 30 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -68,24 +68,33 @@ export class PlaywrightDiffScreenshotter implements DiffScreenshotter {
6868
theme: DiffTheme;
6969
image: DiffRenderOptions["image"];
7070
}): Promise<string> {
71-
const lease = await acquireSharedBrowser({
72-
config: this.config,
73-
idleMs: this.browserIdleMs,
74-
});
71+
let lease: BrowserLease;
72+
try {
73+
lease = await acquireSharedBrowser({
74+
config: this.config,
75+
idleMs: this.browserIdleMs,
76+
});
77+
} catch (error) {
78+
throw buildBrowserUnavailableError(error);
79+
}
7580
let page: Awaited<ReturnType<BrowserInstance["newPage"]>> | undefined;
7681
let currentScale = params.image.scale;
7782
const maxRetries = 2;
7883

7984
try {
8085
for (let attempt = 0; attempt <= maxRetries; attempt += 1) {
81-
page = await lease.browser.newPage({
82-
viewport: {
83-
width: Math.max(Math.ceil(params.image.maxWidth + 240), 1200),
84-
height: 900,
85-
},
86-
deviceScaleFactor: currentScale,
87-
colorScheme: params.theme,
88-
});
86+
try {
87+
page = await lease.browser.newPage({
88+
viewport: {
89+
width: Math.max(Math.ceil(params.image.maxWidth + 240), 1200),
90+
height: 900,
91+
},
92+
deviceScaleFactor: currentScale,
93+
colorScheme: params.theme,
94+
});
95+
} catch (error) {
96+
throw buildBrowserUnavailableError(error);
97+
}
8998
await page.route("**/*", async (route) => {
9099
const requestUrl = route.request().url();
91100
if (requestUrl === "about:blank" || requestUrl.startsWith("data:")) {
@@ -276,22 +285,21 @@ export class PlaywrightDiffScreenshotter implements DiffScreenshotter {
276285
return params.outputPath;
277286
}
278287
throw new Error(IMAGE_SIZE_LIMIT_ERROR);
279-
} catch (error) {
280-
if (error instanceof Error && error.message === IMAGE_SIZE_LIMIT_ERROR) {
281-
throw error;
282-
}
283-
const reason = formatErrorMessage(error);
284-
throw new Error(
285-
`Diff PNG/PDF rendering requires a Chromium-compatible browser. Set browser.executablePath or install Chrome/Chromium. ${reason}`,
286-
{ cause: error },
287-
);
288288
} finally {
289289
await page?.close().catch(() => {});
290290
await lease.release();
291291
}
292292
}
293293
}
294294

295+
function buildBrowserUnavailableError(error: unknown): Error {
296+
const reason = formatErrorMessage(error);
297+
return new Error(
298+
`Diff PNG/PDF rendering requires a Chromium-compatible browser. Set browser.executablePath or install Chrome/Chromium. ${reason}`,
299+
{ cause: error },
300+
);
301+
}
302+
295303
async function writeExternalArtifactFile(params: {
296304
outputPath: string;
297305
write: (tempPath: string) => Promise<void>;
@@ -449,6 +457,7 @@ function scheduleIdleBrowserClose(state: SharedBrowserState, idleMs: number): vo
449457
void closeSharedBrowser();
450458
}
451459
}, idleMs);
460+
state.idleTimer.unref();
452461
}
453462

454463
function clearIdleTimer(state: SharedBrowserState): void {

‎extensions/diffs/src/config.test.ts‎

Lines changed: 31 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -62,7 +62,7 @@ function compileManifestConfigSchema() {
6262
schema: manifest.configSchema,
6363
value,
6464
applyDefaults: true,
65-
}).ok;
65+
});
6666
}
6767

6868
function requireRecord(value: unknown, label: string): Record<string, unknown> {
@@ -216,6 +216,20 @@ describe("resolveDiffsPluginDefaults", () => {
216216
);
217217
});
218218

219+
it("prefers an explicit canonical default value over a deprecated alias", () => {
220+
expectFields(
221+
resolveDiffsPluginDefaults({
222+
defaults: {
223+
fileFormat: "png",
224+
imageFormat: "pdf",
225+
},
226+
}),
227+
{
228+
fileFormat: "png",
229+
},
230+
);
231+
});
232+
219233
it("accepts plugin-wide artifact TTL defaults", () => {
220234
expectFields(
221235
resolveDiffsPluginDefaults({
@@ -240,7 +254,7 @@ describe("resolveDiffsPluginDefaults", () => {
240254
);
241255
});
242256

243-
it("keeps loader-applied schema defaults from shadowing aliases and quality-derived defaults", () => {
257+
it("keeps alias-only config values after manifest validation", () => {
244258
const validate = compileManifestConfigSchema();
245259

246260
const aliasOnly = {
@@ -249,8 +263,11 @@ describe("resolveDiffsPluginDefaults", () => {
249263
imageQuality: "hq",
250264
},
251265
};
252-
expect(validate(aliasOnly)).toBe(true);
253-
expectFields(resolveDiffsPluginDefaults(aliasOnly), {
266+
const validatedAliasOnly = validate(aliasOnly);
267+
if (!validatedAliasOnly.ok) {
268+
throw new Error("Expected alias-only config to pass manifest validation.");
269+
}
270+
expectFields(resolveDiffsPluginDefaults(validatedAliasOnly.value), {
254271
fileFormat: "pdf",
255272
fileQuality: "hq",
256273
fileScale: 2.5,
@@ -262,8 +279,11 @@ describe("resolveDiffsPluginDefaults", () => {
262279
fileQuality: "hq",
263280
},
264281
};
265-
expect(validate(qualityOnly)).toBe(true);
266-
expectFields(resolveDiffsPluginDefaults(qualityOnly), {
282+
const validatedQualityOnly = validate(qualityOnly);
283+
if (!validatedQualityOnly.ok) {
284+
throw new Error("Expected quality-only config to pass manifest validation.");
285+
}
286+
expectFields(resolveDiffsPluginDefaults(validatedQualityOnly.value), {
267287
fileQuality: "hq",
268288
fileScale: 2.5,
269289
fileMaxWidth: 1200,
@@ -301,10 +321,10 @@ describe("diffs plugin schema surfaces", () => {
301321
it("rejects invalid viewerBaseUrl values at manifest-validation time too", () => {
302322
const validate = compileManifestConfigSchema();
303323

304-
expect(validate({ viewerBaseUrl: "javascript:alert(1)" })).toBe(false);
305-
expect(validate({ viewerBaseUrl: "https://example.com/openclaw?x=1" })).toBe(false);
306-
expect(validate({ viewerBaseUrl: "https://example.com/openclaw#frag" })).toBe(false);
307-
expect(validate({ viewerBaseUrl: "https://example.com/openclaw/" })).toBe(true);
324+
expect(validate({ viewerBaseUrl: "javascript:alert(1)" }).ok).toBe(false);
325+
expect(validate({ viewerBaseUrl: "https://example.com/openclaw?x=1" }).ok).toBe(false);
326+
expect(validate({ viewerBaseUrl: "https://example.com/openclaw#frag" }).ok).toBe(false);
327+
expect(validate({ viewerBaseUrl: "https://example.com/openclaw/" }).ok).toBe(true);
308328
});
309329

310330
it("preserves defaults and security for direct safeParse callers", () => {
@@ -344,7 +364,7 @@ describe("diffs plugin schema surfaces", () => {
344364
expectFields(data.security, { allowRemoteViewer: true });
345365
});
346366

347-
it("canonicalizes alias-driven defaults for direct safeParse callers", () => {
367+
it("resolves deprecated aliases before safeParse applies runtime defaults", () => {
348368
const parsed = requireRecord(
349369
diffsPluginConfigSchema.safeParse?.({
350370
defaults: {

0 commit comments

Comments
 (0)