Move leaderboard#3909
Draft
evanpelle wants to merge 1 commit into
Draft
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
💤 Files with no reviewable changes (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (6)
WalkthroughConsolidates left/right in-game sidebars into a single ChangesUI Consolidation & Refactor
sequenceDiagram
participant Renderer as Renderer
participant GameMenu as GameMenu
participant Leaderboard as Leaderboard
participant ReplayPanel as ReplayPanel
participant TeamStats as TeamStats
participant EventBus as EventBus
Renderer->>GameMenu: Query element & assign<br/>`game`, `eventBus`
GameMenu->>EventBus: Subscribe (SpawnBarVisible,<br/>ImmunityBarVisible, SendWinner)
GameMenu->>Leaderboard: Reference & forward tick()
GameMenu->>ReplayPanel: Reference & forward tick()
GameMenu->>TeamStats: Reference & forward tick()
rect rgba(100, 150, 200, 0.5)
Note over GameMenu: Tick loop — compute timer,<br/>team color, visibility
end
rect rgba(100, 150, 200, 0.5)
Note over GameMenu: User actions — pause/play,<br/>replay toggle, settings, exit
GameMenu->>EventBus: Emit events (Pause, ShowReplayPanel, ShowSettingsModal)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
evanpelle
force-pushed
the
move-leaderboard
branch
from
d4174f8 to
a93fc22
Compare
![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: 4
🤖 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 88-92: The code logs when the queried gameMenu is missing/wrong
type but continues and dereferences it; update the guard in GameRenderer so
after checking "if (!gameMenu || !(gameMenu instanceof GameMenu))" you exit
early (return or throw) to avoid null dereference instead of just logging,
ensuring subsequent assignments to gameMenu.game and gameMenu.eventBus only run
when gameMenu is a valid GameMenu instance.
In `@src/client/graphics/layers/GameMenu.ts`:
- Around line 281-283: The menu contains hard-coded user-visible strings and
fallback alt text that must be localized: replace occurrences of "settings",
"exit", "replay", "play/pause" and any inline fallback alt strings (e.g.,
"Player Leaderboard Icon") in GameMenu.ts with calls to translateText(...) and
add matching keys to resources/lang/en.json; update the JSX/templating in the
GameMenu component (search for the strings around the render methods and the
elements referenced by the comment ranges ~281-333 and ~388-401) to call
translateText("menu.settings"), translateText("menu.exit"),
translateText("menu.replay"), translateText("menu.play_pause") (or similar
descriptive keys) and ensure each key exists in resources/lang/en.json with an
English value.
- Line 18: Replace direct Vite asset imports in GameMenu.ts (e.g., exitIcon
import) with the assetUrl() helper calls used elsewhere (call
assetUrl("images/ExitIconWhite.svg") and similarly for other icons referenced in
the file) so assets resolve via the manifest/CDN; also replace hardcoded alt
strings at the render sites (currently "settings", "exit", "replay",
"play/pause") with translateText(...) calls that mirror the existing pattern
used at lines ~281–283 and ~311–313 (provide a fallback string as the second
argument to translateText where appropriate). Ensure you update the import usage
locations to use the returned URL from assetUrl and keep existing variable names
in GameMenu.ts.
In `@src/client/graphics/layers/Leaderboard.ts`:
- Around line 49-52: willUpdate() currently calls updateLeaderboard()
unconditionally when visible is true, causing updateLeaderboard() to change the
players property and call requestUpdate(), which produces an infinite update
loop; fix it by only invoking updateLeaderboard() from willUpdate when the
relevant reactive properties actually changed — check the _changed map for
'visible' or 'game' (the same property keys used by the component) and call
updateLeaderboard() only if one of those keys is present; leave tick() and
setSort() as-is since they already trigger updates appropriately.
🪄 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: ff2dab67-11a3-4656-b38e-f087cc186cf9
📒 Files selected for processing (9)
index.htmlsrc/client/graphics/GameRenderer.tssrc/client/graphics/layers/GameLeftSidebar.tssrc/client/graphics/layers/GameMenu.tssrc/client/graphics/layers/GameRightSidebar.tssrc/client/graphics/layers/Leaderboard.tssrc/client/graphics/layers/PlayerInfoOverlay.tssrc/client/graphics/layers/ReplayPanel.tssrc/client/graphics/layers/TeamStats.ts
💤 Files with no reviewable changes (2)
- src/client/graphics/layers/GameRightSidebar.ts
- src/client/graphics/layers/GameLeftSidebar.ts
Comment on lines
+88
to
+92
| if (!gameMenu || !(gameMenu instanceof GameMenu)) { | ||
| console.error("GameMenu element not found in the DOM"); | ||
| } | ||
| leaderboard.eventBus = eventBus; | ||
| leaderboard.game = game; | ||
|
|
||
| const gameLeftSidebar = document.querySelector( | ||
| "game-left-sidebar", | ||
| ) as GameLeftSidebar; | ||
| if (!gameLeftSidebar || !(gameLeftSidebar instanceof GameLeftSidebar)) { | ||
| console.error("GameLeftSidebar element not found in the DOM"); | ||
| } | ||
| gameLeftSidebar.game = game; | ||
| gameLeftSidebar.eventBus = eventBus; | ||
|
|
||
| const teamStats = document.querySelector("team-stats") as TeamStats; | ||
| if (!teamStats || !(teamStats instanceof TeamStats)) { | ||
| console.error("TeamStats element not found in the DOM"); | ||
| } | ||
| teamStats.eventBus = eventBus; | ||
| teamStats.game = game; | ||
| gameMenu.game = game; | ||
| gameMenu.eventBus = eventBus; |
Contributor
There was a problem hiding this comment.
Stop after failed game-menu lookup to avoid null dereference.
The current guard only logs, but code still assigns gameMenu.game/gameMenu.eventBus. If the element is missing or wrong type, this will throw.
Proposed fix
const gameMenu = document.querySelector("game-menu") as GameMenu;
if (!gameMenu || !(gameMenu instanceof GameMenu)) {
- console.error("GameMenu element not found in the DOM");
+ throw new Error("GameMenu element not found in the DOM");
}
gameMenu.game = game;
gameMenu.eventBus = eventBus;📝 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
| if (!gameMenu || !(gameMenu instanceof GameMenu)) { | |
| console.error("GameMenu element not found in the DOM"); | |
| } | |
| leaderboard.eventBus = eventBus; | |
| leaderboard.game = game; | |
| const gameLeftSidebar = document.querySelector( | |
| "game-left-sidebar", | |
| ) as GameLeftSidebar; | |
| if (!gameLeftSidebar || !(gameLeftSidebar instanceof GameLeftSidebar)) { | |
| console.error("GameLeftSidebar element not found in the DOM"); | |
| } | |
| gameLeftSidebar.game = game; | |
| gameLeftSidebar.eventBus = eventBus; | |
| const teamStats = document.querySelector("team-stats") as TeamStats; | |
| if (!teamStats || !(teamStats instanceof TeamStats)) { | |
| console.error("TeamStats element not found in the DOM"); | |
| } | |
| teamStats.eventBus = eventBus; | |
| teamStats.game = game; | |
| gameMenu.game = game; | |
| gameMenu.eventBus = eventBus; | |
| const gameMenu = document.querySelector("game-menu") as GameMenu; | |
| if (!gameMenu || !(gameMenu instanceof GameMenu)) { | |
| throw new Error("GameMenu element not found in the DOM"); | |
| } | |
| gameMenu.game = game; | |
| gameMenu.eventBus = eventBus; |
🤖 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 88 - 92, The code logs when
the queried gameMenu is missing/wrong type but continues and dereferences it;
update the guard in GameRenderer so after checking "if (!gameMenu || !(gameMenu
instanceof GameMenu))" you exit early (return or throw) to avoid null
dereference instead of just logging, ensuring subsequent assignments to
gameMenu.game and gameMenu.eventBus only run when gameMenu is a valid GameMenu
instance.
| import { ShowSettingsModalEvent } from "./SettingsModal"; | ||
| import { SpawnBarVisibleEvent } from "./SpawnTimer"; | ||
| import { TeamStats } from "./TeamStats"; | ||
| import exitIcon from "/images/ExitIconWhite.svg?url"; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether the referenced file exists and find the correct icon path/name.
fd -i 'ExitIconWhite.svg'
fd -i 'ExitIcon*.svg'
rg -n 'ExitIconWhite.svg|ExitIcon' src/clientRepository: openfrontio/OpenFrontIO
Length of output: 410
🏁 Script executed:
# Find assetUrl function definition
rg -n "assetUrl" src/client --type ts --type tsx -A 2 -B 2Repository: openfrontio/OpenFrontIO
Length of output: 95
🏁 Script executed:
# Check GameMenu.ts for all imports and hardcoded strings
fd -e ts -e tsx | xargs grep -l "GameMenu" | head -5
cat -n src/client/graphics/layers/GameMenu.tsRepository: openfrontio/OpenFrontIO
Length of output: 15797
🏁 Script executed:
rg -n "assetUrl" src/client --type ts -A 5 -B 2 | head -50Repository: openfrontio/OpenFrontIO
Length of output: 3216
🏁 Script executed:
cat -n src/core/AssetUrls.tsRepository: openfrontio/OpenFrontIO
Length of output: 4231
Use assetUrl() helper for icon imports instead of direct Vite imports; fix hardcoded alt text.
Import pattern at line 18 (and lines 19–26) doesn't match the codebase convention. Use assetUrl("images/ExitIconWhite.svg") like SettingsModal.ts and PlayerModerationModal.ts do. Direct Vite imports bypass the asset manifest and CDN base URL system, preventing proper asset resolution during build and deployment.
Also, lines 328, 332, 388, and 400 have hardcoded alt text ("settings", "exit", "replay", "play/pause") that must go through translateText() per the i18n guideline. Lines 281–282 and 311–313 show the correct pattern with fallbacks.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/client/graphics/layers/GameMenu.ts` at line 18, Replace direct Vite asset
imports in GameMenu.ts (e.g., exitIcon import) with the assetUrl() helper calls
used elsewhere (call assetUrl("images/ExitIconWhite.svg") and similarly for
other icons referenced in the file) so assets resolve via the manifest/CDN; also
replace hardcoded alt strings at the render sites (currently "settings", "exit",
"replay", "play/pause") with translateText(...) calls that mirror the existing
pattern used at lines ~281–283 and ~311–313 (provide a fallback string as the
second argument to translateText where appropriate). Ensure you update the
import usage locations to use the returned URL from assetUrl and keep existing
variable names in GameMenu.ts.
Comment on lines
+281
to
+283
| alt=${translateText("help_modal.icon_alt_player_leaderboard") || | ||
| "Player Leaderboard Icon"} | ||
| width="20" |
Contributor
There was a problem hiding this comment.
Localize all new menu labels/alt text through translateText().
Several user-visible strings are hardcoded (e.g., "settings", "exit", "replay", "play/pause" and fallback English alt text). Please route all of them through translateText() and ensure matching resources/lang/en.json keys exist.
As per coding guidelines: "All user-visible text must go through translateText() function with corresponding entries in resources/lang/en.json".
Also applies to: 311-313, 328-333, 388-401
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/client/graphics/layers/GameMenu.ts` around lines 281 - 283, The menu
contains hard-coded user-visible strings and fallback alt text that must be
localized: replace occurrences of "settings", "exit", "replay", "play/pause" and
any inline fallback alt strings (e.g., "Player Leaderboard Icon") in GameMenu.ts
with calls to translateText(...) and add matching keys to
resources/lang/en.json; update the JSX/templating in the GameMenu component
(search for the strings around the render methods and the elements referenced by
the comment ranges ~281-333 and ~388-401) to call
translateText("menu.settings"), translateText("menu.exit"),
translateText("menu.replay"), translateText("menu.play_pause") (or similar
descriptive keys) and ensure each key exists in resources/lang/en.json with an
English value.
Comment on lines
+49
to
52
| willUpdate(_changed: Map<string, unknown>) { | ||
| if (this.visible && this.game !== null) { | ||
| this.updateLeaderboard(); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
head -160 src/client/graphics/layers/Leaderboard.ts | tail -n +40Repository: openfrontio/OpenFrontIO
Length of output: 3328
🏁 Script executed:
wc -l src/client/graphics/layers/Leaderboard.tsRepository: openfrontio/OpenFrontIO
Length of output: 113
🏁 Script executed:
sed -n '145,160p' src/client/graphics/layers/Leaderboard.tsRepository: openfrontio/OpenFrontIO
Length of output: 478
🏁 Script executed:
grep -n "requestUpdate\|willUpdate" src/client/graphics/layers/Leaderboard.tsRepository: openfrontio/OpenFrontIO
Length of output: 147
Stop infinite update loop in willUpdate() when leaderboard is visible.
willUpdate() calls updateLeaderboard() every time Lit schedules an update while the leaderboard is visible. Inside updateLeaderboard(), you modify the players property and call requestUpdate(), which schedules a new update cycle. This creates a continuous feedback loop that will degrade performance.
Only run updateLeaderboard() when the visible or game properties actually change:
Proposed fix
- willUpdate(_changed: Map<string, unknown>) {
- if (this.visible && this.game !== null) {
+ willUpdate(changed: Map<string, unknown>) {
+ if (
+ this.visible &&
+ this.game !== null &&
+ (changed.has("visible") || changed.has("game"))
+ ) {
this.updateLeaderboard();
}
}The tick() method and setSort() already call updateLeaderboard() directly when appropriate, so they do not need changes.
📝 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
| willUpdate(_changed: Map<string, unknown>) { | |
| if (this.visible && this.game !== null) { | |
| this.updateLeaderboard(); | |
| } | |
| willUpdate(changed: Map<string, unknown>) { | |
| if ( | |
| this.visible && | |
| this.game !== null && | |
| (changed.has("visible") || changed.has("game")) | |
| ) { | |
| this.updateLeaderboard(); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/client/graphics/layers/Leaderboard.ts` around lines 49 - 52, willUpdate()
currently calls updateLeaderboard() unconditionally when visible is true,
causing updateLeaderboard() to change the players property and call
requestUpdate(), which produces an infinite update loop; fix it by only invoking
updateLeaderboard() from willUpdate when the relevant reactive properties
actually changed — check the _changed map for 'visible' or 'game' (the same
property keys used by the component) and call updateLeaderboard() only if one of
those keys is present; leave tick() and setSort() as-is since they already
trigger updates appropriately.
github-project-automation
Bot
moved this from Triage
to Development
in OpenFront Release Management
evanpelle
force-pushed
the
move-leaderboard
branch
from
a93fc22 to
f311ee9
Compare
evanpelle
force-pushed
the
move-leaderboard
branch
from
f311ee9 to
ee0c995
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/client/graphics/layers/Leaderboard.ts (1)
49-52:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winStop infinite update loop in
willUpdate()when leaderboard is visible.This issue was previously flagged:
willUpdate()callsupdateLeaderboard()unconditionally when visible, butupdateLeaderboard()callsrequestUpdate()at line 157, creating a continuous feedback loop.Check the
_changedparameter to only runupdateLeaderboard()whenvisibleorgameactually changed:Proposed fix
- willUpdate(_changed: Map<string, unknown>) { - if (this.visible && this.game !== null) { + willUpdate(changed: Map<string, unknown>) { + if ( + this.visible && + this.game !== null && + (changed.has("visible") || changed.has("game")) + ) { this.updateLeaderboard(); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/client/graphics/layers/Leaderboard.ts` around lines 49 - 52, willUpdate currently calls updateLeaderboard whenever the component is visible which causes an infinite loop because updateLeaderboard calls requestUpdate; modify willUpdate to inspect the _changed map and only call updateLeaderboard when the "visible" or "game" keys actually changed (e.g., check _changed.has('visible') || _changed.has('game') ) so you avoid triggering updateLeaderboard on every update and break the feedback loop between willUpdate and requestUpdate.
🤖 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.
Duplicate comments:
In `@src/client/graphics/layers/Leaderboard.ts`:
- Around line 49-52: willUpdate currently calls updateLeaderboard whenever the
component is visible which causes an infinite loop because updateLeaderboard
calls requestUpdate; modify willUpdate to inspect the _changed map and only call
updateLeaderboard when the "visible" or "game" keys actually changed (e.g.,
check _changed.has('visible') || _changed.has('game') ) so you avoid triggering
updateLeaderboard on every update and break the feedback loop between willUpdate
and requestUpdate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4bbe8952-66bb-471c-8aa2-b5015645c122
📒 Files selected for processing (9)
index.htmlsrc/client/graphics/GameRenderer.tssrc/client/graphics/layers/GameLeftSidebar.tssrc/client/graphics/layers/GameMenu.tssrc/client/graphics/layers/GameRightSidebar.tssrc/client/graphics/layers/Leaderboard.tssrc/client/graphics/layers/PlayerInfoOverlay.tssrc/client/graphics/layers/ReplayPanel.tssrc/client/graphics/layers/TeamStats.ts
💤 Files with no reviewable changes (2)
- src/client/graphics/layers/GameRightSidebar.ts
- src/client/graphics/layers/GameLeftSidebar.ts
✅ Files skipped from review due to trivial changes (1)
- src/client/graphics/layers/PlayerInfoOverlay.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- src/client/graphics/layers/ReplayPanel.ts
- src/client/graphics/GameRenderer.ts
- src/client/graphics/layers/TeamStats.ts
- index.html
- src/client/graphics/layers/GameMenu.ts
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.
If this PR fixes an issue, link it below. If not, delete these two lines.
Resolves #(issue number)
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