# 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](https://github.com/features/code-review/), [Gitlab](https://about.gitlab.com/stages-devops-lifecycle/code-review/), [BitBucket](https://bitbucket.org/product/features/code-review), [Gerrit](https://gerrit-documentation.storage.googleapis.com/Documentation/3.4.1/user-review-ui.html), Phabricator's [Differential](https://www.phacility.com/phabricator/differential/), and [Azure Devops](https://docs.microsoft.com/en-us/azure/devops/repos/git/pull-requests?view=azure-devops). This list is agnostic to the choice of the underlying SCM (e.g. [git](https://en.wikipedia.org/wiki/Git), [mercurial](https://en.wikipedia.org/wiki/Mercurial), [perforce](https://en.wikipedia.org/wiki/Perforce), etc.), so I refer to diffs, PRs, and CLs interchangeably.[^1] ## 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](https://about.sourcegraph.com/) or [CodeSearch](https://cs.opensource.google/). - [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](https://en.wikipedia.org/wiki/Infrastructure_as_code) or [Config as Data](https://cloud.google.com/blog/products/containers-kubernetes/understanding-configuration-as-data-in-kubernetes) -- "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](https://lobste.rs/s/hfye67/features_excellent_code_review_tools#c_rsxum1) comment by [@kevincox](https://lobste.rs/u/kevincox). [^1]: As a tangent, I realized when writing this post that there isn't a great SCM-agnostic term for "unit of code change". "Pull Request" is git-flavored, "changelist" is perforce-flavored, and "diff" or "commit" are overloaded with other meaning.