Guidelines for conducting and participating in code reviews.
graph LR
PR[Pull Request] --> SELF[Self Review]
SELF --> AUTO[Automated Checks]
AUTO --> ASSIGN[Assign Reviewers]
ASSIGN --> REVIEW[Peer Review]
REVIEW --> FEEDBACK[Feedback]
FEEDBACK --> CHANGES[Make Changes]
CHANGES --> REVIEW
REVIEW --> APPROVE[Approval]
APPROVE --> MERGE[Merge]
AUTO -.Fails.-> FIX[Fix Issues]
FIX --> AUTO
style REVIEW fill:#bbf,stroke:#333,stroke-width:2px
style APPROVE fill:#dfd,stroke:#0f0,stroke-width:2px
## Description
Brief description of changes and why they're needed.
## Type of Change
- [ ] Bug fix (non-breaking change fixing an issue)
- [ ] New feature (non-breaking change adding functionality)
- [ ] Breaking change (fix or feature causing existing functionality to change)
- [ ] Documentation update
- [ ] Performance improvement
- [ ] Refactoring
## Changes Made
- Added/Modified/Removed X to achieve Y
- Updated Z to fix issue #123
## Testing
- [ ] Unit tests pass
- [ ] Integration tests pass
- [ ] Manual testing completed
- [ ] E2E tests pass (if applicable)
## Screenshots (if applicable)
[Add screenshots for UI changes]
## Checklist
- [ ] Self-review completed
- [ ] Code follows project style guidelines
- [ ] Tests added/updated
- [ ] Documentation updated
- [ ] No new warnings
- [ ] Changes are backward compatible
## Related Issues
Closes #123
Related to #456# Review your changes
git diff main...HEAD
# Check each file
git diff --stat
# Review commit messages
git log --oneline main...HEAD- Does the code do what it's supposed to?
- Are there any obvious bugs?
- Is the code readable and clear?
- Are there adequate tests?
- Does it follow project conventions?
- Are there any security concerns?
# Squash related commits
git rebase -i main
# Write clear commit messages
git commit --amend## Review Notes
- Pay special attention to the timer state machine changes
- The new validation logic is in `domain/src/timer/transitions.rs`
- I'm unsure about the error handling in the use case layer
- Performance might be a concern in the event handler// Reviewer: This could be more efficient
// Author: Good point! Updated to use HashMap for O(1) lookup
// Before
let task = tasks.iter().find(|t| t.id() == &id);
// After
let task = task_map.get(&id);> Why not use async here?
This operation is CPU-bound and very fast (< 1ms). Making it async would add unnecessary overhead without any benefit. The blocking operation doesn't affect the event loop.- Does it follow Clean Architecture principles?
- Are layer boundaries respected?
- Is the design extensible?
- Is the code readable and maintainable?
- Are there any code smells?
- Is there unnecessary complexity?
- Are tests comprehensive?
- Do tests cover edge cases?
- Are tests maintainable?
- Are there any obvious bottlenecks?
- Is memory usage reasonable?
- Are there unnecessary allocations?
- Are inputs validated?
- Are errors handled properly?
- Are there any injection risks?
❌ Bad: "This is wrong"
✅ Good: "This could cause a race condition. Consider using a Mutex here."
❌ Bad: "Terrible naming"
✅ Good: "Could we use a more descriptive name? Maybe `process_timer_tick` instead of `handle`?"🔴 **Must Fix**: Critical issue that blocks merging
🟡 **Should Fix**: Important but not blocking
🟢 **Consider**: Suggestion for improvement
💭 **Thought**: Discussion point or question
✨ **Praise**: Highlighting good code- let result = data.iter().filter(|x| x.is_valid()).collect();
+ let valid_items: Vec<_> = data
+ .iter()
+ .filter(|item| item.is_valid())
+ .collect();// 🔴 Must Fix: Business logic violation
// This allows invalid state transitions
impl Timer {
pub fn complete(&mut self) {
self.state = TimerState::Completed; // Missing validation!
}
}
// Suggested fix:
impl Timer {
pub fn complete(&mut self) -> Result<(), DomainError> {
match self.state {
TimerState::Running => {
self.state = TimerState::Completed;
Ok(())
}
_ => Err(DomainError::InvalidStateTransition)
}
}
}// 🟡 Should Fix: Missing error handling
pub async fn execute(&self) -> Result<TaskDto> {
let task = self.repo.find(id).await?; // What if None?
Ok(TaskDto::from(task))
}
// Suggested:
pub async fn execute(&self) -> Result<TaskDto> {
let task = self.repo.find(id).await?
.ok_or(UseCaseError::TaskNotFound)?;
Ok(TaskDto::from(task))
}// 🟢 Consider: Performance improvement
// This creates unnecessary allocations
let names: Vec<String> = tasks
.iter()
.map(|t| t.name().to_string())
.collect();
// Consider:
let names: Vec<&str> = tasks
.iter()
.map(|t| t.name().as_str())
.collect();- Follows Clean Architecture principles
- Respects layer boundaries
- No circular dependencies
- Proper separation of concerns
- Clear and readable
- Follows project conventions
- No code duplication
- Appropriate abstraction level
- Adequate test coverage
- Tests are meaningful
- Edge cases covered
- Tests follow AAA pattern
- Public APIs documented
- Complex logic explained
- README updated if needed
- Examples provided
- No obvious bottlenecks
- Efficient algorithms used
- Appropriate data structures
- No unnecessary allocations
- Input validation present
- No hardcoded secrets
- Proper error handling
- Safe unwrapping
// ❌ Bad: Can panic
let value = some_option.unwrap();
// ✅ Good: Proper error handling
let value = some_option.ok_or(Error::MissingValue)?;// ❌ Bad: Complex logic without tests
pub fn calculate_score(data: &[Item]) -> u32 {
// Complex calculation...
}
// ✅ Good: Tested logic
#[cfg(test)]
mod tests {
#[test]
fn calculate_score_handles_empty() { }
#[test]
fn calculate_score_sums_correctly() { }
}// ❌ Bad: File not closed
let file = File::open(path)?;
// ... use file
// ✅ Good: Automatic cleanup
let contents = fs::read_to_string(path)?;// ❌ Bad: Unsafe concurrent access
static mut COUNTER: u32 = 0;
// ✅ Good: Thread-safe
use std::sync::atomic::{AtomicU32, Ordering};
static COUNTER: AtomicU32 = AtomicU32::new(0);# .github/workflows/review.yml
name: Review Checks
on: [pull_request]
jobs:
review:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v2
- name: Format Check
run: cargo fmt -- --check
- name: Clippy
run: cargo clippy -- -D warnings
- name: Tests
run: cargo test --workspace
- name: Coverage
run: cargo tarpaulin --min 80# Format check
cargo fmt -- --check
# Linting
cargo clippy -- -D warnings
# Security audit
cargo audit
# Dependency check
cargo outdated- Review promptly (within 24 hours)
- Be respectful and constructive
- Explain the "why" behind feedback
- Acknowledge good code
- Ask questions when unsure
- Test the changes locally
- Don't nitpick minor style issues
- Don't review when tired/frustrated
- Don't approve without understanding
- Don't block on personal preferences
- Don't skip testing the changes
- Don't ignore CI failures
- All CI checks pass
- Required reviews approved
- All feedback addressed
- No unresolved discussions
- Documentation updated
- Tests pass locally
- See Testing Workflow
- Learn Adding Features
- Review Git Workflow