fix: dedupe identical same-key markDefs on every block merge

.changeset/dedupe-identical-markdefs.md

@@ -0,0 +1,7 @@
+---
+'@portabletext/editor': patch
+---
+
+fix: dedupe identical same-key markDefs on every block merge
+
+Splitting a block through the middle of an annotation and merging it back together rejoins the annotation into a single definition, instead of leaving two identical definitions under different keys. Backspace and forward delete at a block boundary get the same treatment: an annotation definition arriving in a block that already holds an identical one under the same `_key` keeps its key instead of being renamed, so anything tracking the annotation keeps following it.

.changeset/identical-markdefs-non-conflicts.md

@@ -0,0 +1,7 @@
+---
+'@portabletext/editor': patch
+---
+
+fix: treat identical same-key markDefs as non-conflicts when inserting text block fragments
+
+Inserting a text block into another (pasting into a block, for example) no longer renames an annotation definition when the destination already carries an identical one under the same `_key`. The definition keeps its key, so anything tracking the annotation (comments, decorations) keeps following it.

packages/editor/gherkin-spec/annotations-across-blocks.feature

@@ -109,9 +109,6 @@ Feature: Annotations Across Blocks
       B:  baz
       """
 
-  # Warning: Possible wrong behaviour
-  # "foo" and "bar" should rejoin as one link
-  # Fixing this is possibly a breaking change
   Scenario: Splitting and merging an annotation across blocks
     Given the editor state is "B: foobar"
     And a "link" "l1" around "foobar"
@@ -121,7 +118,7 @@ Feature: Annotations Across Blocks
     And "{Backspace}" is pressed
     Then the editor state is
       """
-      B: [@link _key="l1":foo][@link _key="l2":bar]
+      B: [@link _key="l1":foobar]
       """
 
   # Warning: Possible wrong behaviour

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

@@ -387,10 +387,6 @@ function planMergeKeyRenameActions(args: {
     context,
     mergingBlock,
     destinationBlock,
-    // `renamedBlock` feeds `insert.block`, which parses the block
-    // standalone and strips any mark that doesn't resolve in its own
-    // `markDefs`, so a deduped def's spans would lose the annotation.
-    dedupeEqualMarkDefs: false,
   })
 
   const renameActions: Array<BehaviorAction> = []

packages/editor/src/internal-utils/delete-internal.ts

@@ -766,12 +766,12 @@ function mergeBlock(
   // The span holding the range's start point survives as an empty span
   // until normalization runs, so it still occupies `startBlock`'s children
   // and is a genuine collision source the plan must include.
-  const {renamedBlock, childRenames, markDefRenames} = planMergeKeyRenames({
-    context: editor.snapshot.context,
-    mergingBlock: endBlock.node,
-    destinationBlock: startBlock.node,
-    dedupeEqualMarkDefs: true,
-  })
+  const {renamedBlock, childRenames, markDefRenames, dedupedMarkDefKeys} =
+    planMergeKeyRenames({
+      context: editor.snapshot.context,
+      mergingBlock: endBlock.node,
+      destinationBlock: startBlock.node,
+    })
 
   for (const {markDefKey, newKey} of markDefRenames) {
     // `getNode` can't resolve a keyed descent into `markDefs` (it's a
@@ -791,7 +791,9 @@ function mergeBlock(
     ])
   }
 
-  const endMarkDefs = renamedBlock.markDefs
+  const endMarkDefs = renamedBlock.markDefs?.filter(
+    (markDef) => !dedupedMarkDefKeys.includes(markDef._key),
+  )
   if (Array.isArray(endMarkDefs) && endMarkDefs.length > 0) {
     const oldDefs = startBlock.node.markDefs ?? []
     setNodeProperties(

packages/editor/src/internal-utils/equality.ts

@@ -264,7 +264,7 @@ function isEqualPortableTextObjects(
 /**
  * More or less copied from Remeda (https://github.com/remeda/remeda/blob/main/packages/remeda/src/isDeepEqual.ts)
  */
-export function isDeepEqual<A, B>(data: A, other: B) {
+export function isDeepEqual<A, B>(data: A, other: B): boolean {
   return isDeepEqualImplementation(data, other)
 }
 

packages/editor/src/internal-utils/plan-merge-key-renames.ts

@@ -27,11 +27,21 @@ export type MergeMarkDefRename = {
  * `destinationBlock` by key. Any child key the merging block shares with
  * the destination has to be renamed before the merge, or the engine'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. There
- * is no such backstop for markDefs: an unrenamed collision either loses
- * one of the two defs to last-wins replacement, or, on the `insert.block`
- * path, gets silently re-minted by `adjustFragmentKeys` (see
- * `dedupeEqualMarkDefs` below).
+ * that node being destroyed and a new one created instead of moved.
+ *
+ * A colliding markDef that is deeply equal to the destination's is left
+ * unrenamed and reported in `dedupedMarkDefKeys` instead: the merge keeps
+ * a single copy under the original `_key` rather than duplicating
+ * identical content under two keys. A colliding markDef that differs gets
+ * renamed like a child would.
+ *
+ * `renamedBlock` keeps every deduped def in its own `markDefs`, even
+ * though the destination already carries it: `renamedBlock` feeds
+ * `insert.block` on some callers' paths, and that path parses the block
+ * standalone, stripping any mark that doesn't resolve in the block's own
+ * `markDefs`. A caller that appends `renamedBlock.markDefs` onto the
+ * destination's must skip the keys listed in `dedupedMarkDefKeys`, or the
+ * def duplicates.
  *
  * Pure: computes the renames and the block they produce without touching
  * an editor. Callers raise or apply the renames themselves, ahead of the
@@ -41,26 +51,13 @@ export function planMergeKeyRenames(args: {
   context: {schema: Schema; keyGenerator: () => string}
   mergingBlock: PortableTextTextBlock
   destinationBlock: PortableTextTextBlock
-  /**
-   * When a merging markDef collides with a destination markDef that is
-   * deeply equal to it, drop the merging markDef (from `renamedBlock` and
-   * from the emitted renames) instead of renaming it, so the merge keeps
-   * a single copy under the original `_key` rather than duplicating
-   * identical content under two keys.
-   *
-   * Must be `false` when `renamedBlock` feeds `insert.block`: that path
-   * parses the block standalone, and `parseBlock`/`parseSpan` strip any
-   * mark that doesn't resolve in the block's own `markDefs`, so a span
-   * whose annotation was deduped away would arrive with the mark gone
-   * rather than re-keyed.
-   */
-  dedupeEqualMarkDefs: boolean
 }): {
   renamedBlock: PortableTextTextBlock
   childRenames: Array<MergeChildRename>
   markDefRenames: Array<MergeMarkDefRename>
+  dedupedMarkDefKeys: Array<string>
 } {
-  const {context, mergingBlock, destinationBlock, dedupeEqualMarkDefs} = args
+  const {context, mergingBlock, destinationBlock} = args
 
   const destinationChildKeys = new Set(
     destinationBlock.children.map((child) => child._key),
@@ -70,14 +67,16 @@ export function planMergeKeyRenames(args: {
   )
 
   const markDefKeyMap = new Map<string, string>()
+  const dedupedMarkDefKeys: Array<string> = []
   const renamedMarkDefs = mergingBlock.markDefs?.flatMap((markDef) => {
     const destinationMarkDef = destinationMarkDefsByKey.get(markDef._key)
     if (!destinationMarkDef) {
       return [markDef]
     }
 
-    if (dedupeEqualMarkDefs && isDeepEqual(markDef, destinationMarkDef)) {
-      return []
+    if (isDeepEqual(markDef, destinationMarkDef)) {
+      dedupedMarkDefKeys.push(markDef._key)
+      return [markDef]
     }
 
     const newKey = context.keyGenerator()
@@ -132,5 +131,6 @@ export function planMergeKeyRenames(args: {
     },
     childRenames,
     markDefRenames,
+    dedupedMarkDefKeys,
   }
 }

packages/editor/src/operations/operation.insert.block.ts

@@ -20,7 +20,11 @@ import {applyInsertNodeAtPath} from '../internal-utils/apply-insert-node'
 import {applySelect, resolveSelection} from '../internal-utils/apply-selection'
 import {applySplitNode} from '../internal-utils/apply-split-node'
 import {deleteRange} from '../internal-utils/delete-range'
-import {isEqualChildren, isEqualMarks} from '../internal-utils/equality'
+import {
+  isDeepEqual,
+  isEqualChildren,
+  isEqualMarks,
+} from '../internal-utils/equality'
 import {setNodeProperties} from '../internal-utils/set-node-properties'
 import {toEngineBlock} from '../internal-utils/values'
 import {getEnclosingBlock} from '../traversal/get-enclosing-block'
@@ -728,8 +732,12 @@ function resolveChildIndex(
 
 /**
  * Reassign keys on spans, inline objects and markDefs that collide with the
- * end block's existing keys. Returns the adjusted block and the adjusted
- * markDefs (to be merged into the end block).
+ * end block's existing keys. A colliding markDef that is deeply equal to
+ * the end block's is left out of the returned `adjustedMarkDefs` instead
+ * of renamed, since the end block already carries it; its key isn't
+ * remapped, so the fragment's marks keep pointing at it unchanged. The
+ * caller must merge the returned `adjustedMarkDefs` into the end block's
+ * own `markDefs`, or the deduped defs are lost.
  */
 function adjustFragmentKeys(args: {
   context: OperationSnapshot['context']
@@ -747,17 +755,24 @@ function adjustFragmentKeys(args: {
   }
 
   const endBlockChildKeys = endBlock.children.map((child) => child._key)
-  const endBlockMarkDefsKeys =
-    endBlock.markDefs?.map((markDef) => markDef._key) ?? []
+  const endBlockMarkDefsByKey = new Map(
+    (endBlock.markDefs ?? []).map((markDef) => [markDef._key, markDef]),
+  )
 
   const markDefKeyMap = new Map<string, string>()
-  const adjustedMarkDefs = block.markDefs?.map((markDef) => {
-    if (endBlockMarkDefsKeys.includes(markDef._key)) {
-      const newKey = context.keyGenerator()
-      markDefKeyMap.set(markDef._key, newKey)
-      return {...markDef, _key: newKey}
+  const adjustedMarkDefs = block.markDefs?.flatMap((markDef) => {
+    const endBlockMarkDef = endBlockMarkDefsByKey.get(markDef._key)
+    if (!endBlockMarkDef) {
+      return [markDef]
+    }
+
+    if (isDeepEqual(markDef, endBlockMarkDef)) {
+      return []
     }
-    return markDef
+
+    const newKey = context.keyGenerator()
+    markDefKeyMap.set(markDef._key, newKey)
+    return [{...markDef, _key: newKey}]
   })
 
   const adjustedChildren = block.children.map((child) => {