diff --git a/packages/mcp/build/lib/collaboration.js b/packages/mcp/build/lib/collaboration.js index 35bd7a13..1fc4c1d6 100644 --- a/packages/mcp/build/lib/collaboration.js +++ b/packages/mcp/build/lib/collaboration.js @@ -4,18 +4,25 @@ import * as Y from "yjs"; import WebSocket from "ws"; import { marked } from "marked"; import { generateJSON } from "@tiptap/html"; -import { getSchema } from "@tiptap/core"; import { Node as PMNode } from "@tiptap/pm/model"; import { updateYFragment } from "y-prosemirror"; import { JSDOM } from "jsdom"; -import { docmostExtensions } from "./docmost-schema.js"; +import { docmostExtensions, docmostSchema } from "./docmost-schema.js"; import { withPageLock } from "./page-lock.js"; import { sanitizeForYjs, findUnstorableAttr } from "./node-ops.js"; import { summarizeChange } from "./diff.js"; -// The ProseMirror schema for the docmost editor, built once (mirrors diff.ts). -// `updateYFragment` needs a real PM Node, so we re-hydrate the transformed JSON -// against this schema before diffing it into the live Yjs fragment. -const docmostSchema = getSchema(docmostExtensions); +/** + * Build the descriptive error for an opaque Yjs encode failure ("Unexpected + * content type"), shared by both encode paths (`buildYDoc` -> `toYdoc` and + * `applyDocToFragment` -> `updateYFragment`) so the message wording stays in one + * place. `label` names the stage that failed (diagnostic). `sanitizeForYjs` + * already stripped `undefined` attrs, so a remaining failure is pinpointed via + * `findUnstorableAttr`. + */ +function unstorableYjsError(safe, label, e) { + const bad = findUnstorableAttr(safe); + return new Error(`Failed to encode document to Yjs (${label}): ${e instanceof Error ? e.message : String(e)}.${bad ? ` Offending attribute: ${bad}.` : " A node/mark attribute likely holds a value Yjs cannot store (e.g. undefined)."}`); +} // Setup DOM environment for Tiptap HTML parsing in Node.js const dom = new JSDOM(""); global.window = dom.window; @@ -453,8 +460,7 @@ export function buildYDoc(doc) { return TiptapTransformer.toYdoc(safe, "default", docmostExtensions); } catch (e) { - const bad = findUnstorableAttr(safe); - throw new Error(`Failed to encode document to Yjs (toYdoc): ${e instanceof Error ? e.message : String(e)}.${bad ? ` Offending attribute: ${bad}.` : " A node/mark attribute likely holds a value Yjs cannot store (e.g. undefined)."}`); + throw unstorableYjsError(safe, "toYdoc", e); } } /** @@ -487,14 +493,18 @@ export function applyDocToFragment(ydoc, newDoc) { }); } catch (e) { - const bad = findUnstorableAttr(safe); - throw new Error(`Failed to encode document to Yjs (updateYFragment): ${e instanceof Error ? e.message : String(e)}.${bad ? ` Offending attribute: ${bad}.` : " A node/mark attribute likely holds a value Yjs cannot store (e.g. undefined)."}`); + throw unstorableYjsError(safe, "updateYFragment", e); } } /** - * Validate that a doc is Yjs-encodable by building (and discarding) a Y.Doc. - * Throws the same descriptive error as the apply path when it is not. Used by - * the dry-run preview so it fails identically to apply. + * Run an independent Yjs-encodability check (the same `sanitizeForYjs` + schema + * the apply path uses) and throw the same descriptive error when the doc cannot + * be stored. Used by the dry-run preview. + * + * Note: it does NOT run `updateYFragment` against the live fragment, so it is an + * encodability GATE, not a byte-for-byte rehearsal of apply — `buildYDoc` + * (`toYdoc`) and `applyDocToFragment` (`updateYFragment`) are two different + * encoders that nonetheless reject the same unstorable attributes. */ export function assertYjsEncodable(doc) { buildYDoc(doc); diff --git a/packages/mcp/build/lib/diff.js b/packages/mcp/build/lib/diff.js index 516a3c81..c19ff9a9 100644 --- a/packages/mcp/build/lib/diff.js +++ b/packages/mcp/build/lib/diff.js @@ -16,13 +16,10 @@ * If recreateTransform / the changeset throws on a pathological document pair, * we fall back to a coarse block-level text diff so the tool never hard-fails. */ -import { getSchema } from "@tiptap/core"; import { Node } from "@tiptap/pm/model"; import { ChangeSet, simplifyChanges } from "@tiptap/pm/changeset"; import { recreateTransform } from "@fellow/prosemirror-recreate-transform"; -import { docmostExtensions } from "./docmost-schema.js"; -/** Build the schema once; it is pure and reused across calls. */ -const schema = getSchema(docmostExtensions); +import { docmostSchema } from "./docmost-schema.js"; /** Recursively concatenate the plain text of a JSON node. */ function plainText(node) { if (!node || typeof node !== "object") @@ -242,8 +239,8 @@ export function diffDocs(oldDocJson, newDocJson, notesHeading = "Примеча let fellBack = false; const changedBlocks = new Set(); try { - const oldNode = Node.fromJSON(schema, oldDocJson); - const newNode = Node.fromJSON(schema, newDocJson); + const oldNode = Node.fromJSON(docmostSchema, oldDocJson); + const newNode = Node.fromJSON(docmostSchema, newDocJson); const tr = recreateTransform(oldNode, newNode, { complexSteps: false, wordDiffs: true, diff --git a/packages/mcp/build/lib/docmost-schema.js b/packages/mcp/build/lib/docmost-schema.js index 976e2d7f..6b6c221d 100644 --- a/packages/mcp/build/lib/docmost-schema.js +++ b/packages/mcp/build/lib/docmost-schema.js @@ -14,7 +14,7 @@ import TaskItem from "@tiptap/extension-task-item"; import Highlight from "@tiptap/extension-highlight"; import Subscript from "@tiptap/extension-subscript"; import Superscript from "@tiptap/extension-superscript"; -import { Node, Extension, Mark } from "@tiptap/core"; +import { Node, Extension, Mark, getSchema } from "@tiptap/core"; // Inlined from @tiptap/core's getStyleProperty (added after 3.20.x) so this // package can stay on the same @tiptap/core version as the editor and avoid a // duplicate-tiptap version split in the monorepo. Reads a single declaration @@ -1126,3 +1126,10 @@ export const docmostExtensions = [ PageBreak, DocmostAttributes, ]; +/** + * The ProseMirror schema for the docmost editor, built ONCE from + * `docmostExtensions`. Pure and reused by every consumer (diff, collaboration + * write-back) so the schema can never drift between call sites — it lives next + * to the extension list it is derived from. + */ +export const docmostSchema = getSchema(docmostExtensions); diff --git a/packages/mcp/src/client.ts b/packages/mcp/src/client.ts index 9873d119..bd891fc9 100644 --- a/packages/mcp/src/client.ts +++ b/packages/mcp/src/client.ts @@ -2995,9 +2995,9 @@ export class DocmostClient { const raw = await this.getPageRaw(pageId); const current = raw.content || { type: "doc", content: [] }; runTransform(current); - // Exercise the same Yjs encoder the apply path uses, so the preview - // fails with the SAME descriptive error when the doc is not encodable - // instead of returning a misleadingly-green diff. + // Run an independent Yjs-encodability check (same sanitize + schema as the + // apply path), so the preview fails with the same descriptive error when + // the doc is not encodable instead of returning a misleadingly-green diff. assertYjsEncodable(newDoc); return { pushed: false, diff --git a/packages/mcp/src/lib/collaboration.ts b/packages/mcp/src/lib/collaboration.ts index cb84f410..7d3fdc0e 100644 --- a/packages/mcp/src/lib/collaboration.ts +++ b/packages/mcp/src/lib/collaboration.ts @@ -4,19 +4,28 @@ import * as Y from "yjs"; import WebSocket from "ws"; import { marked } from "marked"; import { generateJSON } from "@tiptap/html"; -import { getSchema } from "@tiptap/core"; import { Node as PMNode } from "@tiptap/pm/model"; import { updateYFragment } from "y-prosemirror"; import { JSDOM } from "jsdom"; -import { docmostExtensions } from "./docmost-schema.js"; +import { docmostExtensions, docmostSchema } from "./docmost-schema.js"; import { withPageLock } from "./page-lock.js"; import { sanitizeForYjs, findUnstorableAttr } from "./node-ops.js"; import { summarizeChange, VerifyReport } from "./diff.js"; -// The ProseMirror schema for the docmost editor, built once (mirrors diff.ts). -// `updateYFragment` needs a real PM Node, so we re-hydrate the transformed JSON -// against this schema before diffing it into the live Yjs fragment. -const docmostSchema = getSchema(docmostExtensions); +/** + * Build the descriptive error for an opaque Yjs encode failure ("Unexpected + * content type"), shared by both encode paths (`buildYDoc` -> `toYdoc` and + * `applyDocToFragment` -> `updateYFragment`) so the message wording stays in one + * place. `label` names the stage that failed (diagnostic). `sanitizeForYjs` + * already stripped `undefined` attrs, so a remaining failure is pinpointed via + * `findUnstorableAttr`. + */ +function unstorableYjsError(safe: any, label: string, e: unknown): Error { + const bad = findUnstorableAttr(safe); + return new Error( + `Failed to encode document to Yjs (${label}): ${e instanceof Error ? e.message : String(e)}.${bad ? ` Offending attribute: ${bad}.` : " A node/mark attribute likely holds a value Yjs cannot store (e.g. undefined)."}`, + ); +} /** * The resolved value of every content-mutating collab write: the document that @@ -507,10 +516,7 @@ export function buildYDoc(doc: any): Y.Doc { try { return TiptapTransformer.toYdoc(safe, "default", docmostExtensions); } catch (e) { - const bad = findUnstorableAttr(safe); - throw new Error( - `Failed to encode document to Yjs (toYdoc): ${e instanceof Error ? e.message : String(e)}.${bad ? ` Offending attribute: ${bad}.` : " A node/mark attribute likely holds a value Yjs cannot store (e.g. undefined)."}`, - ); + throw unstorableYjsError(safe, "toYdoc", e); } } @@ -543,17 +549,19 @@ export function applyDocToFragment(ydoc: Y.Doc, newDoc: any): void { }); }); } catch (e) { - const bad = findUnstorableAttr(safe); - throw new Error( - `Failed to encode document to Yjs (updateYFragment): ${e instanceof Error ? e.message : String(e)}.${bad ? ` Offending attribute: ${bad}.` : " A node/mark attribute likely holds a value Yjs cannot store (e.g. undefined)."}`, - ); + throw unstorableYjsError(safe, "updateYFragment", e); } } /** - * Validate that a doc is Yjs-encodable by building (and discarding) a Y.Doc. - * Throws the same descriptive error as the apply path when it is not. Used by - * the dry-run preview so it fails identically to apply. + * Run an independent Yjs-encodability check (the same `sanitizeForYjs` + schema + * the apply path uses) and throw the same descriptive error when the doc cannot + * be stored. Used by the dry-run preview. + * + * Note: it does NOT run `updateYFragment` against the live fragment, so it is an + * encodability GATE, not a byte-for-byte rehearsal of apply — `buildYDoc` + * (`toYdoc`) and `applyDocToFragment` (`updateYFragment`) are two different + * encoders that nonetheless reject the same unstorable attributes. */ export function assertYjsEncodable(doc: any): void { buildYDoc(doc); diff --git a/packages/mcp/src/lib/diff.ts b/packages/mcp/src/lib/diff.ts index d0848997..ba216df4 100644 --- a/packages/mcp/src/lib/diff.ts +++ b/packages/mcp/src/lib/diff.ts @@ -17,11 +17,10 @@ * we fall back to a coarse block-level text diff so the tool never hard-fails. */ -import { getSchema } from "@tiptap/core"; import { Node } from "@tiptap/pm/model"; import { ChangeSet, simplifyChanges } from "@tiptap/pm/changeset"; import { recreateTransform } from "@fellow/prosemirror-recreate-transform"; -import { docmostExtensions } from "./docmost-schema.js"; +import { docmostSchema } from "./docmost-schema.js"; /** A single inserted/deleted change with its containing-block context. */ export interface DiffChange { @@ -49,8 +48,6 @@ export interface DiffResult { markdown: string; } -/** Build the schema once; it is pure and reused across calls. */ -const schema = getSchema(docmostExtensions); /** Recursively concatenate the plain text of a JSON node. */ function plainText(node: any): string { @@ -288,8 +285,8 @@ export function diffDocs( const changedBlocks = new Set(); try { - const oldNode = Node.fromJSON(schema, oldDocJson); - const newNode = Node.fromJSON(schema, newDocJson); + const oldNode = Node.fromJSON(docmostSchema, oldDocJson); + const newNode = Node.fromJSON(docmostSchema, newDocJson); const tr = recreateTransform(oldNode, newNode, { complexSteps: false, wordDiffs: true, diff --git a/packages/mcp/src/lib/docmost-schema.ts b/packages/mcp/src/lib/docmost-schema.ts index 63bef5c2..546b9844 100644 --- a/packages/mcp/src/lib/docmost-schema.ts +++ b/packages/mcp/src/lib/docmost-schema.ts @@ -14,7 +14,7 @@ import TaskItem from "@tiptap/extension-task-item"; import Highlight from "@tiptap/extension-highlight"; import Subscript from "@tiptap/extension-subscript"; import Superscript from "@tiptap/extension-superscript"; -import { Node, Extension, Mark } from "@tiptap/core"; +import { Node, Extension, Mark, getSchema } from "@tiptap/core"; // Inlined from @tiptap/core's getStyleProperty (added after 3.20.x) so this // package can stay on the same @tiptap/core version as the editor and avoid a @@ -1223,3 +1223,11 @@ export const docmostExtensions = [ PageBreak, DocmostAttributes, ]; + +/** + * The ProseMirror schema for the docmost editor, built ONCE from + * `docmostExtensions`. Pure and reused by every consumer (diff, collaboration + * write-back) so the schema can never drift between call sites — it lives next + * to the extension list it is derived from. + */ +export const docmostSchema = getSchema(docmostExtensions); diff --git a/packages/mcp/test/unit/comment-cursor-stability.test.mjs b/packages/mcp/test/unit/comment-cursor-stability.test.mjs index e494d131..517d65e5 100644 --- a/packages/mcp/test/unit/comment-cursor-stability.test.mjs +++ b/packages/mcp/test/unit/comment-cursor-stability.test.mjs @@ -73,3 +73,21 @@ test("anchoring a comment mark keeps the cursor in the marked text (issue #152)" const abs = Y.createAbsolutePositionFromRelativePosition(relPos, ydoc); assert.notEqual(abs, null, "comment anchoring must not destroy the cursor anchor"); }); + +// The diagnostic catch branch of applyDocToFragment (#154 review): a doc that +// cannot be hydrated/encoded must be re-thrown wrapped with the stage label, not +// leak the raw ProseMirror/Yjs error. An unknown node type makes +// PMNode.fromJSON (against the docmost schema) throw — a reliable trigger +// (sanitizeForYjs only strips `undefined`, so an undefined attr would be removed +// before it could fail). +test("applyDocToFragment wraps an encode/build failure with the (updateYFragment) diagnostic", () => { + const ydoc = new Y.Doc(); + const bad = { + type: "doc", + content: [{ type: "totally_unknown_node_xyz_12345" }], + }; + assert.throws( + () => applyDocToFragment(ydoc, bad), + /Failed to encode document to Yjs \(updateYFragment\)/, + ); +});