Fix runtime packet handling and swimming state#37
Open
llucasandersen wants to merge 9 commits into
Open
Conversation
|
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:
📝 WalkthroughWalkthroughIntroduces ChangesFolia Scheduling, State Guards, and Chunk Decoding
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 3
🧹 Nitpick comments (4)
src/main/java/tf/tuff/netty/ChunkHandler.java (1)
160-160: 📐 Maintainability & Code Quality | 💤 Low valueUnnecessary decode call.
decodeSectionCoordX(chunkSectionPos)is called but its result is discarded. The comment says it "validates bit layout," but no validation exception is thrown, and the coordinates are decoded again insidedecodeMultiBlockChangePosition. This call can be removed.Suggested fix
- decodeSectionCoordX(chunkSectionPos); // validates bit layout alongside the packed entries below buf.readBoolean();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/tf/tuff/netty/ChunkHandler.java` at line 160, Remove the redundant call to decodeSectionCoordX(chunkSectionPos) — it returns a value that is discarded and the coordinates are already decoded inside decodeMultiBlockChangePosition; delete the standalone call and any comment that implies validation occurs there, leaving decodeMultiBlockChangePosition to perform the decoding/validation for chunkSectionPos.src/main/java/tf/tuff/TuffX.java (1)
93-105: 📐 Maintainability & Code Quality | ⚡ Quick winConsider also unregistering outgoing plugin channels for completeness.
The incoming channel cleanup is correct, but sub-plugins like ViaEntitiesPlugin register outgoing channels with
TuffXas the plugin owner. For defensive cleanup, consider also callinggetServer().getMessenger().unregisterOutgoingPluginChannel(this);to ensure all registered channels are cleaned up during shutdown.♻️ Proposed enhancement
PacketEvents.getAPI().terminate(); getServer().getMessenger().unregisterIncomingPluginChannel(this); + getServer().getMessenger().unregisterOutgoingPluginChannel(this); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/tf/tuff/TuffX.java` around lines 93 - 105, The onDisable method currently unregisters incoming plugin channels but misses outgoing channels; update onDisable in TuffX.java (method onDisable) to also call getServer().getMessenger().unregisterOutgoingPluginChannel(this) (in addition to the existing unregisterIncomingPluginChannel(this)) so any outgoing channels registered by sub-plugins (e.g., ViaEntitiesPlugin) are cleaned up; ensure this call is placed alongside the existing messenger cleanup and executed before returning from onDisable.src/main/java/tf/tuff/tuffactions/swimming/Swimming.java (2)
127-142: 📐 Maintainability & Code Quality | 💤 Low valueUse explicit Iterator for cleaner removal during iteration.
While
ConcurrentHashMap.newKeySet()won't throwConcurrentModificationException, removing via the set reference (rather thaniterator.remove()) during iteration can lead to skipped elements in edge cases. Using an explicit Iterator provides more predictable behavior.♻️ Suggested refactor using Iterator
private void maintainSwimmingState() { - for (UUID playerId : swimmingPlayers) { + var iterator = swimmingPlayers.iterator(); + while (iterator.hasNext()) { + UUID playerId = iterator.next(); Player player = Bukkit.getPlayer(playerId); if (player == null || !player.isOnline()) { - swimmingPlayers.remove(playerId); + iterator.remove(); continue; } if (!player.isInWater()) { - swimmingPlayers.remove(playerId); + iterator.remove(); applySwimmingState(player, false); broadcastSwimState(player, false); continue; } applySwimmingState(player, true); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/tf/tuff/tuffactions/swimming/Swimming.java` around lines 127 - 142, The loop in maintainSwimmingState removes from swimmingPlayers directly while iterating, which can skip elements; change it to use an explicit Iterator over swimmingPlayers (Iterator<UUID> it = swimmingPlayers.iterator()) and call it.remove() when you need to remove the current UUID; keep the same logic calling Bukkit.getPlayer(playerId), applySwimmingState(player, false/true) and broadcastSwimState(player, false) but ensure removals use iterator.remove() so iteration is predictable.
79-90: 🎯 Functional Correctness | 💤 Low valueConsider cancelling the event to prevent state flicker.
The current approach lets the event proceed (setting swimming to false), then schedules a task to re-apply swimming. This creates a brief one-tick window where the player's swimming state is false. Cancelling the event directly would be more responsive and prevent the intermediate state.
♻️ Alternative approach using event cancellation
public void handleToggleSwim(EntityToggleSwimEvent event) { if (!isEnabled()) return; if (!(event.getEntity() instanceof Player)) return; Player player = (Player) event.getEntity(); if (!event.isSwimming() && swimmingPlayers.contains(player.getUniqueId())) { - plugin.getServer().getScheduler().runTask(plugin, () -> { - if (swimmingPlayers.contains(player.getUniqueId()) && player.isOnline()) { - applySwimmingState(player, true); - } - }); + event.setCancelled(true); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/tf/tuff/tuffactions/swimming/Swimming.java` around lines 79 - 90, The handleToggleSwim method presently lets EntityToggleSwimEvent run then re-applies swimming on the next tick causing a flicker; change the logic so when the event is coming from a Player and you detect that swimming should remain true (i.e., !event.isSwimming() && swimmingPlayers.contains(player.getUniqueId())), call event.setCancelled(true) on that EntityToggleSwimEvent to prevent the state flip and then ensure you call applySwimmingState(player, true) (immediately or via the existing scheduler if needed) so the player stays in the expected swimming state without the one-tick false window; update references in handleToggleSwim and keep applySwimmingState usage consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/java/tf/tuff/netty/ChunkHandler.java`:
- Around line 172-202: The leak is caused by double-retaining the ByteBuf:
resolveViaDataOnMainThread retains buf and then writeWithViaOnly retains it
again for the composite; remove the extra retain in writeWithViaOnly so the
composite takes the caller's ownership, and update resolveViaDataOnMainThread so
you do not release the retained buffer in the finally block when you call
writeWithViaOnly (ensure only the code path that does currentCtx.write(...)
releases the retained buffer). Touch the methods resolveViaDataOnMainThread and
writeWithViaOnly to transfer ownership consistently (caller retains once, callee
must not retain) and ensure all other paths still release appropriately.
In `@src/main/java/tf/tuff/tuffactions/swimming/Swimming.java`:
- Around line 40-46: The enable method's null check on swimStateTask can miss
cases where the Task object exists but the scheduled task was cancelled; update
the scheduling guard in enable(...) to schedule a new task when swimStateTask is
null OR swimStateTask.isCancelled() is true, using the same call to
plugin.getServer().getScheduler().runTaskTimer(...) to assign swimStateTask;
reference swimStateTask, enable, maintainSwimmingState and the scheduler
runTaskTimer call when making the change.
In `@src/main/java/tf/tuff/viablocks/ViaBlocksPlugin.java`:
- Around line 82-83: Reload currently clears the viaBlocksEnabledPlayers set in
onTuffXReload which causes online players to lose ViaBlocks-enabled state and
stop receiving chunk/block packets until they re-handshake; instead preserve or
rebuild enabled-player state across reloads by iterating online players after
reload and repopulating viaBlocksEnabledPlayers (or trigger a re-handshake) for
those where CustomBlockListener.hasViaBlocksPlayersInWorld(...) /
plugin.isPlayerEnabled(...) previously returned true; update onTuffXReload to
either restore the set from a persisted snapshot or call the same logic used in
handlePacket -> setPlayerEnabled(player, true) for each online player so
CustomBlockListener.handleChunkLoad continues to prepare caches without
requiring clients to resend viablocks:handshake.
---
Nitpick comments:
In `@src/main/java/tf/tuff/netty/ChunkHandler.java`:
- Line 160: Remove the redundant call to decodeSectionCoordX(chunkSectionPos) —
it returns a value that is discarded and the coordinates are already decoded
inside decodeMultiBlockChangePosition; delete the standalone call and any
comment that implies validation occurs there, leaving
decodeMultiBlockChangePosition to perform the decoding/validation for
chunkSectionPos.
In `@src/main/java/tf/tuff/tuffactions/swimming/Swimming.java`:
- Around line 127-142: The loop in maintainSwimmingState removes from
swimmingPlayers directly while iterating, which can skip elements; change it to
use an explicit Iterator over swimmingPlayers (Iterator<UUID> it =
swimmingPlayers.iterator()) and call it.remove() when you need to remove the
current UUID; keep the same logic calling Bukkit.getPlayer(playerId),
applySwimmingState(player, false/true) and broadcastSwimState(player, false) but
ensure removals use iterator.remove() so iteration is predictable.
- Around line 79-90: The handleToggleSwim method presently lets
EntityToggleSwimEvent run then re-applies swimming on the next tick causing a
flicker; change the logic so when the event is coming from a Player and you
detect that swimming should remain true (i.e., !event.isSwimming() &&
swimmingPlayers.contains(player.getUniqueId())), call event.setCancelled(true)
on that EntityToggleSwimEvent to prevent the state flip and then ensure you call
applySwimmingState(player, true) (immediately or via the existing scheduler if
needed) so the player stays in the expected swimming state without the one-tick
false window; update references in handleToggleSwim and keep applySwimmingState
usage consistent.
In `@src/main/java/tf/tuff/TuffX.java`:
- Around line 93-105: The onDisable method currently unregisters incoming plugin
channels but misses outgoing channels; update onDisable in TuffX.java (method
onDisable) to also call
getServer().getMessenger().unregisterOutgoingPluginChannel(this) (in addition to
the existing unregisterIncomingPluginChannel(this)) so any outgoing channels
registered by sub-plugins (e.g., ViaEntitiesPlugin) are cleaned up; ensure this
call is placed alongside the existing messenger cleanup and executed before
returning from onDisable.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 80cf6668-62b1-44ac-b6d7-b087a2f792bc
⛔ Files ignored due to path filters (1)
builds/TuffXPlus-1.0.1-beta.jaris excluded by!**/*.jar
📒 Files selected for processing (8)
build.gradlesrc/main/java/tf/tuff/TuffX.javasrc/main/java/tf/tuff/netty/ChunkHandler.javasrc/main/java/tf/tuff/tuffactions/swimming/Swimming.javasrc/main/java/tf/tuff/viablocks/CustomBlockListener.javasrc/main/java/tf/tuff/viablocks/ViaBlocksPlugin.javasrc/main/java/tf/tuff/y0/Y0Plugin.javasrc/test/java/tf/tuff/netty/ChunkHandlerTest.java
Comment on lines
172
to
202
| private void resolveViaDataOnMainThread(ChannelHandlerContext ctx, ByteBuf buf, ChannelPromise promise, | ||
| java.util.concurrent.Callable<byte[]> supplier) { | ||
| ByteBuf retained = buf.retain(); | ||
| Bukkit.getScheduler().runTask(viaBlocks.plugin.plugin, () -> { | ||
| byte[] data = null; | ||
| try { | ||
| if (player.isOnline() && isViaActive()) { | ||
| data = supplier.call(); | ||
| } | ||
| } catch (Exception ignored) { | ||
| } | ||
| final byte[] resolvedData = data; | ||
|
|
||
| ChannelHandlerContext currentCtx = this.ctx != null ? this.ctx : ctx; | ||
| currentCtx.channel().eventLoop().execute(() -> { | ||
| try { | ||
| retained.resetReaderIndex(); | ||
| if (resolvedData != null && resolvedData.length > 0) { | ||
| writeWithViaOnly(currentCtx, retained, promise, resolvedData); | ||
| } else { | ||
| currentCtx.write(retained, promise); | ||
| } | ||
| } catch (Exception ignored) { | ||
| } finally { | ||
| if (retained.refCnt() > 0) { | ||
| retained.release(); | ||
| } | ||
| } | ||
| }); | ||
| }); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Memory leak in writeWithViaOnly path.
When resolveViaDataOnMainThread takes the writeWithViaOnly path, there's one unreleased buffer reference:
bufstarts withrefCnt=1- Line 174:
buf.retain()→refCnt=2 - Line 314 (
writeWithViaOnly):buf.retain()→refCnt=3, composite takes ownership of this reference - Line 197:
retained.release()→refCnt=2 - After
ctx.write(composite)completes, composite releases its component →refCnt=1
The original reference is never released, causing a memory leak.
In writeWithViaOnly, you're retaining the buffer to add it to the composite, but the caller (resolveViaDataOnMainThread) also holds a retained reference. You should not retain again in writeWithViaOnly since the caller is passing ownership of its retained reference.
Proposed fix
private void writeWithViaOnly(ChannelHandlerContext ctx, ByteBuf buf, ChannelPromise promise,
byte[] data) throws Exception {
ByteBuf tail = ctx.alloc().buffer();
tail.writeBytes(data);
CompositeByteBuf composite = ctx.alloc().compositeBuffer();
- composite.addComponents(true, buf.retain(), tail);
+ composite.addComponents(true, buf, tail);
ctx.write(composite, promise);
}And adjust resolveViaDataOnMainThread to not release when writing via writeWithViaOnly (since composite takes ownership), or remove the finally release entirely and let the write operations handle it:
try {
retained.resetReaderIndex();
if (resolvedData != null && resolvedData.length > 0) {
writeWithViaOnly(currentCtx, retained, promise, resolvedData);
} else {
currentCtx.write(retained, promise);
}
} catch (Exception ignored) {
- } finally {
- if (retained.refCnt() > 0) {
- retained.release();
- }
+ } catch (Exception e) {
+ if (retained.refCnt() > 0) {
+ retained.release();
+ }
}Also applies to: 308-317
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/java/tf/tuff/netty/ChunkHandler.java` around lines 172 - 202, The
leak is caused by double-retaining the ByteBuf: resolveViaDataOnMainThread
retains buf and then writeWithViaOnly retains it again for the composite; remove
the extra retain in writeWithViaOnly so the composite takes the caller's
ownership, and update resolveViaDataOnMainThread so you do not release the
retained buffer in the finally block when you call writeWithViaOnly (ensure only
the code path that does currentCtx.write(...) releases the retained buffer).
Touch the methods resolveViaDataOnMainThread and writeWithViaOnly to transfer
ownership consistently (caller retains once, callee must not retain) and ensure
all other paths still release appropriately.
Contributor
Author
|
Fixed the current review items on the branch: the ChunkHandler Via-only path now uses consistent buffer ownership, TuffX unregisters outgoing plugin channels on disable, ViaBlocks preserves enabled players across reload, and Swimming now cancels the false toggle while cleaning up iteration/removal behavior. |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/tf/tuff/y0/Y0Plugin.java (1)
425-436:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPrevent stale cross-world chunk payload delivery.
Line 425 currently doesn’t verify that
playeris still inc.getWorld(). If the player changes worlds before this call runs, old-world chunk payloads can still be sent.Suggested fix
public void processAndSendChunk(final Player player, final Chunk c) { if (c == null || player == null || !player.isOnline()) return; + if (!player.getWorld().equals(c.getWorld())) return; if (enabledWorlds != null && !enabledWorlds.contains(c.getWorld().getName())) return;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/tf/tuff/y0/Y0Plugin.java` around lines 425 - 436, The processAndSendChunk method checks if the player is online at the start but does not verify the player remains in the chunk's world before sending cached chunk payloads. If a player switches worlds between the initial validation and the actual packet sending, stale chunk data from the old world could be delivered. Add an additional check right before the loop that sends payloads (where SchedulerCompat.sendPluginMessage is called) to verify that player.getWorld() equals c.getWorld() to ensure the player is still in the correct world, and only proceed with sending the chunk data if this condition is true.
🧹 Nitpick comments (2)
src/main/java/tf/tuff/tuffactions/swimming/Swimming.java (1)
43-46: 💤 Low valueEmpty override can be removed.
This method only delegates to
super.enable(wasEnabled)without adding any behavior. Unless it's intentionally kept as a placeholder for future functionality, it can be removed.Note: The past review comment about checking
isCancelled()no longer applies since the globalswimStateTaskwas replaced with per-player tasks inswimStateTasks.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/tf/tuff/tuffactions/swimming/Swimming.java` around lines 43 - 46, The enable method in the Swimming class is an empty override that only delegates to the parent class implementation without adding any custom behavior. Remove the entire enable method override from the Swimming class, as the parent class implementation will be invoked automatically when the method is not overridden.src/main/java/tf/tuff/util/SchedulerCompat.java (1)
75-82: 💤 Low valueMissing null check for
channelparameter.The
sendPluginMessagemethod validatesplayerandpayloadfor null but notchannel. If a caller passes a null channel,player.sendPluginMessagewill throw an NPE inside the scheduled task.🛡️ Suggested fix
public static void sendPluginMessage(Plugin plugin, Player player, String channel, byte[] payload) { - if (player == null || payload == null || !player.isOnline()) return; + if (player == null || channel == null || payload == null || !player.isOnline()) return; runEntity(player, plugin, () -> { if (player.isOnline()) { player.sendPluginMessage(plugin, channel, payload); } }); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/tf/tuff/util/SchedulerCompat.java` around lines 75 - 82, The sendPluginMessage method validates player and payload parameters for null in the initial guard clause but is missing a null check for the channel parameter. Add channel to the existing null validation in the early return statement (alongside the existing checks for player and payload) to prevent a potential NPE when player.sendPluginMessage is called inside the runEntity task.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/java/tf/tuff/ServerRegistry.java`:
- Around line 24-25: The `running` field in the ServerRegistry class is accessed
from multiple async contexts (in the async reconnect paths at lines 24, 45, and
57) and mutated in the `disconnect()` method on different threads, but it is not
declared as volatile. This allows the field's value to become stale across
threads, potentially permitting reconnect attempts after shutdown. Declare the
`running` field as volatile to ensure thread-safe visibility of all mutations
across the async reconnect code paths and the disconnect method.
In `@src/main/java/tf/tuff/tuffactions/swimming/Swimming.java`:
- Around line 133-145: The no-argument maintainSwimmingState() method is unused
and can be safely removed. Delete the entire method definition (including the
method signature, iterator loop, and all logic within it) since all swimming
state maintenance is handled by the overloaded maintainSwimmingState(Player)
variant that is called within the SchedulerCompat.runEntity task in the
per-player maintenance logic.
In `@src/main/java/tf/tuff/y0/Y0Plugin.java`:
- Around line 847-860: In the sendToNearbyPlayers method, the current
implementation schedules an entity task for every single online player, then
performs distance/world/online checks inside those tasks. Move the distance and
world filtering checks into the global SchedulerCompat.runGlobal task (before
the inner entity scheduling loop) to filter out players that are not nearby or
in the same world without creating unnecessary scheduled tasks. Additionally,
add a readiness check before scheduling each per-player entity task to avoid
scheduling tasks for players that are not ready to receive plugin messages,
thereby reducing scheduler overhead and network churn during frequent
block/light updates.
---
Outside diff comments:
In `@src/main/java/tf/tuff/y0/Y0Plugin.java`:
- Around line 425-436: The processAndSendChunk method checks if the player is
online at the start but does not verify the player remains in the chunk's world
before sending cached chunk payloads. If a player switches worlds between the
initial validation and the actual packet sending, stale chunk data from the old
world could be delivered. Add an additional check right before the loop that
sends payloads (where SchedulerCompat.sendPluginMessage is called) to verify
that player.getWorld() equals c.getWorld() to ensure the player is still in the
correct world, and only proceed with sending the chunk data if this condition is
true.
---
Nitpick comments:
In `@src/main/java/tf/tuff/tuffactions/swimming/Swimming.java`:
- Around line 43-46: The enable method in the Swimming class is an empty
override that only delegates to the parent class implementation without adding
any custom behavior. Remove the entire enable method override from the Swimming
class, as the parent class implementation will be invoked automatically when the
method is not overridden.
In `@src/main/java/tf/tuff/util/SchedulerCompat.java`:
- Around line 75-82: The sendPluginMessage method validates player and payload
parameters for null in the initial guard clause but is missing a null check for
the channel parameter. Add channel to the existing null validation in the early
return statement (alongside the existing checks for player and payload) to
prevent a potential NPE when player.sendPluginMessage is called inside the
runEntity task.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f8367aec-3a04-465c-8bf9-cd3da05302a0
⛔ Files ignored due to path filters (1)
builds/TuffXPlus-1.0.1-beta.jaris excluded by!**/*.jar
📒 Files selected for processing (15)
src/main/java/tf/tuff/ServerRegistry.javasrc/main/java/tf/tuff/TuffX.javasrc/main/java/tf/tuff/netty/ChunkHandler.javasrc/main/java/tf/tuff/tuffactions/creative/CreativeMenu.javasrc/main/java/tf/tuff/tuffactions/swimming/Swimming.javasrc/main/java/tf/tuff/util/SchedulerCompat.javasrc/main/java/tf/tuff/viablocks/CustomBlockListener.javasrc/main/java/tf/tuff/viablocks/PaletteManager.javasrc/main/java/tf/tuff/viablocks/ViaBlocksPlugin.javasrc/main/java/tf/tuff/viaentities/EntityDataHandler.javasrc/main/java/tf/tuff/viaentities/EntityInjector.javasrc/main/java/tf/tuff/viaentities/ViaEntitiesPlugin.javasrc/main/java/tf/tuff/y0/ChunkPacketListener.javasrc/main/java/tf/tuff/y0/ViaBlockIds.javasrc/main/java/tf/tuff/y0/Y0Plugin.java
✅ Files skipped from review due to trivial changes (1)
- src/main/java/tf/tuff/y0/ViaBlockIds.java
🚧 Files skipped from review as they are similar to previous changes (3)
- src/main/java/tf/tuff/TuffX.java
- src/main/java/tf/tuff/viablocks/ViaBlocksPlugin.java
- src/main/java/tf/tuff/netty/ChunkHandler.java
UplandJacob
requested changes
Jun 14, 2026
Contributor
Author
|
Fixed the remaining review items, tightened the CI test bootstrap for MockBukkit and missing ViaVersion state, cleaned up the Y0 and scheduler guards, and ./gradlew test build passes on this branch now. |
There was a problem hiding this comment.
Warning
CodeRabbit couldn't request changes on this pull request because it doesn't have sufficient GitHub permissions.
Please grant CodeRabbit Pull requests: Read and write permission and re-run the review.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/tf/tuff/tuffactions/swimming/Swimming.java (1)
101-123:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMove viewer-side swim updates onto the viewer scheduler.
broadcastSwimState()is reached fromhandleSwimState(),handlePlayerQuit(), and the maintenance timer, but therecipient.canSee(subject)check andactsPlugin.sendPluginMessage(...)still run on that caller's thread. In this Folia-focused PR, that means swimming updates can still touch arbitrary viewers from the wrong execution context even though the rest of the stack was migrated toSchedulerCompat. Please route each recipient branch throughSchedulerCompat.runEntity(...)/SchedulerCompat.sendPluginMessage(...)so the visibility check and plugin-message send both happen on the recipient's scheduler.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/tf/tuff/tuffactions/swimming/Swimming.java` around lines 101 - 123, The recipient visibility check and plugin message sending in broadcastSwimState() and sendSwimState() currently execute on the caller's thread, which is incompatible with Folia's scheduler requirements. Wrap the visibility check (recipient.canSee(subject)) and the sendSwimState() call within broadcastSwimState() using SchedulerCompat.runEntity() for each recipient player, ensuring both the visibility determination and the plugin message transmission in sendSwimState() happen on the recipient's scheduler context rather than the caller's thread.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/java/tf/tuff/TuffX.java`:
- Around line 68-70: The issue is that PacketEvents.getAPI() is called without
null validation in the onEnable method at line 68, and the packetEventsEnabled
flag is only reset conditionally inside a null check in the onDisable method at
line 125. This creates a race condition where if PacketEvents.getAPI() becomes
null between cycles, the flag won't reset, causing a potential NPE on the next
enable cycle. Add a defensive null check for PacketEvents.getAPI() before
calling init() in the onEnable method around line 68-70. Additionally, move the
packetEventsEnabled flag reset statement outside of the null check conditional
in the onDisable method around line 125, so the flag is always reset regardless
of whether the API is null.
---
Outside diff comments:
In `@src/main/java/tf/tuff/tuffactions/swimming/Swimming.java`:
- Around line 101-123: The recipient visibility check and plugin message sending
in broadcastSwimState() and sendSwimState() currently execute on the caller's
thread, which is incompatible with Folia's scheduler requirements. Wrap the
visibility check (recipient.canSee(subject)) and the sendSwimState() call within
broadcastSwimState() using SchedulerCompat.runEntity() for each recipient
player, ensuring both the visibility determination and the plugin message
transmission in sendSwimState() happen on the recipient's scheduler context
rather than the caller's thread.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4101cc4d-55d9-4684-8264-56b328a9af64
📒 Files selected for processing (10)
src/main/java/tf/tuff/ServerRegistry.javasrc/main/java/tf/tuff/TuffX.javasrc/main/java/tf/tuff/netty/ChunkHandler.javasrc/main/java/tf/tuff/tuffactions/TuffActions.javasrc/main/java/tf/tuff/tuffactions/swimming/Swimming.javasrc/main/java/tf/tuff/util/SchedulerCompat.javasrc/main/java/tf/tuff/viablocks/ViaBlocksPlugin.javasrc/main/java/tf/tuff/y0/ViaBlockIds.javasrc/main/java/tf/tuff/y0/Y0Plugin.javasrc/test/java/tf/tuff/TuffXTest.java
💤 Files with no reviewable changes (1)
- src/main/java/tf/tuff/viablocks/ViaBlocksPlugin.java
✅ Files skipped from review due to trivial changes (1)
- src/test/java/tf/tuff/TuffXTest.java
🚧 Files skipped from review as they are similar to previous changes (4)
- src/main/java/tf/tuff/y0/ViaBlockIds.java
- src/main/java/tf/tuff/util/SchedulerCompat.java
- src/main/java/tf/tuff/netty/ChunkHandler.java
- src/main/java/tf/tuff/y0/Y0Plugin.java
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
2 participants
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.
This fixes the async block read path, the broken section block update decoding below Y0, the unreliable Viablocks chunk send path, and the swim state regression. It also cuts cache churn when the relevant features are off or no Tuff players are active.
Closes #21
Closes #14
Closes #8
Closes #5
Refs #17
Refs #30
Summary by CodeRabbit
SchedulerCompatutilities, including safe scheduling and plugin-message sending helpers.ChunkHandlerTestfor packed position decoding (including negative coordinates) and updated JUnit 5 test setup.