[Security solution] Assistant package `assistant/index.tsx` cleanup #190151

stephmilovic · 2024-08-08T14:54:25Z

Summary

Fixes some ugly code in x-pack/packages/kbn-elastic-assistant/impl/assistant/index.tsx.

Extracts existing blocks from assistant/index.tsx into components and hooks.
Removes duplicate calls to createConversations.
Renames variables
Eliminates meaningless wrapper functions
Removes unused const

The cleanup fixes state side effects and a variety of bugs. See Github comments in code for more details.

To test

Cypress tests are coming for basic assistant behavior. As of time of review request, I have the following behaviors covered in Cypress. I will continue to add more Cypress tests until that PR is ready for review
When no connectors or conversations exist
- Shows welcome setup when no connectors or conversations exist
When no conversations exist but connectors do exist, show empty convo
- When invoked on Al Assistant click, ensure system prompt is selected
- When invoked from rules page, ensure proper prompt context and user prompt
- When invoked from alert details, ensure proper prompt context and user prompt
- Shows empty connector
  callout when a conversation that had a connector no longer does
Changing conversations
- Last conversation persists in memory from page to page
- Properly switches back and forth between conversations
- Only allows one conversation called "New chat" at a time
Manual test steps are a bit vague as root behavior here is not changing. Just a general kicking of the tires
- start with fresh es and kibana instances, make sure you do not have any preconfigured connectors
- optionally, run through the scenarios above
- open new conversation, set up a connector, change conversations, change connectors, update title, send messages, update prompts (system and user)
- check assistant loading state
- ensure all conversation behavior still works as it did before

If you find a bug, please check if it exists on main as well.

stephmilovic · 2024-08-08T14:58:05Z

x-pack/packages/kbn-elastic-assistant/impl/assistant/index.tsx

-  const isWelcomeSetup = useMemo(() => {
-    // if any conversation has a connector id, we're not in welcome set up
-    return Object.keys(conversations).some(
-      (conversation) => conversations[conversation]?.apiConfig?.connectorId != null


This was causing a bug, we also need to check that the connector ID is not empty string.

Before:

after:

stephmilovic · 2024-08-08T14:59:15Z

x-pack/packages/kbn-elastic-assistant/impl/assistant/index.tsx

+    if (
+      !isLoadingCurrentUserConversations &&
+      isFetchedConnectors &&
+      currentConversation &&


Adding this condition skips a flash of the missing connector callout when the current conversation is still loading.

x-pack/packages/kbn-elastic-assistant/impl/assistant/use_current_conversation/index.tsx

stephmilovic · 2024-08-08T20:36:27Z

...elastic-assistant/impl/assistant/quick_prompts/quick_prompt_settings/quick_prompt_editor.tsx

+  const setDefaultPromptColor = useCallback((): string => {
+    const randomColor = getRandomEuiColor();
+    handleColorChange(randomColor, { hex: randomColor, isValid: true });
+    return randomColor;
+  }, [handleColorChange]);


I've been wanting to do this for a while now, random EuiColors for quick prompt colors:

color.mov

stephmilovic · 2024-08-08T21:24:57Z

x-pack/packages/kbn-elastic-assistant/impl/assistant/chat_send/use_chat_send.tsx

+    await refetchCurrentUserConversations();
+  }, [onChatCleared, refetchCurrentUserConversations]);
+
+  const handleChatSend = useCallback(


handleChatSend previously existed in assistant/index.tsx. As you can see, it calls handleSendMessage. If you check the old assistant/index.tsx, later on in the components handleChatSend gets assigned to handleSendMessage (handleSendMessage={handleChatSend})

stephmilovic · 2024-08-08T21:25:50Z

x-pack/packages/kbn-elastic-assistant/impl/assistant/chat_send/use_chat_send.tsx


  const { isLoading, sendMessage, abortStream } = useSendMessage();
  const { clearConversation, removeLastMessage } = useConversation();

-  const handlePromptChange = (prompt: string) => {


handlePromptChange was just a wrapper on top of setUserPrompt, so it was eliminated.

stephmilovic · 2024-08-08T21:29:59Z

x-pack/packages/kbn-elastic-assistant/impl/assistant/index.tsx

-
-  // Welcome conversation is a special 'setup' case when no connector exists, mostly extracted to `ConnectorSetup` component,
-  // but currently a bit of state is littered throughout the assistant component. TODO: clean up/isolate this state
-  const blockBotConversation = useMemo(


All this block bot stuff is now UpgradeLicenseCallToAction. All of this type of logic (what to show in the body: upgrade cta, welcome setup, empty convo, or messages) is now in assistant_body/index.tsx

stephmilovic · 2024-08-08T21:31:01Z

x-pack/packages/kbn-elastic-assistant/impl/assistant/index.tsx

-  );
-
-  useEffect(() => {
-    if (areConnectorsFetched && conversationsLoaded && Object.keys(conversations).length > 0) {


This useEffect has relocated to use_current_conversation/index.tsx and is simplified

stephmilovic · 2024-08-08T22:05:43Z

x-pack/packages/kbn-elastic-assistant/impl/assistant/index.tsx

-    }
-  );
-
-  useEffect(() => {


this useEffect has been eliminated entirely. Now there is a function, initializeDefaultConversationWithConnector that handles when a default conversation without a connector is initialized with the default connector. If necessary, initializeDefaultConversationWithConnector is called on conversation change/update.

stephmilovic · 2024-08-13T17:11:12Z

x-pack/packages/kbn-elastic-assistant/impl/assistant/index.tsx

+      Object.keys(conversations).length > 0,
+  });
+
+  const isInitialLoad = useMemo(() => {


This fixes the loading behavior. Notice 3 flashes of incorrect content in the before, and loading indicators in the after.

Before:

loading-bad.mov

After:

load-good.mov

@spong I'm considering using the AssistantAnimatedIcon for the assistant body loading view, but since it is not available in Eui yet, I cannot get that same bounce effect from EuiLoadingLogo. Which do you prefer?

// proper loading component <EuiEmptyPrompt icon={<EuiLoadingLogo logo="logoSecurity" size="xl" />} /> // option with AssistantAnimatedIcon <EuiEmptyPrompt icon={<AssistantAnimatedIcon />} />

option2.mov

Might be best for us to confer with @bojanasan & design here since they're handling all the alignment stuff. This is just on initial load as conversations/prompts and all that get set up, right? If we don't have to gate the full UI with a loader I think that would be best UX (and less jitter), but if not what you showed in option2 above looks good to me.

Ok i sent this thread to @bojanasan

Re: jitter. Without a loading option, there are 3 views shown before showing the proper view:

With the loading UI, there are 2 views: loading and the proper view. No confusion as to if what you're seeing is correct

@stephmilovic I think the loading state with the AI logo works well. I think it looks smoother than the other option, and its nice to use the existing AI icon animation. Thanks for noticing the weird content flashes!

Quick question, it would be great if the body content was centered vertically. Is it off right now because of the browser page height?

yeah its centered its just that the logo has a lot of padding to do that animation around it. this is also a pretty short browser window

stephmilovic · 2024-08-13T19:14:13Z

@elasticmachine merge upstream

...stic-assistant/impl/assistant/api/knowledge_base/entries/use_create_knowledge_base_entry.tsx

spong · 2024-08-15T21:30:54Z

x-pack/packages/kbn-elastic-assistant/impl/assistant/assistant_overlay/index.tsx

+  // Why is this named Title and not Id?
+  const [conversationTitle, setConversationTitle] = useState<string | undefined>(undefined);


That's a a good question...probably leftover from before persistence when title was id? I would say go ahead and rename this, let's get as far away from conflating id/title as we can, as that's been problematic/messy as of late.

spong · 2024-08-15T21:41:44Z

.../packages/kbn-elastic-assistant/impl/assistant/conversations/conversation_selector/index.tsx

Was this just not being used now? Was searching for usages of ConversationSelector and didn't see any, so maybe this was removed awhile back with the settings overhaul?

I am trying to remember why there ended up being multiple of these, but that escapes me now, so it's good we're down-sizing here 🙂

spong · 2024-08-15T22:56:43Z

If there's already messages in a pre-defined chat the prompt is not getting pre-populated

No messages:

With previous message:

edit: Confirmed this is also pre-existing on main

spong · 2024-08-15T23:00:32Z

This might've been pre-existing, but when viewing an attack discovery in the assistant, if the alerts tab is open, the attack discovery prompt context is not available:

CleanShot.2024-08-15.at.16.58.38.mp4

edit: this is pre-existing and reproducible on main

spong · 2024-08-15T23:24:24Z

When connector privileges are missing we don't have any specific gating and the user can't do anything when trying to add a connector:

CleanShot.2024-08-15.at.17.22.47.mp4

edit: this is partially pre-existing -- we have a badge saying there are missing privileges, but can still see the same UX when trying to add a connector:

spong

Checked out, code reviewed, and did a buncha desk testing -- LGTM! 👍

Thank you for all the cleanup here @stephmilovic, you've made a world of difference with this refactor!

Note: I did find a couple small bugs around privileges/pre-defined chats (see comments above) and have verified they were pre-existing. Approving now though as everything else looks good. If you end up fixing them here let me know and I'll re-test and verify.

kibana-ci · 2024-08-20T07:56:13Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: 1382cd4

Failed CI Steps

FTR Configs #74

Test Failures

[job] [logs] FTR Configs #74 / Visualizations - Group 3 lens app - TSVB Open in Lens Dashboard to TSVB to Lens should convert a by reference TSVB viz to a Lens viz

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`integrationAssistant`	540	546	+6
`securitySolution`	5653	5659	+6
total			+12

Async chunks

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

id	before	after	diff
`integrationAssistant`	937.3KB	936.7KB	-679.0B
`securitySolution`	20.7MB	18.0MB	-2.7MB
total			-2.7MB

Page load bundle

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

id	before	after	diff
`securitySolution`	86.2KB	86.2KB	-8.0B

Unknown metric groups

ESLint disabled line counts

id	before	after	diff
`@kbn/elastic-assistant`	16	15	-1

Total ESLint disabled count

id	before	after	diff
`@kbn/elastic-assistant`	17	16	-1

History

💚 Build #228045 succeeded c81feb3
💛 Build #227995 was flaky 405968d
💚 Build #227704 succeeded c1edeec
💛 Build #227468 was flaky 486c828
💔 Build #227460 failed 7926944
💔 Build #227445 failed d230696

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

stephmilovic · 2024-08-27T13:51:14Z

💚 All backports created successfully

Status	Branch	Result
✅	8.15

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

…lastic#190151) (cherry picked from commit 730f8ea) # Conflicts: # x-pack/plugins/translations/translations/fr-FR.json # x-pack/plugins/translations/translations/ja-JP.json # x-pack/plugins/translations/translations/zh-CN.json

…eanup (#190151) (#191501) # Backport This will backport the following commits from `main` to `8.15`: - [[Security solution] Assistant package `assistant/index.tsx` cleanup (#190151)](#190151)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)

stephmilovic added 11 commits August 5, 2024 12:36

cleanup welcome convo

9172fe5

more wip

3f133fa

more

00aca9f

Merge branch 'main' into assistant_package_cleanup

6dc33a8

Merge branch 'main' into assistant_package_cleanup

67e4877

Merge branch 'main' into assistant_package_cleanup

aacc48d

it practically works

476c353

update

de63956

fix type

ed77db6

fix useEffect

03a1825

another great fix

8294f72

stephmilovic added release_note:skip Skip the PR/issue when compiling release notes Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:Security Generative AI Security Generative AI v8.16.0 labels Aug 8, 2024

stephmilovic commented Aug 8, 2024

View reviewed changes

fix tests

952e0c8

stephmilovic commented Aug 8, 2024

View reviewed changes

x-pack/packages/kbn-elastic-assistant/impl/assistant/use_current_conversation/index.tsx Outdated Show resolved Hide resolved

stephmilovic added 5 commits August 8, 2024 11:23

add test

7ba2f75

remove unused files

d169910

fixes

7814717

update quick prompt color

eabd4b4

fix test

65ef9f4

stephmilovic commented Aug 8, 2024

View reviewed changes

stephmilovic mentioned this pull request Aug 13, 2024

Assistant cypress stephmilovic/kibana#18

Closed

stephmilovic added 2 commits August 13, 2024 09:50

Merge branch 'main' into assistant_package_cleanup

6905d68

fix loading

d230696

stephmilovic commented Aug 13, 2024

View reviewed changes

fix connector icons

7926944

elasticmachine and others added 5 commits August 14, 2024 05:14

Merge branch 'main' into assistant_package_cleanup

486c828

fix system prompt on chat clear

dac74f1

Merge branch 'main' into assistant_package_cleanup

ba548b5

update loading icon

c1edeec

fix basic license

405968d

spong reviewed Aug 15, 2024

View reviewed changes

...stic-assistant/impl/assistant/api/knowledge_base/entries/use_create_knowledge_base_entry.tsx Outdated Show resolved Hide resolved

stephmilovic added 2 commits August 15, 2024 15:24

revert entries delete

6a67d87

x

c81feb3

spong reviewed Aug 15, 2024

View reviewed changes

spong approved these changes Aug 15, 2024

View reviewed changes

Merge branch 'main' into assistant_package_cleanup

1382cd4

stephmilovic merged commit 730f8ea into elastic:main Aug 21, 2024
36 checks passed

kibanamachine added the backport:skip This commit does not require backporting label Aug 21, 2024

stephmilovic mentioned this pull request Aug 27, 2024

[8.15] [Security solution] Assistant package assistant/index.tsx cleanup (#190151) #191501

Merged

kibanamachine added the v8.15.1 label Aug 27, 2024

stephmilovic mentioned this pull request Sep 9, 2024

[Security solution] Attack Discovery "View in AI Assistant" button fix #192416

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security solution] Assistant package `assistant/index.tsx` cleanup #190151

[Security solution] Assistant package `assistant/index.tsx` cleanup #190151

stephmilovic commented Aug 8, 2024 •

edited

Loading

stephmilovic Aug 8, 2024

stephmilovic Aug 8, 2024

stephmilovic Aug 8, 2024

stephmilovic Aug 8, 2024

stephmilovic Aug 8, 2024

stephmilovic Aug 8, 2024

stephmilovic Aug 8, 2024 •

edited

Loading

stephmilovic Aug 8, 2024

stephmilovic Aug 13, 2024

stephmilovic Aug 14, 2024

spong Aug 14, 2024

stephmilovic Aug 14, 2024

stephmilovic Aug 14, 2024

bojanasan Aug 14, 2024 •

edited

Loading

stephmilovic Aug 14, 2024

stephmilovic Aug 14, 2024

stephmilovic commented Aug 13, 2024

spong Aug 15, 2024

spong Aug 15, 2024

spong commented Aug 15, 2024 •

edited

Loading

spong commented Aug 15, 2024 •

edited

Loading

spong commented Aug 15, 2024 •

edited

Loading

spong left a comment •

edited

Loading

kibana-ci commented Aug 20, 2024

ESLint disabled line counts

Total ESLint disabled count

stephmilovic commented Aug 27, 2024

		// Why is this named Title and not Id?
		const [conversationTitle, setConversationTitle] = useState<string \| undefined>(undefined);

[Security solution] Assistant package assistant/index.tsx cleanup #190151

[Security solution] Assistant package assistant/index.tsx cleanup #190151

Conversation

stephmilovic commented Aug 8, 2024 • edited Loading

Summary

To test

When no connectors or conversations exist

When no conversations exist but connectors do exist, show empty convo

Changing conversations

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephmilovic Aug 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bojanasan Aug 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephmilovic commented Aug 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spong commented Aug 15, 2024 • edited Loading

spong commented Aug 15, 2024 • edited Loading

spong commented Aug 15, 2024 • edited Loading

spong left a comment • edited Loading

Choose a reason for hiding this comment

kibana-ci commented Aug 20, 2024

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

Metrics [docs]

Module Count

Async chunks

Page load bundle

ESLint disabled line counts

Total ESLint disabled count

History

stephmilovic commented Aug 27, 2024

💚 All backports created successfully

Questions ?

[Security solution] Assistant package `assistant/index.tsx` cleanup #190151

[Security solution] Assistant package `assistant/index.tsx` cleanup #190151

stephmilovic commented Aug 8, 2024 •

edited

Loading

stephmilovic Aug 8, 2024 •

edited

Loading

bojanasan Aug 14, 2024 •

edited

Loading

spong commented Aug 15, 2024 •

edited

Loading

spong commented Aug 15, 2024 •

edited

Loading

spong commented Aug 15, 2024 •

edited

Loading

spong left a comment •

edited

Loading