HIPCTF2/.kilo/plans/905-handler-ownership.md

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:

   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:

   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).