0:00 Hey, I want to tell you a bit about a project I've been working on called required comments. 0:05 I've been thinking about this area for a long time now, probably about three years, and they might make it seem like it's just a small feature, but it's actually a proposal for a complete overhaul of how code review on GitHub works. 0:20 So this is what things look like today. 0:23 If I review a pull request, this is what I see. 0:26 I see a comment box and I see a bunch of buttons down here, either add a single comment or start a review. 0:32 So every time I comment, I have to pause and decide what type of comment I want. 0:37 Do I want a single comment that comments immediately? 0:40 Or do I want to start a review which starts a batch of comments and then submits them all together? 0:45 Now, even if I don't want to start a batch, there's another distinction that's important between these things and adds cognitive load at this point. 0:53 So the add single comment button doesn't really do anything in terms of review status in terms of like having any weight on the PR itself versus start a review, which actually affects something, has a review status associated with it and can block things. 1:09 So even if I don't want to start a batch, I basically kind of have to start a batch if I want this comment to carry any weight. 1:16 Now, when I'm done with the batch, I made either one or more comments and I want to now associate or publish them. 1:25 I can associate a top level review status with them. 1:29 So either comment, approve or request changes. 1:32 Now, if I comment, it doesn't really mean anything. 1:37 Really, a comment is just a vehicle to group a few comments together and then leave a neutral review. 1:43 And in this case, I might as well not include a review status at all because I'm saying, you know, either merge it or don't merge it. 1:50 It doesn't really matter. 1:51 I don't have an opinion here. 1:53 So that top level comment isn't really going to be as valuable. 1:58 Now, approving actually adds my formal approval to the PR. 2:02 So it has meaning and that can interact with branch protection policies to put the PR in a mergeable state. 2:08 But often an approvals top level comment can be a bit redundant, usually just like looks good to me or a nice friendly message. 2:15 It doesn't really mean anything because the approval itself inherently means this is good to go. 2:21 So you don't really get much value out of the top level comment. 2:25 And then request changes, if you select this last one, you actually block the PR from merging. 2:32 But even on these, the top level comment isn't often very actionable. 2:36 It usually points to comments included, like more granular comments included within the review and says, hey, look at these for the actual thing that I want you to change. 2:45 This one also feels pretty bad to people. 2:48 And I've heard from many people who use GitHub that they actually tend to avoid requesting changes because it's it's seen as impolite or something. 2:54 So none of these top level review comments are especially useful for any of these states. 3:01 And most of their value, I think, can be replicated with just a conversation comment saying like, hey, here's some holistic feedback that I want to give you. 3:09 On top of that, when an author is ready to read through all the reviews, all the comments that people leave, all the individual comments kind of carry the same weight, whether they are review comments or individual comments. 3:22 You can't really tell which one is semantically more important. 3:25 Like it's hard for you to find this very important comment. 3:28 You just kind of have to read through all of them and figure out whether something is actually important to integrate or isn't. 3:35 From a reviewers perspective, there's no easy way to take this comment and say, hey, I want to enforce that this is integrated or even I just want to tell if someone did this. 3:48 So it's very hard to see it, especially if it's outdated or if someone has resolved it and it's disappeared even without integrating it. 3:55 And when it's outdated, I mean, in that case, the only way to find it is open the conversation tab and try and look through the outdated comments, some of which are truncated, some of which have disappeared between like ellipses. 4:07 So it's very easy for things to get lost in the current model. 4:13 And so what I'd like to introduce and propose is a new code review model and a new way of thinking about comments within GitHub and pull requests. 4:26 So in this model, there's no requesting changes anymore. 4:30 So the review status can only be either approving. 4:33 I'm good with this. 4:34 I think this is good to merge or neutral. 4:36 Anything else. 4:37 Just I have no opinion. 4:39 Depending on the PR and the branch protection rules, you might need a certain number of people to approve reviews to merge. 4:47 You might need a specific compliance or code owner team to be approving. 4:52 So if you've touched their codes, they need to say approvals. 4:55 But a reviewers top level status is never blocking. 4:58 So you might be asking, OK, so if I am a reviewer and I want to stop this from merging until something happens, how do I do that? 5:07 Well, you still can, but it's a bit different. 5:10 So in this model, people never block merge. 5:13 It's requirements that block merge or tasks. 5:17 So individual comments can now be set to required. 5:20 And then these required comments block merge until they're resolved. 5:24 So now merge requirements might look something like this. 5:27 You need at least two reviewers with write access for approving. 5:31 This team X whose code you've touched needs to approve. 5:35 And also on top of that, no unresolved required comments can exist on this. 5:39 So if there are unresolved required comments, then this is unmergable. 5:44 So it's a bit different from what we have today. 5:47 And I want to show you what that might look like. 5:50 So this is just a regular files change tab. 5:54 This thing I floated down here isn't part of the UI. 5:57 It's just like the merge box. 5:58 I wanted to include it so you can see the status of the PR along every step of the way throughout this flow. 6:05 So right now it's just the default. 6:07 Hey, you need a review from two compliance or code owner teams because you've touched their code. 6:12 And until you do that, then you can merge. 6:14 And this is the most important thing. 6:16 So let's say as a reviewer, let's say I'm not a code owner. 6:22 I'm just a regular reviewer. 6:23 I want to leave a comment. 6:25 You can notice that here there's no longer this like, hey, start a review, leave a single comment or whatever. 6:31 There's just comment. 6:32 I don't really have to think about it. 6:33 There's no cognitive load. 6:34 All I really have to decide is do I want to mark this comment specifically as required? 6:42 And let's say here I don't. 6:43 So I'm just going to leave this comment. 6:45 It's a regular comment. 6:47 Nothing really special about it. 6:49 So you can see it's not really affecting the review. 6:52 So let's say like two hours, four hours go by and the other code owners come in and they review and they approve things. 6:58 So the comment still exists here and you can see that it's not really affecting this. 7:02 I'm a pending reviewer. 7:04 You don't need me. 7:05 The changes are approved. 7:06 It's all good to go. 7:08 But now let's say I add another comment and this time I mark it as required. 7:13 So the moment that I comment here, you can see that now there is an additional merge requirement, which is one required comment that needs to be resolved. 7:24 And this is prioritized. 7:25 So, you know, this is the most important thing. 7:27 So now if I'm the author of this PR and I come in and I look in and I see what I'm going to do. 7:31 And I look in and I see this merge box and I can say, OK, there's one required comment that I need to deal with. 7:36 Let's see what it is. 7:37 And I click on that and it can open and then I can see in context, like with its code context, what needs to be changed. 7:46 If I want to, I can jump to the content to the context and I can see it in there or I can just pay attention to all of the different comments that I need to track. 7:56 So this becomes kind of a to do list. 7:58 And if we want, we can even make this like a bit more in depth. 8:02 And now you can also interact with this comment and tackle it in here. 8:08 So let's close that. 8:09 So now if I want to resolve this comment, I can do that pretty easily. 8:15 It's not a required comment. 8:16 I know that the the reviewer who left this comment didn't think this was super important and it could just be educational. 8:25 It might be a tip. 8:26 It might be a knit. 8:27 It might be just some general feedback, but I can say, OK, I've seen this. 8:30 I'm not really going to do anything with it. 8:32 I'm resolving it. 8:33 I click that and it's resolved. 8:36 It's a bit different now when I try and resolve this comment because this the reviewer set up is required. 8:42 So when I tried to resolve this, it goes into what is this pending resolution state. 8:49 And this is basically saying, hey, this reviewer said that this was really important. 8:54 And so we are not going to inherently trust that this has been done. 8:58 We're going to give the reviewer the tools to say, hey, was this thing actually integrated in the way that I meant? 9:05 And so you see this timeline item is added. 9:07 Hey, Octocat, the author asked to resolve this conversation just now. 9:11 And if you hover over this, you can either un-request resolution or you can see, hey, this is pending Califa's review, who is the reviewer in this case. 9:20 So if I was to jump over to I'm the reviewer now, I am Califa. 9:26 Now I have two options. 9:29 I can either say, OK, you know, the author asked to confirm this as resolved. 9:35 I can either confirm it as resolved because I see that it's done or I've decided it's not that important. 9:40 Or I can see, hey, this isn't really done. 9:42 I'm going to keep it as unresolved. 9:44 So I'm going to go ahead and do that. 9:45 I think there's room for some more improvements here. 9:47 I think you can do a bit more. 9:49 Keep unresolved. 9:50 Then it says, like, reopen this conversation or something like that. 9:54 Something that doesn't maybe we can find language that just make sure that this isn't passive aggressive or something. 10:02 But to say basically someone asked to close this and it's not ready to be closed yet. 10:07 Maybe we even get rid of this entire like asked or didn't like asked to resolve and then didn't resolve and we just backtrack here. 10:15 So now at this point, there's still an open required comment. 10:19 The author can't go around the reviewer to to merge this even without doing the changes. 10:28 And unless they're an admin, I think admins should be able to override this. 10:33 And now as a reviewer, I can say, OK, you know what? 10:36 I looked through this. 10:37 This is kind of good to go. 10:39 And I can go up here and click approve. 10:42 We haven't really talked about this top area yet. 10:44 So you can see instead of the add your review button here that jumps down into like all of these different options, you just really have one button that says approved. 10:53 I click it. I'm approved. 10:55 If I don't click it, then I haven't approved. 10:58 Next to it, we also haven't talked about this new thing, which is similar to like this tracks the file that you viewed as a reviewer specifically personally what files you've seen this tracks which comments you've left. 11:14 And the reason I've added this is so that you never lose track of which comments exist on the page, even when they're outdated, even when they're in different contexts. 11:25 You can always see, hey, what was I thinking in my last review? 11:30 Is this thing good to go based on what I've asked for so far? 11:34 So you can see this right side here is orange and it's marked orange because one of the unresolved comments is required. 11:40 You can see one out of your two comments has been resolved. 11:44 So let's I click approve. 11:47 Now GitHub at this point says, hey, it's a bit weird that you're approving because you have this required comment and required means this needs to happen before merge. 11:57 If you're approving, you're basically saying, you know what, this actually doesn't need to happen. 12:01 I'm approving and I'm saying, you know, this is good to go even without this being done. 12:07 And so inherently, this doesn't make sense to be required. 12:10 So we say you can go ahead and approve, but this comment is going to become optional and unrequired. 12:15 And the moment that I click approve, I can say, OK, that's fine with me. 12:20 Also, I have a note down here that says this could follow auto merge patterns. 12:25 So I had a conversation with Sophie saying, like, you know, it could be really annoying this double confirmation thing every time and basically coming back to approve. 12:33 What if we could just have a checkbox or something that says approve when all of my comments have been acknowledged or integrated? 12:40 And I think that's really smart. 12:42 So we're currently working on how does auto merging a PR when all the checks have passed, what does that look like? 12:49 And I think this can follow similar patterns of just like, hey, give my approval the moment that all of my comments have been resolved. 12:57 OK, so in this case, let's say I resolve and this is going to change my comment to optional. 13:01 I'm going to approve. I click that and you can see immediately the PR is good to merge, at least from a review standpoint. 13:09 And I want to be really clear. This is not because I've approved. 13:13 This was already like had enough approvals. This had all the code owners approving. 13:17 I was an optional reviewer. This is because this comment, which was unresolved and required, is now just a regular comment and is optional. 13:26 So if I go up here and I change this to unapproved, you can see this is still approved because the comment is what was blocking it. 13:35 So if we were to rewind this from before we approve, this is a required comment. 13:40 This is blocking simply like clicking on confirm resolution here does the same thing. 13:45 It's like, OK, this comment is done. You've integrated the changes I want. 13:49 I feel like it's good to go. It's again approved even without my approval. 13:54 And so it was the comment that was what was blocking this. 13:57 So another thing that this little token up here actually this component goes to is when you click it, it opens this page up, which is the your comments page. 14:12 So you can see there's a list of resolved comments. 14:15 These are all the comments that you've made on this PR, regardless of whether they're outdated, regardless of whether they're resolved. 14:21 You have like a bird's eye view of everything that you wanted to happen with this PR. 14:27 And what that means is you can jump in and say, oh, you know what? 14:30 This comment actually isn't resolved like I thought it was. 14:33 But now there's a problem back or something like that or this this didn't happen the way I thought it should. 14:38 So I'm going to click on resolve and then it jumps up to unresolved comments. 14:43 It shows you basically the everything that every kind of comment that you have and its status. 14:49 And you can see the moment that I unresolved this. 14:51 Now there's a required comment that is not resolved. 14:54 And again, this PR is back to being unmergable. 15:00 So I like this model over the other one for a few reasons. 15:05 First, you don't ever have to think about what type of comment model to opt into. 15:11 You don't have to say, is this a review? 15:13 Is this a batch? 15:14 Is this something that can block or is this a single comment that I'm just adding now? 15:18 Everything is just a comment. 15:20 And then you can either mark it as required or not. 15:23 So it's a much simpler model. 15:24 And that also means that, you know, the relative impact of a given comment isn't tied to batching behavior, which right now it is. 15:33 And I don't see a reason for that to exist. 15:36 The second thing I think is it takes the blame of blocking a PR off of an individual reviewer. 15:45 So over here, it's like, hey, Califa requested a change. 15:49 This person is blocking you. 15:50 There's no additional context. 15:52 All you see is there's a certain person who is not allowing merge. 15:55 And instead, it frames it around. 15:57 This is the work that needs to be done. 15:59 So you still see who's asking for the work, but it's a bit clearer about this is what needs to happen in order for this thing to be merged. 16:06 And so the ball's back in your court to basically do the things or comment on the things and ask for them to not be required. 16:16 This new model encourages actionable feedback over high level comments. 16:21 So you can see up here in many like request changes scenarios, the top comment is just like, hey, there's some things that need to be resolved. 16:29 And then you look down and then you see the individual things and you don't know which of those things is really important. 16:36 So here there's no top level comment. 16:38 It just says, hey, here's the thing to do. 16:40 Here's another thing to do. 16:41 I can leave a top level comment unrelated to that if I want to. 16:45 I think you should be able to comment on file like file level comments on an entire file. 16:50 I think you should be able to do PR level comments and all of that should behave pretty similarly. 16:55 So here you have some kind of visual distinction of this comment is more important than the other ones and maybe you should prioritize it. 17:02 And I don't know exactly what the styling is going to look like, but there's something. 17:06 Another thing I like about it is that having that explicit indicator that like formal column of, hey, this is required or not required. 17:15 It means that we can query those things and consolidate tasks into a single list. 17:21 So that clarifies next steps. 17:23 It ensures nothing slips through the cracks, even if it's outdated versus today where you kind of have to open the files change tab, go back to the conversation. 17:31 Tab, scroll through, see if there's anything outdated, go back through history. 17:35 Like it's really easy for things to get lost there. 17:39 And it's also just a lot of overhead for you to have to find and make sure that those things are in the right place. 17:45 And that's both for authors and for reviewers. 17:48 And also there's no reason why this mark is required list can't include those top level comments that just like a conversation comment that just says, hey, mark this comment is required, even though it's not associated with a given diff or line. 18:04 I think all comments should be pretty similar in their functionality, like they should all be threadable. 18:09 They should all be resolvable. 18:11 Yeah. 18:12 And then really a big thing here is approving becomes really simple. 18:17 I don't need to think through all of this stuff. 18:19 It's either just I'm approving or I'm not approving because comment doesn't really mean anything. 18:23 Requested changes is now taken care of by individual required comments. 18:27 And so all I'm left with is either am I approving or am I not approving? 18:32 Now, if we want to encourage like leaving a top level positive comment on approval, something like, hey, this is really great. 18:40 I think that's great. 18:41 And I think we should focus on that. 18:43 But it doesn't necessarily belong or it's not tied to a given commenting model. 18:48 We can just encourage that in different ways. 18:51 So you may be asking, hey, Joel, you've been thinking about this. 18:56 Well, you've been thinking about this for three years now. 18:59 So why is now the right time? 19:01 Well, Microsoft has this initiative called One ES, where they are migrating all of their work onto GitHub. 19:08 And that requires that we build some of the functionality that exists on Azure DevOps and some of their other tools. 19:15 And one of the first features that they've prioritized, along with a few others, is enforcing a policy where all comments must be resolved before merge. 19:25 So I, you know, I already had this idea less fleshed out than it is now as part of the PR vision in December. 19:32 And while it's not exactly the same thing that they're talking about, I think it answers the use case that they're looking for. 19:40 So instead of having to resolve everything, this model says, you know, we are actually explicitly asking people to define what's important and what isn't. 19:49 So you don't need all that overhead of resolving every single comment. 19:52 You can only resolve the things that are truly important. 19:55 So I think because of that need over there, because I see this honestly as one of the most confusing and impactful areas that we could touch in the code review workflow. 20:08 I think now is a really good time to start talking about shipping this. 20:11 And I think this can be shipped incrementally. 20:16 So I've broken this up into a few phases that I think makes sense. 20:20 The first one is just blocking merge. 20:23 This thing that Microsoft wants, the main value of this feature is we can keep the existing review model. 20:30 We don't even need to touch that. 20:31 We just add this required checkbox. 20:34 And if you check this, then all comments that are required need to be need to be resolved. 20:42 And we just add this required to all of our comments. 20:44 Then we can build a GitHub app or something that's like not fully native to just fail if there are any unresolved required comments. 20:51 So you can see this in the checklist just saying, hey, this is failing. 20:54 Maybe details takes you the links to those comments or something like that. 20:57 We can figure out like the best balance of experience here using the tools that we already have at our disposal. 21:06 Then phase two is all about the author experience and creating a to do list of like these are the things that you need to get done in order to merge this. 21:18 And if you've seen like action items or any of the previous visions, you know I've been thinking about this specific thing for a very long time. 21:25 So this just adds a native section. 21:28 It removes the GitHub app because we don't need that anymore. 21:31 It says if there are any required comments, block merge and list them here. 21:35 That's really that's it. 21:36 It just queries for, hey, you know, which comments have this associated required column. 21:44 If they if that's checked, then list them here and block merge. 21:47 That's really it. 21:48 And we can jump back to wherever they are and we can list their context the same way we do right now. 21:52 The third phase is taking this chunk out of the streamlined review model. 21:59 So up until this this up until this point, we haven't touched a review model. 22:04 You still have like review changes. 22:06 You say I'll request the changes on top of this thing. 22:08 So there is a bit of redundancy. 22:10 But at this point, once we've built those things, once we've shipped the value, we can talk about or ideally just do the overhaul of this review model. 22:20 And this includes making all comments the same. 22:23 So like I said, top level conversation comments should also be threadable and resolvable review comments and single comments should just be the same thing. 22:31 This would remove the start review button from comments. 22:36 So it would just be like comment. 22:38 That would be the only button there. 22:40 It would rename add single comments simply to comment. 22:44 Same thing as here. 22:46 Remove request change request changes status. 22:49 So that's a status that won't exist anymore. 22:51 It'll just be approving or neutral like the flow that I ran you through. 22:54 And then here, because we're talking about potentially not having batching, one of the big reasons that people use batch comments and reviews is also to avoid creating a lot of noise for other people who are subscribed to the PR. 23:10 And that's because every comment currently sends a notification. 23:13 So I think we can do some work here to just say, hey, batch like notifications together and make it so that when you leave a bunch of comments within a given time span, they all send as one. 23:23 So I think pretty simple work here. 23:27 It's a pretty low hanging fruit. 23:29 And then finally, replace that top level review changes button on the top right with just approve. 23:36 And now the final, final phase is the reviewer experience. 23:41 And so we've really helped authors until this point. 23:44 So reviewers, I think we can use this phase to like add this component that I was talking about to help people keep track of their past comments. 23:52 We can create that new page to consolidate all past comments, either resolved or otherwise, so that nothing is ever lost. 24:00 And then we can add that double confirmation to required comments so that reviewers can verify their feedback has actually been acted on. 24:07 So yeah, this is it. 24:10 I really enjoyed working on this. 24:13 I think it's really important and it represents a way I see like the future of code review at GitHub. 24:20 And I would love to build it. 24:23 So thank you for listening. 24:24 And please send me feedback. 24:29 Thank you.