Webgl

[evanpelle]

If this PR fixes an issue, link it below. If not, delete these two lines.
Resolves #(issue number)

Description:

Describe the PR.

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

[socket-security]

Review the following changes in direct dependencies. Learn more about Socket for GitHub.

Diff	Package	Supply Chain Security	Vulnerability	Quality	Maintenance	License
	npm/​lil-gui@​0.21.0

View full report

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

Walkthrough

Adds WebGL GameView, many GL render passes, frame derive/cache/upload modules, debug GUI (lil-gui), keyboard/map interaction and controllers. Refactors GameRenderer to a Controller model and wires WebGL into ClientGameRunner. Removes legacy 2D canvas layers and related FX/UI implementations.

Changes

GL integration and pipeline

Layer / File(s)	Summary
Complete GL migration and client wiring src/client/render/gl/..., src/client/render/frame/..., src/client/controllers/*, src/client/graphics/GameRenderer.ts, src/client/ClientGameRunner.ts, resources/atlases/*, package.json	Adds WebGL GameView, Camera, MapInteraction, KeyboardPan, many GL passes (terrain/territory/units/structures/names/FX/lights/UI), dynamic buffers, frame derive/cache/upload helpers, debug GUI and wiring (lil-gui), WebGLFrameBuilder, Controller interface and controllers, and client runner integration; removes legacy 2D canvas Layer implementations and related FX/UI modules.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor Worker
  participant Client as ClientGameRunner
  participant Builder as WebGLFrameBuilder
  participant GL as GameView/GPURenderer
  Worker->>Client: GameUpdateViewData
  Client->>Builder: update(gameView)
  Builder->>GL: uploadFrameData(frame)
  GL->>GL: Render passes (terrain/territory/units/structures/FX/names/lights)

Loading

Estimated code review effort

🎯 5 (Critical) | ⏱️ ~120 minutes

Poem

Pixels wake, the canvas sleeps,
Rails glow, a shockwave sweeps.
Names bloom bright in MSDF light,
Warships glide in GPU night.
Controllers hum, the passes sing—
From 2D past to WebGL spring. 🌊✨

[coderabbitai]

Note

Due to the large number of review comments, Critical severity comments were prioritized as inline comments.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

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

224-230: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Don't confirm builds from throttled ghost state.

The new 50ms throttle can leave ghostUnit.buildableUnit describing the previous tile. If the player moves and confirms quickly, requestConfirmStructure() can go straight into createStructure() with stale canBuild / canUpgrade flags and send an intent for a tile that has not been revalidated yet. Please bypass the throttle for pending confirms, or invalidate readiness whenever the hovered tile changes. Based on learnings both mouse (MouseUpEvent) and keyboard (ConfirmGhostStructureEvent) confirmation paths go through requestConfirmStructure → createStructure.

Also applies to: 366-373, 375-412

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/client/graphics/layers/StructureIconsLayer.ts` around lines 224 - 230,
The 50ms throttle in renderGhost allows ghostUnit.buildableUnit to be stale and
can cause requestConfirmStructure → createStructure to act on outdated
canBuild/canUpgrade flags; update logic so that when the hovered tile or ghost
target changes (compare ghostUnit.tileRef or equivalent) you immediately
invalidate lastGhostQueryAt/readiness (or set buildableUnit to undefined) so the
next confirm triggers an immediate revalidation, and additionally ensure
requestConfirmStructure will bypass the throttle and force a fresh validation
when called from confirmation paths (MouseUpEvent or ConfirmGhostStructureEvent)
by invoking the same validation routine directly; change targets: renderGhost,
requestConfirmStructure, createStructure, and the ghostUnit.buildableUnit
handling to implement this bypass/invalidating behavior.

🟠 Major comments (21)

src/client/render/gl/keyboard-pan.ts-110-116 (1)

110-116: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Prevent divide-by-zero/near-zero pan speed spikes.

Pan speed is divided by zoom directly. A zero or tiny zoom value can produce extreme movement and unstable camera behavior.

💡 Suggested fix

-    const speed = this.panSpeed / this.deps.zoom;
+    const safeZoom = Math.max(this.deps.zoom, 1e-3);
+    const speed = this.panSpeed / safeZoom;

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/client/render/gl/keyboard-pan.ts` around lines 110 - 116, The pan
computation divides by this.deps.zoom which can be zero or extremely small;
clamp the divisor to a safe minimum before computing speed to avoid spikes (e.g.
use const zoom = Math.max(this.deps.zoom, MIN_ZOOM) where MIN_ZOOM is a small
positive constant like 1e-3) and then compute const speed = this.panSpeed /
zoom; update dx/dy logic that uses this.panSpeed, held, and dt accordingly so
camera movement remains stable when zoom is near zero.

src/client/render/gl/passes/point-light-pass.ts-178-184 (1)

178-184: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Validate palette indexing before reading owner colors.

ownerID is used directly to index paletteData. If it is invalid, this writes undefined/NaN into instance data and can corrupt light rendering.

💡 Suggested fix

       const off = count * FLOATS_PER_LIGHT;
       const pOff = unit.ownerID * 4;
+      if (pOff < 0 || pOff + 2 >= this.paletteData.length) continue;
       this.lightData[off + 0] = x;
       this.lightData[off + 1] = y;
       this.lightData[off + 2] = typeIdx;

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/client/render/gl/passes/point-light-pass.ts` around lines 178 - 184, The
code writes owner color bytes into this.lightData using unit.ownerID without
bounds checks (see unit.ownerID, pOff, this.paletteData and this.lightData in
point-light-pass.ts), which can produce undefined/NaN; fix by validating ownerID
before indexing: ensure ownerID is a finite non-negative integer and that pOff =
ownerID * 4 is within this.paletteData.length - 3; if invalid, use a safe
default color (e.g., zeros or a predefined fallback) and then write those
fallback values into this.lightData[off + 3..5]; keep the existing writes to
this.lightData[off + 0..2] intact.

src/client/render/gl/passes/point-light-pass.ts-104-109 (1)

104-109: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Guard unit type count against shader uniform capacity.

header.unitTypes is unbounded, but uniforms/arrays are hard-capped at MAX_LIGHT_TYPES. If this grows beyond 64, typeIdx can exceed shader array bounds and produce undefined rendering.

💡 Suggested fix

     this.typeNames = header.unitTypes;
+    if (this.typeNames.length > MAX_LIGHT_TYPES) {
+      throw new Error(
+        `PointLightPass supports up to ${MAX_LIGHT_TYPES} light types, got ${this.typeNames.length}`,
+      );
+    }
     this.typeConfigs = new Array(header.unitTypes.length);

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/client/render/gl/passes/point-light-pass.ts` around lines 104 - 109,
Guard the number of unit types loaded from header.unitTypes against the shader
uniform capacity MAX_LIGHT_TYPES to avoid out-of-bounds type indices: in the
initialization that sets this.typeNames, this.typeConfigs and this.typeToIdx
(the loop that reads header.unitTypes and uses LIGHT_CONFIGS), limit or truncate
header.unitTypes to at most MAX_LIGHT_TYPES entries (or throw a clear error)
before populating this.typeConfigs and this.typeToIdx so that any computed
typeIdx never exceeds MAX_LIGHT_TYPES-1; ensure any downstream uses of
this.typeNames/typeToIdx assume the truncated length.

src/client/render/gl/dynamic-buffer.ts-16-20 (1)

16-20: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Guard against zero initial capacity.

With initialCapacity = 0, Line 28 never terminates. Validate constructor input or clamp to at least 1.

Suggested fix

   ) {
-    this.capacity = initialCapacity;
+    this.capacity = Math.max(1, initialCapacity);

Also applies to: 26-29

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/client/render/gl/dynamic-buffer.ts` around lines 16 - 20, The constructor
allows initialCapacity = 0 which causes downstream loops to never terminate;
clamp or validate the value in the DynamicBuffer constructor by ensuring
capacity = Math.max(1, initialCapacity) (or throw on invalid input) and allocate
data with that clamped capacity * floatsPerInstance so this.capacity and
this.data are never zero-sized; update any references to capacity/data
initialization in the constructor to use the clamped value.

src/client/render/gl/passes/name-pass/text-program.ts-125-140 (1)

125-140: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Prevent async atlas creation after dispose.

Line 128 async onload can run after dispose() and create a texture that is never deleted. Add a disposed guard and clear handlers/source in dispose().

Suggested fix

 export class TextProgram {
+  private disposed = false;
@@
   private loadAtlas(): void {
     const gl = this.gl;
     const img = new Image();
     img.onload = () => {
+      if (this.disposed) return;
       const tex = gl.createTexture()!;
@@
       this.atlasTex = tex;
       this.atlasReady = true;
     };
     img.src = atlasUrl;
   }
@@
   dispose(): void {
+    this.disposed = true;
     const gl = this.gl;
     gl.deleteProgram(this.program);
     if (this.atlasTex) gl.deleteTexture(this.atlasTex);
   }
 }

Also applies to: 192-196

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/client/render/gl/passes/name-pass/text-program.ts` around lines 125 -
140, The img.onload in loadAtlas can fire after dispose and create a texture
that's never deleted; add a disposed guard (e.g., this._disposed boolean)
checked at the top of the onload handler and only create/bind the texture if not
disposed, set this.atlasTex and this.atlasReady there; in dispose(), set
this._disposed = true, if this.atlasTex exists delete the GL texture, and clear
the Image handlers and src (img.onload = null; img.src = '') or store the Image
reference on the instance so dispose can null its onload and src; apply the same
pattern for the other atlas/image creation block (the block referenced at lines
~192-196).

src/client/render/gl/passes/fx-pass/fx-attack-ring-pass.ts-80-86 (1)

80-86: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Delete the quad vertex buffer in dispose().

Line 80 creates a WebGL buffer that is never released. Keep it as a class field and delete it in lifecycle cleanup.

Suggested fix

 export class FxAttackRingPass {
@@
   private vao: WebGLVertexArrayObject;
+  private quadBuf: WebGLBuffer;
@@
-    const quadBuf = gl.createBuffer()!;
-    gl.bindBuffer(gl.ARRAY_BUFFER, quadBuf);
+    this.quadBuf = gl.createBuffer()!;
+    gl.bindBuffer(gl.ARRAY_BUFFER, this.quadBuf);
@@
   dispose(): void {
     const gl = this.gl;
     gl.deleteProgram(this.program);
     this.instanceBuf.dispose();
+    gl.deleteBuffer(this.quadBuf);
     gl.deleteVertexArray(this.vao);
   }
 }

Also applies to: 206-211

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/client/render/gl/passes/fx-pass/fx-attack-ring-pass.ts` around lines 80 -
86, The quad vertex buffer (quadBuf) created in the fx-attack-ring-pass should
be stored as a class field (e.g., this.quadBuf) instead of a local variable and
released in the class lifecycle cleanup: update the code that creates quadBuf to
assign it to the field and add gl.deleteBuffer(this.quadBuf) (and null the
field) inside the dispose() method; apply the same treatment to the other
buffer(s) created around the later block (lines ~206-211) so every WebGL buffer
allocated by this class (referenced by their variable names) is deleted in
dispose().

src/client/render/gl/passes/nuke-telegraph-pass.ts-61-67 (1)

61-67: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Release the static quad buffer in dispose().

The quad VBO created for attribute 0 is never deleted, so this pass leaks GPU memory across create/dispose cycles.

Suggested fix

 export class NukeTelegraphPass {
@@
   private vao: WebGLVertexArrayObject;
+  private quadBuf: WebGLBuffer;
   private instanceBuf: DynamicInstanceBuffer;
@@
-    const quadBuf = gl.createBuffer()!;
-    gl.bindBuffer(gl.ARRAY_BUFFER, quadBuf);
+    this.quadBuf = gl.createBuffer()!;
+    gl.bindBuffer(gl.ARRAY_BUFFER, this.quadBuf);
@@
   dispose(): void {
     const gl = this.gl;
     gl.deleteProgram(this.program);
     this.instanceBuf.dispose();
+    gl.deleteBuffer(this.quadBuf);
     gl.deleteVertexArray(this.vao);
   }
 }

Also applies to: 146-151

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/client/render/gl/passes/nuke-telegraph-pass.ts` around lines 61 - 67, The
pass creates a GPU buffer quadBuf (via gl.createBuffer and used for attribute 0)
but never deletes it, causing a GPU memory leak across create/dispose cycles;
update the pass's dispose() method to track and call gl.deleteBuffer(quadBuf)
(and any other static quad buffers created around lines 146-151) and null out
the reference so subsequent disposes are no-ops; ensure you reference the same
quadBuf variable used when binding (and any additional quad buffer variables) so
the GL resource is properly released.

src/client/render/gl/passes/trail-pass.ts-86-95 (1)

86-95: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Bind texture unit 2 deterministically when drawing.

uAffiliation always points to unit 2, but unit 2 is only bound conditionally. If affiliationTex is absent, this pass can sample stale global GL state.

Suggested fix

-    gl.uniform1i(this.uAltView, this.altView ? 1 : 0);
+    const hasAffiliation = !!this.affiliationTex;
+    gl.uniform1i(this.uAltView, this.altView && hasAffiliation ? 1 : 0);
@@
-    if (this.affiliationTex) {
-      gl.activeTexture(gl.TEXTURE2);
-      gl.bindTexture(gl.TEXTURE_2D, this.affiliationTex);
-    }
+    gl.activeTexture(gl.TEXTURE2);
+    gl.bindTexture(gl.TEXTURE_2D, this.affiliationTex);

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/client/render/gl/passes/trail-pass.ts` around lines 86 - 95, The shader
uniform uAffiliation is always sampled from texture unit 2 but the code only
binds TEXTURE2 when this.affiliationTex exists, leaving unit 2 in stale state;
ensure TEXTURE2 is deterministically bound every frame by binding either
this.affiliationTex when present or a known fallback (e.g. a pre-created 1x1
transparent/default WebGLTexture stored on the pass, or explicitly binding
null/empty texture) before drawing; update the block around
gl.activeTexture(gl.TEXTURE2) / gl.bindTexture(...) to always activate TEXTURE2
and bind the appropriate texture so uAffiliation never reads undefined state.

src/client/render/gl/passes/territory-pass.ts-117-129 (1)

117-129: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Reset dirty-row sentinels when forcing full uploads.

These methods mark textures dirty but keep previous partial-row bounds. If a partial range was set earlier, the next flush can upload only that stale range instead of the new full state.

Suggested fix

   uploadFullTileState(tileState: Uint16Array): void {
     this.liveTileRef = null;
     this.cpuTileState.set(tileState);
+    this.dirtyRowMin = Infinity;
+    this.dirtyRowMax = -1;
     this.tilesDirty = true;
   }

   setLiveRefs(tileState: Uint16Array, trailState: Uint8Array): void {
     this.liveTileRef = tileState;
     this.liveTrailRef = trailState;
+    this.dirtyRowMin = Infinity;
+    this.dirtyRowMax = -1;
+    this.trailDirtyRowMin = Infinity;
+    this.trailDirtyRowMax = -1;
     this.tilesDirty = true;
     this.trailsDirty = true;
   }
@@
   uploadFullTrailState(trailState: Uint8Array): void {
     this.liveTrailRef = null;
     this.cpuTrailState.set(trailState);
+    this.trailDirtyRowMin = Infinity;
+    this.trailDirtyRowMax = -1;
     this.trailsDirty = true;
   }
@@
   clearTrails(): void {
     this.cpuTrailState.fill(0);
+    this.trailDirtyRowMin = Infinity;
+    this.trailDirtyRowMax = -1;
     this.trailsDirty = true;
   }

Also applies to: 173-189

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/client/render/gl/passes/territory-pass.ts` around lines 117 - 129, When
forcing full uploads in uploadFullTileState and setLiveRefs, also reset the
partial-row sentinels so the next flush does a full upload instead of a stale
subrange: after setting tilesDirty = true (and trailsDirty = true for trail
refs), set the per-texture dirty-range fields (e.g.
tilesDirtyRowStart/tilesDirtyRowEnd and trailsDirtyRowStart/trailsDirtyRowEnd)
to cover the whole texture (start=0, end=this.tileRows or this.trailRows / the
appropriate row count) so any prior partial bounds are cleared; make the same
change for the analogous full-upload methods around lines 173-189.

src/client/render/gl/passes/range-circle-pass.ts-37-40 (1)

37-40: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Store the quad buffer as an instance property and release it in dispose().

The buffer allocated at line 39 is created as a local variable and never released. The dispose() method only deletes the program and VAO, allowing GPU memory to leak when the renderer is recreated multiple times.

Suggested fix

 export class RangeCirclePass {
   private gl: WebGL2RenderingContext;
   private program: WebGLProgram;
   private vao: WebGLVertexArrayObject;
+  private quadBuf: WebGLBuffer;
@@
-    const quadBuf = gl.createBuffer()!;
-    gl.bindBuffer(gl.ARRAY_BUFFER, quadBuf);
+    this.quadBuf = gl.createBuffer()!;
+    gl.bindBuffer(gl.ARRAY_BUFFER, this.quadBuf);
@@
   dispose(): void {
     const gl = this.gl;
     gl.deleteProgram(this.program);
     gl.deleteVertexArray(this.vao);
+    gl.deleteBuffer(this.quadBuf);
   }
 }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/client/render/gl/passes/range-circle-pass.ts` around lines 37 - 40, The
quad vertex buffer created as a local const (quadBuf) should be stored as an
instance property (e.g., this.quadBuf) so it can be deleted later; change the
creation to assign to the property where this.vao is set, update subsequent uses
to bind this.quadBuf instead of quadBuf, and add gl.deleteBuffer(this.quadBuf)
in the class dispose() method (alongside deletion of this.vao and the program)
to free GPU memory; reference the constructor/initialization where this.vao is
created and the dispose() method to make these edits.

src/client/render/gl/passes/unit-pass.ts-270-271 (1)

270-271: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Guard atlas loading against unhandled errors and dispose races.

loadAtlas() is fire-and-forget without error handling. If img.decode() fails, it creates an unhandled promise rejection. If dispose() runs before the image loads, GL writes happen on deleted textures.

Add a disposed flag, wrap GL operations in try-catch, and check the flag after await img.decode():

Suggested fix

 export class UnitPass {
   private atlasTex: WebGLTexture;
+  private disposed = false;
   
   constructor(...) {
     ...
-    this.loadAtlas();
+    void this.loadAtlas();
   }
   
   private async loadAtlas(): Promise<void> {
-    const img = new Image();
-    img.src = unitAtlasUrl;
-    await img.decode();
-    const gl = this.gl;
-    gl.bindTexture(gl.TEXTURE_2D, this.atlasTex);
-    gl.texImage2D(gl.TEXTURE_2D, 0, gl.RGBA, gl.RGBA, gl.UNSIGNED_BYTE, img);
-    gl.texParameteri(gl.TEXTURE_2D, gl.TEXTURE_MIN_FILTER, gl.NEAREST);
-    gl.texParameteri(gl.TEXTURE_2D, gl.TEXTURE_MAG_FILTER, gl.NEAREST);
+    try {
+      const img = new Image();
+      img.src = unitAtlasUrl;
+      await img.decode();
+      if (this.disposed) return;
+      const gl = this.gl;
+      gl.bindTexture(gl.TEXTURE_2D, this.atlasTex);
+      gl.texImage2D(gl.TEXTURE_2D, 0, gl.RGBA, gl.RGBA, gl.UNSIGNED_BYTE, img);
+      gl.texParameteri(gl.TEXTURE_2D, gl.TEXTURE_MIN_FILTER, gl.NEAREST);
+      gl.texParameteri(gl.TEXTURE_2D, gl.TEXTURE_MAG_FILTER, gl.NEAREST);
+    } catch {
+      // Placeholder atlas stays active on load failure.
+    }
   }
   
   dispose(): void {
+    this.disposed = true;
     const gl = this.gl;
     gl.deleteProgram(this.program);
     ...
   }
 }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/client/render/gl/passes/unit-pass.ts` around lines 270 - 271, The call to
loadAtlas() is fire-and-forget and can cause unhandled promise rejections and GL
writes after dispose; modify the loadAtlas function to introduce a disposed flag
(tied to the existing dispose() logic), wrap GL texture creation/update steps in
try/catch, await img.decode() then immediately check the disposed flag before
performing any GL calls, and ensure any caught errors are handled/logged to
prevent unhandled rejections; reference the loadAtlas function, img.decode()
call, GL texture upload logic, and the dispose method when making these changes.

src/client/render/frame/railroad-cache.ts-124-124 (1)

124-124: ⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Shared-tile removals can leave the wrong rail type encoded.

tileRefCount only tells you that some railroad still owns the tile; it does not tell you which RailType should survive there. If two railroads share a junction tile and one is removed, removeRailroad() keeps the previous railroadState whenever the count stays above zero, so the GPU can keep drawing the deleted railroad's orientation.

Also applies to: 223-239

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/client/render/frame/railroad-cache.ts` at line 124, tileRefCount
currently tracks only total references per tile (Map<number, number>), which
lets removals leave the wrong RailType encoded; update the ownership model to
track references per railroad/rail type so you can recompute the surviving
RailType on removal. Replace tileRefCount with a per-tile map of owners (e.g.,
Map<number, Map<railroadId, RailType>> or Map<number, Map<RailType, count>>) and
in removeRailroad() decrement only that railroad's entry, then recompute the
tile's railroadState from the remaining entries (choose the remaining railroad's
RailType or the highest-count RailType) and update the GPU state accordingly so
shared junctions adopt the correct orientation; apply the same fix pattern to
the logic referenced around the 223-239 removal code paths.

src/client/render/gl/passes/conquest-popup-pass.ts-56-60 (1)

56-60: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win

Localize the popup suffixes and sign formatting.

formatGold() and the popup builders hardcode "K", "M", "+", and "-" into user-visible strings, so these popups bypass the repo's translation flow. Please move the visible pieces through translateText()/en.json, or through a localized formatter wired into that flow.

As per coding guidelines "src/client/**/*.{ts,tsx}: All user-visible text must go through translateText() function with corresponding entries in resources/lang/en.json"

Also applies to: 231-264

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/client/render/gl/passes/conquest-popup-pass.ts` around lines 56 - 60,
formatGold() and the popup builder code currently hardcode user-visible suffixes
("K","M") and sign characters ("+","-"); update formatGold and the popup
creation logic in conquest-popup-pass.ts to use translateText() for all visible
strings (e.g., "K", "M", "+", "-") and wire any numeric-formatting through a
localized formatter if available, then add matching keys in
resources/lang/en.json; ensure you replace direct string concatenation in
formatGold and the popup builders with translateText(...) calls (or the
formatter) so all user-facing text follows the repo's translation flow.

src/client/WebGLFrameBuilder.ts-53-67 (1)

53-67: ⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

This path rebuilds and uploads the whole tile state on every update.

update() ignores the delta information in gu, walks every tile in fillTileState(), and then calls uploadTileAndTrailState() unconditionally. That turns sparse live updates into O(map size) CPU work plus a full GPU upload every tick, which is a big regression risk for larger matches.

Also applies to: 181-193

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/client/WebGLFrameBuilder.ts` around lines 53 - 67, The update() path
currently rebuilds and uploads the entire map every tick (fillTileState() +
view.uploadTileAndTrailState()), ignoring the delta info in gu; change update()
and the analogous block at the other mentioned range to apply only the sparse
deltas from gu: read gu's changed tile/unit/structure lists and update
this.tileState and this.unitMap only for those coordinates instead of walking
every tile in fillTileState(); add a minimal api on view (or reuse
uploadTileAndTrailState) to accept incremental updates or per-row dirty ranges
and call that with only the changed tiles/trail rows; keep
trailManager.update/clearDirtyRows compatible by marking only affected trail
rows from gu so GPU uploads and CPU work are proportional to the delta, not the
whole map.

src/client/render/gl/passes/name-pass/index.ts-109-109 (1)

109-109: ⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Don't size the name textures from the initial player list when late joins are supported.

this.maxPlayers = header.maxPlayers ?? header.players.length under-allocates every CPU/GPU row buffer when players arrive after construction. addPlayers() can then assign slots past the allocated capacity, and the next string/player-data write will overflow the fixed row budget for this pass.

Also applies to: 181-192

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/client/render/gl/passes/name-pass/index.ts` at line 109, The code sets
this.maxPlayers from header.players.length which under-allocates buffers when
late joins occur; change the initialization to prefer header.maxPlayers
(this.maxPlayers = header.maxPlayers ???) and never use header.players.length as
the capacity; instead use a declared safe upper bound (or require
header.maxPlayers) for initial CPU/GPU row buffer allocation, and update
addPlayers() to either grow this.maxPlayers and reallocate the row buffers
(CPU/GPU) before assigning new slots or to reject/handle overflow safely —
modify the constructor/site that assigns this.maxPlayers and the buffer
allocation logic (and the addPlayers() implementation referenced later) to
perform capacity checks and reallocation to prevent writes past the allocated
row budget.

src/client/render/gl/passes/name-pass/text-layout.ts-30-31 (1)

30-31: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Handle empty strings before visual-bounds math.

When text is empty, len is 0, and lastCode becomes invalid, producing NaN width/layout.

Patch

  const len = Math.min(text.length, MAX_CHARS);
+  if (len === 0) {
+    return 0;
+  }

Also applies to: 50-56

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/client/render/gl/passes/name-pass/text-layout.ts` around lines 30 - 31,
The code computes visual-bounds math using len and lastCode which breaks when
text is empty; add a guard after computing const len = Math.min(text.length,
MAX_CHARS) to handle len === 0 (either return an empty layout immediately or
skip the lastCode/visual-bounds calculations), and ensure any variables like
lastCode and subsequent width/layout math are not executed for len === 0; update
the same check around the other block referenced (lines ~50-56) so both places
short-circuit for empty text.

src/client/ClientGameRunner.ts-247-250 (1)

247-250: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Add teardown for debug DOM/event resources.

The debug canvas and global keydown listener are attached but no cleanup handle is returned. Re-entering games can stack listeners/canvases.

Suggested direction

-function mountWebGLDebugRenderer(...): { builder: WebGLFrameBuilder; syncCamera: () => void } {
+function mountWebGLDebugRenderer(...): {
+  builder: WebGLFrameBuilder;
+  syncCamera: () => void;
+  dispose: () => void;
+} {
   ...
-  window.addEventListener("keydown", (e) => {
+  const onKeyDown = (e: KeyboardEvent) => {
     if (e.key === "\\") {
       glCanvas.style.display =
         glCanvas.style.display === "none" ? "block" : "none";
     }
-  });
+  };
+  window.addEventListener("keydown", onKeyDown);
   ...
-  return { builder: new WebGLFrameBuilder(view, gameView), syncCamera };
+  return {
+    builder: new WebGLFrameBuilder(view, gameView),
+    syncCamera,
+    dispose: () => {
+      window.removeEventListener("keydown", onKeyDown);
+      glCanvas.remove();
+    },
+  };
}

Also applies to: 265-270

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/client/ClientGameRunner.ts` around lines 247 - 250, ClientGameRunner is
adding a debug canvas (created via createCanvas and assigned to glCanvas with id
"webgl-debug-canvas") and a global keydown listener but never removes them,
causing duplicates on re-entry; update the code that creates these resources
(the createCanvas/glCanvas block and the keydown event registration) to return
or attach a cleanup function/handle (or implement a dispose method on
ClientGameRunner) that removes the canvas from document.body (selecting by the
"webgl-debug-canvas" id or the stored glCanvas reference) and calls
removeEventListener for the keydown handler, and ensure this cleanup is invoked
when the game is torn down or before creating a new debug canvas so
listeners/canvases do not stack.

src/client/ClientGameRunner.ts-344-350 (1)

344-350: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Gate the WebGL mirror behind a debug flag.

This path mounts and syncs a second renderer for every game session, even outside debugging. That adds avoidable CPU/GPU work on the hot frame loop.

Suggested direction

-    const { builder: webglBuilder, syncCamera } = mountWebGLDebugRenderer(
-      gameMap,
-      gameView,
-      gameRenderer.transformHandler,
-    );
-    gameRenderer.onPreRender = syncCamera;
+    let webglBuilder: WebGLFrameBuilder | null = null;
+    if (userSettings.debugWebGLMirror?.()) {
+      const mounted = mountWebGLDebugRenderer(
+        gameMap,
+        gameView,
+        gameRenderer.transformHandler,
+      );
+      webglBuilder = mounted.builder;
+      gameRenderer.onPreRender = mounted.syncCamera;
+    }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/client/ClientGameRunner.ts` around lines 344 - 350, Wrap the call to
mountWebGLDebugRenderer and the assignment gameRenderer.onPreRender = syncCamera
behind a runtime debug flag (e.g. DEBUG_WEBGL_MIRROR or a
config.debug.webglMirror) so the WebGL mirror is only created when debugging;
locate mountWebGLDebugRenderer(...) and webglBuilder/syncCamera references and
conditionally call it, avoid assigning gameRenderer.onPreRender when syncCamera
is undefined, and ensure any created webglBuilder is properly disposed/cleaned
up when the game session ends.

src/client/render/gl/debug/layout.ts-9-746 (1)

9-746: ⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Route the debug labels through translateText().

This tree is full of user-visible labels, but they are all hardcoded English strings. Please move them behind translateText() and add matching en.json entries so the new debug UI follows the client i18n rule. As per coding guidelines src/client/**/*.{ts,tsx}: "All user-visible text must go through translateText() function with corresponding entries in resources/lang/en.json".

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/client/render/gl/debug/layout.ts` around lines 9 - 746, The buildTree
function returns many user-visible labels passed as raw strings to helper
constructors (folder, toggle, slider, color, select); wrap each label string in
translateText(...) and add corresponding keys/entries to resources/lang/en.json
(use a clear namespace like debug.layout.<Section>.<Label> or derive keys from
folder names and field names) so every label (folder titles like "Pass Enables",
slider/ toggle/ color labels like "Bloom Color", option labels like "Mode", and
any custom tooltip strings) goes through translateText; update calls in
buildTree to reference the new keys via translateText and ensure en.json
contains matching English values.

src/client/render/gl/game-view.ts-72-100 (1)

72-100: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Dispose never detaches the DOM event handlers.

All of these listeners are added with anonymous functions, so dispose() cannot remove them. A disposed GameView will still react to document/canvas events, and remounting another view will duplicate input handling.

Also applies to: 403-409

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/client/render/gl/game-view.ts` around lines 72 - 100, The event listeners
on canvas and document are added with inline anonymous callbacks so dispose()
cannot remove them; update GameView to store the bound handler functions as
properties (e.g., this._onPointerDown, this._onPointerMove, this._onPointerUp,
this._onPointerCancel, this._onWheel, this._onContextMenu, this._onDblClick,
this._onAuxClick, this._onKeyDown, this._onKeyUp) that call the corresponding
this.interaction.handle* methods, attach those named handlers instead of
anonymous functions, and then call removeEventListener for each stored handler
inside dispose() to fully detach all listeners (include pointercancel, wheel,
contextmenu, dblclick, auxclick, keydown, keyup).

src/client/render/gl/passes/sam-radius-pass.ts-258-270 (1)

258-270: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Rebuild owner-mode instances after palette or cluster changes.

updateStructures() writes color/group data into the GPU buffer, but setPaletteData() and setAllianceClusters() only update fields. In owner mode, changing either input leaves stale colors and stale merged arcs on screen until some later structure refresh happens.

Suggested fix

  setPaletteData(data: Float32Array): void {
    this.paletteData = data;
+   if (this.colorMode === "owner") this.rebuild();
  }

  setAllianceClusters(clusters: Map<number, number>): void {
    this.allianceClusters = clusters;
+   if (this.colorMode === "owner") this.rebuild();
  }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/client/render/gl/passes/sam-radius-pass.ts` around lines 258 - 270,
setPaletteData and setAllianceClusters only update internal fields but don’t
refresh GPU owner-mode instances, so colors and merged arcs remain stale; modify
setPaletteData(data: Float32Array) and setAllianceClusters(clusters: Map<number,
number>) to, after updating this.paletteData / this.allianceClusters, call
this.rebuild() when this.colorMode === "owner" (so owner-mode structures created
in updateStructures() get recreated with the new palette/cluster data);
reference setPaletteData, setAllianceClusters, colorMode, rebuild(), and
updateStructures() when making the change.

🟡 Minor comments (12)

src/client/render/gl/passes/point-light-pass.ts-139-145 (1)

139-145: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Track and dispose the quad vertex buffer.

The quad buffer is allocated but never deleted. Recreating this pass leaks GPU memory.

💡 Suggested fix

 export class PointLightPass {
@@
   private lightVao: WebGLVertexArrayObject;
   private lightBuf: WebGLBuffer;
+  private quadBuf: WebGLBuffer;
@@
-    const quadBuf = gl.createBuffer()!;
-    gl.bindBuffer(gl.ARRAY_BUFFER, quadBuf);
+    this.quadBuf = gl.createBuffer()!;
+    gl.bindBuffer(gl.ARRAY_BUFFER, this.quadBuf);
@@
   dispose(): void {
     const gl = this.gl;
     gl.deleteProgram(this.lightProg);
     gl.deleteVertexArray(this.lightVao);
     gl.deleteBuffer(this.lightBuf);
+    gl.deleteBuffer(this.quadBuf);
   }
 }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/client/render/gl/passes/point-light-pass.ts` around lines 139 - 145, The
quad vertex buffer (quadBuf) is created but never freed, leaking GPU memory;
store the buffer on the pass instance (e.g., this.quadBuf) when creating it in
the point-light-pass initialization and call gl.deleteBuffer(this.quadBuf) in
the pass's dispose/cleanup method (or the existing destroy method) to ensure the
buffer is deleted when the pass is torn down; also null out this.quadBuf after
deletion and remove any lingering bindings to avoid use-after-free.

src/client/render/gl/render-settings.ts-251-253 (1)

251-253: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Delay URL.revokeObjectURL to avoid download race.

Revoking right after Line 251 can cancel downloads in some environments. Revoke on a short timeout.

Suggested fix

   a.download = "render-settings.json";
   a.click();
-  URL.revokeObjectURL(url);
+  setTimeout(() => URL.revokeObjectURL(url), 0);
 }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/client/render/gl/render-settings.ts` around lines 251 - 253, The
immediate call to URL.revokeObjectURL(url) after a.click() can abort some
downloads; update the function that creates the anchor and calls a.click() to
revoke the object URL asynchronously (e.g., wrap URL.revokeObjectURL(url) in a
short timeout or next tick). Locate the code that calls a.click() and uses the
variable url and replace the direct revoke with a delayed revoke (setTimeout or
similar) so the download can start before URL.revokeObjectURL(url) is invoked.

src/client/render/gl/debug/index.ts-12-12 (1)

12-12: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Localize the debug GUI title.

"Render Settings" is user-visible text and should be routed through translateText() with an en.json key.

As per coding guidelines "All user-visible text must go through translateText() function with corresponding entries in resources/lang/en.json".

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/client/render/gl/debug/index.ts` at line 12, Replace the hardcoded GUI
title string with a call to translateText and add the matching en.json key:
change the GUI instantiation (const gui = new GUI({ title: "Render Settings",
width: 320 });) to use title: translateText('<your_key_here>') (e.g.,
'gui.renderSettings'), and add the corresponding entry in resources/lang/en.json
("gui.renderSettings": "Render Settings"); ensure you import translateText if
not already imported.

src/client/render/gl/passes/name-pass/icon-program.ts-110-148 (1)

110-148: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Prevent late atlas callbacks from allocating textures after disposal.

When dispose() happens before image load completion, img.onload can still allocate textures and mark the pass ready, leaking GPU memory.

Suggested fix

 export class IconProgram {
+  private disposed = false;
@@
   private loadAtlases(): void {
     const gl = this.gl;
     const load = (url: string, cb: (tex: WebGLTexture) => void) => {
       const img = new Image();
       img.onload = () => {
+        if (this.disposed) return;
         const tex = gl.createTexture()!;
@@
   dispose(): void {
+    this.disposed = true;
     const gl = this.gl;
     gl.deleteProgram(this.program);
     if (this.flagAtlasTex) gl.deleteTexture(this.flagAtlasTex);
     if (this.emojiAtlasTex) gl.deleteTexture(this.emojiAtlasTex);
   }
 }

Also applies to: 180-185

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/client/render/gl/passes/name-pass/icon-program.ts` around lines 110 -
148, The image onload handlers in loadAtlases can run after dispose and allocate
GPU textures; modify loadAtlases and dispose so the onload callback first checks
a disposed flag (e.g., this.disposed) and returns early if true, and ensure
dispose nulls or removes img.onload handlers to prevent them firing; update the
onload paths that create textures (producing flagAtlasTex/emojiAtlasTex and
setting iconsReady) to only do so when this.disposed is false, and ensure
dispose also deletes any created WebGL textures if necessary.

src/client/render/gl/passes/name-pass/status-icon-program.ts-103-123 (1)

103-123: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Guard async atlas loading after disposal.

If dispose() runs before the image finishes loading, img.onload can still allocate a texture and store it on a disposed pass. This leaks GPU resources.

Suggested fix

 export class StatusIconProgram {
+  private disposed = false;
@@
   private loadAtlas(): void {
     const gl = this.gl;
     const img = new Image();
     img.onload = () => {
+      if (this.disposed) return;
       const tex = gl.createTexture()!;
@@
       this.statusAtlasTex = tex;
       this.atlasReady = true;
     };
@@
   dispose(): void {
+    this.disposed = true;
     const gl = this.gl;
     gl.deleteProgram(this.program);
     if (this.statusAtlasTex) gl.deleteTexture(this.statusAtlasTex);
   }
 }

Also applies to: 158-162

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/client/render/gl/passes/name-pass/status-icon-program.ts` around lines
103 - 123, The image onload handler in loadAtlas currently creates a GL texture
even if the pass has been disposed, leaking GPU resources; modify the img.onload
callbacks (in loadAtlas and the other onload at lines ~158-162) to first check a
disposed flag (e.g., this.disposed) and return early if set, and only then
create and assign the texture (this.statusAtlasTex) and set this.atlasReady;
also ensure dispose() sets this.disposed = true, removes or nulls pending
img.onload handlers, and deletes any created texture via
gl.deleteTexture(this.statusAtlasTex) to fully guard against leaks.

src/client/render/gl/passes/railroad-pass.ts-192-193 (1)

192-193: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Release the pass-owned VAO in dispose().

this.vao is created in the constructor but never deleted, so this pass leaks GPU resources.

Suggested fix

   dispose(): void {
     const gl = this.gl;
     gl.deleteProgram(this.program);
+    gl.deleteVertexArray(this.vao);
     gl.deleteTexture(this.railroadTex);
     gl.deleteTexture(this.ghostRailTex);
     gl.deleteTexture(this.terrainTex);

Also applies to: 332-339

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/client/render/gl/passes/railroad-pass.ts` around lines 192 - 193, The
pass creates a VAO via this.vao = createMapQuad(gl, mapW, mapH) (and again in
the other creation block around lines 332-339) but never frees it; add cleanup
in the class dispose() method to check for this.vao, call
gl.deleteVertexArray(this.vao) (or the appropriate deletion function for your GL
context), and set this.vao = null to avoid double-free and leaking GPU
resources; apply the same pattern for any other pass-owned GL objects created in
those blocks.

src/client/render/frame/derive/attack-rings.ts-35-41 (1)

35-41: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Keep the targeted extractor aligned with the transport-only contract.

extractAttackRingsFromIds currently trusts transportIds completely, but unlike the full-scan variant it does not re-check u.unitType === UT_TRANSPORT. A stale/misclassified ID can produce incorrect attack rings.

Suggested patch

   for (const id of transportIds) {
     const u = units.get(id);
-    if (!u || u.targetTile === null || !u.isActive || u.retreating) continue;
+    if (
+      !u ||
+      u.unitType !== UT_TRANSPORT ||
+      u.targetTile === null ||
+      !u.isActive ||
+      u.retreating
+    ) continue;
     if (ownerFilter !== undefined && u.ownerID !== ownerFilter) continue;
     const t = u.targetTile;
     rings.push({ x: t % mapW, y: (t - (t % mapW)) / mapW, unitId: u.id });
   }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/client/render/frame/derive/attack-rings.ts` around lines 35 - 41, The
loop in extractAttackRingsFromIds trusts transportIds and can include
stale/non-transport units; add an explicit check that u.unitType ===
UT_TRANSPORT (using the UT_TRANSPORT enum/constant) before pushing a ring.
Update the loop in attack-rings.ts to skip entries where u is falsy or
u.unitType !== UT_TRANSPORT (in addition to the existing checks like
u.targetTile === null, !u.isActive, u.retreating and ownerFilter), so only
confirmed transport units produce rings.

src/client/render/gl/passes/move-indicator-pass.ts-57-59 (1)

57-59: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Store the quad buffer as a class field and delete it in dispose().

Line 57 creates a WebGL buffer that is never deleted. The VAO at line 55 stores a reference to this buffer, but deleting the VAO does not release the buffer. This leaks GPU memory each time the pass is recreated.

Store the buffer as private quadBuf: WebGLBuffer and delete it in dispose() with gl.deleteBuffer(this.quadBuf).

Suggested fix

 export class MoveIndicatorPass {
   private gl: WebGL2RenderingContext;
   private settings: RenderSettings;
   private program: WebGLProgram;
   private vao: WebGLVertexArrayObject;
+  private quadBuf: WebGLBuffer;

   private uCamera: WebGLUniformLocation;

     // Unit quad [0,1]
     this.vao = gl.createVertexArray()!;
     gl.bindVertexArray(this.vao);
-    const buf = gl.createBuffer()!;
-    gl.bindBuffer(gl.ARRAY_BUFFER, buf);
+    this.quadBuf = gl.createBuffer()!;
+    gl.bindBuffer(gl.ARRAY_BUFFER, this.quadBuf);
     gl.bufferData(

   dispose(): void {
     const gl = this.gl;
     gl.deleteProgram(this.program);
+    gl.deleteBuffer(this.quadBuf);
     gl.deleteVertexArray(this.vao);
   }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/client/render/gl/passes/move-indicator-pass.ts` around lines 57 - 59, The
WebGL quad buffer created with gl.createBuffer() in MoveIndicatorPass is never
deleted, leaking GPU memory; add a private field (e.g. private quadBuf:
WebGLBuffer | null) to the MoveIndicatorPass class, assign the created buffer to
this.quadBuf instead of a local variable when creating it (where buf is
currently created), and then release it in dispose() by calling
gl.deleteBuffer(this.quadBuf) (and nulling it) alongside existing VAO cleanup to
ensure the buffer is freed when the pass is destroyed.

src/client/render/gl/passes/bar-pass.ts-94-95 (1)

94-95: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Add quadBuf as an instance variable and delete it in dispose().

Line 94 creates quadBuf as a local variable, but it is never deleted. This leaks one WebGL buffer per BarPass instance. Move quadBuf to a class instance variable and call this.gl.deleteBuffer(this.quadBuf) in the dispose() method alongside the existing buffer cleanup.

Suggested fix

 export class BarPass {
   private gl: WebGL2RenderingContext;
   private settings: RenderSettings;
   private program: WebGLProgram;
   private maxBars = 2048;
 
   private uCamera: WebGLUniformLocation;
   private uBarSize: WebGLUniformLocation;
   private uBarOffset: WebGLUniformLocation;
   private uBorderWidth: WebGLUniformLocation;
   private uThresholds: WebGLUniformLocation;
   private uColorRed: WebGLUniformLocation;
   private uColorOrange: WebGLUniformLocation;
   private uColorYellow: WebGLUniformLocation;
   private uColorGreen: WebGLUniformLocation;
 
   private vao: WebGLVertexArrayObject;
+  private quadBuf: WebGLBuffer;
   private instanceBuf: WebGLBuffer;
 
     // Quad vertices (2 triangles)
-    const quadBuf = gl.createBuffer()!;
+    this.quadBuf = gl.createBuffer()!;
-    gl.bindBuffer(gl.ARRAY_BUFFER, quadBuf);
+    gl.bindBuffer(gl.ARRAY_BUFFER, this.quadBuf);
 
   dispose(): void {
     this.gl.deleteProgram(this.program);
+    this.gl.deleteBuffer(this.quadBuf);
     this.gl.deleteBuffer(this.instanceBuf);
     this.gl.deleteVertexArray(this.vao);
   }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/client/render/gl/passes/bar-pass.ts` around lines 94 - 95, The quadBuf is
created as a local in the BarPass constructor and never deleted, leaking a WebGL
buffer; make quadBuf an instance property on the BarPass class (e.g.,
this.quadBuf) where it's currently created (replace const quadBuf = ... with
this.quadBuf = ...) and add a deletion in the dispose() method by calling
this.gl.deleteBuffer(this.quadBuf) alongside the existing buffer cleanup, then
remove any leftover local references so all uses reference this.quadBuf.

src/client/render/gl/debug/wiring.ts-71-72 (1)

71-72: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Route debug menu labels through translateText().

These strings are user-visible and currently hardcoded.

As per coding guidelines src/client/**/*.{ts,tsx}: “All user-visible text must go through translateText() function with corresponding entries in resources/lang/en.json.”

Also applies to: 96-109, 196-197

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/client/render/gl/debug/wiring.ts` around lines 71 - 72, The GUI labels
are hardcoded; update all user-visible strings added via gui.add (e.g., the call
gui.add({ dump: () => dumpSettings(settings) }, "dump").name("Download JSON"))
and the other strings noted (lines ~96-109 and ~196-197) to use translateText()
with matching keys in resources/lang/en.json. Replace literal label arguments
passed to .name(...) and any other visible text in the wiring.ts debug menu with
translateText("your.key.here"), add the corresponding keys/values to en.json,
and ensure dumpSettings/settings references remain unchanged.

src/client/render/gl/passes/border-compute-pass.ts-258-264 (1)

258-264: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Release the VAO in dispose() as well.

this.vao is created but never deleted, so repeated renderer lifecycles leak GPU objects.

Patch

  dispose(): void {
    const gl = this.gl;
    gl.deleteProgram(this.program);
    gl.deleteTexture(this.borderTex);
    gl.deleteTexture(this.relationTex);
    gl.deleteFramebuffer(this.borderFbo);
+    gl.deleteVertexArray(this.vao);
  }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/client/render/gl/passes/border-compute-pass.ts` around lines 258 - 264,
dispose() currently deletes program, textures and framebuffer but never releases
the created VAO, leaking GPU objects; update the dispose() implementation in
border-compute-pass.ts to check for this.vao and call
gl.deleteVertexArray(this.vao) (or the appropriate OES deleteVertexArrayOES if
using the extension), then null out this.vao after deletion so the VAO is
actually released and not retained.

src/client/render/gl/map-interaction.ts-209-247 (1)

209-247: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Handle modifier changes for hold bindings.

With REPLAY_KEY_BINDINGS, peek turns on only when KeyG goes down with Shift already pressed, and it turns off only when KeyG goes up. Pressing G before Shift, or releasing Shift first, leaves the hold state out of sync with the actual combo.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/client/render/gl/map-interaction.ts` around lines 209 - 247, The
hold-binding logic fails when modifiers change after the main keydown; update
handleKeyDown/handleKeyUp to track current pressed keys and evaluate the hold
condition against the current modifier state instead of only the event that
started the hold: add a small Set (e.g. pressedKeys) that you update in
handleKeyDown and handleKeyUp, then replace the one-off activation code to check
matchesHold(e, this.keys.altViewPeek) using the current pressedKeys and
e.getModifierState (or e.shiftKey) so that when Shift is pressed or released
while the main key (this.keys.altViewPeek.code) remains down you start or stop
peek accordingly; adjust matchesHold (or call it with synthesized state) to
consult current modifier state, and ensure peekHeld is toggled and
applyAltView/applyGridView and deps.emit("altviewpeek", ...) are invoked when
the overall hold condition transitions on or off.

[coderabbitai]

Actionable comments posted: 5

🧹 Nitpick comments (3)

src/client/ClientGameRunner.ts (2)

310-315: 💤 Low value

Missing cleanup for keydown listener.

The keydown listener for toggling the WebGL canvas is added to window but never removed. In a long-running session or if the game is stopped and restarted, this can accumulate duplicate listeners.

Consider returning a cleanup function from mountWebGLDebugRenderer or storing the listener reference for later removal.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/client/ClientGameRunner.ts` around lines 310 - 315, The keydown listener
added in mountWebGLDebugRenderer (window.addEventListener("keydown", ...)) never
gets removed; update mountWebGLDebugRenderer to either return a cleanup function
that removes that listener or store the handler reference and call
window.removeEventListener("keydown", handler) when unmounting/stopping the
game; specifically capture the listener callback (the function checking e.key
=== "\\") as a named/const handler so it can be passed to removeEventListener
and ensure any code that stops/restarts the renderer calls the cleanup.

---

243-248: 💤 Low value

Nested loop for terrain bytes could be simplified.

The double loop is fine, but consider using a single linear iteration for clarity and a small perf win:

Optional simplification

-  for (let y = 0; y < mapHeight; y++) {
-    for (let x = 0; x < mapWidth; x++) {
-      terrainBytes[y * mapWidth + x] = gameMap.terrainByte(gameMap.ref(x, y));
-    }
-  }
+  for (let i = 0; i < mapWidth * mapHeight; i++) {
+    const x = i % mapWidth;
+    const y = Math.floor(i / mapWidth);
+    terrainBytes[i] = gameMap.terrainByte(gameMap.ref(x, y));
+  }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/client/ClientGameRunner.ts` around lines 243 - 248, Replace the nested
x/y loops that fill terrainBytes with a single linear loop over i from 0 to
mapWidth*mapHeight-1; compute x = i % mapWidth and y = Math.floor(i / mapWidth)
and set terrainBytes[i] = gameMap.terrainByte(gameMap.ref(x, y)). Update the
block in ClientGameRunner (variables: terrainBytes, mapWidth, mapHeight,
gameMap) accordingly to simplify iteration and slightly improve performance.

src/client/controllers/BuildPreviewController.ts (1)

135-138: 💤 Low value

Unused variable targetingAlly with void cast.

The targetingAlly variable is computed but explicitly voided. If the ally-targeting warning visual is planned for later, consider removing this dead code and adding it back when the feature is implemented. Otherwise, this adds unnecessary computation each frame.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/client/controllers/BuildPreviewController.ts` around lines 135 - 138,
Remove the dead `targetingAlly` computation and the `void targetingAlly;` line
from BuildPreviewController (the variable and any upstream calculation that only
exists to be void-cast), or if you intend to keep it for a planned visual
feature, instead stop computing it each frame and defer its calculation until
the feature is used; reference the `targetingAlly` symbol and its associated
computation in the BuildPreviewController to locate and remove/refactor those
lines.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/client/ClientGameRunner.ts`:
- Around line 322-331: The ResizeObserver instance created as resizeObs
(observing glCanvas and updating cachedCanvasW/cachedCanvasH) is never
disconnected; make resizeObs a field on the ClientGameRunner instance (instead
of a local const) so it can be accessed later, and call resizeObs.disconnect()
in the game's cleanup/stop method (e.g., in the existing stop/destroy/cleanup
function) to unobserve glCanvas and release resources. Ensure you null out the
field after disconnecting to avoid retaining references.
- Around line 367-375: The RAF loop created in driveFrame uses
requestAnimationFrame without ever saving its handle, so it never stops; modify
the start-up code that calls requestAnimationFrame(driveFrame) to store the
returned handle (e.g., this.driveRafId or a local field) and have driveFrame
update that field each frame, and then cancel that RAF in
ClientGameRunner.stop() (call cancelAnimationFrame(this.driveRafId) and clear
the field) as part of cleanup; ensure you check for presence of the handle
before calling cancelAnimationFrame to avoid errors.

In `@src/client/controllers/WarshipSelectionController.ts`:
- Around line 89-102: The created div (dragRectEl) is appended in
ensureDragRectEl but never removed; add a public dispose() method on
WarshipSelectionController that checks this.dragRectEl, calls remove() on the
element (or document.body.removeChild), and sets this.dragRectEl = null to
prevent duplicates and leaks; update any controller teardown path (where the
controller is destroyed on game end/restart) to call
WarshipSelectionController.dispose() so the element is cleaned up.

In `@src/client/graphics/layers/AttackingTroopsOverlay.ts`:
- Around line 91-99: The RAF loop started in the constructor via the drive()
function keeps re-scheduling forever and isn’t cancelled on teardown; capture
the requestAnimationFrame handle (e.g., store it on this.rafId when calling
requestAnimationFrame(drive)) and change the loop to update that handle each
frame, then cancel it in destroy() using cancelAnimationFrame(this.rafId); also
ensure any re-init path clears an existing rafId before starting a new loop so
multiple loops cannot stack (references: drive, updateLabelDOM, destroy).

In `@src/client/WebGLFrameBuilder.ts`:
- Around line 52-69: The writePaletteEntry method currently writes into
this.palette using smallID without validating bounds, risking out-of-range
writes when smallID >= PALETTE_SIZE; update WebGLFrameBuilder.writePaletteEntry
to check that smallID is within 0 <= smallID < PALETTE_SIZE (use the
PALETTE_SIZE constant) before computing offsets and writing to this.palette, and
on violation either early-return (or throw/log an error) to prevent
out-of-bounds writes—apply this guard for both fill and border writes so the
Float32Array is never indexed past its length.

---

Nitpick comments:
In `@src/client/ClientGameRunner.ts`:
- Around line 310-315: The keydown listener added in mountWebGLDebugRenderer
(window.addEventListener("keydown", ...)) never gets removed; update
mountWebGLDebugRenderer to either return a cleanup function that removes that
listener or store the handler reference and call
window.removeEventListener("keydown", handler) when unmounting/stopping the
game; specifically capture the listener callback (the function checking e.key
=== "\\") as a named/const handler so it can be passed to removeEventListener
and ensure any code that stops/restarts the renderer calls the cleanup.
- Around line 243-248: Replace the nested x/y loops that fill terrainBytes with
a single linear loop over i from 0 to mapWidth*mapHeight-1; compute x = i %
mapWidth and y = Math.floor(i / mapWidth) and set terrainBytes[i] =
gameMap.terrainByte(gameMap.ref(x, y)). Update the block in ClientGameRunner
(variables: terrainBytes, mapWidth, mapHeight, gameMap) accordingly to simplify
iteration and slightly improve performance.

In `@src/client/controllers/BuildPreviewController.ts`:
- Around line 135-138: Remove the dead `targetingAlly` computation and the `void
targetingAlly;` line from BuildPreviewController (the variable and any upstream
calculation that only exists to be void-cast), or if you intend to keep it for a
planned visual feature, instead stop computing it each frame and defer its
calculation until the feature is used; reference the `targetingAlly` symbol and
its associated computation in the BuildPreviewController to locate and
remove/refactor those lines.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 6fdae6da-bdd9-4e8b-9c7b-da54872373fd

📥 Commits

Reviewing files that changed from the base of the PR and between 53cf2d4 and 7b1557b.

⛔ Files ignored due to path filters (1)

• src/client/render/gl/shaders/structure/structure.vert.glsl is excluded by !**/*.glsl

📒 Files selected for processing (66)

• src/client/ClientGameRunner.ts
• src/client/InputHandler.ts
• src/client/WebGLFrameBuilder.ts
• src/client/controllers/BuildPreviewController.ts
• src/client/controllers/WarshipSelectionController.ts
• src/client/graphics/GameRenderer.ts
• src/client/graphics/TransformHandler.ts
• src/client/graphics/layers/AlertFrame.ts
• src/client/graphics/layers/AttackingTroopsOverlay.ts
• src/client/graphics/layers/AttacksDisplay.ts
• src/client/graphics/layers/BuildMenu.ts
• src/client/graphics/layers/ChatDisplay.ts
• src/client/graphics/layers/ChatModal.ts
• src/client/graphics/layers/ControlPanel.ts
• src/client/graphics/layers/Controller.ts
• src/client/graphics/layers/DynamicUILayer.ts
• src/client/graphics/layers/EventsDisplay.ts
• src/client/graphics/layers/GameLeftSidebar.ts
• src/client/graphics/layers/GameRightSidebar.ts
• src/client/graphics/layers/HeadsUpMessage.ts
• src/client/graphics/layers/ImmunityTimer.ts
• src/client/graphics/layers/InGamePromo.ts
• src/client/graphics/layers/Layer.ts
• src/client/graphics/layers/Leaderboard.ts
• src/client/graphics/layers/MainRadialMenu.ts
• src/client/graphics/layers/MultiTabModal.ts
• src/client/graphics/layers/NameLayer.ts
• src/client/graphics/layers/PerformanceOverlay.ts
• src/client/graphics/layers/PlayerInfoOverlay.ts
• src/client/graphics/layers/PlayerPanel.ts
• src/client/graphics/layers/RadialMenu.ts
• src/client/graphics/layers/ReplayPanel.ts
• src/client/graphics/layers/SettingsModal.ts
• src/client/graphics/layers/SpawnTimer.ts
• src/client/graphics/layers/StructureIconsLayer.ts
• src/client/graphics/layers/TeamStats.ts
• src/client/graphics/layers/UILayer.ts
• src/client/graphics/layers/UnitDisplay.ts
• src/client/graphics/layers/WinModal.ts
• src/client/graphics/ui/MoveIndicatorUI.ts
• src/client/graphics/ui/NavalTarget.ts
• src/client/graphics/ui/NukeTelegraph.ts
• src/client/render/frame/derive/player-status.ts
• src/client/render/frame/derive/relation-matrix.ts
• src/client/render/gl/game-view.ts
• src/client/render/gl/passes/selection-box-pass.ts
• src/client/render/gl/passes/structure-pass.ts
• src/client/render/gl/render-settings.json
• src/client/render/gl/render-settings.ts
• src/client/render/gl/renderer.ts
• src/client/render/types/renderer.ts
• src/client/view/GameView.ts
• src/client/view/PlayerView.ts
• src/client/view/UnitView.ts
• src/core/game/GameImpl.ts
• src/core/game/GameMap.ts
• src/core/game/GameView.ts
• tests/client/controllers/BuildPreviewController.test.ts
• tests/client/controllers/WarshipSelectionController.test.ts
• tests/client/graphics/UILayer.test.ts
• tests/client/render/frame/derive/player-status.test.ts
• tests/client/view/GameView.test.ts
• tests/client/view/PlayerView.test.ts
• tests/client/view/UnitView.test.ts
• tests/core/game/GameMap.tileStateBuffer.test.ts
• tests/util/viewStubs.ts

💤 Files with no reviewable changes (8)

• src/client/graphics/ui/MoveIndicatorUI.ts
• src/client/graphics/ui/NukeTelegraph.ts
• src/client/graphics/ui/NavalTarget.ts
• src/client/graphics/layers/Layer.ts
• src/client/graphics/layers/DynamicUILayer.ts
• src/client/graphics/layers/NameLayer.ts
• src/client/graphics/layers/UILayer.ts
• src/client/graphics/layers/StructureIconsLayer.ts

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

• src/client/render/frame/derive/relation-matrix.ts
• src/client/render/gl/game-view.ts

[coderabbitai]

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

ResizeObserver is never disconnected.

The ResizeObserver at line 322 observes glCanvas but is never disconnected when the game stops. This keeps the observer alive and can cause memory leaks if the canvas is removed from the DOM.

Store resizeObs and call resizeObs.disconnect() in the cleanup path.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/client/ClientGameRunner.ts` around lines 322 - 331, The ResizeObserver
instance created as resizeObs (observing glCanvas and updating
cachedCanvasW/cachedCanvasH) is never disconnected; make resizeObs a field on
the ClientGameRunner instance (instead of a local const) so it can be accessed
later, and call resizeObs.disconnect() in the game's cleanup/stop method (e.g.,
in the existing stop/destroy/cleanup function) to unobserve glCanvas and release
resources. Ensure you null out the field after disconnecting to avoid retaining
references.

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Self-driving RAF loop has no cancellation mechanism.

The driveFrame function schedules itself indefinitely via requestAnimationFrame. When the game stops (ClientGameRunner.stop()), this loop keeps running, potentially causing errors or wasted CPU cycles.

Return the RAF handle and cancel it in the cleanup path:

Proposed fix

+  let rafHandle: number;
   const driveFrame = (): void => {
     syncCamera();
-    requestAnimationFrame(driveFrame);
+    rafHandle = requestAnimationFrame(driveFrame);
   };
-  requestAnimationFrame(driveFrame);
+  rafHandle = requestAnimationFrame(driveFrame);

-  return { builder: new WebGLFrameBuilder(view) };
+  return {
+    builder: new WebGLFrameBuilder(view),
+    stop: () => cancelAnimationFrame(rafHandle),
+  };

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/client/ClientGameRunner.ts` around lines 367 - 375, The RAF loop created
in driveFrame uses requestAnimationFrame without ever saving its handle, so it
never stops; modify the start-up code that calls
requestAnimationFrame(driveFrame) to store the returned handle (e.g.,
this.driveRafId or a local field) and have driveFrame update that field each
frame, and then cancel that RAF in ClientGameRunner.stop() (call
cancelAnimationFrame(this.driveRafId) and clear the field) as part of cleanup;
ensure you check for presence of the handle before calling cancelAnimationFrame
to avoid errors.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

DOM element dragRectEl is never removed from the body.

The ensureDragRectEl method appends a div to document.body but there is no cleanup. When the game ends or restarts, the element remains in the DOM, potentially accumulating duplicates.

Consider adding a dispose() method:

Proposed cleanup

+  dispose(): void {
+    if (this.dragRectEl) {
+      this.dragRectEl.remove();
+      this.dragRectEl = null;
+    }
+  }

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	private ensureDragRectEl(): void {
	if (this.dragRectEl !== null) return;
	const el = document.createElement("div");
	el.id = "warship-drag-rect";
	el.style.position = "fixed";
	el.style.pointerEvents = "none";
	el.style.display = "none";
	el.style.zIndex = "30";
	el.style.borderStyle = "dashed";
	el.style.borderWidth = "1px";
	el.style.boxSizing = "border-box";
	document.body.appendChild(el);
	this.dragRectEl = el;
	}
	private ensureDragRectEl(): void {
	if (this.dragRectEl !== null) return;
	const el = document.createElement("div");
	el.id = "warship-drag-rect";
	el.style.position = "fixed";
	el.style.pointerEvents = "none";
	el.style.display = "none";
	el.style.zIndex = "30";
	el.style.borderStyle = "dashed";
	el.style.borderWidth = "1px";
	el.style.boxSizing = "border-box";
	document.body.appendChild(el);
	this.dragRectEl = el;
	}
	dispose(): void {
	if (this.dragRectEl) {
	this.dragRectEl.remove();
	this.dragRectEl = null;
	}
	}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/client/controllers/WarshipSelectionController.ts` around lines 89 - 102,
The created div (dragRectEl) is appended in ensureDragRectEl but never removed;
add a public dispose() method on WarshipSelectionController that checks
this.dragRectEl, calls remove() on the element (or document.body.removeChild),
and sets this.dragRectEl = null to prevent duplicates and leaks; update any
controller teardown path (where the controller is destroyed on game end/restart)
to call WarshipSelectionController.dispose() so the element is cleaned up.

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Stop the self-driven RAF loop on teardown.

Line 96 re-queues forever, but destroy() never cancels it. Re-inits can stack multiple loops and keep updating detached DOM.

🔧 Suggested fix

 export class AttackingTroopsOverlay implements Controller {
   private container: HTMLDivElement;
   private labelTemplate: HTMLDivElement;
+  private rafId: number | null = null;
+  private destroyed = false;
@@
   init() {
+    this.destroyed = false;
@@
     const drive = () => {
+      if (this.destroyed) return;
       this.updateLabelDOM();
-      requestAnimationFrame(drive);
+      this.rafId = requestAnimationFrame(drive);
     };
-    requestAnimationFrame(drive);
+    this.rafId = requestAnimationFrame(drive);
   }
@@
   destroy() {
+    this.destroyed = true;
+    if (this.rafId !== null) {
+      cancelAnimationFrame(this.rafId);
+      this.rafId = null;
+    }
     if (!this.container) return;
     this.clearAllLabels();
     this.container.remove();
     this.eventBus.off(AlternateViewEvent, this.onAlternateView);
   }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/client/graphics/layers/AttackingTroopsOverlay.ts` around lines 91 - 99,
The RAF loop started in the constructor via the drive() function keeps
re-scheduling forever and isn’t cancelled on teardown; capture the
requestAnimationFrame handle (e.g., store it on this.rafId when calling
requestAnimationFrame(drive)) and change the loop to update that handle each
frame, then cancel it in destroy() using cancelAnimationFrame(this.rafId); also
ensure any re-init path clears an existing rafId before starting a new loop so
multiple loops cannot stack (references: drive, updateLabelDOM, destroy).

[coderabbitai]

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

No bounds check on smallID before palette write.

If smallID >= PALETTE_SIZE (4096), writePaletteEntry writes past the Float32Array bounds. While the WebGL view is constructed with maxPlayers: 1024, this assumption is not enforced here.

Consider adding a guard:

Defensive check

 private writePaletteEntry(
   smallID: number,
   fill: Colord,
   border: Colord,
 ): void {
+  if (smallID >= PALETTE_SIZE) {
+    console.warn(`smallID ${smallID} exceeds palette size`);
+    return;
+  }
   const fillRgba = fill.toRgb();

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/client/WebGLFrameBuilder.ts` around lines 52 - 69, The writePaletteEntry
method currently writes into this.palette using smallID without validating
bounds, risking out-of-range writes when smallID >= PALETTE_SIZE; update
WebGLFrameBuilder.writePaletteEntry to check that smallID is within 0 <= smallID
< PALETTE_SIZE (use the PALETTE_SIZE constant) before computing offsets and
writing to this.palette, and on violation either early-return (or throw/log an
error) to prevent out-of-bounds writes—apply this guard for both fill and border
writes so the Float32Array is never indexed past its length.

[coderabbitai]

Actionable comments posted: 3

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/client/ClientGameRunner.ts`:
- Around line 250-254: The injected DOM nodes (the canvas created by
createCanvas() with id "webgl-debug-canvas" and the inputOverlay element created
elsewhere) are never removed when stop() is called, causing stacked invisible
overlays/handlers; update ClientGameRunner construction to accept and store a
cleanup closure and implement stop() (or the runner's dispose path) to call that
closure which removes inputOverlay and glCanvas if they are still connected (use
element.isConnected or parentNode checks) and remove any event listeners; locate
the canvas created via createCanvas() and the inputOverlay variables and ensure
the cleanup closure is passed into the ClientGameRunner constructor and invoked
on stop/dispose so repeated enters don't leave stale DOM nodes or handlers.

In `@src/client/graphics/GameRenderer.ts`:
- Around line 319-321: The resize listener added in initialize() uses an
anonymous handler and can be added multiple times; change initialize() to
register a single named/bound handler (e.g., store this._onWindowResize =
this._onWindowResize || (() =>
this.transformHandler.updateCanvasBoundingRect())) and use that for
window.addEventListener("resize", this._onWindowResize) so duplicates are
avoided, and ensure you remove it where appropriate (e.g., in a dispose/destroy
method or before re-adding) via window.removeEventListener("resize",
this._onWindowResize); reference initialize(),
transformHandler.updateCanvasBoundingRect(),
window.addEventListener/removeEventListener, and the stored this._onWindowResize
handler to locate the changes.

In `@src/client/render/gl/passes/name-pass/text-program.ts`:
- Around line 127-141: The async img.onload in loadAtlas() can run after
dispose() and recreate atlasTex / set atlasReady; to fix, guard the onload
handler by checking a disposed flag (e.g., this.disposed) before creating the
texture and setting atlasReady, and ensure dispose() clears the pending load by
nulling img.onload or setting img.src = '' and deleting/disposing any existing
atlasTex; update loadAtlas() (and the other similar block at lines ~194-198) to
bail if this.disposed is true and have dispose() explicitly cancel the image
load and delete the GL texture.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 592343b9-88d2-44fd-87fc-7d44300f865c

📥 Commits

Reviewing files that changed from the base of the PR and between 7b1557b and 8a4b12c.

⛔ Files ignored due to path filters (7)

• resources/atlases/emoji-atlas.png is excluded by !**/*.png
• resources/atlases/flag-atlas.png is excluded by !**/*.png
• resources/atlases/fx-atlas.png is excluded by !**/*.png
• resources/atlases/icon-atlas.png is excluded by !**/*.png
• resources/atlases/msdf-atlas.png is excluded by !**/*.png
• resources/atlases/status-atlas.png is excluded by !**/*.png
• resources/atlases/unit-atlas.png is excluded by !**/*.png

📒 Files selected for processing (58)

• resources/atlases/emoji-atlas-meta.json
• resources/atlases/flag-atlas-meta.json
• resources/atlases/fx-atlas-meta.json
• resources/atlases/msdf-atlas.json
• resources/atlases/status-atlas-meta.json
• src/client/ClientGameRunner.ts
• src/client/Controller.ts
• src/client/InputHandler.ts
• src/client/TransformHandler.ts
• src/client/UIState.ts
• src/client/controllers/BuildPreviewController.ts
• src/client/controllers/WarshipSelectionController.ts
• src/client/graphics/GameRenderer.ts
• src/client/graphics/ProgressBar.ts
• src/client/graphics/layers/AlertFrame.ts
• src/client/graphics/layers/AttackingTroopsOverlay.ts
• src/client/graphics/layers/AttacksDisplay.ts
• src/client/graphics/layers/BuildMenu.ts
• src/client/graphics/layers/ChatDisplay.ts
• src/client/graphics/layers/ControlPanel.ts
• src/client/graphics/layers/EmojiTable.ts
• src/client/graphics/layers/EventsDisplay.ts
• src/client/graphics/layers/GameLeftSidebar.ts
• src/client/graphics/layers/GameRightSidebar.ts
• src/client/graphics/layers/HeadsUpMessage.ts
• src/client/graphics/layers/ImmunityTimer.ts
• src/client/graphics/layers/InGamePromo.ts
• src/client/graphics/layers/Leaderboard.ts
• src/client/graphics/layers/MainRadialMenu.ts
• src/client/graphics/layers/MultiTabModal.ts
• src/client/graphics/layers/PerformanceOverlay.ts
• src/client/graphics/layers/PlayerActionHandler.ts
• src/client/graphics/layers/PlayerInfoOverlay.ts
• src/client/graphics/layers/PlayerPanel.ts
• src/client/graphics/layers/RadialMenu.ts
• src/client/graphics/layers/RadialMenuElements.ts
• src/client/graphics/layers/ReplayPanel.ts
• src/client/graphics/layers/SendResourceModal.ts
• src/client/graphics/layers/SettingsModal.ts
• src/client/graphics/layers/SpawnTimer.ts
• src/client/graphics/layers/StructureDrawingUtils.ts
• src/client/graphics/layers/TeamStats.ts
• src/client/graphics/layers/UnitDisplay.ts
• src/client/graphics/layers/WinModal.ts
• src/client/graphics/ui/TextIndicator.ts
• src/client/render/gl/passes/conquest-popup-pass.ts
• src/client/render/gl/passes/fx-pass/fx-sprite-pass.ts
• src/client/render/gl/passes/name-pass/atlas-data.ts
• src/client/render/gl/passes/name-pass/debug-program.ts
• src/client/render/gl/passes/name-pass/icon-program.ts
• src/client/render/gl/passes/name-pass/status-icon-program.ts
• src/client/render/gl/passes/name-pass/text-program.ts
• src/client/render/gl/passes/radial-menu-pass.ts
• src/client/render/gl/passes/structure-level-pass.ts
• src/client/render/gl/passes/structure-pass.ts
• src/client/render/gl/passes/unit-pass.ts
• tests/InputHandler.test.ts
• tests/client/graphics/ProgressBar.test.ts

💤 Files with no reviewable changes (2)

• src/client/graphics/ProgressBar.ts
• src/client/graphics/layers/StructureDrawingUtils.ts

✅ Files skipped from review due to trivial changes (3)

• resources/atlases/fx-atlas-meta.json
• src/client/UIState.ts
• resources/atlases/flag-atlas-meta.json

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

• src/client/graphics/layers/ChatDisplay.ts
• src/client/graphics/layers/GameLeftSidebar.ts
• src/client/graphics/layers/ImmunityTimer.ts
• src/client/graphics/layers/PlayerPanel.ts
• src/client/graphics/layers/GameRightSidebar.ts
• src/client/graphics/layers/SettingsModal.ts
• src/client/graphics/layers/AlertFrame.ts
• src/client/graphics/layers/WinModal.ts
• src/client/graphics/layers/BuildMenu.ts
• src/client/render/gl/passes/name-pass/icon-program.ts
• src/client/render/gl/passes/name-pass/status-icon-program.ts
• src/client/graphics/layers/MultiTabModal.ts
• src/client/render/gl/passes/name-pass/atlas-data.ts
• src/client/graphics/layers/AttackingTroopsOverlay.ts
• src/client/graphics/layers/SpawnTimer.ts
• src/client/render/gl/passes/name-pass/debug-program.ts
• src/client/graphics/layers/PerformanceOverlay.ts
• src/client/graphics/layers/AttacksDisplay.ts
• src/client/graphics/layers/ReplayPanel.ts
• src/client/graphics/layers/ControlPanel.ts
• src/client/graphics/layers/PlayerInfoOverlay.ts
• src/client/controllers/BuildPreviewController.ts
• src/client/render/gl/passes/conquest-popup-pass.ts
• src/client/graphics/layers/TeamStats.ts
• src/client/controllers/WarshipSelectionController.ts
• src/client/graphics/layers/RadialMenu.ts
• src/client/render/gl/passes/fx-pass/fx-sprite-pass.ts

[coderabbitai]

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Clean up injected overlay/canvas nodes on stop.

Line 253 and Line 425 add persistent DOM nodes, but stop() never removes them. Re-entering games can stack invisible overlays/canvases and keep stale handlers alive.

Proposed direction

-function createWebGLView(...): { view: WebGLGameView; glCanvas: HTMLCanvasElement; ... } {
+function createWebGLView(...): { view: WebGLGameView; glCanvas: HTMLCanvasElement; ... } {
   ...
   document.body.insertBefore(glCanvas, document.body.firstChild);
   ...
 }

 export class ClientGameRunner {
+  private readonly cleanupDom: (() => void) | null;
   constructor(
     ...
     private webglBuilder: WebGLFrameBuilder | null = null,
+    cleanupDom: (() => void) | null = null,
   ) {
+    this.cleanupDom = cleanupDom;
     this.lastMessageTime = Date.now();
   }

   public stop() {
     ...
+    this.cleanupDom?.();
   }
 }

And when constructing ClientGameRunner, pass a cleanup closure that removes inputOverlay and glCanvas if still connected.

Also applies to: 417-425, 753-768

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/client/ClientGameRunner.ts` around lines 250 - 254, The injected DOM
nodes (the canvas created by createCanvas() with id "webgl-debug-canvas" and the
inputOverlay element created elsewhere) are never removed when stop() is called,
causing stacked invisible overlays/handlers; update ClientGameRunner
construction to accept and store a cleanup closure and implement stop() (or the
runner's dispose path) to call that closure which removes inputOverlay and
glCanvas if they are still connected (use element.isConnected or parentNode
checks) and remove any event listeners; locate the canvas created via
createCanvas() and the inputOverlay variables and ensure the cleanup closure is
passed into the ClientGameRunner constructor and invoked on stop/dispose so
repeated enters don't leave stale DOM nodes or handlers.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Prevent duplicate resize listeners on repeated initialize() calls.

Each call adds a new anonymous resize handler, which can stack listeners and duplicate work.

Suggested fix

 export class GameRenderer {
+  private initialized = false;
+  private readonly onResize = () =>
+    this.transformHandler.updateCanvasBoundingRect();
   private layerTickState = new Map<Controller, { lastTickAtMs: number }>();

   initialize() {
+    if (this.initialized) return;
+    this.initialized = true;
     this.layers.forEach((l) => l.init?.());

-    window.addEventListener("resize", () =>
-      this.transformHandler.updateCanvasBoundingRect(),
-    );
+    window.addEventListener("resize", this.onResize);

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/client/graphics/GameRenderer.ts` around lines 319 - 321, The resize
listener added in initialize() uses an anonymous handler and can be added
multiple times; change initialize() to register a single named/bound handler
(e.g., store this._onWindowResize = this._onWindowResize || (() =>
this.transformHandler.updateCanvasBoundingRect())) and use that for
window.addEventListener("resize", this._onWindowResize) so duplicates are
avoided, and ensure you remove it where appropriate (e.g., in a dispose/destroy
method or before re-adding) via window.removeEventListener("resize",
this._onWindowResize); reference initialize(),
transformHandler.updateCanvasBoundingRect(),
window.addEventListener/removeEventListener, and the stored this._onWindowResize
handler to locate the changes.

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Guard atlas async load against dispose-time race.

loadAtlas() can still run onload after dispose() is called, recreating a texture on a torn-down program lifecycle. This can leak GPU memory and flip ready back to true unexpectedly.

Proposed fix

 export class TextProgram {
+  private disposed = false;
+  private atlasImage: HTMLImageElement | null = null;
   private gl: WebGL2RenderingContext;
@@
   private loadAtlas(): void {
     const gl = this.gl;
     const img = new Image();
+    this.atlasImage = img;
     img.onload = () => {
+      if (this.disposed) return;
       const tex = gl.createTexture()!;
@@
       this.atlasTex = tex;
       this.atlasReady = true;
     };
+    img.onerror = () => {
+      if (this.disposed) return;
+      this.atlasReady = false;
+    };
     img.src = atlasUrl;
   }
@@
   dispose(): void {
+    this.disposed = true;
+    if (this.atlasImage) {
+      this.atlasImage.onload = null;
+      this.atlasImage.onerror = null;
+      this.atlasImage = null;
+    }
     const gl = this.gl;
     gl.deleteProgram(this.program);
     if (this.atlasTex) gl.deleteTexture(this.atlasTex);
+    this.atlasTex = null;
+    this.atlasReady = false;
   }
 }

Also applies to: 194-198

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/client/render/gl/passes/name-pass/text-program.ts` around lines 127 -
141, The async img.onload in loadAtlas() can run after dispose() and recreate
atlasTex / set atlasReady; to fix, guard the onload handler by checking a
disposed flag (e.g., this.disposed) before creating the texture and setting
atlasReady, and ensure dispose() clears the pending load by nulling img.onload
or setting img.src = '' and deleting/disposing any existing atlasTex; update
loadAtlas() (and the other similar block at lines ~194-198) to bail if
this.disposed is true and have dispose() explicitly cancel the image load and
delete the GL texture.

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

src/client/render/gl/passes/conquest-popup-pass.ts (1)

208-224: ⚡ Quick win

Add error handler for atlas image load.

If the MSDF atlas fails to load (network error, CDN issue, missing file), atlasReady stays false and popups silently never render. Adding an onerror handler helps with debugging.

Add error logging

 private loadAtlas(): void {
   const img = new Image();
   img.crossOrigin = "anonymous";
   img.onload = () => {
     const gl = this.gl;
     const tex = gl.createTexture()!;
     gl.bindTexture(gl.TEXTURE_2D, tex);
     gl.texParameteri(gl.TEXTURE_2D, gl.TEXTURE_MIN_FILTER, gl.LINEAR);
     gl.texParameteri(gl.TEXTURE_2D, gl.TEXTURE_MAG_FILTER, gl.LINEAR);
     gl.texParameteri(gl.TEXTURE_2D, gl.TEXTURE_WRAP_S, gl.CLAMP_TO_EDGE);
     gl.texParameteri(gl.TEXTURE_2D, gl.TEXTURE_WRAP_T, gl.CLAMP_TO_EDGE);
     gl.texImage2D(gl.TEXTURE_2D, 0, gl.RGBA, gl.RGBA, gl.UNSIGNED_BYTE, img);
     this.atlasTex = tex;
     this.atlasReady = true;
   };
+  img.onerror = () => {
+    console.error("ConquestPopupPass: failed to load MSDF atlas from", atlasUrl);
+  };
   img.src = atlasUrl;
 }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/client/render/gl/passes/conquest-popup-pass.ts` around lines 208 - 224,
The loadAtlas method doesn't handle image load failures; add an img.onerror
handler alongside img.onload to set this.atlasReady = false (or keep false) and
log the error so failures are visible; specifically update the loadAtlas
function to attach img.onerror that captures the error/event and calls
this.logger.error (or console.error) with context including atlasUrl and the
event, and ensure any partially created texture is cleaned up (using
this.atlasTex / gl.deleteTexture if created) so state remains consistent.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/client/render/gl/passes/conquest-popup-pass.ts`:
- Around line 58-62: The formatGold function currently returns hard-coded "K"
and "M" suffixes; update formatGold to obtain suffixes via translateText (e.g.,
translateText("suffix.k") and translateText("suffix.m")) so all user-visible
text goes through localization, and add the corresponding keys to your language
resources (e.g., English entries for the K and M suffixes) to ensure the popups
use localized abbreviations across locales.

---

Nitpick comments:
In `@src/client/render/gl/passes/conquest-popup-pass.ts`:
- Around line 208-224: The loadAtlas method doesn't handle image load failures;
add an img.onerror handler alongside img.onload to set this.atlasReady = false
(or keep false) and log the error so failures are visible; specifically update
the loadAtlas function to attach img.onerror that captures the error/event and
calls this.logger.error (or console.error) with context including atlasUrl and
the event, and ensure any partially created texture is cleaned up (using
this.atlasTex / gl.deleteTexture if created) so state remains consistent.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: b36bbeea-d073-4cd5-8df4-195ea4ef408d

📥 Commits

Reviewing files that changed from the base of the PR and between b27c298 and b8d72d3.

📒 Files selected for processing (10)

• src/client/graphics/layers/PerformanceOverlay.ts
• src/client/render/gl/passes/conquest-popup-pass.ts
• src/client/render/gl/passes/fx-pass/fx-sprite-pass.ts
• src/client/render/gl/passes/name-pass/icon-program.ts
• src/client/render/gl/passes/name-pass/status-icon-program.ts
• src/client/render/gl/passes/name-pass/text-program.ts
• src/client/render/gl/passes/radial-menu-pass.ts
• src/client/render/gl/passes/structure-level-pass.ts
• src/client/render/gl/passes/structure-pass.ts
• src/client/render/gl/passes/unit-pass.ts

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

• src/client/render/gl/passes/name-pass/text-program.ts
• src/client/render/gl/passes/name-pass/icon-program.ts
• src/client/render/gl/passes/fx-pass/fx-sprite-pass.ts
• src/client/render/gl/passes/name-pass/status-icon-program.ts

[coderabbitai]

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Number suffixes need localization.

The "K" and "M" suffixes are user-visible text in floating popups. Per coding guidelines, all user-visible text must go through translateText(). Some locales use different abbreviations (e.g., "mil" for million in some languages).

Suggested fix using translateText

+import { translateText } from "../../Utils";
+
 function formatGold(gold: number): string {
-  if (gold >= 1_000_000) return (gold / 1_000_000).toFixed(1) + "M";
-  if (gold >= 1_000) return (gold / 1_000).toFixed(1) + "K";
+  if (gold >= 1_000_000)
+    return (gold / 1_000_000).toFixed(1) + translateText("gold.suffix_million");
+  if (gold >= 1_000)
+    return (gold / 1_000).toFixed(1) + translateText("gold.suffix_thousand");
   return gold.toString();
 }

As per coding guidelines: "All user-visible text must go through translateText() function with corresponding entries in resources/lang/en.json"

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	function formatGold(gold: number): string {
	if (gold >= 1_000_000) return (gold / 1_000_000).toFixed(1) + "M";
	if (gold >= 1_000) return (gold / 1_000).toFixed(1) + "K";
	return gold.toString();
	}
	import { translateText } from "../../Utils";
	function formatGold(gold: number): string {
	if (gold >= 1_000_000)
	return (gold / 1_000_000).toFixed(1) + translateText("gold.suffix_million");
	if (gold >= 1_000)
	return (gold / 1_000).toFixed(1) + translateText("gold.suffix_thousand");
	return gold.toString();
	}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/client/render/gl/passes/conquest-popup-pass.ts` around lines 58 - 62, The
formatGold function currently returns hard-coded "K" and "M" suffixes; update
formatGold to obtain suffixes via translateText (e.g., translateText("suffix.k")
and translateText("suffix.m")) so all user-visible text goes through
localization, and add the corresponding keys to your language resources (e.g.,
English entries for the K and M suffixes) to ensure the popups use localized
abbreviations across locales.