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

Hide the assistant entry when there isn't data2summary agent #417

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
6 changes: 2 additions & 4 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)

## Unreleased


### Features

- introduces Pipeline to execute asynchronous operations ([#376](https://github.com/opensearch-project/dashboards-assistant/pull/376))
Expand All @@ -20,7 +19,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
- Use feature flag to disable the rename conversation api ([#401](https://github.com/opensearch-project/dashboards-assistant/pull/410))
- Disable delete conversation api based on feature flag([#409](https://github.com/opensearch-project/dashboards-assistant/pull/409))
- Add options to hide "stop generation" and regenerate button based on feature flag ([#394](https://github.com/opensearch-project/dashboards-assistant/pull/394))

- Hide the assistant entry when there isn't data2summary agent ([#417](https://github.com/opensearch-project/dashboards-assistant/pull/417))

### Bug Fixes

Expand All @@ -37,6 +36,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
### Maintenance

### Refactoring

- Add query assistant summary to the assistant dropdown list ([#395](https://github.com/opensearch-project/dashboards-assistant/pull/395))
- Update dropdown list button label and remove popover title ([#407](https://github.com/opensearch-project/dashboards-assistant/pull/407))
- Add support for registerMessageParser ([#5](https://github.com/opensearch-project/dashboards-assistant/pull/5))
Expand All @@ -45,5 +45,3 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
- Refactor the code to get root agent id by calling the API in ml-commons plugin ([#128](https://github.com/opensearch-project/dashboards-assistant/pull/128))
- build(deps): bump braces from 3.0.2 to 3.0.3 ([#213](https://github.com/opensearch-project/dashboards-assistant/pull/213))
- build(deps): bump ws from 8.16.0 to 8.18.0 ([#221](https://github.com/opensearch-project/dashboards-assistant/pull/221))


1 change: 1 addition & 0 deletions common/constants/llm.ts
Original file line number Diff line number Diff line change
Expand Up @@ -47,3 +47,4 @@ export const TEXT2VEGA_INPUT_SIZE_LIMIT = 400;
export const TEXT2VEGA_RULE_BASED_AGENT_CONFIG_ID = 'os_text2vega';
export const TEXT2VEGA_WITH_INSTRUCTIONS_AGENT_CONFIG_ID = 'os_text2vega_with_instructions';
export const TEXT2PPL_AGENT_CONFIG_ID = 'os_query_assist_ppl';
export const DATA2SUMMARY_AGENT_CONFIG_ID = 'os_data2summary';
41 changes: 36 additions & 5 deletions public/components/ui_action_context_menu.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -16,15 +16,18 @@ import { i18n } from '@osd/i18n';

import { BehaviorSubject } from 'rxjs';
import { useObservable } from 'react-use';
import { HttpSetup } from '../../../../src/core/public';
import { buildContextMenuForActions } from '../../../../src/plugins/ui_actions/public';
import { AI_ASSISTANT_QUERY_EDITOR_TRIGGER } from '../ui_triggers';
import { getUiActions } from '../services';
import { DataPublicPluginSetup } from '../../../../src/plugins/data/public';

import { AGENT_API, DATA2SUMMARY_AGENT_CONFIG_ID } from '../../common/constants/llm';
interface Props {
data: DataPublicPluginSetup;
isQuerySummaryCollapsed$: BehaviorSubject<boolean>;
resultSummaryEnabled$: BehaviorSubject<boolean>;
isSummaryAgentAvailable$: BehaviorSubject<boolean>;
httpSetup: HttpSetup;
label?: string;
}

Expand All @@ -39,6 +42,35 @@ export const ActionContextMenu = (props: Props) => {
});
const resultSummaryEnabled = useObservable(props.resultSummaryEnabled$, false);
const isQuerySummaryCollapsed = useObservable(props.isQuerySummaryCollapsed$, false);
const isSummaryAgentAvailable = useObservable(props.isSummaryAgentAvailable$, false);

async function checkAgentsExist(
http: HttpSetup,
agentConfigName: string | string[],
dataSourceId?: string
) {
const response = await http.get(AGENT_API.CONFIG_EXISTS, {
query: { agentConfigName, dataSourceId },
});
return response;
}
Copy link
Member

Choose a reason for hiding this comment

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

We should move this function out of render function.


useEffect(() => {
const fetchSummaryAgent = async () => {
try {
const summaryAgentStatus = await checkAgentsExist(
Copy link
Member

@SuZhou-Joe SuZhou-Joe Jan 27, 2025

Choose a reason for hiding this comment

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

Do we need to call props.isSummaryAgentAvailable$.next(false) before making the call to check if agent exists? if we switch from data source A(agent configured) to data source B (agent not configured) and the call somehow failed, the status will be incorrect.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, it's import, sorry I forget it., updated~

props.httpSetup,
DATA2SUMMARY_AGENT_CONFIG_ID,
props.data.query.queryString.getQuery().dataset?.dataSource?.id
);
props.isSummaryAgentAvailable$.next(!!summaryAgentStatus.exists);
} catch (error) {
console.error(error);
}
};

fetchSummaryAgent();
}, [props, props.httpSetup]);
Copy link
Member

@SuZhou-Joe SuZhou-Joe Jan 27, 2025

Choose a reason for hiding this comment

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

We should listen to dataSourceId change, listening to props change is too generic. And if the resultSummaryEnabled is false, we do not need to check if agents exists.


useEffect(() => {
const subscription = props.data.query.queryString.getUpdates$().subscribe((query) => {
Expand Down Expand Up @@ -75,14 +107,13 @@ export const ActionContextMenu = (props: Props) => {
[actionContext.datasetId, actionContext.datasetType, actionContext.dataSourceId]
);

// The action button should be not displayed when there is no action and result summary disabled.
if (actionsRef.current.length === 0 && !resultSummaryEnabled) {
// The action button should be not displayed when there is no action and result summary disabled or there is no data2Summary agent
if (!isSummaryAgentAvailable || (actionsRef.current.length === 0 && !resultSummaryEnabled)) {
Copy link
Member

@SuZhou-Joe SuZhou-Joe Jan 27, 2025

Choose a reason for hiding this comment

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

Nit: I'd recommend moving isSummaryAgentAvailable behind !resultSummaryEnabled because we need to make a network request to get the status.

return null;
}

// The action button should be disabled when context menu has no item and result summary disabled.
// The action button should be disabled when context menu has no item and result summary disabled
const actionDisabled = (panels.value?.[0]?.items ?? []).length === 0 && !resultSummaryEnabled;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const actionDisabled = (panels.value?.[0]?.items ?? []).length === 0 && !resultSummaryEnabled;
const actionDisabled = (panels.value?.[0]?.items ?? []).length === 0 && !resultSummaryEnabled && !isSummaryAgentAvailable;

Do we need to add check on isSummaryAgentAvailable?

Copy link
Contributor Author

@Qxisylolo Qxisylolo Jan 27, 2025

Choose a reason for hiding this comment

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

I add this check at first, but if there isn't agent, should we hide the button instead of disabling it?

Copy link
Member

Choose a reason for hiding this comment

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

I think it is OK that we check on both places, but sure it is good enough if we will hide the button if no summary agent is available on target data source.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

update~ and thanks for all the comments


return (
<EuiPopover
button={
Expand Down
8 changes: 7 additions & 1 deletion public/plugin.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -282,7 +282,11 @@ export class AssistantPlugin
}
})();

const { isQuerySummaryCollapsed$, resultSummaryEnabled$ } = setupDeps.queryEnhancements;
const {
isQuerySummaryCollapsed$,
resultSummaryEnabled$,
isSummaryAgentAvailable$,
Qxisylolo marked this conversation as resolved.
Show resolved Hide resolved
} = setupDeps.queryEnhancements;
setupDeps.data.__enhance({
editor: {
queryEditorExtension: {
Expand All @@ -292,9 +296,11 @@ export class AssistantPlugin
getSearchBarButton: () => {
return (
<ActionContextMenu
httpSetup={core.http}
label={this.config.branding.label}
isQuerySummaryCollapsed$={isQuerySummaryCollapsed$}
resultSummaryEnabled$={resultSummaryEnabled$}
isSummaryAgentAvailable$={isSummaryAgentAvailable$}
data={setupDeps.data}
/>
);
Expand Down
Loading