Record missing stats

[DevelopingTom]

Description:

Some stats are missing from the recorded game stats:

• Unit upgrade
• Gold from trade and from steal

The gold from trade/steal was introduced with PR 784 but was quickly reverted with PR 927, probably involuntarily.

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:

IngloriousTom

[coderabbitai]

Walkthrough

Added telemetry recording calls to TradeShipExecution.complete() to measure two outcomes: boatCapturedTrade when a trade ship is captured, and boatArriveTrade when it arrives normally. Test mock objects now include clientID properties to support these telemetry scenarios.

Changes

Cohort / File(s)	Summary
Telemetry instrumentation for trade ship completion src/core/execution/TradeShipExecution.ts	Added two stat recording calls in complete(): records boatCapturedTrade (with tradeShip owner, original owner, gold) when captured, and boatArriveTrade (with srcPort owner, dstPort owner, gold) when not captured
Test mock setup updates tests/core/executions/TradeShipExecution.test.ts	Exposed clientID mock properties (values 1 and 2) on origOwner and dstOwner test objects to support telemetry test scenarios

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

• Straightforward instrumentation additions with no logic or control flow changes
• Mock property additions are minimal and consistent
• Verify telemetry call signatures align with expected stat recording patterns
• Confirm clientID values in tests match their usage contexts

Possibly related PRs

• update meta #1397 — Also modifies TradeShipExecution.complete() to adjust parameters (tradeShipGold call with numPorts), directly affecting the same method
• Add gold fx when a tradeship lands #1322 — Modifies complete() method to update addGold() calls with tile arguments, overlapping implementation area
• Add back the trade ship send stat #1253 — Adds complementary trade-ship telemetry instrumentation (boatSendTrade in tick), similar instrumentation pattern

Suggested reviewers

• evanpelle
• scottanderson

Poem

📊 Trade ships now report their fate,
Captured or arrived—we now calculate,
Each coin and port tracked with care,
Telemetry floats through the air! ⛵✨

Pre-merge checks

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'Record missing stats' accurately summarizes the main change in the pull request—adding telemetry/stat recording calls for previously missing game stats (boatCapturedTrade and boatArriveTrade).
Description check	✅ Passed	The description is directly related to the changeset, explaining the motivation for recording missing stats (unit upgrade and gold from trade/steal) and referencing prior PRs that introduced and reverted similar functionality.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 76bd70a and 29d2d85.

📒 Files selected for processing (2)

• src/core/execution/TradeShipExecution.ts (2 hunks)
• src/core/game/UnitImpl.ts (1 hunks)

🧰 Additional context used

🧠 Learnings (5)

📚 Learning: 2025-05-19T06:00:38.007Z

Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 784
File: src/core/game/StatsImpl.ts:125-134
Timestamp: 2025-05-19T06:00:38.007Z
Learning: In StatsImpl.ts, unused parameters in boat/stats-related methods are intentionally kept for future use and shouldn't be removed.

Applied to files:

• src/core/execution/TradeShipExecution.ts
• src/core/game/UnitImpl.ts

📚 Learning: 2025-05-22T23:48:51.113Z

Learnt from: jrouillard
Repo: openfrontio/OpenFrontIO PR: 848
File: src/core/execution/TradeShipExecution.ts:125-127
Timestamp: 2025-05-22T23:48:51.113Z
Learning: In TradeShipExecution, the path length is already checked in the computeNewPath() function. If the path is empty, it returns PathFindResultType.PathNotFound instead of PathFindResultType.Completed, making additional empty path checks unnecessary when handling the Completed case.

Applied to files:

• src/core/execution/TradeShipExecution.ts

📚 Learning: 2025-08-29T16:16:11.309Z

Learnt from: BrewedCoffee
Repo: openfrontio/OpenFrontIO PR: 1957
File: src/core/execution/PlayerExecution.ts:40-52
Timestamp: 2025-08-29T16:16:11.309Z
Learning: In OpenFrontIO PlayerExecution.ts, when Defense Posts are captured due to tile ownership changes, the intended behavior is to first call u.decreaseLevel() to downgrade them, then still transfer them to the capturing player via captureUnit(). This is not a bug - Defense Posts should be both downgraded and captured, not destroyed outright.

Applied to files:

• src/core/game/UnitImpl.ts

📚 Learning: 2025-10-20T20:15:28.858Z

Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:51-51
Timestamp: 2025-10-20T20:15:28.858Z
Learning: In src/core/execution/FakeHumanExecution.ts, game balance constants like MIRV_COOLDOWN_TICKS, MIRV_HESITATION_ODDS, VICTORY_DENIAL_TEAM_THRESHOLD, VICTORY_DENIAL_INDIVIDUAL_THRESHOLD, and STEAMROLL_CITY_GAP_MULTIPLIER are experimental tuning parameters subject to frequent change during balance testing. Do not flag changes to these values as issues or compare them against previous values.

Applied to files:

• src/core/game/UnitImpl.ts

📚 Learning: 2025-06-05T02:34:45.899Z

Learnt from: Egraveline
Repo: openfrontio/OpenFrontIO PR: 1012
File: src/core/execution/UpgradeStructureExecution.ts:49-54
Timestamp: 2025-06-05T02:34:45.899Z
Learning: In the upgrade system, gold deduction for structure upgrades is handled internally by the `upgradeUnit` method in PlayerImpl, not in the UpgradeStructureExecution class. The UpgradeStructureExecution only needs to check if the player has sufficient gold before calling `upgradeUnit`.

Applied to files:

• src/core/game/UnitImpl.ts

🪛 GitHub Actions: 🧪 CI

src/core/execution/TradeShipExecution.ts

[error] 173-173: TypeError: player.clientID is not a function

🔇 Additional comments (2)

src/core/execution/TradeShipExecution.ts (1)

151-154: Stat recording is correct—no changes needed.

The implementations in StatsImpl.ts (lines 180-189) are clean and properly handle both scenarios. The calls in TradeShipExecution.ts pass the correct parameters in the right order: the capturing player and original owner for boatCapturedTrade, and both port owners for boatArriveTrade. Tests validate the behavior works as intended.

src/core/game/UnitImpl.ts (1)

425-435: Stat recording is safe—approve as-is.

The unitUpgrade call chain properly handles null clientID values. When _makePlayerStats(player) encounters a null clientID at line 56, it returns undefined (line 57), and _addOtherUnit gracefully exits early (line 126). This defensive pattern is consistent across all stats tracking methods (_addAttack, _addBoat, _addBomb, etc.), so stats recording simply skips for players with null IDs rather than causing a pipeline failure.

[evanpelle]

Thanks!

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

tests/core/executions/TradeShipExecution.test.ts (1)

32-32: Add clientID to the pirate mock for consistency.

The origOwner and dstOwner mocks now include clientID to support telemetry recording. However, the pirate mock (lines 46-53) lacks this property. Since a captured trade ship is owned by the pirate when it completes, the telemetry code would need pirate.clientID() in that scenario.

While current tests don't exercise a captured trade completion, adding clientID to the pirate mock prevents future test failures and keeps all player mocks consistent.

Apply this diff:

 pirate = {
   id: jest.fn(() => 3),
+  clientID: jest.fn(() => 3),
   addGold: jest.fn(),
   displayName: jest.fn(() => "Destination"),
   units: jest.fn(() => [piratePort]),
   unitCount: jest.fn(() => 1),
   canTrade: jest.fn(() => true),
 } as any;

Also applies to: 42-42, 46-53

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 29d2d85 and 7a948f9.

📒 Files selected for processing (1)

• tests/core/executions/TradeShipExecution.test.ts (2 hunks)