clean arch violations identified

2026-01-08 00:03:10 +01:00
parent 606b64cec7
commit d984ab24a8
3 changed files with 766 additions and 20 deletions
--- a/docs/ARCHITECTURE.md
+++ b/docs/ARCHITECTURE.md
@@ -361,6 +361,144 @@ Inner layers:

 This keeps the **automation engine** stable and reusable across presentation surfaces (companion today, web or backend orchestrators later), while allowing infrastructure and UI details to evolve without rewriting business logic.

+### 4.2 Critical Clean Architecture Principle: Use Cases Do NOT Call Presenters
+
+**The most important rule in Clean Architecture is that use cases must remain completely independent of presentation concerns.**
+
+#### ❌ WRONG PATTERN (What NOT to do)
+
+```typescript
+// ❌ VIOLATES CLEAN ARCHITECTURE
+class GetRaceDetailUseCase {
+  constructor(
+    private repositories: any,
+    private output: UseCaseOutputPort<GetRaceDetailResult>
+  ) {}
+
+  async execute(input: GetRaceDetailInput): Promise<Result<void, ApplicationError>> {
+    const race = await this.raceRepository.findById(input.raceId);
+    
+    if (!race) {
+      // WRONG: Use case calling presenter
+      const result = Result.err({ code: 'RACE_NOT_FOUND', details: {...} });
+      this.output.present(result);  // ❌ DON'T DO THIS
+      return result;
+    }
+
+    // WRONG: Use case calling presenter
+    const result = Result.ok({ race, league, registrations, drivers, userResult, isUserRegistered, canRegister });
+    this.output.present(result);  // ❌ DON'T DO THIS
+    return result;
+  }
+}
+```
+
+**Why this violates Clean Architecture:**
+- Use cases now **know about presenters** and how to call them
+- Creates **tight coupling** between application logic and presentation
+- Makes use cases **untestable** without mocking presenters
+- Violates the **Dependency Rule** (inner layer depending on outer layer behavior)
+
+#### ✅ CORRECT PATTERN (Clean Architecture)
+
+```typescript
+// ✅ CLEAN ARCHITECTURE - Use case returns data, period
+class GetRaceDetailUseCase {
+  constructor(
+    private repositories: any,
+    private output: UseCaseOutputPort<GetRaceDetailResult>
+  ) {}
+
+  async execute(input: GetRaceDetailInput): Promise<Result<GetRaceDetailResult, ApplicationError>> {
+    const race = await this.raceRepository.findById(input.raceId);
+    
+    if (!race) {
+      return Result.err({ code: 'RACE_NOT_FOUND', details: {...} });
+      // NO .present() call! Just returns the Result.
+    }
+
+    return Result.ok({ race, league, registrations, drivers, userResult, isUserRegistered, canRegister });
+    // NO .present() call! Just returns the Result.
+  }
+}
+```
+
+**The Controller/Wiring Layer (Infrastructure/Presentation):**
+
+```typescript
+// ✅ Controller wires use case to presenter
+class RaceController {
+  constructor(
+    private getRaceDetailUseCase: GetRaceDetailUseCase,
+    private raceDetailPresenter: RaceDetailPresenter
+  ) {}
+
+  async getRaceDetail(params: GetRaceDetailParamsDTO): Promise<RaceDetailDTO> {
+    // 1. Execute use case
+    const result = await this.getRaceDetailUseCase.execute(params);
+    
+    // 2. Pass result to presenter (wiring happens here)
+    this.raceDetailPresenter.present(result);
+    
+    // 3. Get ViewModel from presenter
+    return this.raceDetailPresenter.viewModel;
+  }
+}
+```
+
+### 4.3 The "Presenter Not Presented" Error Explained
+
+Your current architecture has this error because:
+
+1. **Use cases call `.present()`** (violating Clean Architecture)
+2. **Controllers expect presenters to have `.viewModel`**
+3. **But if use case returns early on error without calling `.present()`**, the presenter never gets data
+4. **Controller tries to access `.viewModel`** → throws "Presenter not presented"
+
+**The fix is NOT to add more `.present()` calls to use cases. The fix is to remove ALL `.present()` calls from use cases.**
+
+### 4.4 Your Adapter Pattern is a Smokescreen
+
+Your current code uses adapter classes like:
+
+```typescript
+class RaceDetailOutputAdapter implements UseCaseOutputPort<GetRaceDetailResult> {
+  constructor(private presenter: RaceDetailPresenter) {}
+  
+  present(result: GetRaceDetailResult): void {
+    this.presenter.present(result);
+  }
+}
+```
+
+**This is just hiding the crime.** The adapter still couples the use case to the presenter concept. The real Clean Architecture approach eliminates these adapters entirely and has controllers do the wiring.
+
+### 4.5 The Real Clean Architecture Flow
+
+```
+1. Controller receives HTTP request
+2. Controller calls UseCase.execute()
+3. UseCase returns Result<T, E> (no presenter knowledge)
+4. Controller passes Result to Presenter
+5. Presenter transforms Result → ViewModel
+6. Controller returns ViewModel to HTTP layer
+```
+
+**Key insight:** The use case's `output` port should be **the Result itself**, not a presenter. The controller is responsible for taking that Result and passing it to the appropriate presenter.
+
+### 4.6 What This Means for Your Codebase
+
+**To achieve 100% Clean Architecture, you must:**
+
+1. **Remove all `.present()` calls from use cases** - they should only return Results
+2. **Remove all adapter classes** - they're unnecessary coupling
+3. **Make controllers wire use cases to presenters** - this is where the "glue" belongs
+4. **Use cases return Results, period** - they don't know about presenters, viewmodels, or HTTP
+
+**This is the ONLY way to achieve true Clean Architecture.** Any pattern where use cases call presenters is **not Clean Architecture**, regardless of how many adapter layers you add.
+
+The "presenter not presented" error is a **symptom** of this architectural violation, not the root problem.
+
 ---

 ## 4. Layer-by-Layer Mapping
@@ -717,11 +855,11 @@ This section describes how a typical hosted-session automation run flows through
   - The automation engine (implemented by [`AutomationEngineAdapter`](core/infrastructure/adapters/automation/engine/AutomationEngineAdapter.ts:1) and backed by [`PlaywrightAutomationAdapter`](core/infrastructure/adapters/automation/core/PlaywrightAutomationAdapter.ts:1)) proceeds through steps:

     - Navigate to hosted sessions.
-     - Open “Create a Race” and the hosted-session wizard.
+     - Open "Create a Race" and the hosted-session wizard.
     - For each step (race information, server details, admins, cars, tracks, weather, race options, conditions):
       - Ensure the correct page is active using [`PageStateValidator`](core/domain/services/PageStateValidator.ts:1) and selectors from [`IRACING_SELECTORS`](core/infrastructure/adapters/automation/dom/IRacingSelectors.ts:1).
       - Fill fields and toggles using [`IRacingDomInteractor`](core/infrastructure/adapters/automation/dom/IRacingDomInteractor.ts:1).
-       - Click the correct “Next” / “Create Race” / “New Race” buttons, guarded by [`SafeClickService`](core/infrastructure/adapters/automation/dom/SafeClickService.ts:1) and blocked-selector logic.
+       - Click the correct "Next" / "Create Race" / "New Race" buttons, guarded by [`SafeClickService`](core/infrastructure/adapters/automation/dom/SafeClickService.ts:1) and blocked-selector logic.

   - At each step, the Playwright adapter:
     - Updates the overlay via `updateOverlay(step, message)`.
@@ -741,6 +879,60 @@ This section describes how a typical hosted-session automation run flows through
   - The companion renderer may present a [`RaceCreationResult`](core/domain/value-objects/RaceCreationResult.ts:1) via [`RaceCreationSuccessScreen`](apps/companion/renderer/components/RaceCreationSuccessScreen.tsx).
   - The browser context is closed or re-used based on mode and configuration; debug artifacts may be written by the Playwright adapter for failed runs.

+### 5.2 Clean Architecture Flow Example
+
+**The correct Clean Architecture flow for use cases:**
+
+```typescript
+// ❌ WRONG - Current broken pattern
+class GetRaceDetailUseCase {
+  async execute(input: GetRaceDetailInput): Promise<Result<void, ApplicationError>> {
+    const race = await this.raceRepository.findById(input.raceId);
+    
+    if (!race) {
+      const result = Result.err({ code: 'RACE_NOT_FOUND', details: {...} });
+      this.output.present(result);  // ❌ Use case calling presenter
+      return result;
+    }
+
+    const result = Result.ok({ race, league, registrations, drivers, userResult, isUserRegistered, canRegister });
+    this.output.present(result);  // ❌ Use case calling presenter
+    return result;
+  }
+}
+
+// ✅ CORRECT - Clean Architecture
+class GetRaceDetailUseCase {
+  async execute(input: GetRaceDetailInput): Promise<Result<GetRaceDetailResult, ApplicationError>> {
+    const race = await this.raceRepository.findById(input.raceId);
+    
+    if (!race) {
+      return Result.err({ code: 'RACE_NOT_FOUND', details: {...} });
+      // ✅ No .present() call - just returns Result
+    }
+
+    return Result.ok({ race, league, registrations, drivers, userResult, isUserRegistered, canRegister });
+    // ✅ No .present() call - just returns Result
+  }
+}
+
+// Controller wiring (in infrastructure/presentation layer)
+class RaceController {
+  async getRaceDetail(params: GetRaceDetailParamsDTO): Promise<RaceDetailDTO> {
+    // 1. Call use case
+    const result = await this.getRaceDetailUseCase.execute(params);
+    
+    // 2. Wire to presenter
+    this.raceDetailPresenter.present(result);
+    
+    // 3. Return ViewModel
+    return this.raceDetailPresenter.viewModel;
+  }
+}
+```
+
+**This is the ONLY pattern that respects Clean Architecture.** Your current architecture violates this fundamental principle, which is why you have the "presenter not presented" problem.
+
 ---

 ## 6. How This Serves Admins, Drivers, Teams
--- a/docs/architecture/USECASES.md
+++ b/docs/architecture/USECASES.md
@@ -5,13 +5,13 @@ according to Clean Architecture, in a NestJS-based system.

 The goal is:
 	•	strict separation of concerns
-	•	correct terminology (no fake “ports”)
+	•	correct terminology (no fake "ports")
 	•	minimal abstractions
 	•	long-term consistency

 This is the canonical reference for all use cases in this codebase.

-⸻
+～

 1. Core Concepts (Authoritative Definitions)

@@ -24,7 +24,7 @@ Use Case

 The public execute() method is the input port.

-⸻
+～

 Input
 	•	Pure data
@@ -37,7 +37,7 @@ type GetSponsorsInput = {
 }


-⸻
+～

 Result
 	•	The business outcome of a use case
@@ -50,7 +50,7 @@ type GetSponsorsResult = {
 }


-⸻
+～

 Output Port
 	•	A behavioral boundary
@@ -63,7 +63,7 @@ export interface UseCaseOutputPort<T> {
 }


-⸻
+～

 Presenter
 	•	Implements UseCaseOutputPort<T>
@@ -72,7 +72,7 @@ Presenter
 	•	Holds internal state
 	•	Is pulled by the controller after execution

-⸻
+～

 2. Canonical Use Case Structure

@@ -102,7 +102,85 @@ Rules:
 	•	All output flows through the OutputPort
 	•	The return value signals success or failure only

-⸻
+### ⚠️ ARCHITECTURAL VIOLATION ALERT
+
+**The pattern shown above is INCORRECT and violates Clean Architecture.**
+
+#### ❌ WRONG PATTERN (What NOT to do)
+
+```typescript
+@Injectable()
+export class GetSponsorsUseCase {
+  constructor(
+    private readonly sponsorRepository: ISponsorRepository,
+    private readonly output: UseCaseOutputPort<GetSponsorsResult>,
+  ) {}
+
+  async execute(): Promise<Result<void, ApplicationError>> {
+    const sponsors = await this.sponsorRepository.findAll()
+    
+    this.output.present({ sponsors })  // ❌ WRONG: Use case calling presenter
+    return Result.ok(undefined)
+  }
+}
+```
+
+**Why this violates Clean Architecture:**
+- Use cases **know about presenters** and how to call them
+- Creates **tight coupling** between application logic and presentation
+- Makes use cases **untestable** without mocking presenters
+- Violates the **Dependency Rule** (inner layer depending on outer layer behavior)
+
+#### ✅ CORRECT PATTERN (Clean Architecture)
+
+```typescript
+@Injectable()
+export class GetSponsorsUseCase {
+  constructor(
+    private readonly sponsorRepository: ISponsorRepository,
+    // NO output port needed in constructor
+  ) {}
+
+  async execute(): Promise<Result<GetSponsorsResult, ApplicationError>> {
+    const sponsors = await this.sponsorRepository.findAll()
+    
+    return Result.ok({ sponsors })
+    // ✅ Returns Result, period. No .present() call.
+  }
+}
+```
+
+**The Controller (in API layer) handles the wiring:**
+
+```typescript
+@Controller('/sponsors')
+export class SponsorsController {
+  constructor(
+    private readonly useCase: GetSponsorsUseCase,
+    private readonly presenter: GetSponsorsPresenter,
+  ) {}
+
+  @Get()
+  async getSponsors() {
+    // 1. Execute use case
+    const result = await this.useCase.execute()
+    
+    if (result.isErr()) {
+      throw mapApplicationError(result.unwrapErr())
+    }
+    
+    // 2. Wire to presenter
+    this.presenter.present(result.value)
+    
+    // 3. Return ViewModel
+    return this.presenter.getViewModel()
+  }
+}
+```
+
+**This is the ONLY pattern that respects Clean Architecture.**
+
+～

 Result Model

@@ -116,7 +194,7 @@ Rules:
 	•	No interfaces
 	•	No transport concerns

-⸻
+～

 3. API Layer

@@ -158,7 +236,7 @@ export class GetSponsorsPresenter
 }


-⸻
+～

 Controller

@@ -182,7 +260,7 @@ export class SponsorsController {
 }


-⸻
+～

 Payments Example

@@ -266,7 +344,7 @@ export class PaymentsController {
  }
 }

-⸻
+～

 4. Module Wiring (Composition Root)

@@ -287,7 +365,7 @@ Rules:
 	•	The presenter is bound as the OutputPort implementation
 	•	process.env is not used inside the use case

-⸻
+～

 5. Explicitly Forbidden

@@ -299,7 +377,7 @@ Rules:
 ❌ Mapping logic inside use cases
 ❌ Environment access inside the core

-⸻
+～

 Do / Don’t (Boundary Examples)

@@ -307,6 +385,8 @@ Do / Don’t (Boundary Examples)
 	✅ DO: Keep controllers/services thin and delegating, e.g. [LeagueController.createLeagueSeasonScheduleRace()](apps/api/src/domain/league/LeagueController.ts:291).
 	❌ DON’T: Put business rules in the API layer; rules belong in `./core` use cases/entities/value objects, e.g. [CreateLeagueSeasonScheduleRaceUseCase.execute()](core/racing/application/use-cases/CreateLeagueSeasonScheduleRaceUseCase.ts:38).

+～
+
 6. Optional Extensions

 Custom Output Ports
@@ -322,7 +402,7 @@ interface ComplexOutputPort {
 }


-⸻
+～

 Input Port Interfaces

@@ -335,7 +415,7 @@ Otherwise:

 The use case class itself is the input port.

-⸻
+～

 7. Key Rules (Memorize These)

@@ -348,7 +428,7 @@ Data does not.
 The core produces truth.
 The API interprets it.

-⸻
+～

 TL;DR
 	•	Use cases are injected via DI
@@ -357,4 +437,77 @@ TL;DR
 	•	Results are business models, not DTOs
 	•	Interfaces exist only for behavior variability

-This document is the single source of truth for use case architecture in this project.
+### 🚨 CRITICAL CLEAN ARCHITECTURE CORRECTION
+
+**The examples in this document (sections 2, 3, and the Payments Example) demonstrate the WRONG pattern that violates Clean Architecture.**
+
+#### The Fundamental Problem
+
+The current architecture shows use cases **calling presenters directly**:
+
+```typescript
+// ❌ WRONG - This violates Clean Architecture
+this.output.present({ sponsors })
+```
+
+**This is architecturally incorrect.** Use cases must **never** know about presenters or call `.present()`.
+
+#### The Correct Clean Architecture Pattern
+
+**Use cases return Results. Controllers wire them to presenters.**
+
+```typescript
+// ✅ CORRECT - Use case returns data
+@Injectable()
+export class GetSponsorsUseCase {
+  constructor(private readonly sponsorRepository: ISponsorRepository) {}
+
+  async execute(): Promise<Result<GetSponsorsResult, ApplicationError>> {
+    const sponsors = await this.sponsorRepository.findAll()
+    return Result.ok({ sponsors })
+    // NO .present() call!
+  }
+}
+
+// ✅ CORRECT - Controller handles wiring
+@Controller('/sponsors')
+export class SponsorsController {
+  constructor(
+    private readonly useCase: GetSponsorsUseCase,
+    private readonly presenter: GetSponsorsPresenter,
+  ) {}
+
+  @Get()
+  async getSponsors() {
+    const result = await this.useCase.execute()
+    
+    if (result.isErr()) {
+      throw mapApplicationError(result.unwrapErr())
+    }
+    
+    this.presenter.present(result.value)
+    return this.presenter.getViewModel()
+  }
+}
+```
+
+#### Why This Matters
+
+1. **Dependency Rule**: Inner layers (use cases) cannot depend on outer layers (presenters)
+2. **Testability**: Use cases can be tested without mocking presenters
+3. **Flexibility**: Same use case can work with different presenters
+4. **Separation of Concerns**: Use cases do business logic, presenters do transformation
+
+#### What Must Be Fixed
+
+**All use cases in the codebase must be updated to:**
+1. **Remove** the `output: UseCaseOutputPort<T>` constructor parameter
+2. **Return** `Result<T, E>` directly from `execute()`
+3. **Remove** all `this.output.present()` calls
+
+**All controllers must be updated to:**
+1. **Call** the use case and get the Result
+2. **Pass** `result.value` to the presenter's `.present()` method
+3. **Return** the presenter's `.getViewModel()`
+
+This is the **single source of truth** for correct Clean Architecture in this project.