AI Implementation feature(906): Challenges Page and Challenge Solve Modal 1.02 (#48)
This commit was merged in pull request #48.
This commit is contained in:
@@ -1,124 +0,0 @@
|
||||
# Implementation Plan: Decouple `reloadAtCountdownZero` from `start()`/`stop()`
|
||||
|
||||
## Status
|
||||
Hardening follow-up to Job 905. The store currently clears the
|
||||
`reloadOnZero` handler inside `stop()` (line 197), and `start()` calls
|
||||
`stop()` first (line 110). Because `ChallengesPage` registers its handler
|
||||
from the constructor while `HomeComponent` calls `start()`/`stop()`
|
||||
independently, a `stop()` triggered by the shell's lifecycle can null out
|
||||
the page's handler before the page is destroyed. The fix is to scope
|
||||
`reloadOnZero` ownership to the **subscriber** (the challenges page)
|
||||
and make `start()`/`stop()` only manage the SSE source + tick, never the
|
||||
reload handler.
|
||||
|
||||
## 1. Architectural Reconventions (recap)
|
||||
- `EventStatusStore` is a `providedIn: 'root'` singleton
|
||||
(`frontend/src/app/core/services/event-status.store.ts`).
|
||||
- `start(...)` is called from `HomeComponent.ngOnInit`
|
||||
(`features/home/home.component.ts:132`); `stop()` is invoked from both
|
||||
`ngOnDestroy()` and `destroyRef.onDestroy(...)` in that same component
|
||||
(`features/home/home.component.ts:125, 142`).
|
||||
- `ChallengesPage` currently registers its reload handler in its
|
||||
constructor (`features/challenges/challenges.page.ts:128-133`).
|
||||
- Tests live in `tests/frontend/*`, runnable via `npm test`. No new
|
||||
dependencies; no backend changes.
|
||||
|
||||
## 2. Impacted Files
|
||||
- **To Modify:**
|
||||
- `frontend/src/app/core/services/event-status.store.ts` —
|
||||
separate handler ownership from SSE/timer ownership.
|
||||
- `frontend/src/app/features/challenges/challenges.page.ts` —
|
||||
own the subscription, unregister in `ngOnDestroy` (or via
|
||||
`DestroyRef`).
|
||||
- **To Create:** None.
|
||||
|
||||
## 3. Proposed Changes
|
||||
|
||||
### 3.1 `EventStatusStore` — split "transport lifecycle" from "reload subscription"
|
||||
|
||||
1. Add a per-subscription API that returns an unregister function:
|
||||
```ts
|
||||
subscribeReloadAtCountdownZero(handler: () => void): () => void {
|
||||
this.reloadOnZero = handler;
|
||||
this.reloadOnZeroFired = false;
|
||||
this.ensureWatcher();
|
||||
return () => {
|
||||
if (this.reloadOnZero === handler) {
|
||||
this.reloadOnZero = null;
|
||||
this.reloadOnZeroFired = false;
|
||||
}
|
||||
};
|
||||
}
|
||||
```
|
||||
Keep the existing `reloadAtCountdownZero(handler)` as a thin wrapper
|
||||
that calls `subscribeReloadAtCountdownZero(handler)` and discards
|
||||
the returned disposer (preserves backwards compatibility with the
|
||||
four existing tests that call it directly).
|
||||
2. In `start(...)`: remove the `this.stop()` call. `start()` should
|
||||
only (a) close any **previous source** via a new private
|
||||
`closeTransport()`, (b) open the new source, (c) install the message
|
||||
handler, (d) start the tick interval, (e) `ensureWatcher()`. It
|
||||
must NOT touch `reloadOnZero`, `reloadOnZeroFired`, or
|
||||
`lastAppliedJson`.
|
||||
3. Introduce `closeTransport()` (private) that nulls
|
||||
`source`/`intervalId`/`zeroWatcher` ONLY — no handler reset. The
|
||||
watcher interval is also bound to the transport lifecycle, but
|
||||
`ensureWatcher()` keeps it idempotent, so closing it is fine.
|
||||
4. In `stop()`: keep the current teardown for source + tick + watcher,
|
||||
but **only clear `reloadOnZero`/`reloadOnZeroFired`/`lastAppliedJson`
|
||||
when called via the destroy hook** (i.e., `stop()` retains its
|
||||
existing full-reset semantics for end-of-life). To preserve that
|
||||
for the existing destroyRef hook (`constructor() { destroyRef.onDestroy(() => this.stop()); }`),
|
||||
keep `stop()` as the full reset. The crucial change is step 2:
|
||||
`start()` no longer calls `stop()`, so handler state survives a
|
||||
`start()` cycle.
|
||||
|
||||
### 3.2 `ChallengesPage` — own the subscription
|
||||
|
||||
1. Replace the constructor's
|
||||
`this.eventStatus.reloadAtCountdownZero(...)` call with:
|
||||
```ts
|
||||
const unsub = this.eventStatus.subscribeReloadAtCountdownZero(() => {
|
||||
if (typeof window !== 'undefined') window.location.reload();
|
||||
});
|
||||
this.destroyRef.onDestroy(() => unsub());
|
||||
```
|
||||
This keeps the subscription active across `start()` re-entry on the
|
||||
store, registers the handler as early as possible (constructor), and
|
||||
unregisters when the page is destroyed.
|
||||
2. The existing `this.destroyRef.onDestroy(() => this.store.stop())`
|
||||
for `ChallengesStore` is unrelated and stays.
|
||||
|
||||
### 3.3 Tests
|
||||
|
||||
1. **New spec** (`tests/frontend/event-status.store.watcher.spec.ts`,
|
||||
add one case): `start()` after a `subscribeReloadAtCountdownZero`
|
||||
must NOT clear the registered handler — fire a fake SSE delivery
|
||||
that triggers `applyServerStatus` with a zero-countdown frame, then
|
||||
advance fake timers and assert the handler still fires exactly once.
|
||||
2. **Update** the existing
|
||||
`tests/frontend/challenges-page.spec.ts` regression test
|
||||
"repeating the same zero-countdown SSE frame does not disarm the
|
||||
auto-reload latch (Job 905)" — no behaviour change required; it
|
||||
already exercises `applyServerStatus` directly and continues to pass.
|
||||
3. **New spec** for `subscribeReloadAtCountdownZero` unregister
|
||||
semantics: subscribe → unregister → apply zero-countdown → advance
|
||||
timers → handler must NOT be called.
|
||||
|
||||
### 3.4 Documentation
|
||||
|
||||
- Append one sentence to `docs/guides/event-window.md`'s "Auto-reload
|
||||
at the transition boundary" paragraph noting that the subscription
|
||||
is now page-owned and unregistered on destroy, independent of the
|
||||
transport's `start()`/`stop()`.
|
||||
|
||||
## 4. Test Strategy
|
||||
- Single command: `npm test`. New assertions target
|
||||
`tests/frontend/event-status.store.watcher.spec.ts` and use fake
|
||||
timers (already used in this file). No mocking of `window.location`
|
||||
is required — the handler under test is a plain `jest.fn()`.
|
||||
|
||||
## 5. Non-goals
|
||||
- No backend changes, no new dependencies, no `setup.sh` change.
|
||||
- No changes to other consumers of `EventStatusStore` (HomeComponent's
|
||||
`start()`/`stop()` cycle is preserved by step 3.1.2).
|
||||
@@ -0,0 +1,28 @@
|
||||
# Implementation Plan: Register SeedSampleChallenges migration in DatabaseModule
|
||||
|
||||
## 1. Architectural Reconnaissance
|
||||
- **Codebase style & conventions:** NestJS + TypeORM; migrations are registered manually via an in-source `MIGRATIONS` array (not a filesystem glob) in `backend/src/database/database.module.ts`. Imports follow chronological order matching the array.
|
||||
- **Data Layer:** SQLite via `better-sqlite3`. `DatabaseInitService.init()` (called from `main.ts` before `app.listen()`) executes `dataSource.runMigrations({ transaction: 'each' })`, which **uses the `MIGRATIONS` array** registered with `TypeOrmModule.forRootAsync`. Until the array contains the new class, `npm run setup` and the first app boot will not run `SeedSampleChallenges1700000000600`.
|
||||
- **Test framework:** Jest; backend tests construct their own in-memory `DataSource` with an explicit `migrations: [...]` list — they bypass `DatabaseModule`, which is why `migrations.spec.ts` (already updated for Job 906) passes despite this gap.
|
||||
- **Required tools & dependencies:** none.
|
||||
|
||||
## 2. Impacted Files
|
||||
- **To Modify:**
|
||||
- `backend/src/database/database.module.ts` — add the import and append the new migration class to the `MIGRATIONS` array, right after `UpgradeChallengeAdminSchema1700000000500`.
|
||||
|
||||
## 3. Proposed Changes
|
||||
1. **Edit `database.module.ts`:**
|
||||
- Add the import alongside the other chronological migration imports (between `.../1700000000500-UpgradeChallengeAdminSchema` and the next non-migration import `DatabaseInitService`):
|
||||
```ts
|
||||
import { SeedSampleChallenges1700000000600 } from './migrations/1700000000600-SeedSampleChallenges';
|
||||
```
|
||||
- Append to the `MIGRATIONS` array, immediately after `UpgradeChallengeAdminSchema1700000000500`:
|
||||
```ts
|
||||
SeedSampleChallenges1700000000600,
|
||||
```
|
||||
2. **No other code changes.** The migration class already exists, is exported, and is covered by tests in `tests/backend/migrations.spec.ts`. Once registered in the TypeORM `MIGRATIONS` array, `DatabaseInitService.init()` (called via `main.ts` and indirectly by `npm run setup`) will execute it on next boot.
|
||||
3. **Verification expectation:** After `npm run setup` (or first boot on a fresh DB), `SELECT COUNT(*) FROM challenge;` returns `> 0` and the eight sample names are present. Re-running setup on a DB that already has rows is a no-op (the migration's early-return guard).
|
||||
|
||||
## 4. Test Strategy
|
||||
- No new automated tests required. The existing `tests/backend/migrations.spec.ts` already asserts the seed runs and that values are schema-compatible, schema-correct, and `down()` is scoped correctly. The fix is purely a wiring change that registers an already-tested class with the production `DatabaseModule`, which has no dedicated test (and per the Jobs rule, no new test files should be introduced for a 2-line wiring fix).
|
||||
- **Manual smoke (tester steps, no UI):** on a fresh DB file, run `npm run setup` then `sqlite3 ./data/db.sqlite 'SELECT COUNT(*) FROM challenge;'` and confirm the count is `8`; on a re-run, confirm the count is unchanged (idempotent guard works).
|
||||
Reference in New Issue
Block a user