Repository navigation
Add remote File Explorer downloads - #3925
Closed
austinywang wants to merge 28 commits into
Closed
austinywang wants to merge 28 commits into
austinywang wants to merge 28 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
…emote-files-file-explorer
📝 WalkthroughWalkthroughThis PR implements a "Download to Local…" feature for SSH-backed file browsing, enabling remote-to-local file transfers via SCP. Changes include localization strings, SCP backend API with refactored process management, download directory configuration wiring, FileExplorerView UI flows with notifications, and comprehensive test coverage. ChangesRemote File Download Feature
Sequence DiagramsequenceDiagram
participant User
participant FileExplorerView
participant SSHFileExplorerProvider
participant ProcessSSHFileExplorerTransport
participant SCP as SCP Process
User->>FileExplorerView: Right-click remote file
FileExplorerView->>FileExplorerView: Show context menu
User->>FileExplorerView: Select "Download to Local…"
FileExplorerView->>FileExplorerView: Open destination picker
User->>FileExplorerView: Choose local directory
FileExplorerView->>SSHFileExplorerProvider: download(remotePath, isDirectory, toLocalDirectory)
SSHFileExplorerProvider->>ProcessSSHFileExplorerTransport: download(remotePath, isDirectory, connection, toLocalDirectory)
ProcessSSHFileExplorerTransport->>ProcessSSHFileExplorerTransport: Validate local destination
ProcessSSHFileExplorerTransport->>ProcessSSHFileExplorerTransport: Build SCP command args
ProcessSSHFileExplorerTransport->>SCP: Run scp with timeout/cancellation
SCP-->>ProcessSSHFileExplorerTransport: Success or error
ProcessSSHFileExplorerTransport-->>SSHFileExplorerProvider: Local path or throw error
SSHFileExplorerProvider-->>FileExplorerView: Return result
FileExplorerView->>FileExplorerView: Show completion/failure notification or alert
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (4 errors, 1 warning)
✅ Passed checks (10 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Contributor
Greptile SummaryThis PR adds a "Download to Local…" context menu action to the SSH-backed File Explorer, using
Confidence Score: 5/5Safe to merge. Every issue raised in prior review rounds has been corrected, and the new code is well-structured throughout. All previously flagged issues are resolved: async subprocess management now uses TerminationWaiter continuations and a task-group timeout race instead of semaphores and waitUntilExit; pipe reads are concurrent with process execution; the SIGTERM→SIGKILL grace correctly races the termination signal; the active download Task is stored and cancelled on new download or deinit; actor isolation on the download lifecycle path is statically enforced via @mainactor annotations; error messages use localized exit-status strings rather than raw stderr; scp launch failures are caught and rethrown as a user-friendly FileExplorerError; the remote path is shell-quoted and legacy SCP mode (-O) is enabled; and all new user-facing strings carry complete translations across all 19 supported locales. No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant Coordinator as FileExplorerView.Coordinator (@MainActor)
participant Store as FileExplorerStore
participant Provider as SSHFileExplorerProvider
participant Transport as ProcessSSHFileExplorerTransport (nonisolated)
participant SCP as /usr/bin/scp
User->>Coordinator: Right-click → "Download to Local…"
Coordinator->>User: NSOpenPanel (sheet/modal)
User->>Coordinator: Choose destination directory (URL)
Coordinator->>Store: setDefaultLocalDownloadDirectory(localDirectory)
Coordinator->>Coordinator: cancel activeDownloadTask (if any)
Coordinator->>Coordinator: "activeDownloadTask = Task @MainActor"
Coordinator->>Provider: await provider.download(...)
Provider->>Transport: await transport.download(...)
Transport->>Transport: downloadTarget() — validate dest, check collision
Transport->>SCP: CommandProcess.run(timeout: 1800s)
Note over Transport,SCP: OutputCollector readability handlers + TerminationWaiter
SCP-->>Transport: terminationHandler → TerminationWaiter.finish(status:)
Transport->>Transport: outputCollector.finish()
alt "terminationStatus == 0"
Transport-->>Coordinator: localPath
Coordinator->>User: Notification or NSAlert Download Complete
else "terminationStatus != 0"
Transport-->>Coordinator: throw FileExplorerError.downloadFailed
Coordinator->>User: Notification or NSAlert Download Failed
else timeout 30 min
Transport->>SCP: SIGTERM then SIGKILL grace race
Transport-->>Coordinator: throw FileExplorerError.downloadTimedOut
Coordinator->>User: Download timed out
else Task cancelled
Coordinator->>Transport: terminate() via withTaskCancellationHandler
Transport->>SCP: SIGTERM + detached SIGKILL grace
Coordinator->>Coordinator: cancelDownloadTask suppress UI
end
Reviews (18): Last reviewed commit: "fix: localize remote ssh error strings" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/FileExplorerStore.swift (1)
427-1017: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftExtract the SCP/process transport out of
FileExplorerStore.swift.This file is already 1.7k+ lines, and this change adds another large responsibility: subprocess lifecycle, SSH/SCP argument building, timeout handling, output draining, and kill semantics. Please move
ProcessSSHFileExplorerTransport/CommandProcessinto dedicated source file(s) soFileExplorerStorestays focused on explorer state. As per coding guidelines: “do not accept more than 250 lines added to an existing production Swift file that is already over 800 lines” and “Flag Swift files that mix UI rendering, state ownership, persistence, networking, parsing, subprocess/socket protocol, and platform bridge code in one place.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/FileExplorerStore.swift` around lines 427 - 1017, The file now contains a large subprocess/SSH/SCP transport implementation (ProcessSSHFileExplorerTransport and nested CommandProcess/OutputCollector/TerminationWaiter) that should be extracted into one or more dedicated source files; move ProcessSSHFileExplorerTransport and its helper types (CommandProcess, OutputCollector, TerminationWaiter, SSHCommandResult) into a new file (or files) and keep only the public API surface used by FileExplorerStore (resolveHomePath, listDirectory, download, and scpDownloadArgumentsForTesting) exported with appropriate access control; update references in FileExplorerStore to import/instantiate ProcessSSHFileExplorerTransport as before, ensure all helper methods used externally (scpDownloadArgumentsForTesting, shouldBracketIPv6Literal, scpRemoteDestination, optionKey, normalizedSSHOptions, backgroundSSHOptions, hasSSHOptionKey, bestErrorLine) have the correct access level or are moved alongside the transport, preserve existing semantics (timeouts, termination/force-kill behavior, error types), add any required imports, and run the build to adjust visibility (fileprivate -> internal/public) and tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Sources/FileExplorerStore.swift`:
- Around line 862-878: The code builds targetPath but never checks for an
existing local file/dir, risking silent overwrite; before calling runSCPCommand
(the block using scpDownloadArguments and returning targetPath), stat the
filesystem for targetPath and if it exists either throw
FileExplorerError.downloadFailed with a clear message or generate a
non-conflicting target (e.g., append a numeric suffix) and use that new path;
ensure the existence check covers both files and directories and update the
thrown error detail to include the existing path so callers can handle/confirm
before proceeding.
In `@Sources/FileExplorerView.swift`:
- Around line 637-643: The localized message uses String(format:) with a
localized template containing "%@", which is not locale-aware; update the
assignment to build panel.message using String.localizedStringWithFormat so
positional arguments are formatted per locale (i.e., replace the String(format:
String(localized:..., defaultValue:...), node.name) usage in the panel.message
assignment with a call to String.localizedStringWithFormat using the same
localized template and node.name); apply the same change to the other occurrence
that formats with node.name.
---
Outside diff comments:
In `@Sources/FileExplorerStore.swift`:
- Around line 427-1017: The file now contains a large subprocess/SSH/SCP
transport implementation (ProcessSSHFileExplorerTransport and nested
CommandProcess/OutputCollector/TerminationWaiter) that should be extracted into
one or more dedicated source files; move ProcessSSHFileExplorerTransport and its
helper types (CommandProcess, OutputCollector, TerminationWaiter,
SSHCommandResult) into a new file (or files) and keep only the public API
surface used by FileExplorerStore (resolveHomePath, listDirectory, download, and
scpDownloadArgumentsForTesting) exported with appropriate access control; update
references in FileExplorerStore to import/instantiate
ProcessSSHFileExplorerTransport as before, ensure all helper methods used
externally (scpDownloadArgumentsForTesting, shouldBracketIPv6Literal,
scpRemoteDestination, optionKey, normalizedSSHOptions, backgroundSSHOptions,
hasSSHOptionKey, bestErrorLine) have the correct access level or are moved
alongside the transport, preserve existing semantics (timeouts,
termination/force-kill behavior, error types), add any required imports, and run
the build to adjust visibility (fileprivate -> internal/public) and tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 21e3361d-bfdd-4164-b5e0-beb6a84d52a5
📒 Files selected for processing (7)
Resources/Localizable.xcstringsSources/ContentView.swiftSources/FileExplorerStore.swiftSources/FileExplorerView.swiftSources/RightSidebarPanelView.swiftSources/Workspace.swiftcmuxTests/FileExplorerStoreTests.swift
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit dfe7538. Configure here.
…emote-files-file-explorer
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 9 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Sources/SSHCommandArgumentSupport.swift">
<violation number="1" location="Sources/SSHCommandArgumentSupport.swift:43">
P2: Parse SSH option values using the first key/value separator (whitespace or `=`), not the first `=` anywhere in the string. The current logic misparses space-separated options whose values contain `=` (for example `ControlPath /tmp/path=foo`), which can break SSH option matching.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
1 issue found across 20 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Sources/FileExplorerView.swift">
<violation number="1" location="Sources/FileExplorerView.swift:837">
P2: Update the default download directory after a successful download so the next picker opens in the user’s last chosen destination.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub. |
lawrencecchen
added
the
stale-revisit
This branch was successfully deployed
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Summary
Verification
Closes #3637
Note
Medium Risk
Adds new SSH download flow that spawns and manages
scpprocesses (timeouts, cancellation, path/option handling), which could impact remote workflow reliability and error handling.Overview
Adds a remote-only File Explorer context menu action,
Download to Local…, that prompts for a destination folder and downloads the selected SSH-backed file or directory to disk viascp.This extends the SSH file explorer transport with
download(...), implements a newProcessSSHFileExplorerTransportthat runsscpwith quoting/IPv6 destination handling, background-safe SSH option filtering, destination validation, and timeout/cancellation behavior, and wires per-workspace notifications/alerts for success/failure.Also centralizes shell single-quoting and SSH option parsing in new helpers (
ShellArgumentQuoting,SSHCommandArgumentSupport), tracks a default download directory (LocalDirectoryPathNormalization+defaultLocalDownloadDirectorypropagation), adds full localization strings for the new UI/errors, and expands tests around download arguments/errors and SSH option parsing edge cases.Reviewed by Cursor Bugbot for commit 66b7d50. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Adds a remote‑only “Download to Local…” action in File Explorer to save SSH‑backed files or folders to a chosen local folder via
scp, with safe SSH defaults, strong quoting, a 30‑minute timeout, and per‑workspace notifications. Fully localizes the flow and SSH error strings across app languages, implementing #3637.New Features
scpdownload supports files/directories, IPv6, strict host‑key defaults, background‑safe option filtering, and robust path quoting. Validates the destination and shows localized completion/errors.Bug Fixes
ProcessSSHFileExplorerTransportand shared helpers (ShellArgumentQuoting,SSHCommandArgumentSupport,LocalDirectoryPathNormalization), withworkspaceIdthreading and a synced default download directory between tabs. Tests cover quoting, launch failures, andControlPathvalues containing=or spaces.Written for commit 66b7d50. Summary will update on new commits. Review in cubic
Summary by CodeRabbit
New Features
Localization