Most code review approves code. Here is review that improves it

TL;DR#

A review that reads the diff can only approve or reject what is in it. The expensive defects are almost never in the diff — they are the thing the diff forgot, the assumption it encodes, and the place the fix was put.

Three questions, in descending order of value:

1. What is missing from this change?
2. What will this teach the next person who copies it?
3. Is this fix in the place all the callers go through?

Line-level comments are the cheapest thing in a review and the easiest to produce, which is exactly why they crowd out the other three.

The case this post is about#

A change to a publishing pipeline. The diff: a script that inserts a section into 165 already- published articles, plus a verification step that confirms the insert landed.

The verification function was four lines and carried this docstring:

/** The ONLY verdict that counts: is it on the page a reader loads? */
const liveHas = async (): Promise<boolean> => {
  await page.goto(PUBLIC_URL, { waitUntil: "domcontentloaded" })
  await page.waitForTimeout(5_500)
  return await page.evaluate(
    `(document.body.innerText||"").indexOf(${JSON.stringify(heading)}) !== -1`,
  ) as boolean
}

Read it as a reviewer. The naming is clear. The docstring states the right principle. It fetches the public URL rather than the editor, which is the hard-won part. There is no obvious bug, no style issue, nothing to leave a line comment on.

It is wrong in two independent ways, and the review that found them did not look at this code at all.

The symptom#

The run reported 163 clean insertions. Weeks later the actual review arrived, and it was one sentence with a screenshot:

you are repeating it

The screenshot showed one article with the section twice, back to back.

That is the entire review. No line reference, no suggested diff, no mention of liveHas. And it was worth more than every line comment the change could have attracted, because it asserted against the artifact a reader sees rather than the code that produced it.

The same reviewer later asked two more one-line questions on later changes:

did you generated stitch arts?

are you using the /idea-feature-stitch-art and /idea-feature-stitch-anim

Both found real defects. Neither referenced a line of code.

Why the obvious fix failed#

"Review more carefully." The code had already been read carefully, by someone who had just written a document explaining the exact principle it violated. Attention was not the scarce resource.

"Add a checklist." There was one. The project's capability matrix already carried a rule against selection gestures that overrun their block, written after an earlier incident. The change violated it. A checklist is read by whoever is already being careful.

"Require two approvals." Two readers of the same diff produce two readings of the same visible surface. Both would have seen a clear function with an accurate docstring. Reviewer count does not change what is in the frame.

"Catch it in tests." The test would have been written by the same person, against the same mental model, asserting on the same wrong surface. A self-consistent misunderstanding passes its own tests. That is not a gap in rigour, it is a property of consistency.

Every one of these scales effort on the diff. The defects were not in the diff.

The actual mechanism#

Reviewing a diff answers is this change correct. The expensive questions are different ones, and the frame hides all three.

What is missing? liveHas reads document.body.innerText after a fixed wait, with no scroll — and the platform lazy-renders long articles, so content below the fold is not in the DOM. The absent window.scrollTo loop is the defect. A diff cannot show you a line that was never written. The only way in is to ask what the change claims and check the claim against reality: it claims to answer "is this section present?", so go and look at a page that has one.

What will this teach? The second review — did you generated stitch arts? — found four articles shipped with procedural placeholder covers instead of generated art. The cause was a template with no instruction to generate them. The review's value was not the four covers; it was that the template would have produced the same defect on every future article. A review that fixes the instance and leaves the pattern has done a tenth of the available work.

Where does the fix belong? The third review — are you using the skills — found a bypass: the engine was being called directly, skipping five of its six steps. The first instinct was to fix the call. The actual fault was that the raw invocation had been written into the template, which would teach every future author to bypass the same way. A one-off error had been promoted to the house standard, and only the second question surfaces that.

The pattern across all three: each review asserted against something outside the change. The rendered article, the generated asset, the sanctioned command. None of them could have been reached by reading the diff more attentively, and all three were one sentence long.

The fix#

Review in this order, and spend the time in proportion:

1. WHAT IS MISSING?        — check the claim against the artifact, not the code
   · what does this change claim to do?
   · what would prove it, measured on the thing a user touches?
   · what is NOT in the diff that would have to be, for the claim to hold?

2. WHAT WILL THIS TEACH?   — is the pattern safe to copy?
   · if someone copies this file as a starting point, what do they inherit?
   · does the fix belong in the template, the generator, or the gate instead?

3. WHERE DOES THE FIX GO?  — one guard, or one per caller?
   · grep every caller; a guard in the shared path is a smaller diff AND a
     complete one, where a guard at this call site leaves the siblings broken

4. ONLY THEN: the lines.   — naming, edge cases, style

The ordering is the whole technique. Line comments are satisfying to write, they are visibly "review", and they consume the attention the first three need. Do them last, and accept that some reviews never reach them.

And the output of a good review is rarely a comment. Of the three above, each produced a mechanism: a scroll-and-count check that returns a cardinality instead of a boolean, a preflight rule that flags a placeholder cover, a template that dispatches the sanctioned command. None of them was a line comment, and none of the underlying defects could recur silently afterwards.

How we proved it#

The test of a review is not whether the author agreed. It is whether the defect can happen again.

For each of the three, the fix shipped with a deliberately-broken fixture that the new check must reject, run before the fix landed:

RED  (must fail all 3 assertions)
  reads body.innerText without scrolling — lazy-rendered content reads as absent
  compares page text without folding NBSP — a widow-space copy is missed
  has no not-live verdict — cannot report an edit that failed to land
GREEN  idea-medium-mcp: PASS

That RED fixture is itself worth a note. Its first version passed two of the three assertions — because the fixture's header comment contained the words the check greps for, while its code did neither. A gate a comment can satisfy certifies nothing. The check strips comments now.

Which is the same lesson one layer up: I nearly accepted a passing result from a thing I had built to fail.

Where this fails#

Three honest limits, because "review the artifact" is not free.

It needs an artifact to review. A pure refactor with no behavioural change produces nothing new to look at, and the first question degenerates to "is it still the same?" — which is what a test suite is for. The technique is strongest on changes that claim something observable, and weakest on the ones that deliberately claim nothing.

It is slower, and the cost is real. Opening the page, running the command and checking the provenance takes minutes where skimming a diff takes seconds. On a team reviewing thirty pull requests a day that is not affordable for all of them, and pretending otherwise is how a good practice gets abandoned wholesale. Spend it where a wrong answer is expensive — anything irreversible, anything a user sees, anything that becomes a template.

A one-sentence review lands differently depending on who sends it. "You are repeating it" read as useful because it came with a screenshot and no blame. The same four words without the evidence is just an assertion, and from a senior reviewer to a junior author it is closer to a verdict than a review. The brevity worked because the artifact did the arguing; if you cannot attach the artifact, write the longer version.

What this generalizes to#

The uncomfortable finding is how little the three valuable reviews had in common with reviewing.

None read the diff. Each looked at an artifact — a rendered page, a generated image, a command history — and asked whether it matched what had been claimed. Each was one sentence. Each produced a mechanism rather than a comment.

So if you want reviews that improve code rather than approve it:

Review the artifact, not only the change. Open the page. Run the command. Look at the generated file. The diff is a description of work; the artifact is the work.

Ask what the change teaches. A defect in a template, a generator or an example is not one defect. It is one per future use, and it arrives without anyone making a new mistake.

Treat "it looks fine" as a null result. A diff that reads cleanly has told you about the visible surface only. It is weak evidence about the surface nobody rendered.

Prefer a mechanism to a comment. A comment fixes this instance and relies on memory. A gate fixes the class. If a review cannot produce a mechanism, it should at least produce the question the next mechanism will answer.

The best review I received on any of this was four words long and came with a screenshot. It found a defect that three automated gates, a documented rule, and a careful reading of my own code had all missed — because it was the only one looking at what a reader would actually see.

Related#

• 163 checks reported green. None of them read the published page. — the defect that review found
• A canary that cannot fail proves nothing — why the RED fixture runs first
• Source, not symptom — where the fix belongs

Keep reading#

This is part of Architecture decisions & code review — the judgement calls that outlast the code they were made about. Follow MobileByteSensei for the rest.