AI Implementation feature(859): Admin Area General Settings and Categories (#21)
This commit was merged in pull request #21.
This commit is contained in:
@@ -0,0 +1,118 @@
|
||||
# Implementation Plan: Repair `better-sqlite3` native binding to unblock test suite
|
||||
|
||||
## 0. Problem Statement
|
||||
|
||||
21 of 48 test suites fail with `TypeError: this.sqlite is not a constructor` thrown from `TypeOrm BetterSqlite3Driver.createDatabaseConnection` (`node_modules/typeorm/driver/better-sqlite3/BetterSqlite3Driver.js:88` — `new this.sqlite(database, ...)`). The remaining suites timeout with `Exceeded timeout of 5000 ms` because TypeORM silently retries (10 retries × 1s) on the same error and the test eventually times out. The visible symptoms in Jest's output are:
|
||||
|
||||
- Direct-DB suites (`tests/backend/theme-required.spec.ts`, `database-init.spec.ts`, `admin-categories-service.spec.ts`, `uploads-logo.spec.ts`, plus any other suite that imports `DatabaseModule`, `CommonModule`, or builds its own `TypeOrmModule.forRoot`) all hang/timeout with the same root error.
|
||||
- Suites that use `AppModule` (bootstrap, auth, event-stream, uploads integration tests, etc.) cascade the failure the first time `TypeOrmModule.forRootAsync` is loaded — 16 of 30 backend suites in total trigger the failure mode.
|
||||
|
||||
## 1. Root Cause (verified by direct read + REPL probe)
|
||||
|
||||
`/repo/node_modules/better-sqlite3` directory exists but **its compiled native binding (`better_sqlite3.node`) is missing**.
|
||||
|
||||
- `ls /repo/node_modules/better-sqlite3/build` → does not exist.
|
||||
- `find /repo -name 'better_sqlite3.node'` → zero results anywhere on disk.
|
||||
- `cat /repo/node_modules/better-sqlite3/package.json` shows `"install": "prebuild-install || node-gyp rebuild --release"` but the install step never ran for this install. The result is `require('better-sqlite3')` returns a plain function that throws when called as a constructor (`TypeError: this.sqlite is not a constructor` is what TypeORM sees at its `new this.sqlite(...)` call because its `loadDependencies()` reads the package via `PlatformTools.load("better-sqlite3")`; the require "succeeds" returning the stub function, `this.sqlite = sqlite` is set, then `new this.sqlite(...)` blows up because there is no native binding).
|
||||
|
||||
The trigger: an earlier `npm install sharp --no-audit --no-fund --prefix /repo --save-dev` step (used to add `sharp` so the icon-normalization tests could load it) re-mounted the workspace's dependency tree. The output of that command reported `added 7 packages, and removed 345 packages`, and `removed 345` includes the previously-compiled native bindings for `better-sqlite3` (and likely nuked the per-workspace `node_modules` too — `ls /repo/backend/node_modules` shows only `uuid` left, where before install there were ~50 packages). Other native packages (`argon2`, `sharp` itself) still work because they ship prebuilds that survive reinstall, but `better-sqlite3@11` historically relied on a `node-gyp` build that did not run during the reinstall.
|
||||
|
||||
This is **environmental / build-harness damage**, not a regression in any `.ts` source file. None of the seven files I edited this session (Job 859 + logo-upload changes) need to be reverted.
|
||||
|
||||
## 2. Constraints
|
||||
|
||||
- Plan Mode is active — I will not run install commands or modify files during planning. The full fix is the implementation turn's responsibility.
|
||||
- The container runs Node v20.20.2 on `linux-x64` (confirmed via `node -v` and `os.platform()`).
|
||||
- `better-sqlite3@11.10.0` was resolved and present in `/repo/node_modules/better-sqlite3/package.json` from the failed install.
|
||||
- We must keep `sharp` (added for the icon-normalize feature) and `uuid` (added at backend level) — both were intentional and unrelated to the breakage.
|
||||
- The fix must not require a network install if avoidable; it should rely on re-running the existing install scripts.
|
||||
- The fix must not require any source-code changes.
|
||||
|
||||
## 3. Architectural Reconnaissance
|
||||
|
||||
- **Codebase style & conventions:** NestJS 10 backend + Angular 17 frontend. Workspace uses npm workspaces (`package.json` declares `"workspaces": ["backend", "frontend"]`). Native modules resolve via npm's hoisted `node_modules/`.
|
||||
- **Native-module recovery precedent in this repo:** None — `setup.sh` simply runs `npm ci --no-audit --no-fund || npm install --no-audit --no-fund` at the repo root, then `npm --workspace frontend run build` and `npm --workspace backend run build`. There is no explicit `npm rebuild` step.
|
||||
- **Test framework:** Jest + ts-jest. Two projects: `tests/backend` (node env) and `tests/frontend` (jsdom env). All tests can be re-run with a single `npm test`.
|
||||
- **Required Tools & Dependencies:** No new system tools. The fix needs `npm` (already on PATH) and a working C/C++ toolchain in the container (Node was built natively here, so `node-gyp`/prebuilds should work).
|
||||
|
||||
## 4. Impacted Files (non-source)
|
||||
|
||||
### To Modify (none — no source code edits required for this fix)
|
||||
|
||||
### To Update (operational only)
|
||||
|
||||
- `setup.sh` — add an explicit `npm rebuild better-sqlite3` step (see §5) so a fresh container / CI run never has this regression again.
|
||||
- `package.json` scripts (optional, low-risk) — add `"rebuild-native": "npm rebuild better-sqlite3"` for future debugging.
|
||||
|
||||
### No changes needed
|
||||
|
||||
- `backend/package.json` (already declares `better-sqlite3: ^11.0.0` correctly).
|
||||
- `backend/src/**` (no source regression).
|
||||
- `frontend/src/**` (no source regression).
|
||||
- Any test file (the existing tests are correct).
|
||||
|
||||
## 5. Proposed Changes (ordered, safe, idempotent)
|
||||
|
||||
The fix is two commands + one documentation tweak. All steps are read-only wrt source code.
|
||||
|
||||
1. **Recompile the native binding.** From `/repo`, run:
|
||||
```
|
||||
npm rebuild better-sqlite3 --build-from-source
|
||||
```
|
||||
This forces `npm` to re-run better-sqlite3's `install` script (prebuild-install, falling back to `node-gyp rebuild --release`) against the current Node 20.20.2 binary. Acceptable runtime: ~30–90 seconds. Verifier: `find /repo -name 'better_sqlite3.node'` should return exactly one path under `node_modules/better-sqlite3/build/Release/...`.
|
||||
|
||||
2. **Verify TypeORM can construct the connection.** Run a quick smoke test:
|
||||
```
|
||||
node -e "const db = require('/repo/node_modules/better-sqlite3'); const inst = new db(':memory:'); console.log('memory:', inst.memory);"
|
||||
```
|
||||
This must print `memory: <an object>`. If `inst.memory` is an empty object, the binding is correctly loaded.
|
||||
|
||||
3. **Re-run the entire test suite.** Single command from the repo root:
|
||||
```
|
||||
npm test
|
||||
```
|
||||
Expected: `Test Suites: 48 passed, 48 total / Tests: 271 passed, 271 total` (matching the pre-regression baseline). If the count differs by exactly the new tests added in the prior logo-upload step (still 48 suites, 275 tests), that is the desired outcome.
|
||||
|
||||
4. **Hard-wire the recovery into `setup.sh`.** After the existing `npm ci --no-audit --no-fund || npm install --no-audit --no-fund` step, insert:
|
||||
```
|
||||
# Repro native modules that depend on the host Node ABI.
|
||||
npm rebuild better-sqlite3 --build-from-source || npm rebuild better-sqlite3
|
||||
```
|
||||
This guarantees a fresh clone / CI container that runs `setup.sh` always ends up with a working binding, even if the upstream npm resolution order accidentally drops it again.
|
||||
|
||||
5. **(Optional) Add a maintenance npm script.** In `package.json`'s top-level `scripts`, add:
|
||||
```
|
||||
"rebuild-native": "npm rebuild better-sqlite3 --build-from-source"
|
||||
```
|
||||
so a future implementer can recover with `npm run rebuild-native` when this surfaces again.
|
||||
|
||||
## 6. Test Strategy (verification only — no new test files)
|
||||
|
||||
We will not add a new test for this issue because the test suite itself is the canary. After applying steps 1–3 above, the suite must return to its previous green state.
|
||||
|
||||
Verification checklist executed in the implementation turn:
|
||||
|
||||
| Check | How | Expected result |
|
||||
|---|---|---|
|
||||
| Native binary exists | `find /repo -name 'better_sqlite3.node'` | exactly one file under `build/Release/better_sqlite3.node` |
|
||||
| Node can construct | smoke-test one-liner above | `inst.memory` is an object literal, not `undefined`/throw |
|
||||
| Backend unit tests green | `npx jest --selectProjects backend --testPathPattern=theme-required` | all 6 tests pass within 30s |
|
||||
| Backend integration tests green | `npx jest --selectProjects backend --testPathPattern=uploads-logo` | all 4 tests pass |
|
||||
| Full suite green | `npm test` | 48 / 48 suites, 271+ / 271+ tests pass |
|
||||
|
||||
Mocking strategy: not applicable (this is an environmental fix; no new code paths).
|
||||
|
||||
## 7. Order of Operations (recommended)
|
||||
|
||||
1. Run `npm rebuild better-sqlite3 --build-from-source` from `/repo`.
|
||||
2. Run the smoke-test one-liner to confirm binding loads.
|
||||
3. Run `npx jest --selectProjects backend --testPathPattern=theme-required` — expect green.
|
||||
4. Run the full `npm test` — expect green.
|
||||
5. Edit `setup.sh` (and optionally root `package.json`) to wire the rebuild into the canonical setup flow.
|
||||
6. Re-run `npm test` one final time to confirm the wired step works on a clean install path (skip if we can't fully simulate a clean install inside plan mode).
|
||||
|
||||
## 8. Risk and Rollback
|
||||
|
||||
- Risk is minimal: `npm rebuild` on a single package does not touch other dependencies, and `setup.sh` is only modified to add a recovery step (additive only).
|
||||
- Rollback: if `npm rebuild better-sqlite3 --build-from-source` fails on this image (no compiler toolchain), the fallback `npm rebuild better-sqlite3` (without `--build-from-source`) will attempt to download a precompiled binary via `prebuild-install`. Either path restores the binding; if both fail the issue is environmental and out of scope of the implementation turn.
|
||||
- We are NOT reverting any prior fix per the user's instruction — the seven files changed for Jobs 859 + logo-upload stay exactly as they are.
|
||||
@@ -1,45 +0,0 @@
|
||||
# Implementation Plan: Job 881 — Cross-Tab Authenticated Shell Invalidation
|
||||
|
||||
## 1. Architectural Reconnaissance
|
||||
- **Codebase style & conventions:** TypeScript monorepo with an Angular standalone-component SPA and NestJS API. Frontend services are root-provided Angular injectables using signals, `inject()`, async/await, and typed HTTP calls. Authentication state is held in `AuthService` signals and mirrored to per-tab `sessionStorage`; `HomeComponent` owns the authenticated shell and starts/stops the authenticated event stream. The route tree protects `/`, `/challenges`, `/scoreboard`, and `/blog` through the parent `authGuard`.
|
||||
- **Data Layer:** No database/schema change is required. The server-side logout endpoint already revokes/clears the refresh-cookie state; this job is a browser-context propagation fix. Existing frontend persistence uses `sessionStorage`, which is intentionally tab-scoped and therefore cannot notify sibling tabs. A cross-tab browser event channel should carry an invalidation notification without carrying access tokens or user data.
|
||||
- **Test Framework & Structure:** Root Jest 29 with two projects in `tests/jest.config.js`; frontend tests run in jsdom through `npm test` or `npm run test:frontend`. Tests are already centralized under `tests/frontend`, never beside source. Existing tests are mostly focused TypeScript/pure-contract tests, and `authenticated-event-source.spec.ts` models fetch/SSE boundaries because Angular runtime modules are difficult for the current Jest transform. Add only small logic tests for the notification/listener behavior and source error handling.
|
||||
- **Required Tools & Dependencies:** No new system tools, npm packages, database migrations, or `setup.sh` changes are expected. `BroadcastChannel` and/or the `storage` event are browser platform APIs. The implementation should feature-detect the selected API and remain safe in jsdom/SSR-like contexts. Existing Node/npm/Jest/TypeScript setup is sufficient.
|
||||
|
||||
## 2. Impacted Files
|
||||
- **To Modify:**
|
||||
- `frontend/src/app/core/services/auth.service.ts` — publish a same-browser-context session-invalidation event after local logout/clear and subscribe to peer-tab invalidation events so every tab clears its in-memory and tab-local state.
|
||||
- `frontend/src/app/core/services/authenticated-event-source.service.ts` — distinguish an unauthorized SSE response (`401`, and optionally other auth-failure statuses according to the existing API contract) from generic transport failure and expose it through the existing `error` event contract, or otherwise provide a single auth-invalidation callback path to `AuthService`.
|
||||
- `frontend/src/app/core/services/event-status.store.ts` — register an `error` listener for the authenticated event source and invoke the auth invalidation path on unauthorized stream failure; ensure `stop()` removes/neutralizes the listener and remains idempotent.
|
||||
- `frontend/src/app/features/home/home.component.ts` — react to the centralized auth state becoming unauthenticated, stop/reset shell-owned state as needed, and navigate to `/login` without requiring a quick-tab interaction or reload; avoid duplicate navigation when the local logout already performed it.
|
||||
- **To Create:**
|
||||
- `tests/frontend/auth-cross-tab.spec.ts` — focused tests for cross-tab invalidation publication/consumption and duplicate/self-origin behavior using mocked browser channel/storage boundaries.
|
||||
- If the implementer chooses a pure transport result rather than a direct callback, optionally `frontend/src/app/core/services/auth-session-events.pure.ts` and its corresponding test file; prefer keeping the event-name/payload validation pure and small rather than introducing an unnecessary abstraction.
|
||||
- **Not to Modify:** Backend controllers, refresh-token schema/migrations, shell markup, quick-tab components, routes, or `setup.sh`; the server invalidation and route protection already exist.
|
||||
|
||||
## 3. Proposed Changes
|
||||
1. **Database / Schema Migration:**
|
||||
- No migration. Confirm the existing `POST /api/v1/auth/logout` behavior remains the source of truth for server-side refresh-cookie invalidation.
|
||||
- Do not broadcast JWTs, refresh tokens, usernames, or arbitrary storage contents. Broadcast only a fixed event type (for example, a namespaced `logout`/`session-invalidated` message) and optionally a random per-tab sender identifier if needed to avoid self-processing.
|
||||
|
||||
2. **Backend Logic & APIs:**
|
||||
- No backend endpoint changes. The existing authenticated logout endpoint is called by `AuthService.logout()` before local state is cleared, and hard navigation already proves the server clears the refresh cookie.
|
||||
- Treat an authenticated SSE response with `401` as evidence that the current tab’s session is invalid. Update `AuthenticatedEventSourceService`’s internal fetch path so the response status is available to the existing `error` dispatch, without exposing response bodies or credentials. Generic network/stream errors should remain generic unless the response explicitly indicates unauthorized.
|
||||
- If the transport contract cannot safely distinguish error categories through `Event`, add a minimal typed error event/detail or a separate `unauthorized` listener type in `event-status.pure.ts`; update all implementations and tests consistently. Keep the contract framework-light and compatible with the current custom fetch-based EventSource implementation.
|
||||
|
||||
3. **Frontend UI Integration:**
|
||||
- Centralize cross-tab handling in `AuthService`, because it owns the authoritative `isAuthenticated`, access-token, and session-storage state. Initialize one browser listener for the root singleton, feature-detect `BroadcastChannel`, and provide a fallback using a namespaced `localStorage` key plus the browser `storage` event if BroadcastChannel is unavailable. If both are installed, publish on the primary channel and retain the fallback only if necessary; avoid double-processing identical notifications.
|
||||
- On a local logout, call the server as today, then clear local state and publish the invalidation signal. Ensure peer-tab handling calls a non-broadcasting local-clear method so the notification does not echo indefinitely. The local tab may also receive its own channel message in some test/browser implementations, so make handling idempotent.
|
||||
- On a peer invalidation, clear the access token, current user, and `sessionStorage`; make the state transition observable to consumers. If `AuthService.clear()` is reused by refresh failure or other local failures, define whether those paths should broadcast as well; the preferred rule is that any confirmed local session invalidation broadcasts, while a peer-originated clear suppresses rebroadcast.
|
||||
- Wire the authenticated SSE lifecycle to the same invalidation path. `EventStatusStore.start()` should attach both message and error listeners. When the authenticated event source reports `401`, invoke an injected/session-invalidation callback or call an explicit `AuthService` method; then close the stream. Do not redirect merely for transient network errors, malformed frames, or ordinary stream closure.
|
||||
- In `HomeComponent`, observe the auth signal or subscribe to a narrow invalidation event and navigate to `/login` after a peer logout/SSE unauthorized result. Reset `UserStore`, close the event stream, close open menus/modals if appropriate, and use a navigation guard/flag so the local `onLogout()` path does not trigger duplicate navigation. The next click on Challenges, Scoreboard, or Blog must not be able to continue rendering authenticated content; the route guard should see `isAuthenticated === false` and return the existing login `UrlTree`.
|
||||
- Ensure cleanup: close `BroadcastChannel`, remove `storage` listeners, and detach/neutralize SSE listeners during service/component destruction or `stop()`. Preserve the existing event countdown behavior and shell UI when authentication remains valid.
|
||||
- Keep browser API access guarded for non-browser/test environments. Use the existing Angular DI/signal conventions; do not add a third-party messaging library or move tokens into `localStorage`.
|
||||
|
||||
## 4. Test Strategy
|
||||
- **Target Unit Test File:** `tests/frontend/auth-cross-tab.spec.ts` for the core event protocol and AuthService behavior. Extend `tests/frontend/authenticated-event-source.spec.ts` only with the minimal unauthorized-response contract test if that file remains the established transport boundary. If direct Angular service instantiation is impractical under the current Jest setup, extract a small pure event encoder/decoder and test it directly, while testing the transport contract with the existing fetch mock style.
|
||||
- **Mocking Strategy:**
|
||||
- Mock `BroadcastChannel` with an in-memory registry of channel instances, recording `postMessage`, `addEventListener`, `removeEventListener`, and `close`; deliver messages only to peer instances to model browser behavior and separately cover a self-delivery implementation.
|
||||
- Mock `window.localStorage` and dispatch synthetic `StorageEvent` objects for the fallback path. Assert the implementation only accepts the fixed namespaced key/event shape and ignores malformed or unrelated storage changes.
|
||||
- Use a minimal fake `AuthService` or explicit test doubles for token/user/session-storage operations where testing `EventStatusStore`; use a fake `EventSourceLike` that records listeners and close calls to verify `401` causes invalidation and generic errors do not.
|
||||
- Cover only the core success and directly relevant failure paths: local logout publishes once and clears state; peer notification clears state without rebroadcast; fallback storage notification works when BroadcastChannel is absent; malformed/unrelated messages are ignored; unauthorized SSE invalidates and closes; generic SSE error preserves authentication. Run through the existing root `npm test` command; no browser/UI or persistent `/data` fixture is needed.
|
||||
Reference in New Issue
Block a user