Skip to content

Code Review Guidelines

Last Updated: 2025-01-22

Code review guidelines for maintaining quality and consistency in the AccessALI codebase.

Overview

Code reviews ensure:

  • Quality - Catch bugs and design issues early
  • Knowledge sharing - Spread understanding across team
  • Consistency - Maintain coding standards
  • Mentorship - Help developers grow
  • Documentation - Improve code clarity

Review Process

graph LR
    Create[Create PR] --> Assign[Assign Reviewers]
    Assign --> Review[Review Code]
    Review --> Changes{Changes<br/>Requested?}
    Changes -->|Yes| Update[Update Code]
    Update --> Review
    Changes -->|No| Approve[Approve PR]
    Approve --> Merge[Merge]

Creating a Pull Request

Before Creating PR

  • Code compiles without errors
  • All tests pass locally
  • Linter passes (pnpm lint)
  • Type check passes (pnpm type-check)
  • No console.log statements (use logger)
  • Code follows conventions
  • Added tests for new features
  • Updated documentation

PR Template

## Description

Brief description of changes.

## Type of Change

- [ ] Bug fix
- [ ] New feature
- [ ] Breaking change
- [ ] Documentation update

## Testing

- [ ] Unit tests added/updated
- [ ] Integration tests added/updated
- [ ] E2E tests added/updated
- [ ] Manual testing completed

## Checklist

- [ ] Code follows style guidelines
- [ ] Self-review completed
- [ ] Documentation updated
- [ ] No new warnings
- [ ] Tests pass locally

## Screenshots (if applicable)

Attach before/after screenshots for UI changes.

## Related Issues

Closes #123

PR Title Format

feat(scope): add user authentication
fix(dashboard): correct property count display
docs: update API documentation
refactor(repository): simplify query logic
test: add user repository tests
chore: update dependencies

Reviewing Code

What to Look For

Functionality

  • Code does what it's supposed to do
  • Edge cases are handled
  • Error handling is appropriate
  • No obvious bugs

Design

  • Follows architectural patterns
  • No code duplication
  • Functions are single-purpose
  • Proper separation of concerns
  • Reusable components extracted

Code Quality

  • Readable and understandable
  • Well-named variables and functions
  • Appropriate comments (why, not what)
  • No magic numbers or strings
  • Proper TypeScript types

Testing

  • Adequate test coverage
  • Tests are meaningful
  • Edge cases tested
  • Error scenarios tested

Security

  • No hardcoded secrets
  • Input validation present
  • Authentication checks present
  • SQL injection prevented
  • XSS prevention in place

Performance

  • No N+1 queries
  • Appropriate use of caching
  • Large lists paginated
  • Heavy operations optimized
  • Database queries optimized

Review Comments

Constructive Feedback

Good:
"Consider extracting this logic into a separate function for reusability.

```typescript
function calculateTotalPrice(items: CartItem[]) {
  return items.reduce((sum, item) => sum + item.price, 0)
}
"

Bad: "This is wrong." 

### Comment Types

#### Blocking

```markdown
**[BLOCKING]** This function has a security vulnerability.
User input is not validated before database query.

Please add Zod validation:
\`\`\`typescript
const schema = z.object({
  email: z.string().email(),
})
\`\`\`

Suggestion

**[SUGGESTION]** Consider using Promise.all here for better performance:

\`\`\`typescript
// Current
const user = await getUser()
const properties = await getProperties()

// Suggested
const [user, properties] = await Promise.all([
  getUser(),
  getProperties(),
])
\`\`\`

Question

**[QUESTION]** Why did we choose to use a separate table here instead of embedding the data?

Nitpick

**[NITPICK]** Minor: Consider using a more descriptive variable name.

\`data\` → \`userProfile\`

Response to Reviews

As Author

Accepting Feedback

Good point! Updated to use Promise.all.

✅ Commit: abc123

Explaining Decisions

I chose this approach because...

However, if you feel strongly about the alternative, I'm happy to change it.

Disagreeing

I understand your concern, but I think this approach is better because...

What do you think?

As Reviewer

Following Up

Thanks for the update! Looks good now.

✅ Approved

Suggesting Alternatives

I see your point. Another option could be...

Either way works for me.

Approval Criteria

Required for Approval

  • All automated checks pass (CI)
  • At least one approval from code owner
  • All blocking comments resolved
  • No merge conflicts
  • Documentation updated (if needed)

Before Merging

  • All conversations resolved
  • CI/CD pipeline passes
  • Approved by required reviewers
  • Branch up to date with base
  • Squash commits (if needed)

Best Practices

For Authors

Do

  • Keep PRs small and focused
  • Provide context in description
  • Add screenshots for UI changes
  • Respond to feedback promptly
  • Test changes thoroughly
  • Update documentation
  • Squash fixup commits

Don't

  • Mix multiple concerns in one PR
  • Make large refactorings without discussion
  • Ignore review feedback
  • Force push after review started
  • Merge without approval
  • Leave TODO comments
  • Include unrelated changes

For Reviewers

Do

  • Review promptly (within 24 hours)
  • Be constructive and specific
  • Praise good work
  • Ask questions to understand
  • Suggest alternatives
  • Approve when ready
  • Test locally for complex changes

Don't

  • Be vague or dismissive
  • Nitpick excessively
  • Block on stylistic preferences
  • Rewrite code in comments
  • Delay reviews unnecessarily
  • Approve without reviewing
  • Focus only on negatives

Common Issues

Large PRs

Problem: PR with 1000+ lines of changes

Solution:

  • Break into smaller PRs
  • Use feature flags for incremental releases
  • Separate refactoring from features

Merge Conflicts

Problem: Branch has conflicts with base

Solution: 

git checkout develop
git pull origin develop
git checkout feature-branch
git merge develop
# Resolve conflicts
git add .
git commit
git push

Failed CI

Problem: Tests fail on CI but pass locally

Solution:

  • Check CI logs for specific error
  • Verify environment variables
  • Test with clean node_modules
  • Check for race conditions
  • Ensure database is seeded

Review Checklist

Quick Checklist

  • Read PR description
  • Review changed files
  • Check test coverage
  • Verify no security issues
  • Ensure follows conventions
  • Test locally (if needed)
  • Leave constructive feedback
  • Approve or request changes

Next Steps

  1. Review Commit Conventions
  2. Understand Coding Conventions
  3. Learn Testing practices