Fix broken undo history stack for Pattern Overrides (#57088)

* Fix broken undo history stack for Pattern Overrides

* Fix unit test

* Fix unit test

* Revert uneeded changes

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

@@ -263,13 +263,13 @@ describe( 'useBlockSync hook', () => {
 
 		expect( onInput ).toHaveBeenCalledWith(
 			[ { clientId: 'a', innerBlocks: [], attributes: { foo: 2 } } ],
-			{
+			expect.objectContaining( {
 				selection: {
 					selectionEnd: {},
 					selectionStart: {},
 					initialPosition: null,
 				},
-			}
+			} )
 		);
 		expect( onChange ).not.toHaveBeenCalled();
 	} );
@@ -303,13 +303,13 @@ describe( 'useBlockSync hook', () => {
 
 		expect( onChange ).toHaveBeenCalledWith(
 			[ { clientId: 'a', innerBlocks: [], attributes: { foo: 2 } } ],
-			{
+			expect.objectContaining( {
 				selection: {
 					selectionEnd: {},
 					selectionStart: {},
 					initialPosition: null,
 				},
-			}
+			} )
 		);
 		expect( onInput ).not.toHaveBeenCalled();
 	} );
@@ -406,13 +406,13 @@ describe( 'useBlockSync hook', () => {
 					attributes: { foo: 2 },
 				},
 			],
-			{
+			expect.objectContaining( {
 				selection: {
 					selectionEnd: {},
 					selectionStart: {},
 					initialPosition: null,
 				},
-			}
+			} )
 		);
 		expect( onInput ).not.toHaveBeenCalled();
 	} );
@@ -447,13 +447,16 @@ describe( 'useBlockSync hook', () => {
 			{ clientId: 'a', innerBlocks: [], attributes: { foo: 2 } },
 		];
 
-		expect( onChange1 ).toHaveBeenCalledWith( updatedBlocks1, {
-			selection: {
-				initialPosition: null,
-				selectionEnd: {},
-				selectionStart: {},
-			},
-		} );
+		expect( onChange1 ).toHaveBeenCalledWith(
+			updatedBlocks1,
+			expect.objectContaining( {
+				selection: {
+					initialPosition: null,
+					selectionEnd: {},
+					selectionStart: {},
+				},
+			} )
+		);
 
 		const newBlocks = [
 			{ clientId: 'b', innerBlocks: [], attributes: { foo: 1 } },
@@ -485,13 +488,13 @@ describe( 'useBlockSync hook', () => {
 		// The second callback should be called with the new change.
 		expect( onChange2 ).toHaveBeenCalledWith(
 			[ { clientId: 'b', innerBlocks: [], attributes: { foo: 3 } } ],
-			{
+			expect.objectContaining( {
 				selection: {
 					selectionEnd: {},
 					selectionStart: {},
 					initialPosition: null,
 				},
-			}
+			} )
 		);
 	} );
 
@@ -544,13 +547,13 @@ describe( 'useBlockSync hook', () => {
 		// Only the new callback should be called.
 		expect( onChange2 ).toHaveBeenCalledWith(
 			[ { clientId: 'b', innerBlocks: [], attributes: { foo: 3 } } ],
-			{
+			expect.objectContaining( {
 				selection: {
 					selectionEnd: {},
 					selectionStart: {},
 					initialPosition: null,
 				},
-			}
+			} )
 		);
 	} );
 } );

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

@@ -9,6 +9,7 @@ import { cloneBlock } from '@wordpress/blocks';
  * Internal dependencies
  */
 import { store as blockEditorStore } from '../../store';
+import { undoIgnoreBlocks } from '../../store/undo-ignore';
 
 const noop = () => {};
 
@@ -264,13 +265,18 @@ export default function useBlockSync( {
 				const updateParent = isPersistent
 					? onChangeRef.current
 					: onInputRef.current;
+				const undoIgnore = undoIgnoreBlocks.has( blocks );
+				if ( undoIgnore ) {
+					undoIgnoreBlocks.delete( blocks );
+				}
 				updateParent( blocks, {
 					selection: {
 						selectionStart: getSelectionStart(),
 						selectionEnd: getSelectionEnd(),
 						initialPosition:
 							getSelectedBlocksInitialCaretPosition(),
 					},
+					undoIgnore,
 				} );
 			}
 			previousAreBlocksDifferent = areBlocksDifferent;

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

@@ -3,6 +3,11 @@
  */
 import { Platform } from '@wordpress/element';
 
+/**
+ * Internal dependencies
+ */
+import { undoIgnoreBlocks } from './undo-ignore';
+
 const castArray = ( maybeArray ) =>
 	Array.isArray( maybeArray ) ? maybeArray : [ maybeArray ];
 
@@ -291,10 +296,30 @@ export function deleteStyleOverride( id ) {
 	};
 }
 
-export function syncDerivedBlockAttributes( clientId, attributes ) {
-	return {
-		type: 'SYNC_DERIVED_BLOCK_ATTRIBUTES',
-		clientIds: [ clientId ],
-		attributes,
+/**
+ * A higher-order action that mark every change inside a callback as "non-persistent"
+ * and ignore pushing to the undo history stack. It's primarily used for synchronized
+ * derived updates from the block editor without affecting the undo history.
+ *
+ * @param {() => void} callback The synchronous callback to derive updates.
+ */
+export function syncDerivedUpdates( callback ) {
+	return ( { dispatch, select, registry } ) => {
+		registry.batch( () => {
+			// Mark every change in the `callback` as non-persistent.
+			dispatch( {
+				type: 'SET_EXPLICIT_PERSISTENT',
+				isPersistentChange: false,
+			} );
+			callback();
+			dispatch( {
+				type: 'SET_EXPLICIT_PERSISTENT',
+				isPersistentChange: undefined,
+			} );
+
+			// Ignore pushing undo stack for the updated blocks.
+			const updatedBlocks = select.getBlocks();
+			undoIgnoreBlocks.add( updatedBlocks );
+		} );
 	};
 }

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

@@ -453,14 +453,25 @@ const withBlockTree =
 function withPersistentBlockChange( reducer ) {
 	let lastAction;
 	let markNextChangeAsNotPersistent = false;
+	let explicitPersistent;
 
 	return ( state, action ) => {
 		let nextState = reducer( state, action );
 
-		if ( action.type === 'SYNC_DERIVED_BLOCK_ATTRIBUTES' ) {
-			return nextState.isPersistentChange
-				? { ...nextState, isPersistentChange: false }
-				: nextState;
+		let nextIsPersistentChange;
+		if ( action.type === 'SET_EXPLICIT_PERSISTENT' ) {
+			explicitPersistent = action.isPersistentChange;
+			nextIsPersistentChange = state.isPersistentChange ?? true;
+		}
+
+		if ( explicitPersistent !== undefined ) {
+			nextIsPersistentChange = explicitPersistent;
+			return nextIsPersistentChange === nextState.isPersistentChange
+				? nextState
+				: {
+						...nextState,
+						isPersistentChange: nextIsPersistentChange,
+				  };
 		}
 
 		const isExplicitPersistentChange =
@@ -473,7 +484,7 @@ function withPersistentBlockChange( reducer ) {
 			markNextChangeAsNotPersistent =
 				action.type === 'MARK_NEXT_CHANGE_AS_NOT_PERSISTENT';
 
-			const nextIsPersistentChange = state?.isPersistentChange ?? true;
+			nextIsPersistentChange = state?.isPersistentChange ?? true;
 			if ( state.isPersistentChange === nextIsPersistentChange ) {
 				return state;
 			}

packages/block-editor/src/store/undo-ignore.js

@@ -0,0 +1,4 @@
+// Keep track of the blocks that should not be pushing an additional
+// undo stack when editing the entity.
+// See the implementation of `syncDerivedUpdates` and `useBlockSync`.
+export const undoIgnoreBlocks = new WeakSet();

packages/block-library/src/block/edit.js

@@ -137,6 +137,7 @@ export default function ReusableBlockEdit( {
 	attributes: { ref, overrides },
 	__unstableParentLayout: parentLayout,
 	clientId: patternClientId,
+	setAttributes,
 } ) {
 	const registry = useRegistry();
 	const hasAlreadyRendered = useHasRecursion( ref );
@@ -154,7 +155,9 @@ export default function ReusableBlockEdit( {
 		setBlockEditingMode,
 	} = useDispatch( blockEditorStore );
 	const { getBlockEditingMode } = useSelect( blockEditorStore );
+	const { syncDerivedUpdates } = unlock( useDispatch( blockEditorStore ) );
 
+	// Apply the initial overrides from the pattern block to the inner blocks.
 	useEffect( () => {
 		const initialBlocks =
 			editedRecord.blocks ??
@@ -164,17 +167,19 @@ export default function ReusableBlockEdit( {
 
 		defaultValuesRef.current = {};
 		const editingMode = getBlockEditingMode( patternClientId );
+		// Replace the contents of the blocks with the overrides.
 		registry.batch( () => {
 			setBlockEditingMode( patternClientId, 'default' );
-			__unstableMarkNextChangeAsNotPersistent();
-			replaceInnerBlocks(
-				patternClientId,
-				applyInitialOverrides(
-					initialBlocks,
-					initialOverrides.current,
-					defaultValuesRef.current
-				)
-			);
+			syncDerivedUpdates( () => {
+				replaceInnerBlocks(
+					patternClientId,
+					applyInitialOverrides(
+						initialBlocks,
+						initialOverrides.current,
+						defaultValuesRef.current
+					)
+				);
+			} );
 			setBlockEditingMode( patternClientId, editingMode );
 		} );
 	}, [
@@ -185,6 +190,7 @@ export default function ReusableBlockEdit( {
 		registry,
 		getBlockEditingMode,
 		setBlockEditingMode,
+		syncDerivedUpdates,
 	] );
 
 	const innerBlocks = useSelect(
@@ -220,29 +226,26 @@ export default function ReusableBlockEdit( {
 			: InnerBlocks.ButtonBlockAppender,
 	} );
 
-	// Sync the `overrides` attribute from the updated blocks.
-	// `syncDerivedBlockAttributes` is an action that just like `updateBlockAttributes`
-	// but won't create an undo level.
-	// This can be abstracted into a `useSyncDerivedAttributes` hook if needed.
+	// Sync the `overrides` attribute from the updated blocks to the pattern block.
+	// `syncDerivedUpdates` is used here to avoid creating an additional undo level.
 	useEffect( () => {
 		const { getBlocks } = registry.select( blockEditorStore );
-		const { syncDerivedBlockAttributes } = unlock(
-			registry.dispatch( blockEditorStore )
-		);
 		let prevBlocks = getBlocks( patternClientId );
 		return registry.subscribe( () => {
 			const blocks = getBlocks( patternClientId );
 			if ( blocks !== prevBlocks ) {
 				prevBlocks = blocks;
-				syncDerivedBlockAttributes( patternClientId, {
-					overrides: getOverridesFromBlocks(
-						blocks,
-						defaultValuesRef.current
-					),
+				syncDerivedUpdates( () => {
+					setAttributes( {
+						overrides: getOverridesFromBlocks(
+							blocks,
+							defaultValuesRef.current
+						),
+					} );
 				} );
 			}
 		}, blockEditorStore );
-	}, [ patternClientId, registry ] );
+	}, [ syncDerivedUpdates, patternClientId, registry, setAttributes ] );
 
 	let children = null;
 

packages/core-data/src/entity-provider.js

@@ -196,7 +196,7 @@ export function useEntityBlockEditor( kind, name, { id: _id } = {} ) {
 			if ( noChange ) {
 				return __unstableCreateUndoLevel( kind, name, id );
 			}
-			const { selection } = options;
+			const { selection, ...rest } = options;
 
 			// We create a new function here on every persistent edit
 			// to make sure the edit makes the post dirty and creates
@@ -208,7 +208,10 @@ export function useEntityBlockEditor( kind, name, { id: _id } = {} ) {
 				...updateFootnotes( newBlocks ),
 			};
 
-			editEntityRecord( kind, name, id, edits, { isCached: false } );
+			editEntityRecord( kind, name, id, edits, {
+				isCached: false,
+				...rest,
+			} );
 		},
 		[
 			kind,
@@ -223,11 +226,14 @@ export function useEntityBlockEditor( kind, name, { id: _id } = {} ) {
 
 	const onInput = useCallback(
 		( newBlocks, options ) => {
-			const { selection } = options;
+			const { selection, ...rest } = options;
 			const footnotesChanges = updateFootnotes( newBlocks );
 			const edits = { selection, ...footnotesChanges };
 
-			editEntityRecord( kind, name, id, edits, { isCached: true } );
+			editEntityRecord( kind, name, id, edits, {
+				isCached: true,
+				...rest,
+			} );
 		},
 		[ kind, name, id, updateFootnotes, editEntityRecord ]
 	);