Fix duplicate state broadcasts from server

Author: rgilks

docs/ARCHITECTURE.md

@@ -104,6 +104,8 @@ The backend leverages Cloudflare's edge network.
 
 - **`runGameStateAction(ws, action, onSuccess)`**: Generic handler that reduces boilerplate across all 12+ action handlers. Fetches current state from storage → runs engine function in try/catch → on validation error sends error message to the WebSocket → on exception logs with game code/phase/turn and sends error (state is preserved via clone-on-entry) → on success invokes `onSuccess` callback (typically save state + broadcast). `handleTurnTimeout` has equivalent try/catch protection for the alarm-driven code path.
 
+- **Single state-bearing outbound message per action**: `publishStateChange()` persists state and events first, then emits exactly one state-bearing message (`movementResult`, `combatResult`, or `stateUpdate`). If the resulting state is terminal, the DO appends a separate `gameOver` notification after that state-bearing message. This keeps client phase transitions aligned with a one-action/one-update contract.
+
 - **Filtered broadcasting**: `broadcastFiltered()` checks whether the current scenario has hidden information (fugitive identities in escape scenarios). If no hidden info, the same state goes to both players. If hidden info, `filterStateForPlayer(state, playerId)` is called separately per player — own ships are fully visible, unrevealed enemy ships show `type: 'unknown'`. When adding new hidden state, extend `filterStateForPlayer()` and the check in `broadcastFiltered()`.
 
 - **Single-alarm scheduling**: One alarm per DO, rescheduled after each state change. Three independent deadlines are stored: `disconnectAt` (30s grace), `turnTimeoutAt` (2 min), `inactivityAt` (5 min). `getNextAlarmAt()` computes the nearest deadline. When the alarm fires, `resolveAlarmAction()` returns a discriminated action (`disconnectExpired`, `turnTimeout`, `inactivityTimeout`) and the handler dispatches accordingly.
@@ -186,7 +188,7 @@ All messages are discriminated unions validated at the protocol boundary. `GameS
 POST /create → Worker generates room code + tokens → DO /init
 WebSocket /ws/{code}?playerToken=X → DO accepts, tags socket with player ID
 Both players connected → createGame() → broadcast gameStart
-Game loop: C2S action → engine → broadcast S2C result → save state → restart timer
+Game loop: C2S action → engine → save state/events → restart timer → broadcast S2C result
 Disconnect → 30s grace period → reconnect with token or forfeit
 ```
 

docs/BACKLOG.md

@@ -4,6 +4,67 @@ Remaining work only. Completed items are in git history.
 
 ---
 
+## Review Follow-Up Plan
+
+Current direction is good. This plan is aimed at removing the
+main correctness and coordination risks found in the latest
+project review without rewriting working systems.
+
+### Phase 0.5. Tighten protocol boundary consistency
+
+- Trim chat text before validating it in `src/server/protocol.ts`.
+- Reject messages that become empty after trimming so non-UI
+  clients cannot produce blank chat log entries.
+- Add protocol tests for whitespace-only chat and normal trimmed
+  chat payloads.
+
+### Phase 1. Consolidate protocol validation
+
+- Move C2S/S2C runtime schema ownership next to the shared
+  protocol message definitions instead of maintaining a large
+  hand-written parser island in `src/server/protocol.ts`.
+- Keep room-code, token, and seat-assignment helpers separate if
+  they stay generic; focus schema consolidation on message
+  payloads first.
+- Use the shared schema layer for both runtime validation and
+  compile-time TypeScript inference where practical.
+- Target outcome: one source of truth for protocol shape, smaller
+  validator surface area, and cheaper message evolution.
+
+### Phase 2. Finish consolidating client state ownership
+
+- Extract `ClientContext` and `PlanningState` mutation behind a
+  small store or reducer layer instead of mutating the shared
+  object directly from `main.ts` and helper deps.
+- Move the imperative `setState()` side-effect block toward
+  explicit transition handlers so phase-entry behavior is easier
+  to test in isolation.
+- Keep `GameClient` as the bootstrap/wiring shell for renderer,
+  connection, and UI composition.
+- Target outcome: `main.ts` becomes orchestration glue instead of
+  the default home for future gameplay coupling.
+
+### Phase 3. Enforce shared type boundaries
+
+- Replace broad barrel imports from `shared/types` with direct
+  imports from `shared/types/domain`, `shared/types/protocol`,
+  and `shared/types/scenario`.
+- Update docs and import conventions so new modules follow the
+  bounded split by default.
+- If needed, add a lint rule or lightweight repo check once the
+  bulk migration is complete.
+- Target outcome: the type split becomes architectural reality,
+  not just a file-layout improvement.
+
+### Suggested order
+
+1. Phase 0.5: chat boundary tightening.
+2. Phase 1: protocol schema consolidation.
+3. Phase 2: client store/transition consolidation.
+4. Phase 3: direct shared-type imports and boundary enforcement.
+
+---
+
 ## Maintenance
 
 ### Phase 5. Stronger entity state models

src/server/game-do/game-do.test.ts

@@ -1,6 +1,11 @@
 import { beforeEach, describe, expect, it, vi } from 'vitest';
+import {
+  handleServerMessage,
+  type MessageHandlerDeps,
+} from '../../client/game/message-handler';
 import { must } from '../../shared/assert';
-import type { GameState } from '../../shared/types';
+import type { MovementResult } from '../../shared/engine/game-engine';
+import type { GameState, S2C } from '../../shared/types';
 import type { Env } from './game-do';
 
 vi.mock('cloudflare:workers', () => ({
@@ -21,6 +26,7 @@ import {
   SCENARIOS,
 } from '../../shared/map-data';
 import { GameDO } from './game-do';
+import { toMovementResultMessage, toStateUpdateMessage } from './messages';
 
 class MockStorage {
   private data = new Map<string, unknown>();
@@ -73,6 +79,68 @@ function createCtx(): MockDurableObjectState {
     },
   };
 }
+
+const createSocket = () => ({
+  sent: [] as string[],
+  send(payload: string) {
+    this.sent.push(payload);
+  },
+});
+
+const createMessageHandlerDeps = (
+  state: GameState | null = null,
+): MessageHandlerDeps & { transitionCount: number } => {
+  let transitionCount = 0;
+  const deps: MessageHandlerDeps & { transitionCount: number } = {
+    ctx: {
+      state: 'playing_astrogation',
+      playerId: 0,
+      gameCode: null,
+      reconnectAttempts: 0,
+      latencyMs: 0,
+      gameState: state,
+    },
+    transitionCount,
+    setState(nextState) {
+      deps.ctx.state = nextState;
+    },
+    applyGameState(nextState) {
+      deps.ctx.gameState = nextState;
+    },
+    transitionToPhase() {
+      transitionCount += 1;
+      deps.transitionCount = transitionCount;
+      deps.ctx.state = 'playing_ordnance';
+    },
+    presentMovementResult() {},
+    presentCombatResults() {},
+    showGameOverOutcome() {},
+    storePlayerToken() {},
+    resetTurnTelemetry() {},
+    onAnimationComplete() {},
+    logScenarioBriefing() {},
+    deserializeState(raw) {
+      return raw;
+    },
+    renderer: {
+      setPlayerId() {},
+      clearTrails() {},
+    },
+    ui: {
+      showToast() {},
+      logText() {},
+      setChatEnabled() {},
+      hideReconnecting() {},
+      setPlayerId() {},
+      clearLog() {},
+      showRematchPending() {},
+      showGameOver() {},
+      updateLatency() {},
+    },
+  };
+  return deps;
+};
+
 describe('GameDO', () => {
   beforeEach(() => {
     vi.restoreAllMocks();
@@ -253,4 +321,130 @@ describe('GameDO', () => {
     expect(trace.indexOf('put:gameState')).toBeLessThan(trace.indexOf('send'));
     expect(await ctx.storage.get('gameState')).toEqual(state);
   });
+
+  it('emits one state-bearing message for state-update actions', async () => {
+    const ctx = createCtx();
+    const ws = createSocket();
+    ctx.acceptWebSocket(ws, ['player:0']);
+    const game = createGameDO(ctx);
+    const state = createGame(
+      SCENARIOS.duel,
+      buildSolarSystemMap(),
+      'STAT1',
+      findBaseHex,
+    );
+
+    await (
+      game as unknown as {
+        publishStateChange: (
+          state: GameState,
+          primaryMessage?: unknown,
+          options?: { restartTurnTimer?: boolean; events?: unknown[] },
+        ) => Promise<void>;
+      }
+    ).publishStateChange(state, toStateUpdateMessage(state), {
+      restartTurnTimer: false,
+    });
+
+    const messages = ws.sent.map((payload) => JSON.parse(payload) as S2C);
+
+    expect(messages).toHaveLength(1);
+    expect(messages[0]).toEqual({
+      type: 'stateUpdate',
+      state,
+    });
+
+    const deps = createMessageHandlerDeps();
+    for (const message of messages) {
+      handleServerMessage(deps, message);
+    }
+    expect(deps.transitionCount).toBe(1);
+  });
+
+  it('keeps movement results as the only state-bearing message', async () => {
+    const ctx = createCtx();
+    const ws = createSocket();
+    ctx.acceptWebSocket(ws, ['player:0']);
+    const game = createGameDO(ctx);
+    const state = createGame(
+      SCENARIOS.duel,
+      buildSolarSystemMap(),
+      'MOVE1',
+      findBaseHex,
+    );
+    const movementResult: MovementResult = {
+      state,
+      movements: [],
+      ordnanceMovements: [],
+      events: [],
+    };
+
+    await (
+      game as unknown as {
+        publishStateChange: (
+          state: GameState,
+          primaryMessage?: unknown,
+          options?: { restartTurnTimer?: boolean; events?: unknown[] },
+        ) => Promise<void>;
+      }
+    ).publishStateChange(state, toMovementResultMessage(movementResult), {
+      restartTurnTimer: false,
+    });
+
+    const messages = ws.sent.map((payload) => JSON.parse(payload) as S2C);
+
+    expect(messages).toHaveLength(1);
+    expect(messages[0]).toEqual({
+      type: 'movementResult',
+      movements: [],
+      ordnanceMovements: [],
+      events: [],
+      state,
+    });
+  });
+
+  it('appends game-over after a single state-bearing terminal update', async () => {
+    const ctx = createCtx();
+    const ws = createSocket();
+    ctx.acceptWebSocket(ws, ['player:0']);
+    const game = createGameDO(ctx);
+    const base = createGame(
+      SCENARIOS.duel,
+      buildSolarSystemMap(),
+      'OVER1',
+      findBaseHex,
+    );
+    const state: GameState = {
+      ...base,
+      phase: 'gameOver',
+      winner: 0,
+      winReason: 'Fleet eliminated!',
+    };
+
+    await (
+      game as unknown as {
+        publishStateChange: (
+          state: GameState,
+          primaryMessage?: unknown,
+          options?: { restartTurnTimer?: boolean; events?: unknown[] },
+        ) => Promise<void>;
+      }
+    ).publishStateChange(state, toStateUpdateMessage(state), {
+      restartTurnTimer: false,
+    });
+
+    const messages = ws.sent.map((payload) => JSON.parse(payload) as S2C);
+
+    expect(messages).toEqual([
+      {
+        type: 'stateUpdate',
+        state,
+      },
+      {
+        type: 'gameOver',
+        winner: 0,
+        reason: 'Fleet eliminated!',
+      },
+    ]);
+  });
 });

src/server/game-do/game-do.ts

@@ -484,10 +484,7 @@ export class GameDO extends DurableObject<Env> {
     if (restartTurnTimer) {
       await this.startTurnTimer(state);
     }
-    if (primaryMessage) {
-      this.broadcastFiltered(primaryMessage);
-    }
-    this.broadcastEndOrUpdate(state);
+    this.broadcastStateChange(state, primaryMessage);
   }
   private async runGameStateAction<
     Success extends {
@@ -803,18 +800,17 @@ export class GameDO extends DurableObject<Env> {
       this.broadcast({ type: 'rematchPending' });
     }
   }
-  private broadcastEndOrUpdate(state: GameState) {
+  private broadcastStateChange(
+    state: GameState,
+    primaryMessage?: StatefulServerMessage,
+  ) {
+    this.broadcastFiltered(primaryMessage ?? toStateUpdateMessage(state));
     if (state.phase === 'gameOver') {
       this.broadcast({
         type: 'gameOver',
         winner: must(state.winner),
         reason: must(state.winReason),
       });
-    } else {
-      this.broadcastFiltered({
-        type: 'stateUpdate',
-        state,
-      });
     }
   }
   // --- Messaging ---