feat(leaderboard): add the 2v2 ranked ladder as its own tab - #4763
Merged
Conversation
/leaderboard/ranked returns both ladders in one payload, but the response schema declared only the 1v1 key, so the 2v2 rows were dropped at the parse boundary and the modal rendered a single board. The two ladders are ranked independently — the same public_id can hold a different elo, record and rank on each — so the player list now keeps one LadderState per RankedType and renders whichever its `rankedType` property selects. Both ladder tabs share one element: a page fetch fills every ladder, so switching tabs neither refetches nor waits. Paging is per ladder rather than inferred from the 1v1 page size. The 2v2 board is younger and has no backfill, so it ends several pages before 1v1 does; tracking one flag per ladder stops the short board from asking for pages that will come back empty. An empty ladder now renders the no-data state instead of a bare header row, which is what a freshly launched ranked type shows until games are played. The tablist wraps: this is the fourth tab, and four uppercase labels overflow a narrow phone with nothing to scroll. The 2v2 key is defaulted in the schema so a client on this build keeps working against an API deployment that predates the ladder. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe leaderboard modal adds a 2v2 ranked tab. Player-list state, pagination, current-user rows, empty states, API parsing, translations, layout, and tests now support separate 1v1 and 2v2 ladders. ChangesRanked leaderboard support
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant LeaderboardModal
participant LeaderboardPlayerList
participant RankedLeaderboardResponseSchema
LeaderboardModal->>LeaderboardPlayerList: select 1v1 or 2v2 rankedType
LeaderboardPlayerList->>RankedLeaderboardResponseSchema: parse leaderboard response
RankedLeaderboardResponseSchema-->>LeaderboardPlayerList: provide ladder-specific entries
LeaderboardPlayerList-->>LeaderboardModal: render selected ladder and paging state
Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
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.
🧹 Nitpick comments (2)
tests/client/LeaderboardModal.test.ts (1)
163-171: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
showLaddersetsactiveTabdirectly, so the tab-change path is not exercised.Assigning
activeTabskipsonTabEnter→loadActiveTabData→ensureLoaded, which is exactly the code that must not refetch. The "switching tabs must not refetch" assertion at line 591 therefore proves less than it reads. Driving the switch through the modal's tab handler (as the last test does with a real click) would cover the guard onhasLoadedPlayerstoo.Also applies to: 578-591
🤖 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 `@tests/client/LeaderboardModal.test.ts` around lines 163 - 171, Update the showLadder helper to switch tabs through the modal’s actual tab-change handler rather than assigning activeTab directly, using the same event or click path as the final test. Preserve its waiting for modal.updateComplete and playerList.updateComplete so the switching-tabs assertion exercises onTabEnter, loadActiveTabData, and ensureLoaded, including the hasLoadedPlayers guard.src/client/LeaderboardModal.ts (1)
19-22: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueType the map as partial so the
?? nullfallback is honest.
Record<string, RankedType>tells TypeScript every string key yields aRankedType, soPLAYER_TABS[tab] ?? nulllooks dead to readers and to lint rules, even though at runtime it is the whole point. A partial record makes the lookup type match reality.♻️ Proposed tweak
-const PLAYER_TABS: Record<string, RankedType> = { +const PLAYER_TABS: Partial<Record<string, RankedType>> = { players: RankedType.OneVOne, players2v2: RankedType.TwoVTwo, };🤖 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/LeaderboardModal.ts` around lines 19 - 22, Update the PLAYER_TABS declaration to use a partial record keyed by string, so lookups for unknown tabs correctly remain potentially undefined and the existing ?? null fallback is type-accurate.
🤖 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.
Nitpick comments:
In `@src/client/LeaderboardModal.ts`:
- Around line 19-22: Update the PLAYER_TABS declaration to use a partial record
keyed by string, so lookups for unknown tabs correctly remain potentially
undefined and the existing ?? null fallback is type-accurate.
In `@tests/client/LeaderboardModal.test.ts`:
- Around line 163-171: Update the showLadder helper to switch tabs through the
modal’s actual tab-change handler rather than assigning activeTab directly,
using the same event or click path as the final test. Preserve its waiting for
modal.updateComplete and playerList.updateComplete so the switching-tabs
assertion exercises onTabEnter, loadActiveTabData, and ensureLoaded, including
the hasLoadedPlayers guard.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 27d930b8-4495-4c92-8025-a3e744aa640b
📒 Files selected for processing (6)
resources/lang/en.jsonsrc/client/LeaderboardModal.tssrc/client/components/baseComponents/Modal.tssrc/client/components/leaderboard/LeaderboardPlayerList.tssrc/core/ApiSchemas.tstests/client/LeaderboardModal.test.ts
The blanket mockResolvedValue also answered the clan and tribe prefetches fired by the tab click, feeding them ladder-shaped JSON that crashed their renders after the test ended (unhandled rejection, vitest exit 1). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
github-project-automation
Bot
moved this from Triage
to Complete
in OpenFront Release Management
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.
Add approved & assigned issue number here:
n/a — owner-initiated, no tracking issue.
Description:
GET /leaderboard/rankedreturns both ranked ladders in one payload, butRankedLeaderboardResponseSchemadeclared only the1v1key, so the 2v2 rows were dropped at the parse boundary and the modal rendered a single board. This adds the 2v2 ladder as its own tab.RankedLeaderboardResponseSchemanow declares2v2as well, defaulted to[]so a client on this build keeps working against an API deployment that predates the ladder.public_idcan hold a different elo, record and rank on each), soLeaderboardPlayerListkeeps oneLadderStateperRankedTypeand renders whichever its newrankedTypeproperty selects. That includes the pinned "your ranking" row, which now shows your rank on the ladder you are looking at.<leaderboard-player-list>; a page fetch fills every ladder, so switching tabs neither refetches nor waits.hasMoreis tracked per ladder instead of being inferred from the 1v1 page size. The 2v2 board is younger and has no backfill, so it ends several pages before 1v1 does, and the short board stops asking for pages that come back empty.o-modal's tablist now wraps. This is the fourth tab, and four uppercase labels overflow a narrow phone with nothing to scroll. The class only takes effect in the overflow case that previously clipped.tab=playersstill maps to the 1v1 ladder, so existing#modal=leaderboard&tab=playerslinks and the profile modal's back button are unaffected. The new tab key isplayers2v2.Please complete the following:
Not verified locally: this was written in an environment with no npm registry access, so it has not been built, typechecked, or test-run — no screenshots for the same reason. Please let CI (or a local
npm test+npm run build-prod) be the first real check.New tests in
tests/client/LeaderboardModal.test.tscover: a player ranked separately on each ladder, one request filling both, the no-data state for an empty ladder, independent paging, and the tab selecting the 2v2 ladder.Please put your Discord username so you can be contacted if a bug or regression is found:
DISCORD_USERNAME
🤖 Generated with Claude Code