Switch lock type on BeforeBaseGame saveables#80
Conversation
|
Warning Review limit reached
More reviews will be available in 52 minutes and 43 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe pull request refactors load-cycle tracking in 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
1 participant
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
BeforeBaseGamesaveables for improved reload handlingsameSessionboolean flag prevents repeated execution within the same load sessiononLoadCompletelistener, enabling proper reloading across load cycles (e.g., load save1 → exit to menu → load save1)LoadOrder == BeforeBaseGamefiltering and initialization-on-completion behavior for missing save dataContributions by Author