Who should mark PR comments as resolved?

27 May 2022

Before reading this post - note that when I say "resolving", I'm only talking about clicking the "Resolve" button for that particular comment thread. I am not talking about actually fixing the issue that the comment is describing.

The TLDR; version is that the person who started the PR comment should be the person to mark it as resolved.

Also note that I'm talking about PRs within a team - not open source projects.

The reason I'm writing this post, is that it's something I keep on seeing come up. I leave a PR comment, and the PR author makes a change to fix it, then they resolve my comment before I have a chance to review that fix/change.

What's wrong with this? It means changes can be merged into the main branch that haven't been reviewed at all. Which kindof defeats the purpose of a PR!

Take this scenario...

  1. Bob creates a PR for a feature he's been working on
  2. Both Frank and Jane are reviewing the PR
  3. Jane spots a bug, and adds a comment
  4. Frank doesn't spot the bug and approves the PR
  5. Bob fixes the bug, marks Jane's comment about the bug as resolved, and then completes the PR

However, Bob misunderstood the bug fix, and didn't fix it correctly. Jane, who spotted the bug hasn't had chance to review/accept the new changes, as Bob wrongly completed the PR comment on her behalf. Unreviewed code has now made it into the main branch, which could well be CI/CDing straight to production!

If the author of the PR comment is always the person who has the responsibility of marking that comment as resolved - this gets rid of this problem, and all changes will have had by at least two people's eyes on them.

Search


Recent Posts


Featured Posts


.NET Oxford Links