diff --git a/.changeset/repair-confirm-transients.md b/.changeset/repair-confirm-transients.md new file mode 100644 index 000000000..c75fa2361 --- /dev/null +++ b/.changeset/repair-confirm-transients.md @@ -0,0 +1,19 @@ +--- +'@portabletext/plugin-sdk-value': patch +--- + +fix: only repair divergence that persists, never transient store states + +With two users typing simultaneously (even in different blocks), a remote +transaction arriving interleaved with the listener echoes of this client's +own recent edits leaves the store value transiently wrong until the rebase +corrects it moments later. The whole-value repair used to fire inside that +window: it copied the transient into the editor (deleting real text) and a +follow-up repair restored the text at a drifted offset, scrambling words +the user typed in between. + +The repair now confirms divergence before acting: it waits out the echo +round trip and only applies when the exact (editor, store) state pair is +unchanged, and never while local keystrokes are unflushed. Transients +self-correct and produce no editor writes; genuine divergence is stable +and gets repaired one beat later. diff --git a/packages/plugin-sdk-value/src/plugin.sdk-value.tsx b/packages/plugin-sdk-value/src/plugin.sdk-value.tsx index cf0c1b0ce..4be7e0794 100644 --- a/packages/plugin-sdk-value/src/plugin.sdk-value.tsx +++ b/packages/plugin-sdk-value/src/plugin.sdk-value.tsx @@ -571,6 +571,66 @@ function debugTextOf(value: unknown): string { .join('\n') } +/** + * How long an editor-versus-store divergence must persist, unchanged, + * before the whole-value repair acts on it. When a remote transaction + * arrives interleaved with the listener echoes of this client's own recent + * edits, the store value is transiently wrong until the echo returns and + * the rebase corrects it. A repair fired inside that window copies the + * garbage into the editor and a follow-up repair restores the text at a + * drifted offset, scrambling words the user typed in the meantime. The + * window therefore has to outlast a slow listener echo round trip; real + * divergence is stable and loses nothing by being repaired a beat later. + * + * Tests use a short window: their mock stores are synchronous, so echo + * transients cannot occur, and waiting the production interval would only + * slow every repair assertion down. + */ +const REPAIR_CONFIRM_DELAY = + // @ts-expect-error - dot notation required for Vite to replace at build time + process.env.NODE_ENV === 'test' ? 150 : 1000 + +type PendingRepair = {signature: string; timer: ReturnType} +const pendingRepairs = new WeakMap() + +/** + * Local edits the editor has emitted but not yet flushed into a mutation. + * While any exist, the store necessarily lags the editor and a repair diff + * would "repair away" the user's unflushed keystrokes, so repairs must wait. + */ +const unflushedEdits = new WeakMap() + +function computeRepair( + editor: Editor, + remoteValue: PortableTextBlock[], +): {patches: PtePatch[]; convertible: boolean; signature: string} { + const snapshot = editor.getSnapshot().context.value + // The signature covers the full (editor, store) state, not just the diff: + // with repetitive text, two different transients can produce an identical + // diff, and a repair must only act when the world actually stood still. + const stateSignature = JSON.stringify([snapshot, remoteValue]) + try { + const patches = toEngineSafePatches( + convertPatches(diffValue(snapshot, remoteValue)), + remoteValue, + ) + return {patches, convertible: true, signature: stateSignature} + } catch { + // diffValue can emit path shapes the converter does not understand + // (e.g. array slices when multiple items are dropped). The repair then + // has to fall back to a whole-value update. + return {patches: [], convertible: false, signature: `!${stateSignature}`} + } +} + +function cancelPendingRepair(editor: Editor) { + const pending = pendingRepairs.get(editor) + if (pending) { + clearTimeout(pending.timer) + pendingRepairs.delete(editor) + } +} + function applySync({ editor, getRemoteValue, @@ -584,33 +644,67 @@ function applySync({ return } - const snapshot = editor.getSnapshot().context.value + const first = computeRepair(editor, remoteValue) + if (first.convertible && first.patches.length === 0) { + cancelPendingRepair(editor) + return + } + if (debug.repair.enabled) { - const editorText = debugTextOf(snapshot) - const remoteText = debugTextOf(remoteValue) - if (editorText !== remoteText) { - debug.repair('editor and store texts differ %o', {editorText, remoteText}) - } + debug.repair('editor and store texts diverged %o', { + editorText: debugTextOf(editor.getSnapshot().context.value), + remoteText: debugTextOf(remoteValue), + }) } - let patches: PtePatch[] - try { - patches = toEngineSafePatches( - convertPatches(diffValue(snapshot, remoteValue)), - remoteValue, - ) - } catch { - // diffValue can emit path shapes the converter does not understand - // (e.g. array slices when multiple items are dropped). Fall back to - // the full value sync machinery rather than leaving the editor - // diverged. - editor.send({type: 'update value', value: remoteValue}) + // Same divergence already awaiting confirmation: let that timer decide. + const pending = pendingRepairs.get(editor) + if (pending && pending.signature === first.signature) { return } + cancelPendingRepair(editor) + + const timer = setTimeout(() => { + pendingRepairs.delete(editor) + + // Unflushed local keystrokes mean the store lags the editor by design; + // a repair now would delete them. Re-arm and wait for the flush (the + // divergence either disappears once the push round-trips, or persists + // and gets repaired then). + if (unflushedEdits.get(editor)) { + applySync({editor, getRemoteValue}) + return + } + + // Recompute from scratch: the store may have corrected itself (the + // transient case) or the user may have typed (their edits will push + // and reconverge through the normal flow); only a divergence that is + // still byte-for-byte identical is real and safe to repair. + const latestRemote = getRemoteValue() + if (!latestRemote) { + return + } + const second = computeRepair(editor, latestRemote) + if (second.convertible && second.patches.length === 0) { + return + } + if (second.signature !== first.signature) { + // Still diverged but differently: re-arm so a stable state eventually + // confirms. Transients converge to the empty diff; genuine divergence + // stabilizes to a fixed signature within one flush cycle. + applySync({editor, getRemoteValue}) + return + } + + if (!second.convertible) { + debug.repair('escalating to whole-value sync (unconvertible diff)') + editor.send({type: 'update value', value: latestRemote}) + return + } - if (patches.length) { - debug.repair('applying repair patches %o', patches) - editor.send({type: 'patches', patches, snapshot}) + const snapshot = editor.getSnapshot().context.value + debug.repair('applying confirmed repair patches %o', second.patches) + editor.send({type: 'patches', patches: second.patches, snapshot}) // Patch application is best-effort: the editor skips operations it // cannot resolve against its current tree, and concurrent edits to the @@ -619,25 +713,32 @@ function applySync({ // repair, escalate to the full value sync machinery, which reconciles // arbitrary divergence block by block. const valueAfterPatches = editor.getSnapshot().context.value - if (diffValue(valueAfterPatches, remoteValue).length > 0) { + if (diffValue(valueAfterPatches, latestRemote).length > 0) { if (debug.repair.enabled) { debug.repair('escalating to whole-value sync %o', { editorText: debugTextOf(valueAfterPatches), - remoteText: debugTextOf(remoteValue), + remoteText: debugTextOf(latestRemote), }) } - editor.send({type: 'update value', value: remoteValue}) + editor.send({type: 'update value', value: latestRemote}) } - } + }, REPAIR_CONFIRM_DELAY) + + pendingRepairs.set(editor, {signature: first.signature, timer}) } const listenToEditor = fromCallback( ({sendBack, input}) => { const patchSubscription = input.editor.on('patch', () => { + // Every 'patch' event is a local edit (remote application suppresses + // patch generation), so the store now lags the editor until the next + // mutation flush. + unflushedEdits.set(input.editor, true) sendBack({type: 'patch emitted'}) }) const mutationSubscription = input.editor.on('mutation', (event) => { + unflushedEdits.set(input.editor, false) if (debug.mutation.enabled) { debug.mutation('flushed %o', { flushText: debugTextOf(event.value), diff --git a/packages/plugin-sdk-value/src/plugin.value-sync.browser.test.tsx b/packages/plugin-sdk-value/src/plugin.value-sync.browser.test.tsx index eb3e388fa..76bb31807 100644 --- a/packages/plugin-sdk-value/src/plugin.value-sync.browser.test.tsx +++ b/packages/plugin-sdk-value/src/plugin.value-sync.browser.test.tsx @@ -309,6 +309,72 @@ describe('ValueSyncPlugin', () => { }) }) + // Field regression: when a collaborator's transaction arrives + // interleaved with the listener echoes of this client's own recent + // edits, the store value is transiently wrong until the rebase + // corrects it. A repair fired inside that window used to copy the + // transient into the editor and a follow-up repair restored the text + // at a drifted offset, scrambling words typed in the meantime. The + // repair must wait out the blink and only act on divergence that + // persists. + test('a transiently wrong store value is never copied into the editor', async () => { + const store = createMockValueStore([makeBlock('b1', 'stable text here')]) + const {editor, unmount} = await createSyncedEditor({store}) + cleanup = unmount + + await vi.waitFor(() => { + expect(getEditorText(editor)).toEqual('B: stable text here') + }) + + // A whipsaw self-heals by the end (the second repair restores the + // text), so final-state assertions cannot catch it; in production the + // user types between the two repairs and the restore lands at a + // drifted offset. The direct observable is the repair traffic itself: + // a transient must produce ZERO sync writes into the editor. + const syncWrites: Array = [] + const originalSend = editor.send.bind(editor) + editor.send = ((event: Parameters[0]) => { + if (event.type === 'patches' || event.type === 'update value') { + syncWrites.push(event.type) + } + originalSend(event) + }) as Editor['send'] + + try { + // The store blinks: a wrong value (own edits double-applied) that + // self-corrects shortly after, well inside the confirmation window. + store.setRemoteValue([makeBlock('b1', 'stable tex')]) + await new Promise((resolve) => setTimeout(resolve, 50)) + store.setRemoteValue([makeBlock('b1', 'stable text here')]) + + // Wait past the confirmation window. + await new Promise((resolve) => setTimeout(resolve, 400)) + } finally { + editor.send = originalSend + } + + expect(getEditorText(editor)).toEqual('B: stable text here') + expect(syncWrites).toEqual([]) + }) + + test('persistent divergence still gets repaired', async () => { + const store = createMockValueStore([makeBlock('b1', 'before')]) + const {editor, unmount} = await createSyncedEditor({store}) + cleanup = unmount + + await vi.waitFor(() => { + expect(getEditorText(editor)).toEqual('B: before') + }) + + // A real remote change: it persists, so after the confirmation + // window the repair applies it. + store.setRemoteValue([makeBlock('b1', 'after, and it stays')]) + + await vi.waitFor(() => { + expect(getEditorText(editor)).toEqual('B: after, and it stays') + }) + }) + test('remote decorator toggle syncs `marks` inserts and unsets', async () => { const makeValue = (marks: string[]): PortableTextBlock[] => [ {