Tarun Moorjani

Posted on Feb 24

Code Review for TypeScript React: What to Look For

#react #typescript #programming #webdev

I've reviewed over 1,000 React PRs. Early on, I nitpicked everything: indentation, variable names, comment styles. My reviews were long, exhausting, and ineffective. Developers got defensive. Important issues slipped through while I argued about semicolons.

Then I learned to distinguish between what matters and what doesn't. I started catching bugs that would have shipped. I helped junior developers grow. My reviews became shorter but more valuable.

Here's what I look for now—organized by impact. Not style guides. Not preferences. The things that actually matter.

The Review Priority Pyramid

High Priority (Must Fix)
├─ Security vulnerabilities
├─ Type safety violations
├─ Logic bugs
└─ Performance issues

Medium Priority (Should Fix)
├─ Architecture concerns
├─ Accessibility issues
├─ Missing tests
└─ Poor error handling

Low Priority (Nice to Have)
├─ Code organization
├─ Naming improvements
└─ Minor optimizations

Ignore
├─ Personal style preferences
├─ Subjective opinions
└─ Bikeshedding

Start at the top. Work down. If you're commenting on indentation before type safety, you're doing it wrong.

Category 1: Type Safety (High Priority)

Red Flag: Liberal `any` Usage

// ❌ BLOCK: Defeats TypeScript's purpose
function handleSubmit(data: any) {
  // What shape is data? Who knows!
  api.post('/users', data);
}

const user: any = fetchUser();
user.nonExistentProperty; // No error, runtime bomb

// ✅ APPROVE: Proper typing
interface FormData {
  name: string;
  email: string;
}

function handleSubmit(data: FormData) {
  api.post('/users', data);
}

interface User {
  id: string;
  name: string;
}

const user: User = fetchUser();

Review comment template:

⚠️ Using `any` here removes all type safety. Could we type this as:

interface FormData {
  name: string;
  email: string;
}

This would catch errors at compile time instead of runtime.

Red Flag: Type Assertions (`as`) Without Justification

// ❌ QUESTION: Why is this safe?
const value = unknownData as ComplexType;

// ❌ BLOCK: Lying to TypeScript
const response = await fetch('/api/user');
const user = (await response.json()) as User; // What if server returns different shape?

// ✅ APPROVE: Runtime validation
import { z } from 'zod';

const UserSchema = z.object({
  id: z.string(),
  name: z.string(),
  email: z.string().email(),
});

const response = await fetch('/api/user');
const data = await response.json();
const user = UserSchema.parse(data); // Throws if invalid

Review comment:

🤔 This type assertion looks risky. Can we add runtime validation?

const UserSchema = z.object({...});
const user = UserSchema.parse(data);

Or if we're confident about the API shape, add a comment explaining why the assertion is safe.

Red Flag: Missing Null/Undefined Checks

// ❌ BLOCK: Potential null reference error
function UserProfile({ user }: { user: User | null }) {
  return <div>{user.name}</div>; // Crashes if user is null
}

// ❌ QUESTION: Is this safe?
const firstUser = users[0];
console.log(firstUser.name); // What if array is empty?

// ✅ APPROVE: Proper null handling
function UserProfile({ user }: { user: User | null }) {
  if (!user) return <div>No user found</div>;
  return <div>{user.name}</div>;
}

const firstUser = users[0];
if (firstUser) {
  console.log(firstUser.name);
}

// Or use optional chaining
console.log(users[0]?.name);

Red Flag: Improper Type Narrowing

// ❌ QUESTION: This doesn't narrow the type
function processValue(value: string | number) {
  if (value) { // Doesn't narrow
    return value.toFixed(2); // Error: string doesn't have toFixed
  }
}

// ✅ APPROVE: Proper type guard
function processValue(value: string | number) {
  if (typeof value === 'number') {
    return value.toFixed(2);
  }
  return value;
}

Category 2: React Patterns (High Priority)

Red Flag: Breaking Rules of Hooks

// ❌ BLOCK: Conditional hook usage
function Component({ condition }: { condition: boolean }) {
  if (condition) {
    const data = useFetch('/api/data'); // Hooks must be unconditional
  }
}

// ❌ BLOCK: Hook in loop
function Component({ items }: { items: Item[] }) {
  return items.map(item => {
    const data = useFetch(item.url); // Can't call hooks in loops
  });
}

// ✅ APPROVE: Hooks at top level
function Component({ condition }: { condition: boolean }) {
  const data = useFetch(condition ? '/api/data' : null);

  if (!condition) return null;
  // Use data
}

Red Flag: Missing Dependencies in useEffect/useCallback

// ❌ BLOCK: Stale closure bug
function Component({ userId }: { userId: string }) {
  const [user, setUser] = useState(null);

  useEffect(() => {
    fetchUser(userId).then(setUser);
  }, []); // Missing userId dependency!

  // User won't update when userId changes
}

// ✅ APPROVE: Correct dependencies
useEffect(() => {
  fetchUser(userId).then(setUser);
}, [userId]);

// ✅ ALSO GOOD: Acknowledge ESLint disable
useEffect(() => {
  // Only fetch on mount, ignoring userId changes
  // eslint-disable-next-line react-hooks/exhaustive-deps
  fetchUser(userId).then(setUser);
}, []);

Review comment:

⚠️ `userId` is used in the effect but not in the dependency array. This means the effect won't re-run when `userId` changes, causing stale data.

Add `userId` to dependencies:
useEffect(() => {
  fetchUser(userId).then(setUser);
}, [userId]);

Red Flag: Unnecessary Re-renders

// ❌ QUESTION: This re-renders on every parent render
function Parent() {
  return (
    <Child 
      onClick={() => console.log('clicked')} 
      style={{ margin: 10 }}
      config={{ theme: 'dark' }}
    />
  );
}

const Child = React.memo(({ onClick, style, config }) => {
  // Child re-renders every time because new objects/functions
});

// ✅ APPROVE: Stable references
function Parent() {
  const handleClick = useCallback(() => {
    console.log('clicked');
  }, []);

  const style = useMemo(() => ({ margin: 10 }), []);

  const config = useMemo(() => ({ theme: 'dark' }), []);

  return <Child onClick={handleClick} style={style} config={config} />;
}

// ✅ EVEN BETTER: Move static data outside component
const STYLE = { margin: 10 };
const CONFIG = { theme: 'dark' };

function Parent() {
  const handleClick = useCallback(() => {
    console.log('clicked');
  }, []);

  return <Child onClick={handleClick} style={STYLE} config={CONFIG} />;
}

Red Flag: State Derived from Props

// ❌ QUESTION: This gets out of sync
function Component({ initialCount }: { initialCount: number }) {
  const [count, setCount] = useState(initialCount);

  // If initialCount changes, count stays the same!
  return <div>{count}</div>;
}

// ✅ APPROVE: Use the prop directly
function Component({ count }: { count: number }) {
  return <div>{count}</div>;
}

// ✅ OR: Use key to reset state when prop changes
<Component key={initialCount} initialCount={initialCount} />

// ✅ OR: Sync with useEffect (if you must)
function Component({ initialCount }: { initialCount: number }) {
  const [count, setCount] = useState(initialCount);

  useEffect(() => {
    setCount(initialCount);
  }, [initialCount]);

  return <div>{count}</div>;
}

Red Flag: Mutating State Directly

// ❌ BLOCK: Direct mutation
function Component() {
  const [items, setItems] = useState<Item[]>([]);

  const addItem = (item: Item) => {
    items.push(item); // Mutation!
    setItems(items); // React won't detect the change
  };
}

// ✅ APPROVE: Immutable update
const addItem = (item: Item) => {
  setItems([...items, item]);
};

// ✅ APPROVE: Functional update
const addItem = (item: Item) => {
  setItems(prev => [...prev, item]);
};

Category 3: Performance Issues (High Priority)

Red Flag: Expensive Operations in Render

// ❌ QUESTION: This runs on every render
function Component({ items }: { items: Item[] }) {
  const sorted = items
    .filter(item => item.active)
    .sort((a, b) => b.priority - a.priority)
    .map(item => ({ ...item, formatted: formatItem(item) }));

  return <List items={sorted} />;
}

// ✅ APPROVE: Memoized computation
function Component({ items }: { items: Item[] }) {
  const sorted = useMemo(() => 
    items
      .filter(item => item.active)
      .sort((a, b) => b.priority - a.priority)
      .map(item => ({ ...item, formatted: formatItem(item) })),
    [items]
  );

  return <List items={sorted} />;
}

Red Flag: Large Bundle Imports

// ❌ QUESTION: Importing entire library
import _ from 'lodash'; // Bundles all of lodash
import * as dateFns from 'date-fns'; // Bundles all of date-fns

_.debounce(fn, 100);
dateFns.format(date, 'yyyy-MM-dd');

// ✅ APPROVE: Import only what you need
import debounce from 'lodash/debounce';
import { format } from 'date-fns';

debounce(fn, 100);
format(date, 'yyyy-MM-dd');

Red Flag: Inefficient List Rendering

// ❌ BLOCK: Missing keys or bad keys
items.map((item, index) => (
  <Item key={index} {...item} /> // Index as key is bad
));

items.map(item => (
  <Item {...item} /> // No key at all
));

// ✅ APPROVE: Stable, unique keys
items.map(item => (
  <Item key={item.id} {...item} />
));

Category 4: Security (High Priority)

Red Flag: XSS Vulnerabilities

// ❌ BLOCK: XSS vulnerability
function Comment({ comment }: { comment: string }) {
  return <div dangerouslySetInnerHTML={{ __html: comment }} />;
  // User can inject <script>alert('XSS')</script>
}

// ✅ APPROVE: Sanitize HTML
import DOMPurify from 'dompurify';

function Comment({ comment }: { comment: string }) {
  const sanitized = DOMPurify.sanitize(comment);
  return <div dangerouslySetInnerHTML={{ __html: sanitized }} />;
}

// ✅ BETTER: Use markdown or text
function Comment({ comment }: { comment: string }) {
  return <div>{comment}</div>; // React escapes by default
}

Red Flag: Exposing Sensitive Data

// ❌ BLOCK: Leaking API keys
const API_KEY = 'sk-123456789'; // Hardcoded in frontend
fetch(`https://api.com?key=${API_KEY}`);

// ❌ BLOCK: Logging sensitive data
console.log('User password:', password);
console.log('Credit card:', creditCard);

// ✅ APPROVE: Use environment variables (server-side)
// This should be in backend, not frontend
const response = await fetch('/api/proxy', {
  headers: { 'Authorization': 'Bearer server-side-token' }
});

// ✅ APPROVE: Never log sensitive data
console.log('User logged in:', user.id); // ID only, no sensitive data

Red Flag: Insecure Data Handling

// ❌ QUESTION: Is this input sanitized?
function SearchResults({ query }: { query: string }) {
  const results = await fetch(`/api/search?q=${query}`);
  // What if query contains SQL injection attempts?
}

// ✅ APPROVE: Use URL encoding
function SearchResults({ query }: { query: string }) {
  const encodedQuery = encodeURIComponent(query);
  const results = await fetch(`/api/search?q=${encodedQuery}`);
}

Category 5: Architecture & Design (Medium Priority)

Red Flag: God Components

// ❌ QUESTION: 500+ lines, does everything
function Dashboard() {
  // Fetches 10 different data sources
  // Handles authentication
  // Manages complex state
  // Renders everything

  return (
    <div>
      {/* 400 lines of JSX */}
    </div>
  );
}

// ✅ APPROVE: Decomposed
function Dashboard() {
  return (
    <DashboardLayout>
      <DashboardHeader />
      <DashboardStats />
      <DashboardCharts />
      <DashboardActivity />
    </DashboardLayout>
  );
}

Review comment:

💡 This component is doing a lot. Consider breaking it into smaller components:

- DashboardHeader (user info, navigation)
- DashboardStats (metrics cards)
- DashboardCharts (visualizations)
- DashboardActivity (recent activity)

Each can manage its own data fetching and state.

Red Flag: Prop Drilling

// ❌ QUESTION: Passing props through 5 levels
function App() {
  const user = useAuth();
  return <Layout user={user} />;
}

function Layout({ user }) {
  return <Sidebar user={user} />;
}

function Sidebar({ user }) {
  return <Navigation user={user} />;
}

function Navigation({ user }) {
  return <UserMenu user={user} />;
}

function UserMenu({ user }) {
  return <div>{user.name}</div>;
}

// ✅ APPROVE: Context or custom hook
const UserContext = createContext<User | null>(null);

function App() {
  const user = useAuth();
  return (
    <UserContext.Provider value={user}>
      <Layout />
    </UserContext.Provider>
  );
}

function UserMenu() {
  const user = useContext(UserContext);
  return <div>{user?.name}</div>;
}

Red Flag: Mixed Concerns

// ❌ QUESTION: Business logic in component
function ProductList() {
  const [products, setProducts] = useState([]);

  useEffect(() => {
    fetch('/api/products')
      .then(res => res.json())
      .then(data => {
        // Complex business logic
        const processed = data
          .filter(p => p.stock > 0)
          .map(p => ({
            ...p,
            discountedPrice: calculateDiscount(p),
            shipping: calculateShipping(p),
            taxes: calculateTaxes(p),
          }))
          .sort((a, b) => b.popularity - a.popularity);

        setProducts(processed);
      });
  }, []);

  return <div>{/* render */}</div>;
}

// ✅ APPROVE: Extract to custom hook
function useProducts() {
  const [products, setProducts] = useState([]);

  useEffect(() => {
    fetch('/api/products')
      .then(res => res.json())
      .then(data => {
        const processed = processProducts(data);
        setProducts(processed);
      });
  }, []);

  return products;
}

// Business logic in pure function (testable!)
function processProducts(products: Product[]): ProcessedProduct[] {
  return products
    .filter(p => p.stock > 0)
    .map(p => ({
      ...p,
      discountedPrice: calculateDiscount(p),
      shipping: calculateShipping(p),
      taxes: calculateTaxes(p),
    }))
    .sort((a, b) => b.popularity - a.popularity);
}

function ProductList() {
  const products = useProducts();
  return <div>{/* render */}</div>;
}

Category 6: Accessibility (Medium Priority)

Red Flag: Missing Semantic HTML

// ❌ QUESTION: Not accessible
function Button({ onClick, label }) {
  return (
    <div onClick={onClick} className="button">
      {label}
    </div>
  );
}

// ✅ APPROVE: Proper semantics
function Button({ onClick, label }) {
  return <button onClick={onClick}>{label}</button>;
}

Red Flag: Missing ARIA Labels

// ❌ QUESTION: Screen reader won't know what this is
<button onClick={handleClose}>
  <X /> {/* Icon only */}
</button>

// ✅ APPROVE: Accessible
<button onClick={handleClose} aria-label="Close dialog">
  <X />
</button>

// ✅ ALSO GOOD: Visible text
<button onClick={handleClose}>
  <X />
  <span>Close</span>
</button>

Red Flag: Keyboard Navigation Issues

// ❌ QUESTION: Can't navigate with keyboard
<div onClick={handleClick}>Click me</div>

// ✅ APPROVE: Keyboard accessible
<div
  role="button"
  tabIndex={0}
  onClick={handleClick}
  onKeyDown={(e) => {
    if (e.key === 'Enter' || e.key === ' ') {
      handleClick();
    }
  }}
>
  Click me
</div>

// ✅ BETTER: Use button element
<button onClick={handleClick}>Click me</button>

Category 7: Error Handling (Medium Priority)

Red Flag: Unhandled Promise Rejections

// ❌ QUESTION: What if this fails?
useEffect(() => {
  fetchData().then(setData);
}, []);

// ✅ APPROVE: Error handling
useEffect(() => {
  fetchData()
    .then(setData)
    .catch(error => {
      console.error('Failed to fetch data:', error);
      setError(error);
    });
}, []);

// ✅ BETTER: Async/await with try-catch
useEffect(() => {
  async function loadData() {
    try {
      const data = await fetchData();
      setData(data);
    } catch (error) {
      console.error('Failed to fetch data:', error);
      setError(error);
    }
  }

  loadData();
}, []);

Red Flag: Silent Failures

// ❌ BLOCK: Errors disappear
try {
  await updateUser(userId, data);
} catch (error) {
  // Nothing happens - user thinks it worked!
}

// ✅ APPROVE: Show error to user
try {
  await updateUser(userId, data);
  toast.success('Profile updated');
} catch (error) {
  toast.error('Failed to update profile');
  console.error(error);
}

Category 8: Testing (Medium Priority)

Red Flag: Untestable Code

// ❌ QUESTION: Hard to test - API call inside component
function UserProfile({ userId }: { userId: string }) {
  const [user, setUser] = useState(null);

  useEffect(() => {
    fetch(`/api/users/${userId}`)
      .then(res => res.json())
      .then(setUser);
  }, [userId]);

  return <div>{user?.name}</div>;
}

// ✅ APPROVE: Extract hook - easy to test
function useUser(userId: string) {
  const [user, setUser] = useState(null);

  useEffect(() => {
    fetch(`/api/users/${userId}`)
      .then(res => res.json())
      .then(setUser);
  }, [userId]);

  return user;
}

function UserProfile({ userId }: { userId: string }) {
  const user = useUser(userId);
  return <div>{user?.name}</div>;
}

// Test the hook in isolation
// Test the component with mocked hook

Red Flag: Missing Critical Tests

// Component that handles payments
function CheckoutForm() {
  const handleSubmit = async (data) => {
    await processPayment(data);
    await createOrder(data);
    await sendConfirmation(data);
  };

  return <form onSubmit={handleSubmit}>...</form>;
}

// ❌ QUESTION: Where are the tests?
// No tests for:
// - Payment processing
// - Error handling
// - Edge cases

Review comment:

⚠️ This component handles payment processing but I don't see tests. Could we add:

1. Test successful payment flow
2. Test payment failure handling
3. Test validation errors
4. Test edge cases (e.g., network timeout)

Payment code should have high test coverage.

What NOT to Comment On

❌ Don't Nitpick Style (Use Prettier/ESLint)

// ❌ Bad review comment:
"Please use single quotes instead of double quotes"
"Add a blank line here"
"This line is too long"

// ✅ Better: Configure Prettier/ESLint
// Let tools handle formatting
// Save reviews for meaningful feedback

❌ Don't Enforce Personal Preferences

// ❌ Bad review comment:
"I prefer to call this 'handleButtonClick' instead of 'onClick'"
"Can you use a for loop instead of map?"
"I don't like ternaries, use if/else"

// ✅ Better: Only comment if there's a real issue
// "This variable name is misleading because..."
// "This algorithm is O(n²), could we use..."

❌ Don't Rewrite Code in Comments

// ❌ Bad review comment:
"Here's how I would write this:
function myBetterVersion() {
  // 50 lines of alternative implementation
}
"

// ✅ Better:
"This could be simplified. What do you think about using 
Array.reduce here instead of multiple loops?"

The Art of Constructive Feedback

Use Questions, Not Commands

// ❌ Aggressive
"This is wrong. Use useMemo."

// ✅ Constructive
"Could we use useMemo here? This calculation runs on every render 
and might cause performance issues with large datasets."

Explain the "Why"

// ❌ Not helpful
"Add error handling."

// ✅ Helpful
"Could we add error handling here? If the API fails, users won't 
know what happened. A toast notification would improve UX."

Offer Alternatives

// ❌ Vague
"This could be better."

// ✅ Specific
"Two options for this:
1. Extract to custom hook for reusability
2. Use React Query for caching

Option 1 if we use this once, option 2 if we need it in multiple places."

Distinguish Blocking vs Non-Blocking

// 🔴 BLOCKING (must fix):
"⚠️ BLOCKING: This has an XSS vulnerability..."

// 🟡 NON-BLOCKING (suggestion):
"💡 Suggestion: This could be optimized with useMemo..."

// 🟢 PRAISE (reinforce good practices):
"✅ Nice! Good error handling here."

The Review Checklist

Quick Scan (2 minutes)

[ ] Does it build?
[ ] Are there any any types?
[ ] Any obvious security issues?
[ ] Tests included?

Deep Review (10-20 minutes)

Type Safety:

[ ] No liberal any usage
[ ] Type assertions justified
[ ] Null checks present
[ ] Proper type narrowing

React Patterns:

[ ] Hooks rules followed
[ ] Dependencies correct
[ ] No unnecessary re-renders
[ ] No state mutation

Performance:

[ ] No expensive operations in render
[ ] Bundle size considered
[ ] List keys proper

Security:

[ ] No XSS vulnerabilities
[ ] No exposed secrets
[ ] Input sanitized

Architecture:

[ ] Components focused
[ ] Logic extracted to hooks
[ ] No prop drilling

Accessibility:

[ ] Semantic HTML
[ ] ARIA labels present
[ ] Keyboard navigation works

Error Handling:

[ ] Promises catch errors
[ ] User sees errors
[ ] Graceful degradation

Final Check

[ ] Would I be comfortable merging this?
[ ] Are my comments constructive?
[ ] Did I explain why, not just what?

Example Review Comments

Excellent Review Comment

⚠️ Type Safety Issue

Using `any` here removes type safety. If the API shape changes, 
we won't catch it until runtime.

Could we define an interface?

interface UserResponse {
  id: string;
  name: string;
  email: string;
}

Then validate with Zod:
const UserSchema = z.object({...});
const user = UserSchema.parse(response);

This catches API changes at the boundary.

Poor Review Comment

This is wrong. Use proper types.

When to Approve vs Request Changes

Approve ✅

Minor suggestions only
No blocking issues
Good code that teaches you something
Meets team standards

Request Changes 🔄

Type safety violations
Security issues
Performance problems
Breaking changes without discussion
Missing critical tests

Comment Only 💬

Suggestions for improvement
Questions for clarity
Praise for good patterns
Learning opportunities

Conclusion

Good code review is about:

Catching bugs before they reach users
Teaching through constructive feedback
Maintaining standards without nitpicking
Building trust with respectful comments

Bad code review is:

Nitpicking style
Enforcing preferences
Being vague
Rewriting everything

The golden rule: Review code like you'd want yours reviewed. Be thorough on what matters. Be kind on everything else.

Focus on:

✅ Type safety
✅ React patterns
✅ Performance
✅ Security
✅ Architecture

Ignore:

❌ Style preferences
❌ Personal taste
❌ Bikeshedding

Your job isn't to make code look like you wrote it. It's to make code better than it was.

What do you look for in code reviews? Share your best review practices!

The Review Priority Pyramid

Category 1: Type Safety (High Priority)

Red Flag: Liberal any Usage

Red Flag: Type Assertions (as) Without Justification

Red Flag: Missing Null/Undefined Checks

Red Flag: Improper Type Narrowing

Category 2: React Patterns (High Priority)

Red Flag: Breaking Rules of Hooks

Red Flag: Missing Dependencies in useEffect/useCallback

Red Flag: Unnecessary Re-renders

Red Flag: State Derived from Props

Red Flag: Mutating State Directly

Category 3: Performance Issues (High Priority)

Red Flag: Expensive Operations in Render

Red Flag: Large Bundle Imports

Red Flag: Inefficient List Rendering

Category 4: Security (High Priority)

Red Flag: XSS Vulnerabilities

Red Flag: Exposing Sensitive Data

Red Flag: Insecure Data Handling

Category 5: Architecture & Design (Medium Priority)

Red Flag: God Components

Red Flag: Prop Drilling

Red Flag: Mixed Concerns

Category 6: Accessibility (Medium Priority)

Red Flag: Missing Semantic HTML

Red Flag: Missing ARIA Labels

Red Flag: Keyboard Navigation Issues

Category 7: Error Handling (Medium Priority)

Red Flag: Unhandled Promise Rejections

Red Flag: Silent Failures

Category 8: Testing (Medium Priority)

Red Flag: Untestable Code

Red Flag: Missing Critical Tests

What NOT to Comment On

❌ Don't Nitpick Style (Use Prettier/ESLint)

❌ Don't Enforce Personal Preferences

❌ Don't Rewrite Code in Comments

The Art of Constructive Feedback

Use Questions, Not Commands

Explain the "Why"

Offer Alternatives

Distinguish Blocking vs Non-Blocking

The Review Checklist

Quick Scan (2 minutes)

Deep Review (10-20 minutes)

Final Check

Example Review Comments

Excellent Review Comment

Poor Review Comment

When to Approve vs Request Changes

Approve ✅

Request Changes 🔄

Comment Only 💬

Conclusion

Red Flag: Liberal `any` Usage

Red Flag: Type Assertions (`as`) Without Justification