diff --git a/.superpowers/sdd/2026-08-18-first-visible-vertical-slice/task-7-report.md b/.superpowers/sdd/2026-08-18-first-visible-vertical-slice/task-7-report.md index fb137b1..ef6fd75 100644 --- a/.superpowers/sdd/2026-08-18-first-visible-vertical-slice/task-7-report.md +++ b/.superpowers/sdd/2026-08-18-first-visible-vertical-slice/task-7-report.md @@ -71,3 +71,53 @@ there is no repository-configured lint command to run for this task. this task explicitly prohibited subagents. The implementation was instead reviewed directly against the task brief and verified through the focused and complete web test/build checks above. + +## Review fix round 1 + +### Findings addressed + +- A journey whose `arrivesAt` was already in the past made an immediate poll + but also installed a countdown interval. This could overlap an outstanding + request. +- A late response after store destruction could still apply state and start + follow-on work. + +The store now makes a single immediate poll at local zero with no countdown +interval. It uses one in-flight poll at a time and schedules a deliberate +one-second retry when the server still returns `TRAVELLING` or a poll fails. +All late async continuations check the destroy flag before changing state or +scheduling timers. The browser still never declares arrival or changes the +location itself. + +### TDD evidence + +```powershell +npm test --workspace=@ashen-realms/web -- --watch=false --include='src/app/features/world/world.store.spec.ts' +``` + +RED result: 2 of 9 tests failed against the previous implementation. The +already-expired test observed three calls where a pending request must allow +only two, and the destroy test observed an unwanted second character/location +reload after a late `COMPLETED` response. + +GREEN result: 1 test file passed, 9 tests passed. The expanded cases establish +that `IDLE` and continuing `TRAVELLING` do not reload authoritative character +or location data, expired/skewed client time remains single-flight/throttled, +a transient poll error retries and recovers, and destruction ignores a late +response. + +### Fresh verification + +```powershell +npm test --workspace=@ashen-realms/web -- --watch=false +# 3 test files passed, 13 tests passed + +npm run build:web +# Angular production build completed successfully + +npm exec --workspace=@ashen-realms/web -- prettier --check +# All matched files use Prettier code style + +git diff --check -- +# exit 0 +``` diff --git a/apps/web/src/app/features/world/world.store.spec.ts b/apps/web/src/app/features/world/world.store.spec.ts index 7e97d41..6ff1935 100644 --- a/apps/web/src/app/features/world/world.store.spec.ts +++ b/apps/web/src/app/features/world/world.store.spec.ts @@ -1,5 +1,5 @@ import { TestBed } from '@angular/core/testing'; -import { of } from 'rxjs'; +import { of, Subject, throwError } from 'rxjs'; import { vi } from 'vitest'; import type { CharacterResponse, @@ -53,6 +53,11 @@ const travelling: CurrentTravel = { arrivesAt: '2026-08-18T10:00:10.000Z', }; +const expiredTravelling: CurrentTravel = { + ...travelling, + arrivesAt: '2026-08-18T09:59:59.000Z', +}; + describe('WorldStore', () => { let api: { getCharacter: ReturnType; @@ -79,6 +84,7 @@ describe('WorldStore', () => { }); afterEach(() => { + store.ngOnDestroy(); vi.useRealTimers(); }); @@ -121,6 +127,68 @@ describe('WorldStore', () => { expect(api.getCurrentTravel).toHaveBeenCalledTimes(2); expect(store.currentLocation()).toBe(initialLocation); + expect(api.getCharacter).toHaveBeenCalledOnce(); + expect(api.getCurrentLocation).toHaveBeenCalledOnce(); + }); + + it('makes one immediate poll without a countdown interval for an already expired arrival', async () => { + const pendingTravel = new Subject(); + api.getCurrentTravel.mockReturnValueOnce(of(expiredTravelling)).mockReturnValue(pendingTravel); + + await store.load(); + await vi.advanceTimersByTimeAsync(1_000); + + expect(api.getCurrentTravel).toHaveBeenCalledTimes(2); + + pendingTravel.next({ status: 'IDLE' }); + pendingTravel.complete(); + }); + + it('does not reload character or location when the authoritative poll remains travelling', async () => { + api.getCurrentTravel + .mockReturnValueOnce(of(expiredTravelling)) + .mockReturnValueOnce(of(expiredTravelling)); + + await store.load(); + await vi.advanceTimersByTimeAsync(0); + + expect(api.getCharacter).toHaveBeenCalledOnce(); + expect(api.getCurrentLocation).toHaveBeenCalledOnce(); + }); + + it('retries a transient travel poll failure after one second and recovers from the API', async () => { + api.getCurrentTravel + .mockReturnValueOnce(of(expiredTravelling)) + .mockReturnValueOnce(throwError(() => new Error('Temporary failure'))) + .mockReturnValueOnce(of({ status: 'IDLE' } satisfies CurrentTravel)); + + await store.load(); + await vi.advanceTimersByTimeAsync(999); + + expect(api.getCurrentTravel).toHaveBeenCalledTimes(2); + + await vi.advanceTimersByTimeAsync(1); + + expect(api.getCurrentTravel).toHaveBeenCalledTimes(3); + expect(store.currentTravel()).toEqual({ status: 'IDLE' }); + }); + + it('ignores a late poll response after the store is destroyed', async () => { + const pendingTravel = new Subject(); + api.getCurrentTravel + .mockReturnValueOnce(of(expiredTravelling)) + .mockReturnValueOnce(pendingTravel); + + await store.load(); + store.ngOnDestroy(); + pendingTravel.next({ status: 'COMPLETED', targetLocation: travelling.targetLocation }); + pendingTravel.complete(); + await Promise.resolve(); + + expect(api.getCharacter).toHaveBeenCalledOnce(); + expect(api.getCurrentLocation).toHaveBeenCalledOnce(); + await vi.advanceTimersByTimeAsync(2_000); + expect(api.getCurrentTravel).toHaveBeenCalledTimes(2); }); it('reloads character and location only after the server reports completion', async () => { diff --git a/apps/web/src/app/features/world/world.store.ts b/apps/web/src/app/features/world/world.store.ts index c319488..b6ec9d9 100644 --- a/apps/web/src/app/features/world/world.store.ts +++ b/apps/web/src/app/features/world/world.store.ts @@ -18,6 +18,9 @@ export class WorldStore implements OnDestroy { private readonly loadingState = signal(false); private readonly errorState = signal(null); private countdownTimer: ReturnType | undefined; + private travelRetryTimer: ReturnType | undefined; + private travelPollInFlight = false; + private destroyed = false; readonly character = this.characterState.asReadonly(); readonly currentLocation = this.currentLocationState.asReadonly(); @@ -30,16 +33,31 @@ export class WorldStore implements OnDestroy { constructor(private readonly api: GameApiService) {} async load(): Promise { + if (this.destroyed) { + return; + } + this.loadingState.set(true); this.errorState.set(null); try { - const travel = await this.loadSnapshot(); + const { character, location, travel } = await this.loadSnapshot(); + if (this.destroyed) { + return; + } + + this.characterState.set(character); + this.currentLocationState.set(location); + this.selectedConnectionState.set(null); await this.setCurrentTravel(travel); } catch (error) { - this.errorState.set(this.toErrorMessage(error)); + if (!this.destroyed) { + this.errorState.set(this.toErrorMessage(error)); + } } finally { - this.loadingState.set(false); + if (!this.destroyed) { + this.loadingState.set(false); + } } } @@ -48,6 +66,10 @@ export class WorldStore implements OnDestroy { } async startTravel(): Promise { + if (this.destroyed) { + return; + } + const connection = this.selectedConnectionState(); if (!connection) { return; @@ -58,36 +80,50 @@ export class WorldStore implements OnDestroy { try { const travel = await firstValueFrom(this.api.startTravel(connection.targetLocation.id)); + if (this.destroyed) { + return; + } + await this.setCurrentTravel(travel); } catch (error) { - this.errorState.set(this.toErrorMessage(error)); + if (!this.destroyed) { + this.errorState.set(this.toErrorMessage(error)); + } } finally { - this.loadingState.set(false); + if (!this.destroyed) { + this.loadingState.set(false); + } } } ngOnDestroy(): void { + this.destroyed = true; this.stopCountdown(); + this.clearTravelRetry(); } - private async loadSnapshot(): Promise { - const { character, location, travel } = await firstValueFrom( + private loadSnapshot(): Promise<{ + character: CharacterResponse; + location: CurrentLocationResponse; + travel: CurrentTravel; + }> { + return firstValueFrom( forkJoin({ character: this.api.getCharacter(), location: this.api.getCurrentLocation(), travel: this.api.getCurrentTravel(), }), ); - - this.characterState.set(character); - this.currentLocationState.set(location); - this.selectedConnectionState.set(null); - return travel; } private async setCurrentTravel(travel: CurrentTravel): Promise { + if (this.destroyed) { + return; + } + this.currentTravelState.set(travel); this.stopCountdown(); + this.clearTravelRetry(); if (travel.status === 'TRAVELLING') { this.startCountdown(travel.arrivesAt); @@ -102,27 +138,52 @@ export class WorldStore implements OnDestroy { private startCountdown(arrivesAt: string): void { const updateRemainingSeconds = () => { + if (this.destroyed) { + return; + } + const remainingSeconds = Math.max(0, Math.ceil((Date.parse(arrivesAt) - Date.now()) / 1000)); this.remainingSecondsState.set(remainingSeconds); if (remainingSeconds === 0) { this.stopCountdown(); - void this.refreshTravelAfterCountdown(); + this.pollCurrentTravel(); } }; updateRemainingSeconds(); - if (this.countdownTimer === undefined) { + if (this.countdownTimer === undefined && this.remainingSecondsState() !== 0) { this.countdownTimer = setInterval(updateRemainingSeconds, 1_000); } } - private async refreshTravelAfterCountdown(): Promise { + private pollCurrentTravel(): void { + if (this.destroyed || this.travelPollInFlight) { + return; + } + + this.travelPollInFlight = true; + void this.refreshCurrentTravel(); + } + + private async refreshCurrentTravel(): Promise { try { const travel = await firstValueFrom(this.api.getCurrentTravel()); + if (this.destroyed) { + return; + } + await this.setCurrentTravel(travel); + if (travel.status === 'TRAVELLING') { + this.scheduleTravelRetry(); + } } catch (error) { - this.errorState.set(this.toErrorMessage(error)); + if (!this.destroyed) { + this.errorState.set(this.toErrorMessage(error)); + this.scheduleTravelRetry(); + } + } finally { + this.travelPollInFlight = false; } } @@ -133,6 +194,9 @@ export class WorldStore implements OnDestroy { location: this.api.getCurrentLocation(), }), ); + if (this.destroyed) { + return; + } this.characterState.set(character); this.currentLocationState.set(location); @@ -146,6 +210,24 @@ export class WorldStore implements OnDestroy { } } + private scheduleTravelRetry(): void { + if (this.destroyed || this.travelRetryTimer !== undefined) { + return; + } + + this.travelRetryTimer = setTimeout(() => { + this.travelRetryTimer = undefined; + this.pollCurrentTravel(); + }, 1_000); + } + + private clearTravelRetry(): void { + if (this.travelRetryTimer !== undefined) { + clearTimeout(this.travelRetryTimer); + this.travelRetryTimer = undefined; + } + } + private toErrorMessage(error: unknown): string { return error instanceof Error ? error.message : 'Unable to load world state.'; }