superset2/docs/developer_docs_versioned_docs/version-6.1.0/contributing/code-review.md

title, sidebar_position

title	sidebar_position
Code Review Process	4

Code Review Process

Understand how code reviews work in Apache Superset and how to participate effectively.

Overview

Code review is a critical part of maintaining code quality and sharing knowledge across the team. Every change to Superset goes through peer review before merging.

For Authors

Preparing for Review

Before Requesting Review

• Self-review your changes
• Ensure CI checks pass
• Add comprehensive tests
• Update documentation
• Fill out PR template completely
• Add screenshots for UI changes

Self-Review Checklist

# View your changes
git diff upstream/master

# Check for common issues:
# - Commented out code
# - Debug statements (console.log, print)
# - TODO comments that should be addressed
# - Hardcoded values that should be configurable
# - Missing error handling
# - Performance implications

Requesting Review

Auto-Assignment

GitHub will automatically request reviews based on CODEOWNERS file.

Manual Assignment

For specific expertise, request additional reviewers:

• Frontend changes: Tag frontend experts
• Backend changes: Tag backend experts
• Security changes: Tag security team
• Database changes: Tag database experts

Review Request Message

@reviewer This PR implements [feature]. Could you please review:
1. The approach taken in [file]
2. Performance implications of [change]
3. Security considerations for [feature]

Thanks!

Responding to Feedback

Best Practices

• Be receptive: Reviews improve code quality
• Ask questions: Clarify if feedback is unclear
• Explain decisions: Share context for your choices
• Update promptly: Address feedback in timely manner

Comment Responses

# Acknowledging
"Good catch! Fixed in [commit hash]"

# Explaining
"I chose this approach because [reason]. Would you prefer [alternative]?"

# Questioning
"Could you elaborate on [concern]? I'm not sure I understand the issue."

# Disagreeing respectfully
"I see your point, but I think [current approach] because [reason]. What do you think?"

For Reviewers

Review Responsibilities

What to Review

1. Correctness: Does the code do what it claims?
2. Design: Is the approach appropriate?
3. Clarity: Is the code readable and maintainable?
4. Testing: Are tests comprehensive?
5. Performance: Any performance concerns?
6. Security: Any security issues?
7. Documentation: Is it well documented?

Review Checklist

Functionality

• Feature works as described
• Edge cases are handled
• Error handling is appropriate
• Backwards compatibility maintained

Code Quality

• Follows project conventions
• No code duplication
• Clear variable/function names
• Appropriate abstraction levels
• SOLID principles followed

Testing

• Unit tests for business logic
• Integration tests for APIs
• E2E tests for critical paths
• Tests are maintainable
• Good test coverage

Security

• Input validation
• SQL injection prevention
• XSS prevention
• CSRF protection
• Authentication/authorization checks
• No sensitive data in logs

Performance

• Database queries optimized
• No N+1 queries
• Appropriate caching
• Frontend bundle size impact
• Memory usage considerations

Providing Feedback

Effective Comments

# ✅ Good: Specific and actionable
"This query could cause N+1 problems. Consider using
`select_related('user')` to fetch users in a single query."

# ❌ Bad: Vague
"This doesn't look right."

// ✅ Good: Suggests improvement
"Consider using useMemo here to prevent unnecessary
re-renders when dependencies haven't changed."

// ❌ Bad: Just criticism
"This is inefficient."

Comment Types

Use GitHub's comment types:

• Comment: General feedback or questions
• Approve: Changes look good
• Request Changes: Must be addressed before merge

Prefix conventions:

• nit: Minor issue (non-blocking)
• suggestion: Recommended improvement
• question: Seeking clarification
• blocker: Must be fixed
• praise: Highlighting good work

Examples

nit: Consider renaming `getData` to `fetchUserData` for clarity

suggestion: This could be simplified using Array.reduce()

question: Is this intentionally not handling the error case?

blocker: This SQL is vulnerable to injection. Please use parameterized queries.

praise: Excellent test coverage! 👍

Review Process

Timeline

Expected Response Times

• Initial review: Within 2-3 business days
• Follow-up review: Within 1-2 business days
• Critical fixes: ASAP (tag in Slack)

Escalation

If no response after 3 days:

1. Ping reviewer in PR comments
2. Ask in #development Slack channel
3. Tag @apache/superset-committers

Approval Requirements

Minimum Requirements

• 1 approval from a committer for minor changes
• 2 approvals for significant features
• 3 approvals for breaking changes

Special Cases

• Security changes: Require security team review
• API changes: Require API team review
• Database migrations: Require database expert review
• UI/UX changes: Require design review

Merge Process

Who Can Merge

• Committers with write access
• After all requirements met
• CI checks must pass

Merge Methods

• Squash and merge: Default for feature PRs
• Rebase and merge: For clean history
• Create merge commit: Rarely used

Merge Checklist

• All CI checks green
• Required approvals obtained
• No unresolved conversations
• PR title follows conventions
• Milestone set (if applicable)

Review Etiquette

Do's

• ✅ Be kind and constructive
• ✅ Acknowledge time and effort
• ✅ Provide specific examples
• ✅ Suggest solutions
• ✅ Praise good work
• ✅ Consider cultural differences
• ✅ Focus on the code, not the person

Don'ts

• ❌ Use harsh or dismissive language
• ❌ Bikeshed on minor preferences
• ❌ Review when tired or frustrated
• ❌ Make personal attacks
• ❌ Ignore the PR description
• ❌ Demand perfection

Becoming a Reviewer

Path to Reviewer

1. Contribute regularly: Submit quality PRs
2. Participate in discussions: Share knowledge
3. Review others' code: Start with comments
4. Build expertise: Focus on specific areas
5. Get nominated: By existing committers

Reviewer Expectations

• Review PRs in your area of expertise
• Respond within reasonable time
• Mentor new contributors
• Maintain high standards
• Stay current with best practices

Advanced Topics

Reviewing Large PRs

Strategy

1. Request splitting: Ask to break into smaller PRs
2. Review in phases:
   • Architecture/approach first
   • Implementation details second
   • Tests and docs last
3. Use draft reviews: Save comments and submit together

Cross-Team Reviews

When Needed

• Changes affecting multiple teams
• Shared components/libraries
• API contract changes
• Database schema changes

Performance Reviews

Tools

# Backend performance
import cProfile
import pstats

# Profile the code
cProfile.run('function_to_profile()', 'stats.prof')
stats = pstats.Stats('stats.prof')
stats.sort_stats('cumulative').print_stats(10)

// Frontend performance
// Use React DevTools Profiler
// Chrome DevTools Performance tab
// Lighthouse audits

Resources

Internal

• Coding Guidelines
• Testing Guide
• Extension Architecture

External

• Google's Code Review Guide
• Best Practices for Code Review
• The Art of Readable Code

Next: Reporting issues effectively