Hide the assistant entry when there isn't data2summary agent #417
Conversation
Signed-off-by: Qxisylolo <[email protected]>
Qxisylolo
requested review from
lezzago,
joshuali925,
anirudha,
ruanyl,
SuZhou-Joe,
xluo-aws,
gaobinlong,
Hailong-am,
wanglam and
raintygao
as code owners
Signed-off-by: Qxisylolo <[email protected]>
| }; | ||
|
|
||
| fetchSummaryAgent(); | ||
| }, [props, props.httpSetup]); |
There was a problem hiding this comment.
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.
| // 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)) { |
There was a problem hiding this comment.
Nit: I'd recommend moving isSummaryAgentAvailable behind !resultSummaryEnabled because we need to make a network request to get the status.
| async function checkAgentsExist( | ||
| http: HttpSetup, | ||
| agentConfigName: string | string[], | ||
| dataSourceId?: string | ||
| ) { | ||
| const response = await http.get(AGENT_API.CONFIG_EXISTS, { | ||
| query: { agentConfigName, dataSourceId }, | ||
| }); | ||
| return response; | ||
| } |
There was a problem hiding this comment.
We should move this function out of render function.
| 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; |
There was a problem hiding this comment.
| 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?
There was a problem hiding this comment.
I add this check at first, but if there isn't agent, should we hide the button instead of disabling it?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
update~ and thanks for all the comments
| useEffect(() => { | ||
| const fetchSummaryAgent = async () => { | ||
| try { | ||
| const summaryAgentStatus = await checkAgentsExist( |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
yes, it's import, sorry I forget it., updated~
Signed-off-by: Qxisylolo <[email protected]>
Description
This pr hides the assistant entry when there isn't data2summary agent
!! This pr is a hard dependency that should be merged after opensearch-project/OpenSearch-Dashboards#9277 has been merged.
has agent:
no agent:
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.