Skip to content

feat: added support for injecting additional context commands #1045

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

Merged
merged 9 commits into from
Apr 22, 2025

Conversation

anasstahr
Copy link
Contributor

Problem

Currently we don't have a way to support additional context commands from the client side.

Solution

Adding support for this

License

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

amazonQChat.createChat(acquireVsCodeApi(),
{disclaimerAcknowledged: false},
undefined,
undefined,
Copy link
Contributor

Choose a reason for hiding this comment

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

can we have example to show @sage injected here please?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will add

@@ -200,10 +201,11 @@ export const createMynahUi = (
},
onTabAdd: (tabId: string) => {
const defaultTabBarData = tabFactory.getDefaultTabData()
const contextCommands = [...(contextCommandGroups || []), ...(featureConfig?.get('contextCommands') || [])]
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we doing here some thing different in comparison to what was done above? Can we share the logic? Why is 'contextCommands' key used here, but not in the logic above?


const additionalContextCommands: any[] = []
featureConfig.forEach((featureContext: FeatureContext, _feature: string) => {
additionalContextCommands.push({
Copy link
Contributor

Choose a reason for hiding this comment

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

I do not think it's safe to use all the items from featureConfig as additional context commands. Can we just pull that 'highlightCommand' here, which is used for @Sage?

@anasstahr anasstahr marked this pull request as ready for review April 22, 2025 12:52
@anasstahr anasstahr requested a review from a team as a code owner April 22, 2025 12:52
commands: [
{
description: featureConfig.get('highlightCommands')?.variation ?? '',
command: featureConfig.get('highlightCommands')?.value.stringValue ?? '',
Copy link
Contributor

Choose a reason for hiding this comment

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

we should not create empty commands - check if 'highlightCommands' before defining all this section

groupName: 'Additional Commands',
commands: [
{
command: highlightedCommands.value.stringValue ?? '',
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here. highlightedCommands can be undefined here. Can you please test your code with and without FeatureConfig, it should work in both cases.

@volodkevych volodkevych self-requested a review April 22, 2025 15:02
@anasstahr anasstahr merged commit d755da3 into main Apr 22, 2025
6 checks passed
@anasstahr anasstahr deleted the anasstahr/context-commands branch April 22, 2025 15:03
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