Skip to content

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

Merged
dhruvmanila merged 1 commit intomainfrom
dhruv/notebook-sync
Jul 5, 2024
Merged

Consider the content of the new cells during notebook sync#12203
dhruvmanila merged 1 commit intomainfrom
dhruv/notebook-sync

Conversation

@dhruvmanila
Copy link
Copy Markdown
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
@github-actions
Copy link
Copy Markdown
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 force-pushed the dhruv/notebook-sync branch from 95ab301 to 1a98ea3 Compare July 5, 2024 11:14
@dhruvmanila dhruvmanila marked this pull request as ready for review July 5, 2024 11:24
@dhruvmanila dhruvmanila requested a review from MichaReiser July 5, 2024 11:24
Copy link
Copy Markdown
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
@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