Fix incorrect display of ability to upgrade enemy units

[Lavodan]

Description:

Fixes #2135.
Adds an owner check to canUpgradeUnit to prevent enemy units from seemingly being able to be upgraded when Ctrl+clicked on

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:

Lavodan

[coderabbitai]

Walkthrough

Adds an ownership check to canUpgradeUnit in src/core/game/PlayerImpl.ts: the method now returns false when the unit's owner() is not the calling player (unit.owner() !== this), in addition to existing deleted/upgradable/disabled/gold checks.

Changes

Cohort / File(s)	Change Summary
Ownership validation in upgrade eligibility src/core/game/PlayerImpl.ts	Adds an ownership guard to canUpgradeUnit so it returns false if unit.owner() !== this, preserving existing checks (deleted, upgradable, disabled, gold).

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant UI as UI / Controller
  participant Player as PlayerImpl
  participant Unit as Unit

  UI->>Player: request canUpgradeUnit(unit)
  Player->>Unit: unit.owner()
  alt owner mismatch
    rect rgb(255,250,204)
    Note right of Player: ownership check fails
    Player-->>UI: return false
    end
  else owner matches
    Player->>Unit: check deleted/upgradable/disabled
    alt any check fails
      Player-->>UI: return false
    else all checks pass
      Player-->>UI: return true
    end
  end

Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

• Verify unit.owner() !== this is valid in server and client contexts.
• Check related client-side filters or UI hooks that assume upgrade eligibility.
• Run or update unit/integration tests covering upgrade permission.

Possibly related PRs

• Fix the ghost unit turning green when hovering an ennemy structure #2175 — Adds owner filtering in client UI when highlighting incrementable/upgradable structures; directly related to ownership checks.
• upgrades not counting towards building discount bugfix #1361 — Changes unit-upgrade flow and records upgraded units in upgradeUnit; touches same PlayerImpl upgrade area.

Suggested reviewers

• evanpelle

Poem

A small guard stands beside the gate,
"Is it yours?" it asks with gentle weight.
Flags and gold and status too,
One final check to say who may do. 🎖️

Pre-merge checks

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title Check	✅ Passed	The pull request title "Fix incorrect display of ability to upgrade enemy units" is directly related to the primary change in the changeset. The title clearly communicates the main objective—fixing a bug where enemy units incorrectly appear upgradeable—which aligns with the code change that adds ownership validation to the canUpgradeUnit function. The title is concise, specific, and provides sufficient context for someone reviewing the commit history to understand the purpose of the change.
Linked Issues Check	✅ Passed	The code changes directly address the requirement in linked issue #2135. The issue reports that pressing Ctrl+left click on an enemy building incorrectly shows it as incrementable, and the expected behavior is that enemy buildings should not be clickable in the menu. The PR implements this fix by adding an ownership validation in canUpgradeUnit that returns false if unit.owner() !== this, which prevents enemy units from appearing upgradeable. This directly fulfills the objective specified in the linked issue.
Out of Scope Changes Check	✅ Passed	Based on the provided summary, all changes in this pull request are in scope. The modification is limited to src/core/game/PlayerImpl.ts, where an ownership validation was added to the canUpgradeUnit function. This change directly addresses the objective of preventing enemy units from appearing upgradeable. The summary confirms there are no other control-flow or public API changes, and no public/exported-entity signature changes were detected, indicating the change is focused and targeted to the specific issue being resolved.

---

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between bf46ba4 and ed0f899.

📒 Files selected for processing (1)

• src/core/game/PlayerImpl.ts (1 hunks)

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

• src/core/game/PlayerImpl.ts

---

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]

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 91f1748 and 79d9ada.

📒 Files selected for processing (1)

• src/core/game/PlayerImpl.ts (1 hunks)

🧰 Additional context used

🪛 GitHub Actions: 🧪 CI

src/core/game/PlayerImpl.ts

[warning] 1-1: Code style issues found in the file. Run 'npx prettier --write' to fix.

---

[error] 1-1: Prettier formatting check failed. Process completed with exit code 1. Run 'npx prettier --write' to fix code style issues.

🔇 Additional comments (1)

src/core/game/PlayerImpl.ts (1)

905-907: Ownership check correctly prevents upgrading enemy units.

The logic is sound and directly fixes the reported issue. The guard ensures only units owned by the current player can be upgraded, preventing the UI from showing enemy buildings as upgradeable when Ctrl+clicked.

[kkissdev]

Heya! Just to let you know that a milestone should be added. Thanks

[Lavodan]

@kkissdev I don't think I have the ability to set milestones, tags or anything like that. Or at least I don't see the option here on github.

[kkissdev]

Ah, I see.

[jrouillard]

Please fix the prettier issue, we use spaces and not tabs.
Thank you for fixing this bug

[Lavodan]

I may or may not have forgotten to push from my local branch

[jrouillard]

Thanks 👑