Skip to content

Commit f3c14a4

Browse files
authored
Keep track of deleted cell for reorder change request (#12575)
## Summary This PR fixes a bug where the server wouldn't retain the cell content in case of a reorder change request. As mentioned in #12573 (comment), this change request is modeled as (a) remove these cell URIs and (b) add these cell URIs. The cell content isn't provided. But, the way we've modeled the `NotebookCell` (it contains the underlying `TextDocument`), we need to keep track of the deleted cells to get the content. This is not an ideal solution and a better long term solution would be to model it as per the spec but that is a big structural change and will affect multiple parts of the server. Modeling as per the spec would also avoid bugs like #11864. For context, that model would add complexity per #11206 (comment). fixes: #12573 ## Test Plan This video shows the before and after the bug is fixed: https://github.com/user-attachments/assets/2fcad4b5-f9af-4776-8640-4cd1fa16e325
1 parent 3169d40 commit f3c14a4

File tree

2 files changed

+25
-2
lines changed

2 files changed

+25
-2
lines changed

crates/ruff_server/src/edit/notebook.rs

Lines changed: 21 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -114,19 +114,38 @@ impl NotebookDocument {
114114
let start = structure.array.start as usize;
115115
let delete = structure.array.delete_count as usize;
116116

117+
// This is required because of the way the `NotebookCell` is modelled. We include
118+
// the `TextDocument` within the `NotebookCell` so when it's deleted, the
119+
// corresponding `TextDocument` is removed as well. But, when cells are
120+
// re-oredered, the change request doesn't provide the actual contents of the cell.
121+
// Instead, it only provides that (a) these cell URIs were removed, and (b) these
122+
// cell URIs were added.
123+
// https://github.com/astral-sh/ruff/issues/12573
124+
let mut deleted_cells = FxHashMap::default();
125+
117126
// First, delete the cells and remove them from the index.
118127
if delete > 0 {
119128
for cell in self.cells.drain(start..start + delete) {
120129
self.cell_index.remove(&cell.url);
130+
deleted_cells.insert(cell.url, cell.document);
121131
}
122132
}
123133

124134
// Second, insert the new cells with the available information. This array does not
125135
// provide the actual contents of the cells, so we'll initialize them with empty
126136
// contents.
127137
for cell in structure.array.cells.into_iter().flatten().rev() {
128-
self.cells
129-
.insert(start, NotebookCell::new(cell, String::new(), 0));
138+
if let Some(text_document) = deleted_cells.remove(&cell.document) {
139+
let version = text_document.version();
140+
self.cells.push(NotebookCell::new(
141+
cell,
142+
text_document.into_contents(),
143+
version,
144+
));
145+
} else {
146+
self.cells
147+
.insert(start, NotebookCell::new(cell, String::new(), 0));
148+
}
130149
}
131150

132151
// Third, register the new cells in the index and update existing ones that came

crates/ruff_server/src/edit/text_document.rs

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,10 @@ impl TextDocument {
3232
}
3333
}
3434

35+
pub fn into_contents(self) -> String {
36+
self.contents
37+
}
38+
3539
pub fn contents(&self) -> &str {
3640
&self.contents
3741
}

0 commit comments

Comments
 (0)