Fixed pirating not being disabled when a warship's owner has no port in that body of water

[TKTK123456]

Before opening a PR: discuss new features on Discord first, and file bugs or small improvements as issues. You must be assigned to an approved issue — unsolicited PRs will be auto-closed.

Add approved & assigned issue number here:

Resolves #4291

Description:

Fixes pirating not being disabled when a warship's owner has no port in that body of water

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

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

tktk1234567

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 177297ae-3604-4edc-86c1-693d2bf137a1

📥 Commits

Reviewing files that changed from the base of the PR and between 8769095 and 777f728.

📒 Files selected for processing (1)

• tests/Warship.test.ts

🚧 Files skipped from review as they are similar to previous changes (1)

• tests/Warship.test.ts

---

Walkthrough

WarshipExecution.findBestTarget now requires a reachable port in the warship’s water component before trade-ship targeting continues. The test suite resets mocks after each case and adds a regression test for the no-reachable-port case.

Changes

Warship Piracy Reachability Gate

Layer / File(s)	Summary
Water-component reachability filter in findBestTarget src/core/execution/WarshipExecution.ts	hasPort is replaced by hasReachablePort, owner ports are checked with mg.hasWaterComponent against the warship’s water component, and trade-ship filtering keeps the existing safety and ownership checks.
Mock reset and reachability regression test tests/Warship.test.ts	afterEach restores Vitest mocks, and a regression test mocks water-component lookup to verify a trade ship keeps its owner when no reachable port exists.

Estimated code review effort: 2 (Simple) | ~10 minutes

Suggested reviewers: evanpelle

Poem

⚓ A warship scans the waves at dawn,
It checks if ports are in the same blue zone.
No shared water? Then hold the line,
The trade ship sails on, safe and fine. 🌊

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly matches the main fix: disabling pirating when the owner has no port in the same body of water.
Description check	✅ Passed	The description directly describes the same bug fix and links it to issue #4291.
Linked Issues check	✅ Passed	The code and test changes enforce pirating only when a reachable port exists in the warship's water body, matching #4291.
Out of Scope Changes check	✅ Passed	The changes stay focused on warship targeting logic and a matching regression test, with no clear out-of-scope additions.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

---

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.}

[evanpelle]

from claude:

src/core changes require tests (CLAUDE.md) — the diff has none and all 29 existing warship tests pass with or without this change, so the new reachable-port logic is completely uncovered.
The port loop should filter isActive() && !isUnderConstruction() && !isMarkedForDeletion() like TradeShipExecution.ts does — otherwise a warship still captures trades that get instantly deleted for zero gold.
When warshipComponent is null, hasReachablePort stays undefined (skipping via !undefined) and the warshipComponent !== null guard at line 292 is now dead — one always-firing init block with .some() would remove the third state and the dead guard.
This is the third in-file copy of "owner's port in my water component" (findNearestPort, nearestAvailablePortTile) — consider a shared Player-level helper, which would also fix #2.

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

tests/Warship.test.ts (1)

142-146: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Simplify: unused mock branch.

getWaterComponent's tile === warshipTile ? 1 : 2 distinction has no effect since hasWaterComponent is mocked with mockReturnValue(false) unconditionally — the reachability check in findBestTarget never inspects the returned component values differently. Consider returning a constant since the branching doesn't drive any behavior being tested.

♻️ Simplify mock setup

-    const warshipTile = warship.tile();
-    vi.spyOn(game, "getWaterComponent").mockImplementation((tile) =>
-      tile === warshipTile ? 1 : 2,
-    );
+    vi.spyOn(game, "getWaterComponent").mockReturnValue(1);
     vi.spyOn(game, "hasWaterComponent").mockReturnValue(false);

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/Warship.test.ts` around lines 142 - 146, The mock setup in the Warship
test has an unnecessary branch because `hasWaterComponent` is always forced to
return false, so `findBestTarget` never uses different values from
`getWaterComponent`. Simplify the `warship.tile()` / `game.getWaterComponent`
spy in `Warship.test.ts` to return a constant value instead of branching on
`tile === warshipTile`, and keep the `game.hasWaterComponent` mock as-is unless
the test actually needs reachability to vary.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@tests/Warship.test.ts`:
- Around line 143-146: The spies on game.getWaterComponent and
game.hasWaterComponent in the Warship test are not being cleaned up, so they can
leak into later tests. Update the Warship.test setup to restore these vi.spyOn
calls after the assertions, or add an afterEach cleanup in this test file that
restores all mocks. Keep the fix scoped to the existing test helpers and the
specific getWaterComponent/hasWaterComponent spies.

---

Nitpick comments:
In `@tests/Warship.test.ts`:
- Around line 142-146: The mock setup in the Warship test has an unnecessary
branch because `hasWaterComponent` is always forced to return false, so
`findBestTarget` never uses different values from `getWaterComponent`. Simplify
the `warship.tile()` / `game.getWaterComponent` spy in `Warship.test.ts` to
return a constant value instead of branching on `tile === warshipTile`, and keep
the `game.hasWaterComponent` mock as-is unless the test actually needs
reachability to vary.

🪄 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: 3df45008-feab-4233-b559-413a2c3c6c00

📥 Commits

Reviewing files that changed from the base of the PR and between 0a8751a and 8448f00.

📒 Files selected for processing (1)

• tests/Warship.test.ts