Code review is the highest-leverage quality practice most teams do — and also one of the most commonly done poorly. A good review catches a bug before it hits production, questions a design decision while the cost of change is low, and helps a less experienced developer grow. A bad review is bureaucratic delay that burns goodwill without adding value. The difference is entirely in the reviewer's approach.
What changed in 2026
- AI-assisted review is now standard. GitHub Copilot code review, Cursor, and similar tools provide automated comments before a human reviewer sees the PR. Human reviewers should focus on what AI misses: business logic, design tradeoffs, security intent, and team norms.
- PR size norms tightened. Teams that use trunk-based development with feature flags have PRs under 400 lines. Large PRs (>800 lines) are increasingly a team dysfunction signal, not a feature.
- Async-first communication made written review feedback more important. In a remote team, a thoughtfully written review comment is the main form of technical mentorship. The quality of your written feedback matters.
- Merge queues are common. GitHub merge queues serialize PRs; your review approval is now a gate on a queue, not just a signal. Slow reviews block the whole team.
Before you read the diff
- Read the PR title and description. Understand what the author is trying to accomplish. If the description is absent, comment asking for one before proceeding — you cannot review code without knowing its intent.
- Read the linked issue or ticket. Understand the requirements. Ask: does this PR solve the stated problem?
- Check the scope. Is this PR doing one thing? Multiple unrelated changes in one PR are a review smell; the author may need to split it.
- Note the test coverage. Before reading implementation, check if tests exist and if they cover the new behavior.
The review checklist
Correctness
□ Does the logic do what the description says?
□ Are edge cases handled (null, empty, overflow, concurrent access)?
□ Are errors handled and surfaced correctly?
□ Are tests testing behavior, not implementation?
Security
□ Is user input validated/sanitized before use?
□ Are there SQL injection, XSS, or path traversal risks?
□ Are secrets/credentials handled correctly (env vars, not hardcoded)?
□ Is authorization checked at the right layer?
□ Are new dependencies trustworthy and version-pinned?
Design
□ Is this the right abstraction? Could it be simpler?
□ Does this fit the existing architecture patterns?
□ Are public interfaces stable and well-named?
□ Is this change backward-compatible, or does it require a migration?
Readability
□ Are functions and variables named by intent?
□ Is complex logic commented?
□ Is the code level appropriate (not over-engineered, not under-engineered)?
Comment types and how to label them
| Label |
Meaning |
Author must address? |
| (no label) |
Blocking — must be resolved before merge |
Yes |
nit: |
Minor style/readability, non-blocking |
No |
optional: |
Suggestion for improvement, non-blocking |
No |
question: |
Asking for clarification, may become blocking |
Depends |
suggestion: |
Alternative approach worth considering |
No |
praise: |
Call out something done well |
— |
Using these consistently removes ambiguity. An author should never be unsure whether a comment blocks their merge.
Writing good comments
Weak comment:
This is confusing.
Strong comment:
calculate_fee is doing three things: validating the input, computing the base fee, and applying the discount. Consider splitting into validate_fee_input and compute_discounted_fee. This makes each function easier to unit-test independently.
A good comment:
- Explains why the current code is problematic
- Suggests a concrete alternative when possible
- Links to a relevant doc, pattern, or prior art if useful
- Is respectful of the author's effort
Security-focused review patterns
Certain patterns always deserve extra scrutiny:
# Any string interpolation into a query — is it parameterized?
query = f"SELECT * FROM users WHERE id = {user_id}" # ← flag this
# Any file path from user input — is it validated?
path = os.path.join(upload_dir, request.filename) # ← path traversal risk?
# Any new external dependency — is it necessary and trustworthy?
# Check: last release date, download count, maintainer activity
# Any secrets in code or logs
logger.info(f"Connecting with key {api_key}") # ← flag this
In 2026, supply chain attacks via compromised npm and PyPI packages are a real concern. New dependencies added in a PR deserve scrutiny: check the package on its registry, look at recent commits, and verify it is the official package (not a typosquat).
Reviewing tests
Tests are code; review them as carefully as the implementation.
- Does the test name describe what behavior is being verified?
- Does the test have one assertion focus (or a small, cohesive group)?
- Are edge cases tested, or only the happy path?
- Does the test break if the behavior changes? (A test that always passes is worthless.)
- Is the test isolated, or does it depend on external state?
How to handle disagreements
If you disagree with an author's approach:
- Ask a question first: "What was the reasoning for using a global variable here rather than passing it as a parameter?"
- If you still disagree after their explanation, escalate to a technical discussion (a short call or a shared design doc), not an extended comment thread.
- For opinion-based disagreements (naming, code structure), defer to the author if the approach is defensible. Use
optional: or suggestion: labels.
Common mistakes
Reviewing too fast. A 400-line PR reviewed in 5 minutes is not reviewed; it is skimmed. Allocate 15–20 minutes minimum for a substantive change.
Commenting on style that the linter handles. If ESLint or Prettier enforces it, do not comment on it. Invest in linter rules instead.
Blocking on personal preference. "I would have used a for-loop instead of reduce" is a preference, not a bug. Label it optional or say nothing.
Not acknowledging good work. A praise: comment that says "nice abstraction here" takes 5 seconds and builds a positive review culture.
What to skip
- Large batch reviews — if you are looking at a 1200-line PR and trying to review it in one pass, you will miss things. Ask the author to split it.
- Drive-by approvals — clicking "Approve" without reading creates a false safety net and damages trust in the review process.
- Commenting without reading the full context — never comment on line 42 without knowing what line 15 says.
FAQ
How long should a PR review take?
Allow roughly 1 minute per 10 lines of substantive code (not boilerplate or generated code). A 300-line PR should take ~30 minutes for a thorough review.
What if the PR is too large to review properly?
Say so: "This PR is too large for a thorough review. Can you split the refactoring from the feature addition? I can approve the refactor first." Most authors will comply if you explain the reasoning.
Should I review code in a domain I do not know well?
Yes, but adjust your focus. You may not catch algorithmic bugs in unfamiliar domain logic, but you can review structure, error handling, test quality, and naming.
How do I build a good review culture on my team?
Model the behavior you want. Write thoughtful, respectful comments. Acknowledge good work. Address review debt (slow turnaround) in retrospectives. Review guidelines in the team wiki help, but example is more powerful.
Where to go next