fix(collab): prevent stale view when remote Y.js changes bypass sdBlockRev increment (#2099)

tupizz · web-flow · commit 0895a93bb9e7 · 2026-02-19T16:07:32.000-08:00
* fix(collab): prevent stale view when remote Y.js changes bypass sdBlockRev increment

FlowBlockCache uses sdBlockRev for O(1) cache invalidation, but the
collaboration plugin skips incrementing sdBlockRev for Y.js-origin
transactions to avoid an infinite sync loop. This created a window
where remote content changes arrive but the cache returns stale blocks
because the rev appears unchanged.

Fix: add setHasExternalChanges() to FlowBlockCache. When a Y.js-origin
transaction is detected in PresentationEditor, the cache falls back to
JSON comparison for that cycle instead of trusting the matching rev.
The flag auto-clears on commit() so normal editing remains O(1).

Also fix file upload in collab mode: use replaceFile() on the existing
editor rather than destroying and recreating SuperDoc, which caused a
race condition where the empty Y.js room state overwrote the new
document content.

* fix: preserve nodeJson on fast cache hits for external-change fallback
diff --git a/packages/layout-engine/pm-adapter/src/cache.d.ts b/packages/layout-engine/pm-adapter/src/cache.d.ts
@@ -40,9 +40,15 @@ export declare class FlowBlockCache {
    */
   begin(): void;
 
+  /**
+   * Signal that external changes (e.g. Y.js collaboration) may have modified
+   * document content without updating sdBlockRev.
+   */
+  setHasExternalChanges(value: boolean): void;
+
   /**
    * Look up cached blocks for a paragraph by its stable ID.
-   * Returns the cached entry only if the node content matches (via JSON comparison).
+   * Returns the cached entry only if the node content matches.
    *
    * @param id - Stable paragraph ID (sdBlockId or paraId)
    * @param node - Current PM node (JSON object) to compare against cached version
diff --git a/packages/layout-engine/pm-adapter/src/cache.test.ts b/packages/layout-engine/pm-adapter/src/cache.test.ts
@@ -1,7 +1,225 @@
 import { describe, it, expect } from 'vitest';
-import { shiftBlockPositions, shiftCachedBlocks } from './cache.js';
+import { FlowBlockCache, shiftBlockPositions, shiftCachedBlocks } from './cache.js';
 import type { FlowBlock, ParagraphBlock, ImageBlock, DrawingBlock, Run } from '@superdoc/contracts';
 
+describe('FlowBlockCache', () => {
+  const makeParagraphNode = (text: string, rev: number) => ({
+    type: 'paragraph',
+    attrs: { sdBlockId: 'p1', sdBlockRev: rev, paraId: null },
+    content: [{ type: 'run', content: [{ type: 'text', text }] }],
+  });
+
+  const mockBlocks: FlowBlock[] = [
+    { kind: 'paragraph', id: 'p1', runs: [{ text: 'hello', pmStart: 0, pmEnd: 5 } as Run] } as ParagraphBlock,
+  ];
+
+  it('returns MISS when no cached entry exists', () => {
+    const cache = new FlowBlockCache();
+    cache.begin();
+
+    const node = makeParagraphNode('hello', 1);
+    const result = cache.get('p1', node);
+
+    expect(result.entry).toBeNull();
+  });
+
+  it('returns HIT when sdBlockRev matches', () => {
+    const cache = new FlowBlockCache();
+    const node = makeParagraphNode('hello', 1);
+
+    // Populate cache
+    cache.begin();
+    cache.set('p1', JSON.stringify(node), 1, mockBlocks, 0);
+    cache.commit();
+
+    // Same node, same rev → HIT
+    cache.begin();
+    const result = cache.get('p1', node);
+
+    expect(result.entry).not.toBeNull();
+    expect(result.entry!.blocks).toBe(mockBlocks);
+  });
+
+  it('retains serialized node across fast-path hits so external fallback stays incremental', () => {
+    const cache = new FlowBlockCache();
+    const node = makeParagraphNode('hello', 5);
+
+    // Render 1: cache is populated with serialized JSON.
+    cache.begin();
+    cache.set('p1', JSON.stringify(node), 5, mockBlocks, 0);
+    cache.commit();
+
+    // Render 2: local-only fast path hit, caller writes lookup payload into next generation.
+    cache.begin();
+    const fastPathHit = cache.get('p1', node);
+    expect(fastPathHit.entry).not.toBeNull();
+    cache.set('p1', fastPathHit.nodeJson, fastPathHit.nodeRev, fastPathHit.entry!.blocks, 0);
+    cache.commit();
+
+    // Render 3: collaboration/external change mode requires JSON fallback.
+    // With unchanged content this should still be a HIT.
+    cache.setHasExternalChanges(true);
+    cache.begin();
+    const externalFallback = cache.get('p1', node);
+
+    expect(externalFallback.entry).not.toBeNull();
+  });
+
+  it('returns MISS when sdBlockRev differs', () => {
+    const cache = new FlowBlockCache();
+    const nodeV1 = makeParagraphNode('hello', 1);
+    const nodeV2 = makeParagraphNode('hello world', 2);
+
+    // Populate with v1
+    cache.begin();
+    cache.set('p1', JSON.stringify(nodeV1), 1, mockBlocks, 0);
+    cache.commit();
+
+    // v2 has different rev → MISS
+    cache.begin();
+    const result = cache.get('p1', nodeV2);
+
+    expect(result.entry).toBeNull();
+  });
+
+  it('returns MISS when content changes with same sdBlockRev and externalChanges flag is set', () => {
+    const cache = new FlowBlockCache();
+    const nodeOriginal = makeParagraphNode('hello', 5);
+    const nodeModifiedByYjs = makeParagraphNode('hello world from remote user', 5); // Same rev, different content!
+
+    // Populate cache with original content
+    cache.begin();
+    cache.set('p1', JSON.stringify(nodeOriginal), 5, mockBlocks, 0);
+    cache.commit();
+
+    // Y.js-origin transaction changed content but blockNodePlugin didn't increment sdBlockRev.
+    // With the externalChanges flag, the fast path falls through to JSON comparison.
+    cache.setHasExternalChanges(true);
+    cache.begin();
+    const result = cache.get('p1', nodeModifiedByYjs);
+
+    expect(result.entry).toBeNull(); // Correct: JSON comparison catches the content change
+  });
+
+  it('returns HIT when content is unchanged even with externalChanges flag', () => {
+    const cache = new FlowBlockCache();
+    const node = makeParagraphNode('hello', 5);
+
+    cache.begin();
+    cache.set('p1', JSON.stringify(node), 5, mockBlocks, 0);
+    cache.commit();
+
+    // externalChanges flag is set but content is identical — should still HIT
+    cache.setHasExternalChanges(true);
+    cache.begin();
+    const result = cache.get('p1', node);
+
+    expect(result.entry).not.toBeNull(); // JSON comparison confirms content is same
+  });
+
+  it('without externalChanges flag, same sdBlockRev trusts fast path (HIT)', () => {
+    const cache = new FlowBlockCache();
+    const nodeOriginal = makeParagraphNode('hello', 5);
+    const nodeModified = makeParagraphNode('hello world', 5); // Same rev, different content
+
+    cache.begin();
+    cache.set('p1', JSON.stringify(nodeOriginal), 5, mockBlocks, 0);
+    cache.commit();
+
+    // Without the flag, fast path trusts sdBlockRev → HIT (this is the performance path)
+    cache.begin();
+    const result = cache.get('p1', nodeModified);
+
+    expect(result.entry).not.toBeNull(); // Fast path HIT — correct for local-only edits
+  });
+
+  it('commit() clears externalChanges flag', () => {
+    const cache = new FlowBlockCache();
+    const nodeOriginal = makeParagraphNode('hello', 5);
+    const nodeModified = makeParagraphNode('hello world', 5);
+
+    cache.begin();
+    cache.set('p1', JSON.stringify(nodeOriginal), 5, mockBlocks, 0);
+    cache.commit();
+
+    // Set flag and commit (which should clear it)
+    cache.setHasExternalChanges(true);
+    cache.begin();
+    cache.set('p1', JSON.stringify(nodeOriginal), 5, mockBlocks, 0);
+    cache.commit(); // This clears externalChanges
+
+    // Now query with modified content — flag was cleared, so fast path applies
+    cache.begin();
+    const result = cache.get('p1', nodeModified);
+
+    expect(result.entry).not.toBeNull(); // Fast path HIT — flag was cleared by commit
+  });
+
+  it('returns MISS via JSON fallback when sdBlockRev is unavailable', () => {
+    const cache = new FlowBlockCache();
+    const nodeNoRev = { type: 'paragraph', attrs: { sdBlockId: 'p1' }, content: [{ type: 'text', text: 'hello' }] };
+    const nodeNoRevModified = {
+      type: 'paragraph',
+      attrs: { sdBlockId: 'p1' },
+      content: [{ type: 'text', text: 'hello world' }],
+    };
+
+    // Populate without rev
+    cache.begin();
+    cache.set('p1', JSON.stringify(nodeNoRev), null, mockBlocks, 0);
+    cache.commit();
+
+    // Different content, no rev → falls to JSON comparison → MISS
+    cache.begin();
+    const result = cache.get('p1', nodeNoRevModified);
+
+    expect(result.entry).toBeNull(); // Correct: JSON comparison catches the change
+  });
+
+  it('clear() resets all cache state', () => {
+    const cache = new FlowBlockCache();
+    const node = makeParagraphNode('hello', 1);
+
+    cache.begin();
+    cache.set('p1', JSON.stringify(node), 1, mockBlocks, 0);
+    cache.commit();
+
+    cache.clear();
+
+    cache.begin();
+    const result = cache.get('p1', node);
+
+    expect(result.entry).toBeNull(); // Cache was cleared
+  });
+
+  it('commit() discards entries not seen in current render', () => {
+    const cache = new FlowBlockCache();
+    const nodeA = makeParagraphNode('hello', 1);
+    const nodeB = {
+      ...makeParagraphNode('world', 1),
+      attrs: { ...makeParagraphNode('world', 1).attrs, sdBlockId: 'p2' },
+    };
+
+    // Render 1: both paragraphs
+    cache.begin();
+    cache.set('p1', JSON.stringify(nodeA), 1, mockBlocks, 0);
+    cache.set('p2', JSON.stringify(nodeB), 1, mockBlocks, 10);
+    cache.commit();
+
+    // Render 2: only p1 (p2 was deleted)
+    cache.begin();
+    cache.get('p1', nodeA); // access p1
+    cache.set('p1', JSON.stringify(nodeA), 1, mockBlocks, 0);
+    cache.commit();
+
+    // Render 3: p2 should be gone
+    cache.begin();
+    const result = cache.get('p2', nodeB);
+
+    expect(result.entry).toBeNull(); // p2 was pruned
+  });
+});
+
 describe('shiftBlockPositions', () => {
   describe('paragraph blocks', () => {
     it('shifts pmStart and pmEnd in runs', () => {
diff --git a/packages/layout-engine/pm-adapter/src/cache.ts b/packages/layout-engine/pm-adapter/src/cache.ts
@@ -65,6 +65,7 @@ export class FlowBlockCache {
   #next = new Map<string, CachedParagraphEntry>();
   #hits = 0;
   #misses = 0;
+  #hasExternalChanges = false;
 
   /**
    * Begin a new render cycle. Clears the "next" map and resets stats.
@@ -75,9 +76,30 @@ export class FlowBlockCache {
     this.#misses = 0;
   }
 
+  /**
+   * Signal that external changes (e.g. Y.js collaboration) may have modified
+   * document content without updating sdBlockRev. When set, the fast revision
+   * comparison falls through to a JSON equality check to prevent false cache hits.
+   *
+   * The flag is automatically cleared after {@link commit}.
+   */
+  setHasExternalChanges(value: boolean): void {
+    this.#hasExternalChanges = value;
+  }
+
   /**
    * Look up cached blocks for a paragraph by its stable ID.
-   * Returns the cached entry only if the node content matches (via JSON comparison).
+   * Returns the cached entry only if the node content matches.
+   *
+   * Uses a dual comparison strategy:
+   * 1. Fast path: compare sdBlockRev numbers (O(1)). A different rev is a
+   *    definitive miss. A matching rev is a hit **only** when we trust that
+   *    sdBlockRev is always incremented for content changes (i.e. no external
+   *    changes pending).
+   * 2. JSON path: full node serialization + string comparison. Used as a
+   *    safety net when external changes may have bypassed the revision counter
+   *    (e.g. Y.js-origin collaboration transactions) or when revision info is
+   *    unavailable.
    *
    * Always returns the serialized nodeJson to avoid double serialization -
    * pass this to set() instead of the node object.
@@ -92,24 +114,28 @@ export class FlowBlockCache {
     const cached = this.#previous.get(id);
     if (!cached) {
       this.#misses++;
-      if (nodeRev != null) {
-        return { entry: null, nodeRev };
-      }
-      // Serialize once - this is reused in set() to avoid double serialization
       const nodeJson = JSON.stringify(node);
-      return { entry: null, nodeJson };
+      return { entry: null, nodeJson, nodeRev };
     }
 
     if (nodeRev != null && cached.nodeRev != null) {
+      // Fast rejection: different revision is always a miss
       if (cached.nodeRev !== nodeRev) {
         this.#misses++;
         return { entry: null, nodeRev };
       }
-      this.#hits++;
-      return { entry: cached, nodeRev };
+
+      // Fast acceptance: safe only when all changes go through blockNodePlugin
+      // (which always increments sdBlockRev for local edits). When external
+      // changes are pending (e.g. Y.js collaboration), sdBlockRev may not have
+      // been updated despite content changes — fall through to JSON comparison.
+      if (!this.#hasExternalChanges) {
+        this.#hits++;
+        return { entry: cached, nodeRev, nodeJson: cached.nodeJson };
+      }
     }
 
-    // Fallback to JSON comparison when revision is unavailable
+    // JSON comparison: always correct, handles external changes and missing revisions
     const nodeJson = JSON.stringify(node);
     if (cached.nodeJson !== nodeJson) {
       this.#misses++;
@@ -141,10 +167,12 @@ export class FlowBlockCache {
   /**
    * Commit the current render cycle.
    * Swaps "next" to "previous", so only blocks seen in this render are retained.
+   * Clears the external-changes flag since the render cycle consumed it.
    */
   commit(): void {
     this.#previous = this.#next;
     this.#next = new Map();
+    this.#hasExternalChanges = false;
   }
 
   /**
diff --git a/packages/super-editor/src/core/presentation-editor/PresentationEditor.ts b/packages/super-editor/src/core/presentation-editor/PresentationEditor.ts
@@ -2342,6 +2342,15 @@ export class PresentationEditor extends EventEmitter {
       if (transaction) {
         this.#epochMapper.recordTransaction(transaction);
         this.#selectionSync.setDocEpoch(this.#epochMapper.getCurrentEpoch());
+
+        // Detect Y.js-origin transactions (remote collaboration changes).
+        // These bypass the blockNodePlugin's sdBlockRev increment to prevent
+        // feedback loops, so the FlowBlockCache's fast revision comparison
+        // cannot be trusted — signal it to fall through to JSON comparison.
+        const ySyncMeta = transaction.getMeta?.(ySyncPluginKey);
+        if (ySyncMeta?.isChangeOrigin && transaction.docChanged) {
+          this.#flowBlockCache?.setHasExternalChanges(true);
+        }
       }
       if (trackedChangesChanged || transaction?.docChanged) {
         this.#pendingDocChange = true;
diff --git a/packages/superdoc/src/dev/components/SuperdocDev.vue b/packages/superdoc/src/dev/components/SuperdocDev.vue
@@ -214,9 +214,20 @@ const handleNewFile = async (file) => {
     currentFile.value = await getFileObject(url, file.name, file.type);
   }
 
-  nextTick(() => {
-    init();
-  });
+  // In collab mode, use replaceFile() on the existing editor instead of
+  // destroying and recreating SuperDoc. This avoids the Y.js race condition
+  // where empty room state overwrites the DOCX content during reinit.
+  if (useCollaboration && activeEditor.value && !isMarkdown && !isHtml) {
+    try {
+      await activeEditor.value.replaceFile(currentFile.value);
+      console.log('[collab] Replaced file via editor.replaceFile()');
+    } catch (err) {
+      console.error('[collab] replaceFile failed, falling back to full reinit:', err);
+      nextTick(() => init());
+    }
+  } else {
+    nextTick(() => init());
+  }
 
   sidebarInstanceKey.value += 1;
 };
@@ -713,7 +724,7 @@ onMounted(async () => {
     const ydoc = new Y.Doc();
     const provider = new HocuspocusProvider({
       url: 'ws://localhost:3050',
-      name: 'superdoc-dev-room',
+      name: urlParams.get('room') || 'superdoc-dev-room',
       document: ydoc,
     });
 
