RTC: Fix undo breaking for non-synced entities - #83888
Conversation
…ing the undo manager when any synced entity is loaded
🤖 PR meta 🤖🎉 PropsIf 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. Updated as activity occurs, without notifying anyone named here. Add the 📦 Bundle sizeSize Change: -2 B (0%) Total Size: 8.25 MB 📦 View Changed
⚡ PerformanceShow the resultsClient side metrics exclude the server response time. front-end-block-theme
front-end-classic-theme
media-processing
media-upload
post-editor
site-editor
|
| return Boolean( getSyncManager() && getSyncConfig( kind, name ) ); | ||
| }, | ||
|
|
||
| isSynced( kind, name, recordId ) { |
There was a problem hiding this comment.
Can we call this isLoaded to mirror the SyncManager method? isSynced implies sync state to me
| * | ||
| * @return {Array} The record. | ||
| */ | ||
| export function createSyncUndoLevelRecord() { |
There was a problem hiding this comment.
Can this file be TypeScript?
| // events must not be mistaken for new levels. | ||
| let isApplyingHistory = false; | ||
|
|
||
| const yUndoManager = new YMultiDocUndoManager( [], { |
There was a problem hiding this comment.
Now that the OG UndoManager is back in control and delegates undo/redo per-entity, we no longer have a need for the YMultiDocUndoManager. We can just create a Y.UndoManager for each entity and store a reference. This should simplify things a bit
There was a problem hiding this comment.
There was a problem hiding this comment.
Maybe we could go further and just attach an UndoManager to each EntityState, leave that to you
There was a problem hiding this comment.
Good thinking! YMultiDocUndoManager managed undoes across all synced entities, but core-data can be more specific than telling Yjs to handle whatever undo is next. I think I'll bring your commits in here and try the EntityState changes too.
A placeholder in core-data's undo history now names the record whose Yjs level it stands for, and the sync manager keeps one Yjs undo manager per entity instead of a YMultiDocUndoManager. Undoing a placeholder moves that entity's level only. Before, a placeholder meant "whatever is on top of the Yjs stack". After a synced record was unloaded (for example deleted), its placeholder undid another entity's level out of order. Now a level whose record is no longer synced is skipped. When one entity opens a level, the other entities stop capturing and drop their redo levels, so a later change cannot merge into an older level.
# Conflicts: # packages/core-data/src/test/private-selectors.jsdom.test.js
|
@chriszarate Your comments above have been addressed, please review again when you can. Thanks! |
The sync manager defers local changes to a CRDT document while editing alone, and a change's undo level, with its placeholder in core-data's history, opens only when the deferred change is applied. applyUndoLevel took the top level off the history before anything applied them, so an undo right after typing in a synced record moved the edit before it instead, and the typing's placeholder then dropped the redo level. Close the sync manager's level first, which applies deferred changes, as recordEntityEdit already does before it records an edit.
|
@alecgeatches I pushed a bug fix that makes conceptual sense but could use some eyes and testing. Revert if it looks wrong. |
|
AI development test failures seem to be unrelated and also failing on trunk, so going to merge. Thanks @chriszarate! |
What?
Fixes #80722. Replaces earlier draft PR #81063.
Fixes undo when some synced and non-synced entities are loaded in the same editor.
When RTC is disabled for some entity types (e.g. a custom post type), but not completely disabled, this can cause the Yjs undo manager to incorrectly take control of the undo stack for all entities. Below, a post type has been excluded from sync using a filter. Undo works until a synced entity (a category) is loaded via the publish panel, which breaks the undo manager for the post on
trunk:undo-mixed-entity-trunk.mov
With this fix, only synced entities delegate to the Yjs undo manager:
undo-mixed-entity-fix.mov
Why?
When collaboration is enabled, the Yjs undo manager replaces core-data's undo manager after any synced entity is loaded. When a entity type has been excluded from syncing but the same page includes other synced entities, the undo manager for both is replaced incorrectly.
How?
core-data's undo manager stays in charge of the whole undo history, and now only delegates to Yjs on synced entities:
editEntityRecordrecords every edit in core-data's undo manager as before, except edits to a record the sync manager reports as synced (isSynced). Yjs tracks those, and the record's newonUndoLevelOpenedhandler tells core-data each time Yjs opens a new level. core-data then adds a placeholder record for that level to its undo manager (recordSyncUndoLevel), so synced and non-synced levels sit in the same ordered history. Yjs used to manage undo levels between different entities with a YMultiDocUndoManager.undoandredopop from core-data's undo manager. Placeholder undo levels are delegated to the sync manager'sundoHistory, but everything else is applied to the store. Interleaved edits are undone in the order they were made. Levels whose entities are gone (e.g. an unloaded document) are skipped. The logic lives inpackages/core-data/src/utils/sync-undo-levels.js.When core-data records an edit itself, it closes the sync manager's current level and drops its redo levels (
stopCapturing,clearRedo), so a later synced change cannot merge into a level that is no longer on top and a new edit ends the redo history of both.@wordpress/sync: the undo manager exposesundo()/redo()returning whether a level moved,stopCapturing(), andclearRedo(). Reading or moving the history flushes deferred document updates first. The sync manager exposesisLoaded(), andonUndoStackChangeis replaced byonUndoLevelOpened. The sync manager creates its undo manager when it is created, instead of on the first entity load.core-data no longer mirrors the sync manager's undo state:
syncUndoManagerStateand__unstableNotifySyncUndoManagerChangeare gone.hasUndo/hasRedoread the undo manager, and a placeholder added outside of an entity edit changesundoManagerReferenceso selectors run again.Example
There is no new stack. core-data's regular undo manager (
state.undoManager) record keeps the whole ordered history, and each Yjs level is represented in it by a placeholder record withid :'core/entity-sync-undo-level'. The placeholder'schangesare a counter so the record is never considered empty. After typing in a synced post, editing a non-synced custom entity, then typing in the post again, the undo manager holds:and the Yjs undo stack holds two items. When undo pops the last record, the placeholder is delegated to
undoHistory.undo(), which pops the matching Yjs item and updates the entity through its document. Anything else in the record is dispatched as a normalUNDO. So the three undos above revert the second typing, then the custom entity edit, then the first typing. A placeholder whose Yjs item is gone (its document was unloaded) applies nothing and is skipped.The
level: { from: 0, to: 1 }records saved in the undo manager are needed because the undo manager checks thatfrom/toare not equal values, andlevelis a random attribute that represents "undo level" but otherwise has no meaning. Overall, this is just an incrementing ID and ID + 1, with a pointer to the Yjs stack using thecore/entity-sync-undo-levelID.Testing Instructions
First, reproduce the issue on
trunk. Enable the real-time collaboration experiment in Gutenberg, and add this code to the bottom ofgutenberg.php:This will enable Yjs syncing for every type that RTC supports, except
post. Next:On
fix/rtc-entity-undo, try the same steps withpostexcluded from syncing using the same code. You should see that undo continues to work after the pre-publish panel opens.Automated tests:
Use of AI Tools
Claude code for the whole process.