fix(review): address review comments and add .pr-review.json for pragent

This commit is contained in:
Marcos Paulo
2026-08-20 10:14:18 -03:00
parent d93711e5c1
commit c3c4906e11
4 changed files with 125 additions and 43 deletions
+21
View File
@@ -0,0 +1,21 @@
{
"languages": [
"java",
"yaml",
"markdown"
],
"focus": [
"thread-safety",
"paper-chunk-ticketing",
"item-loss-prevention",
"resource-cleanup",
"performance-and-chunk-loading",
"null-safety-and-unit-tests"
],
"exclude_paths": [
"target/**",
"*.bak*",
"docs/**"
],
"instructions": "Canalhandia is a Minecraft Paper 1.21.x server plugin written in modern Java 25. Enforce these core invariants:\n1. Thread Safety: Bukkit API, World, Entity, and Inventory mutations MUST run on the main server thread. Async threads only do pure calculation or async file I/O.\n2. Item & Inventory Safety: Never discard player items. Always handle full-inventory overflow by dropping excess items at the player's location. On multi-step container placement (e.g. chests), roll back blocks if not all items fit.\n3. Lifecycle & Cleanup: All registered chunk tickets, recipes, schedulers, and I/O executors must be cleanly flushed and unloaded in onDisable() and module toggles.\n4. Chunk Loading: Never trigger synchronous chunk generation or loading inside event handlers. Always check world.isChunkLoaded() before querying blocks.\n5. Test Coverage: All domain logic, coordinates math, parsers, and pure helpers must have corresponding JUnit tests in src/test/java."
}
@@ -125,11 +125,14 @@ final class ChunkLoaderListener implements Listener {
event.setDropItems(false); event.setDropItems(false);
event.setExpToDrop(0); event.setExpToDrop(0);
if (player.getGameMode() != GameMode.CREATIVE) { if (player.getGameMode() == GameMode.CREATIVE) {
Msg.ok(player, "Âncora de Chunk #" + loader.id() + " desativada (Modo Criativo).");
} else {
Map<Integer, ItemStack> overflow = player.getInventory().addItem(ChunkAnchorItem.create(plugin, 1)); Map<Integer, ItemStack> overflow = player.getInventory().addItem(ChunkAnchorItem.create(plugin, 1));
for (ItemStack drop : overflow.values()) { for (ItemStack drop : overflow.values()) {
player.getWorld().dropItemNaturally(player.getLocation(), drop); player.getWorld().dropItemNaturally(player.getLocation(), drop);
} }
Msg.ok(player, "Âncora de Chunk #" + loader.id() + " desativada e recolhida para o seu inventário.");
} }
try { try {
@@ -139,11 +142,13 @@ final class ChunkLoaderListener implements Listener {
} }
plugin.blueMap().syncChunkLoaders(); plugin.blueMap().syncChunkLoaders();
Msg.ok(player, "Âncora de Chunk #" + loader.id() + " desativada e recolhida para o seu inventário.");
} }
@EventHandler(priority = EventPriority.HIGH, ignoreCancelled = true) @EventHandler(priority = EventPriority.HIGH, ignoreCancelled = true)
public void onPlayerInteract(PlayerInteractEvent event) { public void onPlayerInteract(PlayerInteractEvent event) {
if (!plugin.settings().moduleEnabled(Module.CHUNKLOADER)) {
return;
}
if (event.getAction() != Action.RIGHT_CLICK_BLOCK) { if (event.getAction() != Action.RIGHT_CLICK_BLOCK) {
return; return;
} }
@@ -1,7 +1,5 @@
package dev.marcospaulo.canalhandia; package dev.marcospaulo.canalhandia;
import net.kyori.adventure.text.Component;
import net.kyori.adventure.text.format.NamedTextColor;
import org.bukkit.HeightMap; import org.bukkit.HeightMap;
import org.bukkit.Location; import org.bukkit.Location;
import org.bukkit.Material; import org.bukkit.Material;
@@ -9,15 +7,13 @@ import org.bukkit.World;
import org.bukkit.block.Block; import org.bukkit.block.Block;
import org.bukkit.block.BlockState; import org.bukkit.block.BlockState;
import org.bukkit.block.Chest; import org.bukkit.block.Chest;
import org.bukkit.block.DoubleChest;
import org.bukkit.entity.Player;
import org.bukkit.event.entity.EntityDamageEvent; import org.bukkit.event.entity.EntityDamageEvent;
import org.bukkit.inventory.Inventory; import org.bukkit.inventory.Inventory;
import org.bukkit.inventory.ItemStack; import org.bukkit.inventory.ItemStack;
import java.util.ArrayList; import java.util.ArrayList;
import java.util.Collections;
import java.util.List; import java.util.List;
import java.util.Map;
/** /**
* Rescues player items on void death, either into a safe nearby chest * Rescues player items on void death, either into a safe nearby chest
@@ -25,6 +21,8 @@ import java.util.List;
*/ */
public final class VoidProtection { public final class VoidProtection {
private static final int MAX_CHECKED_COLUMNS = 120;
private VoidProtection() { private VoidProtection() {
} }
@@ -40,7 +38,7 @@ public final class VoidProtection {
/** /**
* Finds the nearest safe solid block with air space above it within the given horizontal radius. * Finds the nearest safe solid block with air space above it within the given horizontal radius.
* Searches in an expanding spiral from the death coordinate. * Only checks already-loaded chunks and caps checked columns to prevent server hitches.
*/ */
public static Location findSafeChestLocation(World world, Location deathLoc, int radius) { public static Location findSafeChestLocation(World world, Location deathLoc, int radius) {
if (world == null || deathLoc == null || radius <= 0) { if (world == null || deathLoc == null || radius <= 0) {
@@ -49,19 +47,29 @@ public final class VoidProtection {
int centerX = deathLoc.getBlockX(); int centerX = deathLoc.getBlockX();
int centerZ = deathLoc.getBlockZ(); int centerZ = deathLoc.getBlockZ();
int minHeight = world.getMinHeight(); int minHeight = world.getMinHeight();
int checked = 0;
Location best = null; for (int r = 0; r <= radius; r += (r > 8 ? 2 : 1)) {
double bestDistSq = Double.MAX_VALUE; Location ringBest = null;
double ringBestDistSq = Double.MAX_VALUE;
for (int r = 0; r <= radius; r++) { for (int dx = -r; dx <= r; dx += (r > 8 ? 2 : 1)) {
for (int dx = -r; dx <= r; dx++) { for (int dz = -r; dz <= r; dz += (r > 8 ? 2 : 1)) {
for (int dz = -r; dz <= r; dz++) { if (r > 0 && Math.abs(dx) != r && Math.abs(dz) != r) {
if (Math.abs(dx) != r && Math.abs(dz) != r) { continue;
continue; // Only check perimeter of current radius
} }
if (++checked > MAX_CHECKED_COLUMNS) {
return ringBest;
}
int x = centerX + dx; int x = centerX + dx;
int z = centerZ + dz; int z = centerZ + dz;
// Do not load or generate new chunks synchronously on death
if (!world.isChunkLoaded(x >> 4, z >> 4)) {
continue;
}
int topY; int topY;
try { try {
topY = world.getHighestBlockYAt(x, z, HeightMap.MOTION_BLOCKING_NO_LEAVES); topY = world.getHighestBlockYAt(x, z, HeightMap.MOTION_BLOCKING_NO_LEAVES);
@@ -78,24 +86,21 @@ public final class VoidProtection {
if (isSafeGround(ground) && isReplaceable(space)) { if (isSafeGround(ground) && isReplaceable(space)) {
double distSq = (dx * dx) + (dz * dz); double distSq = (dx * dx) + (dz * dz);
if (distSq < bestDistSq) { if (distSq < ringBestDistSq) {
bestDistSq = distSq; ringBestDistSq = distSq;
best = space.getLocation(); ringBest = space.getLocation();
if (r == 0) {
return best;
} }
} }
} }
} }
} if (ringBest != null) {
if (best != null) { return ringBest;
return best;
} }
} }
return best; return null;
} }
private static boolean isSafeGround(Block block) { static boolean isSafeGround(Block block) {
if (block == null) { if (block == null) {
return false; return false;
} }
@@ -103,13 +108,12 @@ public final class VoidProtection {
if (mat.isAir() || !mat.isSolid()) { if (mat.isAir() || !mat.isSolid()) {
return false; return false;
} }
// Avoid placing on hazardous blocks
return mat != Material.LAVA && mat != Material.FIRE && mat != Material.SOUL_FIRE return mat != Material.LAVA && mat != Material.FIRE && mat != Material.SOUL_FIRE
&& mat != Material.CACTUS && mat != Material.MAGMA_BLOCK && mat != Material.SWEET_BERRY_BUSH && mat != Material.CACTUS && mat != Material.MAGMA_BLOCK && mat != Material.SWEET_BERRY_BUSH
&& mat != Material.WITHER_ROSE && mat != Material.POWDER_SNOW; && mat != Material.WITHER_ROSE && mat != Material.POWDER_SNOW;
} }
private static boolean isReplaceable(Block block) { static boolean isReplaceable(Block block) {
if (block == null) { if (block == null) {
return false; return false;
} }
@@ -120,18 +124,24 @@ public final class VoidProtection {
/** /**
* Stores items into a chest (and an adjacent chest if needed) at the target location. * Stores items into a chest (and an adjacent chest if needed) at the target location.
* Returns true if items were successfully stored. * All items must be stored without overflow; on any failure, blocks are rolled back
* and false is returned so caller can safely fall back to keepInventory.
*/ */
public static boolean rescueToChest(List<ItemStack> items, Location chestLoc) { public static boolean rescueToChest(List<ItemStack> items, Location chestLoc) {
if (items == null || items.isEmpty() || chestLoc == null) { if (items == null || items.isEmpty() || chestLoc == null) {
return false; return false;
} }
try {
Block chestBlock = chestLoc.getBlock();
chestBlock.setType(Material.CHEST, false);
Block chestBlock = chestLoc.getBlock();
Material orig1 = chestBlock.getType();
Block adjacent = null;
Material orig2 = null;
try {
chestBlock.setType(Material.CHEST, false);
BlockState state = chestBlock.getState(); BlockState state = chestBlock.getState();
if (!(state instanceof Chest chest)) { if (!(state instanceof Chest chest)) {
chestBlock.setType(orig1, false);
return false; return false;
} }
@@ -140,27 +150,62 @@ public final class VoidProtection {
for (ItemStack item : items) { for (ItemStack item : items) {
if (item != null && !item.getType().isAir()) { if (item != null && !item.getType().isAir()) {
var leftover = inv.addItem(item.clone()); Map<Integer, ItemStack> leftover = inv.addItem(item.clone());
remaining.addAll(leftover.values()); remaining.addAll(leftover.values());
} }
} }
// If items didn't fit in a single chest (27 slots), try expanding to an adjacent double chest
if (!remaining.isEmpty()) { if (!remaining.isEmpty()) {
Block adjacent = findAdjacentSpace(chestLoc); adjacent = findAdjacentSpace(chestLoc);
if (adjacent != null) { if (adjacent == null) {
// Cannot fit all items and no space for second chest -> rollback
inv.clear();
chestBlock.setType(orig1, false);
return false;
}
orig2 = adjacent.getType();
adjacent.setType(Material.CHEST, false); adjacent.setType(Material.CHEST, false);
BlockState adjState = adjacent.getState(); BlockState adjState = adjacent.getState();
if (adjState instanceof Chest adjChest) { if (!(adjState instanceof Chest adjChest)) {
inv.clear();
chestBlock.setType(orig1, false);
adjacent.setType(orig2, false);
return false;
}
Inventory adjInv = adjChest.getInventory(); Inventory adjInv = adjChest.getInventory();
List<ItemStack> secondLeftover = new ArrayList<>();
for (ItemStack rem : remaining) { for (ItemStack rem : remaining) {
adjInv.addItem(rem); Map<Integer, ItemStack> leftover = adjInv.addItem(rem);
} secondLeftover.addAll(leftover.values());
} }
if (!secondLeftover.isEmpty()) {
// Still overflowed double chest -> rollback everything
inv.clear();
adjInv.clear();
chestBlock.setType(orig1, false);
adjacent.setType(orig2, false);
return false;
} }
} }
return true; return true;
} catch (Exception e) { } catch (Exception e) {
try {
if (chestBlock.getType() == Material.CHEST) {
BlockState s = chestBlock.getState();
if (s instanceof Chest c) c.getInventory().clear();
chestBlock.setType(orig1, false);
}
if (adjacent != null && adjacent.getType() == Material.CHEST) {
BlockState s2 = adjacent.getState();
if (s2 instanceof Chest c2) c2.getInventory().clear();
adjacent.setType(orig2 != null ? orig2 : Material.AIR, false);
}
} catch (Exception ignored) {
}
return false; return false;
} }
} }
@@ -1,8 +1,11 @@
package dev.marcospaulo.canalhandia; package dev.marcospaulo.canalhandia;
import org.bukkit.event.entity.EntityDamageEvent; import org.bukkit.event.entity.EntityDamageEvent;
import org.bukkit.inventory.ItemStack;
import org.junit.jupiter.api.Test; import org.junit.jupiter.api.Test;
import java.util.List;
import static org.junit.jupiter.api.Assertions.*; import static org.junit.jupiter.api.Assertions.*;
class VoidProtectionTest { class VoidProtectionTest {
@@ -24,6 +27,14 @@ class VoidProtectionTest {
@Test @Test
void handlesNullWorldOrLocationsGracefully() { void handlesNullWorldOrLocationsGracefully() {
assertNull(VoidProtection.findSafeChestLocation(null, null, 10)); assertNull(VoidProtection.findSafeChestLocation(null, null, 10));
assertNull(VoidProtection.findSafeChestLocation(null, null, 0));
assertFalse(VoidProtection.rescueToChest(null, null)); assertFalse(VoidProtection.rescueToChest(null, null));
assertFalse(VoidProtection.rescueToChest(List.of(), null));
}
@Test
void nullBlocksAreNeitherSafeNorReplaceable() {
assertFalse(VoidProtection.isSafeGround(null));
assertFalse(VoidProtection.isReplaceable(null));
} }
} }