-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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 error boundary to catch errors in visualizations #4518
Conversation
message="Something went wrong." | ||
description={ | ||
<Typography.Text style={{ display: "block" }} ellipsis> | ||
{error.message} |
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.
Let's avoid showing users our dirty laundry and only log it (with debug
). In the future it makes sense to send this into Sentry.
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.
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.
@arikfr Maybe, we could show some message instead - like "Please check visualization options" / "Read docs" (and link to knowledge base) / "Your PC is tired, turn it off" / etc?
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 would go with either one of:
Error while rendering visualization. Please check visualization options and/or dataset.
Error while rendering visualization.
I'm not sure about suggesting checking the option as usually an error boundary will be triggered due to a bug, no?
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, mostly that will be bugs. But I think there are situations when checking options may help - for example, when visualization expects some column to be, say, numeric, but it's date/time. Of course there should be sanity check in the visualization's code, but in this case user can just use another column or update query. Anyway, we could leave just a "Something went wrong" message
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.
"Error while rendering visualization." is probably a bit better than the generic "something went wrong" as it gives a direction to what went wrong :)
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.
Done 7c5024b
bc28c96
to
7c5024b
Compare
👍 |
What type of PR is this? (check all applicable)
Description
Mobile & Desktop Screenshots/Recordings (if there are UI changes)