refactor(mcp): accurate encode-failure labels + diff edge-case tests (#154 review)
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) <noreply@anthropic.com>
This commit is contained in:
@@ -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/,
|
||||
);
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user