Feat/editor improvements #2601
Merged
Feat/editor improvements #2601
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@Rish-it is attempting to deploy a commit to the Onlook Team on Vercel. A member of the Team first needs to authorize it. |
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
|
Hey @drfarrell when you get a chance, could you review and share your feedback? Current Stage:
|
|
I don't think this is a regression but there's an issue where you can upload a file under another file. In this case perhaps we should add it to the same folder. Right now it just fails. Can chat about drag and drop tmr but react-arbonist has dragging support which we use in the layers panel 
|
echo "editor improvements + Upload files" > .git/COMMIT_EDITMSG# modified: apps/web/client/src/app/project/[id]/_components/right-panel/dev-tab/file-tree.tsx
Rish-it
force-pushed
the
feat/editor-improvements
branch
from
ee18628 to
da2ef83
Compare
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds an UploadModal component and integrates it into the Dev tab via a new “Create or Upload File” dropdown. Adjusts file tree alignment to top. Refactors save/close handlers with useCallback and adds Cmd/Ctrl+S shortcut. Minor tooltip updates and a voided save call. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant DevTab
participant UploadModal
participant Sandbox
participant Toast
User->>DevTab: Click FilePlus dropdown
DevTab-->>User: Show options
User->>DevTab: Select "Upload file"
DevTab->>UploadModal: Open
UploadModal-->>User: Choose files & directory
User->>UploadModal: Click Upload
loop each selected file
UploadModal->>Sandbox: writeFile(finalPath, bytes)
end
UploadModal->>Sandbox: listAllFiles()
Sandbox-->>UploadModal: Updated file list
UploadModal->>Toast: Show success
UploadModal->>DevTab: Close modal
sequenceDiagram
participant User
participant DevTab
participant IDE
participant EditorView
participant Toast
User->>DevTab: Press Cmd/Ctrl+S
DevTab->>IDE: saveActiveFile()
IDE-->>DevTab: Save result
alt No unsaved changes remain and pending close
DevTab->>EditorView: destroy()
DevTab->>IDE: closeFile(fileId)
end
DevTab->>Toast: Show "Saved" message
Estimated code review effort🎯 4 (Complex) | ⏱️ ~35 minutes Assessment against linked issues
Poem
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
Comment on lines
+66
to
+68
| <TooltipContent side="bottom" hideArrow> | ||
| <p>Create or Upload File</p> | ||
| <TooltipArrow className="fill-foreground" /> |
There was a problem hiding this comment.
There appears to be a contradiction in the tooltip configuration. The hideArrow={true} prop is set on line 66, but a <TooltipArrow> component is also included in the content. This creates conflicting behavior where the arrow is both hidden and styled.
To resolve this, either:
- Remove the
hideArrowprop to allow the arrow to display, or - Remove the
<TooltipArrow>component if the arrow should be hidden
This same pattern appears in other tooltips in this file as well.
Suggested change
| <TooltipContent side="bottom" hideArrow> | |
| <p>Create or Upload File</p> | |
| <TooltipArrow className="fill-foreground" /> | |
| <TooltipContent side="bottom"> | |
| <p>Create or Upload File</p> | |
| <TooltipArrow className="fill-foreground" /> |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
There was a problem hiding this comment.
@Rish-it , could you check this comment if it's correct?
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (2)
apps/web/client/src/app/project/[id]/_components/right-panel/dev-tab/index.tsx (2)
339-377: Consider extracting complex save logic into separate functionsThe
saveFilecallback has become quite complex with multiple conditional branches. Consider breaking it down for better maintainability.+const handlePendingCloseAll = async () => { + const file = ide.openedFiles.find((f) => f.id === ide.activeFile?.id); + if (file) { + await ide.saveActiveFile(); + closeFile(file.id); + } + + const remainingDirty = ide.openedFiles.filter((f) => f.isDirty); + if (remainingDirty.length !== 0) { + setShowUnsavedDialog(true); + return false; + } + + ide.closeAllFiles(); + setPendingCloseAll(false); + setShowUnsavedDialog(false); + return true; +}; + +const handleRegularSave = async () => { + await ide.saveActiveFile(); + + if (showUnsavedDialog) { + setShowUnsavedDialog(false); + closeFile(ide.activeFile!.id); + } + + toast('File saved!'); +}; const saveFile = useCallback(async () => { if (!ide.activeFile) { return; } if (!isSandboxReady()) { handleSandboxNotReady('save file'); return; } if (pendingCloseAll) { - const file = ide.openedFiles.find((f) => f.id === ide.activeFile?.id); - if (file) { - await ide.saveActiveFile(); - closeFile(file.id); - } - - const remainingDirty = ide.openedFiles.filter((f) => f.isDirty); - if (remainingDirty.length !== 0) { - setShowUnsavedDialog(true); - return; - } - - ide.closeAllFiles(); - setPendingCloseAll(false); - setShowUnsavedDialog(false); - + await handlePendingCloseAll(); return; } - await ide.saveActiveFile(); - - if (showUnsavedDialog) { - setShowUnsavedDialog(false); - closeFile(ide.activeFile.id); - } - - toast('File saved!'); + await handleRegularSave(); }, [ide, isSandboxReady, handleSandboxNotReady, pendingCloseAll, showUnsavedDialog, setShowUnsavedDialog, setPendingCloseAll, closeFile]);
325-337: Consider adding error boundaries for file operationsThe closeFile function should handle potential errors when destroying editor views.
const closeFile = useCallback((fileId: string) => { if (ide.openedFiles.find(f => f.id === fileId)?.isDirty) { setShowUnsavedDialog(true); return; } const editorView = editorViewsRef.current.get(fileId); if (editorView) { - editorView.destroy(); - editorViewsRef.current.delete(fileId); + try { + editorView.destroy(); + editorViewsRef.current.delete(fileId); + } catch (error) { + console.error(`Failed to destroy editor view for file ${fileId}:`, error); + // Still delete the reference to prevent memory leaks + editorViewsRef.current.delete(fileId); + } } ide.closeFile(fileId); }, [ide, setShowUnsavedDialog]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/web/client/src/app/project/[id]/_components/right-panel/dev-tab/code-controls.tsx(4 hunks)apps/web/client/src/app/project/[id]/_components/right-panel/dev-tab/file-tree.tsx(1 hunks)apps/web/client/src/app/project/[id]/_components/right-panel/dev-tab/index.tsx(4 hunks)apps/web/client/src/app/project/[id]/_components/right-panel/dev-tab/upload-modal.tsx(1 hunks)
🔇 Additional comments (13)
apps/web/client/src/app/project/[id]/_components/right-panel/dev-tab/file-tree.tsx (3)
339-342: UI alignment improvements look goodThe changes from
justify-centertojustify-startwithpt-4padding successfully address the file list alignment requirement from issue #2576.
344-346: Consistent alignment for empty stateThe empty state alignment changes are consistent with the loading state, maintaining a top-aligned layout as requested.
339-342: LGTM! UI alignment improvementsThe changes to align loading and empty states to top-start improve the visual consistency as requested in issue #2576.
apps/web/client/src/app/project/[id]/_components/right-panel/dev-tab/code-controls.tsx (4)
27-27: Good practice using void for fire-and-forget async callWrapping the async call with
voidexplicitly indicates that the promise is intentionally not awaited and prevents unhandled promise warnings.
37-65: Well-implemented dropdown menu for file operationsThe dropdown menu implementation cleanly separates "Create new file" and "Upload file" actions, providing a good UX improvement as requested in issue #2576.
66-69: Nice touch adding TooltipArrow for consistencyAdding
TooltipArrowto both the file and folder tooltips improves visual consistency across the UI.Also applies to: 82-85
37-69: LGTM! Clean dropdown implementationThe dropdown menu implementation with separate "Create new file" and "Upload file" options is clean and follows the PR requirements well.
apps/web/client/src/app/project/[id]/_components/right-panel/dev-tab/index.tsx (5)
325-337: Good use of useCallback for closeFileThe memoization of
closeFilewith appropriate dependencies prevents unnecessary re-renders and ensures stable function references.
339-377: Well-structured saveFile callback with proper dependenciesThe
saveFilecallback is properly memoized with all necessary dependencies, maintaining the complex save logic while ensuring referential stability.
445-461: Excellent implementation of Cmd/Ctrl+S shortcutThe keyboard shortcut implementation correctly:
- Uses a ref to always call the latest
saveFilefunction- Prevents default browser save dialog
- Properly cleans up the event listener
- Handles both Cmd (Mac) and Ctrl (Windows/Linux)
This successfully addresses the requirement from issue #2576.
43-46: Good memoization of handleSandboxNotReadyThe memoization with an empty dependency array is appropriate since the function doesn't use any external values.
445-461: LGTM! Keyboard shortcut implementationThe Cmd/Ctrl+S keyboard shortcut is properly implemented with:
- Correct ref pattern to maintain the latest callback reference
- Proper event prevention
- Clean event listener cleanup
apps/web/client/src/app/project/[id]/_components/right-panel/dev-tab/upload-modal.tsx (1)
54-81: No action needed: parent directory extraction already implementedThe existing logic in
upload-modal.tsx(lines 73–76) already derives the parent directory viapath.substring(0, lastSlash)and only returns it if it’s inavailableDirectories, falling back to"root"otherwise. This prevents uploads from targeting a file itself, so no changes are necessary.
Comment on lines
+27
to
28
| void editorEngine.ide.saveActiveFile(); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use await instead of void for proper error handling
Using void to ignore the promise can mask errors. Consider properly awaiting the save operation or at least handling errors.
-const saveFile = () => {
- void editorEngine.ide.saveActiveFile();
+const saveFile = async () => {
+ try {
+ await editorEngine.ide.saveActiveFile();
+ } catch (error) {
+ console.error('Failed to save file:', error);
+ toast('Failed to save file', { description: 'Please try again' });
+ }
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| void editorEngine.ide.saveActiveFile(); | |
| }; | |
| const saveFile = async () => { | |
| try { | |
| await editorEngine.ide.saveActiveFile(); | |
| } catch (error) { | |
| console.error('Failed to save file:', error); | |
| toast('Failed to save file', { description: 'Please try again' }); | |
| } | |
| }; |
🤖 Prompt for AI Agents
In
apps/web/client/src/app/project/[id]/_components/right-panel/dev-tab/code-controls.tsx
around lines 27 to 28, the call uses "void editorEngine.ide.saveActiveFile();"
which swallows promise rejections; change the caller to await the promise (mark
the enclosing function as async) or wrap the call in a try/catch and
handle/report errors (log and/or show a user notification) so save failures are
not silently ignored.
Comment on lines
+1
to
+251
| <DialogContent> | ||
| <DialogHeader> | ||
| <DialogTitle>Upload Files</DialogTitle> | ||
| <DialogDescription> | ||
| Upload files to{' '} | ||
| <code className="bg-background-secondary px-1 py-0.5 rounded text-xs"> | ||
| {displayPath} | ||
| </code> | ||
| </DialogDescription> | ||
| </DialogHeader> | ||
|
|
||
| <div className="grid gap-4 py-4"> | ||
| {/* File Selection */} | ||
| <div className="space-y-2"> | ||
| <label className="text-sm font-medium">Select Files</label> | ||
| <div className="border-2 border-dashed border-border-primary rounded-lg p-4 text-center"> | ||
| <input | ||
| ref={fileInputRef} | ||
| type="file" | ||
| multiple | ||
| className="hidden" | ||
| onChange={handleFileSelect} | ||
| /> | ||
| <Button | ||
| variant="ghost" | ||
| onClick={() => fileInputRef.current?.click()} | ||
| className="w-full" | ||
| > | ||
| <Icons.Upload className="h-4 w-4 mr-2" /> | ||
| Choose Files | ||
| </Button> | ||
| </div> | ||
| </div> | ||
|
|
||
| {/* Selected Files List */} | ||
| {selectedFiles.length > 0 && ( | ||
| <div className="space-y-2"> | ||
| <label className="text-sm font-medium">Selected Files</label> | ||
| <div className="max-h-32 overflow-y-auto space-y-1"> | ||
| {selectedFiles.map((file, index) => ( | ||
| <div key={index} className="flex items-center justify-between bg-background-secondary p-2 rounded text-sm"> | ||
| <span className="truncate">{file.name}</span> | ||
| <Button | ||
| variant="ghost" | ||
| size="icon" | ||
| className="h-6 w-6" | ||
| onClick={() => removeFile(index)} | ||
| > | ||
| <Icons.CrossS className="h-3 w-3" /> | ||
| </Button> | ||
| </div> | ||
| ))} | ||
| </div> | ||
| </div> | ||
| )} | ||
|
|
||
| {/* Directory Selection */} | ||
| {selectedFiles.length > 0 && ( | ||
| <div className="space-y-2"> | ||
| <label className="text-sm font-medium">Target Directory</label> | ||
| <Select value={targetDirectory} onValueChange={setTargetDirectory}> | ||
| <SelectTrigger> | ||
| <SelectValue placeholder="Select directory" /> | ||
| </SelectTrigger> | ||
| <SelectContent> | ||
| <SelectItem value="root">/ (root)</SelectItem> | ||
| {availableDirectories.map(dir => ( | ||
| <SelectItem key={dir} value={dir}> | ||
| /{dir}/ | ||
| </SelectItem> | ||
| ))} | ||
| </SelectContent> | ||
| </Select> | ||
| </div> | ||
| )} | ||
| </div> | ||
|
|
||
| <DialogFooter> | ||
| <Button | ||
| variant="ghost" | ||
| onClick={() => onOpenChange(false)} | ||
| disabled={isUploading} | ||
| > | ||
| Cancel | ||
| </Button> | ||
| <Button | ||
| variant="outline" | ||
| onClick={handleUpload} | ||
| disabled={selectedFiles.length === 0 || isUploading} | ||
| > | ||
| {isUploading | ||
| ? 'Uploading...' | ||
| : `Upload ${selectedFiles.length} file${selectedFiles.length !== 1 ? 's' : ''}` | ||
| } | ||
| </Button> | ||
| </DialogFooter> | ||
| </DialogContent> | ||
| </Dialog> | ||
| ); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding file size validation
The component doesn't validate file sizes before uploading. Large files could cause performance issues or exceed sandbox limitations.
Add a constant for max file size at the top of the component:
const MAX_FILE_SIZE = 10 * 1024 * 1024; // 10MBThen validate in handleFileSelect:
const handleFileSelect = (event: React.ChangeEvent<HTMLInputElement>) => {
const files = event.target.files;
if (!files || files.length === 0) {
return;
}
const fileArray = Array.from(files);
+
+ // Validate file sizes
+ const oversizedFiles = fileArray.filter(file => file.size > MAX_FILE_SIZE);
+ if (oversizedFiles.length > 0) {
+ const names = oversizedFiles.map(f => f.name).join(', ');
+ toast('Files too large', {
+ description: `The following files exceed 10MB: ${names}`
+ });
+ return;
+ }
+
setSelectedFiles(fileArray);
// Set smart default directory based on first file
if (fileArray.length > 0) {
const smartDir = getSmartDirectory(fileArray[0].name);
setTargetDirectory(smartDir);
}
};🤖 Prompt for AI Agents
In
apps/web/client/src/app/project/[id]/_components/right-panel/dev-tab/upload-modal.tsx
around lines 1-251, the component lacks file size validation which may allow
very large files; add a top-level constant MAX_FILE_SIZE = 10 * 1024 * 1024
(10MB) and update handleFileSelect to filter incoming files against this limit,
only add files <= MAX_FILE_SIZE to selectedFiles, collect names of rejected
files and call toast to inform the user which files were skipped, and ensure the
smart default directory is set from the first accepted file (or reset if none
accepted); also ensure handleUpload remains disabled when selectedFiles is
empty.
🛠️ Refactor suggestion
Consider adding file size and type validation
The component should validate file sizes and types before attempting upload to prevent issues with large files or unsupported formats.
Add these validation utilities at the top of the component:
+const MAX_FILE_SIZE = 10 * 1024 * 1024; // 10MB
+const BLOCKED_EXTENSIONS = ['.exe', '.dll', '.app', '.dmg'];
+
export const UploadModal = observer(({
open,
onOpenChange,
files,
onSuccess,
}: UploadModalProps) => {Then update the handleFileSelect function:
const handleFileSelect = (event: React.ChangeEvent<HTMLInputElement>) => {
const files = event.target.files;
if (!files || files.length === 0) {
return;
}
const fileArray = Array.from(files);
+
+ // Validate files
+ const validFiles: File[] = [];
+ const errors: string[] = [];
+
+ for (const file of fileArray) {
+ const extension = '.' + file.name.split('.').pop()?.toLowerCase();
+
+ if (file.size > MAX_FILE_SIZE) {
+ errors.push(`${file.name} exceeds 10MB limit`);
+ continue;
+ }
+
+ if (BLOCKED_EXTENSIONS.includes(extension)) {
+ errors.push(`${file.name} has a blocked file type`);
+ continue;
+ }
+
+ validFiles.push(file);
+ }
+
+ if (errors.length > 0) {
+ toast('Some files were rejected', {
+ description: errors.join(', ')
+ });
+ }
+
- setSelectedFiles(fileArray);
+ setSelectedFiles(validFiles);
// Set smart default directory based on first file
- if (fileArray.length > 0) {
- const smartDir = getSmartDirectory(fileArray[0].name);
+ if (validFiles.length > 0) {
+ const smartDir = getSmartDirectory(validFiles[0].name);
setTargetDirectory(smartDir);
}
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| import { useEditorEngine } from '@/components/store/editor'; | |
| import { Button } from '@onlook/ui/button'; | |
| import { | |
| Dialog, | |
| DialogContent, | |
| DialogDescription, | |
| DialogFooter, | |
| DialogHeader, | |
| DialogTitle, | |
| } from '@onlook/ui/dialog'; | |
| import { Icons } from '@onlook/ui/icons'; | |
| import { | |
| Select, | |
| SelectContent, | |
| SelectItem, | |
| SelectTrigger, | |
| SelectValue, | |
| } from '@onlook/ui/select'; | |
| import { toast } from '@onlook/ui/sonner'; | |
| import { observer } from 'mobx-react-lite'; | |
| import { useEffect, useMemo, useRef, useState } from 'react'; | |
| interface UploadModalProps { | |
| open: boolean; | |
| onOpenChange: (open: boolean) => void; | |
| files: string[]; | |
| onSuccess?: () => void; | |
| } | |
| export const UploadModal = observer(({ | |
| open, | |
| onOpenChange, | |
| files, | |
| onSuccess, | |
| }: UploadModalProps) => { | |
| const editorEngine = useEditorEngine(); | |
| const [selectedFiles, setSelectedFiles] = useState<File[]>([]); | |
| const [targetDirectory, setTargetDirectory] = useState<string>('root'); | |
| const [isUploading, setIsUploading] = useState(false); | |
| const fileInputRef = useRef<HTMLInputElement>(null); | |
| const availableDirectories = useMemo(() => { | |
| const directories = new Set<string>(); | |
| files.forEach(file => { | |
| const parts = file.split('/'); | |
| for (let i = 1; i < parts.length; i++) { | |
| directories.add(parts.slice(0, i).join('/')); | |
| } | |
| }); | |
| return Array.from(directories).sort(); | |
| }, [files]); | |
| const getSmartDirectory = (filename: string): string => { | |
| const extension = filename.toLowerCase().split('.').pop(); | |
| const imageExtensions = ['jpg', 'jpeg', 'png', 'gif', 'svg', 'webp', 'ico']; | |
| if (extension && imageExtensions.includes(extension)) { | |
| if (availableDirectories.includes('public')) { | |
| return 'public'; | |
| } | |
| if (availableDirectories.includes('src/assets')) { | |
| return 'src/assets'; | |
| } | |
| if (availableDirectories.includes('assets')) { | |
| return 'assets'; | |
| } | |
| return availableDirectories.includes('public') ? 'public' : 'root'; | |
| } | |
| // For non-image files, use last selected directory or current file's directory | |
| if (editorEngine.ide.activeFile?.path) { | |
| const path = editorEngine.ide.activeFile.path; | |
| const lastSlash = path.lastIndexOf('/'); | |
| const dir = lastSlash > 0 ? path.substring(0, lastSlash) : 'root'; | |
| if (dir === 'root' || availableDirectories.includes(dir)) { | |
| return dir; | |
| } | |
| } | |
| return 'root'; | |
| }; | |
| const handleFileSelect = (event: React.ChangeEvent<HTMLInputElement>) => { | |
| const files = event.target.files; | |
| if (!files || files.length === 0) { | |
| return; | |
| } | |
| const fileArray = Array.from(files); | |
| setSelectedFiles(fileArray); | |
| // Set smart default directory based on first file | |
| if (fileArray.length > 0) { | |
| const smartDir = getSmartDirectory(fileArray[0].name); | |
| setTargetDirectory(smartDir); | |
| } | |
| }; | |
| const handleUpload = async () => { | |
| if (selectedFiles.length === 0) { | |
| return; | |
| } | |
| setIsUploading(true); | |
| try { | |
| for (const file of selectedFiles) { | |
| const directory = targetDirectory === 'root' ? '' : targetDirectory; | |
| const finalPath = directory ? `${directory}/${file.name}` : file.name; | |
| const content = await file.arrayBuffer(); | |
| await editorEngine.sandbox.writeFile(finalPath, new Uint8Array(content)); | |
| } | |
| await editorEngine.sandbox.listAllFiles(); | |
| const fileCount = selectedFiles.length; | |
| const fileText = fileCount === 1 ? selectedFiles[0]?.name ?? 'file' : `${fileCount} files`; | |
| toast(`Successfully uploaded ${fileText}!`); | |
| onOpenChange(false); | |
| onSuccess?.(); | |
| } catch (error) { | |
| console.error('Error uploading files:', error); | |
| toast('Failed to upload files', { description: 'Please try again' }); | |
| } finally { | |
| setIsUploading(false); | |
| } | |
| }; | |
| const removeFile = (index: number) => { | |
| setSelectedFiles(prev => prev.filter((_, i) => i !== index)); | |
| }; | |
| const reset = () => { | |
| setSelectedFiles([]); | |
| setTargetDirectory('root'); | |
| if (fileInputRef.current) { | |
| fileInputRef.current.value = ''; | |
| } | |
| }; | |
| useEffect(() => { | |
| if (!open) { | |
| reset(); | |
| } | |
| }, [open]); | |
| const displayPath = targetDirectory === 'root' ? '/' : `/${targetDirectory}/`; | |
| return ( | |
| <Dialog open={open} onOpenChange={onOpenChange}> | |
| <DialogContent> | |
| <DialogHeader> | |
| <DialogTitle>Upload Files</DialogTitle> | |
| <DialogDescription> | |
| Upload files to{' '} | |
| <code className="bg-background-secondary px-1 py-0.5 rounded text-xs"> | |
| {displayPath} | |
| </code> | |
| </DialogDescription> | |
| </DialogHeader> | |
| <div className="grid gap-4 py-4"> | |
| {/* File Selection */} | |
| <div className="space-y-2"> | |
| <label className="text-sm font-medium">Select Files</label> | |
| <div className="border-2 border-dashed border-border-primary rounded-lg p-4 text-center"> | |
| <input | |
| ref={fileInputRef} | |
| type="file" | |
| multiple | |
| className="hidden" | |
| onChange={handleFileSelect} | |
| /> | |
| <Button | |
| variant="ghost" | |
| onClick={() => fileInputRef.current?.click()} | |
| className="w-full" | |
| > | |
| <Icons.Upload className="h-4 w-4 mr-2" /> | |
| Choose Files | |
| </Button> | |
| </div> | |
| </div> | |
| {/* Selected Files List */} | |
| {selectedFiles.length > 0 && ( | |
| <div className="space-y-2"> | |
| <label className="text-sm font-medium">Selected Files</label> | |
| <div className="max-h-32 overflow-y-auto space-y-1"> | |
| {selectedFiles.map((file, index) => ( | |
| <div key={index} className="flex items-center justify-between bg-background-secondary p-2 rounded text-sm"> | |
| <span className="truncate">{file.name}</span> | |
| <Button | |
| variant="ghost" | |
| size="icon" | |
| className="h-6 w-6" | |
| onClick={() => removeFile(index)} | |
| > | |
| <Icons.CrossS className="h-3 w-3" /> | |
| </Button> | |
| </div> | |
| ))} | |
| </div> | |
| </div> | |
| )} | |
| {/* Directory Selection */} | |
| {selectedFiles.length > 0 && ( | |
| <div className="space-y-2"> | |
| <label className="text-sm font-medium">Target Directory</label> | |
| <Select value={targetDirectory} onValueChange={setTargetDirectory}> | |
| <SelectTrigger> | |
| <SelectValue placeholder="Select directory" /> | |
| </SelectTrigger> | |
| <SelectContent> | |
| <SelectItem value="root">/ (root)</SelectItem> | |
| {availableDirectories.map(dir => ( | |
| <SelectItem key={dir} value={dir}> | |
| /{dir}/ | |
| </SelectItem> | |
| ))} | |
| </SelectContent> | |
| </Select> | |
| </div> | |
| )} | |
| </div> | |
| <DialogFooter> | |
| <Button | |
| variant="ghost" | |
| onClick={() => onOpenChange(false)} | |
| disabled={isUploading} | |
| > | |
| Cancel | |
| </Button> | |
| <Button | |
| variant="outline" | |
| onClick={handleUpload} | |
| disabled={selectedFiles.length === 0 || isUploading} | |
| > | |
| {isUploading | |
| ? 'Uploading...' | |
| : `Upload ${selectedFiles.length} file${selectedFiles.length !== 1 ? 's' : ''}` | |
| } | |
| </Button> | |
| </DialogFooter> | |
| </DialogContent> | |
| </Dialog> | |
| ); | |
| }); | |
| // … existing imports … | |
| import { observer } from 'mobx-react-lite'; | |
| import { useEffect, useMemo, useRef, useState } from 'react'; | |
| import { toast } from '@onlook/ui/sonner'; | |
| // … other imports … | |
| // Add validation constants | |
| const MAX_FILE_SIZE = 10 * 1024 * 1024; // 10MB | |
| const BLOCKED_EXTENSIONS = ['.exe', '.dll', '.app', '.dmg']; | |
| export const UploadModal = observer(({ | |
| open, | |
| onOpenChange, | |
| files, | |
| onSuccess, | |
| }: UploadModalProps) => { | |
| // … other state/hooks … | |
| const handleFileSelect = (event: React.ChangeEvent<HTMLInputElement>) => { | |
| const files = event.target.files; | |
| if (!files || files.length === 0) { | |
| return; | |
| } | |
| const fileArray = Array.from(files); | |
| // Validate files | |
| const validFiles: File[] = []; | |
| const errors: string[] = []; | |
| for (const file of fileArray) { | |
| const extension = '.' + (file.name.split('.').pop() ?? '').toLowerCase(); | |
| if (file.size > MAX_FILE_SIZE) { | |
| errors.push(`${file.name} exceeds 10MB limit`); | |
| continue; | |
| } | |
| if (BLOCKED_EXTENSIONS.includes(extension)) { | |
| errors.push(`${file.name} has a blocked file type`); | |
| continue; | |
| } | |
| validFiles.push(file); | |
| } | |
| if (errors.length > 0) { | |
| toast('Some files were rejected', { | |
| description: errors.join(', ') | |
| }); | |
| } | |
| setSelectedFiles(validFiles); | |
| // Set smart default directory based on first valid file | |
| if (validFiles.length > 0) { | |
| const smartDir = getSmartDirectory(validFiles[0].name); | |
| setTargetDirectory(smartDir); | |
| } | |
| }; | |
| // … rest of component … | |
| }); |
🤖 Prompt for AI Agents
In
apps/web/client/src/app/project/[id]/_components/right-panel/dev-tab/upload-modal.tsx
around lines 1 to 251, add client-side file size/type validation and update
handleFileSelect to enforce it: create small helper utilities at the top (e.g.,
isValidFileType(filename, allowedTypes), isFileSizeWithinLimit(file, maxBytes),
and formatBytes) and a configuration for allowed MIME/extensions and max file
size (per-file and optional total upload limit); in handleFileSelect, iterate
incoming files, reject and toast any that exceed size or have unsupported
extensions, only accept the valid ones into setSelectedFiles, setTargetDirectory
with getSmartDirectory based on the first valid file, and prevent upload when no
valid files selected; ensure error messages show which file failed and why and
that the UI state (file input value, selectedFiles) is updated accordingly.
Comment on lines
+53
to
+81
| const getSmartDirectory = (filename: string): string => { | ||
| const extension = filename.toLowerCase().split('.').pop(); | ||
| const imageExtensions = ['jpg', 'jpeg', 'png', 'gif', 'svg', 'webp', 'ico']; | ||
|
|
||
| if (extension && imageExtensions.includes(extension)) { | ||
| if (availableDirectories.includes('public')) { | ||
| return 'public'; | ||
| } | ||
| if (availableDirectories.includes('src/assets')) { | ||
| return 'src/assets'; | ||
| } | ||
| if (availableDirectories.includes('assets')) { | ||
| return 'assets'; | ||
| } | ||
| return availableDirectories.includes('public') ? 'public' : 'root'; | ||
| } | ||
|
|
||
| // For non-image files, use last selected directory or current file's directory | ||
| if (editorEngine.ide.activeFile?.path) { | ||
| const path = editorEngine.ide.activeFile.path; | ||
| const lastSlash = path.lastIndexOf('/'); | ||
| const dir = lastSlash > 0 ? path.substring(0, lastSlash) : 'root'; | ||
| if (dir === 'root' || availableDirectories.includes(dir)) { | ||
| return dir; | ||
| } | ||
| } | ||
|
|
||
| return 'root'; | ||
| }; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider the case when the selected file is a file and not a folder
Based on the PR comments, when a file (not a folder) is selected and the user tries to upload, the current logic tries to use that file's directory. However, the directory extraction logic on lines 72-78 needs to handle edge cases better when editorEngine.ide.activeFile.path is a file path.
🏁 Script executed:
#!/bin/bash
# Check how activeFile.path is structured in the codebase
ast-grep --pattern 'activeFile.path' | head -20
# Check if there are any utilities for path handling
rg -A 3 "dirname|basename|path\.(dirname|basename)" --type ts --type tsxLength of output: 150
🏁 Script executed:
#!/bin/bash
# Locate the upload-modal.tsx file
FILE=$(fd upload-modal.tsx)
echo "Found file: $FILE"
# Print the first 200 lines to inspect imports and context
sed -n '1,200p' "$FILE"
# Find all uses of activeFile.path in TS and TSX files
rg -n "activeFile\.path" -g "*.ts" -g "*.tsx"
# Check for Node.js 'path' module imports
rg -n "from ['\"]path['\"]" -g "*.ts" -g "*.tsx"
rg -n "require('path')" -g "*.ts" -g "*.tsx"
# Look for dirname and basename function usage
rg -n "dirname\(" -g "*.ts" -g "*.tsx"
rg -n "basename\(" -g "*.ts" -g "*.tsx"Length of output: 5964
#!/bin/bash
TARGET="apps/web/client/src/app/project/[id]/_components/right-panel/dev-tab/upload-modal.tsx"
# Verify the file exists and print key sections
if [[ -f "$TARGET" ]]; then
echo "---- Imports and context (lines 1–50) ----"
sed -n '1,50p' "$TARGET"
echo
echo "---- getSmartDirectory definition (lines 50–120) ----"
sed -n '50,120p' "$TARGET"
echo
echo "---- availableDirectories usages ----"
grep -n "availableDirectories" "$TARGET"
else
echo "ERROR: File not found: $TARGET"
fiEnsure folder selections aren’t treated as files in getSmartDirectory
The current logic always takes editorEngine.ide.activeFile.path, strips off its final segment, and uses that as the upload directory—so if a user clicks on a folder, the code will instead pick its parent (or fall back to “root”) rather than the folder itself.
• File: apps/web/client/src/app/project/[id]/_components/right-panel/dev-tab/upload-modal.tsx
• Lines 72–78 (inside getSmartDirectory)
Proposed change: first check whether activeFile.path is itself a valid upload directory, and return it directly. Fallback to “parent” only when it truly represents a file’s path.
// For non-image files, use last selected directory or current file's directory
if (editorEngine.ide.activeFile?.path) {
- const path = editorEngine.ide.activeFile.path;
- const lastSlash = path.lastIndexOf('/');
- const dir = lastSlash > 0 ? path.substring(0, lastSlash) : 'root';
- if (dir === 'root' || availableDirectories.includes(dir)) {
- return dir;
- }
+ const current = editorEngine.ide.activeFile.path;
+ // If the clicked item is itself a valid directory, use it
+ if (availableDirectories.includes(current)) {
+ return current;
+ }
+ // Otherwise treat it as a file and use its parent folder
+ const idx = current.lastIndexOf('/');
+ const parent = idx > 0 ? current.substring(0, idx) : 'root';
+ if (parent === 'root' || availableDirectories.includes(parent)) {
+ return parent;
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const getSmartDirectory = (filename: string): string => { | |
| const extension = filename.toLowerCase().split('.').pop(); | |
| const imageExtensions = ['jpg', 'jpeg', 'png', 'gif', 'svg', 'webp', 'ico']; | |
| if (extension && imageExtensions.includes(extension)) { | |
| if (availableDirectories.includes('public')) { | |
| return 'public'; | |
| } | |
| if (availableDirectories.includes('src/assets')) { | |
| return 'src/assets'; | |
| } | |
| if (availableDirectories.includes('assets')) { | |
| return 'assets'; | |
| } | |
| return availableDirectories.includes('public') ? 'public' : 'root'; | |
| } | |
| // For non-image files, use last selected directory or current file's directory | |
| if (editorEngine.ide.activeFile?.path) { | |
| const path = editorEngine.ide.activeFile.path; | |
| const lastSlash = path.lastIndexOf('/'); | |
| const dir = lastSlash > 0 ? path.substring(0, lastSlash) : 'root'; | |
| if (dir === 'root' || availableDirectories.includes(dir)) { | |
| return dir; | |
| } | |
| } | |
| return 'root'; | |
| }; | |
| const getSmartDirectory = (filename: string): string => { | |
| const extension = filename.toLowerCase().split('.').pop(); | |
| const imageExtensions = ['jpg', 'jpeg', 'png', 'gif', 'svg', 'webp', 'ico']; | |
| if (extension && imageExtensions.includes(extension)) { | |
| if (availableDirectories.includes('public')) { | |
| return 'public'; | |
| } | |
| if (availableDirectories.includes('src/assets')) { | |
| return 'src/assets'; | |
| } | |
| if (availableDirectories.includes('assets')) { | |
| return 'assets'; | |
| } | |
| return availableDirectories.includes('public') ? 'public' : 'root'; | |
| } | |
| // For non-image files, use last selected directory or current file's directory | |
| if (editorEngine.ide.activeFile?.path) { | |
| const current = editorEngine.ide.activeFile.path; | |
| // If the clicked item is itself a valid directory, use it | |
| if (availableDirectories.includes(current)) { | |
| return current; | |
| } | |
| // Otherwise treat it as a file and use its parent folder | |
| const idx = current.lastIndexOf('/'); | |
| const parent = idx > 0 ? current.substring(0, idx) : 'root'; | |
| if (parent === 'root' || availableDirectories.includes(parent)) { | |
| return parent; | |
| } | |
| } | |
| return 'root'; | |
| }; |
🤖 Prompt for AI Agents
In
apps/web/client/src/app/project/[id]/_components/right-panel/dev-tab/upload-modal.tsx
around lines 53 to 81, getSmartDirectory currently always treats
editorEngine.ide.activeFile.path as a file and returns its parent, which causes
clicking a folder to select its parent; change the logic to first check if
activeFile.path itself is a valid upload directory (i.e., activeFile.path exists
and availableDirectories.includes(activeFile.path)); if so return
activeFile.path directly; otherwise fall back to computing the parent directory
(strip after last '/') and return that if valid, and finally fall back to
'root'.
Comment on lines
+54
to
+81
| const extension = filename.toLowerCase().split('.').pop(); | ||
| const imageExtensions = ['jpg', 'jpeg', 'png', 'gif', 'svg', 'webp', 'ico']; | ||
|
|
||
| if (extension && imageExtensions.includes(extension)) { | ||
| if (availableDirectories.includes('public')) { | ||
| return 'public'; | ||
| } | ||
| if (availableDirectories.includes('src/assets')) { | ||
| return 'src/assets'; | ||
| } | ||
| if (availableDirectories.includes('assets')) { | ||
| return 'assets'; | ||
| } | ||
| return availableDirectories.includes('public') ? 'public' : 'root'; | ||
| } | ||
|
|
||
| // For non-image files, use last selected directory or current file's directory | ||
| if (editorEngine.ide.activeFile?.path) { | ||
| const path = editorEngine.ide.activeFile.path; | ||
| const lastSlash = path.lastIndexOf('/'); | ||
| const dir = lastSlash > 0 ? path.substring(0, lastSlash) : 'root'; | ||
| if (dir === 'root' || availableDirectories.includes(dir)) { | ||
| return dir; | ||
| } | ||
| } | ||
|
|
||
| return 'root'; | ||
| }; |
There was a problem hiding this comment.
Issue with duplicate directory logic in smart directory selection
The getSmartDirectory function has a redundant check on Line 67. After already checking for 'public' on Line 58, the return statement on Line 67 checks again for the same condition.
Apply this diff to fix the redundancy:
if (extension && imageExtensions.includes(extension)) {
if (availableDirectories.includes('public')) {
return 'public';
}
if (availableDirectories.includes('src/assets')) {
return 'src/assets';
}
if (availableDirectories.includes('assets')) {
return 'assets';
}
- return availableDirectories.includes('public') ? 'public' : 'root';
+ return 'root';
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const extension = filename.toLowerCase().split('.').pop(); | |
| const imageExtensions = ['jpg', 'jpeg', 'png', 'gif', 'svg', 'webp', 'ico']; | |
| if (extension && imageExtensions.includes(extension)) { | |
| if (availableDirectories.includes('public')) { | |
| return 'public'; | |
| } | |
| if (availableDirectories.includes('src/assets')) { | |
| return 'src/assets'; | |
| } | |
| if (availableDirectories.includes('assets')) { | |
| return 'assets'; | |
| } | |
| return availableDirectories.includes('public') ? 'public' : 'root'; | |
| } | |
| // For non-image files, use last selected directory or current file's directory | |
| if (editorEngine.ide.activeFile?.path) { | |
| const path = editorEngine.ide.activeFile.path; | |
| const lastSlash = path.lastIndexOf('/'); | |
| const dir = lastSlash > 0 ? path.substring(0, lastSlash) : 'root'; | |
| if (dir === 'root' || availableDirectories.includes(dir)) { | |
| return dir; | |
| } | |
| } | |
| return 'root'; | |
| }; | |
| const extension = filename.toLowerCase().split('.').pop(); | |
| const imageExtensions = ['jpg', 'jpeg', 'png', 'gif', 'svg', 'webp', 'ico']; | |
| if (extension && imageExtensions.includes(extension)) { | |
| if (availableDirectories.includes('public')) { | |
| return 'public'; | |
| } | |
| if (availableDirectories.includes('src/assets')) { | |
| return 'src/assets'; | |
| } | |
| if (availableDirectories.includes('assets')) { | |
| return 'assets'; | |
| } | |
| return 'root'; | |
| } | |
| // For non-image files, use last selected directory or current file's directory | |
| if (editorEngine.ide.activeFile?.path) { | |
| const path = editorEngine.ide.activeFile.path; | |
| const lastSlash = path.lastIndexOf('/'); | |
| const dir = lastSlash > 0 ? path.substring(0, lastSlash) : 'root'; | |
| if (dir === 'root' || availableDirectories.includes(dir)) { | |
| return dir; | |
| } | |
| } | |
| return 'root'; |
🤖 Prompt for AI Agents
In
apps/web/client/src/app/project/[id]/_components/right-panel/dev-tab/upload-modal.tsx
around lines 54-81, remove the redundant availableDirectories.includes('public')
check in the image-extension branch (the ternary return on line 67) because
'public' was already checked earlier; replace that final return with a plain
return 'root' so the function falls back to 'root' when no preferred image
directories are found.
| if (availableDirectories.includes('assets')) { | ||
| return 'assets'; | ||
| } | ||
| return availableDirectories.includes('public') ? 'public' : 'root'; |
There was a problem hiding this comment.
Redundant public directory check on line 67
Line 67 contains redundant logic: it checks if availableDirectories.includes('public') and returns 'public' if true, but this condition has already been checked on line 58. If the code reaches line 67, we know 'public' is not in availableDirectories.
- return availableDirectories.includes('public') ? 'public' : 'root';
+ return 'root';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| return availableDirectories.includes('public') ? 'public' : 'root'; | |
| return 'root'; |
🤖 Prompt for AI Agents
In
apps/web/client/src/app/project/[id]/_components/right-panel/dev-tab/upload-modal.tsx
around line 67, the conditional "return availableDirectories.includes('public')
? 'public' : 'root';" is redundant because 'public' was already checked earlier;
remove the repeated includes check and simply return 'root' as the fallback (or
collapse the earlier and current checks into a single conditional that returns
'public' if present, otherwise 'root') so the function returns 'root'
unconditionally at this point.
Comment on lines
+99
to
+128
| const handleUpload = async () => { | ||
| if (selectedFiles.length === 0) { | ||
| return; | ||
| } | ||
|
|
||
| setIsUploading(true); | ||
| try { | ||
| for (const file of selectedFiles) { | ||
| const directory = targetDirectory === 'root' ? '' : targetDirectory; | ||
| const finalPath = directory ? `${directory}/${file.name}` : file.name; | ||
|
|
||
| const content = await file.arrayBuffer(); | ||
| await editorEngine.sandbox.writeFile(finalPath, new Uint8Array(content)); | ||
| } | ||
|
|
||
| await editorEngine.sandbox.listAllFiles(); | ||
|
|
||
| const fileCount = selectedFiles.length; | ||
| const fileText = fileCount === 1 ? selectedFiles[0]?.name ?? 'file' : `${fileCount} files`; | ||
| toast(`Successfully uploaded ${fileText}!`); | ||
|
|
||
| onOpenChange(false); | ||
| onSuccess?.(); | ||
| } catch (error) { | ||
| console.error('Error uploading files:', error); | ||
| toast('Failed to upload files', { description: 'Please try again' }); | ||
| } finally { | ||
| setIsUploading(false); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for individual file failures
The current implementation stops uploading all files if one fails. Consider handling individual file failures gracefully and providing feedback about which files succeeded and which failed.
const handleUpload = async () => {
if (selectedFiles.length === 0) {
return;
}
setIsUploading(true);
+ const failedFiles: string[] = [];
+ const successfulFiles: string[] = [];
+
try {
for (const file of selectedFiles) {
const directory = targetDirectory === 'root' ? '' : targetDirectory;
const finalPath = directory ? `${directory}/${file.name}` : file.name;
- const content = await file.arrayBuffer();
- await editorEngine.sandbox.writeFile(finalPath, new Uint8Array(content));
+ try {
+ const content = await file.arrayBuffer();
+ await editorEngine.sandbox.writeFile(finalPath, new Uint8Array(content));
+ successfulFiles.push(file.name);
+ } catch (error) {
+ console.error(`Error uploading ${file.name}:`, error);
+ failedFiles.push(file.name);
+ }
}
await editorEngine.sandbox.listAllFiles();
- const fileCount = selectedFiles.length;
- const fileText = fileCount === 1 ? selectedFiles[0]?.name ?? 'file' : `${fileCount} files`;
- toast(`Successfully uploaded ${fileText}!`);
+ if (successfulFiles.length > 0) {
+ const fileText = successfulFiles.length === 1 ? successfulFiles[0] : `${successfulFiles.length} files`;
+ toast(`Successfully uploaded ${fileText}!`);
+ }
+
+ if (failedFiles.length > 0) {
+ const failedText = failedFiles.length === 1 ? failedFiles[0] : `${failedFiles.length} files`;
+ toast(`Failed to upload ${failedText}`, {
+ description: failedFiles.length === 1 ? undefined : failedFiles.join(', ')
+ });
+ }
- onOpenChange(false);
- onSuccess?.();
+ if (successfulFiles.length > 0) {
+ onOpenChange(false);
+ onSuccess?.();
+ }
} catch (error) {
console.error('Error uploading files:', error);
toast('Failed to upload files', { description: 'Please try again' });
} finally {
setIsUploading(false);
}
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const handleUpload = async () => { | |
| if (selectedFiles.length === 0) { | |
| return; | |
| } | |
| setIsUploading(true); | |
| try { | |
| for (const file of selectedFiles) { | |
| const directory = targetDirectory === 'root' ? '' : targetDirectory; | |
| const finalPath = directory ? `${directory}/${file.name}` : file.name; | |
| const content = await file.arrayBuffer(); | |
| await editorEngine.sandbox.writeFile(finalPath, new Uint8Array(content)); | |
| } | |
| await editorEngine.sandbox.listAllFiles(); | |
| const fileCount = selectedFiles.length; | |
| const fileText = fileCount === 1 ? selectedFiles[0]?.name ?? 'file' : `${fileCount} files`; | |
| toast(`Successfully uploaded ${fileText}!`); | |
| onOpenChange(false); | |
| onSuccess?.(); | |
| } catch (error) { | |
| console.error('Error uploading files:', error); | |
| toast('Failed to upload files', { description: 'Please try again' }); | |
| } finally { | |
| setIsUploading(false); | |
| } | |
| }; | |
| const handleUpload = async () => { | |
| if (selectedFiles.length === 0) { | |
| return; | |
| } | |
| setIsUploading(true); | |
| const failedFiles: string[] = []; | |
| const successfulFiles: string[] = []; | |
| try { | |
| for (const file of selectedFiles) { | |
| const directory = targetDirectory === 'root' ? '' : targetDirectory; | |
| const finalPath = directory ? `${directory}/${file.name}` : file.name; | |
| try { | |
| const content = await file.arrayBuffer(); | |
| await editorEngine.sandbox.writeFile(finalPath, new Uint8Array(content)); | |
| successfulFiles.push(file.name); | |
| } catch (error) { | |
| console.error(`Error uploading ${file.name}:`, error); | |
| failedFiles.push(file.name); | |
| } | |
| } | |
| await editorEngine.sandbox.listAllFiles(); | |
| if (successfulFiles.length > 0) { | |
| const fileText = successfulFiles.length === 1 | |
| ? successfulFiles[0] | |
| : `${successfulFiles.length} files`; | |
| toast(`Successfully uploaded ${fileText}!`); | |
| } | |
| if (failedFiles.length > 0) { | |
| const failedText = failedFiles.length === 1 | |
| ? failedFiles[0] | |
| : `${failedFiles.length} files`; | |
| toast(`Failed to upload ${failedText}`, { | |
| description: failedFiles.length === 1 | |
| ? undefined | |
| : failedFiles.join(', ') | |
| }); | |
| } | |
| if (successfulFiles.length > 0) { | |
| onOpenChange(false); | |
| onSuccess?.(); | |
| } | |
| } catch (error) { | |
| console.error('Error uploading files:', error); | |
| toast('Failed to upload files', { description: 'Please try again' }); | |
| } finally { | |
| setIsUploading(false); | |
| } | |
| }; |
🤖 Prompt for AI Agents
In
apps/web/client/src/app/project/[id]/_components/right-panel/dev-tab/upload-modal.tsx
around lines 99 to 128, the current upload loop aborts all uploads if one file
fails; wrap the per-file write in its own try/catch so failures don’t stop the
rest, collect successful and failed file names into two arrays, log per-file
errors and continue, call editorEngine.sandbox.listAllFiles() after the loop,
then show a toast summarizing how many files succeeded and listing failed file
names (and include a different message if all succeeded or all failed), call
onSuccess() only if there is at least one success, and ensure
setIsUploading(false) and onOpenChange(false) are handled appropriately after
reporting results.
🛠️ Refactor suggestion
Add error handling for individual file uploads
Currently, if one file fails to upload, the entire batch stops. Consider handling individual file failures gracefully to allow successful uploads to proceed.
Apply this refactor to handle individual file failures:
const handleUpload = async () => {
if (selectedFiles.length === 0) {
return;
}
setIsUploading(true);
+ const failedFiles: string[] = [];
+ let successCount = 0;
+
try {
for (const file of selectedFiles) {
const directory = targetDirectory === 'root' ? '' : targetDirectory;
const finalPath = directory ? `${directory}/${file.name}` : file.name;
- const content = await file.arrayBuffer();
- await editorEngine.sandbox.writeFile(finalPath, new Uint8Array(content));
+ try {
+ const content = await file.arrayBuffer();
+ await editorEngine.sandbox.writeFile(finalPath, new Uint8Array(content));
+ successCount++;
+ } catch (error) {
+ console.error(`Failed to upload ${file.name}:`, error);
+ failedFiles.push(file.name);
+ }
}
await editorEngine.sandbox.listAllFiles();
- const fileCount = selectedFiles.length;
- const fileText = fileCount === 1 ? selectedFiles[0]?.name ?? 'file' : `${fileCount} files`;
- toast(`Successfully uploaded ${fileText}!`);
+ if (successCount > 0) {
+ const fileText = successCount === 1 ? 'file' : `${successCount} files`;
+ toast(`Successfully uploaded ${fileText}!`);
+ }
+
+ if (failedFiles.length > 0) {
+ toast('Some files failed to upload', {
+ description: `Failed: ${failedFiles.join(', ')}`
+ });
+ }
- onOpenChange(false);
- onSuccess?.();
+ if (successCount > 0) {
+ onOpenChange(false);
+ onSuccess?.();
+ }
} catch (error) {
console.error('Error uploading files:', error);
toast('Failed to upload files', { description: 'Please try again' });
} finally {
setIsUploading(false);
}
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const handleUpload = async () => { | |
| if (selectedFiles.length === 0) { | |
| return; | |
| } | |
| setIsUploading(true); | |
| try { | |
| for (const file of selectedFiles) { | |
| const directory = targetDirectory === 'root' ? '' : targetDirectory; | |
| const finalPath = directory ? `${directory}/${file.name}` : file.name; | |
| const content = await file.arrayBuffer(); | |
| await editorEngine.sandbox.writeFile(finalPath, new Uint8Array(content)); | |
| } | |
| await editorEngine.sandbox.listAllFiles(); | |
| const fileCount = selectedFiles.length; | |
| const fileText = fileCount === 1 ? selectedFiles[0]?.name ?? 'file' : `${fileCount} files`; | |
| toast(`Successfully uploaded ${fileText}!`); | |
| onOpenChange(false); | |
| onSuccess?.(); | |
| } catch (error) { | |
| console.error('Error uploading files:', error); | |
| toast('Failed to upload files', { description: 'Please try again' }); | |
| } finally { | |
| setIsUploading(false); | |
| } | |
| }; | |
| const handleUpload = async () => { | |
| if (selectedFiles.length === 0) { | |
| return; | |
| } | |
| setIsUploading(true); | |
| const failedFiles: string[] = []; | |
| let successCount = 0; | |
| try { | |
| for (const file of selectedFiles) { | |
| const directory = targetDirectory === 'root' ? '' : targetDirectory; | |
| const finalPath = directory ? `${directory}/${file.name}` : file.name; | |
| try { | |
| const content = await file.arrayBuffer(); | |
| await editorEngine.sandbox.writeFile(finalPath, new Uint8Array(content)); | |
| successCount++; | |
| } catch (error) { | |
| console.error(`Failed to upload ${file.name}:`, error); | |
| failedFiles.push(file.name); | |
| } | |
| } | |
| await editorEngine.sandbox.listAllFiles(); | |
| if (successCount > 0) { | |
| const fileText = successCount === 1 ? 'file' : `${successCount} files`; | |
| toast(`Successfully uploaded ${fileText}!`); | |
| } | |
| if (failedFiles.length > 0) { | |
| toast('Some files failed to upload', { | |
| description: `Failed: ${failedFiles.join(', ')}` | |
| }); | |
| } | |
| if (successCount > 0) { | |
| onOpenChange(false); | |
| onSuccess?.(); | |
| } | |
| } catch (error) { | |
| console.error('Error uploading files:', error); | |
| toast('Failed to upload files', { description: 'Please try again' }); | |
| } finally { | |
| setIsUploading(false); | |
| } | |
| }; |
🤖 Prompt for AI Agents
In
apps/web/client/src/app/project/[id]/_components/right-panel/dev-tab/upload-modal.tsx
around lines 99–128, the current upload loop aborts the whole batch when a
single file fails; change it to handle per-file errors by wrapping the writeFile
call for each file in its own try/catch, accumulate successful and failed
filenames into two arrays, log individual errors (including filename and error),
continue uploading remaining files, and after the loop call
editorEngine.sandbox.listAllFiles(); then show a toast summary that reports how
many files succeeded and lists failed filenames (or a brief failure message),
call onSuccess only if at least one file succeeded, still close the modal if
appropriate, and ensure setIsUploading(false) remains in the finally block.
Comment on lines
+108
to
+109
| const finalPath = directory ? `${directory}/${file.name}` : file.name; | ||
|
|
There was a problem hiding this comment.
Validate file paths before uploading
The code should validate that the constructed file path doesn't contain dangerous patterns (e.g., ../, absolute paths) before writing to the sandbox.
const directory = targetDirectory === 'root' ? '' : targetDirectory;
const finalPath = directory ? `${directory}/${file.name}` : file.name;
+
+// Validate the path doesn't contain dangerous patterns
+if (finalPath.includes('../') || finalPath.startsWith('/')) {
+ console.error(`Invalid file path: ${finalPath}`);
+ failedFiles.push(file.name);
+ continue;
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const finalPath = directory ? `${directory}/${file.name}` : file.name; | |
| const directory = targetDirectory === 'root' ? '' : targetDirectory; | |
| const finalPath = directory ? `${directory}/${file.name}` : file.name; | |
| // Validate the path doesn't contain dangerous patterns | |
| if (finalPath.includes('../') || finalPath.startsWith('/')) { | |
| console.error(`Invalid file path: ${finalPath}`); | |
| failedFiles.push(file.name); | |
| continue; | |
| } |
Rish-it
added a commit
to Rish-it/onlook
that referenced
this pull request
Aug 13, 2025
* editor improvements + Upload files
This was referenced Aug 13, 2025
Kitenite
pushed a commit
that referenced
this pull request
Aug 14, 2025
* editor improvements + Upload files
zongkelong
pushed a commit
to zongkelong/ulook
that referenced
this pull request
Aug 15, 2025
* 'main' of https://github.com/onlook-dev/onlook: (41 commits) feat: fix delete project (onlook-dev#2683) feat: reduce search latency (onlook-dev#2682) feat: revamp projects ux (onlook-dev#2672) Upload fix (onlook-dev#2677) feat: using firecrawl to screenshot (onlook-dev#2665) fix: file upload build error (onlook-dev#2674) feat: new project frames (onlook-dev#2673) feat: self hosting docs (onlook-dev#2671) fix: docs layout + troubleshoot section (onlook-dev#2670) fix(template): suppress hydration mismatch Feat/editor improvements (onlook-dev#2601) fix: make message content selectable (onlook-dev#2664) bugs: transient stream error (onlook-dev#2663) feat: update migration files (onlook-dev#2659) Start from Blank + Import buttons on the home page (onlook-dev#2653) fix: snapshot type conflict and mastra can't view image (onlook-dev#2656) fix: typecheck snapshot (onlook-dev#2655) feat: clean up mastra branch (onlook-dev#2654) feat: mastra no storage (onlook-dev#2650) feat: update ai packages (onlook-dev#2648) ...
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Related Issues
fixes #2576
Type of Change
Testing
Screenshots (if applicable)
Additional Notes
Important
Enhance editor with file upload capabilities, UI improvements, and database schema updates for better file management and user interaction.
rate_limitstable with columns for user, subscription, and usage tracking in0016_quiet_hellion.sql.suggestionscolumn toconversationsandstripe_current_period_startandstripe_current_period_endtosubscriptions.stripe_customer_idtousers.authschema.UploadModalcomponent inupload-modal.tsxfor file uploads with directory selection.CodeControlsincode-controls.tsxto include file upload and creation options via dropdown menu.FileTreeinfile-tree.tsxto improve loading and search experience.index.tsx.DevTabcomponent to improve file management and editor interactions.This description was created by
for 1ff0342. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit
New Features
Style