Add Map Chopping Block#3952
Conversation
|
SAM seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
WalkthroughThis PR adds a new playable map called "Chopping Block" to the game. It includes map asset definitions, a new game map type, and integration points in the build system, translations, and map playlist rotation. ChangesChopping Block Map Addition
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" 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/game/Game.ts`:
- Line 175: Add unit tests that cover the new enum member and its registration:
write a test asserting that the GameCategory (or relevant enum/class) includes
the "ChoppingBlock" member (e.g., check Game.ChoppingBlock or export where the
enum is defined) and another test asserting that the arcade category
registration (the function or map used to register categories, e.g.,
registerCategory or arcadeCategoryEntries) contains the ChoppingBlock entry;
ensure tests import the same symbols added in Game.ts and assert
membership/values so CI enforces the src/core/ change.
🪄 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: 2f20a946-4c81-4416-9370-6526bd2ff21c
⛔ Files ignored due to path filters (4)
map-generator/assets/maps/choppingblock/image.pngis excluded by!**/*.pngresources/maps/choppingblock/map.binis excluded by!**/*.binresources/maps/choppingblock/map16x.binis excluded by!**/*.binresources/maps/choppingblock/map4x.binis excluded by!**/*.bin
📒 Files selected for processing (7)
map-generator/assets/maps/choppingblock/info.jsonmap-generator/main.goresources/lang/en.jsonresources/maps/choppingblock/manifest.jsonresources/maps/choppingblock/thumbnail.webpsrc/core/game/Game.tssrc/server/MapPlaylist.ts
| MiddleEast = "Middle East", | ||
| TaiwanStrait = "Taiwan Strait", | ||
| DanishStraits = "Danish Straits", | ||
| ChoppingBlock = "Chopping Block", |
There was a problem hiding this comment.
Add tests for src/core/ changes.
The coding guidelines require that all changes to src/core/ include tests. The enum addition and category registration should have corresponding test coverage.
Consider adding a test to verify:
- The ChoppingBlock enum member exists
- ChoppingBlock is present in the arcade category
As per coding guidelines: "All changes to src/core/ must include tests"
Also applies to: 263-263
🤖 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/game/Game.ts` at line 175, Add unit tests that cover the new enum
member and its registration: write a test asserting that the GameCategory (or
relevant enum/class) includes the "ChoppingBlock" member (e.g., check
Game.ChoppingBlock or export where the enum is defined) and another test
asserting that the arcade category registration (the function or map used to
register categories, e.g., registerCategory or arcadeCategoryEntries) contains
the ChoppingBlock entry; ensure tests import the same symbols added in Game.ts
and assert membership/values so CI enforces the src/core/ change.
There was a problem hiding this comment.
Map has been tested. Video evidence provided. Thank you rabbit.
There was a problem hiding this comment.
@PatrickPlaysBadly, thanks for sharing the video evidence — that's great for confirming the map plays correctly! 🎉
My concern was specifically about automated unit tests for the TypeScript changes in src/core/game/Game.ts (the ChoppingBlock enum member and its arcade category entry), as the coding guidelines say all src/core/ changes must include tests.
That said, the change itself is very small — just adding one enum value and one list entry — so if the team considers this level of change exempt from that rule, that's totally fine. Would you like to open a follow-up issue to track adding automated test coverage for new GameMapType entries, or is this considered low enough risk to skip?
(🐇 hop hop — the map looks fun either way!)
🧠 Learnings used
Learnt from: FloPinguin
Repo: openfrontio/OpenFrontIO PR: 2874
File: src/server/MapLandTiles.ts:7-11
Timestamp: 2026-01-12T21:37:01.156Z
Learning: In this repository's OpenFrontIO deployment, inter-service HTTP calls to the master should target http://localhost:3000 (master at port 3000) as the canonical address. Apply this as the standard for all server-side TypeScript code that communicates with the master. Avoid hardcoding non-master URLs; centralize the master address (e.g., via config or env) when possible, and ensure internal service communication uses localhost:3000 in this architecture.
Learnt from: FloPinguin
Repo: openfrontio/OpenFrontIO PR: 2887
File: src/core/execution/NukeExecution.ts:118-122
Timestamp: 2026-01-13T20:16:05.535Z
Learning: In code paths that return a Player-like object, prefer returning a union type (Player | TerraNullius) instead of undefined. When a function may fail to find a player, return TerraNullius for the 'not found' case and a Player for valid IDs, and check .isPlayer() (or equivalent) directly on the result instead of guarding with undefined or optional chaining. This should be enforced in Game, GameImpl, and GameView (and similar accessors) to avoid undefined checks and simplify null-safety handling.
Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 3292
File: src/core/execution/TrainExecution.ts:70-84
Timestamp: 2026-02-25T02:38:42.961Z
Learning: Preserve the intentional duplication of boundary tiles when constructing train motion plan paths. Specifically, do not deduplicate tiles at station boundaries in train motion plan logic. This affects both server-side tracking (TrainExecution.ts saveTraversedTiles) and client-side processing (GameView.ts train motion plan processing). Reviewers should ensure changes do not remove or consolidate boundary tiles, as duplicates are expected and documented in MotionPlans.ts, and are necessary for correct car spacing and positioning, especially when speed > 1.
Learnt from: VariableVince
Repo: openfrontio/OpenFrontIO PR: 3549
File: tests/pathfinding/playground/server.ts:246-250
Timestamp: 2026-03-31T17:02:41.798Z
Learning: When migrating/implementing Express 5+ code, use the Express 5 `app.listen(PORT, (error) => { ... })` callback signature: Express 5 passes listen errors (e.g., `EADDRINUSE`) as the first argument to the callback. Handle the error inside that callback instead of relying on Node.js `http.Server`’s `'error'` event semantics.
Learnt from: baculinivan-web
Repo: openfrontio/OpenFrontIO PR: 3688
File: src/client/graphics/layers/GameRightSidebar.ts:235-248
Timestamp: 2026-04-22T08:54:50.345Z
Learning: In the OpenFrontIO/OpenFrontIO repository, do not raise review findings for standard web accessibility (a11y) keyboard/semantic issues that would normally require button semantics (e.g., using clickable `<div>` elements instead of `<button>`, or similar keyboard accessibility/ARIA patterns). This repository intentionally does not enforce those semantic/keyboard-accessibility conventions because gameplay requires touch or mouse input and is not operable via keyboard alone.
There was a problem hiding this comment.
@coderabbitai Dismiss. Individual maps do not require a change in the tests. No map is denoted in a test
github-project-automation
Bot
moved this from Triage
to Development
in OpenFront Release Management
3 participants
Description:
Describe the PR.
Custom designed map based off of a wooden chopping block pattern/production method.
About 50 additional named nations have been added along with the 32 default.
Map is setup for 2 and 4 teams.
https://www.youtube.com/watch?v=SVwQWMtZRuI
https://discord.com/channels/1284581928254701718/1482787372654399489/1482787372654399489
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
PlaysBadly