Code review

Code review is where a team’s engineering culture is either built or quietly lost. Most of the damage comes from how people review, not whether they do.

What I do as author

Open the PR when I start, not when I finish. Draft PR, WIP label, CI running from day one. When it’s ready for eyes, I remove WIP. This gives the reviewer context on the direction before the diff is enormous, and it stops the “I’ve been heads-down for two weeks and now you have four hours to review 3,000 lines” pattern.

Self-review before requesting review. I read every line of my own diff in the GitHub UI. Half the comments I would have gotten, I catch. The other half I’ve already annotated with “I know, I couldn’t figure out a cleaner way — thoughts?” That turns a nitpick round into a conversation.

Wait for CI to be green before pinging a reviewer. Their time is not for catching what a linter would have caught.

What I do as reviewer

Understand before suggesting. If I don’t know why the code was written that way, I ask. “Why this shape?” is a better first comment than “I’d do this differently.” Nine times out of ten the author has a reason I didn’t know.

No comments on the person. “This is confusing” not “you’re confusing.” “This function does too much” not “you did too much here.” Same feedback, different half-life on the team’s trust.

No hyperbole. “This is broken” when I mean “this will fail on empty input.” “This is terrible” when I mean “I’d prefer X.” Precise language de-escalates; dramatic language starts a defense.

I’m a reviewer, not a gatekeeper. My job is to catch things the author missed, not to prove I’m senior. If the code is good and I have no substantive feedback, I approve. I don’t invent things to comment on.

What a good comment actually looks like

Aphorisms about review are cheap; the craft is in the comment itself. A synthesized example, close in shape to comments I’ve left on subscription-cancellation PRs:

The cancel() method sets status = 'cancelled' and returns. Two things I want to check before this lands:

1. What happens to the pending renewal charge that’s already queued in the background job? I read through billing/queue.ts and I don’t see it being pulled — I think we’ll bill someone we just cancelled.
2. Calling cancel() on an already-cancelled subscription looks unhandled. Idempotency will save us most of the time, but the audit-log entry will fire twice.

Neither is a blocker for merging. I’d want to see them handled in a follow-up, or a comment naming why we’re deferring.

Three things make that comment useful:

• Evidence. I read the queue code; I’m not speculating.
• Named risk. “We’ll bill someone we just cancelled” is a specific consequence, not “this might be a bug.”
• Path forward without prescription. The author has more context on the codebase than I do — I name what needs handling, not exactly how.

What I don’t do

No offline “let me quickly explain” over Slack. If the discussion belongs in the PR, it stays in the PR. Otherwise the next person reading the diff a year from now doesn’t see why the code is the way it is.

No “my code / your code” language. It’s the team’s code. Ownership belongs to the codebase, not the last person who touched a file.

The system, not the review, catches most bugs

If bugs are landing in prod that a reviewer should have caught, the answer isn’t “review harder.” Usually it’s a test that would have failed on the case, or a type that would have made the bug unrepresentable, or a CI check that would have run the missing linter. Code review catches judgment failures; automation catches the mechanical ones. Building both is the job. See principles for where this thinking comes from.