Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
"react-dom": "^19.2.3",
"@y/protocols": "^1.0.6-rc.1",
"@y/y": "^14.0.0-rc.23",
"@y/prosemirror": "^2.0.0-6",
"@y/prosemirror": "^2.0.0-7",
"@y/websocket": "^4.0.0-rc.2"
},
"devDependencies": {
Expand Down
2 changes: 1 addition & 1 deletion examples/07-collaboration/13-versioning-yjs14/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
"@mantine/hooks": "^9.0.2",
"react": "^19.2.3",
"react-dom": "^19.2.3",
"@y/prosemirror": "^2.0.0-6",
"@y/prosemirror": "^2.0.0-7",
"@y/protocols": "^1.0.6-rc.1",
"@y/websocket": "^4.0.0-3",
"@y/y": "^14.0.0-rc.23",
Expand Down
28 changes: 26 additions & 2 deletions examples/07-collaboration/14-suggestion-gallery/src/App.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -466,11 +466,35 @@ function VersionMerge({
setup.attrs,
);
renderDiff();
setup.afterDoc.on("update", renderDiff);
// Defer the re-render out of the Yjs observer chain. `afterDoc`'s update
// events fire synchronously inside the *typing* editor's Y transaction
// commit, so rendering the Diff directly in the handler runs enterPreview
// within that editor's sync machinery — a failure there gets caught by
// its y-sync last-resort catch and reduced to a console warning (which
// once hid a stale-diff bug for two months), and it interrupts the
// remaining observers mid-forward. A microtask lets the CRDT forwarding
// complete untouched, coalesces update bursts into one render, and makes
// a render failure surface as a real uncaught error.
let renderQueued = false;
let disposed = false;
const scheduleRenderDiff = () => {
if (renderQueued) {
return;
}
renderQueued = true;
queueMicrotask(() => {
renderQueued = false;
if (!disposed) {
renderDiff();
}
});
};
setup.afterDoc.on("update", scheduleRenderDiff);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we really do need this (which I do question design-wise if we should), then we should at least extract this to a separate utility that is a thunk, taking in the callback to execute, and returns a function which will defer the execution of that function until the next microtask.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do think it's an issue that the error of 1 editor breaks the other (because the error is triggered in the listener).

I'd say we either need:

  • a try / catch around the handler, and log + rethrow errors there manually (and / or call reportError?)
  • the current solution (a microtask to decouple them)
  • have this handled at y-prosemirror level

Without any of these, we don't notice the error, but just get broken behavior (a stale diff editor that's not updated anymore).

fyi, The way to reproduce this issue is shown in the video at Move paragraph up in this doc

Preferred solution?


return () => {
disposed = true;
offs.forEach((off) => off());
setup.afterDoc.off("update", renderDiff);
setup.afterDoc.off("update", scheduleRenderDiff);
};
// eslint-disable-next-line react-hooks/exhaustive-deps
}, []);
Expand Down
21 changes: 15 additions & 6 deletions examples/07-collaboration/14-suggestion-gallery/src/scenarios.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,20 @@
import { addIdsToBlocks } from "@shared/formatConversionTestUtil.js";
import { testDocument } from "@shared/testDocument.js";

import type { GalleryEditor, GalleryPartialBlock } from "./gallerySchema";

// The shared test document is a snapshot fixture and deliberately carries
// empty-string block ids (real ids would be minted at module load, making the
// exporter snapshots that embed them non-deterministic). Empty ids violate the
// editor's id contract though — `getNodeId` throws on them, which crashed the
// large-diff scenarios' `replaceBlocks`/`insertBlocks` calls. Follow the
// conversion-test convention: the consumer assigns ids (on a clone, so the
// shared fixture stays untouched for other importers).
const testDocumentWithIds = structuredClone(
testDocument,
) as unknown as GalleryPartialBlock[];
addIdsToBlocks(testDocumentWithIds);

/**
* A browsable suggestion scenario.
*
Expand Down Expand Up @@ -1590,11 +1603,7 @@ export const scenarios: SuggestionScenario[] = [
"Insert every block type from the shared test document at once — a stress test for large diffs.",
initial: [{ id: "anchor", type: "paragraph", content: "Document start" }],
apply: (editor) =>
editor.insertBlocks(
testDocument as unknown as GalleryPartialBlock[],
"anchor",
"after",
),
editor.insertBlocks(testDocumentWithIds, "anchor", "after"),
feedback: [
{
severity: "high",
Expand All @@ -1609,7 +1618,7 @@ export const scenarios: SuggestionScenario[] = [
category: "Large diffs",
description:
"Remove every block of the shared test document, leaving a single paragraph — a stress test for large diffs.",
initial: testDocument as unknown as GalleryPartialBlock[],
initial: testDocumentWithIds,
apply: (editor) =>
editor.replaceBlocks(editor.document, [
{ type: "paragraph", content: "(all content removed)" },
Expand Down
2 changes: 1 addition & 1 deletion examples/08-extensions/02-versioning/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
"react": "^19.2.3",
"react-dom": "^19.2.3",
"@y/y": "^14.0.0-rc.23",
"@y/prosemirror": "^2.0.0-6"
"@y/prosemirror": "^2.0.0-7"
},
"devDependencies": {
"@types/react": "^19.2.3",
Expand Down
2 changes: 1 addition & 1 deletion packages/core/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@
"yjs": "^13.6.27"
},
"peerDependencies": {
"@y/prosemirror": "^2.0.0-6",
"@y/prosemirror": "^2.0.0-7",
"@y/protocols": "^1.0.6-rc.1",
"@y/y": "^14.0.0-rc.23",
"y-prosemirror": "^1.3.7",
Expand Down
Original file line number Diff line number Diff line change
@@ -1,10 +1,14 @@
import { Schema } from "prosemirror-model";
import { EditorState } from "prosemirror-state";
import { describe, expect, it } from "vite-plus/test";

import { setupTestEnv } from "../../setupTestEnv.js";
import { removeAndInsertBlocks } from "./replaceBlocks.js";
import { BlockNoteEditor } from "../../../../editor/BlockNoteEditor.js";
import { PartialBlock } from "../../../../blocks/defaultBlocks.js";
import { BlockIdentifier } from "../../../../schema/index.js";
import { docToBlocks } from "../../../nodeConversions/nodeToBlock.js";
import { YAttributionMarksExtension } from "../../../../y/extensions/YAttributionMarks.js";

const getEditor = setupTestEnv();

Expand Down Expand Up @@ -233,3 +237,62 @@ describe("Test replaceBlocks", () => {
expect(getEditor().document).toMatchSnapshot();
});
});

/**
* Builds a `blockContainer` holding a single paragraph with the given block
* `id`. When `suggestedDelete` is true, the container carries a
* `y-attributed-delete` mark, simulating a node that a suggestion / version
* diff keeps in the document after it has been deleted — it shares its `id`
* with the live node it was deleted from.
*/
function makeBlockContainer(
schema: Schema,
id: string,
text: string,
suggestedDelete: boolean,
) {
const paragraph = schema.nodes["paragraph"].createChecked(
{},
schema.text(text),
);
const marks = suggestedDelete
? [schema.marks["y-attributed-delete"].create({ id: 1 })]
: undefined;

return schema.nodes["blockContainer"].createChecked({ id }, paragraph, marks);
}

describe("removeAndInsertBlocks with suggested deletions", () => {
// A rendered diff (e.g. of a moved block) shows the same block `id` twice:
// the live node and a suggested-deletion copy, which `docToBlocks` reports
// under a disambiguated id ("0-1" = the second node with id "0"). Removing
// the blocks `editor.document` reports must resolve that id even though, by
// the time the walk reaches the deleted copy, the live node it was counted
// against is already gone from the transaction's doc.
it("removes a live block and its suggested deletion by the ids docToBlocks reports", () => {
const editor = BlockNoteEditor.create({
extensions: [YAttributionMarksExtension()],
});
const schema = editor.pmSchema;
const doc = schema.nodes["doc"].createChecked(
{},
schema.nodes["blockGroup"].createChecked({}, [
makeBlockContainer(schema, "0", "Live", false),
makeBlockContainer(schema, "1", "Other", false),
makeBlockContainer(schema, "0", "Deleted", true),
]),
);
const blocks = docToBlocks(doc);
expect(blocks.map((block) => block.id)).toEqual(["0", "1", "0-1"]);

const tr = EditorState.create({ doc }).tr;
const { removedBlocks } = removeAndInsertBlocks(
tr,
blocks.filter((block) => block.id !== "1"),
[],
);

expect(removedBlocks.map((block) => block.id)).toEqual(["0", "0-1"]);
expect(docToBlocks(tr.doc).map((block) => block.id)).toEqual(["1"]);
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,16 @@ export function removeAndInsertBlocks<
: blocksToRemove[0].id;
let removedSize = 0;

tr.doc.descendants((node, pos) => {
// The IDs to remove were derived from the document as it is *now* (e.g. via
// `editor.document`), so resolve them against that same document rather than
// `tr.doc`, which mutates as blocks get deleted below. This matters for
// suggested-deletion nodes: `getNodeId` disambiguates them from the live node
// sharing their `id` by their index among same-id nodes, so once the live
// node has been deleted from `tr.doc` the index — and thus the ID — would no
// longer match.
const doc = tr.doc;

doc.descendants((node, pos) => {
// Skips traversing nodes after all target blocks have been removed.
if (idsOfBlocksToRemove.size === 0) {
return false;
Expand All @@ -62,14 +71,14 @@ export function removeAndInsertBlocks<
return true;
}

const nodeId = getNodeId(node, tr.doc);
const nodeId = getNodeId(node, doc);

if (!idsOfBlocksToRemove.has(nodeId)) {
return true;
}

// Saves the block that is being deleted.
removedBlocks.push(nodeToBlock(node, tr.doc));
removedBlocks.push(nodeToBlock(node, doc));
idsOfBlocksToRemove.delete(nodeId);

if (blocksToInsert.length > 0 && nodeId === idOfFirstBlock) {
Expand Down
68 changes: 68 additions & 0 deletions packages/core/src/api/getBlocksChangedByTransaction.test.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,13 @@
import { Schema } from "prosemirror-model";
import { EditorState } from "prosemirror-state";
import { describe, expect, it, beforeEach } from "vite-plus/test";

import { setupTestEnv } from "./blockManipulation/setupTestEnv.js";
import { getBlocksChangedByTransaction } from "./getBlocksChangedByTransaction.js";
import { getBlockInfo } from "./getBlockInfoFromPos.js";
import { getNodeById } from "./nodeUtil.js";
import { BlockNoteEditor } from "../editor/BlockNoteEditor.js";
import { YAttributionMarksExtension } from "../y/extensions/YAttributionMarks.js";
import { PartialBlock } from "../blocks/defaultBlocks.js";

const getEditor = setupTestEnv();
Expand Down Expand Up @@ -574,6 +577,71 @@ describe("getBlocksChangedByTransaction", () => {
});
});

/**
* Builds a `blockContainer` holding a single paragraph with the given block
* `id`. When `suggestedDelete` is true, the container carries a
* `y-attributed-delete` mark, simulating a node that a suggestion / version
* diff keeps in the document after it has been deleted — it shares its `id`
* with the live node it was deleted from, and `getNodeId` disambiguates it
* positionally ("0-1" = the deletion-marked node with 1 same-id node before
* it).
*/
function makeBlockContainer(
schema: Schema,
id: string,
text: string,
suggestedDelete: boolean,
) {
const paragraph = schema.nodes["paragraph"].createChecked(
{},
schema.text(text),
);
const marks = suggestedDelete
? [schema.marks["y-attributed-delete"].create({ userIds: ["A"] })]
: undefined;
return schema.nodes["blockContainer"].createChecked({ id }, paragraph, marks);
}

/**
* Regression tests: change tracking on suggestion-rendered docs. Suggested-
* deletion copies duplicate a live block's id and are only disambiguated
* *positionally* (see `getNodeId`), so diffing them across the before/after
* docs used to misreport unchanged copies as delete+insert pairs — they are
* now excluded from the snapshots as rendering artifacts.
*/
describe("getBlocksChangedByTransaction on suggestion docs", () => {
it("reports only the real change when a block before a suggested deletion is deleted", () => {
const suggestionEditor = BlockNoteEditor.create({
extensions: [YAttributionMarksExtension()],
});
const schema = suggestionEditor.pmSchema;
const doc = schema.nodes["doc"].createChecked(
{},
schema.nodes["blockGroup"].createChecked({}, [
makeBlockContainer(schema, "0", "Live", false),
makeBlockContainer(schema, "1", "Other", false),
makeBlockContainer(schema, "0", "Deleted", true),
]),
);

// Delete the live "0" block. The deleted copy is untouched — but its
// positional lying id shifts from "0-1" to "0-0", which used to make the
// before/after diff misreport it as a delete of "0-1" plus an insert of
// "0-0".
const live = doc.firstChild!.child(0);
const tr = EditorState.create({ doc }).tr.delete(1, 1 + live.nodeSize);

const changes = getBlocksChangedByTransaction(tr).map((change) => [
change.type,
change.block.id,
]);

// The only change is the deletion of the live block "0" — the deleted
// copy is a rendering artifact and never appears in change events.
expect(changes).toEqual([["delete", "0"]]);
});
});

/**
* These exercise the ranged optimization: getBlocksChangedByTransaction only
* snapshots the range a transaction touched, not the whole document. In a large
Expand Down
12 changes: 11 additions & 1 deletion packages/core/src/api/getBlocksChangedByTransaction.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import type { BlockSchema } from "../schema/index.js";
import type { InlineContentSchema } from "../schema/inlineContent/types.js";
import type { StyleSchema } from "../schema/styles/types.js";
import { getChangedRange } from "./getChangedRange.js";
import { getNodeId } from "./getBlockInfoFromPos.js";
import { getNodeId, isSuggestedDeletionNode } from "./getBlockInfoFromPos.js";
import { nodeToBlock } from "./nodeConversions/nodeToBlock.js";
import { isNodeBlock } from "./nodeUtil.js";

Expand Down Expand Up @@ -185,6 +185,16 @@ function collectSnapshot<
if (!isNodeBlock(node)) {
return true;
}
// Suggested-deletion copies are rendering artifacts of suggestion /
// version-diff mode, not blocks of the document: they duplicate a live
// block's id and are only disambiguated *positionally* (see `getNodeId`),
// so diffing them across the before/after docs misreports unchanged
// copies as delete+insert pairs whenever their position shifts. Skip the
// whole subtree — everything under a deleted copy is part of the same
// artifact.
if (isSuggestedDeletionNode(node)) {
return false;
}
const parentId = getParentBlockId(doc, pos);
const key = parentId ?? ROOT_KEY;
if (!childrenByParent[key]) {
Expand Down
Loading
Loading