UndoManager: Undoing combination of Y.Map#set and deleting that Y.Map from parent Y.Array results in 1 client with invalid Y.Map

[francisduvivier]

Checklist

• Are you reporting a bug? Use github issues for bug reports and feature requests. For general questions, please use https://discuss.yjs.dev/
• Try to report your issue in the correct repository. Yjs consists of many modules. When in doubt, report it to https://github.com/yjs/yjs/issues/

Describe the bug
When undoing the combination of map set operation together with an operation that deletes that map from its parent array and then synchronizing, the result is that the other client ends up in a bad and different state than the client that did the undo.
That other client ends up with the changed property missing in the YMap instead of being changed back.

So 1 the other client (the one not doing the undo, but receiving the sync result afterwards) ends up in a bad state.

To Reproduce
Run the test code below (also on my fork in undo-redo.bug-reproduction.spec.js)

Reproduction Steps Summary:

• Setup 2 clients and an UndoManager
• add a YArray with a YMap with 2 properties
• use stopCapturing on UndoManager in order to group the next 2 operations
• change 1 of the 2 properties of the YMap
• delete the YMap from the YArray
• sync clients
• call undo() on UndoManager
• sync clients
  Expected:
  YMap is added back to the YArray and has the old content again on both clients
  Actual:
  The other client that is synchronized ends up in a bad state: the value that was changed using set in the YMap is missing there

Workaround
Issue can be kind of worked around by splitting up the operations in the undo manager by using stopCapturing unbetween. when the 2 operations are undone separately, the issue is not reproducible.

Code:

import { init } from '../testHelper.js'
import * as Y from 'yjs'
import * as t from 'lib0/testing'

/**
 * @param {t.TestCase} tc
 */
export const testUndoMapSetAndDeleteFromArray = tc => {
  const { testConnector, array0, array1 } = init(tc, { users: 3 })
  const undoManager = new Y.UndoManager(array0)

  const mapInArray = new Y.Map()
  mapInArray.set('untouchedProp', 'untouched prop value')
  const toBeChangedPropKey = 'toBeChangedProp'
  mapInArray.set(toBeChangedPropKey, 'before change')
  array0.push([mapInArray])
  testConnector.syncAll()

  // START UNDO BLOCK
  undoManager.stopCapturing()

  // Change some value in the YMap
  mapInArray.set(toBeChangedPropKey, 'changed content')
  testConnector.syncAll()

  // undoManager.stopCapturing() // Workaround is to split up the undo and then doing undo twice

  // Delete the YMap from its parent Array
  array0.delete(0, 1)
  testConnector.syncAll()

  // Undo the set + deletion from parent in 1 undo
  undoManager.undo()
  // undoManager.undo() // Workaround is to split up the undo and then doing undo twice

  testConnector.syncAll()

  t.assert(array0.toJSON()[0][toBeChangedPropKey] === 'before change') // OK
  t.assert(array1.toJSON()[0][toBeChangedPropKey] === 'before change') // Failing and undefined instead
}

[francisduvivier]

Note: we also found another woraround
When using { ignoreRemoteMapChanges: true } in the UndoManager constructor, this issue is also not reproducible.

[cacosandon]

I have the same issue. Another workaround is using captureTimeout: 0 so the set + delete are not stacked together.

It's this same issue:
#443

It was resolved, but seems that it's happening again.

[francisduvivier]

I have the same issue. Another workaround is using captureTimeout: 0 so the set + delete are not stacked together.

It's this same issue: #443

It was resolved, but seems that it's happening again.

Issue is slightly different than #443 in the sense that only the second client has the bad map in this bug here

[markcrispo]

I also just hit this issue using XmlFragments and was doing some digging with the debugger before finding this issue.

UndoManager calls redoItem to remake the map and keys. When remaking the Item for 'before change', origin is set to the now-deleted item 'changed content', but parent correctly points to the new, re-created map.

Then the function Item.write serializes the items to sync remote clients, but Item.write only encodes parent if origin is null, so remote clients only see that the 'before change' key has an origin pointing to the deleted map key. As far as I can tell, it looks like remote clients are connecting the recreated key to the deleted map, instead of the recreated map.