Code Review: Spreading Knowledge, Not Guarding a Gate

From the field: 41 comments, 0 about architecture

That PR was 780 lines and carried three days of work in one piece. The reviewer was not a weak engineer — quite the opposite, they were the most careful person on the team. But when you open 780 lines, the brain stops reading and starts scanning. And when you scan, you write down whatever catches your eye: whitespace, variable names, a missing log line.

Architectural objections do not need attention. They need context. To say “this service should not call that one directly” you have to hold the relationship between two services in your head, and by line 400 of 780 that space is already full.

My mistake was this: instead of telling the author to split the PR, I told the reviewer to look more carefully. I was pushing on the wrong side. After the outage three months later we changed the rule: splitting is the author’s job. Attention is not the reviewer’s job.

The inverse relationship between size and value

When we grouped three months of pull requests by size, this is what came out:

The last row is the interesting one: PRs over 800 lines get fewer comments. At that point the reviewer gives up and writes “looks good”. So a big PR does not produce more objections. It silences them completely.

Splitting is not as hard as it looks, but the author has to plan it: first the schema change (its own release), then the new code behind a closed flag, then a small PR that opens the flag. Three reviews, all of them readable.

Comment labels: the cheapest improvement

This was the single change that improved our review culture most. Every comment starts with a label:

blocking:   This line keeps the payment amount as a float. We lose
            cents; it has to be decimal. Must change before merge.

suggestion: This loop could be a single query. Not required now,
            but if it causes trouble later, start here.

nit:        The variable could be "remaining_amount" instead of "x".
            Ignore this if you like, I am just saying it.

question:   Why is there no retry here? I do not know, I want to learn.
            (This label puts the reviewer on the learning side too.)

Two labels are especially strong. nit lets the reviewer say what they think while leaving the author free — without it, every small preference turns into a negotiation. question takes review out of one-way inspection; a junior asking a senior a question shows that the author is not the only one learning in that PR.

blocking has one rule: write in a single sentence why it blocks and what has to change. “This approach feels wrong to me” is not a blocking comment, it is a feeling. It belongs in a conversation, not in a PR.

Waiting time: the quiet quality killer

I did not see waiting time as a quality problem until we measured it. In a waiting PR three things break at once:

• The author loses context. Answering a comment that arrives two days later takes longer than writing the code did.
• The branch starts conflicting. A waiting branch is the one that conflicts most on merge, and conflict resolution produces code nobody reviewed.
• The author grows the PR. A bored person waiting adds one more thing, and the PR slides towards the last row of the table above.

Our rule is one sentence: review comes before your own sprint work. Open PRs are the first thing in the morning. It sounds inefficient — it is not: your 30 minutes unblock two days of someone else’s.

Who approves, and how many

Who reviews should also be decided by ownership, not by volunteering. Without defined file ownership, review always lands on the same two people and those two become the team’s bottleneck — the same boundary problem I described in the monorepo post.

The author’s responsibility

Half of review quality is decided before the PR is opened. We expect three things from the author:

1. Review your own PR first. Read the whole diff before opening it. Whatever you find, nobody else has to read — this single habit visibly lowers the comment count.
2. Write the “why” in the description. The code already says what you did. Write why you chose this path, which alternative you dropped, and what you deliberately left out.
3. Say where you want the reviewer to look. “The risky part is this function” keeps the review from spreading thin.

What to track

What did not work for me

• Putting up a review checklist. We wrote a ten-item list; two weeks later nobody looked at it. What worked was automating the list: formatting, linting and simple patterns moved into CI. People should only look at what only people can look at.
• A “everyone reviews everyone” rotation. It looked fair. In practice someone reviewing an area they did not know could only write formatting comments. Ownership plus deliberate pairing worked better: the person who needed to learn an area was added as the second reviewer there.
• Raising the approval count to two. We did it as a reflex after an outage. The bug rate did not drop; waiting time doubled. We then narrowed it to the list above.

Checklist

Conclusion

Nobody made a mistake on that 780-line PR. The author finished the work, the reviewer looked carefully and wrote 41 comments. The process ran, and three months later there was still an outage. Because what we wanted from review did not match the conditions we gave it: we wanted architectural objections, and we handed over a pile that could only be scanned.

If review is designed as a gate that decides whether code passes, it mostly catches formatting. If it is designed as a channel that spreads knowledge through the team, it catches architecture — and CI catches the formatting anyway.

Measure it with one question: in last month’s reviews, how many times did someone say “I did not know that, I learned something”? If the answer is zero, your review may be running, but it is not working.