WebGL: show alliance request+duration icon, show ally and team mate targets too, some optimization

[VariableVince]

Description:

Show nuke icons during replay too (when there's no localPlayer).
Show alliance request envelope icon, and duration in alliance icon (weren't calculated yet).
Show ally and team mates' targets too (weren't calculated yet).

Remove unnecessary allocations. Nukes loop allocated two new sets, transitive targets was a new set and now uses predicate with fallback to localPlayer.targets, localPlayer.allies and localPlayer.embargoes were both put in new set instead of using .includes directly.

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:

tryout33

[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: 93620882-75a8-4940-a6ee-33325a754919

📥 Commits

Reviewing files that changed from the base of the PR and between d1d25f8 and 0898c67.

📒 Files selected for processing (1)

• src/client/render/frame/derive/PlayerStatus.ts

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

• src/client/render/frame/derive/PlayerStatus.ts

---

Walkthrough

PlayerView adds transitive-target detection; ComputePlayerStatusOptions splits numeric localPlayerSmallID from string localPlayerID; computePlayerStatus now computes per-player nuke flags and alliance progress and emits allianceReq/allianceFraction; GameView passes new options; tests updated to use localPlayerSmallID.

Changes

Transitive Target Support and Player Status Refactor

Layer / File(s)	Summary
Transitive target detection in PlayerView src/client/view/PlayerView.ts	PlayerView.hasTransitiveTarget(sid) checks whether a small ID appears in direct targets, allied players' targets, or same-team players' targets, returning early on match.
Player status contract change src/client/render/frame/derive/PlayerStatus.ts	ComputePlayerStatusOptions adds localPlayerSmallID?: number and changes localPlayerID to string; JSDoc and option extraction updated.
Per-player relative flags and nuke handling src/client/render/frame/derive/PlayerStatus.ts	Per-player loop now scans that player's active nukes to set nukeActive/nukeTargetsMe (uses localPlayerSmallID with tileState), computes alliance, allianceReq, target, and bilateral embargo only when a local player exists and the player isn't local, and derives allianceFraction from alliance expiry when tick/duration present.
Result emission and stored payload src/client/render/frame/derive/PlayerStatus.ts	The condition to include a player now treats allianceReq as an emission trigger; stored PlayerStatusData now persists allianceReq and the computed allianceFraction.
Integration with rendering pipeline src/client/view/GameView.ts	GameView.populateFrame() calls computePlayerStatus() with localPlayerSmallID, localPlayerID (string), tick, allianceDuration(), and an isTransitiveTarget callback from this._myPlayer?.hasTransitiveTarget.
Test coverage for refactored player status tests/client/render/frame/derive/player-status.test.ts	All live-mode tests switched to localPlayerSmallID for options: alliance, target, bilateral embargo, self-relationship exclusion, nuke targets (with/without tileState), entry emission, and deferred-alliance checks updated accordingly.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Suggested labels

UI/UX

Suggested reviewers

• evanpelle

Poem

Transitive paths now cross the field,
Local IDs split, the truths revealed,
Nukes are scanned per player line,
Alliances track their fading time,
GameView threads the new design.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately summarizes the three main changes: showing alliance request+duration icons, showing ally and teammate targets, and optimization improvements.
Description check	✅ Passed	The description clearly relates to the changeset, detailing all major changes including nuke icons in replay mode, alliance calculations, target calculations, and allocation removals.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

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

Actionable comments posted: 2

🤖 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 `@src/client/render/frame/derive/PlayerStatus.ts`:
- Line 98: The code currently sets target = opts.isTransitiveTarget ?
opts.isTransitiveTarget(sid) : false which forces target to false when
isTransitiveTarget is omitted; change this to use the local-player direct-target
check as the default: set target = opts.isTransitiveTarget ?
opts.isTransitiveTarget(sid) : localPlayer.targets.includes(sid) so PlayerStatus
(and the variable target) preserves direct-target behavior when
opts.isTransitiveTarget is not provided.
- Around line 95-125: The nuke scanning loop should run regardless of the
local-player guard so replay mode and self-checks can set
nukeActive/nukeTargetsMe; move the units iteration that sets nukeActive and
nukeTargetsMe (the loop over units that checks u.ownerID === sid, u.isActive,
NUKE_ACTIVE_TYPES.has(u.unitType) and inspects tileState[u.targetTile] against
localPlayerSmallID) out of the "if (localPlayer !== undefined && sid !==
localPlayerSmallID)" block (or duplicate the nuke detection logic before that
guard) while keeping alliance/allianceReq/embargo/target logic inside the guard;
ensure you still respect tileState and localPlayerSmallID checks when computing
nukeTargetsMe.

🪄 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: 45748b66-14df-4053-893f-63ed4c56b08c

📥 Commits

Reviewing files that changed from the base of the PR and between 0ace428 and 418d50e.

📒 Files selected for processing (4)

• src/client/render/frame/derive/PlayerStatus.ts
• src/client/view/GameView.ts
• src/client/view/PlayerView.ts
• tests/client/render/frame/derive/player-status.test.ts

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

tests/client/render/frame/derive/player-status.test.ts (1)

86-336: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift

Use setup() helper for this test suite.

These tests still build state by hand and call computePlayerStatus directly. Please migrate this suite to setup() and exercise core simulation as required by the test conventions.

As per coding guidelines tests/**/*.test.ts: Use the setup() helper from tests/util/Setup.ts to create test game instances and exercise core simulation directly.

🤖 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/client/render/frame/derive/player-status.test.ts` around lines 86 -
336, The test suite currently constructs state manually and calls
computePlayerStatus directly; migrate it to use the tests/util/Setup.ts setup()
helper so tests drive the core simulation instead. Replace direct
playersMap/ps/unitsMap/unit construction with setup() to create a Game instance,
add players/units via the setup APIs, advance the simulation as needed, then
read computed player status via the same public path that computePlayerStatus
uses in production; keep assertions the same but reference the game-derived
status instead of the direct computePlayerStatus call. Ensure cases that need
tileState or localPlayerSmallID set those via the setup API or simulation
helpers (e.g., placing tiles, setting local player) and still test flags like
crown, traitor, disconnected, nukeActive, nukeTargetsMe, alliance/target/embargo
accordingly.

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

Outside diff comments:
In `@tests/client/render/frame/derive/player-status.test.ts`:
- Around line 86-336: The test suite currently constructs state manually and
calls computePlayerStatus directly; migrate it to use the tests/util/Setup.ts
setup() helper so tests drive the core simulation instead. Replace direct
playersMap/ps/unitsMap/unit construction with setup() to create a Game instance,
add players/units via the setup APIs, advance the simulation as needed, then
read computed player status via the same public path that computePlayerStatus
uses in production; keep assertions the same but reference the game-derived
status instead of the direct computePlayerStatus call. Ensure cases that need
tileState or localPlayerSmallID set those via the setup API or simulation
helpers (e.g., placing tiles, setting local player) and still test flags like
crown, traitor, disconnected, nukeActive, nukeTargetsMe, alliance/target/embargo
accordingly.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 24797d10-1987-4be1-82d7-f4a683a67a28

📥 Commits

Reviewing files that changed from the base of the PR and between 418d50e and d1d25f8.

📒 Files selected for processing (2)

• src/client/render/frame/derive/PlayerStatus.ts
• tests/client/render/frame/derive/player-status.test.ts

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

• src/client/render/frame/derive/PlayerStatus.ts