Conversation
…-usused-methods Remove unused CorePlugin methods getCore & getServerType
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughNew Features
Improvements
Development
WalkthroughThis PR moves player connection state into CorePlayer, updates listeners to read/write that flag, removes connection-tracking APIs and ServerType/CorePlugin methods, injects Bungee/Velocity loggers into platform listeners with stricter disconnect validation, removes related tests, and adds a Modrinth publish workflow. ChangesConnection State Refactoring and Platform Integration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
…dentity-logs Add disconnect identity check debug logs & get CorePlayer directly
…kflow Add workflow to publish GitHub release to Modrinth
…te-to-core-player Move connected state into CorePlayer class
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
.github/workflows/publish-modrinth.yml (2)
26-27: ⚡ Quick winConsider adding Gradle dependency caching.
Adding Gradle caching would significantly speed up builds by caching dependencies between workflow runs.
⚡ Proposed addition for Gradle caching
- name: Set up JDK 21 uses: actions/setup-java@v4 with: java-version: '21' distribution: 'temurin' + cache: 'gradle' - name: Build with Gradle run: ./gradlew build🤖 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 @.github/workflows/publish-modrinth.yml around lines 26 - 27, The workflow currently runs the step named "Build with Gradle" which executes "./gradlew build" without caching; add a cache step (using actions/cache@v4) before that step to cache Gradle artifacts (e.g., paths like ~/.gradle/caches and ~/.gradle/wrapper) and use a key that includes the OS and the Gradle wrapper checksum (or JAVA version plus wrapper checksum) so cached dependencies are restored on subsequent runs and invalidated when the wrapper changes; ensure the cache step is placed immediately before the "Build with Gradle" step and references the same working-directory/context as the build command.
16-16: 💤 Low valueConsider pinning actions to commit SHAs for enhanced security.
The static analysis tool flags that actions are not pinned to commit hashes. While version tags (like
@v4) are commonly used, pinning to specific commit SHAs provides better protection against supply chain attacks where a version tag could be moved to malicious code.🔒 Example of SHA pinning
steps: - - uses: actions/checkout@v4 + - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4 with: persist-credentials: false - name: Set up JDK 21 - uses: actions/setup-java@v4 + uses: actions/setup-java@8df1039502a15bceb9433410b1a100fbe190c53b # v4 with: java-version: '21' distribution: 'temurin' - name: Publish to Modrinth - uses: cloudnode-pro/modrinth-publish@v2 + uses: cloudnode-pro/modrinth-publish@<commit-sha> # v2Note: You'll need to look up the current commit SHAs for each action version.
Also applies to: 21-21, 30-30
🤖 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 @.github/workflows/publish-modrinth.yml at line 16, Replace loose action tags with pinned commit SHAs: find the action usages such as "uses: actions/checkout@v4" (and any other "uses: ...@<tag>" instances flagged) and replace the version tag with the exact commit SHA for that release (e.g., "uses: actions/checkout@<commit-sha>"); fetch the current commit SHA for each action release from the action's repository and update the workflow to use the SHA-pinned reference to improve supply-chain security.Source: Linters/SAST tools
src/main/java/xyz/earthcow/networkjoinmessages/common/abstraction/CorePlayer.java (1)
14-16: ⚡ Quick winConsider making
connectedanddisconnectingvolatile for thread visibility.Both fields are accessed across multiple threads—
connectedis checked in async tasks and disconnect handlers, whiledisconnectinguses a check-then-act pattern. Withoutvolatile, changes made by one thread may not be visible to others, leading to potential stale reads.🔒 Proposed fix for thread visibility
// Fields - private boolean connected = false; + private volatile boolean connected = false; private CoreBackendServer lastKnownConnectedServer; - private boolean disconnecting = false; + private volatile boolean disconnecting = false; private String cachedLeaveMessage;🤖 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/xyz/earthcow/networkjoinmessages/common/abstraction/CorePlayer.java` around lines 14 - 16, The fields connected and disconnecting in CorePlayer are accessed from multiple threads; make them thread-visible by declaring them volatile (change "private boolean connected" and "private boolean disconnecting" to volatile). For the check-then-act usage of disconnecting also consider using an AtomicBoolean (replace disconnecting with an AtomicBoolean field and use compareAndSet in places that test-and-set) to ensure atomicity; update all references to use either the volatile boolean or AtomicBoolean API (get/set/compareAndSet) consistently.
🤖 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.
Nitpick comments:
In @.github/workflows/publish-modrinth.yml:
- Around line 26-27: The workflow currently runs the step named "Build with
Gradle" which executes "./gradlew build" without caching; add a cache step
(using actions/cache@v4) before that step to cache Gradle artifacts (e.g., paths
like ~/.gradle/caches and ~/.gradle/wrapper) and use a key that includes the OS
and the Gradle wrapper checksum (or JAVA version plus wrapper checksum) so
cached dependencies are restored on subsequent runs and invalidated when the
wrapper changes; ensure the cache step is placed immediately before the "Build
with Gradle" step and references the same working-directory/context as the build
command.
- Line 16: Replace loose action tags with pinned commit SHAs: find the action
usages such as "uses: actions/checkout@v4" (and any other "uses: ...@<tag>"
instances flagged) and replace the version tag with the exact commit SHA for
that release (e.g., "uses: actions/checkout@<commit-sha>"); fetch the current
commit SHA for each action release from the action's repository and update the
workflow to use the SHA-pinned reference to improve supply-chain security.
In
`@src/main/java/xyz/earthcow/networkjoinmessages/common/abstraction/CorePlayer.java`:
- Around line 14-16: The fields connected and disconnecting in CorePlayer are
accessed from multiple threads; make them thread-visible by declaring them
volatile (change "private boolean connected" and "private boolean disconnecting"
to volatile). For the check-then-act usage of disconnecting also consider using
an AtomicBoolean (replace disconnecting with an AtomicBoolean field and use
compareAndSet in places that test-and-set) to ensure atomicity; update all
references to use either the volatile boolean or AtomicBoolean API
(get/set/compareAndSet) consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7c083dac-a357-4975-a40f-8cff0d1e6f7c
📒 Files selected for processing (11)
.github/workflows/publish-modrinth.ymlsrc/main/java/xyz/earthcow/networkjoinmessages/bungee/BungeeMain.javasrc/main/java/xyz/earthcow/networkjoinmessages/bungee/listeners/PlayerListener.javasrc/main/java/xyz/earthcow/networkjoinmessages/common/abstraction/CorePlayer.javasrc/main/java/xyz/earthcow/networkjoinmessages/common/abstraction/CorePlugin.javasrc/main/java/xyz/earthcow/networkjoinmessages/common/abstraction/ServerType.javasrc/main/java/xyz/earthcow/networkjoinmessages/common/listeners/CorePlayerListener.javasrc/main/java/xyz/earthcow/networkjoinmessages/common/player/PlayerStateStore.javasrc/main/java/xyz/earthcow/networkjoinmessages/velocity/VelocityMain.javasrc/main/java/xyz/earthcow/networkjoinmessages/velocity/listeners/PlayerListener.javasrc/test/java/xyz/earthcow/networkjoinmessages/common/player/PlayerStateStoreTest.java
💤 Files with no reviewable changes (4)
- src/main/java/xyz/earthcow/networkjoinmessages/common/abstraction/ServerType.java
- src/main/java/xyz/earthcow/networkjoinmessages/common/abstraction/CorePlugin.java
- src/main/java/xyz/earthcow/networkjoinmessages/common/player/PlayerStateStore.java
- src/test/java/xyz/earthcow/networkjoinmessages/common/player/PlayerStateStoreTest.java
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/xyz/earthcow/networkjoinmessages/common/abstraction/CorePlayer.java`:
- Around line 14-16: The disconnect dedupe is racy because CorePlayer uses a
volatile boolean 'disconnecting' checked by isDisconnecting() then set via
setDisconnecting(true); replace the boolean with an AtomicBoolean (e.g., private
final AtomicBoolean disconnecting) and change callers (like
CorePlayerListener.onDisconnect) to use disconnecting.compareAndSet(false, true)
(or provide a new method on CorePlayer, e.g., beginDisconnect() that returns
boolean) so only the thread that transitions false->true proceeds with cleanup;
update isDisconnecting()/setDisconnecting() semantics or remove setDisconnecting
to rely on the atomic compare-and-set.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 33a13074-1e85-4136-b269-d87687aaf966
📒 Files selected for processing (1)
src/main/java/xyz/earthcow/networkjoinmessages/common/abstraction/CorePlayer.java
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/main/java/xyz/earthcow/networkjoinmessages/common/abstraction/CorePlayer.java (1)
16-16: ⚡ Quick winCheck-then-set pattern on
connectedcould race under concurrent connects.The
connectedfield usesvolatile booleanwith the same check-then-set pattern (isConnected()→setConnected(true)) that existed with the olddisconnectingfield. If twoonServerConnectedevents fire concurrently for the same player, both could see!isConnected()as true and both runhandleJoin, causing duplicate join processing.Platform event systems typically serialize per-player events, so this may not happen in practice, but it's not guaranteed at the type level. If join duplication is a concern, consider using the same atomic pattern as
disconnecting:♻️ Optional atomic pattern for connected state
- private volatile boolean connected = false; + `@Getter`(AccessLevel.NONE) `@Setter`(AccessLevel.NONE) + private AtomicBoolean connected = new AtomicBoolean(false); + + public boolean markConnected() { + return connected.compareAndSet(false, true); + } + + public boolean isConnected() { + return connected.get(); + } + + public void setConnected(boolean value) { + connected.set(value); + }Then update
CorePlayerListener.handleJointo useif (player.markConnected()) { /* first connect wins */ }.🤖 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/xyz/earthcow/networkjoinmessages/common/abstraction/CorePlayer.java` at line 16, The connected field currently uses a volatile boolean with a check-then-set pattern (isConnected() → setConnected(true)) which can race; replace it with an AtomicBoolean (or equivalent) and add an atomic helper like markConnected() that does return connected.compareAndSet(false, true), update any getters/setters (isConnected()/setConnected) to use the AtomicBoolean, and change CorePlayerListener.handleJoin to call if (player.markConnected()) { /* first connect wins */ } so only the first concurrent connect executes handleJoin.
🤖 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.
Nitpick comments:
In
`@src/main/java/xyz/earthcow/networkjoinmessages/common/abstraction/CorePlayer.java`:
- Line 16: The connected field currently uses a volatile boolean with a
check-then-set pattern (isConnected() → setConnected(true)) which can race;
replace it with an AtomicBoolean (or equivalent) and add an atomic helper like
markConnected() that does return connected.compareAndSet(false, true), update
any getters/setters (isConnected()/setConnected) to use the AtomicBoolean, and
change CorePlayerListener.handleJoin to call if (player.markConnected()) { /*
first connect wins */ } so only the first concurrent connect executes
handleJoin.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d322dede-5d53-4755-8cdf-670b124e527b
📒 Files selected for processing (2)
src/main/java/xyz/earthcow/networkjoinmessages/common/abstraction/CorePlayer.javasrc/main/java/xyz/earthcow/networkjoinmessages/common/listeners/CorePlayerListener.java
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/xyz/earthcow/networkjoinmessages/common/listeners/CorePlayerListener.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
No description provided.