fix: harden world travel polling
This commit is contained in:
@@ -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 <Task-7 files>
|
||||
# All matched files use Prettier code style
|
||||
|
||||
git diff --check -- <Task-7 files>
|
||||
# exit 0
|
||||
```
|
||||
|
||||
@@ -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<typeof vi.fn>;
|
||||
@@ -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<CurrentTravel>();
|
||||
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<CurrentTravel>();
|
||||
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 () => {
|
||||
|
||||
@@ -18,6 +18,9 @@ export class WorldStore implements OnDestroy {
|
||||
private readonly loadingState = signal(false);
|
||||
private readonly errorState = signal<string | null>(null);
|
||||
private countdownTimer: ReturnType<typeof setInterval> | undefined;
|
||||
private travelRetryTimer: ReturnType<typeof setTimeout> | 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<void> {
|
||||
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<void> {
|
||||
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<CurrentTravel> {
|
||||
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<void> {
|
||||
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<void> {
|
||||
private pollCurrentTravel(): void {
|
||||
if (this.destroyed || this.travelPollInFlight) {
|
||||
return;
|
||||
}
|
||||
|
||||
this.travelPollInFlight = true;
|
||||
void this.refreshCurrentTravel();
|
||||
}
|
||||
|
||||
private async refreshCurrentTravel(): Promise<void> {
|
||||
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.';
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user