fix(UI): improve theme management UI flow

[brucenguyen]

Addresses #1919.
There are significant number of whitespace-only changes, hiding whitespace is recommended for review.

The UI flow for theme management is a bit convoluted. You can modify the settings but clicking the global save button does not save it. The user should instead be saving the theme via the theme tab.

This PR admittedly does a bit more than add messages. As a summary:

• Adds info text on each tab that manages theme-only data
• Prevent creation of themes with duplicate names or display names
• Prevent saving to preset themes (themes shipped with CS)
• Moved things around in the theme tab to better separate concerns
• Code generation for preset themes

When I say "moved things" around, it includes removing the weird dropdown create flow that was used to create themes. It makes the implementation over-complicated and the UX also over-complicated. There's now a distinct "Save As New" button.

As for removing the ability to save to preset themes, this is for user protection. Whenever a CS update rolls out, these themes are reset anyway. By forcing the user to create their own theme, this doesn't happen.

Example of the new info text:

New theme selection tab (save button is not available for preset themes):

Future improvements (todo):

• Theme deletion via UI

Summary by CodeRabbit

• New Features

  • Redesigned Themes UI: visible "Selected Theme" block, refreshed "Refresh" action, and "Open Themes Folder" quick action
  • Theme creation popup with real-time validation and "Save As New Theme" flow
  • Theme-save guidance surfaced across Settings, Fonts, Styling and Colors tabs

• Bug Fixes / Reliability

  • Prevent saving over shipped presets; safer filename handling and sanitized theme names
  • Added case-insensitive comparison utility to improve selection matching

• UX

  • Streamlined selection/save flow, clearer success/error feedback and duplicate-name validation

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

Overhauls the Themes tab and save/create flows in the settings UI, adds filename sanitization and preset-awareness to ThemeManager, exposes shipped preset names at build time via a generated header, and surfaces theme-save guidance across related settings tabs.

Changes

Cohort / File(s)	Summary
Settings UI (themes & save guidance) src/Menu/SettingsTabRenderer.cpp	Adds anonymous helpers (IsPresetThemeSelected, RenderSaveInfoText), injects save guidance into Behavior/Fonts/Styling/Colors tabs, fully refactors Themes tab (removes inline create-new placeholder, adds visible "Selected Theme" block, replaces Refresh Themes with Refresh + Open Themes Folder, introduces Save and Save‑As‑New flows, sanitization & duplicate/display-name validation, creation popup with live validation, success/failure UI feedback). Replaces remaining direct iequals uses with Util::IEquals in relevant UI logic.
Theme management (sanitization & preset checks) src/Menu/ThemeManager.h, src/Menu/ThemeManager.cpp	Adds IsPresetTheme(const std::string&) const and SanitizeThemeFileName(std::string); LoadTheme compares against sanitized names; SaveTheme prevents overwriting presets, uses sanitized output filenames, logs failures, and updates in-memory state on successful saves.
Filename helpers src/Utils/FileSystem.h, src/Utils/FileSystem.cpp	Adds Util::FileHelpers::SanitizeFileName(std::string) to replace invalid filename chars and avoid reserved device names.
Case-insensitive util src/Utils/Format.h, src/Utils/Format.cpp	Adds Util::IEquals(std::string_view, std::string_view) for portable case-insensitive string comparisons; replaces previous local helpers.
Build integration for presets CMakeLists.txt, cmake/ThemePresets.h.in	CMake now scans Themes/*.json, builds a sorted list of preset names, generates ThemePresets.h via configure_file, and adds it to the build to expose ThemePresets::names at compile time.

Sequence Diagram(s)

sequenceDiagram
    participant UI as SettingsTabRenderer (UI)
    participant TM as ThemeManager
    participant FS as Filesystem
    participant LOG as Logger

    UI->>TM: Request SaveTheme(selectedName or newName, settings)
    TM->>TM: SanitizeThemeFileName(name)
    TM->>TM: IsPresetTheme(sanitizedName)?
    alt Preset selected
        TM->>LOG: Log "cannot overwrite preset"
        TM-->>UI: return failure
    else Not a preset
        TM->>FS: Write theme JSON (safeFileName)
        alt Write success
            TM-->>UI: return success + changed-settings list
        else Write failure
            TM->>LOG: Log save/write error
            TM-->>UI: return failure
        end
    end

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

• feat(ui): theme management fixes and enhancements #1700: Overlaps with SettingsTabRenderer theme UI changes (create/save/validation flows).
• feat(UI): separate theme settings, UI refactor, font support #1571: Related edits to ThemeManager load/save and UI integration for themes.
• feat(UI): require confirmation to clear shader cache #1757: Touches ThemeManager API/header additions similar to this PR.

Suggested reviewers

• alandtse
• davo0411
• doodlum

Poem

🐰

I nibble names and stitch them neat,
I tidy files for each theme's sheet,
I hop through tabs and sing with cheer,
Save your colors — the rabbit's near,
Hooray for themes both far and near!

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 13.04% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'fix(UI): improve theme management UI flow' directly and clearly summarizes the main change—enhancing the theme management UI with improved flow and user guidance.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

• 📝 Generate docstrings (stacked PR)
• 📝 Generate docstrings (commit on current branch)

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

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

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[github-actions]

No actionable suggestions for changed features.

[coderabbitai]

Actionable comments posted: 3

🧹 Nitpick comments (3)

src/Menu/SettingsTabRenderer.cpp (3)

566-587: showUpdateFeedback has no dismissal mechanism — persists indefinitely

Once set to true, the diff/success block stays visible forever (across theme switches, tab changes, and session restarts until the next save attempt). Consider adding a "Dismiss" button or an auto-hide timer using ImGui::GetTime() to avoid stale feedback misleading the user.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Menu/SettingsTabRenderer.cpp` around lines 566 - 587, The feedback block
controlled by showUpdateFeedback (used with updateSuccess and changedSettings in
the SettingsTabRenderer rendering code) lacks a dismissal mechanism; update the
render logic to allow the user to remove stale messages by adding either a
"Dismiss" button (that sets showUpdateFeedback = false) next to the feedback
area or implement an auto-hide timeout using ImGui::GetTime() (store a float
like updateFeedbackTimestamp when setting showUpdateFeedback and compare current
ImGui::GetTime() to hide after X seconds). Ensure the change is applied in the
same rendering function that draws the block so that both manual dismissal
and/or the timeout clear showUpdateFeedback and stop displaying
changedSettings/updateSuccess.

---

373-375: Add issue reference to PR description

Per project conventions, since this PR addresses a reported issue, the description should include Addresses #1919 (or `Closes `#1919 if it fully resolves the issue) so the issue is linked and closed automatically on merge. As per coding guidelines, suggest adding Addresses #1919 or `Related to `#1919 to the PR description.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Menu/SettingsTabRenderer.cpp` around lines 373 - 375, The PR is missing
the issue reference in its description per project conventions; update the PR
description to include an issue link like "Addresses `#1919`" (or "Closes `#1919`"
if it fully resolves it) so the issue is automatically linked/closed on
merge—this change is administrative and unrelated to code in
SettingsTabRenderer::RenderThemesTab or BeginTabItemWithFont, just add the
appropriate "Addresses `#1919`" or "Related to `#1919`" line to the PR description.

---

696-699: Cancel (and success path) should call ImGui::CloseCurrentPopup()

Setting showCreateThemePopup = false relies on BeginPopupModal's p_open check on the next frame to close the popup. The popup remains visible for an extra frame (the frame in which Cancel is clicked). Adding ImGui::CloseCurrentPopup() closes it immediately and is the documented ImGui idiom for closing a modal from within its own render block.

♻️ Proposed fix

         if (ImGui::Button("Cancel")) {
             showCreateThemePopup = false;
+            ImGui::CloseCurrentPopup();
         }

Apply the same fix to the success path (line 685):

             showCreateThemePopup = false;
+            ImGui::CloseCurrentPopup();
             logger::info("Theme creation complete. ...");

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Menu/SettingsTabRenderer.cpp` around lines 696 - 699, The popup is being
closed by setting showCreateThemePopup = false which delays actual closure one
frame; update both the Cancel button handler and the success/creation path (the
branch that currently sets showCreateThemePopup = false after creating the
theme) to call ImGui::CloseCurrentPopup() in addition to clearing
showCreateThemePopup (i.e., call ImGui::CloseCurrentPopup() before or alongside
setting showCreateThemePopup = false) so the modal closes immediately; look for
the ImGui::Button("Cancel") handler and the theme-success branch in
SettingsTabRenderer:: (where the new theme is accepted) and add the
ImGui::CloseCurrentPopup() call there.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/Menu/SettingsTabRenderer.cpp`:
- Around line 482-488: The code reads themes[currentItem] without checking for
an empty themes vector; change SettingsTabRenderer.cpp to guard against empty
results from DiscoverThemes() by checking themes.empty() before indexing and by
clamping or resetting currentItem when themes is non-empty (e.g., reset
currentItem to 0 or min(currentItem, themes.size()-1)), and render a fallback
message (e.g., "No themes found") instead of accessing selectedTheme, ensuring
any uses of selectedTheme and themeSettings.StatusPalette.InfoColor only occur
when themes is non-empty.

In `@src/Menu/ThemeManager.cpp`:
- Around line 675-685: IsPresetTheme currently uses a hard-coded array of preset
names which can drift from the shipped .json files; update it to derive the
protected/preset set at discovery time instead: when themes are discovered
populate a std::unordered_set<std::string> (or mark a bool on ThemeInfo) for
themes that live in the base install path (not the overwrite/MO2 path) and have
IsPresetTheme consult that set/ThemeInfo flag; alternatively, if you must keep a
compile-time check add a clear comment/static_assert linking this list to the
authoritative source, but preferred fix is to populate ThemeManager::presetNames
(or ThemeInfo::isBuiltin) during theme discovery and replace the hard-coded
array in IsPresetTheme with a lookup against that runtime set/flag.
- Around line 555-558: SanitizeThemeFileName currently only strips illegal path
chars but doesn't guard Windows reserved device names; update
ThemeManager::SanitizeThemeFileName to, after the replace_if step, do a
case-insensitive comparison of the sanitized base name against the reserved set
{NUL, CON, PRN, AUX, COM1..COM9, LPT1..LPT9} and if it matches exactly, modify
the name (e.g., append an underscore or prefix "theme_") so it no longer equals
a device name, then return that safe name.

---

Nitpick comments:
In `@src/Menu/SettingsTabRenderer.cpp`:
- Around line 566-587: The feedback block controlled by showUpdateFeedback (used
with updateSuccess and changedSettings in the SettingsTabRenderer rendering
code) lacks a dismissal mechanism; update the render logic to allow the user to
remove stale messages by adding either a "Dismiss" button (that sets
showUpdateFeedback = false) next to the feedback area or implement an auto-hide
timeout using ImGui::GetTime() (store a float like updateFeedbackTimestamp when
setting showUpdateFeedback and compare current ImGui::GetTime() to hide after X
seconds). Ensure the change is applied in the same rendering function that draws
the block so that both manual dismissal and/or the timeout clear
showUpdateFeedback and stop displaying changedSettings/updateSuccess.
- Around line 373-375: The PR is missing the issue reference in its description
per project conventions; update the PR description to include an issue link like
"Addresses `#1919`" (or "Closes `#1919`" if it fully resolves it) so the issue is
automatically linked/closed on merge—this change is administrative and unrelated
to code in SettingsTabRenderer::RenderThemesTab or BeginTabItemWithFont, just
add the appropriate "Addresses `#1919`" or "Related to `#1919`" line to the PR
description.
- Around line 696-699: The popup is being closed by setting showCreateThemePopup
= false which delays actual closure one frame; update both the Cancel button
handler and the success/creation path (the branch that currently sets
showCreateThemePopup = false after creating the theme) to call
ImGui::CloseCurrentPopup() in addition to clearing showCreateThemePopup (i.e.,
call ImGui::CloseCurrentPopup() before or alongside setting showCreateThemePopup
= false) so the modal closes immediately; look for the ImGui::Button("Cancel")
handler and the theme-success branch in SettingsTabRenderer:: (where the new
theme is accepted) and add the ImGui::CloseCurrentPopup() call there.

[Dlizzio]

Background blur toggle and font scaling shouldn't be tied to themes anyways. That might be the better fix.

Changes listed look good still by its own.

[github-actions]

✅ A pre-release build is available for this PR:
Download

[coderabbitai]

Actionable comments posted: 3

🧹 Nitpick comments (1)

cmake/ThemePresets.h.in (1)

7-9: Prefer inline constexpr over static const for the namespace-scope array

static const at namespace scope gives the array internal linkage, meaning each translation unit that includes this header gets its own copy. const on a global/namespace-scope variable implies static (internal linkage), and memory for that static is only going to be allocated if an address or reference is taken — with a different address in each translation unit. If your compiler is C++17 capable, prefer defining inline constexpr global variables in a header file.

♻️ Proposed fix

-	static const std::array<const char*, `@THEME_PRESET_COUNT`@> names = {
+	inline constexpr std::array<const char*, `@THEME_PRESET_COUNT`@> names = {

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cmake/ThemePresets.h.in` around lines 7 - 9, The namespace-scope array
currently defined as "static const std::array<const char*, `@THEME_PRESET_COUNT`@>
names" should be changed to use inline constexpr to provide a single definition
across TUs; update the declaration for the symbol names to "inline constexpr
std::array<const char*, `@THEME_PRESET_COUNT`@> names = { `@THEME_PRESET_NAMES`@ }"
(remove static/const-only pattern) so it has external linkage under C++17 and
above.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@cmake/ThemePresets.h.in`:
- Line 1: The PR currently references issue `#1919` inline but doesn't use a
GitHub keyword; update the PR description body to include a linking keyword such
as "Addresses `#1919`" (or "Related to `#1919`") so GitHub will automatically
associate the issue with the PR—locate the PR description for the branch that
modifies cmake/ThemePresets.h.in (the change showing "#pragma once") and add the
chosen keyword line to the PR body.

In `@CMakeLists.txt`:
- Around line 238-241: The loop that builds THEME_PRESET_NAMES appends raw
preset stems (THEME_PRESET_NAMES_LIST -> THEME_PRESET_NAMES) into C++ string
literals, which will break if a name contains " or \; fix by escaping
backslashes and double quotes before appending: inside the foreach over
THEME_PRESET_NAMES_LIST, create a local variable (e.g., esc_p) and run
string(REPLACE "\\" "\\\\" esc_p "${p}") then string(REPLACE "\"" "\\\"" esc_p
"${esc_p}"), then append esc_p to THEME_PRESET_NAMES as before; keep the
trailing string(REGEX REPLACE ",\n$" "" THEME_PRESET_NAMES ...) unchanged.

In `@src/Menu/ThemeManager.cpp`:
- Around line 676-683: IsPresetTheme currently does a case-sensitive compare and
should perform a case-insensitive check to match UI behavior; update
ThemeManager::IsPresetTheme to compare themeName against each entry in
ThemePresets::names using the existing case-insensitive comparator (iequals) or
an equivalent case-folding routine (e.g., std::tolower-based compare) so that
"default", "Default", and "DEFAULT" all detect as a preset and prevent
overwriting shipped files.

---

Duplicate comments:
In `@src/Menu/SettingsTabRenderer.cpp`:
- Around line 489-495: The code reads themes[currentItem] without checking
whether themes is empty, causing a crash when DiscoverThemes() found no themes;
update the rendering logic in SettingsTabRenderer (use the variables
currentItem, themes, selectedTheme, and themeSettings.StatusPalette.InfoColor)
to first check if themes.empty() and handle that case (e.g., display a "No
themes found" message or skip accessing themes/currentItem), only index
themes[currentItem] when non-empty, and ensure currentItem is clamped/reset
appropriately after discovery to avoid out-of-bounds access.

In `@src/Menu/ThemeManager.cpp`:
- Around line 556-560: SanitizeThemeFileName currently only replaces illegal
path chars but must also guard against Windows reserved device names; update
ThemeManager::SanitizeThemeFileName to, after normalizing characters, check the
base filename (case-insensitive, without extension) against the reserved set
{CON, PRN, NUL, AUX, COM1..COM9, LPT1..LPT9} and if it matches, alter the name
(for example prefix or suffix with an underscore or append "_file") so it no
longer equals a reserved device name before returning; ensure the check is
applied regardless of letter case and preserves file extension handling expected
by SaveTheme/RefreshThemes.

---

Nitpick comments:
In `@cmake/ThemePresets.h.in`:
- Around line 7-9: The namespace-scope array currently defined as "static const
std::array<const char*, `@THEME_PRESET_COUNT`@> names" should be changed to use
inline constexpr to provide a single definition across TUs; update the
declaration for the symbol names to "inline constexpr std::array<const char*,
`@THEME_PRESET_COUNT`@> names = { `@THEME_PRESET_NAMES`@ }" (remove
static/const-only pattern) so it has external linkage under C++17 and above.

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (3)

src/Menu/SettingsTabRenderer.cpp (3)

1-1: Consider adding a GitHub issue-closing keyword to the PR description

The PR mentions issue #1919 but doesn't use a linking keyword. Adding Closes #1919 or `Addresses `#1919 to the PR description will auto-link and optionally auto-close the issue on merge.

As per coding guidelines, issue references should use appropriate GitHub keywords (Closes #123, `Addresses `#123, etc.).

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Menu/SettingsTabRenderer.cpp` at line 1, Update the PR description to
include a GitHub issue-closing keyword for the referenced issue (e.g., add
"Closes `#1919`" or "Addresses `#1919`" in the PR body) so GitHub will
auto-link/auto-close the issue when merged; ensure the PR description that
mentions SettingsTabRenderer (file SettingsTabRenderer.h /
SettingsTabRenderer.cpp) contains the exact keyword phrase and the issue number.

---

450-450: Redundant self-assignment after LoadThemePreset

themeSettings is auto& themeSettings = globals::menu->GetSettings().Theme — a reference to the actual settings member. LoadThemePreset updates that member in-place, so themeSettings already reflects the new state. The assignment on this line reassigns the struct to itself.

♻️ Proposed fix

-                themeSettings = globals::menu->GetSettings().Theme;

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Menu/SettingsTabRenderer.cpp` at line 450, The line reassigning
themeSettings is redundant because themeSettings is declared as auto&
themeSettings = globals::menu->GetSettings().Theme and LoadThemePreset mutates
that object in-place; remove the assignment "themeSettings =
globals::menu->GetSettings().Theme" (or stop reassigning the referenced struct)
so the code relies on the existing reference updated by LoadThemePreset (locate
where themeSettings and LoadThemePreset are used in SettingsTabRenderer.cpp to
remove the redundant self-assignment).

---

386-396: showUpdateFeedback persists across theme switches, showing stale save results

showUpdateFeedback and changedSettings are static and never reset when currentThemePreset changes. After saving theme A, switching to theme B via the dropdown keeps the old "Theme updated successfully" message visible for theme B.

♻️ Proposed fix — reset on selection change

+    static std::string lastFeedbackTheme;

     // Theme preset dropdown
     if (ComboWithFont("##ThemePreset", &currentItem, items.data(), static_cast<int>(items.size()), Menu::FontRole::Body)) {
         std::string selectedTheme = themes[currentItem].name;
         if (globals::menu->LoadThemePreset(selectedTheme)) {
             themeSettings = globals::menu->GetSettings().Theme;
+            showUpdateFeedback = false;
+            changedSettings.clear();
+            lastFeedbackTheme = selectedTheme;
         }
     }

Also applies to: 576-596

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Menu/SettingsTabRenderer.cpp` around lines 386 - 396, showUpdateFeedback,
changedSettings and updateSuccess are declared static and never cleared when the
theme preset selection changes, causing stale feedback to persist across
presets; modify the theme preset selection handler (the code that updates
currentThemePreset from the dropdown) to reset showUpdateFeedback = false, clear
changedSettings (e.g., changedSettings.clear()) and set updateSuccess = false
whenever currentThemePreset is changed, and apply the same reset logic in the
other selection-change location noted (the block around the second occurrence)
so feedback is cleared on every preset switch.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/Menu/SettingsTabRenderer.cpp`:
- Around line 608-621: The code validates newThemeName before sanitization which
lets names that sanitize to empty pass; after generating safeNewThemeName via
themeManager->SanitizeThemeFileName, re-evaluate emptiness and duplicates using
the sanitized value: set isThemeNameEmpty = strlen(safeNewThemeName) == 0
(instead of checking raw newThemeName), and compare existing theme names against
safeNewThemeName (iequals(t.name, safeNewThemeName)); keep display-name
duplicate logic as-is but ensure isThemeNameError uses the sanitized check so
SaveTheme is blocked when the sanitized filename is empty or duplicates an
existing theme.
- Line 509: currentThemeInfo->themeData is const so using json::operator[] on a
potentially missing "Theme" key is UB; update the code around currentThemeInfo
(e.g., where json savedThemeJson = currentThemeInfo->themeData["Theme"]; is
used) to first check for the key via
currentThemeInfo->themeData.contains("Theme") and then use
currentThemeInfo->themeData.at("Theme") (or
currentThemeInfo->themeData.value("Theme", defaultValue) if a fallback is
acceptable), and handle the missing-key case (log/report error or skip loading)
instead of indexing unconditionally.

---

Duplicate comments:
In `@src/Menu/SettingsTabRenderer.cpp`:
- Around line 480-497: No change needed: the current code correctly displays the
info text and guards the selected theme display against out-of-bounds access;
keep the ImGui calls as-is (the info message around RenderSaveInfoText, the
bounds check using currentItem and themes.size(), and the Selected Theme UI that
uses selectedTheme.displayName and selectedTheme.description).

---

Nitpick comments:
In `@src/Menu/SettingsTabRenderer.cpp`:
- Line 1: Update the PR description to include a GitHub issue-closing keyword
for the referenced issue (e.g., add "Closes `#1919`" or "Addresses `#1919`" in the
PR body) so GitHub will auto-link/auto-close the issue when merged; ensure the
PR description that mentions SettingsTabRenderer (file SettingsTabRenderer.h /
SettingsTabRenderer.cpp) contains the exact keyword phrase and the issue number.
- Line 450: The line reassigning themeSettings is redundant because
themeSettings is declared as auto& themeSettings =
globals::menu->GetSettings().Theme and LoadThemePreset mutates that object
in-place; remove the assignment "themeSettings =
globals::menu->GetSettings().Theme" (or stop reassigning the referenced struct)
so the code relies on the existing reference updated by LoadThemePreset (locate
where themeSettings and LoadThemePreset are used in SettingsTabRenderer.cpp to
remove the redundant self-assignment).
- Around line 386-396: showUpdateFeedback, changedSettings and updateSuccess are
declared static and never cleared when the theme preset selection changes,
causing stale feedback to persist across presets; modify the theme preset
selection handler (the code that updates currentThemePreset from the dropdown)
to reset showUpdateFeedback = false, clear changedSettings (e.g.,
changedSettings.clear()) and set updateSuccess = false whenever
currentThemePreset is changed, and apply the same reset logic in the other
selection-change location noted (the block around the second occurrence) so
feedback is cleared on every preset switch.

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

src/Menu/SettingsTabRenderer.cpp (1)

436-451: isPreset is stale for one frame after a combo selection

isPreset is computed at line 436 from SelectedThemePreset before the combo at line 446 can change it. When LoadThemePreset updates SelectedThemePreset inside the combo block, isPreset and currentThemePreset remain the pre-change values for the rest of the current frame. This means the Save button can appear (or be hidden) incorrectly for one frame after a theme switch.

Moving the isPreset computation — and refreshing currentThemePreset — to after the combo block eliminates the stale-state window:

♻️ Proposed refactor

-        const bool isPreset = IsPresetThemeSelected();
-
         for (size_t i = 0; i < themes.size(); ++i) {
             if (themes[i].name == currentThemePreset) {
                 currentItem = static_cast<int>(i);
                 break;
             }
         }

         // Theme preset dropdown
         if (ComboWithFont("##ThemePreset", &currentItem, items.data(), static_cast<int>(items.size()), Menu::FontRole::Body)) {
-            std::string selectedTheme = themes[currentItem].name;
-            if (globals::menu->LoadThemePreset(selectedTheme)) {
-                // Theme loaded successfully, update UI
+            currentThemePreset = themes[currentItem].name;
+            if (globals::menu->LoadThemePreset(currentThemePreset)) {
                 themeSettings = globals::menu->GetSettings().Theme;
             }
         }
+
+        const bool isPreset = !currentThemePreset.empty() &&
+                              ThemeManager::GetSingleton()->IsPresetTheme(currentThemePreset);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Menu/SettingsTabRenderer.cpp` around lines 436 - 451, The bool isPreset
returned by IsPresetThemeSelected() and the currentThemePreset variable are
computed before the ComboWithFont selection and thus become stale for one frame
when LoadThemePreset(selectedTheme) inside the combo changes the selected
preset; move the IsPresetThemeSelected() call and any refresh of
currentThemePreset to after the ComboWithFont block (and after you call
globals::menu->LoadThemePreset and update themeSettings via
globals::menu->GetSettings().Theme) so the saved/preset UI (e.g., Save button
visibility) uses the post-selection state; specifically update the logic around
IsPresetThemeSelected(), currentThemePreset, ComboWithFont, LoadThemePreset, and
themeSettings/GetSettings().Theme to recompute isPreset and currentThemePreset
after handling the combo result.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/Menu/SettingsTabRenderer.cpp`:
- Around line 608-609: The code calls strlen on safeNewThemeName returned by
ThemeManager::SanitizeThemeFileName (a std::string), causing a compile error;
change the emptiness check to use std::string::empty() (e.g., set
isThemeNameEmpty = safeNewThemeName.empty()) so SanitizeThemeFileName,
safeNewThemeName and isThemeNameEmpty are used correctly without C-style strlen.

---

Duplicate comments:
In `@src/Menu/SettingsTabRenderer.cpp`:
- Line 509: The code uses currentThemeInfo->themeData["Theme"] which can invoke
undefined behavior if the "Theme" key is missing; change the access to a safe
form: first check currentThemeInfo->themeData.contains("Theme") (or use
value("Theme", json{})) and then assign savedThemeJson from
currentThemeInfo->themeData.at("Theme") (or from value) so you don’t call the
const operator[] on a possibly-missing key; update the assignment to use
contains/at or value and keep the target variable savedThemeJson.

---

Nitpick comments:
In `@src/Menu/SettingsTabRenderer.cpp`:
- Around line 436-451: The bool isPreset returned by IsPresetThemeSelected() and
the currentThemePreset variable are computed before the ComboWithFont selection
and thus become stale for one frame when LoadThemePreset(selectedTheme) inside
the combo changes the selected preset; move the IsPresetThemeSelected() call and
any refresh of currentThemePreset to after the ComboWithFont block (and after
you call globals::menu->LoadThemePreset and update themeSettings via
globals::menu->GetSettings().Theme) so the saved/preset UI (e.g., Save button
visibility) uses the post-selection state; specifically update the logic around
IsPresetThemeSelected(), currentThemePreset, ComboWithFont, LoadThemePreset, and
themeSettings/GetSettings().Theme to recompute isPreset and currentThemePreset
after handling the combo result.

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

src/Menu/SettingsTabRenderer.cpp (2)

575-596: ⚠️ Potential issue | 🟡 Minor

showUpdateFeedback is never cleared on theme selection change — stale feedback persists

showUpdateFeedback is a static bool that is set to true on any Save attempt but never reset when the user selects a different theme from the dropdown or switches away from and back to the Themes tab. A previous "Failed to update theme" message will remain visible while the user is viewing a different (possibly healthy) theme, and vice versa.

🐛 Proposed fix — reset feedback when selection changes

     for (size_t i = 0; i < themes.size(); ++i) {
         if (themes[i].name == currentThemePreset) {
+            if (currentItem != static_cast<int>(i)) {
+                showUpdateFeedback = false;
+                changedSettings.clear();
+            }
             currentItem = static_cast<int>(i);
             break;
         }
     }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Menu/SettingsTabRenderer.cpp` around lines 575 - 596, The feedback flag
showUpdateFeedback (a static bool used with updateSuccess and changedSettings)
is never cleared when the user changes the selected theme or leaves/returns to
the Themes tab, causing stale success/error messages to persist; fix this by
resetting showUpdateFeedback = false (and optionally clear changedSettings and
reset updateSuccess) whenever the theme selection is changed (the
dropdown/selection handler in SettingsTabRenderer.cpp) and when the Themes tab
is (re)entered, so feedback only reflects the latest save action.

---

686-692: ⚠️ Potential issue | 🟡 Minor

Update logging to show the actual sanitized filename being saved

SaveTheme sanitizes the theme name internally via SanitizeThemeFileName, so the file is correctly written with the sanitized name and LoadThemePreset will successfully find it. However, the logger::info at line 687 logs the raw unsanitized name, which is misleading about what was actually saved.

♻️ Proposed fix

-            logger::info("Attempting to save new theme: '{}' with display name: '{}'", newThemeName, displayName);
+            logger::info("Attempting to save new theme: '{}' with display name: '{}'", safeNewThemeName, displayName);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Menu/SettingsTabRenderer.cpp` around lines 686 - 692, The info log uses
the raw newThemeName but SaveTheme sanitizes it (via SanitizeThemeFileName) so
the saved file uses a different name; update the logging to report the sanitized
filename instead of the raw input: obtain the sanitized name (the same value
passed to LoadThemePreset, e.g., safeNewThemeName or by calling the same
SanitizeThemeFileName used by SaveTheme) and use that sanitized name in the
logger::info messages around SaveTheme, so the log accurately reflects the
actual file written and matches the argument passed to
globals::menu->LoadThemePreset.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@src/Menu/SettingsTabRenderer.cpp`:
- Around line 575-596: The feedback flag showUpdateFeedback (a static bool used
with updateSuccess and changedSettings) is never cleared when the user changes
the selected theme or leaves/returns to the Themes tab, causing stale
success/error messages to persist; fix this by resetting showUpdateFeedback =
false (and optionally clear changedSettings and reset updateSuccess) whenever
the theme selection is changed (the dropdown/selection handler in
SettingsTabRenderer.cpp) and when the Themes tab is (re)entered, so feedback
only reflects the latest save action.
- Around line 686-692: The info log uses the raw newThemeName but SaveTheme
sanitizes it (via SanitizeThemeFileName) so the saved file uses a different
name; update the logging to report the sanitized filename instead of the raw
input: obtain the sanitized name (the same value passed to LoadThemePreset,
e.g., safeNewThemeName or by calling the same SanitizeThemeFileName used by
SaveTheme) and use that sanitized name in the logger::info messages around
SaveTheme, so the log accurately reflects the actual file written and matches
the argument passed to globals::menu->LoadThemePreset.

---

Duplicate comments:
In `@src/Menu/SettingsTabRenderer.cpp`:
- Line 509: currentThemeInfo is a const pointer so using
currentThemeInfo->themeData["Theme"] will throw if the "Theme" key is missing;
change the access to a non-throwing lookup (e.g., use
currentThemeInfo->themeData.value("Theme", json{}) or check contains/find on
themeData before indexing) and handle the missing-key case (use an empty/default
json or bail gracefully) so the save path won't crash when third-party/malformed
theme files omit "Theme"; update the use site where json savedThemeJson is
created to use this safe lookup.

[brucenguyen]

@coderabbitai review

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

Actionable comments posted: 1

♻️ Duplicate comments (2)

src/Menu/ThemeManager.cpp (1)

670-677: IsPresetTheme still uses case-sensitive comparison — accepted design trade-off.

The UI layer enforces case-insensitive duplicate detection before calling SaveTheme, so a user can't reach this path with a case-variant name through the normal flow. Direct API callers (e.g., SaveTheme("default", ...)) could still bypass it on a case-insensitive NTFS filesystem.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Menu/ThemeManager.cpp` around lines 670 - 677, IsPresetTheme currently
compares themeName to ThemePresets::names using case-sensitive equality which
allows API callers to bypass the preset check on case-insensitive filesystems;
update ThemeManager::IsPresetTheme to perform a case-insensitive comparison
(e.g., normalize both themeName and each preset name to a common case or use a
locale-independent case-insensitive compare) against ThemePresets::names and
return true if any match, ensuring direct SaveTheme calls can't slip past the
preset check.

src/Menu/SettingsTabRenderer.cpp (1)

491-498: ⚠️ Potential issue | 🟡 Minor

const json::operator[] — prefer .at() for explicitness.

currentThemeInfo->themeData is const json (via a const pointer). operator[] on a const nlohmann::json object throws std::out_of_range for missing keys; in debug builds it also asserts. In practice the "Theme" key is guaranteed to exist because DiscoverThemes only populates themes with entries that pass ValidateThemeData, but using .at() makes that contract explicit and removes the ambiguity.

🛡️ Proposed fix

-                   json savedThemeJson = currentThemeInfo->themeData["Theme"];
+                   if (!currentThemeInfo->themeData.contains("Theme")) {
+                       logger::warn("Theme '{}' has no 'Theme' key — skipping save", currentThemePreset);
+                       updateSuccess = false;
+                       showUpdateFeedback = true;
+                       return;
+                   }
+                   json savedThemeJson = currentThemeInfo->themeData.at("Theme");

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Menu/SettingsTabRenderer.cpp` around lines 491 - 498, Replace the use of
operator[] on the const json themeData with .at() to make the required "Theme"
key access explicit: change the assignment to savedThemeJson to use
currentThemeInfo->themeData.at("Theme") (locate the code around
currentThemeInfo, themeData and savedThemeJson in SettingsTabRenderer.cpp) so
the intent and bounds checking are clear and avoid debug assertions from
operator[] on const json.

🧹 Nitpick comments (2)

src/Menu/ThemeManager.cpp (1)

556-569: LoadTheme sanitizes the lookup key but GetThemeInfo does not — consider aligning.

LoadTheme (line 567) derives safeFileName = SanitizeFileName(themeName) and searches by that sanitized name, while GetThemeInfo (line 654) searches by the raw themeName. If the same unsanitized string is passed to both (e.g., "My:Theme"), LoadTheme finds "My_Theme" while GetThemeInfo returns nullptr. Today all callers use clean filename stems, but the inconsistency is a maintenance footgun.

♻️ Proposed fix

 const ThemeManager::ThemeInfo* ThemeManager::GetThemeInfo(const std::string& themeName) const
 {
+    const std::string safeFileName = Util::FileHelpers::SanitizeFileName(themeName);
     auto it = std::find_if(themes.begin(), themes.end(),
-        [&themeName](const ThemeInfo& theme) { return theme.name == themeName; });
+        [&safeFileName](const ThemeInfo& theme) { return theme.name == safeFileName; });

     return (it != themes.end()) ? &(*it) : nullptr;
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Menu/ThemeManager.cpp` around lines 556 - 569, LoadTheme sanitizes the
lookup key (safeFileName = Util::FileHelpers::SanitizeFileName(themeName)) but
GetThemeInfo does not, causing inconsistent lookups; update GetThemeInfo to
apply the same SanitizeFileName to its incoming themeName before comparing to
ThemeInfo::name (or alternatively move sanitization into a shared helper used by
both functions) so both LoadTheme and GetThemeInfo use the same normalized key
when searching the themes vector.

src/Utils/FileSystem.cpp (1)

213-234: SanitizeFileName doesn't strip leading/trailing spaces or trailing dots.

Windows silently removes trailing spaces and trailing dots from filenames (and leading spaces are Windows-invalid). A name like "Theme " passed to std::ofstream would actually create "Theme.json", not "Theme .json", creating a silent path mismatch — SaveTheme returns true but RefreshThemes sees a different stem than expected.

♻️ Proposed fix

 std::string SanitizeFileName(std::string name)
 {
     // Replace invalid characters
     std::replace_if(name.begin(), name.end(), [](char c) {
         auto u = static_cast<unsigned char>(c);
         return c == '\\' || c == '/' || c == ':' || c == '*' || c == '?' || c == '"' || c == '<' || c == '>' || c == '|' || u < 32u || u == 127u; }, '_');
+
+    // Windows disallows filenames that end in a space or dot
+    while (!name.empty() && (name.back() == ' ' || name.back() == '.'))
+        name.pop_back();
+    while (!name.empty() && name.front() == ' ')
+        name.erase(name.begin());
+    if (name.empty())
+        name = "_";

     // Windows reserved device names
     ...

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Utils/FileSystem.cpp` around lines 213 - 234, SanitizeFileName currently
replaces invalid chars but doesn't trim leading/trailing spaces or remove
trailing dots (which Windows silently strips), so update SanitizeFileName to
first trim leading and trailing spaces and then remove any trailing '.'
characters from name (e.g., while (!name.empty() && name.back() == '.')
name.pop_back();), then continue with the existing invalid-char replacement and
reserved-name check (use the same Util::IEquals reserved-name logic after
trimming); also ensure an empty result is handled (e.g., replace with '_' or a
default name) before returning.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/Menu/SettingsTabRenderer.cpp`:
- Around line 441-448: The refresh path resets SelectedThemePreset to "Default"
but never applies it, so when themeManager->GetThemeInfo(currentThemePreset)
returns null you should call the codepath that actually loads/applies the preset
(e.g., invoke LoadThemePreset("Default") or the equivalent apply method on
themeManager or settings) after setting currentThemePreset and
globals::menu->GetSettings().SelectedThemePreset so the ImGui style is updated
immediately; update the block in SettingsTabRenderer (around the Refresh button
handling) to set the preset strings and then call the theme loading/apply
function (LoadThemePreset) to ensure the default theme is applied.

---

Duplicate comments:
In `@src/Menu/SettingsTabRenderer.cpp`:
- Around line 491-498: Replace the use of operator[] on the const json themeData
with .at() to make the required "Theme" key access explicit: change the
assignment to savedThemeJson to use currentThemeInfo->themeData.at("Theme")
(locate the code around currentThemeInfo, themeData and savedThemeJson in
SettingsTabRenderer.cpp) so the intent and bounds checking are clear and avoid
debug assertions from operator[] on const json.

In `@src/Menu/ThemeManager.cpp`:
- Around line 670-677: IsPresetTheme currently compares themeName to
ThemePresets::names using case-sensitive equality which allows API callers to
bypass the preset check on case-insensitive filesystems; update
ThemeManager::IsPresetTheme to perform a case-insensitive comparison (e.g.,
normalize both themeName and each preset name to a common case or use a
locale-independent case-insensitive compare) against ThemePresets::names and
return true if any match, ensuring direct SaveTheme calls can't slip past the
preset check.

---

Nitpick comments:
In `@src/Menu/ThemeManager.cpp`:
- Around line 556-569: LoadTheme sanitizes the lookup key (safeFileName =
Util::FileHelpers::SanitizeFileName(themeName)) but GetThemeInfo does not,
causing inconsistent lookups; update GetThemeInfo to apply the same
SanitizeFileName to its incoming themeName before comparing to ThemeInfo::name
(or alternatively move sanitization into a shared helper used by both functions)
so both LoadTheme and GetThemeInfo use the same normalized key when searching
the themes vector.

In `@src/Utils/FileSystem.cpp`:
- Around line 213-234: SanitizeFileName currently replaces invalid chars but
doesn't trim leading/trailing spaces or remove trailing dots (which Windows
silently strips), so update SanitizeFileName to first trim leading and trailing
spaces and then remove any trailing '.' characters from name (e.g., while
(!name.empty() && name.back() == '.') name.pop_back();), then continue with the
existing invalid-char replacement and reserved-name check (use the same
Util::IEquals reserved-name logic after trimming); also ensure an empty result
is handled (e.g., replace with '_' or a default name) before returning.

---

ℹ️ Review info

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a091a5b and 008a1d3.

📒 Files selected for processing (8)

• cmake/ThemePresets.h.in
• src/Menu/SettingsTabRenderer.cpp
• src/Menu/ThemeManager.cpp
• src/Menu/ThemeManager.h
• src/Utils/FileSystem.cpp
• src/Utils/FileSystem.h
• src/Utils/Format.cpp
• src/Utils/Format.h

🚧 Files skipped from review as they are similar to previous changes (1)

• cmake/ThemePresets.h.in

[alandtse]

Refactors mean there were no actual changes. This looks like a fix.