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/, ); });