fix: rename colliding keys before backspace merges a block into its predecessor

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

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

@@ -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)
   })
 })

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': {