When to Request Changes Instead of Approving with Notes
A code review tool offers two buttons that look equally reasonable. One says approve with notes. The other says request changes. The difference between them is the difference between a comment and a gate, and the entire craft of review sits inside that gap. Approve with notes tells the author: this is fine, I have thoughts, ship it when you like. Request changes tells the author: this does not merge until I am satisfied. One is advisory. The other holds the door shut.
Most review friction comes from people choosing the wrong button, or choosing the right button for the wrong reason. A reviewer who requests changes over a missing comma will burn goodwill fast. A reviewer who approves with notes a change that quietly breaks the login flow is worse. The button is a signal about consequence, and consequences vary enormously from one line of code to the next.
The useful frame is not severity. Severity is a word people argue about. The useful frame is reversibility: what does it cost to fix this after the merge instead of before? A naming debate costs nothing to fix later. A database migration that drops a column costs a great deal. That gap is where the request-changes button belongs.
The cost of a fix after the merge
Picture a pull request that renames a helper function from getUser to fetchUser across eleven files. A reviewer who prefers getUser can leave a note and approve. If the team later changes its mind, the rename runs the other direction in an afternoon. Nothing is lost. The button should be approve with notes.
Now picture a pull request that adds a retry loop to an HTTP client without a backoff limit. The code looks tidy. The tests pass. If that loop ships and a downstream service starts returning 503s, the retries multiply traffic exactly when the service is struggling. Fixing it after the merge means an incident, a rollback, and a postmortem. The reviewer who leaves a note and approves has made a choice that will be expensive to reverse. This is a request-changes situation, even though the diff is small and the code is clean.
The size of the diff is a terrible proxy for the size of the consequence. A forty-line change to a billing calculation can be a request-changes event. A four-hundred-line change to a test suite can be approve with notes. Reviewers who scan for line count and ignore blast radius are reviewing the wrong thing.
There is a practical test worth applying before clicking either button. Ask what happens if this merges exactly as written and nobody touches it again for a month. If the answer is "some people will be mildly annoyed and we fix it in a follow-up", approve with notes. If the answer involves user-visible breakage, data loss, a security hole, a support queue, or a rollback at an inconvenient hour, request changes. The month is the key variable. Reviews that assume a fast follow-up are usually wrong about the follow-up.
When the note is doing work a gate should do
A pattern shows up in teams that review well and in teams that review badly, and it looks the same from the outside. The reviewer leaves a long comment, approves, and moves on. The comment says something like "this will cause a race condition under load, but we can address it later". Later arrives. The race condition is now in production. The comment is now a historical artifact, which is to say, a piece of evidence that the reviewer saw the problem and chose not to block.
That is the failure mode of approve with notes: it converts a known defect into a wish. The button exists for exactly the situation where the reviewer's concerns are real but not urgent, and it gets used instead for concerns that are urgent but inconvenient to raise formally. The convenience is the tell. If leaving a note feels easier than blocking, that is often because blocking would require a conversation the reviewer does not want to have.
Request changes is a conversation starter, and that is its real function. It says: stop, let us talk about this before it goes further. The friction is the point. Approve with notes is frictionless by design, which makes it the wrong tool for anything that deserves friction.
The cases that almost always deserve the gate
Some categories of change earn a request-changes by default. Data migrations top the list. Anything that alters schema, backfills a column, or rewrites rows in place has a one-way quality that notes cannot capture. A reviewer who approves a migration with notes is betting the author reads them before deploying, and the author is often deploying at the end of a long day.
Authentication and authorization changes belong in the same bucket. So do changes to payment flows, anything touching personal data, and anything that alters how secrets are stored or transmitted. The review of these changes is not primarily about code quality. It is about whether the change is correct, because the cost of an incorrect merge is measured in trust and sometimes in regulatory exposure.
Public API contracts deserve the gate as well. Once a field is renamed or an endpoint changes shape, downstream clients break, and the breakage shows up weeks later in someone else's system. The reviewer who approves a contract change with a polite note has pushed the problem onto a person who was not in the room.
There is also a category that gets less attention: changes that are correct but unimplementable by the current team. A clever abstraction that only its author understands, a build step that requires a tool nobody else has installed, a dependency added for a single function. These are not bugs. They are future maintenance costs, and they are worth blocking on precisely because they are easy to wave through.
Style, naming, and the note that should have been a nit
The opposite error is just as common. A reviewer requests changes over a variable named data, or a missing blank line, or a comment that could be clearer. The author now has to make a round trip for something that mattered to exactly one person. This is how request changes loses its authority. Every time the gate is used for a triviality, the next request changes on the same author carries less weight.
Style concerns belong in notes, or better, in automated checks. A linter does not have opinions about tone and does not get tired. If a team finds itself requesting changes over the same three formatting issues every week, the fix is a config file, not a firmer review stance.
The harder judgment is naming and structure. A reviewer who thinks a module is organized the wrong way can leave a note and approve, and the code merges, and six months later the module is still organized the wrong way because nobody went back. If the structure genuinely matters, the gate is appropriate. If it is a preference, the note is appropriate. The way to tell the difference: would the reviewer be comfortable if the author merged this and never revisited it? If yes, note. If no, request changes.
Comments that read as orders
There is a subtler problem with approve with notes. The button says approve, but the comment says something else, and the author has to guess which one the reviewer meant. A note that reads "we should not do this" attached to an approval is a mixed signal, and mixed signals are how reviewers accidentally ship things they opposed.
The author, for their part, usually reads the approval first and the comment second, and treats the approval as permission. That is rational behavior. The button is the contract. If the reviewer wanted a gate, the reviewer should have used the gate.
There is a discipline here worth naming. A note that objects to the change should be a request changes, and a note that suggests an improvement should be an approval. The two buttons map to two different speech acts: "this must change" and "this could change". Reviewers who blur them force the author to interpret, and interpretation under deadline pressure tends toward the permissive reading.
The judgment call that belongs to the reviewer
The button is a judgment about risk, and risk is not evenly distributed across a codebase. A change to a logging line and a change to an authentication check are not the same event, even if both arrive as small diffs on a Tuesday afternoon. Treating them identically is not fairness. It is a failure to look.
The reviewer who defaults to approve with notes is usually optimizing for velocity, and that is a reasonable instinct. The reviewer who defaults to request changes is usually optimizing for safety, and that is also reasonable. Neither default survives contact with a real codebase, because a real codebase contains both a rename and a migration in the same week.
What survives is the habit of asking the month question before clicking. If the change merged tonight and nobody touched it for thirty days, what would be true? If the answer is "fine, maybe a little messy", approve with notes. If the answer is "something we would have to unwind under pressure", request changes. The button follows the answer.
A reviewer who can name which of the two buttons a given change deserves, and can say why in one sentence, is doing the job. A reviewer who reaches for the same button every time is doing something easier than the job, and the codebase will eventually show the difference.
