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

Send chat prompt message to lsp #26

Merged
merged 8 commits into from
Sep 20, 2024
Merged

Send chat prompt message to lsp #26

merged 8 commits into from
Sep 20, 2024

Conversation

angjordn
Copy link
Contributor

Description:

  • Add handler for aws/chat/sendChatPrompt message
  • Intentionally does not handle the Response (currently just logging)

Demo of UI to Logged response

Log_response.mov

@angjordn angjordn requested a review from breedloj September 18, 2024 22:01
Base automatically changed from send-chat-ready-to-lsp to main September 18, 2024 22:30
Comment on lines 34 to 36
e.printStackTrace();
}
return null;
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of swallowing the exception let's wrap the original with an AmazonQPluginException and throw it back to the caller. In that case it should probably be the top level webview command execution logic that catches any error occurring during the processing of one of the commands. That could then log + potentially display an error to the user in the view (this part can come later).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea! Didn't realize there was an AmazonQPluginException. Added to the other LSP send functions also.

Comment on lines 28 to 31
void tabAdd(GenericTabParams params);

@JsonNotification("aws/chat/ready")
void chatReady();
Copy link
Contributor

Choose a reason for hiding this comment

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

Did it not work when annotated as a JsonRequest with a void response type (CompletableFuture<Void>)? Maybe this makes more sense as an explicit fire & forget non blocking type though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah the LSP server did not recognize the request when it was annotated as a JsonRequest. The server is explicitly expecting a Notification. Surprisingly, most of the LSP features are Notifications rather than requests as specified in their doc (https://github.com/aws/language-server-runtimes/tree/main/runtimes#feature-specification-4)

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool, thanks for the info. That makes sense.

@angjordn angjordn force-pushed the send-chat-prompt-to-lsp branch from a5ca0d4 to 4ff9d04 Compare September 19, 2024 19:09
Copy link
Contributor Author

@angjordn angjordn left a comment

Choose a reason for hiding this comment

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

Made the requested updates

Comment on lines 39 to 41
PluginLogger.error("Unhandled chat command: " + command.toString());
}
return null;
Copy link
Contributor

Choose a reason for hiding this comment

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

Avoid the null return by explicitly raising a PluginException here. This is a specific internal failure that should not be squelched.

Copy link
Contributor Author

@angjordn angjordn left a comment

Choose a reason for hiding this comment

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

Throw AmazonQPlugiinException for missing commands and fix checkstyle errors

default:
PluginLogger.error("Unhandled chat command: " + command.toString());
throw new AmazonQPluginException("Unhandled command in ChatCommunicationManager: " + command.toString());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@breedloj Throwing the PluginException as you mentioned. You also mentioned throwing the exception to avoid returning null - I moved the return null statements to the associated cases to be more explicit about which cases null is being returned on.

@@ -34,8 +40,7 @@ public final void handleCommand(final ParsedCommand parsedCommand, final Browser
case TELEMETRY_EVENT:
break;
default:
PluginLogger.info("Unhandled command: " + parsedCommand.getCommand());
break;
throw new AmazonQPluginException("Unhandled command in AmazonQChatViewActionHandler: " + command.toString());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@breedloj Also updated the ActionHandler to a PluginException to match the unhandled command case in ChatCommunicationManager

@breedloj breedloj merged commit 3b09977 into main Sep 20, 2024
1 check passed
@breedloj breedloj deleted the send-chat-prompt-to-lsp branch September 20, 2024 18:16
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.

2 participants