Mastering GitHub Pull Requests: Professional Code Review

Mastering GitHub Pull Requests: Professional Code Review

A pull request is the central artefact of collaborative software development. It is not just a mechanism to merge code — it is a structured conversation where authors explain their changes and reviewers verify correctness, security, and maintainability before the code reaches production. Teams that master the PR process ship faster and with fewer regressions than teams that treat PRs as a checkbox before merging.

This guide covers writing high-quality PR descriptions, the mechanics of the GitHub review workflow, constructive review techniques, the three merge strategies and when to use each, draft PRs, and PR automation with GitHub Actions.

Anatomy of a High-Quality PR Description

A PR description answers three questions for the reviewer: what changed, why it changed, and how to verify it is correct.

## Summary

Fixes a validation bypass on the login form in Safari 17 where the HTML5
`required` attribute was not preventing form submission (issue #423).

The root cause was that Safari 17 treats `type="email"` validation differently
when the `novalidate` attribute is present on the parent form. Removed the
`novalidate` attribute that was added in #312 for unrelated reasons.

## Changes

- `LoginForm.tsx`: Removed `novalidate` from the `<form>` element
- `LoginForm.test.tsx`: Added tests for empty email and empty password submission
- `e2e/login.spec.ts`: Added Playwright test targeting Safari

## Testing

- [ ] Manually verified on Safari 17.0 macOS Ventura
- [ ] Manually verified no regression on Chrome 124 and Firefox 125
- [ ] Unit tests pass: `npm test`
- [ ] E2E tests pass: `npm run test:e2e`

## Screenshots

Before: [form submitted with empty email]
After: [browser validation error shown]

## Related

Closes #423

This description lets a reviewer understand the change without reading the diff, verify the fix makes sense given the root cause, and run the tests themselves if they want to reproduce.

Creating a Pull Request via CLI

# Create a branch and make changes
git checkout -b fix/login-safari-validation
# ... make changes ...
git add -p   # Stage changes interactively
git commit -m "fix: remove novalidate to restore Safari email validation"
git push -u origin fix/login-safari-validation

# Open the PR via GitHub CLI
gh pr create \
  --title "fix: restore email validation on Safari 17" \
  --body-file .github/pr_templates/bug_fix.md \
  --reviewer alice,bob \
  --label "bug,priority: high" \
  --milestone "v2.4.2"

# Or open an interactive prompt
gh pr create

PR templates

<!-- .github/pull_request_template.md — automatically loaded for all new PRs -->
## Summary
<!-- What does this PR do? Why? -->

## Changes
<!-- List the significant changes -->
-

## Testing
- [ ] Unit tests pass (`npm test`)
- [ ] Manual testing completed
- [ ] No new TypeScript errors (`tsc --noEmit`)

## Checklist
- [ ] PR description explains the why, not just the what
- [ ] No secrets or credentials committed
- [ ] Dependency changes documented if applicable

## Related Issues
<!-- Closes #XXX -->

Draft Pull Requests

A draft PR signals: "I'm sharing my approach early — not ready for review, not ready to merge."

# Create a draft PR
gh pr create --draft --title "WIP: implement dark mode"

# Convert draft to ready for review when done
gh pr ready

Uses for draft PRs:

• Share an architectural approach before writing all the code
• Get early feedback on a complex algorithm before polishing
• Coordinate with teammates on a feature being built across multiple PRs
• Keep the branch visible in the PR list without triggering review requests

When a PR is in draft state, GitHub does not send review request notifications and does not allow merging.

The Review Workflow

Requesting reviewers

# Add reviewers via CLI
gh pr edit --add-reviewer alice,@my-org/platform-team

# Request re-review after addressing feedback
gh pr request-review --reviewer alice

Leaving a review

GitHub offers three review types:

• Comment: General feedback, no approval or rejection
• Approve: Code is acceptable to merge
• Request changes: Issues that must be resolved before merging

# Submit a review via CLI
gh pr review 123 --approve --body "LGTM — the fix looks correct and tests are solid"
gh pr review 123 --request-changes --body "See inline comments — needs error handling in the fallback path"

Review comment etiquette

Effective code review comments are specific, explain the reasoning, and propose a solution:

# ❌ Vague
This is wrong.

# ❌ Dictatorial
Change this to use a forEach loop.

# ✓ Specific with reasoning and suggestion
The `map` here creates an array that is never used — only the side effect
(the database write) matters. Consider `Promise.all(items.map(...))` to
await all writes, or use `for...of` if sequential execution is required.
Would a sequential loop work for your use case, or do you need parallel writes?

Prefixes that set expectation on blocking severity:

nit: minor style preference — does not block merge
suggestion: take it or leave it — non-blocking
question: I want to understand, not requesting a change
blocking: must address before this merges

Inline Suggestions

Reviewers can suggest exact code changes that the author can apply with one click:

The current approach checks `id` after the database call, but if the query fails
the error won't be caught. Consider:

```suggestion
const user = await db.users.findByIdOrThrow(userId);
```

The author sees a diff of the suggested change and can apply it directly from the GitHub UI without opening a code editor.

The Three Merge Strategies

GitHub offers three ways to incorporate a pull request:

1. Create a Merge Commit

main: A - B - C - M    (M = merge commit)
               ↑
feature: D - E ↑

Preserves all commits from the feature branch plus adds a merge commit. The full history of how the feature was developed is visible, including intermediate commits ("fix typo", "WIP", "debugging").

Use when: you want to preserve the full development history, or the feature branch has a meaningful series of commits that tell a story.

2. Squash and Merge

main: A - B - C - F    (F = squashed commit combining D+E)

Combines all commits from the feature branch into a single commit on main. The development history is discarded; only the final result is preserved.

Use when: the feature branch has messy intermediate commits that are not meaningful history (debug commits, typo fixes, "address review comments"). This keeps git log on main clean and meaningful.

# Set squash as the default for a repository:
# Settings → General → Pull Requests → Allow squash merging ✓
# Default commit message: Pull request title and description

3. Rebase and Merge

main: A - B - C - D' - E'    (D', E' = rebased copies of D, E)

Replays each feature branch commit on top of main. No merge commit, linear history. The individual commits are preserved but their parent commits change (new SHA).

Use when: you want a linear history without merge commits, and the individual commits are meaningful and well-written. Requires understanding Git rebase — do not use this if team members are not comfortable with rebased histories.

Recommendation for most teams: squash and merge. It produces one clean commit per feature on main, the title matches the PR title, and the description explains the change.

Checking Out a PR Locally

# Check out a PR locally for testing
gh pr checkout 123

# This creates a local branch tracking the PR
# Run the app, run tests, reproduce the bug — whatever review requires

# After reviewing, go back to main
git checkout main

# Leave a review
gh pr review 123 --approve --body "Tested locally on Safari — fix confirmed"

PR Automation with GitHub Actions

Auto-assign reviewers based on changed files

# .github/workflows/auto-assign.yml
name: Auto Assign Reviewers

on:
  pull_request:
    types: [opened, ready_for_review]

jobs:
  assign:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/github-script@v7
        with:
          script: |
            const files = await github.rest.pulls.listFiles({
              owner: context.repo.owner,
              repo: context.repo.repo,
              pull_number: context.payload.pull_request.number
            });

            const changedFiles = files.data.map(f => f.filename);
            const reviewers = new Set();

            if (changedFiles.some(f => f.startsWith('.github/workflows/'))) {
              reviewers.add('platform-lead');
            }
            if (changedFiles.some(f => f.startsWith('src/payments/'))) {
              reviewers.add('payments-team-lead');
            }
            if (changedFiles.some(f => f.includes('auth'))) {
              reviewers.add('security-reviewer');
            }

            if (reviewers.size > 0) {
              await github.rest.pulls.requestReviewers({
                owner: context.repo.owner,
                repo: context.repo.repo,
                pull_number: context.payload.pull_request.number,
                reviewers: [...reviewers]
              });
            }

Auto-label based on PR size

- uses: actions/github-script@v7
  with:
    script: |
      const pr = context.payload.pull_request;
      const additions = pr.additions + pr.deletions;

      let label;
      if (additions < 50) label = 'size: XS';
      else if (additions < 200) label = 'size: S';
      else if (additions < 500) label = 'size: M';
      else if (additions < 1000) label = 'size: L';
      else label = 'size: XL';

      await github.rest.issues.addLabels({
        owner: context.repo.owner,
        repo: context.repo.repo,
        issue_number: pr.number,
        labels: [label]
      });

Frequently Asked Questions

Q: How large should a pull request be?

Smaller PRs merge faster and receive better reviews. A PR with 200 lines changed gets reviewed in 15 minutes; a PR with 2,000 lines gets rubber-stamped or deferred. The widely cited guideline is under 400 lines of meaningful changes (excluding generated files, lock files, and migrations). If a feature requires 2,000 lines, split it into a series of PRs: the data model, the API layer, the UI layer, each with its own PR.

Q: What is the correct response when a reviewer requests changes I disagree with?

Disagree in the PR comments with reasoning, not emotion. Explain why you made the original choice and what trade-offs the suggested change introduces. If you cannot reach agreement in two exchanges, escalate to a synchronous discussion — a 10-minute call resolves most code review disagreements faster than 20 comment threads. After discussion, either implement the change or document the decision in the PR comments and proceed.

Q: Should I squash commits before opening a PR?

Not necessarily — if your team uses "squash and merge," GitHub squashes on merge regardless of how many commits are on the branch. Keep meaningful commits during development (they make the PR review easier to follow chronologically) and let GitHub squash at merge time. Only manually squash before opening the PR if the intermediate commits would confuse reviewers.

Q: How do I handle a PR that has been open for weeks without review?

Ping the assignee in the PR comments with a specific question or request. If no response in 24 hours, ping in your team's Slack/chat channel. If the PR is urgent, work with your manager to unblock it. If the PR is not urgent, consider whether it is still valid — long-lived branches often develop merge conflicts and drift from the current codebase. It may be faster to rebase and update it than to chase down a review.

Key Takeaway

A pull request is a quality gate and a knowledge transfer mechanism. High-quality PR descriptions save reviewer time by explaining the why, not just the what. Constructive review comments are specific, provide reasoning, and propose alternatives rather than dictating solutions. Squash and merge keeps the main branch history clean and readable. Draft PRs enable early collaboration without triggering formal review. Automate reviewer assignment, size labelling, and status checks so the process enforces itself without manual overhead.

Read next: GitHub Issues and Projects: Agile Management for Devs →

Part of the GitHub Mastery Course — engineering the review.