Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR introduces a new ActionableEvents UI layer for alliance-related notifications, extends the displayMessage API with unit and focus context, refactors EventsDisplay into tiered rendering, animates gold gains in ControlPanel, updates domain code to emit messages with new parameters, and migrates tests. ChangesActionableEvents Layer & Message System Expansion
🎯 3 (Moderate) | ⏱️ ~25 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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
+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
Contributor
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 (2)
src/client/hud/layers/ActionableEvents.ts (1)
1-326:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix formatting to unblock CI.
Prettier is failing for this file in the pipeline; please run the formatter before merge.
🤖 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/hud/layers/ActionableEvents.ts` around lines 1 - 326, Prettier/formatting errors are causing CI to fail; run the project's formatter (Prettier) or apply the repo's eslint --fix/format settings to this file so it matches the project's style. Specifically reformat the ActionableEvents class and its methods (tick, render, checkForAllianceExpirations, onAllianceRequestEvent, onAllianceExtensionEvent, onBrokeAllianceEvent, addEvent/removeEvent) so spacing, indentation and line breaks conform to the repo Prettier configuration, then re-run the linter/CI to confirm the formatting issue is resolved.src/client/hud/layers/EventsDisplay.ts (1)
1-1:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRun Prettier to fix formatting issues.
Pipeline reports code style issues. Run
npx prettier --write src/client/hud/layers/EventsDisplay.tsbefore merge.🤖 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/hud/layers/EventsDisplay.ts` at line 1, Formatting in EventsDisplay.ts is out of sync with project Prettier settings; run the formatter and commit the result. Run npx prettier --write on the file (or your repo's configured formatter) and ensure the import line and overall file containing the EventsDisplay class/component (e.g., the import { html, LitElement } line and the EventsDisplay class definition) are reformatted, then stage and commit the updated formatting changes.
🧹 Nitpick comments (2)
src/client/hud/layers/ControlPanel.ts (1)
460-474: 💤 Low valueInline styles in light DOM are acceptable here.
Since shadow DOM is disabled for Tailwind compatibility, this
<style>block lives in the component's template and gets diffed normally by Lit (no duplication). The class namegold-gain-popis specific enough to avoid conflicts.One optional improvement: if this animation is reused elsewhere later, consider extracting it to a shared CSS file.
,🤖 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/hud/layers/ControlPanel.ts` around lines 460 - 474, The inline <style> block defining the gold-gain-pop keyframes and .gold-gain-pop class is acceptable in ControlPanel.ts, but if you want reuse or avoid duplication extract the animation to a shared CSS file: create a shared stylesheet (e.g., animations.css or shared-animations.ts) containing the `@keyframes` gold-gain-pop and .gold-gain-pop rule, export or import that stylesheet into ControlPanel.ts and replace the inline <style> with an import or reference; ensure the selector name (gold-gain-pop) and keyframe name remain unchanged so existing usage in ControlPanel (class="gold-gain-pop") continues to work.src/client/hud/layers/EventsDisplay.ts (1)
208-208: 💤 Low valueRedundant
requestUpdate()call on every tick.Line 205 already triggers a re-render when events change, and
_isVisiblechanges on lines 162/169 do the same. This unconditional call forces a re-render every tick (~10 times/second) even when nothing changed.♻️ Suggested fix
if (this.events.length !== remainingEvents.length) { this.events = remainingEvents; this.requestUpdate(); } - - this.requestUpdate(); }🤖 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/hud/layers/EventsDisplay.ts` at line 208, The unconditional this.requestUpdate() in the EventsDisplay tick handler causes unnecessary re-renders; remove that call and rely on the existing update paths that already call requestUpdate() when events change (the events update mechanism) or when _isVisible is toggled, or guard it so it only calls requestUpdate() when the events list or _isVisible actually changed. Locate the tick handler inside the EventsDisplay class (where requestUpdate() is called each tick) and either delete that call or replace it with a boolean check against the current events/visibility state (use the _isVisible property and whatever internal events collection or last-known-events snapshot the class uses) so renders only happen on real state changes.
🤖 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/hud/layers/ActionableEvents.ts`:
- Around line 184-188: The loop that cleans stale alliances (iterating
this.alliancesCheckedAt and calling removeAllianceRenewalEvents(allianceId))
doesn't call requestUpdate(), so removed renewal prompts remain visible until
another update; update the cleanup paths (the loop at the top of
ActionableEvents and the similar code around lines 76-84) to call
this.requestUpdate() after deletions (or call requestUpdate() inside
removeAllianceRenewalEvents when it actually mutates this.events) so the
component re-renders immediately when alliance renewal events are removed.
---
Outside diff comments:
In `@src/client/hud/layers/ActionableEvents.ts`:
- Around line 1-326: Prettier/formatting errors are causing CI to fail; run the
project's formatter (Prettier) or apply the repo's eslint --fix/format settings
to this file so it matches the project's style. Specifically reformat the
ActionableEvents class and its methods (tick, render,
checkForAllianceExpirations, onAllianceRequestEvent, onAllianceExtensionEvent,
onBrokeAllianceEvent, addEvent/removeEvent) so spacing, indentation and line
breaks conform to the repo Prettier configuration, then re-run the linter/CI to
confirm the formatting issue is resolved.
In `@src/client/hud/layers/EventsDisplay.ts`:
- Line 1: Formatting in EventsDisplay.ts is out of sync with project Prettier
settings; run the formatter and commit the result. Run npx prettier --write on
the file (or your repo's configured formatter) and ensure the import line and
overall file containing the EventsDisplay class/component (e.g., the import {
html, LitElement } line and the EventsDisplay class definition) are reformatted,
then stage and commit the updated formatting changes.
---
Nitpick comments:
In `@src/client/hud/layers/ControlPanel.ts`:
- Around line 460-474: The inline <style> block defining the gold-gain-pop
keyframes and .gold-gain-pop class is acceptable in ControlPanel.ts, but if you
want reuse or avoid duplication extract the animation to a shared CSS file:
create a shared stylesheet (e.g., animations.css or shared-animations.ts)
containing the `@keyframes` gold-gain-pop and .gold-gain-pop rule, export or
import that stylesheet into ControlPanel.ts and replace the inline <style> with
an import or reference; ensure the selector name (gold-gain-pop) and keyframe
name remain unchanged so existing usage in ControlPanel (class="gold-gain-pop")
continues to work.
In `@src/client/hud/layers/EventsDisplay.ts`:
- Line 208: The unconditional this.requestUpdate() in the EventsDisplay tick
handler causes unnecessary re-renders; remove that call and rely on the existing
update paths that already call requestUpdate() when events change (the events
update mechanism) or when _isVisible is toggled, or guard it so it only calls
requestUpdate() when the events list or _isVisible actually changed. Locate the
tick handler inside the EventsDisplay class (where requestUpdate() is called
each tick) and either delete that call or replace it with a boolean check
against the current events/visibility state (use the _isVisible property and
whatever internal events collection or last-known-events snapshot the class
uses) so renders only happen on real state 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: 2a3ef376-d805-4a43-86ab-2420b74f9d6d
📒 Files selected for processing (7)
index.htmlresources/lang/en.jsonsrc/client/Utils.tssrc/client/hud/GameRenderer.tssrc/client/hud/layers/ActionableEvents.tssrc/client/hud/layers/ControlPanel.tssrc/client/hud/layers/EventsDisplay.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- resources/lang/en.json
- index.html
Comment on lines
+184
to
+188
| for (const [allianceId] of this.alliancesCheckedAt) { | ||
| if (!currentAllianceIds.has(allianceId)) { | ||
| this.removeAllianceRenewalEvents(allianceId); | ||
| this.alliancesCheckedAt.delete(allianceId); | ||
| } |
Contributor
There was a problem hiding this comment.
Trigger a re-render when stale alliance prompts are removed.
At Line 186, cleanup can remove entries from this.events, but this path does not call requestUpdate(). A removed renewal prompt can stay visible until another unrelated update occurs.
Proposed fix
for (const [allianceId] of this.alliancesCheckedAt) {
if (!currentAllianceIds.has(allianceId)) {
+ const before = this.events.length;
this.removeAllianceRenewalEvents(allianceId);
+ if (this.events.length !== before) {
+ this.requestUpdate();
+ }
this.alliancesCheckedAt.delete(allianceId);
}
}Also applies to: 76-84
🧰 Tools
🪛 GitHub Actions: 🧪 CI / 🎨 Prettier
[warning] Prettier reported formatting issues in this file.
🤖 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/hud/layers/ActionableEvents.ts` around lines 184 - 188, The loop
that cleans stale alliances (iterating this.alliancesCheckedAt and calling
removeAllianceRenewalEvents(allianceId)) doesn't call requestUpdate(), so
removed renewal prompts remain visible until another update; update the cleanup
paths (the loop at the top of ActionableEvents and the similar code around lines
76-84) to call this.requestUpdate() after deletions (or call requestUpdate()
inside removeAllianceRenewalEvents when it actually mutates this.events) so the
component re-renders immediately when alliance renewal events are removed.
Contributor
|
bro evan where are the damn screenshots?! 🤣 |
Contributor
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/UnitImpl.ts (1)
216-231:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd core tests for the new
unitIDmessage argument.Lines 222, 230, and 341 change core message payload shape, but this diff does not include core tests that assert
unitIDis emitted for capture and destroy flows.As per coding guidelines, "All changes to
src/core/must include tests."Also applies to: 335-342
🤖 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/UnitImpl.ts` around lines 216 - 231, Add core unit tests that assert the new "unitID" message argument is present for both capture and destroy flows: simulate the capture flow in UnitImpl and the destroy flow, spy/mock the mg.displayMessage calls for MessageType.UNIT_CAPTURED_BY_ENEMY and MessageType.CAPTURED_ENEMY_UNIT (and the destroy-related MessageType used in UnitImpl), and assert the message payload includes unitID equal to the unit's id() (and that other payload fields remain unchanged). Use UnitImpl to trigger the flows, intercept mg.displayMessage, and add assertions that the emitted payload contains unitID for both the capture and destroy scenarios.
🧹 Nitpick comments (2)
src/client/hud/layers/ActionableEvents.ts (1)
148-150: 💤 Low valueAdd a guard before accessing
alliance.other.If
alliance.otherrefers to a player that no longer exists (e.g., conquered),this.game.player()could throw. Consider usingplayerBySmallID()which returnsTerraNulliusfor invalid IDs, then check.isPlayer().Proposed defensive check
this.alliancesCheckedAt.set(alliance.id, this.game.ticks()); - const other = this.game.player(alliance.other) as PlayerView; + const otherResult = this.game.playerBySmallID(alliance.other); + if (!otherResult.isPlayer()) continue; + const other = otherResult as PlayerView;🤖 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/hud/layers/ActionableEvents.ts` around lines 148 - 150, The code currently calls this.game.player(alliance.other) which may throw if alliance.other refers to a non-existent player; replace that call with this.game.playerBySmallID(alliance.other) and guard the result by checking .isPlayer() (or handling TerraNullius) before using it—update the variable used (e.g., other) and bail out or skip processing when !other.isPlayer() to avoid exceptions when the referenced player was conquered or removed.src/client/hud/layers/ControlPanel.ts (1)
460-474: 💤 Low valueConsider extracting keyframes to a static style or global CSS.
The inline
<style>inrender()works, but since shadow DOM is disabled, these styles are global anyway. Moving them tostatic stylesor your global CSS file would be slightly cleaner — though not blocking since ControlPanel is a singleton.🤖 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/hud/layers/ControlPanel.ts` around lines 460 - 474, Move the inline <style> block out of ControlPanel.render(): remove the embedded `@keyframes` gold-gain-pop and .gold-gain-pop rules from the JSX returned by the render method and instead declare them either in the ControlPanel component's static styles property or in the global CSS file; update references to the animation class (gold-gain-pop) to remain unchanged so existing uses still apply. Locate the ControlPanel class and its render() method to remove the inline style and add the `@keyframes` gold-gain-pop and .gold-gain-pop rules to the component's static styles (or global stylesheet) so the animation stays available without injecting global styles in render().
🤖 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/PlayerImpl.ts`:
- Around line 855-866: The donation messages currently use the requested
variable `gold` which can over-report the actual transferred amount; update the
two this.mg.displayMessage calls in PlayerImpl (the sender and recipient
messages) to pass `removed` instead of `gold` for the numeric value and to call
renderNumber(removed) for the formatted display; locate the calls to
this.mg.displayMessage around the donation logic (referencing recipient.id(),
this.id(), this.displayName(), renderNumber) and replace the `gold` usages with
`removed`.
---
Outside diff comments:
In `@src/core/game/UnitImpl.ts`:
- Around line 216-231: Add core unit tests that assert the new "unitID" message
argument is present for both capture and destroy flows: simulate the capture
flow in UnitImpl and the destroy flow, spy/mock the mg.displayMessage calls for
MessageType.UNIT_CAPTURED_BY_ENEMY and MessageType.CAPTURED_ENEMY_UNIT (and the
destroy-related MessageType used in UnitImpl), and assert the message payload
includes unitID equal to the unit's id() (and that other payload fields remain
unchanged). Use UnitImpl to trigger the flows, intercept mg.displayMessage, and
add assertions that the emitted payload contains unitID for both the capture and
destroy scenarios.
---
Nitpick comments:
In `@src/client/hud/layers/ActionableEvents.ts`:
- Around line 148-150: The code currently calls this.game.player(alliance.other)
which may throw if alliance.other refers to a non-existent player; replace that
call with this.game.playerBySmallID(alliance.other) and guard the result by
checking .isPlayer() (or handling TerraNullius) before using it—update the
variable used (e.g., other) and bail out or skip processing when
!other.isPlayer() to avoid exceptions when the referenced player was conquered
or removed.
In `@src/client/hud/layers/ControlPanel.ts`:
- Around line 460-474: Move the inline <style> block out of
ControlPanel.render(): remove the embedded `@keyframes` gold-gain-pop and
.gold-gain-pop rules from the JSX returned by the render method and instead
declare them either in the ControlPanel component's static styles property or in
the global CSS file; update references to the animation class (gold-gain-pop) to
remain unchanged so existing uses still apply. Locate the ControlPanel class and
its render() method to remove the inline style and add the `@keyframes`
gold-gain-pop and .gold-gain-pop rules to the component's static styles (or
global stylesheet) so the animation stays available without injecting global
styles in render().
🪄 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: 7b2004ce-3c45-4b61-8362-060849d61d91
📒 Files selected for processing (16)
index.htmlresources/lang/en.jsonsrc/client/Utils.tssrc/client/hud/GameRenderer.tssrc/client/hud/layers/ActionableEvents.tssrc/client/hud/layers/ControlPanel.tssrc/client/hud/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
Contributor
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 (3)
src/core/game/UnitImpl.ts (2)
216-232:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd tests for unit ownership change messages with unit context.
The
displayMessagecalls insetOwnernow includethis.id()as theunitIDparameter, but no tests verify this behavior. As per coding guidelines, all changes tosrc/core/must include tests.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/game/UnitImpl.ts` around lines 216 - 232, Add unit tests that assert UnitImpl.setOwner triggers mg.displayMessage with the unitID equal to the unit's id (this.id()). Specifically, create tests that instantiate a UnitImpl, stub/mock its message gateway (mg) to capture calls, call UnitImpl.setOwner (or the method containing the shown displayMessage calls), and assert that mg.displayMessage is invoked for MessageType.UNIT_CAPTURED_BY_ENEMY and MessageType.CAPTURED_ENEMY_UNIT with the final argument equal to the unit's id; reference UnitImpl.setOwner, mg.displayMessage, MessageType.UNIT_CAPTURED_BY_ENEMY, and MessageType.CAPTURED_ENEMY_UNIT when locating the code under test.
335-342:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd tests for unit destruction messages with unit context.
The
displayMessagecall indisplayMessageOnDeletednow includesthis.id()as theunitIDparameter, but no tests verify this behavior. As per coding guidelines, all changes tosrc/core/must include tests.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/game/UnitImpl.ts` around lines 335 - 342, Add a unit test that asserts UnitImpl.displayMessageOnDeleted invokes MessageGroup.displayMessage with MessageType.UNIT_DESTROYED and passes the deleted unit's id as the unitID parameter: create a test that constructs a UnitImpl (or a minimal stub), spies/mocks its mg (MessageGroup) and owner(), calls displayMessageOnDeleted (or the code path that triggers the shown this.mg.displayMessage), and verifies the mock received arguments including the context { unit: this._type } and that the unitID argument equals the unit's id (this.id()). Use the existing test harness/mock utilities to target UnitImpl.displayMessageOnDeleted, MessageGroup.displayMessage, and MessageType.UNIT_DESTROYED.src/core/game/PlayerImpl.ts (1)
849-867:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd tests for donateGold display message focusPlayerID
donateGoldnow passesfocusPlayerIDintodisplayMessage(so the UI can focus the other player for the “sent” vs “received” gold events), but the existing donation tests only check gold amounts/success and do not assert anyDisplayEvent/displayMessagepayloads for these gold messages. Add a test that spies ongame.displayMessage(or checksGameUpdateType.DisplayEvent) and asserts the last positional arg (focusPlayerID) isrecipient.id()forSENT_GOLD_TO_PLAYERandthis.id()forRECEIVED_GOLD_FROM_PLAYER.🤖 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/PlayerImpl.ts` around lines 849 - 867, Add tests for donateGold to assert the displayMessage focusPlayerID is set correctly: spy on or inspect game.displayMessage / GameUpdateType.DisplayEvent after calling PlayerImpl.donateGold and assert the call for MessageType.SENT_GOLD_TO_PLAYER has its last positional argument equal to recipient.id() and the call for MessageType.RECEIVED_GOLD_FROM_PLAYER has its last positional argument equal to donor.id() (use the PlayerImpl instance's id()). Locate usages of donateGold, displayMessage, and MessageType.SENT_GOLD_TO_PLAYER / MessageType.RECEIVED_GOLD_FROM_PLAYER in the test suite, invoke donateGold with a valid amount, capture the display events, and assert the final argument (focusPlayerID) matches the expected player id for each message type.
♻️ Duplicate comments (3)
src/client/hud/GameRenderer.ts (1)
124-132:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winGuard actionableEvents property assignments after error check.
The code checks if
actionableEventsis anActionableEventsinstance and logs an error, but then continues to set properties on Line 130-132 regardless. If the element is missing or wrong type, this will crash.🛡️ Proposed fix
const actionableEvents = document.querySelector( "actionable-events", ) as ActionableEvents; if (!(actionableEvents instanceof ActionableEvents)) { console.error("actionable events not found"); + throw new Error("actionable-events element is required but not found"); } actionableEvents.eventBus = eventBus; actionableEvents.game = game; actionableEvents.uiState = uiState;🤖 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/hud/GameRenderer.ts` around lines 124 - 132, The code retrieves actionableEvents and logs an error if it's not an ActionableEvents but then unconditionally assigns actionableEvents.eventBus, .game, and .uiState; guard those assignments by only performing them when actionableEvents is an instance of ActionableEvents (or early-return/throw after the error). Update the block in GameRenderer.ts around actionableEvents so that after the instanceof check you either return/exit on error or wrap the three assignments (eventBus, game, uiState) inside the successful branch that confirms actionableEvents is an ActionableEvents.src/core/execution/TradeShipExecution.ts (1)
192-192:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftAdd core test for source-port gold bonus with tile context.
Line 192 now passes the source port's tile to
addGold, changing the event emission signature. As per coding guidelines, all changes tosrc/core/must include tests.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` at line 192, The change to TradeShipExecution using this.srcPort.tile() when calling srcPort.owner().addGold alters the addGold event signature and requires a core test that verifies the source-port gold bonus emits with the tile context; add a unit test in the core test suite that constructs the minimal game state (a srcPort with owner and tile, and a TradeShipExecution invocation or equivalent helper), triggers the gold-award path (calling TradeShipExecution.execute or the relevant method), and asserts that addGold was called/emitted with both the correct gold amount and the source port's tile; reference TradeShipExecution, srcPort.owner().addGold, and srcPort.tile() when locating where to add the test.src/client/hud/layers/ActionableEvents.ts (1)
184-189:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd
requestUpdate()after removing stale alliance renewal events.The cleanup loop removes alliance renewal events (Line 186) but doesn't call
requestUpdate(), so removed prompts stay visible until another unrelated update triggers a re-render.🔄 Proposed fix
for (const [allianceId] of this.alliancesCheckedAt) { if (!currentAllianceIds.has(allianceId)) { + const before = this.events.length; this.removeAllianceRenewalEvents(allianceId); this.alliancesCheckedAt.delete(allianceId); + if (this.events.length !== before) { + this.requestUpdate(); + } } }🤖 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/hud/layers/ActionableEvents.ts` around lines 184 - 189, The cleanup loop iterating this.alliancesCheckedAt removes stale alliance renewal events via removeAllianceRenewalEvents(allianceId) but doesn't trigger a re-render; after calling removeAllianceRenewalEvents(allianceId) and deleting from this.alliancesCheckedAt, call requestUpdate() so the UI reflects the removed prompts. Update the loop in ActionableEvents (the block referencing this.alliancesCheckedAt and removeAllianceRenewalEvents) to invoke requestUpdate() once a removal occurs (or once after the loop if multiple removals) to force an update.
🤖 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/hud/GameRenderer.ts`:
- Line 279: In GameRenderer, ensure the element returned by the querySelector
for "actionableEvents" is validated before being appended to the layers array:
update the guard around the querySelector (the code that locates
actionableEvents) to check for null/undefined and only push actionableEvents
into layers if it is a truthy HTMLElement; if the selector fails, skip adding it
(or log/debug) so layers never contains an invalid entry. Ensure references to
actionableEvents and the layers array are updated accordingly so downstream code
always receives a valid element.
---
Outside diff comments:
In `@src/core/game/PlayerImpl.ts`:
- Around line 849-867: Add tests for donateGold to assert the displayMessage
focusPlayerID is set correctly: spy on or inspect game.displayMessage /
GameUpdateType.DisplayEvent after calling PlayerImpl.donateGold and assert the
call for MessageType.SENT_GOLD_TO_PLAYER has its last positional argument equal
to recipient.id() and the call for MessageType.RECEIVED_GOLD_FROM_PLAYER has its
last positional argument equal to donor.id() (use the PlayerImpl instance's
id()). Locate usages of donateGold, displayMessage, and
MessageType.SENT_GOLD_TO_PLAYER / MessageType.RECEIVED_GOLD_FROM_PLAYER in the
test suite, invoke donateGold with a valid amount, capture the display events,
and assert the final argument (focusPlayerID) matches the expected player id for
each message type.
In `@src/core/game/UnitImpl.ts`:
- Around line 216-232: Add unit tests that assert UnitImpl.setOwner triggers
mg.displayMessage with the unitID equal to the unit's id (this.id()).
Specifically, create tests that instantiate a UnitImpl, stub/mock its message
gateway (mg) to capture calls, call UnitImpl.setOwner (or the method containing
the shown displayMessage calls), and assert that mg.displayMessage is invoked
for MessageType.UNIT_CAPTURED_BY_ENEMY and MessageType.CAPTURED_ENEMY_UNIT with
the final argument equal to the unit's id; reference UnitImpl.setOwner,
mg.displayMessage, MessageType.UNIT_CAPTURED_BY_ENEMY, and
MessageType.CAPTURED_ENEMY_UNIT when locating the code under test.
- Around line 335-342: Add a unit test that asserts
UnitImpl.displayMessageOnDeleted invokes MessageGroup.displayMessage with
MessageType.UNIT_DESTROYED and passes the deleted unit's id as the unitID
parameter: create a test that constructs a UnitImpl (or a minimal stub),
spies/mocks its mg (MessageGroup) and owner(), calls displayMessageOnDeleted (or
the code path that triggers the shown this.mg.displayMessage), and verifies the
mock received arguments including the context { unit: this._type } and that the
unitID argument equals the unit's id (this.id()). Use the existing test
harness/mock utilities to target UnitImpl.displayMessageOnDeleted,
MessageGroup.displayMessage, and MessageType.UNIT_DESTROYED.
---
Duplicate comments:
In `@src/client/hud/GameRenderer.ts`:
- Around line 124-132: The code retrieves actionableEvents and logs an error if
it's not an ActionableEvents but then unconditionally assigns
actionableEvents.eventBus, .game, and .uiState; guard those assignments by only
performing them when actionableEvents is an instance of ActionableEvents (or
early-return/throw after the error). Update the block in GameRenderer.ts around
actionableEvents so that after the instanceof check you either return/exit on
error or wrap the three assignments (eventBus, game, uiState) inside the
successful branch that confirms actionableEvents is an ActionableEvents.
In `@src/client/hud/layers/ActionableEvents.ts`:
- Around line 184-189: The cleanup loop iterating this.alliancesCheckedAt
removes stale alliance renewal events via
removeAllianceRenewalEvents(allianceId) but doesn't trigger a re-render; after
calling removeAllianceRenewalEvents(allianceId) and deleting from
this.alliancesCheckedAt, call requestUpdate() so the UI reflects the removed
prompts. Update the loop in ActionableEvents (the block referencing
this.alliancesCheckedAt and removeAllianceRenewalEvents) to invoke
requestUpdate() once a removal occurs (or once after the loop if multiple
removals) to force an update.
In `@src/core/execution/TradeShipExecution.ts`:
- Line 192: The change to TradeShipExecution using this.srcPort.tile() when
calling srcPort.owner().addGold alters the addGold event signature and requires
a core test that verifies the source-port gold bonus emits with the tile
context; add a unit test in the core test suite that constructs the minimal game
state (a srcPort with owner and tile, and a TradeShipExecution invocation or
equivalent helper), triggers the gold-award path (calling
TradeShipExecution.execute or the relevant method), and asserts that addGold was
called/emitted with both the correct gold amount and the source port's tile;
reference TradeShipExecution, srcPort.owner().addGold, and srcPort.tile() when
locating where to add the test.
🪄 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: 7eb593e0-e168-4c22-a070-e9932bb9bd9b
📒 Files selected for processing (16)
index.htmlresources/lang/en.jsonsrc/client/Utils.tssrc/client/hud/GameRenderer.tssrc/client/hud/layers/ActionableEvents.tssrc/client/hud/layers/ControlPanel.tssrc/client/hud/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
| new HoverHighlightController(game, eventBus, transformHandler, view), | ||
| new AttackingTroopsOverlay(game, transformHandler, eventBus, userSettings), | ||
| eventsDisplay, | ||
| actionableEvents, |
Contributor
There was a problem hiding this comment.
Verify actionableEvents is valid before adding to layers.
Line 279 adds actionableEvents to the layers array without confirming the element was found. If the querySelector at Line 124 fails, this will add an invalid element to the renderer.
This issue is linked to the guard problem at Line 124-132. Fixing that guard will resolve both issues.
🤖 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/hud/GameRenderer.ts` at line 279, In GameRenderer, ensure the
element returned by the querySelector for "actionableEvents" is validated before
being appended to the layers array: update the guard around the querySelector
(the code that locates actionableEvents) to check for null/undefined and only
push actionableEvents into layers if it is a truthy HTMLElement; if the selector
fails, skip adding it (or log/debug) so layers never contains an invalid entry.
Ensure references to actionableEvents and the layers array are updated
accordingly so downstream code always receives a valid element.
github-project-automation
Bot
moved this from Development
to Complete
in OpenFront Release Management
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
2 participants
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:
<actionable-events>that owns alliance prompts (request / renew) and a slimmed-down<events-display>for everything else.<events-display>into two visual tiers: dim/scrolling tier 2 on top (trade results, unit losses, donations, alliance status), prominent tier 1 anchored at the bottom (inbound nukes, naval invasion, attack requests, alliance broken, conquered player, chat). Tier 2 caps at the 4 newest entries; events expire after 8s.<control-panel>, animated with a small fade-in. Fires for trade ships, trains, donations, and conquest. Trade-ship and train arrivals are removed from the events scroll since they're surfaced here instead.MessageType.NUKE_DETONATEDand a server-side emission inNukeExecution.detonate— once an inbound nuke lands or gets intercepted, the inbound warning vanishes and a "detonated" entry takes its place.displayMessagegained optionalunitIDandfocusPlayerIDparams so events can link to a unit or a player. Unit captures and destructions now navigate to the unit's last tile when clicked; donations navigate to the other player.<events-display>; cards persist until the user clicks Accept/Reject/Renew/Ignore or the server-side request timeout expires.<events-display>is now a lightweight log that hides entirely when empty.Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
evan