GH-766 Add multiple notifications at once. #873
Conversation
mydelkonivea
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (8)
eternalcore-core/src/main/java/com/eternalcode/core/translation/Translation.java (2)
9-9: Remove unused import.The import
org.checkerframework.checker.units.qual.Nappears to be unused in the code. Consider removing it to maintain clean imports.-import org.checkerframework.checker.units.qual.N;
138-142: LGTM! Consider adding method documentation.The new alert queue methods are well-structured and follow a consistent pattern. However, adding Javadoc comments would improve maintainability by documenting:
- The purpose of each method
- The specific notification scenario each method handles
- Any relevant return value details
Example documentation format:
/** * Returns a notification for when an alert is successfully added to the queue. * * @return Notice the notification to display */ Notice alertQueueAdded();eternalcore-core/src/main/java/com/eternalcode/core/translation/implementation/PLTranslation.java (1)
303-307: LGTM! The alert queue messages are well-implemented.The new alert queue messages follow consistent formatting and provide clear feedback for queue operations. They integrate well with the existing chat section messages.
Consider adding a description annotation for these messages, similar to other messages in the class:
+ @Description({" ", "# Messages for alert queue management"}) public Notice alertQueueAdded = Notice.chat("<green>► <white>Dodano wiadomość do kolejki!"); public Notice alertQueueRemoved = Notice.chat("<green>► <white>Usunięto wiadomość z kolejki!"); public Notice alertQueueCleared = Notice.chat("<green>► <white>Wyczyszczono kolejkę wiadomości!"); public Notice alertQueueEmpty = Notice.chat("<red>✘ <dark_red>Błąd: <red>Kolejka wiadomości jest pusta!"); public Notice alertQueueSent = Notice.chat("<green>► <white>Wysłano wszystkie wiadomości z kolejki!");eternalcore-core/src/main/java/com/eternalcode/core/feature/essentials/alert/AlertManager.java (2)
19-19: Consider makingAlertManagerclass public for accessibilityThe
AlertManagerclass is currently package-private, which may limit its usability outside of its package. Since it's annotated with@Service, it might be intended for use throughout the application. Consider declaring the class aspublic.Apply this diff to make the class public:
- class AlertManager { + public class AlertManager {
59-59: Consider making the broadcast delay configurableThe delay between broadcasts is hardcoded as
i * 2Lseconds. Consider making this delay configurable to provide flexibility in scheduling broadcasts.eternalcore-core/src/main/java/com/eternalcode/core/feature/essentials/alert/AlertCommand.java (3)
20-20: RenamealertServicetoalertManagerfor consistencyThe variable
alertServiceis of typeAlertManager. Renaming it toalertManagerimproves clarity and maintains consistency with naming conventions.Apply this diff to rename the variable:
- private final AlertManager alertService; + private final AlertManager alertManager;AlertCommand( NoticeService noticeService, - AlertManager alertService + AlertManager alertManager ) { this.noticeService = noticeService; - this.alertService = alertService; + this.alertManager = alertManager; }Also applies to: 25-25, 28-28
58-65: Add a default case to the switch statementIncluding a default case in the switch statement ensures all possible enum values are handled, which improves code robustness and maintainability.
Apply this diff to add a default case:
switch (param) { case all: success = this.alertService.removeBroadcastWithType(type); break; case latest: success = this.alertService.removeLatestBroadcastWithType(type); break; + default: + throw new IllegalArgumentException("Unknown RemoveParam: " + param); }
31-97: Consider adding unit tests for the command methodsAdding unit tests for the command methods ensures they function correctly and helps prevent future regressions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
eternalcore-core/src/main/java/com/eternalcode/core/feature/essentials/AlertCommand.java(0 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/essentials/alert/AlertCommand.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/essentials/alert/AlertManager.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/essentials/alert/RemoveParam.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/translation/Translation.java(2 hunks)eternalcore-core/src/main/java/com/eternalcode/core/translation/implementation/ENTranslation.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/translation/implementation/PLTranslation.java(1 hunks)
💤 Files with no reviewable changes (1)
- eternalcore-core/src/main/java/com/eternalcode/core/feature/essentials/AlertCommand.java
✅ Files skipped from review due to trivial changes (1)
- eternalcore-core/src/main/java/com/eternalcode/core/feature/essentials/alert/RemoveParam.java
🔇 Additional comments (8)
eternalcore-core/src/main/java/com/eternalcode/core/translation/implementation/ENTranslation.java (1)
300-304: LGTM! Alert queue messages are well-structured and consistent.
The new alert queue messages follow the established formatting patterns and provide clear feedback for all queue operations:
- Success messages use the standard green arrow prefix
- Error messages use the standard red X prefix
- Messages are concise and user-friendly
eternalcore-core/src/main/java/com/eternalcode/core/feature/essentials/alert/AlertCommand.java (7)
15-17: Command class is properly defined with correct annotations
The class AlertCommand is correctly annotated with the command name and permissions.
19-29: Constructor and dependency injection are properly implemented
The constructor correctly injects dependencies for NoticeService and AlertManager.
31-39: execute method implementation is correct
The method correctly sends an alert to all players with the specified notice type and message.
41-52: executeAdd method correctly adds an alert to the queue
The method properly adds an alert to the queue and notifies the player.
54-77: executeRemove method handles alert removal properly
The method correctly removes alerts based on the provided parameter and provides appropriate feedback to the player.
79-87: executeClear method correctly clears all alerts
The method successfully clears all alerts from the queue and notifies the player.
89-97: executeSend method correctly sends all alerts
The method properly sends all alerts from the queue and informs the player who executed the command.
eternalcore-core/src/main/java/com/eternalcode/core/feature/essentials/alert/AlertManager.java
Outdated
Show resolved
Hide resolved
eternalcore-core/src/main/java/com/eternalcode/core/feature/essentials/alert/AlertManager.java
Outdated
Show resolved
Hide resolved
eternalcore-core/src/main/java/com/eternalcode/core/feature/essentials/alert/AlertManager.java
Outdated
Show resolved
Hide resolved
eternalcore-core/src/main/java/com/eternalcode/core/feature/essentials/alert/AlertManager.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
eternalcore-core/src/main/java/com/eternalcode/core/feature/essentials/alert/AlertCommand.java (1)
45-45: Standardize the usage of@Senderand@Contextannotations for thePlayerparameter.Currently, there is an inconsistency in the annotations used for the
Playerparameter across the command methods:
executeAdduses@Context Player player(line 45)executeRemoveAlluses@Sender Player sender(line 58)executeRemoveuses@Sender Player sender(line 69)executeClearuses@Context Player player(line 80)executeSenduses@Context Player player(line 90)For consistency and maintainability, consider standardizing the annotation. Preferably, use
@Senderif you want to enforce that only aPlayercan execute the command, or@Contextif additional processing is needed.Also applies to: 58-58, 69-69, 80-80, 90-90
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
buildSrc/src/main/kotlin/Versions.kt(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/essentials/alert/AlertCommand.java(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- buildSrc/src/main/kotlin/Versions.kt
eternalcore-core/src/main/java/com/eternalcode/core/feature/essentials/alert/AlertCommand.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (1)
eternalcore-core/src/main/java/com/eternalcode/core/feature/essentials/alert/AlertManager.java (1)
19-19: Remove commented out code.The commented implementation should be removed as it's no longer needed and adds unnecessary noise to the codebase.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
eternalcore-core/src/main/java/com/eternalcode/core/feature/essentials/alert/AlertCommand.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/essentials/alert/AlertManager.java(1 hunks)
🔇 Additional comments (3)
eternalcore-core/src/main/java/com/eternalcode/core/feature/essentials/alert/AlertManager.java (1)
23-26: LGTM!
The constructor is properly annotated with @Inject for dependency injection.
eternalcore-core/src/main/java/com/eternalcode/core/feature/essentials/alert/AlertCommand.java (2)
58-79: Ensure command overloading does not cause ambiguity in command execution.
Both executeRemoveAll and executeRemove methods are annotated with @Execute(name = "remove"), differentiated by the literals "all" and "latest". Verify that the command parsing framework (litecommands) can correctly distinguish between these methods based on the arguments, and that there is no ambiguity or unexpected behavior when players execute the remove command.
92-98: Handle empty alert queue in executeSend() method.
As previously noted, the executeSend() method sends all alerts from the queue without checking if the queue is empty. This may lead to confusion if there are no alerts to send, yet the player receives a confirmation message. Consider adding a check to inform the player when the alert queue is empty.
eternalcore-core/src/main/java/com/eternalcode/core/feature/essentials/alert/AlertManager.java
Show resolved
Hide resolved
eternalcore-core/src/main/java/com/eternalcode/core/feature/essentials/alert/AlertManager.java
Outdated
Show resolved
Hide resolved
eternalcore-core/src/main/java/com/eternalcode/core/feature/essentials/alert/AlertManager.java
Outdated
Show resolved
Hide resolved
eternalcore-core/src/main/java/com/eternalcode/core/feature/essentials/alert/AlertManager.java
Outdated
Show resolved
Hide resolved
eternalcore-core/src/main/java/com/eternalcode/core/feature/essentials/alert/AlertCommand.java
Outdated
Show resolved
Hide resolved
eternalcore-core/src/main/java/com/eternalcode/core/feature/essentials/alert/AlertCommand.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
eternalcore-core/src/main/java/com/eternalcode/core/feature/essentials/alert/AlertManager.java (2)
18-19: Add class-level documentationAdd Javadoc explaining the purpose of this service, its responsibilities, and thread-safety guarantees.
@Service +/** + * Manages queued alerts for players, allowing multiple notifications to be sent sequentially. + * This service is thread-safe and handles concurrent access from multiple threads. + */ class AlertManager {
78-78: Add documentation for AlertKey recordAdd Javadoc explaining the purpose and usage of this record.
+ /** + * Represents a unique key for identifying broadcasts by UUID and notification type. + * + * @param uuid The UUID of the target player + * @param type The type of notification + */ private record AlertKey(UUID uuid, NoticeTextType type) {}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
eternalcore-core/src/main/java/com/eternalcode/core/feature/essentials/alert/AlertManager.java(1 hunks)
🔇 Additional comments (3)
eternalcore-core/src/main/java/com/eternalcode/core/feature/essentials/alert/AlertManager.java (3)
21-21:
Critical: Use ConcurrentHashMap for thread safety
The broadcasts map should be thread-safe since it's accessed from multiple threads (main thread and scheduler thread).
- private final Map<AlertKey, List<EternalCoreBroadcastImpl<Viewer, Translation, ?>>> broadcasts = new HashMap<>();
+ private final Map<AlertKey, List<EternalCoreBroadcastImpl<Viewer, Translation, ?>>> broadcasts = new ConcurrentHashMap<>();29-31:
Use thread-safe list implementation
The ArrayList isn't thread-safe, which could lead to ConcurrentModificationException.
- this.broadcasts.computeIfAbsent(new AlertKey(uuid, type), k -> new ArrayList<>()).add(broadcast);
+ this.broadcasts.computeIfAbsent(new AlertKey(uuid, type), k -> Collections.synchronizedList(new ArrayList<>())).add(broadcast);33-39: 🛠️ Refactor suggestion
Optimize AlertKey creation and fix return value inconsistency
The methods create multiple AlertKey objects unnecessarily and removeLatestBroadcastWithType has inconsistent return values.
boolean removeBroadcastWithType(UUID uuid, NoticeTextType type) {
+ AlertKey key = new AlertKey(uuid, type);
- if (this.broadcasts.containsKey(new AlertKey(uuid, type))) {
- this.broadcasts.remove(new AlertKey(uuid, type));
+ if (this.broadcasts.containsKey(key)) {
+ this.broadcasts.remove(key);
return true;
}
return false;
}
boolean removeLatestBroadcastWithType(UUID uuid, NoticeTextType type) {
- List<EternalCoreBroadcastImpl<Viewer, Translation, ?>> broadcastList = this.broadcasts.get(new AlertKey(uuid, type));
+ AlertKey key = new AlertKey(uuid, type);
+ List<EternalCoreBroadcastImpl<Viewer, Translation, ?>> broadcastList = this.broadcasts.get(key);
if (broadcastList != null && !broadcastList.isEmpty()) {
broadcastList.remove(broadcastList.size() - 1);
if (broadcastList.isEmpty()) {
- return this.removeBroadcastWithType(uuid, type);
+ this.removeBroadcastWithType(uuid, type);
}
+ return true;
}
return false;
}Also applies to: 41-51
eternalcore-core/src/main/java/com/eternalcode/core/translation/Translation.java
Show resolved
Hide resolved
eternalcore-core/src/main/java/com/eternalcode/core/feature/essentials/alert/AlertCommand.java
Show resolved
Hide resolved
eternalcore-core/src/main/java/com/eternalcode/core/feature/essentials/alert/AlertManager.java
Outdated
Show resolved
Hide resolved
eternalcore-core/src/main/java/com/eternalcode/core/feature/essentials/alert/AlertManager.java
Show resolved
Hide resolved
eternalcore-core/src/main/java/com/eternalcode/core/feature/essentials/alert/AlertManager.java
Outdated
Show resolved
Hide resolved
eternalcore-core/src/main/java/com/eternalcode/core/translation/Translation.java
Show resolved
Hide resolved
Updated methods removeBroadcastWithType and removeLatestBroadcastWithType Removed unnecessary import
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (5)
eternalcore-core/src/main/java/com/eternalcode/core/feature/essentials/alert/AlertCommand.java (2)
14-15: Review permission scope for alert functionalityWith the planned separation of queue functionality, consider creating a hierarchical permission structure:
eternalcore.alertfor basic alertseternalcore.alert.queuefor queue operations
Line range hint
17-31: Implementation doesn't match PR objectives for queue functionalityThe current implementation only supports immediate alert broadcasts. To align with the PR objectives (GH-766), the following features need to be implemented:
- Queue management for multiple alerts
- Bulk dispatch functionality
- Queue status visibility
Consider implementing these features in a separate
AlertQueueCommandclass while keeping this class focused on immediate broadcasts.Would you like me to help generate the implementation for the
AlertQueueCommandclass with the queue functionality?eternalcore-core/src/main/java/com/eternalcode/core/translation/implementation/ENTranslation.java (1)
300-304: LGTM! Consider adding more context to some messages.The alert queue messages are well-structured and follow the project's conventions. However, consider enhancing user feedback with additional context:
- public Notice alertQueueAdded = Notice.chat("<green>► <white>Message added to the queue!"); + public Notice alertQueueAdded = Notice.chat("<green>► <white>Message added to the alert queue! Use /alert send to broadcast all queued messages."); - public Notice alertQueueEmpty = Notice.chat("<red>✘ <dark_red>Error: <red>The message queue is empty!"); + public Notice alertQueueEmpty = Notice.chat("<red>✘ <dark_red>Error: <red>The alert queue is empty! Add messages using /alert add <message>");eternalcore-core/src/main/java/com/eternalcode/core/feature/essentials/alert/AlertQueueCommand.java (2)
21-21: RenamealertServicetoalertManagerfor consistencyThe field
alertServicerefers to an instance ofAlertManager. Renaming it toalertManagerwill improve code readability and maintain consistency with the class name.Also applies to: 24-30
49-56: Refactor duplicated code inexecuteRemoveAllandexecuteRemovemethodsThe methods
executeRemoveAllandexecuteRemovecontain similar logic for removing alerts and sending notices. Consider extracting the common code into a private helper method to adhere to the DRY (Don't Repeat Yourself) principle and improve maintainability.Also applies to: 60-67
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
buildSrc/src/main/kotlin/Versions.kt(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/essentials/alert/AlertCommand.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/essentials/alert/AlertManager.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/essentials/alert/AlertQueueCommand.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/translation/Translation.java(2 hunks)eternalcore-core/src/main/java/com/eternalcode/core/translation/implementation/ENTranslation.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/translation/implementation/PLTranslation.java(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- buildSrc/src/main/kotlin/Versions.kt
🔇 Additional comments (7)
eternalcore-core/src/main/java/com/eternalcode/core/feature/essentials/alert/AlertCommand.java (1)
1-12: Consider separating queue-related functionality into a dedicated command class
Based on the PR objectives to implement an alert queue system and the previous review suggestion, this functionality should be separated into a dedicated AlertQueueCommand class with its own permission scope (eternalcore.alert.queue).
Let's verify if the queue-related code exists elsewhere:
✅ Verification successful
Queue functionality has been properly separated into AlertQueueCommand
The queue-related functionality has been correctly implemented in a dedicated AlertQueueCommand class with:
- Proper permission scope (
eternalcore.alert.queue) - Complete queue operations (add, remove, clear, send)
- Input validation and error handling
- Proper dependency injection
- Clear command structure with descriptive annotations
The original review comment's suggestion has been fully addressed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for AlertQueueCommand implementation
rg -t java "class AlertQueueCommand"
# Search for queue-related methods
ast-grep --pattern 'class $_ {
$$$
$_(queue|Queue)($_) {
$$$
}
$$$
}'
Length of output: 248
Script:
#!/bin/bash
# Let's check the content of AlertQueueCommand.java to verify if it's properly implemented
cat eternalcore-core/src/main/java/com/eternalcode/core/feature/essentials/alert/AlertQueueCommand.java
# Also check if there are any references to this command in other files
rg -l "AlertQueueCommand"
Length of output: 4024
eternalcore-core/src/main/java/com/eternalcode/core/feature/essentials/alert/AlertManager.java (3)
21-21: Thread safety concerns with HashMap persist
The thread safety issue identified in the previous review remains unaddressed.
29-31: Thread safety concerns with ArrayList persist
The thread safety issue with ArrayList identified in the previous review remains unaddressed.
76-76: LGTM! Good use of record for immutable key
The AlertKey record is well-designed and appropriately encapsulates the necessary fields.
eternalcore-core/src/main/java/com/eternalcode/core/translation/Translation.java (2)
9-9: Remove unnecessary empty line
This empty line between imports should be removed for consistency.
137-141: LGTM! Well-structured alert queue notification methods
The new methods follow a clear naming convention and provide comprehensive coverage for all alert queue operations. This aligns perfectly with the PR objective of enabling multiple notifications to be sent at once.
eternalcore-core/src/main/java/com/eternalcode/core/translation/implementation/PLTranslation.java (1)
303-307: LGTM! Alert queue messages are well-implemented.
The new notification messages for alert queue management are properly formatted, follow consistent naming conventions, and provide clear user feedback in Polish for each operation:
- Queue addition
- Queue removal
- Queue clearing
- Empty queue state
- Queue sending completion
eternalcore-core/src/main/java/com/eternalcode/core/feature/essentials/alert/AlertManager.java
Show resolved
Hide resolved
...core-core/src/main/java/com/eternalcode/core/feature/essentials/alert/AlertQueueCommand.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
eternalcore-core/src/main/java/com/eternalcode/core/feature/essentials/alert/AlertManager.java(1 hunks)
🔇 Additional comments (3)
eternalcore-core/src/main/java/com/eternalcode/core/feature/essentials/alert/AlertManager.java (3)
76-76: LGTM!
The AlertKey record is well-designed and appropriately encapsulates the key structure.
21-21:
Replace HashMap with ConcurrentHashMap for thread safety
The broadcasts map should be thread-safe since it's accessed from multiple threads (main thread and scheduler thread).
Apply this change:
- private final Map<AlertKey, List<EternalCoreBroadcastImpl<Viewer, Translation, ?>>> broadcasts = new HashMap<>();
+ private final Map<AlertKey, List<EternalCoreBroadcastImpl<Viewer, Translation, ?>>> broadcasts = new ConcurrentHashMap<>();Likely invalid or redundant comment.
29-31:
Use thread-safe list implementation
The ArrayList used in addBroadcast isn't thread-safe, which could lead to ConcurrentModificationException when accessed from multiple threads.
Apply this change:
- this.broadcasts.computeIfAbsent(new AlertKey(uuid, type), k -> new ArrayList<>()).add(broadcast);
+ this.broadcasts.computeIfAbsent(new AlertKey(uuid, type), k -> Collections.synchronizedList(new ArrayList<>())).add(broadcast);Likely invalid or redundant comment.
eternalcore-core/src/main/java/com/eternalcode/core/feature/essentials/alert/AlertManager.java
Outdated
Show resolved
Hide resolved
eternalcore-core/src/main/java/com/eternalcode/core/feature/essentials/alert/AlertCommand.java
Outdated
Show resolved
Hide resolved
...core-core/src/main/java/com/eternalcode/core/feature/essentials/alert/AlertQueueCommand.java
Outdated
Show resolved
Hide resolved
eternalcore-core/src/main/java/com/eternalcode/core/feature/essentials/alert/AlertManager.java
Outdated
Show resolved
Hide resolved
…sentials/alert/AlertQueueCommand.java Co-authored-by: Norbert Dejlich <ndejlich5@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
eternalcore-core/src/main/java/com/eternalcode/core/feature/essentials/alert/AlertManager.java (1)
38-50: Optimize list operations and key creationThe method can be optimized by:
- Using
removeLast()instead ofremove(size() - 1)- Reusing the AlertKey object
boolean removeLatestBroadcastWithType(UUID uuid, NoticeTextType type) { AlertKey key = new AlertKey(uuid, type); List<EternalCoreBroadcastImpl<Viewer, Translation, ?>> broadcastList = this.broadcasts.get(key); if (broadcastList != null && !broadcastList.isEmpty()) { - broadcastList.remove(broadcastList.size() - 1); + broadcastList.removeLast(); if (broadcastList.isEmpty()) { - this.removeBroadcastWithType(uuid, type); + this.broadcasts.remove(key); } return true; } return false; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
eternalcore-core/src/main/java/com/eternalcode/core/bridge/litecommand/argument/DurationArgument.java(0 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/essentials/alert/AlertCommand.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/essentials/alert/AlertManager.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/feature/essentials/alert/AlertQueueCommand.java(1 hunks)
💤 Files with no reviewable changes (1)
- eternalcore-core/src/main/java/com/eternalcode/core/bridge/litecommand/argument/DurationArgument.java
🚧 Files skipped from review as they are similar to previous changes (2)
- eternalcore-core/src/main/java/com/eternalcode/core/feature/essentials/alert/AlertCommand.java
- eternalcore-core/src/main/java/com/eternalcode/core/feature/essentials/alert/AlertQueueCommand.java
🔇 Additional comments (3)
eternalcore-core/src/main/java/com/eternalcode/core/feature/essentials/alert/AlertManager.java (3)
78-78: LGTM!
The AlertKey record is well-designed and properly encapsulated.
22-22:
Replace HashMap with ConcurrentHashMap for thread safety
The broadcasts map should be thread-safe since it's accessed from multiple threads (main thread and scheduler thread).
- private final Map<AlertKey, List<EternalCoreBroadcastImpl<Viewer, Translation, ?>>> broadcasts = new HashMap<>();
+ private final Map<AlertKey, List<EternalCoreBroadcastImpl<Viewer, Translation, ?>>> broadcasts = new ConcurrentHashMap<>();Likely invalid or redundant comment.
30-32:
Use thread-safe list implementation
The ArrayList used in addBroadcast isn't thread-safe, which could lead to ConcurrentModificationException when accessed from multiple threads.
- this.broadcasts.computeIfAbsent(new AlertKey(uuid, type), k -> new ArrayList<>()).add(broadcast);
+ this.broadcasts.computeIfAbsent(new AlertKey(uuid, type), k -> Collections.synchronizedList(new ArrayList<>())).add(broadcast);Likely invalid or redundant comment.
eternalcore-core/src/main/java/com/eternalcode/core/feature/essentials/alert/AlertManager.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
eternalcore-core/src/main/java/com/eternalcode/core/notice/EternalCoreBroadcast.java (2)
27-28: LGTM! Consider enhancing class documentation.The class declaration and inheritance structure look good. The use of generics ensures type safety in the builder pattern implementation.
Consider expanding the class documentation to include:
- Purpose of each generic type parameter
- Example usage
- Whether this class is intended for extension
/** * This class is an extension of {@link NoticeBroadcastImpl} that provides more methods for creating notices. + * + * @param <Viewer> The type of the viewer that will receive the notices + * @param <Translation> The type containing translation keys/messages + * @param <B> The builder type for method chaining (self-type) */
Line range hint
30-46: LGTM! Consider making the class final if not designed for extension.The constructor properly handles dependency injection and delegates to the parent class. The dependencies are well-organized and follow SOLID principles.
If this class is not designed to be extended further, consider marking it as
finalto make this intention explicit and prevent potential misuse:-public class EternalCoreBroadcast<Viewer, Translation, B extends EternalCoreBroadcast<Viewer, Translation, B>> +public final class EternalCoreBroadcast<Viewer, Translation, B extends EternalCoreBroadcast<Viewer, Translation, B>>eternalcore-core/src/main/java/com/eternalcode/core/notice/NoticeService.java (1)
115-116: Consider queue-specific broadcast implementation.Since this PR implements an alert queue feature, consider whether a specialized broadcast implementation (e.g.,
QueuedBroadcast extends EternalCoreBroadcast) might be beneficial for handling batched notifications differently from immediate ones.eternalcore-core/src/main/java/com/eternalcode/core/translation/implementation/ENTranslation.java (1)
299-303: LGTM! Consider enhancing feedback for queue operations.The alert queue messages are well-implemented and follow the established patterns in the codebase. The messages provide clear feedback for each operation (add, remove, clear, empty, sent).
Consider enhancing the feedback by including queue-specific information:
- public Notice alertQueueAdded = Notice.chat("<green>► <white>Message added to the queue!"); + public Notice alertQueueAdded = Notice.chat("<green>► <white>Message added to the queue! <gray>({QUEUE_SIZE} messages in queue)"); - public Notice alertQueueRemoved = Notice.chat("<green>► <white>Message removed from the queue!"); + public Notice alertQueueRemoved = Notice.chat("<green>► <white>Message removed from the queue! <gray>({QUEUE_SIZE} messages remaining)");This would provide users with more context about the queue state after each operation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
eternalcore-core/src/main/java/com/eternalcode/core/feature/essentials/alert/AlertManager.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/notice/EternalCoreBroadcast.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/notice/NoticeService.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/translation/Translation.java(2 hunks)eternalcore-core/src/main/java/com/eternalcode/core/translation/implementation/ENTranslation.java(1 hunks)eternalcore-core/src/main/java/com/eternalcode/core/translation/implementation/PLTranslation.java(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- eternalcore-core/src/main/java/com/eternalcode/core/feature/essentials/alert/AlertManager.java
🚧 Files skipped from review as they are similar to previous changes (2)
- eternalcore-core/src/main/java/com/eternalcode/core/translation/Translation.java
- eternalcore-core/src/main/java/com/eternalcode/core/translation/implementation/PLTranslation.java
🔇 Additional comments (1)
eternalcore-core/src/main/java/com/eternalcode/core/notice/NoticeService.java (1)
115-116: LGTM! Good practice using interface over implementation.
The change from EternalCoreBroadcastImpl to EternalCoreBroadcast follows the best practice of programming to interfaces rather than implementations, improving abstraction.
Let's verify the impact of this change on dependent code:
✅ Verification successful
Change from implementation to interface is safe to proceed
The verification shows this is a safe change because:
EternalCoreBroadcastis actually the concrete class (not an interface) that extendsNoticeBroadcastImpl- No references to
EternalCoreBroadcastImplwere found in the codebase - The only usages of
EternalCoreBroadcastare in:NoticeService.java(the changed file)AlertManager.java(only as an import)EternalCoreBroadcast.java(the class definition itself)
The change is merely removing a mistaken "Impl" suffix from the class name in the return type, making it consistent with the actual implementation. There are no breaking changes or impacts on dependent code.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any direct references to EternalCoreBroadcastImpl that might break
# Search for direct references to the implementation class
rg "EternalCoreBroadcastImpl" -l
# Search for potential cast operations
ast-grep --pattern 'EternalCoreBroadcastImpl $_'
Length of output: 83
Script:
#!/bin/bash
# Let's try a broader search to understand the broadcast implementation structure
# First, let's find all files containing EternalCoreBroadcast
rg "EternalCoreBroadcast" -l
# Then get more context around any broadcast usage
rg "EternalCoreBroadcast" -B 2 -A 2
# Also search for any class that extends or implements broadcast
ast-grep --pattern 'class $_ implements EternalCoreBroadcast'
ast-grep --pattern 'class $_ extends EternalCoreBroadcast'
Length of output: 2977
3 participants
Alert queue.
Simple feature that allows sending multiple notifications at once.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores
LITE_COMMANDSlibrary to 3.8.0.