← Back

Code Review

Review Priority Order

Check in this order. Earlier items are more likely to be blockers.

  1. Layer correctness — is logic in the right layer? (domain / application / infrastructure / presentation)
  2. Cross-module boundaries — does it import from a module it should not?
  3. Error handling — does it fail early with proper error types?
  4. Naming — do names reflect business language? Any magic numbers?
  5. DDD — are entities behavioral or just data bags? Immutable?
  6. Testing — do tests exist, match scope, use factories, cover edge cases?
  7. Simplification — redundant checks, dead code, over-validation?
  8. Pattern reuse — does something similar already exist?
  9. Schema/API design — designed for consumer needs, not DB shape?
  10. Observability — structured, filterable logs with useful context?
  11. Impact assessment — client impact, migrations, other modules?

Severity Levels

  • must-fix: architecture violations, missing error handling, business logic in wrong layer, security issues, broken test scope, cross-module import violations.
  • suggestion: naming improvements, code simplification, pattern reuse, schema design, logging, DX.
  • question: design intent, client-side impact, release risk, alternative approaches.

Fail-First, Never Silently Proceed

Missing data should throw, not pass through. Use specific error types: NotFoundError, ValidationError, ApplicationError. Generic errors hide problems.

One exception: when throwing would break a critical flow (e.g., booking creation). Log the error and return gracefully. Pragmatism over purity — but only when stakes justify it.

Reuse Before Creating

Before building something new, check if it exists. Reference the exact file and line. Evidence from the codebase is more convincing than theory.

Test Scope Matches Code Scope

  • Private module logic → private tests. Domain logic → unit tests. Infrastructure → integration tests.
  • Use beforeEach over beforeAll — shared state creates flaky tests.
  • Inline snapshots for complex outputs.
  • Verify in dev environment before releasing user-facing changes.

Communication Style

  • Direct and concise. “Fix please” is complete when the problem is obvious. “Why?” is complete when design intent is unclear.
  • Show code suggestions — preferred implementation, not just descriptions.
  • Reference existing code as evidence — link to the file.
  • Create tickets for follow-up — do not block PRs on tangential work.
  • “LGTM” only when truly clean. Can include trailing notes for minor items.

Approval Criteria

  • Approve: no must-fix issues, layers respected, fail-first error handling, naming follows conventions, tests exist and match scope.
  • Changes Requested: any must-fix triggered.
  • Commented: only suggestions and questions, wants discussion before approval.

Evolution

  • Feb 2026 — rewritten in rule-oriented format from ~1,200 PR review patterns.