fix(collaboration): move content validation from beforeTransaction to filterTransaction

.changeset/brown-parents-marry.md

@@ -0,0 +1,5 @@
+---
+"@tiptap/extension-collaboration": patch
+---
+
+Moved content validation from Yjs `beforeTransaction` (whose return value was ignored) to ProseMirror `filterTransaction`, so invalid collaborative changes are now properly blocked.

packages/extension-collaboration/__tests__/filterInvalidContent.spec.ts

@@ -0,0 +1,155 @@
+import { Editor } from '@tiptap/core'
+import Document from '@tiptap/extension-document'
+import Paragraph from '@tiptap/extension-paragraph'
+import Text from '@tiptap/extension-text'
+import type { Plugin } from '@tiptap/pm/state'
+import { ySyncPluginKey } from '@tiptap/y-tiptap'
+import { afterEach, describe, expect, it, vi } from 'vitest'
+import * as Y from 'yjs'
+
+import Collaboration from '../src/index.js'
+
+describe('filterInvalidContent', () => {
+  let editor: Editor | null = null
+  let el: HTMLElement | null = null
+
+  afterEach(() => {
+    editor?.destroy()
+    el?.remove()
+    editor = null
+    el = null
+  })
+
+  const createCollabEditor = async (
+    opts: {
+      onContentError?: (args: { disableCollaboration: () => void }) => void
+    } = {},
+  ) => {
+    const ydoc = new Y.Doc()
+
+    el = document.createElement('div')
+    document.body.appendChild(el)
+
+    editor = new Editor({
+      element: el,
+      extensions: [Document, Paragraph, Text, Collaboration.configure({ document: ydoc })],
+      enableContentCheck: true,
+      onContentError: opts.onContentError ?? (() => {}),
+    })
+
+    await new Promise<void>(resolve => {
+      setTimeout(resolve, 10)
+    })
+
+    return { editor, el, ydoc }
+  }
+
+  const findFilterPlugin = (e: Editor): Plugin | undefined =>
+    e.state.plugins.find(p => p.spec.filterTransaction && p.key.includes('filterInvalidContent'))
+
+  it('rejects Yjs transactions that produce invalid content', async () => {
+    let contentErrorCalled = false
+
+    const { editor: ed } = await createCollabEditor({
+      onContentError: () => {
+        contentErrorCalled = true
+      },
+    })
+
+    const plugin = findFilterPlugin(ed)
+
+    expect(plugin).toBeDefined()
+
+    const tr = ed.state.tr.insertText('x', 1)
+
+    tr.setMeta(ySyncPluginKey, { binding: true })
+
+    vi.spyOn(tr.doc, 'check').mockImplementation(() => {
+      throw new RangeError('Invalid content for node doc')
+    })
+
+    const result = plugin!.spec.filterTransaction!(tr, ed.state)
+
+    expect(result).toBe(false)
+    expect(contentErrorCalled).toBe(true)
+    expect(ed.storage.collaboration.isDisabled).toBe(true)
+  })
+
+  it('allows local (non-Yjs) transactions through', async () => {
+    const { editor: ed } = await createCollabEditor({
+      onContentError: () => {
+        throw new Error('contentError should not fire for local transactions')
+      },
+    })
+
+    ed.commands.insertContent('hello')
+    expect(ed.getText()).toContain('hello')
+  })
+
+  it('blocks Yjs transactions when isDisabled is true', async () => {
+    const { editor: ed } = await createCollabEditor()
+
+    ed.storage.collaboration.isDisabled = true
+
+    const docBefore = ed.state.doc.toJSON()
+
+    const tr = ed.state.tr.insertText('injected', 1)
+
+    tr.setMeta(ySyncPluginKey, { binding: true })
+    ed.view.dispatch(tr)
+
+    expect(ed.state.doc.toJSON()).toEqual(docBefore)
+  })
+
+  it('allows Yjs transactions that do not change the doc', async () => {
+    const { editor: ed } = await createCollabEditor({
+      onContentError: () => {
+        throw new Error('contentError should not fire for metadata-only transactions')
+      },
+    })
+
+    const plugin = findFilterPlugin(ed)
+
+    expect(plugin).toBeDefined()
+
+    const tr = ed.state.tr
+
+    tr.setMeta(ySyncPluginKey, { binding: true })
+
+    expect(tr.docChanged).toBe(false)
+
+    const result = plugin!.spec.filterTransaction!(tr, ed.state)
+
+    expect(result).toBe(true)
+  })
+
+  it('emits contentError with disableCollaboration callback', async () => {
+    let disableCollab: (() => void) | null = null
+
+    const { editor: ed, ydoc } = await createCollabEditor({
+      onContentError: args => {
+        disableCollab = args.disableCollaboration
+      },
+    })
+
+    const plugin = findFilterPlugin(ed)
+
+    expect(plugin).toBeDefined()
+
+    const tr = ed.state.tr.insertText('x', 1)
+
+    tr.setMeta(ySyncPluginKey, { binding: true })
+    vi.spyOn(tr.doc, 'check').mockImplementation(() => {
+      throw new RangeError('Invalid content')
+    })
+
+    plugin!.spec.filterTransaction!(tr, ed.state)
+
+    expect(disableCollab).not.toBeNull()
+
+    const destroySpy = vi.spyOn(ydoc, 'destroy')
+
+    disableCollab!()
+    expect(destroySpy).toHaveBeenCalled()
+  })
+})

packages/extension-collaboration/src/collaboration.ts

@@ -1,10 +1,11 @@
 import { Extension } from '@tiptap/core'
 import { Plugin, PluginKey } from '@tiptap/pm/state'
 import type { EditorView } from '@tiptap/pm/view'
-import { redo, undo, ySyncPlugin, yUndoPlugin, yUndoPluginKey, yXmlFragmentToProsemirrorJSON } from '@tiptap/y-tiptap'
+import { redo, undo, ySyncPlugin, yUndoPlugin, yUndoPluginKey } from '@tiptap/y-tiptap'
 import type { Doc, UndoManager, XmlFragment } from 'yjs'
 
 import { createMappablePosition, getUpdatedPosition } from './helpers/CollaborationMappablePosition.js'
+import { isChangeOrigin } from './helpers/isChangeOrigin.js'
 
 type YSyncOpts = Parameters<typeof ySyncPlugin>[1]
 type YUndoOpts = Parameters<typeof yUndoPlugin>[0]
@@ -217,49 +218,37 @@ export const Collaboration = Extension.create<CollaborationOptions, Collaboratio
 
     const ySyncPluginInstance = ySyncPlugin(fragment, ySyncPluginOptions)
 
-    if (this.editor.options.enableContentCheck) {
-      fragment.doc?.on('beforeTransaction', () => {
-        try {
-          const jsonContent = yXmlFragmentToProsemirrorJSON(fragment)
-
-          if (jsonContent.content.length === 0) {
-            return
-          }
-
-          this.editor.schema.nodeFromJSON(jsonContent).check()
-        } catch (error) {
-          this.editor.emit('contentError', {
-            error: error as Error,
-            editor: this.editor,
-            disableCollaboration: () => {
-              fragment.doc?.destroy()
-              this.storage.isDisabled = true
-            },
-          })
-          // If the content is invalid, return false to prevent the transaction from being applied
-          return false
-        }
-      })
-    }
-
     return [
       ySyncPluginInstance,
       yUndoPluginInstance,
       // Only add the filterInvalidContent plugin if content checking is enabled
       this.editor.options.enableContentCheck &&
         new Plugin({
           key: new PluginKey('filterInvalidContent'),
-          filterTransaction: () => {
-            // When collaboration is disabled, prevent any sync transactions from being applied
-            if (this.storage.isDisabled !== false) {
-              // Destroy the Yjs document to prevent any further sync transactions
-              fragment.doc?.destroy()
-
+          filterTransaction: transaction => {
+            if (!isChangeOrigin(transaction)) {
               return true
             }
-            // TODO should we be returning false when the transaction is a collaboration transaction?
-
-            return true
+            if (this.storage.isDisabled) {
+              return false
+            }
+            if (!transaction.docChanged) {
+              return true
+            }
+            try {
+              transaction.doc.check()
+              return true
+            } catch (error) {
+              this.storage.isDisabled = true
+              this.editor.emit('contentError', {
+                error: error as Error,
+                editor: this.editor,
+                disableCollaboration: () => {
+                  fragment.doc?.destroy()
+                },
+              })
+              return false
+            }
           },
         }),
     ].filter(Boolean)