feat: persist loot history across sessions (#104)#152
Conversation
- Add db.char.history.entries with schemaVersion bump v3 -> v4 - Stamp wallTime alongside session timestamp in all history writers - Restore persisted entries on HistoryFrame.Initialize; merge with Classic API results and seed Retail processedDrops to prevent cross-session duplicates - Preserve earliest wallTime in UpdateEntryByKey carry-forward to fix cross-session timestamp clobber on API replay - FormatTimeAgo prefers wall clock when session/wall disagree by >5m - Add Clear History button with confirmation popup in options - Add enUS locale keys (button reuses existing "Clear History")
Xerrion
added
C-Feature
|
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 48 minutes and 56 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 (2)
📝 WalkthroughWalkthroughThis PR adds persistent cross-session loot history storage via AceDB ( Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
DragonLoot/Listeners/HistoryListener_Classic.lua (1)
50-138:⚠️ Potential issue | 🟠 MajorCross-day-boundary duplicates in the Classic merge.
Every API-derived entry in
RefreshFromAPIis stamped withnowWall = time(). Persisted entries restored from SavedVariables retain their originalwallTime.BuildDedupKeyinHistoryFrame.luaincludesmath_floor(wallTime / 86400)as part of the key:return link .. "|" .. winner .. "|" .. bucket -- (or dropKey .. "|" .. bucket)If a
/reloadhappens after midnight whileC_LootHistorystill has the same items, the API entries get today's bucket and the persisted entries keep yesterday's bucket → keys differ → both are kept, and the user sees the same drop twice. Same hazard exists for entries that do carry adropKey(Retail-shaped entries that ever land in the Classic listener), since the bucket is part of the key for those too.Retail dodges this because
processedDrops[dropKey]is keyed without the bucket and is seeded from persisted entries beforePopulateExistingHistoryruns.Suggested mitigation: when an API entry's item+winner (or
dropKey) matches a persisted entry, reuse the persistedwallTimeinstead of overwriting withnowWall. Something like:🔧 Proposed fix sketch
+ -- Index persisted entries by a wallTime-independent key for back-fill. + local persistedByKey = {} + if ns.historyData then + for _, p in ipairs(ns.historyData) do + local k = (p.dropKey) + or ((p.itemLink or "?") .. "|" .. (p.winner or "?")) + persistedByKey[k] = p + end + end + local now = GetTime() local nowWall = time() local entries = {} for i = 1, numItems do ... + local k = (p.dropKey) or ((itemLink or "?") .. "|" .. (winner or "?")) + local prior = persistedByKey[k] entries[`#entries` + 1] = { ... timestamp = now, - wallTime = nowWall, + wallTime = (prior and prior.wallTime) or nowWall, ... } endThe exact key shape can match
BuildDedupKey's bucket-less form to keep things consistent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@DragonLoot/Listeners/HistoryListener_Classic.lua` around lines 50 - 138, The API-derived entries are stamped with nowWall = time(), causing duplicate cross-day keys versus persisted entries because BuildDedupKey includes a day bucket; fix RefreshFromAPI so that when building each API entry you compute its dedup key via ns.HistoryFrame_BuildDedupKey (buildKey) and if a matching persisted entry exists in ns.historyData reuse that persisted entry's wallTime (and optionally timestamp) instead of overwriting with nowWall before appending to entries; locate the API loop that builds entries (uses nowWall, entries, C_LootHistory.GetItem, C_LootHistory.GetPlayerInfo) and, for each constructed entry, look up the persisted entry by key and copy persisted.wallTime into the API entry when found so the dedup logic later treats them as the same drop.
🧹 Nitpick comments (2)
DragonLoot_Options/Tabs/HistoryTab.lua (1)
244-254: Optional: visually flag this as destructive.The button shares the standard appearance of every other widget in the section. A destructive-action style (e.g., red text or
RED_FONT_COLOR) on the button label, or at minimum an exclamation prefix on the tooltip, would help users avoid wiping history by reflex. Confirmation popup is a strong second line of defense, so this is a nice-to-have.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@DragonLoot_Options/Tabs/HistoryTab.lua` around lines 244 - 254, Change the Clear History button to visually indicate a destructive action by wrapping the label in red font color codes and/or changing the tooltip to include an explicit warning; e.g., when creating clearBtn via W.CreateButton, set text = RED_FONT_COLOR_CODE .. L["Clear History"] .. FONT_COLOR_CODE_CLOSE (or use an appropriate red font object) and update tooltip to prepend an exclamation or "Danger:" (e.g., tooltip = "⚠️ " .. L["Permanently delete all stored loot history entries"]). Keep the existing onClick that calls StaticPopup_Show("DRAGONLOOT_CLEAR_HISTORY") so the confirmation popup remains as the second line of defense.DragonLoot/Display/HistoryFrame.lua (1)
107-121:PersistEntriesaliases entry tables with the saved store.
store[i] = entryshares each entry table by reference betweenns.historyDataanddb.char.history.entries. That's correct given current mutation patterns — every mutating path (AddEntry,UpdateEntryByKey,SetEntries,ClearHistory) wipes and re-copies — but it's a sharp edge. If anything later mutates an entry in place without callingPersistEntries, the saved-vars copy will silently drift in some cases and not in others (depending on whetherUpdateEntryByKeyswapped the slot). Worth a one-line comment, or a deep-copy inPersistEntries, depending on how paranoid you want to be about future contributors.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@DragonLoot/Display/HistoryFrame.lua` around lines 107 - 121, PersistEntries currently writes references by doing store[i] = entry, which aliases ns.historyData entries into db.char.history.entries; change this to store[i] = CopyTable(entry) (or another deep-copy utility) so each saved entry is an independent table, and keep references to ns.historyData, store, db.char.history.entries and the PersistEntries function to locate the change; alternatively, if you prefer not to deep-copy, add a one-line comment above the loop in PersistEntries noting the intentional aliasing and the assumption that all mutating paths re-copy entries.
🤖 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/HistoryFrame.lua`:
- Around line 92-103: BuildDedupKey is intentionally bucketing by wallTime/day
and should not be changed; instead fix the Classic listener replay so it
preserves the original persisted timestamp. In the Classic replay path in
HistoryListener_Classic.lua (the code that re-emits or reconstructs persisted
entries for the API replay), stop overwriting entry.wallTime with time() and
copy the persisted wallTime into the replayed entry (or carry the persisted
timestamp field forward) so the dedup key remains identical for the same
physical drop observed twice across midnight.
In `@DragonLoot/Listeners/HistoryListener_Classic.lua`:
- Line 132: The nil-check chain stops before .profile so it can still nil-index;
update the expression that sets maxEntries (currently referencing
ns.Addon.db.profile.history.maxEntries) to either rely on AceDB by using
ns.Addon.db.profile.history.maxEntries or extend the defensive checks to include
.profile and .profile.history, e.g. (ns.Addon and ns.Addon.db and
ns.Addon.db.profile and ns.Addon.db.profile.history and
ns.Addon.db.profile.history.maxEntries) or 100 so you never nil-index
.profile/.history.
---
Outside diff comments:
In `@DragonLoot/Listeners/HistoryListener_Classic.lua`:
- Around line 50-138: The API-derived entries are stamped with nowWall = time(),
causing duplicate cross-day keys versus persisted entries because BuildDedupKey
includes a day bucket; fix RefreshFromAPI so that when building each API entry
you compute its dedup key via ns.HistoryFrame_BuildDedupKey (buildKey) and if a
matching persisted entry exists in ns.historyData reuse that persisted entry's
wallTime (and optionally timestamp) instead of overwriting with nowWall before
appending to entries; locate the API loop that builds entries (uses nowWall,
entries, C_LootHistory.GetItem, C_LootHistory.GetPlayerInfo) and, for each
constructed entry, look up the persisted entry by key and copy
persisted.wallTime into the API entry when found so the dedup logic later treats
them as the same drop.
---
Nitpick comments:
In `@DragonLoot_Options/Tabs/HistoryTab.lua`:
- Around line 244-254: Change the Clear History button to visually indicate a
destructive action by wrapping the label in red font color codes and/or changing
the tooltip to include an explicit warning; e.g., when creating clearBtn via
W.CreateButton, set text = RED_FONT_COLOR_CODE .. L["Clear History"] ..
FONT_COLOR_CODE_CLOSE (or use an appropriate red font object) and update tooltip
to prepend an exclamation or "Danger:" (e.g., tooltip = "⚠️ " .. L["Permanently
delete all stored loot history entries"]). Keep the existing onClick that calls
StaticPopup_Show("DRAGONLOOT_CLEAR_HISTORY") so the confirmation popup remains
as the second line of defense.
In `@DragonLoot/Display/HistoryFrame.lua`:
- Around line 107-121: PersistEntries currently writes references by doing
store[i] = entry, which aliases ns.historyData entries into
db.char.history.entries; change this to store[i] = CopyTable(entry) (or another
deep-copy utility) so each saved entry is an independent table, and keep
references to ns.historyData, store, db.char.history.entries and the
PersistEntries function to locate the change; alternatively, if you prefer not
to deep-copy, add a one-line comment above the loop in PersistEntries noting the
intentional aliasing and the assumption that all mutating paths re-copy entries.
🪄 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: e671ff33-1844-493b-b94a-94683ccf0439
📒 Files selected for processing (8)
.luacheckrcDragonLoot/Core/Config.luaDragonLoot/Display/HistoryFrame.luaDragonLoot/Listeners/HistoryListener_Classic.luaDragonLoot/Listeners/HistoryListener_Retail.luaDragonLoot/Listeners/LootHistoryChat.luaDragonLoot/Locales/enUS.luaDragonLoot_Options/Tabs/HistoryTab.lua
Updates three assertions in spec/Config_spec.lua to track the CURRENT_SCHEMA bump from 3 to 4 introduced by the persistent loot history feature. Production behavior is unchanged.
Addresses CodeRabbit findings on PR #152: - Cross-midnight duplicate: when a persisted drop is re-observed by C_LootHistory on a later UTC day, reuse the persisted wallTime so the day-bucketed dedup keys collapse. - Tighten nil-chain on maxEntries lookup to guard .profile and .history consistently with the rest of the defensive chain.
|
@coderabbitai Two review-body items addressed inline: Clear History button styling - declining. DragonWidgets' CreateButton factory has no destructive variant exposed in its public API, and the StaticPopup confirmation already provides the safety gate (YES/NO with preferredIndex=3, matching the existing DRAGONLOOT_RESET_PROFILE precedent in ProfilesTab.lua). Adding a manual :SetTextColor would be inconsistent with the rest of the options UI which uses uniform button styling. Happy to reconsider if a future DragonWidgets release adds a destructive = true opt. PersistEntries table aliasing - declining. The aliasing is intentional: AceDB requires the same table identity for its defaults handling (we wipe + copy refs rather than reassign for that reason), and the bidirectional aliasing means subsequent UpdateEntryByKey mutations on a live entry stay in sync with the saved store without needing a second write. Deep-copying on persist would double the memory footprint and force every mutation site to re-trigger a save. The existing entry shape (primitives + simple |
|
Tip For best results, initiate chat on the files or code changes.
Recording both for future reviews: ✏️ Learnings added
🧠 Learnings used |
1 participant
Description
Loot history now persists to
db.char.history.entriesso it survives/reload, relog, andLOOT_HISTORY_CLEAR_HISTORYevents. Entries stamp both an in-sessiontimestampand an absolutewallTime;FormatTimeAgoprefers wall-clock when session and wall disagree by more than 5 minutes (i.e. cross-session entries).Type of Change
Related Issues
Closes #104.
Testing
luacheck .)Screenshots
Checklist
Summary by CodeRabbit
New Features
Chores