Skip to content

Make settings values type safe and centralize path reloads - #499

Open
DirkDoes wants to merge 1 commit into
task/settings-persistencefrom
task/typed-settings
Open

DirkDoes wants to merge 1 commit into
task/settings-persistencefrom
task/typed-settings

Conversation

@DirkDoes

@DirkDoes DirkDoes commented Sep 28, 2026 •

Copy link
Copy Markdown
Member

Purpose of this PR:

Replace object-based setting access with Setting and typed manager methods. Incorrect value types are rejected by the compiler. Runtime parsing stays at JSON/INI/TOML boundaries, with domain validation retained.

Reload Dolphin settings centrally when its executable/user path changes. Settings UI edits surface persistence failures. Persisted property names and representations stay unchanged.

Based on #498. Merge after #498; works without later layers.

How to Test:

dotnet test WheelWizard.sln

551 tests passed (532 unit, 19 headless UI), including the typed API contract and the existing settings/path suites.

What Has Been Changed:

See the focused implementation above. The existing settings JSON contract and imported translation sheets are preserved.

Related Issue Link:

No linked issue. Part of native GitHub stack #503.

Checklist before merging

  • You have created relevant tests

Summary by CodeRabbit

  • New Features
    • Decimal values are now supported in Dolphin and Wheel Wizard settings.
    • Settings changes that fail to save display the relevant error message when available, with a fallback warning.
  • Bug Fixes
    • Improved reliability when updating settings, including handling save failures and restoring values when updates cannot be applied.
    • Window layout scaling now uses the current configured scale consistently.

@coderabbitai

coderabbitai Bot commented Sep 28, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Advanced

Run ID: 7d4fcf07-0d9e-477e-a518-7485c9f0f7d8

📥 Commits

Reviewing files that changed from the base of the PR and between 65e43f8 and 6f1a537.

📒 Files selected for processing (29)
  • WheelWizard.Test/Features/Dolphin/FeaturePathOwnershipTests.cs
  • WheelWizard.Test/Features/Launching/DolphinLaunchServiceTests.cs
  • WheelWizard.Test/Features/Launching/LaunchPathTests.cs
  • WheelWizard.Test/Features/Launching/RetroRewindLaunchServiceTests.cs
  • WheelWizard.Test/Features/MiiRepositoryServiceTests.cs
  • WheelWizard.Test/Features/Settings/DolphinSettingsTests.cs
  • WheelWizard.Test/Features/Settings/SettingsPersistencePathTests.cs
  • WheelWizard.Test/Features/Settings/SettingsRecoveryTests.cs
  • WheelWizard.Test/Features/Settings/SettingsTests.cs
  • WheelWizard.Test/Features/Settings/VirtualSettingsTests.cs
  • WheelWizard.Test/Features/Settings/WhWzSettingsTests.cs
  • WheelWizard.Test/Views/HomeDolphinLaunchTests.cs
  • WheelWizard/Features/Settings/DolphinSettingManager.cs
  • WheelWizard/Features/Settings/ISettingsServices.cs
  • WheelWizard/Features/Settings/RecompSettingManager.cs
  • WheelWizard/Features/Settings/SettingsManager.cs
  • WheelWizard/Features/Settings/Types/DolphinSetting.cs
  • WheelWizard/Features/Settings/Types/RecompSetting.cs
  • WheelWizard/Features/Settings/Types/Setting.cs
  • WheelWizard/Features/Settings/Types/SettingConstants.cs
  • WheelWizard/Features/Settings/Types/VirtualSetting.cs
  • WheelWizard/Features/Settings/Types/WhWzSetting.cs
  • WheelWizard/Features/Settings/WhWzSettingManager.cs
  • WheelWizard/Views/Layout.axaml.cs
  • WheelWizard/Views/Pages/Settings/OtherSettings.axaml.cs
  • WheelWizard/Views/Pages/Settings/RecompSettings.axaml.cs
  • WheelWizard/Views/Pages/Settings/SettingsEditing.cs
  • WheelWizard/Views/Pages/Settings/VideoSettings.axaml.cs
  • WheelWizard/Views/Pages/Settings/WhWzSettings.axaml.cs

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 2 remain after this review.


📝 Walkthrough

Walkthrough

Settings now use generic value types and typed manager APIs. Setting implementations share a generic base, and manager registration uses setting-specific interfaces. Settings views route writes through a helper that handles failed updates. Tests use generic setting types and cover decimal parsing.

Changes

Typed Settings

Layer / File(s) Summary
Generic setting model
WheelWizard/Features/Settings/Types/Setting.cs, WheelWizard/Features/Settings/Types/{DolphinSetting,RecompSetting,WhWzSetting,VirtualSetting}.cs, WheelWizard/Features/Settings/Types/SettingConstants.cs
The non-generic setting base now provides shared change and validity behavior. Generic setting classes provide typed values and validators. Dolphin and Wheel Wizard parsing now accept decimal values in the updated tests; recomp double parsing rejects non-finite values.
Typed manager integration
WheelWizard/Features/Settings/ISettingsServices.cs, WheelWizard/Features/Settings/SettingsManager.cs, WheelWizard/Features/Settings/{DolphinSettingManager,RecompSettingManager,WhWzSettingManager}.cs
Settings properties, manager methods, registration, and validation use typed settings. The managers store setting-specific interfaces. Recommended-settings writes now throw an IOException when a component setting cannot be saved.
Settings view updates
WheelWizard/Views/Pages/Settings/SettingsEditing.cs, WheelWizard/Views/Pages/Settings/{OtherSettings,RecompSettings,VideoSettings,WhWzSettings}.axaml.cs, WheelWizard/Views/Layout.axaml.cs
Settings view handlers use SettingsEditing.Set, which displays a save error or invalid-path warning when a write fails. Location updates use Setting<string>; the success path no longer reloads Dolphin settings or displays the previous saved-path message.
Typed API tests
WheelWizard.Test/Features/Settings/*, WheelWizard.Test/Features/{Dolphin,Launching}/*, WheelWizard.Test/Features/MiiRepositoryServiceTests.cs, WheelWizard.Test/Views/HomeDolphinLaunchTests.cs
Tests use generic settings and typed manager arguments. Dolphin and Wheel Wizard decimal parsing tests now assert successful parsing and values.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Refactor

Suggested reviewers: patchzyy

Merge Risk: ⚪ Minimal · up to 6f1a5

No confirmed issue remains that should block merging after normal checks.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 6f1a5

The settings and path-change behavior warrant design review, but the inspected code retains validation and reloads the selected Dolphin profile after a successful path change. No introduced security issue was established. Failure and recovery behavior is not fully covered by the available evidence.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — A changed Dolphin executable or user directory can select a different local Dolphin configuration profile for subsequent reads and writes. The inspected entrypoint is a local settings edit, not an established remotely reachable entrypoint.

Trust Boundaries and Controls

  • observed — Typed access does not replace runtime validation: Setting validates updates, and Dolphin configuration values are parsed at the file boundary before they become setting values.

Resilience and Maintainability Implications

  • observed — Reload clears the Dolphin manager’s loaded flag before reading the newly resolved profile. File read errors in that loader are handled as empty input, leaving recovery after transient read failure dependent on a later reload.

Hardening Proposals

  • proposed — Exercise path edits through the view across failed persistence, transient profile-read failure, retry, and restart to verify that subsequent Dolphin writes use the intended profile.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.72% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 138 functions across 29 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main changes: type-safe settings values and centralized path reloads.
Description check ✅ Passed The description includes all required template sections, explains the purpose and changes, provides test instructions and results, identifies related stack context, and confirms that relevant tests we…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR

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.

❤️ Share

A rabbit checks each typed setting with care
A decimal hops through parsing to land
The settings now carry their types
A save error gets shown when writes fail
The tests plant fresh values in their rows
Then the rabbit bounds off, ears held high

Comment @coderabbitai help to get the list of available commands.

@DirkDoes
DirkDoes added this pull request to stack #503 September 28, 2026 21:03
@DirkDoes
DirkDoes marked this pull request as ready for review September 28, 2026 21:20
@DirkDoes
DirkDoes requested a review from patchzyy September 28, 2026 21:20

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant