Sentry Code Review
Follow these guidelines when reviewing code for Sentry projects.
Review Checklist
Identifying Problems
Look for these issues in code changes:
- Runtime errors: Potential exceptions, null pointer issues, out-of-bounds access
- Performance: Unbounded O(n²) operations, N+1 queries, unnecessary allocations
- Side effects: Unintended behavioral changes affecting other components
- Backwards compatibility: Breaking API changes without migration path
- ORM queries: Complex Django ORM with unexpected query performance
- Security vulnerabilities: Injection, XSS, access control gaps, secrets exposure
Design Assessment
- Do component interactions make logical sense?
- Does the change align with existing project architecture?
- Are there conflicts with current requirements or goals?
Test Coverage
Every PR should have appropriate test coverage:
- Functional tests for business logic
- Integration tests for component interactions
- End-to-end tests for critical user paths
Verify tests cover actual requirements and edge cases. Avoid excessive branching or looping in test code.
Long-Term Impact
Flag for senior engineer review when changes involve:
- Database schema modifications
- API contract changes
- New framework or library adoption
- Performance-critical code paths
- Security-sensitive functionality
Feedback Guidelines
Tone
- Be polite and empathetic
- Provide actionable suggestions, not vague criticism
- Phrase as questions when uncertain: "Have you considered...?"
Approval
- Approve when only minor issues remain
- Don't block PRs for stylistic preferences
- Remember: the goal is risk reduction, not perfect code
Common Patterns to Flag
Python/Django
# Bad: N+1 query
for user in users:
print(user.profile.name) # Separate query per user
# Good: Prefetch related
users = User.objects.prefetch_related('profile')
TypeScript/React
// Bad: Missing dependency in useEffect
useEffect(() => {
fetchData(userId);
}, []); // userId not in deps
// Good: Include all dependencies
useEffect(() => {
fetchData(userId);
}, [userId]);
Security
# Bad: SQL injection risk
cursor.execute(f"SELECT * FROM users WHERE id = {user_id}")
# Good: Parameterized query
cursor.execute("SELECT * FROM users WHERE id = %s", [user_id])
References
---
name: code-review
description: Review pull requests following Sentry engineering practices, covering security, performance, testing, and design.
---
# Sentry Code Review
Follow these guidelines when reviewing code for Sentry projects.
## Review Checklist
### Identifying Problems
Look for these issues in code changes:
- **Runtime errors**: Potential exceptions, null pointer issues, out-of-bounds access
- **Performance**: Unbounded O(n²) operations, N+1 queries, unnecessary allocations
- **Side effects**: Unintended behavioral changes affecting other components
- **Backwards compatibility**: Breaking API changes without migration path
- **ORM queries**: Complex Django ORM with unexpected query performance
- **Security vulnerabilities**: Injection, XSS, access control gaps, secrets exposure
### Design Assessment
- Do component interactions make logical sense?
- Does the change align with existing project architecture?
- Are there conflicts with current requirements or goals?
### Test Coverage
Every PR should have appropriate test coverage:
- Functional tests for business logic
- Integration tests for component interactions
- End-to-end tests for critical user paths
Verify tests cover actual requirements and edge cases. Avoid excessive branching or looping in test code.
### Long-Term Impact
Flag for senior engineer review when changes involve:
- Database schema modifications
- API contract changes
- New framework or library adoption
- Performance-critical code paths
- Security-sensitive functionality
## Feedback Guidelines
### Tone
- Be polite and empathetic
- Provide actionable suggestions, not vague criticism
- Phrase as questions when uncertain: "Have you considered...?"
### Approval
- Approve when only minor issues remain
- Don't block PRs for stylistic preferences
- Remember: the goal is risk reduction, not perfect code
## Common Patterns to Flag
### Python/Django
```python
# Bad: N+1 query
for user in users:
print(user.profile.name) # Separate query per user
# Good: Prefetch related
users = User.objects.prefetch_related('profile')
```
### TypeScript/React
```typescript
// Bad: Missing dependency in useEffect
useEffect(() => {
fetchData(userId);
}, []); // userId not in deps
// Good: Include all dependencies
useEffect(() => {
fetchData(userId);
}, [userId]);
```
### Security
```python
# Bad: SQL injection risk
cursor.execute(f"SELECT * FROM users WHERE id = {user_id}")
# Good: Parameterized query
cursor.execute("SELECT * FROM users WHERE id = %s", [user_id])
```
## References
- [Sentry Code Review Guidelines](https://develop.sentry.dev/engineering-practices/code-review/)
