styling: responsive "add cell buttons" and reduce placeholder color (m…

…arimo-team#1442) * styling: responsive "add cell buttons" and reduce placeholder color * supsense * tracking * nit * close ws * fix preload
Light2Dark · May 22, 2024 · 9a5472f · 9a5472f
1 parent da6675d
commit 9a5472f
Show file tree

Hide file tree

Showing 8 changed files with 77 additions and 61 deletions.
diff --git a/frontend/e2e-tests/bugs.spec.ts b/frontend/e2e-tests/bugs.spec.ts
@@ -1,13 +1,14 @@
 /* Copyright 2024 Marimo. All rights reserved. */
 import { test, expect } from "@playwright/test";
 import { getAppUrl } from "../playwright.config";
-import { createCellBelow, runCell } from "./helper";
+import { createCellBelow, maybeRestartKernel, runCell } from "./helper";
 
 const appUrl = getAppUrl("bugs.py");
 test.beforeEach(async ({ page }, info) => {
   await page.goto(appUrl);
   if (info.retry) {
     await page.reload();
+    await maybeRestartKernel(page);
   }
 });
 

diff --git a/frontend/src/components/editor/chrome/wrapper/app-chrome.tsx b/frontend/src/components/editor/chrome/wrapper/app-chrome.tsx
@@ -1,10 +1,10 @@
 /* Copyright 2024 Marimo. All rights reserved. */
-import React, { PropsWithChildren, useEffect } from "react";
+import React, { type PropsWithChildren, useEffect, Suspense } from "react";
 import {
   PanelGroup,
   Panel,
   PanelResizeHandle,
-  ImperativePanelHandle,
+  type ImperativePanelHandle,
 } from "react-resizable-panels";
 import { Footer } from "./footer";
 import "./app-chrome.css";
@@ -57,7 +57,7 @@ export const AppChrome: React.FC<PropsWithChildren> = ({ children }) => {
 
   const appBody = (
     <Panel id="app" key={`app-${panelLocation}`} className="relative h-full">
-      {children}
+      <Suspense>{children}</Suspense>
     </Panel>
   );
 
@@ -78,30 +78,32 @@ export const AppChrome: React.FC<PropsWithChildren> = ({ children }) => {
   );
 
   const helpPaneBody = (
-    <div className="flex flex-col h-full flex-1 overflow-hidden mr-[-4px]">
-      <div className="p-3 border-b flex justify-between items-center">
-        <div className="text-sm text-[var(--slate-11)] uppercase tracking-wide font-semibold flex-1">
-          {selectedPanel}
+    <Suspense>
+      <div className="flex flex-col h-full flex-1 overflow-hidden mr-[-4px]">
+        <div className="p-3 border-b flex justify-between items-center">
+          <div className="text-sm text-[var(--slate-11)] uppercase tracking-wide font-semibold flex-1">
+            {selectedPanel}
+          </div>
+          <Button
+            data-testid="close-helper-pane"
+            className="m-0"
+            size="xs"
+            variant="text"
+            onClick={() => setIsOpen(false)}
+          >
+            <XIcon className="w-4 h-4" />
+          </Button>
         </div>
-        <Button
-          data-testid="close-helper-pane"
-          className="m-0"
-          size="xs"
-          variant="text"
-          onClick={() => setIsOpen(false)}
-        >
-          <XIcon className="w-4 h-4" />
-        </Button>
+        {selectedPanel === "files" && <FileExplorerPanel />}
+        {selectedPanel === "errors" && <ErrorsPanel />}
+        {selectedPanel === "variables" && <VariablePanel />}
+        {selectedPanel === "dependencies" && <DependencyGraphPanel />}
+        {selectedPanel === "outline" && <OutlinePanel />}
+        {selectedPanel === "documentation" && <DocumentationPanel />}
+        {selectedPanel === "snippets" && <SnippetsPanel />}
+        {selectedPanel === "logs" && <LogsPanel />}
       </div>
-      {selectedPanel === "files" && <FileExplorerPanel />}
-      {selectedPanel === "errors" && <ErrorsPanel />}
-      {selectedPanel === "variables" && <VariablePanel />}
-      {selectedPanel === "dependencies" && <DependencyGraphPanel />}
-      {selectedPanel === "outline" && <OutlinePanel />}
-      {selectedPanel === "documentation" && <DocumentationPanel />}
-      {selectedPanel === "snippets" && <SnippetsPanel />}
-      {selectedPanel === "logs" && <LogsPanel />}
-    </div>
+    </Suspense>
   );
 
   const helperPane = (

diff --git a/frontend/src/components/editor/file-tree/file-explorer.tsx b/frontend/src/components/editor/file-tree/file-explorer.tsx
@@ -1,7 +1,13 @@
 /* Copyright 2024 Marimo. All rights reserved. */
-import { NodeApi, NodeRendererProps, Tree } from "react-arborist";
+import { type NodeApi, type NodeRendererProps, Tree } from "react-arborist";
 
-import React, { useContext, useEffect, useRef, useState } from "react";
+import React, {
+  Suspense,
+  useContext,
+  useEffect,
+  useRef,
+  useState,
+} from "react";
 import {
   ArrowLeftIcon,
   ChevronDownIcon,
@@ -15,10 +21,10 @@ import {
   UploadIcon,
   ViewIcon,
 } from "lucide-react";
-import { FileInfo } from "@/core/network/types";
+import type { FileInfo } from "@/core/network/types";
 import {
   FILE_TYPE_ICONS,
-  FileType,
+  type FileType,
   PYTHON_CODE_FOR_FILE_TYPE,
   guessFileType,
 } from "./types";
@@ -44,7 +50,7 @@ import { isPyodide } from "@/core/pyodide/utils";
 import { useAsyncData } from "@/hooks/useAsyncData";
 import { ErrorBanner } from "@/plugins/impl/common/error-banner";
 import { Spinner } from "@/components/icons/spinner";
-import { RequestingTree } from "./requesting-tree";
+import type { RequestingTree } from "./requesting-tree";
 
 const RequestingTreeContext = React.createContext<RequestingTree | null>(null);
 
@@ -81,7 +87,9 @@ export const FileExplorer: React.FC<{
           </Button>
           <span className="font-bold">{openFile.name}</span>
         </div>
-        <FileViewer file={openFile} />
+        <Suspense>
+          <FileViewer file={openFile} />
+        </Suspense>
       </>
     );
   }

diff --git a/frontend/src/components/editor/renderers/CellArray.tsx b/frontend/src/components/editor/renderers/CellArray.tsx
@@ -156,7 +156,7 @@ const AddCellButtons: React.FC = () => {
   const aiEnabled = useAtomValue(aiEnabledAtom);
 
   const buttonClass = cn(
-    "px-16 mb-0 rounded-none",
+    "mb-0 rounded-none px-4 sm:px-8 md:px-10 lg:px-16 tracking-wide",
     "hover:bg-accent hover:text-accent-foreground font-semibold uppercase text-xs",
   );
 
@@ -212,7 +212,7 @@ const AddCellButtons: React.FC = () => {
     <div className="flex justify-center mt-4 pt-6 pb-32 group gap-4 w-full">
       <div
         className={cn(
-          "shadow-sm border border-border rounded  transition-all duration-200 overflow-hidden divide-x divide-border",
+          "shadow-sm border border-border rounded transition-all duration-200 overflow-hidden divide-x divide-border flex",
           !isAiButtonOpen && "opacity-0 group-hover:opacity-100 w-fit",
           isAiButtonOpen && "opacity-100 w-full max-w-4xl shadow-lg",
         )}

diff --git a/frontend/src/core/MarimoApp.tsx b/frontend/src/core/MarimoApp.tsx
@@ -3,7 +3,8 @@ import "../css/index.css";
 import "../css/app/App.css";
 import "iconify-icon";
 
-import React, { PropsWithChildren, Suspense, memo } from "react";
+import type React from "react";
+import { type PropsWithChildren, Suspense, memo } from "react";
 import { ErrorBoundary } from "../components/editor/boundary/ErrorBoundary";
 import { TooltipProvider } from "../components/ui/tooltip";
 import { Toaster } from "../components/ui/toaster";
@@ -50,13 +51,13 @@ export const MarimoApp: React.FC = memo(() => {
   const renderBody = () => {
     if (initialMode === "home") {
       return <LazyHomePage.Component />;
-    } else if (initialMode === "read") {
+    }
+    if (initialMode === "read") {
       return <LazyRunPage.Component appConfig={appConfig} />;
-    } else {
-      return (
-        <LazyEditPage.Component userConfig={userConfig} appConfig={appConfig} />
-      );
     }
+    return (
+      <LazyEditPage.Component userConfig={userConfig} appConfig={appConfig} />
+    );
   };
 
   return (

diff --git a/frontend/src/core/codemirror/placeholder/extensions.ts b/frontend/src/core/codemirror/placeholder/extensions.ts
@@ -1,5 +1,5 @@
 /* Copyright 2024 Marimo. All rights reserved. */
-import { Extension } from "@codemirror/state";
+import type { Extension } from "@codemirror/state";
 import { EditorView, keymap, placeholder } from "@codemirror/view";
 
 /**
@@ -73,12 +73,14 @@ export function clickablePlaceholderExtension(opts: {
   return [
     placeholder(placeholderText),
     EditorView.theme({
+      ".cm-placeholder": {
+        color: "var(--slate-8)",
+      },
       ".cm-clickable-placeholder": {
         cursor: "pointer !important",
         pointerEvents: "auto",
-        color: "var(--slate-10)",
+        color: "var(--slate-9)",
         textDecoration: "underline",
-        textDecorationThickness: "0.1em",
         textUnderlineOffset: "0.2em",
       },
       ".cm-clickable-placeholder:hover": {

diff --git a/frontend/src/core/websocket/useWebSocket.tsx b/frontend/src/core/websocket/useWebSocket.tsx
@@ -1,10 +1,11 @@
 /* Copyright 2024 Marimo. All rights reserved. */
 import { useEffect, useState } from "react";
 import ReconnectingWebSocket from "partysocket/ws";
-import { IReconnectingWebSocket } from "./types";
+import type { IReconnectingWebSocket } from "./types";
 import { StaticWebsocket } from "./StaticWebsocket";
 import { isPyodide } from "../pyodide/utils";
 import { PyodideBridge, PyodideWebsocket } from "../pyodide/bridge";
+import { Logger } from "@/utils/Logger";
 
 interface UseWebSocketOptions {
   url: string;
@@ -15,6 +16,8 @@ interface UseWebSocketOptions {
   onError?: (event: WebSocketEventMap["error"]) => void;
 }
 
+let hasMounted = false;
+
 /**
  * A hook for creating a WebSocket connection with React.
  *
@@ -25,6 +28,10 @@ export function useWebSocket(options: UseWebSocketOptions) {
 
   // eslint-disable-next-line react/hook-use-state
   const [ws] = useState<IReconnectingWebSocket>(() => {
+    if (hasMounted) {
+      Logger.warn("useWebSocket should only be called once.");
+    }
+    hasMounted = true;
     const socket: IReconnectingWebSocket = isPyodide()
       ? new PyodideWebsocket(PyodideBridge.INSTANCE)
       : options.static
@@ -45,13 +52,17 @@ export function useWebSocket(options: UseWebSocketOptions) {
 
   useEffect(() => {
     return () => {
+      Logger.warn(
+        "useWebSocket is unmounting. This likely means there is a bug.",
+      );
+      ws.close();
       onOpen && ws.removeEventListener("open", onOpen);
       onClose && ws.removeEventListener("close", onClose);
       onError && ws.removeEventListener("error", onError);
       onMessage && ws.removeEventListener("message", onMessage);
     };
     // eslint-disable-next-line react-hooks/exhaustive-deps
-  }, []);
+  }, [ws]);
 
   return ws;
 }
diff --git a/frontend/src/utils/lazy.ts b/frontend/src/utils/lazy.ts
@@ -10,26 +10,17 @@ interface LazyComponentWithPreload<T> {
 export const reactLazyWithPreload = <T>(
   factory: () => Promise<{ default: React.ComponentType<T> }>,
 ): LazyComponentWithPreload<T> => {
-  let component: React.ComponentType<T> | null = null;
-  const init = factory().then((module) => {
-    component = module.default;
-  });
+  let component: Promise<{ default: React.ComponentType<T> }> | null = null;
 
-  const preload = () => {
-    init.then(() => {
-      if (!component) {
-        throw new Error("Component is not loaded");
-      }
-    });
+  const preload = async () => {
+    if (!component) {
+      component = factory();
+    }
+    return component;
   };
 
   const LazyComponent = React.lazy(() => {
-    return init.then(() => {
-      if (!component) {
-        throw new Error("Component is not loaded");
-      }
-      return { default: component };
-    });
+    return preload();
   });
 
   return {