Sab

[scamiv]

Description:

SharedArrayBuffer tile state, backlog-aware tick ingestion, and extended performance instrumentation

Summary

• Introduces a SharedArrayBuffer-backed tile state and ring buffer so the worker can stream tile changes to the main thread without shipping full per-tick tile payloads.
• Makes the worker self-clocked and lets the client apply game updates in backlog-aware slices, reducing UI jank when the simulation falls behind.
• Extends the performance overlay with detailed metrics (backlog, worker vs render throughput, tile update volumes, SAB ring utilization) to debug performance issues.

Feature Set 1 – Shared tile state between worker and main thread

What it does

• TerrainMapLoader (src/core/game/TerrainMapLoader.ts)

  • TerrainMapData now optionally carries sharedStateBuffer and sharedDirtyBuffer.
  • loadTerrainMap(...) can take an optional sharedStateBuffer. If SAB is supported and crossOriginIsolated is true, it creates a SharedArrayBuffer of size width * height * Uint16.
  • Caching is disabled in SAB-capable environments unless we’re explicitly in the non-shared path, so each game instance gets its own mutable state.

• GameMapImpl (src/core/game/GameMap.ts)

  • The state array (per-tile mutable state) can now be backed by an externally supplied buffer (stateBuffer?: ArrayBufferLike), which in practice is a SharedArrayBuffer in SAB mode.
  • Validates that the buffer length matches width * height to avoid misaligned state.

• GameRunner (src/core/GameRunner.ts)

  • createGameRunner(...) takes a sharedStateBuffer and passes it into loadTerrainMap so the worker’s GameMapImpl views the same SharedArrayBuffer as the client.
  • From the game’s perspective, logic and rules are unchanged; only the backing storage for tile state differs.

• ClientGameRunner (src/client/ClientGameRunner.ts)

  • On client startup, loadTerrainMap(...) is called without a sharedStateBuffer. Inside the loader, if SAB is available, the returned TerrainMapData already includes a fresh SharedArrayBuffer.
  • The client then:
    • Detects if SAB is usable (SharedArrayBuffer, Atomics, and crossOriginIsolated).
    • Extracts gameMap.sharedStateBuffer if present.
    • Passes that same SharedArrayBuffer into the worker via WorkerClient and into GameRunner’s createGameRunner(...).
  • GameView (src/core/game/GameView.ts) gets a usesSharedTileState flag so it knows whether packedTileUpdates are “apply this update” or “this tile has already been updated in shared state; just mark it dirty”.

Interaction with the rest of the game

• Simulation logic (Game, GameImpl, Executor, etc.) is unchanged; they still manipulate GameMap through the existing API.
• In SAB mode:
  • Worker and main thread see the same Uint16Array for tile state. The worker writes into it during game.executeNextTick().
  • The main thread no longer re-applies state diffs; it just uses tile refs to know which tiles to redraw.
• In non-SAB environments:
  • Behavior is effectively identical to main: each thread has its own Uint16Array state, and tile updates continue to flow via packedTileUpdates.

Implications

• Performance: Eliminates per-tick copying of huge BigUint64Array tile update arrays from worker → main when SAB is available.
• Memory: One shared tile-state buffer instead of separate copies in worker and main.
• Correctness: Requires that all tile state mutations stay in the worker. The main thread must not mutate the shared state directly (and doesn’t, given current APIs).
• Environment sensitivity: The shared-state path only activates when SharedArrayBuffer + Atomics + crossOriginIsolated === true. Otherwise, the code falls back to the old, cached, non-SAB behavior.

Feature Set 2 – Shared tile update ring buffer (SAB) and dirty flags

What it does

• New ring buffer implementation (src/core/worker/SharedTileRing.ts)

  • SharedTileRingBuffers: three SharedArrayBuffers:
    • header: Int32Array with write index, read index, overflow flag.
    • data: Uint32Array of tile refs.
    • dirty: Uint8Array of per-tile dirty flags.
  • createSharedTileRingBuffers(capacity, numTiles) & createSharedTileRingViews(...) set up both buffers and typed views.
  • pushTileUpdate(...): append a tile ref into the ring, dropping oldest entries and flagging overflow if full.
  • drainTileUpdates(...): pop up to maxItems into an array, advancing the read pointer atomically.

• Worker integration (src/core/worker/Worker.worker.ts)

  • On init:
    • If sharedTileRingHeader/sharedTileRingData are provided, builds a SharedTileRingViews and keeps dirtyFlags from the dirty buffer.
    • Passes a tileUpdateSink to createGameRunner(...):
      • In the deduping path: only pushes tile ref into the ring if dirtyFlags[tile] was 0 (using Atomics.compareExchange).
      • In the simpler path: pushes every tile ref without deduping if only the ring is present.
  • From then on, every tile update in GameRunner can be streamed into the ring instead of being serialized into packedTileUpdates.

• GameRunner integration (src/core/GameRunner.ts)

  • For each tick:
    • Reads updates[GameUpdateType.Tile].
    • If tileUpdateSink exists:
      • Computes tile refs from the TileUpdate bigint and calls tileUpdateSink per tile.
      • Clears updates[GameUpdateType.Tile] and sets packedTileUpdates to an empty BigUint64Array.
    • Otherwise, fills packedTileUpdates exactly as in main.

• Client integration (src/client/ClientGameRunner.ts)

  • On startup:
    • Computes number of tiles from gameMap.gameMap.width() * height().
    • Allocates ring buffers via createSharedTileRingBuffers and passes header + data + dirty to the worker via WorkerClient.
    • Also keeps a local SharedTileRingViews and Uint8Array for dirtyFlags.
  • When processing a batch of game updates per frame:
    • Calls drainTileUpdates(...) with a MAX_TILE_UPDATES_PER_RENDER cap (~100k).
    • Deduplicates tile refs in a Set.
    • Resets dirty flags for each processed tile via Atomics.store(dirtyFlags, ref, 0).
    • Builds a new packedTileUpdates array containing tile refs as BigInt and attaches it to the merged GameUpdateViewData.

• GameView integration (src/core/game/GameView.ts)

  • In update(...), if usesSharedTileState is true:
    • Treats packedTileUpdates entries as tile refs and does not call updateTile.
    • Simply records updatedTiles so render layers can redraw changed tiles.

Interaction with the rest of the game

• The ring is purely a transport optimization between worker and main; the GameUpdateViewData still presents a packedTileUpdates array to GameView.
• Layers that rely on GameView.recentlyUpdatedTiles() continue to work; they just see tile refs drawn from the ring rather than from decoded TileUpdate bigints.
• Worker messaging remains the same shape (game_update messages); the difference is that packedTileUpdates is empty coming out of the worker in SAB mode and is populated on the client side from the ring.

Implications

• Throughput: The worker can emit tile updates at high volume without incurring postMessage serialization costs.
• GC pressure: Moves from “allocate large BigUint64Array on every tick” to “reuse fixed-size SAB ring and small per-frame arrays”.
• Overflow behavior: When the ring fills, oldest tile refs are dropped and an overflow flag is set; the overlay surfaces whether overflow ever occurred, making it visible when the system is under-provisioned.
• Deduping semantics: With dirty flags, each tile ref appears at most once in the ring per “dirty” period; this can slightly reduce redundant redraws on the main thread compared to the naïve per-update approach.

Feature Set 3 – Backlog-aware tick ingestion and self-clocked worker

What it does

• Worker overhaul (src/core/worker/Worker.worker.ts)

  • Removes the "heartbeat" message type; the worker no longer depends on the main thread poking it.
  • Introduces processPendingTurns():
    • If a GameRunner exists and hasPendingTurns(), loops through executeNextTick() until there are no pending turns.
    • Guarded by an isProcessingTurns flag to avoid re-entrancy.
  • Calls processPendingTurns():
    • Once after initialization is complete.
    • After every turn message is added via addTurn(...).

• WorkerClient (src/core/worker/WorkerClient.ts) & messages (src/core/worker/WorkerMessages.ts)

  • Drops sendHeartbeat() and the "heartbeat" message type entirely.
  • Extends InitMessage to carry shared ring buffers and shared state buffer to the worker.

• ClientGameRunner tick ingestion (src/client/ClientGameRunner.ts)

  • Previously: each game_update from the worker was applied immediately; render and simulation tick rate were tightly coupled.
  • Now:
    • All GameUpdateViewData are appended to pendingUpdates.
    • processPendingUpdates():
      • Runs in requestAnimationFrame slices.
      • Computes a per-frame time budget (sliceBudgetMs) based on the current backlog (backlogTurns), scaling from 8ms up to 1000ms as backlog grows from ~10 to 500 turns.
      • Consumes up to MAX_TICKS_PER_SLICE (1000) updates or until it runs out of budget.
      • For each consumed update:
        • Tells the transport that a turn is complete.
        • Emits SendHashEvent for server hash checking.
        • Updates backlog metrics (updateBacklogMetrics) based on serverTurnHighWater and the tick number.
        • Saves the game if a Win update is present.
      • Once the queue is drained for this batch:
        • Merges the batch’s non-tile updates and tile updates (via ring or fallback).
        • Applies a single gameView.update(...).
        • Calls renderer.tick().
        • Emits TickMetricsEvent with:
          • Last tick execution time (from worker).
          • Network tick delay (currentTickDelay between server messages).
          • Backlog turns, ticks-per-render, worker ticks/s, render ticks/s.
          • Tile update counts and ring buffer metrics (see Feature Set 4).

• Backlog metrics and events

  • ClientGameRunner.updateBacklogMetrics(processedTick) maintains:
    • backlogTurns = max(0, serverTurnHighWater - lastProcessedTick).
    • backlogGrowing flag (whether backlog increased since last sample).
  • Emits BacklogStatusEvent(backlogTurns, backlogGrowing) whenever metrics update.
  • GameRenderer (src/client/graphics/GameRenderer.ts) listens for BacklogStatusEvent and stores backlogTurns.

Interaction with the rest of the game

• Server/client protocol:
  • ServerMessage handling is unchanged for start, desync, error, turn messages.
  • The client still sends Turn intents to the worker and marks them complete with transport.turnComplete(); this now happens inside the per-frame processing loop rather than immediately on each game_update.
• Replay and singleplayer:
  • Both paths use the same ClientGameRunner and worker; the backlog logic applies uniformly, making heavy replays and high-speed games smoother.

Implications

• Responsiveness: When the client falls behind (e.g., slow machine, heavy game, or replays), it can process multiple simulation ticks per render frame without freezing the UI.
• Smoothness: Rendering is no longer forced to run at worker tick rate; frames are decoupled via batching and ticksPerRender tracking.
• Risk: The “self-clocked” worker must never rely on extra heartbeats for progress; all future features that enqueue work must do it through addTurn or analogous queued work so processPendingTurns() sees it.

Feature Set 4 – Render throttling based on simulation backlog

What it does

• GameRenderer.renderGame() (src/client/graphics/GameRenderer.ts)
  • Before rendering:
    • If backlogTurns > 0, dynamically reduces target FPS from 60 down to 10 as backlog grows (capped at 50 turns).
    • Computes a minimum frame interval from this target; if the last render was too recent, immediately schedules another requestAnimationFrame and skips work for this frame.
  • Tracks lastRenderTime to enforce the throttle.

Interaction with the rest of the game

• This is purely a rendering-side change; all game logic and worker behavior remain the same.
• The performance overlay still receives per-frame durations and layer breakdowns via updateFrameMetrics as before.

Implications

• Catch-up behavior: When simulation is behind, rendering yields some headroom for the worker and main-thread tick ingestion to catch up, at the cost of visual frame rate.
• User perception: The game will feel “slower” visually during extreme backlog, but stays interactive and converges instead of hard-freezing.
• Tuning knobs: BASE_FPS, MIN_FPS, and BACKLOG_MAX_TURNS are the primary tuning constants; they are currently hard-coded.

Feature Set 5 – Extended performance overlay & instrumentation

What it does

• Input events (src/client/InputHandler.ts)

  • TickMetricsEvent gains:
    • backlogTurns
    • ticksPerRender
    • workerTicksPerSecond, renderTicksPerSecond
    • tileUpdatesCount
    • ringBufferUtilization, ringBufferOverflows, ringDrainTime
  • New BacklogStatusEvent(backlogTurns, backlogGrowing) to communicate backlog status to the renderer (and any future listeners).

• Performance overlay (src/client/graphics/layers/PerformanceOverlay.ts)

  • Subscribes to the richer TickMetricsEvent and updates new reactive state fields:
    • Backlog turns and tick throughput (worker/render).
    • Ticks per render frame.
    • Tile updates per render, peak tile updates, total tile updates.
    • Ring buffer utilization (% of capacity touched per render), whether any overflow occurred, and time spent draining the ring.
  • Resets tile metrics when the overlay is cleared.
  • Exposes tile-related metrics in getSnapshot() under a new tiles section.
  • Renders new lines in the overlay UI:
    • Worker ticks/s, render ticks/s, ticks per render.
    • Backlog turns.
    • Tile updates/render (current and peak).
    • Ring buffer utilization and overflow count.
    • Ring drain time in ms.

Interaction with the rest of the game

• The overlay remains entirely optional, controlled by UserSettings: updateTickMetrics is a no-op when overlay is disabled or hidden.
• No gameplay logic depends on these new fields; they are purely diagnostic.

Implications

• Debuggability: Makes it much easier to understand whether bottlenecks are in the worker, main-thread tick ingestion, rendering, or tile update streaming.
• Monitoring: Ring buffer metrics expose whether the SAB pipeline is appropriately sized for the map size and load.

Risks / Edge Cases

• SAB Availability: On environments without SharedArrayBuffer (or without crossOriginIsolated), all the new shared-memory paths automatically fall back to the old behavior. It’s important to test both modes.
• Overflow behavior: If the tile ring overflows regularly (e.g., massive tile churn + too small capacity), some tile updates can be dropped before they’re rendered, leading to temporary visual desyncs. The overflow flag and tile metrics are there to make this visible.
• Concurrency: Shared buffers use Atomics to synchronize indices and dirty flags; correct behavior assumes no direct writes outside of the established push/drain/reset flow.

Testing Suggestions

• Run a normal multiplayer game on a SAB-capable origin (crossOriginIsolated true) and:
  • Verify that the game plays normally and rendering remains correct.
  • Observe the performance overlay to confirm reasonable worker/render tick rates and tile update counts.
• Run on an environment where SAB is disabled:
  • Confirm that games still load and run (fallback path).
• Force heavy load (large map, late-game, or high replay speed) and:
  • Check that backlog metrics increase and then decrease as the client catches up.
  • Confirm that render FPS drops under heavy backlog and recovers later.
  • Watch ring buffer utilization and overflow to ensure the capacity scaling per map size is adequate.

Please complete the following:

• I have added screenshots for all UI updates
• I process any text displayed to the user through translateText() and I've added it to the en.json file
• I have added relevant tests to the test directory
• I confirm I have thoroughly tested these changes and take full responsibility for any bugs introduced

Please put your Discord username so you can be contacted if a bug or regression is found:

DISCORD_USERNAME

[coderabbitai]

Walkthrough

Threads a SharedTileRing (SAB) through worker and client, removes the heartbeat in favor of explicit pending-turn processing, adds client-side batched update processing with backlog/backpressure tracking and metrics, extends tick/overlay metrics with tile/ring stats, and adds backlog-aware renderer pacing.

Changes

Cohort / File(s)	Summary
Shared Tile Ring Infrastructure src/core/worker/SharedTileRing.ts	New SAB-backed ring buffer module: buffer/view types, header constants, creation helpers, pushTileUpdate and drainTileUpdates using Atomics.
Worker Protocol & Client Init src/core/worker/WorkerMessages.ts, src/core/worker/WorkerClient.ts, src/core/worker/Worker.worker.ts	Removed heartbeat message; InitMessage now accepts optional SAB fields (sharedTileRingHeader, sharedTileRingData, sharedStateBuffer, sharedDirtyBuffer). WorkerClient forwards SABs; worker initializes tile ring views, provides tileUpdate sink to GameRunner, and processes pending turns via processPendingTurns().
Client Game Runner — Batching & Backlog src/client/ClientGameRunner.ts	Added pendingUpdates queue, time-sliced processPendingUpdates, mergeGameUpdates, server-turn/backlog tracking, emits enriched TickMetricsEvent and BacklogStatusEvent, and constructor wiring for tileRingViews and dirtyFlags.
Events / Input Handler src/client/InputHandler.ts	TickMetricsEvent extended with backlog, ticks-per-render, worker/render rates, tile/ring metrics; new BacklogStatusEvent class added.
Renderer Frame Pacing src/client/graphics/GameRenderer.ts	Subscribes to BacklogStatusEvent; adds backlogTurns, backlogGrowing, lastRenderTime fields; implements backlog-aware frame pacing to throttle/pace renders.
Performance Overlay src/client/graphics/layers/PerformanceOverlay.ts	updateTickMetrics signature expanded; stores and displays tile updates, ring buffer utilization/overflows, ring drain time, worker/render rates and aggregated tile stats; snapshot/UI updated.
Core Game State & Map Loading src/core/game/GameMap.ts, src/core/game/TerrainMapLoader.ts, src/core/game/GameView.ts, src/core/GameRunner.ts	GameMapImpl accepts optional external/shared state buffer; TerrainMapLoader accepts/propagates shared state/dirty buffers and exposes them in TerrainMapData; GameView gains usesSharedTileState flag and branches update logic accordingly; GameRunner accepts optional tileUpdateSink and exposes hasPendingTurns().

Sequence Diagram(s)

sequenceDiagram
    participant Main as Main Thread
    participant Worker as Worker (worker thread)
    participant GameRun as GameRunner
    participant TileRing as SharedTileRing (SAB)
    participant Client as ClientGameRunner
    participant View as GameView
    participant Render as Renderer
    participant Metrics as EventBus

    Note over Main,Worker: Initialization
    Main->>Worker: InitMessage (+optional SABs)
    Worker->>GameRun: createGameRunner(..., tileUpdateSink)
    Worker->>TileRing: createSharedTileRingViews(buffers)  -- if provided

    Note over Worker: Turn processing
    Worker->>Worker: processPendingTurns()
    loop while GameRun.hasPendingTurns()
        Worker->>GameRun: execute next turn
        GameRun->>TileRing: pushTileUpdate(tileRef) via sink (Atomics)  -- if SAB used
    end
    Worker->>Main: postMessage(GameUpdate batch)

    Note over Main,Client: Client receives updates
    Main->>Client: onmessage(GameUpdate)
    Client->>Client: enqueue pendingUpdates
    Client->>Client: processPendingUpdates (time-sliced)
    Client->>Client: mergeGameUpdates -> combinedPackedTileUpdates + tile metrics
    alt usesSharedTileState
        Client->>View: notify dirtyFlags / push shared refs
    else
        Client->>View: apply packed tile updates
    end
    Client->>Metrics: emit TickMetricsEvent & BacklogStatusEvent
    Client->>Render: request render
    Render->>Render: throttle/draw based on backlogTurns

Loading

sequenceDiagram
    participant Client as ClientGameRunner
    participant Queue as pendingUpdates
    participant Merge as mergeGameUpdates
    participant View as GameView
    participant Metrics as EventBus

    Client->>Queue: enqueue(update)
    Client->>Client: processPendingUpdates (frame time-slice)
    loop within timeslice
        Queue->>Merge: collect batch
    end
    Merge->>View: apply merged update (or push shared refs)
    Merge->>Metrics: emit SendHashEvent(s) and TickMetricsEvent
    Metrics->>PerformanceOverlay: updateTickMetrics(...)

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

• Pay special attention to:
  • src/client/ClientGameRunner.ts — batching semantics, time-slice correctness, merge/dedup logic, metric correctness.
  • src/core/worker/SharedTileRing.ts — Atomics layout, pointer arithmetic, overflow handling.
  • src/core/worker/Worker.worker.ts — processPendingTurns reentrancy and ordering guarantees.
  • src/core/game/TerrainMapLoader.ts & src/core/game/GameMap.ts — shared buffer sizing, validation, and cache conditions.
  • Renderer/overlay: backlog pacing math and presentation correctness.

Possibly related PRs

• Catch up mode #2507 — Direct overlap: client batching/backlog, TickMetrics, worker turn processing and heartbeat removal.
• Add performance stats #2338 — Related telemetry and renderer/overlay performance-metric changes.
• Feat : focus attack average position and some movement camera fixes #740 — Related modifications to core GameRunner surface and runtime behavior.

Suggested labels

Feature - Frontend

Suggested reviewers

• scottanderson
• evanpelle

Poem

🧩 Shared rings hum where tile updates play,
Batches gather turns to find their way,
Backlogs counted, render breaths grow slow,
Metrics bloom so we can see them flow,
Threads align — the game moves on, aglow.

Pre-merge checks

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.
Title check	❓ Inconclusive	The title 'Sab' is extremely vague and does not clearly convey the scope or primary purpose of these significant changes to the codebase.	Replace with a descriptive title like 'Add SharedArrayBuffer tile state, ring buffer, and backlog-aware ingestion' to clearly indicate the main architectural changes.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Description check	✅ Passed	The description is comprehensive, well-structured, and directly relates to all major changes in the changeset, covering SharedArrayBuffer integration, tile ring buffers, backlog-aware processing, render throttling, and performance instrumentation.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 6

🧹 Nitpick comments (3)

src/client/InputHandler.ts (1)

128-146: Consider an options object for cleaner API.

The TickMetricsEvent constructor now has 10 optional parameters. This works but can be hard to read at call sites. Consider using an options object:

interface TickMetricsData {
  tickExecutionDuration?: number;
  tickDelay?: number;
  backlogTurns?: number;
  // ... etc
}

export class TickMetricsEvent implements GameEvent {
  constructor(public readonly data: Partial<TickMetricsData> = {}) {}
}

This makes call sites clearer and easier to extend. Not blocking, just a suggestion for maintainability.

src/core/GameRunner.ts (1)

183-196: Tile update routing looks correct.

The branch logic cleanly separates the two paths: push to sink when available, otherwise pack into array. One minor note: creating new BigUint64Array() on every tick when using the sink path allocates an empty typed array each time.

Consider reusing a constant empty array:

+const EMPTY_TILE_UPDATES = new BigUint64Array(0);
+
 // Many tiles are updated; either publish them via a shared sink or pack
 // them into the view data.
 let packedTileUpdates: BigUint64Array;
 const tileUpdates = updates[GameUpdateType.Tile];
 if (this.tileUpdateSink !== undefined) {
   for (const u of tileUpdates) {
     const tileRef = Number(u.update >> 16n) as TileRef;
     this.tileUpdateSink(tileRef);
   }
-  packedTileUpdates = new BigUint64Array();
+  packedTileUpdates = EMPTY_TILE_UPDATES;
 } else {

src/client/ClientGameRunner.ts (1)

670-676: Type casting loses safety.

The as unknown as any[] pattern bypasses TypeScript's type system. Consider a more explicit approach:

 for (const key in gu.updates) {
   const type = Number(key) as GameUpdateType;
-  // We don't care about the specific update subtype here; just treat it
-  // as an array we can concatenate.
-  const updatesForType = gu.updates[type] as unknown as any[];
-  (combinedUpdates[type] as unknown as any[]).push(...updatesForType);
+  const source = gu.updates[type];
+  const target = combinedUpdates[type];
+  for (let i = 0; i < source.length; i++) {
+    target.push(source[i]);
+  }
 }

This avoids the double cast and spread operator overhead on large arrays.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7d8c1c2 and c1bc4a7.

📒 Files selected for processing (12)

• src/client/ClientGameRunner.ts (11 hunks)
• src/client/InputHandler.ts (1 hunks)
• src/client/graphics/GameRenderer.ts (4 hunks)
• src/client/graphics/layers/PerformanceOverlay.ts (6 hunks)
• src/core/GameRunner.ts (5 hunks)
• src/core/game/GameMap.ts (2 hunks)
• src/core/game/GameView.ts (3 hunks)
• src/core/game/TerrainMapLoader.ts (4 hunks)
• src/core/worker/SharedTileRing.ts (1 hunks)
• src/core/worker/Worker.worker.ts (4 hunks)
• src/core/worker/WorkerClient.ts (3 hunks)
• src/core/worker/WorkerMessages.ts (1 hunks)

🧰 Additional context used

🧠 Learnings (6)

📚 Learning: 2025-10-20T20:15:28.858Z

Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:51-51
Timestamp: 2025-10-20T20:15:28.858Z
Learning: In src/core/execution/FakeHumanExecution.ts, game balance constants like MIRV_COOLDOWN_TICKS, MIRV_HESITATION_ODDS, VICTORY_DENIAL_TEAM_THRESHOLD, VICTORY_DENIAL_INDIVIDUAL_THRESHOLD, and STEAMROLL_CITY_GAP_MULTIPLIER are experimental tuning parameters subject to frequent change during balance testing. Do not flag changes to these values as issues or compare them against previous values.

Applied to files:

• src/client/InputHandler.ts
• src/core/GameRunner.ts
• src/client/graphics/layers/PerformanceOverlay.ts

📚 Learning: 2025-10-21T20:06:04.823Z

Learnt from: Saphereye
Repo: openfrontio/OpenFrontIO PR: 2233
File: src/client/HostLobbyModal.ts:891-891
Timestamp: 2025-10-21T20:06:04.823Z
Learning: For the HumansVsNations game mode in `src/client/HostLobbyModal.ts` and related files, the implementation strategy is to generate all nations and adjust their strength for balancing, rather than limiting lobby size based on the number of available nations on the map.

Applied to files:

• src/core/game/TerrainMapLoader.ts

📚 Learning: 2025-10-08T17:14:49.369Z

Learnt from: Foorack
Repo: openfrontio/OpenFrontIO PR: 2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.369Z
Learning: In `ClientGameRunner.ts`, the `myPlayer` field is always set when `shouldPreventWindowClose()` is called, so the null check in that method is sufficient without needing to fetch it again from `gameView.playerByClientID()`.

Applied to files:

• src/core/GameRunner.ts
• src/client/ClientGameRunner.ts

📚 Learning: 2025-06-09T02:20:43.637Z

Learnt from: VariableVince
Repo: openfrontio/OpenFrontIO PR: 1110
File: src/client/Main.ts:293-295
Timestamp: 2025-06-09T02:20:43.637Z
Learning: In src/client/Main.ts, during game start in the handleJoinLobby callback, UI elements are hidden using direct DOM manipulation with classList.add("hidden") for consistency. This includes modals, buttons, and error divs. The codebase follows this pattern rather than using component APIs for hiding elements during game transitions.

Applied to files:

• src/client/ClientGameRunner.ts
• src/client/graphics/GameRenderer.ts

📚 Learning: 2025-08-12T00:31:50.144Z

Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 1752
File: src/core/game/Game.ts:750-752
Timestamp: 2025-08-12T00:31:50.144Z
Learning: In the OpenFrontIO codebase, changes to the PlayerInteraction interface (like adding canDonateGold and canDonateTroops flags) do not require corresponding updates to src/core/Schemas.ts or server serialization code.

Applied to files:

• src/client/ClientGameRunner.ts

📚 Learning: 2025-10-18T11:00:57.142Z

Learnt from: NewYearNewPhil
Repo: openfrontio/OpenFrontIO PR: 2230
File: src/client/graphics/GameRenderer.ts:269-277
Timestamp: 2025-10-18T11:00:57.142Z
Learning: In src/client/graphics/GameRenderer.ts, the GameRecapCapture implementation does not use setCaptureRenderEnabled on layers. Instead, it uses RecapCaptureSurface.capture() to render capture layers (TerrainLayer, TerritoryLayer, RailroadLayer, StructureIconsLayer, UnitLayer) directly to an off-screen canvas without requiring layer-level capture mode methods.

Applied to files:

• src/client/graphics/GameRenderer.ts

🧬 Code graph analysis (7)

src/client/InputHandler.ts (1)

src/core/EventBus.ts (1)

• GameEvent (1-1)

src/core/game/TerrainMapLoader.ts (2)

src/core/game/GameImpl.ts (2)

• map (197-199)
• miniMap (200-202)

src/client/TerrainMapFileLoader.ts (1)

• terrainMapFileLoader (4-4)

src/core/worker/SharedTileRing.ts (1)

src/core/game/GameMap.ts (1)

• TileRef (3-3)

src/core/worker/Worker.worker.ts (3)

src/core/worker/SharedTileRing.ts (3)

• SharedTileRingViews (9-14)
• createSharedTileRingViews (31-43)
• pushTileUpdate (45-64)

src/core/GameRunner.ts (1)

• createGameRunner (35-95)

src/core/game/GameMap.ts (1)

• TileRef (3-3)

src/core/GameRunner.ts (3)

src/core/game/GameView.ts (1)

• tile (106-108)

src/core/game/UnitImpl.ts (1)

• tile (176-178)

src/core/game/GameMap.ts (1)

• TileRef (3-3)

src/core/game/GameMap.ts (3)

src/core/game/GameView.ts (2)

• width (701-703)
• height (704-706)

src/core/game/GameImpl.ts (2)

• width (807-809)
• height (810-812)

src/client/graphics/TransformHandler.ts (1)

• width (48-50)

src/client/graphics/GameRenderer.ts (1)

src/client/InputHandler.ts (1)

• BacklogStatusEvent (148-153)

🔇 Additional comments (23)

src/core/game/GameMap.ts (1)

78-103: Clean addition of shared state buffer support.

The optional stateBuffer parameter keeps backward compatibility. The length validation is correct, and creating a Uint16Array view on the shared buffer is the right approach.

One small note: since this buffer can be shared across threads (via SharedArrayBuffer), callers must ensure proper synchronization when reading/writing state. The current design assumes the worker owns writes and the main thread reads, which should be fine.

src/core/worker/WorkerMessages.ts (1)

34-42: Clean message type extension.

The optional SharedArrayBuffer fields allow the worker to operate in either shared-memory mode or fallback mode. Good design for progressive enhancement.

src/core/worker/WorkerClient.ts (2)

23-29: Good constructor extension for shared buffer support.

The optional parameters keep backward compatibility. Storing them as private fields and forwarding to the worker init message is clean.

---

72-81: Initialization payload correctly wires shared buffers.

The optional chaining on sharedTileRingBuffers?.header handles the undefined case properly.

src/client/InputHandler.ts (1)

148-153: Simple and clear event class.

BacklogStatusEvent is straightforward with required fields. Good.

src/client/graphics/GameRenderer.ts (2)

356-378: Good backlog-aware frame pacing implementation.

The logic reduces render FPS linearly from 60 to 10 as backlog grows from 0 to 50 turns. This allows the simulation to catch up while still showing progress. The early return with requestAnimationFrame keeps the loop alive.

One edge case: if lastRenderTime is 0 (first frame), the check on line 368 ensures we skip throttling, which is correct.

---

316-321: Event subscription correctly updates state.

The BacklogStatusEvent handler updates internal state that drives the frame pacing logic.

src/core/worker/Worker.worker.ts (2)

44-65: Solid reentrancy guard for turn processing.

The isProcessingTurns flag prevents recursive calls. The try/finally ensures the flag is reset even on error.

---

90-99: Clever conditional for tile update sink.

The three-way ternary handles:

1. Ring + dirty flags: uses atomic compare-exchange for deduplication
2. Ring only: direct push (no dedup)
3. No ring: undefined (fallback mode)

The Atomics.compareExchange(dirtyFlags!, tile, 0, 1) === 0 pattern is correct for single-writer "mark if not already marked" logic.

src/client/graphics/layers/PerformanceOverlay.ts (3)

545-548: Verify overflow accumulation logic.

Line 547 does this.ringBufferOverflows += ringBufferOverflows. This assumes each call passes an increment (0 or 1), not a cumulative count. Make sure the caller (TickMetricsEvent producer) sends delta values, not totals. Otherwise the displayed count will grow incorrectly.

---

440-469: Good use of Lit reactive state for new metrics.

All new fields use @state() decorator, which triggers re-renders when values change. Clean approach.

---

326-333: Reset properly clears new tile metrics.

All new fields are zeroed, maintaining consistency with the reset behavior.

src/core/GameRunner.ts (2)

40-48: LGTM! Clean extension of the factory function.

The new optional parameters tileUpdateSink and sharedStateBuffer are well-placed and properly forwarded. This allows callers to opt-in to shared memory features without breaking existing usage.

---

292-295: LGTM! Simple and clear method.

The hasPendingTurns() method provides a clean way to check if there are queued turns to process.

src/core/game/TerrainMapLoader.ts (2)

73-80: stateBuffer only passed for Normal map size.

The stateBuffer is passed to genTerrainFromBin only for GameMapSize.Normal (line 78), not for GameMapSize.Compact (line 80). Verify this is intentional.

---

115-132: LGTM! Clean parameter addition.

The optional stateBuffer parameter is properly forwarded to GameMapImpl.

src/client/ClientGameRunner.ts (3)

173-203: LGTM! Good setup for shared buffer infrastructure.

The cross-origin isolation check and conditional buffer creation is well-structured. The ring capacity scaling with world size (numTiles) is a sensible choice.

---

500-569: Solid frame-pacing logic with adaptive budget.

The dynamic slice budget based on backlog size is a good approach for catching up while keeping UI responsive. The constants are well-documented.

One small note: the array compaction at lines 562-569 could be optimized by using a deque or circular buffer pattern instead of slice(), but for typical backlog sizes this should be fine.

---

745-756: LGTM! Clean backlog tracking.

The metrics update and event emission for backlog status is straightforward and well-structured.

src/core/worker/SharedTileRing.ts (4)

3-14: LGTM! Clean interface definitions.

The separation between raw SharedArrayBuffer handles and typed Views is a good pattern - it allows the buffers to be transferred between workers while views are created locally.

---

66-85: LGTM! Drain implementation is straightforward.

The drain loop correctly advances the read pointer atomically after processing. The maxItems limit prevents unbounded draining.

---

21-29: I'll help you verify the review comment about BacklogStatusEvent. Let me search the codebase to understand the current state of this event definition and its imports.
<function_calls>

shell
#!/bin/bash

Search for BacklogStatusEvent definition and usage

rg -n "BacklogStatusEvent" --type=ts -A3 -B3


</function_calls>

Now let me check the ClientGameRunner.ts file specifically at the mentioned import line:
<function_calls>

shell
#!/bin/bash

Find ClientGameRunner.ts and check imports around line 39

fd -n "ClientGameRunner.ts" --type=f | head -1 | xargs -I {} sh -c 'echo "File: {}"; head -50 {} | tail -15'


</function_calls>

Let me also check the InputHandler file to see what it exports:
<function_calls>

shell
#!/bin/bash

Find InputHandler file and check exports

fd -n "InputHandler" --type=f -e ts | head -5


</function_calls>

---

45-64: Since the repository clone is failing, I'll analyze the concern based on the code snippet provided and my understanding of concurrent ring buffer patterns.

Producer-side overflow handling does create a race condition in concurrent scenarios.

Analyzing the code snippet:

1. Lines 52-54: Read both write and read pointers atomically (independent loads)
2. Lines 58-59: If buffer is full, the producer advances the read pointer by storing to TILE_RING_HEADER_READ_INDEX
3. Line 60: Producer writes the value
4. Line 61: Producer advances write pointer

The race condition described is valid: if the consumer is simultaneously reading and advancing read, and the producer also advances read during overflow, they can interfere with each other's view of the buffer state. This is a known issue with producer-side overflow handling in lock-free ring buffers.

However, without access to:

• How pushTileUpdate is actually called (single producer vs. multiple producers)
• The consumer implementation
• Any documentation or tests
• Whether there are additional synchronization mechanisms in place

I cannot definitively determine whether this is an actual problem or if the SPSC assumption is correctly maintained in practice.

---

Producer-side overflow handling can cause entries to be skipped if producer and consumer race during overflow, as described. However, verification requires examining actual usage patterns, consumer implementation, and any synchronization guarantees in the codebase.

Without access to the repository to verify the usage patterns and consumer side implementation, I cannot confirm whether the SPSC model is enforced or if alternative synchronization exists. The theoretical concern is valid, but practical impact depends on usage context.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Missing translations for new metric labels.

The new performance lines use hardcoded English strings like "Worker ticks/s:", "Backlog turns:", etc. Other labels in this file use translateText() for localization.

Consider adding translation keys:

        <div class="performance-line">
-         Worker ticks/s:
+         ${translateText("performance_overlay.worker_ticks")}
          <span>${this.workerTicksPerSecond.toFixed(1)}</span>
        </div>

This keeps consistency with the rest of the overlay.

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents

In src/client/graphics/layers/PerformanceOverlay.ts around lines 707 to 737,
several new performance label strings are hardcoded in English (e.g., "Worker
ticks/s:", "Render ticks/s:", "Ticks per render:", "Backlog turns:", "Tile
updates/render:", "Ring buffer:", "Ring drain time:") instead of using the
existing translateText() localization helper; replace each hardcoded label with
a call to translateText() using appropriate translation keys (add new keys to
the i18n resource files if needed), keep any punctuation/formatting the same,
and ensure the translated strings are used consistently with surrounding labels
so the overlay supports localization.

[coderabbitai]

Actionable comments posted: 1

♻️ Duplicate comments (1)

src/client/graphics/layers/PerformanceOverlay.ts (1)

707-737: Missing translations for new metric labels.

These new performance lines use hardcoded English strings instead of translateText(), breaking localization consistency with the rest of the overlay.

As noted in the previous review, wrap each label with translateText():

         <div class="performance-line">
-          Worker ticks/s:
+          ${translateText("performance_overlay.worker_ticks_per_second")}
           <span>${this.workerTicksPerSecond.toFixed(1)}</span>
         </div>

Apply similar changes to all new labels: "Render ticks/s:", "Ticks per render:", "Backlog turns:", "Tile updates/render:", "Ring buffer:", and "Ring drain time:". Remember to add the corresponding keys to en.json.

Based on learnings, translation keys should be added to en.json only; other locale files will be updated by dedicated translation PRs.

🧹 Nitpick comments (1)

src/client/graphics/layers/PerformanceOverlay.ts (1)

326-332: Consider resetting all new metrics for consistency.

The reset clears tile/ring metrics but not backlogTurns, ticksPerRender, workerTicksPerSecond, and renderTicksPerSecond. For consistency, consider resetting these as well:

     // reset tile metrics
     this.tileUpdatesPerRender = 0;
     this.tileUpdatesPeak = 0;
     this.ringBufferUtilization = 0;
     this.ringBufferOverflows = 0;
     this.ringDrainTime = 0;
     this.totalTilesUpdated = 0;
+
+    // reset tick/backlog metrics
+    this.backlogTurns = 0;
+    this.ticksPerRender = 0;
+    this.workerTicksPerSecond = 0;
+    this.renderTicksPerSecond = 0;

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c1bc4a7 and 1e3d2e1.

📒 Files selected for processing (2)

• src/client/graphics/layers/PerformanceOverlay.ts (6 hunks)
• src/core/game/TerrainMapLoader.ts (4 hunks)

🧰 Additional context used

🧠 Learnings (5)

📚 Learning: 2025-06-02T14:27:37.609Z

Learnt from: andrewNiziolek
Repo: openfrontio/OpenFrontIO PR: 1007
File: resources/lang/de.json:115-115
Timestamp: 2025-06-02T14:27:37.609Z
Learning: For OpenFrontIO project: When localization keys are renamed in language JSON files, the maintainers separate technical changes from translation content updates. They wait for community translators to update the actual translation values rather than attempting to translate in the same PR. This allows technical changes to proceed while ensuring accurate translations from native speakers.

Applied to files:

• src/client/graphics/layers/PerformanceOverlay.ts

📚 Learning: 2025-08-16T10:52:08.292Z

Learnt from: TheGiraffe3
Repo: openfrontio/OpenFrontIO PR: 884
File: resources/lang/en.json:456-461
Timestamp: 2025-08-16T10:52:08.292Z
Learning: In OpenFrontIO, translation files in resources/lang/*.json (except en.json) should not be updated in regular PRs. Only dedicated translation PRs titled "mls" and made by Aotumori should update non-English locale files. Regular PRs should only update en.json when adding or modifying translation keys.

Applied to files:

• src/client/graphics/layers/PerformanceOverlay.ts

📚 Learning: 2025-10-20T20:15:28.858Z

Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:51-51
Timestamp: 2025-10-20T20:15:28.858Z
Learning: In src/core/execution/FakeHumanExecution.ts, game balance constants like MIRV_COOLDOWN_TICKS, MIRV_HESITATION_ODDS, VICTORY_DENIAL_TEAM_THRESHOLD, VICTORY_DENIAL_INDIVIDUAL_THRESHOLD, and STEAMROLL_CITY_GAP_MULTIPLIER are experimental tuning parameters subject to frequent change during balance testing. Do not flag changes to these values as issues or compare them against previous values.

Applied to files:

• src/client/graphics/layers/PerformanceOverlay.ts

📚 Learning: 2025-11-26T20:49:29.110Z

Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 2519
File: src/core/game/GameView.ts:516-525
Timestamp: 2025-11-26T20:49:29.110Z
Learning: In GameView.ts, when usesSharedTileState is true (SAB mode), packedTileUpdates contains unpacked tile references as BigInt(tileRef) only, because all tile state lives in the shared Uint16Array. In non-SAB mode, packedTileUpdates contains packed TileUpdate bigints in the format (tileRef << 16n | state), which must be decoded via updateTile(tu). Therefore, Number(tu) is correct in SAB mode and shifting right by 16 bits would be wrong.

Applied to files:

• src/core/game/TerrainMapLoader.ts

📚 Learning: 2025-10-21T20:06:04.823Z

Learnt from: Saphereye
Repo: openfrontio/OpenFrontIO PR: 2233
File: src/client/HostLobbyModal.ts:891-891
Timestamp: 2025-10-21T20:06:04.823Z
Learning: For the HumansVsNations game mode in `src/client/HostLobbyModal.ts` and related files, the implementation strategy is to generate all nations and adjust their strength for balancing, rather than limiting lobby size based on the number of available nations on the map.

Applied to files:

• src/core/game/TerrainMapLoader.ts

🧬 Code graph analysis (1)

src/core/game/TerrainMapLoader.ts (1)

src/client/TerrainMapFileLoader.ts (1)

• terrainMapFileLoader (4-4)

🔇 Additional comments (11)

src/core/game/TerrainMapLoader.ts (7)

9-10: LGTM! Clean type extension.

Adding optional SharedArrayBuffer fields to the type definition is backwards compatible and clearly supports the new SAB-based tile state sharing.

---

42-55: Clarify the caching strategy.

The caching logic introduces three boolean flags (useCache, canUseSharedBuffers, shouldUseCache) to determine when to cache. The behavior is:

• Cache is skipped when sharedStateBuffer is provided (expected)
• Cache is also skipped when canUseSharedBuffers is true, even if no buffer was provided

This means when SharedArrayBuffer is available, each call without a provided buffer will create a new SharedArrayBuffer instead of reusing a cached one. If this prevents inappropriate state sharing across game instances, it's correct—but if callers are expected to reuse the same map multiple times, this could be inefficient.

Add a comment explaining why caching is disabled when canUseSharedBuffers is true, or verify this behavior is intentional:

// Don't use cache if we can create SharedArrayBuffer but none was provided
// This prevents sharing state buffers across different game instances
const shouldUseCache = useCache && !canUseSharedBuffers;

If callers should reuse the same SAB-backed map, consider documenting that they must pass the same sharedStateBuffer on subsequent calls.

---

59-67: LGTM! StateBuffer creation is well-structured.

The logic cleanly handles three cases: use provided buffer, create new SAB if possible, or use undefined. The size calculation correctly allocates 2 bytes per tile for Uint16 state.

---

71-75: LGTM! Proper stateBuffer forwarding.

The stateBuffer is correctly passed only to the main game map when mapSize === Normal. The 4x and mini maps don't receive shared state buffers, which is appropriate since they're likely used only for preview/display purposes.

---

95-109: LGTM! Cache consistency is now correct.

The result object properly includes sharedStateBuffer when a SharedArrayBuffer was used, and the cache write (lines 107-109) is now gated by shouldUseCache, matching the cache read logic. This addresses the previous issue where cache was written but not read consistently.

The typeof SharedArrayBuffer !== "undefined" check on line 100 is necessary for environments where SharedArrayBuffer might not be defined at all.

---

116-116: LGTM! Type choice is appropriately flexible.

Using ArrayBufferLike for the stateBuffer parameter allows both ArrayBuffer and SharedArrayBuffer, which gives flexibility while remaining type-safe. The optional parameter maintains backwards compatibility.

---

129-129: LGTM! Clean parameter forwarding.

The stateBuffer is correctly forwarded to the GameMapImpl constructor without unnecessary transformation.

src/client/graphics/layers/PerformanceOverlay.ts (4)

232-243: LGTM! Event forwarding is clean.

The expanded TickMetricsEvent parameters are correctly forwarded to updateTickMetrics.

---

440-468: LGTM! State declarations follow conventions.

The new metric fields are properly declared with @state() decorators and appropriate types.

---

470-481: LGTM! Signature is backward-compatible.

All new parameters are optional, maintaining compatibility while extending functionality.

---

584-591: LGTM! Snapshot includes all tile metrics.

The new tiles block properly captures all expanded tile and ring buffer metrics for JSON export.

[scamiv]

Atomic-SAB Architecture

Shared Tile State (sharedStateBuffer: SharedArrayBuffer → GameMapImpl.state: Uint16Array)
Authoritative per-tile game state (owner, fallout, etc.) - worker writes, main reads directly

Ring Header (SharedTileRingBuffers.header: SharedArrayBuffer → Int32Array[3])
Write index, read index, overflow flag - atomics coordinate producer/consumer access

Ring Data (SharedTileRingBuffers.data: SharedArrayBuffer → Uint32Array(capacity))
Queue of TileRefs needing redraw - worker enqueues, main drains as dirty hints for rendering

Dirty Flags (SharedTileRingBuffers.dirty: SharedArrayBuffer → Uint8Array(numTiles))
Per-tile dirty bitmap (0=clean, 1=dirty) - ensures each tile enqueued at most once while dirty; cleared when main processes tile

[scamiv]

@coderabbitai full review

[coderabbitai]

✅ Actions performed

Full review triggered.

[coderabbitai]

Actionable comments posted: 1

♻️ Duplicate comments (3)

src/client/graphics/layers/PerformanceOverlay.ts (2)

545-548: Overflow tracking acknowledged as temporary debug flag.

Per past review discussion, this is intentionally a flag (not a counter) for debugging purposes and will be removed later.

---

707-737: Missing translations for new metric labels.

Already flagged in previous review - these hardcoded English strings should use translateText() for consistency with other labels in the overlay.

src/client/ClientGameRunner.ts (1)

706-716: Overflow flag read but not reset - acknowledged as debug feature.

Per past review discussion, the overflow flag being read but not cleared is intentional for debugging purposes.

🧹 Nitpick comments (6)

src/client/graphics/GameRenderer.ts (1)

356-378: Backlog-aware frame throttling looks correct.

The logic scales target FPS linearly from 60 to 10 as backlog grows from 0 to 50 turns. When backlogTurns > 0, rendering is skipped if the frame interval hasn't elapsed. Once backlog clears, rendering resumes at full speed.

One small note: BACKLOG_MAX_TURNS = 50 is a magic number. Consider extracting this to a config constant if it needs tuning later.

src/core/worker/Worker.worker.ts (1)

90-99: Simplify: second branch is unreachable dead code.

Looking at lines 73-83: when sharedTileRing is created, dirtyFlags is always set from sharedTileRing.dirtyFlags. So if sharedTileRing is truthy, dirtyFlags is also truthy. The second branch (lines 96-97) can never execute.

          gameRunner = createGameRunner(
            message.gameStartInfo,
            message.clientID,
            mapLoader,
            gameUpdate,
-           sharedTileRing && dirtyFlags
-             ? (tile: TileRef) => {
-                 if (Atomics.compareExchange(dirtyFlags!, tile, 0, 1) === 0) {
-                   pushTileUpdate(sharedTileRing!, tile);
-                 }
-               }
-             : sharedTileRing
-               ? (tile: TileRef) => pushTileUpdate(sharedTileRing!, tile)
-               : undefined,
+           sharedTileRing
+             ? (tile: TileRef) => {
+                 if (Atomics.compareExchange(dirtyFlags!, tile, 0, 1) === 0) {
+                   pushTileUpdate(sharedTileRing!, tile);
+                 }
+               }
+             : undefined,
            message.sharedStateBuffer,
          )

src/client/graphics/layers/PerformanceOverlay.ts (1)

470-481: Consider using an options object for cleaner signature.

With 10 optional parameters, the function signature is getting long. An options object would make call sites more readable and self-documenting.

-  updateTickMetrics(
-    tickExecutionDuration?: number,
-    tickDelay?: number,
-    backlogTurns?: number,
-    ticksPerRender?: number,
-    workerTicksPerSecond?: number,
-    renderTicksPerSecond?: number,
-    tileUpdatesCount?: number,
-    ringBufferUtilization?: number,
-    ringBufferOverflows?: number,
-    ringDrainTime?: number,
-  ) {
+  updateTickMetrics(opts: {
+    tickExecutionDuration?: number;
+    tickDelay?: number;
+    backlogTurns?: number;
+    ticksPerRender?: number;
+    workerTicksPerSecond?: number;
+    renderTicksPerSecond?: number;
+    tileUpdatesCount?: number;
+    ringBufferUtilization?: number;
+    ringBufferOverflows?: number;
+    ringDrainTime?: number;
+  }) {
+    const {
+      tickExecutionDuration,
+      tickDelay,
+      backlogTurns,
+      // ... etc
+    } = opts;

src/core/worker/SharedTileRing.ts (1)

66-85: Drain implementation is correct, but consider pre-sizing output array.

The drain logic is sound. However, using out.push() in a tight loop can trigger array reallocations. For performance with large drains, consider pre-computing count and using array index assignment:

 export function drainTileUpdates(
   views: SharedTileRingViews,
   maxItems: number,
   out: TileRef[],
 ): void {
   const { header, buffer, capacity } = views;
 
   let read = Atomics.load(header, TILE_RING_HEADER_READ_INDEX);
   const write = Atomics.load(header, TILE_RING_HEADER_WRITE_INDEX);
 
-  let count = 0;
-
-  while (read !== write && count < maxItems) {
-    out.push(buffer[read]);
-    read = (read + 1) % capacity;
-    count++;
-  }
+  // Pre-compute available count
+  const available = (write - read + capacity) % capacity;
+  const toRead = Math.min(available, maxItems);
+  const startIndex = out.length;
+  out.length = startIndex + toRead;
+  
+  for (let i = 0; i < toRead; i++) {
+    out[startIndex + i] = buffer[read];
+    read = (read + 1) % capacity;
+  }
 
   Atomics.store(header, TILE_RING_HEADER_READ_INDEX, read);
 }

src/client/ClientGameRunner.ts (2)

571-625: Rendering only occurs when pending queue is fully drained.

The condition on line 572 (pendingStart >= pendingUpdates.length) means rendering happens only after ALL pending updates are processed. During heavy backlog, this delays visual feedback until catch-up is complete.

Consider rendering after each time-slice while still showing stale state, or rendering periodically even with pending updates:

-      // Only update view and render when ALL processing is complete
-      if (
-        this.pendingStart >= this.pendingUpdates.length &&
-        batch.length > 0 &&
-        lastTick !== undefined
-      ) {
+      // Render after each slice to maintain visual responsiveness
+      if (batch.length > 0 && lastTick !== undefined) {

This would provide smoother visual feedback during catch-up, showing intermediate game states rather than waiting for full sync.

---

669-677: Type assertion to any[] bypasses type safety.

The cast to unknown as any[] works but loses compile-time checks. Consider a typed helper:

function appendUpdates<T>(target: T[], source: T[]): void {
  target.push(...source);
}

Then use it with explicit type for each GameUpdateType. Low priority since this is internal.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7d8c1c2 and 1e3d2e1.

📒 Files selected for processing (12)

• src/client/ClientGameRunner.ts (11 hunks)
• src/client/InputHandler.ts (1 hunks)
• src/client/graphics/GameRenderer.ts (4 hunks)
• src/client/graphics/layers/PerformanceOverlay.ts (6 hunks)
• src/core/GameRunner.ts (5 hunks)
• src/core/game/GameMap.ts (2 hunks)
• src/core/game/GameView.ts (3 hunks)
• src/core/game/TerrainMapLoader.ts (4 hunks)
• src/core/worker/SharedTileRing.ts (1 hunks)
• src/core/worker/Worker.worker.ts (4 hunks)
• src/core/worker/WorkerClient.ts (3 hunks)
• src/core/worker/WorkerMessages.ts (1 hunks)

🧰 Additional context used

🧠 Learnings (16)

📚 Learning: 2025-11-26T20:49:29.110Z

Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 2519
File: src/core/game/GameView.ts:516-525
Timestamp: 2025-11-26T20:49:29.110Z
Learning: In GameView.ts, when usesSharedTileState is true (SAB mode), packedTileUpdates contains unpacked tile references as BigInt(tileRef) only, because all tile state lives in the shared Uint16Array. In non-SAB mode, packedTileUpdates contains packed TileUpdate bigints in the format (tileRef << 16n | state), which must be decoded via updateTile(tu). Therefore, Number(tu) is correct in SAB mode and shifting right by 16 bits would be wrong.

Applied to files:

• src/core/worker/WorkerClient.ts
• src/core/GameRunner.ts
• src/core/worker/SharedTileRing.ts
• src/core/game/GameMap.ts
• src/core/worker/Worker.worker.ts
• src/client/ClientGameRunner.ts
• src/core/game/GameView.ts
• src/core/game/TerrainMapLoader.ts

📚 Learning: 2025-10-20T20:15:28.858Z

Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:51-51
Timestamp: 2025-10-20T20:15:28.858Z
Learning: In src/core/execution/FakeHumanExecution.ts, game balance constants like MIRV_COOLDOWN_TICKS, MIRV_HESITATION_ODDS, VICTORY_DENIAL_TEAM_THRESHOLD, VICTORY_DENIAL_INDIVIDUAL_THRESHOLD, and STEAMROLL_CITY_GAP_MULTIPLIER are experimental tuning parameters subject to frequent change during balance testing. Do not flag changes to these values as issues or compare them against previous values.

Applied to files:

• src/client/InputHandler.ts
• src/core/GameRunner.ts
• src/client/graphics/layers/PerformanceOverlay.ts

📚 Learning: 2025-10-08T17:14:49.369Z

Learnt from: Foorack
Repo: openfrontio/OpenFrontIO PR: 2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.369Z
Learning: In `ClientGameRunner.ts`, the `myPlayer` field is always set when `shouldPreventWindowClose()` is called, so the null check in that method is sufficient without needing to fetch it again from `gameView.playerByClientID()`.

Applied to files:

• src/core/GameRunner.ts
• src/client/ClientGameRunner.ts

📚 Learning: 2025-06-02T14:27:37.609Z

Learnt from: andrewNiziolek
Repo: openfrontio/OpenFrontIO PR: 1007
File: resources/lang/de.json:115-115
Timestamp: 2025-06-02T14:27:37.609Z
Learning: For OpenFrontIO project: When localization keys are renamed in language JSON files, the maintainers separate technical changes from translation content updates. They wait for community translators to update the actual translation values rather than attempting to translate in the same PR. This allows technical changes to proceed while ensuring accurate translations from native speakers.

Applied to files:

• src/client/graphics/layers/PerformanceOverlay.ts

📚 Learning: 2025-08-16T10:52:08.292Z

Learnt from: TheGiraffe3
Repo: openfrontio/OpenFrontIO PR: 884
File: resources/lang/en.json:456-461
Timestamp: 2025-08-16T10:52:08.292Z
Learning: In OpenFrontIO, translation files in resources/lang/*.json (except en.json) should not be updated in regular PRs. Only dedicated translation PRs titled "mls" and made by Aotumori should update non-English locale files. Regular PRs should only update en.json when adding or modifying translation keys.

Applied to files:

• src/client/graphics/layers/PerformanceOverlay.ts

📚 Learning: 2025-10-20T11:02:16.969Z

Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:57-57
Timestamp: 2025-10-20T11:02:16.969Z
Learning: In src/core/execution/FakeHumanExecution.ts, the correct MIRV victory denial thresholds are VICTORY_DENIAL_TEAM_THRESHOLD = 0.8 (80% for team games) and VICTORY_DENIAL_INDIVIDUAL_THRESHOLD = 0.65 (65% for individual players), not 0.85 and 0.7 as might be mentioned in some documentation.

Applied to files:

• src/client/graphics/layers/PerformanceOverlay.ts

📚 Learning: 2025-10-27T09:47:26.395Z

Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:770-795
Timestamp: 2025-10-27T09:47:26.395Z
Learning: In src/core/execution/FakeHumanExecution.ts, the selectSteamrollStopTarget() method intentionally allows MIRV targeting when secondHighest city count is 0 (e.g., nuclear endgame scenarios where structures are destroyed). This is valid game design—"if you can afford it, you're good to go"—and should not be flagged as requiring a guard condition.

Applied to files:

• src/client/graphics/layers/PerformanceOverlay.ts

📚 Learning: 2025-10-18T17:54:01.311Z

Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:172-173
Timestamp: 2025-10-18T17:54:01.311Z
Learning: In src/core/execution/FakeHumanExecution.ts, MIRVs and ground attacks should not be mutually exclusive. The considerMIRV() method should not short-circuit maybeAttack() - both actions can occur in the same tick.

Applied to files:

• src/client/graphics/layers/PerformanceOverlay.ts

📚 Learning: 2025-06-22T05:48:19.241Z

Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 786
File: src/client/TerritoryPatternsModal.ts:337-338
Timestamp: 2025-06-22T05:48:19.241Z
Learning: In src/client/TerritoryPatternsModal.ts, the bit shifting operators (<<) used in coordinate calculations with decoder.getScale() are intentional and should not be changed to multiplication. The user scottanderson confirmed this is functioning as intended.

Applied to files:

• src/client/graphics/layers/PerformanceOverlay.ts
• src/core/game/GameView.ts

📚 Learning: 2025-08-12T00:31:50.144Z

Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 1752
File: src/core/game/Game.ts:750-752
Timestamp: 2025-08-12T00:31:50.144Z
Learning: In the OpenFrontIO codebase, changes to the PlayerInteraction interface (like adding canDonateGold and canDonateTroops flags) do not require corresponding updates to src/core/Schemas.ts or server serialization code.

Applied to files:

• src/client/graphics/layers/PerformanceOverlay.ts
• src/client/ClientGameRunner.ts

📚 Learning: 2025-10-08T17:14:49.369Z

Learnt from: Foorack
Repo: openfrontio/OpenFrontIO PR: 2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.369Z
Learning: For the window close confirmation feature in `ClientGameRunner.ts`, the troop count requirement (>10,000 troops) from issue #2137 was intentionally removed because it was arbitrary and troop count can be reported as low despite having significant land. The confirmation now triggers for any alive player regardless of troop count.

Applied to files:

• src/client/graphics/layers/PerformanceOverlay.ts

📚 Learning: 2025-10-18T11:00:57.142Z

Learnt from: NewYearNewPhil
Repo: openfrontio/OpenFrontIO PR: 2230
File: src/client/graphics/GameRenderer.ts:269-277
Timestamp: 2025-10-18T11:00:57.142Z
Learning: In src/client/graphics/GameRenderer.ts, the GameRecapCapture implementation does not use setCaptureRenderEnabled on layers. Instead, it uses RecapCaptureSurface.capture() to render capture layers (TerrainLayer, TerritoryLayer, RailroadLayer, StructureIconsLayer, UnitLayer) directly to an off-screen canvas without requiring layer-level capture mode methods.

Applied to files:

• src/client/graphics/GameRenderer.ts

📚 Learning: 2025-06-09T02:20:43.637Z

Learnt from: VariableVince
Repo: openfrontio/OpenFrontIO PR: 1110
File: src/client/Main.ts:293-295
Timestamp: 2025-06-09T02:20:43.637Z
Learning: In src/client/Main.ts, during game start in the handleJoinLobby callback, UI elements are hidden using direct DOM manipulation with classList.add("hidden") for consistency. This includes modals, buttons, and error divs. The codebase follows this pattern rather than using component APIs for hiding elements during game transitions.

Applied to files:

• src/client/graphics/GameRenderer.ts
• src/client/ClientGameRunner.ts

📚 Learning: 2025-08-29T16:16:11.309Z

Learnt from: BrewedCoffee
Repo: openfrontio/OpenFrontIO PR: 1957
File: src/core/execution/PlayerExecution.ts:40-52
Timestamp: 2025-08-29T16:16:11.309Z
Learning: In OpenFrontIO PlayerExecution.ts, when Defense Posts are captured due to tile ownership changes, the intended behavior is to first call u.decreaseLevel() to downgrade them, then still transfer them to the capturing player via captureUnit(). This is not a bug - Defense Posts should be both downgraded and captured, not destroyed outright.

Applied to files:

• src/core/game/GameView.ts

📚 Learning: 2025-10-26T15:37:07.732Z

Learnt from: GlacialDrift
Repo: openfrontio/OpenFrontIO PR: 2298
File: src/client/graphics/layers/TerritoryLayer.ts:200-210
Timestamp: 2025-10-26T15:37:07.732Z
Learning: In GameImpl.ts lines 124-139, team assignment logic varies by number of teams: when numPlayerTeams < 8, teams are assigned ColoredTeams values (Red, Blue, Yellow, Green, Purple, Orange, Teal); when numPlayerTeams >= 8, teams are assigned generic string identifiers like "Team 1", "Team 2", etc., which are not members of ColoredTeams.

Applied to files:

• src/core/game/GameView.ts

📚 Learning: 2025-10-21T20:06:04.823Z

Learnt from: Saphereye
Repo: openfrontio/OpenFrontIO PR: 2233
File: src/client/HostLobbyModal.ts:891-891
Timestamp: 2025-10-21T20:06:04.823Z
Learning: For the HumansVsNations game mode in `src/client/HostLobbyModal.ts` and related files, the implementation strategy is to generate all nations and adjust their strength for balancing, rather than limiting lobby size based on the number of available nations on the map.

Applied to files:

• src/core/game/TerrainMapLoader.ts

🧬 Code graph analysis (7)

src/core/worker/WorkerClient.ts (1)

src/core/worker/SharedTileRing.ts (1)

• SharedTileRingBuffers (3-7)

src/client/InputHandler.ts (1)

src/core/EventBus.ts (1)

• GameEvent (1-1)

src/core/GameRunner.ts (5)

src/core/game/GameView.ts (2)

• tile (106-108)
• config (668-670)

src/core/game/UnitImpl.ts (1)

• tile (176-178)

src/core/game/GameMap.ts (1)

• TileRef (3-3)

src/core/game/GameImpl.ts (1)

• config (336-338)

src/core/configuration/ConfigLoader.ts (1)

• getConfig (11-27)

src/core/worker/SharedTileRing.ts (1)

src/core/game/GameMap.ts (1)

• TileRef (3-3)

src/client/graphics/GameRenderer.ts (1)

src/client/InputHandler.ts (1)

• BacklogStatusEvent (148-153)

src/core/game/GameMap.ts (3)

src/core/game/GameView.ts (2)

• width (701-703)
• height (704-706)

src/core/game/GameImpl.ts (2)

• width (807-809)
• height (810-812)

src/client/graphics/TransformHandler.ts (1)

• width (48-50)

src/core/worker/Worker.worker.ts (4)

src/core/worker/SharedTileRing.ts (3)

• SharedTileRingViews (9-14)
• createSharedTileRingViews (31-43)
• pushTileUpdate (45-64)

src/core/worker/WorkerMessages.ts (2)

• MainThreadMessage (115-122)
• InitializedMessage (50-52)

src/core/GameRunner.ts (1)

• createGameRunner (35-95)

src/core/game/GameMap.ts (1)

• TileRef (3-3)

🔇 Additional comments (24)

src/core/game/GameView.ts (2)

475-487: LGTM! Clean SAB flag wiring.

The optional usesSharedTileState parameter with a false default keeps backward compatibility. The flag is stored and used to branch tile update handling correctly.

---

516-525: Correct branching for SAB vs non-SAB tile updates.

Based on learnings, in SAB mode packedTileUpdates contains unpacked tile refs (BigInt(tileRef)), so Number(tu) is correct. In non-SAB mode, it contains packed values requiring updateTile(tu) to decode. The logic is sound.

src/core/game/GameMap.ts (1)

78-103: Well-structured external buffer support with proper validation.

The constructor:

1. Validates stateBuffer length matches width * height
2. Throws a clear error with dimensions if mismatch
3. Falls back to internal allocation when no buffer provided

This enables shared memory without breaking existing code paths.

src/core/worker/WorkerClient.ts (2)

23-29: Clean constructor extension for shared buffers.

Optional parameters for sharedTileRingBuffers, sharedStateBuffer, and sharedDirtyBuffer keep the API backward compatible while enabling SAB mode when available.

---

72-81: Initialization payload correctly passes shared buffers.

The init message extracts header/data from sharedTileRingBuffers and includes state/dirty buffers. The optional chaining handles the non-SAB fallback case.

src/core/worker/WorkerMessages.ts (1)

34-42: Clean InitMessage extension for shared buffers.

Optional SharedArrayBuffer fields allow gradual adoption of SAB mode while keeping the message contract backward compatible.

src/client/graphics/GameRenderer.ts (1)

298-300: Good state additions for backlog-aware rendering.

These fields track backlog state from events and enable frame pacing decisions.

src/client/InputHandler.ts (2)

128-146: TickMetricsEvent extended with useful diagnostics.

The new optional fields for backlog, throughput, and ring buffer stats are well-documented with comments. Optional parameters keep backward compatibility.

---

148-153: Clean BacklogStatusEvent definition.

Simple event with backlogTurns count and backlogGrowing flag. Matches usage in GameRenderer.

src/core/worker/Worker.worker.ts (2)

25-27: Module-level state for turn processing and shared ring.

These globals track the processing state and shared buffer views. Clean initialization.

---

44-65: Verification confirms: executeNextTick() is synchronous.

The web search confirms that executeNextTick() is a synchronous, single-tick game-loop method that runs the deterministic per-tick simulation and returns updates—not a Promise. This means the while loop correctly processes pending turns without await statements, and the reentrancy protection with isProcessingTurns flag combined with the try-finally cleanup is appropriate.

src/core/GameRunner.ts (3)

40-48: Clean API extension for shared buffer support.

The optional parameters tileUpdateSink and sharedStateBuffer are well-typed and properly threaded through to dependent functions. Good use of optional parameters to maintain backward compatibility.

---

183-196: Tile update branching logic is correct.

The extraction of tileRef via Number(u.update >> 16n) correctly unpacks the upper bits from the packed format (tileRef << 16n | state). When using a shared sink, returning an empty BigUint64Array is the right approach since tile state lives in the shared buffer.

---

292-295: Simple and clear pending turns check.

Clean implementation. The method name clearly conveys its purpose.

src/client/graphics/layers/PerformanceOverlay.ts (2)

231-243: Clean event forwarding for new metrics.

All new metric fields are properly extracted from the event and passed to updateTickMetrics.

---

440-468: State properties well-structured.

All new metrics use the @state() decorator correctly, and defaults are sensible. Good organization.

src/core/game/TerrainMapLoader.ts (3)

42-67: Cache and shared buffer logic is well-structured.

The caching bypass when SAB is available and the nullish coalescing for stateBuffer are correctly implemented. Previous review concerns have been addressed.

---

69-76: Verify: Compact maps intentionally skip shared state buffer.

Only normal-sized maps receive the stateBuffer. Compact maps (4x variants) do not get shared state. Please confirm this is the intended behavior - if compact maps should also support SAB mode, the buffer would need to be sized appropriately and passed here as well.

---

95-109: Result construction handles buffer types safely.

Good defensive check with instanceof SharedArrayBuffer before assignment. The caching logic correctly applies only when SAB path is not in use.

src/core/worker/SharedTileRing.ts (2)

3-19: Clean interface definitions for shared ring buffer.

Good use of typed interfaces over class hierarchy. The separation of Buffers (raw SharedArrayBuffer) from Views (typed array views) is a clean pattern for cross-thread data sharing.

---

21-43: Factory functions are straightforward.

Clean separation between buffer allocation and view creation. This allows buffers to be transferred to workers while views are created on each side.

src/client/ClientGameRunner.ts (3)

173-203: Shared buffer initialization is well-structured.

Clean feature detection for SAB/Atomics/crossOriginIsolated. Ring capacity sizing to numTiles provides a reasonable upper bound for tile updates.

---

258-272: Instance fields for batched processing are well-organized.

Using pendingStart as a cursor index into pendingUpdates is a good pattern to avoid costly array shifts. The separation of worker/render tick counters enables accurate throughput metrics.

---

745-756: Backlog tracking is straightforward.

Clean calculation of backlog turns and emission of status events. The backlogGrowing flag provides useful trend information for downstream throttling.

[evanpelle]

I wonder if there's a way to abstract the data transfer behind an interface and then we can pop in SharedArrayBufferTransfer or PostMessageTransfer. this way we can avoid the if(sabEnabled) else ... occurrances

[evanpelle]

I think this should be an interface, it's too easy to mix up arguments with this many parameters.

[scamiv]

most of these can/should be removed after evaluation

[evanpelle]

can all the buffer logic be moved to another file? ClientGameRunner is getting pretty large.

[scamiv]

I do believe that, in the current logic, this belong here. I would consider Refactoring this outside of scope.

[evanpelle]

Is this data relevant to InputHandler?

[scamiv]

Semantically, no, but it does extend TickMetricsEvent which was here before.

Most of this is only for the perf overlay and can be removed after testing.

[evanpelle]

I don't think this belongs in InputHandler either

[evanpelle]

I think this logic should be refactored into a function so it can be reused.

[evanpelle]

could we group these into an interface/object?

[evanpelle]

Would it be possible to break this into multiple PRs? I can't really review a PR this large effectively

[evanpelle]

The territory tiles don't seem to render when i tried running this locally

[coderabbitai]

Actionable comments posted: 0

♻️ Duplicate comments (1)

src/client/ClientGameRunner.ts (1)

679-716: Tile ring metrics are correct, but overflow flag should be read‑and‑reset atomically (repeat from earlier review)

The SAB path correctly:

• Drains tile refs with drainTileUpdates.
• Deduplicates them per render slice.
• Clears dirtyFlags per tile.
• Emits BigInt(ref) in packedTileUpdates, which matches the SAB usesSharedTileState contract. Based on learnings, this aligns with the shared‑state design.

One detail from the previous review is still present: TILE_RING_HEADER_OVERFLOW is only read via Atomics.load and never cleared. Once an overflow occurs, the metric will stay non‑zero forever.

To make the overflow metric represent “new overflows since last frame”, switch to an atomic read‑and‑reset:

-      const overflow = Atomics.load(
-        this.tileRingViews.header,
-        TILE_RING_HEADER_OVERFLOW,
-      );
+      const overflow = Atomics.exchange(
+        this.tileRingViews.header,
+        TILE_RING_HEADER_OVERFLOW,
+        0,
+      );

This keeps the header slot at 0 until the next overflow increment, and avoids losing information in the presence of concurrent updates.

🧹 Nitpick comments (3)

src/client/ClientGameRunner.ts (3)

174-203: Shared buffer wiring and fallbacks look solid

The detection of crossOriginIsolated, SharedArrayBuffer, and Atomics, the conditional creation of ring buffers / dirty flags, and the way you thread sharedStateBuffer, sharedTileRingBuffers, and usesSharedTileState through WorkerClient, GameView, and ClientGameRunner all look consistent and safe when SAB is not available. Only a small optional clean‑up would be to simplify the crossOriginIsolated check to const isIsolated = (globalThis as any).crossOriginIsolated === true; if you like.

Also applies to: 205-211, 213-221, 230-240, 271-273, 274-288

---

258-267: Backlog‑aware batching is correct but main‑thread slice budget may be too large

The backlog tracking (serverTurnHighWater, lastProcessedTick, backlogTurns) and queued processing via pendingUpdates with a guard on isProcessingUpdates are sound and avoid re‑entrancy; updateBacklogMetrics gives clean monotonic backlog and a simple BacklogStatusEvent API.

Two refinements to consider:

1. MAX_SLICE_BUDGET_MS = 1000 lets a single frame do up to 1s of synchronous work, which can freeze input and rendering on slower machines. Capping closer to ~50–100ms (and/or tying max budget to device performance) would keep the UI more responsive while still catching up.

2. processFrame does not check this.isActive, so after stop() is called you can still chew through any remaining pendingUpdates, call gameView.update, renderer.tick, and emit metrics. Early‑exiting the frame when !this.isActive (and optionally clearing pendingUpdates in stop()) would make shutdown behavior clearer and avoid work after the runner is logically stopped.

Also applies to: 268-271, 345-364, 500-635, 748-759

---

408-412: Game update batching is mostly fine; consider more defensive merge logic and confirm ordering assumptions

The high‑water tracking on incoming turns (serverTurnHighWater = max(serverTurnHighWater, turnNumber)) combined with updateBacklogMetrics(gu.tick) gives a consistent backlog measure.

In mergeGameUpdates you currently:

• Initialize combinedUpdates from last.updates keys.
• Then for each gu in batch, iterate gu.updates keys and push into combinedUpdates[type].

This relies on last.updates having every GameUpdateType key (with at least an empty array). If that invariant ever changes (e.g. GameUpdates becomes sparse), updates of a type present only in earlier gus will hit combinedUpdates[type] === undefined and the push will throw.

To make this more robust without changing behavior, you could lazily initialize each bucket as you merge:

-    const last = batch[batch.length - 1];
-    const combinedUpdates: GameUpdates = {} as GameUpdates;
-
-    // Initialize combinedUpdates with empty arrays for each existing key
-    for (const key in last.updates) {
-      const type = Number(key) as GameUpdateType;
-      combinedUpdates[type] = [];
-    }
+    const last = batch[batch.length - 1];
+    const combinedUpdates: GameUpdates = {} as GameUpdates;

     const combinedPackedTileUpdates: bigint[] = [];

     for (const gu of batch) {
       for (const key in gu.updates) {
         const type = Number(key) as GameUpdateType;
-        // We don't care about the specific update subtype here; just treat it
-        // as an array we can concatenate.
-        const updatesForType = gu.updates[type] as unknown as any[];
-        (combinedUpdates[type] as unknown as any[]).push(...updatesForType);
+        // We don't care about the specific update subtype here; just treat it as an array we can concatenate.
+        const updatesForType = gu.updates[type] as unknown as any[];
+        const dest = (combinedUpdates[type] as unknown as any[]) ?? [];
+        dest.push(...updatesForType);
+        combinedUpdates[type] = dest as any;
       }
     }

Also, since you now apply a single merged GameUpdateViewData for the whole batch, GameView.update will see all update types for many ticks grouped by type instead of tick‑by‑tick. If any view logic depends on the per‑tick separation or on the relative ordering between update types, that could change behavior; if not, this is fine but worth double‑checking with that invariant in mind.

Also applies to: 465-468, 637-677

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1e3d2e1 and 04a0041.

📒 Files selected for processing (1)

• src/client/ClientGameRunner.ts (11 hunks)

🧰 Additional context used

🧠 Learnings (7)

📓 Common learnings

Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 2519
File: src/core/worker/SharedTileRing.ts:55-63
Timestamp: 2025-11-26T22:27:31.844Z
Learning: In SharedTileRing.ts, the ring buffer is sized to width * height (the map dimensions). Combined with the dirty flag deduplication mechanism (each tile can only be enqueued once until the consumer clears its dirty flag), the number of pending entries is naturally bounded by the map size and drained every render frame. Therefore, ring buffer overflow should be extremely rare or effectively impossible, and any theoretical race condition between producer and consumer modifying the read index would only manifest as redundant tile refs being rendered, not memory corruption, which is acceptable.
<!-- </add_learning>

📚 Learning: 2025-11-26T22:27:31.844Z

Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 2519
File: src/core/worker/SharedTileRing.ts:55-63
Timestamp: 2025-11-26T22:27:31.844Z
Learning: In SharedTileRing.ts, the ring buffer is sized to width * height (the map dimensions). Combined with the dirty flag deduplication mechanism (each tile can only be enqueued once until the consumer clears its dirty flag), the number of pending entries is naturally bounded by the map size and drained every render frame. Therefore, ring buffer overflow should be extremely rare or effectively impossible, and any theoretical race condition between producer and consumer modifying the read index would only manifest as redundant tile refs being rendered, not memory corruption, which is acceptable.
<!-- </add_learning>

Applied to files:

• src/client/ClientGameRunner.ts

📚 Learning: 2025-10-08T17:14:49.369Z

Learnt from: Foorack
Repo: openfrontio/OpenFrontIO PR: 2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.369Z
Learning: In `ClientGameRunner.ts`, the `myPlayer` field is always set when `shouldPreventWindowClose()` is called, so the null check in that method is sufficient without needing to fetch it again from `gameView.playerByClientID()`.

Applied to files:

• src/client/ClientGameRunner.ts

📚 Learning: 2025-08-12T00:31:50.144Z

Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 1752
File: src/core/game/Game.ts:750-752
Timestamp: 2025-08-12T00:31:50.144Z
Learning: In the OpenFrontIO codebase, changes to the PlayerInteraction interface (like adding canDonateGold and canDonateTroops flags) do not require corresponding updates to src/core/Schemas.ts or server serialization code.

Applied to files:

• src/client/ClientGameRunner.ts

📚 Learning: 2025-11-26T20:49:29.140Z

Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 2519
File: src/core/game/GameView.ts:516-525
Timestamp: 2025-11-26T20:49:29.140Z
Learning: In GameView.ts, when usesSharedTileState is true (SAB mode), packedTileUpdates contains unpacked tile references as BigInt(tileRef) only, because all tile state lives in the shared Uint16Array. In non-SAB mode, packedTileUpdates contains packed TileUpdate bigints in the format (tileRef << 16n | state), which must be decoded via updateTile(tu). Therefore, Number(tu) is correct in SAB mode and shifting right by 16 bits would be wrong.

Applied to files:

• src/client/ClientGameRunner.ts

📚 Learning: 2025-06-09T02:20:43.637Z

Learnt from: VariableVince
Repo: openfrontio/OpenFrontIO PR: 1110
File: src/client/Main.ts:293-295
Timestamp: 2025-06-09T02:20:43.637Z
Learning: In src/client/Main.ts, during game start in the handleJoinLobby callback, UI elements are hidden using direct DOM manipulation with classList.add("hidden") for consistency. This includes modals, buttons, and error divs. The codebase follows this pattern rather than using component APIs for hiding elements during game transitions.

Applied to files:

• src/client/ClientGameRunner.ts

📚 Learning: 2025-10-08T17:14:49.369Z

Learnt from: Foorack
Repo: openfrontio/OpenFrontIO PR: 2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.369Z
Learning: For the window close confirmation feature in `ClientGameRunner.ts`, the troop count requirement (>10,000 troops) from issue #2137 was intentionally removed because it was arbitrary and troop count can be reported as low despite having significant land. The confirmation now triggers for any alive player regardless of troop count.

Applied to files:

• src/client/ClientGameRunner.ts

🔇 Additional comments (1)

src/client/ClientGameRunner.ts (1)

724-735: Non‑SAB tile update merge path looks straightforward

The non‑SAB branch that concatenates packedTileUpdates from each gu and reports tileMetrics.count = totalTileUpdates is simple and consistent with the prior packed‑update behavior. No issues from a correctness or TypeScript‑style point of view.

[coderabbitai]

Actionable comments posted: 1

♻️ Duplicate comments (2)

src/client/graphics/layers/PerformanceOverlay.ts (1)

707-737: Localize new performance labels instead of hardcoding English.

These new lines still use raw strings ("Worker ticks/s:", "Render ticks/s:", "Ticks per render:", "Backlog turns:", "Tile updates/render:", "Ring buffer:", "Ring drain time:") while the rest of the overlay uses translateText(...). Please wire these through translateText with new keys in en.json so non‑English locales can translate them later.

src/client/ClientGameRunner.ts (1)

716-726: Overflow flag is never reset.

The overflow flag at line 716 is read but never cleared. Once an overflow occurs (even if rare), tileMetrics.overflow will permanently show 1, making the metric less useful for detecting new overflow events.

Use Atomics.exchange to atomically read and reset:

-const overflow = Atomics.load(
-  this.tileRingViews.header,
-  TILE_RING_HEADER_OVERFLOW,
-);
+const overflow = Atomics.exchange(
+  this.tileRingViews.header,
+  TILE_RING_HEADER_OVERFLOW,
+  0,
+);

Based on learnings, overflow should be extremely rare given the bounded ring size, but the flag should still be reset to accurately reflect new events.

🧹 Nitpick comments (5)

src/client/graphics/layers/PerformanceOverlay.ts (1)

326-333: Consider resetting backlog-related counters on reset as well.

handleReset() now clears tile/ring metrics, but leaves backlogTurns, ticksPerRender, and the tick rate fields unchanged until the next metrics event. If you want the reset button to give a “clean slate” snapshot, you could also zero these during reset; otherwise, current behavior is fine but a bit asymmetric.

src/core/worker/WorkerMessages.ts (1)

35-42: Tighten InitMessage typing for shared buffers.

Right now each shared buffer field is optional and independent, but the worker logic expects either all of sharedTileRingHeader, sharedTileRingData, and sharedDirtyBuffer or none. You could encode this more safely as a union:

type SharedRingInit =
  | {
      sharedTileRingHeader: SharedArrayBuffer;
      sharedTileRingData: SharedArrayBuffer;
      sharedDirtyBuffer: SharedArrayBuffer;
      sharedStateBuffer?: SharedArrayBuffer;
    }
  | {
      sharedTileRingHeader?: undefined;
      sharedTileRingData?: undefined;
      sharedDirtyBuffer?: undefined;
      sharedStateBuffer?: SharedArrayBuffer;
    };

export interface InitMessage extends BaseWorkerMessage, SharedRingInit {
  type: "init";
  gameStartInfo: GameStartInfo;
  clientID: ClientID;
}

This keeps the wire format the same but makes misuse harder.

src/core/worker/WorkerClient.ts (1)

23-29: Constructor SAB wiring is consistent, but sharedDirtyBuffer is redundant.

Passing sharedTileRingBuffers + sharedStateBuffer into the worker makes sense. However, you also take sharedDirtyBuffer separately while SharedTileRingBuffers already includes a dirty buffer. That gives two sources of truth for the same thing.

If possible, simplify to:

constructor(
  private gameStartInfo: GameStartInfo,
  private clientID: ClientID,
  private sharedTileRingBuffers?: SharedTileRingBuffers,
  private sharedStateBuffer?: SharedArrayBuffer,
) { /* ... */ }

and in initialize() send sharedDirtyBuffer: this.sharedTileRingBuffers?.dirty. This keeps the API smaller and removes a chance for mismatch.

src/client/graphics/GameRenderer.ts (1)

360-382: Backlog-based FPS throttling is reasonable; backlogGrowing is unused.

The dynamic FPS clamp (60 → 10 over 0–50 backlog turns) and minimum-frame-interval check look sound and still rely on requestAnimationFrame, so you avoid tight loops. Currently you ignore backlogGrowing; either using it for more aggressive throttling on growth, or dropping the field until needed, would keep things a bit cleaner.

src/core/GameRunner.ts (1)

183-196: Tile update sink path looks correct; consider centralizing the decode.

When tileUpdateSink is present you decode tileRef via Number(u.update >> 16n) and emit that to the sink, then send an empty BigUint64Array to the client. In the legacy path you still pack u.update values as before. This matches the toTileUpdate() format in GameMapImpl and the SAB-mode design (sink handles tile refs, shared buffer holds state).

To avoid magic constants and keep encode/decode in sync, you might want a small helper:

function decodeTileRef(update: bigint): TileRef {
  return Number(update >> 16n) as TileRef;
}

and reuse it both here and anywhere else you decode tile updates.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 04a0041 and 74dbec8.

📒 Files selected for processing (12)

• src/client/ClientGameRunner.ts (11 hunks)
• src/client/InputHandler.ts (1 hunks)
• src/client/graphics/GameRenderer.ts (4 hunks)
• src/client/graphics/layers/PerformanceOverlay.ts (6 hunks)
• src/core/GameRunner.ts (5 hunks)
• src/core/game/GameMap.ts (2 hunks)
• src/core/game/GameView.ts (3 hunks)
• src/core/game/TerrainMapLoader.ts (4 hunks)
• src/core/worker/SharedTileRing.ts (1 hunks)
• src/core/worker/Worker.worker.ts (4 hunks)
• src/core/worker/WorkerClient.ts (3 hunks)
• src/core/worker/WorkerMessages.ts (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (2)

• src/core/game/GameView.ts
• src/client/InputHandler.ts

🧰 Additional context used

🧠 Learnings (17)

📓 Common learnings

Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 2519
File: src/core/worker/SharedTileRing.ts:55-63
Timestamp: 2025-11-26T22:27:31.844Z
Learning: In SharedTileRing.ts, the ring buffer is sized to width * height (the map dimensions). Combined with the dirty flag deduplication mechanism (each tile can only be enqueued once until the consumer clears its dirty flag), the number of pending entries is naturally bounded by the map size and drained every render frame. Therefore, ring buffer overflow should be extremely rare or effectively impossible, and any theoretical race condition between producer and consumer modifying the read index would only manifest as redundant tile refs being rendered, not memory corruption, which is acceptable.
<!-- </add_learning>

📚 Learning: 2025-10-18T11:00:57.142Z

Learnt from: NewYearNewPhil
Repo: openfrontio/OpenFrontIO PR: 2230
File: src/client/graphics/GameRenderer.ts:269-277
Timestamp: 2025-10-18T11:00:57.142Z
Learning: In src/client/graphics/GameRenderer.ts, the GameRecapCapture implementation does not use setCaptureRenderEnabled on layers. Instead, it uses RecapCaptureSurface.capture() to render capture layers (TerrainLayer, TerritoryLayer, RailroadLayer, StructureIconsLayer, UnitLayer) directly to an off-screen canvas without requiring layer-level capture mode methods.

Applied to files:

• src/client/graphics/GameRenderer.ts

📚 Learning: 2025-06-09T02:20:43.637Z

Learnt from: VariableVince
Repo: openfrontio/OpenFrontIO PR: 1110
File: src/client/Main.ts:293-295
Timestamp: 2025-06-09T02:20:43.637Z
Learning: In src/client/Main.ts, during game start in the handleJoinLobby callback, UI elements are hidden using direct DOM manipulation with classList.add("hidden") for consistency. This includes modals, buttons, and error divs. The codebase follows this pattern rather than using component APIs for hiding elements during game transitions.

Applied to files:

• src/client/graphics/GameRenderer.ts
• src/client/ClientGameRunner.ts

📚 Learning: 2025-11-26T20:49:29.140Z

Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 2519
File: src/core/game/GameView.ts:516-525
Timestamp: 2025-11-26T20:49:29.140Z
Learning: In GameView.ts, when usesSharedTileState is true (SAB mode), packedTileUpdates contains unpacked tile references as BigInt(tileRef) only, because all tile state lives in the shared Uint16Array. In non-SAB mode, packedTileUpdates contains packed TileUpdate bigints in the format (tileRef << 16n | state), which must be decoded via updateTile(tu). Therefore, Number(tu) is correct in SAB mode and shifting right by 16 bits would be wrong.

Applied to files:

• src/core/game/GameMap.ts
• src/core/worker/WorkerClient.ts
• src/core/worker/SharedTileRing.ts
• src/core/game/TerrainMapLoader.ts
• src/core/worker/Worker.worker.ts
• src/core/GameRunner.ts
• src/client/ClientGameRunner.ts

📚 Learning: 2025-11-26T22:27:31.844Z

Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 2519
File: src/core/worker/SharedTileRing.ts:55-63
Timestamp: 2025-11-26T22:27:31.844Z
Learning: In SharedTileRing.ts, the ring buffer is sized to width * height (the map dimensions). Combined with the dirty flag deduplication mechanism (each tile can only be enqueued once until the consumer clears its dirty flag), the number of pending entries is naturally bounded by the map size and drained every render frame. Therefore, ring buffer overflow should be extremely rare or effectively impossible, and any theoretical race condition between producer and consumer modifying the read index would only manifest as redundant tile refs being rendered, not memory corruption, which is acceptable.
<!-- </add_learning>

Applied to files:

• src/core/game/GameMap.ts
• src/core/worker/WorkerClient.ts
• src/core/worker/SharedTileRing.ts
• src/core/game/TerrainMapLoader.ts
• src/client/graphics/layers/PerformanceOverlay.ts
• src/core/worker/Worker.worker.ts
• src/client/ClientGameRunner.ts

📚 Learning: 2025-06-22T05:48:19.241Z

Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 786
File: src/client/TerritoryPatternsModal.ts:337-338
Timestamp: 2025-06-22T05:48:19.241Z
Learning: In src/client/TerritoryPatternsModal.ts, the bit shifting operators (<<) used in coordinate calculations with decoder.getScale() are intentional and should not be changed to multiplication. The user scottanderson confirmed this is functioning as intended.

Applied to files:

• src/core/worker/SharedTileRing.ts
• src/client/graphics/layers/PerformanceOverlay.ts

📚 Learning: 2025-11-29T22:22:37.178Z

Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 2539
File: src/core/game/GameImpl.ts:520-542
Timestamp: 2025-11-29T22:22:37.178Z
Learning: In GameImpl.ts, neighborsWithDiag() and forEachNeighborWithDiag() intentionally duplicate coordinate iteration logic. They serve different purposes: forEachNeighborWithDiag() is a zero-allocation hot-path optimization while neighborsWithDiag() is a convenience method that returns an array. Refactoring one to call the other would add callback/closure allocations and indirection overhead, defeating the performance goals.

Applied to files:

• src/core/worker/SharedTileRing.ts
• src/core/game/TerrainMapLoader.ts

📚 Learning: 2025-10-21T20:06:04.823Z

Learnt from: Saphereye
Repo: openfrontio/OpenFrontIO PR: 2233
File: src/client/HostLobbyModal.ts:891-891
Timestamp: 2025-10-21T20:06:04.823Z
Learning: For the HumansVsNations game mode in `src/client/HostLobbyModal.ts` and related files, the implementation strategy is to generate all nations and adjust their strength for balancing, rather than limiting lobby size based on the number of available nations on the map.

Applied to files:

• src/core/game/TerrainMapLoader.ts

📚 Learning: 2025-06-02T14:27:37.609Z

Learnt from: andrewNiziolek
Repo: openfrontio/OpenFrontIO PR: 1007
File: resources/lang/de.json:115-115
Timestamp: 2025-06-02T14:27:37.609Z
Learning: For OpenFrontIO project: When localization keys are renamed in language JSON files, the maintainers separate technical changes from translation content updates. They wait for community translators to update the actual translation values rather than attempting to translate in the same PR. This allows technical changes to proceed while ensuring accurate translations from native speakers.

Applied to files:

• src/client/graphics/layers/PerformanceOverlay.ts

📚 Learning: 2025-08-16T10:52:08.292Z

Learnt from: TheGiraffe3
Repo: openfrontio/OpenFrontIO PR: 884
File: resources/lang/en.json:456-461
Timestamp: 2025-08-16T10:52:08.292Z
Learning: In OpenFrontIO, translation files in resources/lang/*.json (except en.json) should not be updated in regular PRs. Only dedicated translation PRs titled "mls" and made by Aotumori should update non-English locale files. Regular PRs should only update en.json when adding or modifying translation keys.

Applied to files:

• src/client/graphics/layers/PerformanceOverlay.ts

📚 Learning: 2025-10-20T20:15:28.858Z

Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:51-51
Timestamp: 2025-10-20T20:15:28.858Z
Learning: In src/core/execution/FakeHumanExecution.ts, game balance constants like MIRV_COOLDOWN_TICKS, MIRV_HESITATION_ODDS, VICTORY_DENIAL_TEAM_THRESHOLD, VICTORY_DENIAL_INDIVIDUAL_THRESHOLD, and STEAMROLL_CITY_GAP_MULTIPLIER are experimental tuning parameters subject to frequent change during balance testing. Do not flag changes to these values as issues or compare them against previous values.

Applied to files:

• src/client/graphics/layers/PerformanceOverlay.ts
• src/core/GameRunner.ts

📚 Learning: 2025-10-20T11:02:16.969Z

Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:57-57
Timestamp: 2025-10-20T11:02:16.969Z
Learning: In src/core/execution/FakeHumanExecution.ts, the correct MIRV victory denial thresholds are VICTORY_DENIAL_TEAM_THRESHOLD = 0.8 (80% for team games) and VICTORY_DENIAL_INDIVIDUAL_THRESHOLD = 0.65 (65% for individual players), not 0.85 and 0.7 as might be mentioned in some documentation.

Applied to files:

• src/client/graphics/layers/PerformanceOverlay.ts

📚 Learning: 2025-10-27T09:47:26.395Z

Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:770-795
Timestamp: 2025-10-27T09:47:26.395Z
Learning: In src/core/execution/FakeHumanExecution.ts, the selectSteamrollStopTarget() method intentionally allows MIRV targeting when secondHighest city count is 0 (e.g., nuclear endgame scenarios where structures are destroyed). This is valid game design—"if you can afford it, you're good to go"—and should not be flagged as requiring a guard condition.

Applied to files:

• src/client/graphics/layers/PerformanceOverlay.ts

📚 Learning: 2025-10-18T17:54:01.311Z

Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:172-173
Timestamp: 2025-10-18T17:54:01.311Z
Learning: In src/core/execution/FakeHumanExecution.ts, MIRVs and ground attacks should not be mutually exclusive. The considerMIRV() method should not short-circuit maybeAttack() - both actions can occur in the same tick.

Applied to files:

• src/client/graphics/layers/PerformanceOverlay.ts

📚 Learning: 2025-08-12T00:31:50.144Z

Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 1752
File: src/core/game/Game.ts:750-752
Timestamp: 2025-08-12T00:31:50.144Z
Learning: In the OpenFrontIO codebase, changes to the PlayerInteraction interface (like adding canDonateGold and canDonateTroops flags) do not require corresponding updates to src/core/Schemas.ts or server serialization code.

Applied to files:

• src/client/graphics/layers/PerformanceOverlay.ts
• src/client/ClientGameRunner.ts

📚 Learning: 2025-10-08T17:14:49.369Z

Learnt from: Foorack
Repo: openfrontio/OpenFrontIO PR: 2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.369Z
Learning: For the window close confirmation feature in `ClientGameRunner.ts`, the troop count requirement (>10,000 troops) from issue #2137 was intentionally removed because it was arbitrary and troop count can be reported as low despite having significant land. The confirmation now triggers for any alive player regardless of troop count.

Applied to files:

• src/client/graphics/layers/PerformanceOverlay.ts

📚 Learning: 2025-10-08T17:14:49.369Z

Learnt from: Foorack
Repo: openfrontio/OpenFrontIO PR: 2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.369Z
Learning: In `ClientGameRunner.ts`, the `myPlayer` field is always set when `shouldPreventWindowClose()` is called, so the null check in that method is sufficient without needing to fetch it again from `gameView.playerByClientID()`.

Applied to files:

• src/core/GameRunner.ts
• src/client/ClientGameRunner.ts

🧬 Code graph analysis (6)

src/client/graphics/GameRenderer.ts (1)

src/client/InputHandler.ts (1)

• BacklogStatusEvent (148-153)

src/core/worker/WorkerClient.ts (1)

src/core/worker/SharedTileRing.ts (1)

• SharedTileRingBuffers (3-7)

src/core/worker/SharedTileRing.ts (1)

src/core/game/GameMap.ts (1)

• TileRef (3-3)

src/core/game/TerrainMapLoader.ts (2)

src/core/game/GameImpl.ts (2)

• map (197-199)
• miniMap (200-202)

src/client/TerrainMapFileLoader.ts (1)

• terrainMapFileLoader (4-4)

src/core/worker/Worker.worker.ts (6)

src/core/worker/SharedTileRing.ts (3)

• SharedTileRingViews (9-14)
• createSharedTileRingViews (31-43)
• pushTileUpdate (45-64)

src/core/worker/WorkerMessages.ts (2)

• MainThreadMessage (115-122)
• InitializedMessage (50-52)

src/core/GameRunner.ts (1)

• createGameRunner (35-95)

src/core/game/GameView.ts (1)

• tile (106-108)

src/core/game/UnitImpl.ts (1)

• tile (176-178)

src/core/game/GameMap.ts (1)

• TileRef (3-3)

src/core/GameRunner.ts (5)

src/core/game/GameView.ts (2)

• tile (106-108)
• config (670-672)

src/core/game/UnitImpl.ts (1)

• tile (176-178)

src/core/game/GameMap.ts (1)

• TileRef (3-3)

src/core/game/GameImpl.ts (1)

• config (336-338)

src/core/configuration/ConfigLoader.ts (1)

• getConfig (11-27)

🔇 Additional comments (29)

src/client/graphics/layers/PerformanceOverlay.ts (3)

231-244: Metrics event plumbing looks consistent.

TickMetricsEvent fields are forwarded 1:1 into updateTickMetrics, and the method guards on visibility, so the new values should not cause surprises in hidden state. No functional issues seen here.

---

440-553: Tile/ring stats aggregation is straightforward and bounded.

The new fields (backlogTurns, ticksPerRender, worker/render tick rates, tile update counters, ring utilization/overflows, drain time) are updated only when values are provided, with simple rounding and a moving peak and total. This keeps state bounded (60-sample histories + scalar aggregates) and matches the intended SAB/tile-ring design.

---

584-591: Snapshot extension for tile metrics matches internal state.

The tiles block exposes exactly the tracked fields (updatesPerRender, peakUpdates, ringBufferUtilization, ringBufferOverflows, ringDrainTimeMs, totalTilesUpdated), so downstream tools will see a faithful view of what the overlay displays.

src/core/worker/WorkerClient.ts (1)

72-81: Init payload extension matches WorkerMessages.InitMessage.

initialize() now forwards sharedTileRingHeader, sharedTileRingData, sharedStateBuffer, and sharedDirtyBuffer to the worker, which lines up with the new InitMessage fields and lets the worker reconstruct ring views and shared state. No functional issues seen here.

src/core/game/GameMap.ts (1)

78-103: External state buffer support is correctly validated.

Accepting an optional stateBuffer and checking Uint16Array(stateBuffer).length === width * height before using it is a good safety check, and the fallback allocation preserves previous behavior. This is a clean way to plug in a shared SAB without risking out‑of‑bounds state access.

src/client/graphics/GameRenderer.ts (1)

296-321: Backlog status integration is straightforward.

Storing backlogTurns and backlogGrowing and wiring BacklogStatusEvent in initialize() is a clean way to feed renderer pacing from the client runner. No functional issues here.

src/core/worker/Worker.worker.ts (2)

73-83: Shared tile ring initialization matches the buffer contract.

Creating views only when both sharedTileRingHeader and sharedTileRingData are present, then wiring a tileUpdateSink that uses Atomics on dirtyFlags before calling pushTileUpdate, aligns with the SharedTileRing design (ring sized to map, dirty flags dedup per tile). This should safely bound tile refs in the ring and keep overflow behavior limited to dropped/duplicated updates, as intended.

---

85-99: Tile update sink injection into GameRunner is clean.

Passing either a deduping sink (when sharedTileRing and dirtyFlags exist), a non-deduping sink (if you ever chose to allow ring without dirty flags), or undefined falls back nicely to the old packed-update behavior. The extra sharedStateBuffer argument is correctly forwarded to the map loader through createGameRunner.

src/core/GameRunner.ts (3)

35-49: Shared state buffer is threaded cleanly into map loading.

createGameRunner now passes sharedStateBuffer into loadTerrainMap, which in turn feeds it into GameMapImpl. This is a straightforward way to reuse the same SAB between worker and main without changing game logic.

---

87-92: Constructor injection of tileUpdateSink keeps design composable.

Taking an optional tileUpdateSink in the GameRunner constructor lets the worker control where tile updates go (packed array vs shared ring) without changing core game logic. This is a nice example of composition over inheritance.

---

293-295: hasPendingTurns() is simple and works with the worker turn pump.

return this.currTurn < this.turns.length; is exactly what processPendingTurns() needs to know when to stop. Combined with proper error handling in executeNextTick, this gives the worker a safe, backlog-aware way to drain queued turns.

src/core/game/TerrainMapLoader.ts (4)

36-51: SAB-aware caching behavior now matches the intended split.

Using useCache + canUseSharedBuffers to derive shouldUseCache and only reading/writing loadedMaps in that case cleanly separates:

• non-SAB maps that can be safely cached, from
• SAB-backed maps that must be per-session.

This avoids mixing SAB and non-SAB instances in the same cache entry.

---

59-75: State buffer creation and threading into the main map is sound.

stateBuffer prefers an externally provided sharedStateBuffer and otherwise allocates a SAB only when the environment supports it. Passing this into genTerrainFromBin for the main map (but not the mini-map) keeps shared state limited to the hot path while leaving mini-map behavior unchanged.

---

95-105: TerrainMapData extension anticipates shared state/dirty buffers.

Including sharedStateBuffer (when it is actually a SharedArrayBuffer) and a placeholder sharedDirtyBuffer in the returned TerrainMapData gives the client side a single place to look up these buffers. This fits well with the rest of the SAB plumbing.

---

113-130: genTerrainFromBin cleanly forwards the optional stateBuffer.

Plumbing stateBuffer through to GameMapImpl without altering validation or other logic keeps this helper simple and focused. No issues spotted.

src/core/worker/SharedTileRing.ts (5)

1-19: LGTM! Clean interface design.

The interfaces and constants are well-defined. The split between buffers and views is clear and follows good patterns for SharedArrayBuffer usage.

---

21-29: LGTM! Buffer allocation is correct.

The size calculations are accurate, and allocating separate SharedArrayBuffers for header, data, and dirty flags is a good design.

---

31-43: LGTM! View creation is clean.

The typed array views are created correctly, and deriving capacity from buffer.length is the right approach.

---

45-64: LGTM! Producer logic is sound.

The push operation correctly uses Atomics for index coordination. The non-atomic write to buffer[write] at line 62 is safe for a single-producer pattern.

Note: pushTileUpdate doesn't check or set dirtyFlags — it's the caller's responsibility to check dirty flags before calling and set them when enqueueing. This design is fine but ensure the worker code implements this protocol correctly.

Based on learnings, the overflow handling is acceptable given the bounded ring size and deduplication mechanism.

---

66-85: LGTM! Consumer logic is correct.

The drain operation correctly uses Atomics for index coordination and respects the maxItems limit. The loop handles wraparound properly.

src/client/ClientGameRunner.ts (9)

15-15: LGTM! Imports are clean and necessary.

The new imports support SharedTileRing integration and backlog tracking. All are used appropriately throughout the file.

Also applies to: 28-35, 39-39

---

184-213: LGTM! SharedArrayBuffer setup is thorough.

The feature detection correctly checks for SharedArrayBuffer, Atomics, and crossOriginIsolated. The ring capacity is properly sized to the number of tiles, which aligns with the deduplication strategy.

The fallback when shared buffers aren't available ensures backward compatibility.

---

284-298: LGTM! Constructor parameters are clean.

The new parameters for tileRingViews and dirtyFlags are properly typed and stored as instance fields.

---

357-374: LGTM! Batched update processing is well-integrated.

The change from immediate processing to queuing updates and calling processPendingUpdates() enables the new backlog-aware batching mechanism.

---

418-422: LGTM! Backlog tracking is correct.

Tracking serverTurnHighWater using Math.max() ensures the client knows the highest turn number seen, which is essential for calculating how far behind the client simulation is.

Also applies to: 475-478

---

536-645: LGTM! Processing logic is well-structured.

The slice-based processing with dynamic budgets handles backlog effectively. The queue compaction (lines 571-579) is efficient, and deferring rendering until all processing completes ensures consistency.

The metrics calculation for worker/render throughput (lines 601-615) is correct.

---

647-756: LGTM! Merge logic is solid.

The batched merge correctly combines updates, drains and deduplicates tile refs, clears dirty flags atomically, and provides proper fallback for non-SAB mode.

The performance timing for ring draining (line 699-705) is a nice touch for observability.

---

758-769: LGTM! Backlog calculation is correct.

The backlog is calculated as serverTurnHighWater - lastProcessedTick, which accurately reflects how many turns the client is behind.

Note: This method emits BacklogStatusEvent for every update processed (called from line 554), which could result in many events per frame when catching up on a large backlog. If event frequency becomes an issue, consider emitting only when backlog changes by a threshold or at fixed intervals.

---

518-535: Verify the max slice budget doesn't cause UI jank through performance testing.

MAX_SLICE_BUDGET_MS = 1000 allows up to 1 second of synchronous processing in a single frame callback when the backlog exceeds 500 turns. During this period, the UI becomes unresponsive—no input handling, no animations, effectively creating a 1 FPS stutter.

While this may be intentional as a catch-up mechanism for severe backlog scenarios, confirm through load testing that the UX tradeoff is acceptable. If UI responsiveness suffers noticeably, consider reducing the max budget (e.g., 100-200ms) or implementing alternative recovery strategies.

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

Guard against infinite loop when a tick throws.

processPendingTurns() loops while gr.hasPendingTurns() and calls gr.executeNextTick(). In GameRunner.executeNextTick(), this.isExecuting is set to true at the start and only reset to false on the success path; in the catch block it early-returns without clearing the flag.

If a tick throws once:

• hasPendingTurns() stays true (currTurn is not advanced).
• executeNextTick() becomes a no-op (immediately returns because isExecuting is still true).
• while (gr.hasPendingTurns()) never terminates, so the worker spins forever and never reaches the finally that clears isProcessingTurns.

You can fix this by ensuring GameRunner.executeNextTick() always clears isExecuting, even on errors, e.g.:

   public executeNextTick() {
-    if (this.isExecuting) {
-      return;
-    }
+    if (this.isExecuting) {
+      return;
+    }
@@
-    this.isExecuting = true;
+    this.isExecuting = true;
@@
-    try {
+    try {
       const startTime = performance.now();
       updates = this.game.executeNextTick();
       const endTime = performance.now();
       tickExecutionDuration = endTime - startTime;
     } catch (error: unknown) {
@@
-      } else {
-        console.error("Game tick error:", error);
-      }
-      return;
-    }
+      } else {
+        console.error("Game tick error:", error);
+      }
+      return;
+    } finally {
+      this.isExecuting = false;
+    }
@@
-    this.callBack({
+    this.callBack({
       tick: this.game.ticks(),
       packedTileUpdates,
       updates: updates,
       playerNameViewData: this.playerViewData,
       tickExecutionDuration: tickExecutionDuration,
     });
-    this.isExecuting = false;

This keeps the existing behavior but guarantees that a thrown tick does not trap the worker in an endless while loop.

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents

In src/core/worker/Worker.worker.ts around lines 44-65 the while loop can spin
forever if GameRunner.executeNextTick throws because executeNextTick leaves
this.isExecuting true; fix by updating GameRunner.executeNextTick to always
clear this.isExecuting in a finally block (wrap the tick execution/try/catch in
try { ... } finally { this.isExecuting = false; }) and preserve existing error
handling or rethrow after cleanup so the worker cannot be trapped in the
infinite loop.