Protocol hardening: 26.1 client compatibility on 26.2 servers#1
Merged
Protocol hardening: 26.1 client compatibility on 26.2 servers#1
Conversation
Expand remapping coverage for advancements, chunk/block/entity metadata, and force legacy-friendly status responses so older clients receive consistent protocol-safe payloads. Made-with: Cursor
Align legacy entity_type wire ids after omitting sulfur_cube from registry sync (ClientboundAddEntityPacketMixin + LegacyEntityTypeWireRemapper). Fix villager vs zombie_villager metadata (VillagerEntityData2661). Add structured item rewriting (ItemRewriter, ItemComponentSanitizer), attribute wire table and bundle-safe encoding. Debug tracing (PacketMap, spawn, outbound capture). Verified with full e2e (join + stability). Made-with: Cursor
Align the legacy blockstate max to 26.1.2 and parse chunk sections from actual packet bytes so dimension-specific section counts don't corrupt palette decoding. Made-with: Cursor
Use 30207 as the inclusive blockstate cap so both 26.1.1 and newer 26.1.x clients reject no out-of-range chunk palette ids during join. Made-with: Cursor
26.2 inserts sulfur + cinnabar items into the built-in item registry, shifting numeric IDs of all subsequent vanilla items. A 26.1 client decodes the server's 26.2 IDs against its own built-in registry and gets the wrong item (e.g. diamond_pickaxe → chainmail_boots). - Add LegacyItemIdTable: builds a static 26.2→26.1 item wire-ID translation array at startup by iterating BuiltInRegistries.ITEM, skipping 26.2-only entries, and assigning sequential legacy IDs. - Add ItemStackOptionalCodecMixin: wraps ItemStack.OPTIONAL_STREAM_CODEC to emit the translated 26.1 item ID varint for legacy connections. - Add all cinnabar block/item IDs to LegacyLinkConstants alongside existing sulfur entries for consistent handling everywhere. Made-with: Cursor
|
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 a runtime context and per-thread scoped outbound encoding, implements comprehensive 26.2→26.1 outbound translations (items, blocks, chunks, entities, metadata, attributes, advancements), installs mixin-based wire/codec interceptors, and introduces gated tracing/capture and telemetry for legacy-client packet handling. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Server as "Server Thread"
participant Netty as "Netty Channel\n(Pipeline)"
participant Connection as "net.minecraft.network.Connection"
participant ConnMixin as "ConnectionSendMixin"
participant LegacyHandler as "LegacyPacketHandler"
participant EncodingCtx as "LegacyOutboundEncoding\n(thread-local)"
participant PacketEncoder as "PacketEncoder (TAIL)"
participant WirePatcher as "LegacyUpdateAttributesWirePatcher"
rect rgba(200,200,255,0.5)
Note over Connection,LegacyHandler: Outbound flow for legacy recipient
end
Server->>Netty: write(packet)
Netty->>Connection: send(packet)
Connection->>ConnMixin: injected legacylink$rewriteLegacyClientboundPacket
ConnMixin->>LegacyHandler: route if legacy (flatten bundles, prefetch, remap)
LegacyHandler->>EncodingCtx: enterScoped(connection)
LegacyHandler->>LegacyHandler: remap items/blocks/chunks/entities/metadata/registries
LegacyHandler->>EncodingCtx: close() (exit scope)
LegacyHandler->>Netty: write(translatedPacket)
Netty->>PacketEncoder: encode(translatedPacket)
PacketEncoder->>WirePatcher: if update_attributes -> rewriteBuffer(output)
PacketEncoder->>Netty: emit bytes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (14)
src/main/java/dev/ohno/legacylink/encoding/LegacyOutboundEncoding.java (1)
16-22: Prefer a scoped API to prevent ThreadLocal leaks on exceptional paths.Current
enter()/leave()pairing is manual; a try-with-resources guard reduces misuse risk.Proposed refactor
public final class LegacyOutboundEncoding { @@ public static void enter(`@Nullable` Connection connection) { CURRENT.set(connection); } + public static Scope enterScope(`@Nullable` Connection connection) { + CURRENT.set(connection); + return Scope.INSTANCE; + } + public static void leave() { CURRENT.remove(); } @@ public static `@Nullable` Connection connection() { return CURRENT.get(); } + + public static final class Scope implements AutoCloseable { + private static final Scope INSTANCE = new Scope(); + private Scope() {} + `@Override` + public void close() { + leave(); + } + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/dev/ohno/legacylink/encoding/LegacyOutboundEncoding.java` around lines 16 - 22, The manual enter()/leave() pairing on the ThreadLocal CURRENT can leak if exceptions bypass leave(); change the API to provide a scoped AutoCloseable guard (e.g., a nested class LegacyOutboundEncoding.Scope or Guard that sets CURRENT in its constructor and clears it in close()), deprecate or make enter()/leave() private, and update call sites to use try-with-resources (try (LegacyOutboundEncoding.Scope scope = LegacyOutboundEncoding.enterScoped(connection)) { ... }) so CURRENT is always cleared even on exceptions; keep method names like enterScoped/Scope/close to locate the change.src/main/java/dev/ohno/legacylink/LegacyLinkMod.java (1)
36-38: Consider fully resettingLegacyRuntimeContexton shutdown.Only clearing
serverleavesregistryAccess/chunkContainerFactory/sectionCountas stale state; adding a full reset makes shutdown behavior cleaner and safer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/dev/ohno/legacylink/LegacyLinkMod.java` around lines 36 - 38, The shutdown currently only calls LegacyRuntimeContext.bindServer(null) leaving registryAccess, chunkContainerFactory and sectionCount stale; update the shutdown handler to fully reset the runtime context by either calling a new LegacyRuntimeContext.reset() method or explicitly clearing the remaining fields (e.g., set registryAccess to null, set chunkContainerFactory to null, and reset sectionCount to 0) after TranslationStats.dump(); locate the shutdown lambda where ServerLifecycleEvents.SERVER_STOPPING.register(...) is used and ensure the added reset/clearing is invoked there so the context is fully cleared on server stop.src/main/java/dev/ohno/legacylink/handler/rewrite/AdvancementRewriter.java (1)
75-81: Skip packet rebuild when nothing changed.You can return the original packet when
remappedIcons == 0to avoid unnecessary allocations in advancement sync bursts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/dev/ohno/legacylink/handler/rewrite/AdvancementRewriter.java` around lines 75 - 81, In AdvancementRewriter, avoid rebuilding the ClientboundUpdateAdvancementsPacket when no icons were remapped: check the remappedIcons counter after processing and if remappedIcons == 0 return the original packet instance (the incoming ClientboundUpdateAdvancementsPacket) instead of constructing a new one; update the method that currently returns a new ClientboundUpdateAdvancementsPacket to short-circuit and return the original packet when no changes were made.src/main/java/dev/ohno/legacylink/mapping/LegacyAttributeWireTable.java (1)
38-42: Publish rebuilt tables atomically.Line 39 replaces the global map before population is complete. If
rebuild()is ever invoked after startup, readers can observe partial state. Build into local collections, then swap globals and setready=trueas the final publish step.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/dev/ohno/legacylink/mapping/LegacyAttributeWireTable.java` around lines 38 - 42, The rebuild() method currently replaces serverHolderIdToLegacyHolderId and clears LEGACY_SYNCED_IDS before they're fully populated, exposing readers to a partial state; fix by creating local instances (e.g., a new Int2IntOpenHashMap localServerMap and a local collection for legacy IDs), populate those locals fully inside rebuild(), then atomically swap them into the globals serverHolderIdToLegacyHolderId and LEGACY_SYNCED_IDS, and finally set the ready flag to true as the last step; ensure any readers consult the ready flag (or use volatile references) so they never observe partially-built tables during rebuild().src/main/java/dev/ohno/legacylink/debug/LegacyOutboundPacketCapture.java (2)
50-53: Cache the enabled flag to avoid repeated system property lookups.
enabled()is called on every packet send (perlogIfLegacy). Reading a system property each time adds overhead. Cache the result at class load or use a lazy-init pattern.♻️ Proposed fix
+ private static final boolean ENABLED; + static { + String v = System.getProperty("legacylink.captureOutbound", ""); + ENABLED = "true".equalsIgnoreCase(v) || "1".equals(v); + } + public static boolean enabled() { - String v = System.getProperty("legacylink.captureOutbound", ""); - return "true".equalsIgnoreCase(v) || "1".equals(v); + return ENABLED; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/dev/ohno/legacylink/debug/LegacyOutboundPacketCapture.java` around lines 50 - 53, The enabled() method currently reads System.getProperty on every call (used by logIfLegacy), causing needless overhead; change it to read once and cache the boolean (e.g. a static final or a lazily-initialized volatile flag) so subsequent calls return the cached value; update the method enabled() to return that cached field and ensure initialization respects the same semantics ("true" case-insensitive or "1") used today so behavior doesn't change.
64-72: Unnecessary list copy — iterate directly over subPackets().The
subPackets()already returns an iterable; copying to anArrayListjust to get indexed access adds allocation overhead.♻️ Proposed simplification
if (packet instanceof ClientboundBundlePacket bundle) { - List<Packet<? super ClientGamePacketListener>> subs = new ArrayList<>(); - for (Packet<? super ClientGamePacketListener> s : bundle.subPackets()) { - subs.add(s); - } + var subs = bundle.subPackets(); + int count = 0; + for (var ignored : subs) count++; emit(connection, stage, path, Identifier.withDefaultNamespace("bundle"), "ClientboundBundlePacket", - "subPackets=" + subs.size()); - for (int i = 0; i < subs.size(); i++) { - logRecursive(connection, subs.get(i), stage, path + "bundle[" + i + "]."); + "subPackets=" + count); + int i = 0; + for (Packet<? super ClientGamePacketListener> sub : subs) { + logRecursive(connection, sub, stage, path + "bundle[" + i++ + "]."); } return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/dev/ohno/legacylink/debug/LegacyOutboundPacketCapture.java` around lines 64 - 72, Replace the unnecessary ArrayList copy of bundle.subPackets() with direct iteration: first loop over bundle.subPackets() just to count elements (use an int counter) and pass that count to emit(..., "ClientboundBundlePacket", "subPackets=" + count), then a second loop over bundle.subPackets() with an int index (i++) to call logRecursive(connection, packet, stage, path + "bundle[" + i + "]."); remove the List/ArrayList usage and the subs variable while keeping the emit and logRecursive calls as shown.src/main/java/dev/ohno/legacylink/handler/LegacyChunkTranslator.java (1)
223-233: Reflection per block entity — cache the Field lookup.
entry.getClass().getDeclaredField("type")runs for every block entity in every chunk. Cache the field similar toCHUNK_BUFFER_FIELDfor better performance.♻️ Proposed fix — cache block entity type field
+ private static volatile Field blockEntityInfoTypeField; private static int filterChunkBlockEntities(ClientboundLevelChunkPacketData chunkData) throws IllegalAccessException { // ... blockEntities.removeIf(entry -> { try { - Field typeField = entry.getClass().getDeclaredField("type"); - typeField.setAccessible(true); + Field typeField = blockEntityInfoTypeField; + if (typeField == null) { + typeField = entry.getClass().getDeclaredField("type"); + typeField.setAccessible(true); + blockEntityInfoTypeField = typeField; + } BlockEntityType<?> type = (BlockEntityType<?>) typeField.get(entry);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/dev/ohno/legacylink/handler/LegacyChunkTranslator.java` around lines 223 - 233, The block entity filter in LegacyChunkTranslator calls entry.getClass().getDeclaredField("type") for every entry which is expensive; cache the Field per class (e.g. a ConcurrentHashMap<Class<?>, Field>) and reuse it inside the blockEntities.removeIf lambda instead of repeated reflective lookups, mirroring how CHUNK_BUFFER_FIELD is cached; ensure you setAccessible(true) once when populating the cache and handle ReflectiveOperationException when populating the cache, rethrowing or logging as appropriate so the removeIf lambda only retrieves the cached Field and reads the type field value.src/main/java/dev/ohno/legacylink/mapping/LegacyItemIdTable.java (1)
84-91: Redundant substring checks after explicit set membership.
LegacyLinkConstants.SULFUR_ITEM_IDSalready contains allcinnabarandsulfuritems. The substring checks at lines 89-90 are defensive but redundant. If the constant set is kept in sync, these can be removed; otherwise, consider consolidating detection logic.♻️ Simplified detection
private static boolean is26_2Only(Item item) { Identifier key = BuiltInRegistries.ITEM.getKey(item); if (key == null) return true; String id = key.toString(); - return LegacyLinkConstants.SULFUR_ITEM_IDS.contains(id) - || id.contains("cinnabar") - || id.contains("sulfur"); + return LegacyLinkConstants.SULFUR_ITEM_IDS.contains(id); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/dev/ohno/legacylink/mapping/LegacyItemIdTable.java` around lines 84 - 91, The method is26_2Only currently checks both membership in LegacyLinkConstants.SULFUR_ITEM_IDS and substring matches for "cinnabar"/"sulfur" which is redundant; update is26_2Only to rely solely on LegacyLinkConstants.SULFUR_ITEM_IDS.contains(id) (after keeping the constant set authoritative and up-to-date) or, if you prefer a consolidated detection, replace both checks with a single helper like LegacyLinkConstants.isSulfurItem(id) and use that from is26_2Only; ensure you only reference the Identifier key via BuiltInRegistries.ITEM.getKey(item) and return true when key is null as before.src/main/java/dev/ohno/legacylink/debug/PositionPacketTrace.java (2)
117-143: Duplicate reflection helpers — consider extracting to shared utility.
moveEntityIdandrotateHeadEntityIdare nearly identical to those inLegacyOutboundPacketCapture. Extract to a shared utility class (e.g.,PacketReflectionUtil) to reduce duplication.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/dev/ohno/legacylink/debug/PositionPacketTrace.java` around lines 117 - 143, Extract the duplicated reflection logic from moveEntityId and rotateHeadEntityId into a shared utility (e.g., PacketReflectionUtil) and update both PositionPacketTrace and LegacyOutboundPacketCapture to call it; implement a reusable method in PacketReflectionUtil like getIntField(Object packet, Class<?> packetClass, String fieldName, AtomicReference<Field> cachedField) (or similar) that caches the Field, calls setAccessible once, and returns the int value or -1 on ReflectiveOperationException, then replace moveEntityId and rotateHeadEntityId bodies to delegate to that utility using the appropriate packet classes (ClientboundMoveEntityPacket, ClientboundRotateHeadPacket) and the shared cached Field references.
105-111: MissingreturnafterClientboundRotateHeadPackethandling.Unlike other packet handlers that explicitly
return, this block falls through to the method end. While functionally correct, it's inconsistent with the other branches.♻️ Add explicit return for consistency
if (packet instanceof ClientboundRotateHeadPacket p) { LegacyLinkMod.LOGGER.info( "{} type=rotate_head entityId={} yHeadRot={}", base, rotateHeadEntityId(p), p.getYHeadRot() ); + return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/dev/ohno/legacylink/debug/PositionPacketTrace.java` around lines 105 - 111, In PositionPacketTrace::trace (the packet inspection method) the branch handling ClientboundRotateHeadPacket logs via LegacyLinkMod.LOGGER.info with rotateHeadEntityId(p) and p.getYHeadRot() but does not return like the other packet branches; add an explicit return immediately after that LOGGER.info call so the method exits after handling ClientboundRotateHeadPacket to maintain consistency with the other branches and avoid fall-through.src/main/java/dev/ohno/legacylink/handler/LegacyPacketHandler.java (2)
713-726: Use per-entry flag to avoid unnecessarysetValuecalls.The
changedflag is cumulative across all map entries. Once it becomestruefor any entry, all subsequent entries will haveentry.setValue(rewritten)called even if their IDs didn't change. While not a correctness bug (the rewritten list contains identical values), this is wasteful and the logic is confusing.♻️ Proposed fix
+ boolean anyChanged = false; for (Map.Entry<Identifier, IntList> entry : tagMap.entrySet()) { IntList ids = entry.getValue(); IntArrayList rewritten = new IntArrayList(ids.size()); + boolean entryChanged = false; for (int i = 0; i < ids.size(); i++) { int oldId = ids.getInt(i); int mappedId = ItemRewriter.remapItemIdStrict(oldId); rewritten.add(mappedId); - changed |= mappedId != oldId; + entryChanged |= mappedId != oldId; } - if (changed) { + if (entryChanged) { entry.setValue(rewritten); + anyChanged = true; } } - if (changed) { + if (anyChanged) { LegacyLinkMod.LOGGER.debug("[LegacyLink] Remapped item tag payload IDs for legacy client"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/dev/ohno/legacylink/handler/LegacyPacketHandler.java` around lines 713 - 726, The cumulative boolean "changed" leaks across iterations causing entry.setValue(rewritten) to be called for later entries even if their IDs didn't change; change to use a per-entry flag (e.g. boolean changedForEntry) inside the loop over tagMap entries, set it when mappedId != oldId while building rewritten, and only call entry.setValue(rewritten) when changedForEntry is true; update references inside the loop for ids, rewritten, ItemRewriter.remapItemIdStrict, and entry.setValue accordingly so the check is local to each map entry.
554-565: Minor: Prefer!sb.isEmpty()oversb.length() > 0.Modern Java style prefers
StringBuilder.isEmpty()for clarity.♻️ Proposed fix
for (var v : items) { max = Math.max(max, v.id()); - if (sb.length() > 0) { + if (!sb.isEmpty()) { sb.append(','); } sb.append(v.id()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/dev/ohno/legacylink/handler/LegacyPacketHandler.java` around lines 554 - 565, Replace the StringBuilder length check in LegacyPacketHandler (the block that builds the comma-separated ids into sb) by using sb.isEmpty() for clarity: change the condition from "if (sb.length() > 0)" to "if (!sb.isEmpty())" so the code reads more idiomatically while still preserving the existing logic that prefixes commas when sb already contains content.src/main/java/dev/ohno/legacylink/debug/LegacyPacketMapTrace.java (2)
43-46: Cache the system property result to avoid repeated lookups.
enabled()is called on every packet write path. Reading system properties on each call introduces unnecessary overhead. Consider caching the value at class initialization.♻️ Proposed fix
public final class LegacyPacketMapTrace { + private static final boolean ENABLED; + + static { + String v = System.getProperty("legacylink.tracePacketMap", ""); + ENABLED = "true".equalsIgnoreCase(v) || "1".equals(v); + } + private static final AtomicLong SEQ = new AtomicLong(); private static final ThreadLocal<Long> ACTIVE_SEQ = new ThreadLocal<>(); private LegacyPacketMapTrace() {} public static boolean enabled() { - String v = System.getProperty("legacylink.tracePacketMap", ""); - return "true".equalsIgnoreCase(v) || "1".equals(v); + return ENABLED; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/dev/ohno/legacylink/debug/LegacyPacketMapTrace.java` around lines 43 - 46, The enabled() method in LegacyPacketMapTrace repeatedly reads the system property "legacylink.tracePacketMap" on every call; change this to read once at class initialization by adding a private static final boolean (e.g., ENABLED) computed from System.getProperty(...) and modify the enabled() method to simply return that field, preserving the same semantics for "true"/"1" and defaulting to false when unset.
148-173: Avoid double iteration over bundle sub-packets.The method iterates
bundle.subPackets()twice: once to count (lines 153-156) and once to process (lines 158-169). This is inefficient, especially sincesubPackets()may not be a simple list accessor.♻️ Proposed fix - single pass
private static String formatPacketTree(Packet<?> packet, String path) { if (packet instanceof ClientboundBundlePacket bundle) { StringBuilder sb = new StringBuilder(); sb.append("Bundle path=").append(path.isEmpty() ? "/" : path); - int subCount = 0; - for (Packet<? super ClientGamePacketListener> ignored : bundle.subPackets()) { - subCount++; - } - sb.append(" subCount=").append(subCount); int i = 0; for (Packet<? super ClientGamePacketListener> sub : bundle.subPackets()) { if (sub instanceof BundleDelimiterPacket) { i++; continue; } if (sub instanceof ClientboundBundlePacket nested) { sb.append(" {").append(formatPacketTree(nested, path + "/" + i)).append("}"); } else if (isInteresting(sub)) { sb.append(" {").append(formatPacketLeaf(sub)).append("}"); } i++; } + sb.insert(sb.indexOf("Bundle path=") + "Bundle path=".length() + (path.isEmpty() ? 1 : path.length()), " subCount=" + i); return sb.toString(); } return formatPacketLeaf(packet); }Alternatively, append
subCountat the end after the loop completes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/dev/ohno/legacylink/debug/LegacyPacketMapTrace.java` around lines 148 - 173, The code double-iterates bundle.subPackets(); remove the first loop and compute subCount in the single processing loop inside formatPacketTree(Packet<?> packet, String path): initialize int subCount = 0 before the for-loop over bundle.subPackets(), increment subCount once per iteration (including when encountering BundleDelimiterPacket), keep the existing i index behavior for path building, and move the sb.append(" subCount=" + subCount) after the loop so the same single pass both counts and processes sub-packets; references: formatPacketTree, ClientboundBundlePacket, subPackets(), BundleDelimiterPacket, isInteresting, formatPacketLeaf.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Line 24: Update the stale README line about item stack remapping to reflect
that outbound inventory/container flows now perform active remapping and
sanitization; edit the sentence that currently reads "Item stack remapping in
inventory packets is not yet fully implemented" to state that
remapping/sanitization is implemented for outbound inventory/container flows
(and optionally note any remaining limitations or supported directions), so
operators aren't misled about current behavior—locate and modify the existing
sentence in README.md accordingly.
In `@src/main/java/dev/ohno/legacylink/debug/CameraPacketTrace.java`:
- Around line 42-52: The readCameraEntityId method should cache the reflective
Field lookup and broaden exception handling to include runtime access failures
(e.g., InaccessibleObjectException) like the existing
LegacyOutboundPacketCapture.cameraEntityId() does: add a static (volatile)
cached Field for ClientboundSetCameraPacket, perform the lookup once
(synchronized init or double-checked locking) by trying "cameraId" then "id",
call setAccessible(true) inside a try that catches ReflectiveOperationException
and RuntimeException, store null on failure, and then in readCameraEntityId use
the cached Field (returning -1 if null) and call field.getInt(packet) inside a
try that catches both ReflectiveOperationException and RuntimeException to avoid
uncaught module-access errors.
In `@src/main/java/dev/ohno/legacylink/handler/LegacyChunkTranslator.java`:
- Around line 113-120: FriendlyByteBuf allocated with Unpooled.buffer() is never
released causing a Netty buffer leak; wrap the allocation and use of
FriendlyByteBuf in a try/finally, call ReferenceCountUtil.release(...) on the
underlying ByteBuf (e.g. ReferenceCountUtil.release((io.netty.buffer.ByteBuf)
out.unwrap())) in the finally block after extracting bytes, while keeping the
logic that calls writeSectionWithContext(...) and sets CHUNK_BUFFER_FIELD on
chunkData unchanged.
In `@src/main/java/dev/ohno/legacylink/handler/LegacyPacketHandler.java`:
- Around line 710-712: The reflection lookup for
TagNetworkSerialization.NetworkPayload.tags should be cached instead of done
per-call; add a private static final Field TAGS_NETWORK_PAYLOAD_TAGS_FIELD and
initialize it in the class static initializer using
TagNetworkSerialization.NetworkPayload.class.getDeclaredField("tags") and
setAccessible(true) (handle or rethrow checked exceptions as a runtime failure),
then replace the local tagsField usage in remapUpdateRecipes and here with
TAGS_NETWORK_PAYLOAD_TAGS_FIELD and cast
TAGS_NETWORK_PAYLOAD_TAGS_FIELD.get(payload) to Map<Identifier, IntList>.
- Around line 667-670: The reflection lookups inside remapUpdateRecipes
(RecipePropertySet.class.getDeclaredField("items") and
RecipePropertySet.class.getDeclaredConstructor(Set.class)) should be cached as
static fields similar to UPDATE_ATTRIBUTES_REBUILD_CTOR to avoid repeated
overhead; add static final Field RECIPE_ITEMS_FIELD and static final
Constructor<RecipePropertySet> RECIPE_PROP_SET_CTOR initialized in the class
static initializer (setAccessible(true) there), then replace the local
recipeItemsField and ctor usages in remapUpdateRecipes to use these static
fields.
In
`@src/main/java/dev/ohno/legacylink/handler/rewrite/BlockStatePacketRewriter.java`:
- Around line 41-66: remapSectionBlocksUpdate currently mutates the packet's
internal BlockState[] (accessed via SECTION_STATES_FIELD) causing
shared-state/race issues; instead, read the original states array, allocate a
new BlockState[] copy, remap into that new array using
Block.BLOCK_STATE_REGISTRY and RegistryRemapper.remapBlockState, and if any
remaps occurred create and return a new ClientboundSectionBlocksUpdatePacket
instance (or clone the original and set the new array via SECTION_STATES_FIELD
on the new instance) so the original packet's array is never modified; preserve
the existing IllegalAccessException handling and still call
TranslationStats.recordSectionBlocksRemap(remapped) when remaps > 0.
In
`@src/main/java/dev/ohno/legacylink/handler/rewrite/VillagerEntityData2661.java`:
- Around line 100-118: In
stripVillagerOnlyTailSlots(List<SynchedEntityData.DataValue<?>> packedItems, int
booleanSerializerId) avoid unconditionally removing the boolean at
VILLAGER_FINALIZED_26_2 (slot 21); first detect strong villager evidence by
scanning packedItems for a VILLAGER_DATA_26_2 entry whose value() is a
VillagerData instance, and only allow removal of the finalized boolean when that
VillagerData presence is true; leave other entities' metadata intact by skipping
the boolean-stripping branch when no VillagerData is found. Use the existing
symbols stripVillagerOnlyTailSlots, VILLAGER_DATA_26_2, VILLAGER_FINALIZED_26_2,
booleanSerializerId and packedItems to locate and implement the guard.
In `@src/main/java/dev/ohno/legacylink/mapping/LegacyEntityTypeWireRemapper.java`:
- Around line 44-52: The method legacyNetworkId currently falls back to
BuiltInRegistries.ENTITY_TYPE.getId(type) when the key is null or not found in
legacySyncOrder, which accidentally emits modern IDs for filtered entities;
update legacyNetworkId to stop returning the modern registry id in those
branches and instead raise an explicit error (e.g., throw an
IllegalStateException) that includes the EntityType and Identifier/key so the
mismatch is surfaced, and add a brief note in the exception message that
filtered entities must be mapped to a legacy substitute before calling
legacyNetworkId; change both the key == null branch and the idx < 0 branch
accordingly (or alternatively return a documented sentinel value instead of the
modern id if the codebase prefers).
In `@src/main/java/dev/ohno/legacylink/runtime/LegacyRuntimeContext.java`:
- Around line 45-51: registryAccess() and chunkContainerFactory() can return
null before initialize() is called; change both accessors to validate the
backing fields (registryAccess and chunkContainerFactory) and throw an
informative IllegalStateException if they are null (e.g., "LegacyRuntimeContext
not initialized - call initialize() first"), so callers fail fast instead of
producing hidden NPEs; update the methods registryAccess() and
chunkContainerFactory() to perform this null check and throw when uninitialized.
- Around line 31-42: findEntity(int) accesses ServerLevel entity state
off-thread and must be invoked on the server thread; update callers
(LegacyPacketHandler.write, VillagerEntityData2661,
Vanilla261EntityMetadataTailTrim2661, CubeMobEntityData2661) to marshal entity
lookups to the server thread using MinecraftServer.execute(...) (or change
findEntity to accept a callback or return a Future and perform the lookup inside
MinecraftServer.execute), then resume packet handling with the result on
completion; ensure you locate usages of findEntity and replace direct calls with
server.execute lambdas that perform the same level iteration and then pass the
found Entity back to the calling context.
---
Nitpick comments:
In `@src/main/java/dev/ohno/legacylink/debug/LegacyOutboundPacketCapture.java`:
- Around line 50-53: The enabled() method currently reads System.getProperty on
every call (used by logIfLegacy), causing needless overhead; change it to read
once and cache the boolean (e.g. a static final or a lazily-initialized volatile
flag) so subsequent calls return the cached value; update the method enabled()
to return that cached field and ensure initialization respects the same
semantics ("true" case-insensitive or "1") used today so behavior doesn't
change.
- Around line 64-72: Replace the unnecessary ArrayList copy of
bundle.subPackets() with direct iteration: first loop over bundle.subPackets()
just to count elements (use an int counter) and pass that count to emit(...,
"ClientboundBundlePacket", "subPackets=" + count), then a second loop over
bundle.subPackets() with an int index (i++) to call logRecursive(connection,
packet, stage, path + "bundle[" + i + "]."); remove the List/ArrayList usage and
the subs variable while keeping the emit and logRecursive calls as shown.
In `@src/main/java/dev/ohno/legacylink/debug/LegacyPacketMapTrace.java`:
- Around line 43-46: The enabled() method in LegacyPacketMapTrace repeatedly
reads the system property "legacylink.tracePacketMap" on every call; change this
to read once at class initialization by adding a private static final boolean
(e.g., ENABLED) computed from System.getProperty(...) and modify the enabled()
method to simply return that field, preserving the same semantics for "true"/"1"
and defaulting to false when unset.
- Around line 148-173: The code double-iterates bundle.subPackets(); remove the
first loop and compute subCount in the single processing loop inside
formatPacketTree(Packet<?> packet, String path): initialize int subCount = 0
before the for-loop over bundle.subPackets(), increment subCount once per
iteration (including when encountering BundleDelimiterPacket), keep the existing
i index behavior for path building, and move the sb.append(" subCount=" +
subCount) after the loop so the same single pass both counts and processes
sub-packets; references: formatPacketTree, ClientboundBundlePacket,
subPackets(), BundleDelimiterPacket, isInteresting, formatPacketLeaf.
In `@src/main/java/dev/ohno/legacylink/debug/PositionPacketTrace.java`:
- Around line 117-143: Extract the duplicated reflection logic from moveEntityId
and rotateHeadEntityId into a shared utility (e.g., PacketReflectionUtil) and
update both PositionPacketTrace and LegacyOutboundPacketCapture to call it;
implement a reusable method in PacketReflectionUtil like getIntField(Object
packet, Class<?> packetClass, String fieldName, AtomicReference<Field>
cachedField) (or similar) that caches the Field, calls setAccessible once, and
returns the int value or -1 on ReflectiveOperationException, then replace
moveEntityId and rotateHeadEntityId bodies to delegate to that utility using the
appropriate packet classes (ClientboundMoveEntityPacket,
ClientboundRotateHeadPacket) and the shared cached Field references.
- Around line 105-111: In PositionPacketTrace::trace (the packet inspection
method) the branch handling ClientboundRotateHeadPacket logs via
LegacyLinkMod.LOGGER.info with rotateHeadEntityId(p) and p.getYHeadRot() but
does not return like the other packet branches; add an explicit return
immediately after that LOGGER.info call so the method exits after handling
ClientboundRotateHeadPacket to maintain consistency with the other branches and
avoid fall-through.
In `@src/main/java/dev/ohno/legacylink/encoding/LegacyOutboundEncoding.java`:
- Around line 16-22: The manual enter()/leave() pairing on the ThreadLocal
CURRENT can leak if exceptions bypass leave(); change the API to provide a
scoped AutoCloseable guard (e.g., a nested class LegacyOutboundEncoding.Scope or
Guard that sets CURRENT in its constructor and clears it in close()), deprecate
or make enter()/leave() private, and update call sites to use try-with-resources
(try (LegacyOutboundEncoding.Scope scope =
LegacyOutboundEncoding.enterScoped(connection)) { ... }) so CURRENT is always
cleared even on exceptions; keep method names like enterScoped/Scope/close to
locate the change.
In `@src/main/java/dev/ohno/legacylink/handler/LegacyChunkTranslator.java`:
- Around line 223-233: The block entity filter in LegacyChunkTranslator calls
entry.getClass().getDeclaredField("type") for every entry which is expensive;
cache the Field per class (e.g. a ConcurrentHashMap<Class<?>, Field>) and reuse
it inside the blockEntities.removeIf lambda instead of repeated reflective
lookups, mirroring how CHUNK_BUFFER_FIELD is cached; ensure you
setAccessible(true) once when populating the cache and handle
ReflectiveOperationException when populating the cache, rethrowing or logging as
appropriate so the removeIf lambda only retrieves the cached Field and reads the
type field value.
In `@src/main/java/dev/ohno/legacylink/handler/LegacyPacketHandler.java`:
- Around line 713-726: The cumulative boolean "changed" leaks across iterations
causing entry.setValue(rewritten) to be called for later entries even if their
IDs didn't change; change to use a per-entry flag (e.g. boolean changedForEntry)
inside the loop over tagMap entries, set it when mappedId != oldId while
building rewritten, and only call entry.setValue(rewritten) when changedForEntry
is true; update references inside the loop for ids, rewritten,
ItemRewriter.remapItemIdStrict, and entry.setValue accordingly so the check is
local to each map entry.
- Around line 554-565: Replace the StringBuilder length check in
LegacyPacketHandler (the block that builds the comma-separated ids into sb) by
using sb.isEmpty() for clarity: change the condition from "if (sb.length() > 0)"
to "if (!sb.isEmpty())" so the code reads more idiomatically while still
preserving the existing logic that prefixes commas when sb already contains
content.
In `@src/main/java/dev/ohno/legacylink/handler/rewrite/AdvancementRewriter.java`:
- Around line 75-81: In AdvancementRewriter, avoid rebuilding the
ClientboundUpdateAdvancementsPacket when no icons were remapped: check the
remappedIcons counter after processing and if remappedIcons == 0 return the
original packet instance (the incoming ClientboundUpdateAdvancementsPacket)
instead of constructing a new one; update the method that currently returns a
new ClientboundUpdateAdvancementsPacket to short-circuit and return the original
packet when no changes were made.
In `@src/main/java/dev/ohno/legacylink/LegacyLinkMod.java`:
- Around line 36-38: The shutdown currently only calls
LegacyRuntimeContext.bindServer(null) leaving registryAccess,
chunkContainerFactory and sectionCount stale; update the shutdown handler to
fully reset the runtime context by either calling a new
LegacyRuntimeContext.reset() method or explicitly clearing the remaining fields
(e.g., set registryAccess to null, set chunkContainerFactory to null, and reset
sectionCount to 0) after TranslationStats.dump(); locate the shutdown lambda
where ServerLifecycleEvents.SERVER_STOPPING.register(...) is used and ensure the
added reset/clearing is invoked there so the context is fully cleared on server
stop.
In `@src/main/java/dev/ohno/legacylink/mapping/LegacyAttributeWireTable.java`:
- Around line 38-42: The rebuild() method currently replaces
serverHolderIdToLegacyHolderId and clears LEGACY_SYNCED_IDS before they're fully
populated, exposing readers to a partial state; fix by creating local instances
(e.g., a new Int2IntOpenHashMap localServerMap and a local collection for legacy
IDs), populate those locals fully inside rebuild(), then atomically swap them
into the globals serverHolderIdToLegacyHolderId and LEGACY_SYNCED_IDS, and
finally set the ready flag to true as the last step; ensure any readers consult
the ready flag (or use volatile references) so they never observe
partially-built tables during rebuild().
In `@src/main/java/dev/ohno/legacylink/mapping/LegacyItemIdTable.java`:
- Around line 84-91: The method is26_2Only currently checks both membership in
LegacyLinkConstants.SULFUR_ITEM_IDS and substring matches for
"cinnabar"/"sulfur" which is redundant; update is26_2Only to rely solely on
LegacyLinkConstants.SULFUR_ITEM_IDS.contains(id) (after keeping the constant set
authoritative and up-to-date) or, if you prefer a consolidated detection,
replace both checks with a single helper like
LegacyLinkConstants.isSulfurItem(id) and use that from is26_2Only; ensure you
only reference the Identifier key via BuiltInRegistries.ITEM.getKey(item) and
return true when key is null as before.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: c4f1aadf-9cc2-4fd0-9ad4-ffb0c4e0086a
📒 Files selected for processing (35)
README.mdsrc/main/java/dev/ohno/legacylink/LegacyLinkConstants.javasrc/main/java/dev/ohno/legacylink/LegacyLinkMod.javasrc/main/java/dev/ohno/legacylink/config/LegacyLinkConfig.javasrc/main/java/dev/ohno/legacylink/debug/CameraPacketTrace.javasrc/main/java/dev/ohno/legacylink/debug/EntityDataRewriteTrace.javasrc/main/java/dev/ohno/legacylink/debug/LegacyOutboundPacketCapture.javasrc/main/java/dev/ohno/legacylink/debug/LegacyPacketMapTrace.javasrc/main/java/dev/ohno/legacylink/debug/PositionPacketTrace.javasrc/main/java/dev/ohno/legacylink/debug/SpawnPacketTrace.javasrc/main/java/dev/ohno/legacylink/encoding/LegacyOutboundEncoding.javasrc/main/java/dev/ohno/legacylink/handler/LegacyChunkTranslator.javasrc/main/java/dev/ohno/legacylink/handler/LegacyPacketHandler.javasrc/main/java/dev/ohno/legacylink/handler/rewrite/AdvancementRewriter.javasrc/main/java/dev/ohno/legacylink/handler/rewrite/BlockStatePacketRewriter.javasrc/main/java/dev/ohno/legacylink/handler/rewrite/CubeMobEntityData2661.javasrc/main/java/dev/ohno/legacylink/handler/rewrite/ItemComponentSanitizer.javasrc/main/java/dev/ohno/legacylink/handler/rewrite/ItemRewriter.javasrc/main/java/dev/ohno/legacylink/handler/rewrite/Vanilla261EntityMetadataTailTrim2661.javasrc/main/java/dev/ohno/legacylink/handler/rewrite/VillagerEntityData2661.javasrc/main/java/dev/ohno/legacylink/mapping/LegacyAttributeWireTable.javasrc/main/java/dev/ohno/legacylink/mapping/LegacyEntityTypeWireRemapper.javasrc/main/java/dev/ohno/legacylink/mapping/LegacyItemIdTable.javasrc/main/java/dev/ohno/legacylink/mapping/RegistryRemapper.javasrc/main/java/dev/ohno/legacylink/mixin/ClientboundAddEntityPacketMixin.javasrc/main/java/dev/ohno/legacylink/mixin/ConfigurationFinishMixin.javasrc/main/java/dev/ohno/legacylink/mixin/ConnectionSendMixin.javasrc/main/java/dev/ohno/legacylink/mixin/ItemStackOptionalCodecMixin.javasrc/main/java/dev/ohno/legacylink/mixin/PacketEncoderMixin.javasrc/main/java/dev/ohno/legacylink/mixin/ServerCommonConnectionAccessor.javasrc/main/java/dev/ohno/legacylink/mixin/StatusPacketMixin.javasrc/main/java/dev/ohno/legacylink/protocol/LegacyUpdateAttributesWirePatcher.javasrc/main/java/dev/ohno/legacylink/runtime/LegacyRuntimeContext.javasrc/main/java/dev/ohno/legacylink/telemetry/TranslationStats.javasrc/main/resources/legacylink.mixins.json
💤 Files with no reviewable changes (1)
- src/main/java/dev/ohno/legacylink/config/LegacyLinkConfig.java
…ookups - README: document outbound item/container remapping and wire ID translation. - CameraPacketTrace: cache SetCamera field with synchronized init; catch runtime access errors. - LegacyChunkTranslator: release FriendlyByteBuf backing buffer in finally; cache block-entity type Field per class. - LegacyPacketHandler: cache TagNetworkPayload.tags and RecipePropertySet reflection; fix per-tag entry mutation in remapUpdateTags; try-with-resources LegacyOutboundEncoding.Scope; use StringBuilder.isEmpty(). - BlockStatePacketRewriter: clone section update packets via STREAM_CODEC before remapping states (no shared-array mutation). - VillagerEntityData2661: strip finalized boolean at slot 21 only when VillagerData is present at slot 20. - LegacyEntityTypeWireRemapper: throw if entity type is missing from legacy sync order (no silent modern id). - LegacyRuntimeContext: fail-fast accessors when uninitialized; reset() clears chunk context on stop; findEntity marshals to server thread when needed. - AdvancementRewriter: return original packet when no icons were remapped. - LegacyAttributeWireTable: rebuild into locals then swap maps/sets; ready last. - LegacyItemIdTable / LegacyLinkConstants: is26_2OnlyItemId() backed by authoritative SULFUR_ITEM_IDS set. - Debug: cache trace flags; single-pass bundle formatting; PacketReflectionUtil for move/rotate entity ids; PositionPacketTrace return after rotate_head. - LegacyOutboundEncoding: enterScoped AutoCloseable Scope (always clears ThreadLocal). Made-with: Cursor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/dev/ohno/legacylink/handler/LegacyPacketHandler.java`:
- Around line 715-738: The code mutates the reflected tag map inside the
original ClientboundUpdateTagsPacket payload via
TAGS_NETWORK_PAYLOAD_TAGS_FIELD, which can leak legacy rewrites to other
recipients; instead, create a new payload/map and populate it with remapped IDs
(using ItemRewriter.remapItemIdStrict for each id) and set that new payload on a
freshly constructed packet (or return a new packet instance) rather than calling
entry.setValue(...) on the reflected map; update the method in
LegacyPacketHandler to build and return this new packet/payload when any changes
are needed and leave the original packet unmodified.
- Around line 761-771: The non-sulfur branch in prefetchAddEntityMetadataHints
sets clientVisibleEntityTypeById but fails to clear a previously set sulfur
remap flag, leaving stale ids in remappedLegacyEntityIds; update
prefetchAddEntityMetadataHints so that when the spawn is NOT the sulfur type
(i.e., the else branch where you call clientVisibleEntityTypeById.put(id, type))
you also remove id from remappedLegacyEntityIds (e.g.,
remappedLegacyEntityIds.remove(id)) so reused entity ids don't retain the sulfur
remap state (refer to prefetchAddEntityMetadataHints,
clientVisibleEntityTypeById, remappedLegacyEntityIds, and
LegacyLinkConstants.SULFUR_CUBE_ENTITY_ID).
In `@src/main/java/dev/ohno/legacylink/mapping/LegacyEntityTypeWireRemapper.java`:
- Around line 20-21: The static field legacySyncOrder is being swapped in
rebuild() and later read in legacyNetworkId() without safe publication; make the
field thread-safe by declaring it volatile (e.g., private static volatile
List<Identifier> legacySyncOrder) or replace it with an
AtomicReference<List<Identifier>> and update rebuild() to set the new immutable
list reference and have legacyNetworkId() read the reference into a local
variable before using it; this ensures the rebuilt list is safely published to
other threads and prevents reads from seeing the initial empty list.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: b687a7bf-e6c1-4fda-b3bc-1dcd7372eaff
📒 Files selected for processing (18)
README.mdsrc/main/java/dev/ohno/legacylink/LegacyLinkConstants.javasrc/main/java/dev/ohno/legacylink/LegacyLinkMod.javasrc/main/java/dev/ohno/legacylink/debug/CameraPacketTrace.javasrc/main/java/dev/ohno/legacylink/debug/LegacyOutboundPacketCapture.javasrc/main/java/dev/ohno/legacylink/debug/LegacyPacketMapTrace.javasrc/main/java/dev/ohno/legacylink/debug/PacketReflectionUtil.javasrc/main/java/dev/ohno/legacylink/debug/PositionPacketTrace.javasrc/main/java/dev/ohno/legacylink/encoding/LegacyOutboundEncoding.javasrc/main/java/dev/ohno/legacylink/handler/LegacyChunkTranslator.javasrc/main/java/dev/ohno/legacylink/handler/LegacyPacketHandler.javasrc/main/java/dev/ohno/legacylink/handler/rewrite/AdvancementRewriter.javasrc/main/java/dev/ohno/legacylink/handler/rewrite/BlockStatePacketRewriter.javasrc/main/java/dev/ohno/legacylink/handler/rewrite/VillagerEntityData2661.javasrc/main/java/dev/ohno/legacylink/mapping/LegacyAttributeWireTable.javasrc/main/java/dev/ohno/legacylink/mapping/LegacyEntityTypeWireRemapper.javasrc/main/java/dev/ohno/legacylink/mapping/LegacyItemIdTable.javasrc/main/java/dev/ohno/legacylink/runtime/LegacyRuntimeContext.java
✅ Files skipped from review due to trivial changes (1)
- src/main/java/dev/ohno/legacylink/debug/PositionPacketTrace.java
🚧 Files skipped from review as they are similar to previous changes (7)
- src/main/java/dev/ohno/legacylink/encoding/LegacyOutboundEncoding.java
- README.md
- src/main/java/dev/ohno/legacylink/debug/CameraPacketTrace.java
- src/main/java/dev/ohno/legacylink/handler/rewrite/AdvancementRewriter.java
- src/main/java/dev/ohno/legacylink/mapping/LegacyItemIdTable.java
- src/main/java/dev/ohno/legacylink/handler/rewrite/VillagerEntityData2661.java
- src/main/java/dev/ohno/legacylink/runtime/LegacyRuntimeContext.java
…tion - remapUpdateTags: build a new item tag map and NetworkPayload via reflection, shallow-copy the registry→payload map, return new ClientboundUpdateTagsPacket so shared server packets are never mutated for one legacy connection. - prefetchAddEntityMetadataHints: clear remappedLegacyEntityIds when spawn is not sulfur_cube so reused entity ids do not keep slime tail-strip state. - LegacyEntityTypeWireRemapper: volatile legacySyncOrder + local read in legacyNetworkId for safe publication after rebuild(). Made-with: Cursor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/main/java/dev/ohno/legacylink/handler/LegacyPacketHandler.java (1)
739-742: Avoid copying unchanged tag lists unless a remap actually happens.
new IntArrayList(ids)for unchanged entries does extra work even whenanyChangedstays false and the original packet is returned.♻️ Leaner unchanged-entry handling
- } else { - newItemTagMap.put(entry.getKey(), new IntArrayList(ids)); - } + } else { + newItemTagMap.put(entry.getKey(), ids); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/dev/ohno/legacylink/handler/LegacyPacketHandler.java` around lines 739 - 742, The code always allocates a new IntArrayList(ids) for unchanged tag entries which wastes work and can prevent returning the original packet when anyChanged remains false; in the loop in LegacyPacketHandler where newItemTagMap is populated, only create and put a new IntArrayList(ids) when a remap actually occurred (or when anyChanged is true), otherwise reuse the existing ids list (e.g., put entry.getValue() or ids directly) for entry.getKey() so no unnecessary copy is made and the original packet can still be returned when anyChanged is false.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/dev/ohno/legacylink/handler/LegacyPacketHandler.java`:
- Around line 163-170: The write() method lacks a defensive check for non-legacy
connections, so add an early-guard that checks
LegacyTracker.isLegacy(encodeConn) (treat null encodeConn as non-legacy) and, if
false, bypass the legacy translation path by calling ctx.write(msg, promise)
directly and returning; otherwise proceed to enter the LegacyOutboundEncoding
scope and call writeTranslated(ctx, msg, promise) as before. Ensure you
reference the existing encodeConn resolution
(ctx.pipeline().get(HandlerNames.PACKET_HANDLER) instanceof Connection c ? c :
null) and keep writeTranslated(...) and LegacyOutboundEncoding.enterScoped(...)
unchanged for legacy flows.
---
Nitpick comments:
In `@src/main/java/dev/ohno/legacylink/handler/LegacyPacketHandler.java`:
- Around line 739-742: The code always allocates a new IntArrayList(ids) for
unchanged tag entries which wastes work and can prevent returning the original
packet when anyChanged remains false; in the loop in LegacyPacketHandler where
newItemTagMap is populated, only create and put a new IntArrayList(ids) when a
remap actually occurred (or when anyChanged is true), otherwise reuse the
existing ids list (e.g., put entry.getValue() or ids directly) for
entry.getKey() so no unnecessary copy is made and the original packet can still
be returned when anyChanged is false.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3d20da82-fcd9-4038-a134-81774ad5f62f
📒 Files selected for processing (2)
src/main/java/dev/ohno/legacylink/handler/LegacyPacketHandler.javasrc/main/java/dev/ohno/legacylink/mapping/LegacyEntityTypeWireRemapper.java
- write(): if encodeConn is null or not legacy, ctx.write and return without ThreadLocal encoding scope or writeTranslated. - remapUpdateTags: scan tag ids first; return original packet when nothing remaps; build new map only when needed, reusing original IntList refs for unchanged tag entries. Made-with: Cursor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/dev/ohno/legacylink/handler/LegacyPacketHandler.java`:
- Around line 366-369: The current remapEntityData() uses
LegacyRuntimeContext.findEntity(entityId) which may block; remove that blocking
fallback and instead perform a non-blocking best-effort lookup by calling
entityForLegacyRecipient(...) and then clientVisibleEntityTypeById(...) to
resolve the type for the legacy recipient; if those return null/unknown, simply
skip the global LegacyRuntimeContext.findEntity(...) call and leave the type
unresolved rather than blocking the Netty write path. Ensure you only use
entityForLegacyRecipient(...) + clientVisibleEntityTypeById(...) and do not call
LegacyRuntimeContext.findEntity(...) from remapEntityData().
- Around line 217-221: The loop in LegacyPacketHandler currently writes split
packets with ctx.voidPromise() for all but the last, which loses failures;
change the logic to create and collect a ChannelPromise for each child write
(e.g., ctx.newPromise()), call tracePostRewriteIfEnabled(connection, p) then
super.write(ctx, p, childPromise) for every packet, and attach listeners that
aggregate results: if any childPromise fails, fail the original promise,
otherwise when all succeed complete the original promise. Use the existing
promise variable as the aggregate target and ensure you remove reliance on
ctx.voidPromise() so all sub-write failures propagate back to the caller.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 873ad71d-bdea-4530-930c-19a40507adfb
📒 Files selected for processing (1)
src/main/java/dev/ohno/legacylink/handler/LegacyPacketHandler.java
… promises - resolveMetadataEntityType: drop LegacyRuntimeContext.findEntity fallback on Netty path; use recipient-level entity + prefetch only; update javadoc. - ClientboundBundlePacket flatten: per-child ctx.newPromise() with listeners so any encode failure fails the aggregate promise; voidPromise removed. Made-with: Cursor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/main/java/dev/ohno/legacylink/handler/LegacyPacketHandler.java (1)
549-611: Cache recipient-world entity/type once inremapEntityData(...)to avoid repeated lookups.
entityForLegacyRecipient(entityId)is resolved multiple times in this path (directly and viaresolveMetadataEntityType(...)). Capture it once and thread the derived type through logging/rewrite calls to reduce repeated world map probes and avoid intra-call drift under entity-id reuse.♻️ Proposed refactor
public ClientboundSetEntityDataPacket remapEntityData(ClientboundSetEntityDataPacket packet) { int entityId = packet.id(); List<SynchedEntityData.DataValue<?>> packedItems = packet.packedItems(); + Entity recipientEntity = entityForLegacyRecipient(entityId); + EntityType<?> recipientWorldType = toClientVisibleEntityType(entityTypeOf(recipientEntity)); ... - EntityType<?> clientType = resolveMetadataEntityType(entityId); + EntityType<?> clientType = resolveMetadataEntityType(entityId /* optionally pass recipientWorldType */); ... - entityTypeOf(entityForLegacyRecipient(entityId)), + recipientWorldType, clientVisibleEntityTypeById.get(entityId), false ); ... var villagerRewritten = VillagerEntityData2661.rewriteIfNeeded( entityId, items, clientVisibleEntityTypeById.get(entityId), - entityTypeOf(entityForLegacyRecipient(entityId))); + recipientWorldType);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/dev/ohno/legacylink/handler/LegacyPacketHandler.java` around lines 549 - 611, In remapEntityData(...) avoid repeatedly calling entityForLegacyRecipient(entityId) and resolveMetadataEntityType(entityId); instead capture the recipient entity (e.g., var recipient = entityForLegacyRecipient(entityId)) and the derived clientType once, then use those captured values for LegacyPacketMapTrace.logEntityDataContext, clientVisibleEntityTypeById.get(entityId) lookups and the subsequent rewrite/trim calls (CubeMobEntityData2661.rewriteIfNeeded, VillagerEntityData2661.rewriteIfNeeded, Vanilla261EntityMetadataTailTrim2661.trimIfNeeded) to prevent repeated world map probes and potential intra-call drift when entity IDs are reused.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/main/java/dev/ohno/legacylink/handler/LegacyPacketHandler.java`:
- Around line 549-611: In remapEntityData(...) avoid repeatedly calling
entityForLegacyRecipient(entityId) and resolveMetadataEntityType(entityId);
instead capture the recipient entity (e.g., var recipient =
entityForLegacyRecipient(entityId)) and the derived clientType once, then use
those captured values for LegacyPacketMapTrace.logEntityDataContext,
clientVisibleEntityTypeById.get(entityId) lookups and the subsequent
rewrite/trim calls (CubeMobEntityData2661.rewriteIfNeeded,
VillagerEntityData2661.rewriteIfNeeded,
Vanilla261EntityMetadataTailTrim2661.trimIfNeeded) to prevent repeated world map
probes and potential intra-call drift when entity IDs are reused.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 930e8c8f-0782-4dea-98cc-f378b2d3c333
📒 Files selected for processing (1)
src/main/java/dev/ohno/legacylink/handler/LegacyPacketHandler.java
Capture entityForLegacyRecipient once, resolveMetadataEntityType(entityId, recipient) once, prefetchVisibleType after reconcile, and reuse for trace, villager rewrite, cube/tail paths (no repeated world probes or intra-call drift). Made-with: Cursor
This was referenced Apr 12, 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
LegacyItemIdTable+ItemStackOptionalCodecMixin— 26.2 inserts sulfur/cinnabar items into the built-in registry, shifting all subsequent IDs. We build a static translation table at startup and emit 26.1-compatible varints for legacy connections instead of stripping registry entries.LegacyEntityTypeWireRemapper) drops thesulfur_cubeentity type from the synced registry and adjusts wire IDs so the 26.1 client decodes entity spawns correctly.LegacyUpdateAttributesWirePatcher) strips 26.2-only attributes (bounciness, air_drag, friction) fromupdate_attributespackets post-encode.LegacyChunkTranslator) remaps 26.2 block state IDs to 26.1-safe values in palette and direct-encoded sections, with a hard ceiling pinned to 26.1.1's max blockstate ID (30207).LegacyPacketMapTrace,SpawnPacketTrace,EntityDataRewriteTrace, etc.) gated behind system properties for live diagnosis.Commits
bf3f6c3— Harden 26.1 compatibility translation paths and status signaling1be2bc3— Stabilize 26.1 client bridge: entity types, metadata, items, attributesfde0767— Fix 26.1 chunk remap ceiling and section parsing438a632— Pin 26.1 blockstate ceiling to 26.1.1-safe maximum66a8a78— Fix item wire-ID translation for 26.1 clients (sulfur/cinnabar shift)Test plan
DecoderExceptionon entity metadata packetsstable-v2Made with Cursor
Summary by CodeRabbit
New Features
Bug Fixes
Documentation