feat(ui): theme management fixes and enhancements#1700
Conversation
📝 WalkthroughWalkthroughRestructures theme persistence to store Theme data in theme presets (not main config), loads themes via SelectedThemePreset with legacy support and fallback, adds guarded preset load/restore, and enhances settings UI with update feedback and validation. Changes
Sequence DiagramsequenceDiagram
participant Menu
participant ThemeManager
participant State
participant Settings
participant SettingsTabRenderer
Menu->>Settings: Read SelectedThemePreset
alt preset name present
Menu->>ThemeManager: Ensure themes discovered
ThemeManager-->>Menu: Presets available
Menu->>Menu: Backup current Theme
Menu->>Menu: LoadThemePreset(presetName)
alt load success
Menu->>Menu: Apply preset (fonts/icons scheduled)
Menu->>Settings: Update SelectedThemePreset
else load failure
Menu->>Menu: Restore Theme from backup
Menu->>Menu: Attempt "Default" preset
end
else no preset (legacy)
Menu->>Settings: Read legacy Theme blob
Menu->>Menu: Normalize & apply legacy Theme
end
SettingsTabRenderer->>Menu: SaveTheme()/Update request
Menu->>SettingsTabRenderer: Compute diffs (ChangedSetting list)
SettingsTabRenderer->>SettingsTabRenderer: Show update feedback / validation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
🚥 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
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Menu.cpp (1)
474-491: Duplicate assignment of SelectedThemePreset
settings.SelectedThemePreset = themeName;is assigned twice - first at line 474 and again at line 488. The second assignment is redundant.Proposed fix
- // Schedule deferred icon reload to apply theme-specific icon overrides - pendingIconReload = true; - - // Apply background blur enabled state from theme - BackgroundBlur::SetEnabled(settings.Theme.BackgroundBlurEnabled); - - // Mark preset as selected on successful load - settings.SelectedThemePreset = themeName; - - logger::info("Applied theme preset: {}", themeName); + // Schedule deferred icon reload to apply theme-specific icon overrides + pendingIconReload = true; + + // Apply background blur enabled state from theme + BackgroundBlur::SetEnabled(settings.Theme.BackgroundBlurEnabled); + + logger::info("Applied theme preset: {}", themeName);
🤖 Fix all issues with AI agents
In @src/Menu.cpp:
- Line 504: Remove the stray comment artifact "// ... rest of the code remains
the same ..." from Menu.cpp (located around the Menu class / menu-related
functions area) so it doesn't appear in the compiled source; simply delete that
comment line and ensure no other code on that line was accidentally removed or
altered (verify symbols like Menu::render or adjacent functions remain
unchanged).
🧹 Nitpick comments (1)
src/Menu/SettingsTabRenderer.cpp (1)
533-554: Update feedback UI sectionThe feedback section appropriately:
- Shows success/failure messages with theme-appropriate colors
- Lists changed settings with before/after values when changes are detected
- Uses proper indentation for the change list
Minor observation:
showUpdateFeedbackis never reset tofalse, so the feedback will persist until the tab is closed and reopened (static variable resets). Consider adding a dismiss mechanism if this becomes a UX concern.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Menu.cppsrc/Menu/SettingsTabRenderer.cppsrc/State.cpp
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{cpp,cxx,cc,c,h,hpp,hxx,hlsl,hlsli,fx,fxh,py}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not include TODO/FIXME placeholders; provide complete, working solutions
Files:
src/State.cppsrc/Menu/SettingsTabRenderer.cppsrc/Menu.cpp
src/**/*.{cpp,cxx,cc,h,hpp,hxx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
src/**/*.{cpp,cxx,cc,h,hpp,hxx}: Ensure SE/AE/VR runtime compatibility; use runtime detection patterns (e.g., REL::RelocateMember())
Include robust error handling and resource management with graceful degradation in the plugin code
Files:
src/State.cppsrc/Menu/SettingsTabRenderer.cppsrc/Menu.cpp
**/*
⚙️ CodeRabbit configuration file
**/*: When reviewing PRs, please provide suggestions for:
Conventional Commit Titles (if not following https://www.conventionalcommits.org/ or
if the existing title does not describe the code changes):
Format: type(scope): description
Length: 50 characters limit for title, 72 for body
Style: lowercase description, no ending period
Examples:
- feat(vr): add cross-eye sampling
- fix(water): resolve flowmap bug
- docs: update shader documentation
Issue References (if PR fixes bugs or implements features):
Suggest adding appropriate GitHub keywords:Otherwise, use your standard review approach focusing on code quality.
Files:
src/State.cppsrc/Menu/SettingsTabRenderer.cppsrc/Menu.cpp
🧠 Learnings (1)
📓 Common learnings
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-07-05T05:20:45.823Z
Learning: In the skyrim-community-shaders repository, file deletion error handling improvements that replace existence checks and try-catch blocks with std::filesystem::remove error-code-based approaches are considered bug fixes rather than refactoring, as they address inadequate error handling and misleading log messages.
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-06-24T07:17:36.604Z
Learning: When reviewing PRs, always clarify the scope if there are multiple related features or dependencies. WeatherPicker was a separate PR that was already merged, while this PR focuses specifically on WetnessEffects climate preset system enhancements.
Learnt from: ThePagi
Repo: doodlum/skyrim-community-shaders PR: 1369
File: src/Features/SnowCover.cpp:277-293
Timestamp: 2025-08-05T18:22:40.578Z
Learning: In the skyrim-community-shaders SnowCover feature, the wstrtostr and strtowstr utility functions defined in src/Features/SnowCover.cpp are unused and should be removed rather than fixed, as confirmed by the original author ThePagi.
Learnt from: ThePagi
Repo: doodlum/skyrim-community-shaders PR: 1369
File: src/Features/SnowCover.cpp:515-515
Timestamp: 2025-10-02T14:20:33.454Z
Learning: In the Community Shaders codebase (skyrim-community-shaders repository), hardcoded shader resource slot numbers are used directly in code rather than being defined as named constants. This is the established convention and should not be flagged as an issue.
Learnt from: jiayev
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-08-03T18:37:19.690Z
Learning: ISReflectionsRayTracing.hlsl and ISWorldMap.hlsl in the skyrim-community-shaders repository are image-space post-processing shaders that perform color sampling and blending operations that need proper linear color space handling for the linear lighting system. ISReflectionsRayTracing handles screen-space reflections and should use conditional Color::IrradianceToLinear/Gamma conversions similar to ISCompositeLensFlareVolumetricLighting.hlsl. ISWorldMap performs 7x7 color accumulation that should be done in linear space similar to the pattern used in ISSAOComposite.hlsl.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build plugin and addons
🔇 Additional comments (10)
src/State.cpp (2)
848-874: LGTM - LoadTheme refactored to use preset-based loadingThe implementation correctly:
- Returns early when no preset is selected
- Ensures default themes exist and discovers themes if needed
- Falls back to "Default" preset on failure and updates
SelectedThemePresetaccordingly
876-881: SaveTheme() now a no-op for backward compatibilityThe function is intentionally converted to a no-op since theme data is now persisted via theme preset files rather than a separate SettingsTheme.json. The log message clearly documents this behavior change.
src/Menu/SettingsTabRenderer.cpp (4)
333-344: Static UI state variables for theme editingThe introduction of
showValidationError,showUpdateFeedback,updateSuccess, and theChangedSettingstruct for tracking diffs is appropriate for ImGui's immediate-mode paradigm where state must persist across frames.
476-500: Recursive diff walker for theme comparisonThe
diffWalkerlambda correctly:
- Handles object types by recursing through the union of keys
- Falls back to direct comparison for arrays and primitives
- Records path, old value, and new value for differences
One consideration: deeply nested themes could cause stack issues, but in practice theme JSON structures are shallow enough that this is not a concern.
566-584: Validation error UI for theme name inputThe validation flow correctly:
- Highlights the input field border in red when empty and error is shown
- Displays an inline error message below the input
- Resets the error state when a valid name is provided (line 606)
Good UX pattern for inline form validation.
603-630: Create theme button handler with validationThe logic properly:
- Validates theme name before proceeding
- Sets
showValidationError = truewhen name is empty- Clears
showValidationError = falseon valid input before proceeding- Logs success/failure appropriately
src/Menu.cpp (4)
216-241: Load() preserves Theme state and adds legacy migrationThe implementation:
- Preserves current Theme before loading to prevent override from config
- Restores Theme after loading settings
- Provides legacy support for old configs that have Theme data but no SelectedThemePreset
- Normalizes font roles and validates fonts when loading legacy data
This ensures smooth migration for users upgrading from the old theme storage model.
271-278: Save() strips Theme from config outputCorrectly removes the Theme object from the JSON output since theme data should only be stored in theme preset files. This enforces the new persistence model where
SelectedThemePresetis the only theme-related field in the main config.
342-462: Extensive field-by-field fallback for malformed theme dataThe manual field-by-field loading with individual try-catch blocks is verbose but provides robust handling for themes with incomplete or malformed data. This ensures older theme files or user-edited themes with missing fields don't crash the loader.
The approach is appropriate for user-facing data that may be hand-edited, though it creates a maintenance burden if new theme fields are added in the future.
534-546: Init() re-applies user preset after defaultsThe logic correctly ensures the user's selected preset is applied after default theme initialization:
- Checks if a preset is selected
- Discovers themes if not yet done
- Attempts to load the preset with appropriate logging
This prevents the default theme from overriding the user's saved preference.
|
✅ A pre-release build is available for this PR: |
soda3000
force-pushed
the
dev-12-01-2026
branch
from
666f02e to
c3f2f27
Compare
Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/Menu/SettingsTabRenderer.cpp (2)
335-344: Stale feedback state when switching themes or re-entering the tab.The
showUpdateFeedbackandchangedSettingsstatic variables are never reset when the user switches to a different theme or navigates away and back. This could display outdated feedback from a previous operation.Consider resetting the feedback state when the theme selection changes:
♻️ Suggested fix
Add a reset when theme selection changes (around line 412-418):
if (globals::menu->LoadThemePreset(selectedTheme)) { // Theme loaded successfully, update UI themeSettings = globals::menu->GetSettings().Theme; + // Reset update feedback when switching themes + showUpdateFeedback = false; + changedSettings.clear(); }
456-462: Reset validation error state when opening the create theme popup.When the popup is opened, the input fields are cleared (lines 460-462), but
showValidationErroris not reset. This means if a user previously triggered a validation error, cancelled the popup, and reopened it, they would see the error message immediately with an empty field.♻️ Suggested fix
if (isCreatingNewTheme) { // Show popup for new theme creation showCreateThemePopup = true; // Clear the input fields memset(newThemeName, 0, sizeof(newThemeName)); memset(newThemeDisplayName, 0, sizeof(newThemeDisplayName)); memset(newThemeDescription, 0, sizeof(newThemeDescription)); + // Reset validation state when opening popup + showValidationError = false; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Menu.cppsrc/Menu/SettingsTabRenderer.cpp
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{cpp,cxx,cc,c,h,hpp,hxx,hlsl,hlsli,fx,fxh,py}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not include TODO/FIXME placeholders; provide complete, working solutions
Files:
src/Menu/SettingsTabRenderer.cppsrc/Menu.cpp
src/**/*.{cpp,cxx,cc,h,hpp,hxx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
src/**/*.{cpp,cxx,cc,h,hpp,hxx}: Ensure SE/AE/VR runtime compatibility; use runtime detection patterns (e.g., REL::RelocateMember())
Include robust error handling and resource management with graceful degradation in the plugin code
Files:
src/Menu/SettingsTabRenderer.cppsrc/Menu.cpp
**/*
⚙️ CodeRabbit configuration file
**/*: When reviewing PRs, please provide suggestions for:
Conventional Commit Titles (if not following https://www.conventionalcommits.org/ or
if the existing title does not describe the code changes):
Format: type(scope): description
Length: 50 characters limit for title, 72 for body
Style: lowercase description, no ending period
Examples:
- feat(vr): add cross-eye sampling
- fix(water): resolve flowmap bug
- docs: update shader documentation
Issue References (if PR fixes bugs or implements features):
Suggest adding appropriate GitHub keywords:Otherwise, use your standard review approach focusing on code quality.
Files:
src/Menu/SettingsTabRenderer.cppsrc/Menu.cpp
🧠 Learnings (1)
📓 Common learnings
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-07-05T05:20:45.823Z
Learning: In the skyrim-community-shaders repository, file deletion error handling improvements that replace existence checks and try-catch blocks with std::filesystem::remove error-code-based approaches are considered bug fixes rather than refactoring, as they address inadequate error handling and misleading log messages.
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-06-24T07:17:36.604Z
Learning: When reviewing PRs, always clarify the scope if there are multiple related features or dependencies. WeatherPicker was a separate PR that was already merged, while this PR focuses specifically on WetnessEffects climate preset system enhancements.
Learnt from: ThePagi
Repo: doodlum/skyrim-community-shaders PR: 1369
File: src/Features/SnowCover.cpp:277-293
Timestamp: 2025-08-05T18:22:40.578Z
Learning: In the skyrim-community-shaders SnowCover feature, the wstrtostr and strtowstr utility functions defined in src/Features/SnowCover.cpp are unused and should be removed rather than fixed, as confirmed by the original author ThePagi.
Learnt from: ThePagi
Repo: doodlum/skyrim-community-shaders PR: 1369
File: src/Features/SnowCover.cpp:515-515
Timestamp: 2025-10-02T14:20:33.454Z
Learning: In the Community Shaders codebase (skyrim-community-shaders repository), hardcoded shader resource slot numbers are used directly in code rather than being defined as named constants. This is the established convention and should not be flagged as an issue.
Learnt from: jiayev
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-08-03T18:37:19.690Z
Learning: ISReflectionsRayTracing.hlsl and ISWorldMap.hlsl in the skyrim-community-shaders repository are image-space post-processing shaders that perform color sampling and blending operations that need proper linear color space handling for the linear lighting system. ISReflectionsRayTracing handles screen-space reflections and should use conditional Color::IrradianceToLinear/Gamma conversions similar to ISCompositeLensFlareVolumetricLighting.hlsl. ISWorldMap performs 7x7 color accumulation that should be done in linear space similar to the pattern used in ISSAOComposite.hlsl.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build plugin and addons
🔇 Additional comments (9)
src/Menu/SettingsTabRenderer.cpp (4)
5-5: LGTM!The
<set>include is appropriately added to support thestd::set<std::string>usage in thediffWalkerlambda for collecting unique keys from both JSON objects.
476-500: LGTM!The
diffWalkerrecursive lambda correctly handles JSON comparison by:
- Iterating over the union of keys from both old and new objects
- Recursively descending into nested objects
- Recording differences with path, old value, and new value
The implementation properly handles missing keys (using empty
json()as fallback) and null values.
566-584: LGTM!The validation error highlighting correctly:
- Pushes style color and var before the input field when validation fails
- Pops them immediately after the input field (properly balanced)
- Shows an inline error message for user feedback
The implementation follows ImGui best practices for conditional styling.
533-554: LGTM with minor UX consideration.The update feedback display correctly shows success/failure status and lists changed settings. The implementation is functional.
Consider limiting the displayed changes or adding a dismiss button if themes can have many settings changed at once, but this is optional for the current scope.
src/Menu.cpp (5)
216-265: LGTM!The
Load()method correctly implements:
- Theme state preservation during config loading (backup before, restore after)
- Legacy support for old configs that have
Themedata but noSelectedThemePreset- First-launch defaulting to "Default" theme
- Fallback handling when saved preset fails to load
The separation of theme data from main config aligns well with the PR's goal of storing themes in preset files.
268-278: LGTM!The
Save()method correctly enforces the new theme persistence model:
- Saves all settings including
SelectedThemePreset- Explicitly removes
Themeobject from the output JSON- Theme data is now exclusively stored in preset files, not the main config
This clean separation prevents stale theme data from accumulating in the main config.
337-498: Robust error handling for malformed theme data.The
LoadThemePreset()method now includes comprehensive error handling:
- Creates a backup before attempting to load
- Handles
json::out_of_rangespecifically for FullPalette array size mismatch (common when ImGui adds new color indices)- Falls back to default values for missing/malformed fields
- Restores backup on any unhandled exception
The nested try-catch structure correctly differentiates between recoverable partial data issues and fatal errors.
351-420: Defensive field-by-field loading for theme resilience.The individual try-catch blocks for each field allow themes with partial data to load successfully, using defaults for any missing or malformed fields. While the empty catch blocks are typically discouraged, this is an appropriate pattern here for graceful degradation when loading user-created theme files.
529-541: LGTM!The theme re-application during
Init()correctly:
- Ensures theme discovery is complete before attempting to load
- Re-applies the user's selected preset after the default theme baseline is established
- Logs outcomes for debugging without failing initialization if the preset is unavailable
This ensures consistent theme application regardless of initialization order.
2 participants
Summary by CodeRabbit
Bug Fixes
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.