feat: show inventory item count on loot toasts (#151)#152
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughAdds an optional, togglable item-count display to toasts, UI/config changes to support it, localization for many user-facing strings, test coverage for the new behavior, and CI/tooling config updates. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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}
Contributor
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)
spec/SlashCommands_spec.lua (1)
21-33:⚠️ Potential issue | 🟡 MinorRestore original
LibStubin teardown to avoid cross-spec leakage.Line 78 always nils the global, which can break neighboring specs that expect a preexisting
LibStub. Preserve and restore the original value.🧪 Suggested fix
describe("HandleSlashCommand", function() local ns local clearCalled local printedMessage local originalPrint + local originalLibStub before_each(function() originalPrint = _G.print + originalLibStub = _G.LibStub _G.print = function() end @@ after_each(function() _G.print = originalPrint - _G.LibStub = nil + _G.LibStub = originalLibStub end)Also applies to: 76-79
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/SlashCommands_spec.lua` around lines 21 - 33, The test setup overwrites the global _G.LibStub and the teardown currently nils it, causing leakage; capture the original value before overwriting (e.g., store it in originalLibStub alongside originalPrint inside before_each where _G.LibStub is assigned) and restore that saved value in after_each (set _G.LibStub = originalLibStub) instead of unconditionally niling the global so neighboring specs keep their expected LibStub.
🧹 Nitpick comments (2)
DragonToast_Options/Tabs/DisplayTab.lua (1)
200-208: Trigger a layout refresh when toggling item count.For immediate in-session feedback, Line 204 can mirror other layout-affecting controls and refresh active toasts after the value change.
♻️ Suggested tweak
local itemCountToggle = W.CreateToggle(parent, { label = L["Show Item Count"], tooltip = L["Display total inventory count of the looted item on the toast"], get = function() return db.profile.display.showItemCount end, - set = function(value) db.profile.display.showItemCount = value end, + set = function(value) + db.profile.display.showItemCount = value + dtns.ToastManager:UpdateLayout() + end, })As per coding guidelines
DragonToast_Options/**/*.lua: “Review for correct widget lifecycle … proper config key paths matching db.profile schema in Core/Config.lua, and callback wiring.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@DragonToast_Options/Tabs/DisplayTab.lua` around lines 200 - 208, The itemCountToggle's set currently only updates db.profile.display.showItemCount without triggering a layout refresh; update the set handler for itemCountToggle (created in this block) to mirror other layout-affecting controls by calling the same "refresh active toasts" routine used elsewhere after assigning db.profile.display.showItemCount, ensuring you follow the widget lifecycle pattern around LC.AnchorWidget(itemCountToggle, ...) so active toasts update immediately when toggled.spec/ToastFrame_spec.lua (1)
246-290: Good coverage of item count display scenarios.Tests verify:
- Shown with correct text when enabled + itemID present
- Hidden when
showItemCount=false- Hidden when
itemID=nil- Hidden for currency toasts
- Graceful
x0 in bagswhenGetItemCountreturns nil (Classic nil-guard)Consider adding tests for
isXP,isHonor,isReputationexclusions if you want exhaustive coverage, but the currency test demonstrates the pattern works.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/ToastFrame_spec.lua` around lines 246 - 290, Add tests covering the cases where toasts should hide the item count for non-item reward types: create new examples in spec/ToastFrame_spec.lua that mirror the existing "hides count for currency toasts" test but set data.isXP = true, data.isHonor = true, and data.isReputation = true respectively (use makeItemData({ isXP = true }), etc.), call ns.ToastFrame.Populate(frame, data), and assert that frame.itemCount._shown is false (and optionally that _text is untouched); reuse the same patterns and mocks (e.g. _mockGetItemCount, makeItemData, ns.ToastFrame.Populate, frame.itemCount) so the tests follow the existing style.
🤖 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 `@spec/SlashCommands_spec.lua`:
- Around line 21-33: The test setup overwrites the global _G.LibStub and the
teardown currently nils it, causing leakage; capture the original value before
overwriting (e.g., store it in originalLibStub alongside originalPrint inside
before_each where _G.LibStub is assigned) and restore that saved value in
after_each (set _G.LibStub = originalLibStub) instead of unconditionally niling
the global so neighboring specs keep their expected LibStub.
---
Nitpick comments:
In `@DragonToast_Options/Tabs/DisplayTab.lua`:
- Around line 200-208: The itemCountToggle's set currently only updates
db.profile.display.showItemCount without triggering a layout refresh; update the
set handler for itemCountToggle (created in this block) to mirror other
layout-affecting controls by calling the same "refresh active toasts" routine
used elsewhere after assigning db.profile.display.showItemCount, ensuring you
follow the widget lifecycle pattern around LC.AnchorWidget(itemCountToggle, ...)
so active toasts update immediately when toggled.
In `@spec/ToastFrame_spec.lua`:
- Around line 246-290: Add tests covering the cases where toasts should hide the
item count for non-item reward types: create new examples in
spec/ToastFrame_spec.lua that mirror the existing "hides count for currency
toasts" test but set data.isXP = true, data.isHonor = true, and
data.isReputation = true respectively (use makeItemData({ isXP = true }), etc.),
call ns.ToastFrame.Populate(frame, data), and assert that frame.itemCount._shown
is false (and optionally that _text is untouched); reuse the same patterns and
mocks (e.g. _mockGetItemCount, makeItemData, ns.ToastFrame.Populate,
frame.itemCount) so the tests follow the existing style.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 1e8aa7f3-141e-475b-af6c-86d491409d10
📒 Files selected for processing (15)
.luacheckrc.mise.tomlDragonToast/Core/Config.luaDragonToast/Core/ConfigWindow.luaDragonToast/Core/MinimapIcon.luaDragonToast/Core/Presets.luaDragonToast/Core/SlashCommands.luaDragonToast/Display/TestToasts.luaDragonToast/Display/ToastFrame.luaDragonToast/Display/ToastManager.luaDragonToast/Locales/deDE.luaDragonToast/Locales/enUS.luaDragonToast_Options/Tabs/DisplayTab.luaspec/SlashCommands_spec.luaspec/ToastFrame_spec.lua
This was referenced Apr 5, 2026
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.
Description
Adds an optional display of the player's current inventory count for looted items on toast notifications. When enabled, toasts for looted items show how many of that item you already have in your bags (e.g.
x3 in bags).Type of Change
Related Issues
Closes #151
Testing
luacheck .)Screenshots
Not applicable - feature is toggled off by default.
Checklist
Implementation Notes
C_Item.GetItemCounton Retail/MoP, falls back to legacyGetItemCounton TBC Anniversaryor 0guard prevents Lua errors when the API returns nil (e.g. uncached items on Classic)itemCountFontString is anchored belowitemLevelto prevent overlap when both are enabledSIMPLE_MIGRATIONSentry for clean upgradesspec/ToastFrame_spec.luacovering show/hide/nil/currency/Classic-nil-guard pathsSummary by CodeRabbit
New Features
Localization
Tests
Chores