Update SaveManager.lua #64
Conversation
WalkthroughReplaced the autoload button with a toggle control. The new AutoloadToggle’s OnChanged handler writes or deletes settings/autoload.txt and issues notifications. Initialization sets the toggle to false. Removed prior button description logic and initial read of the autoload file. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant UI as AutoloadToggle
participant SM as SaveManager
participant FS as FileSystem
participant Notif as Notification
User->>UI: Toggle change
UI->>SM: OnChanged(state)
alt state == enabled
SM->>FS: write "settings/autoload.txt" = currentConfigName
FS-->>SM: write result
SM->>Notif: notify "Autoload enabled"
else state == disabled
SM->>FS: delete "settings/autoload.txt" (if exists)
FS-->>SM: delete result
SM->>Notif: notify "Autoload disabled"
end
note over UI,SM: Initialization sets toggle to false (no autoload read on startup)
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
Addons/SaveManager.lua (1)
326-326: Use English for code comments.The comment on line 326 is in Thai. For better maintainability and collaboration, use English in code comments.
Apply this diff:
--- ตั้งค่าเริ่มต้นเป็นปิดไว้ก่อน +-- Set initial toggle state to off Options.AutoloadToggle:SetValue(false)(Note: This line should likely be removed entirely per the previous comment about redundancy.)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Addons/SaveManager.lua(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
Addons/SaveManager.lua (1)
src/Elements/Toggle.lua (1)
Toggle:OnChanged(63-66)
| local Toggle = section:AddToggle("AutoloadToggle", { | ||
| Title = "Auto Load Config", | ||
| Default = false, | ||
| Description = "Toggle to set/unset autoload config" | ||
| }) | ||
|
|
||
| Toggle:OnChanged(function(state) | ||
| local name = SaveManager.Options.SaveManager_ConfigList.Value | ||
| local autoloadPath = self.Folder .. "/settings/autoload.txt" | ||
|
|
||
| if state then | ||
| writefile(autoloadPath, name) | ||
| self.Library:Notify({ | ||
| Title = "Interface", | ||
| Content = "Config loader", | ||
| SubContent = string.format("Set %q to auto load", name), | ||
| Duration = 7 | ||
| }) | ||
| else | ||
| if isfile(autoloadPath) then | ||
| delfile(autoloadPath) | ||
| end | ||
| self.Library:Notify({ | ||
| Title = "Interface", | ||
| Content = "Config loader", | ||
| SubContent = "Autoload has been disabled", | ||
| Duration = 7 | ||
| }) | ||
| end | ||
| end) | ||
|
|
||
| if isfile(self.Folder .. "/settings/autoload.txt") then | ||
| local name = readfile(self.Folder .. "/settings/autoload.txt") | ||
| AutoloadButton:SetDesc("Current autoload config: " .. name) | ||
| end | ||
| -- ตั้งค่าเริ่มต้นเป็นปิดไว้ก่อน | ||
| Options.AutoloadToggle:SetValue(false) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Toggle state doesn't reflect actual autoload status on initialization.
The toggle always starts as false, even if autoload.txt exists. This creates a mismatch between the UI state and the actual autoload configuration. Users won't know that autoload is active until they manually check or toggle the control.
Consider initializing the toggle based on file existence:
local Toggle = section:AddToggle("AutoloadToggle", {
Title = "Auto Load Config",
- Default = false,
+ Default = isfile(self.Folder .. "/settings/autoload.txt"),
Description = "Toggle to set/unset autoload config"
})Then register the OnChanged handler. This ensures the UI accurately represents the autoload state.
🤖 Prompt for AI Agents
In Addons/SaveManager.lua around lines 295-327, the autoload toggle always
initializes to false and uses Options.AutoloadToggle:SetValue(false), causing a
UI/config mismatch; fix by computing local autoloadPath = self.Folder ..
"/settings/autoload.txt" first, check isfile(autoloadPath) and set the toggle
state accordingly via Toggle:SetValue(true) if the file exists (or
Toggle:SetValue(false) if not), and only then register the Toggle:OnChanged
handler (i.e., move the OnChanged hookup after setting the initial value) so the
UI reflects actual autoload status on startup and you use the correct Toggle
reference instead of Options.AutoloadToggle.
| Toggle:OnChanged(function(state) | ||
| local name = SaveManager.Options.SaveManager_ConfigList.Value | ||
| local autoloadPath = self.Folder .. "/settings/autoload.txt" | ||
|
|
||
| if state then | ||
| writefile(autoloadPath, name) | ||
| self.Library:Notify({ | ||
| Title = "Interface", | ||
| Content = "Config loader", | ||
| SubContent = string.format("Set %q to auto load", name), | ||
| Duration = 7 | ||
| }) | ||
| else | ||
| if isfile(autoloadPath) then | ||
| delfile(autoloadPath) | ||
| end | ||
| self.Library:Notify({ | ||
| Title = "Interface", | ||
| Content = "Config loader", | ||
| SubContent = "Autoload has been disabled", | ||
| Duration = 7 | ||
| }) | ||
| end | ||
| end) |
There was a problem hiding this comment.
OnChanged fires on initialization, causing unwanted notification.
Based on the Toggle:OnChanged implementation in src/Elements/Toggle.lua (lines 62-65), the handler is invoked immediately with the current toggle value. Since the toggle defaults to false, users will see an "Autoload has been disabled" notification on every UI load, even though autoload was never active in that session.
Consider one of these approaches:
Option 1: Add a flag to suppress the initial notification:
+local initializing = true
Toggle:OnChanged(function(state)
local name = SaveManager.Options.SaveManager_ConfigList.Value
local autoloadPath = self.Folder .. "/settings/autoload.txt"
if state then
writefile(autoloadPath, name)
self.Library:Notify({
Title = "Interface",
Content = "Config loader",
SubContent = string.format("Set %q to auto load", name),
Duration = 7
})
else
+ if initializing then
+ initializing = false
+ return
+ end
if isfile(autoloadPath) then
delfile(autoloadPath)
endOption 2: Initialize the toggle state based on whether autoload.txt exists before registering the handler.
| Toggle:OnChanged(function(state) | ||
| local name = SaveManager.Options.SaveManager_ConfigList.Value | ||
| local autoloadPath = self.Folder .. "/settings/autoload.txt" | ||
|
|
||
| if state then | ||
| writefile(autoloadPath, name) | ||
| self.Library:Notify({ | ||
| Title = "Interface", | ||
| Content = "Config loader", | ||
| SubContent = string.format("Set %q to auto load", name), | ||
| Duration = 7 | ||
| }) |
There was a problem hiding this comment.
Validate config selection before writing to autoload.
The handler writes name to the autoload file without verifying that a config is actually selected. If SaveManager_ConfigList.Value is nil or empty, this will write invalid data to autoload.txt, causing LoadAutoloadConfig() to fail silently or load nothing.
Apply this diff to add validation:
Toggle:OnChanged(function(state)
local name = SaveManager.Options.SaveManager_ConfigList.Value
local autoloadPath = self.Folder .. "/settings/autoload.txt"
if state then
+ if not name or name == "" then
+ self.Library:Notify({
+ Title = "Interface",
+ Content = "Config loader",
+ SubContent = "Please select a config first",
+ Duration = 7
+ })
+ Toggle:SetValue(false)
+ return
+ end
writefile(autoloadPath, name)🤖 Prompt for AI Agents
In Addons/SaveManager.lua around lines 301 to 312, the Toggle OnChanged handler
writes SaveManager.Options.SaveManager_ConfigList.Value directly to autoload.txt
without validating it; this can write nil/empty values. Before calling
writefile, check that name is a non-empty string (not nil and not just
whitespace); if invalid, do not write and instead call self.Library:Notify with
an error/warning message indicating no config selected; only proceed to
writefile and show the success notification when validation passes.
| -- ตั้งค่าเริ่มต้นเป็นปิดไว้ก่อน | ||
| Options.AutoloadToggle:SetValue(false) |
There was a problem hiding this comment.
Remove redundant SetValue call.
Line 327 calls SetValue(false) after the OnChanged handler has already been registered. Since the toggle's Default is already false (line 297) and OnChanged invokes the handler immediately, this second call is unnecessary and will trigger the handler again, potentially showing duplicate notifications.
Apply this diff to remove the redundant call:
end)
--- ตั้งค่าเริ่มต้นเป็นปิดไว้ก่อน
-Options.AutoloadToggle:SetValue(false)📝 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.
| -- ตั้งค่าเริ่มต้นเป็นปิดไว้ก่อน | |
| Options.AutoloadToggle:SetValue(false) | |
| end) |
🤖 Prompt for AI Agents
In Addons/SaveManager.lua around lines 326-327, the call
Options.AutoloadToggle:SetValue(false) is redundant because Default is already
false and the OnChanged handler triggers immediately when registered; remove
this SetValue(false) invocation to avoid invoking the handler twice (thus
preventing duplicate notifications) and keep only the Default configuration and
the OnChanged registration.
1 participant
Summary by CodeRabbit