microsoft / vscode

Visual Studio Code
https://code.visualstudio.com
MIT License
162.63k stars 28.67k forks source link

Provide diff view inside comment card to preview suggested edits #192129

Open JonasMa opened 1 year ago

JonasMa commented 1 year ago

In our code review process at Google we're able to provide suggested edits as a reviewer. Currently we're trying to include these suggestions better in our review flow. We think an inline preview to smaller code suggestions in the comment card will be a very helpful tool for the code author. This could look something like this: image

While this looks a lot like an integrated peek view I'd expect some memory issues when displaying lots of them at the same time. That's why I think this would need to be a bit more custom.

I could see this being useful for all users, not only inside our organisation. What do you think? Is this a feature you could imagine having in your code base? If so we could imagine to help implement it.

alexr00 commented 1 year ago

We have the same need in the GitHub Pull Request and Issues extension, so this is something that we would like to have in our codebase. Given the current comment API, did you have some idea how we could add this?

JonasMa commented 1 year ago

The two obvious places are connecting it either to the Comment or the CommentThread I think. Having it connected to the Comment provides great flexibility as one can add multiple diffs per thread. However this might overload the threads visually. Our mental model of providing a diff is a suggested change by the reviewer, which then can be discussed in the follow-up comments in the thread. That's why I'd suggest having it in the CommentThread instead. This also provides easy options to apply the edit in the additionalActions of the thread p.e.

Regarding the data structure I see two main options we have:

  1. Provide a WorkspaceEdit. This describes the edit precisely and is easily applied to the code base when accepted.
  2. Just provide two strings for the respective sides of the diff and probably the lines. While being the simplest this will be dependent on the vscode diffing algorithm and generally be less flexible when one wants to extend its functionality like jump to next edit chunk.

While editing the code these edits might become stale so we need to decide how we want to handle updating this preview:

  1. The preview handles this. We provide the file path (or it gets taken from the comment location implicitly) and the preview handles the updates
  2. The calling side handles this and provides the file content next to the edit. This requires some effort from the calling side to keep this up to date. However this also introduces a lot of flexibility since it can be decided how responsive this should be, some use-cases may require some static diff only. Also it can be decided how much context around the changes will be provided.

Looking at these options I think having a WorkspacEdit combined with a content (chunk) string on the CommentThread is the most reasonable approach which provides some flexibility. But I might be missing something. What do you think?

Also a note on the implementation of the preview itself: As we potentially can have lots of these edit previews showing at the same time we need to be mindful about resources here. That's why it's probably too costly to just spin up new editor instances in the preview. Unsure if there is already something slim enough we could reuse or if it would be best to have a new component for this.

alexr00 commented 11 months ago

@JonasMa thanks for the reply and for your patience waiting for my response! I'm going to get designing the API for this feature on our October plan so that I can devote time to helping create a good API for you to help implement. I should be able to take a good look at your proposal and write a proper reply next week.

alexr00 commented 11 months ago

@JonasMa would a simple markdown based approach work for you? GitHub uses the following markdown syntax to indicate a suggestion:

image

Then, it uses the lines that the comment applies to as the left of the diff, and the lines within the suggestion block as the right side of the diff. A simple markdown based approach like this would be much quicker to implement as it wouldn't require any new API to discuss. We may still need to be aware of resource use as you say.

For your purposes, does a suggestion always apply only to the lines that a comment applies to?

alexr00 commented 11 months ago

@JonasMa I would be happy to accept a PR for the behavior outlined in https://github.com/microsoft/vscode/issues/192129#issuecomment-1745264952.

alexr00 commented 11 months ago

Also @laurentlb and @hermannloose.

JonasMa commented 11 months ago

Sorry for the late reply. Laurent, Hermann and me aren't working on this project anymore. Maybe @marrej, @poucet or @raphinesse might have a go at this?

poucet commented 11 months ago

How do we keep momentum on this going during this transition? We will definitely need this at some point.

alexr00 commented 11 months ago

@poucet, once you're looking at this again just ping in this issue and I'll see if I have cycles at that time to provide pointers and discuss the problem. If I don't have cycles, then I'll try to get it on our next iteration plan (we run monthly iterations).

Assuming the proposal I made in https://github.com/microsoft/vscode/issues/192129#issuecomment-1745264952 works for you then you shouldn't need too much assistance from me to be able to make a PR.

a-stewart commented 10 months ago

@JonasMa would a simple markdown based approach work for you? GitHub uses the following markdown syntax to indicate a suggestion:

image

Then, it uses the lines that the comment applies to as the left of the diff, and the lines within the suggestion block as the right side of the diff. A simple markdown based approach like this would be much quicker to implement as it wouldn't require any new API to discuss. We may still need to be aware of resource use as you say.

The problem with a markdown approach is it assumes that all code review systems use the same GitHub syntax. From a quick search, GitLab has something similar but slightly different (https://docs.gitlab.com/ee/user/project/merge_requests/reviews/suggestions.html) where you can specify line offsets.

With our system, suggested edits are not included in the comment, but are stored as metadata with the comment.

For your purposes, does a suggestion always apply only to the lines that a comment applies to?

Unfortunately not, for example you might comment the line List<Foo> foos = new ArrayList<Foo>(); and the comment might be "Suggest using a set here instead`. That would result in an edit which changed the highlighted line, but also changed imports at the top of the file too.

Also, with the GitLab example, their suggestion syntax explicitly allowed for offsets to modify more than just the commented lines.

We already have the concept of edits coming from extensions, perhaps it would be best to allow comment providers to pass in edits explicitly, and then it is up to the implementation wrt how suggestions are transmitted.

Basically just offloading the conversion of markdown blocks to edits to the extensions, but keeping the rest of the code the same?

alexr00 commented 10 months ago

The problem with a markdown approach is it assumes that all code review systems use the same GitHub syntax. From a quick search, GitLab has something similar but slightly different (https://docs.gitlab.com/ee/user/project/merge_requests/reviews/suggestions.html) where you can specify line offsets.

A comment providing extension can always modify the comment body before handing it to VS Code to display (GitHub Pull Requests and Issues does a lot of this to linkify usersnames and have better rendering for permalinks and suggestions), so I don't think this alone prevents the markdown approach. We can define our own suggestion syntax for this, which could even include an optional offset like GitLab has.

Unfortunately not, for example you might comment the line List foos = new ArrayList(); and the comment might be "Suggest using a set here instead`. That would result in an edit which changed the highlighted line, but also changed imports at the top of the file too.

In this case, what would the visual representation of the diff in the comment widget show?

a-stewart commented 7 months ago

Hi,

Sorry, this got a bit lost over Christmas.

Assuming this is still an open issue.

A comment providing extension can always modify the comment body before handing it to VS Code to display (GitHub Pull Requests and Issues does a lot of this to linkify usersnames and have better rendering for permalinks and suggestions), so I don't think this alone prevents the markdown approach. We can define our own suggestion syntax for this, which could even include an optional offset like GitLab has.

I think my question is that extensions will need to parse the comment structure from the service they talk with, and will then have the diff stored somehow in memory.

It seems odd to then encode that back into Markdown for VS Code to then pull a WorkspaceEdit or similar out of. Would it not be simpler to instead just pass in the WorkspaceEdit directly as part of the API?

stariolo commented 3 months ago

Proposal: Inline Diff Previews for Comment Cards API

Our team is keen to improve the code review experience in VS Code, especially for comments with suggested edits. We propose adding inline diff previews directly within comment cards, providing a quick, visual way to assess and apply changes without leaving the review flow.

This approach aligns with ICSE-SEIP'23 paper, "Towards More Effective AI-Assisted Programming: A Systematic Design Exploration to Improve Visual Studio IntelliCode's User Experience". See the paper or Summary for more details.

Why This Matters

What is wrong with the current comment card?

Based on user feedback, we identified the following pain points:

Group 1739329586

The concept

Frame 570

[1] Gutter icon: Appear next to the line number [2] Status: Status of the Comment card: Unresolved/Resolved [3] Small summary: Option to add a title to the card [4] Card actions: Area to add actions related to the comment card [5] Message / Error Description: Optionality for having collapsed messages. For comments we will display the entire message. For automatic detected errors: Collapsed message: Show 2/3 lines of the error message | Expanded message: Shows the entire message [6] Area for suggestions: Code preview of the suggested code. [7] Reply input field: This will be visible only for comments. [8] Action buttons area: Area for primary buttons like “Apply/Reject Fix”. [10] Area for external links: No visible for Comments, For automatic detected comments this is an area for external links like: Documentation, Link to external apps that are related to the error.

Figma with card variants and Nomenclature (Pass: GooG24*)

Prototype

gif

Video

Design Principles based on the paper

Implementation Proposal

We propose modifying the Comments API to allow comment providers to include a structured diff along with the comment text. This will enable the display of inline diff previews within the comment card itself. The API can be used for various types of comments, including human comments, automatically detected issues, and potential test results during code review time.

Next Steps

Let us know your thoughts and how we can move forward together!

albertelo commented 3 months ago

@daviddossett Hi David, here's the proposal we had discussed last time we chatted. Feel free to take a look and we're happy to answer questions or clarify