I've reviewed thousands of PRs. Some get approved in 10 minutes. Others sit for days.
The difference isn't code quality. It's the PR description.
Bad PR descriptions look like this:
Title: Fix bug
Description: Fixed the thing
I have to:
- Clone the PR
- Read every line of code
- Figure out what changed
- Guess why it changed
- Hunt for the related issue
- Ask questions in comments
- Wait for answers
- Re-review
Time to approve: 2-3 days, multiple review cycles
Good PR descriptions look like this:
Title: Fix user profile crash when email is null
## Problem
Users with null emails (legacy data from 2019 migration) crash the profile
page when clicking "Edit Profile". Error: "Cannot read property 'toLowerCase'
of null"
Affects ~500 users based on database query.
## Solution
Added null check before calling toLowerCase(). If email is null, show
"Add email" prompt instead.
## Testing
- [x] Manual: Tested with user ID 12345 (has null email)
- [x] Unit: Added test for null email case
- [x] Verified existing tests still pass
## Screenshots
[Before] - Crash screen
[After] - "Add email" prompt
Fixes #1234
I can:
- Understand the problem immediately
- See the solution makes sense
- Trust it's been tested
- Approve in 5 minutes
Time to approve: 5-10 minutes, one review cycle
Let me show you how to write PR descriptions that get approved fast.
The 5-Minute Rule
If a reviewer can't understand your PR in 5 minutes, you've failed.
Your goal: Make reviewing effortless.
Every second a reviewer spends confused is a second they might:
- Ask questions (delays approval)
- Request changes (delays approval)
- Ignore your PR (delays approval)
Your job: Answer all their questions before they ask.
The PR Description Template
Copy this. Customize it. Use it every time.
## Problem
[What bug are you fixing? What feature are you adding? Why does this matter?]
[Impact: How many users? How severe? Business context?]
## Solution
[High-level approach. NOT line-by-line code explanation.]
[Key decisions made and why]
[Any alternatives considered and why rejected]
## Testing
- [ ] Manual testing steps performed
- [ ] Unit tests added/updated
- [ ] Integration tests added/updated
- [ ] Tested edge cases: [list them]
## Risks
[Any concerns? Known limitations? Future work needed?]
## Screenshots/Videos
[If UI change: before/after screenshots or screen recording]
## Related
Fixes #[issue number]
Related to #[other PR or issue]
Depends on #[blocking PR]
---
## Reviewer Notes
[Anything specific reviewers should focus on? Anything you're unsure about?]
Section-by-Section Breakdown
Title: Make It Searchable
Bad titles:
Fix bug
Update component
Changes
WIP: stuff
Good titles:
Fix profile crash when user email is null
Add pagination to user management table
Upgrade React Router from v5 to v6
Refactor authentication to use OAuth2
Template:
[Action] [What] [Where/When/Why]
Examples:
Fix infinite loop in useEffect when deps change rapidly
Add loading state to checkout form submit button
Remove deprecated props from Button component
Upgrade TypeScript from 4.9 to 5.3
Why it matters: Reviewers often search for PRs. "Fix bug" is unsearchable. "Fix profile crash when email is null" is specific.
Problem: Why Does This PR Exist?
This is the most important section. If reviewers don't understand the problem, they can't evaluate the solution.
Bad:
## Problem
Need to fix the profile page
Good:
## Problem
Users with null emails (legacy data from 2019 migration) crash when clicking
"Edit Profile" button.
Error: `Cannot read property 'toLowerCase' of null` at ProfileForm.tsx:45
**Impact:**
- Affects ~500 users (0.5% of active users)
- Support tickets: 12 in last week
- Critical path: Users can't update their profiles
- Business impact: Users can't change their notification preferences
**Root cause:**
The 2019 migration from old auth system didn't require emails. Current code
assumes all users have emails.
What to include:
- Exact error message (if it's a bug)
- Impact: how many users, how severe
- Business context: why this matters
- Root cause: what's actually wrong
Bad example - vague:
## Problem
The form is broken
Good example - specific:
## Problem
Checkout form doesn't validate credit card expiration dates, allowing users to
submit expired cards.
**Impact:**
- Payment processor rejects 15% of transactions
- Users see generic "Payment failed" error
- Support receives ~50 tickets/day asking why payment failed
- Lost revenue: ~$50k/month in abandoned carts
**Root cause:**
Validation regex only checks format (MM/YY) but doesn't compare to current date.
Solution: What Did You Do?
Explain your approach at a high level. Don't describe every line of code.
Bad:
## Solution
Added a check
Bad (too detailed):
## Solution
On line 45, I added an if statement that checks if email is null. If it is,
I set a variable called showAddEmail to true. Then on line 67, I check if
showAddEmail is true, and if so, I render a div with className "add-email-prompt"
that contains a button...
Good:
## Solution
Added null check before processing email:
1. Check if user.email is null
2. If null, show "Add email" prompt instead of edit form
3. Prompt includes link to account settings to add email
**Key decision:**
Chose to show prompt rather than pre-populate with empty string, because empty
string would allow users to save profile without fixing the root issue.
**Alternative considered:**
Backfill all null emails with "noemail@example.com". Rejected because this
creates invalid data and doesn't solve the problem for future users.
What to include:
- High-level approach (numbered steps work well)
- Key decisions and reasoning
- Alternatives considered (shows you thought it through)
Pro tip: If you considered multiple approaches, show them:
## Solution
**Approach taken:** Add validation to frontend form
**Why not these alternatives:**
1. ❌ Fix in backend - Requires API change, affects mobile apps
2. ❌ Backfill null emails - Creates invalid data
3. ❌ Disable form for null emails - Poor UX, users can't update anything
Testing: How Do I Know It Works?
Show you've tested thoroughly. Reviewers trust tested code.
Bad:
## Testing
Tested it, works fine
Good:
## Testing
**Manual testing:**
- [x] Tested with user ID 12345 (has null email) - shows prompt correctly
- [x] Tested with user ID 67890 (has email) - edit form works normally
- [x] Clicked "Add email" link - navigates to settings correctly
- [x] Added email in settings, returned to profile - edit form now appears
**Automated tests:**
- [x] Added unit test: `ProfileForm.test.tsx` - null email case
- [x] Added unit test: `ProfileForm.test.tsx` - valid email case
- [x] Updated integration test: `profile-flow.test.tsx`
- [x] All existing tests passing (142/142)
**Edge cases tested:**
- [x] Email is empty string (not just null)
- [x] Email is whitespace only
- [x] User spams "Add email" button (no duplicate navigations)
**Browser testing:**
- [x] Chrome 120
- [x] Safari 17
- [x] Firefox 121
- [x] Mobile Safari iOS 17
Checklist template:
## Testing
**Functionality:**
- [ ] Happy path works
- [ ] Error cases handled
- [ ] Edge cases tested
**Tests added:**
- [ ] Unit tests for new logic
- [ ] Integration tests for user flow
- [ ] All existing tests still pass
**Manual verification:**
- [ ] Tested locally
- [ ] Tested in staging
- [ ] Verified fix with reproduction steps from bug report
Risks: What Could Go Wrong?
Be honest about risks. Reviewers appreciate transparency.
Bad:
## Risks
None
Good:
## Risks
**Known limitations:**
- This fixes the immediate crash, but doesn't solve the root problem of users
having null emails. Follow-up PR needed to prompt users to add emails.
**Performance impact:**
- Added database query to check email status. Query is fast (~5ms) and cached,
but monitoring recommended.
**Rollback plan:**
- If issues arise, can disable via feature flag `PROFILE_NULL_EMAIL_CHECK`
- No database migrations, safe to rollback
**Future work needed:**
- #1235 - Backfill null emails (non-urgent)
- #1236 - Add email requirement to onboarding flow
What to include:
- Known limitations
- Performance implications
- Rollback strategy
- Future work needed
Screenshots/Videos: Show, Don't Tell
For UI changes, screenshots are mandatory.
Template:
## Screenshots
**Before:**

[Description of problem]
**After:**

[Description of fix]
**Screen recording:**
[Link to Loom/video showing interaction]
Pro tips:
- Use side-by-side before/after when possible
- Annotate screenshots (arrows, circles) to highlight changes
- For interactions, use screen recordings (Loom, CloudApp)
- For responsive changes, show mobile + desktop
Example:
## Screenshots
**Before:** Empty email crashes the page

**After:** Shows "Add email" prompt instead

**Mobile view:**

**Demo video:** https://www.loom.com/share/abc123
Shows full flow: crash → fix → adding email → form appearing
Related: Link Everything
Connect your PR to the broader context.
Template:
## Related
Fixes #1234
Closes #1235
Resolves #1236
Part of #1240 (epic)
Depends on #1238 (must merge first)
Blocks #1242 (waiting on this)
Related to #1237 (similar issue)
[Design spec](link-to-figma)
[Tech spec](link-to-doc)
[Slack discussion](link-to-thread)
GitHub keywords that auto-close issues:
Fixes #123Closes #123Resolves #123
Why link everything:
- Reviewers see full context
- Issues auto-close when PR merges
- Future developers understand the history
- Stakeholders can track progress
Reviewer Notes: Direct Attention
Tell reviewers what to focus on and what to ignore.
Template:
## Reviewer Notes
**Focus areas:**
- `ProfileForm.tsx` - Main logic change
- `profile.test.tsx` - New test coverage
**Can skip:**
- `package-lock.json` - Dependency update (unrelated)
- `translations/` - Auto-generated from i18n tool
**Questions for reviewers:**
- Line 45: Is this error message clear enough?
- Line 67: Should we log this to analytics?
**Known issues (will fix before merge):**
- [ ] TODO on line 123 - will implement before merging
- [ ] Commented-out code - will remove
Why this helps:
- Saves reviewers time
- Gets feedback on specific concerns
- Shows you're thoughtful about the review
Size Matters: Keep PRs Small
Bad: 47 files changed, 3,000 lines added
Reviewers see this and:
- Put it off until "later"
- Skim instead of reviewing carefully
- Miss bugs
Good: 3 files changed, 150 lines added
Reviewers can:
- Review in one sitting
- Understand all changes
- Provide thoughtful feedback
Guidelines:
- Ideal: < 200 lines changed
- OK: 200-400 lines changed
- Large: 400-800 lines changed (split if possible)
- Too large: > 800 lines changed (definitely split)
How to split large PRs:
Epic: Implement user profile redesign
PR 1: Add database schema for profile fields (#1234)
PR 2: Create ProfileCard component (#1235) ← You are here
PR 3: Integrate ProfileCard into profile page (#1236)
PR 4: Add profile editing flow (#1237)
PR 5: Migrate existing users to new schema (#1238)
Each PR:
- Is independently reviewable
- Can be merged separately
- Builds on previous PRs
In your PR description: ""
## Context
This is part 2 of 5 in the profile redesign epic (#1233).
**Previous PRs:**
- [x] #1234 - Database schema (merged)
**This PR:**
- Creates reusable ProfileCard component
- Adds unit tests
- Adds Storybook stories
**Next PRs:**
- [ ] #1236 - Integration into profile page
- [ ] #1237 - Editing flow
- [ ] #1238 - User migration
Real Examples: Bad vs. Good
Example 1: Bug Fix
❌ Bad PR:
Title: fix
fixed the bug where the thing broke
Reviewer questions:
- What bug?
- What thing?
- How did it break?
- How did you fix it?
- How do I test it?
- Is this related to any issue?
✅ Good PR:
Title: Fix duplicate charges when user double-clicks "Pay Now"
## Problem
Users who double-click the "Pay Now" button get charged twice.
**Impact:**
- 23 duplicate charges in the last week
- $4,500 in refunds processed
- 15 angry support tickets
- Payment processor flagging us for suspicious activity
**Root cause:**
No debouncing on submit button. Each click triggers a separate API call.
## Solution
1. Disable button immediately on first click
2. Re-enable only after API response (success or error)
3. Add 1-second debounce as secondary protection
**Why not just debounce:**
Debouncing alone isn't enough. If API is slow, users might click again after
1 second. Disabling the button is more reliable.
## Testing
**Manual:**
- [x] Single click - works normally
- [x] Double click - only one charge created
- [x] Spam clicking - only one charge created
- [x] Slow network (throttled to 3G) - still works correctly
**Automated:**
- [x] Added test: double-click prevention
- [x] Added test: button re-enables on error
- [x] All existing payment tests passing (24/24)
**Verified in production:**
- [x] Tested in staging with production-like load
- [x] Checked Datadog for error rate (0% increase)
## Risks
**Low risk change:**
- Only affects button state, no payment logic changes
- Can rollback via feature flag `PAYMENT_BUTTON_DEBOUNCE`
- If button gets stuck disabled, users can refresh page
## Screenshots
**Before:** Button stays enabled, multiple clicks allowed

**After:** Button disables on first click, re-enables after response

**Demo:** https://www.loom.com/share/abc123
Shows spam clicking, only one charge created
Fixes #1234
Example 2: New Feature
❌ Bad PR:
Title: Add pagination
Added pagination to the table
✅ Good PR:
Title: Add pagination to user management table (1000+ users)
## Problem
User management page loads all users at once, causing:
- 15+ second load time for admins with 1000+ users
- Browser memory issues (crashes on 2000+ users)
- Unable to find specific users quickly
**User feedback:**
"I have 1500 users and the page won't even load anymore" - Customer #4521
## Solution
Implement server-side pagination:
1. Load 50 users per page (configurable)
2. Add pagination controls (prev/next, page numbers)
3. Maintain filters and search across pages
4. Show total count and current range
**Technical approach:**
- Backend: Added `limit` and `offset` query params to `/api/users`
- Frontend: useQuery with pagination state
- Maintained backward compatibility (defaults to 50)
**Design decisions:**
- 50 users per page: balances load time vs. scrolling
- Show 5 page numbers at a time (... for rest)
- Preserve search/filters when changing pages
## Testing
**Manual:**
- [x] Tested with 10 users (shows 1 page)
- [x] Tested with 150 users (shows 3 pages)
- [x] Tested with 1500 users (page load < 2 seconds)
- [x] Search works across all pages
- [x] Filters work with pagination
- [x] URL updates with page number (shareable links)
- [x] Back button works correctly
**Automated:**
- [x] Unit tests: pagination logic
- [x] Unit tests: page number calculation
- [x] Integration test: navigating between pages
- [x] Integration test: filters + pagination
- [x] API tests: limit/offset params
**Performance:**
- Before: 15s load (1000 users)
- After: 1.8s load (50 users per page)
- Backend query time: 45ms (with indexes)
## Risks
**API changes:**
- Added optional query params (backward compatible)
- Old API calls (no params) still work, default to 50
**Future improvements needed:**
- #1245 - Add "items per page" selector
- #1246 - Infinite scroll option
- #1247 - Better mobile pagination UI
## Screenshots
**Before:** All 1000+ users in one long list

**After:** Clean pagination, 50 users per page

**Pagination controls:**

**Mobile view:**

## Related
Fixes #1234
Part of performance epic #1200
Design: https://www.figma.com/file/abc123
Special Cases
Refactoring PRs
Title: Refactor authentication context to use zustand
## Why Refactor?
Current Context API implementation has issues:
- Causes unnecessary re-renders (entire app re-renders on auth change)
- Difficult to test (requires Provider wrapper)
- 200+ lines of boilerplate
## Changes
Replace Context API with zustand:
- ~200 lines removed
- Simpler API (`useAuth()` instead of `useContext(AuthContext)`)
- Better performance (selective subscriptions)
- Easier to test (no Provider needed)
## Migration Strategy
**Backward compatible:**
- Old Context API still works (deprecated)
- New zustand store available alongside
- Will remove Context in next PR after migration complete
**Migration path for other developers:**
diff
- const { user } = useContext(AuthContext);
- const user = useAuth(state => state.user);
## Testing
- [x] All existing tests still pass (no changes needed)
- [x] Added zustand store tests
- [x] Verified auth flow works identically
- [x] No console errors in dev/prod
## Performance
**Before:**
- Login triggers 47 component re-renders
- DevTools shows deep re-render tree
**After:**
- Login triggers 3 component re-renders
- Only components using auth state re-render
Fixes #1234 (performance issue)
Dependency Updates
Title: Upgrade React Router from v5 to v6
## Why Upgrade?
- v5 is deprecated (no security updates)
- v6 has better TypeScript support
- v6 has better hooks API
- Blocking upgrade to React 18
## Breaking Changes
**1. Route syntax changed:**
diff
- } />
**2. useHistory → useNavigate:**
diff
- const history = useHistory();
- history.push('/home');
- const navigate = useNavigate();
- navigate('/home');
**3. Switch → Routes:**
diff
- } />
## Migration Approach
**Completed:**
- [x] Updated all route definitions (34 files)
- [x] Replaced useHistory with useNavigate (67 locations)
- [x] Updated all tests (45 test files)
- [x] Updated type definitions
**Tested:**
- [x] All navigation flows work
- [x] URL params work
- [x] Nested routes work
- [x] Redirects work
- [x] 404 handling works
## Risks
**Large change:** 67 files modified
**Mitigation:**
- Incremental testing (tested each route individually)
- Feature flag: `ENABLE_REACT_ROUTER_V6` (can rollback)
- Deployed to staging for 2 days (no issues)
## Related
Blocks #1240 (React 18 upgrade)
Migration guide: https://reactrouter.com/docs/en/v6/upgrading/v5
Templates for Different PR Types
Bug Fix Template
Title: [Fix: Brief description of bug]
## Problem
[What's broken? Error message? Impact?]
## Root Cause
[Why did this happen?]
## Solution
[What did you change?]
## Testing
- [ ] Reproduced bug before fix
- [ ] Verified fix resolves issue
- [ ] Tested edge cases
- [ ] Added regression test
## Risks
[Any concerns?]
Fixes #[issue]
Feature Template
Title: [Add: Brief description of feature]
## Why This Feature?
[User need? Business value? Use case?]
## Implementation
[Technical approach]
## Testing
- [ ] Manual testing completed
- [ ] Unit tests added
- [ ] Integration tests added
- [ ] Edge cases covered
## Screenshots
[Before/after if applicable]
## Related
Fixes #[issue]
[Design link]
Refactoring Template
Title: [Refactor: What you're refactoring]
## Why Refactor?
[Technical debt? Performance? Maintainability?]
## Changes
[What's different?]
## Migration Path
[How do other devs adapt?]
## Testing
- [ ] All existing tests pass
- [ ] No behavior changes
- [ ] Performance verified
Related to #[issue]
The Self-Review Checklist
Before submitting your PR, review it yourself:
**Before submitting:**
Title:
- [ ] Descriptive and searchable
- [ ] Follows format: [Action] [What] [Where/Why]
Description:
- [ ] Problem is clear
- [ ] Solution makes sense
- [ ] Testing is thorough
- [ ] Risks are addressed
- [ ] Screenshots included (if UI change)
- [ ] Links to related issues/docs
Code:
- [ ] Removed console.logs
- [ ] Removed commented-out code
- [ ] Removed TODO comments (or created issues)
- [ ] No merge conflicts
- [ ] CI passing
Polish:
- [ ] Spell-checked description
- [ ] Code is formatted (Prettier)
- [ ] Linted (ESLint)
Common Mistakes to Avoid
❌ Mistake 1: WIP PRs Without Context
Title: WIP
Description: work in progress, don't review yet
Better: Still provide context even if WIP
Title: [WIP] Add pagination to user table
## Goal
Implementing server-side pagination for tables with 1000+ rows
## Status
- [x] Backend API changes
- [x] Frontend pagination logic
- [ ] UI polish
- [ ] Tests
**Not ready for full review yet**, but happy to get feedback on approach.
**Specifically:**
- Does the API design make sense? (see `UserController.ts`)
- Is the pagination hook reusable enough? (see `usePagination.ts`)
❌ Mistake 2: Giant Unexplained Diffs
Title: Updates
47 files changed, 3,847 insertions(+), 2,103 deletions(-)
Description: Made some changes
Better: Explain large changes
Title: Upgrade to TypeScript 5.0 and fix type errors
## Changes
**67 files changed** - most changes are type fixes:
**Breaking type changes:**
- `strict: true` in tsconfig (caught 45 real bugs)
- `noUncheckedIndexedAccess: true` (array access now properly typed)
**Categories of changes:**
1. Added null checks (23 files)
2. Fixed 'any' types (15 files)
3. Updated library type imports (18 files)
4. Misc type narrowing (11 files)
**Key files:**
- `tsconfig.json` - stricter settings
- `src/types/` - updated type definitions
- `src/utils/` - added type guards
**No behavior changes** - only type safety improvements
❌ Mistake 3: No Screenshots for UI Changes
Visual changes without visuals = reviewers have to check out your branch.
Always include:
- Before/after screenshots
- Different screen sizes (mobile, tablet, desktop)
- Different states (loading, error, success)
- Screen recording for interactions
❌ Mistake 4: Assuming Reviewers Have Context
You've been thinking about this problem for hours. Reviewers have 30 seconds.
Bad:
Fixed the race condition
Good:
Fixed race condition in useEffect when userId changes rapidly
## Problem
When user navigates quickly between profiles (rapid userId changes),
multiple API calls fire simultaneously. The responses arrive out of order,
causing wrong user's data to display.
Example: Click User A → User B → User C rapidly
Response order: C → A → B
Result: User B's data shows (wrong!)
Making Reviewers' Lives Easier
1. Add Comments to Complex Code
// In the PR description: ""
## Reviewer Notes
Line 45-67: Complex memoization logic
The triple-nested useMemo is intentional. We need to:
1. Memoize the filter function (inner)
2. Memoize the filtered data (middle)
3. Memoize the sorted result (outer)
Tried simpler approaches but they caused performance issues with large datasets.
// In the code:
// NOTE: This triple-nesting is intentional for performance.
// Each level prevents re-computation when only specific deps change.
// See PR description for alternatives considered.
const sorted = useMemo(() => {
const filtered = useMemo(() => {
const filterFn = useMemo(() => {
// ...
2. Explain Non-Obvious Decisions
## Design Decisions
**Why setTimeout instead of debounce?**
Debounce would delay the API call, but we want immediate feedback. setTimeout
ensures button re-enables even if API call fails.
**Why not use a library for this?**
Evaluated `react-use` and `lodash`, but both added 20KB for a 5-line solution.
Not worth the bundle size for this simple use case.
3. Point Out Potential Concerns
## Reviewer Notes
**Line 123:** You might think this could cause a memory leak, but it's fine
because we clean up in the useEffect return function (line 145).
**Performance:** This does add an extra render, but only in the error case,
which is rare. Happy path performance is unchanged.
The Ultimate PR Description
Here's a complete example that does everything right:
Title: Fix race condition in user profile when navigating rapidly
## Problem
**What's broken:**
When users navigate rapidly between profiles (clicking different users in quick
succession), the wrong user's data displays.
**Why it happens:**
Multiple API calls fire simultaneously, but responses arrive out of order due
to varying response times. The last response "wins" even if it's for an older
navigation.
**Example scenario:**
1. User clicks Profile A (slow server, 2s response)
2. User clicks Profile B (fast server, 0.5s response)
3. Profile B loads (correct)
4. Profile A loads and overwrites Profile B (wrong!)
**Impact:**
- Affects power users who navigate quickly (~5% of users)
- Reported by 8 users in last 2 weeks
- Privacy concern: Users might see wrong person's data
- No data corruption (read-only), but poor UX and trust issue
**Root cause:**
useEffect doesn't cancel previous fetch when userId changes.
## Solution
Implemented AbortController to cancel in-flight requests:
1. Create AbortController in useEffect
2. Pass signal to fetch
3. Cancel previous request on cleanup
4. Ignore responses from cancelled requests
**Why AbortController:**
- Native browser API (no library needed)
- Proper way to cancel fetch requests
- Works across all modern browsers
**Alternatives considered:**
1. ❌ Track request IDs - complex, error-prone
2. ❌ Debounce navigation - poor UX, feels sluggish
3. ✅ AbortController - clean, standard approach
**Code changes:**
- Modified `useUserProfile.ts` hook
- Added cleanup function to useEffect
- Added error handling for abort errors (expected, not logged)
## Testing
**Manual testing:**
- [x] Rapid navigation (10 users in 5 seconds) - correct user always shows
- [x] Slow network (throttled to 3G) - race condition fixed
- [x] Fast navigation then back button - works correctly
- [x] Navigation during loading - cancels and loads new user
**Automated tests:**
- [x] Unit test: AbortController cleanup
- [x] Unit test: Ignore aborted responses
- [x] Integration test: Rapid navigation scenario
- [x] All existing tests pass (87/87)
**Edge cases:**
- [x] Navigate away while loading - request cancelled
- [x] Navigate to same user - no unnecessary requests
- [x] Network error during navigation - error state shows correctly
**Browser testing:**
- [x] Chrome 120 ✓
- [x] Safari 17 ✓
- [x] Firefox 121 ✓
- [x] Mobile Safari iOS 17 ✓
## Performance Impact
**Before:**
- 3 simultaneous requests for rapid navigation
- Wasted bandwidth
- Wrong data displayed
**After:**
- Only 1 active request at a time
- Previous requests cancelled
- Correct data always displayed
**Metrics:**
- No performance regression
- Actually reduces network traffic (fewer parallel requests)
- Memory usage unchanged
## Risks
**Low risk:**
- Small, focused change (1 file, 10 lines)
- Well-tested pattern (used in production by Facebook, Google)
- No database changes
- No API changes
**Browser support:**
- AbortController supported in all modern browsers
- IE11: Not supported, but we don't support IE11 anymore
**Rollback plan:**
- Feature flag: `USE_ABORT_CONTROLLER` (default: true)
- Can disable if issues arise
- No data migration needed
**Monitoring:**
- Added logging for abort errors (should be 0 in production)
- Dashboard: "Profile Load Success Rate" (should remain 99.8%)
## Screenshots
**Before:** Wrong user data shows after rapid navigation

**After:** Correct user always shows, old requests cancelled

**DevTools Network tab:**
Shows requests being cancelled (status: "cancelled")

**Demo video:** https://www.loom.com/share/abc123
Shows rapid navigation - correct user always displays
## Related
Fixes #1234
Related to #1200 (performance epic)
Similar fix: #1180 (same pattern for posts)
**Documentation updated:**
- Added AbortController pattern to wiki
- Updated `CONTRIBUTING.md` with example
## Reviewer Notes
**Focus areas:**
- `useUserProfile.ts` lines 23-45 - Main change
- `useUserProfile.test.tsx` - New test coverage
**Questions:**
1. Line 34: Should we retry on abort? I chose no, but open to discussion.
2. Line 42: Error logging - is this the right level (debug vs info)?
**Implementation details:**
- The cleanup function (line 45) runs on both unmount AND before next effect
- This ensures we cancel even if userId changes multiple times quickly
- AbortError is expected and not a real error, so we filter it in error logging
**Alternative approaches considered:**
See "Solution" section above. Happy to discuss if you prefer a different approach.
Using AI to Generate PR Descriptions
AI tools like Claude Code, GitHub Copilot, and Claude can dramatically speed up PR description writing. But use them wisely—AI should assist, not replace your thinking.
The AI Workflow
Step 1: Generate the diff context
# Get your changes
git diff main...your-branch > changes.diff
# Or for staged changes
git diff --cached > changes.diff
Step 2: Prompt the AI
Here's my go-to prompt for Claude:
I'm creating a PR. Here's my git diff:
[paste diff]
Please write a PR description following this template:
## Problem
[What bug/need does this address? Impact?]
## Solution
[High-level approach, key decisions]
## Testing
[What testing was done?]
## Risks
[Any concerns or limitations?]
Generate a draft PR description. Focus on the "why" and "what", not
line-by-line code explanation.
Step 3: Review and refine
AI gives you a solid first draft. Now add:
- Business context AI doesn't know
- Testing you actually performed
- Screenshots (AI can't generate these)
- Links to related issues
- Specific reviewer questions
What AI Is Great At
✅ Summarizing code changes
// You changed 15 files
// AI reads the diff and says:
"This PR refactors authentication to use OAuth2 instead of session cookies.
Changes include:
- Replaced SessionAuth with OAuth2Provider
- Updated login flow to redirect to auth server
- Added token refresh logic
- Migrated existing users to new auth system"
✅ Identifying patterns
AI notices: "This change adds null checks in 8 files - looks like you're
fixing a null reference issue throughout the codebase."
✅ Suggesting test cases
AI suggests: "Based on the null checks added, you should test:
- User with null email
- User with empty string email
- User with whitespace-only email"
✅ Writing first draft quickly
Instead of 10 minutes writing from scratch, AI gives you a 2-minute starting point.
What AI Needs Your Help With
❌ Business context
AI doesn't know:
- How many users this affects
- Why this matters to the business
- Customer complaints that triggered this
- Revenue impact
You add:
## Problem
[AI wrote: "Users with null emails can't access their profiles"]
Users with null emails (legacy data from 2019 migration) crash when clicking
"Edit Profile".
**Impact:** ← YOU ADD THIS
- Affects ~500 users (0.5% of active users)
- Support tickets: 12 in last week
- Blocking users from updating notification preferences
❌ Why you chose this approach
AI can see what you did, but not why you rejected alternatives.
You add:
## Solution
[AI wrote: "Added null check before processing email"]
Added null check before processing email
**Why this approach:** ← YOU ADD THIS
Considered three options:
1. Backfill null emails with defaults - rejected (creates invalid data)
2. Disable profile for null emails - rejected (poor UX)
3. Show "Add email" prompt - chosen (guides users to fix root cause)
❌ Actual testing performed
AI can suggest tests, but doesn't know what you actually ran.
You replace AI's generic checklist with specifics:
## Testing
[AI wrote:]
- [ ] Tested happy path
- [ ] Tested edge cases
- [ ] Added unit tests
[You write:]
- [x] Tested with user ID 12345 (has null email) - shows prompt correctly
- [x] Tested with user ID 67890 (has email) - edit form works normally
- [x] Verified with @sarah-qa on staging environment
- [x] Added regression test in profile.test.tsx lines 145-167
GitHub Copilot for PR Descriptions
If you use VS Code with Copilot, you can generate PR descriptions inline:
Method 1: Use Copilot Chat
@workspace Create a PR description for my current changes.
Include problem, solution, and testing sections.
Method 2: In the GitHub PR UI
GitHub Copilot can analyze your diff automatically and suggest:
- PR title
- Description draft
- Relevant reviewers
Pro tip: Copilot is good at titles, decent at summaries, but weak on context. Use it for the skeleton, then flesh it out.
Claude Code for PR Descriptions
Claude Code excels at analyzing codebases and providing context.
Example workflow:
# In your terminal with Claude Code
claude "Analyze my recent changes and write a PR description.
Focus on why these changes matter and what problems they solve."
Claude Code can:
- Read your entire codebase for context
- Understand related files you didn't change
- Identify breaking changes
- Suggest migration paths
Example output:
I analyzed your changes across 12 files. This appears to be migrating
from Redux to Zustand for state management.
## Problem
The current Redux implementation has several issues:
- 200+ lines of boilerplate (actions, reducers, selectors)
- Causes unnecessary re-renders (I see 15+ useSelector calls that
trigger on any state change)
- Difficult to test (requires store wrapper in tests)
## Solution
Migrated to Zustand for cleaner state management:
- Reduced from 200 lines to 45 lines
- Selective subscriptions prevent unnecessary re-renders
- Tests simplified (no Provider needed)
I noticed you kept the old Redux store in `store/legacy/` - is this for
gradual migration?
Advanced: Iterative Refinement with AI
Don't settle for the first draft. Iterate:
Round 1: Generate basic description
Claude, here's my diff. Write a PR description.
Round 2: Add context
Good start. Add that this affects 500 users and we've had 12 support
tickets about it. Also mention we considered backfilling emails but
rejected it because [reasons].
Round 3: Focus on testing
Expand the testing section. I tested with users 12345 and 67890,
verified the fix in staging, and added regression tests. Make it
more specific.
Round 4: Add reviewer notes
Add a "Reviewer Notes" section. I want reviewers to focus on the
null check logic in ProfileForm.tsx and the test coverage in
profile.test.tsx.
The Hybrid Approach (Best Practice)
## My Workflow
1. **AI generates first draft** (2 minutes)
- Paste diff into Claude
- Get basic structure
2. **I add business context** (3 minutes)
- Why this matters
- Impact numbers
- Customer feedback
3. **I add specifics** (3 minutes)
- Actual testing performed
- Decisions made and why
- Risks and concerns
4. **AI polishes** (1 minute)
- "Claude, make this more concise"
- "Reorganize for clarity"
**Total time:** 9 minutes
**Quality:** Better than 30 minutes writing alone
Example: AI-Assisted PR Description
What I gave AI:
Here's my diff. Write a PR description.
[paste 15-file diff for race condition fix]
What AI generated (2 minutes):
## Problem
The application experiences race conditions when rapidly navigating between
user profiles, causing incorrect data to display.
## Solution
Implemented AbortController to cancel in-flight requests when the user
navigates to a different profile.
## Testing
- Tested rapid navigation
- Added unit tests for cleanup
- Verified error handling
What I added (5 minutes):
## Problem
The application experiences race conditions when rapidly navigating between
user profiles, causing incorrect data to display.
**Impact:** ← ADDED
- Reported by 8 users in last 2 weeks
- Affects power users who navigate quickly (~5% of users)
- Privacy concern: Users might see wrong person's data
**Example scenario:** ← ADDED
1. Click Profile A (slow response, 2s)
2. Click Profile B (fast response, 0.5s)
3. B loads correctly
4. A loads and overwrites B (wrong!)
## Solution
Implemented AbortController to cancel in-flight requests when the user
navigates to a different profile.
**Why AbortController:** ← ADDED
- Native browser API (no library needed)
- Standard approach used by React Query, SWR
- Proper way to cancel fetch requests
**Alternatives considered:** ← ADDED
1. Request IDs - too complex
2. Debounce navigation - poor UX
3. AbortController - chosen for simplicity
## Testing
- Tested rapid navigation
- Added unit tests for cleanup
- Verified error handling
**Specifically:** ← ADDED
- [x] Navigated between 10 users in 5 seconds - correct user always shown
- [x] Tested on throttled 3G network
- [x] Verified with @sarah-qa in staging
- [x] Added test in useUserProfile.test.tsx lines 145-167
## Screenshots ← ADDED ENTIRE SECTION
**Before:** Wrong user data after rapid navigation

**After:** Correct user always shows

**Demo:** https://www.loom.com/share/abc123
Fixes #1234 ← ADDED
Result: Professional PR description in 7 minutes instead of 15.
Prompts That Work
For initial draft:
Analyze this git diff and create a PR description with:
1. Problem section (why this change is needed)
2. Solution section (high-level approach)
3. Testing section (what should be tested)
Focus on user impact and business value, not implementation details.
[paste diff]
For refactoring PRs:
This is a refactoring PR. Explain:
1. What problems exist with current code
2. What the refactored approach improves
3. Why this refactor is worth doing now
4. Any migration needed for other developers
[paste diff]
For bug fixes:
This PR fixes a bug. Help me write a description that includes:
1. The exact error users experience
2. Why the bug happens (root cause)
3. How the fix solves it
4. What testing proves it's fixed
[paste diff]
For polishing:
Here's my draft PR description. Make it:
- More concise (remove unnecessary words)
- More specific (replace vague statements)
- Better organized (improve flow)
- Professional (appropriate tone)
[paste your draft]
When NOT to Trust AI
❌ Don't trust AI for:
-
Exact numbers
- AI might say "affects many users"
- You say "affects 500 users (0.5%)"
-
Testing claims
- AI might say "thoroughly tested"
- You say exactly what you tested
-
Business context
- AI doesn't know your customers
- AI doesn't know your roadmap
- AI doesn't know internal discussions
-
Security implications
- AI might miss that you're exposing data
- AI might not flag authentication changes
- Always review security-related descriptions carefully
✅ Do trust AI for:
- Code summaries - What changed across files
- Test suggestions - What you should test
- Structure - Organizing your thoughts
- First draft - Getting started quickly
The AI Safety Checklist
Before submitting an AI-generated PR description: ""
**Review these:**
- [ ] Business context is accurate (not AI assumptions)
- [ ] Numbers are real (not AI estimates)
- [ ] Testing reflects what you ACTUALLY did
- [ ] No hallucinated details (AI making things up)
- [ ] Security implications are addressed
- [ ] Links are correct (AI might invent URLs)
- [ ] Removed AI hedging language ("might", "could", "possibly")
- [ ] Tone is appropriate (not too formal or casual)
Real Example: AI Mistakes to Fix
AI generated:
## Testing
The changes have been thoroughly tested across multiple scenarios and
edge cases. All tests pass successfully.
Problems:
- Vague ("thoroughly tested")
- No specifics
- Sounds like AI boilerplate
You fix it:
## Testing
**Manual:**
- [x] Tested with user ID 12345 (null email) - prompt shows correctly
- [x] Tested with user ID 67890 (valid email) - form works normally
- [x] Rapid clicking "Add email" button - no issues
**Automated:**
- [x] Added unit test: `profile.test.tsx` lines 145-167
- [x] All existing tests pass (87/87)
- [x] Coverage increased from 78% to 82%
Tools Comparison
| Tool | Best For | Limitations |
|---|---|---|
| Claude | Deep analysis, context understanding, refinement | No direct Git integration |
| Claude Code | Full codebase context, terminal integration | Requires Claude subscription |
| GitHub Copilot | In-IDE suggestions, PR UI integration | Less context, shorter responses |
| ChatGPT | General descriptions, formatting help | No code context by default |
My recommendation: Use Claude or Claude Code for complex PRs (refactors, large features). Use Copilot for quick bug fixes and small changes.
The Bottom Line on AI
AI is a 10x multiplier for PR descriptions:
- 15 minutes → 5 minutes with better quality
- Blank page problem solved (AI gives you a start)
- Catches things you forgot (suggested tests, edge cases)
But AI is not a replacement:
- You still need to add context
- You still need to verify accuracy
- You still need to add human judgment
Best practice:
AI writes the skeleton.
You add the soul.
Use AI to:
- ✅ Generate structure quickly
- ✅ Summarize code changes
- ✅ Suggest test cases
- ✅ Polish your writing
Don't use AI to:
- ❌ Replace your thinking
- ❌ Make up business context
- ❌ Claim testing you didn't do
- ❌ Skip the review process
The workflow that works:
- Let AI generate first draft (2 min)
- Add your knowledge (5 min)
- Let AI polish (1 min)
Total: 8 minutes for a great PR description.
Conclusion
Good PR descriptions:
- ✅ Save reviewers time
- ✅ Get faster approvals
- ✅ Reduce back-and-forth
- ✅ Prevent bugs from slipping through
- ✅ Create better documentation
Bad PR descriptions:
- ❌ Waste reviewers' time
- ❌ Sit for days
- ❌ Require multiple review rounds
- ❌ Let bugs slip through
- ❌ Leave no documentation
The formula:
Problem (why) + Solution (what) + Testing (proof) = Fast approval
My challenge to you:
Tomorrow, write a PR description using this template. Time yourself:
- How long to write: ~10 minutes
- How long to get approved: Probably 10x faster
That 10 minutes saves everyone hours.
What's your PR description template? Share your best practices!
Top comments (0)