Fix leaking comment thread when CellComment is reused. #214589
Merged
+50
−50
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rehmsen
commented
Jun 7, 2024
|
Hi @alexr00, did you have a chance to look at this? Do you have any questions? It would be cool to get this fix in. Thanks! |
alexr00
reviewed
Jun 17, 2024
src/vs/workbench/contrib/notebook/browser/view/cellParts/cellComments.ts
Outdated
Show resolved
Hide resolved
Tyriar
previously approved these changes
Jun 17, 2024
src/vs/workbench/contrib/notebook/browser/view/cellParts/cellComments.ts
Outdated
Show resolved
Hide resolved
alexr00
reviewed
Jun 18, 2024
I wonder if VSCode or some extension I have keeps updating this? I am not doing it handishly. Anyways, gone again.
|
@rebornix Are you still happy with the changes I made in response to other reviewers' comments? Can this change be merged? |
|
@rehmsen sorry for the late response, the code looks great! Thank you for the contribution ❤️ |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #214585.
As none of the involved classes are covered by unit tests, to test this one needs to create a notebook with a code cell, then a page long markup cell, then a code cell, so that the two code cells are not visible at the same time, and the renderer reuses the same CellComments for them as you scroll up and down. Then you need to put a notebook comment onto the first cell. Before this change, that comment also is shown incorrectly on the second code cell when scrolling down as it is not correctly cleaned up. with this change, this does not happen, the CommentThreadWidget and associated DOM elements are removed, and then added back again cleanly when scrolling back up to the first code cell.