Code review: blocking vs non-blocking
Published
In an office, a code review is half written and half spoken. Someone leans over and says "this loop is a bit odd, but whatever, ship it", and you hear from their voice that it doesn't matter. On a remote team all you get is the text: "Why is this a loop?" Read at 11pm, alone, that sounds like a problem you have to fix before you can merge.
Every review comment is one of two kinds. A blocking comment means "this must change before it merges": a bug, a security hole, a broken contract. A non-blocking comment means "I'd consider this, but it's your call": style, naming, a small idea. The skill is telling them apart, and then saying which one it is, because the reader can't hear your tone.
A teammate's pull request works, has tests, and fixes a real bug. You would have structured the code differently, with a class instead of three functions. Do you approve it? What do you write?
Say what kind of comment it is#
The simplest habit that fixes most remote review pain is a label at the start of each comment.
Many teams use a format called Conventional Comments, which puts a word like issue,
suggestion, question, nit or praise first, and (blocking) or (non-blocking) after it.
Google's public code review guide recommends the same idea with a plain Nit: prefix for things
that are worth mentioning but not worth holding a merge for.
Here are six comments from a review of a refund worker. Label each one before you post it.
“If two requests for the same order arrive together, both pass this check and the customer is charged twice.”
“Why would you use a for loop here??”
“What happens to this job if the worker restarts halfway through?”
“I think this whole approach is wrong. We should be using events here, not direct calls. Also the retries, the naming, and the table layout...”
“Rename `data` to `pendingRefunds`.”
“This logs the full card number on line 88.”
What decides a blocking comment is the cost of it reaching production, not how strongly you feel. These block:
- Correctness: it does the wrong thing, or the right thing only some of the time (races, retries, time zones, money rounding).
- Security and privacy: secrets in code, missing permission checks, personal data in logs.
- Contracts: a breaking change to an API, event or schema that other teams depend on.
- Things you can't undo cheaply: a migration that locks a big table, a deletion with no backup.
Almost everything else is non-blocking: names, structure you would have done differently, a slightly faster way, a missing comment.
Google publishes its engineering practices guide for code review. Its core rule is that a reviewer should approve a change once it definitely improves the overall health of the code, even if it isn't perfect. "Better than before" is the bar, not "how I would have written it". The same guide asks reviewers to respond within one business day at most, because a slow review blocks a person, not just a change.
Make it easy to review#
Half of good code review is the author's job. Two things help most.
Keep it small. Google's guide on small changes says about 100 lines is usually a reasonable size, and 1,000 lines is usually too large. A study SmartBear ran on code review at Cisco found that reviewers caught fewer defects per line once a review went past about 400 lines, and when they went faster than about 500 lines an hour. A 2,000 line pull request does not get reviewed. It gets skimmed and approved, which is worse than no review because everyone thinks it was checked.
Write the description for someone in another time zone. They will read it when you are asleep, with no chance to ask you anything until tomorrow. A description that works answers:
- Why: the problem, with a link to the ticket or design doc.
- What: the approach in two or three sentences, including anything surprising.
- How to check it: what you tested, and how the reviewer can try it.
- Where to look: "the interesting part is
RefundWorker.retry, the rest is wiring."
## Why
Failed refunds are retried by hand today (about 40 a week, ticket PAY-812).
## What
A worker picks up refunds stuck in FAILED for 10+ minutes and retries them
with the original idempotency key, so a retry can never refund twice.
Gives up after 5 attempts and alerts #payments-oncall.
## How I checked it
Unit tests for the retry and give-up paths. Ran it against staging with
20 forced failures: 20 refunded once, 0 duplicates.
## Where to look
RefundWorker.retry and the migration. The rest is config.That description gets a useful review in one pass. "Fixed refunds" gets "what does this do?", and a day is gone.
Review speed is a team habit#
On a co-located team a slow reviewer costs an afternoon. Across time zones it costs a day per round, and a pull request that needs three rounds takes most of a week. A few habits keep it moving:
- Review before you start new work in the morning, so whoever finished last night isn't waiting all day.
- Approve with comments when the remaining points are non-blocking. The author fixes them and merges without waiting for you again.
- Batch your comments. Read the whole change, then post everything at once. Five comments dripping in over an afternoon means five notifications and five context switches.
- Move big disagreements to a call. If you think the whole approach is wrong, that is a conversation, not twenty comments. Then write the outcome in the pull request.
| Choice | What you gain | What you pay | Pick it when |
|---|---|---|---|
| Blocking comment | Stops a real bug, security hole or breaking change from shipping. | Costs at least one more round trip, which can be a full day across time zones. | Correctness, security, broken contracts, and changes that are expensive to undo. |
| Non-blocking comment (nit, suggestion) | Shares knowledge and taste without slowing the merge. | Easy to ignore, so important points get lost if they are mislabelled as nits. | Style, naming, structure and ideas for later. Label it so it reads that way. |
| Take it to a call | Settles a design disagreement in 20 minutes instead of days of threads. | Needs a shared hour, and the outcome must be written down or it is lost. | When you disagree with the approach, or a thread has gone back and forth three times. |
In the interview#
Remote interviews sometimes include a review exercise: here is a pull request, review it. They watch what you block on and how you word it.
Checkpoint#
1. Under Google's published standard, when should a reviewer approve?
2. Which of these should block a merge?
3. A teammate opens a 2,000 line pull request. What is the most useful reply?
- Blocking: correctness, security, broken contracts, hard to undo. Everything else is a nit.
- Label every comment. In writing, nobody can hear that you meant it lightly.
- About 100 lines is a good change, 1,000 is too big (Google's guide). Past 400 lines, reviews find less.
- Respond within one business day. Across time zones, every round can cost a day.
- Approve with comments when only nits are left.
Every review comment is either blocking, meaning it must change before merging, or non-blocking, meaning it is the author's call. I block on correctness, security, broken contracts and changes that are hard to undo, and I label everything else as a nit or a suggestion, because in a written review nobody can hear my tone. I approve once a change clearly makes the code better, even if it is not how I would have written it. As an author I keep changes small, around a hundred lines where possible, and write a description that says why, what, how I checked it and where to look, so a reviewer in another time zone can review it in one pass. I review within a day, batch my comments, approve with comments when only nits remain, and take design disagreements to a short call, then write the outcome down.