-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Observability AI Assistant] Chat tweaks + keyboard shortcut #176350
Conversation
🤖 GitHub commentsExpand to view the GitHub comments
Just comment with:
|
Triggered this error when opening conversation app from flyout conversation_open_error.mov |
@@ -30,6 +30,20 @@ export function ObservabilityAIAssistantActionMenuItem() { | |||
|
|||
const initialMessages = useMemo(() => [], []); | |||
|
|||
useEffect(() => { | |||
const keyboardListener = (event: KeyboardEvent) => { | |||
if (event.ctrlKey && event.code === 'Semicolon') { |
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.
Is this documented anywhere for users ?
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.
Not yet. Perhaps we can add something instead of the friendly Moose image. @boriskirov ?
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.
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.
I like it! @boriskirov wdyt?
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.
Yes, good idea!
<EuiButtonIcon | ||
aria-label={i18n.translate( | ||
'xpack.observabilityAiAssistant.chatHeader.euiButtonIcon.navigateToConversationsLabel', | ||
{ defaultMessage: 'Navigate to conversations' } |
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.
It's not obvious to me what this button does since it only has icon, wonder if we should add a tooltip to it ?
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.
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.
Yes, I considered that, was wondering if it wouldn't make the UI too busy. But yeah, can be added.
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.
Done!
const handleNavigateToConversations = () => { | ||
if (conversationId) { | ||
router.navigateToConversationsApp('/conversations/{conversationId}', { | ||
path: { | ||
conversationId, | ||
}, | ||
query: {}, | ||
}); | ||
} else { | ||
router.navigateToConversationsApp('/conversations/new', { path: {}, query: {} }); | ||
} | ||
}; |
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.
I'm getting this error when starting a new conversation, then clicking on the conversations icon
Conversation not found
Could not find a conversation with id :conversationId. Make sure the conversation exists and you have access to it.
URL is /observabilityAIAssistant/conversations/%7BconversationId%7D

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.
Picking an existing conversation leads, then clicking the icon results in the same error
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.
Addressed with b03cd4d
x-pack/plugins/observability_ai_assistant/public/components/chat/chat_header.tsx
Show resolved
Hide resolved
navigateToConversationsApp: (path, ...args) => { | ||
navigateToApp('observabilityAIAssistant', { path, ...args }); | ||
}, |
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.
I suggest keeping the router
api simple and using push
/replace
directly.
navigateToConversationsApp: (path, ...args) => { | |
navigateToApp('observabilityAIAssistant', { path, ...args }); | |
}, |
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.
See earlier comment
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.
In order to do so we have to change push
and replace
to use navigateToApp
:
push: (...args) => {
const next = link(...args);
navigateToApp('observabilityAIAssistant', { path: next, replace: false });
},
replace: (path, ...args) => {
const next = link(path, ...args);
navigateToApp('observabilityAIAssistant', { path: next, replace: true });
},
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.
But then we would also use the navigateToApp method when it is not needed, like when we are in the conversations app.
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.
But then we would also use the navigateToApp method when it is not needed, like when we are in the conversations app.
Is that an actual problem?
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.
We could also have two pairs of push
/replace
methods. One pair for navigating within the app (current ones) and the other pair for navigating from another app into the AI Assistant app. But I am not sure if it's actually necessary.
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.
Not sure if it's an actual problem. FWIW, I always found it slightly odd that Kibana has a separate navigate
and navigateToApp
method. Unsure as to why that is, do you know?
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.
No, I don't either
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.
I've tried using replace
from the existing router API:
replace: (path, ...args) => {
const next = link(path, ...args);
history.replace(next);
},
link: (path, ...args) => {
return http.basePath.prepend('/app/observabilityAIAssistant' + link(path, ...args));
},
it really does not navigate the user to a different app, even though it uses link
with the http.basePath.prepend
etc.
It seems we need a dedicated method when navigating from another app into the conversations app.
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.
Implemented b03cd4d
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.
really nice improvements!
const sanitized = routeParam.replace('{', '').replace('}', ''); | ||
|
||
const pathKey = args[0]?.path; | ||
|
||
if (typeof pathKey !== 'object') { | ||
return; | ||
} | ||
|
||
if (Object.keys(pathKey).length === 0) { | ||
navigateToApp('observabilityAIAssistant', { | ||
path: route, | ||
}); | ||
return; | ||
} | ||
|
||
if (Object.keys(pathKey).length === 1) { | ||
navigateToApp('observabilityAIAssistant', { | ||
// @ts-expect-error | ||
path: `${route}/${pathKey[sanitized]}`, | ||
}); | ||
return; | ||
} |
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.
I really dislike this code, but the combination of this API with the Typescript typing is throwing me for a loop. This works but is not ideal. Better suggestions welcome.
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.
Did you try this?
push: (...args) => {
const next = link(...args);
navigateToApp('observabilityAIAssistant', { path: next, replace: false });
},
Addressed with b03cd4d |
@elasticmachine merge upstream |
💚 Build Succeeded
Metrics [docs]Async chunks
History
To update your PR or re-run it, just comment with: |
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.
Nice change!
Summary
Some small tweaks for the Chat Flyout.
<control>
+<;>
(semicolon) to open the assistant.What it looks like
Screen.Recording.2024-02-06.at.23.25.14.mov