Code Review Standards

Code Review Standards

When to Use

• Reviewing pull requests
• Performing code reviews
• Defining review criteria
• Establishing review process

Overview

Code review standards ensure consistent, thorough reviews that catch bugs before they reach production. This skill aggregates criteria from specialized skills.

Review Framework

4-Level Severity Classification

1. CRITICAL 🔴 - Must fix before merge

• Security vulnerabilities
• Data loss risks
• Authentication bypasses
• SQL injection risks

2. HIGH 🟠 - Should fix before merge

• TypeScript strict mode violations
• Missing error handling
• Performance issues (N+1 queries)
• Missing input validation

3. MEDIUM 🟡 - Fix soon (can merge with plan)

• Code quality issues
• Missing tests
• Poor naming
• Missing documentation

4. LOW 🟢 - Nice to have

• Style suggestions
• Optimization opportunities
• Refactoring ideas

---

Review Checklist

1. Correctness

→ See: correctness-criteria.md

• [ ] Logic is correct for all test cases
• [ ] Edge cases handled (null, empty, max, min)
• [ ] Error conditions properly handled
• [ ] Return types match function signatures
• [ ] Async operations properly awaited
• [ ] No race conditions
• [ ] No off-by-one errors

---

2. Security

→ See: security-sentinel skill → See: security-checklist.md

CRITICAL - Must check every review:

• [ ] No hardcoded secrets
• [ ] Input validation with Zod (ALL inputs)
• [ ] Authentication checked on protected routes
• [ ] Authorization enforced (resource ownership)
• [ ] SQL injection prevented (using Drizzle)
• [ ] XSS prevented (no dangerouslySetInnerHTML without sanitization)
• [ ] CSRF protection on state-changing operations
• [ ] No sensitive data in logs
• [ ] Passwords hashed (bcrypt, 12+ rounds)
• [ ] JWTs properly verified

For complete security criteria: → security-sentinel/SKILL.md

---

3. TypeScript Quality

→ See: typescript-strict-guard skill

• [ ] No any types
• [ ] No @ts-ignore without extensive comment
• [ ] No ! non-null assertions without comment
• [ ] Explicit types on all function parameters
• [ ] Explicit return types on all functions
• [ ] Type guards used for unknown types
• [ ] Proper use of generics
• [ ] No implicit any

---

4. Testing

→ See: quality-gates/test-patterns.md

• [ ] Tests exist for new code
• [ ] Tests follow AAA pattern
• [ ] Coverage meets thresholds (75%/90%)
• [ ] UI tests verify DOM state (not just mocks)
• [ ] E2E tests for visual changes
• [ ] No skipped tests without reason
• [ ] Tests are independent
• [ ] Tests clean up after themselves

---

5. Performance

→ See: performance-criteria.md

• [ ] No N+1 query problems
• [ ] Database queries optimized
• [ ] Async operations parallelized where possible
• [ ] Large datasets paginated
• [ ] Images optimized
• [ ] No unnecessary re-renders
• [ ] Expensive calculations memoized

---

6. Code Quality

→ See: maintainability-rules.md

• [ ] No console.log in production code
• [ ] No commented-out code
• [ ] No TODO without GitHub issue
• [ ] Functions have single responsibility
• [ ] Variable names are descriptive
• [ ] No dead code
• [ ] No duplicated logic
• [ ] Proper error messages

---

7. Architecture Compliance

→ See: architecture-patterns skill

• [ ] Correct pattern chosen for problem
• [ ] Pattern implemented correctly
• [ ] No pattern violations
• [ ] Follows Next.js best practices
• [ ] Server vs Client Components correct
• [ ] State management appropriate

---

Review Process

Step 1: Pre-Review (2 minutes)

1. Read PR description 2. Understand what changed and why 3. Check CI/CD status (tests, build, coverage) 4. Identify high-risk areas (auth, payments, data handling)

Step 2: Security Review (5 minutes)

For ALL PRs:

• Check for hardcoded secrets
• Verify input validation
• Check authentication/authorization

For Auth/API/Data PRs:

• Run security-sentinel skill
• Review OWASP Top 10 criteria
• Check for injection risks

→ security-checklist.md

Step 3: Code Review (10-20 minutes)

1. Correctness: Does it work as intended? 2. TypeScript: Strict mode compliance? 3. Testing: Adequate coverage and quality? 4. Performance: Any obvious issues? 5. Quality: Readable, maintainable code? 6. Architecture: Follows established patterns?

Step 4: Write Feedback (5 minutes)

Use severity levels and templates: → review-templates.md

Format:

## 🔴 CRITICAL Issues

- [ ] [Security] Hardcoded API key in auth.ts:45
  - **Risk**: API key exposed in version control
  - **Fix**: Move to environment variable
  - **File**: src/lib/auth.ts:45

## 🟠 HIGH Issues

- [ ] [TypeScript] Using `any` type in processData()
  - **Issue**: No type safety
  - **Fix**: Define explicit interface
  - **File**: src/utils/process.ts:12

## 🟡 MEDIUM Issues

- [ ] [Testing] Missing tests for error cases
  - **Coverage**: Only happy path tested
  - **Needed**: Test null input, invalid format
  - **File**: tests/unit/process.test.ts

## 🟢 LOW Issues / Suggestions

- Consider extracting helper function for readability

Step 5: Verdict

Choose one:

• ✅ APPROVE - No critical/high issues
• 🔄 REQUEST CHANGES - Critical or multiple high issues
• 💬 COMMENT - Questions or low/medium issues only

---

Review Templates

Security Issue Template

🔴 **[Security] [Vulnerability Type]**

**Location**: `src/path/file.ts:123`

**Issue**: [Description of vulnerability]

**Risk**: [What could go wrong]

**Fix**:

// Suggested fix

**Reference**: [OWASP link or skill reference]

TypeScript Issue Template

🟠 **[TypeScript] [Issue Type]**

**Location**: `src/path/file.ts:45`

**Issue**: [What's wrong]

**Fix**:

// Current (bad) function process(data: any) { }

// Suggested (good) function process(data: ProcessData): ProcessedResult { }

**Reference**: typescript-strict-guard skill

Performance Issue Template

🟡 **[Performance] [Issue Type]**

**Location**: `src/path/file.ts:78`

**Issue**: N+1 query problem in getUserProjects()

**Impact**: Linear time complexity, slow for large datasets

**Fix**:

// Use join instead of separate queries const projects = await db .select() .from(projectsTable) .leftJoin(usersTable, eq(projectsTable.userId, usersTable.id))

---

Common Review Patterns

Code Smells

Long Functions

// 🔴 BAD: 100+ line function
function processEverything() {
  // ... 100 lines
}

// ✅ GOOD: Extracted helpers
function processEverything() {
  const validated = validateInput()
  const processed = processData(validated)
  const saved = saveToDatabase(processed)
  return saved
}

Deeply Nested Logic

// 🔴 BAD: 4+ levels of nesting
if (user) {
  if (user.projects) {
    if (user.projects.length > 0) {
      if (user.projects[0].status === 'active') {
        // ...
      }
    }
  }
}

// ✅ GOOD: Early returns
if (!user) return
if (!user.projects || user.projects.length === 0) return
if (user.projects[0].status !== 'active') return
// ...

Magic Numbers

// 🔴 BAD: Unexplained numbers
setTimeout(callback, 3600000)

// ✅ GOOD: Named constants
const ONE_HOUR_MS = 60 * 60 * 1000
setTimeout(callback, ONE_HOUR_MS)

---

Progressive Disclosure

1. SKILL.md (this file) - Review framework overview 2. security-checklist.md - OWASP Top 10 checklist 3. performance-criteria.md - Performance review criteria 4. maintainability-rules.md - Code quality rules 5. review-templates.md - Feedback templates

---

Integration with Other Skills

Code review aggregates criteria from:

• security-sentinel - Security vulnerability checks
• typescript-strict-guard - Type safety validation
• quality-gates - Quality checkpoint framework
• architecture-patterns - Pattern compliance
• nextjs-15-specialist - Next.js best practices

---

Example Review

# Code Review: Add User Authentication

## Summary
Adds JWT-based authentication with login/logout endpoints.

## 🔴 CRITICAL Issues

### 1. Hardcoded JWT Secret
**File**: `src/lib/auth.ts:12`
**Issue**: JWT secret is hardcoded as "secret123"
**Risk**: Anyone can forge JWTs
**Fix**:

• const secret = "secret123"

+ const secret = process.env.JWT_SECRET + if (!secret) throw new Error('JWT_SECRET not set')

## 🟠 HIGH Issues

### 2. Missing Input Validation
**File**: `src/app/api/auth/login/route.ts:15`
**Issue**: User input not validated before use
**Fix**: Add Zod schema validation

const loginSchema = z.object({ email: z.string().email(), password: z.string().min(8), })

const validated = loginSchema.parse(body)

## 🟡 MEDIUM Issues

### 3. Missing Tests
**File**: `tests/integration/auth.test.ts`
**Issue**: No tests for error cases
**Needed**:
- Test invalid email format
- Test wrong password
- Test expired JWT

## Verdict

🔄 **REQUEST CHANGES** - Fix critical and high issues before merge.

Once fixed, this will be a solid authentication implementation.

---

See Also

• security-checklist.md - OWASP Top 10 checklist
• performance-criteria.md - Performance review guide
• maintainability-rules.md - Code quality rules
• review-templates.md - Feedback templates
• ../security-sentinel/SKILL.md - Security patterns
• ../quality-gates/SKILL.md - Quality framework

Maintainability Rules

Code Quality Checklist

Function Complexity

Single Responsibility Principle

// 🔴 BAD: Function does too much
function processUser(userData) {
  // Validate input
  if (!userData.email) throw new Error('Invalid')

  // Hash password
  const hashed = bcrypt.hash(userData.password)

  // Save to database
  await db.insert(usersTable).values({ ...userData, password: hashed })

  // Send welcome email
  await sendEmail(userData.email, 'Welcome!')

  // Log analytics
  analytics.track('user_created')
}

// ✅ GOOD: Each function has one responsibility
async function validateUser(userData: UserInput): UserData {
  return userSchema.parse(userData)
}

async function hashPassword(password: string): Promise<string> {
  return bcrypt.hash(password, 12)
}

async function createUser(data: UserData): Promise<User> {
  return db.insert(usersTable).values(data).returning()
}

async function sendWelcomeEmail(email: string): Promise<void> {
  await sendEmail(email, 'Welcome!')
}

function trackUserCreation(userId: string): void {
  analytics.track('user_created', { userId })
}

Function Length

• ✅ Target: < 20 lines
• ⚠️ Warning: 20-50 lines
• 🔴 Error: > 50 lines

---

Naming Conventions

Variables

// 🔴 BAD: Unclear names
const d = new Date()
const arr = getData()
const x = calculateThing()

// ✅ GOOD: Descriptive names
const createdAt = new Date()
const activeProjects = getActiveProjects()
const totalRevenue = calculateMonthlyRevenue()

Functions

// 🔴 BAD: Unclear what function does
function process() { }
function handle() { }
function doIt() { }

// ✅ GOOD: Verb + noun, describes action
function validateUserInput() { }
function calculateTotalPrice() { }
function fetchActiveProjects() { }

Booleans

// 🔴 BAD: Unclear boolean meaning
const flag = true
const status = false

// ✅ GOOD: is/has/can prefix
const isAuthenticated = true
const hasPermission = false
const canEdit = true

---

Comments

When to Comment

// ✅ GOOD: Complex business logic
// Calculate compound interest using formula: A = P(1 + r/n)^(nt)
// where P = principal, r = rate, n = compounds per year, t = years
function calculateCompoundInterest(principal, rate, years) {
  const n = 12 // Monthly compounding
  return principal * Math.pow(1 + rate / n, n * years)
}

// ✅ GOOD: Explaining "why" not "what"
// Using SHA-256 instead of MD5 because MD5 is cryptographically broken
const hash = crypto.createHash('sha256')

// 🔴 BAD: Stating the obvious
// Increment counter by 1
counter++

// 🔴 BAD: Commented-out code
// const oldWay = processData(input)
// return oldWay.filter(x => x > 0)

---

Error Handling

Informative Error Messages

// 🔴 BAD: Generic error
throw new Error('Error')
throw new Error('Invalid input')

// ✅ GOOD: Specific, actionable error
throw new Error('Email must be a valid email address')
throw new Error(`Project ${projectId} not found`)
throw new Error('Password must be at least 8 characters and contain uppercase, lowercase, number, and special character')

Error Context

// 🔴 BAD: No context
catch (error) {
  throw error
}

// ✅ GOOD: Add context
catch (error) {
  console.error('Failed to create project:', {
    userId,
    projectName,
    error: error.message,
  })
  throw new Error(`Failed to create project: ${error.message}`)
}

---

Code Duplication

DRY Principle (Don't Repeat Yourself)

// 🔴 BAD: Duplicated validation
function createProject(data) {
  if (!data.name) throw new Error('Name required')
  if (data.name.length > 100) throw new Error('Name too long')
  // ...
}

function updateProject(data) {
  if (!data.name) throw new Error('Name required')
  if (data.name.length > 100) throw new Error('Name too long')
  // ...
}

// ✅ GOOD: Extract common validation
const projectSchema = z.object({
  name: z.string().min(1).max(100),
})

function createProject(data) {
  const validated = projectSchema.parse(data)
  // ...
}

function updateProject(data) {
  const validated = projectSchema.parse(data)
  // ...
}

---

Magic Values

Constants for Magic Numbers/Strings

// 🔴 BAD: Magic numbers
if (user.age < 18) { }
setTimeout(callback, 86400000)
if (status === 'pending_approval') { }

// ✅ GOOD: Named constants
const MINIMUM_AGE = 18
const ONE_DAY_MS = 24 * 60 * 60 * 1000
const ProjectStatus = {
  PENDING_APPROVAL: 'pending_approval',
  APPROVED: 'approved',
  REJECTED: 'rejected',
} as const

if (user.age < MINIMUM_AGE) { }
setTimeout(callback, ONE_DAY_MS)
if (status === ProjectStatus.PENDING_APPROVAL) { }

---

Dead Code

Remove Unused Code

// 🔴 BAD: Unused imports, variables, functions
import { unusedFunction } from './utils'

const unusedVariable = 'test'

function neverCalled() {
  return 'unused'
}

// ✅ GOOD: Only keep what's used
import { actuallyUsedFunction } from './utils'

function actuallyUsed() {
  return actuallyUsedFunction()
}

---

File Organization

File Length

• ✅ Target: < 200 lines
• ⚠️ Warning: 200-400 lines
• 🔴 Error: > 400 lines

Module Cohesion

// 🔴 BAD: Everything in one file
// utils.ts (1000 lines)
function dateUtils() { }
function stringUtils() { }
function arrayUtils() { }
function objectUtils() { }

// ✅ GOOD: Separate by concern
// utils/date.ts
export function formatDate() { }
export function parseDate() { }

// utils/string.ts
export function capitalize() { }
export function slugify() { }

// utils/array.ts
export function unique() { }
export function groupBy() { }

---

TODOs and FIXMEs

Link to Issues

// 🔴 BAD: TODO without context
// TODO: Optimize this

// ✅ GOOD: TODO with issue reference
// TODO(#123): Optimize with caching - see issue for benchmarks

---

Review Template

### 🟡 [Code Quality] [Issue Type]

**Location**: `src/path/file.ts:line`

**Issue**: [Description of quality issue]

**Impact**:
- Readability: [How it affects understanding]
- Maintainability: [How it affects future changes]

**Suggestion**:

// Improved implementation

**Priority**: LOW | MEDIUM | HIGH

---

Maintainability Metrics

Cyclomatic Complexity

• ✅ Simple: 1-5
• ⚠️ Moderate: 6-10
• 🔴 Complex: > 10

Function Parameters

• ✅ Good: 0-3 parameters
• ⚠️ Warning: 4-5 parameters
• 🔴 Too many: > 5 parameters (use object)

Nesting Depth

• ✅ Good: 1-2 levels
• ⚠️ Warning: 3 levels
• 🔴 Too deep: > 3 levels (refactor)

---

See Also

• Clean Code
• Refactoring
• ../typescript-strict-guard/SKILL.md

Performance Criteria

Performance Review Checklist

Database Queries

N+1 Query Problem

// 🔴 BAD: N+1 queries (1 + N)
const users = await db.select().from(usersTable)
for (const user of users) {
  const projects = await db
    .select()
    .from(projectsTable)
    .where(eq(projectsTable.userId, user.id)) // N queries!
}

// ✅ GOOD: Single query with join
const usersWithProjects = await db
  .select()
  .from(usersTable)
  .leftJoin(projectsTable, eq(usersTable.id, projectsTable.userId))

Missing Indexes

// Check for slow queries on:
- WHERE clauses without index
- Foreign keys without index
- Sorting columns without index

Unnecessary Data Fetching

// 🔴 BAD: Fetching all columns
const users = await db.select().from(usersTable)

// ✅ GOOD: Select only needed columns
const users = await db
  .select({ id: usersTable.id, name: usersTable.name })
  .from(usersTable)

---

Async Operations

Sequential vs Parallel

// 🔴 BAD: Sequential (slow)
const projects = await fetchProjects()
const users = await fetchUsers()
const tasks = await fetchTasks()

// ✅ GOOD: Parallel (fast)
const [projects, users, tasks] = await Promise.all([
  fetchProjects(),
  fetchUsers(),
  fetchTasks(),
])

Unnecessary Awaits

// 🔴 BAD: Unnecessary await
async function process() {
  await doSomething()
  await doSomethingElse()
  return 'done'
}

// ✅ GOOD: Only await when needed
async function process() {
  doSomething() // Fire and forget
  await doSomethingElse() // Must wait
  return 'done'
}

---

React Performance

Unnecessary Re-renders

// 🔴 BAD: Object created on every render
function Component() {
  const style = { color: 'red' } // New object each render!
  return <div style={style}>Text</div>
}

// ✅ GOOD: Memoize or move outside
const style = { color: 'red' }

function Component() {
  return <div style={style}>Text</div>
}

Expensive Calculations

// 🔴 BAD: Expensive calculation on every render
function Component({ items }) {
  const sorted = items.sort((a, b) => a.value - b.value) // Sorts every render!
  return <List items={sorted} />
}

// ✅ GOOD: Memoize expensive calculation
import { useMemo } from 'react'

function Component({ items }) {
  const sorted = useMemo(
    () => items.sort((a, b) => a.value - b.value),
    [items]
  )
  return <List items={sorted} />
}

Large Lists Without Virtualization

// 🔴 BAD: Rendering 10,000 items
function Component({ items }) {
  return (
    <div>
      {items.map(item => <ItemCard key={item.id} item={item} />)}
    </div>
  )
}

// ✅ GOOD: Use virtualization for large lists
import { Virtualizer } from '@tanstack/react-virtual'

function Component({ items }) {
  // Only render visible items
}

---

Images and Assets

Unoptimized Images

// 🔴 BAD: Large unoptimized image
<img src="/large-image.png" width={100} height={100} />

// ✅ GOOD: Next.js Image component
import Image from 'next/image'

<Image
  src="/large-image.png"
  width={100}
  height={100}
  alt="Description"
/>

Missing Lazy Loading

// 🔴 BAD: All images load immediately
<img src="/image.png" alt="..." />

// ✅ GOOD: Lazy load below-fold images
<img src="/image.png" alt="..." loading="lazy" />

---

Pagination

Loading All Data

// 🔴 BAD: Loading 100,000 records
const projects = await db.select().from(projectsTable)

// ✅ GOOD: Paginate
const projects = await db
  .select()
  .from(projectsTable)
  .limit(20)
  .offset((page - 1) * 20)

---

Caching

No Caching Strategy

// 🔴 BAD: Fetching same data repeatedly
async function getUser(id: string) {
  return db.select().from(usersTable).where(eq(usersTable.id, id))
}

// ✅ GOOD: Cache with React Query
const { data: user } = useQuery({
  queryKey: ['user', id],
  queryFn: () => getUser(id),
  staleTime: 5 * 60 * 1000, // 5 minutes
})

---

Performance Metrics

Target Metrics

• First Contentful Paint (FCP): < 1.8s
• Largest Contentful Paint (LCP): < 2.5s
• Time to Interactive (TTI): < 3.8s
• Total Blocking Time (TBT): < 200ms
• Cumulative Layout Shift (CLS): < 0.1

Database Query Times

• Simple SELECT: < 10ms
• JOIN queries: < 50ms
• Complex queries: < 100ms

API Response Times

• GET endpoint: < 200ms
• POST endpoint: < 500ms
• Complex operations: < 1s

---

Review Template

### 🟡 [Performance] [Issue Type]

**Location**: `src/path/file.ts:line`

**Issue**: [Description of performance problem]

**Impact**:
- Current: [O(n²), 500ms, etc.]
- Expected: [O(n), 50ms, etc.]

**Fix**:

// Optimized implementation

**Metrics**:
- Before: [measurement]
- After: [measurement]

---

See Also

• Next.js Performance
• React Performance
• Database Indexing

Security Checklist

For complete security patterns, see: security-sentinel skill

OWASP Top 10 Quick Checklist

1. Injection Attacks

• [ ] No SQL string concatenation (use Drizzle ORM)
• [ ] No command injection (no exec() with user input)
• [ ] No NoSQL injection (validate with Zod)

2. Broken Authentication

• [ ] Passwords hashed with bcrypt (12+ rounds)
• [ ] JWTs use strong secret from environment
• [ ] Session tokens cryptographically secure
• [ ] Cookies have HttpOnly, Secure, SameSite flags

3. Sensitive Data Exposure

• [ ] No hardcoded secrets (use environment variables)
• [ ] Sensitive data not logged (passwords, credit cards)
• [ ] Passwords excluded from API responses
• [ ] HTTPS enforced in production

4. XML External Entities (XXE)

• [ ] External entities disabled in XML parsers

5. Broken Access Control

• [ ] Authentication checked on protected routes
• [ ] Authorization enforced (resource ownership)
• [ ] No IDOR vulnerabilities (insecure direct object reference)

6. Security Misconfiguration

• [ ] CORS allows specific origins only
• [ ] Error messages don't leak internal details
• [ ] Security headers set (CSP, X-Frame-Options, etc.)
• [ ] Dependencies up to date (npm audit)

7. Cross-Site Scripting (XSS)

• [ ] No dangerouslySetInnerHTML without DOMPurify
• [ ] URLs validated before use
• [ ] User input escaped in templates

8. Insecure Deserialization

• [ ] No eval() or Function() with user input
• [ ] JSON parsed and validated with Zod

9. Using Components with Known Vulnerabilities

• [ ] Dependencies audited (npm audit)
• [ ] No critical vulnerabilities
• [ ] Dependencies regularly updated

10. Insufficient Logging & Monitoring

• [ ] Security events logged (login, logout, failed auth)
• [ ] Errors logged with context
• [ ] No sensitive data in logs

---

Critical Review Points

Authentication Code

// Check for:
- [ ] Password hashing (bcrypt, 12+ rounds)
- [ ] JWT secret from environment
- [ ] Session token generation (crypto.randomBytes)
- [ ] Secure cookie flags

API Routes

// Check for:
- [ ] Input validation with Zod
- [ ] Authentication check
- [ ] Authorization check (ownership)
- [ ] Error handling
- [ ] No sensitive data in responses

Database Queries

// Check for:
- [ ] Using Drizzle ORM (not raw SQL)
- [ ] Parameterized queries
- [ ] No string concatenation
- [ ] Transactions for multi-step operations

---

Security Review Template

## 🔴 Security Issues

### [Vulnerability Type]
**OWASP**: [Which of Top 10]
**Location**: `src/path/file.ts:line`
**Risk Level**: CRITICAL | HIGH | MEDIUM | LOW

**Issue**: [Description]

**Attack Scenario**:
1. [How an attacker could exploit this]
2. [What data/access they could gain]

**Fix**:

// Suggested secure implementation

**Reference**: [security-sentinel skill link or OWASP article]

---

See Also

• security-sentinel/SKILL.md - Complete security patterns
• ../quality-gates/validation-rules.md - Validation rules
• OWASP Top 10