Clean up based on review

mofojed · Apr 26, 2024 · 2eb1140 · 2eb1140
1 parent 3200e29
commit 2eb1140
Show file tree

Hide file tree

Showing 6 changed files with 42 additions and 31 deletions.
diff --git a/plugins/ui/src/deephaven/ui/object_types/ElementMessageStream.py b/plugins/ui/src/deephaven/ui/object_types/ElementMessageStream.py
@@ -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)
@@ -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()
 
@@ -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",

diff --git a/plugins/ui/src/js/src/DashboardPlugin.tsx b/plugins/ui/src/js/src/DashboardPlugin.tsx
@@ -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);
@@ -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);

diff --git a/plugins/ui/src/js/src/styles.scss b/plugins/ui/src/js/src/styles.scss
@@ -57,7 +57,7 @@
 // TODO: Remove once using the @deephaven/components ErrorView: https://github.com/deephaven/web-client-ui/pull/1965
 .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;
@@ -133,7 +133,7 @@
     width: 100%;
     padding: $spacer;
     margin-bottom: 0;
-    color: $danger;
+    color: var(--dh-color-negative-bg);
     background-color: transparent;
     border: 0;
     resize: none;

diff --git a/plugins/ui/src/js/src/widget/DocumentHandler.tsx b/plugins/ui/src/js/src/widget/DocumentHandler.tsx
@@ -52,9 +52,14 @@ function DocumentHandler({
   onClose,
 }: DocumentHandlerProps): JSX.Element {
   log.debug('Rendering document', widget);
+  // We want to know if we're in the middle of an update, so we don't send a close event if we're in the middle of an update
+  // We set this as the first thing of this function, then set it to false at the end in a `useEffect` so it gets set after everything else
+  // is done in the render cycle.
+  const isUpdating = useRef(true);
+  isUpdating.current = true;
+
   const panelOpenCountRef = useRef(0);
   const panelIdIndex = useRef(0);
-  const isUpdating = useRef(false);
 
   // Using `useState` here to initialize the data only once.
   // We don't want to use `useMemo`, because we only want it to be initialized once with the `initialData` (uncontrolled)
@@ -125,8 +130,8 @@ function DocumentHandler({
     [widget, getPanelId, handleClose, handleOpen]
   );
 
-  isUpdating.current = true;
   useEffect(() => {
+    // Reset the updating flag after everything else is done
     isUpdating.current = false;
   });
 

diff --git a/plugins/ui/src/js/src/widget/WidgetErrorView.tsx b/plugins/ui/src/js/src/widget/WidgetErrorView.tsx
@@ -1,5 +1,6 @@
 import React from 'react';
 import { Button } from '@deephaven/components';
+import { vsRefresh } from '@deephaven/icons';
 import ErrorView from './ErrorView';
 import { WidgetError } from './WidgetTypes';
 
@@ -11,14 +12,16 @@ function WidgetErrorView({
   error: WidgetError;
   onReload: () => void;
 }): JSX.Element {
-  const displayMessage = `${error.message}\n\n${error.stack ?? ''}`.trim();
+  const displayMessage = `${error.message.trim()}\n\n${
+    error.stack ?? ''
+  }`.trim();
   return (
     <div className="widget-error-view">
       <div className="widget-error-view-content">
         <ErrorView message={displayMessage} type={error.type} isExpanded />
       </div>
       <div className="widget-error-view-footer">
-        <Button kind="tertiary" onClick={onReset}>
+        <Button kind="tertiary" icon={vsRefresh} onClick={onReset}>
           Reload
         </Button>
       </div>

diff --git a/plugins/ui/src/js/src/widget/WidgetHandler.tsx b/plugins/ui/src/js/src/widget/WidgetHandler.tsx
@@ -66,6 +66,7 @@ function WidgetHandler({
 }: WidgetHandlerProps): JSX.Element | null {
   const [widget, setWidget] = useState<dh.Widget>();
   const [document, setDocument] = useState<ReactNode>();
+  const [error, setError] = useState<WidgetError>();
   const [initialData] = useState(initialDataProp);
 
   // When we fetch a widget, the client is then responsible for the exported objects.
@@ -102,25 +103,13 @@ function WidgetHandler({
         },
         e => {
           log.error('Error setting state: ', e);
+          setError(e);
         }
       );
     },
     [jsonClient]
   );
 
-  const setDocumentError = useCallback(
-    (error: WidgetError) => {
-      // When we get an error for the server, we want to display it to the user.
-      // Display the error in a panel that the user can see.
-      setDocument(
-        <ReactPanel>
-          <WidgetErrorView error={error} onReload={() => sendSetState()} />
-        </ReactPanel>
-      );
-    },
-    [sendSetState]
-  );
-
   const parseDocument = useCallback(
     /**
      * Parse the data from the server, replacing some of the nodes on the way.
@@ -216,6 +205,7 @@ function WidgetHandler({
           log.debug2(METHOD_DOCUMENT_UPDATED, params);
           const [documentParam, stateParam] = params;
           const newDocument = parseDocument(documentParam);
+          setError(undefined);
           setDocument(newDocument);
           if (stateParam != null) {
             try {
@@ -233,15 +223,15 @@ function WidgetHandler({
 
       jsonClient.addMethod(METHOD_DOCUMENT_ERROR, (params: [string]) => {
         log.error('Document error', params);
-        const error: WidgetError = JSON.parse(params[0]);
-        setDocumentError(error);
+        const newError: WidgetError = JSON.parse(params[0]);
+        setError(newError);
       });
 
       return () => {
         jsonClient.rejectAllPendingRequests('Widget was changed');
       };
     },
-    [jsonClient, onDataChange, parseDocument, setDocumentError]
+    [jsonClient, onDataChange, parseDocument]
   );
 
   /**
@@ -272,9 +262,7 @@ function WidgetHandler({
           try {
             await activeClient.receiveAndSend(JSON.parse(data));
           } catch (e) {
-            // We already have an `errorListener` registered when declaring the JSONRPCServerAndClient,
-            // and that contains more information than this error does, so just use that.
-            log.debug('Error receiving data', e);
+            log.error('Error receiving data', e);
           }
         }
       }
@@ -340,19 +328,31 @@ function WidgetHandler({
     [fetch, descriptor]
   );
 
+  const renderedDocument = useMemo(
+    () =>
+      error != null ? (
+        <ReactPanel>
+          <WidgetErrorView error={error} onReload={() => sendSetState()} />
+        </ReactPanel>
+      ) : (
+        document
+      ),
+    [document, error, sendSetState]
+  );
+
   return useMemo(
     () =>
-      document != null ? (
+      renderedDocument != null ? (
         <DocumentHandler
           widget={descriptor}
           initialData={initialData}
           onDataChange={onDataChange}
           onClose={onClose}
         >
-          {document}
+          {renderedDocument}
         </DocumentHandler>
       ) : null,
-    [document, descriptor, initialData, onClose, onDataChange]
+    [renderedDocument, descriptor, initialData, onClose, onDataChange]
   );
 }