From f86b8b69a06469cdfae76e7a1dd9599efb443516 Mon Sep 17 00:00:00 2001 From: claude code agent 227 Date: Wed, 24 Jun 2026 05:56:05 +0300 Subject: [PATCH 1/3] fix(mcp): structural-diff write-back so agent edits don't jump the cursor (#152) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit mutatePageContent wrote agent edits back by DELETING the whole Yjs fragment and re-applying a fresh Y.Doc. Yjs is a CRDT — the editor anchors its selection to node ids — so wiping every id made an open editor's cursor lose its anchor and snap to the end of the document on every agent write. It was most visible on comment anchoring (issue #152): a comment changes no text, yet the cursor jumped. (Before commit 4201f0a3 the anchoring silently no-op'd, so the destructive write never ran for comments — hence the regression.) Fix: write via `updateYFragment` (y-prosemirror) — the same routine the editor uses to sync its own edits into Yjs. It structurally diffs the new doc against the live fragment and touches only changed nodes, preserving the ids of unchanged ones, so the cursor stays put. This improves ALL agent write tools (text edits, node ops, comments, replace) — minimal diff instead of full replace: less collab noise, stable block-ids, other users' cursors no longer disrupted. - collaboration.ts: new `applyDocToFragment` (sanitize -> PMNode.fromJSON against a memoized docmost schema -> updateYFragment in one transact), keeping the `findUnstorableAttr` encode diagnostic; swap the destructive write-back for it. - package.json: `y-prosemirror` promoted to a direct dependency (was transitive). - test: comment-cursor-stability.test.mjs — a Yjs RelativePosition (the cursor anchor) survives both a sibling edit and a comment-mark anchoring (the old full-replace tombstoned it -> null). 292 package tests green. Co-Authored-By: Claude Opus 4.8 --- packages/mcp/build/lib/collaboration.js | 55 +++++++++++--- packages/mcp/node_modules/y-prosemirror | 1 + packages/mcp/package.json | 1 + packages/mcp/src/lib/collaboration.ts | 58 +++++++++++--- .../unit/comment-cursor-stability.test.mjs | 75 +++++++++++++++++++ pnpm-lock.yaml | 3 + 6 files changed, 173 insertions(+), 20 deletions(-) create mode 120000 packages/mcp/node_modules/y-prosemirror create mode 100644 packages/mcp/test/unit/comment-cursor-stability.test.mjs diff --git a/packages/mcp/build/lib/collaboration.js b/packages/mcp/build/lib/collaboration.js index 5140acee..35bd7a13 100644 --- a/packages/mcp/build/lib/collaboration.js +++ b/packages/mcp/build/lib/collaboration.js @@ -4,11 +4,18 @@ 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 { 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); // Setup DOM environment for Tiptap HTML parsing in Node.js const dom = new JSDOM(""); global.window = dom.window; @@ -450,6 +457,40 @@ export function buildYDoc(doc) { 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)."}`); } } +/** + * Write a new ProseMirror doc into the live Yjs fragment by STRUCTURAL DIFF, + * preserving the Yjs identity of unchanged nodes (issue #152). + * + * The previous approach deleted the whole fragment and re-applied a fresh Y.Doc, + * which discarded every Yjs node id. y-prosemirror anchors the editor selection + * to those ids, so an open editor's cursor lost its anchor and snapped to the + * end of the document on every agent write (most visibly on comment anchoring, + * which changes no text at all). `updateYFragment` is exactly the routine the + * editor itself uses to sync ProseMirror edits into Yjs: it diffs the new node + * against the current fragment and touches only the changed children, so + * unchanged nodes keep their ids and the live cursor stays put. + * + * Must run inside a single `transact` so the diff applies atomically (no remote + * update interleaves). Keeps `buildYDoc`'s `findUnstorableAttr` diagnostic for + * the opaque "Unexpected content type" encode failure. + */ +export function applyDocToFragment(ydoc, newDoc) { + const safe = sanitizeForYjs(newDoc); + const fragment = ydoc.getXmlFragment("default"); + try { + const pmNode = PMNode.fromJSON(docmostSchema, safe); + ydoc.transact(() => { + updateYFragment(ydoc, fragment, pmNode, { + mapping: new Map(), + isOMark: new Map(), + }); + }); + } + 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)."}`); + } +} /** * 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 @@ -649,16 +690,10 @@ export async function mutatePageContent(pageId, collabToken, baseUrl, transform) finish(null, mutationResult); return; } - const tempDoc = buildYDoc(newDoc); - // Fetch the fragment immediately before the transact that mutates - // it, rather than reusing a handle grabbed across the transform. - const fragment = ydoc.getXmlFragment("default"); - ydoc.transact(() => { - if (fragment.length > 0) { - fragment.delete(0, fragment.length); - } - Y.applyUpdate(ydoc, Y.encodeStateAsUpdate(tempDoc)); - }); + // Structural diff into the live fragment (issue #152): preserves + // the Yjs ids of unchanged nodes, so an open editor's cursor is not + // yanked to the end of the document on every agent write. + applyDocToFragment(ydoc, newDoc); } catch (e) { // Includes errors thrown by transform (e.g. "afterText not found", diff --git a/packages/mcp/node_modules/y-prosemirror b/packages/mcp/node_modules/y-prosemirror new file mode 120000 index 00000000..16997d1b --- /dev/null +++ b/packages/mcp/node_modules/y-prosemirror @@ -0,0 +1 @@ +../../../node_modules/.pnpm/y-prosemirror@1.3.7_prosemirror-model@1.25.1_prosemirror-state@1.4.3_prosemirror-view@1_0ad6648b7e1f6d6f3287a40e0e62139b/node_modules/y-prosemirror \ No newline at end of file diff --git a/packages/mcp/package.json b/packages/mcp/package.json index 2b1074fb..3edc1902 100644 --- a/packages/mcp/package.json +++ b/packages/mcp/package.json @@ -52,6 +52,7 @@ "jsdom": "^27.4.0", "marked": "^17.0.1", "ws": "^8.19.0", + "y-prosemirror": "1.3.7", "yjs": "^13.6.29", "zod": "^3.22.0" }, diff --git a/packages/mcp/src/lib/collaboration.ts b/packages/mcp/src/lib/collaboration.ts index 6f0ad011..cb84f410 100644 --- a/packages/mcp/src/lib/collaboration.ts +++ b/packages/mcp/src/lib/collaboration.ts @@ -4,12 +4,20 @@ 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 { 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); + /** * The resolved value of every content-mutating collab write: the document that * was written (or the live doc when the transform aborted) plus a verifiable @@ -506,6 +514,42 @@ export function buildYDoc(doc: any): Y.Doc { } } +/** + * Write a new ProseMirror doc into the live Yjs fragment by STRUCTURAL DIFF, + * preserving the Yjs identity of unchanged nodes (issue #152). + * + * The previous approach deleted the whole fragment and re-applied a fresh Y.Doc, + * which discarded every Yjs node id. y-prosemirror anchors the editor selection + * to those ids, so an open editor's cursor lost its anchor and snapped to the + * end of the document on every agent write (most visibly on comment anchoring, + * which changes no text at all). `updateYFragment` is exactly the routine the + * editor itself uses to sync ProseMirror edits into Yjs: it diffs the new node + * against the current fragment and touches only the changed children, so + * unchanged nodes keep their ids and the live cursor stays put. + * + * Must run inside a single `transact` so the diff applies atomically (no remote + * update interleaves). Keeps `buildYDoc`'s `findUnstorableAttr` diagnostic for + * the opaque "Unexpected content type" encode failure. + */ +export function applyDocToFragment(ydoc: Y.Doc, newDoc: any): void { + const safe = sanitizeForYjs(newDoc); + const fragment = ydoc.getXmlFragment("default"); + try { + const pmNode = PMNode.fromJSON(docmostSchema, safe); + ydoc.transact(() => { + updateYFragment(ydoc, fragment, pmNode, { + mapping: new Map(), + isOMark: new Map(), + }); + }); + } 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)."}`, + ); + } +} + /** * 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 @@ -727,16 +771,10 @@ export async function mutatePageContent( return; } - const tempDoc = buildYDoc(newDoc); - // Fetch the fragment immediately before the transact that mutates - // it, rather than reusing a handle grabbed across the transform. - const fragment = ydoc.getXmlFragment("default"); - ydoc.transact(() => { - if (fragment.length > 0) { - fragment.delete(0, fragment.length); - } - Y.applyUpdate(ydoc, Y.encodeStateAsUpdate(tempDoc)); - }); + // Structural diff into the live fragment (issue #152): preserves + // the Yjs ids of unchanged nodes, so an open editor's cursor is not + // yanked to the end of the document on every agent write. + applyDocToFragment(ydoc, newDoc); } catch (e) { // Includes errors thrown by transform (e.g. "afterText not found", // "text not found"): propagate them verbatim to the caller. diff --git a/packages/mcp/test/unit/comment-cursor-stability.test.mjs b/packages/mcp/test/unit/comment-cursor-stability.test.mjs new file mode 100644 index 00000000..e494d131 --- /dev/null +++ b/packages/mcp/test/unit/comment-cursor-stability.test.mjs @@ -0,0 +1,75 @@ +import { test } from "node:test"; +import assert from "node:assert/strict"; +import * as Y from "yjs"; +import { applyDocToFragment } from "../../build/lib/collaboration.js"; + +// Regression for issue #152: agent writes (comment anchoring especially) must +// NOT yank the open editor's cursor to the end of the document. The cursor is a +// Yjs RelativePosition anchored to node ids; the old write-back deleted the whole +// fragment and rebuilt it, destroying every id, so the position no longer +// resolved. `applyDocToFragment` uses `updateYFragment` (the editor's own diff), +// which keeps unchanged nodes' ids — so a RelativePosition still resolves. + +const para = (text, marks) => ({ + type: "paragraph", + content: [{ type: "text", text, ...(marks ? { marks } : {}) }], +}); +const doc = (...paras) => ({ type: "doc", content: paras }); + +/** The XmlText of the Nth paragraph in the live fragment. */ +function paragraphText(ydoc, n) { + const el = ydoc.getXmlFragment("default").get(n); // XmlElement + return el.get(0); // its XmlText child +} + +test("an UNCHANGED node keeps its Yjs identity across an edit (cursor survives)", () => { + const ydoc = new Y.Doc(); + applyDocToFragment(ydoc, doc(para("Hello world"), para("Second"))); + + // Anchor a cursor at offset 5 inside the FIRST (soon-to-be-unchanged) paragraph. + const relPos = Y.createRelativePositionFromTypeIndex(paragraphText(ydoc, 0), 5); + + // Edit only the SECOND paragraph; the first is untouched. + applyDocToFragment(ydoc, doc(para("Hello world"), para("Second edited"))); + + const abs = Y.createAbsolutePositionFromRelativePosition(relPos, ydoc); + assert.notEqual(abs, null, "the cursor's relative position must still resolve"); + assert.equal(abs.index, 5, "the cursor must stay at the same offset"); + // And the edit actually landed. + assert.equal(paragraphText(ydoc, 1).toString(), "Second edited"); +}); + +test("anchoring a comment mark keeps the cursor in the marked text (issue #152)", () => { + const ydoc = new Y.Doc(); + applyDocToFragment(ydoc, doc(para("Hello world"))); + + // The user's cursor sits inside the text that is about to be commented. + const relPos = Y.createRelativePositionFromTypeIndex(paragraphText(ydoc, 0), 3); + + // Agent anchors a comment over "Hello" — text is identical, only a mark added. + applyDocToFragment( + ydoc, + doc({ + type: "paragraph", + content: [ + { + type: "text", + text: "Hello", + marks: [ + { type: "comment", attrs: { commentId: "c1", resolved: false } }, + ], + }, + { type: "text", text: " world" }, + ], + }), + ); + + // The text is intact (the mark splits "Hello" / " world" but reads the same). + const para0 = ydoc.getXmlFragment("default").get(0); + assert.equal(para0.toString().replace(/<[^>]*>/g, ""), "Hello world"); + + // ...and the cursor anchored before the write still resolves (did not jump to + // the document end as it did with the destructive full-replace). + const abs = Y.createAbsolutePositionFromRelativePosition(relPos, ydoc); + assert.notEqual(abs, null, "comment anchoring must not destroy the cursor anchor"); +}); diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index d6af709c..4a55e7a0 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -946,6 +946,9 @@ importers: ws: specifier: 8.20.1 version: 8.20.1 + y-prosemirror: + specifier: 1.3.7 + version: 1.3.7(prosemirror-model@1.25.1)(prosemirror-state@1.4.3)(prosemirror-view@1.40.0)(y-protocols@1.0.6(yjs@13.6.30(patch_hash=1ceeb66dba1f86545c98a3ff7f5152aff9b35caf409091cef9caedb5e65c8810)))(yjs@13.6.30(patch_hash=1ceeb66dba1f86545c98a3ff7f5152aff9b35caf409091cef9caedb5e65c8810)) yjs: specifier: ^13.6.29 version: 13.6.30(patch_hash=1ceeb66dba1f86545c98a3ff7f5152aff9b35caf409091cef9caedb5e65c8810) -- 2.49.1 From c7c0c28e381cf035d145cfa3432ec691532f2dbc Mon Sep 17 00:00:00 2001 From: claude code agent 227 Date: Wed, 24 Jun 2026 12:56:23 +0300 Subject: [PATCH 2/3] refactor(mcp): single docmostSchema + shared encode-error helper + catch test (#152 review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review of #154 (Request changes) — all clean follow-ups, no defect in the fix: 1. Single source of the ProseMirror schema: export `docmostSchema` from docmost-schema.ts (next to docmostExtensions); diff.ts and collaboration.ts import it instead of each calling getSchema(docmostExtensions) — the schema can no longer drift between call sites. Removed both local builds + the now unused getSchema imports. 2. Doc fix: assertYjsEncodable's docstring and the client.ts comment no longer claim "the same encoder as apply" — apply uses updateYFragment, the dry-run uses toYdoc; both reject the same unstorable attrs but are NOT byte-identical. Reworded to "independent encodability gate". 3+4+5. Extracted `unstorableYjsError(safe, label, e)` — buildYDoc and applyDocToFragment now share one message template (label kept for diagnostics: toYdoc vs updateYFragment), so the wording can't drift between dry-run/apply. 6. Test for applyDocToFragment's catch branch: an unknown node type makes the schema-validated PMNode.fromJSON throw, and the function must re-throw it wrapped with the (updateYFragment) diagnostic. build/ rebuilt for the three changed lib modules; 293 package tests green. (Left build/client.js untouched: rebuilding it would pull in a pre-existing, unrelated src/build drift — a listSidebarPages slugId fix never rebuilt on develop — and my client.ts change there is comment-only.) Co-Authored-By: Claude Opus 4.8 --- packages/mcp/build/lib/collaboration.js | 36 ++++++++++------ packages/mcp/build/lib/diff.js | 9 ++-- packages/mcp/build/lib/docmost-schema.js | 9 +++- packages/mcp/src/client.ts | 6 +-- packages/mcp/src/lib/collaboration.ts | 42 +++++++++++-------- packages/mcp/src/lib/diff.ts | 9 ++-- packages/mcp/src/lib/docmost-schema.ts | 10 ++++- .../unit/comment-cursor-stability.test.mjs | 18 ++++++++ 8 files changed, 92 insertions(+), 47 deletions(-) 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\)/, + ); +}); -- 2.49.1 From aca075108cc25243c3faad5d26e0e0426884d11d Mon Sep 17 00:00:00 2001 From: claude code agent 227 Date: Wed, 24 Jun 2026 14:48:59 +0300 Subject: [PATCH 3/3] refactor(mcp): accurate encode-failure labels + diff edge-case tests (#154 review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the approve-with-comments review on PR #154: - applyDocToFragment: hydrate PMNode.fromJSON in its OWN try so a hydration failure (e.g. an unknown node type) is labelled "fromJSON" — the stage that actually threw — instead of the misleading "updateYFragment". The diagnostic comment on unstorableYjsError ("label names the stage that failed") is now truthful. - assertYjsEncodable: also rehearse PMNode.fromJSON(docmostSchema, …) so a doc that would only fail in apply's hydration step is rejected at preview time too, narrowing the preview/apply gap (review suggestion B). Still cheap — no live fragment, no updateYFragment. - Tests: relabel the diagnostic test to (fromJSON); add structural-diff edge cases — neighbour deletion keeps the unchanged node's cursor anchor, doc->empty clears the fragment without throwing, top-level node-type change diffs in place — plus a preview-gate test for the new fromJSON rehearsal. 297/297 green. build/ rebuilt for the changed lib module only (build/client.js left untouched to avoid pulling in pre-existing unrelated src/build drift). Co-Authored-By: Claude Opus 4.8 (1M context) --- packages/mcp/build/lib/collaboration.js | 24 +++++- packages/mcp/src/lib/collaboration.ts | 22 +++++- .../unit/comment-cursor-stability.test.mjs | 79 ++++++++++++++++++- 3 files changed, 117 insertions(+), 8 deletions(-) diff --git a/packages/mcp/build/lib/collaboration.js b/packages/mcp/build/lib/collaboration.js index 1fc4c1d6..fc72bbf3 100644 --- a/packages/mcp/build/lib/collaboration.js +++ b/packages/mcp/build/lib/collaboration.js @@ -483,8 +483,17 @@ export function buildYDoc(doc) { export function applyDocToFragment(ydoc, newDoc) { const safe = sanitizeForYjs(newDoc); const fragment = ydoc.getXmlFragment("default"); + // Hydrate the ProseMirror node in its OWN try so a failure here (e.g. an + // unknown node type) is labelled "fromJSON" — the stage that actually threw — + // instead of being misattributed to the Yjs write stage (#154 review). + let pmNode; + try { + pmNode = PMNode.fromJSON(docmostSchema, safe); + } + catch (e) { + throw unstorableYjsError(safe, "fromJSON", e); + } try { - const pmNode = PMNode.fromJSON(docmostSchema, safe); ydoc.transact(() => { updateYFragment(ydoc, fragment, pmNode, { mapping: new Map(), @@ -504,10 +513,21 @@ export function applyDocToFragment(ydoc, newDoc) { * 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. + * encoders that nonetheless reject the same unstorable attributes. To narrow the + * preview/apply gap it ALSO rehearses the apply path's `PMNode.fromJSON` + * hydration, so a doc that would only fail there (e.g. an unknown node type) is + * rejected at preview time too (#154 review). Still cheap: no live fragment, no + * `updateYFragment`. */ export function assertYjsEncodable(doc) { buildYDoc(doc); + const safe = sanitizeForYjs(doc); + try { + PMNode.fromJSON(docmostSchema, safe); + } + catch (e) { + throw unstorableYjsError(safe, "fromJSON", e); + } } /** Time we wait for the initial handshake/sync before giving up. */ const CONNECT_TIMEOUT_MS = 25000; diff --git a/packages/mcp/src/lib/collaboration.ts b/packages/mcp/src/lib/collaboration.ts index 7d3fdc0e..efc7bf17 100644 --- a/packages/mcp/src/lib/collaboration.ts +++ b/packages/mcp/src/lib/collaboration.ts @@ -540,8 +540,16 @@ export function buildYDoc(doc: any): Y.Doc { export function applyDocToFragment(ydoc: Y.Doc, newDoc: any): void { const safe = sanitizeForYjs(newDoc); const fragment = ydoc.getXmlFragment("default"); + // Hydrate the ProseMirror node in its OWN try so a failure here (e.g. an + // unknown node type) is labelled "fromJSON" — the stage that actually threw — + // instead of being misattributed to the Yjs write stage (#154 review). + let pmNode: PMNode; + try { + pmNode = PMNode.fromJSON(docmostSchema, safe); + } catch (e) { + throw unstorableYjsError(safe, "fromJSON", e); + } try { - const pmNode = PMNode.fromJSON(docmostSchema, safe); ydoc.transact(() => { updateYFragment(ydoc, fragment, pmNode, { mapping: new Map(), @@ -561,10 +569,20 @@ export function applyDocToFragment(ydoc: Y.Doc, newDoc: any): void { * 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. + * encoders that nonetheless reject the same unstorable attributes. To narrow the + * preview/apply gap it ALSO rehearses the apply path's `PMNode.fromJSON` + * hydration, so a doc that would only fail there (e.g. an unknown node type) is + * rejected at preview time too (#154 review). Still cheap: no live fragment, no + * `updateYFragment`. */ export function assertYjsEncodable(doc: any): void { buildYDoc(doc); + const safe = sanitizeForYjs(doc); + try { + PMNode.fromJSON(docmostSchema, safe); + } catch (e) { + throw unstorableYjsError(safe, "fromJSON", e); + } } /** Time we wait for the initial handshake/sync before giving up. */ diff --git a/packages/mcp/test/unit/comment-cursor-stability.test.mjs b/packages/mcp/test/unit/comment-cursor-stability.test.mjs index 517d65e5..23614fb9 100644 --- a/packages/mcp/test/unit/comment-cursor-stability.test.mjs +++ b/packages/mcp/test/unit/comment-cursor-stability.test.mjs @@ -1,7 +1,10 @@ import { test } from "node:test"; import assert from "node:assert/strict"; import * as Y from "yjs"; -import { applyDocToFragment } from "../../build/lib/collaboration.js"; +import { + applyDocToFragment, + assertYjsEncodable, +} from "../../build/lib/collaboration.js"; // Regression for issue #152: agent writes (comment anchoring especially) must // NOT yank the open editor's cursor to the end of the document. The cursor is a @@ -79,8 +82,9 @@ test("anchoring a comment mark keeps the cursor in the marked text (issue #152)" // 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", () => { +// before it could fail). The hydration now has its OWN try, so the label is the +// accurate stage `fromJSON` (the earlier `updateYFragment` label was misleading). +test("applyDocToFragment wraps a hydration failure with the (fromJSON) diagnostic", () => { const ydoc = new Y.Doc(); const bad = { type: "doc", @@ -88,6 +92,73 @@ test("applyDocToFragment wraps an encode/build failure with the (updateYFragment }; assert.throws( () => applyDocToFragment(ydoc, bad), - /Failed to encode document to Yjs \(updateYFragment\)/, + /Failed to encode document to Yjs \(fromJSON\)/, + ); +}); + +// #154 review (suggestion 2): structural-diff edge cases the cursor-survival +// path must handle without losing the unchanged node's id or throwing. + +test("deleting a NEIGHBOUR keeps the unchanged node's cursor anchor (diff path)", () => { + const ydoc = new Y.Doc(); + applyDocToFragment(ydoc, doc(para("Keep me"), para("Delete me"))); + + // Anchor inside the first paragraph, which survives the deletion unchanged. + const relPos = Y.createRelativePositionFromTypeIndex(paragraphText(ydoc, 0), 4); + + // Remove the second paragraph entirely; the first must keep its Yjs identity. + applyDocToFragment(ydoc, doc(para("Keep me"))); + + const abs = Y.createAbsolutePositionFromRelativePosition(relPos, ydoc); + assert.notEqual(abs, null, "the surviving node's cursor anchor must still resolve"); + assert.equal(abs.index, 4, "the cursor must stay at the same offset"); + assert.equal(ydoc.getXmlFragment("default").length, 1, "neighbour was deleted"); + assert.equal(paragraphText(ydoc, 0).toString(), "Keep me"); +}); + +test("writing an EMPTY document clears the fragment without throwing", () => { + const ydoc = new Y.Doc(); + applyDocToFragment(ydoc, doc(para("Something"), para("Else"))); + assert.equal(ydoc.getXmlFragment("default").length, 2); + + assert.doesNotThrow(() => + applyDocToFragment(ydoc, { type: "doc", content: [] }), + ); + assert.equal( + ydoc.getXmlFragment("default").length, + 0, + "the fragment is emptied (doc -> empty)", + ); +}); + +test("changing a top-level node TYPE diffs in place (paragraph -> heading)", () => { + const ydoc = new Y.Doc(); + applyDocToFragment(ydoc, doc(para("Title text"), para("Body"))); + + // Replace the first paragraph with a heading carrying the same text. + applyDocToFragment( + ydoc, + doc( + { type: "heading", attrs: { level: 2 }, content: [{ type: "text", text: "Title text" }] }, + para("Body"), + ), + ); + + const first = ydoc.getXmlFragment("default").get(0); + assert.equal(first.nodeName, "heading", "the top-level node type changed"); + assert.equal(first.toString().replace(/<[^>]*>/g, ""), "Title text"); +}); + +// #154 review (suggestion B / architecture B): the dry-run gate now also +// rehearses PMNode.fromJSON, so a doc that fails ONLY in hydration (not in +// toYdoc) is rejected at preview time, with the accurate `fromJSON` label. +test("assertYjsEncodable rejects an un-hydratable doc at preview time (fromJSON gate)", () => { + const bad = { + type: "doc", + content: [{ type: "totally_unknown_node_xyz_67890" }], + }; + assert.throws( + () => assertYjsEncodable(bad), + /Failed to encode document to Yjs/, ); }); -- 2.49.1