fix: improve ipynb loading hitches (#132)

* fix: improve ipynb loading hitches this fixes three issues that happen when loading an ipynb file in the code editor: 1) when an ipynb file was selected and the file content wasn't loaded, the error handler was called 2) when a parse error occurred, the error handler was called with incorrect arguments 3) when loading an ipynb file, the unparsed file content renders to the page briefly even when the content is valid We also update the loadable usage in the codeeditor to use the loadable class. * 0.6.42-ET741.0 * fix filename container shrinking during load * 0.6.42-ET741.1 * add comments --------- Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
determined-ai · Aug 22, 2024 · 7ddd2b5 · 7ddd2b5
1 parent 8bc776f
commit 7ddd2b5
Show file tree

Hide file tree

Showing 5 changed files with 101 additions and 94 deletions.
diff --git a/package-lock.json b/package-lock.json
diff --git a/package.json b/package.json
@@ -1,6 +1,6 @@
 {
   "name": "@hpe.com/hew",
-  "version": "0.6.41",
+  "version": "0.6.42-ET741.1",
   "type": "module",
   "scripts": {
     "dev": "vite",

diff --git a/src/kit/CodeEditor.tsx b/src/kit/CodeEditor.tsx
@@ -13,7 +13,7 @@ import Message from 'kit/Message';
 import Spinner from 'kit/Spinner';
 import { useTheme } from 'kit/Theme';
 import { ErrorHandler } from 'kit/utils/error';
-import { Loadable, Loaded, NotLoaded } from 'kit/utils/loadable';
+import { Loadable } from 'kit/utils/loadable';
 import { TreeNode, ValueOf } from 'kit/utils/types';
 
 const JupyterRenderer = lazy(() => import('./CodeEditor/IpynbRenderer'));
@@ -100,6 +100,10 @@ const langs = {
   yaml: () => StreamLanguage.define(yaml),
 };
 
+const emptyIpynbFile = JSON.stringify({
+  cells: [],
+});
+
 /**
  * A component responsible to enable the user to view the code for a experiment.
  *
@@ -126,7 +130,7 @@ const CodeEditor: React.FC<Props> = ({
   readonly,
   selectedFilePath = String(files[0]?.key),
 }) => {
-  const loadableFile = useMemo(() => (typeof file === 'string' ? Loaded(file) : file), [file]);
+  const loadableFile = useMemo(() => Loadable.ensureLoadable(file), [file]);
   const sortedFiles = useMemo(() => [...files].sort(sortTree), [files]);
   const {
     themeSettings: { themeIsDark, className: themeClass },
@@ -193,8 +197,7 @@ const CodeEditor: React.FC<Props> = ({
   );
 
   const handleDownloadClick = useCallback(() => {
-    if (!Loadable.isLoadable(loadableFile) || !Loadable.isLoaded(loadableFile) || !activeFile)
-      return;
+    if (!loadableFile.isLoaded || !activeFile) return;
 
     const link = document.createElement('a');
 
@@ -215,7 +218,7 @@ const CodeEditor: React.FC<Props> = ({
     themeClass,
   ];
 
-  const sectionClasses = [loadableFile.isFailed ? css.pageError : css.editor];
+  const sectionClass = loadableFile.isFailed ? css.pageError : css.editor;
 
   const treeClasses = [css.fileTree, viewMode === 'editor' ? css.hideElement : ''];
 
@@ -237,7 +240,10 @@ const CodeEditor: React.FC<Props> = ({
         />
       ) : (
         <Suspense fallback={<Spinner spinning tip="Loading ipynb viewer..." />}>
-          <JupyterRenderer file={Loadable.getOrElse('', loadableFile)} onError={onError} />
+          <JupyterRenderer
+            file={Loadable.getOrElse(emptyIpynbFile, loadableFile)}
+            onError={onError}
+          />
         </Suspense>
       );
   }
@@ -253,39 +259,36 @@ const CodeEditor: React.FC<Props> = ({
         onSelect={handleSelectFile}
       />
       {!!activeFile?.title && (
-        <div className={css.fileDir}>
-          <div className={css.fileInfo}>
-            <div className={css.buttonContainer}>
-              <>
-                {activeFile.icon ?? <Icon decorative name="document" />}
-                <span className={css.filePath}>
-                  <>{activeFile.title}</>
-                </span>
-                {activeFile?.subtitle && (
-                  <span className={css.fileDesc}> {activeFile?.subtitle}</span>
-                )}
-                {readonly && <span className={css.readOnly}>read-only</span>}
-              </>
-            </div>
-            <div className={css.buttonsContainer}>
-              {/*
-               * TODO: Add notebook integration
-               * <Button type="text">Open in Notebook</Button>
-               */}
-              {readonly && file !== NotLoaded && (
-                <Button
-                  icon={<Icon name="download" showTooltip size="small" title="Download File" />}
-                  type="text"
-                  onClick={handleDownloadClick}
-                />
+        <div className={css.fileInfo}>
+          <div className={css.buttonContainer}>
+            <>
+              {activeFile.icon ?? <Icon decorative name="document" />}
+              <span className={css.filePath}>
+                <>{activeFile.title}</>
+              </span>
+              {activeFile?.subtitle && (
+                <span className={css.fileDesc}> {activeFile?.subtitle}</span>
               )}
-            </div>
+              {readonly && <span className={css.readOnly}>read-only</span>}
+            </>
+          </div>
+          <div className={css.buttonsContainer}>
+            {/*
+             * TODO: Add notebook integration
+             * <Button type="text">Open in Notebook</Button>
+             */}
+            {readonly && !loadableFile.isNotLoaded && (
+              <Button
+                icon={<Icon name="download" showTooltip size="small" title="Download File" />}
+                type="text"
+                onClick={handleDownloadClick}
+              />
+            )}
           </div>
         </div>
       )}
-      <div className={sectionClasses.join(' ')}>
-        {/* directly checking tag because loadable.isLoaded only takes loadables */}
-        <Spinner spinning={file === NotLoaded}>{fileContent}</Spinner>
+      <div className={sectionClass}>
+        <Spinner spinning={loadableFile.isNotLoaded}>{fileContent}</Spinner>
       </div>
     </div>
   );

diff --git a/src/kit/CodeEditor/CodeEditor.module.scss b/src/kit/CodeEditor/CodeEditor.module.scss
@@ -1,7 +1,7 @@
 .codeEditorBase {
   display: grid;
   grid-template:
-    'tree title' minmax(38px, min-content)
+    'tree title' minmax(40px, min-content)
     'tree editor' auto / clamp(300px, 25vw, 400px) minmax(0, auto);
   max-width: 100vw;
   min-height: 250px;
@@ -27,38 +27,35 @@
     margin-top: 5em;
     text-align: center;
   }
-  .fileDir {
+  .fileInfo {
+    align-items: center;
+    background-color: var(--theme-stage);
+    border: solid var(--theme-stroke-width) var(--theme-stage-border);
+    border-bottom: none;
+    border-radius: 0;
+    display: flex;
     grid-area: title;
+    justify-content: space-between;
+    padding: 0.25em 1em;
 
-    .fileInfo {
+    .buttonContainer {
       align-items: center;
-      background-color: var(--theme-stage);
-      border: solid var(--theme-stroke-width) var(--theme-stage-border);
-      border-bottom: none;
-      border-radius: 0;
       display: flex;
       justify-content: space-between;
-      padding: 0.25em 1em;
-
-      .buttonContainer {
-        align-items: center;
-        display: flex;
-        justify-content: space-between;
-      }
-      .filePath {
-        margin-left: 10px;
-      }
-      .fileDesc,
-      .readOnly {
-        color: var(--theme-stage-on-weak);
-        margin-left: var(--spacing-xl-2);
-      }
-      .readOnly {
-        font-variant: small-caps;
-      }
-      .buttonsContainer {
-        display: flex;
-      }
+    }
+    .filePath {
+      margin-left: 10px;
+    }
+    .fileDesc,
+    .readOnly {
+      color: var(--theme-stage-on-weak);
+      margin-left: var(--spacing-xl-2);
+    }
+    .readOnly {
+      font-variant: small-caps;
+    }
+    .buttonsContainer {
+      display: flex;
     }
   }
   .editor {

diff --git a/src/kit/CodeEditor/IpynbRenderer.tsx b/src/kit/CodeEditor/IpynbRenderer.tsx
@@ -1,4 +1,6 @@
-import React, { useEffect, useState } from 'react';
+import { mapLeft, match, tryCatch } from 'fp-ts/Either';
+import { pipe } from 'fp-ts/function';
+import React, { useEffect, useMemo } from 'react';
 
 import { ErrorHandler, ErrorType } from 'kit/utils/error';
 import NotebookJS from 'notebook';
@@ -10,38 +12,43 @@ interface Props {
   onError: ErrorHandler;
 }
 
-export const parseNotebook = (file: string, onError: ErrorHandler): string => {
-  try {
-    const json = JSON.parse(file);
-    const notebookJS = NotebookJS.parse(json);
-    return notebookJS.render().outerHTML;
-  } catch (e) {
-    onError('Unable to parse as Notebook!');
-    return '';
-  }
+export const parseNotebook = (file: string): string => {
+  const json = JSON.parse(file);
+  const notebookJS = NotebookJS.parse(json);
+  return notebookJS.render().outerHTML;
 };
 
 const JupyterRenderer: React.FC<Props> = React.memo(({ file, onError }) => {
-  const [__html, setHTML] = useState<string>();
+  // parse the file and store the result as either a successful string or a failed error
+  const parseResult = useMemo(() => {
+    return tryCatch(
+      () => parseNotebook(file),
+      (e) => e,
+    );
+  }, [file]);
 
   useEffect(() => {
-    try {
-      const html = parseNotebook(file, onError);
-      setHTML(html);
-    } catch (error) {
-      onError(error, {
-        publicMessage: 'Failed to load selected notebook.',
-        publicSubject: 'Unable to parse the selected notebook.',
-        silent: true,
-        type: ErrorType.Input,
-      });
-    }
-  }, [file, onError]);
-
-  return __html ? (
-    <div className="ipynb-renderer-root" dangerouslySetInnerHTML={{ __html }} />
-  ) : (
-    <div>{file}</div>
+    // if the parse result is failed, call the error handler
+    pipe(
+      parseResult,
+      mapLeft((e) => {
+        onError(e, {
+          publicMessage: 'Failed to load selected notebook.',
+          publicSubject: 'Unable to parse the selected notebook.',
+          silent: true,
+          type: ErrorType.Input,
+        });
+      }),
+    );
+  }, [parseResult, onError]);
+
+  // if the parse result is failed, fall back to the raw file, otherwise show the parse result html
+  return pipe(
+    parseResult,
+    match(
+      () => <div>{file}</div>,
+      (__html) => <div className="ipynb-renderer-root" dangerouslySetInnerHTML={{ __html }} />,
+    ),
   );
 });