Fix: Add bounds check for slotTimers to prevent silent trade recording failure#75
Open
Reggie-Reuss wants to merge 1 commit intoFlipping-Utilities:masterfrom
Open
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
📝 WalkthroughWalkthroughThree methods now include defensive validation for slot timers. Methods check that slot timers exist and have exactly 8 elements before proceeding with operations dependent on that state, adding early-return guards and reinitializing timers when needed. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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)
📝 Coding Plan
Comment Tip You can disable the changed files summary in the walkthrough.Disable the |
Contributor
|
@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
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
2 participants
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
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