What review is for, what to look at first, and the lifecycle of a PR Intermediate

This page answers: what review really delivers, where to look first, and which steps a PR goes through before merging.

What review is for

A study at Microsoft (Bacchelli & Bird, ICSE 2013): finding bugs is the main motivation for review, but the actual outcome is “less about defects than expected”. Review delivers more in knowledge sharing, awareness of changes, and alternative solutions.

Purpose	Concrete example
Stop expensive bugs	A race condition when decrementing stock, IDOR, a migration that locks a table for 20 minutes
Keep the design healthy	Business logic leaking into the controller, an abstraction gaining a fifth flag
Share knowledge	At least two people understand the payment module; juniors learn how seniors think
Consistency	New code follows the same error, logging and naming conventions as the rest

What review is not for

• Arguing about style. Quotes, import order, line length: put them in Prettier/ESLint and let CI fail. Google: “On matters of style, the style guide is the absolute authority.”
• Replacing tests. A reviewer can’t run every branch in their head. If new logic has no tests, the first comment is asking for tests.
• Making the author do it your way. If their way is correct, readable and follows conventions, your preferring another way is no reason to block.

Priority order: look at the expensive parts first

Google suggests reading a PR in three steps: the big picture (should this PR exist?), the main part, then the rest. If you see a major design problem, send the comment immediately; don’t wait to finish reading — the author is still building on that design.

Priority	Question	Typical severity
1. Correctness and design	Does it solve the right problem? Is there a simpler way? Is it in the right layer? Over-engineering?	Blocking
2. Security and data integrity	Validation at the edge? Queries scoped to the user? Correct transaction boundaries? Backward-compatible migration?	Blocking
3. Tests	Does the new logic have tests? Do the tests fail when the code is wrong?	Usually blocking
4. Readability	Do names say what they mean? Does each function do one thing? Do comments explain why?	Usually non-blocking
5. Style	Has a machine checked it? If not, add a rule to the linter	nit: or no comment

The “look here first” table

Don’t keep checklists in your head. Each kind of change already has its own checklist:

PR touches	Look here first	Checklist
BEGIN/COMMIT, balance deductions, stock, FOR UPDATE	Transaction boundaries, network calls inside transactions, rowCount	Transactions in code
Controllers, DTOs, SQL queries, uploads, logs	Validation at the edge, parameterized queries, secrets and logs	Secure coding
Endpoints by id (/orders/:id)	Is it filtered by the current user?	BOLA/IDOR
Migrations that change/drop columns	Can old and new code run side by side?	Expand-contract
New table, column, endpoint, event names	Casing, units, already-public contracts	Naming
Refactors, service logic	Refactor separated from behavior change, are errors swallowed	Clean code
Commit history, force pushes	Atomic commits, messages, secrets in the diff	Git
New tests with retries, sleep	Is the test stable?	Flaky tests
Loops calling the DB, findMany()/SELECT without LIMIT, async functions	N+1, unbounded queries, missing await	Common mistakes

The lifecycle of a PR

Loading diagram…

Three points worth noting:

• A draft PR on GitHub can’t be merged and doesn’t auto-request code owners. Use it to run CI and self-review before bothering anyone.
• CI runs before humans. Reviewers shouldn’t spend time on a PR that’s red.
• Each round of fixes goes back through CI. Approving and then pushing more logic commits that nobody reviews is a process hole; configure branch protection to dismiss stale approvals on new commits if the team needs it.

Next: PR author.