Conversation
Contributor
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 | ✅ 1 | ❌ 4❌ Failed checks (1 warning, 3 inconclusive)
✅ Passed checks (1 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.
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.
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