The other side of authoring effective merge and pull requests is reviewing the change.

Note that these are guidelines and not hard rules and it’s an important skill to know when to go bend or break rules.

Communicate level of importance

I find that Conventional Comments is a good framework to communicate intention to the author. I’ve experienced making a comment that was an offhand “if this is easy make the change” and have the author spend too much time addressing my idle thought. Similarly I’ve had comments ignored that are of great importance. Communicating how important (or how not important) a comment is gets us on the same page.

You are on the same team as the author

This is more relevant when you are reviewing code within your company. In an open source project the maintainers need to perform some level of gatekeeping and enforce quality standards. Within a company the quality bar still needs to be met but don’t enter a code review with an adversarial attitude (I’m going to find all the mistakes). Work together to ship the best possible solution within a reasonable time window.

Take ownership

This doesn’t mean you should push the author away and take over. You should take ownership and responsibility for shipping something good. Don’t just do comments like “this could be a security concern, you should check with the auth team” instead take action “this could be a security concern, @auth-member-name can you take a look please?”.

Consider maintenance and technical debt

Does the code conform to the patterns in the code base? If not, is there a good reason to buck the trend? Are we at a point where patterns in the codebase should be refactored?

Can we extend or reuse this code in the future? A good smell test for how easy the code is to consume is seeing how easy it is to write tests for it.

Consider product and UX

(Again) does the code conform to patterns in the code base? It’s confusing for users if the UX is different for similar features. Sometimes there are good reasons to go against the established UX patterns. If this is up for discussion it’s a good idea to @tag the designer or product manager directly. Hopefully you are working in place where different disciplines are welcome in the code review.

What is most likely to break?

Asking yourself (and the author) what’s most likely to break when the code is shipped can give guidance on how risky the code diff (or the area we are touching) is. A good follow-up question is “how will we recover?”

Don’t go too far down this rabbit hole. Keep the concerns reasonable and focus on likely scenarios.