enh: make viz responsive, remove extra renders, handle err in main wi…

…ndow (#6629) * enh: improve viz responsiveness, remove extra renders, handle error in main * mark last viz as complete on agent message success * naming * nit * remove outdated comment * r --------- Co-authored-by: Henry Fontanier <henry@dust.tt>
dust-tt · Aug 1, 2024 · 65cd136 · 65cd136
1 parent 250c0b3
commit 65cd136
Show file tree

Hide file tree

Showing 9 changed files with 96 additions and 183 deletions.
diff --git a/front/components/assistant/conversation/AgentMessage.tsx b/front/components/assistant/conversation/AgentMessage.tsx
@@ -211,6 +211,12 @@ export function AgentMessage({
             ...event.message,
           };
         });
+        // Mark the last viz as complete if it is not already.
+        setVisualizations((v) =>
+          v.map((item, index) =>
+            index === v.length - 1 ? { ...item, complete: true } : item
+          )
+        );
         break;
       }
 

diff --git a/front/components/assistant/conversation/actions/VisualizationActionIframe.tsx b/front/components/assistant/conversation/actions/VisualizationActionIframe.tsx
@@ -1,4 +1,4 @@
-import { Spinner } from "@dust-tt/sparkle";
+import { Button, Spinner } from "@dust-tt/sparkle";
 import type {
   CommandResultMap,
   VisualizationRPCCommand,
@@ -38,14 +38,14 @@ const sendResponseToIframe = <T extends VisualizationRPCCommand>(
 // Custom hook to encapsulate the logic for handling visualization messages.
 function useVisualizationDataHandler({
   visualization,
-  onRetry,
   setContentHeight,
+  setIsErrored,
   vizIframeRef,
   workspaceId,
 }: {
   visualization: Visualization;
-  onRetry: () => void;
   setContentHeight: (v: SetStateAction<number>) => void;
+  setIsErrored: (v: SetStateAction<boolean>) => void;
   vizIframeRef: React.MutableRefObject<HTMLIFrameElement | null>;
   workspaceId: string;
 }) {
@@ -98,14 +98,14 @@ function useVisualizationDataHandler({
 
           break;
 
-        case "retry":
-          onRetry();
-          break;
-
         case "setContentHeight":
           setContentHeight(data.params.height);
           break;
 
+        case "setErrored":
+          setIsErrored(true);
+          break;
+
         default:
           assertNever(data);
       }
@@ -117,8 +117,8 @@ function useVisualizationDataHandler({
     visualization.identifier,
     code,
     getFileBlob,
-    onRetry,
     setContentHeight,
+    setIsErrored,
     vizIframeRef,
   ]);
 }
@@ -130,45 +130,41 @@ export function VisualizationActionIframe({
 }: {
   owner: WorkspaceType;
   visualization: Visualization;
-
   onRetry: () => void;
 }) {
-  const [contentHeight, setContentHeight] = useState(0);
-  const [iframeLoaded, setIframeLoaded] = useState(false);
-  const [showSpinner, setShowSpinner] = useState(true);
+  const [contentHeight, setContentHeight] = useState<number>(0);
+  const [isErrored, setIsErrored] = useState(false);
   const [activeIndex, setActiveIndex] = useState(1);
 
   const vizIframeRef = useRef<HTMLIFrameElement>(null);
   const containerRef = useRef<HTMLDivElement>(null);
   const codeRef = useRef<HTMLDivElement>(null);
+  const errorRef = useRef<HTMLDivElement>(null);
 
   const workspaceId = owner.sId;
 
   useVisualizationDataHandler({
     visualization,
     workspaceId,
-    onRetry,
     setContentHeight,
+    setIsErrored,
     vizIframeRef,
   });
 
   const { code, complete: codeFullyGenerated } = visualization;
 
+  const iframeLoaded = contentHeight > 0;
+  const showSpinner =
+    ((!codeFullyGenerated && !code) || (codeFullyGenerated && !iframeLoaded)) &&
+    !isErrored;
+
   useEffect(() => {
     if (!codeFullyGenerated) {
-      // Display spinner over the code block while waiting for code generation.
-      setShowSpinner(!code);
       setActiveIndex(0);
-    } else if (iframeLoaded) {
-      // Display iframe if code is generated and iframe has loaded.
-      setShowSpinner(false);
-      setActiveIndex(1);
     } else {
-      // Show spinner while iframe is loading.
-      setShowSpinner(true);
       setActiveIndex(1);
     }
-  }, [codeFullyGenerated, code, iframeLoaded]);
+  }, [codeFullyGenerated, code]);
 
   useEffect(() => {
     if (!containerRef.current) {
@@ -182,9 +178,13 @@ export function VisualizationActionIframe({
         ? `${codeRef.current?.scrollHeight}px`
         : "100%";
     } else if (activeIndex === 1) {
-      containerRef.current.style.height = `${contentHeight}px`;
+      if (isErrored && errorRef.current) {
+        containerRef.current.style.height = `${errorRef.current.scrollHeight}px`;
+      } else if (!isErrored) {
+        containerRef.current.style.height = `${contentHeight}px`;
+      }
     }
-  }, [activeIndex, contentHeight, codeFullyGenerated]);
+  }, [activeIndex, contentHeight, codeFullyGenerated, isErrored]);
 
   return (
     <div className="relative flex flex-col">
@@ -202,7 +202,9 @@ export function VisualizationActionIframe({
       <div
         className={classNames(
           "transition-height relative w-full overflow-hidden duration-500 ease-in-out",
-          codeFullyGenerated ? "min-h-96" : ""
+          codeFullyGenerated && !isErrored ? "min-h-96" : "",
+          isErrored ? "h-full" : "",
+          activeIndex === 1 ? "max-h-[60vh]" : ""
         )}
         ref={containerRef}
       >
@@ -219,21 +221,38 @@ export function VisualizationActionIframe({
             />
           </div>
           <div className="relative flex h-full w-full shrink-0 items-center justify-center">
-            {codeFullyGenerated && (
+            {codeFullyGenerated && !isErrored && (
               <div
-                style={{ height: `${contentHeight}px` }}
+                style={{
+                  height: !isErrored ? `${contentHeight}px` : "100%",
+                  minHeight: !isErrored ? "96" : undefined,
+                }}
                 className={classNames("max-h-[60vh] w-full")}
               >
                 <iframe
                   ref={vizIframeRef}
-                  // Set a min height so iframe can display error.
-                  className="h-full min-h-96 w-full"
+                  className={classNames(
+                    "h-full w-full",
+                    !isErrored ? "min-h-96" : ""
+                  )}
                   src={`${process.env.NEXT_PUBLIC_VIZ_URL}/content?identifier=${visualization.identifier}`}
                   sandbox="allow-scripts"
-                  onLoad={() => setIframeLoaded(true)}
                 />
               </div>
             )}
+            {isErrored && (
+              <div
+                className="flex h-full w-full flex-col items-center gap-4 py-8"
+                ref={errorRef}
+              >
+                <div className="text-sm text-element-800">
+                  An error occured while rendering the visualization.
+                </div>
+                <div>
+                  <Button label="Retry" onClick={onRetry} size="sm" />
+                </div>
+              </div>
+            )}
           </div>
         </div>
       </div>

diff --git a/front/lib/api/assistant/visualization.ts b/front/lib/api/assistant/visualization.ts
@@ -119,11 +119,15 @@ The generated component should not have any required props / parameters.
 
    
 
-### Outermost div height and width    
+### Responsiveness
 
-The component's outermost JSX tag should have a fixed height and width in pixels, set using the \`style\` prop, e.g. \`<div style={{ height: '600px', width: '600px' }}>...</div>\`.
+The content should be responsive and should not have fixed widths or heights. The component should be able to adapt to different screen sizes.
+The content should never overflow the viewport and should never have horizontal or vertical scrollbars.
 
-The height and width should be set to a fixed value, not a percentage. This style should not use tailwind CSS or any type of custom class. There should be a few pixels of horizontal padding to ensure the content is fully visible by the user.
+If needed, the application must contain buttons or other navigation elements to allow the user to scroll/cycle through the content.
+Never use tailwind's specific values like \`h-[600px]\`.
+
+Always add padding to the content (both horizontal and vertical) to make it look better and make sure the labels are fully visible.
 
     
 
@@ -173,7 +177,7 @@ if (file) {
 
 - Base React is available to be imported. In order to use hooks, they have to be imported at the top of the script, e.g. \`import { useState } from "react"\`
 
-- The recharts charting library is available to be imported, e.g. \`import { LineChart, XAxis, ... } from "recharts"\` & \`<LineChart ...><XAxis dataKey="name"> ...\`. Support for defaultProps will be removed from function components in a future major release. JavaScript default parameters should be used instead.
+- The recharts charting library is available to be imported, e.g. \`import { LineChart, XAxis, ... } from "recharts"\` & \`<LineChart ...><XAxis dataKey="name"> ...\`.
 
 - The papaparse library is available to be imported, e.g. \`import Papa from "papaparse"\` & \`const parsed = Papa.parse(fileContent, {header:true, skipEmptyLines: "greedy"});\`. The \`skipEmptyLines:"greedy"\` configuration should always be used.
 

diff --git a/types/src/front/assistant/visualization.ts b/types/src/front/assistant/visualization.ts
@@ -12,10 +12,6 @@ interface GetFileParams {
   fileId: string;
 }
 
-interface RetryParams {
-  errorMessage: string;
-}
-
 interface SetContentHeightParams {
   height: number;
 }
@@ -24,8 +20,8 @@ interface SetContentHeightParams {
 export type VisualizationRPCRequestMap = {
   getFile: GetFileParams;
   getCodeToExecute: null;
-  retry: RetryParams;
   setContentHeight: SetContentHeightParams;
+  setErrored: void;
 };
 
 // Derive the command type from the keys of the request map
@@ -42,17 +38,17 @@ export type VisualizationRPCRequest = {
 export const validCommands: VisualizationRPCCommand[] = [
   "getFile",
   "getCodeToExecute",
-  "retry",
   "setContentHeight",
+  "setErrored",
 ];
 
 // Command results.
 
 export interface CommandResultMap {
   getFile: { fileBlob: Blob | null };
   getCodeToExecute: { code: string };
-  retry: void;
   setContentHeight: void;
+  setErrored: void;
 }
 
 // TODO(@fontanierh): refactor all these guards to use io-ts instead of manual checks.
@@ -100,12 +96,12 @@ export function isGetCodeToExecuteRequest(
   );
 }
 
-// Type guard for retry.
-export function isRetryRequest(
+// Type guard for setContentHeight.
+export function isSetContentHeightRequest(
   value: unknown
 ): value is VisualizationRPCRequest & {
-  command: "retry";
-  params: RetryParams;
+  command: "setContentHeight";
+  params: SetContentHeightParams;
 } {
   if (typeof value !== "object" || value === null) {
     return false;
@@ -114,21 +110,19 @@ export function isRetryRequest(
   const v = value as Partial<VisualizationRPCRequest>;
 
   return (
-    v.command === "retry" &&
+    v.command === "setContentHeight" &&
     typeof v.identifier === "string" &&
     typeof v.messageUniqueId === "string" &&
     typeof v.params === "object" &&
     v.params !== null &&
-    typeof (v.params as RetryParams).errorMessage === "string"
+    typeof (v.params as SetContentHeightParams).height === "number"
   );
 }
 
-// Type guard for setContentHeight.
-export function isSetContentHeightRequest(
+export function isSetErroredRequest(
   value: unknown
 ): value is VisualizationRPCRequest & {
-  command: "setContentHeight";
-  params: SetContentHeightParams;
+  command: "setErrored";
 } {
   if (typeof value !== "object" || value === null) {
     return false;
@@ -137,12 +131,9 @@ export function isSetContentHeightRequest(
   const v = value as Partial<VisualizationRPCRequest>;
 
   return (
-    v.command === "setContentHeight" &&
+    v.command === "setErrored" &&
     typeof v.identifier === "string" &&
-    typeof v.messageUniqueId === "string" &&
-    typeof v.params === "object" &&
-    v.params !== null &&
-    typeof (v.params as SetContentHeightParams).height === "number"
+    typeof v.messageUniqueId === "string"
   );
 }
 
@@ -156,7 +147,7 @@ export function isVisualizationRPCRequest(
   return (
     isGetCodeToExecuteRequest(value) ||
     isGetFileRequest(value) ||
-    isRetryRequest(value) ||
-    isSetContentHeightRequest(value)
+    isSetContentHeightRequest(value) ||
+    isSetErroredRequest(value)
   );
 }
diff --git a/viz/app/components/Components.tsx b/viz/app/components/Components.tsx
@@ -1,54 +1,5 @@
 "use client";
 
-// We can't use Sparkle components in the viz app,
-// because of client-side rendering issue.
-// So we define the components here.
-
-export const Button = ({
-  label,
-  onClick,
-}: {
-  label: string;
-  onClick: () => void;
-}) => {
-  return (
-    <button
-      onClick={onClick}
-      className="px-4 py-2 text-sm font-medium text-white bg-blue-500 rounded hover:bg-blue-600"
-    >
-      {label}
-    </button>
-  );
-};
-
-export const ErrorMessage = ({
-  children,
-  title,
-}: {
-  children: React.ReactNode;
-  title: string;
-}) => {
-  return (
-    <div className="bg-pink-100 border-l-4 border-pink-500 rounded-lg p-4 max-w-md">
-      <div className="flex items-center mb-2">
-        <svg
-          className="w-6 h-6 text-pink-500 mr-2"
-          fill="currentColor"
-          viewBox="0 0 20 20"
-        >
-          <path
-            fillRule="evenodd"
-            d="M18 10a8 8 0 11-16 0 8 8 0 0116 0zm-7-4a1 1 0 11-2 0 1 1 0 012 0zM9 9a1 1 0 000 2v3a1 1 0 001 1h1a1 1 0 100-2v-3a1 1 0 00-1-1H9z"
-            clipRule="evenodd"
-          />
-        </svg>
-        <h3 className="text-lg font-semibold text-pink-800">{title}</h3>
-      </div>
-      <div className="text-pink-700">{children}</div>
-    </div>
-  );
-};
-
 export const Spinner = () => {
   return (
     <div className="flex items-center justify-center">