Review Changes

Review Changes

Review code changes in a feature branch and identify issues before merging.

Workflow

1. Determine Review Target

• Remote PR: If user provides PR number or URL (e.g., "review PR #123", "review https://github.com/org/repo/pull/123"):

  1. Checkout the PR: gh pr checkout <PR_NUMBER>
  2. Read PR context: gh pr view <PR_NUMBER> --json title,body,comments
  3. Use the PR description and comments as additional context for the review

• Local Changes: If no PR specified, review current branch against default branch (continue to step 2)

2. Detect Branches

# Get current branch
git branch --show-current

# Detect default branch (try remote HEAD, fall back to main)
git symbolic-ref refs/​remotes/​origin/​HEAD 2>/​dev/​null | sed 's@^refs/​remotes/​origin/​@@' || echo "main"

3. Get Changed Files

# List files changed between default branch and current branch
git diff --name-only <default-branch>...HEAD

# Get full diff for analysis
git diff <default-branch>...HEAD

4. Analyze Changes

Read each changed file and review for issues. Adapt review criteria to the detected language.

Review Process

For each changed file, systematically apply the guidelines from the "Things to look for" section:

1. Identify applicable guidelines: Based on the file type and content, determine which guidelines apply:

   • All General Guidelines apply to every file (API changes, auth, database, concurrency, async, external APIs, edge cases, defensive coding, memory/​performance, file system, logging, testing, accessibility, code style)
   • Add Language-Specific Guidelines based on file extension (.ts/.js → TypeScript/​JavaScript, .tsx/.jsx → React, .py → Python, .go → Go, .rs → Rust, .java/.kt → Java/​Kotlin)

2. Review each guideline explicitly: Go through each applicable guideline and check if the code violates it:

   • Read the guideline's checklist items
   • Scan the changed code for violations
   • If a violation is found, note the file, line number, and specific issue
   • Assign severity based on the guideline's indicated level (Critical, Major, or Minor)

3. Skip inapplicable guidelines: If a guideline doesn't apply to the code (e.g., no database operations → skip Database & Persistence), move to the next one.

4. Document findings as you go: For each issue found, immediately record:

   • Which guideline was violated
   • The specific file and line number
   • What the problem is
   • How to fix it

Example review flow for a TypeScript API endpoint file:

□ API & Breaking Changes - Check for signature changes, removed fields
□ Authentication & Authorization - Check for auth middleware on routes
□ Database & Persistence - Check for N+1 queries, missing transactions
□ Concurrency - Check for race conditions in shared state
□ Async Code - Check for post-await validation
□ External API Handling - Check for timeouts, retries, error handling
□ Edge Cases & Boundaries - Check for null checks, empty arrays
□ Defensive Coding - Check for input validation
□ Memory & Performance - Check for unbounded collections, leaks
□ File System Operations - Check for existence checks, error handling
□ Logging & Observability - Check for meaningful error context
□ Testing Quality - N/​A (not a test file)
□ Accessibility - N/​A (not a UI file)
□ Code Style - Check for early returns, flat code
□ TypeScript/​JavaScript - Check for dispose(), path.join(), URLSearchParams
□ React - N/​A (not a React component)

Critical Issues (Must Fix)

Block the merge. These will break production or cause security incidents.

• Broken code: Syntax errors, undefined variables, missing imports, type errors
• Security vulnerabilities: SQL injection, XSS, exposed secrets/​credentials, insecure auth
• Data leaks: Logging sensitive data, exposing PII, missing access controls
• Runtime failures: Code paths that will definitely crash in production

Major Issues (Should Fix)

Will likely cause problems for users or developers.

• Unhandled async: Promises without .catch() or try/​catch, missing await
• Missing error handling: Bare try blocks, swallowed exceptions, no fallback for failures
• No timeouts/​retries: Network calls without timeout, no retry logic for flaky operations
• Resource leaks: Unclosed connections, file handles, event listeners not cleaned up
• Race conditions: Shared state mutations, missing locks, concurrent access issues
• Missing validation: User input not validated, missing null checks on external data

Minor Issues (Nice to Fix)

Won't break anything but would improve the code.

• Code clarity: Confusing variable names, overly complex logic, missing comments on non-obvious code
• Consistency: Mixed patterns, inconsistent naming, style drift from codebase norms
• Performance: Obvious inefficiencies, N+1 patterns, unnecessary re-computation, O(n²) lookups where a Set or Map would provide O(1)
• Dead code: Unused variables, unreachable branches, commented-out code, redundant checks that can never be true due to earlier conditions
• Code duplication: Near-identical code blocks that should be extracted to a helper function
• TODO/​FIXME: Unresolved todos that should be addressed

5. Check Test Coverage

For each new or modified file containing business logic:

• Check if corresponding test file exists
• If tests exist, verify new code paths have coverage
• Flag missing tests for critical paths

6. Format Output

Present findings grouped by severity, ordered Critical → Major → Minor.

## Branch Review: `feature/​xyz` → `main`

### 🔴 Critical (X issues)

**1. [Brief title]**
- **File**: `path/​to/​file.ts:42`
- **Problem**: Clear description of what's wrong
- **Fix**: Specific solution

### 🟠 Major (X issues)

**1. [Brief title]**
- **File**: `path/​to/​file.ts:87`
- **Problem**: Description
- **Fix**: Solution

### 🟡 Minor (X issues)

**1. [Brief title]**
- **File**: `path/​to/​file.ts:123`
- **Problem**: Description
- **Fix**: Solution

---
**Summary**: X critical, Y major, Z minor issues found.
[Ready to merge / Needs fixes before merge]

If no issues found in a category, omit that section. End with clear merge recommendation.

Feedback Tone

• Be constructive and professional—explain why a change is needed, not just what to change
• Provide actionable suggestions: instead of "this is wrong", say "consider X because Y"
• For approvals, acknowledge the specific value of the contribution (e.g., "Good use of early returns for readability")
• When flagging issues, assume positive intent—the author may not have been aware of the concern

Things to look for

Adapt review focus based on detected language and framework.

---

General Guidelines

API & Breaking Changes

Flag changes that could break existing consumers:

• Removed or renamed public functions/​methods without deprecation period
• Changed function signatures (new required parameters, changed return types)
• Modified response shapes in API endpoints (removed fields, changed types)
• Changed default values that alter existing behavior
• Database schema changes without migration scripts

// ❌ Bad - breaking change without migration path
function getUser(id: string): User  // was: function getUser(id: number): User

// ✅ Good - backwards compatible with deprecation
/​** @deprecated Use getUserById(id: string) instead. Will be removed in v3.0 */
function getUser(id: number): User
function getUserById(id: string): User

Authentication & Authorization

Flag as Critical when security boundaries are weakened:

• Missing auth checks on new endpoints or routes
• Downgraded permissions (e.g., changing from admin-only to public)
• Hardcoded credentials or API keys in source code
• JWT/​session issues: Missing expiry, weak secrets, improper validation
• CORS misconfigurations: Overly permissive origins (* in production)

// ❌ Bad - endpoint without auth check
app.get('/​api/​users/:id/​private-data', (req, res) => {
  return db.getPrivateData(req.params.id);
});

// ✅ Good - proper auth middleware
app.get('/​api/​users/:id/​private-data', requireAuth, checkOwnership, (req, res) => {
  return db.getPrivateData(req.params.id);
});

Database & Persistence

• Missing transactions for multi-step operations that should be atomic
• N+1 query patterns: Fetching related data in loops instead of joins/​eager loading
• Missing indexes on frequently queried columns (especially foreign keys)
• Unbounded queries: SELECT * without LIMIT on potentially large tables
• SQL injection: String concatenation in queries instead of parameterized queries

// ❌ Bad - N+1 queries
const users = await db.getUsers();
for (const user of users) {
  user.posts = await db.getPostsByUserId(user.id); // Query per user!
}

// ✅ Good - single query with join
const users = await db.getUsersWithPosts();

Concurrency

Flag as Critical for data corruption risks:

• Shared mutable state accessed from multiple threads without synchronization
• Missing locks/​mutexes when modifying shared resources
• Check-then-act patterns without atomicity (TOCTOU vulnerabilities)
• Deadlock potential: Acquiring multiple locks in inconsistent order

// ❌ Bad - race condition (check-then-act)
if (!cache.has(key)) {
  const value = await expensiveComputation();
  cache.set(key, value); // Another thread may have set it already
}

// ✅ Good - atomic operation
const value = cache.getOrSet(key, () => expensiveComputation());

Async Code

After any await, verify that assumptions made before the await are still valid. The world may have changed: files may have been deleted, state may have been modified, or resources may no longer be available.

Post-Await State Validation

Flag when code returns success without verifying the expected outcome:

// ❌ Bad - assumes setup succeeded
await this.setupController(notebook);
return true;

// ✅ Good - validates result before returning success
await this.setupController(notebook);
if (!this.controllers.has(notebookKey)) {
  logger.warn('Setup did not create controller');
  return false;
}
return true;

External API Handling

• Missing timeouts on HTTP requests (can hang indefinitely)
• No retry logic for transient failures (network blips, 503s)
• Missing circuit breakers for repeatedly failing services
• Insufficient error handling: Not distinguishing between 4xx and 5xx errors
• Missing rate limiting awareness (no backoff on 429 responses)

// ❌ Bad - no timeout, no retry, swallows errors
const data = await fetch(url).then(r => r.json()).catch(() => null);

// ✅ Good - timeout, retry logic, proper error handling
const data = await fetchWithRetry(url, {
  timeout: 5000,
  retries: 3,
  backoff: 'exponential',
  onError: (err, attempt) => logger.warn(`Attempt ${attempt} failed`, err)
});

Edge Cases & Boundaries

• Empty collections: Code assumes arrays/​lists are non-empty
• Null/​undefined handling: Missing checks on optional values
• Boundary values: Off-by-one errors in loops, incorrect range checks
• Type coercion issues: Implicit conversions leading to unexpected behavior
• Unicode/​encoding issues: Assuming ASCII, incorrect string length calculations

// ❌ Bad - assumes non-empty array
const first = items[0].name;

// ✅ Good - handles empty case
const first = items[0]?.name ?? 'default';

Defensive Coding

• Trusting external input: Using data from APIs/​users without validation
• Assuming success: Not handling failure cases for operations that can fail
• Missing bounds checks: Array access without verifying index is valid
• Implicit assumptions: Code relies on undocumented behavior or ordering

// ❌ Bad - trusts external data shape
const userName = apiResponse.user.profile.name;

// ✅ Good - validates structure
const userName = apiResponse?.user?.profile?.name;
if (!userName) {
  throw new ValidationError('Invalid API response: missing user name');
}

Input Sanitization

External data is untrusted. Any input from users, files, APIs, or environment must be sanitized before use in sensitive contexts.

Command Injection

Flag as Critical. Never pass unsanitized input to shell commands. Use argument arrays instead of string interpolation.

// ❌ Bad - command injection vulnerability
const output = execSync(`git log --author="${userInput}"`);

// ✅ Good - use argument array (no shell interpolation)
const output = execFileSync('git', ['log', `--author=${userInput}`]);

// ❌ Bad - file content used in command
const config = fs.readFileSync('config.txt', 'utf8');
execSync(`process ${config}`); // config.txt could contain: "; rm -rf /"

// ✅ Good - validate/​sanitize file content first
const config = fs.readFileSync('config.txt', 'utf8');
if (!/^[a-zA-Z0-9_-]+$/​.test(config)) {
  throw new Error('Invalid config format');
}
execFileSync('process', [config]);

Path Traversal

Flag as Critical. User input in file paths can escape intended directories. Always validate and resolve paths.

// ❌ Bad - path traversal vulnerability
const filePath = `./​uploads/​${userFilename}`;
// userFilename could be: "../​../​../​etc/​passwd"

// ✅ Good - resolve and validate path stays within allowed directory
const uploadsDir = path.resolve('./​uploads');
const filePath = path.resolve(uploadsDir, userFilename);
if (!filePath.startsWith(uploadsDir + path.sep)) {
  throw new Error('Invalid file path');
}

XSS Prevention

Flag as Critical. User input rendered as HTML can execute malicious scripts. Always escape or use safe APIs.

// ❌ Bad - XSS vulnerability
element.innerHTML = `<p>Hello, ${userName}</​p>`;

// ✅ Good - use textContent for plain text
element.textContent = `Hello, ${userName}`;

// ✅ Good - escape if HTML structure needed
import { escape } from 'lodash';
element.innerHTML = `<p>Hello, ${escape(userName)}</​p>`;

// ❌ Bad - user input in URL without validation
link.href = userProvidedUrl; // Could be: "javascript:alert('xss')"

// ✅ Good - validate URL protocol
const url = new URL(userProvidedUrl);
if (!['http:', 'https:'].includes(url.protocol)) {
  throw new Error('Invalid URL protocol');
}
link.href = url.href;

Log Injection

Unsanitized input in logs can forge entries or inject control characters.

// ❌ Bad - log injection
logger.info(`User logged in: ${username}`);
// username could be: "admin\n[ERROR] System compromised"

// ✅ Good - sanitize or use structured logging
logger.info('User logged in', { username: username.replace(/[\n\r]/​g, '') });

// ✅ Better - structured logging with proper escaping
logger.info({ event: 'login', username }); // Let logger handle encoding

SQL Injection

Flag as Critical. Never concatenate user input into queries. Use parameterized queries or prepared statements.

// ❌ Bad - SQL injection
const query = `SELECT * FROM users WHERE name = '${userName}'`;

// ✅ Good - parameterized query
const query = 'SELECT * FROM users WHERE name = ?';
db.query(query, [userName]);

// ✅ Good - ORM with built-in escaping
const user = await User.findOne({ where: { name: userName } });

Memory & Performance

• Unbounded collections: Arrays/​maps that grow without limits
• Memory leaks: Event listeners not removed, closures holding references
• Large allocations in loops: Creating objects that could be reused
• Blocking operations: Synchronous I/​O or CPU-heavy work on main thread
• Missing pagination: Loading entire datasets when only a subset is needed

// ❌ Bad - unbounded cache, memory leak
const cache = new Map();
function getCached(key: string) {
  if (!cache.has(key)) {
    cache.set(key, computeExpensiveValue(key));
  }
  return cache.get(key);
}

// ✅ Good - LRU cache with max size
const cache = new LRUCache({ maxSize: 1000 });

File System Operations

Flag these issues:

• Missing file existence checks: Reading files without verifying they exist first
• Missing directory checks: Writing files without ensuring parent directory exists
• No error handling on fs operations: File operations that assume success without try/​catch or error callbacks

Look for patterns like:

• fs.readFile(path) without prior fs.existsSync(path) or fs.access() check
• fs.writeFile(path) without ensuring path.dirname(path) exists
• open(), fopen(), File.read() etc. without existence validation

Logging & Observability

Flag issues that hamper debugging production issues:

• Insufficient logging: No logs for important operations, errors without context
• Excessive logging: Verbose logs that create noise or performance issues
• Missing correlation IDs: Unable to trace requests across services
• Logging sensitive data: PII, passwords, tokens in log output (this is Critical)
• No metrics/​monitoring: New features without observability hooks

// ❌ Bad - no context for debugging
logger.error('Failed');

// ✅ Good - actionable log with context
logger.error('Payment processing failed', {
  orderId,
  userId,
  errorCode: err.code,
  attempt: retryCount
});

Testing Quality

• Tests that don't assert anything meaningful: Empty tests, assertions that always pass
• Missing edge case coverage: Only happy path tested
• Flaky tests: Tests with race conditions, time dependencies, or external dependencies
• Test pollution: Tests that affect each other (shared state, missing cleanup)
• Mocking too much: Tests that don't exercise real code paths

// ❌ Bad - test doesn't verify behavior
it('should process data', async () => {
  await processData(input);
  expect(true).toBe(true);
});

// ✅ Good - verifies actual behavior
it('should transform input data correctly', async () => {
  const result = await processData(input);
  expect(result.status).toBe('processed');
  expect(result.items).toHaveLength(3);
});

Accessibility

Flag accessibility issues:

• Missing alt text on images
• Non-semantic HTML: Using divs for buttons, missing form labels
• Keyboard inaccessible: Interactive elements not reachable via keyboard
• Missing ARIA labels on icon-only buttons
• Color-only indicators: Information conveyed only through color

// ❌ Bad - inaccessible button
<div onClick={handleClick} className="button">
  <Icon name="delete" />
</​div>

// ✅ Good - accessible button
<button onClick={handleClick} aria-label="Delete item">
  <Icon name="delete" aria-hidden="true" />
</​button>

Code Style

Prefer early returns over nested if statements. Flat code is easier to read and reason about.

// ❌ Bad - deeply nested
function process(data) {
  if (data) {
    if (data.valid) {
      if (data.items.length > 0) {
        return doWork(data);
      }
    }
  }
  return null;
}

// ✅ Good - early returns
function process(data) {
  if (!data) {
    return null;
  }
  if (!data.valid) {
    return null;
  }
  if (data.items.length === 0) {
    return null;
  }
  return doWork(data);
}

Error Messages

Flag error messages that fail to help users understand and resolve problems.

Requirements for good error messages:

• Actionable - Tell users what to do, not just what went wrong
• Contextual - Include what operation was attempted and relevant values
• Specific - "An error occurred" is never helpful
• User-appropriate - Don't expose stack traces or internal details to end users
• Include identifiers - Order IDs, user IDs, etc. for debugging
• Error codes - For programmatic handling and support tickets

// ❌ Bad - generic, not actionable
throw new Error('An error occurred');
throw new Error('Invalid input');
throw new Error('Operation failed');

// ✅ Good - specific, actionable, includes context
throw new Error('Failed to save user profile: email "[email protected]" is already registered. Please use a different email address.');
throw new Error(`Payment failed for order ${orderId}: card declined. Please check your card details or try a different payment method.`);
throw new Error(`File upload failed: ${filename} exceeds the 10MB size limit. Please compress the file or upload a smaller one.`);

For API/​system errors, wrap internal details appropriately:

// ❌ Bad - exposes internals, not helpful to users
throw new Error('ECONNREFUSED 127.0.0.1:5432');
throw new Error('NullPointerException at line 42');
throw new Error(err.stack);

// ✅ Good - user-friendly with error code for support
throw new UserFacingError(
  'Unable to save your changes. Please try again in a few moments.',
  { code: 'DB_UNAVAILABLE', internal: err.message }
);

---

Language/​Framework-Specific Guidelines

TypeScript/​JavaScript

• Unhandled promise rejections, missing await
• any types hiding bugs, incorrect type assertions
• Missing null checks (?. and ?? usage)
• Event listener cleanup in components

Disposable Resources

Flag when objects with dispose() methods are created but never disposed:

• CancellationTokenSource - must call dispose() in finally block
• Disposable subscriptions from event listeners
• Timer handles (setTimeout/setInterval) without cleanup

// ❌ Bad - CancellationTokenSource never disposed (causes listener leaks)
const cts = new CancellationTokenSource();
await doWork(cts.token);

// ✅ Good - disposed in finally block
const cts = new CancellationTokenSource();
try {
  await doWork(cts.token);
} finally {
  cts.dispose();
}

Filesystem

Always use path.resolve() or path.join() to build paths. Hardcoded path strings with / or \ separators break cross-platform compatibility.

// ❌ Bad - hardcoded path separators
const modelPath = "data/​models";
const filePath = dir + "/" + filename;

// ✅ Good - path module handles separators
import path from "path";

const modelPath = path.join("data", "models");
const filePath = path.join(dir, filename);

URLs

Always use URLSearchParams to construct query strings. Manual string concatenation is error-prone and fails to properly encode special characters.

// ❌ Bad - manual concatenation
const url = `/​api/​search?q=${query}&page=${page}`;

// ✅ Good - URLSearchParams handles encoding
const params = new URLSearchParams({ q: query, page: String(page) });
const url = `/​api/​search?${params}`;

Timers and Intervals

Always store timer IDs and clear them on cleanup. Uncleaned timers cause memory leaks and unexpected behavior.

// ❌ Bad - timer never cleared
function startPolling() {
  setInterval(() => fetchData(), 5000);
}

// ✅ Good - return cleanup function
function startPolling() {
  const id = setInterval(() => fetchData(), 5000);
  return () => clearInterval(id);
}

// ❌ Bad - setTimeout in loop without tracking
for (let i = 0; i < 5; i++) {
  setTimeout(() => process(i), i * 1000);
}

// ✅ Good - track all timer IDs for cleanup
const timers = [];
for (let i = 0; i < 5; i++) {
  timers.push(setTimeout(() => process(i), i * 1000));
}
// Later: timers.forEach(clearTimeout);

For precise timing, prefer recursive setTimeout over setInterval. Intervals can drift and stack if callbacks take longer than the interval:

// ⚠️ setInterval can drift and stack callbacks
setInterval(async () => {
  await slowOperation(); // If this takes >1000ms, callbacks stack
}, 1000);

// ✅ Recursive setTimeout waits for completion
function poll() {
  setTimeout(async () => {
    await slowOperation();
    poll(); // Schedule next only after completion
  }, 1000);
}

In Node.js, use unref() for background timers that shouldn't keep the process alive:

// Node.js: timer keeps process alive
const id = setInterval(checkHealth, 30000);

// Node.js: timer won't prevent process exit
const id = setInterval(checkHealth, 30000);
id.unref();

React

• Only use useEffect for side effects (data fetching, subscriptions, DOM mutations). Do not use it to derive or build state—use useMemo or compute during render instead.
• Ensure dependency arrays contain diffable values (primitives, stable references). Objects and arrays created inline will cause infinite re-renders.
• Missing key props in lists, or using array index as key when items can reorder
• State updates on unmounted components (missing cleanup in useEffect)

// ❌ Bad - deriving state in useEffect
const [fullName, setFullName] = useState('');
useEffect(() => {
  setFullName(`${firstName} ${lastName}`);
}, [firstName, lastName]);

// ✅ Good - derive during render
const fullName = `${firstName} ${lastName}`;

// ✅ Also good - memoize if expensive
const fullName = useMemo(() => `${firstName} ${lastName}`, [firstName, lastName]);

Conditional Hook Calls

Flag as Critical. Hooks must be called in the same order on every render. Placing hooks inside conditionals, loops, or after early returns breaks React's internal tracking.

// ❌ Bad - hook inside conditional
function Profile({ userId }) {
  if (!userId) {
    return <div>Please log in</​div>;
  }
  const [user, setUser] = useState(null); // Hook after early return!
  useEffect(() => fetchUser(userId), [userId]);
  return <div>{user?.name}</​div>;
}

// ✅ Good - hooks before any conditionals
function Profile({ userId }) {
  const [user, setUser] = useState(null);
  useEffect(() => {
    if (userId) {
      fetchUser(userId).then(setUser);
    }
  }, [userId]);

  if (!userId) {
    return <div>Please log in</​div>;
  }
  return <div>{user?.name}</​div>;
}

Mutating State Directly

Flag as Critical. React state must be treated as immutable. Direct mutations (.push(), .splice(), obj.prop = x) won't trigger re-renders and cause stale UI.

// ❌ Bad - mutating state directly
const [items, setItems] = useState([]);
const addItem = (item) => {
  items.push(item); // Mutation!
  setItems(items);  // Same reference, no re-render
};

const [user, setUser] = useState({ name: 'Alice', age: 30 });
const updateAge = () => {
  user.age = 31; // Mutation!
  setUser(user);
};

// ✅ Good - create new references
const addItem = (item) => {
  setItems([...items, item]);
};

const updateAge = () => {
  setUser({ ...user, age: 31 });
};

Missing Hook Dependencies

Variables used inside useEffect, useCallback, or useMemo must be listed in the dependency array to avoid stale closures.

// ❌ Bad - missing dependency
const [count, setCount] = useState(0);
const [multiplier, setMultiplier] = useState(2);

useEffect(() => {
  console.log(count * multiplier); // multiplier used but not in deps
}, [count]); // Missing: multiplier

// ✅ Good - all dependencies listed
useEffect(() => {
  console.log(count * multiplier);
}, [count, multiplier]);

Stale Closures in Callbacks

Event handlers and callbacks passed to child components may capture outdated state values if not properly managed.

// ❌ Bad - stale closure in interval
function Counter() {
  const [count, setCount] = useState(0);

  useEffect(() => {
    const id = setInterval(() => {
      console.log(count); // Always logs initial value (0)
      setCount(count + 1); // Always sets to 1
    }, 1000);
    return () => clearInterval(id);
  }, []); // count captured at mount time
}

// ✅ Good - use functional update
useEffect(() => {
  const id = setInterval(() => {
    setCount(c => c + 1); // Always has current value
  }, 1000);
  return () => clearInterval(id);
}, []);

// ✅ Also good - include in deps and re-create interval
useEffect(() => {
  const id = setInterval(() => {
    setCount(count + 1);
  }, 1000);
  return () => clearInterval(id);
}, [count]);

Functional setState for Derived Updates

When the next state depends on the previous state, always use the functional form to avoid race conditions with batched updates.

// ❌ Bad - may lose updates when batched
const increment = () => setCount(count + 1);
const incrementTwice = () => {
  setCount(count + 1);
  setCount(count + 1); // Both read same `count`, only increments by 1
};

// ✅ Good - functional updates always use latest state
const increment = () => setCount(c => c + 1);
const incrementTwice = () => {
  setCount(c => c + 1);
  setCount(c => c + 1); // Correctly increments by 2
};

Async Updates After Unmount

Async operations (fetch, timers) that update state after unmount cause memory leaks and "Can't perform state update on unmounted component" warnings. Use AbortController or cleanup flags.

// ❌ Bad - no cleanup for async operation
useEffect(() => {
  fetch(`/​api/​user/​${id}`)
    .then(res => res.json())
    .then(data => setUser(data)); // May run after unmount
}, [id]);

// ✅ Good - AbortController for fetch
useEffect(() => {
  const controller = new AbortController();
  fetch(`/​api/​user/​${id}`, { signal: controller.signal })
    .then(res => res.json())
    .then(data => setUser(data))
    .catch(err => {
      if (err.name !== 'AbortError') {
        throw err;
      }
    });
  return () => controller.abort();
}, [id]);

// ✅ Good - cleanup flag for other async
useEffect(() => {
  let cancelled = false;
  fetchUser(id).then(data => {
    if (!cancelled) {
      setUser(data);
    }
  });
  return () => { cancelled = true; };
}, [id]);

Inline Objects Breaking Memoization

Inline object/​array literals and arrow functions in JSX create new references on every render, breaking React.memo and causing unnecessary child re-renders.

// ❌ Bad - new object/​function on every render
function Parent() {
  return (
    <Child
      style={{ color: 'red' }}  // New object each render
      onClick={() => doSomething()}  // New function each render
      config={{ timeout: 5000 }}  // New object each render
    />
  );
}

// ✅ Good - stable references
const childStyle = { color: 'red' };
const childConfig = { timeout: 5000 };

function Parent() {
  const handleClick = useCallback(() => doSomething(), []);
  return (
    <Child
      style={childStyle}
      onClick={handleClick}
      config={childConfig}
    />
  );
}

Missing Form preventDefault

Form submissions without e.preventDefault() cause full page reloads in SPAs, losing application state.

// ❌ Bad - page reloads on submit
function LoginForm() {
  const handleSubmit = (e) => {
    // Missing e.preventDefault()
    login(email, password);
  };
  return <form onSubmit={handleSubmit}>...</​form>;
}

// ✅ Good - prevent default form behavior
function LoginForm() {
  const handleSubmit = (e) => {
    e.preventDefault();
    login(email, password);
  };
  return <form onSubmit={handleSubmit}>...</​form>;
}

Timer Cleanup in useEffect

Timers created in useEffect must be cleared in the cleanup function. Failing to do so causes memory leaks and state updates on unmounted components.

// ❌ Bad - timer not cleared
useEffect(() => {
  setTimeout(() => setStatus('ready'), 1000);
  setInterval(() => setCount(c => c + 1), 1000);
}, []);

// ✅ Good - cleanup both timers
useEffect(() => {
  const timeout = setTimeout(() => setStatus('ready'), 1000);
  const interval = setInterval(() => setCount(c => c + 1), 1000);
  return () => {
    clearTimeout(timeout);
    clearInterval(interval);
  };
}, []);

For timers that need to be cleared from event handlers, store the ID in a ref:

// ❌ Bad - can't clear timer from handler
function Debounced() {
  useEffect(() => {
    // How do we clear this from onClick?
  }, []);

  const handleClick = () => {
    setTimeout(() => save(), 500); // Creates new timer each click
  };
}

// ✅ Good - ref holds timer ID
function Debounced() {
  const timerRef = useRef<NodeJS.Timeout>();

  const handleClick = () => {
    clearTimeout(timerRef.current); // Clear previous
    timerRef.current = setTimeout(() => save(), 500);
  };

  useEffect(() => {
    return () => clearTimeout(timerRef.current);
  }, []);
}

XSS and Unsafe HTML

Flag as Critical. React escapes content by default, but dangerouslySetInnerHTML bypasses this protection. Only use it with sanitized content.

// ❌ Bad - XSS vulnerability
function Comment({ text }) {
  return <div dangerouslySetInnerHTML={{ __html: text }} />;
}

// ✅ Good - use a sanitizer library
import DOMPurify from 'dompurify';

function Comment({ text }) {
  return <div dangerouslySetInnerHTML={{ __html: DOMPurify.sanitize(text) }} />;
}

// ✅ Better - render as plain text when possible
function Comment({ text }) {
  return <div>{text}</​div>; // React escapes automatically
}

User-controlled URLs in href or src attributes can execute JavaScript:

// ❌ Bad - javascript: URLs execute code
<a href={userUrl}>Click here</​a>

// ✅ Good - validate protocol
function SafeLink({ url, children }) {
  const safeUrl = useMemo(() => {
    try {
      const parsed = new URL(url);
      return ['http:', 'https:'].includes(parsed.protocol) ? url : '#';
    } catch {
      return '#';
    }
  }, [url]);
  return <a href={safeUrl}>{children}</​a>;
}

Python

• Bare except: clauses swallowing errors
• Missing context managers for resources (with statements)
• Mutable default arguments
• Missing type hints on public APIs

# ❌ Bad - mutable default argument
def append_to(item, target=[]):
    target.append(item)
    return target

# ✅ Good - use None as default
def append_to(item, target=None):
    if target is None:
        target = []
    target.append(item)
    return target

Go

• Ignored error returns (_ = err)
• Unclosed resources (defer patterns)
• Data races in goroutines
• Nil pointer dereferences

// ❌ Bad - ignored error
result, _ := doSomething()

// ✅ Good - handle or propagate error
result, err := doSomething()
if err != nil {
    return fmt.Errorf("doSomething failed: %w", err)
}

Rust

• unwrap() on Result/​Option in non-test code
• Missing error propagation (? operator)
• Unsafe blocks without justification

// ❌ Bad - unwrap in production code
let value = some_option.unwrap();

// ✅ Good - handle the None case
let value = some_option.ok_or_else(|| Error::MissingValue)?;

Java/​Kotlin

• Unchecked exceptions, missing null handling
• Resource leaks (try-with-resources)
• Mutable shared state

// ❌ Bad - resource leak
FileInputStream fis = new FileInputStream("file.txt");
// ... use fis
fis.close(); // May not be called if exception thrown

// ✅ Good - try-with-resources
try (FileInputStream fis = new FileInputStream("file.txt")) {
    // ... use fis
} // Automatically closed