fix: rename colliding keys before a text block merge deletes the merging block

.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.

.changeset/rekey-point-transform-path-scoped.md

@@ -0,0 +1,7 @@
+---
+'@portabletext/editor': patch
+---
+
+fix: re-point positions only at the renamed node's own path when a `_key` changes
+
+Renaming a node's `_key` no longer drags carets, selections, or tracked ranges sitting on an unrelated node that happens to carry the same key (keys are only unique among siblings, so twins across blocks are legal). Positions on the renamed node itself follow the rename as before; everything else stays put.

.changeset/rename-colliding-keys-before-merge.md

@@ -0,0 +1,7 @@
+---
+'@portabletext/editor': patch
+---
+
+fix: rename colliding keys before a text block merge deletes the merging block
+
+Merging a text block into its neighbor (backspace at its start, or forward delete at the end of the block before it) 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 emitted patches express the rename followed by the move instead of a destroy-and-create.

packages/editor/src/behaviors/behavior.abstract.delete.ts

@@ -1,5 +1,11 @@
-import {isSpan, isTextBlock} from '@portabletext/schema'
-import {isTextBlockNode} from '../engine/node/is-text-block-node'
+import {
+  isSpan,
+  isTextBlock,
+  type PortableTextTextBlock,
+  type Schema,
+} from '@portabletext/schema'
+import type {Path} from '../engine/interfaces/path'
+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 +15,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 +89,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 +122,7 @@ export const abstractDeleteBehaviors = [
         }),
         raise({
           type: 'insert.block',
-          block: focusTextBlock.node,
+          block: renamedBlock,
           placement: 'auto',
           select: 'start',
         }),
@@ -231,21 +253,33 @@ export const abstractDeleteBehaviors = [
         return false
       }
 
-      if (!isTextBlockNode(snapshot.context, nextSibling.node)) {
+      if (!isTextBlock(snapshot.context, nextSibling.node)) {
         return false
       }
 
-      return {nextBlock: {node: nextSibling.node, path: nextSibling.path}}
+      const {renamedBlock, renameActions} = planMergeKeyRenames({
+        context: snapshot.context,
+        mergingBlockPath: nextSibling.path,
+        mergingBlock: nextSibling.node,
+        destinationBlock: focusTextBlock.node,
+      })
+
+      return {
+        nextBlockPath: nextSibling.path,
+        renamedBlock,
+        renameActions,
+      }
     },
     actions: [
-      (_, {nextBlock}) => [
+      (_, {nextBlockPath, renamedBlock, renameActions}) => [
+        ...renameActions,
         raise({
           type: 'delete.block',
-          at: nextBlock.path,
+          at: nextBlockPath,
         }),
         raise({
           type: 'insert.block',
-          block: nextBlock.node,
+          block: renamedBlock,
           placement: 'auto',
           select: 'none',
         }),
@@ -332,3 +366,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<BehaviorAction>
+} {
+  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<string, string>()
+  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<BehaviorAction> = []
+
+  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<string, unknown> = {}
+    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,
+  }
+}

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

packages/editor/src/engine/point/step-mapper.test.ts

@@ -300,26 +300,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', () => {
@@ -733,7 +773,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'},
@@ -808,7 +853,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'}],
@@ -823,7 +873,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)
   })
@@ -837,7 +892,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)
   })
 })