refactor: introduce loader-agnostic IItemStorage contract#349
Merged
Indemnity83 merged 8 commits intoMay 8, 2026
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds IItemKey/IItemStorage/IItemView and ItemStorageLookup plus ContainerItemStorage and Fabric adapter implementations. Migrates HasItemStorage, ItemInventoryComponent, network APIs/records, NetworkController/PipeNetwork/planning, pipes/modules, PipeRuntime transfer, block-entity itemStorage exposure (Kiln, Macerator, StirlingEngine, PipeItemStorage), BlockEntityUtil drops, Fabric capability wiring, and tests to the new abstraction. Tests register TestItemKey and Fabric registers FabricItemKey/FabricItemStorage adapters. Sequence Diagram(s)sequenceDiagram
participant Module
participant ItemStorageLookup
participant IItemStorage
participant NetworkController
participant WorldView
Module->>ItemStorageLookup: find(level,pos,side)
ItemStorageLookup-->>Module: IItemStorage
Module->>IItemStorage: extract/insert(key, amt, simulate=false)
Module-->>NetworkController: placeOrder/notifyDelivery(key, amt)
NetworkController->>WorldView: dispatch(provider, requester, key, amt, id)
WorldView-->>NetworkController: dispatched amt
Possibly related issues
Possibly related PRs
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
common/src/main/java/com/logistics/pipe/modules/SupplierModule.java (1)
296-313:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove dead code and simplify available space calculation.
The
if (currentAmount == 0)branch is unreachable—storage.contents()contractually returns only non-empty views per the interface documentation and all implementations filter out empty slots. Additionally, the code only explorestargetStack.getMaxStackSize()capacity by passing that as the limit toinsert(), which is insufficient if the container can accommodate multiple stacks. The unnecessary iteration throughcontents()followed by a singleinsert()call with a capped amount makes this overly complex compared to a directstorage.insert(targetKey, largeAmount, true)approach (as used inInsertionModule).🤖 Prompt for 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. In `@common/src/main/java/com/logistics/pipe/modules/SupplierModule.java` around lines 296 - 313, The loop in SupplierModule calculating availableSpace is overly complex and uses unreachable branches (storage.contents() only returns non-empty IItemView), and it underestimates capacity by capping inserts to targetStack.getMaxStackSize(); replace the entire loop with a single simulated insert: call storage.insert(targetKey, Long.MAX_VALUE, true) (or another sufficiently large long) to compute availableSpace, assign its result to availableSpace and return it; remove references to currentAmount, IItemView iteration, and the dead if (currentAmount == 0) branch so only SupplierModule's availableSpace calculation uses storage.insert(targetKey, Long.MAX_VALUE, true).
🧹 Nitpick comments (5)
common/src/main/java/com/logistics/core/lib/storage/IItemStorage.java (1)
42-46: ⚡ Quick winClarify
contents()contract (currently contradictory).The docs say empty slots “may or may not be included” but the return tag says “non-empty views.” Pick one contract so adapters behave consistently.
Doc-only fix example
- * `@return` an iterable over the non-empty views in this storage + * `@return` an iterable over this storage's views; implementations may include + * empty views unless otherwise documented🤖 Prompt for 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. In `@common/src/main/java/com/logistics/core/lib/storage/IItemStorage.java` around lines 42 - 46, The Javadoc for IItemStorage.contents() is inconsistent: the paragraph says empty slots "may or may not be included" while the `@return` says "non-empty views"; decide on the non-empty contract and update the docs to state clearly that contents() MUST return an Iterable of only non-empty IItemView instances (empty slots must be omitted), then update any implementations and unit tests of IItemStorage.contents() to conform to this contract (search for implementations named contents or classes implementing IItemStorage to adjust behavior).common/src/main/java/com/logistics/pipe/modules/QuickSortModule.java (1)
108-111: 💤 Low valueConsider caching the template stack to avoid repeated allocations.
key.toStack(1)is called multiple times (lines 110, 118, 121, 137, 150) for the same key within this tick cycle. While not a critical issue, caching it could reduce allocations in this hot path.♻️ Optional optimization
IItemView candidateView = slots.get(candidate); IItemKey key = candidateView.resource(); - int stackSize = (int) Math.min(candidateView.amount(), key.toStack(1).getItem().getDefaultMaxStackSize()); - ItemStack queryStack = key.toStack(stackSize); + ItemStack templateStack = key.toStack(1); + int stackSize = (int) Math.min(candidateView.amount(), templateStack.getItem().getDefaultMaxStackSize()); + ItemStack queryStack = key.toStack(stackSize);Then use
templateStack.getItem()for logging instead ofkey.toStack(1).getItem().🤖 Prompt for 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. In `@common/src/main/java/com/logistics/pipe/modules/QuickSortModule.java` around lines 108 - 111, The code repeatedly calls key.toStack(1) for the same IItemKey within this tick; cache that template stack once (e.g., templateStack = key.toStack(1)) and reuse it when computing stackSize (use templateStack.getItem().getDefaultMaxStackSize()), when creating queryStack, and for any logging; update usages in the QuickSortModule block that reference candidateView, key, stackSize, and queryStack to use the cached templateStack instead to avoid repeated allocations.common/src/main/java/com/logistics/pipe/runtime/PipeRuntime.java (1)
441-452: 💤 Low valueUnused parameter in
notifyDropAndDrop.The
insertedparameter is never used — the method always notifies withitem.getStack().getCount(). This is functionally correct for the drop case (we're accounting for the full dropped amount), but the parameter is misleading. Consider removing it for clarity.♻️ Suggested cleanup
- private static void notifyDropAndDrop(Level world, BlockPos pos, TravelingItem item, long inserted) { + private static void notifyDropAndDrop(Level world, BlockPos pos, TravelingItem item) {And update call sites accordingly.
🤖 Prompt for 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. In `@common/src/main/java/com/logistics/pipe/runtime/PipeRuntime.java` around lines 441 - 452, The notifyDropAndDrop method has an unused parameter inserted; remove the inserted parameter from the notifyDropAndDrop signature and from all call sites so the method becomes notifyDropAndDrop(Level world, BlockPos pos, TravelingItem item), and keep the existing behavior of notifying the network with item.getStack().getCount() and calling PipeBlockEntity.dropItem(world, pos, item); update any places that call notifyDropAndDrop to stop passing the now-removed inserted argument.common/src/main/java/com/logistics/core/lib/storage/ItemStorageLookup.java (1)
36-37: 💤 Low valueConsider adding
volatilefor defensive thread safety.The static fields are written once during loader initialization and read from game tick threads. While mod loading order guarantees registration completes before gameplay, adding
volatilewould make the memory visibility guarantees explicit and prevent subtle issues if initialization order ever changes.- private static Finder finder; - private static KeyFactory keyFactory; + private static volatile Finder finder; + private static volatile KeyFactory keyFactory;🤖 Prompt for 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. In `@common/src/main/java/com/logistics/core/lib/storage/ItemStorageLookup.java` around lines 36 - 37, The static fields in ItemStorageLookup (finder and keyFactory) need explicit memory-visibility guarantees: change their declarations to be volatile (e.g., make finder and keyFactory `private static volatile Finder finder;` and `private static volatile KeyFactory keyFactory;`) so writes during loader initialization are guaranteed visible to game tick threads; update only the field declarations in the ItemStorageLookup class to add volatile, no other logic changes required.common/src/main/java/com/logistics/pipe/block/entity/PipeItemStorage.java (1)
49-62: 💤 Low valuePotential integer overflow when casting
acceptedtoint.The
insertmethod acceptsmaxAmountaslongbuttoStack((int) accepted)casts toint. WhilegetInsertableAmountlikely returns a bounded value, ifacceptedexceedsInteger.MAX_VALUE, the cast will silently overflow and create a stack with an incorrect (possibly negative) count.Consider clamping before the cast:
Proposed fix
if (!simulate) { - ItemStack stack = item.toStack((int) accepted); + ItemStack stack = item.toStack((int) Math.min(accepted, Integer.MAX_VALUE)); pipe.acceptInsertedStack(stack, fromDirection, NO_SPEED); }🤖 Prompt for 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. In `@common/src/main/java/com/logistics/pipe/block/entity/PipeItemStorage.java` around lines 49 - 62, The insert method can silently overflow when casting accepted (long) to int for item.toStack; clamp accepted to int range before casting: compute int toInsert = (int)Math.min(accepted, Integer.MAX_VALUE) and use item.toStack(toInsert) and pipe.acceptInsertedStack(...) with that clamped stack to avoid overflow; keep returning the original long accepted (or if business logic requires, adjust the returned value to reflect the actually inserted amount) and reference the insert method, pipe.getInsertableAmount, item.toStack, and pipe.acceptInsertedStack when making the change.
🤖 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 `@common/src/main/java/com/logistics/core/lib/block/BlockEntityUtil.java`:
- Around line 49-57: The comment is incorrect: after building drop stacks (using
remaining, maxStackSize, key.toStack and adding to drops) the code calls
storage.extract(key, amount, false) which performs a real extraction; update the
comment near the loop and the storage.extract call to state that we are
constructing drop stacks from the current contents and then actually removing
that amount from storage by performing a non-simulated extraction via
storage.extract(key, amount, false) so the comment matches the behavior.
In `@common/src/main/java/com/logistics/core/macerator/MaceratorBlockEntity.java`:
- Around line 222-223: The itemStorage(Direction) method ignores the side
parameter, breaking sided inventory rules; update itemStorage to pass the side
into a side-aware storage wrapper (e.g., add a new ContainerItemStorage(this,
side) constructor or factory) and ensure that wrapper delegates sided checks to
the block entity's WorldlyContainer methods (route canPlaceItemThroughFace and
canTakeItemThroughFace using the supplied Direction). Modify or add the
ContainerItemStorage constructor/factory to accept `@Nullable` Direction and use
it when evaluating insertion/extraction permissions so capability consumers
respect the side argument.
In `@common/src/main/java/com/logistics/pipe/block/PipeBlock.java`:
- Around line 488-491: The code treats a non-null ItemStorage as non-connectable
if it has no contents by checking storage.contents().iterator().hasNext();
remove that emptiness check and treat any non-null result from
ItemStorageLookup.find(...) as connectable for PipeConnection.Type.INVENTORY
(i.e., change the condition around ItemStorageLookup.find(world, neighborPos,
direction.getOpposite()) so a non-null storage is sufficient to set candidate =
PipeConnection.Type.INVENTORY), preserving the surrounding logic in PipeBlock
(and keep using the same storage variable and direction/opposite lookup).
In `@common/src/main/java/com/logistics/pipe/modules/BasicExtractorModule.java`:
- Around line 92-104: The loop in BasicExtractorModule recalculates toExtract
per view allowing more than itemsPerPull to be extracted in one tick; fix by
introducing a local allowance (e.g., int allowed = Math.min(itemsPerPull,
remaining)) before iterating storage.contents(), then inside the loop compute
toExtract as Math.min(allowed, remaining) and after a successful extraction
decrement both remaining and allowed by the extracted amount (so allowed tracks
the remaining quota for this tick); update references to remaining/itemsPerPull
in the extraction block (where extract(...), toStack(...), forceAddItem(...),
and anyExtracted are used) so total extracted across the loop never exceeds
itemsPerPull.
In `@fabric/src/main/java/com/logistics/fabric/storage/FabricItemStorage.java`:
- Around line 56-112: asFabric() currently ignores Fabric TransactionContext
semantics (always calling storage.insert/extract with simulate=false) and
unsafely casts view.resource() to FabricItemKey; fix by making insert/extract
honor the provided TransactionContext: when the Fabric transaction requests
simulation, call the underlying storage with simulate=true or buffer the
operation and register a TransactionCloseCallback (or equivalent) to apply
buffered changes only on commit (implement this behavior inside the anonymous
Storage<ItemVariant> insert/extract implementations used by asFabric); and fix
getResource() in the iterator/StorageView by avoiding the direct cast to
FabricItemKey—check if view.resource() is a FabricItemKey and call
FabricItemKey.variant(), otherwise produce the ItemVariant from
view.resource().toStack(1) (e.g., via ItemVariant.of(...) or equivalent) so
non-Fabric IItemKey implementations are supported.
---
Outside diff comments:
In `@common/src/main/java/com/logistics/pipe/modules/SupplierModule.java`:
- Around line 296-313: The loop in SupplierModule calculating availableSpace is
overly complex and uses unreachable branches (storage.contents() only returns
non-empty IItemView), and it underestimates capacity by capping inserts to
targetStack.getMaxStackSize(); replace the entire loop with a single simulated
insert: call storage.insert(targetKey, Long.MAX_VALUE, true) (or another
sufficiently large long) to compute availableSpace, assign its result to
availableSpace and return it; remove references to currentAmount, IItemView
iteration, and the dead if (currentAmount == 0) branch so only SupplierModule's
availableSpace calculation uses storage.insert(targetKey, Long.MAX_VALUE, true).
---
Nitpick comments:
In `@common/src/main/java/com/logistics/core/lib/storage/IItemStorage.java`:
- Around line 42-46: The Javadoc for IItemStorage.contents() is inconsistent:
the paragraph says empty slots "may or may not be included" while the `@return`
says "non-empty views"; decide on the non-empty contract and update the docs to
state clearly that contents() MUST return an Iterable of only non-empty
IItemView instances (empty slots must be omitted), then update any
implementations and unit tests of IItemStorage.contents() to conform to this
contract (search for implementations named contents or classes implementing
IItemStorage to adjust behavior).
In `@common/src/main/java/com/logistics/core/lib/storage/ItemStorageLookup.java`:
- Around line 36-37: The static fields in ItemStorageLookup (finder and
keyFactory) need explicit memory-visibility guarantees: change their
declarations to be volatile (e.g., make finder and keyFactory `private static
volatile Finder finder;` and `private static volatile KeyFactory keyFactory;`)
so writes during loader initialization are guaranteed visible to game tick
threads; update only the field declarations in the ItemStorageLookup class to
add volatile, no other logic changes required.
In `@common/src/main/java/com/logistics/pipe/block/entity/PipeItemStorage.java`:
- Around line 49-62: The insert method can silently overflow when casting
accepted (long) to int for item.toStack; clamp accepted to int range before
casting: compute int toInsert = (int)Math.min(accepted, Integer.MAX_VALUE) and
use item.toStack(toInsert) and pipe.acceptInsertedStack(...) with that clamped
stack to avoid overflow; keep returning the original long accepted (or if
business logic requires, adjust the returned value to reflect the actually
inserted amount) and reference the insert method, pipe.getInsertableAmount,
item.toStack, and pipe.acceptInsertedStack when making the change.
In `@common/src/main/java/com/logistics/pipe/modules/QuickSortModule.java`:
- Around line 108-111: The code repeatedly calls key.toStack(1) for the same
IItemKey within this tick; cache that template stack once (e.g., templateStack =
key.toStack(1)) and reuse it when computing stackSize (use
templateStack.getItem().getDefaultMaxStackSize()), when creating queryStack, and
for any logging; update usages in the QuickSortModule block that reference
candidateView, key, stackSize, and queryStack to use the cached templateStack
instead to avoid repeated allocations.
In `@common/src/main/java/com/logistics/pipe/runtime/PipeRuntime.java`:
- Around line 441-452: The notifyDropAndDrop method has an unused parameter
inserted; remove the inserted parameter from the notifyDropAndDrop signature and
from all call sites so the method becomes notifyDropAndDrop(Level world,
BlockPos pos, TravelingItem item), and keep the existing behavior of notifying
the network with item.getStack().getCount() and calling
PipeBlockEntity.dropItem(world, pos, item); update any places that call
notifyDropAndDrop to stop passing the now-removed inserted argument.
🪄 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 UI
Review profile: CHILL
Plan: Pro
Run ID: def238d3-d023-46d9-86a8-c66685e9f43b
📒 Files selected for processing (69)
common/src/main/java/com/logistics/automation/kiln/KilnBlockEntity.javacommon/src/main/java/com/logistics/core/lib/block/BlockEntityUtil.javacommon/src/main/java/com/logistics/core/lib/block/capability/HasItemStorage.javacommon/src/main/java/com/logistics/core/lib/items/ItemInventoryComponent.javacommon/src/main/java/com/logistics/core/lib/network/CrafterSnapshot.javacommon/src/main/java/com/logistics/core/lib/network/ILogisticsNetwork.javacommon/src/main/java/com/logistics/core/lib/network/IWorldView.javacommon/src/main/java/com/logistics/core/lib/network/IngredientChecker.javacommon/src/main/java/com/logistics/core/lib/network/ItemRequest.javacommon/src/main/java/com/logistics/core/lib/network/NetworkCommand.javacommon/src/main/java/com/logistics/core/lib/network/Order.javacommon/src/main/java/com/logistics/core/lib/network/PlanningView.javacommon/src/main/java/com/logistics/core/lib/network/ProviderCanFulfill.javacommon/src/main/java/com/logistics/core/lib/network/RecipeIngredient.javacommon/src/main/java/com/logistics/core/lib/pipe/DispatchableModule.javacommon/src/main/java/com/logistics/core/lib/pipe/Module.javacommon/src/main/java/com/logistics/core/lib/storage/ContainerItemStorage.javacommon/src/main/java/com/logistics/core/lib/storage/IItemKey.javacommon/src/main/java/com/logistics/core/lib/storage/IItemStorage.javacommon/src/main/java/com/logistics/core/lib/storage/IItemView.javacommon/src/main/java/com/logistics/core/lib/storage/ItemStorageLookup.javacommon/src/main/java/com/logistics/core/macerator/MaceratorBlockEntity.javacommon/src/main/java/com/logistics/pipe/ChassisPipe.javacommon/src/main/java/com/logistics/pipe/Pipe.javacommon/src/main/java/com/logistics/pipe/block/PipeBlock.javacommon/src/main/java/com/logistics/pipe/block/entity/PipeBlockEntity.javacommon/src/main/java/com/logistics/pipe/block/entity/PipeItemStorage.javacommon/src/main/java/com/logistics/pipe/modules/AdvancedExtractorModule.javacommon/src/main/java/com/logistics/pipe/modules/BasicExtractorModule.javacommon/src/main/java/com/logistics/pipe/modules/CraftingModule.javacommon/src/main/java/com/logistics/pipe/modules/ExtractionModule.javacommon/src/main/java/com/logistics/pipe/modules/InsertionModule.javacommon/src/main/java/com/logistics/pipe/modules/PassiveSupplierModule.javacommon/src/main/java/com/logistics/pipe/modules/PolymorphicSinkModule.javacommon/src/main/java/com/logistics/pipe/modules/ProcessModule.javacommon/src/main/java/com/logistics/pipe/modules/ProviderModule.javacommon/src/main/java/com/logistics/pipe/modules/QuickSortModule.javacommon/src/main/java/com/logistics/pipe/modules/RequesterModule.javacommon/src/main/java/com/logistics/pipe/modules/SupplierModule.javacommon/src/main/java/com/logistics/pipe/network/ItemReservation.javacommon/src/main/java/com/logistics/pipe/network/JobCoordinator.javacommon/src/main/java/com/logistics/pipe/network/MinecraftWorldView.javacommon/src/main/java/com/logistics/pipe/network/NetworkController.javacommon/src/main/java/com/logistics/pipe/network/NetworkJob.javacommon/src/main/java/com/logistics/pipe/network/NetworkSnapshotBuilder.javacommon/src/main/java/com/logistics/pipe/network/PipeNetwork.javacommon/src/main/java/com/logistics/pipe/network/PlanNode.javacommon/src/main/java/com/logistics/pipe/network/PlanningContext.javacommon/src/main/java/com/logistics/pipe/network/ProviderSnapshot.javacommon/src/main/java/com/logistics/pipe/network/RequestPlanner.javacommon/src/main/java/com/logistics/pipe/network/ReservationManager.javacommon/src/main/java/com/logistics/pipe/network/WorkOrder.javacommon/src/main/java/com/logistics/pipe/runtime/PipeRuntime.javacommon/src/main/java/com/logistics/power/engine/block/entity/StirlingEngineBlockEntity.javacommon/src/test/java/com/logistics/pipe/network/JobCoordinatorTest.javacommon/src/test/java/com/logistics/pipe/network/NetworkControllerTest.javacommon/src/test/java/com/logistics/pipe/network/ReconciliationServiceTest.javacommon/src/test/java/com/logistics/pipe/network/RequestPlannerTest.javacommon/src/test/java/com/logistics/pipe/network/ReservationManagerTest.javacommon/src/test/java/com/logistics/pipe/network/SinkResolverTest.javacommon/src/test/java/com/logistics/test/MinecraftTestEnvironment.javacommon/src/test/java/com/logistics/test/TestItemKey.javafabric/src/gametest/java/com/logistics/gametest/automation/KilnGameTest.javafabric/src/gametest/java/com/logistics/gametest/power/EngineGameTest.javafabric/src/main/java/com/logistics/fabric/capability/FabricCapabilityRegistration.javafabric/src/main/java/com/logistics/fabric/capability/ItemStorageAccess.javafabric/src/main/java/com/logistics/fabric/storage/FabricItemKey.javafabric/src/main/java/com/logistics/fabric/storage/FabricItemStorage.javafabric/src/main/java/com/logistics/fabric/storage/FabricItemView.java
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
common/src/main/java/com/logistics/pipe/network/NetworkController.java (1)
224-226:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winSkip deferred providers before choosing the dispatch source.
nextDispatchable()only bails out when all providers are deferred, buttryDispatch()still blindly takesentries.getFirst(). If the first provider was deferred and another provider still exists, the same deferred provider can be selected again every loop, which can stallPipeNetwork.tick()in a retry cycle.Suggested fix
- SupplyEntry supply = entries.getFirst(); + SupplyEntry supply = entries.stream() + .filter(e -> !deferredProviders.contains(e.pos)) + .findFirst() + .orElse(null); + if (supply == null) return null; ... - entries.removeFirst(); + entries.remove(supply); if (entries.isEmpty()) supplyTable.remove(order.item()); ... - entries.removeFirst(); + entries.remove(supply); if (entries.isEmpty()) supplyTable.remove(order.item());Also applies to: 242-245, 253-255, 301-302
🤖 Prompt for 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. In `@common/src/main/java/com/logistics/pipe/network/NetworkController.java` around lines 224 - 226, nextDispatchable() only skips when all providers are deferred but tryDispatch() uses entries.getFirst(), causing a deferred provider to be repeatedly chosen; change the call sites (where tryDispatch(orderIt, order, entries) is invoked) to first select the first non-deferred provider from entries (e.g., find first Entry e where !deferredProviders.contains(e.pos)) and pass a entries view or starting index that begins at that non-deferred provider, or filter out deferred entries before calling tryDispatch; update the logic used in tryDispatch (and the other similar call sites that use entries.getFirst()) to respect deferredProviders so the dispatch source is never a deferred provider, and apply the same change to the other occurrences referenced (the other tryDispatch call sites).common/src/main/java/com/logistics/pipe/modules/ProcessModule.java (1)
308-337:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRollback already-created input orders when dispatch aborts.
If one input order is placed and a later input fails satellite resolution, this method returns
0but leaves the earlier order(s) live. That can leak reservations and route ingredients for a process job that was never queued.Possible localized fix
orderDest = network.findSatellite(effectiveDest); if (orderDest == null) { + for (int j = 0; j < orderIds.size(); j++) { + String uuidStr = NbtCompat.getStringAt(orderIds, j, ""); + if (!uuidStr.isEmpty()) { + try { network.cancelOrder(UUID.fromString(uuidStr)); } catch (Exception ignored) {} + } + } NetDbg.out("[Process @ {}] Dispatch rejected: satellite '{}' not found for input {}", ctx.pos(), effectiveDest, i); LogisticsPipe.LOGGER.warn("[Process @ {}] Satellite '{}' not found for input {}; aborting dispatch", ctx.pos(), effectiveDest, i); return 0; }🤖 Prompt for 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. In `@common/src/main/java/com/logistics/pipe/modules/ProcessModule.java` around lines 308 - 337, The code in ProcessModule places multiple input orders (using network.placeOrder) but if a later input fails satellite resolution the method returns 0 without cancelling earlier orders; change the logic to track created order UUIDs (e.g., a List<UUID> createdOrderIds alongside the existing orderIds ListTag) and when you hit the satellite-not-found branch (where network.findSatellite(effectiveDest) returns null) iterate createdOrderIds and call the network cancel API for each created order (e.g., network.cancelOrder(orderId)) before logging and returning 0, ensuring you clean up any reservations created by network.placeOrder and keep orderIds consistent.
♻️ Duplicate comments (1)
common/src/main/java/com/logistics/automation/kiln/KilnBlockEntity.java (1)
200-201:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRespect the
sideargument when exposing item storage.Line 201 ignores
side, so callers can bypass kiln face restrictions defined byWorldlyContainersided methods.Suggested fix
`@Override` public IItemStorage itemStorage(`@Nullable` Direction side) { - return new ContainerItemStorage(this); + return new ContainerItemStorage(this, side); }🤖 Prompt for 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. In `@common/src/main/java/com/logistics/automation/kiln/KilnBlockEntity.java` around lines 200 - 201, KilnBlockEntity.itemStorage currently ignores the side parameter and returns a raw ContainerItemStorage, bypassing WorldlyContainer sided rules; change itemStorage(`@Nullable` Direction side) to return a storage that enforces sided access (e.g., wrap or construct a sided-aware adapter using the KilnBlockEntity/WorldlyContainer APIs) so interactions respect the provided side—replace the direct new ContainerItemStorage(this) with a sided wrapper that consults the side argument when exposing slots.
🧹 Nitpick comments (4)
common/src/main/java/com/logistics/core/lib/storage/IItemView.java (1)
3-15: ⚡ Quick winClarify whether
IItemViewentries can be empty/zero-amount.Please make the contract explicit (e.g., non-null
resource()andamount() > 0) so adapters and consumers implement the same semantics.📝 Proposed Javadoc refinement
/** * Read-only view of a slot or resource within an {`@link` IItemStorage}. * * <p>Equivalent to Fabric's {`@code` StorageView<ItemVariant>} in the loader-agnostic API. * Instances are obtained by iterating {`@link` IItemStorage#contents()}. + * Implementations should expose only non-empty views: + * {`@link` `#resource`()} must be non-null and {`@link` `#amount`()} must be {`@code` > 0}. */ public interface IItemView {🤖 Prompt for 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. In `@common/src/main/java/com/logistics/core/lib/storage/IItemView.java` around lines 3 - 15, Clarify the IItemView contract by updating its Javadoc to state explicitly that resource() never returns null and amount() is always > 0 for any entry produced by IItemStorage#contents(); ensure adapters and consumers enforce/assume these semantics (e.g., validate or filter out empty slots at the storage iteration boundary) and document whether implementations must throw/avoid creating views with amount() == 0 so everyone implements the same non-empty-view invariant for IItemView, resource(), and amount().fabric/src/main/java/com/logistics/fabric/storage/FabricItemKey.java (1)
17-19: ⚡ Quick winAdd a fail-fast null guard in constructor.
A null
ItemVariantwould currently fail later in unrelated methods; fail early at construction.Suggested patch
+import java.util.Objects; + public final class FabricItemKey implements IItemKey { @@ private final ItemVariant variant; public FabricItemKey(ItemVariant variant) { - this.variant = variant; + this.variant = Objects.requireNonNull(variant, "variant"); }🤖 Prompt for 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. In `@fabric/src/main/java/com/logistics/fabric/storage/FabricItemKey.java` around lines 17 - 19, The FabricItemKey constructor currently accepts a possibly null ItemVariant which defers failures; add a fail-fast null guard in the FabricItemKey(ItemVariant variant) constructor (the FabricItemKey class and its variant field) by validating variant != null and throwing an appropriate unchecked exception (e.g., NullPointerException or IllegalArgumentException) with a clear message if null, before assigning this.variant = variant.common/src/main/java/com/logistics/core/lib/storage/IItemStorage.java (1)
19-27: ⚡ Quick winTighten contract semantics for non-positive amounts and return bounds.
Please define
maxAmount <= 0behavior and explicitly require non-negative return values for both methods to keep adapters consistent.Suggested Javadoc tightening
/** * Insert items into this storage. @@ - * `@param` maxAmount the maximum number of items to insert + * `@param` maxAmount the maximum number of items to insert; non-positive values must return {`@code` 0} @@ - * `@return` the amount actually inserted (or that would be inserted if simulating) + * `@return` the amount actually inserted (or that would be inserted if simulating), always {`@code` >= 0} */ @@ /** * Extract items from this storage. @@ - * `@param` maxAmount the maximum number of items to extract + * `@param` maxAmount the maximum number of items to extract; non-positive values must return {`@code` 0} @@ - * `@return` the amount actually extracted (or that would be extracted if simulating) + * `@return` the amount actually extracted (or that would be extracted if simulating), always {`@code` >= 0} */Also applies to: 30-37
🤖 Prompt for 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. In `@common/src/main/java/com/logistics/core/lib/storage/IItemStorage.java` around lines 19 - 27, Update the Javadoc for IItemStorage.insert (and likewise for IItemStorage.extract) to explicitly define behavior when maxAmount <= 0 (treat as no-op and return 0) and require implementations to always return a non-negative value not exceeding the requested maxAmount; mention that simulate=true performs a dry run without mutating state and the returned value is the amount that would be inserted/extracted. Reference the methods by name (insert, extract) and tighten the contract so adapters can rely on non-negative, bounded return values and consistent handling of non-positive input amounts.common/src/main/java/com/logistics/core/lib/network/ILogisticsNetwork.java (1)
34-38: ⚡ Quick winUpdate the API docs to describe
IItemKey, not “item variant”.This interface is now one of the main loader-agnostic extension points, so the stale Fabric-era wording is misleading for callers and addon authors.
Also applies to: 67-78, 117-118, 138-138
🤖 Prompt for 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. In `@common/src/main/java/com/logistics/core/lib/network/ILogisticsNetwork.java` around lines 34 - 38, Update the Javadoc in the ILogisticsNetwork interface to replace the stale phrase "item variant" with "IItemKey" (and briefly describe that IItemKey is the loader-agnostic item identifier) wherever it appears (e.g., the registerSupply method signature and the other Javadoc blocks in this interface). Ensure the parameter descriptions (for maps/keys/collections) reference IItemKey instead of "item variant" and adjust phrasing to make it clear callers should pass IItemKey instances as the item identifier for supply/request/storage-related methods.
🤖 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 `@common/src/main/java/com/logistics/core/lib/block/BlockEntityUtil.java`:
- Around line 41-58: BlockEntityUtil iterates storage.contents() and calls
storage.extract() during that iteration which can break on live iterables; fix
by first collecting the IItemView elements into a temporary List (e.g.,
List<IItemView> views = new ArrayList<>(); for (IItemView v :
storage.contents()) views.add(v);) and then iterate that list to build drops and
call storage.extract(key, amount, false); ensure you reference the same
variables (IItemView, IItemKey, amount, storage.extract) so behavior is
unchanged but iteration is now safe for live iterables.
In
`@common/src/main/java/com/logistics/core/lib/storage/ContainerItemStorage.java`:
- Around line 58-74: The insertIntoSlot method misses a container.setChanged()
call when placing a new stack into an empty slot: after the branch that does
container.setItem(slot, item.toStack((int) toInsert)) (inside insertIntoSlot)
add a container.setChanged() when simulate is false and toInsert > 0 so the
container is marked dirty like the existing-stack branch; ensure you only call
setChanged() in the non-simulate path to mirror the grow((int) toInsert)
behavior.
In `@fabric/src/main/java/com/logistics/fabric/storage/FabricItemStorage.java`:
- Around line 129-143: The insert and extract methods in FabricItemStorage
unsafely cast IItemKey to FabricItemKey (in insert(...) and extract(...)),
risking ClassCastException; change both methods to follow the getResource()
pattern: detect if the incoming IItemKey is a FabricItemKey and use its
variant(), otherwise convert/fallback via the same helper/path used by
getResource() to obtain an ItemVariant (or handle non-Fabric keys safely) before
calling storage.insert(...) or storage.extract(...), preserving the existing
simulate/transaction semantics.
- Around line 97-101: StorageView.extract in FabricItemStorage currently
constructs an IItemKey (FabricItemKey) then calls storage.extract(key,
maxAmount, false), ignoring the provided
net.fabricmc.fabric.api.transfer.v1.transaction.TransactionContext tx and thus
bypassing Fabric transaction semantics; change the implementation to use the
provided tx so extractions participate in rollback — i.e. mirror the outer
insert/extract pattern: convert the ItemVariant to IItemKey (FabricItemKey) as
before, then call the transaction-aware storage.extract path by forwarding or
adapting tx (use the same tx->storage callback/wrapper used elsewhere in this
class) so storage.extract receives the TransactionContext (or is invoked inside
tx.run/adapter) instead of hardcoding false. Ensure you reference
StorageView.extract, FabricItemKey, storage.extract and the TransactionContext
tx when making the change.
In `@fabric/src/main/java/com/logistics/fabric/storage/FabricItemView.java`:
- Around line 19-22: Update FabricItemStorage.contents() to skip StorageView
entries whose amount() == 0 before wrapping them in FabricItemView so callers no
longer receive blank views; specifically iterate the StorageView iterator
(StorageView.iterator()), filter out views where view.amount() == 0, and only
create new FabricItemView(view) for non-empty views, ensuring
FabricItemView.resource() and other callers no longer need to perform redundant
amount checks.
---
Outside diff comments:
In `@common/src/main/java/com/logistics/pipe/modules/ProcessModule.java`:
- Around line 308-337: The code in ProcessModule places multiple input orders
(using network.placeOrder) but if a later input fails satellite resolution the
method returns 0 without cancelling earlier orders; change the logic to track
created order UUIDs (e.g., a List<UUID> createdOrderIds alongside the existing
orderIds ListTag) and when you hit the satellite-not-found branch (where
network.findSatellite(effectiveDest) returns null) iterate createdOrderIds and
call the network cancel API for each created order (e.g.,
network.cancelOrder(orderId)) before logging and returning 0, ensuring you clean
up any reservations created by network.placeOrder and keep orderIds consistent.
In `@common/src/main/java/com/logistics/pipe/network/NetworkController.java`:
- Around line 224-226: nextDispatchable() only skips when all providers are
deferred but tryDispatch() uses entries.getFirst(), causing a deferred provider
to be repeatedly chosen; change the call sites (where tryDispatch(orderIt,
order, entries) is invoked) to first select the first non-deferred provider from
entries (e.g., find first Entry e where !deferredProviders.contains(e.pos)) and
pass a entries view or starting index that begins at that non-deferred provider,
or filter out deferred entries before calling tryDispatch; update the logic used
in tryDispatch (and the other similar call sites that use entries.getFirst()) to
respect deferredProviders so the dispatch source is never a deferred provider,
and apply the same change to the other occurrences referenced (the other
tryDispatch call sites).
---
Duplicate comments:
In `@common/src/main/java/com/logistics/automation/kiln/KilnBlockEntity.java`:
- Around line 200-201: KilnBlockEntity.itemStorage currently ignores the side
parameter and returns a raw ContainerItemStorage, bypassing WorldlyContainer
sided rules; change itemStorage(`@Nullable` Direction side) to return a storage
that enforces sided access (e.g., wrap or construct a sided-aware adapter using
the KilnBlockEntity/WorldlyContainer APIs) so interactions respect the provided
side—replace the direct new ContainerItemStorage(this) with a sided wrapper that
consults the side argument when exposing slots.
---
Nitpick comments:
In `@common/src/main/java/com/logistics/core/lib/network/ILogisticsNetwork.java`:
- Around line 34-38: Update the Javadoc in the ILogisticsNetwork interface to
replace the stale phrase "item variant" with "IItemKey" (and briefly describe
that IItemKey is the loader-agnostic item identifier) wherever it appears (e.g.,
the registerSupply method signature and the other Javadoc blocks in this
interface). Ensure the parameter descriptions (for maps/keys/collections)
reference IItemKey instead of "item variant" and adjust phrasing to make it
clear callers should pass IItemKey instances as the item identifier for
supply/request/storage-related methods.
In `@common/src/main/java/com/logistics/core/lib/storage/IItemStorage.java`:
- Around line 19-27: Update the Javadoc for IItemStorage.insert (and likewise
for IItemStorage.extract) to explicitly define behavior when maxAmount <= 0
(treat as no-op and return 0) and require implementations to always return a
non-negative value not exceeding the requested maxAmount; mention that
simulate=true performs a dry run without mutating state and the returned value
is the amount that would be inserted/extracted. Reference the methods by name
(insert, extract) and tighten the contract so adapters can rely on non-negative,
bounded return values and consistent handling of non-positive input amounts.
In `@common/src/main/java/com/logistics/core/lib/storage/IItemView.java`:
- Around line 3-15: Clarify the IItemView contract by updating its Javadoc to
state explicitly that resource() never returns null and amount() is always > 0
for any entry produced by IItemStorage#contents(); ensure adapters and consumers
enforce/assume these semantics (e.g., validate or filter out empty slots at the
storage iteration boundary) and document whether implementations must
throw/avoid creating views with amount() == 0 so everyone implements the same
non-empty-view invariant for IItemView, resource(), and amount().
In `@fabric/src/main/java/com/logistics/fabric/storage/FabricItemKey.java`:
- Around line 17-19: The FabricItemKey constructor currently accepts a possibly
null ItemVariant which defers failures; add a fail-fast null guard in the
FabricItemKey(ItemVariant variant) constructor (the FabricItemKey class and its
variant field) by validating variant != null and throwing an appropriate
unchecked exception (e.g., NullPointerException or IllegalArgumentException)
with a clear message if null, before assigning this.variant = variant.
🪄 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 UI
Review profile: CHILL
Plan: Pro
Run ID: 0a262c54-ddc0-42c6-b2eb-3a35ead2e219
📒 Files selected for processing (69)
common/src/main/java/com/logistics/automation/kiln/KilnBlockEntity.javacommon/src/main/java/com/logistics/core/lib/block/BlockEntityUtil.javacommon/src/main/java/com/logistics/core/lib/block/capability/HasItemStorage.javacommon/src/main/java/com/logistics/core/lib/items/ItemInventoryComponent.javacommon/src/main/java/com/logistics/core/lib/network/CrafterSnapshot.javacommon/src/main/java/com/logistics/core/lib/network/ILogisticsNetwork.javacommon/src/main/java/com/logistics/core/lib/network/IWorldView.javacommon/src/main/java/com/logistics/core/lib/network/IngredientChecker.javacommon/src/main/java/com/logistics/core/lib/network/ItemRequest.javacommon/src/main/java/com/logistics/core/lib/network/NetworkCommand.javacommon/src/main/java/com/logistics/core/lib/network/Order.javacommon/src/main/java/com/logistics/core/lib/network/PlanningView.javacommon/src/main/java/com/logistics/core/lib/network/ProviderCanFulfill.javacommon/src/main/java/com/logistics/core/lib/network/RecipeIngredient.javacommon/src/main/java/com/logistics/core/lib/pipe/DispatchableModule.javacommon/src/main/java/com/logistics/core/lib/pipe/Module.javacommon/src/main/java/com/logistics/core/lib/storage/ContainerItemStorage.javacommon/src/main/java/com/logistics/core/lib/storage/IItemKey.javacommon/src/main/java/com/logistics/core/lib/storage/IItemStorage.javacommon/src/main/java/com/logistics/core/lib/storage/IItemView.javacommon/src/main/java/com/logistics/core/lib/storage/ItemStorageLookup.javacommon/src/main/java/com/logistics/core/macerator/MaceratorBlockEntity.javacommon/src/main/java/com/logistics/pipe/ChassisPipe.javacommon/src/main/java/com/logistics/pipe/Pipe.javacommon/src/main/java/com/logistics/pipe/block/PipeBlock.javacommon/src/main/java/com/logistics/pipe/block/entity/PipeBlockEntity.javacommon/src/main/java/com/logistics/pipe/block/entity/PipeItemStorage.javacommon/src/main/java/com/logistics/pipe/modules/AdvancedExtractorModule.javacommon/src/main/java/com/logistics/pipe/modules/BasicExtractorModule.javacommon/src/main/java/com/logistics/pipe/modules/CraftingModule.javacommon/src/main/java/com/logistics/pipe/modules/ExtractionModule.javacommon/src/main/java/com/logistics/pipe/modules/InsertionModule.javacommon/src/main/java/com/logistics/pipe/modules/PassiveSupplierModule.javacommon/src/main/java/com/logistics/pipe/modules/PolymorphicSinkModule.javacommon/src/main/java/com/logistics/pipe/modules/ProcessModule.javacommon/src/main/java/com/logistics/pipe/modules/ProviderModule.javacommon/src/main/java/com/logistics/pipe/modules/QuickSortModule.javacommon/src/main/java/com/logistics/pipe/modules/RequesterModule.javacommon/src/main/java/com/logistics/pipe/modules/SupplierModule.javacommon/src/main/java/com/logistics/pipe/network/ItemReservation.javacommon/src/main/java/com/logistics/pipe/network/JobCoordinator.javacommon/src/main/java/com/logistics/pipe/network/MinecraftWorldView.javacommon/src/main/java/com/logistics/pipe/network/NetworkController.javacommon/src/main/java/com/logistics/pipe/network/NetworkJob.javacommon/src/main/java/com/logistics/pipe/network/NetworkSnapshotBuilder.javacommon/src/main/java/com/logistics/pipe/network/PipeNetwork.javacommon/src/main/java/com/logistics/pipe/network/PlanNode.javacommon/src/main/java/com/logistics/pipe/network/PlanningContext.javacommon/src/main/java/com/logistics/pipe/network/ProviderSnapshot.javacommon/src/main/java/com/logistics/pipe/network/RequestPlanner.javacommon/src/main/java/com/logistics/pipe/network/ReservationManager.javacommon/src/main/java/com/logistics/pipe/network/WorkOrder.javacommon/src/main/java/com/logistics/pipe/runtime/PipeRuntime.javacommon/src/main/java/com/logistics/power/engine/block/entity/StirlingEngineBlockEntity.javacommon/src/test/java/com/logistics/pipe/network/JobCoordinatorTest.javacommon/src/test/java/com/logistics/pipe/network/NetworkControllerTest.javacommon/src/test/java/com/logistics/pipe/network/ReconciliationServiceTest.javacommon/src/test/java/com/logistics/pipe/network/RequestPlannerTest.javacommon/src/test/java/com/logistics/pipe/network/ReservationManagerTest.javacommon/src/test/java/com/logistics/pipe/network/SinkResolverTest.javacommon/src/test/java/com/logistics/test/MinecraftTestEnvironment.javacommon/src/test/java/com/logistics/test/TestItemKey.javafabric/src/gametest/java/com/logistics/gametest/automation/KilnGameTest.javafabric/src/gametest/java/com/logistics/gametest/power/EngineGameTest.javafabric/src/main/java/com/logistics/fabric/capability/FabricCapabilityRegistration.javafabric/src/main/java/com/logistics/fabric/capability/ItemStorageAccess.javafabric/src/main/java/com/logistics/fabric/storage/FabricItemKey.javafabric/src/main/java/com/logistics/fabric/storage/FabricItemStorage.javafabric/src/main/java/com/logistics/fabric/storage/FabricItemView.java
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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
`@common/src/main/java/com/logistics/core/lib/storage/ContainerItemStorage.java`:
- Around line 106-119: The contents() method currently iterates all slots but
insert()/extract() apply side-scoped slot filtering; update contents() to mirror
that behavior by checking if side != null and container instanceof
WorldlyContainer and, if so, iterate only the slots returned by
((WorldlyContainer)container).getSlotsForFace(side), otherwise iterate all slots
as before; refactor the ItemView creation into a small helper (e.g.,
createItemView(ItemStack) or createItemViewForSlot(int)) and use it from
contents() to reduce duplication and keep slot access consistent with
insert()/extract().
- Around line 58-74: insertIntoSlot currently only respects the item's max stack
size and can exceed the container's per-slot limit; update both branches to
clamp insertion by the container's limit (container.getMaxStackSize()) as well
as the item's template/current stack limit. For the empty-slot branch (inside
insertIntoSlot where current.isEmpty()), compute toInsert = min(remaining,
template.getMaxStackSize(), container.getMaxStackSize()) before creating the
stack and calling container.setItem/container.setChanged. For the existing-stack
branch, compute slotLimit = min(current.getMaxStackSize(),
container.getMaxStackSize()), then canFit = max(0, slotLimit -
current.getCount()) and use toInsert = min(remaining, canFit); only call
current.grow and container.setChanged when toInsert > 0 and !simulate. This
prevents overfilling and avoids negative canFit.
🪄 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 UI
Review profile: CHILL
Plan: Pro
Run ID: 39e15ad9-c67c-4570-a740-28c1411be660
📒 Files selected for processing (10)
common/src/main/java/com/logistics/automation/kiln/KilnBlockEntity.javacommon/src/main/java/com/logistics/core/lib/block/BlockEntityUtil.javacommon/src/main/java/com/logistics/core/lib/network/ILogisticsNetwork.javacommon/src/main/java/com/logistics/core/lib/storage/ContainerItemStorage.javacommon/src/main/java/com/logistics/core/lib/storage/IItemStorage.javacommon/src/main/java/com/logistics/core/lib/storage/IItemView.javacommon/src/main/java/com/logistics/pipe/modules/ProcessModule.javacommon/src/main/java/com/logistics/pipe/network/NetworkController.javafabric/src/main/java/com/logistics/fabric/storage/FabricItemKey.javafabric/src/main/java/com/logistics/fabric/storage/FabricItemStorage.java
✅ Files skipped from review due to trivial changes (3)
- common/src/main/java/com/logistics/core/lib/storage/IItemView.java
- common/src/main/java/com/logistics/core/lib/storage/IItemStorage.java
- common/src/main/java/com/logistics/core/lib/network/ILogisticsNetwork.java
🚧 Files skipped from review as they are similar to previous changes (4)
- fabric/src/main/java/com/logistics/fabric/storage/FabricItemStorage.java
- common/src/main/java/com/logistics/pipe/modules/ProcessModule.java
- common/src/main/java/com/logistics/pipe/network/NetworkController.java
- common/src/main/java/com/logistics/core/lib/block/BlockEntityUtil.java
- Fix BlockEntityUtil comment: label the extraction as real, not simulated - Mandate non-empty views in IItemStorage.contents() Javadoc - Add volatile to ItemStorageLookup.finder and keyFactory fields - Remove contents().hasNext() guard from PipeBlock inventory detection - Clamp PipeItemStorage accepted count before int cast - Cache key.toStack(1) in QuickSortModule to avoid redundant allocation - Remove unused inserted param from PipeRuntime.notifyDropAndDrop - Simplify SupplierModule.getAvailableSpace to single insert(simulate=true) - Fix BasicExtractorModule to cap total extraction to itemsPerPull - Add Direction-aware ContainerItemStorage constructor; pass side in MaceratorBlockEntity - Fix FabricItemStorage.asFabric to honor TransactionContext via close callback; fix unsafe FabricItemKey cast in getResource
- Snapshot contents() before extracting in BlockEntityUtil to avoid live-iterable mutation - Add container.setChanged() in ContainerItemStorage empty-slot insert path - Pass side to ContainerItemStorage in KilnBlockEntity (WorldlyContainer sided access) - Fix unsafe FabricItemKey cast in FabricItemStorage insert/extract methods - Honor TransactionContext in FabricItemStorage StorageView.extract via close callback - Filter zero-amount views from FabricItemStorage.contents() iterator - Add null guard to FabricItemKey constructor - Cancel earlier orders in ProcessModule when satellite resolution fails mid-loop - Fix NetworkController.tryDispatch to skip deferred providers via index rather than getFirst() - Tighten IItemStorage insert/extract contract Javadoc (maxAmount <= 0, return bounds) - Clarify IItemView contract: resource() non-null, amount() always > 0 - Fix ILogisticsNetwork placeOrder Javadoc: "item variant" -> "IItemKey"
…cope contents() to sided slots
Indemnity83
force-pushed
the
feat/define-itemstorage-transaction-contract
branch
from
27e49e0 to
ab1884f
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@common/src/main/java/com/logistics/pipe/block/PipeBlock.java`:
- Around line 488-490: The current null check against
ItemStorageLookup.find(...) in PipeBlock incorrectly treats non-null sided
wrappers with zero accessible slots as valid inventories; instead, after
obtaining storage (via ItemStorageLookup.find(world, neighborPos,
direction.getOpposite())), explicitly verify that the storage actually exposes
at least one accessible slot on that side (for example by querying the storage
for slot views/iterators for that side, or testing whether any slot allows
insertion/extraction for the given direction) and only classify the face as
INVENTORY when such sided-accessibility is present; update the branch around
storage (the variable named storage and the logic that marks faces as INVENTORY)
to treat storages with no accessible slots as if null so pipes won’t connect to
dead faces.
In `@common/src/main/java/com/logistics/pipe/modules/QuickSortModule.java`:
- Around line 64-66: QuickSortModule currently persists LAST_SLOT_SCANNED /
LAST_SUCCESS_SLOT assuming storage.contents() are stable indices, but the new
storage.contents() API returns ephemeral IItemView objects and extraction is
key-wide via extract(...); update QuickSortModule to stop storing numeric slot
indices across ticks — instead track a stable item key or view identity provided
by the storage API (e.g., use the IItemView's stable key/identifier if
available) or switch to re-resolving candidateView each tick by searching
storage.contents() for a matching key before calling extract(...); remove
reliance on candidate index comparisons and adjust any logic that references
LAST_SLOT_SCANNED / LAST_SUCCESS_SLOT (and the code paths around candidateView,
storage.contents(), and extract(...)) so decisions are based on item keys or
fresh view lookups rather than persisted slot numbers.
In
`@common/src/main/java/com/logistics/power/engine/block/entity/StirlingEngineBlockEntity.java`:
- Around line 148-168: In StirlingEngineBlockEntity.insert, the empty-slot
branch currently places maxAmount items without clamping to the item's stack
limit; change the logic to use the already-created incoming ItemStack but with
its count limited to Math.min(maxAmount, incoming.getMaxStackSize()) (or
equivalent) so you never set an oversized stack; when !simulate set
inventory.setItem(0, the clamped incoming) and return the clamped amount,
otherwise return the clamped amount without mutating inventory.
In `@fabric/src/main/java/com/logistics/fabric/storage/FabricItemStorage.java`:
- Around line 97-107: The extract implementation in
FabricItemStorage.StorageView incorrectly allows extracting any requested
resource and uses the caller maxAmount instead of the view's available amount;
update the StorageView.extract method to first compare the requested ItemVariant
with this view's getResource() (or construct keys from getResource()) and
immediately return 0 if they don't match, then compute the allowed amount as
Math.min(maxAmount, getAmount()) and use that bounded value for the simulated
storage.extract call and the final commit callback (still using
IItemKey/FabricItemKey and storage.extract for the actual mutation); keep the
TransactionContext callback logic but ensure it commits only the simulated
bounded amount.
🪄 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 UI
Review profile: CHILL
Plan: Pro
Run ID: 4c4e0055-a5e7-4ad8-bf68-f7fef9f6e09b
📒 Files selected for processing (69)
common/src/main/java/com/logistics/automation/kiln/KilnBlockEntity.javacommon/src/main/java/com/logistics/core/lib/block/BlockEntityUtil.javacommon/src/main/java/com/logistics/core/lib/block/capability/HasItemStorage.javacommon/src/main/java/com/logistics/core/lib/items/ItemInventoryComponent.javacommon/src/main/java/com/logistics/core/lib/network/CrafterSnapshot.javacommon/src/main/java/com/logistics/core/lib/network/ILogisticsNetwork.javacommon/src/main/java/com/logistics/core/lib/network/IWorldView.javacommon/src/main/java/com/logistics/core/lib/network/IngredientChecker.javacommon/src/main/java/com/logistics/core/lib/network/ItemRequest.javacommon/src/main/java/com/logistics/core/lib/network/NetworkCommand.javacommon/src/main/java/com/logistics/core/lib/network/Order.javacommon/src/main/java/com/logistics/core/lib/network/PlanningView.javacommon/src/main/java/com/logistics/core/lib/network/ProviderCanFulfill.javacommon/src/main/java/com/logistics/core/lib/network/RecipeIngredient.javacommon/src/main/java/com/logistics/core/lib/pipe/DispatchableModule.javacommon/src/main/java/com/logistics/core/lib/pipe/Module.javacommon/src/main/java/com/logistics/core/lib/storage/ContainerItemStorage.javacommon/src/main/java/com/logistics/core/lib/storage/IItemKey.javacommon/src/main/java/com/logistics/core/lib/storage/IItemStorage.javacommon/src/main/java/com/logistics/core/lib/storage/IItemView.javacommon/src/main/java/com/logistics/core/lib/storage/ItemStorageLookup.javacommon/src/main/java/com/logistics/core/macerator/MaceratorBlockEntity.javacommon/src/main/java/com/logistics/pipe/ChassisPipe.javacommon/src/main/java/com/logistics/pipe/Pipe.javacommon/src/main/java/com/logistics/pipe/block/PipeBlock.javacommon/src/main/java/com/logistics/pipe/block/entity/PipeBlockEntity.javacommon/src/main/java/com/logistics/pipe/block/entity/PipeItemStorage.javacommon/src/main/java/com/logistics/pipe/modules/AdvancedExtractorModule.javacommon/src/main/java/com/logistics/pipe/modules/BasicExtractorModule.javacommon/src/main/java/com/logistics/pipe/modules/CraftingModule.javacommon/src/main/java/com/logistics/pipe/modules/ExtractionModule.javacommon/src/main/java/com/logistics/pipe/modules/InsertionModule.javacommon/src/main/java/com/logistics/pipe/modules/PassiveSupplierModule.javacommon/src/main/java/com/logistics/pipe/modules/PolymorphicSinkModule.javacommon/src/main/java/com/logistics/pipe/modules/ProcessModule.javacommon/src/main/java/com/logistics/pipe/modules/ProviderModule.javacommon/src/main/java/com/logistics/pipe/modules/QuickSortModule.javacommon/src/main/java/com/logistics/pipe/modules/RequesterModule.javacommon/src/main/java/com/logistics/pipe/modules/SupplierModule.javacommon/src/main/java/com/logistics/pipe/network/ItemReservation.javacommon/src/main/java/com/logistics/pipe/network/JobCoordinator.javacommon/src/main/java/com/logistics/pipe/network/MinecraftWorldView.javacommon/src/main/java/com/logistics/pipe/network/NetworkController.javacommon/src/main/java/com/logistics/pipe/network/NetworkJob.javacommon/src/main/java/com/logistics/pipe/network/NetworkSnapshotBuilder.javacommon/src/main/java/com/logistics/pipe/network/PipeNetwork.javacommon/src/main/java/com/logistics/pipe/network/PlanNode.javacommon/src/main/java/com/logistics/pipe/network/PlanningContext.javacommon/src/main/java/com/logistics/pipe/network/ProviderSnapshot.javacommon/src/main/java/com/logistics/pipe/network/RequestPlanner.javacommon/src/main/java/com/logistics/pipe/network/ReservationManager.javacommon/src/main/java/com/logistics/pipe/network/WorkOrder.javacommon/src/main/java/com/logistics/pipe/runtime/PipeRuntime.javacommon/src/main/java/com/logistics/power/engine/block/entity/StirlingEngineBlockEntity.javacommon/src/test/java/com/logistics/pipe/network/JobCoordinatorTest.javacommon/src/test/java/com/logistics/pipe/network/NetworkControllerTest.javacommon/src/test/java/com/logistics/pipe/network/ReconciliationServiceTest.javacommon/src/test/java/com/logistics/pipe/network/RequestPlannerTest.javacommon/src/test/java/com/logistics/pipe/network/ReservationManagerTest.javacommon/src/test/java/com/logistics/pipe/network/SinkResolverTest.javacommon/src/test/java/com/logistics/test/MinecraftTestEnvironment.javacommon/src/test/java/com/logistics/test/TestItemKey.javafabric/src/gametest/java/com/logistics/gametest/automation/KilnGameTest.javafabric/src/gametest/java/com/logistics/gametest/power/EngineGameTest.javafabric/src/main/java/com/logistics/fabric/capability/FabricCapabilityRegistration.javafabric/src/main/java/com/logistics/fabric/capability/ItemStorageAccess.javafabric/src/main/java/com/logistics/fabric/storage/FabricItemKey.javafabric/src/main/java/com/logistics/fabric/storage/FabricItemStorage.javafabric/src/main/java/com/logistics/fabric/storage/FabricItemView.java
✅ Files skipped from review due to trivial changes (20)
- common/src/main/java/com/logistics/core/lib/network/ProviderCanFulfill.java
- common/src/main/java/com/logistics/core/lib/storage/IItemView.java
- common/src/main/java/com/logistics/core/lib/network/RecipeIngredient.java
- common/src/main/java/com/logistics/core/lib/network/IngredientChecker.java
- common/src/main/java/com/logistics/core/lib/network/Order.java
- fabric/src/main/java/com/logistics/fabric/capability/ItemStorageAccess.java
- fabric/src/main/java/com/logistics/fabric/storage/FabricItemView.java
- common/src/main/java/com/logistics/core/lib/network/PlanningView.java
- common/src/main/java/com/logistics/core/lib/storage/IItemKey.java
- common/src/main/java/com/logistics/core/lib/block/capability/HasItemStorage.java
- common/src/main/java/com/logistics/pipe/network/RequestPlanner.java
- fabric/src/main/java/com/logistics/fabric/storage/FabricItemKey.java
- common/src/main/java/com/logistics/core/lib/network/CrafterSnapshot.java
- common/src/test/java/com/logistics/test/TestItemKey.java
- common/src/main/java/com/logistics/core/lib/storage/ItemStorageLookup.java
- common/src/main/java/com/logistics/pipe/modules/RequesterModule.java
- common/src/main/java/com/logistics/core/lib/storage/IItemStorage.java
- common/src/main/java/com/logistics/core/lib/network/ILogisticsNetwork.java
- common/src/main/java/com/logistics/pipe/modules/CraftingModule.java
- common/src/main/java/com/logistics/pipe/modules/ProviderModule.java
🚧 Files skipped from review as they are similar to previous changes (10)
- common/src/main/java/com/logistics/automation/kiln/KilnBlockEntity.java
- common/src/main/java/com/logistics/core/macerator/MaceratorBlockEntity.java
- common/src/main/java/com/logistics/pipe/network/ItemReservation.java
- common/src/main/java/com/logistics/pipe/Pipe.java
- common/src/main/java/com/logistics/pipe/modules/AdvancedExtractorModule.java
- fabric/src/main/java/com/logistics/fabric/capability/FabricCapabilityRegistration.java
- common/src/main/java/com/logistics/core/lib/items/ItemInventoryComponent.java
- common/src/main/java/com/logistics/pipe/block/entity/PipeItemStorage.java
- common/src/main/java/com/logistics/pipe/network/JobCoordinator.java
- common/src/main/java/com/logistics/core/lib/network/ItemRequest.java
…maxStackSize; guard FabricStorageView.extract by resource and view amount
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@fabric/src/main/java/com/logistics/fabric/storage/FabricItemStorage.java`:
- Around line 60-80: The current asFabric() implementation (methods insert,
extract, iterator and the StorageView.extract path) only simulates against the
live IItemStorage and defers real mutations via transaction.addCloseCallback,
which breaks transaction-local visibility; instead introduce a
per-TransactionContext staged delta map (keyed by IItemKey / FabricItemKey) that
all fabric-facing operations consult and update during simulate calls so
multiple operations in the same TransactionContext see each other; on
addCloseCallback (or better, register a SnapshotParticipant/attach to the
TransactionContext) apply the accumulated staged deltas to the real storage only
if the transaction commits; update insert, extract, iterator and
StorageView.extract to read/write the staged view and to record deltas rather
than calling storage.insert/storage.extract immediately, and ensure the staged
map is cleared on rollback.
🪄 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 UI
Review profile: CHILL
Plan: Pro
Run ID: 35fe8b66-9cf1-4361-adb8-a99eeb93b80a
📒 Files selected for processing (3)
common/src/main/java/com/logistics/pipe/modules/QuickSortModule.javacommon/src/main/java/com/logistics/power/engine/block/entity/StirlingEngineBlockEntity.javafabric/src/main/java/com/logistics/fabric/storage/FabricItemStorage.java
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
fabric/src/main/java/com/logistics/fabric/storage/FabricItemStorage.java (1)
137-179:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
StagedStorageis still not transaction-correct for general Fabric callers.The per-
IItemKeydelta map only makes same-resource operations compose. A common swap like “extract cobblestone, then insert dirt into that freed slot” still fails in one transaction because the dirt insert probes live state and never sees the staged cobblestone removal. Also, the close callback applies deltas when the currentTransactionContextcloses, so committing a nested transaction can mutate live storage before an outer transaction decides to roll back. This needs an outer-transaction snapshot /SnapshotParticipant-style model rather than per-key totals.In Fabric Transfer API v1, should nested transaction commits remain rollback-able by an outer transaction, and should a Storage<ItemVariant> implementation expose transaction-local visibility for cross-resource operations like extracting one item and inserting a different item into the freed slot within the same transaction?🤖 Prompt for 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. In `@fabric/src/main/java/com/logistics/fabric/storage/FabricItemStorage.java` around lines 137 - 179, StagedStorage's current per-TransactionContext delta map and close callback apply committed deltas immediately and only per-key totals, so nested commits mutate live storage and cross-resource ops (extract A then insert B) don't see transaction-local frees; fix by switching to a SnapshotParticipant-style transaction-local view: replace the IdentityHashMap staged with a TransactionContext-scoped participant that on first access registers as a snapshot participant (instead of applying directly in deltas(TransactionContext) via addCloseCallback), keep per-transaction maps that on nested commit merge into the parent transaction's map (not into live storage) and only flush to live storage when the outermost transaction commits; update deltas(TransactionContext), insert(..., TransactionContext), and extract(..., TransactionContext) to compute effectiveCapacity/effectiveAvailable by combining the live storage probe (storage.insert/storage.extract with probing=true) with the cumulative staged deltas from the current transaction and all its ancestor transaction maps so cross-resource operations see transaction-local state.
🤖 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 `@fabric/src/main/java/com/logistics/fabric/storage/FabricItemStorage.java`:
- Around line 166-177: The storage probe calls inside
FabricItemStorage.insert(...) and FabricItemStorage.extract(...) use
Integer.MAX_VALUE, which incorrectly caps probes to 2_147_483_647; change those
probe arguments to Long.MAX_VALUE (or the appropriate long max constant) when
calling storage.insert(...) and storage.extract(...), and update the other
occurrences referenced (the similar probe around the later lines) so all
capacity/availability probes use long max values instead of Integer.MAX_VALUE.
---
Duplicate comments:
In `@fabric/src/main/java/com/logistics/fabric/storage/FabricItemStorage.java`:
- Around line 137-179: StagedStorage's current per-TransactionContext delta map
and close callback apply committed deltas immediately and only per-key totals,
so nested commits mutate live storage and cross-resource ops (extract A then
insert B) don't see transaction-local frees; fix by switching to a
SnapshotParticipant-style transaction-local view: replace the IdentityHashMap
staged with a TransactionContext-scoped participant that on first access
registers as a snapshot participant (instead of applying directly in
deltas(TransactionContext) via addCloseCallback), keep per-transaction maps that
on nested commit merge into the parent transaction's map (not into live storage)
and only flush to live storage when the outermost transaction commits; update
deltas(TransactionContext), insert(..., TransactionContext), and extract(...,
TransactionContext) to compute effectiveCapacity/effectiveAvailable by combining
the live storage probe (storage.insert/storage.extract with probing=true) with
the cumulative staged deltas from the current transaction and all its ancestor
transaction maps so cross-resource operations see transaction-local state.
🪄 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 UI
Review profile: CHILL
Plan: Pro
Run ID: 333b2cad-ba04-448e-a19e-80ee5a7ff70d
📒 Files selected for processing (1)
fabric/src/main/java/com/logistics/fabric/storage/FabricItemStorage.java
Owner
Author
|
@coderabbitai resume |
Contributor
|
Tip For best results, initiate chat on the files or code changes.
[resume] |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@fabric/src/main/java/com/logistics/fabric/storage/FabricItemStorage.java`:
- Around line 136-199: StagedStorage currently records only per-key
pendingDeltas which breaks slot-based shared-slot implementations; change the
staging model to track slot-level (or full-inventory) staged state and make all
reads consult that staged snapshot: replace pendingDeltas with a stagedSlots map
or a full staged contents copy created in createSnapshot()/readSnapshot(),
update deltas(TransactionContext) to return that staged view, modify
insert(ItemVariant,...)/extract(...) to compute
effectiveCapacity/effectiveAvailable by applying staged slot changes (not just
per-key delta), ensure iterator() and any StorageView/getCapacity logic return
views derived from the staged snapshot, and keep onFinalCommit() applying staged
slot changes back to storage and then clearing the staged snapshot.
🪄 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 UI
Review profile: CHILL
Plan: Pro
Run ID: eb0099e4-2993-4eb0-908a-2123b343eaa6
📒 Files selected for processing (1)
fabric/src/main/java/com/logistics/fabric/storage/FabricItemStorage.java
This was referenced May 7, 2026
Closed
Indemnity83
added a commit
that referenced
this pull request
May 8, 2026
Indemnity83
added a commit
that referenced
this pull request
May 8, 2026
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.
Summary
Closes #333
Removes all
net.fabricmc.fabric.api.transfer.*imports fromcommon/(item storage only; fluid storage is out of scope) and replaces them with four loader-agnostic interfaces. This is the prerequisite for eventually compilingcommon/under NeoForge's toolchain without Fabric API on the classpath.Changes
New interfaces in
core.lib.storage:IItemKey— loader-agnostic item identity (replacesItemVariant)IItemView— read-only snapshot of a slot (replacesStorageView<ItemVariant>)IItemStorage— insert/extract/contents with simulate-boolean semantics (replacesStorage<ItemVariant>+Transaction)ItemStorageLookup— static registry for findingIItemStoragefrom the world and constructingIItemKeyfromItemStackContainerItemStorage— wraps vanillaContainerasIItemStorage(used by Kiln, Macerator)Fabric adapters in
fabric/storage:FabricItemKey— wrapsItemVariantFabricItemView— wrapsStorageView<ItemVariant>FabricItemStorage— bidirectional adapter:wrap()(Fabric→IItemStorage) andasFabric()(IItemStorage→Fabric Storage)Updated throughout
common/:HasItemStorage.itemStorage()return type:Storage<ItemVariant>→IItemStorageOrder,ItemRequest,RecipeIngredient, etc.) and domain types (ProviderModule,PipeRuntime, etc.) useIItemKeyinstead ofItemVariantPipeRuntime.transferItem()uses aninstanceof PipeBlockEntitycheck for pipe-to-pipe routing to avoid circular wrapping, then falls back toItemStorageLookup.find()for external inventoriesTestItemKey(a minimalIItemKeybacked byItemidentity)Notes
IEnergyStorage/FabricEnergyStoragedesign — noITransactioninterface is neededcommon/(tracked separately)HasFluidStorage,FluidTankComponent) is intentionally unchanged — out of scope for this PR