GH-1041 Refactor AdminChat command to support admin chat channel. #1063

mydelkonivea · 2025-07-21T19:48:09Z

Fixes: #1041

github-actions · 2025-07-21T19:48:19Z

📦 Development Build Ready

Warning

Do not use this build in production. It is for testing purposes only and may be unstable.

👉 Click here to download the JAR

coderabbitai

Actionable comments posted: 4

🧹 Nitpick comments (5)

eternalcore-core/src/main/java/com/eternalcode/core/feature/adminchat/AdminTask.java (3)
12-12: Consider reducing the task frequency

Running every second seems too frequent for a simple notification. Consider running every 10-30 seconds instead to reduce server load.
-@Task(period = 1, delay = 1, unit = TimeUnit.SECONDS)
+@Task(period = 10, delay = 1, unit = TimeUnit.SECONDS)
28-32: Add null safety after online check

The player retrieval looks good, but consider storing the player reference to avoid multiple lookups.
-        for (UUID enabledChatPlayerUUID : this.adminChatService.getAdminChatEnabledPlayers()) {
-
-            if (this.server.getPlayer(enabledChatPlayerUUID) == null) {
-                continue;
-            }
+        for (UUID enabledChatPlayerUUID : this.adminChatService.getAdminChatEnabledPlayers()) {
+            var player = this.server.getPlayer(enabledChatPlayerUUID);
+            if (player == null) {
+                continue;
+            }
12-12: Consider increasing the notification interval for better performance.

Running this task every second might be too frequent for spy notifications. Consider increasing to 30 seconds or 1 minute to reduce server load.
-@Task(period = 1, delay = 1, unit = TimeUnit.SECONDS)
+@Task(period = 30, delay = 1, unit = TimeUnit.SECONDS)
eternalcore-core/src/main/java/com/eternalcode/core/feature/adminchat/AdminChatServiceImpl.java (2)
34-36: Consider thread safety for concurrent access.

The HashSet isn't thread-safe, which could cause issues if multiple threads access the service simultaneously. Consider using ConcurrentHashMap.newKeySet() instead.
-private final Collection<UUID> persistentAdminChatPlayers = new HashSet<>();
+private final Collection<UUID> persistentAdminChatPlayers = ConcurrentHashMap.newKeySet();
12-36: Note: Spy settings won't persist across restarts.

The current implementation keeps spy settings in memory only. Consider if persistence is needed for better user experience.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f533904 and 892ed93.

📒 Files selected for processing (8)

eternalcore-api/src/main/java/com/eternalcode/core/feature/adminchat/event/AdminChatService.java (1 hunks)
eternalcore-core/src/main/java/com/eternalcode/core/feature/adminchat/AdminChatCommand.java (3 hunks)
eternalcore-core/src/main/java/com/eternalcode/core/feature/adminchat/AdminChatManagerController.java (1 hunks)
eternalcore-core/src/main/java/com/eternalcode/core/feature/adminchat/AdminChatServiceImpl.java (1 hunks)
eternalcore-core/src/main/java/com/eternalcode/core/feature/adminchat/AdminTask.java (1 hunks)
eternalcore-core/src/main/java/com/eternalcode/core/feature/adminchat/messages/AdminChatMessages.java (1 hunks)
eternalcore-core/src/main/java/com/eternalcode/core/feature/adminchat/messages/ENAdminChatMessages.java (1 hunks)
eternalcore-core/src/main/java/com/eternalcode/core/feature/adminchat/messages/PLAdminChatMessages.java (1 hunks)

🧰 Additional context used

🧠 Learnings (5)

eternalcore-core/src/main/java/com/eternalcode/core/feature/adminchat/messages/AdminChatMessages.java (1)

Learnt from: Rollczi
PR: #896
File: eternalcore-api/src/main/java/com/eternalcode/core/feature/privatechat/toggle/PrivateChatState.java:6-16
Timestamp: 2025-02-13T19:26:59.343Z
Learning: In the PrivateChatState enum, ENABLE means private messages are blocked (msgToggle is ON) and DISABLE means private messages are allowed (msgToggle is OFF). The enum values represent the state of the msgToggle feature itself, not the state of receiving messages.

eternalcore-core/src/main/java/com/eternalcode/core/feature/adminchat/messages/ENAdminChatMessages.java (2)

Learnt from: CitralFlo
PR: #896
File: eternalcore-core/src/main/java/com/eternalcode/core/feature/privatechat/toggle/PrivateChatToggleCommand.java:92-102
Timestamp: 2025-01-28T21:37:36.945Z
Learning: In the EternalCore plugin's private chat feature, PrivateChatToggleState.ON means message blocking is enabled (no messages will be received), while PrivateChatToggleState.OFF means messages are allowed.

Learnt from: CitralFlo
PR: #896
File: eternalcore-core/src/main/java/com/eternalcode/core/feature/privatechat/toggle/PrivateChatToggleCommand.java:1-1
Timestamp: 2025-01-30T19:49:57.192Z
Learning: In the EternalCore project's private chat toggle feature, PrivateChatToggleState.ON means messages are blocked, while OFF means messages are allowed.

eternalcore-core/src/main/java/com/eternalcode/core/feature/adminchat/messages/PLAdminChatMessages.java (2)

Learnt from: CitralFlo
PR: #896
File: eternalcore-core/src/main/java/com/eternalcode/core/feature/privatechat/toggle/PrivateChatToggleCommand.java:92-102
Timestamp: 2025-01-28T21:37:36.945Z
Learning: In the EternalCore plugin's private chat feature, PrivateChatToggleState.ON means message blocking is enabled (no messages will be received), while PrivateChatToggleState.OFF means messages are allowed.

Learnt from: CitralFlo
PR: #896
File: eternalcore-core/src/main/java/com/eternalcode/core/feature/privatechat/toggle/PrivateChatToggleCommand.java:1-1
Timestamp: 2025-01-30T19:49:57.192Z
Learning: In the EternalCore project's private chat toggle feature, PrivateChatToggleState.ON means messages are blocked, while OFF means messages are allowed.

eternalcore-core/src/main/java/com/eternalcode/core/feature/adminchat/AdminChatCommand.java (1)

Learnt from: CitralFlo
PR: #896
File: eternalcore-core/src/main/java/com/eternalcode/core/feature/privatechat/toggle/PrivateChatToggleCommand.java:92-102
Timestamp: 2025-01-28T21:37:36.945Z
Learning: In the EternalCore plugin's private chat feature, PrivateChatToggleState.ON means message blocking is enabled (no messages will be received), while PrivateChatToggleState.OFF means messages are allowed.

eternalcore-core/src/main/java/com/eternalcode/core/feature/adminchat/AdminChatManagerController.java (1)

Learnt from: CitralFlo
PR: #896
File: eternalcore-core/src/main/java/com/eternalcode/core/feature/privatechat/toggle/PrivateChatToggleCommand.java:92-102
Timestamp: 2025-01-28T21:37:36.945Z
Learning: In the EternalCore plugin's private chat feature, PrivateChatToggleState.ON means message blocking is enabled (no messages will be received), while PrivateChatToggleState.OFF means messages are allowed.

🔇 Additional comments (14)

eternalcore-core/src/main/java/com/eternalcode/core/feature/adminchat/messages/AdminChatMessages.java (1)

7-9: Clean interface additions for spy mode messaging

The new methods follow the existing pattern and provide clear messaging support for the admin chat spy feature.

eternalcore-core/src/main/java/com/eternalcode/core/feature/adminchat/AdminChatCommand.java (1)

47-54: Good spy mode toggle implementation

The service integration and conditional messaging work well together. The logic clearly toggles spy mode and provides appropriate feedback.

eternalcore-api/src/main/java/com/eternalcode/core/feature/adminchat/event/AdminChatService.java (1)

1-33: Well-designed service interface

Clean interface with clear method signatures and comprehensive documentation. The toggle method returning the new state is particularly well thought out.

eternalcore-core/src/main/java/com/eternalcode/core/feature/adminchat/AdminChatManagerController.java (1)

41-43: Good early return pattern

Clean check for spy mode with early return keeps the method readable.

eternalcore-core/src/main/java/com/eternalcode/core/feature/adminchat/messages/PLAdminChatMessages.java (1)

18-26: Good Polish translations for spy mode

The messages are clear and follow the existing styling pattern. Using actionbar for the notification reminder is a nice touch.

eternalcore-core/src/main/java/com/eternalcode/core/feature/adminchat/messages/ENAdminChatMessages.java (3)

17-25: Clean message implementation

The new spy-related messages are well-designed with clear wording and consistent formatting. Good choice using action bar for ongoing status notifications.

17-21: Well-designed spy toggle messages.

The enable/disable messages are clear and use consistent styling with the green arrow prefix.

23-24: Perfect choice for action bar notification.

Using an action bar notice for the spy status is ideal - it's visible but not intrusive for ongoing notifications.

eternalcore-core/src/main/java/com/eternalcode/core/feature/adminchat/AdminChatServiceImpl.java (4)

18-26: Clean toggle implementation

The toggle logic is well-designed and returns the new state clearly. Good approach for boolean toggle operations.

34-36: Good defensive programming

Returning an unmodifiable collection prevents external modification. Well done!

12-12: Good choice using HashSet for UUID storage.

HashSet provides fast lookups and automatically handles duplicates for the spy player collection.

18-26: Clean toggle logic with helpful return value.

The toggle method correctly adds/removes players and returns the new state, making it easy for callers to show appropriate messages.

eternalcore-core/src/main/java/com/eternalcode/core/feature/adminchat/AdminTask.java (2)

30-32: Good null check for offline players.

The null check prevents errors when players go offline between getting the UUID list and checking their online status.

34-37: Clean notification implementation.

The notice creation and sending logic is well-structured and properly uses the translation system.

eternalcore-core/src/main/java/com/eternalcode/core/feature/adminchat/AdminChatCommand.java

...re-core/src/main/java/com/eternalcode/core/feature/adminchat/AdminChatManagerController.java

eternalcore-core/src/main/java/com/eternalcode/core/feature/adminchat/AdminChatServiceImpl.java

eternalcore-core/src/main/java/com/eternalcode/core/feature/adminchat/AdminTask.java

P1otrulla

Good job! 😎

...re-core/src/main/java/com/eternalcode/core/feature/adminchat/AdminChatManagerController.java

eternalcore-core/src/main/java/com/eternalcode/core/feature/adminchat/AdminTask.java

…minchat/AdminTask.java Co-authored-by: Piotr Zych <[email protected]>

coderabbitai

Actionable comments posted: 0

♻️ Duplicate comments (1)

eternalcore-core/src/main/java/com/eternalcode/core/feature/adminchat/AdminChatServiceImpl.java (1)

18-18: Thread safety issue already identified

This was already flagged in a previous review - HashSet needs to be replaced with a thread-safe collection.

🧹 Nitpick comments (2)

eternalcore-core/src/main/java/com/eternalcode/core/feature/adminchat/AdminChatPersistentController.java (1)
32-32: Small formatting fix needed

Remove the extra space before the parenthesis.
-    void onAdminChat (AsyncPlayerChatEvent event) {
+    void onAdminChat(AsyncPlayerChatEvent event) {
eternalcore-core/src/main/java/com/eternalcode/core/feature/adminchat/AdminChatServiceImpl.java (1)
27-35: Simplify the toggle method

You can make this cleaner by using the return value of remove().
 @Override
 public boolean toggleChatPersistent(UUID playerUuid) {
-    if (this.persistentAdminChatPlayers.contains(playerUuid)) {
-        this.persistentAdminChatPlayers.remove(playerUuid);
-        return false;
-    } else {
-        this.persistentAdminChatPlayers.add(playerUuid);
-        return true;
-    }
+    if (this.persistentAdminChatPlayers.remove(playerUuid)) {
+        return false;
+    }
+    this.persistentAdminChatPlayers.add(playerUuid);
+    return true;
 }

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c7ad18b and 64f5817.

📒 Files selected for processing (9)

eternalcore-api/src/main/java/com/eternalcode/core/feature/adminchat/event/AdminChatService.java (1 hunks)
eternalcore-core/src/main/java/com/eternalcode/core/feature/adminchat/AdminChatCommand.java (3 hunks)
eternalcore-core/src/main/java/com/eternalcode/core/feature/adminchat/AdminChatPermissionConstant.java (1 hunks)
eternalcore-core/src/main/java/com/eternalcode/core/feature/adminchat/AdminChatPersistentController.java (1 hunks)
eternalcore-core/src/main/java/com/eternalcode/core/feature/adminchat/AdminChatServiceImpl.java (1 hunks)
eternalcore-core/src/main/java/com/eternalcode/core/feature/adminchat/AdminTask.java (1 hunks)
eternalcore-core/src/main/java/com/eternalcode/core/feature/adminchat/messages/AdminChatMessages.java (1 hunks)
eternalcore-core/src/main/java/com/eternalcode/core/feature/adminchat/messages/ENAdminChatMessages.java (1 hunks)
eternalcore-core/src/main/java/com/eternalcode/core/feature/adminchat/messages/PLAdminChatMessages.java (1 hunks)

🧠 Learnings (4)

📓 Common learnings

Learnt from: CitralFlo
PR: EternalCodeTeam/EternalCore#896
File: eternalcore-core/src/main/java/com/eternalcode/core/feature/privatechat/toggle/PrivateChatToggleCommand.java:92-102
Timestamp: 2025-01-28T21:37:36.945Z
Learning: In the EternalCore plugin's private chat feature, PrivateChatToggleState.ON means message blocking is enabled (no messages will be received), while PrivateChatToggleState.OFF means messages are allowed.

Learnt from: CitralFlo
PR: EternalCodeTeam/EternalCore#896
File: eternalcore-core/src/main/java/com/eternalcode/core/feature/privatechat/toggle/PrivateChatToggleCommand.java:1-1
Timestamp: 2025-01-30T19:49:57.192Z
Learning: In the EternalCore project's private chat toggle feature, PrivateChatToggleState.ON means messages are blocked, while OFF means messages are allowed.

Learnt from: Rollczi
PR: EternalCodeTeam/EternalCore#896
File: eternalcore-api/src/main/java/com/eternalcode/core/feature/privatechat/toggle/PrivateChatState.java:6-16
Timestamp: 2025-02-13T19:26:59.343Z
Learning: In the PrivateChatState enum, ENABLE means private messages are blocked (msgToggle is ON) and DISABLE means private messages are allowed (msgToggle is OFF). The enum values represent the state of the msgToggle feature itself, not the state of receiving messages.

eternalcore-core/src/main/java/com/eternalcode/core/feature/adminchat/AdminChatPermissionConstant.java (2)

Learnt from: CitralFlo
PR: #896
File: eternalcore-core/src/main/java/com/eternalcode/core/feature/privatechat/toggle/PrivateChatToggleCommand.java:92-102
Timestamp: 2025-01-28T21:37:36.945Z
Learning: In the EternalCore plugin's private chat feature, PrivateChatToggleState.ON means message blocking is enabled (no messages will be received), while PrivateChatToggleState.OFF means messages are allowed.

Learnt from: CitralFlo
PR: #896
File: eternalcore-core/src/main/java/com/eternalcode/core/feature/privatechat/toggle/PrivateChatToggleCommand.java:1-1
Timestamp: 2025-01-30T19:49:57.192Z
Learning: In the EternalCore project's private chat toggle feature, PrivateChatToggleState.ON means messages are blocked, while OFF means messages are allowed.

eternalcore-core/src/main/java/com/eternalcode/core/feature/adminchat/AdminChatPersistentController.java (1)

Learnt from: CitralFlo
PR: #896
File: eternalcore-core/src/main/java/com/eternalcode/core/feature/privatechat/toggle/PrivateChatToggleCommand.java:92-102
Timestamp: 2025-01-28T21:37:36.945Z
Learning: In the EternalCore plugin's private chat feature, PrivateChatToggleState.ON means message blocking is enabled (no messages will be received), while PrivateChatToggleState.OFF means messages are allowed.

eternalcore-core/src/main/java/com/eternalcode/core/feature/adminchat/AdminChatServiceImpl.java (5)

Learnt from: CitralFlo
PR: #896
File: eternalcore-core/src/main/java/com/eternalcode/core/feature/privatechat/toggle/PrivateChatToggleCommand.java:92-102
Timestamp: 2025-01-28T21:37:36.945Z
Learning: In the EternalCore plugin's private chat feature, PrivateChatToggleState.ON means message blocking is enabled (no messages will be received), while PrivateChatToggleState.OFF means messages are allowed.

Learnt from: CitralFlo
PR: #896
File: eternalcore-core/src/main/java/com/eternalcode/core/feature/privatechat/toggle/PrivateChatToggleCommand.java:1-1
Timestamp: 2025-01-30T19:49:57.192Z
Learning: In the EternalCore project's private chat toggle feature, PrivateChatToggleState.ON means messages are blocked, while OFF means messages are allowed.

Learnt from: vLuckyyy
PR: #856
File: eternalcore-core/src/main/java/com/eternalcode/core/feature/warp/data/WarpDataConfig.java:19-20
Timestamp: 2025-01-12T13:03:07.239Z
Learning: Configuration classes in EternalCore (those implementing ReloadableConfig and annotated with @ConfigurationFile) don't need thread-safe collections as they are managed by the configuration system which handles synchronization.

Learnt from: vLuckyyy
PR: #865
File: eternalcore-core/src/main/java/com/eternalcode/core/feature/essentials/item/give/GiveService.java:0-0
Timestamp: 2024-10-14T20:11:13.610Z
Learning: In the 'EternalCore' project, avoid using null in method signatures in Java code.

Learnt from: CitralFlo
PR: #896
File: eternalcore-core/src/main/java/com/eternalcode/core/feature/privatechat/toggle/PrivateChatToggleServiceImpl.java:24-27
Timestamp: 2025-01-28T21:35:47.750Z
Learning: In EternalCore, async operations using CompletableFuture should remain non-blocking. Avoid using .join() or similar blocking operations to maintain better performance and scalability.

🚧 Files skipped from review as they are similar to previous changes (6)

eternalcore-api/src/main/java/com/eternalcode/core/feature/adminchat/event/AdminChatService.java
eternalcore-core/src/main/java/com/eternalcode/core/feature/adminchat/AdminChatCommand.java
eternalcore-core/src/main/java/com/eternalcode/core/feature/adminchat/messages/AdminChatMessages.java
eternalcore-core/src/main/java/com/eternalcode/core/feature/adminchat/messages/ENAdminChatMessages.java
eternalcore-core/src/main/java/com/eternalcode/core/feature/adminchat/messages/PLAdminChatMessages.java
eternalcore-core/src/main/java/com/eternalcode/core/feature/adminchat/AdminTask.java

🧰 Additional context used

🧠 Learnings (4)

📓 Common learnings

Learnt from: CitralFlo
PR: EternalCodeTeam/EternalCore#896
File: eternalcore-core/src/main/java/com/eternalcode/core/feature/privatechat/toggle/PrivateChatToggleCommand.java:92-102
Timestamp: 2025-01-28T21:37:36.945Z
Learning: In the EternalCore plugin's private chat feature, PrivateChatToggleState.ON means message blocking is enabled (no messages will be received), while PrivateChatToggleState.OFF means messages are allowed.

Learnt from: CitralFlo
PR: EternalCodeTeam/EternalCore#896
File: eternalcore-core/src/main/java/com/eternalcode/core/feature/privatechat/toggle/PrivateChatToggleCommand.java:1-1
Timestamp: 2025-01-30T19:49:57.192Z
Learning: In the EternalCore project's private chat toggle feature, PrivateChatToggleState.ON means messages are blocked, while OFF means messages are allowed.

Learnt from: Rollczi
PR: EternalCodeTeam/EternalCore#896
File: eternalcore-api/src/main/java/com/eternalcode/core/feature/privatechat/toggle/PrivateChatState.java:6-16
Timestamp: 2025-02-13T19:26:59.343Z
Learning: In the PrivateChatState enum, ENABLE means private messages are blocked (msgToggle is ON) and DISABLE means private messages are allowed (msgToggle is OFF). The enum values represent the state of the msgToggle feature itself, not the state of receiving messages.