Code Review Best Practices

Overview

Code review is the primary mechanism by which knowledge spreads across a team, architectural decisions get a second opinion, and subtle bugs get caught before they ship. Done well, it is also the main vehicle for growing engineers — both the reviewer, who has to articulate the why behind a judgment, and the author, who receives targeted, specific feedback on real code they care about.

Done poorly, code review becomes a bottleneck, a source of anxiety, or a rubber-stamp exercise that adds delay without adding value. This page describes how we approach both sides of the review: what authors owe reviewers before the PR opens, and what reviewers owe authors in their feedback.

Code review assumes lint and type errors have already been resolved automatically. For the automated layer that runs before review begins, see Linting Standards and Pre-Commit Hooks.

Why It Matters

Bugs that escape to production are expensive. A reviewer catching a logical error in a PR costs minutes. The same bug caught post-deploy costs hours to diagnose, fix, re-deploy, and potentially communicate to customers.

Knowledge doesn't spread by osmosis. Without review, each engineer works in their own corner of the codebase indefinitely. Review forces cross-pollination: reviewers learn what's changing in areas they don't own, authors get exposed to patterns they haven't seen.

Architecture decisions need a second opinion. A PR is the right moment to ask "does this belong here?" or "what happens under load?" These are architectural questions that become prohibitively expensive to answer after merge.

Culture is set in review. The tone of code review — how feedback is phrased, whether it's personal or technical, whether it's collegial or adversarial — defines a significant part of engineering culture. It is worth being intentional about.

Standards & Best Practices

For authors

Keep PRs small and focused. A PR should do one thing. Small PRs are reviewed faster, reviewed better, and easier to revert if something goes wrong. As a rough guide: if your PR takes more than 30 minutes to review, it's probably too large. Split it.

Write a useful PR description. The description should answer: What does this change do? Why is this change being made? How can a reviewer verify it works? A description that is just a ticket link transfers the cognitive load to the reviewer. A description that explains the "why" makes the review faster and more accurate.

Flag what you want reviewed. If you're unsure about a specific approach, say so in the PR description or a comment. If part of the PR is mechanical (e.g. a rename across 40 files), note it so reviewers don't spend time on it. Reviewers will focus their attention where you direct them.

Respond to every comment. Even if the response is "done" or "disagree, here's why." Leaving comments unresolved signals that you haven't read them. Disagreements are fine — they should be resolved through discussion, not silence.

Don't take feedback personally. Feedback is on the code, not on you. If a comment feels like a personal criticism, flag it — that's a tone issue worth raising directly, not something to internalize.

For reviewers

Review the intent, not just the implementation. Ask yourself: does this PR solve the right problem? An implementation can be technically correct and still be the wrong solution. This is the most valuable question a reviewer can ask, and it requires understanding the context behind the change.

Be specific and actionable. "This is confusing" is not a useful comment. "This function handles both validation and persistence — consider splitting them so each is independently testable" is. Comments should either explain what to change, why to change it, or both.

Distinguish blocking issues from suggestions. Not all feedback should block a merge. Use explicit prefixes to signal intent:

Approve with outstanding nits. If the only remaining comments are non-blocking, approve the PR and note that the nits are optional. Don't leave PRs in a "changes requested" state over trivial style preferences — that creates unnecessary delay.

Praise good work explicitly. When you see something well-done — a clever simplification, a well-named abstraction, a thorough test — say so. Feedback is not only corrective. Positive signals reinforce good patterns and make review feel less adversarial.

Review promptly. A PR sitting unreviewed for days is a productivity blocker. Acknowledge the PR within one business day, even if a full review will take longer. "I'll get to this tomorrow" is a useful signal; silence is not.

Shared norms

Discuss architecture before the code is written. If a PR introduces a significant architectural decision — a new abstraction, a new dependency, a new data model — that decision should ideally be discussed before implementation. A design doc or a brief async message is cheaper than discovering a fundamental disagreement in a PR review.

Don't use review to enforce what tooling should enforce. If you're commenting on formatting, import order, or trailing commas, your linter should be catching that instead. Comments about things automated tooling should handle are noise. Configure the tooling; don't do its job manually in review.

The author has final say on nits. If a comment is explicitly non-blocking, the author can decline it without explanation. The reviewer proposed; the author decides.

How to Implement

Author checklist before requesting review

Before marking a PR as ready for review, the author should be able to answer "yes" to each of these:

• The PR description explains what the change does and why
• The diff is scoped to one concern (no unrelated changes bundled in)
• All CI checks pass
• I've tested the change myself (happy path + at least one edge case)
• I've re-read the diff top-to-bottom as if I were the reviewer
• I've left inline comments on any code I expect to be confusing

Reviewer checklist

When reviewing, work through these layers roughly in order (coarser to finer):

1. Intent — Does this PR solve the right problem? Is the approach correct?
2. Architecture — Does this fit the existing patterns? Are abstractions appropriate?
3. Correctness — Are there logic errors, edge cases, or missing error handling?
4. Tests — Is the behavior tested? Are the tests meaningful?
5. Readability — Are names clear? Is the code structured for the next reader?
6. Style — (If not caught by tooling) anything that violates team conventions

Stop at the first layer that has blocking issues. There's no point commenting on variable names if the entire approach needs rethinking.

Review turnaround SLA

When in doubt, communicate. "I've seen this, I'll review tomorrow morning" costs 10 seconds to write and eliminates hours of uncertainty.

Tools & Templates

PR description template

## What

[One paragraph: what does this change do?]

## Why

[One paragraph: why is this change being made? Link to ticket/issue if relevant.]

## How to verify

[Steps to test or observe the change manually.]

## Notes for reviewers

[Optional: areas of uncertainty, known trade-offs, what NOT to focus on.]

Review comment prefixes

blocking: the session token is stored in localStorage — this is vulnerable to XSS.
          Please move to an httpOnly cookie.

nit: `userList` could be `users` — the `List` suffix is redundant with the type.

suggestion: this could use Array.reduce instead of the manual accumulator,
            which would remove the mutation. Not required.

question: why is this guard needed here but not in the sibling function?

Common Pitfalls

The rubber-stamp review. Approving a PR without reading it because the author is senior, the change looks small, or you're in a hurry. This abdicates the responsibility of review and creates a false sense of safety. A 10-line change can introduce a subtle bug; a PR from a senior engineer can still have architectural problems.

Review as gatekeeping. Treating review as an opportunity to enforce your personal preferences or demonstrate knowledge. Review is a collaborative process aimed at shipping good code — not a performance of technical authority. Comments should serve the codebase, not the reviewer's ego.

Bikeshedding. Spending disproportionate review time on low-stakes style decisions (naming, formatting, comment phrasing) while glossing over correctness or architecture. If a comment is about taste rather than consequence, label it as a nit or skip it.

Long-lived PRs. A PR that sits in review for a week is a liability: it diverges from main, becomes harder to review, and loses its context. Reduce PR size, improve turnaround SLAs, and split large changes into sequential PRs.

Asynchronous blocking. Leaving a "changes requested" state and then going offline for 48 hours. If you request changes, be reachable to follow up. Blocking review without follow-through is worse than no review.

Conflating review with pair programming. Review is not the place to redesign a feature from scratch. If a PR needs fundamental rethinking, schedule a conversation rather than conducting an extended back-and-forth in comments. Comments are expensive context-switching for both parties.