Code Review Essentials

Code review is where a change gets a second pair of eyes before it reaches production — catching bugs, spreading knowledge of the codebase, and keeping a shared standard of quality that no single person could enforce alone. Done well, it's a collaborative safety net; done poorly, it becomes a gate that slows teams down without actually catching much. The difference is almost entirely about what reviewers look for and how they communicate it.

What a Review Is For

A review has three jobs, roughly in priority order: catch correctness problems tests didn't (edge cases, race conditions, security holes), verify the change is a reasonable design fit for the codebase, and spread context so more than one person understands each part of the system. It is not primarily a style check — a linter and formatter should already have settled anything mechanical before a human even opens the diff.

• Correctness: does the code do what it claims to, including edge cases (empty input, concurrent access, network failure) the happy-path tests might miss?

• Design: does this fit the existing architecture, or does it quietly introduce a second way of doing something the codebase already does elsewhere?

• Tests: do the tests actually exercise the new behavior and its edge cases, or just the happy path the author already knew worked?

• Security & data: is user input validated, are secrets kept out of logs, is a migration reversible, could this leak one user's data to another?

• Readability: could someone unfamiliar with this change understand it a year from now without asking the author?

Blocking vs. Nit — What to Flag, and How Hard

Not every comment deserves the same weight. Conflating a genuine correctness bug with a preference about variable naming trains authors to either over-react to every comment or, worse, tune out real ones. Label the severity explicitly so the author knows what's actually blocking merge.

// A PR adds this to a payment webhook handler:
app.post('/webhooks/stripe', async (req, res) => {
  const event = req.body
  await processPayment(event)
  res.sendStatus(200)
})

// BLOCKING: the webhook signature is never verified -- anyone who finds
// this URL can POST a fake 'payment succeeded' event and get free product.
// "This needs to verify the Stripe signature header before trusting the
// payload -- see stripe.webhooks.constructEvent(). This is exploitable
// in production as written, so it has to be fixed before merge."

app.post('/webhooks/stripe', async (req, res) => {
  const event = stripe.webhooks.constructEvent(
    req.body, req.headers['stripe-signature'], WEBHOOK_SECRET
  )
  await processPayment(event)
  res.sendStatus(200)
})

// NIT: non-blocking, author's call whether to address now
// "nit: `event` shadows the outer `event` import a few lines up --
// might rename to `stripeEvent` for clarity, but not blocking."

A useful mental split: BLOCKING for anything that's a bug, a security hole, or a real design problem that will bite the team later — the PR shouldn't merge until it's addressed. NIT (or a similar marker) for genuine preferences the author is free to take or leave. Reserve QUESTION for when you're unsure and want context before deciding severity, rather than asserting something as wrong when you might be missing information.

Giving Constructive Feedback

The goal of a comment is to get a better outcome for the code, not to prove the reviewer is right or the author is wrong. Comment on the code, not the person; explain the reasoning, not just the verdict; and phrase it as a collaborative suggestion rather than a command whenever the issue isn't a hard blocker.

// A PR does this: fetches a list, then loops calling await inside a for-of,
// making N sequential network round-trips where they could run in parallel.

for (const id of userIds) {
  const user = await fetchUser(id)   // one request at a time
  results.push(user)
}

// Weak feedback -- vague, no reasoning, sounds like a command:
// "This is slow. Fix it."

// Better feedback -- explains the why, offers a concrete alternative,
// leaves room for the author to have context the reviewer doesn't:
// "This awaits sequentially inside the loop, so N users means N round-trips
// in series. If these fetches are independent, Promise.all(userIds.map(fetchUser))
// would run them concurrently -- worth it here, or is there a rate limit on
// fetchUser that requires the sequential calls?"

Ask questions when you're genuinely unsure rather than asserting a fix you haven't verified fits — "why did you choose X over Y?" often surfaces a constraint you didn't know about, and it invites a conversation instead of a defensive reaction. Praise good decisions in the diff too; a review that's 100% criticism reads as adversarial even when every individual comment is fair.

Review Etiquette & Process

How a review is run matters as much as what's said in it. Fast turnaround keeps a team's flow moving — a PR sitting unreviewed for two days blocks the author from picking up their next task cleanly. Small PRs get reviewed faster and more thoroughly than large ones, because a reviewer's attention degrades sharply past a few hundred lines of diff.

• As an author: keep PRs small and focused on one change; write a description that explains the why, not just the what; call out anything you're unsure about yourself.

• As a reviewer: aim to give a first pass within a day; read the description and understand the intent before diving into line-by-line comments.

• Approve with comments when only nits remain — don't force a re-review cycle for changes you're not actually blocking on.

• If a disagreement goes back and forth more than two or three times in comments, move it to a call — text loses tone and drags out what a five-minute conversation would resolve.

Common Anti-Patterns

A few review habits reliably make the process worse for everyone, regardless of good intentions.

• Rubber-stamping: approving without actually reading the diff defeats the entire point of review and lets bugs through that a real look would've caught.

• Scope creep: turning a focused PR review into a design debate about an unrelated part of the system — file it as a follow-up instead of blocking the current change on it.

• Bikeshedding: spending the review's attention budget on trivial, easily-reversible choices (naming, formatting) while missing the actual correctness or security issue in the same diff.

• Silent disapproval: leaving a PR sitting with no response instead of an explicit "changes requested" or "approved" — ambiguity blocks the author more than a clear no would.

• Reviewing the person instead of the code: "you always do this" or "why would you write it this way" reads as an attack; "this pattern doesn't handle X" reads as feedback.