Code Review

Code Review Skill

This skill helps you perform thorough code reviews for quality, security, and performance.

When to Use This Skill

• Before creating pull requests
• Reviewing complex changes
• Checking for security vulnerabilities
• Identifying performance issues
• Ensuring code quality standards
• Mentoring junior developers

Code Review Checklist

1. Functionality

• Code does what it's supposed to do
• Edge cases are handled
• Error handling is appropriate
• No obvious bugs
• Logic is correct and efficient

2. Code Quality

• Code is readable and maintainable
• Functions are small and focused
• Variable names are descriptive
• No code duplication
• Follows DRY principle
• Comments explain "why", not "what"

3. Type Safety

• No use of any (or justified with comment)
• Proper TypeScript types
• No type assertions without reason
• Interfaces/types are well-defined
• Generics used appropriately

4. Testing

• New code has tests
• Tests cover edge cases
• Tests are meaningful
• No flaky tests
• Test names are descriptive

5. Performance

• No unnecessary re-renders (React)
• Database queries are optimized
• No N+1 query problems
• Caching used appropriately
• Bundle size impact considered

6. Security

• No SQL injection vulnerabilities
• No XSS vulnerabilities
• Input validation present
• Authentication/authorization checked
• Secrets not committed
• CORS configured properly

Common Issues to Look For

Anti-Patterns

❌ Magic Numbers

// Bad
if (user.age > 18) {
  allowAccess();
}

// Good
const LEGAL_AGE = 18;
if (user.age >= LEGAL_AGE) {
  allowAccess();
}

❌ Deep Nesting

// Bad
if (user) {
  if (user.isActive) {
    if (user.hasPermission) {
      if (resource.isAvailable) {
        // do something
      }
    }
  }
}

// Good
if (!user || !user.isActive || !user.hasPermission || !resource.isAvailable) {
  return;
}
// do something

❌ Large Functions

// Bad
function processUser() {
  // 200 lines of code
}

// Good
function processUser() {
  validateUser();
  enrichUserData();
  saveUser();
  notifyUser();
}

❌ Using any

// Bad
function processData(data: any) {
  return data.value;
}

// Good
interface Data {
  value: string;
}

function processData(data: Data) {
  return data.value;
}

Security Issues

❌ SQL Injection

// Bad
const query = `SELECT * FROM users WHERE id = ${userId}`;

// Good
const query = db.query.users.findFirst({
  where: eq(users.id, userId),
});

❌ XSS Vulnerability

// Bad
element.innerHTML = userInput;

// Good
element.textContent = userInput;
// Or use framework's safe rendering
<div>{userInput}</div>

❌ Exposed Secrets

// Bad
const apiKey = "sk_live_12345...";

// Good
const apiKey = process.env.API_KEY;

❌ Missing Input Validation

// Bad
export async function updateUser(data: any) {
  await db.update(users).set(data);
}

// Good
const updateUserSchema = z.object({
  name: z.string().min(1).max(100),
  email: z.string().email(),
});

export async function updateUser(data: unknown) {
  const validated = updateUserSchema.parse(data);
  await db.update(users).set(validated);
}

Performance Issues

❌ N+1 Queries

// Bad
const posts = await db.query.posts.findMany();
for (const post of posts) {
  const author = await db.query.users.findFirst({
    where: eq(users.id, post.authorId),
  });
  post.author = author;
}

// Good
const posts = await db.query.posts.findMany({
  with: {
    author: true,
  },
});

❌ Missing Memoization

// Bad
function Component({ data }) {
  const processedData = expensiveOperation(data);
  return <div>{processedData}</div>;
}

// Good
function Component({ data }) {
  const processedData = useMemo(() => expensiveOperation(data), [data]);
  return <div>{processedData}</div>;
}

❌ Unnecessary Re-renders

// Bad
function Parent() {
  return <Child => handleClick()} />;
}

// Good
function Parent() {
  const handleClick = useCallback(() => {
    // handle click
  }, []);

  return <Child />;
}

Review Process

1. Understand the Change

# View the diff
git diff main...feature-branch

# See changed files
git diff --name-only main...feature-branch

# See commit history
git log main..feature-branch --oneline

2. Check Code Quality

# Run linter
pnpm biome check .

# Run type checker
pnpm tsc --noEmit

# Run tests
pnpm test

3. Check for Common Issues

# Search for 'any' usage
grep -r "any" apps/ packages/ --include="*.ts" --include="*.tsx"

# Search for console.log
grep -r "console.log" apps/ packages/ --include="*.ts" --include="*.tsx"

# Search for TODO comments
grep -r "TODO" apps/ packages/ --include="*.ts" --include="*.tsx"

# Search for hardcoded credentials
grep -r "password\|secret\|key" apps/ packages/ --include="*.ts"

4. Review Database Changes

# Check migration files
ls -la packages/database/migrations/

# Review migration SQL
cat packages/database/migrations/latest.sql

5. Check Dependencies

# Check for new dependencies
git diff package.json

# Check for security vulnerabilities
pnpm audit

# Check for outdated packages
pnpm outdated

Automated Review Tools

TypeScript Compiler

# Check types across workspace
pnpm tsc --noEmit

# Check specific package
pnpm -F @sgcarstrends/web tsc --noEmit

Biome Linter

# Lint all files
pnpm biome check .

# Lint with details
pnpm biome check . --verbose

# Check specific files
pnpm biome check apps/web/src/**/*.ts

Tests

# Run all tests
pnpm test

# Run with coverage
pnpm test:coverage

# Check coverage threshold
pnpm test -- --coverage --coverageThreshold='{"global":{"branches":80,"functions":80,"lines":80}}'

Review Comments

Constructive Feedback

❌ Poor Comment

This is wrong.

✅ Good Comment

Consider using `useMemo` here to avoid recalculating on every render.
This could impact performance with large datasets.

❌ Poor Comment

Don't use any.

✅ Good Comment

The `any` type bypasses type safety. Consider defining a proper interface:

interface UserData {
  id: string;
  name: string;
  email: string;
}

Comment Categories

🔴 Must Fix - Critical issues that block merge

🔴 This creates an SQL injection vulnerability. Use parameterized queries.

🟡 Should Fix - Important but not blocking

🟡 This function is doing too much. Consider breaking it into smaller functions.

🟢 Suggestion - Nice to have improvements

🟢 You could simplify this with optional chaining: `user?.profile?.name`

💡 Learning - Educational comments

💡 FYI: Next.js automatically optimizes images when using the Image component.

❓ Question - Asking for clarification

❓ Is there a reason we're not using the existing `formatDate` utility?

Review Patterns by Language/Framework

TypeScript/JavaScript

Check for:

• Proper typing (no any)
• Async/await usage
• Error handling
• Modern syntax (optional chaining, nullish coalescing)

// Look for improvements
const value = data && data.user && data.user.name;  // ❌
const value = data?.user?.name;  // ✅

const value = data || 'default';  // ❌ (0, '', false are falsy)
const value = data ?? 'default';  // ✅ (only null/undefined)

React

Check for:

• Proper hooks usage
• Memoization where needed
• Key props in lists
• useEffect dependencies

// Check useEffect dependencies
useEffect(() => {
  fetchData(userId);  // ❌ Missing dependency
}, []);

useEffect(() => {
  fetchData(userId);  // ✅ Correct dependencies
}, [userId]);

Next.js

Check for:

• Server vs client components
• Proper use of async components
• Metadata exports
• Image optimization

// ❌ Missing 'use client'
import { useState } from 'react';

export default function Component() {
  const [state, setState] = useState();
  // ...
}

// ✅ Correct
'use client';
import { useState } from 'react';

export default function Component() {
  const [state, setState] = useState();
  // ...
}

Database (Drizzle)

Check for:

• Proper indexing
• N+1 queries
• Transaction usage
• Migration safety

// ❌ N+1 query
const users = await db.query.users.findMany();
for (const user of users) {
  user.posts = await db.query.posts.findMany({
    where: eq(posts.userId, user.id),
  });
}

// ✅ Single query with join
const users = await db.query.users.findMany({
  with: {
    posts: true,
  },
});

Pull Request Review

Before Approving

• All tests pass
• No linting errors
• Type checks pass
• No security vulnerabilities
• Performance is acceptable
• Documentation updated if needed
• Breaking changes documented
• Commit messages follow convention

Review Tiers

Level 1: Quick Review (< 100 lines)

• Run automated checks
• Scan for obvious issues
• Check tests exist

Level 2: Standard Review (100-500 lines)

• Detailed code review
• Test coverage check
• Performance consideration
• Security check

Level 3: Thorough Review (> 500 lines)

• Architecture review
• Multiple reviewers
• Performance testing
• Security audit
• Documentation review

Self-Review Checklist

Before requesting review:

# 1. Check your changes
git diff main...HEAD

# 2. Run quality checks
pnpm biome check --write .
pnpm tsc --noEmit
pnpm test

# 3. Check for common issues
grep -r "console.log" . --include="*.ts" --include="*.tsx"
grep -r "TODO" . --include="*.ts" --include="*.tsx"

# 4. Review your commits
git log main..HEAD --oneline

# 5. Check PR size
git diff --stat main...HEAD

Code Review Metrics

Track these metrics:

• Review time: Time from PR creation to approval
• Iterations: Number of review rounds
• Comments: Number of review comments
• Defects found: Bugs caught in review
• Coverage delta: Test coverage change

References

• TypeScript Best Practices: https://typescript-eslint.io/rules/
• React Best Practices: https://react.dev/learn
• Security: OWASP Top 10
• Related files:
  • Root CLAUDE.md - Code style guidelines
  • biome.json - Linting rules

Best Practices

1. Be Constructive: Focus on improvement, not criticism
2. Explain Why: Don't just say "wrong", explain the issue
3. Suggest Solutions: Provide alternatives when possible
4. Prioritize: Mark critical issues vs suggestions
5. Be Timely: Review PRs promptly
6. Ask Questions: Seek to understand before judging
7. Praise Good Code: Acknowledge well-written code
8. Stay Professional: Keep feedback objective and respectful