# Pull requests & code review

> Open focused, well-described pull requests, respond to review, review others' code constructively, and merge with the right strategy.

Git & Collaboration roadmap · Everyday workflow · 18 min read · https://levelcode.org/roadmaps/git/pull-requests

On almost every team, code reaches `main` through a **pull request** (PR): a proposal to merge one branch into another, with a place for discussion, automated checks and approval. Git itself has no pull requests; they are a feature of hosting platforms like GitHub (GitLab calls them merge requests). This lesson covers the full life cycle on GitHub — preparing a branch, writing a PR people want to review, handling feedback, reviewing others' work, and choosing how to merge.

## What a pull request is

A PR compares two branches on the server:

- The **base** branch receives the changes, usually `main`.
- The **head** branch contains them, for example `feature/search`.

GitHub shows the commits and the combined diff of head against the point where it left base — the same thing you get locally with `git diff main...feature/search` (three dots: "changes on the branch since it diverged"). The PR is live: push more commits to the head branch and they appear in it automatically. Nothing reaches `main` until someone presses merge.

```text
main            A ── B ── E
                      \
feature/search         C ── D        ← PR #42: feature/search → main
```

Before the merge, people **discuss** and review the change, **automated checks** (tests, linters, builds) run against it, and repository rules decide whether it may be merged at all.

## Preparing a branch worth reviewing

**Start from an up-to-date base.** Create your branch from the latest `main` so the diff contains only your work:

```bash
git switch main
git pull
git switch -c feature/search-notes
```

**Keep it focused and small.** One PR should do one thing: a feature, a bug fix, or a refactor — not all three. Reviewers' attention drops sharply on large diffs; a few hundred changed lines is a sensible upper target. If a feature is big, land it as a sequence of smaller PRs (for example, the data model first, then the API, then the UI).

**Review your own diff first.** Before anyone else sees it, read exactly what you are proposing:

```bash
git fetch
git log --oneline origin/main..HEAD     # the commits in your PR
git diff --stat origin/main...HEAD      # files touched
git diff origin/main...HEAD             # every line
```

You will catch leftover debug logs and accidental file changes. Tidy messy commits with an interactive rebase, then push with an upstream:

```bash
git push -u origin feature/search-notes
```

## Opening the pull request

After a push, GitHub prints a link and shows a "Compare & pull request" banner on the repository page. Or create it from the terminal with GitHub's CLI:

```bash
gh pr create --base main --title "Add search to the notes list" --body-file pr-body.txt --reviewer alice
```

```text
Creating pull request for feature/search-notes into main in ada/notes-app

https://github.com/ada/notes-app/pull/42
```

Add `--draft` to open a **draft PR**: visible and able to run checks, but clearly marked as not ready, so nobody reviews it prematurely. Mark it ready when it is.

### Writing the description

The title should read like a good commit summary: specific and imperative. "Add search to the notes list" beats "Search stuff". The description gives the reviewer the context the diff cannot:

```text
What:
Adds a search box above the notes list that filters notes by title
and body as you type.

Why:
Users with large notebooks can't find anything (#118). This is the
smallest useful version; server-side search can follow if needed.

How to test:
1. Run `npm run dev` and create a few notes.
2. Type in the search box: the list filters immediately.
3. Clear the box: all notes return.

Notes for reviewers:
Filtering lives in `useNoteSearch.ts`. I debounced input by 150 ms;
happy to discuss the value.

Closes #118
```

Writing `Closes #118` (or `Fixes`, `Resolves`) links the issue and closes it automatically when the PR merges into the default branch. Screenshots or a short recording help enormously for UI changes. Many repositories provide a template at `.github/pull_request_template.md` that pre-fills this structure.

> [!TIP]
> Leave comments on your own PR at the tricky spots before requesting review ("This looks odd, but the API returns null for archived notes"). It answers questions before they are asked and makes reviews much faster.

## Checks, rules and updating the PR

Most teams configure **status checks**: CI workflows that run the tests and linters on every push to a PR, shown as green ticks or red crosses. With `gh` you can watch them from the terminal:

```bash
gh pr checks
```

Repository owners usually protect `main` with **branch protection rules** (or rulesets): require passing checks, require one or more approving reviews, forbid direct pushes and force pushes. If the merge button is disabled, the PR page tells you which rule is not satisfied yet.

### Responding to feedback

When a reviewer asks for changes, make them on the same branch and push; the PR updates itself. Two styles are common:

- **Add follow-up commits** ("Handle empty search query"). Reviewers can see exactly what changed since their last look. This is the simplest default, especially if the PR will be squash-merged.
- **Rewrite history cleanly**: commit with `git commit --fixup=<sha>`, then `git rebase -i --autosquash origin/main` and `git push --force-with-lease`. This keeps a tidy series, at the cost of making "what changed since my review" harder to see. Only do it on a branch that is yours alone.

Reply to every comment, even with "Done" or a short explanation of why you disagree. Then re-request review.

### Keeping up with main

If `main` moves on while your PR is open, you may need to bring the branch up to date, either because of conflicts or because a rule requires it. Either merge `main` in (`git merge origin/main`, then push — no force needed) or rebase (`git rebase origin/main`, then `git push --force-with-lease`). Follow your team's convention. GitHub's "Update branch" button does the same on the server.

## Reviewing someone else's pull request

An approval says "I have read this and I am comfortable with it reaching `main`", so take it seriously.

### What to look at, in order

1. **Purpose.** Read the description. Does the change solve the stated problem, and is it the right approach? Raise design concerns first, before polishing details that may be rewritten.
2. **Correctness.** Edge cases, error handling, empty and very large inputs, concurrency, security (untrusted input, secrets, permissions).
3. **Tests.** Is the new behaviour tested, and would the tests fail if the code were wrong?
4. **Readability.** Clear names, reasonable structure, comments where intent is not obvious.
5. **Style.** Last — and ideally left to formatters and linters entirely.

For anything non-trivial, run the code. Check out the PR locally in one command:

```bash
gh pr checkout 42
```

Without `gh`, GitHub exposes every PR as a ref you can fetch: `git fetch origin pull/42/head:pr-42` then `git switch pr-42`.

### Writing comments that help

Comment on specific lines, and make each comment's weight clear. Compare:

| Unhelpful | Helpful |
| --- | --- |
| "This is wrong." | "If `query` is only spaces this returns no notes. Should we `trim()` first?" |
| "Rename this." | "nit: `filtered` reads better than `res2` here; not blocking." |
| "Why didn't you use X?" | "Have you considered the existing `useDebounce` hook? It would remove lines 12–30." |

Ask questions rather than issue verdicts, explain the *why*, and label minor points `nit:`. GitHub also lets you attach a **suggested change** to a line comment (the "add a suggestion" button); the author can apply it with one click. Praise good work too — it tells people what to keep doing.

When you finish, submit the review as one of three types: **Comment** (feedback, no verdict), **Approve**, or **Request changes** (blocking until you re-review).

> [!NOTE]
> Review the code, not the person. "This function is hard to follow" is about code; "you write confusing code" is about a person. Teams with kind, specific, prompt reviews ship faster, because people stop being afraid to open PRs.

## Merging

GitHub offers three merge buttons. The repository settings decide which are allowed.

**Create a merge commit** — every commit from the branch plus a merge commit (like `git merge --no-ff`):

```text
A ── B ── E ──────── M   ← main
      \             /
       C ───────── D
```

**Squash and merge** — all the branch's changes become a single new commit on `main`:

```text
A ── B ── E ── S   ← main      (S contains C + D; C and D are not on main)
```

**Rebase and merge** — the branch's commits are replayed onto `main` individually, without a merge commit:

```text
A ── B ── E ── C' ── D'   ← main
```

Squash is popular because work-in-progress commits never reach `main` and each PR becomes one revertable commit. Merge commits preserve the full branch history. Rebase-and-merge gives linear history while keeping each commit, so it suits PRs whose commits were carefully curated. Which one your team uses is a workflow decision, discussed in the next lesson.

After merging, delete the branch (GitHub offers a button, or `gh pr merge 42 --squash --delete-branch`), and tidy up locally:

```bash
git switch main
git pull
git fetch --prune
git branch -d feature/search-notes
```

> [!WARNING]
> After a squash or rebase merge, `main` contains *new* commits, not your branch's originals, so `git branch -d` may refuse with "not fully merged". Once you have confirmed the PR merged on GitHub, `git branch -D` is safe.

## Practice

Take a change you would like to make to any repository you own on GitHub (or create a small one).

1. Create a branch from an up-to-date `main`, make a focused change in two or more commits, and review your own diff against `origin/main`.
2. Push it and open a **draft** PR with a What / Why / How to test description that closes an issue you created for it.
3. Leave one self-review comment, mark the PR ready, and merge it with "Squash and merge".
4. Clean up locally and confirm the branch is gone everywhere.

**Solution**

```bash
git switch main && git pull
git switch -c docs/install-steps
# ...edit README.md, commit twice...
git fetch
git log --oneline origin/main..HEAD
git diff origin/main...HEAD

git push -u origin docs/install-steps
gh issue create --title "README lacks install steps" --body "New contributors can't set up the project."
# suppose it was issue #7
gh pr create --draft --base main --title "Document installation steps" \
  --body "What: adds install steps to the README.

Why: new contributors could not get started.

How to test: follow the steps on a clean machine.

Closes #7"
```

Add the self-review comment on a line in the "Files changed" tab, then:

```bash
gh pr ready
gh pr merge --squash --delete-branch
git switch main
git pull
git fetch --prune
git branch -D docs/install-steps     # -D: the squash commit is new, so -d may refuse
git branch -a                        # the branch is gone locally and remotely
```

Issue #7 closes automatically because the description contained `Closes #7` and the PR merged into the default branch. (`gh pr merge --delete-branch` may already have deleted the local branch for you; then the `git branch -D` step just reports it is not found.)

## Key takeaways

- A PR proposes merging a head branch into a base branch, combining discussion, CI checks and approval rules.
- Keep PRs small and focused, start from an up-to-date `main`, and review your own diff before asking others.
- A good description explains what, why and how to test, and links issues with `Closes #n`.
- Review purpose and correctness before style; make comments specific, kind and clearly blocking or not.
- Merge commit, squash and rebase-and-merge produce different histories; squash creates one new commit, so delete the local branch with `-D`.

## Quiz

1. What does git diff main...feature (three dots) show, matching what a pull request displays?

   A. The changes made on feature since it diverged from main
   B. Every difference between the current tips of main and feature, in both directions
   C. Only the changes made on main since feature was created
   D. The list of commit messages on feature

   **Answer: A.** Three-dot diff compares the merge base with feature, so it shows only what the branch introduced. Two-dot (or plain) diff between the tips would also show main's newer changes as reversed differences.

2. Which line in a PR description automatically closes issue 118 when the PR is merged into the default branch?

   A. See issue 118
   B. @118 done
   C. Related: #118
   D. Closes #118

   **Answer: D.** GitHub recognises closing keywords such as Closes, Fixes and Resolves followed by an issue reference. A plain mention links the issue but does not close it.

3. A PR with commits C and D is merged with Squash and merge. What appears on main?

   A. C and D plus a merge commit
   B. C and D, replayed with new IDs
   C. A single new commit containing the combined changes of C and D
   D. Nothing until the branch is deleted

   **Answer: C.** Squash and merge creates one new commit with the combined diff. That is also why your local branch's original commits are not in main and git branch -d may refuse to delete it.

4. As a reviewer, which comment is most helpful?

   A. "This is wrong, please fix."
   B. "If items is empty, line 14 divides by zero. Could we return 0 early? Happy to pair on it."
   C. "I would have written this differently."
   D. "LGTM" without running or reading the tests

   **Answer: B.** Good review comments are specific, explain the problem and propose a direction. Vague verdicts and rubber-stamp approvals leave the author guessing or let bugs through.
