code-review

Overview

Review code for quality, user impact, tests, and documentation. Balance technical excellence with practical simplicity.

Core principle: Clean code should serve users, not just developers.

When to Use

• After completing a feature or bug fix
• Before creating a pull request
• When reviewing changes before merge

When NOT to Use

• Trivial changes (typo fixes)
• Documentation-only changes
• Initial exploration/prototyping

Review Process

| Phase | Focus | Key Question | |-------|-------|--------------| | 1. Identify | What changed? | git diff --name-only develop | | 2. User Impact | How does this affect users? | Is UX better or worse? | | 3. Code Quality | Does it follow standards? | KISS + no anti-patterns? | | 4. Tests | Is it covered? | New code = new tests? | | 5. Docs | What needs updating? | llm/state-*.md current? |

---

Phase 1: Identify Changes

Categorize changed files:

• Backend: lib/, app.py
• Frontend: frontend/src/
• Tests: tests/
• Docs: llm/, *.md

Note change type: new feature | bug fix | refactoring | enhancement

---

Phase 2: User Impact

Ask for each change:

1. Does this affect what users see or do?
2. Are error messages user-friendly (not technical jargon)?
3. Are loading states shown?
4. Can users recover from errors?
5. Is this the simplest UX possible?

Red flags:

• Silent failures (user doesn't know something failed)
• Lost work on errors
• Unclear feedback ("Error: 500" vs "Could not save")
• Unnecessary complexity exposed to users

---

Phase 3: Code Quality

KISS Check

Can each function be explained in one sentence? If not, it's too complex.

Backend Anti-patterns (blocking)

• [ ] Silent failures (empty except blocks)
• [ ] God functions (>30 lines, >3 params)
• [ ] SQL injection (f-strings in queries)
• [ ] Missing error context
• [ ] Walrus operators / complex one-liners

Frontend Anti-patterns (blocking)

• [ ] Empty catch blocks
• [ ] Inline fetch (not in service layer)
• [ ] Missing useEffect cleanup
• [ ] any types or as assertions
• [ ] Hardcoded colors (use theme: fg., canvas.)
• [ ] Prop drilling (>5 props)

Security

• [ ] Inputs validated at API boundary
• [ ] SQL parameterized (? placeholders)
• [ ] No secrets in code/logs

---

Phase 4: Test Coverage

| Change Type | Required Test | |-------------|---------------| | New API endpoint | Unit test | | New block | tests/blocks/test_*.py | | Bug fix | Regression test | | User workflow change | E2E test | | Refactoring | Existing tests pass |

Test quality:

• Naming: test_<method>_<scenario>_<expected>
• One behavior per test
• Error cases tested, not just happy path

---

Phase 5: Documentation

Update llm/state-*.md when:

• New API endpoint → state-backend.md
• New block → state-backend.md
• New component/page → state-frontend.md
• Architecture change → state-project.md

Code comments: explain WHY, not what. Lowercase, concise.

---

Output Format

### User Impact
[UX improvements or issues found]

### Anti-patterns
[location + violation + fix, or "none"]

### Code Quality Issues
[severity + location + fix, or "none"]

### Test Coverage
[required: present/missing | gaps if any]

### Documentation Updates
[files needing update, or "none"]

### Verdict
[BLOCK | REQUEST CHANGES | APPROVE]
Reason: [brief explanation]

---

Verdict Rules

| Condition | Verdict | |-----------|---------| | Anti-patterns found | BLOCK | | Security issues | BLOCK | | Missing required tests | REQUEST CHANGES | | Needs doc updates | REQUEST CHANGES | | All checks pass | APPROVE |

---

Golden Rules

1. Anti-patterns are blocking - always reject
2. User experience matters - clean code that hurts UX is bad code
3. KISS wins - one sentence explanation or it's too complex
4. Tests are not optional - new code needs tests
5. Fail loudly - silent failures are never acceptable