Switch lock type on BeforeBaseGame saveables#80
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughRefactors load-cycle tracking in GenericSaveables.Patches.cs: adds a static session-scoped ChangesSession-scoped Load-Once Lock
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
S1API/Internal/Patches/GenericSaveables.Patches.cs (1)
77-81:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDocstring is outdated.
Line 81 states "Uses the LoadedGameFolderPath to detect new load cycles" but the implementation now uses a
sameSessionflag cleared byonLoadComplete. Update to reflect the new mechanism.📝 Suggested docstring update
/// <summary> /// Loads saveables marked with BeforeBaseGame load order BEFORE base game loaders run. /// This runs as a prefix to LoadManager.QueueLoadRequest on the first LoadRequest creation, /// which happens right before base game loaders start processing. - /// Uses the LoadedGameFolderPath to detect new load cycles. + /// Uses a session flag cleared by onLoadComplete to detect new load cycles. /// </summary>🤖 Prompt for 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. In `@S1API/Internal/Patches/GenericSaveables.Patches.cs` around lines 77 - 81, Update the outdated XML docstring for the prefix that loads saveables (the method patch that runs before LoadManager.QueueLoadRequest) to remove the reference to LoadedGameFolderPath and instead state that it uses an internal sameSession flag (cleared by onLoadComplete) to detect new load cycles; mention that this runs on the first LoadRequest creation before base game loaders run and that sameSession is reset by onLoadComplete to allow subsequent load cycle detection.
🤖 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.
Outside diff comments:
In `@S1API/Internal/Patches/GenericSaveables.Patches.cs`:
- Around line 77-81: Update the outdated XML docstring for the prefix that loads
saveables (the method patch that runs before LoadManager.QueueLoadRequest) to
remove the reference to LoadedGameFolderPath and instead state that it uses an
internal sameSession flag (cleared by onLoadComplete) to detect new load cycles;
mention that this runs on the first LoadRequest creation before base game
loaders run and that sameSession is reset by onLoadComplete to allow subsequent
load cycle detection.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 666cc5a7-2076-49ea-a351-eaac9707a942
📒 Files selected for processing (1)
S1API/Internal/Patches/GenericSaveables.Patches.cs
2 participants
Switches the lock on
BeforeBaseGamesaveable loader from save path check to a flag-based system, where the lock is set on first load of all eligible saveables and released when game invokesonLoadComplete.This allows for these saveables to be correctly reloaded on another load cycle (e.g. load save1 -> exit to menu -> load save1). Previous system blocked this exact example.
Tested on Mono with MoreNPCs and MultiDelivery (regression testing and testing the fix) and on IL2CPP with the same + BigWillyMod (regression testing for custom items).
Let me know if I missed anything.
Release Notes
sameSessionboolean flag to prevent repeated execution within the same load sessionsameSessionis cleared by anonLoadCompletelistener so saveables can be reloaded after exiting to menu and reloading the same saveLoadOrder == BeforeBaseGameand missing-save initialization-on-load-complete behaviorContributions by Author