Skip to content

Resolve synchronous onUpdate verdicts in place (no setData write on sync reject)#391

Merged
CarlosNZ merged 1 commit into
mainfrom
dont-run-setdata-on-synchronous-onupdate-errors
Jun 29, 2026
Merged

Resolve synchronous onUpdate verdicts in place (no setData write on sync reject)#391
CarlosNZ merged 1 commit into
mainfrom
dont-run-setdata-on-synchronous-onupdate-errors

Conversation

@CarlosNZ

Copy link
Copy Markdown
Owner

Problem

In the v2 optimistic editing model, an editor-op commit (value edit / key rename / add-to-object) called apply() synchronously — before the consumer's onUpdate verdict was known — then revert()d if onUpdate rejected. Both writes go through setData, so a rejected edit produced two setData calls (apply + revert) for a value that was never accepted.

This pollutes anything downstream of setData. It surfaced via @json-edit-react/utils' useUndo: a synchronously rejected edit wrote a bogus snapshot into the undo stack, so "Undo" stepped back to the illegal value.

The engine already avoided this for instant ops (delete / move / array-add) via their ~100ms settle delay — but editor ops opted out because a delay on the editor close would be perceptible. Sync-ness, however, is directly detectable: when onUpdate returns a non-thenable, the verdict is known in the same tick, so there's no need to apply optimistically at all.

Change

  • runUpdate is now sync-or-async (src/JsonEditor.tsx): it calls onUpdate directly (extracted normalise/toError helpers) and returns the outcome synchronously unless onUpdate is genuinely async. Return type widened to UpdateOutcome | Promise<UpdateOutcome>.
  • submit() sync fast-path (src/contexts/EditingProvider.tsx): on a synchronous editor-op verdict, skip the optimistic apply() and hand the outcome straight to reconcile — whose existing !applied() branch already applies for commit/override and stays put for error/cancel.
  • reconcile() widened its pre-apply close condition to also close a non-held 'editing' session, so a sync reject closes + reverts exactly as before.
  • Added an isThenable helper in src/utils/misc.ts.

Instant, held, and async paths are unchanged.

Behaviour

For synchronous onUpdate rejects of editor ops:

  • setData is not called (was: apply + revert) → clean useUndo history, no autosave/dirty-flag flicker, no value-flash.
  • Events: submit* → cancel* → updateError (was commit* → updateError), matching how instant ops already report sync rejects.
  • Inline error still shows; editor closes; value reverts — end-state identical to before.

Unchanged: valid commits (one setData + updateSuccess); all async rejects (still optimistic apply-then-revert); instant ops; hold() / confirm flows.

Tests & docs

  • Updated the existing sync-reject tests to the new contract (JsonEditor.test.tsx, imperativeHandle.test.tsx).
  • Added regression coverage: sync edit/object-add/rename rejects (no write, cancel*), a valid sync commit, an async-add optimistic-then-revert guard, and an end-to-end useUndo test (sync-rejected edit leaves the undo stack clean).
  • 687 tests pass; pnpm compile and pnpm lint clean.
  • README: reframed the optimistic-model section around the sync/async split, clarified data/setData (local state) vs onUpdate (external writes), updated the lifecycle note.

Follow-up (not in this PR)

Async-validation clean history via an onEditEvent-gated useUndo is the remaining piece — async rejects still write apply-then-revert; consumers wanting clean history under async validation use hold() today. Deliberately scoped out; useUndo is untouched here.

🤖 Generated with Claude Code

@github-actions

Copy link
Copy Markdown

Bundle size impact

json-edit-react

Format Base raw PR raw Δ raw Base gzip PR gzip Δ gzip
esm 58.04 KB 58.31 KB 🔺 +285 B (+0.48%) 20.71 KB 20.79 KB 🔺 +82 B (+0.39%)
cjs 59.54 KB 59.82 KB 🔺 +285 B (+0.47%) 20.75 KB 20.83 KB 🔺 +87 B (+0.41%)

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

This PR refines the v2 optimistic editing pipeline so that synchronous onUpdate verdicts are resolved immediately without performing an optimistic setData write (and therefore without a revert write on sync reject). This prevents downstream consumers of setData (e.g. undo/history, autosave, dirty flags) from observing transient, never-accepted snapshots.

Changes:

  • Added a sync/async split to the update pipeline: runUpdate now returns synchronously when onUpdate is synchronous, and returns a Promise only when onUpdate is genuinely async.
  • Implemented an editor-op “sync fast-path” in the commit engine to skip optimistic apply for synchronous verdicts, plus a reconcile tweak to close non-held editing sessions on pre-apply reject/cancel.
  • Expanded tests (including an integration test using useUndo) and updated README wording to describe the new sync vs async behavior.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.

Show a summary per file
File Description
test/useUndo.test.tsx Adds an end-to-end integration test ensuring sync-rejected edits don’t pollute the undo stack, plus a sanity test for a valid edit producing one undo step.
test/JsonEditor.test.tsx Updates lifecycle expectations and adds coverage distinguishing sync reject (no write; cancel*) vs async reject (optimistic write then revert) and validates sync success still commits once.
test/imperativeHandle.test.tsx Updates imperative confirm() sync-reject expectations to ensure there is no transient setData write.
src/utils/misc.ts Introduces isThenable helper to distinguish synchronous returns from async thenables.
src/JsonEditor.tsx Reworks runUpdate to avoid forcing synchronous results into a microtask and to normalize outcomes synchronously when possible.
src/contexts/EditingProvider.tsx Adds the synchronous editor-op fast-path and adjusts reconcile’s pre-apply close behavior for sync rejects/cancels.
README_V2.md Updates documentation to clearly distinguish setData (local UI state) from onUpdate (external effects) and explains sync vs async optimistic behavior and lifecycle event implications.

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

@CarlosNZ CarlosNZ left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Just missing a CHANGELOG entry (targeting beta8)

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>
@CarlosNZ CarlosNZ force-pushed the dont-run-setdata-on-synchronous-onupdate-errors branch from ed0fb35 to 990d9cb Compare June 29, 2026 11:13
@CarlosNZ CarlosNZ merged commit 5fc75a7 into main Jun 29, 2026
2 checks passed
@CarlosNZ CarlosNZ deleted the dont-run-setdata-on-synchronous-onupdate-errors branch June 29, 2026 11:15
CarlosNZ added a commit that referenced this pull request Jun 29, 2026
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>
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