fix desktop image export for runtime JSX - #6725
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fa4a09fe4f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Visual regression reviewHead: 3 changed · 43 unchanged · 0 new without baseline · 0 failed Changed cases
Unchanged cases
Visual diff is advisory only and does not block merging. |
lefarcen
added
size/L
lefarcen
added
the
needs-validation
|
Thanks for the focused fix here — I’ve marked this for QA validation because it changes the headless image export path users rely on. Once the review/product gates clear, we’ll queue a manual validation pass before merge. |
mrcfps
left a comment
mrcfps
left a comment
There was a problem hiding this comment.
Thanks @mturac — this is a focused, well-scoped fix for #6658. Waiting for runtime JSX mount (including the repo-owned .ui-kit-loading placeholder) before capture, while keeping static HTML and PDF on the fast path, matches the bug and the design-system contract.
I have one non-blocking lifecycle concern on the new resource-timeout wrapper (inline). The readiness script, host boundary errors, and test matrix otherwise look solid.
🔁 Powered by Looper · runner=reviewer · agent=grok-build · An autonomous AI dev team for your GitHub repos.
| export async function waitForArtifactResources( | ||
| operation: Promise<void>, | ||
| timeoutMs = DEFAULT_IMAGE_RENDER_TIMEOUT_MS, | ||
| ): Promise<void> { | ||
| const timeout = validatedTimeout(timeoutMs); | ||
| let timer: ReturnType<typeof setTimeout> | undefined; | ||
| const timedOut = new Promise<never>((_, reject) => { | ||
| timer = setTimeout( | ||
| () => reject(new Error(`Image export timed out waiting for late resources (${timeout}ms)`)), | ||
| timeout, | ||
| ); | ||
| }); | ||
| try { | ||
| await Promise.race([operation, timedOut]); | ||
| } finally { | ||
| if (timer) clearTimeout(timer); | ||
| } | ||
| } |
There was a problem hiding this comment.
Non-blocking: orphaned operation after Promise.race timeout can crash desktop
Problem: When timedOut wins, operation is left pending with no handler. Callers pass waitForPrintableContent(window) here (and again from renderImage after resize). On timeout, exportArtifact returns the error and then window.destroy()s in finally. Destroying the BrowserWindow while webContents.executeJavaScript is still in flight typically rejects that promise — and nothing is awaiting it anymore.
Why it matters: Desktop installs a fatal unhandledRejection filter (apps/desktop/src/main/uncaught-exception.ts) that logs non-harmless rejections and rethrows via setImmediate, which takes down the main process. So a slow/stuck resource settle that correctly fails the export with a timeout can still crash the whole desktop app a tick later — worse for agent/od export workflows than a clean { ok: false } response.
Evidence:
await Promise.race([operation, timedOut])does not attach a rejection handler to the loser.exportArtifactalways destroys the window after a thrown timeout.- Desktop treats unhandled rejections as fatal (not swallowed).
Suggested change: Attach a sink before racing so a late reject/resolve cannot become unhandled, and keep the race semantics:
export async function waitForArtifactResources(
operation: Promise<void>,
timeoutMs = DEFAULT_IMAGE_RENDER_TIMEOUT_MS,
): Promise<void> {
const timeout = validatedTimeout(timeoutMs);
let timer: ReturnType<typeof setTimeout> | undefined;
const timedOut = new Promise<never>((_, reject) => {
timer = setTimeout(
() => reject(new Error(`Image export timed out waiting for late resources (${timeout}ms)`)),
timeout,
);
});
// Prevent destroy-after-timeout from surfacing as a fatal unhandledRejection.
void operation.catch(() => {});
try {
await Promise.race([operation, timedOut]);
} finally {
if (timer) clearTimeout(timer);
}
}A small unit test that rejects operation after the timeout wins (and asserts no unhandled rejection / that the timeout error is still thrown) would lock this in.
🔁 Powered by Looper · runner=reviewer · agent=grok-build · An autonomous AI dev team for your GitHub repos.
xxiaoxiong
left a comment
xxiaoxiong
left a comment
There was a problem hiding this comment.
LGTM. Solves #6658 cleanly. The blank PNG edge case for Babel-compiled runtime JSX comes from the existing waitForPrintableContent finishing before the JSX app actually mounts. The new artifact-export-readiness.ts adds a runtime-JSX-specific readiness check that only delays the image path (PDF keeps the existing fast path):
-
artifactRenderReadinessScript (injected into isolated world 1001): if no script[type="text/babel"|"text/jsx"], resolve(true) immediately — preserves the static HTML / canvas / CSS fast path. Otherwise probes #root / #app / [data-reactroot] / [data-v-app], records initial .ui-kit-loading placeholder, and waits via MutationObserver + double-rAF until the placeholder is replaced OR a meaningful child mounts. 30s ceiling on the readiness promise.
-
waitForArtifactContent wires PDF to settleResources() only, image to readiness first then resources. Both resource waits now also have an explicit late-resources timeout so a hanging requestAnimationFrame / late fetch fails the export instead of leaving a blank image.
-
Image renderImage() second waitForPrintableContent after content resize now goes through waitForArtifactResources so it is also bounded.
Tests cover: isolated world call contract, no-JSX fast path, runtime JSX waiting for mount, MutationObserver settling via double-rAF, timeout error message, PDF path bypass, late-resource timeout, and resize-after-mount path. artifactRenderTarget abstraction makes the webContents mockable.
xxiaoxiong
left a comment
xxiaoxiong
left a comment
There was a problem hiding this comment.
LGTM. Solves #6658 cleanly. The blank PNG edge case for Babel-compiled runtime JSX comes from the existing waitForPrintableContent finishing before the JSX app actually mounts. The new artifact-export-readiness.ts adds a runtime-JSX-specific readiness check that only delays the image path (PDF keeps the existing fast path): (1) Readiness script injected into isolated world 1001: if no script[type=text/babel|text/jsx], resolve(true) immediately—preserves the static HTML / canvas / CSS fast path. Otherwise probes #root / #app / [data-reactroot] / [data-v-app], records initial .ui-kit-loading placeholder, and waits via MutationObserver + double-rAF until the placeholder is replaced OR a meaningful child mounts. 30s ceiling. (2) waitForArtifactContent wires PDF to settleResources only, image to readiness first then resources. Both resource waits now also have an explicit late-resources timeout. (3) Image renderImage second waitForPrintableContent after content resize now goes through waitForArtifactResources so it is also bounded. Tests cover: isolated world call contract, no-JSX fast path, runtime JSX waiting for mount, MutationObserver settling via double-rAF, timeout error, PDF path bypass, late-resource timeout, and resize-after-mount path. artifactRenderTarget abstraction makes the webContents mockable. A+ fix.
xxiaoxiong
left a comment
xxiaoxiong
left a comment
There was a problem hiding this comment.
LGTM — runtime JSX export readiness is a genuinely hard problem and this PR handles it cleanly.
Verified:
artifactRenderReadinessScript(timeout): runs in the renderer's isolated world (ID 1001) and returns a Promise. Only treats<script type='text/babel'|'text/jsx'>artifacts as runtime-JSX; static-rendered artifacts resolvetrueimmediately. MutationObserver ondocument.documentElement(characterData + childList + subtree) catches React/Vue mounts. To avoid flapping on intermediate states, it requireshasMountedContent()to be true across tworequestAnimationFrameticks before resolving — classic 'paint settled' check.- Placeholder detection:
renderPlaceholder(root)recognises theui-kit-loadingclass so an initial skeleton isn't mistaken for content. Captured per-root in aninitialPlaceholdersMap at script start, then compared after each mutation. - Multiple roots: probes
#root, #app, [data-reactroot], [data-v-app](covers React 16/18 + Vue 2/3 conventions). Falls back todocument.bodydirect children when none match. - Timeout path:
finish(false)aftertimeoutms → caller (waitForRenderedArtifactContent) throws 'Image export timed out waiting for runtime-rendered content (Xms)'.validatedTimeoutrejects non-finite / non-positive inputs upfront. waitForArtifactContent(format, target, settleResources): PDF bypasses the runtime JSX wait (print-ready != React-mounted); image path runswaitForRenderedArtifactContentthen racessettleResources()against a late-resource timeout. Two distinct failure modes, two distinct error messages. Good.jsdomadded toapps/desktopdevDeps — appropriate for unit-testing the readiness script without booting Electron.
One stylistic observation (non-blocking): the injected script is a String-built IIFE because it has to run in the renderer's isolated world; the inline var-style + ES5 syntax is intentional for compatibility with the isolated-world script-injection API. Don't refactor to ES2015+ syntax without confirming the world's script parser accepts it.
4 participants
Fixes #6658
Why
Issue #6658 reports that
od exportcan save a blank PNG when a prototype uses Babel standalone to compile JSX in the browser. The existing export wait covers fonts and images, but it can finish before the runtime JSX app mounts. The command then reports success even though the captured page is still empty.This follows up on the confirmed bug with a narrow readiness check for runtime JSX documents. Static HTML, CSS, and canvas exports keep the existing fast path.
What users will see
Image export now waits for a runtime JSX app to mount before capture. If the app or late resources do not become ready within 30 seconds, export fails with a clear timeout instead of writing a blank image.
PDF export keeps its existing behavior.
Surface area
apps/weborapps/desktop(including Electron menu bar)odsubcommand or flag, newtools-dev/tools-packflag, or newOD_*env var/api/*endpoint, new SSE event, or changed shape inpackages/contractsskills/,design-systems/,design-templates/, orcraft/, or change to the skills protocolTRANSLATIONS.mdfor the locale workflow)package.json(dependenciesordevDependencies); workspace-packagepackage.jsonfiles are out of scope. Include a paragraph on what we get vs. what bytes we ship (seeCONTRIBUTING.md→ Code style)Screenshots
Not applicable. This changes the headless export path and does not add a UI entry point.
Bug fix verification
apps/desktop/tests/main/artifact-export-readiness.test.tsmain: not run as a separate checkout. The test covers the new readiness boundary, which does not exist onmain; the current code moves from resource settling directly to capture.Validation
pnpm guardpnpm typecheck: the desktop package and other checked workspaces passed, but the root command stopped inapps/webbecause the localthreemodule is not installedpnpm --filter @open-design/desktop typecheckpnpm --filter @open-design/desktop build./apps/desktop/node_modules/.bin/vitest run --root apps/desktop -c vitest.config.ts tests/main/artifact-export-readiness.test.ts tests/main/artifact-export-image-height.test.ts: 10 tests passedpnpm --filter @open-design/desktop test: 185 tests passed; 13 existing Electron-dependent suites could not start because the local Electron binary is not installedgit diff --cached --check