Skip to content
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

Merged
merged 21 commits into from
Dec 7, 2021
Merged

Collab: Support smart undo/redo #68

merged 21 commits into from
Dec 7, 2021

Conversation

mirka
Copy link
Member

@mirka mirka commented Sep 30, 2021

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

  • Bundle files
  • Add changelog entry (⚠️ selector names were changed)

To test

  1. yarn storybook and open the collab editor story in two browser windows.
  2. Single player undo should work as expected.
  3. Multiplayer undo should work intuitively, as long as they are editing separate blocks. (Single paragraph concurrent edits aren't fully supported yet — Collab: Add support for same-block concurrent editing #71)

The base case for multiplayer undo is like this:

  1. User A types in the first paragraph.
  2. User B types in the second paragraph.
  3. User A triggers an undo. Their own changes in the first paragraph should be reverted, but not User B's changes in the second paragraph.
  4. User B triggers an undo. Their own changes in the second paragraph should be reverted.

@mirka mirka added enhancement New feature or request [Feature] Collaboration Real-time collaborative editing labels Sep 30, 2021
@mirka mirka self-assigned this Sep 30, 2021
@mirka mirka force-pushed the collab/undo branch 2 times, most recently from cabea2e to f7fb63e Compare October 5, 2021 11:20
@mirka mirka force-pushed the collab/undo branch 4 times, most recently from e571387 to 5db5f26 Compare November 12, 2021 08:05
@mirka mirka marked this pull request as ready for review November 17, 2021 11:19
@mirka mirka requested review from johngodley and a team November 17, 2021 11:20
@johngodley
Copy link
Member

yarn storybook and open the collab editor story in two browser windows.

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!

if you want you can test the changes on a sandboxed P2

The patch failed for me so I guess there have been some changes to trunk since.

Copy link
Member

@lamosty lamosty left a 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.

@johngodley
Copy link
Member

@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 as long as they are editing separate blocks as I think the process of adding a block may cross this boundary, and it may be caused by #89, but I'll mention it just in case:

  • Open 2 tabs, type in one
  • Add block to second (block inserter, add paragraph), type in block
  • In first peer, undo a few times. Both blocks are removed. Redo to add the blocks back
  • Second peer can no longer undo the block they added

@mirka
Copy link
Member Author

mirka commented Nov 30, 2021

Thanks for the reviews & testing! 🙏

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 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.

I think the process of adding a block may cross this boundary

@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.mp4

In 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.mp4

I'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.

@mirka mirka merged commit 81ce53c into trunk Dec 7, 2021
@mirka mirka deleted the collab/undo branch December 7, 2021 17:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request [Feature] Collaboration Real-time collaborative editing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants