2661 PR 1/3 Warship Retreat Core, Blue UI Signal, and Transport-First Target Priority#3498
Open
Zixer1 wants to merge 6 commits intoopenfrontio:mainfrom
Open
2661 PR 1/3 Warship Retreat Core, Blue UI Signal, and Transport-First Target Priority#3498Zixer1 wants to merge 6 commits intoopenfrontio:mainfrom
Zixer1 wants to merge 6 commits intoopenfrontio:mainfrom
Conversation
|
|
Contributor
WalkthroughAdds warship repair-and-retreat behavior and persistent retreat state: warships heal and auto-retreat to nearest friendly port when pre-heal health falls below a new configurable threshold; rendering and targeting order updated; config, unit API, execution logic, view, and tests adjusted. Changes
Sequence DiagramsequenceDiagram
actor GameLoop as Game Loop
participant WExec as WarshipExecution
participant Finder as PortFinder
participant Path as Pathfinder
participant Unit as Unit
GameLoop->>WExec: tick()
Note over WExec: snapshot healthBeforeHeal\nhealWarship()
WExec->>WExec: shouldStartRepairRetreat()? (pre-heal < threshold, has port)
alt start retreat
WExec->>Finder: findNearestPort()
Finder-->>WExec: port tile (or none)
WExec->>Unit: setRetreating(true)
WExec->>Path: request path to port
Path-->>WExec: path / status
WExec->>WExec: handleRepairRetreat() (move toward port)
alt reached and fully healed
WExec->>Unit: setRetreating(false)
WExec->>WExec: cancelRepairRetreat()
end
else normal combat flow
WExec->>WExec: patrol()
WExec->>WExec: acquire target (TransportShip > Warship > TradeShip)
WExec->>Unit: shootTarget() / engage
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/core/execution/WarshipExecution.ts (3)
114-131: Redundant port check.Lines 125-127 check
unitCount(Port) === 0and return false. Lines 129-130 checkports.length > 0and return that. If the first check passes (ports exist), the second check will always be true.♻️ Suggested simplification
private shouldStartRepairRetreat( healthBeforeHealing = this.warship.health(), ): boolean { if (this.retreatingForRepair) { return false; } if ( healthBeforeHealing >= this.mg.config().warshipRetreatHealthThreshold() ) { return false; } - if (this.warship.owner().unitCount(UnitType.Port) === 0) { - return false; - } - // Only retreat if there's a friendly port const ports = this.warship.owner().units(UnitType.Port); return ports.length > 0; }🤖 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 114 - 131, The method shouldStartRepairRetreat contains a redundant port existence check: remove the earlier unitCount( UnitType.Port ) === 0 check and only use the ports array check returned from this.warship.owner().units(UnitType.Port); keep the initial early-return checks for this.retreatingForRepair and the health threshold (this.mg.config().warshipRetreatHealthThreshold()), then compute const ports = this.warship.owner().units(UnitType.Port) and return ports.length > 0 as the final decision; this simplifies logic and avoids duplicate queries to the owner.
481-481: Minor: Trailing newline after closing brace.The pipeline reports Prettier formatting issues. Run
npx prettier --write .to fix.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/execution/WarshipExecution.ts` at line 481, Remove the trailing blank line after the final closing brace in WarshipExecution (class/file ending) and reformat the file with Prettier; run npx prettier --write . or apply Prettier rules to src/core/execution/WarshipExecution.ts so the extraneous newline is removed and formatting passes.
245-259: Remove unused method or call it inhandleRepairRetreatto refresh the retreat port.The
refreshRetreatPortTile()method is never called. Consider integrating it intohandleRepairRetreat()around line 215, whereretreatPortTileisundefined. Instead of returningfalsewhen the port is undefined, callrefreshRetreatPortTile()to find a new nearest port and continue the 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 245 - 259, The method refreshRetreatPortTile() is never invoked; update handleRepairRetreat() so when retreatPortTile is undefined it calls refreshRetreatPortTile() (which uses findNearestPort() and updates this.retreatPortTile) instead of immediately returning false; if refreshRetreatPortTile() still returns false then return false, otherwise proceed with the existing retreat logic using the newly set this.retreatPortTile.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/Warship.test.ts`:
- Around line 285-286: The test references a non-existent config property
warshipPortHealingBonus causing TS2339; either add warshipPortHealingBonus():
number to the Config interface and implement it in DefaultConfig (returning the
current default 1) so calls like game.config().warshipPortHealingBonus()
compile, or remove the test line setting warshipPortHealingBonus and keep the
existing fixed heal behavior in WarshipExecution.healWarship(); update the
Config/DefaultConfig definitions if you choose the first option and ensure
WarshipExecution.healWarship() reads the new config method.
- Around line 203-224: The test "Warship prioritizes transport ships over
warships" fails because WarshipExecution needs init time; either create a port
for player1 before building the warship (mirror the "Warship captures trade if
player has port" setup) so targeting initializes in one tick, or leave no port
and advance multiple ticks (e.g., call game.executeNextTick() in a loop like the
other no-port test) to allow WarshipExecution to pick its target; update the
test to use one of these approaches around WarshipExecution and executeNextTick
so warship.targetUnit() becomes a TransportShip.
---
Nitpick comments:
In `@src/core/execution/WarshipExecution.ts`:
- Around line 114-131: The method shouldStartRepairRetreat contains a redundant
port existence check: remove the earlier unitCount( UnitType.Port ) === 0 check
and only use the ports array check returned from
this.warship.owner().units(UnitType.Port); keep the initial early-return checks
for this.retreatingForRepair and the health threshold
(this.mg.config().warshipRetreatHealthThreshold()), then compute const ports =
this.warship.owner().units(UnitType.Port) and return ports.length > 0 as the
final decision; this simplifies logic and avoids duplicate queries to the owner.
- Line 481: Remove the trailing blank line after the final closing brace in
WarshipExecution (class/file ending) and reformat the file with Prettier; run
npx prettier --write . or apply Prettier rules to
src/core/execution/WarshipExecution.ts so the extraneous newline is removed and
formatting passes.
- Around line 245-259: The method refreshRetreatPortTile() is never invoked;
update handleRepairRetreat() so when retreatPortTile is undefined it calls
refreshRetreatPortTile() (which uses findNearestPort() and updates
this.retreatPortTile) instead of immediately returning false; if
refreshRetreatPortTile() still returns false then return false, otherwise
proceed with the existing retreat logic using the newly set
this.retreatPortTile.
🪄 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: 6e780270-d739-4b02-9d97-3d38bb6d0619
📒 Files selected for processing (8)
src/client/graphics/layers/UnitLayer.tssrc/core/configuration/Config.tssrc/core/configuration/DefaultConfig.tssrc/core/execution/WarshipExecution.tssrc/core/game/Game.tssrc/core/game/GameView.tssrc/core/game/UnitImpl.tstests/Warship.test.ts
github-project-automation
bot
moved this from Triage
to Development
in OpenFront Release Management
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/core/execution/WarshipExecution.ts (1)
114-131: Redundant port existence checks.Line 125 already returns
falsewhenunitCount(UnitType.Port) === 0. After that guard, lines 129-130 fetch the same ports and checkports.length > 0, which is guaranteed to be true at that point.♻️ Simplify by removing redundant check
private shouldStartRepairRetreat( healthBeforeHealing = this.warship.health(), ): boolean { if (this.retreatingForRepair) { return false; } if ( healthBeforeHealing >= this.mg.config().warshipRetreatHealthThreshold() ) { return false; } - if (this.warship.owner().unitCount(UnitType.Port) === 0) { - return false; - } - // Only retreat if there's a friendly port - const ports = this.warship.owner().units(UnitType.Port); - return ports.length > 0; + // Only retreat if there's a friendly port + return this.warship.owner().unitCount(UnitType.Port) > 0; }🤖 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 114 - 131, The method shouldStartRepairRetreat currently checks for port existence twice (this.warship.owner().unitCount(UnitType.Port) === 0 and then this.warship.owner().units(UnitType.Port) with a length check). Replace the duplicated checks with a single consistent check: call this.warship.owner().units(UnitType.Port) once, store the result (e.g., ports), return false if ports.length === 0, otherwise continue/return true; remove the unitCount(...) guard to avoid the redundant call. Ensure changes are made inside shouldStartRepairRetreat and reference UnitType.Port, owner().units, and owner().unitCount accordingly.
🤖 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/WarshipExecution.ts`:
- Around line 415-417: The isDocked() method in the WarshipExecution class is
unused and always returns false; remove the isDocked() method from
WarshipExecution unless it is required by an interface or superclass contract—if
it's required, implement it correctly to reflect actual docked state (e.g.,
consult this.dockingStatus or relevant property) and update any interface
signature accordingly; otherwise delete the isDocked() method declaration to
eliminate dead code.
- Around line 245-259: Remove the unused private method
refreshRetreatPortTile(): boolean from the WarshipExecution class; the
retreatPortTile field is already managed elsewhere (e.g., handleRepairRetreat),
so delete the entire refreshRetreatPortTile implementation and any references
(there should be none) to refreshRetreatPortTile to eliminate dead code and keep
the class focused on the single source of truth for retreatPortTile.
---
Nitpick comments:
In `@src/core/execution/WarshipExecution.ts`:
- Around line 114-131: The method shouldStartRepairRetreat currently checks for
port existence twice (this.warship.owner().unitCount(UnitType.Port) === 0 and
then this.warship.owner().units(UnitType.Port) with a length check). Replace the
duplicated checks with a single consistent check: call
this.warship.owner().units(UnitType.Port) once, store the result (e.g., ports),
return false if ports.length === 0, otherwise continue/return true; remove the
unitCount(...) guard to avoid the redundant call. Ensure changes are made inside
shouldStartRepairRetreat and reference UnitType.Port, owner().units, and
owner().unitCount accordingly.
🪄 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: 79365db2-6564-454f-a711-167a75bdf777
📒 Files selected for processing (2)
src/core/execution/WarshipExecution.tstests/Warship.test.ts
✅ Files skipped from review due to trivial changes (1)
- tests/Warship.test.ts
Zixer1
force-pushed
the
split/2661-300-v2-pr1
branch
from
dd282c6 to
e798f4b
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/core/execution/WarshipExecution.ts (2)
415-417:⚠️ Potential issue | 🟡 MinorDead code:
isDocked()always returnsfalseand is never called.Remove this method unless it's required by an interface.
🤖 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 415 - 417, The isDocked() method in WarshipExecution always returns false and is unused; either remove the method from the WarshipExecution class or implement it correctly if required by an interface/contract (confirm whether WarshipExecution implements an interface or base class that declares isDocked). If it's required, replace the stub with a proper check using the class's docking state (e.g., reference whatever property/method tracks docking status inside WarshipExecution); otherwise delete the isDocked() method to eliminate dead code.
245-259:⚠️ Potential issue | 🟡 MinorDead code:
refreshRetreatPortTile()is never called.This method is not used anywhere. The
retreatPortTilefield is managed directly instartRepairRetreat()andhandleRepairRetreat(). Remove it to keep the code clean.🤖 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 245 - 259, Remove the dead private method refreshRetreatPortTile from the WarshipExecution class; locate and delete the refreshRetreatPortTile() implementation and any redundant comments referencing it, and ensure retreatPortTile continues to be managed by startRepairRetreat() and handleRepairRetreat() without calling the removed method; also remove any imports or references exclusively used by refreshRetreatPortTile (e.g., local uses of UnitType.Port within that method) so there are no unused-symbol warnings.
🧹 Nitpick comments (1)
src/core/execution/WarshipExecution.ts (1)
114-131: Redundant port count check.Line 125-127 checks
unitCount(UnitType.Port) === 0, and lines 129-130 checkports.length > 0. These are the same check. You can remove one.♻️ Suggested simplification
private shouldStartRepairRetreat( healthBeforeHealing = this.warship.health(), ): boolean { if (this.retreatingForRepair) { return false; } if ( healthBeforeHealing >= this.mg.config().warshipRetreatHealthThreshold() ) { return false; } - if (this.warship.owner().unitCount(UnitType.Port) === 0) { - return false; - } - // Only retreat if there's a friendly port const ports = this.warship.owner().units(UnitType.Port); return ports.length > 0; }🤖 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 114 - 131, The method shouldStartRepairRetreat has a redundant port existence check: it calls warship.owner().unitCount(UnitType.Port) === 0 and then later builds ports = warship.owner().units(UnitType.Port) and checks ports.length > 0; remove one of these redundant checks (prefer keeping the units(...) call so you can reuse the ports array) by deleting the unitCount(...) check and relying on ports.length > 0 to determine if a friendly Port exists; keep all other guards (retreatingForRepair and mg.config().warshipRetreatHealthThreshold()) intact.
🤖 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/WarshipExecution.ts`:
- Around line 212-215: When retreatPortTile is undefined while the ship is in a
retreat state, cancel the retreat so the ship doesn't get stuck: in the block
around retreatPortTile and retreatingForRepair (used by
shouldStartRepairRetreat() and handleRepairRetreat()), detect retreatPortTile
=== undefined and, if retreatingForRepair is true, reset the retreat state
(clear retreatingForRepair and any associated retreatPortTile/retreat metadata)
before returning false; update the logic in handleRepairRetreat()/where
retreatPortTile is read to perform this state cleanup so subsequent calls won't
treat the ship as still retreating.
---
Duplicate comments:
In `@src/core/execution/WarshipExecution.ts`:
- Around line 415-417: The isDocked() method in WarshipExecution always returns
false and is unused; either remove the method from the WarshipExecution class or
implement it correctly if required by an interface/contract (confirm whether
WarshipExecution implements an interface or base class that declares isDocked).
If it's required, replace the stub with a proper check using the class's docking
state (e.g., reference whatever property/method tracks docking status inside
WarshipExecution); otherwise delete the isDocked() method to eliminate dead
code.
- Around line 245-259: Remove the dead private method refreshRetreatPortTile
from the WarshipExecution class; locate and delete the refreshRetreatPortTile()
implementation and any redundant comments referencing it, and ensure
retreatPortTile continues to be managed by startRepairRetreat() and
handleRepairRetreat() without calling the removed method; also remove any
imports or references exclusively used by refreshRetreatPortTile (e.g., local
uses of UnitType.Port within that method) so there are no unused-symbol
warnings.
---
Nitpick comments:
In `@src/core/execution/WarshipExecution.ts`:
- Around line 114-131: The method shouldStartRepairRetreat has a redundant port
existence check: it calls warship.owner().unitCount(UnitType.Port) === 0 and
then later builds ports = warship.owner().units(UnitType.Port) and checks
ports.length > 0; remove one of these redundant checks (prefer keeping the
units(...) call so you can reuse the ports array) by deleting the unitCount(...)
check and relying on ports.length > 0 to determine if a friendly Port exists;
keep all other guards (retreatingForRepair and
mg.config().warshipRetreatHealthThreshold()) intact.
🪄 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: 655d11db-3898-4dc3-b856-8b24bb5a3e00
📒 Files selected for processing (8)
src/client/graphics/layers/UnitLayer.tssrc/core/configuration/Config.tssrc/core/configuration/DefaultConfig.tssrc/core/execution/WarshipExecution.tssrc/core/game/Game.tssrc/core/game/GameView.tssrc/core/game/UnitImpl.tstests/Warship.test.ts
✅ Files skipped from review due to trivial changes (3)
- src/core/game/Game.ts
- src/client/graphics/layers/UnitLayer.ts
- tests/Warship.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/core/configuration/DefaultConfig.ts
- src/core/configuration/Config.ts
- src/core/game/UnitImpl.ts
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/core/execution/WarshipExecution.ts (1)
209-212:⚠️ Potential issue | 🟡 MinorStuck state when
retreatPortTileis undefined while retreating.If
retreatPortTilebecomes undefined whileretreatingForRepairis true (e.g., timing edge case), the ship gets stuck:handleRepairRetreat()returnsfalsewithout clearing the retreat flag, andshouldStartRepairRetreat()also returnsfalse(already retreating). The warship remains in retreat visual state but cannot actually retreat.Cancel the retreat to ensure consistent state:
🛡️ Suggested fix
const retreatPortTile = this.retreatPortTile; if (retreatPortTile === undefined) { + this.cancelRepairRetreat(); return false; }🤖 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 209 - 212, If retreatPortTile is undefined during a repair retreat, clear the retreating state so the ship doesn't get stuck: inside the logic around retreatPortTile (used by handleRepairRetreat()), detect the undefined case and set the retreat flag (retreatingForRepair) back to false and invoke any existing cancel/cleanup paths (e.g., call or emulate cancelRepairRetreat()/clearRetreatVisualState() if present) so shouldStartRepairRetreat() no longer thinks the unit is retreating; update handleRepairRetreat() to perform this cleanup when retreatPortTile === undefined and ensure any related state/reset logic is centralized so visuals and flags remain consistent.
🧹 Nitpick comments (1)
src/core/execution/WarshipExecution.ts (1)
163-174: Fallback port selection may cause repeated pathfinding failures.The fallback loop ignores water component connectivity. If the warship cannot reach any port (all in different water bodies), this returns an unreachable port. Then pathfinding fails with
NOT_FOUND, which callsfindNearestPort()again, selecting the same unreachable port—creating an infinite retry cycle.Consider returning
undefinedin the fallback case to triggercancelRepairRetreat()instead:♻️ Suggested change
// Fallback if component filtering leaves no options - bestDistance = Infinity; - for (const port of ports) { - const portTile = port.tile(); - const distance = this.mg.euclideanDistSquared(warshipTile, portTile); - if (distance < bestDistance) { - bestDistance = distance; - bestTile = portTile; - } - } - - return bestTile; + // No reachable port in same water component - cancel retreat + return 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 163 - 174, The fallback loop in findNearestPort (WarshipExecution) selects the closest port by distance without checking water-component connectivity, which can return an unreachable port and cause repeated NOT_FOUND retries; update the fallback to only consider ports in the same water component as the warship (or detect reachability) and if none exist return undefined instead of bestTile so the caller (cancelRepairRetreat()) can abort; locate the loop that iterates over ports (uses port.tile(), this.mg.euclideanDistSquared, bestDistance/bestTile) and change its logic to skip ports not in the warship's water component (or add a connectivity check) and return undefined when no reachable port was found.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/core/execution/WarshipExecution.ts`:
- Around line 209-212: If retreatPortTile is undefined during a repair retreat,
clear the retreating state so the ship doesn't get stuck: inside the logic
around retreatPortTile (used by handleRepairRetreat()), detect the undefined
case and set the retreat flag (retreatingForRepair) back to false and invoke any
existing cancel/cleanup paths (e.g., call or emulate
cancelRepairRetreat()/clearRetreatVisualState() if present) so
shouldStartRepairRetreat() no longer thinks the unit is retreating; update
handleRepairRetreat() to perform this cleanup when retreatPortTile === undefined
and ensure any related state/reset logic is centralized so visuals and flags
remain consistent.
---
Nitpick comments:
In `@src/core/execution/WarshipExecution.ts`:
- Around line 163-174: The fallback loop in findNearestPort (WarshipExecution)
selects the closest port by distance without checking water-component
connectivity, which can return an unreachable port and cause repeated NOT_FOUND
retries; update the fallback to only consider ports in the same water component
as the warship (or detect reachability) and if none exist return undefined
instead of bestTile so the caller (cancelRepairRetreat()) can abort; locate the
loop that iterates over ports (uses port.tile(), this.mg.euclideanDistSquared,
bestDistance/bestTile) and change its logic to skip ports not in the warship's
water component (or add a connectivity check) and return undefined when no
reachable port was found.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 18e218d9-65c8-4989-bcd0-7edba0cff014
📒 Files selected for processing (1)
src/core/execution/WarshipExecution.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part of #2661 (split into 3 PRs so they are not too large..)
Description:
Part 1/3 of #2661.
This PR adds warship retreat basics, a blue retreating UI state, and updates target priority.
Added:
Example video:
https://youtu.be/2hE2qeOeY48
Ship retreating:
Ship healing:
Back to being deployed:
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
zixer._