Kryptonite 2.1.0 #58
Kryptonite 2.1.0 #58
Conversation
Reset NU
|
Thank you for opening a pull request to Essence. You've requested to merge something into We unfortunately do not accept any pull requests into Thank you! |
Dependency ReviewThe following issues were found:
License Issuespom.xml
OpenSSF Scorecard
Scanned Files
|
WalkthroughAdds a typed config framework and ConfigCollection implementations, replaces many legacy config classes/GUI with a unified KOS_ConfigItemGui, migrates logging/permissions to Foundry, removes Metrics and numerous legacy GUIs/utilities, and updates build/pom, repositories, and plugin metadata. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Server as Bukkit Server
participant P as Kryptonite
participant F as FoundryConfig
participant L as Foundry Logger
participant SD as SoftwareUtil
Server->>P: onEnable()
P->>F: new FoundryConfig()
P->>L: new Logger(F)
P->>SD: checkSoftware()
SD-->>P: supported options (MINECRAFT, LEAF, ...)
P-->>Server: startup complete
note right of L: Logging via Foundry Logger
sequenceDiagram
autonumber
actor Admin as Player/Admin
participant GUI as KOS_ConfigItemGui
participant CC as ConfigCollection
participant Item as GenericConfigItem<T>
participant IO as YAML/.properties
Admin->>GUI: open(type = MINECRAFT/LEAF)
GUI->>CC: instantiate collection
loop for each item
GUI->>Item: getValue()
Item->>IO: load/read
IO-->>Item: value
Item-->>GUI: metadata (value/isValid/isIdeal)
end
Admin->>GUI: click -> change value
GUI->>Item: setValue(new)
Item->>IO: write/save
GUI->>Admin: refresh
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Potential hotspots:
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (9)
src/main/java/net/lewmc/kryptonite/kos/gui/KOS_PaperWorld_2.java (1)
139-146: Bug: wrong key used for Wither Skull save limit.Reads TRIDENT instead of WITHER_SKULL; GUI shows/edits inconsistent value.
- int value = this.paperWorld.getInt(PaperWorld.Key.ENTITY_PER_CHUNK_SAVE_LIMIT_TRIDENT); + int value = this.paperWorld.getInt(PaperWorld.Key.ENTITY_PER_CHUNK_SAVE_LIMIT_WITHER_SKULL);src/main/java/net/lewmc/kryptonite/kos/gui/KOS_BukkitGui.java (1)
404-437: Range checks: “too low” thresholds use 5 but ideal range starts at 100.Else-if branches should use
< 100to classify “too low”; current logic mislabels 5–99 as “too high”.- } else if (value < 5) { + } else if (value < 100) {(Apply to all five methods above.)
Also applies to: 439-472, 474-507, 509-542, 544-577
src/main/java/net/lewmc/kryptonite/kos/gui/KOS_PaperWorld_1.java (1)
102-145: Bugs: String comparison and unsafe cast in delayChunkUnloadsBy.
- Use
.equalsfor String comparison.- Don’t cast String to int; handle Integer or parse String safely.
- if (value instanceof String && value == "default") { + if ("default".equals(value)) { ... - } else if (value instanceof String && value != "default") { - this.gui.addElement(new StaticGuiElement(id, - new ItemStack(Material.WHITE_CONCRETE), - 1, - click -> this.setInt(PaperWorld.Key.DELAY_CHUNK_UNLOADS_BY, click, (int) value, true), + } else if (value instanceof Integer) { + this.gui.addElement(new StaticGuiElement(id, + new ItemStack(Material.WHITE_CONCRETE), + 1, + click -> this.setInt(PaperWorld.Key.DELAY_CHUNK_UNLOADS_BY, click, (Integer) value, true), ChatColor.WHITE + "Delay Chunk Unloads By", ChatColor.GRAY + String.valueOf(value), ChatColor.GRAY + "'default' (lowest value) is ideal for most servers.", ChatColor.BLUE + "Right click to increase - left click to decrease." )); + } else if (value instanceof String) { + try { + int parsed = Integer.parseInt((String) value); + this.gui.addElement(new StaticGuiElement(id, + new ItemStack(Material.WHITE_CONCRETE), + 1, + click -> this.setInt(PaperWorld.Key.DELAY_CHUNK_UNLOADS_BY, click, parsed, true), + ChatColor.WHITE + "Delay Chunk Unloads By", + ChatColor.GRAY + String.valueOf(value), + ChatColor.GRAY + "'default' (lowest value) is ideal for most servers.", + ChatColor.BLUE + "Right click to increase - left click to decrease." + )); + } catch (NumberFormatException ignored) { + // fall through to barrier below + }src/main/java/net/lewmc/kryptonite/kos/gui/KOS_PaperWorld_5.java (1)
75-89: Hotkey collision with KOS_GuiConstants; Exit ('v') is overridden by “Previous page”.
KOS_GuiConstants now uses t/u/v; this page still uses 'v' for “Previous page” and the layout lacks 't'/'u'. Result: Exit is hidden and Help/Alerts (restartRequired=true) won’t render.Apply this to align with constants and avoid collisions:
- this.gui.addElement(new StaticGuiElement('v', + this.gui.addElement(new StaticGuiElement('y', new ItemStack(Material.OAK_SIGN), 1, click -> { click.getGui().close(); KOS_PaperWorld_4 prevGui = new KOS_PaperWorld_4(this.plugin, this.user); prevGui.show(); return true; }, ChatColor.WHITE + "Previous page" ));- return new String[]{ - "abcdefghi", - "jklmnopqr", - "v w x y z" - }; + return new String[]{ + "abcdefghi", + "jklmnopqr", + "t x v y z" + };Also applies to: 95-102
src/main/java/net/lewmc/kryptonite/kos/gui/KOS_PaperWorld_3.java (1)
85-108: Hotkey mismatch with KOS_GuiConstants; Exit/help/alerts may not render; 'v' conflict.
Constants moved to t/u/v; this page still uses 'v' for Previous and lacks 't'/'u' in layout.- this.gui.addElement(new StaticGuiElement('v', + this.gui.addElement(new StaticGuiElement('y', new ItemStack(Material.OAK_SIGN), ... - return new String[]{ - "abcdefghi", - "jklmnopqr", - "v w x y z" - }; + return new String[]{ + "abcdefghi", + "jklmnopqr", + "t x v y z" + };Also applies to: 116-121
src/main/java/net/lewmc/kryptonite/kos/gui/KOS_PufferfishGui.java (2)
72-79: Constants layout out-of-sync (missing t/u/v).
With KOS_GuiConstants using t/u/v, this GUI won’t show Help/Alerts(true)/Exit unless slots exist.- return new String[]{ - "abcdefgh ", - " ", - " w x y " - }; + return new String[]{ + "abcdefgh ", + " ", + " t x v " + };
118-150: Toggle handlers set the same value in both branches; options cannot be turned off.
E.g., when DAB is true, clicking sets true again.Apply these fixes to toggle correctly:
@@ - if (value) { + if (value) { this.gui.addElement(new StaticGuiElement(id, new ItemStack(Material.LIME_CONCRETE), 1, click -> { click.getGui().close(); - this.pufferfish.setBoolean(Pufferfish.Key.DAB_ENABLED, true); + this.pufferfish.setBoolean(Pufferfish.Key.DAB_ENABLED, false); this.show(); return true; }, @@ - } else { + } else { this.gui.addElement(new StaticGuiElement(id, new ItemStack(Material.RED_CONCRETE), 1, click -> { click.getGui().close(); this.pufferfish.setBoolean(Pufferfish.Key.DAB_ENABLED, true); this.show(); return true; },@@ - this.pufferfish.setBoolean(Pufferfish.Key.ENABLE_ASYNC_MOB_SPAWNING, true); + this.pufferfish.setBoolean(Pufferfish.Key.ENABLE_ASYNC_MOB_SPAWNING, false); @@ - this.pufferfish.setBoolean(Pufferfish.Key.ENABLE_SUFFOCATION_OPTIMIZATION, true); + this.pufferfish.setBoolean(Pufferfish.Key.ENABLE_SUFFOCATION_OPTIMIZATION, false); @@ - this.pufferfish.setBoolean(Pufferfish.Key.INACTIVE_GOAL_SELECTOR_THROTTLE, true); + this.pufferfish.setBoolean(Pufferfish.Key.INACTIVE_GOAL_SELECTOR_THROTTLE, false); @@ - this.pufferfish.setBoolean(Pufferfish.Key.DISABLE_METHOD_PROFILER, true); + this.pufferfish.setBoolean(Pufferfish.Key.DISABLE_METHOD_PROFILER, false);Also applies to: 229-261, 264-296, 299-333, 335-369
src/main/java/net/lewmc/kryptonite/kos/gui/KOS_GuiConstants.java (1)
31-44: Hotkey migration breaks existing GUIs; provide backward‑compat or update all layouts.
Help moved to 't', Alerts(true) to 'u', Exit to 'v', but many GUIs still place these at w/x/y, and some use 'v' for navigation.Two options:
- Short term (recommended): add elements at both old and new keys to avoid regressions.
- Long term: update all GUIs’ layouts to include t/u/v and stop using 'v' for navigation.
Back‑compat patch:
- this.gui.addElement(new StaticGuiElement('t', + this.gui.addElement(new StaticGuiElement('t', new ItemStack(Material.BOOK), ... + // Legacy position + this.gui.addElement(new StaticGuiElement('w', + new ItemStack(Material.BOOK), 1, click -> { + click.getWhoClicked().sendMessage(ChatColor.YELLOW + "https://wiki.lewmc.net/kr-kos-guide.html"); + click.getGui().close(); + return true; + }, + ChatColor.DARK_GREEN + "Help", + ChatColor.GREEN + "Not sure what to do? Click here for", + ChatColor.GREEN + "a link to our Wiki for help." + ));- if (plugin.restartRequired) { - this.gui.addElement(new StaticGuiElement('u', + if (plugin.restartRequired) { + this.gui.addElement(new StaticGuiElement('u', new ItemStack(Material.YELLOW_CONCRETE), ... - } else { + // Legacy position for alerts + this.gui.addElement(new StaticGuiElement('x', + new ItemStack(Material.YELLOW_CONCRETE), 1, click -> true, + ChatColor.GOLD + "Alerts", + ChatColor.YELLOW + "Restart Required - You must restart your", + ChatColor.YELLOW + "server for changes to be applied." + )); + } else { this.gui.addElement(new StaticGuiElement('x',- this.gui.addElement(new StaticGuiElement('v', + this.gui.addElement(new StaticGuiElement('v', new ItemStack(Material.OAK_DOOR), ... ChatColor.DARK_RED + "Exit" )); + // Legacy position + this.gui.addElement(new StaticGuiElement('y', + new ItemStack(Material.OAK_DOOR), 1, click -> { click.getGui().close(); return true; }, + ChatColor.DARK_RED + "Exit" + ));Also applies to: 45-54, 63-72
src/main/java/net/lewmc/kryptonite/utils/UpdateUtil.java (1)
35-61: Update check does blocking I/O without timeouts and leaks the Scanner.
url.openStream()has no connect/read timeouts and can hang the server duringonEnable(). Also,Scannerisn’t closed. UseHttpURLConnectionwith timeouts and try‑with‑resources.Apply this diff:
--- a/src/main/java/net/lewmc/kryptonite/utils/UpdateUtil.java +++ b/src/main/java/net/lewmc/kryptonite/utils/UpdateUtil.java @@ import com.tchristofferson.configupdater.ConfigUpdater; import net.lewmc.foundry.Logger; import net.lewmc.kryptonite.Kryptonite; import java.io.File; import java.io.IOException; +import java.io.InputStream; import java.net.MalformedURLException; import java.net.URL; +import java.net.HttpURLConnection; +import java.net.SocketTimeoutException; +import java.nio.charset.StandardCharsets; import java.util.Scanner; @@ - Scanner s = new Scanner(url.openStream()); - if (s.hasNextLine()) { - String response = s.nextLine(); + HttpURLConnection conn = (HttpURLConnection) url.openConnection(); + conn.setRequestMethod("GET"); + conn.setConnectTimeout(3500); + conn.setReadTimeout(4000); + conn.setRequestProperty("User-Agent", "Kryptonite/" + this.plugin.getDescription().getVersion()); + try (InputStream is = conn.getInputStream(); + Scanner s = new Scanner(is, StandardCharsets.UTF_8)) { + if (s.hasNextLine()) { + String response = s.nextLine(); if (response.isEmpty()) { log.severe("Unable to perform update check: There was no response from the server."); this.log.info(""); } else if (response.equals(this.plugin.getDescription().getVersion())) { log.info("You are running the latest version of Kryptonite."); this.log.info(""); } else { this.plugin.updatePending = true; log.warn("UPDATE > There's a new version of Kryptonite available."); log.warn("UPDATE > Your version: "+this.plugin.getDescription().getVersion()+" - latest version: "+response); log.warn("UPDATE > You can download the latest version from lewmc.net/kryptonite"); this.log.info(""); } - } else { - log.severe("Unable to perform update check: There was no response from the server."); - this.log.info(""); - } + } else { + log.severe("Unable to perform update check: There was no response from the server."); + this.log.info(""); + } + } catch (SocketTimeoutException e) { + log.warn("Unable to perform update check: timeout - " + e); + this.log.info(""); + }
🧹 Nitpick comments (50)
pom.xml (1)
127-132: SNAPSHOT dependency on FoundryUsing 1.3.0-SNAPSHOT on main can reduce reproducibility. Prefer a released version or pin via a property for easy updates.
- <dependency> - <groupId>net.lewmc</groupId> - <artifactId>foundry</artifactId> - <version>1.3.0-SNAPSHOT</version> - <scope>compile</scope> - </dependency> + <properties> + <foundry.version>1.3.0</foundry.version> + </properties> + <dependency> + <groupId>net.lewmc</groupId> + <artifactId>foundry</artifactId> + <version>${foundry.version}</version> + <scope>compile</scope> + </dependency>src/main/java/net/lewmc/kryptonite/kos/config/PaperWorld.java (1)
10-13: Deprecation metadata: add reason/replacement and plan removalInclude since/forRemoval and point to the new config path to guide users.
-/** - * The PaperWorld class manages the paper-world-defaults.yml configuration file. - * @deprecated - */ -@Deprecated +/** + * The PaperWorld class manages the paper-world-defaults.yml configuration file. + * @deprecated since 2.1.0 — replaced by MinecraftConfig/KOS_ConfigItemGui; will be removed in a future release. + */ +@Deprecated(forRemoval = true, since = "2.1.0")src/main/java/net/lewmc/kryptonite/kos/config/Bukkit.java (1)
10-13: Deprecation metadata: add reason/replacement and plan removalMatch the project’s new config direction and include since/forRemoval.
-/** - * The Bukkit class manages the bukkit.yml configuration file. - * @deprecated - */ -@Deprecated +/** + * The Bukkit class manages the bukkit.yml configuration file. + * @deprecated since 2.1.0 — replaced by MinecraftConfig/KOS_ConfigItemGui; scheduled for removal. + */ +@Deprecated(forRemoval = true, since = "2.1.0")src/main/java/net/lewmc/kryptonite/kos/gui/KOS_SpigotGui_2.java (1)
18-21: Deprecation metadata: clarify path forwardAdd since/forRemoval and reference the new GUI (KOS_ConfigItemGui) to aid migration.
-/** - * KOS Spigot GUI (Page 2/2) - * @deprecated - */ -@Deprecated +/** + * KOS Spigot GUI (Page 2/2) + * @deprecated since 2.1.0 — superseded by KOS_ConfigItemGui (MinecraftConfig-backed). + */ +@Deprecated(forRemoval = true, since = "2.1.0")src/main/java/net/lewmc/kryptonite/kos/gui/KOS_PaperWorld_4.java (2)
18-21: Deprecation metadata: add reason/replacement and plan removalSame treatment as other deprecated classes.
-/** - * KOS Paper World GUI (Page 4/5) - * @deprecated - */ -@Deprecated +/** + * KOS Paper World GUI (Page 4/5) + * @deprecated since 2.1.0 — superseded by KOS_ConfigItemGui (MinecraftConfig-backed). + */ +@Deprecated(forRemoval = true, since = "2.1.0")
665-667: Minor UI string fixesSmall copy/paste glitches in disabled-text messages.
- ChatColor.GRAY + "The Alt Item Despawn Rate Jungle is disabled.", + ChatColor.GRAY + "The Alt Item Despawn Rate feature is disabled.", ... - ChatColor.GRAY + "The Alt Item Despawn Rate Jungle is disabled.", + ChatColor.GRAY + "The Alt Item Despawn Rate feature is disabled.",Also applies to: 800-802
src/main/java/net/lewmc/kryptonite/kos/config/Spigot.java (2)
10-13: Deprecation: add guidance.Include since/replacement to aid migration, e.g. “Use MinecraftConfig” and optionally
@Deprecated(since="2.1.0", forRemoval=true).
121-123: Typo in log method name (“veboseInfo”).Likely meant “verboseInfo”; current spelling risks compile/runtime errors depending on LogUtil.
Apply:
- log.veboseInfo("KOS>spigot.yml set '" + key + "' to '" + value + "'"); + log.verboseInfo("KOS>spigot.yml set '" + key + "' to '" + value + "'");(Repeat same fix for the other two occurrences.)
Also applies to: 147-149, 173-175
src/main/resources/plugin.yml (1)
21-21: YAML: space after comma in flow sequence.Conform to linter by adding a space.
- aliases: ['exploit','exploitdb'] + aliases: ['exploit', 'exploitdb']src/main/java/net/lewmc/kryptonite/kos/config/Purpur.java (2)
10-13: Deprecation: add guidance.Add
sinceand replacement link (e.g., new config framework) for consumers.
81-83: Typo in log method name (“veboseInfo”).Use “verboseInfo”.
- log.veboseInfo("KOS>purpur.yml set '" + key + "' to '" + value + "'"); + log.verboseInfo("KOS>purpur.yml set '" + key + "' to '" + value + "'");(Repeat for the other two.)
Also applies to: 107-109, 133-135
src/main/java/net/lewmc/kryptonite/kos/gui/KOS_PurpurGui.java (1)
95-100: Typo in user-facing text (“compatability”).Correct spelling.
- ChatColor.YELLOW + "has a known compatability issue and may not work correctly.", + ChatColor.YELLOW + "has a known compatibility issue and may not work correctly.",- ChatColor.GREEN + "has a known compatability issue and may not work correctly.", + ChatColor.GREEN + "has a known compatibility issue and may not work correctly.",Also applies to: 129-134
src/main/java/net/lewmc/kryptonite/kos/gui/KOS_BukkitGui.java (3)
369-401: Label copy/paste: says “Monster” in Animal spawns section.Fix three occurrences.
- ChatColor.DARK_GREEN + "Ticks Per Monster Spawns", + ChatColor.DARK_GREEN + "Ticks Per Animal Spawns", ... - ChatColor.DARK_RED + "Ticks Per Monster Spawns", + ChatColor.DARK_RED + "Ticks Per Animal Spawns", ... - ChatColor.GOLD + "Ticks Per Monster Spawns", + ChatColor.GOLD + "Ticks Per Animal Spawns",
236-240: Label copy/paste: “Water Underground Creature” section labeled as “Water Ambient”.Adjust the title in all three places.
- ChatColor.DARK_GREEN + "Spawn Limit (Water Ambient)", + ChatColor.DARK_GREEN + "Spawn Limit (Water Underground Creature)", ... - ChatColor.GOLD + "Spawn Limit (Water Ambient)", + ChatColor.GOLD + "Spawn Limit (Water Underground Creature)", ... - ChatColor.DARK_RED + "Spawn Limit (Water Ambient)", + ChatColor.DARK_RED + "Spawn Limit (Water Underground Creature)",Also applies to: 246-260
580-611: GC period thresholds: low check should match range floor.Ideal is 400–600; use
< 400for “too low” instead of< 5.- } else if (value < 5) { + } else if (value < 400) {src/main/java/net/lewmc/kryptonite/utils/PermissionUtil.java (2)
9-13: Deprecation noted—consider pointing to replacement.Add
@deprecatedJavadoc message with the new permission path or API.
14-21: Avoid null lookup; cast to Player directly.
commandSenderis already a Player here; current code risks NPE if lookup fails and is wasteful.- public boolean isOperator(CommandSender commandSender) { - if (commandSender instanceof Player) { - String player = commandSender.getName(); - return Objects.requireNonNull(Bukkit.getServer().getPlayer(player)).isOp(); - } else { - return true; - } - } + public boolean isOperator(CommandSender commandSender) { + if (commandSender instanceof Player) { + return ((Player) commandSender).isOp(); + } + return true; // console and non-player senders + }Also remove the now-unused
java.util.Objectsimport.src/main/java/net/lewmc/kryptonite/kos/gui/KOS_PaperWorld_1.java (1)
56-56: Typo: “Chucks” → “Chunks”.Rename method and call site.
- this.preventMovingIntoUnloadedChucks('c'); + this.preventMovingIntoUnloadedChunks('c');- private void preventMovingIntoUnloadedChucks(char id) { + private void preventMovingIntoUnloadedChunks(char id) {Also applies to: 172-172
src/main/java/net/lewmc/kryptonite/kos/gui/KOS_PaperWorld_5.java (1)
441-447: Misleading tooltip text for mob spawner tick rate.
“Items may despawn too quickly.” is unrelated to spawner tick rate.- ChatColor.RED + "Items may despawn too quickly.", + ChatColor.RED + "Too low may hurt performance or stall spawns.",src/main/java/net/lewmc/kryptonite/kos/gui/KOS_PaperWorld_3.java (2)
66-68: Duplicate slot key 'e' used twice; second element will overwrite the first.
Two elements share id 'e' (armorStandsTick / armorStandsDoCollisionEntityLookups).Consider assigning a unique id (e.g., 'r') and adding it to the layout if this page remains used despite deprecation.
451-459: Inconsistent “ideal” coloring (GREEN text with ORANGE item).
Visual cue is mixed (ideal range but orange concrete).Use GREEN_CONCRETE for the ideal branch to match other pages for these five sections.
Also applies to: 484-495, 519-530, 554-565, 589-600
src/main/java/net/lewmc/kryptonite/kos/gui/KOS_PufferfishGui.java (1)
43-49: Cast to Player without guard can NPE/CCE if sender is not a player.
If invoked from console, this will throw.- this.gui.show((Player) this.user); + if (this.user instanceof Player p) { + this.gui.show(p); + } else { + return; + }dependency-reduced-pom.xml (1)
71-80: New repositories/distributionManagement: confirm availability/creds in CI.
Avoid build breaks on release/snapshot deploys.Also applies to: 90-99
src/main/java/net/lewmc/kryptonite/kos/gui/KOS_SpigotGui_1.java (1)
19-22: Deprecation needs guidance (what to use instead and since when).Add a message and
sinceto both Javadoc and annotation.Apply this diff:
-/** - * KOS Spigot GUI (Page 1/2) - * @deprecated - */ -@Deprecated +/** + * KOS Spigot GUI (Page 1/2) + * @deprecated Since 2.1.0 — replaced by KOS_ConfigItemGui (Minecraft/Spigot pages unified). + */ +@Deprecated(forRemoval = true, since = "2.1.0")src/main/java/net/lewmc/kryptonite/edb/Check.java (1)
408-408: Gate switch to supportsMinecraft: LGTM. Minor label consistency nit.Else-branch currently logs expected
"MinecraftServer". Consider"Minecraft"to match the enum and other labels.Apply this diff:
- "MinecraftServer"); + "Minecraft");src/main/java/net/lewmc/kryptonite/utils/config/ConfigCollection.java (1)
3-3: Prefer Map interface and prevent reassignment.Use
Mapandfinalto avoid accidental field rebinds; behavior remains the same.Apply this diff:
import java.util.HashMap; +import java.util.Map; @@ - public HashMap<String, GenericConfigItem> values = new HashMap<>(); + public final Map<String, GenericConfigItem> values = new HashMap<>();Also applies to: 13-13
src/main/java/net/lewmc/kryptonite/Kryptonite.java (2)
40-44: Load config before reading “verbose”You read getConfig().getBoolean("verbose") before ensuring the config exists. Initialize defaults first to avoid silent false.
Apply:
public void onEnable() { - this.foundryConfig = new FoundryConfig(this); + this.saveDefaultConfig(); // ensure config exists before reading + this.foundryConfig = new FoundryConfig(this); this.foundryConfig.setVerbose(this.getConfig().getBoolean("verbose")); this.foundryConfig.setPluginId("kr"); this.log = new Logger(this.foundryConfig);Optional cleanup (since called above):
- this.saveDefaultConfig();
132-134: Log message wording: prefer “Minecraft” to match enumKeep user-facing text consistent with MINECRAFT.
- supportedConfigs = supportedConfigs + "Server Properties"; + supportedConfigs = supportedConfigs + "Minecraft";src/main/java/net/lewmc/kryptonite/kos/AutoKOS.java (1)
94-96: Message consistencyUpdate wording to reflect the new gate.
- this.log.info("[KOS] 1/6 - Server does not support Server Properties, skipping..."); + this.log.info("[KOS] 1/6 - Server does not support Minecraft (server.properties), skipping...");src/main/java/net/lewmc/kryptonite/config/LeafConfig.java (5)
31-37: Rename “Parallel World Tracing” → “Parallel World Ticking”Keys are parallel-world-ticking; align display names.
- "Parallel World Tracing", + "Parallel World Ticking", - "Parallel World Tracing Threads", + "Parallel World Ticking Threads", - "Parallel World Tracing Log Container Creation Stacktraces", + "Parallel World Ticking Log Container Creation Stacktraces", - "Parallel World Tracing Disable Hard Throw", + "Parallel World Ticking Disable Hard Throw", - "Parallel World Tracing Run Async Tasks Sync", + "Parallel World Ticking Run Async Tasks Sync",Also applies to: 46-50, 61-68, 71-82, 85-96
191-197: Typo: “aynchronous” → “asynchronous”- "Make playerdata saving aynchronous. Warning: might", + "Make playerdata saving asynchronous. Warning: might",
241-247: Label mismatchThis section is for async pathfinding; fix the title.
- "Async Entity Tracker Queue Size", + "Async Pathfinding Queue Size",
256-259: Label mismatchReject-policy is for pathfinding.
- "Async Entity Tracker Queue Size", + "Async Pathfinding Reject Policy",
271-283: Label mismatchMob spawning section labeled as pathfinding.
- "Async Pathfinding", + "Async Mob Spawning",src/main/java/net/lewmc/kryptonite/utils/config/DoubleConfigItem.java (1)
51-72: Use extension checks with endsWith and ensure YAML is always closed.
contains(".yml")/contains(".yaml")can mis-detect backup names; preferendsWith.- Wrap YAML get in a try/finally or ensure close is called on all paths.
Apply this diff:
@Override public Double getValue() { - if (this.file.contains(".properties")) { + if (this.file.endsWith(".properties")) { try { return Double.parseDouble(propFile.getProperty(key)); } catch (NumberFormatException e) { Logger l = new Logger(this.plugin.foundryConfig); l.severe("Unable to parse key '"+this.key+"' in file '"+this.file+"' NumberFormatException"); l.severe(e.getMessage()); return 0.0; } - } else if (this.file.contains(".yml") || file.contains(".yaml")) { + } else if (this.file.endsWith(".yml") || this.file.endsWith(".yaml")) { this.loadFile(); Double value = yamlFile.getDouble(key); yamlFile.close(); return value; } else { new Logger(this.plugin.foundryConfig).severe("Unable to load file: '"+this.file+"' extension not supported."); return 0.0; } }src/main/java/net/lewmc/kryptonite/utils/config/StringConfigItem.java (2)
63-71: Close YAML after save and prefer endsWith for extension checks.Apply this diff:
@Override public void setValue(String value) { - if (this.file.contains(".properties")) { + if (this.file.endsWith(".properties")) { propFile.setProperty(this.key, value); - } else if (file.contains(".yml") || file.contains(".yaml")) { + } else if (this.file.endsWith(".yml") || this.file.endsWith(".yaml")) { this.loadFile(); yamlFile.set(this.key, value); - yamlFile.save(); + yamlFile.save(); + yamlFile.close(); } }
45-56: Tighten extension checks and consistent error handling.Use
endsWithto avoid false positives on backup filenames.Apply this diff:
@Override public String getValue() { - if (this.file.contains(".properties")) { + if (this.file.endsWith(".properties")) { return propFile.getProperty(key); - } else if (this.file.contains(".yml") || file.contains(".yaml")) { + } else if (this.file.endsWith(".yml") || this.file.endsWith(".yaml")) { this.loadFile(); String value = yamlFile.getString(key); yamlFile.close(); return value; } else { new Logger(this.plugin.foundryConfig).severe("Unable to load file: '"+this.file+"' extension not supported."); return null; }src/main/java/net/lewmc/kryptonite/utils/config/BooleanConfigItem.java (1)
56-64: Close YAML after save and tighten extension checks.Apply this diff:
@Override public void setValue(Boolean value) { - if (this.file.contains(".properties")) { + if (this.file.endsWith(".properties")) { propFile.setProperty(this.key, String.valueOf(value)); - } else if (file.contains(".yml") || file.contains(".yaml")) { + } else if (this.file.endsWith(".yml") || this.file.endsWith(".yaml")) { this.loadFile(); yamlFile.set(this.key, value); - yamlFile.save(); + yamlFile.save(); + yamlFile.close(); } }src/main/java/net/lewmc/kryptonite/kos/gui/KOS_ConfigItemGui.java (4)
151-168: Avoid repeated disk reads; reusecurrentfor validation.
getValue()may hit disk; reuse local value to cut I/O.int current = intItem.getValue(); if (click.getType() == ClickType.RIGHT) { - if (intItem.willBeValid(intItem.getValue() + 1)) { + if (intItem.willBeValid(current + 1)) { intItem.setValue(current + 1); } } else if (click.getType() == ClickType.SHIFT_RIGHT) { - if (intItem.willBeValid(intItem.getValue() + 10)) { + if (intItem.willBeValid(current + 10)) { intItem.setValue(current + 10); } } else if (click.getType() == ClickType.LEFT) { - if (intItem.willBeValid(intItem.getValue() - 1)) { + if (intItem.willBeValid(current - 1)) { intItem.setValue(current - 1); } } else if (click.getType() == ClickType.SHIFT_LEFT) { - if (intItem.willBeValid(intItem.getValue() - 10)) { + if (intItem.willBeValid(current - 10)) { intItem.setValue(current - 10); } }
170-187: Same optimization for doubles; reuse local value.Double current = doubleItem.getValue(); if (click.getType() == ClickType.RIGHT) { - if (doubleItem.willBeValid(doubleItem.getValue() + 1.0)) { + if (doubleItem.willBeValid(current + 1.0)) { doubleItem.setValue(current + 1.0); } } else if (click.getType() == ClickType.SHIFT_RIGHT) { - if (doubleItem.willBeValid(doubleItem.getValue() + 10.0)) { + if (doubleItem.willBeValid(current + 10.0)) { doubleItem.setValue(current + 10.0); } } else if (click.getType() == ClickType.LEFT) { - if (doubleItem.willBeValid(doubleItem.getValue() - 1.0)) { + if (doubleItem.willBeValid(current - 1.0)) { doubleItem.setValue(current - 1.0); } } else if (click.getType() == ClickType.SHIFT_LEFT) { - if (doubleItem.willBeValid(doubleItem.getValue() - 10.0)) { + if (doubleItem.willBeValid(current - 10.0)) { doubleItem.setValue(current - 10.0); } }
188-190: Implement cycling for String options.Quick win: cycle through
allowedValueson click; SHIFT to reverse. I can wire this if you confirm desired UX.
68-86: Optional: deterministic ordering.
HashMapiteration order is undefined; consider sorting entries bygetName()for stable GUI.src/main/java/net/lewmc/kryptonite/utils/config/IntegerConfigItem.java (2)
78-87: Close YAML after save and prefer endsWith.Apply this diff:
@Override public void setValue(Integer value) { - if (this.file.contains(".properties")) { + if (this.file.endsWith(".properties")) { propFile.setProperty(this.key, String.valueOf(value)); - } else if (file.contains(".yml") || file.contains(".yaml")) { + } else if (this.file.endsWith(".yml") || this.file.endsWith(".yaml")) { this.loadFile(); yamlFile.set(this.key, value); - yamlFile.save(); + yamlFile.save(); + yamlFile.close(); } }
53-72: Tighten extension checks and ensure YAML close on all paths (already closes here).- if (this.file.contains(".properties")) { + if (this.file.endsWith(".properties")) { ... - } else if (this.file.contains(".yml") || file.contains(".yaml")) { + } else if (this.file.endsWith(".yml") || this.file.endsWith(".yaml")) {src/main/java/net/lewmc/kryptonite/utils/LogUtil.java (3)
52-60: Public API typo: veboseInfo → verboseInfo (keep both for compatibility)Expose a correctly spelled alias to avoid propagating the typo while keeping existing calls working.
Add alongside the existing method:
/** Correctly spelled alias; delegates to legacy method. */ public void verboseInfo(String message) { veboseInfo(message); }
68-72: Only verbose() writes to file; make info/warn/severe honor logfile tooParity with verbose logging improves diagnosability when
logfile=true.public void info(String message) { message = "[Kryptonite] " + message; - Bukkit.getLogger().info(message); + Bukkit.getLogger().info(message); + this.logToFile(Type.LOG_INFO, message); } public void warn(String message) { message = "[Kryptonite] " + message; - Bukkit.getLogger().warning(message); + Bukkit.getLogger().warning(message); + this.logToFile(Type.LOG_WARNING, message); } public void severe(String message) { message = "[Kryptonite] " + message; - Bukkit.getLogger().severe(message); + Bukkit.getLogger().severe(message); + this.logToFile(Type.LOG_SEVERE, message); }Also applies to: 80-84, 92-96
111-111: File append robustness + redundant task cancel
- Use CREATE/WRITE so logging survives deleted/missing files.
- Build the path portably.
cancelTask(task)after a one-shot async run is unnecessary.- Path p = Paths.get(this.plugin.getDataFolder().getAbsolutePath() + "/kryptonite.log"); + Path p = Paths.get(this.plugin.getDataFolder().getAbsolutePath(), "kryptonite.log"); ... - try (BufferedWriter writer = Files.newBufferedWriter(p, StandardOpenOption.APPEND)) { + try (BufferedWriter writer = Files.newBufferedWriter( + p, StandardOpenOption.CREATE, StandardOpenOption.WRITE, StandardOpenOption.APPEND)) { writer.write(s); } catch (IOException e) { Bukkit.getLogger().severe("[Kryptonite] Unable to write to file: " + e.getMessage()); } - foliaLib.getScheduler().cancelTask(task);Also applies to: 114-114, 119-119
src/main/java/net/lewmc/kryptonite/utils/config/GenericConfigItem.java (3)
71-73: Null‑safety for dependency flagGuard against
nullto match the doc “If none, set to true.”- this.dependencyIsEnabled = dependencyIsEnabled; + this.dependencyIsEnabled = (dependencyIsEnabled != null) ? dependencyIsEnabled : Boolean.TRUE; this.plugin = plugin;
140-142: Avoid exposing internal mutable listReturn an unmodifiable view to prevent accidental external mutation.
- public List<String> getDescription() { - return this.description; - } + public List<String> getDescription() { + return java.util.Collections.unmodifiableList(this.description); + }
96-101: Javadoc nits
setValue: “The value to set.” (not “current value”).willBeValid: add@param valuedescription.Also applies to: 110-115
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (45)
dependency-reduced-pom.xml(5 hunks)pom.xml(3 hunks)src/main/java/net/lewmc/kryptonite/Kryptonite.java(5 hunks)src/main/java/net/lewmc/kryptonite/Metrics.java(1 hunks)src/main/java/net/lewmc/kryptonite/config/LeafConfig.java(1 hunks)src/main/java/net/lewmc/kryptonite/config/MinecraftConfig.java(1 hunks)src/main/java/net/lewmc/kryptonite/edb/Check.java(3 hunks)src/main/java/net/lewmc/kryptonite/edb/Patch.java(1 hunks)src/main/java/net/lewmc/kryptonite/kos/AutoKOS.java(4 hunks)src/main/java/net/lewmc/kryptonite/kos/config/Bukkit.java(1 hunks)src/main/java/net/lewmc/kryptonite/kos/config/Leaf.java(0 hunks)src/main/java/net/lewmc/kryptonite/kos/config/PaperWorld.java(1 hunks)src/main/java/net/lewmc/kryptonite/kos/config/Pufferfish.java(1 hunks)src/main/java/net/lewmc/kryptonite/kos/config/Purpur.java(1 hunks)src/main/java/net/lewmc/kryptonite/kos/config/ServerProperties.java(0 hunks)src/main/java/net/lewmc/kryptonite/kos/config/Spigot.java(1 hunks)src/main/java/net/lewmc/kryptonite/kos/gui/KOS_BukkitGui.java(1 hunks)src/main/java/net/lewmc/kryptonite/kos/gui/KOS_ConfigItemGui.java(1 hunks)src/main/java/net/lewmc/kryptonite/kos/gui/KOS_GuiConstants.java(3 hunks)src/main/java/net/lewmc/kryptonite/kos/gui/KOS_LeafGui.java(0 hunks)src/main/java/net/lewmc/kryptonite/kos/gui/KOS_ManualGUI.java(3 hunks)src/main/java/net/lewmc/kryptonite/kos/gui/KOS_PaperWorld_1.java(1 hunks)src/main/java/net/lewmc/kryptonite/kos/gui/KOS_PaperWorld_2.java(1 hunks)src/main/java/net/lewmc/kryptonite/kos/gui/KOS_PaperWorld_3.java(1 hunks)src/main/java/net/lewmc/kryptonite/kos/gui/KOS_PaperWorld_4.java(1 hunks)src/main/java/net/lewmc/kryptonite/kos/gui/KOS_PaperWorld_5.java(1 hunks)src/main/java/net/lewmc/kryptonite/kos/gui/KOS_PufferfishGui.java(1 hunks)src/main/java/net/lewmc/kryptonite/kos/gui/KOS_PurpurGui.java(1 hunks)src/main/java/net/lewmc/kryptonite/kos/gui/KOS_ServerPropertiesGui.java(0 hunks)src/main/java/net/lewmc/kryptonite/kos/gui/KOS_SpigotGui_1.java(3 hunks)src/main/java/net/lewmc/kryptonite/kos/gui/KOS_SpigotGui_2.java(1 hunks)src/main/java/net/lewmc/kryptonite/report/Report.java(1 hunks)src/main/java/net/lewmc/kryptonite/utils/LogUtil.java(6 hunks)src/main/java/net/lewmc/kryptonite/utils/PermissionUtil.java(1 hunks)src/main/java/net/lewmc/kryptonite/utils/SoftwareUtil.java(1 hunks)src/main/java/net/lewmc/kryptonite/utils/UpdateUtil.java(2 hunks)src/main/java/net/lewmc/kryptonite/utils/config/BooleanConfigItem.java(1 hunks)src/main/java/net/lewmc/kryptonite/utils/config/ConfigCollection.java(1 hunks)src/main/java/net/lewmc/kryptonite/utils/config/DoubleConfigItem.java(1 hunks)src/main/java/net/lewmc/kryptonite/utils/config/GenericConfigItem.java(1 hunks)src/main/java/net/lewmc/kryptonite/utils/config/IntegerConfigItem.java(1 hunks)src/main/java/net/lewmc/kryptonite/utils/config/StringConfigItem.java(1 hunks)src/main/resources/plugin.yml(2 hunks)src/main/resources/profiles/FarmFriendly.kos(1 hunks)src/main/resources/profiles/YouHaveTrouble.kos(1 hunks)
💤 Files with no reviewable changes (4)
- src/main/java/net/lewmc/kryptonite/kos/gui/KOS_LeafGui.java
- src/main/java/net/lewmc/kryptonite/kos/config/ServerProperties.java
- src/main/java/net/lewmc/kryptonite/kos/gui/KOS_ServerPropertiesGui.java
- src/main/java/net/lewmc/kryptonite/kos/config/Leaf.java
🧰 Additional context used
🧬 Code graph analysis (26)
src/main/java/net/lewmc/kryptonite/utils/config/GenericConfigItem.java (2)
src/main/java/net/lewmc/kryptonite/Kryptonite.java (1)
Kryptonite(20-198)src/main/java/net/lewmc/kryptonite/utils/PropertiesUtil.java (1)
PropertiesUtil(8-48)
src/main/java/net/lewmc/kryptonite/kos/gui/KOS_PaperWorld_1.java (4)
src/main/java/net/lewmc/kryptonite/kos/gui/KOS_PaperWorld_2.java (1)
Deprecated(20-1061)src/main/java/net/lewmc/kryptonite/kos/gui/KOS_PaperWorld_3.java (1)
Deprecated(21-903)src/main/java/net/lewmc/kryptonite/kos/gui/KOS_PaperWorld_4.java (1)
Deprecated(20-944)src/main/java/net/lewmc/kryptonite/kos/gui/KOS_PaperWorld_5.java (1)
Deprecated(20-788)
src/main/java/net/lewmc/kryptonite/utils/SoftwareUtil.java (1)
src/main/java/net/lewmc/kryptonite/Kryptonite.java (1)
Kryptonite(20-198)
src/main/java/net/lewmc/kryptonite/config/MinecraftConfig.java (4)
src/main/java/net/lewmc/kryptonite/utils/config/BooleanConfigItem.java (1)
BooleanConfigItem(12-95)src/main/java/net/lewmc/kryptonite/utils/config/ConfigCollection.java (1)
ConfigCollection(9-14)src/main/java/net/lewmc/kryptonite/utils/config/GenericConfigItem.java (1)
GenericConfigItem(16-151)src/main/java/net/lewmc/kryptonite/utils/config/IntegerConfigItem.java (1)
IntegerConfigItem(12-125)
src/main/java/net/lewmc/kryptonite/kos/gui/KOS_PaperWorld_5.java (2)
src/main/java/net/lewmc/kryptonite/kos/gui/KOS_PaperWorld_1.java (1)
Deprecated(20-599)src/main/java/net/lewmc/kryptonite/kos/gui/KOS_PaperWorld_4.java (1)
Deprecated(20-944)
src/main/java/net/lewmc/kryptonite/kos/config/Purpur.java (1)
src/main/java/net/lewmc/kryptonite/kos/gui/KOS_PurpurGui.java (1)
Deprecated(20-465)
src/main/java/net/lewmc/kryptonite/kos/gui/KOS_PaperWorld_4.java (3)
src/main/java/net/lewmc/kryptonite/kos/gui/KOS_PaperWorld_1.java (1)
Deprecated(20-599)src/main/java/net/lewmc/kryptonite/kos/gui/KOS_PaperWorld_3.java (1)
Deprecated(21-903)src/main/java/net/lewmc/kryptonite/kos/gui/KOS_PaperWorld_5.java (1)
Deprecated(20-788)
src/main/java/net/lewmc/kryptonite/kos/gui/KOS_PaperWorld_2.java (4)
src/main/java/net/lewmc/kryptonite/kos/gui/KOS_PaperWorld_1.java (1)
Deprecated(20-599)src/main/java/net/lewmc/kryptonite/kos/gui/KOS_PaperWorld_3.java (1)
Deprecated(21-903)src/main/java/net/lewmc/kryptonite/kos/gui/KOS_PaperWorld_4.java (1)
Deprecated(20-944)src/main/java/net/lewmc/kryptonite/kos/gui/KOS_PaperWorld_5.java (1)
Deprecated(20-788)
src/main/java/net/lewmc/kryptonite/kos/config/Pufferfish.java (1)
src/main/java/net/lewmc/kryptonite/kos/gui/KOS_PufferfishGui.java (1)
Deprecated(20-388)
src/main/java/net/lewmc/kryptonite/utils/config/DoubleConfigItem.java (1)
src/main/java/net/lewmc/kryptonite/Kryptonite.java (1)
Kryptonite(20-198)
src/main/java/net/lewmc/kryptonite/kos/config/Spigot.java (2)
src/main/java/net/lewmc/kryptonite/kos/gui/KOS_SpigotGui_1.java (1)
Deprecated(21-757)src/main/java/net/lewmc/kryptonite/kos/gui/KOS_SpigotGui_2.java (1)
Deprecated(20-140)
src/main/java/net/lewmc/kryptonite/kos/gui/KOS_SpigotGui_2.java (2)
src/main/java/net/lewmc/kryptonite/kos/gui/KOS_SpigotGui_1.java (1)
Deprecated(21-757)src/main/java/net/lewmc/kryptonite/kos/config/Spigot.java (1)
Deprecated(12-186)
src/main/java/net/lewmc/kryptonite/kos/gui/KOS_PurpurGui.java (3)
src/main/java/net/lewmc/kryptonite/kos/gui/KOS_PufferfishGui.java (1)
Deprecated(20-388)src/main/java/net/lewmc/kryptonite/kos/gui/KOS_SpigotGui_1.java (1)
Deprecated(21-757)src/main/java/net/lewmc/kryptonite/kos/gui/KOS_BukkitGui.java (1)
Deprecated(20-629)
src/main/java/net/lewmc/kryptonite/utils/config/BooleanConfigItem.java (1)
src/main/java/net/lewmc/kryptonite/Kryptonite.java (1)
Kryptonite(20-198)
src/main/java/net/lewmc/kryptonite/kos/gui/KOS_PaperWorld_3.java (5)
src/main/java/net/lewmc/kryptonite/kos/config/PaperWorld.java (1)
Deprecated(12-390)src/main/java/net/lewmc/kryptonite/kos/gui/KOS_PaperWorld_1.java (1)
Deprecated(20-599)src/main/java/net/lewmc/kryptonite/kos/gui/KOS_PaperWorld_2.java (1)
Deprecated(20-1061)src/main/java/net/lewmc/kryptonite/kos/gui/KOS_PaperWorld_4.java (1)
Deprecated(20-944)src/main/java/net/lewmc/kryptonite/kos/gui/KOS_PaperWorld_5.java (1)
Deprecated(20-788)
src/main/java/net/lewmc/kryptonite/utils/config/IntegerConfigItem.java (1)
src/main/java/net/lewmc/kryptonite/Kryptonite.java (1)
Kryptonite(20-198)
src/main/java/net/lewmc/kryptonite/kos/config/PaperWorld.java (5)
src/main/java/net/lewmc/kryptonite/kos/gui/KOS_PaperWorld_1.java (1)
Deprecated(20-599)src/main/java/net/lewmc/kryptonite/kos/gui/KOS_PaperWorld_2.java (1)
Deprecated(20-1061)src/main/java/net/lewmc/kryptonite/kos/gui/KOS_PaperWorld_3.java (1)
Deprecated(21-903)src/main/java/net/lewmc/kryptonite/kos/gui/KOS_PaperWorld_4.java (1)
Deprecated(20-944)src/main/java/net/lewmc/kryptonite/kos/gui/KOS_PaperWorld_5.java (1)
Deprecated(20-788)
src/main/java/net/lewmc/kryptonite/kos/AutoKOS.java (2)
src/main/java/net/lewmc/kryptonite/Kryptonite.java (1)
Kryptonite(20-198)src/main/java/net/lewmc/kryptonite/config/MinecraftConfig.java (1)
MinecraftConfig(15-96)
src/main/java/net/lewmc/kryptonite/kos/gui/KOS_PufferfishGui.java (2)
src/main/java/net/lewmc/kryptonite/kos/gui/KOS_SpigotGui_1.java (1)
Deprecated(21-757)src/main/java/net/lewmc/kryptonite/kos/config/Pufferfish.java (1)
Deprecated(12-132)
src/main/java/net/lewmc/kryptonite/kos/gui/KOS_SpigotGui_1.java (1)
src/main/java/net/lewmc/kryptonite/config/MinecraftConfig.java (1)
MinecraftConfig(15-96)
src/main/java/net/lewmc/kryptonite/utils/config/StringConfigItem.java (1)
src/main/java/net/lewmc/kryptonite/Kryptonite.java (1)
Kryptonite(20-198)
src/main/java/net/lewmc/kryptonite/config/LeafConfig.java (1)
src/main/java/net/lewmc/kryptonite/Kryptonite.java (1)
Kryptonite(20-198)
src/main/java/net/lewmc/kryptonite/kos/gui/KOS_BukkitGui.java (2)
src/main/java/net/lewmc/kryptonite/kos/gui/KOS_PufferfishGui.java (1)
Deprecated(20-388)src/main/java/net/lewmc/kryptonite/kos/gui/KOS_SpigotGui_1.java (1)
Deprecated(21-757)
src/main/java/net/lewmc/kryptonite/kos/config/Bukkit.java (1)
src/main/java/net/lewmc/kryptonite/kos/gui/KOS_BukkitGui.java (1)
Deprecated(20-629)
src/main/java/net/lewmc/kryptonite/kos/gui/KOS_ManualGUI.java (1)
src/main/java/net/lewmc/kryptonite/Kryptonite.java (1)
Kryptonite(20-198)
src/main/java/net/lewmc/kryptonite/kos/gui/KOS_ConfigItemGui.java (3)
src/main/java/net/lewmc/kryptonite/Kryptonite.java (1)
Kryptonite(20-198)src/main/java/net/lewmc/kryptonite/config/LeafConfig.java (1)
LeafConfig(15-285)src/main/java/net/lewmc/kryptonite/config/MinecraftConfig.java (1)
MinecraftConfig(15-96)
🪛 YAMLlint (1.37.1)
src/main/resources/plugin.yml
[warning] 21-21: too few spaces after comma
(commas)
| values.put("simulation-distance", new IntegerConfigItem( | ||
| file, | ||
| "simulation-distance", | ||
| "Simulation Distance", | ||
| List.of("The distance mobs will be simulated."), | ||
| true, | ||
| 1, | ||
| 50, | ||
| "5 - "+values.get("view-distance").getValue(), | ||
| plugin | ||
| )); |
There was a problem hiding this comment.
Critical: NPE risk when constructing simulation-distance ideal
values.get("view-distance").getValue() runs before any file is loaded; for .properties, propFile is null and will NPE. Don’t read dependent values in the constructor.
Minimal fix:
- "5 - "+values.get("view-distance").getValue(),
+ "5 - 10",Alternative: compute this ideal lazily in getIdealValue() or load the file before reading, but avoid I/O in constructors.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| values.put("simulation-distance", new IntegerConfigItem( | |
| file, | |
| "simulation-distance", | |
| "Simulation Distance", | |
| List.of("The distance mobs will be simulated."), | |
| true, | |
| 1, | |
| 50, | |
| "5 - "+values.get("view-distance").getValue(), | |
| plugin | |
| )); | |
| values.put("simulation-distance", new IntegerConfigItem( | |
| file, | |
| "simulation-distance", | |
| "Simulation Distance", | |
| List.of("The distance mobs will be simulated."), | |
| true, | |
| 1, | |
| 50, | |
| "5 - 10", | |
| plugin | |
| )); |
🤖 Prompt for AI Agents
In src/main/java/net/lewmc/kryptonite/config/MinecraftConfig.java around lines
58-68, constructing the "simulation-distance" IntegerConfigItem currently calls
values.get("view-distance").getValue() during construction which can NPE because
config files (propFile) may not be loaded yet; remove that eager dependent read
and avoid I/O in the constructor. Fix by supplying a static/neutral ideal (or
null/empty string) when constructing the item and move the dependent computation
into a lazy getter (e.g., override/getIdealValue() to compute "5 -
<view-distance>" at runtime using values.get("view-distance").getValue()), or
compute and set the ideal after all values are loaded; do not call
values.get(...).getValue() inside the constructor.
src/main/java/net/lewmc/kryptonite/kos/gui/KOS_ConfigItemGui.java
Outdated
Show resolved
Hide resolved
| /** | ||
| * @deprecated | ||
| */ | ||
| @Deprecated | ||
| public class Metrics { |
There was a problem hiding this comment.
Do not modify bStats Metrics class — revert deprecation
Header explicitly forbids modifications other than package changes. Adding @deprecated may violate bStats requirements.
-/**
- * @deprecated
- */
-@Deprecated
+/* no modifications allowed by bStats other than package */📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * @deprecated | |
| */ | |
| @Deprecated | |
| public class Metrics { | |
| /* no modifications allowed by bStats other than package */ | |
| public class Metrics { |
🤖 Prompt for AI Agents
In src/main/java/net/lewmc/kryptonite/Metrics.java around lines 49 to 53, the
bStats Metrics class was edited to add a @Deprecated annotation and a
deprecation Javadoc; revert those changes so the class matches the original
bStats implementation (remove the @Deprecated annotation and the deprecated
Javadoc), leaving only the allowed package change if that was required, and
ensure no other modifications are present to comply with bStats header
requirements.
| public void setValue(Double value) { | ||
| if (this.file.contains(".properties")) { | ||
| propFile.setProperty(this.key, String.valueOf(value)); | ||
| } else if (file.contains(".yml") || file.contains(".yaml")) { | ||
| this.loadFile(); | ||
| yamlFile.set(this.key, value); | ||
| yamlFile.save(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Close YAML handle after save to avoid file descriptor leaks.
setValue loads the YAML and saves but never closes it. On Windows this can lock the file; on any OS it can leak FDs.
Apply this diff:
@Override
public void setValue(Double value) {
- if (this.file.contains(".properties")) {
+ if (this.file.endsWith(".properties")) {
propFile.setProperty(this.key, String.valueOf(value));
- } else if (file.contains(".yml") || file.contains(".yaml")) {
+ } else if (this.file.endsWith(".yml") || this.file.endsWith(".yaml")) {
this.loadFile();
yamlFile.set(this.key, value);
- yamlFile.save();
+ yamlFile.save();
+ yamlFile.close();
}
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/main/java/net/lewmc/kryptonite/utils/config/DoubleConfigItem.java around
lines 79 to 87, the YAML file handle is left open after calling yamlFile.save(),
which can lock the file or leak file descriptors; fix by ensuring the YAML
handle is closed after saving (e.g., call yamlFile.close() after yamlFile.save()
or wrap loadFile()/set/save in a try/finally so yamlFile.close() always runs)
and handle possible exceptions during save so the close is guaranteed.
| @Override | ||
| public boolean isIdeal() { | ||
| if (idealValue == null) { return true; } | ||
| Double current = this.getValue(); | ||
| if (idealValue.contains("-")) { | ||
| String[] parts = idealValue.split("-"); | ||
| double minIdeal = Double.parseDouble(parts[0]); | ||
| double maxIdeal = Double.parseDouble(parts[1]); | ||
| return current >= minIdeal && current <= maxIdeal; | ||
| } else { | ||
| return current == Double.parseDouble(idealValue); | ||
| } | ||
| } |
There was a problem hiding this comment.
Harden ideal-value parsing; trim parts and avoid exact double equality.
Ranges like "1 - 5" currently throw NumberFormatException (no trim). Exact == on doubles is fragile.
Apply this diff:
@Override
public boolean isIdeal() {
if (idealValue == null) { return true; }
Double current = this.getValue();
- if (idealValue.contains("-")) {
- String[] parts = idealValue.split("-");
- double minIdeal = Double.parseDouble(parts[0]);
- double maxIdeal = Double.parseDouble(parts[1]);
- return current >= minIdeal && current <= maxIdeal;
- } else {
- return current == Double.parseDouble(idealValue);
- }
+ try {
+ if (idealValue.contains("-")) {
+ String[] parts = idealValue.split("-");
+ double minIdeal = Double.parseDouble(parts[0].trim());
+ double maxIdeal = Double.parseDouble(parts[1].trim());
+ return current >= minIdeal && current <= maxIdeal;
+ } else {
+ double target = Double.parseDouble(idealValue.trim());
+ return Math.abs(current - target) <= 1e-9;
+ }
+ } catch (NumberFormatException ex) {
+ new Logger(this.plugin.foundryConfig).severe("Invalid idealValue '"+idealValue+"' for key '"+this.key+"'");
+ return false;
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @Override | |
| public boolean isIdeal() { | |
| if (idealValue == null) { return true; } | |
| Double current = this.getValue(); | |
| if (idealValue.contains("-")) { | |
| String[] parts = idealValue.split("-"); | |
| double minIdeal = Double.parseDouble(parts[0]); | |
| double maxIdeal = Double.parseDouble(parts[1]); | |
| return current >= minIdeal && current <= maxIdeal; | |
| } else { | |
| return current == Double.parseDouble(idealValue); | |
| } | |
| } | |
| @Override | |
| public boolean isIdeal() { | |
| if (idealValue == null) { return true; } | |
| Double current = this.getValue(); | |
| try { | |
| if (idealValue.contains("-")) { | |
| String[] parts = idealValue.split("-"); | |
| double minIdeal = Double.parseDouble(parts[0].trim()); | |
| double maxIdeal = Double.parseDouble(parts[1].trim()); | |
| return current >= minIdeal && current <= maxIdeal; | |
| } else { | |
| double target = Double.parseDouble(idealValue.trim()); | |
| return Math.abs(current - target) <= 1e-9; | |
| } | |
| } catch (NumberFormatException ex) { | |
| new Logger(this.plugin.foundryConfig).severe("Invalid idealValue '"+idealValue+"' for key '"+this.key+"'"); | |
| return false; | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/main/java/net/lewmc/kryptonite/utils/config/DoubleConfigItem.java around
lines 103 to 115, the ideal-value parsing is brittle: split parts aren't trimmed
(so "1 - 5" breaks) and exact double equality is unsafe. Update the range branch
to trim each split part before Double.parseDouble, guard against null current
value, and catch/handle NumberFormatException (e.g., treat malformed ideal as
non-ideal or log and return false). Replace the exact equality check with a
tolerance-based comparison using a small epsilon (e.g., Math.abs(current -
target) < 1e-9) so near-equal doubles count as ideal.
| @Override | ||
| public boolean willBeValid(String value) { | ||
| return dependencyIsEnabled && this.allowedValues.contains(value); | ||
|
|
||
| } |
There was a problem hiding this comment.
Avoid NPE when allowedValues is null; treat null as “allow any”.
allowedValues.contains(value) will NPE if allowedValues is omitted. Aligns with your boolean/integer “null ideal means any” semantics.
Apply this diff:
@Override
public boolean willBeValid(String value) {
- return dependencyIsEnabled && this.allowedValues.contains(value);
+ return this.dependencyIsEnabled && (this.allowedValues == null || this.allowedValues.contains(value));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @Override | |
| public boolean willBeValid(String value) { | |
| return dependencyIsEnabled && this.allowedValues.contains(value); | |
| } | |
| @Override | |
| public boolean willBeValid(String value) { | |
| return this.dependencyIsEnabled && (this.allowedValues == null || this.allowedValues.contains(value)); | |
| } |
🤖 Prompt for AI Agents
In src/main/java/net/lewmc/kryptonite/utils/config/StringConfigItem.java around
lines 79 to 83, the willBeValid method currently calls
allowedValues.contains(value) which can NPE if allowedValues is null; change the
logic so null allowedValues means "allow any" by returning dependencyIsEnabled
&& (allowedValues == null || allowedValues.contains(value)). Update the boolean
expression accordingly so it first checks dependencyIsEnabled and then treats a
null allowedValues as permitting all values.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/main/java/net/lewmc/kryptonite/Kryptonite.java (2)
4-5: Foundry logger wiring looks coherent; consider config load order forverboseThe switch to
net.lewmc.foundry.Loggerwith a sharedFoundryConfigis consistent withUpdateUtil’snew Logger(plugin.foundryConfig)and centralizes logging nicely. The only nuance is thatfoundryConfig.setVerbose(this.getConfig().getBoolean("verbose"))runs beforeinitFilesystem()callssaveDefaultConfig(), so on a first run (noconfig.ymlyet)verbosemay not reflect your intended default.If you want
verboseto always honor the shipped default on first startup, consider either saving/loading the config before constructingFoundryConfig, or explicitly ensuring defaults are copied before reading"verbose".Also applies to: 12-12, 23-24, 37-45
25-33: New configuration options and Leaf detection look consistent; string building could be cleaned upThe renaming to
MINECRAFTand the additions ofPAPER_GLOBAL,PURPUR,PUFFERFISH, andLEAF, plus the corresponding detection forserver.propertiesandconfig/leaf-global.yml, all line up and should integrate cleanly with the rest of the config system.Given the growing list, you might (optionally) replace the manual
supportedConfigs = supportedConfigs + ", ..."pattern with aStringJoineror list +String.join(", ", ...)to avoid leading commas and make future additions less error‑prone, but this is purely a readability/UX polish.Also applies to: 133-134, 173-177
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/net/lewmc/kryptonite/Kryptonite.java(5 hunks)src/main/java/net/lewmc/kryptonite/Metrics.java(0 hunks)
💤 Files with no reviewable changes (1)
- src/main/java/net/lewmc/kryptonite/Metrics.java
🧰 Additional context used
🧬 Code graph analysis (1)
src/main/java/net/lewmc/kryptonite/Kryptonite.java (1)
src/main/java/net/lewmc/kryptonite/utils/UpdateUtil.java (1)
UpdateUtil(13-104)
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
pom.xml (1)
115-117: Critical: ConfigUpdater dependency cannot be resolved.The build is failing because this SNAPSHOT dependency is not available in any configured repository. See the detailed analysis in dependency-reduced-pom.xml review.
src/main/java/net/lewmc/kryptonite/config/MinecraftConfig.java (1)
52-62: Critical: NPE risk when constructing simulation-distance with dependent value.Line 60 calls
values.get("view-distance").getValue()to compute the ideal value for simulation-distance. This will cause a NullPointerException because:
- The
getValue()method for properties files reads frompropFile(see IntegerConfigItem.getValue() in the snippets)- The
propFilefield is not initialized until the config item loads the file- At constructor time, no files have been loaded yet
This will crash immediately when
new MinecraftConfig(plugin)is instantiated.Apply this diff to use a static ideal value:
values.put("simulation-distance", new IntegerConfigItem( file, "simulation-distance", "Simulation Distance", List.of("The distance mobs will be simulated."), true, 1, 50, - "5 - "+values.get("view-distance").getValue(), + "5 - 10", plugin ));If you need the dynamic ideal computation, move it to a lazy getter or compute it after all config items are loaded.
src/main/java/net/lewmc/kryptonite/kos/gui/KOS_ConfigItemGui.java (2)
54-60: Critical: ClassCastException risk with non-player command senders.Lines 55 and 59 unsafely cast
this.usertoPlayerwithout checking if the sender is actually a player. This will crash if the command is executed from console or a command block.Apply this diff to guard against non-player senders:
public void show() { + if (!(this.user instanceof Player player)) { + this.user.sendMessage(ChatColor.RED + "This command can only be used by a player."); + return; + } - this.gui = new InventoryGui(this.plugin, (Player) this.user, "KOS - Server Configuration", this.getElements()); + this.gui = new InventoryGui(this.plugin, player, "KOS - Server Configuration", this.getElements()); this.addElements(); this.gui.build(); - this.gui.show((Player) this.user); + this.gui.show(player); }
223-226: Critical: Invalid layout when config is null.Line 225 returns an array with empty strings
""for rows, which will break InventoryGui's layout parsing. InventoryGui expects each row to be exactly 9 characters representing the 9 inventory slots.Apply this diff to provide a valid layout:
} else { this.plugin.getLogger().severe("Unable to return elements: config is null."); - return new String[] {"",""," t u v "}; + return new String[] {" ", " t u v "}; }Note: Each row must be exactly 9 characters. Spaces represent empty slots.
🧹 Nitpick comments (3)
src/main/java/net/lewmc/kryptonite/config/LeafConfig.java (1)
279-279: Remove debug logging statement before release.This debug log was useful during development but should be removed or made conditional (verbose mode) for production releases.
Apply this diff:
- plugin.getLogger().info("LeafConfig values size = " + values.size());src/main/java/net/lewmc/kryptonite/kos/gui/KOS_ConfigItemGui.java (2)
185-187: String config item handling not implemented.The TODO comment indicates that string configuration item handling is not yet implemented. Users clicking on StringConfigItem entries will not see any editing interface.
Would you like me to generate an implementation for string config item handling? This could include a sign-based input or chat-based value entry system.
71-71: Remove debug logging statements before release.Multiple debug log statements (lines 71, 217-220) should be removed or made conditional for production.
Apply this diff:
- plugin.getLogger().info("Adding config element: id=" + id + " index=" + index + " name=" + config.getName());And remove the logging at lines 217-220 as well.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
dependency-reduced-pom.xml(5 hunks)pom.xml(3 hunks)src/main/java/net/lewmc/kryptonite/config/LeafConfig.java(1 hunks)src/main/java/net/lewmc/kryptonite/config/MinecraftConfig.java(1 hunks)src/main/java/net/lewmc/kryptonite/kos/gui/KOS_ConfigItemGui.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/main/java/net/lewmc/kryptonite/config/LeafConfig.java (1)
src/main/java/net/lewmc/kryptonite/Kryptonite.java (1)
Kryptonite(21-199)
src/main/java/net/lewmc/kryptonite/kos/gui/KOS_ConfigItemGui.java (3)
src/main/java/net/lewmc/kryptonite/Kryptonite.java (1)
Kryptonite(21-199)src/main/java/net/lewmc/kryptonite/config/LeafConfig.java (1)
LeafConfig(14-281)src/main/java/net/lewmc/kryptonite/config/MinecraftConfig.java (1)
MinecraftConfig(14-90)
src/main/java/net/lewmc/kryptonite/config/MinecraftConfig.java (4)
src/main/java/net/lewmc/kryptonite/Kryptonite.java (1)
Kryptonite(21-199)src/main/java/net/lewmc/kryptonite/utils/config/BooleanConfigItem.java (1)
BooleanConfigItem(12-95)src/main/java/net/lewmc/kryptonite/utils/config/ConfigCollection.java (1)
ConfigCollection(9-14)src/main/java/net/lewmc/kryptonite/utils/config/IntegerConfigItem.java (1)
IntegerConfigItem(12-125)
🪛 GitHub Actions: Publish project to LewMC Repo
pom.xml
[error] 1-1: Maven dependency resolution failed. Could not find artifact com.tchristofferson:ConfigUpdater:jar:2.1-SNAPSHOT in any configured repositories. Ensure the artifact is published or update repository definitions.
dependency-reduced-pom.xml
[error] 1-1: Maven dependency resolution failed. Could not find artifact com.tchristofferson:ConfigUpdater:jar:2.1-SNAPSHOT in any configured repositories. Ensure the artifact is published or update repository definitions.
🔇 Additional comments (2)
pom.xml (1)
131-136: Foundry SNAPSHOT dependency is available and accessible.Verification confirms that
net.lewmc:foundry:1.3.0-SNAPSHOTis published to the lewmc-snapshots repository and will be accessible during the build. No action required.dependency-reduced-pom.xml (1)
1-1: The original review comment is incorrect. ConfigUpdater 2.1-SNAPSHOT is available in the sonatype repository, so the artifact can be resolved. The latest published version on Maven Central is 2.2—2.1-SNAPSHOT is a snapshot-only version and would not appear in Maven Central. The dependency-reduced-pom.xml file is a normal output artifact generated by the maven-shade-plugin during the build process and does not indicate a build failure.Likely an incorrect or invalid review comment.
src/main/java/net/lewmc/kryptonite/kos/gui/KOS_ConfigItemGui.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (4)
src/main/java/net/lewmc/kryptonite/utils/config/GenericConfigItem.java (1)
79-92: Fix file resolution and extension checks inloadFile()(and align with subclass usage)Two issues here:
YAML path resolution is very likely wrong.
new File(this.plugin.getDataFolder(), "/../../"+file)passes a child path starting with a separator, so the parent is ignored and this resolves near filesystem root (e.g./pufferfish.yml), not the server root directory. That breaks lookups for files likepufferfish.ymlorconfig/leaf-global.yml.Extension checks are brittle.
Usingfile.contains(".yaml")/".yml"/".properties"is case‑sensitive and could misclassify or accidentally match other substrings.A safer approach is to derive the server root from the plugin data folder, normalize, and use case‑insensitive
endsWith, e.g.:- public void loadFile() { - if (file.contains(".properties")) { - this.propFile = new PropertiesUtil(plugin.getServer().getWorldContainer() + File.separator + file); - } else if (file.contains(".yaml") || file.contains(".yml")) { - this.yamlFile = new Files(plugin.foundryConfig, plugin); - this.yamlFile.loadNoReformat(new File(this.plugin.getDataFolder(),"/../../"+file).getAbsoluteFile()); - } else { - new Logger(plugin.foundryConfig).severe("Unable to load file '"+file+"' file extension not supported."); - new Logger(plugin.foundryConfig).severe("Expect additional errors."); - } - } + public void loadFile() { + // Derive server root from plugins/<name> data folder. + final File serverRoot = this.plugin.getDataFolder() + .getAbsoluteFile() + .getParentFile() // plugins/ + .getParentFile(); // server root + final File target = new File(serverRoot, this.file); + final String lower = this.file.toLowerCase(java.util.Locale.ROOT); + + if (lower.endsWith(".properties")) { + this.propFile = new PropertiesUtil(target.getPath()); + } else if (lower.endsWith(".yaml") || lower.endsWith(".yml")) { + this.yamlFile = new Files(plugin.foundryConfig, plugin); + this.yamlFile.loadNoReformat(target.getAbsoluteFile()); + } else { + Logger log = new Logger(plugin.foundryConfig); + log.severe("Unable to load file '" + file + "' file extension not supported."); + log.severe("Expect additional errors."); + } + }With this in place, the various
*Configclasses that pass plain filenames like"pufferfish.yml"or"config/leaf-global.yml"will resolve correctly under the server root, and extension detection will be robust.src/main/java/net/lewmc/kryptonite/kos/AutoKOS.java (1)
87-102: Critical: server.properties items may NPE without loading fileThis issue was previously flagged and remains unresolved. GenericConfigItem for .properties files doesn't auto-load the backing file, so
getValue()/setValue()calls on lines 93-97 can NPE when accessing the nullpropFile.Load the properties file before any setValue operations:
private void runVanilla() { if (this.softwareUtil.supportsMinecraft()) { this.log.info("[KOS] 1/6 - Running Vanilla optimisations"); MinecraftConfig m = new MinecraftConfig(this.plugin); + // Ensure server.properties is loaded before writes + m.values.forEach((k, item) -> { + if (item.getFile().contains(".properties")) { + item.loadFile(); + } + }); ((IntegerConfigItem)m.values.get(MinecraftConfig.Key.NETWORK_COMPRESSION_THRESHOLD.toString())).setValue(this.patches.getInt("server.network-compression-threshold")); ((IntegerConfigItem)m.values.get(MinecraftConfig.Key.SIMULATION_DISTANCE.toString())).setValue(this.patches.getInt("server.distance.simulation")); ((IntegerConfigItem)m.values.get(MinecraftConfig.Key.VIEW_DISTANCE.toString())).setValue(this.patches.getInt("server.distance.view")); ((BooleanConfigItem)m.values.get(MinecraftConfig.Key.SYNC_CHUNK_WRITES.toString())).setValue(this.patches.getBoolean("server.sync-chunk-writes")); ((BooleanConfigItem)m.values.get(MinecraftConfig.Key.ALLOW_FLIGHT.toString())).setValue(this.patches.getBoolean("server.allow-flight"));src/main/java/net/lewmc/kryptonite/kos/gui/KOS_ConfigItemGui.java (2)
47-69: Guard against non-player senders to prevent ClassCastExceptionThis issue was previously flagged and remains unresolved. The unchecked casts
(Player) this.useron lines 64 and 68 will throwClassCastExceptionif the command is invoked from console or command block.Apply this diff to add player validation:
public void show() { + if (!(this.user instanceof Player player)) { + this.user.sendMessage(ChatColor.RED + "This command can only be used by a player."); + return; + } + if (type == Kryptonite.ConfigurationOptions.MINECRAFT) { this.config = new MinecraftConfig(plugin); } else if (type == Kryptonite.ConfigurationOptions.BUKKIT) { this.config = new BukkitConfig(plugin); } else if (type == Kryptonite.ConfigurationOptions.SPIGOT) { this.config = new SpigotConfig(plugin); } else if (type == Kryptonite.ConfigurationOptions.PURPUR) { this.config = new PurpurConfig(plugin); } else if (type == Kryptonite.ConfigurationOptions.PUFFERFISH) { this.config = new PufferfishConfig(plugin); } else if (type == Kryptonite.ConfigurationOptions.LEAF) { this.config = new LeafConfig(plugin); } else { this.config = null; } - this.gui = new InventoryGui(this.plugin, (Player) this.user, "KOS - Server Configuration", this.getElements()); + this.gui = new InventoryGui(this.plugin, player, "KOS - Server Configuration", this.getElements()); this.addElements(); this.gui.build(); - this.gui.show((Player) this.user); + this.gui.show(player); }
207-236: Invalid layout when config is null; InventoryGui requires valid row stringsThis issue was previously flagged and remains unresolved. Returning empty-string rows
""on line 234 can break InventoryGui layout parsing. Each row must be exactly 9 characters.Apply this diff to provide a valid fallback layout:
} else { this.plugin.getLogger().severe("Unable to return elements: config is null."); - return new String[] {"",""," t u v "}; + return new String[] {" ", " t u v "}; }
🧹 Nitpick comments (4)
src/main/java/net/lewmc/kryptonite/utils/SoftwareUtil.java (1)
38-44: Consider avoiding repeated PufferfishConfig construction and raw cast indabEnabled()The logic is fine, but
dabEnabled()currently allocates a newPufferfishConfigand does a raw(BooleanConfigItem)cast on every call. You could centralize this behind a small helper onPufferfishConfig(e.g.,isDabEnabled(Kryptonite plugin)) or cache a singlePufferfishConfiginstance if this ends up being called frequently from GUIs.src/main/java/net/lewmc/kryptonite/kos/gui/KOS_PaperWorld_1.java (1)
27-32: Deprecation annotation is fine; addingsince/forRemovalis optionalClass- and Javadoc‑level deprecation are consistent with the rest of the PaperWorld GUIs. If you’re targeting Java 9+ and want to satisfy Sonar, you could switch to
@Deprecated(since = "2.1.0", forRemoval = false), but that’s only appropriate if your toolchain and runtime all target 9+.src/main/java/net/lewmc/kryptonite/kos/gui/KOS_ConfigItemGui.java (2)
74-100: Rename local variable to avoid shadowing the fieldThe local variable
configon line 78 shadows the instance field declared at line 29, which can lead to confusion and potential bugs.Apply this diff to rename the local variable:
private void addElements() { if (this.config != null) { int index = 0; - for (Map.Entry<String, GenericConfigItem> entry : this.config.values.entrySet()) { - GenericConfigItem<?> config = entry.getValue(); + for (Map.Entry<String, GenericConfigItem<?>> entry : this.config.values.entrySet()) { + GenericConfigItem<?> item = entry.getValue(); char id = (char) ('a' + index); - plugin.getLogger().info("Adding config element: id=" + id + " index=" + index + " name=" + config.getName()); + plugin.getLogger().info("Adding config element: id=" + id + " index=" + index + " name=" + item.getName()); - ItemStack display = buildDisplayItem(config); + ItemStack display = buildDisplayItem(item); this.gui.addElement(new StaticGuiElement( id, display, 1, click -> { - handleClick(config, click); + handleClick(item, click); return true; } )); index++; } KOS_GuiConstants consts = new KOS_GuiConstants(this.plugin, this.gui); consts.addConstants(); } }
194-196: Remove unused variable or suppress warningThe
stringItemvariable on line 194 is captured but never used due to the TODO placeholder.Consider either:
- Removing the variable binding until strings are implemented:
- } else if (config instanceof StringConfigItem stringItem) { + } else if (config instanceof StringConfigItem) { // TODO: Implement strings.
- Or suppressing the warning if you plan to implement this soon:
+ @SuppressWarnings("unused") } else if (config instanceof StringConfigItem stringItem) { // TODO: Implement strings.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
src/main/java/net/lewmc/kryptonite/config/BukkitConfig.java(1 hunks)src/main/java/net/lewmc/kryptonite/config/LeafConfig.java(1 hunks)src/main/java/net/lewmc/kryptonite/config/MinecraftConfig.java(1 hunks)src/main/java/net/lewmc/kryptonite/config/PufferfishConfig.java(1 hunks)src/main/java/net/lewmc/kryptonite/config/PurpurConfig.java(1 hunks)src/main/java/net/lewmc/kryptonite/config/SpigotConfig.java(1 hunks)src/main/java/net/lewmc/kryptonite/kos/AutoKOS.java(6 hunks)src/main/java/net/lewmc/kryptonite/kos/config/Bukkit.java(0 hunks)src/main/java/net/lewmc/kryptonite/kos/config/Pufferfish.java(0 hunks)src/main/java/net/lewmc/kryptonite/kos/config/Purpur.java(0 hunks)src/main/java/net/lewmc/kryptonite/kos/gui/KOS_BukkitGui.java(0 hunks)src/main/java/net/lewmc/kryptonite/kos/gui/KOS_ConfigItemGui.java(1 hunks)src/main/java/net/lewmc/kryptonite/kos/gui/KOS_ManualGUI.java(8 hunks)src/main/java/net/lewmc/kryptonite/kos/gui/KOS_PaperWorld_1.java(1 hunks)src/main/java/net/lewmc/kryptonite/kos/gui/KOS_PaperWorld_3.java(2 hunks)src/main/java/net/lewmc/kryptonite/kos/gui/KOS_PufferfishGui.java(0 hunks)src/main/java/net/lewmc/kryptonite/kos/gui/KOS_PurpurGui.java(0 hunks)src/main/java/net/lewmc/kryptonite/report/Report.java(1 hunks)src/main/java/net/lewmc/kryptonite/utils/SoftwareUtil.java(3 hunks)src/main/java/net/lewmc/kryptonite/utils/config/ConfigCollection.java(1 hunks)src/main/java/net/lewmc/kryptonite/utils/config/GenericConfigItem.java(1 hunks)
💤 Files with no reviewable changes (6)
- src/main/java/net/lewmc/kryptonite/kos/gui/KOS_PurpurGui.java
- src/main/java/net/lewmc/kryptonite/kos/config/Purpur.java
- src/main/java/net/lewmc/kryptonite/kos/config/Pufferfish.java
- src/main/java/net/lewmc/kryptonite/kos/gui/KOS_BukkitGui.java
- src/main/java/net/lewmc/kryptonite/kos/gui/KOS_PufferfishGui.java
- src/main/java/net/lewmc/kryptonite/kos/config/Bukkit.java
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/java/net/lewmc/kryptonite/config/MinecraftConfig.java
- src/main/java/net/lewmc/kryptonite/utils/config/ConfigCollection.java
🧰 Additional context used
🧬 Code graph analysis (11)
src/main/java/net/lewmc/kryptonite/utils/config/GenericConfigItem.java (2)
src/main/java/net/lewmc/kryptonite/Kryptonite.java (1)
Kryptonite(21-199)src/main/java/net/lewmc/kryptonite/utils/PropertiesUtil.java (1)
PropertiesUtil(8-48)
src/main/java/net/lewmc/kryptonite/config/LeafConfig.java (1)
src/main/java/net/lewmc/kryptonite/Kryptonite.java (1)
Kryptonite(21-199)
src/main/java/net/lewmc/kryptonite/config/PufferfishConfig.java (4)
src/main/java/net/lewmc/kryptonite/Kryptonite.java (1)
Kryptonite(21-199)src/main/java/net/lewmc/kryptonite/utils/config/BooleanConfigItem.java (1)
BooleanConfigItem(12-95)src/main/java/net/lewmc/kryptonite/utils/config/ConfigCollection.java (1)
ConfigCollection(9-14)src/main/java/net/lewmc/kryptonite/utils/config/IntegerConfigItem.java (1)
IntegerConfigItem(12-125)
src/main/java/net/lewmc/kryptonite/kos/gui/KOS_ManualGUI.java (1)
src/main/java/net/lewmc/kryptonite/Kryptonite.java (1)
Kryptonite(21-199)
src/main/java/net/lewmc/kryptonite/utils/SoftwareUtil.java (3)
src/main/java/net/lewmc/kryptonite/config/PufferfishConfig.java (1)
PufferfishConfig(14-161)src/main/java/net/lewmc/kryptonite/utils/config/BooleanConfigItem.java (1)
BooleanConfigItem(12-95)src/main/java/net/lewmc/kryptonite/Kryptonite.java (1)
Kryptonite(21-199)
src/main/java/net/lewmc/kryptonite/config/BukkitConfig.java (3)
src/main/java/net/lewmc/kryptonite/Kryptonite.java (1)
Kryptonite(21-199)src/main/java/net/lewmc/kryptonite/utils/config/ConfigCollection.java (1)
ConfigCollection(9-14)src/main/java/net/lewmc/kryptonite/utils/config/IntegerConfigItem.java (1)
IntegerConfigItem(12-125)
src/main/java/net/lewmc/kryptonite/kos/AutoKOS.java (8)
src/main/java/net/lewmc/kryptonite/Kryptonite.java (1)
Kryptonite(21-199)src/main/java/net/lewmc/kryptonite/config/BukkitConfig.java (1)
BukkitConfig(13-285)src/main/java/net/lewmc/kryptonite/config/MinecraftConfig.java (1)
MinecraftConfig(14-116)src/main/java/net/lewmc/kryptonite/config/PufferfishConfig.java (1)
PufferfishConfig(14-161)src/main/java/net/lewmc/kryptonite/config/PurpurConfig.java (1)
PurpurConfig(14-187)src/main/java/net/lewmc/kryptonite/utils/SoftwareUtil.java (1)
SoftwareUtil(7-45)src/main/java/net/lewmc/kryptonite/utils/config/BooleanConfigItem.java (1)
BooleanConfigItem(12-95)src/main/java/net/lewmc/kryptonite/utils/config/IntegerConfigItem.java (1)
IntegerConfigItem(12-125)
src/main/java/net/lewmc/kryptonite/config/SpigotConfig.java (2)
src/main/java/net/lewmc/kryptonite/utils/config/ConfigCollection.java (1)
ConfigCollection(9-14)src/main/java/net/lewmc/kryptonite/utils/config/StringConfigItem.java (1)
StringConfigItem(12-103)
src/main/java/net/lewmc/kryptonite/config/PurpurConfig.java (4)
src/main/java/net/lewmc/kryptonite/Kryptonite.java (1)
Kryptonite(21-199)src/main/java/net/lewmc/kryptonite/utils/config/BooleanConfigItem.java (1)
BooleanConfigItem(12-95)src/main/java/net/lewmc/kryptonite/utils/config/ConfigCollection.java (1)
ConfigCollection(9-14)src/main/java/net/lewmc/kryptonite/utils/config/IntegerConfigItem.java (1)
IntegerConfigItem(12-125)
src/main/java/net/lewmc/kryptonite/kos/gui/KOS_ConfigItemGui.java (1)
src/main/java/net/lewmc/kryptonite/Kryptonite.java (1)
Kryptonite(21-199)
src/main/java/net/lewmc/kryptonite/kos/gui/KOS_PaperWorld_1.java (5)
src/main/java/net/lewmc/kryptonite/kos/gui/KOS_PaperWorld_3.java (1)
Deprecated(21-903)src/main/java/net/lewmc/kryptonite/kos/config/PaperWorld.java (1)
Deprecated(12-390)src/main/java/net/lewmc/kryptonite/kos/gui/KOS_PaperWorld_2.java (1)
Deprecated(20-1061)src/main/java/net/lewmc/kryptonite/kos/gui/KOS_PaperWorld_4.java (1)
Deprecated(20-944)src/main/java/net/lewmc/kryptonite/kos/gui/KOS_PaperWorld_5.java (1)
Deprecated(20-788)
🪛 GitHub Check: SonarCloud Code Analysis
src/main/java/net/lewmc/kryptonite/config/LeafConfig.java
[failure] 91-91: Define a constant instead of duplicating this literal "async.async-entity-tracker.enabled" 6 times.
src/main/java/net/lewmc/kryptonite/config/BukkitConfig.java
[failure] 142-142: Define a constant instead of duplicating this literal "ticks-per.water-spawns" 3 times.
[failure] 112-112: Define a constant instead of duplicating this literal "ticks-per.monster-spawns" 3 times.
[failure] 189-189: Define a constant instead of duplicating this literal "ticks-per.ambient-spawns" 3 times.
[failure] 21-21: Define a constant instead of duplicating this literal "spawn-limits.monsters" 3 times.
[failure] 158-158: Define a constant instead of duplicating this literal "ticks-per.water-ambient-spawns" 3 times.
[failure] 81-81: Define a constant instead of duplicating this literal "spawn-limits.water-underground-creature" 3 times.
src/main/java/net/lewmc/kryptonite/report/Report.java
[warning] 28-28: This block of commented-out lines of code should be removed.
src/main/java/net/lewmc/kryptonite/kos/gui/KOS_ConfigItemGui.java
[warning] 48-48: Replace the chain of if/else with a switch expression.
[warning] 78-78: Rename "config" which hides the field declared at line 29.
[warning] 194-194: Remove this unused "stringItem" local variable.
[warning] 77-77: Provide the parametrized type for this generic.
[failure] 217-217: Move the update of "index" into this loop's update clause.
src/main/java/net/lewmc/kryptonite/kos/gui/KOS_PaperWorld_1.java
[warning] 31-31: Add 'since' and/or 'forRemoval' arguments to the @deprecated annotation.
🔇 Additional comments (7)
src/main/java/net/lewmc/kryptonite/report/Report.java (1)
20-20: The capability check method is properly implemented.Verification confirms that
supportsMinecraft()is defined inSoftwareUtil.javaat line 14 and is correctly called inReport.javaat line 20. The method exists and is consistently used throughout the codebase. No issues found.src/main/java/net/lewmc/kryptonite/config/PufferfishConfig.java (1)
14-160: Pufferfish config collection looks consistent and well‑structuredKey mapping, descriptions, and use of
BooleanConfigItem/IntegerConfigItemalign with the new config framework and Kryptonite’s server detection. No issues from my side.src/main/java/net/lewmc/kryptonite/kos/gui/KOS_PaperWorld_3.java (2)
17-22: Deprecation metadata onKOS_PaperWorld_3is consistent with the rest of the legacy GUIsMarking the class as deprecated (both Javadoc and annotation) cleanly signals the migration to the new config GUI without changing behavior.
40-42:dabEnabled()call correctly updated to match new SoftwareUtil APIUsing the no‑arg
sw.dabEnabled()here aligns with the refactored utility method and is appropriate since DAB is a global Pufferfish setting, not per‑user.src/main/java/net/lewmc/kryptonite/kos/gui/KOS_ManualGUI.java (2)
47-70: LGTM: Consistent migration to unified config GUIThe change from SERVER_PROPERTIES to MINECRAFT gating and the use of the unified
KOS_ConfigItemGuiconstructor pattern is consistent with the broader refactoring. The updated error messaging provides clear guidance to users.
209-230: LGTM: Element key update for Leaf resolves conflictChanging the Leaf element key from 'g' to 'l' correctly avoids collision with Paper Global's 'g' key, and the layout string on line 244 is updated accordingly.
src/main/java/net/lewmc/kryptonite/config/BukkitConfig.java (1)
18-218: LGTM: Consistent config structureThe BukkitConfig structure correctly follows the established pattern from other config classes (MinecraftConfig, PurpurConfig, PufferfishConfig). The use of IntegerConfigItem with appropriate bounds and ideal ranges provides good guardrails for configuration values.
| values.put("async.async-target-finding", new BooleanConfigItem( | ||
| file, | ||
| "async.async-target-finding", | ||
| "Async Target Finding", | ||
| List.of( | ||
| "Moves the expensive entity target search", | ||
| "calculations (finding nearby entities to attack or", | ||
| "interact with) to a background thread"), | ||
| true, | ||
| true, | ||
| plugin | ||
| )); | ||
|
|
||
| values.put("async.async-playerdata-save", new BooleanConfigItem( | ||
| file, | ||
| "async.async-playerdata-save", | ||
| "Async Playerdata Save", | ||
| List.of( | ||
| "Make playerdata saving aynchronous. Warning: might", | ||
| "cause data loss in some circumstances - use with", | ||
| "extreme caution and ensure robust backups!"), | ||
| true, | ||
| false, | ||
| plugin | ||
| )); |
There was a problem hiding this comment.
Clean up mislabelled config names/descriptions and consider a constant for repeated keys
Functionally this config wiring is fine, but there are a few user‑visible inconsistencies and some easy cleanup:
- Line 181+: description for
async.async-playerdata-savehas a typo:"aynchronous"→"asynchronous". - Lines 234–245:
async.async-pathfinding.queue-sizeis labelled"Async Entity Tracker Queue Size"but controls pathfinding; consider renaming to"Async Pathfinding Queue Size"(and tweak the description as needed). - Lines 248–259:
async.async-pathfinding.reject-policyreuses the"Async Entity Tracker Queue Size"name; this should be something like"Async Pathfinding Reject Policy"to match the description. - Lines 261–276:
async.async-mob-spawning.enabledis displayed as"Async Pathfinding"; that should be"Async Mob Spawning"or similar.
Also, the literal "async.async-entity-tracker.enabled" is repeated several times as a map key and dependency source. If you care about the Sonar warning and readability, you can introduce a small constant, e.g.:
private static final String ASYNC_ENTITY_TRACKER_ENABLED = "async.async-entity-tracker.enabled";and use that in the values.put(...) and dependency lookups.
These are all non‑breaking UX/readability fixes you can apply when convenient.
Also applies to: 234-259, 261-276
🤖 Prompt for AI Agents
In src/main/java/net/lewmc/kryptonite/config/LeafConfig.java around lines
167-191 (and also apply similar changes at 234-259 and 261-276), fix the
user-facing labels and typo and optionally introduce a constant for a repeated
map key: correct the typo "aynchronous" to "asynchronous" in the
async-playerdata-save description; rename the label for
async.async-pathfinding.queue-size from "Async Entity Tracker Queue Size" to
"Async Pathfinding Queue Size" and adjust its description to reference
pathfinding queue size; rename async.async-pathfinding.reject-policy's label to
"Async Pathfinding Reject Policy"; change the label for
async.async-mob-spawning.enabled from "Async Pathfinding" to "Async Mob
Spawning"; and add a private static final String (e.g.,
ASYNC_ENTITY_TRACKER_ENABLED = "async.async-entity-tracker.enabled") to replace
duplicated literal uses in values.put and dependency lookups for readability and
to silence static analysis.
| values.put(Key.DOLPHIN_DISABLE_TREASURE_SEARCHING.toString(), new BooleanConfigItem( | ||
| file, | ||
| Key.DOLPHIN_DISABLE_TREASURE_SEARCHING.toString(), | ||
| "Villager Search Radius (Nearest Bed Sensor)", | ||
| List.of( | ||
| "Stops the dolphin from treasure hunting. Will impact", | ||
| "vanilla behaviour."), | ||
| true, | ||
| true, | ||
| plugin | ||
| )); |
There was a problem hiding this comment.
Fix display name for DOLPHIN_DISABLE_TREASURE_SEARCHING
The config item at lines 105–115 uses the name "Villager Search Radius (Nearest Bed Sensor)" but actually controls dolphin.disable-treasure-searching. This looks like a copy/paste error; renaming to something like "Dolphin Disable Treasure Searching" will make the GUI clearer.
🤖 Prompt for AI Agents
In src/main/java/net/lewmc/kryptonite/config/PurpurConfig.java around lines 105
to 115, the display name for Key.DOLPHIN_DISABLE_TREASURE_SEARCHING is incorrect
(leftover "Villager Search Radius (Nearest Bed Sensor)"); change that string to
a correct, descriptive label such as "Dolphin Disable Treasure Searching" (or
similar clear wording) so the GUI reflects the actual config key, leaving the
rest of the BooleanConfigItem parameters unchanged.
| /** | ||
| * Configuration data for purpur.yml | ||
| * @since 2.1.0 | ||
| */ | ||
| public class SpigotConfig extends ConfigCollection { | ||
| /** | ||
| * Constructs the purpur.yml data. | ||
| * @param plugin Kryptonite - Reference to the main Kryptonite class. | ||
| */ | ||
| public SpigotConfig(Kryptonite plugin) { |
There was a problem hiding this comment.
Fix Javadoc to refer to spigot.yml instead of purpur.yml
The class and constructor Javadocs still mention purpur.yml, but this class represents spigot.yml. Updating those strings will avoid confusion when navigating the config classes.
🤖 Prompt for AI Agents
In src/main/java/net/lewmc/kryptonite/config/SpigotConfig.java around lines 9 to
18, update the Javadoc comments that currently reference "purpur.yml" to instead
reference "spigot.yml": change the class-level Javadoc description and the
constructor Javadoc param description (if it mentions purpur.yml) so they
correctly state this class represents spigot.yml, keeping the rest of the
Javadoc text intact.
| public void runReport(Kryptonite plugin, CommandSender user) { | ||
| this.plugin = plugin; | ||
| this.user = user; | ||
| this.message = new MessageUtil(user); | ||
| this.softwareUtil = new SoftwareUtil(this.plugin); | ||
| } |
There was a problem hiding this comment.
Critical: Report execution is incomplete.
The runReport() method initializes all necessary fields but never calls runKOSReport(), meaning no actual reporting occurs. This renders the Report class non-functional.
Apply this diff to fix:
public void runReport(Kryptonite plugin, CommandSender user) {
this.plugin = plugin;
this.message = new MessageUtil(user);
this.softwareUtil = new SoftwareUtil(this.plugin);
+ this.runKOSReport();
}🤖 Prompt for AI Agents
In src/main/java/net/lewmc/kryptonite/report/Report.java around lines 13 to 17,
the runReport method only initializes fields but never executes the reporting
logic; call the runKOSReport() method after initializing plugin, message, and
softwareUtil so the report actually runs (ensure you call it on the same
instance, handle any checked exceptions or threading concerns as appropriate).
| //Bukkit bukkit = new Bukkit(this.plugin, this.user); | ||
| //for (Bukkit.Key item : Bukkit.Key.values()) { | ||
|
|
||
| } | ||
| //} |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove the commented-out code.
The commented Bukkit reporting logic should be removed entirely to improve code maintainability. If this functionality is needed in the future, it can be retrieved from version control.
As per coding guidelines.
Apply this diff to remove the commented code:
if (this.softwareUtil.supportsCraftBukkit()) {
message.Info("Reporting [2/7] Gathering Bukkit configuration.");
- //Bukkit bukkit = new Bukkit(this.plugin, this.user);
- //for (Bukkit.Key item : Bukkit.Key.values()) {
-
- //}
} else {
message.Info("Reporting [2/7] Bukkit not supported, skipping...");
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| //Bukkit bukkit = new Bukkit(this.plugin, this.user); | |
| //for (Bukkit.Key item : Bukkit.Key.values()) { | |
| } | |
| //} | |
| if (this.softwareUtil.supportsCraftBukkit()) { | |
| message.Info("Reporting [2/7] Gathering Bukkit configuration."); | |
| } else { | |
| message.Info("Reporting [2/7] Bukkit not supported, skipping..."); | |
| } |
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[warning] 28-28: This block of commented-out lines of code should be removed.
🤖 Prompt for AI Agents
In src/main/java/net/lewmc/kryptonite/report/Report.java around lines 28 to 31,
remove the commented-out Bukkit reporting code (the two commented lines and any
surrounding empty comment blocks) so the file no longer contains vestigial
commented logic; commit the cleaned file (rely on VCS to restore if needed
later).
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/main/java/net/lewmc/kryptonite/utils/config/GenericConfigItem.java (1)
78-88: Critical: Unsafe path resolution can point to root or fail.The path construction in lines 80 and 83 is fragile:
- Line 80: Using
getWorldContainer() + File.separator + filemay produce unexpected paths iffilecontains directory separators or if the world container isn't the server root.- Line 83: The pattern
"/../../"+filebecomes an absolute path/../../bukkit.yml→/bukkit.ymlon UNIX systems, ignoring the parent directory entirely.- Lines 79, 81: Extension checks using
.contains()are case-sensitive and match substrings (e.g., "my.PROPERTIES.backup" would incorrectly match).Apply this diff to fix the path resolution and extension checks:
public void loadFile() { - if (file.contains(".properties")) { - this.propFile = new PropertiesUtil(plugin.getServer().getWorldContainer() + File.separator + file); - } else if (file.contains(".yaml") || file.contains(".yml")) { + // Resolve server root: plugins/Kryptonite -> plugins -> server root + final File serverRoot = this.plugin.getDataFolder().getAbsoluteFile().getParentFile().getParentFile(); + final File target = new File(serverRoot, this.file); + final String lower = this.file.toLowerCase(java.util.Locale.ROOT); + + if (lower.endsWith(".properties")) { + this.propFile = new PropertiesUtil(target.getPath()); + } else if (lower.endsWith(".yaml") || lower.endsWith(".yml")) { this.yamlFile = new Files(plugin.foundryConfig, plugin); - this.yamlFile.loadNoReformat(new File(this.plugin.getDataFolder(),"/../../"+file).getAbsoluteFile()); + this.yamlFile.loadNoReformat(target); } else { Logger log = new Logger(plugin.foundryConfig); log.severe("Unable to load file '" + file + "' file extension not supported."); log.severe("Expect additional errors."); } }src/main/java/net/lewmc/kryptonite/config/SpigotConfig.java (1)
12-13: Fix Javadoc references from purpur.yml to spigot.yml.The class and constructor Javadocs reference "purpur.yml" but this class represents "spigot.yml" configuration.
Apply this diff:
/** - * Configuration data for purpur.yml + * Configuration data for spigot.yml * @since 2.1.0 */ public class SpigotConfig extends ConfigCollection { /** - * Constructs the purpur.yml data. + * Constructs the spigot.yml data. * @param plugin Kryptonite - Reference to the main Kryptonite class. */Also applies to: 17-17
🧹 Nitpick comments (5)
src/main/java/net/lewmc/kryptonite/utils/CompatablityUtil.java (1)
20-36: Refactor growing OR-chain for readability and easier future additionsThe compatibility check is still correct, but the long
||chain with repeatedp.getName()calls is getting harder to maintain as more plugins are added. A small refactor using a local variable for the name would reduce duplication and make future edits less error‑prone:- for (Plugin p : plugins) { - if ( - p.getName().equalsIgnoreCase("ClearLag") || - p.getName().equalsIgnoreCase("EntityClearer") || - p.getName().equalsIgnoreCase("StackMob") || - p.getName().equalsIgnoreCase("PluginManager+") || - p.getName().equalsIgnoreCase("PluginManager") || - p.getName().equalsIgnoreCase("AutoPluginLoader") || - p.getName().equalsIgnoreCase("PlugMan") || - p.getName().equalsIgnoreCase("PlugManX") || - p.getName().equalsIgnoreCase("WildStacker") || - p.getName().equalsIgnoreCase("FarmLimiter") || - p.getName().equalsIgnoreCase("ChunkSpawnerLimiter") || - p.getName().equalsIgnoreCase("AntiLagX") || - p.getName().equalsIgnoreCase("WildTools") || - p.getName().equalsIgnoreCase("AutoClear") - ) { - badPlugins.add(p.getName()); - } - } + for (Plugin p : plugins) { + String name = p.getName(); + if ( + name.equalsIgnoreCase("ClearLag") || + name.equalsIgnoreCase("EntityClearer") || + name.equalsIgnoreCase("StackMob") || + name.equalsIgnoreCase("PluginManager+") || + name.equalsIgnoreCase("PluginManager") || + name.equalsIgnoreCase("AutoPluginLoader") || + name.equalsIgnoreCase("PlugMan") || + name.equalsIgnoreCase("PlugManX") || + name.equalsIgnoreCase("WildStacker") || + name.equalsIgnoreCase("FarmLimiter") || + name.equalsIgnoreCase("ChunkSpawnerLimiter") || + name.equalsIgnoreCase("AntiLagX") || + name.equalsIgnoreCase("WildTools") || + name.equalsIgnoreCase("AutoClear") + ) { + badPlugins.add(name); + } + }If this list keeps growing, a follow‑up could move the names into a static
Set<String>and usecontains, but the above keeps the current behavior with minimal change.src/main/java/net/lewmc/kryptonite/utils/config/GenericConfigItem.java (1)
66-66: Consider making constructor protected.Since
GenericConfigItemis abstract and only meant to be extended, making the constructorprotectedprevents direct instantiation attempts and signals the inheritance contract more clearly.Based on learnings
Apply this diff:
- public GenericConfigItem(String file, String key, String name, List<String> description, Boolean dependencyIsEnabled, Kryptonite plugin) { + protected GenericConfigItem(String file, String key, String name, List<String> description, Boolean dependencyIsEnabled, Kryptonite plugin) { this.file = file; this.key = key; this.name = name;src/main/java/net/lewmc/kryptonite/Kryptonite.java (1)
37-37: Consider encapsulating foundryConfig with a getter.While the current public field works, providing a getter method would improve encapsulation and allow for future flexibility (validation, lazy initialization, etc.).
If you prefer to keep the field public for simpler access patterns common in Bukkit plugins, that's acceptable. Otherwise, consider:
- public FoundryConfig foundryConfig; + private FoundryConfig foundryConfig; + + public FoundryConfig getFoundryConfig() { + return foundryConfig; + }Then update all references from
plugin.foundryConfigtoplugin.getFoundryConfig().src/main/java/net/lewmc/kryptonite/config/BukkitConfig.java (1)
117-203: Reduce duplication by extracting repeated description prefix.The phrase "How often (in ticks) the server attempts to spawn" is repeated across 6 spawn-rate config items. Extract it to a constant to improve maintainability.
Apply this diff:
public BukkitConfig(Kryptonite plugin) { String file = "bukkit.yml"; + final String TICKS_DESC_PREFIX = "How often (in ticks) the server attempts to spawn"; values.put(Key.SPAWN_LIMITS_MONSTERS.toString(), new IntegerConfigItem( file, @@ -114,9 +115,9 @@ values.put(Key.TICKS_PER_MONSTER_SPAWNS.toString(), new IntegerConfigItem( file, Key.TICKS_PER_MONSTER_SPAWNS.toString(), "Ticks Per Monster Spawns", List.of( - "How often (in ticks) the server attempts to spawn", + TICKS_DESC_PREFIX, "monsters. The server has less work to do with lower", "numbers, but survival worlds may be less realistic."),Apply similar changes to the other 5 spawn rate items (ANIMAL_SPAWNS, WATER_SPAWNS, WATER_AMBIENT_SPAWNS, AXOLOTL_SPAWNS, AMBIENT_SPAWNS).
src/main/java/net/lewmc/kryptonite/config/SpigotConfig.java (1)
67-150: Consider extracting repeated activation range description prefix.The phrase "The distance the server will 'activate'" appears 6 times. While this is a minor duplication, extracting it to a constant improves maintainability.
Apply this diff:
public SpigotConfig(Kryptonite plugin) { String file = "spigot.yml"; + final String ACTIVATION_DESC = "The distance the server will 'activate'"; int simulationDistance = ((int) new MinecraftConfig(plugin).values.get("simulation-distance").getValue() -1)*16; @@ -65,7 +66,7 @@ Key.ENTITY_ACTIVATION_RANGE_ANIMALS.toString(), "Entity Activation Range (Animals)", - List.of("The distance the server will 'activate'", + List.of(ACTIVATION_DESC, "animal entities."),Apply similar changes to the other 5 activation range descriptions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/main/java/net/lewmc/kryptonite/Kryptonite.java(7 hunks)src/main/java/net/lewmc/kryptonite/config/BukkitConfig.java(1 hunks)src/main/java/net/lewmc/kryptonite/config/SpigotConfig.java(1 hunks)src/main/java/net/lewmc/kryptonite/kos/AutoKOS.java(7 hunks)src/main/java/net/lewmc/kryptonite/kos/config/Spigot.java(0 hunks)src/main/java/net/lewmc/kryptonite/kos/gui/KOS_AutoGUI.java(2 hunks)src/main/java/net/lewmc/kryptonite/kos/gui/KOS_SpigotGui_1.java(0 hunks)src/main/java/net/lewmc/kryptonite/kos/gui/KOS_SpigotGui_2.java(0 hunks)src/main/java/net/lewmc/kryptonite/utils/CompatablityUtil.java(1 hunks)src/main/java/net/lewmc/kryptonite/utils/ConfigurationUtil.java(3 hunks)src/main/java/net/lewmc/kryptonite/utils/config/GenericConfigItem.java(1 hunks)
💤 Files with no reviewable changes (3)
- src/main/java/net/lewmc/kryptonite/kos/config/Spigot.java
- src/main/java/net/lewmc/kryptonite/kos/gui/KOS_SpigotGui_2.java
- src/main/java/net/lewmc/kryptonite/kos/gui/KOS_SpigotGui_1.java
🧰 Additional context used
🧬 Code graph analysis (5)
src/main/java/net/lewmc/kryptonite/utils/config/GenericConfigItem.java (2)
src/main/java/net/lewmc/kryptonite/Kryptonite.java (1)
Kryptonite(21-196)src/main/java/net/lewmc/kryptonite/utils/PropertiesUtil.java (1)
PropertiesUtil(8-48)
src/main/java/net/lewmc/kryptonite/config/SpigotConfig.java (5)
src/main/java/net/lewmc/kryptonite/Kryptonite.java (1)
Kryptonite(21-196)src/main/java/net/lewmc/kryptonite/utils/config/BooleanConfigItem.java (1)
BooleanConfigItem(12-95)src/main/java/net/lewmc/kryptonite/utils/config/ConfigCollection.java (1)
ConfigCollection(9-14)src/main/java/net/lewmc/kryptonite/utils/config/IntegerConfigItem.java (1)
IntegerConfigItem(12-125)src/main/java/net/lewmc/kryptonite/utils/config/StringConfigItem.java (1)
StringConfigItem(12-103)
src/main/java/net/lewmc/kryptonite/config/BukkitConfig.java (2)
src/main/java/net/lewmc/kryptonite/utils/config/ConfigCollection.java (1)
ConfigCollection(9-14)src/main/java/net/lewmc/kryptonite/utils/config/IntegerConfigItem.java (1)
IntegerConfigItem(12-125)
src/main/java/net/lewmc/kryptonite/kos/AutoKOS.java (12)
src/main/java/net/lewmc/kryptonite/Kryptonite.java (1)
Kryptonite(21-196)src/main/java/net/lewmc/kryptonite/utils/ConfigurationUtil.java (1)
ConfigurationUtil(14-101)src/main/java/net/lewmc/kryptonite/utils/MessageUtil.java (1)
MessageUtil(6-28)src/main/java/net/lewmc/kryptonite/utils/SoftwareUtil.java (1)
SoftwareUtil(7-45)src/main/java/net/lewmc/kryptonite/utils/config/BooleanConfigItem.java (1)
BooleanConfigItem(12-95)src/main/java/net/lewmc/kryptonite/utils/config/IntegerConfigItem.java (1)
IntegerConfigItem(12-125)src/main/java/net/lewmc/kryptonite/utils/config/StringConfigItem.java (1)
StringConfigItem(12-103)src/main/java/net/lewmc/kryptonite/config/MinecraftConfig.java (1)
MinecraftConfig(14-116)src/main/java/net/lewmc/kryptonite/config/BukkitConfig.java (1)
BukkitConfig(13-285)src/main/java/net/lewmc/kryptonite/config/SpigotConfig.java (1)
SpigotConfig(15-371)src/main/java/net/lewmc/kryptonite/config/PurpurConfig.java (1)
PurpurConfig(14-187)src/main/java/net/lewmc/kryptonite/config/PufferfishConfig.java (1)
PufferfishConfig(14-161)
src/main/java/net/lewmc/kryptonite/Kryptonite.java (3)
src/main/java/net/lewmc/kryptonite/event/JoinEvent.java (1)
JoinEvent(12-44)src/main/java/net/lewmc/kryptonite/utils/CompatablityUtil.java (1)
CompatablityUtil(8-43)src/main/java/net/lewmc/kryptonite/utils/UpdateUtil.java (1)
UpdateUtil(13-104)
🪛 GitHub Check: SonarCloud Code Analysis
src/main/java/net/lewmc/kryptonite/utils/config/GenericConfigItem.java
[warning] 66-66: Change the visibility of this constructor to "protected".
src/main/java/net/lewmc/kryptonite/config/SpigotConfig.java
[failure] 67-67: Define a constant instead of duplicating this literal "The distance the server will 'activate'" 6 times.
src/main/java/net/lewmc/kryptonite/config/BukkitConfig.java
[failure] 117-117: Define a constant instead of duplicating this literal "How often (in ticks) the server attempts to spawn" 6 times.
src/main/java/net/lewmc/kryptonite/Kryptonite.java
[warning] 37-37: Make foundryConfig a static final constant or non-public and provide accessors if needed.
🔇 Additional comments (7)
src/main/java/net/lewmc/kryptonite/utils/ConfigurationUtil.java (1)
25-28: VerifyfoundryConfiginitialization order.The constructor now depends on
plugin.foundryConfig. Ensure thatfoundryConfigis fully initialized in theKryptoniteplugin class before anyConfigurationUtilinstances are created, otherwise this will throw aNullPointerExceptionat runtime.Run the following script to verify the initialization order:
src/main/java/net/lewmc/kryptonite/Kryptonite.java (2)
4-5: LGTM! Foundry integration and LEAF support added correctly.The migration to Foundry's logging system and the addition of LEAF configuration support are implemented correctly. The enum change from
SERVER_PROPERTIEStoMINECRAFTbetter reflects the actual configuration type.Also applies to: 23-23, 41-44, 131-131, 171-175
41-44: Verify config availability before FoundryConfig initialization.Line 42 calls
this.getConfig().getBoolean("verbose")beforethis.saveDefaultConfig()is invoked (line 90). Ensure that Bukkit's auto-loading of config.yml happens beforeonEnable()or that missing config values have safe defaults.Run the following to verify the initialization order is safe:
src/main/java/net/lewmc/kryptonite/kos/gui/KOS_AutoGUI.java (1)
5-5: LGTM! Logging migration completed correctly.The migration from the old logging utility to Foundry's Logger is implemented correctly and follows the same pattern used throughout the codebase.
Also applies to: 53-53, 114-114
src/main/java/net/lewmc/kryptonite/kos/AutoKOS.java (2)
3-3: LGTM! Config refactoring successfully migrates to typed config items.The refactoring from direct YAML/properties manipulation to typed config items (
MinecraftConfig,BukkitConfig,SpigotConfig,PurpurConfig,PufferfishConfig) is well-structured and maintains the existing logic while providing better type safety and encapsulation.Also applies to: 10-12, 22-22, 34-34, 86-351
85-100: Verify that config items load their files before write operations.The code calls
setValue()on config items without explicitly callingloadFile()first (lines 91-95). Ensure that each config item type'ssetValue()implementation callsloadFile()internally to avoid NPE on the backing file handlers.Based on the relevant snippets,
IntegerConfigItem.setValue()andBooleanConfigItem.setValue()both callthis.loadFile()internally, so this should be safe. However, verify with:src/main/java/net/lewmc/kryptonite/config/BukkitConfig.java (1)
248-251: Verify SPAWN_LIMITS_AMBIENT enum is used or remove it.The
Key.SPAWN_LIMITS_AMBIENTenum constant exists but I don't see a correspondingvalues.put()call in the constructor. Either the config item is missing or the enum constant is unused.Run the following to check if SPAWN_LIMITS_AMBIENT is used elsewhere:
| values.put(Key.MOB_SPAWN_RANGE.toString(), new IntegerConfigItem( | ||
| file, | ||
| Key.MOB_SPAWN_RANGE.toString(), | ||
| "Mob Spawn Range", | ||
| List.of("The distance the server will spawn mobs."), | ||
| true, | ||
| 1, | ||
| 100, | ||
| "3-8", | ||
| plugin | ||
| )); | ||
|
|
||
| values.put(Key.MOB_SPAWN_RANGE.toString(), new IntegerConfigItem( | ||
| file, | ||
| Key.MOB_SPAWN_RANGE.toString(), | ||
| "Entity Activation Range (Mobs)", | ||
| List.of("The distance the server will 'activate' mob", | ||
| "entities."), | ||
| true, | ||
| 1, | ||
| 100, | ||
| "16-"+simulationDistance, | ||
| plugin | ||
| )); |
There was a problem hiding this comment.
Critical: Duplicate map key overwrites first MOB_SPAWN_RANGE entry.
Lines 38-48 and 50-61 both use Key.MOB_SPAWN_RANGE.toString() as the map key. The second entry (intended as "Entity Activation Range (Mobs)") will overwrite the first "Mob Spawn Range" entry, losing that configuration item entirely.
The second entry should likely use a different key. Looking at the enum definition, there's no ENTITY_ACTIVATION_RANGE_MOBS key, so you may need to either:
- Add the missing enum constant, or
- Use an existing key like
ENTITY_ACTIVATION_RANGE_MONSTERSif this was intended to configure that
Apply this diff to fix (assuming you need a new enum constant):
@@ -47,9 +47,9 @@
plugin
));
- values.put(Key.MOB_SPAWN_RANGE.toString(), new IntegerConfigItem(
+ values.put(Key.ENTITY_ACTIVATION_RANGE_MOBS.toString(), new IntegerConfigItem(
file,
- Key.MOB_SPAWN_RANGE.toString(),
+ Key.ENTITY_ACTIVATION_RANGE_MOBS.toString(),
"Entity Activation Range (Mobs)",
List.of("The distance the server will 'activate' mob",
"entities."),
@@ -290,6 +290,10 @@
/**
* Configuration values supported by this format.
*/
public enum Key {
+ ENTITY_ACTIVATION_RANGE_MOBS {
+ @Override
+ public String toString() { return "world-settings.default.entity-activation-range.mobs"; }
+ },
VIEW_DISTANCE {
@Override
public String toString() { return "world-settings.default.view-distance"; }Verify which config key this should actually map to in the Spigot documentation.
Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (7)
src/main/java/net/lewmc/kryptonite/config/LeafConfig.java (7)
105-165: Same NPE risk in async entity tracker dependencies.Lines 115, 129, 143, and 160 have the identical critical issue: calling
.getValue()on config items during construction will cause aNullPointerException. This is the same architectural problem flagged in the previous segment and in past reviews.Additionally, use
Key.ASYNC_ENTITY_TRACKER_ENABLED.toString()instead of the literal string"async.async-entity-tracker.enabled"for consistency.
204-232: Same NPE risk in async pathfinding dependencies.Lines 213 and 227 have the same critical issue: calling
.getValue()during construction will cause aNullPointerException. UseKey.ASYNC_PATHFINDING_ENABLED.toString()instead of the literal string.
261-276: Fix incorrect config label - STILL UNRESOLVED.Line 264 has an incorrect label: it says
"Async Pathfinding"but this config item is for async mob spawning. It should be"Async Mob Spawning". This was flagged in previous reviews but remains unfixed.- "Async Pathfinding", + "Async Mob Spawning", List.of( "Whether asynchronous mob spawning calculations should",
180-191: Fix typo in user-facing description - STILL UNRESOLVED.Line 185 contains a typo:
"aynchronous"should be"asynchronous". This was flagged in previous reviews but remains unfixed.- "Make playerdata saving aynchronous. Warning: might", + "Make playerdata saving asynchronous. Warning: might",
234-246: Fix mislabeled config name and NPE risk - STILL UNRESOLVED.Line 237 has an incorrect label: it says
"Async Entity Tracker Queue Size"but should be"Async Pathfinding Queue Size"since this config item is for pathfinding, not entity tracking. This was flagged in previous reviews but remains unfixed.Additionally, line 241 has the same NPE risk from calling
.getValue()during construction.- "Async Entity Tracker Queue Size", + "Async Pathfinding Queue Size", List.of("Maximum size of the queue for pending", "pathfinding tasks. If set to 0, the queue size", - "dynamically calculated as max-threads * 256."), + "is dynamically calculated as max-threads * 256."), - (Boolean) values.get("async.async-pathfinding.enabled").getValue(), + (Boolean) values.get(Key.ASYNC_PATHFINDING_ENABLED.toString()).getValue(), // Note: still needs lazy evaluation fixNote: The description text also has a minor grammar issue ("dynamically calculated" should be "is dynamically calculated").
248-259: Fix incorrect config label and NPE risk - STILL UNRESOLVED.Line 251 has a severely incorrect label: it says
"Async Entity Tracker Queue Size"but this config item is for the pathfinding reject policy. It should be"Async Pathfinding Reject Policy". This was flagged in previous reviews but remains unfixed.Additionally, line 255 has the same NPE risk from calling
.getValue()during construction.- "Async Entity Tracker Queue Size", + "Async Pathfinding Reject Policy", List.of("The policy to use when the pathfinding task queue", "is full (only relevant if queue-size is > 0) and a", "new task is submitted"), - (Boolean) values.get("async.async-pathfinding.enabled").getValue(), + (Boolean) values.get(Key.ASYNC_PATHFINDING_ENABLED.toString()).getValue(), // Note: still needs lazy evaluation fix
35-89: Critical: NPE risk from calling getValue() during construction - STILL UNRESOLVED.Lines 43, 58, 72, and 86 call
values.get("async.parallel-world-ticking.enabled").getValue()during construction. This was flagged in previous reviews as a critical issue but remains unaddressed. This will cause aNullPointerExceptionbecause configuration files are not loaded at constructor time, sogetValue()will attempt to read from an uninitialized file.Additionally, these lines use the literal string
"async.parallel-world-ticking.enabled"instead of the enum constantKey.ASYNC_PARALLEL_WORLD_TICKING_ENABLED.toString(), which is inconsistent with the pattern used for the keys being defined.Recommended solution:
You need to implement lazy dependency evaluation. Instead of calling
.getValue()during construction, pass the dependency reference and evaluate it lazily when needed. This requires either:
- Modifying the
ConfigItemconstructors to accept aGenericConfigItemreference for dependencies and evaluate lazily, OR- Using a two-phase initialization pattern where dependencies are resolved after files are loaded.
For the literal string issue, use the enum constant:
- (Boolean) values.get("async.parallel-world-ticking.enabled").getValue(), + (Boolean) values.get(Key.ASYNC_PARALLEL_WORLD_TICKING_ENABLED.toString()).getValue(),However, this still doesn't fix the NPE issue - you need architectural changes to support lazy evaluation.
🧹 Nitpick comments (1)
src/main/java/net/lewmc/kryptonite/config/LeafConfig.java (1)
279-355: Good use of enum for configuration keys, but apply consistently.The
Keyenum is a good practice for centralizing configuration key definitions. However, as noted in the static analysis hints and previous comments, this enum should be used consistently throughout the constructor. Currently, it's used for the primary keys (first parameter to ConfigItem constructors) but literal strings are used when looking up dependencies.Based on static analysis.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/net/lewmc/kryptonite/config/LeafConfig.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/main/java/net/lewmc/kryptonite/config/LeafConfig.java (1)
src/main/java/net/lewmc/kryptonite/Kryptonite.java (1)
Kryptonite(21-196)
🪛 GitHub Check: SonarCloud Code Analysis
src/main/java/net/lewmc/kryptonite/config/LeafConfig.java
[failure] 153-153: Define a constant instead of duplicating this literal "Async Entity Tracker Queue Size" 3 times.
[failure] 115-115: Define a constant instead of duplicating this literal "async.async-entity-tracker.enabled" 5 times.
[failure] 43-43: Define a constant instead of duplicating this literal "async.parallel-world-ticking.enabled" 5 times.
🔇 Additional comments (1)
src/main/java/net/lewmc/kryptonite/config/LeafConfig.java (1)
18-33: LGTM! Constructor initialization and first config item are well-structured.The file path construction and the first config item declaration are correct. Using the
Keyenum for configuration keys is a good practice.
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
src/main/java/net/lewmc/kryptonite/utils/config/DoubleConfigItem.java (3)
79-87: Close YAML handle after save and use.endsWith()for extension checks.The YAML file handle is not closed after
yamlFile.save(), which can leak file descriptors or lock the file on Windows. Additionally, extension checks should use.endsWith()instead of.contains().Apply this diff:
@Override public void setValue(Double value) { - if (this.file.contains(".properties")) { + if (this.file.endsWith(".properties")) { propFile.setProperty(this.key, String.valueOf(value)); - } else if (file.contains(".yml") || file.contains(".yaml")) { + } else if (this.file.endsWith(".yml") || this.file.endsWith(".yaml")) { this.loadFile(); yamlFile.set(this.key, value); yamlFile.save(); + yamlFile.close(); } }
104-130: Replace exact double equality with epsilon comparison and add exception handling.Line 129 uses exact
==for double comparison, which is unreliable due to floating-point precision issues. Additionally,NumberFormatExceptionfrom parsing is not handled.Apply this diff:
@Override public boolean isIdeal() { if (idealValue == null) { return true; } - double current = this.getValue(); - String trimmed = idealValue.trim(); - - if (trimmed.contains("-")) { - String[] parts = trimmed.split("-"); - double minIdeal = Double.parseDouble(parts[0].trim()); - double maxIdeal = Double.parseDouble(parts[1].trim()); - return current >= minIdeal && current <= maxIdeal; - } - - if (trimmed.startsWith(">")) { - double threshold = Double.parseDouble(trimmed.substring(1).trim()); - return current > threshold; - } - - if (trimmed.startsWith("<")) { - double threshold = Double.parseDouble(trimmed.substring(1).trim()); - return current < threshold; - } - - return current == Double.parseDouble(trimmed); + try { + double current = this.getValue(); + String trimmed = idealValue.trim(); + + if (trimmed.contains("-")) { + String[] parts = trimmed.split("-"); + double minIdeal = Double.parseDouble(parts[0].trim()); + double maxIdeal = Double.parseDouble(parts[1].trim()); + return current >= minIdeal && current <= maxIdeal; + } + + if (trimmed.startsWith(">")) { + double threshold = Double.parseDouble(trimmed.substring(1).trim()); + return current > threshold; + } + + if (trimmed.startsWith("<")) { + double threshold = Double.parseDouble(trimmed.substring(1).trim()); + return current < threshold; + } + + double target = Double.parseDouble(trimmed); + return Math.abs(current - target) < 1e-9; + } catch (NumberFormatException ex) { + new Logger(this.plugin.foundryConfig).severe("Invalid idealValue '"+idealValue+"' for key '"+this.key+"'"); + return false; + } }
52-72: Use.endsWith()instead of.contains()for file extension checks.Lines 53 and 63 use
.contains()to check file extensions, which can match unwanted substrings (e.g., a file named "my.properties.backup" would incorrectly match).Apply this diff:
@Override public Double getValue() { - if (this.file.contains(".properties")) { + if (this.file.endsWith(".properties")) { try { return Double.parseDouble(propFile.getProperty(key)); } catch (NumberFormatException e) { Logger l = new Logger(this.plugin.foundryConfig); l.severe("Unable to parse key '"+this.key+"' in file '"+this.file+"' NumberFormatException"); l.severe(e.getMessage()); return 0.0; } - } else if (this.file.contains(".yml") || file.contains(".yaml")) { + } else if (this.file.endsWith(".yml") || this.file.endsWith(".yaml")) { this.loadFile(); Double value = yamlFile.getDouble(key); yamlFile.close(); return value; } else { new Logger(this.plugin.foundryConfig).severe("Unable to load file: '"+this.file+"' extension not supported."); return 0.0; } }src/main/java/net/lewmc/kryptonite/kos/gui/KOS_ConfigItemGui.java (2)
47-71: Guard against non-player senders to prevent ClassCastException.Lines 66 and 70 cast
this.usertoPlayerwithout checking if it's actually a Player instance. This will crash if the command is run from console or a command block.Apply this diff:
public void show() { + if (!(this.user instanceof Player player)) { + this.user.sendMessage(ChatColor.RED + "This command can only be used by a player."); + return; + } + if (type == Kryptonite.ConfigurationOptions.MINECRAFT) { this.config = new MinecraftConfig(plugin); } else if (type == Kryptonite.ConfigurationOptions.BUKKIT) { this.config = new BukkitConfig(plugin); } else if (type == Kryptonite.ConfigurationOptions.SPIGOT) { this.config = new SpigotConfig(plugin); } else if (type == Kryptonite.ConfigurationOptions.PAPER_WORLD) { this.config = new PaperWorldConfig(plugin); } else if (type == Kryptonite.ConfigurationOptions.PURPUR) { this.config = new PurpurConfig(plugin); } else if (type == Kryptonite.ConfigurationOptions.PUFFERFISH) { this.config = new PufferfishConfig(plugin); } else if (type == Kryptonite.ConfigurationOptions.LEAF) { this.config = new LeafConfig(plugin); } else { this.config = null; } - this.gui = new InventoryGui(this.plugin, (Player) this.user, "KOS - Server Configuration", this.getElements()); + this.gui = new InventoryGui(this.plugin, player, "KOS - Server Configuration", this.getElements()); this.addElements(); this.gui.build(); - this.gui.show((Player) this.user); + this.gui.show(player); }
209-238: Fix invalid GUI layout with empty-string rows.Line 236 returns an array containing empty strings
""for rows, which can break InventoryGui's layout parsing. Each row should be 9 characters (spaces for empty slots).Apply this diff:
} else { this.plugin.getLogger().severe("Unable to return elements: config is null."); - return new String[] {"",""," t u v "}; + return new String[] {" ", " ", " t u v "}; } }
🧹 Nitpick comments (1)
src/main/java/net/lewmc/kryptonite/kos/gui/KOS_ConfigItemGui.java (1)
151-201: Consider extracting type-specific handlers to reduce complexity.The method has high cognitive complexity (40 vs. 15 allowed) due to nested type checks and click handling. Additionally, StringConfigItem handling is marked TODO.
Consider extracting separate handler methods:
private void handleClick(GenericConfigItem<?> config, GuiElement.Click click) { click.getGui().close(); if (config instanceof BooleanConfigItem boolItem) { handleBooleanClick(boolItem); } else if (config instanceof IntegerConfigItem intItem) { handleIntegerClick(intItem, click.getType()); } else if (config instanceof DoubleConfigItem doubleItem) { handleDoubleClick(doubleItem, click.getType()); } else if (config instanceof StringConfigItem stringItem) { handleStringClick(stringItem, click.getType()); } this.show(); } private void handleBooleanClick(BooleanConfigItem item) { if (item.willBeValid(!item.getValue())) { item.setValue(!item.getValue()); } } private void handleIntegerClick(IntegerConfigItem item, ClickType clickType) { int delta = (clickType == ClickType.SHIFT_LEFT || clickType == ClickType.SHIFT_RIGHT) ? 10 : 1; if (clickType == ClickType.LEFT || clickType == ClickType.SHIFT_LEFT) { delta = -delta; } int newValue = item.getValue() + delta; if (item.willBeValid(newValue)) { item.setValue(newValue); } } // Similar for handleDoubleClick and handleStringClickDo you want me to generate the complete implementation for StringConfigItem handling as well?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/main/java/net/lewmc/kryptonite/config/PaperWorldConfig.java(1 hunks)src/main/java/net/lewmc/kryptonite/kos/AutoKOS.java(8 hunks)src/main/java/net/lewmc/kryptonite/kos/gui/KOS_ConfigItemGui.java(1 hunks)src/main/java/net/lewmc/kryptonite/kos/gui/KOS_PaperWorld_1.java(0 hunks)src/main/java/net/lewmc/kryptonite/utils/config/DoubleConfigItem.java(1 hunks)src/main/java/net/lewmc/kryptonite/utils/config/IntegerConfigItem.java(1 hunks)
💤 Files with no reviewable changes (1)
- src/main/java/net/lewmc/kryptonite/kos/gui/KOS_PaperWorld_1.java
🧰 Additional context used
🧬 Code graph analysis (5)
src/main/java/net/lewmc/kryptonite/config/PaperWorldConfig.java (5)
src/main/java/net/lewmc/kryptonite/Kryptonite.java (1)
Kryptonite(21-196)src/main/java/net/lewmc/kryptonite/utils/config/BooleanConfigItem.java (1)
BooleanConfigItem(12-95)src/main/java/net/lewmc/kryptonite/utils/config/ConfigCollection.java (1)
ConfigCollection(9-14)src/main/java/net/lewmc/kryptonite/utils/config/IntegerConfigItem.java (1)
IntegerConfigItem(12-141)src/main/java/net/lewmc/kryptonite/utils/config/StringConfigItem.java (1)
StringConfigItem(12-103)
src/main/java/net/lewmc/kryptonite/utils/config/DoubleConfigItem.java (1)
src/main/java/net/lewmc/kryptonite/Kryptonite.java (1)
Kryptonite(21-196)
src/main/java/net/lewmc/kryptonite/utils/config/IntegerConfigItem.java (1)
src/main/java/net/lewmc/kryptonite/Kryptonite.java (1)
Kryptonite(21-196)
src/main/java/net/lewmc/kryptonite/kos/AutoKOS.java (12)
src/main/java/net/lewmc/kryptonite/Kryptonite.java (1)
Kryptonite(21-196)src/main/java/net/lewmc/kryptonite/utils/MessageUtil.java (1)
MessageUtil(6-28)src/main/java/net/lewmc/kryptonite/utils/SoftwareUtil.java (1)
SoftwareUtil(7-45)src/main/java/net/lewmc/kryptonite/utils/config/BooleanConfigItem.java (1)
BooleanConfigItem(12-95)src/main/java/net/lewmc/kryptonite/utils/config/IntegerConfigItem.java (1)
IntegerConfigItem(12-141)src/main/java/net/lewmc/kryptonite/utils/config/StringConfigItem.java (1)
StringConfigItem(12-103)src/main/java/net/lewmc/kryptonite/config/MinecraftConfig.java (1)
MinecraftConfig(14-116)src/main/java/net/lewmc/kryptonite/config/BukkitConfig.java (1)
BukkitConfig(13-285)src/main/java/net/lewmc/kryptonite/config/SpigotConfig.java (1)
SpigotConfig(15-371)src/main/java/net/lewmc/kryptonite/config/PaperWorldConfig.java (1)
PaperWorldConfig(15-572)src/main/java/net/lewmc/kryptonite/config/PurpurConfig.java (1)
PurpurConfig(14-187)src/main/java/net/lewmc/kryptonite/config/PufferfishConfig.java (1)
PufferfishConfig(14-161)
src/main/java/net/lewmc/kryptonite/kos/gui/KOS_ConfigItemGui.java (1)
src/main/java/net/lewmc/kryptonite/Kryptonite.java (1)
Kryptonite(21-196)
🪛 GitHub Check: SonarCloud Code Analysis
src/main/java/net/lewmc/kryptonite/config/PaperWorldConfig.java
[failure] 121-121: Define a constant instead of duplicating this literal "the limit." 4 times.
[failure] 80-80: Define a constant instead of duplicating this literal "loaded per chunk. A value of -1 disables the limit." 3 times.
src/main/java/net/lewmc/kryptonite/utils/config/IntegerConfigItem.java
[warning] 40-40: Constructor has 9 parameters, which is greater than 7 authorized.
src/main/java/net/lewmc/kryptonite/kos/gui/KOS_ConfigItemGui.java
[warning] 135-135: Use a primitive boolean expression here.
[warning] 197-197: Complete the task associated to this TODO comment.
[warning] 173-173: Merge this if statement with the enclosing one.
[failure] 219-219: Move the update of "index" into this loop's update clause.
[failure] 151-151: Refactor this method to reduce its Cognitive Complexity from 40 to the 15 allowed.
[warning] 79-79: Provide the parametrized type for this generic.
[warning] 192-192: Merge this if statement with the enclosing one.
🔇 Additional comments (2)
src/main/java/net/lewmc/kryptonite/kos/AutoKOS.java (1)
85-99: LGTM: Configuration framework migration looks correct.The migration from direct API calls to the new config-item framework is well-executed. Each configuration type is instantiated and items are accessed via type-cast pattern-matching.
src/main/java/net/lewmc/kryptonite/config/PaperWorldConfig.java (1)
20-295: LGTM: Declarative configuration definition is clear and consistent.The constructor follows a consistent pattern to define all Paper world configuration items. The static analysis hints about duplicate string literals in descriptions are acceptable for user-facing text.
| public Integer getValue() { | ||
| if (this.file.contains(".properties")) { | ||
| try { | ||
| return Integer.parseInt(propFile.getProperty(key)); | ||
| } | ||
| catch (NumberFormatException e) { | ||
| Logger l = new Logger(this.plugin.foundryConfig); | ||
| l.severe("Unable to parse key '"+this.key+"' in file '"+this.file+"' NumberFormatException"); | ||
| l.severe(e.getMessage()); | ||
| return 0; | ||
| } | ||
| } else if (this.file.contains(".yml") || file.contains(".yaml")) { | ||
| this.loadFile(); | ||
| Integer value = yamlFile.getInt(key); | ||
| yamlFile.close(); | ||
| return value; | ||
| } else { | ||
| new Logger(this.plugin.foundryConfig).severe("Unable to load file: '"+this.file+"' extension not supported."); | ||
| return 0; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use .endsWith() instead of .contains() for file extension checks.
Lines 53 and 63 use .contains() to check file extensions, which can match unwanted substrings.
Apply this diff:
@Override
public Integer getValue() {
- if (this.file.contains(".properties")) {
+ if (this.file.endsWith(".properties")) {
try {
return Integer.parseInt(propFile.getProperty(key));
}
catch (NumberFormatException e) {
Logger l = new Logger(this.plugin.foundryConfig);
l.severe("Unable to parse key '"+this.key+"' in file '"+this.file+"' NumberFormatException");
l.severe(e.getMessage());
return 0;
}
- } else if (this.file.contains(".yml") || file.contains(".yaml")) {
+ } else if (this.file.endsWith(".yml") || this.file.endsWith(".yaml")) {
this.loadFile();
Integer value = yamlFile.getInt(key);
yamlFile.close();
return value;
} else {
new Logger(this.plugin.foundryConfig).severe("Unable to load file: '"+this.file+"' extension not supported.");
return 0;
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public Integer getValue() { | |
| if (this.file.contains(".properties")) { | |
| try { | |
| return Integer.parseInt(propFile.getProperty(key)); | |
| } | |
| catch (NumberFormatException e) { | |
| Logger l = new Logger(this.plugin.foundryConfig); | |
| l.severe("Unable to parse key '"+this.key+"' in file '"+this.file+"' NumberFormatException"); | |
| l.severe(e.getMessage()); | |
| return 0; | |
| } | |
| } else if (this.file.contains(".yml") || file.contains(".yaml")) { | |
| this.loadFile(); | |
| Integer value = yamlFile.getInt(key); | |
| yamlFile.close(); | |
| return value; | |
| } else { | |
| new Logger(this.plugin.foundryConfig).severe("Unable to load file: '"+this.file+"' extension not supported."); | |
| return 0; | |
| } | |
| } | |
| public Integer getValue() { | |
| if (this.file.endsWith(".properties")) { | |
| try { | |
| return Integer.parseInt(propFile.getProperty(key)); | |
| } | |
| catch (NumberFormatException e) { | |
| Logger l = new Logger(this.plugin.foundryConfig); | |
| l.severe("Unable to parse key '"+this.key+"' in file '"+this.file+"' NumberFormatException"); | |
| l.severe(e.getMessage()); | |
| return 0; | |
| } | |
| } else if (this.file.endsWith(".yml") || this.file.endsWith(".yaml")) { | |
| this.loadFile(); | |
| Integer value = yamlFile.getInt(key); | |
| yamlFile.close(); | |
| return value; | |
| } else { | |
| new Logger(this.plugin.foundryConfig).severe("Unable to load file: '"+this.file+"' extension not supported."); | |
| return 0; | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/main/java/net/lewmc/kryptonite/utils/config/IntegerConfigItem.java around
lines 52 to 72, the code uses String.contains() to check file extensions which
can match unintended substrings; change those checks to use
.endsWith(".properties"), .endsWith(".yml") and .endsWith(".yaml") respectively
(update both the properties check and the YAML check), leaving the surrounding
logic the same so only the extension checks are more precise.
| public void setValue(Integer value) { | ||
| if (this.file.contains(".properties")) { | ||
| propFile.setProperty(this.key, String.valueOf(value)); | ||
| } else if (file.contains(".yml") || file.contains(".yaml")) { | ||
| this.loadFile(); | ||
| yamlFile.set(this.key, value); | ||
| yamlFile.save(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Close YAML handle after save and use .endsWith() for extension checks.
The YAML file handle is not closed after yamlFile.save(), which can leak file descriptors. Additionally, extension checks should use .endsWith().
Apply this diff:
@Override
public void setValue(Integer value) {
- if (this.file.contains(".properties")) {
+ if (this.file.endsWith(".properties")) {
propFile.setProperty(this.key, String.valueOf(value));
- } else if (file.contains(".yml") || file.contains(".yaml")) {
+ } else if (this.file.endsWith(".yml") || this.file.endsWith(".yaml")) {
this.loadFile();
yamlFile.set(this.key, value);
yamlFile.save();
+ yamlFile.close();
}
}🤖 Prompt for AI Agents
In src/main/java/net/lewmc/kryptonite/utils/config/IntegerConfigItem.java around
lines 79 to 87, update the file extension checks to use
.endsWith(".properties"), .endsWith(".yml") and .endsWith(".yaml") instead of
.contains(...), and after calling yamlFile.save() ensure the YAML handle is
closed (e.g., call the appropriate yamlFile.close() or close method provided by
the YAML wrapper) to avoid leaking file descriptors; keep existing behavior for
properties path but switch to endsWith and close the yaml resource after save.
=
Summary by CodeRabbit
New Features
Improvements
Removals / Deprecations
✏️ Tip: You can customize this high-level summary in your review settings.