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

[UI] Add Validation and Error Boundaries #8833

Merged
merged 8 commits into from Sep 20, 2023

Conversation

aabidsofi19
Copy link
Contributor

@aabidsofi19 aabidsofi19 commented Sep 20, 2023

Notes for Reviewers

This PR fixes the crash of ui on invalid/corrupt events and adds error boundaries for gracefull error handling

Notes :

  • I have used react-error-boundary ( we use it in other projects also )
  • ajv for validation of response ( i have hardcoded the event schema for now but we can also use openapi codegen )

Signed commits

  • Yes, I signed my commits.

@github-actions github-actions bot added the component/ui User Interface label Sep 20, 2023
@aabidsofi19 aabidsofi19 added the priority/urgent Issue to be addressed immediately label Sep 20, 2023
@aabidsofi19
Copy link
Contributor Author

@theBeginner86

package.json Outdated Show resolved Hide resolved
Signed-off-by: aabidsofi19 <mailtoaabid01@gmail.com>
Signed-off-by: aabidsofi19 <mailtoaabid01@gmail.com>

export const withErrorBoundary = (Component) => {
const WrappedWithErrorBoundary = (props) => (
<ErrorBoundary FallbackComponent>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<ErrorBoundary FallbackComponent>
<ErrorBoundary Fallback>

Or I am missing something?


return (
<div role="alert">
<p>Something went wrong:</p>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<p>Something went wrong:</p>
<h3>Please pardon the mesh</h3>

Copy link
Contributor

@MUzairS15 MUzairS15 left a comment

Choose a reason for hiding this comment

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

We can add error boundary to the top level app , also the component lvl boundary could be made more specific to include the comp name or filename which caused the error. As stacktrace might be difficult to trace through sometimes.

in case of multiple error boundary closest one will be executed


//TODO: This should be generated from OPENAPI schema
const EVENT_SCHEMA = {
type : "object",
Copy link
Contributor

Choose a reason for hiding this comment

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

The schema is already present but the contract b/w fronted and backend is missing.

also, this schema is inaccurate as it lacks category and action

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we were not using that in frontend anywhere yet , thats why forgot . will add those




const EVENT_METADATA_SCHEMA = {
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn’t always the case, in long operations i.e. an operation which invoked multiple other operation the metadata contains a summary which is not an error specifically.

Copy link
Member

Choose a reason for hiding this comment

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

Very good point regarding errors being one of a number of different types of events.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks Uzair , i have not considered that . do you have an example response for this kind of situation

Copy link
Member

@theBeginner86 theBeginner86 left a comment

Choose a reason for hiding this comment

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

We can add error boundary to the top level app , also the component lvl boundary could be made more specific to include the comp name or filename which caused the error. As stacktrace might be difficult to trace through sometimes.

in case of multiple error boundary closest one will be executed

+1

I too think that adding error boundary at root level would be more beneficial. As by doing that we would resolve not only the notifications center issues but unwanted UI crashes in general. Would help us to be future-proof

Copy link
Contributor

@MUzairS15 MUzairS15 left a comment

Choose a reason for hiding this comment

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

Follow up on review suggestions separately.

@MUzairS15 MUzairS15 merged commit fa6ef63 into meshery:master Sep 20, 2023
12 of 13 checks passed
@aabidsofi19
Copy link
Contributor Author

We can add error boundary to the top level app , also the component lvl boundary could be made more specific to include the comp name or filename which caused the error. As stacktrace might be difficult to trace through sometimes.
in case of multiple error boundary closest one will be executed

+1

I too think that adding error boundary at root level would be more beneficial. As by doing that we would resolve not only the notifications center issues but unwanted UI crashes in general. Would help us to be future-proof

error boundary at root level is good to catch all the errors but that also means we wont have a usable ui on error . the more granular we go the better ux and easy debugging . But I Agree to your point that there should be a balance and every component might not need it .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/ui User Interface priority/urgent Issue to be addressed immediately
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants