GH-911 Fix typo in afk config, fix permission check order, add constant for permission for afk permission. #911
Conversation
WalkthroughThe pull request updates the AFK functionality across several components. A new constant for bypass permission is introduced in the command handler, and the logic is restructured to check this permission earlier, thus bypassing any delay if applicable. In addition, a typo in the method description has been corrected. A class responsible for placeholder setup has undergone a renaming to fix a spelling error in both its class name and constructor. Minor adjustments to import statements in some files have been made without affecting their overall functionality. Lastly, a new message field is added to signal when a player is no longer AFK. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
vLuckyyy
changed the title
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportServiceImpl.java (1)
94-98: Consider precision in coordinate calculations.The integer casting might cause slight precision loss at the borders. While this is usually fine for Minecraft coordinates, you might want to round instead of truncating.
- int minX = (int) (centerX - size/2); - int maxX = (int) (centerX + size/2); - int minZ = (int) (centerZ - size/2); - int maxZ = (int) (centerZ + size/2); + int minX = (int) Math.round(centerX - size/2); + int maxX = (int) Math.round(centerX + size/2); + int minZ = (int) Math.round(centerZ - size/2); + int maxZ = (int) Math.round(centerZ + size/2);eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportSafeLocationService.java (1)
48-49: Consider improving coordinate distribution around spawn.Based on previous feedback, the random teleport should generate coordinates in both positive and negative directions from spawn for better balance.
Here's a simple way to achieve this:
- int randomX = spawnX + this.random.nextInt(radius.maxX() - radius.minX() + 1) + radius.minX(); - int randomZ = spawnZ + this.random.nextInt(radius.maxZ() - radius.minZ() + 1) + radius.minZ(); + int randomX = spawnX + (this.random.nextBoolean() ? 1 : -1) * this.random.nextInt(radius.maxX() - radius.minX() + 1); + int randomZ = spawnZ + (this.random.nextBoolean() ? 1 : -1) * this.random.nextInt(radius.maxZ() - radius.minZ() + 1);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
eternalcore-core/src/main/java/com/eternalcode/core/feature/afk/AfkCommand.java(2 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/afk/AfkPlaceholderSetup.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/afk/AfkServiceImpl.java(2 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/afk/AfkTask.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/afk/messages/PLAfkMessages.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportSafeLocationService.java(2 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportServiceImpl.java(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- eternalcore-core/src/main/java/com/eternalcode/core/feature/afk/AfkTask.java
- eternalcore-core/src/main/java/com/eternalcode/core/feature/afk/AfkPlaceholderSetup.java
🧰 Additional context used
🧠 Learnings (1)
eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportSafeLocationService.java (2)
Learnt from: Jakubk15
PR: EternalCodeTeam/EternalCore#897
File: eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportSafeLocationService.java:53-54
Timestamp: 2025-01-24T09:16:19.806Z
Learning: The random teleport feature in EternalCore should generate coordinates in both positive and negative directions from spawn point to ensure balanced distribution of teleport locations.
Learnt from: Jakubk15
PR: EternalCodeTeam/EternalCore#897
File: eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportSafeLocationService.java:53-54
Timestamp: 2025-01-24T09:28:00.144Z
Learning: In Java random teleport implementations, Math.random() < 0.5 is used to achieve a 50/50 chance between positive and negative directions, providing a simple and efficient way to ensure equal distribution around a center point.
🔇 Additional comments (10)
eternalcore-core/src/main/java/com/eternalcode/core/feature/afk/messages/PLAfkMessages.java (2)
16-18: Nice addition of the AFK status message!The new message for when players return from AFK matches well with the existing AFK announcement style.
24-24: Clean formatting adjustments!The consistent spacing and formatting make the code easier to read.
Also applies to: 29-29
eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportServiceImpl.java (1)
90-93: LGTM! Nice improvement to world border handling.The code now properly accounts for world borders with different center points, making it more accurate for all server configurations.
eternalcore-core/src/main/java/com/eternalcode/core/feature/afk/AfkCommand.java (3)
27-28: Nice work extracting the permission to a constant!This makes the code more maintainable and helps prevent typos.
43-43: Description reads much better now!The updated description clearly explains the AFK delay bypass behavior.
47-50: Great improvement in code flow!Moving the bypass check to the start and using early return makes the code easier to follow.
eternalcore-core/src/main/java/com/eternalcode/core/feature/randomteleport/RandomTeleportSafeLocationService.java (2)
39-41: Nice improvement to the attempt count check!The condition now correctly handles the case when attempts reach zero, making the code more logical.
69-80: Great job extracting spawn location logic!The new method cleanly handles both configured and default spawn locations, making the code easier to understand and maintain.
eternalcore-core/src/main/java/com/eternalcode/core/feature/afk/AfkServiceImpl.java (2)
15-15: LGTM! Good addition of concurrent utilities.The import of
ConcurrentHashMapaligns well with the thread-safe implementation.
27-29:✅ Verification successful
Great improvement to thread safety!
Switching to
ConcurrentHashMapis a smart choice here. It makes the code safer when multiple threads are accessing the AFK state, interaction counts, and timestamps.However, let's verify that we're using these maps correctly throughout the codebase:
🏁 Script executed:
#!/bin/bash # Search for any direct synchronization on these maps that might now be redundant rg -A 2 "synchronized.*afkByPlayer|synchronized.*interactionsCount|synchronized.*lastInteraction" # Look for any bulk operations that might need special handling with ConcurrentHashMap ast-grep --pattern 'putAll($_, $_) | clear() | containsAll($_, $_)'Length of output: 169
Below is an additional verification step to ensure that no hidden synchronized blocks or wrapping using
Collections.synchronizedMapare still interacting with these maps. Please run these extra commands and share the results before we finalize the review comment:Once you've run these commands and confirmed that there are no unexpected synchronizations or bulk operations involving these maps, we can update the review comment accordingly.
🏁 Script executed:
[run_scripts] #!/bin/bash # Look for any synchronized blocks involving our map variables across the codebase rg -n "synchronized\s*\(.*(afkByPlayer|interactionsCount|lastInteraction)" . # Also check for any usage of Collections.synchronizedMap around these variables rg -n "Collections\.synchronizedMap" .Length of output: 241
Thread Safety Verified: Good Job!
I've double-checked the codebase for any leftover synchronization blocks or bulk operations involving the AFK maps. The search commands returned no unwanted references, confirming that the shift to
ConcurrentHashMapis correctly implemented and no redundant synchronization exists.No further changes are needed.
vLuckyyy
changed the title
eternalcore-core/src/main/java/com/eternalcode/core/feature/afk/AfkServiceImpl.java
Outdated
Show resolved
Hide resolved
No description provided.