Fix TribeSpawner crash on invalid nation coordinates - #4847
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Walkthrough
ChangesTribe spawning validation
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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
🤖 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/core/execution/TribeSpawner.ts`:
- Around line 25-29: Add focused tests for the construction path in TribeSpawner
that cover both an out-of-bounds nation cell being ignored without throwing and
a valid nation cell preventing positioned tribe spawning. Reuse the existing
test fixtures and spawning APIs, and assert each behavior directly.
🪄 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 Plus
Run ID: 46baaadc-04c1-4f47-bfc3-7f3184f79d80
📒 Files selected for processing (1)
src/core/execution/TribeSpawner.ts
| this.nationTiles = new Set( | ||
| nationCells | ||
| .filter((c) => gs.isValidCoord(c.x, c.y)) | ||
| .map((c) => gs.ref(c.x, c.y)), | ||
| ); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add tests for invalid and valid nation cells.
This file matches src/core/**/*.ts. The repository guideline requires tests for every change in this directory. Add focused tests that verify:
- An out-of-bounds nation cell does not crash construction.
- A valid nation cell still blocks positioned tribe spawning.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/core/execution/TribeSpawner.ts` around lines 25 - 29, Add focused tests
for the construction path in TribeSpawner that cover both an out-of-bounds
nation cell being ignored without throwing and a valid nation cell preventing
positioned tribe spawning. Reuse the existing test fixtures and spawning APIs,
and assert each behavior directly.
Source: Coding guidelines
github-project-automation
Bot
moved this from Triage
to Development
in OpenFront Release Management
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tests/core/execution/TribeSpawner.test.ts`:
- Around line 383-387: Update the tile selection in the test “valid nation cell
prevents positioned tribe from occupying that tile” to use an unowned land tile
by requiring !game.hasOwner(t) in the findLandTile predicate. Keep the existing
land and impassable checks, ensuring the test reaches the nationTiles blocking
logic in TribeSpawner.
🪄 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 Plus
Run ID: 15a0f9ce-9544-4c85-be85-415d744ebd23
📒 Files selected for processing (1)
tests/core/execution/TribeSpawner.test.ts
| test("valid nation cell prevents positioned tribe from occupying that tile", async () => { | ||
| const game = await setup("plains", { bots: 1, gameMap: GameMapType.Asia }); | ||
| const tile = findLandTile(game); | ||
| const x = game.x(tile); | ||
| const y = game.y(tile); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Select an unowned tile for this test.
Line 385 uses findLandTile, but the helper checks only land and impassable state. src/core/execution/TribeSpawner.ts also rejects owned tiles before it checks nationTiles.has(tile). If this tile has an owner, lines 399-405 pass without testing nation-cell blocking.
Update findLandTile to require !game.hasOwner(t).
Proposed test isolation fix
- if (game.isLand(t) && !game.isImpassable(t)) return t;
+ if (
+ game.isLand(t) &&
+ !game.isImpassable(t) &&
+ !game.hasOwner(t)
+ ) {
+ return t;
+ }🤖 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 `@tests/core/execution/TribeSpawner.test.ts` around lines 383 - 387, Update the
tile selection in the test “valid nation cell prevents positioned tribe from
occupying that tile” to use an unowned land tile by requiring !game.hasOwner(t)
in the findLandTile predicate. Keep the existing land and impassable checks,
ensuring the test reaches the nationTiles blocking logic in TribeSpawner.
github-project-automation
Bot
moved this from Development
to Complete
in OpenFront Release Management
2 participants
Description:
The
TribeSpawnerconstructor calledgs.ref(c.x, c.y)on all nation cells without checking if coordinates are valid. If a map has nation coordinates outside its bounds (e.g. the Scandinavia map has "Tver" at x=2007 on a 2006-wide image), the game would crash withError: Invalid coordinatesduring map load.Added a
gs.isValidCoord()filter before building thenationTilesset, matching the existing defensive pattern inspawnPositionedTribe().Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
FloPinguin