#What code review is for
AddedCode review is a second engineer reading a change before it merges, and the two of them agreeing it leaves the codebase better than it was. Most teams say the point is catching bugs. The research says that's only part of it, and not the biggest part. In Modern Code Review: A Case Study at Google (Sadowski et al., ICSE SEIP 2018, nine million reviewed changes), Google engineers described four expectations: education (teaching and learning), maintaining norms (style, API usage, the team's way of doing things), gatekeeping (no one commits arbitrary code without oversight) and accident prevention (bugs and defects), plus using review history later to understand how a change came about. The paper's first finding is that review there "does not center around problem solving": it was introduced to keep code readable and maintainable, and defect finding is "welcomed but not the only focus".
Microsoft's numbers point the same way. Czerwonka, Greiler and Tilford (Code Reviews Do Not Find Bugs, ICSE SEIP 2015) report that only about 15% of review comments point to a possible defect, while at least half are about long-term maintainability. Bacchelli and Bird (Expectations, Outcomes, and Challenges of Modern Code Review, ICSE 2013) found that developers expect review to find defects, but what it mostly delivers is code improvements, knowledge transfer and team awareness. The 15% figure is about what comments say, not how many bugs reviews catch, so don't quote it as "reviews don't find bugs"; quote it as "most of review's value isn't bug-finding".
A useful mental model: a reviewer is an editor, not a proofreader. A proofreader catches typos and house-style slips, which a spell checker can do. An editor asks whether the chapter should exist, whether the argument holds, whether a reader will follow it, and whether a fact is wrong. In code, the spell checker is your formatter, linter, type checker and tests (see Automating the boring parts); the human reviewer edits. And like a good editor, the reviewer comes away knowing the book: after the review, two people understand the change instead of one.
| Purpose | What it looks like in a review | What it isn't |
|---|---|---|
| Accident prevention | "This deletes the row before the upload succeeds; a failed upload loses the data." | A substitute for tests. If the reviewer found it by reading, a test should pin it. |
| Design and maintainability | "This puts device state in two places; can the poller read it from the store?" | A redesign. Big design disagreements belong in a design doc before the code. |
| Knowledge sharing | "There's a withTimeout helper in http.js that clears its timer; use it here." | A lecture. Link the helper or the doc; keep it to what this change needs. |
| Norms | "Errors use the problem() shape so the UI can show detail." | Personal taste. If it isn't in the standard or the surrounding code, it's at most a nit. |
| Gatekeeping and history | "Who can call this? It reboots a device in a live meeting." Later: "why did we add this check?" → the review thread. | A power move. Holding approvals hostage is a breakdown the Google paper names explicitly. |
Here is a real change, made in a throwaway Git repository so the diff is exactly what a reviewer would see. main has a shared http.js with two helpers; the branch adds an endpoint that reboots a conference-room device:
// A real pull request in a throwaway repository: main has shared helpers, the branch adds an endpoint.
import { execFileSync } from "node:child_process";
import { mkdtempSync, mkdirSync, writeFileSync, rmSync } from "node:fs";
import { tmpdir } from "node:os";
import { join } from "node:path";
const dir = mkdtempSync(join(tmpdir(), "review-"));
const env = { ...process.env, GIT_CONFIG_GLOBAL: "/dev/null", GIT_CONFIG_NOSYSTEM: "1" };
const git = (when, ...args) => execFileSync("git", args, { cwd: dir, encoding: "utf8",
env: { ...env, GIT_AUTHOR_NAME: "Priya", GIT_AUTHOR_EMAIL: "priya@example.com", GIT_COMMITTER_NAME: "Priya",
GIT_COMMITTER_EMAIL: "priya@example.com", GIT_AUTHOR_DATE: when, GIT_COMMITTER_DATE: when } });
const write = (path, text) => { mkdirSync(join(dir, path, ".."), { recursive: true }); writeFileSync(join(dir, path), text); };
git("2026-10-05T09:00:00Z", "init", "-q", "-b", "main");
write("src/http.js", `// Shared helpers every handler uses.
export const problem = (status, title, detail) => ({ status, body: { type: "about:blank", title, status, detail } });
export function withTimeout(promise, ms, label) {
let timer;
const timeout = new Promise((_, reject) => { timer = setTimeout(() => reject(new Error(\`\${label} timed out\`)), ms); });
return Promise.race([promise, timeout]).finally(() => clearTimeout(timer));
}
`);
write("src/devices.js", `import { problem } from "./http.js";
export async function getDevice(store, id) {
const device = await store.find(id);
if (!device) return problem(404, "Device not found", \`No device \${id}\`);
return { status: 200, body: device };
}
`);
git("2026-10-05T09:00:00Z", "add", ".");
git("2026-10-05T09:00:00Z", "commit", "-q", "-m", "Device lookup endpoint");
git("2026-10-06T10:00:00Z", "switch", "-q", "-c", "reboot-device");
write("src/devices.js", `import { problem } from "./http.js";
export async function getDevice(store, id) {
const device = await store.find(id);
if (!device) return problem(404, "Device not found", \`No device \${id}\`);
return { status: 200, body: device };
}
export async function rebootDevice(store, agent, id, user) {
const device = await store.find(id);
if (!device) return { status: 404, body: { error: "not found" } };
await Promise.race([
agent.reboot(device.address),
new Promise((_, reject) => setTimeout(() => reject(new Error("timeout")), 5000)),
]);
await store.audit("reboot", id);
return { status: 202, body: { id, state: "rebooting" } };
}
`);
git("2026-10-06T10:00:00Z", "commit", "-q", "-am", "Add reboot endpoint");
console.log(git("2026-10-06T10:00:00Z", "log", "--oneline", "--graph", "--all"));
console.log(git("2026-10-06T10:00:00Z", "diff", "--stat", "main...reboot-device"));
console.log(git("2026-10-06T10:00:00Z", "diff", "main...reboot-device"));
rmSync(dir, { recursive: true, force: true });* cc77944 Add reboot endpoint
* 1974ba3 Device lookup endpoint
src/devices.js | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/src/devices.js b/src/devices.js
index c3c338f..54c1ec9 100644
--- a/src/devices.js
+++ b/src/devices.js
@@ -5,3 +5,14 @@ export async function getDevice(store, id) {
if (!device) return problem(404, "Device not found", `No device ${id}`);
return { status: 200, body: device };
}
+
+export async function rebootDevice(store, agent, id, user) {
+ const device = await store.find(id);
+ if (!device) return { status: 404, body: { error: "not found" } };
+ await Promise.race([
+ agent.reboot(device.address),
+ new Promise((_, reject) => setTimeout(() => reject(new Error("timeout")), 5000)),
+ ]);
+ await store.audit("reboot", id);
+ return { status: 202, body: { id, state: "rebooting" } };
+}What’s happening
- The model builds a real repository: one commit on
mainwithhttp.js(the team'sproblem()error shape and awithTimeouthelper that clears its timer) anddevices.js, then a branchreboot-devicewith one commit. Author, committer and dates are fixed, so the hashes (cc77944,1974ba3) are the same every run.GIT_CONFIG_GLOBAL=/dev/nullkeeps anyone's personal Git settings out of it. git diff main...reboot-device(three dots) is what a pull request page shows: the changes on the branch since it leftmain, ignoring anything that landed onmainmeanwhile. The hunk header@@ -5,3 +5,14 @@ export async function getDevice…says the old file's lines 5–7 became lines 5–18 of the new one; Git prints the nearest function name as context.- Read it as an editor. Accident prevention:
useris passed in and never used, so anyone who can call the API can reboot a room in the middle of a meeting. Norms: the 404 returns{ error: "not found" }, not theproblem()shape every other handler uses, so the UI's error banner will show nothing useful. - Knowledge sharing: the author hand-rolled a timeout with
Promise.raceand a baresetTimeout. It works, but the timer is never cleared, so every successful reboot leaves a five-second timer behind (proved in the next box), and the error says only"timeout". The team already haswithTimeoutfor exactly this; the author didn't know, which is the cheapest thing a review can fix. - Gatekeeping and history:
store.audit("reboot", id)records what happened but not who did it. When someone asks in three months why a room rebooted during a board meeting, the audit log and this review thread are the history. - None of these is a typo a linter would catch, and only one (the permission check) is a classic "bug". That's the research in miniature: the review's value here is mostly norms, shared knowledge and safety, with a defect thrown in.
AdvancedThe review comments on the reboot PR
These are the comments that review leaves, in the Conventional Comments format explained in Giving and receiving feedback (label, whether it blocks, then the why):
src/devices.js:9 issue (blocking): any signed-in user can reboot any device.
`user` is passed in but never checked. Rebooting drops a live call, so this needs the same
rule as the admin UI: return problem(403, …) unless the user has the device-admin role
for the device's site. Please add a test for the viewer case so it stays fixed.
src/devices.js:12 suggestion (non-blocking): use withTimeout from http.js.
It clears its timer and names what timed out ("reboot timed out"). The Promise.race here
leaves a 5 s timer pending after every successful reboot.
src/devices.js:11 todo: return problem(404, "Device not found", …) like getDevice does.
The UI reads `detail` from that shape; { error } shows an empty banner.
src/devices.js:13 question: what should the caller see when the device is offline?
Right now agent.reboot's rejection becomes a 500. Is 504 with a problem body more useful
for the room-status page?
src/devices.js:16 suggestion: record who did it: store.audit("reboot", id, user.id).
general chore: no test file changed. A test for 202, 403 and 404 is enough.
src/devices.js:17 praise: 202 with { state: "rebooting" } is the right answer for an
action that finishes later. Nice.
Summary: requesting changes for the permission check and the tests; the rest are small.
Happy to look again today, or to pair for 15 minutes in the overlap if that's quicker.What’s happening
- Each comment names a line, a label and a decoration. The author can sort them in ten seconds: two things block (
issue (blocking)and thechorefor tests), onetodois a small required fix, and the rest are suggestions, a question and praise. - Every comment says why, not just what: "rebooting drops a live call", "the UI reads
detail". The why is what turns a review into teaching; next time the author applies the rule without being told. - The comment on the timeout points at the existing helper instead of rewriting the code for the author. Google's guide puts it this way: the developer is responsible for fixing the change; the reviewer's job is to point at the problem and help.
- The
questionis a real question: the reviewer doesn't know the right status for an offline device and says so, instead of dressing an opinion up as a rule. - The summary states the verdict, what to fix before approval, and offers a faster channel. On a US–India team that offer matters: a 15-minute call in the overlap can save a whole day of round trips (see Small pull requests and fast reviews).
AdvancedProving the timer leak before you claim it
The suggestion above claims the hand-rolled Promise.race leaves timers behind. A claim in a review should be checkable, so here it is, checked. Node's process.getActiveResourcesInfo() lists what's keeping the event loop alive:
// The branch's hand-rolled timeout vs the team's withTimeout helper. What does each leave behind?
function handRolled(promise, ms) {
return Promise.race([promise, new Promise((_, reject) => setTimeout(() => reject(new Error("timeout")), ms))]);
}
function withTimeout(promise, ms, label) {
let timer;
const timeout = new Promise((_, reject) => { timer = setTimeout(() => reject(new Error(`${label} timed out`)), ms); });
return Promise.race([promise, timeout]).finally(() => clearTimeout(timer));
}
const agent = { reboot: async () => "ok" }; // a healthy device answers at once
const pendingTimers = () => process.getActiveResourcesInfo().filter((r) => r === "Timeout").length;
for (let i = 0; i < 100; i++) await handRolled(agent.reboot(), 300);
console.log(`hand-rolled: 100 reboots finished, ${pendingTimers()} timers still pending`);
await new Promise((resolve) => setTimeout(resolve, 350));
console.log(`350 ms later: ${pendingTimers()} timers pending (they fired and rejected into nothing)`);
for (let i = 0; i < 100; i++) await withTimeout(agent.reboot(), 300, "reboot");
console.log(`withTimeout: 100 reboots finished, ${pendingTimers()} timers still pending`);
const slow = { reboot: () => new Promise((resolve) => setTimeout(resolve, 1000)) };
const errors = await Promise.allSettled([handRolled(slow.reboot(), 50), withTimeout(slow.reboot(), 50, "reboot")]);
console.log("error messages:", errors.map((e) => e.reason.message));hand-rolled: 100 reboots finished, 100 timers still pending
350 ms later: 0 timers pending (they fired and rejected into nothing)
withTimeout: 100 reboots finished, 0 timers still pending
error messages: [ 'timeout', 'reboot timed out' ]What’s happening
- Both functions race the real call against a timer.
handRolledis the branch's code;withTimeoutis the helper already onmain, which keeps a handle to its timer and clears it in.finally(). - After 100 fast reboots with
handRolled, 100 timers are still pending: each one will fire 300 ms later and reject a promise nobody is listening to any more. In a server handling thousands of requests, that's thousands of live timers and closures held for five seconds each, for nothing. - Once they've all fired, the count drops to 0. Nothing crashed, which is why this survives testing: the leak is only visible as memory and event-loop work under load.
- With
withTimeout, the count is 0 right after the loop:.finally(() => clearTimeout(timer))cancels each timer as soon as the reboot answers. - The last line shows the second difference: on a real timeout the messages are
"timeout"and"reboot timed out". When this shows up in a log at 2 a.m., the second one tells you what timed out. That's the kind of thing the reviewer knows because they've debugged it before, and the review is how that knowledge spreads.
AdvancedWhat review isn't, and the breakdowns to avoid
- Not where design gets decided. If the reviewer's real comment is "this whole approach is wrong", the team skipped a step. Agree the approach in a short design note, an issue comment or a 15-minute call before the code (see Architecture decision records). The Google paper lists "review subject" as a breakdown: some teams expected design to be settled before the first review, others wanted to discuss it in the review, and the mismatch caused friction.
- Not a replacement for tests or QA. A reviewer reading 300 lines finds some bugs; a test runs on every change forever. When a review finds a bug, ask for the test that would have caught it.
- Not the lead's approval stamp. If every change waits for the lead, the lead is the bottleneck and the team learns nothing about each other's code. Rotate reviewers, use CODEOWNERS for areas that truly need an expert, and review the reviews instead of every diff.
- Not a power tool. The Google paper names two social breakdowns: tone (authors are sensitive to comments; the paper cites earlier sentiment research finding that negatively worded comments are less likely to be useful) and power (dragging out reviews or withholding approval to make someone change behaviour). Both are a lead's problem to notice and fix.
- Not context-free. The paper's "context" breakdown: reviewers didn't know whether a change was an urgent production fix or a nice-to-have, so they reviewed it with the wrong urgency. A PR description that says why, and how urgent, prevents it.
AdvancedRubber-stamp approvals
Which purpose does each comment serve?
Match each comment to education, norms, gatekeeping or accident prevention:
- "If the upload fails after this line, we've already deleted the old file."
- "We name REST collections in the plural:
/devices, not/device." - "TIL
Intl.RelativeTimeFormat— nice, I didn't know about that." - "This changes who can call the billing export. Can someone from the payments team look?"
Show answer
1 is accident prevention, 2 is norms, 3 is education (going the other way), 4 is gatekeeping.
What’s happening
- Comment 1 describes a sequence that loses data on a failure path. That's a defect: accident prevention, and it should come with a request for a test of the failure path.
- Comment 2 isn't about correctness at all:
/deviceworks. It's about the team's agreed convention, so it's norms, and it's only blocking if the convention is written down (ideally enforced by a linter or an API style check). - Comment 3 is the reviewer learning from the author. Education runs both ways; seniors learn from juniors' reviews too, and saying so is good for the team.
- Comment 4 is gatekeeping in the good sense: a boundary around a sensitive area, with the right owners pulled in. CODEOWNERS automates exactly this (see Automating the boring parts).