Conversation
Currently just adds icon game item for oil rig, it basically either has no effect everywhere or just d oes the same thing as a port, for example it shares port artwork right now
|
|
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds an Oil Rig feature and a "World (+oil)" map: map assets and generator tooling, oil terrain and tile classification, oil-rig execution spawning freight trains for periodic gold, client UI and rendering updates, configuration and defaults, and tests for placement, income, and assets. Changes
Sequence Diagram(s)sequenceDiagram
participant OilRig as OilRigExecution
participant Station as TrainStation
participant Rail as RailNetworkImpl
participant Factory as FactoryExecution
participant Owner as Player
rect rgba(100,150,200,0.5)
Note over OilRig,Owner: Oil rig periodic income flow
OilRig->>OilRig: tick() at configured interval
OilRig->>Station: ensure station exists / create if missing
Station-->>OilRig: station reference
OilRig->>Rail: get cluster for station
Rail-->>OilRig: cluster
OilRig->>Rail: find nearest owned factory in cluster
Rail-->>OilRig: factory station (if any)
OilRig->>OilRig: spawn TrainExecution(mission: "freight", loaded: true)
end
rect rgba(150,200,100,0.5)
Note over Factory,Owner: Freight delivery & payment
Factory->>Factory: onTrainStop() for freight
Factory->>Owner: addGold(amount, tile, icon: "oil")
Owner-->>Factory: emit BonusEventUpdate(icon: "oil")
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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: 2
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
src/core/execution/nation/NationStructureBehavior.ts (2)
459-462:⚠️ Potential issue | 🟠 MajorSample oil tiles directly for Oil Rig placement.
OilRigcurrently samples 25 random territory tiles and then filters withcanBuild(). Since oil deposits are sparse and Oil Rigs must be built on oil terrain, nations can fail to build even when they own valid oil tiles. Sample owned oil tiles for this unit type.Proposed fix
const tiles = type === UnitType.Port ? this.randCoastalTileArray(25) + : type === UnitType.OilRig + ? this.randOilTileArray(25) : randTerritoryTileArray(this.random, this.game, this.player, 25);private randCoastalTileArray(numTiles: number): TileRef[] { const shared = this._sharedWaterComponents; const tiles = Array.from(this.player.borderTiles()).filter((t) => { if (!this.game.isShore(t)) return false; if (shared === null) return false; @@ return Array.from(this.arraySampler(tiles, numTiles)); } + + private randOilTileArray(numTiles: number): TileRef[] { + const tiles = Array.from(this.player.tiles()).filter( + (tile) => this.game.terrainType(tile) === TerrainType.Oil, + ); + return Array.from(this.arraySampler(tiles, numTiles)); + }Also applies to: 591-617
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/execution/nation/NationStructureBehavior.ts` around lines 459 - 462, The current selection of candidate tiles for building structures uses a generic territory sampler (randTerritoryTileArray) and then relies on canBuild(), which causes OilRig to miss sparse oil tiles; update the logic in NationStructureBehavior where tiles is chosen (the branch using UnitType.Port and the cases handling OilRig around the 591-617 region) so that when the unit type is OilRig you sample only tiles that are both owned by this.player and have oil (e.g., add/use a helper like randOwnedOilTileArray(count) or filter randTerritoryTileArray(this.random, this.game, this.player, count) by tile.hasOil && tile.owner===this.player) before calling canBuild(), ensuring OilRig candidates are valid oil-bearing owned tiles.
116-116:⚠️ Potential issue | 🔴 CriticalFix the release-blocking
sharedWaterComponentscrash.CI shows
handleStructures()throws before any nation structure logic runs becausethis.game.sharedWaterComponentsis missing in the active game object or test mock. Please add/align theGameimplementation and mocks, or switch this code to the existing water-component API.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/execution/nation/NationStructureBehavior.ts` at line 116, handleStructures is crashing because NationStructureBehavior uses this.game.sharedWaterComponents which doesn't exist on the active Game/mocks; either add/align that method on the Game class and all test mocks (implement sharedWaterComponents(player): WaterComponent[] and return appropriate mock data) or change NationStructureBehavior to call the existing water-component API used elsewhere (e.g., replace this.game.sharedWaterComponents(this.player) with the canonical method your codebase exposes such as this.game.getSharedWaterComponents(this.player) or this.game.getWaterComponents(this.player)); ensure tests/mocks are updated to match whichever API you choose so this._sharedWaterComponents is always defined before handleStructures runs.tests/core/game/TrainStation.test.ts (2)
88-99:⚠️ Potential issue | 🟡 MinorUse separate owners so this test covers the ally path.
Right now the station owner and train owner are the same
playermock, so the test can pass through self-trade and never prove thatisAlliedWith()drives the ally branch.Proposed test strengthening
it("handles allied trade", () => { + const stationOwner = { + addGold: vi.fn(), + id: 1, + canTrade: vi.fn().mockReturnValue(true), + isOnSameTeam: vi.fn().mockReturnValue(false), + isAlliedWith: vi.fn().mockReturnValue(true), + } as any; + const trainOwner = { + addGold: vi.fn(), + id: 2, + canTrade: vi.fn().mockReturnValue(true), + isOnSameTeam: vi.fn().mockReturnValue(false), + isAlliedWith: vi.fn().mockReturnValue(true), + } as any; unit.type.mockReturnValue(UnitType.City); - player.isAlliedWith.mockReturnValue(true); + unit.owner.mockReturnValue(stationOwner); + trainExecution.owner.mockReturnValue(trainOwner); const station = new TrainStation(game, unit); station.onTrainStop(trainExecution); - expect(unit.owner().addGold).toHaveBeenCalledWith(1000n, unit.tile()); - expect(trainExecution.owner().addGold).toHaveBeenCalledWith( + expect(stationOwner.addGold).toHaveBeenCalledWith(1000n, unit.tile()); + expect(trainOwner.addGold).toHaveBeenCalledWith( 1000n, unit.tile(), );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/core/game/TrainStation.test.ts` around lines 88 - 99, The test uses the same mock `player` for both the station owner and the train owner so it can pass via self-trade; change the setup so the station's owner and `trainExecution.owner()` are different mock players (e.g., `stationOwner` and `trainOwner`), keep `player.isAlliedWith` behavior targeted so `stationOwner.isAlliedWith(stationOwner, trainOwner)` (or the equivalent mock) returns true, then construct `new TrainStation(game, unit)` with `stationOwner` and set `trainExecution.owner()` to `trainOwner`; this ensures the `TrainStation.onTrainStop` ally branch (use of `player.isAlliedWith`) is exercised and both `unit.owner().addGold` and `trainExecution.owner().addGold` are asserted called.
51-60:⚠️ Potential issue | 🟡 MinorRemove the duplicate mock keys from the player object.
Lines 58–59 repeat
isOnSameTeamandisAlliedWiththat are already declared on lines 55–56. In object literals, the second declarations silently overwrite the first ones, making the code redundant.Suggested fix
player = { addGold: vi.fn(), id: 1, canTrade: vi.fn().mockReturnValue(true), isAlliedWith: vi.fn().mockReturnValue(false), isOnSameTeam: vi.fn().mockReturnValue(false), isFriendly: vi.fn().mockReturnValue(false), - isOnSameTeam: vi.fn().mockReturnValue(false), - isAlliedWith: vi.fn().mockReturnValue(false), } as any;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/core/game/TrainStation.test.ts` around lines 51 - 60, The player test fixture contains duplicate mock properties for isOnSameTeam and isAlliedWith on the player object; remove the repeated declarations so each mock (isOnSameTeam, isAlliedWith, and the other mocks like canTrade, isFriendly) is defined only once in the player object used by TrainStation.test.ts (locate the player = { ... } block) to avoid redundant overwrites and keep the single intended mock implementations.tests/economy/ConstructionGold.test.ts (1)
1-144:⚠️ Potential issue | 🟡 MinorRun Prettier so CI can pass.
The new test looks useful, but the pipeline reports a Prettier mismatch for this file.
#!/bin/bash # Format only the changed test file. npx prettier --write tests/economy/ConstructionGold.test.ts🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/economy/ConstructionGold.test.ts` around lines 1 - 144, Run Prettier to fix formatting in the new test file tests/economy/ConstructionGold.test.ts so CI passes; format the file (e.g. run `npx prettier --write tests/economy/ConstructionGold.test.ts` or your project's formatting script) and commit the formatted changes — the failing file contains the "Construction economy" tests and symbols like ConstructionExecution, NukeExecution, SpawnExecution, and the test cases "City charges gold once..." and "MIRV gets more expensive..." referenced in the diff.map-generator/map_generator.go (1)
85-87:⚠️ Potential issue | 🟡 MinorUpdate the pixel-decoding comment for RGB oil detection.
The comment still says red/green values have no impact, but oil detection now uses red, green, and blue. This can mislead map authors.
📝 Proposed comment fix
-// Red/green pixel values have no impact, only blue values are used -// For Land tiles, "Magnitude" is determined by `(Blue - 140) / 2“. +// Near-black red/green/blue pixels are reserved for oil. +// For other Land tiles, "Magnitude" is determined by `(Blue - 140) / 2`.Also applies to: 94-94
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@map-generator/map_generator.go` around lines 85 - 87, The comment above the pixel-decoding logic incorrectly states that red/green values have no impact; update the comment near the pixel decoding in map_generator.go (the lines describing "Magnitude" and blue-based calculations) to explain that oil detection uses red, green, and blue channels and describe how each channel is interpreted (e.g., which channel(s) indicate oil presence/strength versus land/water magnitude) so map authors aren’t misled by the old “red/green have no impact” note; ensure the updated comment also appears where the similar note exists (around the second occurrence at line ~94).src/core/game/PlayerImpl.ts (1)
908-918:⚠️ Potential issue | 🟡 MinorUse an explicit tile check so tile
0still emits bonus events.
if (tile)skips valid tile0, so gold popups from that tile will not be sent.Proposed fix
- if (tile) { + if (tile !== undefined) { this.mg.addUpdate({ type: GameUpdateType.BonusEvent, player: this.id(),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/game/PlayerImpl.ts` around lines 908 - 918, The addGold method uses a truthy check "if (tile)" which skips valid tile 0; change the conditional in PlayerImpl.addGold to an explicit null/undefined check (e.g., if (tile !== undefined && tile !== null) or if (typeof tile !== 'undefined')) so a tile value of 0 still triggers the mg.addUpdate BonusEvent with tile, gold and icon; keep the rest of the payload the same.
🟠 Major comments (24)
resources/lang/hu.json-102-103 (1)
102-103:⚠️ Potential issue | 🟠 MajorPlease move Hungarian translation changes to dedicated localization PRs.
Line 102 and Line 103 add non-English locale values in this feature PR. Please revert these here and ship the key additions through
en.json; translators can then updatehu.jsonin the localization workflow.Based on learnings: OpenFrontIO handles non-English locale updates in dedicated translation PRs, while regular PRs update only
en.json.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/lang/hu.json` around lines 102 - 103, Revert the Hungarian translations for the keys "build_oil_rig" and "build_oil_rig_desc" from this PR and instead add those keys with English values to en.json; keep only the key additions in this feature PR (no non-English locale changes), then open a separate localization PR where translators will update hu.json with the Hungarian strings.resources/lang/tp.json-74-75 (1)
74-75:⚠️ Potential issue | 🟠 MajorRoute these locale additions through translation PRs only.
Line 74 and Line 75 modify a non-English locale file in a feature PR. Please keep this PR to
en.jsonkey/value changes and move these locale strings to the translation team workflow.Based on learnings: For OpenFrontIO, non-English files in
resources/lang/*.jsonare handled by dedicated translation PRs, not regular feature PRs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/lang/tp.json` around lines 74 - 75, Remove the non-English locale additions from this feature PR by reverting the changes to the tp.json keys "build_oil_rig" and "build_oil_rig_desc" so only en.json is modified in this branch; then create a separate translation PR (or submit the strings to the translation workflow) containing these two keys and their translated values so the translation team can review and merge them via the dedicated translation process.resources/lang/de-CH.json-159-160 (1)
159-160:⚠️ Potential issue | 🟠 MajorPlease remove non-English locale edits from this feature PR.
Line 159 and Line 160 add
de-CHtranslations directly. This should be split out into the dedicated translation process; feature PRs should keep localization key changes inen.jsononly.Based on learnings: In OpenFrontIO, only dedicated translation PRs should update non-English locale files under
resources/lang/.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/lang/de-CH.json` around lines 159 - 160, Remove the non-English entries added in this feature PR by deleting the "build_oil_rig" and "build_oil_rig_desc" keys from resources/lang/de-CH.json (they should exist only in en.json); leave the localization key additions in the English locale and submit any German translations via the dedicated translation PR/process instead of modifying de-CH.json in this feature branch.resources/lang/hi.json-71-72 (1)
71-72:⚠️ Potential issue | 🟠 MajorPlease avoid updating
resources/lang/hi.jsonin this PR.Line 71 and Line 72 introduce Hindi translations directly in a feature PR. Per project process, keep only
en.jsonupdates here and move non-English locale updates to the translation PR path.Based on learnings: In OpenFrontIO, translation files in
resources/lang/*.jsonexcepten.jsonare updated only in dedicated translation PRs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/lang/hi.json` around lines 71 - 72, This PR added Hindi locale keys "build_oil_rig" and "build_oil_rig_desc" in resources/lang/hi.json but non-English locale changes must not be included here; revert the changes in resources/lang/hi.json so it matches the branch baseline, ensure the new strings exist only in resources/lang/en.json (using the keys "build_oil_rig" and "build_oil_rig_desc"), and open or move the Hindi translations into the dedicated translation PR/path for localization updates.resources/lang/uk.json-159-160 (1)
159-160:⚠️ Potential issue | 🟠 MajorThis non-English locale update should be split out.
Line 159 and Line 160 add Ukrainian translation content in a feature PR. Please keep this PR focused on English source keys and move locale translations to the project’s translation PR workflow.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/lang/uk.json` around lines 159 - 160, The PR added Ukrainian locale entries "build_oil_rig" and "build_oil_rig_desc" to resources/lang/uk.json which must be removed from this feature PR; revert or remove these two keys from this branch and instead submit them via the project's translation workflow (a separate translation PR) so this feature PR contains only English source keys and code changes. Ensure you reference and remove the exact keys "build_oil_rig" and "build_oil_rig_desc" when reverting the change before opening the dedicated translation PR with the Ukrainian translations.resources/lang/ca.json-156-157 (1)
156-157:⚠️ Potential issue | 🟠 MajorKeep translation content in dedicated translation flow.
Line 156 and Line 157 add Catalan translations in a feature PR. Please keep technical key rollout separate and let community translation updates happen in the dedicated localization process.
Based on learnings: For OpenFrontIO, technical changes and translation content updates are separated so native translators can update locale values in dedicated translation work.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/lang/ca.json` around lines 156 - 157, Remove the Catalan translations from the PR and keep only the technical keys so localization stays in the translation workflow: revert the values for the keys "build_oil_rig" and "build_oil_rig_desc" to their default/placeholder (e.g., English strings or empty strings) instead of adding localized Catalan text, so the keys are present but actual translations are left to the dedicated localization process; do not add new translated content in this feature change.resources/lang/cs.json-85-86 (1)
85-86:⚠️ Potential issue | 🟠 MajorPlease move non-English translation edits out of this feature PR.
Line 85 and Line 86 add Czech locale content. In this repo workflow, regular feature PRs should only update
resources/lang/en.json; non-English locale files are handled in dedicated translation PRs. Please revert these lines here and let translation follow-up handle them.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 locales.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/lang/cs.json` around lines 85 - 86, Revert the non-English additions to resources/lang/cs.json by removing the "build_oil_rig" and "build_oil_rig_desc" keys (the Czech translations added in this PR), restoring that file to its prior state; ensure the only locale changes in this feature PR are made in resources/lang/en.json and leave foreign-language updates to a dedicated translation PR.resources/lang/sh.json-71-72 (1)
71-72:⚠️ Potential issue | 🟠 MajorPlease keep
sh.jsonout of this feature PR.Line 71 and Line 72 add Serbo-Croatian translations directly. In this repository, this should be handled by the dedicated translation process, not mixed into feature delivery PRs.
Based on learnings: For OpenFrontIO, translation files in
resources/lang/*.jsonexcepten.jsonare handled by a dedicated translation team and workflow.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/lang/sh.json` around lines 71 - 72, This PR erroneously adds Serbo-Croatian translations ("build_oil_rig" and "build_oil_rig_desc") directly; revert the additions for those keys from the changeset (remove "build_oil_rig" and "build_oil_rig_desc" entries) and ensure no other non-en JSON language files are modified, then update the PR description or commit message to state translations must be submitted via the dedicated translation workflow and notify the translation team so they can add these keys through the proper process.resources/lang/nl.json-131-132 (1)
131-132:⚠️ Potential issue | 🟠 MajorDutch locale additions should be deferred to translation PRs
Line 131 and Line 132 update
resources/lang/nl.jsonin a regular feature PR. Please keep this change toen.jsonhere and move Dutch text to the translation workflow PR.Based on learnings: non-English locale files are maintained by the dedicated translation flow; regular PRs should update only
en.json.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/lang/nl.json` around lines 131 - 132, This change improperly updates the Dutch locale file resources/lang/nl.json; remove the Dutch entries for "build_oil_rig" and "build_oil_rig_desc" from this feature PR and instead keep only the corresponding English keys in en.json, deferring any nl.json translations to the dedicated translation workflow/PR so non-English locale files remain untouched in this branch.resources/lang/es.json-85-86 (1)
85-86:⚠️ Potential issue | 🟠 MajorSpanish locale changes should not be in this regular feature PR
Line 85 and Line 86 add Spanish translations directly. This should be handled by the dedicated translation process; keep this PR limited to
en.jsonfor new keys.Based on learnings: in OpenFrontIO, only
en.jsonshould be updated in regular PRs; non-English locale updates are reserved for dedicated translation PRs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/lang/es.json` around lines 85 - 86, Remove the Spanish entries "build_oil_rig" and "build_oil_rig_desc" from resources/lang/es.json and instead add the new keys and English text to resources/lang/en.json (use the same key names: build_oil_rig and build_oil_rig_desc); keep this feature PR limited to en.json only and leave non-English locale updates to the dedicated translation workflow.resources/lang/fi.json-102-103 (1)
102-103:⚠️ Potential issue | 🟠 MajorFinnish locale changes should follow the translation-only PR path
Line 102 and Line 103 add Finnish translations in a feature PR. This repo expects non-English locale updates in dedicated translation PRs, not in gameplay feature PRs.
Based on learnings: regular PRs should modify
en.jsononly; updates to other locale files are handled separately by the translation workflow.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/lang/fi.json` around lines 102 - 103, The Finnish locale additions for keys "build_oil_rig" and "build_oil_rig_desc" were added in a feature PR but this repo requires non-English locale changes to go through the translation-only PR path; revert the changes to resources/lang/fi.json in this branch (remove the "build_oil_rig" and "build_oil_rig_desc" entries) and ensure only the corresponding en.json key(s) are touched in this feature PR, then open a separate translation PR for the Finnish translations referencing these keys.resources/lang/sk.json-102-103 (1)
102-103:⚠️ Potential issue | 🟠 MajorSlovak translation updates should be excluded from this feature PR
Line 102 and Line 103 modify
resources/lang/sk.json, which is translator-managed. Please keep onlyen.jsonkey additions in this PR and route Slovak text through the dedicated localization process.Based on learnings: non-English locale files are not updated in regular PRs; dedicated translation PRs handle those changes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/lang/sk.json` around lines 102 - 103, Revert the Slovak locale edits for the keys "build_oil_rig" and "build_oil_rig_desc" in resources/lang/sk.json so this PR only contains the English key additions; ensure the en.json contains the new keys/strings and remove or undo any changes to resources/lang/sk.json (leave it unchanged) so localization updates are routed through the dedicated translation PR/process.resources/lang/fr.json-159-160 (1)
159-160:⚠️ Potential issue | 🟠 MajorFrench translation additions should be handled outside this feature PR
Line 159 and Line 160 update a non-English locale file directly. Please move this to the dedicated translation pipeline and keep only
en.jsonkey updates in this PR.Based on learnings: non-English files under
resources/lang/*.jsonare maintained via dedicated translation PRs, while regular PRs should updateen.jsononly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/lang/fr.json` around lines 159 - 160, Revert the direct edits to the French locale keys "build_oil_rig" and "build_oil_rig_desc" in resources/lang/fr.json and remove them from this PR; instead add or keep the corresponding keys updated only in resources/lang/en.json, and submit the French translations through the dedicated translation pipeline/translation PR process so non-English locale changes are not included in this feature branch.resources/lang/fa.json-112-113 (1)
112-113:⚠️ Potential issue | 🟠 MajorMove Persian locale changes to the translation PR workflow
Line 112 and Line 113 add Persian strings in this feature PR. Per repo workflow, this should not be done in regular PRs; keep the key additions in
en.jsonand let the translation process updatefa.json.Based on learnings: regular PRs should only update
en.json; non-Englishresources/lang/*.jsonupdates are done in dedicated translation PRs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/lang/fa.json` around lines 112 - 113, The Persian locale file has direct additions "build_oil_rig" and "build_oil_rig_desc" in fa.json which violates the translation workflow; revert or remove these two keys from resources/lang/fa.json in this PR and ensure the corresponding key additions exist only in en.json (add them there if missing) so that language maintainers can update fa.json in a dedicated translation PR.resources/lang/et.json-156-157 (1)
156-157:⚠️ Potential issue | 🟠 MajorPlease avoid updating translator-managed locale files in this PR
Line 156 and Line 157 modify Estonian locale content in a normal feature PR. This should be limited to
en.jsonhere and moved to the dedicated translation flow for non-English locales.Based on learnings: non-English files in
resources/lang/*.jsonare handled by dedicated translation PRs, while regular PRs should update onlyen.json.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/lang/et.json` around lines 156 - 157, Revert the edits to the Estonian locale keys "build_oil_rig" and "build_oil_rig_desc" (undo changes to those keys in the non-English locale) and ensure the feature text is added only to the English locale key equivalents in en.json; if translations are needed, leave the non-English keys unchanged and submit the new strings via the dedicated translation workflow so translators can update "build_oil_rig" and "build_oil_rig_desc" in the translation PR instead.resources/lang/eo.json-102-103 (1)
102-103:⚠️ Potential issue | 🟠 MajorNon-English locale update should be moved out of this feature PR
Line 102 and Line 103 add Esperanto strings in a regular feature PR. In this repo, non-English locale files are handled by the translation workflow, so this should stay in
en.jsononly here and be synced later in the dedicated translation process.Based on learnings: In OpenFrontIO,
resources/lang/*.json(excepten.json) should not be updated in regular PRs; non-English updates are handled in dedicated translation PRs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/lang/eo.json` around lines 102 - 103, The Esperanto strings "build_oil_rig" and "build_oil_rig_desc" were added in this feature PR but non-English locale files must not be changed here; revert the changes in resources/lang/eo.json (remove the two keys/values) so this PR only updates en.json, and ensure new string keys are present in the English file (e.g., "build_oil_rig" and "build_oil_rig_desc") to be picked up by the translation workflow later.src/core/execution/nation/NationStructureBehavior.ts-141-146 (1)
141-146:⚠️ Potential issue | 🟠 MajorDo not skip Oil Rigs when there are no coastal tiles.
Oil rigs are placed on oil terrain, not shore tiles. This gate blocks nations with inland oil deposits from ever trying to build one.
Proposed fix
- // Skip coastal structures if no coastal tiles + // Skip coastal structures if no coastal tiles if ( - (structureType === UnitType.Port || - structureType === UnitType.OilRig) && + structureType === UnitType.Port && !hasCoastalTiles ) { continue; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/execution/nation/NationStructureBehavior.ts` around lines 141 - 146, The current gate incorrectly skips both UnitType.Port and UnitType.OilRig when hasCoastalTiles is false; change the condition in NationStructureBehavior (the block that checks structureType and hasCoastalTiles) to only skip Port (i.e. check structureType === UnitType.Port && !hasCoastalTiles) so OilRig placement is not blocked by lack of coastal tiles (oil rigs should rely on oil-terrain checks elsewhere).map-generator/map_generator.go-143-144 (1)
143-144:⚠️ Potential issue | 🟠 MajorPreserve oil when creating 4x/16x maps.
Oil is encoded only as land magnitude
31, butcreateMiniMap()currently keeps whichever non-water land tile is visited last. If a 2x2 block has one oil pixel and normal land after it, the oil disappears frommap4x,map16x, and the thumbnail.🛢️ Proposed downscale fix
func createMiniMap(tm [][]Terrain) [][]Terrain { @@ if miniX < miniWidth && miniY < miniHeight { - // If any of the 4 tiles has water, mini tile is water - if miniMap[miniX][miniY].Type != Water { - miniMap[miniX][miniY] = tm[x][y] - } + source := tm[x][y] + current := miniMap[miniX][miniY] + + // Water still has top priority. + if current.Type == Water { + continue + } + if source.Type == Water { + miniMap[miniX][miniY] = source + continue + } + + // Preserve oil when any source land tile in the block is oil. + if source.Magnitude >= 31 || current.Magnitude < 31 { + miniMap[miniX][miniY] = source + } } } }Also applies to: 264-295
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@map-generator/map_generator.go` around lines 143 - 144, The downscale routines (createMiniMap and the similar 4x/16x map creation logic around the block handling at 264-295) currently overwrite oil tiles because oil is encoded as land magnitude 31 and later pixels in a block can replace it; change the block-aggregation logic to treat Magnitude==31 as highest-priority (if any pixel in the source block has Terrain{Type: Land, Magnitude: 31} the destination must be set to that oil value) instead of keeping the last visited non-water tile—implement by scanning the block for oil first, otherwise choose the highest magnitude land tile as before. Ensure you update the code paths that set terrain[x][y] (and any variables used in createMiniMap/createMap4x/createMap16x) so oil is preserved during downscale.resources/lang/cb.json-59-59 (1)
59-59:⚠️ Potential issue | 🟠 MajorRemove the copied script text from the user-facing string.
This long pasted passage creates avoidable copyright/compliance risk in a shipped localization file.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/lang/cb.json` at line 59, The "copied_to_clipboard" localization value contains a long pasted movie/script excerpt that must be removed and replaced with a short, user-facing message; update the "copied_to_clipboard" string (key: copied_to_clipboard) to a concise, non-copyrighted phrase such as "Copied to clipboard" or "Info copied to clipboard" and remove the entire pasted passage and any informal text (e.g., "btw", "ur", Discord mention).tests/NationStructureBehavior.test.ts-364-377 (1)
364-377:⚠️ Potential issue | 🟠 MajorAdd
sharedWaterComponentsto this game mock.The CI failure shows
handleStructures()now reaches code that callsthis.game.sharedWaterComponents(...), so this partial mock crashes before the oil-rig assertion runs.Proposed test mock fix
const game = { config: () => ({ isUnitDisabled: vi.fn(() => false), gameConfig: () => ({ difficulty: Difficulty.Medium }), unitInfo: vi.fn(() => ({ upgradable: false })), }), unitInfo: vi.fn(() => ({ cost: vi.fn(() => 100n), })), isOceanShore: vi.fn((tile: unknown) => tile === coastalTile), + sharedWaterComponents: vi.fn(() => new Set()), addExecution: vi.fn((execution: unknown) => { queuedExecutions.push(execution); }), };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/NationStructureBehavior.test.ts` around lines 364 - 377, The game mock used in the test is missing the sharedWaterComponents method which causes handleStructures() to crash; add a sharedWaterComponents mock on the game object (the same mock named game in the test) that returns an appropriate value for the test (e.g., vi.fn(() => /* desired shared water result */)) so handleStructures() can call this.game.sharedWaterComponents(...) without throwing and the oil-rig assertion can run; update the mock alongside existing methods like isOceanShore, unitInfo, and addExecution.resources/lang/cb.json-1-544 (1)
1-544:⚠️ Potential issue | 🟠 MajorRemove this non-English locale file from the feature PR.
Regular feature PRs should update only
resources/lang/en.json; non-English locale files are handled by the dedicated translation workflow. Based on learnings, translation files inresources/lang/*.jsonexcepten.jsonshould not be updated in regular PRs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/lang/cb.json` around lines 1 - 544, The PR includes a non-English locale file (cb.json) that should not be part of a feature change; remove/revert the added/modified cb.json from the commit so only en.json is changed, and ensure no references to the "cb" locale remain (check top-level keys like "common" and "main" in the removed file); if translation content is required, leave a note/TODO and open the translation workflow/issue instead.src/core/execution/OilRigExecution.ts-81-89 (1)
81-89:⚠️ Potential issue | 🟠 MajorOnly count factories owned by the oil rig owner.
Line 82 checks for any nearby factory. That means an enemy or ally factory can trigger rail-station creation for this oil rig, even though freight delivery later requires an owned factory.
Scope the nearby factory check to the owner
const nearbyFactory = this.mg.hasUnitNearby( this.oilRig.tile(), this.mg.config().trainStationMaxRange(), UnitType.Factory, + this.oilRig.owner().id(), );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/execution/OilRigExecution.ts` around lines 81 - 89, The nearby factory check in createStation currently counts any nearby factory; restrict it to factories owned by the same owner as this.oilRig by changing the hasUnitNearby call to filter by ownership (or use an owned-specific helper). Specifically, update the logic around mg.hasUnitNearby(this.oilRig.tile(), this.mg.config().trainStationMaxRange(), UnitType.Factory) so it only returns true for units whose owner matches the oil rig owner (e.g., compare unit.owner or unit.ownerId to this.oilRig.owner()/ownerId), then keep adding TrainStationExecution(this.oilRig) only when that owned factory exists.src/core/execution/OilRigExecution.ts-12-43 (1)
12-43:⚠️ Potential issue | 🟠 MajorFix the freight interval calculation.
With
+ this.checkOffset, rigs created on non-zero ticks can spawn early. For example, with interval5and init tick1, the first spawn happens at tick4, not after 5 ticks.Use an explicit next-spawn tick
private active = true; private mg!: Game; - private checkOffset = 0; + private nextFreightTick = 0; @@ init(mg: Game, ticks: number): void { this.mg = mg; - this.checkOffset = - mg.ticks() % Math.max(1, mg.config().oilRigIncomeInterval()); + this.nextFreightTick = + ticks + Math.max(1, mg.config().oilRigIncomeInterval()); } @@ private shouldSpawnFreightTrain(ticks: number): boolean { const interval = Math.max(1, this.mg.config().oilRigIncomeInterval()); - void ticks; - return (this.mg.ticks() + this.checkOffset) % interval === 0; + if (ticks < this.nextFreightTick) { + return false; + } + this.nextFreightTick = ticks + interval; + return true; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/execution/OilRigExecution.ts` around lines 12 - 43, The spawn timing logic uses checkOffset and modulo which causes early spawns for rigs created mid-interval; replace that with an explicit nextSpawnTick field: in init(mg: Game, ticks: number) compute this.nextSpawnTick = mg.ticks() + Math.max(1, mg.config().oilRigIncomeInterval()); change shouldSpawnFreightTrain(ticks) to return mg.ticks() >= this.nextSpawnTick (no modulo/checkOffset and you can ignore the ticks param), and after spawnFreightTrain(...) succeeds advance this.nextSpawnTick by adding the interval (or set to mg.ticks() + interval) so future spawns occur exactly interval ticks apart; remove/stop using checkOffset.src/core/game/TrainStation.ts-314-343 (1)
314-343:⚠️ Potential issue | 🟠 MajorReturn when the destination is dequeued, not when first discovered.
Line 341 can return a non-shortest route. Example: a direct rail to the factory costs
100, but an indirect path through another station costs2; if the direct factory neighbor is seen first, this returns100.Fix the Dijkstra exit condition
while (queue.length > 0) { queue.sort((a, b) => a.distance - b.distance); const current = queue.shift()!; + if (current.station === to) { + return current.distance; + } if ( current.distance > (bestDistance.get(current.station) ?? Number.POSITIVE_INFINITY) ) { continue; @@ const nextDistance = current.distance + rail.tiles.length; - if (neighbor === to) { - return nextDistance; - } - if ( nextDistance >= (bestDistance.get(neighbor) ?? Number.POSITIVE_INFINITY) ) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/game/TrainStation.ts` around lines 314 - 343, The Dijkstra implementation in railroadDistance incorrectly returns when the destination is first discovered (neighbor === to), which can be suboptimal; instead, update bestDistance for the neighbor (using nextDistance) and push it to queue as you do for other neighbors, and only return when the destination is dequeued and processed (i.e., when current.station === to), ensuring you compare current.distance to bestDistance[current.station] before returning; adjust logic around bestDistance, queue push, and the early return to use current.station === to as the exit condition.
🟡 Minor comments (9)
src/client/HelpModal.ts-969-984 (1)
969-984:⚠️ Potential issue | 🟡 MinorTrailing space on Oil Rig
srcline likely breaks Prettier.Line 975 has a trailing space after
${assetUrl("images/OilRigIcon.svg")}— that is the most probable cause of the CI Prettier warning. Easy fix:🧹 Proposed fix
- src=${assetUrl("images/OilRigIcon.svg")} + src=${assetUrl("images/OilRigIcon.svg")}Run
npx prettier --write src/client/HelpModal.tsto be sure no other whitespace gremlins are hiding.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/HelpModal.ts` around lines 969 - 984, Remove the trailing space after the src attribute value in the Oil Rig img tag inside HelpModal.ts—specifically fix the img element that uses assetUrl("images/OilRigIcon.svg") so there is no extra space before the closing `>`; then run npx prettier --write src/client/HelpModal.ts to reformat the file and ensure no other whitespace issues remain (look for the <img ... src=${assetUrl("images/OilRigIcon.svg")} ... /> entry in the HelpModal markup).resources/lang/de.json-112-113 (1)
112-113:⚠️ Potential issue | 🟡 MinorNon-English locale changes should be left to the translation team.
Based on learnings, translation files in
resources/lang/*.json(excepten.json) are updated only via dedicated "mls" translation PRs by the translation team. Please drop the German string changes from this PR and keep just theen.jsonadditions; the translators will pick them up later.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/lang/de.json` around lines 112 - 113, This PR accidentally modifies non-English locale entries ("build_oil_rig" and "build_oil_rig_desc") in resources/lang/de.json; revert those German key-value pairs back to their previous state (remove the new/changed German strings) so only the English additions remain in en.json, leaving translations to the translation team (do not add or change any non-en.json files).src/client/graphics/layers/UILayer.ts-118-118 (1)
118-118:⚠️ Potential issue | 🟡 MinorAdd Oil Rig to deletion progress calculation.
Line 118 creates a loading bar for deleted Oil Rigs, but
getProgress()falls through toreturn 1, so the bar is cleared on the next update.🐛 Proposed fix
case UnitType.City: case UnitType.Factory: case UnitType.Port: case UnitType.DefensePost: + case UnitType.OilRig: return this.deletionProgress(this.game, unit);Also applies to: 329-340
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/graphics/layers/UILayer.ts` at line 118, getProgress() is missing a case for UnitType.OilRig so deletion bars for Oil Rigs immediately fall through to return 1 and disappear; add a UnitType.OilRig branch to the switch in getProgress() that computes and returns the deletion progress the same way other deletable units do (use the same fields/logic used for e.g. UnitType.Factory/other units in that switch). Also apply the same addition to the corresponding deletion-progress handling block around the later code (the block covering the same logic at the 329-340 region) so Oil Rig deletion bars are calculated and rendered consistently.resources/lang/ar.json-74-75 (1)
74-75:⚠️ Potential issue | 🟡 MinorRemove non-English translation changes from this feature PR.
Please keep the new text in
en.jsononly and let the normal translation flow add Arabic later.🧹 Proposed cleanup
- "build_oil_rig": "منصة نفطية", - "build_oil_rig_desc": "لا يمكن بناؤها إلا على حقول النفط. تولّد الذهب مع الوقت، ويمكن للقطارات زيارتها إذا كان هناك مصنع قريب.",Based on learnings, 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/ar.json` around lines 74 - 75, Revert the Arabic locale edits introduced here by removing the added/modified keys "build_oil_rig" and "build_oil_rig_desc" from resources/lang/ar.json so that only en.json contains the new strings; leave the new text in en.json intact and ensure non-English locale files (resources/lang/*.json other than en.json) are not changed in this PR per the translation workflow.resources/maps/worldoil/manifest.json-1-508 (1)
1-508:⚠️ Potential issue | 🟡 MinorRun Prettier on the new manifest.
The manifest data looks consistent, but CI reports a Prettier mismatch for this file.
#!/bin/bash # Format only the new manifest. npx prettier --write resources/maps/worldoil/manifest.json🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/maps/worldoil/manifest.json` around lines 1 - 508, CI reports a Prettier formatting mismatch for the manifest.json file; run Prettier to reformat the JSON (e.g., npx prettier --write manifest.json), or use your editor/IDE formatting integration, then stage and commit the updated manifest (the top-level keys like "map" and "nations" should remain unchanged).resources/lang/pt-PT.json-102-103 (1)
102-103:⚠️ Potential issue | 🟡 MinorAdd the Oil Rig keys used by the build menu too.
This file adds the help text, but the build menu uses
unit_type.oil_rigandbuild_menu.desc.oil_rig. Without those keys, Portuguese users may see raw key names or fallback text.🌍 Proposed localization key additions
"unit_type": { "city": "Cidade", "defense_post": "Posto de Defesa", "port": "Porto", + "oil_rig": "Plataforma petrolífera", "warship": "Naves de Guerra","build_menu": { "desc": { "atom_bomb": "Pequena explosão", "hydrogen_bomb": "Grande explosão", "mirv": "Explosão gigantesca, apenas tem como alvo o jogador selecionado", "missile_silo": "Usado para lançar bombas nucleares", "sam_launcher": "Defende contra bombas nucleares que se aproximam", "warship": "Captura navios de comércio, destrói outros navios e barcos", "port": "Envia navios de comércio para gerar ouro", + "oil_rig": "Gera ouro em jazidas de petróleo", "defense_post": "Aumenta as defesas de fronteiras próximas", "city": "Aumenta a população máxima", "factory": "Cria ferrovias e comboios" },Based on learnings, localization PRs can keep translation content simple while still adding the required technical keys for UI coverage.
🤖 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 102 - 103, Add the missing localization keys used by the build menu: create "unit_type.oil_rig" and "build_menu.desc.oil_rig" entries in the pt-PT JSON (alongside the existing "build_oil_rig" and "build_oil_rig_desc") so UI lookups for unit_type.oil_rig and build_menu.desc.oil_rig resolve; use simple Portuguese text consistent with the existing strings (e.g., short label for unit_type.oil_rig and a concise description for build_menu.desc.oil_rig).src/client/graphics/layers/StructureDrawingUtils.ts-72-72 (1)
72-72:⚠️ Potential issue | 🟡 MinorRemove the trailing space so Prettier passes.
CI is failing the Prettier check on this file.
Proposed formatting fix
- [UnitType.OilRig, { iconPath: oilRigIcon, image: null }], + [UnitType.OilRig, { iconPath: oilRigIcon, image: null }],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/graphics/layers/StructureDrawingUtils.ts` at line 72, Remove the trailing whitespace at the end of the array entry for UnitType.OilRig in StructureDrawingUtils.ts so Prettier passes; locate the tuple "[UnitType.OilRig, { iconPath: oilRigIcon, image: null }]," (in the map/array that builds unit icon definitions) and delete the extra space after the closing comma, then save and run Prettier/format to confirm the file is clean.src/client/graphics/layers/UnitDisplay.ts-30-30 (1)
30-30:⚠️ Potential issue | 🟡 MinorRemove the trailing spaces so Prettier passes.
CI is failing the Prettier check on this file.
Proposed formatting fix
-const oilRigIcon = assetUrl("images/OilRigIcon.svg"); +const oilRigIcon = assetUrl("images/OilRigIcon.svg"); ... - this._oilRig = player.totalUnitLevels(UnitType.OilRig); + this._oilRig = player.totalUnitLevels(UnitType.OilRig);Also applies to: 132-132
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/graphics/layers/UnitDisplay.ts` at line 30, Remove the trailing whitespace characters at the ends of the affected lines so Prettier passes: delete the extra spaces after the const oilRigIcon = assetUrl("images/OilRigIcon.svg"); declaration and the other trailing-space occurrence later in the UnitDisplay component/file (ensure the UnitDisplay component/export lines are trimmed too), then re-run Prettier/format to confirm no remaining trailing spaces.map-generator/assets/maps/worldoil/OIL_EDITING.md-1-28 (1)
1-28:⚠️ Potential issue | 🟡 MinorUse repo-relative paths and run Prettier.
Line 4 points to a local
/home/luke/...path, so the link will be broken for other contributors. CI also reports this Markdown file is not Prettier-formatted.Suggested cleanup
- Open [image.png](/home/luke/CLASS/EECS/481/final/map-generator/assets/maps/worldoil/image.png) in an image editor that preserves exact RGBA values. + Open [`image.png`](./image.png) in an image editor that preserves exact RGBA values.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@map-generator/assets/maps/worldoil/OIL_EDITING.md` around lines 1 - 28, Replace the absolute local path in the Markdown (the link pointing to /home/luke/.../image.png referenced in the OIL_EDITING.md instructions) with a repo-relative path (e.g., assets/maps/worldoil/image.png) and update any other absolute references (such as scripts like apply_oil_reference.py) to use repo-relative links; then run Prettier on OIL_EDITING.md to fix formatting/whitespace so CI passes. Ensure the link text still points to the correct filename (image.png) and that any example commands remain valid after changing the path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d165151d-2bf6-4e70-b8ea-a14536780118
⛔ Files ignored due to path filters (8)
map-generator/assets/maps/worldoil/image.pngis excluded by!**/*.pngmap-generator/assets/maps/worldoil/oil_reference.pngis excluded by!**/*.pngresources/images/OilDropIcon.pngis excluded by!**/*.pngresources/images/OilRigIcon.svgis excluded by!**/*.svgresources/images/buildings/oilRig1.pngis excluded by!**/*.pngresources/maps/worldoil/map.binis excluded by!**/*.binresources/maps/worldoil/map16x.binis excluded by!**/*.binresources/maps/worldoil/map4x.binis excluded by!**/*.bin
📒 Files selected for processing (87)
map-generator/assets/maps/worldoil/OIL_EDITING.mdmap-generator/assets/maps/worldoil/apply_oil_reference.pymap-generator/assets/maps/worldoil/info.jsonmap-generator/main.gomap-generator/map_generator.goresources/lang/ar.jsonresources/lang/bg.jsonresources/lang/bn.jsonresources/lang/ca.jsonresources/lang/cb.jsonresources/lang/cs.jsonresources/lang/da.jsonresources/lang/de-CH.jsonresources/lang/de.jsonresources/lang/el.jsonresources/lang/en.jsonresources/lang/eo.jsonresources/lang/es.jsonresources/lang/et.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.jsonresources/maps/worldoil/manifest.jsonresources/maps/worldoil/thumbnail.webpsrc/client/HelpModal.tssrc/client/InputHandler.tssrc/client/JoinLobbyModal.tssrc/client/UserSettingModal.tssrc/client/components/GameConfigSettings.tssrc/client/components/map/MapPicker.tssrc/client/graphics/SpriteLoader.tssrc/client/graphics/layers/BuildMenu.tssrc/client/graphics/layers/DynamicUILayer.tssrc/client/graphics/layers/FxLayer.tssrc/client/graphics/layers/PlayerInfoOverlay.tssrc/client/graphics/layers/StructureDrawingUtils.tssrc/client/graphics/layers/StructureLayer.tssrc/client/graphics/layers/TerrainLayer.tssrc/client/graphics/layers/UILayer.tssrc/client/graphics/layers/UnitDisplay.tssrc/client/graphics/ui/TextIndicator.tssrc/core/StatsSchemas.tssrc/core/configuration/Config.tssrc/core/configuration/DefaultConfig.tssrc/core/configuration/PastelTheme.tssrc/core/configuration/PastelThemeDark.tssrc/core/execution/AttackExecution.tssrc/core/execution/ConstructionExecution.tssrc/core/execution/FactoryExecution.tssrc/core/execution/OilRigExecution.tssrc/core/execution/TrainExecution.tssrc/core/execution/nation/NationNukeBehavior.tssrc/core/execution/nation/NationStructureBehavior.tssrc/core/game/Game.tssrc/core/game/GameMap.tssrc/core/game/GameUpdates.tssrc/core/game/PlayerImpl.tssrc/core/game/RailNetworkImpl.tssrc/core/game/TrainStation.tssrc/core/game/UnitImpl.tssrc/core/game/UserSettings.tstests/NationStructureBehavior.test.tstests/OilRigExecution.test.tstests/core/game/TrainStation.test.tstests/economy/ConstructionGold.test.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
🧹 Nitpick comments (2)
src/client/graphics/layers/StructureDrawingUtils.ts (1)
373-395: Looks good — septagon mirrors the existing polygon shapes.The new
septagonbranch follows the same pattern aspentagonandoctagon(center, radius, step of2π/n, start angle-π/2). Rendering, closing, fill, and stroke are all consistent.Optional nit: the three branches
pentagon/octagon/septagonare now near-duplicates. If you feel like tidying up later, a small helper likedrawRegularPolygon(ctx, cx, cy, r, sides, startAngle)would remove the copy-paste without adding any dependency.♻️ Optional helper refactor
+ function drawRegularPolygon( + ctx: CanvasRenderingContext2D, + cx: number, + cy: number, + r: number, + sides: number, + startAngle: number, + ) { + const step = (Math.PI * 2) / sides; + ctx.beginPath(); + for (let i = 0; i < sides; i++) { + const angle = startAngle + step * i; + const x = cx + r * Math.cos(angle); + const y = cy + r * Math.sin(angle); + if (i === 0) ctx.moveTo(x, y); + else ctx.lineTo(x, y); + } + ctx.closePath(); + ctx.fill(); + ctx.stroke(); + }Then each case becomes a single call, e.g.
drawRegularPolygon(context, halfIconSize, halfIconSize, halfIconSize - 1, 7, -Math.PI / 2);.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/graphics/layers/StructureDrawingUtils.ts` around lines 373 - 395, The three near-duplicate cases "pentagon", "octagon", and "septagon" in StructureDrawingUtils.ts should be replaced by a small helper to avoid copy/paste: add a drawRegularPolygon(context, cx, cy, r, sides, startAngle) function that begins a path, loops sides times computing angle = startAngle + i*(2*Math.PI/sides), uses moveTo for i==0 and lineTo otherwise, then closePath(), fill() and stroke(); then replace each case body with a single call like drawRegularPolygon(context, halfIconSize, halfIconSize, halfIconSize - 1, N, -Math.PI/2) where N is 5, 7, or 8 for pentagon/septagon/octagon respectively.tests/MapConsistency.test.ts (1)
138-154: Nit: lift the per-map allowlist into a lookup.Hard-coding a branch on
key === "WorldOil"inside the loop will get noisy the next time another map needs extra assets. A smallRecord<GameMapName, string[]>(orPartial<...>) keyed by map name keeps this table-driven and the loop body boring.♻️ Sketch
+const EXTRA_MAP_ASSETS: Partial<Record<GameMapName, string[]>> = { + WorldOil: ["OIL_EDITING.md", "apply_oil_reference.py", "oil_reference.png"], +}; ... - const expected = ["image.png", "info.json"]; - const allowedFiles = - key === "WorldOil" - ? [ - ...expected, - "OIL_EDITING.md", - "apply_oil_reference.py", - "oil_reference.png", - ].sort() - : expected; + const baseFiles = ["image.png", "info.json"]; + const allowedFiles = [ + ...baseFiles, + ...(EXTRA_MAP_ASSETS[key] ?? []), + ].sort();Also worth asking: do
OIL_EDITING.md,apply_oil_reference.py, andoil_reference.pngreally need to live undermap-generator/assets/maps/worldoil/(and thus ship as map assets), or do they belong alongside the generator source where authoring tools usually sit? Shipping authoring docs/scripts as map assets is unusual.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/MapConsistency.test.ts` around lines 138 - 154, Replace the inline branch that special-cases WorldOil inside the loop with a small lookup table: create a Record/Partial (e.g., extraAssetsByMap or perMapAllowlist) mapping GameMapName keys to arrays of extra filenames, then compute allowedFiles as allowedFiles = [...expected, ...(perMapAllowlist[key] || [])].sort() (or just expected when none). Update the comparison that uses files and errors.push unchanged; remove the key === "WorldOil" conditional so future maps can be handled by adding entries to the lookup.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@map-generator/assets/maps/worldoil/OIL_EDITING.md`:
- Line 5: Replace the hard-coded absolute local path in the markdown link "Open
[image.png](/home/luke/CLASS/EECS/481/final/map-generator/assets/maps/worldoil/image.png)"
with a repo-relative path so others can access the image; update that link to
use a relative reference such as "./image.png" (i.e., "Open
[image.png](./image.png)") in the OIL_EDITING.md file.
In `@tests/NationStructureBehavior.test.ts`:
- Around line 2-9: Remove the unused imports ConstructionExecution and UnitType
from the import block that references ConstructionExecution and Game
(Difficulty, PlayerType, TerrainType, UnitType); update the import for Game to
only include the used symbols (Difficulty, PlayerType, TerrainType) so
ESLint/Prettier stop flagging unused imports and the Game import collapses
cleanly.
---
Nitpick comments:
In `@src/client/graphics/layers/StructureDrawingUtils.ts`:
- Around line 373-395: The three near-duplicate cases "pentagon", "octagon", and
"septagon" in StructureDrawingUtils.ts should be replaced by a small helper to
avoid copy/paste: add a drawRegularPolygon(context, cx, cy, r, sides,
startAngle) function that begins a path, loops sides times computing angle =
startAngle + i*(2*Math.PI/sides), uses moveTo for i==0 and lineTo otherwise,
then closePath(), fill() and stroke(); then replace each case body with a single
call like drawRegularPolygon(context, halfIconSize, halfIconSize, halfIconSize -
1, N, -Math.PI/2) where N is 5, 7, or 8 for pentagon/septagon/octagon
respectively.
In `@tests/MapConsistency.test.ts`:
- Around line 138-154: Replace the inline branch that special-cases WorldOil
inside the loop with a small lookup table: create a Record/Partial (e.g.,
extraAssetsByMap or perMapAllowlist) mapping GameMapName keys to arrays of extra
filenames, then compute allowedFiles as allowedFiles = [...expected,
...(perMapAllowlist[key] || [])].sort() (or just expected when none). Update the
comparison that uses files and errors.push unchanged; remove the key ===
"WorldOil" conditional so future maps can be handled by adding entries to the
lookup.
🪄 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: 78608d04-f0a2-4866-b63a-156684d11858
📒 Files selected for processing (12)
map-generator/assets/maps/worldoil/OIL_EDITING.mdresources/maps/worldoil/manifest.jsonsrc/client/HelpModal.tssrc/client/graphics/layers/StructureDrawingUtils.tssrc/client/graphics/layers/UnitDisplay.tssrc/core/game/Game.tssrc/server/MapPlaylist.tstests/MapConsistency.test.tstests/NationStructureBehavior.test.tstests/TranslationSystem.test.tstests/core/game/TrainStation.test.tstests/economy/ConstructionGold.test.ts
✅ Files skipped from review due to trivial changes (3)
- src/server/MapPlaylist.ts
- resources/maps/worldoil/manifest.json
- src/client/HelpModal.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/economy/ConstructionGold.test.ts
- src/client/graphics/layers/UnitDisplay.ts
- tests/core/game/TrainStation.test.ts
- src/core/game/Game.ts
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/core/game/Game.ts (2)
329-329:TrainMissionunion — simple and typed, love it.Typed union beats a class hierarchy here. One small suggestion: consider exporting a const tuple alongside (e.g.
export const TRAIN_MISSIONS = ["trade", "freight"] as const;) so runtime validation/iteration stays in sync with the type without duplicating literals. Optional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/game/Game.ts` at line 329, The exported TrainMission type is fine but lacks a canonical runtime list for validation/iteration; add and export a const tuple (e.g. a TRAIN_MISSIONS constant) that contains the same string literals and use a const assertion so its element types match TrainMission, then update any runtime checks/iteration to reference TRAIN_MISSIONS instead of duplicating the literals; ensure TrainMission remains the union type and keep usages (validation, iteration) in sync by using that constant.
28-28: Tiny nit: single-value union is a bit lonely.
BonusIcon = "oil"works, but a union of one feels like a club with one member. If you expect more icons soon, leave it; otherwise consider just using"oil"inline. Totally optional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/game/Game.ts` at line 28, The exported type BonusIcon currently is a single-member union ("oil"), which is unnecessary; either remove the BonusIcon type and replace its usages with the string literal "oil" directly, or expand it now to an explicit enum/union of expected icons (e.g., export type BonusIcon = "oil" | "water" | ...) if you plan to add icons soon; update all references to BonusIcon (the type name) accordingly so the code compiles with the chosen approach.map-generator/requirements.txt (1)
1-1: PinPillowto a recent safe versionAdding Pillow without a version pin creates two problems: builds become non-reproducible (different installs may pull different versions), and you risk getting a version with known vulnerabilities. The Pillow package has had many security fixes across versions. Specify a minimum safe version such as
Pillow>=12.2.0(the latest) or verify a safe version floor using the OSV query below.Suggested change
-Pillow +Pillow>=12.2.0Use this script to check the latest safe versions and recent security advisories:
#!/bin/bash set -euo pipefail echo "Latest Pillow release on PyPI:" curl -s https://pypi.org/pypi/Pillow/json | jq -r '.info.version' echo echo "Recent OSV advisories for Pillow:" curl -s "https://api.osv.dev/v1/query" \ -H "Content-Type: application/json" \ -d '{"package":{"name":"Pillow","ecosystem":"PyPI"}}' \ | jq -r '.vulns[]? | "\(.id) | introduced: \(.affected[0].ranges[0].events[]?.introduced // "-") | fixed: \(.affected[0].ranges[0].events[]?.fixed // "-")"'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@map-generator/requirements.txt` at line 1, requirements.txt currently lists Pillow without a version pin which can lead to unreproducible builds and pull vulnerable releases; update the entry for Pillow to pin a recent safe minimum (for example change "Pillow" to "Pillow>=12.2.0" or another vetted safe version) so installs are deterministic and avoid known vulnerabilities.
🤖 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/core/game/Game.ts`:
- Line 706: The addGold signature now accepts icon?: BonusIcon but
PlayerImpl.addGold (the implementation that constructs BonusEvent) never
forwards it; update the code that builds the BonusEvent in PlayerImpl (and any
other place that emits BonusEvent) to include the icon field passed into
addGold, and update the BonusEvent shape in GameUpdates.ts (and its exported
type) to include icon?: BonusIcon so the payload carries the icon value through
to the client renderer.
---
Nitpick comments:
In `@map-generator/requirements.txt`:
- Line 1: requirements.txt currently lists Pillow without a version pin which
can lead to unreproducible builds and pull vulnerable releases; update the entry
for Pillow to pin a recent safe minimum (for example change "Pillow" to
"Pillow>=12.2.0" or another vetted safe version) so installs are deterministic
and avoid known vulnerabilities.
In `@src/core/game/Game.ts`:
- Line 329: The exported TrainMission type is fine but lacks a canonical runtime
list for validation/iteration; add and export a const tuple (e.g. a
TRAIN_MISSIONS constant) that contains the same string literals and use a const
assertion so its element types match TrainMission, then update any runtime
checks/iteration to reference TRAIN_MISSIONS instead of duplicating the
literals; ensure TrainMission remains the union type and keep usages
(validation, iteration) in sync by using that constant.
- Line 28: The exported type BonusIcon currently is a single-member union
("oil"), which is unnecessary; either remove the BonusIcon type and replace its
usages with the string literal "oil" directly, or expand it now to an explicit
enum/union of expected icons (e.g., export type BonusIcon = "oil" | "water" |
...) if you plan to add icons soon; update all references to BonusIcon (the type
name) accordingly so the code compiles with the chosen approach.
🪄 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: eab676a8-cef4-4068-9591-02292e8414f0
📒 Files selected for processing (3)
map-generator/requirements.txtsrc/core/game/Game.tstests/NationStructureBehavior.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/NationStructureBehavior.test.ts
3 participants
Resolves #(3129)
Description:
THE OIL RIG FEATURE:
Features Added:
How does the new feature work:
Oil rigs can be placed onto oil terrain, and spawn a freight class train every 10 seconds which pathfinds to the user's nearest factory (it will not spawn unless the user has a connected factory). At the factory, the train grants value proportional to the amount of stacked oil rigs that the freight train came from. The oil rig fits into the existing {port, railroad} category of structure cost since they are all economic structures.
Code changes made to implement:
Terrain:
Maps:
Tests:
Structures:
UI Images:
Build Wheel
Build Menu Hover
Oil Rig Close Icon
Oil Rig Far Icon + Money Animation
Build Menu Icon
Oil Rig Far Icon
Oil Terrain on New World Map
Please complete the following:
Discord username: lukewinsfantasy