-
Notifications
You must be signed in to change notification settings - Fork 14.1k
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
fix(dashboard): incorrect chart error with slow dataset api request #18852
Conversation
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.
lgtm, thanks for the quick fix
@@ -220,6 +220,7 @@ class Chart extends React.PureComponent { | |||
|
|||
return ( | |||
<ChartErrorMessage | |||
key={chartId} |
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.
was this needed to fix the issue too? i'm surprised that setting the key to the id would cause a rerender since i wouldn't expect the id to ever change
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.
This is a bycatch. Just for fixing a React warning about missing key in loops.
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.
ah thanks, i didn't see a loop anywhere. does this mean the Styles
above needs a key too?
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.
The loop is here: https://github.com/apache/superset/blob/157843be087f2f23a0df7c9efad8f2603150f9dc/superset-frontend/src/chart/Chart.jsx#L252
Good catch on the styled component above!
Codecov Report
@@ Coverage Diff @@
## master #18852 +/- ##
=======================================
Coverage 66.21% 66.21%
=======================================
Files 1633 1633
Lines 63210 63210
Branches 6409 6409
=======================================
Hits 41852 41852
Misses 19698 19698
Partials 1660 1660
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
157843b
to
76f2ebc
Compare
SUMMARY
Fix a bug where sever side chart data query errors were displayed as "No results" when the
/dashboard/[id]/datasets
API is slow.Background
#15699 made loading full datasource info for Dashboard charts async, however, some chart types didn't handle incomplete datasource info properly, causing JS errors. #15993 fixed that by re-rendering the charts after the datasources are fully loaded. Then a “No results" state for dashboard chart container is introduced in
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before
Charts with server side errors may show as "no results":
After
Server side errors correctly rendered as unexpected errors:
TESTING INSTRUCTIONS
Here's how to reproduce the bug:
ADDITIONAL INFORMATION