2661 PR 3/3 Warship Manual Override, Aggro Override, and Heal-at-Port Command

[Zixer1]

Part of #2661 (split into 3 PRs so they are not too large..)

Description:

Part 3/3 of #2661.

This PR adds the retreat control and override behavior for warships:

• Manual override: moving a warship manually cancels retreat and suppresses auto-retreat for 5 seconds
• Aggro override: a retreating warship will aggro a nearby enemy transport or warship before continuing retreat
• Heal-at-port command for sending a warship to a friendly port manually
• Friendly-port validation for HealAtPortExecution
• Regression tests for manual override, aggro override, and heal-at-port behavior

Please complete the following:

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

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

zixer._

[coderabbitai]

Walkthrough

The changes introduce a warship healing and retreat system. Warships can now retreat to friendly ports for healing based on configurable health thresholds and healing rates. Port capacity management, docking mechanics, and visual indicators (blue for retreating warships) enable this feature across configuration, core game logic, execution orders, and rendering layers.

Changes

Cohort / File(s)	Summary
Configuration & Defaults src/core/configuration/Config.ts, src/core/configuration/DefaultConfig.ts	Added six new warship healing configuration methods: warshipPortHealingRadius(), warshipPortHealingBonus(), warshipRetreatHealthThreshold(), warshipPassiveHealing(), warshipPassiveHealingRange(), and warshipPortSwitchThreshold().
Retreat & Healing Logic src/core/execution/WarshipExecution.ts	Major overhaul: added retreat-to-port flow with port selection, docking within healing radius, port-capacity checks, passive healing, active docked healing with fractional accumulation, and retreat cancellation when fully healed or port destroyed. Added isDocked() method.
Execution Orders src/core/execution/HealAtPortExecution.ts, src/core/execution/MoveWarshipExecution.ts	New HealAtPortExecution class issues retreat orders with validation and docking setup; MoveWarshipExecution now sets target tile and clears retreat state on initialization.
Core Game API src/core/game/Game.ts, src/core/game/UnitImpl.ts	Added public setRetreating(boolean) method to Unit interface and implementation with state-change guards; modifyHealth() now skips updates when health is unchanged after clamping; orderBoatRetreat() refactored to delegate to setRetreating().
Game View src/core/game/GameView.ts	UnitView.retreating() now accepts Warship or TransportShip instead of only TransportShip; error message updated accordingly.
Rendering src/client/graphics/layers/UnitLayer.ts	handleWarShipEvent() now draws retreating warships in blue (rgb(0,180,255)) with early return; targets in red; defaults to normal sprite. Replaced if/else structure with early returns.
Tests tests/Warship.test.ts	Added comprehensive test suite covering retreat triggering, docking, port capacity, active healing, healing cancellation, stale-target handling, aggro retargeting, retreat suppression, and HealAtPortExecution behavior.

Sequence Diagram

sequenceDiagram
    participant W as Warship
    participant GE as Game Engine
    participant PS as Port Selector
    participant P as Port
    participant Heal as Healing System

    W->>GE: tick() with health check
    alt Health below retreat threshold
        GE->>PS: select friendly port
        PS->>P: evaluate capacity & distance
        PS-->>GE: return best port
        GE->>W: setRetreating(true)<br/>setTargetTile(port)
    end
    
    alt Retreating & near port
        GE->>W: move toward port via pathfinding
        W->>P: enter healing radius
        GE->>Heal: apply docked healing<br/>(fractional accumulation)
        Heal->>W: modifyHealth(+bonus)
    end
    
    alt Fully healed or port destroyed
        GE->>W: setRetreating(false)<br/>clear docking state
    end

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

---

⚓ Warships now seek safe harbor's glow,
Blue flags wave as weary hulls retreat below,
Healing fractured health with port-side care,
Dancing through capacity and distance fair,
From battle's rage to docking's gentle repair. 🏥✨

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately describes the main features added in this PR: warship manual override, aggro override, and heal-at-port command, which are the core changes across multiple files.
Description check	✅ Passed	The description clearly explains the PR's purpose as part 3/3 of issue #2661, detailing the key features: manual override behavior, aggro override, heal-at-port command, and test coverage, all of which are directly reflected in the changeset.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ 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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

🧹 Nitpick comments (4)

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

501-532: Consider renaming for clarity.

Both findNearestAvailablePort() and findNearestAvailablePortTile() return TileRef, but only one includes "Tile" in the name. The difference is whether this.warship is excluded from the capacity check.

A clearer naming might be:

• findNearestAvailablePortTile() → stays as-is
• findNearestAvailablePort() → findNearestAvailablePortTileForSwitch() or simply inline the difference with a parameter

This is a minor readability improvement and can be addressed later.

Also applies to: 585-618

🤖 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 501 - 532, The two
similarly named methods cause confusion — findNearestAvailablePort() and
findNearestAvailablePortTile() both return TileRef but differ only in whether
the warship is excluded from the capacity check; rename or unify them to make
intent explicit. Either rename findNearestAvailablePort() to
findNearestAvailablePortTileForSwitch() (or another clear name) and update all
callers, or replace both with a single method
findNearestAvailablePortTile(includeSelfInCapacityCheck: boolean) and update
callers to pass the appropriate flag; ensure you update references to these
symbols (findNearestAvailablePort, findNearestAvailablePortTile) and any logic
that relies on isPortFullOfHealing so behavior remains identical after the
rename/merge.

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

35-41: Consider adding isActive() check on the port.

The current check verifies the owner has a port at portTile, but does not confirm the port is still active. If a port is destroyed in the same tick (before this execution runs), the warship could be sent to a dead port.

This is likely benign since WarshipExecution will detect the missing port and cancel the retreat (as shown in the tests). Still, adding the check makes the validation more robust and fail-fast.

🔧 Proposed fix

     const isFriendlyPort = this.owner
       .units(UnitType.Port)
-      .some((port) => port.tile() === this.portTile);
+      .some((port) => port.tile() === this.portTile && port.isActive());
     if (!isFriendlyPort) {
       console.warn("HealAtPortExecution: target port is not friendly");
       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 35 - 41, The current
check in HealAtPortExecution that finds a friendly port via
this.owner.units(UnitType.Port).some(port => port.tile() === this.portTile) does
not verify the port is active; update the validation to also call
port.isActive() (or equivalent) so only active ports are considered (e.g.,
change the predicate to check both port.tile() === this.portTile and
port.isActive()). Ensure the early return/log stays in place when no active
friendly port is found so the execution fails fast.

tests/Warship.test.ts (2)

662-674: Timing assumption: 50 ticks = 5 seconds.

The test assumes a specific tick rate (10 ticks/second). If the game's tick rate differs or changes, this test could give false positives/negatives. Consider adding a comment documenting this assumption, or deriving the tick count from a config value if available.

+    // 48 ticks is just under 5 seconds at 10 ticks/sec; retreat should still be suppressed
     executeTicks(game, 48);
     expect(warship.retreating()).toBe(false);

+    // After 50+ ticks, suppression expires and retreat should resume
     let resumedRetreat = false;
     for (let i = 0; i < 5; i++) {

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/Warship.test.ts` around lines 662 - 674, The test in Warship.test.ts
relies on a hardcoded tick count (executeTicks(game, 48) and looping 5 times)
which assumes 10 ticks/sec; update the test to avoid brittle timing by either
(a) adding a clear comment above the block stating the 10 ticks/sec assumption
and why 48/5 were chosen, or (b) derive the tick counts from the game's
tick-rate configuration (use any available symbol like game.tickRate,
game.TICKS_PER_SECOND, or a test helper) and compute the wait as
Math.ceil(desiredSeconds * ticksPerSecond) before calling executeTicks or
game.executeNextTick in the loop, and ensure warship.retreating() is checked
against that computed duration.

---

471-476: Fragile cast to access private internals.

The as unknown as pattern works for testing private behavior, but it will break silently if internal method/property names change. Consider adding a comment noting this coupling, or extracting a test-only interface if this pattern grows.

+    // NOTE: Accessing private internals for testing. Update if WarshipExecution internals change.
     const executionInternals = execution as unknown as {
       findTargetUnit: () => typeof transport | undefined;
       pathfinder: {
         next: () => { status: PathStatus; node: number };
       };
     };

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/Warship.test.ts` around lines 471 - 476, The test currently uses a
fragile cast "as unknown as { findTargetUnit: ..., pathfinder: { next: ... } }"
to access private internals of the execution object; update the test to avoid
silent breakage by extracting a named test-only interface (e.g.,
ExecutionInternals) that exposes findTargetUnit and pathfinder.next (and
PathStatus), then cast execution to that interface, or at minimum add a clear
comment above the cast explaining the coupling and that renaming these internals
requires updating the test; reference the execution variable, executionInternals
alias, findTargetUnit, pathfinder, next and PathStatus when making the change.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@src/core/execution/HealAtPortExecution.ts`:
- Around line 35-41: The current check in HealAtPortExecution that finds a
friendly port via this.owner.units(UnitType.Port).some(port => port.tile() ===
this.portTile) does not verify the port is active; update the validation to also
call port.isActive() (or equivalent) so only active ports are considered (e.g.,
change the predicate to check both port.tile() === this.portTile and
port.isActive()). Ensure the early return/log stays in place when no active
friendly port is found so the execution fails fast.

In `@src/core/execution/WarshipExecution.ts`:
- Around line 501-532: The two similarly named methods cause confusion —
findNearestAvailablePort() and findNearestAvailablePortTile() both return
TileRef but differ only in whether the warship is excluded from the capacity
check; rename or unify them to make intent explicit. Either rename
findNearestAvailablePort() to findNearestAvailablePortTileForSwitch() (or
another clear name) and update all callers, or replace both with a single method
findNearestAvailablePortTile(includeSelfInCapacityCheck: boolean) and update
callers to pass the appropriate flag; ensure you update references to these
symbols (findNearestAvailablePort, findNearestAvailablePortTile) and any logic
that relies on isPortFullOfHealing so behavior remains identical after the
rename/merge.

In `@tests/Warship.test.ts`:
- Around line 662-674: The test in Warship.test.ts relies on a hardcoded tick
count (executeTicks(game, 48) and looping 5 times) which assumes 10 ticks/sec;
update the test to avoid brittle timing by either (a) adding a clear comment
above the block stating the 10 ticks/sec assumption and why 48/5 were chosen, or
(b) derive the tick counts from the game's tick-rate configuration (use any
available symbol like game.tickRate, game.TICKS_PER_SECOND, or a test helper)
and compute the wait as Math.ceil(desiredSeconds * ticksPerSecond) before
calling executeTicks or game.executeNextTick in the loop, and ensure
warship.retreating() is checked against that computed duration.
- Around line 471-476: The test currently uses a fragile cast "as unknown as {
findTargetUnit: ..., pathfinder: { next: ... } }" to access private internals of
the execution object; update the test to avoid silent breakage by extracting a
named test-only interface (e.g., ExecutionInternals) that exposes findTargetUnit
and pathfinder.next (and PathStatus), then cast execution to that interface, or
at minimum add a clear comment above the cast explaining the coupling and that
renaming these internals requires updating the test; reference the execution
variable, executionInternals alias, findTargetUnit, pathfinder, next and
PathStatus when making the change.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: f9478eee-e777-41d9-aaa3-71faa373f62a

📥 Commits

Reviewing files that changed from the base of the PR and between 496f100 and 0c71546.

📒 Files selected for processing (10)

• src/client/graphics/layers/UnitLayer.ts
• src/core/configuration/Config.ts
• src/core/configuration/DefaultConfig.ts
• src/core/execution/HealAtPortExecution.ts
• src/core/execution/MoveWarshipExecution.ts
• src/core/execution/WarshipExecution.ts
• src/core/game/Game.ts
• src/core/game/GameView.ts
• src/core/game/UnitImpl.ts
• tests/Warship.test.ts