20 KiB

Raw Blame History

Fresh Eyes Review Checklist Reference

Extended details, examples, and severity guidance for each checklist category.

Type Safety

What to Check

No any types without justification:

// ◆◆ Severe
function process(data: any) { ... }

// ✓ Good - with justification
// @ts-expect-error: External library has incorrect types
function process(data: any) { ... }

// ✓ Better - narrow the type
function process(data: unknown) {
  if (typeof data === 'string') { ... }
}

Null/undefined handling:

// ◆ Moderate
const user = users.find(u => u.id === id);
return user.name; // might be undefined

// ✓ Good
const user = users.find(u => u.id === id);
return user?.name ?? 'Unknown';

Type guards for unions:

// ◆ Moderate
type Result = Success | Error;
function handle(result: Result) {
  if (result.success) { // Property doesn't exist
    return result.data;
  }
}

// ✓ Good - discriminated union
type Result =
  | { success: true; data: string }
  | { success: false; error: string };

function handle(result: Result) {
  if (result.success) {
    return result.data; // TypeScript knows this is safe
  }
}

Generic constraints:

// ◇ Minor
function getProperty<T>(obj: T, key: string) {
  return obj[key]; // No constraint, unsafe
}

// ✓ Good
function getProperty<T, K extends keyof T>(obj: T, key: K): T[K] {
  return obj[key];
}

Return types on public functions:

// ◇ Minor
export function calculate(x: number, y: number) {
  return x + y; // Inferred, but implicit
}

// ✓ Good - explicit contract
export function calculate(x: number, y: number): number {
  return x + y;
}

Severity Guidance

◆◆ any escaping to public API, type assertions that can crash
◆ Missing null checks, missing return types on public functions
◇ Overly broad types that work but reduce safety

Error Handling

What to Check

All error paths handled:

// ◆◆ Severe - silent failure
async function saveUser(user: User) {
  await db.insert(user); // Might throw, not handled
}

// ✓ Good
async function saveUser(user: User): Promise<Result<void, DbError>> {
  try {
    await db.insert(user);
    return { success: true };
  } catch (error) {
    logger.error('Failed to save user', { user, error });
    return { success: false, error: new DbError('Insert failed') };
  }
}

Meaningful error messages:

// ◆ Moderate
if (!user) throw new Error('Invalid');

// ✓ Good
if (!user) {
  throw new Error(`User not found: id=${userId}, searched at ${new Date()}`);
}

Promise rejection handling:

// ◆◆ Severe
fetchData().then(process); // Unhandled rejection

// ✓ Good
fetchData()
  .then(process)
  .catch(error => {
    logger.error('Fetch failed', { error });
    notifyUser('Data unavailable');
  });

// ✓ Better - async/await
try {
  const data = await fetchData();
  process(data);
} catch (error) {
  logger.error('Fetch failed', { error });
  notifyUser('Data unavailable');
}

Resource cleanup:

// ◆ Moderate
const file = await fs.open('data.txt');
const content = await file.readFile(); // Might throw, file not closed
return content;

// ✓ Good
const file = await fs.open('data.txt');
try {
  return await file.readFile();
} finally {
  await file.close();
}

Severity Guidance

◆◆ Silent failures, unhandled rejections, resource leaks
◆ Poor error messages, missing try/catch, errors swallowed
◇ Error messages could be more helpful, missing context

Security

What to Check

Input validation:

// ◆◆ Severe
app.get('/user/:id', (req, res) => {
  const user = db.query(`SELECT * FROM users WHERE id = ${req.params.id}`);
  res.json(user);
});

// ✓ Good
app.get('/user/:id', (req, res) => {
  const id = parseInt(req.params.id, 10);
  if (isNaN(id) || id < 0) {
    return res.status(400).json({ error: 'Invalid user ID' });
  }
  const user = db.query('SELECT * FROM users WHERE id = ?', [id]);
  res.json(user);
});

No hardcoded secrets:

// ◆◆ Severe
const API_KEY = 'sk_live_abc123xyz789';

// ✓ Good
const API_KEY = process.env.API_KEY;
if (!API_KEY) throw new Error('API_KEY not configured');

Authentication checks:

// ◆◆ Severe
app.delete('/admin/users/:id', (req, res) => {
  db.deleteUser(req.params.id);
  res.json({ success: true });
});

// ✓ Good
app.delete('/admin/users/:id', requireAuth, requireAdmin, (req, res) => {
  db.deleteUser(req.params.id);
  res.json({ success: true });
});

XSS prevention:

// ◆◆ Severe
function displayMessage(msg: string) {
  element.innerHTML = msg; // User content directly in HTML
}

// ✓ Good
function displayMessage(msg: string) {
  element.textContent = msg; // Automatically escaped
}

// Or use sanitization library
import DOMPurify from 'dompurify';
element.innerHTML = DOMPurify.sanitize(msg);

Password handling:

// ◆◆ Severe
if (user.password === inputPassword) { ... }

// ✓ Good
import bcrypt from 'bcrypt';
if (await bcrypt.compare(inputPassword, user.passwordHash)) { ... }

Severity Guidance

◆◆ SQL injection, XSS, auth bypass, exposed secrets, timing attacks
◆ Missing input validation, weak password checks, missing CSRF
◇ Could use stronger encryption, missing rate limits

Testing

What to Check

Tests exist:

// ◆ Moderate - new function, no tests
export function calculateDiscount(price: number, tier: string): number {
  if (tier === 'premium') return price * 0.8;
  if (tier === 'standard') return price * 0.9;
  return price;
}

Edge cases covered:

// ◆ Moderate - only happy path tested
test('calculateDiscount applies premium discount', () => {
  expect(calculateDiscount(100, 'premium')).toBe(80);
});

// ✓ Good - edge cases included
test('calculateDiscount handles edge cases', () => {
  expect(calculateDiscount(100, 'premium')).toBe(80);
  expect(calculateDiscount(100, 'standard')).toBe(90);
  expect(calculateDiscount(100, 'unknown')).toBe(100);
  expect(calculateDiscount(0, 'premium')).toBe(0);
  expect(calculateDiscount(-10, 'premium')).toBe(-8); // Or throw?
});

Actual assertions:

// ◆ Moderate - test doesn't verify behavior
test('user creation', async () => {
  await createUser({ name: 'Alice' }); // No assertion!
});

// ✓ Good
test('user creation', async () => {
  const user = await createUser({ name: 'Alice' });
  expect(user.name).toBe('Alice');
  expect(user.id).toBeDefined();

  const fromDb = await db.getUser(user.id);
  expect(fromDb).toEqual(user);
});

No test pollution:

// ◆ Moderate
test('first test', () => {
  globalState.users = [testUser];
  expect(findUser(1)).toEqual(testUser);
});

test('second test', () => {
  // Fails if first test didn't run or ran differently
  expect(globalState.users.length).toBe(1);
});

// ✓ Good
beforeEach(() => {
  globalState.users = [];
});

afterEach(() => {
  globalState.users = [];
});

test('first test', () => {
  globalState.users = [testUser];
  expect(findUser(1)).toEqual(testUser);
});

test('second test', () => {
  expect(globalState.users.length).toBe(0); // Clean slate
});

Error scenarios tested:

// ◆ Moderate - only success path tested
test('fetchUser retrieves user', async () => {
  const user = await fetchUser(1);
  expect(user.id).toBe(1);
});

// ✓ Good
test('fetchUser handles not found', async () => {
  await expect(fetchUser(999)).rejects.toThrow('User not found');
});

test('fetchUser handles network errors', async () => {
  mockApi.get.mockRejectedValue(new NetworkError());
  await expect(fetchUser(1)).rejects.toThrow(NetworkError);
});

Severity Guidance

◆◆ Critical paths untested, failing tests committed
◆ New functionality without tests, missing error scenarios, test pollution
◇ Could add more edge cases, test names unclear

Code Quality

What to Check

Names reveal intent:

// ◇ Minor
function proc(d: Data): number {
  const x = d.items.filter(i => i.active).length;
  return x * 1.2;
}

// ✓ Good
function calculateActiveItemsWithSurcharge(data: Data): number {
  const activeItemCount = data.items.filter(item => item.active).length;
  const SURCHARGE_MULTIPLIER = 1.2;
  return activeItemCount * SURCHARGE_MULTIPLIER;
}

Single responsibility:

// ◆ Moderate - doing too much
function processUserRequest(userId: string, action: string, data: any) {
  const user = db.getUser(userId);
  if (!user) throw new Error('Not found');

  logger.info('Processing', { userId, action });

  if (action === 'update') {
    db.updateUser(userId, data);
    email.send(user.email, 'Updated');
  } else if (action === 'delete') {
    db.deleteUser(userId);
    cache.clear(userId);
  }

  return { success: true };
}

// ✓ Good - separated concerns
function validateUser(userId: string): User {
  const user = db.getUser(userId);
  if (!user) throw new Error('User not found');
  return user;
}

function updateUser(userId: string, data: UserData): void {
  db.updateUser(userId, data);
  notifyUserUpdated(userId);
}

function deleteUser(userId: string): void {
  db.deleteUser(userId);
  cache.clear(userId);
}

No magic numbers:

// ◇ Minor
if (user.age > 65) { ... }
setTimeout(doWork, 86400000);

// ✓ Good
const SENIOR_AGE_THRESHOLD = 65;
if (user.age > SENIOR_AGE_THRESHOLD) { ... }

const ONE_DAY_MS = 24 * 60 * 60 * 1000;
setTimeout(doWork, ONE_DAY_MS);

DRY violations:

// ◆ Moderate
function validateEmail(email: string): boolean {
  return /^[^\s@]+@[^\s@]+\.[^\s@]+$/.test(email);
}

function validateAdminEmail(email: string): boolean {
  return /^[^\s@]+@[^\s@]+\.[^\s@]+$/.test(email) && email.endsWith('@admin.com');
}

// ✓ Good
const EMAIL_REGEX = /^[^\s@]+@[^\s@]+\.[^\s@]+$/;

function validateEmail(email: string): boolean {
  return EMAIL_REGEX.test(email);
}

function validateAdminEmail(email: string): boolean {
  return validateEmail(email) && email.endsWith('@admin.com');
}

Nested conditionals:

// ◆ Moderate - 4 levels deep
function process(user: User) {
  if (user) {
    if (user.active) {
      if (user.subscription) {
        if (user.subscription.plan === 'premium') {
          return doThing();
        }
      }
    }
  }
  return null;
}

// ✓ Good - early returns
function process(user: User) {
  if (!user) return null;
  if (!user.active) return null;
  if (!user.subscription) return null;
  if (user.subscription.plan !== 'premium') return null;

  return doThing();
}

Dead code:

// ◇ Minor
function calculate(x: number): number {
  const legacy = x * 2; // Unused, remove
  // const oldWay = x + 10; // Commented out, remove
  return x * 3;
}

Severity Guidance

◆◆ Functions >200 lines, cyclomatic complexity >15
◆ Functions >50 lines, DRY violations, deep nesting (>3)
◇ Unclear names, magic numbers, minor complexity

Documentation

What to Check

Public APIs documented:

// ◆ Moderate - exported, no docs
export function transformData(input: RawData, options: Options): ProcessedData {
  // ...
}

// ✓ Good
/**
 * Transforms raw sensor data into processed format for analysis.
 *
 * @param input - Raw data from sensor API
 * @param options - Processing options (sampling rate, filters)
 * @returns Processed data ready for visualization
 * @throws {ValidationError} If input data format is invalid
 *
 * @example
 * const processed = transformData(rawSensorData, {
 *   samplingRate: 100,
 *   filters: ['lowpass', 'normalize']
 * });
 */
export function transformData(input: RawData, options: Options): ProcessedData {
  // ...
}

Complex algorithms explained:

// ◆ Moderate - unclear why
function score(items: Item[]): number {
  return items.reduce((sum, item) => {
    const weight = item.priority * 0.7 + item.age * 0.3;
    return sum + (item.value * weight);
  }, 0) / items.length;
}

// ✓ Good
/**
 * Calculate weighted average score for items.
 *
 * Weight formula: (priority * 0.7) + (age * 0.3)
 * - Priority weighted more heavily (70%) as immediate importance
 * - Age contributes 30% to account for staleness
 *
 * Based on research paper: doi:10.1234/scoring-algorithm
 */
function score(items: Item[]): number {
  return items.reduce((sum, item) => {
    const weight = item.priority * 0.7 + item.age * 0.3;
    return sum + (item.value * weight);
  }, 0) / items.length;
}

Non-obvious decisions:

// ◆ Moderate - unclear why setTimeout
async function syncData() {
  await uploadToServer(data);
  setTimeout(cleanup, 5000);
}

// ✓ Good
async function syncData() {
  await uploadToServer(data);

  // Delay cleanup to allow server-side replication (typically 2-3s).
  // Without delay, we observed 15% data loss in distributed setup.
  // See issue #456 for full investigation.
  setTimeout(cleanup, 5000);
}

Breaking changes noted:

// ◆ Moderate
export function getUsers(): Promise<User[]> {
  // Changed from sync to async, breaking change not documented
}

// ✓ Good
/**
 * Fetch all users from database.
 *
 * @returns Promise resolving to array of users
 *
 * @breaking-change v2.0.0 - Now returns Promise instead of sync array.
 * Migration: Change `const users = getUsers()` to `const users = await getUsers()`
 */
export function getUsers(): Promise<User[]> {
  // ...
}

TODOs with context:

// ◇ Minor
// TODO: optimize this

// ✓ Good
// TODO(@alice): Optimize with caching once user volume >10k (ETA: Q2 2024)
// Current O(n²) acceptable for <1000 users, measured at 45ms p95

Severity Guidance

◆◆ Breaking changes undocumented
◆ Public APIs missing docs, complex algorithms unexplained
◇ TODOs without context, minor doc improvements

Performance

What to Check

N+1 queries:

// ◆ Moderate
async function getUsersWithPosts(userIds: string[]) {
  const users = await db.getUsers(userIds);
  for (const user of users) {
    user.posts = await db.getPostsByUser(user.id); // N queries!
  }
  return users;
}

// ✓ Good
async function getUsersWithPosts(userIds: string[]) {
  const users = await db.getUsers(userIds);
  const posts = await db.getPostsByUsers(userIds); // 1 query

  const postsByUser = posts.reduce((acc, post) => {
    (acc[post.userId] ||= []).push(post);
    return acc;
  }, {});

  return users.map(user => ({
    ...user,
    posts: postsByUser[user.id] || []
  }));
}

Appropriate data structures:

// ◆ Moderate - O(n) lookup
const activeUsers = users.filter(u => u.active);
function isActive(userId: string): boolean {
  return activeUsers.find(u => u.id === userId) !== undefined; // O(n)
}

// ✓ Good - O(1) lookup
const activeUserIds = new Set(users.filter(u => u.active).map(u => u.id));
function isActive(userId: string): boolean {
  return activeUserIds.has(userId); // O(1)
}

Unnecessary allocations:

// ◇ Minor
function processItems(items: Item[]) {
  return items
    .map(i => ({ ...i }))        // Copy 1
    .map(i => ({ ...i, processed: true }))  // Copy 2
    .filter(i => i.active);
}

// ✓ Good
function processItems(items: Item[]) {
  return items
    .filter(i => i.active)
    .map(i => ({ ...i, processed: true }));
}

Async operations:

// ◆ Moderate - sequential, slow
async function loadData() {
  const users = await fetchUsers();    // Wait
  const posts = await fetchPosts();    // Wait
  const comments = await fetchComments(); // Wait
  return { users, posts, comments };
}

// ✓ Good - parallel
async function loadData() {
  const [users, posts, comments] = await Promise.all([
    fetchUsers(),
    fetchPosts(),
    fetchComments()
  ]);
  return { users, posts, comments };
}

Severity Guidance

◆◆ Performance bugs (infinite loops, memory leaks)
◆ Obvious N+1, blocking operations in hot path
◇ Minor optimizations, better data structures possible

Rust-Specific

What to Check

Result over panic:

// ◆◆ Severe
pub fn divide(a: i32, b: i32) -> i32 {
    a / b  // Panics on division by zero
}

// ✓ Good
pub fn divide(a: i32, b: i32) -> Result<i32, &'static str> {
    if b == 0 {
        Err("Division by zero")
    } else {
        Ok(a / b)
    }
}

No unwrap outside tests:

// ◆ Moderate
pub fn get_config() -> Config {
    fs::read_to_string("config.toml")
        .unwrap()  // Bad: panics on missing file
        .parse()
        .unwrap()
}

// ✓ Good
pub fn get_config() -> Result<Config, ConfigError> {
    let content = fs::read_to_string("config.toml")?;
    let config = content.parse()?;
    Ok(config)
}

Ownership/borrowing:

// ◆ Moderate - unnecessary clone
fn process_data(data: Vec<u8>) -> Vec<u8> {
    let copy = data.clone();  // Unnecessary allocation
    transform(copy)
}

// ✓ Good
fn process_data(data: Vec<u8>) -> Vec<u8> {
    transform(data)  // Move ownership
}

// Or if data is needed later:
fn process_data(data: &[u8]) -> Vec<u8> {
    transform(data)  // Borrow
}

Unsafe justification:

// ◆◆ Severe - unjustified unsafe
pub fn get_value(ptr: *const u8) -> u8 {
    unsafe { *ptr }
}

// ✓ Good
/// # Safety
///
/// `ptr` must be:
/// - Properly aligned for type `u8`
/// - Non-null
/// - Valid for reads (pointing to initialized memory)
/// - Not accessed concurrently from other threads
pub unsafe fn get_value(ptr: *const u8) -> u8 {
    unsafe { *ptr }
}

// ✓ Better - avoid unsafe if possible
pub fn get_value(slice: &[u8], index: usize) -> Option<u8> {
    slice.get(index).copied()
}

Severity Guidance

◆◆ Unjustified unsafe, panics in library code, Send/Sync violations
◆ unwrap/expect in production code, unnecessary clones
◇ Could use iterators, minor allocation improvements

Severity Summary

◆◆ Severe (blocking)

Ship with these → security incidents, runtime failures, data loss.

Examples:

SQL injection, XSS, auth bypass
Unhandled errors that crash
Type assertions that can panic
Hardcoded secrets
Unjustified unsafe code (Rust)
Test failures committed

Action: Must fix before merge.

◆ Moderate (should fix)

Ship with these → maintenance burden, degraded quality, potential bugs.

Examples:

Missing null checks
Poor error messages
Missing tests
Significant code duplication
Performance issues (N+1)
Missing docs on public APIs

Action: Fix before merge unless explicitly accepted as tech debt with tracking issue.

◇ Minor (consider addressing)

Ship with these → code could be better, but functional and safe.

Examples:

Unclear variable names
Magic numbers
Missing edge case tests
Minor performance optimizations
Documentation improvements
Code style inconsistencies

Action: Optional improvements. Consider batching in refactoring PR.

Review Patterns

Security-Sensitive Areas

Give extra scrutiny to:

Authentication/authorization logic
User input handling (forms, APIs, queries)
Database queries (SQL injection risk)
File uploads/downloads
Cryptographic operations
Session management
CORS/CSRF protections

Common False Positives

Type safety: as unknown as X sometimes necessary for complex type gymnastics — verify justification comment.

Error handling: Not all errors need recovery — sometimes propagating is correct.

Magic numbers: Domain constants (HTTP codes, standard ports) don't need extraction.

Function length: Pure data transformation can be >50 lines if clear.

When to Escalate

Flag for senior review if:

Security implications unclear
Performance impact uncertain
Architecture decision embedded
Breaking change considerations
Unfamiliar technology or pattern

Checklist Efficiency

Full Review (default)

Run all categories for:

Pre-merge reviews
Critical path changes
Security-sensitive code
Public API changes

Targeted Review

Focus specific categories for:

Refactors → Code Quality, Tests
Bug fixes → Error Handling, Tests, Type Safety
Performance work → Performance, Tests
Security patches → Security, Error Handling
Docs → Documentation

Quick Sanity Check (not Fresh Eyes)

For trivial changes (typos, formatting), skip formal review. Don't invoke Fresh Eyes skill — use judgment.

20 KiB Raw Blame History

Fresh Eyes Review Checklist Reference

Type Safety

What to Check

Severity Guidance

Error Handling

What to Check

Severity Guidance

Security

What to Check

Severity Guidance

Testing

What to Check

Severity Guidance

Code Quality

What to Check

Severity Guidance

Documentation

What to Check

Severity Guidance

Performance

What to Check

Severity Guidance

Rust-Specific

What to Check

Severity Guidance

Severity Summary

◆◆ Severe (blocking)

◆ Moderate (should fix)

◇ Minor (consider addressing)

Review Patterns

Security-Sensitive Areas

Common False Positives

When to Escalate

Checklist Efficiency

Full Review (default)

Targeted Review

Quick Sanity Check (not Fresh Eyes)

20 KiB

Raw Blame History