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

[Obs AI Assistant] Show Conversations list in flyout #175226

Conversation

miltonhultgren
Copy link
Contributor

Summary

Changes the ChatFlyout to use EuiResizeablePanel to split the flyout content in two.
On the left we show the Conversations list and this panel can be collapsed to free up space.
On the right we show the ChatBody like before.

Screenshots

Default:
Screenshot 2024-01-22 at 17 20 04

Conversation list made larger:
Screenshot 2024-01-22 at 17 20 14

Conversation list collapsed:
Screenshot 2024-01-22 at 17 20 20

Conversations link moved to three-dot menu:
Screenshot 2024-01-22 at 17 20 28

@miltonhultgren miltonhultgren added the release_note:skip Skip the PR/issue when compiling release notes label Jan 22, 2024
@miltonhultgren miltonhultgren requested a review from a team as a code owner January 22, 2024 16:23
@apmmachine
Copy link
Contributor

🤖 GitHub comments

Expand to view the GitHub comments

Just comment with:

  • /oblt-deploy : Deploy a Kibana instance using the Observability test environments.
  • /oblt-deploy-serverless : Deploy a serverless Kibana instance using the Observability test environments.
  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

hasOtherConversations
? conversations.value!.conversations[0].conversation.id
: undefined
hasOtherConversations ? hasOtherConversations.conversation.id : undefined
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you deleted the first conversation in the list, you'd get a "conversation not found" error.

@@ -161,13 +163,13 @@ export function ChatBody({
}
};

const handleChangeHeight = (editorHeight: number) => {
const handleChangeHeight = useCallback((editorHeight: number) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This caused the render to loop which made focus be put onto the PromptEditor over and over.

@@ -142,6 +142,8 @@ export function PromptEditor({
useEffect(() => {
if (hidden) {
onChangeHeight(0);
} else if (containerRef.current) {
onChangeHeight(containerRef.current.clientHeight);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because I memoized the callback being passed in, this wasn't being called as often due to less renders so had to make sure it's called at least when this flag switches.

The ResizeablePanel causes more renders to happen which caused the focus bug due to missing memoization

@miltonhultgren miltonhultgren changed the title Oaia show conversations in flyout [Obs AI Assistant] Show Conversations list in flyout Jan 22, 2024
@CoenWarmer
Copy link
Contributor

Did a local checkout and have the following feedback:

Screenshot 2024-01-25 at 12 57 08
  • On smaller viewports (1600px wide) the conversation list looks squashed. Two suggestions: add a minimum absolute width for the conversation list, and give the complete flyout a larger width when the conversation list is expanded
  • The padding on the conversation list needs to be a bit smaller so it's in line with the padding on the chat panel
  • The conversation list needs a dividing border between it and the chat
  • The title bar + actions button can be moved up now that the link to 'go to conversations' has been removed. This will reclaim more vertical space for the chat.
  • In the action menu, I would place the link to the conversation view up, so it's closer to all the other links which navigate the user to a different part of kibana.

@miltonhultgren
Copy link
Contributor Author

miltonhultgren commented Jan 29, 2024

  • On smaller viewports (1600px wide) the conversation list looks squashed. Two suggestions: add a minimum absolute width for the conversation list, and give the complete flyout a larger width when the conversation list is expanded
  • The padding on the conversation list needs to be a bit smaller so it's in line with the padding on the chat panel
  • The conversation list needs a dividing border between it and the chat
  • The title bar + actions button can be moved up now that the link to 'go to conversations' has been removed. This will reclaim more vertical space for the chat.
  • In the action menu, I would place the link to the conversation view up, so it's closer to all the other links which navigate the user to a different part of kibana.

It wasn't straightforward to make the conversations list maintain a minimum width due to how the ResizeablePanels work, and the size changing wasn't smooth. We'll need to follow up with better solutions in this area (in collaboration with Design and the EUI team).

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
observabilityAIAssistant 150.2KB 151.7KB +1.4KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
observabilityAIAssistant 14.5KB 14.5KB +14.0B
Unknown metric groups

async chunk count

id before after diff
observabilityAIAssistant 6 7 +1

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@miltonhultgren
Copy link
Contributor Author

As far as I know, the work attempted in this PR was included in #174677

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:skip Skip the PR/issue when compiling release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants