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.

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(notifications): handle Error type messages #305

Merged

Conversation

andrewazores
Copy link
Member

Fixes #304

The root cause of the bug is that services and components may attempt to display a notification with an Error object as the message to display to the user. This fails because the rendered child component (content of the graphical notification) must be a string. This is a bit tricky because the Notification data type specifies that the message property should be a string, however many/most places where an Error object may be obtained it is typed as 'any', which breaks some of the TS compiler's type-checking guarantees at compile time. To work around this, I apply a simple fix to the notifications service: do a runtime check of the message type, and if it isn't an actual string, apply "JSON.stringify" to serialize it to a string before it is displayed.

@ebaron
Copy link
Member

ebaron commented Sep 28, 2021

No more white screen after this fix! There's 3 error notifications, which might not mean anything to users, but it works.

Screenshot 2021-09-28 at 17-33-07 Cryostat

@andrewazores
Copy link
Member Author

I think that's to be expected at this point. The first one looks like it comes from the frontend's HTTP service, which automatically displays notifications on 500s. The second is from the component that made that request, trying to give more detail about the specific failure. And the last one would have been there normally due to the messaging service connection failure due to also lacking auths.

@andrewazores andrewazores merged commit 6325741 into cryostatio:main Sep 28, 2021
@andrewazores andrewazores deleted the notification-error-message branch September 28, 2021 21:47
mergify bot pushed a commit that referenced this pull request Sep 28, 2021
(cherry picked from commit 6325741)

# Conflicts:
#	src/app/Notifications/Notifications.tsx
ebaron pushed a commit that referenced this pull request Sep 28, 2021
* fix(notifications): handle Error type messages (#305)

(cherry picked from commit 6325741)

# Conflicts:
#	src/app/Notifications/Notifications.tsx

* Resolve conflict

Co-authored-by: Andrew Azores <aazores@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error notifications can cause React child component render failure
2 participants