📦 deps(thirdparty): update snapshots
This commit is contained in:
@@ -0,0 +1,944 @@
|
||||
# Fresh Eyes Review Checklist Reference
|
||||
|
||||
Extended details, examples, and severity guidance for each checklist category.
|
||||
|
||||
## Type Safety
|
||||
|
||||
### What to Check
|
||||
|
||||
**No `any` types without justification:**
|
||||
|
||||
```typescript
|
||||
// ◆◆ Severe
|
||||
function process(data: any) { ... }
|
||||
|
||||
// ✓ Good - with justification
|
||||
// @ts-expect-error: External library has incorrect types
|
||||
function process(data: any) { ... }
|
||||
|
||||
// ✓ Better - narrow the type
|
||||
function process(data: unknown) {
|
||||
if (typeof data === 'string') { ... }
|
||||
}
|
||||
```
|
||||
|
||||
**Null/undefined handling:**
|
||||
|
||||
```typescript
|
||||
// ◆ Moderate
|
||||
const user = users.find(u => u.id === id);
|
||||
return user.name; // might be undefined
|
||||
|
||||
// ✓ Good
|
||||
const user = users.find(u => u.id === id);
|
||||
return user?.name ?? 'Unknown';
|
||||
```
|
||||
|
||||
**Type guards for unions:**
|
||||
|
||||
```typescript
|
||||
// ◆ Moderate
|
||||
type Result = Success | Error;
|
||||
function handle(result: Result) {
|
||||
if (result.success) { // Property doesn't exist
|
||||
return result.data;
|
||||
}
|
||||
}
|
||||
|
||||
// ✓ Good - discriminated union
|
||||
type Result =
|
||||
| { success: true; data: string }
|
||||
| { success: false; error: string };
|
||||
|
||||
function handle(result: Result) {
|
||||
if (result.success) {
|
||||
return result.data; // TypeScript knows this is safe
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
**Generic constraints:**
|
||||
|
||||
```typescript
|
||||
// ◇ Minor
|
||||
function getProperty<T>(obj: T, key: string) {
|
||||
return obj[key]; // No constraint, unsafe
|
||||
}
|
||||
|
||||
// ✓ Good
|
||||
function getProperty<T, K extends keyof T>(obj: T, key: K): T[K] {
|
||||
return obj[key];
|
||||
}
|
||||
```
|
||||
|
||||
**Return types on public functions:**
|
||||
|
||||
```typescript
|
||||
// ◇ Minor
|
||||
export function calculate(x: number, y: number) {
|
||||
return x + y; // Inferred, but implicit
|
||||
}
|
||||
|
||||
// ✓ Good - explicit contract
|
||||
export function calculate(x: number, y: number): number {
|
||||
return x + y;
|
||||
}
|
||||
```
|
||||
|
||||
### Severity Guidance
|
||||
|
||||
- **◆◆** `any` escaping to public API, type assertions that can crash
|
||||
- **◆** Missing null checks, missing return types on public functions
|
||||
- **◇** Overly broad types that work but reduce safety
|
||||
|
||||
---
|
||||
|
||||
## Error Handling
|
||||
|
||||
### What to Check
|
||||
|
||||
**All error paths handled:**
|
||||
|
||||
```typescript
|
||||
// ◆◆ Severe - silent failure
|
||||
async function saveUser(user: User) {
|
||||
await db.insert(user); // Might throw, not handled
|
||||
}
|
||||
|
||||
// ✓ Good
|
||||
async function saveUser(user: User): Promise<Result<void, DbError>> {
|
||||
try {
|
||||
await db.insert(user);
|
||||
return { success: true };
|
||||
} catch (error) {
|
||||
logger.error('Failed to save user', { user, error });
|
||||
return { success: false, error: new DbError('Insert failed') };
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
**Meaningful error messages:**
|
||||
|
||||
```typescript
|
||||
// ◆ Moderate
|
||||
if (!user) throw new Error('Invalid');
|
||||
|
||||
// ✓ Good
|
||||
if (!user) {
|
||||
throw new Error(`User not found: id=${userId}, searched at ${new Date()}`);
|
||||
}
|
||||
```
|
||||
|
||||
**Promise rejection handling:**
|
||||
|
||||
```typescript
|
||||
// ◆◆ Severe
|
||||
fetchData().then(process); // Unhandled rejection
|
||||
|
||||
// ✓ Good
|
||||
fetchData()
|
||||
.then(process)
|
||||
.catch(error => {
|
||||
logger.error('Fetch failed', { error });
|
||||
notifyUser('Data unavailable');
|
||||
});
|
||||
|
||||
// ✓ Better - async/await
|
||||
try {
|
||||
const data = await fetchData();
|
||||
process(data);
|
||||
} catch (error) {
|
||||
logger.error('Fetch failed', { error });
|
||||
notifyUser('Data unavailable');
|
||||
}
|
||||
```
|
||||
|
||||
**Resource cleanup:**
|
||||
|
||||
```typescript
|
||||
// ◆ Moderate
|
||||
const file = await fs.open('data.txt');
|
||||
const content = await file.readFile(); // Might throw, file not closed
|
||||
return content;
|
||||
|
||||
// ✓ Good
|
||||
const file = await fs.open('data.txt');
|
||||
try {
|
||||
return await file.readFile();
|
||||
} finally {
|
||||
await file.close();
|
||||
}
|
||||
```
|
||||
|
||||
### Severity Guidance
|
||||
|
||||
- **◆◆** Silent failures, unhandled rejections, resource leaks
|
||||
- **◆** Poor error messages, missing try/catch, errors swallowed
|
||||
- **◇** Error messages could be more helpful, missing context
|
||||
|
||||
---
|
||||
|
||||
## Security
|
||||
|
||||
### What to Check
|
||||
|
||||
**Input validation:**
|
||||
|
||||
```typescript
|
||||
// ◆◆ Severe
|
||||
app.get('/user/:id', (req, res) => {
|
||||
const user = db.query(`SELECT * FROM users WHERE id = ${req.params.id}`);
|
||||
res.json(user);
|
||||
});
|
||||
|
||||
// ✓ Good
|
||||
app.get('/user/:id', (req, res) => {
|
||||
const id = parseInt(req.params.id, 10);
|
||||
if (isNaN(id) || id < 0) {
|
||||
return res.status(400).json({ error: 'Invalid user ID' });
|
||||
}
|
||||
const user = db.query('SELECT * FROM users WHERE id = ?', [id]);
|
||||
res.json(user);
|
||||
});
|
||||
```
|
||||
|
||||
**No hardcoded secrets:**
|
||||
|
||||
```typescript
|
||||
// ◆◆ Severe
|
||||
const API_KEY = 'sk_live_abc123xyz789';
|
||||
|
||||
// ✓ Good
|
||||
const API_KEY = process.env.API_KEY;
|
||||
if (!API_KEY) throw new Error('API_KEY not configured');
|
||||
```
|
||||
|
||||
**Authentication checks:**
|
||||
|
||||
```typescript
|
||||
// ◆◆ Severe
|
||||
app.delete('/admin/users/:id', (req, res) => {
|
||||
db.deleteUser(req.params.id);
|
||||
res.json({ success: true });
|
||||
});
|
||||
|
||||
// ✓ Good
|
||||
app.delete('/admin/users/:id', requireAuth, requireAdmin, (req, res) => {
|
||||
db.deleteUser(req.params.id);
|
||||
res.json({ success: true });
|
||||
});
|
||||
```
|
||||
|
||||
**XSS prevention:**
|
||||
|
||||
```typescript
|
||||
// ◆◆ Severe
|
||||
function displayMessage(msg: string) {
|
||||
element.innerHTML = msg; // User content directly in HTML
|
||||
}
|
||||
|
||||
// ✓ Good
|
||||
function displayMessage(msg: string) {
|
||||
element.textContent = msg; // Automatically escaped
|
||||
}
|
||||
|
||||
// Or use sanitization library
|
||||
import DOMPurify from 'dompurify';
|
||||
element.innerHTML = DOMPurify.sanitize(msg);
|
||||
```
|
||||
|
||||
**Password handling:**
|
||||
|
||||
```typescript
|
||||
// ◆◆ Severe
|
||||
if (user.password === inputPassword) { ... }
|
||||
|
||||
// ✓ Good
|
||||
import bcrypt from 'bcrypt';
|
||||
if (await bcrypt.compare(inputPassword, user.passwordHash)) { ... }
|
||||
```
|
||||
|
||||
### Severity Guidance
|
||||
|
||||
- **◆◆** SQL injection, XSS, auth bypass, exposed secrets, timing attacks
|
||||
- **◆** Missing input validation, weak password checks, missing CSRF
|
||||
- **◇** Could use stronger encryption, missing rate limits
|
||||
|
||||
---
|
||||
|
||||
## Testing
|
||||
|
||||
### What to Check
|
||||
|
||||
**Tests exist:**
|
||||
|
||||
```typescript
|
||||
// ◆ Moderate - new function, no tests
|
||||
export function calculateDiscount(price: number, tier: string): number {
|
||||
if (tier === 'premium') return price * 0.8;
|
||||
if (tier === 'standard') return price * 0.9;
|
||||
return price;
|
||||
}
|
||||
```
|
||||
|
||||
**Edge cases covered:**
|
||||
|
||||
```typescript
|
||||
// ◆ Moderate - only happy path tested
|
||||
test('calculateDiscount applies premium discount', () => {
|
||||
expect(calculateDiscount(100, 'premium')).toBe(80);
|
||||
});
|
||||
|
||||
// ✓ Good - edge cases included
|
||||
test('calculateDiscount handles edge cases', () => {
|
||||
expect(calculateDiscount(100, 'premium')).toBe(80);
|
||||
expect(calculateDiscount(100, 'standard')).toBe(90);
|
||||
expect(calculateDiscount(100, 'unknown')).toBe(100);
|
||||
expect(calculateDiscount(0, 'premium')).toBe(0);
|
||||
expect(calculateDiscount(-10, 'premium')).toBe(-8); // Or throw?
|
||||
});
|
||||
```
|
||||
|
||||
**Actual assertions:**
|
||||
|
||||
```typescript
|
||||
// ◆ Moderate - test doesn't verify behavior
|
||||
test('user creation', async () => {
|
||||
await createUser({ name: 'Alice' }); // No assertion!
|
||||
});
|
||||
|
||||
// ✓ Good
|
||||
test('user creation', async () => {
|
||||
const user = await createUser({ name: 'Alice' });
|
||||
expect(user.name).toBe('Alice');
|
||||
expect(user.id).toBeDefined();
|
||||
|
||||
const fromDb = await db.getUser(user.id);
|
||||
expect(fromDb).toEqual(user);
|
||||
});
|
||||
```
|
||||
|
||||
**No test pollution:**
|
||||
|
||||
```typescript
|
||||
// ◆ Moderate
|
||||
test('first test', () => {
|
||||
globalState.users = [testUser];
|
||||
expect(findUser(1)).toEqual(testUser);
|
||||
});
|
||||
|
||||
test('second test', () => {
|
||||
// Fails if first test didn't run or ran differently
|
||||
expect(globalState.users.length).toBe(1);
|
||||
});
|
||||
|
||||
// ✓ Good
|
||||
beforeEach(() => {
|
||||
globalState.users = [];
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
globalState.users = [];
|
||||
});
|
||||
|
||||
test('first test', () => {
|
||||
globalState.users = [testUser];
|
||||
expect(findUser(1)).toEqual(testUser);
|
||||
});
|
||||
|
||||
test('second test', () => {
|
||||
expect(globalState.users.length).toBe(0); // Clean slate
|
||||
});
|
||||
```
|
||||
|
||||
**Error scenarios tested:**
|
||||
|
||||
```typescript
|
||||
// ◆ Moderate - only success path tested
|
||||
test('fetchUser retrieves user', async () => {
|
||||
const user = await fetchUser(1);
|
||||
expect(user.id).toBe(1);
|
||||
});
|
||||
|
||||
// ✓ Good
|
||||
test('fetchUser handles not found', async () => {
|
||||
await expect(fetchUser(999)).rejects.toThrow('User not found');
|
||||
});
|
||||
|
||||
test('fetchUser handles network errors', async () => {
|
||||
mockApi.get.mockRejectedValue(new NetworkError());
|
||||
await expect(fetchUser(1)).rejects.toThrow(NetworkError);
|
||||
});
|
||||
```
|
||||
|
||||
### Severity Guidance
|
||||
|
||||
- **◆◆** Critical paths untested, failing tests committed
|
||||
- **◆** New functionality without tests, missing error scenarios, test pollution
|
||||
- **◇** Could add more edge cases, test names unclear
|
||||
|
||||
---
|
||||
|
||||
## Code Quality
|
||||
|
||||
### What to Check
|
||||
|
||||
**Names reveal intent:**
|
||||
|
||||
```typescript
|
||||
// ◇ Minor
|
||||
function proc(d: Data): number {
|
||||
const x = d.items.filter(i => i.active).length;
|
||||
return x * 1.2;
|
||||
}
|
||||
|
||||
// ✓ Good
|
||||
function calculateActiveItemsWithSurcharge(data: Data): number {
|
||||
const activeItemCount = data.items.filter(item => item.active).length;
|
||||
const SURCHARGE_MULTIPLIER = 1.2;
|
||||
return activeItemCount * SURCHARGE_MULTIPLIER;
|
||||
}
|
||||
```
|
||||
|
||||
**Single responsibility:**
|
||||
|
||||
```typescript
|
||||
// ◆ Moderate - doing too much
|
||||
function processUserRequest(userId: string, action: string, data: any) {
|
||||
const user = db.getUser(userId);
|
||||
if (!user) throw new Error('Not found');
|
||||
|
||||
logger.info('Processing', { userId, action });
|
||||
|
||||
if (action === 'update') {
|
||||
db.updateUser(userId, data);
|
||||
email.send(user.email, 'Updated');
|
||||
} else if (action === 'delete') {
|
||||
db.deleteUser(userId);
|
||||
cache.clear(userId);
|
||||
}
|
||||
|
||||
return { success: true };
|
||||
}
|
||||
|
||||
// ✓ Good - separated concerns
|
||||
function validateUser(userId: string): User {
|
||||
const user = db.getUser(userId);
|
||||
if (!user) throw new Error('User not found');
|
||||
return user;
|
||||
}
|
||||
|
||||
function updateUser(userId: string, data: UserData): void {
|
||||
db.updateUser(userId, data);
|
||||
notifyUserUpdated(userId);
|
||||
}
|
||||
|
||||
function deleteUser(userId: string): void {
|
||||
db.deleteUser(userId);
|
||||
cache.clear(userId);
|
||||
}
|
||||
```
|
||||
|
||||
**No magic numbers:**
|
||||
|
||||
```typescript
|
||||
// ◇ Minor
|
||||
if (user.age > 65) { ... }
|
||||
setTimeout(doWork, 86400000);
|
||||
|
||||
// ✓ Good
|
||||
const SENIOR_AGE_THRESHOLD = 65;
|
||||
if (user.age > SENIOR_AGE_THRESHOLD) { ... }
|
||||
|
||||
const ONE_DAY_MS = 24 * 60 * 60 * 1000;
|
||||
setTimeout(doWork, ONE_DAY_MS);
|
||||
```
|
||||
|
||||
**DRY violations:**
|
||||
|
||||
```typescript
|
||||
// ◆ Moderate
|
||||
function validateEmail(email: string): boolean {
|
||||
return /^[^\s@]+@[^\s@]+\.[^\s@]+$/.test(email);
|
||||
}
|
||||
|
||||
function validateAdminEmail(email: string): boolean {
|
||||
return /^[^\s@]+@[^\s@]+\.[^\s@]+$/.test(email) && email.endsWith('@admin.com');
|
||||
}
|
||||
|
||||
// ✓ Good
|
||||
const EMAIL_REGEX = /^[^\s@]+@[^\s@]+\.[^\s@]+$/;
|
||||
|
||||
function validateEmail(email: string): boolean {
|
||||
return EMAIL_REGEX.test(email);
|
||||
}
|
||||
|
||||
function validateAdminEmail(email: string): boolean {
|
||||
return validateEmail(email) && email.endsWith('@admin.com');
|
||||
}
|
||||
```
|
||||
|
||||
**Nested conditionals:**
|
||||
|
||||
```typescript
|
||||
// ◆ Moderate - 4 levels deep
|
||||
function process(user: User) {
|
||||
if (user) {
|
||||
if (user.active) {
|
||||
if (user.subscription) {
|
||||
if (user.subscription.plan === 'premium') {
|
||||
return doThing();
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
return null;
|
||||
}
|
||||
|
||||
// ✓ Good - early returns
|
||||
function process(user: User) {
|
||||
if (!user) return null;
|
||||
if (!user.active) return null;
|
||||
if (!user.subscription) return null;
|
||||
if (user.subscription.plan !== 'premium') return null;
|
||||
|
||||
return doThing();
|
||||
}
|
||||
```
|
||||
|
||||
**Dead code:**
|
||||
|
||||
```typescript
|
||||
// ◇ Minor
|
||||
function calculate(x: number): number {
|
||||
const legacy = x * 2; // Unused, remove
|
||||
// const oldWay = x + 10; // Commented out, remove
|
||||
return x * 3;
|
||||
}
|
||||
```
|
||||
|
||||
### Severity Guidance
|
||||
|
||||
- **◆◆** Functions >200 lines, cyclomatic complexity >15
|
||||
- **◆** Functions >50 lines, DRY violations, deep nesting (>3)
|
||||
- **◇** Unclear names, magic numbers, minor complexity
|
||||
|
||||
---
|
||||
|
||||
## Documentation
|
||||
|
||||
### What to Check
|
||||
|
||||
**Public APIs documented:**
|
||||
|
||||
```typescript
|
||||
// ◆ Moderate - exported, no docs
|
||||
export function transformData(input: RawData, options: Options): ProcessedData {
|
||||
// ...
|
||||
}
|
||||
|
||||
// ✓ Good
|
||||
/**
|
||||
* Transforms raw sensor data into processed format for analysis.
|
||||
*
|
||||
* @param input - Raw data from sensor API
|
||||
* @param options - Processing options (sampling rate, filters)
|
||||
* @returns Processed data ready for visualization
|
||||
* @throws {ValidationError} If input data format is invalid
|
||||
*
|
||||
* @example
|
||||
* const processed = transformData(rawSensorData, {
|
||||
* samplingRate: 100,
|
||||
* filters: ['lowpass', 'normalize']
|
||||
* });
|
||||
*/
|
||||
export function transformData(input: RawData, options: Options): ProcessedData {
|
||||
// ...
|
||||
}
|
||||
```
|
||||
|
||||
**Complex algorithms explained:**
|
||||
|
||||
```typescript
|
||||
// ◆ Moderate - unclear why
|
||||
function score(items: Item[]): number {
|
||||
return items.reduce((sum, item) => {
|
||||
const weight = item.priority * 0.7 + item.age * 0.3;
|
||||
return sum + (item.value * weight);
|
||||
}, 0) / items.length;
|
||||
}
|
||||
|
||||
// ✓ Good
|
||||
/**
|
||||
* Calculate weighted average score for items.
|
||||
*
|
||||
* Weight formula: (priority * 0.7) + (age * 0.3)
|
||||
* - Priority weighted more heavily (70%) as immediate importance
|
||||
* - Age contributes 30% to account for staleness
|
||||
*
|
||||
* Based on research paper: doi:10.1234/scoring-algorithm
|
||||
*/
|
||||
function score(items: Item[]): number {
|
||||
return items.reduce((sum, item) => {
|
||||
const weight = item.priority * 0.7 + item.age * 0.3;
|
||||
return sum + (item.value * weight);
|
||||
}, 0) / items.length;
|
||||
}
|
||||
```
|
||||
|
||||
**Non-obvious decisions:**
|
||||
|
||||
```typescript
|
||||
// ◆ Moderate - unclear why setTimeout
|
||||
async function syncData() {
|
||||
await uploadToServer(data);
|
||||
setTimeout(cleanup, 5000);
|
||||
}
|
||||
|
||||
// ✓ Good
|
||||
async function syncData() {
|
||||
await uploadToServer(data);
|
||||
|
||||
// Delay cleanup to allow server-side replication (typically 2-3s).
|
||||
// Without delay, we observed 15% data loss in distributed setup.
|
||||
// See issue #456 for full investigation.
|
||||
setTimeout(cleanup, 5000);
|
||||
}
|
||||
```
|
||||
|
||||
**Breaking changes noted:**
|
||||
|
||||
```typescript
|
||||
// ◆ Moderate
|
||||
export function getUsers(): Promise<User[]> {
|
||||
// Changed from sync to async, breaking change not documented
|
||||
}
|
||||
|
||||
// ✓ Good
|
||||
/**
|
||||
* Fetch all users from database.
|
||||
*
|
||||
* @returns Promise resolving to array of users
|
||||
*
|
||||
* @breaking-change v2.0.0 - Now returns Promise instead of sync array.
|
||||
* Migration: Change `const users = getUsers()` to `const users = await getUsers()`
|
||||
*/
|
||||
export function getUsers(): Promise<User[]> {
|
||||
// ...
|
||||
}
|
||||
```
|
||||
|
||||
**TODOs with context:**
|
||||
|
||||
```typescript
|
||||
// ◇ Minor
|
||||
// TODO: optimize this
|
||||
|
||||
// ✓ Good
|
||||
// TODO(@alice): Optimize with caching once user volume >10k (ETA: Q2 2024)
|
||||
// Current O(n²) acceptable for <1000 users, measured at 45ms p95
|
||||
```
|
||||
|
||||
### Severity Guidance
|
||||
|
||||
- **◆◆** Breaking changes undocumented
|
||||
- **◆** Public APIs missing docs, complex algorithms unexplained
|
||||
- **◇** TODOs without context, minor doc improvements
|
||||
|
||||
---
|
||||
|
||||
## Performance
|
||||
|
||||
### What to Check
|
||||
|
||||
**N+1 queries:**
|
||||
|
||||
```typescript
|
||||
// ◆ Moderate
|
||||
async function getUsersWithPosts(userIds: string[]) {
|
||||
const users = await db.getUsers(userIds);
|
||||
for (const user of users) {
|
||||
user.posts = await db.getPostsByUser(user.id); // N queries!
|
||||
}
|
||||
return users;
|
||||
}
|
||||
|
||||
// ✓ Good
|
||||
async function getUsersWithPosts(userIds: string[]) {
|
||||
const users = await db.getUsers(userIds);
|
||||
const posts = await db.getPostsByUsers(userIds); // 1 query
|
||||
|
||||
const postsByUser = posts.reduce((acc, post) => {
|
||||
(acc[post.userId] ||= []).push(post);
|
||||
return acc;
|
||||
}, {});
|
||||
|
||||
return users.map(user => ({
|
||||
...user,
|
||||
posts: postsByUser[user.id] || []
|
||||
}));
|
||||
}
|
||||
```
|
||||
|
||||
**Appropriate data structures:**
|
||||
|
||||
```typescript
|
||||
// ◆ Moderate - O(n) lookup
|
||||
const activeUsers = users.filter(u => u.active);
|
||||
function isActive(userId: string): boolean {
|
||||
return activeUsers.find(u => u.id === userId) !== undefined; // O(n)
|
||||
}
|
||||
|
||||
// ✓ Good - O(1) lookup
|
||||
const activeUserIds = new Set(users.filter(u => u.active).map(u => u.id));
|
||||
function isActive(userId: string): boolean {
|
||||
return activeUserIds.has(userId); // O(1)
|
||||
}
|
||||
```
|
||||
|
||||
**Unnecessary allocations:**
|
||||
|
||||
```typescript
|
||||
// ◇ Minor
|
||||
function processItems(items: Item[]) {
|
||||
return items
|
||||
.map(i => ({ ...i })) // Copy 1
|
||||
.map(i => ({ ...i, processed: true })) // Copy 2
|
||||
.filter(i => i.active);
|
||||
}
|
||||
|
||||
// ✓ Good
|
||||
function processItems(items: Item[]) {
|
||||
return items
|
||||
.filter(i => i.active)
|
||||
.map(i => ({ ...i, processed: true }));
|
||||
}
|
||||
```
|
||||
|
||||
**Async operations:**
|
||||
|
||||
```typescript
|
||||
// ◆ Moderate - sequential, slow
|
||||
async function loadData() {
|
||||
const users = await fetchUsers(); // Wait
|
||||
const posts = await fetchPosts(); // Wait
|
||||
const comments = await fetchComments(); // Wait
|
||||
return { users, posts, comments };
|
||||
}
|
||||
|
||||
// ✓ Good - parallel
|
||||
async function loadData() {
|
||||
const [users, posts, comments] = await Promise.all([
|
||||
fetchUsers(),
|
||||
fetchPosts(),
|
||||
fetchComments()
|
||||
]);
|
||||
return { users, posts, comments };
|
||||
}
|
||||
```
|
||||
|
||||
### Severity Guidance
|
||||
|
||||
- **◆◆** Performance bugs (infinite loops, memory leaks)
|
||||
- **◆** Obvious N+1, blocking operations in hot path
|
||||
- **◇** Minor optimizations, better data structures possible
|
||||
|
||||
---
|
||||
|
||||
## Rust-Specific
|
||||
|
||||
### What to Check
|
||||
|
||||
**Result over panic:**
|
||||
|
||||
```rust
|
||||
// ◆◆ Severe
|
||||
pub fn divide(a: i32, b: i32) -> i32 {
|
||||
a / b // Panics on division by zero
|
||||
}
|
||||
|
||||
// ✓ Good
|
||||
pub fn divide(a: i32, b: i32) -> Result<i32, &'static str> {
|
||||
if b == 0 {
|
||||
Err("Division by zero")
|
||||
} else {
|
||||
Ok(a / b)
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
**No unwrap outside tests:**
|
||||
|
||||
```rust
|
||||
// ◆ Moderate
|
||||
pub fn get_config() -> Config {
|
||||
fs::read_to_string("config.toml")
|
||||
.unwrap() // Bad: panics on missing file
|
||||
.parse()
|
||||
.unwrap()
|
||||
}
|
||||
|
||||
// ✓ Good
|
||||
pub fn get_config() -> Result<Config, ConfigError> {
|
||||
let content = fs::read_to_string("config.toml")?;
|
||||
let config = content.parse()?;
|
||||
Ok(config)
|
||||
}
|
||||
```
|
||||
|
||||
**Ownership/borrowing:**
|
||||
|
||||
```rust
|
||||
// ◆ Moderate - unnecessary clone
|
||||
fn process_data(data: Vec<u8>) -> Vec<u8> {
|
||||
let copy = data.clone(); // Unnecessary allocation
|
||||
transform(copy)
|
||||
}
|
||||
|
||||
// ✓ Good
|
||||
fn process_data(data: Vec<u8>) -> Vec<u8> {
|
||||
transform(data) // Move ownership
|
||||
}
|
||||
|
||||
// Or if data is needed later:
|
||||
fn process_data(data: &[u8]) -> Vec<u8> {
|
||||
transform(data) // Borrow
|
||||
}
|
||||
```
|
||||
|
||||
**Unsafe justification:**
|
||||
|
||||
```rust
|
||||
// ◆◆ Severe - unjustified unsafe
|
||||
pub fn get_value(ptr: *const u8) -> u8 {
|
||||
unsafe { *ptr }
|
||||
}
|
||||
|
||||
// ✓ Good
|
||||
/// # Safety
|
||||
///
|
||||
/// `ptr` must be:
|
||||
/// - Properly aligned for type `u8`
|
||||
/// - Non-null
|
||||
/// - Valid for reads (pointing to initialized memory)
|
||||
/// - Not accessed concurrently from other threads
|
||||
pub unsafe fn get_value(ptr: *const u8) -> u8 {
|
||||
unsafe { *ptr }
|
||||
}
|
||||
|
||||
// ✓ Better - avoid unsafe if possible
|
||||
pub fn get_value(slice: &[u8], index: usize) -> Option<u8> {
|
||||
slice.get(index).copied()
|
||||
}
|
||||
```
|
||||
|
||||
### Severity Guidance
|
||||
|
||||
- **◆◆** Unjustified unsafe, panics in library code, Send/Sync violations
|
||||
- **◆** unwrap/expect in production code, unnecessary clones
|
||||
- **◇** Could use iterators, minor allocation improvements
|
||||
|
||||
---
|
||||
|
||||
## Severity Summary
|
||||
|
||||
### ◆◆ Severe (blocking)
|
||||
|
||||
Ship with these → security incidents, runtime failures, data loss.
|
||||
|
||||
Examples:
|
||||
- SQL injection, XSS, auth bypass
|
||||
- Unhandled errors that crash
|
||||
- Type assertions that can panic
|
||||
- Hardcoded secrets
|
||||
- Unjustified unsafe code (Rust)
|
||||
- Test failures committed
|
||||
|
||||
**Action:** Must fix before merge.
|
||||
|
||||
### ◆ Moderate (should fix)
|
||||
|
||||
Ship with these → maintenance burden, degraded quality, potential bugs.
|
||||
|
||||
Examples:
|
||||
- Missing null checks
|
||||
- Poor error messages
|
||||
- Missing tests
|
||||
- Significant code duplication
|
||||
- Performance issues (N+1)
|
||||
- Missing docs on public APIs
|
||||
|
||||
**Action:** Fix before merge unless explicitly accepted as tech debt with tracking issue.
|
||||
|
||||
### ◇ Minor (consider addressing)
|
||||
|
||||
Ship with these → code could be better, but functional and safe.
|
||||
|
||||
Examples:
|
||||
- Unclear variable names
|
||||
- Magic numbers
|
||||
- Missing edge case tests
|
||||
- Minor performance optimizations
|
||||
- Documentation improvements
|
||||
- Code style inconsistencies
|
||||
|
||||
**Action:** Optional improvements. Consider batching in refactoring PR.
|
||||
|
||||
---
|
||||
|
||||
## Review Patterns
|
||||
|
||||
### Security-Sensitive Areas
|
||||
|
||||
Give extra scrutiny to:
|
||||
- Authentication/authorization logic
|
||||
- User input handling (forms, APIs, queries)
|
||||
- Database queries (SQL injection risk)
|
||||
- File uploads/downloads
|
||||
- Cryptographic operations
|
||||
- Session management
|
||||
- CORS/CSRF protections
|
||||
|
||||
### Common False Positives
|
||||
|
||||
**Type safety:** `as unknown as X` sometimes necessary for complex type gymnastics — verify justification comment.
|
||||
|
||||
**Error handling:** Not all errors need recovery — sometimes propagating is correct.
|
||||
|
||||
**Magic numbers:** Domain constants (HTTP codes, standard ports) don't need extraction.
|
||||
|
||||
**Function length:** Pure data transformation can be >50 lines if clear.
|
||||
|
||||
### When to Escalate
|
||||
|
||||
Flag for senior review if:
|
||||
- Security implications unclear
|
||||
- Performance impact uncertain
|
||||
- Architecture decision embedded
|
||||
- Breaking change considerations
|
||||
- Unfamiliar technology or pattern
|
||||
|
||||
---
|
||||
|
||||
## Checklist Efficiency
|
||||
|
||||
### Full Review (default)
|
||||
|
||||
Run all categories for:
|
||||
- Pre-merge reviews
|
||||
- Critical path changes
|
||||
- Security-sensitive code
|
||||
- Public API changes
|
||||
|
||||
### Targeted Review
|
||||
|
||||
Focus specific categories for:
|
||||
- **Refactors** → Code Quality, Tests
|
||||
- **Bug fixes** → Error Handling, Tests, Type Safety
|
||||
- **Performance work** → Performance, Tests
|
||||
- **Security patches** → Security, Error Handling
|
||||
- **Docs** → Documentation
|
||||
|
||||
### Quick Sanity Check (not Fresh Eyes)
|
||||
|
||||
For trivial changes (typos, formatting), skip formal review. Don't invoke Fresh Eyes skill — use judgment.
|
||||
Reference in New Issue
Block a user