BitPhinix / slate-yjs

Yjs binding for Slate
https://docs.slate-yjs.dev
MIT License
514 stars 73 forks source link

applyRemoteEvents breaks on text and inline void combination #390

Open ezeidman opened 1 year ago

ezeidman commented 1 year ago

Hitting an issue when the same YEvent adds both text and an inline void node at the same time. The applyToSlate logic decides to convert the new text to an insert_text operation but that operation is invalid because the inline void gets added first with an insert_node operation.

The insert_text operation fails at:

https://github.com/ianstormtaylor/slate/blob/33a1e9b9239726cdbc1d65a664d88ca3d1c8a41e/packages/slate/src/interfaces/node.ts#L468

because the insertion point is no longer text after inserting the inline void node.

Full stacktrace:

      at Object.leaf (../../node_modules/.pnpm/slate@0.72.8/node_modules/slate/src/interfaces/node.ts:430:13)
      at leaf (../../node_modules/.pnpm/slate@0.72.8/node_modules/slate/src/transforms/general.ts:48:25)
      at Object.applyToDraft [as transform] (../../node_modules/.pnpm/slate@0.72.8/node_modules/slate/src/transforms/general.ts:323:19)
      at transform (../../node_modules/.pnpm/slate@0.72.8/node_modules/slate/src/create-editor.ts:80:18)
      at e.apply (../../node_modules/.pnpm/@slate-yjs+core@0.3.1_slate@0.72.8+yjs@13.5.45/node_modules/@slate-yjs/core/src/plugins/withYjs.ts:272:5)
      at apply (../../node_modules/.pnpm/slate-react@0.72.9_hjlnxowvvig46phsm4rwljrf2i/node_modules/slate-react/src/plugin/with-react.ts:103:5)
      at Object.apply (src/slate/withElementIds.ts:13:7)
      at ../../node_modules/.pnpm/@slate-yjs+core@0.3.1_slate@0.72.8+yjs@13.5.45/node_modules/@slate-yjs/core/src/applyToSlate/index.ts:40:16
          at Array.forEach (<anonymous>)
      at ../../node_modules/.pnpm/@slate-yjs+core@0.3.1_slate@0.72.8+yjs@13.5.45/node_modules/@slate-yjs/core/src/applyToSlate/index.ts:39:52
          at Array.forEach (<anonymous>)
      at ../../node_modules/.pnpm/@slate-yjs+core@0.3.1_slate@0.72.8+yjs@13.5.45/node_modules/@slate-yjs/core/src/applyToSlate/index.ts:38:12
      at Object.fn [as withoutNormalizing] (../../node_modules/.pnpm/slate@0.72.8/node_modules/slate/src/interfaces/editor.ts:1709:7)
      at applyYjsEvents (../../node_modules/.pnpm/@slate-yjs+core@0.3.1_slate@0.72.8+yjs@13.5.45/node_modules/@slate-yjs/core/src/applyToSlate/index.ts:37:10)
      at ../../node_modules/.pnpm/@slate-yjs+core@0.3.1_slate@0.72.8+yjs@13.5.45/node_modules/@slate-yjs/core/src/plugins/withYjs.ts:178:9
      at Object.withOrigin (../../node_modules/.pnpm/@slate-yjs+core@0.3.1_slate@0.72.8+yjs@13.5.45/node_modules/@slate-yjs/core/src/plugins/withYjs.ts:106:5)
      at ../../node_modules/.pnpm/@slate-yjs+core@0.3.1_slate@0.72.8+yjs@13.5.45/node_modules/@slate-yjs/core/src/plugins/withYjs.ts:177:17
      at Object.fn [as withoutNormalizing] (../../node_modules/.pnpm/slate@0.72.8/node_modules/slate/src/interfaces/editor.ts:1709:7)
      at Object.e.applyRemoteEvents (../../node_modules/.pnpm/@slate-yjs+core@0.3.1_slate@0.72.8+yjs@13.5.45/node_modules/@slate-yjs/core/src/plugins/withYjs.ts:176:12)
      at Object.applyRemoteEvents (../../node_modules/.pnpm/@slate-yjs+core@0.3.1_slate@0.72.8+yjs@13.5.45/node_modules/@slate-yjs/core/src/plugins/withYjs.ts:71:12)
      at Array.handleYEvents (../../node_modules/.pnpm/@slate-yjs+core@0.3.1_slate@0.72.8+yjs@13.5.45/node_modules/@slate-yjs/core/src/plugins/withYjs.ts:193:15)
      at Object.callAll (../../node_modules/.pnpm/lib0@0.2.63/node_modules/lib0/function.js:19:12)
      at callEventHandlerListeners (../../node_modules/.pnpm/yjs@13.5.45/node_modules/yjs/src/utils/EventHandler.js:87:5)
      at Array.<anonymous> (../../node_modules/.pnpm/yjs@13.5.45/node_modules/yjs/src/utils/Transaction.js:291:15)
      at Object.callAll (../../node_modules/.pnpm/lib0@0.2.63/node_modules/lib0/function.js:19:12)
      at callAll (../../node_modules/.pnpm/yjs@13.5.45/node_modules/yjs/src/utils/Transaction.js:297:7)
      at transact (../../node_modules/.pnpm/yjs@13.5.45/node_modules/yjs/src/utils/Transaction.js:412:9)
      at readUpdateV2 (../../node_modules/.pnpm/yjs@13.5.45/node_modules/yjs/src/utils/encoding.js:384:3)
      at applyUpdateV2 (../../node_modules/.pnpm/yjs@13.5.45/node_modules/yjs/src/utils/encoding.js:479:3)
      at Object.applyUpdate (../../node_modules/.pnpm/yjs@13.5.45/node_modules/yjs/src/utils/encoding.js:493:65)
      at src/__tests__/slate/testEditor.ts:46:9
      at Object.fn [as withoutNormalizing] (../../node_modules/.pnpm/slate@0.72.8/node_modules/slate/src/interfaces/editor.ts:1709:7)
      at src/__tests__/slate/testEditor.ts:45:12
          at Array.forEach (<anonymous>)
      at transferUpdates (src/__tests__/slate/testEditor.ts:44:11)
      at synchronize (src/__tests__/slate/heterogenousInlines.test.ts:85:18)
      at Object.<anonymous> (src/__tests__/slate/heterogenousInlines.test.ts:47:5)

Repro'd the issue in a simple test:

const paragraph: Paragraph = {
  type: "paragraph",
  elementId: "uuid-1",
  children: [textNode("original text")],
};
const userMention: UserMention = {
  type: "user-mention",
  elementId: "uuid-2",
  collaborator: {
    user: "bob",
    type: "user",
  },
  children: [{ text: "" } as any],
};

describe("tests heterogenous inlines", () => {
  it("store two inlines without flushing", () => {
    const editor1 = getNewYjsEditor("#1");
    const editor2 = getNewYjsEditorFromExisting("#2", editor1);

    editor1.apply({
      type: "insert_node",
      node: paragraph,
      path: [0],
    });
    synchronize(editor1, editor2);
    editor1.apply({
      type: "insert_node",
      node: userMention,
      path: [0, 0],
    });
    editor1.apply({
      type: "insert_node",
      node: textNode("new text"),
      path: [0, 0],
    });
    expect(() => synchronize(editor1, editor2)).toThrowError(
      "Cannot get the leaf node at path [0,0] because it refers to a non-leaf node: [object Object]",
    );
  });

  it("store two inlines with flushing", () => {
    const editor1 = getNewYjsEditor("#1");
    const editor2 = getNewYjsEditorFromExisting("#2", editor1);

    editor1.apply({
      type: "insert_node",
      node: paragraph,
      path: [0],
    });
    synchronize(editor1, editor2);
    editor1.apply({
      type: "insert_node",
      node: userMention,
      path: [0, 0],
    });
    editor1.flushLocalChanges();
    editor1.apply({
      type: "insert_node",
      node: textNode("new text"),
      path: [0, 0],
    });
    synchronize(editor1, editor2);
  });
});

function textNode(text: string): TextNode {
  return {
    type: "text",
    text,
  };
}

function synchronize(editor1: TestEditor, editor2: TestEditor) {
  editor1.flushLocalChanges();
  editor2.flushLocalChanges();
  transferUpdates(editor1, editor2);
  transferUpdates(editor2, editor1);

  checkYjsMatches(editor1, editor2);
  checkSlateMatchesYjs(editor1);
  checkSlateMatchesYjs(editor2);
}

For some reason flushing changes between edits is a workaround but on the other hand we also found that this workaround no longer works if the Yjs updates are merged together. Not being able to merge updates is possible but also requires special handling and feels antithetical to the spirit of Yjs :)

It seems like when we decide to insert_text that's based only on what's currently at the insertion point and doesn't factor in what other changes in the same delta might have inserted (around https://github.com/BitPhinix/slate-yjs/blob/main/packages/core/src/applyToSlate/textEvent.ts#L163).

ezeidman commented 1 year ago

I ran into a variant of this that results in incorrect formatting instead of an error getting thrown. It's a similar idea where we convert a delta insert into an insert_text but the insert_text is no longer the correct operation to use if something else was inserted first at the same location.

This is my repro:

const paragraph: Paragraph = {
  type: "paragraph",
  elementId: "uuid-1",
  children: [textNode("original text")],
};
function textNode(text: string): TextNode {
  return {
    type: "text",
    text,
  };
}
function boldTextNode(text: string): TextNode {
  return {
    type: "text",
    text,
    bold: true,
  };
}

  it("adds text nodes with different formatting", () => {
    const { editor1, editor2, synchronize } = setupTreeTestEnvironment();

    editor1.apply({
      type: "insert_node",
      node: paragraph,
      path: [0],
    });
    synchronize(editor1, editor2);
    editor1.apply({
      type: "insert_node",
      node: boldTextNode("new text"),
      path: [0, 0],
    });
    editor1.apply({
      type: "insert_node",
      node: textNode("new text"),
      path: [0, 0],
    });
    synchronize(editor1, editor2);
  });

In this example all of the new text shows up as bold in editor 2