Project Init
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
This commit is contained in:
Yaojia Wang
582
.claude/skills/code-reviewer.md
Normal file
582
.claude/skills/code-reviewer.md
Normal file
@@ -0,0 +1,582 @@
|
||||
# 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**.
|
||||
Reference in New Issue
Block a user