Add random skin tile for owned skins#3324
Open
theosorus wants to merge 1 commit intoopenfrontio:mainfrom
Open
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:
WalkthroughThis pull request introduces random cosmetic pattern selection. Users can now select a "Random" option to have the system automatically choose from their owned skins. Changes include a new translation string, client-side logic to detect and resolve random patterns, UI modal updates to display a random skin tile, and comprehensive test coverage. Changes
Sequence DiagramsequenceDiagram
actor User
participant Modal as TerritoryPatternsModal
participant Storage as localStorage
participant Cosmetics as Cosmetics.ts
participant API as API/Server
User->>Modal: Click random skin tile
Modal->>Modal: selectRandomPattern()
Modal->>Storage: Set pattern = "random"
Modal->>Modal: Refresh UI & dispatch event
Modal->>User: Show confirmation message
Modal->>Modal: Close modal
Note over Cosmetics: Later...
Cosmetics->>Storage: Check isRandomSkinSelected()
Storage-->>Cosmetics: true
Cosmetics->>Cosmetics: resolveRandomPattern()
Cosmetics->>API: Fetch owned patterns
API-->>Cosmetics: Return owned skins
Cosmetics->>Cosmetics: Select random from owned
Cosmetics-->>User: Apply cosmetic pattern
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
theosorus
force-pushed
the
feature/random-skin-button
branch
2 times, most recently
from
e54afb8 to
a635dd6
Compare
|
|
![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
🧹 Nitpick comments (3)
resources/lang/fr.json (1)
855-856: Please keep non-English locale updates in the translation workflow.This feature PR updates
resources/lang/fr.json. In this repo, non-English locale updates are usually handled in dedicated translation PRs; consider limiting this PR toen.jsonand letting translators sync other locales separately.Based on learnings: In OpenFrontIO, translation files in
resources/lang/*.json(excepten.json) should not be updated in regular PRs; only dedicated translation PRs should update non-English locale files.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/lang/fr.json` around lines 855 - 856, This PR incorrectly modifies non-English locale keys "selected" and "random_skin" in the French translations; revert changes to those keys in the fr.json update and remove any edits to resources/lang/fr.json from this PR, keeping only updates to en.json so that non-English locale changes are handled in a dedicated translation PR as per the repo workflow.src/client/Cosmetics.ts (1)
133-135: Avoid hardcoding the pattern storage key.This introduces a second raw
"territoryPattern"key path. Please centralize this key (single source of truth) to prevent drift betweenUserSettingsand cosmetics resolution.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/Cosmetics.ts` around lines 133 - 135, isRandomSkinSelected currently hardcodes the "territoryPattern" storage key; instead, reference the centralized key constant used by UserSettings (or add one if missing) to avoid duplicated string literals. Update the isRandomSkinSelected function to read the key via that exported constant (e.g., UserSettings.TERRITORY_PATTERN_KEY or an exported TERRITORY_PATTERN constant) and compare its value to "random", ensuring all code uses the single shared identifier rather than the raw "territoryPattern" string.src/client/TerritoryPatternsModal.ts (1)
117-144: Track ownership with a boolean/count instead of building full objects.
ownedPatternsstores fullPlayerPatternobjects but is only used to check whether at least one owned skin exists. Consider replacing it with a simple counter/flag to reduce render-time allocations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/TerritoryPatternsModal.ts` around lines 117 - 144, The loop in TerritoryPatternsModal.ts currently builds a full ownedPatterns array of PlayerPattern objects simply to check for existence; change this to a simple boolean flag (e.g., hasOwnedPattern) or an integer counter (ownedCount) inside the same loop that iterates over patterns and colorPalettes and calls patternRelationship, incrementing/setting when rel === "owned" and pattern !== null, and stop allocating PlayerPattern objects; then update any downstream usage that inspects ownedPatterns to instead check hasOwnedPattern/ownedCount or convert to a small array only when full objects are actually needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@resources/lang/fa.json`:
- Around line 718-720: Revert the non-English locale edit in
resources/lang/fa.json by removing the added/changed key "random_skin" so this
PR only modifies resources/lang/en.json; ensure no other changes to files under
resources/lang/*.json (except en.json) remain in the diff, and leave future
updates to fa.json to the dedicated translation/“mls” PR process.
In `@resources/lang/fi.json`:
- Around line 628-629: This change adds a Finnish localized string
("random_skin": "Satunnainen") in resources/lang/fi.json which violates the
workflow for non-English translations; remove or revert the "random_skin" entry
from this PR and leave the key either absent or set to the default English value
so that only technical changes remain, and move the Finnish translation into the
dedicated translation flow/MLS PR handled by Aotumori.
In `@resources/lang/ja.json`:
- Around line 855-857: This PR erroneously modifies the non-English locale file
resources/lang/ja.json (keys "selected" and "random_skin"); revert those changes
so ja.json remains unchanged in this feature branch and ensure only en.json or
code changes stay in this PR, then if these Japanese translations are required
open a dedicated translation/“mls” PR (to be handled by Aotumori) that updates
resources/lang/ja.json with the "selected" and "random_skin" entries.
In `@resources/lang/ko.json`:
- Around line 718-720: Revert the edit in resources/lang/ko.json for the
"random_skin" key (undo the non-English change), ensure the corresponding change
is applied to the canonical en.json ("random_skin": "Random" or the intended
English string), and then submit/route the non-English translation update
through the dedicated translation workflow/MLS PR owned by Aotumori so
translations are propagated rather than editing resources/lang/ko.json directly
in this feature PR.
In `@src/client/Cosmetics.ts`:
- Around line 151-155: The long inline expression passed as colorPalette in the
ownedPatterns.push call is causing Prettier style failures; extract the computed
value into a short local const (e.g., const colorPalette = cp ?
cosmetics.colorPalettes?.[cp.name] : undefined) and then call
ownedPatterns.push({ name, patternData: pattern.pattern, colorPalette }); this
keeps the logic in the push intact (references: ownedPatterns.push, patternData,
colorPalette, cp, cosmetics.colorPalettes) and satisfies the formatter.
In `@src/client/TerritoryPatternsModal.ts`:
- Around line 179-206: The random tile lacks the selected styling because the
template doesn't conditionally apply the "selected" classes when random mode is
active; update the template that builds randomTile to include the same
selected-state classNames/attributes used for other pattern tiles (matching the
logic used elsewhere, e.g., compare against this.selectedPatternId or a RANDOM
sentinel) so it renders as selected when active, and modify
selectRandomPattern() to set that sentinel/state (e.g., this.selectedPatternId =
RANDOM_ID or this.isRandomSelected = true) and request an update so the
conditional styling is applied; ensure buttons.splice still inserts the tile in
the same place.
---
Nitpick comments:
In `@resources/lang/fr.json`:
- Around line 855-856: This PR incorrectly modifies non-English locale keys
"selected" and "random_skin" in the French translations; revert changes to those
keys in the fr.json update and remove any edits to resources/lang/fr.json from
this PR, keeping only updates to en.json so that non-English locale changes are
handled in a dedicated translation PR as per the repo workflow.
In `@src/client/Cosmetics.ts`:
- Around line 133-135: isRandomSkinSelected currently hardcodes the
"territoryPattern" storage key; instead, reference the centralized key constant
used by UserSettings (or add one if missing) to avoid duplicated string
literals. Update the isRandomSkinSelected function to read the key via that
exported constant (e.g., UserSettings.TERRITORY_PATTERN_KEY or an exported
TERRITORY_PATTERN constant) and compare its value to "random", ensuring all code
uses the single shared identifier rather than the raw "territoryPattern" string.
In `@src/client/TerritoryPatternsModal.ts`:
- Around line 117-144: The loop in TerritoryPatternsModal.ts currently builds a
full ownedPatterns array of PlayerPattern objects simply to check for existence;
change this to a simple boolean flag (e.g., hasOwnedPattern) or an integer
counter (ownedCount) inside the same loop that iterates over patterns and
colorPalettes and calls patternRelationship, incrementing/setting when rel ===
"owned" and pattern !== null, and stop allocating PlayerPattern objects; then
update any downstream usage that inspects ownedPatterns to instead check
hasOwnedPattern/ownedCount or convert to a small array only when full objects
are actually needed.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (37)
resources/lang/ar.jsonresources/lang/bg.jsonresources/lang/bn.jsonresources/lang/cs.jsonresources/lang/da.jsonresources/lang/de.jsonresources/lang/el.jsonresources/lang/en.jsonresources/lang/eo.jsonresources/lang/es.jsonresources/lang/fa.jsonresources/lang/fi.jsonresources/lang/fr.jsonresources/lang/gl.jsonresources/lang/he.jsonresources/lang/hi.jsonresources/lang/hu.jsonresources/lang/id.jsonresources/lang/it.jsonresources/lang/ja.jsonresources/lang/ko.jsonresources/lang/mk.jsonresources/lang/nl.jsonresources/lang/pl.jsonresources/lang/pt-BR.jsonresources/lang/pt-PT.jsonresources/lang/ru.jsonresources/lang/sh.jsonresources/lang/sk.jsonresources/lang/sl.jsonresources/lang/sv-SE.jsonresources/lang/tp.jsonresources/lang/tr.jsonresources/lang/uk.jsonresources/lang/zh-CN.jsonsrc/client/Cosmetics.tssrc/client/TerritoryPatternsModal.ts
resources/lang/fa.json
Outdated
Comment on lines
718
to
720
| }, | ||
| "random_skin": "تصادفی" | ||
| }, |
Contributor
There was a problem hiding this comment.
Move this non-English locale change to a dedicated translation PR.
Line [719] updates resources/lang/fa.json in a regular feature PR. In this repo, non-English locale updates should be handled only in the dedicated translation flow; please revert this here and keep only resources/lang/en.json in this PR.
Based on learnings: In OpenFrontIO, translation files in resources/lang/*.json (except en.json) should not be updated in regular PRs; only dedicated “mls” PRs by Aotumori should update non-English locale files.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@resources/lang/fa.json` around lines 718 - 720, Revert the non-English locale
edit in resources/lang/fa.json by removing the added/changed key "random_skin"
so this PR only modifies resources/lang/en.json; ensure no other changes to
files under resources/lang/*.json (except en.json) remain in the diff, and leave
future updates to fa.json to the dedicated translation/“mls” PR process.
github-project-automation
bot
moved this from Triage
to Development
in OpenFront Release Management
theosorus
force-pushed
the
feature/random-skin-button
branch
from
a635dd6 to
4682daa
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (1)
src/client/TerritoryPatternsModal.ts (1)
155-162:⚠️ Potential issue | 🟡 MinorRandom mode still has no clear selected state in the grid.
After selecting random, the random tile does not render as selected, so users can see an incorrect selected tile.
💡 Proposed fix
import { fetchCosmetics, getPlayerCosmetics, handlePurchase, + isRandomSkinSelected, patternRelationship, } from "./Cosmetics"; @@ private renderPatternGrid(): TemplateResult { const buttons: TemplateResult[] = []; + const randomSelected = isRandomSkinSelected(); @@ const isSelected = - (isDefaultPattern && this.selectedPattern === null) || + (isDefaultPattern && + this.selectedPattern === null && + !randomSelected) || (!isDefaultPattern && this.selectedPattern && this.selectedPattern.name === pattern?.name && (this.selectedPattern.colorPalette?.name ?? null) === (colorPalette?.name ?? null)); @@ - if (this.showOnlyOwned && ownedPatterns.length >= 1) { + if (this.showOnlyOwned && ownedPatterns.length > 0) { const randomTile = html` <div - class="flex flex-col items-center justify-between gap-2 p-3 bg-white/5 backdrop-blur-sm border border-white/10 rounded-xl w-48 h-full transition-all duration-200 hover:bg-white/10 hover:border-white/20 hover:shadow-xl" + class="flex flex-col items-center justify-between gap-2 p-3 backdrop-blur-sm border rounded-xl w-48 h-full transition-all duration-200 hover:shadow-xl + ${randomSelected + ? "bg-blue-500/20 border-blue-400/60 ring-2 ring-blue-400/50" + : "bg-white/5 border-white/10 hover:bg-white/10 hover:border-white/20"}" >Also applies to: 179-206, 348-364
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/TerritoryPatternsModal.ts` around lines 155 - 162, The current isSelected logic in TerritoryPatternsModal.ts fails to treat the "random" option as a selectable match; update the comparison in the isSelected computation (the block using isDefaultPattern, isSelected, this.selectedPattern, pattern, colorPalette) to explicitly detect the random state (e.g., when the selectedPattern represents random or pattern represents the random tile) and return true for that case; apply the same fix to the other duplicated comparison blocks (the ones around lines 179-206 and 348-364) so the random tile renders as selected when random is chosen.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@resources/lang/da.json`:
- Around line 590-591: This change modifies the non-English locale file
resources/lang/da.json (the "random_skin" key); revert the edit to
resources/lang/da.json so only resources/lang/en.json is changed in this PR, and
if the Danish translation needs updating open a dedicated translation PR that
updates resources/lang/da.json (refer to the "random_skin" key when making that
translation PR).
In `@resources/lang/el.json`:
- Around line 673-674: This PR unintentionally modifies the non-English locale
file resources/lang/el.json by adding the "random_skin" key; revert the change
so only resources/lang/en.json is updated in this PR per project convention,
remove the addition of "random_skin" from el.json (or restore the previous
el.json content) and ensure any intended translation changes are moved into a
dedicated translation PR; reference the "random_skin" key to locate the edit and
leave only en.json edits in this branch.
In `@resources/lang/gl.json`:
- Around line 590-591: This change modified a non-English locale file
(resources/lang/gl.json) by adding or changing the "random_skin" key to
"Aleatorio"; revert that edit so non-English translation files are untouched in
this PR and ensure only resources/lang/en.json is modified here—remove or
restore the "random_skin" change in resources/lang/gl.json and leave
translations to a dedicated "mls" translation PR as per project convention.
In `@resources/lang/sk.json`:
- Around line 590-591: Revert the non-English locale change by removing the
added "random_skin" translation from sk.json so that only the English locale is
changed in this PR; specifically undo the modification that added the
"random_skin": "Náhodný" entry (or revert the commit/patch that touched sk.json)
and ensure any intended UI copy updates are instead applied to en.json only,
following the project convention that only en.json is updated in regular PRs.
In `@resources/lang/sv-SE.json`:
- Around line 578-579: Revert the change in resources/lang/sv-SE.json so
non-English locale edits are not included in this PR: remove the added/modified
"random_skin" entry (and any other changes in resources/lang/*.json except
en.json) and restore the file to its original state, ensuring only
resources/lang/en.json contains translation edits in this branch; target the
change around the "random_skin" key to locate and undo the modification.
In `@resources/lang/tp.json`:
- Around line 321-324: Revert the non-English locale change in tp.json by
removing the added territory_patterns entry ("random_skin": "Wile") so this PR
only contains en.json translation edits; locate the JSON object named
"territory_patterns" and remove the "random_skin" key/value (or restore tp.json
to its previous state) leaving all translation work for a dedicated translation
PR.
In `@resources/lang/zh-CN.json`:
- Around line 855-856: This change modified non-English locale keys ("selected"
and "random_skin") in resources/lang/zh-CN.json which violates the convention to
only update resources/lang/en.json in regular PRs; revert the edits to
resources/lang/zh-CN.json so it remains unchanged in this PR, ensure only the
corresponding updates exist in resources/lang/en.json, and if translated strings
are needed submit them in a separate dedicated translation PR referencing the
same keys ("selected", "random_skin").
In `@tests/client/RandomSkin.test.ts`:
- Around line 1-146: This test file has Prettier/style violations; run the
project's Prettier formatter against the RandomSkin.test.ts file (or run npx
prettier --write using the repo config), stage the rewritten file, and commit
the formatted changes so CI passes; focus on formatting the tests that reference
isRandomSkinSelected, getPlayerCosmeticsRefs, and patternRelationship to match
the project's Prettier rules.
- Around line 137-144: The test currently sets localStorage with
"territoryPattern" and calls getPlayerCosmeticsRefs() but asserts
refs.patternName is undefined, contradicting the test title and comments; update
the assertion to expect the resolved pattern (e.g.,
expect(refs.patternName).toBe("pattern_a")) so the test verifies that
getPlayerCosmeticsRefs / getSelectedPatternName returns the pattern from the
"territoryPattern" value when not in random mode.
---
Duplicate comments:
In `@src/client/TerritoryPatternsModal.ts`:
- Around line 155-162: The current isSelected logic in TerritoryPatternsModal.ts
fails to treat the "random" option as a selectable match; update the comparison
in the isSelected computation (the block using isDefaultPattern, isSelected,
this.selectedPattern, pattern, colorPalette) to explicitly detect the random
state (e.g., when the selectedPattern represents random or pattern represents
the random tile) and return true for that case; apply the same fix to the other
duplicated comparison blocks (the ones around lines 179-206 and 348-364) so the
random tile renders as selected when random is chosen.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (38)
resources/lang/ar.jsonresources/lang/bg.jsonresources/lang/bn.jsonresources/lang/cs.jsonresources/lang/da.jsonresources/lang/de.jsonresources/lang/el.jsonresources/lang/en.jsonresources/lang/eo.jsonresources/lang/es.jsonresources/lang/fa.jsonresources/lang/fi.jsonresources/lang/fr.jsonresources/lang/gl.jsonresources/lang/he.jsonresources/lang/hi.jsonresources/lang/hu.jsonresources/lang/id.jsonresources/lang/it.jsonresources/lang/ja.jsonresources/lang/ko.jsonresources/lang/mk.jsonresources/lang/nl.jsonresources/lang/pl.jsonresources/lang/pt-BR.jsonresources/lang/pt-PT.jsonresources/lang/ru.jsonresources/lang/sh.jsonresources/lang/sk.jsonresources/lang/sl.jsonresources/lang/sv-SE.jsonresources/lang/tp.jsonresources/lang/tr.jsonresources/lang/uk.jsonresources/lang/zh-CN.jsonsrc/client/Cosmetics.tssrc/client/TerritoryPatternsModal.tstests/client/RandomSkin.test.ts
🚧 Files skipped from review as they are similar to previous changes (23)
- resources/lang/bg.json
- resources/lang/ar.json
- resources/lang/ja.json
- resources/lang/ko.json
- resources/lang/pl.json
- resources/lang/mk.json
- resources/lang/he.json
- resources/lang/pt-PT.json
- resources/lang/en.json
- resources/lang/sl.json
- resources/lang/hi.json
- resources/lang/uk.json
- resources/lang/bn.json
- resources/lang/cs.json
- resources/lang/eo.json
- resources/lang/es.json
- resources/lang/pt-BR.json
- resources/lang/ru.json
- resources/lang/nl.json
- resources/lang/fi.json
- resources/lang/hu.json
- resources/lang/tr.json
- resources/lang/it.json
theosorus
force-pushed
the
feature/random-skin-button
branch
from
4682daa to
671ae26
Compare
When a player owns 1+ skins, a "Random" tile appears in the "My Skins" grid next to "Default". Selecting it stores a "random" marker in localStorage. Each time the player joins a game, a random owned skin is picked, so the skin changes every game.
theosorus
force-pushed
the
feature/random-skin-button
branch
from
671ae26 to
3d196bf
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (5)
resources/lang/sv-SE.json (1)
578-579:⚠️ Potential issue | 🟠 MajorMove Swedish locale edits to the dedicated translation PR flow.
At Line [579], this non-English locale update should not be part of a regular feature PR. Please revert this file change here and keep only
resources/lang/en.jsonupdates.Based on learnings: In OpenFrontIO, translation files in resources/lang/*.json (except en.json) should not be updated in regular PRs. Only dedicated translation PRs titled "mls" and made by Aotumori should update non-English locale files.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/lang/sv-SE.json` around lines 578 - 579, This commit includes an unauthorized change to a non-English locale file (resources/lang/sv-SE.json) — specifically the "random_skin" key — which should not be modified in regular feature PRs; revert the change in resources/lang/sv-SE.json (restore it to the state from main) or remove the "random_skin" edit so only resources/lang/en.json contains translation edits, and ensure future commits follow the translation flow (only the dedicated "mls" PRs by Aotumori modify non-English locale files).resources/lang/zh-CN.json (1)
855-856:⚠️ Potential issue | 🟠 MajorPlease remove
zh-CNlocalization edits from this feature PR.At Line [856], this updates a non-English locale file in a regular PR. Please revert this file change and leave only the
en.jsonkey addition in this branch.Based on learnings: In OpenFrontIO, translation files in resources/lang/*.json (except en.json) should not be updated in regular PRs. Only dedicated translation PRs titled "mls" and made by Aotumori should update non-English locale files.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/lang/zh-CN.json` around lines 855 - 856, Revert the non-English locale edits in resources/lang/zh-CN.json by removing the newly added keys ("selected" and "random_skin") so this file matches the upstream branch state; keep only the en.json changes in this PR and ensure no other files under resources/lang/*.json (except en.json) are modified before committing.resources/lang/tp.json (1)
321-324:⚠️ Potential issue | 🟠 MajorPlease keep non-English locale edits out of this PR.
At Line [323],
resources/lang/tp.jsonwas changed in a regular feature PR. Please revert this file change and keep onlyresources/lang/en.jsonupdates here, then let the translation team handle non-English locales in their dedicated translation PR flow.Based on learnings: In OpenFrontIO, translation files in resources/lang/*.json (except en.json) should not be updated in regular PRs. Only dedicated translation PRs titled "mls" and made by Aotumori should update non-English locale files.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/lang/tp.json` around lines 321 - 324, Revert the non-English locale edit in tp.json by removing the added territory_patterns -> random_skin ("Wile") entry so tp.json is restored to its original state and only en.json contains the intended changes; locate the change by looking for the "territory_patterns" and "random_skin" keys in tp.json, undo that diff, commit the revert with a clear message (e.g., "revert accidental tp.json translation change"), and push—leave any translation updates to the translation team via the dedicated "mls" PR workflow.src/client/TerritoryPatternsModal.ts (1)
179-206:⚠️ Potential issue | 🟡 MinorRandom tile still does not reliably show selected state.
When random mode is active, UI selection can be unclear because the random tile has no explicit selected binding, and pattern selection logic still uses
selectedPattern.Suggested fix (add explicit random-selected state in render)
import { fetchCosmetics, getPlayerCosmetics, handlePurchase, + isRandomSkinSelected, patternRelationship, } from "./Cosmetics"; @@ private renderPatternGrid(): TemplateResult { const buttons: TemplateResult[] = []; const ownedPatterns: PlayerPattern[] = []; + const randomSelected = isRandomSkinSelected(); @@ - const isSelected = - (isDefaultPattern && this.selectedPattern === null) || - (!isDefaultPattern && - this.selectedPattern && - this.selectedPattern.name === pattern?.name && - (this.selectedPattern.colorPalette?.name ?? null) === - (colorPalette?.name ?? null)); + const isSelected = + !randomSelected && + ((isDefaultPattern && this.selectedPattern === null) || + (!isDefaultPattern && + this.selectedPattern && + this.selectedPattern.name === pattern?.name && + (this.selectedPattern.colorPalette?.name ?? null) === + (colorPalette?.name ?? null))); @@ - const randomTile = html` + const randomTile = html` <div - class="flex flex-col items-center justify-between gap-2 p-3 bg-white/5 backdrop-blur-sm border border-white/10 rounded-xl w-48 h-full transition-all duration-200 hover:bg-white/10 hover:border-white/20 hover:shadow-xl" + class="flex flex-col items-center justify-between gap-2 p-3 backdrop-blur-sm rounded-xl w-48 h-full transition-all duration-200 hover:shadow-xl ${ + randomSelected + ? "bg-blue-500/20 border border-blue-500/50 shadow-[0_0_10px_rgba(59,130,246,0.3)]" + : "bg-white/5 border border-white/10 hover:bg-white/10 hover:border-white/20" + }" > <button class="group relative flex flex-col items-center w-full gap-2 rounded-lg cursor-pointer transition-all duration-200 flex-1" + aria-pressed=${randomSelected} `@click`=${() => this.selectRandomPattern()} >Also applies to: 348-364
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/TerritoryPatternsModal.ts` around lines 179 - 206, The random tile lacks an explicit selected binding so the UI can't reliably show its selected state; add an explicit flag (e.g., this.randomSelected or use a sentinel value like this.selectedPattern === 'RANDOM') and update the render of the randomTile (the element created where buttons.splice(1, 0, randomTile) is used) to apply the same selected CSS/classes/attributes as other pattern tiles when that flag is true, and modify the click handler (selectRandomPattern) to set this flag (and clear it when a concrete pattern is chosen) so selection checks elsewhere that read selectedPattern or pattern selection state properly reflect the random mode; repeat the same change for the second occurrence mentioned (around the other block at lines 348-364).resources/lang/hu.json (1)
634-635:⚠️ Potential issue | 🟠 MajorMove non-English locale value updates to the dedicated MLS translation flow.
This feature PR adds Hungarian translated content directly. Per repo workflow, keep translation-key technical changes in regular PRs and move non-English value updates to the translation pipeline.
Suggested minimal adjustment in this PR
- }, - "random_skin": "Véletlenszerű" + }Based on learnings: In OpenFrontIO, non-English locale updates should be handled in dedicated “mls” translation PRs (typically by the translation team), while regular PRs should avoid non-English translation content updates.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/lang/hu.json` around lines 634 - 635, Revert the Hungarian translation value for the key "random_skin" and keep only the technical key change in this PR: replace "Véletlenszerű" with the English source/default (e.g., "Random") or an empty placeholder, then remove the non-English content and move the Hungarian string into the MLS translation pipeline so the translation team can supply the localized value in a dedicated MLS translation PR.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@resources/lang/eo.json`:
- Around line 634-636: This PR accidentally modifies a non-English translation
file: remove the added key "random_skin": "Hazarda" from resources/lang/eo.json
and keep the English change only in resources/lang/en.json; ensure no other
edits to files under resources/lang/*.json except en.json are included in this
branch and revert/unstage any changes to eo.json so non-English locale updates
are left for the dedicated "mls" translation PR process (only by Aotumori).
In `@resources/lang/nl.json`:
- Around line 855-856: Revert the inadvertent Dutch locale edit by restoring
resources/lang/nl.json to its previous state (remove the added/modified keys
"random_skin" and/or "selected" changes), leaving only the intended English
locale changes in resources/lang/en.json; ensure no other files under
resources/lang/*.json (except en.json) are changed in this PR so translation
updates remain handled by the dedicated translation flow.
In `@resources/lang/pt-PT.json`:
- Around line 590-592: Revert the addition of the "random_skin" key from the
non-English locale file and ensure the new translation key exists only in the
English locale (resources/lang/en.json); specifically remove "random_skin":
"Aleatório" from the non-English locale and keep the key added only in en.json
so the translation team can propagate it to other locales.
In `@resources/lang/sl.json`:
- Around line 634-636: Revert the Slovenian locale edit by removing the
"random_skin" key/value from the sl.json locale file (undo the addition shown as
"random_skin": "Naključno") so only the English key remains in this feature PR;
ensure the corresponding English entry is added/kept in en.json and move any
non-English updates for "random_skin" into the dedicated translation PR/workflow
handled by the translation team.
---
Duplicate comments:
In `@resources/lang/hu.json`:
- Around line 634-635: Revert the Hungarian translation value for the key
"random_skin" and keep only the technical key change in this PR: replace
"Véletlenszerű" with the English source/default (e.g., "Random") or an empty
placeholder, then remove the non-English content and move the Hungarian string
into the MLS translation pipeline so the translation team can supply the
localized value in a dedicated MLS translation PR.
In `@resources/lang/sv-SE.json`:
- Around line 578-579: This commit includes an unauthorized change to a
non-English locale file (resources/lang/sv-SE.json) — specifically the
"random_skin" key — which should not be modified in regular feature PRs; revert
the change in resources/lang/sv-SE.json (restore it to the state from main) or
remove the "random_skin" edit so only resources/lang/en.json contains
translation edits, and ensure future commits follow the translation flow (only
the dedicated "mls" PRs by Aotumori modify non-English locale files).
In `@resources/lang/tp.json`:
- Around line 321-324: Revert the non-English locale edit in tp.json by removing
the added territory_patterns -> random_skin ("Wile") entry so tp.json is
restored to its original state and only en.json contains the intended changes;
locate the change by looking for the "territory_patterns" and "random_skin" keys
in tp.json, undo that diff, commit the revert with a clear message (e.g.,
"revert accidental tp.json translation change"), and push—leave any translation
updates to the translation team via the dedicated "mls" PR workflow.
In `@resources/lang/zh-CN.json`:
- Around line 855-856: Revert the non-English locale edits in
resources/lang/zh-CN.json by removing the newly added keys ("selected" and
"random_skin") so this file matches the upstream branch state; keep only the
en.json changes in this PR and ensure no other files under resources/lang/*.json
(except en.json) are modified before committing.
In `@src/client/TerritoryPatternsModal.ts`:
- Around line 179-206: The random tile lacks an explicit selected binding so the
UI can't reliably show its selected state; add an explicit flag (e.g.,
this.randomSelected or use a sentinel value like this.selectedPattern ===
'RANDOM') and update the render of the randomTile (the element created where
buttons.splice(1, 0, randomTile) is used) to apply the same selected
CSS/classes/attributes as other pattern tiles when that flag is true, and modify
the click handler (selectRandomPattern) to set this flag (and clear it when a
concrete pattern is chosen) so selection checks elsewhere that read
selectedPattern or pattern selection state properly reflect the random mode;
repeat the same change for the second occurrence mentioned (around the other
block at lines 348-364).
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (38)
resources/lang/ar.jsonresources/lang/bg.jsonresources/lang/bn.jsonresources/lang/cs.jsonresources/lang/da.jsonresources/lang/de.jsonresources/lang/el.jsonresources/lang/en.jsonresources/lang/eo.jsonresources/lang/es.jsonresources/lang/fa.jsonresources/lang/fi.jsonresources/lang/fr.jsonresources/lang/gl.jsonresources/lang/he.jsonresources/lang/hi.jsonresources/lang/hu.jsonresources/lang/id.jsonresources/lang/it.jsonresources/lang/ja.jsonresources/lang/ko.jsonresources/lang/mk.jsonresources/lang/nl.jsonresources/lang/pl.jsonresources/lang/pt-BR.jsonresources/lang/pt-PT.jsonresources/lang/ru.jsonresources/lang/sh.jsonresources/lang/sk.jsonresources/lang/sl.jsonresources/lang/sv-SE.jsonresources/lang/tp.jsonresources/lang/tr.jsonresources/lang/uk.jsonresources/lang/zh-CN.jsonsrc/client/Cosmetics.tssrc/client/TerritoryPatternsModal.tstests/client/RandomSkin.test.ts
✅ Files skipped from review due to trivial changes (1)
- tests/client/RandomSkin.test.ts
🚧 Files skipped from review as they are similar to previous changes (24)
- resources/lang/pl.json
- resources/lang/hi.json
- resources/lang/fa.json
- resources/lang/bn.json
- resources/lang/ko.json
- resources/lang/bg.json
- resources/lang/cs.json
- resources/lang/id.json
- resources/lang/it.json
- resources/lang/gl.json
- resources/lang/fr.json
- resources/lang/ru.json
- resources/lang/pt-BR.json
- resources/lang/da.json
- resources/lang/ja.json
- resources/lang/ar.json
- resources/lang/es.json
- resources/lang/he.json
- resources/lang/sh.json
- resources/lang/el.json
- resources/lang/mk.json
- resources/lang/tr.json
- resources/lang/uk.json
- resources/lang/sk.json
resources/lang/eo.json
Outdated
Comment on lines
634
to
636
| }, | ||
| "random_skin": "Hazarda" | ||
| }, |
Contributor
There was a problem hiding this comment.
Please remove non-English locale edits from this PR.
At Line [635], this adds a key in resources/lang/eo.json during a regular feature PR. Keep the English key in resources/lang/en.json and move Esperanto updates to the dedicated translation PR process.
Based on learnings: In OpenFrontIO, translation files in resources/lang/*.json (except en.json) should not be updated in regular PRs. Only dedicated translation PRs titled "mls" and made by Aotumori should update non-English locale files.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@resources/lang/eo.json` around lines 634 - 636, This PR accidentally modifies
a non-English translation file: remove the added key "random_skin": "Hazarda"
from resources/lang/eo.json and keep the English change only in
resources/lang/en.json; ensure no other edits to files under
resources/lang/*.json except en.json are included in this branch and
revert/unstage any changes to eo.json so non-English locale updates are left for
the dedicated "mls" translation PR process (only by Aotumori).
resources/lang/nl.json
Outdated
Comment on lines
+855
to
+856
| "selected": "geselecteerd", | ||
| "random_skin": "Willekeurig" |
Contributor
There was a problem hiding this comment.
Keep Dutch locale changes out of this feature PR.
At Line [856], this non-English locale update should be handled in the dedicated translation flow, not in this PR. Please revert this file change and keep only resources/lang/en.json updated here.
Based on learnings: For the OpenFrontIO project, do not suggest updating translation files in resources/lang/*.json except for en.json. The project has a dedicated translation team that handles all other locale files.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@resources/lang/nl.json` around lines 855 - 856, Revert the inadvertent Dutch
locale edit by restoring resources/lang/nl.json to its previous state (remove
the added/modified keys "random_skin" and/or "selected" changes), leaving only
the intended English locale changes in resources/lang/en.json; ensure no other
files under resources/lang/*.json (except en.json) are changed in this PR so
translation updates remain handled by the dedicated translation flow.
resources/lang/pt-PT.json
Outdated
Comment on lines
590
to
592
| }, | ||
| "random_skin": "Aleatório" | ||
| }, |
Contributor
There was a problem hiding this comment.
Move this locale change to the translation pipeline.
At Line [591], this non-English locale file is updated in a regular feature PR. Please revert this file change in this PR and keep the key addition in resources/lang/en.json only.
Based on learnings: For the OpenFrontIO project, do not suggest updating translation files in resources/lang/*.json except for en.json. The project has a dedicated translation team that handles all other locale files.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@resources/lang/pt-PT.json` around lines 590 - 592, Revert the addition of the
"random_skin" key from the non-English locale file and ensure the new
translation key exists only in the English locale (resources/lang/en.json);
specifically remove "random_skin": "Aleatório" from the non-English locale and
keep the key added only in en.json so the translation team can propagate it to
other locales.
resources/lang/sl.json
Outdated
Comment on lines
634
to
636
| }, | ||
| "random_skin": "Naključno" | ||
| }, |
Contributor
There was a problem hiding this comment.
Please revert Slovenian locale edits from this PR.
At Line [635], this is a non-English translation update in a regular feature PR. Keep only the English key addition in this PR and move sl.json updates to the dedicated translation PR workflow.
Based on learnings: For the OpenFrontIO project, do not suggest updating translation files in resources/lang/*.json except for en.json. The project has a dedicated translation team that handles all other locale files.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@resources/lang/sl.json` around lines 634 - 636, Revert the Slovenian locale
edit by removing the "random_skin" key/value from the sl.json locale file (undo
the addition shown as "random_skin": "Naključno") so only the English key
remains in this feature PR; ensure the corresponding English entry is added/kept
in en.json and move any non-English updates for "random_skin" into the dedicated
translation PR/workflow handled by the translation team.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/client/TerritoryPatternsModal.ts (1)
179-203:⚠️ Potential issue | 🟡 MinorRandom mode still has ambiguous selected state.
The Random tile has no selected styling, and
this.selectedPattern = nullcan make Default appear selected in random mode. This keeps selection feedback unclear.Proposed fix sketch
+import { isRandomSkinSelected } from "./Cosmetics"; ... - const isSelected = - (isDefaultPattern && this.selectedPattern === null) || + const randomSelected = isRandomSkinSelected(); + const isSelected = + (isDefaultPattern && this.selectedPattern === null && !randomSelected) || (!isDefaultPattern && this.selectedPattern && this.selectedPattern.name === pattern?.name && (this.selectedPattern.colorPalette?.name ?? null) === (colorPalette?.name ?? null)); ... - const randomTile = html` + const randomTile = html` <div - class="flex flex-col items-center justify-between gap-2 p-3 bg-white/5 backdrop-blur-sm border border-white/10 rounded-xl w-48 h-full transition-all duration-200 hover:bg-white/10 hover:border-white/20 hover:shadow-xl" + class="flex flex-col items-center justify-between gap-2 p-3 rounded-xl w-48 h-full transition-all duration-200 ${randomSelected + ? "bg-blue-500/20 border border-blue-500/50 shadow-[0_0_10px_rgba(59,130,246,0.3)]" + : "bg-white/5 backdrop-blur-sm border border-white/10 hover:bg-white/10 hover:border-white/20 hover:shadow-xl"}" >Also applies to: 348-353
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/TerritoryPatternsModal.ts` around lines 179 - 203, The Random tile lacks selected styling because selection logic treats this.selectedPattern === null as Default; update selection state and rendering so Random is explicitly selectable: modify selectRandomPattern() to set a distinct flag or sentinel (e.g., this.selectedPattern = '__RANDOM__' or set this.isRandomSelected = true) and ensure other selection paths clear that flag; then update the Random tile render block (the template that calls `@click`=${() => this.selectRandomPattern()}) to apply the same "selected" CSS when the sentinel/flag is active, and change the Default tile rendering logic to only appear selected for the real default pattern (not when the random sentinel/flag is set). Ensure the same change is applied to the duplicate Random tile instance mentioned.
🧹 Nitpick comments (1)
src/client/Cosmetics.ts (1)
133-135: Avoid hardcoded settings key duplication.Using
"territoryPattern"directly here can drift fromUserSettingsstorage behavior over time. Please read/write through a shared constant/helper to keep one source of truth.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/Cosmetics.ts` around lines 133 - 135, The function isRandomSkinSelected currently compares localStorage.getItem("territoryPattern") to "random" using the hardcoded key "territoryPattern"; change it to use the centralized key/helper used by your settings layer (e.g., a exported constant or accessor on UserSettings) instead of the literal string and read via that helper (e.g., UserSettings.get/TERRITORY_PATTERN_KEY) so both reads/writes share one source of truth; update isRandomSkinSelected to reference that constant/helper and keep the "random" comparison the same.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/client/TerritoryPatternsModal.ts`:
- Line 205: The code inserts randomTile into the buttons array at index 1 using
buttons.splice(1, 0, randomTile), which places it after the Default tile; change
the insertion to place Random first by using buttons.splice(0, 0, randomTile) or
buttons.unshift(randomTile) so randomTile becomes the first element (refer to
variables/buttons and randomTile in TerritoryPatternsModal).
---
Duplicate comments:
In `@src/client/TerritoryPatternsModal.ts`:
- Around line 179-203: The Random tile lacks selected styling because selection
logic treats this.selectedPattern === null as Default; update selection state
and rendering so Random is explicitly selectable: modify selectRandomPattern()
to set a distinct flag or sentinel (e.g., this.selectedPattern = '__RANDOM__' or
set this.isRandomSelected = true) and ensure other selection paths clear that
flag; then update the Random tile render block (the template that calls
`@click`=${() => this.selectRandomPattern()}) to apply the same "selected" CSS
when the sentinel/flag is active, and change the Default tile rendering logic to
only appear selected for the real default pattern (not when the random
sentinel/flag is set). Ensure the same change is applied to the duplicate Random
tile instance mentioned.
---
Nitpick comments:
In `@src/client/Cosmetics.ts`:
- Around line 133-135: The function isRandomSkinSelected currently compares
localStorage.getItem("territoryPattern") to "random" using the hardcoded key
"territoryPattern"; change it to use the centralized key/helper used by your
settings layer (e.g., a exported constant or accessor on UserSettings) instead
of the literal string and read via that helper (e.g.,
UserSettings.get/TERRITORY_PATTERN_KEY) so both reads/writes share one source of
truth; update isRandomSkinSelected to reference that constant/helper and keep
the "random" comparison the same.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
resources/lang/en.jsonsrc/client/Cosmetics.tssrc/client/TerritoryPatternsModal.tstests/client/RandomSkin.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- resources/lang/en.json
- tests/client/RandomSkin.test.ts
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
2 participants
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:
Adds a "Random" tile (🎲) to the skin selector grid. When a player owns 1 or more skins and views "My Skins", the Random tile appears as the first tile in the grid. Selecting it stores a
"random"marker in localStorage. Each time the player joins a game, a random owned skin is picked from their collection — so the skin changes every game automatically.Changes:
selectRandomPattern()method that stores the"random"markerisRandomSkinSelected()andresolveRandomPattern()— resolves a random owned skin at game join time ingetPlayerCosmeticsRefs()Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
gazeux33