diff --git a/.changeset/apply-autoresolve-to-engine-block.md b/.changeset/apply-autoresolve-to-engine-block.md new file mode 100644 index 0000000000..4de8b9a3c5 --- /dev/null +++ b/.changeset/apply-autoresolve-to-engine-block.md @@ -0,0 +1,15 @@ +--- +'@portabletext/editor': patch +--- + +fix: apply value-sync auto-resolutions to the block the engine receives + +When an `update value` carried a block the editor could repair +automatically (a child missing its `_key`, an empty `children` array, +unused `markDefs`), the repair was emitted as patches while the raw, +un-repaired block proceeded into the editor. The editor then held an +invalid shape that diverged from the document: the emitted patch minted +one key, internal normalization minted another. The repair is now +applied to the block before it enters the editor, so the emitted patch +and the editor state agree, and the editor never holds the un-repaired +shape. diff --git a/packages/editor/src/editor/sync-machine.ts b/packages/editor/src/editor/sync-machine.ts index 8aff9245ee..f5bc053560 100644 --- a/packages/editor/src/editor/sync-machine.ts +++ b/packages/editor/src/editor/sync-machine.ts @@ -1,4 +1,4 @@ -import type {Patch} from '@portabletext/patches' +import {applyAll, type Patch} from '@portabletext/patches' import {isSpan, isTextBlock, type PortableTextBlock} from '@portabletext/schema' import type {ActorRefFrom} from 'xstate' import { @@ -703,7 +703,13 @@ function syncBlock({ ) if (validation.valid || validation.resolution?.autoResolve) { - const engineBlock = toEngineBlock(block, { + // Apply the auto-resolution to the block the engine receives, so the + // engine never holds the un-repaired shape. + const repairedBlock = + !validation.valid && validation.resolution?.autoResolve + ? (applyAll([block], validation.resolution.patches).at(0) ?? block) + : block + const engineBlock = toEngineBlock(repairedBlock, { schemaTypes: context.schema, }) @@ -785,8 +791,20 @@ function syncBlock({ } if (validation.valid || validation.resolution?.autoResolve) { + // Apply the auto-resolution to the block the engine receives. Without + // this the repair only exists as the emitted patches above while the + // raw block proceeds into the engine: the engine ends up holding the + // un-repaired shape (e.g. a keyless child), diverging from the document + // that received the minted key, and the next sync against that invalid + // engine state can kill the sync. + const repairedBlock = + !validation.valid && validation.resolution?.autoResolve + ? (applyAll(validationValue, validation.resolution.patches).at(0) ?? + block) + : block + if (oldBlock._key === block._key && oldBlock._type === block._type) { - debug.syncValue('Updating block', oldBlock, block) + debug.syncValue('Updating block', oldBlock, repairedBlock) withoutNormalizing(editorEngine, () => { withRemoteChanges(editorEngine, () => { @@ -795,14 +813,14 @@ function syncBlock({ context, editorEngine, oldEngineBlock, - block, + block: repairedBlock, index, }) }) }) }) } else { - debug.syncValue('Replacing block', oldBlock, block) + debug.syncValue('Replacing block', oldBlock, repairedBlock) withoutNormalizing(editorEngine, () => { withRemoteChanges(editorEngine, () => { @@ -810,7 +828,7 @@ function syncBlock({ replaceBlock({ context, editorEngine, - block, + block: repairedBlock, index, }) }) diff --git a/packages/editor/tests/container-normalization.test.tsx b/packages/editor/tests/container-normalization.test.tsx index ade54c81db..68221615c4 100644 --- a/packages/editor/tests/container-normalization.test.tsx +++ b/packages/editor/tests/container-normalization.test.tsx @@ -398,7 +398,7 @@ describe('container normalization', () => { children: [ { _type: 'span', - _key: 'k4', + _key: 'k3', text: '', marks: [], }, diff --git a/packages/editor/tests/event.operation.test.tsx b/packages/editor/tests/event.operation.test.tsx index 1bcd0b4f94..18989709e2 100644 --- a/packages/editor/tests/event.operation.test.tsx +++ b/packages/editor/tests/event.operation.test.tsx @@ -105,17 +105,13 @@ describe('event.operation', () => { expect(patches).toEqual([]) }) - test('Scenario: Normalization fix operations are delivered adjacent to their trigger', async () => { + test('Scenario: Auto-resolved blocks arrive repaired in their own insert operation', async () => { const {editor} = await createTestEditor() const operations = collectOperations(editor) - // A text block with no children triggers a normalization fix that - // inserts a placeholder span. Value sync applies in a batch, so the fix - // runs at the batch close and is delivered after its trigger - // (application order). The opposite, nested order for unbatched applies - // is pinned at the unit level in `operation-channel.test.ts`; the - // public contract is only adjacency, which is why the docs steer - // consumers toward snapshot-seeded recompute. + // A text block with no children is auto-resolved by `validateValue` at + // sync ingress: the placeholder span is part of the inserted node + // itself, not a separate normalization fix operation. editor.send({type: 'update value', value: [emptyBlock('b1')]}) await vi.waitFor(() => { @@ -128,16 +124,61 @@ describe('event.operation', () => { type: 'insert', path: [0], position: 'before', - node: emptyBlock('b1'), + node: { + ...emptyBlock('b1'), + children: [{_type: 'span', _key: 'k2', text: '', marks: []}], + }, + }, + ]) + }) + }) + + test('Scenario: Normalization fix operations are delivered adjacent to their trigger', async () => { + const {editor} = await createTestEditor() + const operations = collectOperations(editor) + + // Duplicate child keys pass value validation (no dup-key check at + // ingress) and are repaired by engine normalization, which renames the + // second occurrence. Value sync applies in a batch, so the fix runs at + // the batch close and is delivered after its trigger (application + // order). The opposite, nested order for unbatched applies is pinned + // at the unit level in `operation-channel.test.ts`; the public + // contract is only adjacency, which is why the docs steer consumers + // toward snapshot-seeded recompute. + // The differing marks keep the two spans from also triggering the + // adjacent same-mark merge, so the rename is the only fix. + const dupBlock: PortableTextBlock = { + _type: 'block', + _key: 'b1', + style: 'normal', + markDefs: [], + children: [ + {_type: 'span', _key: 'dup', text: 'one', marks: []}, + {_type: 'span', _key: 'dup', text: 'two', marks: ['strong']}, + ], + } + editor.send({type: 'update value', value: [dupBlock]}) + + await vi.waitFor(() => { + expect(operations).toEqual([ + { + type: 'unset', + path: [{_key: 'k0'}], }, { type: 'insert', - path: [{_key: 'b1'}, 'children', 0], + path: [0], position: 'before', - node: {_type: 'span', _key: 'k3', text: '', marks: []}, + node: dupBlock, + }, + { + type: 'set', + path: [{_key: 'b1'}, 'children', 1, '_key'], + value: 'k2', inverse: { - type: 'unset', - path: [{_key: 'b1'}, 'children', {_key: 'k3'}], + type: 'set', + path: [{_key: 'b1'}, 'children', 1, '_key'], + value: 'dup', }, }, ]) diff --git a/packages/editor/tests/event.update-value.test.tsx b/packages/editor/tests/event.update-value.test.tsx index 5a2dc55931..1ef0fefd7a 100644 --- a/packages/editor/tests/event.update-value.test.tsx +++ b/packages/editor/tests/event.update-value.test.tsx @@ -1,6 +1,11 @@ import {createTestKeyGenerator} from '@portabletext/test' import {describe, expect, test, vi} from 'vitest' -import {defineSchema, type EditorEmittedEvent} from '../src' +import { + defineSchema, + type EditorEmittedEvent, + type MutationEvent, + type Patch, +} from '../src' import {EventListenerPlugin} from '../src/plugins/plugin.event-listener' import {createTestEditor} from '../src/test/vitest' import {toTextspec} from '../test-utils/to-textspec' @@ -1912,3 +1917,156 @@ describe('event.update value: adjacent same-mark spans', () => { }) }) }) + +describe('event.update value: auto-resolved invalid blocks', () => { + // Regression: `validateValue` auto-resolutions (e.g. minting a missing + // child `_key`) were emitted as outbound patches while the *raw* block + // proceeded into the engine. The engine ended up holding the un-repaired + // shape (a keyless child), diverging from the document that received the + // minted key, and the next sync against that invalid engine state killed + // the sync silently. + const keylessChildBlock = { + _key: 'b0', + _type: 'block', + children: [{_type: 'span', text: 'hello changed', marks: []}], + markDefs: [], + style: 'normal', + } + + test('Scenario: a mid-session update with a keyless child is repaired once and the sync survives', async () => { + const patches: Array = [] + const {editor} = await createTestEditor({ + keyGenerator: createTestKeyGenerator(), + schemaDefinition: defineSchema({}), + initialValue: [ + { + _key: 'b0', + _type: 'block', + children: [{_key: 's0', _type: 'span', text: 'hello', marks: []}], + markDefs: [], + style: 'normal', + }, + ], + children: ( + { + if (event.type === 'patch') { + patches.push(event.patch) + } + }} + /> + ), + }) + + // A changed block arrives whose span lost its `_key`. + editor.send({type: 'update value', value: [keylessChildBlock]}) + + // The auto-resolution is emitted as a patch AND applied to the block + // the engine receives: one key, minted once, on both sides. + await vi.waitFor(() => { + expect(patches).toEqual([ + { + type: 'set', + path: [{_key: 'b0'}, 'children', 0], + value: { + _type: 'span', + _key: 'k2', + text: 'hello changed', + marks: [], + }, + }, + ]) + expect(editor.getSnapshot().context.value).toEqual([ + { + _key: 'b0', + _type: 'block', + children: [ + {_key: 'k2', _type: 'span', text: 'hello changed', marks: []}, + ], + markDefs: [], + style: 'normal', + }, + ]) + }) + + // The sync is still alive: a later update still lands. + editor.send({ + type: 'update value', + value: [ + { + _key: 'b0', + _type: 'block', + children: [{_key: 's9', _type: 'span', text: 'recovered', marks: []}], + markDefs: [], + style: 'normal', + }, + ], + }) + + await vi.waitFor( + () => { + expect(editor.getSnapshot().context.value).toEqual([ + { + _key: 'b0', + _type: 'block', + children: [ + {_key: 's9', _type: 'span', text: 'recovered', marks: []}, + ], + markDefs: [], + style: 'normal', + }, + ]) + }, + // The sync machine parks in `busy` while its own emitted mutation + // flushes and re-checks on a 1s timer, so the recovery value can + // take a beat over a second to land. + {timeout: 5000}, + ) + }) + + test('Scenario: a startup value with a keyless child is repaired in the engine without emitting patches', async () => { + const patches: Array = [] + const mutations: Array = [] + const {editor} = await createTestEditor({ + keyGenerator: createTestKeyGenerator(), + schemaDefinition: defineSchema({}), + initialValue: [ + { + _key: 'b0', + _type: 'block', + children: [{_type: 'span', text: 'hello', marks: []}], + markDefs: [], + style: 'normal', + }, + ], + children: ( + { + if (event.type === 'patch') { + patches.push(event.patch) + } + if (event.type === 'mutation') { + mutations.push(event) + } + }} + /> + ), + }) + + await vi.waitFor(() => { + expect(editor.getSnapshot().context.value).toEqual([ + { + _key: 'b0', + _type: 'block', + children: [{_key: 'k2', _type: 'span', text: 'hello', marks: []}], + markDefs: [], + style: 'normal', + }, + ]) + }) + + // No mutation leaves a pristine editor on open. + expect(patches).toEqual([]) + expect(mutations).toEqual([]) + }) +}) diff --git a/packages/editor/tests/pteWarningsSelfSolving.test.tsx b/packages/editor/tests/pteWarningsSelfSolving.test.tsx index f798322cc2..4c2c7bf119 100644 --- a/packages/editor/tests/pteWarningsSelfSolving.test.tsx +++ b/packages/editor/tests/pteWarningsSelfSolving.test.tsx @@ -54,7 +54,7 @@ describe('when PTE would display warnings, instead it self solves', () => { _type: 'block', children: [ { - _key: 'k3', + _key: 'k2', _type: 'span', text: 'Hello with a new key', marks: [], @@ -174,7 +174,7 @@ describe('when PTE would display warnings, instead it self solves', () => { _type: 'block', children: [ { - _key: 'k3', + _key: 'k2', _type: 'span', text: '', marks: [], @@ -188,7 +188,7 @@ describe('when PTE would display warnings, instead it self solves', () => { _type: 'block', children: [ { - _key: 'k5', + _key: 'k3', _type: 'span', text: '', marks: [],