Skip to content

Conversation

@Zsailer
Copy link
Collaborator

@Zsailer Zsailer commented May 30, 2025

Supercedes #81

@Zsailer
Copy link
Collaborator Author

Zsailer commented May 30, 2025

Addressed all comments in #81

@Zsailer Zsailer marked this pull request as ready for review May 30, 2025 16:24
@Zsailer
Copy link
Collaborator Author

Zsailer commented May 30, 2025

I think we need to move the execution count out of the ydoc and into awareness.

I'll do this in a separate PR.

@3coins
Copy link
Collaborator

3coins commented May 30, 2025

@Zsailer
These actions seem to cause the duplicate issue with this PR.

  1. Execute cell 1 which has a streaming output
  2. Execute cell 2, and re-execute cell 2 while cell 1 is running
  3. This causes 2 duplicate outputs for cell 2
duplicate-outputs.mov

# Specifically update the running cell's execution state if cell_id is provided
if cell_id:
notebook = await yroom.get_jupyter_ydoc()
_, target_cell = notebook.find_cell(cell_id)
Copy link
Collaborator

@3coins 3coins Jun 2, 2025

Choose a reason for hiding this comment

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

@Zsailer
PR 80 also passed cells as a second argument here, is this missing? I can't locate the find_cell API in YNotebook or YBaseDoc to verify this.
https://github.com/jupyter-ai-contrib/jupyter-server-documents/pull/81/files#diff-7c1b5e5cd83f31f24af67c439d6a9422528ddb74a5b20598b25230a798d613d7R252-R253

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I moved this API to the ydocs.py module here. We don't need to pass the list of cells anymore, since that class already has the list as a property.

@Zsailer
Copy link
Collaborator Author

Zsailer commented Jun 3, 2025

@3coins I think this is ready to go.

I just rebuilt and tested and everything seems to be working well 🚀

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.

@Zsailer
Looks good. Thanks for all the updates.

@3coins 3coins merged commit 2ff5322 into jupyter-ai-contrib:main Jun 3, 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.

2 participants