Conversation
Contributor
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds skins as a first-class cosmetic across schemas, client caching and resolution, UI selection and preview (mutually exclusive with patterns), server flare-based authorization, and GPU atlas-based skin rendering with per-player anchors. ChangesPlayer Skin Cosmetics Feature
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server
participant GPURenderer
participant SkinAtlasArray
participant GPU
Client->>Server: fetchCosmetics()
Server-->>Client: cosmetics payload (includes skins)
Client->>Client: cache cosmetics
Client->>GPURenderer: setPlayerSkin(smallID, url)
GPURenderer->>SkinAtlasArray: request(url)
SkinAtlasArray-->>GPURenderer: onLayerReady(url, layer)
GPURenderer->>GPU: upload layer indices & anchors
GPURenderer->>GPU: draw (bind skin atlas, layer, anchors)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning Review ran into problems🔥 ProblemsStopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/client/render/gl/passes/TerritoryPass.ts (1)
411-448: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winPush timing into the pass instead of reading it here.
Reading
performance.now()insetHighlightOwner()anddraw()makes this pass pull external state on its own. Move current time and hover-start time into setters or anupdate()call fromGPURendererso the pass stays a pure consumer of pushed data. As per coding guidelines,src/client/render/gl/passes/*.ts: "Pass classes must cache uniform locations at construct time and never read DOM events or game state directly—they must be pure consumers of data pushed via setters".src/client/WebGLFrameBuilder.ts (1)
158-176:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSkin URLs stop syncing after the first sighting.
Once a
smallIDis added toknownSmallIDs, this loop exits before the skin branch runs, sosetPlayerSkin()is only called once. If the resolved skin arrives later or changes during the session, the renderer never gets the new URL. Track the last pushed skin URL per player, likelastSpawnTile, and resend on change.🤖 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 158 - 176, The syncPlayers loop currently early-continues when smallID is already in knownSmallIDs, preventing subsequent skin updates; modify syncPlayers (and add a smallID->lastSkinUrl map, e.g. lastSkinBySmallID) so that after computing flagUrl and skin (and before or instead of the continue) you compare skin?.url (resolved via assetUrl) with lastSkinBySmallID.get(smallID) and if different call this.view.setPlayerSkin(smallID, assetUrl(skin.url)) and update the map; keep the knownSmallIDs flow for palette/border writes (writePaletteEntry) but ensure skin handling always runs so updated or late-resolved skin URLs are resent even for previously seen smallIDs.
🤖 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/components/CosmeticButton.ts`:
- Around line 95-97: In CosmeticButton, the img alt currently uses the raw
cosmetic name (alt=${c.name}); update it to use the localization helper (e.g.,
alt={translateText(c.name)}) so all user-visible text goes through
translateText(), add the corresponding key/value to resources/lang/en.json, and
ensure translateText is imported into the CosmeticButton component if not
already imported.
In `@src/client/PatternInput.ts`:
- Around line 140-143: The alt text for the skin image uses raw this.skin.name
in previewContent inside PatternInput (previewContent = html`<img ...
alt=${this.skin.name}`) — replace that with a call to translateText() so
user-visible text is localized (e.g., alt={translateText(this.skin.name)}), and
add a corresponding entry key in the language resources; ensure you import/ use
the existing translateText helper used across src/client components and update
PatternInput to pass the translated string to the img alt attribute.
In `@src/client/render/gl/passes/SkinAtlasArray.ts`:
- Around line 109-124: The atlas currently only exposes a single global cell
size (this.cellW/this.cellH) to TerritoryPass via setSkinAtlas(tex, cellW,
cellH), so smaller images uploaded with entry.cellW/entry.cellH are rendered
stretched into the larger global cell; fix by recording per-layer dimensions
when uploading and sending them to the renderer: extend the atlas to store
per-layer size metadata (keyed by layer index used in uploadImage) and change
the API used by TerritoryPass (e.g., add setSkinAtlasLayer(layer, cellW, cellH)
or include a per-layer size map) so TerritoryPass gets the native
entry.cellW/entry.cellH for each layer instead of the single global
this.cellW/this.cellH; update calls that rely on setSkinAtlas, uploadImage,
resize, and any consumers in TerritoryPass to read per-layer dimensions.
- Around line 91-131: The request method currently reserves a layer
(this.nextLayer++) and inserts an entries map entry before the image loads,
causing failed loads to leak slots and block retries; change the flow so the
layer is allocated only after successful img.onload (move increment and entry
creation into the onload handler), or if you prefer to keep early insertion then
ensure img.onerror removes the entry from this.entries and decrements/recycles
this.nextLayer/capacity so the slot is reusable; update related logic that calls
this.resize, this.uploadImage, and this.onLayerReady to use the newly allocated
layer value created inside onload (or reclaimed on error) so failed URLs do not
permanently consume atlas layers.
In `@src/core/game/UserSettings.ts`:
- Around line 282-288: getSelectedSkinName currently returns empty or malformed
strings from storage which can leak invalid skin names; update
getSelectedSkinName (and use SKIN_KEY/getCached) so that after removing the
"skin:" prefix you validate the result and return null for any falsy or invalid
value (e.g., empty string or characters outside the allowed set); implement a
simple sanity check (non-empty and matching the project's skin-name pattern,
e.g., alphanumerics, hyphen/underscore) and return null when the check fails so
callers always receive either a valid skin name or null.
In `@src/server/Worker.ts`:
- Around line 431-433: Remove the hot-path console.log that prints
`console.log(\`!!! got cosmetic result: ${JSON.stringify(cosmeticResult)}\`)` in
the join flow; replace it with a structured, non-hot-path log (e.g., call an
existing logger such as this.logger.info or processLogger.debug) that emits
`cosmeticResult` as structured metadata (not a stringified payload) or drop the
log entirely if unnecessary — ensure you update the call site where
`cosmeticResult` is referenced so it no longer uses console.log and uses the
chosen structured logger at an appropriate level.
---
Outside diff comments:
In `@src/client/WebGLFrameBuilder.ts`:
- Around line 158-176: The syncPlayers loop currently early-continues when
smallID is already in knownSmallIDs, preventing subsequent skin updates; modify
syncPlayers (and add a smallID->lastSkinUrl map, e.g. lastSkinBySmallID) so that
after computing flagUrl and skin (and before or instead of the continue) you
compare skin?.url (resolved via assetUrl) with lastSkinBySmallID.get(smallID)
and if different call this.view.setPlayerSkin(smallID, assetUrl(skin.url)) and
update the map; keep the knownSmallIDs flow for palette/border writes
(writePaletteEntry) but ensure skin handling always runs so updated or
late-resolved skin URLs are resent even for previously seen smallIDs.
🪄 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: 9cef81b7-f4c8-4692-93d1-96298d9d9905
⛔ Files ignored due to path filters (1)
src/client/render/gl/shaders/map-overlay/territory.frag.glslis excluded by!**/*.glsl
📒 Files selected for processing (15)
src/client/Cosmetics.tssrc/client/PatternInput.tssrc/client/Store.tssrc/client/TerritoryPatternsModal.tssrc/client/WebGLFrameBuilder.tssrc/client/components/CosmeticButton.tssrc/client/render/gl/GameView.tssrc/client/render/gl/Renderer.tssrc/client/render/gl/passes/SkinAtlasArray.tssrc/client/render/gl/passes/TerritoryPass.tssrc/core/CosmeticSchemas.tssrc/core/Schemas.tssrc/core/game/UserSettings.tssrc/server/Privilege.tssrc/server/Worker.ts
Comment on lines
+95
to
+97
| src=${c.url} | ||
| alt=${c.name} | ||
| class="w-full h-full object-contain pointer-events-none" |
Contributor
There was a problem hiding this comment.
Translate skin image alt label instead of using raw cosmetic name.
Line [96] should use translated cosmetic text for localization consistency.
💡 Proposed fix
- return html`<img
+ return html`<img
src=${c.url}
- alt=${c.name}
+ alt=${translateCosmetic("territory_patterns.skin", c.name)}
class="w-full h-full object-contain pointer-events-none"
draggable="false"
loading="lazy"
/>`;📝 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
| src=${c.url} | |
| alt=${c.name} | |
| class="w-full h-full object-contain pointer-events-none" | |
| return html`<img | |
| src=${c.url} | |
| alt=${translateCosmetic("territory_patterns.skin", c.name)} | |
| class="w-full h-full object-contain pointer-events-none" | |
| draggable="false" | |
| loading="lazy" | |
| />`; |
🤖 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/components/CosmeticButton.ts` around lines 95 - 97, In
CosmeticButton, the img alt currently uses the raw cosmetic name
(alt=${c.name}); update it to use the localization helper (e.g.,
alt={translateText(c.name)}) so all user-visible text goes through
translateText(), add the corresponding key/value to resources/lang/en.json, and
ensure translateText is imported into the CosmeticButton component if not
already imported.
Comment on lines
+140
to
+143
| previewContent = html`<img | ||
| src=${this.skin.url} | ||
| alt=${this.skin.name} | ||
| class="pointer-events-none" |
Contributor
There was a problem hiding this comment.
Localize skin image alt text.
Line [142] uses raw this.skin.name; route this through translation helper to keep client text localization-consistent.
💡 Proposed fix
-import { getPlayerCosmetics } from "./Cosmetics";
+import { getPlayerCosmetics, translateCosmetic } from "./Cosmetics";
@@
- alt=${this.skin.name}
+ alt=${translateCosmetic("territory_patterns.skin", this.skin.name)}🤖 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/PatternInput.ts` around lines 140 - 143, The alt text for the skin
image uses raw this.skin.name in previewContent inside PatternInput
(previewContent = html`<img ... alt=${this.skin.name}`) — replace that with a
call to translateText() so user-visible text is localized (e.g.,
alt={translateText(this.skin.name)}), and add a corresponding entry key in the
language resources; ensure you import/ use the existing translateText helper
used across src/client components and update PatternInput to pass the translated
string to the img alt attribute.
Comment on lines
+282
to
+288
| /** Returns the bare skin name (no `skin:` prefix), or null if none selected. */ | ||
| getSelectedSkinName(): string | null { | ||
| const data = this.getCached(SKIN_KEY); | ||
| if (data === null) return null; | ||
| const skinPrefix = "skin:"; | ||
| return data.startsWith(skinPrefix) ? data.slice(skinPrefix.length) : data; | ||
| } |
Contributor
There was a problem hiding this comment.
Normalize invalid stored skin values before returning.
Line 287 can return "" (or malformed names) from localStorage. That can leak an invalid skinName into wire refs later. Return null for non-valid names to keep refs schema-safe.
Suggested fix
getSelectedSkinName(): string | null {
const data = this.getCached(SKIN_KEY);
if (data === null) return null;
const skinPrefix = "skin:";
- return data.startsWith(skinPrefix) ? data.slice(skinPrefix.length) : data;
+ const normalized = data.startsWith(skinPrefix)
+ ? data.slice(skinPrefix.length)
+ : data;
+ if (!/^[a-z0-9_]+$/.test(normalized)) return null;
+ return normalized;
}🤖 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/core/game/UserSettings.ts` around lines 282 - 288, getSelectedSkinName
currently returns empty or malformed strings from storage which can leak invalid
skin names; update getSelectedSkinName (and use SKIN_KEY/getCached) so that
after removing the "skin:" prefix you validate the result and return null for
any falsy or invalid value (e.g., empty string or characters outside the allowed
set); implement a simple sanity check (non-empty and matching the project's
skin-name pattern, e.g., alphanumerics, hyphen/underscore) and return null when
the check fails so callers always receive either a valid skin name or null.
Comment on lines
+431
to
+433
| console.log( | ||
| `!!! got cosmetic result: ${JSON.stringify(cosmeticResult)}`, | ||
| ); |
Contributor
There was a problem hiding this comment.
Remove hot-path debug console.log in join flow.
Line [431] logs every successful join and bypasses structured logging, which can create noisy production logs.
💡 Proposed fix
- console.log(
- `!!! got cosmetic result: ${JSON.stringify(cosmeticResult)}`,
- );📝 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
| console.log( | |
| `!!! got cosmetic result: ${JSON.stringify(cosmeticResult)}`, | |
| ); |
🤖 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/server/Worker.ts` around lines 431 - 433, Remove the hot-path console.log
that prints `console.log(\`!!! got cosmetic result:
${JSON.stringify(cosmeticResult)}\`)` in the join flow; replace it with a
structured, non-hot-path log (e.g., call an existing logger such as
this.logger.info or processLogger.debug) that emits `cosmeticResult` as
structured metadata (not a stringified payload) or drop the log entirely if
unnecessary — ensure you update the call site where `cosmeticResult` is
referenced so it no longer uses console.log and uses the chosen structured
logger at an appropriate level.
github-project-automation
Bot
moved this from Triage
to Development
in OpenFront Release Management
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/client/render/gl/passes/TerritoryPass.ts (1)
411-448: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winPush time into
TerritoryPassinstead of reading it here.
setHighlightOwner()anddraw()now callperformance.now()directly, so this pass is no longer a pure consumer of renderer state. PassnowSecondsand the hover-start timestamp in through setters/update and keepdraw()to cached values only.As per coding guidelines,
src/client/render/gl/passes/*.ts: Pass classes must cache uniform locations at construct time and never read DOM events or game state directly—they must be pure consumers of data pushed via setters.🤖 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/TerritoryPass.ts` around lines 411 - 448, The pass currently reads time via performance.now() inside setHighlightOwner() and draw(), violating the "push state in" rule; change this to accept pushed timestamps: stop calling performance.now() in setHighlightOwner and draw, add/set or update setters such as setHighlightOwner(ownerID: number, nowSeconds: number) or add setNow(nowSeconds: number) and setHoverStartTime(hoverStartTime: number) to store this.hoverStartTime and this.nowSeconds on the instance, and modify draw() to use the cached this.nowSeconds and this.hoverStartTime when uploading uTime and uHoverStartTime uniforms (retain existing uniform names uTime/uHoverStartTime/uHighlightOwner), ensuring all time reads are removed from draw() and setHighlightOwner().
🤖 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/Renderer.ts`:
- Around line 280-288: The on-layer-ready callback currently sets skinLayerCpu
for every smallID in pendingSkinPlayers[url] without checking if the player's
skin URL changed; update the loop in that callback to verify each player's
current skin URL (via the same player->url map used by setPlayerSkin, e.g.,
playerSkins.get(smallID) or the appropriate mapping) equals the url being
handled before writing this.skinLayerCpu[smallID] and calling
uploadSkinLayerTex; only then clear the entry for that smallID (and remove the
pending set if empty). This prevents stale async skin loads from overwriting
newer skins while keeping use of pendingSkinPlayers, skinLayerCpu,
uploadSkinLayerTex and setPlayerSkin intact.
In `@src/client/Store.ts`:
- Around line 240-243: The affiliate mixed-type filter that checks r.type for
"pattern", "skin", "flag", or "pack" currently shows the misleading empty-state
key store.no_skins; update the empty-state copy to a neutral key (e.g.,
store.no_items) or introduce a new i18n key like "no_items_available" and use
that in the same conditional/renderer where store.no_skins is referenced so
affiliate mixed-type results display a neutral “no items available” message
instead of "no_skins".
In `@src/client/TerritoryPatternsModal.ts`:
- Around line 125-137: The current selection check in the render logic (inside
TerritoryPatternsModal) marks the default pattern tile selected even when a skin
is active because selectedPattern is null; update the pattern branch so a
pattern is treated as selected only if no skin is active (this.selectedSkinName
=== null) or an explicit selectedPattern matches: adjust the r.type ===
"pattern" conditional that references this.selectedPattern,
this.selectedSkinName, and r.cosmetic (and r.colorPalette) so that the
default-pattern case requires this.selectedSkinName === null before returning
true; keep the existing equality checks for explicit selectedPattern matches.
In `@src/core/game/UserSettings.ts`:
- Around line 282-296: Add unit tests for the new UserSettings skin API:
exercise getSelectedSkinName and setSelectedSkinName to cover (1) when no value
is stored getSelectedSkinName() returns null, (2) when
setSelectedSkinName("foo") returns "foo", (3) when the stored value includes the
"skin:" prefix (simulate by using setCached or setSelectedSkinName if it accepts
prefixed values) getSelectedSkinName() strips the "skin:" prefix and returns the
bare name, and (4) clearing behavior: calling setSelectedSkinName(undefined) or
removeCached(SKIN_KEY) makes getSelectedSkinName() return null. Place tests
alongside other core tests under src/core/... (e.g., a new test file for
UserSettings) and reference the UserSettings class and its methods
getSelectedSkinName/setSelectedSkinName (and SKIN_KEY if needed) when asserting
expected stored and retrieved values.
---
Outside diff comments:
In `@src/client/render/gl/passes/TerritoryPass.ts`:
- Around line 411-448: The pass currently reads time via performance.now()
inside setHighlightOwner() and draw(), violating the "push state in" rule;
change this to accept pushed timestamps: stop calling performance.now() in
setHighlightOwner and draw, add/set or update setters such as
setHighlightOwner(ownerID: number, nowSeconds: number) or add setNow(nowSeconds:
number) and setHoverStartTime(hoverStartTime: number) to store
this.hoverStartTime and this.nowSeconds on the instance, and modify draw() to
use the cached this.nowSeconds and this.hoverStartTime when uploading uTime and
uHoverStartTime uniforms (retain existing uniform names
uTime/uHoverStartTime/uHighlightOwner), ensuring all time reads are removed from
draw() and setHighlightOwner().
🪄 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: f3aea32f-4f5c-4920-9c02-3b9e27755356
⛔ Files ignored due to path filters (1)
src/client/render/gl/shaders/map-overlay/territory.frag.glslis excluded by!**/*.glsl
📒 Files selected for processing (14)
src/client/Cosmetics.tssrc/client/PatternInput.tssrc/client/Store.tssrc/client/TerritoryPatternsModal.tssrc/client/WebGLFrameBuilder.tssrc/client/components/CosmeticButton.tssrc/client/render/gl/GameView.tssrc/client/render/gl/Renderer.tssrc/client/render/gl/passes/SkinAtlasArray.tssrc/client/render/gl/passes/TerritoryPass.tssrc/core/CosmeticSchemas.tssrc/core/Schemas.tssrc/core/game/UserSettings.tssrc/server/Privilege.ts
Comment on lines
+240
to
+243
| (r.type === "pattern" || | ||
| r.type === "skin" || | ||
| r.type === "flag" || | ||
| r.type === "pack") && |
Contributor
There was a problem hiding this comment.
Use neutral empty-state copy for affiliate mixed-type results.
This filter now includes patterns, skins, flags, and packs, but the empty state still uses store.no_skins, which is misleading in affiliate mode. Please switch to a neutral key/value (for example, “no items available”).
🤖 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/Store.ts` around lines 240 - 243, The affiliate mixed-type filter
that checks r.type for "pattern", "skin", "flag", or "pack" currently shows the
misleading empty-state key store.no_skins; update the empty-state copy to a
neutral key (e.g., store.no_items) or introduce a new i18n key like
"no_items_available" and use that in the same conditional/renderer where
store.no_skins is referenced so affiliate mixed-type results display a neutral
“no items available” message instead of "no_skins".
Comment on lines
+282
to
+296
| /** Returns the bare skin name (no `skin:` prefix), or null if none selected. */ | ||
| getSelectedSkinName(): string | null { | ||
| const data = this.getCached(SKIN_KEY); | ||
| if (data === null) return null; | ||
| const skinPrefix = "skin:"; | ||
| return data.startsWith(skinPrefix) ? data.slice(skinPrefix.length) : data; | ||
| } | ||
|
|
||
| setSelectedSkinName(skinName: string | undefined): void { | ||
| if (skinName === undefined) { | ||
| this.removeCached(SKIN_KEY); | ||
| } else { | ||
| this.setCached(SKIN_KEY, skinName); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Add tests for the new core skin setting behavior.
This adds new persisted core behavior (getSelectedSkinName/setSelectedSkinName) but no tests are included in the provided changes. Please add focused tests for valid/invalid stored values and clear/reset behavior.
As per coding guidelines: src/core/**/*.ts: “All changes to src/core/ must include tests”.
🤖 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/core/game/UserSettings.ts` around lines 282 - 296, Add unit tests for the
new UserSettings skin API: exercise getSelectedSkinName and setSelectedSkinName
to cover (1) when no value is stored getSelectedSkinName() returns null, (2)
when setSelectedSkinName("foo") returns "foo", (3) when the stored value
includes the "skin:" prefix (simulate by using setCached or setSelectedSkinName
if it accepts prefixed values) getSelectedSkinName() strips the "skin:" prefix
and returns the bare name, and (4) clearing behavior: calling
setSelectedSkinName(undefined) or removeCached(SKIN_KEY) makes
getSelectedSkinName() return null. Place tests alongside other core tests under
src/core/... (e.g., a new test file for UserSettings) and reference the
UserSettings class and its methods getSelectedSkinName/setSelectedSkinName (and
SKIN_KEY if needed) when asserting expected stored and retrieved values.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/client/render/gl/passes/TerritoryPass.ts (1)
412-448: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winPush time into
TerritoryPassinstead of reading globals here.
setHighlightOwner()anddraw()now readperformance.now()directly. That breaks the pass contract in this folder: this pass should only consume state pushed fromRenderer.As per coding guidelines, "Pass classes must cache uniform locations at construct time and never read DOM events or game state directly—they must be pure consumers of data pushed via setters."♻️ Suggested shape
- setHighlightOwner(ownerID: number): void { - if (ownerID !== this.highlightOwner && ownerID !== 0) { - this.hoverStartTime = performance.now() / 1000; - } + setHighlightOwner(ownerID: number, nowSec: number): void { + if (ownerID !== this.highlightOwner && ownerID !== 0) { + this.hoverStartTime = nowSec; + } this.highlightOwner = ownerID; } - draw(cameraMatrix: Float32Array): void { + draw(cameraMatrix: Float32Array, nowSec: number): void { @@ - gl.uniform1f(this.uTime, performance.now() / 1000); + gl.uniform1f(this.uTime, nowSec); gl.uniform1f(this.uHoverStartTime, this.hoverStartTime);
Renderercan computenowSeconce per frame and pass it into both calls.🤖 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/TerritoryPass.ts` around lines 412 - 448, The pass reads performance.now() inside setHighlightOwner() and draw() (setting hoverStartTime and uTime/uHoverStartTime), which violates the pass contract; change the API so the frame time is pushed in from Renderer: update setHighlightOwner(ownerID, nowSec) to accept nowSec and use that instead of performance.now() when updating hoverStartTime, and change draw(cameraMatrix, nowSec) to accept nowSec and use it to set the uTime and uHoverStartTime uniforms (and remove any direct performance.now() calls). Ensure you only change the signatures for TerritoryPass methods (setHighlightOwner and draw) and update callers in Renderer to compute nowSec once per frame and pass it through.
♻️ Duplicate comments (4)
src/client/PatternInput.ts (1)
131-135:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winLocalize the skin preview alt text.
Line [134] uses raw
this.skin.name. Please route alt text through cosmetic translation.💡 Suggested fix
-import { getPlayerCosmetics } from "./Cosmetics"; +import { getPlayerCosmetics, translateCosmetic } from "./Cosmetics"; @@ - alt=${this.skin.name} + alt=${translateCosmetic("territory_patterns.skin", this.skin.name)}As per coding guidelines:
src/client/**/*.{ts,tsx}requires “All user-visible text must go throughtranslateText()function with corresponding entries inresources/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/PatternInput.ts` around lines 131 - 135, The img alt currently uses raw this.skin.name; change it to pass the alt string through translateText (e.g., translateText(...)) so user-visible text is localized, update the PatternInput component to call translateText for the alt attribute (replace this.skin.name with translateText(...) where previewContent is built), ensure translateText is imported/available in PatternInput, and add the corresponding key/value in resources/lang/en.json (or adapt to use an interpolated key that includes the skin name if needed).src/client/TerritoryPatternsModal.ts (1)
111-123:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAvoid double-selected tiles when a skin is active.
Line [112] allows default pattern selection whenever
selectedPatternis null, even if a skin is selected.💡 Suggested fix
const isSelected = r.type === "pattern" - ? (r.cosmetic === null && this.selectedPattern === null) || - (r.cosmetic !== null && + ? (r.cosmetic === null && + this.selectedPattern === null && + this.selectedSkinName === null) || + (this.selectedSkinName === null && + r.cosmetic !== null && this.selectedPattern?.name === r.cosmetic.name && (this.selectedPattern?.colorPalette?.name ?? null) === (r.colorPalette?.name ?? 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/TerritoryPatternsModal.ts` around lines 111 - 123, The pattern-selection condition currently treats a default pattern (r.cosmetic === null) as selected whenever this.selectedPattern === null, which lets a pattern tile appear selected even if a skin is active; update the check in TerritoryPatternsModal so the default-pattern branch requires both this.selectedPattern === null AND this.selectedSkinName === null, i.e., change the (r.cosmetic === null && this.selectedPattern === null) clause to also verify this.selectedSkinName === null so a skin prevents the default pattern from being considered selected.src/client/components/CosmeticButton.ts (1)
91-94:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTranslate skin image alt label.
Line [93] uses raw
c.namefor alt text. Please use cosmetic translation for consistency.💡 Suggested fix
- alt=${c.name} + alt=${translateCosmetic("territory_patterns.skin", c.name)}As per coding guidelines:
src/client/**/*.{ts,tsx}requires “All user-visible text must go throughtranslateText()function with corresponding entries inresources/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/components/CosmeticButton.ts` around lines 91 - 94, The img alt currently uses the raw cosmetic name (c.name) in the CosmeticButton component; replace that with a call to translateText(...) so the alt is localizable (use the same translation key pattern you use elsewhere and add the corresponding entry to resources/lang/en.json), e.g., update the alt attribute in CosmeticButton to translateText for the cosmetic label and add the new key/value in resources/lang/en.json so all user-visible text goes through translateText().src/client/Store.ts (1)
247-252:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse neutral empty-state copy for affiliate mixed items.
Line [251] still shows
store.no_skins, but this grid now includes patterns, skins, flags, and packs.💡 Suggested fix
- ${translateText("store.no_skins")} + ${translateText("store.no_items")}(If
store.no_itemsdoes not exist yet, add it inresources/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/Store.ts` around lines 247 - 252, The empty-state copy currently uses translateText("store.no_skins") but this grid shows mixed item types; update the message key to a neutral one like translateText("store.no_items") in the conditional that returns the empty-state (the block where items.length === 0 in Store.ts) and, if the key doesn't exist, add "store.no_items" to resources/lang/en.json with appropriate neutral copy; ensure you only change the translation key reference and add the new entry to the locale file.
🤖 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/Cosmetics.ts`:
- Around line 539-565: The code currently can return a cached skinName even when
cosmetics is null; change the logic so skinName is only emitted when cosmetics
is available and validated: require cosmetics to be truthy before looking up
cosmetics.skins and before letting skinName remain set; if cosmetics is
null/undefined, force skinName = undefined (and call
userSettings.setSelectedPatternName(undefined) as already done when clearing
skins). Update the branch around the skinName/cosmetics check (symbols:
skinName, cosmetics, cosmetics?.skins, getUserMe(),
userSettings.setSelectedPatternName) to gate all skin resolution on cosmetics
being loaded and only return skinName when the skin exists and the user has the
appropriate flare.
In `@src/client/render/gl/passes/SkinAtlasArray.ts`:
- Around line 107-130: The onload/onerror handlers in SkinAtlasArray currently
run after dispose() and can call resize()/uploadImage() or onLayerReady, leaking
GPU resources; add a disposed guard and unbind handlers: set a boolean
this.disposed = true in dispose(), and in each image callback (the img.onload
blocks used around uploadImage/resize and the other occurrences at the noted
regions) first check if this.disposed and return early; also clear img.onload
and img.onerror (or removeEventListener if used) when disposing or immediately
after use to prevent late callbacks from reviving the atlas and invoking methods
like resize(), uploadImage(), or onLayerReady on a disposed object.
---
Outside diff comments:
In `@src/client/render/gl/passes/TerritoryPass.ts`:
- Around line 412-448: The pass reads performance.now() inside
setHighlightOwner() and draw() (setting hoverStartTime and
uTime/uHoverStartTime), which violates the pass contract; change the API so the
frame time is pushed in from Renderer: update setHighlightOwner(ownerID, nowSec)
to accept nowSec and use that instead of performance.now() when updating
hoverStartTime, and change draw(cameraMatrix, nowSec) to accept nowSec and use
it to set the uTime and uHoverStartTime uniforms (and remove any direct
performance.now() calls). Ensure you only change the signatures for
TerritoryPass methods (setHighlightOwner and draw) and update callers in
Renderer to compute nowSec once per frame and pass it through.
---
Duplicate comments:
In `@src/client/components/CosmeticButton.ts`:
- Around line 91-94: The img alt currently uses the raw cosmetic name (c.name)
in the CosmeticButton component; replace that with a call to translateText(...)
so the alt is localizable (use the same translation key pattern you use
elsewhere and add the corresponding entry to resources/lang/en.json), e.g.,
update the alt attribute in CosmeticButton to translateText for the cosmetic
label and add the new key/value in resources/lang/en.json so all user-visible
text goes through translateText().
In `@src/client/PatternInput.ts`:
- Around line 131-135: The img alt currently uses raw this.skin.name; change it
to pass the alt string through translateText (e.g., translateText(...)) so
user-visible text is localized, update the PatternInput component to call
translateText for the alt attribute (replace this.skin.name with
translateText(...) where previewContent is built), ensure translateText is
imported/available in PatternInput, and add the corresponding key/value in
resources/lang/en.json (or adapt to use an interpolated key that includes the
skin name if needed).
In `@src/client/Store.ts`:
- Around line 247-252: The empty-state copy currently uses
translateText("store.no_skins") but this grid shows mixed item types; update the
message key to a neutral one like translateText("store.no_items") in the
conditional that returns the empty-state (the block where items.length === 0 in
Store.ts) and, if the key doesn't exist, add "store.no_items" to
resources/lang/en.json with appropriate neutral copy; ensure you only change the
translation key reference and add the new entry to the locale file.
In `@src/client/TerritoryPatternsModal.ts`:
- Around line 111-123: The pattern-selection condition currently treats a
default pattern (r.cosmetic === null) as selected whenever this.selectedPattern
=== null, which lets a pattern tile appear selected even if a skin is active;
update the check in TerritoryPatternsModal so the default-pattern branch
requires both this.selectedPattern === null AND this.selectedSkinName === null,
i.e., change the (r.cosmetic === null && this.selectedPattern === null) clause
to also verify this.selectedSkinName === null so a skin prevents the default
pattern from being considered selected.
🪄 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: cb4dd99b-a246-4dbc-a1be-fab5c33281be
⛔ Files ignored due to path filters (1)
src/client/render/gl/shaders/map-overlay/territory.frag.glslis excluded by!**/*.glsl
📒 Files selected for processing (14)
src/client/Cosmetics.tssrc/client/PatternInput.tssrc/client/Store.tssrc/client/TerritoryPatternsModal.tssrc/client/WebGLFrameBuilder.tssrc/client/components/CosmeticButton.tssrc/client/render/gl/GameView.tssrc/client/render/gl/Renderer.tssrc/client/render/gl/passes/SkinAtlasArray.tssrc/client/render/gl/passes/TerritoryPass.tssrc/core/CosmeticSchemas.tssrc/core/Schemas.tssrc/core/game/UserSettings.tssrc/server/Privilege.ts
Comment on lines
+539
to
565
| let skinName = userSettings.getSelectedSkinName() ?? undefined; | ||
| if (skinName) { | ||
| const skin = cosmetics?.skins?.[skinName]; | ||
| if (cosmetics && !skin) { | ||
| // Cosmetics loaded but the saved skin no longer exists. | ||
| skinName = undefined; | ||
| } else if (skin) { | ||
| const userMe = await getUserMe(); | ||
| if (userMe) { | ||
| const flares = userMe.player.flares ?? []; | ||
| const hasWildcard = flares.includes("skin:*"); | ||
| if (!hasWildcard && !flares.includes(`skin:${skin.name}`)) { | ||
| skinName = undefined; | ||
| } | ||
| } | ||
| } | ||
| if (skinName === undefined) { | ||
| userSettings.setSelectedPatternName(undefined); | ||
| } | ||
| } | ||
|
|
||
| return { | ||
| flag: flag ?? undefined, | ||
| patternName: pattern?.name ?? undefined, | ||
| patternColorPaletteName: pattern?.colorPalette?.name ?? undefined, | ||
| skinName, | ||
| }; |
Contributor
There was a problem hiding this comment.
Do not emit skinName when cosmetics catalog is unavailable.
When fetchCosmetics() returns null, this branch can still return a locally cached skinName without validation. That can produce avoidable server-side rejection for stale/invalid skin refs. Gate skin refs on loaded cosmetics (same safety model as pattern resolution).
Suggested fix
- let skinName = userSettings.getSelectedSkinName() ?? undefined;
- if (skinName) {
+ let skinName: string | undefined;
+ if (cosmetics) {
+ skinName = userSettings.getSelectedSkinName() ?? undefined;
+ }
+ if (skinName && cosmetics) {
const skin = cosmetics?.skins?.[skinName];
if (cosmetics && !skin) {
// Cosmetics loaded but the saved skin no longer exists.
skinName = undefined;
} else if (skin) {
const userMe = await getUserMe();
if (userMe) {
const flares = userMe.player.flares ?? [];
const hasWildcard = flares.includes("skin:*");
if (!hasWildcard && !flares.includes(`skin:${skin.name}`)) {
skinName = undefined;
}
}
}
if (skinName === undefined) {
userSettings.setSelectedPatternName(undefined);
}
}🤖 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/Cosmetics.ts` around lines 539 - 565, The code currently can
return a cached skinName even when cosmetics is null; change the logic so
skinName is only emitted when cosmetics is available and validated: require
cosmetics to be truthy before looking up cosmetics.skins and before letting
skinName remain set; if cosmetics is null/undefined, force skinName = undefined
(and call userSettings.setSelectedPatternName(undefined) as already done when
clearing skins). Update the branch around the skinName/cosmetics check (symbols:
skinName, cosmetics, cosmetics?.skins, getUserMe(),
userSettings.setSelectedPatternName) to gate all skin resolution on cosmetics
being loaded and only return skinName when the skin exists and the user has the
appropriate flare.
Comment on lines
+107
to
+130
| img.onload = () => { | ||
| entry.img = img; | ||
| entry.cellW = Math.min(img.naturalWidth, MAX_SKIN_DIM); | ||
| entry.cellH = Math.min(img.naturalHeight, MAX_SKIN_DIM); | ||
|
|
||
| // Either the cell size or the capacity may be too small — resize once. | ||
| const needW = Math.max(this.cellW, entry.cellW); | ||
| const needH = Math.max(this.cellH, entry.cellH); | ||
| const needCap = Math.max(this.capacity, layer + 1); | ||
| if ( | ||
| needW !== this.cellW || | ||
| needH !== this.cellH || | ||
| needCap !== this.capacity | ||
| ) { | ||
| this.resize(needW, needH, needCap); | ||
| } | ||
|
|
||
| this.uploadImage(img, layer, entry.cellW, entry.cellH); | ||
| entry.ready = true; | ||
| this.onLayerReady(url, layer); | ||
| }; | ||
| img.onerror = () => { | ||
| console.warn("Skin image failed to load:", url); | ||
| }; |
Contributor
There was a problem hiding this comment.
Stop late image loads from reviving a disposed atlas.
After dispose(), a pending img.onload can still call resize() / uploadImage() and fire callbacks into a dead renderer. That leaks GPU objects during teardown or context loss.
🛠️ Minimal guard
export class SkinAtlasArray {
+ private disposed = false;
+
private gl: WebGL2RenderingContext;
private tex: WebGLTexture;
@@
const img = new Image();
img.crossOrigin = "anonymous";
img.onload = () => {
+ if (this.disposed) return;
entry.img = img;
entry.cellW = Math.min(img.naturalWidth, MAX_SKIN_DIM);
entry.cellH = Math.min(img.naturalHeight, MAX_SKIN_DIM);
@@
};
img.onerror = () => {
+ if (this.disposed) return;
console.warn("Skin image failed to load:", url);
};
img.src = url;
}
@@
private uploadImage(
img: HTMLImageElement,
layer: number,
imgW: number,
imgH: number,
): void {
+ if (this.disposed) return;
// Canvas is sized to atlas cell; clear, then draw the image at its native
@@
private resize(cellW: number, cellH: number, layers: number): void {
+ if (this.disposed) return;
this.gl.deleteTexture(this.tex);
@@
dispose(): void {
+ this.disposed = true;
this.gl.deleteTexture(this.tex);
this.entries.clear();
}
}Also applies to: 169-186, 211-214
🤖 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/SkinAtlasArray.ts` around lines 107 - 130, The
onload/onerror handlers in SkinAtlasArray currently run after dispose() and can
call resize()/uploadImage() or onLayerReady, leaking GPU resources; add a
disposed guard and unbind handlers: set a boolean this.disposed = true in
dispose(), and in each image callback (the img.onload blocks used around
uploadImage/resize and the other occurrences at the noted regions) first check
if this.disposed and return early; also clear img.onload and img.onerror (or
removeEventListener if used) when disposing or immediately after use to prevent
late callbacks from reviving the atlas and invoking methods like resize(),
uploadImage(), or onLayerReady on a disposed object.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
src/client/render/gl/Renderer.ts (1)
280-288:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard stale async skin completions.
If the same player switches skins before the first PNG finishes loading, the old URL stays in
pendingSkinPlayers, and its later callback can overwrite the newer choice. Track the latest requested URL persmallIDand ignore/remove stale waiters before writingskinLayerCpu.Also applies to: 736-749
🤖 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/Renderer.ts` around lines 280 - 288, The callback that processes completed skin PNGs can apply an old URL's result after a player has requested a new skin; to fix, keep a per-player latest-request tracking (e.g., a Map from smallID to the most recently requested URL) and, inside the completion handler that iterates the Set from pendingSkinPlayers, check that the completed URL matches the tracked latest URL for each smallID before writing this.skinLayerCpu[smallID] and calling this.uploadSkinLayerTex(); also remove/ignore stale smallIDs from the waiting set (and update the latest map when initiating requests). Ensure the same guard is added for the other similar block around lines 736-749.
🤖 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/TerritoryPass.ts`:
- Around line 412-415: The pass is reading performance.now() directly which
violates the rule that TERRITORY_PASS classes must only consume externally
pushed state; instead add a renderer-driven setter to push the current time into
TerritoryPass (e.g. setNow(nowSeconds:number) or setClockTime), store it on the
instance (hoverStartTime or a separate clockNow field), and replace all direct
performance.now() uses in setHighlightOwner() and draw() (and the occurrences
around lines 447–448) with the stored field; ensure
setHighlightOwner(ownerID:number) updates hoverStartTime using the stored clock
value when owner changes and all timing logic in draw() reads the injected clock
field.
In `@src/client/render/gl/Renderer.ts`:
- Around line 278-291: The atlas callbacks can run after the renderer is
disposed and access freed GL objects; add a disposed guard to the callbacks
registered with SkinAtlasArray (the two arrow callbacks passed when constructing
this.skinAtlas) so they return early if this.disposed (or equivalent) is true,
and/or ensure SkinAtlasArray.dispose() suppresses further callbacks; update the
two callbacks (the onLayerReady that uses this.pendingSkinPlayers,
this.skinLayerCpu, uploadSkinLayerTex and the onAtlasChanged that calls
this.territoryPass.setSkinAtlas) to check and return immediately when the
renderer is disposed.
- Around line 736-749: In Renderer.setPlayerSkin, clear the previous skin layer
immediately when starting an async load: set skinLayerCpu[smallID] to 0 (and
call uploadSkinLayerTex() if necessary) before calling skinAtlas.request(url) so
the old skin is not shown while decoding; then proceed to check
skinAtlas.getLayer(url) and either set skinLayerCpu[smallID] = layer + 1 or add
smallID to pendingSkinPlayers as currently implemented.
In `@src/core/Schemas.ts`:
- Around line 541-555: Add unit tests that exercise the new skin-related schemas
to lock behavior: create tests that validate CosmeticNameSchema (including
optional skinName usage), PlayerSkinSchema (valid name/url and failure cases),
and PlayerCosmeticsSchema (verify flag, pattern, color, and skin accept optional
values and that invalid shapes are rejected). Reference the schema symbols
PlayerSkinSchema, PlayerCosmeticsSchema (and its .shape.skin),
CosmeticNameSchema, PlayerPatternSchema, PlayerColorSchema, and FlagSchema when
constructing test cases; assert both successful parses and expected zod errors
for malformed inputs. Place tests alongside existing core schema tests for
src/core so changes to core are covered.
---
Duplicate comments:
In `@src/client/render/gl/Renderer.ts`:
- Around line 280-288: The callback that processes completed skin PNGs can apply
an old URL's result after a player has requested a new skin; to fix, keep a
per-player latest-request tracking (e.g., a Map from smallID to the most
recently requested URL) and, inside the completion handler that iterates the Set
from pendingSkinPlayers, check that the completed URL matches the tracked latest
URL for each smallID before writing this.skinLayerCpu[smallID] and calling
this.uploadSkinLayerTex(); also remove/ignore stale smallIDs from the waiting
set (and update the latest map when initiating requests). Ensure the same guard
is added for the other similar block around lines 736-749.
🪄 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: 05ad55a3-8dcd-4e5a-b8e3-a7db235bdb27
⛔ Files ignored due to path filters (1)
src/client/render/gl/shaders/map-overlay/territory.frag.glslis excluded by!**/*.glsl
📒 Files selected for processing (15)
src/client/Cosmetics.tssrc/client/PatternInput.tssrc/client/Store.tssrc/client/TerritoryPatternsModal.tssrc/client/WebGLFrameBuilder.tssrc/client/components/CosmeticButton.tssrc/client/render/gl/GameView.tssrc/client/render/gl/Renderer.tssrc/client/render/gl/passes/SkinAtlasArray.tssrc/client/render/gl/passes/TerritoryPass.tssrc/core/CosmeticSchemas.tssrc/core/Schemas.tssrc/core/game/UserSettings.tssrc/server/Privilege.tstests/Privilege.test.ts
Comment on lines
+412
to
+415
| if (ownerID !== this.highlightOwner && ownerID !== 0) { | ||
| // Reset scroll origin so skin begins panning from offset 0 on each hover. | ||
| this.hoverStartTime = performance.now() / 1000; | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Push time into TerritoryPass via setters; avoid reading performance.now() inside the pass.
This pass should consume externally pushed state only. Move clock values to renderer-driven setters and use stored fields in setHighlightOwner()/draw().
♻️ Minimal refactor sketch
+ private timeSeconds = 0;
+
+ setTimeSeconds(timeSeconds: number): void {
+ this.timeSeconds = timeSeconds;
+ }
setHighlightOwner(ownerID: number): void {
if (ownerID !== this.highlightOwner && ownerID !== 0) {
- this.hoverStartTime = performance.now() / 1000;
+ this.hoverStartTime = this.timeSeconds;
}
this.highlightOwner = ownerID;
}
draw(cameraMatrix: Float32Array): void {
@@
- gl.uniform1f(this.uTime, performance.now() / 1000);
+ gl.uniform1f(this.uTime, this.timeSeconds);
gl.uniform1f(this.uHoverStartTime, this.hoverStartTime);As per coding guidelines, src/client/render/gl/passes/*.ts pass classes “must be pure consumers of data pushed via setters.”
Also applies to: 447-448
🤖 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/TerritoryPass.ts` around lines 412 - 415, The
pass is reading performance.now() directly which violates the rule that
TERRITORY_PASS classes must only consume externally pushed state; instead add a
renderer-driven setter to push the current time into TerritoryPass (e.g.
setNow(nowSeconds:number) or setClockTime), store it on the instance
(hoverStartTime or a separate clockNow field), and replace all direct
performance.now() uses in setHighlightOwner() and draw() (and the occurrences
around lines 447–448) with the stored field; ensure
setHighlightOwner(ownerID:number) updates hoverStartTime using the stored clock
value when owner changes and all timing logic in draw() reads the injected clock
field.
Comment on lines
+736
to
+749
| setPlayerSkin(smallID: number, url: string): void { | ||
| this.skinAtlas.request(url); | ||
| const layer = this.skinAtlas.getLayer(url); | ||
| if (layer >= 0) { | ||
| this.skinLayerCpu[smallID] = layer + 1; | ||
| this.uploadSkinLayerTex(); | ||
| } else { | ||
| let set = this.pendingSkinPlayers.get(url); | ||
| if (!set) { | ||
| set = new Set(); | ||
| this.pendingSkinPlayers.set(url, set); | ||
| } | ||
| set.add(smallID); | ||
| } |
Contributor
There was a problem hiding this comment.
Clear the old layer while the new skin is still loading.
The doc says this path writes 0 immediately, but the implementation leaves the previous skinLayerCpu[smallID] active until decode finishes. That shows the wrong skin during async loads.
💡 Minimal fix
setPlayerSkin(smallID: number, url: string): void {
this.skinAtlas.request(url);
const layer = this.skinAtlas.getLayer(url);
if (layer >= 0) {
this.skinLayerCpu[smallID] = layer + 1;
this.uploadSkinLayerTex();
} else {
+ this.skinLayerCpu[smallID] = 0;
+ this.uploadSkinLayerTex();
let set = this.pendingSkinPlayers.get(url);
if (!set) {
set = new Set();
this.pendingSkinPlayers.set(url, set);
}📝 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
| setPlayerSkin(smallID: number, url: string): void { | |
| this.skinAtlas.request(url); | |
| const layer = this.skinAtlas.getLayer(url); | |
| if (layer >= 0) { | |
| this.skinLayerCpu[smallID] = layer + 1; | |
| this.uploadSkinLayerTex(); | |
| } else { | |
| let set = this.pendingSkinPlayers.get(url); | |
| if (!set) { | |
| set = new Set(); | |
| this.pendingSkinPlayers.set(url, set); | |
| } | |
| set.add(smallID); | |
| } | |
| setPlayerSkin(smallID: number, url: string): void { | |
| this.skinAtlas.request(url); | |
| const layer = this.skinAtlas.getLayer(url); | |
| if (layer >= 0) { | |
| this.skinLayerCpu[smallID] = layer + 1; | |
| this.uploadSkinLayerTex(); | |
| } else { | |
| this.skinLayerCpu[smallID] = 0; | |
| this.uploadSkinLayerTex(); | |
| let set = this.pendingSkinPlayers.get(url); | |
| if (!set) { | |
| set = new Set(); | |
| this.pendingSkinPlayers.set(url, set); | |
| } | |
| set.add(smallID); | |
| } | |
| } |
🤖 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/Renderer.ts` around lines 736 - 749, In
Renderer.setPlayerSkin, clear the previous skin layer immediately when starting
an async load: set skinLayerCpu[smallID] to 0 (and call uploadSkinLayerTex() if
necessary) before calling skinAtlas.request(url) so the old skin is not shown
while decoding; then proceed to check skinAtlas.getLayer(url) and either set
skinLayerCpu[smallID] = layer + 1 or add smallID to pendingSkinPlayers as
currently implemented.
Comment on lines
+541
to
555
| skinName: CosmeticNameSchema.optional(), | ||
| }); | ||
|
|
||
| export const PlayerSkinSchema = z.object({ | ||
| name: CosmeticNameSchema, | ||
| url: z.string(), | ||
| }); | ||
|
|
||
| // Server converts refs to the actual cosmetics here | ||
| export const PlayerCosmeticsSchema = z.object({ | ||
| flag: FlagSchema.optional(), | ||
| pattern: PlayerPatternSchema.optional(), | ||
| color: PlayerColorSchema.optional(), | ||
| skin: PlayerSkinSchema.optional(), | ||
| }); |
Contributor
There was a problem hiding this comment.
Add tests for new core skin schema paths.
Line 541 through Line 555 changes core wire/schema contracts (skinName, PlayerSkinSchema, PlayerCosmeticsSchema.skin) but there are no core tests in this change set to lock behavior.
As per coding guidelines: src/core/**/*.ts: “All changes to src/core/ must include tests”.
🤖 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/core/Schemas.ts` around lines 541 - 555, Add unit tests that exercise the
new skin-related schemas to lock behavior: create tests that validate
CosmeticNameSchema (including optional skinName usage), PlayerSkinSchema (valid
name/url and failure cases), and PlayerCosmeticsSchema (verify flag, pattern,
color, and skin accept optional values and that invalid shapes are rejected).
Reference the schema symbols PlayerSkinSchema, PlayerCosmeticsSchema (and its
.shape.skin), CosmeticNameSchema, PlayerPatternSchema, PlayerColorSchema, and
FlagSchema when constructing test cases; assert both successful parses and
expected zod errors for malformed inputs. Place tests alongside existing core
schema tests for src/core so changes to core are covered.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/server/Privilege.ts (1)
177-217:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEnforce server-side mutual exclusivity for
patternNameandskinName.
isAllowed()currently permits both fields together, which breaks the single-slot cosmetic contract. Reject requests when both are present.Suggested fix
isAllowed(flares: string[], refs: PlayerCosmeticRefs): CosmeticResult { + if (refs.patternName && refs.skinName) { + return { + type: "forbidden", + reason: "invalid cosmetics: pattern and skin are mutually exclusive", + }; + } + const cosmetics: PlayerCosmetics = {};🤖 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/server/Privilege.ts` around lines 177 - 217, In isAllowed (Privilege.ts) add a server-side check that rejects requests when both refs.patternName and refs.skinName are provided: detect the mutual exclusivity violation at the start of isAllowed and return a CosmeticResult of type "forbidden" with an appropriate reason like "pattern and skin cannot be used together"; keep the existing validations for pattern, skin, color, flag unchanged and referenced by the same methods isPatternAllowed, isSkinAllowed, isColorAllowed, isFlagAllowed.
♻️ Duplicate comments (4)
src/client/TerritoryPatternsModal.ts (1)
111-123:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPrevent default pattern from being selected while a skin is active.
The pattern-selection branch ignores
selectedSkinName, so both a skin tile and the default pattern tile can appear selected.Proposed fix
const isSelected = r.type === "pattern" - ? (r.cosmetic === null && this.selectedPattern === null) || - (r.cosmetic !== null && - this.selectedPattern?.name === r.cosmetic.name && - (this.selectedPattern?.colorPalette?.name ?? null) === - (r.colorPalette?.name ?? null)) + ? this.selectedSkinName === null && + ((r.cosmetic === null && this.selectedPattern === null) || + (r.cosmetic !== null && + this.selectedPattern?.name === r.cosmetic.name && + (this.selectedPattern?.colorPalette?.name ?? null) === + (r.colorPalette?.name ?? 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/TerritoryPatternsModal.ts` around lines 111 - 123, The pattern-selection logic in the r.type === "pattern" branch doesn't account for an active skin, so the default pattern (r.cosmetic === null) can be marked selected even when this.selectedSkinName is non-null; change the condition in TerritoryPatternsModal that determines selected state (the ternary checking r.type === "pattern") to require this.selectedSkinName === null when evaluating the default pattern case (i.e., only treat r.cosmetic === null as selected if this.selectedSkinName is also null), keeping the existing comparisons for non-null r.cosmetic and preserving the skin branch (the IIFE) as-is.src/client/components/CosmeticButton.ts (1)
91-94:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTranslate the skin image alt label.
Line [93] uses raw
c.nameinalt; use the existing cosmetic translation helper.Proposed fix
- alt=${c.name} + alt=${translateCosmetic("territory_patterns.pattern", c.name)}As per coding guidelines:
src/client/**/*.{ts,tsx}: “All user-visible text must go throughtranslateText()function with corresponding entries inresources/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/components/CosmeticButton.ts` around lines 91 - 94, The img alt currently uses raw c.name; update it to pass through the cosmetic translation helper (translateText) so alt uses the localized label instead. Locate the img in the CosmeticButton component (the element using src=${c.url} and alt=${c.name}) and replace alt=${c.name} with a call to the existing translation helper (e.g., translateText(c.name) or the project's cosmetic translation helper) and ensure the translation key is wired into resources/lang/en.json.src/client/PatternInput.ts (1)
132-135:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winLocalize the skin preview alt text.
Line [134] uses raw
this.skin.nameforalt; route it through the cosmetic translation helper.Proposed fix
-import { getPlayerCosmetics } from "./Cosmetics"; +import { getPlayerCosmetics, translateCosmetic } from "./Cosmetics"; @@ - alt=${this.skin.name} + alt=${translateCosmetic("territory_patterns.pattern", this.skin.name)}As per coding guidelines:
src/client/**/*.{ts,tsx}: “All user-visible text must go throughtranslateText()function with corresponding entries inresources/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/PatternInput.ts` around lines 132 - 135, The alt attribute for the skin preview in PatternInput (where previewContent is set) currently uses this.skin.name directly; change it to pass the name through the cosmetic translation helper by calling translateText(this.skin.name) (or the project’s cosmetic wrapper) so user-visible text is localized; update the previewContent construction in PatternInput.ts (the img src/alt block) to use translateText(this.skin.name) and ensure a matching entry exists in resources/lang/en.json.src/client/Store.ts (1)
247-252:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse neutral empty-state copy for affiliate mixed-type results.
This grid now mixes multiple cosmetic types, so
store.no_skinsis misleading.Proposed fix
- ${translateText("store.no_skins")} + ${translateText("store.no_items")}🤖 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/Store.ts` around lines 247 - 252, The empty-state message currently uses the misleading key "store.no_skins" when rendering the grid for items in Store.ts; change it to a neutral key (e.g., "store.no_items" or "store.no_results") and update the translateText call in the same return branch so the message is appropriate for mixed cosmetic types—locate the conditional that checks items.length === 0 in the Store component and replace translateText("store.no_skins") with the new neutral translation key (and add the new key to your i18n resource files).
🤖 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.
Outside diff comments:
In `@src/server/Privilege.ts`:
- Around line 177-217: In isAllowed (Privilege.ts) add a server-side check that
rejects requests when both refs.patternName and refs.skinName are provided:
detect the mutual exclusivity violation at the start of isAllowed and return a
CosmeticResult of type "forbidden" with an appropriate reason like "pattern and
skin cannot be used together"; keep the existing validations for pattern, skin,
color, flag unchanged and referenced by the same methods isPatternAllowed,
isSkinAllowed, isColorAllowed, isFlagAllowed.
---
Duplicate comments:
In `@src/client/components/CosmeticButton.ts`:
- Around line 91-94: The img alt currently uses raw c.name; update it to pass
through the cosmetic translation helper (translateText) so alt uses the
localized label instead. Locate the img in the CosmeticButton component (the
element using src=${c.url} and alt=${c.name}) and replace alt=${c.name} with a
call to the existing translation helper (e.g., translateText(c.name) or the
project's cosmetic translation helper) and ensure the translation key is wired
into resources/lang/en.json.
In `@src/client/PatternInput.ts`:
- Around line 132-135: The alt attribute for the skin preview in PatternInput
(where previewContent is set) currently uses this.skin.name directly; change it
to pass the name through the cosmetic translation helper by calling
translateText(this.skin.name) (or the project’s cosmetic wrapper) so
user-visible text is localized; update the previewContent construction in
PatternInput.ts (the img src/alt block) to use translateText(this.skin.name) and
ensure a matching entry exists in resources/lang/en.json.
In `@src/client/Store.ts`:
- Around line 247-252: The empty-state message currently uses the misleading key
"store.no_skins" when rendering the grid for items in Store.ts; change it to a
neutral key (e.g., "store.no_items" or "store.no_results") and update the
translateText call in the same return branch so the message is appropriate for
mixed cosmetic types—locate the conditional that checks items.length === 0 in
the Store component and replace translateText("store.no_skins") with the new
neutral translation key (and add the new key to your i18n resource files).
In `@src/client/TerritoryPatternsModal.ts`:
- Around line 111-123: The pattern-selection logic in the r.type === "pattern"
branch doesn't account for an active skin, so the default pattern (r.cosmetic
=== null) can be marked selected even when this.selectedSkinName is non-null;
change the condition in TerritoryPatternsModal that determines selected state
(the ternary checking r.type === "pattern") to require this.selectedSkinName ===
null when evaluating the default pattern case (i.e., only treat r.cosmetic ===
null as selected if this.selectedSkinName is also null), keeping the existing
comparisons for non-null r.cosmetic and preserving the skin branch (the IIFE)
as-is.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0fd92c6f-1bad-407c-ac9d-5c4d5ea8f708
⛔ Files ignored due to path filters (1)
src/client/render/gl/shaders/map-overlay/territory.frag.glslis excluded by!**/*.glsl
📒 Files selected for processing (15)
src/client/Cosmetics.tssrc/client/PatternInput.tssrc/client/Store.tssrc/client/TerritoryPatternsModal.tssrc/client/WebGLFrameBuilder.tssrc/client/components/CosmeticButton.tssrc/client/render/gl/GameView.tssrc/client/render/gl/Renderer.tssrc/client/render/gl/passes/SkinAtlasArray.tssrc/client/render/gl/passes/TerritoryPass.tssrc/core/CosmeticSchemas.tssrc/core/Schemas.tssrc/core/game/UserSettings.tssrc/server/Privilege.tstests/Privilege.test.ts
github-project-automation
Bot
moved this from Development
to Complete
in OpenFront Release Management
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description:
Add image-based territory skins as a new cosmetic type, rendered alongside the existing 1-bit patterns. Skins render a single PNG centered on each player's spawn tile — opaque pixels show the skin (multiplied by team color in team games, raw colors in FFA), transparent pixels and tiles outside the image bounds fall through to the regular player palette color.
Cosmetic plumbing
SkinSchemainCosmeticSchemas.ts, optionalskinsmap onCosmeticsSchemaPlayerSkin,PlayerCosmetics.skin,PlayerCosmeticRefs.skinNameinSchemas.tsPrivilegeCheckerImpl.isSkinAllowed(gated byskin:*/skin:<name>flares)PATTERN_KEY(pattern:andskin:share one slot — they're mutually exclusive)getPlayerCosmeticsRefsonly emits askinNamewhen cosmetics are loaded, the skin exists in the catalog, and the user has the right flare — otherwise drops the ref and clears storageRenderer
SkinAtlasArray— fixedTEXTURE_2D_ARRAY, 1024×1024 per layer, exact layer count allocated once at game start from the locked-in player set. No resize, no callbacks, no retainedHTMLImageElement. Zero GPU cost when no players have skins (1×1 placeholder).skinLayerTex(R8UI 4096×1) — per-playerlayer + 1(0= no skin)skinAnchorTex(RG16UI 4096×1) — per-player spawn tile, so the PNG center anchors at each player's spawn (re-uploads when the player re-picks during spawn phase)WebGLFrameBuilder.syncPlayerscollects unique skin URLs on first sync and callsview.initSkinAtlas(urls)once;clearCaches()resets so seek/replay re-initializesterritory.frag.glsl: skin branch is mutually exclusive with patterns; bounds-checks UVs against[0, 1]so the image is a single stamp, not tiled; alpha-blends against the player palette color so transparent pixels and out-of-bounds tiles render as the regular player colorHover highlight (global UX change, not skin-scoped)
UI
CosmeticButtonrenders skins as a single<img>(object-contain)TerritoryPatternsModalmerges patterns + skins into one grid; single "default" tile clears bothStorepattern tab includes skin entries (purchasable, not-yet-owned)PatternInputlobby button previews the active skin when one is setMemory
Tests
tests/Privilege.test.ts: 13 new cases coveringisSkinAllowed(wildcard, exact-match, missing flare, missing skin, forged refs) andisAllowedintegration (allowed/forbidden paths, short-circuit when invalid skin is paired with valid other cosmetics)Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
evan