Fix multi-entity multi-property undo redo

youknowriad · youknowriad · commit b0a3292e3f47 · 2023-05-24T15:33:40.000+01:00
diff --git a/packages/core-data/src/actions.js b/packages/core-data/src/actions.js
@@ -411,9 +411,8 @@ export const undo =
 			return;
 		}
 		dispatch( {
-			type: 'EDIT_ENTITY_RECORD',
-			...undoEdit,
-			meta: { isUndo: true },
+			type: 'UNDO',
+			stackedEdits: undoEdit,
 		} );
 	};
 
@@ -429,9 +428,8 @@ export const redo =
 			return;
 		}
 		dispatch( {
-			type: 'EDIT_ENTITY_RECORD',
-			...redoEdit,
-			meta: { isRedo: true },
+			type: 'REDO',
+			stackedEdits: redoEdit,
 		} );
 	};
 
diff --git a/packages/core-data/src/reducer.js b/packages/core-data/src/reducer.js
@@ -183,6 +183,26 @@ export function themeGlobalStyleVariations( state = {}, action ) {
 	return state;
 }
 
+const withMultiEntityRecordEdits = ( reducer ) => ( state, action ) => {
+	if ( action.type === 'UNDO' || action.type === 'REDO' ) {
+		const { stackedEdits } = action;
+
+		let newState = state;
+		stackedEdits.forEach( ( { kind, name, recordId, edits } ) => {
+			newState = reducer( newState, {
+				type: 'EDIT_ENTITY_RECORD',
+				kind,
+				name,
+				recordId,
+				edits,
+			} );
+		} );
+		return newState;
+	}
+
+	return reducer( state, action );
+};
+
 /**
  * Higher Order Reducer for a given entity config. It supports:
  *
@@ -196,6 +216,8 @@ export function themeGlobalStyleVariations( state = {}, action ) {
  */
 function entity( entityConfig ) {
 	return compose( [
+		withMultiEntityRecordEdits,
+
 		// Limit to matching action type so we don't attempt to replace action on
 		// an unhandled action.
 		ifMatchingAction(
@@ -411,8 +433,9 @@ export const entities = ( state = {}, action ) => {
 /**
  * @typedef {Object} UndoStateMeta
  *
- * @property {number} offset          Where in the undo stack we are.
- * @property {Object} [flattenedUndo] Flattened form of undo stack.
+ * @property {number} list   The undo stack.
+ * @property {number} offset Where in the undo stack we are.
+ * @property {Object} cache  Cache of unpersisted transient edits.
  */
 
 /** @typedef {Array<Object> & UndoStateMeta} UndoState */
@@ -422,10 +445,7 @@ export const entities = ( state = {}, action ) => {
  *
  * @todo Given how we use this we might want to make a custom class for it.
  */
-const UNDO_INITIAL_STATE = Object.assign( [], { offset: 0 } );
-
-/** @type {Object} */
-let lastEditAction;
+const UNDO_INITIAL_STATE = { list: [], offset: 0 };
 
 /**
  * Reducer keeping track of entity edit undo history.
@@ -436,107 +456,103 @@ let lastEditAction;
  * @return {UndoState} Updated state.
  */
 export function undo( state = UNDO_INITIAL_STATE, action ) {
+	const omitPendingRedos = ( currentState ) => {
+		return {
+			...currentState,
+			list: currentState.list.slice(
+				0,
+				currentState.offset || undefined
+			),
+			offset: 0,
+		};
+	};
+
+	const appendCachedEditsToLastUndo = ( currentState ) => {
+		if ( ! currentState.cache ) {
+			return currentState;
+		}
+
+		let nextState = {
+			...currentState,
+			list: [ ...currentState.list ],
+		};
+		nextState = omitPendingRedos( nextState );
+		let previousUndoState = nextState.list.pop();
+		currentState.cache.forEach( ( edit ) => {
+			previousUndoState = appendEditsToStack( previousUndoState, edit );
+		} );
+		nextState.list.push( previousUndoState );
+
+		return {
+			...nextState,
+			cache: undefined,
+		};
+	};
+
+	const appendEditsToStack = (
+		stack = [],
+		{ kind, name, recordId, edits }
+	) => {
+		const existingEditIndex = stack?.findIndex(
+			( { kind: k, name: n, recordId: r } ) => {
+				return k === kind && n === name && r === recordId;
+			}
+		);
+
+		const nextStack = stack.filter(
+			( _, index ) => index !== existingEditIndex
+		);
+		nextStack.push( {
+			kind,
+			name,
+			recordId,
+			edits: {
+				...( existingEditIndex !== -1
+					? stack[ existingEditIndex ].edits
+					: {} ),
+				...edits,
+			},
+		} );
+		return nextStack;
+	};
+
 	switch ( action.type ) {
-		case 'EDIT_ENTITY_RECORD':
 		case 'CREATE_UNDO_LEVEL':
-			let isCreateUndoLevel = action.type === 'CREATE_UNDO_LEVEL';
-			const isUndoOrRedo =
-				! isCreateUndoLevel &&
-				( action.meta.isUndo || action.meta.isRedo );
-			if ( isCreateUndoLevel ) {
-				action = lastEditAction;
-			} else if ( ! isUndoOrRedo ) {
-				// Don't lose the last edit cache if the new one only has transient edits.
-				// Transient edits don't create new levels so updating the cache would make
-				// us skip an edit later when creating levels explicitly.
-				if (
-					Object.keys( action.edits ).some(
-						( key ) => ! action.transientEdits[ key ]
-					)
-				) {
-					lastEditAction = action;
-				} else {
-					lastEditAction = {
-						...action,
-						edits: {
-							...( lastEditAction && lastEditAction.edits ),
-							...action.edits,
-						},
-					};
-				}
-			}
+			return appendCachedEditsToLastUndo( state );
 
-			/** @type {UndoState} */
-			let nextState;
-
-			if ( isUndoOrRedo ) {
-				// @ts-ignore we might consider using Object.assign({}, state)
-				nextState = [ ...state ];
-				nextState.offset =
-					state.offset + ( action.meta.isUndo ? -1 : 1 );
-
-				if ( state.flattenedUndo ) {
-					// The first undo in a sequence of undos might happen while we have
-					// flattened undos in state. If this is the case, we want execution
-					// to continue as if we were creating an explicit undo level. This
-					// will result in an extra undo level being appended with the flattened
-					// undo values.
-					// We also have to take into account if the `lastEditAction` had opted out
-					// of being tracked in undo history, like the action that persists the latest
-					// content right before saving. In that case we have to update the `lastEditAction`
-					// to avoid returning early before applying the existing flattened undos.
-					isCreateUndoLevel = true;
-					if ( ! lastEditAction.meta.undo ) {
-						lastEditAction.meta.undo = {
-							edits: {},
-						};
-					}
-					action = lastEditAction;
-				} else {
-					return nextState;
-				}
-			}
+		case 'UNDO':
+		case 'REDO': {
+			const nextState = appendCachedEditsToLastUndo( state );
+			return {
+				...nextState,
+				offset: state.offset + ( action.type === 'UNDO' ? -1 : 1 ),
+			};
+		}
 
-			if ( ! action.meta.undo ) {
-				return state;
-			}
+		case 'EDIT_ENTITY_RECORD':
+			const isCachedChange = Object.keys( action.edits ).every(
+				( key ) => action.transientEdits[ key ]
+			);
 
-			// Transient edits don't create an undo level, but are
-			// reachable in the next meaningful edit to which they
-			// are merged. They are defined in the entity's config.
-			if (
-				! isCreateUndoLevel &&
-				! Object.keys( action.edits ).some(
-					( key ) => ! action.transientEdits[ key ]
-				)
-			) {
-				// @ts-ignore we might consider using Object.assign({}, state)
-				nextState = [ ...state ];
-				nextState.flattenedUndo = {
-					...state.flattenedUndo,
-					...action.edits,
+			if ( isCachedChange ) {
+				return {
+					...state,
+					cache: appendEditsToStack( state.cache, action ),
 				};
-				nextState.offset = state.offset;
-				return nextState;
 			}
 
-			// Clear potential redos, because this only supports linear history.
-			nextState =
-				// @ts-ignore this needs additional cleanup, probably involving code-level changes
-				nextState || state.slice( 0, state.offset || undefined );
-			nextState.offset = nextState.offset || 0;
-			nextState.pop();
-			if ( ! isCreateUndoLevel ) {
-				nextState.push( {
-					kind: action.meta.undo.kind,
-					name: action.meta.undo.name,
-					recordId: action.meta.undo.recordId,
-					edits: {
-						...state.flattenedUndo,
-						...action.meta.undo.edits,
-					},
-				} );
-			}
+			let nextState = omitPendingRedos( state );
+			nextState = appendCachedEditsToLastUndo( nextState );
+			nextState = { ...nextState, list: [ ...nextState.list ] };
+			const previousUndoState = nextState.list.pop();
+			nextState.list.push(
+				appendEditsToStack( previousUndoState, {
+					kind: action.kind,
+					name: action.name,
+					recordId: action.recordId,
+					edits: action.meta.undo.edits,
+				} )
+			);
 			// When an edit is a function it's an optimization to avoid running some expensive operation.
 			// We can't rely on the function references being the same so we opt out of comparing them here.
 			const comparisonUndoEdits = Object.values(
@@ -546,15 +562,16 @@ export function undo( state = UNDO_INITIAL_STATE, action ) {
 				( edit ) => typeof edit !== 'function'
 			);
 			if ( ! isShallowEqual( comparisonUndoEdits, comparisonEdits ) ) {
-				nextState.push( {
-					kind: action.kind,
-					name: action.name,
-					recordId: action.recordId,
-					edits: isCreateUndoLevel
-						? { ...state.flattenedUndo, ...action.edits }
-						: action.edits,
-				} );
+				nextState.list.push( [
+					{
+						kind: action.kind,
+						name: action.name,
+						recordId: action.recordId,
+						edits: action.edits,
+					},
+				] );
 			}
+
 			return nextState;
 	}
 
diff --git a/packages/core-data/src/selectors.ts b/packages/core-data/src/selectors.ts
@@ -73,8 +73,8 @@ interface EntityConfig {
 	kind: string;
 }
 
-interface UndoState extends Array< Object > {
-	flattenedUndo: unknown;
+interface UndoState {
+	list: Array< Object >;
 	offset: number;
 }
 
@@ -889,7 +889,9 @@ function getCurrentUndoOffset( state: State ): number {
  * @return The edit.
  */
 export function getUndoEdit( state: State ): Optional< any > {
-	return state.undo[ state.undo.length - 2 + getCurrentUndoOffset( state ) ];
+	return state.undo.list[
+		state.undo.list.length - 2 + getCurrentUndoOffset( state )
+	];
 }
 
 /**
@@ -901,7 +903,9 @@ export function getUndoEdit( state: State ): Optional< any > {
  * @return The edit.
  */
 export function getRedoEdit( state: State ): Optional< any > {
-	return state.undo[ state.undo.length + getCurrentUndoOffset( state ) ];
+	return state.undo.list[
+		state.undo.list.length + getCurrentUndoOffset( state )
+	];
 }
 
 /**
@@ -1142,11 +1146,7 @@ export const hasFetchedAutosaves = createRegistrySelector(
 export const getReferenceByDistinctEdits = createSelector(
 	// This unused state argument is listed here for the documentation generating tool (docgen).
 	( state: State ) => [],
-	( state: State ) => [
-		state.undo.length,
-		state.undo.offset,
-		state.undo.flattenedUndo,
-	]
+	( state: State ) => [ state.undo.list.length, state.undo.offset ]
 );
 
 /**
diff --git a/packages/core-data/src/test/reducer.js b/packages/core-data/src/test/reducer.js
