GH-1296 feat(helpop): add helpop reply functionality and related events #1296

vLuckyyy · 2026-02-09T20:13:41Z

No description provided.

…nctionality

gemini-code-assist

Code Review

This pull request introduces a valuable helpop reply feature, complete with a new service, command, and event. The implementation is well-structured and the code is generally clean. I've identified a couple of areas for improvement related to configurability and code coupling to enhance maintainability, which are detailed in the specific comments.

gemini-code-assist · 2026-02-09T20:14:54Z

eternalcore-core/src/main/java/com/eternalcode/core/feature/helpop/HelpOpReplyCommand.java

+            .placeholder("{TEXT}", escapedMessage);
+
+        for (Player onlinePlayer : this.server.getOnlinePlayers()) {
+            if (!onlinePlayer.hasPermission(HelpOpCommand.HELPOP_SPY)) {


The HELPOP_SPY constant is referenced from HelpOpCommand. This creates a tight coupling between two command classes. It would be better to define this permission constant in a more central and accessible place, for example, the HelpOpService interface. This would improve modularity and make the code easier to maintain.

For example, you could add String HELPOP_SPY_PERMISSION = "eternalcore.helpop.spy"; to HelpOpService and use it in both command classes.

gemini-code-assist · 2026-02-09T20:14:54Z

eternalcore-core/src/main/java/com/eternalcode/core/feature/helpop/HelpOpServiceImpl.java

+class HelpOpServiceImpl implements HelpOpService {
+
+    private final Cache<UUID, Boolean> helpOpSenders = CacheBuilder.newBuilder()
+        .expireAfterWrite(Duration.ofHours(1))


The cache expiration is hardcoded to 1 hour. The Javadoc for HelpOpService#hasSentHelpOp states that 'Helpop requests expire after a configured duration (default: 1 hour)', which implies this value should be configurable. Consider adding a setting for this in HelpOpSettings and using it here to allow for configuration.

imDMK · 2026-02-09T20:24:12Z

eternalcore-api/src/main/java/com/eternalcode/core/feature/helpop/HelpOpService.java

+     *
+     * @param playerUuid the UUID of the player who sent the helpop request
+     */
+    void markSender(@NotNull UUID playerUuid);


imDMK · 2026-02-09T20:26:46Z

eternalcore-core/src/main/java/com/eternalcode/core/feature/helpop/HelpOpServiceImpl.java

+import org.jspecify.annotations.NonNull;
+
+@Service
+class HelpOpServiceImpl implements HelpOpService {


Suggested change

class HelpOpServiceImpl implements HelpOpService {

class CachedHelpOpService implements HelpOpService {

why though?? So we should add the Cached prefix to everything that has cache? I think we have other services and they don't have that prefix, so we need to be consistent.

imDMK · 2026-02-09T20:28:35Z

eternalcore-core/src/main/java/com/eternalcode/core/feature/helpop/HelpOpServiceImpl.java

+class HelpOpServiceImpl implements HelpOpService {
+
+    private final Cache<UUID, Boolean> helpOpSenders = CacheBuilder.newBuilder()
+        .expireAfterWrite(Duration.ofHours(1))


imo too much

Suggested change

.expireAfterWrite(Duration.ofHours(1))

.expireAfterWrite(Duration.ofMinutes(10))

Let's make this configurable

Jakubk15 · 2026-02-09T20:21:45Z

eternalcore-api/src/main/java/com/eternalcode/core/feature/helpop/event/HelpOpReplyEvent.java

+    /**
+     * Gets the reply message content.
+     *
+     * @return the reply content


Suggested change

* @return the reply content

* @return the reply content as raw string

I think it would be good to specify this in Javadoc, as the tag-escaping process is done after calling the event: https://github.com/EternalCodeTeam/EternalCore/pull/1296/changes#diff-2c33053246b78aac204d193999e57efd64d38f95eeb1bd6cc571be001f923810R62

Jakubk15 · 2026-02-09T20:23:32Z

eternalcore-core/src/main/java/com/eternalcode/core/feature/helpop/HelpOpServiceImpl.java

+class HelpOpServiceImpl implements HelpOpService {
+
+    private final Cache<UUID, Boolean> helpOpSenders = CacheBuilder.newBuilder()
+        .expireAfterWrite(Duration.ofHours(1))


Agreed with Gemini here; it won't hurt us if we make this configurable.

Jakubk15 · 2026-02-09T20:29:06Z

eternalcore-core/src/main/java/com/eternalcode/core/feature/helpop/HelpOpReplyCommand.java

+        for (Player onlinePlayer : this.server.getOnlinePlayers()) {
+            if (!onlinePlayer.hasPermission(HelpOpCommand.HELPOP_SPY)) {
+                continue;
+            }
+
+            notice = notice.player(onlinePlayer.getUniqueId());
+        }


https://github.com/EternalCodeTeam/EternalCore/blob/master/eternalcore-core/src/main/java/com/eternalcode/core/viewer/BukkitViewerProvider.java#L54
I think you can use .onlinePlayers(String permission)

Jakubk15 · 2026-02-09T20:35:08Z

eternalcore-core/src/main/java/com/eternalcode/core/feature/helpop/HelpOpServiceImpl.java

+@Service
+class HelpOpServiceImpl implements HelpOpService {
+
+    private final Cache<UUID, Boolean> helpOpSenders = CacheBuilder.newBuilder()


Question, do we plan to manually invalidate this cache? The current implementation just allows the administrators to reply to a single HelpOp request infinite times in the given time period.

Jakubk15 · 2026-02-09T20:38:44Z

eternalcore-core/src/main/java/com/eternalcode/core/feature/helpop/HelpOpReplyCommand.java

+            .placeholder("{TEXT}", escapedMessage);
+
+        for (Player onlinePlayer : this.server.getOnlinePlayers()) {
+            if (!onlinePlayer.hasPermission(HelpOpCommand.HELPOP_SPY)) {


I like the concept, but in a few hundred player servers with players spamming the helpop and admins replying, it will basically also send the reply to all other administrators with the permission, and they have absolutely no way to toggle it.

A /helpoptoggle <all/reply/requests> with a simple cache should suffice IMO. Up for discussion with other reviewers.

Jakubk15 · 2026-02-09T20:41:18Z

...alcore-core/src/main/java/com/eternalcode/core/feature/helpop/messages/ENHelpOpMessages.java

+
+    @Comment("# {ADMIN} - Admin who replied, {PLAYER} - Player who received the reply, {TEXT} - Reply message")
+    Notice adminReply = BukkitNotice.builder()
+        .chat("<dark_gray>[<color:#9d6eef>HelpOp<dark_gray>] <white>{ADMIN} <dark_gray>-> <white>{PLAYER}<dark_gray>: <white>{TEXT}")


Suggested change

.chat("<dark_gray>[<color:#9d6eef>HelpOp<dark_gray>] <white>{ADMIN} <dark_gray>-> <white>{PLAYER}<dark_gray>: <white>{TEXT}")

.chat("<dark_gray>[<color:#9d6eef>HelpOp<dark_gray>] <white>{ADMIN} <dark_gray>→ <white>{PLAYER}<dark_gray>: <white>{TEXT}")

CitralFlo · 2026-02-11T18:33:42Z

eternalcore-api/src/main/java/com/eternalcode/core/feature/helpop/event/HelpOpReplyEvent.java

+    private String content;
+    private boolean cancelled;
+
+    public HelpOpReplyEvent(@NotNull Player admin, @NotNull Player target, @NotNull String content) {


why not the Jspecify?????????????????????

applies for the whole class

CitralFlo · 2026-02-11T18:37:27Z

...alcore-core/src/main/java/com/eternalcode/core/feature/helpop/messages/PLHelpOpMessages.java

+        .sound(Sound.ENTITY_EXPERIENCE_ORB_PICKUP, 1.0f, 1.0f)
+        .build();
+
+    @Comment(" ")


Suggested change

@Comment(" ")

@Comment("# {PLAYER} - Nazwa gracza")

CitralFlo · 2026-02-11T18:38:47Z

eternalcore-core/src/main/java/com/eternalcode/core/feature/helpop/HelpOpReplyCommand.java

+        this.eventCaller = eventCaller;
+        this.server = server;
+    }
+


maybe add default execute - when player is not provided - then admin replays to the last helpop message

CitralFlo · 2026-02-11T18:40:15Z

eternalcore-core/src/main/java/com/eternalcode/core/feature/helpop/HelpOpReplyCommand.java

+
+@Command(name = "helpopreply")


maybe add alias "hr"

feat(helpop): add helpop reply functionality and related events

996bd1a

vLuckyyy requested a review from a team as a code owner February 9, 2026 20:13

feat(helpop): add admin reply messages and translations for helpop fu…

76bf7ab

…nctionality

vLuckyyy changed the title ~~feat(helpop): add helpop reply functionality and related events~~ GH-1296 feat(helpop): add helpop reply functionality and related events Feb 9, 2026

gemini-code-assist bot reviewed Feb 9, 2026

View reviewed changes

vLuckyyy requested review from CitralFlo, Jakubk15 and imDMK February 9, 2026 20:17

style(helpop): format constructor parameters for better readability

32303c8

imDMK approved these changes Feb 9, 2026

View reviewed changes

Jakubk15 reviewed Feb 9, 2026

View reviewed changes

CitralFlo approved these changes Feb 11, 2026

View reviewed changes

	class HelpOpServiceImpl implements HelpOpService {
	class CachedHelpOpService implements HelpOpService {

	.expireAfterWrite(Duration.ofHours(1))
	.expireAfterWrite(Duration.ofMinutes(10))

	* @return the reply content
	* @return the reply content as raw string

	.chat("<dark_gray>[<color:#9d6eef>HelpOp<dark_gray>] <white>{ADMIN} <dark_gray>-> <white>{PLAYER}<dark_gray>: <white>{TEXT}")
	.chat("<dark_gray>[<color:#9d6eef>HelpOp<dark_gray>] <white>{ADMIN} <dark_gray>→ <white>{PLAYER}<dark_gray>: <white>{TEXT}")


		@Command(name = "helpopreply")

Uh oh!

GH-1296 feat(helpop): add helpop reply functionality and related events #1296

Are you sure you want to change the base?

GH-1296 feat(helpop): add helpop reply functionality and related events #1296

Conversation

vLuckyyy commented Feb 9, 2026

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Feb 9, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Feb 9, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants