Skip to content

[9.0][Security Assistant] Add space id provider #225033

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

Merged
merged 7 commits into from
Jun 25, 2025
Merged

[9.0][Security Assistant] Add space id provider #225033

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
077d05d
add space id provider
angorayc Jun 24, 2025
53e3255
unit tests
angorayc Jun 24, 2025
9db92d6
[CI] Auto-commit changed files from 'node scripts/eslint --no-cache -…
kibanamachine Jun 24, 2025
cb16778
update unit tests
angorayc Jun 25, 2025
829f0b8
unit tests
angorayc Jun 25, 2025
856f7b1
unit tests
angorayc Jun 25, 2025
270b0cb
align code with main
angorayc Jun 25, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 19 additions & 1 deletion .../plugins/security_solution/public/assistant/stack_management/management_settings.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import {
import { useKibana } from '../../common/lib/kibana';
import { useConversation } from '@kbn/elastic-assistant/impl/assistant/use_conversation';
import { QueryClient, QueryClientProvider } from '@tanstack/react-query';
import { useSpaceId } from '../../common/hooks/use_space_id';

// Mock the necessary hooks and components
jest.mock('@kbn/elastic-assistant', () => ({
Expand All @@ -35,11 +36,15 @@ jest.mock('@kbn/elastic-assistant/impl/assistant/use_conversation', () => ({
jest.mock('../../common/lib/kibana', () => ({
useKibana: jest.fn(),
}));
jest.mock('../../common/hooks/use_space_id', () => ({
Copy link
Member

@KDKHD KDKHD Jun 25, 2025

Choose a reason for hiding this comment

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

Is it possible to modify this test file so that it does not mock AssistantSettingsManagement? Alternatively, it would be great if we could add a test that would fail if is removed by accident in the future.

Copy link
Contributor

@e40pud e40pud Jun 25, 2025

Choose a reason for hiding this comment

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

I guess we could try to do that by wrapping assistant settings management component into the AssistantSpaceIdProvider within the plugin setup if we have access to active space id from there https://github.com/elastic/kibana/blob/main/x-pack/solutions/security/plugins/security_solution/public/plugin.tsx#L214. This way we would avoid using useSpaceId hook completely.

But if that is too much work (+ I guess this is an urgent fix), I think we can also go with the current solution and update it later if needed.

Copy link
Contributor Author

@angorayc angorayc Jun 25, 2025

Choose a reason for hiding this comment

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

In the implementation on the main branch:

AssistantSpaceIdProvider was introduced but was accidentally dropped during its 9.0 backport
x-pack/solutions/security/plugins/security_solution/public/assistant/stack_management/management_settings.tsx:

https://github.com/elastic/kibana/pull/221566/files#diff-09e012f07b9ef5bd3559d3e05261436872407428a0fa8d1e2b4651b8f887cabc

I applied the same fix for its test file but some tests failed, I'm still trying to figure out:
x-pack/solutions/security/plugins/security_solution/public/assistant/stack_management/management_settings.test.tsx:
https://github.com/elastic/kibana/pull/221566/files#diff-14290c240aafc6f614c221dbde3b575f627f0ab8484b7186ed4d8e029fc9c3b7

Copy link
Contributor

@e40pud e40pud Jun 25, 2025

Choose a reason for hiding this comment

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

Sounds good! Thank you for the fix - tested locally and it worked. Will leave approval to @KDKHD

Copy link
Contributor Author

@angorayc angorayc Jun 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Found another line dropped during the backport: https://github.com/elastic/kibana/pull/221566/files#diff-09e012f07b9ef5bd3559d3e05261436872407428a0fa8d1e2b4651b8f887cabcR16

I'd keep AssistantSettingsManagement to align with the code with the main branch:
https://github.com/elastic/kibana/pull/221566/files#diff-14290c240aafc6f614c221dbde3b575f627f0ab8484b7186ed4d8e029fc9c3b7

and added scenarios for both space id is available and unavailable.

I'll also re-think the error display for the AssistantSpaceIdProvider and try not to let it blocks the UI (I'll do the enhancement in a new PR and do the code change from the main branch).

useSpaceId: jest.fn(),
}));

const useAssistantContextMock = useAssistantContext as jest.Mock;
const useFetchCurrentUserConversationsMock = useFetchCurrentUserConversations as jest.Mock;
const useKibanaMock = useKibana as jest.Mock;
const useConversationMock = useConversation as jest.Mock;
const useSpaceIdMock = useSpaceId as jest.Mock;

describe('ManagementSettings', () => {
const queryClient = new QueryClient();
Expand All @@ -55,9 +60,11 @@ describe('ManagementSettings', () => {
const renderComponent = ({
isAssistantEnabled = true,
conversations,
spaceId,
}: {
isAssistantEnabled?: boolean;
conversations: Record<string, Conversation>;
spaceId?: string;
}) => {
useAssistantContextMock.mockReturnValue({
baseConversations,
Expand Down Expand Up @@ -101,6 +108,8 @@ describe('ManagementSettings', () => {
getDefaultConversation,
});

useSpaceIdMock.mockReturnValue(spaceId);

return render(
<MemoryRouter>
<QueryClientProvider client={queryClient}>
Expand All @@ -117,14 +126,23 @@ describe('ManagementSettings', () => {
it('navigates to home if securityAIAssistant is disabled', () => {
renderComponent({
conversations: mockConversations,
spaceId: 'default',
});
expect(navigateToApp).toHaveBeenCalledWith('home');
});

it('renders AssistantSettingsManagement when conversations are available and securityAIAssistant is enabled', () => {
it('renders AssistantSettingsManagement when spaceId, conversations are available and securityAIAssistant is enabled', () => {
renderComponent({
conversations: mockConversations,
spaceId: 'default',
});
expect(screen.getByTestId('AssistantSettingsManagement')).toBeInTheDocument();
});

it('does not render AssistantSettingsManagement when spaceId is not available', () => {
renderComponent({
conversations: mockConversations,
});
expect(screen.queryByTestId('AssistantSettingsManagement')).not.toBeInTheDocument();
});
});
22 changes: 13 additions & 9 deletions ...urity/plugins/security_solution/public/assistant/stack_management/management_settings.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,15 @@ import {
useFetchCurrentUserConversations,
WELCOME_CONVERSATION_TITLE,
} from '@kbn/elastic-assistant';
import { AssistantSpaceIdProvider } from '@kbn/elastic-assistant/impl/assistant/use_space_aware_context';
import { useConversation } from '@kbn/elastic-assistant/impl/assistant/use_conversation';
import type { FetchConversationsResponse } from '@kbn/elastic-assistant/impl/assistant/api';
import { SECURITY_AI_SETTINGS } from '@kbn/elastic-assistant/impl/assistant/settings/translations';
import { CONVERSATIONS_TAB } from '@kbn/elastic-assistant/impl/assistant/settings/const';
import type { SettingsTabs } from '@kbn/elastic-assistant/impl/assistant/settings/types';

import { useKibana } from '../../common/lib/kibana';
import { useSpaceId } from '../../common/hooks/use_space_id';

const defaultSelectedConversationId = WELCOME_CONVERSATION_TITLE;

Expand All @@ -32,7 +34,7 @@ export const ManagementSettings = React.memo(() => {
http,
assistantAvailability: { isAssistantEnabled },
} = useAssistantContext();

const spaceId = useSpaceId();
const {
application: {
navigateToApp,
Expand Down Expand Up @@ -131,14 +133,16 @@ export const ManagementSettings = React.memo(() => {
}

if (conversations) {
return (
<AssistantSettingsManagement
selectedConversation={currentConversation}
dataViews={dataViews}
onTabChange={handleTabChange}
currentTab={currentTab}
/>
);
return spaceId ? (
<AssistantSpaceIdProvider spaceId={spaceId}>
<AssistantSettingsManagement
selectedConversation={currentConversation}
dataViews={dataViews}
onTabChange={handleTabChange}
currentTab={currentTab}
/>
</AssistantSpaceIdProvider>
) : null;
}

return <></>;
Expand Down