Leaderboard update part 1: configurable columns#4453
Conversation
WalkthroughAdds persisted leaderboard column preferences to UserSettings (leaderboardColumns, setLeaderboardColumns, toggleLeaderboardColumn) with allowed-key filtering and default fallback. The Leaderboard HUD is refactored to render columns dynamically based on these preferences, generalizes sorting, adds a column settings panel, and includes matching locale strings and tests. ChangesConfigurable leaderboard columns
Sequence Diagram(s)sequenceDiagram
participant Player
participant Leaderboard
participant UserSettings
participant LocalStorage
Player->>Leaderboard: click column checkbox
Leaderboard->>Leaderboard: toggleColumn(key)
Leaderboard->>UserSettings: toggleLeaderboardColumn(key)
UserSettings->>UserSettings: filter to allowed keys
UserSettings->>LocalStorage: persist updated column list
UserSettings-->>Leaderboard: refreshed leaderboardColumns()
Leaderboard->>Leaderboard: ensureVisibleSortKey()
Leaderboard->>Leaderboard: render() with visibleColumns()
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
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 |
a059744 to
7534539
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/hud/layers/Leaderboard.ts`:
- Around line 186-202: The ranking logic in updateLeaderboard is using the full
playerViews list, so dead players can affect the computed fallback place for the
local player. Update the Leaderboard.updateLeaderboard flow to filter the
game.playerViews() result to alive players before sorting, or compute the place
from an alive-only list, so the local player’s rank is based only on active
players.
In `@tests/PackedPlayerUpdates.test.ts`:
- Around line 51-55: The PackedPlayerUpdates assertions are incorrectly using
Number(alice.gold()) for the new goldPerMinute slot, which can hide a packing
bug in PackedPlayerUpdates. Update the expectations in the affected test cases
to assert goldPerMinute with its known income value directly, using the relevant
PackedPlayerUpdates/record tuple checks instead of deriving that field from
alice.gold().
In `@tests/PlayerUpdateDiff.test.ts`:
- Around line 134-211: The new tests are bypassing the shared core-simulation
harness by constructing players and calling game.addPlayer directly. Update
these cases to use the setup() helper from tests/util/Setup.ts to create the
game instance and obtain players, keeping the assertions around PlayerInfo,
game.player, toUpdate, donateGold, and drainPackedPlayerUpdates intact. If a
special exception applies, make it explicit; otherwise route both tests through
the standard test setup pattern.
🪄 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: ef07d4b5-718a-4069-92f2-e1e7acf75e78
📒 Files selected for processing (19)
resources/lang/en.jsonsrc/client/hud/layers/Leaderboard.tssrc/client/render/types/Renderer.tssrc/client/view/GameView.tssrc/client/view/PlayerView.tssrc/core/game/Game.tssrc/core/game/GameImpl.tssrc/core/game/GameUpdateUtils.tssrc/core/game/GameUpdates.tssrc/core/game/PlayerImpl.tssrc/core/game/UserSettings.tstests/GameUpdateUtils.test.tstests/PackedPlayerUpdates.test.tstests/PlayerUpdateDiff.test.tstests/UserSettings.test.tstests/client/render/frame/derive/nuke-telegraphs.test.tstests/client/render/frame/derive/player-status.test.tstests/client/view/GameView.test.tstests/util/viewStubs.ts
|
I think this is tackling too many things at once, can you split them into multiple PRs? |
What are you referring to specifically? |
I mean, you added adjustable columns, and a gold/min, would be good to split into 2 prs |
That was the ask in the issue. Happy to split if you like, but I think adding extra columns without the ability to remove them will add clutter we don't want there |
PR 1 should be configurable columns, PR 2 will be the addition of new items You can do something like
|
ae514fe to
9b665f8
Compare
|
@ryanbarlow97 this PR now only adds the configurable columns. Will provide link to separate PR with the new columns once I have it |
Add approved & assigned issue number here:
Refs #4074
Description:
Adds a settings cog to the in-game leaderboard so players can choose which existing columns to show.
This first PR only makes the existing Owned, Gold, and Max troops columns configurable. The new leaderboard items are split into a follow-up PR.
Testing:
npx vitest tests/UserSettings.test.ts --run;npx tsc --noEmit; targeted ESLint/PrettierPlease complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
jsshap