Implement ghost system with leader boards, downloads, local saves + tests - #252
Closed
TheRealJoelmatic wants to merge 4 commits into
Closed
Implement ghost system with leader boards, downloads, local saves + tests#252TheRealJoelmatic wants to merge 4 commits into
TheRealJoelmatic wants to merge 4 commits into
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis pull request introduces comprehensive ghost leaderboard and local ghost replay functionality to WheelWizard. It adds services for fetching online leaderboards, discovering and parsing local Dolphin ghost files, managing track variants and hex mappings, UI pages for browsing and filtering ghosts, and supporting converters, models, and components. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant UI as GhostTimesPage (Online Tab)
participant Service as GhostLeaderboardService
participant API as rwfc.net API
participant Response as GhostLeaderboardResponse
User->>UI: Click Refresh / Change Filter
activate UI
UI->>UI: Validate Track ID
UI->>UI: Read CC, Glitch, Type from UI
UI->>Service: GetLeaderboardAsync(trackId, cc, ...)
deactivate UI
activate Service
Service->>Service: Build query URL with parameters
Service->>API: GET /api/timetrial/leaderboard?...
activate API
API-->>Service: HTTP 200 + JSON
deactivate API
Service->>Response: Deserialize JSON to GhostLeaderboardResponse
activate Response
Response-->>Service: Populated object
deactivate Response
Service-->>UI: GhostLeaderboardResponse
deactivate Service
activate UI
UI->>UI: Populate Submissions collection
UI->>UI: Update IsLoading flag
deactivate UI
sequenceDiagram
participant User as User
participant UI as GhostTimesPage (Local Tab)
participant LocalService as LocalGhostService
participant FileSystem as File System
participant Parser as RkgParser
participant LocalGhostData as LocalGhostData
User->>UI: Switch to Local Tab / Change CC
activate UI
UI->>LocalService: GetTrackGhostsAsync(trackId, hexValue)
deactivate UI
activate LocalService
LocalService->>FileSystem: Scan /ghosts/{hexValue}/{cc}/ folders
activate FileSystem
FileSystem-->>LocalService: List of .rkg files
deactivate FileSystem
loop For each .rkg file
LocalService->>Parser: ParseRkgFile(filePath)
activate Parser
Parser->>FileSystem: Read binary file
Parser->>Parser: Validate header & extract metadata
Parser->>Parser: Decode lap splits, date, Mii name
Parser-->>LocalService: LocalGhostData
deactivate Parser
end
LocalService->>LocalGhostData: Build LocalTrackGhosts with lists
LocalService-->>UI: LocalTrackGhosts
deactivate LocalService
activate UI
UI->>UI: Sort ghosts by total time
UI->>UI: Assign rank numbers
UI->>UI: Filter by variant status
UI->>UI: Populate LocalGhosts collection
deactivate UI
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 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)
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: 44
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@WheelWizard.Test/Features/Ghosts/GhostTrackServiceTests.cs`:
- Around line 76-109: The StubHttpMessageHandler currently uses a
non-thread-safe Dictionary _requestCounts updated in SendAsync and read in
GetRequestCount; replace _requestCounts with a
System.Collections.Concurrent.ConcurrentDictionary<string,int> and update
SendAsync to increment counts atomically (e.g., using AddOrUpdate or
TryGetValue/CompareExchange pattern) and adjust GetRequestCount to read from the
ConcurrentDictionary; update the constructor/type declaration for _requestCounts
and ensure SendAsync, GetRequestCount, and any initializations reference the new
ConcurrentDictionary to make the handler thread-safe.
In `@WheelWizard/Converters/GhostDisplayConverters.cs`:
- Around line 55-57: In GhostDisplayConverters (the code path that looks up
CountryFlags), normalize the incoming countryCode using culture-invariant
casing: replace the culture-sensitive call to countryCode.ToUpper() with
countryCode.ToUpperInvariant() before looking up in the CountryFlags dictionary
so ISO 3166-1 alpha-2 lookups are reliable across locales.
In `@WheelWizard/Models/GhostTrack.cs`:
- Around line 23-35: The two enums GhostLocation and GhostLocationFilter are
identical; remove GhostLocationFilter and consolidate all uses to GhostLocation
(update method signatures, property types, switch/case statements,
serialization/deserialization code, and any tests referencing
GhostLocationFilter). Replace parameter types, return types, casts, and
attributes that reference GhostLocationFilter with GhostLocation, and run/build
to fix any compiler errors introduced by the rename; keep the enum definition
named GhostLocation and delete the GhostLocationFilter declaration.
- Around line 11-13: DisplayName currently prefixes non-custom tracks with
Console (which can be the literal "Standard"); change it so standard tracks
don't show the "Standard" console prefix by returning just Name when TrackType
== GhostTrackType.Retro or when Console == "Standard". Update the DisplayName
getter (refer to the DisplayName property, IsCustomTrack and Console) to
conditionally omit Console for standard/retro tracks and only include Console
for non-standard custom consoles.
In `@WheelWizard/Models/LocalGhostData.cs`:
- Around line 32-33: AverageLapDisplay currently truncates fractional
milliseconds by casting AverageLapMs to uint; change it to round the average
before converting to an unsigned integer. Update the AverageLapDisplay property
to call Math.Round (or an equivalent rounding method) on AverageLapMs and then
convert to uint when passing into FormatTime, referencing the AverageLapMs and
AverageLapDisplay members and FormatTime to locate the change.
- Around line 77-83: The properties BestTime150 and BestTime200 currently call
OrderBy(...).FirstOrDefault(), which sorts the entire concatenated sequence;
replace that with a linear min scan (e.g., use LINQ's MinBy(g => g.TotalTimeMs)
if available or implement a simple loop/aggregate) over
Ghosts150.Concat(VariantGhosts150) and Ghosts200.Concat(VariantGhosts200) to
return the element with smallest TotalTimeMs (return null if the concatenated
sequence is empty); update both BestTime150 and BestTime200 to use the linear
min approach, preserving the return type LocalGhostData?.
In `@WheelWizard/Models/WorldRecordsResponse.cs`:
- Around line 64-69: The switch expression silently maps unknown
trackInfo.Category values to GhostTrackType.All; change this so unknown
categories are logged: replace the switch expression assigning track.TrackType
with logic that detects the default case (either convert to a switch statement
or a small helper MapCategoryToGhostTrackType method) and emit a log entry via
your existing logger (e.g., ILogger) including the offending trackInfo.Category
and a track identifier (e.g., trackInfo.Id or track.Name) before returning
GhostTrackType.All; keep the known branches ("retro" => Retro, "custom" =>
Custom) unchanged.
In `@WheelWizard/Services/GhostLeaderboardService.cs`:
- Around line 42-46: Replace the string-interpolated log in the catch block (the
Log.Error call that currently uses $"Failed to load track info from API:
{ex.Message}") with a structured log template so Serilog can index fields; keep
the exception object but change the message to use a named property (e.g.,
"Failed to load track info from API: {ErrorMessage}") and pass ex.Message as the
template argument. Update the catch in GhostLeaderboardService (where Exception
ex is caught) to call Log.Error(ex, "Failed to load track info from API:
{ErrorMessage}", ex.Message) or simply Log.Error(ex, "Failed to load track info
from API") if you prefer the exception only.
- Around line 7-16: The GhostLeaderboardService currently instantiates
HttpClient in its constructor and defines a Dispose method without implementing
IDisposable; change the class to accept an HttpClient via constructor injection
(or an IHttpClientFactory if you prefer) instead of new-ing _httpClient in
GhostLeaderboardService(), remove internal ownership if injected, implement
IDisposable on GhostLeaderboardService only if the service actually
owns/disposes the client (i.e., dispose _httpClient in Dispose() when you
created it locally), and ensure the User-Agent header is configured by the
caller or a named/typed client rather than unconditionally adding it inside the
class.
In `@WheelWizard/Services/GhostSaveHelper.cs`:
- Line 3: Remove the unused Serilog namespace import from the top of the file:
delete the "using Serilog;" line in GhostSaveHelper.cs so the file no longer
contains an unused using; leave other using statements and the GhostSaveHelper
class/methods unchanged.
In `@WheelWizard/Services/GhostTrackService.cs`:
- Around line 17-21: The boolean flags (_tracksLoaded, _trackInfoLoaded,
_trackMappingsInitialized) are not thread-safe and can cause duplicate API calls
or race conditions; add a single SemaphoreSlim (e.g., private readonly
SemaphoreSlim _loadLock = new(1,1)) and wrap cache-initializing methods such as
GetAllTracksAsync and any track-info/mapping initialization logic with a single
async lock (await _loadLock.WaitAsync()/finally _loadLock.Release()) so only one
caller populates _allTracks and _trackInfoCache, then set the flags inside the
locked section; alternatively replace the flag+fetch pattern with an
AsyncLazy-style Task<T> field per resource to memoize the in-flight load (e.g.,
a Task<List<ApiTrack>> _allTracksLoadTask) and await that in GetAllTracksAsync
to ensure a single concurrent request.
- Around line 33-39: GetFilteredTracks currently reads _allTracks synchronously
and can return an empty list if GetAllTracksAsync() hasn't finished; change the
API so callers reliably get loaded data: make GetFilteredTracks async (rename to
GetFilteredTracksAsync) and add an EnsureTracksLoadedAsync helper that checks
_tracksLoaded and awaits GetAllTracksAsync() when false, then apply the filter
against _allTracks; update all callers to await the new method. If you cannot
change callers, alternatively add an explicit guard at the top of
GetFilteredTracks that logs/warns when _tracksLoaded is false and either throws
an InvalidOperationException or calls
GetAllTracksAsync().Wait()/GetAwaiter().GetResult() to synchronously block until
data is loaded—choose one approach and apply the same pattern to other similar
methods referencing _allTracks.
- Around line 49-51: The code awaits worldRecordsTask twice: first via
Task.WhenAll(trackInfoTask, worldRecordsTask) in GhostTrackService, then again
with "var response = await worldRecordsTask"; remove the redundant await and
read the completed result directly (e.g., use worldRecordsTask.Result or capture
the result from Task.WhenAll) so you only await once; adjust the assignment to
"var response = worldRecordsTask.Result" (or equivalent) to avoid the
unnecessary second await while keeping trackInfoTask/worldRecordsTask names
intact.
In `@WheelWizard/Services/LocalGhostService.cs`:
- Around line 26-39: The cache check in GetAllLocalGhostsAsync uses both
_hasScannedFolder and _cachedTrackGhosts.Count > 0 which causes re-scans when
the folder was scanned but empty; change the early-return condition to rely only
on _hasScannedFolder (i.e. if (_hasScannedFolder) { Log.Information(...); return
_cachedTrackGhosts; }) so once ScanGhostsFolder() has run it won't be run again,
and keep the existing Log.Information and ScanGhostsFolder() calls and flags
(_hasScannedFolder, _cachedTrackGhosts, ScanGhostsFolder,
GetAllLocalGhostsAsync) intact.
- Around line 10-21: The _cachedTrackGhosts dictionary in LocalGhostService is
not thread-safe and is accessed from async methods (GetAllLocalGhostsAsync,
GetTrackGhostsAsync) which can cause race conditions; replace the
Dictionary<uint, LocalTrackGhosts> _cachedTrackGhosts with a
ConcurrentDictionary<uint, LocalTrackGhosts> and update all usages (reads,
writes, TryGetValue, indexers, Add/Remove patterns) to use ConcurrentDictionary
APIs; additionally, ensure ScanGhostsFolder is protected against concurrent
scans (e.g., with a lock or an async SemaphoreSlim) so updates to the concurrent
dictionary occur in a controlled manner.
- Around line 119-131: The code creates an extra nested "1" folder when
!cc.HasValue for variant tracks because targetFolderPath is already
Path.Combine(hexFolder, "1"); remove the redundant creation of
Path.Combine(targetFolderPath, "1") and only create the variant subfolders under
the existing targetFolderPath; specifically update the block guarded by
!_cc.HasValue_ and _variantMappingService.IsVariantTrack(trackName)_ so it does
not call Directory.CreateDirectory(Path.Combine(targetFolderPath, "1")) but
instead uses the existing targetFolderPath to create "150" and "200" (and the
optional "1" should be removed).
In `@WheelWizard/Services/RkgParser.cs`:
- Around line 274-324: Replace the manual UTF-16 BE decoding in ParseMiiData
with the existing BigEndianBinaryHelper.GetUtf16String to avoid duplication:
call BigEndianBinaryHelper.GetUtf16String on the extracted nameBytes (instead of
Encoding.BigEndianUnicode.GetString) and keep the existing fallback to
Encoding.Unicode (UTF-16 LE) when the BE result is empty/invalid; preserve the
trimming, control-character filtering logic and the Log.Debug messages
(references: ParseMiiData and BigEndianBinaryHelper.GetUtf16String) so
behavior/logging remains the same while reusing the helper for BE decoding.
- Around line 55-65: The code re-extracts bitfields with ExtractBits (variables
timeMinutes, timeSeconds, timeMs, vehicleId, characterId) solely for logging
even though parsed values are already stored on ghostData; remove those
redundant ExtractBits calls and update the Log.Debug calls to reference the
already-parsed properties on ghostData (e.g., use ghostData.TotalTimeMs /
ghostData.TrackId / the ghostData fields that hold minutes/seconds/milliseconds
and vehicle/character ids) instead of recomputing via ExtractBits; ensure you
only keep the Log.Debug lines but change their format parameters to use
ghostData.<property> and delete the unused local variables and extra ExtractBits
invocations.
- Around line 203-209: The current validation in RkgParser (using year, month,
day) allows impossible dates because it only checks day <= 31; replace the day
check with a proper month-specific bounds check using
DateTime.DaysInMonth((int)year, (int)month) (i.e. verify day >= 1 && day <=
DateTime.DaysInMonth(...)) and then construct new DateTime((int)year,
(int)month, (int)day); also avoid swallowing exceptions silently—if you retain
catch logic elsewhere, ensure it logs or surfaces parsing failures rather than
silently returning a fallback.
In `@WheelWizard/Services/TrackHexMappingService.cs`:
- Around line 234-240: The current loop in TrackHexMappingService that builds
the index (using Directory.EnumerateFiles and the local dictionary 'index')
silently ignores duplicate SZS filenames due to the if
(!index.ContainsKey(name)) check; change this so duplicates are logged: when
index already contains 'name', call the service logger (e.g., _logger or the
class's ILogger) to emit a warning that the duplicate filename was skipped and
include both the existing mapped path (index[name]) and the new 'file' path,
keeping the existing behavior of not overwriting the first entry but making the
skip observable.
- Around line 112-117: ComputeGhostFolderHash currently reads the whole file
into memory via File.ReadAllBytes and should be changed to stream the file; add
an overload CrcHelper.ComputeCrc32(Stream stream) that computes CRC32 by reading
the stream in chunks, then modify ComputeGhostFolderHash to open a FileStream
for szsPath (using a using or try/finally) and pass that stream to
CrcHelper.ComputeCrc32(stream) and return the hex string; reference
ComputeGhostFolderHash and CrcHelper.ComputeCrc32 to locate the changes.
In `@WheelWizard/Services/TrackVariantMappingService.cs`:
- Around line 97-107: The method GetGhostFolderPath in
TrackVariantMappingService builds and returns a path even when the directory
doesn't exist; rename it to BuildGhostFolderPath (or Add an ensureExists bool)
to clarify behavior, update all call sites (e.g., LocalGhostService) to use the
new name or to create the directory when needed, and update the XML/doc comment
on the method to state it constructs a path that may not exist; reference the
existing method GetGhostFolderPath, IsVariantTrack, and GetHexValueForTrack when
making changes.
In `@WheelWizard/Views/Components/LapTimesGraph.axaml`:
- Around line 9-14: The control's bindings (e.g., HasData and the other control
properties used via plain {Binding ...}) are currently resolving against the
parent DataContext; either set the runtime DataContext to the control instance
in the LapTimesGraph constructor (DataContext = this) or add an ElementName
self-binding: give the root UserControl an x:Name (e.g., x:Name="self") and
change bindings like {Binding HasData} to {Binding HasData, ElementName=self}
for all control-property bindings in LapTimesGraph.axaml so they target the
control's own properties (e.g., HasData and the other named properties
referenced).
In `@WheelWizard/Views/Components/LapTimesGraph.axaml.cs`:
- Around line 152-153: The fastest-index selection is computed later against
_lapTimesMs, causing a race with the posted UpdateDataPoints call; capture the
fastest index from the same snapshot used to build points before calling
Dispatcher.UIThread.Post and pass that captured value into UpdateDataPoints (or
store it on a temporary/local snapshot variable) so UpdateDataPoints uses the
precomputed fastestIndex instead of reading _lapTimesMs at dispatch time; update
the call site where Dispatcher.UIThread.Post(() => UpdateDataPoints(points),
...) and the UpdateDataPoints signature/usage accordingly (referencing
UpdateDataPoints, _lapTimesMs, and the dispatcher post).
- Around line 26-29: The LapTimesProperty default uses a shared mutable
instance; change the StyledProperty registration for LapTimesProperty on
LapTimesGraph to use null as the default instead of new List<int>() so each
control doesn't share state; update the AvaloniaProperty.Register call for
LapTimesProperty (property name LapTimes) to pass null as the default
value—UpdateGraph already handles null so no other logic changes are required.
- Around line 15-16: The code hardcodes GraphWidth/GraphHeight for mapping data
points which becomes desynced from the rendered Paths when Stretch="Fill";
update the mapping and marker placement to use the actual rendered bounds
instead of the constants: query the actual pixel size of the plot/Path (e.g.,
use areaPath.RenderSize or path.Data.Bounds transformed to the control’s
RenderSize / ActualWidth/ActualHeight) and compute scale/offset from those
values, then use that scale when converting data X/Y to Canvas.SetLeft/SetTop
for the marker circles (replace usages of GraphWidth/GraphHeight in the
point-mapping function and where markers are positioned). Ensure this logic runs
after layout (Loaded/SizeChanged) so markers are recalculated when the control
resizes.
In `@WheelWizard/Views/Layout.axaml`:
- Around line 142-144: Replace the hardcoded Text="Ghosts" on the
SidebarRadioButton with the localized resource used by other sidebar entries:
change the Text attribute to reference the lang resource (e.g.
Text="{StaticResource lang:Ghosts}" or the project's existing lang resource
syntax) on the SidebarRadioButton for the Ghosts page (the element with
PageType="{x:Type pages:GhostsPage}"); if the "Ghosts" key is missing from the
lang resource file, add a "Ghosts" entry to the language resources so the
control resolves correctly.
In `@WheelWizard/Views/Pages/GhostsPage.axaml`:
- Around line 56-62: Remove the redundant Unchecked handlers on the RadioButton
declarations (AllTracksRadio, RetroTracksRadio, CustomTracksRadio) so
FilterChanged is only wired to Checked; within the GhostsPage.axaml RadioButtons
remove the Unchecked="FilterChanged" attributes and leave only
Checked="FilterChanged" (so ApplySearchFilter/FilterChanged only runs once per
user selection when a RadioButton becomes checked).
- Around line 83-98: The transparent overlay Button that wires to
ViewTimesButton_Click is missing accessible metadata for screen readers; add an
AutomationProperties.Name (or AutomationProperties.HelpText) and/or a ToolTip
bound to a descriptive property (or a static string) on that Button so assistive
tech can identify its purpose, keeping the Click handler and Tag binding intact;
ensure the new properties are set on the existing Button element (the one with
Click="ViewTimesButton_Click") so the visual layout stays the same while
improving accessibility.
In `@WheelWizard/Views/Pages/GhostsPage.axaml.cs`:
- Around line 157-162: The class is hiding the inherited PropertyChanged event
with "new", which can break Avalonia bindings; remove the "new event
PropertyChanged" declaration and rely on the base implementation (or implement
INotifyPropertyChanged properly) — delete the PropertyChanged field in this file
and change OnPropertyChanged to call the base notification (e.g., call
base.OnPropertyChanged(propertyName) if UserControlBase exposes it) or, if the
base does not provide an invoker, implement INotifyPropertyChanged on this class
and raise the single canonical PropertyChanged event (use the PropertyChanged
symbol only once, not a hidden duplicate).
- Around line 46-53: Constructor GhostsPage starts LoadTracksAsync
fire-and-forget and discards exceptions; change the call in the constructor so
the returned Task is observed and any unhandled exceptions are logged (e.g.,
call LoadTracksAsync().ContinueWith(...) or store the Task and await it from an
async initializer), referencing the GhostsPage constructor and the
LoadTracksAsync method and ensure the continuation logs exceptions to your
logger (or invokes Dispatcher/UI-safe error handling) instead of using `_ =
LoadTracksAsync()`.
- Around line 99-105: The null check on the field _tracks inside
ApplySearchFilter is dead because _tracks is initialized to a new
ObservableCollection<ApiTrack>() and never null or read; remove the _tracks ==
null branch and update ApplySearchFilter to directly obtain the filtered results
from _ghostTrackService.GetFilteredTracks() (or simply set FilteredTracks to an
empty ObservableCollection when that service returns null/empty). Also consider
removing the unused _tracks field entirely if nothing else reads it; reference
_tracks, ApplySearchFilter, _ghostTrackService, GetFilteredTracks, and
FilteredTracks when making the changes.
In `@WheelWizard/Views/Pages/GhostTimesPage.axaml`:
- Around line 255-264: The two Button elements that use emoji in Content (the
ones with Click handlers ShowGhostFolder_Click and RefreshLocalGhosts_Click and
Classes="SecondaryButton") may render inconsistently; replace the emoji Content
with a PathIcon (like other buttons in this view) using appropriate SVG/PathData
for a folder and refresh icon, keep the same Height/Padding/Classes and Click
handler bindings, and ensure the PathIcon is placed as the Button's Content so
styling/size matches existing icons in the file.
In `@WheelWizard/Views/Pages/GhostTimesPage.axaml.cs`:
- Around line 349-406: LoadLocalGhosts can complete out-of-order and overwrite
newer UI state; add a cancellation/sequence guard by introducing a per-call
CancellationTokenSource (e.g. _loadLocalGhostsCts) or an incrementing request id
(e.g. _loadLocalGhostsRequestId) and cancel/advance the previous one at the
start of LoadLocalGhosts, pass the token or capture the current request id into
awaited calls (EnsureTrackMappingsInitializedAsync, GetTrackGhostsAsync) and
bail out before mutating state if cancelled or if the request id has changed;
only set _currentLocalTrackGhosts, LocalGhosts, IsLoadingLocalGhosts and raise
PropertyChanged when the operation is the active (non-cancelled) request.
- Around line 365-371: The early return when hex resolution fails leaves stale
ghost data; update the failure branch in the method that calls
_trackVariantMappingService.GetHexValueForTrack (use referenced
_selectedTrack.Name and _trackHexMappingService) to clear/reset the local ghost
state by assigning an empty collection to _currentLocalTrackGhosts (or null) and
ensure any exposed summary/UI binding (e.g.,
LocalGhostsSummary/IsLoadingLocalGhosts) is updated/raised before returning so
the UI no longer shows counts from a previously selected track.
- Around line 117-131: SetTrack currently fires
_ghostTrackService.EnsureTrackMappingsInitializedAsync() without awaiting it,
leaving failures unobserved and allowing dependent work (e.g.,
LoadOnlineLeaderboardSafe) to run before mappings exist; change SetTrack to
await EnsureTrackMappingsInitializedAsync (or make an async SetTrackAsync) and
handle exceptions (try/catch and log or surface) before proceeding to set
TrackTitle and calling LoadOnlineLeaderboardSafe when IsOnlineTabSelected, so
mapping initialization completes deterministically.
- Around line 472-477: LocalCcChanged does redundant work by rebinding then
forcing a full disk refresh; change it so toggling CC only rebinds in-memory
unless the CC actually changed the underlying local cache. In LocalCcChanged,
call LoadGhostsForSelectedCc() only for simple toggles and remove the
unconditional _localGhostService.RefreshGhostData() and LoadLocalGhosts() calls;
alternatively, add a guard that compares the new CC selection to the previous
selection (store previous in a field like _currentCc) and only invoke
_localGhostService.RefreshGhostData() and LoadLocalGhosts() when the selection
truly changed or when the cache is stale.
In `@WheelWizard/Views/Patterns/TrackListItem.axaml`:
- Around line 47-75: The RETRO and CT badges (PART_RetroBadge and
PART_CustomBadge) are always visible; change them to show only for the
appropriate track type by binding their Visibility (or using DataTriggers) to
the viewmodel properties that indicate type (e.g., IsRetro, IsCustom or
TrackType) and set Visibility to Collapsed when the property is false or doesn’t
match; update the TrackListItem template to use either boolean-to-visibility
converters or a DataTrigger on TrackType to toggle PART_RetroBadge and
PART_CustomBadge so only the matching badge is rendered.
- Around line 102-137: The preview uses ConsoleColor="Yellow" which mismatches
the case-sensitive style selectors in TrackListItem.axaml (selectors like
patterns|TrackListItem[ConsoleColor=yellow] targeting
TextBlock#PART_ConsoleLabel); update the preview markup so the ConsoleColor
attribute value is "yellow" (lowercase) to match the selectors and restore
design-time styling.
In `@WheelWizard/Views/Patterns/TrackListItem.axaml.cs`:
- Around line 90-98: The OnPropertyChanged override currently checks
IsCustomTrackProperty, IsRetroTrackProperty and ConsoleProperty to call
UpdateVisibility, but UpdateVisibility always hides _consoleLabel (sets
_consoleLabel.IsVisible = false) and ignores the Console value; either remove
ConsoleProperty from the conditional in OnPropertyChanged so changes to Console
don't call UpdateVisibility, or update UpdateVisibility to respect the Console
property (read the Console getter/ConsoleProperty and set
_consoleLabel.IsVisible accordingly) so Console changes correctly affect
visibility; modify either OnPropertyChanged (remove ConsoleProperty) or
UpdateVisibility (implement Console-based visibility logic) and keep other
checks for IsCustomTrackProperty/IsRetroTrackProperty unchanged.
- Around line 74-81: The Click handler for the PART_ViewTimesButton is attached
in OnApplyTemplate via viewTimesButton.Click but never removed, risking a memory
leak; modify TrackListItem to store the handler delegate (e.g., a private
RoutedEventHandler field) when subscribing in OnApplyTemplate (or remove any
existing subscription first) and unsubscribe it when the template is torn down
or the control is disposed—either implement IDisposable.Dispose to detach the
handler or add a cleanup override that finds PART_ViewTimesButton and does
viewTimesButton.Click -= storedHandler; ensure references to ViewTimesClick,
viewTimesButton, and the stored handler are used to safely unsubscribe.
In `@WheelWizard/Views/Popups/GhostDetailsWindow.axaml`:
- Around line 13-149: The XAML uses hardcoded UI strings (Window Title,
TextBlocks and Button Content) which must be replaced with localization resource
bindings: change Title="Ghost Run Details" to bind to a resource key (e.g.,
Title="{StaticResource Loc_GhostRunDetails}"), replace the Text of the TextBlock
with "Lap Times" and "Setup Details" and each field label ("Character:",
"Vehicle:", "Drift:", "Controller:", "Date Set:") and the Close Button Content
("Close") to use localized resource keys (e.g., Text="{StaticResource
Loc_LapTimes}", Content="{StaticResource Loc_Close}"); add corresponding entries
to your localization resource dictionary and ensure the converters and existing
bindings (e.g., CountryCodeToFlag, CharacterIdToName) remain unchanged while
swapping literal strings for resource references.
In `@WheelWizard/Views/Popups/GhostDetailsWindow.axaml.cs`:
- Around line 21-117: PopulateLapTimes repeats grid/label/time creation logic
for each lap, average, and fastest rows; extract that into a helper (e.g.,
CreateLapRow) to remove duplication: add a private static Grid
CreateLapRow(string label, string time, bool isBold = false, int bottomMargin =
4) that builds and returns the Grid configured with two columns, BodyText label
and right-aligned monospace time (apply SemiBold when isBold), then replace the
repeated constructions in PopulateLapTimes (the per-lap loop, avgGrid creation
using FormatTime((int)averageMs), and fastestGrid using
submission.FastestLapDisplay) with calls to CreateLapRow and add the returned
Grid to LapTimesContainer.
- Line 74: The Average() call on submission.LapSplitsMs can throw if the
sequence is empty; update the computation in GhostDetailsWindow.axaml.cs (the
averageMs calculation) to handle empty sequences by either checking
sequence.Any() before calling Average() and using a safe fallback (e.g., 0 or
null) or by using Take(expectedLaps).DefaultIfEmpty(0).Average() so Average
never runs on an empty sequence; ensure downstream code handles the chosen
fallback consistently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f0e08dc9-ece5-4ffc-99d0-b88cc8b4e95c
📒 Files selected for processing (28)
WheelWizard.Test/Features/Ghosts/GhostTrackServiceTests.csWheelWizard.Test/Features/Ghosts/TrackVariantMappingServiceTests.csWheelWizard/Converters/GhostDisplayConverters.csWheelWizard/Models/GhostLeaderboard.csWheelWizard/Models/GhostTrack.csWheelWizard/Models/LocalGhostData.csWheelWizard/Models/WorldRecordsResponse.csWheelWizard/Services/GhostLeaderboardService.csWheelWizard/Services/GhostSaveHelper.csWheelWizard/Services/GhostTrackService.csWheelWizard/Services/LocalGhostService.csWheelWizard/Services/PathManager.csWheelWizard/Services/RkgParser.csWheelWizard/Services/TrackHexMappingService.csWheelWizard/Services/TrackVariantMappingService.csWheelWizard/SetupExtensions.csWheelWizard/Views/App.axamlWheelWizard/Views/Components/LapTimesGraph.axamlWheelWizard/Views/Components/LapTimesGraph.axaml.csWheelWizard/Views/Layout.axamlWheelWizard/Views/Pages/GhostTimesPage.axamlWheelWizard/Views/Pages/GhostTimesPage.axaml.csWheelWizard/Views/Pages/GhostsPage.axamlWheelWizard/Views/Pages/GhostsPage.axaml.csWheelWizard/Views/Patterns/TrackListItem.axamlWheelWizard/Views/Patterns/TrackListItem.axaml.csWheelWizard/Views/Popups/GhostDetailsWindow.axamlWheelWizard/Views/Popups/GhostDetailsWindow.axaml.cs
Comment on lines
+76
to
+109
| private sealed class StubHttpMessageHandler : HttpMessageHandler | ||
| { | ||
| private readonly Dictionary<string, string> _responses; | ||
| private readonly Dictionary<string, int> _requestCounts = new(StringComparer.OrdinalIgnoreCase); | ||
|
|
||
| public StubHttpMessageHandler(Dictionary<string, string> responses) | ||
| { | ||
| _responses = responses; | ||
| } | ||
|
|
||
| public int GetRequestCount(string url) | ||
| { | ||
| return _requestCounts.TryGetValue(url, out var count) ? count : 0; | ||
| } | ||
|
|
||
| protected override Task<HttpResponseMessage> SendAsync(HttpRequestMessage request, CancellationToken cancellationToken) | ||
| { | ||
| var url = request.RequestUri!.ToString(); | ||
| _requestCounts[url] = GetRequestCount(url) + 1; | ||
|
|
||
| if (!_responses.TryGetValue(url, out var content)) | ||
| { | ||
| return Task.FromResult(new HttpResponseMessage(HttpStatusCode.NotFound) | ||
| { | ||
| Content = new StringContent(string.Empty) | ||
| }); | ||
| } | ||
|
|
||
| return Task.FromResult(new HttpResponseMessage(HttpStatusCode.OK) | ||
| { | ||
| Content = new StringContent(content, Encoding.UTF8, "application/json") | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using ConcurrentDictionary for thread-safety in the stub handler.
While unlikely to cause issues in these sequential tests, _requestCounts could have race conditions if tests were parallelized or if SendAsync were called concurrently. Using ConcurrentDictionary with AddOrUpdate would be more robust.
♻️ Proposed thread-safe improvement
+using System.Collections.Concurrent;
...
private sealed class StubHttpMessageHandler : HttpMessageHandler
{
private readonly Dictionary<string, string> _responses;
- private readonly Dictionary<string, int> _requestCounts = new(StringComparer.OrdinalIgnoreCase);
+ private readonly ConcurrentDictionary<string, int> _requestCounts = new(StringComparer.OrdinalIgnoreCase);
...
protected override Task<HttpResponseMessage> SendAsync(HttpRequestMessage request, CancellationToken cancellationToken)
{
var url = request.RequestUri!.ToString();
- _requestCounts[url] = GetRequestCount(url) + 1;
+ _requestCounts.AddOrUpdate(url, 1, (_, count) => count + 1);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@WheelWizard.Test/Features/Ghosts/GhostTrackServiceTests.cs` around lines 76 -
109, The StubHttpMessageHandler currently uses a non-thread-safe Dictionary
_requestCounts updated in SendAsync and read in GetRequestCount; replace
_requestCounts with a
System.Collections.Concurrent.ConcurrentDictionary<string,int> and update
SendAsync to increment counts atomically (e.g., using AddOrUpdate or
TryGetValue/CompareExchange pattern) and adjust GetRequestCount to read from the
ConcurrentDictionary; update the constructor/type declaration for _requestCounts
and ensure SendAsync, GetRequestCount, and any initializations reference the new
ConcurrentDictionary to make the handler thread-safe.
Comment on lines
+55
to
+57
| if (value is string countryCode && !string.IsNullOrEmpty(countryCode)) | ||
| { | ||
| return CountryFlags.TryGetValue(countryCode.ToUpper(), out var flag) ? flag : countryCode; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In C#, is string.ToUpper() culture-sensitive, and is ToUpperInvariant recommended for normalizing ISO country codes?
💡 Result:
Yes, string.ToUpper is culture-sensitive. It uses the casing rules of the current culture by default (equivalent to ToUpper(CultureInfo.CurrentCulture)), which can lead to different results across cultures, such as the Turkish "i" becoming "İ" instead of "I". ToUpperInvariant is recommended for normalizing ISO country codes (ISO 3166-1 alpha-2 codes like "us", "gb"). These codes are standardized in uppercase using invariant casing rules, and ToUpperInvariant ensures consistent, culture-independent normalization for identifiers, keys, or comparisons. Official docs advise invariant methods for non-display string operations like normalization, file names, or security decisions. ISO standards require uppercase, aligning with ToUpperInvariant usage.
Citations:
- 1: https://learn.microsoft.com/en-us/dotnet/core/extensions/performing-culture-insensitive-string-operations
- 2: https://learn.microsoft.com/en-us/dotnet/api/system.string.toupperinvariant?view=net-9.0
- 3: https://msdn.microsoft.com/library/system.string.toupper
- 4: https://learn.microsoft.com/en-us/dotnet/api/system.string.toupper?view=net-10.0
- 5: https://msdn.microsoft.com/en-us/library/system.string.toupperinvariant
- 6: https://stackoverflow.com/questions/3550213/in-c-sharp-what-is-the-difference-between-toupper-and-toupperinvariant
- 7: https://ebudur.wordpress.com/2014/02/24/what-is-the-difference-between-toupper-and-toupperinvariant-methods-of-string-class-in-net/
- 8: https://stackoverflow.com/questions/773703/normalization-of-strings-with-string-toupperinvariant
- 9: https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/quality-rules/ca1308
- 10: https://learn.microsoft.com/en-us/dotnet/api/system.globalization.regioninfo.twoletterisoregionname?WT.mc_id=WD-MVP-5001077&view=net-7.0
- 11: ISO 3166-2 countryCodes must be uppercase zmap/zlint#855
🏁 Script executed:
#!/bin/bash
# First, find the GhostDisplayConverters.cs file
fd -type f -name "GhostDisplayConverters.cs"Repository: TeamWheelWizard/WheelWizard
Length of output: 241
🏁 Script executed:
# Once we have the file location, examine the specific lines mentioned
# Let's check the general structure around line 55-57
if [ -f "WheelWizard/Converters/GhostDisplayConverters.cs" ]; then
echo "=== Lines 50-65 of GhostDisplayConverters.cs ==="
sed -n '50,65p' "WheelWizard/Converters/GhostDisplayConverters.cs" | cat -n
echo ""
echo "=== Full context of method containing the code ==="
# Find the method context
head -70 "WheelWizard/Converters/GhostDisplayConverters.cs" | tail -30 | cat -n
fiRepository: TeamWheelWizard/WheelWizard
Length of output: 2713
🏁 Script executed:
# Search for other ToUpper() calls in the same file and related files
echo "=== Checking for other ToUpper() calls in GhostDisplayConverters.cs ==="
rg "ToUpper" "WheelWizard/Converters/GhostDisplayConverters.cs" -nRepository: TeamWheelWizard/WheelWizard
Length of output: 248
🏁 Script executed:
# Check how CountryFlags dictionary is defined and used
echo "=== Checking CountryFlags dictionary definition ==="
rg "CountryFlags" "WheelWizard/Converters/GhostDisplayConverters.cs" -A 2 -B 2 -nRepository: TeamWheelWizard/WheelWizard
Length of output: 618
🏁 Script executed:
# Check for other similar ToUpper() patterns in the Converters folder that might have the same issue
echo "=== All ToUpper() calls in Converters directory ==="
rg "ToUpper\(\)" "WheelWizard/Converters/" -nRepository: TeamWheelWizard/WheelWizard
Length of output: 277
🏁 Script executed:
# Check for dictionary lookups with ToUpper in the entire codebase
echo "=== Dictionary/Hash lookups with ToUpper in the codebase ==="
rg "TryGetValue.*ToUpper|\.ToUpper.*TryGetValue" -nRepository: TeamWheelWizard/WheelWizard
Length of output: 286
Use culture-invariant country-code normalization.
The code at line 57 uses countryCode.ToUpper(), which is culture-sensitive and can fail dictionary lookups in locales with special casing rules (e.g., Turkish). Since the CountryFlags dictionary uses standardized ISO 3166-1 alpha-2 codes in uppercase, use ToUpperInvariant() instead.
Suggested fix
- return CountryFlags.TryGetValue(countryCode.ToUpper(), out var flag) ? flag : countryCode;
+ return CountryFlags.TryGetValue(countryCode.ToUpperInvariant(), out var flag) ? flag : countryCode;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (value is string countryCode && !string.IsNullOrEmpty(countryCode)) | |
| { | |
| return CountryFlags.TryGetValue(countryCode.ToUpper(), out var flag) ? flag : countryCode; | |
| if (value is string countryCode && !string.IsNullOrEmpty(countryCode)) | |
| { | |
| return CountryFlags.TryGetValue(countryCode.ToUpperInvariant(), out var flag) ? flag : countryCode; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@WheelWizard/Converters/GhostDisplayConverters.cs` around lines 55 - 57, In
GhostDisplayConverters (the code path that looks up CountryFlags), normalize the
incoming countryCode using culture-invariant casing: replace the
culture-sensitive call to countryCode.ToUpper() with
countryCode.ToUpperInvariant() before looking up in the CountryFlags dictionary
so ISO 3166-1 alpha-2 lookups are reliable across locales.
Comment on lines
+11
to
+13
| public bool IsCustomTrack => TrackType == GhostTrackType.Custom; | ||
| public bool IsRetro => TrackType == GhostTrackType.Retro; | ||
| public string DisplayName => IsCustomTrack ? Name : $"{Console} {Name}"; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how DisplayName is used in the UI to verify expected format
rg -n "DisplayName" --type=cs -C3Repository: TeamWheelWizard/WheelWizard
Length of output: 34998
🏁 Script executed:
#!/bin/bash
# Find and examine WorldRecordsResponse.cs around the claimed lines
find . -name "WorldRecordsResponse.cs" -type fRepository: TeamWheelWizard/WheelWizard
Length of output: 115
🏁 Script executed:
#!/bin/bash
# Get full content of GhostTrack.cs
cat -n WheelWizard/Models/GhostTrack.csRepository: TeamWheelWizard/WheelWizard
Length of output: 1077
🏁 Script executed:
#!/bin/bash
# Search for ghost track usage patterns in UI pages, especially related to display
rg -n "GhostTrack|_selectedTrack" --type=cs WheelWizard/Views/Pages/GhostTimesPage.axaml.cs | head -50Repository: TeamWheelWizard/WheelWizard
Length of output: 3069
🏁 Script executed:
#!/bin/bash
# Read WorldRecordsResponse.cs around the claimed lines 70-74
cat -n WheelWizard/Models/WorldRecordsResponse.cs | sed -n '65,80p'Repository: TeamWheelWizard/WheelWizard
Length of output: 659
🏁 Script executed:
#!/bin/bash
# Search for where GhostTrack objects are created and Console is assigned
rg -n "Console\s*=" --type=cs -B2 -A2 | grep -A2 -B2 "GhostTrack"Repository: TeamWheelWizard/WheelWizard
Length of output: 618
🏁 Script executed:
#!/bin/bash
# Look for any tests or initialization of GhostTrack with Console values
rg -n "new GhostTrack|Console.*=.*Standard" --type=csRepository: TeamWheelWizard/WheelWizard
Length of output: 448
🏁 Script executed:
#!/bin/bash
# Check all usages of DisplayName property on GhostTrack objects
rg -n "\.DisplayName|_selectedTrack\.DisplayName" --type=cs -B1 -A1Repository: TeamWheelWizard/WheelWizard
Length of output: 759
🏁 Script executed:
#!/bin/bash
# Look at the full context of GhostsPage.axaml.cs where GhostTrack is created
cat -n WheelWizard/Views/Pages/GhostsPage.axaml.cs | sed -n '140,160p'Repository: TeamWheelWizard/WheelWizard
Length of output: 887
DisplayName for standard tracks will include "Standard" prefix.
The Console property is intentionally set to "Standard" for standard tracks (WorldRecordsResponse.cs:75), so DisplayName will produce strings like "Standard Mario Circuit". This format is consistent with how GhostTimesPage.axaml.cs displays track titles (lines 124-126), suggesting this is the intended behavior. However, consider whether including "Standard" in user-facing display names is the desired UX, or if standard tracks should display without a console prefix.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@WheelWizard/Models/GhostTrack.cs` around lines 11 - 13, DisplayName currently
prefixes non-custom tracks with Console (which can be the literal "Standard");
change it so standard tracks don't show the "Standard" console prefix by
returning just Name when TrackType == GhostTrackType.Retro or when Console ==
"Standard". Update the DisplayName getter (refer to the DisplayName property,
IsCustomTrack and Console) to conditionally omit Console for standard/retro
tracks and only include Console for non-standard custom consoles.
Comment on lines
+23
to
+35
| public enum GhostLocation | ||
| { | ||
| All, | ||
| Online, | ||
| Local | ||
| } | ||
|
|
||
| public enum GhostLocationFilter | ||
| { | ||
| All, | ||
| Online, | ||
| Local | ||
| } No newline at end of file |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
GhostLocation and GhostLocationFilter enums appear identical.
Both enums have the same values (All, Online, Local). Consider consolidating into a single enum to avoid confusion and potential maintenance burden.
🧹 Proposed fix - remove duplicate enum
public enum GhostLocation
{
All,
Online,
Local
}
-
-public enum GhostLocationFilter
-{
- All,
- Online,
- Local
-}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@WheelWizard/Models/GhostTrack.cs` around lines 23 - 35, The two enums
GhostLocation and GhostLocationFilter are identical; remove GhostLocationFilter
and consolidate all uses to GhostLocation (update method signatures, property
types, switch/case statements, serialization/deserialization code, and any tests
referencing GhostLocationFilter). Replace parameter types, return types, casts,
and attributes that reference GhostLocationFilter with GhostLocation, and
run/build to fix any compiler errors introduced by the rename; keep the enum
definition named GhostLocation and delete the GhostLocationFilter declaration.
Comment on lines
+64
to
+69
| track.TrackType = trackInfo.Category.ToLowerInvariant() switch | ||
| { | ||
| "retro" => GhostTrackType.Retro, | ||
| "custom" => GhostTrackType.Custom, | ||
| _ => GhostTrackType.All | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider logging when category doesn't match known values.
Unknown categories silently map to GhostTrackType.All. If new categories are added to the API, this could cause unexpected behavior. Consider logging when an unknown category is encountered.
♻️ Add logging for unknown categories
track.TrackType = trackInfo.Category.ToLowerInvariant() switch
{
"retro" => GhostTrackType.Retro,
"custom" => GhostTrackType.Custom,
- _ => GhostTrackType.All
+ var unknown => LogAndReturnDefault(unknown)
};
+
+ private static GhostTrackType LogAndReturnDefault(string category)
+ {
+ if (!string.IsNullOrEmpty(category))
+ Serilog.Log.Debug("Unknown track category '{Category}', defaulting to All", category);
+ return GhostTrackType.All;
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@WheelWizard/Models/WorldRecordsResponse.cs` around lines 64 - 69, The switch
expression silently maps unknown trackInfo.Category values to
GhostTrackType.All; change this so unknown categories are logged: replace the
switch expression assigning track.TrackType with logic that detects the default
case (either convert to a switch statement or a small helper
MapCategoryToGhostTrackType method) and emit a log entry via your existing
logger (e.g., ILogger) including the offending trackInfo.Category and a track
identifier (e.g., trackInfo.Id or track.Name) before returning
GhostTrackType.All; keep the known branches ("retro" => Retro, "custom" =>
Custom) unchanged.
Comment on lines
+74
to
+81
| var viewTimesButton = e.NameScope.Find<Button>("PART_ViewTimesButton"); | ||
| if (viewTimesButton != null) | ||
| { | ||
| viewTimesButton.Click += (_, args) => | ||
| { | ||
| ViewTimesClick?.Invoke(this, args); | ||
| }; | ||
| } |
There was a problem hiding this comment.
Event handler subscription may cause memory leak.
The Click event handler is subscribed in OnApplyTemplate but never unsubscribed. If the template is re-applied or the control is disposed, this could prevent garbage collection. Consider implementing IDisposable or using a weak event pattern, or unsubscribe in a cleanup method.
🛡️ Proposed fix to track and unsubscribe from event
+ private Button? _viewTimesButton;
+ private EventHandler<RoutedEventArgs>? _viewTimesClickHandler;
+
protected override void OnApplyTemplate(TemplateAppliedEventArgs e)
{
base.OnApplyTemplate(e);
- var viewTimesButton = e.NameScope.Find<Button>("PART_ViewTimesButton");
- if (viewTimesButton != null)
+ // Unsubscribe from previous button if template is re-applied
+ if (_viewTimesButton != null && _viewTimesClickHandler != null)
+ {
+ _viewTimesButton.Click -= _viewTimesClickHandler;
+ }
+
+ _viewTimesButton = e.NameScope.Find<Button>("PART_ViewTimesButton");
+ if (_viewTimesButton != null)
{
- viewTimesButton.Click += (_, args) =>
+ _viewTimesClickHandler = (_, args) =>
{
ViewTimesClick?.Invoke(this, args);
};
+ _viewTimesButton.Click += _viewTimesClickHandler;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| var viewTimesButton = e.NameScope.Find<Button>("PART_ViewTimesButton"); | |
| if (viewTimesButton != null) | |
| { | |
| viewTimesButton.Click += (_, args) => | |
| { | |
| ViewTimesClick?.Invoke(this, args); | |
| }; | |
| } | |
| private Button? _viewTimesButton; | |
| private EventHandler<RoutedEventArgs>? _viewTimesClickHandler; | |
| protected override void OnApplyTemplate(TemplateAppliedEventArgs e) | |
| { | |
| base.OnApplyTemplate(e); | |
| // Unsubscribe from previous button if template is re-applied | |
| if (_viewTimesButton != null && _viewTimesClickHandler != null) | |
| { | |
| _viewTimesButton.Click -= _viewTimesClickHandler; | |
| } | |
| _viewTimesButton = e.NameScope.Find<Button>("PART_ViewTimesButton"); | |
| if (_viewTimesButton != null) | |
| { | |
| _viewTimesClickHandler = (_, args) => | |
| { | |
| ViewTimesClick?.Invoke(this, args); | |
| }; | |
| _viewTimesButton.Click += _viewTimesClickHandler; | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@WheelWizard/Views/Patterns/TrackListItem.axaml.cs` around lines 74 - 81, The
Click handler for the PART_ViewTimesButton is attached in OnApplyTemplate via
viewTimesButton.Click but never removed, risking a memory leak; modify
TrackListItem to store the handler delegate (e.g., a private RoutedEventHandler
field) when subscribing in OnApplyTemplate (or remove any existing subscription
first) and unsubscribe it when the template is torn down or the control is
disposed—either implement IDisposable.Dispose to detach the handler or add a
cleanup override that finds PART_ViewTimesButton and does viewTimesButton.Click
-= storedHandler; ensure references to ViewTimesClick, viewTimesButton, and the
stored handler are used to safely unsubscribe.
Comment on lines
+90
to
+98
| protected override void OnPropertyChanged(AvaloniaPropertyChangedEventArgs change) | ||
| { | ||
| base.OnPropertyChanged(change); | ||
|
|
||
| if (change.Property == IsCustomTrackProperty || change.Property == IsRetroTrackProperty || change.Property == ConsoleProperty) | ||
| { | ||
| UpdateVisibility(); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
ConsoleProperty triggers UpdateVisibility but the visibility logic ignores it.
Line 94 includes ConsoleProperty in the property change check that triggers UpdateVisibility(), but UpdateVisibility() always sets _consoleLabel.IsVisible = false regardless of the Console value. Either remove ConsoleProperty from the trigger condition, or implement actual visibility logic based on the Console value.
♻️ Option A: Remove unused trigger
- if (change.Property == IsCustomTrackProperty || change.Property == IsRetroTrackProperty || change.Property == ConsoleProperty)
+ if (change.Property == IsCustomTrackProperty || change.Property == IsRetroTrackProperty)
{
UpdateVisibility();
}♻️ Option B: Implement Console-based visibility
private void UpdateVisibility()
{
if (_consoleLabel != null)
{
- _consoleLabel.IsVisible = false;
+ _consoleLabel.IsVisible = !string.IsNullOrEmpty(Console);
}Also applies to: 100-105
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@WheelWizard/Views/Patterns/TrackListItem.axaml.cs` around lines 90 - 98, The
OnPropertyChanged override currently checks IsCustomTrackProperty,
IsRetroTrackProperty and ConsoleProperty to call UpdateVisibility, but
UpdateVisibility always hides _consoleLabel (sets _consoleLabel.IsVisible =
false) and ignores the Console value; either remove ConsoleProperty from the
conditional in OnPropertyChanged so changes to Console don't call
UpdateVisibility, or update UpdateVisibility to respect the Console property
(read the Console getter/ConsoleProperty and set _consoleLabel.IsVisible
accordingly) so Console changes correctly affect visibility; modify either
OnPropertyChanged (remove ConsoleProperty) or UpdateVisibility (implement
Console-based visibility logic) and keep other checks for
IsCustomTrackProperty/IsRetroTrackProperty unchanged.
Comment on lines
+13
to
+149
| Title="Ghost Run Details" | ||
| WindowStartupLocation="CenterOwner" | ||
| Background="{StaticResource Neutral950}" | ||
| Icon="{StaticResource AppIconPath}"> | ||
|
|
||
| <Window.Resources> | ||
| <converters:CharacterIdToNameConverter x:Key="CharacterIdToName" /> | ||
| <converters:VehicleIdToNameConverter x:Key="VehicleIdToName" /> | ||
| <converters:DriftTypeToNameConverter x:Key="DriftTypeToName" /> | ||
| <converters:ControllerTypeToNameConverter x:Key="ControllerTypeToName" /> | ||
| <converters:LapTimesToAverageConverter x:Key="LapTimesToAverage" /> | ||
| <converters:CountryCodeToFlagConverter x:Key="CountryCodeToFlag" /> | ||
| </Window.Resources> | ||
|
|
||
| <ScrollViewer Padding="20"> | ||
| <StackPanel Spacing="20"> | ||
| <!-- Header --> | ||
| <Border Background="{StaticResource Neutral900}" CornerRadius="8" Padding="20"> | ||
| <StackPanel> | ||
| <StackPanel Orientation="Horizontal" Spacing="10" Margin="0,0,0,8"> | ||
| <TextBlock Text="{Binding PlayerName}" | ||
| Classes="PageTitleText" | ||
| FontSize="24" | ||
| FontWeight="Bold" /> | ||
| <TextBlock Text="{Binding CountryAlpha2, Converter={StaticResource CountryCodeToFlag}}" | ||
| Classes="BodyText" | ||
| FontSize="20" | ||
| VerticalAlignment="Center" /> | ||
| </StackPanel> | ||
| <StackPanel Orientation="Horizontal" Spacing="15"> | ||
| <TextBlock Text="{Binding FinishTimeDisplay, StringFormat=Final Time: {0}}" | ||
| Classes="BodyText" | ||
| FontSize="16" | ||
| FontWeight="SemiBold" | ||
| Foreground="{StaticResource Primary}" /> | ||
| <TextBlock Text="{Binding Rank, StringFormat=Rank: #{0}}" | ||
| Classes="BodyText" | ||
| FontSize="14" | ||
| Foreground="{StaticResource Neutral400}" /> | ||
| </StackPanel> | ||
| </StackPanel> | ||
| </Border> | ||
|
|
||
| <!-- Lap Times --> | ||
| <Border Background="{StaticResource Neutral900}" CornerRadius="8" Padding="20"> | ||
| <StackPanel> | ||
| <TextBlock Text="Lap Times" | ||
| Classes="BodyText" | ||
| FontSize="18" | ||
| FontWeight="Bold" | ||
| Margin="0,0,0,15" /> | ||
|
|
||
| <!-- Individual Lap Times --> | ||
| <Border Background="{StaticResource Neutral800}" CornerRadius="6" Padding="15" Margin="0,15,0,0"> | ||
| <StackPanel x:Name="LapTimesContainer" /> | ||
| </Border> | ||
| </StackPanel> | ||
| </Border> | ||
|
|
||
| <!-- Setup Details --> | ||
| <Border Background="{StaticResource Neutral900}" CornerRadius="8" Padding="20"> | ||
| <StackPanel> | ||
| <TextBlock Text="Setup Details" | ||
| Classes="BodyText" | ||
| FontSize="18" | ||
| FontWeight="Bold" | ||
| Margin="0,0,0,15" /> | ||
|
|
||
| <Grid ColumnDefinitions="*,*" RowDefinitions="Auto,Auto,Auto,Auto,Auto"> | ||
| <TextBlock Grid.Column="0" Grid.Row="0" | ||
| Text="Character:" | ||
| Classes="BodyText" | ||
| Foreground="{StaticResource Neutral400}" | ||
| Margin="0,0,10,6" /> | ||
| <TextBlock Grid.Column="1" Grid.Row="0" | ||
| Text="{Binding CharacterId, Converter={StaticResource CharacterIdToName}}" | ||
| Classes="BodyText" | ||
| HorizontalAlignment="Right" | ||
| Margin="10,0,0,6" /> | ||
|
|
||
| <TextBlock Grid.Column="0" Grid.Row="1" | ||
| Text="Vehicle:" | ||
| Classes="BodyText" | ||
| Foreground="{StaticResource Neutral400}" | ||
| Margin="0,0,10,6" /> | ||
| <TextBlock Grid.Column="1" Grid.Row="1" | ||
| Text="{Binding VehicleId, Converter={StaticResource VehicleIdToName}}" | ||
| Classes="BodyText" | ||
| HorizontalAlignment="Right" | ||
| Margin="10,0,0,6" /> | ||
|
|
||
| <TextBlock Grid.Column="0" Grid.Row="2" | ||
| Text="Drift:" | ||
| Classes="BodyText" | ||
| Foreground="{StaticResource Neutral400}" | ||
| Margin="0,0,10,6" /> | ||
| <TextBlock Grid.Column="1" Grid.Row="2" | ||
| Text="{Binding DriftType, Converter={StaticResource DriftTypeToName}}" | ||
| Classes="BodyText" | ||
| HorizontalAlignment="Right" | ||
| Margin="10,0,0,6" /> | ||
|
|
||
| <TextBlock Grid.Column="0" Grid.Row="3" | ||
| Text="Controller:" | ||
| Classes="BodyText" | ||
| Foreground="{StaticResource Neutral400}" | ||
| Margin="0,0,10,6" /> | ||
| <TextBlock Grid.Column="1" Grid.Row="3" | ||
| Text="{Binding ControllerType, Converter={StaticResource ControllerTypeToName}}" | ||
| Classes="BodyText" | ||
| HorizontalAlignment="Right" | ||
| Margin="10,0,0,6" /> | ||
|
|
||
| <TextBlock Grid.Column="0" Grid.Row="4" | ||
| Text="Date Set:" | ||
| Classes="BodyText" | ||
| Foreground="{StaticResource Neutral400}" /> | ||
| <TextBlock Grid.Column="1" Grid.Row="4" | ||
| Text="{Binding DateSet}" | ||
| Classes="BodyText" | ||
| FontFamily="DejaVu Sans Mono,Adwaita Mono,Consolas,Monaco,monospace" | ||
| HorizontalAlignment="Right" | ||
| Margin="10,0,0,0" /> | ||
| </Grid> | ||
| </StackPanel> | ||
| </Border> | ||
|
|
||
| <!-- Close Button --> | ||
| <StackPanel Orientation="Horizontal" HorizontalAlignment="Center" Margin="0,10,0,0"> | ||
| <Button Content="Close" | ||
| Padding="20,10" | ||
| Click="CloseButton_Click" | ||
| Background="{StaticResource Neutral800}" | ||
| Foreground="{StaticResource Neutral100}" | ||
| CornerRadius="6" | ||
| BorderThickness="0" /> | ||
| </StackPanel> |
There was a problem hiding this comment.
Localize popup strings for i18n consistency.
Hardcoded UI strings are used at Line 13 (Title), Line 59 (Lap Times), Line 75 (Setup Details), Lines 83/94/105/116/127 (field labels), and Line 142 (Close). These should use language resources like the rest of the app.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@WheelWizard/Views/Popups/GhostDetailsWindow.axaml` around lines 13 - 149, The
XAML uses hardcoded UI strings (Window Title, TextBlocks and Button Content)
which must be replaced with localization resource bindings: change Title="Ghost
Run Details" to bind to a resource key (e.g., Title="{StaticResource
Loc_GhostRunDetails}"), replace the Text of the TextBlock with "Lap Times" and
"Setup Details" and each field label ("Character:", "Vehicle:", "Drift:",
"Controller:", "Date Set:") and the Close Button Content ("Close") to use
localized resource keys (e.g., Text="{StaticResource Loc_LapTimes}",
Content="{StaticResource Loc_Close}"); add corresponding entries to your
localization resource dictionary and ensure the converters and existing bindings
(e.g., CountryCodeToFlag, CharacterIdToName) remain unchanged while swapping
literal strings for resource references.
Comment on lines
+21
to
+117
| private void PopulateLapTimes(GhostSubmission submission, GhostTrackInfo? trackInfo) | ||
| { | ||
| LapTimesContainer.Children.Clear(); | ||
|
|
||
| var expectedLaps = trackInfo?.Laps ?? 3; | ||
|
|
||
| var actualLaps = submission.LapSplitsDisplay.Take(expectedLaps).ToList(); | ||
|
|
||
| for (int i = 0; i < actualLaps.Count; i++) | ||
| { | ||
| var grid = new Grid | ||
| { | ||
| ColumnDefinitions = new ColumnDefinitions("*,*"), | ||
| Margin = new Avalonia.Thickness(0, 0, 0, 4) | ||
| }; | ||
|
|
||
| var lapLabel = new TextBlock | ||
| { | ||
| Text = $"Lap {i + 1}:", | ||
| Classes = { "BodyText" }, | ||
| Margin = new Avalonia.Thickness(0, 0, 10, 0) | ||
| }; | ||
| Grid.SetColumn(lapLabel, 0); | ||
|
|
||
| var lapTime = new TextBlock | ||
| { | ||
| Text = actualLaps[i], | ||
| Classes = { "BodyText" }, | ||
| FontFamily = "DejaVu Sans Mono,Adwaita Mono,Consolas,Monaco,monospace", | ||
| HorizontalAlignment = Avalonia.Layout.HorizontalAlignment.Right, | ||
| Margin = new Avalonia.Thickness(10, 0, 0, 0) | ||
| }; | ||
| Grid.SetColumn(lapTime, 1); | ||
|
|
||
| grid.Children.Add(lapLabel); | ||
| grid.Children.Add(lapTime); | ||
| LapTimesContainer.Children.Add(grid); | ||
| } | ||
|
|
||
| var avgGrid = new Grid | ||
| { | ||
| ColumnDefinitions = new ColumnDefinitions("*,*"), | ||
| Margin = new Avalonia.Thickness(0, 0, 0, 4) | ||
| }; | ||
|
|
||
| var avgLabel = new TextBlock | ||
| { | ||
| Text = "Average Lap:", | ||
| Classes = { "BodyText" }, | ||
| Margin = new Avalonia.Thickness(0, 0, 10, 0) | ||
| }; | ||
| Grid.SetColumn(avgLabel, 0); | ||
|
|
||
| var averageMs = submission.LapSplitsMs.Take(expectedLaps).Average(); | ||
| var avgTime = new TextBlock | ||
| { | ||
| Text = FormatTime((int)averageMs), | ||
| Classes = { "BodyText" }, | ||
| FontFamily = "DejaVu Sans Mono,Adwaita Mono,Consolas,Monaco,monospace", | ||
| HorizontalAlignment = Avalonia.Layout.HorizontalAlignment.Right, | ||
| Margin = new Avalonia.Thickness(10, 0, 0, 0) | ||
| }; | ||
| Grid.SetColumn(avgTime, 1); | ||
|
|
||
| avgGrid.Children.Add(avgLabel); | ||
| avgGrid.Children.Add(avgTime); | ||
| LapTimesContainer.Children.Add(avgGrid); | ||
|
|
||
| var fastestGrid = new Grid | ||
| { | ||
| ColumnDefinitions = new ColumnDefinitions("*,*"), | ||
| Margin = new Avalonia.Thickness(0, 0, 0, 0) | ||
| }; | ||
|
|
||
| var fastestLabel = new TextBlock | ||
| { | ||
| Text = "Fastest Lap:", | ||
| Classes = { "BodyText" }, | ||
| Margin = new Avalonia.Thickness(0, 0, 10, 0) | ||
| }; | ||
| Grid.SetColumn(fastestLabel, 0); | ||
|
|
||
| var fastestTime = new TextBlock | ||
| { | ||
| Text = submission.FastestLapDisplay, | ||
| Classes = { "BodyText" }, | ||
| FontFamily = "DejaVu Sans Mono,Adwaita Mono,Consolas,Monaco,monospace", | ||
| FontWeight = Avalonia.Media.FontWeight.SemiBold, | ||
| HorizontalAlignment = Avalonia.Layout.HorizontalAlignment.Right, | ||
| Margin = new Avalonia.Thickness(10, 0, 0, 0) | ||
| }; | ||
| Grid.SetColumn(fastestTime, 1); | ||
|
|
||
| fastestGrid.Children.Add(fastestLabel); | ||
| fastestGrid.Children.Add(fastestTime); | ||
| LapTimesContainer.Children.Add(fastestGrid); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider extracting lap row creation to reduce duplication.
The grid/label/time creation pattern is repeated for each lap, average, and fastest lap rows. A helper method could reduce duplication.
♻️ Proposed helper method
private static Grid CreateLapRow(string label, string time, bool isBold = false, int bottomMargin = 4)
{
var grid = new Grid
{
ColumnDefinitions = new ColumnDefinitions("*,*"),
Margin = new Avalonia.Thickness(0, 0, 0, bottomMargin)
};
var lapLabel = new TextBlock
{
Text = label,
Classes = { "BodyText" },
Margin = new Avalonia.Thickness(0, 0, 10, 0)
};
Grid.SetColumn(lapLabel, 0);
var lapTime = new TextBlock
{
Text = time,
Classes = { "BodyText" },
FontFamily = "DejaVu Sans Mono,Adwaita Mono,Consolas,Monaco,monospace",
HorizontalAlignment = Avalonia.Layout.HorizontalAlignment.Right,
Margin = new Avalonia.Thickness(10, 0, 0, 0)
};
if (isBold) lapTime.FontWeight = Avalonia.Media.FontWeight.SemiBold;
Grid.SetColumn(lapTime, 1);
grid.Children.Add(lapLabel);
grid.Children.Add(lapTime);
return grid;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@WheelWizard/Views/Popups/GhostDetailsWindow.axaml.cs` around lines 21 - 117,
PopulateLapTimes repeats grid/label/time creation logic for each lap, average,
and fastest rows; extract that into a helper (e.g., CreateLapRow) to remove
duplication: add a private static Grid CreateLapRow(string label, string time,
bool isBold = false, int bottomMargin = 4) that builds and returns the Grid
configured with two columns, BodyText label and right-aligned monospace time
(apply SemiBold when isBold), then replace the repeated constructions in
PopulateLapTimes (the per-lap loop, avgGrid creation using
FormatTime((int)averageMs), and fastestGrid using submission.FastestLapDisplay)
with calls to CreateLapRow and add the returned Grid to LapTimesContainer.
| }; | ||
| Grid.SetColumn(avgLabel, 0); | ||
|
|
||
| var averageMs = submission.LapSplitsMs.Take(expectedLaps).Average(); |
There was a problem hiding this comment.
Average() throws if LapSplitsMs is empty.
If submission.LapSplitsMs is empty, Take(expectedLaps).Average() throws InvalidOperationException. This could occur with malformed API data or edge cases.
🐛 Proposed fix with null check
- var averageMs = submission.LapSplitsMs.Take(expectedLaps).Average();
+ var lapsForAverage = submission.LapSplitsMs.Take(expectedLaps).ToList();
+ var averageMs = lapsForAverage.Count > 0 ? lapsForAverage.Average() : 0;
var avgTime = new TextBlock
{
Text = FormatTime((int)averageMs),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| var averageMs = submission.LapSplitsMs.Take(expectedLaps).Average(); | |
| var lapsForAverage = submission.LapSplitsMs.Take(expectedLaps).ToList(); | |
| var averageMs = lapsForAverage.Count > 0 ? lapsForAverage.Average() : 0; | |
| var avgTime = new TextBlock | |
| { | |
| Text = FormatTime((int)averageMs), |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@WheelWizard/Views/Popups/GhostDetailsWindow.axaml.cs` at line 74, The
Average() call on submission.LapSplitsMs can throw if the sequence is empty;
update the computation in GhostDetailsWindow.axaml.cs (the averageMs
calculation) to handle empty sequences by either checking sequence.Any() before
calling Average() and using a safe fallback (e.g., 0 or null) or by using
Take(expectedLaps).DefaultIfEmpty(0).Average() so Average never runs on an empty
sequence; ensure downstream code handles the chosen fallback consistently.
Comment on lines
+32
to
+33
| public double AverageLapMs => LapSplitsMs.Count > 0 ? LapSplitsMs.Select(x => (double)x).Average() : 0; | ||
| public string AverageLapDisplay => FormatTime((uint)AverageLapMs); |
There was a problem hiding this comment.
Average lap display truncates instead of rounding.
(uint)AverageLapMs drops fractional milliseconds; displayed average can be systematically low.
Proposed fix
- public string AverageLapDisplay => FormatTime((uint)AverageLapMs);
+ public string AverageLapDisplay => FormatTime((uint)Math.Round(AverageLapMs, MidpointRounding.AwayFromZero));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| public double AverageLapMs => LapSplitsMs.Count > 0 ? LapSplitsMs.Select(x => (double)x).Average() : 0; | |
| public string AverageLapDisplay => FormatTime((uint)AverageLapMs); | |
| public double AverageLapMs => LapSplitsMs.Count > 0 ? LapSplitsMs.Select(x => (double)x).Average() : 0; | |
| public string AverageLapDisplay => FormatTime((uint)Math.Round(AverageLapMs, MidpointRounding.AwayFromZero)); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@WheelWizard/Models/LocalGhostData.cs` around lines 32 - 33, AverageLapDisplay
currently truncates fractional milliseconds by casting AverageLapMs to uint;
change it to round the average before converting to an unsigned integer. Update
the AverageLapDisplay property to call Math.Round (or an equivalent rounding
method) on AverageLapMs and then convert to uint when passing into FormatTime,
referencing the AverageLapMs and AverageLapDisplay members and FormatTime to
locate the change.
Comment on lines
+77
to
+83
| public LocalGhostData? BestTime150 => Ghosts150.Concat(VariantGhosts150) | ||
| .OrderBy(g => g.TotalTimeMs) | ||
| .FirstOrDefault(); | ||
|
|
||
| public LocalGhostData? BestTime200 => Ghosts200.Concat(VariantGhosts200) | ||
| .OrderBy(g => g.TotalTimeMs) | ||
| .FirstOrDefault(); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Avoid sorting the full list to compute best time.
OrderBy(...).FirstOrDefault() performs full sort on each property access. A linear min scan is cheaper and clearer.
Proposed fix
public LocalGhostData? BestTime150 => Ghosts150.Concat(VariantGhosts150)
- .OrderBy(g => g.TotalTimeMs)
- .FirstOrDefault();
+ .Aggregate((LocalGhostData?)null, (best, g) => best is null || g.TotalTimeMs < best.TotalTimeMs ? g : best);
public LocalGhostData? BestTime200 => Ghosts200.Concat(VariantGhosts200)
- .OrderBy(g => g.TotalTimeMs)
- .FirstOrDefault();
+ .Aggregate((LocalGhostData?)null, (best, g) => best is null || g.TotalTimeMs < best.TotalTimeMs ? g : best);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| public LocalGhostData? BestTime150 => Ghosts150.Concat(VariantGhosts150) | |
| .OrderBy(g => g.TotalTimeMs) | |
| .FirstOrDefault(); | |
| public LocalGhostData? BestTime200 => Ghosts200.Concat(VariantGhosts200) | |
| .OrderBy(g => g.TotalTimeMs) | |
| .FirstOrDefault(); | |
| public LocalGhostData? BestTime150 => Ghosts150.Concat(VariantGhosts150) | |
| .Aggregate((LocalGhostData?)null, (best, g) => best is null || g.TotalTimeMs < best.TotalTimeMs ? g : best); | |
| public LocalGhostData? BestTime200 => Ghosts200.Concat(VariantGhosts200) | |
| .Aggregate((LocalGhostData?)null, (best, g) => best is null || g.TotalTimeMs < best.TotalTimeMs ? g : best); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@WheelWizard/Models/LocalGhostData.cs` around lines 77 - 83, The properties
BestTime150 and BestTime200 currently call OrderBy(...).FirstOrDefault(), which
sorts the entire concatenated sequence; replace that with a linear min scan
(e.g., use LINQ's MinBy(g => g.TotalTimeMs) if available or implement a simple
loop/aggregate) over Ghosts150.Concat(VariantGhosts150) and
Ghosts200.Concat(VariantGhosts200) to return the element with smallest
TotalTimeMs (return null if the concatenated sequence is empty); update both
BestTime150 and BestTime200 to use the linear min approach, preserving the
return type LocalGhostData?.
Comment on lines
+117
to
+131
| public void SetTrack(GhostTrack track) | ||
| { | ||
| _selectedTrack = track; | ||
| _ = _ghostTrackService.EnsureTrackMappingsInitializedAsync(); | ||
|
|
||
| if (_selectedTrack != null) | ||
| { | ||
| TrackTitle.Text = string.IsNullOrEmpty(_selectedTrack.Console) | ||
| ? _selectedTrack.Name | ||
| : $"{_selectedTrack.Console} {_selectedTrack.Name}"; | ||
|
|
||
| if (IsOnlineTabSelected) | ||
| { | ||
| _ = LoadOnlineLeaderboardSafe(); | ||
| } |
There was a problem hiding this comment.
Avoid fire-and-forget track mapping initialization in SetTrack.
_ghostTrackService.EnsureTrackMappingsInitializedAsync() is started but not awaited, so initialization failures are unobserved and dependent flows can run before mappings are ready.
Proposed fix
-public void SetTrack(GhostTrack track)
+public async Task SetTrackAsync(GhostTrack track)
{
_selectedTrack = track;
- _ = _ghostTrackService.EnsureTrackMappingsInitializedAsync();
+ await _ghostTrackService.EnsureTrackMappingsInitializedAsync();
if (_selectedTrack != null)
{
TrackTitle.Text = string.IsNullOrEmpty(_selectedTrack.Console)
? _selectedTrack.Name
: $"{_selectedTrack.Console} {_selectedTrack.Name}";
if (IsOnlineTabSelected)
{
- _ = LoadOnlineLeaderboardSafe();
+ await LoadOnlineLeaderboardSafe();
}
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| public void SetTrack(GhostTrack track) | |
| { | |
| _selectedTrack = track; | |
| _ = _ghostTrackService.EnsureTrackMappingsInitializedAsync(); | |
| if (_selectedTrack != null) | |
| { | |
| TrackTitle.Text = string.IsNullOrEmpty(_selectedTrack.Console) | |
| ? _selectedTrack.Name | |
| : $"{_selectedTrack.Console} {_selectedTrack.Name}"; | |
| if (IsOnlineTabSelected) | |
| { | |
| _ = LoadOnlineLeaderboardSafe(); | |
| } | |
| public async Task SetTrackAsync(GhostTrack track) | |
| { | |
| _selectedTrack = track; | |
| await _ghostTrackService.EnsureTrackMappingsInitializedAsync(); | |
| if (_selectedTrack != null) | |
| { | |
| TrackTitle.Text = string.IsNullOrEmpty(_selectedTrack.Console) | |
| ? _selectedTrack.Name | |
| : $"{_selectedTrack.Console} {_selectedTrack.Name}"; | |
| if (IsOnlineTabSelected) | |
| { | |
| await LoadOnlineLeaderboardSafe(); | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@WheelWizard/Views/Pages/GhostTimesPage.axaml.cs` around lines 117 - 131,
SetTrack currently fires
_ghostTrackService.EnsureTrackMappingsInitializedAsync() without awaiting it,
leaving failures unobserved and allowing dependent work (e.g.,
LoadOnlineLeaderboardSafe) to run before mappings exist; change SetTrack to
await EnsureTrackMappingsInitializedAsync (or make an async SetTrackAsync) and
handle exceptions (try/catch and log or surface) before proceeding to set
TrackTitle and calling LoadOnlineLeaderboardSafe when IsOnlineTabSelected, so
mapping initialization completes deterministically.
Comment on lines
+472
to
+477
| private void LocalCcChanged(object? sender, RoutedEventArgs e) | ||
| { | ||
| LoadGhostsForSelectedCc(); | ||
| _localGhostService.RefreshGhostData(); | ||
| LoadLocalGhosts(); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
CC toggle currently does redundant work and extra disk refresh.
LocalCcChanged first rebinds from memory, then forces full cache refresh and reload. For simple CC switching, this causes avoidable I/O and UI churn.
Proposed fix
private void LocalCcChanged(object? sender, RoutedEventArgs e)
{
LoadGhostsForSelectedCc();
- _localGhostService.RefreshGhostData();
- LoadLocalGhosts();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| private void LocalCcChanged(object? sender, RoutedEventArgs e) | |
| { | |
| LoadGhostsForSelectedCc(); | |
| _localGhostService.RefreshGhostData(); | |
| LoadLocalGhosts(); | |
| } | |
| private void LocalCcChanged(object? sender, RoutedEventArgs e) | |
| { | |
| LoadGhostsForSelectedCc(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@WheelWizard/Views/Pages/GhostTimesPage.axaml.cs` around lines 472 - 477,
LocalCcChanged does redundant work by rebinding then forcing a full disk
refresh; change it so toggling CC only rebinds in-memory unless the CC actually
changed the underlying local cache. In LocalCcChanged, call
LoadGhostsForSelectedCc() only for simple toggles and remove the unconditional
_localGhostService.RefreshGhostData() and LoadLocalGhosts() calls;
alternatively, add a guard that compares the new CC selection to the previous
selection (store previous in a field like _currentCc) and only invoke
_localGhostService.RefreshGhostData() and LoadLocalGhosts() when the selection
truly changed or when the cache is stale.
Collaborator
|
Looks amazing, great work! One thing I noticed was that the "Back" button in a track's detail view looks off (it's still functional though) -- maybe it should be further up as in the room details page. Other than that, I haven't noticed any major issues when testing it out. |
Member
|
This PR doesn’t really follow the patterns we already have in place in Wheel Wizard. there are several emoji buttons and we’ve already moved to Refit instead of creating new HTTP clients directly. In its current state, I can’t merge this PR. I’m happy to do a full review and point out the changes needed, but only if those issues are actually going to be addressed properly. Otherwise, I’d rather not spend the time on a detailed review that won’t lead to a mergeable result. |
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
4 participants
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.
Purpose of this PR
It adds ghost support (track list, track selection, ghost path, and local ghost handling) using dynamic mapping logic instead of a hardcoded table approach.
How to Test:
What Has Been Changed:
Added first implementation of Ghost logic.
Implemented dynamic track variant mapping from API track metadata (courseId grouping).
Implemented dynamic hash resolution without hardcoded hash dictionaries.
Added fallback mapping support for non-slot/symbolic track naming.
Added lazy/on-demand hash computation with cache to improve UX.
Added test coverage for ghost variant mapping and ghost track service behaviour.
Related Issue Link:
N/A
Checklist before merging
Summary by CodeRabbit
New Features
Tests