# Code Reviewer Skill

This skill ensures all frontend and backend code follows proper coding standards, best practices, and maintains high quality.

## Purpose

Automatically review code for:
- **Coding Standards**: Naming conventions, formatting, structure
- **Best Practices**: Design patterns, error handling, security
- **Code Quality**: Readability, maintainability, performance
- **Common Issues**: Anti-patterns, code smells, potential bugs

## When to Use

This skill is automatically applied when:
- Backend agent generates code
- Frontend agent generates code
- Any code modifications are proposed
- Code refactoring is performed

## Review Checklist

### Backend Code (TypeScript/NestJS)

#### 1. Naming Conventions
```typescript
// ✅ CORRECT
export class UserService {
  async getUserById(userId: string): Promise<User> { }
}

const MAX_RETRY_ATTEMPTS = 3;

// ❌ INCORRECT
export class userservice {
  async getuser(id) { }
}

const max_retry = 3;
```

**Rules**:
- Classes: `PascalCase`
- Functions/variables: `camelCase`
- Constants: `UPPER_SNAKE_CASE`
- Files: `kebab-case.ts`
- Interfaces: `IPascalCase` or `PascalCase`

#### 2. TypeScript Best Practices
```typescript
// ✅ CORRECT: Strong typing
interface CreateUserDto {
  email: string;
  name: string;
  age?: number;
}

async function createUser(dto: CreateUserDto): Promise<User> {
  // Implementation
}

// ❌ INCORRECT: Using 'any'
async function createUser(dto: any): Promise<any> {
  // Don't use 'any'
}
```

**Rules**:
- ❌ Never use `any` type
- ✅ Use proper interfaces/types
- ✅ Use `readonly` where appropriate
- ✅ Use generics for reusable code

#### 3. Error Handling
```typescript
// ✅ CORRECT: Proper error handling
export class IssueService {
  async getIssueById(id: string): Promise<Issue> {
    try {
      const issue = await this.issueRepository.findOne({ where: { id } });

      if (!issue) {
        throw new NotFoundException(`Issue not found: ${id}`);
      }

      return issue;
    } catch (error) {
      this.logger.error(`Failed to get issue ${id}`, error);
      throw error;
    }
  }
}

// ❌ INCORRECT: Silent failures
async getIssueById(id: string) {
  const issue = await this.issueRepository.findOne({ where: { id } });
  return issue; // Returns null/undefined without error
}
```

**Rules**:
- ✅ Use custom error classes
- ✅ Log errors with context
- ✅ Throw descriptive errors
- ❌ Don't swallow errors silently
- ✅ Use try-catch for async operations

#### 4. Dependency Injection (NestJS)
```typescript
// ✅ CORRECT: Constructor injection
@Injectable()
export class IssueService {
  constructor(
    @InjectRepository(Issue)
    private readonly issueRepository: Repository<Issue>,
    private readonly auditService: AuditService,
    private readonly logger: Logger,
  ) {}
}

// ❌ INCORRECT: Direct instantiation
export class IssueService {
  private issueRepository = new IssueRepository();
  private auditService = new AuditService();
}
```

**Rules**:
- ✅ Use constructor injection
- ✅ Mark dependencies as `private readonly`
- ✅ Use `@Injectable()` decorator
- ❌ Don't create instances manually

#### 5. Database Operations
```typescript
// ✅ CORRECT: Parameterized queries, proper error handling
async findByEmail(email: string): Promise<User | null> {
  return this.userRepository.findOne({
    where: { email },
    select: ['id', 'email', 'name'] // Only select needed fields
  });
}

// ❌ INCORRECT: SQL injection risk, selecting all fields
async findByEmail(email: string) {
  return this.connection.query(`SELECT * FROM users WHERE email = '${email}'`);
}
```

**Rules**:
- ✅ Use ORM (TypeORM/Prisma)
- ✅ Parameterized queries only
- ✅ Select only needed fields
- ✅ Use transactions for multi-step operations
- ❌ Never concatenate SQL strings

#### 6. Service Layer Structure
```typescript
// ✅ CORRECT: Clean service structure
@Injectable()
export class IssueService {
  constructor(
    private readonly issueRepository: IssueRepository,
    private readonly auditService: AuditService,
  ) {}

  // Public API
  async create(dto: CreateIssueDto, userId: string): Promise<Issue> {
    const validated = this.validateDto(dto);
    const issue = await this.createIssue(validated, userId);
    await this.logAudit(issue, userId);
    return issue;
  }

  // Private helper methods
  private validateDto(dto: CreateIssueDto): CreateIssueDto {
    // Validation logic
    return dto;
  }

  private async createIssue(dto: CreateIssueDto, userId: string): Promise<Issue> {
    // Creation logic
  }

  private async logAudit(issue: Issue, userId: string): Promise<void> {
    // Audit logging
  }
}
```

**Rules**:
- ✅ Single Responsibility Principle
- ✅ Public methods for API, private for helpers
- ✅ Keep methods small and focused
- ✅ Extract complex logic to helper methods

### Frontend Code (React/TypeScript)

#### 1. Component Structure
```typescript
// ✅ CORRECT: Functional component with TypeScript
import { FC, useState, useEffect } from 'react';
import styles from './IssueCard.module.css';

interface IssueCardProps {
  issueId: string;
  onUpdate?: (issue: Issue) => void;
}

export const IssueCard: FC<IssueCardProps> = ({ issueId, onUpdate }) => {
  const [issue, setIssue] = useState<Issue | null>(null);
  const [loading, setLoading] = useState(false);
  const [error, setError] = useState<string | null>(null);

  useEffect(() => {
    fetchIssue();
  }, [issueId]);

  const fetchIssue = async () => {
    // Implementation
  };

  if (loading) return <LoadingSpinner />;
  if (error) return <ErrorMessage message={error} />;
  if (!issue) return null;

  return (
    <div className={styles.card}>
      <h3>{issue.title}</h3>
    </div>
  );
};

// ❌ INCORRECT: Class component, no types
export default function issuecard(props) {
  const [data, setdata] = useState();

  useEffect(() => {
    fetch('/api/issues/' + props.id)
      .then(r => r.json())
      .then(d => setdata(d));
  });

  return <div>{data?.title}</div>;
}
```

**Rules**:
- ✅ Use functional components with hooks
- ✅ Define prop interfaces
- ✅ Use `FC<Props>` type
- ✅ Handle loading/error states
- ✅ Use CSS modules or styled-components
- ❌ Don't use default exports
- ❌ Don't use inline styles (except dynamic)

#### 2. State Management
```typescript
// ✅ CORRECT: Zustand store with TypeScript
import { create } from 'zustand';

interface ProjectStore {
  projects: Project[];
  loading: boolean;
  error: string | null;

  // Actions
  fetchProjects: () => Promise<void>;
  addProject: (project: Project) => void;
}

export const useProjectStore = create<ProjectStore>((set, get) => ({
  projects: [],
  loading: false,
  error: null,

  fetchProjects: async () => {
    set({ loading: true, error: null });
    try {
      const projects = await ProjectService.getAll();
      set({ projects, loading: false });
    } catch (error) {
      set({
        error: error instanceof Error ? error.message : 'Unknown error',
        loading: false
      });
    }
  },

  addProject: (project) => {
    set(state => ({ projects: [...state.projects, project] }));
  },
}));

// ❌ INCORRECT: No types, mutating state
const useProjectStore = create((set) => ({
  projects: [],
  addProject: (project) => {
    set(state => {
      state.projects.push(project); // Mutation!
      return state;
    });
  },
}));
```

**Rules**:
- ✅ Define store interface
- ✅ Immutable updates
- ✅ Handle loading/error states
- ✅ Use TypeScript
- ❌ Don't mutate state directly

#### 3. Custom Hooks
```typescript
// ✅ CORRECT: Proper custom hook
import { useState, useEffect } from 'react';

interface UseIssueResult {
  issue: Issue | null;
  loading: boolean;
  error: string | null;
  refetch: () => Promise<void>;
}

export const useIssue = (issueId: string): UseIssueResult => {
  const [issue, setIssue] = useState<Issue | null>(null);
  const [loading, setLoading] = useState(false);
  const [error, setError] = useState<string | null>(null);

  const fetchIssue = async () => {
    try {
      setLoading(true);
      setError(null);
      const data = await IssueService.getById(issueId);
      setIssue(data);
    } catch (err) {
      setError(err instanceof Error ? err.message : 'Failed to fetch');
    } finally {
      setLoading(false);
    }
  };

  useEffect(() => {
    fetchIssue();
  }, [issueId]);

  return { issue, loading, error, refetch: fetchIssue };
};

// Usage
const { issue, loading, error, refetch } = useIssue('123');

// ❌ INCORRECT: No error handling, no types
function useIssue(id) {
  const [data, setData] = useState();

  useEffect(() => {
    fetch(`/api/issues/${id}`)
      .then(r => r.json())
      .then(setData);
  }, [id]);

  return data;
}
```

**Rules**:
- ✅ Name starts with "use"
- ✅ Return object with named properties
- ✅ Include loading/error states
- ✅ Provide refetch capability
- ✅ Define return type interface

#### 4. Event Handlers
```typescript
// ✅ CORRECT: Typed event handlers
import { ChangeEvent, FormEvent } from 'react';

export const IssueForm: FC = () => {
  const [title, setTitle] = useState('');

  const handleTitleChange = (e: ChangeEvent<HTMLInputElement>) => {
    setTitle(e.target.value);
  };

  const handleSubmit = async (e: FormEvent<HTMLFormElement>) => {
    e.preventDefault();
    await createIssue({ title });
  };

  return (
    <form onSubmit={handleSubmit}>
      <input
        value={title}
        onChange={handleTitleChange}
      />
      <button type="submit">Create</button>
    </form>
  );
};

// ❌ INCORRECT: No types, inline functions
export const IssueForm = () => {
  const [title, setTitle] = useState('');

  return (
    <form onSubmit={(e) => {
      e.preventDefault();
      createIssue({ title });
    }}>
      <input
        value={title}
        onChange={(e) => setTitle(e.target.value)}
      />
    </form>
  );
};
```

**Rules**:
- ✅ Type event parameters
- ✅ Extract handlers to named functions
- ✅ Use `preventDefault()` for forms
- ❌ Avoid inline arrow functions in JSX (performance)

#### 5. Performance Optimization
```typescript
// ✅ CORRECT: Memoization
import { memo, useMemo, useCallback } from 'react';

export const IssueCard = memo<IssueCardProps>(({ issue, onUpdate }) => {
  const formattedDate = useMemo(
    () => new Date(issue.createdAt).toLocaleDateString(),
    [issue.createdAt]
  );

  const handleClick = useCallback(() => {
    onUpdate?.(issue);
  }, [issue, onUpdate]);

  return (
    <div onClick={handleClick}>
      <h3>{issue.title}</h3>
      <span>{formattedDate}</span>
    </div>
  );
});

// ❌ INCORRECT: Re-computing on every render
export const IssueCard = ({ issue, onUpdate }) => {
  const formattedDate = new Date(issue.createdAt).toLocaleDateString();

  return (
    <div onClick={() => onUpdate(issue)}>
      <h3>{issue.title}</h3>
      <span>{formattedDate}</span>
    </div>
  );
};
```

**Rules**:
- ✅ Use `memo` for expensive components
- ✅ Use `useMemo` for expensive computations
- ✅ Use `useCallback` for callback props
- ❌ Don't over-optimize (measure first)

## Common Anti-Patterns to Avoid

### Backend

❌ **God Classes**: Classes with too many responsibilities
❌ **Magic Numbers**: Use named constants instead
❌ **Callback Hell**: Use async/await
❌ **N+1 Queries**: Use eager loading or joins
❌ **Ignoring Errors**: Always handle errors
❌ **Hardcoded Values**: Use config/environment variables

### Frontend

❌ **Prop Drilling**: Use Context or state management
❌ **Inline Styles**: Use CSS modules or styled-components
❌ **Large Components**: Break into smaller components
❌ **Missing Keys**: Always provide keys in lists
❌ **Premature Optimization**: Measure before optimizing
❌ **Missing Error Boundaries**: Wrap components with error boundaries

## Security Checklist

### Backend
- [ ] Validate all input
- [ ] Sanitize user data
- [ ] Use parameterized queries
- [ ] Implement authentication/authorization
- [ ] Hash passwords (bcrypt)
- [ ] Use HTTPS
- [ ] Set security headers
- [ ] Rate limiting on APIs
- [ ] Log security events

### Frontend
- [ ] Sanitize user input (XSS prevention)
- [ ] Validate before sending to backend
- [ ] Secure token storage (httpOnly cookies)
- [ ] CSRF protection
- [ ] Content Security Policy
- [ ] No sensitive data in localStorage
- [ ] Validate API responses

## Code Review Process

When reviewing code:

1. **First Pass: Architecture & Design**
   - Does it follow SOLID principles?
   - Is the structure logical?
   - Are there clear separation of concerns?

2. **Second Pass: Implementation**
   - Correct naming conventions?
   - Proper error handling?
   - Type safety?
   - Performance considerations?

3. **Third Pass: Testing & Edge Cases**
   - Are there unit tests?
   - Edge cases handled?
   - Error scenarios covered?

4. **Fourth Pass: Security & Best Practices**
   - Any security vulnerabilities?
   - Follows best practices?
   - Documentation adequate?

## Automated Checks

Recommended tools:
- **Backend**: ESLint, Prettier, TypeScript compiler
- **Frontend**: ESLint, Prettier, TypeScript compiler
- **Both**: Husky (pre-commit hooks), SonarQube

## Review Feedback Format

```markdown
## Code Review: [File Name]

### ✅ Good Practices
- [What was done well]

### ⚠️ Issues Found

#### Critical (Must Fix)
- [ ] Issue 1: [Description]
  - Location: `file.ts:123`
  - Fix: [Suggested fix]

#### Moderate (Should Fix)
- [ ] Issue 2: [Description]

#### Minor (Consider)
- [ ] Issue 3: [Description]

### 💡 Suggestions
- [Improvement suggestions]

### Overall Rating: [Approved / Needs Changes / Rejected]
```

## Usage

This skill is automatically invoked by the main coordinator whenever backend or frontend agents generate code. The coordinator will:

1. Receive code from agent
2. Apply code-reviewer skill
3. Report any issues found
4. Request fixes if needed
5. Approve once standards are met

---

**Remember**: The goal is not perfection, but **maintainability, reliability, and consistency**.