fix: address CodeRabbit code quality findings#133
Merged
Conversation
- Add nil-safety guards to DisplayUtils.ApplyBackdrop, LootAnimations, RollAnimations, RollManager StaticPopup, RollFrame OnRollButtonClick, and LootListener_Retail OnLootReady - Add GetHistoryDB() guarded accessor to HistoryFrame - Fix non-animated path missing frame:Hide() in LootAnimations and RollAnimations - Fix FillMissingDefaults skipping top-level scalar defaults (enabled, debug) with shared ApplyScalarDefaults helper - Fix ResetToDefaults not resetting top-level scalar defaults - Add fmt-check to justfile check recipe Closes #132
📝 WalkthroughWalkthroughAdded nil-safety guards across display and listener modules, introduced top-level scalar defaults handling in core config, and updated the build Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
DragonLoot/Core/Config.lua (1)
159-168:⚠️ Potential issue | 🟠 MajorGuard section type before indexing to avoid migration-time Lua errors.
At Line 164,
if not profile[section] thenmisses truthy non-table values (e.g., a corrupted SavedVariables string/bool). Then Line 168 indexesprofile[section][key]and can throw. Treat non-table sections as invalid and reset them to{}before key iteration.Suggested fix
local function FillMissingDefaults(profile) ApplyScalarDefaults(profile, false) -- Fill section (table) defaults for section, sectionDefaults in pairs(defaults.profile) do if type(sectionDefaults) == "table" then - if not profile[section] then + if type(profile[section]) ~= "table" then profile[section] = {} end for key, defaultValue in pairs(sectionDefaults) do local currentValue = profile[section][key]As per coding guidelines "
**/*.lua: Lua addon code. Review for nil safety, global leaks, and logic errors."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@DragonLoot/Core/Config.lua` around lines 159 - 168, The loop after ApplyScalarDefaults iterates defaults.profile but only checks `if not profile[section] then` which fails when profile[section] exists but is not a table; before indexing profile[section][key] ensure profile[section] is a table by checking type(profile[section]) ~= "table" and if so replace it with an empty table (profile[section] = {}), then proceed to iterate keys; update the section/type guard around the loop that processes sectionDefaults in the code that references defaults.profile so non-table/corrupted SavedVariables are reset safely.
🧹 Nitpick comments (2)
DragonLoot/Display/RollAnimations.lua (1)
40-45: Consider hardening nestedrollFrame/animationreads.Line [40] and several animation reads still assume nested subtables exist once
profileexists. Using local fallback tables would make this path resilient to partially missing config.As per coding guidelines
**/*.lua: Lua addon code. Review for nil safety, global leaks, and logic errors.Also applies to: 56-63, 90-111
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@DragonLoot/Display/RollAnimations.lua` around lines 40 - 45, Reads assume db.rollFrame and its animation subtable always exist; make them nil-safe by introducing local fallback tables before accessing nested fields. For example, in the roll animation logic use local rollFrame = db.rollFrame or {} and local animation = rollFrame.animation or db.animation or {} then derive local scale = rollFrame.scale or 1.0 and local animEnabled = animation.enabled or false and use animEnabled for the frame:SetAlpha/animation branches. Apply the same pattern to other places referenced (the blocks around the current roll logic and the other animation blocks) so all nested accesses (rollFrame, rollFrame.animation, db.animation) tolerate partially missing config.DragonLoot/Display/LootAnimations.lua (1)
64-68: Harden nested config reads for partial-profile safety.Line [64] / Line [68] and Line [114] / Line [123] / Line [134] still assume
lootWindowandanimationsubtables exist onceprofileexists. If those subtables are missing (e.g., partial SavedVariables), these paths can still nil-index.♻️ Suggested hardening
function ns.LootAnimations.PlayOpen(frame) local db = ns.Addon.db and ns.Addon.db.profile if not db then frame:Show() return end - local scale = db.lootWindow.scale or 1.0 + local lootWindow = db.lootWindow or {} + local animation = db.animation or {} + local scale = lootWindow.scale or 1.0 @@ - if not db.animation.enabled then + if not animation.enabled then @@ - local duration = db.animation.openDuration or 0.3 + local duration = animation.openDuration or 0.3 @@ - local animName = db.animation.lootOpenAnim or "fadeIn" + local animName = animation.lootOpenAnim or "fadeIn"function ns.LootAnimations.PlayClose(frame, onFinished) @@ - if not db.animation.enabled then + local animation = db.animation or {} + if not animation.enabled then @@ - local duration = db.animation.closeDuration or 0.5 + local duration = animation.closeDuration or 0.5 @@ - local animName = db.animation.lootCloseAnim or "fadeOut" + local animName = animation.lootCloseAnim or "fadeOut"As per coding guidelines
**/*.lua: Lua addon code. Review for nil safety, global leaks, and logic errors.Also applies to: 80-87, 114-135
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@DragonLoot/Display/LootAnimations.lua` around lines 64 - 68, Multiple places assume sub-tables exist on the profile (db.lootWindow, db.animation), causing nil-index errors for partial SavedVariables; update reads to guard these accesses and provide defaults (e.g., replace direct uses like `local scale = db.lootWindow.scale or 1.0` with a guarded form `local scale = (db.lootWindow and db.lootWindow.scale) or 1.0` and change conditional checks like `if not db.animation.enabled then` to `if not (db.animation and db.animation.enabled) then`). Apply the same pattern for all occurrences referenced (around ns.LootAnimations, any reads of db.lootWindow, db.animation, and their fields such as scale, enabled) so every nested config read is nil-safe and uses sensible defaults.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@DragonLoot/Core/Config.lua`:
- Around line 159-168: The loop after ApplyScalarDefaults iterates
defaults.profile but only checks `if not profile[section] then` which fails when
profile[section] exists but is not a table; before indexing
profile[section][key] ensure profile[section] is a table by checking
type(profile[section]) ~= "table" and if so replace it with an empty table
(profile[section] = {}), then proceed to iterate keys; update the section/type
guard around the loop that processes sectionDefaults in the code that references
defaults.profile so non-table/corrupted SavedVariables are reset safely.
---
Nitpick comments:
In `@DragonLoot/Display/LootAnimations.lua`:
- Around line 64-68: Multiple places assume sub-tables exist on the profile
(db.lootWindow, db.animation), causing nil-index errors for partial
SavedVariables; update reads to guard these accesses and provide defaults (e.g.,
replace direct uses like `local scale = db.lootWindow.scale or 1.0` with a
guarded form `local scale = (db.lootWindow and db.lootWindow.scale) or 1.0` and
change conditional checks like `if not db.animation.enabled then` to `if not
(db.animation and db.animation.enabled) then`). Apply the same pattern for all
occurrences referenced (around ns.LootAnimations, any reads of db.lootWindow,
db.animation, and their fields such as scale, enabled) so every nested config
read is nil-safe and uses sensible defaults.
In `@DragonLoot/Display/RollAnimations.lua`:
- Around line 40-45: Reads assume db.rollFrame and its animation subtable always
exist; make them nil-safe by introducing local fallback tables before accessing
nested fields. For example, in the roll animation logic use local rollFrame =
db.rollFrame or {} and local animation = rollFrame.animation or db.animation or
{} then derive local scale = rollFrame.scale or 1.0 and local animEnabled =
animation.enabled or false and use animEnabled for the frame:SetAlpha/animation
branches. Apply the same pattern to other places referenced (the blocks around
the current roll logic and the other animation blocks) so all nested accesses
(rollFrame, rollFrame.animation, db.animation) tolerate partially missing
config.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 3ddae38c-15d9-4573-b657-793b6b6348b6
📒 Files selected for processing (9)
DragonLoot/Core/Config.luaDragonLoot/Display/DisplayUtils.luaDragonLoot/Display/HistoryFrame.luaDragonLoot/Display/LootAnimations.luaDragonLoot/Display/RollAnimations.luaDragonLoot/Display/RollFrame.luaDragonLoot/Display/RollManager.luaDragonLoot/Listeners/LootListener_Retail.luajustfile
- Config.lua: strengthen section type guard in FillMissingDefaults
(if not -> type check) to handle corrupted non-table SavedVariables
- LootAnimations.lua: harden nested db subtable reads with or {} fallbacks
- RollAnimations.lua: same nested subtable hardening
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@DragonLoot/Display/RollAnimations.lua`:
- Around line 35-39: The early-return when db is missing leaves animation state
dirty; before returning from the db-less path replace the single frame:Show()
with normalized state: set isClosing = false, call StopAll(frame) (the cleanup
function used elsewhere), show the frame, and reset visual baselines via
frame:SetAlpha(1) and frame:SetScale(1) so a resumed/reenabled UI isn't
half-transparent or scaled incorrectly; reference the same isClosing,
StopAll(frame), frame:Show(), frame:SetAlpha and frame:SetScale symbols used in
the RollAnimations/RollFrame animation cleanup.
🪄 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: cb111d2f-5f62-45be-8f16-cccdbd073560
📒 Files selected for processing (3)
DragonLoot/Core/Config.luaDragonLoot/Display/LootAnimations.luaDragonLoot/Display/RollAnimations.lua
🚧 Files skipped from review as they are similar to previous changes (2)
- DragonLoot/Core/Config.lua
- DragonLoot/Display/LootAnimations.lua
Stop stale animations, reset alpha/scale, and clear isClosing flag in the fallback paths of PlayOpen/PlayClose and PlayShow/PlayHide when db.profile is unavailable.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
DragonLoot/Display/LootAnimations.lua (1)
74-79:⚠️ Potential issue | 🟠 MajorCancel in-flight animations before non-animated open path.
When
animation.enabledis false, this branch shows the frame without stopping a potentially running close animation. That closeonFinishedcan still fire and re-hide the frame.Suggested fix
if not animation.enabled then + ns.LootAnimations.StopAll(frame) frame:SetAlpha(1) frame:SetScale(scale) frame:Show() return end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@DragonLoot/Display/LootAnimations.lua` around lines 74 - 79, When taking the non-animated open path (when animation.enabled is false), first cancel any in-flight animation and prevent its completion handler from re-hiding the frame by calling the animation group's stop method and clearing its OnFinished script (e.g. call animation:Stop() and animation:SetScript("OnFinished", nil) or the equivalent used elsewhere), then set frame alpha/scale and show the frame; reference the animation table/animation group, its OnFinished handler, and the frame variable so the in-flight close animation cannot later hide the frame.
🧹 Nitpick comments (1)
DragonLoot/Display/LootAnimations.lua (1)
97-99: Consider a shared accessor for loot-window scale/profile lookups.The repeated profile-chain lookups are easy to drift. A small local helper (similar to the RollFrame DB accessor pattern) would keep fallback behavior consistent across success/error callbacks.
Based on learnings,
DragonLoot/Display/RollFrame.luaalready uses a localGetRollFrameDB()helper to centralize guarded DB access.Also applies to: 150-152, 166-167
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@DragonLoot/Display/LootAnimations.lua` around lines 97 - 99, Introduce a small local accessor (e.g., GetLootWindowDB or GetLootWindowScale) to centralize the guarded ns.Addon.db.profile -> lootWindow lookups and default fallback (1.0), then replace the repeated chains (the occurrences feeding frame:SetScale(s) and similar lookups around the file) with calls to that helper; ensure the helper mirrors the pattern used by GetRollFrameDB in DragonLoot/Display/RollFrame.lua, returns either the lootWindow subtable or the numeric scale, and is used at the three spots referenced so fallback behavior stays consistent across success/error callbacks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@DragonLoot/Display/LootAnimations.lua`:
- Around line 74-79: When taking the non-animated open path (when
animation.enabled is false), first cancel any in-flight animation and prevent
its completion handler from re-hiding the frame by calling the animation group's
stop method and clearing its OnFinished script (e.g. call animation:Stop() and
animation:SetScript("OnFinished", nil) or the equivalent used elsewhere), then
set frame alpha/scale and show the frame; reference the animation
table/animation group, its OnFinished handler, and the frame variable so the
in-flight close animation cannot later hide the frame.
---
Nitpick comments:
In `@DragonLoot/Display/LootAnimations.lua`:
- Around line 97-99: Introduce a small local accessor (e.g., GetLootWindowDB or
GetLootWindowScale) to centralize the guarded ns.Addon.db.profile -> lootWindow
lookups and default fallback (1.0), then replace the repeated chains (the
occurrences feeding frame:SetScale(s) and similar lookups around the file) with
calls to that helper; ensure the helper mirrors the pattern used by
GetRollFrameDB in DragonLoot/Display/RollFrame.lua, returns either the
lootWindow subtable or the numeric scale, and is used at the three spots
referenced so fallback behavior stays consistent across success/error callbacks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: a3a74949-f53f-4634-acc4-88a80930a753
📒 Files selected for processing (2)
DragonLoot/Display/LootAnimations.luaDragonLoot/Display/RollAnimations.lua
🚧 Files skipped from review as they are similar to previous changes (1)
- DragonLoot/Display/RollAnimations.lua
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
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Addresses the code quality issues identified by CodeRabbit in PR #128, tracked in #132.
Changes
Nil-safety guards (7 files)
ApplyBackdropearly return if db/profile missingPlayOpen/PlayCloseguarded db access with fallback show/hide;RestoreDbAnchorchain guard;onFinishedcallbacks completed with intermediate locals; non-animated pathframe:Hide()fixPlayShow/PlayHideguarded db access with fallback show/hide;onFinishedcallbacks completed; non-animated pathframe:Hide()fixOnAccept4-level chain guardOnRollButtonClickuses existingGetRollFrameDB()accessorOnLootReadyguarded likeOnLootOpenedGetHistoryDB()accessor, used inGetEntrySpacing/GetContentPaddingConfig fixes
FillMissingDefaultsnow handles top-level scalar defaults (enabled,debug) via sharedApplyScalarDefaultshelper;ResetToDefaultsalso fixed to reset scalars on profile resetTooling
checkrecipe now runsfmt-check lint test(waslint test)False positives (not addressed)
LootListener_RetailL63,LootListener_ClassicL46,ListenerSharedL89), not fromLootFrame.lualocal _, ns = ...- 29/31 files use_(discard); onlyInit.luaandSlashCommands.luauseADDON_NAME. This is the project conventionVerification
just lint- 0 warningsjust fmt-check- cleanjust test- 29/29 passingCloses #132
Summary by CodeRabbit
Bug Fixes
Chores