Profile: Only load one month of games by default#3491
Open
dlin2028 wants to merge 4 commits intoFAForever:developfrom
Open
Profile: Only load one month of games by default#3491dlin2028 wants to merge 4 commits intoFAForever:developfrom
dlin2028 wants to merge 4 commits intoFAForever:developfrom
Conversation
…nstead of lagging for veteran players
📝 WalkthroughWalkthroughDefault time period now defaults to LAST_MONTH; time-period selection changes trigger a unified onRatingTypeChange flow that computes a nullable "since" OffsetDateTime and passes it to StatisticsService.getRatingHistory, which supports optional since-based filtering. Changes
Sequence DiagramsequenceDiagram
participant UI as User
participant Controller as PlayerInfoWindowController
participant Service as StatisticsService
participant DB as Database/Cache
UI->>Controller: Change time period selection
Controller->>Controller: Compute since (ALL_TIME → null, else → OffsetDateTime at UTC)
Controller->>Service: getRatingHistory(player, leaderboard, since)
activate Service
alt since is not null
Service->>DB: Query with filter<br/>gamePlayerStats.scoreTime > since.toInstant()
else since is null
Service->>DB: Query all rating history for player/leaderboard
end
DB-->>Service: Return entries
Service->>Controller: Emit Flux<LeaderboardRatingJournal>
deactivate Service
Controller->>Controller: Update ratingData and plot graph
Controller-->>UI: Render updated graph
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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
🧹 Nitpick comments (2)
src/test/java/com/faforever/client/player/PlayerInfoWindowControllerTest.java (1)
100-101: Strengthen assertions for the newsincecontract.Using
any()for the third argument misses the core behavior added in this PR. Assert that defaultLAST_MONTHsends a non-nullsince, and add a case forALL_TIMEexpectingnull.🧪 Suggested assertion tightening
import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.argThat; import static org.mockito.ArgumentMatchers.eq; @@ - verify(statisticsService, times(2)).getRatingHistory(eq(player), eq(leaderboard), any()); + verify(statisticsService, times(2)).getRatingHistory( + eq(player), + eq(leaderboard), + argThat(Objects::nonNull) + );And add a dedicated test:
- set
instance.timePeriodComboBox.setValue(TimePeriod.ALL_TIME)- trigger
instance.onRatingTypeChange()- verify third arg is
null.Also applies to: 186-186
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/com/faforever/client/player/PlayerInfoWindowControllerTest.java` around lines 100 - 101, The test currently stubs statisticsService.getRatingHistory(player, ..., any()) which hides the new "since" contract; change the stub for the default case to assert the third argument is non-null (e.g., use an argument matcher like argThat(Objects::nonNull) or eq with a constructed Instant) so the test verifies LAST_MONTH supplies a non-null since, and add a new test that sets instance.timePeriodComboBox.setValue(TimePeriod.ALL_TIME), calls instance.onRatingTypeChange(), and verifies statisticsService.getRatingHistory(player, ..., null) was invoked (i.e., the third arg is null); target the getRatingHistory(...) call and onRatingTypeChange() in PlayerInfoWindowControllerTest for these changes.src/test/java/com/faforever/client/stats/StatisticsServiceTest.java (1)
53-61: Add coverage for the newsince != nullfilter path.This test validates only the
nullpath. Please add a second test that passes a concreteOffsetDateTimeand verifies thegamePlayerStats.scoreTimepredicate is included.✅ Suggested test extension
`@Test` public void testGetStatisticsForPlayer() throws Exception { @@ } + +@Test +public void testGetStatisticsForPlayerSinceDate() { + PlayerInfo player = PlayerInfoBuilder.create().defaultValues().username("junit").get(); + OffsetDateTime since = OffsetDateTime.parse("2026-02-18T00:00:00Z"); + Flux<ElideEntity> resultFlux = Flux.just(leaderboardMapper.map(Instancio.create(LeaderboardRatingJournal.class))); + when(fafApiAccessor.getAll(any())).thenReturn(resultFlux); + + StepVerifier.create(instance.getRatingHistory(player, leaderboard, since)) + .expectNextCount(1) + .verifyComplete(); + + verify(fafApiAccessor).getAll(argThat( + ElideMatchers.hasFilter( + qBuilder().instant("gamePlayerStats.scoreTime").after(since.toInstant(), false).and() + .intNum("gamePlayerStats.player.id").eq(player.getId()).and() + .intNum("leaderboard.id").eq(leaderboard.id()) + ) + )); +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/com/faforever/client/stats/StatisticsServiceTest.java` around lines 53 - 61, Add a new test in StatisticsServiceTest that calls instance.getRatingHistory(player, leaderboard, since) with a concrete OffsetDateTime (e.g. OffsetDateTime.now().minusDays(1)) and assert the reactive result (StepVerifier) as in the existing test; then verify(fafApiAccessor).getAll(argThat(...)) also contains the additional predicate for gamePlayerStats.scoreTime (use ElideMatchers.hasFilter with a qBuilder() expression that includes a date/time comparison on "gamePlayerStats.scoreTime" such as greaterThanOrEqual(since)), and keep the existing verification for page size (ElideMatchers.hasPageSize(10000)).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/com/faforever/client/player/PlayerInfoWindowController.java`:
- Around line 475-478: Guard against a null value returned by
timePeriodComboBox.getValue(): before calling timePeriod.getDate() check if the
value is null and treat it as a safe fallback (for example TimePeriod.ALL_TIME
or a sensible default), then compute OffsetDateTime since only when the resolved
TimePeriod is not ALL_TIME; update the logic around the timePeriod and since
variables (referencing timePeriodComboBox, TimePeriod, timePeriod, and
getDate()) to perform a null-check or use Optional to avoid dereferencing a null
TimePeriod.
---
Nitpick comments:
In
`@src/test/java/com/faforever/client/player/PlayerInfoWindowControllerTest.java`:
- Around line 100-101: The test currently stubs
statisticsService.getRatingHistory(player, ..., any()) which hides the new
"since" contract; change the stub for the default case to assert the third
argument is non-null (e.g., use an argument matcher like
argThat(Objects::nonNull) or eq with a constructed Instant) so the test verifies
LAST_MONTH supplies a non-null since, and add a new test that sets
instance.timePeriodComboBox.setValue(TimePeriod.ALL_TIME), calls
instance.onRatingTypeChange(), and verifies
statisticsService.getRatingHistory(player, ..., null) was invoked (i.e., the
third arg is null); target the getRatingHistory(...) call and
onRatingTypeChange() in PlayerInfoWindowControllerTest for these changes.
In `@src/test/java/com/faforever/client/stats/StatisticsServiceTest.java`:
- Around line 53-61: Add a new test in StatisticsServiceTest that calls
instance.getRatingHistory(player, leaderboard, since) with a concrete
OffsetDateTime (e.g. OffsetDateTime.now().minusDays(1)) and assert the reactive
result (StepVerifier) as in the existing test; then
verify(fafApiAccessor).getAll(argThat(...)) also contains the additional
predicate for gamePlayerStats.scoreTime (use ElideMatchers.hasFilter with a
qBuilder() expression that includes a date/time comparison on
"gamePlayerStats.scoreTime" such as greaterThanOrEqual(since)), and keep the
existing verification for page size (ElideMatchers.hasPageSize(10000)).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2d511211-e9f0-440f-aaaf-069664604417
📒 Files selected for processing (5)
src/main/java/com/faforever/client/player/PlayerInfoWindowController.javasrc/main/java/com/faforever/client/stats/StatisticsService.javasrc/main/resources/theme/user_info_window.fxmlsrc/test/java/com/faforever/client/player/PlayerInfoWindowControllerTest.javasrc/test/java/com/faforever/client/stats/StatisticsServiceTest.java
src/main/java/com/faforever/client/player/PlayerInfoWindowController.java
Outdated
Show resolved
Hide resolved
dlin2028
changed the title
Co-authored-by: dlin2028 <28829406+dlin2028@users.noreply.github.com>
…-check Guard against null TimePeriod from timePeriodComboBox and add contract tests
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/test/java/com/faforever/client/player/PlayerInfoWindowControllerTest.java (1)
186-192: Consider addingwaitForFxEvents()for consistency with new tests.The new tests at lines 200 and 210 call
waitForFxEvents()before verification, but this existing test does not. While the synchronous stub likely prevents flakiness, adding the call would improve consistency across the test methods.♻️ Suggested consistency improvement
`@Test` public void testOnRatingTypeChange() { testSetPlayerInfoBean(); instance.ratingTypeComboBox.setValue(leaderboard); instance.onRatingTypeChange(); + waitForFxEvents(); verify(statisticsService, times(2)).getRatingHistory(eq(player), eq(leaderboard), any()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/com/faforever/client/player/PlayerInfoWindowControllerTest.java` around lines 186 - 192, Add a call to waitForFxEvents() before the verification in testOnRatingTypeChange to match the other tests' pattern: after calling instance.ratingTypeComboBox.setValue(leaderboard) and instance.onRatingTypeChange(), invoke waitForFxEvents() and then run the verify(statisticsService, times(2)).getRatingHistory(eq(player), eq(leaderboard), any()) assertion to ensure JavaFX events are flushed consistently across tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@src/test/java/com/faforever/client/player/PlayerInfoWindowControllerTest.java`:
- Around line 186-192: Add a call to waitForFxEvents() before the verification
in testOnRatingTypeChange to match the other tests' pattern: after calling
instance.ratingTypeComboBox.setValue(leaderboard) and
instance.onRatingTypeChange(), invoke waitForFxEvents() and then run the
verify(statisticsService, times(2)).getRatingHistory(eq(player),
eq(leaderboard), any()) assertion to ensure JavaFX events are flushed
consistently across tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cd536040-f5d4-4035-87fa-76f8c77fdb1d
📒 Files selected for processing (3)
src/main/java/com/faforever/client/player/PlayerInfoWindowController.javasrc/test/java/com/faforever/client/player/PlayerInfoWindowControllerTest.javasrc/test/java/com/faforever/client/stats/StatisticsServiceTest.java
🚧 Files skipped from review as they are similar to previous changes (2)
- src/test/java/com/faforever/client/stats/StatisticsServiceTest.java
- src/main/java/com/faforever/client/player/PlayerInfoWindowController.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Currently profile page gets all games by default, this causes a lot of lag for players with a lot of games. This changes the default to only fetch games from the last month, significantly reducing loading time.
Before:
After:
Summary by CodeRabbit
New Features
Bug Fixes