First code review after the vertical slice

The player-management vertical slice was complete.

Frontend, API gateway, service, and database worked together. Players could be created, displayed, and deactivated. Access control worked. The UI was polished.

From a product standpoint: a success.

From an architecture standpoint: a problem.

What looked good at first glance

The implementation had real strengths.

The vertical slice structure was preserved. Players code lived with players. Backend validation, tenant context, soft delete, indexes — all correctly in place. A PrimeNG DataView instead of a table. Transloco fully wired up. No hardcoded labels.

That was not nothing.

But then came the frontend domain layer.

The drift

Anyone comparing this feature with the app’s existing domain layer — say, todos or members-directory — sees immediately that two fundamentally different patterns had emerged.

The expected pattern:

Component dispatches intent via facade
  → facade calls injectDispatch (void, no promise)
    → eventGroup: deactivateRequested
      → CommandStore withEventHandlers
        → exhaustMap → infrastructure/players.command.ts
          → mapResponse
            → deactivateSucceeded | deactivateFailed
              → ReadStore: reload via reloadOnWriteSuccess$
              → CommandStore: notification via notificationEvents

What actually got built:

// Component
private async deactivateConfirmedPlayer(id: string): Promise<void> {
  this.deleteFeedback.set(null);
  this.deleteErrorDetail.set(null);
  try {
    await this.facade.deactivatePlayer(id);
    this.deleteFeedback.set('success');
  } catch (error) {
    this.deleteFeedback.set('error');
    this.deleteErrorDetail.set(this.toErrorMessage(error));
  }
}

Five .set() calls. async/await through every layer. try/catch as the error channel. The component orchestrates the use case itself.

This isn’t a style issue. It’s an architectural break.

Why this is dangerous even though it works

This is exactly where the trap lies.

Working code isn’t the same as code that holds up. The difference usually only becomes visible once the next feature arrives, once two requests come in at the same time, or once the first developer reopens the code six months later.

Race conditions. The deactivatePlayer pattern doesn’t use exhaustMap. That means: double-clicking delete sends two parallel PATCH requests. No protection, no backpressure. The reactive pattern with exhaustMap structurally rules exactly that out.

No event system, no layering. The domain lib had no +state/events/, application/, or infrastructure/ folders. There was no eventGroup, no withEventHandlers, no injectDispatch. HttpClient sat directly in the signal store. URL normalization got repeated three times. A custom toErrorMessage was defined both in the store and in the component.

Component as mini-controller. The architecture rule states: the component is a UI adapter, not an orchestrator. Here, the component managed five local signals, drove the use-case flow itself, and threaded error handling through every layer. That’s exactly what “presentation must not contain business orchestration” means.

Feedback via local state instead of notification events. The monorepo has a notification-events lib that exists for exactly this purpose: dispatching feedback from the CommandStore, centralized and decoupled. Instead, deleteFeedback and deleteErrorDetail got kept as signals in the component — parallel data sources for the same domain event.

Any single piece of this is fixable. The pattern is the problem.

Why the drift happened

This isn’t a question of agent capability. It’s a question of guidance quality.

CLAUDE.md and frontend.md described the event-driven architecture pattern in one line:

Event-driven UI flows: user intent → facade → command store
→ infrastructure → success/failure event → read store → VM.

That’s a statement of intent, not an implementation instruction.

An LLM has a strong prior for what it knows from training examples: async/await, try/catch, firstValueFrom, imperative signal mutations. That’s the simplest, most familiar Angular pattern. Without explicit counter-guidance, that’s exactly what gets used — especially when the feature to implement looks small and manageable.

The reference project (libs/todos/domain) was sitting in the repo. But a passively present pattern isn’t an active constraint. There’s no instruction to read it before implementation. No folder structure prescribed. No list of explicit prohibitions.

So the model learned what it could from the context — and then used the most obvious pattern.

What changed so this doesn’t happen again

Two places got updated.

frontend.md was extended with three new sections:

• the mandatory structure of a domain lib (+state/events/, +state/, application/, infrastructure/) with file naming and purpose comments
• the complete command flow as a code example for eventGroup, withEventHandlers, exhaustMap, mapResponse, injectDispatch, and notificationEvents — each with a reference to the concrete file in libs/todos/domain
• an explicit list of prohibitions: no firstValueFrom in stores, no async/await for HTTP, no HttpClient in withProps, no promise-returning facades, no feedback state in components, no resource.reload() from components

AGENTS.md got a mandatory reading list for every frontend domain implementation:

libs/todos/domain/src/lib/+state/todos-command.store.ts
libs/todos/domain/src/lib/+state/todos-read.store.ts
libs/todos/domain/src/lib/application/todos-list.facade.ts
libs/todos/domain/src/lib/infrastructure/todos.command.ts
libs/todos/domain/src/lib/infrastructure/todos.resource.ts
libs/todos/domain/src/lib/infrastructure/todos.mapper.ts

The reference project is no longer passive knowledge. It’s now an active mandatory step.

What got refactored

The domain lib got deleted entirely and rebuilt following the correct pattern.

Before: three flat files, no layering, no event system.

After:

libs/tournament/players/domain/src/lib/
├── +state/
│   ├── events/
│   │   ├── players-create.events.ts
│   │   ├── players-deactivate.events.ts
│   │   ├── players-read.events.ts
│   │   └── players-ui.events.ts
│   ├── players-command.store.ts
│   ├── players-read.store.ts
│   └── players-read.mapper.ts
├── application/
│   └── players-list.facade.ts
└── infrastructure/
    ├── players.command.ts
    ├── players.endpoints.ts
    ├── players.mapper.ts
    └── players.resource.ts

PlayersCommandStore now handles HTTP via exhaustMap and mapResponse. PlayersListFacade uses injectDispatch and returns void. The infrastructure functions are pure observables with no async/await. httpResource has a parse: callback backed by a zod schema. Notifications come from the store via notificationEvents, not from the component.

The component simplified accordingly. No try/catch, no feedback signals, no async. Dialog visibility lives in the ReadStore and reacts to createSucceeded.

The backend, API gateway, and database were correct and stayed unchanged.

What this review shows

The actually interesting part of this drift isn’t the result — it’s how it came about.

The code wasn’t built carelessly. It delivered a working UI, with correct auth, a correct database structure, complete i18n, clean styling. If you only check whether the feature works, you see nothing problematic.

But if you compare the pattern with the rest of the project, you see it immediately: two architectures got built at the same time.

That’s the real danger of drift.

Not the one bad commit. But the first feature that establishes a deviating pattern — which every following feature then adopts as an apparently valid standard.

Schedule, tournaments, group phase, tournament day — they’re all coming. And they’ll follow the first pattern they find in the codebase.

That’s why this review isn’t optional. And that’s why the reset was the right call.

An agent orients itself on existing patterns. If the first domain feature is clean, the odds are higher that every subsequent feature will be clean too. If the first feature drifts, the next drift is already baked in.