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.

Sign up for GitHub

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

Keep previous query result if current query result in error #8863

Merged
merged 4 commits into from
Nov 15, 2024
Merged

Keep previous query result if current query result in error #8863

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2edde87
keep previous result
abbyhu2000 Nov 14, 2024
f591508
Changeset file for PR #8863 created/updated
opensearch-changeset-bot[bot] Nov 14, 2024
6c9da3b
add some comment
abbyhu2000 Nov 14, 2024
e70cf4e
invalid first query shows refresh data page
abbyhu2000 Nov 14, 2024
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
2 changes: 2 additions & 0 deletions changelogs/fragments/8863.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
fix:
- Keep previous query result if current query result in error ([#8863](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/8863))
20 changes: 9 additions & 11 deletions src/plugins/discover/public/application/view_components/canvas/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,8 +84,13 @@ export default function DiscoverCanvas({ setHeaderActionMenu, history, optionalR
if (next.bucketInterval && next.bucketInterval !== fetchState.bucketInterval)
shouldUpdateState = true;
if (next.chartData && next.chartData !== fetchState.chartData) shouldUpdateState = true;
// we still want to show rows from the previous query while current query is loading
if (next.status !== ResultStatus.LOADING && next.rows && next.rows !== fetchState.rows) {
// we still want to show rows from the previous query while current query is loading or the current query results in error
if (
next.status !== ResultStatus.LOADING &&
next.status !== ResultStatus.ERROR &&
next.rows &&
next.rows !== fetchState.rows
) {
shouldUpdateState = true;
setRows(next.rows);
}
Expand Down Expand Up @@ -152,20 +157,13 @@ export default function DiscoverCanvas({ setHeaderActionMenu, history, optionalR
timeFieldName={timeField}
/>
)}
{fetchState.status === ResultStatus.ERROR && (
<DiscoverNoResults
queryString={data.query.queryString}
query={data.query.queryString.getQuery()}
savedQuery={data.query.savedQueries}
timeFieldName={timeField}
/>
)}
{fetchState.status === ResultStatus.UNINITIALIZED && (
<DiscoverUninitialized onRefresh={() => refetch$.next()} />
)}
{fetchState.status === ResultStatus.LOADING && !rows?.length && <LoadingSpinner />}
{(fetchState.status === ResultStatus.READY ||
(fetchState.status === ResultStatus.LOADING && !!rows?.length)) &&
(fetchState.status === ResultStatus.LOADING && !!rows?.length) ||
Copy link
Member

@kavilla kavilla Nov 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

nit: imo this is getting really hard to understand when and where and i can imagine a regression if someone slightly changes the logic here.

i think we should consider a restructuring of this code.

to me this would be the psuedo code lemme know if im wrong

if rows.length exists, show results based on status
   switch fetchState.status
      READY, LOADING, ERROR
         isEnhancements return either component A or component B
      default
        nothing
else if no rows, handle different status states
   switch fetchState.status
      case: NO_RESULTS
        return no results component
      case: UNITIALIZED, ERROR
        return uniitilizaed component
      case: LOADING
        return loading spinner
      default:
        nothing

Copy link
Member Author

@abbyhu2000 abbyhu2000 Nov 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

i think the above psudo logics are not the same as the current logics

Rows exist or not exist only make a difference for loading status and error status;
ready status and un-initialized and no result status do not care about rows exist or not

fetchState.status === ResultStatus.ERROR) &&
Copy link
Member

Choose a reason for hiding this comment

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

if the first query returned an error and there are no previous query or previous rows, what does it show?

Copy link
Member Author

Choose a reason for hiding this comment

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

Should we show no result UI here if the first query we execute is invalid? @ashwin-pc

Copy link
Member

Choose a reason for hiding this comment

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

Yes it should show the last valid UI state

(isEnhancementsEnabled ? (
<>
<MemoizedDiscoverChartContainer {...fetchState} />
Expand Down
Loading