Code Reviewer

Code Review

Review recent code changes for quality, security, maintainability, and adherence to project patterns.

Instructions

1. Get Recent Changes

Run git diff to see uncommitted changes. If no uncommitted changes, run git diff HEAD~1 to review the last commit.

Focus on modified files and analyze them thoroughly.

2. Review Checklist

Apply this checklist to all changed code:

Security (CRITICAL)

• Hardcoded credentials (API keys, passwords, tokens)
• SQL injection risks (string concatenation in queries - should use Knex parameterized queries)
• XSS vulnerabilities (unescaped user input in responses)
• Missing input validation (DTOs should use Zod schemas)
• Authentication bypasses (missing @UseGuards(JwtAuthGuard))
• Path traversal risks (user-controlled file paths)
• CSRF vulnerabilities
• Insecure dependencies (outdated, vulnerable)

Clean Architecture (CRITICAL)

Clean Architecture - API (API only):

Dependency Rule Violations:

• ❌ core/ MUST NEVER import from adapters/ - Check all imports in core files
• ❌ UseCases depending on concrete implementations (e.g., SqlRepository) instead of interfaces
• ❌ Domain models importing infrastructure code
• ✅ Core should only import from core/, shared-kernel/, or "shared" package

Layer Violations:

• Controllers containing business logic (should only handle HTTP → UseCase → HTTP)
• Business logic in repositories/queries (should be in UseCases)
• Direct database access in controllers (should go through UseCases)

Clean Architecture - Web (Web only):

Dependency Rule Violations:

• ❌ core/ importing from infrastructure/ or views/ directories
• ❌ Presentational components with business logic
• ✅ Core should only import from core/ or "shared" package

Example violation:

// BAD - core importing from infrastructure
// File: src/features/create-site/core/createSite.reducer.ts
import { HttpSiteService } from "../../infrastructure/site-service/HttpSiteService";

// GOOD - core depending on interface only
// File: src/features/create-site/core/createSite.reducer.ts
import type { SiteService } from "./gateways/SiteService";

Redux Architecture (CRITICAL - Web only)

ESTABLISHED Pattern:

• ❌ Refactoring Redux to other state management (Zustand, Jotai, MobX)
• ❌ Using createSlice for new code (legacy - maintenance only)
• ❌ Actions using imperative tense (completeStep, fetchData)
• ✅ Use event-based Redux with createReducer
• ✅ Actions in passive tense (stepCompleted, dataFetched)

Example violations:

// BAD - createSlice in new code
export const newFeatureSlice = createSlice({
  name: 'newFeature',
  initialState,
  reducers: { /* ... */ }
});

// GOOD - createReducer for new code
export const newFeatureReducer = createReducer(initialState, (builder) => {
  builder.addCase(featureDataFetched, (state, action) => { /* ... */ });
});

// BAD - imperative action names
export const fetchSiteData = createAction("site/fetch");
export const completeSiteCreation = createAction("site/complete");

// GOOD - passive/event-based action names
export const siteDataFetched = createAction("site/dataFetched");
export const siteCreationCompleted = createAction("site/creationCompleted");

UseCase Pattern (CRITICAL - API only)

Result Pattern:

• ❌ UseCases not implementing UseCase<Request, TResult<Response, Error>>
• ❌ UseCases not returning TResult<Data, Error>
• ❌ Using throw for domain errors instead of fail("ErrorType")
• ❌ Error types that are action-based ("CreateFailed") instead of state-based ("AlreadyExists", "NotFound")
• ✅ All UseCases should use success(data) or fail("ErrorType")

Dependency Injection:

• ❌ UseCases depending on concrete classes (e.g., SqlSiteRepository) instead of interfaces
• ❌ Missing gateway interfaces in core/gateways/
• ✅ Constructor parameters should be typed as interfaces, not concrete classes

Controller Pattern (CRITICAL - API only)

DTOs:

• ❌ DTOs defined in controller files (should be in /packages/shared/src/api-dtos/)
• ❌ Missing Zod schema validation with ZodValidationPipe
• ❌ Business logic validation in DTOs (should be in UseCases)
• ✅ All request/response DTOs should be in shared package with both schema + type export

Authentication:

• ❌ Routes missing @UseGuards(JwtAuthGuard) (unless explicitly public)
• ❌ Unprotected routes without clear justification

Result Mapping:

• ❌ Not handling Result errors (missing if (result.isFailure()) check)
• ❌ Inconsistent HTTP status mapping (e.g., *NotFound should throw NotFoundException)
• ❌ Returning domain entities instead of ViewModels

Container/Presentational Pattern (HIGH - Web only)

ViewData Pattern:

• ❌ Multiple useAppSelector calls in container
• ❌ Selectors not following select{Feature}ViewData naming
• ❌ Presentational components using Redux hooks
• ✅ Single ViewData selector per container
• ✅ Presentational components receive all data via props

Example violations:

// BAD - multiple selectors in container
const Container = () => {
  const sites = useAppSelector(selectSites);
  const loading = useAppSelector(selectLoading);
  const error = useAppSelector(selectError);
  // ...
};

// GOOD - single ViewData selector
const Container = () => {
  const viewData = useAppSelector(selectSitesViewData);
  return <SitesPage viewData={viewData} />;
};

// BAD - presentational component with Redux
const SitesPage = () => {
  const sites = useAppSelector(selectSites); // ❌ NO REDUX IN PRESENTATIONAL
  return <div>{sites.map(...)}</div>;
};

// GOOD - presentational component with props only
const SitesPage = ({ viewData }: { viewData: SitesViewData }) => {
  return <div>{viewData.sites.map(...)}</div>;
};

Redux Hooks (HIGH - Web only)

• ❌ Using untyped useSelector or useDispatch from react-redux
• ✅ Always use useAppSelector and useAppDispatch from @/shared/views/hooks/store.hooks

Example violation:

// BAD - untyped hooks
import { useSelector, useDispatch } from 'react-redux';
const data = useSelector(state => state.feature); // No type safety
const dispatch = useDispatch();

// GOOD - typed hooks
import { useAppSelector, useAppDispatch } from '@/shared/views/hooks/store.hooks';
const data = useAppSelector(selectFeatureData); // Fully typed
const dispatch = useAppDispatch();

CQS (Command-Query Separation) (HIGH - API only)

Repository vs Query:

• ❌ Repository interfaces mixing reads and writes
• ❌ Query interfaces with write methods
• ❌ Write UseCases using Query interfaces for data modification
• ✅ Write operations should use Repository, read-only operations should use Query

Database Patterns (CRITICAL - API only)

Migrations:

• ❌ Direct database schema changes without migration
• ❌ Missing down() function in migrations
• ❌ Missing indexes on foreign keys or frequently queried columns

Table Types:

• ❌ New tables without corresponding Sql* type in tableTypes.d.ts
• ❌ Using any type for SQL operations
• ❌ Optional properties (?:) instead of nullable (| null) in table types
• ❌ Using string for timestamp columns (should be Date - Knex auto-converts)
• ✅ All SQL table types should use snake_case column names

Naming Convention:

• ❌ Database columns using camelCase (should be snake_case)
• ❌ Application properties using snake_case (should be camelCase)
• ✅ Repository code should map between snake_case (DB) and camelCase (app)

Transactions:

• ❌ Multi-table operations without transactions
• ✅ Operations spanning multiple tables should use sqlConnection.transaction()

Testing Pattern (CRITICAL)

Testing - API (API only):

InMemory Implementations:

• ❌ New Repository/Query interfaces without InMemory implementation
• ❌ Missing test helper methods (prefixed with _) in InMemory classes
• ❌ Unit tests accessing real database (should use InMemory)

Unit Tests:

• ❌ UseCase instantiation in beforeEach() (should be in each it())
• ❌ Using toMatchObject() when toEqual() is appropriate (less exhaustive)
• ❌ Missing failure path tests (only testing happy path)
• ❌ Not verifying side effects (e.g., data saved, not saved on failure)
• ❌ Using real services (random IDs, current dates) instead of deterministic ones

Event Testing:

• ❌ Success paths not asserting published events
• ❌ Error paths not asserting NO events published
• ❌ Event assertions without complete type (should use expect().toEqual<EventType>(...))
• ✅ Use InMemoryEventPublisher and assert eventPublisher.events length and content

Integration Tests:

• ❌ SQL Repository/Query without integration test file (*.integration-spec.ts)
• ❌ Controller without integration test
• ✅ Integration tests should test against real database (testcontainers)

Testing - Web (Web only):

• ❌ Unit tests using HTTP services (should use InMemory)
• ❌ Not using builder pattern for test state
• ❌ Hardcoded test data instead of builders
• ✅ Use StoreBuilder pattern for test setup
• ✅ InMemory services for all external dependencies

Reference: src/features/create-project/core/urban-project/__tests__/_testStoreHelpers.ts

Example violation:

// BAD - hardcoded test state
it("should display sites", () => {
  const store = configureStore({
    preloadedState: {
      sites: {
        list: [{ id: "1", name: "Site 1" }],
        loading: false
      }
    }
  });
  // ...
});

// GOOD - builder pattern
it("should display sites", () => {
  const store = new StoreBuilder()
    .withSiteData({ id: "1", name: "Site 1" })
    .build();
  // ...
});

Dependency Injection (HIGH - API only)

NestJS Modules:

• ❌ Not using factory pattern (useFactory) for providers
• ❌ Missing inject array in factory providers
• ❌ Not importing required modules (SqlConnectionModule, EventPublisherModule)
• ❌ Using injection tokens for UseCases (should use direct injection of RealEventPublisher)
• ✅ UseCases should inject RealEventPublisher directly, not via token

Gateway Pattern (HIGH)

Gateway Pattern - API (API only):

See Controller Pattern section for API gateway checks.

Gateway Pattern - Web (Web only):

• ❌ HTTP services without InMemory mock implementation
• ❌ HTTP services not validating DTOs with Zod safeParse()
• ❌ Services not registered in store's extraArgument
• ❌ Thunks not accessing services via extra parameter
• ✅ HTTP implementation + InMemory mock required
• ✅ Validate request/response with shared Zod schemas

Reference: src/features/onboarding/infrastructure/create-user-service/HttpCreateUserService.ts

Example violation:

// BAD - no DTO validation
async createUser(data: CreateUserDto) {
  const response = await fetch('/api/users', {
    method: 'POST',
    body: JSON.stringify(data) // ❌ No validation
  });
  return response.json(); // ❌ No validation
}

// GOOD - Zod validation
import { createUserDtoSchema, createUserResponseSchema } from "shared";

async createUser(data: CreateUserDto) {
  const validated = createUserDtoSchema.safeParse(data);
  if (!validated.success) throw new Error("Invalid request");

  const response = await fetch('/api/users', {
    method: 'POST',
    body: JSON.stringify(validated.data)
  });

  const json = await response.json();
  const result = createUserResponseSchema.safeParse(json);
  if (!result.success) throw new Error("Invalid response");
  return result.data;
}

Naming Conventions (HIGH)

Naming Conventions - API (API only):

File Naming:

• ❌ UseCase files not following [verb][Noun].usecase.ts pattern
• ❌ Repository files not following Sql[Name]Repository.ts pattern
• ❌ Query files not following Sql[Name]Query.ts pattern
• ❌ Test files not matching source file name with .spec.ts or .integration-spec.ts

Import Aliases:

• ❌ Using relative imports (../../../) for cross-module (should use @/)
• ❌ Using @/ for shared package (should use "shared")
• ❌ Long import chains when path alias would be clearer

Naming Conventions - Web (Web only):

Actions:

• ❌ Actions using imperative tense (completeStep, fetchData)
• ✅ Actions in passive tense (stepCompleted, dataFetched)

Selectors:

• ❌ Selectors not following select{Feature}ViewData pattern
• ✅ One ViewData selector per container component

Reducers:

• ❌ Using createSlice in new code (legacy - maintenance only)
• ✅ Use createReducer for new reducers

Import Aliases:

• ❌ Using relative imports (../../../) for cross-feature imports (should use @/)
• ❌ Using @/ for shared package (should use "shared")
• ✅ Use @/ for imports within web app
• ✅ Use "shared" for shared package imports
• ✅ Use relative ./ or ../ only within same feature folder

Example violations:

// BAD - relative import for cross-feature
import { selectUserData } from "../../../user/core/user.selectors";

// GOOD - use @/ alias
import { selectUserData } from "@/features/user/core/user.selectors";

// BAD - using @/ for shared package
import type { Site } from "@/shared";

// GOOD - use "shared" for package imports
import type { Site } from "shared";

TypeScript & Node.js Compatibility (CRITICAL)

Erasable Types:

• ❌ TypeScript enum usage (use Zod z.enum() or const object instead)
• ❌ Namespace declarations
• ⚠️ Class parameter properties in new code (legacy allowed, but avoid in new code)
• ✅ Use type over interface unless extending
• ✅ Use Zod schemas for enum-like types

Type Safety:

• ❌ Using any type (use unknown when type is truly unknown)
• ❌ Missing explicit return types on public functions/methods
• ❌ Not using import type { } for type-only imports
• ✅ strict: true compliance

Code Quality (HIGH)

Function/File Size:

• Large functions (>50 lines) - consider breaking down
• Large files (>800 lines) - consider splitting by responsibility
• Deep nesting (>4 levels) - refactor for readability

Error Handling:

• Missing error handling in async operations
• console.log statements left in production code (use proper logging)
• Swallowing errors without logging

State Management:

• Mutation of shared state
• Missing immutability in domain models

Performance (MEDIUM)

• Inefficient algorithms (O(n²) when O(n log n) possible)
• N+1 query patterns (should use joins or batch queries)
• Missing database indexes on frequently queried columns
• Unnecessary re-renders in React
• Missing memoization opportunities in React
• Large bundle size additions

Best Practices (MEDIUM)

• TODO/FIXME without tickets or context
• Missing JSDoc for complex public APIs
• Poor variable naming (x, tmp, data, result without context)
• Magic numbers without explanation
• Inconsistent formatting (should run pnpm format)

3. Confidence-Based Filtering

Only report issues you are highly confident about:

• ✅ HIGH Confidence - Clear violations visible in the diff:

  • Missing @UseGuards(JwtAuthGuard) on route
  • core/ importing from adapters/
  • UseCase not returning TResult<>
  • TypeScript enum usage
  • Missing table type after migration

• ⚠️ MEDIUM Confidence - Probable but needs context:

  • Report with caveat: "Possible issue (verify context)..."
  • Large functions/files (may be justified)
  • Missing tests (may exist elsewhere)

• ❌ LOW Confidence - Skip reporting:

  • Require deep codebase knowledge
  • May have valid reasons not visible in diff
  • Speculative or style-only concerns

When in doubt, skip the issue - focus on clear, actionable problems.

4. Output Format

For each issue found, use this format:

[SEVERITY] Issue Title
File: path/to/file.ts:line
Issue: Description of the problem
Fix: How to resolve it

// Bad
<code snippet showing the problem>

// Good
<code snippet showing the fix>

Severity Levels:

• CRITICAL: Security, architecture violations, type safety issues
• HIGH: Missing tests, pattern violations, significant bugs
• MEDIUM: Performance, maintainability, code quality
• LOW: Style, minor improvements

5. ADR Check

After reviewing code quality, evaluate whether the changes involve an architectural decision worth documenting:

• New pattern or abstraction introduced
• New library or tool adopted
• Convention established that future code should follow
• Significant structural change

If any of these apply, add a note at the end of the review:

💡 **ADR suggestion**: These changes introduce [brief description]. Consider running `/adr` to document this architectural decision.

If changes follow existing patterns, skip this section silently.

6. Summary

End with a summary:

## Review Summary

- Critical: X issues
- High: X issues
- Medium: X issues
- Low: X issues

## Verdict

- ✅ APPROVE: No Critical or High issues
- ⚠️ CAUTION: Medium issues only (can proceed with awareness)
- ❌ BLOCK: Critical or High issues must be fixed

Scope

$ARGUMENTS

If no scope provided, review all uncommitted changes.

Important Notes

• Focus on high-confidence issues only - don't report speculative concerns
• Prioritize architecture violations:
  • API: Clean Architecture, Result pattern, CQS
  • Web: Clean Architecture, Redux patterns, Container/Presentational separation
• Check security (authentication, SQL injection, input validation)
• Verify testing:
  • API: InMemory implementations, unit + integration tests
  • Web: Builder pattern, InMemory services
• Validate naming:
  • API: Files, database columns, imports
  • Web: Actions (passive tense), selectors (ViewData), imports
• Ensure type safety (no enums, no any, proper Result usage for API)
• Check established patterns:
  • Web: Redux is ESTABLISHED - no refactoring to Zustand/Jotai/MobX
  • Web: Use createReducer for new code, not createSlice
• Be concise but actionable - provide clear fixes with code examples

Converted and distributed by TomeVault — claim your Tome and manage your conversions.