Issues backlog: PhoneApp onDestroyed, custom commands enhancement, employee appearances, more checks in CombatBehaviour#84
Issues backlog: PhoneApp onDestroyed, custom commands enhancement, employee appearances, more checks in CombatBehaviour#84k073l wants to merge 4 commits into
Conversation
📝 WalkthroughWalkthroughThis PR enhances console command registration, adds a type-safe employee appearance API, and improves error handling with centralized logging and component lifecycle management. The custom console registry is exposed to allow UI injection patches to register commands in the in-game command list, the new EmployeeManager provides safe access to base-game employee data, and CombatBehaviour refactors error reporting through Logger while PhoneAppButtonHandler ensures proper cleanup. ChangesCustom Console Commands UI Integration
Employee Appearance Wrapper API
Error Handling and Component Lifecycle Management
Sequence Diagram(s)sequenceDiagram
participant CommandListScreen
participant ConsolePatches
participant CustomConsoleRegistry
participant UIContainer
CommandListScreen->>ConsolePatches: S1CommandListScreen.Start (Harmony postfix)
ConsolePatches->>CustomConsoleRegistry: iterate registry.Keys
loop For each custom command
ConsolePatches->>UIContainer: instantiate command entry prefab
ConsolePatches->>UIContainer: populate Command/Description/Example text
ConsolePatches->>UIContainer: append entry to commandEntries
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ 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.
Actionable comments posted: 5
🧹 Nitpick comments (1)
S1API/Console/CustomConsoleRegistry.cs (1)
14-14: ⚡ Quick winKeep the registry encapsulated.
ConsolePatchesonly needs to enumerate commands. Making the field itselfinternallets any internal caller bypassRegister()'s trim/empty-key checks and mutate the dictionary directly. Expose a read-only view or iterator instead.Suggested change
- internal static readonly Dictionary<string, BaseConsoleCommand> registry = new Dictionary<string, BaseConsoleCommand>(StringComparer.OrdinalIgnoreCase); + private static readonly Dictionary<string, BaseConsoleCommand> registry = new Dictionary<string, BaseConsoleCommand>(StringComparer.OrdinalIgnoreCase); + internal static IReadOnlyDictionary<string, BaseConsoleCommand> Registry => registry;🤖 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/Console/CustomConsoleRegistry.cs` at line 14, The registry field is currently internal which allows bypassing Register() validations; make the dictionary private (private static readonly Dictionary<string, BaseConsoleCommand> registry) and expose a read-only view or enumerator instead (for example a public/static IReadOnlyDictionary<string, BaseConsoleCommand> or a method IEnumerable<KeyValuePair<string,BaseConsoleCommand>> GetRegisteredCommands()) so ConsolePatches can enumerate without mutating; keep Register() (and its trim/empty-key checks) as the single mutating entry point and return or expose only the read-only wrapper from the new accessor.
🤖 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 `@S1API/Entities/Behaviour/CombatBehaviour.cs`:
- Around line 92-93: The DefaultWeaponAssetPath setter currently calls
Object.Instantiate(gameObject) and then GetComponent<AvatarEquippable>, which
creates an undeleted runtime clone; instead, remove the Instantiate call and
validate the assigned prefab directly by calling
GetComponent<AvatarEquippable>() or GetComponentInChildren<AvatarEquippable>()
on the prefab reference (or this.gameObject/prefab variable used in the setter)
to check for AvatarEquippable, and keep the existing null-handling (log/throw)
path if the component is missing.
In `@S1API/Entities/Employees/EmployeeManager.cs`:
- Line 33: The wrapper must guard against nulls from the underlying API: change
the EmployeeManager appearance wrapper so that when
S1Employees.EmployeeManager.Instance.GetAppearance(male, index) returns null you
return null (or propagate appropriately) instead of always calling new
EmployeeAppearance(...); similarly update GetRandomAppearance to use the out
AvatarSettings? avatarSettings only if the base call returns true and
avatarSettings is non-null — do not always construct new
AvatarSettings(avatarSettings) or always return true; instead call
S1Employees.EmployeeManager.Instance.GetRandomAppearance(...) and if it returns
false or the out avatarSettings is null, set the wrapper out to null and return
false, otherwise construct new AvatarSettings(avatarSettings) and return true.
Ensure you reference EmployeeAppearance, EmployeeManager.Instance.GetAppearance,
GetRandomAppearance, AvatarSettings and the avatarSettings out parameter when
applying the checks.
In `@S1API/Internal/Patches/ConsolePatches.cs`:
- Line 43: The static HashSet _addedCommandsToList is being cleared only in
AddCommands() via S1Console.Awake which prevents newly created
S1CommandListScreen instances from receiving custom entries; move or add the
reset so duplicate tracking is cleared at the start of each command-list screen
creation by clearing _addedCommandsToList at the beginning of
AddCustomCommandEntries() (or change dedupe to an instance-scoped set inside
S1CommandListScreen) so each S1CommandListScreen gets its own fresh tracking and
custom keys are added correctly.
- Around line 174-189: The postfix currently adds custom command entries even
when their CommandWord collides with a built-in command; before instantiating or
adding an entry inside the foreach over CustomConsoleRegistry.registry.Keys,
check for collisions with the game's native registry (e.g., S1Console.commands
or the equivalent built-in command map) and skip entries whose key already
exists there so only executable custom commands are shown; update the logic
around CustomConsoleRegistry.registry, _addedCommandsToList, CommandEntryPrefab
and CommandEntryContainer to continue when a built-in command owns the key.
In `@S1API/PhoneApp/PhoneApp.cs`:
- Around line 651-655: OnDestroy currently unconditionally calls DestroyInternal
and can re-enter destruction when PhoneApp.OnDestroyed destroys _appPanel (which
tears down the handler); add a re-entrancy guard: introduce a private bool
(e.g., _isDestroying) on PhoneApp, set it true at the start of DestroyInternal
(or OnDestroyed) and skip any further destruction if it's already true, and
update OnDestroy to check that flag (or phoneApp != null &&
!phoneApp._isDestroying) before calling DestroyInternal to prevent double runs.
---
Nitpick comments:
In `@S1API/Console/CustomConsoleRegistry.cs`:
- Line 14: The registry field is currently internal which allows bypassing
Register() validations; make the dictionary private (private static readonly
Dictionary<string, BaseConsoleCommand> registry) and expose a read-only view or
enumerator instead (for example a public/static IReadOnlyDictionary<string,
BaseConsoleCommand> or a method
IEnumerable<KeyValuePair<string,BaseConsoleCommand>> GetRegisteredCommands()) so
ConsolePatches can enumerate without mutating; keep Register() (and its
trim/empty-key checks) as the single mutating entry point and return or expose
only the read-only wrapper from the new accessor.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ddf3d5e3-e041-4fb5-ab8d-cc937bd56ae3
📒 Files selected for processing (5)
S1API/Console/CustomConsoleRegistry.csS1API/Entities/Behaviour/CombatBehaviour.csS1API/Entities/Employees/EmployeeManager.csS1API/Internal/Patches/ConsolePatches.csS1API/PhoneApp/PhoneApp.cs
| var equippable = Object.Instantiate(gameObject).GetComponent<AvatarEquippable>(); | ||
| if (equippable == null) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Unity, does Object.Instantiate(GameObject) create a runtime scene clone that should be destroyed if used only for metadata/type checks?
💡 Result:
Yes, calling Object.Instantiate(GameObject) creates a complete, functional clone of the object hierarchy in the runtime scene [1][2]. It is an expensive operation that performs serialization and deserialization, allocates memory, registers the new objects with the engine's internal systems (physics, rendering, scripting), and triggers lifecycle methods like Awake and OnEnable [1][3]. Using Instantiate solely for metadata or type checks is strongly discouraged because: 1. Performance Impact: It incurs significant CPU and memory overhead, including Garbage Collection (GC) allocations, which can cause performance hitches [4][3]. 2. Lifecycle Side Effects: Because it creates a full clone, components' Awake and OnEnable methods will execute on the new instance, which may cause unintended side effects or errors if your code is not designed for it [1]. 3. Resource Management: If you instantiate an object for a temporary check, you must manually call Object.Destroy(clone) to remove it from the scene; otherwise, you will create a memory leak and clutter your scene with useless objects [3]. For metadata or type checks, you should access the original GameObject or prefab asset directly without instantiating it. You can access components, check types (e.g., using GetComponent or Type comparisons), and read serializable data directly from the reference without creating a runtime clone [1][2]. If you need to inspect an asset's properties that are only available at runtime, consider using Editor-time tools or custom serialization approaches that do not require full instantiation [2][5].
Citations:
- 1: https://docs.unity3d.com/ScriptReference/Object.Instantiate.html
- 2: https://docs.unity3d.com/6000.5/Documentation/Manual/script-serialization-how-unity-uses.html
- 3: https://gamedev.stackexchange.com/questions/182009/unity-performance-recycling-pooled-objects-or-re-instantiating
- 4: https://discussions.unity.com/t/instantiateasync-and-extreme-gc-usage/874242
- 5: https://gist.github.com/OscarAbraham/72a636234c2df2abe407cb6d632ca57b
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the file and show relevant surrounding code (small slice)
FILE="S1API/Entities/Behaviour/CombatBehaviour.cs"
if [ ! -f "$FILE" ]; then
echo "Missing file: $FILE" >&2
exit 1
fi
echo "---- Lines around 80-120 ----"
nl -ba "$FILE" | sed -n '70,130p'
echo "---- Search for DefaultWeaponAssetPath ----"
rg -n "DefaultWeaponAssetPath" "$FILE" || true
echo "---- Search for Instantiate(gameObject) / Instantiate( ----"
rg -n "Instantiate\s*\(" "$FILE" || true
echo "---- Search for Destroy( / DestroyImmediate( ----"
rg -n "Destroy(Immediate)?\s*\(" "$FILE" || true
echo "---- Show enclosing method for lines 92-93 ----"
# Try to find nearest method signature above the target lines
# Using awk to print from nearest earlier line containing typical method keywords.
TARGET_START=92
TARGET_END=93
nl -ba "$FILE" | awk -v s="$TARGET_START" -v e="$TARGET_END" '
NR==s {found=1}
found==1 && NR>e {exit}
{
if (found==0) {last_sig=$0}
if (NR<=e) {
# heuristics: store last line that looks like method signature
if ($0 ~ /(public|private|protected|internal)\s+.*\)\s*\{/) last_sig=$0
if ($0 ~ /void\s+\w+\s*\(.*\)\s*\{/) last_sig=$0
if ($0 ~ /\bTask\b.*\)\s*\{/) last_sig=$0
}
}
END{
print "Last likely method signature before target:\n" last_sig
}' 2>/dev/null || trueRepository: ifBars/S1API
Length of output: 148
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="S1API/Entities/Behaviour/CombatBehaviour.cs"
if [ ! -f "$FILE" ]; then
echo "Missing file: $FILE" >&2
exit 1
fi
echo "---- Lines 70-130 (with numbers via awk) ----"
awk 'NR>=70 && NR<=130 {printf "%d\t%s\n", NR, $0}' "$FILE" | sed -n '1,80p'
echo "---- Search for DefaultWeaponAssetPath ----"
rg -n "DefaultWeaponAssetPath" "$FILE" || true
echo "---- Search for Instantiate(gameObject) ----"
rg -n "Instantiate\s*\(" "$FILE" || true
echo "---- Search for Destroy( / DestroyImmediate( ----"
rg -n "Destroy(Immediate)?\s*\(" "$FILE" || trueRepository: ifBars/S1API
Length of output: 3033
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Search for any Destroy usage within the file
FILE="S1API/Entities/Behaviour/CombatBehaviour.cs"
echo "---- Destroy usage in file ----"
rg -n "Destroy(Immediate)?\s*\(" "$FILE" || true
# 2) Show the full property setter area (broader context)
echo "---- Lines 60-120 ----"
awk 'NR>=55 && NR<=120 {printf "%d\t%s\n", NR, $0}' "$FILE" | sed -n '1,120p'
# 3) Find all writes/usages of DefaultWeaponAssetPath across the repo
echo "---- Repo-wide usage of DefaultWeaponAssetPath ----"
rg -n "DefaultWeaponAssetPath\s*=" -S . || true
echo "---- Repo-wide reads/usages of DefaultWeaponAssetPath ----"
rg -n "DefaultWeaponAssetPath" -S . || trueRepository: ifBars/S1API
Length of output: 3271
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="S1API/Entities/Behaviour/CombatBehaviour.cs"
echo "---- Lines 150-220 ----"
awk 'NR>=150 && NR<=220 {printf "%d\t%s\n", NR, $0}' "$FILE"
echo "---- All occurrences of DefaultWeaponAssetPath (repo-wide) ----"
rg -n "DefaultWeaponAssetPath" -S . || trueRepository: ifBars/S1API
Length of output: 3627
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find AvatarEquippable definition
echo "---- Finding AvatarEquippable files ----"
fd -a "AvatarEquippable" . || true
# Also search for class name
rg -n "class\s+AvatarEquippable\b" -S . || true
# Show AvatarEquippable Awake/OnEnable/Start/OnDestroy if present
# (best-effort; we only need whether it self-destructs)
rg -n "AvatarEquippable.*\{|\bvoid\s+(Awake|OnEnable|Start|OnDestroy)\b|Destroy" -S . || trueRepository: ifBars/S1API
Length of output: 11307
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="S1API/Entities/Behaviour/CombatBehaviour.cs"
echo "---- Search for GetAssetPathFromEquippable ----"
rg -n "GetAssetPathFromEquippable" "$FILE" || true
echo "---- Show method(s) around GetAssetPathFromEquippable ----"
# Print a window around each match (awk line numbers)
python3 - <<'PY'
import re, pathlib
p=pathlib.Path("S1API/Entities/Behaviour/CombatBehaviour.cs")
txt=p.read_text()
lines=txt.splitlines()
pat=re.compile(r'GetAssetPathFromEquippable')
for i,l in enumerate(lines, start=1):
if pat.search(l):
start=max(1,i-20); end=min(len(lines), i+80)
print(f"\n--- window around line {i} ({start}-{end}) ---")
for j in range(start, end+1):
print(f"{j}\t{lines[j-1]}")
PY
echo "---- Search for Instantiate(gameObject) in this file ----"
rg -n "Instantiate\s*\(" "$FILE" || trueRepository: ifBars/S1API
Length of output: 11934
Avoid Instantiate in DefaultWeaponAssetPath setter (lines 92-93)
Object.Instantiate(gameObject) creates a full runtime clone every time DefaultWeaponAssetPath is set, and this code path never destroys that instance. Instead, validate the prefab asset directly via GetComponent/GetComponentInChildren to avoid extra allocations and lifecycle side effects.
Proposed fix
- var equippable = Object.Instantiate(gameObject).GetComponent<AvatarEquippable>();
+ var equippable = gameObject.GetComponent<AvatarEquippable>()
+ ?? gameObject.GetComponentInChildren<AvatarEquippable>(true);
if (equippable == null)
{
Logger.Error("Could not get AvatarEquippable from weapon at path: " + value);
return;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var equippable = Object.Instantiate(gameObject).GetComponent<AvatarEquippable>(); | |
| if (equippable == null) | |
| var equippable = gameObject.GetComponent<AvatarEquippable>() | |
| ?? gameObject.GetComponentInChildren<AvatarEquippable>(true); | |
| if (equippable == null) | |
| { | |
| Logger.Error("Could not get AvatarEquippable from weapon at path: " + value); | |
| return; | |
| } |
🤖 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/Entities/Behaviour/CombatBehaviour.cs` around lines 92 - 93, The
DefaultWeaponAssetPath setter currently calls Object.Instantiate(gameObject) and
then GetComponent<AvatarEquippable>, which creates an undeleted runtime clone;
instead, remove the Instantiate call and validate the assigned prefab directly
by calling GetComponent<AvatarEquippable>() or
GetComponentInChildren<AvatarEquippable>() on the prefab reference (or
this.gameObject/prefab variable used in the setter) to check for
AvatarEquippable, and keep the existing null-handling (log/throw) path if the
component is missing.
| return null; | ||
| } | ||
|
|
||
| return new EmployeeAppearance(S1Employees.EmployeeManager.Instance.GetAppearance(male, index)); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find all declarations/usages to confirm return types and failure conventions.
rg -nP --type=cs -C3 '\b(GetAppearance|GetRandomAppearance)\s*\('
rg -nP --type=cs -C3 '\bclass\s+EmployeeAppearance\b|\bclass\s+AvatarSettings\b'Repository: ifBars/S1API
Length of output: 3944
Fix null-safety at wrapper boundary in EmployeeManager appearance APIs
GetAppearance(...) is nullable but wraps the base-game return value directly (new EmployeeAppearance(...GetAppearance...)) without guarding against a null base result. Likewise, GetRandomAppearance(...) always returns true and always constructs new AvatarSettings(avatarSettings) even though the base out parameter (avatarSettings) may be null per the wrapper’s nullable AvatarSettings? contract.
🤖 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/Entities/Employees/EmployeeManager.cs` at line 33, The wrapper must
guard against nulls from the underlying API: change the EmployeeManager
appearance wrapper so that when
S1Employees.EmployeeManager.Instance.GetAppearance(male, index) returns null you
return null (or propagate appropriately) instead of always calling new
EmployeeAppearance(...); similarly update GetRandomAppearance to use the out
AvatarSettings? avatarSettings only if the base call returns true and
avatarSettings is non-null — do not always construct new
AvatarSettings(avatarSettings) or always return true; instead call
S1Employees.EmployeeManager.Instance.GetRandomAppearance(...) and if it returns
false or the out avatarSettings is null, set the wrapper out to null and return
false, otherwise construct new AvatarSettings(avatarSettings) and return true.
Ensure you reference EmployeeAppearance, EmployeeManager.Instance.GetAppearance,
GetRandomAppearance, AvatarSettings and the avatarSettings out parameter when
applying the checks.
| if (__instance == null) | ||
| return; | ||
|
|
||
| _addedCommandsToList.Clear(); |
There was a problem hiding this comment.
Reset duplicate tracking when the command list screen starts.
_addedCommandsToList is static, but it is cleared in AddCommands() on S1Console.Awake. If the game creates a fresh S1CommandListScreen later in the same console lifetime, every custom key is still marked as already added and the new screen gets no custom entries. Clear this set in AddCustomCommandEntries() or scope dedupe to the current screen instance.
Suggested change
- _addedCommandsToList.Clear();
var commandTypes = ReflectionUtils.GetDerivedClasses<BaseConsoleCommand>();
foreach (var type in commandTypes)
{
Logger.Msg($"Found console command: {type.FullName}");
@@
private static void AddCustomCommandEntries(S1CommandListScreen __instance)
{
try
{
+ _addedCommandsToList.Clear();
if (__instance == null || __instance.CommandEntryPrefab == null ||
__instance.CommandEntryContainer == null)
return;Also applies to: 151-164
🤖 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/ConsolePatches.cs` at line 43, The static HashSet
_addedCommandsToList is being cleared only in AddCommands() via S1Console.Awake
which prevents newly created S1CommandListScreen instances from receiving custom
entries; move or add the reset so duplicate tracking is cleared at the start of
each command-list screen creation by clearing _addedCommandsToList at the
beginning of AddCustomCommandEntries() (or change dedupe to an instance-scoped
set inside S1CommandListScreen) so each S1CommandListScreen gets its own fresh
tracking and custom keys are added correctly.
| foreach (var commandKey in CustomConsoleRegistry.registry.Keys) | ||
| { | ||
| try | ||
| { | ||
| if (_addedCommandsToList.Contains(commandKey)) | ||
| continue; | ||
| var rt = Object.Instantiate(__instance.CommandEntryPrefab, __instance.CommandEntryContainer); | ||
| rt.Find("Command").GetComponent<TextMeshProUGUI>().text = | ||
| CustomConsoleRegistry.registry[commandKey].CommandWord; | ||
| rt.Find("Description").GetComponent<TextMeshProUGUI>().text = | ||
| CustomConsoleRegistry.registry[commandKey].CommandDescription; | ||
| rt.Find("Example").GetComponent<TextMeshProUGUI>().text = | ||
| CustomConsoleRegistry.registry[commandKey].ExampleUsage; | ||
|
|
||
| commandEntries?.Add(rt); | ||
| _addedCommandsToList.Add(commandKey); |
There was a problem hiding this comment.
Don't inject rows for commands the base console will shadow.
Both routing prefixes hand execution back to S1Console.commands when a native command already owns the key, but this postfix still injects the custom entry unconditionally. A custom CommandWord that collides with a built-in command will be visible in the list even though the player can never execute it. Filter those collisions here, or reject them during registration.
🤖 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/ConsolePatches.cs` around lines 174 - 189, The postfix
currently adds custom command entries even when their CommandWord collides with
a built-in command; before instantiating or adding an entry inside the foreach
over CustomConsoleRegistry.registry.Keys, check for collisions with the game's
native registry (e.g., S1Console.commands or the equivalent built-in command
map) and skip entries whose key already exists there so only executable custom
commands are shown; update the logic around CustomConsoleRegistry.registry,
_addedCommandsToList, CommandEntryPrefab and CommandEntryContainer to continue
when a built-in command owns the key.
| private void OnDestroy() | ||
| { | ||
| // Destroy phone app when button handler is destroyed | ||
| phoneApp?.DestroyInternal(); | ||
| } |
There was a problem hiding this comment.
Prevent re-entrant destruction from handler teardown.
OnDestroy() at Line 654 unconditionally calls DestroyInternal(). When PhoneApp.OnDestroyed() (Line 188+) destroys _appPanel (Line 192), this handler is destroyed too and re-enters DestroyInternal(), causing OnDestroyed() to run twice.
Suggested guard-based fix
public abstract class PhoneApp : Registerable
{
+ private bool _isDestroying;
+ internal bool IsDestroying => _isDestroying;
...
protected override void OnDestroyed()
{
+ if (_isDestroying) return;
+ _isDestroying = true;
+
if (_appPanel != null)
{
Object.Destroy(_appPanel);
_appPanel = null;
}
...
}
}
...
internal class PhoneAppButtonHandler : MonoBehaviour
{
internal PhoneApp phoneApp;
...
private void OnDestroy()
{
- // Destroy phone app when button handler is destroyed
- phoneApp?.DestroyInternal();
+ if (phoneApp != null && !phoneApp.IsDestroying)
+ phoneApp.DestroyInternal();
}
}🤖 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/PhoneApp/PhoneApp.cs` around lines 651 - 655, OnDestroy currently
unconditionally calls DestroyInternal and can re-enter destruction when
PhoneApp.OnDestroyed destroys _appPanel (which tears down the handler); add a
re-entrancy guard: introduce a private bool (e.g., _isDestroying) on PhoneApp,
set it true at the start of DestroyInternal (or OnDestroyed) and skip any
further destruction if it's already true, and update OnDestroy to check that
flag (or phoneApp != null && !phoneApp._isDestroying) before calling
DestroyInternal to prevent double runs.
1 participant
Closes a bunch of issues: #74, #76, #83.
Also closes one issue from Trello - custom commands now appear in the command list screen, just like native commands do.
Release Notes
OnDestroy()wiring forPhoneAppButtonHandlerto properly tear down the associatedPhoneAppwhen the button handler component is destroyedEmployeeManagerandEmployeeAppearancewrapper classes with access to appearance settings and mugshotsCombatBehaviour.DefaultWeaponAssetPathsetter with safer type casting usingCrossType.Ischecks, switched to melonlogger for error reporting, and updated XML documentationCustomConsoleRegistry.registryinternal to support assembly-level access for custom command integrationContributions by Author