[Discussion] Reviews for pull requests

[queerviolet]

Problem

Overview

What should the review flow for pull requests look like? The main complexity here is that we have, at a minimum, three kinds of users coming in with different mental models.

Green Witch just wants to make a comment on a PR.

Bald Witch knows about Reviews (as a clustering of comments) from the web and wants to create them explicitly.

Boy Witch wants the behavior of Reviews (grouping many comments and not spamming their reviewees) but doesn't know what they are.

We'd like to keep them all happy (because if we don't they will ritually sacrifice us and eat our souls, just as they did to Macbeth—poor guy, but he really had it coming).

User Impact/High Level Use Cases

As Green Witch, I can add one-off comments to a PR
As Boy Witch, I can avoid spamming my co-workers with notifications. (This despite the fact that Boy Witch doesn't know what Reviews are, coming in.)
As Bald Witch, I can indicate that multiple changes are needed for my approval.

Justification

Users are asking for this functionality, and we have it on the web. If you have a lot of comments, the current review flow is pretty unfriendly—you end up spamming your coworkers.

Discussion & Mocks

After some faffing about trying to make the flow work with just one button, I think we should probably just have two: Send and Start Review.

We can workshop the names, but something I like about these is that (1) they're relatively short, and (2) it's obvious that send puts something on the server.

This arrangement makes Send feel like the default action. We may want to flip and de-emphasize it, if we want to encourage people to create reviews.

I also think that starting another comment should do the same thing as clicking Start Review. That is, a "Review" is just one or more comments that are sitting there in a draft state. The key point here is discoverability: if I want to make multiple comments before Sending them all out, but reviews sound strange and foreign to me, then I might try... creating multiple comments without clicking send. If we make that work, we then provide an on-ramp for people to discover what reviews are, and why they're useful.

Once a comment has been added to a review, we should show it inline, just as we do other comments, but with some clear indicator it's a draft:

Then when I add another comment, the options presented should be a little different:

Here, we want to make it clear that we're in a different state—specifically, that there's a review pending, and that comments are, preferentially, added to the review.

Finally, clicking any of the Draft badges or going to the Description page will show me my pending review:

We may want to workshop the buttons on this page, but that's the general idea.

[RMacfarlane]

I like the flow of this, I like having the whole review grouped on the Description page and being able to navigate to that from any of the draft badges.

I think it would be nice to be able to start and end a review in the same place. So in place of the Send button after a review has been started, I think there should be a Submit Review button the comment. Likewise, should there be a button to start a review on the Description page?

I would opt to not have the n comments in review text. Though it provides some reminder of where you are in the review, I think it doesn't add too much value weighed against the cost of making an API change to support it - it doesn't seem like a typical contribution point to the UI.

[mmanela]

I have a couple different thoughts here, one is on the proposal above and one is on how it merges with the current review mechanism.

1. In terms of making clear the distinction between single comment and review, should we just try to match closely to the GH model? Their comment box looks like this below. I think the "Add single comment" button is clearer in intent than saying Send.
   

GitHub uses the language "Pending" instead of draft, I like either but unless we have a strong reason to be different might as well match.

The other thing I noticed on GitHub is that they put the submit review button on every file.

This might be a convenient thing so you don't need to bounce around to the description page but I think what @queerviolet proposes is simpler ,especially since we will also need to support things like approval status and that would be easier in the description page:

2. Today the PR extension has a review mode where after you click checkout you are now in "Review Mode"
   

When I first used the extension, I thought that this was the same thing as the GitHub review, but this is different. An option could be to make this the actual bulk review mechanism, but that would mean we require you to checkout the branch to do a review, and I am not sure if everyone would want that. But my concern is the confusion over the two concepts.

[queerviolet]

@RMacfarlane — Concur, we don't need the n comments in review text. Mostly, I had it there to make it clear that there's a review ongoing, but I think we have enough other affordances that it's unnecessary

@mmanela — I actually don't think it would be too bad to require the branch be checked out, since then we could easily make multi-file suggested changes, something we've been discussing. Also, agree that we should just have a Finish review button.

What contribution points do we need to add to the VS Code API to enable this? Any chance of adding those for the early Dec release?

[rebornix]

While playing around with microsoft/vscode#64010, I found that GitHub v3 API is limited in Review comments management, with which we can't archive the same experience as github.com. Some blockers/limitations I have noticed

• We can create a review with draft comments, but once we created the pending review, there is no way to add more comments to it
• We don't know if there is a pending review or not. GitHub for Visual Studio fetches all reviews and check if the latest one is pending, but it would be great if we can have an explicit api.
• To fetch all comments visible to myself, we at least need to make two API calls
  1. Fetch all comments through /repo/**/pulls/**/comments
  2. Fetch all reviews, check if the first one is pending or not, and then fetch all comments in this pending review, as they are not shown in the first step, even if I already logged in.

For now to test the API, I wrote a sample extension https://github.com/rebornix/comments-provider-sample .

[dsmilkov]

Any timeline for this feature? We are using VSCode extensively for TensorFlow.js and our reviews can sometimes have 20-30 comments. We are currently using Reviewable, but if we had a review mode with draft comments in VSCode I can see us completely switching to VSCode for reviews.

[sahlouls]

Any updates coming for the new year ?

[RMacfarlane]

This is on our plan for January

[RMacfarlane]

Completed by 6d1f0de and cd7f73a