Initial commit
This commit is contained in:
Zhongwei Li
851
skills/documentation-pr/SKILL.md
Normal file
851
skills/documentation-pr/SKILL.md
Normal file
@@ -0,0 +1,851 @@
|
||||
---
|
||||
name: oss:documentation-pr
|
||||
description: Phase 4 of OSS contribution - Document changes and create a comprehensive, reviewable pull request. Writes clear PR description, documents code changes, creates changelog entries, and prepares for review. Use when implementation is complete and ready to submit work.
|
||||
---
|
||||
|
||||
# Phase 4: Documentation & PR Creation
|
||||
|
||||
Document changes and create a comprehensive, reviewable pull request.
|
||||
|
||||
## Purpose
|
||||
|
||||
Create a pull request that:
|
||||
- Clearly communicates changes
|
||||
- Justifies the approach
|
||||
- Makes reviewer's job easy
|
||||
- Increases merge probability
|
||||
- Serves as documentation
|
||||
|
||||
## When to Use
|
||||
|
||||
**Triggers:**
|
||||
- "PR 작성"
|
||||
- "pull request 준비"
|
||||
- "문서화해줘"
|
||||
- "리뷰 준비"
|
||||
|
||||
**Use when:**
|
||||
- Implementation is complete
|
||||
- Tests pass locally
|
||||
- Ready to submit work
|
||||
- Want to create high-quality PR
|
||||
|
||||
## PR Creation Framework
|
||||
|
||||
### Step 0: CONTRIBUTING.md Final Verification
|
||||
|
||||
**MANDATORY: Final compliance check before PR submission**
|
||||
- Review CONTRIBUTING.md requirements from Phase 1 one last time
|
||||
- Verify ALL requirements are met:
|
||||
- Code style and formatting standards
|
||||
- Commit message format and conventions
|
||||
- Branch naming requirements
|
||||
- Testing requirements
|
||||
- Documentation standards
|
||||
- PR submission process
|
||||
- **PR MUST strictly follow all CONTRIBUTING.md guidelines**
|
||||
|
||||
### Step 1: Pre-PR Checklist
|
||||
|
||||
Verify everything is ready before creating PR.
|
||||
|
||||
```markdown
|
||||
### Pre-PR Checklist
|
||||
|
||||
**CONTRIBUTING.md Compliance:**
|
||||
- [ ] All contribution guidelines followed
|
||||
- [ ] Commit messages follow required format
|
||||
- [ ] Branch named according to conventions
|
||||
- [ ] Code style matches project standards
|
||||
|
||||
**Code quality:**
|
||||
- [ ] All tests pass locally
|
||||
- [ ] Linting passes
|
||||
- [ ] Type checking passes (if applicable)
|
||||
- [ ] Build succeeds
|
||||
- [ ] No compiler warnings
|
||||
|
||||
**Functionality:**
|
||||
- [ ] All requirements implemented
|
||||
- [ ] Edge cases handled
|
||||
- [ ] Error handling complete
|
||||
- [ ] Manual testing done
|
||||
|
||||
**Tests:**
|
||||
- [ ] New tests added
|
||||
- [ ] Existing tests still pass
|
||||
- [ ] Coverage meets threshold
|
||||
- [ ] Tests are meaningful
|
||||
|
||||
**Documentation:**
|
||||
- [ ] Code comments added where needed
|
||||
- [ ] README updated (if needed)
|
||||
- [ ] CHANGELOG entry added
|
||||
- [ ] API docs updated (if applicable)
|
||||
|
||||
**Git hygiene:**
|
||||
- [ ] Branch is up to date with main
|
||||
- [ ] Commits are logical and focused
|
||||
- [ ] Commit messages are clear
|
||||
- [ ] No merge commits (rebased if needed)
|
||||
- [ ] No secrets or sensitive data
|
||||
|
||||
**Review readiness:**
|
||||
- [ ] Self-reviewed all changes
|
||||
- [ ] Removed debugging code
|
||||
- [ ] Removed commented-out code
|
||||
- [ ] No unrelated changes
|
||||
```
|
||||
|
||||
### Step 2: Branch Management
|
||||
|
||||
Ensure clean branch state.
|
||||
|
||||
**Branch best practices:**
|
||||
|
||||
**IMPORTANT: Follow CONTRIBUTING.md branch naming conventions**
|
||||
|
||||
```bash
|
||||
# Create feature branch following project conventions
|
||||
# Check CONTRIBUTING.md for required format (e.g., feature/, fix/, etc.)
|
||||
git checkout -b feature/issue-123-description
|
||||
|
||||
# Update from main
|
||||
git fetch origin
|
||||
git rebase origin/main
|
||||
|
||||
# Check status
|
||||
git status
|
||||
# Should be clean, ahead of main
|
||||
|
||||
# View your commits
|
||||
git log origin/main..HEAD --oneline
|
||||
# Should be focused, logical commits
|
||||
|
||||
# If commits need cleanup, interactive rebase
|
||||
git rebase -i origin/main
|
||||
# Squash, reword, reorder as needed
|
||||
```
|
||||
|
||||
**Commit message quality:**
|
||||
|
||||
**IMPORTANT: Follow CONTRIBUTING.md commit message format**
|
||||
- Check project's required commit message convention
|
||||
- Some projects use Conventional Commits, others have custom formats
|
||||
- Verify before writing commit messages
|
||||
|
||||
```markdown
|
||||
### Good Commit Messages
|
||||
|
||||
**Format (verify with CONTRIBUTING.md):**
|
||||
```
|
||||
type(scope): brief description
|
||||
|
||||
Detailed explanation of what and why (not how).
|
||||
Focus on the motivation and context.
|
||||
|
||||
Closes #123
|
||||
```
|
||||
|
||||
**Types:**
|
||||
- feat: New feature
|
||||
- fix: Bug fix
|
||||
- docs: Documentation only
|
||||
- style: Formatting, no code change
|
||||
- refactor: Code change without behavior change
|
||||
- test: Adding tests
|
||||
- chore: Build process, tooling
|
||||
|
||||
**Examples:**
|
||||
|
||||
✅ **Good:**
|
||||
```
|
||||
fix(auth): handle null user in session validation
|
||||
|
||||
Previously, the session validator crashed when user
|
||||
was null during logout race conditions. Now returns
|
||||
early with invalid session.
|
||||
|
||||
Closes #123
|
||||
```
|
||||
|
||||
❌ **Bad:**
|
||||
```
|
||||
fix stuff
|
||||
```
|
||||
|
||||
✅ **Good:**
|
||||
```
|
||||
feat(export): add CSV export functionality
|
||||
|
||||
Implements CSV export with customizable columns and
|
||||
optional header row. Uses streaming for large datasets
|
||||
to avoid memory issues.
|
||||
|
||||
- Add exportToCSV function
|
||||
- Add column selection UI
|
||||
- Add tests for edge cases
|
||||
|
||||
Closes #456
|
||||
```
|
||||
|
||||
❌ **Bad:**
|
||||
```
|
||||
added feature
|
||||
```
|
||||
```
|
||||
|
||||
### Step 3: PR Title
|
||||
|
||||
Craft clear, descriptive PR title.
|
||||
|
||||
**Title format:**
|
||||
|
||||
```markdown
|
||||
### PR Title
|
||||
|
||||
**Format:** [Type] Brief description of change
|
||||
|
||||
**Examples:**
|
||||
|
||||
✅ Good titles:
|
||||
- "Fix: Handle null values in session validation"
|
||||
- "Feature: Add CSV export with column selection"
|
||||
- "Refactor: Extract validation logic to separate module"
|
||||
- "Docs: Add examples for authentication flow"
|
||||
|
||||
❌ Bad titles:
|
||||
- "Fix bug"
|
||||
- "Update code"
|
||||
- "Changes"
|
||||
- "PR for issue #123"
|
||||
|
||||
**Guidelines:**
|
||||
- Start with type: Fix/Feature/Refactor/Docs
|
||||
- Use imperative mood ("Add" not "Added")
|
||||
- Be specific but concise
|
||||
- Mention issue number if applicable
|
||||
- Max ~60-70 characters
|
||||
```
|
||||
|
||||
### Step 4: PR Description
|
||||
|
||||
Write comprehensive PR description.
|
||||
|
||||
**Description template:**
|
||||
|
||||
```markdown
|
||||
## PR Description Template
|
||||
|
||||
### Summary
|
||||
[2-3 sentences: What changes, why they're needed, what problem they solve]
|
||||
|
||||
### Changes Made
|
||||
- [Change 1: specific, actionable description]
|
||||
- [Change 2]
|
||||
- [Change 3]
|
||||
|
||||
### Type of Change
|
||||
- [ ] Bug fix (non-breaking change which fixes an issue)
|
||||
- [ ] New feature (non-breaking change which adds functionality)
|
||||
- [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)
|
||||
- [ ] Documentation update
|
||||
- [ ] Refactoring (no functional changes)
|
||||
- [ ] Performance improvement
|
||||
- [ ] Test coverage improvement
|
||||
|
||||
### Related Issue
|
||||
Fixes #[issue-number]
|
||||
<!-- or -->
|
||||
Relates to #[issue-number]
|
||||
|
||||
### How to Test
|
||||
1. [Step 1: how to set up test scenario]
|
||||
2. [Step 2: what to do]
|
||||
3. [Step 3: what to verify]
|
||||
|
||||
**Expected behavior:** [what should happen]
|
||||
|
||||
### Screenshots (if applicable)
|
||||
**Before:**
|
||||
[screenshot or description]
|
||||
|
||||
**After:**
|
||||
[screenshot or description]
|
||||
|
||||
### Checklist
|
||||
- [ ] My code follows the project's style guidelines
|
||||
- [ ] I have performed a self-review of my code
|
||||
- [ ] I have commented my code, particularly in hard-to-understand areas
|
||||
- [ ] I have made corresponding changes to the documentation
|
||||
- [ ] My changes generate no new warnings
|
||||
- [ ] I have added tests that prove my fix is effective or that my feature works
|
||||
- [ ] New and existing unit tests pass locally with my changes
|
||||
- [ ] Any dependent changes have been merged and published
|
||||
|
||||
### Additional Notes
|
||||
[Any context, tradeoffs, alternative approaches considered, future work, or breaking changes]
|
||||
```
|
||||
|
||||
**Customized example:**
|
||||
|
||||
```markdown
|
||||
## Add CSV Export Functionality
|
||||
|
||||
### Summary
|
||||
Implements CSV export feature requested in #456. Users can now export data
|
||||
tables to CSV format with customizable column selection and optional headers.
|
||||
Uses streaming approach to handle large datasets without memory issues.
|
||||
|
||||
### Changes Made
|
||||
- Add `exportToCSV` function in `utils/export.js`
|
||||
- Add column selection checkbox UI in export dialog
|
||||
- Add "Include Headers" toggle option
|
||||
- Implement streaming for datasets >10k rows
|
||||
- Add comprehensive tests for edge cases (empty data, special characters, large datasets)
|
||||
- Update README with export feature documentation
|
||||
|
||||
### Type of Change
|
||||
- [x] New feature (non-breaking change which adds functionality)
|
||||
|
||||
### Related Issue
|
||||
Fixes #456
|
||||
|
||||
### How to Test
|
||||
1. Navigate to any data table page
|
||||
2. Click "Export" button in toolbar
|
||||
3. Select columns to export using checkboxes
|
||||
4. Toggle "Include Headers" option
|
||||
5. Click "Download CSV"
|
||||
6. Verify downloaded file contains expected data
|
||||
|
||||
**Expected behavior:** CSV file downloads with selected columns, properly escaped special characters, and headers if enabled.
|
||||
|
||||
**Edge cases to test:**
|
||||
- Empty dataset → Downloads empty file or shows warning
|
||||
- Large dataset (>10k rows) → Progress indicator shows, no memory issues
|
||||
- Special characters in data → Properly escaped in CSV
|
||||
|
||||
### Screenshots
|
||||
**Export Dialog:**
|
||||
[screenshot of new export UI]
|
||||
|
||||
**Sample Output:**
|
||||
```csv
|
||||
Name,Email,Role
|
||||
John Doe,john@example.com,Admin
|
||||
Jane Smith,jane@example.com,User
|
||||
```
|
||||
|
||||
### Checklist
|
||||
- [x] My code follows the project's style guidelines
|
||||
- [x] I have performed a self-review of my code
|
||||
- [x] I have commented my code, particularly in hard-to-understand areas
|
||||
- [x] I have made corresponding changes to the documentation
|
||||
- [x] My changes generate no new warnings
|
||||
- [x] I have added tests that prove my fix is effective or that my feature works
|
||||
- [x] New and existing unit tests pass locally with my changes
|
||||
- [x] Any dependent changes have been merged and published
|
||||
|
||||
### Additional Notes
|
||||
|
||||
**Design decisions:**
|
||||
- Used streaming for large datasets instead of loading all in memory
|
||||
- Followed RFC 4180 for CSV format to ensure compatibility
|
||||
- Made column selection persistent in localStorage
|
||||
|
||||
**Future improvements (out of scope for this PR):**
|
||||
- Add Excel export format
|
||||
- Add export templates
|
||||
- Add scheduled exports
|
||||
|
||||
**Breaking changes:** None
|
||||
```
|
||||
|
||||
### Step 5: Project-Specific PR Templates
|
||||
|
||||
Adapt to project's PR template if exists.
|
||||
|
||||
**Check for templates:**
|
||||
```bash
|
||||
# Look for PR template
|
||||
cat .github/PULL_REQUEST_TEMPLATE.md
|
||||
cat .github/pull_request_template.md
|
||||
cat docs/pull_request_template.md
|
||||
|
||||
# Some projects use issue templates
|
||||
ls .github/ISSUE_TEMPLATE/
|
||||
```
|
||||
|
||||
**Follow template exactly:**
|
||||
- Don't skip sections
|
||||
- Fill in all required fields
|
||||
- Check all relevant boxes
|
||||
- Provide requested information
|
||||
|
||||
**If no template:**
|
||||
- Use framework's template above
|
||||
- Look at recent merged PRs for format
|
||||
- Follow community conventions
|
||||
|
||||
### Step 6: Review Preparation
|
||||
|
||||
Make reviewer's job easy.
|
||||
|
||||
**Reviewer-friendly practices:**
|
||||
|
||||
```markdown
|
||||
### Making PR Easy to Review
|
||||
|
||||
**Size:**
|
||||
- 🟢 Small: < 200 lines changed
|
||||
- 🟡 Medium: 200-500 lines
|
||||
- 🔴 Large: > 500 lines (consider splitting)
|
||||
|
||||
**If PR is large:**
|
||||
- Explain why it can't be split
|
||||
- Provide roadmap of changes
|
||||
- Highlight key areas to review
|
||||
- Offer to review in parts
|
||||
|
||||
**Structure:**
|
||||
- Logical commit history
|
||||
- Each commit compiles/works
|
||||
- Related changes grouped
|
||||
- Unrelated changes separated
|
||||
|
||||
**Communication:**
|
||||
- Clear descriptions
|
||||
- Inline comments on tricky code
|
||||
- Link to design docs
|
||||
- Explain tradeoffs
|
||||
|
||||
**Context:**
|
||||
- Why this approach?
|
||||
- What alternatives considered?
|
||||
- Any performance implications?
|
||||
- Breaking changes?
|
||||
```
|
||||
|
||||
**Add code comments in PR:**
|
||||
|
||||
Use GitHub's review comment feature to explain:
|
||||
- Why specific approach taken
|
||||
- Known limitations
|
||||
- Areas you want feedback on
|
||||
- Anything non-obvious
|
||||
|
||||
**Example:**
|
||||
```javascript
|
||||
// (Add PR comment: "This uses binary search instead of linear scan
|
||||
// because dataset can be large. Benchmarked 100x faster on 10k items.")
|
||||
const index = binarySearch(array, target)
|
||||
```
|
||||
|
||||
### Step 7: CI/CD Preparation
|
||||
|
||||
Ensure automated checks will pass.
|
||||
|
||||
```markdown
|
||||
### CI/CD Checklist
|
||||
|
||||
**Before pushing:**
|
||||
- [ ] All tests pass locally
|
||||
- [ ] Linting passes
|
||||
- [ ] Type checking passes
|
||||
- [ ] Build succeeds
|
||||
- [ ] Coverage meets threshold
|
||||
|
||||
**After pushing:**
|
||||
- [ ] Monitor CI/CD pipeline
|
||||
- [ ] All checks pass
|
||||
- [ ] No flaky test failures
|
||||
- [ ] Build artifacts generated (if applicable)
|
||||
|
||||
**If CI fails:**
|
||||
- Fix immediately
|
||||
- Don't wait for reviewer
|
||||
- Force push if fixing commits
|
||||
- Comment explaining fixes
|
||||
```
|
||||
|
||||
**Common CI failures:**
|
||||
|
||||
```markdown
|
||||
### Troubleshooting CI
|
||||
|
||||
**Tests fail in CI but pass locally:**
|
||||
- [ ] Check for timezone assumptions
|
||||
- [ ] Check for file path assumptions (Windows vs Unix)
|
||||
- [ ] Check for race conditions
|
||||
- [ ] Check for missing test data
|
||||
- [ ] Check for environment differences
|
||||
|
||||
**Linting fails:**
|
||||
```bash
|
||||
# Run same linter locally
|
||||
npm run lint
|
||||
# Auto-fix if possible
|
||||
npm run lint:fix
|
||||
```
|
||||
|
||||
**Build fails:**
|
||||
```bash
|
||||
# Clean build locally
|
||||
rm -rf node_modules dist
|
||||
npm install
|
||||
npm run build
|
||||
```
|
||||
|
||||
**Coverage below threshold:**
|
||||
```bash
|
||||
# Check coverage locally
|
||||
npm run test:coverage
|
||||
# Add missing tests
|
||||
```
|
||||
```
|
||||
|
||||
### Step 8: PR Submission
|
||||
|
||||
Submit PR and engage with feedback.
|
||||
|
||||
**Submission process:**
|
||||
|
||||
```bash
|
||||
# Push branch to remote
|
||||
git push -u origin feature/issue-123-description
|
||||
|
||||
# Create PR via CLI (if using gh)
|
||||
gh pr create --title "Fix: Handle null user in session" \
|
||||
--body-file pr-description.md \
|
||||
--label bug \
|
||||
--assignee @me
|
||||
|
||||
# Or via GitHub web interface
|
||||
# 1. Go to repository
|
||||
# 2. Click "Pull requests" tab
|
||||
# 3. Click "New pull request"
|
||||
# 4. Select your branch
|
||||
# 5. Fill in title and description
|
||||
# 6. Click "Create pull request"
|
||||
```
|
||||
|
||||
**After submission:**
|
||||
|
||||
```markdown
|
||||
### Post-Submission Actions
|
||||
|
||||
**Immediately:**
|
||||
- [ ] Comment on original issue linking to PR
|
||||
"Submitted PR #789 to address this issue"
|
||||
- [ ] Add appropriate labels (if permissions allow)
|
||||
- [ ] Request review from maintainers (if process requires)
|
||||
- [ ] Link any related issues/PRs
|
||||
|
||||
**Monitor:**
|
||||
- [ ] CI/CD status - fix if failing
|
||||
- [ ] Review comments - respond promptly
|
||||
- [ ] Merge conflicts - resolve quickly
|
||||
- [ ] Feedback - address constructively
|
||||
|
||||
**Be responsive:**
|
||||
- Respond to comments within 24-48 hours
|
||||
- Thank reviewers for feedback
|
||||
- Explain reasoning if disagreeing
|
||||
- Make requested changes promptly
|
||||
- Keep discussion respectful and professional
|
||||
```
|
||||
|
||||
### Step 9: Handling Feedback
|
||||
|
||||
Respond to review comments effectively.
|
||||
|
||||
**Review response best practices:**
|
||||
|
||||
```markdown
|
||||
### Responding to Reviews
|
||||
|
||||
**Good practices:**
|
||||
|
||||
✅ **Acknowledge feedback:**
|
||||
"Good catch! I'll fix this."
|
||||
"That's a great point. Let me refactor this."
|
||||
|
||||
✅ **Explain reasoning:**
|
||||
"I used approach X because Y. However, I see your point about Z. Let me try W instead."
|
||||
|
||||
✅ **Ask clarifying questions:**
|
||||
"I'm not sure I understand the concern here. Could you elaborate on the edge case you're thinking of?"
|
||||
|
||||
✅ **Suggest alternatives:**
|
||||
"Would you prefer approach A or B? I think A is simpler but B is more extensible."
|
||||
|
||||
✅ **Mark resolved:**
|
||||
After addressing comment, reply "Done" or "Fixed in [commit]" and resolve thread
|
||||
|
||||
❌ **Avoid:**
|
||||
- Defensive responses
|
||||
- Ignoring feedback
|
||||
- Taking criticism personally
|
||||
- Arguing unnecessarily
|
||||
- Making excuses
|
||||
|
||||
**Types of feedback:**
|
||||
|
||||
**1. Bugs/Issues (Must fix):**
|
||||
- Fix immediately
|
||||
- Add test to prevent regression
|
||||
- Thank reviewer
|
||||
|
||||
**2. Style/Convention (Must fix):**
|
||||
- Follow project standards
|
||||
- Even if you disagree
|
||||
- Consistency matters
|
||||
|
||||
**3. Suggestions (Evaluate):**
|
||||
- Consider merit
|
||||
- Discuss tradeoffs
|
||||
- Implement if better
|
||||
- Explain if not
|
||||
|
||||
**4. Questions (Answer):**
|
||||
- Clarify approach
|
||||
- Add comments if unclear
|
||||
- May indicate code needs simplification
|
||||
|
||||
**5. Nitpicks (Optional):**
|
||||
- Fix if easy
|
||||
- Push back if not valuable
|
||||
- "Will fix in follow-up" if time-consuming
|
||||
```
|
||||
|
||||
**Making changes:**
|
||||
|
||||
```bash
|
||||
# Make requested changes
|
||||
[edit files]
|
||||
|
||||
# Commit changes
|
||||
git add [files]
|
||||
git commit -m "Address review feedback: improve error handling"
|
||||
|
||||
# Push to PR
|
||||
git push
|
||||
|
||||
# Comment on review
|
||||
"Changes made in [commit-hash]"
|
||||
```
|
||||
|
||||
### Step 10: Merge Preparation
|
||||
|
||||
Final steps before merge.
|
||||
|
||||
```markdown
|
||||
### Pre-Merge Checklist
|
||||
|
||||
**Code review:**
|
||||
- [ ] All reviewer comments addressed
|
||||
- [ ] Requested changes made
|
||||
- [ ] Approvals received (per project policy)
|
||||
- [ ] No unresolved threads
|
||||
|
||||
**CI/CD:**
|
||||
- [ ] All checks passing
|
||||
- [ ] No merge conflicts
|
||||
- [ ] Branch up to date with main
|
||||
|
||||
**Final review:**
|
||||
- [ ] Re-review your changes
|
||||
- [ ] Check for any last-minute issues
|
||||
- [ ] Verify all commits are squashed (if project requires)
|
||||
- [ ] Verify commit message is clean
|
||||
|
||||
**Documentation:**
|
||||
- [ ] CHANGELOG updated
|
||||
- [ ] README current
|
||||
- [ ] Migration guide (if breaking)
|
||||
- [ ] Release notes drafted (if applicable)
|
||||
|
||||
**Ready to merge:**
|
||||
- [ ] Maintainer approval received
|
||||
- [ ] All checks passed
|
||||
- [ ] No outstanding concerns
|
||||
- [ ] Follow-up issues created (if any)
|
||||
```
|
||||
|
||||
## PR Quality Checklist
|
||||
|
||||
Use this to ensure high-quality PRs:
|
||||
|
||||
```markdown
|
||||
### PR Quality Checklist
|
||||
|
||||
**Clarity:**
|
||||
- [ ] Title is clear and descriptive
|
||||
- [ ] Description explains what and why
|
||||
- [ ] Changes are well-organized
|
||||
- [ ] Context is provided
|
||||
|
||||
**Completeness:**
|
||||
- [ ] All requirements addressed
|
||||
- [ ] Tests included
|
||||
- [ ] Documentation updated
|
||||
- [ ] Edge cases handled
|
||||
|
||||
**Reviewability:**
|
||||
- [ ] PR is focused (single concern)
|
||||
- [ ] Size is reasonable
|
||||
- [ ] Commits are logical
|
||||
- [ ] Code is self-explanatory
|
||||
|
||||
**Technical:**
|
||||
- [ ] Follows conventions
|
||||
- [ ] No obvious issues
|
||||
- [ ] Tests pass
|
||||
- [ ] CI passes
|
||||
|
||||
**Communication:**
|
||||
- [ ] Original issue linked
|
||||
- [ ] How to test explained
|
||||
- [ ] Tradeoffs documented
|
||||
- [ ] Breaking changes highlighted
|
||||
```
|
||||
|
||||
## Common Pitfalls
|
||||
|
||||
**Avoid:**
|
||||
|
||||
❌ **Vague descriptions** - "Fixed bug" tells nothing
|
||||
❌ **Huge PRs** - Hard to review, less likely to merge
|
||||
❌ **Mixing concerns** - Multiple unrelated changes
|
||||
❌ **No tests** - Reviewers will ask for them
|
||||
❌ **Ignoring CI failures** - Shows lack of diligence
|
||||
❌ **Poor commit messages** - Makes history useless
|
||||
❌ **Defensive attitude** - Makes collaboration difficult
|
||||
❌ **Rushing** - Quality beats speed
|
||||
|
||||
## Output Format
|
||||
|
||||
Provide PR creation guide:
|
||||
|
||||
```markdown
|
||||
# 📤 Pull Request Ready: [Issue Title]
|
||||
|
||||
**Issue:** #[number]
|
||||
**Branch:** [branch-name]
|
||||
**Status:** Ready to submit
|
||||
|
||||
---
|
||||
|
||||
## PR Information
|
||||
|
||||
**Title:**
|
||||
```
|
||||
[Type]: [Clear description]
|
||||
```
|
||||
|
||||
**Description:**
|
||||
```
|
||||
[Full PR description using template]
|
||||
```
|
||||
|
||||
**Labels:** [suggested labels]
|
||||
|
||||
---
|
||||
|
||||
## Pre-Submission Checklist
|
||||
|
||||
**Code:**
|
||||
- ✅ Tests pass
|
||||
- ✅ Linting passes
|
||||
- ✅ Build succeeds
|
||||
- ✅ Self-reviewed
|
||||
|
||||
**Documentation:**
|
||||
- ✅ Comments added
|
||||
- ✅ README updated
|
||||
- ✅ CHANGELOG entry
|
||||
- ✅ API docs updated
|
||||
|
||||
**Git:**
|
||||
- ✅ Branch up to date
|
||||
- ✅ Commits clean
|
||||
- ✅ No secrets
|
||||
- ✅ No merge commits
|
||||
|
||||
---
|
||||
|
||||
## Submission Command
|
||||
|
||||
```bash
|
||||
# Push branch
|
||||
git push -u origin [branch-name]
|
||||
|
||||
# Create PR (via gh CLI)
|
||||
gh pr create --title "[title]" --body "[description]"
|
||||
|
||||
# Or use GitHub web interface
|
||||
# https://github.com/[owner]/[repo]/compare/[branch]
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Post-Submission
|
||||
|
||||
**Next steps:**
|
||||
1. Monitor CI/CD pipeline
|
||||
2. Respond to review comments promptly
|
||||
3. Address feedback constructively
|
||||
4. Keep PR updated with main branch
|
||||
|
||||
**Timeline:**
|
||||
- Review typically within: [project-specific]
|
||||
- Address feedback within: 24-48 hours
|
||||
- Merge after: approvals + CI pass
|
||||
|
||||
---
|
||||
|
||||
## 🎉 Contribution Complete!
|
||||
|
||||
Thank you for contributing to open source!
|
||||
|
||||
**After merge:**
|
||||
- [ ] Close related issues
|
||||
- [ ] Update local main branch
|
||||
- [ ] Delete feature branch
|
||||
- [ ] Celebrate! 🎉
|
||||
|
||||
**Future contributions:**
|
||||
- Consider more complex issues
|
||||
- Help review others' PRs
|
||||
- Improve documentation
|
||||
- Engage with community
|
||||
```
|
||||
|
||||
## Integration with Main Framework
|
||||
|
||||
When invoked from main framework:
|
||||
|
||||
1. **Receive context:** Implemented changes from Phase 3, test results, branch state
|
||||
2. **Guide documentation:** PR title, description, checklist
|
||||
3. **Verify readiness:** All quality checks met
|
||||
4. **Return PR content:** Ready to copy/paste or submit
|
||||
5. **Update tracker:** Mark Phase 4 complete - contribution done!
|
||||
|
||||
This completes the full OSS contribution workflow.
|
||||
|
||||
## Learning from Reviews
|
||||
|
||||
**Track feedback patterns:**
|
||||
- What do reviewers commonly ask for?
|
||||
- What mistakes do you repeat?
|
||||
- What can you catch in self-review?
|
||||
- How to improve next PR?
|
||||
|
||||
**Build reputation:**
|
||||
- High-quality PRs
|
||||
- Responsive to feedback
|
||||
- Respectful communication
|
||||
- Consistent contributions
|
||||
|
||||
**Each PR is practice for the next one!**
|
||||
822
skills/issue-analysis/SKILL.md
Normal file
822
skills/issue-analysis/SKILL.md
Normal file
@@ -0,0 +1,822 @@
|
||||
---
|
||||
name: oss:issue-analysis
|
||||
description: Phase 2 of OSS contribution - Deep analysis combining issue requirements with codebase exploration. Extracts requirements, explores code structure, identifies exact code locations to fix, traces execution paths, and maps code-level changes needed. Use when starting work on selected issue.
|
||||
---
|
||||
|
||||
# Phase 2: Issue Analysis & Code Exploration
|
||||
|
||||
Deep analysis of issue requirements combined with codebase exploration to identify exact code changes needed.
|
||||
|
||||
## Purpose
|
||||
|
||||
Transform an issue into actionable code-level understanding by:
|
||||
- **Understanding requirements:** What needs to be done and why
|
||||
- **Exploring codebase:** Project structure, conventions, patterns
|
||||
- **Locating code:** Find exact files/functions to modify
|
||||
- **Identifying problems:** Pinpoint broken/missing code
|
||||
- **Planning changes:** Map requirements to specific code modifications
|
||||
|
||||
## When to Use
|
||||
|
||||
**Triggers:**
|
||||
- "이 이슈 분석해줘"
|
||||
- "코드에서 어디를 고쳐야 하나?"
|
||||
- "이슈와 코드 연결"
|
||||
- "문제 있는 코드 찾기"
|
||||
|
||||
**Use when:**
|
||||
- Starting work on a selected issue
|
||||
- Need to understand both requirements AND code
|
||||
- Want to identify exact modification points
|
||||
- Ready to plan implementation
|
||||
|
||||
## Integrated Analysis Framework
|
||||
|
||||
### Step 0: Review CONTRIBUTING.md Requirements
|
||||
|
||||
**MANDATORY: Before analyzing the issue, review project contribution guidelines**
|
||||
- Refer to CONTRIBUTING.md read in Phase 1
|
||||
- Ensure your analysis aligns with project conventions
|
||||
- Note any specific requirements for:
|
||||
- Code style and formatting
|
||||
- Testing requirements
|
||||
- Documentation standards
|
||||
- Commit message format
|
||||
- Branch naming conventions
|
||||
|
||||
**All analysis and subsequent work MUST comply with these guidelines**
|
||||
|
||||
### Step 1: Issue Type & Requirements Extraction
|
||||
|
||||
Identify issue type and extract core requirements.
|
||||
|
||||
**Issue Types:**
|
||||
|
||||
**Bug Fix:**
|
||||
```markdown
|
||||
### Bug Analysis Template
|
||||
|
||||
**Current Behavior:**
|
||||
[What actually happens - be specific]
|
||||
|
||||
**Expected Behavior:**
|
||||
[What should happen - reference docs/specs]
|
||||
|
||||
**Reproduction Steps:**
|
||||
1. [Step 1]
|
||||
2. [Step 2]
|
||||
3. [Observe: ...]
|
||||
|
||||
**Environment:**
|
||||
- Version: [version]
|
||||
- Platform: [OS/browser/etc]
|
||||
|
||||
**Impact:**
|
||||
- Severity: [Critical/High/Medium/Low]
|
||||
- Affected users: [who/how many]
|
||||
```
|
||||
|
||||
**Feature Request:**
|
||||
```markdown
|
||||
### Feature Analysis Template
|
||||
|
||||
**User Story:**
|
||||
As a [user type], I want [capability] so that [benefit].
|
||||
|
||||
**Functional Requirements:**
|
||||
1. [Requirement 1 - must have]
|
||||
2. [Requirement 2 - must have]
|
||||
3. [Requirement 3 - nice to have]
|
||||
|
||||
**Acceptance Criteria:**
|
||||
- [ ] [Criterion 1 - specific and testable]
|
||||
- [ ] [Criterion 2]
|
||||
- [ ] [Criterion 3]
|
||||
```
|
||||
|
||||
**Refactoring:**
|
||||
```markdown
|
||||
### Refactoring Analysis Template
|
||||
|
||||
**Current Problems:**
|
||||
1. [Problem 1: e.g., duplicated code across X files]
|
||||
2. [Problem 2: e.g., poor separation of concerns]
|
||||
|
||||
**Desired Outcome:**
|
||||
[What the code should look like after refactoring]
|
||||
|
||||
**Constraints:**
|
||||
- [ ] No behavior changes
|
||||
- [ ] All existing tests must pass
|
||||
- [ ] No API changes (if library)
|
||||
```
|
||||
|
||||
### Step 2: Project Structure & Convention Understanding
|
||||
|
||||
Explore codebase to understand organization and conventions.
|
||||
|
||||
**A. Directory Structure Mapping:**
|
||||
|
||||
```bash
|
||||
# Get project overview
|
||||
tree -L 2 -d
|
||||
|
||||
# Identify key directories
|
||||
ls -la
|
||||
```
|
||||
|
||||
```markdown
|
||||
### Project Structure
|
||||
|
||||
**Main source:** [path to primary code]
|
||||
|
||||
**Key directories:**
|
||||
- `src/` or `lib/` - Main source code
|
||||
- `tests/` or `__tests__/` - Test files
|
||||
- `docs/` - Documentation
|
||||
- `.github/` - CI/CD, workflows
|
||||
|
||||
**Organization principle:**
|
||||
- [x] By feature (e.g., /users, /products)
|
||||
- [ ] By layer (e.g., /models, /views, /controllers)
|
||||
- [ ] By type (e.g., /components, /utils, /services)
|
||||
|
||||
**Technologies:**
|
||||
- Language: [language + version]
|
||||
- Framework: [framework]
|
||||
- Build tool: [npm/cargo/maven/etc]
|
||||
- Testing: [jest/pytest/etc]
|
||||
```
|
||||
|
||||
**B. Code Conventions Discovery:**
|
||||
|
||||
```bash
|
||||
# Check for style configs
|
||||
cat .prettierrc .eslintrc package.json
|
||||
|
||||
# Find naming patterns
|
||||
find src -type f -name "*.js" | head -10
|
||||
```
|
||||
|
||||
```markdown
|
||||
### Code Conventions
|
||||
|
||||
**File Naming:**
|
||||
- Components: [e.g., PascalCase.tsx]
|
||||
- Utilities: [e.g., camelCase.ts]
|
||||
- Tests: [e.g., file.test.js or file_test.go]
|
||||
|
||||
**Code Style:**
|
||||
- Indentation: [2 spaces / 4 spaces / tabs]
|
||||
- Quotes: [single / double]
|
||||
- Import order: [external, internal, relative]
|
||||
|
||||
**Naming Patterns:**
|
||||
- Classes: [PascalCase]
|
||||
- Functions: [camelCase / snake_case]
|
||||
- Constants: [UPPER_SNAKE_CASE]
|
||||
```
|
||||
|
||||
**C. Testing Patterns:**
|
||||
|
||||
```bash
|
||||
# Find test locations
|
||||
find . -name "*test*" -type f | head -5
|
||||
|
||||
# Check test framework
|
||||
cat package.json | grep -A5 "scripts"
|
||||
```
|
||||
|
||||
```markdown
|
||||
### Testing Strategy
|
||||
|
||||
**Test Location:**
|
||||
- Unit tests: [e.g., __tests__/, alongside source]
|
||||
- Integration tests: [location]
|
||||
|
||||
**Test Framework:** [Jest / pytest / go test / etc]
|
||||
|
||||
**Test Naming Pattern:**
|
||||
- [describe/test, test_, Test*]
|
||||
|
||||
**Run Commands:**
|
||||
```bash
|
||||
npm test # All tests
|
||||
npm test -- file.test.js # Specific test
|
||||
npm test -- --coverage # With coverage
|
||||
```
|
||||
```
|
||||
|
||||
### Step 3: Code Location Discovery
|
||||
|
||||
Find exact files and functions related to the issue.
|
||||
|
||||
**A. Entry Point Discovery:**
|
||||
|
||||
```bash
|
||||
# For UI bugs/features - find component
|
||||
rg "ComponentName" --type tsx
|
||||
rg "class.*ComponentName"
|
||||
|
||||
# For API/backend - find endpoint
|
||||
rg "app\.(get|post|put).*endpoint"
|
||||
rg "@app.route|router"
|
||||
|
||||
# For CLI - find command
|
||||
rg "subcommand|command.*name"
|
||||
|
||||
# For library - find exported function
|
||||
rg "export (function|class|const).*functionName"
|
||||
```
|
||||
|
||||
```markdown
|
||||
### Entry Points
|
||||
|
||||
**Primary entry point:**
|
||||
- File: `src/components/UserProfile.tsx:45`
|
||||
- Function/Component: `UserProfile`
|
||||
- Purpose: [what this code does]
|
||||
|
||||
**How to trigger:**
|
||||
[Steps to reach this code - e.g., click button, call API]
|
||||
```
|
||||
|
||||
**B. Find Similar/Related Code:**
|
||||
|
||||
```bash
|
||||
# Find similar features for reference
|
||||
rg "similar-feature" --type js
|
||||
|
||||
# Find similar patterns
|
||||
find . -name "*Similar*.js"
|
||||
|
||||
# Find related tests
|
||||
rg "describe.*YourFeature" tests/
|
||||
```
|
||||
|
||||
```markdown
|
||||
### Reference Examples
|
||||
|
||||
**Similar code to reference:**
|
||||
1. `src/components/UserSettings.tsx:120`
|
||||
- Similar pattern for form handling
|
||||
- Shows validation approach
|
||||
|
||||
2. `src/utils/validation.ts:45`
|
||||
- Input validation logic
|
||||
- Error handling pattern
|
||||
```
|
||||
|
||||
### Step 4: Code-Level Problem Identification
|
||||
|
||||
**THIS IS THE CRITICAL STEP - Find exact code that needs fixing**
|
||||
|
||||
**A. For Bug Fixes - Trace to Root Cause:**
|
||||
|
||||
```bash
|
||||
# Search for error message or symptom
|
||||
rg "error message text"
|
||||
|
||||
# Find function definitions
|
||||
rg "function buggyFunction|def buggy_function"
|
||||
|
||||
# Find all callers
|
||||
rg "buggyFunction\("
|
||||
```
|
||||
|
||||
```markdown
|
||||
### Bug Root Cause Analysis
|
||||
|
||||
**Symptom Location:**
|
||||
- File: `src/auth/session.ts:78`
|
||||
- Function: `validateSession`
|
||||
- Problem: Crashes when `user` is null
|
||||
|
||||
**Code Analysis:**
|
||||
```typescript
|
||||
// CURRENT CODE (BROKEN)
|
||||
function validateSession(sessionId: string) {
|
||||
const session = getSession(sessionId)
|
||||
return session.user.id // ❌ CRASHES if user is null
|
||||
}
|
||||
```
|
||||
|
||||
**Root Cause:**
|
||||
- Missing null check for `session.user`
|
||||
- Happens during logout race condition
|
||||
|
||||
**Needs to be changed to:**
|
||||
```typescript
|
||||
// FIXED CODE
|
||||
function validateSession(sessionId: string) {
|
||||
const session = getSession(sessionId)
|
||||
if (!session?.user) { // ✅ Add null check
|
||||
return null
|
||||
}
|
||||
return session.user.id
|
||||
}
|
||||
```
|
||||
|
||||
**Additional locations to check:**
|
||||
- [ ] `src/auth/logout.ts:34` - may have similar issue
|
||||
- [ ] `src/auth/session.test.ts` - add test for null user
|
||||
```
|
||||
|
||||
**B. For Features - Identify Where to Add Code:**
|
||||
|
||||
```bash
|
||||
# Find where similar features are implemented
|
||||
rg "similar feature" --type js
|
||||
|
||||
# Find integration points
|
||||
rg "import.*Component|from.*module"
|
||||
```
|
||||
|
||||
```markdown
|
||||
### Feature Implementation Points
|
||||
|
||||
**Where to add code:**
|
||||
|
||||
**1. New function needed:**
|
||||
- Location: `src/utils/export.ts` (new file)
|
||||
- Function: `exportToCSV(data, options)`
|
||||
- Based on: `src/utils/exportToJSON.ts` (similar pattern)
|
||||
|
||||
**2. UI Integration:**
|
||||
- File: `src/components/DataTable.tsx:120`
|
||||
- Add: Export button in toolbar
|
||||
- Pattern: Follow existing "Download" button at line 115
|
||||
|
||||
**3. Hook into existing flow:**
|
||||
- File: `src/components/DataTable.tsx:45`
|
||||
- Add: Import new export function
|
||||
- Call: On button click handler
|
||||
|
||||
**Code to add:**
|
||||
```typescript
|
||||
// In DataTable.tsx
|
||||
import { exportToCSV } from '@/utils/export'
|
||||
|
||||
// Around line 120, add:
|
||||
<Button onClick={() => exportToCSV(data, { headers: true })}>
|
||||
Export CSV
|
||||
</Button>
|
||||
```
|
||||
```
|
||||
|
||||
**C. For Refactoring - Map Code Smells:**
|
||||
|
||||
```bash
|
||||
# Find duplicated code
|
||||
rg -A10 "duplicated pattern"
|
||||
|
||||
# Find long functions
|
||||
# (Manual inspection)
|
||||
```
|
||||
|
||||
```markdown
|
||||
### Refactoring Map
|
||||
|
||||
**Code smells identified:**
|
||||
|
||||
**1. Duplicated validation logic:**
|
||||
- `src/auth/login.ts:34-50` (17 lines)
|
||||
- `src/auth/register.ts:28-44` (17 lines)
|
||||
- `src/auth/resetPassword.ts:15-31` (17 lines)
|
||||
|
||||
**Solution:**
|
||||
- Extract to: `src/auth/validators.ts`
|
||||
- New function: `validateUserInput(input)`
|
||||
- Replace 3 duplications with function call
|
||||
|
||||
**2. God function:**
|
||||
- `src/api/userController.ts:100-350` (250 lines!)
|
||||
- Does: validation + business logic + DB + response
|
||||
|
||||
**Solution:**
|
||||
- Split into:
|
||||
- `validateUserData()` at line 100
|
||||
- `createUser()` at line 150
|
||||
- `sendWelcomeEmail()` at line 200
|
||||
```
|
||||
|
||||
### Step 5: Execution Path Tracing
|
||||
|
||||
Trace code flow to understand full context.
|
||||
|
||||
```markdown
|
||||
### Execution Flow
|
||||
|
||||
**For [bug/feature]: [description]**
|
||||
|
||||
```
|
||||
1. Entry: src/components/Login.tsx:89 - handleSubmit()
|
||||
└─> Calls: validateCredentials(username, password)
|
||||
|
||||
2. Flow: src/auth/validation.ts:23 - validateCredentials()
|
||||
└─> Calls: checkPassword(password)
|
||||
└─> Returns: {valid: boolean, error?: string}
|
||||
|
||||
3. Bug: src/auth/validation.ts:45 - checkPassword()
|
||||
└─> ❌ PROBLEM: Doesn't handle null password
|
||||
└─> FIX: Add null check at line 46
|
||||
|
||||
4. Result: Returns to handleSubmit, shows error
|
||||
```
|
||||
|
||||
**Key functions in flow:**
|
||||
- `handleSubmit()` @ Login.tsx:89 - Form submission
|
||||
- `validateCredentials()` @ validation.ts:23 - Main validator
|
||||
- `checkPassword()` @ validation.ts:45 - ❌ NEEDS FIX HERE
|
||||
|
||||
**Data transformations:**
|
||||
- Input: `{username: string, password: string}`
|
||||
- Validation: checks format, length, special chars
|
||||
- Output: `{valid: boolean, error?: string}`
|
||||
```
|
||||
|
||||
### Step 6: Dependency & Impact Analysis
|
||||
|
||||
Understand what depends on your changes.
|
||||
|
||||
```bash
|
||||
# Find all callers
|
||||
rg "functionName\("
|
||||
|
||||
# Find all imports
|
||||
rg "import.*ModuleName"
|
||||
|
||||
# Find test coverage
|
||||
rg "describe.*functionName" tests/
|
||||
```
|
||||
|
||||
```markdown
|
||||
### Dependencies & Impact
|
||||
|
||||
**Upstream (what calls this code):**
|
||||
- `src/components/Login.tsx:89` - Login form
|
||||
- `src/components/Register.tsx:67` - Registration form
|
||||
- `src/api/authController.ts:120` - API endpoint
|
||||
|
||||
**Impact of change:**
|
||||
- 🟢 Low risk - adding validation only
|
||||
- All callers already handle error case
|
||||
- Existing tests cover error path
|
||||
|
||||
**Downstream (what this calls):**
|
||||
- `src/utils/crypto.ts:hashPassword()` - crypto library
|
||||
- `src/models/User.ts:findByUsername()` - DB query
|
||||
|
||||
**Side effects:**
|
||||
- None - pure validation function
|
||||
- No state mutations
|
||||
- No external calls
|
||||
|
||||
**Test coverage:**
|
||||
- Current: `tests/auth/validation.test.ts`
|
||||
- Need to add: test case for null password
|
||||
```
|
||||
|
||||
### Step 7: Modification Plan
|
||||
|
||||
Create concrete plan of what code to change.
|
||||
|
||||
```markdown
|
||||
### Modification Plan
|
||||
|
||||
**Files to modify:**
|
||||
|
||||
**1. PRIMARY FIX: src/auth/validation.ts**
|
||||
- **Line 45-50:** Function `checkPassword()`
|
||||
- **Change type:** Add null/undefined check
|
||||
- **Before:**
|
||||
```typescript
|
||||
function checkPassword(password: string): boolean {
|
||||
return password.length >= 8 // ❌ Crashes if null
|
||||
}
|
||||
```
|
||||
- **After:**
|
||||
```typescript
|
||||
function checkPassword(password: string | null): boolean {
|
||||
if (!password) { // ✅ Add null check
|
||||
return false
|
||||
}
|
||||
return password.length >= 8
|
||||
}
|
||||
```
|
||||
- **Risk:** 🟢 Low - defensive programming
|
||||
|
||||
**2. ADD TEST: tests/auth/validation.test.ts**
|
||||
- **Line:** After line 67 (add new test case)
|
||||
- **Add:**
|
||||
```typescript
|
||||
it('should return false for null password', () => {
|
||||
expect(checkPassword(null)).toBe(false)
|
||||
})
|
||||
```
|
||||
|
||||
**3. UPDATE TYPES: src/types/auth.ts** (if needed)
|
||||
- **Line 12:** Update PasswordInput type to allow null
|
||||
|
||||
**Files NOT to change:**
|
||||
- ❌ `src/components/Login.tsx` - already handles errors correctly
|
||||
- ❌ `src/api/authController.ts` - no changes needed
|
||||
```
|
||||
|
||||
### Step 8: Edge Cases & Implicit Requirements
|
||||
|
||||
Identify edge cases from code analysis.
|
||||
|
||||
```markdown
|
||||
### Edge Cases Discovered
|
||||
|
||||
**From code inspection:**
|
||||
- [ ] Null password: `checkPassword(null)` - FOUND in code trace
|
||||
- [ ] Empty string: `checkPassword("")` - check if handled
|
||||
- [ ] Very long password: `checkPassword("x".repeat(10000))` - DOS risk?
|
||||
- [ ] Unicode characters: `checkPassword("パスワード")` - supported?
|
||||
- [ ] SQL injection: `checkPassword("' OR 1=1--")` - sanitized?
|
||||
|
||||
**Integration concerns:**
|
||||
- Depends on: crypto library (trusted)
|
||||
- Affects: Login, Register, Reset Password flows
|
||||
- Breaking change: No - only adding validation
|
||||
|
||||
**Performance:**
|
||||
- No loops or heavy computation
|
||||
- O(1) complexity
|
||||
- No performance concerns
|
||||
|
||||
**Security:**
|
||||
- Input validation ✅
|
||||
- No sensitive data logged
|
||||
- Follow OWASP guidelines
|
||||
```
|
||||
|
||||
### Step 9: Acceptance Criteria & Scope
|
||||
|
||||
Define concrete success criteria and scope.
|
||||
|
||||
```markdown
|
||||
### Scope Definition
|
||||
|
||||
**In Scope:**
|
||||
1. Add null check to `checkPassword()` function
|
||||
2. Add unit test for null password case
|
||||
3. Verify no crashes on null input
|
||||
|
||||
**Out of Scope:**
|
||||
1. Password strength improvements (separate issue #456)
|
||||
2. UI error message improvements (separate issue #457)
|
||||
3. Refactoring validation.ts (not requested)
|
||||
|
||||
**Complexity:** 🟢 Simple (< 2 hours)
|
||||
- Single function change
|
||||
- One test case addition
|
||||
- Low risk change
|
||||
|
||||
### Acceptance Criteria
|
||||
|
||||
**Functional:**
|
||||
- [ ] `checkPassword(null)` returns `false` (no crash)
|
||||
- [ ] `checkPassword(undefined)` returns `false` (no crash)
|
||||
- [ ] `checkPassword("valid")` still works correctly
|
||||
- [ ] Existing tests still pass
|
||||
|
||||
**Quality:**
|
||||
- [ ] New test added and passing
|
||||
- [ ] No linting errors
|
||||
- [ ] Type definitions updated
|
||||
- [ ] Code follows project conventions
|
||||
|
||||
**Verification:**
|
||||
```bash
|
||||
npm test # All tests pass
|
||||
npm run lint # No errors
|
||||
npm run type-check # No type errors
|
||||
```
|
||||
|
||||
**Manual testing:**
|
||||
1. Open login form
|
||||
2. Submit with empty password
|
||||
3. Verify: No crash, shows error message
|
||||
```
|
||||
|
||||
## Analysis Patterns by Issue Type
|
||||
|
||||
### Bug Fix Pattern
|
||||
|
||||
1. **Understand symptom** - Read issue description
|
||||
2. **Reproduce locally** - Follow reproduction steps
|
||||
3. **Explore codebase** - Find project structure
|
||||
4. **Locate bug** - Trace code to root cause
|
||||
5. **Identify fix point** - Exact line to change
|
||||
6. **Plan changes** - What code to modify
|
||||
7. **Consider side effects** - Impact analysis
|
||||
|
||||
### Feature Pattern
|
||||
|
||||
1. **Understand need** - User story, use cases
|
||||
2. **Explore codebase** - Structure and conventions
|
||||
3. **Find similar features** - Reference implementations
|
||||
4. **Identify integration points** - Where to add code
|
||||
5. **Plan new code** - What to create
|
||||
6. **Map dependencies** - What it will interact with
|
||||
7. **Design interface** - API, props, parameters
|
||||
|
||||
### Refactoring Pattern
|
||||
|
||||
1. **Identify code smell** - What's wrong
|
||||
2. **Explore codebase** - Understand context
|
||||
3. **Map all instances** - Find all occurrences
|
||||
4. **Plan extraction** - New structure
|
||||
5. **Ensure test coverage** - Safety net
|
||||
6. **Plan incremental steps** - Small safe changes
|
||||
7. **Verify no behavior change** - Tests still pass
|
||||
|
||||
## Output Format
|
||||
|
||||
Provide comprehensive analysis with code-level details:
|
||||
|
||||
```markdown
|
||||
# 🔍 Issue Analysis: [Issue Title]
|
||||
|
||||
**Issue:** #[number] | **Type:** [bug/feature/refactor]
|
||||
**Status:** Ready to implement
|
||||
|
||||
---
|
||||
|
||||
## Summary
|
||||
[2-3 sentences: what needs to be done, what code needs to change, why]
|
||||
|
||||
---
|
||||
|
||||
## Requirements
|
||||
|
||||
### Issue Requirements
|
||||
[What user/maintainer wants]
|
||||
|
||||
### Code Requirements
|
||||
[What code needs to change based on exploration]
|
||||
|
||||
---
|
||||
|
||||
## Project Understanding
|
||||
|
||||
### Structure
|
||||
[Key directories and organization]
|
||||
|
||||
### Conventions
|
||||
[Naming, style, patterns to follow]
|
||||
|
||||
### Testing
|
||||
[Framework, location, how to run]
|
||||
|
||||
---
|
||||
|
||||
## Code Analysis
|
||||
|
||||
### Entry Points
|
||||
- [File:line - function/component name]
|
||||
|
||||
### Problem Code Identified
|
||||
|
||||
**File: [path]**
|
||||
**Line: [number]**
|
||||
**Function: [name]**
|
||||
|
||||
**Current code (BROKEN):**
|
||||
```language
|
||||
[actual problematic code]
|
||||
```
|
||||
|
||||
**Problem:**
|
||||
- [what's wrong]
|
||||
- [why it breaks]
|
||||
|
||||
**Fix needed:**
|
||||
```language
|
||||
[corrected code]
|
||||
```
|
||||
|
||||
### Execution Flow
|
||||
```
|
||||
[trace from entry to problem]
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Modification Plan
|
||||
|
||||
### Files to Change
|
||||
|
||||
**1. [file path]**
|
||||
- **Line:** [number]
|
||||
- **Function:** [name]
|
||||
- **Change:** [specific modification]
|
||||
- **Before/After:** [code snippets]
|
||||
|
||||
**2. [test file]**
|
||||
- **Add:** [new test cases]
|
||||
|
||||
### Impact Analysis
|
||||
- **Upstream callers:** [list]
|
||||
- **Risk:** 🟢/🟡/🔴
|
||||
- **Breaking change:** Yes/No
|
||||
|
||||
---
|
||||
|
||||
## Testing Plan
|
||||
|
||||
**Existing tests to verify:**
|
||||
- [test file:description]
|
||||
|
||||
**New tests to add:**
|
||||
- [ ] Test case 1: [description]
|
||||
- [ ] Test case 2: [description]
|
||||
|
||||
**Test commands:**
|
||||
```bash
|
||||
npm test
|
||||
npm test -- specific.test.js
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Edge Cases
|
||||
- [ ] [Edge case 1 - how to handle]
|
||||
- [ ] [Edge case 2 - how to handle]
|
||||
|
||||
---
|
||||
|
||||
## Acceptance Criteria
|
||||
- [ ] [Specific, testable criterion 1]
|
||||
- [ ] [Specific, testable criterion 2]
|
||||
- [ ] All tests pass
|
||||
- [ ] Follows CONTRIBUTING.md guidelines
|
||||
|
||||
---
|
||||
|
||||
## Implementation Checklist
|
||||
- [ ] Fix code at [file:line]
|
||||
- [ ] Add test at [test-file]
|
||||
- [ ] Run tests - all pass
|
||||
- [ ] Run linter - no errors
|
||||
- [ ] Manual verification
|
||||
|
||||
---
|
||||
|
||||
## Next Steps
|
||||
|
||||
✅ Analysis complete - Ready for **Phase 3: Solution Implementation**
|
||||
|
||||
**Start with:**
|
||||
1. [Specific first task - e.g., "Fix checkPassword() at validation.ts:45"]
|
||||
2. [Second task]
|
||||
3. [Third task]
|
||||
```
|
||||
|
||||
## Integration with Main Framework
|
||||
|
||||
When invoked from main framework:
|
||||
|
||||
1. **Receive context:** Issue URL, type from Phase 1
|
||||
2. **Execute integrated analysis:**
|
||||
- Extract requirements
|
||||
- Explore codebase structure
|
||||
- Locate exact code to change
|
||||
- Identify problems in code
|
||||
- Plan modifications
|
||||
3. **Return comprehensive analysis:** Ready to implement
|
||||
4. **Update tracker:** Mark Phase 2 complete
|
||||
5. **Transition:** Phase 3 (Implementation) with concrete code-level plan
|
||||
|
||||
## Common Pitfalls
|
||||
|
||||
**Avoid:**
|
||||
|
||||
❌ **Analyzing requirements without looking at code** - Will miss context
|
||||
❌ **Exploring code without understanding requirements** - Will get lost
|
||||
❌ **Assuming code structure** - Always verify by reading
|
||||
❌ **Stopping at "what" without finding "where"** - Need exact locations
|
||||
❌ **Ignoring existing patterns** - Must follow project conventions
|
||||
❌ **Not tracing full execution path** - Will miss side effects
|
||||
❌ **Forgetting to check tests** - Test changes are part of solution
|
||||
|
||||
## Verification Before Implementation
|
||||
|
||||
**Before moving to Phase 3, verify:**
|
||||
|
||||
- [ ] Requirements fully understood
|
||||
- [ ] Codebase structure mapped
|
||||
- [ ] Exact modification points identified
|
||||
- [ ] Problem code located (for bugs)
|
||||
- [ ] Integration points clear (for features)
|
||||
- [ ] All duplications found (for refactoring)
|
||||
- [ ] Execution path traced
|
||||
- [ ] Dependencies identified
|
||||
- [ ] Test strategy planned
|
||||
- [ ] Edge cases listed
|
||||
- [ ] Impact assessed and acceptable
|
||||
|
||||
If any unclear, dig deeper or ask maintainer.
|
||||
340
skills/issue-discovery/SKILL.md
Normal file
340
skills/issue-discovery/SKILL.md
Normal file
@@ -0,0 +1,340 @@
|
||||
---
|
||||
name: oss:issue-discovery
|
||||
description: Phase 1 of OSS contribution - Find and evaluate suitable issues matching your skills, time, and learning goals. Filters by labels, assesses project health, and provides structured recommendations. Use when starting OSS contribution, looking for beginner-friendly issues, or evaluating multiple issue options.
|
||||
---
|
||||
|
||||
# Phase 1: Issue Discovery & Triage
|
||||
|
||||
Find and evaluate suitable issues to work on in open source projects.
|
||||
|
||||
## Purpose
|
||||
|
||||
Help contributors identify issues that match their:
|
||||
- Skill level and experience
|
||||
- Available time commitment
|
||||
- Learning goals
|
||||
- Interest areas
|
||||
|
||||
## When to Use
|
||||
|
||||
**Triggers:**
|
||||
- "좋은 이슈 찾아줘"
|
||||
- "beginner-friendly 이슈 추천"
|
||||
- "이 프로젝트에서 뭘 할 수 있을까?"
|
||||
- "이 이슈가 나한테 맞을까?"
|
||||
|
||||
**Use when:**
|
||||
- Starting contribution to a new project
|
||||
- Looking for next issue after completing one
|
||||
- Evaluating multiple issue options
|
||||
- Unsure which issue to tackle
|
||||
|
||||
## Discovery Process
|
||||
|
||||
### Step 1: Understand Contributor Profile
|
||||
|
||||
Ask or infer:
|
||||
- **Experience level:** First-time, intermediate, experienced
|
||||
- **Tech stack familiarity:** Languages, frameworks, tools
|
||||
- **Time availability:** Quick fix, moderate, substantial project
|
||||
- **Goals:** Learn, build portfolio, fix personal pain point, give back
|
||||
- **Preferences:** Bug fix, feature, docs, tests, refactoring
|
||||
|
||||
### Step 2: Project Assessment
|
||||
|
||||
Before searching issues, evaluate project health and read contribution guidelines:
|
||||
|
||||
**MANDATORY: Read CONTRIBUTING.md**
|
||||
- **MUST read and understand** the repository's CONTRIBUTING.md file
|
||||
- Note required workflow, branch naming, commit conventions
|
||||
- Identify testing requirements and code style guidelines
|
||||
- Check for CLA (Contributor License Agreement) requirements
|
||||
- Understand PR submission process and review expectations
|
||||
- **All subsequent phases MUST follow these guidelines**
|
||||
|
||||
**Health indicators:**
|
||||
- Recent commit activity (last 7-30 days)
|
||||
- Responsive maintainers (issue/PR response time)
|
||||
- Clear contribution guidelines (CONTRIBUTING.md present)
|
||||
- Active community (discussions, recent merges)
|
||||
- Good documentation
|
||||
|
||||
**Red flags:**
|
||||
- No activity for 6+ months
|
||||
- Many ignored PRs or issues
|
||||
- Hostile or dismissive maintainer responses
|
||||
- No CONTRIBUTING.md or unclear guidelines
|
||||
- Constant breaking changes
|
||||
|
||||
Output format:
|
||||
```markdown
|
||||
### Project Health Check
|
||||
- **Activity:** [recent commits/releases]
|
||||
- **Responsiveness:** [avg maintainer response time]
|
||||
- **Community:** [# contributors, discussion activity]
|
||||
- **CONTRIBUTING.md:** ✅ Read and understood / ⚠️ Unclear / ❌ Missing
|
||||
- Key requirements: [workflow, testing, style, etc.]
|
||||
- **Assessment:** ✅ Good to contribute / ⚠️ Proceed with caution / ❌ Not recommended
|
||||
```
|
||||
|
||||
### Step 3: Issue Filtering
|
||||
|
||||
Use multiple filters to find candidates:
|
||||
|
||||
**Critical filters (MUST apply):**
|
||||
- **No linked PR:** Exclude issues that already have associated pull requests
|
||||
- Check issue references, linked PRs in GitHub UI
|
||||
- Skip issues marked "has-pr" or with PR links in comments
|
||||
- **Beginner-friendly priority:** Focus on accessible issues
|
||||
- Labels: `good first issue`, `beginner-friendly`, `help wanted`
|
||||
- Labels: `up-for-grabs`, `easy`, `low-hanging-fruit`
|
||||
- **High priority labels:** Prioritize important work
|
||||
- Look for: `priority: high`, `high-priority`, `important`, `urgent`
|
||||
- Repository-specific priority indicators
|
||||
- Issues referenced in roadmap or milestones
|
||||
|
||||
**By issue type:**
|
||||
- `documentation`, `bug`, `enhancement`
|
||||
- Prefer well-scoped, clearly defined issues
|
||||
|
||||
**By complexity:**
|
||||
- **Simple (1-4 hours):** Typos, docs, simple bugs, config changes
|
||||
- **Moderate (1-2 days):** Feature additions, refactoring, moderate bugs
|
||||
- **Complex (1+ weeks):** Architecture changes, major features, complex bugs
|
||||
|
||||
**By recency:**
|
||||
- Prefer issues updated within last 30 days
|
||||
- Check for assigned developers
|
||||
- Look for maintainer engagement
|
||||
|
||||
### Step 4: Individual Issue Evaluation
|
||||
|
||||
For each candidate issue, assess:
|
||||
|
||||
#### Quality Indicators ✅
|
||||
|
||||
**Clear description:**
|
||||
- Problem statement is specific
|
||||
- Expected behavior defined
|
||||
- Actual behavior described
|
||||
- Steps to reproduce (for bugs)
|
||||
|
||||
**Good context:**
|
||||
- Relevant error messages/logs
|
||||
- Environment details (version, OS, browser)
|
||||
- Screenshots or examples
|
||||
- Links to related issues/discussions
|
||||
|
||||
**Maintainer engagement:**
|
||||
- Maintainer has commented
|
||||
- Issue is confirmed/triaged
|
||||
- No one currently assigned
|
||||
- Not marked as "wontfix" or "blocked"
|
||||
|
||||
#### Warning Signs ⚠️
|
||||
|
||||
- **Has linked PR** - Issue already being worked on
|
||||
- Vague or unclear requirements
|
||||
- No maintainer response
|
||||
- Already assigned to someone
|
||||
- Marked as "blocked", "on-hold", or "needs-discussion"
|
||||
- Very old issue (6+ months) with no activity
|
||||
- Duplicate of another issue
|
||||
- Controversial or disputed approach
|
||||
|
||||
#### Evaluation Template
|
||||
|
||||
For each candidate issue:
|
||||
|
||||
```markdown
|
||||
## Issue: [Title] (#[number])
|
||||
**URL:** [link]
|
||||
**Labels:** [labels]
|
||||
**Created:** [date] | **Updated:** [date]
|
||||
|
||||
### Quick Assessment
|
||||
- **Clarity:** ⭐⭐⭐⭐☆ (4/5) - [brief note]
|
||||
- **Scope:** 🔵 Small | 🟡 Medium | 🔴 Large
|
||||
- **Difficulty:** 🟢 Easy | 🟡 Moderate | 🔴 Hard
|
||||
- **Time estimate:** [hours/days]
|
||||
|
||||
### Requirements Understanding
|
||||
- **What needs to be done:** [1-2 sentences]
|
||||
- **Success criteria:** [how to know it's complete]
|
||||
- **Unknowns:** [what's unclear or needs investigation]
|
||||
|
||||
### Skill Match
|
||||
- **Required skills:** [list]
|
||||
- **Your match:** ✅ Good fit / ⚠️ Stretch goal / ❌ Too advanced
|
||||
- **Learning opportunity:** [what you'll learn]
|
||||
|
||||
### Decision
|
||||
✅ **Good choice because:** [reasons]
|
||||
⚠️ **Consider if:** [conditions]
|
||||
❌ **Skip because:** [reasons]
|
||||
|
||||
**Recommendation:** [Proceed / Ask maintainer first / Choose another]
|
||||
```
|
||||
|
||||
### Step 5: Multi-Issue Comparison
|
||||
|
||||
When evaluating multiple issues, create comparison table:
|
||||
|
||||
```markdown
|
||||
## Issue Comparison
|
||||
|
||||
| Issue | Difficulty | Time | Learning Value | Impact | Priority |
|
||||
|-------|-----------|------|----------------|--------|----------|
|
||||
| #123 | 🟢 Easy | 2h | ⭐⭐☆ | Medium | 🥇 High |
|
||||
| #456 | 🟡 Medium | 1d | ⭐⭐⭐ | High | 🥈 Med |
|
||||
| #789 | 🔴 Hard | 1w | ⭐⭐⭐⭐ | High | 🥉 Low |
|
||||
|
||||
### Recommendation
|
||||
Start with **#123** because:
|
||||
1. Quick win to familiarize with codebase
|
||||
2. Clear requirements, low risk
|
||||
3. Sets foundation for #456 later
|
||||
|
||||
**Progression path:** #123 → #456 → #789
|
||||
```
|
||||
|
||||
## Strategic Considerations
|
||||
|
||||
### First Contribution Strategy
|
||||
|
||||
For first-time contributors to a project:
|
||||
|
||||
1. **Start small:** Choose simple issue to learn workflow
|
||||
2. **Build trust:** Demonstrate quality before tackling complex work
|
||||
3. **Learn codebase:** Use first PR to understand conventions
|
||||
4. **Engage community:** Interact respectfully with maintainers
|
||||
|
||||
**Recommended progression:**
|
||||
```
|
||||
First PR: Documentation fix or typo
|
||||
↓
|
||||
Second PR: Simple bug fix or small feature
|
||||
↓
|
||||
Third PR: Moderate complexity work
|
||||
↓
|
||||
Ongoing: Complex features, architecture improvements
|
||||
```
|
||||
|
||||
### Learning-Oriented Selection
|
||||
|
||||
When goal is learning:
|
||||
|
||||
- **Choose stretch issues:** Slightly above comfort level
|
||||
- **Look for patterns:** Issues that teach transferable skills
|
||||
- **Seek feedback:** Projects with detailed code reviews
|
||||
- **Diverse types:** Mix bugs, features, refactoring, docs
|
||||
|
||||
### Impact-Oriented Selection
|
||||
|
||||
When goal is maximizing value:
|
||||
|
||||
- **User-facing issues:** Direct user benefit
|
||||
- **Bug fixes:** Immediate problem resolution
|
||||
- **Documentation:** Helps many future contributors
|
||||
- **Performance:** Benefits all users
|
||||
|
||||
### Portfolio Building
|
||||
|
||||
For building public portfolio:
|
||||
|
||||
- **Substantial features:** Show design skills
|
||||
- **Complex bugs:** Show debugging ability
|
||||
- **Cross-cutting work:** Show system understanding
|
||||
- **Leadership:** Help triage, review others' PRs
|
||||
|
||||
## Engagement Before Starting
|
||||
|
||||
Before beginning work, **always:**
|
||||
|
||||
1. **Comment on issue:**
|
||||
```
|
||||
"Hi! I'd like to work on this issue.
|
||||
|
||||
My understanding is: [brief summary]
|
||||
|
||||
I'm planning to: [approach]
|
||||
|
||||
Does this sound good? Any guidance appreciated!"
|
||||
```
|
||||
|
||||
2. **Wait for confirmation:**
|
||||
- Maintainer gives go-ahead
|
||||
- No one else is assigned
|
||||
- Approach is approved
|
||||
|
||||
3. **Ask questions:**
|
||||
- Clarify unclear requirements
|
||||
- Confirm edge cases
|
||||
- Request guidance on approach
|
||||
|
||||
**Why this matters:**
|
||||
- Avoids duplicate work
|
||||
- Ensures approach is correct
|
||||
- Builds relationship with maintainers
|
||||
- Shows respect for project process
|
||||
|
||||
## Common Pitfalls
|
||||
|
||||
**Avoid:**
|
||||
|
||||
❌ **Starting without commenting** - Someone else might be working on it
|
||||
❌ **Choosing glamorous but too-hard issues** - Will frustrate you and waste time
|
||||
❌ **Ignoring "needs discussion" label** - Issue might not be ready
|
||||
❌ **Taking assigned issues** - Respect others' claimed work
|
||||
❌ **Multiple issues at once** - Finish one before starting next
|
||||
❌ **Stale issues** - May be outdated or deprioritized
|
||||
|
||||
## Output Format
|
||||
|
||||
Provide structured recommendation:
|
||||
|
||||
```markdown
|
||||
# 🎯 Issue Discovery Results
|
||||
|
||||
## Selected Issue
|
||||
**Title:** [Issue title]
|
||||
**URL:** [link]
|
||||
**Status:** [open/triaged/confirmed]
|
||||
|
||||
### Why This Issue?
|
||||
1. [Reason 1: skill match, learning, impact, etc.]
|
||||
2. [Reason 2]
|
||||
3. [Reason 3]
|
||||
|
||||
### What You'll Do
|
||||
[1-2 sentence summary of the work]
|
||||
|
||||
### Prerequisites
|
||||
- [ ] Comment on issue to claim
|
||||
- [ ] Wait for maintainer approval
|
||||
- [ ] Fork repository
|
||||
- [ ] Set up development environment
|
||||
|
||||
### Next Steps
|
||||
Ready to move to **Phase 2: Issue Analysis**?
|
||||
|
||||
---
|
||||
|
||||
## Alternative Options
|
||||
|
||||
If this doesn't work out, consider:
|
||||
1. **[Issue #]** - [brief description, why alternative]
|
||||
2. **[Issue #]** - [brief description, why alternative]
|
||||
```
|
||||
|
||||
## Integration with Main Framework
|
||||
|
||||
When invoked from main framework:
|
||||
|
||||
1. **Receive context:** User profile, project URL, preferences
|
||||
2. **Execute discovery:** Filter and evaluate issues
|
||||
3. **Return recommendation:** Selected issue + reasoning
|
||||
4. **Update tracker:** Mark Phase 1 complete
|
||||
5. **Transition:** Prepare context for Phase 2
|
||||
|
||||
Can be re-invoked at any time if selected issue becomes unavailable or user wants different option.
|
||||
783
skills/solution-implementation/SKILL.md
Normal file
783
skills/solution-implementation/SKILL.md
Normal file
@@ -0,0 +1,783 @@
|
||||
---
|
||||
name: oss:solution-implementation
|
||||
description: Phase 3 of OSS contribution - Design and implement the solution following project standards and best practices. Writes code following conventions, adds/updates tests, handles edge cases. Use when issue analysis is complete and ready to write code.
|
||||
---
|
||||
|
||||
# Phase 3: Solution Implementation
|
||||
|
||||
Design and implement the solution following project standards and best practices.
|
||||
|
||||
## Purpose
|
||||
|
||||
Transform the implementation plan into working code that:
|
||||
- Solves the issue completely
|
||||
- Follows project conventions
|
||||
- Passes all tests
|
||||
- Handles edge cases
|
||||
- Is maintainable and reviewable
|
||||
|
||||
## When to Use
|
||||
|
||||
**Triggers:**
|
||||
- "솔루션 구현"
|
||||
- "이슈 해결 시작"
|
||||
- "코드 작성"
|
||||
- "테스트 작성"
|
||||
|
||||
**Use when:**
|
||||
- Issue analysis (Phase 2) is complete
|
||||
- Code locations identified
|
||||
- Ready to write code
|
||||
- Need implementation guidance
|
||||
- Want to follow best practices
|
||||
|
||||
## Implementation Framework
|
||||
|
||||
### Step 0: CONTRIBUTING.md Compliance Check
|
||||
|
||||
**MANDATORY: Final verification before implementation**
|
||||
- Review all CONTRIBUTING.md requirements from Phase 1
|
||||
- Ensure implementation will follow:
|
||||
- Code style and formatting standards
|
||||
- Commit message format
|
||||
- Branch naming conventions
|
||||
- Testing requirements
|
||||
- Documentation standards
|
||||
- **All code written MUST strictly follow these guidelines**
|
||||
|
||||
### Step 1: Implementation Planning
|
||||
|
||||
Plan your approach before writing code.
|
||||
|
||||
**Design decisions:**
|
||||
|
||||
```markdown
|
||||
### Implementation Plan
|
||||
|
||||
**Approach:**
|
||||
[High-level description of solution approach]
|
||||
|
||||
**Why this approach:**
|
||||
1. [Reason 1: e.g., matches existing patterns]
|
||||
2. [Reason 2: e.g., minimal invasive change]
|
||||
3. [Reason 3: e.g., easily testable]
|
||||
|
||||
**Alternative approaches considered:**
|
||||
- **[Approach A]:** [why not chosen]
|
||||
- **[Approach B]:** [why not chosen]
|
||||
|
||||
**Implementation order:**
|
||||
1. [Task 1] - [rationale for doing first]
|
||||
2. [Task 2] - [builds on previous]
|
||||
3. [Task 3] - [completes the feature]
|
||||
|
||||
**Checkpoints:**
|
||||
- After step 1: [what to verify]
|
||||
- After step 2: [what to verify]
|
||||
- After step 3: [final verification]
|
||||
```
|
||||
|
||||
### Step 2: Test-Driven Development (TDD)
|
||||
|
||||
Start with tests when possible.
|
||||
|
||||
**TDD workflow:**
|
||||
|
||||
1. **Write failing test:**
|
||||
- Captures the requirement
|
||||
- Specific and focused
|
||||
- Fails for right reason
|
||||
|
||||
2. **Implement minimal solution:**
|
||||
- Make test pass
|
||||
- No premature optimization
|
||||
- Follow existing patterns
|
||||
|
||||
3. **Refactor:**
|
||||
- Clean up code
|
||||
- Extract duplications
|
||||
- Improve readability
|
||||
- Tests still pass
|
||||
|
||||
**When TDD makes sense:**
|
||||
- Bug fixes (test reproduces bug)
|
||||
- New utility functions
|
||||
- Clear acceptance criteria
|
||||
- Well-understood requirements
|
||||
|
||||
**When to code first:**
|
||||
- Exploratory work
|
||||
- UI/UX iteration
|
||||
- Complex interactions
|
||||
- Unclear requirements
|
||||
|
||||
**Test-first template:**
|
||||
|
||||
```markdown
|
||||
### TDD Cycle
|
||||
|
||||
**Red (Failing Test):**
|
||||
```javascript
|
||||
// test/feature.test.js
|
||||
describe('NewFeature', () => {
|
||||
it('should do X when Y', () => {
|
||||
const result = newFeature(input)
|
||||
expect(result).toBe(expected) // ❌ FAILS
|
||||
})
|
||||
})
|
||||
```
|
||||
|
||||
**Green (Minimal Implementation):**
|
||||
```javascript
|
||||
// src/feature.js
|
||||
function newFeature(input) {
|
||||
// Simplest thing that makes test pass
|
||||
return expected
|
||||
}
|
||||
```
|
||||
|
||||
**Refactor (Clean Up):**
|
||||
```javascript
|
||||
// src/feature.js
|
||||
function newFeature(input) {
|
||||
// Proper implementation
|
||||
// Handles edge cases
|
||||
// Clean and readable
|
||||
return properResult
|
||||
}
|
||||
```
|
||||
|
||||
**Verify:** All tests pass ✅
|
||||
```
|
||||
|
||||
### Step 3: Code Implementation
|
||||
|
||||
Write production code following project conventions.
|
||||
|
||||
**Quality checklist:**
|
||||
|
||||
```markdown
|
||||
### Code Quality
|
||||
|
||||
**Correctness:**
|
||||
- [ ] Solves the stated problem
|
||||
- [ ] Handles all requirements
|
||||
- [ ] Covers edge cases
|
||||
- [ ] No logical errors
|
||||
|
||||
**Convention adherence:**
|
||||
- [ ] Naming matches project style
|
||||
- [ ] Formatting follows guidelines
|
||||
- [ ] Imports organized correctly
|
||||
- [ ] Comments where helpful
|
||||
|
||||
**Code quality:**
|
||||
- [ ] DRY (Don't Repeat Yourself)
|
||||
- [ ] Single Responsibility Principle
|
||||
- [ ] Appropriate abstractions
|
||||
- [ ] Clear variable/function names
|
||||
|
||||
**Error handling:**
|
||||
- [ ] Validates inputs
|
||||
- [ ] Handles errors gracefully
|
||||
- [ ] Provides clear error messages
|
||||
- [ ] Fails safely
|
||||
|
||||
**Performance:**
|
||||
- [ ] No obvious inefficiencies
|
||||
- [ ] Appropriate data structures
|
||||
- [ ] Avoids unnecessary work
|
||||
- [ ] Acceptable time/space complexity
|
||||
|
||||
**Security:**
|
||||
- [ ] Input validation
|
||||
- [ ] No injection vulnerabilities
|
||||
- [ ] Proper authorization checks
|
||||
- [ ] Sensitive data protection
|
||||
```
|
||||
|
||||
**Implementation patterns:**
|
||||
|
||||
**For Bug Fixes:**
|
||||
```markdown
|
||||
### Bug Fix Implementation
|
||||
|
||||
**1. Add test that reproduces bug:**
|
||||
```javascript
|
||||
it('should handle edge case correctly', () => {
|
||||
// This test currently fails
|
||||
const result = buggyFunction(edgeCase)
|
||||
expect(result).toBe(correctValue)
|
||||
})
|
||||
```
|
||||
|
||||
**2. Locate root cause:**
|
||||
- Function: `[name]` @ [file:line]
|
||||
- Problem: [what's wrong]
|
||||
- Why it happens: [explanation]
|
||||
|
||||
**3. Implement fix:**
|
||||
```javascript
|
||||
// Before:
|
||||
function buggyFunction(input) {
|
||||
return input.value // Fails when input.value is undefined
|
||||
}
|
||||
|
||||
// After:
|
||||
function buggyFunction(input) {
|
||||
return input?.value ?? defaultValue // Handles undefined
|
||||
}
|
||||
```
|
||||
|
||||
**4. Verify:**
|
||||
- [ ] New test passes
|
||||
- [ ] Existing tests pass
|
||||
- [ ] Manual testing confirms fix
|
||||
```
|
||||
|
||||
**For Features:**
|
||||
```markdown
|
||||
### Feature Implementation
|
||||
|
||||
**1. Define interface:**
|
||||
```typescript
|
||||
// Define types/interfaces first
|
||||
interface NewFeature {
|
||||
doSomething(param: Type): ReturnType
|
||||
}
|
||||
```
|
||||
|
||||
**2. Implement core logic:**
|
||||
```javascript
|
||||
function coreFunction() {
|
||||
// Happy path first
|
||||
// Clear, readable code
|
||||
// One level of abstraction
|
||||
}
|
||||
```
|
||||
|
||||
**3. Handle edge cases:**
|
||||
```javascript
|
||||
function coreFunction(input) {
|
||||
// Validate inputs
|
||||
if (!isValid(input)) {
|
||||
throw new Error('Invalid input: ...')
|
||||
}
|
||||
|
||||
// Handle empty/null
|
||||
if (isEmpty(input)) {
|
||||
return defaultValue
|
||||
}
|
||||
|
||||
// Core logic
|
||||
const result = process(input)
|
||||
|
||||
// Post-conditions
|
||||
validateResult(result)
|
||||
|
||||
return result
|
||||
}
|
||||
```
|
||||
|
||||
**4. Add tests for each path:**
|
||||
- [ ] Happy path
|
||||
- [ ] Edge cases
|
||||
- [ ] Error cases
|
||||
```
|
||||
|
||||
**For Refactoring:**
|
||||
```markdown
|
||||
### Refactoring Implementation
|
||||
|
||||
**1. Ensure test coverage:**
|
||||
- [ ] Tests exist for current behavior
|
||||
- [ ] Tests are comprehensive
|
||||
- [ ] Tests pass before refactoring
|
||||
|
||||
**2. Refactor incrementally:**
|
||||
|
||||
**Step 1:** Extract function
|
||||
```javascript
|
||||
// Before:
|
||||
function complexFunction() {
|
||||
// Long complex code
|
||||
// Mixing concerns
|
||||
// Hard to understand
|
||||
}
|
||||
|
||||
// After Step 1:
|
||||
function complexFunction() {
|
||||
const data = fetchData()
|
||||
const processed = processData(data)
|
||||
return formatOutput(processed)
|
||||
}
|
||||
|
||||
function fetchData() { /* extracted */ }
|
||||
function processData(data) { /* extracted */ }
|
||||
function formatOutput(processed) { /* extracted */ }
|
||||
```
|
||||
|
||||
**Step 2:** Improve naming
|
||||
**Step 3:** Remove duplication
|
||||
**Step 4:** Simplify logic
|
||||
|
||||
**After each step:**
|
||||
- [ ] Tests still pass
|
||||
- [ ] Code is more readable
|
||||
- [ ] Behavior unchanged
|
||||
```
|
||||
|
||||
### Step 4: Test Implementation
|
||||
|
||||
Write comprehensive tests.
|
||||
|
||||
**Test coverage goals:**
|
||||
|
||||
```markdown
|
||||
### Test Strategy
|
||||
|
||||
**Unit tests:** (test individual functions)
|
||||
- Location: [where tests go]
|
||||
- Coverage target: [percentage or specific cases]
|
||||
|
||||
**Test cases:**
|
||||
|
||||
**Happy path:**
|
||||
```javascript
|
||||
it('should work with valid input', () => {
|
||||
expect(feature(validInput)).toBe(expected)
|
||||
})
|
||||
```
|
||||
|
||||
**Edge cases:**
|
||||
```javascript
|
||||
it('should handle empty input', () => {
|
||||
expect(feature([])).toBe(defaultValue)
|
||||
})
|
||||
|
||||
it('should handle null input', () => {
|
||||
expect(feature(null)).toThrow('Invalid input')
|
||||
})
|
||||
|
||||
it('should handle large input', () => {
|
||||
const largeInput = generateLargeData()
|
||||
expect(feature(largeInput)).toBeDefined()
|
||||
})
|
||||
```
|
||||
|
||||
**Error cases:**
|
||||
```javascript
|
||||
it('should throw on invalid input', () => {
|
||||
expect(() => feature(invalid)).toThrow()
|
||||
})
|
||||
|
||||
it('should provide clear error message', () => {
|
||||
expect(() => feature(invalid))
|
||||
.toThrow('Expected X but got Y')
|
||||
})
|
||||
```
|
||||
|
||||
**Integration tests:** (if needed)
|
||||
```javascript
|
||||
it('should integrate with existing feature', () => {
|
||||
// Test feature working with other parts
|
||||
})
|
||||
```
|
||||
|
||||
**Snapshot tests:** (for UI, if applicable)
|
||||
```javascript
|
||||
it('should render correctly', () => {
|
||||
const tree = renderer.create(<Component />).toJSON()
|
||||
expect(tree).toMatchSnapshot()
|
||||
})
|
||||
```
|
||||
```
|
||||
|
||||
**Test best practices:**
|
||||
|
||||
```markdown
|
||||
### Test Quality
|
||||
|
||||
**Clear and focused:**
|
||||
- [ ] One assertion per test (generally)
|
||||
- [ ] Clear test names describe behavior
|
||||
- [ ] Tests are independent
|
||||
- [ ] No test interdependencies
|
||||
|
||||
**Readable:**
|
||||
- [ ] Arrange-Act-Assert structure
|
||||
- [ ] Descriptive variable names
|
||||
- [ ] Comments for complex setups
|
||||
- [ ] DRY (use helpers/fixtures)
|
||||
|
||||
**Maintainable:**
|
||||
- [ ] Tests don't test implementation details
|
||||
- [ ] Tests are resilient to refactoring
|
||||
- [ ] Test data is realistic
|
||||
- [ ] Mocks/stubs are minimal
|
||||
|
||||
**Fast:**
|
||||
- [ ] Unit tests run quickly
|
||||
- [ ] Minimal external dependencies
|
||||
- [ ] No unnecessary setup/teardown
|
||||
```
|
||||
|
||||
### Step 5: Local Verification
|
||||
|
||||
Test thoroughly before committing.
|
||||
|
||||
**Verification checklist:**
|
||||
|
||||
```markdown
|
||||
### Local Testing
|
||||
|
||||
**Build:**
|
||||
```bash
|
||||
# Clean build
|
||||
[build command]
|
||||
|
||||
# Check for warnings
|
||||
[any warnings to address?]
|
||||
```
|
||||
|
||||
**Tests:**
|
||||
```bash
|
||||
# Run all tests
|
||||
[test command]
|
||||
# ✅ All pass
|
||||
|
||||
# Run with coverage
|
||||
[coverage command]
|
||||
# ✅ Coverage meets threshold
|
||||
|
||||
# Run specific affected tests
|
||||
[command for affected tests]
|
||||
# ✅ Pass
|
||||
```
|
||||
|
||||
**Linting:**
|
||||
```bash
|
||||
# Check code style
|
||||
[lint command]
|
||||
# ✅ No errors
|
||||
|
||||
# Auto-fix if available
|
||||
[format command]
|
||||
```
|
||||
|
||||
**Type checking (if applicable):**
|
||||
```bash
|
||||
[type check command]
|
||||
# ✅ No type errors
|
||||
```
|
||||
|
||||
**Manual testing:**
|
||||
- [ ] Feature works as expected
|
||||
- [ ] Edge cases handled
|
||||
- [ ] Error messages clear
|
||||
- [ ] UI looks correct (if applicable)
|
||||
- [ ] Performance acceptable
|
||||
|
||||
**Regression check:**
|
||||
- [ ] Existing features still work
|
||||
- [ ] No unintended side effects
|
||||
- [ ] Related functionality intact
|
||||
```
|
||||
|
||||
### Step 6: Code Review Self-Check
|
||||
|
||||
Review your own code before submitting.
|
||||
|
||||
**Self-review checklist:**
|
||||
|
||||
```markdown
|
||||
### Self Code Review
|
||||
|
||||
**Completeness:**
|
||||
- [ ] All requirements implemented
|
||||
- [ ] All edge cases handled
|
||||
- [ ] All acceptance criteria met
|
||||
- [ ] No TODO/FIXME left unaddressed
|
||||
|
||||
**Code quality:**
|
||||
- [ ] Code is self-explanatory
|
||||
- [ ] Complex parts have comments
|
||||
- [ ] No dead code or commented-out code
|
||||
- [ ] No debugging code (console.log, etc)
|
||||
|
||||
**Changes are minimal:**
|
||||
- [ ] Only changed what's necessary
|
||||
- [ ] No unrelated refactoring
|
||||
- [ ] No formatting-only changes mixed in
|
||||
- [ ] Git diff is clean and focused
|
||||
|
||||
**Testing:**
|
||||
- [ ] Tests are comprehensive
|
||||
- [ ] Tests are meaningful (not just for coverage)
|
||||
- [ ] Tests pass consistently
|
||||
- [ ] Edge cases have tests
|
||||
|
||||
**Documentation:**
|
||||
- [ ] Public APIs documented
|
||||
- [ ] Complex logic explained
|
||||
- [ ] README updated if needed
|
||||
- [ ] Migration guide if breaking
|
||||
|
||||
**Security:**
|
||||
- [ ] No secrets/credentials committed
|
||||
- [ ] User input validated
|
||||
- [ ] No obvious vulnerabilities
|
||||
- [ ] Dependencies are safe
|
||||
|
||||
**Performance:**
|
||||
- [ ] No performance regressions
|
||||
- [ ] Algorithms are efficient
|
||||
- [ ] Resource usage reasonable
|
||||
- [ ] No memory leaks
|
||||
|
||||
**Commits:**
|
||||
- [ ] Commits are logical
|
||||
- [ ] Commit messages are clear
|
||||
- [ ] No merge commits from main (rebase if needed)
|
||||
- [ ] History is clean
|
||||
```
|
||||
|
||||
### Step 7: Documentation
|
||||
|
||||
Document your changes appropriately.
|
||||
|
||||
**Documentation types:**
|
||||
|
||||
```markdown
|
||||
### Documentation Updates
|
||||
|
||||
**Code comments:**
|
||||
```javascript
|
||||
/**
|
||||
* Converts user data to CSV format
|
||||
*
|
||||
* @param {User[]} users - Array of user objects
|
||||
* @param {ExportOptions} options - Export configuration
|
||||
* @returns {string} CSV formatted string
|
||||
* @throws {ValidationError} If users array is invalid
|
||||
*
|
||||
* @example
|
||||
* const csv = exportToCSV(users, { includeHeaders: true })
|
||||
*/
|
||||
function exportToCSV(users, options) {
|
||||
// Implementation
|
||||
}
|
||||
```
|
||||
|
||||
**README updates:**
|
||||
- [ ] New features documented
|
||||
- [ ] Usage examples added
|
||||
- [ ] Installation steps updated
|
||||
- [ ] API changes reflected
|
||||
|
||||
**CHANGELOG:**
|
||||
- [ ] Entry added with:
|
||||
- Version (if applicable)
|
||||
- Date
|
||||
- Type: [Added/Changed/Fixed/Removed]
|
||||
- Description
|
||||
|
||||
```markdown
|
||||
## [Unreleased]
|
||||
### Added
|
||||
- Export to CSV functionality (#123)
|
||||
- New `exportToCSV` function
|
||||
- Customizable column selection
|
||||
- Header row optional
|
||||
```
|
||||
|
||||
**API documentation:**
|
||||
- [ ] New endpoints documented
|
||||
- [ ] Parameters described
|
||||
- [ ] Response format specified
|
||||
- [ ] Error codes listed
|
||||
- [ ] Examples provided
|
||||
|
||||
**Migration guide (if breaking):**
|
||||
```markdown
|
||||
## Migrating to v2.0
|
||||
|
||||
### Breaking Changes
|
||||
|
||||
**Function signature changed:**
|
||||
```javascript
|
||||
// Old:
|
||||
doSomething(param)
|
||||
|
||||
// New:
|
||||
doSomething(param, options)
|
||||
```
|
||||
|
||||
**Migration steps:**
|
||||
1. Find all calls to `doSomething`
|
||||
2. Add empty options object: `doSomething(param, {})`
|
||||
3. Configure options as needed
|
||||
```
|
||||
```
|
||||
|
||||
## Implementation Best Practices
|
||||
|
||||
**Start small:**
|
||||
- Implement core functionality first
|
||||
- Add features incrementally
|
||||
- Test each increment
|
||||
- Commit working states
|
||||
|
||||
**Follow patterns:**
|
||||
- Match existing code style
|
||||
- Use same abstractions
|
||||
- Don't introduce new paradigms
|
||||
- Be consistent
|
||||
|
||||
**Communicate:**
|
||||
- Ask questions early
|
||||
- Share progress on complex issues
|
||||
- Discuss tradeoffs
|
||||
- Welcome feedback
|
||||
|
||||
**Iterate:**
|
||||
- Don't aim for perfection first try
|
||||
- Reviewers will have feedback
|
||||
- Be open to changes
|
||||
- Learn from reviews
|
||||
|
||||
## Common Pitfalls
|
||||
|
||||
**Avoid:**
|
||||
|
||||
❌ **Scope creep** - Stick to issue requirements
|
||||
❌ **Over-engineering** - Keep it simple
|
||||
❌ **Ignoring conventions** - Match project style
|
||||
❌ **Skipping tests** - Tests are requirements
|
||||
❌ **Poor commit messages** - Future you will be confused
|
||||
❌ **Not testing edge cases** - Where bugs hide
|
||||
❌ **Mixing concerns** - One logical change per commit
|
||||
❌ **Committing secrets** - Check before pushing
|
||||
|
||||
## Output Format
|
||||
|
||||
Provide implementation summary:
|
||||
|
||||
```markdown
|
||||
# ✅ Implementation Complete: [Issue Title]
|
||||
|
||||
**Issue:** #[number]
|
||||
**Branch:** [branch-name]
|
||||
|
||||
---
|
||||
|
||||
## Changes Made
|
||||
|
||||
### Core Implementation
|
||||
|
||||
**1. [Primary change]**
|
||||
- File: `[path]`
|
||||
- Changes: [description]
|
||||
- Lines modified: ~[count]
|
||||
|
||||
**2. [Secondary change]**
|
||||
- File: `[path]`
|
||||
- Changes: [description]
|
||||
|
||||
### Tests Added
|
||||
|
||||
**Unit tests:**
|
||||
- `[test-file]` - [N] test cases
|
||||
- Happy path
|
||||
- Edge cases: [list]
|
||||
- Error cases: [list]
|
||||
|
||||
**Coverage:** [percentage] (previous: [X]%, delta: +[Y]%)
|
||||
|
||||
### Documentation
|
||||
|
||||
- [ ] Code comments added
|
||||
- [ ] README updated
|
||||
- [ ] CHANGELOG entry added
|
||||
- [ ] API docs updated
|
||||
|
||||
---
|
||||
|
||||
## Verification
|
||||
|
||||
**Local testing:**
|
||||
- ✅ All tests pass
|
||||
- ✅ Linting passes
|
||||
- ✅ Build succeeds
|
||||
- ✅ Manual testing complete
|
||||
|
||||
**Checklist:**
|
||||
- ✅ Requirements met
|
||||
- ✅ Edge cases handled
|
||||
- ✅ Follows conventions
|
||||
- ✅ Self-review complete
|
||||
|
||||
---
|
||||
|
||||
## Commits
|
||||
|
||||
```bash
|
||||
# Commit history
|
||||
git log --oneline origin/main..HEAD
|
||||
```
|
||||
|
||||
**Summary:**
|
||||
- [N] commits
|
||||
- All commits are focused and logical
|
||||
- Commit messages are clear
|
||||
|
||||
---
|
||||
|
||||
## Next Steps
|
||||
|
||||
✅ Implementation complete - Ready for **Phase 4: Documentation & PR**
|
||||
|
||||
**Before PR:**
|
||||
- [ ] Final review
|
||||
- [ ] Rebase on main (if needed)
|
||||
- [ ] Squash commits (if needed)
|
||||
- [ ] Push to remote
|
||||
```
|
||||
|
||||
## Integration with Main Framework
|
||||
|
||||
When invoked from main framework:
|
||||
|
||||
1. **Receive context:** Issue analysis from Phase 2, requirements, code locations
|
||||
2. **Guide implementation:** Step-by-step with quality checks
|
||||
3. **Verify completion:** All requirements met, tests pass
|
||||
4. **Return summary:** What was implemented and how
|
||||
5. **Update tracker:** Mark Phase 3 complete
|
||||
6. **Transition:** Ready for documentation and PR creation
|
||||
|
||||
Implementation phase may iterate with Phase 2 if code structure different than expected.
|
||||
|
||||
## Incremental Development
|
||||
|
||||
**For large implementations:**
|
||||
|
||||
1. **Milestone 1:** Core functionality
|
||||
- Implement minimal working version
|
||||
- Add basic tests
|
||||
- Commit: "feat: add core X functionality"
|
||||
|
||||
2. **Milestone 2:** Edge cases
|
||||
- Handle edge cases
|
||||
- Add edge case tests
|
||||
- Commit: "feat: handle X edge cases"
|
||||
|
||||
3. **Milestone 3:** Polish
|
||||
- Error messages
|
||||
- Documentation
|
||||
- Commit: "docs: add X documentation"
|
||||
|
||||
**Benefits:**
|
||||
- Can get early feedback
|
||||
- Easier to review
|
||||
- Simpler to debug
|
||||
- Clear progress tracking
|
||||
Reference in New Issue
Block a user