Code Review Standards

Pull Request Review Process for iSu Technologies

🎯 Purpose

Code reviews ensure:

Quality: Catch bugs before production
Security: Identify vulnerabilities
Knowledge Sharing: Team learns from each other
Consistency: Maintain coding standards
POPIA Compliance: Verify data handling

All code must be reviewed before merging to main.

📋 PR Submission Checklist

Before Creating PR:

Code compiles without errors
All tests pass locally
New tests added for new features
No console.log() or debugging code left
Code formatted (Prettier/ESLint)
No sensitive data hardcoded (API keys, passwords)
Branch up-to-date with main
Self-review completed

📝 PR Description Template

## What Changed
[Brief description of what this PR does]

## Why
[Business/technical reason for change]

## How to Test
1. Checkout branch: `git checkout feature/xyz`
2. Run: `npm install && npm run dev`
3. Navigate to: [URL or feature location]
4. Expected behavior: [What reviewer should see]

## Screenshots/Video
[If UI change, include before/after screenshots]

## Checklist
- [ ] Tests added/updated
- [ ] Documentation updated
- [ ] No breaking changes (or migration plan provided)
- [ ] POPIA compliance verified (if handling customer data)
- [ ] Security considerations reviewed

## Related Issues
Closes #123
Related to #456

👀 Review Process

Step 1: Automated Checks (CI/CD)

✅ Build passes
✅ All tests pass
✅ Lint checks pass
✅ Code coverage ≥ 80%
✅ Security scan passes (no critical/high vulnerabilities)

If automated checks fail: Fix before requesting review

Step 2: Human Review

Who Reviews:

Assign 1-2 reviewers (use GitHub’s reviewer feature)
At least 1 approval required to merge
Tech lead approval required for:
- Security-sensitive changes
- Database migrations
- API changes
- Architecture changes

Review Timeline:

Simple PRs (<50 lines): 4 hours
Medium PRs (50-200 lines): 24 hours
Large PRs (200+ lines): 48 hours

If urgent: Tag with urgent label and Slack reviewer

Step 3: Reviewer Responsibilities

What to Review:

1. Functionality

Does code do what PR description says?
Are edge cases handled?
Are error states handled gracefully?

2. Code Quality

Is code readable and maintainable?
Are variable/function names clear?
Is logic simple or overly complex?
Are there code smells (duplicated code, long functions)?

3. Testing

Are new features tested?
Do tests cover edge cases?
Are test names descriptive?

4. Security

No hardcoded secrets or API keys
User input validated and sanitized
SQL injection prevention (parameterized queries)
XSS prevention (escaped output)
Authentication/authorization checked
POPIA compliance (customer data handling)

5. Performance

No N+1 queries
Efficient algorithms
Appropriate caching
Database indexes used

6. Documentation

Complex logic commented
API changes documented
README updated if needed

💬 Review Comments

Types of Comments:

🔴 MUST FIX (Blocking)

Security vulnerabilities
Bugs that break functionality
POPIA violations
Performance issues causing significant slowdown

🟡 SHOULD FIX (Non-blocking but important)

Code quality issues
Missing tests
Poor naming
Refactoring opportunities

🟢 NITS (Optional, personal preference)

Style preferences
Minor formatting
Alternative approaches (both valid)

💡 LEARNING (Informational)

Explaining why something works
Sharing best practices
Suggesting resources

Comment Examples:

Good Comments:

🔴 MUST FIX: SQL injection vulnerability here.
Use parameterized query instead:
`db.query('SELECT * FROM users WHERE id = ?', [userId])`

🟡 SHOULD FIX: This function is 80 lines long and does multiple things.
Consider breaking into smaller functions:
- validateInput()
- processData()
- saveToDatabase()

🟢 NIT: Personal preference, but I find `isActive` clearer than `active`
for boolean variables. Either works though!

💡 LEARNING: Nice use of the builder pattern here! For others reading,
here's a good article on this: [link]

Bad Comments:

❌ "This is wrong" (no explanation)
❌ "Why didn't you use X?" (condescending)
❌ "I would have done this differently" (not constructive)

✅ Approval Criteria

Approve when:

All MUST FIX comments addressed
Tests pass
Code meets quality standards
You’d be comfortable if this broke production (you’d know how to fix it)

Request Changes when:

MUST FIX comments not addressed
Tests failing
Security concerns
Significant quality issues

Comment Only (no approval/rejection) when:

Minor nits
Learning comments
Questions for clarification

🔄 After Review

Author Responsibilities:

If changes requested:

Address all MUST FIX comments
Address SHOULD FIX comments (or explain why not)
Push new commits (don’t force push - reviewers need to see changes)
Re-request review from same reviewers
Comment on addressed items: “Fixed in abc123”

If approved:

Merge using “Squash and merge” (clean history)
Delete feature branch after merge
Deploy to staging (automatic via CI/CD)
Verify in staging
Create production deployment (following deployment process)

📊 PR Size Guidelines

Ideal PR Size: 50-200 lines

Too Small (<20 lines):

Okay for: Bug fixes, typos, config changes
Consider: Batch related small changes

Too Large (>500 lines):

Hard to review thoroughly
High chance of missing issues
Solution: Break into smaller PRs
- Example: If adding a new feature
  - PR 1: Database schema
  - PR 2: Backend API
  - PR 3: Frontend UI
  - PR 4: Tests and documentation

🚨 Emergency Hotfixes

For production-breaking bugs only:

Create hotfix branch from main
Fix the specific bug (minimal code change)
Tag PR with hotfix label
Get expedited review (30 min turnaround)
Merge and deploy immediately
Create follow-up PR for proper fix if needed

Not for:

Features disguised as hotfixes
Skipping tests
Avoiding review

📈 Code Review Metrics

Track and improve:

Review turnaround time: Target <24 hours
PR merge rate: Target 95% (5% rejected/closed)
Bugs caught in review: Celebrate this!
Bugs escaped to production: Learn from these

Monthly review:

Which reviewers are fastest?
Which reviewers catch most bugs?
Are PRs getting smaller over time?

💡 Best Practices

For Authors:

✅ Keep PRs small and focused ✅ Write clear descriptions ✅ Self-review before requesting review ✅ Respond to feedback promptly ✅ Be receptive to criticism (it’s about code, not you) ✅ Ask questions if feedback unclear

For Reviewers:

✅ Review within 24 hours ✅ Be constructive and kind ✅ Explain reasoning for suggestions ✅ Approve if it’s “good enough” (don’t block on nits) ✅ Pair program for complex reviews ✅ Learn from code you review

🎓 Learning Resources

Improve your code review skills:

Google’s Code Review Guide
The Art of Giving and Receiving Code Reviews
Internal: Review PRs from senior developers to see quality standards

❓ FAQs

Q: What if reviewer is taking too long? A: Politely ping after 24 hours. If still no response, ask tech lead to assign different reviewer.

Q: What if I disagree with review feedback? A: Discuss! Code reviews are conversations. If you can’t agree, escalate to tech lead.

Q: Do I need approval for documentation-only changes? A: Yes, but these should be fast. Typo fixes can be self-merged after 4 hours if no feedback.

Q: Can I merge my own PR? A: No, except emergency hotfixes with tech lead approval.

Q: What if automated checks fail but I think it’s a false positive? A: Don’t override. Fix the test or the check configuration. Never “skip CI”.

iSu Technologies Code Review Standards Ship quality code through collaborative review Last Updated: 09/11/2025