Adds a basic undo/redo implementation using 'fast-json-patch'

[anmolarora1]

Fixes #105

This adds a store-enhancer to allow creating patches/inverse-patches (diffs) between the states associated with all undoable actions, and also two reducers, for undo and redo operations respectively.
The purpose of this draft is to get the approach verified and identify any bottlenecks that we see at this point.

[raineorshine]

This looks like a great start. Thanks!

[anmolarora1]

@raineorshine I've made some changes to the code and the basic undo/redo functionality seems to work correctly for now. It's based on two assumptions -

1. The caret positioning doesn't need to be maintained for now and would be added once the ContentEditable changes are merged
2. The setCursor issue is resolved, that is, the setCursor is not being called after every newThought action. If we don't want that, the existing code can be updated to call the undo and redo actions thrice in case of an existingThoughtChange action followed by newThought and setCursor actions (We'd have to revert three actions instead of two in a single undo/redo op)

[raineorshine]

Here's what I found:

• The undo/redo toolbar buttons are visually larger than the other buttons. They should be the same size.
• The undo/redo button should be disabled when there is nothing left to undo/redo, respectively.
• You should not be allowed to undo back to the tutorial.
• toggleSidebar, search, and modal* actions should be ignored.
• The cursor disappears after one or two undo/redo actions.
• Undo toggleAttribute (e.g. =sort) does not work

[raineorshine]

TypeError: Cannot read property 'value' of null

Steps to Reproduce

1. Create a
2. Create b
3. Activate deleteThought on b
4. Activate deleteThought on a
5. Close alert
6. Undo

[raineorshine]

I'll do a more detailed review of the code itself after some of the above bugs are fixed, but my initial inspection is positive. Thanks for taking the time to organize the code nicely.

[anmolarora1]

• The undo/redo toolbar buttons are visually larger than the other buttons. They should be the same size.

Fixed it in the latest commit

• The undo/redo button should be disabled when there is nothing left to undo/redo, respectively.

Fixed now.

• You should not be allowed to undo back to the tutorial.

Updated

• toggleSidebar, search, and modal* actions should be ignored.

Sure. I've made the changes in the latest commit.
Besides, I'd suggest we should remove these actions from the issue description that we created earlier

• The cursor disappears after one or two undo/redo actions.

@raineorshine Do you mean the caret or the cursor? I couldn't replicate this one.

• Undo toggleAttribute (e.g. =sort) does not work

I wasn't able to replicate this. I tried with some other toggleAttributes like the table view & pin open and they seemed to work fine. Would you mind sharing the steps to replicate this?

[anmolarora1]

TypeError: Cannot read property 'value' of null

Steps to Reproduce

1. Create a
2. Create b
3. Activate deleteThought on b
4. Activate deleteThought on a
5. Close alert
6. Undo

Couldn't replicate this as well. I'll try to share a screencast for the same.
Meanwhile, can you please check it at your end with the latest changes that I've made?

[raineorshine]

Besides, I'd suggest we should remove these actions from the issue description that we created earlier

Done

[raineorshine]

TypeError: Cannot read property 'value' of null

Steps to Reproduce

1. Create a
2. Create b
3. Activate deleteThought on b
4. Activate deleteThought on a
5. Close alert
6. Undo

Couldn't replicate this as well. I'll try to share a screencast for the same.
Meanwhile, can you please check it at your end with the latest changes that I've made?

Yes, still occurring for me on 61835d7 in Chrome.

[raineorshine]

• The cursor disappears after one or two undo/redo actions.

@raineorshine Do you mean the caret or the cursor? I couldn't replicate this one.

The cursor:

[raineorshine]

• Undo toggleAttribute (e.g. =sort) does not work

I wasn't able to replicate this. I tried with some other toggleAttributes like the table view & pin open and they seemed to work fine. Would you mind sharing the steps to replicate this?

toggleAttribute seems to be working for all of them now. The only thing a little off with toggleSort is that sometimes there is a dead undo.

Steps to Reproduce

1. Create a
2. Create three subthoughts: b, z, and c
3. Select a and activate toggleSort
4. Add another subthought m to a
5. Activate undo twice

Current Behavior

The first undo reverses the creation of m, and the second undo does nothing.

If undo is activated a third time, toggleSort will be reversed.

Expected Behavior

The second undo should reverse the activation of toggleSort.

[anmolarora1]

• Undo toggleAttribute (e.g. =sort) does not work

I wasn't able to replicate this. I tried with some other toggleAttributes like the table view & pin open and they seemed to work fine. Would you mind sharing the steps to replicate this?

toggleAttribute seems to be working for all of them now. The only thing a little off with toggleSort is that sometimes there is a dead undo.

Steps to Reproduce

1. Create a
2. Create three subthoughts: b, z, and c
3. Select a and activate toggleSort
4. Add another subthought m to a
5. Activate undo twice

Current Behavior

The first undo reverses the creation of m, and the second undo does nothing.

If undo is activated a third time, toggleSort will be reversed.

Expected Behavior

The second undo should reverse the activation of toggleSort.

That dead undo represents the setCursor action that's being triggered after toggleAttribute. It updates the datanonce. An inverse of that operation doesn't produce any noticeable results, hence appearing dead

[anmolarora1]

TypeError: Cannot read property 'value' of null

Steps to Reproduce

1. Create a
2. Create b
3. Activate deleteThought on b
4. Activate deleteThought on a
5. Close alert
6. Undo

Couldn't replicate this as well. I'll try to share a screencast for the same.
Meanwhile, can you please check it at your end with the latest changes that I've made?

Yes, still occurring for me on 61835d7 in Chrome.

Okay, I was replicate it now. We were using different ways to trigger 'delete'.
I was using the toolbar icons to trigger the same set of operations, and it worked well. But with shortcuts, it didn't.
Nevertheless, after some digging here's what I found -

The reason for this error that we're getting -
On two consecutive deletes, this is how the patches look like

After a delete operation, when we manually close the alert box, the value of state.alert is changed from a valid object to null. The undo op tries to restore the alert value at this path - state/alert/value. But since that path is null, we get the error - TypeError: Cannot read property 'value' of null
One way to deal with this is to create an undo op for the close alert action. That would help in preserving the correct state value but won't be enough to give us the intended result due to the following issue (Non-serializable state)

From my understanding of our current approach for archiveThough, we're storing a react element in the redux state, hence making it non-serializable. So an undo/redo doesn't work as expected.
This approach doesn't seem ideal unless there's a strong reason for backing it.

https://redux.js.org/faq/organizing-state#can-i-put-functions-promises-or-other-non-serializable-items-in-my-store-state
reduxjs/redux#1248

[raineorshine]

That dead undo represents the setCursor action that's being triggered after toggleAttribute. It updates the datanonce. An inverse of that operation doesn't produce any noticeable results, hence appearing dead

Okay, great. What solution do you propose?

From my understanding of our current approach for archiveThough, we're storing a react element in the redux state, hence making it non-serializable. So an undo/redo doesn't work as expected.
This approach doesn't seem ideal unless there's a strong reason for backing it.

redux.js.org/faq/organizing-state#can-i-put-functions-promises-or-other-non-serializable-items-in-my-store-state
reduxjs/redux#1248

Yes, storing a component in the state seems like a bad idea! Will you propose a solution? Thanks!

[anmolarora1]

From my understanding of our current approach for archiveThough, we're storing a react element in the redux state, hence making it non-serializable. So an undo/redo doesn't work as expected.
This approach doesn't seem ideal unless there's a strong reason for backing it.
redux.js.org/faq/organizing-state#can-i-put-functions-promises-or-other-non-serializable-items-in-my-store-state
reduxjs/redux#1248

Yes, storing a component in the state seems like a bad idea! Will you propose a solution? Thanks!

I inspected all the occurrences of the alert reducer and it seems that we're storing the component only for the archiveThought action. These are the 2 solutions that I'd suggest -

1. The component stored in the value represents - 1) the deleted thought, which is just a piece of text. 2) The undo button which is used to undo the archive op. Since we're working on the generic undo approach which will cover undoing an archived thought, we can get rid of this redundant button, unless it serves some definitive purpose
2. Assuming that we need to have both the undo buttons in place, we can add an undo button to the alert box based on the meta-data - alertType - And trigger the generic undo action on its click.
   This approach would save us from the state conflicts that we're likely face when the user hits undo from within the alert modal, and then tries to do the same from the toolbar.

[anmolarora1]

That dead undo represents the setCursor action that's being triggered after toggleAttribute. It updates the datanonce. An inverse of that operation doesn't produce any noticeable results, hence appearing dead

Okay, great. What solution do you propose?

So far, we've found out two fatuous occurrences of setCursor where it's just being used to update the datanonce so far - newThought and toggleAttribute. And there might be other such instances so handling them individually doesn't seem like the best idea.
The proposed solution - While monitoring the actions in the undoRedoReducerEnhancer, we can easily detect if a setCursor action is just updating the datanonce, and merge it with the preceding inverse-patch in the state. That'd help us retain all the state changes in the patches/inverse-patches while getting rid of the dead undo/redo ops at the same time.
What are your thoughts?

[raineorshine]

I inspected all the occurrences of the alert reducer and it seems that we're storing the component only for the archiveThought action. These are the 2 solutions that I'd suggest -

1. The component stored in the value represents - 1) the deleted thought, which is just a piece of text. 2) The undo button which is used to undo the archive op. Since we're working on the generic undo approach which will cover undoing an archived thought, we can get rid of this redundant button, unless it serves some definitive purpose
2. Assuming that we need to have both the undo buttons in place, we can add an undo button to the alert box based on the meta-data - alertType - And trigger the generic undo action on its click.
   This approach would save us from the state conflicts that we're likely face when the user hits undo from within the alert modal, and then tries to do the same from the toolbar.

That's a great explanation, thanks! Since that's the only place it's used, and we are adding real Undo functionality, I'd say we can remove the Undo button completely and use a regular alert.

So far, we've found out two fatuous occurrences of setCursor where it's just being used to update the datanonce so far - newThought and toggleAttribute. And there might be other such instances so handling them individually doesn't seem like the best idea.

Agreed

The proposed solution - While monitoring the actions in the undoRedoReducerEnhancer, we can easily detect if a setCursor action is just updating the datanonce, and merge it with the preceding inverse-patch in the state. That'd help us retain all the state changes in the patches/inverse-patches while getting rid of the dead undo/redo ops at the same time.
What are your thoughts?

This sounds right to me. My only question is whether it needs to be more general, or if setCursor with only the dataNonce is the only situation that needs to be detected. I wonder if there are other undoable actions that might cause a dead undo state as well.

I guess we can start with your proposed solution and then expand it if we discover there are more dead undo states. Could you take 15 minutes or so to try a variety of different actions and see if there are any other dead undo states? If not, then proceed with the proposed solution. If there are, incorporate that into your analysis and see if you can find a solution that handles both cases in as generic way as possible. Thanks!

[anmolarora1]

I inspected all the occurrences of the alert reducer and it seems that we're storing the component only for the archiveThought action. These are the 2 solutions that I'd suggest -

1. The component stored in the value represents - 1) the deleted thought, which is just a piece of text. 2) The undo button which is used to undo the archive op. Since we're working on the generic undo approach which will cover undoing an archived thought, we can get rid of this redundant button, unless it serves some definitive purpose
2. Assuming that we need to have both the undo buttons in place, we can add an undo button to the alert box based on the meta-data - alertType - And trigger the generic undo action on its click.
   This approach would save us from the state conflicts that we're likely face when the user hits undo from within the alert modal, and then tries to do the same from the toolbar.

That's a great explanation, thanks! Since that's the only place it's used, and we are adding real Undo functionality, I'd say we can remove the Undo button completely and use a regular alert.

Makes sense. Let's create a new issue for this one and fix it quickly

So far, we've found out two fatuous occurrences of setCursor where it's just being used to update the datanonce so far - newThought and toggleAttribute. And there might be other such instances so handling them individually doesn't seem like the best idea.

Agreed

The proposed solution - While monitoring the actions in the undoRedoReducerEnhancer, we can easily detect if a setCursor action is just updating the datanonce, and merge it with the preceding inverse-patch in the state. That'd help us retain all the state changes in the patches/inverse-patches while getting rid of the dead undo/redo ops at the same time.
What are your thoughts?

This sounds right to me. My only question is whether it needs to be more general, or if setCursor with only the dataNonce is the only situation that needs to be detected. I wonder if there are other undoable actions that might cause a dead undo state as well.

I guess we can start with your proposed solution and then expand it if we discover there are more dead undo states. Could you take 15 minutes or so to try a variety of different actions and see if there are any other dead undo states? If not, then proceed with the proposed solution. If there are, incorporate that into your analysis and see if you can find a solution that handles both cases in as generic way as possible. Thanks!

Thanks! I'll do that

[raineorshine]

Makes sense. Let's create a new issue for this one and fix it quickly

Created #837

[anmolarora1]

On two consecutive deletes, this is how the patches look like

After a delete operation, when we manually close the alert box, the value of state.alert is changed from a valid object to null. The undo op tries to restore the alert value at this path - state/alert/value. But since that path is null, we get the error - TypeError: Cannot read property 'value' of null
One way to deal with this is to create an undo op for the close alert action. That would help in preserving the correct state value but won't be enough to give us the intended result due to the following issue (Non-serializable state)

From my understanding of our current approach for archiveThough, we're storing a react element in the redux state, hence making it non-serializable. So an undo/redo doesn't work as expected.
This approach doesn't seem ideal unless there's a strong reason for backing it.

https://redux.js.org/faq/organizing-state#can-i-put-functions-promises-or-other-non-serializable-items-in-my-store-state
reduxjs/redux#1248

After some further digging, I found out that there's another gratuitous issue - There are some state representations that we might not want to be replicated. In this case, it's the appearance of the alert modal on an undo operation. As can be seen in the above state snapshot, the two consecutive inverse-patches contain some valid alert values. So, hitting undo for the first time would result in a state where the alert value is a valid one, causing an alert modal to popup. That's one of the UI states which we do not want to be re-created. This means some sections of the state need to be ignored altogether while creating patches/inverse-patches so we don't end up in an unwanted state after the undo/redo ops.
The solution is to create a generic function that allows ignoring some parts of the state (like alert).
@raineorshine Let me know if that makes sense, and if you have any thoughts to make this approach even better

[raineorshine]

Beautiful! Thanks!