reaflow icon indicating copy to clipboard operation
reaflow copied to clipboard

Undo doesn't handle mass undos, history gets partially lost

Open Vadorequest opened this issue 3 years ago • 8 comments

I'm submitting a...

[x] Bug report

Current behavior

When using undo/redo, when using the cmd+z shortcut, if we keep pressing on it then the history gets messed up somehow and we cannot do cmd+shift+z more than once/twice.

When doing cmd+z multiple time (not spamming) then it works as expected.

Expected behavior

It should work the same way whether spamming cmd+z or not.

Environment

reaflow 3.0.5

Vadorequest avatar Feb 09 '21 22:02 Vadorequest

Couldn't reproduce the issue with the SB demo at https://reaflow.dev/?path=/story/demos-undo-redo--simple

Might be related to the shared store I'm using? (Recoil)

Vadorequest avatar Feb 10 '21 16:02 Vadorequest

I tried throttling the undo/redo, but it didn't yield good results.

I couldn't locate the root of the issue. I feel like applying too many undos too fast corrupts the history or something similar.


I created an online demo: https://poc-nextjs-reaflow-iqs5cxpy3.vercel.app/

Add 2 nodes by dragging the ports, then use undo/redo through cmd+z and while the undos will work, it won't be possible to redo them when using the shortcuts. Using the undo/redo buttons works fine though.

Vadorequest avatar Feb 17 '21 10:02 Vadorequest

I tried removing most of my business logic (custom nodes, recoil) and use something as basic as possible and the issue still persists.

https://poc-nextjs-reaflow-mzxz3a5h0.vercel.app/

Added a "Add node" button (top right). When keeping cmd+z pressed after adding nodes, the "redo" action cannot be done. Doing undos one by one works correctly though (as before).


I also noticed when the issue happens, and it happens even when clicking manually.

Video: https://youtu.be/4llZSog66O0

👉 At this point, I believe the issue is from reaflow undo/redo implementation. When "undoing" from the "initial state", it looses the whole history.

Vadorequest avatar Feb 17 '21 11:02 Vadorequest

Okay, I think I've found the root cause.

It's because I automatically add a "start" node if there is no "start" node. And when I do the last "undo", it removes the "start" node, which in turn recreates the start nodes, which counts as a new action and removes the "redo" history.

Vadorequest avatar Feb 17 '21 11:02 Vadorequest

Okay, so that was one of the reasons it didn't work properly. Fixed by https://github.com/Vadorequest/poc-nextjs-reaflow/commit/7dd9256bb5fbcae668e5bd381a2e7e79e3485f04

But now, I'm back at square one, when I first noticed the issue with undo/redo, which only allows partial redo when undoing too much at once using the cmd+z shortcut. (the above issue I just fixed was added in the meantime and wasn't why I opened this issue in the first place)

Vadorequest avatar Feb 17 '21 11:02 Vadorequest

@amcdnl After much time spent on this, I'm still unsure of the root issue. It seems to work fine in the demo, but doesn't on my app.

I'm thinking about adding a few options to the useUndoRedo built-in utility, such as:

  • Disable binding shortcuts (make them optional)
  • Exposing more internal stuff (like ref, etc. Basically everything that's needed to build our own shortcuts)

Does that make sense to you?

Vadorequest avatar Feb 19 '21 17:02 Vadorequest

Can you make a PR for the disable?

You should be able to get the refs.

amcdnl avatar Feb 22 '21 12:02 amcdnl

I'll make a PR to allow disabling default shortcuts binding then.

The callbackRef and callbackRef aren't exposed. I think that's the most straightforward way to do that. I don't see how I can use the ref at this time.

https://github.com/reaviz/reaflow/blob/0c15d993798d390f372f7e763df7658b852ef3d9/src/helpers/useUndo.ts#L200-L209

Vadorequest avatar Feb 22 '21 13:02 Vadorequest