History can get out of state if closing history in an appended transaction

[jljorgenson18]

Issue details

This is a bit hard to explain, but we have functionality that will fire an appended transaction that closes history to add two changes to the undo stack. This normally works great, except that we have further appended transactions that will change the document. The transaction batch that is then dispatched looks like

1. The original transaction
2. Appended transaction that closes history to create two entries on the undo stack
3. Another appended transaction that normalizes some of the document.

When executing an undo with the additional appended transaction, the inversion is then off and the undo fails due to position mapping issues.

The only way we've been able to solve this is by preventing those additional appended transactions on a case by case basis, but this issue keeps popping up. Ideally, prosemirror-history would account for additional downstream appended transactions.

Steps to reproduce

1. Create at least two plugins.
2. Have the first plugin append a transaction that closes history and adds a few more steps after the history is closed
3. Have the second plugin make another change to the document
4. Attempt to undo

(Sorry I don't have a repro link just yet)

ProseMirror version

prosemirror-history 1.0.4

[marijnh]

Reproduction code would be great here, since this is a weird enough use case that I can't precisely imagine what the code would look like (specifically, when the appendTransaction hooks kick in).

[jljorgenson18]

I can try to build a repro case, but here are the details of our specific use case.

1. A paste occurs
2. An append transaction fires in a clipboard plugin. A new transaction is made that closes the history and then this simplifies the pasted content further. This allows the user to "undo" the auto simplification of the clipboard content.
3. Another append transaction fires and then normalizes the document.

Because step 3 changes the document, the step inversion is now off and the undo fails for the user. We've been trying to limit downstream append transactions because of this problem, but they keep resurfacing and causing us problems.

I think the expected behavior is that downstream append transactions are part of the new history slice. Instead, it seems like they are being dropped.

[jljorgenson18]

Ok, I figured this out. I tried creating a repro case with https://glitch.com/edit/#!/typical-babcat?path=index.js:35:25 and I couldn't seem to do it. Then after some tweaking I realized that the bug we are fighting is due to us pasting a pretty large document. If that document takes too long AND there is an additional appended transaction, the newGroupDelay threshold is passed, even though everything is synchronous.

I'm not sure how best to proceed besides upping the newGroupDelay or somehow tricking prosemirror-history. I think the long term fix would be to address the case where the transaction is appended and when it takes a little long here

else if (tr.getMeta("addToHistory") !== false && !(appended && appended.getMeta("addToHistory") === false)) {
    // Group transforms that occur in quick succession into one event.
    let newGroup = history.prevTime < (tr.time || 0) - options.newGroupDelay ||
        !appended && !isAdjacentTo(tr, history.prevRanges)
    let prevRanges = appended ? mapRanges(history.prevRanges, tr.mapping) : rangesFor(tr.mapping.maps[tr.steps.length - 1])
    return new HistoryState(history.done.addTransform(tr, newGroup ? state.selection.getBookmark() : null,
                                                      options, mustPreserveItems(state)),
                            Branch.empty, prevRanges, tr.time)
  }

Maybe just changing

let newGroup = history.prevTime < (tr.time || 0) - options.newGroupDelay ||
        !appended && !isAdjacentTo(tr, history.prevRanges)

to

let newGroup = !appended && (history.prevTime < (tr.time || 0) - options.newGroupDelay || !isAdjacentTo(tr, history.prevRange))

Would be sufficient

(Actually that was not sufficient)

[jljorgenson18]

I think the easiest thing would be to add a new meta field and command for prosemirror history that guarantees that the transaction will be grouped with the previous group. Something like

let newGroup = (history.prevTime < (tr.time || 0) - options.newGroupDelay && !tr.getMeta('mergeWithPreviousGroup')) ||
        (!appended && !isAdjacentTo(tr, history.prevRanges))

[marijnh]

There's already code in place that prevents starting a new group for appended transactions (see the !appended in the line you pasted). Can you tell why that isn't kicking in here?

[jljorgenson18]

That line is not sufficient if the appended transaction takes too long to create. It goes to a new group because of this condition

history.prevTime < (tr.time || 0) - options.newGroupDelay

and the fact that this is an "or"

Upping the newGroupDelay fixes part of the problem, but that changes the batching behavior and larger documents/processing could still yield issues.

[jljorgenson18]

@marijnh I'd be happy to submit a PR and knock this out, I'm just hoping to know which direction is appropriate.

[marijnh]

Ah, I think I see what you mean. Does attached patch address this?

[jljorgenson18]

So I had tried this and this seemed to cause problems when having an appended transaction where you close the history. With this patch, it adds only one entry to the undo stack and we can't do the desired double undo. I think we just need to account for appended transactions when

1. The history was closed
2. The initial transaction that was dispatched didn't have any steps but the appended transaction did have steps

[marijnh]

Could you propose a patch along those lines?

[jljorgenson18]

I can dig into further and see if I can come up with a patch.

[jljorgenson18]

So because appended transactions don't know if other transactions were appended before it (which is a big part of the problem), there isn't a great way to make a robust solution. What we are doing instead is resetting the transaction time to the current time right before it is dispatched instead of when it is created. This allows the next appended transaction to be merged into the right group. However, there is another scenario that is problematic that we are trying to work around. What happens when you have an append transaction that changes the doc on undo?

We have a bookend plugin that will occasionally add empty paragraphs at the top and bottom of our document. This plugin will fire after each document changing transaction, including undo (which is sort of what we want). However, it seems to screw up history if adding a transform on undo.

The sequence of events is

1. Paste happens
2. Paste simplification happens (before bookend plugin)
3. Bookend plugin append transaction fires and essentially does nothing
4. Undo happens
5. Paste simplification revert happens
6. Bookend plugin append transaction fires, adding to the undo stack immediately after the undo finishes.

Said another way, history.undo triggers a history.done

Since appended transactions can check if they are appending themselves after an undo/redo, is there something we could add to prosemirror-history to make sure the state doesn't get out of whack?

Edit: We did end up getting around this by doing a migration away from using append transaction to close history, but it's a less elegant, bulkier solution. The better solution would be something inside of prosemirror-history.

[jljorgenson18]

Any updates on this? I could try a patch, but I would need to know the direction to head. I'm not sure what "should" happen when undoing triggers a done action. The best I can come up with is checking the appended transaction meta field on an appended transaction to see if it was an undo and then add to the undo stack instead of the done stack.

[marijnh]

I'm still not really clear on why this is causing problems at all. Are you in a position where you can create a demo of the problem now?

[jljorgenson18]

I can try to create a demo, but there were two basic problems with history

1. If the append transaction takes too long to dispatch and goes past the new group delay, it does not get merged in with the previous history group. There is basically no great way to solve this except to change your code to be faster or to manipulate the transaction times.
2. If triggering an undo causes an append transaction to fire and change the document, you end up adding an entry to the done stack. This prevents the user from being able to undo at all.

As an example, if you have five groups in the undo stack

1 - 2 - 3 - 4 - 5

And if you undo to go back to 4, the append transaction would fire and change document, causing the user to immediately go back to a 5th group. Every time you undo at this point, you go from 5 to 4 to 5.

What I am suggesting is that the appended transaction does not add to "done" stack if it is appended from an undo. Instead, it would add to the undo stack or try to get merged with what was previously undone.

I can try to modify the example I sent earlier to show the effect.

[jljorgenson18]

Also, I should have brought this up before but because these are actually two different issues, I could create a new github issue so we can close this one and we can focus on the "appended transaction during an undo" problem. Would you prefer that?

[jljorgenson18]

Here is a pretty crude example showing the problem https://glitch.com/~typical-babcat

[marijnh]

So do I understand correctly the undo history isn't actually corrupted, it just behaves in an undesirable way? What would you expect to happen when you append transactions during undo?

[jljorgenson18]

The undo history itself isn't corrupted, but traversing the history ends up being blocked.

I'm not entirely sure what should happen. I would think that the appended transactions would somehow be done in the same "group" as the undo or redo.

You can check appended transaction here

    // Group transforms that occur in quick succession into one event.
    let newGroup = !appended && (history.prevTime < (tr.time || 0) - options.newGroupDelay ||
                                 !isAdjacentTo(tr, history.prevRanges))
    let prevRanges = appended ? mapRanges(history.prevRanges, tr.mapping) : rangesFor(tr.mapping.maps[tr.steps.length - 1])
    return new HistoryState(history.done.addTransform(tr, newGroup ? state.selection.getBookmark() : null,
                                                      options, mustPreserveItems(state)),
                            Branch.empty, prevRanges, tr.time)

And if "appended" was an undo, we could change the history transform slightly.

[jljorgenson18]

So I tried to set "addToHistory" to false on the appended transaction if it was an undo. For whatever reason, it caused the the undo history to be blocked again. I think it was due the fact that the document structure was actually altered.

[jljorgenson18]

We gave up on this and worked around it by splitting up our append transactions and tweaking some behavior.

We had to do the following to fix the issue

• We had to change the time of the transaction to trick prosemirror-history into merging the transactions into a single group correctly
• We had to wrap our undo command to add a meta field and then we used that meta field to skip append transaction calls.
• We created separate append transaction calls to better split the different states when closing the history

These bugs are still present but we will just work around them going forward.

[jljorgenson18]

@marijnh Even though we gave up on this, we finally upgraded to the new version of Prosemirror-history and this patch ProseMirror/prosemirror-history@ab0951c ended up causing a bunch of broken tests and breakages. We call closeHistory in an appended transaction and that closed history doesn't force a new group. I think it would make sense to revert this change if no one else has complained.

[marijnh]

Could you show a (minimal) example of a reasonable use of history that is broken by that patch? I'd like to understand the issue before reverting.

[jljorgenson18]

I can create an example later today but you just need to do the following

1. Dispatch a transaction that changes the document and starts it's own group
2. Create a transaction in appendTransaction that will append off of the transaction in step 1
3. Close history on the appended transaction
4. Actually modify the document on the appended transaction

The actions taken at step 4 will get merged in history with the actions taken at step 1, even though the history has been closed.

As an example for when we use this, we use it for our "typbehind" logic. Example is that when you type
"* "
We start a list but we actual process the list conversion in the appended transaction. We close history to add a group to the history stack so we can easily undo the typebehind rule processing.

[jljorgenson18]

I haven't really tested this but this should be all you need to repro the issue

appendTransaction(transactions, oldState, newState) {
  const tr = closeHistory(newState.tr);
  tr.insertText('Some text'); // This gets merged in with the initial transaction in history even though history was closed
  return tr;
}

[marijnh]

Oh, right, I see what you mean now. Does attached patch resolve it?

[jljorgenson18]

Yep that fixes it! I think at this point, we can close this specific issue as resolved. I could open another issue for the whole "undo triggers a done" problem but I could wait until someone else comes across it since we already worked around it.

[marijnh]

I've published the fix as 1.1.3