measuredco / puck

The visual editor for React
https://puckeditor.com
MIT License
5.26k stars 318 forks source link

Reintroduce diffed history #160

Open chrisvxd opened 1 year ago

chrisvxd commented 1 year ago

154 removed diffed history to support #143. The existing diffed history was implemented for performance reasons, but resulted in corrupt history when reordering array items in #143.

However, this may be an issue the reordering implementation of #143 rather than the diffed history introduced in #111. I was able to reproduce this issue using both the original deep-diff implementation and also a fast-json-patch implementation.

Aside: the original deep-diff implementation also stored the original appState as part of the diff, which was rendering the diffing semi-obsolete. Instead, it should pass in the current state (via use-action-history) and apply the diff to that.

Steps to reproduce

  1. Reintroduce diffed history removed in #143
  2. Drag in a ButtonGroup
  3. Add a new item to the buttons array
  4. Add another new item to the buttons array
  5. Move the item at position 1 ("Learn more") to position 3 so it reads "Button", "Button", "Learn more"
  6. Undo the history

What I expected to happen

The array items to be reversed to "Learn more", "Button", "Button"

What actually happened

The array items were corrupted to "Learn more", "Learn more", "Button"

Example

https://github.com/measuredco/puck/assets/985961/f1dbec6f-c7d2-4f54-950a-13a1d590843f

chrisvxd commented 1 year ago

Hey @Yuddomack. I made some changes to your undo/redo history as part of #154 and would appreciate any thoughts you had on this issue 🙏

Yuddomack commented 1 year ago

@chrisvxd thank you for your fix 🙏

first, I'll take a look again at the before merged! second, I'm curious what you think about recording actions based on commands.

ex)

dispatch({ type: "insert", index: 1, ... }) // executed 
<-> 
[{
  forward: () => dispatch({ type: "insert", index: 1, ... }),
  rewind: () => dispatch({ type: "remove", index: 1, ... }),
}, ... ] // history

(it's originally proposed idea. I don't mean to do the same thing as this! I'm just considering about developing this concept.)

chrisvxd commented 1 year ago

I think storing the entire history is more robust, as it works for any action, not just insert/remove. In that proposal, we'd have to encode a different forward/backward path for every action.

Additionally, we now log UI state in the history, which is useful for the user's context when undoing / redoing (#153) and I believe necessary for #143

Yuddomack commented 1 year ago

Right, potential for complex edge cases to occur of each actions.

then I still have some concerns about saving the entire state, but since it hasn't been an issue yet, it seems better to keep storing the entire history!

chrisvxd commented 1 year ago

@Yuddomack I'm with you - I would rather go back to diffed history as you introduced in your PR, but the only way for me to get #143 to behave was by storing the whole state. Either this was a bug with the original diff history implementation in #111, or it is a bug in #143, but I'm not sure.

Yuddomack commented 1 year ago

Okay, then keep this and I'll check it and if I find the cause of the problem, I'll mention it!

chrisvxd commented 11 months ago

The array field has been significantly refactored since this issue was opened. Suspect its possible that the original diffing implementation would work now.