Error alert component #4084

castelblanque · 2022-01-14T11:57:36Z

Signed-off-by: Rafa Castelblanque rcastelblanq@vmware.com

Description of the change

Introduces a React ErrorAlert component aimed at improving readability of UI errors and that could replace existing <Alert> usages.

It is able to turn this kind of UI error:

into this:

It can detect RPC errors and show them indented while not affecting existing plain text errors.
The change also allows to specify a stack of errors when creating a custom error (or an extension of it).

Benefits

Better error readability on UI

Possible drawbacks

None that I can think of.

Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>

castelblanque · 2022-01-14T11:58:08Z

dashboard/src/actions/apps.ts

+      dispatch(
+        errorApp(
+          e instanceof Error
+            ? new FetchError("Unable to list apps", [e])


Only place where the new constructor for errors is used by now.

castelblanque · 2022-01-14T11:58:28Z

dashboard/src/components/AppList/AppList.tsx

@@ -131,7 +131,7 @@ function AppList() {
        className="margin-t-xl"
      >
        {error ? (
-          <Alert theme="danger">Unable to list apps: {error.message}</Alert>
+          <ErrorAlert>{error}</ErrorAlert>


Only usage of the new ErrorAlert component by now.

Great, just to confirm: you mean there are other errors out there (like there is a problem with this package...) which are not being handled by this novel ErrorAlert, don't you?
So this is just a kind of proof of concept, no?

Exactly, I first wanted the community to review it. If accepted, we can start using it from now on and eventually replace existing <Alert> with it.

castelblanque · 2022-01-14T12:00:12Z

dashboard/src/shared/RpcError.ts

@@ -0,0 +1,24 @@
+export class RpcError extends Error {
+  // Following https://github.com/grpc/grpc-go/blob/master/internal/status/status.go#L140
+  static RPC_ERROR_REGEX = /rpc error: code = (\w+) desc = (.*)/;


Until we can have GRPC code to properly throw a dedicated error object, we can only guess if it is an rpc error by checking the error message string.

You should be able to check the grpc error code? At least, the gRPC dashboard code should be throwing an error object with a .code property. That's what we're using here already: https://github.com/kubeapps/kubeapps/blob/master/dashboard/src/shared/Auth.ts#L71-L88

Thanks, will definitely take a look. It would be simpler.

castelblanque · 2022-01-14T12:00:47Z

dashboard/src/shared/types.ts

-  // via err.contructor  == FOO
-  constructor(message?: string) {
+  // via err.constructor  == FOO
+  constructor(message?: string, causes?: Error[]) {


Allows to optionally specify a stack of causing errors.

Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>

castelblanque · 2022-01-14T12:14:33Z

dashboard/src/components/ErrorAlert/ErrorAlert.tsx

+import "./ErrorAlert.css";
+
+export interface IErrorAlert {
+  children: CustomError | Error | string;


Component supports rendering this three types.

What if there are no children?

Children is a mandatory item it seems. In this case doesn't make sense to show the error block without any content, does it?

castelblanque · 2022-01-14T12:20:02Z

This can be a complimentary work to #3723

antgamdia

Great. It seems a great UI improvement; I only have some minor comments and questions but in general LGTM!
Thank you!

antgamdia · 2022-01-19T13:46:52Z

dashboard/src/components/AppList/AppList.tsx

@@ -131,7 +131,7 @@ function AppList() {
        className="margin-t-xl"
      >
        {error ? (
-          <Alert theme="danger">Unable to list apps: {error.message}</Alert>
+          <ErrorAlert>{error}</ErrorAlert>


Great, just to confirm: you mean there are other errors out there (like there is a problem with this package...) which are not being handled by this novel ErrorAlert, don't you?
So this is just a kind of proof of concept, no?

antgamdia · 2022-01-19T13:47:58Z

dashboard/src/components/ErrorAlert/ErrorAlert.tsx

+import "./ErrorAlert.css";
+
+export interface IErrorAlert {
+  children: CustomError | Error | string;


What if there are no children?

antgamdia · 2022-01-19T13:50:03Z

dashboard/src/components/ErrorAlert/ErrorAlert.tsx

+  } else if (children instanceof Error) {
+    messages = [createWrap(children.message, 0, false)];
+  } else {
+    messages = [children];


As above, what if we have no response from the other side of the wire. For example, the API server being OOMkilled by k8s and the gRPC call just returns the nginx error 502 page ? - meaning no explicit error message being sent.

Same as commented above, it does not make sense to have the error block with empty data. For the error codes, it will show exactly the same as current <Alert> component, just the passed children.

antgamdia · 2022-01-19T13:51:24Z

dashboard/src/components/RpcErrorMessage/RpcErrorMessage.scss

@@ -0,0 +1,9 @@
+.error-alert-rpc {
+  ul > li {
+    font-weight: bolder;


We are using explicit numbers in the rest of the CSS files, usually 400, 500 or 600. See what you think

Sure, will change it to be aligned 😃

antgamdia · 2022-01-19T13:51:32Z

dashboard/src/components/RpcErrorMessage/RpcErrorMessage.scss

+    font-weight: bolder;
+
+    span.rpc-value {
+      font-weight: normal;


antgamdia · 2022-01-19T13:52:56Z

dashboard/src/shared/RpcError.ts

@@ -0,0 +1,24 @@
+export class RpcError extends Error {


I wonder if we should rename Rpc to GRpc instead

Yeah, I thought about it, but error message states rpc error: so I went ahead with a generic rpc naming for everything.

antgamdia · 2022-01-19T13:54:26Z

dashboard/src/shared/RpcError.ts

+  private extractData(message: string): [string, string, string] {
+    const found = message.trim().match(RpcError.RPC_ERROR_REGEX);
+    return found
+      ? [found[1], found[2], message.substring(0, message.indexOf(found[0]))]


I'd rather check found.length to avoid undesired undefined

Added tests for empty strings and undefined and it seems ok.

I was curious, so checked the match return for empty strings. It is undefined, for a positive case, the data is in the array. So we are good.

Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>

Rafa Castelblanque added 3 commits January 14, 2022 12:27

Error alert component (#3912)

9da9ac5

Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>

Fixed CSS linting (#3912)

fd0093d

Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>

Fix prettier (#3912)

b26b044

Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>

castelblanque commented Jan 14, 2022

View reviewed changes

Added support for regular Error (#3912)

43a6579

Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>

castelblanque commented Jan 14, 2022

View reviewed changes

castelblanque mentioned this pull request Jan 14, 2022

Improve UI handling while backend is still starting up or upgrading #3912

Closed

antgamdia approved these changes Jan 19, 2022

View reviewed changes

Added some tests (#3912)

0aac11a

Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>

castelblanque merged commit 853c6cc into master Jan 20, 2022

castelblanque deleted the 3912-ui-message-startup branch January 20, 2022 10:06

absoludity mentioned this pull request Feb 25, 2022

User-friendly error handling for failed kubeapps-apis server rpc calls #3723

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Error alert component #4084

Error alert component #4084

castelblanque commented Jan 14, 2022

castelblanque Jan 14, 2022

castelblanque Jan 14, 2022

antgamdia Jan 19, 2022

castelblanque Jan 20, 2022

castelblanque Jan 14, 2022

absoludity Jan 24, 2022

castelblanque Jan 28, 2022

castelblanque Jan 14, 2022 •

edited

castelblanque Jan 14, 2022

antgamdia Jan 19, 2022

castelblanque Jan 20, 2022

castelblanque commented Jan 14, 2022

antgamdia left a comment

antgamdia Jan 19, 2022

antgamdia Jan 19, 2022

antgamdia Jan 19, 2022

castelblanque Jan 20, 2022

antgamdia Jan 19, 2022

castelblanque Jan 20, 2022

antgamdia Jan 19, 2022

antgamdia Jan 19, 2022

castelblanque Jan 20, 2022

antgamdia Jan 19, 2022

castelblanque Jan 20, 2022

castelblanque Jan 20, 2022 •

edited

Error alert component #4084

Error alert component #4084

Conversation

castelblanque commented Jan 14, 2022

Description of the change

Benefits

Possible drawbacks

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

castelblanque Jan 14, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

castelblanque commented Jan 14, 2022

antgamdia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

castelblanque Jan 20, 2022 • edited

Choose a reason for hiding this comment

castelblanque Jan 14, 2022 •

edited

castelblanque Jan 20, 2022 •

edited