Repository: boardgameio/boardgame.io (ts)

Issue:
Undo / redo validity condition seems wrong in the presence of stages
Currently it says:

https://github.com/boardgameio/boardgame.io/blob/60b32e5d786446914106743f569539269cc1b577/src/master/master.ts#L242-L253

But `activePlayers` can be non-null and there is only one valid player. For example:

```
activePlayers: {0: "selectColor"}
```

It seems to me that undo should be allowed in this case.

Pull request:
feat: Improve undo/redo
- Add plugins state and playerID to undo/redo entries.
- Only allow players to undo/redo their own actions.
- Improve undo/redo validity check in master (fixes #565).

Patch:
diff --git a/src/core/flow.ts b/src/core/flow.ts
index 9654e91..a645ba2 100644
--- a/src/core/flow.ts
+++ b/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;
   }
 
   ////////////
diff --git a/src/core/initialize.ts b/src/core/initialize.ts
index 0738ccd..4d7fd27 100644
--- a/src/core/initialize.ts
+++ b/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;
 }
diff --git a/src/core/reducer.ts b/src/core/reducer.ts
index 0b55489..2266550 100644
--- a/src/core/reducer.ts
+++ b/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,
diff --git a/src/master/master.ts b/src/master/master.ts
index bd18965..bb789ff 100644
--- a/src/master/master.ts
+++ b/src/master/master.ts
@@ -242,9 +242,16 @@ export class Master {
     // that can make moves right now and the person doing the
     // action is that player.
     if (action.type == UNDO || action.type == REDO) {
+      const hasActivePlayers = state.ctx.activePlayers !== null;
+      const isCurrentPlayer = state.ctx.currentPlayer === playerID;
+
       if (
-        state.ctx.currentPlayer !== playerID ||
-        state.ctx.activePlayers !== null
+        // If activePlayers is empty, non-current players can’t undo.
+        (!hasActivePlayers && !isCurrentPlayer) ||
+        // If player is not active or multiple players are active, can’t undo.
+        (hasActivePlayers &&
+          (state.ctx.activePlayers[playerID] === undefined ||
+            Object.keys(state.ctx.activePlayers).length > 1))
       ) {
         logging.error(`playerID=[${playerID}] cannot undo / redo right now`);
         return;
diff --git a/src/types.ts b/src/types.ts
index 48e5b90..e6601d4 100644
--- a/src/types.ts
+++ b/src/types.ts
@@ -270,10 +270,14 @@ export interface Game<G extends any = any, CtxWithPlugins extends Ctx = Ctx> {
   flow?: ReturnType<typeof Flow>;
 }
 
-type Undo<G extends any = any> = {
+export type Undo<G extends any = any> = {
   G: G;
   ctx: Ctx;
+  plugins: {
+    [pluginName: string]: PluginState;
+  };
   moveType?: string;
+  playerID?: PlayerID;
 };
 
 export namespace Server {


Test patch:
diff --git a/src/core/flow.test.js b/src/core/flow.test.js
index 74b0708..eb65a69 100644
--- a/src/core/flow.test.js
+++ b/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({
diff --git a/src/core/reducer.test.ts b/src/core/reducer.test.ts
index 15de18b..8239d75 100644
--- a/src/core/reducer.test.ts
+++ b/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,19 +419,22 @@ 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', () => {
@@ -439,13 +442,26 @@ describe('undo stack', () => {
     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', () => {
diff --git a/src/master/master.test.ts b/src/master/master.test.ts
index b64353b..5db608d 100644
--- a/src/master/master.test.ts
+++ b/src/master/master.test.ts
@@ -202,21 +202,49 @@ describe('update', () => {
     expect(sendAll).toHaveBeenCalled();
   });
 
-  test('undo / redo', async () => {
-    await master.onUpdate(ActionCreators.undo(), 2, 'matchID', '0');
-    expect(error).not.toBeCalled();
+  describe('undo / redo', () => {
+    test('player 0 can undo', async () => {
+      await master.onUpdate(ActionCreators.undo(), 2, 'matchID', '0');
+      expect(error).not.toBeCalled();
+    });
 
-    await master.onUpdate(ActionCreators.undo(), 2, 'matchID', '1');
-    expect(error).toHaveBeenCalledWith(
-      `playerID=[1] cannot undo / redo right now`
-    );
+    test('player 1 can’t undo', async () => {
+      await master.onUpdate(ActionCreators.undo(), 2, 'matchID', '1');
+      expect(error).toHaveBeenCalledWith(
+        `playerID=[1] cannot undo / redo right now`
+      );
+    });
+
+    test('player can’t undo with multiple active players', async () => {
+      const setActivePlayers = ActionCreators.gameEvent(
+        'setActivePlayers',
+        [{ all: 'A' }],
+        '0'
+      );
+      await master.onUpdate(setActivePlayers, 2, 'matchID', '0');
+      await master.onUpdate(ActionCreators.undo('0'), 3, 'matchID', '0');
+      expect(error).toHaveBeenCalledWith(
+        `playerID=[0] cannot undo / redo right now`
+      );
+    });
+
+    test('player can undo if they are the only active player', async () => {
+      const endStage = ActionCreators.gameEvent('endStage', undefined, '0');
+      await master.onUpdate(endStage, 3, 'matchID', '0');
+      await master.onUpdate(ActionCreators.undo('1'), 4, 'matchID', '1');
+      expect(error).not.toBeCalled();
+
+      // Clean-up active players.
+      const endStage2 = ActionCreators.gameEvent('endStage', undefined, '1');
+      await master.onUpdate(endStage2, 4, 'matchID', '1');
+    });
   });
 
   test('game over', async () => {
     let event = ActionCreators.gameEvent('endGame');
-    await master.onUpdate(event, 2, 'matchID', '0');
+    await master.onUpdate(event, 5, 'matchID', '0');
     event = ActionCreators.gameEvent('endTurn');
-    await master.onUpdate(event, 3, 'matchID', '0');
+    await master.onUpdate(event, 6, 'matchID', '0');
     expect(error).toHaveBeenCalledWith(`game over - matchID=[matchID]`);
   });