Add configurable leaderboard income columns#4453
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds a rolling gold-per-minute stat on the server, carries it through player update paths, and lets the leaderboard show configurable columns saved in user settings. ChangesgoldPerMinute stat and configurable leaderboard columns
Sequence Diagram(s)sequenceDiagram
participant PlayerImpl
participant GameUpdateUtils
participant GameView
participant PlayerView
participant Leaderboard
PlayerImpl->>GameUpdateUtils: toUpdate() / toFullUpdate()
GameUpdateUtils-->>GameView: packedPlayerUpdates + PlayerUpdate
GameView->>PlayerView: stateFromUpdate()
PlayerView-->>Leaderboard: goldPerMinute(), tiles, gold, troops
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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
|
Add approved & assigned issue number here:
Resolves #4074
Description:
Adds gold/min and configurable scoreboard columns. Adds optional cities column.
Testing:
npm testPlease complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
jsshap