Decoupling UI and Logic#1
Merged
Merged
Conversation
Changed IBotbase and IPlugin Interface Decoupled Chat Plugin
Subviews still have to be decoupled
Removed Pickepet Timer from UI and created new one in TrainingManager.cs
Added headless prototyp
Improved decoupling for Party Plugin
Egezenn
reviewed
Apr 9, 2026
|
|
||
| [Option("launch-clientless", Required = false, HelpText = "Start clientless")] | ||
| public bool LaunchClientless { get; set; } | ||
| [Option('h', "headless", Required = false, HelpText = "Start the bot without graphical user interface.")] |
Member
There was a problem hiding this comment.
This is ambigious, standard is to reserve -h for help.
We already have the help messages, it was just not specified for this letter😅.
Member
Author
There was a problem hiding this comment.
Yeah this was just a quick test will change that once I'm done! 😂
Thanks for pointing it out.
Member
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a headless CLI startup path and BotCL orchestrator; splits UI contracts into view interfaces (IBotbaseView/IPluginView); separates plugins into Plugin + View + Manager; refactors botbases to remove UI from core; updates assembly loading for headless mode; centralizes many managers (General, Party, Skills, Training). ChangesHeadless startup, core bootstrap, and plugin/view split
Sequence Diagram(s)sequenceDiagram
rect rgba(0,128,255,0.5)
User->>Program: start with --headless
Program->>BotCL: Initialize(profile)
end
rect rgba(0,200,100,0.5)
BotCL->>PluginManager: LoadAssemblies(isHeadless=true)
PluginManager->>BotCL: instantiate IPlugin (skip IPluginView)
BotCL->>EventManager: Fire plugin & botbase load events
end
rect rgba(255,128,0,0.5)
Program->>Console: REPL loop (commands)
Console->>CLIManager: Execute(command, args)
CLIManager->>Kernel.Bot: start / stop
CLIManager->>GeneralPlugin: start-client / show/hide
CLIManager->>ChatPlugin: chat -> ChatManager.Send(...)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
Botbases/RSBot.Trade/TradeBase.cs (1)
36-43:⚠️ Potential issue | 🟠 MajorRemove
MessageBox.Showfrom botbase logic to prevent headless execution failures.Line 36-41 contains a WinForms UI dialog in the
IBotbase.Start()method. This re-couples the logic layer to WinForms and will block or crash headless/non-UI execution contexts. Replace the dialog with logging and allow the state change to propagate throughKernel.Bot.Stop().Codebase pattern for error notification in botbase logic:
- MessageBox.Show( - "The active character can't trade goods! Make sure that you have the correct job and suite equiped.", - "Can't trade", - MessageBoxButtons.OK, - MessageBoxIcon.Error - ); + Log.Notify("[Trade] Active character can't trade goods. Ensure the correct job and suit are equipped."); Kernel.Bot.Stop();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Botbases/RSBot.Trade/TradeBase.cs` around lines 36 - 43, Remove the UI dialog call MessageBox.Show from the IBotbase.Start() implementation (TradeBase.cs) and replace it with a non-UI error report and return flow: log the error via the project's logging facility (e.g., use the same logger used elsewhere or Kernel's logger) with a clear message that the active character can't trade, then call Kernel.Bot.Stop() to propagate state change; ensure no WinForms references remain in IBotbase.Start()/TradeBase and that the method exits after logging so headless contexts are not blocked by UI.Plugins/RSBot.Chat/Views/Main.cs (1)
14-19:⚠️ Potential issue | 🟡 MinorDisabling
CheckForIllegalCrossThreadCallsmasks threading issues.Setting
CheckForIllegalCrossThreadCalls = false(line 16) suppresses cross-thread operation exceptions rather than handling them properly. SinceOnMessageReceivedevents may fire from non-UI threads, consider usingInvoke/BeginInvokeinAppendMessageto marshal UI updates to the correct thread.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Plugins/RSBot.Chat/Views/Main.cs` around lines 14 - 19, Remove the CheckForIllegalCrossThreadCalls = false line from the Main constructor and marshal UI updates instead: update AppendMessage to check InvokeRequired and use Invoke/BeginInvoke to perform UI changes on the UI thread (so OnMessageReceived handlers wired in SubscribeEvents will call AppendMessage safely); keep SubscribeEvents as-is but ensure any event handlers that call AppendMessage rely on its marshaling logic.Botbases/RSBot.Training/Views/Main.Designer.cs (1)
897-897:⚠️ Potential issue | 🟡 MinorRemove unused field
radioStand.The field declared at line 897 is not referenced anywhere in the codebase and should be removed as dead code.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Botbases/RSBot.Training/Views/Main.Designer.cs` at line 897, Remove the dead field declaration for radioStand: delete the private SDUI.Controls.Radio radioStand; declaration from the Main.Designer.cs and ensure no residual references remain (search for "radioStand" in InitializeComponent, Dispose, or any designer-generated arrays); if any leftover references exist, remove them as well so the partial class no longer expects that member.Plugins/RSBot.General/Views/Main.cs (1)
110-130:⚠️ Potential issue | 🟡 MinorConsider adding disposal check before
BeginInvoke.If the control is disposed before the 5-second delay completes (e.g., user closes the window quickly after launch),
BeginInvokemay throw anObjectDisposedException.🛡️ Add disposal guard
if (!string.IsNullOrEmpty(Kernel.LaunchMode)) { Task.Run(async () => { await Task.Delay(5000); + if (IsDisposed || Disposing) + return; if (Kernel.LaunchMode == "client") { BeginInvoke(new Action(() => { btnStartClient_Click(btnStartClient, EventArgs.Empty); })); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Plugins/RSBot.General/Views/Main.cs` around lines 110 - 130, The Task.Run callback can call BeginInvoke after a 5s delay even if the form/control was closed, causing ObjectDisposedException; before calling BeginInvoke in both Kernel.LaunchMode branches, guard with the control disposal/handle checks (e.g., verify !this.IsDisposed && !this.Disposing && this.IsHandleCreated) and only call btnStartClient_Click or btnStartClientless_Click via BeginInvoke when those checks pass (use the same guard in both places referencing Kernel.LaunchMode, BeginInvoke, btnStartClient_Click and btnStartClientless_Click).
🧹 Nitpick comments (14)
Library/RSBot.Core/Plugins/IBotbaseView.cs (1)
9-40: Polish XML docs for clarity and consistency.Line 9 (“Gets internal the name.”) and Line 17/34/40 wording can be tightened for readability and consistency with the new contract.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Library/RSBot.Core/Plugins/IBotbaseView.cs` around lines 9 - 40, The XML documentation for the IBotbaseView interface contains awkward and inconsistent phrasing; update the doc comments for the properties and member to be clear and consistent with the contract: change "Gets internal the name." to something like "Gets the internal name." for the Name property, reword DisplayName and TabText summaries to consistently describe their intended UI roles (e.g., "Gets the display label shown in the botbase ComboBox." and "Gets the text shown on the TabPage."), and clarify the View member summary to "Gets the UI control for this view."; adjust the Translate summary line to match the new concise style so all docs for Name, DisplayName, TabText, View (and any following Translate comment) read clearly and consistently.Library/RSBot.Core/Plugins/IPluginView.cs (1)
8-50: XML comments should match getter-only contract.Line 8, Line 17, Line 25, Line 34, and Line 44 say “Gets or sets”, but these properties are getter-only. Updating to “Gets” will avoid API confusion.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Library/RSBot.Core/Plugins/IPluginView.cs` around lines 8 - 50, Update the XML doc summaries for the getter-only properties InternalName, DisplayName, DisplayAsTab, Index, and RequireIngame so they read "Gets" (not "Gets or sets") and adjust any related wording (e.g., "Gets the name" / "Gets a value indicating whether...") to match the read-only contract in the IPluginView interface; ensure the <summary> and corresponding <value> descriptions consistently reflect that these properties are get-only.Library/RSBot.Core/Client/ReferenceManager.cs (1)
52-125: Extract shared load steps to a single private routine.The two overloads now duplicate the same ordered load pipeline. Centralizing that sequence will prevent drift and future parity bugs.
Refactor sketch
+ private void LoadCoreData() + { + LoadClientInfo(); + LoadMapInfo(); + LoadTextData(); + LoadCharacterData(); + LoadItemData(); + LoadSkillData(); + LoadQuestData(); + LoadShopData(); + LoadTeleportData(); + LoadAlchemyData(); + LoadOptLevelData(); + LoadLevelData(); + LoadEventRewardData(); + } + public void Load(int languageTab, BackgroundWorker worker) { LanguageTab = languageTab; //until language wizard is reworked? var sw = Stopwatch.StartNew(); @@ - LoadClientInfo(); - LoadMapInfo(); - ... - LoadEventRewardData(); + LoadCoreData(); @@ public void Load() { var sw = Stopwatch.StartNew(); - LoadClientInfo(); - LoadMapInfo(); - ... - LoadEventRewardData(); + LoadCoreData();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Library/RSBot.Core/Client/ReferenceManager.cs` around lines 52 - 125, Extract the duplicated sequence of data-loading calls into a new private helper (e.g., private void LoadAllData(BackgroundWorker worker = null)) that contains the ordered calls LoadClientInfo(), LoadMapInfo(), LoadTextData(), LoadCharacterData(), LoadItemData(), LoadSkillData(), LoadQuestData(), LoadShopData(), LoadTeleportData(), LoadAlchemyData(), LoadOptLevelData(), LoadLevelData(), LoadEventRewardData(); inside that helper, if worker != null emit the same worker.ReportProgress steps before each corresponding load so the worker overload keeps progress updates; then simplify Load(int, BackgroundWorker) and Load() to start/stop the Stopwatch, call the new LoadAllData(worker) (passing the worker from the overload or null), keep Log.Debug/GetDebugInfo, Log.Notify and EventManager.FireEvent as before.Botbases/RSBot.Training/TrainingManager.cs (1)
12-19: Timer lifecycle should be explicitly disposed.Line 12-19 and Line 40-50 start recurring timer work, but no disposal path is present. Please add cleanup to avoid orphaned callbacks.
♻️ Proposed refactor
-public class TrainingManager +public class TrainingManager : IDisposable { private readonly Timer _pickPetTimer = new(200); public TrainingManager() { _pickPetTimer.Elapsed += OnPickPetTimerElapsed; _pickPetTimer.AutoReset = true; _pickPetTimer.Start(); } + public void Dispose() + { + _pickPetTimer.Elapsed -= OnPickPetTimerElapsed; + _pickPetTimer.Stop(); + _pickPetTimer.Dispose(); + }Also applies to: 40-50
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Botbases/RSBot.Training/TrainingManager.cs` around lines 12 - 19, The recurring Timer _pickPetTimer started in the TrainingManager constructor lacks a disposal path; implement IDisposable on TrainingManager, add a Dispose method that stops the timer, unsubscribes OnPickPetTimerElapsed from _pickPetTimer.Elapsed, and calls _pickPetTimer.Dispose(), and ensure any other timers started in the class (the ones referenced around lines 40-50) receive the same stop/unsubscribe/dispose treatment so callbacks are not orphaned; also ensure consumers call Dispose or use a using pattern.Botbases/RSBot.Alchemy/AlchemyBase.cs (1)
13-13: MakeIsActivenull-safe for botbase transitions.Line 13 directly dereferences
Kernel.Bot.Botbase.Name; using null-safe access avoids transition-timeNullReferenceExceptionrisk.💡 Proposed refactor
- public static bool IsActive => Kernel.Bot.Running && Kernel.Bot.Botbase.Name == "RSBot.Alchemy"; + public static bool IsActive => + Kernel.Bot.Running && string.Equals(Kernel.Bot.Botbase?.Name, "RSBot.Alchemy", StringComparison.Ordinal);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Botbases/RSBot.Alchemy/AlchemyBase.cs` at line 13, The IsActive property dereferences Kernel.Bot.Botbase.Name and can throw during botbase transitions; change IsActive to perform null-safe checks on Kernel.Bot and Kernel.Bot.Botbase (e.g., use null-conditional access and compare the Name safely) and ensure the Running check is evaluated null-safely so IsActive only returns true when Kernel.Bot exists, Running is true, and the Botbase Name equals "RSBot.Alchemy".Plugins/RSBot.Map/MapPlugin.cs (1)
15-18: KeepIPluginfree of direct view calls to finish decoupling.Line 17 still reaches into
Views.View.Instancefrom plugin logic. Consider raising an event (or moving this step intoMapView) so UI initialization stays in the view layer.Possible direction
public void OnLoadCharacter() { - Views.View.Instance?.InitUniqueObjects(); + // Raise a map-character-loaded event and let MapView react. + // EventManager.FireEvent("OnMapCharacterLoaded"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Plugins/RSBot.Map/MapPlugin.cs` around lines 15 - 18, MapPlugin.OnLoadCharacter currently calls Views.View.Instance?.InitUniqueObjects(), coupling IPlugin to the UI; remove that direct view access and instead either raise a domain event (e.g., CharacterLoaded or MapInitialized) from MapPlugin.OnLoadCharacter that MapView subscribes to, or move the InitUniqueObjects invocation into MapView (e.g., handle CharacterLoaded in MapView and call InitUniqueObjects there). Update MapPlugin.OnLoadCharacter to emit the event or trigger the existing plugin lifecycle signal and ensure MapView listens and performs the UI initialization, leaving MapPlugin free of any Views.View.Instance references.Library/RSBot.Core/BotCL.cs (2)
7-18: Consider adding error handling for initialization failures.The initialization sequence has no try-catch blocks. If any step fails (e.g.,
Game.ReferenceManager.Load()orLoadAssemblies()), the application will crash without meaningful feedback to the user in headless mode.♻️ Suggested improvement with basic error handling
public static void Initialize(string profile) { + try + { ProfileManager.SetSelectedProfile(profile); GlobalConfig.Load(); Kernel.Initialize(); Game.Initialize(); Game.InitializeArchiveFiles(); Game.ReferenceManager.Load(); Kernel.PluginManager.LoadAssemblies(true); Kernel.BotbaseManager.LoadAssemblies(true); LoadExtensions(); + } + catch (Exception ex) + { + Log.Error($"Failed to initialize BotCL: {ex.Message}"); + throw; + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Library/RSBot.Core/BotCL.cs` around lines 7 - 18, Wrap the entire initialization sequence in BotCL.Initialize (around calls like ProfileManager.SetSelectedProfile, GlobalConfig.Load, Kernel.Initialize, Game.Initialize, Game.InitializeArchiveFiles, Game.ReferenceManager.Load, Kernel.PluginManager.LoadAssemblies, Kernel.BotbaseManager.LoadAssemblies, and LoadExtensions) in a try/catch that catches Exception, logs a clear error message and the exception (use the project's logger if available, otherwise Console.Error), and exit with a non‑zero status (e.g., Environment.Exit(1)) or perform any available cleanup; ensure the catch includes the exception details (ex.Message and ex.StackTrace) so headless runs get meaningful feedback.
15-16: Clarify the boolean parameter meaning with a named argument or comment.The
trueparameter passed toLoadAssemblies()is ambiguous. Consider using a named argument for clarity.- Kernel.PluginManager.LoadAssemblies(true); - Kernel.BotbaseManager.LoadAssemblies(true); + Kernel.PluginManager.LoadAssemblies(isHeadless: true); + Kernel.BotbaseManager.LoadAssemblies(isHeadless: true);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Library/RSBot.Core/BotCL.cs` around lines 15 - 16, The two calls Kernel.PluginManager.LoadAssemblies(true) and Kernel.BotbaseManager.LoadAssemblies(true) pass an ambiguous boolean; open the LoadAssemblies method signature to get the parameter name and replace the literal with a named argument (e.g., Kernel.PluginManager.LoadAssemblies(recursive: true)) or, if you cannot change callsite, add a short inline comment clarifying the intent (e.g., // true = include dependencies) so the meaning of the boolean for LoadAssemblies is explicit.Plugins/RSBot.General/Views/Main.cs (2)
30-30: Remove commented-out code.The commented-out
CheckForIllegalCrossThreadCalls = falseis good to disable (it was a bad practice), but the comment should be removed entirely rather than left in the codebase.🧹 Remove the commented line
public Main() { - //CheckForIllegalCrossThreadCalls = false; - InitializeComponent(); SubscribeEvents(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Plugins/RSBot.General/Views/Main.cs` at line 30, Remove the leftover commented-out line "CheckForIllegalCrossThreadCalls = false;" from Main.cs to clean up dead code; locate the comment inside the Main form/view class (in Plugins/RSBot.General/Views/Main.cs) and delete that single commented line so only active code remains.
554-569: Inconsistent access pattern between static and instance methods.
KillClient()is called statically viaGeneralManager.KillClient(), butStartClientAsync()is called viaGeneralPlugin.Instance.Manager.StartClientAsync(). Consider makingStartClientAsyncstatic as well for consistency, or document why the instance method is required.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Plugins/RSBot.General/Views/Main.cs` around lines 554 - 569, The access is inconsistent: KillClient() is static on GeneralManager but StartClientAsync() is called on GeneralPlugin.Instance.Manager; make StartClientAsync static as well so you can call GeneralManager.StartClientAsync() to match KillClient(), or add a static wrapper on GeneralManager that forwards to the existing instance method; update the StartClientAsync declaration (and any internal instance references) to be static or add the static wrapper and replace the call site (currently using GeneralPlugin.Instance.Manager.StartClientAsync()) with GeneralManager.StartClientAsync().Plugins/RSBot.General/GeneralManager.cs (2)
225-243: RedundantTask.Runwrapping async code.
Task.Runis unnecessary here since the method is alreadyasync TaskandHandleRegionalAuthis async. This adds thread pool overhead without benefit.♻️ Remove unnecessary Task.Run
public static async Task StartClientlessAsync() { - await Task.Run(async () => - { - if (!Game.Clientless) - { - Game.Clientless = true; - Log.StatusLang("StartingClientless"); - EventManager.FireEvent("OnClientlessProcessStarted"); - - var userAuthenticated = await HandleRegionalAuth(); - - if (userAuthenticated) - { - Game.Start(); - } - } - }); + if (Game.Clientless) + return; + + Game.Clientless = true; + Log.StatusLang("StartingClientless"); + EventManager.FireEvent("OnClientlessProcessStarted"); + + var userAuthenticated = await HandleRegionalAuth(); + + if (userAuthenticated) + { + Game.Start(); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Plugins/RSBot.General/GeneralManager.cs` around lines 225 - 243, The StartClientlessAsync method unnecessarily wraps its async logic in Task.Run, causing needless thread-pool scheduling; remove the Task.Run wrapper and make the method execute its body directly (keep the checks and side-effects: set Game.Clientless, call Log.StatusLang("StartingClientless"), EventManager.FireEvent("OnClientlessProcessStarted")), then await HandleRegionalAuth() and call Game.Start() if it returns true so the method remains properly asynchronous without extra threading overhead.
245-255:Task.Runfor synchronous code is inconsistent with other patterns.
DisconnectAsyncusesTask.Runfor synchronous operations whileStartClientAsyncdoesn't. Either document why offloading is needed here, or make the pattern consistent across methods.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Plugins/RSBot.General/GeneralManager.cs` around lines 245 - 255, DisconnectAsync currently wraps synchronous operations in Task.Run unlike StartClientAsync; remove the unnecessary offload and make the async pattern consistent by performing the operations directly and returning a completed Task (or mark method synchronous if appropriate). Locate DisconnectAsync and replace the Task.Run block so it executes Game.Clientless = false, EventManager.FireEvent("OnAutoReloginOngoing") and Kernel.Proxy.Shutdown() inline, then return Task.CompletedTask (or change signature to void/Task as per project convention) to match StartClientAsync's pattern.Botbases/RSBot.Training/TrainingBase.cs (1)
36-41: Consider checking if loop is already running before starting.
Bundles.Loop.Start()is called when distance > 80, but immediately after, there's a check ifBundles.Loop.Runningto return early. IfStart()is idempotent this is fine, but consider reordering for clarity.♻️ Reorder checks for clarity
//Begin the loopback if needed - if (Container.Bot.Area.Position.DistanceToPlayer() > 80) - Bundles.Loop.Start(); - if (Bundles.Loop.Running) return; + + if (Container.Bot.Area.Position.DistanceToPlayer() > 80) + { + Bundles.Loop.Start(); + return; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Botbases/RSBot.Training/TrainingBase.cs` around lines 36 - 41, The current logic calls Bundles.Loop.Start() when Container.Bot.Area.Position.DistanceToPlayer() > 80 but only checks Bundles.Loop.Running afterwards; reorder to check Bundles.Loop.Running first (if Bundles.Loop.Running return) and then call Bundles.Loop.Start() when distance > 80, or alternatively make/ensure Bundles.Loop.Start() is idempotent so calling it while running is safe; locate this in TrainingBase.cs around the loop-start logic referencing Container.Bot.Area.Position.DistanceToPlayer(), Bundles.Loop.Start(), and Bundles.Loop.Running and apply the reorder or idempotency safeguard.Plugins/RSBot.Skills/SkillsManager.cs (1)
110-154: Magic number indices reduce maintainability.The switch statement uses hardcoded indices 0-9 to map to
MonsterRarityenum values. This is fragile and hard to maintain. Consider using an array or dictionary to map indices to enum values.♻️ Use a mapping array
+ private static readonly MonsterRarity[] _rarityMapping = new[] + { + MonsterRarity.General, + MonsterRarity.Champion, + MonsterRarity.Giant, + MonsterRarity.GeneralParty, + MonsterRarity.ChampionParty, + MonsterRarity.GiantParty, + MonsterRarity.Elite, + MonsterRarity.EliteStrong, + MonsterRarity.Unique, + MonsterRarity.Event + }; public static void ApplyAttackSkills() { foreach (var collection in SkillManager.Skills.Values) collection.Clear(); for (var i = 0; i < _numMonsterTypes; i++) { var skillIds = PlayerConfig.GetArray<uint>("RSBot.Skills.Attacks_" + i); + var rarity = _rarityMapping[i]; foreach (var skillId in skillIds) { var skillInfo = Game.Player.Skills.GetSkillInfoById(skillId); if (skillInfo == null) continue; - switch (i) - { - case 1: - SkillManager.Skills[MonsterRarity.Champion].Add(skillInfo); - continue; - // ... other cases - } + SkillManager.Skills[rarity].Add(skillInfo); } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Plugins/RSBot.Skills/SkillsManager.cs` around lines 110 - 154, The loop in SkillsManager that reads PlayerConfig.GetArray<uint>("RSBot.Skills.Attacks_" + i) uses hardcoded magic indices in a switch to choose MonsterRarity—replace that switch with a clear index->enum mapping (e.g., a static MonsterRarity[] or Dictionary<int,MonsterRarity> defined on SkillsManager) keyed by the loop index (_numMonsterTypes) and defaulting to MonsterRarity.General; then lookup the target rarity for each i and do SkillManager.Skills[targetRarity].Add(skillInfo) (keep the existing null check for skillInfo) so the mapping is centralized and maintainable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Application/RSBot/Program.cs`:
- Around line 128-132: The current Console.ReadLine() parsing uses
.ToLower().Split(',') so commands with spaces (e.g., "start client") and
inconsistent input formats are brittle; update the parsing in Program.cs where
input, command, and args are created (the Console.ReadLine()?.ToLower() call and
subsequent input/command/args variables) to split on whitespace (or both commas
and whitespace) with StringSplitOptions.RemoveEmptyEntries (e.g., use
.Split(new[] {' ', ','}, StringSplitOptions.RemoveEmptyEntries) or Regex.Split
to normalize input), then recompute command = input[0] and args =
input.Skip(1).ToArray(); apply the same change to the other parsing block
referenced (lines ~145-148) so both handlers expect the same input format and
consider adding a "help" command handler to document the expected format.
- Around line 124-168: The loop uses a local bool running that is never changed,
so add an "exit"/"quit" command branch in the switch that gracefully stops
services and breaks the loop; specifically, add cases for "exit" and "quit" to
call Kernel.Bot?.Stop(), attempt to stop or await
GeneralPlugin.Instance.Manager.StopClientAsync() (or call a synchronous stop if
available), set running = false (or call Environment.Exit(0) if immediate
termination is desired) so the while (running) loop will terminate and resources
are cleaned up before exiting.
- Around line 170-184: HandleChatCommand currently returns silently when
args.Length < 2; update it to inform the user and exit early by writing a clear
error/usage message to Console (e.g., indicate required arguments and valid
ChatType values) instead of returning silently. Locate the HandleChatCommand
method, validate args and the Enum.TryParse for ChatType, and when arguments are
insufficient write a concise Console.WriteLine explaining expected usage
(mention ChatType, message, optional receiver) before returning so callers know
why ChatManager.Send was not invoked.
- Line 146: GeneralPlugin.Instance.Manager.StartClientAsync() is being invoked
fire-and-forget which can swallow exceptions; change the call in Program (the
spot calling StartClientAsync) to either await the Task by making the caller
async (await GeneralPlugin.Instance.Manager.StartClientAsync()) or capture the
returned Task and attach proper error handling (e.g., var t =
GeneralPlugin.Instance.Manager.StartClientAsync(); t.ContinueWith(tx => log
exception if tx.IsFaulted) or explicitly try/catch around an awaited call) so
any exceptions from StartClientAsync are observed and logged.
In `@Application/RSBot/Views/Main.cs`:
- Around line 171-184: The code checks newBotbase.Value but not
newBotbaseView.Value, causing a possible null reference when calling
newBotbaseView.Value.Translate() and accessing View/Name/Text; update the logic
around the Kernel.BotbaseManager.BotsViews lookup (newBotbaseView) to verify
newBotbaseView.Value is not null before using it, and if it is null log a clear
error (similar to the existing Log.Error for newBotbase) and return early;
ensure you reference newBotbaseView.Value.Translate(),
newBotbaseView.Value.View, newBotbaseView.Value.Name and
LanguageManager.GetLangBySpecificKey only after the null check.
In `@Botbases/RSBot.Training/TrainingBase.cs`:
- Around line 64-68: The current check only tests
Kernel.Bot.Botbase.Area.Position.X == 0 which can incorrectly treat valid
positions with X==0 as unconfigured; update the validation in TrainingBase to
verify the full position (e.g., check Kernel.Bot.Botbase.Area.Position.X == 0 &&
Kernel.Bot.Botbase.Area.Position.Y == 0 && Kernel.Bot.Botbase.Area.Position.Z ==
0) or, better, use a dedicated configured flag/property (e.g.,
Area.IsConfigured) if available; then call
Log.WarnLang("ConfigureTrainingAreaBeforeStartBot") and Kernel.Bot.Stop() only
when the entire position is the origin or IsConfigured is false.
- Around line 15-17: The Area property currently dereferences Container.Bot
directly and can throw if Container.Bot is null; update the Area getter (public
Area Area => Container.Bot.Area) to use a null-safe accessor so it returns
Container.Bot?.Area or a sensible default when Container.Bot is null, and ensure
any callers handle a possible null Area; locate this in TrainingBase (the Area
property) and keep Register() and Start() logic unchanged but remove the unsafe
direct access to Container.Bot to match the null checks used elsewhere (e.g.,
ConfigSubscriber.cs).
In `@Botbases/RSBot.Training/TrainingManager.cs`:
- Around line 42-43: The guard in TrainingManager.cs is inverted: change the
condition so the method returns only when the bot is not running or the game is
not ready; replace the current check "if (Kernel.Bot.Running || !Game.Ready)
return;" with "if (!Kernel.Bot.Running || !Game.Ready) return;" so active bot
operation no longer short-circuits pet pickup logic (referencing
Kernel.Bot.Running and Game.Ready).
In `@Library/RSBot.Core/Plugins/BotbaseManager.cs`:
- Line 32: LoadAssemblies currently instantiates every IBotbaseView even when
isHeadless is true; change it to avoid constructing view instances up front by
discovering and storing the view Type (or a lazy factory) instead of new-ing
IBotbaseView when isHeadless; update the logic in LoadAssemblies and the other
spots mentioned (the blocks that populate BotsViews and the code around the
IBotbaseView handling at 81-106) to record Type or Func<IBotbaseView> and only
Instantiate/create the IBotbaseView when actually needed (i.e., when !isHeadless
or when a view is first shown), leaving static initializers/constructors
untouched during headless runs.
- Around line 81-90: Move the Assembly.LoadFrom call into the per-assembly try
block so a bad/missing-dependency DLL is caught and skipped instead of aborting
startup: inside GetExtensionsFromAssembly, wrap the Assembly.LoadFrom(file) and
subsequent assembly.GetTypes() in the existing try/catch that already handles
"invalid botbase" so that exceptions from LoadFrom are caught locally; ensure
the method still returns empty bots/views when an assembly fails and does not
rethrow so LoadAssemblies can continue processing other files.
In `@Library/RSBot.Core/Plugins/PluginManager.cs`:
- Line 33: LoadAssemblies currently only avoids storing views but
GetExtensionsFromAssembly still constructs every IPluginView via
Activator.CreateInstance(type), which causes WinForms initialization in headless
mode; modify GetExtensionsFromAssembly (and any place using
Activator.CreateInstance for types implementing IPluginView) to accept the
isHeadless flag (propagate from LoadAssemblies) and skip instantiation/creation
of IPluginView instances entirely when isHeadless is true (instead only record
metadata or skip view types), ensuring no Activator.CreateInstance(type) runs
for IPluginView implementations under headless mode.
In `@Plugins/RSBot.Chat/Bundle/Network/ChatResponse.cs`:
- Line 56: The EventManager.FireEvent calls in ChatResponse.cs are hardcoding
ChatType.All which drops the original GM chat type; locate the two FireEvent
invocations (where ChatType.All is passed) and replace the hardcoded enum with
the actual chat type variable used by the method (preserve ChatType.AllGM when
present) so the original chat type (e.g., chatType or the method's type
parameter) is forwarded to EventManager.FireEvent instead of ChatType.All.
In `@Plugins/RSBot.Chat/ChatManager.cs`:
- Around line 11-20: In Send (Plugins.RSBot.Chat.ChatManager.Send) add
validation for ChatType.Private so you do not call Bundle.Chat.SendChatPacket or
PlayerConfig.Set when receiver is null/empty; specifically, check if type ==
ChatType.Private and string.IsNullOrWhiteSpace(receiver) and bail out (or
log/throw) before attempting to send or set "RSBot.Chat.LastWhisper" to avoid
creating invalid packets/config state.
In `@Plugins/RSBot.CommandCenter/Components/Command/StartCommandExecutor.cs`:
- Line 15: Guard access to BotbaseView when building the notification string to
avoid a NullReferenceException: update the expression used in
Game.ShowNotification (currently Game.ShowNotification($"[RSBot] Starting bot
[{Kernel.Bot?.BotbaseView.DisplayName}]") in StartCommandExecutor.cs) to use a
null-safe access for BotbaseView (e.g. Kernel.Bot?.BotbaseView?.DisplayName) or
substitute a default display name when BotbaseView is null; make the identical
change in StopCommandExecutor.cs where Game.ShowNotification uses
Kernel.Bot?.BotbaseView.DisplayName.
In `@Plugins/RSBot.General/GeneralManager.cs`:
- Around line 81-152: OnGatewayServerDisconnected is an async void handler that
can throw unhandled exceptions — wrap the entire method body in a try/catch that
catches Exception, logs the error (including exception details) and ensures any
cleanup/sequence state (e.g. leaving _reloginSeq logic untouched) still occurs;
locate the method OnGatewayServerDisconnected and add the try { /* existing body
*/ } catch (Exception ex) { Log.Error/Log.Exception("OnGatewayServerDisconnected
failed", ex); /* optionally EventManager.FireEvent("OnError") or similar */ } so
exceptions from HandleRegionalAuth, StartClientProcess, ClientManager.Kill, or
other awaited calls do not crash the process.
- Around line 153-166: The OnEnterGame method spins on while (!Game.Ready) with
no timeout; add a timeout/cancellation guard (e.g., max wait of ~30s or
configurable via GlobalConfig) so the loop stops if Game.Ready never becomes
true: implement a timed loop using a CancellationToken/Stopwatch and await
Task.Delay(...) inside it, and if the timeout elapses log an error and return
(or abort the startup) instead of spinning; also consider changing the signature
from async void OnEnterGame to async Task OnEnterGame for better error
propagation and testability.
- Around line 43-80: Change OnAgentServerDisconnected from async void to async
Task and add a top-level try-catch that logs exceptions (use Log.Error) around
the entire method body; update any event invocation to call it as a
fire-and-forget if they cannot await (e.g. _ = OnAgentServerDisconnected();) so
exceptions are observed; keep existing logic (Kernel.Bot.Stop,
ClientManager.Kill, EventManager.FireEvent, Interlocked/Volatile _reloginSeq
checks, HandleRegionalAuth, StartClientProcess) intact but ensure unhandled
exceptions from those calls are caught and logged.
In `@Plugins/RSBot.Quest/Views/Main.cs`:
- Around line 24-25: The code currently adds View.SidebarElement with
EventManager.FireEvent("OnAddSidebarElement") and subscribes handlers in
SubscribeEvents() but never removes/unsubscribes on teardown; update Dispose()
in Main to mirror setup by (1) unsubscribing every event handler registered by
SubscribeEvents() (remove the same delegates/handlers you added there), and (2)
remove the sidebar element by either firing a symmetric
EventManager.FireEvent("OnRemoveSidebarElement", View.SidebarElement) or
directly removing View.SidebarElement from its parent container (e.g.,
parent.Controls.Remove(View.SidebarElement)); ensure Dispose() nulls
View.SidebarElement or marks it disposed after removal to avoid duplicates on
Main recreation.
In `@Plugins/RSBot.Skills/SkillsManager.cs`:
- Around line 79-101: UpdateMastery blocks the calling thread by using
Thread.Sleep and unsafely toggles _isUpdatingMastery; make UpdateMastery async
(Task-returning) and replace Thread.Sleep(500) with await Task.Delay(500), call
LearnMasteryHandler.LearnMastery appropriately from the async method (or wrap it
in Task.Run if it is blocking), and protect _isUpdatingMastery using a
thread-safe mechanism (e.g., Interlocked.CompareExchange or a private
SemaphoreSlim) to prevent concurrent runs started by events like OnSpUpdated.
---
Outside diff comments:
In `@Botbases/RSBot.Trade/TradeBase.cs`:
- Around line 36-43: Remove the UI dialog call MessageBox.Show from the
IBotbase.Start() implementation (TradeBase.cs) and replace it with a non-UI
error report and return flow: log the error via the project's logging facility
(e.g., use the same logger used elsewhere or Kernel's logger) with a clear
message that the active character can't trade, then call Kernel.Bot.Stop() to
propagate state change; ensure no WinForms references remain in
IBotbase.Start()/TradeBase and that the method exits after logging so headless
contexts are not blocked by UI.
In `@Botbases/RSBot.Training/Views/Main.Designer.cs`:
- Line 897: Remove the dead field declaration for radioStand: delete the private
SDUI.Controls.Radio radioStand; declaration from the Main.Designer.cs and ensure
no residual references remain (search for "radioStand" in InitializeComponent,
Dispose, or any designer-generated arrays); if any leftover references exist,
remove them as well so the partial class no longer expects that member.
In `@Plugins/RSBot.Chat/Views/Main.cs`:
- Around line 14-19: Remove the CheckForIllegalCrossThreadCalls = false line
from the Main constructor and marshal UI updates instead: update AppendMessage
to check InvokeRequired and use Invoke/BeginInvoke to perform UI changes on the
UI thread (so OnMessageReceived handlers wired in SubscribeEvents will call
AppendMessage safely); keep SubscribeEvents as-is but ensure any event handlers
that call AppendMessage rely on its marshaling logic.
In `@Plugins/RSBot.General/Views/Main.cs`:
- Around line 110-130: The Task.Run callback can call BeginInvoke after a 5s
delay even if the form/control was closed, causing ObjectDisposedException;
before calling BeginInvoke in both Kernel.LaunchMode branches, guard with the
control disposal/handle checks (e.g., verify !this.IsDisposed && !this.Disposing
&& this.IsHandleCreated) and only call btnStartClient_Click or
btnStartClientless_Click via BeginInvoke when those checks pass (use the same
guard in both places referencing Kernel.LaunchMode, BeginInvoke,
btnStartClient_Click and btnStartClientless_Click).
---
Nitpick comments:
In `@Botbases/RSBot.Alchemy/AlchemyBase.cs`:
- Line 13: The IsActive property dereferences Kernel.Bot.Botbase.Name and can
throw during botbase transitions; change IsActive to perform null-safe checks on
Kernel.Bot and Kernel.Bot.Botbase (e.g., use null-conditional access and compare
the Name safely) and ensure the Running check is evaluated null-safely so
IsActive only returns true when Kernel.Bot exists, Running is true, and the
Botbase Name equals "RSBot.Alchemy".
In `@Botbases/RSBot.Training/TrainingBase.cs`:
- Around line 36-41: The current logic calls Bundles.Loop.Start() when
Container.Bot.Area.Position.DistanceToPlayer() > 80 but only checks
Bundles.Loop.Running afterwards; reorder to check Bundles.Loop.Running first (if
Bundles.Loop.Running return) and then call Bundles.Loop.Start() when distance >
80, or alternatively make/ensure Bundles.Loop.Start() is idempotent so calling
it while running is safe; locate this in TrainingBase.cs around the loop-start
logic referencing Container.Bot.Area.Position.DistanceToPlayer(),
Bundles.Loop.Start(), and Bundles.Loop.Running and apply the reorder or
idempotency safeguard.
In `@Botbases/RSBot.Training/TrainingManager.cs`:
- Around line 12-19: The recurring Timer _pickPetTimer started in the
TrainingManager constructor lacks a disposal path; implement IDisposable on
TrainingManager, add a Dispose method that stops the timer, unsubscribes
OnPickPetTimerElapsed from _pickPetTimer.Elapsed, and calls
_pickPetTimer.Dispose(), and ensure any other timers started in the class (the
ones referenced around lines 40-50) receive the same stop/unsubscribe/dispose
treatment so callbacks are not orphaned; also ensure consumers call Dispose or
use a using pattern.
In `@Library/RSBot.Core/BotCL.cs`:
- Around line 7-18: Wrap the entire initialization sequence in BotCL.Initialize
(around calls like ProfileManager.SetSelectedProfile, GlobalConfig.Load,
Kernel.Initialize, Game.Initialize, Game.InitializeArchiveFiles,
Game.ReferenceManager.Load, Kernel.PluginManager.LoadAssemblies,
Kernel.BotbaseManager.LoadAssemblies, and LoadExtensions) in a try/catch that
catches Exception, logs a clear error message and the exception (use the
project's logger if available, otherwise Console.Error), and exit with a
non‑zero status (e.g., Environment.Exit(1)) or perform any available cleanup;
ensure the catch includes the exception details (ex.Message and ex.StackTrace)
so headless runs get meaningful feedback.
- Around line 15-16: The two calls Kernel.PluginManager.LoadAssemblies(true) and
Kernel.BotbaseManager.LoadAssemblies(true) pass an ambiguous boolean; open the
LoadAssemblies method signature to get the parameter name and replace the
literal with a named argument (e.g.,
Kernel.PluginManager.LoadAssemblies(recursive: true)) or, if you cannot change
callsite, add a short inline comment clarifying the intent (e.g., // true =
include dependencies) so the meaning of the boolean for LoadAssemblies is
explicit.
In `@Library/RSBot.Core/Client/ReferenceManager.cs`:
- Around line 52-125: Extract the duplicated sequence of data-loading calls into
a new private helper (e.g., private void LoadAllData(BackgroundWorker worker =
null)) that contains the ordered calls LoadClientInfo(), LoadMapInfo(),
LoadTextData(), LoadCharacterData(), LoadItemData(), LoadSkillData(),
LoadQuestData(), LoadShopData(), LoadTeleportData(), LoadAlchemyData(),
LoadOptLevelData(), LoadLevelData(), LoadEventRewardData(); inside that helper,
if worker != null emit the same worker.ReportProgress steps before each
corresponding load so the worker overload keeps progress updates; then simplify
Load(int, BackgroundWorker) and Load() to start/stop the Stopwatch, call the new
LoadAllData(worker) (passing the worker from the overload or null), keep
Log.Debug/GetDebugInfo, Log.Notify and EventManager.FireEvent as before.
In `@Library/RSBot.Core/Plugins/IBotbaseView.cs`:
- Around line 9-40: The XML documentation for the IBotbaseView interface
contains awkward and inconsistent phrasing; update the doc comments for the
properties and member to be clear and consistent with the contract: change "Gets
internal the name." to something like "Gets the internal name." for the Name
property, reword DisplayName and TabText summaries to consistently describe
their intended UI roles (e.g., "Gets the display label shown in the botbase
ComboBox." and "Gets the text shown on the TabPage."), and clarify the View
member summary to "Gets the UI control for this view."; adjust the Translate
summary line to match the new concise style so all docs for Name, DisplayName,
TabText, View (and any following Translate comment) read clearly and
consistently.
In `@Library/RSBot.Core/Plugins/IPluginView.cs`:
- Around line 8-50: Update the XML doc summaries for the getter-only properties
InternalName, DisplayName, DisplayAsTab, Index, and RequireIngame so they read
"Gets" (not "Gets or sets") and adjust any related wording (e.g., "Gets the
name" / "Gets a value indicating whether...") to match the read-only contract in
the IPluginView interface; ensure the <summary> and corresponding <value>
descriptions consistently reflect that these properties are get-only.
In `@Plugins/RSBot.General/GeneralManager.cs`:
- Around line 225-243: The StartClientlessAsync method unnecessarily wraps its
async logic in Task.Run, causing needless thread-pool scheduling; remove the
Task.Run wrapper and make the method execute its body directly (keep the checks
and side-effects: set Game.Clientless, call
Log.StatusLang("StartingClientless"),
EventManager.FireEvent("OnClientlessProcessStarted")), then await
HandleRegionalAuth() and call Game.Start() if it returns true so the method
remains properly asynchronous without extra threading overhead.
- Around line 245-255: DisconnectAsync currently wraps synchronous operations in
Task.Run unlike StartClientAsync; remove the unnecessary offload and make the
async pattern consistent by performing the operations directly and returning a
completed Task (or mark method synchronous if appropriate). Locate
DisconnectAsync and replace the Task.Run block so it executes Game.Clientless =
false, EventManager.FireEvent("OnAutoReloginOngoing") and
Kernel.Proxy.Shutdown() inline, then return Task.CompletedTask (or change
signature to void/Task as per project convention) to match StartClientAsync's
pattern.
In `@Plugins/RSBot.General/Views/Main.cs`:
- Line 30: Remove the leftover commented-out line
"CheckForIllegalCrossThreadCalls = false;" from Main.cs to clean up dead code;
locate the comment inside the Main form/view class (in
Plugins/RSBot.General/Views/Main.cs) and delete that single commented line so
only active code remains.
- Around line 554-569: The access is inconsistent: KillClient() is static on
GeneralManager but StartClientAsync() is called on
GeneralPlugin.Instance.Manager; make StartClientAsync static as well so you can
call GeneralManager.StartClientAsync() to match KillClient(), or add a static
wrapper on GeneralManager that forwards to the existing instance method; update
the StartClientAsync declaration (and any internal instance references) to be
static or add the static wrapper and replace the call site (currently using
GeneralPlugin.Instance.Manager.StartClientAsync()) with
GeneralManager.StartClientAsync().
In `@Plugins/RSBot.Map/MapPlugin.cs`:
- Around line 15-18: MapPlugin.OnLoadCharacter currently calls
Views.View.Instance?.InitUniqueObjects(), coupling IPlugin to the UI; remove
that direct view access and instead either raise a domain event (e.g.,
CharacterLoaded or MapInitialized) from MapPlugin.OnLoadCharacter that MapView
subscribes to, or move the InitUniqueObjects invocation into MapView (e.g.,
handle CharacterLoaded in MapView and call InitUniqueObjects there). Update
MapPlugin.OnLoadCharacter to emit the event or trigger the existing plugin
lifecycle signal and ensure MapView listens and performs the UI initialization,
leaving MapPlugin free of any Views.View.Instance references.
In `@Plugins/RSBot.Skills/SkillsManager.cs`:
- Around line 110-154: The loop in SkillsManager that reads
PlayerConfig.GetArray<uint>("RSBot.Skills.Attacks_" + i) uses hardcoded magic
indices in a switch to choose MonsterRarity—replace that switch with a clear
index->enum mapping (e.g., a static MonsterRarity[] or
Dictionary<int,MonsterRarity> defined on SkillsManager) keyed by the loop index
(_numMonsterTypes) and defaulting to MonsterRarity.General; then lookup the
target rarity for each i and do SkillManager.Skills[targetRarity].Add(skillInfo)
(keep the existing null check for skillInfo) so the mapping is centralized and
maintainable.
🪄 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 Plus
Run ID: 1c069f29-2ea3-4f2d-a2e7-b3b9c727f7ba
📒 Files selected for processing (93)
Application/RSBot/Program.csApplication/RSBot/RSBot.csprojApplication/RSBot/Views/Main.csBotbases/RSBot.Alchemy/AlchemyBase.csBotbases/RSBot.Alchemy/AlchemyView.csBotbases/RSBot.Alchemy/Bundle/Attribute/AttributeBundle.csBotbases/RSBot.Alchemy/Bundle/Enhance/EnhanceBundle.csBotbases/RSBot.Alchemy/Bundle/Magic/MagicBundle.csBotbases/RSBot.Alchemy/Subscriber/AlchemyEventsSubscriber.csBotbases/RSBot.Lure/LureBotbase.csBotbases/RSBot.Lure/LureView.csBotbases/RSBot.Trade/Bundle/AttackBundle.csBotbases/RSBot.Trade/Bundle/RouteBundle.csBotbases/RSBot.Trade/TradeBase.csBotbases/RSBot.Trade/TradeManager.csBotbases/RSBot.Trade/TradeView.csBotbases/RSBot.Training/Bootstrap.csBotbases/RSBot.Training/Container.csBotbases/RSBot.Training/TrainingBase.csBotbases/RSBot.Training/TrainingManager.csBotbases/RSBot.Training/TrainingView.csBotbases/RSBot.Training/Views/Main.Designer.csBotbases/RSBot.Training/Views/Main.csBotbases/RSBot.Training/Views/Main.resxBotbases/RSBot.Training/Views/View.csLibrary/RSBot.Core/Bot.csLibrary/RSBot.Core/BotCL.csLibrary/RSBot.Core/Client/ReferenceManager.csLibrary/RSBot.Core/Plugins/BotbaseManager.csLibrary/RSBot.Core/Plugins/IBotbase.csLibrary/RSBot.Core/Plugins/IBotbaseView.csLibrary/RSBot.Core/Plugins/IPlugin.csLibrary/RSBot.Core/Plugins/IPluginView.csLibrary/RSBot.Core/Plugins/PluginManager.csPlugins/RSBot.Chat/Bootstrap.csPlugins/RSBot.Chat/Bundle/Chat.csPlugins/RSBot.Chat/Bundle/Network/ChatResponse.csPlugins/RSBot.Chat/ChatManager.csPlugins/RSBot.Chat/ChatPlugin.csPlugins/RSBot.Chat/ChatView.csPlugins/RSBot.Chat/Views/Main.csPlugins/RSBot.CommandCenter/Components/Command/StartCommandExecutor.csPlugins/RSBot.CommandCenter/Components/Command/StopCommandExecutor.csPlugins/RSBot.General/Bootstrap.csPlugins/RSBot.General/GeneralManager.csPlugins/RSBot.General/GeneralPlugin.csPlugins/RSBot.General/GeneralView.csPlugins/RSBot.General/Views/Main.csPlugins/RSBot.Inventory/InventoryManager.csPlugins/RSBot.Inventory/InventoryPlugin.csPlugins/RSBot.Inventory/InventoryView.csPlugins/RSBot.Inventory/Views/Main.csPlugins/RSBot.Items/Bootstrap.csPlugins/RSBot.Items/ItemsManager.csPlugins/RSBot.Items/ItemsPlugin.csPlugins/RSBot.Items/ItemsView.csPlugins/RSBot.Log/Bootstrap.csPlugins/RSBot.Log/LogManager.csPlugins/RSBot.Log/LogPlugin.csPlugins/RSBot.Log/LogView.csPlugins/RSBot.Map/Bootstrap.csPlugins/RSBot.Map/MapManager.csPlugins/RSBot.Map/MapPlugin.csPlugins/RSBot.Map/MapView.csPlugins/RSBot.Party/Bootstrap.csPlugins/RSBot.Party/PartyManager.csPlugins/RSBot.Party/PartyPlugin.csPlugins/RSBot.Party/PartyView.csPlugins/RSBot.Protection/Bootstrap.csPlugins/RSBot.Protection/Components/Player/StatPointsHandler.csPlugins/RSBot.Protection/ProtectionManager.csPlugins/RSBot.Protection/ProtectionPlugin.csPlugins/RSBot.Protection/ProtectionView.csPlugins/RSBot.Python/Bootstrap.csPlugins/RSBot.Python/PythonManager.csPlugins/RSBot.Python/PythonPlugin.csPlugins/RSBot.Python/PythonView.csPlugins/RSBot.Quest/QuestManager.csPlugins/RSBot.Quest/QuestPlugin.csPlugins/RSBot.Quest/QuestView.csPlugins/RSBot.Quest/Views/Main.csPlugins/RSBot.ServerInfo/ServerInfo.csPlugins/RSBot.ServerInfo/ServerInfoPlugin.csPlugins/RSBot.ServerInfo/ServerInfoView.csPlugins/RSBot.Skills/Bootstrap.csPlugins/RSBot.Skills/SkillsManager.csPlugins/RSBot.Skills/SkillsPlugin.csPlugins/RSBot.Skills/SkillsView.csPlugins/RSBot.Skills/Views/Main.csPlugins/RSBot.Statistics/Bootstrap.csPlugins/RSBot.Statistics/StatisticsManager.csPlugins/RSBot.Statistics/StatisticsPlugin.csPlugins/RSBot.Statistics/StatisticsView.cs
💤 Files with no reviewable changes (16)
- Botbases/RSBot.Training/Views/Main.resx
- Botbases/RSBot.Training/Container.cs
- Plugins/RSBot.Log/Bootstrap.cs
- Plugins/RSBot.Map/Bootstrap.cs
- Plugins/RSBot.General/Bootstrap.cs
- Plugins/RSBot.Party/Bootstrap.cs
- Library/RSBot.Core/Plugins/IPlugin.cs
- Plugins/RSBot.Chat/Bootstrap.cs
- Library/RSBot.Core/Plugins/IBotbase.cs
- Plugins/RSBot.ServerInfo/ServerInfo.cs
- Plugins/RSBot.Skills/Bootstrap.cs
- Plugins/RSBot.Protection/Bootstrap.cs
- Plugins/RSBot.Statistics/Bootstrap.cs
- Plugins/RSBot.Items/Bootstrap.cs
- Botbases/RSBot.Training/Bootstrap.cs
- Plugins/RSBot.Python/Bootstrap.cs
Comment on lines
+170
to
+184
| private static void HandleChatCommand(string[] args) | ||
| { | ||
| if (args.Length < 2) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| if (!Enum.TryParse<ChatType>(args[0], true, out var type)) return; | ||
|
|
||
| var message = args[1]; | ||
| var receiver = args.Length > 2 ? args[2] : null; | ||
|
|
||
| ChatManager.Send(type, message, receiver); | ||
| Console.WriteLine($"Message ({type}): {message}."); | ||
| } |
There was a problem hiding this comment.
Silent failure when chat arguments are insufficient.
HandleChatCommand returns silently without informing the user when fewer than 2 arguments are provided. This makes debugging command issues difficult.
♻️ Proposed fix to add user feedback
private static void HandleChatCommand(string[] args)
{
if (args.Length < 2)
{
+ Console.WriteLine("Usage: chat,<type>,<message>[,<receiver>]");
+ Console.WriteLine("Types: All, Party, Guild, Union, Private, Stall, Global");
return;
}
- if (!Enum.TryParse<ChatType>(args[0], true, out var type)) return;
+ if (!Enum.TryParse<ChatType>(args[0], true, out var type))
+ {
+ Console.WriteLine($"Invalid chat type: {args[0]}");
+ 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
| private static void HandleChatCommand(string[] args) | |
| { | |
| if (args.Length < 2) | |
| { | |
| return; | |
| } | |
| if (!Enum.TryParse<ChatType>(args[0], true, out var type)) return; | |
| var message = args[1]; | |
| var receiver = args.Length > 2 ? args[2] : null; | |
| ChatManager.Send(type, message, receiver); | |
| Console.WriteLine($"Message ({type}): {message}."); | |
| } | |
| private static void HandleChatCommand(string[] args) | |
| { | |
| if (args.Length < 2) | |
| { | |
| Console.WriteLine("Usage: chat,<type>,<message>[,<receiver>]"); | |
| Console.WriteLine("Types: All, Party, Guild, Union, Private, Stall, Global"); | |
| return; | |
| } | |
| if (!Enum.TryParse<ChatType>(args[0], true, out var type)) | |
| { | |
| Console.WriteLine($"Invalid chat type: {args[0]}"); | |
| return; | |
| } | |
| var message = args[1]; | |
| var receiver = args.Length > 2 ? args[2] : null; | |
| ChatManager.Send(type, message, receiver); | |
| Console.WriteLine($"Message ({type}): {message}."); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Application/RSBot/Program.cs` around lines 170 - 184, HandleChatCommand
currently returns silently when args.Length < 2; update it to inform the user
and exit early by writing a clear error/usage message to Console (e.g., indicate
required arguments and valid ChatType values) instead of returning silently.
Locate the HandleChatCommand method, validate args and the Enum.TryParse for
ChatType, and when arguments are insufficient write a concise Console.WriteLine
explaining expected usage (mention ChatType, message, optional receiver) before
returning so callers know why ChatManager.Send was not invoked.
Comment on lines
171
to
+184
| var newBotbase = Kernel.BotbaseManager.Bots.FirstOrDefault(bot => bot.Value.Name == name); | ||
| var newBotbaseView = Kernel.BotbaseManager.BotsViews.FirstOrDefault(bot => bot.Value.Name == name); | ||
| if (newBotbase.Value == null) | ||
| { | ||
| Log.Error($"Botbase [{name}] could not be found!"); | ||
|
|
||
| return; | ||
| } | ||
|
|
||
| newBotbase.Value.Translate(); | ||
| newBotbaseView.Value.Translate(); | ||
|
|
||
| var control = newBotbase.Value.View; | ||
| control.Name = newBotbase.Value.Name; | ||
| control.Text = LanguageManager.GetLangBySpecificKey(newBotbase.Value.Name, "TabText", newBotbase.Value.TabText); | ||
| var control = newBotbaseView.Value.View; | ||
| control.Name = newBotbaseView.Value.Name; | ||
| control.Text = LanguageManager.GetLangBySpecificKey(newBotbaseView.Value.Name, "TabText", newBotbaseView.Value.TabText); |
There was a problem hiding this comment.
Potential null reference if botbase view is not found.
newBotbaseView.Value could be null if the botbase exists in Bots but not in BotsViews. The null check on line 173 only validates newBotbase.Value, leaving newBotbaseView.Value unchecked before use on lines 180-184.
🛡️ Proposed fix to add null check
var newBotbase = Kernel.BotbaseManager.Bots.FirstOrDefault(bot => bot.Value.Name == name);
var newBotbaseView = Kernel.BotbaseManager.BotsViews.FirstOrDefault(bot => bot.Value.Name == name);
- if (newBotbase.Value == null)
+ if (newBotbase.Value == null || newBotbaseView.Value == null)
{
Log.Error($"Botbase [{name}] could not be found!");
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 newBotbase = Kernel.BotbaseManager.Bots.FirstOrDefault(bot => bot.Value.Name == name); | |
| var newBotbaseView = Kernel.BotbaseManager.BotsViews.FirstOrDefault(bot => bot.Value.Name == name); | |
| if (newBotbase.Value == null) | |
| { | |
| Log.Error($"Botbase [{name}] could not be found!"); | |
| return; | |
| } | |
| newBotbase.Value.Translate(); | |
| newBotbaseView.Value.Translate(); | |
| var control = newBotbase.Value.View; | |
| control.Name = newBotbase.Value.Name; | |
| control.Text = LanguageManager.GetLangBySpecificKey(newBotbase.Value.Name, "TabText", newBotbase.Value.TabText); | |
| var control = newBotbaseView.Value.View; | |
| control.Name = newBotbaseView.Value.Name; | |
| control.Text = LanguageManager.GetLangBySpecificKey(newBotbaseView.Value.Name, "TabText", newBotbaseView.Value.TabText); | |
| var newBotbase = Kernel.BotbaseManager.Bots.FirstOrDefault(bot => bot.Value.Name == name); | |
| var newBotbaseView = Kernel.BotbaseManager.BotsViews.FirstOrDefault(bot => bot.Value.Name == name); | |
| if (newBotbase.Value == null || newBotbaseView.Value == null) | |
| { | |
| Log.Error($"Botbase [{name}] could not be found!"); | |
| return; | |
| } | |
| newBotbaseView.Value.Translate(); | |
| var control = newBotbaseView.Value.View; | |
| control.Name = newBotbaseView.Value.Name; | |
| control.Text = LanguageManager.GetLangBySpecificKey(newBotbaseView.Value.Name, "TabText", newBotbaseView.Value.TabText); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Application/RSBot/Views/Main.cs` around lines 171 - 184, The code checks
newBotbase.Value but not newBotbaseView.Value, causing a possible null reference
when calling newBotbaseView.Value.Translate() and accessing View/Name/Text;
update the logic around the Kernel.BotbaseManager.BotsViews lookup
(newBotbaseView) to verify newBotbaseView.Value is not null before using it, and
if it is null log a clear error (similar to the existing Log.Error for
newBotbase) and return early; ensure you reference
newBotbaseView.Value.Translate(), newBotbaseView.Value.View,
newBotbaseView.Value.Name and LanguageManager.GetLangBySpecificKey only after
the null check.
Comment on lines
+15
to
+17
| public TrainingManager Manager { get; private set; } | ||
| public string Name => "RSBot.Training"; | ||
| public Area Area => Container.Bot.Area; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check how IBotbase.Area is accessed to determine if it could be called before Register()
rg -n "\.Area" --type=cs -C3 | grep -i "botbase"Repository: Silkroad-Developer-Community/RSBot
Length of output: 31731
🏁 Script executed:
cat -n Botbases/RSBot.Training/TrainingBase.csRepository: Silkroad-Developer-Community/RSBot
Length of output: 3833
🏁 Script executed:
# Find IBotbase interface to understand the contract
fd -e cs -type f | xargs grep -l "interface IBotbase" | head -5Repository: Silkroad-Developer-Community/RSBot
Length of output: 248
🏁 Script executed:
# Check when Register() is called - look for callers
rg "\.Register\(\)" --type=cs -B2 -A2 | head -50Repository: Silkroad-Developer-Community/RSBot
Length of output: 428
🏁 Script executed:
# Check BotbaseManager to understand initialization order
cat -n Library/RSBot.Core/Plugins/BotbaseManager.cs | head -100Repository: Silkroad-Developer-Community/RSBot
Length of output: 3747
🏁 Script executed:
# Check if Tick() is called before Register() - look at bot lifecycle
rg "Register\(\)|Tick\(\)" Library/RSBot.Core/Plugins/ --type=cs -B3 -A3 | head -80Repository: Silkroad-Developer-Community/RSBot
Length of output: 1340
🏁 Script executed:
# Verify if Container is a class/struct that could have null Bot property
fd -type f -name "*.cs" | xargs grep -l "class Container\|struct Container" | head -5Repository: Silkroad-Developer-Community/RSBot
Length of output: 248
🏁 Script executed:
# Check IBotbase interface definition
cat -n Library/RSBot.Core/Plugins/IBotbase.csRepository: Silkroad-Developer-Community/RSBot
Length of output: 1204
🏁 Script executed:
# Search for Container class definition to understand its structure
rg "class Container|struct Container" --type=cs -A 10 | head -40Repository: Silkroad-Developer-Community/RSBot
Length of output: 1409
🏁 Script executed:
# Check if Tick() is guaranteed to be called only after Register()
rg "\.Tick\(" Library/RSBot.Core/Plugins/ --type=cs -B 5 -A 2Repository: Silkroad-Developer-Community/RSBot
Length of output: 60
🏁 Script executed:
# Get full Container.cs from Training botbase
cat -n Botbases/RSBot.Training/Container.csRepository: Silkroad-Developer-Community/RSBot
Length of output: 698
🏁 Script executed:
# Check how Tick is called - look in Kernel or bot running code
rg "Tick\(\)" Library/RSBot.Core/ --type=cs -B 3 -A 1 | head -60Repository: Silkroad-Developer-Community/RSBot
Length of output: 644
🏁 Script executed:
# Check if other botbases use similar pattern - look for Area property implementations
rg "public Area Area" Botbases/ --type=cs -B 2 -A 2Repository: Silkroad-Developer-Community/RSBot
Length of output: 1722
🏁 Script executed:
# Check how Bot.cs actually calls Tick to understand the guarantee
cat -n Library/RSBot.Core/Bot.cs | grep -A 20 "Botbase.Tick"Repository: Silkroad-Developer-Community/RSBot
Length of output: 671
🏁 Script executed:
# Check if Container.Bot can be null - look for null checks on it
rg "Container\.Bot" Botbases/RSBot.Training/ --type=cs -B 2 -A 2 | grep -E "(if|null|\?)" | head -20Repository: Silkroad-Developer-Community/RSBot
Length of output: 1959
🏁 Script executed:
# Look at full Bot.cs to understand when Tick() is actually called
cat -n Library/RSBot.Core/Bot.cs | sed -n '50,80p'Repository: Silkroad-Developer-Community/RSBot
Length of output: 1021
🏁 Script executed:
# Critical check: Verify line 64 uses Kernel.Bot.Botbase not Container.Bot - compare which one is being used
cat -n Botbases/RSBot.Training/TrainingBase.cs | sed -n '62,70p'Repository: Silkroad-Developer-Community/RSBot
Length of output: 439
Remove unsafe access to uninitialized Container.Bot.
The Area property accesses Container.Bot.Area without null checking. While Container.Bot is initialized in Register(), the property can still be accessed before initialization completes or in edge cases. ConfigSubscriber.cs elsewhere in the codebase explicitly validates both Container.Lock and Container.Bot for null, confirming the risk is real.
Use the null-coalescing operator to safely handle the potential null reference:
🛡️ Apply null-safe accessor
- public Area Area => Container.Bot.Area;
+ public Area Area => Container.Bot?.Area;Note: Line 64 in Start() uses Kernel.Bot.Botbase.Area which references a different botbase instance than Container.Bot, so they operate independently.
📝 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
| public TrainingManager Manager { get; private set; } | |
| public string Name => "RSBot.Training"; | |
| public Area Area => Container.Bot.Area; | |
| public TrainingManager Manager { get; private set; } | |
| public string Name => "RSBot.Training"; | |
| public Area Area => Container.Bot?.Area; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Botbases/RSBot.Training/TrainingBase.cs` around lines 15 - 17, The Area
property currently dereferences Container.Bot directly and can throw if
Container.Bot is null; update the Area getter (public Area Area =>
Container.Bot.Area) to use a null-safe accessor so it returns
Container.Bot?.Area or a sensible default when Container.Bot is null, and ensure
any callers handle a possible null Area; locate this in TrainingBase (the Area
property) and keep Register() and Start() logic unchanged but remove the unsafe
direct access to Container.Bot to match the null checks used elsewhere (e.g.,
ConfigSubscriber.cs).
Comment on lines
+43
to
+80
| private async void OnAgentServerDisconnected() | ||
| { | ||
| Kernel.Bot.Stop(); | ||
|
|
||
| // Skiped: Cuz managing from ClientlessManager | ||
| if (Game.Clientless) | ||
| return; | ||
|
|
||
| ClientManager.Kill(); | ||
|
|
||
| if (GlobalConfig.Get<bool>("RSBot.General.EnableAutomatedLogin")) | ||
| { | ||
| var reloginSeq = Interlocked.Increment(ref _reloginSeq); | ||
|
|
||
| EventManager.FireEvent("OnAutoReloginStarted"); | ||
|
|
||
| int delay = 10000; | ||
| if (GlobalConfig.Get("RSBot.General.EnableWaitAfterDC", false)) | ||
| delay = GlobalConfig.Get<int>("RSBot.General.WaitAfterDC") * 60 * 1000; | ||
|
|
||
| Log.Warn($"Attempting relogin in {delay / 1000} seconds..."); | ||
| await Task.Delay(delay); | ||
|
|
||
| if (reloginSeq != Volatile.Read(ref _reloginSeq)) | ||
| return; | ||
|
|
||
| var userAuthenticated = await HandleRegionalAuth(); | ||
| if (!userAuthenticated) | ||
| { | ||
| Log.Warn("Regional auth failed!"); | ||
| return; | ||
| } | ||
|
|
||
| await StartClientProcess(); | ||
| return; | ||
| } | ||
| EventManager.FireEvent("OnClientDisconnected"); | ||
| } |
There was a problem hiding this comment.
async void event handlers swallow exceptions.
The OnAgentServerDisconnected method is async void, which means any exceptions thrown will crash the application or be silently swallowed depending on the synchronization context. Consider wrapping the body in a try-catch with logging, or using a fire-and-forget pattern with explicit error handling.
🛡️ Add exception handling
private async void OnAgentServerDisconnected()
{
+ try
+ {
Kernel.Bot.Stop();
// ... rest of method ...
EventManager.FireEvent("OnClientDisconnected");
+ }
+ catch (Exception ex)
+ {
+ Log.Fatal(ex);
+ }
}📝 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
| private async void OnAgentServerDisconnected() | |
| { | |
| Kernel.Bot.Stop(); | |
| // Skiped: Cuz managing from ClientlessManager | |
| if (Game.Clientless) | |
| return; | |
| ClientManager.Kill(); | |
| if (GlobalConfig.Get<bool>("RSBot.General.EnableAutomatedLogin")) | |
| { | |
| var reloginSeq = Interlocked.Increment(ref _reloginSeq); | |
| EventManager.FireEvent("OnAutoReloginStarted"); | |
| int delay = 10000; | |
| if (GlobalConfig.Get("RSBot.General.EnableWaitAfterDC", false)) | |
| delay = GlobalConfig.Get<int>("RSBot.General.WaitAfterDC") * 60 * 1000; | |
| Log.Warn($"Attempting relogin in {delay / 1000} seconds..."); | |
| await Task.Delay(delay); | |
| if (reloginSeq != Volatile.Read(ref _reloginSeq)) | |
| return; | |
| var userAuthenticated = await HandleRegionalAuth(); | |
| if (!userAuthenticated) | |
| { | |
| Log.Warn("Regional auth failed!"); | |
| return; | |
| } | |
| await StartClientProcess(); | |
| return; | |
| } | |
| EventManager.FireEvent("OnClientDisconnected"); | |
| } | |
| private async void OnAgentServerDisconnected() | |
| { | |
| try | |
| { | |
| Kernel.Bot.Stop(); | |
| // Skiped: Cuz managing from ClientlessManager | |
| if (Game.Clientless) | |
| return; | |
| ClientManager.Kill(); | |
| if (GlobalConfig.Get<bool>("RSBot.General.EnableAutomatedLogin")) | |
| { | |
| var reloginSeq = Interlocked.Increment(ref _reloginSeq); | |
| EventManager.FireEvent("OnAutoReloginStarted"); | |
| int delay = 10000; | |
| if (GlobalConfig.Get("RSBot.General.EnableWaitAfterDC", false)) | |
| delay = GlobalConfig.Get<int>("RSBot.General.WaitAfterDC") * 60 * 1000; | |
| Log.Warn($"Attempting relogin in {delay / 1000} seconds..."); | |
| await Task.Delay(delay); | |
| if (reloginSeq != Volatile.Read(ref _reloginSeq)) | |
| return; | |
| var userAuthenticated = await HandleRegionalAuth(); | |
| if (!userAuthenticated) | |
| { | |
| Log.Warn("Regional auth failed!"); | |
| return; | |
| } | |
| await StartClientProcess(); | |
| return; | |
| } | |
| EventManager.FireEvent("OnClientDisconnected"); | |
| } | |
| catch (Exception ex) | |
| { | |
| Log.Fatal(ex); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Plugins/RSBot.General/GeneralManager.cs` around lines 43 - 80, Change
OnAgentServerDisconnected from async void to async Task and add a top-level
try-catch that logs exceptions (use Log.Error) around the entire method body;
update any event invocation to call it as a fire-and-forget if they cannot await
(e.g. _ = OnAgentServerDisconnected();) so exceptions are observed; keep
existing logic (Kernel.Bot.Stop, ClientManager.Kill, EventManager.FireEvent,
Interlocked/Volatile _reloginSeq checks, HandleRegionalAuth, StartClientProcess)
intact but ensure unhandled exceptions from those calls are caught and logged.
Comment on lines
+81
to
+152
| private async void OnGatewayServerDisconnected() | ||
| { | ||
| AutoLogin.Pending = false; | ||
| EventManager.FireEvent("OnAutoLoginAborted"); | ||
|
|
||
| var wasClientless = Game.Clientless; | ||
|
|
||
| if (!Kernel.Proxy.IsConnectedToAgentserver && !Kernel.Proxy.IsSwitchingToAgentserver) | ||
| { | ||
| if (GlobalConfig.Get<bool>("RSBot.General.EnableAutomatedLogin")) | ||
| { | ||
| var reloginSeq = Interlocked.Increment(ref _reloginSeq); | ||
|
|
||
| EventManager.FireEvent("OnAutoReloginStarted"); | ||
|
|
||
| // Gateway disconnect can happen briefly during Gateway -> Agent switch (or the switch can start late | ||
| // due to thread scheduling). Give it a short grace period before we decide it's a real DC. | ||
| await Task.Delay(2000); | ||
|
|
||
| if (reloginSeq != Volatile.Read(ref _reloginSeq)) | ||
| return; | ||
|
|
||
| if (Kernel.Proxy.IsConnectedToAgentserver || Kernel.Proxy.IsSwitchingToAgentserver) | ||
| return; | ||
|
|
||
| EventManager.FireEvent("OnAutoReloginOngoing"); | ||
|
|
||
| Log.StatusLang("Ready"); | ||
| Kernel.Proxy.Shutdown(); | ||
|
|
||
| int delay = 10000; | ||
| if (GlobalConfig.Get("RSBot.General.EnableWaitAfterDC", false)) | ||
| delay = GlobalConfig.Get<int>("RSBot.General.WaitAfterDC") * 60 * 1000; | ||
|
|
||
| Log.Warn($"Attempting relogin in {delay / 1000} seconds..."); | ||
| await Task.Delay(delay); | ||
|
|
||
| // Prevent double-start if the disconnect event fires multiple times. | ||
| if (reloginSeq != Volatile.Read(ref _reloginSeq)) | ||
| return; | ||
|
|
||
| var userAuthenticated = await HandleRegionalAuth(); | ||
| if (!userAuthenticated) | ||
| { | ||
| Log.Warn("Regional auth failed!"); | ||
| return; | ||
| } | ||
|
|
||
| if (wasClientless) | ||
| { | ||
| // Clientless relogin: restart proxy flow only. | ||
| Game.Clientless = true; | ||
| Game.Start(); | ||
| } | ||
| else | ||
| { | ||
| // Client relogin: restart client process. | ||
| ClientManager.Kill(); | ||
| await StartClientProcess(); | ||
| } | ||
|
|
||
| return; | ||
| } | ||
|
|
||
| EventManager.FireEvent("OnClientDisconnected"); | ||
|
|
||
| Log.StatusLang("Ready"); | ||
| Kernel.Proxy.Shutdown(); | ||
|
|
||
| Game.Clientless = false; | ||
| } | ||
| } |
There was a problem hiding this comment.
Same async void exception handling concern as OnAgentServerDisconnected.
This complex handler should also have a try-catch wrapper to prevent unhandled exceptions from crashing the application. The logic itself is well-structured with proper sequence guards.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Plugins/RSBot.General/GeneralManager.cs` around lines 81 - 152,
OnGatewayServerDisconnected is an async void handler that can throw unhandled
exceptions — wrap the entire method body in a try/catch that catches Exception,
logs the error (including exception details) and ensures any cleanup/sequence
state (e.g. leaving _reloginSeq logic untouched) still occurs; locate the method
OnGatewayServerDisconnected and add the try { /* existing body */ } catch
(Exception ex) { Log.Error/Log.Exception("OnGatewayServerDisconnected failed",
ex); /* optionally EventManager.FireEvent("OnError") or similar */ } so
exceptions from HandleRegionalAuth, StartClientProcess, ClientManager.Kill, or
other awaited calls do not crash the process.
Comment on lines
+153
to
+166
| private async void OnEnterGame() | ||
| { | ||
| while (!Game.Ready) | ||
| await Task.Delay(100); | ||
|
|
||
| var startBot = GlobalConfig.Get<bool>("RSBot.General.StartBot"); | ||
| var useReturnScroll = GlobalConfig.Get<bool>("RSBot.General.UseReturnScroll"); | ||
|
|
||
| if (useReturnScroll) | ||
| Game.Player.UseReturnScroll(); | ||
|
|
||
| if (startBot) | ||
| Kernel.Bot.Start(); | ||
| } |
There was a problem hiding this comment.
Busy-wait loop without timeout could spin indefinitely.
The while (!Game.Ready) loop has no timeout or cancellation. If Game.Ready never becomes true due to a bug or network issue, this method will spin forever, wasting CPU cycles.
⏱️ Add timeout guard
private async void OnEnterGame()
{
- while (!Game.Ready)
- await Task.Delay(100);
+ var timeout = DateTime.UtcNow.AddSeconds(30);
+ while (!Game.Ready)
+ {
+ if (DateTime.UtcNow > timeout)
+ {
+ Log.Warn("Timed out waiting for Game.Ready");
+ return;
+ }
+ await Task.Delay(100);
+ }
var startBot = GlobalConfig.Get<bool>("RSBot.General.StartBot");📝 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
| private async void OnEnterGame() | |
| { | |
| while (!Game.Ready) | |
| await Task.Delay(100); | |
| var startBot = GlobalConfig.Get<bool>("RSBot.General.StartBot"); | |
| var useReturnScroll = GlobalConfig.Get<bool>("RSBot.General.UseReturnScroll"); | |
| if (useReturnScroll) | |
| Game.Player.UseReturnScroll(); | |
| if (startBot) | |
| Kernel.Bot.Start(); | |
| } | |
| private async void OnEnterGame() | |
| { | |
| var timeout = DateTime.UtcNow.AddSeconds(30); | |
| while (!Game.Ready) | |
| { | |
| if (DateTime.UtcNow > timeout) | |
| { | |
| Log.Warn("Timed out waiting for Game.Ready"); | |
| return; | |
| } | |
| await Task.Delay(100); | |
| } | |
| var startBot = GlobalConfig.Get<bool>("RSBot.General.StartBot"); | |
| var useReturnScroll = GlobalConfig.Get<bool>("RSBot.General.UseReturnScroll"); | |
| if (useReturnScroll) | |
| Game.Player.UseReturnScroll(); | |
| if (startBot) | |
| Kernel.Bot.Start(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Plugins/RSBot.General/GeneralManager.cs` around lines 153 - 166, The
OnEnterGame method spins on while (!Game.Ready) with no timeout; add a
timeout/cancellation guard (e.g., max wait of ~30s or configurable via
GlobalConfig) so the loop stops if Game.Ready never becomes true: implement a
timed loop using a CancellationToken/Stopwatch and await Task.Delay(...) inside
it, and if the timeout elapses log an error and return (or abort the startup)
instead of spinning; also consider changing the signature from async void
OnEnterGame to async Task OnEnterGame for better error propagation and
testability.
Comment on lines
+24
to
+25
| View.SidebarElement = new QuestSidebarElement(); | ||
| EventManager.FireEvent("OnAddSidebarElement", View.SidebarElement); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# 1) Locate add/remove sidebar events and sidebar element ownership paths
rg -n --type=cs -C2 'OnAddSidebarElement|OnRemoveSidebarElement|SidebarElement' Plugins/RSBot.Quest
# 2) Check how often quest Main view is instantiated
rg -n --type=cs -C2 'new\s+Main\s*\(' Plugins
# 3) Check whether this view unsubscribes/cleans up on disposal
rg -n --type=cs -C2 'Dispose\s*\(|UnsubscribeEvent\s*\(' Plugins/RSBot.Quest/Views/Main.cs Plugins/RSBot.QuestRepository: Silkroad-Developer-Community/RSBot
Length of output: 8472
🏁 Script executed:
# Check the full Main.cs file to see the Dispose implementation and overall lifecycle
cat -n Plugins/RSBot.Quest/Views/Main.csRepository: Silkroad-Developer-Community/RSBot
Length of output: 9318
🏁 Script executed:
# Check Main.Designer.cs for any custom Dispose implementation
grep -n "Dispose\|UnsubscribeEvent" Plugins/RSBot.Quest/Views/Main.Designer.cs | head -30Repository: Silkroad-Developer-Community/RSBot
Length of output: 217
🏁 Script executed:
# Search for handlers of OnAddSidebarElement and any related cleanup
rg -n "OnAddSidebarElement" --type=cs -A5 -B2
# Check if there's an OnRemoveSidebarElement event or any cleanup
rg -n "OnRemoveSidebarElement" --type=cs
# Check the View.cs to understand SidebarElement lifecycle
cat -n Plugins/RSBot.Quest/Views/View.csRepository: Silkroad-Developer-Community/RSBot
Length of output: 2054
🏁 Script executed:
# Check if the old sidebar element is removed when a new one is created
# Look at the controls collection management in Application/RSBot/Views/Main.cs
rg -n "pSidebarCustom.Controls" --type=cs -B3 -A3
# Also check if there's any cleanup in Dispose for event subscriptions
rg -n "UnsubscribeEvent" Plugins/RSBot.Quest/Views/Main.csRepository: Silkroad-Developer-Community/RSBot
Length of output: 507
Unguarded sidebar registration and event subscription leaks on Main recreation.
Lines 24–25 register a sidebar element and fire OnAddSidebarElement, which unconditionally adds the control to the parent sidebar container. Additionally, lines 38–39 subscribe to events without cleanup. When Main is recreated (triggered by disposal check in View.cs), a new sidebar element is added without removing the previous one, and new event handler subscriptions accumulate without being unsubscribed. This causes resource leaks and potential UI duplication.
Implement cleanup in Dispose: unsubscribe the events subscribed in SubscribeEvents(), and fire a corresponding cleanup event (or directly remove the sidebar element from the parent container) to maintain lifecycle symmetry.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Plugins/RSBot.Quest/Views/Main.cs` around lines 24 - 25, The code currently
adds View.SidebarElement with EventManager.FireEvent("OnAddSidebarElement") and
subscribes handlers in SubscribeEvents() but never removes/unsubscribes on
teardown; update Dispose() in Main to mirror setup by (1) unsubscribing every
event handler registered by SubscribeEvents() (remove the same
delegates/handlers you added there), and (2) remove the sidebar element by
either firing a symmetric EventManager.FireEvent("OnRemoveSidebarElement",
View.SidebarElement) or directly removing View.SidebarElement from its parent
container (e.g., parent.Controls.Remove(View.SidebarElement)); ensure Dispose()
nulls View.SidebarElement or marks it disposed after removal to avoid duplicates
on Main recreation.
Comment on lines
+79
to
+101
| public void UpdateMastery(byte level, RefSkillMastery record,decimal gap = 0) | ||
| { | ||
| if (_isUpdatingMastery) return; | ||
| while (level + gap < Game.Player.Level) | ||
| { | ||
| _isUpdatingMastery = true; | ||
| var nextMasteryLevel = Game.ReferenceManager.GetRefLevel((byte)(level + 1)); | ||
|
|
||
| if (nextMasteryLevel.Exp_M > Game.Player.SkillPoints) | ||
| { | ||
| Log.Debug( | ||
| $"Auto. upping mastery cancelled due to insufficient skill points. Required: {nextMasteryLevel.Exp_M}" | ||
| ); | ||
|
|
||
| break; | ||
| } | ||
| Log.Notify($"Auto. train mastery [{record.Name} to lv. {nextMasteryLevel}"); | ||
| LearnMasteryHandler.LearnMastery(record.ID); | ||
| level += 1; | ||
| Thread.Sleep(500); | ||
| } | ||
| _isUpdatingMastery = false; | ||
| } |
There was a problem hiding this comment.
Blocking Thread.Sleep in UpdateMastery can freeze the calling thread.
The while loop with Thread.Sleep(500) is blocking. If OnSpUpdated is fired on the UI thread or a critical thread, this will cause hangs. Consider using async/await with Task.Delay instead.
Additionally, _isUpdatingMastery is accessed without synchronization but could be called from different threads via events.
⚡ Refactor to async pattern
- private bool _isUpdatingMastery = false;
+ private int _isUpdatingMastery = 0;
- public void UpdateMastery(byte level, RefSkillMastery record, decimal gap = 0)
+ public async Task UpdateMasteryAsync(byte level, RefSkillMastery record, decimal gap = 0)
{
- if (_isUpdatingMastery) return;
+ if (Interlocked.CompareExchange(ref _isUpdatingMastery, 1, 0) != 0)
+ return;
+ try
+ {
while (level + gap < Game.Player.Level)
{
- _isUpdatingMastery = true;
var nextMasteryLevel = Game.ReferenceManager.GetRefLevel((byte)(level + 1));
if (nextMasteryLevel.Exp_M > Game.Player.SkillPoints)
{
Log.Debug(
$"Auto. upping mastery cancelled due to insufficient skill points. Required: {nextMasteryLevel.Exp_M}"
);
break;
}
Log.Notify($"Auto. train mastery [{record.Name} to lv. {nextMasteryLevel}");
LearnMasteryHandler.LearnMastery(record.ID);
level += 1;
- Thread.Sleep(500);
+ await Task.Delay(500);
+ }
+ }
+ finally
+ {
+ Interlocked.Exchange(ref _isUpdatingMastery, 0);
}
- _isUpdatingMastery = false;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Plugins/RSBot.Skills/SkillsManager.cs` around lines 79 - 101, UpdateMastery
blocks the calling thread by using Thread.Sleep and unsafely toggles
_isUpdatingMastery; make UpdateMastery async (Task-returning) and replace
Thread.Sleep(500) with await Task.Delay(500), call
LearnMasteryHandler.LearnMastery appropriately from the async method (or wrap it
in Task.Run if it is blocking), and protect _isUpdatingMastery using a
thread-safe mechanism (e.g., Interlocked.CompareExchange or a private
SemaphoreSlim) to prevent concurrent runs started by events like OnSpUpdated.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
Plugins/RSBot.Inventory/InventoryManager.cs (1)
28-68: ⚡ Quick winExtract duplicated toggle logic into a shared helper.
Both toggle methods implement the same read/contains/remove-or-add/write flow; centralizing it reduces drift and keeps behavior changes consistent.
Refactor sketch
public static bool ToggleItemAtTrainplaceByCodeName(string codeName) { - var items = PlayerConfig.GetArray<string>("RSBot.Inventory.ItemsAtTrainplace").ToList(); - bool isEnabled; - - if (items.Contains(codeName)) - { - items.Remove(codeName); - isEnabled = false; - } - else - { - items.Add(codeName); - isEnabled = true; - } - - PlayerConfig.SetArray("RSBot.Inventory.ItemsAtTrainplace", items); - return isEnabled; + return ToggleCodeName("RSBot.Inventory.ItemsAtTrainplace", codeName); } @@ public static bool ToggleAutoUseByCodeName(string codeName) { - var items = PlayerConfig.GetArray<string>("RSBot.Inventory.AutoUseAccordingToPurpose").ToList(); - bool isNowEnabled; - - if (items.Contains(codeName)) - { - items.Remove(codeName); - isNowEnabled = false; - } - else - { - items.Add(codeName); - isNowEnabled = true; - } - - PlayerConfig.SetArray("RSBot.Inventory.AutoUseAccordingToPurpose", items); - return isNowEnabled; + return ToggleCodeName("RSBot.Inventory.AutoUseAccordingToPurpose", codeName); } + +private static bool ToggleCodeName(string key, string codeName) +{ + if (string.IsNullOrWhiteSpace(codeName)) + throw new System.ArgumentException("codeName must not be null or whitespace.", nameof(codeName)); + + codeName = codeName.Trim(); + var items = PlayerConfig.GetArray<string>(key).ToList(); + var enabled = !items.Remove(codeName); + if (enabled) items.Add(codeName); + PlayerConfig.SetArray(key, items); + return enabled; +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Plugins/RSBot.Inventory/InventoryManager.cs` around lines 28 - 68, Extract the duplicated read/contains/remove-or-add/write logic from ToggleItemAtTrainplaceByCodeName and ToggleAutoUseByCodeName into a single helper (e.g., ToggleStringInConfigArray) that takes the PlayerConfig key (string) and codeName and returns the new enabled state; both ToggleItemAtTrainplaceByCodeName and ToggleAutoUseByCodeName should call this helper with "RSBot.Inventory.ItemsAtTrainplace" and "RSBot.Inventory.AutoUseAccordingToPurpose" respectively, and the helper should use PlayerConfig.GetArray and PlayerConfig.SetArray to perform the list mutation and return the boolean result.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Plugins/RSBot.Chat/Views/Main.cs`:
- Around line 19-21: The subscription to EventManager.SubscribeEvent in
SubscribeEvents (and the other similar subscription at lines ~42–48) can cause
AppendMessage to run before the control has a window handle or after disposal;
change the lifecycle to subscribe in OnHandleCreated and unsubscribe in Dispose
(or use EventManager.UnsubscribeEvent) so handlers are only active while the
control has a valid handle, and inside AppendMessage guard with
IsHandleCreated/IsDisposed and use InvokeRequired + BeginInvoke only when safe
to marshal to the UI thread to avoid cross-thread access and Invoke throwing
during disposal.
In `@Plugins/RSBot.General/PacketHandler/GatewayServerListResponse.cs`:
- Around line 101-102: The event is fired inside the loop while
Serverlist.Servers is still being built, causing race/collection-modified
issues; update GatewayServerListResponse by removing the
EventManager.FireEvent("OnServerListUpdated") call from inside the loop that
adds to Serverlist.Servers and instead call FireEvent once after the loop
completes (or, alternatively, create an immutable snapshot of Serverlist.Servers
and pass that to listeners), so UpdateServerInfo listeners only see a
fully-built list.
In `@Plugins/RSBot.Inventory/InventoryManager.cs`:
- Around line 28-46: Add a defensive guard at the start of
ToggleItemAtTrainplaceByCodeName to validate the codeName parameter (ensure it's
not null, empty, or whitespace) and return without mutating PlayerConfig if
invalid; update PlayerConfig only when codeName is valid. Make the same guard
change for the related toggling method(s) in this class (e.g., the toggle that
lives around lines 50-68) so neither ToggleItemAtTrainplaceByCodeName nor the
other toggle writes null/empty/whitespace values into
"RSBot.Inventory.ItemsAtTrainplace" or other persisted lists.
In `@Plugins/RSBot.ServerInfo/ServerInfoManager.cs`:
- Around line 9-12: GetServers currently returns the mutable backing list
Serverlist.Servers which allows callers to observe and modify the live
collection and causes concurrent-enumeration issues in UpdateServerInfo; change
GetServers (in ServerInfoManager) to return a snapshot or a read-only view
(e.g., new List<Server>(Serverlist.Servers) or Serverlist.Servers.AsReadOnly())
so callers iterate a stable collection and cannot mutate the underlying
Serverlist.Servers.
In `@Plugins/RSBot.ServerInfo/Views/Main.cs`:
- Around line 15-20: Unsubscribe the event subscription to avoid Invoke() on a
disposed control: add a corresponding UnsubscribeEvents() that calls
EventManager.UnsubscribeEvent("OnServerListUpdated", UpdateServerInfo) (mirror
of SubscribeEvents()), and call UnsubscribeEvents() from Dispose (override
Dispose(bool disposing) or Dispose()) before base.Dispose(disposing);
additionally, guard UpdateServerInfo() with a quick check (if (IsDisposed ||
!IsHandleCreated) return;) before any Invoke/BeginInvoke to prevent marshaling
to a dead handle.
---
Nitpick comments:
In `@Plugins/RSBot.Inventory/InventoryManager.cs`:
- Around line 28-68: Extract the duplicated read/contains/remove-or-add/write
logic from ToggleItemAtTrainplaceByCodeName and ToggleAutoUseByCodeName into a
single helper (e.g., ToggleStringInConfigArray) that takes the PlayerConfig key
(string) and codeName and returns the new enabled state; both
ToggleItemAtTrainplaceByCodeName and ToggleAutoUseByCodeName should call this
helper with "RSBot.Inventory.ItemsAtTrainplace" and
"RSBot.Inventory.AutoUseAccordingToPurpose" respectively, and the helper should
use PlayerConfig.GetArray and PlayerConfig.SetArray to perform the list mutation
and return the boolean result.
🪄 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 Plus
Run ID: a15e8947-a3a3-4497-8c38-7cbe8ff85a4d
📒 Files selected for processing (10)
Plugins/RSBot.Chat/Views/Main.csPlugins/RSBot.General/PacketHandler/GatewayServerListResponse.csPlugins/RSBot.Inventory/Bootstrap.csPlugins/RSBot.Inventory/InventoryManager.csPlugins/RSBot.Inventory/Views/Main.csPlugins/RSBot.Log/Views/Main.csPlugins/RSBot.Quest/QuestManager.csPlugins/RSBot.Quest/Views/Main.csPlugins/RSBot.ServerInfo/ServerInfoManager.csPlugins/RSBot.ServerInfo/Views/Main.cs
💤 Files with no reviewable changes (1)
- Plugins/RSBot.Inventory/Bootstrap.cs
✅ Files skipped from review due to trivial changes (1)
- Plugins/RSBot.Inventory/Views/Main.cs
🚧 Files skipped from review as they are similar to previous changes (2)
- Plugins/RSBot.Quest/QuestManager.cs
- Plugins/RSBot.Quest/Views/Main.cs
Comment on lines
19
to
+21
| private void SubscribeEvents() | ||
| { | ||
| EventManager.SubscribeEvent("OnEnterGame", OnEnterGame); | ||
| EventManager.SubscribeEvent("OnMessageReceived", AppendMessage); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the Main.cs file and get its location
find . -name "Main.cs" -path "*/RSBot.Chat/Views/*"Repository: Silkroad-Developer-Community/RSBot
Length of output: 113
🏁 Script executed:
# Read the Main.cs file to see the subscription and callback implementation
cat -n Plugins/RSBot.Chat/Views/Main.cs | head -100Repository: Silkroad-Developer-Community/RSBot
Length of output: 3270
🏁 Script executed:
# Look for AppendMessage method implementation
rg -A 20 "private.*AppendMessage|void AppendMessage" Plugins/RSBot.Chat/Views/Main.csRepository: Silkroad-Developer-Community/RSBot
Length of output: 641
🏁 Script executed:
# Find where OnMessageReceived is fired
rg -r "OnMessageReceived" Plugins/RSBot.Chat/ --type csRepository: Silkroad-Developer-Community/RSBot
Length of output: 60
🏁 Script executed:
# Check constructor and OnHandleCreated in Main.cs
rg -A 10 "public Main|OnHandleCreated|Dispose" Plugins/RSBot.Chat/Views/Main.csRepository: Silkroad-Developer-Community/RSBot
Length of output: 340
🏁 Script executed:
# Search more broadly for where OnMessageReceived is fired
rg -r "OnMessageReceived" . --type cs | head -20Repository: Silkroad-Developer-Community/RSBot
Length of output: 7718
🏁 Script executed:
# Look at ChatResponse.cs as mentioned in the review comment
cat -n Plugins/RSBot.Chat/Bundle/Network/ChatResponse.cs | sed -n '40,70p'Repository: Silkroad-Developer-Community/RSBot
Length of output: 1256
🏁 Script executed:
# Check if EventManager is defined and understand its behavior
rg -r "class EventManager|SubscribeEvent" . --type cs | head -10Repository: Silkroad-Developer-Community/RSBot
Length of output: 5825
🏁 Script executed:
# Check if there's any Dispose or OnHandleCreated in Main.cs
wc -l Plugins/RSBot.Chat/Views/Main.csRepository: Silkroad-Developer-Community/RSBot
Length of output: 115
🏁 Script executed:
# Get the full Main.cs to see if there's Dispose or other lifecycle methods
tail -100 Plugins/RSBot.Chat/Views/Main.cs | cat -nRepository: Silkroad-Developer-Community/RSBot
Length of output: 3464
🏁 Script executed:
# Check if there's any evidence of when the control becomes visible/handle created
# Look for any parent form or designer-generated code that might show lifecycle
find Plugins/RSBot.Chat/Views -name "*.Designer.cs" -o -name "*.cs" | xargs grep -l "Main\|DoubleBufferedControl"Repository: Silkroad-Developer-Community/RSBot
Length of output: 186
🏁 Script executed:
# Check DoubleBufferedControl base class to understand initialization
rg -A 5 "class DoubleBufferedControl" --type csRepository: Silkroad-Developer-Community/RSBot
Length of output: 60
🏁 Script executed:
# Check the Designer file to see if there are any lifecycle overrides
head -50 Plugins/RSBot.Chat/Views/Main.Designer.csRepository: Silkroad-Developer-Community/RSBot
Length of output: 2169
🏁 Script executed:
# Check if SDUI.Controls.DoubleBufferedControl is defined elsewhere in codebase
rg -r "class DoubleBufferedControl" --type csRepository: Silkroad-Developer-Community/RSBot
Length of output: 475
🏁 Script executed:
# Find where SDUI.Controls is defined to understand DoubleBufferedControl
find . -name "*.cs" -type f -exec grep -l "class DoubleBufferedControl" {} \;Repository: Silkroad-Developer-Community/RSBot
Length of output: 60
🏁 Script executed:
# Check if EventManager.SubscribeEvent is ever unsubscribed in Dispose or elsewhere
rg "UnsubscribeEvent|SubscribeEvent" Plugins/RSBot.Chat/ --type csRepository: Silkroad-Developer-Community/RSBot
Length of output: 574
Defer this subscription until the control has a handle, or guard the callback lifecycle.
OnMessageReceived can fire AppendMessage before the view's window handle exists. In that state, InvokeRequired may return false, allowing the method to access txt* controls directly from a worker thread. Additionally, once disposal begins, Invoke(...) can throw. Subscribe in OnHandleCreated and unsubscribe in Dispose, or at minimum guard IsHandleCreated and IsDisposed before marshaling. Also applies to lines 42–48 and the corresponding event subscription.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Plugins/RSBot.Chat/Views/Main.cs` around lines 19 - 21, The subscription to
EventManager.SubscribeEvent in SubscribeEvents (and the other similar
subscription at lines ~42–48) can cause AppendMessage to run before the control
has a window handle or after disposal; change the lifecycle to subscribe in
OnHandleCreated and unsubscribe in Dispose (or use
EventManager.UnsubscribeEvent) so handlers are only active while the control has
a valid handle, and inside AppendMessage guard with IsHandleCreated/IsDisposed
and use InvokeRequired + BeginInvoke only when safe to marshal to the UI thread
to avoid cross-thread access and Invoke throwing during disposal.
Comment on lines
101
to
+102
| Log.Notify($"Found server: {serverName} ({state})"); | ||
| EventManager.FireEvent("OnServerListUpdated"); |
There was a problem hiding this comment.
Publish OnServerListUpdated only after the list is complete.
FireEvent(...) is being called while Serverlist.Servers is still growing. Because EventManager dispatches packet-thread listeners asynchronously, UpdateServerInfo() can refresh against a partially built list or hit Collection was modified while this loop is still appending. Emit the event once after the loop, or pass an immutable snapshot.
Possible fix
Log.Notify($"Found server: {serverName} ({state})");
- EventManager.FireEvent("OnServerListUpdated");
}
+ EventManager.FireEvent("OnServerListUpdated");
AutoLogin.Handle();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Plugins/RSBot.General/PacketHandler/GatewayServerListResponse.cs` around
lines 101 - 102, The event is fired inside the loop while Serverlist.Servers is
still being built, causing race/collection-modified issues; update
GatewayServerListResponse by removing the
EventManager.FireEvent("OnServerListUpdated") call from inside the loop that
adds to Serverlist.Servers and instead call FireEvent once after the loop
completes (or, alternatively, create an immutable snapshot of Serverlist.Servers
and pass that to listeners), so UpdateServerInfo listeners only see a
fully-built list.
Comment on lines
+28
to
+46
| public static bool ToggleItemAtTrainplaceByCodeName(string codeName) | ||
| { | ||
| var items = PlayerConfig.GetArray<string>("RSBot.Inventory.ItemsAtTrainplace").ToList(); | ||
| bool isEnabled; | ||
|
|
||
| if (items.Contains(codeName)) | ||
| { | ||
| items.Remove(codeName); | ||
| isEnabled = false; | ||
| } | ||
| else | ||
| { | ||
| items.Add(codeName); | ||
| isEnabled = true; | ||
| } | ||
|
|
||
| PlayerConfig.SetArray("RSBot.Inventory.ItemsAtTrainplace", items); | ||
| return isEnabled; | ||
| } |
There was a problem hiding this comment.
Validate codeName before mutating persisted config lists.
Without a guard, null/empty/whitespace values can be written into player config and then reused by GetItemsAtTrainplace() / GetItemsByPurpose(). Add a defensive check at method entry.
Suggested fix
public static bool ToggleItemAtTrainplaceByCodeName(string codeName)
{
+ if (string.IsNullOrWhiteSpace(codeName))
+ throw new System.ArgumentException("codeName must not be null or whitespace.", nameof(codeName));
+ codeName = codeName.Trim();
+
var items = PlayerConfig.GetArray<string>("RSBot.Inventory.ItemsAtTrainplace").ToList();
bool isEnabled;
@@
public static bool ToggleAutoUseByCodeName(string codeName)
{
+ if (string.IsNullOrWhiteSpace(codeName))
+ throw new System.ArgumentException("codeName must not be null or whitespace.", nameof(codeName));
+ codeName = codeName.Trim();
+
var items = PlayerConfig.GetArray<string>("RSBot.Inventory.AutoUseAccordingToPurpose").ToList();
bool isNowEnabled;Also applies to: 50-68
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Plugins/RSBot.Inventory/InventoryManager.cs` around lines 28 - 46, Add a
defensive guard at the start of ToggleItemAtTrainplaceByCodeName to validate the
codeName parameter (ensure it's not null, empty, or whitespace) and return
without mutating PlayerConfig if invalid; update PlayerConfig only when codeName
is valid. Make the same guard change for the related toggling method(s) in this
class (e.g., the toggle that lives around lines 50-68) so neither
ToggleItemAtTrainplaceByCodeName nor the other toggle writes
null/empty/whitespace values into "RSBot.Inventory.ItemsAtTrainplace" or other
persisted lists.
Comment on lines
+9
to
+12
| public static List<Server> GetServers() | ||
| { | ||
| return Serverlist.Servers; | ||
| } |
There was a problem hiding this comment.
Return a snapshot here instead of the backing list.
GetServers() currently hands callers the same mutable Serverlist.Servers instance that the packet handler is still updating. That keeps the UI coupled to Serverlist and makes UpdateServerInfo() vulnerable to concurrent-enumeration failures. Returning a copy/read-only collection fixes both.
Possible fix
+using System;
using System.Collections.Generic;
namespace RSBot.ServerInfo
{
public class ServerInfoManager
{
- public static List<Server> GetServers()
+ public static IReadOnlyList<Server> GetServers()
{
- return Serverlist.Servers;
+ return Serverlist.Servers == null
+ ? Array.Empty<Server>()
+ : Serverlist.Servers.ToArray();
}
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Plugins/RSBot.ServerInfo/ServerInfoManager.cs` around lines 9 - 12,
GetServers currently returns the mutable backing list Serverlist.Servers which
allows callers to observe and modify the live collection and causes
concurrent-enumeration issues in UpdateServerInfo; change GetServers (in
ServerInfoManager) to return a snapshot or a read-only view (e.g., new
List<Server>(Serverlist.Servers) or Serverlist.Servers.AsReadOnly()) so callers
iterate a stable collection and cannot mutate the underlying Serverlist.Servers.
Comment on lines
+15
to
+20
| SubscribeEvents(); | ||
| UpdateServerInfo(); | ||
| } | ||
|
|
||
| private void SubscribeEvents() | ||
| { | ||
| EventManager.SubscribeEvent("OnServerListUpdated", UpdateServerInfo); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -t f "Main.cs" | grep -i serverinfoRepository: Silkroad-Developer-Community/RSBot
Length of output: 117
🏁 Script executed:
cat -n Plugins/RSBot.ServerInfo/Views/Main.csRepository: Silkroad-Developer-Community/RSBot
Length of output: 1347
🏁 Script executed:
rg "OnServerListUpdated" -A 3 -B 3Repository: Silkroad-Developer-Community/RSBot
Length of output: 1191
🏁 Script executed:
# Check for Dispose implementation and any handle/disposed checks
rg "Dispose|IsHandleCreated|IsDisposed" Plugins/RSBot.ServerInfo/Views/Main.csRepository: Silkroad-Developer-Community/RSBot
Length of output: 60
🏁 Script executed:
# Look for where the event is raised
rg -t cs "OnServerListUpdated" --iglob "*GatewayServerListResponse*" -A 2 -B 2Repository: Silkroad-Developer-Community/RSBot
Length of output: 539
🏁 Script executed:
# Check when GatewayServerListResponse is called - look for context
cat -n Plugins/RSBot.General/PacketHandler/GatewayServerListResponse.cs | head -50Repository: Silkroad-Developer-Community/RSBot
Length of output: 1842
🏁 Script executed:
# Check if DoubleBufferedControl is defined in this repo
rg "class DoubleBufferedControl" -A 10Repository: Silkroad-Developer-Community/RSBot
Length of output: 60
🏁 Script executed:
# Check for any Dispose or unsubscribe patterns in Main.cs more carefully
cat -n Plugins/RSBot.ServerInfo/Views/Main.csRepository: Silkroad-Developer-Community/RSBot
Length of output: 1347
🏁 Script executed:
# Check if there's a designer-generated partial class
fd -t f "Main.Designer.cs" Plugins/RSBot.ServerInfo/Views/Repository: Silkroad-Developer-Community/RSBot
Length of output: 126
🏁 Script executed:
# Check what DoubleBufferedControl might be from SDUI.Controls
rg "DoubleBufferedControl" -A 5 -B 2 | head -30Repository: Silkroad-Developer-Community/RSBot
Length of output: 1688
🏁 Script executed:
# Understand EventManager subscription/unsubscription patterns
rg "UnsubscribeEvent" -B 2 -A 2 | head -40Repository: Silkroad-Developer-Community/RSBot
Length of output: 60
🏁 Script executed:
cat -n Plugins/RSBot.ServerInfo/Views/Main.Designer.csRepository: Silkroad-Developer-Community/RSBot
Length of output: 3074
🏁 Script executed:
# Search for any Dispose override in the ServerInfo plugin
rg "override.*Dispose|Dispose\(" Plugins/RSBot.ServerInfo/ -A 3 -B 1Repository: Silkroad-Developer-Community/RSBot
Length of output: 952
🏁 Script executed:
# Look for patterns where controls DO unsubscribe from events
rg "UnsubscribeEvent|Dispose" Plugins/RSBot.Chat/Views/ -A 2 -B 2 | head -60Repository: Silkroad-Developer-Community/RSBot
Length of output: 902
🏁 Script executed:
# Check if other views in the codebase properly unsubscribe from events
rg "SubscribeEvent|UnsubscribeEvent" Plugins/RSBot.Chat/Views/Main.cs -A 2 -B 2Repository: Silkroad-Developer-Community/RSBot
Length of output: 286
🏁 Script executed:
# Check what the Chat view does for similar event subscriptions
cat -n Plugins/RSBot.Chat/Views/Main.cs | head -80Repository: Silkroad-Developer-Community/RSBot
Length of output: 2699
Unsubscribe from OnServerListUpdated in Dispose to prevent post-disposal errors.
The event subscription in the constructor is never unsubscribed. If a packet arrives while the control is being disposed, Invoke() will throw an ObjectDisposedException. Either unsubscribe in Dispose, or guard with IsHandleCreated and IsDisposed checks before marshaling in UpdateServerInfo().
Also applies to: 24-28
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Plugins/RSBot.ServerInfo/Views/Main.cs` around lines 15 - 20, Unsubscribe the
event subscription to avoid Invoke() on a disposed control: add a corresponding
UnsubscribeEvents() that calls
EventManager.UnsubscribeEvent("OnServerListUpdated", UpdateServerInfo) (mirror
of SubscribeEvents()), and call UnsubscribeEvents() from Dispose (override
Dispose(bool disposing) or Dispose()) before base.Dispose(disposing);
additionally, guard UpdateServerInfo() with a quick check (if (IsDisposed ||
!IsHandleCreated) return;) before any Invoke/BeginInvoke to prevent marshaling
to a dead handle.
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)
Plugins/RSBot.Party/Views/Main.cs (1)
528-541:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDuplicate auto-reform logic between View and Manager.
This handler contains the same auto-reform logic that exists in
PartyManager.OnPartyMemberLeave. Both are subscribed to the"OnPartyMemberLeave"event, soBundle.Container.PartyMatching.Create()may be called twice when a member leaves.Consider removing the auto-reform logic from the View since
PartyManagernow handles this responsibility.🔧 Proposed fix
private void OnPartyMemberLeave(PartyMember member) { Log.NotifyLang("UserLeftParty", member.Name); listParty.Items.RemoveByKey(member.Name); - - if (Bundle.Container.PartyMatching.Config.AutoReform) - if ( - Game.Party != null - && !Bundle.Container.PartyMatching.HasMatchingEntry - && Game.Party.Members?.Count < Game.Party.Settings.MaxMember - ) - Bundle.Container.PartyMatching.Create(); }🤖 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 `@Plugins/RSBot.Party/Views/Main.cs` around lines 528 - 541, The OnPartyMemberLeave handler in Main.cs duplicates the auto-reform logic now handled in PartyManager.OnPartyMemberLeave; remove the conditional block that checks Bundle.Container.PartyMatching.Config.AutoReform and calls Bundle.Container.PartyMatching.Create() (the entire if (...) { Bundle.Container.PartyMatching.Create(); } section) but keep the Log.NotifyLang("UserLeftParty", member.Name) and listParty.Items.RemoveByKey(member.Name) lines so the view still logs and updates the UI; rely on PartyManager.OnPartyMemberLeave to perform auto-reform.
🧹 Nitpick comments (1)
Plugins/RSBot.Party/PartyManager.cs (1)
30-40: 💤 Low valueConsider adding exception handling to
async voidevent handler.
async voidmethods propagate exceptions to the synchronization context, which can crash the application if unhandled. Since this is an event handler fired viaDynamicInvoke, exceptions after theawaitwill be unobserved.🛡️ Suggested defensive wrapper
private async void OnEnterGame() { + try + { await Task.Delay(5000); if ( Game.Ready && Bundle.Container.PartyMatching.Config.AutoReform && !Bundle.Container.PartyMatching.HasMatchingEntry ) Bundle.Container.PartyMatching.Create(); + } + catch (Exception ex) + { + Log.Error($"Error in OnEnterGame: {ex.Message}"); + } }🤖 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 `@Plugins/RSBot.Party/PartyManager.cs` around lines 30 - 40, OnEnterGame is an async void event handler that can raise unobserved exceptions after the await; wrap the body (at minimum the awaited section and calls to Bundle.Container.PartyMatching.Create()) in a try/catch that catches Exception, logs the error (including exception message and stack) via your component logger, and prevents the exception from escaping the method; if possible prefer changing the handler to async Task and update the event invocation to await it, otherwise keep async void but ensure the try/catch is present around the await and subsequent calls.
🤖 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 `@Application/RSBot/Program.cs`:
- Around line 128-132: The code currently lowercases the entire console input
(Console.ReadLine()?.ToLower().Split(',')) which forces payload/arguments to
lowercase; instead read the raw line, split it into parts, validate parts
non-empty, then only call ToLower() on the command token (assign to variable
command from parts[0].ToLower()) while leaving args (parts.Skip(1).ToArray())
unmodified so chat/content casing is preserved; apply the same change to the
other occurrence around the block using the same variables (input, command,
args).
- Around line 118-123: Duplicate event firing occurs after
BotCL.Initialize(ProfileManager.SelectedProfile) because Initialize already
triggers the plugin and botbase load events; remove the explicit
EventManager.FireEvent calls for "OnLoadPlugins" and "OnLoadBotbases" in
Program.cs startup so subscribers are not invoked twice—specifically delete or
comment out the lines calling EventManager.FireEvent("OnLoadPlugins") and
EventManager.FireEvent("OnLoadBotbases") immediately following
BotCL.Initialize(...) to rely on the existing load behavior inside
BotCL.Initialize.
In `@Plugins/RSBot.Party/PartyManager.cs`:
- Around line 86-97: The empty catch in OnPartyData silently swallows
exceptions; modify OnPartyData to explicitly check for nulls (e.g., if (Game ==
null || Game.Party == null) { OnPartyDismiss(); return; }) instead of relying on
an exception, and remove the empty catch block; if you still need to catch
unexpected errors, catch Exception ex and log it via the plugin logger (or
similar) with context ("OnPartyData failed") while preserving existing call to
OnPartyDismiss when appropriate so Game.Party nulls are handled
deterministically.
In `@Plugins/RSBot.Skills/Views/Main.cs`:
- Around line 62-66: The UI-thread marshaling calls (e.g., the Invoke call that
calls OnRemoveItemPerk) should be hardened: before marshaling check control
lifecycle flags (if IsDisposed || Disposing || !IsHandleCreated) and return
early to avoid raising events on a torn-down control, and replace synchronous
Invoke with asynchronous BeginInvoke to avoid blocking the caller thread; apply
this pattern to all similar blocks (the Invoke usages that call OnRemoveItemPerk
and the other marshaled event callbacks referenced in the review) so each uses
the lifecycle guard and BeginInvoke.
---
Outside diff comments:
In `@Plugins/RSBot.Party/Views/Main.cs`:
- Around line 528-541: The OnPartyMemberLeave handler in Main.cs duplicates the
auto-reform logic now handled in PartyManager.OnPartyMemberLeave; remove the
conditional block that checks Bundle.Container.PartyMatching.Config.AutoReform
and calls Bundle.Container.PartyMatching.Create() (the entire if (...) {
Bundle.Container.PartyMatching.Create(); } section) but keep the
Log.NotifyLang("UserLeftParty", member.Name) and
listParty.Items.RemoveByKey(member.Name) lines so the view still logs and
updates the UI; rely on PartyManager.OnPartyMemberLeave to perform auto-reform.
---
Nitpick comments:
In `@Plugins/RSBot.Party/PartyManager.cs`:
- Around line 30-40: OnEnterGame is an async void event handler that can raise
unobserved exceptions after the await; wrap the body (at minimum the awaited
section and calls to Bundle.Container.PartyMatching.Create()) in a try/catch
that catches Exception, logs the error (including exception message and stack)
via your component logger, and prevents the exception from escaping the method;
if possible prefer changing the handler to async Task and update the event
invocation to await it, otherwise keep async void but ensure the try/catch is
present around the await and subsequent calls.
🪄 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 Plus
Run ID: 79569ca3-5c89-4926-becf-90b88b1cce7e
📒 Files selected for processing (5)
Application/RSBot/Program.csPlugins/RSBot.Party/PartyManager.csPlugins/RSBot.Party/PartyPlugin.csPlugins/RSBot.Party/Views/Main.csPlugins/RSBot.Skills/Views/Main.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- Plugins/RSBot.Party/PartyPlugin.cs
Comment on lines
+86
to
+97
| public void OnPartyData() | ||
| { | ||
| try | ||
| { | ||
| if (Game.Party.Members == null) | ||
| { | ||
| OnPartyDismiss(); | ||
| return; | ||
| } | ||
| } | ||
| catch { } | ||
| } |
There was a problem hiding this comment.
Empty catch block silently swallows exceptions.
The empty catch { } hides potential errors. If Game.Party is null, the exception is silently ignored. Consider either logging the exception or handling the null case explicitly.
🛠️ Proposed fix
public void OnPartyData()
{
- try
- {
- if (Game.Party.Members == null)
- {
- OnPartyDismiss();
- return;
- }
- }
- catch { }
+ if (Game.Party?.Members == null)
+ {
+ OnPartyDismiss();
+ }
}📝 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
| public void OnPartyData() | |
| { | |
| try | |
| { | |
| if (Game.Party.Members == null) | |
| { | |
| OnPartyDismiss(); | |
| return; | |
| } | |
| } | |
| catch { } | |
| } | |
| public void OnPartyData() | |
| { | |
| if (Game.Party?.Members == null) | |
| { | |
| OnPartyDismiss(); | |
| } | |
| } |
🤖 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 `@Plugins/RSBot.Party/PartyManager.cs` around lines 86 - 97, The empty catch in
OnPartyData silently swallows exceptions; modify OnPartyData to explicitly check
for nulls (e.g., if (Game == null || Game.Party == null) { OnPartyDismiss();
return; }) instead of relying on an exception, and remove the empty catch block;
if you still need to catch unexpected errors, catch Exception ex and log it via
the plugin logger (or similar) with context ("OnPartyData failed") while
preserving existing call to OnPartyDismiss when appropriate so Game.Party nulls
are handled deterministically.
Comment on lines
+62
to
+66
| if (this.InvokeRequired) | ||
| { | ||
| this.Invoke(new Action<uint, ItemPerk>(OnRemoveItemPerk), targetId, removedPerk); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify all marshaling sites in this file and whether lifecycle guards are present.
fd -i "Main.cs" | xargs rg -n "InvokeRequired|\\.Invoke\\(|BeginInvoke|IsHandleCreated|IsDisposed|Disposing"Repository: Silkroad-Developer-Community/RSBot
Length of output: 5778
🏁 Script executed:
# Check the file exists and read the exact lines to verify the pattern
head -n 70 "Plugins/RSBot.Skills/Views/Main.cs" | tail -n 15Repository: Silkroad-Developer-Community/RSBot
Length of output: 659
🏁 Script executed:
# Also verify class declaration to ensure it inherits from Control/Form
head -n 20 "Plugins/RSBot.Skills/Views/Main.cs"Repository: Silkroad-Developer-Community/RSBot
Length of output: 604
🏁 Script executed:
# Find DoubleBufferedControl definition to confirm it's a Control subclass
fd -i "DoubleBufferedControl.cs" -o -i "DoubleBufferedControl.ts" | head -5Repository: Silkroad-Developer-Community/RSBot
Length of output: 195
🏁 Script executed:
# Check if this file has any lifecycle guards anywhere
rg -n "IsDisposed|Disposing|IsHandleCreated" "Plugins/RSBot.Skills/Views/Main.cs"Repository: Silkroad-Developer-Community/RSBot
Length of output: 60
🏁 Script executed:
# Find DoubleBufferedControl class definition
find . -name "*.cs" -type f | xargs rg -l "class DoubleBufferedControl" | head -1Repository: Silkroad-Developer-Community/RSBot
Length of output: 60
🏁 Script executed:
# Check what the actual event handlers do (are they lightweight or could blocking be an issue?)
sed -n '62,68p; 89,95p; 483,489p' "Plugins/RSBot.Skills/Views/Main.cs"Repository: Silkroad-Developer-Community/RSBot
Length of output: 718
🏁 Script executed:
# Search for DoubleBufferedControl across all files
rg -n "class DoubleBufferedControl" --type csRepository: Silkroad-Developer-Community/RSBot
Length of output: 60
🏁 Script executed:
# Check if SDUI.Controls is external or part of this project
find . -name "*.cs" | xargs rg -l "namespace SDUI.Controls" | head -3Repository: Silkroad-Developer-Community/RSBot
Length of output: 60
Harden UI-thread marshaling for control lifecycle and avoid blocking event threads.
The new pattern lacks lifecycle guards and uses synchronous Invoke. Events can fire during control disposal or before handle creation, causing crashes. Also, Invoke blocks the calling thread (event source). Add guards (IsDisposed || Disposing || !IsHandleCreated) and use BeginInvoke for event callbacks.
Proposed fix pattern (apply to all added marshaling blocks)
+ if (IsDisposed || Disposing || !IsHandleCreated)
+ return;
+
if (this.InvokeRequired)
{
- this.Invoke(new Action<uint, ItemPerk>(OnRemoveItemPerk), targetId, removedPerk);
+ BeginInvoke(new Action<uint, ItemPerk>(OnRemoveItemPerk), targetId, removedPerk);
return;
}Applies to lines: 62-66, 89-93, 483-487, 506-510, 540-544, 556-560, 571-575, 585-589, 600-604
📝 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
| if (this.InvokeRequired) | |
| { | |
| this.Invoke(new Action<uint, ItemPerk>(OnRemoveItemPerk), targetId, removedPerk); | |
| return; | |
| } | |
| if (IsDisposed || Disposing || !IsHandleCreated) | |
| return; | |
| if (this.InvokeRequired) | |
| { | |
| BeginInvoke(new Action<uint, ItemPerk>(OnRemoveItemPerk), targetId, removedPerk); | |
| 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 `@Plugins/RSBot.Skills/Views/Main.cs` around lines 62 - 66, The UI-thread
marshaling calls (e.g., the Invoke call that calls OnRemoveItemPerk) should be
hardened: before marshaling check control lifecycle flags (if IsDisposed ||
Disposing || !IsHandleCreated) and return early to avoid raising events on a
torn-down control, and replace synchronous Invoke with asynchronous BeginInvoke
to avoid blocking the caller thread; apply this pattern to all similar blocks
(the Invoke usages that call OnRemoveItemPerk and the other marshaled event
callbacks referenced in the review) so each uses the lifecycle guard and
BeginInvoke.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
Application/RSBot/Program.cs (1)
134-145:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winInfinite loop with no exit condition.
The
runningvariable is initialized totruebut never set tofalse. Users have no way to gracefully exit the headless REPL loop.🐛 Proposed fix to add exit command handling
bool running = true; while (running) { Console.Write("> "); var input = Console.ReadLine()?.Split(','); if (input == null || input.Length == 0) continue; var command = input[0].ToLower(); var args = input.Skip(1).ToArray(); + if (command == "exit" || command == "quit") + { + running = false; + Console.WriteLine("Exiting..."); + Kernel.Bot?.Stop(); + continue; + } + CLIManager.Execute(command, args); }🤖 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 `@Application/RSBot/Program.cs` around lines 134 - 145, The REPL loop never changes running so it never exits; modify the loop around the while (running) block to detect exit commands (e.g., when command equals "exit", "quit", or "bye") after reading and normalizing input and then set running = false or break the loop to terminate gracefully; ensure this check occurs before calling CLIManager.Execute(command, args) so the exit command does not get dispatched to CLIManager and handle empty/null input as before.
🧹 Nitpick comments (3)
Library/RSBot.Core/Components/Command/CLIManager.cs (2)
15-15: ⚡ Quick winPrefer
ToLowerInvariant()for command name normalization.Using
ToLowerInvariant()instead ofToLower()ensures culture-independent matching of command names, which is a best practice for identifiers like command names.♻️ Proposed fix
public static void Register(ICLICommand command) { - _commands[command.Name.ToLower()] = command; + _commands[command.Name.ToLowerInvariant()] = command; }🤖 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 `@Library/RSBot.Core/Components/Command/CLIManager.cs` at line 15, The command registration uses culture-sensitive ToLower() which can cause incorrect matching; update the normalization to use ToLowerInvariant() when indexing the _commands dictionary (replace command.Name.ToLower() with command.Name.ToLowerInvariant()) in the CLIManager registration logic so command name lookups are culture-independent.
25-25: ⚡ Quick winPrefer
ToLowerInvariant()for command lookup.Using
ToLowerInvariant()instead ofToLower()ensures culture-independent command matching.♻️ Proposed fix
public static void Execute(string commandName, string[] args) { - if (_commands.TryGetValue(commandName.ToLower(), out var command)) + if (_commands.TryGetValue(commandName.ToLowerInvariant(), out var command)) { command.Execute(args); }🤖 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 `@Library/RSBot.Core/Components/Command/CLIManager.cs` at line 25, The lookup uses commandName.ToLower(), which is culture-sensitive; change it to commandName.ToLowerInvariant() where the dictionary lookup occurs (the _commands.TryGetValue call that uses commandName.ToLower()). Update any other occurrences in CLIManager that normalize command names (e.g., any code that previously called ToLower on commandName or keys when adding/normalizing entries) to use ToLowerInvariant so command matching is culture-independent.Application/RSBot/Program.cs (1)
141-141: ⚡ Quick winPrefer
ToLowerInvariant()for command normalization.Using
ToLowerInvariant()instead ofToLower()ensures culture-independent command matching, which is a best practice for command identifiers.♻️ Proposed fix
- var command = input[0].ToLower(); + var command = input[0].ToLowerInvariant(); var args = input.Skip(1).ToArray();🤖 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 `@Application/RSBot/Program.cs` at line 141, The command normalization currently uses input[0].ToLower(); change it to use ToLowerInvariant() for culture-independent matching by replacing the call in the Program.cs line that sets the local variable command (var command = input[0].ToLower();) with ToLowerInvariant(); ensure this applies to the same variable initialization where input is read so command comparisons remain consistent across cultures.
🤖 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 `@Plugins/RSBot.General/GeneralCLICommands.cs`:
- Around line 13-17: The Execute method currently fire-and-forgets
GeneralPlugin.Instance.Manager.StartClientAsync(), which swallows exceptions and
causes Log.Notify("Client started") to run too early; update Execute to either
synchronously block and propagate errors by calling
StartClientAsync().GetAwaiter().GetResult() (or Task.Run(() =>
StartClientAsync()).GetAwaiter().GetResult()) before calling Log.Notify, or
attach a continuation to the returned Task
(StartClientAsync().ContinueWith(...)) that logs success when completed and
logs/handles exceptions from the Task so failures aren't swallowed; adjust usage
of StartClientAsync, Execute, and Log.Notify accordingly.
---
Duplicate comments:
In `@Application/RSBot/Program.cs`:
- Around line 134-145: The REPL loop never changes running so it never exits;
modify the loop around the while (running) block to detect exit commands (e.g.,
when command equals "exit", "quit", or "bye") after reading and normalizing
input and then set running = false or break the loop to terminate gracefully;
ensure this check occurs before calling CLIManager.Execute(command, args) so the
exit command does not get dispatched to CLIManager and handle empty/null input
as before.
---
Nitpick comments:
In `@Application/RSBot/Program.cs`:
- Line 141: The command normalization currently uses input[0].ToLower(); change
it to use ToLowerInvariant() for culture-independent matching by replacing the
call in the Program.cs line that sets the local variable command (var command =
input[0].ToLower();) with ToLowerInvariant(); ensure this applies to the same
variable initialization where input is read so command comparisons remain
consistent across cultures.
In `@Library/RSBot.Core/Components/Command/CLIManager.cs`:
- Line 15: The command registration uses culture-sensitive ToLower() which can
cause incorrect matching; update the normalization to use ToLowerInvariant()
when indexing the _commands dictionary (replace command.Name.ToLower() with
command.Name.ToLowerInvariant()) in the CLIManager registration logic so command
name lookups are culture-independent.
- Line 25: The lookup uses commandName.ToLower(), which is culture-sensitive;
change it to commandName.ToLowerInvariant() where the dictionary lookup occurs
(the _commands.TryGetValue call that uses commandName.ToLower()). Update any
other occurrences in CLIManager that normalize command names (e.g., any code
that previously called ToLower on commandName or keys when adding/normalizing
entries) to use ToLowerInvariant so command matching is culture-independent.
🪄 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 Plus
Run ID: 5c8b3b42-fc8e-4613-a8ef-c98e465877ba
📒 Files selected for processing (14)
Application/RSBot/Program.csApplication/RSBot/RSBot.csprojLibrary/RSBot.Core/Bot.csLibrary/RSBot.Core/Components/Command/CLIManager.csLibrary/RSBot.Core/Components/Command/CoreCLICommands.csLibrary/RSBot.Core/Components/Command/ICLICommand.csLibrary/RSBot.Core/Kernel.csLibrary/RSBot.Core/Plugins/BotbaseManager.csPlugins/RSBot.Chat/ChatCLICommands.csPlugins/RSBot.Chat/ChatPlugin.csPlugins/RSBot.General/GeneralCLICommands.csPlugins/RSBot.General/GeneralPlugin.csPlugins/RSBot.Python/Components/API/ModuleLoader/StubGenerator.csPlugins/RSBot.Python/Components/Loader/PythonRuntimeManager.cs
✅ Files skipped from review due to trivial changes (3)
- Library/RSBot.Core/Components/Command/ICLICommand.cs
- Plugins/RSBot.Python/Components/API/ModuleLoader/StubGenerator.cs
- Plugins/RSBot.Python/Components/Loader/PythonRuntimeManager.cs
🚧 Files skipped from review as they are similar to previous changes (4)
- Plugins/RSBot.Chat/ChatPlugin.cs
- Library/RSBot.Core/Bot.cs
- Plugins/RSBot.General/GeneralPlugin.cs
- Library/RSBot.Core/Plugins/BotbaseManager.cs
- Stable console cursor - Fixes to some errors - Help messages outputted only to console
Egezenn
merged commit fd78363
into
Silkroad-Developer-Community:main
1 of 2 checks passed
Egezenn
added a commit
that referenced
this pull request
May 21, 2026
* Rebrand to OasisBot - Requires changes to #8 - Icons made by @kis1yi - Theme accent changed to (202, 179, 139) #CAB38B | (182, 159, 119) #B69F77 - Text references changed to OasisBot - Changes links to OasisBot (https://github.com/Silkroad-Developer-Community/RSBot -> https://github.com/Silkroad-Developer-Community/OasisBot) - Internals untouched * Update SDUI reference and finalize OasisBot rebranding * DMCA * Revert copyright notice for Python - Never merged - All contributions from current org members -> @Day4Date, @kis1yi, @Egezenn - myildirimofficial/RSBot#965 * Decoupling UI and Logic (#1) * Added IBotbaseView and IPluginView Interfaces Changed IBotbase and IPlugin Interface Decoupled Chat Plugin * General Plugin decoupled Subviews still have to be decoupled * Added Inventory Plugin to new Interface * Decoupled Training Botbase Removed Pickepet Timer from UI and created new one in TrainingManager.cs * Added new Interface to all existing Plugins and Botbases * Test * Decoupled Skills Plugin Added headless prototyp * --headless starts now without Main UI Improved decoupling for Party Plugin * removed launchSettings * test * removed "h" keyword for headless launch * Fix for General Plugin * Decoupled ServerInfo Plugin * invoked skills plugin * Reworked Party decoupling * Some spelling * Stabilize behavior * Command registries for plugins, headless attach * Lower, start-client, exit commands * Fixes and improvements - Stable console cursor - Fixes to some errors - Help messages outputted only to console --------- Co-authored-by: DayDate <DayDate> Co-authored-by: Egezenn <egezenn@tutanota.com> * Tasks, autoupdate, general config, robust profile system (#8) - Fetches every 12h (only once via general config caching the last check) RSBot.AutoUpdate{True} RSBot.Language{en_US} RSBot.showExitDialog{True} RSBot.Theme.Auto{True} RSBot.LastUpdateCheck{638506116000000000} * Move Application/RSBot -> Application/OasisBot, Move URL dependencies - Break stuff once
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.
With this PR i will decouple the UI and the Logic.
This is still Work in Progress.
My approch is to add a new Interface for the view (IPluginView / IBotbaseView) and use the old one (IPlugin / IBotbase) for the logic.
For now, everything should work as always. I added the new Interfaces for all existing Plugins/Botbases.
Decoupled Plugins (so far):
Summary by CodeRabbit
New Features
Refactor
Bug Fixes