diff --git a/.changeset/fix-key-rename-undo-inverse.md b/.changeset/fix-key-rename-undo-inverse.md new file mode 100644 index 0000000000..339b22c532 --- /dev/null +++ b/.changeset/fix-key-rename-undo-inverse.md @@ -0,0 +1,7 @@ +--- +'@portabletext/editor': patch +--- + +fix: locate a renamed node by its new key when undoing a `_key` change + +Undoing an edit that renamed a node's own `_key` (a collision-avoiding rename ahead of a block merge, for example) left the node stuck under its new key instead of restoring the old one: the undo step tried to find the node by the key it had before the rename, which no longer resolved to anything once the rename had applied. Undoing now locates the node by its current key first, so a rename reverts cleanly along with everything else in the same undo step. diff --git a/.changeset/rename-colliding-keys-before-merge.md b/.changeset/rename-colliding-keys-before-merge.md new file mode 100644 index 0000000000..2e4ee6d199 --- /dev/null +++ b/.changeset/rename-colliding-keys-before-merge.md @@ -0,0 +1,7 @@ +--- +'@portabletext/editor': patch +--- + +fix: rename colliding keys before backspace merges a block into its predecessor + +Backspacing at the start of a text block to merge it into the previous block could silently mint fresh `_key`s for any child span or annotation whose key collided with one already in the destination block. On the wire this read as those nodes being destroyed and re-created rather than moved, so a collaborator applying the same patches lost their caret through the merge instead of following it. The merge now renames the colliding keys first, so the wire shows the rename followed by the move, and collaborators keep their position. diff --git a/packages/editor/src/behaviors/behavior.abstract.delete.ts b/packages/editor/src/behaviors/behavior.abstract.delete.ts index 6d0297c2be..bba95507f7 100644 --- a/packages/editor/src/behaviors/behavior.abstract.delete.ts +++ b/packages/editor/src/behaviors/behavior.abstract.delete.ts @@ -1,5 +1,12 @@ -import {isSpan, isTextBlock} from '@portabletext/schema' +import { + isSpan, + isTextBlock, + type PortableTextTextBlock, + type Schema, +} from '@portabletext/schema' +import type {Path} from '../engine/interfaces/path' import {isTextBlockNode} from '../engine/node/is-text-block-node' +import {isEqualMarks} from '../internal-utils/equality' import {getFocusChild} from '../selectors/selector.get-focus-child' import {getFocusTextBlock} from '../selectors/selector.get-focus-text-block' import {isAtTheEndOfBlock} from '../selectors/selector.is-at-the-end-of-block' @@ -9,7 +16,7 @@ import {getSibling} from '../traversal/get-sibling' import {getBlockEndPoint} from '../utils/util.get-block-end-point' import {getBlockStartPoint} from '../utils/util.get-block-start-point' import {isEmptyTextBlock} from '../utils/util.is-empty-text-block' -import {raise} from './behavior.types.action' +import {raise, type BehaviorAction} from './behavior.types.action' import {defineBehavior} from './behavior.types.behavior' export const abstractDeleteBehaviors = [ @@ -83,10 +90,26 @@ export const abstractDeleteBehaviors = [ block: previousBlock, }) - return {previousBlockEndPoint, focusTextBlock} + const {renamedBlock, renameActions} = planMergeKeyRenames({ + context: snapshot.context, + mergingBlockPath: focusTextBlock.path, + mergingBlock: focusTextBlock.node, + destinationBlock: previousBlock.node, + }) + + return { + previousBlockEndPoint, + focusTextBlock, + renamedBlock, + renameActions, + } }, actions: [ - (_, {previousBlockEndPoint, focusTextBlock}) => [ + ( + _, + {previousBlockEndPoint, focusTextBlock, renamedBlock, renameActions}, + ) => [ + ...renameActions, raise({ type: 'delete.block', at: focusTextBlock.path, @@ -100,7 +123,7 @@ export const abstractDeleteBehaviors = [ }), raise({ type: 'insert.block', - block: focusTextBlock.node, + block: renamedBlock, placement: 'auto', select: 'start', }), @@ -332,3 +355,107 @@ export const abstractDeleteBehaviors = [ actions: [({event}) => [raise({...event, type: 'delete'})]], }), ] + +/** + * A block merge folds `mergingBlock`'s children (and markDefs) into + * `destinationBlock` by key. Any key the merging block shares with the + * destination has to be renamed before the merge, or `insert.block`'s own + * collision handling mints a fresh key for it, which reads on the wire as + * that node being destroyed and a new one created instead of moved. + * + * Renames are raised as `set`/`child.set` events against the still-living + * `mergingBlock`, so they land on the wire before the merge's `unset`. + * `renamedBlock` mirrors the same renames applied to the value the caller + * already captured, since raising the rename events now doesn't change + * that captured value. + */ +function planMergeKeyRenames(args: { + context: {schema: Schema; keyGenerator: () => string} + mergingBlockPath: Path + mergingBlock: PortableTextTextBlock + destinationBlock: PortableTextTextBlock +}): { + renamedBlock: PortableTextTextBlock + renameActions: Array +} { + const {context, mergingBlockPath, mergingBlock, destinationBlock} = args + + const destinationChildKeys = new Set( + destinationBlock.children.map((child) => child._key), + ) + const destinationMarkDefKeys = new Set( + (destinationBlock.markDefs ?? []).map((markDef) => markDef._key), + ) + + const markDefKeyMap = new Map() + const renamedMarkDefs = mergingBlock.markDefs?.map((markDef) => { + if (!destinationMarkDefKeys.has(markDef._key)) { + return markDef + } + + const newKey = context.keyGenerator() + markDefKeyMap.set(markDef._key, newKey) + return {...markDef, _key: newKey} + }) + + const renameActions: Array = [] + + for (const markDef of mergingBlock.markDefs ?? []) { + const newKey = markDefKeyMap.get(markDef._key) + + if (newKey) { + renameActions.push( + raise({ + type: 'set', + at: [...mergingBlockPath, 'markDefs', {_key: markDef._key}, '_key'], + value: newKey, + }), + ) + } + } + + const renamedChildren = mergingBlock.children.map((child) => { + const currentMarks = isSpan(context, child) ? child.marks : undefined + const remappedMarks = currentMarks?.map( + (mark) => markDefKeyMap.get(mark) ?? mark, + ) + const marksChanged = Boolean( + remappedMarks && !isEqualMarks(remappedMarks, currentMarks), + ) + + const newKey = destinationChildKeys.has(child._key) + ? context.keyGenerator() + : child._key + + const props: Record = {} + if (newKey !== child._key) { + props['_key'] = newKey + } + if (marksChanged) { + props['marks'] = remappedMarks + } + + if (Object.keys(props).length > 0) { + renameActions.push( + raise({ + type: 'child.set', + at: [...mergingBlockPath, 'children', {_key: child._key}], + props, + }), + ) + } + + return marksChanged + ? {...child, _key: newKey, marks: remappedMarks} + : {...child, _key: newKey} + }) + + return { + renamedBlock: { + ...mergingBlock, + children: renamedChildren, + ...(mergingBlock.markDefs ? {markDefs: renamedMarkDefs} : {}), + }, + renameActions, + } +} diff --git a/packages/editor/src/engine/core/apply-operation.ts b/packages/editor/src/engine/core/apply-operation.ts index bb1263ca79..62299b20eb 100644 --- a/packages/editor/src/engine/core/apply-operation.ts +++ b/packages/editor/src/engine/core/apply-operation.ts @@ -169,10 +169,23 @@ export function applyOperation(editor: Editor, op: EngineOperation): void { if (!op.inverse && !editor.isProcessingRemoteChanges) { const previousValue = getValue(editor.snapshot.context.value, path) + const lastSegment = path[path.length - 1] + + // Renaming a node's own `_key` moves how it resolves: from here on + // it's found by the new key, so the inverse (which runs after the + // rename) has to target that key too, or it can never find the + // node to restore. + const inversePath = + lastSegment === '_key' && + typeof value === 'string' && + isKeyedSegment(path[path.length - 2]) + ? [...path.slice(0, -2), {_key: value}, '_key'] + : path + op.inverse = previousValue === undefined - ? {type: 'unset', path} - : {type: 'set', path, value: previousValue} + ? {type: 'unset', path: inversePath} + : {type: 'set', path: inversePath, value: previousValue} } // Root-level value replacement: set editor.snapshot.context.value directly diff --git a/packages/editor/src/engine/point/step-mapper.test.ts b/packages/editor/src/engine/point/step-mapper.test.ts index e61868d09f..93d990c099 100644 --- a/packages/editor/src/engine/point/step-mapper.test.ts +++ b/packages/editor/src/engine/point/step-mapper.test.ts @@ -396,26 +396,66 @@ describe(mapPointThroughStep.name, () => { }) describe('rekey', () => { - test('substitutes the old key with the new key wherever it appears in the path', () => { + test('substitutes the old key with the new key at the segment directly under the step path', () => { const point = { path: [{_key: 'b1'}, 'children', {_key: 's1'}], offset: 3, } - const step: Step = {type: 'rekey', oldKey: 's1', newKey: 's2'} + const step: Step = { + type: 'rekey', + path: [{_key: 'b1'}, 'children'], + oldKey: 's1', + newKey: 's2', + } expect(mapPointThroughStep(step, point)).toEqual({ path: [{_key: 'b1'}, 'children', {_key: 's2'}], offset: 3, }) }) - test('is a no-op when the old key is not in the path', () => { + test('is a no-op when the old key is not the segment under the step path', () => { const point = { path: [{_key: 'b1'}, 'children', {_key: 's1'}], offset: 3, } - const step: Step = {type: 'rekey', oldKey: 's9', newKey: 's2'} + const step: Step = { + type: 'rekey', + path: [{_key: 'b1'}, 'children'], + oldKey: 's9', + newKey: 's2', + } expect(mapPointThroughStep(step, point)).toBe(point) }) + + test('is a no-op when the key matches but at a different depth than the step path', () => { + const point = { + path: [ + {_key: 'b1'}, + 'children', + {_key: 's1'}, + 'children', + {_key: 's1'}, + ], + offset: 3, + } + const step: Step = { + type: 'rekey', + path: [{_key: 'b1'}, 'children'], + oldKey: 's1', + newKey: 's2', + } + + expect(mapPointThroughStep(step, point)).toEqual({ + path: [ + {_key: 'b1'}, + 'children', + {_key: 's2'}, + 'children', + {_key: 's1'}, + ], + offset: 3, + }) + }) }) describe('replace.children', () => { @@ -829,7 +869,12 @@ describe(mapPointThroughStep.name, () => { ], offset: 3, } - const step: Step = {type: 'rekey', oldKey: 'cell1', newKey: 'cell2'} + const step: Step = { + type: 'rekey', + path: [{_key: 'container1'}, 'rows', {_key: 'row1'}, 'cells'], + oldKey: 'cell1', + newKey: 'cell2', + } expect(mapPointThroughStep(step, point)).toEqual({ path: [ {_key: 'container1'}, @@ -904,7 +949,12 @@ describe(mapPointThroughSteps.name, () => { offset: 3, text: 'abc', }, - {type: 'rekey', oldKey: 's1', newKey: 's2'}, + { + type: 'rekey', + path: [{_key: 'b1'}, 'children'], + oldKey: 's1', + newKey: 's2', + }, ] expect(mapPointThroughSteps(steps, point)).toEqual({ path: [{_key: 'b1'}, 'children', {_key: 's2'}], @@ -919,7 +969,12 @@ describe(mapPointThroughSteps.name, () => { } const steps: Step[] = [ {type: 'remove.node', path: [{_key: 'b1'}, 'children', {_key: 's1'}]}, - {type: 'rekey', oldKey: 's1', newKey: 's2'}, + { + type: 'rekey', + path: [{_key: 'b1'}, 'children'], + oldKey: 's1', + newKey: 's2', + }, ] expect(mapPointThroughSteps(steps, point)).toEqual(null) }) @@ -933,7 +988,14 @@ describe(mapPointThroughSteps.name, () => { }) test('a null point stays null through a batch', () => { - const steps: Step[] = [{type: 'rekey', oldKey: 's1', newKey: 's2'}] + const steps: Step[] = [ + { + type: 'rekey', + path: [{_key: 'b1'}, 'children'], + oldKey: 's1', + newKey: 's2', + }, + ] expect(mapPointThroughSteps(steps, null)).toEqual(null) }) }) diff --git a/packages/editor/src/engine/point/step-mapper.ts b/packages/editor/src/engine/point/step-mapper.ts index 10aeed0a00..b5e1de276f 100644 --- a/packages/editor/src/engine/point/step-mapper.ts +++ b/packages/editor/src/engine/point/step-mapper.ts @@ -38,8 +38,19 @@ export type UnsetTextStep = { path: Path } +/** + * `path` is the array holding the renamed node (its parent path, one + * segment shallower than the node itself): the node's own segment, + * `oldKey`, is only rewritten at that exact depth under that exact + * parent, never wherever the key value happens to recur. A rename only + * ever touches one node; matching by key value alone would also rewrite + * an unrelated point that merely walks through a same-keyed node + * elsewhere in the tree (a duplicate-key merge's destination block, most + * notably, since it starts out sharing every key the donor block does). + */ export type RekeyStep = { type: 'rekey' + path: Path oldKey: string newKey: string } @@ -145,20 +156,26 @@ export function mapPointThroughStep( } case 'rekey': { - let path: Path | undefined - - for (let i = 0; i < point.path.length; i++) { - const segment = point.path[i] + if ( + point.path.length <= step.path.length || + !pathEquals(point.path.slice(0, step.path.length), step.path) + ) { + return point + } - if (isKeyedSegment(segment) && segment._key === step.oldKey) { - if (path === undefined) { - path = [...point.path] - } - path[i] = {_key: step.newKey} - } + const segment = point.path[step.path.length] + if (!isKeyedSegment(segment) || segment._key !== step.oldKey) { + return point } - return path === undefined ? point : {path, offset: point.offset} + return { + path: [ + ...step.path, + {_key: step.newKey}, + ...point.path.slice(step.path.length + 1), + ], + offset: point.offset, + } } case 'replace.children': { diff --git a/packages/editor/src/engine/point/transform-point.ts b/packages/editor/src/engine/point/transform-point.ts index f1d6512b67..32dc0e378d 100644 --- a/packages/editor/src/engine/point/transform-point.ts +++ b/packages/editor/src/engine/point/transform-point.ts @@ -47,7 +47,16 @@ function operationToSteps(op: EngineOperation): Step[] { ? op.inverse.value : undefined - return oldKey ? [{type: 'rekey', oldKey, newKey: op.value}] : [] + return oldKey + ? [ + { + type: 'rekey', + path: nodePath.slice(0, -1), + oldKey, + newKey: op.value, + }, + ] + : [] } if (propertyName === 'text') { diff --git a/packages/editor/src/internal-utils/interpret-transaction.test.ts b/packages/editor/src/internal-utils/interpret-transaction.test.ts index b8f41ba536..461fa501b1 100644 --- a/packages/editor/src/internal-utils/interpret-transaction.test.ts +++ b/packages/editor/src/internal-utils/interpret-transaction.test.ts @@ -292,6 +292,108 @@ describe(interpretTransaction.name, () => { ] satisfies Array) }) + test('Scenario: block-merge-duplicate-keys recognizes each renamed, reinserted child as its own move', () => { + // Unlike block-merge-backspace, kept as full blocks (not a textspec + // seed): a duplicate `_key` across two blocks has no textspec notation. + const fixture = readFixture('block-merge-duplicate-keys') + const seed = fixture.seed as Array + + const steps = interpretTransaction(seed, fixture.patches) + + // The merge renames every colliding child of `kB` ahead of the merge, + // then unsets `kB` as a whole. Each renamed child reappears as its own + // `insert` under `kA`, so key-reappearance recognizes each one + // individually as a `move.node`, on top of (not instead of) the + // ordinary span-merge fold that then absorbs the first moved child + // (`k2`, holding "foo ") into `kA`'s own last span. `kB` itself never + // reappears anywhere, so its own `remove.node` survives the three + // moves that share its slot, relocated after the last of them: `kB` + // could have held more than what moved, and this is what invalidates + // whatever it didn't. + expect(steps).toEqual([ + { + type: 'rekey', + path: [{_key: 'kB'}, 'children'], + oldKey: 's1', + newKey: 'k2', + }, + { + type: 'rekey', + path: [{_key: 'kB'}, 'children'], + oldKey: 's2', + newKey: 'k3', + }, + { + type: 'rekey', + path: [{_key: 'kB'}, 'children'], + oldKey: 's3', + newKey: 'k4', + }, + { + type: 'move.node', + from: [{_key: 'kB'}, 'children', {_key: 'k2'}], + to: [{_key: 'kA'}, 'children', {_key: 'k2'}], + }, + { + type: 'move.node', + from: [{_key: 'kB'}, 'children', {_key: 'k3'}], + to: [{_key: 'kA'}, 'children', {_key: 'k3'}], + }, + { + type: 'move.node', + from: [{_key: 'kB'}, 'children', {_key: 'k4'}], + to: [{_key: 'kA'}, 'children', {_key: 'k4'}], + }, + {type: 'remove.node', path: [{_key: 'kB'}]}, + { + type: 'move.text', + from: { + path: [{_key: 'kA'}, 'children', {_key: 'k2'}], + offset: 0, + length: 4, + }, + to: { + path: [{_key: 'kA'}, 'children', {_key: 's3'}], + offset: 4, + }, + }, + {type: 'remove.node', path: [{_key: 'kA'}, 'children', {_key: 'k2'}]}, + {type: 'remove.node', path: [{_key: 'kA'}, 'children', {_key: 'k5'}]}, + ] satisfies Array) + + // The caret that sat in kB's third span (" baz") at offset 3 follows + // its rename to `k4`, then that span's move into `kA`, landing at the + // same offset: `k4` never folds into anything (its neighbor `k3` + // carries a different mark), so nothing shifts its text or offset + // along the way. + const mapped = mapPointThroughSteps(steps, { + path: [{_key: 'kB'}, 'children', {_key: 's3'}], + offset: 3, + }) + + expect(mapped).toEqual({ + path: [{_key: 'kA'}, 'children', {_key: 'k4'}], + offset: 3, + }) + + // `kA` and `kB` start out sharing every child key (`s1`/`s2`/`s3`), so + // a `rekey` step rewriting by key value alone would also catch a + // caret sitting in `kA`'s own, untouched span of the same name and + // resolve it into the donor's twin after the merge. Each `rekey` step + // only touches the segment directly under `kB`'s own children path, + // so a caret already living under `kA` never matches and comes + // through every step unchanged. + for (const untouchedCaret of [ + {path: [{_key: 'kA'}, 'children', {_key: 's1'}], offset: 2}, + {path: [{_key: 'kA'}, 'children', {_key: 's2'}], offset: 1}, + {path: [{_key: 'kA'}, 'children', {_key: 's3'}], offset: 2}, + ]) { + expect(mapPointThroughSteps(steps, untouchedCaret)).toEqual( + untouchedCaret, + ) + } + }) + test('Scenario: annotation-add-mid-span carves the span in three by moving both flanks', () => { const fixture = readFixture('annotation-add-mid-span') const seed = seedFromTextspec( @@ -1132,6 +1234,150 @@ describe('interpretTransaction oracle: multi-span block splits (key-reappearance }) describe('interpretTransaction: adversarial recognizer misfires', () => { + test('Probe rename-identity: a rename must never lend its lineage to an unrelated node that merely bears the same key value', () => { + const kP = 'kP' + const s1 = 's1' + const kQ = 'kQ' + const collidingKey = 'X' + const kR = 'kR' + const r1 = 'r1' + const seed = [ + makeBlock(kP, s1, 'def'), + makeBlock(kQ, collidingKey, 'abc'), + makeBlock(kR, r1, 'zzz'), + ] + + const patches: Array = [ + // Renames kP's own span to a key that happens to equal kQ's + // pre-existing, wholly unrelated span key. + { + type: 'set', + path: [{_key: kP}, 'children', {_key: s1}, '_key'], + value: collidingKey, + origin: 'local', + }, + // Unrelated: kQ (holding its own, pre-existing 'X') is removed as a + // whole, nothing to do with kP's rename. + {type: 'unset', path: [{_key: kQ}], origin: 'local'}, + // Unrelated: a brand-new span, coincidentally also keyed 'X', + // appears under kR. + { + type: 'insert', + path: [{_key: kR}, 'children', {_key: r1}], + position: 'after', + items: [{_type: 'span', _key: collidingKey, text: 'qqq', marks: []}], + origin: 'local', + }, + ] + + const steps = interpretTransaction(seed, patches) + + // A value-based rename lineage would see kQ's own 'X' descendant as + // "the same node kP renamed" (same key string) and pair it with the + // unrelated insert under kR, teleporting a caret from kQ's dead 'abc' + // into 'qqq'. Identity is checked by path, not key value alone: kQ's + // 'X' never sat where the rename recorded 'X' as living, so nothing + // pairs, and kQ's removal stays a plain `remove.node`. + expect(steps).toEqual([ + { + type: 'rekey', + path: [{_key: kP}, 'children'], + oldKey: s1, + newKey: collidingKey, + }, + {type: 'remove.node', path: [{_key: kQ}]}, + ] satisfies Array) + + const mapped = mapPointThroughSteps(steps, { + path: spanPath(kQ, collidingKey), + offset: 1, + }) + + expect(mapped).toBeNull() + }) + + test('Probe partial-collision merge: a container remove.node survives descendant moves and still invalidates the sibling it does not cover', () => { + const kA = 'kA' + const kB = 'kB' + const collidingKey = 's1' + const renamedKey = 'k2' + const untouchedSibling = 's9' + const seed = [ + makeBlock(kA, collidingKey, 'foo '), + { + _type: 'block', + _key: kB, + style: 'normal', + markDefs: [], + children: [ + {_type: 'span', _key: collidingKey, text: 'bar', marks: []}, + {_type: 'span', _key: untouchedSibling, text: 'baz', marks: []}, + ], + } satisfies PortableTextBlock, + ] + + const patches: Array = [ + // Only `s1` collides with kA's own child, so only it is renamed; + // `s9` has no collision and keeps its key across the merge. + { + type: 'set', + path: [{_key: kB}, 'children', {_key: collidingKey}, '_key'], + value: renamedKey, + origin: 'local', + }, + {type: 'unset', path: [{_key: kB}], origin: 'local'}, + { + type: 'insert', + path: [{_key: kA}, 'children', {_key: collidingKey}], + position: 'after', + items: [{_type: 'span', _key: renamedKey, text: 'bar', marks: []}], + origin: 'local', + }, + { + type: 'insert', + path: [{_key: kA}, 'children', {_key: renamedKey}], + position: 'after', + items: [ + {_type: 'span', _key: untouchedSibling, text: 'baz', marks: []}, + ], + origin: 'local', + }, + ] + + const steps = interpretTransaction(seed, patches) + + // The renamed child resolves as its own `move.node`; `kB`'s own key + // never reappears anywhere, so its `remove.node` survives (relocated + // after that move) rather than being dropped outright: `s9` reused + // its own key untouched, and nothing resolves that reappearance as a + // move of its own, so the container's surviving step is what has to + // invalidate it. + expect(steps).toEqual([ + { + type: 'rekey', + path: [{_key: kB}, 'children'], + oldKey: collidingKey, + newKey: renamedKey, + }, + { + type: 'move.node', + from: spanPath(kB, renamedKey), + to: spanPath(kA, renamedKey), + }, + {type: 'remove.node', path: [{_key: kB}]}, + ] satisfies Array) + + // A caret that sat in the untouched sibling doesn't dangle on a path + // that no longer resolves to anything: the surviving `remove.node` + // nulls it cleanly instead. + const mapped = mapPointThroughSteps(steps, { + path: spanPath(kB, untouchedSibling), + offset: 2, + }) + + expect(mapped).toBeNull() + }) + test('Probe A: an unrelated identical delete and insert in different blocks never pairs', () => { const b1 = 'b1' const s1 = 's1' @@ -1658,6 +1904,152 @@ describe('interpretTransaction: adversarial recognizer misfires', () => { offset: 2, }) }) + + test('Probe chained rename: a descendant renamed twice in the same transaction still resolves its own move', () => { + const source = 'source' + const sibling = 'sibling' + const destination = 'destination' + const anchor = 'anchor' + const seed = [ + { + _type: 'block', + _key: source, + style: 'normal', + markDefs: [], + children: [ + {_type: 'span', _key: 'A', text: 'foo', marks: []}, + {_type: 'span', _key: sibling, text: 'zzz', marks: []}, + ], + } satisfies PortableTextBlock, + makeBlock(destination, anchor, 'bar'), + ] + + const patches: Array = [ + { + type: 'set', + path: [{_key: source}, 'children', {_key: 'A'}, '_key'], + value: 'B', + origin: 'local', + }, + { + type: 'set', + path: [{_key: source}, 'children', {_key: 'B'}, '_key'], + value: 'C', + origin: 'local', + }, + {type: 'unset', path: [{_key: source}], origin: 'local'}, + { + type: 'insert', + path: [{_key: destination}, 'children', {_key: anchor}], + position: 'after', + items: [{_type: 'span', _key: 'C', text: 'foo', marks: []}], + origin: 'local', + }, + ] + + const steps = interpretTransaction(seed, patches) + + // Each `set _key` in the chain records its own step; the second + // rename's lineage (recorded against `C`) is what the container's own + // unset resolves the descendant's reappearance against, so the two + // hops (A to B, B to C) compose instead of only the last one counting. + // `sibling` keeps its own key across the unset, with nothing to + // recognize its reappearance, so the container's own `remove.node` + // survives (relocated after the move) to invalidate it. + expect(steps).toEqual([ + { + type: 'rekey', + path: [{_key: source}, 'children'], + oldKey: 'A', + newKey: 'B', + }, + { + type: 'rekey', + path: [{_key: source}, 'children'], + oldKey: 'B', + newKey: 'C', + }, + { + type: 'move.node', + from: [{_key: source}, 'children', {_key: 'C'}], + to: [{_key: destination}, 'children', {_key: 'C'}], + }, + {type: 'remove.node', path: [{_key: source}]}, + ] satisfies Array) + + const mapped = mapPointThroughSteps(steps, { + path: [{_key: source}, 'children', {_key: 'A'}], + offset: 2, + }) + + expect(mapped).toEqual({ + path: [{_key: destination}, 'children', {_key: 'C'}], + offset: 2, + }) + }) + + test('Probe rename-then-container-hop: a rename recorded against a container identity that itself later changes pairs with nothing', () => { + const originalContainerKey = 'kB' + const renamedContainerKey = 'kC' + const seed = [makeBlock(originalContainerKey, 'A', 'foo')] + + const patches: Array = [ + // The descendant is renamed while its container still answers to + // `kB`: the lineage this records lives at `kB/children/B`. + { + type: 'set', + path: [{_key: originalContainerKey}, 'children', {_key: 'A'}, '_key'], + value: 'B', + origin: 'local', + }, + // The container itself is renamed next, the same shape a + // colliding-block rename ahead of a merge produces: the descendant's + // recorded lineage still reads `kB`, which the container no longer + // answers to. + { + type: 'set', + path: [{_key: originalContainerKey}, '_key'], + value: renamedContainerKey, + origin: 'local', + }, + {type: 'unset', path: [{_key: renamedContainerKey}], origin: 'local'}, + ] + + const steps = interpretTransaction(seed, patches) + + // Neither rename resolves into a `move.node`: the container's own key + // (`kC`) never existed before this transaction, so it isn't a + // recognized container move either, and the descendant's stale + // lineage never reaches `resolveNodeMoves` at all. Both `rekey` steps + // stand, followed by a plain `remove.node` for the container. + expect(steps).toEqual([ + { + type: 'rekey', + path: [{_key: originalContainerKey}, 'children'], + oldKey: 'A', + newKey: 'B', + }, + { + type: 'rekey', + path: [], + oldKey: originalContainerKey, + newKey: renamedContainerKey, + }, + {type: 'remove.node', path: [{_key: renamedContainerKey}]}, + ] satisfies Array) + + // A caret in the renamed descendant follows both renames (a `rekey` + // step matches by recorded path, not by whether the move it was part + // of ever got recognized), then lands inside the container's own + // removal and nulls: nothing carries it out to wherever the container + // went, because nothing recognized that it went anywhere. + const mapped = mapPointThroughSteps(steps, { + path: [{_key: originalContainerKey}, 'children', {_key: 'A'}], + offset: 2, + }) + + expect(mapped).toBeNull() + }) }) describe('interpretTransaction oracle: repeated text across two blocks', () => { diff --git a/packages/editor/src/internal-utils/interpret-transaction.ts b/packages/editor/src/internal-utils/interpret-transaction.ts index 05c2e0e0de..7bb9eec2b8 100644 --- a/packages/editor/src/internal-utils/interpret-transaction.ts +++ b/packages/editor/src/internal-utils/interpret-transaction.ts @@ -113,6 +113,16 @@ export function interpretTransaction( ), ) + // A `set` on `_key` (a rename raised ahead of a merge to dodge a + // collision, say) gives a transaction-local key the same lineage back to + // a pre-existing node, but that lineage is only trusted for a + // container-walk match (see the `unset` case below) on the exact path + // the rename produced: the map records where the renamed node lives, + // not just the key value it now wears, so an unrelated pre-existing + // node that happens to bear the same key elsewhere is never mistaken + // for it. + const renamedKeyPaths = new Map() + const pushStep = (step: Step): number => slots.push(step) - 1 const pushSlot = (): number => slots.push(null) - 1 @@ -210,7 +220,17 @@ export function interpretTransaction( if (propertyName === '_key') { const oldKey = getValue(workingCopy, patch.path) if (typeof oldKey === 'string' && typeof patch.value === 'string') { - pushStep({type: 'rekey', oldKey, newKey: patch.value}) + const containerPath = nodePath.slice(0, -1) + pushStep({ + type: 'rekey', + path: containerPath, + oldKey, + newKey: patch.value, + }) + renamedKeyPaths.set(patch.value, [ + ...containerPath, + {_key: patch.value}, + ]) } } else if (propertyName === 'text') { pushStep({ @@ -280,6 +300,35 @@ export function interpretTransaction( slotIndex, }) } + + // A renamed descendant is independently trackable even though + // the removal here targets its container, not the descendant + // itself (a block merge unsets the whole donor block after + // renaming every colliding child ahead of it, say): each one + // reappearing elsewhere is its own move, worth recognizing on + // top of whatever the container's own key does. It's identified + // by its exact path, not just its key value: a rename recorded + // against one node must never sweep in some other, unrelated + // node that happens to carry a pre-existing key equal to that + // same new value (`renamedKeyPaths` guards exactly this). An + // ordinary (never-renamed) descendant stays untracked here: + // nothing resolves its own move. + for (const keyedNode of collectKeyedNodes( + sourceNode, + patch.path as Path, + )) { + if (keyedNode.key === lastSegment._key) { + continue + } + const renamedPath = renamedKeyPaths.get(keyedNode.key) + if (renamedPath && pathEquals(renamedPath, keyedNode.path)) { + nodeRemovals.push({ + key: keyedNode.key, + path: keyedNode.path, + slotIndex, + }) + } + } } else if (lastSegment === 'text') { pushStep({type: 'unset.text', path: patch.path.slice(0, -1) as Path}) } @@ -467,6 +516,25 @@ function assembleSteps( const supersededSlots = new Set() const nodeMoveByInsertionSlot = new Map() + + // A container's `remove.node` step can end up shared, as a removal + // slot, by more than one recognized move: a block merge unsets the + // whole donor block in one step, and every renamed child it carried + // resolves its own move against that same slot. When the move IS the + // container itself (its `from` is exactly the removed step's own path, + // the block-move shape), the step is fully accounted for and dropped. + // When the move is a strict descendant of it instead (the merge shape), + // the container can still hold more + // than what moved (the contract every `remove.node` step exists to + // enforce), so its step survives, relocated after the last such + // descendant move so a point reaches every move before it can be + // nulled. + const wholeNodeConsumedSlots = new Set() + const relocatedRemovalBySlot = new Map< + number, + {step: Step; afterInsertionSlot: number} + >() + for (const move of nodeMoves) { supersededSlots.add(move.removalSlotIndex) nodeMoveByInsertionSlot.set(move.insertionSlotIndex, move) @@ -477,6 +545,27 @@ function assembleSteps( supersededSlots.add(i) } } + + const removalStep = slots[move.removalSlotIndex] + if (removalStep?.type === 'remove.node') { + if (pathEquals(removalStep.path, move.from)) { + wholeNodeConsumedSlots.add(move.removalSlotIndex) + } else { + const existing = relocatedRemovalBySlot.get(move.removalSlotIndex) + if ( + !existing || + move.insertionSlotIndex > existing.afterInsertionSlot + ) { + relocatedRemovalBySlot.set(move.removalSlotIndex, { + step: removalStep, + afterInsertionSlot: move.insertionSlotIndex, + }) + } + } + } + } + for (const slotIndex of wholeNodeConsumedSlots) { + relocatedRemovalBySlot.delete(slotIndex) } const steps: Array = [] @@ -495,6 +584,12 @@ function assembleSteps( const nodeMove = nodeMoveByInsertionSlot.get(slotIndex) if (nodeMove) { steps.push({type: 'move.node', from: nodeMove.from, to: nodeMove.to}) + const relocatedRemoval = relocatedRemovalBySlot.get( + nodeMove.removalSlotIndex, + ) + if (relocatedRemoval?.afterInsertionSlot === slotIndex) { + steps.push(relocatedRemoval.step) + } continue } diff --git a/packages/editor/src/internal-utils/set-node-properties.ts b/packages/editor/src/internal-utils/set-node-properties.ts index e2b4866aac..344cc7fb81 100644 --- a/packages/editor/src/internal-utils/set-node-properties.ts +++ b/packages/editor/src/internal-utils/set-node-properties.ts @@ -42,23 +42,33 @@ export function setNodeProperties( if (propsRecord[key] !== nodeRecord[key]) { if (propsRecord[key] != null) { const hadProperty = nodeRecord.hasOwnProperty(key) + const lastSegment = currentPath[currentPath.length - 1] + + // Renaming the node's own `_key` moves how it resolves: from here + // on it's found by the new key, so the inverse (which runs after + // the rename) has to target that key too, or it can never find + // the node to restore. + const renamedPath = + key === '_key' && + typeof propsRecord[key] === 'string' && + isKeyedSegment(lastSegment) + ? [...currentPath.slice(0, -1), {_key: propsRecord[key] as string}] + : undefined + const inversePath = renamedPath + ? [...renamedPath, key] + : [...currentPath, key] + editor.apply({ type: 'set', path: [...currentPath, key], value: propsRecord[key], inverse: hadProperty - ? {type: 'set', path: [...currentPath, key], value: nodeRecord[key]} - : {type: 'unset', path: [...currentPath, key]}, + ? {type: 'set', path: inversePath, value: nodeRecord[key]} + : {type: 'unset', path: inversePath}, }) - // After _key changes, update the path for subsequent operations - if (key === '_key' && typeof propsRecord[key] === 'string') { - const lastSegment = currentPath[currentPath.length - 1] - if (isKeyedSegment(lastSegment)) { - currentPath = [ - ...currentPath.slice(0, -1), - {_key: propsRecord[key] as string}, - ] - } + + if (renamedPath) { + currentPath = renamedPath } } else if (nodeRecord.hasOwnProperty(key)) { // Value is null/undefined and property exists on node: unset it diff --git a/packages/editor/tests/__fixtures__/wire-catalogue/block-merge-duplicate-keys.json b/packages/editor/tests/__fixtures__/wire-catalogue/block-merge-duplicate-keys.json new file mode 100644 index 0000000000..469064e30e --- /dev/null +++ b/packages/editor/tests/__fixtures__/wire-catalogue/block-merge-duplicate-keys.json @@ -0,0 +1,321 @@ +{ + "scenario": "block-merge-duplicate-keys", + "schema": { + "decorators": [ + { + "name": "strong" + } + ] + }, + "seed": [ + { + "_type": "block", + "_key": "kA", + "children": [ + { + "_type": "span", + "_key": "s1", + "text": "foo ", + "marks": [] + }, + { + "_type": "span", + "_key": "s2", + "text": "bar", + "marks": [ + "strong" + ] + }, + { + "_type": "span", + "_key": "s3", + "text": " baz", + "marks": [] + } + ], + "markDefs": [], + "style": "normal" + }, + { + "_type": "block", + "_key": "kB", + "children": [ + { + "_type": "span", + "_key": "s1", + "text": "foo ", + "marks": [] + }, + { + "_type": "span", + "_key": "s2", + "text": "bar", + "marks": [ + "strong" + ] + }, + { + "_type": "span", + "_key": "s3", + "text": " baz", + "marks": [] + } + ], + "markDefs": [], + "style": "normal" + } + ], + "seedTerse": [ + "foo ,bar, baz", + "foo ,bar, baz" + ], + "actions": [ + "select {caret at start of block 2}", + "send {type: 'delete.backward', unit: 'character'}" + ], + "patches": [ + { + "type": "set", + "path": [ + { + "_key": "kB" + }, + "children", + { + "_key": "s1" + }, + "_key" + ], + "value": "k2", + "origin": "local" + }, + { + "type": "set", + "path": [ + { + "_key": "kB" + }, + "children", + { + "_key": "s2" + }, + "_key" + ], + "value": "k3", + "origin": "local" + }, + { + "type": "set", + "path": [ + { + "_key": "kB" + }, + "children", + { + "_key": "s3" + }, + "_key" + ], + "value": "k4", + "origin": "local" + }, + { + "type": "unset", + "path": [ + { + "_key": "kB" + } + ], + "origin": "local" + }, + { + "type": "set", + "path": [ + { + "_key": "kA" + }, + "markDefs" + ], + "value": [], + "origin": "local" + }, + { + "type": "setIfMissing", + "path": [ + { + "_key": "kA" + }, + "children" + ], + "value": [], + "origin": "local" + }, + { + "type": "insert", + "path": [ + { + "_key": "kA" + }, + "children", + { + "_key": "s3" + } + ], + "position": "after", + "items": [ + { + "_type": "span", + "_key": "k5", + "marks": [], + "text": "" + } + ], + "origin": "local" + }, + { + "type": "setIfMissing", + "path": [ + { + "_key": "kA" + }, + "children" + ], + "value": [], + "origin": "local" + }, + { + "type": "insert", + "path": [ + { + "_key": "kA" + }, + "children", + { + "_key": "s3" + } + ], + "position": "after", + "items": [ + { + "_type": "span", + "_key": "k2", + "text": "foo ", + "marks": [] + } + ], + "origin": "local" + }, + { + "type": "setIfMissing", + "path": [ + { + "_key": "kA" + }, + "children" + ], + "value": [], + "origin": "local" + }, + { + "type": "insert", + "path": [ + { + "_key": "kA" + }, + "children", + { + "_key": "k2" + } + ], + "position": "after", + "items": [ + { + "_type": "span", + "_key": "k3", + "text": "bar", + "marks": [ + "strong" + ] + } + ], + "origin": "local" + }, + { + "type": "setIfMissing", + "path": [ + { + "_key": "kA" + }, + "children" + ], + "value": [], + "origin": "local" + }, + { + "type": "insert", + "path": [ + { + "_key": "kA" + }, + "children", + { + "_key": "k3" + } + ], + "position": "after", + "items": [ + { + "_type": "span", + "_key": "k4", + "text": " baz", + "marks": [] + } + ], + "origin": "local" + }, + { + "type": "diffMatchPatch", + "path": [ + { + "_key": "kA" + }, + "children", + { + "_key": "s3" + }, + "text" + ], + "value": "@@ -1,4 +1,8 @@\n baz\n+foo \n", + "origin": "local" + }, + { + "type": "unset", + "path": [ + { + "_key": "kA" + }, + "children", + { + "_key": "k2" + } + ], + "origin": "local" + }, + { + "type": "unset", + "path": [ + { + "_key": "kA" + }, + "children", + { + "_key": "k5" + } + ], + "origin": "local" + } + ], + "resultTerse": [ + "foo ,bar, bazfoo ,bar, baz" + ] +} diff --git a/packages/editor/tests/block-merge-duplicate-keys.test.tsx b/packages/editor/tests/block-merge-duplicate-keys.test.tsx new file mode 100644 index 0000000000..73ddde1502 --- /dev/null +++ b/packages/editor/tests/block-merge-duplicate-keys.test.tsx @@ -0,0 +1,416 @@ +import type {Patch} from '@portabletext/patches' +import {defineSchema, type PortableTextBlock} from '@portabletext/schema' +import {createTestKeyGenerator} from '@portabletext/test' +import {describe, expect, test, vi} from 'vitest' +import {userEvent} from 'vitest/browser' +import {EventListenerPlugin} from '../src/plugins' +import {createTestEditor} from '../src/test/vitest' + +function duplicateKeyedChildren() { + return [ + {_type: 'span', _key: 's1', text: 'foo ', marks: []}, + {_type: 'span', _key: 's2', text: 'bar', marks: ['strong']}, + {_type: 'span', _key: 's3', text: ' baz', marks: []}, + ] +} + +function duplicateKeyedInitialValue(): Array { + return [ + { + _type: 'block', + _key: 'kA', + children: duplicateKeyedChildren(), + markDefs: [], + style: 'normal', + }, + { + _type: 'block', + _key: 'kB', + children: duplicateKeyedChildren(), + markDefs: [], + style: 'normal', + }, + ] +} + +function duplicateKeyedChildrenWithLink(linkMarkDefKey: string) { + return [ + {_type: 'span', _key: 's1', text: 'foo ', marks: []}, + {_type: 'span', _key: 's2', text: 'bar', marks: ['strong', linkMarkDefKey]}, + {_type: 'span', _key: 's3', text: ' baz', marks: []}, + ] +} + +function duplicateKeyedInitialValueWithLink(): Array { + return [ + { + _type: 'block', + _key: 'kA', + children: duplicateKeyedChildrenWithLink('link1'), + markDefs: [{_type: 'link', _key: 'link1', href: 'https://a.example'}], + style: 'normal', + }, + { + _type: 'block', + _key: 'kB', + children: duplicateKeyedChildrenWithLink('link1'), + markDefs: [{_type: 'link', _key: 'link1', href: 'https://b.example'}], + style: 'normal', + }, + ] +} + +const duplicateKeyMergeSchema = defineSchema({decorators: [{name: 'strong'}]}) + +/** + * Backspace-merge `kB` into `kA` in a fresh editor over + * `duplicateKeyedInitialValue`, capturing every patch it emits. + */ +async function mergeDuplicateKeyedBlocks() { + const patches: Array = [] + + const {editor, locator} = await createTestEditor({ + keyGenerator: createTestKeyGenerator(), + schemaDefinition: duplicateKeyMergeSchema, + initialValue: duplicateKeyedInitialValue(), + children: ( + { + if (event.type === 'patch') { + patches.push(event.patch) + } + }} + /> + ), + }) + + await userEvent.click(locator) + + editor.send({ + type: 'select', + at: { + anchor: {path: [{_key: 'kB'}, 'children', {_key: 's1'}], offset: 0}, + focus: {path: [{_key: 'kB'}, 'children', {_key: 's1'}], offset: 0}, + }, + }) + + await vi.waitFor(() => { + expect(editor.getSnapshot().context.selection).toEqual({ + anchor: {path: [{_key: 'kB'}, 'children', {_key: 's1'}], offset: 0}, + focus: {path: [{_key: 'kB'}, 'children', {_key: 's1'}], offset: 0}, + backward: false, + }) + }) + + editor.send({type: 'delete.backward', unit: 'character'}) + + await vi.waitFor(() => { + expect(editor.getSnapshot().context.value.length).toBe(1) + }) + + return {editor, patches} +} + +describe('Feature: block merge renames colliding keys', () => { + test('Scenario: backspace-merging blocks whose children share keys renames instead of re-minting', async () => { + const {editor, patches} = await mergeDuplicateKeyedBlocks() + + await vi.waitFor(() => { + expect(editor.getSnapshot().context.value).toEqual([ + { + _type: 'block', + _key: 'kA', + children: [ + {_type: 'span', _key: 's1', text: 'foo ', marks: []}, + {_type: 'span', _key: 's2', text: 'bar', marks: ['strong']}, + {_type: 'span', _key: 's3', text: ' bazfoo ', marks: []}, + {_type: 'span', _key: 'k3', text: 'bar', marks: ['strong']}, + {_type: 'span', _key: 'k4', text: ' baz', marks: []}, + ], + markDefs: [], + style: 'normal', + }, + ]) + }) + + // The three colliding children get renamed with keyed `set` patches on + // `_key` before the merging block is unset, so a receiver applying the + // patches in order never sees a duplicate key and can follow each + // renamed child by its new key instead of losing it to a destroy/create. + const keySetPatches = patches.filter( + (patch) => patch.type === 'set' && patch.path.at(-1) === '_key', + ) + const unsetKbIndex = patches.findIndex( + (patch) => + patch.type === 'unset' && + patch.path.length === 1 && + typeof patch.path[0] === 'object' && + patch.path[0] !== null && + '_key' in patch.path[0] && + patch.path[0]._key === 'kB', + ) + + expect(keySetPatches).toEqual([ + { + type: 'set', + path: [{_key: 'kB'}, 'children', {_key: 's1'}, '_key'], + value: 'k2', + origin: 'local', + }, + { + type: 'set', + path: [{_key: 'kB'}, 'children', {_key: 's2'}, '_key'], + value: 'k3', + origin: 'local', + }, + { + type: 'set', + path: [{_key: 'kB'}, 'children', {_key: 's3'}, '_key'], + value: 'k4', + origin: 'local', + }, + ]) + expect(unsetKbIndex).toBeGreaterThan(-1) + for (const keySetPatch of keySetPatches) { + expect(patches.indexOf(keySetPatch)).toBeLessThan(unsetKbIndex) + } + + // Every child inserted back under `kA` carries a renamed key: none of + // the merging block's original `s1`/`s2`/`s3` keys reach the insert, so + // nothing collides with `kA`'s own children of the same name. + const insertedKeys = patches.flatMap((patch) => + patch.type === 'insert' + ? patch.items.flatMap((item) => + typeof item === 'object' && item !== null && '_key' in item + ? [item['_key']] + : [], + ) + : [], + ) + expect(insertedKeys).toEqual(['k5', 'k2', 'k3', 'k4']) + }) + + test('Scenario: undoing the merge restores both original blocks, every `_key` included', async () => { + const {editor} = await mergeDuplicateKeyedBlocks() + + const mergedValue: Array = [ + { + _type: 'block', + _key: 'kA', + children: [ + {_type: 'span', _key: 's1', text: 'foo ', marks: []}, + {_type: 'span', _key: 's2', text: 'bar', marks: ['strong']}, + {_type: 'span', _key: 's3', text: ' bazfoo ', marks: []}, + {_type: 'span', _key: 'k3', text: 'bar', marks: ['strong']}, + {_type: 'span', _key: 'k4', text: ' baz', marks: []}, + ], + markDefs: [], + style: 'normal', + }, + ] + + expect(editor.getSnapshot().context.value).toEqual(mergedValue) + + editor.send({type: 'history.undo'}) + + await vi.waitFor(() => { + expect(editor.getSnapshot().context.value).toEqual( + duplicateKeyedInitialValue(), + ) + }) + + editor.send({type: 'history.redo'}) + + await vi.waitFor(() => { + expect(editor.getSnapshot().context.value).toEqual(mergedValue) + }) + + editor.send({type: 'history.undo'}) + + await vi.waitFor(() => { + expect(editor.getSnapshot().context.value).toEqual( + duplicateKeyedInitialValue(), + ) + }) + }) + + test("Scenario: a remote receiver's caret follows the renamed, reinserted child through the merge", async () => { + const {patches} = await mergeDuplicateKeyedBlocks() + + // Editor 2 starts from the same duplicate-keyed document and receives + // editor 1's patches as if they came over the wire. + const {editor: editor2, locator: locator2} = await createTestEditor({ + keyGenerator: createTestKeyGenerator(), + schemaDefinition: duplicateKeyMergeSchema, + initialValue: duplicateKeyedInitialValue(), + }) + + await userEvent.click(locator2) + + editor2.send({ + type: 'select', + at: { + anchor: {path: [{_key: 'kB'}, 'children', {_key: 's3'}], offset: 3}, + focus: {path: [{_key: 'kB'}, 'children', {_key: 's3'}], offset: 3}, + }, + }) + + await vi.waitFor(() => { + expect(editor2.getSnapshot().context.selection).toEqual({ + anchor: {path: [{_key: 'kB'}, 'children', {_key: 's3'}], offset: 3}, + focus: {path: [{_key: 'kB'}, 'children', {_key: 's3'}], offset: 3}, + backward: false, + }) + }) + + editor2.send({ + type: 'patches', + patches: patches.map((patch) => ({...patch, origin: 'remote'})), + snapshot: undefined, + }) + + await vi.waitFor(() => { + expect(editor2.getSnapshot().context.value).toEqual([ + { + _type: 'block', + _key: 'kA', + children: [ + {_type: 'span', _key: 's1', text: 'foo ', marks: []}, + {_type: 'span', _key: 's2', text: 'bar', marks: ['strong']}, + {_type: 'span', _key: 's3', text: ' bazfoo ', marks: []}, + {_type: 'span', _key: 'k3', text: 'bar', marks: ['strong']}, + {_type: 'span', _key: 'k4', text: ' baz', marks: []}, + ], + markDefs: [], + style: 'normal', + }, + ]) + }) + + // The caret sat in kB's third span (" baz") at offset 3. That span + // reappears, renamed to `k4`, as the merged block's last child, so the + // caret follows it there at the same offset instead of dangling on the + // now-deleted `kB`. + await vi.waitFor(() => { + expect(editor2.getSnapshot().context.selection).toEqual({ + anchor: {path: [{_key: 'kA'}, 'children', {_key: 'k4'}], offset: 3}, + focus: {path: [{_key: 'kA'}, 'children', {_key: 'k4'}], offset: 3}, + backward: false, + }) + }) + }) + + test('Scenario: a colliding markDef is renamed and every mark referencing it is rewritten before the merge', async () => { + const schemaDefinition = defineSchema({ + decorators: [{name: 'strong'}], + annotations: [{name: 'link', fields: [{name: 'href', type: 'string'}]}], + }) + + const patches: Array = [] + + const {editor: editor1, locator: locator1} = await createTestEditor({ + keyGenerator: createTestKeyGenerator(), + schemaDefinition, + initialValue: duplicateKeyedInitialValueWithLink(), + children: ( + { + if (event.type === 'patch') { + patches.push(event.patch) + } + }} + /> + ), + }) + + await userEvent.click(locator1) + + editor1.send({ + type: 'select', + at: { + anchor: {path: [{_key: 'kB'}, 'children', {_key: 's1'}], offset: 0}, + focus: {path: [{_key: 'kB'}, 'children', {_key: 's1'}], offset: 0}, + }, + }) + + await vi.waitFor(() => { + expect(editor1.getSnapshot().context.selection).not.toBeNull() + }) + + editor1.send({type: 'delete.backward', unit: 'character'}) + + await vi.waitFor(() => { + expect(editor1.getSnapshot().context.value).toEqual([ + { + _type: 'block', + _key: 'kA', + children: [ + {_type: 'span', _key: 's1', text: 'foo ', marks: []}, + { + _type: 'span', + _key: 's2', + text: 'bar', + marks: ['strong', 'link1'], + }, + {_type: 'span', _key: 's3', text: ' bazfoo ', marks: []}, + {_type: 'span', _key: 'k4', text: 'bar', marks: ['strong', 'k2']}, + {_type: 'span', _key: 'k5', text: ' baz', marks: []}, + ], + markDefs: [ + {_type: 'link', _key: 'link1', href: 'https://a.example'}, + {_type: 'link', _key: 'k2', href: 'https://b.example'}, + ], + style: 'normal', + }, + ]) + }) + + // `kB`'s markDef collides with `kA`'s own `link1`, so it's renamed + // ahead of the merge, same as a colliding child key, before `kB` is + // unset. + const markDefKeySetPatch = patches.find( + (patch) => + patch.type === 'set' && + patch.path.at(-1) === '_key' && + patch.path.at(-3) === 'markDefs', + ) + expect(markDefKeySetPatch).toEqual({ + type: 'set', + path: [{_key: 'kB'}, 'markDefs', {_key: 'link1'}, '_key'], + value: 'k2', + origin: 'local', + }) + + // `s2` carries the renamed markDef in its `marks`, so the rename + // rewrites that reference too, not just the markDef's own `_key`. + const marksRewritePatch = patches.find( + (patch) => patch.type === 'set' && patch.path.at(-1) === 'marks', + ) + expect(marksRewritePatch).toEqual({ + type: 'set', + path: [{_key: 'kB'}, 'children', {_key: 'k4'}, 'marks'], + value: ['strong', 'k2'], + origin: 'local', + }) + + // Editor 2 starts from the same duplicate-markDef-keyed document and + // receives editor 1's patches as if they came over the wire. + const {editor: editor2} = await createTestEditor({ + keyGenerator: createTestKeyGenerator(), + schemaDefinition, + initialValue: duplicateKeyedInitialValueWithLink(), + }) + + editor2.send({ + type: 'patches', + patches: patches.map((patch) => ({...patch, origin: 'remote'})), + snapshot: undefined, + }) + + await vi.waitFor(() => { + expect(editor2.getSnapshot().context.value).toEqual( + editor1.getSnapshot().context.value, + ) + }) + }) +}) diff --git a/packages/editor/tests/wire-catalogue.test.tsx b/packages/editor/tests/wire-catalogue.test.tsx index 245b398ce3..48a5220ad0 100644 --- a/packages/editor/tests/wire-catalogue.test.tsx +++ b/packages/editor/tests/wire-catalogue.test.tsx @@ -205,6 +205,67 @@ describe('wire catalogue', () => { }) }) + test('Scenario: merging two blocks whose children share keys renames before the merge', async () => { + // Duplicate `_key`s across the two blocks: textspec mints keys itself + // and can't express a collision, so this seed is hand-built. See the + // file header. + const keyGenerator = createTestKeyGenerator() + const schemaDefinition = defineSchema({decorators: [{name: 'strong'}]}) + const duplicateKeyedChildren = (): Array => [ + {_type: 'span', _key: 's1', text: 'foo ', marks: []}, + {_type: 'span', _key: 's2', text: 'bar', marks: ['strong']}, + {_type: 'span', _key: 's3', text: ' baz', marks: []}, + ] + const seed: Array> = [ + { + _type: 'block', + _key: 'kA', + children: duplicateKeyedChildren(), + markDefs: [], + style: 'normal', + }, + { + _type: 'block', + _key: 'kB', + children: duplicateKeyedChildren(), + markDefs: [], + style: 'normal', + }, + ] + + const {editor, patches, schema, seedTerse} = await setupLegacyScenario({ + keyGenerator, + schemaDefinition, + initialValue: seed, + }) + + editor.send({ + type: 'select', + at: { + anchor: {path: [{_key: 'kB'}, 'children', {_key: 's1'}], offset: 0}, + focus: {path: [{_key: 'kB'}, 'children', {_key: 's1'}], offset: 0}, + }, + }) + editor.send({type: 'delete.backward', unit: 'character'}) + + await vi.waitFor(() => { + expect(editor.getSnapshot().context.value.length).toBe(1) + }) + + await writeCapture({ + scenario: 'block-merge-duplicate-keys', + schema, + seed, + seedTerse, + actions: [ + 'select {caret at start of block 2}', + "send {type: 'delete.backward', unit: 'character'}", + ], + patches, + resultTerse: getTersePt(editor.getSnapshot().context), + }) + }) + test('Scenario: adding a decorator mid-span', async () => { const keyGenerator = createTestKeyGenerator() const seed = 'B: foo ^bar| baz'