Conversation
Force PacketEvents user version to 26.2 for LegacyLink-marked connections at handshake/config transitions so Reaper movement logic uses modern semantics while wire translation remains 26.1-compatible. Made-with: Cursor
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 1 minutes and 31 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughIntroduces a new PacketEventsVersionBridge that conditionally forces PacketEvents per-user client version to V_26_2 via reflection for legacy connections, and invokes it from existing mixin injection points and legacy setup paths. Changes
Sequence Diagram(s)sequenceDiagram
participant Connection
participant PacketEventsAPI
participant ProtocolManager
participant User
Connection->>PacketEventsAPI: PacketEvents.getAPI() (reflective)
PacketEventsAPI->>ProtocolManager: api.getProtocolManager() (reflective)
ProtocolManager->>User: getUser(protocolManager, connection.channel) (reflective)
User->>User: setClientVersion(V_26_2) (reflective)
Note over Connection,User: If any lookup fails or V_26_2 missing → exit silently (debug/warn logs)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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: 1
🧹 Nitpick comments (1)
src/main/java/dev/ohno/legacylink/mixin/HandshakeMixin.java (1)
27-50: Extract repeated legacy setup sequence to one helper.Lines 33-35 and Lines 47-49 duplicate the same sequence. Consider a small private helper to keep ordering consistent across both injection paths.
♻️ Suggested refactor
+ private void legacylink$prepareLegacyConnection() { + LegacyTracker.markLegacy(this.connection); + LegacyPacketHandler.install(this.connection); + PacketEventsVersionBridge.force26_2IfPresent(this.connection); + } + `@Inject`(method = "beginLogin", at = `@At`("HEAD"), cancellable = true) private void legacylink$acceptLegacyClient(ClientIntentionPacket packet, boolean transfer, CallbackInfo ci) { if (packet.protocolVersion() == LegacyLinkConstants.PROTOCOL_26_1) { @@ - LegacyTracker.markLegacy(this.connection); - LegacyPacketHandler.install(this.connection); - PacketEventsVersionBridge.force26_2IfPresent(this.connection); + legacylink$prepareLegacyConnection(); @@ `@Inject`(method = "handleIntention", at = `@At`("HEAD")) private void legacylink$markLegacyOnStatusPing(ClientIntentionPacket packet, CallbackInfo ci) { if (packet.protocolVersion() == LegacyLinkConstants.PROTOCOL_26_1) { - LegacyTracker.markLegacy(this.connection); - LegacyPacketHandler.install(this.connection); - PacketEventsVersionBridge.force26_2IfPresent(this.connection); + legacylink$prepareLegacyConnection(); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/dev/ohno/legacylink/mixin/HandshakeMixin.java` around lines 27 - 50, Extract the repeated legacy setup sequence (LegacyTracker.markLegacy(this.connection); LegacyPacketHandler.install(this.connection); PacketEventsVersionBridge.force26_2IfPresent(this.connection);) into a single private helper method on HandshakeMixin (e.g., private void applyLegacySetup(ConnectionType connection) or applyLegacySetup()) and replace the duplicated blocks in legacylink$acceptLegacyClient and legacylink$markLegacyOnStatusPing with a call to that helper to ensure ordering is identical in both injection paths.
🤖 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/integration/PacketEventsVersionBridge.java`:
- Around line 38-47: The call to Enum.valueOf((Class<? extends Enum>)
clientVersionClass.asSubclass(Enum.class), "V_26_2") can throw
IllegalArgumentException when the ClientVersion enum lacks V_26_2; add a handler
to catch IllegalArgumentException around the Enum.valueOf invocation in
PacketEventsVersionBridge (the block that computes normalizedVersion and invokes
setClientVersion) and treat it like a missing PacketEvents class: call
logUnavailableOnce() (or otherwise no-op) so the optional PacketEvents
integration doesn't break login/config flow; ensure the catch does not hide real
reflection issues handled by the existing ReflectiveOperationException handler.
---
Nitpick comments:
In `@src/main/java/dev/ohno/legacylink/mixin/HandshakeMixin.java`:
- Around line 27-50: Extract the repeated legacy setup sequence
(LegacyTracker.markLegacy(this.connection);
LegacyPacketHandler.install(this.connection);
PacketEventsVersionBridge.force26_2IfPresent(this.connection);) into a single
private helper method on HandshakeMixin (e.g., private void
applyLegacySetup(ConnectionType connection) or applyLegacySetup()) and replace
the duplicated blocks in legacylink$acceptLegacyClient and
legacylink$markLegacyOnStatusPing with a call to that helper to ensure ordering
is identical in both injection paths.
🪄 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: 75f36bb6-4305-4dbf-8e88-84c9b30fe174
📒 Files selected for processing (3)
src/main/java/dev/ohno/legacylink/integration/PacketEventsVersionBridge.javasrc/main/java/dev/ohno/legacylink/mixin/ConfigurationFinishMixin.javasrc/main/java/dev/ohno/legacylink/mixin/HandshakeMixin.java
Canonicalize item remapping through the legacy wire-id table across packet and utility paths, add missing equipment item rewriting, and make PacketEvents version normalization safely no-op when V_26_2 is unavailable while keeping reflection failures visible. Made-with: Cursor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/main/java/dev/ohno/legacylink/handler/rewrite/ItemRewriter.java (1)
22-32: Centralize the item-id round-trip in one helper.
remapItemToLegacySafe()andremapItemIdStrict()now inline the sametoLegacyId()→serverItemIdFromLegacyWire()translation thatRegistryRemapper.remapItem()already owns. Calling that helper from both sites would keep this PR’s canonicalization logic in one place and reduce the chance of these paths drifting again.♻️ Proposed cleanup
import dev.ohno.legacylink.mapping.LegacyItemIdTable; +import dev.ohno.legacylink.mapping.RegistryRemapper; ... - int serverId = Item.getId(item); - int legacyWireId = LegacyItemIdTable.toLegacyId(serverId); - int canonicalServerId = LegacyItemIdTable.serverItemIdFromLegacyWire(legacyWireId); + int canonicalServerId = RegistryRemapper.remapItem(Item.getId(item)); Item mapped = BuiltInRegistries.ITEM.byId(canonicalServerId); ... - int legacyWireId = LegacyItemIdTable.toLegacyId(itemId); - return LegacyItemIdTable.serverItemIdFromLegacyWire(legacyWireId); + return RegistryRemapper.remapItem(itemId);Also applies to: 72-74
🤖 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/ItemRewriter.java` around lines 22 - 32, Replace the duplicated two-step canonicalization (LegacyItemIdTable.toLegacyId → serverItemIdFromLegacyWire) in remapItemToLegacySafe with a call to the central helper RegistryRemapper.remapItem so canonicalization lives in one place; do the same change for remapItemIdStrict (the other inlined occurrence) and preserve the existing fallback behavior (return Items.STONE when the remapped item is null). Ensure you call RegistryRemapper.remapItem(serverId) (or the exact helper signature) to obtain the canonical server id/item and then keep the null-check/fallback logic intact.
🤖 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/rewrite/ItemRewriter.java`:
- Around line 22-32: Replace the duplicated two-step canonicalization
(LegacyItemIdTable.toLegacyId → serverItemIdFromLegacyWire) in
remapItemToLegacySafe with a call to the central helper
RegistryRemapper.remapItem so canonicalization lives in one place; do the same
change for remapItemIdStrict (the other inlined occurrence) and preserve the
existing fallback behavior (return Items.STONE when the remapped item is null).
Ensure you call RegistryRemapper.remapItem(serverId) (or the exact helper
signature) to obtain the canonical server id/item and then keep the
null-check/fallback logic intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4dcfe419-d7bd-4cdc-b6a2-8e122cfe43a2
📒 Files selected for processing (5)
src/main/java/dev/ohno/legacylink/handler/LegacyPacketHandler.javasrc/main/java/dev/ohno/legacylink/handler/rewrite/ItemRewriter.javasrc/main/java/dev/ohno/legacylink/integration/PacketEventsVersionBridge.javasrc/main/java/dev/ohno/legacylink/mapping/RegistryRemapper.javasrc/main/java/dev/ohno/legacylink/mixin/HandshakeMixin.java
✅ Files skipped from review due to trivial changes (2)
- src/main/java/dev/ohno/legacylink/mixin/HandshakeMixin.java
- src/main/java/dev/ohno/legacylink/integration/PacketEventsVersionBridge.java
Ensure ItemStackTemplate stream codec remaps item ids for legacy sessions so update_advancements icons cannot send out-of-range ids. Also keep status ping path lightweight and collapse non-vanilla namespaces to safe legacy item fallbacks. Made-with: Cursor
Replace duplicated LegacyItemIdTable round-trip logic in ItemRewriter with RegistryRemapper.remapItem for both remapItemToLegacySafe and remapItemIdStrict while preserving the existing stone fallback. Made-with: Cursor
This was referenced Apr 15, 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.
Force PacketEvents user version to 26.2 for LegacyLink-marked connections at handshake/config transitions so Reaper movement logic uses modern semantics while wire translation remains 26.1-compatible.
Made-with: Cursor
Summary by CodeRabbit
Bug Fixes
New Features
Refactor