Fixed playerPanel doesnt show flag | Added flag name on hover#3851
Fixed playerPanel doesnt show flag | Added flag name on hover#3851Boostry123 wants to merge 5 commits intoopenfrontio:mainfrom
Conversation
WalkthroughrenderIdentityRow now reads the flag path from other.cosmetics.flag, extracts a flagCode via regex, resolves country metadata with Countries.find(...), and renders the flag ChangesPlayer Panel Flag Rendering
Estimated Code Review Effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/client/graphics/layers/PlayerPanel.ts (1)
499-524:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winBroken regex makes the flag never appear for most path formats.
The regex
/\/([^.]+)\./at line 500 does not exclude/inside[^.]+, so it greedily matches from the first slash in the path. For any path like/images/flags/us.svg, it capturesimages/flags/us, and thenflagCode.split("/")[1]yields"flags"— not a country code — soCountries.find()returnsundefined. Because line 514 guards oncountry &&, the<img>is suppressed entirely, which defeats this PR's main goal.Two fixes are needed:
- Correct the regex to extract just the filename stem (no slashes, no extension).
- Separate the render guard from country resolution: use
activeFlagPathalone to decide whether to show the image, and use the country result only as a tooltip/alt fallback.🐛 Proposed fix
- const flagCode = activeFlagPath?.match(/\/([^.]+)\./)?.[1]; + // Extract the bare filename stem: e.g. "us" from "/images/flags/us.svg" + const flagCode = activeFlagPath?.match(/\/([^/.]+)\.[^/]*$/)?.[1]; // Find the country based on that code const country = flagCode - ? Countries.find((c) => c.code === flagCode.split("/")[1]) + ? Countries.find((c) => c.code === flagCode) : flagCode;- ${country && activeFlagPath + ${activeFlagPath ? html`<img src=${activeFlagPath} - title=${typeof country !== "string" ? country.name : country} - alt=${typeof country !== "string" ? country.name : country} + title=${(country && typeof country !== "string") ? country.name : (flagCode ?? "")} + alt=${(country && typeof country !== "string") ? country.name : (flagCode ?? "")} class="h-10 w-10 rounded-full object-cover" `@error`=${(e: Event) => { (e.target as HTMLImageElement).style.display = "none"; }} />` : ""}Run this to confirm actual flag path formats stored in
cosmetics.flag:#!/bin/bash # Find how cosmetics.flag values are set/formatted in the codebase rg -n "\.flag\s*=" --type ts -C 2 rg -n "cosmetics" src/core/CosmeticSchemas.ts -A 3 2>/dev/null || true rg -n "flag.*assetUrl\|assetUrl.*flag" --type ts -C 2🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/client/graphics/layers/PlayerPanel.ts` around lines 499 - 524, The flag extraction regex and render guard are incorrect: update the flag filename extraction so flagCode pulls only the filename stem (no slashes or extension) from activeFlagPath (fix the regex used in flagCode), then change the rendering condition to rely solely on activeFlagPath (not on country) so the <img> is shown whenever activeFlagPath exists; keep Countries.find(...) (used to build tooltip/alt) as a separate lookup and fall back to displaying the raw flagCode or a sensible string when Countries.find(...) returns undefined (adjust uses of flagCode, Countries.find, and the img title/alt accordingly in PlayerPanel).
🤖 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/graphics/layers/PlayerPanel.ts`:
- Around line 517-518: The Prettier error is caused by missing spaces around the
strict inequality in the template attributes for title and alt in PlayerPanel
(the expressions `typeof country!== "string"`); update those expressions to
include spaces around `!==` (e.g., `typeof country !== "string"`) for both title
and alt, then run `npx prettier --write
src/client/graphics/layers/PlayerPanel.ts` to reformat the file and fix the CI
check.
---
Outside diff comments:
In `@src/client/graphics/layers/PlayerPanel.ts`:
- Around line 499-524: The flag extraction regex and render guard are incorrect:
update the flag filename extraction so flagCode pulls only the filename stem (no
slashes or extension) from activeFlagPath (fix the regex used in flagCode), then
change the rendering condition to rely solely on activeFlagPath (not on country)
so the <img> is shown whenever activeFlagPath exists; keep Countries.find(...)
(used to build tooltip/alt) as a separate lookup and fall back to displaying the
raw flagCode or a sensible string when Countries.find(...) returns undefined
(adjust uses of flagCode, Countries.find, and the img title/alt accordingly in
PlayerPanel).
🪄 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: eb40bb54-33ad-439d-9e5f-748ebff273b2
📒 Files selected for processing (1)
src/client/graphics/layers/PlayerPanel.ts
github-project-automation
Bot
moved this from Triage
to Development
in OpenFront Release Management
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/graphics/layers/PlayerPanel.ts`:
- Around line 505-507: The flag image is being suppressed when
Countries.find(...) fails and the else branch widens country to string; update
the logic in PlayerPanel.ts so the image src/rendering depends only on
activeFlagPath (render if activeFlagPath is truthy) and set the else branch for
country to undefined (not flagCode) so country remains Country|undefined; keep
country (from Countries.find(flagCode.split("/")[1])) only for title/alt (use
country?.name or conditional to supply labels) and remove the unnecessary typeof
country !== "string" runtime guard.
- Around line 502-506: The flag extraction logic around activeFlagPath and
flagCode in PlayerPanel.ts fails for hashed build asset names; update the
matching to extract the 2-letter country code before any hash instead of
splitting on "/"—for example run a case-insensitive regex match like
/([a-z]{2})(?:-[a-f0-9]+)?\.svg$/i against activeFlagPath and use the captured
group as the flagCode, then pass that flagCode directly into Countries.find((c)
=> c.code === flagCode); keep the safe fallback when no match is found so
country can be undefined.
🪄 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: e046b928-e3c5-4b81-bc5b-473329ce0f38
📒 Files selected for processing (1)
src/client/graphics/layers/PlayerPanel.ts
| const flagCode = activeFlagPath?.match(/\/([^.]+)\./)?.[1]; | ||
|
|
||
| // Find the country based on that code | ||
| const country = flagCode | ||
| ? Countries.find((c) => c.code === flagCode.split("/")[1]) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find where cosmetics.flag is assigned or read to understand the path format
rg -n "cosmetics[.\[]" --type ts -A 2 -B 2 | grep -i flag | head -40
# Also check cosmetics.json for flag entry shapes
fd "cosmetics.json" --exec cat {}Repository: openfrontio/OpenFrontIO
Length of output: 2640
🏁 Script executed:
# Check how the flag is used after extraction in PlayerPanel.ts
rg -n "flagCode" src/client/graphics/layers/PlayerPanel.ts -A 5 -B 2
# Check if there's validation for custom URLs elsewhere
rg -n "cosmetics.flag" src/client/graphics/layers/ -A 3 -B 1 | head -50Repository: openfrontio/OpenFrontIO
Length of output: 2042
🏁 Script executed:
# Find Countries definition and what codes it contains
rg -n "Countries\s*=" src/client --type ts -A 5 | head -30
# See how 'country' variable is used after line 506 in PlayerPanel.ts
rg -n "const country = " src/client/graphics/layers/PlayerPanel.ts -A 10Repository: openfrontio/OpenFrontIO
Length of output: 418
🏁 Script executed:
# Find the Countries import in PlayerPanel.ts
rg -n "^import|^from|Countries" src/client/graphics/layers/PlayerPanel.ts | head -20
# Find Countries definition in the entire codebase
rg -n "const Countries|export.*Countries" src/client --type ts -B 2 -A 3Repository: openfrontio/OpenFrontIO
Length of output: 939
🏁 Script executed:
# Read the countries.json file to see code structure
cat resources/countries.json | head -50Repository: openfrontio/OpenFrontIO
Length of output: 913
🏁 Script executed:
# Check GameView.ts flagAssignment and asset generation
rg -n "flag:" src/core/game/GameView.ts -B 3 -A 3
# Check if assetUrl or asset hashing changes the path before reaching PlayerPanel
rg -n "assetUrl|flagAssignments" src/client/graphics/layers/PlayerPanel.ts -B 2 -A 2
# Check core/AssetUrls to see how flag paths are transformed
cat src/core/AssetUrls.ts | head -40Repository: openfrontio/OpenFrontIO
Length of output: 2500
Flag code extraction breaks for hashed asset paths—improve regex to handle hash suffixes.
The current regex /\/([^.]+)\./ incorrectly handles bundled assets. For /_assets/flags/us-abc123.svg (which appears after build hashing), it captures _assets/flags/us-abc123, then split("/")[1] yields flags instead of the country code us. The Countries.find() silently fails because no entry has code "flags".
The proposed regex /([^/]+)\.[^.]+$/ extracts just the filename but still includes the hash suffix: for us-abc123.svg it captures us-abc123, which also fails the country lookup. Strip the hash portion using a pattern like /([a-z]{2})(?:-[a-f0-9]+)?\.svg$/ to extract the 2-letter code before any hash:
Suggested refactor
- const flagCode = activeFlagPath?.match(/\/([^.]+)\./)?.[1];
-
- // Find the country based on that code
- const country = flagCode
- ? Countries.find((c) => c.code === flagCode.split("/")[1])
- : flagCode;
+ // Extract the 2-letter code from paths like "/flags/us.svg" or "/_assets/flags/us-hash.svg"
+ const flagCode = activeFlagPath?.match(/([a-z]{2})(?:-[a-f0-9]+)?\.svg$/)?.[1];
+ const country = flagCode
+ ? Countries.find((c) => c.code === flagCode)
+ : 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/graphics/layers/PlayerPanel.ts` around lines 502 - 506, The flag
extraction logic around activeFlagPath and flagCode in PlayerPanel.ts fails for
hashed build asset names; update the matching to extract the 2-letter country
code before any hash instead of splitting on "/"—for example run a
case-insensitive regex match like /([a-z]{2})(?:-[a-f0-9]+)?\.svg$/i against
activeFlagPath and use the captured group as the flagCode, then pass that
flagCode directly into Countries.find((c) => c.code === flagCode); keep the safe
fallback when no match is found so country can be undefined.
| my: PlayerView, | ||
| myPanelClicked: boolean = false, | ||
| ) { | ||
| const activeFlagPath = myPanelClicked |
There was a problem hiding this comment.
why can't we alwasy use other? if you click on yourself wont other be yourself? then we dont' need myPanelClicked?
| const country = flagCode | ||
| ? Countries.find((c) => c.code === flagCode.split("/")[1]) |
There was a problem hiding this comment.
why do we need to do this lookup? Can we do what NameLayer.ts does?
|
|
Boostry123
force-pushed
the
fix/playerPanelFlag
branch
from
184d922 to
79a3dad
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/client/graphics/layers/PlayerPanel.ts (1)
499-504:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFlag code extraction still breaks for hashed/bundled asset paths.
The regex
/\/([^.]+)\./on a path like/_assets/flags/us-abc123.svgcaptures_assets/flags/us-abc123, soflagCode.split("/")[1]gives"flags"— which matches nothing inCountries.The
elsebranch also still assignsflagCode(astring | undefined) tocountry, widening its type tostring | Country | undefinedand making thetypeof country !== "string"guard necessary even though it is alwaystrueat runtime (whenflagCodeis falsy it isundefined, not"").Both problems are fixed together:
🛠️ Proposed fix
- const activeFlagPath = other.cosmetics.flag; - const flagCode = activeFlagPath?.match(/\/([^.]+)\./)?.[1]; - - // Find the country based on that code - const country = flagCode - ? Countries.find((c) => c.code === flagCode.split("/")[1]) - : flagCode; + const activeFlagPath = other.cosmetics.flag; + // Extract 2-letter code from "/flags/us.svg" or "/_assets/flags/us-abc123.svg" + const flagCode = activeFlagPath?.match(/\/([a-z]{2})(?:-[a-f0-9]+)?\.svg$/i)?.[1]; + const country = flagCode + ? Countries.find((c) => c.code === flagCode.toLowerCase()) + : undefined;Then in the template the
typeofguard can be dropped:- title=${typeof country !== "string" ? country?.name : country} - alt=${typeof country !== "string" ? country?.name : country} + title=${country?.name ?? flagCode ?? ""} + alt=${country?.name ?? flagCode ?? ""}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/client/graphics/layers/PlayerPanel.ts` around lines 499 - 504, activeFlagPath handling extracts the wrong segment and can produce a string in country; instead derive the basename and the short country code before extension and hyphenation, then look up Countries by that short code and set country to undefined when no code exists. Concretely, replace the regex-based flagCode extraction so it first obtains the path basename from activeFlagPath (last segment after "/"), strips the file extension, then splits on "-" and takes the first token as the flagCode; then compute country = flagCode ? Countries.find(c => c.code === flagCode) : undefined (using the existing Countries lookup and the existing country variable), and remove the unnecessary typeof country !== "string" guard in the template.
🤖 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/graphics/layers/PlayerPanel.ts`:
- Line 498: The declaration for activeFlagPath in PlayerPanel.ts is missing a
trailing semicolon which breaks Prettier CI; add the semicolon to the const
declaration (the identifier activeFlagPath where it is set to
other.cosmetics.flag) and then run npx prettier --write
src/client/graphics/layers/PlayerPanel.ts (or your repo-wide prettier) to
reformat and ensure no other formatting issues remain before merging.
---
Duplicate comments:
In `@src/client/graphics/layers/PlayerPanel.ts`:
- Around line 499-504: activeFlagPath handling extracts the wrong segment and
can produce a string in country; instead derive the basename and the short
country code before extension and hyphenation, then look up Countries by that
short code and set country to undefined when no code exists. Concretely, replace
the regex-based flagCode extraction so it first obtains the path basename from
activeFlagPath (last segment after "/"), strips the file extension, then splits
on "-" and takes the first token as the flagCode; then compute country =
flagCode ? Countries.find(c => c.code === flagCode) : undefined (using the
existing Countries lookup and the existing country variable), and remove the
unnecessary typeof country !== "string" guard in the template.
🪄 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: ed76ec06-48e3-4504-a7ea-d24109ab2f2e
📒 Files selected for processing (1)
src/client/graphics/layers/PlayerPanel.ts
| other: PlayerView, | ||
| my: PlayerView, | ||
| ) { | ||
| const activeFlagPath = other.cosmetics.flag |
There was a problem hiding this comment.
Missing semicolon on activeFlagPath declaration is still breaking Prettier CI.
Line 498 has no semicolon:
- const activeFlagPath = other.cosmetics.flag
+ const activeFlagPath = other.cosmetics.flag;Run npx prettier --write src/client/graphics/layers/PlayerPanel.ts to confirm all remaining formatting issues are cleared before merging.
📝 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.
| const activeFlagPath = other.cosmetics.flag | |
| const activeFlagPath = other.cosmetics.flag; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/client/graphics/layers/PlayerPanel.ts` at line 498, The declaration for
activeFlagPath in PlayerPanel.ts is missing a trailing semicolon which breaks
Prettier CI; add the semicolon to the const declaration (the identifier
activeFlagPath where it is set to other.cosmetics.flag) and then run npx
prettier --write src/client/graphics/layers/PlayerPanel.ts (or your repo-wide
prettier) to reformat and ensure no other formatting issues remain before
merging.
|
looks like ci is failing |
3 participants
Description:
Noticed the playerPanel should have had a flag, but it wasn't working.
Fixed plus added flag name when hovering.
Before:
After:
Please complete the following:
Resolves #123).translateText()and added strings toen.json.tests/directory.npm testpasses.Please put your Discord username so you can be contacted if a bug or regression is found:
Boostry