A pull request is a conversation and a gate at the same time. Whatever passes through it becomes someone's problem at two in the morning, which is why ten minutes of careful reading now beats two hours of untangling a bad merge later.

The trouble is that reviews drift. You open a 600-line diff, get absorbed in the logic around line 40, and never look at the migration file at the bottom. A checklist does not make review slower. It makes it repeatable, and it keeps the boring categories from disappearing behind the interesting ones.

Start With the Pull Request Itself

Before reading a line of code, check the shape of the change. The description should explain why this exists, not just what moved. If it says "fixes bug", ask which bug and how anyone would know it is fixed.

Scope comes next. A pull request that adds a feature, renames a dozen variables, and reformats a config file is three reviews wearing one coat. Ask for the formatting churn to be split out. Large diffs get shallow reviews; small ones get real ones.

Size and freshness

Somewhere around 200 to 400 lines is where reviewers stay sharp. Past that, expect to skim. Confirm the branch is current with the target branch, and that continuous integration is genuinely green rather than "green except for the flaky one".

Code Quality: The First Honest Read

Read the diff top to bottom once without commenting. You are checking whether you can follow it.

• Names. Variables and functions should say what they hold or do. If you cannot guess a variable's purpose from its name, the next reader cannot either.
• Function size. Anything that needs a scroll to read is usually doing several jobs. Look for the seam.
• Duplication. Three copies of the same validation rule is a bug waiting for the day someone fixes two of them.
• Dead weight. Commented-out blocks, unused imports, and leftover debug prints should not survive the merge.
• Edge cases. Empty lists, zero, negatives, missing keys, very long strings. Ask what happens in each.

Logic you cannot verify by reading

When the rule is subtle — date arithmetic, retries, permission checks — ask for a test rather than a longer explanation. Nobody remembers the explanation eighteen months from now.

Security Checks Worth Doing Every Time

You do not need to be a security specialist to catch the common failures. Most incidents start small.

• Trust boundaries. Anything from a user, a webhook, or a third-party API is untrusted. Confirm it is validated before use and encoded when it is rendered.
• Authorization. Authentication is not authorization. A new endpoint should check that this user may touch this record, not simply that somebody is signed in.
• Queries and paths. SQL, shell commands, and file paths assembled from user input remain the classic injection trio.
• Secrets. Scan for keys, tokens, connection strings, and stray environment files. A secret committed once has to be rotated, so raise it loudly rather than quietly.
• Logging. Passwords, tokens, and personal data do not belong in log lines or error responses.

Performance and Resource Use

You are not hunting micro-optimizations. You are hunting work that grows badly as data accumulates.

• A database query inside a loop, or a lazy-loaded relation touched once per item.
• Queries with no limit against a table that will not stay small.
• Reading an entire file or collection into memory when a stream would do.
• New indexes, or the absence of one on a column that will be filtered constantly.
• Caches added without an invalidation story.

Tests, Errors, and Documentation

Tests should fail for the right reason. A test that only asserts no exception was thrown tells you almost nothing. Check that new behaviour is covered and that at least one failure path is exercised.

Error handling deserves the same attention. Swallowed exceptions and generic "something went wrong" messages turn small production problems into long ones. Errors should be logged with enough context to reproduce the issue and surfaced to whoever can act on it.

Then documentation. Does the README still describe how to run this? Is there a migration step, a config flag, or an environment variable a teammate needs to know about? If the change alters behaviour someone depends on, say so in the description or the changelog.

The 15-Point Checklist

1. PR description explains the why, not just the what.
2. Diff is scoped, with no unrelated refactors or formatting noise.
3. Branch is current and CI is green.
4. Names describe intent; no temporary or placeholder identifiers.
5. Functions do one job and read in one sitting.
6. Duplicated logic is reused or extracted.
7. Edge cases are handled: empty, zero, negative, missing, very long.
8. Errors are caught, logged with context, and surfaced.
9. Untrusted input is validated and encoded.
10. Authorization is checked on new routes and queries.
11. No secrets, tokens, or debug output in the diff.
12. Dependency changes are justified and reviewed.
13. Queries are bounded, indexed, and never run inside loops.
14. Tests cover new behaviour and at least one failure path.
15. Docs, config, and migration notes are updated.

Make It Fit Your Team

Fifteen items is a lot for a two-line fix. Use judgement: a copy change needs item one and nothing else, while a new payment endpoint deserves the full pass. The checklist is a floor, not a ritual.

Two habits make it stick. Comment on the code, not the person — "this query runs once per item, can we batch it?" lands better than "this is wrong". And when you raise the same point in three separate reviews, move it out of the comment thread and into a linter rule, a pull request template, or the team's written conventions.

Review is the last cheap moment to catch something and the first good chance to teach. Keep the list close, keep the comments kind, and keep the merges boring.

Photo: RDNE Stock project / Pexels