Conversation
## Description: 1v1 ranked leaderboard. - Renames Stats to Leaderboard - Merges Clans and Users into the same Modal <img width="765" height="744" alt="image" src="https://github.com/user-attachments/assets/e49cb1f6-3551-4045-9681-b847d473ccd3" /> ## Please complete the following: - [X] I have added screenshots for all UI updates - [X] I process any text displayed to the user through translateText() and I've added it to the en.json file - [X] I have added relevant tests to the test directory - [X] I confirm I have thoroughly tested these changes and take full responsibility for any bugs introduced ## Please put your Discord username so you can be contacted if a bug or regression is found: wraith4081
#2979) ## Description: update visuals of current user section in leaderboard modal ## Please complete the following: - [X] I have added screenshots for all UI updates - [X] I process any text displayed to the user through translateText() and I've added it to the en.json file - [X] I have added relevant tests to the test directory - [X] I confirm I have thoroughly tested these changes and take full responsibility for any bugs introduced ## Please put your Discord username so you can be contacted if a bug or regression is found: wraith4081
WalkthroughReplaces the legacy Stats modal with a new Leaderboard feature: adds LeaderboardModal and three leaderboard components, new player/clan/ranked schemas and API fetchers, updates UI entry points/translations/markup, removes StatsModal and its tests, and adds comprehensive tests for the new leaderboard UI. Changes
Sequence Diagram(s)sequenceDiagram
participant User as Client/User
participant Modal as LeaderboardModal<br/>rgba(70,130,180,0.5)
participant PlayerList as LeaderboardPlayerList<br/>rgba(34,139,34,0.5)
participant ClanTable as LeaderboardClanTable<br/>rgba(255,165,0,0.5)
participant API as Api.fetch*()<br/>rgba(148,0,211,0.5)
participant Server as Server/Backend
User->>Modal: open leaderboard
Modal->>PlayerList: activate players tab (load)
PlayerList->>API: fetchPlayerLeaderboard(page)
API->>Server: GET /leaderboard/ranked?page=...
Server-->>API: ranked payload / 400
API-->>PlayerList: parsed RankedLeaderboardResponse / reached_limit
PlayerList-->>Modal: currentUserEntry / paging state
User->>Modal: switch to clans tab
Modal->>ClanTable: activate clans tab (load)
ClanTable->>API: fetchClanLeaderboard()
API->>Server: GET /public/clans/leaderboard
Server-->>API: clans payload
API-->>ClanTable: parsed ClanLeaderboardResponse
ClanTable-->>Modal: date-range-change event
Modal-->>User: render updated UI
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ 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 |
|
@ryanbarlow97 Please remove the json language files other than en.json |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/client/LeaderboardModal.ts`:
- Around line 179-186: Remove the debug console output and test-only behavior
from the LeaderboardModal by eliminating or gating the block that checks reset
&& this.testRandomUser && this.playerData.length > 0; specifically remove the
console.log call and either delete the entire testRandomUser override of
this.currentUserEntry or wrap it behind a build-time flag (e.g.,
process.env.NODE_ENV === 'test' or a dedicated feature flag) so production
builds never execute the random user selection; update any references to
testRandomUser or add a clear comment if you keep a gated test path.
🧹 Nitpick comments (5)
tests/client/LeaderboardModal.test.ts (2)
98-101: Consider exposing test-friendly accessors.The heavy use of
as unknown as { ... }casts to access private members works but is fragile. If the component's internal structure changes, these tests will fail silently at runtime rather than at compile time.A cleaner approach: expose a
protectedorpublictest interface, or use a helper function that centralizes these casts:// Helper to reduce duplication and centralize casts function getModalInternals(modal: LeaderboardModal) { return modal as unknown as { activeTab: string; playerData: Array<Record<string, unknown>>; currentUserEntry?: { playerId: string } | null; isModalOpen: boolean; loadClanLeaderboard: () => Promise<void>; loadPlayerLeaderboard: (reset: boolean) => Promise<void>; }; }This is optional since the current approach works.
253-317: Good error and empty state coverage.The interaction tests cover tab switching, empty data state, and error state well. Consider adding a similar test for the players tab interaction (the current tests only cover clan tab switching explicitly, while player data loading is tested separately in "Player Data Mapping").
src/client/LeaderboardModal.ts (3)
113-124: String-based error detection is fragile.Checking
errorJson?.message?.includes("Page must be between")couples tightly to server error wording. If the message changes, pagination breaks silently.Consider having the API return an explicit field (e.g.,
hasMore: false) or a 204 status instead.
227-254: Add explicit handling for "rank" sort.The
defaultcase returns0, which keeps the original API order. This works, but an explicitcase "rank"with a comment would make the intent clearer.Suggested change
switch (this.clanSortBy) { + case "rank": + // Keep original API order (already sorted by rank) + return 0; case "games": aVal = a.games; bVal = b.games;
504-509: Use existingplayer.winRateinstead of recalculating.Line 507 recalculates
winRatefromplayer.gamesandplayer.wins, butPlayerLeaderboardEntryalready has awinRatefield computed at fetch time (line 140).Suggested fix
private renderPlayerRow(player: PlayerLeaderboardEntry) { const isCurrentUser = this.currentUserEntry?.playerId === player.playerId; const displayRank = player.rank; - const winRate = player.games > 0 ? player.wins / player.games : 0; + const winRate = player.winRate;
github-project-automation
bot
moved this from Triage
to Development
in OpenFront Release Management
@wraith4081 do you have org yet? |
Actually, just checked, this is updating the header of a group, not adding any langs |
It can still cause some unneeded issue. Hypothetically if tomorrow two updated languages are fetched from Crowdin and merged here, they won't yet contain the new key. It's best to keep one source of truth to prevent mistakes or bugs or unnecessary extra work :) |
its not my branch really, i'd rather @wraith4081 could do it with org |
Aotumuri
left a comment
Aotumuri
left a comment
There was a problem hiding this comment.
Could you revert the changes to the non-English files?
I understand that the tags have changed, so this might be the correct approach from a technical point of view.
However, keeping these files unchanged is important for us, because otherwise the synchronization between Crowdin and this repository will break.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/client/LeaderboardModal.ts`:
- Around line 763-769: The date-range rendering in LeaderboardModal (the
clanData block that builds dateRange) uses new Date(...).toLocaleDateString(),
which applies local timezone and can shift dates; change it to treat the server
values as UTC/date-only before formatting (e.g., parse the clanData.start and
clanData.end as UTC or use Date.UTC/getUTCDate/getUTCMonth/getUTCFullYear or
format from an ISO date-only string) and then produce a UTC-based date string
for display so users in other time zones see the intended dates.
- Around line 621-631: The virtualizer is creating a second scroll context by
using scroller: true inside the .virtualizer-container which also has
overflow-y-auto, causing scroll position mismatches; fix by removing scroller:
true from the virtualize() call so the parent .virtualizer-container is the
single scroll owner (adjust any logic in handleScroll() that reads the container
scroll to continue using the parent container), or alternatively remove
overflow-y-auto from .virtualizer-container and give the virtualizer a fixed
height so scroller: true becomes the single scroll container — update the
virtualize call in the render where items: this.playerData, renderItem: (p) =>
this.renderPlayerRow(p) is invoked and ensure handleScroll() and showStickyUser
logic reference the actual scroll container you choose.
♻️ Duplicate comments (1)
src/client/LeaderboardModal.ts (1)
159-166: Remove test-only random user override before merge.This changes the current user and logs to console in production. Please delete it or gate it behind a test-only flag.
🧹 Nitpick comments (1)
src/client/Api.ts (1)
229-235: Avoid brittle string matching for pagination end.This relies on
message.includes("Page must be between"). If server text changes, the client will never detect end-of-list and will keep erroring. Prefer a structured error code or a dedicated status (e.g., 416/204) and check that instead.
wraith4081
dismissed stale reviews from coderabbitai[bot] and Aotumuri
via
9fc7f5d
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@index.html`:
- Around line 197-201: Remove the legacy stats-modal element (the custom element
with id "page-stats" and tag name "stats-modal") that was left in the markup
after renaming to leaderboard-modal; locate the unused <stats-modal
id="page-stats" ...> block and delete it so only the new <leaderboard-modal
id="page-leaderboard" ...> remains, ensuring there are no leftover duplicates or
unused IDs in the document.
In `@tests/client/LeaderboardModal.test.ts`:
- Around line 320-344: The test asserts the fetch call too early because
loadActiveTabData() waits on modal.updateComplete before calling fetch; after
dispatching the click on "#clan-leaderboard-tab" you should await the component
updates (await modal.updateComplete and await
awaitChildUpdate("leaderboard-clan-table") or at least a microtask tick) before
asserting global.fetch was called and before further checks of load results;
update the test to move the expect(global.fetch).toHaveBeenCalledWith(...) and
any data checks to after those awaits so they run only after loadActiveTabData()
triggers fetch.
🧹 Nitpick comments (2)
tests/client/LeaderboardModal.test.ts (1)
79-95: Avoid double fetch stubs.The top-level fetch stub is replaced by the inner
beforeEach, so it adds noise. Consider keeping only one.♻️ Optional cleanup
-beforeEach(() => { - vi.stubGlobal( - "fetch", - vi.fn(async (input: any) => { - const url = - typeof input === "string" ? input : (input?.url ?? String(input)); - - if (url.includes("/public/clans/leaderboard")) { - return jsonRes({ start: "...", end: "...", clans: [] }); - } - if (url.includes("/leaderboard/ranked")) { - return jsonRes({ "1v1": [] }); - } - return jsonRes({}, false, 404); - }), - ); -});src/client/components/leaderboard/LeaderboardTabs.ts (1)
21-71: Avoid derived state for tab classes.
playerClass/clanClassare derived fromactiveTabbut only updated on clicks, so they can drift if the parent setsactiveTab. Compute classes fromactiveTabinstead.♻️ Suggested refactor
- `@state`() - private playerClass = this.getTabClass(this.activeTab === "players"); - `@state`() - private clanClass = this.getTabClass(this.activeTab === "clans"); - private handleTabChange(tab: LeaderboardTab) { + this.activeTab = tab; this.dispatchEvent( new CustomEvent<LeaderboardTab>("tab-change", { detail: tab, bubbles: true, composed: true, }), ); - - this.playerClass = this.getTabClass(tab === "players"); - this.clanClass = this.getTabClass(tab === "clans"); } render() { + const playerClass = this.getTabClass(this.activeTab === "players"); + const clanClass = this.getTabClass(this.activeTab === "clans"); return html` <div role="tablist" class="flex gap-2 p-1 bg-white/5 rounded-full border border-white/10 mb-6 w-fit mx-auto mt-4" > <button type="button" role="tab" - class="${this.playerClass}" + class="${playerClass}" `@click`=${() => this.handleTabChange("players")} id="player-leaderboard-tab" aria-selected=${this.activeTab === "players"} > ${translateText("leaderboard_modal.ranked_tab")} </button> <button type="button" role="tab" - class="${this.clanClass}" + class="${clanClass}" `@click`=${() => this.handleTabChange("clans")} id="clan-leaderboard-tab" aria-selected=${this.activeTab === "clans"} > ${translateText("leaderboard_modal.clans_tab")} </button> </div> `; }
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@index.html`:
- Line 143: The Tailwind classes z-40001 and z-99999 are invalid and won't
generate CSS; update the markup where the element with class string containing
"peer in-[.in-game]:hidden z-40001 ... z-[?]" (the sidebar/menu element) to use
arbitrary value bracket syntax: replace z-40001 with z-[40001] (and any z-99999
occurrences elsewhere) so Tailwind recognizes and emits those custom z-index
values.
🧹 Nitpick comments (1)
src/client/LeaderboardModal.ts (1)
12-14: Prefer composition over extendingBaseModal.Extending
BaseModalbuilds a class hierarchy. If possible, wrap a modal instance or delegate behavior so the component stays composable (and use union types for variants).
github-project-automation
bot
moved this from Development
to Final Review
in OpenFront Release Management
github-project-automation
bot
moved this from Final Review
to Complete
in OpenFront Release Management
If this PR fixes an issue, link it below. If not, delete these two lines. Resolves #(issue number) @wraith4081 's pr updates the stats modal to show both 1v1 and clan stats - [x] I have added screenshots for all UI updates - [x] I process any text displayed to the user through translateText() and I've added it to the en.json file - [x] I have added relevant tests to the test directory - [x] I confirm I have thoroughly tested these changes and take full responsibility for any bugs introduced regression is found: w.o.n --------- Co-authored-by: Wraith <54374743+wraith4081@users.noreply.github.com> Co-authored-by: iamlewis <lewismmmm@gmail.com>
If this PR fixes an issue, link it below. If not, delete these two lines. Resolves #(issue number) @wraith4081 's pr updates the stats modal to show both 1v1 and clan stats - [x] I have added screenshots for all UI updates - [x] I process any text displayed to the user through translateText() and I've added it to the en.json file - [x] I have added relevant tests to the test directory - [x] I confirm I have thoroughly tested these changes and take full responsibility for any bugs introduced regression is found: w.o.n --------- Co-authored-by: Wraith <54374743+wraith4081@users.noreply.github.com> Co-authored-by: iamlewis <lewismmmm@gmail.com>
If this PR fixes an issue, link it below. If not, delete these two lines.
Resolves #(issue number)
Description:
@wraith4081 's pr
updates the stats modal to show both 1v1 and clan stats
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
w.o.n