Repository: boardgameio/boardgame.io (ts)

Issue:
Undo redo not reflected in log
Here is a minimal reproduce
https://codesandbox.io/s/bgio-undo-redo-not-reflected-in-log-hcmdi

Make a move then undo.
Game state is rolled back. but log still presents

Pull request:
feat: Add log entries for undo/redo actions
Closes #820

- Add `deltalog` entries when processing undo/redo actions.
- Add undo/redo `deltalog`s to the client’s log.
- Display all non-automatic actions in the debug panel’s log pane.
- Use “assumed” `playerID` for undo/redo dispatchers on the client (i.e. if `playerID` is null/undefined for a single-player client, use `currentPlayer`).

Patch:
diff --git a/src/client/client.ts b/src/client/client.ts
index 187b0d5..6f33d41 100644
--- a/src/client/client.ts
+++ b/src/client/client.ts
@@ -42,6 +42,24 @@ interface DebugOpt {
   impl?: typeof Debug;
 }
 
+/**
+ * Standardise the passed playerID, using currentPlayer if appropriate.
+ */
+function assumedPlayerID(
+  playerID: PlayerID | null | undefined,
+  store: Store,
+  multiplayer?: unknown
+): PlayerID {
+  // In singleplayer mode, if the client does not have a playerID
+  // associated with it, we attach the currentPlayer as playerID.
+  if (!multiplayer && (playerID === null || playerID === undefined)) {
+    const state = store.getState();
+    playerID = state.ctx.currentPlayer;
+  }
+
+  return playerID;
+}
+
 /**
  * createDispatchers
  *
@@ -57,20 +75,11 @@ function createDispatchers(
 ) {
   return innerActionNames.reduce((dispatchers, name) => {
     dispatchers[name] = function(...args: any[]) {
-      let assumedPlayerID = playerID;
-
-      // In singleplayer mode, if the client does not have a playerID
-      // associated with it, we attach the currentPlayer as playerID.
-      if (!multiplayer && (playerID === null || playerID === undefined)) {
-        const state = store.getState();
-        assumedPlayerID = state.ctx.currentPlayer;
-      }
-
       store.dispatch(
         ActionCreators[storeActionType](
           name,
           args,
-          assumedPlayerID,
+          assumedPlayerID(playerID, store, multiplayer),
           credentials
         )
       );
@@ -170,10 +179,18 @@ export class _ClientImpl<G extends any = any> {
       this.store.dispatch(ActionCreators.reset(this.initialState));
     };
     this.undo = () => {
-      this.store.dispatch(ActionCreators.undo(this.playerID, this.credentials));
+      const undo = ActionCreators.undo(
+        assumedPlayerID(this.playerID, this.store, this.multiplayer),
+        this.credentials
+      );
+      this.store.dispatch(undo);
     };
     this.redo = () => {
-      this.store.dispatch(ActionCreators.redo(this.playerID, this.credentials));
+      const redo = ActionCreators.redo(
+        assumedPlayerID(this.playerID, this.store, this.multiplayer),
+        this.credentials
+      );
+      this.store.dispatch(redo);
     };
 
     this.store = null;
@@ -196,7 +213,9 @@ export class _ClientImpl<G extends any = any> {
 
       switch (action.type) {
         case Actions.MAKE_MOVE:
-        case Actions.GAME_EVENT: {
+        case Actions.GAME_EVENT:
+        case Actions.UNDO:
+        case Actions.REDO: {
           const deltalog = state.deltalog;
           this.log = [...this.log, ...deltalog];
           break;
diff --git a/src/client/debug/log/Log.svelte b/src/client/debug/log/Log.svelte
index b6bb600..d9a70d8 100644
--- a/src/client/debug/log/Log.svelte
+++ b/src/client/debug/log/Log.svelte
@@ -5,7 +5,6 @@
 
   const { secondaryPane } = getContext('secondaryPane');
 
-  import { MAKE_MOVE } from '../../../core/action-types';
   import { CreateGameReducer } from '../../../core/reducer';
   import TurnMarker from './TurnMarker.svelte';
   import PhaseMarker from './PhaseMarker.svelte';
@@ -24,9 +23,7 @@
 
       if (!automatic) {
         state = reducer(state, action);
-      }
 
-      if (action.type == MAKE_MOVE) {
         if (logIndex == 0) {
           break;
         }
@@ -40,7 +37,7 @@
   function OnLogClick(e) {
     const { logIndex } = e.detail;
     const state = rewind(logIndex);
-    const renderedLogEntries = log.filter(e => e.action.type == MAKE_MOVE);
+    const renderedLogEntries = log.filter(e => !e.automatic);
     client.overrideGameState(state);
 
     if (pinned == logIndex) {
@@ -90,7 +87,7 @@
 
   $: {
     log = $client.log;
-    renderedLogEntries = log.filter(e => e.action.type == MAKE_MOVE);
+    renderedLogEntries = log.filter(e => !e.automatic);
 
     let eventsInCurrentPhase = 0;
     let eventsInCurrentTurn = 0;
diff --git a/src/client/debug/log/LogEvent.svelte b/src/client/debug/log/LogEvent.svelte
index 7b4ff09..813b37d 100644
--- a/src/client/debug/log/LogEvent.svelte
+++ b/src/client/debug/log/LogEvent.svelte
@@ -12,6 +12,19 @@
 
   const args = action.payload.args || [];
   const playerID = action.payload.playerID;
+  let actionType; 
+  switch (action.type) {
+    case 'UNDO':
+      actionType = 'undo';
+      break;
+    case 'REDO':
+      actionType = 'redo';
+    case 'GAME_EVENT':
+    case 'MAKE_MOVE':
+    default:
+      actionType = action.payload.type;
+      break;
+  }
 </script>
 
 <style>
@@ -115,7 +128,7 @@
   on:click={() => dispatch('click', { logIndex })}
   on:mouseenter={() => dispatch('mouseenter', { logIndex })}
   on:mouseleave={() => dispatch('mouseleave')}>
-  <div>{action.payload.type}({args.join(',')})</div>
+  <div>{actionType}({args.join(',')})</div>
 
   {#if payloadComponent}
     <svelte:component this={payloadComponent} {payload} />
diff --git a/src/core/reducer.ts b/src/core/reducer.ts
index 72f25d7..f3099cf 100644
--- a/src/core/reducer.ts
+++ b/src/core/reducer.ts
@@ -89,6 +89,32 @@ function updateUndoRedoState(
   };
 }
 
+/**
+ * Process state, adding the initial deltalog for this action.
+ */
+function initializeDeltalog(
+  state: State,
+  action: ActionShape.MakeMove | ActionShape.Undo | ActionShape.Redo,
+  move?: Move
+): State {
+  // Create a log entry for this action.
+  const logEntry: LogEntry = {
+    action,
+    _stateID: state._stateID,
+    turn: state.ctx.turn,
+    phase: state.ctx.phase,
+  };
+
+  if (typeof move === 'object' && move.redact === true) {
+    logEntry.redact = true;
+  }
+
+  return {
+    ...state,
+    deltalog: [logEntry],
+  };
+}
+
 /**
  * CreateGameReducer
  *
@@ -236,20 +262,7 @@ export function CreateGameReducer({
         }
 
         // On the server, construct the deltalog.
-        // Create a log entry for this move.
-        let logEntry: LogEntry = {
-          action,
-          _stateID: state._stateID,
-          turn: state.ctx.turn,
-          phase: state.ctx.phase,
-        };
-
-        if ((move as LongFormMove).redact === true) {
-          logEntry.redact = true;
-        }
-
-        // Add the deltalog to state.
-        state.deltalog = [logEntry];
+        state = initializeDeltalog(state, action, move);
 
         // Allow the flow reducer to process any triggers that happen after moves.
         state = game.flow.processMove(state, action.payload);
@@ -271,6 +284,8 @@ export function CreateGameReducer({
       }
 
       case Actions.UNDO: {
+        state = { ...state, deltalog: [] };
+
         if (game.disableUndo) {
           error('Undo is not enabled');
           return state;
@@ -303,24 +318,29 @@ export function CreateGameReducer({
           return state;
         }
 
+        state = initializeDeltalog(state, action);
+
         return {
           ...state,
           G: restore.G,
           ctx: restore.ctx,
           plugins: restore.plugins,
+          _stateID: state._stateID + 1,
           _undo: _undo.slice(0, _undo.length - 1),
           _redo: [last, ..._redo],
         };
       }
 
       case Actions.REDO: {
-        const { _undo, _redo } = state;
+        state = { ...state, deltalog: [] };
 
         if (game.disableUndo) {
           error('Redo is not enabled');
           return state;
         }
 
+        const { _undo, _redo } = state;
+
         if (_redo.length == 0) {
           return state;
         }
@@ -335,11 +355,14 @@ export function CreateGameReducer({
           return state;
         }
 
+        state = initializeDeltalog(state, action);
+
         return {
           ...state,
           G: first.G,
           ctx: first.ctx,
           plugins: first.plugins,
+          _stateID: state._stateID + 1,
           _undo: [..._undo, first],
           _redo: _redo.slice(1),
         };
diff --git a/src/types.ts b/src/types.ts
index e6601d4..2982f85 100644
--- a/src/types.ts
+++ b/src/types.ts
@@ -82,7 +82,11 @@ export interface PluginState {
 }
 
 export interface LogEntry {
-  action: ActionShape.MakeMove | ActionShape.GameEvent;
+  action:
+    | ActionShape.MakeMove
+    | ActionShape.GameEvent
+    | ActionShape.Undo
+    | ActionShape.Redo;
   _stateID: number;
   turn: number;
   phase: string;


Test patch:
diff --git a/src/core/reducer.test.ts b/src/core/reducer.test.ts
index 68095c8..ff0368e 100644
--- a/src/core/reducer.test.ts
+++ b/src/core/reducer.test.ts
@@ -387,9 +387,9 @@ describe('undo / redo', () => {
     expect(state.G).toMatchObject({ A: true, C: true });
   });
 
-  test('redo has no effect if nothing to redo', () => {
+  test('redo only resets deltalog if nothing to redo', () => {
     const state = reducer(initialState, makeMove('move', 'A', '0'));
-    expect(reducer(state, redo())).toEqual(state);
+    expect(reducer(state, redo())).toEqual({ ...state, deltalog: [] });
   });
 });
 
@@ -484,13 +484,13 @@ describe('undo stack', () => {
 
   test('can’t undo at the start of a turn', () => {
     const newState = reducer(state, undo());
-    expect(state).toEqual(newState);
+    expect(newState).toEqual({ ...state, deltalog: [] });
   });
 
   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);
+    expect(newState).toEqual({ ...state, deltalog: [] });
   });
 });
 
@@ -546,7 +546,7 @@ describe('redo stack', () => {
     expect(state._redo).toHaveLength(1);
     const newState = reducer(state, redo('0'));
     expect(state._redo).toHaveLength(1);
-    expect(newState).toEqual(state);
+    expect(newState).toEqual({ ...state, deltalog: [] });
   });
 });