FIX: Prevent diffing against stale snapshot or position - #165
Conversation
|
Update I may have found another problem like that. Still looking into it... 👀 I'll convert back to draft until I know more. |
Investigated, but did not find more. So this would be ready for review 🙏 |
|
My AI review picked up some concerns here:
|
This changes the `applyDiff` utility to read out the current state of `yText` as its "before" content, rather then basing its computations on a passed in "before" snapshot. This stabilizes the editing experience for multiple users concurrently editing the same post. Before, the passed on "before" snapshot may already be outdated as concurrent edits where made in the editor. So it's content did not reflect the contents of yText anymore. Applying the diff would then result in weird splitting of words, inserts in the wrong place and loss of characters. With this change applied, concurrent edits result in a more human readable result, even when changes are happening concurrently in the same place.
This removes some optimization code from the markdown sync that was causing problems for concurrent edits: diffs where applied in the wrong place, etc. With this change, we always compute a real diff between the textarea's current value and the target. The approach before was looking at the event delta and the length of the text, rather than the actual content.
This updates the handleTextChange function to also patch the DOM value against the Yjs target when moving the carent instead of only relying on the event delta.
4963d64 to
eda6065
Compare
|
Hey @SamSaffron, sorry it took me so long to get back to this. I was out 🏕️ for a couple of weeks... Your AI has a valid point! Thanks for that. I updated the code accordingly, so that we also patch the actual DOM value against the Yjs target instead of only relying on the event delta for positioning the caret. |
3 participants
This change aims to prevent diffing against a stale snapshot/position when applying a change in the scenario of multiple concurrent edits on the same post.
See the corresponding topic in meta: https://meta.discourse.org/t/prevent-race-condition-during-concurrent-edits/407261
Changes
Update
applyDiffto use current state ofyTextChange the
applyDiffutility to read out the current state ofyTextas its "before" content, rather then basing computations on a passed in "before" snapshot of the content. With concurrent edits happening, the snapshot can be outdated already and not reflect the current editor state any more, as its computed synchronously before.This stabilizes the editing experience for multiple users concurrently editing the same post. Before, the passed on "before" snapshot may already be outdated as concurrent edits where made in the editor. So it's content did not reflect the contents of yText anymore. Applying the diff would then result in weird splitting of words, inserts in the wrong place and loss of characters.
With this change applied, concurrent edits result in a more human readable result, even when changes are happening concurrently in the same place.
Update
handleTextChangefor markdownRemove some optimization code from the markdown sync that was causing problems for concurrent edits: diffs where applied in the wrong place, etc.
With this change, we always compute a real diff between the textarea's current value and the target. The approach before was looking at the event delta and the length of the text, rather than the actual content.