Bug: Synchronous update re-entry issue with editor.focus

[etrepum]

Follow-up to #8356

There's an edge case where we would leave orphaned deferred update callbacks on the editor when a sync update is called from a sync update with certain rare preconditions. #8356 added a workaround but the solution here is should be a bit more robust with better timing guarantees for when those callbacks will happen.

[mayrang]

In updateEditorSync's inline path (line 1097), what if we snapshot editor._deferred.length before updateFn() and, if the inline call pushed deferred callbacks, schedule a microtask that calls triggerDeferredUpdateCallbacks? It's idempotent — triggerDeferredUpdateCallbacks resets _deferred = [] (line 861), so any outer or sync commit that ran first leaves the microtask as a no-op. This removes the race source rather than relying on the line 514-523 guard to recover. The guard could be retired in the same PR or kept as a defensive layer.

Does this match what you had in mind for the "better timing guarantees", or were you thinking of binding _deferred to commit cycles instead?

[etrepum]

I don’t think microtasks should be involved at all in this particular case

[mayrang]

What if _deferred lived on pendingEditorState? Each commit drains only its own pending's queue, so callbacks fire from whichever commit drained the pending they were attached to — no shared queue, nothing to race over. $onUpdate would need to ensure a pending exists when called, but that's the same clone-and-schedule path $beginUpdate already uses. editor._deferred can stay as a getter aliasing the current pending's queue if anything reads it externally.

Poked at smaller paths first — discrete: true on focus, $onUpdate scheduling its own microtask, focus-specific DOM listener — but they either only partially fix things or bring microtasks back. So I think it has to be structural.

Does that match, or is the cause somewhere else?

[etrepum]

I think one of the problematic cases here is when the EditorState is replaced with setEditorState during an update. There are some quirks in how that works that meta depends on, and I don’t work there so it’s hard to come up with the best solution since I can’t see or fix the code that depends on that behavior. If I knew the right fix off the top of my head without really diving in with full context then it would be fixed already 😅

[mayrang]

Yeah, I tried a few angles too but couldn't find one that works without Meta-side visibility 😅

[abhinavgautam01]

after reading through the LexicalUpdates.ts source across both the pre- and post-#8356 versions, here's what I think is happening:

The updateEditorSync inline path (when activeEditor === editor && options === undefined) just calls updateFn() directly, no pendingEditorState is created, no commit is scheduled. So if anything inside that updateFn() pushes to editor._deferred via $onUpdate, those callbacks get stranded. nothing ever drains them...

The fix could live entirely inside updateEditorSync... drain any newly-pushed _deferred entries right after the inline call, only when no pending commit is already coming:

export function updateEditorSync(
  editor: LexicalEditor,
  updateFn: () => void,
  options?: EditorUpdateOptions,
): void {
  if (activeEditor === editor && options === undefined) {
    const deferredLengthBefore = editor._deferred.length;
    updateFn();
    if (
      editor._deferred.length > deferredLengthBefore &&
      editor._pendingEditorState === null
    ) {
      triggerDeferredUpdateCallbacks(editor, editor._deferred);
    }
  } else {
    $beginUpdate(editor, updateFn, options);
  }
}

This doesn't touch $beginUpdate, $commitPendingUpdates or the setEditorState path at all... triggerDeferredUpdateCallbacks already resets _deferred = [] so it's idempotent if a commit races in. The setEditorState clearing of _deferred in the editorStateWasCloned + !shouldUpdate branch goes through $beginUpdate, not updateEditorSync, so it's unaffected...

Is there a case where _pendingEditorState being non-null inside the inline path is part of the problem too..!?

[etrepum]

Your analysis is incorrect, activeEditor === editor is only true when an update is already happening (another $beginUpdate is already in the call stack). The sequencing would also be wrong because these _deferred are only supposed to be called after the update that scheduled them has completed (it is where $onUpdate pushes to).

[etrepum]

The actual root cause here is the case where an update is flushed inside of an update, which is an error in almost every situation except for some weird special cases around setEditorState

[abhinavgautam01]

Your analysis is incorrect, activeEditor === editor is only true when an update is already happening (another $beginUpdate is already in the call stack). The sequencing would also be wrong because these _deferred are only supposed to be called after the update that scheduled them has completed (it is where $onUpdate pushes to).

ohh... I misread when activeEditor === editor is true... that condition only holds inside an existing $beginUpdate call stack, so a commit is already coming and _deferred won't be orphaned there...

[abhinavgautam01]

The actual root cause here is the case where an update is flushed inside of an update, which is an error in almost every situation except for some weird special cases around setEditorState

got it...

setRootElement calls $commitPendingUpdates directly (and setEditorState does too when !this._updating). When either of these fires mid-update... e.g. setRootElement being called inside an editor.update() callback... it triggers a premature flush. If focus() then runs after that flush (pushing to _deferred via $onUpdate), the outer $beginUpdate has already committed and nothing drains _deferred again.

so the invariant being broken is: $commitPendingUpdates should not be called while editor._updating is true, except it has to be for setRootElement and setEditorState to work correctly in some cases....

Is the right shape of the fix to track whether a commit was already triggered mid-update and schedule a follow-up drain of _deferred after the outer update completes? Or is this more about making setRootElement/setEditorState defer their commit when called inside an update..!?

[danyalahmed1995]

@etrepum I dug into this a bit locally and found a current-main failing case around the setEditorState() path mentioned above.

The reduced test I have calls setEditorState() during an active editor.update() and tracks callback ordering. What I’m seeing is that the outer onUpdate fires before the outer update callback has returned:

actual:
update start
onUpdate
update end
after update

The expected ordering I was testing against was:

update start
update end
after update
onUpdate

So this looks like the broader timing issue rather than only the already-covered #8356 _pendingEditorState === null workaround. I haven’t changed production code, only added the failing test locally.

Does that sound like the right invariant to capture here 😅 ? or is setEditorState() allowed to flush onUpdate before the active update returns in this case?