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:
✨ 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: 9
🤖 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/Features/GameBanana/Domain/GameBananaTag.cs`:
- Around line 19-20: The converter's Read path in GameBananaTag.cs returns an
empty array when reader.TokenType != JsonTokenType.StartArray without advancing
the Utf8JsonReader, which violates System.Text.Json expectations; update the
Read implementation in the GameBananaTag converter (the Read method in
GameBananaTag or its JsonConverter) to consume/skip the unexpected token (e.g.,
call reader.Skip() or otherwise advance the reader past the current value)
before returning an empty collection so the reader position is correct for
subsequent parsing.
In `@WheelWizard/Services/Launcher/Helpers/ModsLaunchHelper.cs`:
- Around line 77-84: The check that decides whether to prompt/clear the target
Patches folder only inspects top-level files and so flattens nested mod trees;
update checks and copy logic in ModsLaunchHelper to work recursively: replace
Directory.EnumerateFiles(targetFolderPath).Any() with
Directory.EnumerateFiles(targetFolderPath, "*",
SearchOption.AllDirectories).Any() so nested files trigger the clear prompt and
deletion still uses Directory.Delete(targetFolderPath, true). When staging files
in patch mode, preserve each file's relative path (use Path.GetRelativePath or
subtract the mod folder root) and recreate subdirectories under targetFolderPath
before copying so subfolder structure and same-named files from different paths
do not overwrite each other; adjust the copy loop that currently uses bare
filenames to build destination paths using the computed relative path.
In `@WheelWizard/Services/ModStorageSystem.cs`:
- Around line 20-30: GetDisplayName and GetClearFolderPrompt are returning
hardcoded English strings for ModStorageSystem.Patches ("Patches" and the
clear-folder prompt) instead of using localized resources like
Common.PageTitle_Mods and Phrases.*; update these to use new localization keys
(e.g., Common.PageTitle_Patches and Phrases.Question_LaunchClearPatches_Extra)
added to your resource files, replace the literal "Patches" in GetDisplayName
and the literal prompt in GetClearFolderPrompt with those resource lookups, and
add corresponding entries in the language resource files so all branches of the
ModStorageSystem enum are localized consistently.
- Around line 48-60: The null-check `normalizedTitle is not null` in
IsPatchesTag is redundant because normalizedTitle is guaranteed non-null/
non-whitespace after the early return; remove that clause and simplify the
return to only evaluate the titleOnly comparisons (using titleOnly.Equals(...,
StringComparison.OrdinalIgnoreCase) for "patch" and "patches"); keep the earlier
Trim(), Split(':', 2)[0].Trim() logic for titleOnly and leave the initial
string.IsNullOrWhiteSpace guard intact.
In `@WheelWizard/Views/Pages/ModsPage.axaml`:
- Around line 29-32: Replace the hard-coded label "Use Patches" on the CheckBox
with a lookup to the app's localization resources so the text follows the
current language; edit the CheckBox (Classes="Switch", IsChecked="{Binding
UsePatches, Mode=TwoWay}") and change Content to use the existing resource key
(e.g., Content="{StaticResource UsePatches}" or Content="{DynamicResource
UsePatches}" / or Content="{Binding Source={StaticResource Localization},
Path=UsePatchesLabel}" depending on your resource pattern) so the UI reads the
localized string instead of inline text.
In `@WheelWizard/Views/Patterns/ModBrowserListItem.axaml`:
- Around line 76-83: The badge TextBlock inside PART_PatchesBadge currently uses
a hardcoded string "Uses Patches!" so it won't be localized; change the Text
attribute on that TextBlock to reference the app's localization resource (e.g.,
replace the literal with a resource binding such as
StaticResource/DynamicResource for a key like UsesPatches_Text or use your
project's localization markup extension) and add the corresponding key/value to
the localization resource files for each supported language (update the resource
dictionaries or .resx/loc files used by the app). Ensure you modify the
TextBlock element (the Text property) and add the new resource key to the
translation files so the badge text is localized.
In `@WheelWizard/Views/Popups/ModManagement/ModBrowserWindow.axaml`:
- Around line 32-37: The "Patches only" label on the PatchesOnlyToggle CheckBox
is hard-coded and must be localized; change the Content attribute for the
control named PatchesOnlyToggle to use a localization resource key (e.g.,
Content="{DynamicResource PatchesOnlyLabel}" or Content="{Binding
LocalizedStrings.PatchesOnly}" depending on the project's localization pattern)
instead of the inline string, and add the corresponding "PatchesOnlyLabel" entry
to the app's localization resources so translators can provide translations.
In `@WheelWizard/Views/Popups/ModManagement/ModBrowserWindow.axaml.cs`:
- Around line 100-107: LoadMods currently calls EnsurePatchesOnlyResultsAsync
which itself loops and calls LoadMods, creating a recursive nested call chain;
fix by adding a flag parameter to LoadMods (e.g., bool skipEnsurePatchesOnly =
false) and only call EnsurePatchesOnlyResultsAsync when that flag is false, then
update EnsurePatchesOnlyResultsAsync to call LoadMods(...,
skipEnsurePatchesOnly:true) to avoid re-entering EnsurePatchesOnlyResultsAsync;
alternatively move the EnsurePatchesOnlyResultsAsync call out of LoadMods and
invoke it from the callers ModPopupWindow_Loaded, ModListView_ScrollChanged, and
ReloadSearchResults so LoadMods no longer triggers the loop.
- Around line 233-237: EnsurePatchesOnlyResultsAsync can loop indefinitely when
ShowPatchesOnly is true; add an upper bound so it only fetches a limited number
of additional pages (e.g., scan up to 5 pages). Implement a local counter or
constant (maxPagesToScan) and loop while ShowPatchesOnly && _hasMoreMods &&
!LoadedMods.Any(mod => mod.Mod.UsesPatches) && pagesScanned < maxPagesToScan,
calling LoadMods(_currentPage + 1, _currentSearchTerm) and incrementing
pagesScanned; if the loop exits because pagesScanned reached the limit and no
patches were found, set a clear UI signal (e.g., set a NoPatchesFound flag or
call an existing method to show a “no patches found” message) so the user is
informed instead of continuing to fetch indefinitely.
🪄 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: 455f258e-7d29-4433-b992-562350e6a11c
📒 Files selected for processing (18)
WheelWizard/Features/GameBanana/Domain/GameBananaModPreview.csWheelWizard/Features/GameBanana/Domain/GameBananaTag.csWheelWizard/Features/Settings/ISettingsServices.csWheelWizard/Features/Settings/SettingsManager.csWheelWizard/Services/Launcher/Helpers/ModsLaunchHelper.csWheelWizard/Services/Launcher/RrBetaLauncher.csWheelWizard/Services/Launcher/RrLauncher.csWheelWizard/Services/ModStorageSystem.csWheelWizard/Services/PathManager.csWheelWizard/Views/App.axaml.csWheelWizard/Views/Layout.axamlWheelWizard/Views/Layout.axaml.csWheelWizard/Views/Pages/ModsPage.axamlWheelWizard/Views/Pages/ModsPage.axaml.csWheelWizard/Views/Patterns/ModBrowserListItem.axamlWheelWizard/Views/Patterns/ModBrowserListItem.axaml.csWheelWizard/Views/Popups/ModManagement/ModBrowserWindow.axamlWheelWizard/Views/Popups/ModManagement/ModBrowserWindow.axaml.cs
Comment on lines
+20
to
+30
| public static string GetDisplayName(ModStorageSystem storageSystem) => | ||
| storageSystem == ModStorageSystem.Patches ? "Patches" : Common.PageTitle_Mods; | ||
|
|
||
| public static string GetClearFolderPrompt(ModStorageSystem storageSystem) | ||
| { | ||
| if (storageSystem == ModStorageSystem.MyStuff) | ||
| return Phrases.Question_LaunchClearModsFound_Extra; | ||
|
|
||
| //todo: translation lol | ||
| return "You are about to launch the game without mods. Do you want to clear your Patches folder?"; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Address missing localization for Patches display strings.
Lines 21 and 29 contain hardcoded English strings ("Patches" and the prompt text) while MyStuff uses localized resources. The TODO on line 28 acknowledges this. Consider adding these strings to your language resources for consistency.
Would you like me to help identify all the required localization keys and open an issue to track this?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@WheelWizard/Services/ModStorageSystem.cs` around lines 20 - 30,
GetDisplayName and GetClearFolderPrompt are returning hardcoded English strings
for ModStorageSystem.Patches ("Patches" and the clear-folder prompt) instead of
using localized resources like Common.PageTitle_Mods and Phrases.*; update these
to use new localization keys (e.g., Common.PageTitle_Patches and
Phrases.Question_LaunchClearPatches_Extra) added to your resource files, replace
the literal "Patches" in GetDisplayName and the literal prompt in
GetClearFolderPrompt with those resource lookups, and add corresponding entries
in the language resource files so all branches of the ModStorageSystem enum are
localized consistently.
Comment on lines
+29
to
+32
| <CheckBox Classes="Switch" | ||
| VerticalAlignment="Center" | ||
| Content="Use Patches" | ||
| IsChecked="{Binding UsePatches, Mode=TwoWay}" /> |
There was a problem hiding this comment.
Localize the storage-mode switch label.
"Use Patches" should come from the existing language resources instead of inline text so it changes with the rest of the page.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@WheelWizard/Views/Pages/ModsPage.axaml` around lines 29 - 32, Replace the
hard-coded label "Use Patches" on the CheckBox with a lookup to the app's
localization resources so the text follows the current language; edit the
CheckBox (Classes="Switch", IsChecked="{Binding UsePatches, Mode=TwoWay}") and
change Content to use the existing resource key (e.g., Content="{StaticResource
UsePatches}" or Content="{DynamicResource UsePatches}" / or Content="{Binding
Source={StaticResource Localization}, Path=UsePatchesLabel}" depending on your
resource pattern) so the UI reads the localized string instead of inline text.
Comment on lines
+76
to
+83
| <Border x:Name="PART_PatchesBadge" | ||
| IsVisible="False" | ||
| Margin="0,6,0,0" | ||
| Padding="6,2" | ||
| Background="#1E5E3A" | ||
| CornerRadius="9999" | ||
| HorizontalAlignment="Left"> | ||
| <TextBlock Classes="TinyText" Text="Uses Patches!" Foreground="#D6FFE2" /> |
There was a problem hiding this comment.
Localize the patch badge text.
"Uses Patches!" bypasses the app's language resources, so this badge will stay English even when the rest of the browser is translated.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@WheelWizard/Views/Patterns/ModBrowserListItem.axaml` around lines 76 - 83,
The badge TextBlock inside PART_PatchesBadge currently uses a hardcoded string
"Uses Patches!" so it won't be localized; change the Text attribute on that
TextBlock to reference the app's localization resource (e.g., replace the
literal with a resource binding such as StaticResource/DynamicResource for a key
like UsesPatches_Text or use your project's localization markup extension) and
add the corresponding key/value to the localization resource files for each
supported language (update the resource dictionaries or .resx/loc files used by
the app). Ensure you modify the TextBlock element (the Text property) and add
the new resource key to the translation files so the badge text is localized.
Comment on lines
+32
to
+37
| <CheckBox Grid.Row="2" | ||
| x:Name="PatchesOnlyToggle" | ||
| Classes="Switch" | ||
| Margin="0,0,0,8" | ||
| Content="Patches only" | ||
| Click="PatchesOnlyToggle_Click" /> |
There was a problem hiding this comment.
Localize the new filter label.
"Patches only" is user-facing text in a localized view; keeping it inline will leave this control untranslated.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@WheelWizard/Views/Popups/ModManagement/ModBrowserWindow.axaml` around lines
32 - 37, The "Patches only" label on the PatchesOnlyToggle CheckBox is
hard-coded and must be localized; change the Content attribute for the control
named PatchesOnlyToggle to use a localization resource key (e.g.,
Content="{DynamicResource PatchesOnlyLabel}" or Content="{Binding
LocalizedStrings.PatchesOnly}" depending on the project's localization pattern)
instead of the inline string, and add the corresponding "PatchesOnlyLabel" entry
to the app's localization resources so translators can provide translations.
Comment on lines
+233
to
+237
| private async Task EnsurePatchesOnlyResultsAsync() | ||
| { | ||
| while (ShowPatchesOnly && _hasMoreMods && !LoadedMods.Any(mod => mod.Mod.UsesPatches)) | ||
| await LoadMods(_currentPage + 1, _currentSearchTerm); | ||
| } |
There was a problem hiding this comment.
Consider adding an upper bound on pages loaded.
If the API returns many consecutive pages without any patches-tagged mods, this loop will continue making API calls until _hasMoreMods becomes false. This could result in excessive network requests and a poor user experience.
Consider adding a maximum page limit (e.g., 5 additional pages) and showing a message to the user if no patches are found.
🛡️ Proposed safeguard
private async Task EnsurePatchesOnlyResultsAsync()
{
+ const int maxExtraPages = 5;
+ var pagesLoaded = 0;
- while (ShowPatchesOnly && _hasMoreMods && !LoadedMods.Any(mod => mod.Mod.UsesPatches))
+ while (ShowPatchesOnly && _hasMoreMods && pagesLoaded < maxExtraPages && !LoadedMods.Any(mod => mod.Mod.UsesPatches))
+ {
await LoadMods(_currentPage + 1, _currentSearchTerm, ensurePatchResults: false);
+ pagesLoaded++;
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@WheelWizard/Views/Popups/ModManagement/ModBrowserWindow.axaml.cs` around
lines 233 - 237, EnsurePatchesOnlyResultsAsync can loop indefinitely when
ShowPatchesOnly is true; add an upper bound so it only fetches a limited number
of additional pages (e.g., scan up to 5 pages). Implement a local counter or
constant (maxPagesToScan) and loop while ShowPatchesOnly && _hasMoreMods &&
!LoadedMods.Any(mod => mod.Mod.UsesPatches) && pagesScanned < maxPagesToScan,
calling LoadMods(_currentPage + 1, _currentSearchTerm) and incrementing
pagesScanned; if the loop exits because pagesScanned reached the limit and no
patches were found, set a clear UI signal (e.g., set a NoPatchesFound flag or
call an existing method to show a “no patches found” message) so the user is
informed instead of continuing to fetch indefinitely.
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
1 participant
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.
Summary by CodeRabbit
Release Notes