feat(sftp): bundle folder uploads and improve cancel/delete operations - #116
Conversation
- Bundle folder uploads as single tasks showing aggregate progress - Add unique file transfer IDs for proper cancellation tracking - Fix cancel button to call cancelExternalUpload for external uploads - Improve backend cancel detection using cancelled flag instead of error message - Use SSH exec with rm -rf for fast folder deletion on remote servers - Add FolderUp icon for folder upload tasks in transfer queue - Add i18n key for upload cancelled message Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 70a172216a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Refactors the SFTP upload mechanism to provide a more unified and robust experience. - **Bundles folder uploads**: When uploading a folder from the local machine, it now appears as a single, aggregated task in the UI, showing overall progress instead of individual files. - **Enhances cancellation**: Implements a new upload service and controller to manage transfers, allowing for more immediate and reliable cancellation of both individual files and bundled folder uploads. - **Improves UI feedback**: Adds dedicated buttons for cancelling active uploads and dismissing completed, failed, or cancelled tasks. - **Faster folder deletion**: Utilizes SSH `rm -rf` command for rapid remote folder deletion, falling back to SFTP rmdir if SSH exec is unavailable. - Updates internationalization keys for single item deletion confirmation.
Adopts a "fail-fast" strategy for SFTP uploads, stopping the entire transfer process upon encountering any error during file or directory operations. Introduces a new `isFatalUploadError` helper to consolidate and expand the definition of errors that should halt an upload, now explicitly including cases where the target directory is deleted or inaccessible during the transfer. Removes specific fatal error checks from various components, streamlining error propagation and simplifying the overall error handling logic. Ensures "Scanning files..." placeholder tasks are consistently removed when actual upload tasks are added, preventing potential state inconsistencies.
Disables render tracking by default, making it an opt-in debugging feature. This change significantly reduces development log noise by only logging render information when explicitly enabled via a debug flag.
Also, explicitly sets `aria-describedby={undefined}` on dialog content. This ensures proper accessibility semantics when a description is not provided, preventing potential issues or warnings.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6196c6e3c3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6196c6e3c3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- uploadService.ts: Mark all bundle tasks as cancelled on early loop exit - uploadService.ts: Don't set wasCancelled for actual errors, preserving the error result for proper UI feedback - useSftpModalTransfers.ts: Re-throw real errors instead of masking them as cancellations, only return cancelled:true for user-initiated cancels This ensures genuine failures (permission denied, disk full, connection loss) are reported as errors with proper toast messages, while user cancellations are correctly identified and handled. Co-Authored-By: Claude <noreply@anthropic.com>
Addressed Review FeedbackFixed the issues raised in the code review in commit c1959ad: 1. Mark bundle task cancelled on early loop exitWhen if (controller?.isCancelled()) {
wasCancelled = true;
// Mark all created tasks as cancelled before breaking
for (const [, bundleTaskId] of bundleTaskIds) {
const progress = bundleProgress.get(bundleTaskId);
if (progress && progress.completedCount < progress.fileCount) {
callbacks?.onTaskCancelled?.(bundleTaskId);
}
}
break;
}2. Treat upload failures as failures, not cancellationsRemoved 3. Don't mask write errors as cancelled uploadsUpdated } catch (error) {
const wasCancelled = cancelledTransferIdsRef.current.has(taskId);
if (wasCancelled) {
cancelledTransferIdsRef.current.delete(taskId);
return { success: false, cancelled: true };
}
// Real error - propagate it by re-throwing
throw error;
}This ensures genuine failures (permission denied, disk full, connection loss) are reported with proper error messages in the UI. |
Summary
rm -rfcommand for remote folder deletion instead of slow recursive SFTP rmdirChanges
Bundle folder uploads
detectRootFoldershelper to group entries by root folderFix cancel upload
fileTransferIdfor backend cancellation trackingactiveFileTransferIdsRefto track all active uploadscancelExternalUploadto cancel all active file uploadsuploadState.cancelledflag instead of just error messagecancelUploadRef.currentto break out of loopFast folder deletion
execSshCommandhelper function in sftpBridge.cjsclient.client(underlying ssh2 Client) to executerm -rfcommandTest plan
🤖 Generated with Claude Code