code-review

# 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/)