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.

Sign up for GitHub

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

Jump to bottom

[Security Solution] AI settings #184678

Merged
merged 42 commits into from
Jun 28, 2024
Merged

[Security Solution] AI settings #184678

merged 42 commits into from
Jun 28, 2024

Conversation

angorayc
Copy link
Contributor

@angorayc angorayc commented Jun 3, 2024 •
edited
Loading

Summary

https://github.com/elastic/security-team/issues/9222
Screenshot 2024-06-23 at 11 30 15
Screenshot 2024-06-23 at 11 30 54
Screenshot 2024-06-23 at 11 37 11
Screenshot 2024-06-25 at 13 36 19

Screenshot 2024-06-25 at 13 33 59 Screenshot 2024-06-23 at 11 40 31 Screenshot 2024-06-23 at 11 41 23

Knowledge base:
#186847

Checklist

Delete any items that are not applicable to this PR.

angorayc
angorayc commented Jun 19, 2024
View reviewed changes
...ssistant/impl/assistant/conversations/conversation_settings/conversation_settings_editor.tsx
};
setConversationSettings({
...conversationSettings,
[updatedConversation.id || updatedConversation.title]: updatedConversation,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sometimes id can be an empty string:
additional conversation

@angorayc angorayc changed the title connectors tab AI settings Jun 19, 2024
angorayc
angorayc commented Jun 19, 2024
View reviewed changes
x-pack/packages/kbn-elastic-assistant/impl/assistant/settings/assistant_settings_management.tsx
setSelectedConversation(
// conversationSettings has title as key, sometime has id as key
conversationSettings[selectedConversation.id] ||
conversationSettings[selectedConversation.title]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@YulNaumenko Is this the do with the existing issue?
conversationSettings has title as key, sometime has id as key

angorayc
angorayc commented Jun 21, 2024
View reviewed changes
...ic-assistant/impl/assistant/conversations/conversation_settings/use_conversation_changed.tsx
Comment on lines +88 to +91
onSelectedConversationChange({
...newSelectedConversation,
id: newSelectedConversation.id || newSelectedConversation.title,
});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

id can be title sometimes.

Copy link
Contributor Author

@angorayc angorayc Jun 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Before

Screen.Recording.2024-06-17.at.11.19.17.mov

After

Screen.Recording.2024-06-21.at.21.19.50.mov

Copy link
Member

Choose a reason for hiding this comment

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

@YulNaumenko, thoughts on persisting all the base conversations on initial conversation request that way we can get rid of all this extra title vs id logic? I don't like side-effects on a GET, but can't think of a different way of handling this than what we're doing now, which has been a source of bugs requiring conditional checks like this.

angorayc
angorayc commented Jun 21, 2024
View reviewed changes
...astic-assistant/impl/assistant/conversations/conversation_settings/conversation_settings.tsx
http={http}
isDisabled={isDisabled}
isFlyoutMode={isFlyoutMode}
selectedConversation={selectedConversationWithApiConfig}
Copy link
Contributor Author

@angorayc angorayc Jun 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Adding api config to the selected conversation to fix the scenario: conversation not updated when changing a prompt.
In this video, I updated data quality dashboard conversation with system prompt xxx, but it wasn't saved.

Screen.Recording.2024-06-21.at.22.29.19.mov

@angorayc
Copy link
Contributor Author

buildkite test this

@angorayc
Copy link
Contributor Author

buildkite test this

@angorayc
Copy link
Contributor Author

buildkite test this

@angorayc
Copy link
Contributor Author

buildkite test this

@angorayc
Copy link
Contributor Author

/ci

@angorayc angorayc mentioned this pull request Jun 24, 2024
Draft
10 tasks
spong
spong reviewed Jun 27, 2024
View reviewed changes
...bn-elastic-assistant/impl/assistant/conversations/conversation_settings_management/index.tsx
Copy link
Member

Choose a reason for hiding this comment

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

@bojanasan, any reason we didn't make the Conversation Title editable in the flyout?

Right now it's just used as the flyout title and not an actual editable field, so the only way to change the title is to select the conversation in the assistant and then click the title.

Copy link

Choose a reason for hiding this comment

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

@spong I wasn't sure if it was helpful to have it in the table because I'm expecting that the workflow of defining the prompt would be happening in the AI Chat UI (during conversation) as the user is testing the prompt. Additionally the length would impact row height significantly. Do you think that the user will be needing to see the description here? Relevant figma comment

Copy link
Member

Choose a reason for hiding this comment

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

Was your comment meant for the other thread linked in figma? (I responded there as well).

This is about making the Title of the conversation editable in the conversation details flyout.

spong
spong reviewed Jun 27, 2024
View reviewed changes
...nt/prompt_editor/system_prompt/system_prompt_settings_management/use_system_prompt_table.tsx Outdated
Comment on lines 76 to 81
<RowActions<SystemPromptTableItem>
rowItem={prompt}
onEdit={onEditActionClicked}
onDelete={isDeletable ? onDeleteActionClicked : undefined}
isDeletable={isDeletable}
/>
Copy link
Member

Choose a reason for hiding this comment

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

As with the other comment on actions, should we just show them since there are only ever two instead of adding another click into the popover?

angorayc
angorayc commented Jun 27, 2024
View reviewed changes
...stant/impl/assistant/prompt_editor/system_prompt/system_prompt_settings_management/index.tsx Outdated
const pagination = useMemo(
() => ({
pageIndex: DEFAULT_PAGE_INDEX,
pageSize: DEFAULT_PAGE_SIZE,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In quick prompts table and system prompts table, I checked with @bojanasan to give it a bigger default page size as currently new item is added to the bottom of the list and we do not have timestamp to sort on.
Screenshot 2024-06-27 at 20 59 30

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
integrationAssistant 514 525 +11
securitySolution 5535 5565 +30
total +41

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
@kbn/elastic-assistant 138 139 +1

Async chunks

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

id before after diff
aiAssistantManagementSelection 44.6KB 45.4KB +855.0B
integrationAssistant 844.0KB 844.3KB +230.0B
securitySolution 13.4MB 13.6MB +144.6KB
total +145.6KB

Page load bundle

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

id before after diff
securitySolution 83.4KB 83.4KB +8.0B
Unknown metric groups

API count

id before after diff
@kbn/elastic-assistant 165 166 +1

async chunk count

id before after diff
securitySolution 98 99 +1

History

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

spong
spong approved these changes Jun 28, 2024
View reviewed changes
Copy link
Member

@spong spong left a comment

Choose a reason for hiding this comment

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

Checked out, tested locally, and performed a high-level code review -- LGTM!

Thank you so much @angorayc for all your efforts here. It has really come together in unifying our Assistant Settings views 🙂

Looks like you've addressed most of my comments, with the remainder being mostly design related, which I think we can iterate on going forward, so looks good to me! Thanks again!

@angorayc angorayc merged commit 72e1d11 into elastic:main Jun 28, 2024
36 checks passed
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Jun 28, 2024
@angorayc angorayc mentioned this pull request Jul 8, 2024
Open
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@peluja1012 peluja1012 peluja1012 left review comments

@bojanasan bojanasan bojanasan left review comments

@spong spong spong approved these changes

@sorenlouv sorenlouv sorenlouv approved these changes

@kapral18 kapral18 kapral18 approved these changes

@jpdjere jpdjere jpdjere approved these changes

@semd semd semd approved these changes

Assignees
No one assigned
Labels
backport:skip This commit does not require backporting release_note:enhancement Team:Security Generative AI Security Generative AI v8.15.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
10 participants
@angorayc @kibana-ci @sorenlouv @peluja1012 @kapral18 @bojanasan @spong @jpdjere @semd @kibanamachine