Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds unit docked/in-combat flags and game trade-route blocking; implements HealAtPortExecution; refactors warship docking/retreat/combat logic to allow firing while retreating and to sync retreat intent; updates default config conquest/growth values; extends game hashing and conquest gold flow; adds tests for retreat and trade-route blocking. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Game as Game
participant Warship as Warship
participant Port as Port
participant Enemy as Enemy
Client->>Warship: detect low health -> request retreat
Warship->>Game: find nearest friendly port
Game->>Port: query port ownership & distance
Port-->>Game: return port tile
Game-->>Warship: assign retreat target tile
Warship->>Warship: setRetreating(true), setPatrol/targetTile(port)
Enemy->>Warship: fire shells (damage)
Warship->>Warship: setInCombat()
Warship->>Game: findAggroTarget(prefer transports)
Game-->>Warship: return hostile target
Warship->>Enemy: fire while moving toward port
Warship->>Port: arrive -> setDocked(true) / heal
sequenceDiagram
participant Attacker as Attacker
participant TradeShip as TradeShip
participant Game as Game
participant RouteStore as RouteStore
Attacker->>TradeShip: capture (owner change)
TradeShip->>Game: notify capture with src/dst ids
Game->>RouteStore: blockTradeRouteUntil(src,dst, now+100)
RouteStore-->>Game: store blocked src->dst until tick
Note over Game,RouteStore: PortExecution.tradingPorts() will exclude blocked dst
Game->>Game: ticks advance...
Note over Game,RouteStore: entry expires after TRADE_ROUTE_BLOCK_DURATION_TICKS
RouteStore-->>Game: route becomes available again
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 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}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/core/game/GameImpl.ts (1)
1262-1277:⚠️ Potential issue | 🟡 MinorPass
goldCapturedinstead ofgoldtogoldAmountparameter.The floating badge displays
+${renderNumber(this.latestGoldAmount)}(a positive gain), but currently receives the fullgoldamount. However, the message text displaysgoldCaptured(the actual amount the conqueror gains). Since the badge represents what the player receives, both should usegoldCaptured.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/game/GameImpl.ts` around lines 1262 - 1277, The floating badge currently receives the full variable gold instead of the actual amount gained; in the displayMessage call inside GameImpl (the call with "events_display.received_gold_from_conquest", MessageType.CONQUERED_PLAYER, conqueror.id(), gold, {...}), change the fourth argument from gold to goldCaptured so the badge and the message meta ({ gold: renderNumber(goldCaptured) }) both reflect the actual captured amount; ensure any use of latestGoldAmount or related badge state is set from goldCaptured where this displayMessage is triggered.
🧹 Nitpick comments (10)
src/core/game/Game.ts (1)
620-623: Document the asymmetric setter semantics.
setDocked(docked: boolean)is a normal toggle, butsetInCombat()only stamps "now" and relies on a timer to decay (3 ticks perUnitImpl.isInCombat). That mismatch is easy to miss when reading the interface. A short doc comment would save the next reader a trip to the impl:📝 Suggested doc
isDocked(): boolean; setDocked(docked: boolean): void; + /** True while the unit was hit or fired within the last few ticks. */ isInCombat(): boolean; + /** Stamps the unit as in combat at the current tick. There is no explicit clear; the flag decays automatically. */ setInCombat(): void;🤖 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 - 623, Add a short doc comment to the Game interface methods to clarify the asymmetric setter semantics: document that setDocked(docked: boolean) directly sets the docked state, while setInCombat() does not accept a boolean but instead stamps the current time (marks "now") and relies on a timed decay (see UnitImpl.isInCombat which decays over ~3 ticks) to clear combat status; include references to isDocked, setDocked, isInCombat, setInCombat and UnitImpl.isInCombat so readers know where to find the implementation behavior.src/core/game/UnitImpl.ts (1)
25-26: Replace the magic numbers with named constants.Two unexplained numbers in this state machine:
_lastCombatTick: number = -100— sentinel meaning "never in combat".mg.ticks() - this._lastCombatTick <= 3— hardcoded combat-decay window.For non-native-English readers (and future-you), a small named constant pair is much easier to follow, and the threshold is the kind of thing playtest balance tweaks will want to touch:
♻️ Suggested cleanup
+const COMBAT_DECAY_TICKS = 3; +const NEVER_IN_COMBAT_TICK = Number.NEGATIVE_INFINITY; + export class UnitImpl implements Unit { @@ - private _docked: boolean = false; - private _lastCombatTick: number = -100; + private _docked: boolean = false; + private _lastCombatTick: number = NEVER_IN_COMBAT_TICK; @@ - isInCombat(): boolean { - return this.mg.ticks() - this._lastCombatTick <= 3; - } + isInCombat(): boolean { + return this.mg.ticks() - this._lastCombatTick <= COMBAT_DECAY_TICKS; + }If gameplay balance is going to keep tuning this, promoting
COMBAT_DECAY_TICKStoConfig(e.g.combatDecayTicks(): Tick) is even better.Also applies to: 367-369
🤖 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 25 - 26, Replace the magic numeric literals in UnitImpl with named constants: introduce a sentinel constant (e.g. NEVER_IN_COMBAT = -100) to initialize _lastCombatTick and a COMBAT_DECAY_TICKS constant (e.g. 3) for the decay window used in the check mg.ticks() - this._lastCombatTick <= 3; update all occurrences (including the logic around lines 367–369) to use these constants, and consider exposing COMBAT_DECAY_TICKS via Config (e.g. Config.combatDecayTicks()) if you expect gameplay tuning to change the value.src/core/execution/PortExecution.ts (1)
125-151: Blocked ports still inflate the close-bonus index.The
continueskips weighting for blocked ports, buti(fromports.entries()) andproximityBonusPortsNb(ports.length)both still count them. So if the two closest ports are blocked, the next port shows up ati = 2and may slip outside the close-bonus window even though it is, in practice, the closest non-blocked port. Likely minor for a playtest, but a tighter version would filter first and then weight:♻️ Suggested tweak
- for (const [i, otherPort] of ports.entries()) { - if ( - this.mg.isTradeRouteBlocked( - this.port.id(), - otherPort.id(), - this.mg.ticks(), - ) - ) { - continue; - } - + const openPorts = ports.filter( + (p) => + !this.mg.isTradeRouteBlocked(this.port.id(), p.id(), this.mg.ticks()), + ); + const proximityBonusCount = this.mg + .config() + .proximityBonusPortsNb(openPorts.length); + for (const [i, otherPort] of openPorts.entries()) { const expanded = new Array(otherPort.level()).fill(otherPort); weightedPorts.push(...expanded); const tooClose = this.mg.manhattanDist(this.port!.tile(), otherPort.tile()) < this.mg.config().tradeShipShortRangeDebuff(); - const closeBonus = - i < this.mg.config().proximityBonusPortsNb(ports.length); + const closeBonus = i < proximityBonusCount;🤖 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 125 - 151, The loop in PortExecution.ts uses the original ports array and index `i` even when some ports are skipped by mg.isTradeRouteBlocked, causing proximityBonusPortsNb(ports.length) to count blocked ports; fix by pre-filtering ports to a new array of non-blocked candidates (call it e.g. filteredPorts) using mg.isTradeRouteBlocked, then iterate over filteredPorts with entries(), use proximityBonusPortsNb(filteredPorts.length) and apply the same weighting/tooClose/friendly checks against each otherPort so the close-bonus index and counts reflect only available trade targets.tests/core/executions/TradeShipExecution.test.ts (1)
161-173: Avoid reaching into private state from tests.
(game as any).tradeRouteBlockedUntil.get(routeKey)couples this test to the exact field name and key format insideGameImpl. If the impl ever switches to nested maps, two-level keys, or a different data structure, this test silently breaks even though behavior is correct. A public-API-only check is more robust:♻️ Public-API alternative
it("does not add a second blacklist event when the ship is recaptured", () => { - const routeKey = `${srcPort.id()}:${dstPort.id()}`; tradeShip.owner = vi.fn(() => pirate); tradeShipExecution.tick(1); - const blockedUntil = (game as any).tradeRouteBlockedUntil.get(routeKey); + // Probe the blocked-until tick by binary-searching boundaries via the public API. + let blockedUntil = game.ticks(); + while ( + game.isTradeRouteBlocked(srcPort.id(), dstPort.id(), blockedUntil) + ) { + blockedUntil++; + } tradeShip.owner = vi.fn(() => origOwner); tradeShipExecution.tick(2); - expect((game as any).tradeRouteBlockedUntil.get(routeKey)).toBe( - blockedUntil, - ); + expect( + game.isTradeRouteBlocked(srcPort.id(), dstPort.id(), blockedUntil - 1), + ).toBe(true); + expect( + game.isTradeRouteBlocked(srcPort.id(), dstPort.id(), blockedUntil), + ).toBe(false);(Or expose a small
tradeRouteBlockedUntil(src, dst): Tick | nullhelper onGameif you'd rather keep the test direct.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/core/executions/TradeShipExecution.test.ts` around lines 161 - 173, The test currently reaches into private state via (game as any).tradeRouteBlockedUntil.get(routeKey); instead, add a small public accessor on the Game API (e.g. tradeRouteBlockedUntil(srcId: string, dstId: string): Tick | null or isTradeRouteBlocked(srcId,dstId): boolean), implement it on GameImpl to read the private map and return the appropriate value, and update the test to call that accessor with srcPort.id() and dstPort.id() after invoking tradeShipExecution.tick(...); keep the rest of the test assertions identical but use Game.tradeRouteBlockedUntil(srcPort.id(), dstPort.id()) (or isTradeRouteBlocked) rather than touching internal fields.src/core/configuration/DefaultConfig.ts (1)
515-524:conquerGoldAmount— small readability tweak.Logic is correct and
BigIntdivision truncates toward zero, which is what you want for gold. As a tiny improvement, an exhaustiveswitchoverPlayerTypedocuments the intent and future-proofs the function so a new player type cannot silently fall into the “half” branch.♻️ Suggested rewrite
- public conquerGoldAmount(captured: Player): Gold { - if ( - captured.type() === PlayerType.Bot || - captured.type() === PlayerType.Nation - ) { - return captured.gold(); - } else { - return captured.gold() / 2n; - } - } + public conquerGoldAmount(captured: Player): Gold { + switch (captured.type()) { + case PlayerType.Bot: + case PlayerType.Nation: + return captured.gold(); + case PlayerType.Human: + return captured.gold() / 2n; + default: + assertNever(captured.type()); + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/configuration/DefaultConfig.ts` around lines 515 - 524, The current conquerGoldAmount function uses an if/else on captured.type() which is correct but non-exhaustive; replace it with an exhaustive switch on captured.type() (using the PlayerType enum) inside conquerGoldAmount(Player captured): Gold so each PlayerType case (PlayerType.Bot, PlayerType.Nation, PlayerType.Human or other known types) explicitly returns the intended value (full captured.gold() for Bot/Nation, half captured.gold()/2n for Human) and add a default case that either throws or handles unknown PlayerType to avoid silently treating new types as "half".tests/Warship.test.ts (1)
556-891: Solid coverage for retreat / combat / heal-at-port flows.The new tests pin down the most subtle parts of the rework: simultaneous retreat + return-fire, aggro pickup mid-retreat, manual-move cancellation + 50-tick suppression window,
isInCombaton hit and on fire, docked-target exclusion, andHealAtPortExecutionhappy/sad paths. Good safety net.One tiny readability note: the title at Line 673 says “5 seconds” which is only true at the default 100 ms turn interval. A short comment near the top of that test (e.g.
// 50 ticks @ ~100ms) would save future readers a trip intoWarshipExecution.shouldStartRepairRetreat.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Warship.test.ts` around lines 556 - 891, Add a short clarifying comment to the test titled "Manual MoveWarshipExecution suppresses auto-retreat for 5 seconds before retreat starts" that explains the "5 seconds" wording maps to ~50 ticks at the default ~100ms turn interval (for example: "// 50 ticks @ ~100ms"); place the comment near the top of that test (just after the config setup or before creating the warship) so readers don't need to inspect WarshipExecution.shouldStartRepairRetreat to understand the timing assumption.tests/PortExecution.test.ts (1)
108-204: Tests reach into private state — consider tiny helpers.The four new tests poke
_ticksandhash()through(game as any), which works today but breaks the moment those names change. Also, each test re-builds the sameplayer + port + executionscaffolding. A smallsetupBlockedTradeRoute()helper plus a(game as any).advanceTicks(n)(or evenGame#advanceTicksForTest) would keep tests focused on the new behavior without losing coverage. Not blocking, just easier to read.🤖 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 - 204, Tests access private game internals (_ticks and hash()) and duplicate scaffolding; extract small test helpers and an internal test-only API to avoid casting to any. Add a helper function (e.g. setupBlockedTradeRoute) used by the tests to create the player/port/PortExecution and block a route via game.blockTradeRouteUntil(port.id(), blockedPort.id(), ...), and add a test-only method on Game (e.g. advanceTicksForTest or Game#advanceTicksForTest) that advances ticks by n (internally increments _ticks) and/or exposes hash() in a stable way for assertions, then update the tests to call those helpers instead of poking (game as any)._ticks and (game as any).hash().src/core/game/GameImpl.ts (1)
522-558: Trade-route block API looks clean; one tiny robustness tweak.
blockTradeRouteUntilaccepts anytick. If a caller passes a tick that is already in the past (tick <= this._ticks), you currently still write the entry and rely on the next prune to clear it. A short-circuit keeps the map smaller and the intent obvious:♻️ Suggested tweak
blockTradeRouteUntil(srcPortId: number, dstPortId: number, tick: Tick): void { this.pruneExpiredTradeRouteBlocks(this._ticks); + if (tick <= this._ticks) { + return; + } const routeKey = this.makeTradeRouteKey(srcPortId, dstPortId); const existing = this.tradeRouteBlockedUntil.get(routeKey) ?? 0; if (tick > existing) { this.tradeRouteBlockedUntil.set(routeKey, tick); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/game/GameImpl.ts` around lines 522 - 558, blockTradeRouteUntil currently may insert past or current ticks into tradeRouteBlockedUntil then rely on prune; update blockTradeRouteUntil (and use makeTradeRouteKey) to early-return when the provided tick is <= this._ticks so you don't write stale entries, i.e. before calling pruneExpiredTradeRouteBlocks/setting the map check if tick <= this._ticks and simply return; keep the existing logic that only updates the map when tick > existing otherwise.src/core/execution/WarshipExecution.ts (2)
183-208: Inconsistent handling of "warship has no water component".
findNearestPort(Line 192) throws whenwarshipComponent === null, but the newnearestAvailablePortTile(Line 504) silently treatsnullas “any port is fine”. So:
- Initial port pick (
startRepairRetreat) — crashes the execution.- Fallback re-pick (
findNearestAvailablePortTile/findBetterPortTile) — silently picks a port that may be unreachable.Pick one policy and apply it in both places. The lenient version is probably what you actually want during fallback (you can't make pathfinding worse), but then
findNearestPortshouldn't throw either.♻️ Suggested unification
- const warshipComponent = this.mg.getWaterComponent(warshipTile); - if (warshipComponent === null) { - throw new Error(`Warship at tile ${warshipTile} has no water component`); - } - - const nearest = findMinimumBy( - ports, - (port) => this.mg.euclideanDistSquared(warshipTile, port.tile()), - (port) => { - const portComponent = this.mg.getWaterComponent(port.tile()); - if (portComponent === null) { - throw new Error(`Port at tile ${port.tile()} has no water component`); - } - return portComponent === warshipComponent; - }, - ); - - return nearest?.tile(); + const warshipComponent = this.mg.getWaterComponent(warshipTile); + const nearest = findMinimumBy( + ports, + (port) => this.mg.euclideanDistSquared(warshipTile, port.tile()), + (port) => + warshipComponent === null || + this.mg.hasWaterComponent(port.tile(), warshipComponent), + ); + return nearest?.tile();Also applies to: 499-561
🤖 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 183 - 208, The code inconsistently throws in findNearestPort when warshipComponent is null while fallback logic (nearestAvailablePortTile / findNearestAvailablePortTile / findBetterPortTile) treats null as “any port is fine”; unify to the lenient policy: change findNearestPort to not throw on a null warshipComponent, allow a null warshipComponent to match any port (i.e., treat portComponent === null or warshipComponent === null as acceptable), and remove the error throw; update the predicate inside findNearestPort to mirror the logic used by nearestAvailablePortTile so both initial pick (startRepairRetreat -> findNearestPort) and fallback repicks behave the same.
429-457:dockedShipsAtPortinfers docked state instead of usingisDocked().The new
Unit.isDocked()flag is the source of truth elsewhere in this file, but here you re-derive "docked" asretreating() && targetTile===undefined && near port. That works becausehandleRepairRetreatclearstargetTileand setssetDocked(true)together, but it’s easy for the two definitions to drift later (e.g. someone clearstargetTilefor a different reason).Combining the explicit flag with the per-port distance check is more honest:
♻️ Suggested rewrite
private dockedShipsAtPort(port: Unit, excludeShip?: Unit): Unit[] { const portTile = port.tile(); const dockingRadius = this.mg.config().warshipDockingRange(); const dockingRadiusSq = dockingRadius * dockingRadius; return this.warship .owner() .units(UnitType.Warship) .filter((ship) => { if (excludeShip && ship === excludeShip) { return false; } - if (!ship.retreating()) { - return false; - } - // Docked ships are retreating ships that are stationary at the port. - if (ship.targetTile() !== undefined) { + if (!ship.isDocked()) { return false; } return ( this.mg.euclideanDistSquared(ship.tile(), portTile) <= dockingRadiusSq ); }); }🤖 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 429 - 457, The docked detection in dockedShipsAtPort currently re-derives docked state via retreating() && targetTile()===undefined; change it to use the canonical Unit.isDocked() flag: in dockedShipsAtPort (and by extension the isPortFullOfHealing call flow) filter warship units by ship.isDocked() (still honoring excludeShip and the euclidean distance check using mg.euclideanDistSquared and dockingRadiusSq) instead of checking retreating()/targetTile(), so the explicit flag is the source of truth while preserving the per-port proximity filter.
🤖 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
only repaints when units are updated, causing sticky/jumpy blinking; fix it by
making the UnitLayer actively invalidate/redraw on a 500ms wall-clock cadence
while any retreating units exist: detect retreating units (use the same
predicate that decides to call drawRetreatCross for a UnitView), start a single
shared timer (setInterval or RAF loop with 500ms toggle) when the first
retreating unit appears and stop it when none remain, and have the timer call
the layer's redraw/invalidate method so drawRetreatCross runs each tick;
alternatively, if you prefer a stable visual, remove the Date.now() blink logic
in drawRetreatCross and always draw the cross.
In `@src/core/configuration/Config.ts`:
- Line 89: Add a short clarification to the PR description noting that
Config.conquerGoldAmount (and its implementation
DefaultConfig.conquerGoldAmount) returns full gold for bots/nations but half
gold for human players on conquest; mention "50% on conquest" applies to human
players only so reviewers won't interpret the bot/nation full-gold behavior as a
bug.
In `@src/core/execution/HealAtPortExecution.ts`:
- Around line 8-58: The init() should verify the portTile is reachable over
water before setting retreat so retreats don't get silently cancelled; update
HealAtPortExecution.init to perform the same water-reachability check used by
WarshipExecution.nearestAvailablePortTile (or call that helper) to ensure the
warship's current tile can reach this.portTile (the same
connectivity/pathfinding used by
WarshipExecution.refreshRetreatPortTile/handleRepairRetreat); if the port is not
reachable, log a clear warning like "HealAtPortExecution: port tile not
reachable by water" and return without changing warship state.
In `@src/core/execution/TradeShipExecution.ts`:
- Line 10: Replace the direct reference to TRADE_ROUTE_BLOCK_DURATION_TICKS in
TradeShipExecution by adding a tradeRouteBlockDuration(): Tick accessor to the
Config interface and using config.tradeRouteBlockDuration() wherever
TRADE_ROUTE_BLOCK_DURATION_TICKS is currently imported/used in
TradeShipExecution (and related uses in the same module); implement
tradeRouteBlockDuration() in DefaultConfig to return the current
TRADE_ROUTE_BLOCK_DURATION_TICKS value, remove the import of
TRADE_ROUTE_BLOCK_DURATION_TICKS from GameImpl in TradeShipExecution, and update
all call sites in that file to read the duration from the injected Config
instance instead.
In `@src/core/execution/WarshipExecution.ts`:
- Around line 276-312: The syncExternalRetreatOrder handler currently sets
retreatingForRepair and retreatPortTile when adopting an external retreat, but
does not clear the manual-move suppression flag; update syncExternalRetreatOrder
(after setting this.retreatingForRepair = true and this.retreatPortTile =
friendlyPort.tile()) to also reset this.lastManualMoveTickRetreatDisabled = 0 so
the patrolTile change from HealAtPortExecution is treated as an order (not a
manual override) and won't block shouldStartRepairRetreat; this ties into
handleManualPatrolOverride and cancelRepairRetreat behavior so the suppression
window is cleared when adopting an external retreat.
---
Outside diff comments:
In `@src/core/game/GameImpl.ts`:
- Around line 1262-1277: The floating badge currently receives the full variable
gold instead of the actual amount gained; in the displayMessage call inside
GameImpl (the call with "events_display.received_gold_from_conquest",
MessageType.CONQUERED_PLAYER, conqueror.id(), gold, {...}), change the fourth
argument from gold to goldCaptured so the badge and the message meta ({ gold:
renderNumber(goldCaptured) }) both reflect the actual captured amount; ensure
any use of latestGoldAmount or related badge state is set from goldCaptured
where this displayMessage is triggered.
---
Nitpick comments:
In `@src/core/configuration/DefaultConfig.ts`:
- Around line 515-524: The current conquerGoldAmount function uses an if/else on
captured.type() which is correct but non-exhaustive; replace it with an
exhaustive switch on captured.type() (using the PlayerType enum) inside
conquerGoldAmount(Player captured): Gold so each PlayerType case
(PlayerType.Bot, PlayerType.Nation, PlayerType.Human or other known types)
explicitly returns the intended value (full captured.gold() for Bot/Nation, half
captured.gold()/2n for Human) and add a default case that either throws or
handles unknown PlayerType to avoid silently treating new types as "half".
In `@src/core/execution/PortExecution.ts`:
- Around line 125-151: The loop in PortExecution.ts uses the original ports
array and index `i` even when some ports are skipped by mg.isTradeRouteBlocked,
causing proximityBonusPortsNb(ports.length) to count blocked ports; fix by
pre-filtering ports to a new array of non-blocked candidates (call it e.g.
filteredPorts) using mg.isTradeRouteBlocked, then iterate over filteredPorts
with entries(), use proximityBonusPortsNb(filteredPorts.length) and apply the
same weighting/tooClose/friendly checks against each otherPort so the
close-bonus index and counts reflect only available trade targets.
In `@src/core/execution/WarshipExecution.ts`:
- Around line 183-208: The code inconsistently throws in findNearestPort when
warshipComponent is null while fallback logic (nearestAvailablePortTile /
findNearestAvailablePortTile / findBetterPortTile) treats null as “any port is
fine”; unify to the lenient policy: change findNearestPort to not throw on a
null warshipComponent, allow a null warshipComponent to match any port (i.e.,
treat portComponent === null or warshipComponent === null as acceptable), and
remove the error throw; update the predicate inside findNearestPort to mirror
the logic used by nearestAvailablePortTile so both initial pick
(startRepairRetreat -> findNearestPort) and fallback repicks behave the same.
- Around line 429-457: The docked detection in dockedShipsAtPort currently
re-derives docked state via retreating() && targetTile()===undefined; change it
to use the canonical Unit.isDocked() flag: in dockedShipsAtPort (and by
extension the isPortFullOfHealing call flow) filter warship units by
ship.isDocked() (still honoring excludeShip and the euclidean distance check
using mg.euclideanDistSquared and dockingRadiusSq) instead of checking
retreating()/targetTile(), so the explicit flag is the source of truth while
preserving the per-port proximity filter.
In `@src/core/game/Game.ts`:
- Around line 620-623: Add a short doc comment to the Game interface methods to
clarify the asymmetric setter semantics: document that setDocked(docked:
boolean) directly sets the docked state, while setInCombat() does not accept a
boolean but instead stamps the current time (marks "now") and relies on a timed
decay (see UnitImpl.isInCombat which decays over ~3 ticks) to clear combat
status; include references to isDocked, setDocked, isInCombat, setInCombat and
UnitImpl.isInCombat so readers know where to find the implementation behavior.
In `@src/core/game/GameImpl.ts`:
- Around line 522-558: blockTradeRouteUntil currently may insert past or current
ticks into tradeRouteBlockedUntil then rely on prune; update
blockTradeRouteUntil (and use makeTradeRouteKey) to early-return when the
provided tick is <= this._ticks so you don't write stale entries, i.e. before
calling pruneExpiredTradeRouteBlocks/setting the map check if tick <=
this._ticks and simply return; keep the existing logic that only updates the map
when tick > existing otherwise.
In `@src/core/game/UnitImpl.ts`:
- Around line 25-26: Replace the magic numeric literals in UnitImpl with named
constants: introduce a sentinel constant (e.g. NEVER_IN_COMBAT = -100) to
initialize _lastCombatTick and a COMBAT_DECAY_TICKS constant (e.g. 3) for the
decay window used in the check mg.ticks() - this._lastCombatTick <= 3; update
all occurrences (including the logic around lines 367–369) to use these
constants, and consider exposing COMBAT_DECAY_TICKS via Config (e.g.
Config.combatDecayTicks()) if you expect gameplay tuning to change the value.
In `@tests/core/executions/TradeShipExecution.test.ts`:
- Around line 161-173: The test currently reaches into private state via (game
as any).tradeRouteBlockedUntil.get(routeKey); instead, add a small public
accessor on the Game API (e.g. tradeRouteBlockedUntil(srcId: string, dstId:
string): Tick | null or isTradeRouteBlocked(srcId,dstId): boolean), implement it
on GameImpl to read the private map and return the appropriate value, and update
the test to call that accessor with srcPort.id() and dstPort.id() after invoking
tradeShipExecution.tick(...); keep the rest of the test assertions identical but
use Game.tradeRouteBlockedUntil(srcPort.id(), dstPort.id()) (or
isTradeRouteBlocked) rather than touching internal fields.
In `@tests/PortExecution.test.ts`:
- Around line 108-204: Tests access private game internals (_ticks and hash())
and duplicate scaffolding; extract small test helpers and an internal test-only
API to avoid casting to any. Add a helper function (e.g. setupBlockedTradeRoute)
used by the tests to create the player/port/PortExecution and block a route via
game.blockTradeRouteUntil(port.id(), blockedPort.id(), ...), and add a test-only
method on Game (e.g. advanceTicksForTest or Game#advanceTicksForTest) that
advances ticks by n (internally increments _ticks) and/or exposes hash() in a
stable way for assertions, then update the tests to call those helpers instead
of poking (game as any)._ticks and (game as any).hash().
In `@tests/Warship.test.ts`:
- Around line 556-891: Add a short clarifying comment to the test titled "Manual
MoveWarshipExecution suppresses auto-retreat for 5 seconds before retreat
starts" that explains the "5 seconds" wording maps to ~50 ticks at the default
~100ms turn interval (for example: "// 50 ticks @ ~100ms"); place the comment
near the top of that test (just after the config setup or before creating the
warship) so readers don't need to inspect
WarshipExecution.shouldStartRepairRetreat to understand the timing assumption.
🪄 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: faffafb7-d3c3-4e29-b720-fa730f3b8f05
📒 Files selected for processing (15)
src/client/graphics/layers/UnitLayer.tssrc/core/configuration/Config.tssrc/core/configuration/DefaultConfig.tssrc/core/execution/HealAtPortExecution.tssrc/core/execution/PortExecution.tssrc/core/execution/TradeShipExecution.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/PortExecution.test.tstests/Warship.test.tstests/core/executions/TradeShipExecution.test.ts
| private drawRetreatCross(unit: UnitView) { | ||
| // Blink: 500ms on, 500ms off | ||
| if (Math.floor(Date.now() / 500) % 2 === 0) return; | ||
| const x = this.game.x(unit.tile()); | ||
| const y = this.game.y(unit.tile()); | ||
| const ctx = this.context; | ||
| ctx.save(); | ||
| const cx = x + 0.5; | ||
| const cy = y + 0.5; | ||
| ctx.lineCap = "square"; | ||
| ctx.strokeStyle = "rgb(0,0,0)"; | ||
| ctx.lineWidth = 1; | ||
| ctx.beginPath(); | ||
| ctx.moveTo(cx, cy - 1.5); | ||
| ctx.lineTo(cx, cy + 1.5); | ||
| ctx.moveTo(cx - 1.5, cy); | ||
| ctx.lineTo(cx + 1.5, cy); | ||
| ctx.stroke(); | ||
| ctx.restore(); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm whether retreating warships are reliably part of the per-tick update set.
rg -nP -C3 '\bmotionPlannedUnitIds\b|\bupdatesSinceLastTick\b'
rg -nP -C3 'retreating\(\)' --type=tsRepository: openfrontio/OpenFrontIO
Length of output: 24924
🏁 Script executed:
#!/bin/bash
# Search for how retreat is initiated and managed in WarshipExecution
rg -A 10 "orderRetreat\|startRetreat\|setRetreating" src/core/execution/WarshipExecution.ts | head -60
# Check the main retreat loop logic
rg -B 3 -A 15 "if.*retreat" src/core/execution/WarshipExecution.ts | head -80
# Find where motion plans are set for retreating warships
rg -B 2 -A 8 "retreatingForRepair" src/core/execution/WarshipExecution.ts | head -50Repository: openfrontio/OpenFrontIO
Length of output: 3673
🏁 Script executed:
#!/bin/bash
# Check if warship with a target tile gets a motion plan
rg -B 3 -A 10 "setTargetTile\|motionPlan" src/core/execution/WarshipExecution.ts | head -80
# Check the main tick() method to see if motion planning happens for retreating warships
rg -B 2 -A 20 "^ tick\(\)" src/core/execution/WarshipExecution.ts | head -100Repository: openfrontio/OpenFrontIO
Length of output: 49
🏁 Script executed:
#!/bin/bash
# Get the full file structure and look for tick or planning logic
wc -l src/core/execution/WarshipExecution.ts
# Check if there's a tick method and motion planning
rg "tick|motionPlan|moveUnit" src/core/execution/WarshipExecution.ts | head -20
# Look at how motion is handled for warships
rg -B 5 -A 10 "refreshRetreatPortTile\|moveTowardTarget" src/core/execution/WarshipExecution.ts | head -60Repository: openfrontio/OpenFrontIO
Length of output: 549
🏁 Script executed:
#!/bin/bash
# Read the tick() method from WarshipExecution to see the flow
sed -n '100,200p' src/core/execution/WarshipExecution.ts
# Also check the handleRepairRetreat method more carefully
sed -n '550,650p' src/core/execution/WarshipExecution.tsRepository: openfrontio/OpenFrontIO
Length of output: 6222
🏁 Script executed:
#!/bin/bash
# Get the full tick() method
sed -n '68,125p' src/core/execution/WarshipExecution.ts
# Check if there's a UnitExecution parent or trait that handles motion planning
rg "class WarshipExecution" src/core/execution/WarshipExecution.ts -A 5
# Search for how target tiles result in motion plans
rg -B 2 -A 8 "targetTile\|moveToward" src/core/execution/ -t ts | head -100Repository: openfrontio/OpenFrontIO
Length of output: 1849
🏁 Script executed:
#!/bin/bash
# Look at the patrol() method which handles movement
rg -B 2 -A 15 "private patrol\(\)" src/core/execution/WarshipExecution.ts
# Look at refreshRetreatPortTile()
rg -B 2 -A 15 "refreshRetreatPortTile" src/core/execution/WarshipExecution.ts
# Check if setTargetTile causes motion planning
rg -B 3 -A 8 "setTargetTile" src/core/game/GameView.ts | head -50Repository: openfrontio/OpenFrontIO
Length of output: 1506
🏁 Script executed:
#!/bin/bash
# Check what warship.move() does and if it creates motion plans
rg -B 3 -A 10 "\.move\(" src/core/game/Game.ts | head -50
# Also check UnitImpl.move
rg -B 3 -A 10 "move\(" src/core/game/UnitImpl.ts | head -50
# Check the motionPlannedUnitIds logic in GameView
rg -B 5 -A 15 "rebuildMotionPlannedUnitIdsCacheIfDirty\|motionPlannedUnitIdsCache" src/core/game/GameView.tsRepository: openfrontio/OpenFrontIO
Length of output: 374
🏁 Script executed:
#!/bin/bash
# Check what onUnitMoved does
rg -B 3 -A 20 "onUnitMoved" src/core/game/GameView.ts src/core/game/Game.ts | head -80
# Also check how motionPlanned is determined
rg -B 3 -A 8 "motionPlan\(\)" src/core/game/GameView.ts | head -60Repository: openfrontio/OpenFrontIO
Length of output: 49
🏁 Script executed:
#!/bin/bash
# Search for motion plan in GameView more broadly
rg "motionPlan" src/core/game/GameView.ts | head -20
# Check how a unit gets into motionPlannedUnitIds
rg -B 5 -A 5 "motionPlannedUnitIdsCache" src/core/game/GameView.ts | head -100
# Look at the MotionPlan type/interface
rg "MotionPlan\|class.*Plan\|interface.*Plan" src/core/ -t ts | head -30Repository: openfrontio/OpenFrontIO
Length of output: 1225
🏁 Script executed:
#!/bin/bash
# Find where unitMotionPlans is modified
rg "unitMotionPlans\b" src/core/game/GameView.ts | head -40
# Look for setMotionPlan or addMotionPlan methods
rg "setMotionPlan|addMotionPlan|removeMotionPlan" src/core/game/ -t ts | head -40
# Check what MotionPlan contains
rg "interface MotionPlan|type MotionPlan|class MotionPlan" src/core/ -t ts -A 10 | head -60Repository: openfrontio/OpenFrontIO
Length of output: 1206
🏁 Script executed:
#!/bin/bash
# Check how motion plans are created from pathfinder results
rg -B 5 -A 10 "PathStatus\|pathfinder.next" src/core/execution/WarshipExecution.ts
# Check if patrol() or movement creates/updates motion plans
rg -B 3 -A 15 "case PathStatus" src/core/execution/WarshipExecution.ts
# Search for where motion plans are set
rg "unitMotionPlans\.set\|unitMotionPlans\.delete" src/core/game/GameView.ts -B 2 -A 2 | head -80Repository: openfrontio/OpenFrontIO
Length of output: 2211
🏁 Script executed:
#!/bin/bash
# Check where unitMotionPlans is set from server updates
rg -B 10 -A 5 "unitMotionPlans\.set" src/core/game/GameView.ts
# Look for GameUpdates and how units are marked as updated
rg "GameUpdateType.Unit\|updatedUnitIds" src/core/game/ -t ts -B 2 -A 2 | head -60Repository: openfrontio/OpenFrontIO
Length of output: 582
🏁 Script executed:
#!/bin/bash
# Search for where motion plans are created or scheduled
rg "MotionPlan|planId" src/core/execution/ -t ts | head -40
# Check if units with active movement get motion plans
rg -B 5 -A 10 "moveUnit\|scheduleMovement\|createMotionPlan" src/core/ -t ts | head -80
# Let me check if warships get motion plans when they move
rg "Warship.*motion\|motion.*Warship" src/core/ -t ts -i | head -20Repository: openfrontio/OpenFrontIO
Length of output: 1031
🏁 Script executed:
#!/bin/bash
# Check if WarshipExecution records motion plans
rg "recordMotionPlan" src/core/execution/WarshipExecution.ts
# Check all unit types to see which ones record motion plans
rg "recordMotionPlan" src/core/execution/ -t ts
# This is key: if warships DON'T record motion plans, they won't be in motionPlannedUnitIds
# Check what warships DO have for movement
rg -B 5 -A 5 "warship.*move\|move.*warship" src/core/execution/WarshipExecution.ts -iRepository: openfrontio/OpenFrontIO
Length of output: 383
🏁 Script executed:
#!/bin/bash
# Confirm: WarshipExecution does NOT record motion plans
# Let's verify this is the complete picture by checking if there are any other ways warships get into redraw sets
# Check what GameUpdateType.Unit contains for warships
rg "GameUpdateType.Unit" src/core/game/ -t ts -B 3 -A 3 | head -60
# Check if there's any special handling for retreating warships in updates
rg "retreating\|Warship" src/core/game/PlayerImpl.ts -B 2 -A 2 | head -80Repository: openfrontio/OpenFrontIO
Length of output: 1869
Retreat-cross blink may look sticky because redraws depend on unit updates, not wall-clock time.
drawRetreatCross uses Date.now() for the blink cadence, but the cross is only painted when the unit is redrawn in tick(). Warships do not record motion plans (unlike transport ships), so they're not in motionPlannedUnitIds. Instead, they are redrawn only when they appear in updatedUnitIds from GameUpdateType.Unit updates. If a retreating warship's state does not change every tick—or if the unit-update batch is sparse—the redraw will be irregular. The cross will then blink only during those redraws, making the on/off cadence appear stuck or jumpy rather than a smooth 500 ms blink.
For a clean 500 ms blink, the layer must redraw retreating warships on every wall-clock interval. Otherwise, consider dropping the blink effect and drawing the cross steadily—simpler and visually stable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/client/graphics/layers/UnitLayer.ts` around lines 477 - 496,
drawRetreatCross currently blinks using Date.now() but only repaints when units
are updated, causing sticky/jumpy blinking; fix it by making the UnitLayer
actively invalidate/redraw on a 500ms wall-clock cadence while any retreating
units exist: detect retreating units (use the same predicate that decides to
call drawRetreatCross for a UnitView), start a single shared timer (setInterval
or RAF loop with 500ms toggle) when the first retreating unit appears and stop
it when none remain, and have the timer call the layer's redraw/invalidate
method so drawRetreatCross runs each tick; alternatively, if you prefer a stable
visual, remove the Date.now() blink logic in drawRetreatCross and always draw
the cross.
| startManpower(playerInfo: PlayerInfo): number; | ||
| troopIncreaseRate(player: Player | PlayerView): number; | ||
| goldAdditionRate(player: Player | PlayerView): Gold; | ||
| conquerGoldAmount(captured: Player): Gold; |
There was a problem hiding this comment.
LGTM — clean interface addition.
The new conquerGoldAmount(captured: Player): Gold hook keeps balance tweaks centralized in Config. Just a heads-up: DefaultConfig.conquerGoldAmount returns full gold for bots/nations and half for humans, while the PR title mentions "50% on conquest" — worth a quick line in the PR description so future readers don't think the bot/nation case is a bug.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/configuration/Config.ts` at line 89, Add a short clarification to
the PR description noting that Config.conquerGoldAmount (and its implementation
DefaultConfig.conquerGoldAmount) returns full gold for bots/nations but half
gold for human players on conquest; mention "50% on conquest" applies to human
players only so reviewers won't interpret the bot/nation full-gold behavior as a
bug.
| export class HealAtPortExecution implements Execution { | ||
| constructor( | ||
| private readonly owner: Player, | ||
| private readonly warshipId: number, | ||
| private readonly portTile: TileRef, | ||
| ) {} | ||
|
|
||
| init(mg: Game, ticks: number): void { | ||
| if (!mg.isValidRef(this.portTile)) { | ||
| console.warn(`HealAtPortExecution: port tile ${this.portTile} not valid`); | ||
| return; | ||
| } | ||
|
|
||
| const warship = this.owner | ||
| .units(UnitType.Warship) | ||
| .find((u) => u.id() === this.warshipId); | ||
|
|
||
| if (!warship) { | ||
| console.warn("HealAtPortExecution: warship not found"); | ||
| return; | ||
| } | ||
|
|
||
| if (!warship.isActive()) { | ||
| console.warn("HealAtPortExecution: warship is not active"); | ||
| return; | ||
| } | ||
|
|
||
| const isFriendlyPort = this.owner | ||
| .units(UnitType.Port) | ||
| .some((port) => port.tile() === this.portTile); | ||
| if (!isFriendlyPort) { | ||
| console.warn("HealAtPortExecution: target port is not friendly"); | ||
| return; | ||
| } | ||
|
|
||
| // Set warship to go to the port | ||
| warship.setPatrolTile(this.portTile); | ||
| warship.setTargetTile(this.portTile); | ||
| warship.setRetreating(true); // Enable healing behavior | ||
| } | ||
|
|
||
| tick(ticks: number): void {} | ||
|
|
||
| isActive(): boolean { | ||
| return false; | ||
| } | ||
|
|
||
| activeDuringSpawnPhase(): boolean { | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm TileRef is a primitive (so `===` is meaningful) and that callers always pass a port's own tile.
rg -nP -C2 'export\s+type\s+TileRef\b' --type=ts
rg -nP -C3 'new\s+HealAtPortExecution\(' --type=tsRepository: openfrontio/OpenFrontIO
Length of output: 1064
🏁 Script executed:
# Check for handleRepairRetreat in WarshipExecution
rg -n 'handleRepairRetreat' --type=ts -B2 -A5
# Check for water component methods
rg -n 'getWaterComponent|hasWaterComponent' --type=ts -B1 -A1
# Check for tile comparison helpers
rg -n 'tile.*equal|compareTile|sameTile' --type=tsRepository: openfrontio/OpenFrontIO
Length of output: 9035
🏁 Script executed:
# Get the full handleRepairRetreat method to see the flow
sed -n '314,340p' src/core/execution/WarshipExecution.ts
# Check what happens when port is unreachable in findNearestAvailablePortTile
sed -n '495,530p' src/core/execution/WarshipExecution.tsRepository: openfrontio/OpenFrontIO
Length of output: 1864
🏁 Script executed:
# Look at the test cases for HealAtPortExecution
sed -n '700,800p' tests/Warship.test.ts
# Check refreshRetreatPortTile to see full flow
rg -n 'refreshRetreatPortTile' src/core/execution/WarshipExecution.ts -A10 -B2Repository: openfrontio/OpenFrontIO
Length of output: 4026
🏁 Script executed:
# Check if there are tile comparison helpers or utilities
rg -n 'tileEqual|sameTile|compareTile|tileRefEqual' --type=ts
# Verify the full context of HealAtPortExecution test
sed -n '718,760p' tests/Warship.test.ts
# Check Game interface to confirm method signatures
rg -n 'getWaterComponent|hasWaterComponent' src/core/game/Game.tsRepository: openfrontio/OpenFrontIO
Length of output: 1690
Add water component reachability check to avoid silent retreat cancellation.
The execution correctly sets the warship state to retreat toward the port, but if the port is in an isolated water body, WarshipExecution.handleRepairRetreat() will silently cancel the retreat when refreshRetreatPortTile() finds no reachable port. A reachability check in HealAtPortExecution.init() catches this upfront with a clear warning, matching the pattern already used in WarshipExecution.nearestAvailablePortTile().
♻️ Suggested reachability check
+ const friendlyPort = this.owner
+ .units(UnitType.Port)
+ .find((port) => port.tile() === this.portTile);
+ if (!friendlyPort) {
+ console.warn("HealAtPortExecution: target port is not friendly");
+ return;
+ }
+
+ const warshipComponent = mg.getWaterComponent(warship.tile());
+ if (
+ warshipComponent !== null &&
+ !mg.hasWaterComponent(this.portTile, warshipComponent)
+ ) {
+ console.warn("HealAtPortExecution: port not reachable by water");
+ return;
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/execution/HealAtPortExecution.ts` around lines 8 - 58, The init()
should verify the portTile is reachable over water before setting retreat so
retreats don't get silently cancelled; update HealAtPortExecution.init to
perform the same water-reachability check used by
WarshipExecution.nearestAvailablePortTile (or call that helper) to ensure the
warship's current tile can reach this.portTile (the same
connectivity/pathfinding used by
WarshipExecution.refreshRetreatPortTile/handleRepairRetreat); if the port is not
reachable, log a clear warning like "HealAtPortExecution: port tile not
reachable by water" and return without changing warship state.
| Unit, | ||
| UnitType, | ||
| } from "../game/Game"; | ||
| import { TRADE_ROUTE_BLOCK_DURATION_TICKS } from "../game/GameImpl"; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Move the route-block duration into Config.
Pulling TRADE_ROUTE_BLOCK_DURATION_TICKS from GameImpl ties an execution module to a concrete impl module just to read a balance number. That's the same kind of tunable as tradeShipSpawnRate, safeFromPiratesCooldownMax, etc., which all live behind Config(). Putting it on Config keeps the layering clean (execution → Config, not execution → GameImpl) and lets future game modes override it.
♻️ Sketch
-import { TRADE_ROUTE_BLOCK_DURATION_TICKS } from "../game/GameImpl";
@@
- this.mg.blockTradeRouteUntil(
- this.srcPortId,
- this.dstPortId,
- this.mg.ticks() + TRADE_ROUTE_BLOCK_DURATION_TICKS,
- );
+ this.mg.blockTradeRouteUntil(
+ this.srcPortId,
+ this.dstPortId,
+ this.mg.ticks() + this.mg.config().tradeRouteBlockDuration(),
+ );Then add tradeRouteBlockDuration(): Tick to Config and implement it in DefaultConfig.
Also applies to: 78-82
🤖 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 10, Replace the direct
reference to TRADE_ROUTE_BLOCK_DURATION_TICKS in TradeShipExecution by adding a
tradeRouteBlockDuration(): Tick accessor to the Config interface and using
config.tradeRouteBlockDuration() wherever TRADE_ROUTE_BLOCK_DURATION_TICKS is
currently imported/used in TradeShipExecution (and related uses in the same
module); implement tradeRouteBlockDuration() in DefaultConfig to return the
current TRADE_ROUTE_BLOCK_DURATION_TICKS value, remove the import of
TRADE_ROUTE_BLOCK_DURATION_TICKS from GameImpl in TradeShipExecution, and update
all call sites in that file to read the duration from the injected Config
instance instead.
| private handleManualPatrolOverride(): void { | ||
| const patrolTile = this.warship.patrolTile(); | ||
| if ( | ||
| this.lastObservedPatrolTile !== undefined && | ||
| patrolTile !== this.lastObservedPatrolTile | ||
| ) { | ||
| this.lastManualMoveTickRetreatDisabled = this.mg.ticks(); | ||
| if (this.retreatingForRepair) { | ||
| this.cancelRepairRetreat(false); | ||
| } | ||
| } | ||
| this.lastObservedPatrolTile = patrolTile; | ||
| } | ||
|
|
||
| private syncExternalRetreatOrder(): void { | ||
| if (this.retreatingForRepair || !this.warship.retreating()) { | ||
| return; | ||
| } | ||
|
|
||
| const orderedPortTile = | ||
| this.warship.targetTile() ?? this.warship.patrolTile(); | ||
| if (orderedPortTile === undefined) { | ||
| return; | ||
| } | ||
|
|
||
| const friendlyPort = this.warship | ||
| .owner() | ||
| .units(UnitType.Port) | ||
| .find((port) => port.tile() === orderedPortTile); | ||
| if (!friendlyPort) { | ||
| return; | ||
| } | ||
|
|
||
| this.retreatingForRepair = true; | ||
| this.retreatPortTile = friendlyPort.tile(); | ||
| this.warship.setTargetUnit(undefined); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm HealAtPortExecution is the only external setter that flips retreating + patrolTile together.
rg -nP -C3 'setRetreating\(true\)' --type=ts
rg -nP -C3 'setPatrolTile\(' --type=tsRepository: openfrontio/OpenFrontIO
Length of output: 3159
🏁 Script executed:
#!/bin/bash
# Extract the key methods to understand the flow:
# 1. handleManualPatrolOverride
# 2. syncExternalRetreatOrder
# 3. shouldStartRepairRetreat
# Look for lastManualMoveTickRetreatDisabled usage
echo "=== handleManualPatrolOverride and syncExternalRetreatOrder ==="
sed -n '276,312p' src/core/execution/WarshipExecution.ts
echo ""
echo "=== shouldStartRepairRetreat ==="
sed -n '161,181p' src/core/execution/WarshipExecution.ts
echo ""
echo "=== All uses of lastManualMoveTickRetreatDisabled ==="
rg -n 'lastManualMoveTickRetreatDisabled' src/core/execution/WarshipExecution.ts
echo ""
echo "=== cancelRepairRetreat to see what it resets ==="
rg -nP -A10 'cancelRepairRetreat' src/core/execution/WarshipExecution.ts | head -30
echo ""
echo "=== All uses of retreatingForRepair in syncExternalRetreatOrder context ==="
sed -n '290,312p' src/core/execution/WarshipExecution.tsRepository: openfrontio/OpenFrontIO
Length of output: 3516
syncExternalRetreatOrder should reset the manual-move suppression flag when adopting a retreat order.
When HealAtPortExecution sets patrolTile and retreating = true, the next tick of WarshipExecution runs handleManualPatrolOverride → syncExternalRetreatOrder:
handleManualPatrolOverridedetects thepatrolTilechange and setslastManualMoveTickRetreatDisabled = ticks(blocking auto-retreat for 50 ticks).syncExternalRetreatOrderthen recognizes this is a retreat order and setsretreatingForRepair = true.
The bug: if the port is destroyed or the warship heals before docking, cancelRepairRetreat() clears retreatingForRepair but leaves lastManualMoveTickRetreatDisabled active. If the warship takes damage again, shouldStartRepairRetreat refuses to auto-retreat because the 50-tick window is still running. The user explicitly asked to heal, but after the port dies, the warship sits idle instead of retreating.
Fix: In syncExternalRetreatOrder, after setting retreatingForRepair = true (line 310), reset this.lastManualMoveTickRetreatDisabled = 0. This tells the system the patrolTile change was a retreat order, not a manual move, so the suppression window should not apply.
🤖 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 276 - 312, The
syncExternalRetreatOrder handler currently sets retreatingForRepair and
retreatPortTile when adopting an external retreat, but does not clear the
manual-move suppression flag; update syncExternalRetreatOrder (after setting
this.retreatingForRepair = true and this.retreatPortTile = friendlyPort.tile())
to also reset this.lastManualMoveTickRetreatDisabled = 0 so the patrolTile
change from HealAtPortExecution is treated as an order (not a manual override)
and won't block shouldStartRepairRetreat; this ties into
handleManualPatrolOverride and cancelRepairRetreat behavior so the suppression
window is cleared when adopting an external retreat.
github-project-automation
Bot
moved this from Triage
to Development
in OpenFront Release Management
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)
src/core/game/GameImpl.ts (1)
1247-1283:⚠️ Potential issue | 🟠 Major
displayMessagereports wronggoldAmountfor gold captured animations.The message sent to
conqueror.id()passesgoldAmount: gold(full amount), but the conqueror only receivedgoldCaptured(half amount from the 50% capture mechanic). EventsDisplay uses thisgoldAmountto trigger animations and update the gold UI. The text template shows the correct amount viarenderNumber(goldCaptured), but the animation uses the uncorrected parameter. This causes any gold animation, counter, or stats display to show double the actual captured amount.Fix: pass
goldCapturedinstead ofgoldas thegoldAmountparameter.The gold evaporation (other 50% disappears) is intentional behavior per the PR description and correctly implemented in the code.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/game/GameImpl.ts` around lines 1247 - 1283, The displayMessage call in GameImpl is passing the full gold variable to the conqueror's notification (MessageType.CONQUERED_PLAYER) which drives the gold animation/UI, but the actual awarded amount is goldCaptured; update the displayMessage invocation that currently supplies gold (for conqueror.id()) to provide goldCaptured instead so the animation and UI reflect the true captured amount (ensure the surrounding block that uses renderNumber(goldCaptured), conqueror.addGold(goldCaptured), and the ConquestEvent payload remain unchanged).
♻️ Duplicate comments (1)
src/core/execution/WarshipExecution.ts (1)
290-312:⚠️ Potential issue | 🟡 Minor
syncExternalRetreatOrderstill leaves the manual-move suppression flag set.When
HealAtPortExecutionflipspatrolTileandretreating = truetogether, the nextWarshipExecution.tickrunshandleManualPatrolOverridefirst and stampslastManualMoveTickRetreatDisabled = mg.ticks(). Then this method recognizes the patrol change as a retreat order and setsretreatingForRepair = true. So far so good.The trouble shows up later: if the home port is destroyed mid-retreat, or the warship heals and leaves on its own,
cancelRepairRetreat()resetsretreatingForRepairbut leaveslastManualMoveTickRetreatDisabledintact. For the next ~50 ticks,shouldStartRepairRetreatwill refuse to auto-retreat even after heavy damage, because it still thinks a real human just issued a manual order.A single line fix here would keep the two state machines from getting out of sync:
🛠️ Suggested fix
this.retreatingForRepair = true; this.retreatPortTile = friendlyPort.tile(); + this.lastManualMoveTickRetreatDisabled = 0; this.warship.setTargetUnit(undefined); }🤖 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 290 - 312, When syncExternalRetreatOrder detects an external retreat (in WarshipExecution.syncExternalRetreatOrder) and sets retreatingForRepair = true / retreatPortTile, also clear the manual-move suppression marker (reset lastManualMoveTickRetreatDisabled) so the manual-order suppression and the retreat state stay in sync; likewise ensure cancelRepairRetreat clears lastManualMoveTickRetreatDisabled as well so shouldStartRepairRetreat and handleManualPatrolOverride don’t remain out-of-sync after the retreat is cancelled or the port is gone.
🧹 Nitpick comments (7)
src/core/execution/TribeExecution.ts (1)
86-92: Confirm bulk deletion is intended, and consider hoisting the predicate.The previous version deleted at most one structure per tick. The new loop schedules a
DeleteUnitExecutionfor every eligible structure in the same tick. Since nothing inside the loop mutatestribesynchronously (eachDeleteUnitExecutionruns later),this.tribe.canDeleteUnit()will return the same value for all iterations — so this tick will either queue deletions for all eligible structures or none. IfcanDeleteUnit()is a rate limiter meant to pace deletions over time, this change effectively bypasses it.If the intent (per the "make bots weaker" objective) is indeed to dump all structures at once, the predicate can be checked once outside the loop for clarity and a tiny perf win.
♻️ Optional cleanup: check the gate once
private deleteAllStructures() { + if (!this.tribe.canDeleteUnit()) return; for (const unit of this.tribe.units()) { - if (Structures.has(unit.type()) && this.tribe.canDeleteUnit()) { + if (Structures.has(unit.type())) { this.mg.addExecution(new DeleteUnitExecution(this.tribe, unit.id())); } } }Could you confirm whether
canDeleteUnit()is supposed to gate a single deletion per tick, or just act as a global on/off switch? If it's the former, the loop probably needs abreakafter the first scheduled deletion (restoring the old cadence) or some other pacing mechanism.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/execution/TribeExecution.ts` around lines 86 - 92, The current deleteAllStructures() iterates all units and calls this.tribe.canDeleteUnit() each time, which either wrongly allows bulk deletion or redundantly rechecks the same rate-limit; decide the intended behavior and update accordingly: if canDeleteUnit() is meant to permit only one deletion per tick, evaluate this.tribe.canDeleteUnit() (or call it and store the boolean) and after scheduling the first new DeleteUnitExecution(this.tribe, unit.id()) via this.mg.addExecution(...) break out of the loop to restore single-deletion cadence; if bulk deletion is intended, hoist the predicate by testing this.tribe.canDeleteUnit() once before the for loop and only enter the loop to add DeleteUnitExecution for every matching unit, improving clarity and a small perf win (references: deleteAllStructures, this.tribe.canDeleteUnit, this.tribe.units, DeleteUnitExecution, this.mg.addExecution).src/core/game/UnitImpl.ts (1)
26-26: Optional: name the combat window and skip the no-op update.Two small polish items, both optional:
- The
-100initial value and the<= 3threshold are tunable balance knobs, but right now they live as magic numbers in two places. A shared constant (or aConfigaccessor like the rest of the gameplay timings) makes intent obvious to readers and easier to tweak in playtests.setInCombat()always pushes atoUpdate()even when the unit is already in combat. During sustained fire this adds redundant updates per shot. A small early-out keeps the wire chatter lower.♻️ Sketch
- private _lastCombatTick: number = -100; + private static readonly COMBAT_WINDOW_TICKS = 3; + private _lastCombatTick: number = Number.NEGATIVE_INFINITY; @@ isInCombat(): boolean { - return this.mg.ticks() - this._lastCombatTick <= 3; + return this.mg.ticks() - this._lastCombatTick <= UnitImpl.COMBAT_WINDOW_TICKS; } setInCombat(): void { + const wasInCombat = this.isInCombat(); this._lastCombatTick = this.mg.ticks(); - this.mg.addUpdate(this.toUpdate()); + if (!wasInCombat) { + this.mg.addUpdate(this.toUpdate()); + } }Also applies to: 367-374
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/game/UnitImpl.ts` at line 26, Replace the magic numbers used for the combat timing by introducing a shared constant (e.g., COMBAT_WINDOW_TICKS or read from your existing Config accessor) and use it to initialize the private field _lastCombatTick and in any comparisons (instead of -100 and <= 3); additionally, modify setInCombat (the method that currently calls toUpdate()) to early-return when the unit is already considered in combat (i.e., if now - _lastCombatTick < COMBAT_WINDOW_TICKS) so you only call toUpdate() when the in-combat state actually changes, reducing redundant updates.src/core/execution/HealAtPortExecution.ts (1)
8-58: Optional: a fire-once side effect doesn't need theExecutionshape.
tickis empty,isActiveis permanentlyfalse, and all the work runs ininit. The class only exists so it can be queued throughaddExecution. A plain function (or static method) does the same thing more directly and removes the "execution that is never active" oddity:export function healAtPort( mg: Game, owner: Player, warshipId: number, portTile: TileRef, ): void { /* validation + setRetreating */ }Callers replace
addExecution(new HealAtPortExecution(...))withhealAtPort(mg, ...). Composition over class hierarchy, fewer moving parts, and the lifecycle question goes away.(The water-reachability check from the prior review is still worth folding into whichever shape this lands in.)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/execution/HealAtPortExecution.ts` around lines 8 - 58, The HealAtPortExecution class implements Execution but does all work in init, leaving tick empty and isActive always false; replace this fire-once execution with a plain function to simplify semantics: create a new export function healAtPort(mg: Game, owner: Player, warshipId: number, portTile: TileRef) that copies the validation and side-effect logic currently in HealAtPortExecution.init (including water-reachability check if present elsewhere), and update callers to call healAtPort(...) instead of addExecution(new HealAtPortExecution(...)); remove the HealAtPortExecution class (init/tick/isActive/activeDuringSpawnPhase) once callers are migrated.tests/PortExecution.test.ts (1)
156-156: Optional: pull the tick-advance hack into a small helper.
(game as any)._ticks += Nrepeats in three places and ties the tests to a private field. A tiny helper keeps tests readable and gives one spot to change if the field ever moves:const advanceTicks = (g: Game, n: number) => { (g as unknown as { _ticks: number })._ticks += n; };Also: it would be nice to assert the converse direction in the "blocked is omitted" test — i.e.
game.blockTradeRouteUntil(port.id(), blockedPort.id(), ...)does not blockblockedPort -> port— to lock in the directional contract documented inGameImpl.Also applies to: 181-181, 199-199
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/PortExecution.test.ts` at line 156, Create a tiny test helper to centralize the private tick manipulation and replace the three occurrences of `(game as any)._ticks += N` with e.g. `advanceTicks(game, N)` (helper should cast Game to a type exposing `_ticks` and increment it); update the three places currently modifying `_ticks` to call this helper to avoid repeating the hack and make future field moves easier. Also, in the "blocked is omitted" test, add the converse assertion that calling `game.blockTradeRouteUntil(port.id(), blockedPort.id(), ...)` does not block the reverse direction (`blockedPort -> port`) by asserting the reverse route remains available, ensuring the directional contract of `GameImpl.blockTradeRouteUntil` is enforced.src/core/game/GameImpl.ts (1)
522-572: LGTM — directional block, deterministic prune, tidy hash inclusion.Two small things, both optional:
isTradeRouteBlockedquietly deletes expired entries on read. That's fine deterministically (all clients run the same lookups in the same order), but it does mean two methods can mutate the map; just keep that in mind if anyone ever introduces parallel reads.- The hash sort allocates a fresh array via
Array.from(...).sort(...)each call. With route-block counts in the tens this is invisible, but if a balance change ever pushes counts higher, swapping to a pre-sorted insertion (or sorting numeric keys) would be cheaper.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/game/GameImpl.ts` around lines 522 - 572, isTradeRouteBlocked currently mutates tradeRouteBlockedUntil by deleting expired entries; instead make it read-only: call pruneExpiredTradeRouteBlocks(this._ticks) or pruneExpiredTradeRouteBlocks(nowTick) at the start of isTradeRouteBlocked and remove the this.tradeRouteBlockedUntil.delete(...) line so reads do not alter the map (refer to isTradeRouteBlocked, pruneExpiredTradeRouteBlocks, tradeRouteBlockedUntil, makeTradeRouteKey). Also avoid allocating a new array on each hash() call: replace Array.from(this.tradeRouteBlockedUntil.entries()).sort(...) with a cheaper approach such as maintaining sorted insertion of keys when setting entries or iterating over pre-collected sorted keys cached outside hash (or at least collect and sort only the keys: Array.from(this.tradeRouteBlockedUntil.keys()).sort(...) then lookup values) so hash() no longer performs an unnecessary full entries copy and sort on every call (refer to hash and tradeRouteBlockedUntil).tests/Warship.test.ts (1)
556-891: Solid coverage for the new retreat+combat behavior.The new cases nicely lock down the tricky interactions: simultaneous retreat-and-fire, manual-move overrides (immediate cancel and the 50‑tick suppression window),
HealAtPortExecutiontaking over the warship,isInCombat()flips on hit and on fire, the "don't pick on docked friends" rule, and continued movement while shooting back. Reads clearly and should catch regressions inWarshipExecution.One tiny thought: the title of the suppression test at line 673 says "5 seconds" while the implementation uses 50 ticks. That's the same thing today, but if the tick rate ever changes, the title will quietly lie. Phrasing it as "for 50 ticks" would age better. Totally optional.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Warship.test.ts` around lines 556 - 891, Rename the test title "Manual MoveWarshipExecution suppresses auto-retreat for 5 seconds before retreat starts" to reflect ticks rather than seconds (e.g. "Manual MoveWarshipExecution suppresses auto-retreat for 50 ticks before retreat starts") so it stays accurate if tick rate changes; update the test string in the tests/Warship.test.ts test that begins with that exact title and leave the test body (uses executeTicks and the 48+5 loop) unchanged.src/core/execution/WarshipExecution.ts (1)
26-27: Nice cleanup — unit‑levelisDocked+ aggro‑while‑retreating reads clean.Switching dock state to live on the unit (
setDocked/isDocked) and havingWarshipExecutionjust read it removes a whole class of "execution and unit disagree about whether the ship is parked" bugs. ThedockedShipsAtPortdefinition (retreating + notargetTile+ within squared docking range) is a clean, composable rule, and keepingapplyActiveDockedHealinggated on the executing warship being in that list avoids double‑healing weirdness when several ships share a port.
findRetreatAggroTargetplus the "fire while retreating, but don't clear the target this tick" logic inhandleRepairRetreatis a really clean way to layer combat on top of movement without coupling the two — the earlysetTargetUnit(retreatAggroTarget)then fall-through to the port-pathing block keeps both behaviors honest.Small things you may want to tidy when you have a quiet moment (all optional, no behavior change needed for this PR):
nearestAvailablePortTile/findNearestAvailablePort/findNearestAvailablePortTile/findBetterPortTileare four tiny wrappers around the same helper. Folding them into one method that takes anexcludeSelf: booleanand an optional "compare against current with switch threshold" mode would shrink the surface area.findRetreatAggroTargetandfindTargetUnitshare most of their "is this a legit hostile" filter (owner, ally,alreadySentShell,isDocked). Extracting a smallisAttackable(unit)predicate would keep them in lockstep if the rules grow.- The double
isDockedblock at lines 71–83 is correct but reads twice — pulling the "should I leave the dock?" decision into a single helper that returns a boolean would make the intent obvious.Also applies to: 57-57, 71-83, 128-150, 210-251, 319-334, 357-357, 437-456, 466-468, 499-560, 590-591, 660-660, 679-679, 737-737
🤖 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 26 - 27, WarshipExecution should consistently read unit dock state via isDocked/setDocked and gate per-tick healing to only ships present in the computed dockedShipsAtPort list: ensure applyActiveDockedHealing checks the executing WarshipExecution's unit is in dockedShipsAtPort before healing; preserve the findRetreatAggroTarget + handleRepairRetreat flow (keep the early setTargetUnit(retreatAggroTarget) then fall-through to port pathing). While here, consolidate the four tiny port helpers (nearestAvailablePortTile, findNearestAvailablePort, findNearestAvailablePortTile, findBetterPortTile) into a single helper that accepts excludeSelf: boolean and an optional current-vs-candidate threshold mode, extract a shared isAttackable(unit) predicate used by findRetreatAggroTarget and findTargetUnit (checking owner, allies, alreadySentShell, isDocked), and collapse the duplicated isDocked decision block (lines around the leave-dock check) into a single helper returning a boolean.
🤖 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/game/UnitImpl.ts`:
- Around line 134-135: toUpdate() currently snapshots isInCombat() causing stale
client state; instead include the server's _lastCombatTick in the update payload
and remove the one-shot inCombat snapshot: add _lastCombatTick to the object
returned by toUpdate(), update the client-side UnitView parsing so
UnitView.isInCombat() computes (gameView.ticks() - data._lastCombatTick <= 3)
(mirroring the server isInCombat()), and ensure any code paths that relied on
data.inCombat now use the computed UnitView.isInCombat(); this keeps combat
visuals in sync without extra scheduled addUpdate() calls.
---
Outside diff comments:
In `@src/core/game/GameImpl.ts`:
- Around line 1247-1283: The displayMessage call in GameImpl is passing the full
gold variable to the conqueror's notification (MessageType.CONQUERED_PLAYER)
which drives the gold animation/UI, but the actual awarded amount is
goldCaptured; update the displayMessage invocation that currently supplies gold
(for conqueror.id()) to provide goldCaptured instead so the animation and UI
reflect the true captured amount (ensure the surrounding block that uses
renderNumber(goldCaptured), conqueror.addGold(goldCaptured), and the
ConquestEvent payload remain unchanged).
---
Duplicate comments:
In `@src/core/execution/WarshipExecution.ts`:
- Around line 290-312: When syncExternalRetreatOrder detects an external retreat
(in WarshipExecution.syncExternalRetreatOrder) and sets retreatingForRepair =
true / retreatPortTile, also clear the manual-move suppression marker (reset
lastManualMoveTickRetreatDisabled) so the manual-order suppression and the
retreat state stay in sync; likewise ensure cancelRepairRetreat clears
lastManualMoveTickRetreatDisabled as well so shouldStartRepairRetreat and
handleManualPatrolOverride don’t remain out-of-sync after the retreat is
cancelled or the port is gone.
---
Nitpick comments:
In `@src/core/execution/HealAtPortExecution.ts`:
- Around line 8-58: The HealAtPortExecution class implements Execution but does
all work in init, leaving tick empty and isActive always false; replace this
fire-once execution with a plain function to simplify semantics: create a new
export function healAtPort(mg: Game, owner: Player, warshipId: number, portTile:
TileRef) that copies the validation and side-effect logic currently in
HealAtPortExecution.init (including water-reachability check if present
elsewhere), and update callers to call healAtPort(...) instead of
addExecution(new HealAtPortExecution(...)); remove the HealAtPortExecution class
(init/tick/isActive/activeDuringSpawnPhase) once callers are migrated.
In `@src/core/execution/TribeExecution.ts`:
- Around line 86-92: The current deleteAllStructures() iterates all units and
calls this.tribe.canDeleteUnit() each time, which either wrongly allows bulk
deletion or redundantly rechecks the same rate-limit; decide the intended
behavior and update accordingly: if canDeleteUnit() is meant to permit only one
deletion per tick, evaluate this.tribe.canDeleteUnit() (or call it and store the
boolean) and after scheduling the first new DeleteUnitExecution(this.tribe,
unit.id()) via this.mg.addExecution(...) break out of the loop to restore
single-deletion cadence; if bulk deletion is intended, hoist the predicate by
testing this.tribe.canDeleteUnit() once before the for loop and only enter the
loop to add DeleteUnitExecution for every matching unit, improving clarity and a
small perf win (references: deleteAllStructures, this.tribe.canDeleteUnit,
this.tribe.units, DeleteUnitExecution, this.mg.addExecution).
In `@src/core/execution/WarshipExecution.ts`:
- Around line 26-27: WarshipExecution should consistently read unit dock state
via isDocked/setDocked and gate per-tick healing to only ships present in the
computed dockedShipsAtPort list: ensure applyActiveDockedHealing checks the
executing WarshipExecution's unit is in dockedShipsAtPort before healing;
preserve the findRetreatAggroTarget + handleRepairRetreat flow (keep the early
setTargetUnit(retreatAggroTarget) then fall-through to port pathing). While
here, consolidate the four tiny port helpers (nearestAvailablePortTile,
findNearestAvailablePort, findNearestAvailablePortTile, findBetterPortTile) into
a single helper that accepts excludeSelf: boolean and an optional
current-vs-candidate threshold mode, extract a shared isAttackable(unit)
predicate used by findRetreatAggroTarget and findTargetUnit (checking owner,
allies, alreadySentShell, isDocked), and collapse the duplicated isDocked
decision block (lines around the leave-dock check) into a single helper
returning a boolean.
In `@src/core/game/GameImpl.ts`:
- Around line 522-572: isTradeRouteBlocked currently mutates
tradeRouteBlockedUntil by deleting expired entries; instead make it read-only:
call pruneExpiredTradeRouteBlocks(this._ticks) or
pruneExpiredTradeRouteBlocks(nowTick) at the start of isTradeRouteBlocked and
remove the this.tradeRouteBlockedUntil.delete(...) line so reads do not alter
the map (refer to isTradeRouteBlocked, pruneExpiredTradeRouteBlocks,
tradeRouteBlockedUntil, makeTradeRouteKey). Also avoid allocating a new array on
each hash() call: replace
Array.from(this.tradeRouteBlockedUntil.entries()).sort(...) with a cheaper
approach such as maintaining sorted insertion of keys when setting entries or
iterating over pre-collected sorted keys cached outside hash (or at least
collect and sort only the keys:
Array.from(this.tradeRouteBlockedUntil.keys()).sort(...) then lookup values) so
hash() no longer performs an unnecessary full entries copy and sort on every
call (refer to hash and tradeRouteBlockedUntil).
In `@src/core/game/UnitImpl.ts`:
- Line 26: Replace the magic numbers used for the combat timing by introducing a
shared constant (e.g., COMBAT_WINDOW_TICKS or read from your existing Config
accessor) and use it to initialize the private field _lastCombatTick and in any
comparisons (instead of -100 and <= 3); additionally, modify setInCombat (the
method that currently calls toUpdate()) to early-return when the unit is already
considered in combat (i.e., if now - _lastCombatTick < COMBAT_WINDOW_TICKS) so
you only call toUpdate() when the in-combat state actually changes, reducing
redundant updates.
In `@tests/PortExecution.test.ts`:
- Line 156: Create a tiny test helper to centralize the private tick
manipulation and replace the three occurrences of `(game as any)._ticks += N`
with e.g. `advanceTicks(game, N)` (helper should cast Game to a type exposing
`_ticks` and increment it); update the three places currently modifying `_ticks`
to call this helper to avoid repeating the hack and make future field moves
easier. Also, in the "blocked is omitted" test, add the converse assertion that
calling `game.blockTradeRouteUntil(port.id(), blockedPort.id(), ...)` does not
block the reverse direction (`blockedPort -> port`) by asserting the reverse
route remains available, ensuring the directional contract of
`GameImpl.blockTradeRouteUntil` is enforced.
In `@tests/Warship.test.ts`:
- Around line 556-891: Rename the test title "Manual MoveWarshipExecution
suppresses auto-retreat for 5 seconds before retreat starts" to reflect ticks
rather than seconds (e.g. "Manual MoveWarshipExecution suppresses auto-retreat
for 50 ticks before retreat starts") so it stays accurate if tick rate changes;
update the test string in the tests/Warship.test.ts test that begins with that
exact title and leave the test body (uses executeTicks and the 48+5 loop)
unchanged.
🪄 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: 96508a1d-8b74-41e1-a6dc-f697ba076c02
📒 Files selected for processing (14)
src/client/graphics/layers/UnitLayer.tssrc/core/execution/HealAtPortExecution.tssrc/core/execution/PortExecution.tssrc/core/execution/TradeShipExecution.tssrc/core/execution/TribeExecution.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/PortExecution.test.tstests/Warship.test.tstests/core/executions/TradeShipExecution.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/core/game/GameUpdates.ts
- tests/core/executions/TradeShipExecution.test.ts
- src/core/game/Game.ts
| docked: this._docked, | ||
| inCombat: this.isInCombat(), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm that warships emit updates often enough that stale inCombat is not user-visible.
# Look at all callers of setInCombat and how often warships push UnitUpdates while idle.
rg -nP '\bsetInCombat\s*\(' --type=ts -C2
rg -nP '\bmaybeAddUnitUpdate\s*\(|\baddUpdate\s*\(this\.toUpdate' src/core/game/UnitImpl.ts -C1Repository: openfrontio/OpenFrontIO
Length of output: 3181
🏁 Script executed:
#!/bin/bash
# Check isInCombat() implementation and the snapshot logic in toUpdate()
rg -nP 'isInCombat\s*\(' src/core/game/UnitImpl.ts -A5 -B2
rg -nP 'toUpdate\s*\(' src/core/game/UnitImpl.ts -A15 | head -80Repository: openfrontio/OpenFrontIO
Length of output: 2731
🏁 Script executed:
#!/bin/bash
# Check if inCombat is being computed fresh on the client or if it's just read from data
rg -nP 'isInCombat|inCombat' src/core/game/GameView.ts -B2 -A2Repository: openfrontio/OpenFrontIO
Length of output: 243
🏁 Script executed:
#!/bin/bash
# Check how often idle/docked warships would naturally emit updates
rg -nP 'maybeAddUnitUpdate|patrol|WarshipExecution' src/core/execution/WarshipExecution.ts --type=ts | head -30Repository: openfrontio/OpenFrontIO
Length of output: 1086
🏁 Script executed:
#!/bin/bash
# Check the patrol() implementation in WarshipExecution
rg -nP 'private patrol\(\)|patrol\s*\(\)\s*\{' src/core/execution/WarshipExecution.ts -A30Repository: openfrontio/OpenFrontIO
Length of output: 985
🏁 Script executed:
#!/bin/bash
# Check if warships skip updates when docked or stationary
rg -nP 'isDocked|isActive|addUpdate' src/core/execution/WarshipExecution.ts | head -40Repository: openfrontio/OpenFrontIO
Length of output: 387
🏁 Script executed:
#!/bin/bash
# Check the move() method to see if it emits updates unconditionally
rg -nP '^\s+move\s*\(' src/core/game/UnitImpl.ts -A10Repository: openfrontio/OpenFrontIO
Length of output: 386
🏁 Script executed:
#!/bin/bash
# Check the contexts where isDocked() is checked around lines 71, 80, 148
rg -nP 'isDocked|move' src/core/execution/WarshipExecution.ts -B3 -A3 | head -80Repository: openfrontio/OpenFrontIO
Length of output: 2581
🏁 Script executed:
#!/bin/bash
# Check what onUnitMoved does - does it emit updates?
rg -nP 'onUnitMoved' src/core/game/GameImpl.ts -B2 -A10Repository: openfrontio/OpenFrontIO
Length of output: 424
🏁 Script executed:
#!/bin/bash
# Check if there are other ways a docked warship emits updates (passive/active healing)
rg -nP 'applyActiveDockedHealing|applyPassiveHealing' src/core/execution/WarshipExecution.ts -B2 -A15Repository: openfrontio/OpenFrontIO
Length of output: 1134
🏁 Script executed:
#!/bin/bash
# Check how long a warship typically stays docked (repair duration config)
rg -nP 'dockedHealingDuration|repairDuration|docking' src/core/game --type=ts -i | head -30Repository: openfrontio/OpenFrontIO
Length of output: 49
🏁 Script executed:
#!/bin/bash
# Check if passive healing actually modifies health and triggers updates
rg -nP 'applyPassiveHealing|passiveHeal' src/core/execution/WarshipExecution.ts -B5 -A15Repository: openfrontio/OpenFrontIO
Length of output: 1287
🏁 Script executed:
#!/bin/bash
# Check modifyHealth to confirm it always calls addUpdate
rg -nP 'modifyHealth\s*\(' src/core/game/UnitImpl.ts -A5Repository: openfrontio/OpenFrontIO
Length of output: 309
🏁 Script executed:
#!/bin/bash
# Look for any other periodic events that might refresh unit updates for docked warships
rg -nP 'touch\(\)|addUpdate.*toUpdate' src/core/execution/WarshipExecution.tsRepository: openfrontio/OpenFrontIO
Length of output: 49
🏁 Script executed:
#!/bin/bash
# Get the full modifyHealth implementation
rg -nP 'modifyHealth\s*\(' src/core/game/UnitImpl.ts -A15Repository: openfrontio/OpenFrontIO
Length of output: 549
🏁 Script executed:
#!/bin/bash
# Check if applyActiveDockedHealing calls modifyHealth and how often
rg -nP 'applyActiveDockedHealing\s*\(' src/core/execution/WarshipExecution.ts -A30 | head -50Repository: openfrontio/OpenFrontIO
Length of output: 1630
🏁 Script executed:
#!/bin/bash
# Check if fully healed docked warship will eventually undock naturally
rg -nP 'undock|setDocked.*false' src/core/execution/WarshipExecution.ts -B3 -A3 | head -50Repository: openfrontio/OpenFrontIO
Length of output: 690
🏁 Script executed:
#!/bin/bash
# Verify the full modifyHealth flow to confirm it calls addUpdate
rg -nP 'modifyHealth\s*\(' src/core/game/UnitImpl.ts -A20 | head -40Repository: openfrontio/OpenFrontIO
Length of output: 685
🏁 Script executed:
#!/bin/bash
# Check setDocked implementation to confirm it triggers addUpdate
rg -nP 'setDocked\s*\(' src/core/game/UnitImpl.ts -A8Repository: openfrontio/OpenFrontIO
Length of output: 339
🏁 Script executed:
#!/bin/bash
# Understand the edge case: fully healed but passive healing = 0
# Check what happens when warship is docked, healing is disabled, and health = max
rg -nP 'warshipPassiveHealing|warshipPortHealingBonus' src/core/config --type=ts -iRepository: openfrontio/OpenFrontIO
Length of output: 123
🏁 Script executed:
#!/bin/bash
# Find config files
fd -type f -name "*config*" --type ts | head -20Repository: openfrontio/OpenFrontIO
Length of output: 238
🏁 Script executed:
#!/bin/bash
# Search for warshipPassiveHealing configuration value
rg -nP 'warshipPassiveHealing|warshipPortHealing' --type=tsRepository: openfrontio/OpenFrontIO
Length of output: 2077
inCombat is a one-shot snapshot; client may see stale state in edge cases.
toUpdate() captures the current value of isInCombat() (which returns this.mg.ticks() - this._lastCombatTick <= 3). The client then reads only this.data.inCombat and does not recompute.
For moving warships, frequent patrol updates refresh the snapshot. For docked warships actively healing, modifyHealth() calls generate updates each tick. However, if a docked warship takes damage and healing is disabled (or unavailable), no updates are generated, leaving the red tint and retreat marker stale for up to 4 ticks until undocking or the next unrelated state change.
Two approaches to fix:
- Send
_lastCombatTickto the client and letUnitView.isInCombat()compute it againstgameView.ticks(), mirroring the server. - Schedule a final
addUpdate()when the 3-tick combat window expires.
🤖 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 134 - 135, toUpdate() currently
snapshots isInCombat() causing stale client state; instead include the server's
_lastCombatTick in the update payload and remove the one-shot inCombat snapshot:
add _lastCombatTick to the object returned by toUpdate(), update the client-side
UnitView parsing so UnitView.isInCombat() computes (gameView.ticks() -
data._lastCombatTick <= 3) (mirroring the server isInCombat()), and ensure any
code paths that relied on data.inCombat now use the computed
UnitView.isInCombat(); this keeps combat visuals in sync without extra scheduled
addUpdate() calls.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/core/game/GameImpl.ts (1)
1262-1268:⚠️ Potential issue | 🟡 MinorSend the captured amount in the display payload too.
The conqueror now receives
goldCaptured, but this event still publishesgoldAmount: gold. Any client code readinggoldAmountwill show the old full amount even though the player only got the reduced share.💡 Small fix
this.displayMessage( "events_display.received_gold_from_conquest", MessageType.CONQUERED_PLAYER, conqueror.id(), - gold, + goldCaptured, { gold: renderNumber(goldCaptured), name: conquered.displayName(), }, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/game/GameImpl.ts` around lines 1262 - 1268, The displayMessage call in GameImpl (this.displayMessage with MessageType.CONQUERED_PLAYER) is still passing the full gold amount and only includes renderNumber(goldCaptured) under the payload key `gold`; update the call so the event uses the actual captured share (pass goldCaptured instead of gold as the positional amount argument) and include the captured amount in the payload as well (add a distinct payload field such as capturedGold or replace the existing gold payload value with renderNumber(goldCaptured)) so clients read the reduced amount the conqueror actually received.
♻️ Duplicate comments (3)
src/client/graphics/layers/UnitLayer.ts (1)
477-496:⚠️ Potential issue | 🟡 MinorThe retreat cross blink still depends on redraw luck, not a real 500 ms repaint cadence.
drawRetreatCross()toggles offDate.now(), but this layer only repaints when units are updated or motion-planned. Retreating warships without frequent redraws will show a sticky/jumpy blink instead of a steady 500 ms one. Based on learnings: UI/layer updates should be wall-clock throttled (independent of simulation speed).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/graphics/layers/UnitLayer.ts` around lines 477 - 496, The blink logic in drawRetreatCross (UnitLayer.drawRetreatCross) depends on Date.now() but the layer only repaints on unit updates, causing uneven blinking; fix by driving repaints on a wall-clock timer: when the UnitLayer detects any unit in retreat start a single setInterval (500ms) that calls the layer's redraw/invalidate method (e.g., requestRedraw/invalidatе/repaint) to force repaints, and clear the interval when no units are retreating; remove reliance on repaint luck (you can keep the Date.now() toggle for drawing but ensure the interval triggers redraws).src/core/execution/WarshipExecution.ts (1)
290-312:⚠️ Potential issue | 🟡 MinorClear the manual-move suppression when adopting an external retreat order.
handleManualPatrolOverride()runs first and stampslastManualMoveTickRetreatDisabledwhenHealAtPortExecutionchangespatrolTile.syncExternalRetreatOrder()then adopts that same change as a retreat order, but the suppression window stays active. If the port dies or the retreat is canceled soon after, auto-retreat is still blocked for the next 50 ticks.🔧 Small fix
this.retreatingForRepair = true; this.retreatPortTile = friendlyPort.tile(); + this.lastManualMoveTickRetreatDisabled = 0; this.warship.setTargetUnit(undefined);🤖 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 290 - 312, When syncExternalRetreatOrder() adopts an external retreat (i.e. sets retreatingForRepair = true and retreatPortTile, and calls this.warship.setTargetUnit(undefined)), also clear the manual-move suppression by resetting lastManualMoveTickRetreatDisabled so the manual-move block set by handleManualPatrolOverride() is removed; locate lastManualMoveTickRetreatDisabled in the same class and set it to a neutral value (e.g. undefined/0) inside syncExternalRetreatOrder() right after adopting the retreat so subsequent auto-retreat logic is not erroneously suppressed.src/core/execution/HealAtPortExecution.ts (1)
35-46:⚠️ Potential issue | 🟡 MinorReject unreachable friendly ports here as well.
A friendly port in a different water component still flips the ship into retreat mode, then
WarshipExecutioncancels it on the next tick. Guarding reachability in this execution avoids the brief false retreat state and makes the order deterministic.🛟 Suggested guard
- const isFriendlyPort = this.owner - .units(UnitType.Port) - .some((port) => port.tile() === this.portTile); - if (!isFriendlyPort) { + const friendlyPort = this.owner + .units(UnitType.Port) + .find((port) => port.tile() === this.portTile); + if (!friendlyPort) { console.warn("HealAtPortExecution: target port is not friendly"); return; } + + const warshipComponent = mg.getWaterComponent(warship.tile()); + if ( + warshipComponent !== null && + !mg.hasWaterComponent(this.portTile, warshipComponent) + ) { + console.warn("HealAtPortExecution: port not reachable by water"); + return; + } // Set warship to go to the port warship.setPatrolTile(this.portTile);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/execution/HealAtPortExecution.ts` around lines 35 - 46, The current HealAtPortExecution accepts any friendly port (computed via this.owner.units(UnitType.Port) and this.portTile) even when it’s in a different water component, causing a transient retreat state; add a reachability check after determining isFriendlyPort and before calling warship.setPatrolTile / setTargetTile / setRetreating(true) to ensure the port is in the same reachable water component as the warship (use the appropriate pathing/reachability helper available in the codebase, e.g., map/graph reachability or warship.canReach/tilesInSameWaterComponent), and if not reachable, log/reject the port and return early to avoid flipping retreat state for unreachable friendly ports.
🤖 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/TribeExecution.ts`:
- Around line 86-90: The loop in deleteAllStructures() can enqueue multiple
DeleteUnitExecution instances in one tick because it re-checks
this.tribe.canDeleteUnit() per iteration and no longer skips already-marked
units; fix it by first skipping units where unit.isMarkedForDeletion() is true
and then, inside the loop, stop enqueuing once this.tribe.canDeleteUnit()
becomes false (e.g., check canDeleteUnit() at loop start and break when it
returns false after adding a DeleteUnitExecution). Reference:
deleteAllStructures(), this.tribe.units(), Structures, unit.type(),
unit.isMarkedForDeletion(), this.tribe.canDeleteUnit(), mg.addExecution(new
DeleteUnitExecution(...)), unit.id().
In `@src/core/execution/WarshipExecution.ts`:
- Around line 434-456: dockedShipsAtPort currently infers docking by distance
and must instead check each ship's actual assigned retreat/docking port; update
the filter in dockedShipsAtPort to require ship.retreating() and
ship.targetTile() to indicate stationary AND that the ship's recorded
retreat/docking port reference matches the passed port (e.g. ship.dockingPort()
=== port or ship.assignedRetreatPortId === port.id), skipping ships with no
assigned port, so counts used by isPortFullOfHealing() reflect the true port
association rather than geometry.
---
Outside diff comments:
In `@src/core/game/GameImpl.ts`:
- Around line 1262-1268: The displayMessage call in GameImpl
(this.displayMessage with MessageType.CONQUERED_PLAYER) is still passing the
full gold amount and only includes renderNumber(goldCaptured) under the payload
key `gold`; update the call so the event uses the actual captured share (pass
goldCaptured instead of gold as the positional amount argument) and include the
captured amount in the payload as well (add a distinct payload field such as
capturedGold or replace the existing gold payload value with
renderNumber(goldCaptured)) so clients read the reduced amount the conqueror
actually received.
---
Duplicate comments:
In `@src/client/graphics/layers/UnitLayer.ts`:
- Around line 477-496: The blink logic in drawRetreatCross
(UnitLayer.drawRetreatCross) depends on Date.now() but the layer only repaints
on unit updates, causing uneven blinking; fix by driving repaints on a
wall-clock timer: when the UnitLayer detects any unit in retreat start a single
setInterval (500ms) that calls the layer's redraw/invalidate method (e.g.,
requestRedraw/invalidatе/repaint) to force repaints, and clear the interval when
no units are retreating; remove reliance on repaint luck (you can keep the
Date.now() toggle for drawing but ensure the interval triggers redraws).
In `@src/core/execution/HealAtPortExecution.ts`:
- Around line 35-46: The current HealAtPortExecution accepts any friendly port
(computed via this.owner.units(UnitType.Port) and this.portTile) even when it’s
in a different water component, causing a transient retreat state; add a
reachability check after determining isFriendlyPort and before calling
warship.setPatrolTile / setTargetTile / setRetreating(true) to ensure the port
is in the same reachable water component as the warship (use the appropriate
pathing/reachability helper available in the codebase, e.g., map/graph
reachability or warship.canReach/tilesInSameWaterComponent), and if not
reachable, log/reject the port and return early to avoid flipping retreat state
for unreachable friendly ports.
In `@src/core/execution/WarshipExecution.ts`:
- Around line 290-312: When syncExternalRetreatOrder() adopts an external
retreat (i.e. sets retreatingForRepair = true and retreatPortTile, and calls
this.warship.setTargetUnit(undefined)), also clear the manual-move suppression
by resetting lastManualMoveTickRetreatDisabled so the manual-move block set by
handleManualPatrolOverride() is removed; locate
lastManualMoveTickRetreatDisabled in the same class and set it to a neutral
value (e.g. undefined/0) inside syncExternalRetreatOrder() right after adopting
the retreat so subsequent auto-retreat logic is not erroneously suppressed.
🪄 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: f9413f1a-cc01-4156-8acf-598f5fac92df
📒 Files selected for processing (16)
src/client/graphics/layers/UnitLayer.tssrc/core/configuration/Config.tssrc/core/configuration/DefaultConfig.tssrc/core/execution/HealAtPortExecution.tssrc/core/execution/PortExecution.tssrc/core/execution/TradeShipExecution.tssrc/core/execution/TribeExecution.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/PortExecution.test.tstests/Warship.test.tstests/core/executions/TradeShipExecution.test.ts
✅ Files skipped from review due to trivial changes (1)
- src/core/game/GameView.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/core/game/GameUpdates.ts
- src/core/execution/PortExecution.ts
- tests/PortExecution.test.ts
| private deleteAllStructures() { | ||
| for (const unit of this.tribe.units()) { | ||
| if (!Structures.has(unit.type())) continue; | ||
| if (unit.isMarkedForDeletion()) continue; | ||
| this.mg.addExecution(new DeleteUnitExecution(this.tribe, unit.id())); | ||
| return; | ||
| if (Structures.has(unit.type()) && this.tribe.canDeleteUnit()) { | ||
| this.mg.addExecution(new DeleteUnitExecution(this.tribe, unit.id())); | ||
| } |
There was a problem hiding this comment.
This can queue multiple structure deletes in one tick.
Line 88 re-checks canDeleteUnit() before any queued DeleteUnitExecution runs, so the loop can enqueue one delete execution per structure in the same pass. Removing the old isMarkedForDeletion() filter also means already-marked structures get picked again instead of moving on.
♻️ Suggested fix
private deleteAllStructures() {
+ if (!this.tribe.canDeleteUnit()) return;
+
for (const unit of this.tribe.units()) {
- if (Structures.has(unit.type()) && this.tribe.canDeleteUnit()) {
+ if (Structures.has(unit.type()) && !unit.isMarkedForDeletion()) {
this.mg.addExecution(new DeleteUnitExecution(this.tribe, unit.id()));
+ return;
}
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/execution/TribeExecution.ts` around lines 86 - 90, The loop in
deleteAllStructures() can enqueue multiple DeleteUnitExecution instances in one
tick because it re-checks this.tribe.canDeleteUnit() per iteration and no longer
skips already-marked units; fix it by first skipping units where
unit.isMarkedForDeletion() is true and then, inside the loop, stop enqueuing
once this.tribe.canDeleteUnit() becomes false (e.g., check canDeleteUnit() at
loop start and break when it returns false after adding a DeleteUnitExecution).
Reference: deleteAllStructures(), this.tribe.units(), Structures, unit.type(),
unit.isMarkedForDeletion(), this.tribe.canDeleteUnit(), mg.addExecution(new
DeleteUnitExecution(...)), unit.id().
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 (2)
src/core/game/GameImpl.ts (1)
1262-1269:⚠️ Potential issue | 🟡 MinorUse
goldCapturedin the display payload too.This conquest path now awards
goldCaptured, butgoldAmountstill sends the old full value. Any client code readinggoldAmountwill be out of sync with the new balance rule.💡 Suggested fix
this.displayMessage( "events_display.received_gold_from_conquest", MessageType.CONQUERED_PLAYER, conqueror.id(), - gold, + goldCaptured, { gold: renderNumber(goldCaptured), name: conquered.displayName(), }, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/game/GameImpl.ts` around lines 1262 - 1269, The display payload is still sending the old full gold value (gold/goldAmount) instead of the newly computed goldCaptured; update the call to displayMessage in GameImpl (the events_display.received_gold_from_conquest / MessageType.CONQUERED_PLAYER path) so that the numeric argument and any payload fields named goldAmount or gold use goldCaptured (and renderNumber(goldCaptured)) instead of gold, referencing the existing symbols displayMessage, goldCaptured, gold, renderNumber, and conquered.displayName() so client code receives the corrected captured amount.src/core/execution/PortExecution.ts (1)
125-142:⚠️ Potential issue | 🟡 MinorFilter blocked destinations before applying proximity bonuses.
Right now blocked ports are skipped inside the loop, but they still count in
iandports.length. That means a blocked near port can eat one of the proximity-bonus slots for the remaining valid ports.💡 Suggested fix
- const weightedPorts: Unit[] = []; - - for (const [i, otherPort] of ports.entries()) { - if ( - this.mg.isTradeRouteBlocked( - this.port.id(), - otherPort.id(), - this.mg.ticks(), - ) - ) { - continue; - } + const availablePorts = ports.filter( + (otherPort) => + !this.mg.isTradeRouteBlocked( + this.port.id(), + otherPort.id(), + this.mg.ticks(), + ), + ); + + const weightedPorts: Unit[] = []; + + for (const [i, otherPort] of availablePorts.entries()) { const expanded = new Array(otherPort.level()).fill(otherPort); weightedPorts.push(...expanded); const tooClose = this.mg.manhattanDist(this.port!.tile(), otherPort.tile()) < this.mg.config().tradeShipShortRangeDebuff(); const closeBonus = - i < this.mg.config().proximityBonusPortsNb(ports.length); + i < this.mg.config().proximityBonusPortsNb(availablePorts.length);🤖 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 125 - 142, The loop currently skips blocked ports with this.mg.isTradeRouteBlocked but still uses the original loop index i and ports.length when computing proximity bonuses, so blocked ports steal proximity slots; fix by filtering blocked destinations before applying proximity logic—either build a filtered array (e.g., validPorts = ports.filter(p => !this.mg.isTradeRouteBlocked(...))) and iterate over validPorts (use its index and length for proximityBonusPortsNb), or keep the loop but maintain a separate counter (unblockedIndex) that increments only for non-blocked ports and use unblockedIndex and the count of unblocked ports when computing closeBonus and proximityBonusPortsNb; update references in PortExecution methods (the for loop over ports, weightedPorts push, tooClose, closeBonus and calls to proximityBonusPortsNb) accordingly.
♻️ Duplicate comments (4)
src/core/game/UnitImpl.ts (1)
134-135:⚠️ Potential issue | 🟡 MinorSend combat time, not a one-shot boolean.
Line 135 snapshots
isInCombat()into the update, but the client cannot age that flag out after the 3-tick window. Docked or otherwise idle warships can stay red / in-combat on the client until some unrelated update arrives. Send_lastCombatTick(or remaining combat ticks) and letUnitView.isInCombat()compute against current game ticks instead.🤖 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 134 - 135, The patch currently serializes a one-shot boolean by writing inCombat: this.isInCombat(); instead send the combat timing so the client can age it: include this._lastCombatTick (or computed remaining combat ticks) in the update payload instead of isInCombat(), and update the client-side UnitView.isInCombat() to compute in-combat state by comparing current game tick to _lastCombatTick; change the serializer in UnitImpl (where docked: this._docked is set) to emit the timestamp/ticks field (e.g. lastCombatTick) so the UI can determine red/idle state correctly.src/core/execution/WarshipExecution.ts (2)
434-456:⚠️ Potential issue | 🟠 MajorPort healing capacity is still assigned by geometry, not by chosen port.
A docked ship here is counted for any friendly port within docking radius, so clustered ports can both think the same ship is consuming one of their healing slots. That makes
isPortFullOfHealing()and the split inapplyActiveDockedHealing()wrong when ports are near each other. Track the ship's assigned retreat/docking port and filter on that instead of distance alone.🤖 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 434 - 456, The dockedShipsAtPort method currently counts any retreating, stationary ship within geometry-only range, causing multiple nearby ports to each claim the same ship; change the filter to require the ship to be assigned to this specific port by checking the ship's docking/retreat port field (e.g., ship.assignedDockPort or ship.assignedRetreatPort) instead of only distance. Update dockedShipsAtPort (and callers like isPortFullOfHealing/applyActiveDockedHealing) to rely on that accessor (create the accessor on Unit if missing) and keep the existing checks (ship.retreating(), ship.targetTile() undefined) but replace the euclideanDistSquared check with a strict equality check comparing the ship's assigned port to the provided port. Ensure the owner().units(UnitType.Warship) filter uses this new assigned-port check so clustered ports no longer double-count the same ship.
290-312:⚠️ Potential issue | 🟡 MinorClear manual-move suppression when adopting an external retreat order.
handleManualPatrolOverride()marks any patrol-tile change as a manual move beforesyncExternalRetreatOrder()converts a friendly-port order intoretreatingForRepair. BecauselastManualMoveTickRetreatDisabledis left intact, a later cancelled heal order can still block auto-retreat for 50 ticks even though that patrol change was actually a retreat command. Reset the suppression timestamp when you adopt the external retreat.🤖 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 290 - 312, When adopting an external retreat in syncExternalRetreatOrder(), also clear the manual-move suppression timestamp so a prior patrol-change flagged as a manual move doesn't block auto-retreat; specifically, inside syncExternalRetreatOrder() where you set this.retreatingForRepair = true, also reset this.lastManualMoveTickRetreatDisabled (e.g. set to undefined or 0) so future cancelled heal orders won't be erroneously suppressed — this touches the same behavior set by handleManualPatrolOverride().src/client/graphics/layers/UnitLayer.ts (1)
477-496:⚠️ Potential issue | 🟡 MinorBlink cadence still depends on simulation redraws.
Line 479 uses
Date.now()for a 500 ms blink, but this layer only repaints units whentick()sees updates or motion-planned movement. Retreating warships can sit without either, so the cross will freeze or jump instead of blinking steadily. Either invalidate this layer on a wall-clock interval while any retreating warship exists, or make the cross non-blinking. Based on learnings: In this TypeScript game codebase, UI/layer updates should be wall-clock throttled (independent of simulation speed).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/graphics/layers/UnitLayer.ts` around lines 477 - 496, The blink in drawRetreatCross() currently uses Date.now() but the layer only repaints on simulation-driven tick(), so the cross freezes; fix by driving repaints on a wall-clock interval while any retreating unit exists: add a 500ms timer on UnitLayer that, when any unit.isRetreating() (or equivalent retreat flag on UnitView) is present, calls the layer's repaint/invalidate method (start the timer when the first retreating unit appears and stop/clear it when none remain), ensure the timer is cleared on layer teardown, and keep drawRetreatCross() logic unchanged so the blink cadence becomes independent of tick().
🤖 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/game/GameImpl.ts`:
- Around line 543-557: The isTradeRouteBlocked method is accidentally deleting
live blocks when queried for a future tick because it compares blockedUntil
against the caller-supplied nowTick; update the logic to only remove expired
blocks using the authoritative current game tick (e.g., this.currentTick or a
dedicated getter) instead of the passed nowTick, or only delete when nowTick ===
this.currentTick; modify the comparison in isTradeRouteBlocked (and keep use of
makeTradeRouteKey and tradeRouteBlockedUntil) so that
tradeRouteBlockedUntil.delete(routeKey) is only called when the block has
actually expired relative to the game's real current tick.
---
Outside diff comments:
In `@src/core/execution/PortExecution.ts`:
- Around line 125-142: The loop currently skips blocked ports with
this.mg.isTradeRouteBlocked but still uses the original loop index i and
ports.length when computing proximity bonuses, so blocked ports steal proximity
slots; fix by filtering blocked destinations before applying proximity
logic—either build a filtered array (e.g., validPorts = ports.filter(p =>
!this.mg.isTradeRouteBlocked(...))) and iterate over validPorts (use its index
and length for proximityBonusPortsNb), or keep the loop but maintain a separate
counter (unblockedIndex) that increments only for non-blocked ports and use
unblockedIndex and the count of unblocked ports when computing closeBonus and
proximityBonusPortsNb; update references in PortExecution methods (the for loop
over ports, weightedPorts push, tooClose, closeBonus and calls to
proximityBonusPortsNb) accordingly.
In `@src/core/game/GameImpl.ts`:
- Around line 1262-1269: The display payload is still sending the old full gold
value (gold/goldAmount) instead of the newly computed goldCaptured; update the
call to displayMessage in GameImpl (the
events_display.received_gold_from_conquest / MessageType.CONQUERED_PLAYER path)
so that the numeric argument and any payload fields named goldAmount or gold use
goldCaptured (and renderNumber(goldCaptured)) instead of gold, referencing the
existing symbols displayMessage, goldCaptured, gold, renderNumber, and
conquered.displayName() so client code receives the corrected captured amount.
---
Duplicate comments:
In `@src/client/graphics/layers/UnitLayer.ts`:
- Around line 477-496: The blink in drawRetreatCross() currently uses Date.now()
but the layer only repaints on simulation-driven tick(), so the cross freezes;
fix by driving repaints on a wall-clock interval while any retreating unit
exists: add a 500ms timer on UnitLayer that, when any unit.isRetreating() (or
equivalent retreat flag on UnitView) is present, calls the layer's
repaint/invalidate method (start the timer when the first retreating unit
appears and stop/clear it when none remain), ensure the timer is cleared on
layer teardown, and keep drawRetreatCross() logic unchanged so the blink cadence
becomes independent of tick().
In `@src/core/execution/WarshipExecution.ts`:
- Around line 434-456: The dockedShipsAtPort method currently counts any
retreating, stationary ship within geometry-only range, causing multiple nearby
ports to each claim the same ship; change the filter to require the ship to be
assigned to this specific port by checking the ship's docking/retreat port field
(e.g., ship.assignedDockPort or ship.assignedRetreatPort) instead of only
distance. Update dockedShipsAtPort (and callers like
isPortFullOfHealing/applyActiveDockedHealing) to rely on that accessor (create
the accessor on Unit if missing) and keep the existing checks
(ship.retreating(), ship.targetTile() undefined) but replace the
euclideanDistSquared check with a strict equality check comparing the ship's
assigned port to the provided port. Ensure the owner().units(UnitType.Warship)
filter uses this new assigned-port check so clustered ports no longer
double-count the same ship.
- Around line 290-312: When adopting an external retreat in
syncExternalRetreatOrder(), also clear the manual-move suppression timestamp so
a prior patrol-change flagged as a manual move doesn't block auto-retreat;
specifically, inside syncExternalRetreatOrder() where you set
this.retreatingForRepair = true, also reset
this.lastManualMoveTickRetreatDisabled (e.g. set to undefined or 0) so future
cancelled heal orders won't be erroneously suppressed — this touches the same
behavior set by handleManualPatrolOverride().
In `@src/core/game/UnitImpl.ts`:
- Around line 134-135: The patch currently serializes a one-shot boolean by
writing inCombat: this.isInCombat(); instead send the combat timing so the
client can age it: include this._lastCombatTick (or computed remaining combat
ticks) in the update payload instead of isInCombat(), and update the client-side
UnitView.isInCombat() to compute in-combat state by comparing current game tick
to _lastCombatTick; change the serializer in UnitImpl (where docked:
this._docked is set) to emit the timestamp/ticks field (e.g. lastCombatTick) so
the UI can determine red/idle state correctly.
🪄 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: 97705e6e-be33-411a-bed6-29b3e1132e10
📒 Files selected for processing (16)
src/client/graphics/layers/UnitLayer.tssrc/core/configuration/Config.tssrc/core/configuration/DefaultConfig.tssrc/core/execution/HealAtPortExecution.tssrc/core/execution/PortExecution.tssrc/core/execution/TradeShipExecution.tssrc/core/execution/TribeExecution.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/PortExecution.test.tstests/Warship.test.tstests/core/executions/TradeShipExecution.test.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- tests/PortExecution.test.ts
- src/core/configuration/Config.ts
- src/core/game/GameView.ts
- src/core/game/Game.ts
- src/core/execution/HealAtPortExecution.ts
- src/core/configuration/DefaultConfig.ts
| isTradeRouteBlocked( | ||
| srcPortId: number, | ||
| dstPortId: number, | ||
| nowTick: Tick, | ||
| ): boolean { | ||
| const routeKey = this.makeTradeRouteKey(srcPortId, dstPortId); | ||
| const blockedUntil = this.tradeRouteBlockedUntil.get(routeKey); | ||
| if (blockedUntil === undefined) { | ||
| return false; | ||
| } | ||
| if (blockedUntil <= nowTick) { | ||
| this.tradeRouteBlockedUntil.delete(routeKey); | ||
| return false; | ||
| } | ||
| return true; |
There was a problem hiding this comment.
Don’t let a future-time query clear the live block.
nowTick is caller input here. If anything asks “is this blocked at tick X in the future?”, this code deletes the route immediately, so later current-tick checks see it as unblocked too early.
💡 Suggested fix
isTradeRouteBlocked(
srcPortId: number,
dstPortId: number,
nowTick: Tick,
): boolean {
const routeKey = this.makeTradeRouteKey(srcPortId, dstPortId);
const blockedUntil = this.tradeRouteBlockedUntil.get(routeKey);
if (blockedUntil === undefined) {
return false;
}
- if (blockedUntil <= nowTick) {
+ if (blockedUntil <= this._ticks) {
this.tradeRouteBlockedUntil.delete(routeKey);
return false;
}
- return true;
+ return blockedUntil > nowTick;
}📝 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.
| isTradeRouteBlocked( | |
| srcPortId: number, | |
| dstPortId: number, | |
| nowTick: Tick, | |
| ): boolean { | |
| const routeKey = this.makeTradeRouteKey(srcPortId, dstPortId); | |
| const blockedUntil = this.tradeRouteBlockedUntil.get(routeKey); | |
| if (blockedUntil === undefined) { | |
| return false; | |
| } | |
| if (blockedUntil <= nowTick) { | |
| this.tradeRouteBlockedUntil.delete(routeKey); | |
| return false; | |
| } | |
| return true; | |
| isTradeRouteBlocked( | |
| srcPortId: number, | |
| dstPortId: number, | |
| nowTick: Tick, | |
| ): boolean { | |
| const routeKey = this.makeTradeRouteKey(srcPortId, dstPortId); | |
| const blockedUntil = this.tradeRouteBlockedUntil.get(routeKey); | |
| if (blockedUntil === undefined) { | |
| return false; | |
| } | |
| if (blockedUntil <= this._ticks) { | |
| this.tradeRouteBlockedUntil.delete(routeKey); | |
| return false; | |
| } | |
| return blockedUntil > nowTick; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/game/GameImpl.ts` around lines 543 - 557, The isTradeRouteBlocked
method is accidentally deleting live blocks when queried for a future tick
because it compares blockedUntil against the caller-supplied nowTick; update the
logic to only remove expired blocks using the authoritative current game tick
(e.g., this.currentTick or a dedicated getter) instead of the passed nowTick, or
only delete when nowTick === this.currentTick; modify the comparison in
isTradeRouteBlocked (and keep use of makeTradeRouteKey and
tradeRouteBlockedUntil) so that tradeRouteBlockedUntil.delete(routeKey) is only
called when the block has actually expired relative to the game's real current
tick.
1 participant
Description:
db1c450 (HEAD -> playtest, origin/playtest) warship updates
b8136c3 increase gold from trade ships
6bd515b increase nuke speed 6->8
8157bae capture only 50% of gold when conquering player
867782c make bots weaker
425ce89 update attack logic (less snowbally)
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
DISCORD_USERNAME