Skip to main content
Version: Next

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​

External​

Next: Reporting issues effectively