Gmacko PR Review

Gmacko PR Review

Review pull requests against coding standards, INITIAL_PLAN.md, and feature acceptance criteria.

When to Use

• A PR is ready for review
• User asks for code review
• Before merging to main/​staging
• After a major feature implementation

Workflow

digraph pr_review {
    rankdir=TB;
    node [shape=box];
    
    start [label="Start Review" shape=ellipse];
    fetch [label="1. Fetch PR Context"];
    plan [label="2. Check Against Plan"];
    standards [label="3. Check Coding Standards"];
    security [label="4. Security Review"];
    tests [label="5. Test Coverage"];
    summary [label="6. Generate Summary"];
    decision [label="7. Recommendation"];
    done [label="Review Complete" shape=ellipse];
    
    start -> fetch -> plan -> standards;
    standards -> security -> tests -> summary -> decision -> done;
}

Review Checklist

1. PR Metadata

• Title follows convention ([Type]: Description)
• Description explains the "why"
• Related issues linked
• Appropriate labels assigned
• Correct base branch

2. Plan Alignment

• Changes match feature plan scope
• No scope creep (unplanned features)
• Acceptance criteria addressed
• Non-goals respected

3. Code Quality

• Follows TypeScript strict mode
• Uses interface over type for objects
• No any types (use unknown if needed)
• Proper error handling
• No console.log statements
• No commented-out code

4. Naming Conventions

• Files: kebab-case
• Components: PascalCase
• Functions/​variables: camelCase
• Constants: SCREAMING_SNAKE_CASE

5. Import Order

1. React/​external libraries
2. @repo/​* workspace packages
3. @/​* internal aliases
4. Relative imports

6. Architecture

• Follows provider hierarchy
• Uses appropriate shared package
• tRPC procedures follow patterns
• Database changes have migrations

7. Security

• No secrets in code
• No .env files modified (only .env.example)
• Input validation on API endpoints
• Proper auth checks
• No SQL injection risks

8. Performance

• No unnecessary re-renders
• Appropriate use of useMemo/useCallback
• Images optimized
• Bundle size considered

9. Testing

• New code has tests (if applicable)
• Existing tests pass
• Manual testing documented

10. Cross-Platform (if applicable)

• Web and mobile consistency
• Platform-specific code isolated
• Shared logic in packages

Execution Steps

Step 1: Fetch PR Context

Get PR details:

# Get PR info
gh pr view [number] --json title,body,labels,files,additions,deletions

# Get diff
gh pr diff [number]

# Get related issues
gh pr view [number] --json linkedIssues

Identify:

• What files changed
• What type of change (feature/​bug/​refactor)
• Which areas affected (web/​mobile/​api/​db)

Step 2: Check Against Plan

If PR relates to a feature plan:

1. Find the plan: docs/​ai/​handoffs/​{feature}-plan.md
2. Check acceptance criteria
3. Verify scope alignment
4. Note any deviations

## Plan Alignment Check

Plan: docs/​ai/​handoffs/​dark-mode-plan.md
Issue: #456

### Acceptance Criteria
- [x] Toggle switches theme (IMPLEMENTED)
- [x] Preference persisted (IMPLEMENTED)
- [ ] Respects system preference (NOT FOUND)

### Scope Notes
- PR includes additional animation (not in plan) - ASK about scope

Step 3: Check Coding Standards

Review code against project standards:

## Code Quality Review

### TypeScript
- [x] Strict mode compliance
- [x] No `any` types
- [!] Line 45: Consider using `unknown` instead of type assertion

### Naming
- [x] File naming correct
- [x] Component naming correct
- [!] Line 23: Variable `x` should be more descriptive

### Imports
- [x] Import order correct
- [!] Line 5: External import should come before @repo import

### Patterns
- [x] tRPC procedures follow patterns
- [x] Error handling present

Step 4: Security Review

Check for security issues:

## Security Review

- [x] No hardcoded secrets
- [x] .env files not modified
- [x] Auth checks in place
- [x] Input validation present
- [!] Line 78: Consider rate limiting this endpoint

Step 5: Test Coverage

Verify testing:

## Test Coverage

- [ ] Unit tests for ThemeToggle component
- [x] Integration test for theme API
- [!] No E2E tests for theme switching flow

### Manual Testing Checklist
- [ ] Web: Theme toggles correctly
- [ ] Web: Preference persists on refresh
- [ ] Mobile: Theme toggles correctly
- [ ] Mobile: Preference persists

Step 6: Generate Summary

Create review summary:

# PR Review Summary

**PR**: #123 - [Feature]: Add dark mode support
**Reviewer**: AI Assistant
**Date**: 2025-01-05

## Overall Assessment

**Recommendation**: APPROVE with minor suggestions

## Highlights
- Clean implementation of theme switching
- Good use of Zustand for state management
- Proper separation of web/​mobile code

## Issues Found

### Must Fix (Blocking)
None

### Should Fix (Non-blocking)
1. **Line 45**: Use `unknown` type instead of assertion
2. **Line 78**: Add rate limiting to theme endpoint

### Consider (Suggestions)
1. Add unit tests for ThemeToggle
2. Consider adding system preference detection
3. Animation could be smoother

## Checklist Completion
- Plan alignment: 2/​3 criteria met
- Code quality: 8/​10 checks passed
- Security: All checks passed
- Tests: Partial coverage

## Files Reviewed
- `packages/​store/​src/​stores/​theme.ts` - OK
- `apps/​web/​src/​components/​theme-toggle.tsx` - Minor issues
- `apps/​mobile/​src/​screens/​settings.tsx` - OK
- `packages/​api/​src/​routers/​user.ts` - Suggestion

Step 7: Recommendation

Provide clear recommendation:

Review Comment Templates

Approval

LGTM! Clean implementation that follows our patterns.

Minor suggestions:
- Consider adding tests for edge cases
- The animation could be smoother

Approved for merge.

Request Changes

Good progress! A few things need attention before merge:

**Must fix:**
1. Line 45: Security issue - input not validated
2. Line 78: Missing auth check

**Questions:**
- Is the scope creep (animations) intentional?

Please address the blocking issues and I'll re-review.

Red Flags

Integration

• Input: PR number or URL
• References: Feature plan, coding standards, INITIAL_PLAN.md
• Output: Review summary with recommendation
• Next: Developer addresses feedback or merges