Bug with history (again?)

[nosferatu500]

Hello again, I think I found something.

sandbox link

I'm trying to implement transition (jump) between different states in my project and found one issue.

If I try to undo or redo a state in a loop it doesn't work as expected.

PS: Loop is just an example for this issue, I know I can get the value directly from the snapshots array.

Expected:
Case 1:
initial value 0
UpdateValue (x3) -> undo by step (x3) -> redo by step (x3) -> 3

Case 2:
initial value 0
UpdateValue (x3) -> undo all -> redo all -> 3

---

Actual:
Case 1:
initial value 0
UpdateValue (x3) -> undo by step (x3) -> redo by step (x3) -> 3 <- fine

Case 2:
initial value 0
UpdateValue (x3) -> undo all -> redo all -> 0 <- bad

[dai-shi]

Thanks for reporting.
Seems like it's related with batching.
Added some delay: https://codesandbox.io/s/react-typescript-forked-xcrni
Not yet sure how this is happening though.

Would you like to create a minimal test to catch the bug? (Note that I'm not even sure if this is fixable at this point.)

[hee10k]

Hello, @dai-shi
#144 "Thank you for making Valtio" again🙏

I found in the code that the lengths of the "ops" array are different
when called through "setTimeout" and when not.

I think the problem is that proxyObject.saveHistory() function is called
when multiple properties of the proxy object change at once.

In the case of the example provided above, it seems to work well using the code below.
But I don't know each operation of ops and I don't know what side effect there is.
(Not solved yet, after moving the index to undo, redo and updating the value, the bug occurs again)

// https://github.com/pmndrs/valtio/blob/b4604b591f33b7be4cc10f81e18a14f848af0b8d/src/utils/proxyWithHistory.ts#L62
subscribe: () =>
  subscribe(proxyObject, (ops) => {
    if (
      ops.some(
        (op) =>
          op[1][0] === 'value' &&
          op[1].length === 2 &&   // added
          (op[0] !== 'set' || op[2] !== proxyObject.history.wip)
      )
    ) {
      proxyObject.saveHistory()
    }
  }),

[dai-shi]

Thanks for digging into it.
Can you tell me how exactly ops are different? The one that works and the other one that doesn't.

op[1].length === 2 &&   // added

My gut feeling is this is not correct. It can be more than two if we have nested objects. But, I may need to take time to look into it to have better understanding.

Also, if you can create a minimal test case, that would be helpful.

[hee10k]

My gut feeling is this is not correct. It can be more than two if we have nested objects. But, I may need to take time to look into it to have better understanding.

Right, It have to be change from op[1].length === 2 && to op[1].length > 1 &&
The difference is below

Change with	op[1].length	op[1]
direct	2	[['value', 'data']] or [['value', 'some', 'deep', 'keys']]
undo/redo	1	[['value']]

Also, if you can create a minimal test case, that would be helpful.

I forked the code sandbox you'll be able to check it.

[dai-shi]

thanks! looking into it.

[dai-shi]

@hee10k I see the reproduction in the codesandbox example, and try to reproduce in a test. However, it doesn't seem to reproduce. Can you check? #354

[hee10k]

@dai-shi Sure, I think firing click event twice is make just 1 snapshot in history.
#354 comment and I made some change to clear code sandbox Testing

[dai-shi]

thanks for your help!
Please try #354.
https://ci.codesandbox.io/status/pmndrs/valtio/pr/354
Find "Local Install Instructions" ☝️

[hee10k]

Great!
to me, it works fine.👍

[dai-shi]

Released: https://github.com/pmndrs/valtio/releases/tag/v1.2.12

Thanks for reporting and investigating.