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

Emit events for collaborative sessions #139

Merged
merged 14 commits into from
Apr 28, 2023
Merged

Conversation

hbcarlos
Copy link
Member

Emits events during a collaborative session.

Adds a new lab plugin to log the events in the dev console of the client. This might be useful to debug issues from users that don't have access to the terminal where the server is running, for example, users of JupyterHub-based deployments.

Adds a new dialog to alert users that multiple rooms are accessing the same file.

@hbcarlos hbcarlos self-assigned this Apr 21, 2023
@github-actions
Copy link
Contributor

Binder 👈 Launch a Binder on branch hbcarlos/jupyter_collaboration/rtc-events

@codecov-commenter
Copy link

codecov-commenter commented Apr 21, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (b46809d) 0.00% compared to head (b6a2440) 0.00%.

Additional details and impacted files
@@          Coverage Diff          @@
##            main    #139   +/-   ##
=====================================
  Coverage   0.00%   0.00%           
=====================================
  Files          7       7           
  Lines        408     452   +44     
=====================================
- Misses       408     452   +44     
Impacted Files Coverage Δ
jupyter_collaboration/app.py 0.00% <0.00%> (ø)
jupyter_collaboration/handlers.py 0.00% <0.00%> (ø)
jupyter_collaboration/rooms.py 0.00% <0.00%> (ø)
jupyter_collaboration/utils.py 0.00% <0.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

else:
self._emit(
"initialize",
"There is another collaborative session accessing the same file.\nThe synchronization between rooms is not supported and you might lose some of your progress.",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From here, I thought synchronization between rooms was supported. Could you clarify?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we should rather say that it is experimental.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The UX of synchronization between rooms is pretty bad. With a good connection, a noticeable delay makes it unpredictable.

When two users are editing the same file using different models (for example, a user editing a notebook using the notebook view and another one using the plain text editor), propagating the changes from user A in room A to user B in room B takes a few seconds (there is a 1s delay to save the document + 1s delay for polling the file assuming that the communication and load delay is 0). During those 2s, user B can write a sentence that will disappear without any notification or asking the user if they want to overwrite the content.

In addition, we use the default content manager to load and save documents. When loading or saving a notebook, the content manager throws an error if the notebook is invalid (so it doesn't load/save the content). If user A in room A is editing the notebook using the plain text view, room B (with user B using the notebook editor) will be updated only when room A saves a valid notebook. Suppose user A forgets a curly bracket (which often happens because there is no visual indicator). In that case, everything user B is typing will be deleted once user A fixes the notebook without allowing user B to overwrite what is on the disk. Furthermore, if user B doesn't realize that their changes are not being saved, they can be typing for hours, and everything will be deleted.

The combination of auto-saving and room synchronization can be catastrophic; in my opinion, we can not call that "supported".

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough, I agree that synchronization happening "through the file system" is not ideal, but we need to have at least a way to propagate changes between views. Otherwise, if the frontend reacts to file changes made in the backend, and you have one user with a notebook opened as a notebook document, and another user with the same notebook opened as a JSON document, what is the behavior?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise, if the frontend reacts to file changes made in the backend, and you have one user with a notebook opened as a notebook document, and another user with the same notebook opened as a JSON document, what is the behavior?

Yes, that's why I'm not removing the sync between rooms. Instead, I notify users saying that it is dangerous to use it.

Ideally, a secondary communication channel would be created to allow users to intervene in those decisions. I'm on it as well.

Copy link
Member

@fcollonval fcollonval left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot @hbcarlos this is a great addition.

I have minor wording comments and two suggestions:

  • Use an enum for the level of message
  • Use the JupyterLab logger instead of the browser console

jupyter_collaboration/events/session.yaml Outdated Show resolved Hide resolved
jupyter_collaboration/events/session.yaml Outdated Show resolved Hide resolved
jupyter_collaboration/events/session.yaml Outdated Show resolved Hide resolved
jupyter_collaboration/events/session.yaml Outdated Show resolved Hide resolved
jupyter_collaboration/events/session.yaml Outdated Show resolved Hide resolved
packages/collaboration-extension/src/filebrowser.ts Outdated Show resolved Hide resolved
packages/collaboration-extension/src/filebrowser.ts Outdated Show resolved Hide resolved
packages/collaboration-extension/src/filebrowser.ts Outdated Show resolved Hide resolved
packages/collaboration-extension/src/filebrowser.ts Outdated Show resolved Hide resolved
packages/collaboration-extension/src/filebrowser.ts Outdated Show resolved Hide resolved
hbcarlos and others added 3 commits April 26, 2023 15:24
Co-authored-by: Afshin Taylor Darian <git@darian.email>
Co-authored-by: Afshin Taylor Darian <git@darian.email>
Co-authored-by: Afshin Taylor Darian <git@darian.email>
@fcollonval fcollonval added enhancement New feature or request and removed maintenance labels Apr 27, 2023
Copy link
Member

@fcollonval fcollonval left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @hbcarlos

Some final minor suggestions from me and it should be ready.

package.json Outdated Show resolved Hide resolved
packages/collaboration-extension/src/filebrowser.ts Outdated Show resolved Hide resolved
packages/collaboration-extension/src/filebrowser.ts Outdated Show resolved Hide resolved
@afshin
Copy link
Member

afshin commented Apr 27, 2023

cf. (for linking or if others are interested) the JEP for publishing versioned schema files: jupyter/enhancement-proposals#108

Copy link
Member

@fcollonval fcollonval left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @hbcarlos

@hbcarlos hbcarlos merged commit 3797752 into jupyterlab:main Apr 28, 2023
16 of 17 checks passed
@hbcarlos hbcarlos deleted the rtc-events branch April 28, 2023 15:18
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.

None yet

5 participants