vvzvlad/gitmost
1
0
Fork 0
Code Issues 14 Pull Requests 12 Packages Projects Releases Activity

refactor(mcp): single docmostSchema + shared encode-error helper + catch test (#152 review)

Browse Source 
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 <noreply@anthropic.com>
This commit is contained in:
claude code agent 227
2026-06-24 12:56:23 +03:00
parent f86b8b69a0
commit c7c0c28e38
8 changed files with 92 additions and 47 deletions
Download Patch File Download Diff File Expand all files Collapse all files

36
packages/mcp/build/lib/collaboration.js
View File

@@ -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("<!DOCTYPE html><html><body></body></html>");
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);

9
packages/mcp/build/lib/diff.js
View File

@@ -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,

9
packages/mcp/build/lib/docmost-schema.js
View File

@@ -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);

6
packages/mcp/src/client.ts
View File

@@ -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,

42
packages/mcp/src/lib/collaboration.ts
View File

@@ -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);

9
packages/mcp/src/lib/diff.ts
View File

@@ -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<string>();
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,

10
packages/mcp/src/lib/docmost-schema.ts
View File

@@ -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);

18
packages/mcp/test/unit/comment-cursor-stability.test.mjs
View File

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