feat: update discover summary UX #8700

ruanyl · 2024-10-24T10:48:21Z

Description

Trigger summary generation on initial query, add manual trigger for subsequent queries.

This PR updates the summary generation logic to improve user experience. Previously, summaries were auto-triggered on every ppl query. Now, summaries will only be auto-generated on the first query execution. For any subsequent queries, users will need to click the "Generate Summary" button to trigger the summary generation manually.

Screen.Recording.2024-10-24.at.18.42.46.mov

Issues Resolved

Screenshot

Testing the changes

Changelog

feat: trigger discover summary generation on initial query add manual trigger for subsequent queries

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

github-actions · 2024-10-24T10:48:55Z

❌ 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.

github-actions · 2024-10-24T10:56:22Z

❌ 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.

codecov · 2024-10-24T11:12:38Z

Codecov Report

Attention: Patch coverage is 85.24590% with 9 lines in your changes missing coverage. Please review.

Project coverage is 60.75%. Comparing base (53e8404) to head (63f292f).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
...c/query_assist/components/query_assist_summary.tsx	86.00%	1 Missing and 6 partials ⚠️
...nts/public/query_assist/utils/create_extension.tsx	77.77%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8700      +/-   ##
==========================================
+ Coverage   60.71%   60.75%   +0.03%     
==========================================
  Files        3798     3798              
  Lines       90662    90668       +6     
  Branches    14260    14263       +3     
==========================================
+ Hits        55047    55081      +34     
+ Misses      32163    32093      -70     
- Partials     3452     3494      +42

Flag	Coverage Δ
Linux_1	`29.06% <ø> (+<0.01%)`	⬆️
Linux_2	`56.39% <ø> (?)`
Linux_3	`37.58% <ø> (+<0.01%)`	⬆️
Linux_4	`29.84% <85.24%> (+0.01%)`	⬆️
Windows_1	`29.08% <ø> (+<0.01%)`	⬆️
Windows_2	`56.34% <ø> (ø)`
Windows_3	`37.58% <ø> (+<0.01%)`	⬆️
Windows_4	`29.84% <85.24%> (+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.

Hailong-am · 2024-10-25T06:51:55Z

src/plugins/query_enhancements/public/query_assist/components/query_assist_summary.tsx

+  const manualTriggerVisible =
+    !shouldAutoTrigger &&
+    canGenerateSummary &&
+    lastUsedQueryStateRef.current?.answer !== queryState.answer;


should we compare result as well?

Maybe unnecessary and too much to have a deep equality check on the results? The "Generate summary" button will be available to for a manual trigger if needed when results updated.

chishui

Thanks for making this change, there are some minor UX changes which might not
be covered:

The title "Response" needs update?
Summary panel should be under PPL edit panel?
"Generate Summary" with a sparkling icon next to it?
From UX mock in the video, it looks like, even the summary is generated, the "Generate summary" is also clickable?

chishui · 2024-10-28T03:14:09Z

src/plugins/query_enhancements/public/query_assist/components/query_assist_bar.tsx

@@ -88,6 +87,7 @@ export const QueryAssistBar: React.FC<QueryAssistInputProps> = (props) => {
        language: params.language,
        dataset: selectedDataset,
      });
+      updateQueryState({ question: previousQuestionRef.current, answer: response.query });


This is a much better implementation. 👍

chishui · 2024-10-28T03:22:47Z

src/plugins/query_enhancements/public/query_assist/hooks/use_query_assist.ts

  question: string;
-  question$: BehaviorSubject<string>;
-  updateQuestion: (question: string) => void;
+  answer: string;


answer is quite vague, can we use generatedQuery or xxxQuery to make it more specific?

Good point, I will update

chishui · 2024-10-28T03:25:56Z

src/plugins/query_enhancements/public/query_assist/components/query_assist_summary.tsx

+    queryState.answer && queryState.answer !== props.data.query.queryString.getQuery().query;
+  // It can generate summary when
+  // 1. it has the current generated query(answer)
+  // 2. the generated query didn't been changed by user


query didn't been changed by user

The definition of it means user doesn't click "run" button after changing query, right?

Yes, let me update the comment so it's more precise.

chishui · 2024-10-28T03:41:35Z

src/plugins/query_enhancements/public/query_assist/components/query_assist_summary.tsx

+  // 1. it has the current generated query(answer)
+  // 2. the generated query didn't been changed by user
+  // 3. there are search results
+  const canGenerateSummary = Boolean(results.length) && Boolean(queryState.answer) && !queryChanged;


There could be a case that when question changes and answer remains the same, but we can't generate summary, is that expected?

I think it can generated summary in such case. canGenerateSummary should be true

chishui · 2024-10-28T03:43:18Z

src/plugins/query_enhancements/public/query_assist/components/query_assist_summary.tsx

+    canGenerateSummary &&
+    lastUsedQueryStateRef.current?.answer !== queryState.answer;
+  // The visibility of panel action buttons: thumbs up/down and copy to clipboard buttons
+  const actionButtonVisible = summary && !loading && !queryChanged && !manualTriggerVisible;


shall we also check if text to ppl is loading?

Sounds reasonable to me, but seems no good way to read the PPL loading state, I think I'll keep it simple for now

chishui · 2024-10-28T03:51:25Z

src/plugins/query_enhancements/public/query_assist/components/query_assist_summary.tsx

+  const getPanelMessage = useCallback(() => {
+    if (loading) {
+      return (
+        <EuiText size="s" data-test-subj="queryAssist_summary_loading">


nit: (refactor suggestion):(non-blocking)
Looks like some duplicate code could be saved:

if (xxx) { return ( <EuiText xxxx> ) }

What about

const renderConditionalText = (shouldRender, dataTestSubj, i18nKey, default) { if (!shouldRender) return null; return ( <EuiText size="s" data-test-subj={dataTestSubj}>.... ) }

I'm not sure if I fully understood, but to my understanding, renderConditionalText is pretty much doing what EuiText does, I don't see much value to move the conditional logic into the function.

src/plugins/query_enhancements/public/query_assist/components/query_assist_summary.tsx

chishui · 2024-10-28T04:02:25Z

src/plugins/query_enhancements/public/query_assist/components/query_assist_summary.tsx

-    },
-    [props.usageCollection, METRIC_APP]
-  );
+  const queryChanged =


Will this be recalculated whenever there's state change?

Yes, the change of queryState will make the component rerender.

ruanyl · 2024-10-29T02:50:41Z

Thanks for making this change, there are some minor UX changes which might not be covered:

The title "Response" needs update?

Summary panel should be under PPL edit panel?

"Generate Summary" with a sparkling icon next to it?

From UX mock in the video, it looks like, even the summary is generated, the "Generate summary" is also clickable?

Thank you very much for the detailed review

Will update
Will be in another PR
~~Will update~~ I will wait for this icon added to OUI
Yes, I think that will allow user to click and regenerate

It will only auto-trigger the summary generating the first time a ppl query was generated. The following summary generation requires user to click a "Generate summary" button. Signed-off-by: Yulong Ruan <[email protected]>

Signed-off-by: Yulong Ruan <[email protected]>

+ change variable `answer` -> `generatedQuery` to be more specific Signed-off-by: Yulong Ruan <[email protected]>

github-actions bot added the all-star-contributor label Oct 24, 2024

github-actions bot added the failed changeset label Oct 24, 2024

ruanyl added the backport 2.x label Oct 24, 2024

github-actions bot removed the failed changeset label Oct 24, 2024

opensearch-changeset-bot bot added a commit to ruanyl/OpenSearch-Dashboards that referenced this pull request Oct 24, 2024

Changeset file for PR opensearch-project#8700 created/updated

33a91d3

Hailong-am reviewed Oct 25, 2024

View reviewed changes

chishui reviewed Oct 28, 2024

View reviewed changes

ruanyl and others added 5 commits October 29, 2024 12:31

feat: update discover summary UX

0f399d0

It will only auto-trigger the summary generating the first time a ppl query was generated. The following summary generation requires user to click a "Generate summary" button. Signed-off-by: Yulong Ruan <[email protected]>

Changeset file for PR opensearch-project#8700 created/updated

7f3db3e

fix i18n

099a2b0

Signed-off-by: Yulong Ruan <[email protected]>

tmp

905049c

Signed-off-by: Yulong Ruan <[email protected]>

fix: move inline css to class

63f292f

+ change variable `answer` -> `generatedQuery` to be more specific Signed-off-by: Yulong Ruan <[email protected]>

ruanyl force-pushed the discover-results-summary-ui-tweaks branch from eae71a9 to 63f292f Compare October 29, 2024 06:01

Changeset file for PR opensearch-project#8700 created/updated

7abfb7f

ruanyl added the v2.19.0 label Oct 31, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: update discover summary UX #8700

feat: update discover summary UX #8700

ruanyl commented Oct 24, 2024 •

edited

Loading

github-actions bot commented Oct 24, 2024

github-actions bot commented Oct 24, 2024

codecov bot commented Oct 24, 2024 •

edited

Loading

Hailong-am Oct 25, 2024

ruanyl Oct 25, 2024

chishui left a comment

chishui Oct 28, 2024

chishui Oct 28, 2024

ruanyl Oct 29, 2024

chishui Oct 28, 2024

ruanyl Oct 29, 2024

chishui Oct 28, 2024

ruanyl Oct 29, 2024

chishui Oct 28, 2024

ruanyl Oct 29, 2024

chishui Oct 28, 2024

ruanyl Oct 29, 2024

chishui Oct 28, 2024

ruanyl Oct 29, 2024

ruanyl commented Oct 29, 2024 •

edited

Loading

feat: update discover summary UX #8700

Are you sure you want to change the base?

feat: update discover summary UX #8700

Conversation

ruanyl commented Oct 24, 2024 • edited Loading

Description

Issues Resolved

Screenshot

Testing the changes

Changelog

Check List

github-actions bot commented Oct 24, 2024

❌ Empty Changelog Section

github-actions bot commented Oct 24, 2024

❌ Empty Changelog Section

codecov bot commented Oct 24, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chishui left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ruanyl commented Oct 29, 2024 • edited Loading

ruanyl commented Oct 24, 2024 •

edited

Loading

codecov bot commented Oct 24, 2024 •

edited

Loading

ruanyl commented Oct 29, 2024 •

edited

Loading