OpenVelo Agent
83 lines
7.7 KiB
Markdown
83 lines
7.7 KiB
Markdown
# Implementation Plan: Admin Area General Settings and Categories 1.12 (Job 894)
|
|
|
|
## 1. Architectural Reconnaissance
|
|
|
|
- **Codebase style & conventions:**
|
|
- Backend: NestJS 10 with TypeORM + better-sqlite3, TypeScript, async/await throughout, zod-validated DTOs.
|
|
- Frontend: Angular 17 standalone components, signals + reactive forms, OnPush change detection, jest-jsdom for tests.
|
|
- Tests live under `/repo/tests/backend` and `/repo/tests/frontend` (a single root-level `tests/` directory — never alongside source). They are executed with `npm test` from the repository root via `tests/jest.config.js` (multi-project Jest config, two projects: `backend` and `frontend`).
|
|
- **Data Layer:** SQLite via TypeORM (`/data/hipctf/db.sqlite` by default). Theme catalog is file-based (`backend/themes/*.json`), loaded at module-init by `ThemeLoaderService` and filtered at request-time by `AdminGeneralService.listThemes()`.
|
|
- **Test Framework & Structure:** Jest 29 with `ts-jest`. Two projects under `tests/jest.config.js`. Backend project uses `node` environment, frontend uses `jsdom` with `tests/frontend/jest.setup.ts`. New tests must be placed under `tests/backend/` (or `tests/frontend/`) and be discoverable by the existing globs (`<rootDir>/backend/**/*.spec.ts` / `<rootDir>/frontend/**/*.spec.ts`).
|
|
- **Required Tools & Dependencies:** No new dependencies are required. The fix is a path-resolution change in the existing service. No `setup.sh` change is required (the canonical themes already ship in the repo at `backend/themes/`).
|
|
|
|
## 2. Impacted Files
|
|
|
|
- **To Modify:**
|
|
- `backend/src/modules/admin/general.service.ts` — change the `THEMES_DIR` resolution so the default `./themes` resolves to the backend project's `themes/` directory regardless of process CWD. This is the only behavioural change needed.
|
|
- **To Create:**
|
|
- `tests/backend/admin-general-list-themes.spec.ts` — minimal unit test that asserts `listThemes()` returns all 10 canonical themes for the default configuration (the current `admin-general-service.spec.ts` already has a `listThemes` suite but it uses `process.env.THEMES_DIR` and a temp dir, so we add a focused new spec that exercises the **default** path-resolution without any env override, which is the regression case).
|
|
|
|
## 3. Proposed Changes
|
|
|
|
### Root-cause analysis
|
|
|
|
`backend/src/modules/admin/general.service.ts:80-103` (`listThemes`) reads
|
|
```ts
|
|
const themesDir = path.resolve(this.config.get<string>('THEMES_DIR', './themes'));
|
|
```
|
|
`path.resolve('./themes', ...)` is resolved against `process.cwd()`. The backend is started with `node /repo/backend/dist/main.js` from `/repo` (per `package.json` `start` script), so `./themes` resolves to `/repo/themes`, which does not exist. The 10 canonical JSON files live at `/repo/backend/themes/`. Because the directory does not exist, `present` stays empty and every theme is filtered out, so `GET /api/v1/admin/general/themes` returns `[]` and the Admin → General "Global theme" `<select>` renders with zero options.
|
|
|
|
### 1. Backend logic — fix default path resolution
|
|
|
|
In `backend/src/modules/admin/general.service.ts`:
|
|
|
|
- Replace the inline resolution with a small private helper, e.g.:
|
|
```ts
|
|
private resolveThemesDir(raw: string): string {
|
|
if (path.isAbsolute(raw)) return raw;
|
|
// Resolve relative to the backend project root (one level up from
|
|
// backend/src/modules/admin at compile time, i.e. backend/), so the
|
|
// default './themes' always points at backend/themes regardless of
|
|
// the process CWD used to launch `node backend/dist/main.js`.
|
|
const backendRoot = path.resolve(__dirname, '..', '..', '..');
|
|
return path.resolve(backendRoot, raw);
|
|
}
|
|
```
|
|
and use it in `listThemes()`:
|
|
```ts
|
|
const themesDir = this.resolveThemesDir(this.config.get<string>('THEMES_DIR', './themes'));
|
|
```
|
|
- Rationale: from `backend/src/modules/admin/general.service.ts`, three `..` segments walk back to the backend package root (`backend/`). At runtime the compiled file lives at `backend/dist/modules/admin/general.service.js`, so `__dirname` is `backend/dist/modules/admin` and three `..` segments still reach `backend/`. This makes `./themes` always point at the canonical `backend/themes/` directory shipped in the repo, without changing the public `THEMES_DIR` contract (an absolute path or a custom relative path still works).
|
|
- Keep the rest of `listThemes()` unchanged: it still intersects the loader's catalog with on-disk JSON `id`s, so a custom `THEMES_DIR` pointing at a different folder still filters accordingly.
|
|
|
|
### 2. Frontend / signal hardening (defensive, small)
|
|
|
|
The job reports a browser console warning
|
|
`RuntimeError: NG0600: Writing to signals is not allowed in a computed or an effect by default`
|
|
when the Admin General page loads with the empty theme list. Once the backend returns 10 themes, this symptom goes away. To make the component robust if the network call fails, no structural change to the signal graph is needed — the existing pattern (`signal<ThemeView[]>([])` written in `ngOnInit` via `this.themes.set(themes)`) is correct and not inside an `effect()`/`computed()`. We **do not** introduce new signal writes; we only verify nothing in `frontend/src/app/features/admin/general.component.ts` writes a signal from a computed/effect, and leave the file untouched unless the implementer finds an unrelated `effect()` that mutates state (none exists today — verified by grep). If the implementer wants to be extra defensive, a single optional one-liner fallback in `general.pure.ts` (e.g. a pure `BUILTIN_THEME_VIEW_FALLBACK` array used when the API returns `[]`) is acceptable but not required.
|
|
|
|
### 3. `setup.sh` / data layer
|
|
|
|
No changes. `/data` is not touched by this job. The `THEMES_DIR` env var (and its default) continue to work the same way; only the default resolution now points at the correct directory.
|
|
|
|
## 4. Test Strategy
|
|
|
|
### Target Unit Test File
|
|
|
|
- `tests/backend/admin-general-list-themes.spec.ts` (new) — single focused regression spec for the path-resolution bug.
|
|
|
|
### Mocking Strategy
|
|
|
|
- Instantiate `AdminGeneralService` directly with hand-rolled fakes for the three injected collaborators (`SettingsService`, `ThemeLoaderService`, `SseHubService`) and a `ConfigService` whose `.get('THEMES_DIR', './themes')` returns the **default** `'./themes'` (no `process.env` override) so the test exercises the same code path the real backend takes.
|
|
- `ThemeLoaderService.listThemes()` is faked to return 10 entries with the canonical ids (`classic`, `midnight`, `sunset`, `forest`, `cyber`, `paper`, `crimson`, `ocean`, `neon`, `monochrome`).
|
|
- Do NOT mock `fs` or `path`. The test relies on the real `backend/themes/` directory shipped in the repo (this is exactly the regression case).
|
|
|
|
### Cases (minimal, single-file)
|
|
|
|
1. **Regression:** with no `THEMES_DIR` override, `listThemes()` returns exactly 10 entries whose ids are a permutation of the canonical `THEME_IDS` set, and each entry has `id === key` and a non-empty `name`. This is the single must-have test that locks in the fix.
|
|
2. **Robustness:** when called twice in a row, the result is referentially equivalent (no I/O ordering surprises).
|
|
|
|
That is intentionally the entire scope. We do **not** add tests for: visual rendering of the `<select>` (frontend tests focus on logic), custom `THEMES_DIR` overrides (already covered by the existing `admin-general-service.spec.ts` "filter to on-disk themes" suite), `PUT /settings` happy-path (already covered), the unknown-`themeKey` 400 (already covered by the existing `rejects unknown themeKey` case in `admin-general-service.spec.ts`), and the runtime NG0600 warning (a downstream symptom that goes away once the list is non-empty — not a separate contract).
|
|
|
|
The single test must run with `npm test -- tests/backend/admin-general-list-themes.spec.ts` (or simply `npm test`) from the repository root.
|