fix: HistoryEditor helpers leaving flags stuck when fn throws, and nested withNewBatch losing the enclosing batch's split

[suyash-vyas]

Description

HistoryEditor.withMerging, HistoryEditor.withNewBatch and HistoryEditor.withoutMerging set internal state flags (MERGING, SPLITTING_ONCE) on WeakMaps before calling the user-provided fn() and restore the previous state afterwards. If fn() throws, the restoration code is never reached and the editor is left permanently stuck in a corrupted state: every subsequent operation merges into a single undo batch, nothing merges, or the next operation is forced into a new batch.

This PR wraps the fn() call in try/finally so the flags are always restored, following the pattern withoutSaving already uses in the same file.

In withNewBatch, the cleanup also only clears SPLITTING_ONCE if this call armed it (if (!prevSplitting) SPLITTING_ONCE.delete(editor)). When withNewBatch is nested inside another withNewBatch that has not applied an operation yet, the flag belongs to the enclosing call: the inner call leaves it pending if nothing was applied, and leaves it consumed if the first operation already started the batch, so later operations of the enclosing batch merge as usual.

Behaviour change vs main

Besides the throw cases, one non-throwing case changes: a nested withNewBatch that applies nothing, before the enclosing batch has applied anything.

editor.insertText('x')
HistoryEditor.withNewBatch(editor, () => {
  HistoryEditor.withNewBatch(editor, () => {}) // applies nothing
  editor.insertText('y')
})

On main the inner call's unconditional delete wipes the enclosing batch's pending split, so y merges into the x batch and one undo removes both, contrary to the documented behaviour ("ensuring that the first operation starts a new batch"). With this PR y starts a new batch. This is the same defect as the throw-before-any-operation case raised in review, reached via return instead of throw; the cleanup cannot distinguish the two without special-casing exceptions. Every other non-throwing sequence, nested or not, behaves exactly as on main.

Issue

N/A — discovered via code inspection. Follows up on #5837, which fixed the same bug for withoutSaving.

Context

withoutSaving in the same file (history-editor.ts) and withoutNormalizing in core (without-normalizing.ts) already wrap fn() in try/finally; this applies the same pattern to the three remaining helpers.

Tests

• test/undo/with_new_batch/throws.tsx, test/undo/without_merging/throws.tsx: flags restored after a throw (fail on main).
• test/undo/with_new_batch/nested.tsx: nested call whose first operation starts the batch; the enclosing batch's next operation merges.
• test/undo/with_new_batch/nested-throws.tsx, nested-empty.tsx: nested call that throws / returns without applying anything; the enclosing batch's first operation still starts a new batch (nested-empty fails on main).
• test/history-editor.ts: isMerging / isSplittingOnce / isSaving are undefined again after each helper throws.

Checks

• The new code matches the existing patterns and styles.
• The tests pass with yarn test.
• The linter passes with yarn lint. (Fix errors with yarn fix.)
• The relevant examples still work. (Run examples with yarn start.)
• You've added a changeset if changing functionality. (Add one with yarn changeset add.)

[changeset-bot]

🦋 Changeset detected

Latest commit: 154d2e5

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
slate-history	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[zbeyens]

Findings

• P2 packages/slate-history/src/history-editor.ts:105: withNewBatch now always deletes SPLITTING_ONCE in finally, but it never records the previous splitting state. In a nested withNewBatch, if the inner callback throws before any operation and the outer callback catches it, this clears the outer pending split, so the next outer operation can merge when it should start a new batch. Capture/restore the prior splitting state, or only clear the flag when this call actually consumed it.

[suyash-vyas]

Findings

• P2 packages/slate-history/src/history-editor.ts:105: withNewBatch now always deletes SPLITTING_ONCE in finally, but it never records the previous splitting state. In a nested withNewBatch, if the inner callback throws before any operation and the outer callback catches it, this clears the outer pending split, so the next outer operation can merge when it should start a new batch. Capture/restore the prior splitting state, or only clear the flag when this call actually consumed it.

Tried addressing your findings can you please check again.

[zbeyens]

@codex review

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 37b2a9de91

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Do not re-arm the outer split flag after nested batches

When withNewBatch is nested inside another withNewBatch before the outer callback has applied an operation, prevSplitting is true; if the inner callback applies an operation, with-history consumes that flag by setting SPLITTING_ONCE back to undefined, but this finally block restores it to true. The next operation in the outer callback is then forced into another new undo batch even though it is a subsequent operation and should merge as usual, regressing nested batch behavior introduced by this cleanup change.

Useful? React with 👍 / 👎.

[suyash-vyas]

@zbeyens sorry for the delay on this. Pushed changes that address this comment, please re-review. For context:

The finally now clears the flag only if this call armed it:

if (!prevSplitting) {
  SPLITTING_ONCE.delete(editor)
}

If an enclosing withNewBatch armed the flag, the inner call leaves it as the callback left it: still pending when nothing was applied (the first finding), or consumed when the first operation ran (this finding).

One heads-up on the first finding #6063 (comment), since its fix changes a non-throwing behaviour vs main: keeping the enclosing batch's pending split alive also applies when the inner callback returns normally without applying anything.

editor.insertText('x')
HistoryEditor.withNewBatch(editor, () => {
  HistoryEditor.withNewBatch(editor, () => {}) // applies nothing
  editor.insertText('y')
})

On main the inner call's unconditional delete wipes the outer's pending split, so y merges into the x batch and one undo removes both, contrary to the documented behaviour ("ensuring that the first operation starts a new batch"). With this PR y starts a new batch. It is the same defect as that finding reached via return instead of throw; the cleanup can't tell the two apart without special-casing exceptions, so fixing one fixes the other. Covered by test/undo/with_new_batch/nested-empty.tsx, which fails on main.