Fix: Add bounds check for slotTimers to prevent silent trade recording failure#75
Conversation
If slotTimers is deserialized as an empty list rather than null (e.g. from a recovered or manually constructed JSON file), hydrateSlotTimers() skips initialization because it only checks for null. The three unguarded .get(slot) calls in screenOfferEvent() then throw IndexOutOfBoundsException on every GE offer event, silently dropping all real-time trades with no user-visible error. - AccountData.hydrateSlotTimers: reinitialize if size != 8, not just null - NewOfferEventPipelineHandler.screenOfferEvent: early return with log warning if slotActivityTimers is missing or undersized - FlippingPlugin.setWidgetsOnSlotTimers: skip loop if list undersized
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdded defensive checks and stricter initialization for slot timers: methods now verify the presence and exact count (8) of slot timers before operating, logging and early-returning or reinitializing when the condition is not met. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/main/java/com/flippingutilities/controller/FlippingPlugin.java (1)
1004-1015: Consider adding a similar guard for consistency.The
startSlotTimersmethod accessesgetSlotTimers()without validating size. WhileforEachwon't throw on an undersized list, this could lead to silent partial updates if timers are missing. For consistency with the other guards, consider adding a null/size check, or at minimum a log warning.♻️ Optional: Add defensive check
private ScheduledFuture startSlotTimers() { - return executor.scheduleAtFixedRate(() -> - dataHandler.viewAccountData(currentlyLoggedInAccount).getSlotTimers().forEach(slotWidgetTimer -> + return executor.scheduleAtFixedRate(() -> { + List<SlotActivityTimer> slotTimers = dataHandler.viewAccountData(currentlyLoggedInAccount).getSlotTimers(); + if (slotTimers == null || slotTimers.size() < 8) { + log.warn("slotTimers missing or undersized in startSlotTimers, skipping update"); + return; + } + slotTimers.forEach(slotWidgetTimer -> clientThread.invokeLater(() -> { try { slotsPanel.updateTimerDisplays(slotWidgetTimer.getSlotIndex(), slotWidgetTimer.createFormattedTimeString()); slotWidgetTimer.updateTimerDisplay(); } catch (Exception e) { log.error("exception when trying to update timer", e); } - })), 1000, 1000, TimeUnit.MILLISECONDS); + })); + }, 1000, 1000, TimeUnit.MILLISECONDS); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/flippingutilities/controller/FlippingPlugin.java` around lines 1004 - 1015, The startSlotTimers lambda calls dataHandler.viewAccountData(currentlyLoggedInAccount).getSlotTimers() and then blindly iterates/update slotsPanel.updateTimerDisplays and slotWidgetTimer.updateTimerDisplay; add a defensive check to validate the returned slot timers collection (null check and a size/emptiness check or expected-size check) before the forEach, and log a warning (including currentlyLoggedInAccount) and skip the update if validation fails so you avoid silent partial updates and keep behavior consistent with other guards.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/main/java/com/flippingutilities/controller/FlippingPlugin.java`:
- Around line 1004-1015: The startSlotTimers lambda calls
dataHandler.viewAccountData(currentlyLoggedInAccount).getSlotTimers() and then
blindly iterates/update slotsPanel.updateTimerDisplays and
slotWidgetTimer.updateTimerDisplay; add a defensive check to validate the
returned slot timers collection (null check and a size/emptiness check or
expected-size check) before the forEach, and log a warning (including
currentlyLoggedInAccount) and skip the update if validation fails so you avoid
silent partial updates and keep behavior consistent with other guards.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c2fe29d7-611d-4bfc-9dd9-5ca5ea70217c
📒 Files selected for processing (3)
src/main/java/com/flippingutilities/controller/FlippingPlugin.javasrc/main/java/com/flippingutilities/controller/NewOfferEventPipelineHandler.javasrc/main/java/com/flippingutilities/model/AccountData.java
forEach on an empty list will not throw, but if slotTimers is undersized the timer updates will silently do nothing. Guard added for consistency with the other three call sites and to surface the condition via log warning rather than silent no-op.
|
Good catch from CodeRabbit on Also acknowledged the docstring coverage warning — that appears to be a pre-existing repo-wide condition rather than something introduced by this PR. |
2 participants
Summary
hydrateSlotTimers()only checks fornull, but Gson deserializes an empty JSON array[]as a non-null emptyArrayList— bypassing initialization entirely.get(slot)calls inscreenOfferEvent()then throwIndexOutOfBoundsExceptionon every GE offer event, silently dropping all real-time tradesscreenOfferEvent(), which masks the failure — the plugin appears functional but records nothingsetWidgetsOnSlotTimers()for the same patternHow I found this
I discovered this while investigating why only certain items were saving in my trade history. After my data recovery from a RuneLite crash that corrupted both my main JSON and backup JSON simultaneously, the recovered file had
"slotTimers": []. This caused 16 days of silent trade recording failure before I traced it toIndexOutOfBoundsExceptionerrors inclient.log.Full root cause analysis: Phase 6: slotTimers Bug
Is this likely in normal use?
In a typical crash, probably not — the plugin falls back to the backup file which usually has valid 8-entry slotTimers. But it can happen in rare edge cases:
slotTimersboundary (theoretically possible for medium-sized account files in the 8-16KB range)"slotTimers": []The fix is a one-line change to the root cause (
nullcheck →null || size() != 8) plus two defensive guards at the.get()call sites.Changes
AccountData.hydrateSlotTimerssize() != 8, not justnullNewOfferEventPipelineHandler.screenOfferEventlog.warnif slotTimers undersizedFlippingPlugin.setWidgetsOnSlotTimersNote
Java is not my primary language, so this would benefit from review by someone more experienced with the codebase — similar to my other PR (#74). Happy to adjust based on feedback.
Summary by CodeRabbit