fix: Spurious post-import edits (#12620)

server/commands/documentCollaborativeUpdater.test.ts

@@ -0,0 +1,79 @@
+import { Node } from "prosemirror-model";
+import { prosemirrorToYDoc } from "y-prosemirror";
+import { schema } from "@server/editor";
+import { buildDocument, buildUser } from "@server/test/factories";
+import documentCollaborativeUpdater from "./documentCollaborativeUpdater";
+
+describe("documentCollaborativeUpdater", () => {
+  const buildYDoc = (content: object[]) => {
+    const doc = Node.fromJSON(schema, { type: "doc", content });
+    return prosemirrorToYDoc(doc, "default");
+  };
+
+  it("persists canonical JSON without empty attrs on marks", async () => {
+    const user = await buildUser();
+    const document = await buildDocument({
+      teamId: user.teamId,
+      userId: user.id,
+    });
+
+    const ydoc = buildYDoc([
+      {
+        type: "paragraph",
+        content: [
+          {
+            type: "text",
+            text: "Deciders:",
+            marks: [{ type: "strong" }],
+          },
+        ],
+      },
+    ]);
+
+    await documentCollaborativeUpdater({
+      documentId: document.id,
+      ydoc,
+      sessionCollaboratorIds: [user.id],
+      isLastConnection: true,
+      clientVersion: null,
+    });
+
+    await document.reload();
+
+    const marks = JSON.stringify(document.content).match(/"attrs":\{\}/g);
+    expect(marks).toBeNull();
+
+    const text = document.content?.content?.[0]?.content?.[0];
+    expect(text?.marks).toEqual([{ type: "strong" }]);
+  });
+
+  it("does not persist when content is unchanged", async () => {
+    const user = await buildUser();
+    const content = [
+      {
+        type: "paragraph",
+        content: [{ type: "text", text: "Hello" }],
+      },
+    ];
+    const ydoc = buildYDoc(content);
+
+    const document = await buildDocument({
+      teamId: user.teamId,
+      userId: user.id,
+      content: Node.fromJSON(schema, { type: "doc", content }).toJSON(),
+    });
+
+    const updatedAt = document.updatedAt;
+
+    await documentCollaborativeUpdater({
+      documentId: document.id,
+      ydoc,
+      sessionCollaboratorIds: [user.id],
+      isLastConnection: true,
+      clientVersion: null,
+    });
+
+    await document.reload();
+    expect(document.updatedAt).toEqual(updatedAt);
+  });
+});

server/commands/documentCollaborativeUpdater.ts

@@ -1,8 +1,10 @@
 import isEqual from "fast-deep-equal";
 import { uniq } from "es-toolkit/compat";
+import { Node } from "prosemirror-model";
 import { yDocToProsemirrorJSON } from "y-prosemirror";
 import * as Y from "yjs";
 import type { ProsemirrorData } from "@shared/types";
+import { schema } from "@server/editor";
 import Logger from "@server/logging/Logger";
 import { Document, Event } from "@server/models";
 import { sequelize } from "@server/storage/database";
@@ -50,7 +52,14 @@ export default async function documentCollaborativeUpdater({
       });
 
     const state = Y.encodeStateAsUpdate(ydoc);
-    const content = yDocToProsemirrorJSON(ydoc, "default") as ProsemirrorData;
+
+    // Round-trip through the schema so the stored JSON is canonical. The raw
+    // y-prosemirror output includes empty `attrs: {}` on every mark, and outputs
+    // properties in a different order - resulting in spurious "edits"
+    const content = Node.fromJSON(
+      schema,
+      yDocToProsemirrorJSON(ydoc, "default")
+    ).toJSON() as ProsemirrorData;
     const isUnchanged = isEqual(document.content, content);
     const isDeleted = !!document.deletedAt;
     const lastModifiedById = isDeleted

server/models/helpers/ProsemirrorHelper.tsx

@@ -20,6 +20,7 @@ import Diff from "@shared/editor/extensions/Diff";
 import { EditorStyleHelper } from "@shared/editor/styles/EditorStyleHelper";
 import type { ExtendedChange } from "@shared/editor/lib/ChangesetHelper";
 import textBetween from "@shared/editor/lib/textBetween";
+import { withTrailingNode } from "@shared/editor/lib/trailingNode";
 import EditorContainer from "@shared/editor/components/Styles";
 import GlobalStyles from "@shared/styles/globals";
 import light from "@shared/styles/theme";
@@ -106,15 +107,16 @@ export class ProsemirrorHelper extends SharedProsemirrorHelper {
    * @returns The content as a Y.Doc.
    */
   static toYDoc(input: string | ProsemirrorData, fieldName = "default"): Y.Doc {
-    if (typeof input === "object") {
-      return prosemirrorToYDoc(
-        ProsemirrorHelper.toProsemirror(input),
-        fieldName
-      );
+    const node =
+      typeof input === "object"
+        ? ProsemirrorHelper.toProsemirror(input)
+        : parser.parse(input);
+    if (!node) {
+      return new Y.Doc();
     }
-
-    const node = parser.parse(input);
-    return node ? prosemirrorToYDoc(node, fieldName) : new Y.Doc();
+    // Normalize to the editor's trailing-node form so the document opens without
+    // the editor inserting a trailing paragraph, which would be a spurious edit.
+    return prosemirrorToYDoc(withTrailingNode(node), fieldName);
   }
 
   /**

shared/editor/extensions/TrailingNode.ts

@@ -1,6 +1,9 @@
-import type { NodeType } from "prosemirror-model";
 import { Plugin, PluginKey } from "prosemirror-state";
 import Extension from "../lib/Extension";
+import {
+  requiresTrailingNode,
+  trailingNodeNotAfter,
+} from "../lib/trailingNode";
 
 /**
  * Options for the TrailingNode extension.
@@ -20,15 +23,12 @@ export default class TrailingNode extends Extension<TrailingNodeOptions> {
   get defaultOptions(): TrailingNodeOptions {
     return {
       node: "paragraph",
-      notAfter: ["paragraph", "heading"],
+      notAfter: trailingNodeNotAfter,
     };
   }
 
   get plugins() {
     const plugin = new PluginKey(this.name);
-    const disabledNodes = Object.entries(this.editor.schema.nodes)
-      .map(([, value]) => value)
-      .filter((node: NodeType) => this.options.notAfter.includes(node.name));
 
     return [
       new Plugin({
@@ -49,38 +49,12 @@ export default class TrailingNode extends Extension<TrailingNodeOptions> {
           },
         }),
         state: {
-          init: (_, state) => {
-            const lastNode = state.tr.doc.lastChild;
-
-            // If paragraph has no text (only images/media), add trailing node
-            if (
-              lastNode?.type.name === "paragraph" &&
-              lastNode.content.size > 0 &&
-              lastNode.textContent.length === 0
-            ) {
-              return true;
-            }
-
-            return lastNode ? !disabledNodes.includes(lastNode.type) : false;
-          },
-          apply: (tr, value) => {
-            if (!tr.docChanged) {
-              return value;
-            }
-
-            const lastNode = tr.doc.lastChild;
-
-            // If paragraph has no text (only images/media), add trailing node
-            if (
-              lastNode?.type.name === "paragraph" &&
-              lastNode.content.size > 0 &&
-              lastNode.textContent.length === 0
-            ) {
-              return true;
-            }
-
-            return lastNode ? !disabledNodes.includes(lastNode.type) : false;
-          },
+          init: (_, state) =>
+            requiresTrailingNode(state.doc, this.options.notAfter),
+          apply: (tr, value) =>
+            tr.docChanged
+              ? requiresTrailingNode(tr.doc, this.options.notAfter)
+              : value,
         },
       }),
     ];

shared/editor/lib/trailingNode.test.ts

@@ -0,0 +1,77 @@
+import { Schema } from "prosemirror-model";
+import { requiresTrailingNode, withTrailingNode } from "./trailingNode";
+
+const schema = new Schema({
+  nodes: {
+    doc: { content: "block+" },
+    paragraph: { group: "block", content: "inline*" },
+    heading: { group: "block", content: "inline*" },
+    code_block: { group: "block", content: "inline*" },
+    image: { group: "inline", inline: true },
+    text: { group: "inline" },
+  },
+});
+
+const doc = (...children: object[]) =>
+  schema.nodeFromJSON({ type: "doc", content: children });
+
+const paragraph = (text?: string) => ({
+  type: "paragraph",
+  content: text ? [{ type: "text", text }] : [],
+});
+
+describe("requiresTrailingNode", () => {
+  it("is false when the document ends in a paragraph", () => {
+    expect(requiresTrailingNode(doc(paragraph("hello")))).toBe(false);
+  });
+
+  it("is false when the document ends in a heading", () => {
+    expect(
+      requiresTrailingNode(
+        doc({ type: "heading", content: [{ type: "text", text: "title" }] })
+      )
+    ).toBe(false);
+  });
+
+  it("is true when the document ends in another block type", () => {
+    expect(
+      requiresTrailingNode(
+        doc({ type: "code_block", content: [{ type: "text", text: "x" }] })
+      )
+    ).toBe(true);
+  });
+
+  it("is true when the last paragraph contains only non-text content", () => {
+    expect(
+      requiresTrailingNode(
+        doc({ type: "paragraph", content: [{ type: "image" }] })
+      )
+    ).toBe(true);
+  });
+});
+
+describe("withTrailingNode", () => {
+  it("appends a trailing paragraph when required", () => {
+    const result = withTrailingNode(
+      doc({ type: "code_block", content: [{ type: "text", text: "x" }] })
+    );
+    expect(result.childCount).toBe(2);
+    expect(result.lastChild?.type.name).toBe("paragraph");
+    expect(result.lastChild?.content.size).toBe(0);
+  });
+
+  it("is a no-op when a trailing paragraph already exists", () => {
+    const input = doc(
+      { type: "code_block", content: [{ type: "text", text: "x" }] },
+      paragraph()
+    );
+    expect(withTrailingNode(input).eq(input)).toBe(true);
+  });
+
+  it("is idempotent", () => {
+    const once = withTrailingNode(
+      doc({ type: "code_block", content: [{ type: "text", text: "x" }] })
+    );
+    expect(withTrailingNode(once).eq(once)).toBe(true);
+  });
+});

shared/editor/lib/trailingNode.ts

@@ -0,0 +1,53 @@
+import { Fragment, type Node } from "prosemirror-model";
+
+/** Node names after which a trailing paragraph is not required. */
+export const trailingNodeNotAfter = ["paragraph", "heading"];
+
+/**
+ * Determines whether the editor would insert a trailing paragraph after the
+ * document's last node. Mirrors the behavior of the TrailingNode extension so
+ * that stored content can be normalized to match the editor, avoiding a
+ * spurious edit the first time a document is opened.
+ *
+ * @param doc The document node to inspect.
+ * @param notAfter Node names after which a trailing node is not required.
+ * @returns whether a trailing paragraph is required.
+ */
+export function requiresTrailingNode(
+  doc: Node,
+  notAfter: string[] = trailingNodeNotAfter
+): boolean {
+  const lastNode = doc.lastChild;
+  if (!lastNode) {
+    return false;
+  }
+  // A paragraph holding only non-text content (eg. images) still needs a
+  // trailing node so the cursor can be placed after it.
+  if (
+    lastNode.type.name === "paragraph" &&
+    lastNode.content.size > 0 &&
+    lastNode.textContent.length === 0
+  ) {
+    return true;
+  }
+  return !notAfter.includes(lastNode.type.name);
+}
+
+/**
+ * Appends a trailing paragraph to the document if the editor would add one on
+ * load, returning the normalized document unchanged otherwise.
+ *
+ * @param doc The document node to normalize.
+ * @param notAfter Node names after which a trailing node is not required.
+ * @returns the document, with a trailing paragraph appended when required.
+ */
+export function withTrailingNode(
+  doc: Node,
+  notAfter: string[] = trailingNodeNotAfter
+): Node {
+  const paragraph = doc.type.schema.nodes.paragraph;
+  if (!paragraph || !requiresTrailingNode(doc, notAfter)) {
+    return doc;
+  }
+  return doc.copy(doc.content.append(Fragment.from(paragraph.create())));
+}