Undo restores incorrect ctx

[mgrau]

When performing an undo operation, ctx takes the value of 2 moves prior, instead of 1 move prior. This is particularly problematic when a move changes the phase. I've found that using

        return {
          ...state,
          G: restore.G,
          ctx: last.ctx,
          _undo: _undo.slice(0, _undo.length - 1),
          _redo: [last, ..._redo],
        };

seems to give the correct behavior.

[nicolodavis]

But then G and ctx would come from different snapshots, which doesn't make sense. The last element of _undo (i.e. _undo[length-1]) is the current game state, so it makes sense to restore to _undo[length - 2].

I tried to reproduce your problem with some examples, but I am not able to (things seem to be working correctly, even when the phase changes).

Also note that we have a bunch of unit tests for the undo logic here. Can you demonstrate your problem via an additional test here?

[mgrau]

I agree, it doesn't make sense to use G and ctx from different game states. I need to look around more to see where else ctx is changing. I noticed that if I restore ctx to _undo[_undo.length - 1].ctx, the undo unit tests still pass. I'll focus on trying to replicate my problem with a test.

…

On Mon, May 6, 2019 at 3:55 PM Nicolo John Davis ***@***.***> wrote: But then G and ctx would come from different snapshots, which doesn't make sense. The last element of _undo (i.e. _undo[length-1]) is the current game state, so it makes sense to restore to _undo[length - 2]. I tried to reproduce your problem with some examples, but I am not able to (things seem to be working correctly, even when the phase changes). Also note that we have a bunch of unit tests for the undo logic here <https://github.com/nicolodavis/boardgame.io/blob/master/src/core/reducer.test.js#L267>. Can you demonstrate your problem via an additional test here? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#389 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAKKG3F7EM37NIWQVCK3NITPUA2EZANCNFSM4HJ2W46A> .

[nicolodavis]

I think that's probably because the tests only verify the state in G. Maybe you can add a test that also checks ctx to see if all looks good (I'm happy to get that in regardless of whether a bug exists).

[mgrau]

It looks like G and ctx are appended to _undo in processMove(), however, this is done before ctx is updated by updateAndDetach(), which means that the current ctx is not reflected in _undo. I don't think this is intentional. I've made a test that highlights this behavior.

I'm not sure the best place to fix this, but I've added

state._undo[state._undo.length - 1].ctx = state.ctx;

after ctx is updated by updateAndDetach().

[nicolodavis]

Thanks for the fix, @mgrau!