PR authors: small PRs, clear descriptions, self-review first Intermediate

This page answers: what the author does so a PR gets reviewed quickly and thoroughly. Most of a review’s quality is decided before the reviewer opens the PR.

Loading diagram…

1. Small, single-purpose PRs

Google Small CLs: small PRs get reviewed faster and more thoroughly, have fewer bugs, and are easier to roll back.

• Reference points: “100 lines is usually a reasonable size for a CL, and 1000 lines is usually too large”.
• File count matters too: 200 lines in one file can be fine; spread across 50 files it’s usually too big.

How to split a large feature, e.g., “partial refunds”:

Approach	Example
Stacking	PR 1: refunds table + repository. PR 2 (based on PR 1): RefundService. PR 3: endpoint
By layer (horizontal)	API contract + stub first, so the frontend can work in parallel
Vertical slices	Card refunds first, wallets later; each goes all the way from API to DB
Behind a feature flag	Merge gradually into main, turn on the flag when complete

SmartBear’s study at Cisco (a review-tool vendor, data from one team) recommends reviewing no more than 200–400 lines at a time. The density of defects found drops clearly when reading faster than 500 lines/hour.

Treat that as a directional signal, not a standard. But it matches experience: a 2,000-line PR doesn’t get reviewed, it gets skimmed.

2. Separate refactors from behavior changes

Google: “do refactorings in a separate CL from feature changes or bug fixes”; small local variable renames can stay together.

# Before — one PR, the reviewer must separate 600 lines of moves from 40 lines of new logic
feat(refund): support partial refunds     +640 −410

# After — two PRs, one question each
refactor(payment): extract PaymentGateway from OrderService   +380 −370   # old tests pass, tests unchanged
feat(refund): support partial refunds                          +260 −40    # only new logic + tests

For a refactor PR, the reviewer only asks “does it change behavior?” — old tests passing without changes is the evidence. See also Clean code and Git.

3. PR description: why, how it was tested, risks, rollback

Google: “Fix bug” is an inadequate description — which bug, fixed how?

• First line: an imperative sentence saying what the PR does.
• Body: the why and the context the diff can’t show.

A PR description that only says 'fixed'

✕ Bad description

Fix bug refund

✓ Good description

Prevent refunding more than was collected when two requests run concurrently

Two refund requests both read refunded = 0 and both write (INC-231, lost 2.1 million VND).
Switch to a conditional UPDATE + rowCount check, add a CHECK constraint.
Tests: integration test with 2 concurrent requests on real PostgreSQL.
Rollback: revert the PR; the constraint is additive, no down migration needed.

Why

A reviewer reading the bad version has to guess what the bug is and read the whole diff to learn the risks. Six months later, git log only shows “Fix bug refund” — nobody knows why that CHECK constraint exists.

Put the template at .github/pull_request_template.md (GitHub only applies it once the file is on the default branch):

## Why
<!-- The business or technical problem. Link the ticket/incident. -->
Customers wanting a partial refund must wait for CS to do it by hand (~120 tickets/week, PAY-512).

## What changed
<!-- Approach, design decisions, rejected options and why. -->
- Add `RefundService.refundPartial()`; the total refunded is capped by a CHECK constraint.
- Chose not to call the payment gateway inside the transaction; use an outbox.

## Where to start reading
<!-- Suggested order for the reviewer. -->
`refund.service.ts` → `refund.service.spec.ts` → migration.

## How it was tested
- Unit tests for 6 rounding cases; integration tests with real PostgreSQL.
- Manual run on staging: refunded 30% of a 199,000 VND order, reconciliation matches.

## Risks and rollback
- Risk: refunding more than collected when 2 requests run concurrently → blocked by `FOR UPDATE` + CHECK.
- Rollback: turn off the `refund.partial` flag. The migration only adds a table (expand), no down needed.

## Out of scope
- Multi-currency refunds: a later PR.

The “Out of scope” section saves the most comments: reviewers don’t have to ask “what about case X?”.

4. Self-review before requesting

Open the “Files changed” tab and read the diff like a reviewer. This self-review usually catches:

• Leftover console.log, an .env.local file, tests with .only.
• Commented-out code, TODOs without tickets.

Pre-annotate tricky spots with comments: “Deliberately no retry here because…” — cheaper than the reviewer asking. Before merging, reread the PR description: PRs often change during review, and the old description easily becomes wrong.

Author checklist

1. One purpose per PR; refactors and behavior changes in separate PRs (or at least separate commits).
2. Description has: why, what changed, where to start reading, how it was tested, risks, rollback, out of scope.
3. Self-read the diff: no debug logs, .only, .env files, commented-out code, TODOs without tickets.
4. CI green before requesting review; open as a draft while not ready.
5. New logic has tests; tests fail when the logic is inverted.
6. Migrations and API changes follow expand-contract; rollback is possible without a down migration.
7. Reply to every comment: “Done”, an explanation plus a code change for clarity, or the trade-off when you disagree.
8. Reread the description before merging so it matches the final code.

Next: Reviewer.