feat: embed DragonWidgets as shared widget library in DragonToast_Options#147
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 6 minutes and 18 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughAdded a DragonWidgets external dependency and submodule; removed almost all local widget and layout modules and migrated options panel creation to Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Suggested labels
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 2
🧹 Nitpick comments (2)
DragonToast_Options/DragonToast_Options.toc (1)
1-3: Switch these client interface declarations to the repo’s packager-guard pattern.BigWigs packager supports
#@keyword@ ... #@end-keyword@blocks in TOC files, so this shared TOC can follow the project’s preferred flavor-splitting pattern instead of keeping extra## Interface-*entries inline. (github.com)Based on learnings: "Applies to .toc : Use version-specific loading via BigWigsMods packager comment directives in the TOC file (e.g.,
#retail@,#tbc-anniversary@,#version-mists@), not ## Interface- mid-file directives"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@DragonToast_Options/DragonToast_Options.toc` around lines 1 - 3, Replace the inline Interface declarations (the lines "## Interface: 120001", "## Interface-BCC: 20505", "## Interface-Mists: 50503") with the repo’s packager-guard blocks using BigWigs packager directives (e.g. the #@keyword@ ... #@end-keyword@ pattern) so version-specific loading is handled by the packager; locate the shared TOC entries (the three Interface lines) and wrap the appropriate TOC content in the corresponding packager guards such as #@retail@ / #@end-retail@, #@tbc-anniversary@ / #@end-tbc-anniversary@ or #@version-mists@ / #@end-version-mists@ to replace the Interface-* lines.DragonToast_Options/Core.lua (1)
87-95: Keep the panel-creation failure path consistent.Lines 105-108 and 118-121 already treat
CreateOptionsPanel()as a recoverable failure. Line 95 is the one remaining hard dereference, so ifDW.CreateOptionsPanel()returnsnil,ns.RefreshVisibleWidgetsbecomes the first crash instead of lettingOpen()/Toggle()bail out cleanly.🛡️ Suggested guard
panelResult = DW.CreateOptionsPanel({ name = "DragonToastOptionsFrame", title = "DragonToast Options", width = 800, height = 600, tabs = tabDefs, }) - ns.RefreshVisibleWidgets = panelResult.RefreshVisibleWidgets + if not panelResult then + ns.RefreshVisibleWidgets = nil + return + end + + ns.RefreshVisibleWidgets = panelResult.RefreshVisibleWidgets endAs per coding guidelines,
**/*.luashould be reviewed for nil safety and logic errors.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@DragonToast_Options/Core.lua` around lines 87 - 95, panelResult may be nil from DW.CreateOptionsPanel(), but the code currently unconditionally assigns ns.RefreshVisibleWidgets = panelResult.RefreshVisibleWidgets which will crash; change this to guard the assignment so it only sets ns.RefreshVisibleWidgets when panelResult is non-nil (e.g. check panelResult then assign), leaving Open() / Toggle() to handle a missing panel consistently; reference the symbols DW.CreateOptionsPanel, panelResult, ns.RefreshVisibleWidgets, Open, and Toggle when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@DragonToast_Options/Core.lua`:
- Around line 26-29: The luacheck warning comes from referencing the global
DragonWidgetsNS implicitly; update the binding for DW by reading the global
explicitly (use _G.DragonWidgetsNS) so the linter recognizes the external
dependency, leaving the existing nil guard (the if not DW then error(...) block)
unchanged to preserve runtime safety.
In `@DragonToast_Options/Tabs/ProfilesTab.lua`:
- Around line 214-215: The StaticPopup_Show call in the set = function(value)
for the profile selector is passing the profile name as a text substitution (2nd
arg) instead of as dialog data, so the dialog's OnAccept handler (which reads
self.data and calls DeleteProfile) never receives the profile and does nothing;
update the StaticPopup_Show invocation for "DRAGONTOAST_OPTIONS_DELETE_PROFILE"
to supply the selected profile name as the dialog data (4th argument) so
OnAccept can read self.data and call DeleteProfile correctly.
---
Nitpick comments:
In `@DragonToast_Options/Core.lua`:
- Around line 87-95: panelResult may be nil from DW.CreateOptionsPanel(), but
the code currently unconditionally assigns ns.RefreshVisibleWidgets =
panelResult.RefreshVisibleWidgets which will crash; change this to guard the
assignment so it only sets ns.RefreshVisibleWidgets when panelResult is non-nil
(e.g. check panelResult then assign), leaving Open() / Toggle() to handle a
missing panel consistently; reference the symbols DW.CreateOptionsPanel,
panelResult, ns.RefreshVisibleWidgets, Open, and Toggle when making the change.
In `@DragonToast_Options/DragonToast_Options.toc`:
- Around line 1-3: Replace the inline Interface declarations (the lines "##
Interface: 120001", "## Interface-BCC: 20505", "## Interface-Mists: 50503") with
the repo’s packager-guard blocks using BigWigs packager directives (e.g. the
#@keyword@ ... #@end-keyword@ pattern) so version-specific loading is handled by
the packager; locate the shared TOC entries (the three Interface lines) and wrap
the appropriate TOC content in the corresponding packager guards such as
#@retail@ / #@end-retail@, #@tbc-anniversary@ / #@end-tbc-anniversary@ or
#@version-mists@ / #@end-version-mists@ to replace the Interface-* lines.
🪄 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: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: e597e2bd-7ce5-4fb0-9884-4051c9711c3b
📒 Files selected for processing (22)
.pkgmetaDragonToast_Options/Core.luaDragonToast_Options/DragonToast_Options.tocDragonToast_Options/LayoutConstants.luaDragonToast_Options/Tabs/AnimationTab.luaDragonToast_Options/Tabs/AppearanceTab.luaDragonToast_Options/Tabs/DisplayTab.luaDragonToast_Options/Tabs/FiltersTab.luaDragonToast_Options/Tabs/GeneralTab.luaDragonToast_Options/Tabs/ProfilesTab.luaDragonToast_Options/Widgets/Button.luaDragonToast_Options/Widgets/ColorPicker.luaDragonToast_Options/Widgets/Description.luaDragonToast_Options/Widgets/Dropdown.luaDragonToast_Options/Widgets/Header.luaDragonToast_Options/Widgets/Panel.luaDragonToast_Options/Widgets/ScrollFrame.luaDragonToast_Options/Widgets/Slider.luaDragonToast_Options/Widgets/TabGroup.luaDragonToast_Options/Widgets/TextInput.luaDragonToast_Options/Widgets/Toggle.luaDragonToast_Options/Widgets/WidgetConstants.lua
💤 Files with no reviewable changes (13)
- DragonToast_Options/Widgets/WidgetConstants.lua
- DragonToast_Options/Widgets/Description.lua
- DragonToast_Options/Widgets/ColorPicker.lua
- DragonToast_Options/Widgets/Panel.lua
- DragonToast_Options/LayoutConstants.lua
- DragonToast_Options/Widgets/ScrollFrame.lua
- DragonToast_Options/Widgets/Button.lua
- DragonToast_Options/Widgets/TextInput.lua
- DragonToast_Options/Widgets/Slider.lua
- DragonToast_Options/Widgets/Header.lua
- DragonToast_Options/Widgets/Toggle.lua
- DragonToast_Options/Widgets/TabGroup.lua
- DragonToast_Options/Widgets/Dropdown.lua
| local DW = DragonWidgetsNS | ||
| if not DW then | ||
| error("[DragonToast_Options] DragonWidgets is not loaded. Ensure DragonWidgets is installed and enabled.", 2) | ||
| end |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "*.toc" -type f | head -20Repository: Xerrion/DragonToast
Length of output: 138
🏁 Script executed:
find . -name ".pkgmeta" -type fRepository: Xerrion/DragonToast
Length of output: 73
🏁 Script executed:
cat DragonToast_Options/DragonToast_Options.tocRepository: Xerrion/DragonToast
Length of output: 1159
🏁 Script executed:
if [ -f ".pkgmeta" ]; then cat ".pkgmeta"; fiRepository: Xerrion/DragonToast
Length of output: 1056
🏁 Script executed:
head -50 "DragonToast_Options/Libs/DragonWidgets/DragonWidgets.lua"Repository: Xerrion/DragonToast
Length of output: 178
🏁 Script executed:
sed -n '20,35p' "DragonToast_Options/Core.lua"Repository: Xerrion/DragonToast
Length of output: 662
Address the luacheck lint warning by making the embedded library dependency explicit.
The embedded DragonWidgets.lua is correctly listed first in the .toc file and properly mapped in .pkgmeta, so the load order is sound. However, line 26 references DragonWidgetsNS as an undeclared global, triggering luacheck W113. Use _G.DragonWidgetsNS to make the global dependency explicit and silence the linter:
🔧 Suggested fix
-local DW = DragonWidgetsNS
+local DW = _G.DragonWidgetsNS
if not DW then
error("[DragonToast_Options] DragonWidgets is not loaded. Ensure DragonWidgets is installed and enabled.", 2)
endThe nil guard is in place, so runtime safety is already covered.
🧰 Tools
🪛 GitHub Actions: Lint
[warning] 26-26: luacheck warning (W113): accessing undefined variable 'DragonWidgetsNS'
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@DragonToast_Options/Core.lua` around lines 26 - 29, The luacheck warning
comes from referencing the global DragonWidgetsNS implicitly; update the binding
for DW by reading the global explicitly (use _G.DragonWidgetsNS) so the linter
recognizes the external dependency, leaving the existing nil guard (the if not
DW then error(...) block) unchanged to preserve runtime safety.
| set = function(value) | ||
| local dialog = StaticPopup_Show("DRAGONTOAST_OPTIONS_DELETE_PROFILE", value) | ||
| if dialog then | ||
| dialog.data = value | ||
| end | ||
| StaticPopup_Show("DRAGONTOAST_OPTIONS_DELETE_PROFILE", value) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "ProfilesTab.lua" -type fRepository: Xerrion/DragonToast
Length of output: 105
🏁 Script executed:
rg -n "StaticPopup_Show\|DRAGONTOAST_OPTIONS_DELETE_PROFILE" --type luaRepository: Xerrion/DragonToast
Length of output: 45
🏁 Script executed:
sed -n '200,230p' DragonToast_Options/Tabs/ProfilesTab.luaRepository: Xerrion/DragonToast
Length of output: 1167
🏁 Script executed:
# Find the OnAccept handler definition for the delete popup dialog
rg -A 10 'StaticPopupDialogs\[.*DRAGONTOAST_OPTIONS_DELETE_PROFILE' DragonToast_Options/ --type luaRepository: Xerrion/DragonToast
Length of output: 931
🌐 Web query:
WoW API StaticPopup_Show arguments data fourth parameter
💡 Result:
The WoW API function StaticPopup_Show (also referred to as StaticPopup:Show) has the signature: success = StaticPopup_Show(which [, text_arg1, text_arg2, data, insertedFrame]) The fourth parameter is 'data', which is an optional table (can be nil). It is described as: "Data shared with the dialog, if applicable." This data is accessible in the popup's handlers (e.g., OnAccept, OnShow) via the frame's .data field. It is used to display item info (requires data.useLinkForItemInfo to be set) or to set up OnCancel options. Examples include passing item links or other custom data for use in callbacks.
Citations:
- 1: https://warcraft.wiki.gg/wiki/API_StaticPopup_Show
- 2: https://www.wowinterface.com/forums/showthread.php?t=58679
- 3: https://wowpedia.fandom.com/wiki/Creating_simple_pop-up_dialog_boxes
- 4: https://addonstudio.org/wiki/WoW:Creating_simple_pop-up_dialog_boxes
Pass the selected profile in the popup's data slot.
StaticPopup_Show accepts text substitutions in the 2nd and 3rd arguments and shared dialog data in the 4th argument. Since OnAccept reads self.data, the current call only passes value as a text substitution; the handler never receives the profile name and DeleteProfile silently no-ops.
Suggested fix
- StaticPopup_Show("DRAGONTOAST_OPTIONS_DELETE_PROFILE", value)
+ StaticPopup_Show("DRAGONTOAST_OPTIONS_DELETE_PROFILE", value, nil, value)📝 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.
| set = function(value) | |
| local dialog = StaticPopup_Show("DRAGONTOAST_OPTIONS_DELETE_PROFILE", value) | |
| if dialog then | |
| dialog.data = value | |
| end | |
| StaticPopup_Show("DRAGONTOAST_OPTIONS_DELETE_PROFILE", value) | |
| set = function(value) | |
| StaticPopup_Show("DRAGONTOAST_OPTIONS_DELETE_PROFILE", value, nil, value) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@DragonToast_Options/Tabs/ProfilesTab.lua` around lines 214 - 215, The
StaticPopup_Show call in the set = function(value) for the profile selector is
passing the profile name as a text substitution (2nd arg) instead of as dialog
data, so the dialog's OnAccept handler (which reads self.data and calls
DeleteProfile) never receives the profile and does nothing; update the
StaticPopup_Show invocation for "DRAGONTOAST_OPTIONS_DELETE_PROFILE" to supply
the selected profile name as the dialog data (4th argument) so OnAccept can read
self.data and call DeleteProfile correctly.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
DragonToast_Options/Tabs/ProfilesTab.lua (1)
209-211:⚠️ Potential issue | 🔴 CriticalPass the profile name as dialog
data(4th argument) soOnAcceptcan read it.The
OnAccepthandler at line 58 readsself.datato get the profile name, butStaticPopup_Showcurrently only passesvalueas a text substitution (2nd arg). The delete will silently no-op becauseself.datais nil.- StaticPopup_Show("DRAGONTOAST_OPTIONS_DELETE_PROFILE", value) + StaticPopup_Show("DRAGONTOAST_OPTIONS_DELETE_PROFILE", value, nil, value)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@DragonToast_Options/Tabs/ProfilesTab.lua` around lines 209 - 211, The delete dialog is not receiving the profile name in its OnAccept handler because StaticPopup_Show in the set function only supplies the text substitution (2nd arg); update the set function that calls StaticPopup_Show("DRAGONTOAST_OPTIONS_DELETE_PROFILE", value) to pass the profile name as the dialog data (fourth argument) so the popup's OnAccept (which reads self.data) can access it; ensure you still keep the text substitution if needed and reference the popup key "DRAGONTOAST_OPTIONS_DELETE_PROFILE" and the set handler where StaticPopup_Show is invoked.
🧹 Nitpick comments (1)
DragonToast_Options/Core.lua (1)
75-80: Silent failure on missing DragonToastNS may confuse users.When
DragonToastNSis missing,CreateOptionsPanelprints a warning and returns early, butOpen()at line 108 then also returns silently. Consider adding a user-visible message inOpen()whenpanelResultremains nil after creation attempt, or restructure to fail once with a clear message.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@DragonToast_Options/Core.lua` around lines 75 - 80, CreateOptionsPanel currently returns early when ns.dtns/_G.DragonToastNS is missing, causing Open() to silently no-op if panelResult stays nil; update Open() (the function that calls CreateOptionsPanel and checks panelResult) to detect a nil panelResult after attempting creation and show a clear user-visible message (e.g., print or UI popup) describing that DragonToastNS is missing and options cannot be opened, or alternatively move the missing-namespace check into Open() so you fail once with a clear message before calling CreateOptionsPanel; reference CreateOptionsPanel, Open, and panelResult when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@DragonToast_Options/Tabs/FiltersTab.lua`:
- Line 84: The table entry for key "showGroupLoot" in FiltersTab.lua has a
tooltip string that exceeds 120 chars; fix it by breaking the tooltip into two
shorter string literals concatenated with .. (or assign to a local variable) so
the line length is under 120, e.g. update the { key = "showGroupLoot", label =
L["Show Group Loot"], tooltip = ... } entry to use a concatenated tooltip (or
local tooltip variable) while keeping the L[...] lookup pieces the same.
In `@DragonToast_Options/Tabs/ProfilesTab.lua`:
- Line 119: The description string passed to W.CreateDescription (variable desc)
exceeds 120 chars; shorten or split it to satisfy luacheck. Replace the long
literal L["Profiles allow you to save different configurations for different
characters."] with either a shorter localization key or concatenate multiple
shorter string literals (e.g., L["Profiles allow you to save "] .. L["different
configurations for different characters."]) so the source line length for the
call to W.CreateDescription(parent, ...) is under 120 characters while
preserving the full text.
---
Duplicate comments:
In `@DragonToast_Options/Tabs/ProfilesTab.lua`:
- Around line 209-211: The delete dialog is not receiving the profile name in
its OnAccept handler because StaticPopup_Show in the set function only supplies
the text substitution (2nd arg); update the set function that calls
StaticPopup_Show("DRAGONTOAST_OPTIONS_DELETE_PROFILE", value) to pass the
profile name as the dialog data (fourth argument) so the popup's OnAccept (which
reads self.data) can access it; ensure you still keep the text substitution if
needed and reference the popup key "DRAGONTOAST_OPTIONS_DELETE_PROFILE" and the
set handler where StaticPopup_Show is invoked.
---
Nitpick comments:
In `@DragonToast_Options/Core.lua`:
- Around line 75-80: CreateOptionsPanel currently returns early when
ns.dtns/_G.DragonToastNS is missing, causing Open() to silently no-op if
panelResult stays nil; update Open() (the function that calls CreateOptionsPanel
and checks panelResult) to detect a nil panelResult after attempting creation
and show a clear user-visible message (e.g., print or UI popup) describing that
DragonToastNS is missing and options cannot be opened, or alternatively move the
missing-namespace check into Open() so you fail once with a clear message before
calling CreateOptionsPanel; reference CreateOptionsPanel, Open, and panelResult
when making the change.
🪄 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: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 6d97ba79-644a-4626-b924-3c6b1f048e12
⛔ Files ignored due to path filters (1)
DragonToast_Options/Libs/DragonWidgetsis excluded by!**/Libs/**
📒 Files selected for processing (11)
.gitmodules.luacheckrcDragonToast/Locales/deDE.luaDragonToast/Locales/enUS.luaDragonToast_Options/Core.luaDragonToast_Options/Tabs/AnimationTab.luaDragonToast_Options/Tabs/AppearanceTab.luaDragonToast_Options/Tabs/DisplayTab.luaDragonToast_Options/Tabs/FiltersTab.luaDragonToast_Options/Tabs/GeneralTab.luaDragonToast_Options/Tabs/ProfilesTab.lua
✅ Files skipped from review due to trivial changes (1)
- .gitmodules
🚧 Files skipped from review as they are similar to previous changes (2)
- DragonToast_Options/Tabs/AnimationTab.lua
- DragonToast_Options/Tabs/AppearanceTab.lua
1 participant
Summary
Replaces the inline widget stack in DragonToast_Options with DragonWidgets, a new standalone shared library. All widget files and LayoutConstants.lua are removed from this repo; the library is loaded inline from
Libs\DragonWidgets\via the TOC.Changes
DragonToast_Options/DragonToast_Options.toc: removedDragonWidgetsfromDependencies, added inlineLibs\DragonWidgets\file blockDragonToast_Options/Core.lua: rewired toDragonWidgetsNSbridge (ns.DW), addedns.RefreshVisibleWidgetsDragonToast_Options/Tabs/*.lua(6 files): updated to usens.DW.Widgetsandns.DW.LayoutConstantsDragonToast_Options/LayoutConstants.lua: deleted (moved to DragonWidgets)DragonToast_Options/Widgets/(12 files): deleted (moved to DragonWidgets).pkgmeta: addedDragonToast/DragonToast_Options/Libs/DragonWidgetsexternal pointing to https://github.com/Xerrion/DragonWidgetsTesting
Summary by CodeRabbit
Refactor
Chores
User-facing changes