The review comment I regret

Early on as a lead I left a comment that said, in full: "why not just use a map here?"

Six words, no malice in them, and I remember the reply arriving forty minutes later with a rewritten function and a slightly anxious "is this better?". The change was fine either way. What I had actually communicated, without meaning to, was that a senior person had looked at their work and found it wanting, and they'd spent forty minutes on something that didn't matter to get back to neutral.

The comment wasn't wrong. It was unlabelled. The reviewer knows whether they're blocking a merge, suggesting an improvement, or thinking out loud. The author has no idea, and in the absence of a signal they will assume the strongest reading. That asymmetry is most of what makes code review feel worse than it needs to.

So now I label. Three words at the front of the comment, and the convention costs nothing:

Blocking means I think this is a defect or a real risk and I'd like it addressed before merge. Suggestion means I'd have done it differently, take it or leave it, and I genuinely mean leave it. Musing means I'm thinking about something adjacent and you should feel free to ignore me entirely.

The proportions are instructive. When I started tracking my own comments for a month, blocking ones were under a fifth. The rest was preference dressed as feedback, and before I was labelling them, all of it read as blocking to the person on the other end.

Review the change, not the version you'd have written

The habit I've had to unlearn hardest is reading a pull request as a diff against the implementation in my head.

There are usually several reasonable ways to solve a problem. If the author picked one and it works and it's maintainable, the fact that I'd have picked another is not a finding. It's a preference, and stating it as anything else costs you credibility for the times when you do have something real.

The question I try to hold instead is whether this change solves the problem it claims to. Which means the first thing I read is not the code, it's the ticket, and if I can't tell from the pull request what problem it's solving I ask that before I look at a single line. A surprising number of review rounds are two people disagreeing about the requirement while appearing to disagree about a function.

The things I do treat as blocking are narrow. Correctness. Anything touching money, permissions or tenancy. Anything that will be expensive to change later, meaning data models and public interfaces, because those calcify while implementations don't. A missing test on a path that has broken before. That list is short deliberately.

Size is a review problem before it's an engineering problem

A four-hundred-line pull request gets a real review. A two-thousand-line one gets a rubber stamp with three comments about naming, and everybody involved knows it.

I don't have a hard limit because hard limits get gamed, but when something arrives that large the useful move is usually to ask for the review to be split by concern rather than by file. Often the author already knows which parts are risky and which are mechanical, and they'll tell you if you ask. "Which bit do you want me to look at hardest" is the highest-yield question I know for a big change.

The related thing is timing. A review that arrives two days after the pull request is not a review, it's an obstacle. I'd rather give a shallower review within a few hours than a thorough one on Thursday for something opened on Tuesday, because by Thursday the author has moved on, rebuilt context to answer me, and learnt that opening a pull request means waiting.

Approve with comments, more than you think

Blocking a merge over things that aren't blocking is the most common failure I see in teams that take review seriously. It comes from a good instinct and it trains people to batch up changes, avoid review, and treat the process as a gate to be passed rather than help to be sought.

Approving with unresolved suggestions requires trusting that the author will use their judgement, which is uncomfortable the first few times and then becomes the normal state of a team that's working. If I can't trust someone's judgement on a suggestion-level comment, that's a conversation to have directly, not something to enforce through the merge button.

Not everything should be a pull request

The reviews that go worst are the ones where the disagreement is architectural and it surfaces after the code is written. By then the author has spent three days on it and every comment costs them work, so the discussion is no longer about the best design, it's about sunk cost with both parties pretending otherwise.

The fix is upstream and it's cheap: fifteen minutes and a diagram before anything is built, for anything that touches more than one module. I don't always remember to ask for it. Every time I've skipped it on something significant, I've paid for it in a review thread that ran to forty comments and left somebody demoralised.

For junior developers I've also started reviewing in person more, sitting together rather than commenting. It's slower and it doesn't scale, and it teaches about five times as much per round, because the useful part isn't the finding, it's the reasoning that produced it. Written comments transmit conclusions. They're very bad at transmitting how you got there.

I did apologise for the map comment, eventually, and probably made it weirder in the process. But the labelling convention came out of it, and the team uses it now, including on my pull requests. Being told "blocking: this breaks on an empty array" by someone I hired is the most direct evidence I have that any of this worked.