split clantag out, and some ui improvements - #4744
Conversation
|
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 PR propagates clan tags through player updates and views, adds clan-aware leaderboard columns, and refactors stats tables around typed column definitions. It also updates defaults, translations, icons, picker behavior, sorting, virtualization, and related tests. ChangesClan-aware leaderboard
Column registry
Stats table flow
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant PlayerImpl
participant PlayerUpdate
participant PlayerView
participant StatsTable
participant ColumnPicker
PlayerImpl->>PlayerUpdate: serialize clanTag
PlayerUpdate->>PlayerView: apply clanTag
PlayerView->>StatsTable: build clan-aware rows
StatsTable->>ColumnPicker: provide hideable columns and selection
ColumnPicker->>StatsTable: return columns-changed selection
StatsTable->>StatsTable: render and sort visible columns
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/client/view/PlayerView.ts (1)
58-64: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winApply later clan-tag updates to the view.
diffPlayerUpdate()now emits clan-tag-only updates, butGameView.update()routes those toapplyUpdate(), which never updatesthis.static. Existing leaderboard rows therefore keep the original clan tag.Proposed fix
applyUpdate(pu: PlayerUpdate): void { applyStateUpdate(this.state, pu); + if (pu.clanTag !== undefined) { + this.static.clanTag = pu.clanTag; + } // applyStateUpdate refreshes outgoingEmojis every tick; re-apply theAlso applies to: 441-445
🤖 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/view/PlayerView.ts` around lines 58 - 64, Update the PlayerView update path, specifically applyUpdate() and its clan-tag handling, so clan-tag-only updates emitted by diffPlayerUpdate() also replace the corresponding value in this.static. Preserve the existing staticFromUpdate() initialization and ensure leaderboard rows reflect later clan-tag changes.
🤖 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.
Outside diff comments:
In `@src/client/view/PlayerView.ts`:
- Around line 58-64: Update the PlayerView update path, specifically
applyUpdate() and its clan-tag handling, so clan-tag-only updates emitted by
diffPlayerUpdate() also replace the corresponding value in this.static. Preserve
the existing staticFromUpdate() initialization and ensure leaderboard rows
reflect later clan-tag changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f9fffad5-5dd1-44d0-8d10-ab21bb73a350
⛔ Files ignored due to path filters (2)
resources/images/GuildIconWhite.svgis excluded by!**/*.svgresources/images/TeamIconWhite.svgis excluded by!**/*.svg
📒 Files selected for processing (27)
CREDITS.mdresources/lang/en.jsonsrc/client/StatsConstants.tssrc/client/components/StatsTable.tssrc/client/hud/HotbarIcons.tssrc/client/hud/layers/PlayerStats.tssrc/client/hud/layers/TeamStats.tssrc/client/hud/layers/lib/StatsColumns.tssrc/client/render/types/Renderer.tssrc/client/view/GameView.tssrc/client/view/PlayerView.tssrc/core/game/Game.tssrc/core/game/GameUpdateUtils.tssrc/core/game/GameUpdates.tssrc/core/game/PlayerImpl.tssrc/core/game/UserSettings.tstests/ColumnPicker.test.tstests/GameLeftSidebar.test.tstests/GameUpdateUtils.test.tstests/PlayerStats.test.tstests/PlayerUpdateDiff.test.tstests/StatsColumns.test.tstests/StatsTableVirtualization.test.tstests/TeamStats.test.tstests/UserSettings.test.tstests/client/view/PlayerView.test.tstests/util/viewStubs.ts
Celant
left a comment
There was a problem hiding this comment.
Reviewed the diff with attention on the src/core changes, the column registry, and the sort/aggregate logic. I did not read all ~500 diff lines of StatsTable.ts rendering line by line — flagging that so the coverage of this review isn't overstated. All 13 checks are green.
The shape of this is good. A few things I checked that hold up:
clanTagis threaded through consistently. Added toPlayerUpdate,PlayerStatic, thePlayerinterface,PlayerImpl.toUpdate(), and — the easy one to miss — both halves ofdiffPlayerUpdate, the equality short-circuit and thesetIfDifferentlist. Missing the second would have produced a field that silently never updates.- Anonymisation is handled.
PlayerView.clanTag()mirrorsdisplayName()'s guard, so an anonymised player doesn't leak their tag through the new column. That's a real privacy edge and it would have been easy to skip. - The split is genuine, not additive.
PlayerInfo.displayNameonmainis alreadyformatPlayerDisplayName(name, clanTag), so this is pulling apart an existing concatenation rather than introducing new state. - All four new/reused
labelKeys resolve —clanandrankare added toen.json,playerandteamwere already there. - The sort logic correctly handles "no orderable column visible" by preserving
buildRowsorder rather than sorting on a missing key, and team aggregation correctly restricts itself to columns that have a number behind them.
One migration issue I'd want addressed before merge, and two smaller notes.
Description:
split out clan tag, and made it filterable
also added proper cell splits with rank/clan/player
also updated the team icon to be the same as the button, was the player icon before:

Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
w.o.n