fix: keep leading no-op operations from consuming undo-step boundaries

.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/types/editor-engine.ts

@@ -11,6 +11,7 @@ import type {
   TextBlockConfig,
 } from '../renderers/renderer.types'
 import type {ResolvedContainers} from '../schema/resolve-containers'
+import type {EditorSelection} from './editor'
 
 type HistoryItem = {
   operations: EngineOperation[]
@@ -77,6 +78,13 @@ export interface PortableTextEditorEngine extends DOMEditor {
   verifiedUniqueChildGroups: Set<string>
   remotePatches: Array<RemotePatch>
   undoStepId: string | undefined
+  /**
+   * Engine selection snapshotted when `undoStepId` is minted. The step's
+   * pre-state selection for undo: by the time the first history-affecting
+   * operation applies, operation implementations may have parked the
+   * selection on transient nodes their own step removes again.
+   */
+  undoStepSelection: EditorSelection
 
   isDeferringMutations: boolean
   isNormalizingNode: boolean

packages/editor/tests/event.history.undo.test.tsx

@@ -16,6 +16,99 @@ import type {EditorSelection} from '../src/types/editor'
 import {toTextspec} from '../test-utils/to-textspec'
 
 describe('event.history.undo', () => {
+  test('Scenario: Undoing a `child.set` that replaces an empty span\u2019s text', async () => {
+    const keyGenerator = createTestKeyGenerator()
+    const filledBlockKey = keyGenerator()
+    const filledSpanKey = keyGenerator()
+    const emptyBlockKey = keyGenerator()
+    const emptySpanKey = keyGenerator()
+    const {editor} = await createTestEditor({
+      keyGenerator,
+      schemaDefinition: defineSchema({}),
+      children: (
+        <BehaviorPlugin
+          behaviors={[
+            defineBehavior<{text: string}>({
+              on: 'custom.insert text',
+              actions: [
+                ({event}) => [execute({type: 'insert.text', text: event.text})],
+              ],
+            }),
+            defineBehavior<{text: string}>({
+              on: 'custom.replace empty span',
+              actions: [
+                ({event}) => [
+                  execute({
+                    type: 'child.set',
+                    at: [
+                      {_key: emptyBlockKey},
+                      'children',
+                      {_key: emptySpanKey},
+                    ],
+                    props: {text: event.text},
+                  }),
+                ],
+              ],
+            }),
+          ]}
+        />
+      ),
+      initialValue: [
+        {
+          _key: filledBlockKey,
+          _type: 'block',
+          children: [
+            {_key: filledSpanKey, _type: 'span', text: 'foo', marks: []},
+          ],
+          markDefs: [],
+          style: 'normal',
+        },
+        {
+          _key: emptyBlockKey,
+          _type: 'block',
+          children: [{_key: emptySpanKey, _type: 'span', text: '', marks: []}],
+          markDefs: [],
+          style: 'normal',
+        },
+      ],
+    })
+
+    editor.send({
+      type: 'select',
+      at: {
+        anchor: {
+          path: [{_key: filledBlockKey}, 'children', {_key: filledSpanKey}],
+          offset: 3,
+        },
+        focus: {
+          path: [{_key: filledBlockKey}, 'children', {_key: filledSpanKey}],
+          offset: 3,
+        },
+      },
+    })
+    editor.send({type: 'custom.insert text', text: '!'})
+
+    // Replacing an empty span's text leads with a zero-length
+    // `remove.text`, an operation that doesn't affect history. The
+    // following `insert.text` must still open its own undo step instead
+    // of merging into the previous one.
+    editor.send({type: 'custom.replace empty span', text: 'bar'})
+
+    await vi.waitFor(() => {
+      expect(toTextspec(editor.getSnapshot().context)).toEqual(
+        ['B: foo!|', 'B: bar'].join('\n'),
+      )
+    })
+
+    editor.send({type: 'history.undo'})
+
+    await vi.waitFor(() => {
+      expect(toTextspec(editor.getSnapshot().context)).toEqual(
+        ['B: foo!|', 'B: '].join('\n'),
+      )
+    })
+  })
+
   test('Scenario: Undoing action sets', async () => {
     const {editor, locator} = await createTestEditor({
       children: (