Fix/translate incoming unit notifications#3856
Fix/translate incoming unit notifications#3856mike-s-zaugg wants to merge 2 commits intoopenfrontio:mainfrom
Conversation
Show player info (resources, alliances, stats) when clicking on a player during replay playback. Action buttons are hidden as they are not applicable in replay mode.
Atom bomb, hydrogen bomb, MIRV, and naval invasion inbound alerts were hardcoded English strings. Wire them through the existing translation system so they respect the player's language setting. Changes: - Add params field to UnitIncomingUpdate so substitution values (player name, troop count) can be passed alongside the key - Translate in onUnitIncomingEvent using the same startsWith(events_display.) guard already used for DisplayMessageUpdate - Add four new keys to en.json: atom_bomb_inbound, hydrogen_bomb_inbound, naval_invasion_inbound, mirv_inbound - Replace hardcoded template literals in NukeExecution, TransportShipExecution, and MIRVExecution with key + params Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
WalkthroughThis PR introduces localized incoming unit notifications with parameterized messages and adds replay-mode support to the PlayerPanel. Execution classes now call a revised displayIncomingUnit API that accepts translation keys and payload objects instead of hardcoded strings. PlayerPanel is enhanced to render read-only UI in replay mode even when the local player is unavailable. ChangesLocalized Incoming Unit Messages with Parameters
Replay Mode Support in PlayerPanel
sequenceDiagram
participant Exec as Execution<br/>(MIRVExecution,<br/>NukeExecution, etc.)
participant Game as Game API<br/>(displayIncomingUnit)
participant GameImpl as GameImpl
participant Updates as GameUpdates<br/>(UnitIncoming)
participant Display as EventsDisplay
participant Lang as Language File
Exec->>Game: displayIncomingUnit(id, "events_display.mirv_inbound",<br/>type, targetId, {name: "Alice"})
Game->>GameImpl: (forward call)
GameImpl->>Updates: Create UnitIncoming with<br/>translationKey + params
Updates->>Display: Update received
Display->>Lang: Fetch translation for<br/>"events_display.mirv_inbound"
Lang-->>Display: Return template
Display->>Display: Substitute {name} with<br/>payload.name
Display->>Display: Render localized message<br/>"Alice - MIRV INBOUND"
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes The PR spans multiple files with changes to both core game logic (API threading) and execution/display layers, but the underlying pattern is consistent: replacing hardcoded strings with translation keys and payloads. The replay mode logic in PlayerPanel introduces conditional rendering that requires careful verification of all branches. No complex interdependencies within each cohort. Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/core/game/GameImpl.ts (1)
945-961:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftAdd tests for the new core update field propagation.
displayIncomingUnitnow changes core update payload shape by addingparams. Please add tests that verifyUnitIncomingupdates include and preserveparamsthrough this path (and still work whenparamsis omitted).As per coding guidelines: "
src/core/**/*.ts: All changes tosrc/core/must include tests".🤖 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 `@src/core/game/GameImpl.ts` around lines 945 - 961, Add unit tests for the new params propagation in displayIncomingUnit: write tests that call GameImpl.displayIncomingUnit (or simulate the public API that triggers it) and assert that the resulting update emitted via addUpdate with type GameUpdateType.UnitIncoming contains the params object exactly as passed and that when params is omitted the update either has params undefined or omitted per the payload contract; ensure tests cover both object-preservation (deep equality) and absence case, and assert the playerID uses smallID as before. Use the existing test harness for GameImpl updates to capture addUpdate outputs and locate tests near other GameUpdateType.UnitIncoming specs.
🤖 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/graphics/layers/PlayerPanel.ts`:
- Around line 881-884: The replay-path sets viewer = my ?? other causing
perspective widgets to compare a player to themselves; update the calls and
helpers so they no-op when there is no real "me": change renderIdentityRow and
renderRelationPillIfNation signatures to accept my: PlayerView | null and
immediately return/do nothing if my is null, and similarly guard renderStats (or
its internal call to other.hasEmbargoAgainst(other)) so it doesn't check
embargoes when my is null; then either pass my (which may be null) into those
functions or only call them when my != null to ensure replay rendering doesn't
show self-relations or incorrect embargo/trade status.
---
Outside diff comments:
In `@src/core/game/GameImpl.ts`:
- Around line 945-961: Add unit tests for the new params propagation in
displayIncomingUnit: write tests that call GameImpl.displayIncomingUnit (or
simulate the public API that triggers it) and assert that the resulting update
emitted via addUpdate with type GameUpdateType.UnitIncoming contains the params
object exactly as passed and that when params is omitted the update either has
params undefined or omitted per the payload contract; ensure tests cover both
object-preservation (deep equality) and absence case, and assert the playerID
uses smallID as before. Use the existing test harness for GameImpl updates to
capture addUpdate outputs and locate tests near other
GameUpdateType.UnitIncoming specs.
🪄 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: 7bbdecd5-588f-4158-80d4-976120fdf00f
📒 Files selected for processing (9)
resources/lang/en.jsonsrc/client/graphics/layers/EventsDisplay.tssrc/client/graphics/layers/PlayerPanel.tssrc/core/execution/MIRVExecution.tssrc/core/execution/NukeExecution.tssrc/core/execution/TransportShipExecution.tssrc/core/game/Game.tssrc/core/game/GameImpl.tssrc/core/game/GameUpdates.ts
| // In replay mode myPlayer() is null; use other as stand-in so read-only rendering works | ||
| const viewer = my ?? other; | ||
| const myGoldNum = viewer.gold(); | ||
| const myTroopsNum = Number(viewer.troops()); |
There was a problem hiding this comment.
Heads-up: in replay mode viewer becomes other, so a couple of "perspective" widgets compare the player to themselves.
When my is null (replay), viewer = other. That value then flows into:
- Line 952 →
renderIdentityRow(other, viewer)→renderRelationPillIfNation(other, my=other)→ readsthis.otherProfile.relations?.[other.smallID()], i.e. the player's relation to themselves. You'll get a "Neutral" pill on every Nation in replay. - Line 1003 →
renderStats(other, viewer)→other.hasEmbargoAgainst(other), which is essentially always falsy, so the trading row will read "Active" for every player in replay regardless of the real embargo state.
The send-resource and moderation modals are safe because Actions is hidden in replay, so viewer never leaks there in practice. Still, the two display widgets above will show a small fib in replay mode.
Easiest fix: only render those perspective-dependent bits when there is a real "me", and let the helpers no-op otherwise. Something like:
♻️ Suggested tweak
- <div class="mb-1">${this.renderIdentityRow(other, viewer)}</div>
+ <div class="mb-1">${this.renderIdentityRow(other, my)}</div>
@@
- ${this.renderStats(other, viewer)}
+ ${my ? this.renderStats(other, my) : ""}…and let renderIdentityRow / renderRelationPillIfNation accept my: PlayerView | null and bail early when it is null. A typed union (PlayerView | null) keeps the signatures honest without any extra class plumbing.
🤖 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 `@src/client/graphics/layers/PlayerPanel.ts` around lines 881 - 884, The
replay-path sets viewer = my ?? other causing perspective widgets to compare a
player to themselves; update the calls and helpers so they no-op when there is
no real "me": change renderIdentityRow and renderRelationPillIfNation signatures
to accept my: PlayerView | null and immediately return/do nothing if my is null,
and similarly guard renderStats (or its internal call to
other.hasEmbargoAgainst(other)) so it doesn't check embargoes when my is null;
then either pass my (which may be null) into those functions or only call them
when my != null to ensure replay rendering doesn't show self-relations or
incorrect embargo/trade status.
github-project-automation
Bot
moved this from Triage
to Development
in OpenFront Release Management
1 participant
Description:
Incoming unit alert notifications (atom bomb, hydrogen bomb, MIRV, and naval invasion inbound) were hardcoded English strings, bypassing the translation system entirely. This fix wires them through the existing i18n pipeline.
Changes:
params?field toUnitIncomingUpdateso substitution values (player name, troop count) travel alongside the translation keystartsWith("events_display.")translation guard inonUnitIncomingEventthat already exists inonDisplayMessageEventen.json:atom_bomb_inbound,hydrogen_bomb_inbound,naval_invasion_inbound,mirv_inboundNukeExecution,TransportShipExecution, andMIRVExecutionwith key + paramsPlease complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
sxndrexe