Skip to content

Issues backlog: PhoneApp onDestroyed, custom commands enhancement, employee appearances, more checks in CombatBehaviour #84

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
k073l wants to merge 4 commits into
base: stable
Choose a base branch
from
Open

Issues backlog: PhoneApp onDestroyed, custom commands enhancement, employee appearances, more checks in CombatBehaviour #84

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
01b05f1
fix(phoneapp): wire OnDestroyed
k073l Jun 4, 2026
d2563ce
feat(commands): add custom commands to command list
k073l Jun 4, 2026
a8c0dfa
feat(employees): expose employee appearance data
k073l Jun 4, 2026
b6197c2
fix(combat): switch to melonlogger, typecast safely, update xml docs
k073l Jun 5, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion S1API/Console/CustomConsoleRegistry.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ internal static class CustomConsoleRegistry
{
private static readonly Logging.Log Logger = new Logging.Log("Console");

private static readonly Dictionary<string, BaseConsoleCommand> registry = new Dictionary<string, BaseConsoleCommand>(StringComparer.OrdinalIgnoreCase);
internal static readonly Dictionary<string, BaseConsoleCommand> registry = new Dictionary<string, BaseConsoleCommand>(StringComparer.OrdinalIgnoreCase);

internal static void Register(BaseConsoleCommand command)
{
Expand Down
28 changes: 21 additions & 7 deletions S1API/Entities/Behaviour/CombatBehaviour.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
using UnityEngine;
using MelonLoader;
using S1API.Entities.Equippables;
using S1API.Internal.Utils;
using Object = UnityEngine.Object;

namespace S1API.Entities.Behaviour;
Expand All @@ -22,6 +23,8 @@ namespace S1API.Entities.Behaviour;
/// </summary>
public class CombatBehaviour
{
private static readonly Logging.Log Logger = new("CombatBehaviour");

/// <summary>
/// INTERNAL: NPC reference
/// </summary>
Expand Down Expand Up @@ -57,8 +60,13 @@ public float GiveUpTime
/// <summary>
/// Gets or sets the default weapon asset path for the NPC's combat behaviour.
/// This property allows you to specify the weapon that the NPC will use by default.
/// <see cref="Weapon"/> for convenience when setting this property.
/// Use <see cref="Weapon"/> or <see cref="AvatarEquippablePaths"/> for convenience when setting this property.
/// Set to an empty string to clear the default weapon.
/// </summary>
/// <remarks>
/// Using type-safe <see cref="SetDefaultWeapon(Equippable)"/>
/// or <see cref="SetDefaultWeapon(EquippableBuilder)"/> is generally preferred and advised.
/// </remarks>
public string DefaultWeaponAssetPath
{
get
Expand All @@ -74,21 +82,27 @@ public string DefaultWeaponAssetPath
return;
}

var go = Resources.Load(value) as GameObject;
if (go == null)
var go = Resources.Load(value);
if (!CrossType.Is<GameObject>(go, out var gameObject) || gameObject == null)
{
Debug.LogError("Could not find weapon at path: " + value);
Logger.Error("Could not find weapon at path: " + value);
return;
}

var equippable = Object.Instantiate(go).GetComponent<AvatarEquippable>();
var equippable = Object.Instantiate(gameObject).GetComponent<AvatarEquippable>();
if (equippable == null)
Comment on lines +92 to 93
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Jun 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

🧩 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:

🏁 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 || true

Repository: 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" || true

Repository: 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 . || true

Repository: 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 . || true

Repository: 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 . || true

Repository: 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" || true

Repository: 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.

Suggested change
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.

{
Debug.LogError("Could not get AvatarEquippable from weapon at path: " + value);
Logger.Error("Could not get AvatarEquippable from weapon at path: " + value);
return;
}

if (!CrossType.Is<AvatarWeapon>(equippable, out var avatarWeapon))
{
Logger.Error("AvatarEquippable at path is not an AvatarWeapon: " + value);
return;
}

NPC.S1NPC.Behaviour.CombatBehaviour.DefaultWeapon = equippable as AvatarWeapon;
NPC.S1NPC.Behaviour.CombatBehaviour.DefaultWeapon = avatarWeapon;
}
}

Expand Down
88 changes: 88 additions & 0 deletions S1API/Entities/Employees/EmployeeManager.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
#if IL2CPPMELON
using S1Employees = Il2CppScheduleOne.Employees;
#elif MONOMELON
using S1Employees = ScheduleOne.Employees;
#endif
using System.Collections.Generic;
using S1API.Avatar;
using UnityEngine;

namespace S1API.Entities.Employees
{
/// <summary>
/// Provides methods for managing employee appearances and related data.
/// </summary>
public static class EmployeeManager
{
private static readonly Logging.Log Logger = new("EmployeeManager");

/// <summary>
/// Gets an employee appearance by index.
/// </summary>
/// <param name="male">Whether to choose from male employee appearance pool</param>
/// <param name="index">The index of the appearance to retrieve</param>
/// <returns>An <see cref="EmployeeAppearance"/> representing the employee appearance at the specified index if successful; otherwise, null.</returns>
public static EmployeeAppearance? GetAppearance(bool male, int index)
{
if (!S1Employees.EmployeeManager.InstanceExists)
{
Logger.Error("EmployeeManager instance does not exist; cannot get appearance");
return null;
}

return new EmployeeAppearance(S1Employees.EmployeeManager.Instance.GetAppearance(male, index));
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Jun 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

🧩 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.

}

/// <summary>
/// Gets a random employee appearance.
/// </summary>
/// <param name="male">Whether to choose from male employee appearance pool</param>
/// <param name="index">The index of the appearance that was retrieved</param>
/// <param name="settings">The avatar settings of the appearance that was retrieved</param>
/// <returns>True if an appearance was successfully retrieved; otherwise, false.</returns>
public static bool GetRandomAppearance(bool male, out int index, out AvatarSettings? settings)
{
if (!S1Employees.EmployeeManager.InstanceExists)
{
settings = null;
index = -1;
Logger.Error("EmployeeManager instance does not exist; cannot get random appearance");
return false;
}

S1Employees.EmployeeManager.Instance.GetRandomAppearance(male, out var i, out var avatarSettings);
index = i;
settings = new AvatarSettings(avatarSettings);
return true;
}
}

/// <summary>
/// Represents an employee appearance, including avatar settings and mugshot sprite.
/// </summary>
public class EmployeeAppearance
{
/// <summary>
/// INTERNAL: The underlying employee appearance from the base game.
/// </summary>
internal S1Employees.EmployeeManager.EmployeeAppearance S1EmployeeAppearance;

/// <summary>
/// Gets the avatar settings associated with this employee appearance.
/// </summary>
public AvatarSettings Settings => new(S1EmployeeAppearance.Settings);

/// <summary>
/// Gets the mugshot sprite associated with this employee appearance.
/// </summary>
public Sprite Mugshot => S1EmployeeAppearance.Mugshot;

/// <summary>
/// INTERNAL: Initializes a new instance of the EmployeeAppearance class with the specified base game type appearance.
/// </summary>
internal EmployeeAppearance(S1Employees.EmployeeManager.EmployeeAppearance s1EmployeeAppearance)
{
S1EmployeeAppearance = s1EmployeeAppearance;
}
}
}
67 changes: 66 additions & 1 deletion S1API/Internal/Patches/ConsolePatches.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,16 @@
using HarmonyLib;
using S1API.Console;
using S1API.Internal.Utils;
using UnityEngine;
using Object = UnityEngine.Object;
#if (MONOMELON || MONOBEPINEX || IL2CPPBEPINEX)
using S1Console = ScheduleOne.Console;

using S1CommandListScreen = ScheduleOne.CommandListScreen;
using TMPro;
#elif (IL2CPPMELON)
using S1Console = Il2CppScheduleOne.Console;
using S1CommandListScreen = Il2CppScheduleOne.CommandListScreen;
using Il2CppTMPro;
#endif

#if (IL2CPPMELON || IL2CPPBEPINEX)
Expand All @@ -35,6 +40,7 @@ private static void AddCommands(S1Console __instance)
if (__instance == null)
return;

_addedCommandsToList.Clear();
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Jun 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

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.

var commandTypes = ReflectionUtils.GetDerivedClasses<BaseConsoleCommand>();
foreach (var type in commandTypes)
{
Expand Down Expand Up @@ -134,5 +140,64 @@ private static bool RouteCustomCommandsIl2Cpp(Il2CppSystem.Collections.Generic.L
}
}
#endif

#if (MONOMELON || MONOBEPINEX)
private static FieldInfo? _commandEntriesField;
#endif

/// <summary>
/// Custom commands that were added to command list screen, stored here to prevent duplicate additions.
/// </summary>
private static HashSet<string> _addedCommandsToList = new();

/// <summary>
/// Adds custom commands to command list screen.
/// </summary>
[HarmonyPatch(typeof(S1CommandListScreen), "Start")]
[HarmonyPostfix]
private static void AddCustomCommandEntries(S1CommandListScreen __instance)
{
try
{
if (__instance == null || __instance.CommandEntryPrefab == null ||
__instance.CommandEntryContainer == null)
return;
#if (MONOMELON || MONOBEPINEX)
_commandEntriesField ??=
typeof(S1CommandListScreen)
.GetField("commandEntries", BindingFlags.NonPublic | BindingFlags.Instance);
var commandEntries = _commandEntriesField?.GetValue(__instance) as List<RectTransform>;
#elif (IL2CPPMELON || IL2CPPBEPINEX)
var commandEntries = __instance?.commandEntries;
#endif

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);
Comment on lines +174 to +189
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Jun 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

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.

}
catch (Exception e)
{
Logger.Warning($"[Console] Failed to add command '{commandKey}' to command list screen: {e.Message}");
}
}
}
catch (Exception e)
{
Logger.Warning($"[Console] Failed to add custom commands to command list screen: {e.Message}");
}
}
}
}
6 changes: 6 additions & 0 deletions S1API/PhoneApp/PhoneApp.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -648,6 +648,12 @@ private void Update()
}
}

private void OnDestroy()
{
// Destroy phone app when button handler is destroyed
phoneApp?.DestroyInternal();
}
Comment on lines +651 to +655
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Jun 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

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.


private bool IsHoveringButton()
{
// This is the same logic as native App<T>.IsHoveringButton()
Expand Down
Loading