rubicanjr/diff-review-strategy

Diff Review Strategy

PR Size Categories and Review Depth

| Category | Lines Changed | Review Depth | Action | |----------|--------------|--------------|--------| | XS | 1–10 | Quick scan | Auto-approve if tests pass, typo/doc fixes | | S | 11–50 | Focused | Check edge cases, naming, one logic path | | M | 51–200 | Thorough | Full logic review, design check, test coverage | | L | 201–500 | Walkthrough | Request author explanation, check design first | | XL | 500+ | Split required | Block merge, ask to split into logical units |

XS/S review checklist

[ ] 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?

M review checklist

[ ] 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)?

L/XL protocol

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]"

Performance Review Checklist

Database

[ ] 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?

React / Frontend

[ ] 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)

General backend

[ ] 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?

Architecture Conformance Checks

Layer violation detection

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

Red flags in the diff:

• import { db } from '../db' inside a route handler file
• import { Router } from 'express' inside a service file
• Direct fetch() calls inside a React component (should be in a hook or service)

Dependency direction (clean architecture)

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

Feature coupling detection

[ ] 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?

Framework-Specific Review Patterns

React

[ ] 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)

Next.js

[ ] 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?

Express

[ ] 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?

Review Comment Templates

Nitpick (non-blocking)

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

Suggestion (non-blocking, better approach available)

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 (must fix before merge)

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])

Praise (leave these too — morale matters)

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

Review Anti-Patterns to Avoid

• Bike-shedding on style that the linter already enforces
• Asking "why not X?" without explaining why X would be better
• Leaving blockers with no suggested fix
• Reviewing XL PRs line by line without first understanding the design
• Approving without reading tests