zhongwei/gh-propstreet-claude-powerpack
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Initial commit

Browse Source
This commit is contained in:
Zhongwei Li
2025-11-30 08:48:32 +08:00
commit b0bc2cf2e3
11 changed files with 2373 additions and 0 deletions
Download Patch File Download Diff File Expand all files Collapse all files

271
skills/update-pr/EXAMPLES.md Normal file
View File

@@ -0,0 +1,271 @@
# 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:
```markdown
## 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:
```markdown
## 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
```markdown
## 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
```markdown
## 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
```markdown
## 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
```markdown
## 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"