undo image paste in rendered notebook markdown will just remove the attachment

[amunger]

Testing #180759

1. paste an image into a markdown cell to create an attached image
2. render the markdown cell
3. ctrl+z to undo
   🐛 broken image link

likely because the undo operation isn't able to edit the markdown content, but the undo operation probably shouldn't happen at all

[mjbvz]

I've investigated this issue but am unsure how best to fix it (or even which layer to fix it in). The issue can also happen for normal workspace edits and is not specific to drop or paste.

Here's my understanding of the issue:

1. On paste, we apply a workspace edit. This workspace edit both adds text to the current cell and updates that cell's metadata

2. When you then enter into preview mode and press undo, an undo operation is requested for the entire notebook (the file://*.ipynb resource)

3. At the start of the undo call, the undoRedoService is in the following state:

Notice how there are two undo stacks: one for the cell and one for the notebook. The edit in each of these two stacks has the same groupId. However critically, the edit for the entire notebook (the one in the file://*.ipynb stack) has a higher groupOrder

6. When trying to undo, we now check if there are any other edits in the same group that should also be undone:

vscode/src/vs/platform/undoRedo/common/undoRedoService.ts

Line 1090 in efb49cc

if (element.groupId) {

However this code ends up not finding the cell text change because its group order is lower. This means we only undo the metadata change, causing the broken image

---

This same flow works correctly if the cell is still in edit mode and focused. The key difference is that when the cell is being editing, the undo operation is instead requested for the notebook cell itself (vscode-notebook-cell:...) instead of for the entire notebook. This means that the above code finds the metadata edit on the entire notebook and also undoes it

@rebornix @jrieken For fixing this, should we try to make the NotebookTextModel push a single edit for the entire workspace edit? Right now it ends up pushing two elements into the undoRedoService: one for the metadata change (a IWorkspaceUndoRedoElement) and one for the cell text change (a IResourceUndoRedoElement)

Also open to other suggestions as I'm not too familiar with this area of our code

[rebornix]

@mjbvz thanks for the great analysis and it turns out that our undo/redo support (along with notebook.undoRedoPerCell setting) is not sufficient.

By default notebook.undoRedoPerCell is enabled, so the cell doesn't share the undo/redo stack with the notebook document. If we turn it off, then I think the scenario you showed above works.

The challenge here is when it's enabled (default value), cell text model and notebook text model have separate undo/redo stack, which works fine until we create an undo/redo group which pushes UndoElement to both stacks.

However this code ends up not finding the cell text change because its group order is lower. This means we only undo the metadata change, causing the broken image

Conceptually this is no different from creating an undo/redo group which pushes UndoElements to two separate files' undo stacks. I wonder if we should tweak this piece of code to make it work. cc @alexdima