feat(ui): add welcome window#1431
Conversation
WalkthroughAdds a Home page renderer with first-time setup UI and persistence, integrates "Home" as a built-in menu item, loads/releases a new Discord icon, introduces PathHelpers for fonts/icons/settings, and surfaces a first-time-setup overlay and input-swallow gating in overlay/menu logic. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Menu
participant FeatureList as FeatureListRenderer
participant Home as HomePageRenderer
participant Overlay as OverlayRenderer
participant FS as FileSystem
participant OS as Shell/Browser
User->>Menu: Open menu
Menu->>FeatureList: Render left column
FeatureList->>User: Show built-ins (Home, General, Advanced, Display)
User->>FeatureList: Select "Home"
FeatureList->>Home: RenderHomePage()
rect rgba(200,230,255,0.12)
note right of Home: First-time setup gating & persistence
Home->>FS: Read UserSettings.json
alt First run or not completed
Home->>User: Show first-time setup dialog (hotkey capture)
User->>Home: Set hotkey / Click Continue
Home-->>FS: Write FirstTimeSetupCompleted = true
else Completed
Home->>Home: Render page content
end
end
rect rgba(220,255,220,0.12)
note right of Home: External links / Discord action
opt Discord icon loaded
User->>Home: Click Discord link/button
Home->>OS: Open DISCORD_URL (ShellExecute)
end
end
rect rgba(255,230,200,0.12)
note right of Overlay: Global overlay behavior
Menu->>Overlay: Render overlay
alt Menu enabled OR First-time setup needed
Overlay->>Home: May invoke RenderFirstTimeSetupOverlay()
Home->>Overlay: ShouldShowFirstTimeSetup() true -> overlay shown, settings hidden
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Flag potential breaking changes that are not documented:
1. Identify changes to public APIs/exports, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints (including removed/renamed items and changes to types, required params, return values, defaults, or behavior).
2. Ignore purely internal/private changes (e.g., code not exported from package entry points or marked internal).
3. Verify documentation exists: a "Breaking Change" section in the PR description and updates to CHANGELOG.md.Please share your feedback with us on this Discord post. Pre-merge checks (2 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing Touches
🧪 Generate unit tests
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 |
Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/Menu/FeatureListRenderer.cpp (2)
182-204: Do not hard-code built-in names or assume there are exactly 4The current logic:
- Hard-codes names (“Home”, “General”, “Advanced”, “Display”)
- Stops after 4 items
- Accidentally demotes “Feature Issues” (also a BuiltInMenu) below the “Features” header, reducing visibility
Render the contiguous prefix of BuiltInMenu items instead. This keeps “Feature Issues” at the top when present, and future-proofs the list.
Apply this diff:
- // Find where built-in menus end (Home, General, Advanced, Display) - size_t builtInMenuCount = 0; - for (size_t i = 0; i < menuList.size(); i++) { - if (std::holds_alternative<BuiltInMenu>(menuList[i])) { - const BuiltInMenu& menu = std::get<BuiltInMenu>(menuList[i]); - if (menu.name == "Home" || menu.name == "General" || menu.name == "Advanced" || menu.name == "Display") { - builtInMenuCount++; - } - } - } - - // First render the built-in menus (Home, General, Advanced, Display) - size_t renderedBuiltIns = 0; - for (size_t i = 0; i < menuList.size() && renderedBuiltIns < 4; i++) { - if (std::holds_alternative<BuiltInMenu>(menuList[i])) { - const BuiltInMenu& menu = std::get<BuiltInMenu>(menuList[i]); - if (menu.name == "Home" || menu.name == "General" || menu.name == "Advanced" || menu.name == "Display") { - std::visit(ListMenuVisitor{ i, selectedMenu, categoryExpansionStates }, menuList[i]); - renderedBuiltIns++; - } - } - } + // Render the contiguous prefix of built-in menus (e.g., Feature Issues, Home, General, Advanced, Display) + size_t builtInsEnd = 0; + while (builtInsEnd < menuList.size() && std::holds_alternative<BuiltInMenu>(menuList[builtInsEnd])) { + ++builtInsEnd; + } + for (size_t i = 0; i < builtInsEnd; ++i) { + std::visit(ListMenuVisitor{ i, selectedMenu, categoryExpansionStates }, menuList[i]); + }
208-218: Skip already-rendered built-ins without name checksWith the contiguous built-in prefix rendered above, the rest loop can start at builtInsEnd and render everything else. This avoids string comparisons and keeps “Feature Issues” out of the “Features” section.
Apply this diff:
- // Then render the rest (features and categories, but skip already rendered built-ins) - for (size_t i = 0; i < menuList.size(); i++) { - if (std::holds_alternative<BuiltInMenu>(menuList[i])) { - const BuiltInMenu& menu = std::get<BuiltInMenu>(menuList[i]); - if (menu.name == "Home" || menu.name == "General" || menu.name == "Advanced" || menu.name == "Display") { - continue; // Skip, already rendered - } - } - std::visit(ListMenuVisitor{ i, selectedMenu, categoryExpansionStates }, menuList[i]); - } + // Then render everything after the built-in prefix (features, categories, etc.) + for (size_t i = builtInsEnd; i < menuList.size(); ++i) { + std::visit(ListMenuVisitor{ i, selectedMenu, categoryExpansionStates }, menuList[i]); + }
🧹 Nitpick comments (9)
src/Menu/HomePageRenderer.h (1)
3-4: Remove unused headers from the header filestd::functional and are not used in this header. Dropping them from the header reduces rebuilds and transitive compile time.
Apply this diff:
-#include <functional> -#include <string> +// (no additional includes needed here)src/Menu.h (1)
76-97: Consider centralizing icon releases to prevent driftReleasing icons one-by-one in Menu::~Menu() is easy to miss when adding new icons. A small helper on UIIcons (ReleaseAll) would reduce maintenance risk.
Example (outside this hunk):
struct UIIcons { // ... existing members ... void ReleaseAll() { saveSettings.Release(); loadSettings.Release(); clearCache.Release(); logo.Release(); discord.Release(); characters.Release(); grass.Release(); lighting.Release(); sky.Release(); landscape.Release(); water.Release(); debug.Release(); materials.Release(); postProcessing.Release(); } };Then call
uiIcons.ReleaseAll();in the destructor.src/Menu.cpp (1)
123-146: Add releases for new icons via a single helper to avoid omissionsThe explicit releases for
discordandpostProcessinglook correct, but this pattern is prone to future omissions as icons grow. If you addUIIcons::ReleaseAll()(see Menu.h comment), replace the list here with a single call.- uiIcons.saveSettings.Release(); - uiIcons.loadSettings.Release(); - uiIcons.clearCache.Release(); - uiIcons.logo.Release(); - uiIcons.discord.Release(); - uiIcons.characters.Release(); - uiIcons.grass.Release(); - uiIcons.lighting.Release(); - uiIcons.sky.Release(); - uiIcons.landscape.Release(); - uiIcons.water.Release(); - uiIcons.debug.Release(); - uiIcons.materials.Release(); - uiIcons.postProcessing.Release(); + uiIcons.ReleaseAll();src/Utils/UI.cpp (1)
317-318: Avoid hard-coded denominator in the “icons loaded” summaryHard-coding “14” will drift the moment you add/remove an icon. Compute or centralize the total target count.
Apply this diff:
- logger::info("InitializeMenuIcons: Loaded {}/14 icons successfully", iconsLoaded); + logger::info("InitializeMenuIcons: Loaded {}/{} icons successfully", iconsLoaded, 14);And, preferably, declare a single source of truth near the function:
// Outside the diffed lines, near the top of InitializeMenuIcons: static constexpr int kTotalIconTargets = 14; // ... and then logger::info("InitializeMenuIcons: Loaded {}/{} icons successfully", iconsLoaded, kTotalIconTargets);src/Menu/HomePageRenderer.cpp (5)
108-108: Security: Validate URL handling and consider using safer alternativesUsing
ShellExecuteAwith hardcoded URLs is generally safe, but consider these improvements:
- Use
ShellExecuteW(wide string version) for better Unicode support- Check the return value to handle failures gracefully
- Consider centralizing these URLs as constants for easier maintenance
- ShellExecuteA(NULL, "open", "https://discord.com/invite/nkrQybAsyy", NULL, NULL, SW_SHOWNORMAL); + HINSTANCE result = ShellExecuteW(NULL, L"open", L"https://discord.com/invite/nkrQybAsyy", NULL, NULL, SW_SHOWNORMAL); + if ((INT_PTR)result <= 32) { + logger::warn("Failed to open Discord link"); + }Also applies to: 123-123, 148-148, 153-153, 158-158
275-276: Redundant menu retrievalThe menu singleton is retrieved again at line 275, but it was already needed for the
selectedKeyinitialization. Consider moving it up to avoid redundant calls.- auto menu = Menu::GetSingleton(); - static uint32_t currentMenuKey = menu ? menu->GetSettings().ToggleKey : VK_END; + static uint32_t currentMenuKey = menu ? menu->GetSettings().ToggleKey : VK_END;
289-293: Missing bounds validation for combo box selectionThe combo box callback doesn't validate that
currentItemis within bounds before using it to index intokeyValues. While ImGui should handle this internally, explicit validation would be safer.if (ImGui::Combo("Menu Hotkey", ¤tItem, keyNames, IM_ARRAYSIZE(keyNames))) { + if (currentItem >= 0 && currentItem < IM_ARRAYSIZE(keyValues)) { selectedKey = keyValues[currentItem]; keyChanged = true; + } }
359-364: Inefficient file reading for JSONReading JSON using the stream extraction operator without checking for parse errors could lead to issues with malformed files.
std::ifstream file(setupPath); if (file.is_open()) { json setupData; - file >> setupData; + try { + setupData = json::parse(file); + } catch (const json::parse_error& e) { + logger::warn("Failed to parse setup file: {}", e.what()); + file.close(); + isFirstTimeSetupShown = true; + ImGui::OpenPopup("Welcome to Community Shaders!"); + return true; + } file.close();
304-306: Confusing logic for default settingsThe condition
if (!keyChanged)followed by settingselectedKey = VK_ENDis confusing. If the key hasn't changed,selectedKeyis alreadyVK_ENDby default.- if (!keyChanged) { - selectedKey = VK_END; // Ensure default - } + // Use default key if user didn't change it + if (!keyChanged) { + selectedKey = VK_END; + }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
package/Interface/CommunityShaders/Icons/Action Icons/discord.pngis excluded by!**/*.png
📒 Files selected for processing (6)
src/Menu.cpp(3 hunks)src/Menu.h(1 hunks)src/Menu/FeatureListRenderer.cpp(4 hunks)src/Menu/HomePageRenderer.cpp(1 hunks)src/Menu/HomePageRenderer.h(1 hunks)src/Utils/UI.cpp(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{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/HomePageRenderer.hsrc/Menu.cppsrc/Menu/HomePageRenderer.cppsrc/Menu.hsrc/Menu/FeatureListRenderer.cppsrc/Utils/UI.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/HomePageRenderer.hsrc/Menu.cppsrc/Menu/HomePageRenderer.cppsrc/Menu.hsrc/Menu/FeatureListRenderer.cppsrc/Utils/UI.cpp
⏰ 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 (8)
src/Menu/HomePageRenderer.h (2)
6-23: Static-only UI renderer API looks goodClear separation of concerns with private section renderers and first-time-setup helpers. Interface is minimal and discoverable.
21-23: Please verify the out-of-line definition ofisFirstTimeSetupShownI was unable to locate
src/Menu/HomePageRenderer.cpp(or any implementation) in the repository, so please confirm:
- That
static bool isFirstTimeSetupShown;insrc/Menu/HomePageRenderer.hhas exactly one corresponding definition in its .cpp, for example:bool HomePageRenderer::isFirstTimeSetupShown = false;- If you’d rather keep it inline, update the header to:
and remove any out-of-line definition to avoid ODR violations.inline static bool isFirstTimeSetupShown = false;Please check the
src/Menudirectory (or your chosen implementation path) to ensure the static member is defined or converted correctly.src/Menu.h (1)
84-86: Discord icon addition is consistent with the PR’s UI goalsThe new
uiIcons.discordslot is well placed under a social/external links comment and matches the loading/destruction changes elsewhere.src/Menu.cpp (1)
20-21: Including HomePageRenderer here is correctThis keeps the renderer dependency local to Menu and avoids leaking the new Home page details into unrelated units.
src/Menu/FeatureListRenderer.cpp (2)
13-14: Correct dependency addedIncluding HomePageRenderer enables the new built-in Home item without leaking it elsewhere.
70-75: Adding “Home” as a built-in is fineThe insertion order keeps core menus grouped, pending the left-column rendering logic fix below.
src/Menu/HomePageRenderer.cpp (2)
92-94: Good defensive programming for icon availability checkThe multi-condition check for Discord icon availability is thorough and prevents potential crashes from null pointer access or invalid texture dimensions.
391-414: Robust error handling in setup completionThe implementation includes proper exception handling, logging, and fallback behavior to prevent repeated popup displays even if file operations fail.
|
✅ A pre-release build is available for this PR: |
Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.
davo0411
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
src/Menu/OverlayRenderer.h (1)
52-52: Param is unused in implementation; mark it [[maybe_unused]] or remove it
RenderFirstTimeSetupOverlaytakeskeyIdToStringbut never uses it. Either drop the parameter or annotate it to avoid needless noise and casts.Apply this minimal change to declaration:
- static void RenderFirstTimeSetupOverlay(const std::function<const char*(uint32_t)>& keyIdToString); + static void RenderFirstTimeSetupOverlay([[maybe_unused]] const std::function<const char*(uint32_t)>& keyIdToString);src/Menu/OverlayRenderer.cpp (1)
201-206: Replace (void) cast with [[maybe_unused]]; optional: actually use the key printerThe
(void)keyIdToStringsuppression is a minor smell. Prefer annotating the param in both declaration and definition. Optionally, you could show a hint inside the dialog like “Press to open the menu later,” leveragingkeyIdToString.Apply to the definition:
-void OverlayRenderer::RenderFirstTimeSetupOverlay(const std::function<const char*(uint32_t)>& keyIdToString) +void OverlayRenderer::RenderFirstTimeSetupOverlay([[maybe_unused]] const std::function<const char*(uint32_t)>& keyIdToString) { - (void)keyIdToString; // Suppress unused parameter warning if (HomePageRenderer::ShouldShowFirstTimeSetup()) { HomePageRenderer::RenderFirstTimeSetupDialog(); } }src/Menu/HomePageRenderer.cpp (4)
78-86: Centering with TextWrapped is impreciseYou compute
introSizeusingCalcTextSize(introText)(unwrapped) but render withTextWrapped, which reflows text. The centering will be off for narrower windows.One option:
- ImVec2 introSize = ImGui::CalcTextSize(introText); - ImGui::SetCursorPosX((windowSize.x - introSize.x) * 0.5f); - ImGui::TextWrapped("%s", introText); + float wrap = ImGui::GetContentRegionAvail().x * 0.85f; + ImGui::SetCursorPosX((windowSize.x - wrap) * 0.5f); + ImGui::PushTextWrapPos(ImGui::GetCursorPosX() + wrap); + ImGui::TextUnformatted(introText); + ImGui::PopTextWrapPos();
248-255: Manual input capture flags are unusual; verify interaction with your input queueSetting
io.WantCaptureMouse/Keyboarddirectly can fight with your backend’s input routing if those flags are already derived from active windows. ImGui typically sets these for you based on hovered/active widgets; backends read them to decide whether to forward inputs to the app.If you observe conflicts, drop these lines and rely on the standard Dear ImGui behavior. You may still force the cursor via
ImGui::GetIO().MouseDrawCursor = trueas you already do in OverlayRenderer.
383-415: I/O path and JSON parse behavior are fine; consider caching result after first read
ShouldShowFirstTimeSetup()does a file existence check + parse every frame until completion. It’s inexpensive, but you can cache the result (e.g., a tri-state “unknown/true/false”) to skip repeated disk I/O on steady-state launches.If you’d like, I can provide a small
std::optional<bool>caching pattern here.
419-449: Make the settings write more robust with atomic file replaceIf the process crashes mid-write,
UserSettings.jsoncould end up truncated. An atomic write (write to temp, thenrename) avoids corruption.Example patch:
- // Write back to file - std::filesystem::create_directories(userSettingsPath.parent_path()); - std::ofstream outFile(userSettingsPath); - if (outFile.is_open()) { - outFile << settings.dump(2); - outFile.close(); - } + // Write back atomically + std::filesystem::create_directories(userSettingsPath.parent_path()); + const auto tmpPath = userSettingsPath; + const auto tmpName = tmpPath += ".tmp"; + { + std::ofstream out(tmpName, std::ios::binary | std::ios::trunc); + if (out.is_open()) { + const auto payload = settings.dump(2); + out.write(payload.data(), static_cast<std::streamsize>(payload.size())); + out.flush(); + } + } + std::error_code ec; + std::filesystem::rename(tmpName, userSettingsPath, ec); + if (ec) { + std::filesystem::remove(tmpName); + // fall through: session flag below prevents repeated popups + }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
src/Menu/HomePageRenderer.cpp(1 hunks)src/Menu/HomePageRenderer.h(1 hunks)src/Menu/OverlayRenderer.cpp(3 hunks)src/Menu/OverlayRenderer.h(1 hunks)src/Utils/UI.cpp(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/Menu/HomePageRenderer.h
- src/Utils/UI.cpp
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{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/OverlayRenderer.hsrc/Menu/HomePageRenderer.cppsrc/Menu/OverlayRenderer.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/OverlayRenderer.hsrc/Menu/HomePageRenderer.cppsrc/Menu/OverlayRenderer.cpp
⏰ 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 (2)
src/Menu/OverlayRenderer.cpp (2)
2-2: LGTM: required include addedIncluding
HomePageRenderer.hhere is appropriate for the new overlay coordination.
47-51: Guarding drawSettings while showing first-time setup is good UXOnly drawing settings when
menu.IsEnabledavoids conflicting UI while the setup dialog is present.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/Menu/HomePageRenderer.cpp (1)
323-364: Initialize hotkey selection from existing setting and guard against OOB indexingCurrent behavior starts with “END” selected (index 0) and writes it on Enter/Escape, potentially overwriting a user’s existing ToggleKey. Also, keyValues[selectedKey] is used without bounds checking. The dialog should reflect the active key on first open and index should be validated.
Apply this diff:
- // Hotkey selection - centered - static int selectedKey = 0; + // Hotkey selection - centered + static int selectedKey = -1; // lazy-init from current setting @@ const uint32_t keyValues[] = { VK_END, VK_INSERT, VK_HOME, VK_DELETE, VK_PRIOR, VK_NEXT, VK_F9, VK_F10, VK_F11, VK_F12 }; + + // Initialize from the current setting so we don't overwrite inadvertently + if (selectedKey == -1) { + uint32_t currentKey = menu ? menu->GetSettings().ToggleKey : VK_END; + selectedKey = 0; + for (int i = 0; i < IM_ARRAYSIZE(keyValues); ++i) { + if (keyValues[i] == currentKey) { + selectedKey = i; + break; + } + } + } @@ - if (ImGui::Button("Continue", ImVec2(buttonWidth, 30)) || shouldClose) { + if (ImGui::Button("Continue", ImVec2(buttonWidth, 30)) || shouldClose) { // Apply the selected hotkey if (menu) { - menu->GetSettings().ToggleKey = keyValues[selectedKey]; + const int idx = (selectedKey >= 0 && selectedKey < IM_ARRAYSIZE(keyValues)) ? selectedKey : 0; + menu->GetSettings().ToggleKey = keyValues[idx]; } MarkFirstTimeSetupComplete(); }Per the retrieved learnings, persistence of static local variables across sessions isn’t a concern for one-time dialogs, but please confirm whether you want Escape to confirm the selection. If Escape should cancel instead of apply, we can adjust the key handling and help text.
🧹 Nitpick comments (8)
src/Menu/HomePageRenderer.cpp (8)
45-75: Title font selection and scaling are safe, but consider scoping font scale more tightlyYou correctly guard against missing fonts and reset the window font scale. If you want to reduce side effects on subsequent layout within the same frame, consider a scoped helper that pushes/pops scale and font together; functionally fine as-is.
78-86: Centering wrapped text may look offYou center based on single-line width but render with TextWrapped, so multi-line blocks won’t be visually centered. If you care about visual centering, wrap the paragraph in a child of fixed width and center the child, not the first line.
Example approach outside this range:
const float paraWidth = ImGui::GetContentRegionAvail().x * 0.75f; ImGui::SetCursorPosX((windowSize.x - paraWidth) * 0.5f); ImGui::BeginChild("intro", ImVec2(paraWidth, 0), false); ImGui::TextWrapped("%s", introText); ImGui::EndChild();
89-120: ShellExecuteA result is ignored; log failures to aid support ticketsShellExecuteA returns an HINSTANCE; values <= 32 indicate failure. Logging helps diagnose user reports that links don’t open.
Apply this diff inline to both the Discord banner and fallback button handlers:
- if (ImGui::ImageButton("##DiscordButton", menu->uiIcons.discord.texture, iconSize)) { - ShellExecuteA(NULL, "open", "https://discord.com/invite/nkrQybAsyy", NULL, NULL, SW_SHOWNORMAL); - } + if (ImGui::ImageButton("##DiscordButton", menu->uiIcons.discord.texture, iconSize)) { + HINSTANCE h = ShellExecuteA(NULL, "open", "https://discord.com/invite/nkrQybAsyy", NULL, NULL, SW_SHOWNORMAL); + if ((INT_PTR)h <= 32) { + logger::warn("Failed to open Discord invite (code={}).", (INT_PTR)h); + } + }And similarly for the fallback button.
135-162: Deduplicate external link opening and log failuresYou have three nearly identical ShellExecuteA calls. Factor into a small helper to DRY and unify logging.
Minimal refactor idea (outside this range):
static void OpenURL(const char* url) { HINSTANCE h = ShellExecuteA(NULL, "open", url, NULL, NULL, SW_SHOWNORMAL); if ((INT_PTR)h <= 32) logger::warn("Failed to open URL: {} (code={}).", url, (INT_PTR)h); }Then call OpenURL("...") in the button handlers.
355-371: Confirm whether Escape should “apply and continue”Pressing Escape currently applies the selection and completes setup. That’s atypical UX for a first-time dialog. If Escape should cancel, separate confirm vs. cancel and adjust the help text accordingly.
Example adjustment outside this range:
const bool confirm = ImGui::IsKeyPressed(ImGuiKey_Enter) || ImGui::Button("Continue", ImVec2(buttonWidth, 30)); const bool cancel = ImGui::IsKeyPressed(ImGuiKey_Escape); if (confirm) { /* apply + complete */ } else if (cancel) { /* keep dialog open or set selectedKey from current to discard changes */ }Also change the hint to “Press Enter to continue” if Escape ceases to confirm.
383-415: File I/O on every frame; consider memoizing ShouldShowFirstTimeSetup()OverlayRenderer likely calls this each frame. Re-reading/parsing JSON every frame is unnecessary. Cache the result after the first evaluation until MarkFirstTimeSetupComplete() flips the session guard.
Example lightweight memoization:
// At top of function after the session guard: static int cached = -1; // -1 unknown, 0 false, 1 true if (cached != -1) return cached == 1; // Replace each `return true/false;` with `cached = 1/0; return cached == 1;` // and reset `cached = -1;` inside MarkFirstTimeSetupComplete().
398-399: Use the json alias consistentlyYou define
using json = nlohmann::json;but then instantiatenlohmann::jsondirectly. Minor style nit; pick one for consistency.Apply this diff:
- nlohmann::json settings; + json settings;…and similarly in MarkFirstTimeSetupComplete.
Also applies to: 422-423
21-22: Minor: remove unused includes/aliasesIt looks like , , and aren’t used in this TU, and the
using json = ...alias isn’t used if you keep the fully-qualified type. Optional cleanup.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/Menu/HomePageRenderer.cpp(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{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/HomePageRenderer.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/HomePageRenderer.cpp
🧠 Learnings (1)
📚 Learning: 2025-08-25T09:07:45.236Z
Learnt from: davo0411
PR: doodlum/skyrim-community-shaders#1431
File: src/Menu/HomePageRenderer.cpp:0-0
Timestamp: 2025-08-25T09:07:45.236Z
Learning: First-time setup dialogs in the Community Shaders project are typically one-time only dialogs that cannot be manually reopened by users, so static variable persistence across dialog sessions is not a concern for these types of dialogs.
Applied to files:
src/Menu/HomePageRenderer.cpp
🔇 Additional comments (4)
src/Menu/HomePageRenderer.cpp (4)
26-39: Solid page composition and sectioningThe high-level composition (child window + Welcome, Quick Links, FAQ) is clean and easy to extend. No issues here.
417-449: Confirm ToggleKey persistence to disk
I wasn’t able to locate any code that writes the updatedToggleKeyback out—MarkFirstTimeSetupCompleteonly persistsFirstTimeSetupCompleted. Without invoking the settings manager’s save/flush API after you setToggleKey, the new hotkey will be lost on restart.• Verify the save/flush method on
menu->GetSettings()(e.g.Save(),Flush(), or similar)
• Call it immediately after updatingmenu->GetSettings().ToggleKey
• Ensure theSettingsclass (whereToggleKeylives) is serialized in the same path asUserSettings.json
277-298: Possible division-by-zero when computing logo aspect ratioYou don’t validate logo texture dimensions; size.y could be 0. Guard like you already do for the Discord icon.
Apply this diff:
- if (menu && menu->uiIcons.logo.texture) { + if (menu && menu->uiIcons.logo.texture && + menu->uiIcons.logo.size.x > 0 && menu->uiIcons.logo.size.y > 0) {⛔ Skipped due to learnings
Learnt from: davo0411 PR: doodlum/skyrim-community-shaders#1107 File: src/Menu.cpp:310-315 Timestamp: 2025-05-31T02:51:24.195Z Learning: In the Community Shaders codebase, when validating UI icons, checking for texture presence (non-null pointer) is sufficient. Avoid adding dimension validation checks as icon sizes should vary based on user needs (4K+ resolution users may want higher resolution icons). Don't artificially limit icon dimensions.
250-255: Unable to verify ImGui IO assignments in HomePageRenderer.cppI couldn’t open or read
src/Menu/HomePageRenderer.cppto confirm whether the code is still writing toio.WantCaptureMouse/io.WantCaptureKeyboardor howio.MouseDrawCursoris being scoped and restored. Please manually verify that:
io.WantCaptureMouseandio.WantCaptureKeyboardare not assigned anywhere (especially around lines 250–255, 269–274, and 372–374).io.MouseDrawCursoris only set when the window is active and is restored to its previous value afterImGui::End().
alandtse
left a comment
alandtse
left a comment
There was a problem hiding this comment.
Please also fix the commit message using the conventional commit spec and respecting github character limits.
| } | ||
|
|
||
| // Check if first-time setup has been completed by looking at UserSettings.json | ||
| std::filesystem::path userSettingsPath = "Data\\SKSE\\Plugins\\CommunityShaders\\UserSettings.json"; |
There was a problem hiding this comment.
We have helper functions to find paths, etc. Don't introduce yet another hard coded path.
There was a problem hiding this comment.
While i am in here, going to fix icon/font paths too since these are hardcoded. might as well not half ass it. I haven't actually read through this filesystem.cpp so good to know it's here. Will use this from now on.
i.e. fonts currently are -> "Data\Interface\CommunityShaders\Fonts\Jost-Regular.ttf" but this can be moved into this function
There was a problem hiding this comment.
I'm pretty sure Discord doesn't allow you to use their logo like this.
There was a problem hiding this comment.
We use it in all of our branding currently? If its an issue here its likely an issue everywhere.
There was a problem hiding this comment.
Yah, I don't care if our Nexus page gets a TM takedown. I care if our Github repo does.
There was a problem hiding this comment.
Very fair point - you reckon banner is okay if i remake minus official discord logo?
I think the banner is much nicer than just a button, breaks up wall of text
There was a problem hiding this comment.
https://discord.com/branding Please review this and lmk what you think
Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.
davo0411
changed the title
Updated title, fixed |
davo0411
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/Menu/OverlayRenderer.cpp (1)
34-39: First-time setup dialog is still gated out by early return; include it in the skip predicateAs previously noted, the early return in
ShouldSkipRendering()preventsRenderFirstTimeSetupOverlay()and the cursor toggle from ever running when only the first-time setup should be shown.Fix by allowing rendering when
HomePageRenderer::ShouldShowFirstTimeSetup()is true. Either updateShouldSkipRendering()or guard the early return with that condition.Preferred patch (update predicate):
bool OverlayRenderer::ShouldSkipRendering() { auto shaderCache = globals::shaderCache; auto failed = shaderCache->GetFailedTasks(); auto hide = shaderCache->IsHideErrors(); auto* abTestingManager = ABTestingManager::GetSingleton(); - return !(shaderCache->IsCompiling() || - Menu::GetSingleton()->IsEnabled || - abTestingManager->IsEnabled() || - (failed && !hide) || - globals::features::performanceOverlay.settings.ShowInOverlay); + return !(shaderCache->IsCompiling() || + Menu::GetSingleton()->IsEnabled || + abTestingManager->IsEnabled() || + (failed && !hide) || + globals::features::performanceOverlay.settings.ShowInOverlay || + HomePageRenderer::ShouldShowFirstTimeSetup()); }Alternative (guard the early return):
- if (ShouldSkipRendering()) { + if (!HomePageRenderer::ShouldShowFirstTimeSetup() && ShouldSkipRendering()) { auto& io = ImGui::GetIO(); io.ClearInputKeys(); io.ClearEventsQueue(); return; }
🧹 Nitpick comments (13)
src/Utils/FileSystem.h (1)
3-3: Self-include is redundant and brittle; drop itThis header includes itself (
#include "FileSystem.h"). It’s harmless due to#pragma once, but it’s confusing and can create odd include-order dependencies. Safe to remove.-#include "FileSystem.h"src/Menu/ThemeManager.cpp (1)
130-135: Log the resolved font path on failure for easier diagnosisGood switch to a path helper. When loading fails, logging the actual path helps users troubleshoot packaging/MO2 VFS issues.
- if (!io.Fonts->AddFontFromFileTTF(fontPath.string().c_str(), - std::round(fontSize), &font_config)) { - logger::warn("ThemeManager::ReloadFont() - Failed to load custom font. Using default font."); + if (!io.Fonts->AddFontFromFileTTF(fontPath.string().c_str(), + std::round(fontSize), &font_config)) { + logger::warn("ThemeManager::ReloadFont() - Failed to load custom font at {}. Using default font.", fontPath.string()); io.Fonts->AddFontDefault(); }src/Menu.cpp (1)
189-194: Mirror ThemeManager: include the path in the failure logFor consistency with ThemeManager and to aid debugging, include the computed font path in the warning.
- if (!imgui_io.Fonts->AddFontFromFileTTF(fontPath.string().c_str(), - std::round(fontSize), &font_config)) { - logger::warn("Menu::Init() - Failed to load custom font. Using default font."); + if (!imgui_io.Fonts->AddFontFromFileTTF(fontPath.string().c_str(), + std::round(fontSize), &font_config)) { + logger::warn("Menu::Init() - Failed to load custom font at {}. Using default font.", fontPath.string()); imgui_io.Fonts->AddFontDefault(); }src/Menu/HomePageRenderer.h (2)
10-13: Make constants header-safe and ODR-proof; prefer string_viewFor cross-TU headers, prefer inline constexpr with std::string_view for string-like constants to avoid any ODR headaches.
- static constexpr const char* DISCORD_URL = "https://discord.com/invite/nkrQybAsyy"; + inline static constexpr std::string_view DISCORD_URL = "https://discord.com/invite/nkrQybAsyy";Also update includes:
-#include <functional> -#include <string> +#include <string_view>
15-20: API shape looks good; consider noexcept if implementations don’t throwIf
ShouldShowFirstTimeSetup()andRenderFirstTimeSetupDialog()don’t throw, marking themnoexcepthelps document intent and can enable small codegen wins.Please confirm whether the implementations can throw (e.g., JSON parsing exceptions). If they can, consider catching internally and downgrading to logged warnings to avoid taking down the frame loop.
src/Utils/FileSystem.cpp (2)
31-80: Avoid “UserSettings” vs “SettingsUser” naming inversionIn the same helper set we now have:
- GetUserSettingsPath() → .../UserSettings.json
- GetSettingsUserPath() → .../SettingsUser.json
This inversion is easy to confuse at call sites. Recommend either:
- Deprecate GetSettingsUserPath()/GetSettingsTestPath()/GetSettingsDefaultPath() in favor of a single GetSettingsPath(enum class Kind { User, Test, Default }) or,
- Rename to GetUserSettingsPath(), GetTestSettingsPath(), GetDefaultSettingsPath() for consistency.
If you prefer a low-impact change now, add brief comments on each function clarifying intended consumers until a consolidation pass is made.
- std::filesystem::path GetSettingsUserPath() + // User-specific settings written by the UI (not the same as UserSettings.json) + std::filesystem::path GetSettingsUserPath()
41-55: Consider real-path counterparts for UI assetsYou already expose GetShadersRealPath()/GetFeaturesRealPath(). If the UI ever needs to load fonts/icons directly from the DLL’s folder (e.g., outside MO2 VFS), adding GetInterfaceRealPath()/GetFontsRealPath()/GetIconsRealPath() would mirror that pattern and give resilience if Data/ is not writable or not mounted.
src/Menu/HomePageRenderer.cpp (6)
4-11: Drop unused headersstd::ranges, , and are not used in this TU. Removing them speeds builds and reduces PCH churn.
-#include <format> -#include <ranges> -#include <sstream>
78-86: Centering logic doesn’t match wrapped renderingYou compute center using unwrapped width, then call TextWrapped, which can wrap to narrower width and appear offset. Use the same “wrapped-size-then-center” pattern you already use for versionText/laterText.
- // Intro text - centered - const char* introText = - "Community Shaders provides advanced graphics enhancements for Skyrim.\n" - "This comprehensive collection of features brings modern rendering techniques\n" - "to enhance your visual experience."; - ImVec2 introSize = ImGui::CalcTextSize(introText); - ImGui::SetCursorPosX((windowSize.x - introSize.x) * 0.5f); - ImGui::TextWrapped("%s", introText); + // Intro text - wrapped and truly centered + const char* introText = + "Community Shaders provides advanced graphics enhancements for Skyrim.\n" + "This comprehensive collection of features brings modern rendering techniques\n" + "to enhance your visual experience."; + const float introPadding = 40.0f; + ImGui::SetCursorPosX(introPadding); + ImGui::BeginGroup(); + ImGui::PushTextWrapPos(windowSize.x - introPadding); + ImVec2 introSizeWrapped = ImGui::CalcTextSize(introText, nullptr, true, windowSize.x - introPadding * 2); + float introCenterOffset = (windowSize.x - introPadding * 2 - introSizeWrapped.x) * 0.5f; + if (introCenterOffset > 0) { + ImGui::SetCursorPosX(introPadding + introCenterOffset); + } + ImGui::TextWrapped("%s", introText); + ImGui::PopTextWrapPos(); + ImGui::EndGroup();
148-161: Centralize link URLs and add a robust OpenUrl helper (use ShellExecuteW + error check)Hard-coded URLs are scattered and ShellExecuteA return value is ignored. Centralize constants and use a single helper to open URLs with proper error handling.
- if (ImGui::Button("Nexus Mods", ImVec2(buttonWidth, 0))) { - ShellExecuteA(NULL, "open", "https://www.nexusmods.com/skyrimspecialedition/mods/86492", NULL, NULL, SW_SHOWNORMAL); - } + if (ImGui::Button("Nexus Mods", ImVec2(buttonWidth, 0))) { + OpenUrl(L"https://www.nexusmods.com/skyrimspecialedition/mods/86492"); + } @@ - if (ImGui::Button("GitHub Repository", ImVec2(buttonWidth, 0))) { - ShellExecuteA(NULL, "open", "https://github.com/doodlum/skyrim-community-shaders", NULL, NULL, SW_SHOWNORMAL); - } + if (ImGui::Button("GitHub Repository", ImVec2(buttonWidth, 0))) { + OpenUrl(L"https://github.com/doodlum/skyrim-community-shaders"); + } @@ - if (ImGui::Button("GitHub Wiki", ImVec2(buttonWidth, 0))) { - ShellExecuteA(NULL, "open", "https://github.com/doodlum/skyrim-community-shaders/wiki", NULL, NULL, SW_SHOWNORMAL); - } + if (ImGui::Button("GitHub Wiki", ImVec2(buttonWidth, 0))) { + OpenUrl(L"https://github.com/doodlum/skyrim-community-shaders/wiki"); + }Add this helper near the top of the file (outside the shown ranges):
// Open a URL in the default browser. Returns true on success. static bool OpenUrl(const wchar_t* url) { const auto r = reinterpret_cast<INT_PTR>(ShellExecuteW(nullptr, L"open", url, nullptr, nullptr, SW_SHOWNORMAL)); if (r <= 32) { logger::warn("Failed to open URL: {}", std::string(url, url + wcslen(url))); return false; } return true; }Follow-up: Also switch the Discord button handlers to use OpenUrl(L"...") and centralize URLs as constants (e.g., in a header) to avoid future drift.
109-111: Apply the OpenUrl helper to Discord link and prefer wide-char APISame rationale as Quick Links: reduce duplication and add error checks.
- if (ImGui::ImageButton("##DiscordButton", menu->uiIcons.discord.texture, iconSize)) { - ShellExecuteA(NULL, "open", DISCORD_URL, NULL, NULL, SW_SHOWNORMAL); - } + if (ImGui::ImageButton("##DiscordButton", menu->uiIcons.discord.texture, iconSize)) { + OpenUrl(L"" DISCORD_URL L""); + } @@ - if (ImGui::Button("Join Discord Server", ImVec2(buttonWidth, 0))) { - ShellExecuteA(NULL, "open", DISCORD_URL, NULL, NULL, SW_SHOWNORMAL); - } + if (ImGui::Button("Join Discord Server", ImVec2(buttonWidth, 0))) { + OpenUrl(L"" DISCORD_URL L""); + }If DISCORD_URL is currently a narrow string literal, define a wide version (e.g., DISCORD_URL_W) or store URLs as std::wstring.
Also applies to: 124-126
41-75: Font selection heuristic may be brittleChoosing Fonts[1] assumes a specific load order. Consider asking ThemeManager (or Menu) for a designated “title” font handle to avoid accidental changes if font lists are re-ordered.
501-513: Consider logging on write failures and explicit truncationCatching and ignoring all exceptions makes failures silent. At minimum, log a warning so we can diagnose broken setups. Also open with std::ios::trunc explicitly.
- std::ofstream outFile(userSettingsPath); + std::ofstream outFile(userSettingsPath, std::ios::out | std::ios::trunc); @@ - } catch (const std::exception&) { - // If we can't write the file, just mark as shown this session to avoid repeated popups - } + } catch (const std::exception& e) { + logger::warn("Failed to write FirstTimeSetupCompleted to {}: {}", userSettingsPath.string(), e.what()); + // Avoid repeated popups in this session even if persistence failed + }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (10)
src/Menu.cpp(5 hunks)src/Menu/FeatureListRenderer.cpp(4 hunks)src/Menu/HomePageRenderer.cpp(1 hunks)src/Menu/HomePageRenderer.h(1 hunks)src/Menu/OverlayRenderer.cpp(3 hunks)src/Menu/OverlayRenderer.h(1 hunks)src/Menu/ThemeManager.cpp(2 hunks)src/Utils/FileSystem.cpp(1 hunks)src/Utils/FileSystem.h(1 hunks)src/Utils/UI.cpp(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/Menu/OverlayRenderer.h
- src/Menu/FeatureListRenderer.cpp
- src/Utils/UI.cpp
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{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/ThemeManager.cppsrc/Utils/FileSystem.hsrc/Menu.cppsrc/Menu/OverlayRenderer.cppsrc/Menu/HomePageRenderer.cppsrc/Utils/FileSystem.cppsrc/Menu/HomePageRenderer.h
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/ThemeManager.cppsrc/Utils/FileSystem.hsrc/Menu.cppsrc/Menu/OverlayRenderer.cppsrc/Menu/HomePageRenderer.cppsrc/Utils/FileSystem.cppsrc/Menu/HomePageRenderer.h
🧠 Learnings (2)
📚 Learning: 2025-08-25T09:07:45.236Z
Learnt from: davo0411
PR: doodlum/skyrim-community-shaders#1431
File: src/Menu/HomePageRenderer.cpp:0-0
Timestamp: 2025-08-25T09:07:45.236Z
Learning: First-time setup dialogs in the Community Shaders project are typically one-time only dialogs that cannot be manually reopened by users, so static variable persistence across dialog sessions is not a concern for these types of dialogs.
Applied to files:
src/Menu/HomePageRenderer.cpp
📚 Learning: 2025-07-05T05:20:45.823Z
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#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.
Applied to files:
src/Utils/FileSystem.cpp
⏰ 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 (11)
src/Utils/FileSystem.h (1)
35-93: I wasn’t able to locateFileSystem.h/.cppor theUtil::PathHelpersdefinitions in the repo—please confirm the file paths and that all of the newGet*Pathhelpers are implemented (and update your import/query paths accordingly) so we can verify the naming and[[nodiscard]]suggestions.src/Menu.cpp (2)
129-139: LGTM: releasing new icon texturesReleasing
discordandpostProcessingalongside existing icons looks correct and keeps lifetime management consistent.
619-620: Correct: swallow input during first-time setup tooIncluding
HomePageRenderer::ShouldShowFirstTimeSetup()here ensures the game doesn’t receive input while the setup dialog is up.src/Menu/OverlayRenderer.cpp (2)
47-51: Overlay visibility/mouse cursor handling is correctShowing the cursor when either the menu is enabled or first-time setup is active, and suppressing the full settings pane during first-time setup, is the right UX.
201-206: Good separation: thin wrapper delegates to HomePageRenderer
RenderFirstTimeSetupOverlay()is a clean hook point; once the early-return issue is addressed, this will behave as intended.src/Menu/HomePageRenderer.h (1)
28-30: Ensure a single definition ofHomePageRenderer::isFirstTimeSetupShown
I wasn’t able to locate any.cppimplementations in the workspace—please manually verify that:
bool HomePageRenderer::isFirstTimeSetupShownis defined exactly once in a corresponding.cppfile (with an initializer).- There are no duplicate or missing definitions across your implementation files.
src/Utils/FileSystem.cpp (1)
31-80: Good centralization of UI/resource pathsThese helpers make call sites cleaner and reduce hard-coded paths throughout the UI code. Naming and directory structure align with existing Data/SKSE/Plugins/CommunityShaders layout.
src/Menu/HomePageRenderer.cpp (4)
420-429: Clarify Enter/Escape behavior for first-time setupCurrently Escape confirms and completes setup the same as Enter/Continue. That might surprise users who press Esc to “cancel”. Confirm desired UX. If Esc should not confirm, restrict to Enter (and keypad Enter).
-bool shouldClose = ImGui::IsKeyPressed(ImGuiKey_Enter) || ImGui::IsKeyPressed(ImGuiKey_Escape); +bool shouldClose = ImGui::IsKeyPressed(ImGuiKey_Enter) || ImGui::IsKeyPressed(ImGuiKey_KeypadEnter);
139-161: Nice centered row layout and spacingThe math for centering three fixed-width buttons with ItemSpacing is clear and correct. This reads well.
277-298: Guard against zero-sized logo to avoid divide-by-zeroaspectRatio = width/height will fault if height is 0 (or produce inf). Also ensure the texture size is valid before AddImage.
- if (menu && menu->uiIcons.logo.texture) { + if (menu && menu->uiIcons.logo.texture) { ImVec2 windowPos = ImGui::GetWindowPos(); ImVec2 windowSize = ImGui::GetWindowSize(); - ImVec2 textureSize = menu->uiIcons.logo.size; - float aspectRatio = textureSize.x / textureSize.y; + ImVec2 textureSize = menu->uiIcons.logo.size; + if (textureSize.x > 0.0f && textureSize.y > 0.0f) { + float aspectRatio = textureSize.x / textureSize.y; float logoHeight = LOGO_WATERMARK_HEIGHT; float logoWidth = logoHeight * aspectRatio; ImVec2 logoMin(windowPos.x + (windowSize.x - logoWidth) * 0.5f, windowPos.y + (windowSize.y - logoHeight) * 0.5f); ImVec2 logoMax(logoMin.x + logoWidth, logoMin.y + logoHeight); ImU32 watermarkColor = IM_COL32(255, 255, 255, 60); ImGui::GetWindowDrawList()->AddImage(menu->uiIcons.logo.texture, logoMin, logoMax, ImVec2(0, 0), ImVec2(1, 1), watermarkColor); + } else { + logger::warn("Logo texture size invalid ({}x{}); skipping watermark.", textureSize.x, textureSize.y); + } }⛔ Skipped due to learnings
Learnt from: davo0411 PR: doodlum/skyrim-community-shaders#1107 File: src/Menu.cpp:310-315 Timestamp: 2025-05-31T02:51:24.195Z Learning: In the Community Shaders codebase, when validating UI icons, checking for texture presence (non-null pointer) is sufficient. Avoid adding dimension validation checks as icon sizes should vary based on user needs (4K+ resolution users may want higher resolution icons). Don't artificially limit icon dimensions.
240-245: License file not found – please verify before shippingI wasn’t able to locate any LICENSE or COPYING file in the repository, yet the UI text claims the project is “licensed under GPL.” Before merging this change:
- Confirm the project’s actual license and ensure a corresponding LICENSE (or COPYING) file is present at the repository root.
- Avoid hard-coding the license name in UI strings; instead:
- Link the UI text to the repository’s LICENSE file.
- Or surface the license identifier via a build-time constant (e.g.,
PROJECT_LICENSE) so the text always stays in sync.
|
There is an alternative PR title as: Since the welcome window greets the user, and the element for selecting the key is part of this window in addition to other content in it and is simply a convenient setting. It seems to me that such a title better reflects the essence |
davo0411
changed the title
|
Make sure you've tested this with VR users or disabled it. You may trap some people unable to move with this. |
thank you i didnt think of this. will report back |
Users state a couple very minor QoL isssues but nothing game breaking. Several conflicting reports, some had it work perfect, some had the message only show up on monitor and not in headset. Didn't get users stuck though, just had to take headset off to dismiss. Can make follow up at some point to address these but i don't have the time atm to go back and forth with testers for something that is this minor. |
|
Disable for VR then if you don't have time to fully confirm. Note, only some users can see it in the hmd. Others are using unsupported openvr dlls so they'd end up with it on the desktop screen only. |
Fixed, should never show in VR now. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/Menu/HomePageRenderer.cpp (2)
148-161: Centralize quick-link URLs as constants.Prior feedback asked to avoid hard-coded links. Define them once and reuse.
Apply this diff:
using json = nlohmann::json; +namespace { + constexpr const char* NEXUS_URL = "https://www.nexusmods.com/skyrimspecialedition/mods/86492"; + constexpr const char* GITHUB_URL = "https://github.com/doodlum/skyrim-community-shaders"; + constexpr const char* WIKI_URL = "https://github.com/doodlum/skyrim-community-shaders/wiki"; +} +- if (ImGui::Button("Nexus Mods", ImVec2(buttonWidth, 0))) { - ShellExecuteA(NULL, "open", "https://www.nexusmods.com/skyrimspecialedition/mods/86492", NULL, NULL, SW_SHOWNORMAL); - } + if (ImGui::Button("Nexus Mods", ImVec2(buttonWidth, 0))) { + ShellExecuteA(NULL, "open", NEXUS_URL, NULL, NULL, SW_SHOWNORMAL); + } @@ - if (ImGui::Button("GitHub Repository", ImVec2(buttonWidth, 0))) { - ShellExecuteA(NULL, "open", "https://github.com/doodlum/skyrim-community-shaders", NULL, NULL, SW_SHOWNORMAL); - } + if (ImGui::Button("GitHub Repository", ImVec2(buttonWidth, 0))) { + ShellExecuteA(NULL, "open", GITHUB_URL, NULL, NULL, SW_SHOWNORMAL); + } @@ - if (ImGui::Button("GitHub Wiki", ImVec2(buttonWidth, 0))) { - ShellExecuteA(NULL, "open", "https://github.com/doodlum/skyrim-community-shaders/wiki", NULL, NULL, SW_SHOWNORMAL); - } + if (ImGui::Button("GitHub Wiki", ImVec2(buttonWidth, 0))) { + ShellExecuteA(NULL, "open", WIKI_URL, NULL, NULL, SW_SHOWNORMAL); + }Also applies to: 21-25
169-246: Hard-coded FAQ content increases maintenance burden.Prior feedback questioned adding another FAQ surface with baked-in text. Consider trimming or sourcing from a single canonical location (e.g., Wiki) to avoid release churn for copy changes.
🧹 Nitpick comments (5)
src/Menu/FeatureListRenderer.cpp (3)
21-27: Align CORE_MENU_NAMES with actual built-ins (Display isn’t added).CORE_MENU_NAMES lists "Display", but BuildMenuList adds only "Home", "General", "Advanced". This mismatch makes the “core” render loop scan the entire list every frame and the comments become misleading.
If “Display” isn’t a built-in anymore, update the array and comments:
-// Core built-in menu names that always appear first in the menu list -constexpr std::array<const char*, 4> CORE_MENU_NAMES = { "Home", "General", "Advanced", "Display" }; +// Core built-in menus that always appear first +constexpr std::array<const char*, 3> CORE_MENU_NAMES = { "Home", "General", "Advanced" };If “Display” is still intended as a built-in, add it to the menu list in BuildMenuList for consistency.
189-199: Remove unused pre-count and simplify ‘core’ render loop.
coreMenuCountis computed then never used;renderedCoreMenus < CORE_MENU_NAMES.size()can be dropped or made to use the actual present count. Simpler is best here.Apply this diff:
- // Find where core built-in menus end (Home, General, Advanced, Display) - size_t coreMenuCount = 0; - for (size_t i = 0; i < menuList.size(); i++) { - if (std::holds_alternative<BuiltInMenu>(menuList[i])) { - const BuiltInMenu& menu = std::get<BuiltInMenu>(menuList[i]); - if (IsCoreMenu(menu.name)) { - coreMenuCount++; - } - } - } - - // First render the core built-in menus (Home, General, Advanced, Display) - size_t renderedCoreMenus = 0; - for (size_t i = 0; i < menuList.size() && renderedCoreMenus < CORE_MENU_NAMES.size(); i++) { + // Render the core built-in menus first (in list order) + for (size_t i = 0; i < menuList.size(); i++) { if (std::holds_alternative<BuiltInMenu>(menuList[i])) { const BuiltInMenu& menu = std::get<BuiltInMenu>(menuList[i]); if (IsCoreMenu(menu.name)) { std::visit(ListMenuVisitor{ i, selectedMenu, categoryExpansionStates }, menuList[i]); - renderedCoreMenus++; } } }Also applies to: 200-208
212-216: Consider keeping “Feature Issues” above the “Features” header.You insert “Feature Issues” at the beginning of the list but render the “Features” header before rendering non-core built-ins, so “Feature Issues” appears under the header. If the intent is a top-of-list warning, render it before drawing the header or treat it as a “core” built-in in the first loop.
src/Menu/HomePageRenderer.cpp (2)
79-86: Centering with wrapped text is off.You center using unwrapped
CalcTextSizeand then callTextWrapped, which can reflow and misalign. Consider the same wrap-and-center approach you used later (lines 391–415).Want a small patch to reuse that pattern here?
256-265: Extract remaining magic numbers to constants.Window size (500x400), rounding (8.0f), border (1.0f), and the Continue button width (140.0f) should live in HomePageRenderer::Constants alongside TITLE_FONT_SCALE/LOGO_WATERMARK_HEIGHT/QUICK_LINKS_BUTTON_WIDTH for consistency.
Also applies to: 420-422
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package/Interface/CommunityShaders/Icons/Action Icons/discord.pngis excluded by!**/*.png
📒 Files selected for processing (4)
src/Menu.cpp(5 hunks)src/Menu.h(1 hunks)src/Menu/FeatureListRenderer.cpp(4 hunks)src/Menu/HomePageRenderer.cpp(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/Menu.h
- src/Menu.cpp
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{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/HomePageRenderer.cppsrc/Menu/FeatureListRenderer.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/HomePageRenderer.cppsrc/Menu/FeatureListRenderer.cpp
🧠 Learnings (1)
📚 Learning: 2025-08-25T09:07:45.255Z
Learnt from: davo0411
PR: doodlum/skyrim-community-shaders#1431
File: src/Menu/HomePageRenderer.cpp:0-0
Timestamp: 2025-08-25T09:07:45.255Z
Learning: First-time setup dialogs in the Community Shaders project are typically one-time only dialogs that cannot be manually reopened by users, so static variable persistence across dialog sessions is not a concern for these types of dialogs.
Applied to files:
src/Menu/HomePageRenderer.cpp
⏰ 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 (2)
src/Menu/FeatureListRenderer.cpp (1)
80-83: LGTM: Home page integration point is clean.Prepending the “Home” built-in and delegating to HomePageRenderer keeps responsibilities separated.
src/Menu/HomePageRenderer.cpp (1)
423-429: Confirm Escape behavior finalizes setup.Right now Enter/Escape both mark setup complete. If Escape should cancel instead of confirming, adjust
shouldCloseaccordingly.
| namespace | ||
| { | ||
| // Core built-in menu names that always appear first in the menu list | ||
| constexpr std::array<const char*, 4> CORE_MENU_NAMES = { "Home", "General", "Advanced", "Display" }; | ||
|
|
||
| bool IsCoreMenu(const std::string& menuName) | ||
| { | ||
| return std::find(CORE_MENU_NAMES.begin(), CORE_MENU_NAMES.end(), menuName) != CORE_MENU_NAMES.end(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Add missing <array> include to avoid compile error.
You introduced std::array in this block but didn’t include the header.
Apply this diff to add the include:
#include <algorithm>
+#include <array>
#include <filesystem>
#include <format>
#include <imgui.h>
#include <ranges>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| namespace | |
| { | |
| // Core built-in menu names that always appear first in the menu list | |
| constexpr std::array<const char*, 4> CORE_MENU_NAMES = { "Home", "General", "Advanced", "Display" }; | |
| bool IsCoreMenu(const std::string& menuName) | |
| { | |
| return std::find(CORE_MENU_NAMES.begin(), CORE_MENU_NAMES.end(), menuName) != CORE_MENU_NAMES.end(); | |
| } | |
| } | |
| #include <algorithm> | |
| #include <array> | |
| #include <filesystem> | |
| #include <format> | |
| #include <imgui.h> | |
| #include <ranges> |
🤖 Prompt for AI Agents
In src/Menu/FeatureListRenderer.cpp around lines 19 to 28, the code uses
std::array but the header <array> is not included which causes a compile error;
add a single line include for <array> near the other includes at the top of the
file (e.g., alongside existing standard headers) so the constexpr std::array
declaration and subsequent usage compile successfully.
| // Hotkey selection - clickable hotkey text | ||
| // Show current toggle key and allow user to change it by clicking on it | ||
| auto& themeSettings = menu->GetTheme(); | ||
| const char* currentKeyName = Util::Input::KeyIdToString(menu->GetSettings().ToggleKey); | ||
|
|
There was a problem hiding this comment.
Null-dereference risk on Menu in hotkey section.
menu can be null; dereferencing for theme, ToggleKey, and settingToggleKey would crash.
Apply this diff to add safe fallbacks and guards:
- // Hotkey selection - clickable hotkey text
- // Show current toggle key and allow user to change it by clicking on it
- auto& themeSettings = menu->GetTheme();
- const char* currentKeyName = Util::Input::KeyIdToString(menu->GetSettings().ToggleKey);
+ // Hotkey selection - clickable hotkey text
+ // Show current toggle key and allow user to change it by clicking on it
+ ImVec4 baseHotkeyColor = ImGui::GetStyleColorVec4(ImGuiCol_Text);
+ uint32_t currentKey = VK_END;
+ if (menu) {
+ baseHotkeyColor = menu->GetTheme().StatusPalette.CurrentHotkey;
+ currentKey = menu->GetSettings().ToggleKey;
+ }
+ const char* currentKeyName = Util::Input::KeyIdToString(currentKey);
@@
- // Choose color based on hover state - darken when hovered.
- ImVec4 hotkeyColor = hovered ?
- ImVec4(themeSettings.StatusPalette.CurrentHotkey.x * 0.7f,
- themeSettings.StatusPalette.CurrentHotkey.y * 0.7f,
- themeSettings.StatusPalette.CurrentHotkey.z * 0.7f,
- themeSettings.StatusPalette.CurrentHotkey.w) :
- themeSettings.StatusPalette.CurrentHotkey;
+ // Choose color based on hover state - darken when hovered.
+ ImVec4 hotkeyColor = hovered
+ ? ImVec4(baseHotkeyColor.x * 0.7f, baseHotkeyColor.y * 0.7f, baseHotkeyColor.z * 0.7f, baseHotkeyColor.w)
+ : baseHotkeyColor;
@@
- if (clicked) {
- menu->settingToggleKey = true;
- }
+ if (clicked && menu) {
+ menu->settingToggleKey = true;
+ }
@@
- if (menu->settingToggleKey) {
+ if (menu && menu->settingToggleKey) {
const char* pressKeyText = "Press any key to set as toggle key...";
float pressKeyWidth = ImGui::CalcTextSize(pressKeyText).x;
ImGui::SetCursorPosX((windowWidth - pressKeyWidth) * 0.5f);
ImGui::Text("%s", pressKeyText);
}Also applies to: 365-373, 376-386, 381-386
🤖 Prompt for AI Agents
In src/Menu/HomePageRenderer.cpp around lines 339-343 (and similarly at 365-373,
376-386, 381-386), the code dereferences menu without a null check when
accessing theme, settings.ToggleKey, and other members; add a nullptr guard and
safe fallbacks: first check if (menu == nullptr) and return or skip rendering
the hotkey section, or use a local safe reference (e.g., auto* menuPtr = menu;
if (!menuPtr) { /* use default theme/settings or return */ }), then when reading
ToggleKey and theme use defaults if menu is null (e.g., default key name and
default theme colors) and avoid calling methods on menu; ensure all uses in the
listed line ranges are protected by the same guard or fallback logic to prevent
null-dereference crashes.
alandtse
changed the title
4 participants
Bottom discord banner is the updated one that fits branding legal requirements
Users will only see the popup to select the menu hotkey when one of the following is true:
Fresh install: UserSettings.json doesn't exist → popup shows
Deleted settings: User deletes UserSettings.json → popup shows
Corrupted file: Can't read UserSettings.json → popup shows
Missing flag: File exists but no FirstTimeSetupCompleted field → popup shows
Also includes minor refactor for file path locations commonly used by UI elements, and a refactor for menu headings to be less rigid.
Summary by CodeRabbit
New Features
UI Improvements
Behavior