Code Review — How to Give and Get Useful Feedback

Ch 18 covered the mechanics of opening a PR. This chapter covers the harder half: the conversation that follows. Code review is the back-and-forth between the PR author and one or more reviewers — leaving comments, requesting changes, approving, eventually merging.

It is the single most awkward part of joining a real team. The first time someone tells you "this could be cleaner" on a Monday morning, you'll feel personally attacked. The first time you have to tell someone their code could be cleaner, you'll find yourself rewriting the comment five times. There's a real skill here that nobody teaches and everybody learns by getting it wrong for a year.

This chapter gives you the shortcut: what reviewers actually look at, how to write comments that move the PR forward instead of starting fights, what each GitHub button means, and how to receive feedback without flinching.

Why Code Review Exists

The naive answer is "to catch bugs". That's part of it. The bigger answer is closer to:

A second pair of eyes on every change keeps the codebase coherent as it grows, surfaces bugs that the author was too close to see, and spreads knowledge so the project doesn't have a single bus-factor.

In other words, review does four things and only one of them is bug-catching:

1. Catch defects the author missed.
2. Maintain consistency across a codebase that more than one person edits.
3. Spread knowledge so the team isn't a bunch of single-owner silos.
4. Mentor — every comment is a tiny lesson, going both ways.

Teams that treat review as "catch bugs only" end up with grumpy reviewers and missed opportunities. Teams that treat it as all four end up with a stronger codebase and a stronger team.

Figure 1 — Inside a reviewer's head. The order matters: understand the intent first, then check for correctness, then look at fit and maintainability. Comments come last, after you've actually read the change.

What Reviewers Actually Look At

Open any PR. There's hundreds of lines of diff. Where do you even start? A working checklist most senior reviewers run, roughly in this order.

1. Does the PR description make sense?

Before reading any code, read the description. If you can't tell what the PR is trying to do, neither can anybody else, and the answer to every later question becomes "I don't know — what's the goal?". Leave a comment: "Can you add 2 sentences on the why?" Wait for that. Then review the code.

2. Does the change match the description?

The PR says "fix typo in header". The diff also restructures three unrelated files. That's a different conversation: "Looks like there's a refactor mixed into this typo fix — can we split it?" PRs that change more than they claim are nearly always a problem.

3. Read the tests first

Counterintuitively, the tests are the best place to start. They tell you what behaviour the author thinks they're producing. Often the bug is in the test (wrong expected value, missing case) and finding it there is faster than spelunking the source.

4. Then read the code

Now read the production code with the test contract in your head. Specifically check:

• Edge cases: empty input, null, very long input, very small/large numbers, Unicode, timezones, simultaneous calls.
• Error paths: what happens when the network call fails? When the DB query returns zero rows? When the user is logged out?
• Naming: would a new reader understand processData without scrolling, or does it need to be normaliseUserPayloadForStripe?
• Dead code: imports you don't use, branches you can't reach, console.logs left over from debugging.

5. Check the cross-cutting concerns

• Security: input validated? Secrets not logged? Auth checks not bypassed?
• Performance: any N+1 queries, blocking calls in hot loops, big arrays held in memory?
• Backwards compatibility: if it changes an API or schema, will old clients break?
• Tests for new behaviour: every new feature should have a test that would catch its absence.

6. Then the nitpicks

Style, variable naming, comment wording. These matter, but only after the previous categories. If you start with nitpicks, the author fixes them, you go back to the main review, and find a big bug — frustrating for both sides.

How to Write a Useful Review Comment

Bad comments make the author defensive and slow the PR down. Good comments move it forward. The difference is almost always wording.

The four-part pattern

A comment that consistently lands:

[Suggestion | Question | Bug | Nit]: <what>
 
<why>
<optional concrete proposal>

The prefix tells the author what kind of attention this needs. Examples:

• Bug: Something is genuinely broken. Author must fix to merge.
• Suggestion: A better approach exists. Author can take or leave it with a one-line response.
• Question: You're not sure if it's right, you want clarification. Author replies; you decide.
• Nit: Stylistic pick. Author can ignore entirely. Some teams require labelling nits explicitly so they never block a merge.

Concrete examples

Bad:

This is wrong.

Good:

Bug: formatPrice(0) returns "$0.00", but the test on line 14 expects "Free" for the free tier. We should add an if (cents === 0) return "Free"; at the top, or update the test.

Bad:

Why are you doing this?

Good:

Question: I'd have reached for a for…of loop here — is there a reason you went with forEach? No strong preference, just curious about the call-site difference.

Bad:

Use const here.

Good:

Nit: let user (line 12) is never reassigned. const would let the linter prove no one mutates it later. Take or leave.

The pattern is the same every time: state the issue, explain why, offer a path. That's three sentences. Anything shorter risks looking like a peck. Anything longer probably belongs in a synchronous chat.

Tone — pretend you're paired

Imagine you're sitting next to the author at the same screen. You'd never say "this is wrong" out loud — you'd say "huh, what about this case?". Write your review comments the way you'd say them. Adding "I think" or "could we" or "what about" costs you two seconds and changes the entire emotional load of the comment.

This isn't about being soft. The strongest reviewers in any org are also the politest, because their comments get read instead of skimmed.

The Three Review Buttons on GitHub

When you submit a review on GitHub, you pick one of three options. They have specific meanings.

A common newcomer mistake: clicking Request changes for a typo or a nit. That blocks the merge and is treated as a serious "stop ship" signal. Use Comment for non-blocking feedback. Reserve Request changes for things that actually warrant blocking.

Counter-mistake: rubber-stamp approving

The other extreme is approving every PR after a 10-second skim. That defeats the entire point of review. If you don't have time to read it properly, leave a comment ("Out of time today, can someone else take this?") instead of approving.

Disagreeing Without Going to War

Some review threads turn into multi-day arguments. They burn out everyone involved and almost always end with someone capitulating out of fatigue rather than agreement. A few tactics that prevent this.

Move the discussion off the PR

After 2-3 round-trips on the same point, drop it from the PR thread and pick it up in chat or a 10-minute call. Text is the worst medium for nuance, and PRs are the worst medium for text.

Distinguish "I'd do it differently" from "this is wrong"

Most disagreements are taste. If the author's approach works, is tested, and is consistent with the codebase, your preferred way is just your preferred way. Approve. Maybe say "personally I'd have used X, but this works — fine to merge".

If it's genuinely wrong (bug, security issue, breaks an invariant), explain the failure mode concretely. "This breaks because…" not "I don't like…".

"Trust the author, suggest the change"

A useful default in remote teams: the author is closer to the problem than the reviewer. If they considered your suggestion and chose differently, they probably have a reason. Ask for it before pushing. "Did you consider X? Curious why you picked Y" — and then actually listen to the answer.

Escalate when stuck

If two reasonable people genuinely can't agree, ask a third. A senior engineer or tech lead breaks the tie. The point isn't who "wins" — it's that the team doesn't stall over one PR.

Receiving Review Feedback Without Flinching

You'll get more reviews than you'll give. Reading critique of your code is a skill. Three things help.

1. Separate "the code" from "you"

The reviewer is reviewing the code. They are not reviewing you. The fix for both bad code and good code is the same — change the code. There's no character grade in code review.

Junior engineers often read a comment as "you're stupid". Senior engineers read the same comment as "interesting, let me check". The senior reading is closer to accurate. The reviewer is almost always trying to make the codebase better, not to dunk on you.

2. Reply to every thread

Each comment should get a response, even just a 👍 or "good catch, fixed in <commit-sha>". Silent threads are how reviewers learn to feel ignored. The two minutes you spend acknowledging is the cheapest team-relationship investment you'll ever make.

3. Push back when you think they're wrong

Reviewers are sometimes wrong. You're the one who built the thing. If a comment misses context, say so: "Actually X already handles this case — see line 88 of helpers.js". Don't silently capitulate just to merge — that ships worse code and trains the reviewer to leave more wrong comments. Politely pushing back is part of the job.

The shape: "I see what you mean, but here's why I did it this way..." then explain. The reviewer either updates their mental model or comes back with a better argument. Either outcome is a win.

4. Squash the WIP commits

After your branch has commits like "WIP", "fix typo", "address review", "more review", the PR will be merged with squash-and-merge (Ch 18) anyway. Don't waste time tidying — but do edit the squashed commit message on the merge dialog so the final commit on main reads cleanly.

When You're the Only Person on the Project

Solo developers can — and should — review their own PRs. The act of opening a PR on yourself, walking away for 20 minutes, then coming back and reading it cold catches a ton of stuff. You'll find debug console.logs, comments you forgot to remove, broken indentation, and at least one "wait, what was I doing here" moment.

The trick is the delay. Same-second review is useless — you still remember exactly what you wrote. 20 minutes is enough that you read it more like a reviewer than an author.

On simpleappshipper.com (a solo project) this is the actual loop:

1. Open the PR.
2. Go make coffee or check something else for 10-20 minutes.
3. Come back, click "Files changed", read every line as if it's a stranger's.
4. If anything makes you pause, leave a self-comment, fix it, push.
5. Merge when clean.

Step 3 catches more bugs than CI does, by a wide margin.

What a Healthy Review Cycle Looks Like

For an average PR (~100-300 lines):

Anything slower than that on small PRs is a smell — either the team is too busy, the PR is too big, or the priority isn't shared. Anything faster is great if the review was real, and a problem if it was a rubber stamp.

Big PRs (> 500 lines, multi-file refactors) legitimately take longer. The fix is usually "don't make big PRs", not "review faster".

CodeRabbit, Copilot Code Review, and Other Bots

In 2026, several tools (CodeRabbit, GitHub Copilot for PRs, Graphite Reviewer) post AI-generated review comments on every PR. They're useful but not a replacement.

What they're good for:

• Catching common patterns (missing error handling, accidentally committed secrets, obvious bugs).
• Summarising the diff for human reviewers.
• Suggesting tests for new functions.

What they're not good for:

• Knowing whether the architectural decision fits the project's style.
• Understanding the bug the PR is fixing.
• Spotting that the change conflicts with an unrelated invariant elsewhere in the codebase.

Treat the bot as a tireless intern that catches the easy stuff. Don't approve a PR because the bot did — read it yourself.

ultrareview — The simpleappshipper.com Specific One

The Claude Code skill /ultrareview runs a multi-agent cloud review of the current branch or a GitHub PR. It's user-triggered, billed per run, and surfaces findings the author sees in their terminal — bugs, missing tests, style issues, all at once.

It is not a substitute for a human reviewer. It's a powerful pre-flight check the author runs before requesting human review. The author addresses what's real, ignores what's noise, then asks a human to look. By the time the human opens the PR, the easy 80% of the review is already addressed.

If the project has the skill installed, run /ultrareview on your own PRs as part of step 3 of the solo-review loop above. It's like having an extra opinion without scheduling a meeting.

Mental Model — Three Sentences

1. Code review exists to catch defects, maintain consistency, spread knowledge, and mentor — bug-catching is only one of four jobs.
2. A useful comment names the type (Bug / Suggestion / Question / Nit), the what, the why, and a concrete path — not a one-word "wrong".
3. The three review buttons mean specific things: Approve unblocks merge, Request changes blocks it, Comment signals feedback without a verdict — use them deliberately.

Try It Yourself (15 Minutes)

The fastest way to internalise reviewing: review.

1. Find an open-source repo you use. Open the Pull requests tab on GitHub.
2. Pick a PR with 50-200 lines of diff. Read the description first. Then the tests. Then the production code.
3. In your head (or in a private notepad), draft 3-5 comments using the four-part pattern. Don't post them on someone else's PR unless they invited it.
4. Compare your draft comments to what the actual maintainers wrote in the existing review.
5. Notice the difference. Their version is almost certainly shorter, kinder, and more specific. That gap is the skill.

Repeat once a week for a month. Reviewing other people's code is the fastest way to get better at writing your own.

Where This Lands in the Series

Chs 15-19 covered the operational layer: CI, lint, tests, PRs, review. Everything you need to ship and maintain a codebase with other humans. The next chapters open Part 3 — frameworks — starting with why "throw a script tag in HTML" stopped scaling and what React, Next, and friends actually do that vanilla JS doesn't.