Code Review Guidelines - General

This document contains general code review rules applicable across all teams and tech stacks.

Part 1: Review Process Workflow

Part 2: Issue Severity Classification

Severity	Definition	Action	Examples
🔴 Critical	Security risk, data loss, production break	Block merge	SQL injection, exposed secrets, missing auth
🟠 Major	Violates architecture, significant bugs	Block merge	Missing base class inheritance, N+1 queries, no tests
🟡 Minor	Style violations, minor improvements	Request change, can merge after fix	Naming inconsistency, missing docstring
🔵 Suggestion	Nice-to-have, refactoring ideas	Comment only, don't block	Alternative approach, future improvement

Part 3: Approval Requirements

Change Type	Minimum Reviewers	Senior Approval Required
Bug fix (small)	1	No
Feature (new)	2	Yes
Database/Migration changes	2	Yes (mandatory)
Security-related	2	Yes (mandatory)
Core/Shared utilities	2	Yes (mandatory)
Hotfix (production)	1	Yes (post-merge review)

Part 4: PR Template

## Summary
<!-- What does this PR do and why? -->

## Type of Change
- [ ] Bug fix (non-breaking change fixing an issue)
- [ ] New feature (non-breaking change adding functionality)
- [ ] Breaking change (fix or feature causing existing functionality to change)
- [ ] Refactor (code change that neither fixes a bug nor adds a feature)
- [ ] Documentation update

## Related Ticket
<!-- Link to Jira/Linear ticket -->

## Testing Performed
<!-- Describe the tests you ran -->
- [ ] Manual testing completed
- [ ] Tested on local environment

## Screenshots (if UI changes)
<!-- Add screenshots here -->

## Breaking Changes
<!-- List any breaking changes and migration steps -->

## Developer Checklist
- [ ] Pre-commit hooks pass
- [ ] Self-review of code completed
- [ ] No debug code or print statements
- [ ] Module owner added as reviewer (if applicable)

Part 5: Review Comment Guidelines

Comment Format

Use prefixes to indicate severity:

[CRITICAL] - Must fix before merge
[MAJOR] - Should fix before merge
[MINOR] - Nice to fix, can merge after
[SUGGESTION] - Optional improvement
[QUESTION] - Seeking clarification
[PRAISE] - Positive feedback

Example Comments

Good:

[MAJOR] This query inside the loop will cause N+1 issues.
Consider using eager loading on line 45.

Bad:

Fix this.

Response SLAs

PR Size	Initial Review	Re-review
Small (<100 lines)	4 hours	2 hours
Medium (100-500 lines)	8 hours	4 hours
Large (>500 lines)	24 hours	8 hours

Part 6: Reviewer Assignment Rules

Weekly Rotating Review Group

A rotating group of 3-4 engineers is assigned weekly to handle PR reviews:

Rule	Description
Group Size	3-4 engineers per week
Rotation	Changes every Monday
Team Representation	At least one member from each team for fair representation
Notification Channel	`#eng-pr-reviews` Slack channel
Responsibility	Group ensures all assigned PRs are reviewed and merged

Weekly Rotation Schedule Example:

Week 1: @dev-A (Team Alpha), @dev-B (Team Beta), @dev-C (Team Gamma)
Week 2: @dev-D (Team Alpha), @dev-E (Team Beta), @dev-F (Team Gamma)
Week 3: @dev-G (Team Alpha), @dev-H (Team Beta), @dev-I (Team Gamma)
... (continues rotating)

Module Owner Requirement (Developer Responsibility)

Important

Adding the module owner as a reviewer is the developer's sole responsibility.

Developers MUST add the relevant module owner to their PR based on the areas of code changed.

If the developer fails to add the module owner:

The developer is solely responsible for any issues that arise
PRs may be reverted if module owner review was skipped
Repeated violations will be flagged in performance reviews

Review Assignment Checklist (for Developers)

Before requesting review, ensure:
- [ ] At least 1 reviewer from the weekly rotating group is added
- [ ] Module owner is added (if applicable to changed areas)
- [ ] Posted PR link in #eng-pr-reviews channel

General Rules

Author cannot approve: Developer cannot approve their own PR
Escalation: If no review within SLA, post reminder in #eng-pr-reviews
Blocking reviews: Module owner approval is required for merge (when applicable)
Load balancing: Rotating group helps distribute review load fairly

Part 7: Pre-Submission Checklist (All Teams)

#	Check	Verification
1	Pre-commit hooks pass	Must show all green
2	Tests added/updated	New features require tests; bug fixes require regression tests
3	Self-review completed	Read your own diff line-by-line before submission
4	No debug code	Remove `print()`, `console.log()`, `debugger`, commented code
5	Documentation updated	Docstrings/comments for new classes/complex methods

PR Title Format

[TYPE] Brief description where TYPE = FEAT, FIX, REFACTOR, DOCS, TEST, CHORE

Required PR Sections

Summary (what and why)
Type of change
Testing performed
Related ticket/issue link
Checklist confirmation
Dependent PRs if any
Dependent SQL Queries if any

General Code Review Guidelines