Skip to content

Send chat prompt message to lsp - #26

Merged
breedloj merged 8 commits into
mainfrom
send-chat-prompt-to-lsp
Sep 20, 2024
Merged

Send chat prompt message to lsp#26
breedloj merged 8 commits into
mainfrom
send-chat-prompt-to-lsp

Conversation

@angjordn

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

@angjordn angjordn left a comment

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
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.

@angjordn angjordn left a comment

Copy link
Copy Markdown
Contributor Author

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

return null;
default:
PluginLogger.error("Unhandled chat command: " + command.toString());
throw new AmazonQPluginException("Unhandled command in ChatCommunicationManager: " + command.toString());

Copy link
Copy Markdown
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.

default:
PluginLogger.info("Unhandled command: " + parsedCommand.getCommand());
break;
throw new AmazonQPluginException("Unhandled command in AmazonQChatViewActionHandler: " + command.toString());

Copy link
Copy Markdown
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
@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