chore: add more requirements#1481
Conversation
WalkthroughThe plugin updates DLL validation lists in Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Game as Game
participant Plugin as XSEPlugin
participant FS as Filesystem
note over Plugin: Startup
Game->>Plugin: Initialize()
Plugin->>FS: Enumerate loaded DLLs
FS-->>Plugin: DLL list
rect rgba(230,240,255,0.5)
note over Plugin: Incompatible check (new: trainwreck.dll)
Plugin->>Plugin: If any incompatible DLL present
alt Incompatible found
Plugin-->>Game: Report error and abort
else None found
Plugin->>Plugin: Continue
end
end
rect rgba(235,255,235,0.5)
note over Plugin: Required check (new: CrashLogger.dll)
Plugin->>Plugin: Verify required DLLs present
alt Missing required
Plugin-->>Game: Report missing dependency
else All present
Plugin-->>Game: Initialization complete
end
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
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.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/XSEPlugin.cpp (2)
173-175: Safer incompatible-DLL detection: don’t LoadLibrary() the things you’re disablingAdding SSEReShadeHelper.dll and trainwreck.dll makes sense (SSEReShadeHelper is obsolete; trainwreck is widely discouraged). But using LoadLibrary to “detect” these will execute their DllMain and keep them loaded, risking side effects and crashes. Prefer a non-executing presence/loaded check (filesystem + GetModuleHandleW) and never load them. (nexusmods.com)
Apply this diff to avoid executing entry points and to prevent accidental loading:
@@ - for (const auto dll : incompatibleDLLs) { - if (LoadLibrary(dll)) { - auto errorMessage = std::format("Incompatible DLL {} detected", stl::utf16_to_utf8(dll).value_or("<unicode conversion error>"s)); - logger::error("{}", errorMessage); - errors.push_back(errorMessage); - } - } + for (const auto dll : incompatibleDLLs) { + std::filesystem::path p{dll}; + const bool presentOnDisk = std::filesystem::exists(p); + const bool alreadyLoaded = ::GetModuleHandleW(p.filename().c_str()) != nullptr; + if (presentOnDisk || alreadyLoaded) { + auto errorMessage = std::format("Incompatible DLL {} detected", stl::utf16_to_utf8(dll).value_or("<unicode conversion error>"s)); + logger::error("{}", errorMessage); + errors.push_back(errorMessage); + } + }Add this include (outside the changed hunk):
#include <filesystem>Also applies to: 177-183
186-188: Crash logger requirement: allow “either-or” with NetScriptFramework to avoid breaking known setupsRequiring CrashLogger.dll is reasonable (it supports SSE/AE/VR), but many modlists still use NetScriptFramework (NSF). Crash Logger explicitly states only one crash logger should be active at a time; making CrashLogger.dll hard-required will disable Community Shaders for NSF users. Suggest: require “at least one of {CrashLogger.dll, NetScriptFramework.dll}”. (nexusmods.com)
Confirm whether any supported lists in your ecosystem still ship NSF; if yes, adopt the “either-or” check below.
Apply this diff to keep Engine Fixes required as-is and gate the crash-logger requirement flexibly:
@@ - const std::array requiredDLLs = { - REL::Module::IsVR() ? L"Data/SKSE/Plugins/EngineFixesVR.dll" : L"Data/SKSE/Plugins/EngineFixes.dll", - L"Data/SKSE/Plugins/CrashLogger.dll" - }; + const std::array requiredDLLs = { + REL::Module::IsVR() ? L"Data/SKSE/Plugins/EngineFixesVR.dll" : L"Data/SKSE/Plugins/EngineFixes.dll" + }; @@ - for (const auto dll : requiredDLLs) { + for (const auto dll : requiredDLLs) { if (!LoadLibrary(dll)) { auto errorMessage = std::format("Required DLL {} was missing", stl::utf16_to_utf8(dll).value_or("<unicode conversion error>"s)); logger::error("{}", errorMessage); errors.push_back(errorMessage); } } + + // Require at least one crash logger (CrashLogger or NetScriptFramework) + { + const std::array crashLoggerCandidates = { + L"Data/SKSE/Plugins/CrashLogger.dll", + L"Data/SKSE/Plugins/NetScriptFramework.dll" + }; + bool hasLogger = false; + for (const auto dll : crashLoggerCandidates) { + std::filesystem::path p{dll}; + if (std::filesystem::exists(p) || ::GetModuleHandleW(p.filename().c_str()) != nullptr) { + hasLogger = true; + break; + } + } + if (!hasLogger) { + auto errorMessage = std::format("Required crash logger (CrashLogger.dll or NetScriptFramework.dll) was missing"); + logger::error("{}", errorMessage); + errors.push_back(errorMessage); + } + }Note: trainwreck.dll being incompatible aligns with community guidance to avoid it. (reddit.com)
Also applies to: 190-196
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/XSEPlugin.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/XSEPlugin.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/XSEPlugin.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
|
✅ A pre-release build is available for this PR: |
2 participants
Summary by CodeRabbit