Fix that player info was not displayed on side window when opening a private channel #3426
Conversation
WalkthroughReplaces ChatChannelUser-based side-panel wiring with PlayerInfo via PlayerService: controllers now bind to an observable PlayerInfo, avatar loading and rating/achievement lookups use PlayerInfo and AvatarService, and tests are updated to mock PlayerService and use the new playerProperty model. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as UI (PrivateChatTab)
participant TabCtrl as PrivateChatTabController
participant PlayerSvc as PlayerService
participant InfoCtrl as PrivatePlayerInfoController
participant Avatar as AvatarService
Note over TabCtrl,PlayerSvc: New flow: resolve online PlayerInfo
UI->>TabCtrl: open private chat for playerName
TabCtrl->>PlayerSvc: getPlayerByNameIfOnline(playerName)
alt player online
PlayerSvc-->>TabCtrl: Optional<PlayerInfo>
TabCtrl->>InfoCtrl: playerProperty.set(playerInfo)
InfoCtrl->>InfoCtrl: subscribe to playerProperty changes
InfoCtrl->>Avatar: load avatar for playerInfo
Avatar-->>InfoCtrl: avatar image
InfoCtrl->>InfoCtrl: load ratings & achievements (async)
InfoCtrl-->>UI: update side-panel UI
else player offline/absent
PlayerSvc-->>TabCtrl: Optional.empty
TabCtrl->>InfoCtrl: playerProperty.set(null)
InfoCtrl->>InfoCtrl: clear UI / show fallback
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
🔇 Additional comments (11)
Comment |
src/main/java/com/faforever/client/player/PrivatePlayerInfoController.java
Outdated
Show resolved
Hide resolved
Marc-Spector
force-pushed
the
fix/show-player-info-on-side-window
branch
from
876d1bc to
61f23f9
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
src/test/java/com/faforever/client/chat/PrivateChatTabControllerTest.java (1)
100-107: Tighten stubbing; verify own vs peer username.
- Prefer strict stubs to surface unintended interactions; keep lenient only where necessary (e.g., theme file URL).
- Also consider returning a different value from chatService.getCurrentUsername() than playerName to mirror a real DM (self vs peer), unless this test relies on them matching.
src/test/java/com/faforever/client/player/PrivatePlayerInfoControllerTest.java (1)
198-201: Assert side-effect triggered by ‘when(showing)’ gating.This test relies on player.when(showing) having fired to call getPlayerAchievements(...). If flakiness appears, explicitly ensure the node is “showing” (attach to a Scene) before verification.
src/main/java/com/faforever/client/player/PrivatePlayerInfoController.java (2)
102-108: Dispose subscription to avoid leaks across tab lifecycles.player.when(showing).subscribe(...) isn’t retained; repeated show/hide or controller reuse can accumulate subscribers. Store and dispose in onDestroy(), or compose with a lifecycle operator (e.g., takeUntil(hiding)) if available.
Example:
@@ - player.when(showing).subscribe((playerInfo) -> { + this.playerSubscription = player.when(showing).subscribe(playerInfo -> { if (playerInfo != null) { loadReceiverRatingInformation(playerInfo); populateUnlockedAchievementsLabel(playerInfo); } });And:
+ private reactor.core.Disposable playerSubscription; + + @Override + protected void onDestroy() { + if (playerSubscription != null) { + playerSubscription.dispose(); + } + }
122-125: Use parameterized logging for clarity.Close the quote and use placeholders.
- throwable -> log.error("Could not load achievements for player '" + player.getId(), throwable)); + throwable -> log.error("Could not load achievements for player '{}' ", player.getId(), throwable));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/main/java/com/faforever/client/chat/PrivateChatTabController.java(4 hunks)src/main/java/com/faforever/client/player/PrivatePlayerInfoController.java(3 hunks)src/test/java/com/faforever/client/chat/PrivateChatTabControllerTest.java(4 hunks)src/test/java/com/faforever/client/player/PrivatePlayerInfoControllerTest.java(7 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Codacy Static Code Analysis
src/test/java/com/faforever/client/player/PrivatePlayerInfoControllerTest.java
Show resolved
Hide resolved
src/test/java/com/faforever/client/player/PrivatePlayerInfoControllerTest.java
Outdated
Show resolved
Hide resolved
|
@coderabbitai resolve |
✅ Actions performedComments resolved. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/test/java/com/faforever/client/player/PrivatePlayerInfoControllerTest.java (1)
3-17: Add missing import for LeaderboardRating (compilation fix).
LeaderboardRatingis referenced but not imported; tests won’t compile.Apply:
import com.faforever.client.domain.api.Leaderboard; +import com.faforever.client.domain.api.LeaderboardRating; import com.faforever.client.domain.server.PlayerInfo;
🧹 Nitpick comments (4)
src/test/java/com/faforever/client/player/PrivatePlayerInfoControllerTest.java (4)
53-55: Makeplayerfield private.Minor test hygiene: keep fields private to the class. No behavior change.
- ObjectProperty<PlayerInfo> player; + private ObjectProperty<PlayerInfo> player;
72-75: Future‑proof i18n stub against method-name drift.Controller may call
getWithDefault(...)instead ofgetOrDefault(...). Stub both to avoid brittle tests.- lenient().when(i18n.getOrDefault(leaderboard.technicalName(), leaderboard.nameKey())) - .thenReturn(leaderboard.technicalName()); + lenient().when(i18n.getOrDefault(leaderboard.technicalName(), leaderboard.nameKey())) + .thenReturn(leaderboard.technicalName()); + lenient().when(i18n.getWithDefault(leaderboard.technicalName(), leaderboard.nameKey())) + .thenReturn(leaderboard.technicalName());If
getWithDefaultdoesn’t exist on I18n in this branch, skip this addition.
139-143: Strengthen verification to ensure the change triggers the call (not prior setup).
verify(leaderboardService).getLeaderboards()can pass due to earlier invocations. Reset/clear before the action and assert one call, or use times.@Test public void testRatingChange() { - runOnFxThreadAndWait(() -> playerInfo.setLeaderboardRatings(Map.of("test", Instancio.create(LeaderboardRating.class)))); - verify(leaderboardService).getLeaderboards(); + org.mockito.Mockito.clearInvocations(leaderboardService); + runOnFxThreadAndWait(() -> + playerInfo.setLeaderboardRatings( + Map.of(leaderboard.technicalName(), Instancio.create(LeaderboardRating.class)))); + verify(leaderboardService, org.mockito.Mockito.times(1)).getLeaderboards(); }This also aligns the map key with the mocked leaderboard name.
195-198: Make achievement-call verify resilient to async.Guard against timing flakiness by using Mockito’s timeout.
- verify(achievementService).getPlayerAchievements(playerInfo.getId()); + verify(achievementService, org.mockito.Mockito.timeout(1000)) + .getPlayerAchievements(playerInfo.getId());Optionally add a static import for
timeout.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/test/java/com/faforever/client/player/PrivatePlayerInfoControllerTest.java(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/test/java/com/faforever/client/player/PrivatePlayerInfoControllerTest.java (1)
src/main/java/com/faforever/client/chat/PrivateUserInfoController.java (3)
Component(30-178)leaderboards(161-176)leaderboard(164-171)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (9)
src/test/java/com/faforever/client/player/PrivatePlayerInfoControllerTest.java (9)
60-67: Good: bind once, mutate the source property.Binding
instance.playerProperty()toplayerand mutatingplayerin tests avoids “bound value cannot be set” errors observed earlier.
89-93: LGTM on helper; once import is added, this builds cleanly.
LeaderboardRatingMapBuilder+Instanciousage is fine.
95-111: UI state assertions for idle player look solid.Covers visibility, labels, and values. Nice.
114-118: LGTM for game-in-progress path.Setting
GameStatus.PLAYINGand asserting wrapper visibility is appropriate.
121-127: LGTM for no‑game → joins‑game transition.
130-136: LGTM for leaves‑game transition.
146-157: LGTM for null‑player visibility checks.
160-192: LGTM for null → new player rebind flow.Good coverage of visibility and label content after rebind.
200-203: LGTM getRoot test.
Sheikah45
left a comment
Sheikah45
left a comment
There was a problem hiding this comment.
Do you know why the player property in chatUser was unreliable?
|
The The downlords-faf-client/src/main/java/com/faforever/client/chat/ChatChannel.java Lines 144 to 146 in 9e64010 The player will be on the list when he responds to your messages or you have a message history (I have not tested this case) |
Sheikah45
force-pushed
the
fix/show-player-info-on-side-window
branch
from
f96ff73 to
b7490ff
Compare
Closes #3423
Summary by CodeRabbit