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

Consider the content of the new cells during notebook sync #12203

Merged
merged 1 commit into from
Jul 5, 2024

Conversation

dhruvmanila
Copy link
Member

@dhruvmanila dhruvmanila commented Jul 5, 2024

Summary

This PR fixes the bug where the server was not considering the cells.structure.didOpen field to sync up the new content of the newly added cells.

The parameters corresponding to this request provides two fields to get the newly added cells:

  1. cells.structure.array.cells: This is a list of NotebookCell which doesn't contain any cell content. The only useful information from this array is the cell kind and the cell document URI which we use to initialize the new cell in the index.
  2. cells.structure.didOpen: This is a list of TextDocumentItem which corresponds to the newly added cells. This actually contains the text content and the version.

This wasn't a problem before because we initialize the cell with an empty string and this isn't a problem when someone just creates an empty cell. But, when someone copy-pastes a cell, the cell needs to be initialized with the content.

fixes: #12201

Test Plan

First, let's see the panic in action:

  1. Press Esc to allow using the keyboard to perform cell actions (move around, copy, paste, etc.)
  2. Copy the second cell with c key
  3. Delete the second cell with dd key
  4. Paste the copied cell with p key

You can see that the content isn't synced up because the unused-import for sys is still being highlighted but it's being used in the second cell. And, the hover isn't working either. Then, as I start editing the second cell, it panics.

Screen.Recording.2024-07-05.at.16.49.09.mov

Now, here's the preview of the fixed version:

Screen.Recording.2024-07-05.at.16.49.39.mov

@dhruvmanila dhruvmanila added the bug Something isn't working label Jul 5, 2024
Copy link
Contributor

github-actions bot commented Jul 5, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

@dhruvmanila dhruvmanila marked this pull request as ready for review July 5, 2024 11:24
Copy link
Member

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

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

The inline comments are very helpful. Thanks for adding them.

@dhruvmanila dhruvmanila changed the title Consider the cell content during notebook sync Consider the content of the new cells during notebook sync Jul 5, 2024
@dhruvmanila dhruvmanila merged commit 7910bee into main Jul 5, 2024
20 checks passed
@dhruvmanila dhruvmanila deleted the dhruv/notebook-sync branch July 5, 2024 11:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Server doesn't sync cell content if copy-pasted directly
2 participants