As a reviewer you’re responsible for improving safety, enforcing broadly agreed upon standards and should use the opportunity to teach and learn from your colleagues.
Code reviews have a few goals.
The #1 priority in a code review is to provide a layer of safety to protect things from breaking for your users.
🚔 Enforcement of broadly agreed upon standards
In the cases where there are broadly agreed upon standards that aren’t enforced by your test infrastructure we may need to enforce standards in reviews. Reviewers should never enforce standards that aren’t broadly agreed upon.
🧠 Education and context sharing
Nobody comes to a company already knowing the stack. Code reviews provide a chance for us to learn from each other.
Authors are required to get an approval from any colleague in order to merge code, but that doesn’t give reviewers a license to block reviews for just any reason.
❌ No stylistic preferences
Linters enforce style guidelines. Reviewers shouldn’t push their personal preferences in a blocking code review.
❌ No blocking tips
Offering tips for how things could be improved is great, but reviewers shouldn’t block PRs on nits.
✅Block unsafe or non-standard code
Only safety issues and broadly agreed upon standards that aren’t caught by the linter should block PRs.
The most important goal of a code review is to provide a layer of safety. The top priority of a reviewer is to look for risks that may have been missed. There are many known types of risks that folks will be looking for and many unknown risks that you’ll need to use your discretion to navigate.
🔐 Security and privacy
Any PR that introduces (or appears to introduce) a security or privacy issue should be blocked until the issue is resolved.
Reviewers should be on the lookout for bugs where the product might not behave as intended by the author or could otherwise cause issues for users, integrity of our data, etc..
🧨 Traps for other engineers
If a method is called
renameUser but it actually deletes the user we could expect future engineers to get confused and will be at risk of introducing bugs.
🕸 System failures
It’s not always easy to spot changes that could trigger cascading failures or instability of our systems. Reviewers should be on the lookout for how unexpected circumstances might impact the broader network.