Skip to content

useUndo: optional onEditEvent corrector for async-reject history#392

Merged
CarlosNZ merged 5 commits into
mainfrom
use-undo-event-sniffer
Jun 29, 2026
Merged

useUndo: optional onEditEvent corrector for async-reject history#392
CarlosNZ merged 5 commits into
mainfrom
use-undo-event-sniffer

Conversation

@CarlosNZ

Copy link
Copy Markdown
Owner

Stacked on #391 (dont-run-setdata-on-synchronous-onupdate-errors). Review/merge that first; this branch only adds the useUndo change.

Problem

PR #391 stopped synchronous onUpdate rejects from writing to setData, so useUndo's history stays clean for the common local/JSON-Schema validation case. But an asynchronous onUpdate can't be decided in time: it commits optimistically (setData(new)), then reverts (setData(old)) when the promise rejects. Both writes reach useUndo's set, so a rejected async edit pushes two snapshots and "Undo" steps back to the invalid value.

This is rare (it needs async validation that rejects — sync validation is already handled, and async gating is better done with hold()), so the fix is opt-in and unobtrusive.

Change

useUndo now returns an optional onEditEvent:

const { set, undo, redo, onEditEvent } = useUndo(data, setData)
return <JsonEditor data={data} setData={set} onEditEvent={onEditEvent} />
  • set is unchanged — drop onEditEvent and the hook behaves exactly as before (correct for all sync cases).
  • The handler is a pure corrector: it captures the stacks on submitEdit/submitRename/submitAdd (tagged with the operation), restores them on a matching updateError (erasing the apply+revert pair), and clears the marker on updateSuccess. The operation tag prevents a delete/move updateError (no submit*, different op) from consuming an edit's marker.
  • A synchronous reject never reaches set, so its updateError restores stacks that never moved — a harmless no-op.

Why a corrector rather than gating

set is (data) => void — it never learns which path/operation triggered a write, so snapshots can't be tagged per-op; the event stream knows the op but holds no reference to a snapshot. The two channels are decoupled, so the pragmatic design leaves set untouched and uses onEditEvent purely to undo the damage of a reverted optimistic commit.

Scope (documented, not engineered around)

Corrects a single in-flight edit — the realistic case. It does not unwind two genuinely-concurrent async rejections (single marker), nor an async-rejected delete/move (instant ops have no submit* to anchor to). Both are rare; the README points to hold() for watertight history under heavy concurrent async validation.

Tests & docs

  • Extended the UndoEditor harness; added the without-onEditEvent case (pins the documented pollution), the with-onEditEvent fix, a valid async edit (one step, no over-correction), a sync-reject no-op, and two renderHook unit tests of the corrector mechanics.
  • 693 tests pass; pnpm compile and pnpm lint clean.
  • Documented as an "Async validation (optional)" section in the useUndo README only — not promoted in the main README.

🤖 Generated with Claude Code

CarlosNZ and others added 2 commits June 29, 2026 19:40
In the optimistic editing model an editor-op commit (edit / rename /
object-add) applied synchronously before the onUpdate verdict was known,
then reverted if rejected — so a rejected edit wrote to setData twice
(apply + revert) for a value that was never accepted. This polluted
anything downstream of setData, notably useUndo's history, which recorded
a snapshot of the rejected value.

When onUpdate returns synchronously its verdict is known in the same tick,
so there's no need to apply optimistically. runUpdate now returns the
outcome synchronously (no async wrapper) and submit() skips the optimistic
apply for editor ops, handing the verdict straight to reconcile. A sync
reject writes nothing to setData (no value-flash, clean undo history); a
valid edit still commits once. Async onUpdate is unchanged — it still
applies optimistically to hide latency. This is the symmetric completion
of the instant-op behaviour (delete / move / array-add already pre-empt a
sync reject via their settle delay).

Events for a sync reject become submit* -> cancel* -> updateError (no
commit*). Updated the affected tests, added regression coverage including
an end-to-end useUndo test, and documented the sync/async split plus the
data/setData-vs-onUpdate distinction.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
An asynchronous onUpdate that rejects commits optimistically then reverts,
so both writes reach useUndo's `set` and the reverted (invalid) value would
otherwise be reachable via undo. (Synchronous rejects are already clean —
the engine resolves them in place without writing.)

useUndo now returns an optional `onEditEvent`. Wire it to the editor's
onEditEvent and the hook erases a reverted optimistic commit from history;
omit it and the hook behaves exactly as before (`set` is unchanged). It
captures the stacks on submit*, restores them on a matching updateError,
and clears the marker on updateSuccess; the operation tag stops a
delete/move updateError consuming an edit's marker.

Scope: corrects a single in-flight edit (the realistic case); concurrent
async rejects and async-rejected delete/move are out of scope and
documented. Tests cover the without/with-wiring contrast, a valid async
edit, the sync no-op, and the corrector mechanics; documented in the
useUndo README.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@github-actions

Copy link
Copy Markdown

Bundle size impact

@json-edit-react/utils

Format Base raw PR raw Δ raw Base gzip PR gzip Δ gzip
esm 4.62 KB 5.02 KB 🔺 +410 B (+8.67%) 2.09 KB 2.21 KB 🔺 +122 B (+5.69%)
cjs 4.70 KB 5.12 KB 🔺 +429 B (+8.91%) 2.09 KB 2.21 KB 🔺 +122 B (+5.71%)

Measured from build/index.{cjs,esm}.js. Gzip at level 9.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds an opt-in onEditEvent handler from useUndo to correct undo history pollution caused by async onUpdate rejections (optimistic apply followed by revert), without changing the existing set behavior for all other cases.

Changes:

  • Extend useUndo to return an onEditEvent corrector that restores pre-submit undo/redo stacks on a matching updateError.
  • Add integration + unit coverage demonstrating the “polluted history” case when not wired, and the corrected behavior when wired.
  • Document the optional wiring pattern and its intended scope/limitations in the useUndo README.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
test/useUndo.test.tsx Adds integration and unit tests covering async-reject history pollution and the new correction mechanism.
packages/utils/src/undo/useUndo.ts Implements the onEditEvent corrector via a single in-flight marker (queues + operation).
packages/utils/src/undo/types.ts Extends the public UseUndoResult API surface to include onEditEvent.
packages/utils/src/undo/README.md Documents the optional onEditEvent wiring and when it’s needed (async validation that can reject).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread packages/utils/src/undo/types.ts Outdated
@CarlosNZ CarlosNZ force-pushed the use-undo-event-sniffer branch from 6b19b5d to 63f7fcd Compare June 29, 2026 11:10
@CarlosNZ CarlosNZ force-pushed the dont-run-setdata-on-synchronous-onupdate-errors branch from ed0fb35 to 990d9cb Compare June 29, 2026 11:13
Base automatically changed from dont-run-setdata-on-synchronous-onupdate-errors to main June 29, 2026 11:15
CarlosNZ and others added 2 commits June 29, 2026 23:20
Sync after PR #391 (sync-reject engine fix + beta.8 CHANGELOG) was
squash-merged to main. Only conflict was test/useUndo.test.tsx,
resolved to this branch's superset (the engine-PR sync tests plus the
useUndo async-reject corrector tests).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The hook always returns onEditEvent, but declaring it required on
UseUndoResult breaks consumers who type a mock/stub of the result (they'd
have to supply it even without async validation). Optional keeps the
feature opt-in without that API-surface change. Unit tests assert the
returned value with `!`. Addresses the PR #392 review comment.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@CarlosNZ CarlosNZ merged commit c898aa6 into main Jun 29, 2026
2 checks passed
@CarlosNZ CarlosNZ deleted the use-undo-event-sniffer branch June 29, 2026 11:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants