-
-
Notifications
You must be signed in to change notification settings - Fork 105
Feature: Implement /rewrite command for message improvement using Cha… #1378
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
base: develop
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| 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. | ||
| * <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. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| * <p> | ||
| * Users can optionally specify a tone/style for the rewrite. If not provided, defaults to CLEAR. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| */ | ||
| public final class RewriteMsgCommand extends SlashCommandAdapter { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. dont abbreviate,
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @tj-wazei is this a requirement or an optional suggestion?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
| private static final Logger logger = LoggerFactory.getLogger(RewriteMsgCommand.class); | ||
Zabuzard marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| public static final String COMMAND_NAME = "rewrite-msg"; | ||
SquidXTV marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| 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; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please use
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unnecessary variable and can be inlined with the return. |
||
|
|
||
| return """ | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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)** | ||
Taz03 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| **Original:** | ||
| %s | ||
| **Rewritten:** | ||
Taz03 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| %s""".formatted(toneLabel, userMessage, rewrittenMessage); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. dont forget to |
||
| } | ||
|
|
||
| private static String buildChatGptPrompt(String userMessage, MsgTone tone) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. rename this to |
||
| return """ | ||
| Please rewrite the following message to make it clearer, more professional, \ | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. | ||
Taz03 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| Original message: | ||
Taz03 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| %s""".formatted(tone.description, userMessage); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. dont forget to |
||
| } | ||
|
|
||
| /** | ||
| * 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); | ||
Zabuzard marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| final OptionData toneOption = new OptionData(OptionType.STRING, TONE_OPTION, | ||
| "The tone/style for the rewritten message (default: " + MsgTone.CLEAR.displayName | ||
| + ")", | ||
| false); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing documentation
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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(); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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(); | ||
Zabuzard marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| final Optional<String> rewrittenMessage = this.rewrite(userMessage, tone); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Inconsistent use of
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: '{}'", | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
| userMessage); | ||
| event.getHook() | ||
| .editOriginal( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unnecessary use of Ideally, this entire block is a good candidate for
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. to be fair, |
||
|
|
||
| event.getHook().editOriginal(response).queue(); | ||
| } | ||
|
|
||
| private MsgTone parseTone(@Nullable OptionMapping toneOption, String userId) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You'll need to update the 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; | ||
| } | ||
| } | ||
| } | ||
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.
Please remove
ChatGPTfrom the docs as you leak too much implementation details. "using AI" is fine.