From 546b7925ed72af7d4f97b5d449414f9aad15879c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20Hamburger=20Gr=C3=B8ngaard?= Date: Fri, 2 Oct 2026 18:11:22 +0200 Subject: [PATCH] fix: keep same-key siblings indexed when a duplicate `_key` is renamed `update value` syncs blocks by position, so removing a block above the caret replaces each later block with the content of the block after it. For a moment the replaced block and the not-yet-replaced block below it share a `_key`, and the duplicate-key normalizer renames the second one with a `set` on `_key`. `handleKeyChange` then pruned the map entries under the old keyed path, which the surviving sibling shares, and re-added only the renamed node. Every shifted block lost its `blockIndexMap` entry along with its spans' entries. A later collapsed `delete.backward` compared paths against the missing entry (`comparePathsInTree` reads it as -1), `before()` found no position, and `deleteCollapsed` fell back to the editor start: one Backspace deleted everything from the top of the document to the caret. After a rename, `handleKeyChange` now rebuilds the entries of every sibling carrying the old or the new key, first occurrence wins, which is what `buildIndexMaps` produces for the same value. This holds for root blocks, spans and container children, and for three or more siblings sharing a key. Pinned by map tests with full expected maps (red on the old transform), an `update value` plus Backspace test, and a remote patch batch that renames one of three same-key blocks and then unsets by key, which guards against restoring the siblings last-wins. Only renames are covered. Inserting, removing or replacing a node while its key is duplicated among its siblings can still leave the map different from a fresh build, as before. --- .../block-index-map-duplicate-key-rename.md | 7 + .../transform-block-index-map.test.ts | 252 ++++++++++++++++++ .../transform-block-index-map.ts | 103 ++++++- packages/editor/tests/event.patches.test.tsx | 83 ++++++ .../editor/tests/event.update-value.test.tsx | 102 +++++++ 5 files changed, 534 insertions(+), 13 deletions(-) create mode 100644 .changeset/block-index-map-duplicate-key-rename.md diff --git a/.changeset/block-index-map-duplicate-key-rename.md b/.changeset/block-index-map-duplicate-key-rename.md new file mode 100644 index 0000000000..46ea9ad6ff --- /dev/null +++ b/.changeset/block-index-map-duplicate-key-rename.md @@ -0,0 +1,7 @@ +--- +'@portabletext/editor': patch +--- + +fix: keep same-key siblings indexed when a duplicate `_key` is renamed + +Pressing Backspace after the editor receives a new value that removes a block above the caret no longer deletes everything from the start of the document to the caret. For example, with the blocks "foo", "bar", "baz" and "qux", syncing a value without "bar" and then pressing Backspace at the end of "baz" now leaves "foo", "ba" and "qux". Previously it left a single empty block followed by "qux". diff --git a/packages/editor/src/internal-utils/transform-block-index-map.test.ts b/packages/editor/src/internal-utils/transform-block-index-map.test.ts index 5d94977f43..d32761ceae 100644 --- a/packages/editor/src/internal-utils/transform-block-index-map.test.ts +++ b/packages/editor/src/internal-utils/transform-block-index-map.test.ts @@ -3,6 +3,7 @@ import { defineSchema, type PortableTextBlock, } from '@portabletext/schema' +import {createTestKeyGenerator} from '@portabletext/test' import {describe, expect, test} from 'vitest' import type {EngineOperation} from '../engine/interfaces/operation' import {defineContainer} from '../renderers/renderer.types' @@ -586,6 +587,257 @@ describe('transformBlockIndexMap', () => { }) }) + test('renaming the second of two root blocks sharing a _key keeps the first block and its spans indexed', () => { + const keyGenerator = createTestKeyGenerator() + const blockKey = keyGenerator() + const spanKey = keyGenerator() + const newBlockKey = keyGenerator() + const makeValue = (secondKey: string) => [ + { + _key: blockKey, + _type: 'block', + children: [{_key: spanKey, _type: 'span', text: 'foo'}], + } as PortableTextBlock, + { + _key: secondKey, + _type: 'block', + children: [{_key: spanKey, _type: 'span', text: 'bar'}], + } as PortableTextBlock, + ] + const beforeValue = makeValue(blockKey) + const afterValue = makeValue(newBlockKey) + const map = new BlockIndexMap() + buildIndexMaps( + {schema: tableSchema, value: beforeValue, containers: tableContainers}, + {blockIndexMap: map}, + ) + transformBlockIndexMap( + map, + { + type: 'set', + path: [1, '_key'], + value: newBlockKey, + inverse: {type: 'set', path: [1, '_key'], value: blockKey}, + }, + beforeValue, + afterValue, + {schema: tableSchema, containers: tableContainers}, + ) + + expect(Object.fromEntries([...map].sort())).toEqual({ + '[_key=="k0"]': 0, + '[_key=="k0"].children[_key=="k1"]': 0, + '[_key=="k2"]': 1, + '[_key=="k2"].children[_key=="k1"]': 0, + }) + }) + + test('renaming the middle of three root blocks sharing a _key indexes the first block and every span only a later block has', () => { + const keyGenerator = createTestKeyGenerator() + const blockKey = keyGenerator() + const fooSpanKey = keyGenerator() + const barSpanKey = keyGenerator() + const newBlockKey = keyGenerator() + const makeValue = (middleKey: string) => [ + { + _key: blockKey, + _type: 'block', + children: [{_key: fooSpanKey, _type: 'span', text: 'foo'}], + } as PortableTextBlock, + { + _key: middleKey, + _type: 'block', + children: [{_key: barSpanKey, _type: 'span', text: 'bar'}], + } as PortableTextBlock, + { + _key: blockKey, + _type: 'block', + children: [ + {_key: fooSpanKey, _type: 'span', text: 'baz'}, + {_key: barSpanKey, _type: 'span', text: 'qux'}, + ], + } as PortableTextBlock, + ] + const beforeValue = makeValue(blockKey) + const afterValue = makeValue(newBlockKey) + const map = new BlockIndexMap() + buildIndexMaps( + {schema: tableSchema, value: beforeValue, containers: tableContainers}, + {blockIndexMap: map}, + ) + transformBlockIndexMap( + map, + { + type: 'set', + path: [1, '_key'], + value: newBlockKey, + inverse: {type: 'set', path: [1, '_key'], value: blockKey}, + }, + beforeValue, + afterValue, + {schema: tableSchema, containers: tableContainers}, + ) + + expect(Object.fromEntries([...map].sort())).toEqual({ + '[_key=="k0"]': 0, + '[_key=="k0"].children[_key=="k1"]': 0, + '[_key=="k0"].children[_key=="k2"]': 1, + '[_key=="k3"]': 1, + '[_key=="k3"].children[_key=="k2"]': 0, + }) + }) + + test("renaming a root block to a later sibling's _key indexes the renamed block's spans", () => { + const keyGenerator = createTestKeyGenerator() + const blockKey = keyGenerator() + const fooSpanKey = keyGenerator() + const laterBlockKey = keyGenerator() + const barSpanKey = keyGenerator() + const makeValue = (firstKey: string) => [ + { + _key: firstKey, + _type: 'block', + children: [{_key: fooSpanKey, _type: 'span', text: 'foo'}], + } as PortableTextBlock, + { + _key: laterBlockKey, + _type: 'block', + children: [ + {_key: barSpanKey, _type: 'span', text: 'bar'}, + {_key: fooSpanKey, _type: 'span', text: 'baz'}, + ], + } as PortableTextBlock, + ] + const beforeValue = makeValue(blockKey) + const afterValue = makeValue(laterBlockKey) + const map = new BlockIndexMap() + buildIndexMaps( + {schema: tableSchema, value: beforeValue, containers: tableContainers}, + {blockIndexMap: map}, + ) + transformBlockIndexMap( + map, + { + type: 'set', + path: [0, '_key'], + value: laterBlockKey, + inverse: {type: 'set', path: [0, '_key'], value: blockKey}, + }, + beforeValue, + afterValue, + {schema: tableSchema, containers: tableContainers}, + ) + + expect(Object.fromEntries([...map].sort())).toEqual({ + '[_key=="k2"]': 0, + '[_key=="k2"].children[_key=="k1"]': 0, + '[_key=="k2"].children[_key=="k3"]': 0, + }) + }) + + test('renaming the second of two spans sharing a _key keeps the first span indexed', () => { + const keyGenerator = createTestKeyGenerator() + const blockKey = keyGenerator() + const spanKey = keyGenerator() + const newSpanKey = keyGenerator() + const makeValue = (secondKey: string) => [ + { + _key: blockKey, + _type: 'block', + children: [ + {_key: spanKey, _type: 'span', text: 'foo'}, + {_key: secondKey, _type: 'span', text: 'bar'}, + ], + } as PortableTextBlock, + ] + const beforeValue = makeValue(spanKey) + const afterValue = makeValue(newSpanKey) + const map = new BlockIndexMap() + buildIndexMaps( + {schema: tableSchema, value: beforeValue, containers: tableContainers}, + {blockIndexMap: map}, + ) + transformBlockIndexMap( + map, + { + type: 'set', + path: [{_key: blockKey}, 'children', 1, '_key'], + value: newSpanKey, + inverse: { + type: 'set', + path: [{_key: blockKey}, 'children', 1, '_key'], + value: spanKey, + }, + }, + beforeValue, + afterValue, + {schema: tableSchema, containers: tableContainers}, + ) + + expect(Object.fromEntries([...map].sort())).toEqual({ + '[_key=="k0"]': 0, + '[_key=="k0"].children[_key=="k1"]': 0, + '[_key=="k0"].children[_key=="k2"]': 1, + }) + }) + + test('renaming the second of two rows sharing a _key keeps the first row and its cells indexed', () => { + const keyGenerator = createTestKeyGenerator() + const tableKey = keyGenerator() + const rowKey = keyGenerator() + const cellKey = keyGenerator() + const newRowKey = keyGenerator() + const makeValue = (secondKey: string) => [ + { + _key: tableKey, + _type: 'table', + rows: [ + { + _key: rowKey, + _type: 'row', + cells: [{_key: cellKey, _type: 'cell', content: []}], + }, + { + _key: secondKey, + _type: 'row', + cells: [{_key: cellKey, _type: 'cell', content: []}], + }, + ], + } as unknown as PortableTextBlock, + ] + const beforeValue = makeValue(rowKey) + const afterValue = makeValue(newRowKey) + const map = new BlockIndexMap() + buildIndexMaps( + {schema: tableSchema, value: beforeValue, containers: tableContainers}, + {blockIndexMap: map}, + ) + transformBlockIndexMap( + map, + { + type: 'set', + path: [{_key: tableKey}, 'rows', 1, '_key'], + value: newRowKey, + inverse: { + type: 'set', + path: [{_key: tableKey}, 'rows', 1, '_key'], + value: rowKey, + }, + }, + beforeValue, + afterValue, + {schema: tableSchema, containers: tableContainers}, + ) + + expect(Object.fromEntries([...map].sort())).toEqual({ + '[_key=="k0"]': 0, + '[_key=="k0"].rows[_key=="k1"]': 0, + '[_key=="k0"].rows[_key=="k1"].cells[_key=="k2"]': 0, + '[_key=="k0"].rows[_key=="k3"]': 1, + '[_key=="k0"].rows[_key=="k3"].cells[_key=="k2"]': 0, + }) + }) + test('unset of a container property prunes its descendants', () => { const before = [ { diff --git a/packages/editor/src/internal-utils/transform-block-index-map.ts b/packages/editor/src/internal-utils/transform-block-index-map.ts index 264dde868f..b61fe39f40 100644 --- a/packages/editor/src/internal-utils/transform-block-index-map.ts +++ b/packages/editor/src/internal-utils/transform-block-index-map.ts @@ -242,6 +242,12 @@ function resolveChildIndexInValue( return -1 } +type SiblingContext = { + children: ReadonlyArray + keyedPrefix: Path + serializedPrefix: string +} + /** * Resolve the sibling array an op path points into, together with the * serialized path prefix shared by every sibling's map key. Returns @@ -253,14 +259,9 @@ function resolveSiblingContext( context: PureTransformContext, value: ReadonlyArray, opPath: Path, -): - | { - children: ReadonlyArray - serializedPrefix: string - } - | undefined { +): SiblingContext | undefined { if (opPath.length === 1) { - return {children: value, serializedPrefix: ''} + return {children: value, keyedPrefix: [], serializedPrefix: ''} } const fieldSegment = opPath[opPath.length - 2] if (typeof fieldSegment !== 'string') { @@ -282,9 +283,11 @@ function resolveSiblingContext( if (!childrenResult || childrenResult.fieldName !== fieldSegment) { return undefined } + const keyedPrefix: Path = [...keyedParentPath, fieldSegment] return { children: childrenResult.children, - serializedPrefix: serializePath([...keyedParentPath, fieldSegment]), + keyedPrefix, + serializedPrefix: serializePath(keyedPrefix), } } @@ -298,10 +301,7 @@ function resolveSiblingContext( */ function reindexSiblings( map: BlockIndexMap, - siblingContext: { - children: ReadonlyArray - serializedPrefix: string - }, + siblingContext: SiblingContext, startIndex: number, ): void { for ( @@ -564,7 +564,84 @@ function handleKeyChange( } const parentSegments = nodePath.slice(0, -1) const newKeyedNodePath: Path = [...parentSegments, childIndex] - addSubtree(map, context, afterValue, newKeyedNodePath) + rebuildEntriesForSiblingsWithKeys( + map, + context, + afterValue, + newKeyedNodePath, + [ + resolveNodeAtPath(beforeValue, nodePath)?._key, + resolveNodeAtPath(afterValue, newKeyedNodePath)?._key, + ], + ) +} + +function rebuildEntriesForSiblingsWithKeys( + map: BlockIndexMap, + context: PureTransformContext, + afterValue: ReadonlyArray, + nodePath: Path, + keys: ReadonlyArray, +): void { + const siblingContext = resolveSiblingContext(context, afterValue, nodePath) + if (!siblingContext) { + return + } + for (const key of new Set(keys)) { + if (key !== undefined) { + rebuildEntriesForSiblingsWithKey( + map, + context, + afterValue, + siblingContext, + key, + ) + } + } +} + +function rebuildEntriesForSiblingsWithKey( + map: BlockIndexMap, + context: PureTransformContext, + afterValue: ReadonlyArray, + siblingContext: SiblingContext, + key: string, +): void { + const siblingPath: Path = [...siblingContext.keyedPrefix, {_key: key}] + const siblingEntryKey = serializePath(siblingPath) + const siblingIndexes: Array = [] + siblingContext.children.forEach((sibling, index) => { + if (sibling._key === key) { + siblingIndexes.push(index) + } + }) + map.delete(siblingEntryKey) + for (const index of siblingIndexes) { + walkKeyedChildrenInValue( + siblingContext.children[index]!, + siblingPath, + (childPath) => { + map.delete(serializePath(childPath)) + }, + ) + } + const resolved = + siblingIndexes.length > 0 + ? resolveIndexableNode(context, afterValue, siblingPath) + : undefined + if (!resolved) { + return + } + map.set(siblingEntryKey, siblingIndexes[0]!) + for (const index of siblingIndexes) { + collectDescendantIndexes( + context, + siblingContext.children[index]!, + siblingPath, + resolved.containerOfParent, + map, + ) + } } /** diff --git a/packages/editor/tests/event.patches.test.tsx b/packages/editor/tests/event.patches.test.tsx index 861f215741..80d9a76512 100644 --- a/packages/editor/tests/event.patches.test.tsx +++ b/packages/editor/tests/event.patches.test.tsx @@ -688,6 +688,89 @@ describe('event.patches', () => { }) }) + test('Scenario: `unset` by a `_key` shared by sibling blocks after renaming the middle one', async () => { + const keyGenerator = createTestKeyGenerator() + const fooBlockKey = keyGenerator() + const fooSpanKey = keyGenerator() + const barBlockKey = keyGenerator() + const barSpanKey = keyGenerator() + const bazBlockKey = keyGenerator() + const bazSpanKey = keyGenerator() + const {editor} = await createTestEditor({ + keyGenerator, + initialValue: [ + { + _type: 'block', + _key: fooBlockKey, + children: [{_type: 'span', _key: fooSpanKey, text: 'foo', marks: []}], + markDefs: [], + style: 'normal', + }, + { + _type: 'block', + _key: barBlockKey, + children: [{_type: 'span', _key: barSpanKey, text: 'bar', marks: []}], + markDefs: [], + style: 'normal', + }, + { + _type: 'block', + _key: bazBlockKey, + children: [{_type: 'span', _key: bazSpanKey, text: 'baz', marks: []}], + markDefs: [], + style: 'normal', + }, + ], + }) + + const renamedBlockKey = keyGenerator() + + editor.send({ + type: 'patches', + patches: [ + { + type: 'set', + origin: 'remote', + path: [{_key: barBlockKey}, '_key'], + value: fooBlockKey, + }, + { + type: 'set', + origin: 'remote', + path: [{_key: bazBlockKey}, '_key'], + value: fooBlockKey, + }, + { + type: 'set', + origin: 'remote', + path: [1, '_key'], + value: renamedBlockKey, + }, + {type: 'unset', origin: 'remote', path: [{_key: fooBlockKey}]}, + ], + snapshot: undefined, + }) + + await vi.waitFor(() => { + expect(editor.getSnapshot().context.value).toEqual([ + { + _type: 'block', + _key: renamedBlockKey, + children: [{_type: 'span', _key: barSpanKey, text: 'bar', marks: []}], + markDefs: [], + style: 'normal', + }, + { + _type: 'block', + _key: fooBlockKey, + children: [{_type: 'span', _key: bazSpanKey, text: 'baz', marks: []}], + markDefs: [], + style: 'normal', + }, + ]) + }) + }) + test('Scenario: `set` block object key', async () => { const keyGenerator = createTestKeyGenerator() const imageKey = keyGenerator() diff --git a/packages/editor/tests/event.update-value.test.tsx b/packages/editor/tests/event.update-value.test.tsx index 89cf81c32b..23b7a01de9 100644 --- a/packages/editor/tests/event.update-value.test.tsx +++ b/packages/editor/tests/event.update-value.test.tsx @@ -1957,6 +1957,108 @@ describe('event.update value', () => { }) }) + test('Scenario: Deleting backward after a remote update removed a block above the caret', async () => { + const keyGenerator = createTestKeyGenerator() + const fooBlockKey = keyGenerator() + const fooSpanKey = keyGenerator() + const barBlockKey = keyGenerator() + const barSpanKey = keyGenerator() + const bazBlockKey = keyGenerator() + const bazSpanKey = keyGenerator() + const quxBlockKey = keyGenerator() + const quxSpanKey = keyGenerator() + + const fooBlock = { + _type: 'block', + _key: fooBlockKey, + children: [{_type: 'span', _key: fooSpanKey, text: 'foo', marks: []}], + markDefs: [], + style: 'normal', + } + const barBlock = { + _type: 'block', + _key: barBlockKey, + children: [{_type: 'span', _key: barSpanKey, text: 'bar', marks: []}], + markDefs: [], + style: 'normal', + } + const bazBlock = { + _type: 'block', + _key: bazBlockKey, + children: [{_type: 'span', _key: bazSpanKey, text: 'baz', marks: []}], + markDefs: [], + style: 'normal', + } + const quxBlock = { + _type: 'block', + _key: quxBlockKey, + children: [{_type: 'span', _key: quxSpanKey, text: 'qux', marks: []}], + markDefs: [], + style: 'normal', + } + + const {editor} = await createTestEditor({ + keyGenerator, + schemaDefinition: defineSchema({}), + initialValue: [fooBlock, barBlock, bazBlock, quxBlock], + }) + + const endOfBaz = { + anchor: { + path: [{_key: bazBlockKey}, 'children', {_key: bazSpanKey}], + offset: 3, + }, + focus: { + path: [{_key: bazBlockKey}, 'children', {_key: bazSpanKey}], + offset: 3, + }, + } + + editor.send({type: 'select', at: endOfBaz}) + + await vi.waitFor(() => { + expect(editor.getSnapshot().context.selection).toEqual({ + ...endOfBaz, + backward: false, + }) + }) + + editor.send({ + type: 'update value', + value: [fooBlock, bazBlock, quxBlock], + }) + + await vi.waitFor(() => { + expect(editor.getSnapshot().context.value).toEqual([ + fooBlock, + bazBlock, + quxBlock, + ]) + }) + + editor.send({type: 'select', at: endOfBaz}) + + await vi.waitFor(() => { + expect(editor.getSnapshot().context.selection).toEqual({ + ...endOfBaz, + backward: false, + }) + }) + + editor.send({type: 'delete.backward', unit: 'character'}) + + await vi.waitFor(() => { + expect(editor.getSnapshot().context.value).toEqual([ + fooBlock, + { + ...bazBlock, + children: [{_type: 'span', _key: bazSpanKey, text: 'ba', marks: []}], + }, + quxBlock, + ]) + }) + }) + test("Scenario: Updating an inline object's `text` field", async () => { const schemaDefinition = defineSchema({ inlineObjects: [