-
-
Notifications
You must be signed in to change notification settings - Fork 17
GH-976 Fix chat clear logic to directly send empty messages. #1047
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughThe update increases the number of chat lines cleared by the Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes were found. ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
📦 Development Build ReadyWarning Do not use this build in production. It is for testing purposes only and may be unstable. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (1)
eternalcore-core/src/main/java/com/eternalcode/core/configuration/implementation/PluginConfiguration.java (1)
196-196
: Consider the performance impact of doubling the clear count.Increasing from 128 to 256 lines means each player will receive 256 individual messages during chat clear. This could create network overhead, especially with many online players.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
eternalcore-core/src/main/java/com/eternalcode/core/configuration/implementation/PluginConfiguration.java
(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/chat/ChatCommand.java
(2 hunks)
👮 Files not reviewed due to content moderation or server errors (2)
- eternalcore-core/src/main/java/com/eternalcode/core/configuration/implementation/PluginConfiguration.java
- eternalcore-core/src/main/java/com/eternalcode/core/feature/chat/ChatCommand.java
🧰 Additional context used
🧠 Learnings (1)
eternalcore-core/src/main/java/com/eternalcode/core/configuration/implementation/PluginConfiguration.java (1)
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.
🔇 Additional comments (12)
eternalcore-core/src/main/java/com/eternalcode/core/configuration/implementation/PluginConfiguration.java (2)
196-196
: Configuration update looks good!Doubling the clear count from 128 to 256 makes sense with the new direct message approach.
196-196
: Configuration update looks good!Doubling the lines to clear makes sense with the new empty message approach.
eternalcore-core/src/main/java/com/eternalcode/core/feature/chat/ChatCommand.java (10)
24-24
: Clean import and constant additions!The Server import and EMPTY_CHAT_STRING constant are well-implemented. Using a single space is smart to ensure messages are properly sent.
Also applies to: 31-31
36-36
: Constructor updates follow good patterns!Adding the Server dependency is the right approach for accessing online players. The injection pattern is consistent with existing code.
Also applies to: 46-46, 53-53
69-73
: New clear implementation works as intended!The logic correctly sends empty messages to all players. Just be aware this generates more network traffic than the previous approach (256 messages per player vs. one notice), but it achieves the desired direct messaging goal.
31-31
: Good choice for the empty message content.Using a single space ensures the message isn't completely empty while remaining minimally visible to players.
36-36
: Proper dependency injection for accessing online players.Adding the Server field is the right approach for the new chat clearing implementation.
46-46
: Constructor properly updated for dependency injection.The Server parameter addition follows good DI practices.
53-53
: Standard field initialization.Proper assignment of the injected dependency.
24-24
: Good additions for the new approach!The Server import and empty chat constant are clean implementations for the direct message strategy.
Also applies to: 31-31
36-36
: Constructor changes are properly implemented!Server dependency is correctly injected and stored following good practices.
Also applies to: 46-46, 53-53
69-73
: Clean and effective implementation!The new direct message approach is straightforward and correctly sends empty messages to all online players. Much simpler than the previous newline-based method.
eternalcore-core/src/main/java/com/eternalcode/core/feature/chat/ChatCommand.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good
Fixes: #976