My app gives every ProseMirror block a unique ID. First the client assigns locally unique temp IDs, later the server provides globally unique IDs which are written into the document with an addToHistory: false transaction. The transaction only contains setNodeMarkup operations.
I’ve noticed that sometimes I can’t undo past this transaction. If I hit undo I just see the caret jump to an odd position, and nothing else change. If I comment out the dispatch call on that one addToHistory: false transaction, undo starts working properly again.
As mentioned in another thread, I’m in the process of moving from TipTap to plain ProseMirror. It’ll be much easier to create a simple stand-alone script once I’ve done that, so I’ll resurrect this thread once that’s done.
I still don’t have an isolated example - it’s difficult to reproduce the bug outside of my full codebase, but I have made some progress identifying the problem. I’ve also implemented a hacky fix, by forking history.js (from prosemirror-history) into my project.
In applyTransaction in that file, there is a big if/else-if/else, the last branch of which, as I understand it, handles adding addToHistory === false transactions.
Those transactions can’t simply be discarded because they might alter document mappings. So appendTransaction returns
new HistoryState(history.done.addMaps(tr.mapping.maps),
history.undone.addMaps(tr.mapping.maps),
mapRanges(history.prevRanges, tr.mapping), history.prevTime)
I discovered that if I didn’t update history with tr.mappings, everything started working properly again. i.e. if I simply return the original history object passed in to applyTransaction.
(my hacky fix is to do this when the problematic transaction is detected, using a metadata property)
The reason this looks like a bug to me is that, as mentioned above, the transaction in question only contains setNodeMarkup operations. Can those lead to changed mappings? I would have thought not.
Have you tried recording the changes for which this goes wrong and trying to replay them outside of your full setup? As long as this can’t be reduced to a simple test case, it’s hard for me to say anything about whether it’s a problem in the library or the use.
This finally got to the top of my to-do list and I’ve got a small isolated repro.
The description at the start of this thread is a bit out of date now, but it doesn’t matter because I’ve followed your advice @marijn and recorded the behaviour of the plugin as json. The repro is a stripped down version of prosemirror-example-setup.
I didn’t get it down to a single script, but it’s all very standard other than plugin.ts which shows the issue.
I’m half expecting there to be something invalid in the transaction my plugin adds, but then ideally ProseMirror should trap that?
Interestingly, it turns out that with a simpler schema (i.e. without the page node type), the bug doesn’t show up, so the link above is still the best repro I have.
I was able to reproduce this (deterministically and regardless of schema). The undo history will end up mapping the inverse of the step that wraps the paragraph in a blockquote over the step that updates that blockquote to add an ID, and because that step replaced the start and end token of the node, that caused the deleted flag to be added to the map result for the positions before and after it, causing ReplaceAroundStep.map to drop the step entirely.
I’ve pushed patches (to prosemirror-transform) that add more granular information about deletion to map results, and makes ReplaceAroundStep.map and ReplaceStep.map use this to avoid deleting steps that could be validly mapped.
Could you take a look to see if the current code works for you?
Thanks for such a quick response. I’m not sure if I’m correctly pulling in your fix, but right now I’m still seeing the same problem. I’ve pushed a commit to the same repo. For me it still has the undo problem.
Checking the fix against my real app might take a bit longer because I’ve not switched over to the new typescript versions. I’ll hopefully find time today.
I’m not directly testing with your repository, but with a local test script that uses the schema and the step JSON to test undo with those steps. That works as I’d expect it to now.
I’ve looked in node_modules to confirm I’ve got your fixes, and made sure I’ve done an “Empty cache and hard reload”. It’s definitely still behaving badly in the browser.
It occurred to me that you’re probably running all of the packages off master, so I tried changing all the deps to pull in from github (transform, commands, history, inputrules, keymap, model, state, view) but now I’m getting the error Adding different instances of a keyed plugin (plugin$)
If I add neither inputrules or keymap, that removes the error, but then I can’t reproduce the bug.
OK I’ve made some progress here. Your fix is working now in my isolated code. TBH I don’t know what I did to fix it. I think deleting yarn.lock and node_modules did the trick.
I’ve not been able to test it yet in my main app, because I seem to have two versions of prosemirror-transform loaded, which is causing problems.
But it looks like the npm version is also pulled in, because other prosemirror modules depend on it. Can you advise how I can ensure only the github version of prosemirror-transform is loaded? Thanks.
I’ve published prosemirror-transform 1.6.0. For next time, it’s often possible to work around this kind of stuff by just copying the content of dist/ from a local build over the copy installed in your node_modules.
I have the same problem because the other packages still depend on 1.5.0. However I tried your dist/ hack and can confirm it’s working great in my full app. Unfortunately I can’t deploy the fix because I’m using a PaaS and can’t do node_modules hacks (well, not easily…)
I did notice a small caret placement issue after an undo that was previously broken - I’ll see if I can capture the transform steps to replicate that.