Code Review Typescript

Code Review — TypeScript

When asked to review code — a file, a function, a PR diff, or a module — follow this checklist systematically. Always provide specific, actionable feedback with line references and concrete improvement suggestions. Never give vague praise or criticism.

---

1. Understand the Intent First

Before writing any comment:

• Read the code top-to-bottom at least once without commenting.
• Understand what the code is supposed to do, not just what it does.
• Note the surrounding context (imports, types, callers).

---

2. Correctness & Logic

Check for bugs, edge cases, and incorrect assumptions.

• Null/undefined safety: are optional values guarded before use? Look for unchecked ?. chains that silently return undefined instead of throwing.
• Type narrowing gaps: is typeof, instanceof, or a discriminated union guard used before accessing a narrowed type?
• Return completeness: does every code path return a value? Does TypeScript's noImplicitReturns flag these?
• Array/object mutation: is the caller's data mutated unintentionally? Flag in-place sorts or push/splice on received arrays.
• Equality: == vs === — always use === in TypeScript.
• Off-by-one errors, incorrect loop bounds, or fence-post mistakes.

Example:

// ❌ Bug: silently returns undefined if user is not found — caller likely expects to throw
const name = users.find(u => u.id === id)?.name;

// ✅ Fix: be explicit about the missing case
const user = users.find(u => u.id === id);
if (!user) throw new Error(`User ${id} not found`);
const name = user.name;

---

3. Type Safety

• any usage: flag every any. Suggest unknown + type guard, or a proper interface.
• Type assertions (as): flag casts that bypass the type system without a guard. as unknown as X is almost always wrong.
• Non-null assertions (!): flag every ! postfix operator — it disables null checking. Require a comment explaining why it's safe, or replace with a guard.
• strict: true: if the tsconfig lacks strict: true, flag it. At minimum, strictNullChecks and noImplicitAny must be enabled.
• Overly wide types: string | number | boolean | object — suggest a discriminated union or a named type.
• interface vs type: prefer interface for object shapes that may be extended; type for unions, intersections, and aliases.

Example:

// ❌ Unsafe: assertion without evidence
const result = response.data as UserProfile;

// ✅ Safe: validate at the boundary
if (!isUserProfile(response.data)) throw new TypeError('Unexpected API response shape');
const result: UserProfile = response.data;

---

4. Async & Concurrency

• Unhandled promise rejections: every Promise must be either await-ed, .catch()-ed, or explicitly void-ed with a comment.
• Floating async functions: async functions called without await silently drop errors.
• Blocking in async context: fs.readFileSync, execSync, or CPU-heavy loops in an async function block the event loop — flag them.
• Sequential vs parallel: independent await calls that could run with Promise.all are a performance issue.
• Promise.all failure modes: Promise.all fails fast on the first rejection; use Promise.allSettled when partial failure is acceptable.
• Race conditions: shared mutable state mutated across await points without a lock is a race condition.

Example:

// ❌ Sequential when independent
const user = await fetchUser(id);
const orders = await fetchOrders(id);

// ✅ Parallel
const [user, orders] = await Promise.all([fetchUser(id), fetchOrders(id)]);

---

5. Security

• Hardcoded secrets: flag any hardcoded API keys, tokens, passwords, or connection strings as CRITICAL.
• Never log secrets or PII: check console.log / logger calls for request bodies, auth headers, or user data.
• Input validation: all external inputs (HTTP request bodies, query params, env vars) must be validated — flag raw req.body.x access without a schema (Zod, Valibot, or class-validator).
• Prototype pollution: Object.assign({}, userInput) or {...userInput} with untrusted input can pollute prototypes — prefer explicit field extraction.
• SQL/NoSQL injection: never use template literals or string concatenation to build queries; always use parameterized queries or an ORM.
• eval / Function(): flag any dynamic code execution.
• Path traversal: path.join(baseDir, userInput) without normalization and boundary-checking is a vulnerability.

---

6. Design & Architecture

• Single Responsibility: each function/class should do one thing. If a name needs "and", flag it.
• DRY: flag duplicated logic and suggest extraction.
• Dependency direction: high-level modules should not import implementation details from lower-level modules.
• Magic values: hardcoded strings and numbers should be named constants or config entries.
• Long parameter lists (4+): suggest an options object with a typed interface.
• Boolean parameters: a doX(data, true) call is opaque — suggest two functions or a discriminated union.
• God functions: functions over ~50 lines are a signal to decompose.

---

7. Error Handling

• Silent swallowing: catch (e) {} or catch (e) { return null; } hides failures — require logging and re-throwing or explicit handling.
• Error types: throwing plain strings (throw "something failed") loses stack traces — always throw Error instances or subclasses.
• Error propagation: errors at async boundaries must be caught or they become unhandled rejections.
• Result types: in domain logic, consider Result patterns over throwing for expected failures.

// ❌ Loses the stack trace
throw "user not found";

// ✅
throw new UserNotFoundError(`User ${id} not found`);

---

8. Testing

• Coverage: is the changed code covered? Call out specific untested paths explicitly.
• Test scope: unit tests must mock external I/O (HTTP clients, DB, filesystem). Integration tests may use real services.
• Edge cases: are error paths, empty inputs, and boundary values tested — not just the happy path?
• Assertions: tests must assert on the expected output or side effect — not just that the code ran without throwing.
• Isolation: tests must not share mutable state or depend on execution order. Use beforeEach/afterEach to reset.
• Naming: it('should when ') — not it('test 1').
• expect.assertions(n): use in async tests to catch cases where the assertion is never reached.

---

9. Style & Consistency

• ESLint: flag obvious ESLint violations if a config is present.
• const over let: prefer const for all values that are not reassigned.
• Imports: external packages before internal modules; no wildcard imports (import * as foo).
• Naming: camelCase for variables/functions, PascalCase for types/classes/components, SCREAMING_SNAKE_CASE for module-level constants.
• console.log: flag any left in production code — use a proper logger.
• TODO/FIXME comments: must include context and ideally a ticket reference. Flag vague or stale ones.
• Unused variables: flag _unused that are not intentional; TypeScript's noUnusedLocals should catch these.

---

10. Output Format

Structure your response as follows:

Summary

A 2–4 sentence overview: what does the code do, is it generally well-written, what is the most important concern?

Critical Issues 🔴

Issues that must be fixed before merging: security vulnerabilities, data loss risks, correctness bugs, broken contracts. Number each item.

Improvements 🟡

Things that are not blocking but meaningfully improve quality: design issues, missing tests, performance problems, unclear naming. Number each item.

Minor / Nits 🟢

Style, convention, and cleanup items that improve consistency but have minimal functional impact. Can be bulleted for brevity.

Positive Highlights ✅

Call out what is done well. This is not optional — good feedback is balanced.

---

Quick Reference Checklist

• [ ] Logic is correct for all known edge cases
• [ ] No unchecked null/undefined access
• [ ] No any — use unknown + type guard or a proper type
• [ ] No unsafe type assertions (as X) without a guard
• [ ] No non-null assertions (!) without justification
• [ ] strict: true (or at minimum strictNullChecks + noImplicitAny) in tsconfig
• [ ] All promises are awaited, caught, or explicitly voided
• [ ] No blocking I/O in async context
• [ ] Independent async calls use Promise.all
• [ ] No hardcoded secrets or PII in logs
• [ ] All external inputs are validated with a schema
• [ ] No silent exception swallowing
• [ ] Errors thrown as Error instances, not strings
• [ ] Test coverage exists for the changed code
• [ ] No console.log left in production paths
• [ ] No magic numbers or strings — use named constants

Source & license

This open-source skill is cataloged on AgentStack and links to its original source — we do not rehost the code.

• Author: ucdavis
• Source: ucdavis/ai-skills-registry
• License: MIT

Install and usage instructions live in the source repository linked above.