Skip to content
LEARNREACTSAAS
Chapter 96Lesson 3

Designing a reviewable pull request

Designing the GitHub pull request as the team's unit of change, sized small, reviewable, and reversible, with descriptions and review conventions a teammate can act on.

So far you’ve opened pull requests as a formality: finish a branch, click merge. That’s the right instinct when you’re the only person who will ever read the code. The moment a second engineer joins, the pull request stops being your checkpoint and becomes the team’s unit of change: the object a teammate reviews, CI checks, and main’s history records as one line. Everyone downstream sees your change through it.

Picture the failure a careless one causes. A reviewer opens a nine-hundred-line diff, scrolls for a minute, can’t hold it in their head, types “LGTM,” and approves. The bug in line 600 ships. Nobody caught it because nobody could: the PR was un-reviewable, so it got rubber-stamped instead of reviewed.

The fix is not to review harder; it’s to design the pull request. Trunk-based Git told you to keep one PR per logical change, and the loop runs on the fixup commits and squash-merge from Reflog, bisect & rescue. It all hangs on three words: small, reviewable, reversible.

The pull request lifecycle

Section titled “The pull request lifecycle”

A pull request is not a single event; it’s a short pipeline of checkpoints, and the goal at every one is the same: land the change in one round of review. Each later section of this lesson lives at one of these stages, fixup commits at the discussion stage, CODEOWNERS at the reviewer-request stage.

Scrub through the lifecycle below. Each step highlights the active stage and names who does the work.

You push the finished branch: `git push -u origin feat/invoice-status`. The remote now has your commits, but nothing is proposed yet.
You open the PR against `main`, on the web or with `gh pr create`. This is where the diff and the description come together into a proposal.
Reviewers get requested — automatically, if a CODEOWNERS file maps the changed files to a team (covered later this lesson).
CI runs its checks against the PR's commits. Wiring CI to run on every PR is the next chapter's job.
A preview deployment of the branch goes live at its own URL, so reviewers click through the real feature. The mechanics arrive in a later chapter.
Reviewers leave comments; you push fixup commits in response. This is the negotiation, where most of the back-and-forth happens.
A reviewer approves. The proposal is now ratified — but not yet on `main`.
You squash-merge. Every commit on the branch collapses into one clean commit on `main`.
The merged branch is auto-deleted. The work is on `main`; the branch has served its purpose.

Most stages are a single command. Here is the bare happy path, push, propose, merge, in the GitHub CLI .

The happy path
git push -u origin feat/invoice-status
gh pr create --fill
gh pr merge --squash --delete-branch

Small, reviewable, reversible

Section titled “Small, reviewable, reversible”

These aren’t three nice-to-haves. They’re three constraints that reinforce each other, and most of the decisions in this lesson follow from them.

Small. Roughly four hundred changed lines is the ceiling for a review someone can do in one sitting. It isn’t a lint rule; it’s a fact about human attention. A reviewer can hold a few hundred lines in their head, trace each change to the code that calls it, and form a real opinion. Push past that and attention degrades; north of eight hundred lines the review turns into skimming, and the PR gets a thumbs-up nobody could have honestly given.

Reviewable. One logical change per PR, statable in a single sentence (“adds a status filter to the invoices list”) that covers every line in the diff. The drive-by edit breaks this: you’re fixing a bug, you notice the formatting is off, and “while I’m in here” you reformat the whole module. Now the diff is a bug fix wearing a hundred lines of whitespace churn, and the lines that need scrutiny are exactly the ones you’ve hidden.

Reversible. A single git revert (from Reflog, bisect & rescue) should undo the change cleanly, with nothing else coming along. This pays off weeks later, when something breaks in production and you need to back out just the change that caused it. If that PR also bundled two unrelated features, reverting it tears out two things that were working fine.

The three chain together. Small enables reviewable: a small diff is almost forced to be about one thing. Reviewable enables reversible: one logical change is, by construction, one thing you can revert. They aren’t goals you balance; they’re a single property seen from three angles. This is where junior and senior diverge. The junior optimizes for fewer PRs, because batching feels efficient; the senior optimizes for each PR fitting in one head, and ships more, smaller PRs as a result.

The figure makes “small” concrete: review quality degrades gradually as the diff grows, which is why four hundred is a heuristic and not a gate.

Line count is a proxy for the attention a reviewer can give. As the diff grows, review quality slides from real to ceremonial.

Splitting a big change into a stack

Section titled “Splitting a big change into a stack”

Some changes are genuinely big. Renaming a core type across the codebase, or shipping a feature that needs a refactor and a new helper and the user-facing wiring, won’t fit in a hundred lines. The senior move isn’t to abandon small; it’s to split the big change into a stack of small PRs.

A stack is a dependency chain: each PR builds on the branch of the one before it instead of on main, and each link is small enough to review on its own. Say adding a status filter to the invoices list means reshaping the query first. You’d ship it as a chain of four: a pure refactor with no behavior change, then a small extraction, then the call-site swap, then the feature itself.

One big change as a stack of four
PR 1  feat/filter-refactor   Reshape the invoice query (no behavior change)
PR 2  feat/filter-helper     Extract a reusable status-filter helper
PR 3  feat/filter-callsite   Use the helper where the list is built
PR 4  feat/filter-ui         Add the status dropdown to the list UI

Each link reviews in one sitting and merges in order: PR 1 first, then PR 2 once PR 1 is in. The pure-refactor PR is the most valuable alone, because a reviewer can approve it fast precisely because it changes no behavior, confidence you only get when it isn’t tangled with the feature.

The mechanic is one flag. Instead of opening a PR against main, point it at the previous branch:

Terminal window
git switch -c feat/filter-helper
gh pr create --base feat/filter-refactor

--base tells GitHub the PR merges into feat/filter-refactor, not main, so the diff shows only this link’s changes rather than the whole stack. The cost is bookkeeping: when PR 1 merges, you re-point PR 2’s base to main, and so on down the chain. Tools like Graphite, git-spice, and Sapling automate that re-pointing, but reach for them only once you’re routinely juggling three or more dependent PRs. Below that, chaining --base by hand is fine.

What a good PR description argues

Section titled “What a good PR description argues”

A diff is a perfect record of what changed and a useless record of why. It shows every line that moved and not one word about the bug behind it, the approach you rejected, or the part you want looked at hardest. Leave the description blank and the reviewer must reverse-engineer your intent from the code before judging whether the code matches it. That is slow, and it is where reviewers miss things: their attention goes to reconstructing what you meant, leaving none for whether you’re right.

So treat a pull request as a proposal with an argument attached: the diff is the proposal, the what you want to merge; the description is the argument, the why it’s correct and safe. A blank description asks the reviewer to build that argument for you before they can judge it.

A good argument has six sections. Read it once as a whole, the way a reviewer would, then walk it section by section.

## What
Adds a status filter to the invoices list. Users can now narrow the
list to Draft, Sent, Paid, or Archived invoices from a dropdown above
the table.


## Why
Closes INV-412. Support has been fielding "where did my paid invoices
go" tickets because the list mixes every status together with no way
to focus. This is the smallest fix that addresses the complaint.


## How
The filter is driven by a URL search param (`?status=`) so a filtered
view is shareable and survives a refresh. I considered client-side
filtering of the full list but rejected it — the list is paginated
server-side, so filtering on the client would silently miss rows on
other pages.


## Test plan
- Added a unit test covering each status case and the "all" default.
- Manually filtered to each status against seeded data; confirmed
  counts match the badge totals.
- Checked an empty result (no archived invoices) renders the empty
  state, not a blank table.


## Screenshots
Before / after of the list with the new dropdown. (Once preview
deployments are wired, a link to the live preview goes here instead.)


## Risks / rollback
Read-only change — no schema or data migration. If the filter
misbehaves, a single `git revert` removes the dropdown and restores
the unfiltered list with no cleanup.

One paragraph on the user-visible change, checked against by everything below. State what a user can now do, not how the code does it.

## What
Adds a status filter to the invoices list. Users can now narrow the
list to Draft, Sent, Paid, or Archived invoices from a dropdown above
the table.


## Why
Closes INV-412. Support has been fielding "where did my paid invoices
go" tickets because the list mixes every status together with no way
to focus. This is the smallest fix that addresses the complaint.


## How
The filter is driven by a URL search param (`?status=`) so a filtered
view is shareable and survives a refresh. I considered client-side
filtering of the full list but rejected it — the list is paginated
server-side, so filtering on the client would silently miss rows on
other pages.


## Test plan
- Added a unit test covering each status case and the "all" default.
- Manually filtered to each status against seeded data; confirmed
  counts match the badge totals.
- Checked an empty result (no archived invoices) renders the empty
  state, not a blank table.


## Screenshots
Before / after of the list with the new dropdown. (Once preview
deployments are wired, a link to the live preview goes here instead.)


## Risks / rollback
Read-only change — no schema or data migration. If the filter
misbehaves, a single `git revert` removes the dropdown and restores
the unfiltered list with no cleanup.

Link the ticket and state the problem solved. The diff can never contain this, and it’s what lets a reviewer judge whether the change is the right thing to build at all.

## What
Adds a status filter to the invoices list. Users can now narrow the
list to Draft, Sent, Paid, or Archived invoices from a dropdown above
the table.


## Why
Closes INV-412. Support has been fielding "where did my paid invoices
go" tickets because the list mixes every status together with no way
to focus. This is the smallest fix that addresses the complaint.


## How
The filter is driven by a URL search param (`?status=`) so a filtered
view is shareable and survives a refresh. I considered client-side
filtering of the full list but rejected it — the list is paginated
server-side, so filtering on the client would silently miss rows on
other pages.


## Test plan
- Added a unit test covering each status case and the "all" default.
- Manually filtered to each status against seeded data; confirmed
  counts match the badge totals.
- Checked an empty result (no archived invoices) renders the empty
  state, not a blank table.


## Screenshots
Before / after of the list with the new dropdown. (Once preview
deployments are wired, a link to the live preview goes here instead.)


## Risks / rollback
Read-only change — no schema or data migration. If the filter
misbehaves, a single `git revert` removes the dropdown and restores
the unfiltered list with no cleanup.

Only the non-obvious choices and the alternatives you rejected; skip what the diff already makes plain. The rejected alternative is often the most valuable line here.

## What
Adds a status filter to the invoices list. Users can now narrow the
list to Draft, Sent, Paid, or Archived invoices from a dropdown above
the table.


## Why
Closes INV-412. Support has been fielding "where did my paid invoices
go" tickets because the list mixes every status together with no way
to focus. This is the smallest fix that addresses the complaint.


## How
The filter is driven by a URL search param (`?status=`) so a filtered
view is shareable and survives a refresh. I considered client-side
filtering of the full list but rejected it — the list is paginated
server-side, so filtering on the client would silently miss rows on
other pages.


## Test plan
- Added a unit test covering each status case and the "all" default.
- Manually filtered to each status against seeded data; confirmed
  counts match the badge totals.
- Checked an empty result (no archived invoices) renders the empty
  state, not a blank table.


## Screenshots
Before / after of the list with the new dropdown. (Once preview
deployments are wired, a link to the live preview goes here instead.)


## Risks / rollback
Read-only change — no schema or data migration. If the filter
misbehaves, a single `git revert` removes the dropdown and restores
the unfiltered list with no cleanup.

What you actually ran, not what you intend to run: the seeded test, the manual flow, the edge case. This is your evidence that the change works.

## What
Adds a status filter to the invoices list. Users can now narrow the
list to Draft, Sent, Paid, or Archived invoices from a dropdown above
the table.


## Why
Closes INV-412. Support has been fielding "where did my paid invoices
go" tickets because the list mixes every status together with no way
to focus. This is the smallest fix that addresses the complaint.


## How
The filter is driven by a URL search param (`?status=`) so a filtered
view is shareable and survives a refresh. I considered client-side
filtering of the full list but rejected it — the list is paginated
server-side, so filtering on the client would silently miss rows on
other pages.


## Test plan
- Added a unit test covering each status case and the "all" default.
- Manually filtered to each status against seeded data; confirmed
  counts match the badge totals.
- Checked an empty result (no archived invoices) renders the empty
  state, not a blank table.


## Screenshots
Before / after of the list with the new dropdown. (Once preview
deployments are wired, a link to the live preview goes here instead.)


## Risks / rollback
Read-only change — no schema or data migration. If the filter
misbehaves, a single `git revert` removes the dropdown and restores
the unfiltered list with no cleanup.

For any UI change, before/after images so the reviewer sees the result without checking out the branch.

## What
Adds a status filter to the invoices list. Users can now narrow the
list to Draft, Sent, Paid, or Archived invoices from a dropdown above
the table.


## Why
Closes INV-412. Support has been fielding "where did my paid invoices
go" tickets because the list mixes every status together with no way
to focus. This is the smallest fix that addresses the complaint.


## How
The filter is driven by a URL search param (`?status=`) so a filtered
view is shareable and survives a refresh. I considered client-side
filtering of the full list but rejected it — the list is paginated
server-side, so filtering on the client would silently miss rows on
other pages.


## Test plan
- Added a unit test covering each status case and the "all" default.
- Manually filtered to each status against seeded data; confirmed
  counts match the badge totals.
- Checked an empty result (no archived invoices) renders the empty
  state, not a blank table.


## Screenshots
Before / after of the list with the new dropdown. (Once preview
deployments are wired, a link to the live preview goes here instead.)


## Risks / rollback
Read-only change — no schema or data migration. If the filter
misbehaves, a single `git revert` removes the dropdown and restores
the unfiltered list with no cleanup.

What could break and how to undo it, often a git revert. A clear rollback is one a reviewer can approve with less fear.

1 / 1

You won’t type those six headings every time. The repo ships a .github/pull_request_template.md that pre-fills the box with this skeleton the moment you open a PR, making the good structure the path of least resistance. A later chapter covers authoring it.

One point of senior judgment: resist bolting a checkbox onto that template, the ”☐ I ran the tests” kind. Anyone can tick it in two seconds without running anything, so it looks like a gate but enforces nothing. The CI status check in the next chapter enforces that fact. Reserve the description for the argument; let the machine enforce the facts.

Review your own PR first

Section titled “Review your own PR first”

Before you request a reviewer, open your own PR’s “Files changed” tab and read the diff as if it were a stranger’s. Reading to approve surfaces things that reading while writing never does: the debug console.log you forgot, the variable you renamed everywhere but one place, the test you meant to add. In about a minute you’ll catch roughly half the comments a reviewer would have left.

Where the diff needs context, leave the comment yourself. A line like “intentionally not handling the archived case here; it’s a separate ticket, INV-419” pre-answers the reviewer’s exact question and saves a round-trip. Self-review is the author’s last step, after the description and before you request review.

When to open a draft PR

Section titled “When to open a draft PR”

GitHub lets you open a PR in draft status: a “Draft” badge, no merge button, a signal that you want eyes, not approval. The trade-off is attention. A normal PR lands in reviewers’ queues and gets picked up; people learn that drafts aren’t ready and skip them, so a draft opened “just to be safe” can sit ignored for days. Open one only when one of these is true:

  • The approach is uncertain, and a reviewer glancing at the direction now could save you a day of building the wrong thing.
  • The PR depends on another that hasn’t merged yet, and you want it visible as in-progress without inviting an approval it can’t act on.
  • You’re using CI as an early-warning signal, pushing to watch the checks run, and reviewers genuinely shouldn’t read it yet.

Otherwise, open a normal PR. Opening a draft is one line:

Terminal window
gh pr create --draft

Review from both sides: author and reviewer

Section titled “Review from both sides: author and reviewer”

Review is a conversation, and you sit on both sides of it constantly: author one hour, reviewer the next. Each seat needs enough working vocabulary to take part. The deep methodology, severity levels and a layered review stack, is a later chapter’s subject; here we want fluency in the everyday exchange.

Four comment styles that keep a review moving

Section titled “Four comment styles that keep a review moving”

A reviewer who knows only one kind of comment, “change this,” creates friction on every line, because not every observation is a demand. Four shapes do most of the work:

  • Suggestion. A suggestion code fence renders as a one-click “apply” button on the author’s side. Reach for it on the small stuff, a typo, a clearer name, a tiny refactor, where writing the fix is faster than describing it.
  • Question. “Why this approach and not that one?” A real question is valuable when the reviewer is missing context the author has. It becomes a problem as a demand in disguise: “have you considered not doing it this way?” is an instruction wearing a question mark, and everyone can tell.
  • Blocking comment. “This has to change before I approve.” Make it visible rather than bury it in a thread: attach it to the Request changes review action, which flips the PR’s state to “changes requested” for everyone to see. (Turning that state into a hard merge block takes a ruleset , the next lesson’s job; here it’s the signal that matters.)
  • Nit. A comment you mark as non-blocking: “nit: I’d inline this, but take it or leave it.” It says you noticed and you care about the bar, but you won’t hold the PR hostage over style. Naming a nit as a nit keeps it from reading as a demand.

The suggestion fence is the one bit of syntax here: a fenced block with the word suggestion, holding the exact replacement for the lines you’re commenting on.

```suggestion
const statuses = ['draft', 'sent', 'paid', 'archived'] as const;
```

As the author, you have one habit: respond to every comment, even if the reply is just “good catch, fixed” or “deferring this to issue #418.” A comment with no reply reads as dismissal, because the reviewer can’t tell whether you disagreed, missed it, or silently complied.

The 60-second pass and the 30-minute pass

Section titled “The 60-second pass and the 30-minute pass”

As the reviewer, you work in two gears. New reviewers do one undifferentiated pass: a slow read of a PR that turns out to be dead on arrival, or a fast skim of one that deserved real scrutiny. Run two passes instead, each with its own job.

The 60-second pass is triage. Read the description, scan the diff, and flag only what makes the PR dead on arrival: no tests where there obviously should be some, a missing migration, a committed secret, a description that says “see commits.” This bounces a not-ready PR in a minute instead of consuming thirty.

The 30-minute pass is the actual review. Re-read with full attention, follow each change to the code that calls it, open the preview deployment and click through the real feature, and then leave your review. The 60-second pass decides whether the PR earns the 30-minute pass, where the real findings come from.

Exercise: be the reviewer

Section titled “Exercise: be the reviewer”

The fastest way to learn what makes a diff reviewable is to be the reviewer. The exercise below is a small feat/invoice-status PR with three planted defects, each one this lesson primed you to catch. Open each file, click the lines that deserve a comment, and write what you’d say. Submit your review to see which of the three you caught.

You're reviewing a teammate's PR that adds a status filter to the invoices list. Leave a comment on every line that deserves one. Click any line to leave a review comment, then press Submit review.

src/db/queries/list-invoices.ts
import { and, asc, eq } from 'drizzle-orm';
import { eq, and, asc } from 'drizzle-orm';
import { db } from '@/db';
import { invoices } from '@/db/schema';


const FILTERABLE_STATUSES = ['draft', 'sent', 'paid'];


export async function listInvoices(orgId: string, status?: string) {
  const where = [eq(invoices.orgId, orgId)];
  if (status && FILTERABLE_STATUSES.includes(status)) {
    where.push(eq(invoices.status, status));
  }
  return db
    .select()
    .from(invoices)
    .where(and(...where))
    .orderBy(asc(invoices.createdAt));
}

The everyday loop: fixup commits and squash-merge

Section titled “The everyday loop: fixup commits and squash-merge”

The review cycle is where the previous two lessons pay off: a reviewer comments, you fix, you push, they re-review and approve, you squash-merge. The only choice is how you commit each fix.

Terminal window
git commit -m "address review"
git push

Works fine. The squash-merge absorbs this commit, so main ends up clean either way. The cost is during review: “address review” says nothing, and the fix floats free of the commit it corrects, so the open PR’s history reads as noise.

Squash-merge collapses both to the identical commit on main, so the difference is purely about legibility while the PR is open: --fixup keeps the intent of each fix visible during review; the plain commit doesn’t.

The reviewer doesn’t re-read the whole diff after each push, either. GitHub’s “changes since your last review” filter on the Files changed tab shows only the commits added since they last looked, so a five-line fix is a five-line re-review.

This is why Trunk-based Git made squash-merge the default and Reflog, bisect & rescue taught --fixup. Review sees every incremental change, the work-in-progress commits and fixups, which is what a reviewer needs to follow your reasoning; the squash collapses all of it into one commit, so the messy history never reaches main, while the PR page keeps the full conversation for anyone who later asks why a line is there.

gh, CODEOWNERS, and the squash-merge setting

Section titled “gh, CODEOWNERS, and the squash-merge setting”

So far the workflow runs on habit and judgment. A few tools and repo settings turn that judgment into something the repo enforces. Three are worth knowing now; the rules that lock them down are the next lesson’s job.

The gh CLI, one line per command

Section titled “The gh CLI, one line per command”

Our default for opening and reviewing PRs is the GitHub web UI, where the diff, the conversation, and the merge button all live. The gh CLI is faster once the commands are in your fingers, and it composes with shell aliases and scripts. Treat this as a recognition list: reach for these when clicking feels slow.

gh — the commands you'll actually use
gh pr create          # open a PR from the current branch
gh pr view --web      # open the current branch's PR in the browser
gh pr checkout 123    # check out a teammate's PR locally to test it
gh pr review          # approve, comment, or request changes
gh pr merge --squash --delete-branch   # squash-merge and clean up the branch

CODEOWNERS routes reviewers automatically

Section titled “CODEOWNERS routes reviewers automatically”

When changes to the billing code should always reach the billing team, you don’t want that to depend on whoever opened the PR remembering to add them. A CODEOWNERS file handles it: when a PR touches an owned path, GitHub automatically requests a review from the matching owner.

.github/CODEOWNERS
# Default owner for everything in the repo
*                    @org/eng


# Money paths get the billing leads on every change
/src/lib/billing/    @org/billing-leads


# Schema changes go through a database owner
/src/db/schema.ts    @org/dba

The file is data, not enforcement. On its own it only auto-requests the right reviewers; nothing blocks a merge if the owner never looks. Turning “requested” into “required” takes a ruleset, the next lesson’s topic.

The globs are gitignore-style, and the last matching line wins, not the most specific. So write the file general-to-specific, the catch-all * first and narrow paths below it. A broad pattern placed after a narrow one silently overrides it, the most common way a CODEOWNERS file goes wrong. Reserve owners for high-stakes zones such as auth, billing, schema, and infra; ordinary feature work doesn’t need the ceremony.

Squash-and-merge as the repo’s only merge button

Section titled “Squash-and-merge as the repo’s only merge button”

Trunk-based Git made squash-merge a discipline. One repo setting makes it structural. In Settings → General → Pull Requests, enable Squash and merge and turn off both Allow merge commits and Allow rebase merging. The merge button now offers one choice, so main gets exactly one commit per PR, with no “I picked merge-commit just this once” exceptions. Pair it with Automatically delete head branches so merged branches don’t pile up.

Two more settings are worth knowing by name, though you won’t enable them by default:

  • Allow auto-merge queues a PR to merge itself the moment its checks pass and its reviews land, so you don’t babysit the button through a slow CI run. Reach for it once you trust your CI.
  • Merge queue serializes merges, rebasing and re-checking each PR against the latest main before it lands, which prevents the race where two PRs pass CI alone but break main together. It earns its keep on larger teams; below a certain volume it’s just overhead, so it’s outside the 2026 startup minimum.

Where the PR fits from here

Section titled “Where the PR fits from here”

The pull request is now the team’s unit of change: small, reviewable, reversible, argued for in its description, negotiated in review, and squashed into one line of main’s history. Most of what follows builds on that object. The next chapter makes CI a required gate on every PR and the next lesson enforces these habits with rulesets; later chapters attach a preview deployment to each PR, ship schema migrations as small PRs, and cover the full review methodology.

GitHub Docs — About pull requests
docs.github.com

The canonical reference for the PR lifecycle, drafts, and review states.
GitHub Docs — About code owners
docs.github.com

CODEOWNERS syntax, glob rules, and the last-match-wins ordering in full.
GitHub CLI manual
cli.github.com

Every gh command and flag, including the pr subcommands from this lesson.

External resources

Section titled “External resources”

The references below go deeper on the three pillars of this lesson: sizing, stacking, and review etiquette.

Google Eng Practices — Small CLs
google.github.io

Google's canonical case for one-logical-change reviews — the doctrine behind 'small, reviewable, reversible.'
The Pragmatic Engineer — Stacked Diffs
newsletter.pragmaticengineer.com

Why Meta and Google ship features as stacks of small PRs, and the tooling that automates the chain.
Conventional Comments
conventionalcomments.org

A tiny spec for labelling review comments — nit, suggestion, issue, question — so intent is never ambiguous.