Visualstudio: Code Reviewing in Visual Studio

[donokuda]

Continuing the work that @simurai and @shana did for pull-requests (#20) and exploring the code review process in Visual Studio.

1. Click on an open pull request in the GitHub explorer to show PR details
2. View list of "Files changed" tab
3. Select a file from the list to open it in Visual Studio in a diff view
4. Hover over a line and click on the speech bubble opens a popup to add a comment
5. New comment appears in the GitHub explorer conversation tab
6. A description of number of comments, last commenter, and latest comment time is added above the line in the diff view.

Mocks!

Example of popup when adding a new comment:

Here's an example when specific parts of the code has been reviewed using diff notes (5 + 6). Conversations around a particular file are grouped together under the conversations tab in the GitHub Explorer. In the editor, a speech icon calls out lines with associated comments:

Would love some :thought_balloon: on this flow

/cc @github/windows

[donokuda]

Variation of the comment modal using the Peek Definition design pattern:

[shana]

I think the flow makes sense. So how does the conversation view look like in general? We show the PR comments first, and then the file-specific comments?

[donokuda]

So how does the conversation view look like in general? We show the PR comments first, and then the file-specific comments?

PR comments and file comments will appear in the order they are created similar to how pull request conversations are displayed on dotcom/dotcom-mobile. Here's a larger example with updated timestamps demonstrating the order of which comments appear:

Currently, I feel like it's difficult to follow a pull-request's conversation with both general comments and file specific comments in the same feed. I'll try out a version with file specific comments separated out from the main conversation to see what that could be like.

Just wondering: When you're in the process of having your code reviewed, what do you look for when you are receiving feedback in order to move forward?

[shana]

Let's see if I can put this into words... So there's two sides to a code review: the reviewer(s) and the reviewee.

Reviewer
On the side of the reviewer, I add comments and then switch the assignment of the PR, if the PR is assigned to me. If it isn't, I just wait for the feedback replies and code changes. There's no easy way to know when the reviewee is done and I can look at it again to see the results of the feedback, so we rely on comments and regular checking in. When I'm happy with a PR, I leave an emoji comment.

Reviewee
On the side of the reviewee, what happens is that I wait for feedback and then respond, by doing code changes and by replying to the comments. I usually always reply to the inline feedback immediately as a way of marking to myself that I have seen it and it's on a todo list. This takes advantage of the fact that on the website comments get hidden when the diff changes.

So what's happening here is that I'm using the visual aid of the disappearing comments to mark what things I've handled completely and what things I still have to handle in a PR. Some feedback just requires a reply and no code changes, so I scan for my comments regularly to see if anything still needs handling. Finally, when everything is handled, I signal back that the PR is ready to merge or have more feedback: I do this by switching who is assigned to the PR, and we also usually add a little emoji comment (like :sparkles: or :shipit: or :rocket: or :pineapple:) to mark that I'm done with this phase of feedback.

In VS those visual cues won't be so easily accessible, if at all. Tracking which feedback I need to act on and which I've already dealt with is potentially a problem we have to solve here.

[donokuda]

@shana :raised_hands: This is excellent. Thank you!

So what's happening here is that I'm using the visual aid of the disappearing comments to mark what things I've handled completely and what things I still have to handle in a PR.

Good point. I've often use the collapsed "outdated diff" comment as the "done" equivalent for a PR. How about separating out file-specific comments out to their own sub page in the conversations tab? This allows a reviewee to see which files have been reviewed, jump to that file, and commit changes. When the reviewee pushes their branch with their changes, any files with an outdated diff (implying a fix has been implemented) are marked with an :heavy_multiplication_x: (or maybe a :heavy_check_mark: ).

Here's a wireframe of what this could consist of:

There's no easy way to know when the reviewee is done and I can look at it again to see the results of the feedback, so we rely on comments and regular checking in.

Yeah. In my experience I've often :bell: and re-:bell: anyone who has left feedback for another review. I want to explore possible ways to indicate opened PRs that are ready for re-review. There's the assignee field, but it's limited to one person.

Maybe something along the lines of:

1. Reviewee gets notified that their PR has been reviewed
2. Checks inline comments, makes changes to related files
3. Commits and pushes changes to their branch
4. Clicks a button that indicates the PR is ready for another review
5. Reviewers get notified that a PR they've reviewed is ready for another look
6. Reviewer selects the open PR and reviews the updated files
7. Reviewer is satisfied with changes, comments with a :+1:
8. Reviewee merges their changes

[shana]

Marking outdated comments feels right, yup!

There's some changes to PRs (https://github.com/github/core-product/issues/256) that have just shipped to staff, that might provide some new ideas. I feel like the workflow you describe works well, we just need to figure out a way of indicating that the PR is ready for review or that a review is ok/not ok to merge. Let's work on the assumption that we have such a way and do a UI on that assumption, and then I'll figure out a way to implement it in case we don't have an API to track these state changes.

The workflow that GitHub relies on for code reviews is to code review the whole PR as a unit. This is not actually how most open source projects work, and depending on the size of a codebase, it's not how a lot of non-oss projects work either. GitHub internal development relies on small modular codebases controlled by small teams. Changes can be made small, so PRs are manageable as a whole unit of review.

Now imagine a project whose codebase runs into the gigabytes, with thousands of files, with 20 different teams contributing to it. Any PR made on such a codebase will likely affect multiple teams and will touch a large number of files. Reviews require :eyes: from multiple teams, each responsible for their one areas of the code, plus someone else in charge of making sure that the change as a whole integrates well into the system (because each individual reviewing team will tend to fall into the "hey, it works for me" mode of reviewing and will not pay close attention to the interactions across systems that a change might cause). Having everyone review the whole PR is a waste of time and potential cause for things slipping past people, because everyone's trying to find the change that's relevant to them among a huge bunch of irrelevant (to them) changes.

A common strategy in this scenario is to make each commit a reviewable unit that's related to a particular area of the codebase. This way, each owner of an area can concentrate on reviewing the parts that matter to them (one or more individual commits), and sign off on those individual commits. The sum of all the sign offs of the individual commits by each reviewer gets added to the go/no go status of the PR as a whole (i.e., you add a bunch of different reviewers, those reviewers review and sign off on individual commits, optionally you have one other reviewer responsible for making sure the change as a whole makes sense, and when everyone has signed off on all the commits that make up the PR, the PR is automatically good to go)

How would we handle this type of code reviewing in our UI?

[donokuda]

we just need to figure out a way of indicating that the PR is ready for review or that a review is ok/not ok to merge.

Yup! There might be an opportunity where we can leverage commit sign-offs and outdated diffs to infer that a PR is ready for another review or ready to merge.

A common strategy in this scenario is to make each commit a reviewable unit that's related to a particular area of the codebase. This way, each owner of an area can concentrate on reviewing the parts that matter to them (one or more individual commits), and sign off on those individual commits.

I wonder what it might be like to assign teams or individuals to review specific commits:

1. Start creating a pull-request
2. Add title, description, and optionally lead assignee (the individual responsible for looking at the overall PR)
3. View list of commits, select one or more commits, and mark them so they require signing off by an individual or team
4. Open pull request

Reviewer: Signing off

1. Reviewer sees an open pull request that requires their review
2. Opens PR, views list of commits assigned to them for review
3. Clicks on commit to view its details, inspects each file, and leaves inline feedback
4. On commits not needing any feedback, reviewer clicks on a "Finish Review" button. The commit is amended with a "reviewed-by:" tag.

I started sketching out some ideas for these flows but wanted to communicate some of my higher level thinking around this.

If we introduce signing off as a requirement for a merge-able PR, we could use something similar to our status bar...

This area could hold more information such as showing which commits still needs reviewing.

[donokuda]

Moving conversation over to https://github.com/github/windows-ecosystem/issues/21 :smile:

[wetteral]

I'm curious what happened to this, seems like a super cool feature.

[julealgon]

I'm also really curious about this feature. It would be the best feature for the whole extension IMHO. The link to the conversation provided by donokuda is broken. Do we have any updates on the status of this feature, and why it was never implemented?

[shana]

We agree that code review is one of the best features we can have, and we're working on it! We've split it up into different features that include #491 #492 and #549 to start with, and given the new PR review features on github.com, we want to leverage the API for it on Visual Studio as well. We're not a large team so things move slowly, but hopefully we'll be able to ship cool stuff like this soon 😄

[haacked]

The code review API is in preview - https://developer.github.com/changes/2016-12-16-review-requests-api/

[ankitbko]

Bump! I am curious what happened to this feature?

[shana]

@ankitbko It shipped! 🎉 Grab the extension and check out the GitHub pane, accessible from the Pull Requests button in Team Explorer Home or from View -> Other Windows -> GitHub. You can view PR details, checkout PRs, jump to the files and comment inline in the file diff, among other things 😄

[ankitbko]

Thats great @shana. Thanks.