diff --git a/packages/editor/src/editor/remote-patches.ts b/packages/editor/src/editor/remote-patches.ts index aedd650b8..056b99e4e 100644 --- a/packages/editor/src/editor/remote-patches.ts +++ b/packages/editor/src/editor/remote-patches.ts @@ -37,8 +37,25 @@ export function setupRemotePatches({ let changed = false withRemoteChanges(editor, () => { - withoutNormalizing(editor, () => { - withoutPatching(editor, () => { + // `withoutPatching` must wrap `withoutNormalizing`, not the other way + // around: `withoutNormalizing` runs a final normalize pass on exit, + // and with the previous nesting that pass ran after patching was + // restored. Normalization fallout of remote application (e.g. merging + // adjacent same-mark spans a collaborator's formatting toggle left + // behind) was then emitted as local patches and pushed back to the + // server. Every receiving client pushed its own competing "cleanup" + // of the same structure, and the interleaved merges corrupted the + // shared document (fragments deleted after their text had moved, + // text applied twice). Normalization fallout of remote changes must + // stay local; the originating client runs the same normalization as + // a genuine local edit and pushes it, so the server still converges + // to the normalized form. + // + // NOTE (draft): blanket suppression conflicts with the self-solving + // documents contract; see the PR description for the enumeration and + // the proposed hold-and-discard alternative. + withoutPatching(editor, () => { + withoutNormalizing(editor, () => { pluginWithoutHistory(editor, () => { for (const patch of patches) { try { @@ -59,9 +76,11 @@ export function setupRemotePatches({ } }) }) + if (changed) { + normalize(editor) + } }) if (changed) { - normalize(editor) editor.onChange() } }) diff --git a/packages/editor/src/editor/sync-machine.ts b/packages/editor/src/editor/sync-machine.ts index 8aff9245e..666785120 100644 --- a/packages/editor/src/editor/sync-machine.ts +++ b/packages/editor/src/editor/sync-machine.ts @@ -600,10 +600,10 @@ function clearEditor({ editorEngine: PortableTextEditorEngine doneSyncing: boolean }) { - withoutNormalizing(editorEngine, () => { - pluginWithoutHistory(editorEngine, () => { - withRemoteChanges(editorEngine, () => { - withoutPatching(editorEngine, () => { + withoutPatching(editorEngine, () => { + withoutNormalizing(editorEngine, () => { + pluginWithoutHistory(editorEngine, () => { + withRemoteChanges(editorEngine, () => { if (doneSyncing) { return } @@ -640,9 +640,9 @@ function removeExtraBlocks({ }) { let isChanged = false - withoutNormalizing(editorEngine, () => { - withRemoteChanges(editorEngine, () => { - withoutPatching(editorEngine, () => { + withoutPatching(editorEngine, () => { + withoutNormalizing(editorEngine, () => { + withRemoteChanges(editorEngine, () => { const childrenLength = editorEngine.snapshot.context.value.length if (value.length < childrenLength) { @@ -707,9 +707,9 @@ function syncBlock({ schemaTypes: context.schema, }) - withoutNormalizing(editorEngine, () => { - withRemoteChanges(editorEngine, () => { - withoutPatching(editorEngine, () => { + withoutPatching(editorEngine, () => { + withoutNormalizing(editorEngine, () => { + withRemoteChanges(editorEngine, () => { editorEngine.apply({ type: 'insert', path: [editorEngine.snapshot.context.value.length], @@ -788,9 +788,9 @@ function syncBlock({ if (oldBlock._key === block._key && oldBlock._type === block._type) { debug.syncValue('Updating block', oldBlock, block) - withoutNormalizing(editorEngine, () => { - withRemoteChanges(editorEngine, () => { - withoutPatching(editorEngine, () => { + withoutPatching(editorEngine, () => { + withoutNormalizing(editorEngine, () => { + withRemoteChanges(editorEngine, () => { updateBlock({ context, editorEngine, @@ -804,9 +804,9 @@ function syncBlock({ } else { debug.syncValue('Replacing block', oldBlock, block) - withoutNormalizing(editorEngine, () => { - withRemoteChanges(editorEngine, () => { - withoutPatching(editorEngine, () => { + withoutPatching(editorEngine, () => { + withoutNormalizing(editorEngine, () => { + withRemoteChanges(editorEngine, () => { replaceBlock({ context, editorEngine, diff --git a/packages/editor/tests/remote-patches-normalization.test.tsx b/packages/editor/tests/remote-patches-normalization.test.tsx new file mode 100644 index 000000000..b33753254 --- /dev/null +++ b/packages/editor/tests/remote-patches-normalization.test.tsx @@ -0,0 +1,134 @@ +import type {Patch} from '@portabletext/patches' +import {insert} from '@portabletext/patches' +import {createTestKeyGenerator} from '@portabletext/test' +import {describe, expect, test, vi} from 'vitest' +import {createTestEditor} from '../src/test/vitest' + +/** + * Normalization fallout of remote patch application must stay local. + * + * When a collaborator's formatting toggle splits a span, the receiving + * editor's normalizer merges the resulting adjacent same-mark spans back + * together, which is correct local hygiene. But if those merge operations + * are emitted as local patches, every receiving client pushes its own + * competing "cleanup" of the same structure back to the server, and the + * interleaved merges corrupt the shared document (fragments unset after + * their text moved elsewhere, text applied twice). Field signature: + * formatting appears to work, then the formatted region's text is + * duplicated at the end of the block and the marks are gone. + */ +describe('remote patch application does not emit normalization patches', () => { + test('merging adjacent same-mark spans created by remote patches stays local', async () => { + const keyGenerator = createTestKeyGenerator() + const b1 = keyGenerator() + const s1 = keyGenerator() + + const {editor} = await createTestEditor({ + keyGenerator, + initialValue: [ + { + _type: 'block', + _key: b1, + children: [{_type: 'span', _key: s1, text: 'foo', marks: []}], + markDefs: [], + style: 'normal', + }, + ], + }) + + const emittedPatches: Patch[] = [] + const patchSubscription = editor.on('patch', (event) => { + if ('patch' in event) { + emittedPatches.push(event.patch) + } + }) + const emittedMutations: Patch[][] = [] + const mutationSubscription = editor.on('mutation', (event) => { + emittedMutations.push(event.patches) + }) + + // A collaborator's split arrives: a second span with identical (empty) + // marks lands next to the existing one. The local normalizer merges the + // two spans into one. + editor.send({ + type: 'patches', + patches: [ + insert( + [{_type: 'span', _key: keyGenerator(), text: 'bar', marks: []}], + 'after', + [{_key: b1}, 'children', {_key: s1}], + ), + ], + snapshot: undefined, + }) + + await vi.waitFor(() => { + const block = editor.getSnapshot().context.value[0] as { + children: {text: string}[] + } + // normalized: the spans merged into one + expect(block.children).toHaveLength(1) + expect(block.children[0]!.text).toBe('foobar') + }) + + // The merge itself must not be broadcast: no local patches, no mutation. + await new Promise((resolve) => setTimeout(resolve, 600)) + expect(emittedPatches).toEqual([]) + expect(emittedMutations).toEqual([]) + + patchSubscription.unsubscribe() + mutationSubscription.unsubscribe() + }) + + test('genuine local edits still emit patches after remote application', async () => { + const keyGenerator = createTestKeyGenerator() + const b1 = keyGenerator() + const s1 = keyGenerator() + + const {editor} = await createTestEditor({ + keyGenerator, + initialValue: [ + { + _type: 'block', + _key: b1, + children: [{_type: 'span', _key: s1, text: 'foo', marks: []}], + markDefs: [], + style: 'normal', + }, + ], + }) + + editor.send({ + type: 'patches', + patches: [ + insert( + [{_type: 'span', _key: keyGenerator(), text: 'bar', marks: []}], + 'after', + [{_key: b1}, 'children', {_key: s1}], + ), + ], + snapshot: undefined, + }) + await vi.waitFor(() => { + const block = editor.getSnapshot().context.value[0] as { + children: {text: string}[] + } + expect(block.children[0]!.text).toBe('foobar') + }) + + const emittedPatches: Patch[] = [] + const subscription = editor.on('patch', (event) => { + if ('patch' in event) { + emittedPatches.push(event.patch) + } + }) + + editor.send({type: 'insert.text', text: '!'}) + + await vi.waitFor(() => { + expect(emittedPatches.length).toBeGreaterThan(0) + }) + + subscription.unsubscribe() + }) +})