Conversation
…LogUtil throughout the project replacing slf4j
…cket and ExprPacketField) fix: description for ExprPacketField and SecExprNewPacket to show the available fields
add: buildServer task
refactor: rename PacketDefinitionRegistry, MainPacketDefinitions and SkBeePacketDefinitions
refactor: make use of lambda
…ny syntax collisions and looks a lot better in code)
refactor: Definition based classes for a more abstract and expandable approach This did a lot and I cba to write it all out, look at the changes if you're interested nerd
Clarified the relationship between PacketEventsSK and SkBee in the README.
Added note about SkBee requirement for element registration.
Add clarification about entity pose requirement.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds a schema-driven field/constructor registry system; migrates packet/meta/fake-entity construction and expressions to it; refactors ConversionUtil, PlayerSkinManager, and logging; updates build/config/docs/lang; relocates packet API packages; and adds a Modrinth-based UpdateChecker with join notifications. ChangesCore schema and registries
API, packages, removals
Skins and simple elements
Logging, loader, build, config, updates
Sequence Diagram(s) sequenceDiagram
participant Sec as SkriptSection
participant Reg as PacketFieldRegistry
participant Schema as FieldSchema
participant Ctx as ConstructionContext
participant Packet as PacketWrapper
Sec->>Reg: getSchema(packetType)
Reg-->>Schema: returns FieldSchema(accessors, constructor)
Sec->>Ctx: assemble values map
Schema->>Packet: constructor.apply(Ctx)
Packet-->>Sec: constructed packet instance
sequenceDiagram
participant Sec as SkriptSection
participant Reg as MetaFieldRegistry
participant Schema as FieldSchema
participant Ctx as ConstructionContext
participant Meta as EntityMeta
Sec->>Reg: getSchema(entityType)
Reg-->>Schema: return FieldSchema(accessors, constructor)
Sec->>Ctx: assemble values map
Schema->>Meta: constructor.apply(Ctx)
Meta-->>Sec: constructed meta
Estimated code review effort: 🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs:
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
Qodana for JVM8 new problems were found
View the detailed Qodana reportTo be able to view the detailed Qodana report, you can either:
To get - name: 'Qodana Scan'
uses: JetBrains/qodana-action@v2025.3.2
with:
upload-result: trueContact Qodana teamContact us at qodana-support@jetbrains.com
|
There was a problem hiding this comment.
Pull request overview
This PR bumps PacketEventsSK to v1.1.2, adds a Modrinth-based update checker, and performs a sizable refactor of “packet/meta/fake entity field” handling by introducing a schema/accessor registry system that replaces many specialized expressions/sections with generic “field” access.
Changes:
- Add an update-checker (config + Modrinth API integration + join notifications).
- Introduce a generic field registry framework (
FieldSchema/FieldAccessor/ConstructionContext) and use it for packets, entity meta, and fake entities. - Restructure several packages and remove legacy registries/expressions in favor of the new field system; update build + docs accordingly.
Reviewed changes
Copilot reviewed 108 out of 109 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main/resources/lang/english.lang | Adds/updates language keys for newly introduced types. |
| src/main/resources/config.yml | Bumps version header and adds update-checker configuration section. |
| src/main/java/dev/threeadd/packeteventssk/update/UpdateChecker.java | New Modrinth update check logic (sync/async) and console logging. |
| src/main/java/dev/threeadd/packeteventssk/update/ModrinthVersion.java | New Modrinth version JSON wrapper and supported-version checks. |
| src/main/java/dev/threeadd/packeteventssk/update/JoinUpdateListener.java | Notifies privileged players on join when an update is available. |
| src/main/java/dev/threeadd/packeteventssk/PacketEventsSK.java | Switches logging to LogUtil, wires metrics + update checker, removes UserManager registration. |
| src/main/java/dev/threeadd/packeteventssk/metrics/MetricsLoader.java | Moves MetricsLoader into a dedicated metrics package. |
| src/main/java/dev/threeadd/packeteventssk/element/simple/SimpleElementRegistration.java | Updates imports to new effect package naming. |
| src/main/java/dev/threeadd/packeteventssk/element/simple/effect/EffGlow.java | Package rename to ...simple.effect. |
| src/main/java/dev/threeadd/packeteventssk/element/simple/effect/EffDisplayedSkin.java | Package rename + adjusts PlayerSkinManager API usage and simplifies viewer handling. |
| src/main/java/dev/threeadd/packeteventssk/element/general/Types.java | Updates/extends type registrations (interaction hand) and refactors some patterns/strings. |
| src/main/java/dev/threeadd/packeteventssk/element/general/section/SecExprNewPacket.java | Reworks new-packet section to use packet field schemas/accessors + auto-doc generation. |
| src/main/java/dev/threeadd/packeteventssk/element/general/GeneralElementRegistration.java | Registers new packet/meta field registries and new expressions/sections. |
| src/main/java/dev/threeadd/packeteventssk/element/general/field/packet/SkBeePacketFieldRegistrar.java | Adds SkBee-dependent packet field registration (NBT). |
| src/main/java/dev/threeadd/packeteventssk/element/general/field/packet/ServerBoundPacketFieldRegistrar.java | Adds serverbound packet schemas/accessors and constructors. |
| src/main/java/dev/threeadd/packeteventssk/element/general/field/packet/PacketFieldRegistry.java | Introduces packet field registry singleton and registrar list. |
| src/main/java/dev/threeadd/packeteventssk/element/general/field/packet/ClientBoundPacketFieldRegistrar.java | Adds clientbound packet schemas/accessors and constructors. |
| src/main/java/dev/threeadd/packeteventssk/element/general/field/meta/MetaFieldRegistry.java | Introduces inheriting meta field registry keyed by entity type. |
| src/main/java/dev/threeadd/packeteventssk/element/general/field/meta/LivingMetaFieldRegistrar.java | Adds (currently incomplete) living-entity meta field schema. |
| src/main/java/dev/threeadd/packeteventssk/element/general/field/meta/ItemMetaFieldRegistrar.java | Adds item-entity meta field schema and constructor. |
| src/main/java/dev/threeadd/packeteventssk/element/general/field/meta/InteractionMetaFieldRegistrar.java | Adds interaction-entity meta field schema and constructor. |
| src/main/java/dev/threeadd/packeteventssk/element/general/field/meta/BaseMetaFieldRegistrar.java | Adds base entity meta field schema + conditional pose field. |
| src/main/java/dev/threeadd/packeteventssk/element/general/expression/prop/ExprPlayerSkin.java | Package rename to new expression namespace. |
| src/main/java/dev/threeadd/packeteventssk/element/general/expression/prop/ExprPacketPacketType.java | Package rename to new expression namespace. |
| src/main/java/dev/threeadd/packeteventssk/element/general/expression/prop/ExprPacketField.java | Migrates packet-field expression to schema/accessor system + improves docs and change typing. |
| src/main/java/dev/threeadd/packeteventssk/element/general/expression/prop/ExprMetaField.java | New generic meta-field expression built on meta field registry. |
| src/main/java/dev/threeadd/packeteventssk/element/general/expression/prop/ExprEntityId.java | Package rename to new expression namespace. |
| src/main/java/dev/threeadd/packeteventssk/element/general/expression/ExprSkinFromValue.java | Package rename to new expression namespace. |
| src/main/java/dev/threeadd/packeteventssk/element/general/event/EvtPacketSendOrReceive.java | Updates imports + changes syntax to require literal packet type (%*packettype%). |
| src/main/java/dev/threeadd/packeteventssk/element/general/effect/EffFetchSkin.java | Updates syntax patterns (adds optional “fetch” and “named/with name”). |
| src/main/java/dev/threeadd/packeteventssk/element/general/effect/EffCancelPacket.java | Updates PacketSendOrReceiveEvent import path. |
| src/main/java/dev/threeadd/packeteventssk/element/entity/sections/EffSecCreateFakeEntity.java | Removes legacy fake-entity creation effect section. |
| src/main/java/dev/threeadd/packeteventssk/element/entity/section/SecExprNewFakeEntity.java | New fake-entity constructor section built on fake-entity field schemas/accessors. |
| src/main/java/dev/threeadd/packeteventssk/element/entity/field/FakePlayerEntityFieldRegistrar.java | Adds player fake-entity schema (skin/name + constructor). |
| src/main/java/dev/threeadd/packeteventssk/element/entity/field/FakeEntityFieldRegistry.java | Introduces inheriting fake-entity field registry. |
| src/main/java/dev/threeadd/packeteventssk/element/entity/field/FakeBaseEntityFieldRegistrar.java | Adds base fake-entity fields (viewers/location/meta/id/uuid) + constructor. |
| src/main/java/dev/threeadd/packeteventssk/element/entity/expressions/prop/player/ExprFakePlayerEntitySkin.java | Removes legacy fake-player skin property expression. |
| src/main/java/dev/threeadd/packeteventssk/element/entity/expressions/prop/meta/living/ExprFakeLivingEntityMetaHealth.java | Removes legacy fake living-meta health expression. |
| src/main/java/dev/threeadd/packeteventssk/element/entity/expressions/prop/meta/item/ExprFakeItemMetaItem.java | Removes legacy dropped-item meta item expression. |
| src/main/java/dev/threeadd/packeteventssk/element/entity/expressions/prop/meta/interaction/ExprFakeInterationMetaHeightOrWidth.java | Removes legacy interaction meta width/height expression. |
| src/main/java/dev/threeadd/packeteventssk/element/entity/expressions/prop/meta/display/text/ExprFakeTextDisplayMetaTextShadowed.java | Removes legacy text-display shadowed expression. |
| src/main/java/dev/threeadd/packeteventssk/element/entity/expressions/prop/meta/display/text/ExprFakeTextDisplayMetaText.java | Removes legacy text-display text expression. |
| src/main/java/dev/threeadd/packeteventssk/element/entity/expressions/prop/meta/display/text/ExprFakeTextDisplayMetaBackGroundColor.java | Removes legacy text-display background color expression. |
| src/main/java/dev/threeadd/packeteventssk/element/entity/expressions/prop/meta/display/item/ExprFakeItemDisplayMetaItem.java | Removes legacy item-display item expression. |
| src/main/java/dev/threeadd/packeteventssk/element/entity/expressions/prop/meta/display/ExprFakeDisplayMetaViewRange.java | Removes legacy display view-range expression. |
| src/main/java/dev/threeadd/packeteventssk/element/entity/expressions/prop/meta/display/ExprFakeDisplayMetaTranslation.java | Removes legacy display translation expression. |
| src/main/java/dev/threeadd/packeteventssk/element/entity/expressions/prop/meta/display/ExprFakeDisplayMetaTransformationInterpolationDuration.java | Removes legacy display transform interpolation duration expression. |
| src/main/java/dev/threeadd/packeteventssk/element/entity/expressions/prop/meta/display/ExprFakeDisplayMetaTeleportInterpolationDuration.java | Removes legacy display teleport interpolation duration expression. |
| src/main/java/dev/threeadd/packeteventssk/element/entity/expressions/prop/meta/display/ExprFakeDisplayMetaScale.java | Removes legacy display scale expression. |
| src/main/java/dev/threeadd/packeteventssk/element/entity/expressions/prop/meta/display/ExprFakeDisplayMetaLeftOrRightRotation.java | Removes legacy display rotation expression. |
| src/main/java/dev/threeadd/packeteventssk/element/entity/expressions/prop/meta/display/ExprFakeDisplayMetaInterpolationDelay.java | Removes legacy display interpolation delay expression. |
| src/main/java/dev/threeadd/packeteventssk/element/entity/expressions/prop/meta/display/ExprFakeDisplayMetaHeightOrWidth.java | Removes legacy display height/width expression. |
| src/main/java/dev/threeadd/packeteventssk/element/entity/expressions/prop/meta/display/ExprFakeDisplayMetaBillboard.java | Removes legacy display billboard expression. |
| src/main/java/dev/threeadd/packeteventssk/element/entity/expressions/prop/meta/display/block/ExprFakeBlockDisplayMetaBlock.java | Removes legacy block-display block-data expression. |
| src/main/java/dev/threeadd/packeteventssk/element/entity/expressions/prop/meta/base/ExprFakeMetaSwimming.java | Removes legacy base-meta swimming expression. |
| src/main/java/dev/threeadd/packeteventssk/element/entity/expressions/prop/meta/base/ExprFakeMetaSprinting.java | Removes legacy base-meta sprinting expression. |
| src/main/java/dev/threeadd/packeteventssk/element/entity/expressions/prop/meta/base/ExprFakeMetaSneaking.java | Removes legacy base-meta sneaking expression. |
| src/main/java/dev/threeadd/packeteventssk/element/entity/expressions/prop/meta/base/ExprFakeMetaSilent.java | Removes legacy base-meta silent expression. |
| src/main/java/dev/threeadd/packeteventssk/element/entity/expressions/prop/meta/base/ExprFakeMetaPose.java | Removes legacy base-meta pose expression. |
| src/main/java/dev/threeadd/packeteventssk/element/entity/expressions/prop/meta/base/ExprFakeMetaInvisible.java | Removes legacy base-meta invisible expression. |
| src/main/java/dev/threeadd/packeteventssk/element/entity/expressions/prop/meta/base/ExprFakeMetaGravity.java | Removes legacy base-meta gravity expression. |
| src/main/java/dev/threeadd/packeteventssk/element/entity/expressions/prop/meta/base/ExprFakeMetaGlowing.java | Removes legacy base-meta glowing expression. |
| src/main/java/dev/threeadd/packeteventssk/element/entity/expressions/prop/meta/base/ExprFakeMetaFrozenTime.java | Removes legacy base-meta frozen-time expression. |
| src/main/java/dev/threeadd/packeteventssk/element/entity/expressions/prop/meta/base/ExprFakeMetaFire.java | Removes legacy base-meta fire expression. |
| src/main/java/dev/threeadd/packeteventssk/element/entity/expressions/prop/meta/base/ExprFakeMetaElytra.java | Removes legacy base-meta elytra expression. |
| src/main/java/dev/threeadd/packeteventssk/element/entity/expressions/prop/meta/base/ExprFakeMetaCustomNameVisible.java | Removes legacy base-meta custom-name-visible expression. |
| src/main/java/dev/threeadd/packeteventssk/element/entity/expressions/prop/meta/base/ExprFakeMetaCustomName.java | Removes legacy base-meta custom-name expression. |
| src/main/java/dev/threeadd/packeteventssk/element/entity/expressions/prop/meta/base/ExprFakeMetaAirTime.java | Removes legacy base-meta air-time expression. |
| src/main/java/dev/threeadd/packeteventssk/element/entity/expressions/prop/ExprFakeEntityViewers.java | Removes legacy fake-entity viewers property expression. |
| src/main/java/dev/threeadd/packeteventssk/element/entity/expressions/prop/ExprFakeEntityUuid.java | Removes legacy fake-entity uuid property expression. |
| src/main/java/dev/threeadd/packeteventssk/element/entity/expressions/prop/ExprFakeEntityType.java | Removes legacy fake-entity type property expression. |
| src/main/java/dev/threeadd/packeteventssk/element/entity/expressions/prop/ExprFakeEntityMeta.java | Removes legacy fake-entity meta property expression. |
| src/main/java/dev/threeadd/packeteventssk/element/entity/expressions/prop/ExprFakeEntityLocation.java | Removes legacy fake-entity location property expression. |
| src/main/java/dev/threeadd/packeteventssk/element/entity/expressions/prop/ExprFakeEntityId.java | Removes legacy fake-entity id property expression. |
| src/main/java/dev/threeadd/packeteventssk/element/entity/expression/prop/living/ExprFakeLivingEntityAttribute.java | Package rename to new expression namespace. |
| src/main/java/dev/threeadd/packeteventssk/element/entity/expression/prop/ExprVisibleFakeEntities.java | Package rename to new expression namespace. |
| src/main/java/dev/threeadd/packeteventssk/element/entity/expression/prop/ExprFakeEntityField.java | New generic fake-entity field expression built on fake-entity field registry. |
| src/main/java/dev/threeadd/packeteventssk/element/entity/expression/ExprFakeEntityFromUuid.java | Package rename to new expression namespace. |
| src/main/java/dev/threeadd/packeteventssk/element/entity/expression/ExprFakeEntityFromId.java | Package rename to new expression namespace. |
| src/main/java/dev/threeadd/packeteventssk/element/entity/expression/ExprFakeEntityEventValue.java | Package rename to new expression namespace. |
| src/main/java/dev/threeadd/packeteventssk/element/entity/expression/ExprFakeEntitiesAll.java | Package rename to new expression namespace. |
| src/main/java/dev/threeadd/packeteventssk/element/entity/EntityElementRegistration.java | Registers fake-entity field registry; replaces removed legacy sections/expressions with new ones. |
| src/main/java/dev/threeadd/packeteventssk/element/entity/effect/EffTeleportFakeEntity.java | Adjusts syntax pattern spacing for fake-entity wording. |
| src/main/java/dev/threeadd/packeteventssk/element/entity/effect/EffSpawnFakeEntity.java | Adjusts syntax pattern spacing for fake-entity wording. |
| src/main/java/dev/threeadd/packeteventssk/element/entity/effect/EffRideFakeEntity.java | Adjusts syntax patterns spacing for fake-entity wording. |
| src/main/java/dev/threeadd/packeteventssk/element/entity/effect/EffKillFakeEntity.java | Adjusts syntax pattern spacing for fake-entity wording. |
| src/main/java/dev/threeadd/packeteventssk/config/Configurable.java | Adds config keys for update checker enabled/async. |
| src/main/java/dev/threeadd/packeteventssk/config/Config.java | Removes slf4j logger usage and changes config-update failure behavior to throw. |
| src/main/java/dev/threeadd/packeteventssk/api/util/LogUtil.java | New centralized console/player messaging utility (MiniMessage + prefix). |
| src/main/java/dev/threeadd/packeteventssk/api/util/field/InheritingFieldRegistry.java | New inheriting registry implementation for type hierarchies. |
| src/main/java/dev/threeadd/packeteventssk/api/util/field/FieldSchema.java | New field schema record with field lookup and readable-doc rendering. |
| src/main/java/dev/threeadd/packeteventssk/api/util/field/FieldRegistrar.java | New registrar interface for registry population. |
| src/main/java/dev/threeadd/packeteventssk/api/util/field/FieldAccessor.java | New accessor record with aliases + getter/setter. |
| src/main/java/dev/threeadd/packeteventssk/api/util/field/ConstructionContext.java | New context type for constructing objects from schema arguments. |
| src/main/java/dev/threeadd/packeteventssk/api/util/field/BaseFieldRegistry.java | New base registry + builder and registrar workflow. |
| src/main/java/dev/threeadd/packeteventssk/api/util/ConversionUtil.java | Expands conversion helpers (timespan, billboard/transform enums, locations) with null-handling. |
| src/main/java/dev/threeadd/packeteventssk/api/general/UserManager.java | Removed legacy PacketEvents user caching listener. |
| src/main/java/dev/threeadd/packeteventssk/api/general/SkBeePacketRegistrations.java | Removed legacy SkBee packet-registration hook. |
| src/main/java/dev/threeadd/packeteventssk/api/general/PacketConstructorRegistry.java | Removed legacy packet constructor/field registry. |
| src/main/java/dev/threeadd/packeteventssk/api/general/packet/PacketTypeRegistry.java | Moves PacketTypeRegistry into api.general.packet and cleans imports. |
| src/main/java/dev/threeadd/packeteventssk/api/general/packet/PacketSendOrReceiveListener.java | Moves listener into api.general.packet. |
| src/main/java/dev/threeadd/packeteventssk/api/general/packet/PacketSendOrReceiveEvent.java | Moves event into api.general.packet. |
| src/main/java/dev/threeadd/packeteventssk/api/entity/EntityMetaPropertyExpression.java | Removes legacy meta-property expression base class. |
| src/main/java/dev/threeadd/packeteventssk/AddonLoader.java | Switches to LogUtil, changes Registration settings, and adds element-count logging. |
| README.md | Updates SkBee section and clarifies optional addon-provided types. |
| build.gradle.kts | Bumps version/deps, tweaks repositories, and adds a local “buildServer” copy task. |
Comments suppressed due to low confidence (2)
src/main/java/dev/threeadd/packeteventssk/update/UpdateChecker.java:88
- If
getLatestVersionFromModrinth()returnsnull,latestReleaseFutureis cancelled andupdateVersionFutureis never completed/cancelled (becausethenApplywon’t run for a cancelled future). That can leave callers waiting forever. Ensure the returned future always completes (e.g.,completeExceptionally, or complete with a sentinel/Optional and handle it explicitly).
src/main/java/dev/threeadd/packeteventssk/update/UpdateChecker.java:102 getLatestVersionFromModrinth()opens a stream/reader without closing it, and usesURL#openStream()without timeouts. This can leak resources and hang startup on slow networks. Use try-with-resources and anHttpURLConnection(or HttpClient) with connect/read timeouts, and handle an empty JSON array safely beforeelements.get(0).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (18)
src/main/java/dev/threeadd/packeteventssk/api/util/ConversionUtil.java-52-55 (1)
52-55:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftUsing
getFirst()world is incorrect for multi-world servers.
com.github.retrooper.packetevents.protocol.world.Locationdoesn't carry world information, so defaulting to the first world will produce wrong results on any multi-world server. If world context is unavailable from the PE Location, callers need to supply it explicitly or this method should require aWorldparameter.Suggested signature change
-public static Location toBukkitLocation(com.github.retrooper.packeteventssk.protocol.world.Location location) { +public static Location toBukkitLocation(com.github.retrooper.packetevents.protocol.world.Location location, World world) { if (location == null) return null; - return new Location(Bukkit.getWorlds().getFirst(), location.getX(), location.getY(), location.getZ(), location.getYaw(), location.getPitch()); + return new Location(world, location.getX(), location.getY(), location.getZ(), location.getYaw(), location.getPitch()); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/dev/threeadd/packeteventssk/api/util/ConversionUtil.java` around lines 52 - 55, The toBukkitLocation method in ConversionUtil uses Bukkit.getWorlds().getFirst() which breaks multi-world servers because com.github.retrooper.packetevents.protocol.world.Location has no world info; change the API to require the caller to supply the target World (e.g., replace or overload public static Location toBukkitLocation(...location) with public static Location toBukkitLocation(World world, com.github.retrooper.packetevents.protocol.world.Location location)), remove the getFirst() call, construct the Bukkit Location using the provided World, and update all call sites to pass the correct World (or add an overload that delegates to the new method and mark the no-world signature deprecated).src/main/java/dev/threeadd/packeteventssk/api/simple/PlayerSkinManager.java-89-89 (1)
89-89:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDebug logging uses ERROR level instead of DEBUG/TRACE.
This line logs routine operational data (target and viewer names) at ERROR level. This will pollute error logs during normal operation and may trigger monitoring alerts for non-error conditions. Use
log.debug()orlog.trace()for diagnostic output.Proposed fix
- log.error("target: {}, viewers: {}", target.getName(), viewers.stream().map(Player::getName).toList()); + log.debug("target: {}, viewers: {}", target.getName(), viewers.stream().map(Player::getName).toList());🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/dev/threeadd/packeteventssk/api/simple/PlayerSkinManager.java` at line 89, In PlayerSkinManager replace the routine diagnostic log call that currently uses log.error(...) (the line logging "target: {}, viewers: {}" with target.getName() and viewers.stream().map(Player::getName).toList()) with a debug or trace level call (e.g., log.debug(...)/log.trace(...)) and wrap it with a log.isDebugEnabled()/isTraceEnabled() guard or use a parameterized/debug lambda-style logger to avoid the viewer stream allocation when debug is disabled.src/main/java/dev/threeadd/packeteventssk/api/simple/PlayerSkinManager.java-158-165 (1)
158-165:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftPotential race condition: packet sent before async teleport completes.
teleportAsync()returns aCompletableFuture, but theSTART_LOADING_CHUNKSpacket is sent immediately after without awaiting completion. This may cause the packet to arrive before the player is teleported, leading to inconsistent client state.Proposed fix using thenRun
- player.teleportAsync(player.getLocation()); // replaces WrapperPlayServerPlayerPositionAndLook cause it's advertised as buggy if done using the packet - - WrapperPlayServerChangeGameState startLoadingChunksPacket = new WrapperPlayServerChangeGameState( - Reason.START_LOADING_CHUNKS, - 0.0f); - PacketEvents.getAPI().getPlayerManager().sendPacket(player, startLoadingChunksPacket); + player.teleportAsync(player.getLocation()).thenRun(() -> { + WrapperPlayServerChangeGameState startLoadingChunksPacket = new WrapperPlayServerChangeGameState( + Reason.START_LOADING_CHUNKS, + 0.0f); + PacketEvents.getAPI().getPlayerManager().sendPacket(player, startLoadingChunksPacket); + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/dev/threeadd/packeteventssk/api/simple/PlayerSkinManager.java` around lines 158 - 165, teleportAsync returns a CompletableFuture and the START_LOADING_CHUNKS packet is currently sent immediately afterward, creating a race; change the flow so the START_LOADING_CHUNKS packet (WrapperPlayServerChangeGameState with Reason.START_LOADING_CHUNKS) is only sent after player.teleportAsync(player.getLocation()) completes by chaining the future (e.g., player.teleportAsync(...).thenRun(() -> PacketEvents.getAPI().getPlayerManager().sendPacket(player, startLoadingChunksPacket))) and handle errors on the future (e.g., exceptionally/logging) to avoid swallowing exceptions.src/main/java/dev/threeadd/packeteventssk/update/UpdateChecker.java-80-88 (1)
80-88:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
updateVersionFuturenever completes iflatestReleaseFutureis cancelled.When
latestReleaseFuture.cancel(true)is called (after the fix above), thethenApplycallback won't execute, leavingupdateVersionFuturein an incomplete state. Callers awaiting this future will hang indefinitely.Use
whenCompleteorhandleinstead to ensureupdateVersionFutureis always resolved.Suggested fix using handle()
- latestReleaseFuture.thenApply(version -> { - if (version.getUpdateVersion().compareTo(PLUGIN_VERSION) <= 0) { - updateVersionFuture.cancel(true); + latestReleaseFuture.handle((version, throwable) -> { + if (throwable != null || version == null) { + updateVersionFuture.completeExceptionally( + throwable != null ? throwable : new RuntimeException("Failed to fetch version")); + } else if (version.getUpdateVersion().compareTo(PLUGIN_VERSION) <= 0) { + updateVersionFuture.completeExceptionally(new RuntimeException("Already up to date")); } else { CURRENT_UPDATE_VERSION = version; updateVersionFuture.complete(CURRENT_UPDATE_VERSION); } return true; });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/dev/threeadd/packeteventssk/update/UpdateChecker.java` around lines 80 - 88, The callback on latestReleaseFuture uses thenApply so if latestReleaseFuture is cancelled the lambda never runs and updateVersionFuture remains unresolved; change the chaining to use handle or whenComplete on latestReleaseFuture (referencing latestReleaseFuture.thenApply(...) currently) and in that handler always complete or cancel updateVersionFuture (updateVersionFuture.complete(...), updateVersionFuture.cancel(...), or updateVersionFuture.completeExceptionally(...)) depending on whether the incoming result is null/exceptional or the version is older (compare using version.getUpdateVersion().compareTo(PLUGIN_VERSION)), and still set CURRENT_UPDATE_VERSION when appropriate so callers never hang.src/main/java/dev/threeadd/packeteventssk/update/UpdateChecker.java-94-103 (1)
94-103:⚠️ Potential issue | 🟠 Major | ⚡ Quick winResource leak:
BufferedReaderis never closed.The
BufferedReaderwrapping the URL stream is never closed, leaking the underlying socket connection. Use try-with-resources.Suggested fix
private static ModrinthVersion getLatestVersionFromModrinth() { try { URL url = new URI("https://api.modrinth.com/v2/project/CmkvxP9q/version").toURL(); - BufferedReader reader = new BufferedReader(new InputStreamReader(url.openStream())); - JsonArray elements = new Gson().fromJson(reader, JsonArray.class); - JsonElement latestVersion = elements.get(0); - return new ModrinthVersion(latestVersion); + try (BufferedReader reader = new BufferedReader(new InputStreamReader(url.openStream()))) { + JsonArray elements = new Gson().fromJson(reader, JsonArray.class); + JsonElement latestVersion = elements.get(0); + return new ModrinthVersion(latestVersion); + } } catch (IOException | URISyntaxException e) { LogUtil.error("Checking for updates failed"); } return null; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/dev/threeadd/packeteventssk/update/UpdateChecker.java` around lines 94 - 103, The BufferedReader created in UpdateChecker (the block that builds URL url, BufferedReader reader, reads JsonArray elements and returns new ModrinthVersion) is never closed; wrap the URL.openStream()/BufferedReader in a try-with-resources so the stream and reader are automatically closed (e.g., use try (BufferedReader reader = new BufferedReader(new InputStreamReader(url.openStream()))) { ... }) and move the JSON parsing/ModrinthVersion construction inside that try, keeping the existing catch for IOException | URISyntaxException and returning null on failure.src/main/java/dev/threeadd/packeteventssk/element/entity/section/SecExprNewFakeEntity.java-163-165 (1)
163-165:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid returning
[null]from the expression getter.Line 164 wraps a nullable result directly. When creation fails, this returns an array containing
nullinstead of no value, which can break downstream handling.Proposed fix
`@Override` protected WrapperEntity `@Nullable` [] get(Event event) { - return new WrapperEntity[]{createEntity(event)}; + WrapperEntity entity = createEntity(event); + return entity == null ? null : new WrapperEntity[]{entity}; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/dev/threeadd/packeteventssk/element/entity/section/SecExprNewFakeEntity.java` around lines 163 - 165, The getter SecExprNewFakeEntity.get(Event event) currently returns new WrapperEntity[]{createEntity(event)} which can yield an array containing a null element when createEntity(event) fails; change the method to call createEntity(event), check the result, and return either a single-element array containing the non-null WrapperEntity or an empty array (no elements) when createEntity returns null so downstream code never receives [null]; update the implementation around get(Event) and the createEntity call accordingly.src/main/java/dev/threeadd/packeteventssk/element/entity/expression/prop/ExprFakeEntityField.java-141-153 (1)
141-153:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTighten array compatibility before invoking field setters.
On Line 144, any array is accepted for any expected array type. That can push incompatible arrays into the setter and trigger runtime type failures.
Proposed fix
- if (!isCompatible && expected.isArray() && newValue.getClass().isArray()) { - isCompatible = true; - } + if (!isCompatible && expected.isArray() && newValue.getClass().isArray()) { + Class<?> componentType = expected.getComponentType(); + int len = Array.getLength(newValue); + boolean componentsCompatible = true; + for (int i = 0; i < len; i++) { + Object element = Array.get(newValue, i); + if (element != null && !componentType.isInstance(element)) { + componentsCompatible = false; + break; + } + } + isCompatible = componentsCompatible; + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/dev/threeadd/packeteventssk/element/entity/expression/prop/ExprFakeEntityField.java` around lines 141 - 153, The code in ExprFakeEntityField currently treats any array as compatible whenever expected.isArray(), which can still pass incompatible element types to targetField.setter(); tighten the check by, when expected.isArray() && newValue.getClass().isArray(), comparing the component types: obtain expected.getComponentType() and newValue.getClass().getComponentType() and require the new component type to be assignable to the expected component type (taking into account primitive<->wrapper differences if necessary) before setting; only call ((BiConsumer<WrapperEntity, Object>) targetField.setter()).accept(entity, newValue) if that component-type compatibility check passes, otherwise log the same Skript.warning and continue.build.gradle.kts-16-16 (1)
16-16:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove hardcoded developer-specific path.
The
serverLocationcontains an absolute Windows path with a specific username. This breaks build portability for other developers and should not be committed to the repository.Consider one of these solutions:
- Move to
gradle.properties(add to.gitignore)- Use a relative path with a sensible default
- Make it configurable with clear documentation
🔧 Suggested fix using gradle.properties
In
build.gradle.kts, change to:-val serverLocation = "C:/Users/jaspe/Desktop/Servers/packetSKTestServer/plugins" +val serverLocation: String by projectThen create a
gradle.propertiesfile (add to.gitignore):serverLocation=C:/Users/jaspe/Desktop/Servers/packetSKTestServer/pluginsAdd to
.gitignore:gradle.propertiesAnd provide a
gradle.properties.example:# Copy this file to gradle.properties and configure your server path serverLocation=path/to/your/server/plugins🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@build.gradle.kts` at line 16, The build currently hardcodes a developer-specific absolute path in the serverLocation variable in build.gradle.kts; replace this by reading a configurable property (e.g., project.findProperty("serverLocation") or System.getenv("SERVER_LOCATION")) with a sensible relative-path default (like "./plugins") to preserve portability, then move the developer-specific value into a local gradle.properties (and add gradle.properties to .gitignore) and provide a gradle.properties.example with a placeholder path; update any references to serverLocation in build.gradle.kts to use the new property lookup.src/main/java/dev/threeadd/packeteventssk/element/general/field/meta/DisplayMetaFieldRegistrar.java-82-84 (1)
82-84:⚠️ Potential issue | 🟠 Major | ⚡ Quick winShadow strength field is wired to the wrong setter.
Line 83 maps
"display shadow strength"tosetShadowRadius(...)instead ofsetShadowStrength(...), so writes hit the wrong property.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/dev/threeadd/packeteventssk/element/general/field/meta/DisplayMetaFieldRegistrar.java` around lines 82 - 84, The display shadow strength field is wired to the wrong setter: in DisplayMetaFieldRegistrar the mapping using AbstractDisplayMeta::getShadowStrength currently calls meta.setShadowRadius(newNum.floatValue()); change that call to meta.setShadowStrength(newNum.floatValue()) so the "display shadow strength" field reads via getShadowStrength and writes to setShadowStrength (ensure the method name matches the AbstractDisplayMeta API).src/main/java/dev/threeadd/packeteventssk/api/util/field/BaseFieldRegistry.java-46-47 (1)
46-47:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard against empty field-name varargs to avoid runtime crashes.
Line 47 and Line 53 dereference
names[0]without validating input. Calling either method with no names throwsArrayIndexOutOfBoundsExceptionduring registration.Proposed fix
public <V> Builder<T> requiredField(Class<V> expectedType, Function<T, V> getter, `@Nullable` BiConsumer<T, V> setter, String... names) { + if (names.length == 0) { + throw new IllegalArgumentException("At least one field name is required"); + } accessors.add(new FieldAccessor<>(names[0], Arrays.copyOfRange(names, 1, names.length), expectedType, false, (Function<BaseType, V>) getter, (BiConsumer<BaseType, V>) setter)); return this; } @@ public <V> Builder<T> optionalField(Class<V> expectedType, Function<T, V> getter, `@Nullable` BiConsumer<T, V> setter, String... names) { + if (names.length == 0) { + throw new IllegalArgumentException("At least one field name is required"); + } accessors.add(new FieldAccessor<>(names[0], Arrays.copyOfRange(names, 1, names.length), expectedType, true, (Function<BaseType, V>) getter, (BiConsumer<BaseType, V>) setter)); return this; }Also applies to: 52-53
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/dev/threeadd/packeteventssk/api/util/field/BaseFieldRegistry.java` around lines 46 - 47, The Builder<T> methods (requiredField and the similar optionalField overload) currently dereference names[0] without validation causing ArrayIndexOutOfBoundsException when called with no names; add an input guard at the start of each method to check that names is non-null and names.length>0 and throw an IllegalArgumentException (with a clear message like "field names must contain at least one name") if the check fails so the call site gets a clear error instead of an OOB exception; update both requiredField(...) and optionalField(...) in class BaseFieldRegistry.Builder to perform this validation before constructing the new FieldAccessor.src/main/java/dev/threeadd/packeteventssk/api/util/field/InheritingFieldRegistry.java-20-29 (1)
20-29:⚠️ Potential issue | 🟠 Major | ⚡ Quick winProtect parent-chain traversal from cycles.
getSchemahas no cycle detection. IfgetParentKeyever forms a cycle, this loop never terminates and can hang the server thread.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/dev/threeadd/packeteventssk/api/util/field/InheritingFieldRegistry.java` around lines 20 - 29, In InheritingFieldRegistry, getSchema(K key) currently follows the parent chain via getParentKey with no cycle detection; add cycle protection by tracking visited keys (e.g., a HashSet<K> visited) as you walk the chain in getSchema and stop if a key is encountered twice—on detection either return null or throw a specific exception/log an error to avoid an infinite loop; ensure you still call super.getSchema for each current key and update visited before advancing to getParentKey.src/main/java/dev/threeadd/packeteventssk/api/util/field/InheritingFieldRegistry.java-68-83 (1)
68-83:⚠️ Potential issue | 🟠 Major | ⚡ Quick winChild accessors cannot override inherited ones with current merge order.
Parent accessors are added before child accessors, and lookup returns the first match. A child field with the same name/alias is effectively ignored.
Proposed fix
- mergedAccessors.addAll(parentSchema.accessors()); + mergedAccessors.addAll(parentSchema.accessors()); @@ - mergedAccessors.addAll(this.accessors); + // ensure child definitions override parent by removing conflicting inherited entries + mergedAccessors.removeIf(parent -> + this.accessors.stream().anyMatch(child -> child.matches(parent.name()))); + mergedAccessors.addAll(this.accessors);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/dev/threeadd/packeteventssk/api/util/field/InheritingFieldRegistry.java` around lines 68 - 83, The merge currently appends parent accessors before child accessors so children cannot override parents; change InheritingFieldRegistry's merge logic (the mergedAccessors construction that uses classToTypeMap, REGISTRY, FieldSchema, this.accessors, constructor) to start with the child's accessors and then add parent accessors only when they do not conflict with any child accessor (compare accessor identity by field name and aliases), i.e., build a set of child accessor names/aliases from this.accessors, add those first to mergedAccessors, then iterate parentSchema.accessors() and add each parent accessor only if none of its names/aliases exist in the child set, then put the new FieldSchema into REGISTRY.src/main/java/dev/threeadd/packeteventssk/element/general/field/meta/LivingMetaFieldRegistrar.java-23-24 (1)
23-24:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid truncating health values to integers.
Line 23 and Line 39 use
intValue(), which drops fractional health values. This can produce incorrect state when scripts pass decimal health.Suggested fix
- (meta, newNum) -> meta.setHealth(newNum.intValue()), + (meta, newNum) -> meta.setHealth(newNum.floatValue()), ... - meta.setHealth(health.intValue()); + meta.setHealth(health.floatValue());Also applies to: 37-40
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/dev/threeadd/packeteventssk/element/general/field/meta/LivingMetaFieldRegistrar.java` around lines 23 - 24, The health field registration in LivingMetaFieldRegistrar is truncating fractional health by calling newNum.intValue(); update the lambda handlers that call meta.setHealth(newNum.intValue()) (and the similar occurrence around lines 37–40) to use the appropriate floating conversion (e.g., newNum.doubleValue() or newNum.floatValue()) so decimals are preserved and ensure meta.setHealth is called with the matching double/float type; adjust any method signature or cast if necessary to accept the floating type.src/main/java/dev/threeadd/packeteventssk/element/general/field/meta/InteractionMetaFieldRegistrar.java-20-24 (1)
20-24:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve numeric precision for interaction dimensions.
Line 20, Line 24, Line 36, and Line 41 coerce
Numbertoint, which truncates fractional width/height values. This silently changes user input and metadata behavior.Suggested fix
- (meta, newNum) -> meta.setWidth(newNum.intValue()), + (meta, newNum) -> meta.setWidth(newNum.floatValue()), ... - (meta, newNum) -> meta.setHeight(newNum.intValue()), + (meta, newNum) -> meta.setHeight(newNum.floatValue()), ... - meta.setWidth(width.intValue()); + meta.setWidth(width.floatValue()); ... - meta.setHeight(height.intValue()); + meta.setHeight(height.floatValue());Also applies to: 34-42
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/dev/threeadd/packeteventssk/element/general/field/meta/InteractionMetaFieldRegistrar.java` around lines 20 - 24, The registrar currently truncates fractional interaction dimensions by calling newNum.intValue() in the optionalField(Number.class, InteractionMeta::getWidth, (meta, newNum) -> meta.setWidth(newNum.intValue()), "interaction width") and the analogous height lambda; update these to preserve precision by using newNum.doubleValue() (i.e. (meta, newNum) -> meta.setWidth(newNum.doubleValue()) and -> meta.setHeight(newNum.doubleValue())) and update InteractionMeta's getters/setters (getWidth/getHeight and setWidth/setHeight) and their types from int to double so the metadata stores and returns fractional values instead of truncated ints.src/main/java/dev/threeadd/packeteventssk/element/general/section/SecExprNewMeta.java-168-170 (1)
168-170:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not return arrays containing a null meta instance.
Line 169 currently returns
new EntityMeta[]{createMeta(event)}even when creation fails, yielding[null]. This can trigger downstream NPEs in Skript pipelines.Suggested fix
`@Override` protected EntityMeta `@Nullable` [] get(Event event) { - return new EntityMeta[]{createMeta(event)}; + EntityMeta meta = createMeta(event); + return meta == null ? null : new EntityMeta[]{meta}; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/dev/threeadd/packeteventssk/element/general/section/SecExprNewMeta.java` around lines 168 - 170, SecExprNewMeta.get currently returns new EntityMeta[]{createMeta(event)} which can produce an array [null]; change it to call createMeta(event) into a local variable, check if that result is null and return null (or an empty array per project convention) instead of returning an array containing a null element, otherwise return new EntityMeta[]{meta}; ensure you respect the `@Nullable` return and update get to use the local variable (referencing SecExprNewMeta.get and createMeta).src/main/java/dev/threeadd/packeteventssk/element/general/expression/prop/ExprMetaField.java-152-154 (1)
152-154:⚠️ Potential issue | 🟠 Major | ⚡ Quick winArray compatibility check is too permissive for SET.
Line 152–154 marks all array-to-array assignments as compatible without checking component type. This can route incompatible values into the setter and fail at runtime.
Suggested fix
- if (!isCompatible && expected.isArray() && newValue.getClass().isArray()) { - isCompatible = true; + if (!isCompatible && expected.isArray() && newValue.getClass().isArray()) { + Class<?> expectedComponent = expected.getComponentType(); + Class<?> actualComponent = newValue.getClass().getComponentType(); + isCompatible = expectedComponent != null + && actualComponent != null + && (expectedComponent.isAssignableFrom(actualComponent) + || expectedComponent == Object.class); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/dev/threeadd/packeteventssk/element/general/expression/prop/ExprMetaField.java` around lines 152 - 154, The current array-to-array compatibility branch in ExprMetaField (the block that sets isCompatible = true when expected.isArray() && newValue.getClass().isArray()) is too permissive; change it to check component-type compatibility instead of accepting any arrays. Update the compatibility check in the method around ExprMetaField so that when both expected.isArray() and newValue.getClass().isArray() you compare expected.getComponentType() against newValue.getClass().getComponentType() (using isAssignableFrom or equivalent), and also handle primitive-vs-wrapper component cases (e.g., map int[] vs Integer[] appropriately) before marking isCompatible true so only compatible array assignments reach the setter.src/main/java/dev/threeadd/packeteventssk/element/general/section/SecExprNewPacket.java-91-96 (1)
91-96:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard
type == nullbefore usinggetName().At Line 95,
this.type.getName()can throw if Line 91 resolves tonull(invalid/empty literal path). Fail early with a clear error before schema lookup.Proposed patch
this.type = packetTypeLiteral.getSingle(); + if (this.type == null) { + Skript.error("The packet type literal resolved to null."); + return false; + } this.schema = PacketFieldRegistry.INSTANCE.getSchema(this.type); if (this.schema == null) { Skript.error("Packet creation for " + this.type.getName() + " is not currently supported. Consider creating/handling it through reflection."); return false; // can't return an empty packet so this expr can't be used }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/dev/threeadd/packeteventssk/element/general/section/SecExprNewPacket.java` around lines 91 - 96, Guard against a null packet type before using it: check the result of packetTypeLiteral.getSingle() (stored in this.type) for null and call Skript.error with a clear message and return false if it's null, before calling PacketFieldRegistry.INSTANCE.getSchema(this.type) or this.type.getName(); update the logic in SecExprNewPacket around the fields this.type and this.schema so the null-check happens first and prevents calling this.type.getName() when packetTypeLiteral is empty.src/main/java/dev/threeadd/packeteventssk/element/general/field/packet/SkBeePacketFieldRegistrar.java-25-31 (1)
25-31:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix the SkBee availability gate flow and log message.
Line 26 logs "Hooked into SkBee NBT using NBT-API" while in the
!enabledbranch, which is misleading. Additionally, line 30 catchesClassNotFoundExceptionand logs an error but lacks areturn falsestatement, allowing registration to proceed unexpectedly. Both paths should cleanly short-circuit.Proposed patch
boolean enabled = (boolean) nbtApiClass.getMethod("isEnabled").invoke(null); if (!enabled) { - LogUtil.info("Hooked into SkBee NBT using NBT-API"); + LogUtil.info("SkBee NBT API is disabled, PacketEventsSK NBT packet fields will not be registered"); return false; } } catch (ClassNotFoundException ignored) { LogUtil.error("SkBee not found, PacketEventsSK elements depending on NBT will not be registered"); + return false; } catch (Exception e) { throw new IllegalStateException("Failed to hook into SkBee NBT", e); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/dev/threeadd/packeteventssk/element/general/field/packet/SkBeePacketFieldRegistrar.java` around lines 25 - 31, In SkBeePacketFieldRegistrar, fix the SkBee availability gate: change the misleading LogUtil.info message inside the !enabled branch to indicate SkBee NBT integration is disabled (e.g., "SkBee NBT integration disabled, skipping NBT-API hook") and keep the existing return false; also ensure the ClassNotFoundException catch block both logs the error via LogUtil.error("SkBee not found, PacketEventsSK elements depending on NBT will not be registered") and immediately returns false so registration is short-circuited; likewise, if there is a generic Exception catch (Exception e) in the same method, log the exception and return false to prevent continuing when detection fails.
🟡 Minor comments (4)
src/main/java/dev/threeadd/packeteventssk/api/util/ConversionUtil.java-42-45 (1)
42-45:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPrecision loss: casting to
floatwhenVector3dexpectsdouble.The method casts to
floatbutVector3dtakesdoublearguments. This unnecessarily truncates precision.Proposed fix
public static Vector3d toPeVectorD(Vector vector) { if (vector == null) return null; - return new Vector3d((float) vector.getX(), (float) vector.getY(), (float) vector.getZ()); + return new Vector3d(vector.getX(), vector.getY(), vector.getZ()); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/dev/threeadd/packeteventssk/api/util/ConversionUtil.java` around lines 42 - 45, The toPeVectorD method in ConversionUtil is unnecessarily casting coordinates to float causing precision loss; update to pass the vector's double values directly to the Vector3d constructor (i.e., remove the (float) casts) so new Vector3d receives double arguments from vector.getX(), vector.getY(), vector.getZ(); keep the existing null check and method signature unchanged.src/main/java/dev/threeadd/packeteventssk/api/simple/PlayerSkinManager.java-39-39 (1)
39-39:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRedundant
Bukkit.getPlayer()call.
targetis already aPlayerobject, soBukkit.getPlayer(target.getUniqueId())is unnecessary and adds overhead.Proposed fix
- if (skin.equals(SkinManager.getPlayer(Bukkit.getPlayer(target.getUniqueId())))) { + if (skin.equals(SkinManager.getPlayer(target))) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/dev/threeadd/packeteventssk/api/simple/PlayerSkinManager.java` at line 39, The code performs a redundant Bukkit.getPlayer(...) call: inside PlayerSkinManager use the existing Player instance `target` directly instead of looking it up again; change the comparison from skin.equals(SkinManager.getPlayer(Bukkit.getPlayer(target.getUniqueId()))) to skin.equals(SkinManager.getPlayer(target)) so `target` is used as the argument to SkinManager.getPlayer and avoid the unnecessary Bukkit.getPlayer lookup.src/main/java/dev/threeadd/packeteventssk/update/UpdateChecker.java-57-60 (1)
57-60:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
exceptionallytreats all failures as "up to date".Network failures, JSON parse errors, and cancellation all trigger the "Plugin is up to date!" message, which is misleading. Consider distinguishing between "already current" and "check failed" scenarios.
Suggested improvement
}).exceptionally(throwable -> { - LogUtil.mini("<green>Plugin is up to date!"); + if (throwable.getCause() instanceof RuntimeException && + throwable.getCause().getMessage() != null && + throwable.getCause().getMessage().contains("up to date")) { + LogUtil.mini("<green>Plugin is up to date!"); + } else { + LogUtil.info("Could not determine update status"); + } return true; });Alternatively, use a custom exception class for cleaner discrimination.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/dev/threeadd/packeteventssk/update/UpdateChecker.java` around lines 57 - 60, The current CompletableFuture chain in UpdateChecker uses .exceptionally(...) to always log "Plugin is up to date!" on any failure; change the error handling so that only a successful version-compare path logs "up to date" while network/parse/cancellation errors log a distinct failure message (or propagate). Locate the CompletableFuture callback (the lambda currently passed to .exceptionally) and replace it with a combination of .thenApply/.thenAccept for the normal version-check logic (compare remoteVersion to localVersion and log "<green>Plugin is up to date!" only when equal/newer) and a separate .exceptionally or .whenComplete block that logs a clear error message including the throwable for failures; alternatively implement a custom exception type and use .handle(...) to branch success vs failure. Ensure you reference the UpdateChecker method that builds the future and only log success inside the success branch while logging errors with throwable details in the failure branch.src/main/java/dev/threeadd/packeteventssk/element/general/Types.java-197-197 (1)
197-197:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSkin debug text shows the wrong field for
value.Line 197 prints
prop.getSignature()for bothvalueandsignature, so the value output is incorrect and debugging skin payloads becomes misleading.Suggested fix
- return "skin: " + skin.properties().stream().map(prop -> "value: '" + prop.getSignature() + "', signature: '" + prop.getSignature() + "'").toList(); + return "skin: " + skin.properties().stream().map(prop -> "value: '" + prop.getValue() + "', signature: '" + prop.getSignature() + "'").toList();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/dev/threeadd/packeteventssk/element/general/Types.java` at line 197, In Types.java the skin debug string builds each property using prop.getSignature() for both fields; change the mapping to use prop.getValue() for the "value" portion and prop.getSignature() for the "signature" portion (i.e., update the lambda used in skin.properties().stream().map(...) to call prop.getValue() for the value field so the debug output correctly shows value and signature).
🧹 Nitpick comments (9)
src/main/java/dev/threeadd/packeteventssk/api/util/LogUtil.java (1)
17-45: 💤 Low valueConsider adding a private constructor to prevent instantiation.
This is a utility class with only static methods. Adding a private constructor would make the design intent explicit and prevent accidental instantiation.
public class LogUtil { + private LogUtil() { + // Utility class + } + private static final MiniMessage MM = MiniMessage.miniMessage();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/dev/threeadd/packeteventssk/api/util/LogUtil.java` around lines 17 - 45, LogUtil is a pure utility class and should not be instantiated; add a private no-arg constructor in the LogUtil class (e.g., private LogUtil() { throw new AssertionError("no instances"); }) to prevent instantiation and make intent explicit; optionally also mark the class final if you want to prohibit subclassing—this change touches the LogUtil class constructor only and does not affect existing static methods like sendMessage, sendRichMessage, info, warning, error, or mini.src/main/java/dev/threeadd/packeteventssk/api/util/ConversionUtil.java (1)
81-91: 💤 Low valueMissing null guards for consistency.
Other conversion methods in this class return
nullwhen passednull. These methods will throwNullPointerExceptioninstead, which is inconsistent with the established pattern.Add null guards for consistency
public static long toTicks(Timespan timespan) { + if (timespan == null) return 0L; return timespan.getAs(Timespan.TimePeriod.TICK); } public static Display.Billboard toBillboard(AbstractDisplayMeta.BillboardConstraints billboardConstraints) { + if (billboardConstraints == null) return null; return Display.Billboard.valueOf(billboardConstraints.name()); } public static AbstractDisplayMeta.BillboardConstraints toBillboardConstraints(Display.Billboard billboard) { + if (billboard == null) return null; return AbstractDisplayMeta.BillboardConstraints.valueOf(billboard.name()); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/dev/threeadd/packeteventssk/api/util/ConversionUtil.java` around lines 81 - 91, The three conversion methods (toTicks, toBillboard, toBillboardConstraints) currently throw NullPointerException if given null, which is inconsistent; add null guards so each returns null when its input is null: in toTicks check if Timespan timespan is null and return null before calling timespan.getAs(Timespan.TimePeriod.TICK), in toBillboard check if AbstractDisplayMeta.BillboardConstraints billboardConstraints is null and return null before calling Display.Billboard.valueOf(...), and in toBillboardConstraints check if Display.Billboard billboard is null and return null before calling AbstractDisplayMeta.BillboardConstraints.valueOf(...).src/main/java/dev/threeadd/packeteventssk/api/simple/PlayerSkinManager.java (1)
54-54: 💤 Low valueVariable name violates Java naming conventions.
Viewershould beviewer(camelCase for local variables).Proposed fix
- for (Player Viewer : viewers) { - targetMap.put(Viewer.getUniqueId(), skin); + for (Player viewer : viewers) { + targetMap.put(viewer.getUniqueId(), skin);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/dev/threeadd/packeteventssk/api/simple/PlayerSkinManager.java` at line 54, The local loop variable in the for-each inside PlayerSkinManager uses an uppercase name ("Viewer") which violates Java naming conventions; change the loop to use a camelCase name (e.g., "viewer") in the for-each header (for (Player viewer : viewers)) and update all references inside that loop body to the new name so the code compiles and follows conventions.src/main/java/dev/threeadd/packeteventssk/update/JoinUpdateListener.java (1)
19-23: 💤 Low value
thenAcceptcallback may execute on async thread.When
getUpdateVersion(true)completes asynchronously, thethenAcceptcallback runs on whichever thread completes the future (the Bukkit async scheduler thread). While Bukkit'sPlayer.sendMessage()is generally thread-safe, consider usingthenAcceptAsyncwith the main thread executor or wrapping the message sending in a sync task for consistency with Bukkit conventions.Suggested fix to ensure main thread execution
Bukkit.getScheduler().runTaskLater(PacketEventsSK.getInstance(), () -> - UpdateChecker.getUpdateVersion(true).thenAccept(version -> { - LogUtil.sendRichMessage(player, "Update available: <green>%s", version.getUpdateVersion()); - LogUtil.sendRichMessage(player, "Download at: <green>%s", version.getUpdateLink()); - }), 30L); + UpdateChecker.getUpdateVersion(true).thenAccept(version -> + Bukkit.getScheduler().runTask(PacketEventsSK.getInstance(), () -> { + LogUtil.sendRichMessage(player, "Update available: <green>%s", version.getUpdateVersion()); + LogUtil.sendRichMessage(player, "Download at: <green>%s", version.getUpdateLink()); + }) + ), 30L);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/dev/threeadd/packeteventssk/update/JoinUpdateListener.java` around lines 19 - 23, The callback passed to UpdateChecker.getUpdateVersion(true) uses thenAccept which may run on an async thread; update JoinUpdateListener so the LogUtil.sendRichMessage calls execute on the main Bukkit thread—either use thenAcceptAsync with a main-thread executor or change the callback to schedule a synchronous task via Bukkit.getScheduler().runTask(PacketEventsSK.getInstance(), () -> { LogUtil.sendRichMessage(player, ...); });—target the thenAccept usage in JoinUpdateListener and ensure the code that calls LogUtil.sendRichMessage runs inside a Bukkit main-thread runnable.src/main/java/dev/threeadd/packeteventssk/update/ModrinthVersion.java (1)
32-34: 💤 Low valueReturning mutable internal list.
getSupportedVersions()returns the internalArrayListdirectly, allowing callers to modify the internal state. Consider returning an unmodifiable view.Suggested fix
public List<Version> getSupportedVersions() { - return this.supportedVersions; + return List.copyOf(this.supportedVersions); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/dev/threeadd/packeteventssk/update/ModrinthVersion.java` around lines 32 - 34, The getter ModrinthVersion.getSupportedVersions() exposes the internal mutable field supportedVersions; change it to return an unmodifiable view or a defensive copy (e.g., Collections.unmodifiableList(supportedVersions) or new ArrayList<>(supportedVersions)) so external callers cannot mutate the internal ArrayList — update the getSupportedVersions method in class ModrinthVersion to return the safer immutable/copy result.src/main/resources/config.yml (1)
11-12: ⚡ Quick winConsider defaulting async to true.
The update checker defaults to running synchronously on the main thread. If the Modrinth API is slow or unreachable, this could delay server startup.
Suggested change
- # If the update checker should run asynchronously or on the main thread - async: false + # If the update checker should run asynchronously or on the main thread + async: true🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/resources/config.yml` around lines 11 - 12, Change the default for the update checker from synchronous to asynchronous by setting the config key "async" to true in the update checker configuration (the "async" entry in src/main/resources/config.yml), and update any associated documentation/comments and tests that assume the old default; ensure that components reading this config (the update-checker initialization code that consumes the "async" flag) continue to behave correctly when async is true and that any startup blocking logic is guarded accordingly.src/main/java/dev/threeadd/packeteventssk/element/general/field/meta/DisplayMetaFieldRegistrar.java (1)
292-300: ⚡ Quick winPreserve decimal values for shadow radius/strength.
Line 294 and Line 299 coerce to
intValue(), truncating fractional values. These fields are handled as floating-point elsewhere in this registrar.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/dev/threeadd/packeteventssk/element/general/field/meta/DisplayMetaFieldRegistrar.java` around lines 292 - 300, The code in DisplayMetaFieldRegistrar is truncating fractional shadow values by using Number.intValue() for context.getOptional("display shadow radius", Number.class) and ("display shadow strength", Number.class); change these to preserve decimals by reading the Number as a floating type (e.g. call Number.doubleValue() or floatValue()) and pass that floating value to the meta setters (meta.setShadowRadius(...) and meta.setShadowStrength(...)) so the registrar retains fractional shadow radius/strength like the rest of the class.src/main/java/dev/threeadd/packeteventssk/api/util/field/FieldAccessor.java (1)
8-15: ⚡ Quick winDefensively copy
aliasesto keep accessor metadata immutable.The record currently stores and exposes a mutable array reference; callers can mutate aliases after registration and change matching behavior at runtime.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/dev/threeadd/packeteventssk/api/util/field/FieldAccessor.java` around lines 8 - 15, FieldAccessor currently stores the mutable String[] aliases reference; change the record to defensively copy aliases in its canonical constructor (store Arrays.copyOf(aliases, aliases.length) or equivalent) and ensure any accessor that exposes aliases (the implicit aliases() accessor) returns an immutable/copy view—e.g., replace direct exposure with Arrays.copyOf(this.aliases, this.aliases.length) or wrap in List.of(...) if you change the component type—so callers cannot mutate the internal array; update FieldAccessor's canonical constructor and any place that returns or assigns the aliases field accordingly.src/main/java/dev/threeadd/packeteventssk/api/util/field/BaseFieldRegistry.java (1)
27-29: ⚡ Quick winDo not expose a live mutable view of registry values.
Line 28 returns
REGISTRY.values()directly, which allows external removals and mutates internal state. Return an unmodifiable snapshot/view instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/dev/threeadd/packeteventssk/api/util/field/BaseFieldRegistry.java` around lines 27 - 29, getAllSchemas() currently returns REGISTRY.values() which exposes a live mutable view; change it to return an unmodifiable defensive copy of the values instead (e.g., create a new collection from REGISTRY.values() and wrap it with an unmodifiable view or use List.copyOf) so callers can't mutate the internal REGISTRY; update the method getAllSchemas() to return Collections.unmodifiableCollection(new ArrayList<>(REGISTRY.values())) (or List.copyOf(REGISTRY.values())) to protect the internal state of REGISTRY and still satisfy the Collection<FieldSchema<K, BaseType>> return type.
fixes: nullability warnings (they are required fields, using appropriate getters)
fixes: all registries field getters and filed accessors should match
Owner
Author
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 108 out of 109 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (2)
src/main/java/dev/threeadd/packeteventssk/update/UpdateChecker.java:93
getUpdateVersioncan return aCompletableFuturethat never completes: whenlatest == nullyou cancellatestReleaseFuture, but you never complete/cancelupdateVersionFuture, and thethenApply(...)attached tolatestReleaseFuturewon't run on cancellation. EnsureupdateVersionFutureis completed exceptionally/cancelled when the Modrinth call fails/cancels, and consider propagating exceptions withwhenCompleteinstead of manualcancel(true)branching.
src/main/java/dev/threeadd/packeteventssk/update/UpdateChecker.java:105getLatestVersionFromModrinth()usesurl.openStream()without timeouts and doesn't close the reader, which can hang the server thread (especially whenasync: false) and leak resources. Use anHttpURLConnection(orHttpClient) with connect/read timeouts and try-with-resources, and include the exception details in the log so failures are diagnosable.
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Refactor
Documentation