feat: Improve undo/redo (#823)

* fix(reducer): Include plugins’ state in undo/redo stack

* feat(reducer): Include playerID in undo/redo stack

* feat(reducer): Prevent a player from undoing another player’s action

* feat(reducer): Prevent a player from redoing another player’s undo

* refactor(reducer): Factor out hasPlayerID check for actions

* fix(master): Improve undo/redo checks

Fixes #565

Author: delucis

src/core/flow.test.js

@@ -201,34 +201,6 @@ describe('phases', () => {
 });
 
 describe('turn', () => {
-  test('onBegin with undoable feature', () => {
-    const onBegin = jest.fn(G => G);
-    const flow = Flow({
-      turn: { onBegin },
-      disableUndo: false,
-    });
-    const state = { ctx: flow.ctx(2) };
-
-    expect(onBegin).not.toHaveBeenCalled();
-    const updatedState = flow.init(state);
-    expect(onBegin).toHaveBeenCalled();
-    expect(updatedState._undo).toHaveLength(1);
-  });
-
-  test('onBegin without undoable feature', () => {
-    const onBegin = jest.fn(G => G);
-    const flow = Flow({
-      turn: { onBegin },
-      disableUndo: true,
-    });
-    const state = { ctx: flow.ctx(2) };
-
-    expect(onBegin).not.toHaveBeenCalled();
-    const updatedState = flow.init(state);
-    expect(onBegin).toHaveBeenCalled();
-    expect(updatedState._undo).toHaveLength(0);
-  });
-
   test('onEnd', () => {
     const onEnd = jest.fn(G => G);
     const flow = Flow({

src/core/flow.ts

@@ -43,7 +43,6 @@ export function Flow({
   turn,
   events,
   plugins,
-  disableUndo,
 }: Game) {
   // Attach defaults.
   if (moves === undefined) {
@@ -297,9 +296,7 @@ export function Flow({
 
     G = conf.turn.wrapped.onBegin({ ...state, G, ctx });
 
-    const _undo = disableUndo ? [] : [{ G, ctx }];
-
-    return { ...state, G, ctx, _undo, _redo: [] };
+    return { ...state, G, ctx, _undo: [], _redo: [] } as State;
   }
 
   ////////////

src/core/initialize.ts

@@ -63,5 +63,16 @@ export function InitializeGame({
   initial = game.flow.init(initial);
   initial = plugins.Flush(initial, { game });
 
+  // Initialize undo stack.
+  if (!game.disableUndo) {
+    initial._undo = [
+      {
+        G: initial.G,
+        ctx: initial.ctx,
+        plugins: initial.plugins,
+      },
+    ];
+  }
+
   return initial;
 }

src/core/reducer.test.ts

@@ -314,10 +314,10 @@ test('undo / redo', () => {
 
   let state = InitializeGame({ game });
 
-  state = reducer(state, makeMove('move', 'A'));
+  state = reducer(state, makeMove('move', 'A', '0'));
   expect(state.G).toMatchObject({ A: true });
 
-  state = reducer(state, makeMove('move', 'B'));
+  state = reducer(state, makeMove('move', 'B', '0'));
   expect(state.G).toMatchObject({ A: true, B: true });
   expect(state._undo[1].ctx.events).toBeUndefined();
   expect(state._undo[1].ctx.random).toBeUndefined();
@@ -340,7 +340,7 @@ test('undo / redo', () => {
   expect(state.G).toEqual({});
 
   state = reducer(state, redo());
-  state = reducer(state, makeMove('move', 'C'));
+  state = reducer(state, makeMove('move', 'C', '0'));
   expect(state.G).toMatchObject({ A: true, C: true });
 
   state = reducer(state, undo());
@@ -351,7 +351,7 @@ test('undo / redo', () => {
 
   state = reducer(state, undo());
   state = reducer(state, undo());
-  state = reducer(state, makeMove('roll'));
+  state = reducer(state, makeMove('roll', null, '0'));
   expect(state.G).toMatchObject({ roll: 4 });
 
   state = reducer(state, undo());
@@ -377,12 +377,12 @@ test('disable undo / redo', () => {
 
   let state = InitializeGame({ game });
 
-  state = reducer(state, makeMove('move', 'A'));
+  state = reducer(state, makeMove('move', 'A', '0'));
   expect(state.G).toMatchObject({ A: true });
   expect(state._undo).toEqual([]);
   expect(state._redo).toEqual([]);
 
-  state = reducer(state, makeMove('move', 'B'));
+  state = reducer(state, makeMove('move', 'B', '0'));
   expect(state.G).toMatchObject({ A: true, B: true });
   expect(state._undo).toEqual([]);
   expect(state._redo).toEqual([]);
@@ -419,33 +419,49 @@ describe('undo stack', () => {
   test('contains initial state at start of game', () => {
     expect(state._undo).toHaveLength(1);
     expect(state._undo[0].ctx).toEqual(state.ctx);
+    expect(state._undo[0].plugins).toEqual(state.plugins);
   });
 
   test('grows when a move is made', () => {
-    state = reducer(state, makeMove('basic'));
+    state = reducer(state, makeMove('basic', null, '0'));
     expect(state._undo).toHaveLength(2);
     expect(state._undo[1].moveType).toBe('basic');
     expect(state._undo[1].ctx).toEqual(state.ctx);
+    expect(state._undo[1].plugins).toEqual(state.plugins);
   });
 
   test('shrinks when a move is undone', () => {
     state = reducer(state, undo());
     expect(state._undo).toHaveLength(1);
     expect(state._undo[0].ctx).toEqual(state.ctx);
+    expect(state._undo[0].plugins).toEqual(state.plugins);
   });
 
   test('grows when a move is redone', () => {
     state = reducer(state, redo());
     expect(state._undo).toHaveLength(2);
     expect(state._undo[1].moveType).toBe('basic');
     expect(state._undo[1].ctx).toEqual(state.ctx);
+    expect(state._undo[1].plugins).toEqual(state.plugins);
   });
 
   test('is reset when a turn ends', () => {
     state = reducer(state, makeMove('endTurn'));
     expect(state._undo).toHaveLength(1);
     expect(state._undo[0].ctx).toEqual(state.ctx);
-    expect(state._undo[0].moveType).toBeUndefined();
+    expect(state._undo[0].plugins).toEqual(state.plugins);
+    expect(state._undo[0].moveType).toBe('endTurn');
+  });
+
+  test('can’t undo at the start of a turn', () => {
+    const newState = reducer(state, undo());
+    expect(state).toEqual(newState);
+  });
+
+  test('can’t undo another player’s move', () => {
+    state = reducer(state, makeMove('basic', null, '1'));
+    const newState = reducer(state, undo('0'));
+    expect(state).toEqual(newState);
   });
 });
 
@@ -467,7 +483,7 @@ describe('redo stack', () => {
   });
 
   test('grows when a move is undone', () => {
-    state = reducer(state, makeMove('basic'));
+    state = reducer(state, makeMove('basic', null, '0'));
     state = reducer(state, undo());
     expect(state._redo).toHaveLength(1);
     expect(state._redo[0].moveType).toBe('basic');
@@ -479,21 +495,30 @@ describe('redo stack', () => {
   });
 
   test('is reset when a move is made', () => {
-    state = reducer(state, makeMove('basic'));
+    state = reducer(state, makeMove('basic', null, '0'));
     state = reducer(state, undo());
     state = reducer(state, undo());
     expect(state._redo).toHaveLength(2);
-    state = reducer(state, makeMove('basic'));
+    state = reducer(state, makeMove('basic', null, '0'));
     expect(state._redo).toHaveLength(0);
   });
 
   test('is reset when a turn ends', () => {
-    state = reducer(state, makeMove('basic'));
+    state = reducer(state, makeMove('basic', null, '0'));
     state = reducer(state, undo());
     expect(state._redo).toHaveLength(1);
     state = reducer(state, makeMove('endTurn'));
     expect(state._redo).toHaveLength(0);
   });
+
+  test('can’t redo another player’s undo', () => {
+    state = reducer(state, makeMove('basic', null, '1'));
+    state = reducer(state, undo('1'));
+    expect(state._redo).toHaveLength(1);
+    const newState = reducer(state, redo('0'));
+    expect(state._redo).toHaveLength(1);
+    expect(newState).toEqual(state);
+  });
 });
 
 describe('undo / redo with stages', () => {

src/core/reducer.ts

@@ -19,8 +19,20 @@ import {
   State,
   Move,
   LongFormMove,
+  Undo,
 } from '../types';
 
+/**
+ * Check if the payload for the passed action contains a playerID.
+ */
+const actionHasPlayerID = (
+  action:
+    | ActionShape.MakeMove
+    | ActionShape.GameEvent
+    | ActionShape.Undo
+    | ActionShape.Redo
+) => action.payload.playerID !== null && action.payload.playerID !== undefined;
+
 /**
  * Returns true if a move can be undone.
  */
@@ -46,6 +58,37 @@ const CanUndoMove = (G: any, ctx: Ctx, move: Move): boolean => {
   return move.undoable;
 };
 
+/**
+ * Update the undo and redo stacks for a move or event.
+ */
+function updateUndoRedoState(
+  state: State,
+  opts: {
+    game: Game;
+    action: ActionShape.GameEvent | ActionShape.MakeMove;
+  }
+): State {
+  if (opts.game.disableUndo) return state;
+
+  const undoEntry: Undo = {
+    G: state.G,
+    ctx: state.ctx,
+    plugins: state.plugins,
+    playerID: opts.action.payload.playerID || state.ctx.currentPlayer,
+  };
+
+  if (opts.action.type === 'MAKE_MOVE') {
+    undoEntry.moveType = opts.action.payload.type;
+  }
+
+  return {
+    ...state,
+    _undo: [...state._undo, undoEntry],
+    // Always reset redo stack when making a move or event
+    _redo: [],
+  };
+}
+
 /**
  * CreateGameReducer
  *
@@ -88,8 +131,7 @@ export function CreateGameReducer({
 
         // Ignore the event if the player isn't active.
         if (
-          action.payload.playerID !== null &&
-          action.payload.playerID !== undefined &&
+          actionHasPlayerID(action) &&
           !game.flow.isPlayerActive(state.G, state.ctx, action.payload.playerID)
         ) {
           error(`disallowed event: ${action.payload.type}`);
@@ -109,6 +151,9 @@ export function CreateGameReducer({
         // Execute plugins.
         newState = plugins.Flush(newState, { game, isClient: false });
 
+        // Update undo / redo state.
+        newState = updateUndoRedoState(newState, { game, action });
+
         return { ...newState, _stateID: state._stateID + 1 };
       }
 
@@ -139,8 +184,7 @@ export function CreateGameReducer({
 
         // Ignore the move if the player isn't active.
         if (
-          action.payload.playerID !== null &&
-          action.payload.playerID !== undefined &&
+          actionHasPlayerID(action) &&
           !game.flow.isPlayerActive(state.G, state.ctx, action.payload.playerID)
         ) {
           error(`disallowed move: ${action.payload.type}`);
@@ -207,23 +251,12 @@ export function CreateGameReducer({
         // Add the deltalog to state.
         state.deltalog = [logEntry];
 
-        const prevTurnCount = state.ctx.turn;
-
         // Allow the flow reducer to process any triggers that happen after moves.
         state = game.flow.processMove(state, action.payload);
         state = plugins.Flush(state, { game });
 
         // Update undo / redo state.
-        // Only update undo stack if the turn has not been ended
-        if (state.ctx.turn === prevTurnCount && !game.disableUndo) {
-          state._undo = state._undo.concat({
-            G: state.G,
-            ctx: state.ctx,
-            moveType: action.payload.type,
-          });
-        }
-        // Always reset redo stack when making a move
-        state._redo = [];
+        state = updateUndoRedoState(state, { game, action });
 
         return {
           ...state,
@@ -252,11 +285,19 @@ export function CreateGameReducer({
         const last = _undo[_undo.length - 1];
         const restore = _undo[_undo.length - 2];
 
+        // Only allow players to undo their own moves.
+        if (
+          actionHasPlayerID(action) &&
+          action.payload.playerID !== last.playerID
+        ) {
+          return state;
+        }
+
         // Only allow undoable moves to be undone.
         const lastMove: Move = game.flow.getMove(
           restore.ctx,
           last.moveType,
-          action.payload.playerID
+          last.playerID
         );
         if (!CanUndoMove(state.G, state.ctx, lastMove)) {
           return state;
@@ -285,6 +326,14 @@ export function CreateGameReducer({
 
         const first = _redo[0];
 
+        // Only allow players to redo their own undos.
+        if (
+          actionHasPlayerID(action) &&
+          action.payload.playerID !== first.playerID
+        ) {
+          return state;
+        }
+
         return {
           ...state,
           G: first.G,