Allow extensions to provide a source mapper for comments on notebooks. #246317
Conversation
VSCode can render review comments in the NotebookEditor, but only when they use vscode-notebook-cell: URLs. These URLs require knowing the internal cell ID, which is not exposed to extensions. Another challenge is that comments and notebook serializers are often provided by different extensions (e.g. comments by the Github Pull Request extension, the NotebookSerializer by the Jupyter extension). So in practice, it is difficult to provide such comments. This change proposes a different approach, where NotebookSerializers can optionally provide a `NotebookMapper`, which can translate file ranges to notebook ranges. If such a mapper is provided, VS Code will, upon adding a notebook document or a document comment, convert the document comment into the corresponding notebook comment, so that it is rendered in the NotebookEditor. As we cannot change the type of `CommentThread.range` on the fly, a new `NotebookRange2` type is created which inherits from `Range` and additionally contains the cell index and cell part (source, metadata, maybe later outputs). When a thread is associated with a specific cell via the vscode-notebook-cell: URI, it is sufficient that all downstream components consider only the positions shard with `Range`. The cell index and cell part are only used to determine the correct cell. Currently, the NotebookEditor renders notebook comments at cell granularity only and does not care about the exact Range where the comment was left. However, this is a natural extension that would already be supported by the proposed NotebookMapper API.
|
@rebornix @alexr00 Here is a proposal for how we could address #214017. One aspect I am still unsure about is if it would not be better to keep the mapped range independent from the original range. Ideally, I would like the same comment to show both in the the Text and the NotebookEditor, as two different views of the same model. This would require more changes to the NotebookEditor, so it can read and respect the mapped range of comments attached to a file: URL. If that makes sense to you, let me know and I can try that out, too. Looking forward to your feedback. |
|
Thanks for checking. I have adjusted the unit tests to pass I verfied that Now passes locally. I also tried But that fails for me with:
I am trying to figure out why. Maybe that is some Mac issue? In the meanwhile, is there any way I can trigger CI on Github or does this happen automatically after a while? |
|
Looks like it is run automatically, and the tests still fail. I figured out how to run the tests locally - the main issue was that you first need to run the UI locally for it to create the build/ folder - it seems the test script itself does not actually do the build - it relies on the files already being there. Now that I figured that out, I will work on fixing all the tests and send a new push when done. Sorry for the delays. In the meantime, any opinions on the design are also very welcome. |
|
I will yield to @rebornix review, I only chimed in as I saw the build be red. |
|
Gentle ping @alexr00 @rebornix - any thoughts on this? |
|
This would be phenomenal to have! 😮 Hopefully you'll be able to merge this soon. Godspeed team 🙏🏻 |
|
@rehmsen thank you for the PR. We will try to get to this, but our focus isn't on Notebooks + Comments at the moment. Thank you for being so patient! |
|
Understood. We are not blocked on this as we have patched our version already, but it would of course be nice to get this into upstream so it is also useful to others. |
|
Like @alexr00 mentioned above, supporting comments in notebooks is a stretch for now. In the meantime, I’ve been exploring other areas where we might leverage source mapping. I noticed that the Search View includes experimental support for searching text in closed notebook documents via search.experimental.closedNotebookRichContentResults. For example, when a user clicks a match in the Search View (say, the 10th match), the notebook document is opened and we try to locate the same match in the editor. But this isn’t very accurate, we’re just matching the same text and position heuristically. I wonder if we could use a source map instead to accurately navigate to the correct match location. I will dig into this a bit more and see if I can build a prototype on top of this concept for notebook search. Will keep you all posted. @rehmsen |
VSCode can render review comments in the NotebookEditor, but only when they use vscode-notebook-cell: URLs. These URLs require knowing the internal cell ID, which is not exposed to extensions.
Another challenge is that comments and notebook serializers are often provided by different extensions (e.g. comments by the Github Pull Request extension, the NotebookSerializer by the Jupyter extension).
So in practice, it is difficult to provide such comments.
This change proposes a different approach, where NotebookSerializers can optionally provide a
NotebookMapper, which can translate file ranges to notebook ranges. If such a mapper is provided, VS Code will, upon adding a notebook document or a document comment, convert the document comment into the corresponding notebook comment, so that it is rendered in the NotebookEditor.As we cannot change the type of
CommentThread.rangeon the fly, a newNotebookRange2type is created which inherits fromRangeand additionally contains the cell index and cell part (source, metadata, maybe later outputs). When a thread is associated with a specific cell via the vscode-notebook-cell: URI, it is sufficient that all downstream components consider only the positions shared withRange. The cell index and cell part are only used to determine the correct cell.Currently, the NotebookEditor renders notebook comments at cell granularity only and does not care about the exact Range where the comment was left. However, this is a natural extension that would already be supported by the proposed NotebookMapper API.
Issue #214017