Repository navigation
fix(inventory): return false from combine when an item is missing - #2252
Conversation
Rs2Inventory.combine(int, int) and combine(String, String) pass the result of get(...) to combine(Rs2ItemModel, Rs2ItemModel), which dereferenced both arguments. A missing item threw a NullPointerException on the script thread, and a missing secondary item left the primary item selected. Return false before any click when either item is null. Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. Walkthrough
Merge Risk:
|
Problem
Rs2Inventory.combinethrows aNullPointerExceptionwhen one of the two items is not in the inventory.java.lang.NullPointerException(Cannot invoke "...Rs2ItemMod...", message cut off by telemetry)Rs2Inventory.lambda$combine$2:173<-Rs2Inventory.get:747<-Rs2Inventory.combine:173<-Rs2Inventory.combine:137, called from a third-party plugin on its script threadIn the reported case the primary item had already been selected with
use(primaryItem)before the lookup of the missing secondary item threw, so besides the uncaught exception the item was left selected with the Use cursor active.Root cause
combine(int, int)andcombine(String, String)pass the result ofget(...)straight intocombine(Rs2ItemModel, Rs2ItemModel).get(...)returns null for an item that is not in the inventory, and the model overload dereferenced bothprimaryandsecondaryinside its filter lambdas without a null check.Change
Rs2Inventory.combine(Rs2ItemModel, Rs2ItemModel)returnsfalseat the top when either argument is null, beforeuse(primaryItem), so nothing is left selected. Both delegating overloads are covered by the same guard. Returningfalsematches the existing Javadoc contract and the existing not-found paths.Rs2InventoryCombineTestwith three cases: by id with the secondary missing, by id with the primary missing, by name with the secondary missing. Each assertsfalseand thatuse(...)is never called.docs/entity-guides/items.md: gotcha 21 describing the null-item case, as the repo AGENTS.md asks for when fixing an entity-assumption bug underutil/.Validation
./gradlew :client:runUnitTests --tests "*Rs2InventoryCombineTest" --tests "*Rs2InventoryInvokeMenuTest": BUILD SUCCESSFUL,Rs2InventoryCombineTest3/3 passed,Rs2InventoryInvokeMenuTest4/4 passed.Code review approved the commit. Siblings checked in review:
combineClosestoverloads already returnfalseon empty lists,use(Rs2ItemModel)already null-guards, and theRs2Fletchingcallers all go through the guarded function.Known gaps
falsewithout a log line, while the two neighbouring not-found paths log.falsewith the Use cursor left active. Follow-up candidate: resolve the secondary item before callinguse(primaryItem).🤖 Generated with Claude Code