Conversation
fix: PacketSendOrReceiveEvent always returning true for canExecuteAsynchronously
…vent causing major player ping issues and netty channels being occupied.
fix: packet priority syntax conflict with StructEvent fix: allowing non-MONITOR priority for async and sync fix: ExprPacketField working for non-netty processed events fix: ExprPacketField and EffCancelPacket allowing alteration/cancellation of a MONITOR priority event
…method since that's where they should apply
add: listening to all packets for the on packet event add: property expr packet's packettype
…pt for re-encoding all events (packetevents does this by default anyway unless the setting is disabled)
refactor: use paper-plugin instead of plugin
bump: version of SKR
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR upgrades PacketEventsSK to v1.1.1: adds PacketListenerPriority routing across packet events and listeners, replaces static EventPacketMapper with reflective wrapper construction, migrates plugin metadata to Paper descriptor, introduces CI/JDK tooling and Qodana config, adds a packet-type expression and Skript priority parsing, and expands debug output depth. ChangesBuild Infrastructure, CI, and Plugin Configuration
Packet Event Priority System and Core Refactoring
Skript Elements and Expressions with Priority Support
Debug Utilities Enhancement
🎯 3 (Moderate) | ⏱️ ~25 minutes
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (1)
src/main/java/dev/threeadd/packeteventssk/api/general/PacketSendOrReceiveListener.java (1)
115-120: ⚡ Quick winPreserve the reflection failure cause in thrown exceptions.
Line 117 and Line 120 throw new
IllegalStateExceptionwithout attaching the original exception, which makes wrapper-constructor failures hard to debug.Proposed fix
- } catch (InvocationTargetException | InstantiationException | IllegalAccessException | NoSuchMethodError | - NoSuchMethodException e) { - throw new IllegalStateException("Couldn't create packet for: " + type); + } catch (InvocationTargetException | InstantiationException | IllegalAccessException | NoSuchMethodError | + NoSuchMethodException e) { + throw new IllegalStateException("Couldn't create packet for: " + type, 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/api/general/PacketSendOrReceiveListener.java` around lines 115 - 120, The thrown IllegalStateExceptions lose the original reflection failure cause; update the try/catch in PacketSendOrReceiveListener so that the catch block rethrows using the caught exception as the cause (e.g., new IllegalStateException("Couldn't create packet for: " + type, e)), and if you need a fallback throw after the try/catch, capture the caught exception into a local Throwable (e.g., lastFailure) inside the catch and use that as the cause in the final throw to preserve the root cause from InvocationTargetException/InstantiationException/IllegalAccessException/NoSuchMethodError/NoSuchMethodException.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/qodana_code_quality.yml:
- Around line 18-22: The workflow currently grants overly broad repo permissions
by setting permissions.contents to write; since the job uses push-fixes: 'none',
change permissions.contents from write to read to follow least privilege. Update
the permissions block (permissions.contents) in
.github/workflows/qodana_code_quality.yml to use read instead of write and keep
pull-requests and checks permissions as-is; confirm there are no other steps
that require contents: write before committing the change.
- Around line 9-10: The qodana job unconditionally reads secrets.QODANA_TOKEN
which fails for forked PRs because GitHub doesn't expose secrets; update the
qodana job (job name "qodana") to include a job-level guard that prevents
running on forked pull requests, e.g. add an if condition that only allows the
job when the PR comes from the same repository (e.g. if: github.event_name !=
'pull_request' || github.event.pull_request.head.repo.full_name ==
github.repository) so the job won't attempt to access secrets.QODANA_TOKEN for
forks.
In `@build.gradle.kts`:
- Line 36: Replace the dynamic version selector so the build is pinning an exact
Paper dev bundle: locate the call to
paperweight.paperDevBundle("$serverVersion.build.+") and change it to use a
concrete build number (e.g.,
paperweight.paperDevBundle("$serverVersion.build.<BUILD_NUMBER>")) so it no
longer uses the '+' wildcard; update any variable or constant that supplies the
build number accordingly and document the chosen build number in the file or
project docs for reproducibility.
- Around line 68-70: The build currently sets
sourceCompatibility/targetCompatibility to "21" in the compileJava block but
other JVM metadata (lines declaring Java 25) are still targeting Java 25; update
the compile tasks to use options.release = 21 and unify all JVM
toolchain/metadata settings to Java 21 so compile-time and generated metadata
match. Specifically, replace/augment usage of
sourceCompatibility/targetCompatibility in the compileJava (and compileTestJava
if present) configuration with options.release = 21, ensure any javaToolchains
or compileKotlin/compileTestKotlin targets also specify Java 21, and remove or
change any lingering references to Java 25 so all produced class metadata and
dependency selection use Java 21.
In
`@src/main/java/dev/threeadd/packeteventssk/api/general/PacketSendOrReceiveListener.java`:
- Around line 43-47: The current containsKey + put sequence on the
ConcurrentHashMap INSTANCES can race and register duplicate listeners; replace
that pattern with an atomic computeIfAbsent on INSTANCES to create and store a
PacketSendOrReceiveListener for the given priority only once, and call
PacketEvents.getAPI().getEventManager().registerListener(...) only when
computeIfAbsent created the new listener (use the returned instance to determine
whether registration is needed).
In
`@src/main/java/dev/threeadd/packeteventssk/api/general/PacketTypeRegistry.java`:
- Around line 15-16: The code caches a single Iterator instance in
PACKETS_ITERATOR which is consumed on first use and causes subsequent
getAllPacketsIterator() callers to get empty/partial results; change the
implementation so PACKETS_ITERATOR is not a shared, static consumed
iterator—instead create and return a fresh iterator (or use an unmodifiable
Collection/stream) each time getAllPacketsIterator() is called, e.g. iterate
over the underlying packet collection of PacketTypeCommon entries within
PacketTypeRegistry and construct a new Iterator for every call rather than
reusing the PACKETS_ITERATOR field.
In
`@src/main/java/dev/threeadd/packeteventssk/element/general/effect/EffCancelPacket.java`:
- Around line 51-53: The init check in EffCancelPacket currently uses "if
(data.getPriority() != PacketListenerPriority.MONITOR)" which wrongly rejects
all priorities except MONITOR; change it to check equality so MONITOR is
rejected: in the EffCancelPacket init method (the block referencing
data.getPriority() and Skript.error), replace the condition with
"data.getPriority() == PacketListenerPriority.MONITOR" and keep the Skript.error
and return false to block MONITOR only.
In
`@src/main/java/dev/threeadd/packeteventssk/element/general/expressions/prop/ExprPacketField.java`:
- Around line 119-127: The guard that prevents altering packets only runs when
data.getPacketType() != null, letting "on any packet" bypass NETTY and MONITOR
checks; in ExprPacketField adjust the conditional so the checks against
data.getProcessType() and data.getPriority() (calls to data.getProcessType(),
EvtPacketSendOrReceive.ProcessType.NETTY, and PacketListenerPriority.MONITOR and
the Skript.error calls) run for all packet events (remove or move the packetType
!= null gate) so SET cannot bypass the NETTY/MONITOR protections for "on any
packet".
---
Nitpick comments:
In
`@src/main/java/dev/threeadd/packeteventssk/api/general/PacketSendOrReceiveListener.java`:
- Around line 115-120: The thrown IllegalStateExceptions lose the original
reflection failure cause; update the try/catch in PacketSendOrReceiveListener so
that the catch block rethrows using the caught exception as the cause (e.g., new
IllegalStateException("Couldn't create packet for: " + type, e)), and if you
need a fallback throw after the try/catch, capture the caught exception into a
local Throwable (e.g., lastFailure) inside the catch and use that as the cause
in the final throw to preserve the root cause from
InvocationTargetException/InstantiationException/IllegalAccessException/NoSuchMethodError/NoSuchMethodException.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8238f2a2-87d4-416e-ac85-02a4368d6de1
📒 Files selected for processing (24)
.github/workflows/gradle-build.yml.github/workflows/qodana_code_quality.ymlREADME.mdbuild.gradle.ktsqodana.yamlsrc/main/java/dev/threeadd/packeteventssk/PacketEventsSK.javasrc/main/java/dev/threeadd/packeteventssk/api/general/EntityTracker.javasrc/main/java/dev/threeadd/packeteventssk/api/general/EventPacketMapper.javasrc/main/java/dev/threeadd/packeteventssk/api/general/PacketConstructorRegistry.javasrc/main/java/dev/threeadd/packeteventssk/api/general/PacketSendOrReceiveEvent.javasrc/main/java/dev/threeadd/packeteventssk/api/general/PacketSendOrReceiveListener.javasrc/main/java/dev/threeadd/packeteventssk/api/general/PacketTypeRegistry.javasrc/main/java/dev/threeadd/packeteventssk/api/simple/PlayerSkinManager.javasrc/main/java/dev/threeadd/packeteventssk/api/util/DebugUtil.javasrc/main/java/dev/threeadd/packeteventssk/element/general/GeneralElementRegistration.javasrc/main/java/dev/threeadd/packeteventssk/element/general/Types.javasrc/main/java/dev/threeadd/packeteventssk/element/general/effect/EffCancelPacket.javasrc/main/java/dev/threeadd/packeteventssk/element/general/effect/EffSendOrReceivePacket.javasrc/main/java/dev/threeadd/packeteventssk/element/general/event/EvtPacketSendOrReceive.javasrc/main/java/dev/threeadd/packeteventssk/element/general/expressions/prop/ExprPacketField.javasrc/main/java/dev/threeadd/packeteventssk/element/general/expressions/prop/ExprPacketPacketType.javasrc/main/resources/config.ymlsrc/main/resources/paper-plugin.ymlsrc/main/resources/plugin.yml
💤 Files with no reviewable changes (3)
- src/main/java/dev/threeadd/packeteventssk/api/general/EventPacketMapper.java
- src/main/resources/plugin.yml
- src/main/java/dev/threeadd/packeteventssk/PacketEventsSK.java
Qodana for JVM6 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.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/main/java/dev/threeadd/packeteventssk/api/general/PacketTypeRegistry.java`:
- Around line 42-44: You added a new public API method getAllPackets() but
removed the old directional getters; to avoid a compatibility break, restore
getAllSendPackets() and getAllReceivePackets() as deprecated wrappers that
delegate to the new method (or to PACKETS filtered by direction) and annotate
them with `@Deprecated` and a Javadoc pointing to getAllPackets(). Update
PacketTypeRegistry by reintroducing the two methods (calling getAllPackets() or
filtering PACKETS) and mark them deprecated so downstream code still compiles
for one transition release.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: eb1f3186-e024-4a11-b115-441c5e06de2f
📒 Files selected for processing (5)
src/main/java/dev/threeadd/packeteventssk/api/general/PacketSendOrReceiveListener.javasrc/main/java/dev/threeadd/packeteventssk/api/general/PacketTypeRegistry.javasrc/main/java/dev/threeadd/packeteventssk/element/general/Types.javasrc/main/java/dev/threeadd/packeteventssk/element/general/effect/EffCancelPacket.javasrc/main/java/dev/threeadd/packeteventssk/element/general/expressions/prop/ExprPacketField.java
🚧 Files skipped from review as they are similar to previous changes (4)
- src/main/java/dev/threeadd/packeteventssk/element/general/expressions/prop/ExprPacketField.java
- src/main/java/dev/threeadd/packeteventssk/element/general/effect/EffCancelPacket.java
- src/main/java/dev/threeadd/packeteventssk/element/general/Types.java
- src/main/java/dev/threeadd/packeteventssk/api/general/PacketSendOrReceiveListener.java
Owner
Author
|
@CodeRabbit rewrite the description of this PR to be more specific. Make it say added: |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain
🏁 Script executed: Length of output: 516 Let me try writing the body to a file first: 🏁 Script executed: Length of output: 266
Dev v1.1.1Added
Changed
Fixed
|
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
Bug Fixes
Documentation
Chores