Most teams don’t have a code-review problem; they have a PR-size problem, a commit-message problem, and a culture-of-silence problem wearing a code-review trench coat. Fix the inputs and review gets cheap. Leave them alone and no amount of process saves you.

This piece is the opinionated, no-ceremony version: the handful of habits that make review genuinely useful to the reviewer, the author, and the unlucky engineer reading git blame two years from now.

TL;DR

Code is read far more than it is written — review is where you pay (or refuse) the interest on that read cost.

Use Conventional Commits; let the subject say what and the body say why.

Keep PRs around 200 lines, with a soft ceiling near 400 — split with stacked PRs or a prep refactor.

Set a review-response SLO measured in hours; make bumping culturally safe.

Prefix review comments with nit:, question:, suggestion:, or blocking: to remove ambiguity.

Use a short PR template with a Risk section, and let CI handle lint, types, secrets, and commit-lint.

Review the code, not the coder — and never let a formatter-fixable nit eat human review time.

Why This Topic Earns Its Keep

Code is read far more often than it is written. Every change a team ships is written once and then read — in review, in git blame, in incident post-mortems, in onboarding, in refactors — for as long as the code lives. Most of the cost of a codebase is in those reads.

Code review is where you pay the interest on that read cost, or refuse to. A good review pipeline turns today’s work into readable history. A bad one turns it into an archaeology dig.

This post is opinionated and grounded. It is not about ceremony. It is about the handful of habits that make review genuinely useful — to the reviewer, to the author, and to the person who has to fix the production bug two years from now.

The Three Failure Modes of PR Review

Most teams that are unhappy with review are stuck in one of three shapes. Each has a telltale symptom and a known fix.

Too big. PRs land with 1,200 lines of diff across 40 files. Reviewers skim, approve, and hope. The symptom is rubber-stamp LGTMs and a test suite that sometimes catches a regression and often doesn’t. The fix is not stronger willpower from reviewers — it is a PR-size ceiling the team agrees to and enforces.

Too slow. PRs sit in review for days. Authors context-switch, rebase, re-push, re-explain. The symptom is stale branches, merge conflicts, and morale slowly bleeding out. The fix is a review-response SLO measured in hours, not days, and the cultural permission to bump.

Too lenient. Reviews consist of “nice!” and three rocket emojis. Defects leak. The symptom is a rising escaped-bug count and a growing lore of “don’t touch that file.” The fix is not harsher reviewers; it is an agreed checklist of things every review must actually check, written down.

The rest of this article is what to put in each of those slots.

Commit Hygiene

A commit is a unit of explanation. If a teammate runs git log --oneline and cannot tell what happened, the commit has failed — regardless of whether the code works.

Conventional Commits

Conventional Commits is a simple grammar for the subject line:

<type>(<scope>): <short summary>

<body>

<footer>

The types that pay rent:

feat — a new user-visible feature
fix — a bug fix
refactor — behaviour unchanged, structure improved
chore — tooling, dependencies, boring housekeeping
docs — documentation only
test — adding or fixing tests only
perf — performance improvement without behaviour change

You will occasionally see build, ci, style, revert. Use them if your team finds them useful. Don’t argue about them.

The scope is optional and names the area of the code: feat(auth): ..., fix(billing): .... Scopes pay for themselves the moment you have more than one person touching the repo, because they make git log --grep and release notes actually navigable.

Subject vs Body: Where the “Why” Lives

The subject answers what. The body answers why. A commit whose body says “fixes the bug” is a commit with no body. You already said that in the subject.

The body is where you write down:

Why this change instead of the three other ways you considered
What you tried first that didn’t work
Any non-obvious constraint the reader should know (e.g., “this column must stay nullable until the backfill in #482 completes”)
A link to the ticket, incident, or design doc

Six months from now, the body is what rescues you from yourself.

The Atomic-Commit Rule

One logical change per commit. If you have to write “and” in the subject line, you have two commits.

A refactor that enables a feature is two commits: the refactor first (behaviour identical, tests pass), then the feature on top (tests change). Reviewers can then read the refactor as a pure rearrangement and the feature as pure new behaviour. Both are easier to reason about alone than smashed together.

This also means each commit is safely revertable. If the feature breaks production, git revert the feature commit — not the refactor underneath it.

When to Squash, and When Not To

Squash when the intermediate commits are noise: “WIP”, “fix typo”, “respond to review”, “revert last thing”. Those are artefacts of working, not of thinking. The merged history should read like the thinking, not like the working.

Do not squash when the intermediate commits each tell a real story — the refactor-then-feature pattern above, or a multi-step migration where each step is independently meaningful. Squashing those destroys information that future readers need.

Rule of thumb: if you can write a clean, single Conventional-Commit subject that describes the whole PR, squash. If you can’t, don’t.

Good Commit, Bad Commit

A bad commit:

fixed stuff

A good commit:

fix(billing): treat 402 from Stripe as retryable, not fatal

We were bubbling 402 "card declined on idempotency retry" up as a
payment failure, which double-charged ~3% of customers who clicked
"pay" twice within the 5s window. Stripe returns 402 with a
`previous_response` field in that case and the original charge is
the source of truth.

This change inspects `previous_response` and short-circuits to the
original receipt when present. Added a contract test against
Stripe's test-mode idempotency replay.

Refs: INC-2214

The bad version tells the next reader nothing. The good version tells them: what changed, why, what it prevents, and how to verify it. That second commit is worth re-reading; the first is worth deleting.

PR Size

Aim for around 200 lines of diff as a target, with a soft ceiling around 400. These numbers are approximate — a 600-line diff that is 90% generated code, or a pure file move, is fine. A 200-line diff touching ten subsystems is not.

Why this range? Because review quality drops off a cliff once the diff exceeds what a reviewer can hold in their head. Past a few hundred lines, comments get sparser, issues get missed, and the review becomes an act of trust rather than examination. This is observable in any team that graphs review-comment density against PR size.

When a change is genuinely large, you have two clean ways to split it:

Stacked PRs. Split the work into a sequence of PRs, each merged (or at least reviewed and approved) on top of the previous. Tools like Graphite or the built-in stacked-branch workflows in Git help; so does just branching off your own in-review branch and opening a second PR against it.

Preparation PR. Merge a no-op refactor or scaffold first — add the interface, introduce the new module, move the file — then open the feature PR against the refactored base. Reviewers can read the scaffold without feature noise, and the feature PR is small because most of its plumbing is already in.

The question to ask before opening a large PR is never “can the reviewer handle this?” It is “can this be split?” The answer is almost always yes.

Review Speed vs Depth

A review pipeline has two knobs: how fast reviews happen, and how thoroughly they examine the code. Most teams accidentally optimise one at the expense of the other.

A workable target for most teams: first response within the working day, merge decision within 24 hours for normal PRs. Not instant — reviewers also need to think — but not days. A PR that sits for three days costs the author far more than three days, because they’ve moved on, the branch has rotted, and returning to it is an expensive context switch.

Depth and speed are not opposed when PRs are small. The reason reviews are slow is almost always that PRs are big, so reviewers postpone them to “when I have time for a proper look.” Shrink the PRs and same-day review becomes routine.

Make the SLO explicit. Write it down. Give authors cultural permission to bump after a few hours of silence — not because reviewers are lazy, but because silence is ambiguous and ambiguity is expensive.

Review Comments That Help vs Harm

A good review comment is specific, actionable, and about the code. A bad one is vague, opinion-as-fact, or about the author.

Prefix your comments with intent. This is the single highest-leverage habit a team can adopt:

nit: — stylistic preference, take it or leave it
question: — I don’t understand; explain and I may have no further ask
suggestion: — concrete alternative, worth considering
blocking: — this must change before merge, and here is why

“nit:” is not a weak word. It’s a precise one. It tells the author “you can ignore this and I won’t argue.” Without it, every comment feels like a blocker, and reviews turn into trench warfare.

Some phrasings that help:

“Is there a reason this isn’t X? X would avoid the null case below.” — invites context, doesn’t assert.
“suggestion: extracting this to a helper would make the retry logic reusable in the webhook handler.” — concrete, with justification.
“blocking: this reads req.body.userId without a check and will 500 on malformed input. Either validate upstream or guard here.” — says what, says why, says how to unblock.

Phrasings that harm:

“Why did you do it this way?” — accusatory, no alternative offered.
“This is wrong.” — unfalsifiable, offers no path forward.
“I would never write it like this.” — about the author, not the code.

The underlying principle: review the code, not the coder. Assume the author had a reason for every choice, and ask about the ones you don’t understand before asserting they’re wrong. Half the time there is a good reason. The other half, the author discovers it themselves the moment they try to explain.

PR Templates That Actually Get Filled Out

Most PR templates are skipped because they ask for things the author can’t answer or doesn’t care about. A good template asks only for information the reviewer genuinely needs, and it asks in a way the author can answer in two minutes.

A template that holds up in practice:

## What

<One or two sentences. What does this change do from a user's
or caller's perspective?>

## Why

<Link to the ticket, incident, or design doc. If none, explain
the motivation in one paragraph.>

## How

<Notes on the approach — especially anything non-obvious, any
alternative you rejected, any follow-up you explicitly deferred.>

## Testing

- [ ] Unit tests added / updated
- [ ] Integration or e2e coverage considered
- [ ] Manually verified (describe briefly)

## Risk

<What could go wrong? What's the blast radius if this is buggy?
Is there a feature flag, a rollback plan, or a staged rollout?>

## Checklist

- [ ] PR is under ~400 lines of diff, or split with reason
- [ ] Commit messages follow Conventional Commits
- [ ] No secrets, credentials, or `.env` files in the diff
- [ ] Docs updated if behaviour changed

The Risk section is the one most templates omit and the one reviewers most want. It forces the author to think about failure modes before asking someone else to.

Keep the template short. Templates that ask for twelve sections get deleted on the first PR.

Pre-Merge Checks That Matter

CI gates should catch the problems humans are bad at catching, so humans can spend their review time on the problems they are good at catching.

The ones that earn their keep:

Lint. Style, obvious bugs, unused imports. Humans should never review these.
Type check. For typed languages, a clean type check is non-negotiable.
Unit and integration tests. Including the ones the PR added. Flaky tests must be fixed or quarantined, not tolerated.
Secret scanning. gitleaks, trufflehog, or equivalent. Credentials committed to Git are still the most common cause of incidents that could have been trivially prevented.
Dependency and SBOM diff. When package-lock.json or go.sum changes, surface what’s new. npm audit is noisy; a diff of the lockfile plus a known-vulns check is usually enough.
Commit-lint. Enforce the Conventional Commit format at CI time, not at review time.

Here’s a minimal GitHub Action that enforces Conventional Commits on every PR:

name: Commit Lint

on:
  pull_request:
    types: [opened, synchronize, edited]

jobs:
  commitlint:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v4
        with:
          fetch-depth: 0
      - uses: actions/setup-node@v4
        with:
          node-version: 20
      - run: npm install --no-save @commitlint/cli @commitlint/config-conventional
      - name: Lint commits in PR
        run: |
          npx commitlint \
            --from ${{ github.event.pull_request.base.sha }} \
            --to ${{ github.event.pull_request.head.sha }} \
            --config .commitlintrc.json

With a .commitlintrc.json of:

{ "extends": ["@commitlint/config-conventional"] }

For bot-driven review of PR metadata — size, missing description, missing tests — Danger.js is the usual tool. A single rule that enforces the PR-size norm:

// dangerfile.js
import { danger, warn, fail } from "danger";

const bigPR = 400;
const changed = danger.github.pr.additions + danger.github.pr.deletions;

if (changed > bigPR) {
  warn(
    `:elephant: This PR is ${changed} lines. Consider splitting it — ` +
    `reviews lose signal above ~${bigPR} lines. If this is unavoidable ` +
    `(generated code, file moves), note the reason in the description.`,
  );
}

const body = danger.github.pr.body ?? "";
if (!/##\s*Risk/i.test(body)) {
  fail("PR description is missing the **Risk** section.");
}

Danger shines when the rule is easy to state but tedious to check by eye. Don’t use it to police taste.

What Not to Do in Review

Some common reviewer behaviours actively destroy value. Catch yourself doing any of these:

Style nitpicks that a formatter should handle. If your team has Prettier, Black, gofmt, or equivalent, spaces-vs-tabs and trailing-comma debates are the formatter’s problem. Humans should never spend review time on them.

Architecture debates mid-PR. If someone has spent three days building a feature and you’re proposing a structurally different approach, that conversation should have happened before the code was written. Raise the concern, but don’t hold the PR hostage — file a follow-up ticket for the refactor. Next time, do the design review earlier.

Shaming. “You still haven’t learned that we prefer early returns?” “This is the third time I’ve told you.” That’s not review, that’s punishment. If someone keeps making the same mistake, the system needs to catch it — a lint rule, a pairing session, a documented convention — not a reviewer with a long memory.

Drive-by opinions. A reviewer who drops five comments and disappears for two days is worse than no reviewer. Commit to the review or hand it off.

The Rubber-Duck PR

Open a draft PR before you think you’re done. Push what you have, read your own diff as if you were the reviewer, and fix the obvious things first.

This does two things. One, you catch half the comments you would have gotten, before anyone else has to type them. Two, you get a honest sense of the PR’s shape — its size, its story, its seams — at a point where splitting it is still cheap.

The draft PR is also the right place to ask structural questions: “I’m thinking of putting this in services/billing — does that match where fees lives?” A one-line question on a draft is cheaper than a 400-line PR in the wrong module.

Mark it draft. Say “not ready for full review, want a sanity check on the approach.” Most teammates are happy to skim. The ones who aren’t will tell you, politely, to come back when it’s ready.

Closing Checklist

Before clicking Merge, the author and reviewer should be able to say yes to all of these:

Subject lines follow Conventional Commits. Bodies explain why for any non-trivial change.
PR is split at atomic boundaries — each commit, if reverted, leaves the repo in a working state.
Diff is under the team’s size ceiling, or the description explains why not.
Risks are named. Rollback or feature-flag plan exists if the blast radius is wide.
Tests cover the new behaviour and any bug being fixed. Flaky tests are fixed, not retried.
No secrets, credentials, or personal data in the diff.
The PR description is still accurate — if the implementation changed during review, the summary changed with it.
Every blocking: comment is resolved. Every nit: comment is addressed or acknowledged.

None of this is heroic. It’s just the small, repeatable hygiene that makes the next PR — yours or a teammate’s — a little cheaper to review. Compound that over a year and it’s the difference between a codebase people are happy to open and one they quietly avoid.

Code review isn’t a tax on shipping. It’s where a team decides, explicitly, what kind of codebase it wants to live in six months from now.

Code Review and Commit Hygiene That Actually Helps