Skip to content
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

Add a command to show list of supported commands #396

Merged
merged 4 commits into from Feb 7, 2024
Merged

Conversation

Amnish04
Copy link
Collaborator

@Amnish04 Amnish04 commented Feb 1, 2024

Currently, when a user enters an unsupported command, a toast message is displayed suggesting the commands was invalid.

I have added a new command called /commands that allows users to quickly refer to the list of commands instead of the general /help command.
image

The list of commands is also displayed when an invalid command is entered instead of showing the toast message.

This fixes #356

@Amnish04 Amnish04 self-assigned this Feb 1, 2024
Copy link
Collaborator

@rjwignar rjwignar left a comment

Choose a reason for hiding this comment

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

The feature works, and the code looks great!
I've added some general comments but the only change I require before approving this is updating the Commands section of the helpText constant in src/components/Message/AppMessage/Help.tsx to include a description for your new command.

Comment on lines 178 to 186
} catch (err: any) {
error({
title: `Unknown Command`,
message: `Command not recognized. Use /help to get help on valid commands.`,
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just curious. Since you've added logic for the /commands command, is there any circumstance where this Toast message would appear?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That is just a fallback if for some reason, the execution of /commands command runs into an exception (which I don't think should ever happen).

Should we remove it?

Copy link
Collaborator

Choose a reason for hiding this comment

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

"Should never happen" is not the same as "Is impossible." I'd leave it so we don't crash. If it never shows the error toast, that's fine.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You're right. I did not remove it, but changed the wording to be same as if it happened with any other command since the Unknown Command toast is always shown now
image

src/components/Message/AppMessage/Help.tsx Outdated Show resolved Hide resolved
Comment on lines 142 to 149
| Command | Description |
|-----|------|
| /help | Shows this help message. |
| /commands | Shows a list of **supported commands** in ChatCraft |
| /new | Creates a new chat. |
| /clear | Erases all messages in the current chat. |
| /summary [max-length] | Uses ChatGPT to create a summary of the current chat. Optionally takes a maximum word length (defaults to 500). |
| /import&nbsp;<url> | Loads the provided URL and imports the text. Where possible, ChatCraft will try to get raw text vs. HTML from sites like GitHub. NOTE: to prevent abuse, you must be logged into use the import command. |`;
Copy link
Collaborator

Choose a reason for hiding this comment

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

The commands list in helpText should be updated to include your new /commands command(Lines 91-97), as you've done here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure, I missed that. Thanks for noticing

forceScroll();
} catch (err: any) {
error({
title: `Unknown Command`,
Copy link
Owner

Choose a reason for hiding this comment

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

Why is /commands better than showing /help with all commands?

We should show the command that we failed to identify. I get a lot of errors from chatcraft when i try to paste /something and it's not immediately obvious without an indicator of what i typed

Copy link
Collaborator

Choose a reason for hiding this comment

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

it reminds me of
image

Copy link
Collaborator

@rjwignar rjwignar Feb 2, 2024

Choose a reason for hiding this comment

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

We should show the command that we failed to identify. I get a lot of errors from chatcraft when i try to paste /something and it's not immediately obvious without an indicator of what i typed

That's a good point.
Would it be better to have that indicated in /commands or inside the original Toast error message we currently have in the main branch?
Something like below?

/commands
image

Toast Message:
image

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@tarasglek The /help command displays all the general help information related to ChatCraft. So it gets a little harder for user to skim through and find the supported commands.
image

Using /commands, it just becomes a little more convenient to look them up.
image

But you're right, we should also show the command that we typed as its not even added to the chat.

@rjwignar I think we should add it to the toast message?

@mingming-ma I like the idea of suggesting a similar command. Should we have a separate issue for that?

Copy link

cloudflare-pages bot commented Feb 2, 2024

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 8a8d1ae
Status: ✅  Deploy successful!
Preview URL: https://86bc40f9.console-overthinker-dev.pages.dev
Branch Preview URL: https://amnish04-issue356.console-overthinker-dev.pages.dev

View logs

@Amnish04
Copy link
Collaborator Author

Amnish04 commented Feb 2, 2024

Just pushed some changes. I've restored the toast message for Unknown Command and now it also displays what command was typed. The list of commands is also displayed.
image

Copy link
Collaborator

@rjwignar rjwignar left a comment

Choose a reason for hiding this comment

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

I like how the toast message displays the unrecognized command now.

With these changes, ChatCraft will now display both a Toast error message print the list of commands (by invoking /commands) when an unrecognized command is sent:
image.

These changes have my approval, but I'd like to see how others feel about both being displayed. @humphd, @mingming-ma, what are your thoughts on this?

Copy link
Collaborator

@mingming-ma mingming-ma left a comment

Choose a reason for hiding this comment

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

For the most similar commands, I think it might be not so helpful if we have all commands displayed. I think we can import the commandsHelpText to Help so when new commands coming in we don't need to change twice

Comment on lines 91 to 97
| Command | Description |
|-----|------|
| /help | Shows this help message. |
| /commands | Shows a list of **supported commands** in ChatCraft |
| /new | Creates a new chat. |
| /clear | Erases all messages in the current chat. |
| /summary&nbsp;[max-length] | Uses ChatGPT to create a summary of the current chat. Optionally takes a maximum word length (defaults to 500). |
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
| Command | Description |
|-----|------|
| /help | Shows this help message. |
| /commands | Shows a list of **supported commands** in ChatCraft |
| /new | Creates a new chat. |
| /clear | Erases all messages in the current chat. |
| /summary&nbsp;[max-length] | Uses ChatGPT to create a summary of the current chat. Optionally takes a maximum word length (defaults to 500). |
${commandsHelpText}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@mingming-ma Agreed, I'll commit that.

@tarasglek
Copy link
Owner

I don't think we need to toast here, how about we show the error inline in the commands help message reply?

@Amnish04
Copy link
Collaborator Author

Amnish04 commented Feb 5, 2024

I don't think we need to toast here, how about we show the error inline in the commands help message reply?

Yes, I think that makes more sense.

@Amnish04
Copy link
Collaborator Author

Amnish04 commented Feb 7, 2024

@tarasglek Just pushed the changes.

Any invalid command now shows up inline with the commands help text.

Here are the scenarios:

1. /typo
image

2. /commands
image

3. /commands query-some-command
image

I am now allowing the CommandsHelpCommand to accept arguments which we can later use to implement a suggest closest command feature based on @mingming-ma's suggestion.

My only concern is if I could use a simpler approach for this.

@tarasglek
Copy link
Owner

tarasglek commented Feb 7, 2024

lgtm

@Amnish04
Copy link
Collaborator Author

Amnish04 commented Feb 7, 2024

@mingming-ma @rjwignar If you guys also think this is good, should I merge?

@mingming-ma
Copy link
Collaborator

lgtm

Copy link
Collaborator

@rjwignar rjwignar left a comment

Choose a reason for hiding this comment

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

Changes look great! LGTM

@Amnish04
Copy link
Collaborator Author

Amnish04 commented Feb 7, 2024

Thanks for reviewing everyone!

@Amnish04 Amnish04 merged commit a1ab665 into main Feb 7, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Show help for unknown command
5 participants