fix: emit item-keyed markDefs patches instead of whole-array sets

.changeset/guard-keyed-markdefs-unsets.md

@@ -0,0 +1,7 @@
+---
+'@portabletext/plugin-sdk-value': patch
+---
+
+fix: guard outgoing keyed `markDefs` unsets against store-referenced definitions
+
+An upcoming `@portabletext/editor` release prunes unused annotation definitions as item-keyed `unset` patches instead of whole-array sets. A client that has diverged from the store can prune a definition the store's spans still reference, which would orphan another client's annotation. Outgoing keyed `markDefs` unsets are now dropped while the store's spans reference the definition, mirroring the existing guard on decomposed whole-array sets; at worst an unused definition lingers until a later converged session prunes it. Against current editor releases the guard is inert.

.changeset/markdefs-item-patches.md

@@ -0,0 +1,9 @@
+---
+'@portabletext/editor': patch
+---
+
+fix: emit item-keyed `markDefs` patches instead of whole-array sets
+
+Changing a block's annotations previously emitted a `set` of the whole `markDefs` array. When two clients annotated the same block concurrently, the last writer's array overwrote the other's at the server while both clients' span `marks` references survived, leaving annotations without definitions. Added definitions are now emitted as a `setIfMissing` plus an item `insert`, removed definitions as keyed `unset`s, and undoing those edits emits the item-keyed counterparts. Item-keyed operations merge at the server instead of overwriting.
+
+New definitions are prepended to `markDefs` instead of appended, so code that reads the array positionally sees new definitions first. Consumers observing `patch` or `mutation` events see the finer-grained shapes; explicit whole-array writes through `block.set` still emit a whole-array `set`.

.changeset/undo-step-boundaries.md

@@ -0,0 +1,9 @@
+---
+'@portabletext/editor': patch
+---
+
+fix: keep leading no-op operations from consuming undo-step boundaries
+
+A single undo could revert more than the most recent edit: when an editing flow's first operation was one that doesn't affect history (a zero-length text change, or an operation without an inverse), the following operations merged into the previous undo step. Undo now reverts exactly one step.
+
+Undo also restores the selection from before the step as it was when the edit began, instead of a mid-flow selection that may reference nodes the undo itself removes (which dropped the cursor entirely).

packages/editor/src/behaviors/behavior.perform-event.ts

@@ -98,6 +98,7 @@ function performEventInternal({
 }: PerformEventArgs) {
   if (mode === 'send' && !isNativeBehaviorEvent(event)) {
     editor.undoStepId = defaultKeyGenerator()
+    editor.undoStepSelection = editor.snapshot.context.selection
   }
 
   if (debug.behaviors.enabled) {
@@ -269,6 +270,7 @@ function performEventInternal({
       if (actionSetIndex > 0) {
         // Since there are multiple action sets
         editor.undoStepId = defaultKeyGenerator()
+        editor.undoStepSelection = editor.snapshot.context.selection
 
         undoStepCreated = true
       }
@@ -282,6 +284,7 @@ function performEventInternal({
         // All actions performed recursively from now will be squashed into this
         // undo step
         editor.undoStepId = defaultKeyGenerator()
+        editor.undoStepSelection = editor.snapshot.context.selection
 
         undoStepCreated = true
       }

packages/editor/src/editor/create-editor-engine.tsx

@@ -74,6 +74,7 @@ export function createEditorEngine(
   editor.verifiedUniqueChildGroups = new Set<string>()
   editor.remotePatches = []
   editor.undoStepId = undefined
+  editor.undoStepSelection = null
 
   editor.isDeferringMutations = false
   editor.isNormalizingNode = false

packages/editor/src/editor/subscriber.history.ts

@@ -100,7 +100,11 @@ export function subscribeHistory({
         operation.text.length === 0)
 
     if (isNoOp) {
-      previousUndoStepId = currentUndoStepId
+      // Deliberately not advancing `previousUndoStepId`: it tracks the
+      // last operation that affected history. Advancing it here would
+      // let a no-op leading a new undo step consume the step boundary,
+      // and the following real operations would merge into the previous
+      // step (undoing more than the last step's worth of changes).
       return
     }
 
@@ -118,7 +122,16 @@ export function subscribeHistory({
       previousUndoStepId,
       operationsInProgress: event.operationsInProgress,
       isNormalizingNode: event.isNormalizingNode,
-      selectionBeforeApply: event.beforeSelection,
+      // When this operation opens a new undo step, the step's pre-state
+      // selection is the one snapshotted when the undo step ID was
+      // minted: operation implementations may park the selection on
+      // transient nodes (removed again within the same step) before the
+      // first history-affecting operation applies.
+      selectionBeforeApply:
+        currentUndoStepId !== undefined &&
+        currentUndoStepId !== previousUndoStepId
+          ? editor.undoStepSelection
+          : event.beforeSelection,
     })
 
     // Make sure we don't exceed the maximum number of undo steps we want

packages/editor/src/editor/subscriber.patch-generation.ts

@@ -1,13 +1,8 @@
-import {
-  insert,
-  set,
-  setIfMissing,
-  unset,
-  type Patch,
-} from '@portabletext/patches'
+import {insert, setIfMissing, unset, type Patch} from '@portabletext/patches'
 import {subscribeToOperations} from '../engine/core/operation-channel'
 import {
   insertNodePatch,
+  setNodePatch,
   textPatch,
 } from '../internal-utils/operation-to-patches'
 import {isEqualToEmptyEditor} from '../internal-utils/values'
@@ -77,7 +72,7 @@ export function subscribePatchGeneration({
         patches = [...patches, ...insertNodePatch(operation)]
         break
       case 'set':
-        patches = [...patches, set(operation.value, operation.path)]
+        patches = [...patches, ...setNodePatch(operation, previousValue)]
         break
       case 'unset':
         patches = [...patches, unset(operation.path)]

packages/editor/src/engine/core/apply-operation.ts

@@ -49,8 +49,19 @@ export function applyOperation(editor: Editor, op: EngineOperation): void {
         // `span.marks` or `block.markDefs`) rather than a structural
         // child. Apply the insert as a plain data patch on the root
         // block so the result matches what the datastore computed.
-        // These operations only arrive from remote patches, so no
-        // inverse is needed.
+        if (
+          !op.inverse &&
+          !editor.isProcessingRemoteChanges &&
+          typeof node._key === 'string'
+        ) {
+          // Unkeyed sidecar members can't be addressed for removal, so
+          // they carry no inverse; no local operation site inserts them.
+          op.inverse = {
+            type: 'unset',
+            path: [...path.slice(0, -1), {_key: node._key}],
+          }
+        }
+
         applyOnRootBlock(editor, path, {
           type: 'insert',
           path: path.slice(1),
@@ -295,15 +306,29 @@ export function applyOperation(editor: Editor, op: EngineOperation): void {
         // structural child. Removing it never affects the selection, so
         // apply it as a plain data patch on the root block.
         if (!op.inverse && !editor.isProcessingRemoteChanges) {
-          const arrayValue = getValue(
-            editor.snapshot.context.value,
-            path.slice(0, -1),
-          )
-          if (Array.isArray(arrayValue)) {
+          const removedMember = getValue(editor.snapshot.context.value, path)
+
+          if (isKeyedSegment(lastSegment) && removedMember !== undefined) {
+            // A keyed inverse keeps undo emitting item-level patches;
+            // restoring via a whole-array `set` would overwrite members
+            // another client wrote meanwhile.
             op.inverse = {
-              type: 'set',
-              path: path.slice(0, -1),
-              value: arrayValue,
+              type: 'insert',
+              path: [...path.slice(0, -1), 0],
+              position: 'before',
+              node: removedMember as Node,
+            }
+          } else {
+            const arrayValue = getValue(
+              editor.snapshot.context.value,
+              path.slice(0, -1),
+            )
+            if (Array.isArray(arrayValue)) {
+              op.inverse = {
+                type: 'set',
+                path: path.slice(0, -1),
+                value: arrayValue,
+              }
             }
           }
         }

packages/editor/src/engine/core/normalize-node.ts

@@ -8,7 +8,6 @@ import {applyInsertNodeAtPath} from '../../internal-utils/apply-insert-node'
 import {applyMergeNode} from '../../internal-utils/apply-merge-node'
 import {createPlaceholderBlock} from '../../internal-utils/create-placeholder-block'
 import {debug} from '../../internal-utils/debug'
-import {isEqualMarkDefs} from '../../internal-utils/equality'
 import {setNodeProperties} from '../../internal-utils/set-node-properties'
 import {getChildFieldName} from '../../paths/get-child-field-name'
 import {serializePath} from '../../paths/serialize-path'
@@ -367,8 +366,8 @@ export const normalizeNode: WithEditorFirstArg<Editor['normalizeNode']> = (
    * Remove markDefs not in use
    */
   if (isTextBlock({schema: editor.snapshot.context.schema}, node)) {
-    const newMarkDefs = (node.markDefs || []).filter((def) => {
-      return node.children.find((child) => {
+    const unusedMarkDefs = (node.markDefs || []).filter((def) => {
+      return !node.children.find((child) => {
         return (
           isSpan({schema: editor.snapshot.context.schema}, child) &&
           Array.isArray(child.marks) &&
@@ -377,9 +376,17 @@ export const normalizeNode: WithEditorFirstArg<Editor['normalizeNode']> = (
       })
     })
 
-    if (node.markDefs && !isEqualMarkDefs(newMarkDefs, node.markDefs)) {
+    if (unusedMarkDefs.length > 0) {
       debug.normalization('removing markDef not in use')
-      setNodeProperties(editor, {markDefs: newMarkDefs}, path)
+      for (const unusedMarkDef of unusedMarkDefs) {
+        // Keyed removal instead of setting the whole array: the emitted
+        // patch then only touches this definition, so it merges with
+        // definitions another client writes concurrently.
+        editor.apply({
+          type: 'unset',
+          path: [...path, 'markDefs', {_key: unusedMarkDef._key}],
+        })
+      }
       return
     }
   }

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

@@ -179,7 +179,7 @@ function isEqualSpans(a: ChildLike, b: ChildLike): boolean {
   return isEqualProps(a, b, ['_key', '_type', 'text', 'marks'])
 }
 
-export function isEqualMarkDefs(
+function isEqualMarkDefs(
   a: Array<PortableTextObject>,
   b: Array<PortableTextObject>,
 ): boolean {

packages/editor/src/internal-utils/operation-to-patches.test.ts

@@ -12,7 +12,7 @@ import {createEditor} from '../engine/create-editor'
 import type {Node} from '../engine/interfaces/node'
 import {defaultKeyGenerator} from '../utils/key-generator'
 import {buildIndexMaps} from './build-index-maps'
-import {insertNodePatch, textPatch} from './operation-to-patches'
+import {insertNodePatch, setNodePatch, textPatch} from './operation-to-patches'
 
 function buildBlockIndexMap(
   schema: any,
@@ -413,3 +413,116 @@ describe('defensive setIfMissing patches', () => {
     })
   })
 })
+
+describe(setNodePatch.name, () => {
+  const markDefsPath = [{_key: 'b0'}, 'markDefs']
+
+  function blockWithMarkDefs(markDefs: Array<object> | undefined) {
+    return [
+      {
+        _type: 'block',
+        _key: 'b0',
+        children: [{_type: 'span', _key: 's0', text: 'foo', marks: []}],
+        style: 'normal',
+        ...(markDefs === undefined ? {} : {markDefs}),
+      },
+    ] as Array<PortableTextBlock>
+  }
+
+  test('Scenario: Creating an absent `markDefs` property emits a `setIfMissing`', () => {
+    expect(
+      setNodePatch(
+        {
+          type: 'set',
+          path: markDefsPath,
+          value: [],
+        },
+        blockWithMarkDefs(undefined),
+      ),
+    ).toEqual([
+      {
+        type: 'setIfMissing',
+        path: markDefsPath,
+        value: [],
+      },
+    ])
+  })
+
+  test('Scenario: Setting an existing `markDefs` property passes through as a plain set', () => {
+    expect(
+      setNodePatch(
+        {
+          type: 'set',
+          path: markDefsPath,
+          value: [],
+        },
+        blockWithMarkDefs([
+          {_key: 'l0', _type: 'link', href: 'https://example.com'},
+        ]),
+      ),
+    ).toEqual([
+      {
+        type: 'set',
+        path: markDefsPath,
+        value: [],
+      },
+    ])
+  })
+
+  test('Scenario: Setting a non-empty array on an absent property passes through as a plain set', () => {
+    expect(
+      setNodePatch(
+        {
+          type: 'set',
+          path: markDefsPath,
+          value: [{_key: 'l0', _type: 'link', href: 'https://example.com'}],
+        },
+        blockWithMarkDefs(undefined),
+      ),
+    ).toEqual([
+      {
+        type: 'set',
+        path: markDefsPath,
+        value: [{_key: 'l0', _type: 'link', href: 'https://example.com'}],
+      },
+    ])
+  })
+
+  test('Scenario: Block missing from the pre-apply value passes through as a plain set', () => {
+    expect(
+      setNodePatch(
+        {
+          type: 'set',
+          path: [{_key: 'other-block'}, 'markDefs'],
+          value: [],
+        },
+        blockWithMarkDefs(undefined),
+      ),
+    ).toEqual([
+      {
+        type: 'set',
+        path: [{_key: 'other-block'}, 'markDefs'],
+        value: [],
+      },
+    ])
+  })
+
+  test('Scenario: Paths not ending in `markDefs` pass through as plain sets', () => {
+    expect(
+      setNodePatch(
+        {
+          type: 'set',
+          path: [{_key: 'b0'}, 'style'],
+          value: 'h1',
+        },
+        blockWithMarkDefs([]),
+      ),
+    ).toEqual([
+      {
+        type: 'set',
+        path: [{_key: 'b0'}, 'style'],
+        value: 'h1',
+      },
+    ])
+  })
+})