Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
Vadorequest opened this issue Feb 9, 2021 · 8 comments
Open

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

Vadorequest opened this issue Feb 9, 2021 · 8 comments

Comments

@Vadorequest
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

Okay, so that was one of the reasons it didn't work properly. Fixed by Vadorequest/rwa-faunadb-reaflow-nextjs-magic@7dd9256

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
Copy link
Contributor Author

Vadorequest commented Feb 19, 2021

@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?

@amcdnl
Copy link
Member

amcdnl commented Feb 22, 2021

Can you make a PR for the disable?

You should be able to get the refs.

@amcdnl amcdnl added enhancement New feature or request investigate and removed enhancement New feature or request labels Feb 22, 2021
@Vadorequest
Copy link
Contributor Author

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.

return {
canUndo,
canRedo,
count: manager.current.count,
history: manager.current.history,
clear,
redo,
undo
} as UndoResult;
};

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants