-
Notifications
You must be signed in to change notification settings - Fork 899
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
Keep previous query result if current query result in error #8863
Keep previous query result if current query result in error #8863
Conversation
Signed-off-by: abbyhu2000 <[email protected]>
Signed-off-by: abbyhu2000 <[email protected]>
{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) || | ||
fetchState.status === ResultStatus.ERROR) && |
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.
if the first query returned an error and there are no previous query or previous rows, what does it show?
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.
Should we show no result UI here if the first query we execute is invalid? @ashwin-pc
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.
Yes it should show the last valid UI state
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #8863 +/- ##
==========================================
- Coverage 60.92% 56.39% -4.54%
==========================================
Files 3800 1239 -2561
Lines 90841 25913 -64928
Branches 14313 4437 -9876
==========================================
- Hits 55343 14613 -40730
+ Misses 31978 10559 -21419
+ Partials 3520 741 -2779
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Signed-off-by: abbyhu2000 <[email protected]>
{(fetchState.status === ResultStatus.READY || | ||
(fetchState.status === ResultStatus.LOADING && !!rows?.length)) && | ||
(fetchState.status === ResultStatus.LOADING && !!rows?.length) || |
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: 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
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 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
* keep previous result Signed-off-by: abbyhu2000 <[email protected]> * Changeset file for PR #8863 created/updated * add some comment Signed-off-by: abbyhu2000 <[email protected]> * invalid first query shows refresh data page Signed-off-by: abbyhu2000 <[email protected]> --------- Signed-off-by: abbyhu2000 <[email protected]> Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com> (cherry picked from commit bca4f5c) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
|
…8870) * keep previous result * Changeset file for PR #8863 created/updated * add some comment * invalid first query shows refresh data page --------- (cherry picked from commit bca4f5c) Signed-off-by: abbyhu2000 <[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> Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com>
…pensearch-project#8863)" This reverts commit bca4f5c. Signed-off-by: abbyhu2000 <[email protected]>
Description
Currently, an error query will display no result.
This PR change to keep previous query result if current query result in error.
Issues Resolved
Screenshot
If the query result in error, it will keep the previous query result without updating the canvas
https://github.com/user-attachments/assets/f72b8a74-ad71-4261-8493-fc5e3b24a74b
If the first query result in error, it will show the refresh data screen.
Screen.Recording.2024-11-13.at.5.24.18.PM.mov
Testing the changes
Changelog
Check List
yarn test:jest
yarn test:jest_integration