RTC: Fix Yjs undo manager to update UI state when undo stack changes

packages/block-editor/src/components/inner-blocks/use-inner-block-template-sync.js

@@ -91,7 +91,9 @@ export default function useInnerBlockTemplateSync(
 			);
 
 			if ( ! fastDeepEqual( nextBlocks, currentInnerBlocks ) ) {
-				__unstableMarkNextChangeAsNotPersistent();
+				__unstableMarkNextChangeAsNotPersistent( {
+					history: 'ignore',
+				} );
 				replaceInnerBlocks(
 					clientId,
 					nextBlocks,

packages/block-editor/src/components/provider/test/use-block-sync.js

@@ -277,6 +277,46 @@ describe( 'useBlockSync hook', () => {
 		expect( onChange ).not.toHaveBeenCalled();
 	} );
 
+	it( 'passes undoIgnore when a non-persistent block change ignores history', async () => {
+		const onChange = jest.fn();
+		const onInput = jest.fn();
+		const value1 = [
+			{ clientId: 'a', innerBlocks: [], attributes: { foo: 1 } },
+		];
+		let registry;
+		const setRegistry = ( reg ) => {
+			registry = reg;
+		};
+		render(
+			<TestWrapper
+				setRegistry={ setRegistry }
+				value={ value1 }
+				onChange={ onChange }
+				onInput={ onInput }
+			/>
+		);
+		onChange.mockClear();
+		onInput.mockClear();
+
+		registry
+			.dispatch( blockEditorStore )
+			.__unstableMarkNextChangeAsNotPersistent( {
+				history: 'ignore',
+			} );
+		registry
+			.dispatch( blockEditorStore )
+			.updateBlockAttributes( 'a', { foo: 2 } );
+
+		expect( onInput ).toHaveBeenCalledWith(
+			[ { clientId: 'a', innerBlocks: [], attributes: { foo: 2 } } ],
+			expect.objectContaining( {
+				selection: expect.any( Object ),
+				undoIgnore: true,
+			} )
+		);
+		expect( onChange ).not.toHaveBeenCalled();
+	} );
+
 	it( 'calls onChange if a persistent change occurs', async () => {
 		const onChange = jest.fn();
 		const onInput = jest.fn();

packages/block-editor/src/components/provider/use-block-sync.js

@@ -342,13 +342,15 @@ export default function useBlockSync( {
 		const {
 			getSelectedBlocksInitialCaretPosition,
 			isLastBlockChangePersistent,
+			__unstableGetLastBlockChangeHistoryMode,
 			__unstableIsLastBlockChangeIgnored,
 			areInnerBlocksControlled,
 			getBlockParents,
 		} = registry.select( blockEditorStore );
 
 		let blocks = getBlocks( clientId );
 		let isPersistent = isLastBlockChangePersistent();
+		let blockHistoryMode = __unstableGetLastBlockChangeHistoryMode();
 		let previousAreBlocksDifferent = false;
 		let prevSelectionStart = getSelectionStart();
 		let prevSelectionEnd = getSelectionEnd();
@@ -367,6 +369,8 @@ export default function useBlockSync( {
 			}
 
 			const newIsPersistent = isLastBlockChangePersistent();
+			const newBlockHistoryMode =
+				__unstableGetLastBlockChangeHistoryMode();
 			const newBlocks = getBlocks( clientId );
 			const areBlocksDifferent = newBlocks !== blocks;
 			blocks = newBlocks;
@@ -377,6 +381,7 @@ export default function useBlockSync( {
 			) {
 				pendingChangesRef.current.incoming = null;
 				isPersistent = newIsPersistent;
+				blockHistoryMode = newBlockHistoryMode;
 				return;
 			}
 
@@ -409,6 +414,7 @@ export default function useBlockSync( {
 				registry.batch( () => {
 					if ( blocksChanged ) {
 						isPersistent = newIsPersistent;
+						blockHistoryMode = newBlockHistoryMode;
 
 						// For inner block controllers (clientId is set), restore external IDs
 						// before passing blocks to the parent.
@@ -438,9 +444,13 @@ export default function useBlockSync( {
 						const updateParent = isPersistent
 							? onChangeRef.current
 							: onInputRef.current;
-						updateParent( blocksForParent, {
+						const updateOptions = {
 							selection: selectionForParent,
-						} );
+						};
+						if ( blockHistoryMode === 'ignore' ) {
+							updateOptions.undoIgnore = true;
+						}
+						updateParent( blocksForParent, updateOptions );
 					}
 
 					if (

packages/block-editor/src/store/actions.js

@@ -1646,12 +1646,20 @@ export function __unstableMarkLastChangeAsPersistent() {
 }
 
 /**
- * Action that signals that the next block change should be marked explicitly as not persistent.
+ * Action that signals that the next block change should be marked explicitly
+ * as not persistent.
  *
+ * By default, non-persistent changes may still merge into undo history. Use
+ * `history: 'ignore'` for derived changes that should never be captured by undo.
+ *
+ * @param {Object} options         Options object.
+ * @param {string} options.history How the change should interact with history.
  * @return {Object} Action object.
  */
-export function __unstableMarkNextChangeAsNotPersistent() {
-	return { type: 'MARK_NEXT_CHANGE_AS_NOT_PERSISTENT' };
+export function __unstableMarkNextChangeAsNotPersistent( {
+	history = 'merge',
+} = {} ) {
+	return { type: 'MARK_NEXT_CHANGE_AS_NOT_PERSISTENT', history };
 }
 
 /**

packages/block-editor/src/store/reducer.js

@@ -419,18 +419,20 @@ const withBlockTree =
  */
 function withPersistentBlockChange( reducer ) {
 	let lastAction;
-	let markNextChangeAsNotPersistent = false;
+	let nextHistoryMode;
 
 	return ( state, action ) => {
 		const nextState = reducer( state, action );
 
-		const wasMarkedAsNotPersistent = markNextChangeAsNotPersistent;
-		markNextChangeAsNotPersistent =
-			action.type === 'MARK_NEXT_CHANGE_AS_NOT_PERSISTENT';
+		const pendingHistoryMode = nextHistoryMode;
+		nextHistoryMode =
+			action.type === 'MARK_NEXT_CHANGE_AS_NOT_PERSISTENT'
+				? action.history ?? 'merge'
+				: undefined;
 
 		const isExplicitPersistentChange =
 			action.type === 'MARK_LAST_CHANGE_AS_PERSISTENT' ||
-			wasMarkedAsNotPersistent;
+			pendingHistoryMode;
 
 		// Defer to previous state value (or default) unless changing or
 		// explicitly marking as persistent.
@@ -442,15 +444,25 @@ function withPersistentBlockChange( reducer ) {
 		}
 
 		const isPersistentChange = isExplicitPersistentChange
-			? ! wasMarkedAsNotPersistent
+			? ! pendingHistoryMode
 			: ! isUpdatingSameBlockAttribute( action, lastAction );
 
 		// In comparing against the previous action, consider only those which
 		// would have qualified as one which would have been ignored or not
 		// have resulted in a changed state.
 		lastAction = action;
 
-		return { ...nextState, isPersistentChange };
+		if ( pendingHistoryMode === 'ignore' ) {
+			return {
+				...nextState,
+				isPersistentChange,
+				lastBlockChangeHistoryMode: 'ignore',
+			};
+		}
+
+		const { lastBlockChangeHistoryMode, ...blockChange } = nextState;
+
+		return { ...blockChange, isPersistentChange };
 	};
 }
 

packages/block-editor/src/store/selectors.js

@@ -2930,6 +2930,25 @@ export function isLastBlockChangePersistent( state ) {
 	return state.blocks.isPersistentChange;
 }
 
+/**
+ * Returns how the most recent block change interacts with undo history.
+ *
+ * - `persistent` changes create a new undo level.
+ * - `merge` changes do not create a new undo level, but may merge into the
+ *    prior stack item history.
+ * - `ignore` changes should never be captured by undo history.
+ *
+ * @param {Object} state Block editor state.
+ *
+ * @return {'persistent'|'merge'|'ignore'} Block change history behavior.
+ */
+export function __unstableGetLastBlockChangeHistoryMode( state ) {
+	if ( state.blocks.lastBlockChangeHistoryMode ) {
+		return state.blocks.lastBlockChangeHistoryMode;
+	}
+	return state.blocks.isPersistentChange === false ? 'merge' : 'persistent';
+}
+
 /**
  * Returns the block list settings for an array of blocks, if any exist.
  *

packages/block-editor/src/store/test/actions.js

@@ -53,6 +53,7 @@ const {
 	updateBlockListSettings,
 	updateSettings,
 	validateBlocksToTemplate,
+	__unstableMarkNextChangeAsNotPersistent,
 	registerInserterMediaCategory,
 	setBlockEditingMode,
 	unsetBlockEditingMode,
@@ -814,6 +815,26 @@ describe( 'actions', () => {
 		} );
 	} );
 
+	describe( '__unstableMarkNextChangeAsNotPersistent', () => {
+		it( 'should use merge history by default', () => {
+			expect( __unstableMarkNextChangeAsNotPersistent() ).toEqual( {
+				type: 'MARK_NEXT_CHANGE_AS_NOT_PERSISTENT',
+				history: 'merge',
+			} );
+		} );
+
+		it( 'should allow ignoring history for the next change', () => {
+			expect(
+				__unstableMarkNextChangeAsNotPersistent( {
+					history: 'ignore',
+				} )
+			).toEqual( {
+				type: 'MARK_NEXT_CHANGE_AS_NOT_PERSISTENT',
+				history: 'ignore',
+			} );
+		} );
+	} );
+
 	describe( 'mergeBlocks', () => {
 		afterEach( () => {
 			getBlockTypes().forEach( ( block ) => {

packages/block-editor/src/store/test/reducer.js

@@ -2236,6 +2236,52 @@ describe( 'state', () => {
 					expect( subsequentState.isPersistentChange ).toBe( true );
 				} );
 
+				it( 'should flag ignored history for an explicitly marked not persistent change', () => {
+					let original = deepFreeze(
+						blocks( undefined, {
+							type: 'RESET_BLOCKS',
+							blocks: [
+								{
+									clientId: 'kumquat',
+									attributes: {},
+									innerBlocks: [],
+								},
+							],
+						} )
+					);
+					original = blocks( original, {
+						type: 'MARK_NEXT_CHANGE_AS_NOT_PERSISTENT',
+						history: 'ignore',
+					} );
+
+					const nextState = blocks( original, {
+						type: 'UPDATE_BLOCK_ATTRIBUTES',
+						clientIds: [ 'kumquat' ],
+						attributes: {
+							updated: true,
+						},
+					} );
+
+					expect( nextState.isPersistentChange ).toBe( false );
+					expect( nextState.lastBlockChangeHistoryMode ).toBe(
+						'ignore'
+					);
+
+					// A subsequent change should clear the ignored history.
+					const subsequentState = blocks( nextState, {
+						type: 'UPDATE_BLOCK_ATTRIBUTES',
+						clientIds: [ 'kumquat' ],
+						attributes: {
+							other: true,
+						},
+					} );
+
+					expect( subsequentState.isPersistentChange ).toBe( true );
+					expect(
+						subsequentState.lastBlockChangeHistoryMode
+					).toBeUndefined();
+				} );
+
 				it( 'should retain reference for same state, same persistence', () => {
 					const original = deepFreeze(
 						blocks( undefined, {

packages/core-data/src/private-actions.js

@@ -197,6 +197,24 @@ export function receiveViewConfig( kind, name, config ) {
 	};
 }
 
+/**
+ * Returns an action object used to notify core-data that the sync undo manager
+ * state changed outside of the core-data reducer, e.g. The Yjs UndoManager
+ * captured an undo level.
+ *
+ * @param {Object}  state         The sync undo stack state.
+ * @param {boolean} state.hasRedo Whether there are changes to redo.
+ * @param {boolean} state.hasUndo Whether there are changes to undo.
+ *
+ * @return {Object} Action object.
+ */
+export function __unstableNotifySyncUndoManagerChange( state ) {
+	return {
+		type: 'SYNC_UNDO_MANAGER_CHANGE',
+		...state,
+	};
+}
+
 /**
  * Returns an action object used to set the sync connection status for an entity or collection.
  *

packages/core-data/src/private-selectors.ts

@@ -21,18 +21,6 @@ const EMPTY_OBJECT = {};
  * Returns the previous edit from the current undo offset
  * for the entity records edits history, if any.
  *
- * Known Issue: Every-time state.undoManager changes, the getUndoManager
- * private selector is called (if used within useSelect and things like that)
- * which ensures the UI is always properly reactive. But, it's not the case with
- * the custom "sync" undo manager.
- *
- * Assumption: When an undo/redo is created, other parts of the core-data state
- * are likely changing simultaneously, which will trigger the selectors again.
- *
- * This issue is acceptable based on the assumption above.
- *
- * @see https://github.com/WordPress/gutenberg/pull/72407/files#r2580214235 for more details.
- *
  * @param state State tree.
  *
  * @return The undo manager.

packages/core-data/src/reducer.js

@@ -460,6 +460,22 @@ export function undoManager( state = createUndoManager() ) {
 	return state;
 }
 
+// Stores a snapshot of the sync undo manager's undo/redo availability so
+// core-data selectors can react to undo stack changes.
+export function syncUndoManagerState(
+	state = { hasRedo: false, hasUndo: false },
+	action
+) {
+	switch ( action.type ) {
+		case 'SYNC_UNDO_MANAGER_CHANGE':
+			return {
+				hasRedo: action.hasRedo,
+				hasUndo: action.hasUndo,
+			};
+	}
+	return state;
+}
+
 export function editsReference( state = {}, action ) {
 	switch ( action.type ) {
 		case 'EDIT_ENTITY_RECORD':
@@ -750,6 +766,7 @@ export default combineReducers( {
 	themeGlobalStyleRevisions,
 	entities,
 	editsReference,
+	syncUndoManagerState,
 	undoManager,
 	embedPreviews,
 	userPermissions,

packages/core-data/src/resolvers.js

@@ -287,6 +287,11 @@ export const getEntityRecord =
 								);
 							}
 						},
+						onUndoStackChange: ( undoState ) => {
+							dispatch.__unstableNotifySyncUndoManagerChange(
+								undoState
+							);
+						},
 						restoreUndoMeta: ( ydoc, meta ) => {
 							const selectionHistory =
 								meta.get( 'selectionHistory' );