There are five or six of us, depending on the week, and we run what we build. No separate operations team, no on-call rotation staffed by people who never touched the code. That single fact has shaped how I review a pull request more than any article about review etiquette. The engineer who approves a migration on Thursday is the engineer reading Postgres logs about it on Saturday morning.
Most writing about code review assumes a larger company: a gate between the people who write code and the people who own the system, a queue, an approval quorum, a policy about how many eyes a change needs. On a team this size there is nobody standing behind that gate. Review has to earn its hours some other way.
What review is for when the team is five people
We ship and operate a lot of surface in Korea: a community platform, an English-first marketplace inside it, a retail consulting tool that runs in seven languages, and an application suite for university students. That is more surface area than we have people. Nobody gets to be the sole owner of a subsystem, because the week that person is on a plane, the subsystem becomes unmaintainable.
Review does three things for us, roughly in this order of value.
It moves operational context. After a review, at least two people know what the change does at 2am. Not the diff, the behavior: what it retries, what it writes, what it will look like in the logs when it goes wrong. That is the whole reason I read a PR I did not write.
It catches the class of bug that hides from the test suite. Mostly these are bugs about data we do not have locally and languages the author does not read. More on both below.
It makes a decision findable. Six months later, the comment thread is often the only place the reasoning survives. Commit messages get squashed, docs go stale, but "we chose to fail closed here because the alternative shows the customer a wrong price" stays attached to the line it explains.
What review is not for: formatting, import order, naming style, or anything a type checker can decide. Those run in CI and I want zero human attention on them. I also do not want an approval count. A number of approvals measures nothing on a team where the reviewer pool is three people.
The reviewer is the one who gets paged
Our production is self-hosted. There is no managed database quietly failing over behind the scenes, no platform team who will notice before we do. When a change is wrong, someone here restarts a container and reads a log file.
That turns review into an operational exercise. The questions I actually ask myself while reading a diff:
- How will I know this broke? A path that returns an empty list when the query fails looks identical in the UI to a path that returns an empty list because there is nothing to show. If the difference is invisible in the logs, I will spend an hour on it later.
- If this is wrong, what is the fix at 2am? There are three answers, and they are not equal. Flipping a value in config is fine. Reverting and redeploying is acceptable. "Write and ship a patch" means the change needs more scrutiny now, because I do not want to be writing code at 2am.
- What does this hold, and for how long? A migration that takes an exclusive lock on a busy table is a production incident on a box we own. So is a request that keeps a Redis connection while it waits on something slow.
- What happens the second time this runs? Retries are the default in every layer we use. A handler that is safe once and destructive twice is the most common serious bug I find in review, and it is nearly always invisible in the tests, because the tests call it once.
None of that is about code quality. It is about whether the change is survivable.
The comments that waste the week
I have written all of these, which is how I know they are worthless.
The first is the style opinion on code a formatter already owns. If the tool did not complain, I should not either.
The second is a rewrite request disguised as a question. "Have you considered a factory here?" is not a question when the reviewer means "change this." The author then spends a day guessing whether the comment was blocking. Say which one it is.
The third is architecture reopened at review time. If the pull request is the first time I am hearing about the approach, the failure happened days earlier, when nobody asked. Re-litigating it in a review thread costs the author a week and produces a worse design than a fifteen minute conversation would have.
The fourth is the hardest to give up: comment volume as a proxy for effort. Fifteen nits and no question about failure behavior is a review that felt thorough and caught nothing.
The difference between a comment that helps and one that does not is usually whether it names a consequence. Two examples from real threads, weak version and the version that was worth writing.
Weak: "Maybe extract this into a helper?"
Better: "This price formatting block also exists in the listing
serializer. When the rounding rule changes, one of the two
will get missed and the search results will disagree with
the detail page. Worth having one copy before this merges."
Weak: "This looks wrong."
Better: "If carrierSubsidy is null, this falls through and returns the
retail price as the final number instead of failing. On the
consulting screen that shows the customer a price that is too
high, and nobody finds out until someone argues with it in
the shop. Should a missing subsidy raise instead?"
The second version is longer, and the author can act on it without asking me anything. It also does the thing I care most about: it says what the user sees when the code is wrong.
The comments that save an incident
Three questions find most of what we catch.
Which language is this string in when it reaches a person
This is the one where review genuinely outperforms every other check we have, because the bug is plainly visible in the diff and completely invisible in the running app if you only ever look at it in English.
The phone shop tool renders the entire consultation in seven languages. The student application suite takes an English-friendly form and has to produce Korean-ready output, with sample answers in both languages. A hardcoded English label in a template passes lint, passes types, passes tests, and passes a reviewer who opens the page in their own browser with their own locale.
So new user-facing text gets a key, never a literal, and I flag it in review every time. I also flag string concatenation that builds a sentence out of fragments, because word order is not the same in Korean as in English and the fragments cannot be reassembled correctly. And I ask about any element with a fixed width that now holds translated text, because the longest translation is not the one in the screenshot.
What does this do when the row is not there
Community data is user-generated and it is messy in ways seed data never is. A group whose member count was never filled in. A guide whose author deleted their account. A marketplace listing pointing at an uploaded image that no longer exists. Locally everything has a parent, because the fixtures created both sides. In production the orphan exists and the code path that assumed it did not is now a 500.
I ask this on every join, every optional foreign key, and every place code reads two fields and assumes they agree.
What is the state after this fails halfway
Not the happy path, not the fully failed path, the middle. A job posting written to the database while the notification fails. A file uploaded while the row that references it is never committed. This is the question that finds the bugs that produce support messages nobody can reproduce.
Mobile changes get a different review
A web deploy is reversible in minutes. Our self-hosted backend is reversible too: rebuild the image, restart the container, and the old behavior is back.
A React Native release is not. A bad build goes through store review, lands on devices, and stays there until a fixed build clears review and every user updates. Some users never update. So the reviewer's job on a mobile pull request is different: find every decision that is about to become permanent, and ask whether it should be a value the server can change instead.
In practice that means three things.
First, anything risky goes behind a server-controlled flag or a remote config value, so the fix does not require the store. Second, the client has to tolerate a server response it does not recognize yet, and the server has to tolerate a client that shipped months ago. New fields get added, old fields do not get removed on the same day, and an unknown enum value must not crash a list screen. Third, an unreachable API has to fail into a state a user can recover from without reinstalling.
That third one is easy to get wrong in a way review is good at catching, because the code that handles the error is right there in the diff and the condition that triggers it almost never happens in development.
What review cannot catch
Being honest about this matters more than any review checklist, because every failure a review cannot catch needs a home somewhere else.
| Failure | Does review catch it | Where we actually catch it |
|---|---|---|
| Missing or hardcoded translation | Reliably, it is visible in the diff | Review, plus a CI check for untranslated keys |
| Layout broken by a longer translation | Almost never | Opening the screen with the locale switched |
| Query that is fine on 40 rows and slow on 40,000 | Sometimes, if the reviewer knows the table | Query timing against production-shaped data |
| Missing index | Rarely | The Postgres slow query log, after the fact |
| Migration that locks a busy table | Sometimes | Running it against a copy of the production database |
| Double-write from a retry | Sometimes | A test that calls the path twice on purpose |
| Broken environment variable or container config | No, it is usually not in the diff at all | Nothing, unless we write it into the PR |
That last row is the one that hurt us most. When production is self-hosted, part of every change lives outside the repository: an environment file, a compose change, an image that has to be rebuilt rather than pulled. A reviewer reading only the diff sees a complete change that is in fact half a change. Now the PR description carries the deploy step, and the review includes it. A pull request that says "no deploy steps" is a claim the reviewer is checking, not a formality.
Say what you mean, then approve
We use the ordinary vocabulary. A comment is blocking, a suggestion, or a note. The part that actually matters is that the reviewer names the failure and the author picks the fix. I do not want to specify the implementation from the outside; I want the author to know what I am afraid of.
Approve with comments is our default. Blocking is reserved for a correctness or safety problem I can state in one sentence. On a team of five, blocking a pull request over taste means the feature waits on one person's mood, and it teaches everyone to route around review rather than through it.
The review before the review does more work than any of this. The author reads their own diff, in the diff view, before asking anyone else to. Half of what I would have commented on, I catch on my own code that way. And the PR description answers three things: what changes, what a user sees if it is wrong, and how to undo it. That third field is the highest value line in our whole process. If the answer is complicated, we have learned something important before anyone reads a line of code.
Where we still get it wrong
Review is best at the cheap things and worst at the expensive ones. It reliably catches a missing translation key, which takes two minutes to fix, and reliably misses a query that will fall over at ten times the current data volume, which takes a weekend.
We under-review changes that are not code. A pricing rule constant, a new content category, a threshold in a config file: each one ships user-visible behavior with nothing in the diff that looks like risk, and they slide through with an approval nobody thought hard about.
And I still approve things at 11pm that I would have questioned at 10am. The honest fix for that is not a better checklist. It is admitting that a tired reviewer is not a reviewer, and leaving the tab open until morning.
