add entrypoint-based custom export api - #197
Conversation
|
Confirmed — Now let me reconsider the HTML export CSP more carefully as it's the main new security surface. The static HTML CSP allows Let me verify the sanitizer runs in the isolated realm and operates on the main world's DOM string. In I've completed a thorough review. Let me check one last thing — whether Let me verify the The handler param type in I've reviewed all 29 files. Let me summarize my findings. The overall implementation is careful and well-tested, with strong attention to resource cleanup, timeouts, CSP, and sanitization. I found no blocking issues. Let me note a couple of minor observations. I reviewed the full PR. This is a well-constructed change that extends the existing PDF export into a general export API (HTML/PNG/JPG/PDF browser modes plus server-mode custom handlers). The implementation is careful about the things that matter most here — resource cleanup, wall-clock deadlines, size caps, CSP, and HTML sanitization — and it's backed by thorough unit tests on both the backend and frontend. Observations (none blocking):
Kernel-review points I specifically checked and found clean: LGTM! |
- implement design proposed in #114
- harden remote browser request handling - apply size limits to exported html and screenshots
2d84940 to
791b03c
Compare
|
I've completed a thorough review. This PR was already reviewed by ask-bonk (concluded LGTM), and the subsequent commits addressed prior findings (screenshot pixel caps, byte limits, extension regex alignment, TOCTOU fix). Let me do a final consideration of the key security surfaces and correctness. Key things verified:
The prior review already surfaced the minor fragility notes (workerd error-string coupling in LGTM! |
|
I have completed a thorough independent review of all 29 changed files. This PR was already reviewed twice by ask-bonk (both concluding LGTM), and the subsequent commits addressed the earlier local AI review findings (screenshot pixel caps, byte limits, extension regex alignment, TOCTOU fix, print-media-only-for-PDF). My independent pass confirms those conclusions and I found no new actionable issues. Points I verified independently:
The prior reviews' minor non-blocking notes remain accurate (workerd error-string coupling in LGTM! |
| Make Gadget UIs responsive and usable on both desktop and phones by default. | ||
|
|
||
| Every Gadget UI can be exported to PDF using platform-owned controls outside the Gadget. Never add print or export UI to a Gadget and never call \`window.print()\`. When asked to support or improve PDF export, only add standard print CSS such as \`@media print\`, \`@page\`, and CSS fragmentation properties so the PDF remains readable. | ||
| Every Gadget UI can be exported to HTML or PDF using platform-owned controls outside the Gadget. Never add print or export UI to a Gadget and never call \`window.print()\`. Browser-mode PDF exports render using print media; HTML, PNG, and JPEG exports render using screen media. When asked to support or improve PDF export, use standard print CSS such as \`@media print\`, \`@page\`, and CSS fragmentation properties so the output remains readable. |
There was a problem hiding this comment.
At this point we should probably split all the export stuff into its own section with a heading.
| Every Gadget UI can be exported to PDF using platform-owned controls outside the Gadget. Never add print or export UI to a Gadget and never call \`window.print()\`. When asked to support or improve PDF export, only add standard print CSS such as \`@media print\`, \`@page\`, and CSS fragmentation properties so the PDF remains readable. | ||
| Every Gadget UI can be exported to HTML or PDF using platform-owned controls outside the Gadget. Never add print or export UI to a Gadget and never call \`window.print()\`. Browser-mode PDF exports render using print media; HTML, PNG, and JPEG exports render using screen media. When asked to support or improve PDF export, use standard print CSS such as \`@media print\`, \`@page\`, and CSS fragmentation properties so the output remains readable. | ||
|
|
||
| During a browser-mode export, the client context is initialized with another special global variable named \`gadgetExportFormatId\`. This variable is only defined during export; during normal interactive rendering, referencing it directly throws a \`ReferenceError\`. Guard access with \`typeof gadgetExportFormatId !== "undefined"\` or read \`globalThis.gadgetExportFormatId\`. Use \`gadgetExportFormatId\` when the Gadget supports multiple HTML, PDF, PNG, or JPEG export variants. Do not declare or import \`gadgetExportFormatId\` in client.js. |
There was a problem hiding this comment.
Does the gadget need to signal somehow when it's ready to render?
Could we say: Use top-level await to wait until the the UI is fully rendered.
I like this since it feels natural and doesn't require defining any further API surfaces.
|
|
||
| During a browser-mode export, the client context is initialized with another special global variable named \`gadgetExportFormatId\`. This variable is only defined during export; during normal interactive rendering, referencing it directly throws a \`ReferenceError\`. Guard access with \`typeof gadgetExportFormatId !== "undefined"\` or read \`globalThis.gadgetExportFormatId\`. Use \`gadgetExportFormatId\` when the Gadget supports multiple HTML, PDF, PNG, or JPEG export variants. Do not declare or import \`gadgetExportFormatId\` in client.js. | ||
|
|
||
| To add, replace, or disable export formats, server.js may export a class named \`ExportHandler\`, which must extend \`WorkerEntrypoint\`. Its \`getExportFormats(gadget)\` method returns the complete list of formats, and its \`export(gadget, id)\` method returns a \`ReadableStream<Uint8Array>\` for formats whose mode is \`"server"\`. Read any needed Gadget state before \`export()\` returns; do not capture the borrowed \`gadget\` parameter in the returned stream. If \`getExportFormats(gadget)\` returns only browser-mode formats, do not implement \`export(gadget, id)\`. \`export\` is valid as a JavaScript class method name; write it directly as \`async export(gadget, id)\`, without quoting it or using a computed property. Browser mode supports \`text/html\`, \`application/pdf\`, \`image/png\`, and \`image/jpeg\`; server mode supports any media type. Each format must contain a unique non-empty \`id\`, a \`label\`, a \`mode\`, a \`contentType\`, and a \`fileExtension\` beginning with a dot. Returning an empty list disables export. The Workshop supplies default HTML and PDF formats only when server.js does not export \`ExportHandler\` at all. |
There was a problem hiding this comment.
We should provide example code.
| return renderGadgetInBrowser(browser, bundle.jsCode, title, browserGadget, format); | ||
| } | ||
|
|
||
| prepareGadgetChat(chatId?: number): void { |
There was a problem hiding this comment.
Name seems kinda vague.
Maybe: checkChatExistsAndMaterializeDrafts
| if (format.mode === "server") { | ||
| if (!handler) throw new Error("The Gadget export handler is unavailable."); | ||
| using gadget = await this.getGadgetFacet(gadgetId, chatId); | ||
| // Cap'n Web and native RPC stubs interoperate at runtime, but their types do not yet. |
There was a problem hiding this comment.
Hmm, getGadgetFacet() actually returns a NativeRpcStub that has been coerced to Cap'n Web RpcStub, so this is just coercing it back. That's going to need some cleanup at some point but maybe not in this PR.
|
|
||
| if (format.mode === "server") { | ||
| if (!handler) throw new Error("The Gadget export handler is unavailable."); | ||
| using gadget = await this.getGadgetFacet(gadgetId, chatId); |
There was a problem hiding this comment.
#resolveGadgetExportFormats also calls this internally, maybe it should just return the stub rather than acquire a new one here?
| if (!browser) throw new Error("Gadget export is not configured for this deployment."); | ||
| let bundle = this.getGadgetUiBundle(gadgetId, chatId); | ||
| if (!bundle) throw new Error("This Gadget does not have a UI to export."); | ||
| let browserGadget = await this.getGadgetFacet(gadgetId, chatId); |
There was a problem hiding this comment.
Here too we could potentially reuse the stub from #resolveGadgetExportFormats.
| // Cap'n Web and native RPC stubs interoperate at runtime, but their types do not yet. | ||
| return await exportServerFormat(() => | ||
| handler.export(gadget as unknown as NativeRpcStub<any>, format.id)); | ||
| } |
There was a problem hiding this comment.
Nitpick but I feel like } else { here wrapping the rest of the function would be clearer.
Implements the Gadget file export API design proposed in #114, which I would read first (it's small). The API interface is unchanged in this PR.
The implementation is an extension of the existing PDF export infrastructure. The same remote browser setup is extended to support HTML, PNG, and JPG exports in addition to PDF. DOMPurify is added as a dependency to assist with HTML sanitization on export. This is slightly unfortunate since newer browsers support the HTML Sanitizer API which we could use instead, but the older version of Chromium used by Cloudflare's remote browsers does not support this API. I also refactored functions passed to
page.evaluate()out into a newpackages/workshop-backend/browser/browser-export-page.tsfile so they can be properly typed for the browser environment that they run in.This PR doesn't attempt to detect or prevent Gadget code changes during the export process. It's possible that Gadget code will change between the user selecting an export format and the exported file being produced. This came up repeatedly in local AI code review, but I'm not sure it's a big problem in practice. Exports may fail or produce mismatched file types if Gadget code changes at specific points in the process. We could detect these changes and abort the export, but this feels harsh since most code changes are unlikely to be problematic. Gadget export code (if any exists) should be infrequently updated and cover a small fraction of all total Gadget code. More sophisticated change detection and revision pinning across export operations both add complexity that I wasn't sure we needed. If we did want to revisit this in the future, we could add stronger guardrails without changing the current Gadget API interface.