RTC: Fix CRDT deferred updates resulting in jumbled typing#78756
Conversation
|
Size Change: -12 B (0%) Total Size: 8.19 MB 📦 View Changed
ℹ️ View Unchanged
|
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
| * @param {ObjectID} objectId Object ID. | ||
| */ | ||
| async function createPersistedCRDTDoc( | ||
| function createPersistedCRDTDoc( |
There was a problem hiding this comment.
Does this need to return a promise still?
There was a problem hiding this comment.
Fixed in 82be026!
I'm still working on an e2e test failure related to the undo stack.
|
Still working on the failing $ npm run test:e2e -- test/e2e/specs/editor/various/inner-blocks-templates.spec.js
> gutenberg@23.3.0-rc.1 test:e2e
> npm exec --workspace @wordpress/e2e-tests-playwright -- wp-scripts test-playwright --config playwright.config.ts test/e2e/specs/editor/various/inner-blocks-templates.spec.js
Running 1 test using 1 worker
✓ 1 [chromium] › specs/editor/various/inner-blocks-templates.spec.js:25:2 › Inner blocks templates › applying block templates asynchronously does not create a persistent change in the editor (5.5s)
1 passed (8.2s)Edit: I can also run |
|
The "works fine locally" issue ended up being a previously interrupted test ( |
|
There's definitely a timing issue between the new synchronous CRDT commit and the undo/redo stack. Digging in some more. |
278a411 to
88b2004
Compare
|
The failing
In short, when the Yjs undo manager adds a new item to the stack, it doesn't notify the UI that something has changed so it can re-render the undo/redo UI toolbar actions. We relied on the fact that typically other things are also changing in core data, so UI state will probably get updated anyway. This seemed to work okay, but "the bug exists but is hidden" is no longer true here due to the timing change. Previously the I think this particular issue should be spun out into a new PR where we ensure Yjs undo operations affect the core data store and update the UI, see the test start failing (correctly), and fix the incorrect undo stack addition there. With that bug fixed, the current PR should begin passing. |
This PR is dependent on #78864 before it can be merged.
What?
Fix a race condition where rapid concurrent typing over the WebSocket RTC provider with a small delay could lose characters or cause the cursor to end up in the wrong spot.
Before (on
trunk, with a simulated 100ms delay on the WebSocket server):deferred-update-trunk.mov
After (this branch, also with a simulated delay):
deferred-update-fix.mov
The main change is to commit local editor changes to the local
Y.Docsynchronously, so that remote CRDT updates don't overwrite local editor state before it is committed.Why?
With WebSocket sync and a small network delay, a remote update can arrive in the short window after Gutenberg has accepted a local keystroke but before that keystroke has been applied to the local
Y.Doc. When that happens, the CRDT merge can treat the staleY.Docas authoritative and overwrite the local editor state with another user's stale block snapshot.This is easier to hit with WebSockets with a production delay than with HTTP polling because remote updates routinely arrive between local keystrokes. The result looks like characters are being dropped, but the local character was committed to the local editor store and then overwritten before it became part of the CRDT merge.
How?
SyncManager.update()now applies local CRDT updates immediately instead of wrapping the whole update inyieldToEventLoop(). This ensures we apply local editor changes before a remote update can be merged into the local document.The PR also narrows an existing
clientIdpreservation behavior introduced in #78483 (also me), designed to address code editor updates. The new change still preserves the code editor fix, which ignores randomly-generated client IDs fromcontent-only updates, but skips that behavior for normal block merges. This change is necessary to preserve selection update behavior in normal CRDT merges, which relies on a block'sclientId.Both the synchronous CRDT update and
clientIdchanges are necessary for the fix, although theclientIdfix is more defensive. Correct character placement depends on both ordering and cursor context. Applying local changes synchronously ensures theY.Docalready contains the local keystroke before a remote update is merged. Allowing regular editor changeclientIds to stay fixed ensures the selection/cursor hint points at the same RichText instance in theY.Doc, so the merge can apply the next character at the intended position instead of diffing the cursor from an incorrect block.This PR also adds a
npm run rtc:ws:slowcommand, which introduces the 100ms delay described above for local testing and (future) e2e testing.Safety Of Changes
The visual editor uses block
clientIds to scope selection and cursor updates. IfmergeCrdtBlocks()always skipsclientIdupdates (as #78483 introduced), the editor's selected block ID can drift from theY.Docblock ID. Next, selection history cannot reliably resolve the rightY.Text, and fast repeated typing can lose the cursor hint used by the rich-text merge. This PR keepsclientIdpreservation only for the Code Editor path where reparsing creates fresh random clientIds on each edit. Inserted blocks still get their clientIds through the existingcreateNewYBlock()insertion path. The new clientId update only applies when an existing Y block is being updated to match an incoming visual-editor block.Skipping the outer
yieldToEventLoop()should be safe because the selection timing behavior is still preserved where it belongs:applyPostChangesToCRDTDoc()continues to defer selection history updates withsetTimeout( ..., 0 ). Local content and block updates are synchronous again, while the undo/selection ordering remains delayed. Because content updates are no longer deferred,createPersistedCRDTDoc()can serialize the Y.Doc immediately.Follow-up
An important note is that the new
collaboration-concurrent-typing.spec.tstest does not currently run in CI. A follow-up PR should enable this and all passing WS e2e tests into a normal CI run, so WS-specific regressions are discovered quickly.Testing Instructions
npm run rtc:ws:slowto set upwp-envwith a local delayed WebSockets server.To type quickly using the snippet in the videos above, use this:
Automated checks
Use of AI Tools
AI assistance: Yes
Tool(s): Codex
Used for: Investigating the CRDT update race, adding focused test coverage, fixing the issue.