Mistakes I see engineers making in their code reviews

Original Article ↗ Hacker News Discussion ↗

Blocking vs non‑blocking reviews and what “approval” means

  • Major debate around whether non‑blocking comments can be safely ignored.
  • One camp: if you approve without blocking, you’re explicitly saying “OK to merge as is”; expecting changes anyway is confusing and unfair.
  • Opposing camp: any comment pointing out a potential problem should normally be treated as de facto blocking unless clearly marked optional; ignoring such comments is seen as careless or arrogant.
  • Some teams avoid “changes requested” and rely on comments + eventual approval; others treat a comment‑only review as “I don’t approve yet.”
  • Concern that early blocks serialize work and slow delivery, especially when reviewers are unavailable.

What counts as a blocking issue

  • Strict view: there’s no such thing as a “non‑blocking issue”; if it matters, block.
  • More pragmatic view: spectrum from serious bugs to “janky but acceptable for now,” tech debt, or minor polish; blocking on everything would stall progress.
  • Suggested practice: block only for things that will break existing behavior, fail to deliver the stated feature, or clearly violate agreed standards; treat the rest as suggestions or future work (tracked via TODOs/tickets).

Professionalism, responsibility, and culture

  • Disagreement over whether failing to block on a suspected bug is reviewer negligence.
  • Some see ignoring non‑blocking comments as acceptable disagreement; others see it as refusing to engage and “not doing your job.”
  • Frustration with toxic patterns: floods of nitpicks, PRs used as weapons, passive blocking by never approving.
  • Several people stress that authors remain responsible for their code even after review; review reduces accidents, not willful bad decisions.

Number and nature of comments

  • Article’s “don’t leave too many comments” is contested. Some think 100+ comments is never productive; others say big or off‑standard PRs may justify many, but prefer pairing or a call in those cases.
  • Broad agreement that style and formatting comments should be automated by linters/formatters and CI, not human reviewers.

Consistency vs personal taste

  • Widely accepted that reviews shouldn’t enforce arbitrary personal preferences.
  • Strong counterpoint: consistency and predictability in a mature codebase (naming, patterns, architecture) are crucial, even if rooted in someone’s “taste,” and are legitimate review concerns when tied to documented conventions.