Who Should Mark PR Comments As Resolved: PR author or PR reviewer?

A lot of developers are confused about who should mark PR comments as resolved. There are many code review tools but there is no defined process for resolving PR comments.

Hence, sometimes the PR reviewers resolve their respective comment(s) and approve the code to be merged into the master branch. Sometimes the PR author does the work of marking comments as resolved, and the PR enters the CI/CD pipeline. There is no standard as such, and it might appear that there is no right way of resolving PR comments.

In this blog, we discuss the same, who should mark PR comments as resolved? The PR author? Or the PR reviewer who added a comment?

What Are PR Comments, PR Author, PR reviewer, and PR Maintainer?

PR Author

The developer who raises a specific PR request is a PR author for the same.

PR Comments

After PR is raised, domain expert(s) get assigned to the pull request to review it. These reviewers may add a comment as feedback, a suggestion, a question, or a request to make a change in code to improve the functionality, and readability, or to comply with the defined code quality conventions (if any). These comments are referred to as PR comments. 

PR Reviewer

The reviewer is called the PR comment author, aka PR reviewer or assignee. The PR reviewers only guide the PR author to implement the suggestions for improving the code quality. PR reviewers don’t solve the issue for the PR author. Rightly so, PR reviewers could be internal team members or they could even be external reviewers. Hence, their role is only to share their expertise and guide the PR author. 

Maintainer

A maintainer is someone who approves the PR to be merged into the master branch. Marking PRs as ready to be merged is a right that always remains with the maintainer.

The Current PR Review Process

Once the PR reviewer adds feedback, the status of the conversation changes to ‘Active’.

Now, the PR author has two options:

1. Pushback on PR Comments and Request Resolving it Without Any Rework

In some cases, the PR author can rightfully deny abiding by the suggestions of the PR reviewer. 

This is because-

• How a feature or code logic is implemented can be subjective.

• At times, the PR reviewer nitpicks trivial changes that are not important and can be skipped for better software delivery efficiency. 

• In open source projects, a lot of comments are from contributors which are often trivial.

So, in such circumstances, what if the PR reviewer & the PR author are at the tug of war? Who will resolve the PR comments now?

2. Implement the changes

If the PR reviewer’s comment is critical to the software quality, the developer reworks the code and commits it back for the PR reviewer to review it again.

Again, the problem is the absence of a clear procedure to mark the PR comment as completed. 

There is confusion about whether the PR reviewer should always resolve the comments or sometimes the PR author can mark comments resolved too after the code rework.

Why is PR Comments Approval a Problem?

One can advocate that the PR author has made the necessary changes and updated the code. So, nothing wrong with the PR author marking PR reviewer comments as resolved. This could also speed up the code review process by minimizing the back-and-forth communication between the PR author and the PR reviewer.

But there is a caveat. 

A PR author might be biased in assessing the efficacy of the solution. 

If they self-approve the issue raised by the PR reviewer, it defeats the purpose of code review. There is a high chance that if the PR authors themselves mark the comments as resolved, the code vulnerabilities may make their way into CI/CD pipelines, and possibly into the production environment as well.

That’s a huge risk.

But if PR authors aren’t allowed to resolve on their own, and if they have to wait for prolonged times for the reviewer to be available, wouldn’t this be a big impediment to developer productivity? 

Not to mention, it would also hamper the maker time of the developer, as this would need frequent context switching between active code development and PR comments implementation. 

In fact, in the absence of a proper PR review tools stack, a developer may spend productive hours shuffling between the PR comments looking for which ones are addressed and which ones are still open.

These are all valid propositions. But giving the reigns of resolving PR comments in the hands of PR authors is just too risky, and thus not recommended.

Who Should Approve the PR Comments?

Well, as a convention, 

• It’s no harm to always let the authority lie with the PR reviewer. 

• PR reviewers are folks who have years of experience & thorough understanding of the code base. 

• PR reviewers have a more polished approach to coding, and could better make decisions around design patterns for application scalability and maintainability.

• By allowing PR authors to self-approve the PR comments, the opportunity for them to learn the nuances of software architecture and coding best practices is lost or delayed.

• When satisfied with the solution, PR reviewers can mark their comments as resolved, aka approved.

• If they are not satisfied with the solution, they can mark PR review comments as unresolved and may drop a descriptive reason explaining the decision.

• This approach to PR comment approval not only helps improve the code quality & security, and software performance metrics but also saves teams from accruing unnecessary tech debt.

• It’s a good idea to involve & delegate the code review responsibility to the next domain expert in case of a rift between the PR author & PR reviewer.

• Under no circumstances, a code should be allowed to move to CI/CD pipelines without necessary checks.

Should the PR Author Approve the PR Comment?

Ideally, NO.

But there could be exceptions (though not recommended). When the changes are trivial, PR authors might be allowed to mark the comments as completed. When the code commits only involve fixing a typo, removing commented methods, lazy classes or refactoring code smells such that it does not change anything in terms of functionality, quality, and performance; the PR author may self-resolve and request the repository maintainer to trigger the CI/CD pipeline.

How to Make Your Code Review Process Flawless?

Engineering analytics tool that gives you clear visibility into your engineering work helps you to kill the pain out of your code review process. Hatica enables you to track key code review metrics to understand bottlenecks in code review cycle, and improve collaboration:

With Hatica's review collaboration dashboard, developers can easily collaborate on code reviews, track review progress, and identify areas for improvement.

With a single pane view of actionable, and data-driven metrics, engineering teams can recognize engineering work, and improve the overall code review process:

Unreviewed PRs Merged

It’s a big red flag and signifies inefficient code review and software development process. If the number of unreviewed PRs merged is high, it means the teams are not following the SDLC best practices. Possibly, the code review best practices are being overlooked. The merged PRs may also introduce bugs in production.

Issues without PR

Ideally, each issue created in an issue tracking tool (Jira) should be allocated to a PR request. When it’s not the case, it becomes tough to track such issues and introduces unnecessary complexity in the SDLC process. 

PRs Created, PRs Merged, PRs Reviewed

These are complementary metrics to each other. The difference between the PRs created and the PRs reviewed reflects the efficacy of code review practices. If the difference is huge, it means that either the reviewer developers are unavailable, or the process is broken. Similarly, the difference between PRs reviewed and PRs merged reflects both, the quality of code and the engineering efficiency. If a high number of PRs are getting reviewed but not merged, it means the CI/CD pipeline process needs improvement, or the tests are failing, and the code review is not done thoroughly.

Review Time

It’s the time duration for which a PR sits in the review stage. High review time could be an alarm for engineering managers as it denotes an inefficient code review process. Maybe, senior developers are unavailable for the review or the PR author is violating PR best practices. 

Reviewer Reaction Time

It’s a measure of how fast a reviewer responds to the follow-up comments by the PR author. High reviewer reaction time could be a bottleneck as it breaks the flow state of the PR author.

Rework Time 

It’s the delta of time elapsed between the first review and when PR gets merged. High rework time could be a signal that the quality of the code is not good, the requirements are not well-detailed, or a high reviewer reaction time. Low rework time could indicate a streamlined SDLC process where developers are writing high-quality code. 

Subscribe to the Hatica blog today to read more about unblocking developers, and boosting productivity with engineering analytics.