Skip to content

Conversation

@dlqqq
Copy link
Collaborator

@dlqqq dlqqq commented May 23, 2025

Description

Fixes #9.

Fixes #89.

This PR adds out-of-band change handling for text files. Supporting notebooks is still a WIP and will be done in a future PR, as this PR is already quite long.

Demo video

Screen.Recording.2025-05-23.at.4.52.39.PM.mov

Backend changes

  • Adds a new YRoom.reload_ydoc() method. This method:

    • Disconnects all clients with close code 4000 (discussed later).
    • Immediately purges the message queue, since none of the updates can be applied anymore.
    • Immediately stops YRoomFileAPI to block any new saves.
    • Re-initializes self._ydoc, self._awareness, and self._jupyter_ydoc to reset the shared docs to an empty state.
    • Re-initializes YRoomFileAPI, reloading the YDoc content from the ContentsManager.
    • Is fully synchronous, so it won't be interrupted by a coroutine and shouldn't be impacted by any race conditions.
  • Updates YRoomFileAPI to watch the file on a loop & detect out-of-band changes based on the last_modified timestamp. When this happens, YRoomFileAPI calls YRoom.reload_ydoc().

  • When an out-of-band change is detected, the server closes each WS with close code 4000. This is a special value that the frontend also uses to become informed of out-of-band changes.

Frontend changes

The changes are pretty complicated. In summary, this PR overrides the '@jupyterlab/codemirror-extension:binding' plugin & re-implements it.

When this plugin is activated, it adds an extension to the Codemirror editor that syncs it with the underlying YText object within a YFile or a YCell within a YNotebook.

I've modified the YFile object to add the following to its interface:

// new methods on YFile
reset(): void
resetSignal: ISignal<any, null>

The reset() method re-initializes the YDoc & awareness (similar to YRoom.reload_ydoc(), but doesn't load content). It then calls resetSignal.emit() to tell the Codemirror editor to clear itself. Whenever the client receives close code 4000, the reset() method is called, since the YDoc needs to be reset when an out-of-band change occurs.

I then modified the Codemirror Yjs plugin's interface. Instead of taking ytext: YText, it now takes:

// new arguments to the Codemirror Yjs plugin
getYText: () => YText
resetSignal: ISignal<any, null>

When the reset signal fires, it just calls getYText() again to get the new YText instance, then resets the editor to an empty state. The editor then gets populated with the new content following the SS1 + SS2 handshake.

Reviewer instructions

This PR also adds a dev/reset-untitled-txt.sh script. To test this PR:

  1. Checkout this branch & run jlpm build
  2. Start JupyterLab
  3. Open a new file untitled.txt and make some edits
  4. Run the following in another terminal:
chmod +x ./dev/reset-untitled-txt.sh
./dev/reset-untitled-txt.sh
  1. Return to the editor and watch the OOB change get handled in 5 seconds.

Next steps

References

The close code range 4000-4999 is reserved for private use (by applications) according to RFC 6455.

@dlqqq dlqqq changed the title [WIP] Handle out-of-band changes on text files Handle document resets / out-of-band changes on text files May 27, 2025
@dlqqq
Copy link
Collaborator Author

dlqqq commented May 27, 2025

Added reviewer instructions & opened new issues for follow-up items.

This PR is now ready for review! 🎉

@dlqqq dlqqq marked this pull request as ready for review May 27, 2025 22:46
@dlqqq dlqqq force-pushed the handle-out-of-band-changes branch from ee6c495 to 54c993d Compare May 28, 2025 16:19
@dlqqq
Copy link
Collaborator Author

dlqqq commented May 28, 2025

Rebased the PR onto #84, addressed merge conflicts, and renamed the plugin to reflect the new package name.

@3coins
Copy link
Collaborator

3coins commented May 28, 2025

@dlqqq
I tried the PR, and found situations where there weren't any updates/notifications in JupyterLab unless I re-opened the notebook. Added details in #89 to track this.

@dlqqq
Copy link
Collaborator Author

dlqqq commented May 29, 2025

@3coins Thank you for catching that issue. I found a way to reproduce #89 and added those details in a new comment.

I've just pushed a few commits that fix #89. These changes now:

  1. Log all exceptions in _watch_file() (which were silently halting the loop, causing the issue).
  2. Fixes the exception I encountered by initializing all of our attributes in the constructor.
  3. Only saves YDoc changes after the YDoc's content is loaded. This prevents a useless save upon every load/reload of the YDoc, allowing out-of-band changes to be made easily with vim (without raising the warning about the file having been changed since last write).

Here's a demo video showing that it works well now:

Screen.Recording.2025-05-29.at.10.42.49.AM.mov

Copy link
Collaborator

@3coins 3coins left a comment

Choose a reason for hiding this comment

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

@dlqqq
Added some minor comments, looks good otherwise.

One side-effect I observed now is that the dirty flag of text file is not reset, this seems to be new which wasn't present with your previous commit. This could be a front-end issue, so can be tackled in a separate PR. Ok to merge and iterate.

Screenshot 2025-05-29 at 12 55 11 PM

@dlqqq
Copy link
Collaborator Author

dlqqq commented May 29, 2025

@3coins Thank you for the feedback & for catching these bugs! Really appreciate your help in testing this branch.

I've incorporated your suggestions & fixed the main issue you just reported; the "unsaved changes" icon is now hidden after an out-of-band change.

Screen.Recording.2025-05-29.at.2.25.34.PM.mov

Copy link
Collaborator

@3coins 3coins left a comment

Choose a reason for hiding this comment

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

Good to merge after recent updates. 🚀

@ellisonbg
Copy link
Collaborator

Thanks for this work, going to merge so @dlqqq can work on the out of band changes for notebooks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Out of band changes are not detected Gracefully handle YDoc resets / out-of-band changes

4 participants