feat: allow trade ships to trade with own ports at reduced gold#3785
Open
Zixer1 wants to merge 3 commits intoopenfrontio:mainfrom
Open
feat: allow trade ships to trade with own ports at reduced gold#3785Zixer1 wants to merge 3 commits intoopenfrontio:mainfrom
Zixer1 wants to merge 3 commits intoopenfrontio:mainfrom
Conversation
Ports can now send trade ships to the player's own ports, useful in late game when embargoes cut off all external trade partners. - Self-trade gives 40% of normal trade gold (similar to factory self-provision ratio) - Gold is credited once (not double like cross-player trade) - Same port cannot trade with itself - Own ports get base weight only (no proximity/friendly bonuses) so external trade is still preferred when available - Self-trade ships skip embargo checks but still require destination port to be active Config: tradeShipSelfGoldMultiplier() = 0.4
Contributor
WalkthroughThis pull request introduces self-trade functionality for trade ships, enabling players to trade between their own ports. It adds a configuration multiplier for self-trade gold earnings, updates port selection logic to distinguish own ports from competitor ports, and modifies trade completion logic to apply different gold payouts and messaging for self-trades versus regular trades. Changes
Sequence Diagram(s)sequenceDiagram
participant Player
participant PortExecution
participant TradeShipExecution
participant Config
participant SourcePort
participant DestPort
Player->>PortExecution: tradingPorts()
PortExecution->>PortExecution: Separate own ports vs other players' ports
PortExecution->>SourcePort: owner()
SourcePort-->>PortExecution: Owner ID
PortExecution->>DestPort: owner()
DestPort-->>PortExecution: Owner ID
PortExecution->>PortExecution: Identify isSelfTrade
PortExecution->>PortExecution: Apply base weight only for self-trades
PortExecution-->>Player: ports list
Player->>TradeShipExecution: tick()
TradeShipExecution->>TradeShipExecution: Compute isSelfTrade from source/dest owners
TradeShipExecution->>DestPort: isActive()
alt Destination inactive or owner changed
TradeShipExecution->>TradeShipExecution: cancelTrade()
else Trade completed
alt Self-trade
TradeShipExecution->>Config: tradeShipSelfGoldMultiplier()
Config-->>TradeShipExecution: 0.4
TradeShipExecution->>SourcePort: Credit gold with multiplier
TradeShipExecution-->>Player: Single RECEIVED_GOLD_FROM_TRADE message
else Regular trade
TradeShipExecution->>SourcePort: Credit gold (normal)
TradeShipExecution-->>Player: Two RECEIVED_GOLD_FROM_TRADE messages
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
tests/PortExecution.test.ts (1)
108-132: Tighten the assertion totoBe(1).With
proximityBonusPortsNb=0,tradeShipShortRangeDebuff=0, both ports at level 1, and the source port excluded, the result should be exactly 1 — same as the "no proximity/friendly bonuses" test below. UsingtoBeGreaterThanOrEqual(1)would silently pass if a future regression accidentally added bonus weight to own ports.✏️ Proposed tightening
- // Should include the player's other port - expect(ports.length).toBeGreaterThanOrEqual(1); - expect(ports.some((p) => p.owner() === player)).toBe(true); + // Should include the player's other port exactly once (base weight, no bonuses) + expect(ports.length).toBe(1); + expect(ports[0].owner()).toBe(player);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/PortExecution.test.ts` around lines 108 - 132, The test in PortExecution.test.ts is too loose: in the "Self-trade: own ports appear as destinations" test, tighten the assertion on the ports list returned by PortExecution.tradingPorts() from expect(ports.length).toBeGreaterThanOrEqual(1) to expect(ports.length).toBe(1) so the test fails on any unexpected extra ports (given the configured proximityBonusPortsNb and tradeShipShortRangeDebuff are zero and the source port is excluded); locate the assertion after calling new PortExecution(port1).init(...).tick(...) and update the length expectation accordingly while keeping the subsequent expect(ports.some((p) => p.owner() === player)).toBe(true).src/core/execution/PortExecution.ts (1)
99-153: LGTM — own/other split is clean and the cachedownerremoves repeated calls.A few small notes for awareness, none blocking:
- Own ports now contribute to
ports.length, which feedsproximityBonusPortsNb(ports.length). SinceproximityBonusPortsNbgrows with total ports, this slightly widens the proximity bonus window for other-player ports. This actually nudges things further toward "external trade preferred", which matches the PR intent — just be aware of it if you tune the formula later.p !== this.port!andp !== ownerare reference checks — fine here, but ifPlayer/Unitever gain a copy/view variant, prefer comparing by id (e.g.p.id() !== owner.id()).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/execution/PortExecution.ts` around lines 99 - 153, Two small issues to consider: proximityBonusPortsNb is currently called with ports.length that includes ownPorts (which widens the proximity bonus window for external ports) and the reference checks like the own-port filter use object identity (p !== this.port!) which could break if Unit/Player gain view/copy semantics; to fix, compute a separate count for proximity (e.g. const proximityCount = ports.length - ownPorts.length or build a portsForProximity array) and pass proximityCount to this.mg.config().proximityBonusPortsNb(...), and replace identity checks with id comparisons (e.g. owner.units(...).filter(p => p.id() !== this.port!.id()) and any other p !== this.port! checks -> compare p.id() to this.port!.id()); update references in PortExecution.tradingPorts and where proximityBonusPortsNb is called.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/core/execution/TradeShipExecution.ts`:
- Around line 202-220: The self-trade branch in TradeShipExecution (inside the
this.isSelfTrade block) currently calls this.mg.displayMessage with the same
message key and name placeholder as cross-player trades, so the popup shows the
player's own displayName as the trade partner; change the display to a
self-trade-specific message by replacing the message key (e.g. use
"events_display.received_gold_from_self_trade") or remove the partner name
placeholder in the this.mg.displayMessage call for the this.isSelfTrade branch,
and update any corresponding stats/params so the payload no longer passes
srcPort.owner().displayName() as the partner name (adjust message params where
this.srcPort.owner().displayName() is passed).
- Around line 85-100: The self-trade branch currently only verifies
_dstPort.isActive() and skips validating ownership, so a port captured
mid-journey can cause a self-trade to incorrectly pay the original owner; update
TradeShipExecution to re-check ownership for self-trades by calling
tradeShipOwner.canTrade(dstPortOwner) (or comparing expected owner id vs current
dstPort.owner) before allowing completion: if the ownership check fails, call
this.tradeShip.delete(false) and set this.active = false (same behavior as the
non-self branch) — place this check where the self-trade active check currently
is (the isSelfTrade && !this._dstPort.isActive() block) or immediately before
invoking complete() so arrival honors current dstPort ownership.
---
Nitpick comments:
In `@src/core/execution/PortExecution.ts`:
- Around line 99-153: Two small issues to consider: proximityBonusPortsNb is
currently called with ports.length that includes ownPorts (which widens the
proximity bonus window for external ports) and the reference checks like the
own-port filter use object identity (p !== this.port!) which could break if
Unit/Player gain view/copy semantics; to fix, compute a separate count for
proximity (e.g. const proximityCount = ports.length - ownPorts.length or build a
portsForProximity array) and pass proximityCount to
this.mg.config().proximityBonusPortsNb(...), and replace identity checks with id
comparisons (e.g. owner.units(...).filter(p => p.id() !== this.port!.id()) and
any other p !== this.port! checks -> compare p.id() to this.port!.id()); update
references in PortExecution.tradingPorts and where proximityBonusPortsNb is
called.
In `@tests/PortExecution.test.ts`:
- Around line 108-132: The test in PortExecution.test.ts is too loose: in the
"Self-trade: own ports appear as destinations" test, tighten the assertion on
the ports list returned by PortExecution.tradingPorts() from
expect(ports.length).toBeGreaterThanOrEqual(1) to expect(ports.length).toBe(1)
so the test fails on any unexpected extra ports (given the configured
proximityBonusPortsNb and tradeShipShortRangeDebuff are zero and the source port
is excluded); locate the assertion after calling new
PortExecution(port1).init(...).tick(...) and update the length expectation
accordingly while keeping the subsequent expect(ports.some((p) => p.owner() ===
player)).toBe(true).
🪄 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: bbff1d47-aefe-450e-8ac8-a8bc7461ef51
📒 Files selected for processing (5)
src/core/configuration/Config.tssrc/core/configuration/DefaultConfig.tssrc/core/execution/PortExecution.tssrc/core/execution/TradeShipExecution.tstests/PortExecution.test.ts
Comment on lines
+202
to
+220
| } else if (this.isSelfTrade) { | ||
| // Self-trade: reduced gold, credited once | ||
| const multiplier = this.mg.config().tradeShipSelfGoldMultiplier(); | ||
| gold = BigInt(Math.floor(Number(gold) * multiplier)); | ||
| this.srcPort.owner().addGold(gold, this._dstPort.tile()); | ||
| this.mg.displayMessage( | ||
| "events_display.received_gold_from_trade", | ||
| MessageType.RECEIVED_GOLD_FROM_TRADE, | ||
| this.srcPort.owner().id(), | ||
| gold, | ||
| { | ||
| gold: renderNumber(gold), | ||
| name: this.srcPort.owner().displayName(), | ||
| }, | ||
| ); | ||
| // Record stats | ||
| this.mg | ||
| .stats() | ||
| .boatArriveTrade(this.srcPort.owner(), this._dstPort.owner(), gold); |
Contributor
There was a problem hiding this comment.
Self-trade message shows the player their own name.
The cross-player branch below shows the source the destination player's name and vice versa. Here the recipient is srcPort.owner() and the name placeholder is also srcPort.owner().displayName(), so the popup reads as if you traded with yourself by name. Functionally fine, but a small UX nit — consider using a self-trade-specific message key (e.g. events_display.received_gold_from_self_trade) or omitting the partner name so the wording reflects the new mechanic.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/execution/TradeShipExecution.ts` around lines 202 - 220, The
self-trade branch in TradeShipExecution (inside the this.isSelfTrade block)
currently calls this.mg.displayMessage with the same message key and name
placeholder as cross-player trades, so the popup shows the player's own
displayName as the trade partner; change the display to a self-trade-specific
message by replacing the message key (e.g. use
"events_display.received_gold_from_self_trade") or remove the partner name
placeholder in the this.mg.displayMessage call for the this.isSelfTrade branch,
and update any corresponding stats/params so the payload no longer passes
srcPort.owner().displayName() as the partner name (adjust message params where
this.srcPort.owner().displayName() is passed).
Contributor
Author
There was a problem hiding this comment.
Eh, This should be fine.. Too many different chat labels might get confusing for the player
Contributor
There was a problem hiding this comment.
@Zixer1, fair point — keeping the label count low is a reasonable UX decision. Acknowledged, no change needed here! 👍
(⳹´･ω･)ﾉ
🧠 Learnings used
Learnt from: deshack
Repo: openfrontio/OpenFrontIO PR: 3244
File: src/core/execution/nation/NationStructureBehavior.ts:621-625
Timestamp: 2026-02-19T08:49:23.183Z
Learning: In src/core/execution/nation/NationStructureBehavior.ts factoryValue() and TrainStation.ts, when determining trade relationship type for rail network connectivity scoring, the check order `player.isOnSameTeam(neighbor) ? "team" : player.isAlliedWith(neighbor) ? "ally" : "other"` is intentional. Team relationships take precedence over ally relationships, so players who are both on the same team AND allied receive the lower "team" trade gold, not "ally" gold. This reflects game design: team members are inherently cooperative (lower incentive needed), while allies from separate teams need higher incentives.
Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 2607
File: src/core/execution/PlayerExecution.ts:271-295
Timestamp: 2025-12-13T14:58:29.645Z
Learning: In src/core/execution/PlayerExecution.ts surroundedBySamePlayer(), the `as Player` cast on `mg.playerBySmallID(scan.enemyId)` is intentional. Since scan.enemyId comes from ownerID() on an owned tile and playerBySmallID() only returns Player or undefined, the cast expresses a known invariant. The maintainers prefer loud failures (runtime errors) over silent masking (early returns with guards) for corrupted game state scenarios at trusted call sites.
Learnt from: andybellenie
Repo: openfrontio/OpenFrontIO PR: 3509
File: src/server/GameServer.ts:667-680
Timestamp: 2026-03-24T23:21:38.781Z
Learning: In openfrontio/OpenFrontIO, the clan-tag privacy feature in `src/server/GameServer.ts` (`gameInfoForClient()`, PR `#3509`) is intentionally a partial mitigation against tag sniping, not a complete fix. A larger clan system rework is in development. The bypass via rejoin with a different clanTag (identityUpdate path in Worker.ts) is a known, accepted limitation. Do not flag this as a critical issue in future reviews of this file.
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: 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.
github-project-automation
Bot
moved this from Triage
to Development
in OpenFront Release Management
Zixer1
force-pushed
the
feat/self-port-trading
branch
from
d8b25a9 to
ad0e289
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (8)
src/client/graphics/layers/AttacksDisplay.ts (1)
380-389: Minor:disabledon the cancel button is dead code.The button is only rendered in the
elsebranch whereboat.warshipState() === "patrolling"is already true, sodisabled: boat.warshipState() !== "patrolling"will always evaluate tofalse. It mirrors the same harmless dead pattern at lines 289–295 / 325–331 (disabled: attack.retreating), so feel free to keep it for stylistic symmetry — but it could just be dropped.♻️ Optional cleanup
: this.renderButton({ content: "\u274C", onClick: () => this.emitBoatCancelIntent(boat.id()), className: "ml-auto text-left shrink-0", - disabled: boat.warshipState() !== "patrolling", })}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/graphics/layers/AttacksDisplay.ts` around lines 380 - 389, The cancel button's disabled prop is redundant because it's only rendered in the else branch where boat.warshipState() === "patrolling", so disabled: boat.warshipState() !== "patrolling" is always false; remove the dead expression from the renderButton call in AttacksDisplay (look for renderButton invocation that passes disabled and the surrounding ternary using boat.warshipState()), leaving the button enabled and still using onClick: () => this.emitBoatCancelIntent(boat.id()) for cancellation behavior.src/core/game/GameView.ts (1)
125-130:UnitView.retreatPort()is permanently stubbed.This always returns
undefinedbecauseretreatPortisn't part ofUnitUpdate. That's fine today (no client consumer), but it's a silent foot-gun for future UI work that may want to draw the retreat target. Either thread the field throughUnitUpdateor add a short JSDoc note like// Not exposed to client; server-only state.so the gap is obvious.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/game/GameView.ts` around lines 125 - 130, The UnitView.retreatPort() method is a permanent stub returning undefined and should either be exposed via UnitUpdate or clearly documented as server-only; update the code by either (A) adding a retreatPort?: TileRef | undefined field to the UnitUpdate shape and threading it through construction and update logic so UnitView.retreatPort() returns that value, or (B) if the field must remain server-only, add a concise JSDoc comment on UnitView.retreatPort() and setRetreatPort() (and optionally on the UnitView class) stating “Not exposed to client; server-only state” so future authors aren’t surprised; locate and modify the UnitView class methods retreatPort and setRetreatPort and the UnitUpdate type/object to implement the chosen option.tests/Warship.test.ts (1)
701-714: Brittle coupling to the 50-tick magic constant.
executeTicks(game, 48)plus a 5-tick window encodes themanualMoveRetreatDisabledDuration = 50constant fromWarshipExecution.tsas two separate numbers. If that constant ever changes, this test will silently pass for wrong reasons or fail mysteriously. Consider exposing the duration via a config getter (e.g.,warshipManualOverrideDuration()) and reading it here, or at least adding a comment that ties 48/5 to the implementation constant.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Warship.test.ts` around lines 701 - 714, The test hardcodes 48 and 5 to mirror the internal manualMoveRetreatDisabledDuration (50) in WarshipExecution.ts, which is brittle; expose that duration via a getter (e.g., warshipManualOverrideDuration() or export manualMoveRetreatDisabledDuration) and use it in the test to compute the ticks to run (e.g., executeTicks(game, duration - 2) and loop for a small window like 1..3 or compute window = duration - executedTicks) so the test reads the canonical constant instead of magic numbers; locate usages around executeTicks, warship.warshipState(), and game.executeNextTick() and replace the 48/5 literals with the value from the new getter/export.src/core/game/Game.ts (2)
877-877:unit(id)returnsundefinedon miss, whileplayer(id)throws.Small consistency nit:
Game.player(id)throws when the player is missing, but the newunit(id)returnsundefined. New callers (e.g.,HealAtPortExecution,DeleteUnitExecution,UpgradeStructureExecution) all have to add a defensiveif (!unit)warning path, which can drift over time. Consider either documenting the contract on the interface (/**@returnsundefined if id is unknown or the unit was already deleted */) or aligning withplayer()semantics for unknown ids. Either is fine; the important part is to make the contract explicit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/game/Game.ts` at line 877, The Game.unit(id: number) API currently returns undefined on miss while Game.player(id) throws; make the contract explicit and consistent: either update the Game.unit signature/implementation to throw the same Error as Game.player when a unit id is not found, or add a clear JSDoc on Game.unit (e.g., "/** `@returns` undefined if id is unknown or the unit was already deleted */") and keep the current behavior; if you choose throwing, update callers such as HealAtPortExecution, DeleteUnitExecution, and UpgradeStructureExecution to remove defensive checks and rely on the exception, otherwise add the JSDoc and review those callers to ensure they handle undefined appropriately.
620-657:Unitinterface is becoming a grab-bag; consider a tagged union per role.This PR adds six more warship-only methods (
warshipState,setWarshipState,isInCombat,setInCombat,retreatPort,setRetreatPort) to aUnitinterface that already contains nuke-only, trade-ship-only, train-only, SAM-only, and silo-only members. Every call site needs to know which methods are valid for whichtype(), andUnitViewalready has to throw at runtime for unsupported setters.A typed-union shape based on
UnitTypewould catch a lot of these mismatches at compile time and aligns with the project's preference for composition-over-inheritance / typed unions over class hierarchies. Roughly:type CommonUnit = { /* id, type, owner, tile, isActive, ... */ }; type WarshipUnit = CommonUnit & { type(): UnitType.Warship; warshipState(): WarshipMovementState; setWarshipState(state: WarshipMovementState): void; isInCombat(): boolean; setInCombat(): void; retreatPort(): TileRef | undefined; setRetreatPort(tile: TileRef | undefined): void; // patrolTile, etc. }; type TradeShipUnit = CommonUnit & { type(): UnitType.TradeShip; /* ... */ }; // ... type Unit = WarshipUnit | TradeShipUnit | /* ... */;Doesn't have to happen in this PR, but worth opening a follow-up so warship-only methods stop leaking onto trains/missiles/etc.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/game/Game.ts` around lines 620 - 657, The Unit interface is leaking many role-specific methods (e.g., warshipState, setWarshipState, isInCombat, setInCombat, retreatPort, setRetreatPort) across all unit types; replace this with a tagged-union: extract common members into a CommonUnit (id, type(), owner, tile, isActive, health, troops, etc.), define WarshipUnit = CommonUnit & { type(): UnitType.Warship; warshipState(): WarshipMovementState; setWarshipState(state: WarshipMovementState): void; isInCombat(): boolean; setInCombat(): void; retreatPort(): TileRef | undefined; setRetreatPort(tile?: TileRef): void; patrolTile(), etc. }, and analogous TradeShipUnit, SAMUnit, SiloUnit, TrainUnit types, then change Unit = WarshipUnit | TradeShipUnit | ...; update all call sites (and UnitView) to narrow on unit.type() or use type predicates so only valid methods are invoked for each branch. Ensure implementations/classes implement the specific interface matching their UnitType and adjust any runtime throws in UnitView to compile-time safe narrowing.src/core/execution/WarshipExecution.ts (2)
226-309:findBestTarget: nice consolidation, but the trade-ship branch is leaking.Two small points:
patrolTile!andpatrolRangeSquared!non-null assertions on lines 287–288 work today only becausehasPortis computed in the sameifblock first. A future edit that reorders these blocks loses that invariant silently. Consider just initializing them eagerly whenincludeTradeShipsis true, or extracting the trade-ship filter into its own helper that takes the precomputed values explicitly.- The function does two clearly different jobs (combat targeting vs. trade-ship hunting) and only shares the exclusion predicate. Splitting into
findCombatTarget(types)andfindTradeShipTarget()that both call a sharedisCandidate(unit)helper would remove the boolean parameter and read more cleanly for non-native English speakers.Both are nice-to-haves; correctness looks fine.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/execution/WarshipExecution.ts` around lines 226 - 309, The trade-ship handling in findBestTarget leaks nullable state via patrolTile! and patrolRangeSquared! and mixes two responsibilities; fix by extracting the trade-ship filtering into a separate helper (e.g., isTradeShipHuntCandidate(unit, patrolTile, patrolRangeSquared, warshipComponent)) and eagerly computing the trade-hunt-specific values when includeTradeShips is true (compute hasPort, patrolTile, patrolRangeSquared, and warshipComponent before iterating), or split findBestTarget into findCombatTarget(types) and findTradeShipTarget() that both reuse a shared isCandidate(unit) predicate; update callers to use the new functions and remove any non-null assertions inside the loop.
536-599: Four near-identical "available port" helpers — names are hard to tell apart.
nearestAvailablePortTile,findNearestAvailablePort,findBetterPortTile,findNearestAvailablePortTilediffer by one or two words and one of them takes anexcludeShip. For non-native English speakers this is genuinely confusing. Suggest a small rename pass:
nearestAvailablePort(excludeShip?)→ returns{ tile, distSquared } | undefinednearestAvailablePortExcludingSelf()→ wraps the above withthis.warshipcloserAvailablePortThanCurrent()→ for the "switch port" caseNo behavior change needed — just clarity.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/execution/WarshipExecution.ts` around lines 536 - 599, Rename the three helpers to clearer, distinct names and update all references: change nearestAvailablePortTile(excludeShip?) to nearestAvailablePort(excludeShip?) and keep its return type { tile, distSquared } | undefined; change the existing findNearestAvailablePortTile() wrapper to nearestAvailablePortExcludingSelf() which calls nearestAvailablePort(this.warship) and returns .tile; change findBetterPortTile() to closerAvailablePortThanCurrent() and keep its logic but call nearestAvailablePort() for the result; ensure you update any call sites that referenced nearestAvailablePortTile, findNearestAvailablePort, findBetterPortTile, or findNearestAvailablePortTile to use the new names (nearestAvailablePort, nearestAvailablePortExcludingSelf, closerAvailablePortThanCurrent) so behavior is unchanged.src/core/game/UnitImpl.ts (1)
373-378:orderBoatRetreatmixing transport-ship state into "warshipState".Now that transport-ship retreat is also stored in
_warshipState, the field name is misleading — it really means "non-patrolling movement state for any boat". Either rename it (e.g.,_movementState/MovementState) or split into two fields (one per unit role) so that the type only carries states that are valid for that role. This fits composition-over-inheritance better than treating one tagged-string union as covering two unrelated kinds of unit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/game/UnitImpl.ts` around lines 373 - 378, The method orderBoatRetreat in UnitImpl currently writes retreat state into the misleading _warshipState via setWarshipState even for TransportShip; change the model so the state only represents the correct role: either (A) rename _warshipState and APIs (setWarshipState, warshipState, WarshipState) to a neutral name like _movementState / setMovementState / MovementState and update all callers including orderBoatRetreat and warship-specific logic to use the new neutral names, or (B) split into two distinct fields and APIs (e.g., _warshipState with setWarshipState for warship-only logic and _transportMovementState with setTransportMovementState for transport/boat movement) and update orderBoatRetreat to set the transport-specific field and leave warship logic unchanged; locate usages in UnitImpl (orderBoatRetreat, setWarshipState) and any consumers that read/write warshipState and update them accordingly to preserve type-safety and only allow valid states per unit role.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/client/graphics/layers/UnitLayer.ts`:
- Around line 477-496: drawRetreatCross currently blinks using Date.now() but is
only called from handleWarShipEvent for units in updatesSinceLastTick or
motionPlannedUnitIds, so stationary/docked warships won't be redrawn each tick
and their blink freezes; either ensure docked/retreating ships are included in
per-tick redraws (extend motionPlannedUnitIds semantics or add a needsBlink set
and include its IDs in the tick update path so handleWarShipEvent is invoked
every frame for those units) or remove the blink gate in drawRetreatCross and
render the marker statically; also consider renaming drawRetreatCross to a
neutral name like drawNonPatrolMarker and update its callers for clarity.
In `@src/core/execution/HealAtPortExecution.ts`:
- Around line 4-7: The JSDoc for HealAtPortExecution mismatches the
implementation: update either the text or the check in isFriendlyPort —
currently isFriendlyPort iterates this.owner.units(UnitType.Port) (only
same-player ports). Either change the docstring to "Only works with ports owned
by the same player" or broaden the logic in isFriendlyPort to include allied
players' ports (e.g., query allied players' units or use a helper that checks
alliance status) so the behavior and comment match; refer to isFriendlyPort and
the this.owner.units(UnitType.Port) iteration to locate the code to change.
In `@src/core/game/UnitImpl.ts`:
- Around line 364-371: Extract the magic number into a named constant (e.g.
IN_COMBAT_TICKS = 3) and make isInCombat() use that constant (replace <= 3).
Also ensure the client gets notified when the combat flag expires: when setting
_lastCombatTick in setInCombat(), schedule an explicit update at tick
(_lastCombatTick + IN_COMBAT_TICKS + 1) so that mg.addUpdate(this.toUpdate()) is
called when isInCombat() transitions from true to false; if your game manager
has a tick scheduling API (e.g. mg.scheduleUpdateAt or similar) use that,
otherwise ensure the per-tick handler emits an update when isInCombat() flips to
false. Ensure references: isInCombat, setInCombat, _lastCombatTick,
mg.addUpdate, toUpdate.
In `@tests/Warship.test.ts`:
- Around line 716-733: The test currently asserts only the immediate
post-initialization state; to ensure the retreat isn't cancelled on subsequent
ticks by WarshipExecution.handleManualPatrolOverride, run several game ticks
(call game.executeNextTick() in a short loop) after adding the
HealAtPortExecution and then assert the warship's patrolTile() and targetTile()
still equal portTile and warship.warshipState() is not "patrolling" (or
otherwise validate it reached/stayed in retreat) to prove steady-state behavior
for HealAtPortExecution and WarshipExecution interaction.
---
Nitpick comments:
In `@src/client/graphics/layers/AttacksDisplay.ts`:
- Around line 380-389: The cancel button's disabled prop is redundant because
it's only rendered in the else branch where boat.warshipState() ===
"patrolling", so disabled: boat.warshipState() !== "patrolling" is always false;
remove the dead expression from the renderButton call in AttacksDisplay (look
for renderButton invocation that passes disabled and the surrounding ternary
using boat.warshipState()), leaving the button enabled and still using onClick:
() => this.emitBoatCancelIntent(boat.id()) for cancellation behavior.
In `@src/core/execution/WarshipExecution.ts`:
- Around line 226-309: The trade-ship handling in findBestTarget leaks nullable
state via patrolTile! and patrolRangeSquared! and mixes two responsibilities;
fix by extracting the trade-ship filtering into a separate helper (e.g.,
isTradeShipHuntCandidate(unit, patrolTile, patrolRangeSquared,
warshipComponent)) and eagerly computing the trade-hunt-specific values when
includeTradeShips is true (compute hasPort, patrolTile, patrolRangeSquared, and
warshipComponent before iterating), or split findBestTarget into
findCombatTarget(types) and findTradeShipTarget() that both reuse a shared
isCandidate(unit) predicate; update callers to use the new functions and remove
any non-null assertions inside the loop.
- Around line 536-599: Rename the three helpers to clearer, distinct names and
update all references: change nearestAvailablePortTile(excludeShip?) to
nearestAvailablePort(excludeShip?) and keep its return type { tile, distSquared
} | undefined; change the existing findNearestAvailablePortTile() wrapper to
nearestAvailablePortExcludingSelf() which calls
nearestAvailablePort(this.warship) and returns .tile; change
findBetterPortTile() to closerAvailablePortThanCurrent() and keep its logic but
call nearestAvailablePort() for the result; ensure you update any call sites
that referenced nearestAvailablePortTile, findNearestAvailablePort,
findBetterPortTile, or findNearestAvailablePortTile to use the new names
(nearestAvailablePort, nearestAvailablePortExcludingSelf,
closerAvailablePortThanCurrent) so behavior is unchanged.
In `@src/core/game/Game.ts`:
- Line 877: The Game.unit(id: number) API currently returns undefined on miss
while Game.player(id) throws; make the contract explicit and consistent: either
update the Game.unit signature/implementation to throw the same Error as
Game.player when a unit id is not found, or add a clear JSDoc on Game.unit
(e.g., "/** `@returns` undefined if id is unknown or the unit was already deleted
*/") and keep the current behavior; if you choose throwing, update callers such
as HealAtPortExecution, DeleteUnitExecution, and UpgradeStructureExecution to
remove defensive checks and rely on the exception, otherwise add the JSDoc and
review those callers to ensure they handle undefined appropriately.
- Around line 620-657: The Unit interface is leaking many role-specific methods
(e.g., warshipState, setWarshipState, isInCombat, setInCombat, retreatPort,
setRetreatPort) across all unit types; replace this with a tagged-union: extract
common members into a CommonUnit (id, type(), owner, tile, isActive, health,
troops, etc.), define WarshipUnit = CommonUnit & { type(): UnitType.Warship;
warshipState(): WarshipMovementState; setWarshipState(state:
WarshipMovementState): void; isInCombat(): boolean; setInCombat(): void;
retreatPort(): TileRef | undefined; setRetreatPort(tile?: TileRef): void;
patrolTile(), etc. }, and analogous TradeShipUnit, SAMUnit, SiloUnit, TrainUnit
types, then change Unit = WarshipUnit | TradeShipUnit | ...; update all call
sites (and UnitView) to narrow on unit.type() or use type predicates so only
valid methods are invoked for each branch. Ensure implementations/classes
implement the specific interface matching their UnitType and adjust any runtime
throws in UnitView to compile-time safe narrowing.
In `@src/core/game/GameView.ts`:
- Around line 125-130: The UnitView.retreatPort() method is a permanent stub
returning undefined and should either be exposed via UnitUpdate or clearly
documented as server-only; update the code by either (A) adding a retreatPort?:
TileRef | undefined field to the UnitUpdate shape and threading it through
construction and update logic so UnitView.retreatPort() returns that value, or
(B) if the field must remain server-only, add a concise JSDoc comment on
UnitView.retreatPort() and setRetreatPort() (and optionally on the UnitView
class) stating “Not exposed to client; server-only state” so future authors
aren’t surprised; locate and modify the UnitView class methods retreatPort and
setRetreatPort and the UnitUpdate type/object to implement the chosen option.
In `@src/core/game/UnitImpl.ts`:
- Around line 373-378: The method orderBoatRetreat in UnitImpl currently writes
retreat state into the misleading _warshipState via setWarshipState even for
TransportShip; change the model so the state only represents the correct role:
either (A) rename _warshipState and APIs (setWarshipState, warshipState,
WarshipState) to a neutral name like _movementState / setMovementState /
MovementState and update all callers including orderBoatRetreat and
warship-specific logic to use the new neutral names, or (B) split into two
distinct fields and APIs (e.g., _warshipState with setWarshipState for
warship-only logic and _transportMovementState with setTransportMovementState
for transport/boat movement) and update orderBoatRetreat to set the
transport-specific field and leave warship logic unchanged; locate usages in
UnitImpl (orderBoatRetreat, setWarshipState) and any consumers that read/write
warshipState and update them accordingly to preserve type-safety and only allow
valid states per unit role.
In `@tests/Warship.test.ts`:
- Around line 701-714: The test hardcodes 48 and 5 to mirror the internal
manualMoveRetreatDisabledDuration (50) in WarshipExecution.ts, which is brittle;
expose that duration via a getter (e.g., warshipManualOverrideDuration() or
export manualMoveRetreatDisabledDuration) and use it in the test to compute the
ticks to run (e.g., executeTicks(game, duration - 2) and loop for a small window
like 1..3 or compute window = duration - executedTicks) so the test reads the
canonical constant instead of magic numbers; locate usages around executeTicks,
warship.warshipState(), and game.executeNextTick() and replace the 48/5 literals
with the value from the new getter/export.
🪄 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: ee827bdf-65a4-4b89-9954-44a684219691
📒 Files selected for processing (16)
src/client/graphics/layers/AttacksDisplay.tssrc/client/graphics/layers/UnitLayer.tssrc/client/graphics/ui/NavalTarget.tssrc/core/execution/DeleteUnitExecution.tssrc/core/execution/HealAtPortExecution.tssrc/core/execution/TradeShipExecution.tssrc/core/execution/TransportShipExecution.tssrc/core/execution/UpgradeStructureExecution.tssrc/core/execution/WarshipExecution.tssrc/core/game/Game.tssrc/core/game/GameImpl.tssrc/core/game/GameUpdates.tssrc/core/game/GameView.tssrc/core/game/UnitImpl.tstests/Warship.test.tstests/core/executions/TradeShipExecution.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/core/execution/TradeShipExecution.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/core/execution/TradeShipExecution.ts (2)
33-33: Use.id()equality on Player for consistency.Line 82 in the same diff compares with
dstPortOwner.id() === this.srcPort.owner().id(), but lines 33 and 87 compare Player references directly (===/!==). Both work in practice since Player instances are looked up by id, but mixing the two styles inside the same change is easy to misread. Suggest aligning on.id().♻️ Suggested change
- this.isSelfTrade = srcPort.owner() === _dstPort.owner(); + this.isSelfTrade = srcPort.owner().id() === _dstPort.owner().id();(and the matching
dstPortOwner.id() !== this.srcPort.owner().id()on line 87 if you keep that branch).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/execution/TradeShipExecution.ts` at line 33, The isSelfTrade assignment uses object reference equality between players (this.isSelfTrade = srcPort.owner() === _dstPort.owner()), which is inconsistent with other comparisons using .id() (e.g., dstPortOwner.id() === this.srcPort.owner().id()); change the check to compare player ids instead (call .id() on both srcPort.owner() and _dstPort.owner()) so all player equality checks (including the related dstPortOwner.id() !== this.srcPort.owner().id() branch) use the same .id() style for consistency.
199-200:Number(gold)can lose precision for very large gold totals.
goldis abigint. Round-tripping throughNumberis safe only up to2^53 - 1. Realistic in-game gold values stay far below that ceiling today, but iftradeShipGoldever returns a larger value the floor-and-convert would silently truncate. Doing the math inbigintavoids the gotcha and is just as readable.♻️ Suggested fix
- // Self-trade: reduced gold, credited once - const multiplier = this.mg.config().tradeShipSelfGoldMultiplier(); - gold = BigInt(Math.floor(Number(gold) * multiplier)); + // Self-trade: reduced gold, credited once. + // Multiplier is a small fraction (e.g. 0.4); scale by 1000 to keep + // the math entirely in bigint and avoid Number precision loss. + const multiplier = this.mg.config().tradeShipSelfGoldMultiplier(); + const scale = 1000n; + const scaledMultiplier = BigInt(Math.round(multiplier * Number(scale))); + gold = (gold * scaledMultiplier) / scale;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/execution/TradeShipExecution.ts` around lines 199 - 200, The code converts bigint `gold` to Number which can lose precision; replace the Number(gold) * multiplier pattern (see this.mg.config().tradeShipSelfGoldMultiplier() and the local variable `gold` in TradeShipExecution) with a pure-bigint computation: represent the floating multiplier as an integer numerator/denominator (or scale it to an integer factor), multiply `gold` by the integer numerator, then integer-divide by the denominator to achieve the floor, ensuring you never cast `gold` to Number and you preserve bigint semantics.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/core/execution/TradeShipExecution.ts`:
- Around line 77-91: The current shouldCancel boolean in TradeShipExecution lets
a captured self-trade ship be deleted when the destination port is lost; update
the condition so capture takes precedence as in the non-self branch: ensure the
self-trade branch checks !this.wasCaptured before cancelling (i.e., mirror the
non-self clause that uses !this.wasCaptured && (!dstActive ||
!tradeShipOwner.canTrade(...))). Refactor the combined boolean into three
explicit cases for clarity: 1) non-self trade cancellation when dst was captured
by src owner, 2) non-self cancellation when NOT this.wasCaptured and (dst
inactive or tradeShipOwner.canTrade(dstPortOwner) is false), and 3) self-trade
cancellation only when NOT this.wasCaptured and (dst inactive or dstPortOwner
!== this.srcPort.owner()); call this.cancelTrade() and return when any case is
true.
---
Nitpick comments:
In `@src/core/execution/TradeShipExecution.ts`:
- Line 33: The isSelfTrade assignment uses object reference equality between
players (this.isSelfTrade = srcPort.owner() === _dstPort.owner()), which is
inconsistent with other comparisons using .id() (e.g., dstPortOwner.id() ===
this.srcPort.owner().id()); change the check to compare player ids instead (call
.id() on both srcPort.owner() and _dstPort.owner()) so all player equality
checks (including the related dstPortOwner.id() !== this.srcPort.owner().id()
branch) use the same .id() style for consistency.
- Around line 199-200: The code converts bigint `gold` to Number which can lose
precision; replace the Number(gold) * multiplier pattern (see
this.mg.config().tradeShipSelfGoldMultiplier() and the local variable `gold` in
TradeShipExecution) with a pure-bigint computation: represent the floating
multiplier as an integer numerator/denominator (or scale it to an integer
factor), multiply `gold` by the integer numerator, then integer-divide by the
denominator to achieve the floor, ensuring you never cast `gold` to Number and
you preserve bigint semantics.
🪄 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: 47bbd423-a8d9-478c-9906-480c512d20a2
📒 Files selected for processing (2)
src/core/execution/TradeShipExecution.tstests/core/executions/TradeShipExecution.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/core/executions/TradeShipExecution.test.ts
Comment on lines
+77
to
91
| const dstActive = this._dstPort.isActive(); | ||
| const shouldCancel = | ||
| // Non-self-trade: cancel if destination port was captured back, or | ||
| // (not yet captured) if trade is no longer viable | ||
| (!this.isSelfTrade && | ||
| (dstPortOwner.id() === this.srcPort.owner().id() || | ||
| (!this.wasCaptured && | ||
| (!dstActive || !tradeShipOwner.canTrade(dstPortOwner))))) || | ||
| // Self-trade: cancel if destination port is no longer active or was captured | ||
| (this.isSelfTrade && | ||
| (!dstActive || dstPortOwner !== this.srcPort.owner())); | ||
| if (shouldCancel) { | ||
| this.cancelTrade(); | ||
| return; | ||
| } |
Contributor
There was a problem hiding this comment.
Captured self-trade ship cannot reroute when destination is also lost.
For non-self trades, the (!this.wasCaptured && (!dstActive || !canTrade)) guard lets a captured ship fall through to the rerouting block at lines 95–118, so the new owner's ship can pick another friendly port instead of being deleted. The self-trade branch has no !this.wasCaptured guard, so if the trade ship gets captured by player C and the destination port is also lost (deactivated, or taken by yet another player D), this block deletes the captured ship instead of letting the captured-ship logic find one of C's ports.
Mirror the non-self path so capture takes precedence over the self-trade ownership check.
🛡️ Suggested fix
- const dstActive = this._dstPort.isActive();
- const shouldCancel =
- // Non-self-trade: cancel if destination port was captured back, or
- // (not yet captured) if trade is no longer viable
- (!this.isSelfTrade &&
- (dstPortOwner.id() === this.srcPort.owner().id() ||
- (!this.wasCaptured &&
- (!dstActive || !tradeShipOwner.canTrade(dstPortOwner))))) ||
- // Self-trade: cancel if destination port is no longer active or was captured
- (this.isSelfTrade &&
- (!dstActive || dstPortOwner !== this.srcPort.owner()));
- if (shouldCancel) {
- this.cancelTrade();
- return;
- }
+ const dstActive = this._dstPort.isActive();
+ const srcOwnerId = this.srcPort.owner().id();
+ let shouldCancel = false;
+ if (this.isSelfTrade) {
+ // Self-trade: cancel only if not yet captured and the round-trip
+ // is no longer possible (dst inactive or no longer owned by us).
+ shouldCancel =
+ !this.wasCaptured &&
+ (!dstActive || dstPortOwner.id() !== srcOwnerId);
+ } else if (this.wasCaptured) {
+ // Captured cross-player ship: only cancel if the source player
+ // already retook the destination.
+ shouldCancel = dstPortOwner.id() === srcOwnerId;
+ } else {
+ shouldCancel =
+ dstPortOwner.id() === srcOwnerId ||
+ !dstActive ||
+ !tradeShipOwner.canTrade(dstPortOwner);
+ }
+ if (shouldCancel) {
+ this.cancelTrade();
+ return;
+ }This also flattens the condition into three clear cases, which is easier to read than the combined boolean expression.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/execution/TradeShipExecution.ts` around lines 77 - 91, The current
shouldCancel boolean in TradeShipExecution lets a captured self-trade ship be
deleted when the destination port is lost; update the condition so capture takes
precedence as in the non-self branch: ensure the self-trade branch checks
!this.wasCaptured before cancelling (i.e., mirror the non-self clause that uses
!this.wasCaptured && (!dstActive || !tradeShipOwner.canTrade(...))). Refactor
the combined boolean into three explicit cases for clarity: 1) non-self trade
cancellation when dst was captured by src owner, 2) non-self cancellation when
NOT this.wasCaptured and (dst inactive or tradeShipOwner.canTrade(dstPortOwner)
is false), and 3) self-trade cancellation only when NOT this.wasCaptured and
(dst inactive or dstPortOwner !== this.srcPort.owner()); call this.cancelTrade()
and return when any case is true.
Contributor
|
there needs to be a way to disable trade with yourself to ensure it never picks myself over someone else |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description:
Ports can now send trade ships to the player's own ports, useful in late game when embargoes cut off all external trade partners. This would allow players to trade with themselves and keep trade routes active.
Config: tradeShipSelfGoldMultiplier() = 0.4
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
zixer._