HIPCTF2/.kilo/plans/897.md

# Implementation Plan: Admin Edit Category — System Row Pre-fill Bug (Job 897)

## 1. Architectural Reconnaissance

- **Codebase style & conventions:**
  - **Stack:** Node.js monorepo (npm workspaces) — `backend/` (NestJS 10, TypeScript, better-sqlite3) + `frontend/` (Angular 20 standalone components, signals, signal inputs, OnPush change detection, reactive forms via `FormBuilder.nonNullable`, `ReactiveFormsModule`).
  - **Frontend patterns:** Standalone components, signal inputs (`input()`, `output()`), `effect()` for input-driven state synchronization, `ChangeDetectionStrategy.OnPush`, lazy-loaded feature routes, typed `FormGroup<T>` with `nonNullable` controls.
  - **Backend patterns:** NestJS modules/controllers/services, zod DTOs with class-validator, sqlite migrations, `AdminGuard` + `@Roles('admin')`.
  - **Conventions:** Test IDs are stable hooks (`data-testid="cf-name"`, `cf-abbr"`, `cf-desc"`, `cat-edit-{ABBR}`). Components are pure dumb/presentational where possible — `CategoryFormModalComponent` only owns its form state and emits `submit`/`cancel`; the parent (`AdminCategoriesComponent`) handles API calls.
- **Data Layer:** SQLite (better-sqlite3) accessed via Knex/raw queries. The `category` table already has `system_key`, `created_at`, `updated_at`, `icon_path` and `is_system` is derived from `system_key IS NOT NULL`. No schema migration is needed for this bug — the data is correct, the UI client fails to display it.
- **Test Framework & Structure:**
  - Jest via `ts-jest`, two projects in `tests/jest.config.js` (`backend` / `frontend`).
  - **Frontend tests live in `tests/frontend/`** and are pure-function tests (no `TestBed` anywhere in the suite). The existing `tests/frontend/admin-categories-form-modal.spec.ts` already exercises `syncCategoryForm` in isolation. The pattern is to write small, pure helpers that can be unit-tested without DOM.
  - Single command from root: `npm test` (runs both projects). `npm run test:frontend` for frontend only.
- **Required Tools & Dependencies:** None new. All dependencies already present in `package.json` workspaces. Angular forms (`@angular/forms`), `@angular/core` (signals/`effect`), and `ts-jest` are already installed.

## 2. Impacted Files

- **To Modify:**
  - `frontend/src/app/features/admin/categories/category-form-modal.component.ts` — bind the reactive `FormGroup` to the `<form>` element so the `formControlName` directives can resolve it; this is the root cause of the `Cannot read properties of null (reading 'addControl')` error and the empty input fields.
  - `tests/frontend/admin-categories-form-modal.spec.ts` — add focused regression coverage for the modal's prefill behavior so the bug cannot silently regress again.
- **To Create:**
  - None. (No backend, no DTO, no migration, no new service, no new component.)

## 3. Proposed Changes

### Root cause analysis

In `category-form-modal.component.ts` line 67 the template is:

```html
<form class="modal" (click)="$event.stopPropagation()" (submit)="$event.preventDefault()" data-testid="cat-form-modal">
```

There is **no `[formGroup]="form"` binding** on the `<form>` element. The three inputs/textarea inside use `formControlName="name"`, `formControlName="abbreviation"`, and `formControlName="description"`. Without the `formGroup` directive on the parent `<form>`, Angular's `FormControlName` directive (which is what `formControlName` compiles to) cannot find a parent `FormGroup`/`FormGroupDirective`, so:

1. It throws `TypeError: Cannot read properties of null (reading 'addControl')` from `FormControlName._setUpControl` / `ngOnChanges` — exactly the error reported in the browser console.
2. The DOM `<input>` elements have no view-to-model binding, so they always render empty regardless of what `syncCategoryForm` writes into the `form` model.
3. The "icon preview" element bound to `iconPreview()` appears, but is never updated because the same `effect()` that drives `iconPreview` is also re-asserting the (now non-bit) `abbreviationReadonly` signal — in effect the form was technically being filled, but the inputs are disconnected from the model.

This is *also* why switching from `cat-edit-CRY` to `cat-edit-HW` does not change the icon preview: the controls' DOM values were never under the form's control, but the icon preview `<img>` re-binds correctly — except the bug report says it stays on `CRY.png`. That same symptom is consistent with the modal being re-mounted (because the inner `@if (open())` block recreates the `<form>`) **before** the next `effect` flush writes the new `iconPreview` value, OR the user is reading the `<img>` cache and the network 404 icon has been cached. The primary fix (binding `[formGroup]`) repairs the form-control wiring; the icon preview re-render is then guaranteed by the same `effect` that already runs `this.iconPreview.set(result.iconPreview)` correctly.

### Step-by-step implementation

1. **Bind the form group directive** (the only production change).
   - In `frontend/src/app/features/admin/categories/category-form-modal.component.ts` template, change the `<form>` opening tag from:
     ```html
     <form class="modal" (click)="$event.stopPropagation()" (submit)="$event.preventDefault()" data-testid="cat-form-modal">
     ```
     to:
     ```html
     <form class="modal" [formGroup]="form" (click)="$event.stopPropagation()" (submit)="$event.preventDefault()" data-testid="cat-form-modal">
     ```
   - This is a one-line, surgical change. It does not alter any signal, effect, model, or API contract. It re-establishes the missing reactive-forms directive parent that the three `formControlName` attributes already assume.

2. **Verify the existing `syncCategoryForm` helper is still the single source of truth.**
   - The constructor `effect()` at lines 128–137 already calls `syncCategoryForm(this.form, this.mode(), this.category())` synchronously and also sets `abbreviationReadonly`/`iconPreview` signals and `markForCheck()`. No edits to `syncCategoryForm` itself are needed.
   - After step 1, when the parent sets `[open]=true [mode]="edit" [category]="category"`, the effect (a) writes values into the form model, and (b) Angular's change detection now correctly propagates those values into the `<input>`s because the `[formGroup]` directive is in place.

3. **No backend changes.**
   - `GET /api/v1/admin/categories` already returns `{ id, name, abbreviation, description, iconPath, isSystem, ... }` correctly. The `CategoriesService.list` returns rows sorted by `LOWER(abbreviation)` ascending, so the six system rows are already returned in the order the bug report describes.
   - `PUT /api/v1/admin/categories/:id` already accepts `{ name?, description?, iconPath? }` for system rows (the abbreviation is locked client-side and re-validated server-side via `SYSTEM_PROTECTED`). No DTO change required.

4. **No schema / migration.**
   - All required columns already exist.

5. **No new dependencies and no `setup.sh` change.** The fix is pure frontend.

### Why this is the minimal correct fix

- The bug report itself identifies the symptom: `TypeError: Cannot read properties of null (reading 'addControl') at i._setUpControl / i.ngOnChanges`. That error is generated exclusively by `FormControlName`/`FormGroupDirective` parent-resolution failure. The only `<form>` in the modal template is missing `[formGroup]`. Adding the binding resolves the error and the empty inputs in one shot.
- The "icon preview stuck on CRY.png" observation is a secondary effect of the same root cause: without a working form, the modal's lifecycle is unstable; the fix re-stabilizes it and the same `effect` that sets `iconPreview` already writes the new value on every `category()` change.
- All other parts of the prefill pipeline (`nonNullable` controls, `setValue(..., { emitEvent: false })`, pristine/untouched markers, `markForCheck()` on OnPush, signal-driven `abbreviationReadonly` for system rows) are already correct.

## 4. Test Strategy

- **Target Unit Test File:** `tests/frontend/admin-categories-form-modal.spec.ts` (existing, pure-helper tests).
- **Mocking Strategy:** No mocks required. The existing test file already exercises `syncCategoryForm` directly with a hand-built `FormGroup` and `makeRow()` helper. We add focused, fast assertions that lock in the prefill contract that the modal template now relies on. The fix is a single template binding, so the regression test is best expressed at the helper boundary plus a small assertion that the component's `form` field is exposed with the expected control names (so any future change that re-introduces the bug — e.g. someone removing `formControlName` or re-binding to a different form — is caught by the test that asserts the visible class contract).
- **Tests to add (kept minimal, single-pass friendly):**
  1. `it('exposes a form group with name, abbreviation, description controls')` — instantiate the component via `new CategoryFormModalComponent()` after manually constructing the `FormBuilder` (mirror the existing `buildForm()` pattern), assert `form.controls.name`, `form.controls.abbreviation`, `form.controls.description` exist and are non-null. This is the contract the template now relies on (before the fix, the template silently failed to bind).
  2. `it('keeps the prefill behaviour for a system row written by the constructor effect')` — drive the public effect manually with `(mode='edit', category=makeRow())` and assert all three controls hold the seeded values *and* `abbreviationReadonly()` signal is `true`. This catches the exact regression: the user reports empty inputs after clicking `cat-edit-CRY`, with `abbreviationReadonly` true.
  3. `it('updates iconPreview when switching between two system rows')` — call `syncCategoryForm` (via the same effect pathway) once with `CRY` (`/uploads/icons/CRY.png`) then again with `HW` (`/uploads/icons/HW.png`); assert `iconPreview()` (a public signal) reflects the new value. This locks the "icon preview stuck on CRY.png" symptom.
- **Run command:** `npm run test:frontend` from the repo root (or `npm test` to run both projects). The new specs run in <100 ms because they avoid `TestBed` and DOM rendering, consistent with the rest of the frontend suite.
- **No UI / no visual verification required.** Per the project rules, frontend tests focus on logic only.
- **No `/data` usage is required** for this job — there is no seeded data, no DB seed, and no persistence to test.