From 3f7e1bdc7bfa38cf59757cc9b42c058f93f4f757 Mon Sep 17 00:00:00 2001 From: agent_coder Date: Fri, 3 Jul 2026 01:34:53 +0300 Subject: [PATCH 1/2] fix(export): stop comment.renderHTML returning a live jsdom node on the server (#298) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Page/space export (Markdown & HTML, both via jsonToHtml -> generateHTML) crashed with "Export failed:undefined" on any page carrying a `comment` mark. Root cause: comment.renderHTML returned a LIVE DOM node (document.createElement + a click listener) whenever a global `document` existed — and the in-process MCP module injects a jsdom global.window+global.document into the Node server, defeating the old `typeof document === "undefined"` guard. The server export runs happy-dom's DOMSerializer, which crashes appending the foreign jsdom node (NodeUtility.isInclusiveAncestor -> "Cannot read properties of undefined (reading 'length')"). comment is the only extension returning a live node. Fix: widen the guard with an isNodeRuntime check (process.versions.node) so on any Node runtime renderHTML returns the plain, serializable spec array — even when MCP injected jsdom globals. The browser branch (createElement + click -> ACTIVE_COMMENT_EVENT) is untouched, so in-editor comment interactivity is preserved (Vite defines only process.env as a member-expression substitution, no `process` object in the browser bundle, so isNodeRuntime is false there). The mcp schema mirror already returns a spec array and is not on the export path (tiptapExtensions imports Comment from @docmost/editor-ext), so no mirror change is needed. Also: export-modal now reads the real error text from the response Blob (responseType:'blob' made err.response.data.message always undefined) so a failed export shows the server's message instead of "undefined". Adds a regression test that runs the real jsonToHtml on a comment-marked doc with jsdom globals injected (reproduces the crash on the unpatched code, passes after). closes #298 Co-Authored-By: Claude Opus 4.8 (1M context) --- .../src/components/common/export-modal.tsx | 19 ++++- .../export/export-comment.spec.ts | 82 +++++++++++++++++++ .../editor-ext/src/lib/comment/comment.ts | 15 +++- 3 files changed, 114 insertions(+), 2 deletions(-) create mode 100644 apps/server/src/integrations/export/export-comment.spec.ts diff --git a/apps/client/src/components/common/export-modal.tsx b/apps/client/src/components/common/export-modal.tsx index 2a83debf..274c18ed 100644 --- a/apps/client/src/components/common/export-modal.tsx +++ b/apps/client/src/components/common/export-modal.tsx @@ -14,6 +14,22 @@ import { notifications } from "@mantine/notifications"; import { exportSpace } from "@/features/space/services/space-service"; import { useTranslation } from "react-i18next"; +// The export request uses `responseType: "blob"`, so a server error body arrives +// as a Blob rather than parsed JSON — `err.response?.data.message` is therefore +// always undefined. Read and parse the blob to surface the real error message. +async function extractExportError(err: any): Promise { + const data = err?.response?.data; + if (data instanceof Blob) { + try { + const json = JSON.parse(await data.text()); + return json?.message ?? ""; + } catch { + return ""; + } + } + return data?.message ?? err?.message ?? ""; +} + interface ExportModalProps { id: string; type: "space" | "page"; @@ -52,8 +68,9 @@ export default function ExportModal({ }); onClose(); } catch (err) { + const message = await extractExportError(err); notifications.show({ - message: "Export failed:" + err.response?.data.message, + message: t("Export failed") + (message ? `: ${message}` : ""), color: "red", }); console.error("export error", err); diff --git a/apps/server/src/integrations/export/export-comment.spec.ts b/apps/server/src/integrations/export/export-comment.spec.ts new file mode 100644 index 00000000..7805f7ab --- /dev/null +++ b/apps/server/src/integrations/export/export-comment.spec.ts @@ -0,0 +1,82 @@ +import { JSDOM } from 'jsdom'; +import { jsonToHtml } from '../../collaboration/collaboration.util'; + +/** + * Regression test for issue #298: page/space export (Markdown/HTML) crashes on + * pages that contain inline comments. + * + * The in-process MCP module injects a jsdom `global.window` + `global.document` + * into the Node server (see packages/mcp/src/lib/collaboration.ts). Before the + * fix, the comment mark's `renderHTML` guard was only + * `typeof window === "undefined" || typeof document === "undefined"`, so with + * BOTH jsdom globals present it took the interactive browser branch and returned + * a LIVE jsdom node. The export path serializes via happy-dom's + * DOMSerializer, and appending a foreign jsdom node crashed happy-dom + * ("Cannot read properties of undefined (reading 'length')"). + * + * We reproduce the MCP-loaded server by injecting jsdom globals, then export a + * doc containing a comment mark and assert the serialization SUCCEEDS and emits + * the expected serializable . + * + * Non-vacuity: this test only exercises the buggy branch because BOTH jsdom + * `window` AND `document` are set below. If the `isNodeRuntime` condition is + * removed from the guard in packages/editor-ext/src/lib/comment/comment.ts, + * `renderHTML` returns a live jsdom node and `jsonToHtml` throws — this test + * then fails. (In a plain node env without the injected globals the guard's + * `typeof window === "undefined"` clause already short-circuits, so it is the + * injected globals that make this assertion meaningful.) + */ +describe('export with inline comments (issue #298)', () => { + const originalWindow = (global as any).window; + const originalDocument = (global as any).document; + + beforeAll(() => { + const dom = new JSDOM(''); + (global as any).window = dom.window; + (global as any).document = dom.window.document; + }); + + afterAll(() => { + (global as any).window = originalWindow; + (global as any).document = originalDocument; + }); + + const docWithComment = (resolved: boolean) => ({ + type: 'doc', + content: [ + { + type: 'paragraph', + content: [ + { + type: 'text', + marks: [ + { + type: 'comment', + attrs: { commentId: 'c-123', resolved }, + }, + ], + text: 'commented text', + }, + ], + }, + ], + }); + + it('exports a page with an unresolved comment mark without crashing', () => { + let html: string; + expect(() => { + html = jsonToHtml(docWithComment(false)); + }).not.toThrow(); + + expect(html).toContain('data-comment-id="c-123"'); + expect(html).toContain('class="comment-mark"'); + expect(html).toContain('commented text'); + }); + + it('exports a resolved comment mark with the resolved class/attr', () => { + const html = jsonToHtml(docWithComment(true)); + expect(html).toContain('data-comment-id="c-123"'); + expect(html).toContain('comment-mark resolved'); + expect(html).toContain('data-resolved="true"'); + }); +}); diff --git a/packages/editor-ext/src/lib/comment/comment.ts b/packages/editor-ext/src/lib/comment/comment.ts index ec896357..9d6680d2 100644 --- a/packages/editor-ext/src/lib/comment/comment.ts +++ b/packages/editor-ext/src/lib/comment/comment.ts @@ -172,7 +172,20 @@ export const Comment = Mark.create({ const commentId = HTMLAttributes?.["data-comment-id"] || null; const resolved = HTMLAttributes?.["data-resolved"] || false; - if (typeof window === "undefined" || typeof document === "undefined") { + // The in-process MCP module injects a jsdom `global.document` into the Node + // server, so `typeof document === "undefined"` is not enough to detect SSR. + // On any Node runtime always return a plain, serializable spec array; the + // interactive live-DOM branch below is browser-only. This stops server-side + // HTML/Markdown export (happy-dom DOMSerializer) from appending a foreign + // jsdom node into a happy-dom tree. + const isNodeRuntime = + typeof process !== "undefined" && !!process.versions?.node; + + if ( + typeof window === "undefined" || + typeof document === "undefined" || + isNodeRuntime + ) { return [ "span", mergeAttributes(this.options.HTMLAttributes, HTMLAttributes, { -- 2.52.0 From 4d8315da5c656d141a7a83118d405fc239181052 Mon Sep 17 00:00:00 2001 From: agent_coder Date: Fri, 3 Jul 2026 02:29:09 +0300 Subject: [PATCH 2/2] docs(#298 review): document the browser-safety invariant of the isNodeRuntime guard (F1) The whole fix's correctness rests on isNodeRuntime being false in the browser (so the interactive live-DOM comment branch still runs), and that is NOT covered by any test (client vitest runs under jsdom->node where isNodeRuntime is true). Document it: Vite substitutes only process.env, not the bare process object, so typeof process is undefined in the client bundle; do not add a process polyfill without revisiting this guard, or comment interactivity dies silently. Co-Authored-By: Claude Opus 4.8 (1M context) --- packages/editor-ext/src/lib/comment/comment.ts | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/packages/editor-ext/src/lib/comment/comment.ts b/packages/editor-ext/src/lib/comment/comment.ts index 9d6680d2..7d59cef5 100644 --- a/packages/editor-ext/src/lib/comment/comment.ts +++ b/packages/editor-ext/src/lib/comment/comment.ts @@ -178,6 +178,14 @@ export const Comment = Mark.create({ // interactive live-DOM branch below is browser-only. This stops server-side // HTML/Markdown export (happy-dom DOMSerializer) from appending a foreign // jsdom node into a happy-dom tree. + // Safe in the browser: Vite substitutes only `process.env` (a member + // expression), NOT the bare `process` object, so `typeof process` is + // "undefined" in the client bundle → isNodeRuntime is false → the interactive + // live-DOM branch below still runs and comment marks stay clickable in the + // editor. This browser-safety is load-bearing and NOT covered by a test + // (client vitest runs under jsdom→node, where isNodeRuntime is true). Do NOT + // add a `process` polyfill (e.g. vite-plugin-node-polyfills) without + // revisiting this guard, or comment interactivity dies silently. const isNodeRuntime = typeof process !== "undefined" && !!process.versions?.node; -- 2.52.0