Feature/improve team leaderboard#3340
Open
Lako-Lako wants to merge 6 commits intoopenfrontio:mainfrom
Open
Conversation
|
|
Contributor
WalkthroughTeamStats now supports clickable and keyboard-accessible sortable headers, stores raw numeric totals for correct sorting, and emits a GoToPlayerEvent (via eventBus) targeting the strongest alive player when a team row is activated. Reactive Changes
Sequence Diagram(s)sequenceDiagram
participant User as "User (click/keyboard)"
participant UI as "TeamStats UI"
participant BUS as "eventBus"
participant Game as "Game / Players"
User->>UI: activate team row (click / Enter / Space)
UI->>Game: compute strongest alive player for team (use raw totals)
Game-->>UI: returns playerId
UI->>BUS: emit GoToPlayerEvent(playerId)
BUS->>Game: route event -> focus / goto player
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/client/graphics/layers/TeamStats.ts (1)
46-47: Remove duplicate@state()on_sortKey.
@state()is applied twice to the same field. Keep only one to avoid duplicate reactive metadata registration.Proposed fix
- `@state`() `@state`() private _sortKey:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/graphics/layers/TeamStats.ts` around lines 46 - 47, The field decorator `@state`() is applied twice to the same private field _sortKey in TeamStats; remove the duplicate so _sortKey has only one `@state`() decorator to avoid duplicate reactive metadata registration — locate the _sortKey declaration in the TeamStats class and delete the redundant `@state`(), leaving a single `@state`() above _sortKey.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/client/graphics/layers/StructureIconsLayer.ts`:
- Around line 192-198: When unitUpdates reference units that no longer exist,
the loop in StructureIconsLayer skips them but leaves stale entries in
rendersByUnitId; update the loop handling unitUpdates so that when
this.game.unit(update.id) returns undefined you remove the corresponding entry
from rendersByUnitId and also clean up any visual resources for that unit (e.g.,
call the existing render disposal/remove method or destroy the sprite/container
stored in rendersByUnitId[update.id]). Ensure you reference the same keys
(update.id) and use the existing render cleanup code/path so orphaned renders
are removed when a unit is deleted.
In `@src/client/graphics/layers/TeamStats.ts`:
- Around line 119-123: The current mapping in TeamStats.ts builds a translation
key via `team_colors.${rawTeam.toLowerCase()}` and calls `translateText(key)`
for every `rawTeam`; avoid doing this for generic team IDs like "Team 1"/"Team
N" to prevent repeated missing-key fallbacks. Update the logic around
`rawTeam`/`translateText` (the block that computes `key`, `translated`, and
`teamName`) to first detect generic labels (e.g., match `/^team\s*\d+$/i` or a
"Team <number>" pattern) and, when matched, set `teamName = rawTeam` (skip
creating `key` and calling `translateText`); otherwise continue to build `key`
and use `translateText` as before. This change should be localized to the
mapping in TeamStats.ts that references `rawTeam`, `key`, and `translateText`.
- Around line 169-176: totalGoldRaw is a bigint and casting to Number loses
precision; update the sorting logic in TeamStats.ts to compare bigints directly
instead of Number(a.totalGoldRaw). Create a bigint comparator (e.g.,
compareBigInt) that accepts two bigint values and returns -1/0/1 based on
this._sortOrder (use v1 < v2 / v1 > v2 checks, and invert for "desc"), and use
that comparator in the "gold" branch of the switch on this._sortKey so the sort
uses the bigint comparison without converting to Number.
- Around line 242-313: Header divs that call setSort are not
keyboard-accessible; add role="button", tabindex="0", and an `@keydown` handler
that invokes this.setSort(...) when Enter or Space is pressed (mirror the
existing `@click` behavior in the same elements that call setSort). For team rows
(the elements currently using class="contents" with an `@click` handler to select
a team), stop using display:contents for that interactive element — either
change the container to a semantic interactive element (button or anchor) or
move the clickable element inside so it can be focused; then add tabindex="0"
(if not a native control) plus role/aria attributes and an `@keydown` handler that
triggers the same team-selection handler used by the `@click`. Ensure the keyboard
handlers check for Enter/Space and call the same selection methods so both mouse
and keyboard invoke identical logic.
---
Nitpick comments:
In `@src/client/graphics/layers/TeamStats.ts`:
- Around line 46-47: The field decorator `@state`() is applied twice to the same
private field _sortKey in TeamStats; remove the duplicate so _sortKey has only
one `@state`() decorator to avoid duplicate reactive metadata registration —
locate the _sortKey declaration in the TeamStats class and delete the redundant
`@state`(), leaving a single `@state`() above _sortKey.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
resources/lang/en.jsonsrc/client/FlagInput.tssrc/client/FlagInputModal.tssrc/client/GameModeSelector.tssrc/client/LangSelector.tssrc/client/PatternInput.tssrc/client/Utils.tssrc/client/components/Footer.tssrc/client/components/LobbyPlayerView.tssrc/client/components/MainLayout.tssrc/client/components/PatternButton.tssrc/client/components/PlayPage.tssrc/client/graphics/layers/GameLeftSidebar.tssrc/client/graphics/layers/PlayerInfoOverlay.tssrc/client/graphics/layers/PlayerPanel.tssrc/client/graphics/layers/StructureIconsLayer.tssrc/client/graphics/layers/TeamStats.tssrc/client/graphics/layers/UnitLayer.tstests/perf/StructureIconsLayerLookupPerf.ts
github-project-automation
bot
moved this from Triage
to Development
in OpenFront Release Management
Lako-Lako
force-pushed
the
feature/improve_team_leaderboard
branch
from
7354b99 to
02a2400
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/client/graphics/layers/TeamStats.ts (2)
169-172:⚠️ Potential issue | 🟠 MajorFix gold sorting to compare
bigintdirectly.
totalGoldRawisbigint; converting toNumber(...)can lose precision and produce wrong order for large totals.Suggested fix
.sort((a, b) => { const compare = (v1: number, v2: number) => this._sortOrder === "asc" ? v1 - v2 : v2 - v1; + const compareBigInt = (v1: bigint, v2: bigint) => { + if (v1 === v2) return 0; + const less = v1 < v2; + return this._sortOrder === "asc" ? (less ? -1 : 1) : less ? 1 : -1; + }; switch (this._sortKey) { case "gold": - return compare(Number(a.totalGoldRaw), Number(b.totalGoldRaw)); + return compareBigInt(a.totalGoldRaw, b.totalGoldRaw);#!/bin/bash # Verify bigint field + lossy numeric cast in sort branch rg -nP 'totalGoldRaw:\s*bigint|Number\(a\.totalGoldRaw\)|Number\(b\.totalGoldRaw\)' src/client/graphics/layers/TeamStats.ts -C2🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/graphics/layers/TeamStats.ts` around lines 169 - 172, The "gold" sort branch in TeamStats is casting totalGoldRaw (a bigint) to Number which can lose precision; replace the Number(...) casts in the case "gold" branch so you compare the bigint values directly (e.g., use a BigInt-aware comparison or call compare with a.totalGoldRaw and b.totalGoldRaw rather than Number(a.totalGoldRaw)/Number(b.totalGoldRaw)), keeping the branch inside the method that switches on this._sortKey.
239-240:⚠️ Potential issue | 🟠 MajorAdd keyboard-accessible interaction semantics for sortable headers and team-row actions.
These handlers are click-only, and the row target uses
class="contents"(not a robust focus/semantic target). Keyboard users currently cannot perform sorting/team focus actions.Also applies to: 250-251, 261-262, 272-273, 285-286, 296-297, 307-308, 324-329, 348-353
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/graphics/layers/TeamStats.ts` around lines 239 - 240, The sortable header cells in TeamStats.ts are only click-driven and some team rows use class="contents", making them inaccessible to keyboard users; update each clickable header (where `@click` calls this.setSort("...")) to include role="button", tabindex="0" and a keydown handler that triggers this.setSort(...) on Enter or Space, and similarly add role="button", tabindex="0" and a keydown handler for the team-row actions (the elements with class="contents") to invoke the existing team-selection/focus action (the same callback used by the click handler) on Enter/Space so keyboard users can activate sorting and team focus.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/client/graphics/layers/TeamStats.ts`:
- Around line 46-47: The property _sortKey has two consecutive `@state`()
decorators; remove the duplicate so only a single `@state`() decorates the
_sortKey field (locate the declaration of _sortKey in the TeamStats class and
delete the extra `@state`() line immediately above it).
---
Duplicate comments:
In `@src/client/graphics/layers/TeamStats.ts`:
- Around line 169-172: The "gold" sort branch in TeamStats is casting
totalGoldRaw (a bigint) to Number which can lose precision; replace the
Number(...) casts in the case "gold" branch so you compare the bigint values
directly (e.g., use a BigInt-aware comparison or call compare with
a.totalGoldRaw and b.totalGoldRaw rather than
Number(a.totalGoldRaw)/Number(b.totalGoldRaw)), keeping the branch inside the
method that switches on this._sortKey.
- Around line 239-240: The sortable header cells in TeamStats.ts are only
click-driven and some team rows use class="contents", making them inaccessible
to keyboard users; update each clickable header (where `@click` calls
this.setSort("...")) to include role="button", tabindex="0" and a keydown
handler that triggers this.setSort(...) on Enter or Space, and similarly add
role="button", tabindex="0" and a keydown handler for the team-row actions (the
elements with class="contents") to invoke the existing team-selection/focus
action (the same callback used by the click handler) on Enter/Space so keyboard
users can activate sorting and team focus.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 626f261b-6396-4ac7-ad4d-c66b7e035862
📒 Files selected for processing (1)
src/client/graphics/layers/TeamStats.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/client/graphics/layers/TeamStats.ts (1)
354-360:⚠️ Potential issue | 🟠 MajorInteractive row wrapper still uses
display: contents, which breaks keyboard/AT behavior.Line 354 and Line 379 keep
class="contents"on the focusable interactive element (role+tabindex+ key handler). This pattern is unreliable for screen readers and keyboard focus. Please move interactivity to a real focusable element (e.g.,button/a) that is notdisplay: contents, or make per-cell controls focusable.#!/bin/bash # Verify focusable/interactive semantics on display: contents wrappers. rg -nP 'class="contents"[\s\S]{0,220}(role="(link|button)"|tabindex="0"|@keydown=)' src/client/graphics/layers/TeamStats.ts -C2Also applies to: 379-385
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/graphics/layers/TeamStats.ts` around lines 354 - 360, The interactive row wrapper uses class="contents" (CSS display: contents) with role="link", tabindex="0", `@click` and `@keydown` handlers (see the element wrapping rows in TeamStats.ts that calls handleTeamClick(team) and _handleKeyDown), which breaks keyboard/AT support; fix by replacing the display:contents wrapper with a real focusable element (e.g., a <button> or <a>) or move the interactive handlers onto per-cell focusable controls: create a semantic element (button or anchor) that is not display:contents, attach `@click` to handleTeamClick(team) and `@keydown` to _handleKeyDown there, remove role/tabindex from the contents class, and ensure each interactive cell has its own focusable control if row-level semantics are not appropriate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/client/graphics/layers/TeamStats.ts`:
- Around line 354-355: Prettier/style CI failed for the TeamStats component due
to formatting inconsistencies (e.g. the <div class="contents"> block and nearby
lines around the render/JSX), so run Prettier on
src/client/graphics/layers/TeamStats.ts and commit the formatting-only changes;
specifically reformat the JSX/HTML blocks (including the occurrences around the
"contents" div and the lines flagged at roughly the later JSX section) to match
the project's Prettier config and push the resulting diff.
---
Duplicate comments:
In `@src/client/graphics/layers/TeamStats.ts`:
- Around line 354-360: The interactive row wrapper uses class="contents" (CSS
display: contents) with role="link", tabindex="0", `@click` and `@keydown` handlers
(see the element wrapping rows in TeamStats.ts that calls handleTeamClick(team)
and _handleKeyDown), which breaks keyboard/AT support; fix by replacing the
display:contents wrapper with a real focusable element (e.g., a <button> or <a>)
or move the interactive handlers onto per-cell focusable controls: create a
semantic element (button or anchor) that is not display:contents, attach `@click`
to handleTeamClick(team) and `@keydown` to _handleKeyDown there, remove
role/tabindex from the contents class, and ensure each interactive cell has its
own focusable control if row-level semantics are not appropriate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 228f2117-8874-483c-a5e1-4cf22e200dc6
📒 Files selected for processing (1)
src/client/graphics/layers/TeamStats.ts
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/client/graphics/layers/TeamStats.ts (1)
362-368:⚠️ Potential issue | 🟠 MajorInteractive row wrapper still uses
display: contents, which is unreliable for accessibility.
class="contents"on a focusable/interactive container (role+tabindex+ key/click handlers) can break semantics and keyboard/screen-reader behavior. Move interaction to a real interactive element (button/a) that is notdisplay: contents.Proposed direction
- <div - class="contents" - role="link" - tabindex="0" - `@click`=${() => this.handleTeamClick(team)} - `@keydown`=${(e: KeyboardEvent) => - this._handleKeyDown(e, () => this.handleTeamClick(team))} - > - <div class="py-1.5 ... cursor-pointer ${team.isMyTeam ? "font-bold" : ""}"> - ${team.teamName} - </div> + <div class="contents"> + <button + type="button" + class="py-1.5 border-b border-slate-500 text-center hover:bg-slate-600/60 ${team.isMyTeam ? "font-bold" : ""}" + `@click`=${() => this.handleTeamClick(team)} + `@keydown`=${(e: KeyboardEvent) => + this._handleKeyDown(e, () => this.handleTeamClick(team))} + > + ${team.teamName} + </button>Apply the same pattern to the non-units branch as well.
#!/bin/bash # Verify there are no focusable/interactive wrappers still using class="contents" rg -nP 'class="contents"[\s\S]{0,200}(role=|tabindex=|@click=|@keydown=)' src/client/graphics/layers/TeamStats.ts -C2Expected result after fix: no matches where
class="contents"is also interactive.Also applies to: 400-406
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/graphics/layers/TeamStats.ts` around lines 362 - 368, The interactive wrapper uses class="contents" with role/tabindex and handlers which is inaccessible; replace the focusable/display:contents container with a real interactive element (e.g., a <button> or <a>) and attach the click and key handlers directly to it instead of using display:contents. Update the instances in TeamStats (the element that calls this.handleTeamClick(team) and this._handleKeyDown) and the similar non-units branch (the block around the 400-406 region) so the wrapper is a semantic interactive element (no class="contents") and preserve existing handler calls and ARIA role as needed.
🧹 Nitpick comments (1)
src/client/graphics/layers/TeamStats.ts (1)
82-106: Please add targeted regression tests for new interaction paths.This introduces non-trivial behavior (
setSort, keyboard activation, andGoToPlayerEventemission). Add tests for sort toggling, keyboard-triggered sorting, and team click choosing the strongest alive player.Also applies to: 171-205, 210-221
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/graphics/layers/TeamStats.ts` around lines 82 - 106, Add targeted unit/integration tests covering the new interaction paths: (1) test setSort toggles sort order when called twice for the same key and sets default "desc" when switching keys by exercising the setSort method and asserting _sortKey/_sortOrder and that updateTeamStats is invoked; (2) test keyboard activation by simulating KeyboardEvent inputs to _handleKeyDown (Enter and Space) to ensure it prevents default and invokes the provided callback; and (3) test team-click behavior that chooses the strongest alive player and emits a GoToPlayerEvent by simulating the team click UI path (or invoking the handler used by the click) on a team with mixed alive/dead players and asserting the emitted GoToPlayerEvent payload targets the strongest alive player. Include assertions for event emission and state changes and mock/stub updateTeamStats and event emitter to isolate behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/client/graphics/layers/TeamStats.ts`:
- Around line 362-368: The interactive wrapper uses class="contents" with
role/tabindex and handlers which is inaccessible; replace the
focusable/display:contents container with a real interactive element (e.g., a
<button> or <a>) and attach the click and key handlers directly to it instead of
using display:contents. Update the instances in TeamStats (the element that
calls this.handleTeamClick(team) and this._handleKeyDown) and the similar
non-units branch (the block around the 400-406 region) so the wrapper is a
semantic interactive element (no class="contents") and preserve existing handler
calls and ARIA role as needed.
---
Nitpick comments:
In `@src/client/graphics/layers/TeamStats.ts`:
- Around line 82-106: Add targeted unit/integration tests covering the new
interaction paths: (1) test setSort toggles sort order when called twice for the
same key and sets default "desc" when switching keys by exercising the setSort
method and asserting _sortKey/_sortOrder and that updateTeamStats is invoked;
(2) test keyboard activation by simulating KeyboardEvent inputs to
_handleKeyDown (Enter and Space) to ensure it prevents default and invokes the
provided callback; and (3) test team-click behavior that chooses the strongest
alive player and emits a GoToPlayerEvent by simulating the team click UI path
(or invoking the handler used by the click) on a team with mixed alive/dead
players and asserting the emitted GoToPlayerEvent payload targets the strongest
alive player. Include assertions for event emission and state changes and
mock/stub updateTeamStats and event emitter to isolate behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dd0f486b-03a9-4730-a80e-48e0292b36d3
📒 Files selected for processing (1)
src/client/graphics/layers/TeamStats.ts
Author
|
Except for tests this should be done, I am unsure how to make tests so it'll take me a bit to add them if needed. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
If this PR fixes an issue, link it below. If not, delete these two lines.
Resolves #3330
Description:
Describe the PR.
This PR adds the sortability of the player leaderboard to the team leaderboard. This allows sorting for tiles, gold, troops and the units i.e. SAMS etc. Additionally clicking on a team in the leaderboard now focuses on the largest player of that team.
This is my first pr overall so any feedback would be appreciated.
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
lakoeris