Pull Request Best Practices: Code Review, Templates, and Merge Strategies

┌─────────────────────────────────────────────────────────────┐
│                     PULL REQUEST                             │
├─────────────────────────────────────────────────────────────┤
│  TITLE: [Type] Brief description of change                   │
│         feat: Add user authentication flow                   │
├─────────────────────────────────────────────────────────────┤
│  DESCRIPTION:                                                │
│  ├── What changed and WHY                                   │
│  ├── How to test                                            │
│  ├── Screenshots (if UI)                                    │
│  ├── Related issues: Fixes #123                             │
│  └── Checklist                                              │
├─────────────────────────────────────────────────────────────┤
│  METADATA:                                                   │
│  ├── Reviewers: @teammate                                   │
│  ├── Labels: feature, needs-review                          │
│  ├── Milestone: v2.0                                        │
│  └── Project: Q1 Sprint                                     │
├─────────────────────────────────────────────────────────────┤
│  CHECKS:                                                     │
│  ├── ✓ CI/CD Pipeline                                       │
│  ├── ✓ Code Coverage                                        │
│  ├── ✓ Security Scan                                        │
│  └── ✓ Required Reviewers                                   │
└─────────────────────────────────────────────────────────────┘

## Description
<!-- What changed and why? Link to design docs if applicable -->

## Type of Change
- [ ] Bug fix (non-breaking change fixing an issue)
- [ ] New feature (non-breaking change adding functionality)
- [ ] Breaking change (fix or feature causing existing functionality to change)
- [ ] Documentation update

## How Has This Been Tested?
<!-- Describe the tests you ran and how to reproduce -->
- [ ] Unit tests
- [ ] Integration tests
- [ ] Manual testing

## Screenshots (if applicable)
<!-- Add screenshots for UI changes -->

## Checklist
- [ ] My code follows the project's style guidelines
- [ ] I have performed a self-review
- [ ] I have added tests that prove my fix/feature works
- [ ] New and existing unit tests pass locally
- [ ] I have updated documentation as needed
- [ ] My changes generate no new warnings

## Related Issues
<!-- Link related issues: Fixes #123, Relates to #456 -->

.github/
└── PULL_REQUEST_TEMPLATE/
    ├── feature.md
    ├── bugfix.md
    ├── hotfix.md
    └── documentation.md

┌─────────────────────────────────────────────────────────────┐
│                   SELF-REVIEW CHECKLIST                      │
├─────────────────────────────────────────────────────────────┤
│  □ Review your own diff in "Files changed" tab              │
│  □ Remove debug code, console.logs, commented code          │
│  □ Check for typos in strings, comments, variable names     │
│  □ Verify tests pass locally                                │
│  □ Run linter and fix issues                                │
│  □ Add comments on complex sections to guide reviewers      │
│  □ Ensure PR description explains the WHY                   │
│  □ Link related issues                                      │
│  □ Request specific reviewers who know this code            │
└─────────────────────────────────────────────────────────────┘

┌─────────────────────────────────────────────────────────────┐
│                    REVIEW PRIORITIES                         │
├─────────────────────────────────────────────────────────────┤
│                                                              │
│   1. CORRECTNESS (Must fix)                                 │
│      └── Does it work? Logic errors? Edge cases?            │
│                                                              │
│   2. SECURITY (Must fix)                                    │
│      └── Injection risks? Auth issues? Data exposure?       │
│                                                              │
│   3. DESIGN (Should discuss)                                │
│      └── Architecture fit? Maintainability? Patterns?       │
│                                                              │
│   4. READABILITY (Nice to have)                             │
│      └── Clear names? Good comments? Understandable?        │
│                                                              │
│   5. STYLE (Automate instead)                               │
│      └── Formatting? Linting rules? Let tools handle it     │
│                                                              │
└─────────────────────────────────────────────────────────────┘

<!-- Bad: Vague -->
This is wrong.

<!-- Good: Specific with suggestion -->
This will throw a NullPointerException if `user` is null (line 42).
Consider adding a null check:
```java
if (user != null && user.isActive()) {

**Review comment prefixes:**

| Prefix | Meaning | Action Required |
|--------|---------|-----------------|
| `blocking:` | Must fix before merge | Yes |
| `suggestion:` | Consider this approach | Optional |
| `question:` | Need clarification | Response needed |
| `nit:` | Minor nitpick | Optional |
| `praise:` | Good work! | None |

### Review Workflow

## Merge Strategies

### Comparison

### When to Use Each

| Strategy | Best For | Avoid When |
|----------|----------|------------|
| **Merge commit** | Feature branches with meaningful commit history | Main has many merge commits already |
| **Squash and merge** | Feature branches with messy commits (WIP, fixup) | Commits should be preserved individually |
| **Rebase and merge** | Clean branches with atomic commits | Commits are already on main (double commits) |

### Configuring Default Strategy

In GitHub repository settings:

1. Go to **Settings > General**
2. Scroll to **Pull Requests**
3. Configure allowed merge methods:
   - Allow merge commits
   - Allow squash merging ✓ (default for most teams)
   - Allow rebase merging

You can also set a default merge button behavior.

## Automated Checks

### Required Status Checks

Configure in **Settings > Branches > Branch protection rules**:

```yaml
# Example CI workflow for PR checks
name: PR Checks
on:
  pull_request:
    branches: [main]

jobs:
  lint:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v4
      - name: Run linter
        run: npm run lint

  test:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v4
      - name: Run tests
        run: npm test

  build:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v4
      - name: Build
        run: npm run build

  security:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v4
      - name: Run security scan
        uses: snyk/actions/node@master
        env:
          SNYK_TOKEN: ${{ secrets.SNYK_TOKEN }}

name: PR Size Check
on: pull_request

jobs:
  size-check:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v4
        with:
          fetch-depth: 0

      - name: Check PR Size
        run: |
          ADDITIONS=$(gh pr view ${{ github.event.pull_request.number }} --json additions -q '.additions')
          DELETIONS=$(gh pr view ${{ github.event.pull_request.number }} --json deletions -q '.deletions')
          TOTAL=$((ADDITIONS + DELETIONS))

          if [ $TOTAL -gt 500 ]; then
            echo "::warning::Large PR detected ($TOTAL lines changed). Consider splitting into smaller PRs."
          fi
        env:
          GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}

# .github/labeler.yml
frontend:
  - 'src/components/**'
  - 'src/pages/**'
  - '**/*.css'

backend:
  - 'src/api/**'
  - 'src/services/**'

documentation:
  - '**/*.md'
  - 'docs/**'

tests:
  - '**/*.test.ts'
  - '**/*.spec.ts'

# .github/workflows/labeler.yml
name: Labeler
on: pull_request

jobs:
  label:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/labeler@v5
        with:
          repo-token: ${{ secrets.GITHUB_TOKEN }}

# Default owners for everything
* @org/core-team

# Frontend code
/src/components/ @org/frontend-team
/src/pages/ @org/frontend-team
*.css @org/frontend-team
*.tsx @org/frontend-team

# Backend code
/src/api/ @org/backend-team
/src/services/ @org/backend-team

# Infrastructure
/terraform/ @org/devops-team
/.github/workflows/ @org/devops-team
Dockerfile @org/devops-team

# Security-sensitive code requires security team review
/src/auth/ @org/security-team @org/backend-team
/src/encryption/ @org/security-team

# Documentation
/docs/ @org/docs-team
*.md @org/docs-team

# Package management
package.json @org/core-team
package-lock.json @org/core-team

┌─────────────────────────────────────────────────────────────┐
│                    STACKED PRs                               │
├─────────────────────────────────────────────────────────────┤
│                                                              │
│   main                                                       │
│     │                                                        │
│     └──► feature/auth-base (PR #1)                          │
│              │                                               │
│              └──► feature/auth-oauth (PR #2)                │
│                      │                                       │
│                      └──► feature/auth-ui (PR #3)           │
│                                                              │
│   Workflow:                                                  │
│   1. Create PR #1 against main                              │
│   2. Create PR #2 against feature/auth-base                 │
│   3. Create PR #3 against feature/auth-oauth                │
│   4. Review and merge PR #1                                 │
│   5. Retarget PR #2 to main (GitHub auto-updates)           │
│   6. Review and merge PR #2                                 │
│   7. Retarget PR #3 to main                                 │
│   8. Review and merge PR #3                                 │
│                                                              │
└─────────────────────────────────────────────────────────────┘

# Create draft PR with GitHub CLI
gh pr create --draft --title "feat: Add auth" --body "WIP: Authentication system"

# Mark ready for review when done
gh pr ready

# Option 1: Merge main into your branch
git checkout feature-branch
git fetch origin
git merge origin/main
git push

# Option 2: Rebase onto main (cleaner but requires force push)
git checkout feature-branch
git fetch origin
git rebase origin/main
git push --force-with-lease

# Using GitHub CLI to analyze PR metrics
gh pr list --state merged --json number,createdAt,mergedAt,additions,deletions \
  --jq '.[] | "\(.number): \(.additions + .deletions) lines, \(((.mergedAt | fromdate) - (.createdAt | fromdate)) / 3600 | floor) hours"'

□ Self-review completed
□ Tests added/updated
□ Documentation updated
□ PR title follows conventions
□ Description explains what and why
□ Screenshots for UI changes
□ Related issues linked
□ Appropriate reviewers assigned

□ Understand the context (read description first)
□ Check for correctness and edge cases
□ Look for security issues
□ Verify tests are meaningful
□ Assess maintainability
□ Provide actionable feedback
□ Approve, request changes, or comment