y-prosemirror icon indicating copy to clipboard operation
y-prosemirror copied to clipboard

Fix `cursorStateField` bug

Open andrictham opened this issue 1 year ago • 1 comments

Summary

This PR fixes an issue with the cursorStateField param that’s passed into yCursorPlugin.

Setting cursorStateField enables awareness state to be namespaced, so that cursors from different subdocuments syncing over the same provider will be able to read and write their own cursor state.

However, it is currently broken. Setting cursorStateField doesn’t work as intended: the cursor plugin writes cursor state to the correct field, but doesn’t read back from it when rendering cursors.

This fixes #86

Current Behavior

If a custom value of cursorStateField is set, the cursor plugin correctly uses it to set awareness state: https://github.com/yjs/y-prosemirror/blob/fd08cf3c7c307c1446178765baa94603a3a8416a/src/plugins/cursor-plugin.js#L239-L242

However, in the createDecorations function, the value aw.cursor is still used. This is reading a hardcoded field with the name "cursor" from the awareness state. https://github.com/yjs/y-prosemirror/blob/fd08cf3c7c307c1446178765baa94603a3a8416a/src/plugins/cursor-plugin.js#L101-L112

This results in cursors not rendering properly if you set cursorStateField to a custom value.

Proposed Changes

  • All references to a hardcoded "cursor" field are replaced with a dynamic reference to the field name set in cursorStateField.

Tests

  • I ran npm run test and confirmed that all tests are successful
  • I manually tested this in my own repo which uses a custom CollaborationCursor extension in TipTap, and confirmed that this fixes the original issue where cursor state between subdocuments are wrongly replicated.
    • With this fix, if a subdocument’s guid is passed into the cursorStateField param, each subdocument’s cursor state will now sync to separate awareness fields.
    • Merging this will unblock the downstream TipTap issue here: https://github.com/ueberdosis/tiptap/issues/5271

andrictham avatar Jul 11 '24 14:07 andrictham

@dmonad I wonder if it’s possible to get this merged, or if not, is there something I’m misunderstanding about how cursorStateField is supposed to work, or something I'm missing in my PR?

I’m reliant on this fix so I can get subdocuments working in my app.

Let me know how I can help 😀

andrictham avatar Aug 09 '24 17:08 andrictham