Skip to content
Back to Quality Assurance

Review Etiquette

Review promptly, respond to feedback gracefully, and keep the review focused on the code, not the person.

Why

Code review is a social process as much as a technical one, and the etiquette determines whether it is a collaborative improvement or an adversarial battle. The most common etiquette failure is delay — the PR sits for 3 days because the reviewer is "busy," the author is blocked, the feature slips, and the team's velocity drops. The second is defensiveness — the reviewer leaves a comment, the author argues instead of considering it, and the review becomes a debate to win rather than a problem to solve. The third is personalization — "you always write this bug" makes the author defensive, while "this pattern has a bug" keeps the focus on the code. Good etiquette is not about being nice; it is about being effective — a review that is prompt, focused, and respectful produces better code faster, because the feedback is received and acted on rather than debated and delayed.

When

On every review interaction — when requesting a review, when reviewing, when responding to feedback, and when approving or requesting changes.

How

1. Review promptly (within 4-24 hours)
   - A PR that sits for 3 days blocks the author and slows the team
   - If you cannot review today, say so and suggest another reviewer
   - Small PRs (< 200 lines) should be reviewed the same day

2. Keep the PR small and focused
   - One PR per feature or fix, not a grab-bag of changes
   - Target < 400 lines — large PRs cannot be reviewed thoroughly
   - If the PR is large, ask the author to split it

3. Respond to feedback gracefully
   - Thank the reviewer for catching a bug — they just saved you from a production issue
   - If you disagree, explain why with evidence, not emotion
   - If the reviewer is wrong, educate kindly — "actually, this is safe because..."
   - Do not argue to win — argue to find the right answer

4. Keep comments focused on the code
   - Critique the code, not the author: "this function is too long" not "you write long functions"
   - Use "we" for suggestions: "we could extract this to a helper" not "you should extract this"
   - Avoid "always" and "never" — they sound accusatory and are usually wrong

5. Approve when the code is good enough
   - Do not block on nits — approve and leave nits as non-blocking comments
   - The perfect is the enemy of the good — a merged PR with a nit is better than a blocked PR
   - If the code is correct, secure, and tested, approve it

Example

// Requesting a review (author etiquette):
// PR description:
"""
## What
Add input validation to the user registration endpoint.

## Why
The current endpoint accepts any string as an email, which causes
database errors when the string is not a valid email.

## How
Use zod to validate the email format before inserting into the database.

## Testing
- Added unit tests for valid and invalid emails
- Tested manually with the API: POST /register with { email: "not-an-email" }

## Checklist
- [x] Tests added
- [x] No breaking changes
"""

// Reviewing promptly (reviewer etiquette):
// - Review within 4 hours for a small PR, 24 hours for a large one
// - If you cannot review today: "I can't review today, can you ask @teammate?"
// - Do not let the PR sit silently for 3 days

// Responding to feedback (author etiquette):
// Reviewer: "Line 18: what if the email is an empty string?"
// Bad response: "It won't be, the frontend validates it."
// Good response: "Good catch. I'll add a min length check:
//   z.string().email().min(1). I'll also add a test for the empty string."

// Reviewer: "Consider using a transaction here."
// Bad response: "It's fine, the operations are independent."
// Good response: "You're right, if the second insert fails, the first
//   is committed. I'll wrap both in a transaction."

// Keeping comments focused (reviewer etiquette):
// Bad: "You always forget to handle the error case."
// Good: "The error case is not handled here. What should happen
//   if the database is unavailable?"

// Bad: "This function is too long, you should split it."
// Good: "This function is 80 lines and handles three concerns
//   (validation, database, email). Could we split it into three
//   smaller functions for readability?"

Common Problems

  • ⚠Letting PRs sit for days because the reviewer is "busy" — the author is blocked, the feature slips, and the team learns that review is a bottleneck, so they stop opening PRs and accumulate unreviewed changes.
  • ⚠Arguing with every comment instead of considering the feedback — the reviewer spends 30 minutes defending their code, the review takes a week, and the team avoids reviewing that person's PRs.
  • ⚠Blocking the PR on a nit (variable naming, import order) — the author cannot merge a correct, tested, secure change because of a style preference, and the review process becomes a bottleneck.
  • ⚠Approving without actually reviewing — the reviewer clicks "approve" to be nice or to avoid conflict, the bug ships, and the review provides no value.

Best Practices

  • ✓Review within 24 hours (same day for small PRs) — a prompt review keeps the author unblocked and the team's velocity high, and it shows respect for the author's time.
  • ✓Keep PRs small (< 400 lines) and focused on one change — small PRs can be reviewed thoroughly in 15 minutes, large PRs get a superficial skim that misses bugs.
  • ✓Respond to feedback with thanks and evidence, not defensiveness — the reviewer caught a bug, thank them; if you disagree, explain why with a test or a reference, not an argument.
  • ✓Approve when the code is correct, secure, and tested, and leave nits as non-blocking comments — perfect is the enemy of good, and a merged PR with a nit is better than a blocked feature.