Pull Request Process

Guidelines for submitting and reviewing pull requests

Pull Request Process

Comprehensive guide for submitting high-quality pull requests and participating in the code review process.

Before Submitting

Pre-Submission Checklist

Complete this checklist before submitting your pull request:

  • Self-Review: Review your own code for quality and consistency
  • Test Coverage: Ensure adequate test coverage for new functionality
  • Documentation: Update documentation as needed
  • Lint and Type Check: Run all code quality checks
  • Build Test: Verify the application builds successfully
  • Performance: Consider performance implications of changes
  • Security: Review for potential security issues
  • Breaking Changes: Document any breaking changes

Code Quality Checks

Run these commands before submitting:

# Type checking
pnpm type-check

# Linting
pnpm lint

# Format code
pnpm format

# Run tests
pnpm test

# Build application
pnpm build

Pull Request Template

Use this template when creating pull requests:

## Description
Brief description of changes made and the problem they solve.

## 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
- [ ] Performance improvement
- [ ] Code refactoring

## Related Issues
Closes #123
Related to #456

## Changes Made
- Added barcode scanning functionality
- Updated product search to include SKU matching
- Improved error handling in inventory service
- Added unit tests for new features

## Testing
- [ ] Unit tests added/updated
- [ ] Integration tests added/updated
- [ ] Manual testing completed
- [ ] Regression testing performed

## Screenshots (if applicable)
Include screenshots of UI changes or new features.

## Performance Impact
Describe any performance implications of the changes.

## Breaking Changes
List any breaking changes and migration steps.

## Deployment Notes
Any special deployment considerations or database migrations.

## Checklist
- [ ] Code follows project style guidelines
- [ ] Self-review completed
- [ ] Tests added and passing
- [ ] Documentation updated
- [ ] No breaking changes or breaking changes documented
- [ ] Performance impact considered
- [ ] Security implications reviewed

Review Process

Automated Checks

All pull requests must pass these automated checks:

  • Type Checking: Ensures TypeScript compilation without errors
  • Linting: Checks code style and quality using ESLint
  • Testing: Runs unit and integration tests
  • Build: Verifies successful application build
  • Security: Scans for known vulnerabilities
  • Coverage: Ensures test coverage meets minimum requirements

Human Review Requirements

  • Minimum Reviews: At least one approved review required
  • Domain Expertise: Complex changes may require domain expert review
  • Security Review: Security-sensitive changes require security team review
  • Breaking Changes: Breaking changes require maintainer approval

Review Guidelines

For Authors

  1. Provide Context: Clearly explain what and why in the PR description
  2. Keep Changes Focused: Submit small, focused PRs when possible
  3. Respond Promptly: Address reviewer feedback in a timely manner
  4. Test Thoroughly: Ensure changes work as expected
  5. Update Documentation: Keep documentation in sync with code changes

For Reviewers

  1. Be Constructive: Provide helpful, actionable feedback
  2. Focus on Important Issues: Prioritize correctness, security, and maintainability
  3. Ask Questions: Seek clarification when code intent is unclear
  4. Suggest Improvements: Offer specific suggestions for improvement
  5. Acknowledge Good Work: Recognize well-written code and good practices

Review Checklist

Code Quality

  • Code is readable and well-organized
  • Functions and classes have single responsibilities
  • Variable and function names are descriptive
  • Code follows established patterns and conventions
  • No code duplication or copy-paste errors

Functionality

  • Changes meet the stated requirements
  • Edge cases are handled appropriately
  • Error handling is comprehensive
  • Performance is acceptable
  • No obvious bugs or logical errors

Testing

  • New functionality has adequate test coverage
  • Tests are meaningful and test the right things
  • Tests pass consistently
  • Integration tests cover important workflows
  • Manual testing has been performed

Security

  • Input validation is implemented
  • Authorization checks are in place
  • Sensitive data is handled properly
  • No hardcoded secrets or credentials
  • SQL injection and XSS vulnerabilities addressed

Documentation

  • Code changes are documented
  • Public APIs have proper documentation
  • README and other docs are updated
  • Breaking changes are documented
  • Migration guides provided if needed

Merging Guidelines

Merge Requirements

Before merging, ensure:

  • All automated checks pass
  • Required approvals obtained
  • Conflicts resolved
  • Documentation updated
  • Breaking changes documented

Merge Strategies

We use Squash and Merge for most changes:

  • Keeps commit history clean
  • Combines related commits into single commit
  • Preserves PR discussion context
  • Easier to revert if needed

Use Merge Commit for:

  • Large feature branches with meaningful commit history
  • Releases and version bumps
  • When preserving detailed commit history is important

Post-Merge

After merging:

  1. Delete Feature Branch: Clean up merged branches
  2. Monitor Deployments: Watch for deployment issues
  3. Update Issues: Close related issues
  4. Notify Stakeholders: Inform relevant team members

Common Issues and Solutions

Large Pull Requests

Problem: PR is too large and difficult to review

Solutions:

  • Break into smaller, focused PRs
  • Use stacked PRs for related changes
  • Provide detailed description and context
  • Offer to walk reviewers through changes

Merge Conflicts

Problem: Conflicts with main branch

Solutions:

# Update your branch
git checkout main
git pull origin main
git checkout your-branch
git rebase main

# Resolve conflicts and continue
git add .
git rebase --continue

# Force push (if needed)
git push --force-with-lease origin your-branch

Failed Checks

Problem: Automated checks failing

Solutions:

  • Review error messages carefully
  • Run checks locally before pushing
  • Fix issues incrementally
  • Ask for help if needed

Slow Review Process

Problem: PR not getting reviewed quickly

Solutions:

  • Request reviews from specific team members
  • Provide more context in PR description
  • Break large PRs into smaller ones
  • Follow up politely after reasonable time

Best Practices

Writing Good PRs

  1. Clear Title: Descriptive title that explains the change
  2. Detailed Description: Explain what, why, and how
  3. Small and Focused: Keep changes small and focused
  4. Test Coverage: Include appropriate tests
  5. Documentation: Update relevant documentation

Reviewing Effectively

  1. Timely Reviews: Review PRs promptly
  2. Thorough Examination: Check code, tests, and documentation
  3. Constructive Feedback: Provide helpful, specific feedback
  4. Security Focus: Pay attention to security implications
  5. Learning Opportunity: Use reviews as learning opportunities

Communication

  1. Be Respectful: Maintain professional, respectful tone
  2. Ask Questions: Don't hesitate to ask for clarification
  3. Explain Reasoning: Provide rationale for suggestions
  4. Acknowledge Efforts: Recognize good work and improvements
  5. Follow Up: Ensure feedback is addressed appropriately