Implement boat unit details #2359
Conversation
|
Warning Rate limit exceeded@kerodkibatu has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 22 minutes and 49 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (4)
WalkthroughAdds tracking of a TradeShip's source unit and distance traveled through simulation state and updates, surfaces distance-based trade gold via a new helper in the player UI, and adds a localized "(estimate)" label. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as PlayerInfoOverlay
participant Calc as calculateTradeShipGold()
participant UV as UnitView
participant CFG as config()
UI->>Calc: request estimate for TradeShip unit
activate Calc
Calc->>UV: read distanceTraveled() and sourceUnitId()
Calc->>UV: count owner's ports (totalUnitLevels Port)
Calc->>CFG: tradeShipGold(distance, numPorts)
CFG-->>Calc: return bigint | null
deactivate Calc
alt estimate present
UI->>UI: render gold value + " (estimate)"
else
UI->>UI: no estimate shown
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks✅ 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: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/client/graphics/layers/PlayerInfoOverlay.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-12T00:31:50.144Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 1752
File: src/core/game/Game.ts:750-752
Timestamp: 2025-08-12T00:31:50.144Z
Learning: In the OpenFrontIO codebase, changes to the PlayerInteraction interface (like adding canDonateGold and canDonateTroops flags) do not require corresponding updates to src/core/Schemas.ts or server serialization code.
Applied to files:
src/client/graphics/layers/PlayerInfoOverlay.ts
📚 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/client/graphics/layers/PlayerInfoOverlay.ts
🧬 Code graph analysis (1)
src/client/graphics/layers/PlayerInfoOverlay.ts (3)
src/core/configuration/DefaultConfig.ts (1)
tradeShipGold(380-387)src/core/game/GameView.ts (5)
unit(658-660)UnitView(44-179)targetUnitId(127-129)owner(109-111)owner(636-638)src/client/Utils.ts (3)
translateText(92-147)renderNumber(19-43)renderTroops(15-17)
🔇 Additional comments (2)
src/client/graphics/layers/PlayerInfoOverlay.ts (2)
410-414: Clean conditional initialization.The type-safe initialization and conditional calculation for TradeShip units is clear and correct.
448-457: LGTM!The TransportShip troop display correctly uses
translateText()andrenderTroops(), following established patterns and meeting the i18n requirements.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
resources/lang/en.json(1 hunks)src/client/graphics/layers/PlayerInfoOverlay.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/client/graphics/layers/PlayerInfoOverlay.ts
…. Removed estimate text from overlay and replaced tile tracking with distance traveled in trade ship execution.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/core/executions/TradeShipExecution.test.ts (1)
71-81: Add test cases to verify distance tracking behavior.The mock scaffolding is in place, but no tests verify that distance is tracked correctly. Consider adding test cases that check:
setDistanceTraveledis called with the correct distance as the ship movesdistanceTraveledreturns the expected value- Distance tracking affects gold calculation when trade completes
Example test case structure:
it("should track distance traveled", () => { // Set initial distance tradeShip.setDistanceTraveled(10); // Verify distance is tracked expect(tradeShip.distanceTraveled()).toBe(10); // Add more assertions for movement and gold calculation });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
resources/lang/en.json(1 hunks)src/client/graphics/layers/PlayerInfoOverlay.ts(2 hunks)src/core/game/Game.ts(2 hunks)src/core/game/GameUpdates.ts(1 hunks)tests/core/executions/TradeShipExecution.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/core/game/GameUpdates.ts
- resources/lang/en.json
- src/client/graphics/layers/PlayerInfoOverlay.ts
- src/core/game/Game.ts
🧰 Additional context used
🧠 Learnings (1)
📚 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:
tests/core/executions/TradeShipExecution.test.ts
🔇 Additional comments (1)
tests/core/executions/TradeShipExecution.test.ts (1)
71-71: Mock scaffolding looks correct.The distance tracking mock uses a closure to maintain state between setter and getter calls. This is a clean pattern for test mocks.
Also applies to: 78-81
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/core/executions/TradeShipExecution.test.ts (1)
120-128: Enhance the trade completion test to verify distance-based gold calculation.The test verifies that trade completion triggers cleanup and messages, but doesn't verify the core new functionality: that gold amount is calculated correctly based on
distanceTraveled().Enhance this test:
it("should complete trade and award gold", () => { + // Set initial distance + tradeShip.setDistanceTraveled(5); + tradeShipExecution["pathFinder"] = { nextTile: jest.fn(() => ({ type: 2, node: 2001 })), } as any; tradeShipExecution.tick(1); + + // Verify distance was read during gold calculation + expect(tradeShip.distanceTraveled).toHaveBeenCalled(); + + // Verify both players received gold + expect(origOwner.addGold).toHaveBeenCalled(); + expect(dstOwner.addGold).toHaveBeenCalled(); + expect(tradeShip.delete).toHaveBeenCalledWith(false); expect(tradeShipExecution.isActive()).toBe(false); expect(game.displayMessage).toHaveBeenCalled(); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
resources/lang/en.json(1 hunks)src/core/execution/TradeShipExecution.ts(3 hunks)tests/core/executions/TradeShipExecution.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/core/execution/TradeShipExecution.ts
- resources/lang/en.json
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/core/game/Game.ts (1)
499-501: Add documentation for TradeShip-specific methods.These methods lack documentation explaining their purpose and behavior. Since they're marked "Only for trade ships", clarify:
- What happens when called on non-TradeShip units?
- What does
distanceTraveled()return when not set (given it's optional in params)?- What is the source unit (Port, City, etc.)?
Apply this diff to add documentation:
// Trade Ships setSafeFromPirates(): void; // Only for trade ships isSafeFromPirates(): boolean; // Only for trade ships - sourceUnit(): Unit | undefined; // Only for trade ships - distanceTraveled(): number; // Only for trade ships - setDistanceTraveled(distance: number): void; // Only for trade ships + sourceUnit(): Unit | undefined; // Only for trade ships - returns the Port or City that created this TradeShip + distanceTraveled(): number; // Only for trade ships - returns distance traveled for trade value calculation (0 if not set) + setDistanceTraveled(distance: number): void; // Only for trade ships - updates distance traveled
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/core/game/Game.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-12T00:31:50.144Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 1752
File: src/core/game/Game.ts:750-752
Timestamp: 2025-08-12T00:31:50.144Z
Learning: In the OpenFrontIO codebase, changes to the PlayerInteraction interface (like adding canDonateGold and canDonateTroops flags) do not require corresponding updates to src/core/Schemas.ts or server serialization code.
Applied to files:
src/core/game/Game.ts
📚 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/game/Game.ts
| [UnitType.TradeShip]: { | ||
| targetUnit: Unit; | ||
| sourceUnit: Unit; | ||
| lastSetSafeFromPirates?: number; | ||
| distanceTraveled?: number; | ||
| }; |
There was a problem hiding this comment.
Fix type consistency between params and interface methods.
The param types don't match the interface method signatures:
- Line 256:
sourceUnit: Unit(required) conflicts with line 499:sourceUnit(): Unit | undefined - Line 258:
distanceTraveled?: number(optional) conflicts with line 500:distanceTraveled(): number(non-optional)
Either:
- Make
sourceUnitoptional in params:sourceUnit?: Unit, OR make the interface return non-optional:sourceUnit(): Unit - Make
distanceTraveledrequired in params:distanceTraveled: number, OR make the interface return optional:distanceTraveled(): number | undefined
Based on the context that TradeShips are created with a source Port and track distance as they travel, the recommended fix is:
[UnitType.TradeShip]: {
targetUnit: Unit;
- sourceUnit: Unit;
+ sourceUnit?: Unit;
lastSetSafeFromPirates?: number;
distanceTraveled?: number;
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| [UnitType.TradeShip]: { | |
| targetUnit: Unit; | |
| sourceUnit: Unit; | |
| lastSetSafeFromPirates?: number; | |
| distanceTraveled?: number; | |
| }; | |
| [UnitType.TradeShip]: { | |
| targetUnit: Unit; | |
| sourceUnit?: Unit; | |
| lastSetSafeFromPirates?: number; | |
| distanceTraveled?: number; | |
| }; |
🤖 Prompt for AI Agents
In src/core/game/Game.ts around lines 254 to 259, the TradeShip param types are
inconsistent with the interface methods: make sourceUnit optional and
distanceTraveled required to match the method signatures; change the param
declaration to use sourceUnit?: Unit and distanceTraveled: number so
sourceUnit() can still return Unit | undefined while distanceTraveled()
continues to return a non-optional number.
evanpelle
left a comment
evanpelle
left a comment
There was a problem hiding this comment.
I'm just thinking this is actually much more complicated than I originally thought.
- Since gold is determined by number of ports, src & dst players will have a different amount of gold
- Capturing a trade ship changes the amount of gold, eg the captor has fewer ports than the original owner.
This could confuse players, seeing the number change like that...
What if we just show it from the view of "myPlayer". So the player sees how much gold they would get if it landed on their port. This keeps all the values consistent. They only change when the player builds or loses a port
…. Updated logic to retrieve the number of ports based on the current player instead of the unit owner, ensuring accurate gold calculation for trade ships.
3 participants
Description:
Fixes #2354 and #2353: display ship information in the unit details panel.
Used simple manhattan distance function for boat value estimate.
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
kerverse