-
Notifications
You must be signed in to change notification settings - Fork 52
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Collab: Support smart undo/redo #68
Conversation
cabea2e
to
f7fb63e
Compare
e571387
to
5db5f26
Compare
Sorry, can you remind me how to get this working in storybook? When I try it it loads the storybook with some options, but I don't see a way to actually run it. I'm probably missing something very easy!
The patch failed for me so I guess there have been some changes to trunk since. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Super great work @mirka!
I've tested it in the Storybook and reviewed the code and it all looks fine. The tests are really nice, wow.
I'm wondering: does it make sense to add some block modifications to the tests as well? Like making the text bold. I know it shouldn't really matter to the yjs undo manager, so your call.
@lamosty helped me out - for me Storybook keeps defaulting to showing a wide settings panel that hides the editor. It tested out as described above, and the no-collab editor undo works as normal too. The code looks great. There seems to be some strange edge cases when both peers add a block. This may be part of your caveat
|
Thanks for the reviews & testing! 🙏
@lamosty Good one. You're right that the undo manager does not care, but testing formatting will become very relevant after #71 where format edits become less straightforward in Yjs. I'll definitely keep that in mind.
@johngodley Argh I can totally see how that could potentially happen, but somehow I can't trigger it. Am I doing it correctly? Were you able to reproduce it consistently? CleanShot.2021-11-30.at.20.11.44.mp4In contrast, this is the main "cross-boundary" case that is expected to happen — when User B types in an empty paragraph that User A added: CleanShot.2021-11-30.at.20.42.07.mp4I'm trying to determine if you're seeing legitimate cross-boundary issues (which we can't do much about in this PR), or if it's something different. |
I need to figure out a good way to wire this up
# Conflicts: # src/components/collaborative-editing/use-yjs/index.js
Proposed changes
This enables undo/redo functionality, which was previously disabled when collaborative editing.
The crux of multiplayer undo is that the "origin" of each change is tracked, so that each player is only undoing their own changes and not others. Yjs provides an Undo Manager module to handle origin-aware undo stacks.
Integrating this module required some changes to the data flow, but on the whole I think those changes are good. Data flow is now more simplified, and I took this opportunity to rename some things for better readability. Our main integration point is now in redux middleware — for both blocks state updates and undo actions. These will all safely no-op when collab is not enabled or initialized.
✅ UI integration tests pass (single player and multiplayer undo)
TODO before merging
To test
yarn storybook
and open the collab editor story in two browser windows.The base case for multiplayer undo is like this: