-
Notifications
You must be signed in to change notification settings - Fork 976
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
[workspace]Hide the assistant entry when there isn't data2summary agent #9277
Conversation
Signed-off-by: Qxisylolo <qianxisy@amazon.com>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #9277 +/- ##
=======================================
Coverage 61.70% 61.70%
=======================================
Files 3816 3815 -1
Lines 91829 91819 -10
Branches 14543 14543
=======================================
- Hits 56666 56660 -6
+ Misses 31507 31505 -2
+ Partials 3656 3654 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
question$?: BehaviorSubject<string>; | ||
} | ||
|
||
const QueryAssistWrapper: React.FC<QueryAssistWrapperProps> = (props) => { | ||
const [visible, setVisible] = useState(false); | ||
const [question, setQuestion] = useState(''); | ||
const [isQuerySummaryCollapsed, setIsQuerySummaryCollapsed] = useState(true); | ||
const [isSummaryAgentAvailable, setIsSummaryAgentAvailable] = useState(false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about we using useObservable(props.isSummaryAgentAvailable$)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the comments, already been updated. but would it be more organized to update state of isSummaryAgentAvailable
together with isQuerySummaryCollapsed
and question
together in useEffect?
useEffect(() => { | ||
setIsSummaryAgentAvailable(false); | ||
const fetchSummaryAgent = async () => { | ||
try { | ||
const summaryAgentStatus = await checkAgentsExist( | ||
props.http, | ||
DATA2SUMMARY_AGENT_CONFIG_ID, | ||
selectedDataset.current?.dataSource?.id | ||
); | ||
setIsSummaryAgentAvailable(summaryAgentStatus.exists); | ||
} catch (error) { | ||
// eslint-disable-next-line no-console | ||
console.error(error); | ||
} | ||
}; | ||
if (isEnabledByCapability) { | ||
fetchSummaryAgent(); | ||
} | ||
}, [selectedDataset.current?.dataSource?.id, props.http, isEnabledByCapability]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there case that assistant is not installed but we turn on result summary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this summary agent work correctly if the dashboard assistant was disabled before this code change? After this change, the isSummaryAgentAvailable$
will be updated out of query enhancements, right? Just want to confirm if it's dependent on the dashboard assistant before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will perform the check in https://github.com/opensearch-project/dashboards-assistant/blob/d9266cf6dbe3af769433241bd47adedc4e482f2c/public/plugin.tsx#L285-L320 to see if the assistant is enabled. If it is, the action context menu will be rendered, and action context menu will call API to check if summary agent exists.
Signed-off-by: Qxisylolo <qianxisy@amazon.com>
@@ -108,7 +113,6 @@ describe('QueryStringManager', () => { | |||
test('clearQueryHistory clears the query history', () => { | |||
service.addToQueryHistory({ query: 'test query 1', language: 'sql' }); | |||
service.addToQueryHistory({ query: 'test query 2', language: 'sql' }); | |||
expect(service.getQueryHistory()).toHaveLength(2); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: The fixing of unit tests seems not related to the current PR. Shall we separate a PR to fix it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I will revert this commit. thanks~
Signed-off-by: Qxisylolo <qianxisy@amazon.com>
7844fab
to
3e37c55
Compare
…nt (#9277) * fix summary entry when no agent in workspace Signed-off-by: Qxisylolo <qianxisy@amazon.com> * Changeset file for PR #9277 created/updated * resolve comments Signed-off-by: Qxisylolo <qianxisy@amazon.com> * fix test Signed-off-by: Qxisylolo <qianxisy@amazon.com> * separate test Signed-off-by: Qxisylolo <qianxisy@amazon.com> --------- Signed-off-by: Qxisylolo <qianxisy@amazon.com> Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com> Co-authored-by: SuZhou-Joe <suzhou@amazon.com> (cherry picked from commit 46ee60c) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Description
This pr hides the assistant entry when there isn't data2summary agent.
Screenshot
No agent:


Has agent:
Testing the changes
Changelog
Check List
yarn test:jest
yarn test:jest_integration