gh-propstreet-claude-powerpack/skills/update-pr/EXAMPLES.md

PR Description Examples

Examples of comprehensive vs incomplete PR descriptions.

Good Example: Complete Coverage

This example shows a PR that properly documents multiple categories of changes:

## Summary

Refactors UserService to eliminate 600 lines of duplication by extracting common
validation logic. Additionally includes critical bug fixes for session handling,
EF Core query optimization, and test infrastructure improvements.

## User Impact

**User Management:**
- More consistent validation behavior across all user operations
- Better error messages when validation fails

**Reliability:**
- Bug fix: Sessions no longer expire prematurely during long operations
- Bug fix: Race condition in concurrent user updates resolved

**Performance:**
- 40% faster user lookup queries through optimized includes

## Technical Notes

### 1. UserService Refactoring

Extracted common validation logic into `UserValidationService`:

- **Before**: 3 nearly-identical validation methods across `Create`, `Update`, `Import`
- **After**: Single `ValidateUser()` method with operation-specific extensions
- **Files**: `UserService.cs`, `UserValidationService.cs` (new)

### 2. Bug Fixes

**Session Expiration (Critical)**
- **Location**: `SessionManager.cs:125`
- **Problem**: Timeout calculated from session start, not last activity
- **Impact**: Users logged out during long form submissions
- **Fix**: Track last activity timestamp, reset on each request

**Concurrent Update Race Condition**
- **Location**: `UserRepository.cs:89`
- **Problem**: No optimistic concurrency on user updates
- **Impact**: Last write wins, potentially losing data
- **Fix**: Added `RowVersion` column with EF Core concurrency token

### 3. Query Optimization

- **Location**: `UserRepository.cs:45`
- **Problem**: N+1 query pattern when loading users with roles
- **Fix**: Added explicit `.Include(u => u.Roles)` with split query
- **Measured**: 40% reduction in query time for user list endpoint

### 4. Test Infrastructure

- New `UserTestFixture` base class for consistent test setup
- Extracted common assertions to `UserAssertions` helper
- Added integration test for concurrent update scenario

### 5. Configuration

- Added `SessionTimeoutMinutes` to `appsettings.json` (default: 30)
- New `ConcurrencyRetryCount` setting for optimistic concurrency retries

## Testing

Total: 127 tests Passed: 127 Failed: 0 New tests: 8

- Unit tests for validation extraction
- Integration test for session timeout behavior
- Concurrency test for race condition fix

## Implementation Approach

1. **feat: extract user validation service** - Core refactoring work
2. **fix: session timeout calculation** - Critical bug fix
3. **fix: add optimistic concurrency to user updates** - Race condition fix
4. **perf: optimize user query includes** - Query performance
5. **test: add user service test fixtures** - Test infrastructure
6. **chore: add session configuration** - Configuration changes

---

Generated with [Claude Code](https://claude.com/claude-code)

Why This Is Good

• Documents ALL commits, not just the main feature
• Bug fixes are prominently highlighted with impact
• Performance improvement is measured and documented
• Test coverage is quantified
• Configuration changes are noted
• Each section has specific file references

---

Bad Example: Incomplete Coverage

This example shows what to avoid:

## Summary

Refactors UserService to eliminate duplication.

## Technical Notes

- New base class
- Moved duplicate code
- Tests passing

## Testing

All tests pass.

Why This Is Bad

• Missing bug fixes: The session and concurrency fixes aren't mentioned
• No impact context: Doesn't explain WHY the refactoring matters
• Vague descriptions: "Moved duplicate code" tells reviewers nothing
• Missing commits: Only describes 1 of 6 commits
• No configuration changes: Settings changes completely omitted
• No performance details: Query optimization not mentioned
• No file references: Reviewers don't know where to look

---

Example: Feature with Multiple Side Effects

When a feature PR includes necessary side effects:

Good

## Summary

Adds export-to-CSV functionality for reports. Also fixes date formatting
inconsistency discovered during development and adds missing null checks
in the report generator.

## User Impact

**New Feature:**
- Users can now export any report to CSV format
- Supports all report types (daily, weekly, monthly)

**Bug Fixes:**
- Dates now display consistently in user's timezone across all reports
- Reports no longer fail when optional fields are null

## Technical Notes

### 1. CSV Export Feature
[Details of main feature]

### 2. Date Formatting Fix
- **Location**: `DateFormatter.cs:45`
- **Problem**: Some dates used UTC, others used local time
- **Fix**: Standardized on user's configured timezone

### 3. Null Safety
- **Location**: `ReportGenerator.cs:112, 156, 203`
- **Problem**: Null reference exceptions on optional fields
- **Fix**: Added null-conditional operators and default values

Bad

## Summary

Adds CSV export for reports.

## Changes

- Added ExportService
- Updated ReportController
- Fixed some bugs

The bad version:

• Hides important bug fixes under "Fixed some bugs"
• Doesn't explain the impact of the bugs that were fixed
• Reviewers might miss that this PR changes date behavior

---

Example: Infrastructure-Heavy PR

When the main work is test/infrastructure improvements:

Good

## Summary

Overhauls test infrastructure to support parallel execution, reducing CI
time from 12 minutes to 4 minutes. Includes migration of 45 test classes
to new fixture pattern.

## User Impact

**Developer Experience:**
- CI feedback 3x faster
- Local test runs significantly quicker
- Tests now properly isolated (no more flaky failures)

## Technical Notes

### 1. Parallel Test Execution
- **Problem**: Tests ran sequentially due to shared database state
- **Solution**: New `IsolatedDatabaseFixture` creates per-test databases
- **Result**: Full parallelization across all CPU cores

### 2. Test Migration
- Migrated 45 test classes to new fixture pattern
- Removed hardcoded IDs that caused parallel conflicts
- Added scoped assertions that filter to test-created data

### 3. Performance Results
| Metric | Before | After |
|--------|--------|-------|
| CI Duration | 12:34 | 4:12 |
| Local (8 cores) | 8:45 | 1:23 |
| Flaky test rate | 12% | 0% |

## Testing

All 312 tests pass in parallel mode.

Bad

## Summary

Updated tests.

## Changes

- Changed test base class
- Updated 45 files

---

Checklist for Complete Coverage

Before finalizing your PR description, verify:

• Every commit message is reflected in the description
• Bug fixes are prominently documented (not hidden)
• Performance improvements include measurements
• Configuration changes are listed
• Test coverage changes are quantified
• File paths are included for key changes
• User impact is clearly separated from technical details
• "Why" is explained, not just "what"