Conversation
Contributor
WalkthroughPR introduces ActionableEvents, a new UI layer for actionable in-game notifications with alliance request/renewal prompts. The game's displayMessage infrastructure is extended to carry unitID and focusPlayerID context. EventsDisplay is simplified by removing action buttons and complex lifecycle logic. ControlPanel gains a gold-gain pop animation. All message emission sites are updated to use the new parameters. ChangesActionableEvents Layer & Message System Expansion
Sequence Diagram(s)sequenceDiagram
participant GameRenderer
participant GameView
participant ActionableEvents
participant EventBus
GameRenderer->>ActionableEvents: wire eventBus, game, uiState
GameView->>ActionableEvents: emit DisplayEvent / AllianceRequest / BrokeAlliance / AllianceExtension
ActionableEvents->>ActionableEvents: onDisplayMessageEvent() / tick() expire/add prompts
ActionableEvents->>EventBus: emit TransportIntentEvent (accept/reject/renew)
ActionableEvents->>GameRenderer: render() sorted actionable cards
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 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
🤖 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/GameRenderer.ts`:
- Around line 133-141: The code currently logs if
document.querySelector("actionable-events") is not an instance of
ActionableEvents but then continues to dereference actionableEvents (setting
actionableEvents.eventBus, .game, .uiState and later inserting layers), causing
a crash if missing; update the guard around actionableEvents in GameRenderer
(the variable actionableEvents and type ActionableEvents) to perform an early
return or throw after the console.error so no property writes or layer insertion
run when actionableEvents is falsy/incorrect, and apply the same guarded pattern
to the other occurrence around lines where layer insertion happens (the second
actionableEvents usage) to ensure eventBus, game and uiState are only assigned
when actionableEvents is a valid ActionableEvents instance.
In `@src/core/execution/TradeShipExecution.ts`:
- Around line 192-193: Add a core simulation unit test that asserts both source
and destination port gold-bonus events are emitted when TradeShipExecution runs:
locate where TradeShipExecution calls srcPort.owner().addGold(gold,
this.srcPort.tile()) and _dstPort.owner().addGold(gold, this._dstPort.tile())
and write a test in the core simulation test suite that (1) sets up a trade
between two ports, (2) spies or subscribes to the core event/bonus emitter for
the source owner and destination owner, (3) executes the TradeShipExecution, and
(4) asserts that each owner received a gold-add event including the correct gold
amount and that the source event includes the tile context (this.srcPort.tile())
while the destination event includes this._dstPort.tile().
🪄 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: 66197d95-9ab2-47fd-a2c2-0c1c0857e646
📒 Files selected for processing (14)
index.htmlsrc/client/Utils.tssrc/client/graphics/GameRenderer.tssrc/client/graphics/layers/ActionableEvents.tssrc/client/graphics/layers/ControlPanel.tssrc/client/graphics/layers/EventsDisplay.tssrc/core/execution/NukeExecution.tssrc/core/execution/TradeShipExecution.tssrc/core/game/Game.tssrc/core/game/GameImpl.tssrc/core/game/GameUpdates.tssrc/core/game/PlayerImpl.tssrc/core/game/UnitImpl.tstests/client/graphics/layers/ActionableEventsAlliance.test.ts
Comment on lines
+133
to
+141
| const actionableEvents = document.querySelector( | ||
| "actionable-events", | ||
| ) as ActionableEvents; | ||
| if (!(actionableEvents instanceof ActionableEvents)) { | ||
| console.error("actionable events not found"); | ||
| } | ||
| actionableEvents.eventBus = eventBus; | ||
| actionableEvents.game = game; | ||
| actionableEvents.uiState = uiState; |
Contributor
There was a problem hiding this comment.
Guard actionableEvents before property writes and layer insertion.
The null/type check only logs, then Line 139 still dereferences the value. If the element is missing, renderer init crashes.
Suggested fix
- const actionableEvents = document.querySelector(
+ const actionableEvents = document.querySelector(
"actionable-events",
- ) as ActionableEvents;
- if (!(actionableEvents instanceof ActionableEvents)) {
+ ) as ActionableEvents | null;
+ if (!(actionableEvents instanceof ActionableEvents)) {
console.error("actionable events not found");
- }
- actionableEvents.eventBus = eventBus;
- actionableEvents.game = game;
- actionableEvents.uiState = uiState;
+ } else {
+ actionableEvents.eventBus = eventBus;
+ actionableEvents.game = game;
+ actionableEvents.uiState = uiState;
+ }
@@
- actionableEvents,
+ ...(actionableEvents instanceof ActionableEvents
+ ? [actionableEvents]
+ : []),Also applies to: 304-305
🤖 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/GameRenderer.ts` around lines 133 - 141, The code
currently logs if document.querySelector("actionable-events") is not an instance
of ActionableEvents but then continues to dereference actionableEvents (setting
actionableEvents.eventBus, .game, .uiState and later inserting layers), causing
a crash if missing; update the guard around actionableEvents in GameRenderer
(the variable actionableEvents and type ActionableEvents) to perform an early
return or throw after the console.error so no property writes or layer insertion
run when actionableEvents is falsy/incorrect, and apply the same guarded pattern
to the other occurrence around lines where layer insertion happens (the second
actionableEvents usage) to ensure eventBus, game and uiState are only assigned
when actionableEvents is a valid ActionableEvents instance.
Comment on lines
+192
to
193
| this.srcPort.owner().addGold(gold, this.srcPort.tile()); | ||
| this._dstPort.owner().addGold(gold, this._dstPort.tile()); |
Contributor
There was a problem hiding this comment.
Add a core test for source-port gold bonus emission.
Line 192 changes core event output (source-side gold now includes a tile context). Please add a core simulation test that verifies bonus/event emission for both source and destination ports.
As per coding guidelines, "All changes to src/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/execution/TradeShipExecution.ts` around lines 192 - 193, Add a core
simulation unit test that asserts both source and destination port gold-bonus
events are emitted when TradeShipExecution runs: locate where TradeShipExecution
calls srcPort.owner().addGold(gold, this.srcPort.tile()) and
_dstPort.owner().addGold(gold, this._dstPort.tile()) and write a test in the
core simulation test suite that (1) sets up a trade between two ports, (2) spies
or subscribes to the core event/bonus emitter for the source owner and
destination owner, (3) executes the TradeShipExecution, and (4) asserts that
each owner received a gold-add event including the correct gold amount and that
the source event includes the tile context (this.srcPort.tile()) while the
destination event includes this._dstPort.tile().
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: 3
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/UnitImpl.ts (1)
197-232:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd core tests for new
unitIDmessage payload fields.
UnitImplbehavior changed in core by addingthis.id()to threedisplayMessage(...)paths, but no matchingUnitImpltests are included here to lock this contract. Please add tests that assertunitIDis present for capture (both sender/receiver messages) and destroy notifications.As per coding guidelines: “
src/core/... All changes tosrc/core/must include tests”.Also applies to: 326-342
🤖 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/core/game/GameImpl.ts`:
- Around line 936-939: The code computes focusID by calling
this.player(focusPlayerID).smallID() without ensuring the player exists; use the
existing hasPlayer() guard to avoid the player() call when the ID is invalid.
Change the ternary to first check focusPlayerID !== undefined &&
this.hasPlayer(focusPlayerID) and only then call
this.player(focusPlayerID).smallID(); otherwise set focusID to undefined (or
handle per existing semantics), so GameImpl.player() cannot be invoked with a
missing PlayerID.
- Around line 929-930: Tests are missing for the new optional parameters on
GameImpl.displayMessage: add unit tests that call GameImpl.displayMessage (or
helper wrapper used in tests) and assert the produced GameUpdate with type
GameUpdateType.DisplayEvent contains the passed unitID as-is and the
focusPlayerID converted to smallID in the payload's DisplayEvent, and also
assert behavior when each optional parameter is undefined (i.e., absent from the
payload). Target the GameImpl.displayMessage invocation paths updated around
DisplayEvent and verify the update payload fields for unitID and focusPlayerID
(including conversion logic) in positive and undefined cases.
In `@tests/core/executions/NukeExecution.test.ts`:
- Around line 188-200: The test currently constructs a NukeExecution for
UnitType.MIRVWarhead with a null launch ref, so the detonation path isn't
exercised; update the test to create a valid missile silo (or other valid launch
unit) for 'player', obtain its reference (use game.ref(...) or the created
unit's ref) and pass that ref into new NukeExecution(UnitType.MIRVWarhead,
player, game.ref(50, 50), <siloRef>) so the MIRV launch code in NukeExecution is
executed, then run executeTicks(game, 200) and assert that game.displayMessage
was not called with NUKE_DETONATED. Ensure you reference NukeExecution,
UnitType.MIRVWarhead, and game.displayMessage when making these changes.
🪄 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: 89dee3df-72e9-4227-a64f-8adc37b0882a
📒 Files selected for processing (16)
index.htmlresources/lang/en.jsonsrc/client/Utils.tssrc/client/graphics/GameRenderer.tssrc/client/graphics/layers/ActionableEvents.tssrc/client/graphics/layers/ControlPanel.tssrc/client/graphics/layers/EventsDisplay.tssrc/core/execution/NukeExecution.tssrc/core/execution/TradeShipExecution.tssrc/core/game/Game.tssrc/core/game/GameImpl.tssrc/core/game/GameUpdates.tssrc/core/game/PlayerImpl.tssrc/core/game/UnitImpl.tstests/client/graphics/layers/ActionableEventsAlliance.test.tstests/core/executions/NukeExecution.test.ts
✅ Files skipped from review due to trivial changes (1)
- resources/lang/en.json
🚧 Files skipped from review as they are similar to previous changes (12)
- src/core/execution/TradeShipExecution.ts
- src/core/game/GameUpdates.ts
- src/client/Utils.ts
- index.html
- src/client/graphics/GameRenderer.ts
- src/core/game/Game.ts
- tests/client/graphics/layers/ActionableEventsAlliance.test.ts
- src/core/execution/NukeExecution.ts
- src/core/game/PlayerImpl.ts
- src/client/graphics/layers/ControlPanel.ts
- src/client/graphics/layers/ActionableEvents.ts
- src/client/graphics/layers/EventsDisplay.ts
Comment on lines
+929
to
+930
| unitID?: number, | ||
| focusPlayerID?: PlayerID, |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Search for tests covering the new unitID and focusPlayerID parameters in displayMessage
# Search for tests that verify unitID in DisplayEvent updates
rg -n 'DisplayEvent.*unitID|unitID.*DisplayEvent' --type=ts --glob='*test*' -C3
# Search for tests that verify focusPlayerID in DisplayEvent updates
rg -n 'DisplayEvent.*focusPlayerID|focusPlayerID.*DisplayEvent' --type=ts --glob='*test*' -C3
# Search for displayMessage calls with the new parameters
rg -n 'displayMessage\(' --type=ts --glob='*test*' -A5 | rg -C2 'unitID|focusPlayerID'Repository: openfrontio/OpenFrontIO
Length of output: 49
🏁 Script executed:
cat -n src/core/game/GameImpl.ts | sed -n '920,950p'Repository: openfrontio/OpenFrontIO
Length of output: 1027
🏁 Script executed:
fd -t f "test\|spec" src/core/game/ --type=fRepository: openfrontio/OpenFrontIO
Length of output: 49
🏁 Script executed:
rg -l "displayMessage" src/core/ --type=ts --glob="*test*"Repository: openfrontio/OpenFrontIO
Length of output: 49
🏁 Script executed:
rg -n "DisplayEvent" src/core/game/ --type=ts -A2 -B2 | head -50Repository: openfrontio/OpenFrontIO
Length of output: 878
🏁 Script executed:
rg -n "interface DisplayMessageUpdate" src/core/game/GameUpdates.ts -A15Repository: openfrontio/OpenFrontIO
Length of output: 544
🏁 Script executed:
fd "test\.ts" src/core/ --type fRepository: openfrontio/OpenFrontIO
Length of output: 49
🏁 Script executed:
fd "NukeExecution" src/core/ --type fRepository: openfrontio/OpenFrontIO
Length of output: 103
🏁 Script executed:
fd "NukeExecution.test" src/core/ --type fRepository: openfrontio/OpenFrontIO
Length of output: 49
🏁 Script executed:
rg -l "displayMessage" src/core/ --type tsRepository: openfrontio/OpenFrontIO
Length of output: 570
🏁 Script executed:
rg -n "displayMessage.*unitID|displayMessage.*focusPlayerID" src/core/ --type tsRepository: openfrontio/OpenFrontIO
Length of output: 49
🏁 Script executed:
fd -e test.ts src/ --type fRepository: openfrontio/OpenFrontIO
Length of output: 389
🏁 Script executed:
cat -n src/core/execution/NukeExecution.ts | grep -A5 -B5 "displayMessage"Repository: openfrontio/OpenFrontIO
Length of output: 558
🏁 Script executed:
git log --oneline -n 20 -- src/core/game/GameImpl.tsRepository: openfrontio/OpenFrontIO
Length of output: 1416
Add tests for the new unitID and focusPlayerID parameters in displayMessage.
The code correctly adds these parameters to the displayMessage method and passes them through to the DisplayEvent update. However, per the coding guidelines, all changes to src/core/ must include tests. Tests are missing to verify:
- The
unitIDparameter is included in theGameUpdateType.DisplayEventupdate payload - The
focusPlayerIDparameter is converted tosmallIDand included in the update payload - These optional parameters work correctly when undefined
Also applies to: 936-939, 947-948
🤖 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 929 - 930, Tests are missing for the
new optional parameters on GameImpl.displayMessage: add unit tests that call
GameImpl.displayMessage (or helper wrapper used in tests) and assert the
produced GameUpdate with type GameUpdateType.DisplayEvent contains the passed
unitID as-is and the focusPlayerID converted to smallID in the payload's
DisplayEvent, and also assert behavior when each optional parameter is undefined
(i.e., absent from the payload). Target the GameImpl.displayMessage invocation
paths updated around DisplayEvent and verify the update payload fields for
unitID and focusPlayerID (including conversion logic) in positive and undefined
cases.
Comment on lines
+936
to
+939
| const focusID = | ||
| focusPlayerID !== undefined | ||
| ? this.player(focusPlayerID).smallID() | ||
| : undefined; |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add validation for focusPlayerID to prevent runtime errors.
The code calls this.player(focusPlayerID).smallID() without checking if the player exists. The player() method throws an error if the PlayerID is not found (lines 620-625), which would crash the game simulation if an invalid ID is passed.
Consider adding validation using the existing hasPlayer() method:
🛡️ Proposed validation
const focusID =
focusPlayerID !== undefined
- ? this.player(focusPlayerID).smallID()
+ ? (this.hasPlayer(focusPlayerID)
+ ? this.player(focusPlayerID).smallID()
+ : undefined)
: undefined;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const focusID = | |
| focusPlayerID !== undefined | |
| ? this.player(focusPlayerID).smallID() | |
| : undefined; | |
| const focusID = | |
| focusPlayerID !== undefined | |
| ? (this.hasPlayer(focusPlayerID) | |
| ? this.player(focusPlayerID).smallID() | |
| : undefined) | |
| : undefined; |
🤖 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 936 - 939, The code computes focusID
by calling this.player(focusPlayerID).smallID() without ensuring the player
exists; use the existing hasPlayer() guard to avoid the player() call when the
ID is invalid. Change the ternary to first check focusPlayerID !== undefined &&
this.hasPlayer(focusPlayerID) and only then call
this.player(focusPlayerID).smallID(); otherwise set focusID to undefined (or
handle per existing semantics), so GameImpl.player() cannot be invoked with a
missing PlayerID.
Comment on lines
+188
to
+200
| test("MIRVWarhead detonation does NOT emit NUKE_DETONATED", () => { | ||
| for (let x = 48; x < 53; x++) { | ||
| for (let y = 48; y < 53; y++) { | ||
| otherPlayer.conquer(game.ref(x, y)); | ||
| } | ||
| } | ||
|
|
||
| const displayMessageSpy = vi.spyOn(game, "displayMessage"); | ||
|
|
||
| game.addExecution( | ||
| new NukeExecution(UnitType.MIRVWarhead, player, game.ref(50, 50), null), | ||
| ); | ||
| executeTicks(game, 200); |
Contributor
There was a problem hiding this comment.
MIRV negative case can pass without exercising detonation path
This test does not create a missile silo and passes null launch ref, so NUKE_DETONATED absence may be true even if MIRV logic is broken. Make MIRV launch under valid conditions, then assert no detonation message.
Suggested fix
test("MIRVWarhead detonation does NOT emit NUKE_DETONATED", () => {
+ player.buildUnit(UnitType.MissileSilo, game.ref(1, 1), {});
for (let x = 48; x < 53; x++) {
for (let y = 48; y < 53; y++) {
otherPlayer.conquer(game.ref(x, y));
}
}
const displayMessageSpy = vi.spyOn(game, "displayMessage");
game.addExecution(
- new NukeExecution(UnitType.MIRVWarhead, player, game.ref(50, 50), null),
+ new NukeExecution(
+ UnitType.MIRVWarhead,
+ player,
+ game.ref(50, 50),
+ game.ref(1, 1),
+ ),
);
executeTicks(game, 200);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| test("MIRVWarhead detonation does NOT emit NUKE_DETONATED", () => { | |
| for (let x = 48; x < 53; x++) { | |
| for (let y = 48; y < 53; y++) { | |
| otherPlayer.conquer(game.ref(x, y)); | |
| } | |
| } | |
| const displayMessageSpy = vi.spyOn(game, "displayMessage"); | |
| game.addExecution( | |
| new NukeExecution(UnitType.MIRVWarhead, player, game.ref(50, 50), null), | |
| ); | |
| executeTicks(game, 200); | |
| test("MIRVWarhead detonation does NOT emit NUKE_DETONATED", () => { | |
| player.buildUnit(UnitType.MissileSilo, game.ref(1, 1), {}); | |
| for (let x = 48; x < 53; x++) { | |
| for (let y = 48; y < 53; y++) { | |
| otherPlayer.conquer(game.ref(x, y)); | |
| } | |
| } | |
| const displayMessageSpy = vi.spyOn(game, "displayMessage"); | |
| game.addExecution( | |
| new NukeExecution( | |
| UnitType.MIRVWarhead, | |
| player, | |
| game.ref(50, 50), | |
| game.ref(1, 1), | |
| ), | |
| ); | |
| executeTicks(game, 200); |
🤖 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/core/executions/NukeExecution.test.ts` around lines 188 - 200, The test
currently constructs a NukeExecution for UnitType.MIRVWarhead with a null launch
ref, so the detonation path isn't exercised; update the test to create a valid
missile silo (or other valid launch unit) for 'player', obtain its reference
(use game.ref(...) or the created unit's ref) and pass that ref into new
NukeExecution(UnitType.MIRVWarhead, player, game.ref(50, 50), <siloRef>) so the
MIRV launch code in NukeExecution is executed, then run executeTicks(game, 200)
and assert that game.displayMessage was not called with NUKE_DETONATED. Ensure
you reference NukeExecution, UnitType.MIRVWarhead, and game.displayMessage when
making these changes.
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
1 participant
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.
Resolves #3910
Description:
Describe the PR.
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
DISCORD_USERNAME