ruff server: Defer notebook cell deletion to avoid an error message
#11864
Conversation
|
|
Any chance that this solves #11851 |
Created #11867 for that. According to this message's log it seems he was got multiple error on closing a notebook document with multiple cells in it. |
|
Reading through the comments, I think the change makes sense. However, I would find it helpful if we can expand the comment explaining how cells are closed now with an explanation on how VS code handles it. I think that would also be helpful for someone coming back to this PR to understand why and what has changed. |
CodSpeed Performance ReportMerging #11864 will degrade performances by 9.48%Comparing Summary
Benchmarks breakdown
|
## 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
Summary
Fixes astral-sh/ruff-vscode#496.
Cells are no longer removed from the notebook index when a notebook gets updated, but rather when
textDocument/didCloseis called for them. This solves an issue where their premature removal from the notebook cell index would cause their URL to be un-queryable in thetextDocument/didClosehandler.Test Plan
Create and then delete a notebook cell in VS Code. No error should appear.