Review Comments

Write review comments that are specific, kind, and actionable, so the author can fix the issue without confusion.

Why

Code review comments are communication, and communication style determines whether the feedback is received or resented. A comment like "this is wrong" is technically correct but demoralizing — the author feels attacked, becomes defensive, and the next review is adversarial. A comment like "this could be a bug, what happens if userId is null?" is specific (points to the exact issue), kind (asks a question, not an accusation), and actionable (the author knows what to check and fix). Good review comments also teach — "consider using a Map here for O(1) lookups instead of array.find" shares knowledge without condescension. The goal of review is better code, not proving the reviewer is smarter. Comments that are vague ("this is bad"), personal ("you always do this"), or unactionable ("fix this" with no explanation) waste time and damage the team's culture. A team that reviews kindly produces better code faster, because the feedback is received and acted on rather than debated.

When

On every review comment, as the team's shared standard for how to communicate feedback.

How

1. Be specific: point to the exact line and the exact issue
   - Bad: "This is wrong."
   - Good: "Line 42: if userId is null, this throws. Add a null check: if (!userId) return null;"
   - Quote the code, reference the line, name the issue

2. Be kind: assume the author is smart and did their best
   - Bad: "Did you even test this?"
   - Good: "What happens if the list is empty? I don't see a test for that case."
   - Ask questions, do not make accusations
   - Critique the code, not the person

3. Be actionable: tell the author what to do
   - Bad: "This is inefficient."
   - Good: "This is O(n²) because of the nested loop. Consider using a Set for O(n): const seen = new Set();"
   - Provide the fix, or a clear direction for the fix

4. Explain the why, not just the what
   - Bad: "Use a Map here."
   - Good: "Use a Map here for O(1) lookups — the array.find is O(n) and this runs in a hot loop."
   - The author learns the reason and can apply it next time

5. Distinguish blocking from non-blocking
   - Blocking: "This has a SQL injection vulnerability — must fix before merge."
   - Non-blocking: "Consider extracting this to a helper for reuse. (nit)"
   - Use labels: [blocking], [nit], [question], [suggestion]

Example

// Bad review comments (vague, unkind, unactionable):
// "This is wrong."
// "Did you even test this?"
// "This is inefficient."
// "Use a Map here."
// "Fix this."

// Good review comments (specific, kind, actionable):

// Correctness:
"Line 42: if userId is null, user.name throws a TypeError.
What happens if the session expires before the fetch completes?
Add a null check:
  if (!user) return null;
Or throw a more descriptive error:
  if (!user) throw new NotFoundError('User not found');"

// Security:
"Line 18: the userId comes from req.query and is passed directly to the
SQL query. This is a SQL injection vulnerability — an attacker could pass
userId=1; DROP TABLE users. Use a parameterized query:
  db.query('SELECT * FROM users WHERE id = $1', [userId])
And validate the input:
  const userId = z.string().uuid().parse(req.query.userId);"

// Performance:
"Line 30: this is O(n²) because findUser is called inside the map,
and findUser does a .find() over all users. For 1000 items, that's
1,000,000 comparisons. Consider building a Map once:
  const userMap = new Map(users.map(u => [u.id, u]));
  const results = items.map(item => userMap.get(item.userId));
This is O(n) and will be 1000x faster for large lists."

// Readability (nit):
"[nit] Line 50: the variable name 'd' is unclear. Consider 'daysUntilExpiry'
so the reader knows what it represents without tracing the logic."

// Question:
"[question] Line 60: why is the timeout 30s here? The default is 5s.
Is there a reason this endpoint needs a longer timeout? If so, could
we add a comment explaining it?"

Common Problems

⚠Leaving a comment like "this is wrong" without explaining what is wrong or how to fix it — the author must guess, asks for clarification, and the review takes an extra day.
⚠Mixing personal criticism with code feedback — "you always forget to handle null" makes the author defensive, and the valid technical point is lost in the emotional response.
⚠Marking every minor style preference as blocking — the author cannot merge until they rename a variable from d to daysUntilExpiry, and the review becomes a bottleneck rather than a quality gate.
⚠Leaving 50 comments on a 100-line PR — the author is overwhelmed, addresses the easy ones, and the important security comment is lost in the noise.

Best Practices

✓Be specific (quote the line, name the issue), kind (assume the author did their best), and actionable (provide the fix or a clear direction) — this is the minimum for every review comment.
✓Distinguish blocking issues (security, correctness) from non-blocking (style, nits) with labels, so the author can merge once the blocking issues are fixed and address nits separately.
✓Explain the why, not just the what — "use a Map for O(1) lookups" teaches the author, "use a Map" is an order that does not transfer to the next situation.
✓Prioritize the comments — lead with the blocking issues, then the important suggestions, then the nits, so the author addresses the most important feedback first and does not get lost in the noise.