AI Implementation feature(905): Challenges Page and Challenge Solve Modal 1.01 (#47)

2026-07-23 01:20:00 +00:00
parent 1bd4a5a198
commit cbfa9c35ca
9 changed files with 437 additions and 115 deletions
@@ -0,0 +1,124 @@
+# 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).