Code Review Checklist

Code Review Checklist

Quick Checklist (paste into PR comments)

## Review — [area]

### Security
- [ ] No user input interpolated directly into SQL, shell commands, or HTML
- [ ] Auth/authorization check present on every new endpoint
- [ ] No secrets, tokens, or passwords in code or committed config
- [ ] Sensitive data not logged or exposed in error responses

### Correctness
- [ ] All code paths return a value or throw — no silent `undefined` returns
- [ ] Edge cases handled: empty array, null/undefined, zero, empty string
- [ ] Error paths tested, not just the happy path
- [ ] No off-by-one errors in loops or pagination

### Performance
- [ ] No N+1 queries inside loops
- [ ] Payloads not unbounded (pagination or limit enforced)
- [ ] Expensive computations not re-running on every render/request
- [ ] No synchronous blocking I/O on the main thread

### Maintainability
- [ ] Names describe what the thing IS or DOES, not how it's implemented
- [ ] Functions do one thing; complex ones have a comment explaining why
- [ ] No magic numbers — named constants used instead
- [ ] Dead code removed before merging

### Tests
- [ ] New behavior has at least one test
- [ ] Tests assert outcomes, not implementation details
- [ ] Mocks reset between tests
- [ ] No `console.log` or skipped tests left behind

Security

SQL / Command Injection

// ❌ TRIGGERS REVIEW — string interpolation into query
const results = await db.query(`SELECT * FROM orders WHERE user_id = ${userId}`);

// ✅ PASS — parameterized query
const results = await db.query('SELECT id, total FROM orders WHERE user_id = $1', [userId]);

Missing Authorization

// ❌ TRIGGERS REVIEW — authenticated but no ownership check
export async function DELETE(req: Request, { params }: { params: { id: string } }) {
  await db.post.delete({ where: { id: params.id } });
  return new Response(null, { status: 204 });
}

// ✅ PASS — session verified, ownership enforced
export async function DELETE(req: Request, { params }: { params: { id: string } }) {
  const session = await getServerSession();
  if (!session) return new Response('Unauthorized', { status: 401 });

  const post = await db.post.findUnique({ where: { id: params.id } });
  if (!post) return new Response('Not found', { status: 404 });
  if (post.authorId !== session.user.id) return new Response('Forbidden', { status: 403 });

  await db.post.delete({ where: { id: params.id } });
  return new Response(null, { status: 204 });
}

Correctness

Unhandled Edge Cases

// ❌ TRIGGERS REVIEW — crashes if array is empty, returns wrong index if not sorted
function findCheapest(products: Product[]): Product {
  return products.sort((a, b) => a.price - b.price)[0];
}

// ✅ PASS — explicit guard, original array not mutated
function findCheapest(products: Product[]): Product | null {
  if (products.length === 0) return null;
  return [...products].sort((a, b) => a.price - b.price)[0];
}

Silent undefined Returns

// ❌ TRIGGERS REVIEW — if no match, returns undefined; callers rarely handle this
function getConfig(key: string) {
  const configs = loadConfigs();
  return configs.find(c => c.key === key)?.value;
}

// ✅ PASS — explicit about the missing case
function getConfig(key: string): string {
  const config = loadConfigs().find(c => c.key === key);
  if (!config) throw new Error(`Config key "${key}" not found`);
  return config.value;
}

Performance

N+1 in a Loop

// ❌ TRIGGERS REVIEW — 1 query per comment = N+1
const comments = await db.comment.findMany({ where: { postId } });
const enriched = await Promise.all(
  comments.map(async (c) => ({
    ...c,
    author: await db.user.findUnique({ where: { id: c.authorId } }), // ← N queries
  })),
);

// ✅ PASS — single query with include
const comments = await db.comment.findMany({
  where: { postId },
  include: { author: { select: { id: true, name: true } } },
});

Unbounded Payloads

// ❌ TRIGGERS REVIEW — returns entire table; catastrophic on 1M row tables
export async function GET() {
  const users = await db.user.findMany();
  return Response.json(users);
}

// ✅ PASS — hard cap enforced
export async function GET(req: Request) {
  const limit = Math.min(Number(new URL(req.url).searchParams.get('limit') ?? 20), 100);
  const cursor = new URL(req.url).searchParams.get('cursor') ?? undefined;
  const users = await db.user.findMany({ take: limit, cursor: cursor ? { id: cursor } : undefined });
  return Response.json({ data: users, nextCursor: users.at(-1)?.id ?? null });
}

Maintainability

Magic Numbers

// ❌ TRIGGERS REVIEW — what does 86400 mean? what does 3 mean?
if (Date.now() - user.lastLoginAt > 86400 * 1000 * 3) {
  invalidateSession(user.id);
}

// ✅ PASS — self-documenting
const INACTIVE_SESSION_DAYS = 3;
const MS_PER_DAY = 24 * 60 * 60 * 1000;

if (Date.now() - user.lastLoginAt > INACTIVE_SESSION_DAYS * MS_PER_DAY) {
  invalidateSession(user.id);
}

Function Doing Too Much

// ❌ TRIGGERS REVIEW — one function handles parsing, validation, DB write, email, logging
async function handleRegistration(req: Request) {
  const body = await req.json();
  if (!body.email || !body.password) return error(400, 'Missing fields');
  if (body.password.length < 8) return error(400, 'Password too short');
  const existing = await db.user.findUnique({ where: { email: body.email } });
  if (existing) return error(409, 'Already exists');
  const user = await db.user.create({ data: { ...body, password: hash(body.password) } });
  await sendWelcomeEmail(user.email);
  logger.info({ userId: user.id }, 'User registered');
  return ok(user);
}

// ✅ PASS — each concern in its own function; handleRegistration becomes an orchestrator
async function handleRegistration(req: Request) {
  const body = await parseRegistrationBody(req);
  await assertEmailAvailable(body.email);
  const user = await createUser(body);
  await sendWelcomeEmail(user.email).catch(err =>
    logger.warn({ err, userId: user.id }, 'Welcome email failed — non-fatal'),
  );
  return ok(user);
}

Tests

Asserting Behavior, Not Implementation

// ❌ TRIGGERS REVIEW — tests internal call count, not observable output
it('calls hashPassword once', async () => {
  const spy = vi.spyOn(authUtils, 'hashPassword');
  await createUser({ email: 'a@b.com', password: 'secret' });
  expect(spy).toHaveBeenCalledTimes(1);
});

// ✅ PASS — tests what the user experiences
it('stores a hashed password, not plaintext', async () => {
  await createUser({ email: 'a@b.com', password: 'secret' });
  const user = await db.user.findUnique({ where: { email: 'a@b.com' } });
  expect(user?.passwordHash).toBeDefined();
  expect(user?.passwordHash).not.toBe('secret');
});

Checklist

• No injection vectors — SQL, shell, HTML all use parameterized/escaped APIs
• Every new endpoint has an auth check
• All edge cases considered: empty, null, zero, max length
• No N+1 queries introduced
• Paginated or limited response for collection endpoints
• Magic numbers replaced with named constants
• Functions are focused — each does one coherent thing
• New behavior covered by at least one test
• Tests assert outcomes, not spy call counts
• No leftover console.log, it.skip, or TODO: remove this