A pull request sits for two days. Then someone leaves eight comments about naming, one comment that the migration is irreversible, and an approval that still feels like a warning. The author spends the afternoon renaming variables and still ships the dangerous part because it was buried. That is not a code review. That is a delay with extra feelings.
Code review best practices are not a personality test and they are not a chance to rewrite the diff in your favorite style. A useful review answers three questions: is this change safe, is it the change we meant, and can the next person operate it. If you cannot answer those, you are not done. If you have answered them and you are still arguing about tabs, you are wasting the only afternoon both of you had.
This is how to give a code review that helps the author ship the same day, without pretending every comment is equally important.
Read the ticket before you read the diff
Open the pull request description. Open the ticket. Open the screenshot if there is one. If the description is empty, that is the first review comment, and it is not a nit. You cannot judge a change you cannot name.
Ask what user-visible thing this is supposed to do. “Refactor auth” is not a thing. “Session cookie is HttpOnly and login no longer stores the JWT in localStorage” is a thing. If you cannot find that sentence, ask for it before you line-comment the helper rename.
If the PR is 1,800 lines, do not start in the middle of a CSS file. Start at the entry point: the route, the job, the migration, the public function. Trace one request. You are not reading a novel. You are walking a path.
Review for risk, not for your taste
Taste is real. Teams should have a linter and a formatter so taste dies in CI, not in a human thread. If Prettier would fix it, do not comment. If the linter would catch it, fix the linter. Humans should spend comments on things machines cannot see.
Risk looks like this: a migration that cannot roll back, a cache key that forgot the tenant, a permission check that moved from the server to the client, a retry that can double-charge, a feature flag that defaults on in production, an API shape the mobile app cannot ignore. Say the risk in the first sentence of the comment. “This will double-charge if the webhook retries” is a review. “I would have written this differently” is a diary entry.
If you are unsure, say you are unsure. “I think this races with the refund job; can you walk me through the lock?” is better than a fake confident block. Fake certainty is how reviews become fights.
Put comments in buckets the author can act on
Authors do not fail at GitHub. They fail at a wall of equal-looking comments. Separate them, in the summary, so the author can sequence the work:
- Blockers: must change before merge. Data loss, security, wrong behavior, missing tests for the money path.
- Should: real problems that can be a follow-up if you explicitly agree, with a ticket. Do not hide a should as a blocker to feel thorough.
- Nits: naming, extra lines, your preferred helper. Prefix them. “Nit:” is a contract that the author may ignore.
If everything is a blocker, nothing is. People will start merging through you. That is how you get the outage you were trying to prevent, plus a team that DMs around your reviews.
Write the summary first, even if you write it last. Three bullets. “Looks right for the coupon path. Blocker: the unique index is missing so two checkouts can create two discounts. Nits in the formatter range, ignore them if you want.” Now the author knows the job.
Ask questions that unpack the code
A good review question is a gift. “What happens if Stripe sends the webhook twice?” is a test the author can run in their head. “Why didn’t you use Redux” is a taste fight.
When something looks wrong, describe the scenario, not the author’s intelligence. “If the user has no default payment method, this page throws and they cannot leave” is specific. “This is messy” is not.
If you do not understand a function, that is data. Either the name is lying or you are missing context. Ask. Do not silently skip the billing module because it looks scary. The scary module is why the review exists.
Check the parts people skip
Everyone reads the happy path in the new component. Fewer people read the migration, the cron, the IAM policy, the feature flag default, the error message the user will see, and the test that was deleted because it was “flaky.” Read those.
Tests: you are not counting them. You are asking whether a test would fail if the bug you worry about were real. A snapshot of class names will not. A test that the refund is idempotent might.
Observability: if this can fail in production, will logs tell you which tenant and which request? If the answer is “we’ll know from the 500,” you do not know. See logging that helps at 2am if you need a bar. This is not extra credit. This is how you debug the thing you just approved.
Rollback: if this ships at 4 p.m. and is wrong, what do we do at 4:20? Revert the PR? Turn off a flag? Run a forward fix? If the answer is “hope,” that belongs in the review.
Timing: you are a dependency
A review that lands three days later is a different change. The author has context decay. The branch has conflicts. The product manager has already demoed it. Same-day review on a normal-sized PR is a professional courtesy, not a hero move.
If you cannot look today, say so and name when you can, or pass it. Silence is a block. “I’ll have this by 3” lets people plan. A PR with two reviewers and no SLA becomes an orphan.
Interrupt for production, money, and security. Do not interrupt a customer call for a typo in a README. Calibrate. The person who treats every ping as an incident trains people not to ping, including when it is an incident.
Approve with comments when the remaining work is nits
GitHub’s “approve” is not a character reference. It means “I would ship this with the remaining comments as optional.” If you leave blockers and also approve, you taught the author that blockers are decorative. If you request changes for nits, you taught them that you cannot be predicted.
Use request-changes for blockers. Use approve-with-comments for the rest. If you need a second look at the migration after they fix it, say “re-request me on the SQL, I don’t need to re-read the UI.” That sentence saves twenty minutes.
Do not surprise people in public
If the approach is wrong, do not leave a 40-comment tour of their failure on a PR ten people watch. Talk. Five minutes of voice can save a rewrite that a thread will make personal. Then write the conclusion on the PR so the record exists: “We agreed to split the migration; updating the description.”
Public channels are for decisions. They are not for dominance. If you are angry, wait. Angry reviews are how good engineers start hiding work in huge PRs so there is less surface area. You will then complain that PRs are too big. You built that.
Reviewing people more junior than you
Explain the why in one sentence. “We don’t put secrets in the frontend because anything in the bundle is public” teaches. “Wrong” does not. Link the doc if it exists. If it does not, the review comment can become the doc. That is cheaper than repeating yourself next month.
Do not take the keyboard in the comments. Suggest. If they are stuck, offer to pair. Pairing is for “I don’t know where the spine is.” Written review is for “here is the error, here is line 40.” Choose.
When they get it right, say that too. Not a paragraph of praise. One line: “The idempotency key on the refund is the right shape.” People repeat what you notice.
Reviewing people more senior than you
You are still allowed to ask about the irreversible migration. Seniority does not make a missing index safe. Pack the question. “I might be missing context: if we abort this deploy, can we undo the backfill?” is a review, not insubordination.
If they wave you off, you still wrote the question on the PR. That record is for the incident review, not for a gotcha. Most seniors would rather you asked.
What to do when you disagree
Disagree on the user outcome and the failure mode, not on identity. “I’m worried this API cannot version” can be resolved with a date, a header, or a new endpoint. “You always over-abstract” cannot.
Time-box the argument. Fifteen minutes. Then pick: author’s call, your call because of risk you can name, or a third person. Endless review threads are how teams ship around the process.
If you lose the argument, do not sabotage with a pile of nits. Approve or request the actual remaining blocker. Being right later is allowed. Being corrosive is expensive.
Keep the PR small enough to review
You cannot give a good review of a rewrite, a feature, and a dependency bump in one diff. If you are the reviewer, you can still say “this needs to split; I can review the migration today if you peel it out.” That is a code review best practice aimed at the author, and it is fair. Huge PRs get fake approvals. Fake approvals are how you get the Sunday rollback.
If you are reviewing your own habits as an author, write a PR that a tired person can finish in twenty minutes. Description, screenshot, test plan, rollback. The companion article on writing a pull request that gets reviewed is the other half of this job. Reviewers cannot rescue a 3,000-line surprise. See also git commits your teammates can actually use.
After it merges
If you learned something, put it where the next review will not need you: a lint rule, a README, a checklist in the PR template. “Please consider error handling” as a ritual comment is a process smell. Make the machine say it, or make the template ask for the rollback plan.
If the change was scary, watch it. Not forever. An hour of metrics after deploy is part of the review. Approving and vanishing is how you train authors not to trust you when they say “please look, this touches billing.”
A working bar
A good code review is specific, timely, and ordered by damage. It assumes the author is on the same side. It uses tools for taste and humans for risk. It leaves a summary a stranger could follow. It does not confuse thoroughness with volume.
You will miss things. The point is not a perfect reading. The point is that the dangerous parts got a second brain, the author can ship without guessing which comments matter, and the next person can operate the system. That is the whole job. Everything else is optional, and optional comments should look optional.
Artikals will keep treating review as part of shipping, not as a gate that exists to prove you were careful. Be careful where it counts. Then let people merge.
