Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
JonasMa opened this issue Sep 4, 2023 · 14 comments
Open

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

JonasMa opened this issue Sep 4, 2023 · 14 comments
Assignees
Labels
api api-proposal feature-request Request for new features or functionality
Milestone

Comments

@JonasMa
Copy link

JonasMa commented Sep 4, 2023

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
Copy link
Member

alexr00 commented Sep 7, 2023

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?

@alexr00 alexr00 added feature-request Request for new features or functionality api api-proposal labels Sep 7, 2023
@alexr00 alexr00 added this to the On Deck milestone Sep 7, 2023
@JonasMa
Copy link
Author

JonasMa commented Sep 12, 2023

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
Copy link
Member

alexr00 commented Sep 26, 2023

@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 alexr00 modified the milestones: On Deck, October 2023 Sep 26, 2023
@alexr00
Copy link
Member

alexr00 commented Oct 3, 2023

@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?

@joaomoreno joaomoreno changed the title Provide diff view inside comment card to preview suggested edits. Provide diff view inside comment card to preview suggested edits Oct 6, 2023
@alexr00
Copy link
Member

alexr00 commented Oct 19, 2023

@JonasMa I would be happy to accept a PR for the behavior outlined in #192129 (comment).

@alexr00 alexr00 modified the milestones: October 2023, Backlog Oct 20, 2023
@alexr00
Copy link
Member

alexr00 commented Oct 23, 2023

Also @laurentlb and @hermannloose.

@JonasMa
Copy link
Author

JonasMa commented Oct 23, 2023

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
Copy link

poucet commented Oct 25, 2023

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

@alexr00
Copy link
Member

alexr00 commented Oct 25, 2023

@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 #192129 (comment) works for you then you shouldn't need too much assistance from me to be able to make a PR.

@a-stewart
Copy link
Contributor

@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
Copy link
Member

alexr00 commented Nov 16, 2023

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
Copy link
Contributor

a-stewart commented Feb 19, 2024

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
Copy link

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

  • Clearer Communication: Diffs help avoid misunderstandings about the proposed changes.
  • Improved Usability: Aligned with user expectations from other code review tools.
  • Faster Reviews: Reviewers can instantly see the impact of suggestions, speeding up the review process.
  • Reduced Context Switching: No need to open separate diff views, keeping reviewers focused on the code.
  • Common pattern across comments: There are patterns for code review comments that were proven to be successful, it makes sense to reuse them in VSCode as well.

What is wrong with the current comment card?

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

  • Comment cards expands on width making it hard to users to hit the buttons in the right corner
    -- You can see the previous discussion on this thread.
  • Extra click to see and apply a code edit suggestion: Even if the suggestion is small (i.e. rename a variable) users need to click “Show fix” to see the fix. This is mentally heavy and takes a lot of time for users.

Group 1739329586

The concept

  • Visual: Small diffs are displayed directly within the comment card, user can apply/reject from there.
  • Expandable: Larger diffs should be expanded for apply/reject.
  • Interactive: Users can quickly apply or reject the suggestion or minimize the card
  • Efficient: Implementation is mindful of resource usage for performance.
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

  • Glanceable suggestions. Suggestions should be proactively visible to the user to make it easy to discover and take action.
  • Juxtaposition. The suggestion should explicitly indicate and juxtapose the existing code affected by the suggestion and the proposed code to improve user comprehension and support quick action.
  • Simplicity through familiarity. Reusing existing familiar interface elements to indicate proposed changes reduces visual clutter, improves comprehension, and reduces cognitive load. Comment card component we will use to provide a suggestion in context with the message.
  • Sufficient visibility for validation. The users should be able to see the whole suggestion when taking the action to prevent premature-commitment. We will display the entire snippet of the code for small suggestions (up to 8 lines) with the “Apply”/”Reject” CTA (call to action). But for large suggestions we will replace the “Apply”/”Reject” CTA for “Show fix” that will open the “Refactor Preview”.
  • Snoozability of suggestions. The user should be able to snooze inline suggestions to prevent interruptions. By allowing users to minimize the suggestion or to “Reject” the suggestion we are covering this solution.

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

  • Gather Feedback: We'd love to hear from the VS Code team and community on this proposal.
  • Collaborate: We're eager to contribute to the implementation of this feature.

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

@albertelo
Copy link

@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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api api-proposal feature-request Request for new features or functionality
Projects
None yet
Development

No branches or pull requests

6 participants