Skip to content

Pull Requests and Code Reviews

Pull Requests and Code Reviews

Why PRs matter in this course

PRs are the primary artifact showing collaboration, communication, and engineering rigor. Reviews improve code quality, catch logic and design issues early, and share knowledge across the team.

Anatomy of a Good PR

  • Clear title and concise description — follow Conventional Commits in the title when applicable.
  • Small, focused scope — easier and faster to review.
  • Link related issues (use Fixes #123 to auto-close issues) and include testing notes.
  • Include screenshots/GIFs for UI changes and short reproduction steps for reviewers.
  • Ensure passing CI (tests, linters) before requesting a review.

Reviewer checklist (what to look for)

  • Correctness: does the code do what's intended and handle edge cases?
  • Readability: are names, structure, and comments clear?
  • Tests: are there unit/integration tests, and do they cover edge cases?
  • Security: any input validation, secrets leakage, or unsafe operations?
  • Performance: any obvious performance regressions or improvements?
  • API/contract changes: are they documented and backward-compatible?

Merge guidance and conflict handling

  • Prefer Squash and merge for feature branches to keep main tidy (one meaningful commit per feature).
  • Use Merge commit when you want to preserve branch history and context.
  • Fast-forward merges are OK for single-commit branches but are used less frequently.

Conflict handling:

# Fetch latest main
git fetch origin
# Rebase your branch on top of main (resolve conflicts if any)
git rebase origin/main
# After resolving conflicts and finishing the rebase
git push --force-with-lease
  • Use --force-with-lease when pushing rebased branches to reduce the chance of overwriting others' work.
  • If a rebase is risky for a shared branch, prefer merging main into your branch and resolving conflicts there.

PR templates and automation

  • Add a PULL_REQUEST_TEMPLATE.md in .github/ or .gitlab/ to standardize required info (summary, testing, notes, checklist).
  • Use commitlint and lint-staged/husky to enforce commit and style rules locally.
  • Configure CI to run tests, linters, and security scanners on PRs. Require those checks in branch protection rules.

Reviewer etiquette

  • Be constructive and specific — suggest improvements, not just criticisms.
  • Ask clarifying questions rather than assuming intent.
  • Leave small nit comments as suggestions; for blocking issues, explain why they must be addressed.
  • If you're requesting changes, give guidance and, where possible, sample code or a suggested fix.

Example PR template (short)

## Summary

Describe the change and why it was made.

## Testing

How was this tested? Include commands or CI links.

## Checklist

- [ ] Tests added or updated
- [ ] Documentation updated
- [ ] CI passes

Practical tips

  • Keep PRs small and self-contained.
  • Run tests and linters locally before pushing.
  • Reference related tickets and design discussions.

1. PR Lifecycle (Visual)

sequenceDiagram
    participant Dev as Developer
    participant Repo as Remote Repo
    participant CI as CI Pipeline
    participant Rev as Reviewer
    Dev->>Repo: Push branch
    Dev->>Repo: Open Draft PR
    Repo->>CI: Run tests/linters
    CI-->>Repo: Status checks
    Rev->>Repo: Review & comments
    Dev->>Repo: Address feedback
    Rev->>Repo: Approve
    Dev->>Repo: Squash & Merge
    Repo-->>Dev: Pull updated main

2. Roles & Responsibilities

Role Responsibilities
Author Clear PR description, responds to feedback, keeps scope tight
Reviewer Evaluate correctness, clarity, risks, improvements
Maintainer Enforce standards, merge policy, resolve ambiguity
CI Automated guard (tests, lint, security)

3. PR Size Guidance

Size Lines Changed Review Time Target Recommendation
XS <100 <10 min Fine
S 100–300 <20 min Preferred size
M 300–600 30–45 min Consider splitting
L 600–900 >45 min Split into logical PRs
XL >900 Fatigue risk MUST split

4. Anti-Patterns

Pattern Why Harmful Better
Mixed concerns (feature + refactor + formatting) Hard to review / revert Separate PRs
Drive-by “LGTM” Shallow review Use checklist mindset
Huge late PR Bottlenecks Draft early for feedback
Silent conflict resolution Hidden logic changes Comment explaining resolution
Ping-pong nit wars Wastes time Adopt formatter / style guide

5. Suggested Review SLAs (Team Agreement)

Event SLA
First response Within 4 business hours
Follow-up after changes Within 2 business hours
Production hotfix PR Immediate priority

6. Feedback Categorization (Label in Comments)

Tag Meaning Example
nit Minor style / optional tweak variable naming suggestion
suggestion Alternate approach propose extracting function
question Clarification needed "Why is cache invalidation here?"
blocking Must address before merge missing validation

7. Lightweight Security/Privacy Scan

Check Question
Secrets Any keys/tokens committed?
Input Validation Are new endpoints validating input?
Logging Sensitive data accidentally logged?
Dependencies New packages vetted (license / activity)?

8. Automation Ideas

Automation Tool
PR label by size GitHub Action script
Conventional commit check commitlint action
Secret scanning GitHub Advanced Security / gitleaks
Preview environments Vercel / Netlify / ephemeral infra
Test coverage comment Coverage action

9. Enhanced Checklist

Author: - [ ] Scope single-purpose - [ ] Title clear + Conventional Commit style - [ ] Description covers why + how - [ ] Linked issues / tickets - [ ] Tests & docs updated - [ ] No secrets / large binaries - [ ] CI all green

Reviewer: - [ ] Understands intent - [ ] Logic & edge cases sound - [ ] Error handling present - [ ] Performance reasonable - [ ] Security/privacy scan passed - [ ] Tests meaningful & pass - [ ] Approve or actionable changes requested