Upload fix #2677
Upload fix #2677
Conversation
|
@Rish-it is attempting to deploy a commit to the Onlook Team on Vercel. A member of the Team first needs to authorize it. |
|
Caution Review failedThe pull request is closed. WalkthroughDerives basePath as the active file's containing directory, passes it into UploadModal, and updates UploadModal to accept basePath, choose target directory accordingly, track per-file upload results, and adjust post-upload toasts and modal behavior based on success/partial/failure. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CodeControls
participant UploadModal
participant FileSystem as FileSystemService
participant IDE as IDE/FileList
participant Toast as ToastService
User->>CodeControls: Open Upload
CodeControls->>UploadModal: basePath (dir of active file)
User->>UploadModal: Select files, Confirm
UploadModal->>UploadModal: Resolve targetDir (basePath or root)
loop for each file
UploadModal->>FileSystem: Write file (binary/text)
FileSystem-->>UploadModal: success/failure
UploadModal->>UploadModal: Record per-file result
end
alt All succeed
UploadModal->>IDE: listAllFiles/refresh
UploadModal->>Toast: Success (count/name)
UploadModal-->>User: Close modal, onSuccess
else Some succeed
UploadModal->>IDE: listAllFiles/refresh
UploadModal->>Toast: Partial success (counts)
UploadModal-->>User: Keep modal open
else All fail
UploadModal->>Toast: Failure
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Assessment against linked issues
Possibly related PRs
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI 💡 Knowledge Base configuration:
You can enable these settings in your CodeRabbit configuration. 📒 Files selected for processing (1)
✨ 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/web/client/src/app/project/[id]/_components/right-panel/dev-tab/upload-modal.tsx (2)
126-126: Consider await for async operationThe
listAllFiles()method appears to be asynchronous based on the pattern used elsewhere in the codebase. Consider awaiting this operation to ensure the file list is refreshed before showing the success toast.- editorEngine.sandbox.listAllFiles(); + await editorEngine.sandbox.listAllFiles();The same applies to line 145:
- editorEngine.sandbox.listAllFiles(); + await editorEngine.sandbox.listAllFiles();
110-117: Consider adding file-specific error trackingWhile the current implementation tracks success/failure per file, it doesn't capture which specific files failed. This information could be valuable for users when dealing with partial failures.
Consider tracking failed filenames to provide more detailed feedback:
const uploadResults: boolean[] = []; + const failedFiles: string[] = []; for (const file of selectedFiles) { const directory = targetDirectory === 'root' ? '' : targetDirectory; const finalPath = directory ? `${directory}/${file.name}` : file.name; let success: boolean; if (isBinaryFile(file.name)) { const content = await file.arrayBuffer(); success = await editorEngine.sandbox.writeBinaryFile(finalPath, new Uint8Array(content)); } else { const content = await file.text(); success = await editorEngine.sandbox.writeFile(finalPath, content); } uploadResults.push(success); + if (!success) { + failedFiles.push(file.name); + } }Then update the partial failure message:
toast(`Partially uploaded files`, { - description: `${successCount} uploaded successfully, ${failedCount} failed. Please try again for the failed files.` + description: `${successCount} uploaded successfully, ${failedCount} failed (${failedFiles.join(', ')}). Please try again for the failed files.` });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (2)
apps/web/client/src/app/project/[id]/_components/right-panel/dev-tab/code-controls.tsx(2 hunks)apps/web/client/src/app/project/[id]/_components/right-panel/dev-tab/upload-modal.tsx(3 hunks)
🔇 Additional comments (5)
apps/web/client/src/app/project/[id]/_components/right-panel/dev-tab/code-controls.tsx (2)
30-36: Potential issue with root-level filesThe current implementation returns an empty string for files at the root level (when
lastSlashis 0 or -1), which is correct. However, the conditionlastSlash > 0means files like/file.txt(with lastSlash = 0) will return an empty string, while files likefile.txt(with lastSlash = -1) will also return an empty string. This is the expected behavior, but consider adding a comment to clarify this edge case handling.
135-135: LGTM! Proper prop passing to UploadModalThe
basePathprop is correctly passed to theUploadModalcomponent, ensuring that uploaded files will be placed in the same directory as the active file rather than under the file itself.apps/web/client/src/app/project/[id]/_components/right-panel/dev-tab/upload-modal.tsx (3)
104-120: Critical fix for upload success detectionExcellent implementation! The code now properly checks the boolean return values from
writeBinaryFileandwriteFileoperations, which was the root cause of the false success messages. This ensures that the UI accurately reflects the actual upload status.
122-146: Well-structured error handling with clear user feedbackThe three-way branching logic provides excellent user feedback:
- Complete success → success toast and modal closure
- Complete failure → clear failure message
- Partial success → detailed breakdown with counts
The decision to keep the modal open for partial failures is user-friendly, allowing immediate retry of failed uploads.
73-79: Smart directory resolution using basePathThe implementation correctly uses the provided
basePathwhen it exists inavailableDirectories, falling back to 'root' otherwise. This ensures files are uploaded to the same directory as the active file, fixing the issue where files were incorrectly placed under the active file.
* '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) ...
2 participants
Description
Fixed three critical bugs in the file upload functionality:
False Success Messages
writeFile/writeBinaryFileoperations and only shows success when all uploads actually succeed.Improved Error Handling
Added comprehensive error feedback with three scenarios:
Enhanced User Experience
Related Issues
Fixes #2675
Closes #2675
Type of Change
Testing
bun run typecheck)bun test)Screenshots
Additional Notes
The fix addresses the root cause where the upload logic was only catching exceptions but not checking the boolean return values from sandbox write operations.
Files could fail to write (returning
false) without throwing an error, leading to false success messages.Important
Fixes file upload feedback in
upload-modal.tsxby checking operation results and providing detailed success/failure messages.upload-modal.tsxby checking return values fromwriteFileandwriteBinaryFile.upload-modal.tsx.basePathcalculation incode-controls.tsxto derive directory path fromactiveFile.basePathtoUploadModalincode-controls.tsx.uploadResultsarray to track success of each file upload inupload-modal.tsx.upload-modal.tsxto reflect accurate upload status.This description was created by
for 92a77c3. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor