Repository navigation
[lexical][lexical-y] Bug Fix: Continue notifying update listeners after failures - #9614
wozhenbang2004 wants to merge 9 commits into
Conversation
## Description A throwing update listener currently prevents later listeners from seeing an already committed state. Notify every update listener before rethrowing the first failure. Keep other listener types fail-fast, preserve the existing onError contract, and do not call an attempted update-listener cleanup twice after an exception. Closes meta#9514 ## Test plan ### Before Isolated production triggerListeners, extracted from the current source with only TypeScript annotations removed: ```text 8 dispatch cases: 3 passed, 5 failed Error: ["bad"] !== ["bad",1] Error: 2 !== 1 ``` ### After The same dispatch cases against the patched function: ```text 8 dispatch cases: 8 passed, 0 failed ``` Covers subsequent notifications, the original thrown value including undefined, cleanup errors, self-unregistering listeners, non-update fail-fast behavior, and _updating restoration. Full Vitest and code-quality checks are still pending in the fork; no full-suite pass is claimed.
## Description Cover later committed-state notifications, the first thrown value including undefined, cleanup failures and self-unregistration. Preserve onError behavior and non-update fail-fast dispatch. ## Test plan ### Before ```text Isolated current dispatcher: 8 cases, 3 passed, 5 failed ``` ### After ```text Isolated patched dispatcher: 8 cases, 8 passed, 0 failed ``` The full new Vitest tests and code-quality checks will be run on the completed fix branch. No full-suite pass is claimed by this commit.
## Description Exercise a real two-client collaboration session with a throwing application listener registered before the binding. Check that the committed deletion and a subsequent edit reach every peer whether onError reports or rethrows. ## Test plan ### Before The control branch run 41127281136 failed both collaboration cases: the remote document retained t2z after the local deletion. ### After The same regression is now part of the fix branch. Full Vitest validation is pending on the completed change; no passing run is claimed by this commit.
## Description Move the meta#9514 expected-failure scenario to ThrowingUpdateListener.test.ts, where it runs as a normal regression for both onError contracts. Leave unrelated expected failures and the pending-state case intact. ## Test plan ### Before The original test.fails passes only because later collaboration listeners miss the deletion. Both new normal regression variants fail on the unpatched runtime in fork run 41127281136. ### After The expected-failure duplicate is removed in favor of the two normal regressions. The complete patched Vitest run is pending; no passing result is claimed by this commit.
## Description Document the lexical patch behavior for update listener notification and cleanup after exceptions, including a manual Verify path. ## Test plan ### Before ```text Isolated current dispatcher: 8 cases, 3 passed, 5 failed ``` ### After ```text Isolated patched dispatcher: 8 cases, 8 passed, 0 failed ``` The final branch will be checked with the focused Vitest command and pnpm run ci-check, including check-changesets. Those full checks are pending.
## Description Wrap the nextUnregister assignment after nesting the listener dispatch in error isolation. This changes formatting only. Closes meta#9514 ## Changeset .changeset/lexical-update-listener-errors.md documents the lexical patch. ## Test plan ### Before Control commit 6f466dc, fork run 41128429042: ```sh pnpm exec vitest run --project unit packages/lexical/src/__tests__/unit/LexicalUpdateErrors.test.ts packages/lexical/src/__tests__/unit/LexicalUpdateListenerIsolation.test.ts packages/lexical-y/src/__tests__/unit/UpdateErrors.test.ts packages/lexical-y/src/__tests__/unit/regressions/ThrowingUpdateListener.test.ts ``` ```text Test Files 2 failed | 2 passed (4) Tests 10 failed | 41 passed | 1 expected fail (52) ``` ### After The same command on fix commit a950bd8, fork run 41129069653: ```text Test Files 4 passed (4) Tests 51 passed (51) ``` The duplicate meta#9514 expected-failure test was moved to normal regressions. pnpm run ci-check flagged only this source file for Prettier. All related lexical/lexical-y unit tests are running separately; code quality and the focused tests will be rerun on the formatted head. No full green CI result is claimed before those checks finish.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Hi @wozhenbang2004! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks! |
|
AI review status for this pull request.
|
![meta-code-review[bot]](https://avatars.githubusercontent.com/in/5334748?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thanks for this fix, it looks good. Notifying every update listener before rethrowing the first error closes the #9514 fork, and it keeps what the caller of editor.update receives and the onError contract (pinned by an update listener that throws in LexicalUpdateErrors.test.ts) unchanged. The new core tests and the two-peer @lexical/y regression check both peers and a fresh reader, which is the evidence this change needs.
A few optional, non-blocking suggestions:
- Report the errors after the first (
packages/lexical/src/LexicalUpdates.ts:979). When two update listeners throw, the second error is now discarded without any report.$beginUpdatealready sends an error the caller cannot receive toonError(LexicalUpdates.ts:1479-1486), and option 1 in #9514 suggestedonErrorreporting. Passing the later errors toeditor._onErrorinside a try/catch would keep a second, independent bug visible;keeps the first of multiple listener errorswould then assert it. - Drain updates queued by the listeners that now run (
LexicalUpdates.ts:885-888). After the rethrow the commit skips$triggerEnqueuedUpdates, so aneditor.update()called by a later listener stays queued until the next unrelated update.$beginUpdateschedules that drain in a microtask when a listener throws during recovery (LexicalUpdates.ts:1472-1475); doing the same here would match it. - Other listener types, as a follow-up (
LexicalUpdates.ts:839-867). Mutation, decorator and text-content listeners run before the update listeners and still stop at their first throw, so a throwing text-content listener still keeps the@lexical/ybinding from writing that commit, with the same fork as #9514. The description scopes this PR to update listeners, so a follow-up issue would be enough. - Changeset wording (
.changeset/lexical-update-listener-errors.md:7). "or whose next listener throws" reads as if another listener threw, and the early clear also fixes a non-throwing case: a listener that removes itself on a later call no longer runs its previous cleanup twice. Something like "An update listener's cleanup now runs once, even when it throws, when its listener throws on its next call, or when the listener removes itself" would cover both; CONTRIBUTING.md asks changeset entries to be written for people who use Lexical.
| // Every update listener must see the committed state, even if an | ||
| // earlier listener throws. Keep the existing error contract: callers | ||
| // receive the first error and onError is not called for listeners. | ||
| rethrown ??= {error}; |
There was a problem hiding this comment.
Optional, non-blocking: errors after the first are discarded here without any report. $beginUpdate already sends an error the caller cannot receive to onError (lines 1479-1486, covered by an error thrown while the editor is restored is reported too). Passing the second and later listener errors to editor._onError inside a try/catch would keep an independent listener bug visible while the caller still gets the first.
| if (type !== 'update') { | ||
| throw error; | ||
| } |
There was a problem hiding this comment.
Possible follow-up: decorator and text-content listeners (and mutation listeners, which never reach this function) run before the update listeners in the same commit (lines 839-867) and still stop at their first throw, so a throwing text-content listener still keeps a collaboration binding from writing the commit. Since the description scopes this change to update listeners, a follow-up issue would track it.
| if (rethrown !== null) { | ||
| throw rethrown.error; | ||
| } |
There was a problem hiding this comment.
Optional, non-blocking: once this rethrows, the commit tail at lines 885-888 is skipped, so an editor.update() queued by a listener that ran after the failure stays queued until the next unrelated update. $beginUpdate schedules $triggerEnqueuedUpdates in a microtask for the same case (lines 1472-1475); doing that before the error leaves the commit would match it.
|
|
||
| [fix] Notify every update listener before rethrowing the first listener error | ||
|
|
||
| Update-listener cleanups that throw, or whose next listener throws, are not called again on the next commit. |
There was a problem hiding this comment.
Nit: "or whose next listener throws" reads as if another listener threw. The early clear also means a listener that removes itself on a later call no longer runs its previous cleanup twice; something like "An update listener's cleanup now runs once, even when it throws, when its listener throws on its next call, or when the listener removes itself" covers both (CONTRIBUTING.md asks entries to be written for people who use Lexical).
## Description When an update listener throws, later listeners now run and can queue editor.update calls. Rethrowing the first failure skips the commit tail that normally drains those updates, leaving them stalled until an unrelated external update. Schedule the existing queued-update pump in a microtask before rethrowing the first listener failure. Add two regressions for reporting and rethrowing onError handlers; both assert that the queued update commits once without another external update, the queue is emptied, and onError remains unused. Clarify the existing cleanup changeset wording. ## Changeset Update .changeset/lexical-update-listener-errors.md with the queued-update recovery behavior and a Verify path. ## Test plan ### Before Test-only control 9167f2c preserves the previous runtime. Run: https://github.com/wozhenbang2004/lexical/actions/runs/41137920517 ```sh pnpm exec vitest run --project unit packages/lexical/src/__tests__/unit/LexicalUpdateErrors.test.ts packages/lexical/src/__tests__/unit/LexicalUpdateListenerIsolation.test.ts packages/lexical-y/src/__tests__/unit/UpdateErrors.test.ts packages/lexical-y/src/__tests__/unit/regressions/ThrowingUpdateListener.test.ts ``` ```text Test Files 1 failed | 3 passed (4) Tests 2 failed | 51 passed (53) AssertionError: expected 'after' to be 'queued' ``` Both new queue regressions fail; all 51 existing focused tests still pass. ### After The same command on 04df8f1, run https://github.com/wozhenbang2004/lexical/actions/runs/41139189328: ```text Test Files 4 passed (4) Tests 53 passed (53) ``` ```sh pnpm exec vitest run --project unit packages/lexical/src/__tests__/unit packages/lexical-y/src/__tests__/unit ``` ```text Test Files 273 passed | 2 skipped (275) Tests 12920 passed | 203 expected fail | 2 skipped (13125) ``` pnpm run ci-check succeeded, including TypeScript, Flow, ESLint, Prettier and changesets. Formatting diagnostics produced no diff. ```text Changeset check passed: 51 pending changeset(s) valid. Found 0 errors ``` Ubuntu 24.04.5 and Node 24.22.1 were exercised. Node 22, browser-mode and E2E suites were not exercised by this validation run.
There was a problem hiding this comment.
Thanks for the careful fix and the detailed test plan. This looks good. Calling every update listener before rethrowing the first error fixes #9514 and keeps the existing contract: the caller still gets the first thrown value, onError is still not called for listener errors, and _updating is restored. The new core and @lexical/y regressions fail without the change. The changeset follows CONTRIBUTING.md (lexical patch, a [fix] summary and a Verify: paragraph).
The suggestions below are optional and non-blocking:
- Errors after the first are dropped silently (
packages/lexical/src/LexicalUpdates.ts, line 979).rethrown ??= {error}keeps the first error and discards the rest without reporting them. Before this change those listeners never ran, so nothing regresses. But now, if a binding or history listener fails after a failing app listener, the second failure leaves no trace.$beginUpdatealready reports a secondary commit error toonErrorinside a try/catch (same file, lines 1481-1488). Doing the same for the second and later listener errors would still leaveonErrorunused in the single-error case thatLexicalUpdateErrors.test.tsasserts. #9514 also suggested reporting throughonError, so this is a design call for you and the maintainers. cleans up a listener that unregisters itselfpasses without the fix (packages/lexical/src/__tests__/unit/LexicalUpdateListenerIsolation.test.ts, line 200). The listener unregisters on its first call, before any cleanup is stored, so the test passes onmainand also with thelistenerMap.set(listener, undefined)block removed. The block actually fixes a listener that unregisters itself on a later call. Onmain, that listener's stored cleanup runs twice: once intriggerListenersand again inunregisterListener(packages/lexical/src/LexicalEditor.ts). Unregistering on the second call and expecting two cleanups would cover what the changeset says.- Possible follow-up (existing behaviour): when an update listener throws, the commit's
onUpdate/$onUpdatecallbacks are still skipped.editor._deferredis cleared atLexicalUpdates.tslines 777-780, andtriggerDeferredUpdateCallbacksat lines 885-887 is never reached. The new microtask recovers queued updates but not these callbacks. If you'd rather keep this PR narrow, this could be a separate issue.
| // Every update listener must see the committed state, even if an | ||
| // earlier listener throws. Keep the existing error contract: callers | ||
| // receive the first error and onError is not called for listeners. | ||
| rethrown ??= {error}; |
There was a problem hiding this comment.
Optional, non-blocking: every error after the first is discarded here without any report. Before this change those listeners never ran, so nothing regresses, but a failing binding or history listener behind a failing app listener now leaves no trace. $beginUpdate already reports its secondary commit error to onError inside a try/catch (lines 1481-1488). Doing the same for the second and later listener errors would still leave onError unused in the single-error case.
| expect(editor._updating).toBe(false); | ||
| }); | ||
|
|
||
| test('cleans up a listener that unregisters itself', () => { |
There was a problem hiding this comment.
Optional: this listener unregisters on its first call, before any cleanup is stored, so the test passes on main and also without the listenerMap.set(listener, undefined) change. The case the change fixes is a listener that unregisters itself on a later call: on main, its stored cleanup runs twice (once in triggerListeners, once in unregisterListener). Unregistering on the second call and expecting cleanups to be 2 would cover it.
… call ## Description The self-unregistering update-listener test previously removed the listener on its first call, before any cleanup was stored. Parameterize it for both the first and second call. The second-call case must run the previous and newly returned cleanup once each, and a third commit must cause no further listener calls or cleanups. ## Test plan ### Before Control 19acd9d uses the exact candidate tests with only the early cleanup-clearing block removed. Run: https://github.com/wozhenbang2004/lexical/actions/runs/41146246337 ```sh pnpm exec vitest run --project unit packages/lexical/src/__tests__/unit/LexicalUpdateErrors.test.ts packages/lexical/src/__tests__/unit/LexicalUpdateListenerIsolation.test.ts packages/lexical-y/src/__tests__/unit/UpdateErrors.test.ts packages/lexical-y/src/__tests__/unit/regressions/ThrowingUpdateListener.test.ts ``` ```text Test Files 1 failed | 3 passed (4) Tests 3 failed | 51 passed (54) AssertionError: expected 3 to be 2 // Object.is equality ``` The new second-call case fails because the stored cleanup is invoked twice. The first-call case passes; the other two failures are the existing cleanup-after-exception regressions. ### After The same command on candidate 99897dd, run https://github.com/wozhenbang2004/lexical/actions/runs/41146060897: ```text Test Files 4 passed (4) Tests 54 passed (54) ``` ```sh pnpm exec vitest run --project unit packages/lexical/src/__tests__/unit packages/lexical-y/src/__tests__/unit ``` ```text Test Files 273 passed | 2 skipped (275) Tests 12921 passed | 203 expected fail | 2 skipped (13126) ``` pnpm run ci-check succeeded, including TypeScript, Flow, ESLint, Prettier and changesets. Formatting diagnostics produced no diff. ```text Changeset check passed: 51 pending changeset(s) valid. Found 0 errors ``` Ubuntu 24.04.5 and Node 24.22.1 were exercised. Node 22, browser-mode and E2E suites were not exercised by these validation runs.
There was a problem hiding this comment.
Thanks for the fix and the detailed test plan. This looks good. Calling every update listener before rethrowing the first error fixes #9514 and keeps the existing contract: the caller still gets the first thrown value, onError is still not called for listener errors, and editor._updating is restored. The new core and @lexical/y regressions fail without the runtime change, and the new on call 2 case now covers a listener that unregisters itself while it has a stored cleanup. The changeset follows .changeset/README.md: a lexical patch entry, a [fix] summary and a Verify: paragraph.
The suggestions below are optional and non-blocking:
- Errors after the first are dropped silently (
packages/lexical/src/LexicalUpdates.ts, line 979).rethrown ??= {error}keeps the first error and discards the rest without reporting them anywhere. Before this change those listeners never ran, so nothing regresses. But now a history or collaboration listener that fails after a failing app listener leaves no trace.$beginUpdatealready reports a secondary commit error toonErrorinside a try/catch (same file, lines 1481-1488). Doing the same for the second and later listener errors would still leaveonErrorunused in the single-error case thatLexicalUpdateErrors.test.tsasserts. Option 1 in #9514 suggested reporting throughonError, so this is a design call for you and the maintainers. - A listener whose own cleanup throws still misses that commit (
LexicalUpdates.ts, lines 952-971). The stored cleanup and the listener call share onetry. When the cleanup throws, that listener is skipped for the commit and called again on the next one.mainbehaves the same, but the changeset's first line says every update listener is notified. Either call the listener after a failed cleanup, or mention the exception in the changeset. - Possible follow-up (existing behaviour): when an update listener throws, the commit's
onUpdate/$onUpdatecallbacks are still skipped.editor._deferredis cleared atLexicalUpdates.tslines 777-780, andtriggerDeferredUpdateCallbacks(lines 885-887) is never reached. The new microtask recovers queued updates but not these callbacks. This could be a separate issue if you'd rather keep this PR narrow. - Test plan: the After output in the description was recorded before the last commit, which adds the
on call 2case (54 focused tests now). AGENTS.md asks the Test plan to carry the real output of the command that was run, so a refreshed paste would match the current head.
| // Every update listener must see the committed state, even if an | ||
| // earlier listener throws. Keep the existing error contract: callers | ||
| // receive the first error and onError is not called for listeners. | ||
| rethrown ??= {error}; |
There was a problem hiding this comment.
Optional: the second and later listener errors are discarded here without any report. Reporting them through onError inside a try/catch, as $beginUpdate does for a secondary commit error (lines 1481-1488), would keep the first-error contract and still leave a trace when a binding or history listener fails behind a failing app listener.
| // been called. Do not call it again on the next commit. | ||
| listenerMap.set(listener, undefined); | ||
| } | ||
| unregister(); |
There was a problem hiding this comment.
Nit: when this cleanup throws, the listener(...payload) call below is skipped, so this listener misses the commit (it runs again on the next one). main does the same, but the changeset says every update listener is notified. Either call the listener after a failed cleanup, or mention the exception in the changeset.
…havior ## Description Clarify the existing changeset without changing runtime code. A listener whose own previous cleanup throws remains skipped for that commit; notification continues to the other listeners. Replace the overbroad summary and document this exception. ## Test plan Manual comparison of the changeset with the existing cleanup/listener control flow and inspection of the two changed documentation lines. ### Before The summary said: `[fix] Notify every update listener before rethrowing the first listener error`. It did not state the own-cleanup exception. ### After The summary says: `[fix] Continue notifying update listeners after listener or cleanup errors`. The next paragraph now begins: `As before, a listener whose own cleanup throws is skipped for that commit.` Only this changeset file is edited. Automated tests, check-changesets, and the browser/platform matrix were not rerun for this wording-only change.
Description
An update listener that throws can prevent a later collaboration binding from seeing an already committed change. The binding then permanently misses that change, including after later successful edits.
Continue notifying remaining update listeners before rethrowing the first failure to the caller. Preserve non-Error thrown values, the existing
onErrorcontract, and_updatingrestoration. Clear attempted update-listener cleanups before invoking them so a cleanup is not called again when it or the following listener invocation throws. Other listener types retain fail-fast behavior.When a listener failure interrupts the commit tail, schedule the existing queued-update pump in a microtask so updates queued by later listeners commit without waiting for an unrelated external update. The caller still receives the first listener failure, and listener errors do not invoke
onError.Add core isolation/cleanup/queued-update regressions and two-client convergence regressions for reporting and rethrowing
onErrorhandlers. Move the existing #9514test.failsscenario into normal regression coverage. The self-unregistration cases cover both the first call and a later call with a stored cleanup, including no further callbacks after removal.Secondary listener errors remain unreported in this PR. Deferred
onUpdate/$onUpdatecallbacks retain their existing behavior when an update listener throws.Closes #9514
Changeset
.changeset/lexical-update-listener-errors.mdadds alexicalpatch entry and aVerify:path, including queued-update recovery and clarification of cleanup behavior.Test plan
Before
The same focused command is used for both controls and the final candidate:
pnpm exec vitest run --project unit packages/lexical/src/__tests__/unit/LexicalUpdateErrors.test.ts packages/lexical/src/__tests__/unit/LexicalUpdateListenerIsolation.test.ts packages/lexical-y/src/__tests__/unit/UpdateErrors.test.ts packages/lexical-y/src/__tests__/unit/regressions/ThrowingUpdateListener.test.tsOriginal control
6f466dc6e4b0b74ebebf96b4a1c6667942484fb7has the regression tests with the unchanged runtime: original baseline run.Follow-up control
9167f2c2327eaa8a5e0904c4aec6a0a4a077b9b9adds the two queued-update regressions to the previous PR head without changing its runtime: queued-update baseline run.Both new queue cases fail while the 51 existing focused tests pass. They inspect the committed state with
getEditorState().readafter natural microtasks, without forcing a flush or issuing another external update.Cleanup control
19acd9d34ccc4c965d9db180ed869b53fd7593d1uses the exact final tests with only the early cleanup-clearing block removed: stored-cleanup control run.The first-call self-unregistration case passes. The new second-call case detects the stored cleanup being called twice; the other two failures are the existing cleanup-after-exception regressions.
After
Tested candidate
99897dda119faf86115004d73426ee58ea2936ff: passing validation run. The published commit6d65420b71eeeacf5b369a11c2204fed46b65a31has the identical Git tree87899511a0d5c91fc48971d94919f194727081a2. The final follow-up changes only the self-unregistration test coverage.The focused command:
Relative to the original 52-test control, remove the duplicate #9514
test.failsscenario (covered by the two normal collaboration regressions), add two queued-update cases, and retain the first-call self-unregistration case while adding a second-call case, yielding 54 tests.All related core and collaboration unit tests:
pnpm exec vitest run --project unit packages/lexical/src/__tests__/unit packages/lexical-y/src/__tests__/unitpnpm run ci-checkexited successfully (TypeScript, Flow, ESLint, Prettier, and changesets). The formatting diagnostic produced no diff. Relevant output:These are fork validation results on Ubuntu 24.04.5 and Node 24.22.1. Node 22, browser-mode, and E2E suites were not exercised in these runs.