Implementation Plan: Admin Area General Settings — Actionable Page-Title Validation Message (Job 884)

0. Status — Already Implemented?

Not fully implemented. The reactive-form plumbing that disables the Save button (Validators.required, custom pageTitleNotBlankValidator, [disabled]="submitting() || form.invalid") works correctly for the empty / whitespace-only negative case, and the field's classes (ng-dirty ng-invalid ng-touched) update as expected. However, the actionable inline error message required by this Job is not shown to the user.

Concrete gap (verified in source):

frontend/src/app/features/admin/general.component.ts:199-202 — showPageTitleError is an Angular computed() that reads this.form.controls.pageTitle.touched, .dirty, .invalid. These are not signals, so the computed never re-evaluates when the control's status flips.
frontend/src/app/features/admin/general.component.ts:204-217 — pageTitleMessage is a computed() that reads this.form.controls.pageTitle.value and .errors. Same problem: the computed is not subscribed to the form control's valueChanges / statusChanges, so it never re-runs after the user types.
Result: the template <div ... data-testid="general-pageTitle-error">{{ pageTitleMessage() }}</div> at line 53 stays at Angular's  placeholder even after the field is ng-dirty ng-invalid ng-touched.

Therefore a code change is required.

1. Architectural Reconnaissance

Codebase style & conventions: TypeScript, NestJS REST API + Angular 17+ standalone components with ChangeDetectionStrategy.OnPush, signal-based state (signal, computed, effect), reactive forms with fb.nonNullable.group(...), takeUntilDestroyed() for RxJS → signal bridging. Validators and pure helpers live in sibling *.pure.ts files (e.g. general.pure.ts). data-testid attributes are the contract for every input, button, and status element.
Data Layer: SQLite via better-sqlite3 (per package.json); persisted key/value settings in the setting table. No schema migration is needed for this Job — only the frontend inline validation message is broken.
Test Framework & Structure: Jest 29 with ts-jest, jsdom environment for frontend. All tests live in /repo/tests/ (never alongside source). Frontend spec files are pure-function tests on *.pure.ts validators/helpers (e.g. admin-general-pure.spec.ts, landing-markdown.spec.ts, change-password-modal.spec.ts) — there are currently no TestBed component-level tests in the repo. npm test runs both backend and frontend projects via tests/jest.config.js.
Required Tools & Dependencies: None new. The fix uses Angular primitives (signal, computed, toSignal from @angular/core/rxjs-interop, takeUntilDestroyed) already present in the file. No setup.sh changes, no new packages.

2. Impacted Files

To Modify:
- frontend/src/app/features/admin/general.component.ts — fix the reactivity gap for showPageTitleError and pageTitleMessage so the inline <div data-testid="general-pageTitle-error"> renders when the field is touched && invalid.
To Create:
- tests/frontend/admin-general-page-title-error.spec.ts — pure-function unit tests covering the new pageTitleError branches the component will rely on (already partially covered, but we extend them to lock down the specific "whitespace-only" → 'required' message that drives the actionable UX).

(No backend changes, no schema migration, no new routes, no DTO changes.)

3. Proposed Changes

3.1 Backend / API

None. The PUT /api/v1/admin/general/settings route, DTO, and general.service.ts are unchanged. The server-side trim + 1–120 validation already returns 400 VALIDATION_FAILED and preserves the stored value — exactly the behavior the Job reports on the negative path. The bug is purely client-side rendering of the error message.

3.2 Frontend Logic — `frontend/src/app/features/admin/general.component.ts`

The root cause is that computed() does not re-evaluate when reactive-form control flags change. Replace the non-reactive reads with signal-backed local state that IS subscribed to valueChanges and statusChanges.

Concrete edits:

Add explicit reactive signals for the pageTitle control, declared at the top of the class alongside the other signals:

private readonly pageTitleValue = signal<string>('');
private readonly pageTitleInvalid = signal<boolean>(false);
private readonly pageTitleTouchedOrDirty = signal<boolean>(false);

Wire the control to those signals in the existing constructor, right after the welcomeMarkdown.valueChanges subscription (using the same takeUntilDestroyed(this.destroyRef) pattern already in the file):
```
const pt = this.form.controls.pageTitle;
this.pageTitleValue.set(pt.value);
this.pageTitleInvalid.set(pt.invalid);
pt.valueChanges
  .pipe(takeUntilDestroyed(this.destroyRef))
  .subscribe((v) => this.pageTitleValue.set(v));
pt.statusChanges
  .pipe(takeUntilDestroyed(this.destroyRef))
  .subscribe(() => {
    this.pageTitleInvalid.set(pt.invalid);
    this.pageTitleTouchedOrDirty.set(pt.touched || pt.dirty);
  });
pt.events
  ?.pipe(takeUntilDestroyed(this.destroyRef))
  ?.subscribe?.(); // no-op safety; see note below
```
Note: do NOT introduce a third subscription — the blur/input events that flip touched/dirty are dispatched by the DOM and re-validate via Angular's value sync, which already emits a statusChanges (and, when the value changes, a valueChanges). If touched/dirty flips without a status change (rare — e.g. focus loss while value is unchanged), also subscribe to the Angular events observable filtered by EventType.Blur:
```
import { EventType } from '@angular/forms';
...
pt.events
  .pipe(
    filter((e) => e.type === EventType.Blur),
    takeUntilDestroyed(this.destroyRef),
  )
  .subscribe(() => this.pageTitleTouchedOrDirty.set(pt.touched || pt.dirty));
```
Confirm with a quick grep that EventType is exported from @angular/forms in the installed Angular version before relying on it; if not available in this version, fall back to a focusout listener bound via @HostListener('focusout') on a directive, OR simply call this.pageTitleTouchedOrDirty.set(pt.touched || pt.dirty) after every valueChanges AND statusChanges emission — both reliably fire on input/blur in modern Angular reactive forms. Preferred approach (no extra imports): set pageTitleTouchedOrDirty inside the existing statusChanges subscription AND inside the existing valueChanges subscription (covering both dirty/touched transitions).

Replace the broken computeds so they read from the new signals:

readonly showPageTitleError = computed(
  () => this.pageTitleTouchedOrDirty() && this.pageTitleInvalid(),
);

readonly pageTitleMessage = computed(() => {
  const err = pageTitleError(this.pageTitleValue());
  if (err === 'required') return 'Page title is required and cannot contain only whitespace.';
  if (err === 'maxlength') return 'Page title must be 120 characters or fewer.';
  // fallback to the raw control errors so the {whitespace:true} branch from
  // the custom validator still surfaces the same human-readable text
  const controlErrors = this.form.controls.pageTitle.errors;
  if (controlErrors?.['whitespace']) return 'Page title is required and cannot contain only whitespace.';
  return null;
});

The existing pageTitleError helper in general.pure.ts:3-8 already returns 'required' for both empty and whitespace-only strings (line 5: if (v.trim().length === 0) return 'required';), so the actionable message will fire for '', ' ', '\t\n ', etc., exactly as the Job requires.

Patch the form after load — extend the existing applySettings() (line 250) to also call this.pageTitleTouchedOrDirty.set(false) so loading valid settings from the backend does not flash the error on first paint. After patching the value, also sync this.pageTitleValue (a single pt.setValue(...) will trigger valueChanges, which already updates pageTitleValue).
No template change — <div class="field-error" data-testid="general-pageTitle-error">{{ pageTitleMessage() }}</div> at line 53 already lives inside the @if (showPageTitleError()) block; once the computeds are reactive it will render with the actionable text and the data-testid will appear in the DOM (matching the negative-case expectation in the Job description).

3.3 Pure helper — `frontend/src/app/features/admin/general.pure.ts`

No changes needed. pageTitleError() already returns 'required' for both empty and whitespace-only strings (verified at line 5). Existing tests in tests/frontend/admin-general-pure.spec.ts:91-114 already lock down this behavior — we extend that suite, not the helper.

4. Test Strategy

Per the project's existing convention (all tests/frontend/*.spec.ts are pure-function tests against *.pure.ts modules), and per the Job's "minimal, focused tests" rule, we do not introduce a TestBed-based component test. The existing admin-general-pure.spec.ts is the right home.

Target test files:
- Extend tests/frontend/admin-general-pure.spec.ts with two new describe blocks:
  1. describe('pageTitleMessage') — pure-function mapping (the same mapping the component's computed now uses). Asserts:
    - pageTitleMessage('') → required message
    - pageTitleMessage(' ') → required message (the Job's specific edge case)
    - pageTitleMessage('\t\n ') → required message
    - pageTitleMessage('OpenVelo') → null
    - pageTitleMessage('a'.repeat(121)) → maxlength message
  2. To keep this pure-function-friendly, extract the message-mapping function out of the component into general.pure.ts as export function pageTitleMessage(value, errors?): string | null, export it, and import it from the test. This is the minimal refactor that lets us assert the exact user-visible text without spinning up TestBed. The component's pageTitleMessage computed becomes a one-liner that calls this pure function.
- Add (new file) tests/frontend/admin-general-page-title-error.spec.ts only if the extracted helper's behavior isn't already covered by extending admin-general-pure.spec.ts. Per "minimal tests, single command", prefer extending the existing file to avoid duplicate test setup.
Mocking strategy: No mocks needed. The new pure function has no dependencies on FormControl, HttpClient, or Angular DI. It is a pure (value: string, errors?: ValidationErrors | null) => string | null mapping. The reactive plumbing inside the component is exercised manually per the Job description (no automated UI tests, per the Job rules).
Run command: npm test (from /repo) executes jest --config tests/jest.config.js --selectProjects frontend (and backend). Single command, no UI required, fully headless under jsdom.

5. Out of Scope / Non-Goals

Backend DTO, general.service.ts, controller, or setting table — all already correct.
Adding new validators (the existing pageTitleNotBlankValidator + Validators.required + Validators.maxLength(120) are sufficient).
Visual / CSS changes — the existing .field-error style and red color tokens are already correct.
Component-harness / TestBed tests — explicitly out per Job rules (minimal infra, no visual confirmation).
Refactoring other admin pages — not requested.

12 KiB Raw Blame History Unescape Escape