-
Notifications
You must be signed in to change notification settings - Fork 211
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
frontend: Improve error handling in list pages #2771
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
9c562cd
to
a506dd5
Compare
a506dd5
to
b21b802
Compare
Updated the error message to use Alert component from mui, and made 403 error display as blue since it's probably expected that user with limited access won't be able to see some things |
b21b802
to
c4eadaf
Compare
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.
Pretty cool. I have left a couple of comments.
...tend/src/components/horizontalPodAutoscaler/__snapshots__/HPADetails.Error.stories.storyshot
Show resolved
Hide resolved
Signed-off-by: Oleksandr Dubenko <[email protected]>
c4eadaf
to
6581bd2
Compare
Signed-off-by: Oleksandr Dubenko <[email protected]>
Signed-off-by: Oleksandr Dubenko <[email protected]>
Signed-off-by: Oleksandr Dubenko <[email protected]>
Signed-off-by: Oleksandr Dubenko <[email protected]>
6581bd2
to
21ad9da
Compare
joaquimrocha
approved these changes
Feb 4, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
bug
Something isn't working
enhancement
New feature or request
frontend
Issues related to the frontend
lgtm
This PR has been approved by a maintainer
size:XL
This PR changes 500-999 lines, ignoring generated files.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR combines and replaces different fields we have for error handling, currently we have 'error' 'errorMessage' 'clusterErrors' props in various places.
error
is not enough since we can have more than one errorerrorMessage
is a string and loses important context like which cluster the error is coming from and limits how we can render itclusterErrors
prop has a limitation of one error per cluster but we can have more (see Role page example)New field is
errors
an array ofApiError
ApiError is also updated to be a class instead of an interface and now contains all relevant information (if available) like cluster name, namespace of a resource and status code.
ApiError message is now also parsed from backend response (if available)
To display the error the
ClusterGroupErrorMessage
component was updated since it is already can display error.I would also change the name to something more generic like
ErrorAlerts
or something but it's not a priority.Testing done / Examples
All error messages by default are collapsed, in screenshots I'll expand them
When resource is not available, endpoint returns 404
When user doesn't have access 403
When user doesn't have access and List page loads multiple resource
Role page loads Roles and ClusterRoles
When two clusters are selected and one of them doesn't have access
When two clusters are selected and one of them doesn't have access for a List page with multiple resources
Cluster overview page with no access
![image](https://private-user-images.githubusercontent.com/4517681/406451782-a4c34663-43f3-466a-abde-2a7452d5e65f.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MzkwMDk3MzUsIm5iZiI6MTczOTAwOTQzNSwicGF0aCI6Ii80NTE3NjgxLzQwNjQ1MTc4Mi1hNGMzNDY2My00M2YzLTQ2NmEtYWJkZS0yYTc0NTJkNWU2NWYucG5nP1gtQW16LUFsZ29yaXRobT1BV1M0LUhNQUMtU0hBMjU2JlgtQW16LUNyZWRlbnRpYWw9QUtJQVZDT0RZTFNBNTNQUUs0WkElMkYyMDI1MDIwOCUyRnVzLWVhc3QtMSUyRnMzJTJGYXdzNF9yZXF1ZXN0JlgtQW16LURhdGU9MjAyNTAyMDhUMTAxMDM1WiZYLUFtei1FeHBpcmVzPTMwMCZYLUFtei1TaWduYXR1cmU9NWNlODJjMjdjNDZkZDc1OTlhMjYwNDU4MzE4NTNiY2NhOGMzYzk4YWZlNWU5ZTU3NDMyYTFhMjkwOTgwNWQ2MSZYLUFtei1TaWduZWRIZWFkZXJzPWhvc3QifQ.jE7GOmjOVxNw2HgF7qiYif03z6AhYk9IdQKhVw6LRQ0)