Add Ranked 1v1 Leaderboard #3008
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
WalkthroughThis PR replaces the Changes
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;
| // If test mode is enabled, override the current user with a random player from the list | ||
| if (reset && this.testRandomUser && this.playerData.length > 0) { | ||
| const randomIndex = Math.floor(Math.random() * this.playerData.length); | ||
| this.currentUserEntry = { ...this.playerData[randomIndex] }; | ||
| console.log( | ||
| `[LeaderboardTest] Selected random user: ${this.currentUserEntry.username} (Rank ${this.currentUserEntry.rank})`, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Remove debug code before merge.
This test block includes console.log output that should not ship to production. Consider removing the entire testRandomUser feature or moving it behind a build-time flag.
Suggested fix
- // If test mode is enabled, override the current user with a random player from the list
- if (reset && this.testRandomUser && this.playerData.length > 0) {
- const randomIndex = Math.floor(Math.random() * this.playerData.length);
- this.currentUserEntry = { ...this.playerData[randomIndex] };
- console.log(
- `[LeaderboardTest] Selected random user: ${this.currentUserEntry.username} (Rank ${this.currentUserEntry.rank})`,
- );
- }📝 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.
| // If test mode is enabled, override the current user with a random player from the list | |
| if (reset && this.testRandomUser && this.playerData.length > 0) { | |
| const randomIndex = Math.floor(Math.random() * this.playerData.length); | |
| this.currentUserEntry = { ...this.playerData[randomIndex] }; | |
| console.log( | |
| `[LeaderboardTest] Selected random user: ${this.currentUserEntry.username} (Rank ${this.currentUserEntry.rank})`, | |
| ); | |
| } |
🤖 Prompt for AI Agents
In `@src/client/LeaderboardModal.ts` around lines 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.
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.
| `; | ||
| } | ||
|
|
||
| private renderClanLeaderboard() { |
There was a problem hiding this comment.
can this clan leaderboard update go in another PR? It created a regression and isn't really related to 1v1.
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.
| class="virtualizer-container h-full overflow-y-auto scrollbar-thin scrollbar-thumb-white/20 ${this | ||
| .showStickyUser | ||
| ? "pb-20" | ||
| : "pb-0"}" | ||
| @scroll=${() => this.handleScroll()} | ||
| > | ||
| ${virtualize({ | ||
| items: this.playerData, | ||
| renderItem: (p) => this.renderPlayerRow(p), | ||
| scroller: true, | ||
| })} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and read the LeaderboardModal.ts file
find . -name "LeaderboardModal.ts" -type fRepository: openfrontio/OpenFrontIO
Length of output: 99
🏁 Script executed:
# Check if the file exists and get its size
wc -l src/client/LeaderboardModal.tsRepository: openfrontio/OpenFrontIO
Length of output: 101
🏁 Script executed:
# Read the relevant section and surrounding context to understand handleScroll
sed -n '600,650p' src/client/LeaderboardModal.tsRepository: openfrontio/OpenFrontIO
Length of output: 2284
🏁 Script executed:
# Search for handleScroll implementation
grep -n "handleScroll\|maybeLoadMorePlayers" src/client/LeaderboardModal.ts | head -20Repository: openfrontio/OpenFrontIO
Length of output: 300
🏁 Script executed:
# Look for how .virtualizer-container is used
rg "virtualizer-container" src/client/LeaderboardModal.ts -A 2 -B 2Repository: openfrontio/OpenFrontIO
Length of output: 444
🏁 Script executed:
# Check package.json for `@lit-labs/virtualizer` version
cat package.json | grep -A 2 -B 2 "virtualizer"Repository: openfrontio/OpenFrontIO
Length of output: 242
🏁 Script executed:
# Read the handleScroll and maybeLoadMorePlayers implementations
sed -n '271,295p' src/client/LeaderboardModal.tsRepository: openfrontio/OpenFrontIO
Length of output: 972
🏁 Script executed:
# Read more of maybeLoadMorePlayers to see the full implementation
sed -n '276,310p' src/client/LeaderboardModal.tsRepository: openfrontio/OpenFrontIO
Length of output: 1447
🌐 Web query:
@lit-labs/virtualizer scroller option documentation behavior
💡 Result:
In @lit-labs/virtualizer, scroller is a boolean attribute/property that controls whether the virtualizer itself is the scroll container:
- Default (
scrollernot set): the virtualizer is not a scroller. It behaves like a normal block element that sizes itself to the full (virtual) height/width of all items, and it listens to scrolling on the window or a scrollable ancestor. [1] - With
scrollerenabled: the virtualizer becomes the scroll container (i.e., it scrolls itself). In this mode you should explicitly size it for your layout; it has a defaultmin-height: 150pxto avoid collapsing, but that’s usually not what you want. [1]
How to enable
<lit-virtualizer scroller ...></lit-virtualizer>[1]virtualize({ scroller: true, items, renderItem })[1]
Docs section: “scroller attribute / property” (API Reference). [1]
Sources:
[1] https://www.npmjs.com/package/%40lit-labs/virtualizer
🌐 Web query:
lit-labs virtualizer scroller: true how does it work
💡 Result:
In @lit-labs/virtualizer, scroller: true (or the scroller attribute on <lit-virtualizer>) changes which element owns the scroll:
- Default (
scrollerfalse / omitted): the virtualizer is not the scrolling container. It instead sizes itself to the full “virtual” size of the entire list (including items not in the DOM) and listens to scroll events from the window or the nearest scrollable ancestor. As you scroll, it reuses/adds/removes DOM children so only enough items to fill the viewport (plus some buffer) exist at once. [1] - With
scroller: true: the virtualizer element itself becomes the scrollable container, so you scroll the virtualizer, and native APIs likescrollTop,scrollTo(), etc. apply directly to it. You typically must give it an explicit height/size (otherwise it may collapse; it has only a small defaultmin-height). [1][2]
So, “how it works” is mostly: virtualization stays the same (render only what’s visible), but scroller: true makes the virtualizer host the scrollbar instead of delegating scrolling to an ancestor/window. [1][2]
Sources: [1] [2]
🏁 Script executed:
# Let's check the `@lit-labs/virtualizer` package to understand scroller behavior better
npm info `@lit-labs/virtualizer`@2.1.1 2>/dev/null | grep -A 5 -B 5 "description\|homepage" || cat package-lock.json 2>/dev/null | grep -A 20 '"@lit-labs/virtualizer"' | head -30Repository: openfrontio/OpenFrontIO
Length of output: 933
🏁 Script executed:
# Check if there are any other uses of virtualize in the codebase to see how scroller is typically used
rg "virtualize\(" src/ -A 3 -B 2Repository: openfrontio/OpenFrontIO
Length of output: 466
🌐 Web query:
lit-labs virtualizer scroller: true nested inside overflow-y-auto parent container
💡 Result:
If your <lit-virtualizer> is inside an overflow-y-auto parent, you typically should not set scroller: true (or the scroller attribute). By default, the virtualizer is not a scroller; it “watches” scrolling on the window or some other scrollable ancestor and virtualizes based on that. (npmjs.com)
Recommended (parent scrolls; virtualizer is not a scroller)
<div class="overflow-y-auto h-full">
<lit-virtualizer
.items=${items}
.renderItem=${renderItem}
></lit-virtualizer>
</div>If you really want scroller: true (virtualizer scrolls), don’t nest it in another overflow-y-auto
When scroller is set, the virtualizer itself becomes the scrolling element, so nesting it inside another scroll container usually creates competing/nested scrolling. Also, you should explicitly size the virtualizer (e.g., fixed height / flex layout). (npmjs.com)
Citations:
- 1: https://www.npmjs.com/package/%40lit-labs/virtualizer?utm_source=openai
- 2: https://www.npmjs.com/package/%40lit-labs/virtualizer?utm_source=openai
🌐 Web query:
lit virtualizer scroller true inside scrollable parent element mismatch
💡 Result:
If you put <lit-virtualizer scroller> inside another scrollable container, you’ll often see “mismatch” behavior (wrong viewport/offset calculations, items rendering “late/early”, blank gaps) because you’ve effectively created two competing scroll contexts.
Use one of these patterns:
A) Scrollable parent (recommended): don’t set scroller
Let the ancestor be the scroller; the virtualizer will react to scrolling of “some other scrollable ancestor” by design. [1]
<div class="panel">
<lit-virtualizer
.items=${items}
.renderItem=${renderItem}>
</lit-virtualizer>
</div>.panel { height: 400px; overflow: auto; }B) Virtualizer is the scroller: set scroller, but then size it explicitly
If you set scroller, the virtualizer becomes the scrolling element, and Lit notes you should explicitly size it (the default min-height is just a fallback). [1]
<lit-virtualizer
scroller
style="height:400px"
.items=${items}
.renderItem=${renderItem}>
</lit-virtualizer>C) If you must keep an outer container, wire the scroll element explicitly (directive / older API)
If you’re using the directive / API that supports it, set scrollTarget to the actual scrolling element so events/measurements come from the right place. [2]
Rule of thumb: if there’s a scrollable parent, remove scroller; if you want the virtualizer to scroll, remove the parent’s overflow:auto and give the virtualizer a height. [1]
Sources: [1] [2]
Remove scroller: true or restructure to avoid competing scroll containers.
The virtualizer with scroller: true creates its own scroll context, but it's nested inside .virtualizer-container which has overflow-y-auto. This creates two competing scroll controllers, causing misalignment between scroll events and actual scroll position.
Choose one approach:
- Remove
scroller: true— let the parent.virtualizer-containerbe the sole scroll container. The virtualizer will react to its ancestor's scrolling by design. - Remove
overflow-y-autofrom parent and give the virtualizer explicit height. Thenscroller: trueworks correctly as the single scroll owner.
The current setup causes handleScroll() to read from .virtualizer-container, but the virtualizer's internal position may not match, breaking infinite loading and sticky user visibility.
🤖 Prompt for AI Agents
In `@src/client/LeaderboardModal.ts` around lines 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.
| if (this.clanData) { | ||
| const start = new Date(this.clanData.start).toLocaleDateString(); | ||
| const end = new Date(this.clanData.end).toLocaleDateString(); | ||
| dateRange = html`<span | ||
| class="text-sm font-normal text-white/40 ml-2 wrap-break-words" | ||
| >(${start} - ${end})</span | ||
| >`; |
There was a problem hiding this comment.
Date range can shift by one day in many time zones.
toLocaleDateString() uses local time. If the API sends UTC midnight, users in the US will see the previous date. Consider rendering in UTC or using a date-only value from the server.
✅ Possible fix
- const start = new Date(this.clanData.start).toLocaleDateString();
- const end = new Date(this.clanData.end).toLocaleDateString();
+ const start = new Date(this.clanData.start).toLocaleDateString(undefined, {
+ timeZone: "UTC",
+ });
+ const end = new Date(this.clanData.end).toLocaleDateString(undefined, {
+ timeZone: "UTC",
+ });📝 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.
| if (this.clanData) { | |
| const start = new Date(this.clanData.start).toLocaleDateString(); | |
| const end = new Date(this.clanData.end).toLocaleDateString(); | |
| dateRange = html`<span | |
| class="text-sm font-normal text-white/40 ml-2 wrap-break-words" | |
| >(${start} - ${end})</span | |
| >`; | |
| if (this.clanData) { | |
| const start = new Date(this.clanData.start).toLocaleDateString(undefined, { | |
| timeZone: "UTC", | |
| }); | |
| const end = new Date(this.clanData.end).toLocaleDateString(undefined, { | |
| timeZone: "UTC", | |
| }); | |
| dateRange = html`<span | |
| class="text-sm font-normal text-white/40 ml-2 wrap-break-words" | |
| >(${start} - ${end})</span | |
| >`; |
🤖 Prompt for AI Agents
In `@src/client/LeaderboardModal.ts` around lines 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.
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