Writing review comments and handling disagreement Intermediate

Estimated reading time: 3 min read

Read first

Reviewers: respond fast, read the design first

This page answers: how to comment so the author immediately knows what the problem is and whether the fix is mandatory, and what to do when the two sides disagree.

Ask instead of command, and state the severity

Every comment must tell the author two things: what the problem is and whether the fix is mandatory. Google uses the prefixes Nit:, Optional:/Consider:, FYI:. Conventional Comments standardizes this into a syntax:

<label> [decorations]: <subject>

[discussion]

Label	Use when	Blocking by default?
`praise:`	Sincerely praising a good approach	No
`nitpick:`	Personal preference, trivial matters	No (non-blocking by nature)
`suggestion:`	Proposing an improvement, saying why it’s better	Depends on decoration
`issue:`	A specific problem; ideally paired with a suggestion	Usually yes
`todo:`	A small but necessary change	Yes
`question:`	A suspicion you’re not sure about; ask the author to clarify	Depends on decoration
`thought:`	An idea that came up while reading, no request	No
`chore:`	Process tasks before merging (run a job, update docs)	Yes
`note:`	Information the reader should know	No

The “Blocking by default?” column is this cluster’s interpretation. The spec only says explicitly that nitpick and thought are non-blocking “by nature” and note is always non-blocking; other labels should state it with a decoration. The spec also has three optional labels: typo, polish, quibble.

Decorations: (blocking), (non-blocking), (if-minor) — only fix if the change is small. Examples: suggestion (non-blocking): ..., issue (security, blocking): ....

A team doesn’t need all 9 labels. Minimal default: issue, question, suggestion, nit, praise, plus (blocking) when blocking.

Comments before and after

#	Before	After	Why it’s better
1	“This is wrong.”	`issue (blocking):` Two concurrent refund requests both read `refunded = 0` and both write — this can refund twice. Use `UPDATE ... SET refunded = refunded + $1 WHERE refunded + $1 <= amount` and check `rowCount`?	A concrete failure scenario and a direction for the fix
2	“Why did you do it this way?”	`question:` I see `findAll()` followed by filtering in the app. The `orders` table has ~8 million rows — is there a reason not to push the `WHERE` down into the query?	Asks to understand, brings numbers
3	“Rename the variable.”	`nitpick (non-blocking):` `timeout` → `timeoutMs` so call sites don’t have to guess the unit.	Has a reason, labeled non-blocking
4	“You forgot validation again.”	`issue (security, blocking):` `sortBy` from the query string goes straight into `ORDER BY`. Please run it through an allowlist map like the Identifier section in Secure coding.	About the code, not the person; links the convention
5	“Rewrite this with the Strategy pattern.”	`thought:` If a third refund method is added later, this `switch` may be worth extracting into a Strategy. With 2 branches now, keeping it is fine.	Doesn’t push over-engineering
6	“Missing tests.”	`todo:` Add tests for refunding exactly 100% and refunding more than collected (must return 422). These are the CHECK constraint’s boundaries.	Says which cases and why
7	(No comment on the good parts)	`praise:` Extracting `calculateRefund()` into a pure function lets the 6 rounding cases be tested without mocks — very clean.	Conventional Comments recommends at least one sincere praise
8	15 comments: “missing semicolon”, “use single quotes”	One comment: `chore:` The repo doesn’t run Prettier in CI yet; I’ll open a separate PR to add it. Not blocking this PR.	Fix the system instead of each line
9	Reviewer accepts “OK, clean it up in the next PR.”	`issue (blocking):` The new code adds a third flag to `processOrder`. “Later” cleanups usually never happen — please split it into two functions in this PR.	New complexity must be cleaned up before merging

Two short rules:

Comment on the code, not the person. “This function…”, not “You…”.
If the author explains in the thread, ask them to put it in the code. Google: “Explanations written only in the code review tool are not helpful to future code readers.” Clearer code is best; a code comment is the second option.

When you’re the reviewer getting pushback

Per Google:

Consider whether they’re right. The author often understands that code better than you.
If you still believe your request improves code health, explain again, with data or design principles. “Technical facts and data overrule opinions and personal preferences.”
“Clean it up in the next PR”: Google calls this “a common way for codebases to degenerate”. New complexity introduced by the PR is cleaned up before merging, unless it’s an emergency. For pre-existing issues around it: create a ticket assigned to the author, optionally with a TODO pointing to the ticket.
If you’re polite, people rarely get upset about being asked to fix things; they get upset about how the comment is written.

When you’re the author receiving comments

Don’t reply while annoyed. Ask: “What useful thing is the reviewer trying to say?”
If you don’t understand, ask instead of guessing and fixing in the wrong direction.
If you disagree, state the trade-off; don’t write “No, I’m not doing that”.
Wherever the reviewer got confused → make the code clearer right there; don’t just explain in the thread.

When to move to a call

Loading diagram…

Google recommends meeting in person or on a video call when consensus is hard, recording the outcome as a comment on the PR, and only then escalating (team discussion, tech lead, maintainer, engineering manager).

“Don’t let a CL sit around because the author and the reviewer can’t come to an agreement.”
The “2 rounds” threshold is this cluster’s convention, not Google’s; it prevents 40-comment threads.

Next: Anti-patterns and metrics.

#engineering #code-review #conventional-comments #teamwork