refactor(menu): regroup Advanced tabs by purpose#28
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR reorganizes the Advanced Settings UI by introducing a dedicated Shaders tab, refactoring shader controls into component methods, restructuring Diagnostics with separate logging and runtime debug sections, adding shader compilation statistics, expanding the Testing section, and relocating the cache confirmation checkbox from Shaders to Behavior tab. ChangesAdvanced Settings UI Reorganization
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
No actionable suggestions for changed features. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Menu/AdvancedSettingsRenderer.cpp`:
- Around line 409-412: The ImGui::SameLine() call is misplaced inside the
ImGui::Combo change branch and is dead code; move the layout call out of the
conditional or remove it so it's used only for arranging widgets, and ensure
ImGui::Combo("Log Level", &item_current, items, IM_ARRAYSIZE(items)) and
globals::state->SetLogLevel(static_cast<spdlog::level::level_enum>(item_current))
remain intact while deleting or relocating ImGui::SameLine() so it is not
executed only when SetLogLevel runs.
- Line 268: Replace the unsafe ImGui::Text(std::format(...).c_str()) call so the
result string isn't treated as a format string: either pass the formatted string
to ImGui::TextUnformatted(...) or call ImGui::Text("%s",
shaderCache->GetShaderStatsString().c_str()); update the usage around
ImGui::Text, std::format and shaderCache->GetShaderStatsString() in
AdvancedSettingsRenderer (the line creating the "Shader Compiler" text)
accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f09c8d14-5240-47f1-8563-935bb53160af
📒 Files selected for processing (5)
docs/development/vscode-setup.mdsrc/FeatureIssues.cppsrc/Menu/AdvancedSettingsRenderer.cppsrc/Menu/AdvancedSettingsRenderer.hsrc/Menu/SettingsTabRenderer.cpp
Addresses CodeRabbit findings on PR #28: - RenderShaderCompileStatistics passed the result of std::format to ImGui::Text via .c_str(); ImGui::Text treats its first arg as a printf format string, so any '%' in the shader stats would be interpreted. Use the "%s" + .c_str() form directly. - The Log Level combo had an ImGui::SameLine() inside the change branch, before a non-widget call — dead layout op. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Refactors the in-game Settings → Advanced UI to group options by purpose rather than by audience, while moving one UI behavior toggle to a more appropriate tab and updating developer documentation accordingly.
Changes:
- Reorganized Advanced settings into purpose-based tabs (Shaders / Diagnostics / Disable at Boot / Testing) and split large sections into helper render functions.
- Moved “Skip Clear Cache …” from shader-related settings to UI Behavior in the Behavior tab.
- Updated VSCode setup documentation to match the new Advanced menu path for the built-in shader file watcher.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Menu/SettingsTabRenderer.cpp | Moves the “Skip Clear Cache …” checkbox into the Behavior tab UI behavior section. |
| src/Menu/AdvancedSettingsRenderer.h | Updates the Advanced settings renderer interface to new purpose-based sections + helper methods. |
| src/Menu/AdvancedSettingsRenderer.cpp | Implements the new Advanced tab layout and refactors rendering into purpose-based sections. |
| src/FeatureIssues.cpp | Removes the extra nested “Testing” collapsing header so the testing UI renders inline. |
| docs/development/vscode-setup.md | Updates documentation to the new Advanced → Shaders → Cache & File Watcher path. |
Comments suppressed due to low confidence (2)
src/Menu/AdvancedSettingsRenderer.cpp:171
- The SliderInt max uses std::thread::hardware_concurrency() directly. Per the standard this can return 0, which would make the slider range invalid (min=1 > max=0) and can trigger ImGui assertions. Consider clamping the max to at least 1 (e.g., use std::max(1u, std::thread::hardware_concurrency()) or a small helper) for both thread sliders.
ImGui::SliderInt("Compiler Threads", &shaderCache->compilationThreadCount, 1, static_cast<int32_t>(std::thread::hardware_concurrency()));
if (auto _tt = Util::HoverTooltipWrapper()) {
ImGui::Text(
"Number of threads used to compile shaders at startup. "
"Defaults to all logical cores minus one for OS headroom (E-cores included). "
"Higher values finish compilation faster but may make the system less responsive.");
}
ImGui::SliderInt("Background Compiler Threads", &shaderCache->backgroundCompilationThreadCount, 1, static_cast<int32_t>(std::thread::hardware_concurrency()));
if (auto _tt = Util::HoverTooltipWrapper()) {
src/Menu/AdvancedSettingsRenderer.cpp:114
State::SetDefines()does parsing + logging; calling it on everyImGui::InputTextchange will re-parse and potentially spam debug logs on every keystroke. Since you already apply/clear-cache onIsItemDeactivatedAfterEdit()/ Enter, consider only callingSetDefinesin that commit path (or throttle) to reduce per-keystroke work while typing.
// Shader Defines input
auto& shaderDefines = globals::state->shaderDefinesString;
if (ImGui::InputText("Shader Defines", &shaderDefines)) {
globals::state->SetDefines(shaderDefines);
}
if (ImGui::IsItemDeactivatedAfterEdit() || (ImGui::IsItemActive() &&
(ImGui::IsKeyPressed(ImGuiKey_Enter) ||
ImGui::IsKeyPressed(ImGuiKey_KeypadEnter)))) {
globals::state->SetDefines(shaderDefines);
shaderCache->Clear();
}
|
✅ A pre-release build is available for this PR: |
Addresses CodeRabbit findings on PR #28: - RenderShaderCompileStatistics passed the result of std::format to ImGui::Text via .c_str(); ImGui::Text treats its first arg as a printf format string, so any '%' in the shader stats would be interpreted. Use the "%s" + .c_str() form directly. - The Log Level combo had an ImGui::SameLine() inside the change branch, before a non-widget call — dead layout op. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
57dbe9f to
0315274
Compare
Addresses CodeRabbit findings on PR #28: - RenderShaderCompileStatistics passed the result of std::format to ImGui::Text via .c_str(); ImGui::Text treats its first arg as a printf format string, so any '%' in the shader stats would be interpreted. Use the "%s" + .c_str() form directly. - The Log Level combo had an ImGui::SameLine() inside the change branch, before a non-widget call — dead layout op. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
0315274 to
d6931b9
Compare
Copilot findings on PR #28: - RenderLoggingControls cached the log-level combo index in a static initialized only on first call. If anything else changed the log level (config reload, console command, another caller of SetLogLevel), the combo kept showing the old value and re-selecting the same item would silently write the wrong level back. Drop the static and read globals::state->GetLogLevel() each frame. - The "Skip Clear Cache Dialogue" checkbox label conflicted with the underlying SkipClearCacheConfirmation field and used "dialogue" (a conversation) where "dialog" / "confirmation" was meant. Rename to "Skip Clear Cache Confirmation" to match the field. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (2)
src/Menu/AdvancedSettingsRenderer.cpp:171
std::thread::hardware_concurrency()is permitted to return 0. Passing that directly as the slider max can produce an invalid range (min=1, max=0) and can also undermine clamping logic elsewhere. Consider computing a safe max likemax(1u, hardware_concurrency())(and reusing it for both sliders).
Util::DrawSectionHeader("Threading");
ImGui::SliderInt("Compiler Threads", &shaderCache->compilationThreadCount, 1, static_cast<int32_t>(std::thread::hardware_concurrency()));
if (auto _tt = Util::HoverTooltipWrapper()) {
ImGui::Text(
"Number of threads used to compile shaders at startup. "
"Defaults to all logical cores minus one for OS headroom (E-cores included). "
"Higher values finish compilation faster but may make the system less responsive.");
}
ImGui::SliderInt("Background Compiler Threads", &shaderCache->backgroundCompilationThreadCount, 1, static_cast<int32_t>(std::thread::hardware_concurrency()));
if (auto _tt = Util::HoverTooltipWrapper()) {
src/FeatureIssues.cpp:1527
- Tooltip text refers to 'Create Test Inis' but elsewhere uses the acronym 'INI'. Consider using 'Create Test INIs' for consistency.
ImGui::Text(
"Removes all test INI files and restores any modified INI files to their original state.\n"
"This undoes all changes made by 'Create Test Inis'.\n"
"Restart CS after restoring to see normal operation.");
Copilot finding on PR #28: "Inis" reads as a word; everywhere else in the file the acronym is rendered "INIs" ("test INI files", "the INI" etc.). Align the button label and its tooltip reference. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Addresses CodeRabbit findings on PR #28: - RenderShaderCompileStatistics passed the result of std::format to ImGui::Text via .c_str(); ImGui::Text treats its first arg as a printf format string, so any '%' in the shader stats would be interpreted. Use the "%s" + .c_str() form directly. - The Log Level combo had an ImGui::SameLine() inside the change branch, before a non-widget call — dead layout op. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Copilot findings on PR #28: - RenderLoggingControls cached the log-level combo index in a static initialized only on first call. If anything else changed the log level (config reload, console command, another caller of SetLogLevel), the combo kept showing the old value and re-selecting the same item would silently write the wrong level back. Drop the static and read globals::state->GetLogLevel() each frame. - The "Skip Clear Cache Dialogue" checkbox label conflicted with the underlying SkipClearCacheConfirmation field and used "dialogue" (a conversation) where "dialog" / "confirmation" was meant. Rename to "Skip Clear Cache Confirmation" to match the field. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Copilot finding on PR #28: "Inis" reads as a word; everywhere else in the file the acronym is rendered "INIs" ("test INI files", "the INI" etc.). Align the button label and its tooltip reference. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
b282ff0 to
211b77c
Compare
Old Advanced tabs (Developer / Logging / Shader Debug / Disable
at Boot / Testing) mixed audience and topic. Regrouped into four
alphabetical, purpose-based tabs:
- Shaders: Compile Flags, Threading, Cache & File Watcher,
Replace Original Shaders, Statistics
- Diagnostics: Logging, Runtime Debug, Shader Blocking (dev-only)
- Disable at Boot: unchanged
- Testing: A/B harness + dev-mode test scaffolding
Also:
- Drop redundant CollapsingHeader("Testing") inside the Testing
tab and CollapsingHeader("Active Shaders") inside the Shader
Blocking panel -- both nested headings under their own tab/panel.
- Move "Skip Clear Cache Dialogue" from Shaders tab to Behavior
(it's UI behavior, not a shader setting).
- Update docs/development/vscode-setup.md menu path to match.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Addresses CodeRabbit findings on PR #28: - RenderShaderCompileStatistics passed the result of std::format to ImGui::Text via .c_str(); ImGui::Text treats its first arg as a printf format string, so any '%' in the shader stats would be interpreted. Use the "%s" + .c_str() form directly. - The Log Level combo had an ImGui::SameLine() inside the change branch, before a non-widget call — dead layout op. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Copilot findings on PR #28: - RenderLoggingControls cached the log-level combo index in a static initialized only on first call. If anything else changed the log level (config reload, console command, another caller of SetLogLevel), the combo kept showing the old value and re-selecting the same item would silently write the wrong level back. Drop the static and read globals::state->GetLogLevel() each frame. - The "Skip Clear Cache Dialogue" checkbox label conflicted with the underlying SkipClearCacheConfirmation field and used "dialogue" (a conversation) where "dialog" / "confirmation" was meant. Rename to "Skip Clear Cache Confirmation" to match the field. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Copilot finding on PR #28: "Inis" reads as a word; everywhere else in the file the acronym is rendered "INIs" ("test INI files", "the INI" etc.). Align the button label and its tooltip reference. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
211b77c to
9bfa371
Compare
Address Copilot review comments on PR #28: - Use the local `shaderCache` alias consistently inside RenderShaderCompileFlags() instead of redundantly dereferencing globals::shaderCache for the two Clear() calls. - Clamp std::thread::hardware_concurrency() to at least 1 before using it as the SliderInt max, since the standard permits it to return 0 if the implementation can't detect core count, which would produce an invalid slider range (min=1, max=0) and trip ImGui assertions.
Address follow-up review comments on PR #28: - maxThreads now falls back to the actual compile-pool thread count (which itself has a sensible default) when hardware_concurrency() returns 0, instead of pinning users to 1 thread on platforms where the OS query fails. - compilationThreadCount and backgroundCompilationThreadCount are snapped back into [1, maxThreads] before being passed to the slider, so a stale persisted config can't render the slider in an out-of-range state. - Tooltip wording: "Intended for developing." -> "Intended for development."
Summary
CollapsingHeader("Testing")inside the Testing tab, andCollapsingHeader("Active Shaders")inside the Shader Blocking panel.docs/development/vscode-setup.mdaccordingly.New tab contents
No functional change to shader compilation, blocking, A/B, or boot toggles — purely reorganization plus the dedupe.
Test plan
ALLpreset, launch in-game, open Settings → AdvancedTableNextColumnor ImGui assertion warnings in log🤖 Generated with Claude Code
Summary by CodeRabbit
Documentation
Refactor