Repository navigation
perf: synchronize terminal repository client-side - #151
Conversation
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
![swear-review[bot]](https://avatars.githubusercontent.com/u/49574926?s=60&v=4){kind=small}
| var player = ctx.player(); | ||
| if (player != null && player.containerMenu instanceof StorageTerminalMenu menu | ||
| && menu.containerId == packet.containerId()) { | ||
| menu.sendFullRepository(); |
There was a problem hiding this comment.
This handler responds to every TerminalRepositoryResyncPacket by immediately sending a full repository snapshot via menu.sendFullRepository() (which forces sendRepositoryUpdate to serialize the whole repository into chunked packets), with no rate limiting or cooldown. The packet only carries a containerId and the guard here only verifies the player has that menu open, so any client with the terminal open can spam this packet to repeatedly trigger full-repository serialization/transmission, causing avoidable server CPU and network load. Consider rate-limiting resync responses per player (e.g., a minimum interval or in-flight flag) or verifying the client's reported state is actually stale before resending a full snapshot.
medium · security
— Swear Review
| if (player.level().isClientSide() | ||
| || player.isSpectator() | ||
| || !stillValid(player) | ||
| || packet.containerId() != containerId | ||
| || packet.quickMove() | ||
| || !getCarried().isEmpty()) return false; |
There was a problem hiding this comment.
On the CRAFTABLE page, packet.quickMove() is treated as an invalid action and the method returns false without delegating to super.handleRepositoryAction, unlike the STORAGE branch which explicitly forwards quick-move requests to the superclass. The client screen sends quick-move packets for any page where the repository is active (slotClicked sends TerminalRepositoryActionPacket for both PICKUP and QUICK_MOVE), so shift+clicking a craftable item in the repository will be silently ignored. If quick-move is intentionally disabled on the craftable page (to keep click-as-select semantics), make that explicit; otherwise shift+click extraction is silently broken compared to the STORAGE page.
medium · bug
— Swear Review
| match.selectionOutputKey(level, output).orElse(null); | ||
| if (key == null || output.isEmpty() || !getResourceView().matches(key) | ||
| || !matchesCraftableFilter(key, output, query, level)) continue; | ||
| if (key == null || output.isEmpty()) continue; |
There was a problem hiding this comment.
The craftable candidate loops previously filtered recipes with !getResourceView().matches(key) and !matchesCraftableFilter(...) before invoking the expensive computeCraftableStatus preview simulation (and recordNextCraftableThreshold). Those early-exit guards were removed from all three loops, and filtering is now deferred entirely to visibleCraftableDisplayStacks. As a result, the full status simulation runs for every recipe candidate even when an active search filter would have excluded most of them. On large recipe catalogs this can noticeably increase craftable-refresh latency. Consider retaining an inexpensive pre-filter (e.g., matching by the output key/name only) while still keeping the unfiltered displayStacks list for the repository, or documenting the trade-off if the extra cost is acceptable.
medium · performance
— Swear Review
| totalItemTypes = displayStacks.size(); | ||
| List<ItemStack> visibleStacks = visibleCraftableDisplayStacks( | ||
| displayStacks, core.getLevel()); | ||
| totalItemTypes = visibleStacks.size(); |
There was a problem hiding this comment.
totalItemTypes is still used as the scroll-range basis on the server side (getMaxScrollOffset/rowAlignedScrollOffset and the tests in CraftingTests/TerminalFlowTests). Changing it to the filtered visible count while setRepositoryStacks stores the unfiltered list means the server-side scrollbar range now only reflects the filtered subset, whereas the client repository (totalEntries()) reflects the unfiltered list. When a search filter is active these two scroll ranges can disagree, which can cause the scroll offset clamp on the server to differ from the client's expectations. If the scroll clamping should still be based on the full item count, compute totalItemTypes from displayStacks and track the filtered count separately.
low · maintainability
— Swear Review
| int index = scrollOffset + visibleIndex; | ||
| if (visibleIndex < 0 || visibleIndex >= visibleRows * StorageTerminalMenu.DISPLAY_COLS | ||
| || index >= visibleEntries().size()) return -1; | ||
| return visibleEntries().get(index).serial(); |
There was a problem hiding this comment.
index = scrollOffset + visibleIndex uses int arithmetic. maxScrollOffset() can legally return values up to the largest 9-aligned int (near Integer.MAX_VALUE), so adding a small visibleIndex can overflow to a negative number. The subsequent guard only checks index >= visibleEntries().size(), which a negative index always passes, so visibleEntries().get(index) would throw IndexOutOfBoundsException. Use long arithmetic (and cast the guard comparison) so the bounds check also rejects overflowed values.
| int index = scrollOffset + visibleIndex; | |
| if (visibleIndex < 0 || visibleIndex >= visibleRows * StorageTerminalMenu.DISPLAY_COLS | |
| || index >= visibleEntries().size()) return -1; | |
| return visibleEntries().get(index).serial(); | |
| long index = (long) scrollOffset + visibleIndex; | |
| if (visibleIndex < 0 || visibleIndex >= (long) visibleRows * StorageTerminalMenu.DISPLAY_COLS | |
| || index >= visibleEntries().size()) return -1; | |
| return visibleEntries().get((int) index).serial(); |
low · bug
— Swear Review
| int entryCount = buf.readVarInt(); | ||
| if (entryCount < 0 || entryCount > MAX_ENTRIES_PER_PACKET) { | ||
| throw new IllegalArgumentException("Too many terminal repository entries"); | ||
| } |
There was a problem hiding this comment.
The combined check entryCount < 0 || entryCount > MAX_ENTRIES_PER_PACKET always throws "Too many terminal repository entries", but buf.readVarInt() can legitimately decode to a negative value for a malformed/oversized packet. In that case the error message is misleading and makes debugging harder. Consider separating the two cases so a negative count reports a distinct message.
| int entryCount = buf.readVarInt(); | |
| if (entryCount < 0 || entryCount > MAX_ENTRIES_PER_PACKET) { | |
| throw new IllegalArgumentException("Too many terminal repository entries"); | |
| } | |
| int entryCount = buf.readVarInt(); | |
| if (entryCount < 0) { | |
| throw new IllegalArgumentException("Negative terminal repository entry count"); | |
| } | |
| if (entryCount > MAX_ENTRIES_PER_PACKET) { | |
| throw new IllegalArgumentException("Too many terminal repository entries"); | |
| } |
low · maintainability
— Swear Review
Swear ReviewMode: Full PR Findings:
0 findings total Status: ✅ Completed |
| int chunkIndex = buf.readVarInt(); | ||
| int chunkCount = buf.readVarInt(); |
There was a problem hiding this comment.
chunkIndex/chunkCount are read here but only validated later inside the compact constructor, i.e., after up to MAX_ENTRIES_PER_PACKET entries have already been decoded and allocated. For a network-facing decoder, validate the chunk header immediately after reading it so a malformed packet (e.g., negative chunkIndex or chunkCount <= 0) is rejected before any entry parsing work is done.
| int chunkIndex = buf.readVarInt(); | |
| int chunkCount = buf.readVarInt(); | |
| int chunkIndex = buf.readVarInt(); | |
| int chunkCount = buf.readVarInt(); | |
| if (chunkCount <= 0 || chunkIndex < 0 || chunkIndex >= chunkCount) { | |
| throw new IllegalArgumentException("Invalid terminal repository chunk"); | |
| } |
low · other
— Swear Review
| entries = List.copyOf(entries); | ||
| if (entries.size() > MAX_ENTRIES_PER_PACKET) { | ||
| throw new IllegalArgumentException("Too many terminal repository entries"); | ||
| } |
There was a problem hiding this comment.
List.copyOf(entries) will throw an opaque NullPointerException if entries is null, and it runs before the size-cap check, so an oversized list is fully copied before being discarded. Validate null and size first, then make the defensive copy.
| entries = List.copyOf(entries); | |
| if (entries.size() > MAX_ENTRIES_PER_PACKET) { | |
| throw new IllegalArgumentException("Too many terminal repository entries"); | |
| } | |
| if (entries == null) { | |
| throw new IllegalArgumentException("Missing terminal repository entries"); | |
| } | |
| if (entries.size() > MAX_ENTRIES_PER_PACKET) { | |
| throw new IllegalArgumentException("Too many terminal repository entries"); | |
| } | |
| entries = List.copyOf(entries); |
low · maintainability
— Swear Review
| @Override | ||
| public boolean mayPickup(Player player) { | ||
| return delegate.mayPickup(player); | ||
| } |
There was a problem hiding this comment.
This wrapper presents a read-only remote slot, yet it delegates the "interactive" behaviors to the delegate slot: mayPickup() returns delegate.mayPickup(player) (the delegate GhostSlot.mayPickup() always returns true), and allowModification()/onTake()/onQuickCraft() are forwarded to the delegate. Combined with the no-op set() and remove() (which returns ItemStack.EMPTY), the slot advertises itself as pickable/modifiable but silently drops any mutation attempt. The screen only intercepts PICKUP/QUICK_MOVE click types; other paths such as THROW (pressing Q over a slot), SWAP (number keys 1-9) and QUICK_CRAFT (drag) still route through the vanilla clicked() flow on the client, which calls remove()/set() on this slot. The result is inconsistent client behavior (e.g., Q-throw does nothing because remove returns EMPTY while the item still appears present, drag operations accumulate empty stacks) and a risk of client/server desync. Recommendation: make the wrapper fully consistent with a read-only slot — override mayPickup to return false, and make onTake/onQuickCraft no-ops (or remove the delegate forwarding) — so that every interaction goes through the packet-based action handler rather than the vanilla mutation path.
| @Override | |
| public boolean mayPickup(Player player) { | |
| return delegate.mayPickup(player); | |
| } | |
| @Override | |
| public boolean mayPickup(Player player) { | |
| return false; | |
| } |
medium · bug
— Swear Review
| continue; | ||
| } | ||
| next.put(key, display.copy()); | ||
| if (old == null || !ItemStack.isSameItemSameComponents(old, display)) { |
There was a problem hiding this comment.
ItemStack.isSameItemSameComponents() compares item type and components but intentionally ignores the stack count. However, in this codebase the display stacks for a resource are created with setCount(1) and carry their real amounts in the auto_storage:terminal_display_amount custom-data tag (see TerminalDisplayStack.create/amount, and StorageCoreBlockEntity.displayFor). That tag is a component, so isSameItemSameComponents correctly detects an amount-only change (e.g. 1 -> 64). So this is not a count bug. However, there IS a real issue here for non-item views: index() falls back to StorageResourceKey.item(TerminalDisplayStack.strip(display), registries) only when TerminalResourceDisplay.key(display) is absent. strip() removes the amount tags but NOT the auto_storage:terminal_resource tag, so typed displays keep their real key. For plain ITEM view displays, stripping also removes the amount tag and the item key remains unchanged — which is correct since serial is keyed to the resource, not the amount. No count-related defect exists in these two comparisons.
low · other
— Swear Review
| import java.util.Map; | ||
|
|
||
| final class TerminalRepositoryServer { | ||
| private final Map<StorageResourceKey, Long> serials = new HashMap<>(); |
There was a problem hiding this comment.
serials is populated via serials.computeIfAbsent(key, ignored -> nextSerial++) and entries are never removed. However, this map is only used to assign each distinct resource key a stable serial, and the class is created per-menu-instance (new TerminalRepositoryServer() in StorageTerminalMenu). As long as the menu/container stays open, keys for resources that leave the terminal remain mapped, but the terminal's distinct resource key space is bounded by the storage core's type capacity; the serials map cannot grow beyond the total number of distinct keys ever seen. This is bounded per open container and is released when the menu closes, so it is not an unbounded memory leak in practice. Still, a removal path (or bounding the map to keys present in previous) would be a worthwhile hardening.
low · other
— Swear Review
| if (indexed.put(key, display.copy()) != null) { | ||
| throw new IllegalStateException("Duplicate terminal repository resource " + key); | ||
| } |
There was a problem hiding this comment.
index() throws IllegalStateException on a duplicate key. The current callers pass core.getTerminalDisplayStacks(...) (per-key aggregated) or buildCraftableDisplayStacks output (built from a LinkedHashMap keyed by StorageResourceKey, so unique). The fallback key derivation via StorageResourceKey.item(strip(display), registries) also maps a display to the same key for identical resources. So duplicates should not occur from existing callers, and the fail-fast guard is reasonable defensive validation. No change strictly required.
low · other
— Swear Review
| List<TerminalRepositoryUpdatePacket> updateChanges( | ||
| int containerId, | ||
| Map<StorageResourceKey, ItemStack> changed, | ||
| java.util.Set<StorageResourceKey> removed | ||
| ) { |
There was a problem hiding this comment.
updateChanges() does not consult fullPending, so incremental packets can be emitted while a full snapshot is pending. At the actual call sites (sendRepositoryUpdate in StorageTerminalMenu), when repositoryFullRequested is true and repositoryFullListPending is false, updateChanges is deliberately called first to flush pending deltas and then fullSnapshot is sent in the same tick, so the client always receives the full snapshot afterward. Given the current call sequence, no state corruption occurs. If future callers invoke updateChanges directly between requestFull() and update(), this ordering guarantee could be lost, so it is worth documenting.
low · other
— Swear Review
| private long nextSerial = 1; | ||
| private long revision; | ||
| private boolean fullPending = true; |
There was a problem hiding this comment.
This class is instantiated once per menu (final TerminalRepositoryServer repositoryServer = new TerminalRepositoryServer(); in StorageTerminalMenu) and all callers (sendRepositoryUpdate, repositoryKey, repositoryDisplay, handleRepositoryAction, etc.) execute on the Minecraft server thread. The repositoryServer is never accessed from a client thread or async worker. Within the server tick, menu callbacks are serialized, so the check-then-act on fullPending and the nextSerial++ increment are effectively single-threaded. No thread-safety defect can be demonstrated from the call sites; the object is confined to the server thread.
low · other
— Swear Review
| Map<StorageResourceKey, ItemStack> copy = new LinkedHashMap<>(); | ||
| source.forEach((key, stack) -> copy.put(key, stack.copy())); | ||
| return Map.copyOf(copy); |
There was a problem hiding this comment.
copy() builds a LinkedHashMap then wraps it with Map.copyOf(), which does not preserve iteration order. However, previous is only used for lookups and diffing in changedEntries/updateChanges, and the client re-sorts entries in visibleEntries() (see TerminalClientRepository), so ordering of the server-side map has no user-visible effect. Map.copyOf also benefits from iteration stability. This is a non-issue in practice.
low · other
— Swear Review
| StorageResourceKey keyFor(long serial) { | ||
| for (Map.Entry<StorageResourceKey, Long> entry : serials.entrySet()) { | ||
| if (entry.getValue() == serial) return entry.getKey(); | ||
| } | ||
| return null; | ||
| } |
There was a problem hiding this comment.
keyFor() linearly scans serials on every call. It is invoked from repositoryKey(serial) / repositoryDisplay(serial), which are only called when a client action packet (click/transfer) is handled, i.e. infrequently per player interaction, not per display frame. With a terminal's typical distinct-resource counts this O(n) scan is negligible. A reverse serial->key map would be a micro-optimization; not required.
low · performance
— Swear Review
| self.assertNotIn("while (delta < 0)", screen) | ||
| self.assertNotIn("while (delta >= 9)", screen) | ||
|
|
||
| def test_terminal_scroll_uses_client_repository_without_server_packet(self): |
There was a problem hiding this comment.
This new test method largely duplicates the assertions already covered by test_terminal_scrollbar_uses_client_repository_without_server_scroll_packets (e.g., TerminalRepositoryUpdatePacket in the entrypoint, and the negative TerminalScrollPacket checks). The only marginally unique assertion is TerminalClientRepository in the menu source. Keeping two near-identical scroll-regression tests doubles maintenance burden and produces confusing duplicate failures if the client-repository contract regresses. Consider removing this method or merging its unique assertion into the comprehensive test above.
low · maintainability
— Swear Review
| buf.writeVarInt(packet.expectedView().ordinal()); | ||
| buf.writeVarInt(packet.direction().ordinal()); |
There was a problem hiding this comment.
The wire format serializes the enums by their ordinal() (and decodes back via values()[id] in requireById/byId). This is internally consistent today, but it makes the network protocol silently change if any constant is reordered or inserted in the middle of TerminalResourceView or TerminalContainerTransferDirection in a future update — old and new clients/servers would interpret the same bytes as different views/directions without any encode/decode error, potentially causing wrong container transfers. Since this is a playToServer packet, consider either giving the enums explicit stable wire IDs (and mapping them in the codec) or at minimum ensuring any enum reordering forces a protocol version bump.
low · maintainability
— Swear Review
| if (containerId < 0 || stateId < 0 || serial <= 0) { | ||
| throw new IllegalArgumentException("Invalid terminal repository transfer"); | ||
| } |
There was a problem hiding this comment.
This constructor validation is stricter than its only producer. In StorageTerminalScreen.mouseClicked, a WITHDRAW packet is sent for an empty carried stack when button == 0 on a non-item view (the !menu.getCarried().isEmpty() guard only applies to button == 1, i.e. DEPOSIT). The server rejects that packet in handleRepositoryContainerTransfer (withdraw path returns false), so it is not a functional bug, but the sender still allows an obviously invalid direction/hold combination that the receiver must defensively ignore. If a client were ever allowed to send this with an empty carry, the mismatch between the client-side construction (no validation) and the server-side assumption (getCarried().isEmpty() is rejected) could lead to a silent no-op rather than a meaningful error. Consider validating on the client side too, or documenting why an empty-carry WITHDRAW is intentionally sent.
low · bug
— Swear Review
| long serial() { | ||
| return repository.serialAt(visibleIndex, visibleRows); | ||
| } |
There was a problem hiding this comment.
serial() is dead code: a codebase-wide search for \.serial() (and for TerminalRepositorySlot) shows this method is never invoked. The only call site (StorageTerminalScreen) accesses repository.serialAt(visibleIndex, visibleRows) directly in both slotClicked and mouseClicked, bypassing this wrapper. Consider removing the unused method or having the screen use it for consistency.
low · maintainability
— Swear Review
| int chunkCount, | ||
| List<TerminalRepositoryEntry> entries | ||
| ) implements CustomPacketPayload { | ||
| static final int MAX_ENTRIES_PER_PACKET = 512; |
There was a problem hiding this comment.
The 512-entry cap bounds chunking by entry count only, not by serialized byte size. Each entry can carry a full ItemStack (including its components/NBT) plus an optional resource key with a variant CompoundTag, so a chunk of 512 large/complex entries can exceed Minecraft's maximum network packet size (2^21 bytes). When that happens the connection is forcibly closed and the terminal repository sync fails. Consider bounding chunks by estimated encoded size (e.g., in TerminalRepositoryServer.chunk) rather than relying solely on the fixed entry-count cap, or at least document the size assumption behind this constant.
medium · bug
— Swear Review
| TerminalContainerTransferDirection direction = | ||
| TerminalContainerTransferDirection.byWireId(buf.readVarInt()); | ||
| if (direction == null) { | ||
| throw new IllegalArgumentException("Unknown terminal container transfer direction"); | ||
| } |
There was a problem hiding this comment.
Deserialization error handling is inconsistent between the two enum lookups: TerminalResourceView.requireWireId throws IllegalArgumentException for unknown wire IDs, whereas TerminalContainerTransferDirection.byWireId returns null and is explicitly null-checked here. Since the compact constructor's Objects.requireNonNull(direction) would throw a generic NullPointerException for an invalid direction, the explicit check here is somewhat redundant; conversely, relying on requireWireId's exception is fine. For consistency and clearer failures, consider having byWireId throw for unknown IDs (or document the contract), or drop the redundant null check and let the constructor reject it uniformly.
low · maintainability
— Swear Review
| RegistryFriendlyByteBuf buf = new RegistryFriendlyByteBuf( | ||
| Unpooled.buffer(), registries); | ||
| try { | ||
| writeEntry(buf, entry); | ||
| return buf.readableBytes(); | ||
| } finally { | ||
| buf.release(); | ||
| } |
There was a problem hiding this comment.
encodedEntrySize is called once per entry inside the chunking loop in TerminalRepositoryServer.chunk(), and each invocation allocates and releases a fresh Unpooled.buffer()/RegistryFriendlyByteBuf just to measure the serialized size of a single entry. For a large repository snapshot (hundreds/thousands of entries), this creates avoidable buffer allocation and GC pressure on the server thread. Consider passing a single reusable scratch buffer into the loop (e.g., buf.clear() before each entry) or computing the size without a full serialization pass.
| RegistryFriendlyByteBuf buf = new RegistryFriendlyByteBuf( | |
| Unpooled.buffer(), registries); | |
| try { | |
| writeEntry(buf, entry); | |
| return buf.readableBytes(); | |
| } finally { | |
| buf.release(); | |
| } | |
| RegistryFriendlyByteBuf buf = new RegistryFriendlyByteBuf( | |
| Unpooled.buffer(), registries); | |
| try { | |
| writeEntry(buf, entry); | |
| return buf.readableBytes(); | |
| } finally { | |
| buf.release(); | |
| } |
low · performance
— Swear Review
| TerminalClientRepository repository = menu.clientRepository(); | ||
| long serial = hoveredSlot instanceof TerminalRepositorySlot repositorySlot | ||
| ? repositorySlot.serial() | ||
| : repository == null | ||
| ? -1 : repository.serialAt(hoveredSlot.index, visibleRows); | ||
| if (serial <= 0) return true; | ||
| minecraft.getConnection().send(new TerminalRepositoryContainerTransferPacket( |
There was a problem hiding this comment.
In the held-container transfer branch of mouseClicked(), the serial is computed as -1 whenever the client repository is null, and the if (serial <= 0) return true immediately consumes the click. This silently disables held-container transfers whenever the repository isn't available (e.g., before the first repository sync, after a failed sync that triggered recovery, or during non-item pages). The old behavior always sent TerminalHeldContainerTransferPacket. Since the legacy packet and its server handler are still registered (AutoStorage/StorageTerminalMenu), the code should fall back to sending the legacy packet when the repository is unavailable instead of dropping the click.
high · bug
— Swear Review
| long serial = hoveredSlot instanceof TerminalRepositorySlot repositorySlot | ||
| ? repositorySlot.serial() | ||
| : repository == null | ||
| ? -1 : repository.serialAt(hoveredSlot.index, visibleRows); |
There was a problem hiding this comment.
In mouseClicked(), the fallback repository.serialAt(hoveredSlot.index, visibleRows) passes the full menu slot index. TerminalRepositorySlot instances are only installed for menuIndex < DISPLAY_SLOTS (replaceSlot), so when hoveredSlot is not a TerminalRepositorySlot, hoveredSlot.index can be >= DISPLAY_SLOTS (player inventory) and thus outside the visible grid. serialAt expects a 0-based visible index and guards on visibleIndex >= visibleRows * DISPLAY_COLS; using hoveredSlot.index produces the wrong entry or -1, so the wrong container transfer (or none) is triggered for a hovered slot that isn't one of the replaced repository slots. Since the guard at the top of this branch already limits hoveredSlot.index to the visible grid, the non-repository-slot fallback should be removed (a matching TerminalRepositorySlot is guaranteed within that range), or the visible index should be derived explicitly.
medium · bug
— Swear Review
| preferences.sortMode(), | ||
| preferences.sortOrder(), | ||
| preferences.resourceView()); | ||
| repository.setScrollOffset(repository.getScrollOffset(visibleRows), visibleRows); |
There was a problem hiding this comment.
updateClientRepositoryView() calls repository.setScrollOffset(repository.getScrollOffset(visibleRows), visibleRows), which reads the current (already clamped/aligned) offset and writes it back unchanged. This is a no-op that obscures intent; drop the call or add a comment explaining what re-clamping is intended to achieve.
low · maintainability
— Swear Review
| repository.setView( | ||
| searchBox == null ? "" : searchBox.getValue(), | ||
| preferences.sortMode(), | ||
| preferences.sortOrder(), | ||
| preferences.resourceView()); |
There was a problem hiding this comment.
updateClientRepositoryView() is invoked from init(), renderBg() (every frame while the item view is active) and containerTick() (every tick), and it always calls repository.setView(...). Inside setView, when the search text is non-empty, the dirty flag is set and the full entry list is re-filtered/re-sorted on the next visibleEntries() access, which is then triggered by maxScrollOffset()/totalEntries() in the same render frame. Since the search string only changes when the user types, repeated renders with the same text keep the cached visible list (setView early-returns when nothing changed). The only repeated cost is the cheap string equality check. No performance problem is evident here; no comment issued.
low · other
— Swear Review
| @Override | ||
| public ItemStack remove(int amount) { | ||
| return ItemStack.EMPTY; | ||
| } |
There was a problem hiding this comment.
All terminal slot interactions are routed to the server via TerminalRepositoryActionPacket / TerminalRepositoryContainerTransferPacket, and the server-side StorageTerminalMenu.clicked() early-returns for display-slot indices so remove() on the client slot is never a valid withdrawal path. The no-op remove()/set()/mayPickup() design is consistent with the new architecture, so no issue here.
low · other
— Swear Review
| @Override | ||
| public int getMaxStackSize() { | ||
| return delegate.getMaxStackSize(); | ||
| } |
There was a problem hiding this comment.
getMaxStackSize()/getMaxStackSize(ItemStack) forward to the delegate slot, which for these repository slots is a GhostSlot bound to displayInventory (whose container returns Integer.MAX_VALUE from getMaxStackSize). As a result every repository slot reports Integer.MAX_VALUE regardless of the item it actually displays (e.g. 1 for a non-stackable item). The delegate here is just a positional placeholder, not the slot holding the displayed item, so the value is semantically wrong and can mislead vanilla/mod logic that derives transfer amounts or drag-split math from slot.getMaxStackSize(). Consider deriving it from the displayed item, e.g. getItem().getMaxStackSize().
| @Override | |
| public int getMaxStackSize() { | |
| return delegate.getMaxStackSize(); | |
| } | |
| @Override | |
| public int getMaxStackSize() { | |
| return getItem().getMaxStackSize(); | |
| } |
medium · bug
— Swear Review
| @Override | ||
| public boolean isFake() { | ||
| return delegate.isFake(); | ||
| } |
There was a problem hiding this comment.
isFake() forwards to delegate.isFake(). The delegate is a GhostSlot over displayInventory, which is a real (non-fake) slot, so this virtual slot reports isFake() == false even though its displayed item does not actually live in any container (it is a pure repository projection). Client logic and mod integrations that use isFake() to decide whether a slot is safe to interact with (creative clone, pickup simulation, etc.) will treat it as a real inventory slot. Since this slot intentionally rejects all direct mutations, it should report true here.
| @Override | |
| public boolean isFake() { | |
| return delegate.isFake(); | |
| } | |
| @Override | |
| public boolean isFake() { | |
| return true; | |
| } |
low · bug
— Swear Review
| ITEM(0), | ||
| FLUID(1), | ||
| ENERGY(2), | ||
| GAS(3), | ||
| STATION_WORK(4), | ||
| OTHER(5), | ||
| ALL(6); |
There was a problem hiding this comment.
The explicit wireId values exactly duplicate the enum ordinals, but only the new transfer packets (TerminalHeldContainerTransferPacket/TerminalRepositoryContainerTransferPacket) use wireId. All other wire encodings still rely on ordinal order: StorageTerminalMenu's data-slot sync uses resourceView.ordinal()/byId(v), and TerminalPreferences.write/read serializes resourceView.ordinal(). If the enum constants are ever reordered (e.g. to change the UI cycle order), the wireId-based packets would decode to a different view than the ordinal-based data-slot/preferences sync, causing the server's packet.expectedView() != resourceView guard to reject legitimate transfers silently. Consider migrating all wire encodings to wireId, or add a SelfTest assertion that wireId() == ordinal() (or that the values match the intended wire order) so the two schemes cannot diverge unnoticed.
low · maintainability
— Swear Review
| private void setScrollTarget(int target) { | ||
| TerminalClientRepository repository = menu.clientRepository(); | ||
| if (repository == null) return; | ||
| repository.setScrollOffset(target, visibleRows); | ||
| } |
There was a problem hiding this comment.
Legacy (non-repository) scrolling is broken. The old sendScrollTarget(...) sent a TerminalScrollPacket to the server, but it has been replaced by setScrollTarget(...), which only calls repository.setScrollOffset(...) and returns immediately when menu.clientRepository() is null (line 1074). All callers (scrollbar press at line 889, drag at 927, wheel step at 943, and containerTick at 993) now route through setScrollTarget, so in the null-repository fallback path there is no way to send the new scroll position to the server. Since the server menu still owns scrollOffset (synchronized via data slots), scrollbar clicks/drags/wheel scrolling will be completely non-functional on the legacy path. Consider sending the TerminalScrollPacket (or an equivalent scroll message) to the server when the repository is null.
high · bug
— Swear Review
| long serial = slot instanceof TerminalRepositorySlot repositorySlot | ||
| ? repositorySlot.serial() | ||
| : repository.serialAt(slotId, visibleRows); | ||
| if (serial > 0 && minecraft != null && minecraft.getConnection() != null) { | ||
| minecraft.getConnection().send(new TerminalRepositoryActionPacket( | ||
| menu.containerId, | ||
| serial, | ||
| mouseButton, | ||
| clickType == ClickType.QUICK_MOVE)); | ||
| } | ||
| return; |
There was a problem hiding this comment.
This new override swallows clicks on all display slots (0 <= slotId < visibleRows * DISPLAY_COLS) for PICKUP/QUICK_MOVE in repository mode. However, repositionSlots()/replaceSlot(...) converts every slot in the grid (menuIndex < DISPLAY_SLOTS, i.e. up to 18 rows) into a TerminalRepositorySlot, while menu.clicked(...) still intercepts clicks in the display-slot range. When the slot is a TerminalRepositorySlot whose serial resolves to -1 (e.g. empty row due to row-aligned scrolling, or no entry at that index), the click is dropped (line 831-842). Additionally, the first block only forwards serial-based actions; because TerminalRepositorySlot.set/remove/mayPlace are all no-ops returning empty/false, double-click pickup (ClickType.PICKUP with button 0 on an already picked-up stack) and other non-PICKUP/QUICK_MOVE flows are no longer possible in repository mode. Note also that the non-item resource branch in mouseClicked returns true when hoveredSlot is not a TerminalRepositorySlot, silently consuming the click; with the full grid replaced by repository slots, this affects every display slot in non-item views, not just the currently visible rows. Please confirm clicks on empty/off-grid display slots are intentionally dropped and that the server-side clicked(...) handler (which reads slot.getItem() from a client-populated slot) cannot be reached in this mode.
medium · bug
— Swear Review
| () -> menuIndex < visibleRows * StorageTerminalMenu.DISPLAY_COLS | ||
| && delegate.isActive()); |
There was a problem hiding this comment.
The isActive() predicate captures visibleRows by value at slot-creation time. Slots are only (re)built in init() -> repositionSlots(), while visibleRows is also mutated server-side via applySettings(...) (in StorageTerminalMenu) with no client-side slot rebuild. If the window is resized or the row count otherwise changes without re-running init()/repositionSlots(), slots created with the old visibleRows will keep the stale predicate, so either the active range is mis-computed or clicks beyond the current visible grid are routed. Prefer referencing the current field (e.g. menuIndex < this.visibleRows * DISPLAY_COLS) or re-deriving it from the geometry at render time.
medium · bug
— Swear Review
| private void sendSearchPacket() { | ||
| if (menu.clientRepository() != null) return; |
There was a problem hiding this comment.
sendSearchPacket() now returns immediately when menu.clientRepository() != null. In repository mode the search is handled entirely client-side, but the search string also flows into updateClientRepositoryView() -> repository.setView(...) only during containerTick/render. Since updateClientRepositoryView() is not invoked from the scheduleSearch/sendSearchPacket path (only the debounce timer calls sendSearchPacket, which early-returns), the new search text is not applied to the repository until the next frame. That is functionally fine for rendering, but the bigger issue is that updateClientRepositoryView() also resets scrollOffset = 0 in setView whenever the filter/sort/view changes — combined with the per-frame calls this means any transient preference difference resets user scroll position. Please confirm the search-filter change is intended to reset scroll on every tick while typing (as opposed to only when the filter actually stabilizes).
medium · bug
— Swear Review
| updateClientRepositoryView(); | ||
| TerminalClientRepository repository = menu.clientRepository(); |
There was a problem hiding this comment.
updateClientRepositoryView() is invoked from three places: init(), renderBg() (every frame), and containerTick() (every tick). Although TerminalClientRepository.setView(...) deduplicates unchanged state internally and the recompute only runs when dirty, the per-frame call still needs to re-fetch and compare all view parameters and re-evaluate visibleRows-dependent scroll calculations on every frame. Since setView also resets scrollOffset on any change, any subtle drift in the passed preferences (e.g. a terminal preference object that is re-created per frame) will repeatedly reset the scroll position. Consider gating the per-frame call behind an explicit dirty flag (set by search responder, button changes, etc.) rather than calling it unconditionally in renderBg.
low · maintainability
— Swear Review
| if (buf.readerIndex() - packetStart > MAX_SERIALIZED_BYTES) { | ||
| throw new IllegalArgumentException( | ||
| "Terminal repository packet exceeds serialized size limit"); | ||
| } |
There was a problem hiding this comment.
The serialized size check is only performed after every entry has been fully deserialized. Since this packet is registered via playToClient, decode() runs on the client and a malicious or buggy server can send up to MAX_ENTRIES_PER_PACKET entries with large NBT CompoundTags and ItemStacks; the client will allocate all of those objects before this limit is ever enforced, defeating the purpose of the safety check. Enforce the limit incrementally inside the entry loop (e.g., re-check buf.readerIndex() - packetStart > MAX_SERIALIZED_BYTES after each entry, and also guard each individual entry's NBT/ItemStack allocation) so oversized payloads are rejected before significant memory is allocated.
medium · bug
— Swear Review
Summary
Verification
./gradlew build --no-daemon./gradlew runGameTestServer --no-daemon./gradlew runRecipeAddonGameTestServer --no-daemon./gradlew runCompatFixtureGameTestServers --no-daemon./gradlew runPneumaticCraftGameTestServer --no-daemon./gradlew runCompatibilityMatrixGameTestServer --no-daemon./gradlew runCompatibilityMatrixGameTestServer -PterminalScaleTypes=30000 --no-daemonPYTHONDONTWRITEBYTECODE=1 python3 -m unittest discover scripts./gradlew runData --no-daemon