Features of Excellent Code Review Tools

Last week, I onboarded onto a new code review system. In learning how to review code in this new tool, I got to thinking about best features of previous code review systems I’ve used. What follows is an opinionated (and very incomplete) list of what I think are “table stakes” for code review in 2021.

For the purposes of this post, “code review tool” refers to a web UI for reviewing code – think Github, Gitlab, BitBucket, Gerrit, Phabricator’s Differential, and Azure Devops. This list is agnostic to the choice of the underlying SCM (e.g. git, mercurial, perforce, etc.), so I refer to diffs, PRs, and CLs interchangeably.¹

Requesting and Writing Reviews

Table Stakes:

• [As author] Ability to view diffs in a work-in-progress state before sending them out for review.
• [As reviewer] Ability to comment on specific lines.
• [As reviewer] Ability to mark a comment as “actionable” (or “unresolved”), distinguishable from a “no action needed” comment.
• [As reviewer] By default, just the diff is shown, but there are affordances for seeing more context of each changed file.
• Reviewers are able to signal whether or not they think the diff is mergeable.
• The system tracks “whose turn is it” to send comments.
  • Useful for reviewers/authors to know if a particular PR requires their action.
• The system allows you to draft comments (and responses!) before sending them to your reviewer/reviewee.

Nice to Have:

• [As reviewer] Ability to comment on a specific substring of a line.
• [As reviewer] Ability to make in-line suggestions.
  • [As author] Ability to accept in-line suggestions within the tool.
• [As reviewer] Ability to jump from files in the diff view into a full copy of the existing file.
  • Alternatively, strong integration with a separate “code search” system, such as SourceGraph or CodeSearch.
• [As reviewer] Ability to mark certain files as “reviewed” (this state is only visible to the reviewer) to make it easier to pause/resume reviews of larger PRs.
  • Bonus points if the tool automatically tracks which files you’ve viewed.
• [As author] Automated “Round robin” reviewer assignment within a team.
  • Why? “Round robin” reviewer assignment load balances code review responsibilities across a team, increases everyone’s exposure to the team’s code, and reduces some of the power differential in code review (since everyone reviews each other’s code).
  • This works best in small- to mid-sized teams of roughly equally leveled engineers. This approach may not work in larger teams, or teams that have a large variety of experience levels.
• [As author] The system can automatically find reviewers based on changed files.
• Customizable code display settings.
  • e.g. Preferred tab width, font selection, dark/light mode, etc.
• The system can enforce that a particular reviewer (or team) signs off on a diff based on file(s) changed.
  • This is especially important in monorepo projects, where each team only “owns” a subset of paths in the repo.
• Ability to customize a personal dashboard of active incoming/outgoing PRs.
  • e.g. Ability to create persistent filters based on author, filepath, PR status, etc.

Really Nice to Have:

• Reviewers can signal the degree of their confidence in the code (e.g. “rubber stamp” < “looks good to me” < “full approval”)
• [As author] Ability to view and respond to review comments inline within your IDE.
• Comments can be formatted (Markdown or WYSIWYG). Comments can contain inline/embedded code snippets (either from the existing checked-in code, or from the current diff).

Pull Requests (PRs) / Change Lists (CLs) / Diffs

Table Stakes:

• Ability to attach issues/bugs/tasks to diffs.
  • Implicitly, this means that the code review system needs to be somewhat aware of the system you use to keep track of issues.
• Code review history (including review comments) is saved somewhere, preferably indefinitely.
• Each PR/CL/Diff gets a unique identifier that can be referred to during review & after submission. (e.g. #123, cl/123)
• Ability to view the difference between versions of the same PR.
  • For example: Author sends out version 1. Reviewer makes comments on version 1. Author edits the PR, and sends out version 2. Reviewer can choose to see the diff between version 1 and version 2 OR the diff between the “base” and any of the versions.
• Ability to stack diffs/commits. In other words, a PR can use another PR as its “base”.
• Controls over who is able to submit changes.
  • i.e. Only authors can submit, only approvers can submit, some mixture of each. (Personally, I prefer that the author triggers submission, but it’s sometimes useful to allow non-authors to trigger a submit)

Nice to Have:

• Ability to submit diffs from within the code review tool.
• Ability to rollback / revert a submitted diff from within the tool.
• When a diff is submitted, the associated bug(s)/task(s) gets automatically updated with a note.
• PR descriptions can contain Markdown formatting, which is displayed correctly within the tool.
• The PR description is saved in the SCM’s history as the description of the diff/commit itself.
  • Note: This is not SCM agnostic, and does not apply cleanly to some Git workflows.

Really Nice to Have

• Automated sync/merging before in-tool submission.
  • i.e. Unless there are merge conflicts that cannot be automatically resolved, clicking “merge” should do the right thing.
• Opt-in setting to auto-merge the PR once it’s been sufficiently approved.
• Ability to track the rollout status of release(s)/build(s) associated with a submitted diff.
• [As reviewer] The PR description is itself reviewable (ditto individual commit descriptions, for Git workflow). Reviewers can comment on and suggest changes to the review.

Automation

Table Stakes:

• Configurable pre-merge/pre-submit CI workflows (e.g. tests, linters).
• Ability to prevent submission if criteria are not met (e.g. tests passed, sufficient number of reviewers signed off).

Nice to Have:

• CI workflows can be manually triggered from within the tool.
• Ability to customize when a given CI workflow is run (e.g. upon initial review request, upon each update, upon submission).
• Ability for CI tools to themselves comment on specific lines, offer inline suggestions, etc.

Really Nice to Have:

• A full history of all CI workflow runs against all versions of the PR.
• Ability for CI tools to act as reviewers.
  • e.g. A CI tool that could automatically approve certain relatively risk-free config changes.
• Customizable per-diff presubmits.
  • Example: “Prevent this submission of this diff until PR #123 is deployed in prod”)
• Customizable per-diff reviewer automations.
  • Example: “After $COWORKER approves this diff, add someone from $EXTERNAL_TEAM as a reviewer”
• Customizable post-submit notifications.
  • Example: “Send me an email when this merged diff gets deployed to staging/prod.”

After making this list, my primary takeaway is that code review tools benefit from being tightly integrated with other development tools. A code review tool in a silo can only do so much. A tool that is able to talk to the bug tracker, rollouts system, internal team database, etc. will be able to tick many more of the boxes under “Nice to have”.

As infrastructure configuration moves to version control – such as with Infrastructure as Code or Config as Data – “code review” begins to encompass more than just changes to logic running in released software: operations configuration and infrastructure management now go through “code review” as well. This further compounds benefits to a the “code review” tool being part of a larger system – one that is aware of release pipelines, operational state, and so on.

I’ve never worked with a single tool that has all of these features. Though, some of the $BIGCO internal solutions I’ve used have gotten close.

Update: Oct. 13, 2021: Added a few extra notes about PR descriptions, prompted by this Lobste.rs comment by @kevincox.