← Back

Code Review Patterns

Architecture over code style. The biggest blockers are always about wrong-layer placement, not formatting. These are the repeating issues that show up across codebases. Ordered by how often they block PRs.

1. Architecture Layer Enforcement

Severity: must-fix. Business logic inside resolvers, controllers, or mutations belongs in use cases, services, or jobs. Input validation belongs in use cases, not presentation. Domain never depends on infrastructure. Never import from one module into another — use the public API (Facade).

When use cases need to be combined, the facade is the only valid place. Shared folders between schemas prevent independent evolution and can leak sensitive data — duplicate instead.

2. Naming and Conventions

Severity: suggestion. Names must reflect business language. Magic numbers must become named constants. File names must match their exports. When a name is unclear, ask what the domain calls it.

  • Hide complexity behind simple names. translate(hotels) is better than translateIfNeeded(hotels).
  • When naming conflicts do not exist, use the simple name. regionGroups over expediaRegionGroups if there is only one.
  • Follow existing naming patterns in the codebase before inventing new ones.

3. Code Cleanliness

Severity: suggestion. Commented-out code should be deleted, not kept. Dead imports, unused constants, console.logs, and redundant conditionals are noise. If code is repeated multiple times, extract it. If unrelated changes snuck into a PR, roll them back.

The rule: do not comment, just delete the line. Version control remembers.

4. DDD Enforcement

Severity: must-fix. Entities must be behavioral (methods and computed properties), not data bags. Entities must be immutable — always return new objects to avoid side effects. When a query selects only specific columns, the return type is a new interface, not the full domain entity.

  • Use object parameters when a function takes more than 2 arguments.
  • Domain services belong in the context that owns the logic, not in the caller.
  • Facades are the only place where use cases can be combined.

5. Fail-First Error Handling

Severity: must-fix. Missing data should throw, not pass through silently. Use specific error types (NotFoundError, ValidationError) — generic ApplicationError hides the real problem.

One exception: when throwing would break a critical flow (payment processing, booking creation). In that case, log the error and return gracefully. Pragmatism over purity — but only when the stakes justify it.

Error levels depend on business impact, not error type. A ValidationFailed could be critical. A NotFound could be expected.

6. Testing Rigor

Severity: must-fix / suggestion. Test scope must match code scope. Private module logic gets private tests. Domain logic gets unit tests. Infrastructure gets integration tests.

  • Use beforeEach over beforeAll — shared state creates flaky tests.
  • Use inline snapshots for complex outputs.
  • Question unexpected test modifications — why did test code change?
  • Demand edge case tests — empty inputs, boundary values, outside-range scenarios.
  • Verify in dev environment before releasing user-facing changes.

7. Database and ORM

Severity: suggestion / must-fix. Use the ORM for all queries. Raw SQL is the exception and must be justified with performance data. Consistency and type safety matter more than marginal performance gains.

  • Always chunk large WHERE IN queries. Without a limit, production databases hit query size limits under load. Default chunk size should be generous when not sorting.
  • Question magic numbers in timeouts and batch sizes — they must have data backing them.
  • Do not expose ORM internals (Prisma client) outside its package.

8. API and Schema Design

Severity: suggestion. Schema serves consumers, not the database. Never share types between public and private schemas — duplicate if needed.

  • Split fat mutations. Each mutation handles one use case. updateUserPreferredLocale and updateUserPreferredCurrency — not a generic updateUser.
  • Design fields for specific use cases — single-purpose fields, not multi-purpose ones.
  • Understand consumer needs first. Ask for the Figma. Discuss the schema before implementing.
  • Offset pagination by default. Cursor pagination only for infinite scroll.

9. Pattern Reuse

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

When an existing pattern solves the problem, use it. When a new implementation looks similar to an existing one, check for differences before duplicating. Use existing utilities (groupBy, withTranslation, etc.) instead of reimplementing.

10. Observability and Logging

Severity: suggestion. Every log must include consistent filterable identifiers — providerCode, propertyId, userId. Use domain language in log identifiers, not internal database IDs.

  • Keep the same base data on related logs — enables correlation across entries.
  • Be specific in messages. “Property has no room_type in DB” is better than “not found.”
  • Do not duplicate what the framework provides — timestamps, locations are automatic.
  • Log AI agent inputs and outputs — essential for debugging AI-driven flows.

11. Impact Assessment

Severity: question. Cross-module changes, schema migrations, client-facing modifications, and cascade deletes all need impact questions before approval.

Ask: does this change require client-side changes? Does deleting this data affect how existing records display? Is backward compatibility needed? Has this been tested in a dev environment?

Create follow-up tickets for tangential work rather than blocking the current PR. Pragmatic pacing — no rush when risk is high.

12. Redundant Logic Elimination

Severity: suggestion. When the structure already guarantees context (private folder = private scope), remove conditionals that check for it. Trust the layer you are in.

  • Remove unnecessary intermediate variables when the data is already available.
  • Question silenced warnings — do not disable linters without investigating the root cause.
  • Accept minor performance costs for readability. A new object for clarity is acceptable.

13. Documentation and AI Rules Quality

Severity: suggestion. Rules files must be minimal and inline. Never use @ imports in rules — this forces loading all rules and breaks the purpose of separation. Keep code examples simplified to what matters.

  • Document the “why” alongside the “what” — AI and humans both need context.
  • If references to actual files are not trustworthy, include the content directly.
  • Cross-reference related docs when rules span multiple files.

Decision Criteria

When reviewing a PR, check architecture layers first, naming second. Earlier items in the pattern list are more likely to be blockers. A single must-fix issue means changes requested — no exceptions. When only suggestions and questions remain, comment and discuss. LGTM only when truly clean.

The review serves the codebase, not the PR. Every merged PR becomes the standard the next PR follows. Low standards compound.

Anti-patterns

  • Participation trophy approvals — LGTM without reading the code.
  • Style-only reviews — catching formatting while missing architecture violations.
  • Blocking on tangential work — creating tickets is better than holding up progress.
  • Theory without evidence — “this is better” without pointing to the existing pattern or data.
  • Reviewing only the diff — changes need context. Check what the code does, not just what changed.