Nestjs Code Review

NestJS Code Review

Overview

Provides structured code review for NestJS applications. Findings categorized by severity (Critical, Warning, Suggestion) with actionable recommendations. Delegates to nestjs-code-review-expert agent for deep analysis.

When to Use

• "review NestJS code", "NestJS code review", "check my NestJS controller/service"
• Before merging pull requests or after implementing new features
• Validating NestJS decorators, DI patterns, guard implementations
• Architecture validation for NestJS modules and providers
• Reviewing DTOs, pipes, interceptors, and database integration (TypeORM, Prisma, Drizzle)

Instructions

1. Identify Scope: Determine which NestJS files and modules are under review. Use glob and grep to discover controllers, services, modules, guards, interceptors, and pipes in the target area.

2. Analyze Module Structure: Verify proper module organization — each feature should have its own module with clearly defined imports, controllers, providers, and exports. Check for circular dependencies and proper module boundaries.

3. Review Dependency Injection: Validate that all injectable services use constructor injection. Check provider scoping (singleton, request, transient) matches the intended lifecycle. Ensure no direct instantiation bypasses the DI container.

4. Evaluate Controllers: Review HTTP method usage, route naming, status codes, request/response DTOs, validation pipes, and OpenAPI decorators. Confirm controllers are thin — business logic belongs in services.

5. Assess Services & Business Logic: Check that services encapsulate business logic properly. Verify error handling, transaction management, and proper separation from infrastructure concerns. Look for service methods that are too large or have too many responsibilities.

6. Check Security: Review guard implementations, authentication/authorization patterns, input validation with class-validator, and protection against common vulnerabilities (injection, XSS, CSRF).

7. Review Testing: Assess test coverage for controllers, services, guards, and pipes. Verify proper mocking strategies and that tests validate behavior, not implementation details.

8. Validate Findings (Required checkpoint): Before finalizing, verify each Critical and Warning finding has reproducible evidence (file path, line numbers, exact code snippet) and a concrete, actionable fix. Remove or downgrade findings that are style preferences, overly subjective, or lack concrete remediation.

9. Produce Review Report: Generate structured report with severity-classified findings (Critical, Warning, Suggestion), positive observations, and prioritized recommendations with code examples.

Examples

Example 1: Reviewing a Controller

// ❌ Bad: Fat controller with business logic and missing validation
@Controller('users')
export class UserController {
  constructor(private readonly userRepo: Repository<User>) {}

  @Post()
  async create(@Body() body: any) {
    const user = this.userRepo.create(body);
    return this.userRepo.save(user);
  }
}

// ✅ Good: Thin controller with proper DTOs, validation, and service delegation
@Controller('users')
@ApiTags('Users')
export class UserController {
  constructor(private readonly userService: UserService) {}

  @Post()
  @HttpCode(HttpStatus.CREATED)
  @ApiOperation({ summary: 'Create a new user' })
  @ApiResponse({ status: 201, type: UserResponseDto })
  async create(
    @Body(ValidationPipe) createUserDto: CreateUserDto,
  ): Promise<UserResponseDto> {
    return this.userService.create(createUserDto);
  }
}

Example 2: Reviewing Dependency Injection

// ❌ Bad: Direct instantiation bypasses DI
@Injectable()
export class OrderService {
  private readonly logger = new Logger();
  private readonly emailService = new EmailService();

  async createOrder(dto: CreateOrderDto) {
    this.emailService.send(dto.email, 'Order created');
  }
}

// ✅ Good: Proper constructor injection
@Injectable()
export class OrderService {
  private readonly logger = new Logger(OrderService.name);

  constructor(
    private readonly orderRepository: OrderRepository,
    private readonly emailService: EmailService,
  ) {}

  async createOrder(dto: CreateOrderDto): Promise<Order> {
    const order = await this.orderRepository.create(dto);
    await this.emailService.send(dto.email, 'Order created');
    return order;
  }
}

Example 3: Reviewing Error Handling

// ❌ Bad: Generic error handling with information leakage
@Get(':id')
async findOne(@Param('id') id: string) {
  try {
    return await this.service.findOne(id);
  } catch (error) {
    throw new HttpException(error.message, 500);
  }
}

// ✅ Good: Domain-specific exceptions with proper HTTP mapping
@Get(':id')
async findOne(@Param('id', ParseUUIDPipe) id: string): Promise<UserResponseDto> {
  const user = await this.userService.findOne(id);
  if (!user) {
    throw new NotFoundException(`User with ID ${id} not found`);
  }
  return user;
}

Example 4: Reviewing Guard Implementation

// ❌ Bad: Authorization logic in controller
@Get('admin/dashboard')
async getDashboard(@Req() req: Request) {
  if (req.user.role !== 'admin') {
    throw new ForbiddenException();
  }
  return this.dashboardService.getData();
}

// ✅ Good: Guard-based authorization with decorator
@Get('admin/dashboard')
@UseGuards(JwtAuthGuard, RolesGuard)
@Roles(Role.ADMIN)
async getDashboard(): Promise<DashboardDto> {
  return this.dashboardService.getData();
}

Example 5: Reviewing Module Organization

// ❌ Bad: Monolithic module with everything
@Module({
  imports: [TypeOrmModule.forFeature([User, Order, Product, Review])],
  controllers: [UserController, OrderController, ProductController],
  providers: [UserService, OrderService, ProductService, ReviewService],
})
export class AppModule {}

// ✅ Good: Feature-based module organization
@Module({
  imports: [UserModule, OrderModule, ProductModule],
})
export class AppModule {}

@Module({
  imports: [TypeOrmModule.forFeature([User])],
  controllers: [UserController],
  providers: [UserService, UserRepository],
  exports: [UserService],
})
export class UserModule {}

Review Output Format

Structure all code review findings as follows:

1. Summary

Brief overview with an overall quality score (1-10) and key observations.

2. Critical Issues (Must Fix)

Issues that could cause security vulnerabilities, data corruption, or production failures.

3. Warnings (Should Fix)

Issues that violate best practices, reduce maintainability, or could lead to bugs.

4. Suggestions (Consider Improving)

Improvements for code readability, performance, or developer experience.

5. Positive Observations

Well-implemented patterns and good practices to acknowledge and encourage.

6. Recommendations

Prioritized next steps with code examples for the most impactful improvements.

Best Practices

• Controllers should be thin — delegate all business logic to services
• Use DTOs with class-validator for all request/response payloads
• Apply ParseUUIDPipe, ParseIntPipe, etc. for parameter validation
• Use domain-specific exception classes extending HttpException
• Organize code into feature modules with clear boundaries and exports
• Prefer constructor injection — never use new for injectable services
• Apply guards for authentication and authorization, not inline checks
• Use interceptors for cross-cutting concerns (logging, caching, transformation)
• Add OpenAPI decorators (@ApiTags, @ApiOperation, @ApiResponse) to all endpoints
• Write unit tests for services and integration tests for controllers

Constraints and Warnings

• Do not enforce a single ORM — the codebase may use TypeORM, Prisma, Drizzle, or MikroORM
• Respect existing project conventions even if they differ from NestJS defaults
• Focus on high-confidence issues — avoid false positives on style preferences
• When reviewing microservices patterns, consider transport-layer specific constraints
• Do not suggest architectural rewrites unless critical issues warrant them

References

See the references/ directory for detailed review checklists and pattern documentation:

• references/patterns.md — NestJS best practice patterns with examples
• references/anti-patterns.md — Common NestJS anti-patterns to flag during review
• references/checklist.md — Comprehensive review checklist organized by area

NestJS Anti-Patterns

Controller Anti-Patterns

Fat Controllers

Business logic in controllers makes code untestable and violates single responsibility.

// ❌ Anti-pattern: Business logic in controller
@Post('register')
async register(@Body() dto: RegisterDto) {
  const existing = await this.userRepo.findOne({ email: dto.email });
  if (existing) throw new ConflictException('Email exists');

  const salt = await bcrypt.genSalt(10);
  const hash = await bcrypt.hash(dto.password, salt);

  const user = this.userRepo.create({ ...dto, password: hash });
  await this.userRepo.save(user);

  const token = this.jwtService.sign({ sub: user.id });
  await this.emailService.sendWelcome(user.email);

  return { user, token };
}

Fix: Move all logic to a service, keep the controller thin.

Direct Repository Access in Controllers

Controllers should never access repositories directly — always go through a service layer.

// ❌ Anti-pattern
@Controller('products')
export class ProductController {
  constructor(
    @InjectRepository(Product)
    private readonly productRepo: Repository<Product>,
  ) {}
}

// ✅ Fix: Inject the service instead
@Controller('products')
export class ProductController {
  constructor(private readonly productService: ProductService) {}
}

Service Anti-Patterns

God Service

A service that handles too many concerns becomes a maintenance nightmare.

// ❌ Anti-pattern: Service handling users, orders, payments, emails
@Injectable()
export class AppService {
  async createUser() { /* ... */ }
  async processOrder() { /* ... */ }
  async chargePayment() { /* ... */ }
  async sendEmail() { /* ... */ }
  async generateReport() { /* ... */ }
}

Fix: Split into focused services — UserService, OrderService, PaymentService, etc.

Tight Coupling to Infrastructure

Services that directly depend on infrastructure (HTTP clients, file system, specific databases) are hard to test and replace.

// ❌ Anti-pattern: Direct infrastructure dependency
@Injectable()
export class NotificationService {
  async send(userId: string, message: string) {
    const response = await fetch('https://api.sendgrid.com/v3/mail/send', {
      method: 'POST',
      headers: { Authorization: `Bearer ${process.env.SENDGRID_KEY}` },
      body: JSON.stringify({ to: userId, content: message }),
    });
    return response.json();
  }
}

// ✅ Fix: Use an interface/abstraction
interface EmailProvider {
  send(to: string, content: string): Promise<void>;
}

@Injectable()
export class NotificationService {
  constructor(
    @Inject('EMAIL_PROVIDER')
    private readonly emailProvider: EmailProvider,
  ) {}

  async send(userId: string, message: string): Promise<void> {
    const user = await this.userService.findOne(userId);
    await this.emailProvider.send(user.email, message);
  }
}

Dependency Injection Anti-Patterns

Manual Instantiation

Bypassing the DI container loses lifecycle management and testability.

// ❌ Anti-pattern
@Injectable()
export class OrderService {
  private logger = new Logger(); // Not managed by DI
  private cache = new CacheService(); // Not injectable, not mockable

  async process() {
    this.logger.log('Processing');
  }
}

Circular Dependencies

Two services depending on each other create circular dependency issues.

// ❌ Anti-pattern: Circular dependency
@Injectable()
export class UserService {
  constructor(private orderService: OrderService) {}
}

@Injectable()
export class OrderService {
  constructor(private userService: UserService) {}
}

// ✅ Fix: Use forwardRef or restructure with events
@Injectable()
export class OrderService {
  constructor(
    @Inject(forwardRef(() => UserService))
    private userService: UserService,
  ) {}
}

// ✅ Better fix: Break the cycle with an event
@Injectable()
export class OrderService {
  constructor(private eventEmitter: EventEmitter2) {}

  async create(dto: CreateOrderDto) {
    const order = await this.save(dto);
    this.eventEmitter.emit('order.created', { userId: dto.userId });
  }
}

Module Anti-Patterns

Monolithic AppModule

Placing all controllers, services, and entities in AppModule destroys modularity.

// ❌ Anti-pattern
@Module({
  imports: [TypeOrmModule.forFeature([User, Order, Product, Invoice, Report])],
  controllers: [UserCtrl, OrderCtrl, ProductCtrl, InvoiceCtrl, ReportCtrl],
  providers: [UserSvc, OrderSvc, ProductSvc, InvoiceSvc, ReportSvc],
})
export class AppModule {}

Over-Exporting

Exporting everything from a module breaks encapsulation.

// ❌ Anti-pattern: Exporting internal implementation details
@Module({
  providers: [UserService, UserRepository, PasswordHasher, UserMapper],
  exports: [UserService, UserRepository, PasswordHasher, UserMapper],
})
export class UserModule {}

// ✅ Fix: Export only the public API
@Module({
  providers: [UserService, UserRepository, PasswordHasher, UserMapper],
  exports: [UserService], // Only what other modules need
})
export class UserModule {}

Error Handling Anti-Patterns

Swallowing Errors

Catching errors without proper handling or logging hides bugs.

// ❌ Anti-pattern
async findUser(id: string) {
  try {
    return await this.repo.findOne(id);
  } catch (error) {
    return null; // Error silently swallowed
  }
}

Exposing Internal Errors

Sending raw error messages or stack traces to clients leaks internal details.

// ❌ Anti-pattern
catch (error) {
  throw new HttpException(error.stack, 500); // Stack trace exposed
}

Testing Anti-Patterns

Testing Implementation Instead of Behavior

Tests that assert on internal method calls are brittle and break on refactoring.

// ❌ Anti-pattern: Testing implementation
it('should call repository.save', async () => {
  await service.create(dto);
  expect(mockRepo.save).toHaveBeenCalledWith(expect.objectContaining(dto));
});

// ✅ Fix: Test behavior
it('should create a user and return it', async () => {
  const result = await service.create(dto);
  expect(result.email).toBe(dto.email);
  expect(result.id).toBeDefined();
});

No Test Isolation

Tests that depend on shared state or execution order are fragile.

// ❌ Anti-pattern: Shared mutable state between tests
describe('UserService', () => {
  const users = []; // Shared state

  it('creates a user', () => {
    users.push(createUser()); // Mutates shared state
    expect(users).toHaveLength(1);
  });

  it('checks user count', () => {
    expect(users).toHaveLength(1); // Depends on previous test
  });
});

Configuration Anti-Patterns

Hardcoded Configuration

Configuration values hardcoded in source code can't be changed per environment.

// ❌ Anti-pattern
@Injectable()
export class DatabaseService {
  private readonly host = 'localhost';
  private readonly port = 5432;
  private readonly password = 'secretpassword';
}

// ✅ Fix: Use ConfigModule
@Injectable()
export class DatabaseService {
  constructor(private readonly configService: ConfigService) {}

  getConfig() {
    return {
      host: this.configService.getOrThrow('DB_HOST'),
      port: this.configService.get('DB_PORT', 5432),
    };
  }
}

NestJS Code Review Checklist

Module Structure

• [ ] Each feature has its own module with clear boundaries
• [ ] Modules export only the necessary public API
• [ ] No circular module dependencies
• [ ] Shared utilities are in a dedicated SharedModule
• [ ] Global modules (@Global()) are used sparingly
• [ ] Lazy-loaded modules for performance-critical applications

Controllers

• [ ] Controllers are thin — no business logic
• [ ] Proper HTTP methods and status codes
• [ ] All parameters validated with pipes (ParseUUIDPipe, ParseIntPipe, etc.)
• [ ] Request bodies validated with DTOs and ValidationPipe
• [ ] OpenAPI decorators present (@ApiTags, @ApiOperation, @ApiResponse)
• [ ] Proper error responses documented
• [ ] No direct repository or database access
• [ ] Route versioning applied consistently

Services

• [ ] Single responsibility — each service owns one domain concern
• [ ] Constructor injection for all dependencies
• [ ] No direct instantiation (new) of injectable services
• [ ] Proper error handling with domain-specific exceptions
• [ ] Transaction management for atomic operations
• [ ] Logging with proper context (Logger with class name)
• [ ] No hardcoded values — use ConfigService

DTOs and Validation

• [ ] Request DTOs with class-validator decorators
• [ ] Response DTOs that exclude sensitive fields
• [ ] Proper type definitions (no any)
• [ ] Validation error messages are user-friendly
• [ ] Nested objects properly validated with @ValidateNested and @Type
• [ ] Array items validated with @ArrayMinSize, @ArrayMaxSize
• [ ] Optional fields marked with @IsOptional

Dependency Injection

• [ ] All services use constructor injection
• [ ] Proper provider scoping (Singleton, Request, Transient)
• [ ] Custom providers use proper tokens (string or Symbol)
• [ ] No circular dependencies (or resolved with forwardRef)
• [ ] Interface-based injection with @Inject token for abstraction

Guards and Authorization

• [ ] All protected routes have authentication guards
• [ ] Role-based authorization uses guards + decorators, not inline checks
• [ ] Guards are composable and reusable
• [ ] Custom decorators for extracting user context (@CurrentUser)
• [ ] Public routes explicitly marked with @Public or similar

Interceptors and Middleware

• [ ] Cross-cutting concerns handled by interceptors (logging, caching, transformation)
• [ ] Response format is consistent across all endpoints
• [ ] Middleware used for request-level concerns (CORS, compression, logging)
• [ ] No business logic in interceptors or middleware

Error Handling

• [ ] Global exception filter registered
• [ ] Domain-specific exception classes extend HttpException
• [ ] Error responses don't expose internal details (stack traces, SQL errors)
• [ ] Proper HTTP status codes for different error types
• [ ] Unhandled promise rejections caught
• [ ] Meaningful error messages for debugging

Database Integration

• [ ] Repository pattern separates data access from business logic
• [ ] Queries use parameterized inputs (no string concatenation)
• [ ] Database transactions for multi-step operations
• [ ] Migrations exist for schema changes
• [ ] Connection pooling configured for production
• [ ] No N+1 query problems (use eager loading or batch queries)

Security

• [ ] Input validation on all endpoints
• [ ] Authentication guard on protected routes
• [ ] Rate limiting configured (ThrottlerModule)
• [ ] CORS properly configured
• [ ] Security headers (helmet) enabled
• [ ] Sensitive data not logged
• [ ] Environment variables for secrets

Testing

• [ ] Unit tests for services with mocked dependencies
• [ ] Integration tests for controllers with e2e setup
• [ ] Guard and pipe tests
• [ ] Test coverage meets project thresholds
• [ ] Tests are independent and isolated
• [ ] Mocking strategy is consistent

Performance

• [ ] Database queries optimized (indexes, selected fields)
• [ ] Caching applied for frequently accessed data
• [ ] Async operations don't block the event loop
• [ ] Pagination implemented for list endpoints
• [ ] Large payloads use streaming when appropriate
• [ ] Connection pools properly sized

Documentation

• [ ] OpenAPI/Swagger decorators on all endpoints
• [ ] API examples in decorators
• [ ] README updated for new features
• [ ] Inline comments for complex business logic only

NestJS Best Practice Patterns

Controller Patterns

Thin Controllers

Controllers should only handle HTTP concerns — route mapping, request parsing, response formatting. All business logic belongs in services.

// ✅ Thin controller delegating to service
@Controller('orders')
@ApiTags('Orders')
export class OrderController {
  constructor(private readonly orderService: OrderService) {}

  @Post()
  @HttpCode(HttpStatus.CREATED)
  @ApiOperation({ summary: 'Create an order' })
  @ApiResponse({ status: 201, type: OrderResponseDto })
  async create(
    @Body(ValidationPipe) dto: CreateOrderDto,
    @CurrentUser() user: AuthUser,
  ): Promise<OrderResponseDto> {
    return this.orderService.create(dto, user.id);
  }

  @Get(':id')
  @ApiOperation({ summary: 'Get order by ID' })
  async findOne(
    @Param('id', ParseUUIDPipe) id: string,
  ): Promise<OrderResponseDto> {
    return this.orderService.findOneOrFail(id);
  }
}

Proper HTTP Status Codes

• 200 — Successful GET, PUT, PATCH
• 201 — Successful POST that creates a resource
• 204 — Successful DELETE with no response body
• 400 — Validation failure
• 401 — Missing or invalid authentication
• 403 — Valid authentication but insufficient permissions
• 404 — Resource not found
• 409 — Conflict (duplicate resource)
• 422 — Unprocessable entity (business rule violation)

Service Patterns

Single Responsibility Services

Each service should own one domain concern. Avoid "god services" that handle multiple unrelated operations.

@Injectable()
export class OrderService {
  constructor(
    private readonly orderRepository: OrderRepository,
    private readonly paymentService: PaymentService,
    private readonly eventEmitter: EventEmitter2,
  ) {}

  async create(dto: CreateOrderDto, userId: string): Promise<Order> {
    const order = await this.orderRepository.create({ ...dto, userId });
    await this.paymentService.processPayment(order);
    this.eventEmitter.emit('order.created', new OrderCreatedEvent(order));
    return order;
  }
}

Transaction Management

Use database transactions for operations that must be atomic.

@Injectable()
export class TransferService {
  constructor(private readonly dataSource: DataSource) {}

  async transfer(from: string, to: string, amount: number): Promise<void> {
    await this.dataSource.transaction(async (manager) => {
      await manager.decrement(Account, { id: from }, 'balance', amount);
      await manager.increment(Account, { id: to }, 'balance', amount);
    });
  }
}

Module Patterns

Feature Module Organization

Each feature should be a self-contained module with clear exports.

@Module({
  imports: [
    TypeOrmModule.forFeature([Order, OrderItem]),
    PaymentModule,
  ],
  controllers: [OrderController],
  providers: [OrderService, OrderRepository],
  exports: [OrderService], // Only export what other modules need
})
export class OrderModule {}

Dynamic Module Pattern

Use for configurable shared modules.

@Module({})
export class CacheModule {
  static forRoot(options: CacheOptions): DynamicModule {
    return {
      module: CacheModule,
      global: true,
      providers: [
        { provide: CACHE_OPTIONS, useValue: options },
        CacheService,
      ],
      exports: [CacheService],
    };
  }
}

Guard and Interceptor Patterns

Composable Guards

Stack guards for layered security.

@UseGuards(JwtAuthGuard, RolesGuard, ThrottlerGuard)
@Roles(Role.ADMIN)
@Controller('admin')
export class AdminController {}

Response Transformation Interceptor

Standardize API responses.

@Injectable()
export class TransformInterceptor<T> implements NestInterceptor<T, ApiResponse<T>> {
  intercept(context: ExecutionContext, next: CallHandler): Observable<ApiResponse<T>> {
    return next.handle().pipe(
      map(data => ({
        success: true,
        data,
        timestamp: new Date().toISOString(),
      })),
    );
  }
}

DTO Patterns

Request DTOs with Validation

Always validate incoming data with class-validator decorators.

export class CreateUserDto {
  @IsString()
  @IsNotEmpty()
  @MaxLength(100)
  @ApiProperty({ example: 'John Doe' })
  name: string;

  @IsEmail()
  @ApiProperty({ example: 'john@example.com' })
  email: string;

  @IsString()
  @MinLength(12)
  @Matches(/^(?=.*[a-z])(?=.*[A-Z])(?=.*\d)/, {
    message: 'Password must contain uppercase, lowercase, and number',
  })
  password: string;

  @IsEnum(UserRole)
  @IsOptional()
  @ApiPropertyOptional({ enum: UserRole, default: UserRole.USER })
  role?: UserRole = UserRole.USER;
}

Response DTOs with Exclusion

Never expose internal fields (passwords, internal IDs) in responses.

export class UserResponseDto {
  @ApiProperty()
  id: string;

  @ApiProperty()
  name: string;

  @ApiProperty()
  email: string;

  @ApiProperty({ enum: UserRole })
  role: UserRole;

  @Exclude()
  password: string;

  @Exclude()
  deletedAt: Date;

  constructor(partial: Partial<UserResponseDto>) {
    Object.assign(this, partial);
  }
}

Error Handling Patterns

Domain Exception Classes

Create domain-specific exceptions for clear error semantics.

export class OrderNotFoundException extends NotFoundException {
  constructor(orderId: string) {
    super(`Order with ID ${orderId} not found`);
  }
}

export class InsufficientBalanceException extends UnprocessableEntityException {
  constructor(required: number, available: number) {
    super(`Insufficient balance: required ${required}, available ${available}`);
  }
}

Global Exception Filter

Centralize error formatting for consistent API responses.

@Catch()
export class GlobalExceptionFilter implements ExceptionFilter {
  catch(exception: unknown, host: ArgumentsHost) {
    const ctx = host.switchToHttp();
    const response = ctx.getResponse<Response>();

    const status = exception instanceof HttpException
      ? exception.getStatus()
      : HttpStatus.INTERNAL_SERVER_ERROR;

    const message = exception instanceof HttpException
      ? exception.message
      : 'Internal server error';

    response.status(status).json({
      success: false,
      statusCode: status,
      message,
      timestamp: new Date().toISOString(),
    });
  }
}