Code Review

---
name: code-review
description: Use when reviewing code, opening a PR, asked to check a diff, or told "review this". Covers review mindset, what to look for, severity levels, comment style, and per-language checklists (Go, Flutter/Dart, Nuxt/Vue).
---

# Code Review Standards

A review is not a style check — it's a correctness + maintainability gate. The goal is to ship better software, not win arguments.

---

## 1. Mindset

- **Assume good intent.** The author made reasonable choices given what they knew.
- **Ask, don't dictate.** "What do you think about X?" lands better than "Change X to Y."
- **Separate blocking from non-blocking.** Be explicit about which comments must be addressed.
- **Don't review what a linter can catch.** Formatting, import order, unused vars → CI's job.
- **Praise good work.** If something is elegant, say so. It signals what to repeat.

---

## 2. Severity Labels

Use these prefixes so the author knows what to prioritize:

| Label | Meaning |
|---|---|
| `[blocking]` | Must fix before merge. Correctness, security, data loss risk. |
| `[suggestion]` | Worth considering. Not required. |
| `[nit]` | Minor style/naming. Author can ignore or batch. |
| `[question]` | I don't understand this — help me. |
| `[praise]` | Calling out something done well. |

---

## 3. What to Look For (in priority order)

### Correctness
- Does the logic actually do what the PR description says?
- Are edge cases handled: empty input, null, zero, concurrent access?
- Are errors handled — or silently swallowed?
- Off-by-one errors in loops, slices, pagination?

### Security
- User input validated before use? (SQL injection, XSS, path traversal)
- Auth checks in place? Can an unauthenticated user hit this endpoint?
- Secrets hardcoded? Logged?
- File paths constructed from user data without sanitization?

### Data integrity
- DB writes inside transactions where multiple rows change together?
- Rollback on partial failure?
- Migrations reversible?

### Performance
- N+1 queries (loop that fetches inside the loop)?
- Missing index on a filtered/joined column?
- Large payload loaded into memory when streaming would work?

### Maintainability
- Can you understand this in 6 months without the author?
- Magic numbers with no explanation?
- Logic duplicated across 3+ places instead of extracted?
- Function doing 3 different things?

### Tests
- Happy path + at least one error path covered?
- Test name describes what it tests, not how?
- No assertions that always pass?

---

## 4. What NOT to Block On

Don't block a merge over:
- Personal style preferences not in the agreed linting rules
- "I would have done it differently" when both approaches are correct
- Refactors outside the PR scope ("while you're here, fix X")
- Cosmetic naming when the current name is clear enough

If you want something changed but it's not blocking: use `[suggestion]` or `[nit]`.

---

## 5. Comment Style

**Bad:** "This is wrong."
**Good:** "This will panic if `items` is nil — `len(nil)` returns 0 but the index access on line 42 assumes non-empty. Add an early return."

**Bad:** "Use a map here."
**Good:** "[suggestion] Replacing the slice search with a map lookup would make this O(1) if `users` grows large."

Always include: **what** is the issue, **why** it matters, and (when helpful) **how** to fix it.

---

## 6. Go Checklist

- [ ] Errors returned, not ignored (`_ = err` is a red flag)
- [ ] Error messages lowercase, no trailing punctuation (`fmt.Errorf("failed to fetch user")`)
- [ ] Context passed as first arg to functions that do I/O
- [ ] Goroutines have a clear owner and exit path
- [ ] Mutexes protecting shared state; no data races
- [ ] `defer` used for cleanup (file close, mutex unlock), not in hot loops
- [ ] SQL queries use parameterized inputs, not `fmt.Sprintf`
- [ ] Structs exported only when needed across packages
- [ ] Interface defined at the consumer, not the producer
- [ ] No `init()` side effects that make tests unpredictable

---

## 7. Flutter / Dart Checklist

- [ ] BLoC events/states follow part-file pattern
- [ ] All BLoC handlers private (`_onEventName`)
- [ ] Navigation and SnackBars only in `BlocListener`, never `BlocBuilder`
- [ ] `buildWhen`/`listenWhen` used to prevent unnecessary rebuilds
- [ ] `copyWith` present on states with 2+ fields
- [ ] No business logic in widgets — belongs in BLoC or service
- [ ] `context.read()` only in callbacks, never in `build()`
- [ ] `const` constructors on widgets that never change
- [ ] Dispose controllers, subscriptions, animation controllers
- [ ] No hardcoded strings — use constants or l10n

---

## 8. Nuxt / Vue Checklist

- [ ] No explicit Vue/component/composable imports (auto-imported)
- [ ] `~/` path alias used, not `@/`
- [ ] Business logic in composables, not in `<script setup>`
- [ ] `storeToRefs` used when destructuring Pinia store
- [ ] No `any` types — `unknown` or generics instead
- [ ] No `<style>` blocks — Tailwind only
- [ ] No interpolated Tailwind class names (`:class="\`bg-${color}\`"`)
- [ ] Semantic color tokens used (`bg-default`, `text-muted`), not hardcoded colors
- [ ] Every async action has a loading state
- [ ] Empty and error states both handled with action buttons
- [ ] `FetchError` caught and handled by status code

---

## 9. PR Description Quality

A PR without context wastes reviewer time. Ideal description has:
1. **What** — one sentence summary of the change
2. **Why** — motivation (ticket link, bug report, user request)
3. **How** — notable implementation decisions and trade-offs
4. **Test plan** — how to verify it works

If the PR description is missing, ask before reviewing: "Can you add context on what this is solving? Will review faster."

---

## 10. When to Request Changes vs. Approve with Comments

**Request changes** when:
- There's a correctness or security issue
- The approach is fundamentally wrong and needs rethinking
- Tests are missing for non-trivial logic

**Approve with comments** when:
- Only suggestions/nits remain
- You trust the author to address minor points

**Approve immediately** when:
- The code is correct and the suggestions are truly cosmetic

Don't leave a PR in "changes requested" for nits — that blocks shipping.