Merged
Conversation
Add a small JSON parser (ParseConfigurationName) to extract the custom name from RenamableConfigurationData JSON (handles compact and pretty-printed forms). After loading storage contents, parse and apply the Configuration.Name directly to placeableStorage so renames are restored from save data instead of relying on deferred onLoadComplete. Parsing and assignment are wrapped in try/catch with a logged warning on failure.
Ensure Dealer.HomeEvent.Building is updated when setting NPC dealer Home so the schedule system knows where to send the dealer. Adds reflection-based logic (field or property access depending on MONOMELON) in both NPCDealer.cs and NPCPrefabIdentity.cs to retrieve the HomeEvent object and set its Building member after Home is resolved. Wraps the operations in try/catch; one location logs a warning on failure, the other silently ignores exceptions. This fixes timing issues where Dealer.Awake() runs before S1API sets Home.
Add Harmony patches to refresh the DealerManagementApp dropdown when the app is opened or refreshed. This calls the internal RefreshDropdown (with reflection fallbacks for different build targets) and restores the selected dealer index to avoid stale mugshot sprites and region text for custom dealers whose properties are set after initial population. Includes platform-specific handling (IL2CPP vs Mono/BepInEx) and logs failures.
Add detailed logging and error handling to DriveToCarParkSpec: log unresolved parking lots, warn about lots with no ParkingSpot children, log vehicle creation/spawn failures and exceptions, and catch unexpected errors. Introduce VehiclesAtNoSpotLots HashSet to track vehicles assigned to lots with no spots and remove entries when vehicles are removed. Add a Harmony prefix patch on LandVehicle.SetVisible to prevent the game from hiding tracked vehicles (keeps them visible when the game would call SetVisible(false)). Also include a small build-define fix and include the schedule namespace in the land-vehicle patches.
Add reset/cleanup routines to prevent stale static state and leaked delegates across save loads and scene changes. SceneStateCleaner now invokes these resets. Changes include: - DialogueChoiceListener: added ResetState to clear expected choice and callback. - DialogueInjector: added ResetState to clear pending injections and hooked flag. - NPCDealer: added ClearStaticDelegates to clear Dealer.onDealerRecruited static field. - TimeManagerShim: added ResetDelegates to clear accumulated delegates and IL2CPP lists. - ContactsAppPatches: added ResetState to clear cached initialization flags. - NPCPatches: added ResetState to clear saved addiction map and loading dealers list. These updates prevent dead wrapper delegates, duplicated hooks, stale caches, and other cross-load leaks so systems reinitialize correctly on next load.
📝 WalkthroughWalkthroughThe PR adds comprehensive state-reset mechanisms across multiple subsystems (dialogue, contacts, NPC patches, time manager, dealer callbacks) invoked on scene unload to prevent stale references. It enhances error logging and vehicle tracking in DriveToCarParkSpec, introduces dealer management UI synchronization via Harmony patching, adds vehicle visibility guards for no-spot parking lots, and implements custom configuration name persistence in storage. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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.
Actionable comments posted: 4
🧹 Nitpick comments (3)
S1API/Internal/Patches/DealerManagementAppPatches.cs (1)
74-88:dealerscast may silently fail on IL2CPP at runtime
dealersObj as System.Collections.Generic.List<S1Economy.Dealer>(line 76) will yieldnullif the IL2CPP runtime exposesdealersas anIl2CppSystem.Collections.Generic.List<T>(or another IL2CPP wrapper collection). This would silently skip the index restoration step with no diagnostic. If the two branches are merged (see above), add a null-path log here so failures are observable.💡 Suggested guard
var dealers = dealersObj as System.Collections.Generic.List<S1Economy.Dealer>; if (dealers == null) + { + Logger.Warning($"Could not cast 'dealers' to List<Dealer>; actual type: {dealersObj?.GetType()}"); return; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@S1API/Internal/Patches/DealerManagementAppPatches.cs` around lines 74 - 88, The cast "var dealers = dealersObj as System.Collections.Generic.List<S1Economy.Dealer>"/null-path can silently fail on IL2CPP; update the block around ReflectionUtils.TryGetFieldOrProperty(instance, "dealers") to detect when dealersObj is non-null but the cast yields null, log a warning/error (including dealersObj.GetType().FullName and context like selectedDealer) so failures are observable, and keep the existing early-return behavior; also ensure similar logging is added for the dropdown path (dropdownObj / dropdown and dropdown.SetValueWithoutNotify) so both failure modes are visible at runtime.S1API/Entities/Schedule/ActionSpecs/DriveToCarParkSpec.cs (2)
247-273: Remove the redundantwrapperalias —vis already in scope.
var wrapper = v;on line 249 is a no-op alias. Usevdirectly throughout the spawn block.♻️ Proposed cleanup
- var wrapper = v; - if (wrapper != null && wrapper.S1LandVehicle != null) + if (v != null && v.S1LandVehicle != null) { try { ... - wrapper.Spawn(VehicleSpawnPosition, spawnRot); + v.Spawn(VehicleSpawnPosition, spawnRot); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@S1API/Entities/Schedule/ActionSpecs/DriveToCarParkSpec.cs` around lines 247 - 273, Remove the redundant local alias "wrapper" and use the existing variable "v" directly: delete the line that declares "var wrapper = v;" and replace subsequent references to "wrapper" in the spawn block with "v" (including the null-check against v.S1LandVehicle, the Spawn call using VehicleSpawnPosition and VehicleSpawnRotation, and any logging that references VehicleCode). Keep the try/catch and network manager logic (nm and nm.IsServer) unchanged; ensure you still check v != null && v.S1LandVehicle != null before calling v.Spawn(...).
211-215: Extract the no-spots guard into a local variable to avoid duplication.The condition
gameLot != null && (gameLot.ParkingSpots == null || gameLot.ParkingSpots.Count == 0)is evaluated identically at line 211 and again at line 284. Hoisting it once eliminates the duplication and ensures consistency.♻️ Proposed refactor
+ bool lotHasNoSpots = gameLot != null && + (gameLot.ParkingSpots == null || gameLot.ParkingSpots.Count == 0); + // Warn if the lot has no parking spots - if (gameLot != null && (gameLot.ParkingSpots == null || gameLot.ParkingSpots.Count == 0)) + if (lotHasNoSpots) { Logger.Warning(...); } ... // Track vehicles created for lots with no spots - if (gameLot != null && (gameLot.ParkingSpots == null || gameLot.ParkingSpots.Count == 0)) + if (lotHasNoSpots) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@S1API/Entities/Schedule/ActionSpecs/DriveToCarParkSpec.cs` around lines 211 - 215, Extract the repeated guard into a single local boolean to avoid duplicate evaluation: introduce a local like hasNoSpots (e.g. bool hasNoSpots = gameLot != null && (gameLot.ParkingSpots == null || gameLot.ParkingSpots.Count == 0)) inside the method in DriveToCarParkSpec where gameLot is available, then replace both occurrences of the original condition with hasNoSpots and keep the existing Logger.Warning call and any other branches intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@S1API/Entities/Schedule/ActionSpecs/DriveToCarParkSpec.cs`:
- Around line 202-205: The warning logged when ResolveGameLot() returns null
omits the wrapper's path and thus prints empty Name/GUID, making diagnostics
ambiguous; update the Logger.Warning call in DriveToCarParkSpec (where lotObj is
null after calling ResolveGameLot()) to include whether ParkingLot wrapper was
used and its object path (e.g., reference ParkingLot and its Path or wrapper
identifier) along with ParkingLotName and ParkingLotGUID so the message clearly
shows the wrapper-object path and that resolution was deferred/failed.
- Around line 314-319: DriveToCarParkSpec.VehiclesAtNoSpotLots can retain stale
references across scene reloads if OnDestroy removal fails; add
VehiclesAtNoSpotLots.Clear() inside SceneStateCleaner.ResetForSceneChange() in
the afterUnload branch (alongside the other static clears) to fully reset the
cache, and also update the XML summary on DriveToCarParkSpec to reference the
actual patch method SetVisible_Prefix instead of "LandVehicle.Park".
In `@S1API/Internal/Patches/DealerManagementAppPatches.cs`:
- Around line 35-52: The branch for IL2CPPMELON attempts a direct call to
instance.RefreshDropdown() which is a compile-time error and cannot be caught;
replace that branch with the same reflection-only approach used for the other
targets: use typeof(S1DealerManagementApp).GetMethod("RefreshDropdown",
BindingFlags.NonPublic | BindingFlags.Instance) to find the method, null-check
the returned MethodInfo, and call method.Invoke(instance, null) instead of
calling instance.RefreshDropdown() so the code compiles for IL2CPP builds.
In `@S1API/Internal/Patches/StoragePatches.cs`:
- Around line 52-85: ParseConfigurationName currently uses IndexOf('"', ...)
which fails on escaped quotes and non-string values; instead, update
ParseConfigurationName to do a small stateful scan starting at the colon after
the "Value" key: skip whitespace, check the first non-space char—if it's a
quote, iterate character-by-character collecting characters until you find a
non-escaped closing quote (treat backslash as an escape for the next char); if
it's 'n','t','f' (null/true/false) or a digit/minus, treat as non-string and
return null; on any malformed sequence return null; keep try/catch behavior and
use these checks inside ParseConfigurationName to reliably extract the string
value while handling escaped quotes and avoiding false matches.
---
Nitpick comments:
In `@S1API/Entities/Schedule/ActionSpecs/DriveToCarParkSpec.cs`:
- Around line 247-273: Remove the redundant local alias "wrapper" and use the
existing variable "v" directly: delete the line that declares "var wrapper = v;"
and replace subsequent references to "wrapper" in the spawn block with "v"
(including the null-check against v.S1LandVehicle, the Spawn call using
VehicleSpawnPosition and VehicleSpawnRotation, and any logging that references
VehicleCode). Keep the try/catch and network manager logic (nm and nm.IsServer)
unchanged; ensure you still check v != null && v.S1LandVehicle != null before
calling v.Spawn(...).
- Around line 211-215: Extract the repeated guard into a single local boolean to
avoid duplicate evaluation: introduce a local like hasNoSpots (e.g. bool
hasNoSpots = gameLot != null && (gameLot.ParkingSpots == null ||
gameLot.ParkingSpots.Count == 0)) inside the method in DriveToCarParkSpec where
gameLot is available, then replace both occurrences of the original condition
with hasNoSpots and keep the existing Logger.Warning call and any other branches
intact.
In `@S1API/Internal/Patches/DealerManagementAppPatches.cs`:
- Around line 74-88: The cast "var dealers = dealersObj as
System.Collections.Generic.List<S1Economy.Dealer>"/null-path can silently fail
on IL2CPP; update the block around
ReflectionUtils.TryGetFieldOrProperty(instance, "dealers") to detect when
dealersObj is non-null but the cast yields null, log a warning/error (including
dealersObj.GetType().FullName and context like selectedDealer) so failures are
observable, and keep the existing early-return behavior; also ensure similar
logging is added for the dropdown path (dropdownObj / dropdown and
dropdown.SetValueWithoutNotify) so both failure modes are visible at runtime.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
S1API/Dialogues/DialogueChoiceListener.csS1API/Dialogues/DialogueInjector.csS1API/Entities/NPCDealer.csS1API/Entities/Schedule/ActionSpecs/DriveToCarParkSpec.csS1API/Internal/Entities/NPCPrefabIdentity.csS1API/Internal/Lifecycle/SceneStateCleaner.csS1API/Internal/Lifecycle/TimeManagerShim.csS1API/Internal/Patches/ContactsAppPatches.csS1API/Internal/Patches/DealerManagementAppPatches.csS1API/Internal/Patches/LandVehiclePatches.csS1API/Internal/Patches/NPCPatches.csS1API/Internal/Patches/StoragePatches.cs
Comment on lines
+35
to
+52
| #if (IL2CPPMELON) | ||
| try | ||
| { | ||
| instance.RefreshDropdown(); | ||
| } | ||
| catch | ||
| { | ||
| var method = typeof(S1DealerManagementApp).GetMethod("RefreshDropdown", | ||
| BindingFlags.NonPublic | BindingFlags.Public | BindingFlags.Instance); | ||
| method?.Invoke(instance, null); | ||
| } | ||
| #elif (MONOMELON || MONOBEPINEX || IL2CPPBEPINEX) | ||
| var method = typeof(S1DealerManagementApp).GetMethod("RefreshDropdown", | ||
| BindingFlags.NonPublic | BindingFlags.Instance); | ||
| if (method == null) | ||
| return; | ||
| method.Invoke(instance, null); | ||
| #endif |
There was a problem hiding this comment.
instance.RefreshDropdown() is a compile-time error — try/catch cannot rescue it
This is the confirmed root cause of the pipeline failure. The try { instance.RefreshDropdown(); } catch { ... reflection fallback ... } pattern is fatally flawed: a catch block only handles runtime exceptions; it has no effect on a compile-time symbol-not-found error. The reflection fallback inside the catch is therefore permanently dead code.
The IL2CPPMELON branch should be collapsed into the same reflection-only call already used by the other targets.
🔧 Proposed fix — unify both branches to reflection-only
-#if (IL2CPPMELON)
- try
- {
- instance.RefreshDropdown();
- }
- catch
- {
- var method = typeof(S1DealerManagementApp).GetMethod("RefreshDropdown",
- BindingFlags.NonPublic | BindingFlags.Public | BindingFlags.Instance);
- method?.Invoke(instance, null);
- }
-#elif (MONOMELON || MONOBEPINEX || IL2CPPBEPINEX)
+#if (IL2CPPMELON || MONOMELON || MONOBEPINEX || IL2CPPBEPINEX)
var method = typeof(S1DealerManagementApp).GetMethod("RefreshDropdown",
BindingFlags.NonPublic | BindingFlags.Instance);
if (method == null)
+ {
+ Logger.Warning("RefreshDropdown method not found on S1DealerManagementApp");
return;
+ }
method.Invoke(instance, null);
`#endif`🧰 Tools
🪛 GitHub Actions: IL2CPP Build Check
[error] 38-38: Build failed: DealerManagementApp does not contain a definition for 'RefreshDropdown' and no accessible extension method 'RefreshDropdown' accepting a first argument of type 'DealerManagementApp' could be found (are you missing a using directive or an assembly reference?). The failing command was: 'dotnet build S1API/S1API.csproj --no-restore -c Il2CppMelon -v normal'.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@S1API/Internal/Patches/DealerManagementAppPatches.cs` around lines 35 - 52,
The branch for IL2CPPMELON attempts a direct call to instance.RefreshDropdown()
which is a compile-time error and cannot be caught; replace that branch with the
same reflection-only approach used for the other targets: use
typeof(S1DealerManagementApp).GetMethod("RefreshDropdown",
BindingFlags.NonPublic | BindingFlags.Instance) to find the method, null-check
the returned MethodInfo, and call method.Invoke(instance, null) instead of
calling instance.RefreshDropdown() so the code compiles for IL2CPP builds.
ifBars
approved these changes
Feb 24, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Bug Fixes & Stability Improvements
Summary
Changes
Reset static state on scene save/reload
Adds
SceneStateCleanerandTimeManagerShimto properly clear static caches (dialogue listeners, injectors, NPC dealers, contacts, NPC patches) when scenes are saved or reloaded.Improve DriveToCarPark logging and visibility fix
Reworks
DriveToCarParkSpecwith improved debug logging and fixes a bug where land vehicles could become invisible viaLandVehiclePatches.Patch DealerManagementApp dropdown
Adds
DealerManagementAppPatchesto fix the dealer management dropdown not populating correctly for modded NPC dealers.Fix dealers home location
Introduces
NPCPrefabIdentityto correctly resolve dealer home locations, fixing misrouted NPCs.Restore storage configuration name from save
Adds
StoragePatchesto restore the storage configuration name that was being lost when loading a saved game.Test plan
Release Notes
ResetState()methods toDialogueChoiceListenerandDialogueInjectorto clear static state across save/reload cycles, preventing stale listeners and pending injectionsClearStaticDelegates()inNPCDealerto prevent accumulation of dead wrapper delegates on scene changesNPCDealer.Homesetter to updateDealer.HomeEvent.Buildingfield via reflection, ensuring correct NPC routing and preventing home location mismatchesVehiclesAtNoSpotLotstracking to prevent in-game hiding of vehicles assigned to parking lots without spotsLandVehicle.SetVisibleto keep tracked vehicles visible and preserve their stateDealerManagementAppPatchesto refresh and repopulate the dealer dropdown on open/refresh with proper selected index restoration, supporting modded NPC dealersStoragePatcheswith JSON parsing to extract and reapplyRenamableConfigurationDatanames that were lost on save loadSceneStateCleanerto reset all static caches on scene save/reload including dialogue, contacts, NPC patches, time manager, and dealer stateResetDelegates()method inTimeManagerShimto clear previously registered sleep and hour-pass callbacksResetState()toContactsAppPatchesto reinitialize on scene transitionsContribution Summary