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

Add discover summary error info and button click logic enhancement #8352

Merged
merged 5 commits into from
Oct 12, 2024

Conversation

chishui
Copy link
Contributor

@chishui chishui commented Sep 26, 2024

Description

Improve some logic for discover query assist summary.

  1. Display error info when agent returns exception
  2. after click thumbup/thumbdown buttons, the other button would be hidden.

Issues Resolved

Screenshot

Screenshot 2024-09-26 at 11 55 32

Testing the changes

Changelog

Check List

  • All tests pass
    • yarn test:jest
    • yarn test:jest_integration
  • New functionality includes testing.
  • New functionality has been documented.
  • Update CHANGELOG.md
  • Commits are signed per the DCO using --signoff

Copy link
Contributor

❌ Empty Changelog Section

The Changelog section in your PR description is empty. Please add a valid changelog entry or entries. If you did add a changelog entry, check to make sure that it was not accidentally included inside the comment block in the Changelog section.

Signed-off-by: Liyun Xiu <[email protected]>
Copy link
Contributor

❌ Empty Changelog Section

The Changelog section in your PR description is empty. Please add a valid changelog entry or entries. If you did add a changelog entry, check to make sure that it was not accidentally included inside the comment block in the Changelog section.

Copy link

codecov bot commented Sep 26, 2024

Codecov Report

Attention: Patch coverage is 83.33333% with 3 lines in your changes missing coverage. Please review.

Project coverage is 60.93%. Comparing base (58e1645) to head (ab3c2e1).
Report is 17 commits behind head on main.

Files with missing lines Patch % Lines
...c/query_assist/components/query_assist_summary.tsx 83.33% 0 Missing and 3 partials ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #8352   +/-   ##
=======================================
  Coverage   60.93%   60.93%           
=======================================
  Files        3767     3767           
  Lines       89387    89394    +7     
  Branches    13985    13989    +4     
=======================================
+ Hits        54471    54476    +5     
  Misses      31519    31519           
- Partials     3397     3399    +2     
Flag Coverage Δ
Linux_1 28.95% <ø> (ø)
Linux_2 56.25% <ø> (ø)
Linux_3 37.75% <ø> (+<0.01%) ⬆️
Linux_4 29.92% <83.33%> (+<0.01%) ⬆️
Windows_1 28.97% <ø> (ø)
Windows_2 56.20% <ø> (ø)
Windows_3 37.75% <ø> (ø)
Windows_4 29.92% <83.33%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

ruanyl
ruanyl previously approved these changes Sep 26, 2024
Copy link
Member

@kavilla kavilla left a comment

Choose a reason for hiding this comment

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

i like this feature. it's cool to see. some questions and suggestions

export const QueryAssistSummary: React.FC<QueryAssistSummaryProps> = (props) => {
const { query, search } = props.data;
const [summary, setSummary] = useState(null); // store fetched data
Copy link
Member

Choose a reason for hiding this comment

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

do we know for sure the summary will never be ''. null for me implies no summary was retrieved where as if the summary was an empty string the user might not be able to give feedback on the summary even though the information can be used to capture cases where query assist didn't output any summary.

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 case LLM fails to generate summary, we would render an error message. But if LLM is able to generate summary, I believe there's always some outputs.

@@ -76,6 +82,9 @@ export const QueryAssistSummary: React.FC<QueryAssistSummaryProps> = (props) =>
defaultMessage:
'Thank you for the feedback. Try again by adjusting your question so that I have the opportunity to better assist you.',
});
const errorPrompt = i18n.translate('queryEnhancements.queryAssist.summary.errorPrompt', {
Copy link
Member

Choose a reason for hiding this comment

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

should we consider adding : Try again by adjusting your question so that I have the opportunity to better assist you.

Or do we have an error message that we can display to the user for more insight?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some errors are not caused by questions, it might be frustrating when there's backend issue but we keep asking users to change questions.

@kavilla kavilla added the discover for discover reinvent label Sep 26, 2024
@kavilla
Copy link
Member

kavilla commented Sep 26, 2024

does this go to 2.18? 2.17 next?

cc: @joshuali925 @ashwin-pc

@kavilla
Copy link
Member

kavilla commented Sep 26, 2024

@chishui @ruanyl @joshuali925 @opensearch-project/opensearch-ux should we add a retry button?

does query assist support retries with the context that the user didn't think the summary was good enough and will try again? or will it just give back the same summary

@chishui
Copy link
Contributor Author

chishui commented Sep 27, 2024

@chishui @ruanyl @joshuali925 @opensearch-project/opensearch-ux should we add a retry button?

does query assist support retries with the context that the user didn't think the summary was good enough and will try again? or will it just give back the same summary

We'll have a new UI which is to add such button to regenerate summary, will make change once receives new design.

Signed-off-by: Liyun Xiu <[email protected]>
@ashwin-pc
Copy link
Member

Lets add discover-next and 2.18 version labels to all these discover changes. Make sure to backport them to 2.x as well

@chishui
Copy link
Contributor Author

chishui commented Sep 30, 2024

@kavilla @ruanyl @ashwin-pc if you guys have no other questions on this PR, could you help approve?

@kavilla kavilla added the discover for discover reinvent label Sep 30, 2024
@chishui
Copy link
Contributor Author

chishui commented Oct 8, 2024

@kavilla could you review this PR again? It's been a while, thanks

Comment on lines +272 to +273
onClick={() => onFeedback(true)}
data-test-subj="queryAssist_summary_buttons_thumbup"
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we disable the button when feedback === FeedbackStatus.THUMB_UP, so that we don't need to check if (feedback !== FeedbackStatus.NONE) return; for onFeedback()

@ruanyl ruanyl merged commit c4049eb into opensearch-project:main Oct 12, 2024
68 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Oct 12, 2024
…8352)

* Add discover summary error info and button click logic enhancement

Signed-off-by: Liyun Xiu <[email protected]>

* add changelog

Signed-off-by: Liyun Xiu <[email protected]>

* Address comments

Signed-off-by: Liyun Xiu <[email protected]>

---------

Signed-off-by: Liyun Xiu <[email protected]>
(cherry picked from commit c4049eb)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
SuZhou-Joe pushed a commit that referenced this pull request Oct 12, 2024
…8352) (#8568)

* Add discover summary error info and button click logic enhancement



* add changelog



* Address comments



---------


(cherry picked from commit c4049eb)

Signed-off-by: Liyun Xiu <[email protected]>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
sejli pushed a commit to sejli/OpenSearch-Dashboards that referenced this pull request Oct 21, 2024
…pensearch-project#8352)

* Add discover summary error info and button click logic enhancement

Signed-off-by: Liyun Xiu <[email protected]>

* add changelog

Signed-off-by: Liyun Xiu <[email protected]>

* Address comments

Signed-off-by: Liyun Xiu <[email protected]>

---------

Signed-off-by: Liyun Xiu <[email protected]>
Qxisylolo pushed a commit to Qxisylolo/OpenSearch-Dashboards that referenced this pull request Oct 30, 2024
…pensearch-project#8352)

* Add discover summary error info and button click logic enhancement

Signed-off-by: Liyun Xiu <[email protected]>

* add changelog

Signed-off-by: Liyun Xiu <[email protected]>

* Address comments

Signed-off-by: Liyun Xiu <[email protected]>

---------

Signed-off-by: Liyun Xiu <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants