From aa9cb89959821d6f826b01b76ab660a7c7dc29ce Mon Sep 17 00:00:00 2001 From: Tadeu Tupinamba Date: Wed, 18 Feb 2026 13:19:54 -0300 Subject: [PATCH 1/2] 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. --- .../layout-engine/pm-adapter/src/cache.d.ts | 8 +- .../pm-adapter/src/cache.test.ts | 195 +++++++++++++++++- .../layout-engine/pm-adapter/src/cache.ts | 40 +++- .../presentation-editor/PresentationEditor.ts | 17 +- .../src/dev/components/SuperdocDev.vue | 19 +- 5 files changed, 268 insertions(+), 11 deletions(-) diff --git a/packages/layout-engine/pm-adapter/src/cache.d.ts b/packages/layout-engine/pm-adapter/src/cache.d.ts index ce88d8b301..31f86537d5 100644 --- 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 index 934af3f996..cdda044413 100644 --- a/packages/layout-engine/pm-adapter/src/cache.test.ts +++ b/packages/layout-engine/pm-adapter/src/cache.test.ts @@ -1,7 +1,200 @@ 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('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 index 664c493c36..d2c9bd1f1d 100644 --- 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(); #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. @@ -101,15 +123,23 @@ export class FlowBlockCache { } 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 }; + } } - // 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 +171,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 index 7a93018b02..8dcb0fc582 100644 --- a/packages/super-editor/src/core/presentation-editor/PresentationEditor.ts +++ b/packages/super-editor/src/core/presentation-editor/PresentationEditor.ts @@ -2270,6 +2270,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; @@ -3662,7 +3671,13 @@ export class PresentationEditor extends EventEmitter { // Keep selection visible when context menu (SlashMenu) is open const slashMenuOpen = activeEditor?.state ? !!SlashMenuPluginKey.getState(activeEditor.state)?.open : false; - if (!hasFocus && !slashMenuOpen) { + // Keep selection visible when focus is on editor UI surfaces (toolbar, dropdowns). + // Naive-UI portals dropdown content under .v-binder-follower-content at level, + // so it won't be inside [data-editor-ui-surface]. Check both. + const activeEl = document.activeElement; + const isOnEditorUi = !!(activeEl as Element)?.closest?.('[data-editor-ui-surface], .v-binder-follower-content'); + + if (!hasFocus && !slashMenuOpen && !isOnEditorUi) { try { this.#clearSelectedFieldAnnotationClass(); this.#localSelectionLayer.innerHTML = ''; diff --git a/packages/superdoc/src/dev/components/SuperdocDev.vue b/packages/superdoc/src/dev/components/SuperdocDev.vue index a6034aae7a..950e54cd3f 100644 --- 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, }); From 42eb281cb4b165e90b1849074adf5ce4251adcba Mon Sep 17 00:00:00 2001 From: Nick Bernal Date: Thu, 19 Feb 2026 15:34:16 -0800 Subject: [PATCH 2/2] fix: preserve nodeJson on fast cache hits for external-change fallback --- .../pm-adapter/src/cache.test.ts | 25 +++++++++++++++++++ .../layout-engine/pm-adapter/src/cache.ts | 8 ++---- 2 files changed, 27 insertions(+), 6 deletions(-) diff --git a/packages/layout-engine/pm-adapter/src/cache.test.ts b/packages/layout-engine/pm-adapter/src/cache.test.ts index cdda044413..d41ad9f913 100644 --- a/packages/layout-engine/pm-adapter/src/cache.test.ts +++ b/packages/layout-engine/pm-adapter/src/cache.test.ts @@ -40,6 +40,31 @@ describe('FlowBlockCache', () => { 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); diff --git a/packages/layout-engine/pm-adapter/src/cache.ts b/packages/layout-engine/pm-adapter/src/cache.ts index d2c9bd1f1d..becdc7f5c0 100644 --- a/packages/layout-engine/pm-adapter/src/cache.ts +++ b/packages/layout-engine/pm-adapter/src/cache.ts @@ -114,12 +114,8 @@ 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) { @@ -135,7 +131,7 @@ export class FlowBlockCache { // been updated despite content changes — fall through to JSON comparison. if (!this.#hasExternalChanges) { this.#hits++; - return { entry: cached, nodeRev }; + return { entry: cached, nodeRev, nodeJson: cached.nodeJson }; } }