Conversation
|
|
|
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 Panama and Puerto Rico maps: new static map assets and manifests, registry and playlist entries, enum and category updates, many country and translation additions, and a test-localStorage mock change. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ 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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/core/game/Game.ts (1)
83-140:⚠️ Potential issue | 🟠 MajorPanama still needs the enum hook.
This PR also adds Panama assets and registers
panamainmap-generator/main.go, butGameMapTypeonly getsPuertoRico.BinaryLoaderGameMapLoader.ts:20-30builds the file name from this enum, soresources/maps/panamacannot be selected or loaded yet. Please addGameMapType.Panama(and its UI text), or leave Panama out of this PR.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/game/Game.ts` around lines 83 - 140, The GameMapType enum is missing a Panama entry causing BinaryLoaderGameMapLoader (which builds filenames from GameMapType) to be unable to load resources/maps/panama; add a new member to the GameMapType enum (e.g., Panama = "Panama") with the appropriate UI label so the map name matches the assets, or alternatively remove the Panama registration added in map-generator/main.go so the new assets aren't selectable until the enum/UI hook is added; update any place that enumerates GameMapType strings if necessary to include the new Panama value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.gitignore:
- Around line 17-30: Remove the personal ignore block labeled "#Personally Added
-- KCMitch" from the shared .gitignore and add those rules to your local Git
exclude or your global gitignore instead; also remove any ignore rule that
targets a file that is already tracked (so it has no effect) and commit the
cleaned .gitignore without the personal lines.
In `@resources/countries.json`:
- Around line 1815-1819: The JSON contains a duplicate country entry with
"code": "do" and "name": "República Dominicana" which conflicts with the
existing entry for the Dominican Republic; either remove this duplicate object
or assign it a unique "code" if it was intended to represent a different
country/flag, and ensure only one object uses the "code": "do" (verify against
the original Dominican Republic entry to keep the correct name/continent
fields).
In `@resources/lang/es.json`:
- Around line 418-420: The translation for the key "attitude" in
resources/lang/es.json is incorrect for the player_info_overlay context — it
should reflect diplomacy/stanced meaning, not physical orientation; update the
value for "attitude" from "Orientación" to "Actitud" (or another
diplomacy-focused term like "Diplomacia") and verify that any usages in
player_info_overlay reference the same key so the label reads correctly next to
hostile/friendly values.
In `@resources/maps/puertorico/manifest.json`:
- Around line 104-107: The Vieques nation entry has coordinates [3000, 480]
which exceed the declared map width (2392); update the "coordinates" array for
the object with "flag": "pr-vieques" and "name": "Vieques" in manifest.json so
the x value is <=2392 (or regenerate/clamp both x and y to the declared map
width/height), ensuring the spawn point lies inside the map bounds and keeping
the coordinates key as the array of two numbers.
- Line 17: The manifest currently sets the map fallback label incorrectly:
replace the value of manifest.name (the "name" property in manifest.json) from
"Pangaea" to the actual map name "Puerto Rico" so untranslated locales and
accessibility labels use the correct runtime fallback; update the "name" string
in the manifest.json object accordingly.
In `@src/server/MapPlaylist.ts`:
- Line 37: The frequency map contains an invalid key "Caribbean" which is not
part of GameMapName (derived from GameMapType); remove the "Caribbean: 3" entry
or add "Caribbean" to the GameMapType enum if it truly belongs there, and to
catch similar mistakes enable stricter typing by removing Partial from the
frequency declaration (change Partial<Record<GameMapName, number>> to
Record<GameMapName, number>) so TypeScript will surface missing/invalid keys;
locate the frequency object, the GameMapName type, and the GameMapType enum to
apply the change.
In `@tests/setup.ts`:
- Around line 2-5: The shared test setup currently defines
globalThis.localStorage but removed the canvas mock, causing failures where
tests call getContext("2d"); restore a canvas mock by defining
HTMLCanvasElement.prototype.getContext (or re-adding the previous canvas mock)
in the same setup file so that calls like canvas.getContext("2d") succeed;
specifically add a short shim on HTMLCanvasElement.prototype.getContext (and any
other methods used by tests, e.g., toDataURL) alongside the existing
Object.defineProperty(globalThis, "localStorage", ...) so tests like those
calling getContext("2d") no longer fail.
---
Outside diff comments:
In `@src/core/game/Game.ts`:
- Around line 83-140: The GameMapType enum is missing a Panama entry causing
BinaryLoaderGameMapLoader (which builds filenames from GameMapType) to be unable
to load resources/maps/panama; add a new member to the GameMapType enum (e.g.,
Panama = "Panama") with the appropriate UI label so the map name matches the
assets, or alternatively remove the Panama registration added in
map-generator/main.go so the new assets aren't selectable until the enum/UI hook
is added; update any place that enumerates GameMapType strings if necessary to
include the new Panama value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b8daa236-6d33-4b51-b083-948fe1ecef2b
⛔ Files ignored due to path filters (51)
map-generator/assets/maps/panama/image.pngis excluded by!**/*.pngmap-generator/assets/maps/puertorico/image.pngis excluded by!**/*.pngpackage-lock.jsonis excluded by!**/package-lock.jsonresources/flags/co-cho.svgis excluded by!**/*.svgresources/flags/co-cor.svgis excluded by!**/*.svgresources/flags/cr-h.svgis excluded by!**/*.svgresources/flags/cr-p.svgis excluded by!**/*.svgresources/flags/custom/frame.svgis excluded by!**/*.svgresources/flags/pa-10.svgis excluded by!**/*.svgresources/flags/pa-2.svgis excluded by!**/*.svgresources/flags/pa-3.svgis excluded by!**/*.svgresources/flags/pa-4.svgis excluded by!**/*.svgresources/flags/pa-5.svgis excluded by!**/*.svgresources/flags/pa-7.svgis excluded by!**/*.svgresources/flags/pa-ky.svgis excluded by!**/*.svgresources/flags/pa-nb.svgis excluded by!**/*.svgresources/flags/pr-ab.svgis excluded by!**/*.svgresources/flags/pr-adjuntas.svgis excluded by!**/*.svgresources/flags/pr-aibonito.svgis excluded by!**/*.svgresources/flags/pr-arecibo.svgis excluded by!**/*.svgresources/flags/pr-barceloneta.svgis excluded by!**/*.svgresources/flags/pr-barranquitas.svgis excluded by!**/*.svgresources/flags/pr-bayamon.svgis excluded by!**/*.svgresources/flags/pr-caguas.svgis excluded by!**/*.svgresources/flags/pr-carolina.svgis excluded by!**/*.svgresources/flags/pr-cr.svgis excluded by!**/*.svgresources/flags/pr-culebra.svgis excluded by!**/*.svgresources/flags/pr-dorado.svgis excluded by!**/*.svgresources/flags/pr-fajardo.svgis excluded by!**/*.svgresources/flags/pr-guaynabo.svgis excluded by!**/*.svgresources/flags/pr-humacao.svgis excluded by!**/*.svgresources/flags/pr-jayuya.svgis excluded by!**/*.svgresources/flags/pr-lares.svgis excluded by!**/*.svgresources/flags/pr-luquillo.svgis excluded by!**/*.svgresources/flags/pr-maricao.svgis excluded by!**/*.svgresources/flags/pr-maunabo.svgis excluded by!**/*.svgresources/flags/pr-mayaguez.svgis excluded by!**/*.svgresources/flags/pr-ponce.svgis excluded by!**/*.svgresources/flags/pr-salinas.svgis excluded by!**/*.svgresources/flags/pr-sg.svgis excluded by!**/*.svgresources/flags/pr-sj.svgis excluded by!**/*.svgresources/flags/pr-ss.svgis excluded by!**/*.svgresources/flags/pr-utuado.svgis excluded by!**/*.svgresources/flags/pr-vieques.svgis excluded by!**/*.svgresources/flags/rd.svgis excluded by!**/*.svgresources/maps/panama/map.binis excluded by!**/*.binresources/maps/panama/map16x.binis excluded by!**/*.binresources/maps/panama/map4x.binis excluded by!**/*.binresources/maps/puertorico/map.binis excluded by!**/*.binresources/maps/puertorico/map16x.binis excluded by!**/*.binresources/maps/puertorico/map4x.binis excluded by!**/*.bin
📒 Files selected for processing (16)
.gitignoremap-generator/assets/maps/panama/info.jsonmap-generator/assets/maps/puertorico/info.jsonmap-generator/main.gomap-generator/map-generatorresources/countries.jsonresources/lang/en.jsonresources/lang/es.jsonresources/maps/panama/manifest.jsonresources/maps/panama/thumbnail.webpresources/maps/puertorico/manifest.jsonresources/maps/puertorico/thumbnail.webpsrc/core/game/Game.tssrc/server/MapPlaylist.tstests/InputHandler.test.tstests/setup.ts
tests/setup.ts
Outdated
| Object.defineProperty(globalThis, "localStorage", { | ||
| value: window.localStorage, | ||
| configurable: true, | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -e
echo "Current shared test setup:"
sed -n '1,20p' tests/setup.ts
echo
echo "Canvas-dependent tests:"
rg -n 'getContext\("2d"\)|createElement\("canvas"\)' tests/clientRepository: openfrontio/OpenFrontIO
Length of output: 450
🏁 Script executed:
cat -n tests/client/graphics/UILayer.test.ts | sed -n '40,60p'Repository: openfrontio/OpenFrontIO
Length of output: 874
🏁 Script executed:
rg -n "canvas|getContext" tests/client/graphics/UILayer.test.tsRepository: openfrontio/OpenFrontIO
Length of output: 212
🏁 Script executed:
# Check if vitest-canvas-mock is available in package.json or node_modules
rg "vitest-canvas-mock" . --type json --type jsRepository: openfrontio/OpenFrontIO
Length of output: 354
🏁 Script executed:
# Check what's currently in the full setup.ts
cat -n tests/setup.tsRepository: openfrontio/OpenFrontIO
Length of output: 261
Restore the canvas mock in the shared test setup.
This adds localStorage, but drops the shared canvas setup. Tests like tests/client/graphics/ProgressBar.test.ts:8-11 and tests/client/graphics/UILayer.test.ts:32-36 use canvas methods like getContext("2d"), so they will fail without the mock in the jsdom environment.
Suggested fix
+import "vitest-canvas-mock";
+
// Add global mocks or configuration here if needed
Object.defineProperty(globalThis, "localStorage", {
value: window.localStorage,
configurable: true,
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/setup.ts` around lines 2 - 5, The shared test setup currently defines
globalThis.localStorage but removed the canvas mock, causing failures where
tests call getContext("2d"); restore a canvas mock by defining
HTMLCanvasElement.prototype.getContext (or re-adding the previous canvas mock)
in the same setup file so that calls like canvas.getContext("2d") succeed;
specifically add a short shim on HTMLCanvasElement.prototype.getContext (and any
other methods used by tests, e.g., toDataURL) alongside the existing
Object.defineProperty(globalThis, "localStorage", ...) so tests like those
calling getContext("2d") no longer fail.
github-project-automation
bot
moved this from Triage
to Development
in OpenFront Release Management
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/server/MapPlaylist.ts (1)
40-40:⚠️ Potential issue | 🔴 CriticalRemove or rename the
Caribbeanfrequency entry.Line 40 uses a key that is not in
GameMapType, so this object does not type-check and CI is already failing. Use a validGameMapNamehere, or drop this entry until that map exists.Possible minimal fix
- Caribbean: 3,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/MapPlaylist.ts` at line 40, The object in MapPlaylist contains an invalid key "Caribbean" which is not part of GameMapType/GameMapName and breaks type-checking; remove the "Caribbean" entry or rename it to a valid GameMapName used by your types, and ensure the map key aligns with the GameMapType type (update the MapPlaylist mapping where "Caribbean" appears so the key matches an existing GameMapName or delete the key if the map doesn't exist yet).resources/countries.json (1)
1816-1820:⚠️ Potential issue | 🟠 MajorDo not add a second
docountry code.Line 1817 reuses the code that already exists at Line 635. That makes the flag list ambiguous, and both rows point to the same
/flags/do.svgasset, so this new entry cannot behave like a separate option.Possible fix
- { - "code": "do", - "continent": "North America", - "name": "República Dominicana" - },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/countries.json` around lines 1816 - 1820, The JSON entry duplicates the existing country code "do" (code field) — remove or correct this duplicate: either delete this object with name "República Dominicana" and code "do" so only the original entry remains, or if this entry was meant to represent a different country, change the "code" value to a unique ISO code and update the "name" accordingly; locate the duplicate by searching for the object with "code": "do" and "name": "República Dominicana".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@resources/countries.json`:
- Around line 1816-1820: The JSON entry duplicates the existing country code
"do" (code field) — remove or correct this duplicate: either delete this object
with name "República Dominicana" and code "do" so only the original entry
remains, or if this entry was meant to represent a different country, change the
"code" value to a unique ISO code and update the "name" accordingly; locate the
duplicate by searching for the object with "code": "do" and "name": "República
Dominicana".
In `@src/server/MapPlaylist.ts`:
- Line 40: The object in MapPlaylist contains an invalid key "Caribbean" which
is not part of GameMapType/GameMapName and breaks type-checking; remove the
"Caribbean" entry or rename it to a valid GameMapName used by your types, and
ensure the map key aligns with the GameMapType type (update the MapPlaylist
mapping where "Caribbean" appears so the key matches an existing GameMapName or
delete the key if the map doesn't exist yet).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 42a572ec-a26d-4e08-9363-5c5d03b2fd67
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (6)
map-generator/main.goresources/countries.jsonresources/lang/en.jsonsrc/core/game/Game.tssrc/server/MapPlaylist.tstests/InputHandler.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/core/game/Game.ts
- map-generator/main.go
- resources/lang/en.json
- tests/InputHandler.test.ts
Changed the Coordinates to Vieques to within the Map range
Removed personal .gitignore lines that leaked through
Fixed the name from a carried over manifest.json file.
Removed duplicate country code
Removed Non-Existent Map Name
RaptorJesus67
left a comment
RaptorJesus67
left a comment
There was a problem hiding this comment.
Fixed all the Critical Errors holding it up.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/maps/puertorico/manifest.json`:
- Around line 18-159: The manifest references 28 Puerto Rico municipality flags
(pr-*) but only pr-sj exists in countries.json, causing FlagSchema validation
failures because countryCodes is built from countries.json; add the missing 27
entries to countries.json matching the existing pattern used for other
territories (e.g., es-ct, gb-eng, pa-*) so each code (pr-ab, pr-adjuntas,
pr-aibonito, pr-arecibo, pr-barceloneta, pr-barranquitas, pr-bayamon, pr-caguas,
pr-carolina, pr-cr, pr-culebra, pr-dorado, pr-fajardo, pr-guaynabo, pr-humacao,
pr-jayuya, pr-lares, pr-luquillo, pr-maricao, pr-maunabo, pr-mayaguez, pr-ponce,
pr-salinas, pr-sg, pr-ss, pr-utuado, pr-vieques) is present with a matching flag
code and display name; follow the same object shape and metadata used by
existing entries so FlagSchema and countryCodes include these flags (SVG assets
already in resources/flags/).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1fd73f22-3bf9-41b0-9f3f-8a01aad6b413
📒 Files selected for processing (4)
resources/countries.jsonresources/lang/es.jsonresources/maps/puertorico/manifest.jsonsrc/server/MapPlaylist.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/server/MapPlaylist.ts
- resources/lang/es.json
Added missing Puerto Rico countries
Added more missing Puerto Rican nations to countries.json
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)
resources/countries.json (1)
16-2564:⚠️ Potential issue | 🟠 MajorNine new country codes are missing corresponding SVG flag files, which will silently fall back to
xx.svg.The integrity check found these
codevalues inresources/countries.jsonhave no matching file inresources/flags/:
pr-agJatibonicugb-hnfl-crEmpire of Haitifr-gpcu-lhcu-ormx-yucThe UI code in
src/client/FlagInputModal.tsandsrc/client/graphics/layers/PlayerPanel.tsbuilds paths like/flags/${country.code}.svgand silently hides the image on load failure. Without the matching SVG files, these entries will degrade to the fallback flag (xx.svg) with no error logged.Either add the missing flag SVG files to
resources/flags/or remove the entries fromresources/countries.jsonbefore merging.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/countries.json` around lines 16 - 2564, The countries.json contains nine entries whose flag SVGs are missing (codes: pr-ag, Jatibonicu, gb-hn, fl-cr, Empire of Haiti, fr-gp, cu-lh, cu-or, mx-yuc) which causes silent fallback to xx.svg when FlagInputModal.ts and PlayerPanel.ts request /flags/${country.code}.svg; fix by either adding the corresponding SVG files named exactly as the codes into resources/flags/ or removing those code objects from resources/countries.json, and ensure filenames match the code strings used by the UI (/flags/{code}.svg).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@resources/countries.json`:
- Around line 16-2564: The countries.json contains nine entries whose flag SVGs
are missing (codes: pr-ag, Jatibonicu, gb-hn, fl-cr, Empire of Haiti, fr-gp,
cu-lh, cu-or, mx-yuc) which causes silent fallback to xx.svg when
FlagInputModal.ts and PlayerPanel.ts request /flags/${country.code}.svg; fix by
either adding the corresponding SVG files named exactly as the codes into
resources/flags/ or removing those code objects from resources/countries.json,
and ensure filenames match the code strings used by the UI (/flags/{code}.svg).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b258e110-4383-42d7-9445-a32156a431f5
📒 Files selected for processing (1)
resources/countries.json
|
I'm trying to get the map added to the game, but am I missing something? Is there any other code I need to fix to satisfy a merge? |
|
Hi, there are some issues: You are editing files which need no edit: There are also several panama files you added, but this PR is about puerto rico? And sorry, but I dont really like this map because it doesn't really offer "new gameplay"? Its an "one main island map" similar to iceland or australia (and others). The only difference are the rivers. But thats just my opinion :) |
3 participants
Description:
Adding the Map of Puerto Rico and the appropriate flags for each nation. There should be 28 nations with "pr-xxx.svg" nomenclature.
Please complete the following:
Feel free to delete any files that might conflict (i.e.
.gitignore). I've tested the map and it works well and the nations seem appropriately spaced and not to crammed or placed too sparse.Please put your Discord username so you can be contacted if a bug or regression is found:
DISCORD_USERNAME -> KCMitch