Only send vetoes on player action #3436
Conversation
WalkthroughRefactors veto handling: adds a VetoKey factory, replaces incremental applied-veto updates with a full-map replacement, changes setTokensForMap signature and behavior, and updates controllers and tests to use the new APIs and improved null-safety. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as MapTileController
participant Key as VetoKey
participant Svc as TeamMatchmakingService
participant Store as AppliedVetoes Map
UI->>UI: compute local assignment (null-safe)
UI->>Key: VetoKey.of(assignment)
Key-->>UI: VetoKey
UI->>Svc: setTokensForMap(vetoKey, tokens)
activate Svc
alt tokens > 0
Svc->>Store: put(vetoKey, tokens)
else tokens == 0
Svc->>Store: remove(vetoKey)
end
Svc->>Svc: sendVetoes() if map changed
deactivate Svc
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 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)
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}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/main/java/com/faforever/client/preferences/VetoKey.java(1 hunks)src/main/java/com/faforever/client/teammatchmaking/TeamMatchmakingMapTileController.java(7 hunks)src/main/java/com/faforever/client/teammatchmaking/TeamMatchmakingService.java(5 hunks)src/test/java/com/faforever/client/teammatchmaking/TeamMatchmakingMapTileControllerTest.java(0 hunks)src/test/java/com/faforever/client/teammatchmaking/TeamMatchmakingServiceTest.java(1 hunks)
💤 Files with no reviewable changes (1)
- src/test/java/com/faforever/client/teammatchmaking/TeamMatchmakingMapTileControllerTest.java
🧰 Additional context used
🧬 Code graph analysis (1)
src/main/java/com/faforever/client/teammatchmaking/TeamMatchmakingMapTileController.java (1)
src/main/java/com/faforever/client/fx/NodeController.java (1)
NodeController(8-35)
🪛 GitHub Actions: Checks
src/test/java/com/faforever/client/teammatchmaking/TeamMatchmakingServiceTest.java
[error] 644-644: Mockito TooManyActualInvocations: fafServerAccessor.setPlayerVetoes(...) called 2 times, but expected to be called 1 time in testSendVetoesOnVetoesChange.
src/main/java/com/faforever/client/teammatchmaking/TeamMatchmakingService.java
[error] 269-269: TooManyActualInvocations: sendVetoes(...) called >1 times for setPlayerVetoes; expected 1, but was 2 (observed at TeamMatchmakingService.java:269 and FafServerAccessor.java:338).
⏰ 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). (2)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: checks
🔇 Additional comments (6)
src/main/java/com/faforever/client/preferences/VetoKey.java (1)
10-12: LGTM! Clean factory method.The static factory method centralizes VetoKey creation from MapPoolAssignment, improving consistency across the codebase.
src/main/java/com/faforever/client/teammatchmaking/TeamMatchmakingMapTileController.java (2)
168-171: LGTM! Improved null safety.The added null checks prevent potential NPEs when
assignment.getValue()returns null, which is appropriate for a property that may not always have a value.Also applies to: 181-183, 215-220
175-175: LGTM! Consistent use of the factory method.Using
VetoKey.of(value)instead of manual construction improves consistency and reduces code duplication.Also applies to: 187-187, 220-220
src/main/java/com/faforever/client/teammatchmaking/TeamMatchmakingService.java (3)
272-283: LGTM! Improved veto change detection.The refactored
setTokensForMapmethod correctly:
- Removes entries when tokens are set to 0
- Updates entries for non-zero values
- Only calls
sendVetoes()when the value actually changes (new entry added or existing entry modified)The condition on line 280 properly detects actual changes by checking both new entries (
previousValue == null && vetoTokensApplied > 0) and modified entries (previousValue != null && previousValue != vetoTokensApplied).
285-293: LGTM! Safer map replacement strategy.The renamed
updateVetoesmethod now builds a temporary map first, then replaces the entireappliedVetoesmap usingclear()+putAll(). This is safer than incremental updates when receiving the full veto state from the server.
242-247: Verify: Connection state listener may contradict PR intent.The PR title is "Only send vetoes on player action," but the connection state listener at line 245 calls
sendVetoes()on connection, which is not a player action. This is the root cause of the test failure inTeamMatchmakingServiceTest.javaline 644.Consider whether:
- This listener should remain for state synchronization on reconnection (in which case, fix the test as noted in the test file review)
- This listener should be removed to fully align with the PR title (send vetoes only on explicit player changes via
setTokensForMap)To verify the intended behavior, please clarify whether sending vetoes on connection is:
- Intentional for state sync on reconnection
- Should be removed per the PR title
If it's intentional, the test in
TeamMatchmakingServiceTest.javaneeds to be fixed as indicated in the test file review.
| public void testSendVetoesOnVetoesChange() { | ||
| connectionState.set(ConnectionState.CONNECTED); | ||
| when(fafServerAccessor.getConnectionState()).thenReturn(ConnectionState.CONNECTED); | ||
| // Clear invocations from connection state change | ||
| org.mockito.Mockito.clearInvocations(fafServerAccessor); | ||
|
|
||
| matchmakerPrefs.getAppliedVetoes().put(new VetoKey(1, 1), 1); | ||
| instance.setTokensForMap(new VetoKey(1, 1), 1); | ||
|
|
||
| verify(fafServerAccessor, times(1)).setPlayerVetoes(anyList()); | ||
| } |
There was a problem hiding this comment.
Fix the test setup to avoid triggering sendVetoes twice.
The test fails because setPlayerVetoes is called twice:
- Line 639 sets
connectionStatetoCONNECTED, triggering the connection state listener inTeamMatchmakingService(lines 242-247) which callssendVetoes() - Line 642 calls
setTokensForMap, which also callssendVetoes()when the value changes (line 281)
To test only the veto change behavior, clear mock invocations after setting up the connection state.
Apply this diff:
@Test
public void testSendVetoesOnVetoesChange() {
connectionState.set(ConnectionState.CONNECTED);
when(fafServerAccessor.getConnectionState()).thenReturn(ConnectionState.CONNECTED);
+ clearInvocations(fafServerAccessor);
instance.setTokensForMap(new VetoKey(1, 1), 1);
verify(fafServerAccessor, times(1)).setPlayerVetoes(anyList());
}🧰 Tools
🪛 GitHub Actions: Checks
[error] 644-644: Mockito TooManyActualInvocations: fafServerAccessor.setPlayerVetoes(...) called 2 times, but expected to be called 1 time in testSendVetoesOnVetoesChange.
🤖 Prompt for AI Agents
In
src/test/java/com/faforever/client/teammatchmaking/TeamMatchmakingServiceTest.java
around lines 638 to 645, the test setup triggers sendVetoes twice (once when
setting connection state to CONNECTED and again when calling setTokensForMap),
so clear the mock invocation history after initializing the connection state;
specifically, after setting connectionState and stubbing
fafServerAccessor.getConnectionState(), call
Mockito.clearInvocations(fafServerAccessor) (or reset/clearInvocations on the
relevant mock) before calling instance.setTokensForMap so that verify(...,
times(1)).setPlayerVetoes(...) only observes the veto change invocation.
Summary by CodeRabbit
Release Notes
Refactor
Bug Fixes
Tests