Code Review

On a small team, review is quality control. Nobody else is checking. Take it seriously — on both sides.

Why we review

• Catch bugs early — cheapest place to catch them is before merge.
• Share context — the second person who reads the code owns it too.
• Raise the floor — juniors learn patterns, seniors stay in touch with the code.
• Slow the blast radius — two people thought about this before it shipped.

Not for: gatekeeping, ego, bikeshedding style the tool would have caught.

As the author

Before you request review

• Self-reviewed on GitHub (not just in your editor). Read the diff as if someone else wrote it.
• CI green.
• Rebased on latest main.
• Description explains why, not just what. See § Git — PR template.
• Screenshots / recordings for UI changes.
• Obvious follow-ups noted (// TODO: ... with a ticket, not orphaned).

Size it right

• < 400 LoC diff ideal.
• Bigger → reviewer reads it shallower. Split or feature-flag.
• A 2,000-line PR gets rubber-stamped. That’s on you, not the reviewer.

Ask for the review you need

• Draft PR if you want early eyes before it’s done. Say so in the description.
• “Design review” — you want feedback on the approach, not line-by-line. Say so.
• “Ship review” — final pass, looking for merge. Default.
• Request specific people, not a channel.

Receiving feedback

• Assume good intent. Reviewers are trying to help. If a comment lands wrong, it’s almost always tone, not hostility.
• Don’t take it personally. Your code is not you.
• Respond to every comment, even if just with 👍 or “done”. Silent commits leave the reviewer guessing.
• Resolve your own conversations once addressed. The reviewer re-opens if they disagree.
• Disagree and commit: argue your case once, politely, with evidence. If the reviewer still disagrees and they’re the expert here, ship their way.
• Push back on bad review too. “I’ll change it” isn’t always right. “I left it because X, happy to change if you feel strongly” is a fine reply.

Don’t

• Force-push over a review mid-way without warning — reviewers lose context. Add a comment: “rebased on main, pushed fix for your comment on X.”
• Argue style the linter already accepts.
• Merge with unresolved comments without acknowledging them.

As the reviewer

What to look for (in order)

1. Is it correct? Does it do what it says? Edge cases considered?
2. Is it safe? Any security, PII, secrets, SQL injection, auth holes?
3. Does it match our defaults? See § Stack Selection, § Architecture. Flag deviations — they might be valid, but they need explaining.
4. Are there tests? See § Testing. No test files = block merge.
5. Is it clear? Would someone six months from now understand why? Names, structure, comments for non-obvious whys.
6. Is it the right size? If it does three unrelated things, ask to split.
7. Is it simpler than alternatives? Too abstract? Too clever? Premature optimization?

Style the linter handles: don’t comment on. Style the linter doesn’t handle but you care about: fine, but mark as nit: so the author knows it’s optional.

How to write a review comment

• Be specific. “This is weird” → “This runs the DB query in a loop; pull it out before the loop.”
• Suggest, don’t demand. “How about…” lands better than “Don’t do this.”
• Explain why. “Doesn’t work with RLS enabled” beats “Change this.”
• Use suggestions (GitHub’s suggestion blocks) for small fixes — author applies with a click.
• Tag severity when it’s not obvious:
  • blocker: — must change before merge.
  • nit: — optional, not blocking.
  • q: — just a question, not a request.

Ship or stall — the three verdicts

GitHub offers Approve / Request Changes / Comment. Our convention:

Don’t sit on a PR. If you started a review, finish it within a business day unless you ping the author saying otherwise. A stale review is worse than no review — the author is blocked and doesn’t know why.

Review pace for a small team

• Default response time: within 1 business day.
• Critical path (blocks a teammate) — same-day.
• Urgent hotfix — within an hour if you’re free, otherwise pass to someone who is.

The review mindset

• You’re not writing this PR. Don’t rewrite it in your head. If you disagree on approach, say so, but don’t nit the implementation of an approach you don’t like — talk about the approach first.
• Compliments are fine. “Nice, this is cleaner than the old version” costs nothing and matters.
• If you’re tired, don’t review. A bad review is worse than a late one.

Review checklist (auto-blockers)

Block merge if any of these are true:

• No tests added or modified for the change. See § Testing.
• Secrets or credentials in the diff.
• console.log or print() left in production code paths.
• New any in TypeScript without a comment explaining why.
• body: any or untyped input on an API boundary.
• SQL constructed by string concatenation.
• A new dependency added without rationale in the PR description.
• A migration that’s not reversible without manual steps.

Anything else is a judgment call.

Special cases

Reviewing a senior’s PR as a junior

You’re still expected to review. Ask questions — they’re how you learn.

• “Can you explain why this is Server Actions rather than an API route? I would’ve done the latter” — totally fair.
• If something looks wrong, say so. Seniors make mistakes; they’d rather you catch them.

Reviewing a junior’s PR

• Be kind. Be specific. Explain the why so they learn the pattern, not just the fix.
• Don’t accept a PR with major issues just to “not discourage them.” You’re letting bad code land.
• Pair-review at the desk for complex PRs if Teams is too async.

Reviewing the reviewer

If a review comment seems wrong, push back once, with evidence. If the reviewer holds their position and they’re the expert — ship their way. If you both keep disagreeing, pull in a third person. Don’t litigate in comments for three days.

Reviewing your own PR (merging without review)

Don’t. Branch protection prevents it. If you genuinely need to ship without review (outage hotfix, reviewer all on PTO):

1. Ship.
2. Open a follow-up “review-only” PR against the change after the fact.
3. Get sign-off retroactively.

Common anti-patterns

• “LGTM” without reading. If you didn’t open the files, don’t approve. You’re co-owner of what ships.
• Endless nit review. Approve with nits. Don’t hold a PR hostage for tab vs space.
• Style argument debates. If the linter allows it, stop. Change the linter config in a separate PR if it matters.
• Scope creep in review. “While you’re here, could you also refactor X?” — no. That’s a follow-up.
• Approve-and-forget. If you approved, you share ownership. If it breaks prod, it’s partly on you.