yjs / y-prosemirror

ProseMirror editor binding for Yjs
https://demos.yjs.dev/prosemirror/prosemirror.html
MIT License
335 stars 116 forks source link

Infinite cursor awareness update loop when two prosemirror docs edit same underlying y.XmlFragment #85

Open milesingrams opened 2 years ago

milesingrams commented 2 years ago

Describe the bug I am using TipTap (https://tiptap.dev/) which uses y-prosemirror under the hood to handle collaboration cursors. I have a semi unique situation where I have a page with two editors both which edit the same Y.XmlFragment. When I click on either of the editors to begin editing the cursor starts flickering and runs and infinite cycle of updating the awareness state. I have narrowed down the cycle to this block of code:

https://github.com/yjs/y-prosemirror/blob/984175f7fd7960b1478f10370829659f001ccd3b/src/plugins/cursor-plugin.js#L113-L136

The infinite loop is cycling between:

awareness.setLocalStateField(cursorStateField, { 
  anchor, head 
}) 

and

awareness.setLocalStateField(cursorStateField, null)

My guess is that while the cursor plugin beautifully handles multiple prosemirror documents on a page, it has issues when two prosemirror documents edit the same underlying Y.XmlFragment.

Perhaps more logic around the editor gaining and losing focus could resolve this. i.e. only update awareness when editor is actively focused and set awareness to null when editor loses focus.

Expected behavior I expect multiple editors to be able to seamlessly edit the same Y.XmlFragment without causing the awareness updates to enter an infinite loop.

Environment Information

Anyways I'm sure this isn't the most common use case but I would greatly appreciate the help. Thank you for such an awesome tool!

milesingrams commented 2 years ago

I managed to get this to work by explicitly unsetting the cursor state only on focusout. Not sure if this code will inevitably cause issues but seems to be working pretty smoothly so far.

Here is the modified code I am using:

const updateCursorInfo = () => {
    const ystate = ySyncPluginKey.getState(view.state)
    const current = awareness.getLocalState() || {}
    if (view.hasFocus() && ystate.binding) {
        const selection = getSelection(view.state)
        const anchor = absolutePositionToRelativePosition(selection.anchor, ystate.type, ystate.binding.mapping)
        const head = absolutePositionToRelativePosition(selection.head, ystate.type, ystate.binding.mapping)
        if (
            !current[cursorStateField] ||
            !Y.compareRelativePositions(Y.createRelativePositionFromJSON(current[cursorStateField].anchor), anchor) ||
            !Y.compareRelativePositions(Y.createRelativePositionFromJSON(current[cursorStateField].head), head)
        ) {
            awareness.setLocalStateField(cursorStateField, {
                anchor,
                head,
            })
        }
    }
}

const unsetCursorInfo = () => {
    const current = awareness.getLocalState() || {}
    if (current[cursorStateField]) {
        awareness.setLocalStateField(cursorStateField, null)
    }
}

awareness.on('change', awarenessListener)
view.dom.addEventListener('focusin', updateCursorInfo)
view.dom.addEventListener('focusout', unsetCursorInfo) // Dont use update function for blur event and instead unset
return {
    update: updateCursorInfo,
    destroy: () => {
        view.dom.removeEventListener('focusin', updateCursorInfo)
        view.dom.removeEventListener('focusout', unsetCursorInfo)
        awareness.off('change', awarenessListener)
        unsetCursorInfo()
    },
}

Let me know if you think this would break in certain situations. Happy to submit a PR if you think it's worth adding!

bdbch commented 11 months ago

I think we also ran into this issue while working with React in strict mode. (but it would also happen if you have two Prosemirror plugin instances listening on the same provider causing the editors to throw updates up to react).

furkan3ayraktar commented 11 months ago

@bdbch We've been using the solution from @milesingrams for a long while without any issues. I've created a PR with that solution here.