Code Review Process

Overview

This document establishes the standard code review workflow for all development work. The core principle is: commit first, evaluate before pushing. This ensures all code is reviewed for quality, security, and best practices before entering the shared codebase.

Review Options

We support two code review approaches:

1. Manual Review (Interactive) - For direct-to-main pushes - Ask Claude Code to review your local commits - Immediate feedback in your development session - Best for: Quick fixes, small changes, solo development

2. Automated Review (PR Workflow) - For feature branches - Create pull request → automated review runs - GitHub Actions + Claude API - Best for: Large features, multi-file changes, significant refactors

See Automated PR Review for the automated workflow.

---

The Review Workflow (Manual)

1. Complete Implementation

Implement features following standard development practices: - Follow conventions in /root/tower-fleet/docs/reference/app-conventions.md - Use proper Git workflow from /root/GIT_PATTERNS.md - Adhere to styling guide in /root/tower-fleet/docs/reference/styling-guide.md

2. Commit Changes Locally

git add -A
git commit -m "feat: implement feature"

DO NOT push yet. Commits should be local and ready for review.

3. Request Code Review

Ask Claude to evaluate recent changes:

"Evaluate the code - assess recent changes/commits"

Claude will review: - Recent commits (last 5-10) - Changed files - Type safety and linting - Security considerations - React/framework best practices - Code quality and patterns

4. Review Assessment

Claude provides a structured assessment including:

Critical Issues (P0) - Must fix before pushing

• Type safety violations
• Security vulnerabilities
• Framework anti-patterns (e.g., React Rules violations)
• Data integrity risks

High Priority (P1) - Should fix soon

• Error handling gaps
• Missing validation
• Performance concerns
• Resource leaks

Medium Priority (P2) - Nice to have

• Code quality improvements
• Missing tests
• Documentation gaps
• Cleanup tasks

5. Fix Issues

Address issues in priority order:

P0 (Critical) - Fix immediately:

# Fix issues
git add -A
git commit -m "fix: address critical review findings"

P1/P2 - Fix or document: - Fix now if quick - Create TODO items for later - Document known issues if intentional

6. Re-review (if needed)

For significant changes, request another review:

"Re-evaluate the fixes"

7. Push to Remote

Only after review and critical fixes:

git push

---

Review Checklist

Claude evaluates code against this checklist:

Type Safety

• [ ] No TypeScript errors (npx tsc --noEmit)
• [ ] No any types without justification
• [ ] Proper type imports and interfaces
• [ ] Nullable types handled correctly

Linting & Code Quality

• [ ] No ESLint errors (npm run lint)
• [ ] No unused variables or imports
• [ ] Consistent code style
• [ ] No deprecated API usage

React Best Practices

• [ ] No components created during render
• [ ] No synchronous setState in useEffect
• [ ] Proper hook dependencies
• [ ] No direct DOM manipulation (unless necessary)
• [ ] Keys on list items

Security

• [ ] Input validation on all user data
• [ ] XSS prevention (sanitize rendered content)
• [ ] SQL injection prevention (parameterized queries)
• [ ] Authentication checks on API routes
• [ ] RLS policies properly configured
• [ ] File upload size limits
• [ ] CORS configuration reviewed

Data Integrity

• [ ] Database migrations for schema changes
• [ ] Foreign key constraints where appropriate
• [ ] Cascade delete behavior considered
• [ ] Transaction handling for multi-step operations
• [ ] Proper error rollback

Error Handling

• [ ] Try-catch blocks where needed
• [ ] User-friendly error messages
• [ ] Backend errors logged
• [ ] Partial failure scenarios handled
• [ ] Loading and error states in UI

Performance

• [ ] No N+1 query patterns
• [ ] Expensive operations memoized
• [ ] Large lists virtualized if needed
• [ ] Images optimized
• [ ] Bundle size reasonable

Testing

• [ ] Type checking passes
• [ ] Lint passes
• [ ] Manual testing completed
• [ ] Edge cases considered
• [ ] Data persistence verified

---

Common Issues Found in Reviews

Type Safety Violations

Problem:

const coords = feature.geometry.coordinates  // ❌ Type error

Fix:

const coords = (feature.geometry as Point).coordinates  // ✅
// Or better: runtime check
if (feature.geometry.type === 'Point') {
  const coords = feature.geometry.coordinates
}

React Component Creation

Problem:

export function Component({ type }) {
  const Icon = getIcon(type)  // ❌ Created every render
  return <Icon />
}

Fix:

export function Component({ type }) {
  const Icon = useMemo(() => getIcon(type), [type])  // ✅
  return <Icon />
}

Missing Input Validation

Problem:

const file = formData.get("file") as File
// No size check ❌
const { data } = await supabase.storage.upload(...)

Fix:

const file = formData.get("file") as File
if (!file) return Response.json({ error: "No file" }, { status: 400 })

const maxSize = 5 * 1024 * 1024  // 5MB
if (file.size > maxSize) {
  return Response.json({ error: "File too large" }, { status: 400 })
}

XSS Vulnerabilities

Problem:

<p>{gpxData.metadata.name}</p>  // ❌ Unsanitized user input

Fix:

// For plain text, React escapes by default - OK
<p>{gpxData.metadata.name}</p>  // ✅ React auto-escapes

// For HTML content, sanitize first
import DOMPurify from 'dompurify'
<div dangerouslySetInnerHTML={{
  __html: DOMPurify.sanitize(htmlContent)
}} />

Sequential Operations (Performance)

Problem:

for (const item of items) {
  await fetch("/api/create", { body: JSON.stringify(item) })  // ❌ Sequential
}

Fix:

await Promise.all(
  items.map(item =>
    fetch("/api/create", { body: JSON.stringify(item) })
  )
)  // ✅ Parallel

Missing Cleanup

Problem:

// Upload file to storage
await supabase.storage.from('files').upload(path, file)

// Delete location from DB
await supabase.from('locations').delete().eq('id', id)
// ❌ File in storage is orphaned

Fix:

// Option 1: Cascade delete via database trigger
// Option 2: Manual cleanup
const { data: location } = await supabase
  .from('locations')
  .select('file_path')
  .eq('id', id)
  .single()

if (location?.file_path) {
  await supabase.storage.from('files').remove([location.file_path])
}

await supabase.from('locations').delete().eq('id', id)

---

When to Request Review

Always Review

• Before pushing to main
• After implementing new features
• After significant refactoring
• When adding external dependencies
• When changing database schema
• When modifying authentication/authorization

Optional Review

• Minor documentation updates
• Fixing typos
• Updating comments
• Configuration tweaks (unless security-related)

---

Review Response Template

When requesting review, Claude responds with:

## Code Review: [Feature Name]

### ✅ Strengths
- Well-structured architecture
- Good separation of concerns
- Proper security considerations

### 🚨 Critical Issues (P0)
1. **[Issue]** - [Description]
   - Location: `file.ts:123`
   - Impact: [Impact]
   - Fix: [Recommendation]

### ⚠️ High Priority (P1)
1. **[Issue]** - [Description]

### 📋 Medium Priority (P2)
1. **[Issue]** - [Description]

### 🎯 Priority Order for Fixes
1. P0: [List]
2. P1: [List]
3. P2: [List]

---

Integration with Git Workflow

Standard Development Flow

# 1. Start feature
git checkout -b feature/gpx-upload

# 2. Implement feature
# ... write code ...

# 3. Commit locally
git add -A
git commit -m "feat: add GPX upload support"

# 4. Request review
# Ask Claude: "evaluate the code"

# 5. Fix critical issues
git add -A
git commit -m "fix: address P0 review findings"

# 6. Push after approval
git push origin feature/gpx-upload

# 7. Create PR (if using PRs) or merge to main

Amending Commits (Before Push)

If review finds issues before pushing:

# Fix issues
git add -A
git commit --amend --no-edit  # Add to previous commit

# Or create new fix commit
git commit -m "fix: address review findings"

Never amend commits that have been pushed.

---

Automation

Automated PR Review (Implemented)

For feature branches, we now have automated code review via GitHub Actions:

# Create feature branch
git checkout -b feature/new-dashboard

# Work and commit
git add -A && git commit -m "feat: add dashboard"
git push -u origin feature/new-dashboard

# Create PR - automated review runs
gh pr create --fill

# Review feedback posted automatically
# Fix P0 issues and merge

See Automated PR Review for complete documentation.

Pre-push Hook (Future Enhancement)

Could add .git/hooks/pre-push:

#!/bin/bash
# Run linting and type checking before push
npm run lint || exit 1
npx tsc --noEmit || exit 1

---

Documentation Maintenance

When code review reveals documentation gaps:

1. Update relevant docs in /root/tower-fleet/docs/
2. Add to code-patterns.md if new pattern discovered
3. Update CLAUDE.md if process changes needed
4. Create TODO in PROJECTS.md for larger doc efforts

---

Milestone Tagging

For significant features/phases, tag the completion commit:

# After pushing reviewed code
git tag -a <tag-name> -m "Description"
git push origin --tags

Tag naming conventions: - phase<N>-<name> - Major system phases (e.g., phase1-relationships) - v<major>.<minor>.<patch> - Versioned releases - <feature>-v1 - Feature milestones

Why tags over commit SHAs in docs? - Tags survive rebases and squashes - Human-readable names - Queryable: git tag -l, git show <tag>

Document tags in project's CLAUDE.md so Claude Code can reference them.

---

Related Documentation

• Automated PR Review: /root/tower-fleet/docs/workflows/automated-pr-review.md
• Development Environment: /root/tower-fleet/docs/workflows/development-environment.md
• Testing & QA: /root/tower-fleet/docs/workflows/testing-qa.md
• Git Patterns: /root/GIT_PATTERNS.md
• GitHub Actions: /root/tower-fleet/docs/reference/github-actions.md
• Code Patterns: /root/tower-fleet/docs/reference/code-patterns.md
• App Conventions: /root/tower-fleet/docs/reference/app-conventions.md