Add player info on mouseover and add display options #1608
Conversation
- Introduce a new overlay for displaying player stats near the cursor. - Implement settings to toggle between different information display modes. - Enhance user experience by providing contextual player information.
WalkthroughAdds a configurable "Information Display" feature with three modes (overlay, mouse HUD, both); introduces a mouse-follow HUD element, centralizes hover/mouse logic into PlayerInfoManager and PlayerInfoService, wires a keybind to force the mouse HUD, updates renderer/index, settings UI, user settings storage, and adds tests and localization. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SettingsModal
participant UserSettings
participant InputHandler
participant EventBus
participant GameRenderer
participant PlayerInfoManager
participant PlayerInfoService
participant PlayerInfoOverlay
participant PlayerInfoMouseOverlay
User->>SettingsModal: Click toggle info display
SettingsModal->>UserSettings: setInfoDisplayMode(next)
User->>InputHandler: Press/Release ControlRight
InputHandler->>EventBus: emit ForcePlayerInfoMouseOverlayEvent(true/false)
EventBus->>PlayerInfoMouseOverlay: forward force state
User->>GameRenderer: Move mouse
GameRenderer->>PlayerInfoManager: notify mouse move (x,y)
PlayerInfoManager->>PlayerInfoService: getHoverInfo(x,y) (throttled)
PlayerInfoService-->>PlayerInfoManager: hoverInfo
PlayerInfoManager-->>PlayerInfoOverlay: notify hoverInfo
PlayerInfoManager-->>PlayerInfoMouseOverlay: notify hoverInfo + mouse coords
PlayerInfoOverlay->>UserSettings: query showPlayerInfoOverlay()
PlayerInfoMouseOverlay->>UserSettings: query showPlayerInfoMouseOverlay()
Note right of overlays: Render based on settings, force, and dragging
Estimated code review effort🎯 4 (Complex) | ⏱️ ~30–45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
resources/lang/fr.json (1)
258-334: Remove French translations from this PRThe English keys are already in
resources/lang/en.json. To follow our workflow, please revert the changes in the French file and let the translation team handlefr.jsonin a separate PR.• resources/lang/fr.json (lines 258–334): undo added translations
🧹 Nitpick comments (4)
.gitignore (1)
12-12: Add trailing slash so only the folder is ignored
.ideawithout the slash also ignores any file whose name merely ends with.idea. A clearer, stricter pattern is to tell Git to skip the directory itself:-.idea +.idea/Tiny change, but it avoids surprises later.
src/core/game/UserSettings.ts (1)
53-72: Consider using consistent storage patterns and simplifying the boolean query methods.The new methods break the established pattern used by other settings. The existing methods use the generic
get(key, defaultValue)andset(key, value)with booleans, but these new methods directly access localStorage with strings.For better consistency and maintainability:
+ infoDisplayMode(): string { + return localStorage.getItem("settings.infoDisplayMode") ?? "both"; + } + + setInfoDisplayMode(mode: string): void { + localStorage.setItem("settings.infoDisplayMode", mode); + } + + showPlayerInfoOverlay(): boolean { + const mode = this.infoDisplayMode(); + return mode === "overlay" || mode === "both"; + } + + showPlayerInfoMouseOverlay(): boolean { + const mode = this.infoDisplayMode(); + return mode === "mousehud" || mode === "both"; + }Could be simplified to:
+ infoDisplayMode(): string { + return localStorage.getItem("settings.infoDisplayMode") ?? "both"; + } + + setInfoDisplayMode(mode: string): void { + localStorage.setItem("settings.infoDisplayMode", mode); + } + + showPlayerInfoOverlay(): boolean { + const mode = this.infoDisplayMode(); + return mode !== "mousehud"; + } + + showPlayerInfoMouseOverlay(): boolean { + const mode = this.infoDisplayMode(); + return mode !== "overlay"; + }src/client/graphics/layers/SettingsModal.ts (1)
322-346: Consider using a distinct icon and simplifying the conditional logic.The button implementation follows the established pattern, but using the same
mouseIconfor both left-click settings and info display mode could confuse users since these are different features.The conditional logic for displaying mode descriptions could be simplified:
- <div class="text-sm text-slate-400"> - ${this.userSettings.infoDisplayMode() === "overlay" - ? translateText("user_setting.info_display_overlay_only") - : this.userSettings.infoDisplayMode() === "mousehud" - ? translateText("user_setting.info_display_mousehud_only") - : translateText("user_setting.info_display_both")} - </div> + <div class="text-sm text-slate-400"> + ${translateText(`user_setting.info_display_${this.userSettings.infoDisplayMode()}_only`) || + translateText("user_setting.info_display_both")} + </div>And similarly for the short labels section.
src/client/graphics/layers/PlayerInfoMouseOverlay.ts (1)
49-69: Consider using ResizeObserver for accurate dimensionsThe
offsetWidthandoffsetHeightmight return 0 if the element hasn't been rendered yet or hasdisplay: none. This could lead to incorrect positioning on the first render.Consider caching dimensions or using ResizeObserver to track the actual rendered size of the overlay for more reliable positioning calculations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.gitignore(1 hunks)resources/lang/en.json(1 hunks)resources/lang/fr.json(2 hunks)src/client/graphics/GameRenderer.ts(3 hunks)src/client/graphics/layers/BasePlayerInfoOverlay.ts(1 hunks)src/client/graphics/layers/PlayerInfoMouseOverlay.ts(1 hunks)src/client/graphics/layers/PlayerInfoOverlay.ts(4 hunks)src/client/graphics/layers/SettingsModal.ts(2 hunks)src/client/index.html(1 hunks)src/core/game/UserSettings.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (9)
resources/lang/en.json (4)
Learnt from: andrewNiziolek
PR: #1007
File: resources/lang/de.json:115-115
Timestamp: 2025-06-02T14:27:37.609Z
Learning: For OpenFrontIO project: When localization keys are renamed in language JSON files, the maintainers separate technical changes from translation content updates. They wait for community translators to update the actual translation values rather than attempting to translate in the same PR. This allows technical changes to proceed while ensuring accurate translations from native speakers.
Learnt from: Aotumuri
PR: #1357
File: resources/lang/de.json:523-540
Timestamp: 2025-07-12T08:41:35.101Z
Learning: In OpenFrontIO project localization files, always check the en.json source file before flagging potential spelling errors in other language files, as some keys may intentionally use non-standard spellings that need to be consistent across all translations.
Learnt from: scottanderson
PR: #949
File: resources/lang/en.json:8-10
Timestamp: 2025-05-30T03:53:52.231Z
Learning: For the OpenFrontIO project, do not suggest updating translation files in resources/lang/*.json except for en.json. The project has a dedicated translation team that handles all other locale files.
Learnt from: Aotumuri
PR: #1357
File: resources/lang/zh_cn.json:527-539
Timestamp: 2025-07-12T08:42:02.109Z
Learning: In OpenFrontIO project, the territory pattern key "embelem" is the correct spelling used in all language files, not "emblem". This is the technical identifier that appears in en.json and must be consistent across all localization files.
src/client/index.html (1)
Learnt from: VariableVince
PR: #1110
File: src/client/Main.ts:293-295
Timestamp: 2025-06-09T02:20:43.637Z
Learning: In src/client/Main.ts, during game start in the handleJoinLobby callback, UI elements are hidden using direct DOM manipulation with classList.add("hidden") for consistency. This includes modals, buttons, and error divs. The codebase follows this pattern rather than using component APIs for hiding elements during game transitions.
src/client/graphics/GameRenderer.ts (4)
Learnt from: VariableVince
PR: #1110
File: src/client/Main.ts:293-295
Timestamp: 2025-06-09T02:20:43.637Z
Learning: In src/client/Main.ts, during game start in the handleJoinLobby callback, UI elements are hidden using direct DOM manipulation with classList.add("hidden") for consistency. This includes modals, buttons, and error divs. The codebase follows this pattern rather than using component APIs for hiding elements during game transitions.
Learnt from: devalnor
PR: #1248
File: src/client/graphics/layers/TerritoryInfoLayer.ts:20-20
Timestamp: 2025-06-22T21:51:14.990Z
Learning: In TerritoryInfoLayer.ts, the highlightedTerritory field uses both null and undefined intentionally: undefined represents initial state or inactive layer (Ctrl released), while null represents active layer with no territory being highlighted at cursor position. This distinction is important for proper state change detection.
Learnt from: devalnor
PR: #1195
File: src/client/graphics/layers/AlertFrame.ts:18-18
Timestamp: 2025-06-20T20:11:00.965Z
Learning: In the OpenFrontIO codebase, UserSettings instances are created directly with new UserSettings() in each component that needs them. This pattern is used consistently across at least 12+ files including OptionsMenu, EventsDisplay, DarkModeButton, Main, UserSettingModal, UsernameInput, NameLayer, AlertFrame, UILayer, InputHandler, ClientGameRunner, and GameView. This is the established architectural pattern and should be followed for consistency.
Learnt from: devalnor
PR: #1195
File: src/client/graphics/layers/AlertFrame.ts:18-18
Timestamp: 2025-06-20T20:11:00.965Z
Learning: In the OpenFrontIO codebase, UserSettings instances are created directly with new UserSettings() in components like OptionsMenu, EventsDisplay, and AlertFrame. This is the established pattern and should be followed for consistency rather than using dependency injection.
resources/lang/fr.json (4)
Learnt from: andrewNiziolek
PR: #1007
File: resources/lang/de.json:115-115
Timestamp: 2025-06-02T14:27:37.609Z
Learning: For OpenFrontIO project: When localization keys are renamed in language JSON files, the maintainers separate technical changes from translation content updates. They wait for community translators to update the actual translation values rather than attempting to translate in the same PR. This allows technical changes to proceed while ensuring accurate translations from native speakers.
Learnt from: Aotumuri
PR: #1357
File: resources/lang/de.json:523-540
Timestamp: 2025-07-12T08:41:35.101Z
Learning: In OpenFrontIO project localization files, always check the en.json source file before flagging potential spelling errors in other language files, as some keys may intentionally use non-standard spellings that need to be consistent across all translations.
Learnt from: scottanderson
PR: #949
File: resources/lang/en.json:8-10
Timestamp: 2025-05-30T03:53:52.231Z
Learning: For the OpenFrontIO project, do not suggest updating translation files in resources/lang/*.json except for en.json. The project has a dedicated translation team that handles all other locale files.
Learnt from: VariableVince
PR: #957
File: resources/maps/GatewayToTheAtlantic.json:175-179
Timestamp: 2025-05-30T16:16:27.118Z
Learning: In OpenFrontIO project, flag values in map JSON files (like GatewayToTheAtlantic.json) should match the actual flag file names in the resources directory (e.g., "French foreign legion" corresponds to "French foreign legion.svg"). The naming convention follows the file naming system rather than code naming conventions.
src/client/graphics/layers/SettingsModal.ts (2)
Learnt from: VariableVince
PR: #1110
File: src/client/Main.ts:293-295
Timestamp: 2025-06-09T02:20:43.637Z
Learning: In src/client/Main.ts, during game start in the handleJoinLobby callback, UI elements are hidden using direct DOM manipulation with classList.add("hidden") for consistency. This includes modals, buttons, and error divs. The codebase follows this pattern rather than using component APIs for hiding elements during game transitions.
Learnt from: devalnor
PR: #1195
File: src/client/graphics/layers/AlertFrame.ts:18-18
Timestamp: 2025-06-20T20:11:00.965Z
Learning: In the OpenFrontIO codebase, UserSettings instances are created directly with new UserSettings() in each component that needs them. This pattern is used consistently across at least 12+ files including OptionsMenu, EventsDisplay, DarkModeButton, Main, UserSettingModal, UsernameInput, NameLayer, AlertFrame, UILayer, InputHandler, ClientGameRunner, and GameView. This is the established architectural pattern and should be followed for consistency.
src/client/graphics/layers/PlayerInfoMouseOverlay.ts (2)
Learnt from: VariableVince
PR: #1110
File: src/client/Main.ts:293-295
Timestamp: 2025-06-09T02:20:43.637Z
Learning: In src/client/Main.ts, during game start in the handleJoinLobby callback, UI elements are hidden using direct DOM manipulation with classList.add("hidden") for consistency. This includes modals, buttons, and error divs. The codebase follows this pattern rather than using component APIs for hiding elements during game transitions.
Learnt from: devalnor
PR: #1248
File: src/client/graphics/layers/TerritoryInfoLayer.ts:20-20
Timestamp: 2025-06-22T21:51:14.990Z
Learning: In TerritoryInfoLayer.ts, the highlightedTerritory field uses both null and undefined intentionally: undefined represents initial state or inactive layer (Ctrl released), while null represents active layer with no territory being highlighted at cursor position. This distinction is important for proper state change detection.
src/core/game/UserSettings.ts (3)
Learnt from: devalnor
PR: #1195
File: src/client/graphics/layers/AlertFrame.ts:18-18
Timestamp: 2025-06-20T20:11:00.965Z
Learning: In the OpenFrontIO codebase, UserSettings instances are created directly with new UserSettings() in each component that needs them. This pattern is used consistently across at least 12+ files including OptionsMenu, EventsDisplay, DarkModeButton, Main, UserSettingModal, UsernameInput, NameLayer, AlertFrame, UILayer, InputHandler, ClientGameRunner, and GameView. This is the established architectural pattern and should be followed for consistency.
Learnt from: devalnor
PR: #1195
File: src/client/graphics/layers/AlertFrame.ts:18-18
Timestamp: 2025-06-20T20:11:00.965Z
Learning: In the OpenFrontIO codebase, UserSettings instances are created directly with new UserSettings() in components like OptionsMenu, EventsDisplay, and AlertFrame. This is the established pattern and should be followed for consistency rather than using dependency injection.
Learnt from: VariableVince
PR: #1110
File: src/client/Main.ts:293-295
Timestamp: 2025-06-09T02:20:43.637Z
Learning: In src/client/Main.ts, during game start in the handleJoinLobby callback, UI elements are hidden using direct DOM manipulation with classList.add("hidden") for consistency. This includes modals, buttons, and error divs. The codebase follows this pattern rather than using component APIs for hiding elements during game transitions.
src/client/graphics/layers/BasePlayerInfoOverlay.ts (4)
Learnt from: VariableVince
PR: #1110
File: src/client/Main.ts:293-295
Timestamp: 2025-06-09T02:20:43.637Z
Learning: In src/client/Main.ts, during game start in the handleJoinLobby callback, UI elements are hidden using direct DOM manipulation with classList.add("hidden") for consistency. This includes modals, buttons, and error divs. The codebase follows this pattern rather than using component APIs for hiding elements during game transitions.
Learnt from: devalnor
PR: #1248
File: src/client/graphics/layers/TerritoryInfoLayer.ts:20-20
Timestamp: 2025-06-22T21:51:14.990Z
Learning: In TerritoryInfoLayer.ts, the highlightedTerritory field uses both null and undefined intentionally: undefined represents initial state or inactive layer (Ctrl released), while null represents active layer with no territory being highlighted at cursor position. This distinction is important for proper state change detection.
Learnt from: scottanderson
PR: #784
File: src/core/game/StatsImpl.ts:34-38
Timestamp: 2025-05-21T04:10:33.435Z
Learning: In the codebase, PlayerStats is defined as a type inferred from a Zod schema that is marked as optional, which means PlayerStats already includes undefined as a possible type (PlayerStats | undefined).
Learnt from: Ble4Ch
PR: #1063
File: src/core/configuration/PastelThemeDark.ts:53-53
Timestamp: 2025-06-10T09:56:44.473Z
Learning: In ColorAllocator class in src/core/configuration/Colors.ts, the correct method names are assignColor(id: string): Colord for general color assignment and assignTeamColor(team: Team): Colord for team-specific colors. There are no assignPlayerColor() or assignBotColor() methods.
src/client/graphics/layers/PlayerInfoOverlay.ts (4)
Learnt from: VariableVince
PR: #1110
File: src/client/Main.ts:293-295
Timestamp: 2025-06-09T02:20:43.637Z
Learning: In src/client/Main.ts, during game start in the handleJoinLobby callback, UI elements are hidden using direct DOM manipulation with classList.add("hidden") for consistency. This includes modals, buttons, and error divs. The codebase follows this pattern rather than using component APIs for hiding elements during game transitions.
Learnt from: devalnor
PR: #1248
File: src/client/graphics/layers/TerritoryInfoLayer.ts:20-20
Timestamp: 2025-06-22T21:51:14.990Z
Learning: In TerritoryInfoLayer.ts, the highlightedTerritory field uses both null and undefined intentionally: undefined represents initial state or inactive layer (Ctrl released), while null represents active layer with no territory being highlighted at cursor position. This distinction is important for proper state change detection.
Learnt from: Ble4Ch
PR: #1063
File: src/core/configuration/PastelThemeDark.ts:53-53
Timestamp: 2025-06-10T09:56:44.473Z
Learning: In ColorAllocator class in src/core/configuration/Colors.ts, the correct method names are assignColor(id: string): Colord for general color assignment and assignTeamColor(team: Team): Colord for team-specific colors. There are no assignPlayerColor() or assignBotColor() methods.
Learnt from: devalnor
PR: #1432
File: src/client/graphics/layers/NameLayer.ts:143-143
Timestamp: 2025-07-14T20:41:57.645Z
Learning: In the OpenFrontIO project, the Layer system (like NameLayer) doesn't use Lit framework and has no disconnectedCallback() or unmount mechanism available. This creates challenges for proper cleanup of event listeners and timeouts, leading to potential memory leaks. This is a known architectural limitation that should be addressed in future work.
🧬 Code Graph Analysis (2)
src/client/graphics/layers/SettingsModal.ts (1)
src/client/LangSelector.ts (1)
translateText(244-264)
src/client/graphics/layers/PlayerInfoMouseOverlay.ts (3)
src/client/InputHandler.ts (1)
MouseMoveEvent(31-36)src/client/graphics/layers/BasePlayerInfoOverlay.ts (1)
OVERLAY_CONFIG(304-304)src/core/game/GameView.ts (1)
PlayerView(180-363)
🔇 Additional comments (7)
src/client/index.html (1)
270-270: Clean integration of the new mouse HUD element.The placement outside the fixed-position overlay container makes sense since the mouse HUD needs dynamic positioning behavior different from the static overlays.
resources/lang/en.json (1)
332-341: Comprehensive localization coverage for the new feature.The translation keys are well-structured and provide both detailed descriptions and concise labels for flexible UI usage. The text is clear and user-friendly, covering all three display modes appropriately.
src/client/graphics/GameRenderer.ts (3)
25-25: Good import organization.The import is properly placed in alphabetical order with other layer imports.
132-143: Clean integration following established patterns.The implementation correctly follows the same pattern used for other UI components in this file:
- DOM query with type checking
- Error logging for missing elements
- Property assignment for dependencies
- Addition to the layers array
The userSettings property addition to both overlays makes sense for conditional rendering based on display mode.
267-267: Proper layer ordering in the rendering pipeline.The mouseHUD is positioned appropriately in the layers array, maintaining the established rendering order.
src/client/graphics/layers/SettingsModal.ts (1)
117-136: Clean cycling logic for the display modes.The switch statement clearly handles all three modes and cycles through them in a logical order. The implementation follows the established pattern used by other toggle methods in this class.
src/client/graphics/layers/PlayerInfoOverlay.ts (1)
40-51: Good refactoring to use base class methodsThe refactoring successfully delegates hover detection to the base class while maintaining the original show/hide behavior. The simplified logic is easier to understand and maintain.
github-project-automation
bot
moved this from Triage
to Development
in OpenFront Release Management
drillskibo
added
the
Feature - Frontend
Enhance the mouse event management for player info overlays, improving maintainability and clarity. This change allows for better control of mouse interactions and ensures proper cleanup of event listeners. - Introduced mouseMoveCallback for handling mouse movements - Added methods to manage mouse down, up, and leave events - Ensured event listeners are removed on component destruction
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/client/graphics/layers/BasePlayerInfoOverlay.ts (1)
170-172: Avoid type casting with better type guardsThe type cast
as PlayerViewcould be avoided by improving the type system. Consider adding a type guard or refining the return type ofowner.isPlayer().if (owner && owner.isPlayer()) { - this.handlePlayerHover(owner as PlayerView); + // If isPlayer() returned owner is PlayerView, no cast needed + this.handlePlayerHover(owner);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
resources/lang/en.json(1 hunks)src/client/graphics/GameRenderer.ts(3 hunks)src/client/graphics/layers/BasePlayerInfoOverlay.ts(1 hunks)src/client/graphics/layers/PlayerInfoMouseOverlay.ts(1 hunks)src/core/game/UserSettings.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- resources/lang/en.json
- src/client/graphics/GameRenderer.ts
- src/client/graphics/layers/PlayerInfoMouseOverlay.ts
- src/core/game/UserSettings.ts
🧰 Additional context used
🧠 Learnings (1)
src/client/graphics/layers/BasePlayerInfoOverlay.ts (4)
Learnt from: VariableVince
PR: #1110
File: src/client/Main.ts:293-295
Timestamp: 2025-06-09T02:20:43.637Z
Learning: In src/client/Main.ts, during game start in the handleJoinLobby callback, UI elements are hidden using direct DOM manipulation with classList.add("hidden") for consistency. This includes modals, buttons, and error divs. The codebase follows this pattern rather than using component APIs for hiding elements during game transitions.
Learnt from: devalnor
PR: #1248
File: src/client/graphics/layers/TerritoryInfoLayer.ts:20-20
Timestamp: 2025-06-22T21:51:14.990Z
Learning: In TerritoryInfoLayer.ts, the highlightedTerritory field uses both null and undefined intentionally: undefined represents initial state or inactive layer (Ctrl released), while null represents active layer with no territory being highlighted at cursor position. This distinction is important for proper state change detection.
Learnt from: scottanderson
PR: #784
File: src/core/game/StatsImpl.ts:34-38
Timestamp: 2025-05-21T04:10:33.435Z
Learning: In the codebase, PlayerStats is defined as a type inferred from a Zod schema that is marked as optional, which means PlayerStats already includes undefined as a possible type (PlayerStats | undefined).
Learnt from: devalnor
PR: #1432
File: src/client/graphics/layers/NameLayer.ts:143-143
Timestamp: 2025-07-14T20:41:57.645Z
Learning: In the OpenFrontIO project, the Layer system (like NameLayer) doesn't use Lit framework and has no disconnectedCallback() or unmount mechanism available. This creates challenges for proper cleanup of event listeners and timeouts, leading to potential memory leaks. This is a known architectural limitation that should be addressed in future work.
🔇 Additional comments (1)
src/client/graphics/layers/BasePlayerInfoOverlay.ts (1)
102-107: Event listener setup properly addresses cleanup concernsGood implementation storing the callback reference for proper cleanup in the destroy method. This addresses the previous review comment about missing event listener cleanup.
scilone
force-pushed
the
1500-cursor-stats-overlay
branch
from
ac02d96 to
9af7927
Compare
Refactor the BasePlayerInfoOverlay to improve the structure of stat definitions by replacing labels with codes. This change enhances maintainability and consistency across the codebase. - Changed 'label' property to 'code' in StatDefinition interface - Updated STAT_DEFINITIONS to use codes instead of labels - Adjusted emojiMap and rowMap to reflect the new structure
…o 1500-cursor-stats-overlay
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/client/graphics/layers/PlayerInfoOverlay.ts (1)
85-203: Break down large rendering method for better readability.The
renderPlayerInfo()method handles too many responsibilities. Consider splitting it into smaller, focused methods likerenderPlayerHeader(),renderPlayerStats(), andrenderPlayerUnits().Example refactor:
private renderPlayerInfo(player: PlayerView) { + return html` + <div class="p-2"> + ${this.renderPlayerHeader(player)} + ${this.renderPlayerStats(player)} + ${this.renderPlayerUnits(player)} + ${this.renderPlayerRelation(player)} + </div> + `; + } + + private renderPlayerHeader(player: PlayerView) { const myPlayer = this.game.myPlayer(); const isFriendly = myPlayer?.isFriendly(player); - let relationHtml: TemplateResult | null = null; // ... header rendering logic + } + + private renderPlayerStats(player: PlayerView) { + // ... stats rendering logic + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
resources/lang/en.json(1 hunks)src/client/graphics/GameRenderer.ts(3 hunks)src/client/graphics/layers/PlayerInfoOverlay.ts(3 hunks)src/client/graphics/layers/SettingsModal.ts(2 hunks)src/client/index.html(1 hunks)src/core/game/UserSettings.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- src/client/index.html
- resources/lang/en.json
- src/core/game/UserSettings.ts
- src/client/graphics/GameRenderer.ts
- src/client/graphics/layers/SettingsModal.ts
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: VariableVince
PR: openfrontio/OpenFrontIO#1110
File: src/client/Main.ts:293-295
Timestamp: 2025-06-09T02:20:43.637Z
Learning: In src/client/Main.ts, during game start in the handleJoinLobby callback, UI elements are hidden using direct DOM manipulation with classList.add("hidden") for consistency. This includes modals, buttons, and error divs. The codebase follows this pattern rather than using component APIs for hiding elements during game transitions.
📚 Learning: in src/client/main.ts, during game start in the handlejoinlobby callback, ui elements are hidden usi...
Learnt from: VariableVince
PR: openfrontio/OpenFrontIO#1110
File: src/client/Main.ts:293-295
Timestamp: 2025-06-09T02:20:43.637Z
Learning: In src/client/Main.ts, during game start in the handleJoinLobby callback, UI elements are hidden using direct DOM manipulation with classList.add("hidden") for consistency. This includes modals, buttons, and error divs. The codebase follows this pattern rather than using component APIs for hiding elements during game transitions.
Applied to files:
src/client/graphics/layers/PlayerInfoOverlay.ts
📚 Learning: in territoryinfolayer.ts, the highlightedterritory field uses both null and undefined intentionally:...
Learnt from: devalnor
PR: openfrontio/OpenFrontIO#1248
File: src/client/graphics/layers/TerritoryInfoLayer.ts:20-20
Timestamp: 2025-06-22T21:51:14.990Z
Learning: In TerritoryInfoLayer.ts, the highlightedTerritory field uses both null and undefined intentionally: undefined represents initial state or inactive layer (Ctrl released), while null represents active layer with no territory being highlighted at cursor position. This distinction is important for proper state change detection.
Applied to files:
src/client/graphics/layers/PlayerInfoOverlay.ts
📚 Learning: in colorallocator class in src/core/configuration/colors.ts, the correct method names are assigncolo...
Learnt from: Ble4Ch
PR: openfrontio/OpenFrontIO#1063
File: src/core/configuration/PastelThemeDark.ts:53-53
Timestamp: 2025-06-10T09:56:44.473Z
Learning: In ColorAllocator class in src/core/configuration/Colors.ts, the correct method names are assignColor(id: string): Colord for general color assignment and assignTeamColor(team: Team): Colord for team-specific colors. There are no assignPlayerColor() or assignBotColor() methods.
Applied to files:
src/client/graphics/layers/PlayerInfoOverlay.ts
📚 Learning: in the openfrontio project, the layer system (like namelayer) doesn't use lit framework and has no d...
Learnt from: devalnor
PR: openfrontio/OpenFrontIO#1432
File: src/client/graphics/layers/NameLayer.ts:143-143
Timestamp: 2025-07-14T20:41:57.645Z
Learning: In the OpenFrontIO project, the Layer system (like NameLayer) doesn't use Lit framework and has no disconnectedCallback() or unmount mechanism available. This creates challenges for proper cleanup of event listeners and timeouts, leading to potential memory leaks. This is a known architectural limitation that should be addressed in future work.
Applied to files:
src/client/graphics/layers/PlayerInfoOverlay.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: 🏗️ Build
- GitHub Check: 🔬 Test
🔇 Additional comments (5)
src/client/graphics/layers/PlayerInfoOverlay.ts (5)
20-25: Clean event handling setup.The event listener setup is simple and clear. Good use of descriptive method names.
27-34: Good performance optimization with mouse throttling.The 100ms throttle prevents excessive updates during mouse movement. Clean implementation.
36-51: Well-structured visibility and state management.The
shouldRender()method properly checks both activation state and user settings. Thehide()andmaybeShow()methods have clear responsibilities and good delegation to base class methods.
58-83: Clean utility methods with proper translation handling.Both
getRelationName()anddisplayUnitCount()are well-structured with appropriate default cases and conditional logic. Good use of translation functions.
205-228: Clean main render method with proper conditional logic.The render method properly uses
shouldRender()and has good separation between player and unit info rendering. Clean CSS transitions for visibility.
This update introduces a new PlayerInfoManager to handle player information overlays, enhancing the user experience by providing real-time data on player and unit interactions. The changes improve maintainability and performance by centralizing hover information logic. - Added PlayerInfoManager for managing hover data and mouse events - Integrated PlayerInfoService for fetching player and unit details - Updated PlayerInfoMouseOverlay and PlayerInfoOverlay to utilize the new manager - Improved event handling for mouse movements and data subscriptions
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
src/client/graphics/layers/PlayerInfoService.ts (3)
34-44: Use clearer function names and consider extracting coordinate typeThe function works correctly but could have a clearer name. Also, consider defining a coordinate type for better reusability.
+type Coordinate2D = { x: number; y: number }; + -function euclideanDistWorld( - coord: { x: number; y: number }, +function calculateWorldDistance( + coord: Coordinate2D, tileRef: TileRef, game: GameView, ): number {
62-78: Consider using functions instead of class for better simplicityThe service class could be simplified by using plain functions with a factory pattern. This would align better with functional programming practices and reduce complexity.
-export class PlayerInfoService { - private readonly game: GameView; - private transform: TransformHandler; - - private readonly emojiMap = Object.fromEntries( - STAT_DEFINITIONS.map(({ code, emoji }) => [code, emoji]), - ); - - private readonly rowMap = Object.fromEntries( - STAT_DEFINITIONS.map(({ code, row }) => [code, row]), - ); - - constructor(game: GameView, transform: TransformHandler) { - this.game = game; - this.transform = transform; - } +const emojiMap = Object.fromEntries( + STAT_DEFINITIONS.map(({ code, emoji }) => [code, emoji]), +); + +const rowMap = Object.fromEntries( + STAT_DEFINITIONS.map(({ code, row }) => [code, row]), +); + +export function createPlayerInfoService(game: GameView, transform: TransformHandler) { + return { + findNearestUnit: (worldCoord: Coordinate2D) => findNearestUnit(game, worldCoord), + getHoverInfo: (x: number, y: number) => getHoverInfo(game, transform, x, y), + getRelationClass, + getRelation: (player: PlayerView) => getRelation(game, player), + getShortDisplayName, + calculatePlayerStats, + formatStats: (player: PlayerView) => formatStats(player), + }; +}
79-91: Update function call if renaming suggestion is acceptedIf you rename
euclideanDistWorldtocalculateWorldDistance, update the calls here..filter( (u) => - euclideanDistWorld(worldCoord, u.tile(), this.game) < + calculateWorldDistance(worldCoord, u.tile(), this.game) < OVERLAY_CONFIG.unitDetectionRadius, ) - .sort(distSortUnitWorld(worldCoord, this.game)); + .sort(createDistanceSorter(worldCoord, this.game));src/client/graphics/layers/PlayerInfoManager.ts (2)
32-44: Simplify singleton getInstance implementationThe nullish coalescing assignment operator is concise but could be clearer for readability.
static getInstance( game: GameView, transform: TransformHandler, eventBus: EventBus, ): PlayerInfoManager { - PlayerInfoManager.instance ??= new PlayerInfoManager( - game, - transform, - eventBus, - ); + if (!PlayerInfoManager.instance) { + PlayerInfoManager.instance = new PlayerInfoManager( + game, + transform, + eventBus, + ); + } return PlayerInfoManager.instance; }
111-118: Remove redundant null check and non-null assertionThe null check is unnecessary since
forEachwon't execute ifcurrentHoverInfois null. The non-null assertion on line 114 is also unnecessary.private notifyDataSubscribers() { - if (this.currentHoverInfo) { - this.dataSubscribers.forEach((callback) => - callback(this.currentHoverInfo!), - ); - } + if (!this.currentHoverInfo) return; + this.dataSubscribers.forEach((callback) => + callback(this.currentHoverInfo), + ); }src/client/graphics/layers/PlayerInfoOverlay.ts (1)
176-296: Consider breaking down renderPlayerInfo into smaller functionsThis method is quite long and handles many responsibilities. Consider extracting the flag rendering and stats sections into separate methods for better readability.
private renderPlayerFlag(player: PlayerView): TemplateResult { if (!player.cosmetics.flag) return html``; if (player.cosmetics.flag.startsWith("!")) { return html`<div class="h-8 mr-1 aspect-[3/4] player-flag" ${ref((el) => { if (el instanceof HTMLElement) { requestAnimationFrame(() => { renderPlayerFlag(player.cosmetics.flag!, el); }); } })} ></div>`; } return html`<img class="h-8 mr-1 aspect-[3/4]" src=${"/flags/" + player.cosmetics.flag + ".svg"} />`; } private renderPlayerStats(player: PlayerView): TemplateResult[] { const stats = []; if (player.troops() >= 1) { stats.push(html`<div class="text-sm opacity-80" translate="no"> ${translateText("player_info_overlay.d_troops")}: ${renderTroops(player.troops())} </div>`); } // ... other stats return stats; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/client/graphics/layers/PlayerInfoManager.ts(1 hunks)src/client/graphics/layers/PlayerInfoMouseOverlay.ts(1 hunks)src/client/graphics/layers/PlayerInfoOverlay.ts(6 hunks)src/client/graphics/layers/PlayerInfoService.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (8)
📚 Learning: in src/client/main.ts, during game start in the handlejoinlobby callback, ui elements are hidden usi...
Learnt from: VariableVince
PR: openfrontio/OpenFrontIO#1110
File: src/client/Main.ts:293-295
Timestamp: 2025-06-09T02:20:43.637Z
Learning: In src/client/Main.ts, during game start in the handleJoinLobby callback, UI elements are hidden using direct DOM manipulation with classList.add("hidden") for consistency. This includes modals, buttons, and error divs. The codebase follows this pattern rather than using component APIs for hiding elements during game transitions.
Applied to files:
src/client/graphics/layers/PlayerInfoManager.tssrc/client/graphics/layers/PlayerInfoMouseOverlay.tssrc/client/graphics/layers/PlayerInfoOverlay.ts
📚 Learning: in territoryinfolayer.ts, the highlightedterritory field uses both null and undefined intentionally:...
Learnt from: devalnor
PR: openfrontio/OpenFrontIO#1248
File: src/client/graphics/layers/TerritoryInfoLayer.ts:20-20
Timestamp: 2025-06-22T21:51:14.990Z
Learning: In TerritoryInfoLayer.ts, the highlightedTerritory field uses both null and undefined intentionally: undefined represents initial state or inactive layer (Ctrl released), while null represents active layer with no territory being highlighted at cursor position. This distinction is important for proper state change detection.
Applied to files:
src/client/graphics/layers/PlayerInfoMouseOverlay.tssrc/client/graphics/layers/PlayerInfoService.tssrc/client/graphics/layers/PlayerInfoOverlay.ts
📚 Learning: in the openfrontio project, the layer system (like namelayer) doesn't use lit framework and has no d...
Learnt from: devalnor
PR: openfrontio/OpenFrontIO#1432
File: src/client/graphics/layers/NameLayer.ts:143-143
Timestamp: 2025-07-14T20:41:57.645Z
Learning: In the OpenFrontIO project, the Layer system (like NameLayer) doesn't use Lit framework and has no disconnectedCallback() or unmount mechanism available. This creates challenges for proper cleanup of event listeners and timeouts, leading to potential memory leaks. This is a known architectural limitation that should be addressed in future work.
Applied to files:
src/client/graphics/layers/PlayerInfoMouseOverlay.ts
📚 Learning: in the codebase, playerstats is defined as a type inferred from a zod schema that is marked as optio...
Learnt from: scottanderson
PR: openfrontio/OpenFrontIO#784
File: src/core/game/StatsImpl.ts:34-38
Timestamp: 2025-05-21T04:10:33.435Z
Learning: In the codebase, PlayerStats is defined as a type inferred from a Zod schema that is marked as optional, which means PlayerStats already includes undefined as a possible type (PlayerStats | undefined).
Applied to files:
src/client/graphics/layers/PlayerInfoService.ts
📚 Learning: in the codebase, playerstats is defined as `z.infer` where playerstatssche...
Learnt from: scottanderson
PR: openfrontio/OpenFrontIO#784
File: src/core/game/StatsImpl.ts:34-38
Timestamp: 2025-05-21T04:10:33.435Z
Learning: In the codebase, PlayerStats is defined as `z.infer<typeof PlayerStatsSchema>` where PlayerStatsSchema has `.optional()` applied at the object level, making PlayerStats a union type that already includes undefined (PlayerStats | undefined).
Applied to files:
src/client/graphics/layers/PlayerInfoService.ts
📚 Learning: in statsimpl.ts, unused parameters in boat/stats-related methods are intentionally kept for future u...
Learnt from: scottanderson
PR: openfrontio/OpenFrontIO#784
File: src/core/game/StatsImpl.ts:125-134
Timestamp: 2025-05-19T06:00:38.007Z
Learning: In StatsImpl.ts, unused parameters in boat/stats-related methods are intentionally kept for future use and shouldn't be removed.
Applied to files:
src/client/graphics/layers/PlayerInfoService.tssrc/client/graphics/layers/PlayerInfoOverlay.ts
📚 Learning: in colorallocator class in src/core/configuration/colors.ts, the correct method names are assigncolo...
Learnt from: Ble4Ch
PR: openfrontio/OpenFrontIO#1063
File: src/core/configuration/PastelThemeDark.ts:53-53
Timestamp: 2025-06-10T09:56:44.473Z
Learning: In ColorAllocator class in src/core/configuration/Colors.ts, the correct method names are assignColor(id: string): Colord for general color assignment and assignTeamColor(team: Team): Colord for team-specific colors. There are no assignPlayerColor() or assignBotColor() methods.
Applied to files:
src/client/graphics/layers/PlayerInfoOverlay.ts
📚 Learning: the removetroops function in playerimpl.ts already prevents negative troop counts by using minint(th...
Learnt from: 1brucben
PR: openfrontio/OpenFrontIO#977
File: src/core/execution/AttackExecution.ts:123-125
Timestamp: 2025-05-31T18:15:03.445Z
Learning: The removeTroops function in PlayerImpl.ts already prevents negative troop counts by using minInt(this._troops, toInt(troops)) to ensure it never removes more troops than available.
Applied to files:
src/client/graphics/layers/PlayerInfoOverlay.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: 🏗️ Build
…o 1500-cursor-stats-overlay
I'm not sure I understand. In the settings, there is already the option to choose between cursor display or overlay. Are you suggesting adding a shortcut to quickly switch between the different modes? |
…ursor-stats-overlay
|
@scilone yes, we discussed this in the last meeting and would prefer to have a hotkey like shift that toggles the display rather than, or in addition to, having a setting |
|
I didn't realize there was a setting to toggle between the Mouse HUD, the anchored display on the right, or both. |
…ursor-stats-overlay
This update introduces a new feature that allows users to force the display of player information at the cursor position, enhancing the user experience during gameplay. The changes improve the interactivity of the player info overlay and provide more control to the users. - Added keybind for forcing player info overlay - Implemented event handling for the new overlay feature - Updated rendering logic to accommodate forced visibility - Refactored related components for better maintainability
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
src/client/InputHandler.ts (1)
151-169: Critical: keybind action mismatch breaks the feature by defaultDefaults define togglePlayerInfo, but runtime checks use forcePlayerInfoMouseOverlay. Unless the user manually sets the keybind in Settings, nothing will fire.
- Make the default use forcePlayerInfoMouseOverlay.
- Keep the UI action and runtime checks consistent.
Apply this diff:
this.keybinds = { toggleView: "Space", - togglePlayerInfo: "ShiftRight", + forcePlayerInfoMouseOverlay: "ShiftRight", centerCamera: "KeyC",With this change, the checks at keydown/keyup (already using forcePlayerInfoMouseOverlay) will work out of the box.
Optionally, for backward compatibility with any saved togglePlayerInfo settings:
- After loading from localStorage, map togglePlayerInfo to forcePlayerInfoMouseOverlay if present.
Also applies to: 271-277, 317-321
🧹 Nitpick comments (5)
src/client/InputHandler.ts (2)
71-74: Event type: consider a discriminated union over class-based eventsCurrent code uses classes for events. A simple typed union (e.g., { type: "ForcePlayerInfoMouseOverlay"; value: boolean }) improves ergonomics and testability, and avoids class hierarchies.
This is optional and can be a follow-up refactor.
353-359: Remove noisy debug logging or guard by environmentThe console.log spam on every KeyUp is noisy. Consider removing or gating under a dev flag.
tests/client/graphics/layers/PlayerInfoMouseOverlay.test.ts (1)
95-107: Use stable handler refs in add/removeEventListenerThe mock binds new function instances on add/remove, so removeEventListener won’t match the original handlers. Use arrow functions as fields and pass the same refs in both add/remove.
Apply this diff to the mock:
- private setupCanvasEventListeners() { + private setupCanvasEventListeners() { this.canvas = document.querySelector("canvas"); if (this.canvas) { - this.canvas.addEventListener( - "mousedown", - this.handleMouseDown.bind(this), - ); - this.canvas.addEventListener("mouseup", this.handleMouseUp.bind(this)); - this.canvas.addEventListener( - "mouseleave", - this.handleMouseLeave.bind(this), - ); + this.canvas.addEventListener("mousedown", this.handleMouseDown); + this.canvas.addEventListener("mouseup", this.handleMouseUp); + this.canvas.addEventListener("mouseleave", this.handleMouseLeave); } } private removeCanvasEventListeners() { if (this.canvas) { - this.canvas.removeEventListener( - "mousedown", - this.handleMouseDown.bind(this), - ); - this.canvas.removeEventListener("mouseup", this.handleMouseUp.bind(this)); - this.canvas.removeEventListener( - "mouseleave", - this.handleMouseLeave.bind(this), - ); + this.canvas.removeEventListener("mousedown", this.handleMouseDown); + this.canvas.removeEventListener("mouseup", this.handleMouseUp); + this.canvas.removeEventListener("mouseleave", this.handleMouseLeave); this.canvas = null; } } - private handleMouseDown() { - this.isDragging = true; - } + private handleMouseDown = () => { this.isDragging = true; }; - private handleMouseUp() { - this.isDragging = false; - } + private handleMouseUp = () => { this.isDragging = false; }; - private handleMouseLeave() { - this.isDragging = false; - } + private handleMouseLeave = () => { this.isDragging = false; };This aligns with the production overlay and makes the cleanup behavior realistic.
Also applies to: 110-123, 125-135
src/client/graphics/layers/PlayerInfoMouseOverlay.ts (2)
109-116: Canvas lookup is global and brittle; prefer compositiondocument.querySelector("canvas") is fragile. Pass the canvas (or a getter) into this layer, or fetch from an injected dependency (e.g., TransformHandler if it exposes the canvas). This keeps concerns local and testable.
Also applies to: 118-125
141-158: Positioning logic is solid; consider using styleMap for readabilitygetHUDPosition clamps to viewport and uses config-driven sizes/offsets. Rendering uses inline style strings. Optional: use Lit’s styleMap for clarity and fewer string templates.
Also applies to: 236-254
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
resources/lang/en.json(2 hunks)src/client/InputHandler.ts(5 hunks)src/client/UserSettingModal.ts(1 hunks)src/client/graphics/layers/PlayerInfoMouseOverlay.ts(1 hunks)tests/client/graphics/layers/PlayerInfoMouseOverlay.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- resources/lang/en.json
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-14T20:41:57.645Z
Learnt from: devalnor
PR: openfrontio/OpenFrontIO#1432
File: src/client/graphics/layers/NameLayer.ts:143-143
Timestamp: 2025-07-14T20:41:57.645Z
Learning: In the OpenFrontIO project, the Layer system (like NameLayer) doesn't use Lit framework and has no disconnectedCallback() or unmount mechanism available. This creates challenges for proper cleanup of event listeners and timeouts, leading to potential memory leaks. This is a known architectural limitation that should be addressed in future work.
Applied to files:
src/client/graphics/layers/PlayerInfoMouseOverlay.ts
🧬 Code Graph Analysis (2)
src/client/InputHandler.ts (1)
src/core/EventBus.ts (1)
GameEvent(1-1)
src/client/UserSettingModal.ts (1)
src/client/LangSelector.ts (1)
translateText(246-266)
🔇 Additional comments (3)
src/client/InputHandler.ts (1)
135-135: State flag makes sense to avoid repeat emitsKeeping forcePlayerInfoMouseOverlay as a guard prevents repeat emissions when the key auto-repeats. Looks good.
tests/client/graphics/layers/PlayerInfoMouseOverlay.test.ts (1)
349-369: Good coverage of EventBus subscription wiringAsserting eventBus.on is invoked with ForcePlayerInfoMouseOverlayEvent ensures the overlay listens correctly. Nice.
src/client/graphics/layers/PlayerInfoMouseOverlay.ts (1)
127-135: Render gating looks correctThe visibility checks are simple and clear: active, user setting or forced, has info, and not dragging. Good.
|
@drillskibo the anchor was now the default behavior |
This change modifies the keybinding for toggling the player info overlay to improve user accessibility and streamline controls. - Changed keybind from ShiftRight to ControlLeft for forcePlayerInfoMouseOverlay
This update introduces a new feature that allows users to force the display of player information at the cursor position, enhancing the user experience during gameplay. The changes improve the interactivity of the player info overlay and provide more control to the users. - Added keybind for forcing player info overlay - Implemented event handling for the new overlay feature - Updated rendering logic to accommodate forced visibility - Refactored related components for better maintainability
…ursor-stats-overlay
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
src/client/graphics/layers/PlayerInfoMouseOverlay.ts (5)
116-123: Scope canvas lookup or allow injection to avoid wrong elementdocument.querySelector("canvas") can pick the wrong canvas if multiple exist. Prefer passing the canvas in or scoping the query.
Apply this diff to support optional injection:
- private setupCanvasEventListeners() { - this.canvas = document.querySelector("canvas"); + @property({ type: Object }) + public canvasEl?: HTMLCanvasElement; + + private setupCanvasEventListeners() { + this.canvas = this.canvasEl ?? (document.querySelector("canvas") as HTMLCanvasElement | null); if (this.canvas) { this.canvas.addEventListener("mousedown", this.handleMouseDown); this.canvas.addEventListener("mouseup", this.handleMouseUp); this.canvas.addEventListener("mouseleave", this.handleMouseLeave); } }Alternatively, get the canvas from GameView if exposed (e.g., game.canvas).
229-231: Avoid unconditional re-render every ticktick() calls requestUpdate() every frame even when nothing changed. Gate it to reduce work.
Apply this diff:
- tick() { - this.requestUpdate(); - } + tick() { + if (this._isActive && (this.hasInfo || this.forcePlayerInfoMouseOverlay)) { + this.requestUpdate(); + } + }
144-151: Rename inner class to avoid confusion with host tagThe inner div uses class "mouse-hud" inside the element, which is easy to mix up. Use a distinct class.
Apply this diff:
- private getHudElement(): HTMLElement | null { - return this.querySelector(".mouse-hud") as HTMLElement; - } + private getHudElement(): HTMLElement | null { + return this.querySelector(".mouse-hud-content") as HTMLElement; + }- <div - class="mouse-hud fixed pointer-events-none z-50 px-3 py-2 bg-gray-800/70 backdrop-blur-sm shadow-lg rounded-lg text-white max-w-[90%] whitespace-nowrap transition-opacity duration-200" + <div + class="mouse-hud-content fixed pointer-events-none z-50 px-3 py-2 bg-gray-800/70 backdrop-blur-sm shadow-lg rounded-lg text-white max-w-[90%] whitespace-nowrap transition-opacity duration-200" style="left: ${position.x}px; top: ${position.y}px; opacity: ${opacity};" >Also applies to: 248-260
248-256: Optional perf: prefer transform translate over left/topUsing CSS transforms tends to be smoother for mouse-follow UI. Consider transform: translate(...) with will-change: transform.
Example:
- style="left: ${position.x}px; top: ${position.y}px; opacity: ${opacity};" + style="transform: translate(${position.x}px, ${position.y}px); opacity: ${opacity}; will-change: transform;"
12-14: Long-term: prefer composition over inheritanceConsider extracting the HUD as a small presentational function or component composed into a host, instead of extending LitElement directly. This reduces tight coupling and eases testing.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/client/InputHandler.ts(5 hunks)src/client/UserSettingModal.ts(1 hunks)src/client/graphics/layers/PlayerInfoMouseOverlay.ts(1 hunks)src/client/graphics/layers/SettingsModal.ts(2 hunks)tests/client/graphics/layers/PlayerInfoMouseOverlay.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/client/graphics/layers/PlayerInfoMouseOverlay.test.ts
- src/client/graphics/layers/SettingsModal.ts
- src/client/InputHandler.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-14T20:41:57.645Z
Learnt from: devalnor
PR: openfrontio/OpenFrontIO#1432
File: src/client/graphics/layers/NameLayer.ts:143-143
Timestamp: 2025-07-14T20:41:57.645Z
Learning: In the OpenFrontIO project, the Layer system (like NameLayer) doesn't use Lit framework and has no disconnectedCallback() or unmount mechanism available. This creates challenges for proper cleanup of event listeners and timeouts, leading to potential memory leaks. This is a known architectural limitation that should be addressed in future work.
Applied to files:
src/client/graphics/layers/PlayerInfoMouseOverlay.ts
🧬 Code Graph Analysis (2)
src/client/UserSettingModal.ts (1)
src/client/LangSelector.ts (1)
translateText(246-266)
src/client/graphics/layers/PlayerInfoMouseOverlay.ts (7)
src/client/graphics/layers/Layer.ts (1)
Layer(1-7)src/core/EventBus.ts (1)
EventBus(7-44)src/core/game/UserSettings.ts (1)
UserSettings(3-145)src/client/graphics/layers/PlayerInfoManager.ts (1)
PlayerInfoManager(11-120)src/client/graphics/layers/PlayerInfoService.ts (2)
HoverInfo(54-60)OVERLAY_CONFIG(214-214)src/client/InputHandler.ts (1)
ForcePlayerInfoMouseOverlayEvent(71-73)src/core/game/PlayerImpl.ts (1)
relation(446-452)
🔇 Additional comments (2)
src/client/UserSettingModal.ts (1)
440-449: Keybind wiring consistent and conflict-free — LGTMAction name matches InputHandler and defaultKey="ControlRight" avoids modifier clashes. Value binding and change handler are correct.
src/client/graphics/layers/PlayerInfoMouseOverlay.ts (1)
239-261: Overall implementation reads clean and idiomatic — nice workLifecycle wiring, subscriptions, and light DOM rendering are clear. The visibility logic and positioning are straightforward.
- Prevent `init` from reinitializing an active overlay. - Correct player info overlay position to respect top margin. - Ensure event listeners are properly cleaned up during disconnection.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
tests/client/graphics/layers/PlayerInfoMouseOverlay.test.ts (4)
101-107: Don’t overwriteeventBus.oninside the classSetting
this.eventBus.on = jest.fn()mutates the dependency under test and hides mistakes. Let the test provide spies/mocks.Apply this diff:
- this.eventBus.on = jest.fn(); this.eventBus.on( ForcePlayerInfoMouseOverlayEvent, this.forceOverlayEventHandler, );
560-572: Duplicate tests and misleading name around dragging behaviorTwo tests cover the same behavior, and one name suggests opacity changes while the code does not render at all during drag.
Apply this diff to rename the first and remove the duplicate:
- it("should set opacity to 0 when dragging", () => { + it("should not render when dragging", () => { @@ - expect(htmlString).toBe(""); + expect(htmlString).toBe(""); }); @@ - it("should not render when dragging", () => { - overlay.init(); - overlay["isDragging"] = true; - - const result = overlay.render(); - const htmlString = result.strings.join(""); - - expect(htmlString).toBe(""); - }); + // (removed duplicate)Also applies to: 593-601
31-33: Tighten types: replaceanywith typed unions and a mock interfaceUse small, explicit types for hover info and your mock manager. This makes intent clear and prevents accidental misuse.
Apply these diffs:
- private playerInfoManager: any; + private playerInfoManager: PlayerInfoManagerMock | null = null;- private onHoverInfoUpdate(hoverInfo: any) { + private onHoverInfoUpdate(hoverInfo: HoverInfo) { this.player = hoverInfo.player; this.unit = hoverInfo.unit; this.hasInfo = !!(this.player ?? this.unit); }- let mockPlayerInfoManager: any; + let mockPlayerInfoManager: PlayerInfoManagerMock;Add these types once (after the
jest.mock(...)line):type HoverInfo = { player: PlayerView | null; unit: UnitView | null; mouseX?: number; mouseY?: number; }; interface PlayerInfoManagerMock { init: jest.Mock; subscribeToData: jest.Mock; unsubscribeFromData: jest.Mock; subscribeToMouse: jest.Mock; unsubscribeFromMouse: jest.Mock; getPlayerInfoService: jest.Mock; }Also applies to: 75-79, 232-235, 16-17
161-166: Naming consistency:HUDvsHud
getHudElementvsgetHUDPositionmixes acronym casing. Prefer one style (e.g.,Hud) for readability.Apply this small rename for consistency:
- private getHUDPosition(): { x: number; y: number } { + private getHudPosition(): { x: number; y: number } { @@ - const position = this.getHUDPosition(); + const position = this.getHudPosition();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/client/graphics/layers/PlayerInfoMouseOverlay.ts(1 hunks)tests/client/graphics/layers/PlayerInfoMouseOverlay.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/client/graphics/layers/PlayerInfoMouseOverlay.ts
🔇 Additional comments (1)
tests/client/graphics/layers/PlayerInfoMouseOverlay.test.ts (1)
304-316: Assert unsubscribe uses the same callback that was subscribedTo catch regressions like the
.bindissue, verify the exact function reference is used for unsubscribe.Add this assertion to the destroy test:
overlay.init(); const dataCb = mockPlayerInfoManager.subscribeToData.mock.calls[0][0]; const mouseCb = mockPlayerInfoManager.subscribeToMouse.mock.calls[0][0]; overlay.destroy(); expect(mockPlayerInfoManager.unsubscribeFromData).toHaveBeenCalledWith(dataCb); expect(mockPlayerInfoManager.unsubscribeFromMouse).toHaveBeenCalledWith(mouseCb);
| this.playerInfoManager.subscribeToData(this.onHoverInfoUpdate.bind(this)); | ||
| this.playerInfoManager.subscribeToMouse( | ||
| this.onMousePositionUpdate.bind(this), | ||
| ); |
There was a problem hiding this comment.
Unsubscribing with new .bind references will leak listeners
You subscribe with bound callbacks, but unsubscribe with newly bound functions. These are different references, so unsubscription won’t work. This can hide bugs in the real component and cause memory leaks.
Fix by keeping stable references.
Apply this diff to reuse the same callbacks:
- this.playerInfoManager.subscribeToData(this.onHoverInfoUpdate.bind(this));
- this.playerInfoManager.subscribeToMouse(
- this.onMousePositionUpdate.bind(this),
- );
+ this.playerInfoManager.subscribeToData(this.onHoverInfoUpdateBound);
+ this.playerInfoManager.subscribeToMouse(this.onMousePositionUpdateBound);- this.playerInfoManager?.unsubscribeFromData(
- this.onHoverInfoUpdate.bind(this),
- );
- this.playerInfoManager?.unsubscribeFromMouse(
- this.onMousePositionUpdate.bind(this),
- );
+ this.playerInfoManager?.unsubscribeFromData(this.onHoverInfoUpdateBound);
+ this.playerInfoManager?.unsubscribeFromMouse(this.onMousePositionUpdateBound);Add the stable references once (outside these ranges):
// place near other fields (e.g., after forceOverlayEventHandler)
private onHoverInfoUpdateBound = (hoverInfo: HoverInfo) =>
this.onHoverInfoUpdate(hoverInfo);
private onMousePositionUpdateBound = (x: number, y: number) =>
this.onMousePositionUpdate(x, y);Also applies to: 56-61
🤖 Prompt for AI Agents
In tests/client/graphics/layers/PlayerInfoMouseOverlay.test.ts around lines
47-50 (and also 56-61), you create subscribers using new .bind calls which
produce different function references than the ones used to unsubscribe; create
stable bound callback fields near the other fields (e.g., after
forceOverlayEventHandler) such as onHoverInfoUpdateBound and
onMousePositionUpdateBound that call the instance methods, then use these stable
fields for both subscribeToData/subscribeToMouse and the corresponding
unsubscribe calls to ensure unsubscription works and avoid listener leaks.
| this.eventBus.off( | ||
| ForcePlayerInfoMouseOverlayEvent, | ||
| this.forceOverlayEventHandler, | ||
| ); | ||
| this.removeCanvasEventListeners(); | ||
| this._isActive = false; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Duplicate event-bus unsubscription; centralize cleanup
You call eventBus.off in both destroy() and disconnectedCallback(). Pick one lifecycle to avoid double-unsubscribing and confusion. Typical pattern: route destroy() through disconnectedCallback() to centralize DOM and bus cleanup.
Apply this diff to use disconnectedCallback() from destroy():
- this.eventBus.off(
- ForcePlayerInfoMouseOverlayEvent,
- this.forceOverlayEventHandler,
- );
- this.removeCanvasEventListeners();
+ this.disconnectedCallback();This keeps a single source of truth for teardown.
Also applies to: 91-99
🤖 Prompt for AI Agents
In tests/client/graphics/layers/PlayerInfoMouseOverlay.test.ts around lines
62-67 (and similarly 91-99), the eventBus.off call is duplicated in both
destroy() and disconnectedCallback(); refactor so only disconnectedCallback()
performs event-bus and DOM cleanup and have destroy() call
disconnectedCallback() to centralize teardown. Remove the eventBus.off,
removeCanvasEventListeners, and _isActive reset from destroy(), ensure
disconnectedCallback() contains the full unsubscribe and cleanup logic, and
update destroy() to simply invoke disconnectedCallback() (or delegate to the
same cleanup helper) to avoid double-unsubscribing.
| private setupCanvasEventListeners() { | ||
| this.canvas = document.querySelector("canvas"); | ||
| if (this.canvas) { | ||
| this.canvas.addEventListener( | ||
| "mousedown", | ||
| this.handleMouseDown.bind(this), | ||
| ); | ||
| this.canvas.addEventListener("mouseup", this.handleMouseUp.bind(this)); | ||
| this.canvas.addEventListener( | ||
| "mouseleave", | ||
| this.handleMouseLeave.bind(this), | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Canvas event listeners won’t be removed due to .bind creating new functions
You add/remove listeners using .bind(this) in both places. Each .bind call creates a new function, so removeEventListener won’t remove what was added.
Use stable handler references (arrow properties) and pass them directly.
Apply this diff to pass stable handlers:
- this.canvas.addEventListener(
- "mousedown",
- this.handleMouseDown.bind(this),
- );
- this.canvas.addEventListener("mouseup", this.handleMouseUp.bind(this));
- this.canvas.addEventListener(
- "mouseleave",
- this.handleMouseLeave.bind(this),
- );
+ this.canvas.addEventListener("mousedown", this.handleMouseDown);
+ this.canvas.addEventListener("mouseup", this.handleMouseUp);
+ this.canvas.addEventListener("mouseleave", this.handleMouseLeave);- this.canvas.removeEventListener(
- "mousedown",
- this.handleMouseDown.bind(this),
- );
- this.canvas.removeEventListener("mouseup", this.handleMouseUp.bind(this));
- this.canvas.removeEventListener(
- "mouseleave",
- this.handleMouseLeave.bind(this),
- );
+ this.canvas.removeEventListener("mousedown", this.handleMouseDown);
+ this.canvas.removeEventListener("mouseup", this.handleMouseUp);
+ this.canvas.removeEventListener("mouseleave", this.handleMouseLeave);- private handleMouseDown() {
- this.isDragging = true;
- }
+ private handleMouseDown = () => {
+ this.isDragging = true;
+ };
- private handleMouseUp() {
- this.isDragging = false;
- }
+ private handleMouseUp = () => {
+ this.isDragging = false;
+ };
- private handleMouseLeave() {
- this.isDragging = false;
- }
+ private handleMouseLeave = () => {
+ this.isDragging = false;
+ };Also applies to: 124-137, 139-149
🤖 Prompt for AI Agents
In tests/client/graphics/layers/PlayerInfoMouseOverlay.test.ts around lines
109-122 (also apply same fix at 124-137 and 139-149), event listeners are
added/removed using this.handleX.bind(this) which creates new functions so
removeEventListener cannot match them; replace bound callsites with stable
handler references: create arrow-bound properties on the class (e.g.
handleMouseDown = (e) => { ... }) or store the bound function once
(this._boundMouseDown = this.handleMouseDown.bind(this)) and use that single
reference for both addEventListener and removeEventListener, then update
add/remove calls to pass the same function reference.
| document.querySelector = jest.fn().mockReturnValue(mockCanvas); | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Avoid global mutation of document.querySelector; use a spy and restore
Assigning document.querySelector = jest.fn() pollutes global state across tests. Spy and restore in afterEach.
Apply this diff:
@@
let mockUnit: UnitView;
let mockCanvas: HTMLCanvasElement;
+ let querySpy: jest.SpyInstance;
@@
- mockCanvas = document.createElement("canvas");
- document.querySelector = jest.fn().mockReturnValue(mockCanvas);
+ mockCanvas = document.createElement("canvas");
+ querySpy = jest
+ .spyOn(document, "querySelector")
+ .mockReturnValue(mockCanvas as any);
@@
afterEach(() => {
jest.clearAllMocks();
+ querySpy?.mockRestore();
});Optional: also snapshot and restore window.innerWidth/innerHeight when you redefine them to avoid cross-test leakage.
const origW = window.innerWidth;
const origH = window.innerHeight;
// mutate in test...
// in afterEach:
Object.defineProperty(window, "innerWidth", { value: origW, configurable: true });
Object.defineProperty(window, "innerHeight", { value: origH, configurable: true });Also applies to: 236-246, 292-295, 231-235
Refactor tests to spy on document.querySelector and restore after each run
Global reassignment of document.querySelector can leak mocks into other tests. Instead, use jest.spyOn and call mockRestore() in your afterEach().
Affected tests (all under tests/client/graphics/layers/PlayerInfoMouseOverlay.test.ts at):
- Lines 231–235
- Lines 236–246
- Lines 283–284
- Lines 292–295
Apply this pattern in each block:
@@ setup before each test
- mockCanvas = document.createElement("canvas");
- document.querySelector = jest.fn().mockReturnValue(mockCanvas);
+ mockCanvas = document.createElement("canvas");
+ querySpy = jest
+ .spyOn(document, "querySelector")
+ .mockReturnValue(mockCanvas as any);
@@ teardown after each test
- jest.clearAllMocks();
+ jest.clearAllMocks();
+ querySpy?.mockRestore();Optional: if you override window.innerWidth or innerHeight, snapshot their originals and restore in afterEach to prevent cross-test leakage:
const origW = window.innerWidth;
const origH = window.innerHeight;
// … mutate in test …
afterEach(() => {
Object.defineProperty(window, "innerWidth", { value: origW, configurable: true });
Object.defineProperty(window, "innerHeight", { value: origH, configurable: true });
});🤖 Prompt for AI Agents
In tests/client/graphics/layers/PlayerInfoMouseOverlay.test.ts around lines
283–284 (and also address the other affected blocks at 231–235, 236–246,
292–295), stop reassigning document.querySelector globally and instead use
jest.spyOn(document, "querySelector").mockReturnValue(mockCanvas) inside each
test or beforeEach, and call the spy's mockRestore() in an afterEach() to avoid
leaking mocks across tests; if any tests mutate window.innerWidth/innerHeight,
capture their originals at test start and restore them in afterEach() using
Object.defineProperty with configurable: true.
|
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/client/graphics/layers/PlayerInfoOverlay.ts (3)
124-130: Defensive guard for maybeShow()Add a light guard so maybeShow() ignores events if the layer isn’t active or the manager is missing (future-proofing).
Apply this diff:
public async maybeShow(x: number, y: number) { + if (!this._isActive || !this.playerInfoManager) return; this.hide(); const hoverInfo = await this.playerInfoManager .getPlayerInfoService() .getHoverInfo(x, y); this.onHoverInfoUpdate(hoverInfo); }
100-103: Unsubscribe from data on DOM detach to avoid stale updatesdestroy() handles cleanup, but if the element is detached without destroy() being called, you’ll keep receiving data updates. Unsubscribe here too (idempotent with destroy()).
Apply this diff:
disconnectedCallback() { super.disconnectedCallback(); - this.removeEventListeners(); + this.playerInfoManager?.unsubscribeFromData(this.hoverCallback); + this.removeEventListeners(); }
77-93: Avoid redundant requestUpdate()setVisible() and hide() already call requestUpdate(). The extra requestUpdate() here is unnecessary.
Apply this diff:
private onHoverInfoUpdate(hoverInfo: HoverInfo) { if (!this.userSettings?.showPlayerInfoOverlay()) { this.hide(); return; } this.player = hoverInfo.player; this.playerProfile = hoverInfo.playerProfile; this.unit = hoverInfo.unit; if (this.player || this.unit) { this.setVisible(true); } else { this.hide(); } - this.requestUpdate(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/client/graphics/layers/PlayerInfoOverlay.ts(6 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-09T02:20:43.637Z
Learnt from: VariableVince
PR: openfrontio/OpenFrontIO#1110
File: src/client/Main.ts:293-295
Timestamp: 2025-06-09T02:20:43.637Z
Learning: In src/client/Main.ts, during game start in the handleJoinLobby callback, UI elements are hidden using direct DOM manipulation with classList.add("hidden") for consistency. This includes modals, buttons, and error divs. The codebase follows this pattern rather than using component APIs for hiding elements during game transitions.
Applied to files:
src/client/graphics/layers/PlayerInfoOverlay.ts
🧬 Code Graph Analysis (1)
src/client/graphics/layers/PlayerInfoOverlay.ts (9)
src/core/game/UserSettings.ts (1)
UserSettings(3-145)src/client/graphics/layers/PlayerInfoManager.ts (1)
PlayerInfoManager(11-120)src/client/graphics/layers/PlayerInfoService.ts (1)
HoverInfo(54-60)src/client/InputHandler.ts (1)
ContextMenuEvent(45-50)src/client/graphics/layers/RadialMenu.ts (1)
CloseRadialMenuEvent(14-16)src/core/game/GameView.ts (5)
x(585-587)y(588-590)unit(572-574)UnitView(47-178)player(510-516)src/core/game/GameMap.ts (2)
x(122-124)y(126-128)src/core/game/PlayerImpl.ts (1)
relation(446-452)src/client/Utils.ts (1)
translateText(82-137)
🔇 Additional comments (1)
src/client/graphics/layers/PlayerInfoOverlay.ts (1)
18-20: Good move to composition via PlayerInfoManager/ServiceClean separation of concerns and reuse: this overlay only renders; PlayerInfoManager centralizes events; PlayerInfoService handles relations/labels. This is idiomatic and easier to test.
Also applies to: 58-68, 170-193, 204-210
| private playerInfoManager!: PlayerInfoManager; | ||
| private _isActive = false; | ||
|
|
||
| private lastMouseUpdate = 0; | ||
| private hoverCallback = (hoverInfo: HoverInfo) => | ||
| this.onHoverInfoUpdate(hoverInfo); | ||
| private contextMenuHandler = (e: ContextMenuEvent) => | ||
| this.maybeShow(e.x, e.y); | ||
| private closeRadialMenuHandler = () => this.hide(); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make event listener management idempotent
If init() is ever called twice (or future refactors re-introduce multiple call sites), listeners will be re-registered. Add a simple guard to avoid duplicates.
Apply this diff:
@@
private playerInfoManager!: PlayerInfoManager;
private _isActive = false;
+ private listenersAttached = false;
private hoverCallback = (hoverInfo: HoverInfo) =>
this.onHoverInfoUpdate(hoverInfo);
private contextMenuHandler = (e: ContextMenuEvent) =>
this.maybeShow(e.x, e.y);
private closeRadialMenuHandler = () => this.hide();
@@
protected setupEventListeners() {
- this.eventBus.on(ContextMenuEvent, this.contextMenuHandler);
- this.eventBus.on(CloseRadialMenuEvent, this.closeRadialMenuHandler);
+ if (this.listenersAttached) return;
+ this.eventBus.on(ContextMenuEvent, this.contextMenuHandler);
+ this.eventBus.on(CloseRadialMenuEvent, this.closeRadialMenuHandler);
+ this.listenersAttached = true;
}
private removeEventListeners() {
- this.eventBus.off(ContextMenuEvent, this.contextMenuHandler);
- this.eventBus.off(CloseRadialMenuEvent, this.closeRadialMenuHandler);
+ if (!this.listenersAttached) return;
+ this.eventBus.off(ContextMenuEvent, this.contextMenuHandler);
+ this.eventBus.off(CloseRadialMenuEvent, this.closeRadialMenuHandler);
+ this.listenersAttached = false;
}Also applies to: 105-113
🤖 Prompt for AI Agents
In src/client/graphics/layers/PlayerInfoOverlay.ts around lines 48-55 (and
similarly at 105-113), the event listener fields are created unconditionally so
calling init() multiple times will register duplicate listeners; add an
idempotency guard (e.g., a private boolean like listenersRegistered or check
_isActive) and return early if listeners are already registered, and only attach
the hover, contextMenu and closeRadialMenu handlers when the guard is false;
also ensure the corresponding teardown/reset clears that flag so init() can run
again if properly destroyed.
| init() { | ||
| this.eventBus.on(MouseMoveEvent, (e: MouseMoveEvent) => | ||
| this.onMouseEvent(e), | ||
| this.playerInfoManager = PlayerInfoManager.getInstance( | ||
| this.game, | ||
| this.transform, | ||
| this.eventBus, | ||
| ); | ||
| this.eventBus.on(ContextMenuEvent, (e: ContextMenuEvent) => | ||
| this.maybeShow(e.x, e.y), | ||
| ); | ||
| this.eventBus.on(CloseRadialMenuEvent, () => this.hide()); | ||
|
|
||
| this.playerInfoManager.init(); | ||
| this.playerInfoManager.subscribeToData(this.hoverCallback); | ||
| this.setupEventListeners(); | ||
| this._isActive = true; |
There was a problem hiding this comment.
Prevent double subscription and pre-init crashes by moving listener setup out of connectedCallback()
setupEventListeners() is called in both init() and connectedCallback(). This registers handlers twice and can call maybeShow() before playerInfoManager is initialized, causing runtime errors. Attach/detach listeners only in init()/destroy().
Apply this diff:
@@
init() {
+ if (this._isActive) return;
this.playerInfoManager = PlayerInfoManager.getInstance(
this.game,
this.transform,
this.eventBus,
);
this.playerInfoManager.init();
this.playerInfoManager.subscribeToData(this.hoverCallback);
this.setupEventListeners();
this._isActive = true;
}
@@
connectedCallback() {
super.connectedCallback();
- this.setupEventListeners();
}
disconnectedCallback() {
super.disconnectedCallback();
- this.removeEventListeners();
}Also applies to: 95-103
🤖 Prompt for AI Agents
In src/client/graphics/layers/PlayerInfoOverlay.ts around lines 58-68 and also
lines 95-103, setupEventListeners() is currently called both in init() and
connectedCallback(), causing double registrations and allowing maybeShow()/other
handlers to run before playerInfoManager is initialized; remove
setupEventListeners() (and any corresponding attach code) from
connectedCallback() and ensure it is only called once in init() after
playerInfoManager is constructed and initialized, and add the corresponding
teardown/unsubscribe calls to destroy() to detach listeners and unsubscribe
hoverCallback so handlers cannot run after destroy or before init.
Description:
Relevant issue: #1500
This PR introduces a new mouse overlay feature for displaying player information near the cursor, along with configurable display mode settings. The implementation enhances user experience by providing contextual player information in different display modes.
One row
Two row
Ships
Toggle menu
Cursor only
Overlay only
Both
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
Scilone