Comprehensive code review knowledge including security, performance, accessibility, and quality standards across multiple languages and frameworks
This skill inherits all available tools. When active, it can use any tool Claude has access to.
Comprehensive code review knowledge base for systematic evaluation of code changes across security, performance, accessibility, quality, and testing dimensions.
This skill provides structured review patterns, checklists, and best practices for conducting thorough code reviews. It covers multiple programming languages, frameworks, and architectural patterns with a focus on actionable feedback.
Authentication & Authorization
Input Validation
Data Protection
Common Vulnerabilities (OWASP Top 10)
Good:
// Parameterized queries prevent SQL injection
const user = await db.query(
'SELECT * FROM users WHERE email = $1',
[userEmail]
);
// Proper password hashing
const hashedPassword = await bcrypt.hash(password, 12);
// Secure session configuration
app.use(session({
secret: process.env.SESSION_SECRET,
resave: false,
saveUninitialized: false,
cookie: {
httpOnly: true,
secure: true, // HTTPS only
sameSite: 'strict',
maxAge: 3600000
}
}));
Bad:
// SQL injection vulnerability
const user = await db.query(
`SELECT * FROM users WHERE email = '${userEmail}'`
);
// Weak password hashing
const hashedPassword = crypto.createHash('md5').update(password).digest('hex');
// Insecure session
app.use(session({
secret: 'hardcoded-secret',
cookie: { secure: false }
}));
Database & Queries
Frontend Performance
API & Network
Memory Management
Bad - N+1 Query:
// Loads users, then queries each user's posts separately
const users = await User.findAll();
for (const user of users) {
user.posts = await Post.findAll({ where: { userId: user.id } });
}
Good - Eager Loading:
// Single query with JOIN
const users = await User.findAll({
include: [{ model: Post }]
});
Bad - No Memoization:
function ExpensiveComponent({ data }) {
// Recalculates on every render
const processed = expensiveOperation(data);
return <div>{processed}</div>;
}
Good - With Memoization:
function ExpensiveComponent({ data }) {
const processed = useMemo(
() => expensiveOperation(data),
[data]
);
return <div>{processed}</div>;
}
Semantic HTML
Keyboard Navigation
Screen Reader Support
Color & Contrast
Forms & Inputs
Good:
// Proper form accessibility
<form>
<label htmlFor="email">
Email Address <span aria-label="required">*</span>
</label>
<input
id="email"
type="email"
required
aria-required="true"
aria-describedby="email-error"
autoComplete="email"
/>
<span id="email-error" role="alert">
{errors.email}
</span>
</form>
// Accessible modal
<div
role="dialog"
aria-modal="true"
aria-labelledby="dialog-title"
aria-describedby="dialog-description"
>
<h2 id="dialog-title">Confirm Action</h2>
<p id="dialog-description">Are you sure?</p>
<button onClick={handleConfirm}>Confirm</button>
<button onClick={handleCancel}>Cancel</button>
</div>
Bad:
// Poor accessibility
<div onClick={handleClick}>Click me</div> // Should be button
<img src="photo.jpg" /> // Missing alt text
<input placeholder="Email" /> // Placeholder not a label
<div class="error">{error}</div> // Not associated with input
Readability
Maintainability
Error Handling
Code Comments
Good:
// Clear naming and single responsibility
const MAX_LOGIN_ATTEMPTS = 3;
const LOCKOUT_DURATION_MS = 15 * 60 * 1000; // 15 minutes
async function validateUserCredentials(
email: string,
password: string
): Promise<User | null> {
const user = await findUserByEmail(email);
if (!user) {
return null;
}
if (isAccountLocked(user)) {
throw new AccountLockedError(user.lockoutUntil);
}
const isValid = await verifyPassword(password, user.passwordHash);
if (!isValid) {
await incrementFailedAttempts(user);
return null;
}
await resetFailedAttempts(user);
return user;
}
Bad:
// Poor naming, multiple responsibilities, magic numbers
async function check(e: string, p: string) {
const u = await db.query('SELECT * FROM users WHERE email = ?', [e]);
if (!u) return null;
if (u.fa > 3 && Date.now() - u.lat < 900000) throw new Error('locked');
const ok = await bcrypt.compare(p, u.ph);
if (!ok) {
await db.query('UPDATE users SET fa = fa + 1, lat = ? WHERE id = ?', [Date.now(), u.id]);
return null;
}
await db.query('UPDATE users SET fa = 0 WHERE id = ?', [u.id]);
return u;
}
Unit Tests
Integration Tests
E2E Tests
Test Quality
Good Unit Test:
describe('UserService.validateCredentials', () => {
it('should return user when credentials are valid', async () => {
// Arrange
const email = 'user@example.com';
const password = 'SecurePass123!';
const mockUser = { id: 1, email, passwordHash: 'hash' };
jest.spyOn(userRepo, 'findByEmail').mockResolvedValue(mockUser);
jest.spyOn(bcrypt, 'compare').mockResolvedValue(true);
// Act
const result = await userService.validateCredentials(email, password);
// Assert
expect(result).toEqual(mockUser);
expect(userRepo.findByEmail).toHaveBeenCalledWith(email);
});
it('should throw AccountLockedError when account is locked', async () => {
// Arrange
const email = 'locked@example.com';
const password = 'password';
const lockoutUntil = new Date(Date.now() + 900000);
const mockUser = {
id: 2,
email,
failedAttempts: 3,
lockoutUntil
};
jest.spyOn(userRepo, 'findByEmail').mockResolvedValue(mockUser);
// Act & Assert
await expect(
userService.validateCredentials(email, password)
).rejects.toThrow(AccountLockedError);
});
});
Code Documentation
README Documentation
API Documentation
Architecture Documentation
Type Safety
any types (use unknown if needed)Good:
// Proper typing with generics
interface ApiResponse<T> {
data: T;
status: number;
error?: string;
}
async function fetchData<T>(url: string): Promise<ApiResponse<T>> {
try {
const response = await fetch(url);
const data = await response.json() as T;
return { data, status: response.status };
} catch (error) {
return {
data: {} as T,
status: 500,
error: error instanceof Error ? error.message : 'Unknown error'
};
}
}
// Type guards
function isUser(obj: unknown): obj is User {
return (
typeof obj === 'object' &&
obj !== null &&
'id' in obj &&
'email' in obj
);
}
Good:
// Destructuring and default values
function createUser({
email,
name,
role = 'user',
active = true
}: CreateUserInput) {
return { email, name, role, active };
}
// Optional chaining and nullish coalescing
const userName = user?.profile?.name ?? 'Anonymous';
// Async/await over promise chains
async function processData() {
try {
const data = await fetchData();
const processed = await transform(data);
await save(processed);
} catch (error) {
logger.error('Processing failed', error);
throw error;
}
}
Component Structure
Good:
interface UserListProps {
users: User[];
onUserSelect: (userId: string) => void;
}
const UserList: React.FC<UserListProps> = ({ users, onUserSelect }) => {
const [searchTerm, setSearchTerm] = useState('');
const filteredUsers = useMemo(
() => users.filter(u => u.name.includes(searchTerm)),
[users, searchTerm]
);
const handleSelect = useCallback(
(userId: string) => {
onUserSelect(userId);
},
[onUserSelect]
);
return (
<div>
<input
value={searchTerm}
onChange={(e) => setSearchTerm(e.target.value)}
placeholder="Search users..."
/>
<ul>
{filteredUsers.map(user => (
<li key={user.id}>
<button onClick={() => handleSelect(user.id)}>
{user.name}
</button>
</li>
))}
</ul>
</div>
);
};
Bad:
// Multiple issues
function UserList({ users, onUserSelect }) { // No types
const [searchTerm, setSearchTerm] = useState('');
// No memoization - filters on every render
const filteredUsers = users.filter(u => u.name.includes(searchTerm));
return (
<div>
<ul>
{filteredUsers.map(user => ( // No key
<li>
{/* Inline function creates new reference every render */}
<button onClick={() => onUserSelect(user.id)}>
{user.name}
</button>
</li>
))}
</ul>
</div>
);
}
Server Setup
Good:
import express from 'express';
import helmet from 'helmet';
import rateLimit from 'express-rate-limit';
const app = express();
// Security middleware
app.use(helmet());
app.use(express.json({ limit: '10mb' }));
// Rate limiting
const limiter = rateLimit({
windowMs: 15 * 60 * 1000,
max: 100
});
app.use('/api/', limiter);
// Error handling
app.use((err: Error, req: Request, res: Response, next: NextFunction) => {
logger.error('Unhandled error', { error: err, path: req.path });
res.status(500).json({ error: 'Internal server error' });
});
// Graceful shutdown
process.on('SIGTERM', async () => {
logger.info('SIGTERM received, closing server...');
await server.close();
await db.disconnect();
process.exit(0);
});
Python Best Practices
Good:
from dataclasses import dataclass
from typing import List, Optional
from contextlib import contextmanager
@dataclass
class User:
id: int
email: str
name: str
role: str = 'user'
def validate_email(email: str) -> bool:
"""Validate email format using regex."""
pattern = r'^[a-zA-Z0-9._%+-]+@[a-zA-Z0-9.-]+\.[a-zA-Z]{2,}$'
return re.match(pattern, email) is not None
def filter_active_users(users: List[User]) -> List[User]:
"""Return only active users using list comprehension."""
return [user for user in users if user.active]
@contextmanager
def database_connection(conn_string: str):
"""Context manager for database connections."""
conn = create_connection(conn_string)
try:
yield conn
finally:
conn.close()
SQL Best Practices
Good:
-- Proper indexing and pagination
CREATE INDEX idx_users_email ON users(email);
CREATE INDEX idx_posts_user_created ON posts(user_id, created_at);
-- Efficient query with proper joins
SELECT
u.id,
u.name,
u.email,
COUNT(p.id) as post_count
FROM users u
LEFT JOIN posts p ON u.id = p.user_id
WHERE u.active = true
GROUP BY u.id, u.name, u.email
ORDER BY u.created_at DESC
LIMIT 20 OFFSET 0;
-- Transaction for consistency
BEGIN;
UPDATE accounts SET balance = balance - 100 WHERE id = 1;
UPDATE accounts SET balance = balance + 100 WHERE id = 2;
COMMIT;
## Files Changed Analysis
### High Risk Files (require thorough review)
- Authentication/authorization code
- Database migrations
- Security configurations
- Payment processing
- Data encryption
### Medium Risk Files
- Business logic
- API endpoints
- Database queries
- External integrations
### Low Risk Files
- UI components
- Tests
- Documentation
- Configuration files
Questions to Ask:
## 🔴 Critical Issues (Must Fix)
- [ ] **Security**: [Description]
```suggestion
[Suggested fix]
[Suggested fix]
[Suggested fix]
### 4. Approval Criteria
**Approve when:**
- [ ] No critical security issues
- [ ] All tests passing
- [ ] Code coverage maintained/improved
- [ ] Documentation updated
- [ ] No performance regressions
- [ ] Accessibility requirements met
- [ ] Code follows style guide
- [ ] Changes are backward compatible (or migration plan exists)
**Request changes when:**
- Critical security vulnerabilities
- Failing tests
- Missing test coverage
- Breaking changes without migration
- Performance regressions
- Accessibility violations
---
## Common Issues Reference
### Security Issues
- Hardcoded secrets
- SQL injection vulnerabilities
- Missing authentication checks
- Insecure random number generation
- Sensitive data in logs
- Missing CSRF protection
### Performance Issues
- N+1 queries
- Missing database indexes
- Unnecessary re-renders (React)
- Large bundle sizes
- Synchronous operations blocking event loop
- Memory leaks
### Code Smells
- Functions > 50 lines
- Deeply nested conditionals (> 3 levels)
- Duplicate code
- Magic numbers
- Poor naming
- God objects
### Missing Error Handling
- Unhandled promise rejections
- Missing try/catch blocks
- No input validation
- Silent failures
- Generic error messages
### Incomplete Tests
- Missing edge cases
- No error scenario tests
- Flaky tests
- No integration tests
- Missing accessibility tests
---
## Review Workflow
1. **Initial Scan** (2 minutes)
- Review PR description
- Check file changes count
- Identify high-risk files
2. **Deep Dive** (15-30 minutes)
- Review each file systematically
- Apply category checklists
- Note issues with severity
3. **Testing Verification** (5 minutes)
- Check test coverage
- Verify tests are passing
- Review test quality
4. **Documentation Check** (3 minutes)
- Verify docs are updated
- Check for breaking changes
- Ensure examples are current
5. **Feedback Generation** (5 minutes)
- Organize comments by severity
- Provide code suggestions
- Add positive feedback
6. **Decision** (1 minute)
- Approve, request changes, or comment
- Set follow-up items
---
## Tools Integration
### Automated Checks
- **ESLint/Prettier**: Code style
- **SonarQube**: Code quality metrics
- **Snyk/Dependabot**: Security vulnerabilities
- **Jest/Vitest**: Test coverage
- **Lighthouse**: Performance/accessibility
- **TypeScript**: Type safety
### Manual Review Focus
- Business logic correctness
- Architecture decisions
- Security implications
- User experience impact
- Code maintainability
---
## Conclusion
Effective code reviews balance thoroughness with efficiency. Use this skill as a checklist to ensure consistent, high-quality reviews that catch critical issues while fostering team learning and code improvement.
**Key Principles:**
- Security first
- Performance matters
- Accessibility is non-negotiable
- Quality over speed
- Constructive feedback
- Continuous improvement