Code Review

Introduction

Code review is reading a teammate's diff before it merges—catching bugs, sharing knowledge, and keeping style consistent. On GitHub this happens in Pull Requests. This chapter covers what reviewers look for, how authors prepare diffs, and why small PRs plus green CI make review humane.

Prerequisites

Why Review?

Benefit	Example
Catch defects	Off-by-one, null handling
Spread context	New hire learns auth module
Enforce standards	Tests required, no secrets
Shared ownership	Whole team knows `main`

Review is not gatekeeping—it is quality and learning.

Reviewer Checklist

Correctness

Does change match PR description?
Edge cases handled?
Errors surfaced to user or logs appropriately?

Tests and CI

Tests added or updated for behavior change?
CI pipeline green (unit, lint, build)?

Security

No credentials, tokens, or private URLs
User input validated; SQL/command injection considered

Maintainability

Names clear; functions focused
No dead code or debug console.log
Diff size reasonable

Docs

README or API docs updated if behavior changed?

Tip

Review the Diff, Not the Person

Ask questions: "Could this throw if user is null?" not "You always miss null checks."

Author Habits Before Requesting Review

bash

git diff main...HEAD
git log main..HEAD --oneline

Self-check:

PR title matches Conventional Commit style if team uses it
Test plan filled in
Screenshots for UI changes
Rebased or merged latest main—no surprise conflicts

Push early Draft PR for feedback on direction before polishing.

Reading a PR Diff on GitHub

Files changed tab—line comments on specific hunks
Review changes → Comment / Approve / Request changes
Suggestion button inserts committable patch for trivial fixes

Local checkout of PR branch (GitHub CLI optional):

bash

gh pr checkout 42
npm test

Without CLI: fetch branch name from PR and git fetch origin pull/42/head:pr-42.

Approval and Merge Policy

Typical rules on protected main:

At least one approval
CI must pass
No request changes outstanding
Author merges or maintainer merges per team rule

Do not approve what you did not read—LGTM without reading hurts trust.

When to Split or Send Back

Request changes when:

Scope creep (unrelated refactor in same PR)
Missing tests for risky logic
Breaking change without discussion

Prefer follow-up PR for nice-to-have refactors noted in review.

FAQ

How fast should review happen?

Team SLA varies—24 hours common; ping after that politely.

Nitpick comments?

Mark nit: for optional style—author may defer to follow-up.

Review own code?

Self-review diff first; second pair of eyes still valuable.

Large legacy PR?

Ask author to split; review file-by-file if unavoidable.

Approve with minor nits?

Some teams approve with "non-blocking nits"—document team norm.

Review docs-only PR?

Check accuracy, links, and tone—typos matter for learning sites.