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

Support suggestions #239

Closed
wants to merge 11 commits into from
Closed

Conversation

davidbrochart
Copy link
Collaborator

@davidbrochart davidbrochart commented Feb 23, 2024

See jupyterlab/jupyterlab#16008.

This PR adds a menu to the Notebook page (it should add it to every shared document in the future), which allows to choose between "Editing" and "Suggesting". The former is the current state (edit document directly), and the later is an editing mode where edits become suggestions, which is the purpose of this PR.
When entering the suggesting mode, the root provider for the shared document is retrieved and a request to fork the document is done with context.model.sharedModel.provider.fork(). This triggers a PUT /api/collaboration/fork_room request with the root room ID of the document in the body. The response gives back a fork ID, which is the room ID for the forked document that is created in the backend. The forked document is a dependency of the root room, and it is kept in sync with the root document: updates to the root document are applied to the forked document, but not the other way around. The shared document is disconnected from the root provider, and connected to a new provider for the forked document. The forked document is advertised on the root Y document as a new state entry fork_{forkId}, so that other users can connect to the forked room. They can do that by calling WebSocketProvider.connect(forkId, sharedModel) on their current provider.
JupyterLab shows a notification when a client enters the suggestion mode, and the user can choose to view the suggestion or remain on the root document. When viewing the suggestion, changes are synchronized with the suggester and it is also possible to collaborate on the suggestion, since it is a regular room. A user (not the suggester) can merge the suggestion or discard it in the Review menu. When merged, a PUT /api/collaboration/merge_room request is done with the fork room ID and the root room ID in the body.
A menu shows which room the document is currently connect to. It can be "Root" for the root room, or the ID of a fork. Users can connect to anyone of them at any time.

@davidbrochart davidbrochart marked this pull request as draft February 23, 2024 15:07
Copy link
Contributor

Binder 👈 Launch a Binder on branch davidbrochart/jupyter_collaboration/suggestions

@ellisonbg
Copy link

@Zsailer @dlqqq for review

@trungleduc
Copy link
Member

Thank @davidbrochart for working on this feature. After playing with this PR in JupyterCAD, I like the simplicity of the fork idea and how easy to integrate it into our project. Here are my few feedbacks:

  • The hardest point is to keep track of ongoing suggestions. For now, we need to listen to the state-changed event of the YDoc to build the list of suggestions. In our case, we have multiple opened documents with only one view to list the suggestions, it becomes harder to keep track of the suggestion list when switching documents.
  • The forked<->root switching mechanism does not work as expected, returning to the root from a forked room brings all changes from the forked room to the root room.
  • It's not possible to discard a forked room without merging it.

My wish list for the improvement:

  • Suggestion can store metadata like label, author,... It would help to create a rich UI for the suggestion panel.
  • Being able to list all suggestions with metadata of a document anytime, delete the suggestion

@davidbrochart
Copy link
Collaborator Author

Thanks for the feedback Trung! You suggestions make a lot of sense, I'll work on them soon.

@davidbrochart
Copy link
Collaborator Author

Closing in favor of #292.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants