Code Review — Checklists, Heuristics, and PR Etiquette

What it is

Code review is the deliberate practice of one engineer reading another's proposed changes before they land on the main branch. The goals are layered: catch defects early (cheaper than catching them in production), spread knowledge of the codebase across the team, enforce conventions, and give junior engineers a fast feedback loop. Modern review is asynchronous and pull-request-centred — GitHub, GitLab, Gerrit, and Bitbucket all hang reviews off the same idiom: a diff, a description, line-level comments, and an approval gate.

A good review is a conversation about intent, not a syntax check. Linters and CI catch the mechanical things; the human reviewer asks "why this approach", "what happens if the network drops here", "is this naming going to make sense in six months", "is this the right place for this logic". The best review feedback is specific, kind, and grounded in the change in front of you — not a treatise on what the codebase should look like in some imagined future.

This article gives a checklist, the heuristics that drive it, and the PR-authoring practices that make a review fast and useful for both sides.

The reviewer's first-pass checklist

Before opening individual files, scan the PR overall. Three questions in 30 seconds:

1. Does the description tell me why? Title + 1-3 sentence summary + linked issue. If you cannot tell what problem this solves, ask before reading the diff.
2. Is the PR small enough to review? Under ~400 lines of meaningful diff is the sweet spot. Larger PRs deserve a "please split" comment, not a thorough review — fatigue makes large reviews bad reviews.
3. Are tests included? New behaviour without tests is a yellow flag; deleted or weakened tests without explanation is a red flag.

If any of these fail, comment and stop. A reviewer's most powerful tool is the question "can we discuss this first?"

The reviewer's deep-pass checklist

Once the meta is solid, walk through the diff with these in mind. Most defects fall into one of these buckets.

Correctness

• Does the code do what the PR description says? Do the tests prove it?
• Edge cases: empty input, single element, max int, negative, null/undefined, Unicode, very long strings, large numbers.
• Off-by-one errors in loops, slices, ranges.
• &&/|| precedence; == vs ===; integer vs float division.
• Error paths: what is returned/raised when the happy path fails?
• Concurrency: shared mutable state, race conditions, deadlocks.
• Are there hidden assumptions about ordering (dict iteration, file system order, test order)?

Control flow

• Nesting depth — over 3 levels is a smell; early returns help.
• Long functions — under ~50 lines per function; if longer, justify it.
• if/else chains that should be a dispatch table or polymorphism (see Open-Closed).
• Mutation through long chains makes diffs hard to reason about.

Naming

• Variable names describe the content, not the type (users, not user_list).
• Function names describe behaviour with a verb (calculate_total, not total_calculator).
• Boolean names start with is_, has_, should_, can_.
• No abbreviations without context (usr, cnt, tmp).
• No "magic numbers" — name constants.
• Consistency with the rest of the file/module.

Error handling

• Are exceptions caught at the right level? Catching too high loses context; too low duplicates handling.
• Naked except: (Python) or catch (e) { } (JS) — swallows errors silently.
• Are errors logged with enough context (request ID, user ID, input) to diagnose?
• Are partial failures handled? Network calls, file writes, DB transactions.
• Retries with backoff for transient failures, not blind retries.

Tests

• Is each new behaviour exercised by at least one test?
• Are tests fast (unit), isolated (no shared state), and readable (name describes behaviour)?
• Are negative paths tested, not just happy paths?
• Are mocks at the right boundary (see Testing Strategies)?
• Does the test verify the behaviour or just the implementation?

Security

• Inputs from outside the trust boundary (HTTP, files, env) — are they validated?
• SQL queries — parameterized, never string-interpolated.
• Shell commands — argv arrays, never bash -c "$user_input".
• Secrets — not in code, not in logs, not in error messages.
• Authentication / authorization — checked on every endpoint that needs it.
• Cryptography — using standard libraries, not hand-rolled.
• Dependencies — pinned versions, scanned for CVEs.

Performance

• O(n²) where O(n) would do (see Big-O).
• DB queries in a loop (N+1 query problem).
• Synchronous I/O on hot paths in async runtimes.
• Allocations inside tight loops.
• Caching opportunities — but only if profiling shows the cost matters.

Readability

• Code reads top-to-bottom like prose.
• The "happy path" is obvious; error and edge handling is bracketed.
• Comments explain why, not what (the code already shows what).
• Magic constants extracted and named.
• No dead code, no commented-out blocks.

Architectural fit

• Does the change belong in this module, or is it leaking into a layer that shouldn't know about it?
• Does it follow the codebase's established patterns?
• If it introduces a new pattern, is it deliberate and worth the cognitive cost?
• Does it create cyclic dependencies?

Read the diff twice. First pass: structure, intent, fit. Second pass: line by line, looking for bugs. Trying to do both at once produces shallow reviews.

Writing a reviewable PR

The author's job is to make the reviewer's life easy. A reviewable PR has these properties:

PR title conventions

fix: prevent crash when cart is empty
feat(billing): add Stripe webhook handler
refactor(auth): extract token issuance into AuthService
test(checkout): add edge cases for international currencies
docs: clarify rate-limit behaviour in README
chore: bump TypeScript to 5.5

Conventional Commits (type(scope): subject) is a common standard. The type acts as a coarse filter when scanning a branch's history.

A PR description template

## Summary

One sentence on *what* this change does and *why*.

## Context

(Optional) Background a reviewer needs — a linked issue, a design doc,
or a brief explanation of the problem.

## Changes

- Bullet 1 — concrete, file-level when useful
- Bullet 2
- Bullet 3

## Test plan

- [x] Added unit tests for X, Y, Z
- [x] Manually verified A in dev environment
- [ ] Smoke-tested staging deploy

## Screenshots / output (if UI or CLI)

![before](...)  ![after](...)

Comment etiquette — for reviewers

Reviewers shape the team's culture. The same finding can be educational or demoralizing depending on phrasing.

# Good
nit: this could be `Object.fromEntries(pairs)` and skip the loop, but
not a blocker.

q: what happens if `user` is null here? I see the early return three
lines up but want to make sure I'm tracing it correctly.

suggestion: I'd extract `validateCart` to a separate function — it's
become big enough that it has its own reason to change.

blocker: this catches all errors and swallows them silently. We need
at least a logger.warn() with the error, or we'll lose visibility.

praise: love this — the parametrize table makes the edge cases obvious.

# Bad
This is wrong.

The "bad" example is technically a comment; it is also a non-starter for a conversation. The good versions name the concern, suggest a path forward, and leave the author room to push back.

Five heuristics for reviewer comments

1. Comment on the code, not the coder. "This function" vs "you". "We" if it's a shared decision.
2. Be specific. "This is unclear" → "the cleanup flag in line 23 — is that for the temp file or the connection?"
3. Show, don't just tell. A code suggestion (GitHub's "Suggested change") is faster than a paragraph.
4. Distinguish blocker from nit. Don't leave the author guessing what's required.
5. Praise the things you'd want praised. Sustained criticism without acknowledgement burns reviewers out as fast as authors.

If you find yourself writing a paragraph in a review comment, consider whether it should be a synchronous conversation instead. PR threads are a poor medium for nuanced design debates — schedule 15 minutes and walk through it.

Comment etiquette — for authors

Receiving review feedback is also a skill. The team's effectiveness depends on it.

Five practices for authors

1. Read every comment carefully before reacting. Don't dismiss "nit" as automatically optional — sometimes the nit is the right call.
2. Push back, with reason. A reviewer's "I'd extract this" is a suggestion, not an order. If you disagree, explain why; if the reviewer is unconvinced, ask for a second opinion.
3. Resolve a comment when it's addressed. Either by changing the code or by writing a short reply explaining why you didn't. Don't leave threads dangling.
4. Don't take it personally. Reviewers comment on the code. The code is not you.
5. Re-request review explicitly after pushing changes. Some reviewers don't see the new commits otherwise.

Replying to feedback

# Good
> nit: this could be Object.fromEntries(pairs)

Good call — done in <commit>.

> blocker: this swallows errors silently

You're right; added a `logger.error(err, { ctx: 'parseCart' })` and a
test for the error path.

> suggestion: extract validateCart

I'd actually like to keep it inline here — it's only used in this
function and pulling it out adds an indirection. Happy to discuss
synchronously if you feel strongly.

GitHub-specific conventions

Most teams converge on a small set of GitHub features. Knowing them well saves time on every PR.

Useful gh commands

# Create a PR with a description from a heredoc
gh pr create --title "fix: cart crash on empty input" --body "$(cat <<'EOF'
## Summary
Empty carts threw at line 42; now they return 0.

## Test plan
- [x] Added unit test for empty cart
EOF
)"

Output:

https://github.com/example/repo/pull/4271

# List your open PRs
gh pr list --author "@me"

Output:

#4271  fix: cart crash on empty input             feat/cart-empty
#4258  refactor: extract AuthService              refactor/auth
#4221  docs: clarify rate-limit behaviour         docs/ratelimit

# Review a PR locally
gh pr checkout 4271
npm test
gh pr review 4271 --approve --body "LGTM — nice tests"

Output:

Branch feat/cart-empty set up to track 'origin/feat/cart-empty'.
Reviewed pull request OWNER/REPO#4271

# See all comments on a PR
gh pr view 4271 --comments

Output:

#4271 fix: cart crash on empty input
author: alicedev
state:  OPEN

— bobreviewer (suggestion) at line 42 of src/cart.ts
  Consider returning 0 explicitly rather than relying on reduce's
  initial accumulator — easier to grep for.
— bobreviewer (blocker) at line 67 of src/cart.ts
  This silently catches; please log.

# Get the raw inline comments via the API
gh api repos/example/repo/pulls/4271/comments

Output: (none — exits 0 on success)

# Merge with squash + a clean commit message
gh pr merge 4271 --squash --subject "fix: cart crash on empty input (#4271)"

Output:

✓ Squashed and merged pull request OWNER/REPO#4271

Draft PRs

gh pr create --draft --title "WIP: stripe integration"

Output: (none — exits 0 on success)

Draft PRs say "I want CI to run and feedback on direction, but don't approve yet". When ready:

gh pr ready 4271

Output:

✓ Pull request OWNER/REPO#4271 is marked as "ready for review"

Branch naming

feat/<short-desc>      — new feature
fix/<short-desc>       — bug fix
refactor/<short-desc>  — no behaviour change
docs/<short-desc>      — documentation
chore/<short-desc>     — dependency bumps, CI tweaks
hotfix/<short-desc>    — production fix

Some teams add a ticket prefix: JIRA-1234/fix-cart-crash. Pick a scheme and stick with it.

Diff hygiene

The diff is what the reviewer reads. Make it tell a clear story.

Commit structure

For most teams the merge strategy is squash, so individual commits inside a PR don't survive. They still matter while the PR is open — a clean history makes self-review easier.

Two viable styles:

Either way: no merge commits inside a feature branch. Rebase, don't merge from main. Merge commits in feature branches make the diff for the PR unreadable.

# Update your branch with the latest main
git fetch origin
git rebase origin/main
git push --force-with-lease

Output:

Successfully rebased and updated refs/heads/feat/cart-empty.

--force-with-lease is safer than --force. It refuses to push if someone else has pushed to your branch since you last fetched. Reach for it by default.

Whitespace and formatting diffs

A PR mixing functional changes with whitespace reflows is unreviewable — the reviewer cannot tell what changed. Three habits prevent this:

1. Run the formatter on commit. Pre-commit hooks (pre-commit, lint-staged) format only the staged files.
2. Land formatting changes in their own PR. "chore: reformat repo with prettier" is its own diff.
3. Configure your editor to format on save. Eliminates the "fixed an unrelated indent" problem.

Spotting common bug shapes

After enough reviews, certain shapes catch the eye. Learn the patterns; you will find more bugs faster.

Off-by-one

# Wrong — misses the last element
for i in range(len(xs) - 1):
    process(xs[i])

# Right
for x in xs:
    process(x)

The fix is usually "use the higher-level iteration construct" — for x in xs, arr.forEach, for ... of.

Mutation while iterating

# Wrong — modifying xs during iteration
for x in xs:
    if should_drop(x):
        xs.remove(x)   # skips the next element

# Right — comprehension
xs = [x for x in xs if not should_drop(x)]

Async forgotten

// Wrong — returns a promise no one awaits
function handler(req, res) {
  someAsyncCheck(req);
  res.send("ok");
}

// Right
async function handler(req, res) {
  await someAsyncCheck(req);
  res.send("ok");
}

A linter (@typescript-eslint/no-floating-promises) catches this; not every codebase has the rule on.

Try/catch swallowing

# Wrong — disappears all errors
try:
    do_work()
except:
    pass

# Right — be specific, log, re-raise if you can't handle
try:
    do_work()
except KnownError as e:
    logger.warning("expected error in do_work", exc_info=e)
    raise

Null/undefined in chains

// Wrong — crashes if user is undefined
const email = user.profile.email;

// Right — optional chaining + a default
const email = user?.profile?.email ?? "unknown";

N+1 queries

# Wrong — one query per order
for order in orders:
    customer = db.query(Customer).filter_by(id=order.customer_id).one()
    print(customer.name)

# Right — one bulk query, then a dict lookup
customer_ids = [o.customer_id for o in orders]
customers = {c.id: c for c in db.query(Customer).filter(Customer.id.in_(customer_ids))}
for order in orders:
    print(customers[order.customer_id].name)

Time and timezone bugs

# Wrong — local time, ambiguous
expires = datetime.now() + timedelta(hours=1)

# Right — timezone-aware UTC
from datetime import datetime, timedelta, timezone
expires = datetime.now(timezone.utc) + timedelta(hours=1)

Race conditions on shared state

// Wrong — two parallel awaits both read 0, both write 1
async function increment() {
  const cur = await store.get("counter");
  await store.set("counter", cur + 1);
}

// Right — atomic operation
await store.incr("counter");

Security-focused review

Security review is regular review with extra suspicion of inputs and an extra read for sinks. Five recurring categories:

1. Injection. SQL, command, LDAP, XPath, NoSQL. Parameterize. Never interpolate user input into a query string.
2. Auth and access control. Every endpoint is authenticated unless intentionally public. Authorization checks happen on the server, after authentication, with the resource's owner verified.
3. Cryptography. Use libraries (bcrypt, argon2, nacl, cryptography); never roll your own. No MD5, no SHA1 for new code.
4. Secrets management. No secrets in repos, env vars at minimum, secret-manager preferred. No secrets in logs, no secrets in error messages.
5. Dependency hygiene. Lockfile committed, audit tool (npm audit, pip-audit, cargo audit) in CI, supply-chain awareness (typosquatting, malicious updates).

# Check dependencies for known CVEs
npm audit
pip-audit

Output:

found 0 vulnerabilities

# Detect secrets in a diff
gitleaks detect --no-banner --staged

Output:

○ no leaks found

If a secret has been committed — even to a private repo, even briefly — it must be rotated. git filter-branch or git filter-repo can scrub history, but assume the secret is compromised.

Performance-focused review

Performance review is mostly about catching the same handful of mistakes. Reach for a profiler only when the structural review is clean.

A useful PR comment:

This loop calls `db.users.find({ id })` per order. For ~1000 orders this
is 1000 queries. Consider `db.users.find({ id: { $in: ids } })` once
and a map lookup.

Accessibility, i18n, and observability — quick passes

For user-facing code:

• Alt text on <img>, labels on form inputs, focus management on modals.
• All user-visible strings extracted to the translation file.
• Color contrast meets WCAG AA (4.5:1).

For all code that runs in production:

• Logs include enough context to diagnose without reproducing.
• New metrics for new failure modes.
• Tracing for new HTTP calls.

These are usually missed in the rush of feature work and are the bugs the team will hit at 3 AM.

Review velocity — making reviews fast

Long-lived PRs decay. The author forgets the context; the reviewer forgets the agreed scope; merge conflicts pile up. Two team-level habits keep PRs short-lived:

1. First response under one business day. Even just "I'll look at this tomorrow" — the author should not be guessing.
2. Approve when blockers are addressed, even if nits remain. Block "needs another round of review" on substance, not cosmetics.

For the author, the equivalent practice is work on something else while a PR is in review. Don't sit waiting; pick up the next ticket. Context-switching back when feedback lands is much cheaper than blocked engineering time.

When to escalate

Some PRs need more than one reviewer.

The author should pre-emptively request the right reviewer; the on-PR reviewer should add them when missing.

Common pitfalls

• Reviewing for syntax. Linters and formatters do this. Reviewers should focus on intent, design, edge cases, and behaviour.
• Bikeshedding on naming or formatting in a feature PR. If there's a convention to discuss, do it in a doc or in a dedicated PR.
• Approving without reading. "LGTM" with no comments on a 1,500-line PR is not a review.
• Rubber-stamping a teammate. Familiarity is no substitute for skepticism. Review the change, not the person.
• Demanding scope creep. "While you're here, can you also fix X?" — refuse, file a follow-up. PRs balloon when reviewers expand scope.
• Conflicting reviewers, no resolution. Two reviewers disagree, the author is stuck. The author should propose a resolution and ask the disagreeing reviewers to confirm; if still stuck, escalate.
• Comments without specifics. "This feels off" is not actionable. Always give the author a concrete fix or question.
• Force-push during review. Force-pushing rewrites the diff under the reviewer's feet. Push new commits instead until the PR is approved; squash on merge.
• Letting CI failures linger. Red CI is a strong "do not review yet" signal. Authors who push without checking CI burn reviewer time.
• Reviewing too late in the day. Tired reviews are shallow reviews. The hardest PRs deserve fresh eyes.
• Reviewer becomes co-author. Suggesting one or two lines is great; rewriting the PR in comments puts the author in a humiliating position. Pair-program instead.
• Author defensiveness. A review comment is not an accusation. Treat the change like a shared artifact, not a personal possession.
• Author silence. Disappearing for three days after feedback — reviewer momentum is lost. Reply same day, even if just to acknowledge.

Real-world recipes

A reusable PR template

Save as .github/PULL_REQUEST_TEMPLATE.md:

## Summary

<!-- One sentence: what changed and why. -->

## Linked issue

Fixes #

## Changes

-
-

## Test plan

- [ ] Unit tests added/updated
- [ ] Integration tests pass locally
- [ ] Manual verification:

## Screenshots / sample output (if applicable)

## Risk

<!-- low / medium / high — what could break, what's the rollback? -->

A repo CODEOWNERS file

# Save as .github/CODEOWNERS

# Default owners
*               @alicedev @bobreviewer

# Frontend
/src/web/       @frontend-team

# Database migrations need DBA review
/db/migrations/ @dbreviewer

# Anything in security/
/src/security/  @security-team

# Infra and CI
/.github/       @platform-team
/terraform/     @platform-team

A pre-commit hook setup

# .pre-commit-config.yaml
repos:
  - repo: https://github.com/pre-commit/pre-commit-hooks
    rev: v5.0.0
    hooks:
      - id: trailing-whitespace
      - id: end-of-file-fixer
      - id: check-yaml
      - id: check-merge-conflict

  - repo: https://github.com/astral-sh/ruff-pre-commit
    rev: v0.6.9
    hooks:
      - id: ruff
        args: [--fix]
      - id: ruff-format

  - repo: https://github.com/gitleaks/gitleaks
    rev: v8.20.0
    hooks:
      - id: gitleaks

pre-commit install

Output:

pre-commit installed at .git/hooks/pre-commit

Every commit now runs the formatters, lint, and secret-scan locally — the PR arrives at the reviewer pre-validated.

A PR self-review pass

Before requesting review, do a self-review:

gh pr create --draft
gh pr view --web

Output: (none — exits 0 on success)

Read your own diff as if it were a stranger's. Catch:

• Debug print / console.log left in.
• Commented-out blocks.
• TODOs without a ticket reference.
• Files included by accident (e.g. local .env, IDE configs).
• Logic the description doesn't mention.

gh pr ready

Output:

✓ Pull request OWNER/REPO#4271 is marked as "ready for review"

Splitting a too-big PR

A 1500-line PR comes back with "please split". The mechanical recipe:

# Save the current state
git checkout -b feat/big-change-original

# Start the first slice
git checkout main
git checkout -b feat/big-change-1-refactor

# Cherry-pick or check out just the refactor commits/files
git checkout feat/big-change-original -- src/refactor-target.ts
git add . && git commit -m "refactor: extract checkout helpers"
gh pr create --draft --title "refactor: extract checkout helpers"

# Repeat for the next logical slice

Output: (none — exits 0 on success)

For each slice, the test plan is "no behaviour change; tests still pass". The full feature lands across three or four mergeable PRs.

Reviewing a security-sensitive PR

A focused checklist for code that handles auth, payments, PII, or external input:

[ ] Input validation present at every external boundary
[ ] SQL/command/template inputs parameterized
[ ] AuthN check on every protected endpoint
[ ] AuthZ check verifies resource ownership
[ ] Secrets not in code, logs, or error messages
[ ] Rate limiting on expensive or sensitive endpoints
[ ] Cryptography uses a vetted library, current algorithms
[ ] Dependencies scanned for known CVEs
[ ] Logging captures the right events without leaking PII
[ ] Tests cover the negative paths (invalid inputs, missing auth)

A "good" review trace

A small PR review thread that does several things right:

PR: feat(cart): refuse adding zero-quantity items

bobreviewer:
  praise: nice — the test names read like a spec.

  q (line 14): is `qty <= 0` intentional? The schema lets qty be 0,
  which I think we should reject, but also `-1` — I'd want to test
  that case too.

  nit (line 42): `parseInt(qty, 10)` — small thing, but for readability
  could be `Number(qty)` and we'd avoid the radix arg.

  suggestion (line 67): would `class CartError extends Error` be worth it
  here? Not a blocker; just thinking about how errors are typed elsewhere
  in the codebase.

alicedev:
  > q: is qty <= 0 intentional?

  Yes — added a test for qty = -1 as well in <commit>.

  > nit: parseInt(qty, 10)

  Done.

  > suggestion: CartError class

  Thought about it; the rest of the codebase uses error codes on a single
  AppError class. Sticking with that pattern for consistency.

bobreviewer:
  Sounds good — LGTM!

Specific, kind, actionable, and resolved. A review like this — under half an hour total — produces a measurably better PR than a "looks fine" rubber-stamp.

Tips

The best review is the one that ships. Aim for "this change moves us forward" rather than "this change is perfect". Perfect is the enemy of merged.

Cross-link: git covers rebase/squash, gh covers the GitHub CLI, Testing Strategies covers what tests to look for, SOLID covers the design heuristics that drive structural review.

Reviews are a high-trust act. The author is asking the reviewer to spend attention; the reviewer is asking the author to consider feedback that may sting. Both sides should assume the other is acting in good faith — the team's velocity depends on that assumption holding.