Draft
Conversation
Contributor
Author
|
@coderabbitai review |
Contributor
|
can this bro stop touching my draft pr pls |
Contributor
|
actually not even my pr cuz i didnt ask for creating a pr yet |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
PolyMod.csproj (1)
27-27:⚠️ Potential issue | 🟠 MajorUpgrade Scriban to patch known vulnerabilities.
Scriban 6.2.1 is vulnerable to high-severity DoS and sandbox escape issues (GHSA-wgh7-7m3c-fx25, GHSA-5rpf-x9jg-8j5p, GHSA-grr9-747v-xvcp) that were fixed in 6.6.0. Upgrade to version 7.0.3 (latest, released March 23, 2026) to resolve the NuGet audit failures (NU1902, NU1903, NU1904) currently blocking the CI/CD pipeline.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@PolyMod.csproj` at line 27, Update the Scriban package reference to a secure patched release: replace the PackageReference for "Scriban" currently set to Version="6.2.1" with Version="7.0.3" to address the reported vulnerabilities and NuGet audit failures; after updating the PackageReference, restore packages and run the test/build to catch any breaking API changes introduced between Scriban 6.x and 7.x and adjust usages of the Scriban API in the codebase if compilation errors surface.
🧹 Nitpick comments (8)
src/Multiplayer/Multiplayer.cs (5)
246-250: Exception handling wraps .NET exception into IL2CPP exception — stack trace is lost.When catching exceptions, you create a new
Il2CppSystem.Exceptionwith only the message. Consider logging the full stack trace before wrapping, which you partially do inDeserialize_Postfixbut not here.♻️ Improved error handling
catch (Exception ex) { - Plugin.logger.LogError("Multiplayer> Error during HandleStartLobbyGameModded: " + ex.Message); + Plugin.logger.LogError($"Multiplayer> Error during HandleStartLobbyGameModded: {ex.Message}\n{ex.StackTrace}"); tcs.SetException(new Il2CppSystem.Exception(ex.Message)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Multiplayer/Multiplayer.cs` around lines 246 - 250, The catch block in HandleStartLobbyGameModded currently logs only ex.Message and then wraps into a new Il2CppSystem.Exception losing the .NET stack trace; update the catch to log the full exception (e.g., ex.ToString()) via Plugin.logger.LogError and construct the Il2CppSystem.Exception with the full details (message + stack trace) before calling tcs.SetException so stack information is preserved; target the catch in Multiplayer.HandleStartLobbyGameModded where Plugin.logger.LogError(...) and tcs.SetException(...) are called.
51-59: Method nameSteamClient_get_SteamIdis misleading.This method patches
SystemInfo.deviceUniqueIdentifier, not anything Steam-related. Consider renaming to match the actual patched property.♻️ Proposed rename
- public static void SteamClient_get_SteamId(ref string __result) + public static void SystemInfo_deviceUniqueIdentifier_Postfix(ref string __result)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Multiplayer/Multiplayer.cs` around lines 51 - 59, The method name SteamClient_get_SteamId is misleading because it postfix-patches SystemInfo.deviceUniqueIdentifier; rename it to something clearly matching the patched property (for example SystemInfo_deviceUniqueIdentifier_Postfix or Patch_SystemInfo_deviceUniqueIdentifier) and update the method identifier accordingly while keeping the existing Harmony attributes ([HarmonyPostfix], [HarmonyPatch(typeof(SystemInfo), nameof(SystemInfo.deviceUniqueIdentifier), MethodType.Getter)]) and parameter signature (ref string __result) unchanged so the patch behavior remains intact.
236-236: Inconsistent JSON serializers used.Line 236 uses
System.Text.Json.JsonSerializer.Serialize()forsetupGameDataViewModel, while line 377 usesNewtonsoft.Json.JsonConvert.SerializeObject()forgameState.Settings. This inconsistency can lead to subtle serialization differences (e.g., property naming, null handling). Consider standardizing on one serializer.Also applies to: 377-377
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Multiplayer/Multiplayer.cs` at line 236, The code uses two different JSON serializers (System.Text.Json.JsonSerializer.Serialize for setupGameDataViewModel and Newtonsoft.Json.JsonConvert.SerializeObject for gameState.Settings), causing inconsistent serialization; pick one serializer across the file (either System.Text.Json or Newtonsoft.Json), then update the other call(s) to the chosen API (e.g., replace JsonSerializer.Serialize or JsonConvert.SerializeObject accordingly) and ensure any serializer-specific settings (naming policy, null handling, converters) are applied consistently where setupGameDataViewModel and gameState.Settings are serialized so the output format is uniform.
188-203:BackendAdapter_StartLobbyGame_Moddedfire-and-forgets the async task.The discarded task (
_ = HandleStartLobbyGameModded(...)) means unobserved exceptions may be swallowed silently in some runtime configurations. While exceptions are caught insideHandleStartLobbyGameModded, consider adding a continuation to log any unexpected failures.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Multiplayer/Multiplayer.cs` around lines 188 - 203, BackendAdapter_StartLobbyGame_Modded currently fire-and-forgets HandleStartLobbyGameModded which can hide unexpected exceptions; capture the returned Task from HandleStartLobbyGameModded instead of discarding it, and attach a continuation (e.g., ContinueWith or a safe fire-and-forget helper) that logs any exception and ensures taskCompletionSource is set appropriately; reference the BackendAdapter_StartLobbyGame_Modded method, the HandleStartLobbyGameModded call, the taskCompletionSource variable and the __result assignment when making this change.
22-23: GLD caches have no eviction strategy — potential memory leak.
_gldCacheand_versionCachegrow unbounded as games are played. Consider adding a size limit or using an LRU cache to prevent memory growth over long sessions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Multiplayer/Multiplayer.cs` around lines 22 - 23, The two static dictionaries _gldCache and _versionCache in Multiplayer lack eviction and can grow unbounded; replace them with a bounded/LRU strategy (for example use a thread-safe LRU cache implementation or System.Runtime.Caching/MemoryCache with size limit and eviction policy) and update all accessors that read/write these maps (references to _gldCache and _versionCache and any Get/Add methods in class Multiplayer) to use the new cache API; ensure thread-safety, set a reasonable max entries (configurable), and add unit tests or telemetry to validate eviction behavior.src/Multiplayer/ViewModels/SetupGameDataViewModel.cs (2)
7-7: Inconsistent indentation: line 7 uses tabs while other lines use spaces.This line uses tab characters for indentation while the rest of the file uses spaces. Normalize to a consistent style.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Multiplayer/ViewModels/SetupGameDataViewModel.cs` at line 7, The property declaration serializedGameState uses tab indentation while the rest of the file uses spaces; normalize the indentation by replacing the leading tab(s) on the serializedGameState property with the same number of spaces used elsewhere in the file so the declaration "public byte[] serializedGameState { get; set; }" matches the file's spacing style.
5-9: Non-nullable properties lack initialization and may cause compiler warnings.With
<Nullable>enable</Nullable>in the project, these properties should either have default values, be marked nullable, or userequired(C# 11+) to avoid CS8618 warnings.♻️ Proposed fix using `required` modifier (C# 11+)
public class SetupGameDataViewModel : IMonoServerResponseData { - public string lobbyId { get; set; } + public required string lobbyId { get; set; } - public byte[] serializedGameState { get; set; } + public required byte[] serializedGameState { get; set; } - public string gameSettingsJson { get; set; } + public required string gameSettingsJson { get; set; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Multiplayer/ViewModels/SetupGameDataViewModel.cs` around lines 5 - 9, Properties lobbyId, serializedGameState and gameSettingsJson on SetupGameDataViewModel are non-nullable but not initialized, which triggers CS8618; fix by either marking them nullable (string? lobbyId, byte[]? serializedGameState, string? gameSettingsJson), giving them safe defaults (e.g. = string.Empty or = Array.Empty<byte>()), or—if using C# 11+—add the required modifier to each property (required string lobbyId { get; set; }, required byte[] serializedGameState { get; set; }, required string gameSettingsJson { get; set; }) so callers must initialize them. Ensure you pick one consistent approach across the three properties.src/Multiplayer/ViewModels/IMonoServerResponseData.cs (1)
3-5: Interface name may be misleading.Based on the context snippets,
SetupGameDataViewModelimplements this interface but is used as a request payload (serialized and sent toStartLobbyGameModded), not as response data. The server response type isServerResponse<LobbyGameViewModel>.Consider renaming to
IMonoServerRequestDataor a more generic name likeIMonoServerDatato accurately reflect its usage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Multiplayer/ViewModels/IMonoServerResponseData.cs` around lines 3 - 5, The interface IMonoServerResponseData is misnamed because SetupGameDataViewModel (which implements it) is used as a request payload to StartLobbyGameModded rather than a server response; rename the interface to a name that reflects usage (for example IMonoServerRequestData or IMonoServerData), update the interface declaration name and all references/implementations (e.g., in SetupGameDataViewModel) and ensure any serialization or method signatures that reference IMonoServerResponseData (including the call site StartLobbyGameModded and any ServerResponse<T> usages like ServerResponse<LobbyGameViewModel>) are updated to the new name to keep types consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Multiplayer/Multiplayer.cs`:
- Line 227: Update the log message passed to Plugin.logger.LogInfo in
Multiplayer.cs to fix the typo: change "Multiplayer> GameState and Settiings
created" to "Multiplayer> GameState and Settings created" so the log reads
correctly; locate the call to Plugin.logger.LogInfo and edit the string literal
accordingly.
- Around line 28-30: In Init(), the code retrieves BuildConfig via
BuildConfigHelper.GetSelectedBuildConfig() but then forces
buildConfig.customServerURL = LOCAL_SERVER_URL, ignoring the configurable
Plugin.config.backendUrl; change the assignment to set
buildConfig.customServerURL to Plugin.config.backendUrl (with LOCAL_SERVER_URL
as a fallback if Plugin.config.backendUrl is null/empty) so the BuildConfig uses
the configured backend URL; update the reference in Init(), leaving BuildConfig
and BuildConfigHelper.GetSelectedBuildConfig() unchanged.
- Around line 292-295: Remove the stray semicolon after the object
initialization and replace the unreliable shuffling using OrderBy(x =>
Il2CppSystem.Guid.NewGuid()) when selecting a random TribeType: instead use a
single proper random source (e.g., System.Random or RandomNumberGenerator) to
pick an index or to shuffle the Enum.GetValues<TribeType>().Where(t => t !=
TribeType.None).ToArray(); then select by random index; update the code that
assigns tribe (the selection logic around the tribe variable and the OrderBy
call) to use that deterministic single-random-source selection and remove the
extraneous ';' token.
- Around line 160-175: FetchGldById currently blocks on HttpClient by calling
GetAsync().Result and ReadAsStringAsync().Result and instantiates a new
HttpClient per call; change FetchGldById to an async method (e.g. Task<string>
FetchGldByIdAsync) that awaits client.GetAsync(url).ConfigureAwait(false) and
await response.Content.ReadAsStringAsync().ConfigureAwait(false), and use a
single reused HttpClient instance (e.g. a static readonly HttpClient on the
class) instead of creating one per call; update callers (including
Deserialize_Postfix) to call the async variant (propagate async/Task where
possible) or, if changing the call chain is impossible, wrap the async work in
Task.Run and use ConfigureAwait(false) to avoid deadlocks.
---
Outside diff comments:
In `@PolyMod.csproj`:
- Line 27: Update the Scriban package reference to a secure patched release:
replace the PackageReference for "Scriban" currently set to Version="6.2.1" with
Version="7.0.3" to address the reported vulnerabilities and NuGet audit
failures; after updating the PackageReference, restore packages and run the
test/build to catch any breaking API changes introduced between Scriban 6.x and
7.x and adjust usages of the Scriban API in the codebase if compilation errors
surface.
---
Nitpick comments:
In `@src/Multiplayer/Multiplayer.cs`:
- Around line 246-250: The catch block in HandleStartLobbyGameModded currently
logs only ex.Message and then wraps into a new Il2CppSystem.Exception losing the
.NET stack trace; update the catch to log the full exception (e.g.,
ex.ToString()) via Plugin.logger.LogError and construct the
Il2CppSystem.Exception with the full details (message + stack trace) before
calling tcs.SetException so stack information is preserved; target the catch in
Multiplayer.HandleStartLobbyGameModded where Plugin.logger.LogError(...) and
tcs.SetException(...) are called.
- Around line 51-59: The method name SteamClient_get_SteamId is misleading
because it postfix-patches SystemInfo.deviceUniqueIdentifier; rename it to
something clearly matching the patched property (for example
SystemInfo_deviceUniqueIdentifier_Postfix or
Patch_SystemInfo_deviceUniqueIdentifier) and update the method identifier
accordingly while keeping the existing Harmony attributes ([HarmonyPostfix],
[HarmonyPatch(typeof(SystemInfo), nameof(SystemInfo.deviceUniqueIdentifier),
MethodType.Getter)]) and parameter signature (ref string __result) unchanged so
the patch behavior remains intact.
- Line 236: The code uses two different JSON serializers
(System.Text.Json.JsonSerializer.Serialize for setupGameDataViewModel and
Newtonsoft.Json.JsonConvert.SerializeObject for gameState.Settings), causing
inconsistent serialization; pick one serializer across the file (either
System.Text.Json or Newtonsoft.Json), then update the other call(s) to the
chosen API (e.g., replace JsonSerializer.Serialize or
JsonConvert.SerializeObject accordingly) and ensure any serializer-specific
settings (naming policy, null handling, converters) are applied consistently
where setupGameDataViewModel and gameState.Settings are serialized so the output
format is uniform.
- Around line 188-203: BackendAdapter_StartLobbyGame_Modded currently
fire-and-forgets HandleStartLobbyGameModded which can hide unexpected
exceptions; capture the returned Task from HandleStartLobbyGameModded instead of
discarding it, and attach a continuation (e.g., ContinueWith or a safe
fire-and-forget helper) that logs any exception and ensures taskCompletionSource
is set appropriately; reference the BackendAdapter_StartLobbyGame_Modded method,
the HandleStartLobbyGameModded call, the taskCompletionSource variable and the
__result assignment when making this change.
- Around line 22-23: The two static dictionaries _gldCache and _versionCache in
Multiplayer lack eviction and can grow unbounded; replace them with a
bounded/LRU strategy (for example use a thread-safe LRU cache implementation or
System.Runtime.Caching/MemoryCache with size limit and eviction policy) and
update all accessors that read/write these maps (references to _gldCache and
_versionCache and any Get/Add methods in class Multiplayer) to use the new cache
API; ensure thread-safety, set a reasonable max entries (configurable), and add
unit tests or telemetry to validate eviction behavior.
In `@src/Multiplayer/ViewModels/IMonoServerResponseData.cs`:
- Around line 3-5: The interface IMonoServerResponseData is misnamed because
SetupGameDataViewModel (which implements it) is used as a request payload to
StartLobbyGameModded rather than a server response; rename the interface to a
name that reflects usage (for example IMonoServerRequestData or
IMonoServerData), update the interface declaration name and all
references/implementations (e.g., in SetupGameDataViewModel) and ensure any
serialization or method signatures that reference IMonoServerResponseData
(including the call site StartLobbyGameModded and any ServerResponse<T> usages
like ServerResponse<LobbyGameViewModel>) are updated to the new name to keep
types consistent.
In `@src/Multiplayer/ViewModels/SetupGameDataViewModel.cs`:
- Line 7: The property declaration serializedGameState uses tab indentation
while the rest of the file uses spaces; normalize the indentation by replacing
the leading tab(s) on the serializedGameState property with the same number of
spaces used elsewhere in the file so the declaration "public byte[]
serializedGameState { get; set; }" matches the file's spacing style.
- Around line 5-9: Properties lobbyId, serializedGameState and gameSettingsJson
on SetupGameDataViewModel are non-nullable but not initialized, which triggers
CS8618; fix by either marking them nullable (string? lobbyId, byte[]?
serializedGameState, string? gameSettingsJson), giving them safe defaults (e.g.
= string.Empty or = Array.Empty<byte>()), or—if using C# 11+—add the required
modifier to each property (required string lobbyId { get; set; }, required
byte[] serializedGameState { get; set; }, required string gameSettingsJson {
get; set; }) so callers must initialize them. Ensure you pick one consistent
approach across the three properties.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 68bd4990-e2cc-462a-9d93-f1319d541190
📒 Files selected for processing (6)
.gitignorePolyMod.csprojsrc/Multiplayer/Multiplayer.cssrc/Multiplayer/ViewModels/IMonoServerResponseData.cssrc/Multiplayer/ViewModels/SetupGameDataViewModel.cssrc/Plugin.cs
src/Multiplayer/Multiplayer.cs
Outdated
Comment on lines
+28
to
+30
| BuildConfig buildConfig = BuildConfigHelper.GetSelectedBuildConfig(); | ||
| buildConfig.buildServerURL = BuildServerURL.Custom; | ||
| buildConfig.customServerURL = LOCAL_SERVER_URL; |
There was a problem hiding this comment.
Hardcoded LOCAL_SERVER_URL overrides the configurable backendUrl.
Init() sets buildConfig.customServerURL = LOCAL_SERVER_URL (localhost), ignoring Plugin.config.backendUrl. This appears unintentional since you log backendUrl on line 32 but don't use it for the build config.
🐛 Proposed fix
internal static void Init()
{
Harmony.CreateAndPatchAll(typeof(Client));
BuildConfig buildConfig = BuildConfigHelper.GetSelectedBuildConfig();
buildConfig.buildServerURL = BuildServerURL.Custom;
- buildConfig.customServerURL = LOCAL_SERVER_URL;
+ buildConfig.customServerURL = Plugin.config.backendUrl;
Plugin.logger.LogInfo($"Multiplayer> Server URL set to: {Plugin.config.backendUrl}");📝 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
| BuildConfig buildConfig = BuildConfigHelper.GetSelectedBuildConfig(); | |
| buildConfig.buildServerURL = BuildServerURL.Custom; | |
| buildConfig.customServerURL = LOCAL_SERVER_URL; | |
| BuildConfig buildConfig = BuildConfigHelper.GetSelectedBuildConfig(); | |
| buildConfig.buildServerURL = BuildServerURL.Custom; | |
| buildConfig.customServerURL = Plugin.config.backendUrl; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Multiplayer/Multiplayer.cs` around lines 28 - 30, In Init(), the code
retrieves BuildConfig via BuildConfigHelper.GetSelectedBuildConfig() but then
forces buildConfig.customServerURL = LOCAL_SERVER_URL, ignoring the configurable
Plugin.config.backendUrl; change the assignment to set
buildConfig.customServerURL to Plugin.config.backendUrl (with LOCAL_SERVER_URL
as a fallback if Plugin.config.backendUrl is null/empty) so the BuildConfig uses
the configured backend URL; update the reference in Init(), leaving BuildConfig
and BuildConfigHelper.GetSelectedBuildConfig() unchanged.
Comment on lines
+160
to
+175
| using var client = new HttpClient(); | ||
| var url = $"{Plugin.config.backendUrl.TrimEnd('/')}/api/mods/gld/{modGldVersion}"; | ||
| Plugin.logger?.LogDebug($"FetchGldById: Requesting URL: {url}"); | ||
|
|
||
| var response = client.GetAsync(url).Result; | ||
| Plugin.logger?.LogDebug($"FetchGldById: Response status: {response.StatusCode}"); | ||
|
|
||
| if (response.IsSuccessStatusCode) | ||
| { | ||
| var gld = response.Content.ReadAsStringAsync().Result; | ||
| Plugin.logger?.LogInfo($"FetchGldById: Successfully fetched mod GLD ({gld.Length} chars)"); | ||
| return gld; | ||
| } | ||
|
|
||
| var errorContent = response.Content.ReadAsStringAsync().Result; | ||
| Plugin.logger?.LogError($"FetchGldById: Failed with status {response.StatusCode}: {errorContent}"); |
There was a problem hiding this comment.
Synchronous blocking calls on HttpClient can cause deadlocks and thread pool starvation.
Using .Result on GetAsync() and ReadAsStringAsync() blocks the calling thread. In UI contexts (Unity), this can deadlock. Additionally, creating a new HttpClient per request can exhaust sockets.
♻️ Proposed fix: reuse HttpClient and make method async
+ private static readonly HttpClient _httpClient = new();
+
- private static string? FetchGldById(int modGldVersion)
+ private static async System.Threading.Tasks.Task<string?> FetchGldByIdAsync(int modGldVersion)
{
if(!allowGldMods) return null;
try
{
- using var client = new HttpClient();
var url = $"{Plugin.config.backendUrl.TrimEnd('/')}/api/mods/gld/{modGldVersion}";
Plugin.logger?.LogDebug($"FetchGldById: Requesting URL: {url}");
- var response = client.GetAsync(url).Result;
+ var response = await _httpClient.GetAsync(url);
Plugin.logger?.LogDebug($"FetchGldById: Response status: {response.StatusCode}");
if (response.IsSuccessStatusCode)
{
- var gld = response.Content.ReadAsStringAsync().Result;
+ var gld = await response.Content.ReadAsStringAsync();
Plugin.logger?.LogInfo($"FetchGldById: Successfully fetched mod GLD ({gld.Length} chars)");
return gld;
}
- var errorContent = response.Content.ReadAsStringAsync().Result;
+ var errorContent = await response.Content.ReadAsStringAsync();
Plugin.logger?.LogError($"FetchGldById: Failed with status {response.StatusCode}: {errorContent}");
}Note: This would require refactoring Deserialize_Postfix to handle async, which may be complex for a Harmony postfix. If synchronous calls are necessary, consider using ConfigureAwait(false) and wrapping in Task.Run().
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Multiplayer/Multiplayer.cs` around lines 160 - 175, FetchGldById
currently blocks on HttpClient by calling GetAsync().Result and
ReadAsStringAsync().Result and instantiates a new HttpClient per call; change
FetchGldById to an async method (e.g. Task<string> FetchGldByIdAsync) that
awaits client.GetAsync(url).ConfigureAwait(false) and await
response.Content.ReadAsStringAsync().ConfigureAwait(false), and use a single
reused HttpClient instance (e.g. a static readonly HttpClient on the class)
instead of creating one per call; update callers (including Deserialize_Postfix)
to call the async variant (propagate async/Task where possible) or, if changing
the call chain is impossible, wrap the async work in Task.Run and use
ConfigureAwait(false) to avoid deadlocks.
There was a problem hiding this comment.
@johnklipi, thanks for the heads-up! Regardless of authorship, since this code is part of the PR it's worth flagging to whoever owns it. The concern around blocking .Result calls on HttpClient and socket exhaustion from per-request HttpClient instantiation is worth addressing before merging. Feel free to pass this along to the original author if needed! 😊
| VersionManager.GameLogicDataVersion | ||
| ); | ||
|
|
||
| Plugin.logger.LogInfo("Multiplayer> GameState and Settiings created"); |
There was a problem hiding this comment.
Typo: "Settiings" should be "Settings".
✏️ Fix typo
- Plugin.logger.LogInfo("Multiplayer> GameState and Settiings created");
+ Plugin.logger.LogInfo("Multiplayer> GameState and Settings created");📝 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
| Plugin.logger.LogInfo("Multiplayer> GameState and Settiings created"); | |
| Plugin.logger.LogInfo("Multiplayer> GameState and Settings created"); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Multiplayer/Multiplayer.cs` at line 227, Update the log message passed to
Plugin.logger.LogInfo in Multiplayer.cs to fix the typo: change "Multiplayer>
GameState and Settiings created" to "Multiplayer> GameState and Settings
created" so the log reads correctly; locate the call to Plugin.logger.LogInfo
and edit the string literal accordingly.
Comment on lines
+292
to
+295
| tribe = Enum.GetValues<TribeType>().Where(t => t != TribeType.None) | ||
| .OrderBy(x => Il2CppSystem.Guid.NewGuid()).First() | ||
| }; | ||
| ; |
There was a problem hiding this comment.
Stray semicolon and non-deterministic random tribe selection.
- Line 295 has a stray semicolon.
- Using
OrderBy(x => Il2CppSystem.Guid.NewGuid())for shuffling is unreliable — GUIDs are generated per comparison, not per element, leading to inconsistent ordering. Use a proper random source.
🐛 Proposed fix
var botPlayer = new PlayerData
{
type = PlayerDataType.Bot,
state = PlayerDataFriendshipState.Accepted,
knownTribe = true,
- tribe = Enum.GetValues<TribeType>().Where(t => t != TribeType.None)
- .OrderBy(x => Il2CppSystem.Guid.NewGuid()).First()
+ tribe = Enum.GetValues<TribeType>()
+ .Where(t => t != TribeType.None)
+ .OrderBy(_ => System.Random.Shared.Next())
+ .First()
};
- ;
botPlayer.botDifficulty = (BotDifficulty)botDifficulty;📝 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
| tribe = Enum.GetValues<TribeType>().Where(t => t != TribeType.None) | |
| .OrderBy(x => Il2CppSystem.Guid.NewGuid()).First() | |
| }; | |
| ; | |
| tribe = Enum.GetValues<TribeType>() | |
| .Where(t => t != TribeType.None) | |
| .OrderBy(_ => System.Random.Shared.Next()) | |
| .First() | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Multiplayer/Multiplayer.cs` around lines 292 - 295, Remove the stray
semicolon after the object initialization and replace the unreliable shuffling
using OrderBy(x => Il2CppSystem.Guid.NewGuid()) when selecting a random
TribeType: instead use a single proper random source (e.g., System.Random or
RandomNumberGenerator) to pick an index or to shuffle the
Enum.GetValues<TribeType>().Where(t => t != TribeType.None).ToArray(); then
select by random index; update the code that assigns tribe (the selection logic
around the tribe variable and the OrderBy call) to use that deterministic
single-random-source selection and remove the extraneous ';' token.
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
3 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.
Summary by CodeRabbit
New Features
Chores