Skip to content

Conversation

@dlqqq
Copy link
Collaborator

@dlqqq dlqqq commented Jun 2, 2025

Description

This PR adds YDoc reset / out-of-band change handling for notebooks. To do so, this PR adds a new ResettableNotebook class.

  • ResettableNotebook inherits from the Notebook widget provided by @jupyterlab/notebook.
  • Here, the implementation has been modified to listen to the resetSignal implemented by a custom YNotebook class in custom_ydocs.ts.
  • The notebook state is automatically reset when resetSignal is emitted to, as done in Handle document resets / out-of-band changes on text files #83.
  • Finally, the custom ResettableNotebook class is made the default by implementing the createNotebook() method in RtcNotebookContentFactory, which already overrides the default Notebook factory.

Demo

Screen.Recording.2025-06-02.at.10.43.16.AM.mov

Technical details

@3coins This PR modifies the source code structure slightly. The src/notebook.ts file has been split into multiple files under the src/notebook-factory directory:

src/notebook-factory
|- index.ts            # < re-exports everything here
|- notebook-factory.ts # < most of `src/notebook.ts`
|- notebook.ts         # < defines custom `ResettableNotebook` added by this PR
|- plugin.ts           # < defines the `NotebookFactoryPlugin` provided

Regarding issue #76: I managed to fix the issue when opening the notebook for the first time in a server session. However, after re-opening the notebook, the dirty "unsaved changes" indicator still appears. Will investigate further to see if I can find a fix.

@3coins
Copy link
Collaborator

3coins commented Jun 2, 2025

@dlqqq
Let's check for #76 after PR #90 is merged, which might fix it.

@3coins
Copy link
Collaborator

3coins commented Jun 2, 2025

Opening the notebook in editor (Open With -> Editor), causes endless loop of notifications and edits, without any updates to the file. This could be an edge case that is particular to the Editor, I came across while trying to edit the file directly within JLab, which might not strictly qualify as oob change.

Steps to reproduce:

  1. Open a notebook with some code
  2. Open notebook in Editor (Open With -> Editor)
  3. An endless loop of notifications follow causing edits, even without making any changes to the file in the Editor. (On my initial test, the data in the Notebook vanished, but re-appeared when I opened notebook again)
Screenshot 2025-06-02 at 1 19 24 PM

@3coins
Copy link
Collaborator

3coins commented Jun 2, 2025

I just came across another obscure error, while launching a new notebook (untitled.ipynb), I had just deleted a file (oob-test.ipynb), the notebook failed to load with a loading icon. This also broke Cmd + C when trying to shut down server. Can confirm that deleting a notebook causes this irrecoverable state where you can't open new notebooks, and only a restart fixes.

Screenshot 2025-06-02 at 1 29 18 PM

Here is the exception in the server log:

[E 2025-06-02 13:25:37.322 ServerDocsApp] Exception occurred in `_watch_file() background task for YRoom 'text:file:b1decca7-f6e5-46a8-bf56-d96f1f97110a'. Halting for 5 seconds.
    Traceback (most recent call last):
      File "/Users/pijain/projects/jupyter-server-documents/jupyter_server_documents/rooms/yroom_file_api.py", line 183, in _watch_file
        await self._check_oob_changes()
      File "/Users/pijain/projects/jupyter-server-documents/jupyter_server_documents/rooms/yroom_file_api.py", line 217, in _check_oob_changes
        path = self.get_path()
      File "/Users/pijain/projects/jupyter-server-documents/jupyter_server_documents/rooms/yroom_file_api.py", line 93, in get_path
        raise RuntimeError(
            f"Unable to locate file with ID: '{self.file_id}'."
        )
    RuntimeError: Unable to locate file with ID: 'b1decca7-f6e5-46a8-bf56-d96f1f97110a'.

@dlqqq
Copy link
Collaborator Author

dlqqq commented Jun 2, 2025

@3coins Thanks for testing & finding these issues! Really appreciate your time doing so. I was able to use this afternoon to review all of the Jupyter AI PRs that built up last week.

  1. The first one is definitely a bug. It's unclear if it was introduced by this PR or if it already exists in the main branch, so I'll check tomorrow.

  2. The second one is likely because we do not yet handle file deletion by deleting the YRoom & notifying the user. Since that is future work assigned to me this week, I think it's best to address that in a future PR. I'll open an issue for this tomorrow & include your report there. 👍

@dlqqq
Copy link
Collaborator Author

dlqqq commented Jun 2, 2025

I can anticipate that we will get more bug reports about exceptions like this:

    RuntimeError: Unable to locate file with ID: 'b1decca7-f6e5-46a8-bf56-d96f1f97110a'.

We ought to add a _last_known_path attribute just for logging so we know which file we cannot locate.

@dlqqq
Copy link
Collaborator Author

dlqqq commented Jun 3, 2025

Opened a new issue to track file deletion handling: #96.

@dlqqq
Copy link
Collaborator Author

dlqqq commented Jun 3, 2025

First bug appears related to the attempted fix for #76, i.e. saving on some state updates triggers an infinite loop if 2 YRooms point to the same file. By never saving state dictionary updates as done previously, the first bug is resolved.

I'll see if I can find a fix for #76 using a different approach. If not, I think I will revert my attempted fix to resolve the first bug, then address #76 in the future.

dlqqq added 3 commits June 3, 2025 11:26
Partially fixes #76 where the "unsaved changes" icon appears when a
notebook is opened. The bug no longer appears after a notebook is opened
for the *first* time, but still appears after closing & re-opening the
notebook across the server session.
@dlqqq dlqqq force-pushed the handle-oob-edits-notebooks branch from c267ceb to 7846e7e Compare June 3, 2025 19:40
@dlqqq
Copy link
Collaborator Author

dlqqq commented Jun 3, 2025

@3coins Fixed the first bug that you reported. Using 2 editors for the same notebook file no longer causes an infinite loop. I will show the key line that needed to be removed in a follow-up comment.

There are still a few bugs, but they are either minor or complex enough to deserve a separate PR. I'll document these here for now:

  1. (minor) An out-of-band change notification pops up after opening the notebook in a 2nd editor, even if all the user did was open the file.

  2. (separate PR) If the 2nd text editor corrupts the notebook into invalid JSON at any point, then the notebook room is broken until the server is restarted. Same goes for any other editor like vim.

  3. (minor) An exception is logged in the browser console ~50% of the time when an out-of-band change is made on a notebook:

Screenshot 2025-06-03 at 3 57 49 PM

I spent an extra hour trying to fix 3), but I could not find the cause of this bug. It seems to have something to do with notebook UI virtualization, but the source code is over 2000 lines long. The exception has no impact on the user experience as far as I can tell, so I think this is safe to ignore for now.

I started today at 7:30am so I have to log off. Will open issues for these once I'm back.

Comment on lines -194 to -196

const state = this._sharedModel.ydoc.getMap('state');
state.set('document_id', this._yWebsocketProvider.roomname);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removing these lines was what fixed the first bug you reported.

Basically, opening the file in a second editor would cause ydoc.state['document_id'] to bounce back and forth between the text:file:... and json:notebook:....

I verified that ydoc.state['document_id'] is not used in JupyterLab or anywhere else. It is exclusive to the docprovider in jupyter_collaboration, which we override.

@dlqqq
Copy link
Collaborator Author

dlqqq commented Jun 3, 2025

I also changed the implementation to not create a new NotebookModel on reset. Previously, that caused a weird bug I had noticed where the "dirty" indicator was frozen after reset. This was because all of the consumers listening to NotebookModel signals stopped getting events, since the NotebookModel changed.

So, I found a new way to refresh the notebook UI without creating a new NotebookModel. That fixed the user-facing issue, but introduced the browser console exception reported above.

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
Thanks for the hard work on this PR, and documenting all the scenarios. I concur with all your findings, I could replicate them, but overall I think this is a huge improvement compared to before. Also, don't see any regression on the existing non oob experience. Great job! 🎉

@dlqqq
Copy link
Collaborator Author

dlqqq commented Jun 4, 2025

@3coins Thank you as well for opening an issue for the new bug you found. I've opened 3 follow-up issues for the bugs I discovered yesterday:

  1. (minor) Out-of-band notifications appear for all opened documents #98
  2. (minor) Out-of-band update in notebooks causes uncaught exception in browser console #99
  3. Handle invalid notebooks #100

Merging this PR now. 🎉

@dlqqq dlqqq merged commit 01e4e23 into jupyter-ai-contrib:main Jun 4, 2025
2 of 6 checks passed
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.

Handle YDoc reset on notebooks

2 participants