Diff Review Strategy Agent Skill

[ ] Does the change do exactly what the title says?
[ ] Are edge cases handled (null, empty, out-of-range)?
[ ] Are variable names clear?
[ ] Are tests updated or added?

[ ] Is the design the simplest solution?
[ ] Are error paths handled?
[ ] Is there duplication that should be extracted?
[ ] Does it follow existing patterns in the codebase?
[ ] Are there security implications (user input, auth)?
[ ] Is the test coverage meaningful (not just happy path)?

1. Read the PR description and linked ticket first
2. Review architecture/design before line-by-line reading
3. Request a walkthrough if intent is unclear
4. If XL: comment "Please split by [feature / layer / file]"

[ ] N+1 query? (loop calling DB inside a loop)
[ ] Missing index on filtered/sorted column?
[ ] SELECT * where only specific columns needed?
[ ] Missing pagination on list endpoints?
[ ] Transaction missing on multi-step writes?

[ ] Unnecessary re-renders? (missing memo/useCallback/useMemo)
[ ] Large component re-renders on every keystroke?
[ ] Images missing width/height (layout shift)?
[ ] Heavy dependency imported at top level (should be lazy)?
[ ] Bundle size impact? (check with next build or vite --report)

[ ] Synchronous I/O inside async handler?
[ ] Missing cache for repeated identical queries?
[ ] Large payload returned when only summary needed?
[ ] Polling where event/webhook would be better?

Controller → Service → Repository → DB     (ALLOWED)
Controller → Repository → DB               (VIOLATION: skip service)
Service → Controller                       (VIOLATION: wrong direction)
Repository → Service                       (VIOLATION: wrong direction)

Entities (innermost) — no imports from outer rings
Use Cases — import Entities only
Adapters — import Use Cases and Entities
Frameworks — import everything, imported by nothing

[ ] Does feature A import from feature B's internals?
[ ] If yes, should this be a shared utility or event instead?
[ ] Are feature-specific types leaking into shared modules?

[ ] Hooks called conditionally? (rules of hooks violation)
[ ] Missing key prop in lists? Or key is array index?
[ ] useEffect missing cleanup return for subscriptions/timers?
[ ] State mutation instead of new object? ({ ...prev, field: val })
[ ] Prop drilling 3+ levels deep? (consider context or composition)

[ ] Data fetching happening client-side when it could be server-side?
[ ] 'use client' added unnecessarily to a component with no interactivity?
[ ] Large third-party lib imported in a Server Component?
[ ] Dynamic route missing generateStaticParams for static generation?
[ ] API route missing input validation?

[ ] Middleware added in wrong order? (auth before body-parser?)
[ ] Error handler missing 4-argument signature (err, req, res, next)?
[ ] Async handler missing try/catch or asyncHandler wrapper?
[ ] User input used directly in SQL/file path/shell command?
[ ] Missing rate limiter on public endpoints?

nit: Consider renaming `data` to `userProfile` for clarity — easy to
confuse with the raw API response above.

suggestion: This could use `Promise.all` to run both requests in
parallel instead of sequentially. Would save ~300ms per call.

  // instead of:
  const a = await fetchA()
  const b = await fetchB()

  // consider:
  const [a, b] = await Promise.all([fetchA(), fetchB()])

blocker: This passes `req.params.id` directly into the SQL query
string — SQL injection risk. Use a parameterized query:

  db.query('SELECT * FROM users WHERE id = $1', [req.params.id])

nice: Clean separation of concerns here. The service layer staying
unaware of the HTTP layer makes this easy to test.