Skip to content

Commit

Permalink
Consider the content of the new cells during notebook sync (#12203)
Browse files Browse the repository at this point in the history
## 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 <kbd>Esc</kbd> to allow using the keyboard to perform cell
actions (move around, copy, paste, etc.)
2. Copy the second cell with <kbd>c</kbd> key
3. Delete the second cell with <kbd>dd</kbd> key
4. Paste the copied cell with <kbd>p</kbd> 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.


https://github.com/astral-sh/ruff/assets/67177269/fc58364c-c8fc-4c11-a917-71b6dd90c1ef

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


https://github.com/astral-sh/ruff/assets/67177269/207872dd-dca6-49ee-8b6e-80435c7ef22e
  • Loading branch information
dhruvmanila authored Jul 5, 2024
1 parent f3ccd15 commit 7910bee
Show file tree
Hide file tree
Showing 2 changed files with 35 additions and 7 deletions.
34 changes: 30 additions & 4 deletions crates/ruff_server/src/edit/notebook.rs
Original file line number Diff line number Diff line change
Expand Up @@ -109,31 +109,55 @@ impl NotebookDocument {
text_content,
}) = cells
{
// The structural changes should be done first, as they may affect the cell index.
if let Some(structure) = structure {
let start = structure.array.start as usize;
let delete = structure.array.delete_count as usize;

// First, delete the cells and remove them from the index.
if delete > 0 {
for cell in self.cells.drain(start..start + delete) {
self.cell_index.remove(&cell.url);
}
}

// Second, insert the new cells with the available information. This array does not
// provide the actual contents of the cells, so we'll initialize them with empty
// contents.
for cell in structure.array.cells.into_iter().flatten().rev() {
self.cells
.insert(start, NotebookCell::new(cell, String::new(), version));
.insert(start, NotebookCell::new(cell, String::new(), 0));
}

// register any new cells in the index and update existing ones that came after the insertion
for (i, cell) in self.cells.iter().enumerate().skip(start) {
self.cell_index.insert(cell.url.clone(), i);
// Third, register the new cells in the index and update existing ones that came
// after the insertion.
for (index, cell) in self.cells.iter().enumerate().skip(start) {
self.cell_index.insert(cell.url.clone(), index);
}

// Finally, update the text document that represents the cell with the actual
// contents. This should be done at the end so that both the `cells` and
// `cell_index` are updated before we start applying the changes to the cells.
if let Some(did_open) = structure.did_open {
for cell_text_document in did_open {
if let Some(cell) = self.cell_by_uri_mut(&cell_text_document.uri) {
cell.document = TextDocument::new(
cell_text_document.text,
cell_text_document.version,
);
}
}
}
}

if let Some(cell_data) = data {
for cell in cell_data {
if let Some(existing_cell) = self.cell_by_uri_mut(&cell.document) {
existing_cell.kind = cell.kind;
}
}
}

if let Some(content_changes) = text_content {
for content_change in content_changes {
if let Some(cell) = self.cell_by_uri_mut(&content_change.document.uri) {
Expand All @@ -143,9 +167,11 @@ impl NotebookDocument {
}
}
}

if let Some(metadata_change) = metadata_change {
self.metadata = serde_json::from_value(serde_json::Value::Object(metadata_change))?;
}

Ok(())
}

Expand Down
8 changes: 5 additions & 3 deletions crates/ruff_server/src/session/index.rs
Original file line number Diff line number Diff line change
Expand Up @@ -145,14 +145,16 @@ impl Index {
encoding: PositionEncoding,
) -> crate::Result<()> {
// update notebook cell index
if let Some(lsp_types::NotebookDocumentCellChangeStructure { did_open, .. }) =
cells.as_ref().and_then(|cells| cells.structure.as_ref())
if let Some(lsp_types::NotebookDocumentCellChangeStructure {
did_open: Some(did_open),
..
}) = cells.as_ref().and_then(|cells| cells.structure.as_ref())
{
let Some(path) = self.url_for_key(key).cloned() else {
anyhow::bail!("Tried to open unavailable document `{key}`");
};

for opened_cell in did_open.iter().flatten() {
for opened_cell in did_open {
self.notebook_cells
.insert(opened_cell.uri.clone(), path.clone());
}
Expand Down

0 comments on commit 7910bee

Please sign in to comment.