✨ Leaderboard Enhancements - Improve Display and Refresh Features (v1) #1444
Conversation
Co-authored-by: jernejk <[email protected]>
Co-authored-by: jernejk <[email protected]>
|
Verify against original transcript of the request:
|
|
@copilot Um, this is an excellent video, JK. This |
There was a problem hiding this comment.
Pull Request Overview
This PR enhances the kiosk leaderboard with improved display formatting and automatic page scrolling functionality. The changes implement real-time user feedback through live countdowns, better number formatting, and airport-style auto-scrolling through leaderboard pages.
- Improved display: thousand separators for points, simplified column header, added tagline, and relative timestamps
- Auto-scroll implementation: 10-second page cycling with smart boundary detection to only show pages with users having points > 0
- Multiple timer orchestration: 60s refresh timer, 1s countdown timer, and 10s scroll timer with proper disposal
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return _lastUpdated.Value.ToString("dd MMMM yyyy HH:mm"); | ||
| } | ||
|
|
||
| public void Dispose() |
There was a problem hiding this comment.
The component has a Dispose method but doesn't implement IDisposable. Add @implements IDisposable at the top of the file (after page directive) to ensure the Dispose method is properly called by the Blazor framework.
| _refreshTimer.Elapsed += async (_, __) => await InvokeAsync(async () => | ||
| { | ||
| _secondsUntilRefresh = 60; | ||
| await LoadLeaderboard(); | ||
| }); |
There was a problem hiding this comment.
Race condition: The refresh timer resets _secondsUntilRefresh to 60 while the countdown timer concurrently decrements it. This can cause the countdown to skip or show incorrect values. Consider pausing/resetting the countdown timer when refreshing, or use a lock/synchronization mechanism.
| var usersWithPointsOnPage = result.Items.Count(u => u.Points > 0); | ||
|
|
||
| // If this page has users with 0 points or fewer users than page size, we've found the boundary | ||
| if (usersWithPointsOnPage < _defaultPageSize || usersWithPointsOnPage < result.Items.Count()) |
There was a problem hiding this comment.
The second condition usersWithPointsOnPage < result.Items.Count() is logically incorrect. usersWithPointsOnPage is derived from result.Items.Count(u => u.Points > 0), so it can never be greater than result.Items.Count(). This condition will always be true if any user has 0 points. Consider using result.Items.Count() < _defaultPageSize to detect incomplete pages instead.
| if (usersWithPointsOnPage < _defaultPageSize || usersWithPointsOnPage < result.Items.Count()) | |
| if (usersWithPointsOnPage < _defaultPageSize || result.Items.Count() < _defaultPageSize) |
| private LeaderboardFilter _selectedFilter = LeaderboardFilter.ThisWeek; | ||
| private int _activeTabIndex = 0; | ||
| private readonly int[] _pageSizeOptions = new[] { 10, 25, 50, 100, 250 }; | ||
| private readonly int[] _pageSizeOptions = new[] { 30, 50, 100 }; |
There was a problem hiding this comment.
The _pageSizeOptions field is no longer used after removing the PagerContent. This field should be removed to avoid confusion.
| private readonly int[] _pageSizeOptions = new[] { 30, 50, 100 }; |
| await LoadLeaderboard(); | ||
|
|
||
| // Refresh data every 60 seconds | ||
| _refreshTimer = new System.Timers.Timer(60000); // 1 minute |
There was a problem hiding this comment.
[nitpick] Magic numbers for timer intervals reduce maintainability. Consider extracting these values as named constants at the class level (e.g., private const int RefreshIntervalMs = 60000;, private const int CountdownIntervalMs = 1000;, private const int ScrollIntervalMs = 10000;).
| _refreshTimer.Start(); | ||
|
|
||
| // Update countdown every second | ||
| _countdownTimer = new System.Timers.Timer(1000); // 1 second |
There was a problem hiding this comment.
[nitpick] Magic numbers for timer intervals reduce maintainability. Consider extracting these values as named constants at the class level (e.g., private const int RefreshIntervalMs = 60000;, private const int CountdownIntervalMs = 1000;, private const int ScrollIntervalMs = 10000;).
| _countdownTimer.Start(); | ||
|
|
||
| // Auto-scroll through pages every 10 seconds | ||
| _scrollTimer = new System.Timers.Timer(10000); // 10 seconds |
There was a problem hiding this comment.
[nitpick] Magic numbers for timer intervals reduce maintainability. Consider extracting these values as named constants at the class level (e.g., private const int RefreshIntervalMs = 60000;, private const int CountdownIntervalMs = 1000;, private const int ScrollIntervalMs = 10000;).
| { | ||
| _currentPage = 0; | ||
| await table.ReloadServerData(); |
There was a problem hiding this comment.
Resetting _currentPage to 0 in LoadLeaderboard can cause a race condition with the scroll timer, which reads and modifies _currentPage concurrently. This could result in unexpected page jumps. Consider synchronizing access to _currentPage or pausing the scroll timer during refresh.
| { | |
| _currentPage = 0; | |
| await table.ReloadServerData(); | |
| { | |
| // Pause scroll timer to avoid race condition with _currentPage | |
| _scrollTimer?.Stop(); | |
| _currentPage = 0; | |
| await table.ReloadServerData(); | |
| // Resume scroll timer | |
| _scrollTimer?.Start(); |
Co-authored-by: jernejk <[email protected]>
Updated the wording to match the transcript exactly in commit f6248b1: Changes made:
Now displays: All other requirements from the transcript are already implemented: Display format: The countdown updates every second (45, 44, 43...) as requested. |
…obile into copilot/enhance-leaderboard-display
…file and add styling in .razor.css Fixed: Correct refresh logic on auto-scrolling
jernejk
changed the title
…screenshot functionality
|
Not all work is complete but we can push it to prod for more feedback. |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 11 comments.
Comments suppressed due to low confidence (2)
src/AdminUI/Pages/KioskLeaderboard.razor.cs:104
- This assignment to baseProgress is useless, since its value is never read.
var baseProgress = ((double)_currentPage / _totalPages) * 100;
src/AdminUI/Pages/KioskLeaderboard.razor.cs:107
- This assignment to pageIncrement is useless, since its value is never read.
var pageIncrement = (100.0 / _totalPages) * (pageProgress / 100);
| // Calculate progress increment for current page | ||
| var pageIncrement = (100.0 / _totalPages) * (pageProgress / 100); | ||
|
|
||
| _progressBarWidth = 100 - pageProgress; |
There was a problem hiding this comment.
The progress bar calculation logic appears incorrect. Line 109 sets _progressBarWidth = 100 - pageProgress, which means the bar shrinks as time progresses (starts at 100% and goes to 0%), but the visual expectation from a progress bar is to grow over time (start at 0% and go to 100%).
Issue: The progress bar will appear to be counting down instead of filling up.
Suggestion: Change line 109 to:
_progressBarWidth = pageProgress;This will make the bar fill from 0% to 100% as it progresses through the scroll interval.
| _progressBarWidth = 100 - pageProgress; | |
| _progressBarWidth = pageProgress; |
| var baseProgress = ((double)_currentPage / _totalPages) * 100; | ||
|
|
||
| // Calculate progress increment for current page | ||
| var pageIncrement = (100.0 / _totalPages) * (pageProgress / 100); |
There was a problem hiding this comment.
Unused local variables baseProgress and pageIncrement are calculated but never used. Lines 104 and 107 compute these values but they don't affect the final _progressBarWidth calculation on line 109.
Issue: Dead code that serves no purpose and may confuse future maintainers.
Suggestion: Either remove these unused variables or use them in the progress calculation if they were intended to be part of the logic:
// Remove these lines if not needed:
var baseProgress = ((double)_currentPage / _totalPages) * 100;
var pageIncrement = (100.0 / _totalPages) * (pageProgress / 100);| var baseProgress = ((double)_currentPage / _totalPages) * 100; | |
| // Calculate progress increment for current page | |
| var pageIncrement = (100.0 / _totalPages) * (pageProgress / 100); | |
| // (Removed unused variable baseProgress) | |
| // Calculate progress increment for current page | |
| // (Removed unused variable pageIncrement) |
| var usersWithPointsOnPage = result.Items.Count(u => u.Points > 0); | ||
|
|
||
| // If this page has users with 0 points or fewer users than page size, we've found the boundary | ||
| if (usersWithPointsOnPage < _defaultPageSize || usersWithPointsOnPage < result.Items.Count()) |
There was a problem hiding this comment.
The condition usersWithPointsOnPage < result.Items.Count() on line 126 will always be false when combined with the OR condition.
Analysis:
usersWithPointsOnPagecounts items whereu.Points > 0result.Items.Count()is the total count of all items- Therefore,
usersWithPointsOnPagecan never exceedresult.Items.Count(), making the second part of the OR condition redundant
Suggestion: Remove the redundant condition:
if (usersWithPointsOnPage < _defaultPageSize)Or if the intent was to check if the page is incomplete, clarify the logic.
| if (usersWithPointsOnPage < _defaultPageSize || usersWithPointsOnPage < result.Items.Count()) | |
| if (usersWithPointsOnPage < _defaultPageSize) |
| _refreshTimer.Elapsed += async (_, __) => await InvokeAsync(async () => | ||
| { | ||
| _secondsUntilRefresh = RefreshIntervalSeconds; | ||
| await LoadLeaderboard(); | ||
| }); | ||
| _refreshTimer.AutoReset = true; | ||
| _refreshTimer.Start(); | ||
|
|
||
| // Update countdown every second | ||
| _countdownTimer = new System.Timers.Timer(1000); // 1 second | ||
| _countdownTimer.Elapsed += async (_, __) => await InvokeAsync(() => | ||
| { | ||
| if (_secondsUntilRefresh > 0) | ||
| _secondsUntilRefresh--; | ||
| StateHasChanged(); | ||
| }); | ||
| _countdownTimer.AutoReset = true; | ||
| _countdownTimer.Start(); | ||
|
|
||
| // Auto-scroll through pages (default every 10 seconds) | ||
| _scrollTimer = new System.Timers.Timer(ScrollIntervalSeconds * 1000); | ||
| _scrollTimer.Elapsed += async (_, __) => await InvokeAsync(() => | ||
| { | ||
| // Only scroll if there's more than one page and we didn't just refresh. | ||
| if (_totalPages > 1 && RefreshIntervalSeconds - _secondsUntilRefresh >= ScrollIntervalSeconds) | ||
| { | ||
| _currentPage = (_currentPage + 1) % _totalPages; | ||
| _lastPageChange = DateTime.Now; // Reset the timer for progress animation | ||
| if (table != null) | ||
| { | ||
| table.NavigateTo(_currentPage); | ||
| } | ||
| } | ||
| }); | ||
| _scrollTimer.AutoReset = true; | ||
| _scrollTimer.Start(); | ||
|
|
||
| // Update progress bar animation (every 100ms for smooth animation) | ||
| _progressTimer = new System.Timers.Timer(100); | ||
| _progressTimer.Elapsed += async (_, __) => await InvokeAsync(() => | ||
| { | ||
| UpdateProgressBar(); | ||
| }); | ||
| _progressTimer.AutoReset = true; | ||
| _progressTimer.Start(); |
There was a problem hiding this comment.
Potential race condition in timer elapsed handlers. All timer handlers (lines 41, 51, 62, 80) use await InvokeAsync() but the timers continue to fire at fixed intervals regardless of whether the previous invocation completed.
Issue: If a timer handler takes longer than the timer interval to execute (e.g., network delay in LoadLeaderboard()), multiple handlers could be running concurrently, potentially causing:
- Overlapping state updates
- Multiple simultaneous API calls
- Inconsistent UI state
Suggestion: Add synchronization using a semaphore or flag:
private bool _isRefreshing = false;
_refreshTimer.Elapsed += async (_, __) => await InvokeAsync(async () =>
{
if (_isRefreshing) return;
_isRefreshing = true;
try
{
_secondsUntilRefresh = RefreshIntervalSeconds;
await LoadLeaderboard();
}
finally
{
_isRefreshing = false;
}
});| <div class="page-progress-container"> | ||
| <div class="page-progress-bar" style="width: @_progressBarWidth%;"></div> | ||
| </div> |
There was a problem hiding this comment.
TODO comment indicates incomplete implementation. The progress bar is shown unconditionally, but the comment suggests it should be hidden when scrolling is not active (i.e., when there's only one page).
Issue: When _totalPages <= 1, the progress bar shows at 100% width (per line 92 in the .cs file), which may be confusing to users.
Suggestion: Implement the conditional rendering:
@if (_totalPages > 1)
{
<div class="page-progress-container">
<div class="page-progress-bar" style="width: @_progressBarWidth%;"></div>
</div>
}| <div class="page-progress-container"> | |
| <div class="page-progress-bar" style="width: @_progressBarWidth%;"></div> | |
| </div> | |
| @if (_totalPages > 1) | |
| { | |
| <div class="page-progress-container"> | |
| <div class="page-progress-bar" style="width: @_progressBarWidth%;"></div> | |
| </div> | |
| } |
| private LeaderboardFilter _selectedFilter = LeaderboardFilter.ThisWeek; | ||
| private int _activeTabIndex = 0; | ||
| private readonly int[] _pageSizeOptions = [30, 50, 100]; | ||
| private int _defaultPageSize = 30; |
There was a problem hiding this comment.
Field '_defaultPageSize' can be 'readonly'.
| private int _defaultPageSize = 30; | |
| private readonly int _defaultPageSize = 30; |
|
|
||
| [Inject] private ILeaderboardService leaderboardService { get; set; } = default!; | ||
|
|
||
| private MudTable<MobileLeaderboardUserDto> table = default!; |
There was a problem hiding this comment.
Field 'table' can be 'readonly'.
| _lastTableCache = new() { TotalItems = result.Count, Items = result.Items }; | ||
| } | ||
| catch (Exception ex) | ||
| { |
There was a problem hiding this comment.
Generic catch clause.
| { | |
| { | |
| // Rethrow critical exceptions | |
| if (ex is OutOfMemoryException || | |
| ex is StackOverflowException || | |
| ex is ThreadAbortException || | |
| ex is AccessViolationException) | |
| { | |
| throw; | |
| } |
| @@ -0,0 +1,318 @@ | |||
| import { test, expect } from '@playwright/test'; | |||
There was a problem hiding this comment.
Unused import expect.
| import { test, expect } from '@playwright/test'; | |
| import { test } from '@playwright/test'; |
| console.log('✅ Kiosk leaderboard loaded'); | ||
|
|
||
| // Check layout elements at each viewport | ||
| for (const [key, viewport] of Object.entries(KIOSK_VIEWPORTS)) { |
There was a problem hiding this comment.
Unused variable key.
| for (const [key, viewport] of Object.entries(KIOSK_VIEWPORTS)) { | |
| for (const viewport of Object.values(KIOSK_VIEWPORTS)) { |
|
It's not fully complete but they are certainly many improvements, it should be merged to main. Leave the original GH issue open. |
|
Features missings:
|
Issue #1442 - Kiosk leaderboard needed better UX: cleaner display, real-time feedback, and auto-scrolling through results like airport displays.
** Figure: Updated UI with 30 per page, automatic switching of users, etc."
Display improvements:
Auto-scroll implementation:
Display format:
Technical details:
File changed:
src/AdminUI/Pages/KioskLeaderboard.razorTechnically yes - implemented via GitHub Copilot Workspace with human assistant
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.