102 lines
3.4 KiB
Markdown
102 lines
3.4 KiB
Markdown
# Code Review Guide
|
|
|
|
## Review Checklist
|
|
|
|
### 1. Correctness
|
|
- [ ] Code does what the issue/PR description says
|
|
- [ ] Edge cases are handled
|
|
- [ ] Error conditions are managed properly
|
|
- [ ] No obvious bugs or logic errors
|
|
|
|
### 2. Security
|
|
- [ ] No hardcoded secrets or credentials
|
|
- [ ] Input validation at boundaries
|
|
- [ ] SQL injection prevention (parameterized queries)
|
|
- [ ] XSS prevention (output encoding)
|
|
- [ ] Authentication/authorization checks present
|
|
- [ ] Sensitive data not logged
|
|
- [ ] Secrets follow Vault structure (see `docs/vault-secrets-structure.md`)
|
|
|
|
### 3. Testing
|
|
- [ ] Tests exist for new functionality
|
|
- [ ] Tests cover happy path AND error cases
|
|
- [ ] Coverage meets 85% minimum
|
|
- [ ] Tests are readable and maintainable
|
|
- [ ] No flaky tests introduced
|
|
|
|
### 4. Code Quality
|
|
- [ ] Follows Google Style Guide for the language
|
|
- [ ] Functions are focused and reasonably sized
|
|
- [ ] No unnecessary complexity
|
|
- [ ] DRY - no significant duplication
|
|
- [ ] Clear naming for variables and functions
|
|
- [ ] No dead code or commented-out code
|
|
|
|
### 4a. TypeScript Strict Typing (see `typescript.md`)
|
|
- [ ] **NO `any` types** — explicit types required everywhere
|
|
- [ ] **NO lazy `unknown`** — only for error catches with immediate narrowing
|
|
- [ ] **Explicit return types** on all exported/public functions
|
|
- [ ] **Explicit parameter types** — never implicit any
|
|
- [ ] **No type assertions** (`as Type`) — use type guards instead
|
|
- [ ] **No non-null assertions** (`!`) — use proper null handling
|
|
- [ ] **Interfaces for objects** — not inline types
|
|
- [ ] **Discriminated unions** for variant types
|
|
|
|
### 5. Documentation
|
|
- [ ] Complex logic has explanatory comments
|
|
- [ ] Public APIs are documented
|
|
- [ ] README updated if needed
|
|
- [ ] Breaking changes noted
|
|
|
|
### 6. Performance
|
|
- [ ] No obvious N+1 queries
|
|
- [ ] No blocking operations in hot paths
|
|
- [ ] Resource cleanup (connections, file handles)
|
|
- [ ] Reasonable memory usage
|
|
|
|
### 7. Dependencies
|
|
- [ ] No deprecated packages
|
|
- [ ] No unnecessary new dependencies
|
|
- [ ] Dependency versions pinned appropriately
|
|
|
|
## Review Process
|
|
|
|
### Getting Context
|
|
```bash
|
|
# List the issue being addressed
|
|
~/.config/mosaic/rails/git/issue-list.sh -i {issue-number}
|
|
|
|
# View the changes
|
|
git diff main...HEAD
|
|
```
|
|
|
|
### Providing Feedback
|
|
- Be specific: point to exact lines/files
|
|
- Explain WHY something is problematic
|
|
- Suggest alternatives when possible
|
|
- Distinguish between blocking issues and suggestions
|
|
- Be constructive, not critical of the person
|
|
|
|
### Feedback Categories
|
|
- **Blocker**: Must fix before merge (security, bugs, test failures)
|
|
- **Should Fix**: Important but not blocking (code quality, minor issues)
|
|
- **Suggestion**: Optional improvements (style preferences, nice-to-haves)
|
|
- **Question**: Seeking clarification
|
|
|
|
### Review Comment Format
|
|
```
|
|
[BLOCKER] Line 42: SQL injection vulnerability
|
|
The user input is directly interpolated into the query.
|
|
Use parameterized queries instead:
|
|
`db.query("SELECT * FROM users WHERE id = ?", [userId])`
|
|
|
|
[SUGGESTION] Line 78: Consider extracting to helper
|
|
This pattern appears in 3 places. A shared helper would reduce duplication.
|
|
```
|
|
|
|
## After Review
|
|
1. Update issue with review status
|
|
2. If changes requested, assign back to author
|
|
3. If approved, note approval in issue comments
|
|
4. For merges, ensure CI passes first
|