Repository navigation
feat(sftp): stream packed downloads and restore folders locally - #4179
jinshenganyuci wants to merge 2 commits into
Conversation
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d8717fe07c
ℹ️ 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".
| transferRuntime.enqueue([{ ...task, ownerId, packedTargetPaths: targetPaths, | ||
| controlKind: "packed-download", progressMode: "bytes", resumable: false, retryable: false, |
There was a problem hiding this comment.
Although resumable is false, useAppStartupEffects calls pause() for every unfinished row during close/quit. If the packed stream finishes before the matching Resume completes, the store's pause latch causes patchTask to discard the terminal state; after runWalk unregisters, Resume can take the dedicated directory-resume path. For a multi-selection, that path recursively downloads the saved common remoteDirectory, including unselected siblings, into localDirectory. I reproduced this pause → packed completion → dedicated resume sequence with the existing store. Exclude packed rows from automatic pause/resume rather than relying only on the UI capability flags.
Useful? React with 👍 / 👎.
| temp = await fsp.mkdtemp(path.join(destination, ".netcatty-download-")); | ||
| const archive = path.join(temp, "archive"), stage = path.join(temp, "contents"); | ||
| await fsp.mkdir(stage, { mode: 0o700 }); |
There was a problem hiding this comment.
The root AGENTS.md explicitly requires temporary files to use tempDirBridge.getTempFilePath(), but this allocates both the received archive and extracted tree in an unmanaged directory under the chosen destination. If the worker/app is terminated before finally runs, those potentially large duplicate contents remain outside the configured temp directory; getTempDirInfo() and clearTempDir() cannot list or clear them in Settings > System. Keep these scratch artifacts within the managed-temp facility instead of bypassing its cleanup and visibility contract.
AGENTS.md reference: AGENTS.md:L151-L151
Useful? React with 👍 / 👎.
| let temp; | ||
| try { | ||
| const destination = await fsp.realpath(localDirectory); | ||
| for (const name of names) if (await statIfPresent(path.join(destination, name))) return conflict(); |
There was a problem hiding this comment.
On an unsupported client or connection, such as Windows or a non-UTF-8 session, an existing selected basename returns conflict here before compatibility is checked below. The UI handles that result by asking for another directory and retrying packed download, so it never reaches ordinary SFTP fallback for the chosen target. Existing-directory downloads therefore require disabling the setting or choosing another destination, even though these environments should use the ordinary download workflow. I confirmed that an unsupported non-UTF-8 request returns conflict when its local root already exists. Check compatibility before applying the packed-only destination policy.
Useful? React with 👍 / 👎.
| const report = (phase, bytes) => { | ||
| safeSend(event?.sender, "netcatty:sftp:packedProgress", { transferId, phase, bytes }); |
There was a problem hiding this comment.
When a packed folder download originates in a detached session window, its task is enqueued only in that renderer's sftpTransferCenterStore, and these progress messages target only its sender. Other windows populate their centers through onGlobalSftpTransferEvent in useAppStartupEffects, but the worker fan-out does not translate packedProgress, and this path emits no shared queued or terminal events. Consequently the main window cannot display or control folder downloads started in the detached window, unlike ordinary downloads. Publish the packed lifecycle through the shared transfer-event path.
Useful? React with 👍 / 👎.
Summary
Downloading a remote folder currently transfers its files individually. Add default-on packed downloads for folders and multi-selections: the server streams a tar archive over SSH, and Netcatty validates it and restores the selected files and folders in the chosen local destination. For example, downloading
composeinto Downloads producesDownloads/compose, not an archive to unpack manually. No archive file is created on the server.Type of Change
Related Issue (optional)
Related to #3912 and follows #3936. This adds the download workflow; the existing Compress... action continues to create an archive on the server.
Changes Made
Screenshots / Demo
Verified with the actual Electron renderer/preload/IPC against an isolated loopback SSH/SFTP fixture:
composewith a hidden.env, empty directory and a Chinese/quote-containing binary filename; verify the exact restored layout and bytes.Testing
Validation applies to
d8717fe07c28b9fe79b9f4b267705f966f8db894.npm run lint,npm run buildandgit diff --checkpassed.npm testwas run: 16,083 tests, 16,026 passed, 6 failed, 51 skipped. Four failures were missing local plugin example build artifacts; after building the three examples, all four affected tests passed. The two remaining Bash prompt/readiness tests reproduce on untouched upstream main4a55492with this Mac's Bash 3.2; their sources are unchanged by this PR.Checklist