feat: Display deephaven.ui widget errors in a panel so user can see them by mofojed · Pull Request #436 · deephaven/deephaven-plugins

mofojed · 2024-04-25T21:19:21Z

Widget can now emit a documentError on the server to indicate there was an error rendering the document
Client listens for documentError and will display the error message in a panel along with a Reload button to reload the widget
Updated @deephaven/* packages to 0.76.0, which includes the ErrorView and is also in deephaven-core 0.34.1
Fixes Errors not visible when opening deephaven.ui widget from panels menu #437
Fixes Render error handling #85
Tested with a ui_boom and ui_boom_counter components:

from deephaven import ui

@ui.component
def ui_boom():
    raise Exception("BOOM!")

@ui.component
def ui_boom_counter():
    value, set_value = ui.use_state(0)

    if value > 5:
        raise ValueError("BOOM! Value is bigger than 5.")
    
    return ui.button(f"Count is {value}", on_press=lambda: set_value(value + 1))

boom = ui_boom()
boom_counter = ui_boom_counter()

- Add an error listener to a command - WIP, does not actually display the error yet but is wired up to listen to it. - Need to pass the error down to `DocumentHandler`? Or have a separate `DocumentErrorHandler` widget?

- Errors now display in a panel - Still need to clean up the look of it, add a Reload button

- TODO: Fix the Reload button. Doesn't quite seem to be working.

- Cool

- We just send a state reset to the object on the server

- If we're currently re-rendering we shouldn't emit the close

- If a callback throws, display a document error

- It could be possible for a component to expect an error to be thrown and it could handle that exception... I don't think we should handle this right now. May revisit later.

- Will fill the contents of the panel if necessary

mattrunyon · 2024-04-26T14:34:01Z

+            # Something catastrophic happened, log it and close the connection
            logger.exception(e)
+            self._connection.on_close()


Is there an example of something that can cause this? Just want to test it

Mmmm I can't think of a way it could raise an exception right now... but I want to make sure we don't log the error at all. Uncaught exceptions on submit_task are not logged and do not shut down the system: deephaven/deephaven-core#5192
I'll add that note to the comment, this is more just to be safe. Really, the issue above should be fixed and then this should be removed (because the server will just shut down at that point).

mattrunyon · 2024-04-26T14:38:12Z

+        request = self._make_notification(
+            "documentError",
+            json.dumps(
+                {
+                    "message": str(error),
+                    "type": type(error).__name__,
+                    "stack": stack_trace,
+                    "code": ErrorCode.DOCUMENT_ERROR.value,
+                }


Should this be in the JSON schema? I don't think we're using it really, but probably not bad to keep it updated unless you don't think we should use schema checking at some point in the future

Yes it should be, good call.

mattrunyon · 2024-04-26T15:10:47Z

+/**
+ * Component that displays an error message in a textarea so user can scroll and a copy button.
+ * TODO: Use the one from @deephaven/components when it's merged: https://github.com/deephaven/web-client-ui/pull/1965
+ */


Going to leave this for review in the web-client-ui PR

If you want to test w/ the component, you can externalize @deephaven/components in vite instead of copying the component into dh.ui. Then just import it from @deephaven/components. It's provided via remote-component.config.ts in app-utils

- They're not quite correct yet, but I want to refactor some other things first.

- Now it will just render as an overlay for any panels that are open, rather than closing/re-opening panels

- No longer needed now that we have overlay rendering over existing content

- Now it will check that the error cases are displayed correctly

- Still need the package to be published

mattrunyon · 2024-05-02T16:51:18Z

-    "@deephaven/jsapi-bootstrap": "^0.74.0",
-    "@deephaven/jsapi-components": "^0.74.0",
+    "@deephaven/chart": "^0.73.0",
+    "@deephaven/components": "^0.73.0",


Now we probably do want to bump these to use the ErrorView from @deephaven/components

mattrunyon · 2024-05-03T15:31:35Z

@@ -0,0 +1,13 @@
+/** A panel that uses the ReactPanelContentOverlayContext and if that content is set, renders it in a view with a partially transparent background */


I think this comment needs to go above the component, not at the top of the file

mattrunyon · 2024-05-09T17:44:44Z

Noticed #85 is also fixed by this PR

The styling is inconsistent between components that didn't render and components that did

Same background color

Different background color on the right and darker looking overlay

mofojed · 2024-05-09T18:31:21Z

@mattrunyon On the right it is overlaid on top of content, so it appears as a shade. This is akin to when we have a table that doesn't open because of an error, and a table that gets an error, e.g.:

...though it looks like the shade there isn't done at all. Looking into it a bit further, seems like we've got different values for all the scrims: https://github.com/search?q=repo%3Adeephaven%2Fweb-client-ui%20black-opacity&type=code

For IrisGrid we use 10% (which I think is incorrect): https://github.com/deephaven/web-client-ui/blob/3de52d6fa0512792c97928f65f0b4b1080da2c49/packages/components/src/LoadingOverlay.scss#L5

For ChartPanel we use 50% (which is probably better): https://github.com/deephaven/web-client-ui/blob/3de52d6fa0512792c97928f65f0b4b1080da2c49/packages/dashboard-core-plugins/src/panels/ChartPanel.scss#L5

And then here I've got 90%.

@dsmmcken Which scrim % should be used in these cases?

mattrunyon · 2024-05-09T18:37:08Z

In my pictures with and without content, both panels have the same color (look at the bottom of the panels). So I think that the opacity is applied at the wrong element or is not being applied if there's no content.

mofojed · 2024-05-09T18:44:15Z

@dsmmcken here it is with 50%, what do you think?

mofojed · 2024-05-09T18:45:45Z

Light theme we may want to use something different rather than black-opacity...:

mofojed · 2024-05-09T18:46:41Z

@mattrunyon the error overlays overtop of the content if it exists and that's when a transparency is applied.

mofojed · 2024-05-09T18:55:55Z

Used bg-content instead so it themes correctly, and up'd it to 80%.

dsmmcken · 2024-05-09T19:01:41Z

mattrunyon · 2024-05-09T19:13:53Z

The error box itself should be more opaque (the red). If you had a more complex component under it, I think it might be too low contrast and too hard to read

- Blends better with light themes

mofojed · 2024-05-10T13:21:53Z

A bit of blur would achieve the same thing:

mofojed · 2024-05-10T14:23:20Z

@mattrunyon Don approved with the blur.

mattrunyon · 2024-05-10T16:33:27Z

Ok. I'm still not a fan of the red text on transparent red background. Seems like it's still too low contrast, but I'll defer to Don on this one

mattrunyon

Both light and dark theme with and without content fail the chrome contrast ratio test for accessibility (I checked the contrast ratio on the "ValueError" span w/ dev tools).

That said, it's not worth holding this up since it should be fine in most cases still. And it would be a change in web-client-ui to fix the contrast of ErrorView

mofojed · 2024-05-10T17:31:50Z

@mattrunyon there's a PR for improving error view in web-client-ui: deephaven/web-client-ui#2001

- Reverted package-lock.json to before deephaven#436 - Fixed the jsapi-types stuff, then do an npm install to re-update the package-lock.json

- Reverted package-lock.json to before #436 - Fixed the jsapi-types stuff, then do an npm install to re-update the package-lock.json

mofojed added 9 commits April 25, 2024 10:12

fix: Errors not visible when opening deephaven.ui widget

1d00715

- Add an error listener to a command - WIP, does not actually display the error yet but is wired up to listen to it. - Need to pass the error down to `DocumentHandler`? Or have a separate `DocumentErrorHandler` widget?

Create an ErrorView to display the errors

636052f

- Errors now display in a panel - Still need to clean up the look of it, add a Reload button

Display the full stack trace for errors in a panel

2669a9e

- TODO: Fix the Reload button. Doesn't quite seem to be working.

Reload document correctly if there's an error

ef25b59

- Cool

Remove the widget onReset stuff

bff8f14

- We just send a state reset to the object on the server

Fix widget closing when it was updating to an error state

5b29a9a

- If we're currently re-rendering we shouldn't emit the close

Display an error when a callback throws an error

95958a5

- If a callback throws, display a document error

Remove displaying an error on a callback

d180a22

- It could be possible for a component to expect an error to be thrown and it could handle that exception... I don't think we should handle this right now. May revisit later.

Add a couple of TODOs

fbb58d3

mofojed requested a review from mattrunyon April 25, 2024 21:19

mofojed self-assigned this Apr 25, 2024

Make it so the error is expanded by default

3939afe

- Will fill the contents of the panel if necessary

mattrunyon requested changes Apr 26, 2024

View reviewed changes

mofojed added 3 commits April 26, 2024 11:51

Don't need to update the packages, keep as a separate PR

99f157d

Update the JSON schema

3200e29

Clean up based on review

2eb1140

mofojed requested a review from mattrunyon April 26, 2024 18:17

mofojed added 7 commits April 29, 2024 10:42

WIP add e2e tests

177ed22

- They're not quite correct yet, but I want to refactor some other things first.

Add react panel content overlay

856c92d

- Now it will just render as an overlay for any panels that are open, rather than closing/re-opening panels

Get rid of the isUpdating ref in DocumentHandler

8686d6c

- No longer needed now that we have overlay rendering over existing content

Fix up styling so that it takes up the full height of the panel

5bebceb

Update e2e tests for deephaven.ui

86c1d30

- Now it will check that the error cases are displayed correctly

Clean up unused import

6ba45e3

Use ErrorView from @deephaven/components

03d79b4

- Still need the package to be published

mattrunyon requested changes May 3, 2024

View reviewed changes

mofojed added 2 commits May 9, 2024 10:07

Merge remote-tracking branch 'origin/main' into 1949-errors-not-visible

46f7740

Update packages

51972e2

mofojed requested a review from mattrunyon May 9, 2024 14:15

Resolve review comment

e6ac2d2

mofojed added 2 commits May 9, 2024 16:37

Update to use bg-opacity

6ff6810

- Blends better with light themes

Merge remote-tracking branch 'origin/main' into 1949-errors-not-visible

8e31040

Add a little bit of blur

76ed3f2

mattrunyon approved these changes May 10, 2024

View reviewed changes

mofojed merged commit b23b571 into deephaven:main May 10, 2024

mofojed added a commit to mofojed/deephaven-plugins that referenced this pull request May 17, 2024

build: Fix package-lock changes

cabe171

- Reverted package-lock.json to before deephaven#436 - Fixed the jsapi-types stuff, then do an npm install to re-update the package-lock.json

mofojed mentioned this pull request May 17, 2024

build: Fix package-lock changes #472

Merged

mofojed added a commit that referenced this pull request May 21, 2024

build: Fix package-lock changes (#472)

39a883e

- Reverted package-lock.json to before #436 - Fixed the jsapi-types stuff, then do an npm install to re-update the package-lock.json

mofojed linked an issue May 28, 2024 that may be closed by this pull request

Show an error message for dh.ui panels that could not rehydrate #327

Closed

		@@ -0,0 +1,13 @@
		/** A panel that uses the ReactPanelContentOverlayContext and if that content is set, renders it in a view with a partially transparent background */

Conversation

mofojed commented Apr 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mattrunyon Apr 26, 2024

Choose a reason for hiding this comment

Uh oh!

mofojed Apr 26, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mattrunyon Apr 26, 2024

Choose a reason for hiding this comment

Uh oh!

mofojed Apr 26, 2024

Choose a reason for hiding this comment

Uh oh!

mattrunyon Apr 26, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mattrunyon May 2, 2024

Choose a reason for hiding this comment

Uh oh!

mattrunyon May 3, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mattrunyon commented May 9, 2024

Uh oh!

mofojed commented May 9, 2024

Uh oh!

mattrunyon commented May 9, 2024

Uh oh!

mofojed commented May 9, 2024

Uh oh!

mofojed commented May 9, 2024

Uh oh!

mofojed commented May 9, 2024

Uh oh!

mofojed commented May 9, 2024

Uh oh!

dsmmcken commented May 9, 2024

Uh oh!

mattrunyon commented May 9, 2024

Uh oh!

mofojed commented May 10, 2024

Uh oh!

mofojed commented May 10, 2024

Uh oh!

mattrunyon commented May 10, 2024

Uh oh!

mattrunyon left a comment

Choose a reason for hiding this comment

Uh oh!

mofojed commented May 10, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mofojed commented Apr 25, 2024 •

edited

Loading