pr-quality-checklist

What this skill does

# PR Quality Checklist Skill

## Summary
Comprehensive guide for creating high-quality pull requests that are easy to review, well-documented, and follow team standards. Includes templates, size guidelines, and best practices for both authors and reviewers.

## When to Use
- Before creating any pull request
- When reviewing others' PRs
- To establish team PR standards
- During onboarding of new team members
- When PR reviews are taking too long

## Quick Checklist

### Before Creating PR
- [ ] PR title follows convention: `type(scope): description`
- [ ] Description includes summary, related tickets, and test plan
- [ ] Changes are focused and related (single concern)
- [ ] Code is self-reviewed for obvious issues
- [ ] Tests added/updated and passing
- [ ] No debugging code (console.log, debugger, etc.)
- [ ] TypeScript/type errors resolved
- [ ] Documentation updated if needed
- [ ] Screenshots included for UI changes
- [ ] Changeset added (for user-facing changes)

---

## PR Title Format

### Convention
```
{type}({scope}): {short description}
```

### Types
- **feat**: New feature
- **fix**: Bug fix
- **refactor**: Code change that neither fixes a bug nor adds a feature
- **docs**: Documentation only
- **style**: Formatting, missing semicolons, etc. (no code change)
- **test**: Adding or updating tests
- **chore**: Maintenance tasks, dependency updates
- **perf**: Performance improvement
- **ci**: CI/CD changes

### Examples
```
feat(auth): add OAuth2 login support
fix(cart): resolve race condition in checkout
refactor(search): extract query param parsing logic
docs(api): update authentication examples
test(payment): add integration tests for Stripe
chore(deps): update Next.js to v15
perf(images): implement lazy loading for gallery
ci(deploy): add staging environment workflow
```

### Scope Guidelines
- Use component/module name: `auth`, `cart`, `search`, `api`
- Be specific but not too granular: `user-profile` not `user-profile-settings-modal`
- Consistent with codebase structure

---

## PR Description Template

### Standard Template
```markdown
## Summary
<!-- 1-3 bullet points describing what changed -->
- Added OAuth2 authentication flow
- Integrated with Auth0 provider
- Updated login UI components

## Related Tickets
<!-- Link to Linear/Jira/GitHub issues -->
- Closes #123
- Related to #456
- Fixes ENG-789

## Changes
<!-- Detailed list of changes -->
### Added
- `src/lib/auth/oauth2.ts` - OAuth2 client implementation
- `src/app/api/auth/callback/route.ts` - Auth callback handler
- `src/components/OAuthButtons.tsx` - OAuth login buttons

### Modified
- `src/components/LoginForm.tsx` - Added OAuth buttons
- `src/lib/auth/index.ts` - Export new auth methods
- `README.md` - Updated setup instructions

### Removed
- `src/lib/auth/legacy.ts` - Deprecated auth code
- `src/components/OldLoginForm.tsx` - Replaced by new form

## Screenshots
<!-- Required for UI changes -->
### Desktop
![Desktop view](https://imgur.com/desktop.png)

### Mobile
![Mobile view](https://imgur.com/mobile.png)

### Before/After (if applicable)
| Before | After |
|--------|-------|
| ![Before](before.png) | ![After](after.png) |

## Testing
<!-- How was this tested? -->
- [ ] Unit tests added/updated
- [ ] Integration tests pass
- [ ] Manual testing completed
- [ ] Tested on Chrome, Firefox, Safari
- [ ] Mobile responsive (tested on iOS/Android)
- [ ] Edge cases tested (empty state, error states, loading)

## Breaking Changes
<!-- If any breaking changes -->
⚠️ **Breaking Change**: The `login()` function signature has changed.

**Before:**
```typescript
login(username: string, password: string)
```

**After:**
```typescript
login({ username: string, password: string, provider?: 'local' | 'oauth' })
```

**Migration:**
```typescript
// Old
await login('user', 'pass');

// New
await login({ username: 'user', password: 'pass' });
```

## Checklist
- [ ] Code follows project style guidelines
- [ ] Self-review completed
- [ ] Comments added for complex logic
- [ ] Documentation updated (if needed)
- [ ] Changeset added (if user-facing change)
- [ ] No console.log/debugger statements left
- [ ] No TypeScript errors
- [ ] Tests pass locally
- [ ] Build succeeds
```

### Minimal Template (for small changes)
```markdown
## Summary
Brief description of the change.

## Changes
- Modified `file.ts` to fix XYZ

## Testing
- [ ] Tests pass
- [ ] Verified manually
```

---

## Size Guidelines

### Ideal PR Size
- **Lines changed**: < 300 (excluding generated files)
- **Files changed**: < 15
- **Review time**: < 30 minutes
- **Commits**: 1-5 logical commits

### Size Categories
| Size | Lines | Files | Review Time | Recommendation |
|------|-------|-------|-------------|----------------|
| **XS** | < 50 | 1-3 | 5 min | ✅ Ideal for hotfixes |
| **S** | 50-150 | 3-8 | 15 min | ✅ Good size |
| **M** | 150-300 | 8-15 | 30 min | ⚠️ Consider splitting |
| **L** | 300-500 | 15-25 | 1 hour | ❌ Should split |
| **XL** | 500+ | 25+ | 2+ hours | ❌ Must split |

### When to Split PRs

#### 1. By Feature Phase
```
PR #1: MVP implementation (core functionality)
PR #2: Polish and edge cases
PR #3: Additional features
```

#### 2. By Layer
```
PR #1: Database schema changes
PR #2: Backend API implementation
PR #3: Frontend UI integration
PR #4: Tests and documentation
```

#### 3. By Concern
```
PR #1: Refactoring (no behavior change)
PR #2: New feature (builds on refactored code)
PR #3: Tests for new feature
```

#### 4. By Risk Level
```
PR #1: High-risk changes (need careful review)
PR #2: Low-risk changes (routine updates)
```

### Exceptions to Size Limits
- Generated code (migrations, API clients, types)
- Renaming/moving files (show with `git mv`)
- Bulk formatting changes (separate PR, pre-approved)
- Third-party library integration (well-documented)

---

## Refactoring PRs

### Refactoring Template
```markdown
## Summary
Refactoring and cleanup for [area]

**Goal**: Improve code maintainability without changing behavior

## Motivation
- Reduce code duplication (3 similar functions → 1 reusable utility)
- Improve type safety (any → specific types)
- Remove dead code identified during feature work

## Related Tickets
- ENG-XXX: Improve [area] maintainability
- ENG-YYY: Remove deprecated [feature]

## Stats
- **Lines added**: +91
- **Lines removed**: -1,330
- **Net**: -1,239 ✅
- **Files changed**: 23

## Changes

### Removed (Dead Code)
- `/api/old-endpoint` - unused, replaced by `/api/new-endpoint` in v2.0
- `useDeprecatedHook.ts` - replaced by `useNewHook.ts` (ENG-234)
- `legacy-utils.ts` - functions no longer called anywhere

### Refactored
- **Extracted common logic**: Query param parsing now in `lib/url-utils.ts`
- **Consolidated validation**: 3 duplicate Zod schemas → 1 shared schema
- **Improved types**: Replaced 12 `any` types with proper interfaces

### No Behavior Changes
- [ ] All tests pass (no test modifications needed)
- [ ] Same inputs → same outputs
- [ ] No user-facing changes

## Testing
- [ ] Full test suite passes (no new tests needed)
- [ ] Manual smoke test completed
- [ ] No regressions identified

## Review Focus
- Verify removed code is truly unused (checked with `rg` search)
- Confirm refactored logic is equivalent
- Check no subtle behavior changes introduced
```

### Refactoring Best Practices
- **Separate refactoring from features**: Never mix
- **Verify zero behavior change**: Tests should not need updates
- **Document removed code**: Explain why safe to remove
- **Search thoroughly**: Use `rg`/`grep` to verify no usage
- **Celebrate negative LOC**: Highlight code reduction

---

## Review Checklist

### For Authors (Self-Review)
Before requesting review, check:

#### Code Quality
- [ ] Code is DRY (no obvious duplication)
- [ ] Functions are single-purpose and focused
- [ ] Variable names are clear and descriptive
- [ ] No magic numbers or hardcoded values
- [ ] Error handling is comprehensive
- [ ] Edge cases are handled

#### Testing
- [ ] Tests cover new func