Skip to content

Fix: Ship stat display#3931

Open
ACassidy95 wants to merge 4 commits into
openfrontio:mainfrom
ACassidy95:fix-ship-stat-display
Open

Fix: Ship stat display#3931
ACassidy95 wants to merge 4 commits into
openfrontio:mainfrom
ACassidy95:fix-ship-stat-display

Conversation

@ACassidy95
Copy link
Copy Markdown

@ACassidy95 ACassidy95 commented May 15, 2026

If this PR fixes an issue, link it below. If not, delete these two lines.
Resolves #3441

Description:

This change enables the unitTypeToBoatUnit Record type in StatsSchemas.ts for use in recording boat-usage stats, along with updating method/function signatures to accept the appropriate BoatUnit type. This includes updating test code wherever necessary in order to satisfy updated signatures.

Please complete the following:

  • I have added screenshots for all UI updates
  • I process any text displayed to the user through translateText() and I've added it to the en.json file
  • I have added relevant tests to the test directory
  • I confirm I have thoroughly tested these changes and take full responsibility for any bugs introduced

Please put your Discord username so you can be contacted if a bug or regression is found:

DISCORD_USERNAME: TheStoneyLonesome

@CLAassistant
Copy link
Copy Markdown

CLAassistant commented May 15, 2026

CLA assistant check
All committers have signed the CLA.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 15, 2026

Review Change Stack

Walkthrough

Reactivates and exports a UnitType→boat-key mapping, adds BoatUnitType parameters to all boat-stat methods, updates StatsImpl to dispatch by boat type, threads the new parameter through ship execution and deletion code paths, and updates tests to match the new signatures.

Changes

Ship Statistics Type Tracking

Layer / File(s) Summary
Boat unit type mapping contract
src/core/StatsSchemas.ts
Introduces unitTypeToBoatUnit constant mapping UnitType.TradeShip"trade" and UnitType.TransportShip"trans", with satisfies Record<BoatUnitType, BoatUnit>.
Stats interface and imports
src/core/game/Stats.ts
Imports BoatUnitType and adds type: BoatUnitType parameter to seven boat-stat methods (boatSendTrade, boatArriveTrade, boatCapturedTrade, boatDestroyTrade, boatSendTroops, boatArriveTroops, boatDestroyTroops).
Stats implementation with unit type dispatch
src/core/game/StatsImpl.ts
Imports BoatUnitType and unitTypeToBoatUnit; refactors _addBoat to accept boatType: BoatUnitType and map it via unitTypeToBoatUnit; updates all seven boat-stat methods to accept and forward type.
Ship execution and arrival tracking
src/core/execution/TradeShipExecution.ts, src/core/execution/TransportShipExecution.ts
TradeShipExecution reformats three stats calls (multi-line); TransportShipExecution updates send/arrival stats calls to pass UnitType.TransportShip and appropriate troop counts.
Unit destruction side effects
src/core/game/UnitImpl.ts
delete() updated to call boatDestroyTroops and boatDestroyTrade with explicit UnitType and troop count arguments.
Test updates for new signatures
tests/Stats.test.ts
Seven test cases updated to pass explicit UnitType parameters: trade tests pass UnitType.TradeShip, troop tests pass UnitType.TransportShip.

🎯 3 (Moderate) | ⏱️ ~20 minutes

⛵ Trade and transport now sail their separate ways,
No more confusion in the stats displays,
A mapping wakes from commented sleep,
Each ship type counted true and deep! 🚢

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description check ✅ Passed The description clearly explains the change enables unitTypeToBoatUnit in StatsSchemas.ts and updates signatures accordingly, with linked issue #3441 about swapped ship statistics.
Linked Issues check ✅ Passed All code changes directly address issue #3441: the commented-out unitTypeToBoatUnit mapping is re-enabled, method signatures are updated to pass BoatUnitType parameters, and tests are updated to match [#3441].
Out of Scope Changes check ✅ Passed All changes are directly scoped to fixing ship stat display: re-enabling unitTypeToBoatUnit, updating boat-stat method signatures, and updating related call sites and tests.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Title check ✅ Passed The title 'Fix: Ship stat display' directly addresses the PR's main objective of resolving issue #3441 by restoring the unitTypeToBoatUnit mapping to fix swapped ship statistics display.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

Tip

💬 Introducing Slack Agent: The best way for teams to turn conversations into code.

Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.

  • Generate code and open pull requests
  • Plan features and break down work
  • Investigate incidents and troubleshoot customer tickets together
  • Automate recurring tasks and respond to alerts with triggers
  • Summarize progress and report instantly

Built for teams:

  • Shared memory across your entire org—no repeating context
  • Per-thread sandboxes to safely plan and execute work
  • Governance built-in—scoped access, auditability, and budget controls

One agent for your entire SDLC. Right inside Slack.

👉 Get started


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
src/core/game/Stats.ts (1)

37-80: ⚡ Quick win

Tighten boat method parameter types by method intent.

These signatures still allow the wrong ship type at compile time (trade methods can take transport type, and vice versa). That can silently reintroduce swapped stats. Use specific literal types per method.

Proposed type-tightening diff
-import { Player, TerraNullius } from "./Game";
+import { Player, TerraNullius, UnitType } from "./Game";
@@
-  boatSendTrade(player: Player, target: Player, type: BoatUnitType): void;
+  boatSendTrade(
+    player: Player,
+    target: Player,
+    type: UnitType.TradeShip,
+  ): void;
@@
-    type: BoatUnitType,
+    type: UnitType.TradeShip,
@@
-    type: BoatUnitType,
+    type: UnitType.TradeShip,
@@
-  boatDestroyTrade(player: Player, target: Player, type: BoatUnitType): void;
+  boatDestroyTrade(
+    player: Player,
+    target: Player,
+    type: UnitType.TradeShip,
+  ): void;
@@
-    type: BoatUnitType,
+    type: UnitType.TransportShip,
@@
-    type: BoatUnitType,
+    type: UnitType.TransportShip,
@@
-    type: BoatUnitType,
+    type: UnitType.TransportShip,
🤖 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/Stats.ts` around lines 37 - 80, The boat-related method
signatures (boatSendTrade, boatArriveTrade, boatCapturedTrade, boatDestroyTrade,
boatSendTroops, boatArriveTroops, boatDestroyTroops) currently accept a generic
BoatUnitType which allows passing the wrong ship kind; change each method's
"type" parameter to the precise literal/enum value that matches the method
intent (e.g., use the trade-specific literal/enum member for all *Trade methods
and the transport/ troop-specific literal/enum member for all *Troops methods),
and then update any implementations/call sites to use those narrowed types so
the compiler enforces correct ship-kind usage rather than allowing swapping
trade vs transport types.
🤖 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.

Nitpick comments:
In `@src/core/game/Stats.ts`:
- Around line 37-80: The boat-related method signatures (boatSendTrade,
boatArriveTrade, boatCapturedTrade, boatDestroyTrade, boatSendTroops,
boatArriveTroops, boatDestroyTroops) currently accept a generic BoatUnitType
which allows passing the wrong ship kind; change each method's "type" parameter
to the precise literal/enum value that matches the method intent (e.g., use the
trade-specific literal/enum member for all *Trade methods and the transport/
troop-specific literal/enum member for all *Troops methods), and then update any
implementations/call sites to use those narrowed types so the compiler enforces
correct ship-kind usage rather than allowing swapping trade vs transport types.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: f55f6756-07a7-4790-8b5f-a0580273f512

📥 Commits

Reviewing files that changed from the base of the PR and between ca565ea and 0085934.

📒 Files selected for processing (7)
  • src/core/StatsSchemas.ts
  • src/core/execution/TradeShipExecution.ts
  • src/core/execution/TransportShipExecution.ts
  • src/core/game/Stats.ts
  • src/core/game/StatsImpl.ts
  • src/core/game/UnitImpl.ts
  • tests/Stats.test.ts

@ACassidy95 ACassidy95 marked this pull request as ready for review May 15, 2026 20:09
@ACassidy95 ACassidy95 requested a review from a team as a code owner May 15, 2026 20:09
@ACassidy95 ACassidy95 force-pushed the fix-ship-stat-display branch from 0085934 to 69100fe Compare May 15, 2026 23:42
@ACassidy95 ACassidy95 changed the title Fix ship stat display Fix: Ship stat display May 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Triage

Development

Successfully merging this pull request may close these issues.

2 participants