From 053693b9d55cb1363947b2fa4c825b6a4c2eb3b7 Mon Sep 17 00:00:00 2001 From: Tom Moor Date: Sun, 7 Jun 2026 21:13:23 -0400 Subject: [PATCH] fix: Spurious post-import edits (#12620) --- .../documentCollaborativeUpdater.test.ts | 79 +++++++++++++++++++ .../commands/documentCollaborativeUpdater.ts | 11 ++- server/models/helpers/ProsemirrorHelper.tsx | 18 +++-- shared/editor/extensions/TrailingNode.ts | 48 +++-------- shared/editor/lib/trailingNode.test.ts | 77 ++++++++++++++++++ shared/editor/lib/trailingNode.ts | 53 +++++++++++++ 6 files changed, 240 insertions(+), 46 deletions(-) create mode 100644 server/commands/documentCollaborativeUpdater.test.ts create mode 100644 shared/editor/lib/trailingNode.test.ts create mode 100644 shared/editor/lib/trailingNode.ts diff --git a/server/commands/documentCollaborativeUpdater.test.ts b/server/commands/documentCollaborativeUpdater.test.ts new file mode 100644 index 0000000000..26377526af --- /dev/null +++ b/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); + }); +}); diff --git a/server/commands/documentCollaborativeUpdater.ts b/server/commands/documentCollaborativeUpdater.ts index 3f93c644f6..88b3b8334b 100644 --- a/server/commands/documentCollaborativeUpdater.ts +++ b/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 diff --git a/server/models/helpers/ProsemirrorHelper.tsx b/server/models/helpers/ProsemirrorHelper.tsx index 8d4043aaef..250c958ab6 100644 --- a/server/models/helpers/ProsemirrorHelper.tsx +++ b/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); } /** diff --git a/shared/editor/extensions/TrailingNode.ts b/shared/editor/extensions/TrailingNode.ts index 45d56ec58b..19495eb8a0 100644 --- a/shared/editor/extensions/TrailingNode.ts +++ b/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 { 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 { }, }), 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, }, }), ]; diff --git a/shared/editor/lib/trailingNode.test.ts b/shared/editor/lib/trailingNode.test.ts new file mode 100644 index 0000000000..6a2413aa3f --- /dev/null +++ b/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); + }); +}); diff --git a/shared/editor/lib/trailingNode.ts b/shared/editor/lib/trailingNode.ts new file mode 100644 index 0000000000..34113dc227 --- /dev/null +++ b/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()))); +}