Code review is the most impactful daily activity most engineers do not do well. A mediocre review catches a typo and approves in ten minutes. A good review catches a race condition, improves an API design, and teaches the author something they carry for years. The gap is not time — it is approach.
What changed in 2026
- AI review bots handle the mechanical layer. GitHub Copilot PR reviews, CodeRabbit, and similar tools now catch undefined variable references, obvious logic inversions, and style issues before a human ever opens the diff. This shifts the human reviewer's job toward intent, design, and domain correctness.
- Async review culture matured. Most teams have written norms around response time (typically 1 business day) and comment severity labels. Unstructured "looks good to me" approvals are increasingly a red flag in engineering culture.
- Smaller PRs became the norm. Stacked PRs and draft PRs are first-class on GitHub and GitLab; reviewing 50 lines six times beats reviewing 300 lines once.
The mindset before you open the diff
- Read the PR title and description first. Know what it is supposed to do before you judge whether it does it.
- Run it if you can. 10 minutes of clicking beats 30 minutes of static analysis for UI and API behavior changes.
- Assume good intent. The author made reasonable choices given what they knew. Your job is to improve the outcome, not demonstrate superiority.
Risk-based triage
Not all lines are equal. Allocate your attention by blast radius.
| Area |
Risk level |
What to check |
| Auth, permissions, sessions |
Critical |
Bypass conditions, privilege escalation, token handling |
| Data mutations (DB writes, deletes) |
High |
Transactions, rollback paths, missing validation |
| Concurrency, async flows |
High |
Race conditions, unhandled rejections, lock scope |
| External API calls |
Medium |
Error handling, retry logic, timeout |
| Business logic |
Medium |
Edge cases, off-by-one, missing conditions |
| UI, copy, styling |
Low |
Cosmetic; delegate to designer if available |
| Config / infra changes |
High |
Blast radius on prod; flag for a second reviewer |
The review checklist
Correctness
- Does the code do what the description says?
- Are all edge cases handled (empty input, null, zero, max values)?
- Are error paths explicit and tested?
Design
- Would you add this abstraction? Is it the right level?
- Will this code be easy to change in 6 months?
- Are names clear and consistent with the rest of the codebase?
Security
- Is user input validated and sanitised before use?
- Are secrets handled via env vars, not hardcoded?
- Are permissions checked before returning sensitive data?
Tests
- Do the tests actually exercise the logic, or do they just hit the happy path?
- Are failure cases tested?
- Would a test failure give a useful error message?
Observability
- Are new error paths logged?
- Are new slow operations instrumented?
Writing comments that help
Use a severity prefix so authors know how to respond:
blocking: This will cause a null pointer in production when user has no email.
nit: You could simplify this to a one-liner — but it is fine as-is.
question: I am not sure I understand why we need the retry here. Can you explain?
suggestion: Consider extracting this into a helper; we do similar logic in billing.
Good comments explain why, not just what:
# Bad
blocking: Rename this variable.
# Good
blocking: `data` is ambiguous here — it could be raw API response or parsed response.
Consider `rawUserResponse` or `parsedUser` depending on what it holds at this point.
How to give feedback on a sensitive change
- Cite a principle, not a preference. "I prefer X" loses to "X follows our error-handling convention from the ADR."
- Offer an alternative. Blocking comments without a suggestion force the author to guess.
- Ask questions before blocking. "I may be missing context — why is this fetched twice?" often reveals a good reason.
How to pick what to block on
- Block on correctness — the code will behave wrong in production.
- Block on security — the change introduces a vulnerability.
- Block on broken tests or missing coverage for critical paths.
- Suggest, do not block, on style unless you have a linter rule to enforce it.
- Never block on personal preference that has no objective tradeoff.
Common mistakes
Reviewing 500-line PRs. Studies consistently show review quality drops sharply past 300 lines. Ask for the PR to be split.
Leaving the PR in "commented" state. Either approve, request changes, or block. Limbo wastes everyone's time.
Nitpicking manually what a linter should catch. If you find yourself commenting on indentation or import order, add a linter rule and automate it away.
Approving without understanding. "I trust you" is not a review. It just adds your name to the liability without adding value.
What to skip
- Style comments without a linter — every style debate wastes time that a
.eslintrc or ruff.toml would end permanently.
- Re-architecting in the comments — if the whole approach is wrong, say so early and offer to pair, not in 40 inline comments.
- Reviewing tests last — read the tests first; they document the intended behavior better than the description.
FAQ
How long should a code review take?
A well-scoped PR (under 300 lines, single concern) should take 20–40 minutes for a thorough review. If it is taking longer, the PR is probably too large.
What if I disagree with the author after they push back?
Escalate to a third reviewer or an agreed-upon team decision record. Do not let it become a standoff.
Should I review code outside my expertise?
Partially. Review for readability, naming, and test coverage regardless of domain. Flag domain-specific concerns as questions and tag someone with context.
How do I handle a PR where everything is wrong?
Open with a direct, kind message: "This approach will be hard to maintain because X. I would suggest pairing to redesign before we go further." Block, do not enumerate 80 nits.
Where to go next