Skip to content

feat(client): storage explorer#1996

Closed
RawanAbdelkhalek wants to merge 12 commits intodevfrom
storage-file-explorer-clean
Closed

feat(client): storage explorer#1996
RawanAbdelkhalek wants to merge 12 commits intodevfrom
storage-file-explorer-clean

Conversation

@RawanAbdelkhalek
Copy link
Copy Markdown

@RawanAbdelkhalek RawanAbdelkhalek commented Oct 7, 2025

Description

ticket number SEMOSS/community#372
Closes SEMOSS/community#372

@RawanAbdelkhalek RawanAbdelkhalek requested a review from a team as a code owner October 7, 2025 17:01
@github-actions
Copy link
Copy Markdown

github-actions bot commented Oct 7, 2025

@CodiumAI-Agent /describe

@QodoAI-Agent
Copy link
Copy Markdown

Title

feat(client): storage explorer


User description

Description

ticket number SEMOSS/community#372


PR Type

Enhancement


Description

  • Add storage file explorer UI

  • Introduce storage item tree component

  • Enable download, upload, delete actions

  • Refactor file explorer props and imports


Diagram Walkthrough

flowchart LR
  EngineRoutes["Engine routes update"] -- "add STORAGE/VECTOR files pages" --> EngineFilePage["EngineFilePage"]
  EngineFilePage -- "renders" --> StorageFileExplorer["StorageFileExplorer"]
  StorageFileExplorer -- "lists, uploads, downloads, deletes" --> StorageExplorerItem["StorageExplorerItem"]
  FileExplorer["App/Insight FileExplorer"] -- "minor refactor" --> FileExplorerItem["FileExplorerItem"]
Loading

File Walkthrough

Relevant files
Enhancement
7 files
FileExplorer.tsx
Make space optional, drag callbacks optional; minor refactor
+100/-94
FileExplorerItem.tsx
Add action container; refactor children mapping                   
+142/-123
StorageExplorerItem.tsx
New storage tree item with select/download/delete               
+264/-0 
StorageFileExplorer.tsx
New storage file explorer with CRUD and upload                     
+689/-0 
index.ts
Export StorageFileExplorer and reorder exports                     
+5/-4     
EngineFilePage.tsx
Support engineType prop; render storage/vector views         
+22/-6   
engine.constants.ts
Add STORAGE/VECTOR files routes; helper HOC                           
+22/-12 
Formatting
1 files
index.ts
Normalize export quotes order                                                       
+5/-5     

@github-actions
Copy link
Copy Markdown

github-actions bot commented Oct 7, 2025

@CodiumAI-Agent /review

@github-actions
Copy link
Copy Markdown

github-actions bot commented Oct 7, 2025

@CodiumAI-Agent /improve

@QodoAI-Agent
Copy link
Copy Markdown

QodoAI-Agent commented Oct 7, 2025

PR Code Suggestions ✨

Latest suggestions up to 016fc9c

CategorySuggestion                                                                                                                                    Impact
Possible issue
Fix selected state shape mismatch

selectedMembers is typed as Record<string, true> but the select-all logic stores
full member objects, causing type and truthiness inconsistencies. Store a boolean
flag per ID to avoid unexpected behavior and potential runtime issues. Also type the
reducer accumulator to ensure strict keys.

packages/client/src/components/settings/PendingMembersTable.tsx [548-587]

-const [selectedMembers, setSelectedMembers] = useState<
-		Record<string, true>
-	>({});
+const [selectedMembers, setSelectedMembers] = useState<Record<string, true>>({});
 ...
 <Checkbox
-	checked={
-		Object.keys(
-			selectedMembers,
-		).length ===
-			renderedMembers.length &&
-		renderedMembers.length >
-			0
-	}
-	onChange={() => {
-		if (
-			Object.keys(
-				selectedMembers,
-			).length !==
-			renderedMembers.length
-		) {
-			const updatedMembers =
-				renderedMembers.reduce(
-					(
-						acc,
-						val,
-					) => {
-						acc[
-							val.ID
-						] =
-							val;
-
-						return acc;
-					},
-					{},
-				);
-
-			setSelectedMembers(
-				updatedMembers,
-			);
-		} else {
-			setSelectedMembers(
-				{},
-			);
-		}
-	}}
+  checked={
+    renderedMembers.length > 0 &&
+    Object.keys(selectedMembers).length === renderedMembers.length
+  }
+  onChange={() => {
+    if (Object.keys(selectedMembers).length !== renderedMembers.length) {
+      const updatedMembers = renderedMembers.reduce<Record<string, true>>((acc, val) => {
+        acc[val.ID] = true;
+        return acc;
+      }, {});
+      setSelectedMembers(updatedMembers);
+    } else {
+      setSelectedMembers({});
+    }
+  }}
 />
Suggestion importance[1-10]: 9

__

Why: Correctly identifies a real bug where selectedMembers (typed as Record<string, true>) is incorrectly populated with member objects in the select-all reducer; proposed code aligns types and logic precisely with the PR context.

High
Guard null toolbox and disable select

toolbox can be null, yet its properties are accessed without a guard, causing
runtime crashes when no toolbox is selected. Disable the tool selector until a
toolbox is chosen and guard against null before reading its fields.

packages/playground/src/components/plan/ToolCallDetails.tsx [104-128]

 <Select
 	value={details.tool_name}
 	label="Select Tool"
+	disabled={!toolbox || getMCP.status !== "SUCCESS"}
 	onChange={(e) => {
+		if (!toolbox) {
+			return;
+		}
 		onDetailsChange({
 			...details,
-			tool_name: e.target.value,
+			tool_name: e.target.value as string,
 			title: toolbox.name,
 			_meta: {
 				map: {
 					SMSS_PROJECT_ID: toolbox.id,
 					SMSS_PROJECT_NAME: toolbox.name,
 				},
 			},
 		});
 	}}
 	fullWidth
 >
Suggestion importance[1-10]: 8

__

Why: This catches a real null-dereference risk in the new component (toolbox accessed without check) and suggests disabling the select until data is ready. It directly matches the new code and prevents runtime crashes, high relevance and impact.

Medium
Fix async map misuse in upload

Using map(async ...) without awaiting means fileLocations may be incomplete or out
of order. Build fileLocations synchronously to ensure the subsequent query receives
the correct list. Also avoid redundant = fileLocations += assignments.

packages/client/src/components/engine/StorageFileExplorer.tsx [143-150]

-upload.map(async (file, index) => {
-	const fileLocation = file.fileLocation.replace(/\\/g, "/");
-	if (index + 1 === upload.length) {
-		fileLocations = fileLocations += `"${fileLocation}"`;
-	} else {
-		fileLocations = fileLocations += `"${fileLocation}", `;
-	}
-});
+const fileLocations = upload
+	.map((file) => `"${file.fileLocation.replace(/\\/g, "/")}"`)
+	.join(", ");
Suggestion importance[1-10]: 7

__

Why: Correctly points out misuse of map(async ...) and redundant concatenation; replacing with a synchronous map+join is accurate and improves reliability and readability of building fileLocations.

Medium
Restore ornaments consistency across messages

Making ornaments optional only on ResponseTextPixelMessage but removing it from
AbstractPixelMessage can break consumers expecting ornaments on all message types.
Either keep ornaments in the base type or make it optional consistently to prevent
runtime access errors.

packages/playground/src/types.d.ts [81-115]

+interface AbstractPixelMessage {
+	messageId: string;
+	parentMessageId?: string;
+	visible: boolean;
+	dateCreated: string;
+	ornaments?: Record<string, unknown>;
+}
+
 interface ResponseTextPixelMessage extends AbstractPixelMessage {
 	type: "RESPONSE_TEXT";
 	content: string;
-	ornaments: {
+	ornaments?: {
 		PLAYGROUND_MESSAGE_TYPE?: "COT";
+		[key: string]: unknown;
 	};
 }
Suggestion importance[1-10]: 7

__

Why: The PR removes ornaments from the base message while some consumers might rely on it; adding an optional ornaments back to AbstractPixelMessage is a coherent, low-risk fix that can prevent runtime undefined access. Good consistency improvement, moderate impact.

Medium
Align props with MUI and ensure stable id

value/defaultValue typed as unknown and forwarding arbitrary props to MuiTextField
can cause runtime type/prop mismatches and React controlled/uncontrolled warnings.
Restrict the props to align with MUI's TextFieldProps and ensure id stability to
avoid autofill issues previously handled. Wrap forwarded props with proper typing
and default an internal id when not provided.

libs/ui/src/components/TextField/TextField.tsx [1-225]

-export interface TextFieldProps {
-	/**
-	 * The value to associated with the input element (if controlled).
-	 */
-	value?: unknown;
+import { TextField as MuiTextField, type SxProps, type TextFieldProps as MuiTextFieldProps } from "@mui/material";
+import { useEffect, useId, useState } from "react";
 
-	/**
-	 * The value to associated with the input element (if uncontrolled).
-	 */
-	defaultValue?: unknown;
-...
+export type TextFieldProps = Omit<MuiTextFieldProps, "color"> & {
+	sx?: SxProps;
+};
+
 export const TextField: React.FC<TextFieldProps> = ({
 	type = "text",
 	autoComplete = "off",
+	id,
 	sx,
 	...otherProps
 }) => {
+	const reactId = useId();
+	const [componentId, setComponentId] = useState<string | undefined>(id);
+
+	useEffect(() => {
+		if (!id) {
+			setComponentId(componentId ?? `tf-${reactId}`);
+		}
+	}, [id, componentId, reactId]);
+
 	return (
 		<MuiTextField
+			id={id ?? componentId}
 			type={type}
 			autoComplete={autoComplete}
 			sx={sx}
 			{...otherProps}
 		/>
 	);
 };
Suggestion importance[1-10]: 6

__

Why: The suggestion correctly highlights potential controlled/uncontrolled issues and lost stable id behavior after refactor, proposing tighter typing and id defaulting. Impact is moderate and reasonable, though not strictly required for correctness.

Low
Safely encode tool parameters

Interpolating JSON.stringify(tool.parameters) directly into a template risks
malformed Pixel when values contain quotes or special characters. Encode the
parameters to avoid injection and parsing errors. Use an encode wrapper similar to
other queries.

packages/playground/src/stores/message/response-message.store.ts [299-308]

+const encodedParams = `<encode>${JSON.stringify(tool.parameters)}</encode>`;
 const response = await room.runRoomPixel<[string]>(
-	`RunMCPTool(project = [ "${tool._meta.map.SMSS_PROJECT_ID}" ], function=[ "${tool.name}" ], paramValues=[ ${JSON.stringify(tool.parameters)} ]);`,
+	`RunMCPTool(project = ["${tool._meta.map.SMSS_PROJECT_ID}"], function=["${tool.name}"], paramValues=[${encodedParams}]);`,
 );
 
 const { output } = response.pixelReturn[0];
-
-// save the response
 await this.saveToolExecution(tool, output, false);
Suggestion importance[1-10]: 5

__

Why: The concern about direct interpolation of JSON into the Pixel string is reasonable; proposing <encode> aligns with patterns elsewhere, though the necessity depends on Pixel parsing, so impact is moderate.

Low
General
Preserve and surface tool errors

Swallowing all errors hides critical failures and offers no user feedback. Capture
the error, still fail the step, and rethrow or log so callers can react and UI can
surface the problem.

packages/playground/src/stores/room/room.store.ts [607-638]

 processTool = async (
-	messageId: string,
-	toolId: string,
-	toolName: string,
-	toolResponse: string,
+  messageId: string,
+  toolId: string,
+  toolName: string,
+  toolResponse: string,
 ): Promise<void> => {
-	try {
-		const message = this.getMessage(messageId);
-		if (!message || message instanceof ResponseMessageStore !== true) {
-			return;
-		}
+  try {
+    const message = this.getMessage(messageId);
+    if (!message || !(message instanceof ResponseMessageStore)) {
+      throw new Error("Invalid message for tool processing");
+    }
 
-		const tool = message.getTool(toolId, toolName);
-		if (!tool) {
-			return;
-		}
+    const tool = message.getTool(toolId, toolName);
+    if (!tool) {
+      throw new Error("Tool not found on message");
+    }
 
-		// save the response with the tool
-		await message.saveToolExecution(
-			tool,
-			toolResponse,
-			this.mode === "executing",
-		);
+    await message.saveToolExecution(
+      tool,
+      toolResponse,
+      this.mode === "executing",
+    );
 
-		// verify if it is correct if executing
-		this.plan?.verifyToolStepExecution(
-			tool._meta.map.SMSS_PROJECT_ID,
-			tool.name,
-		);
-	} catch {
-		this.plan?.failStepExecution();
-	}
+    this.plan?.verifyToolStepExecution(
+      tool._meta.map.SMSS_PROJECT_ID,
+      tool.name,
+    );
+  } catch (e) {
+    this.plan?.failStepExecution();
+    throw e;
+  }
 };
Suggestion importance[1-10]: 7

__

Why: Avoids silent failures by rethrowing after failing the step, enhancing debuggability; change is reasonable and fits surrounding execution/plan flow without contradicting PR logic.

Medium
Safely handle unknown errors

The thrown error re-wrap uses e.message but e is unknown, which can cause a crash.
Narrow the error to unknown and safely extract a message. Also, use await
this.runRoomPixel(..., false) if you don't want to toggle loading here.

packages/playground/src/stores/room/room.store.ts [355-374]

 linkAgent = async (agent: Agent) => {
-	try {
-		const { errors } = await this.runRoomPixel<
-			[
-				{
-					roomId: string;
-				},
-			]
-		>(
-			`SetRoomWorkspace(roomId=${JSON.stringify(this._store.roomId)}, workspaceId=${JSON.stringify(agent.workspace_id)});`,
-		);
+  try {
+    const { errors } = await this.runRoomPixel<
+      [{ roomId: string }]
+    >(
+      `SetRoomWorkspace(roomId=${JSON.stringify(this._store.roomId)}, workspaceId=${JSON.stringify(agent.workspace_id)});`,
+      false
+    );
 
-		if (errors?.length > 0) {
-			throw new Error(errors?.join(", ") || undefined);
-		}
-		this.setAgentId(agent.workspace_id);
-	} catch (e) {
-		throw new Error(e.message || "Error linking agent");
-	}
+    if (errors && errors.length > 0) {
+      throw new Error(errors.join(", "));
+    }
+    this.setAgentId(agent.workspace_id);
+  } catch (e: unknown) {
+    const message =
+      typeof e === "object" && e !== null && "message" in e
+        ? String((e as { message?: unknown }).message)
+        : "Error linking agent";
+    throw new Error(message);
+  }
 };
Suggestion importance[1-10]: 6

__

Why: Improves robustness by narrowing catch to unknown and safely extracting the message; moderate impact and consistent with new runRoomPixel API allowing a loading flag.

Low
Remove duplicate error checks

The error handling block is duplicated and will never reach the second check. Remove
the redundant section to prevent unreachable code and keep logic clear. Keep a
single, early return on errors.

packages/client/src/components/engine/StorageFileExplorer.tsx [257-272]

 if (response.errors && response.errors.length > 0) {
 	notification.add({
 		color: "error",
 		message: `Failed to delete files: ${response.errors[0]}`,
 	});
 	return;
 }
 
-// Check if the response contains errors
-if (response.errors && response.errors.length > 0) {
-	notification.add({
-		color: "error",
-		message: `Failed to delete files: ${response.errors[0]}`,
-	});
-	return;
-}
-
Suggestion importance[1-10]: 6

__

Why: The duplicated response.errors check is real and redundant; removing it clarifies flow without changing behavior, a minor but correct cleanup.

Low

Previous suggestions

Suggestions up to commit a38f1a5
CategorySuggestion                                                                                                                                    Impact
Possible issue
Harden delete error handling

Guard against missing response.errors and remove duplicate checks to avoid runtime
errors. Also fix the typo in the error message to ensure clear user feedback. This
prevents crashes when errors is undefined and streamlines the success path.

packages/client/src/components/engine/StorageFileExplorer.tsx [206-298]

 const handleDelete = async (filePath?: string) => {
     const pathsToDelete = filePath ? [filePath] : selected;
-
     if (pathsToDelete.length === 0) return;
 
+    // Single delete
     if (pathsToDelete.length === 1) {
         const deleteQuery = `Storage(storage = "${id}") |
         DeleteFromStorage(storagePath="${pathsToDelete[0]}", leaveFolderStructure=false);`;
 
         try {
             const response = await monolithStore.runQuery(deleteQuery);
+            const errors = response?.errors ?? [];
 
-            // Check if the response contains an error message
-            if (response.errors.length > 0) {
+            if (errors.length > 0) {
                 notification.add({
                     color: "error",
-                    message: `Failed to delete file response: ${response.errors[0]}`,
-                });
-            } else {
-                // Successful deletion
-                notification.add({
-                    color: "success",
-                    message: `Successfully deleted document`,
-                });
-
-                setExpandedPaths((prev) =>
-                    prev.filter((p) => !p.startsWith(pathsToDelete[0])),
-                );
-
-                if (selectedFile === pathsToDelete[0]) {
-                    setSelectedFile("");
-                }
-                setSelected([]);
-                refreshFiles();
-            }
-        } catch (e) {
-            notification.add({
-                color: "error",
-                message: `Failed to deleteeeeee file(s): ${e}`,
-            });
-        }
-    } else {
-        const pathsString = pathsToDelete
-            .map((path) => `"${path}"`)
-            .join(", ");
-        const deleteQuery = `Storage(storage = "${id}") |
-        DeleteFromStorage(storagePaths=[${pathsString}], leaveFolderStructure=false);`;
-
-        try {
-            const response = await monolithStore.runQuery(deleteQuery);
-
-            if (response.errors && response.errors.length > 0) {
-                notification.add({
-                    color: "error",
-                    message: `Failed to delete files: ${response.errors[0]}`,
+                    message: `Failed to delete file: ${errors[0]}`,
                 });
                 return;
             }
 
-            // Check if the response contains errors
-            if (response.errors && response.errors.length > 0) {
-                notification.add({
-                    color: "error",
-                    message: `Failed to delete files: ${response.errors[0]}`,
-                });
-                return;
-            }
-
-            pathsToDelete.forEach((path) => {
-                setExpandedPaths((prev) =>
-                    prev.filter((p) => !p.startsWith(path)),
-                );
-                if (selectedFile === path) {
-                    setSelectedFile("");
-                }
-            });
-            setSelected([]);
-            setShowDeleteDialog(false);
-            refreshFiles();
-
-            // Show success message only if we got here without errors
             notification.add({
                 color: "success",
-                message: `Successfully deleted ${pathsToDelete.length} file(s)`,
+                message: `Successfully deleted document`,
             });
+
+            setExpandedPaths((prev) => prev.filter((p) => !p.startsWith(pathsToDelete[0])));
+            if (selectedFile === pathsToDelete[0]) {
+                setSelectedFile("");
+            }
+            setSelected([]);
+            refreshFiles();
         } catch (e) {
             notification.add({
                 color: "error",
-                message: `Failed to delete file(s): ${e.message || e}`,
+                message: `Failed to delete file(s): ${e instanceof Error ? e.message : String(e)}`,
             });
         }
+        return;
+    }
+
+    // Bulk delete
+    const pathsString = pathsToDelete.map((path) => `"${path}"`).join(", ");
+    const deleteQuery = `Storage(storage = "${id}") |
+        DeleteFromStorage(storagePaths=[${pathsString}], leaveFolderStructure=false);`;
+
+    try {
+        const response = await monolithStore.runQuery(deleteQuery);
+        const errors = response?.errors ?? [];
+
+        if (errors.length > 0) {
+            notification.add({
+                color: "error",
+                message: `Failed to delete files: ${errors[0]}`,
+            });
+            return;
+        }
+
+        pathsToDelete.forEach((path) => {
+            setExpandedPaths((prev) => prev.filter((p) => !p.startsWith(path)));
+            if (selectedFile === path) {
+                setSelectedFile("");
+            }
+        });
+        setSelected([]);
+        setShowDeleteDialog(false);
+        refreshFiles();
+
+        notification.add({
+            color: "success",
+            message: `Successfully deleted ${pathsToDelete.length} file(s)`,
+        });
+    } catch (e) {
+        notification.add({
+            color: "error",
+            message: `Failed to delete file(s): ${e instanceof Error ? e.message : String(e)}`,
+        });
     }
 };
Suggestion importance[1-10]: 8

__

Why: The suggestion correctly removes duplicate error checks, guards against possibly undefined response.errors, and fixes a typo, reducing runtime error risk and improving user feedback without changing behavior.

Medium
Guard missing required prop

When type is 'app', ensure space is defined to prevent constructing a Pixel query
with undefined, which can lead to backend errors. Early-return a loading or error
state if space is missing before calling usePixel.

packages/client/src/components/common/File/FileExplorer.tsx [20-35]

 interface FileExplorerProps {
-	expandedPaths: string[];
-	onToggleExpand: (path: string) => void;
-	/** Type of file opened */
-	type: "app" | "insight";
-
-    /** Space where the file is located */
+    expandedPaths: string[];
+    onToggleExpand: (path: string) => void;
+    type: "app" | "insight";
     space?: string;
-
-	/** insight id */
-	insightId?: string | null;
-
-	/** Trigger a callback when an file is selected */
-	onSelect?: (path: string) => void;
-
-    /** Triggered when the Label starts dragging */
+    insightId?: string | null;
+    onSelect?: (path: string) => void;
     onDragStart?: (event: React.DragEvent<HTMLDivElement>, path: string) => void;
-
-	/** Triggered when the item ends dragging */
-	onDragEnd?: (event: React.DragEvent<HTMLDivElement>, path: string) => void;
+    onDragEnd?: (event: React.DragEvent<HTMLDivElement>, path: string) => void;
 }
 ...
+// Guard against missing space for app type
+const shouldQueryAssets = type === 'app' && Boolean(space);
 const getAssets = usePixel<
     {
         lastModified: string;
         name: string;
         path: string;
         type: 'directory' | 'file';
     }[]
 >(
-    type === 'app'
-        ? `BrowseAsset(filePath=["version/assets"], space=["${space}"]);`
-        : '',
+    shouldQueryAssets ? `BrowseAsset(filePath=["version/assets"], space=["${space}"]);` : '',
     {},
     insightId,
 );
+if (type === 'app' && !space) {
+    return <LoadingScreen.Trigger description="Missing space for application assets." />;
+}
Suggestion importance[1-10]: 6

__

Why: Ensuring space exists when type is 'app' avoids constructing a query with undefined; the proposed guard aligns with the new optional space prop and improves resilience, though it's a minor control-flow addition.

Low
Security
Escape query-interpolated strings

Quote-interpolate user-derived values in Pixel queries to avoid malformed queries or
injection via path/filename. Escape double quotes and backslashes in pathsToDownload
and filename before embedding them in template literals.

packages/client/src/components/engine/StorageFileExplorer.tsx [300-452]

+const escapePixelString = (s: string) => s.replace(/\\/g, "\\\\").replace(/"/g, '\\"');
+
 const handleDownload = async (path?: string) => {
     const pathsToDownload = path ? [path] : selected;
-
     if (pathsToDownload.length === 0) return;
 
     if (pathsToDownload.length === 1) {
         try {
-            const filename = extractFilename(pathsToDownload[0]);
+            const rawPath = pathsToDownload[0];
+            const filename = extractFilename(rawPath);
 
-            if (pathsToDownload[0].endsWith("/")) {
-                notification.add({
-                    color: "error",
-                    message:
-                        "Cannot download a directory. Please select a file.",
-                });
+            if (rawPath.endsWith("/")) {
+                notification.add({ color: "error", message: "Cannot download a directory. Please select a file." });
                 return;
             }
 
-            const downloadQuery = `Storage("${id}") | PullFromStorage(storagePath="${pathsToDownload[0]}", filePath="${filename}") | DownloadAsset(filePath=["${filename}"], space=["insight"]);`;
+            const safePath = escapePixelString(rawPath);
+            const safeFilename = escapePixelString(filename);
+            const downloadQuery = `Storage("${id}") | PullFromStorage(storagePath="${safePath}", filePath="${safeFilename}") | DownloadAsset(filePath=["${safeFilename}"], space=["insight"]);`;
 
             const response = await monolithStore.runQuery(downloadQuery);
-
-            const fileKey = response.pixelReturn[0]?.output;
-
+            const fileKey = response?.pixelReturn?.[0]?.output;
             if (!fileKey) {
-                throw new Error(
-                    "Failed to get file key for download. The file may not exist or there was a server error.",
-                );
+                throw new Error("Failed to get file key for download. The file may not exist or there was a server error.");
             }
 
-            await monolithStore.download(
-                configStore.store.insightID,
-                fileKey,
-            );
-
-            notification.add({
-                color: "success",
-                message: `Successfully downloaded: ${filename}`,
-            });
+            await monolithStore.download(configStore.store.insightID, fileKey);
+            notification.add({ color: "success", message: `Successfully downloaded: ${filename}` });
         } catch (e) {
             let errorMessage = "Download failed: ";
-            if (e instanceof Error) {
-                if (e.message.includes("directory")) {
-                    errorMessage =
-                        "Cannot download directories. Please select a file.";
-                } else if (e.message.includes("file key")) {
-                    errorMessage =
-                        "File not found or server error occurred.";
-                } else if (
-                    e.message.includes("network") ||
-                    e.message.includes("fetch")
-                ) {
-                    errorMessage =
-                        "Network error. Please check your connection and try again.";
-                } else {
-                    errorMessage += e.message;
-                }
-            } else {
-                errorMessage += "An unexpected error occurred.";
-            }
-
-            notification.add({
-                color: "error",
-                message: errorMessage,
-            });
+            errorMessage += e instanceof Error ? e.message : "An unexpected error occurred.";
+            notification.add({ color: "error", message: errorMessage });
         }
     } else {
         try {
             const downloadedFiles: string[] = [];
 
-            for (const path of pathsToDownload) {
-                if (path.endsWith("/")) {
-                    throw new Error(
-                        "Cannot download directories. Please select files only.",
-                    );
+            for (const p of pathsToDownload) {
+                if (p.endsWith("/")) {
+                    throw new Error("Cannot download directories. Please select files only.");
+                }
+                const filename = extractFilename(p);
+                const safePath = escapePixelString(p);
+                const safeFilename = escapePixelString(filename);
+                const downloadQuery = `Storage("${id}") | PullFromStorage(storagePath="${safePath}", filePath="${safeFilename}");`;
+                await monolithStore.runQuery(downloadQuery);
+                downloadedFiles.push(safeFilename);
+            }
+
+            if (downloadedFiles.length > 0) {
+                const storageMetaData = await monolithStore.runQuery(`GetEngineMetadata(engine=["${id}"]);`);
+                const storageName = storageMetaData.pixelReturn[0].output.database_name;
+                const userName = configStore.store.user.name;
+                const now = new Date();
+                const formattedDate = now.toISOString().replace(/[:]/g, "-").replace(/\..+/, "").replace("T", "_");
+
+                const filePathsString = downloadedFiles.map((file) => `"${file}"`).join(", ");
+                const zipName = `${storageName}_${userName}_${formattedDate}.zip`;
+                const safeZipName = escapePixelString(zipName);
+                const zipQuery = `ZipFiles(filePaths=[${filePathsString}], filePath="${safeZipName}") | DownloadAsset(filePath=["${safeZipName}"], space=["insight"]);`;
+
+                const response = await monolithStore.runQuery(zipQuery);
+                const fileKey = response?.pixelReturn?.[0]?.output;
+                if (!fileKey) {
+                    throw new Error("Failed to get file key for download. The files may not exist or there was a server error.");
                 }
 
-                const filename = extractFilename(path);
-                const downloadQuery = `Storage("${id}") | PullFromStorage(storagePath="${path}", filePath="${filename}");`;
-
-                await monolithStore.runQuery(downloadQuery);
-                downloadedFiles.push(filename);
-            }
-            if (downloadedFiles.length > 0) {
-                const storageMetaData = await monolithStore.runQuery(
-                    `GetEngineMetadata(engine=["${id}"]);`,
-                );
-                const storageName =
-                    storageMetaData.pixelReturn[0].output.database_name;
-                const userName = configStore.store.user.name;
-                const now = new Date();
-                const formattedDate = now
-                    .toISOString()
-                    .replace(/[:]/g, "-")
-                    .replace(/\..+/, "")
-                    .replace("T", "_");
-
-                const filePathsString = downloadedFiles
-                    .map((file) => `"${file}"`)
-                    .join(", ");
-                const zipQuery = `ZipFiles(filePaths=[${filePathsString}], filePath="${storageName}_${userName}_${formattedDate}.zip") 
-                                | DownloadAsset(filePath=["${storageName}_${userName}_${formattedDate}.zip"], space=["insight"]);`;
-
-                const response = await monolithStore.runQuery(zipQuery);
-
-                const fileKey = response.pixelReturn[0]?.output;
-
-                if (!fileKey) {
-                    throw new Error(
-                        "Failed to get file key for download. The files may not exist or there was a server error.",
-                    );
-                }
-
-                await monolithStore.download(
-                    configStore.store.insightID,
-                    fileKey,
-                );
-
+                await monolithStore.download(configStore.store.insightID, fileKey);
                 setSelected([]);
             }
         } catch (e) {
             let errorMessage = "ZIP download failed: ";
-            if (e instanceof Error) {
-                if (e.message.includes("directory")) {
-                    errorMessage +=
-                        "Cannot download directories. Please select files only.";
-                } else if (e.message.includes("file key")) {
-                    errorMessage +=
-                        "Files not found or server error occurred.";
-                } else if (
-                    e.message.includes("network") ||
-                    e.message.includes("fetch")
-                ) {
-                    errorMessage +=
-                        "Network error. Please check your connection and try again.";
-                } else {
-                    errorMessage += e.message;
-                }
-            } else {
-                errorMessage += "An unexpected error occurred.";
-            }
-            notification.add({
-                color: "error",
-                message: errorMessage,
-            });
+            errorMessage += e instanceof Error ? e.message : "An unexpected error occurred.";
+            notification.add({ color: "error", message: errorMessage });
         }
     }
 };
Suggestion importance[1-10]: 7

__

Why: Escaping paths and filenames before embedding in Pixel queries is a sound security and robustness improvement; while not fixing an observed bug, it prevents malformed queries and potential injection issues.

Medium

tevanburen and others added 3 commits October 7, 2025 13:58
* feat: begin the discover UI
* feat(playground): adding basic CoT
* feat(playground): removing knowledge. should be done as a tool
* feat(playground): allowing user to copy and paste an image
* feat(playground): adding ability to autoscroll
* feat: todos
* feat(playground): fixing ability to perform chain-of-thought and plan out steps
* chore(ui): fixing theme
* chore(ui): cleaning up file-upload
* chore(ui): cleaning up typography
* chore(ui): cleaning up LoadingScreen
* chore(ui): cleaning up List
* chore(client): isolating material to semoss/ui
* chore(ui): removing appbar
* chore(ui): removing toolbar
Copy link
Copy Markdown
Contributor

@neelneelneel neelneelneel left a comment

Choose a reason for hiding this comment

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

Can we make a few modifications:

  • Fix ability to download
  • Allow the ability to click and open a file

@RawanAbdelkhalek
Copy link
Copy Markdown
Author

@neelneelneel
the download fix need the backend branch to be merged.

@neelneelneel
Copy link
Copy Markdown
Contributor

Closed by #2517

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Storage Engine Base App: Ability to manage files via a file browser within storage catalog

4 participants