feat(history): add encounter and search filter to loot history (#89)#172
Open
Xerrion wants to merge 12 commits into
Open
feat(history): add encounter and search filter to loot history (#89)#172Xerrion wants to merge 12 commits into
Xerrion wants to merge 12 commits into
Conversation
|
Warning Review limit reached
More reviews will be available in 50 minutes and 27 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis PR adds encounter-based filtering to the loot history frame. It introduces a dropdown and search box to filter items by boss/encounter, Classic event capture for encounter context, schema migration to v5, comprehensive test coverage, and a persistent filter bar visibility toggle. ChangesEncounter filter feature and Classic support
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
🚥 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 |
Xerrion
added
A-History
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 `@DragonLoot/Display/HistoryFrame.lua`:
- Around line 192-193: RestoreFilter currently sets filterState.search =
persisted.search or "" but doesn't ensure persisted.search is a string; if
persisted.search is a non-string, MatchesFilter (which calls
string_lower(search)) can error. Update RestoreFilter to coerce persisted.search
to a string before assignment (e.g., check type(persisted.search) == "string"
and fall back to ""), so filterState.search is always a string; reference
symbols: RestoreFilter, filterState.search, persisted.search, MatchesFilter,
string_lower.
- Line 1171: CreateFrame calls in the Display layer are leaking globals because
they pass explicit names; replace the named frames (e.g., the
DragonLootHistoryEncounterDropdown and DragonLootHistorySearchBox created at the
CreateFrame calls, and other named frames DragonLootHistoryScroll,
DragonLootHistoryScrollBar, DragonLootHistoryFrame) by passing nil as the name
so no global is created. Also harden RestoreFilter(): when assigning
persisted.search into filterState.search inside RestoreFilter(), coerce
non-string values to an empty string (e.g., if type(persisted.search) ~=
"string" then set filterState.search = "") so MatchesFilter() won’t call
string_lower on non-strings. Ensure these changes are applied to the
functions/locations that use CreateFrame for the named widgets and the
RestoreFilter function.
In `@DragonLoot/Listeners/EncounterListener_Classic.lua`:
- Around line 39-57: The ENCOUNTER_START event is registered on the local frame
but never unregistered, so add an explicit unregister lifecycle: expose a
cleanup/stop method (e.g., ns.EncounterListener_Classic.Stop or .Destroy) that
calls frame:UnregisterEvent("ENCOUNTER_START") and clears the OnEvent script
(frame:SetScript("OnEvent", nil)), and ensure any caller of the listener (tests
or init/shutdown code) invokes this when the listener is no longer needed;
update the ns.EncounterListener_Classic table to include the stop/destroy method
and use the existing local frame and event name (frame, ENCOUNTER_START,
ns.EncounterListener_Classic) to perform the unregister.
In `@DragonLoot/Listeners/HistoryListener_Classic.lua`:
- Around line 104-115: The rebuild in RefreshFromAPI in
HistoryListener_Classic.lua is overwriting earlier drops with the latest
ns.currentEncounter; update the logic in the block that sets
entry.encounterID/encounterName so it only assigns encounter metadata when the
entry does not already have it (e.g., if entry.encounterID == nil or
entry.rollID/uniqueDropKey is not present), or persist encounter info keyed by
the stable drop identifier (roll ID or drop key) and read from that map instead
of always applying ns.currentEncounter; modify the code that reads
ns.currentEncounter and writes entry.encounterID/entry.encounterName to check
existing values first (or use a per-drop cache) so RefreshFromAPI no longer
reattributes earlier drops to the latest boss.
In `@spec/wow_mock.lua`:
- Around line 74-78: M.SetInstanceInfo currently calls pairs(info) unguarded and
will error if info is nil; update M.SetInstanceInfo to first validate the input
(e.g., if not info or type(info) ~= "table" then return end) before iterating,
then merge entries into M._instanceInfo as before (referencing the function
M.SetInstanceInfo and the table M._instanceInfo).
- Around line 36-38: The mock GetItemInfo currently "invent"s data by returning
a default quality when link is nil; change GetItemInfo to return nil (no values)
when link is nil so callers see uncached/missing item behavior (reference
function GetItemInfo in spec/wow_mock.lua). Also add a nil-guard to
M.SetInstanceInfo (the function that calls pairs(info)) so it returns early or
handles a nil info argument (e.g., if not info then return end) to avoid calling
pairs on nil.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 17b740e6-1a63-432b-b412-d194ba05081f
📒 Files selected for processing (13)
.luacheckrcAGENTS.mdDragonLoot/Core/Config.luaDragonLoot/Display/HistoryFrame.luaDragonLoot/DragonLoot.tocDragonLoot/Listeners/EncounterListener_Classic.luaDragonLoot/Listeners/HistoryListener_Classic.luaDragonLoot/Locales/enUS.luaDragonLoot_Options/Tabs/HistoryTab.luaspec/Config_spec.luaspec/EncounterListener_Classic_spec.luaspec/HistoryFilter_spec.luaspec/wow_mock.lua
- RestoreFilter: type-guard persisted encounterID (number) and search (string) before assignment so a corrupted SavedVariable cannot poison string.lower at filter time. - HistoryFrame: drop named globals on the two #89-introduced frames (encounter dropdown, search box); pre-existing scroll/scrollbar/container names retained. - EncounterListener_Classic: refactor to Initialize/Shutdown lifecycle matching sister listeners; wire into Init.lua MODULES (Retail-safe via nil-guard in loop and file-level WOW_PROJECT_MAINLINE early return). - spec/wow_mock: GetItemInfo(nil) returns nil to mirror real API; SetInstanceInfo guards non-table input.
…PI (#89) UIDropDownMenu_RefreshDropDownSize requires self:GetName() for child icon lookup (envTable[self:GetName().."Button"..i.."Icon"]) with no member fallback. It runs on the next OnUpdate tick after UIDropDownMenu_Initialize sets shouldRefresh = true, so an anonymous dropdown throws 'attempt to concatenate a nil value' on every flavor (Retail, MoP Classic, TBC Anniversary). Search box (SearchBoxTemplate) is unaffected and stays anonymous.
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.
Closes #89
Summary
Adds an encounter dropdown + free-text search filter to the loot history window. Filter state persists across reloads. A new options toggle hides the filter bar for users who don't want it.
Changes
Data model
db.profile.history.filter = { encounterID = nil, search = "", barVisible = true }.Classic encounter tagging
Listeners/EncounterListener_Classic.luacapturesENCOUNTER_STARTpayload intons.currentEncounter.encounterIDandencounterNameat tag time, matching Retail behavior.select(8, GetInstanceInfo())change) per ADR-0004.Filter UI
UIDropDownMenuTemplateencounter dropdown +SearchBoxTemplatesearch box + visible/total counter.EJ_GetEncounterInfo→ numeric fallback).Persistence
Options
ns.HistoryFrame.ApplySettings()— scroll frame re-anchors to reclaim/yield the filter-bar height without reload.Tests
spec/HistoryFilter_spec.lua): predicate purity, sentinel handling, persistence round-trip.spec/EncounterListener_Classic_spec.lua).Verification
mise exec -- just check— stylua + luacheck + busted all green.Out of scope
AGENTS.mdupdates (separate concern).searchBox:SetTextre-fires the OnTextChanged hook — non-blocking, follow-up if needed.Summary by CodeRabbit
New Features
Documentation
Chores