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

Handle errors globally with Error Middleware and React Error Boundary #134

Merged

Conversation

GitanjaliChhetri
Copy link
Contributor

@GitanjaliChhetri GitanjaliChhetri commented Nov 19, 2022

Summary of the PR:

  1. Handle all known errors from dicomweb-client, dcmjs, dmv, and the app itself using ErrorMiddleware.js (an extension of pubsub pattern). Errors are now funneled to the onError event. User is notified based on the notification type (configurable, toast and console for now) for each source of error.
  2. A debug info button is added to the header component. It is subscribed to the onError event and keeps a record of all 3 types of error - Server, Data Parsing, and Viewer.
  3. ErrorBoundary.tsx : The React error boundary component is responsible for catching (unexpected) rendering errors and displays a modal window as a fallback component. It is currently wrapped around the App component.

image
image
image

Copy link
Collaborator

@igoroctaviano igoroctaviano left a comment

Choose a reason for hiding this comment

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

Left a few comments. Please prioritize the removal of all formatting changes so we have a clearer pull request to review.

Thank you in advance.

src/App.tsx Outdated Show resolved Hide resolved
src/DicomWebManager.ts Outdated Show resolved Hide resolved
src/components/ErrorHandler/CustomPubSub.js Outdated Show resolved Hide resolved
src/components/ErrorHandler/ErrorMiddleware.js Outdated Show resolved Hide resolved
src/data/slides.tsx Outdated Show resolved Hide resolved
@GitanjaliChhetri GitanjaliChhetri marked this pull request as ready for review November 23, 2022 03:30
src/components/Header.tsx Outdated Show resolved Hide resolved
src/services/NotificationMiddleware.js Show resolved Hide resolved
src/services/NotificationMiddleware.js Outdated Show resolved Hide resolved
src/services/NotificationMiddleware.js Outdated Show resolved Hide resolved
src/components/OpticalPathItem.tsx Show resolved Hide resolved
Copy link
Collaborator

@igoroctaviano igoroctaviano left a comment

Choose a reason for hiding this comment

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

@GitanjaliChhetri code LGTM. Feel free to assign it to @hackermd for final review/approval.

@GitanjaliChhetri
Copy link
Contributor Author

@hackermd I am unable to add you as one of the reviewers. Please know this PR is ready for your review. Thanks!

Copy link
Collaborator

@hackermd hackermd left a comment

Choose a reason for hiding this comment

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

@GitanjaliChhetri, thanks so much for this great work!

I only have a few comments regarding coding style and questions regarding naming things.

src/components/Header.tsx Outdated Show resolved Hide resolved
src/services/NotificationMiddleware.js Outdated Show resolved Hide resolved
src/services/NotificationMiddleware.js Outdated Show resolved Hide resolved
src/services/NotificationMiddleware.js Outdated Show resolved Hide resolved
src/services/NotificationMiddleware.js Show resolved Hide resolved
@GitanjaliChhetri
Copy link
Contributor Author

@hackermd I have addressed the requested changes. Please let me know if this can be merged? Thank you.

@fedorov
Copy link
Member

fedorov commented Jan 27, 2023

@hackermd if you could let us know when you might have time to look into this, it would be helpful!

Copy link
Collaborator

@hackermd hackermd left a comment

Choose a reason for hiding this comment

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

@GitanjaliChhetri it's an elegant implementation. Please address the coding style issues and make sure the automated tests pass. Then we should be ready to merge.

@fedorov fedorov added the idc:priority Priority for the NCI's Imaging Data Commons label Mar 27, 2023
@igoroctaviano
Copy link
Collaborator

@hackermd comments addressed. Can you please re-review? also, looks like the deploy gh-pages are not working. I think its because of action permissions. https://stackoverflow.com/questions/72851548/permission-denied-to-github-actionsbot

Copy link
Collaborator

@hackermd hackermd left a comment

Choose a reason for hiding this comment

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

Thank you very much @igoroctaviano!

@hackermd
Copy link
Collaborator

looks like the deploy gh-pages are not working. I think its because of action permissions. https://stackoverflow.com/questions/72851548/permission-denied-to-github-actionsbot

Not sure why this is happening. The settings are correct and it should work. Maybe a quirk related to the moving of the repository to the ImagingDataCommons organization. Could this be an issue with a global org-wide setting?

@hackermd hackermd merged commit 1c7b6a6 into ImagingDataCommons:master Apr 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
idc:priority Priority for the NCI's Imaging Data Commons released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants