Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@
import org.togetherjava.tjbot.features.mathcommands.wolframalpha.WolframAlphaCommand;
import org.togetherjava.tjbot.features.mediaonly.MediaOnlyChannelListener;
import org.togetherjava.tjbot.features.messages.MessageCommand;
import org.togetherjava.tjbot.features.messages.RewriteMsgCommand;
import org.togetherjava.tjbot.features.moderation.BanCommand;
import org.togetherjava.tjbot.features.moderation.KickCommand;
import org.togetherjava.tjbot.features.moderation.ModerationActionsStore;
Expand Down Expand Up @@ -207,6 +208,7 @@ public static Collection<Feature> createFeatures(JDA jda, Database database, Con
features.add(new ChatGptCommand(chatGptService, helpSystemHelper));
features.add(new JShellCommand(jshellEval));
features.add(new MessageCommand());
features.add(new RewriteMsgCommand(chatGptService));

FeatureBlacklist<Class<?>> blacklist = blacklistConfig.normal();
return blacklist.filterStream(features.stream(), Object::getClass).toList();
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,153 @@
package org.togetherjava.tjbot.features.messages;

import net.dv8tion.jda.api.events.interaction.command.SlashCommandInteractionEvent;
import net.dv8tion.jda.api.interactions.commands.OptionMapping;
import net.dv8tion.jda.api.interactions.commands.OptionType;
import net.dv8tion.jda.api.interactions.commands.build.OptionData;
import org.jetbrains.annotations.Nullable;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import org.togetherjava.tjbot.features.CommandVisibility;
import org.togetherjava.tjbot.features.SlashCommandAdapter;
import org.togetherjava.tjbot.features.chatgpt.ChatGptModel;
import org.togetherjava.tjbot.features.chatgpt.ChatGptService;

import java.util.Arrays;
import java.util.Objects;
import java.util.Optional;

/**
* The implemented command is {@code /rewrite-msg}, which allows users to have their message
* rewritten in a clearer, more professional, or better structured form using ChatGPT AI.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove ChatGPT from the docs as you leak too much implementation details. "using AI" is fine.

* <p>
* The rewritten message is shown as an ephemeral message visible only to the user who triggered the
* command, making it perfect for getting quick writing improvements without cluttering the channel.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

making it perfect for getting quick writing improvements without cluttering the channel. is unnecessary for code documentation.

* <p>
* Users can optionally specify a tone/style for the rewrite. If not provided, defaults to CLEAR.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

defaults to CLEAR does not tell a developer what CLEAR is so please can you change this so it makes sense without coupling it to the enum (that isn't even linked).

*/
public final class RewriteMsgCommand extends SlashCommandAdapter {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dont abbreviate, RewriteMsgCommand. but to avoid confusion i would try to stick to the actual slash-command name, so just RewriteCommand would be better imo

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about doing that in the beginning, but I found that it may be confusing, since it's possible to be interpreted "rewrite something else, other than a message", so I found it better to write "Message" as abbreviation to avoid verbosity

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but that confusion then also applies to the slashcommand. if u think its confusing then the slash command has to be renamed as well, to /rewrite-msg for example. then the class name is okay again

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/rewrite is perfectly fine

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tj-wazei is this a requirement or an optional suggestion?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is his opinion as code-reviewer. and now we come to a consensus. i am also fine with rewrite but im still proposing to call the class RewriteCommand in that case

private static final Logger logger = LoggerFactory.getLogger(RewriteMsgCommand.class);
public static final String COMMAND_NAME = "rewrite-msg";
private static final String MESSAGE_OPTION = "message";
private static final String TONE_OPTION = "tone";
private static final int MAX_MESSAGE_LENGTH = 500;
private static final int MIN_MESSAGE_LENGTH = 3;
private static final ChatGptModel CHAT_GPT_MODEL = ChatGptModel.HIGH_QUALITY;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use ChatGptModel.FASTEST - you do not need HIGH_QUALITY for re-writing a message. Primarily because you do not need increased reasoning capability for what you are trying to do.

Copy link
Member Author

@firasrg firasrg Jan 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about that at first, but then I realized: what if the AI ​​doesn't take enough time to think? That said, you're right that the FASTEST model is more likely to be correct in most situations. However, I still disagree with using this model if the user's tone is TECHNICAL or DETAILED

I think to make the AI selection dynamic, WDYT ?


private final ChatGptService chatGptService;

private static String buildResponse(String userMessage, String rewrittenMessage, MsgTone tone) {
final String toneLabel = tone.displayName;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unnecessary variable and can be inlined with the return.


return """
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The response to the user cannot be a single message. We discussed on Discord that users on mobile cannot copy sections of an embed. This is actually also the case for regular text messages.

**Rewritten message (%s)**
**Original:**
%s
**Rewritten:**
%s""".formatted(toneLabel, userMessage, rewrittenMessage);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dont forget to String#stripIndent when using multi-line strings

}

private static String buildChatGptPrompt(String userMessage, MsgTone tone) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rename this to createAiPrompt, you do not need to leak the implementation detail here.

return """
Please rewrite the following message to make it clearer, more professional, \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You do not need to be kind to an AI and the prompt is too wordy for the intent. Change it to either this or similar:

Rewrite the following message to be clearer, more professional, and better structured. Keep the original meaning. Do not use em-dashes. If it is already well-written, make minor improvements.
Original message: %s

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok but what about the tone? you didn't consider it in your example, why?

and better structured. Maintain the original meaning while improving the quality \
of the writing. Do NOT use em-dashes (—). %s
If the message is already well-written, provide minor improvements.
Original message:
%s""".formatted(tone.description, userMessage);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dont forget to String#stripIndent when using multi-line strings

}

/**
* Creates the slash command definition and configures available options for rewriting messages.
*
* @param chatGptService service for interacting with ChatGPT
*/
public RewriteMsgCommand(ChatGptService chatGptService) {
super(COMMAND_NAME, "Let AI rephrase and improve your message", CommandVisibility.GUILD);

this.chatGptService = chatGptService;

final OptionData messageOption =
new OptionData(OptionType.STRING, MESSAGE_OPTION, "The message you want to rewrite",
true)
.setMinLength(MIN_MESSAGE_LENGTH)
.setMaxLength(MAX_MESSAGE_LENGTH);

final OptionData toneOption = new OptionData(OptionType.STRING, TONE_OPTION,
"The tone/style for the rewritten message (default: " + MsgTone.CLEAR.displayName
+ ")",
false);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

y not also make it auto-completeable?


Arrays.stream(MsgTone.values())
.forEach(tone -> toneOption.addChoice(tone.displayName, tone.name()));

getData().addOptions(messageOption, toneOption);
}

@Override
public void onSlashCommand(SlashCommandInteractionEvent event) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing documentation

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After reviewing other command implementations, I found that there was no need to document it

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it has javadoc already from the parent implementation its overriding

final String userMessage =
Objects.requireNonNull(event.getOption(MESSAGE_OPTION)).getAsString();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need for requireNonNull


final MsgTone tone = parseTone(event.getOption(TONE_OPTION), event.getUser().getId());

event.deferReply(true).queue();

final Optional<String> rewrittenMessage = this.rewrite(userMessage, tone);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inconsistent use of this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean, because if it's not being used everywhere like in the call of parseTone()? Or do you mean that it's useless ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

former. remove it, its not adding anything to the code reader other than confusion


if (rewrittenMessage.isEmpty()) {
logger.debug("Failed to obtain a response for /rewrite-msg, original message: '{}'",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do not hard code the name here, instead used the constant you defined COMMAND_NAME

userMessage);
event.getHook()
.editOriginal(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't good UX. If the user spent a fair bit of time trying to craft their message and an error occurs, they lose it. Instead, return back the original message - an embed might be useful here to add the fact there was an "error" but the user can still copy their original message and send that, should they wish.

"An error occurred while processing your request. Please try again later.")
.queue();
return;
}

final String response = buildResponse(userMessage, rewrittenMessage.orElseThrow(), tone);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unnecessary use of orElseThrow since it will never throw. You are doing an isEmpty check above.

Ideally, this entire block is a good candidate for rewrittenMessage.ifPresentOrElse() if you want that fluency.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to be fair, if (foo.isEmpty()) ... foo.orElseThrow() is the normal way ur supposed to deal with this in case you are not able to utilize the other, better, methods of the API. (but in this case, try to put the extraction of the content right after the if-check, not 20 lines later)


event.getHook().editOriginal(response).queue();
}

private MsgTone parseTone(@Nullable OptionMapping toneOption, String userId)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are you taking in the userId just for the logs? seems unnecessary

throws IllegalArgumentException {
if (toneOption == null) {
logger.debug("Tone option not provided for user: {}, using default CLEAR", userId);
return MsgTone.CLEAR;
}
Comment on lines +120 to +123
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cannot be null


final String toneValue = toneOption.getAsString();
final MsgTone tone = MsgTone.valueOf(toneValue);

logger.debug("Parsed tone '{}' for user: {}", tone.displayName, userId);

return tone;
}

private Optional<String> rewrite(String userMessage, MsgTone tone) {
final String rewritePrompt = buildChatGptPrompt(userMessage, tone);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can inline this String


return chatGptService.ask(rewritePrompt, tone.displayName, CHAT_GPT_MODEL);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You'll need to update the ask method. The original implementor of the ChatGptService did not make this abstract enough so your request will be mixed with the following prefixed:

For code supplied for review, refer to the old code supplied rather than  rewriting the code. DON'T supply a corrected version of the code.

KEEP IT CONCISE, NOT MORE THAN 280 WORDS

%s

Question: %s

}

private enum MsgTone {
CLEAR("Clear", "Make it clear and easy to understand."),
PRO("Pro", "Use a professional and polished tone."),
DETAILED("Detailed", "Expand with more detail and explanation."),
TECHNICAL("Technical", "Use technical and specialized language where appropriate.");

private final String displayName;
private final String description;

MsgTone(String displayName, String description) {
this.displayName = displayName;
this.description = description;
}
}
}
Loading