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

feat: Display deephaven.ui widget errors in a panel so user can see them #436

Merged
merged 26 commits into from
May 10, 2024
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
1d00715
fix: Errors not visible when opening deephaven.ui widget
mofojed Apr 19, 2024
636052f
Create an ErrorView to display the errors
mofojed Apr 25, 2024
2669a9e
Display the full stack trace for errors in a panel
mofojed Apr 25, 2024
ef25b59
Reload document correctly if there's an error
mofojed Apr 25, 2024
bff8f14
Remove the widget onReset stuff
mofojed Apr 25, 2024
5b29a9a
Fix widget closing when it was updating to an error state
mofojed Apr 25, 2024
95958a5
Display an error when a callback throws an error
mofojed Apr 25, 2024
d180a22
Remove displaying an error on a callback
mofojed Apr 25, 2024
fbb58d3
Add a couple of TODOs
mofojed Apr 25, 2024
3939afe
Make it so the error is expanded by default
mofojed Apr 26, 2024
99f157d
Don't need to update the packages, keep as a separate PR
mofojed Apr 26, 2024
3200e29
Update the JSON schema
mofojed Apr 26, 2024
2eb1140
Clean up based on review
mofojed Apr 26, 2024
177ed22
WIP add e2e tests
mofojed Apr 29, 2024
856c92d
Add react panel content overlay
mofojed Apr 29, 2024
8686d6c
Get rid of the isUpdating ref in DocumentHandler
mofojed Apr 29, 2024
5bebceb
Fix up styling so that it takes up the full height of the panel
mofojed Apr 29, 2024
86c1d30
Update e2e tests for deephaven.ui
mofojed Apr 29, 2024
6ba45e3
Clean up unused import
mofojed Apr 29, 2024
03d79b4
Use ErrorView from @deephaven/components
mofojed Apr 30, 2024
46f7740
Merge remote-tracking branch 'origin/main' into 1949-errors-not-visible
mofojed May 9, 2024
51972e2
Update packages
mofojed May 9, 2024
e6ac2d2
Resolve review comment
mofojed May 9, 2024
6ff6810
Update to use bg-opacity
mofojed May 9, 2024
8e31040
Merge remote-tracking branch 'origin/main' into 1949-errors-not-visible
mofojed May 10, 2024
76ed3f2
Add a little bit of blur
mofojed May 10, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ pre-commit run --all-files
All steps should pass.

To bypass the pre-commit hook, you can commit with the `--no-verify` flag, for example:

```shell
git commit --no-verify -m "commit message"`
```
Expand All @@ -67,7 +68,7 @@ We use [Playwright](https://playwright.dev/) for end-to-end tests. We test again

You should be able to pass arguments to these commands as if you were running Playwright via CLI directly. For example, to test only `matplotlib.spec.ts` you could run `npm run e2e:docker -- ./tests/matplotlib.spec.ts`, or to test only `matplotlib.spec.ts` in Firefox, you could run `npm run e2e:docker -- --project firefox ./tests/matplotlib.spec.ts`. See [Playwright CLI](https://playwright.dev/docs/test-cli) for more details.

It is highly recommended to use `npm run e2e:docker` (instead of `npm run e2e`) as CI also uses the same environment. You can also use `npm run e2e:update-snapshots` to regenerate snapshots in said environment.
It is highly recommended to use `npm run e2e:docker` (instead of `npm run e2e`) as CI also uses the same environment. You can also use `npm run e2e:update-snapshots` to regenerate snapshots in said environment. Run Playwright in [UI Mode](https://playwright.dev/docs/test-ui-mode) with `npm run e2e:ui` when creating new tests or debugging, as this will allow you to run each test individually, see the browser as it runs it, inspect the console, evaluate locators, etc.

### Running Python tests

Expand Down Expand Up @@ -229,6 +230,6 @@ After you have successfully run `tools/release.sh` once, you should be able to d

As part of the release process, `cog` will, per our `cog.toml` configuration, invoke `tools/update_version.sh <packageName> <newVersion>`, which is a script that uses `sed` to update a plugin's version number in whatever source file we happen to use as the source of truth for version information in the given plugin.

*[WARNING]* If you change where the source of truth for a plugin's version is located, you must update `tools/update_version.sh` to update the correct file with a new version number.
_[WARNING]_ If you change where the source of truth for a plugin's version is located, you must update `tools/update_version.sh` to update the correct file with a new version number.

We use `tools/update_version.sh` to remove any `.dev0` "developer version" suffix before creating a release, and to put the `.dev0` version suffix back after completing the release.
814 changes: 407 additions & 407 deletions package-lock.json

Large diffs are not rendered by default.

11 changes: 6 additions & 5 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -16,16 +16,17 @@
"test:ci": "run-p test:ci:*",
"test:ci:unit": "jest --config jest.config.unit.cjs --ci --cacheDirectory $PWD/.jest-cache",
"test:ci:lint": "jest --config jest.config.lint.cjs --ci --cacheDirectory $PWD/.jest-cache",
"e2e": "playwright run",
"e2e": "playwright test",
"e2e:ui": "playwright test --ui",
"e2e:docker": "DEEPHAVEN_PORT=10001 ./tools/run_docker.sh e2e-tests",
"e2e:update-snapshots": "./tools/run_docker.sh update-snapshots",
"update-dh-packages": "lerna run --concurrency 1 update-dh-packages"
},
"devDependencies": {
"@deephaven/babel-preset": "^0.74.0",
"@deephaven/eslint-config": "^0.74.0",
"@deephaven/prettier-config": "^0.74.0",
"@deephaven/tsconfig": "^0.74.0",
"@deephaven/babel-preset": "^0.72.0",
"@deephaven/eslint-config": "^0.72.0",
"@deephaven/prettier-config": "^0.72.0",
"@deephaven/tsconfig": "^0.72.0",
"@playwright/test": "^1.41.2",
"@testing-library/jest-dom": "^5.16.4",
"@testing-library/react": "^12.1.3",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,7 @@ def _render(self) -> None:
# Send the error to the client for displaying to the user
# If there's an error sending it to the client, then it will be caught by the render exception handler
# and logged as an error message.
# Just log it as debug here.
# Just log it as debug here so we don't show it in the console and in the error panel.
stack_trace = traceback.format_exc()
logging.debug("Error rendering document: %s %s", repr(e), stack_trace)
self._send_document_error(e, stack_trace)
Expand Down Expand Up @@ -199,6 +199,8 @@ def _process_callable_queue(self) -> None:
self._render_state = _RenderState.IDLE
except Exception as e:
# Something catastrophic happened, log it and close the connection
# We're just being safe to make sure there is an error logged if something unexpected does occur,
# as `submit_task` does not log any uncaught exceptions currently: https://github.com/deephaven/deephaven-core/issues/5192
logger.exception(e)
self._connection.on_close()

Expand Down Expand Up @@ -369,6 +371,7 @@ def _send_document_error(self, error: Exception, stack_trace: str) -> None:

Args:
error: The error that occurred
stack_trace: The stack trace of the error
"""
request = self._make_notification(
"documentError",
Expand Down
28 changes: 14 additions & 14 deletions plugins/ui/src/js/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -42,21 +42,21 @@
},
"dependencies": {
"@adobe/react-spectrum": "^3.34.1",
"@deephaven/chart": "^0.74.0",
"@deephaven/components": "^0.74.0",
"@deephaven/dashboard": "^0.74.0",
"@deephaven/dashboard-core-plugins": "^0.74.0",
"@deephaven/grid": "^0.74.0",
"@deephaven/icons": "^0.74.0",
"@deephaven/iris-grid": "^0.74.0",
"@deephaven/jsapi-bootstrap": "^0.74.0",
"@deephaven/jsapi-components": "^0.74.0",
"@deephaven/chart": "^0.73.0",
"@deephaven/components": "^0.73.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

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

"@deephaven/dashboard": "^0.73.0",
"@deephaven/dashboard-core-plugins": "^0.73.0",
"@deephaven/grid": "^0.73.0",
"@deephaven/icons": "^0.73.0",
"@deephaven/iris-grid": "^0.73.0",
"@deephaven/jsapi-bootstrap": "^0.73.0",
"@deephaven/jsapi-components": "^0.73.0",
"@deephaven/jsapi-types": "^1.0.0-dev0.33.3",
"@deephaven/log": "^0.74.0",
"@deephaven/plugin": "^0.74.0",
"@deephaven/react-hooks": "^0.74.0",
"@deephaven/redux": "^0.74.0",
"@deephaven/utils": "^0.74.0",
"@deephaven/log": "^0.73.0",
"@deephaven/plugin": "^0.73.0",
"@deephaven/react-hooks": "^0.73.0",
"@deephaven/redux": "^0.73.0",
"@deephaven/utils": "^0.73.0",
"@fortawesome/react-fontawesome": "^0.2.0",
"@react-types/shared": "^3.22.0",
"classnames": "^2.5.1",
Expand Down
4 changes: 2 additions & 2 deletions plugins/ui/src/js/src/DashboardPlugin.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ export function DashboardPlugin(
widgetId: string;
widget: WidgetDescriptor;
}) => {
log.info('Opening widget with ID', widgetId, widget);
log.debug('Opening widget with ID', widgetId, widget);
setWidgetMap(prevWidgetMap => {
const newWidgetMap = new Map(prevWidgetMap);
const oldWidget = newWidgetMap.get(widgetId);
Expand Down Expand Up @@ -213,7 +213,7 @@ export function DashboardPlugin(
);

const handleWidgetClose = useCallback((widgetId: string) => {
log.info('handleWidgetClose', widgetId);
log.debug('handleWidgetClose', widgetId);
setWidgetMap(prevWidgetMap => {
const newWidgetMap = new Map(prevWidgetMap);
newWidgetMap.delete(widgetId);
Expand Down
2 changes: 2 additions & 0 deletions plugins/ui/src/js/src/layout/ReactPanel.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import { ReactPanelProps } from './LayoutUtils';
import { useParentItem } from './ParentItemContext';
import { ReactPanelContext } from './ReactPanelContext';
import { usePortalPanelManager } from './PortalPanelManagerContext';
import ReactPanelContentOverlay from './ReactPanelContentOverlay';

const log = Log.module('@deephaven/js-plugin-ui/ReactPanel');

Expand Down Expand Up @@ -201,6 +202,7 @@ function ReactPanel({
{children}
</Flex>
</View>
<ReactPanelContentOverlay />
</ReactPanelContext.Provider>,
portal,
contentKey
Expand Down
13 changes: 13 additions & 0 deletions plugins/ui/src/js/src/layout/ReactPanelContentOverlay.tsx
Original file line number Diff line number Diff line change
@@ -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 */
Copy link
Contributor

Choose a reason for hiding this comment

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

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


import React from 'react';
import { usePanelContentOverlay } from './usePanelContentOverlay';

export function ReactPanelContentOverlay(): JSX.Element | null {
const overlayContent = usePanelContentOverlay();
return overlayContent != null ? (
<div className="dh-react-panel-overlay">{overlayContent}</div>
) : null;
}

export default ReactPanelContentOverlay;
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
import { createContext } from 'react';

/** Context that defined a ReactNode to overlay on top of the content in a ReactPanel */
export const ReactPanelContentOverlayContext =
createContext<React.ReactNode | null>(null);

export default ReactPanelContentOverlayContext;
12 changes: 12 additions & 0 deletions plugins/ui/src/js/src/layout/usePanelContentOverlay.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
import { useContext } from 'react';
import { ReactPanelContentOverlayContext } from './ReactPanelContentOverlayContext';

/**
* Gets the overlay content from the nearest panel context.
* @returns The overlay content or null if not in a panel
*/
export function usePanelContentOverlay(): React.ReactNode | null {
return useContext(ReactPanelContentOverlayContext);
}

export default usePanelContentOverlay;
139 changes: 96 additions & 43 deletions plugins/ui/src/js/src/styles.scss
Original file line number Diff line number Diff line change
Expand Up @@ -31,84 +31,137 @@
}
}

.widget-error-view {
.ui-widget-error-view {
display: flex;
flex-direction: column;
gap: $spacer-1;
mofojed marked this conversation as resolved.
Show resolved Hide resolved
flex-grow: 1;
max-height: 100%;

.widget-error-view-content {
position: relative;
flex-shrink: 1;
overflow: hidden;
display: flex;
flex-direction: column;
flex-grow: 1;
}

.widget-error-view-footer {
display: flex;
justify-content: flex-end;
align-items: center;
gap: $spacer-1;
flex-wrap: wrap;
flex-shrink: 0;
}
}

// TODO: Remove once using the @deephaven/components ErrorView: https://github.com/deephaven/web-client-ui/pull/1965
.error-view {
.ui-error-view {
position: relative;
color: $danger;
color: var(--dh-color-negative-bg);
border-radius: $border-radius;
background-color: negative-opacity($exception-transparency);
display: flex;
flex-direction: column;
flex-grow: 0;
flex-grow: 1;
font-family: $font-family-monospace;
transition: all $transition ease-in-out;
max-height: 150px;

&.expanded {
flex-grow: 1;
max-height: 100%;
}
transition: flex-grow $transition ease-in-out;

font-family: $font-family-monospace;
.error-view-header {
display: flex;
flex-direction: row;
justify-content: space-between;
text-wrap: nowrap;
max-width: 100%;

label {
display: block;
padding: $spacer;
font-weight: bold;
.error-view-header-text {
display: flex;
flex-direction: row;
align-items: center;
padding-left: $spacer;
padding-right: $spacer;
font-weight: bold;
text-overflow: ellipsis;
flex-shrink: 1;
overflow: hidden;
white-space: nowrap;

span {
flex-shrink: 1;
overflow: hidden;
white-space: nowrap;
text-overflow: ellipsis;
padding-left: $spacer-1;
}
}

.error-view-buttons {
display: flex;
flex-direction: row;
gap: 1px;
overflow: hidden;
border-radius: 0 $border-radius 0 0;
flex-shrink: 0;

.btn-danger {
border-radius: 0;
color: $black;
opacity: 0.8;
padding: $spacer-1;
&:active {
color: $black;
}
}

.error-view-copy-button {
min-width: 3rem;
}

.error-view-expand-button {
svg {
margin-right: $spacer-1;
}
}
}
}

textarea {
width: 100%;
.error-view-text {
max-width: 100%;
padding: $spacer;
color: $danger;
margin-bottom: 0;
color: var(--dh-color-negative-bg);
background-color: transparent;
border: 0;
resize: none;
outline: none;
min-height: 100px;
transition: height $transition ease-in-out;
white-space: pre;
flex-grow: 1;
overflow: auto;
}
}

.error-view-buttons {
display: flex;
flex-direction: row;
gap: 1px;
position: absolute;
top: 0;
right: 0;

.btn-danger {
color: $black;
opacity: 0.8;
padding: $spacer-1;
&:active {
color: $black;
}
}

.error-view-copy-button {
border-radius: 0;
min-width: 3rem;
}
.dh-react-panel-overlay {
background-color: black-opacity(90);
position: absolute;
top: 0;
left: 0;
right: 0;
bottom: 0;
padding: $spacer;
display: flex;
flex-direction: column;
justify-content: center;
align-items: center;
z-index: 1000;

.error-view-expand-button {
border-radius: 0 $border-radius 0 0;
svg {
margin-right: $spacer-1;
}
}
.ui-widget-error-view {
max-width: 100%;
}
}
Loading
Loading