Repository navigation
fix(sftp): reject archive-window folder drags with a clear extract-first error - #3687
netcatty-bot wants to merge 2 commits into
Conversation
netcatty-bot
added
automation:bot-pr
|
@codex review |
Codex Review SummaryThis 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: f6afecbc0a
ℹ️ 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".
| const size = entry.file?.size ?? entry.size; | ||
| return size === undefined || size === 0; |
There was a problem hiding this comment.
A legitimate zero-byte, extensionless virtual file (for example an empty README dragged from an archive, email client, or another shell data source without a disk path) has exactly the same shape as this heuristic and is therefore rejected as a folder. Since file size and the absence of a dot do not identify a directory, this prevents supported empty files from being uploaded; the rejection needs an actual folder/source signal or must otherwise avoid classifying ambiguous zero-byte files as directories.
Useful? React with 👍 / 👎.
| const rawError = isSftpVirtualFolderUploadError(item.error) | ||
| ? t("sftp.upload.virtualFolderError") | ||
| : item.error; |
There was a problem hiding this comment.
This translation only covers callers of reportSftpUploadResults, but the native SFTP paste flow calls uploadExternalEntries and reports failures through showUploadResults in useSftpKeyboardShortcuts.ts, where failed.error is concatenated directly. If a path-less archive item arrives through that supported clipboard path, users see the raw netcatty:virtual-folder-upload marker rather than the extract-first message; either all consumers need the mapping or the upload layer should return a display-safe error.
Useful? React with 👍 / 👎.
|
@codex review |
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
netcatty-bot
added
automation:codex-clean
|
Codex reported no major issues. This PR is marked ready for human review/merge. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 94fa2fe3a6
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const withVirtualFolderResults = (results: UploadResult[]): UploadResult[] => [ | ||
| ...virtualFolderResults, | ||
| ...results, | ||
| ]; |
There was a problem hiding this comment.
When a mixed drop contains a rejected archive folder and another entry whose conflict is skipped or stopped, this helper returns both the sentinel failure and a cancelled result. Both reportSftpUploadResults and showUploadResults immediately return when any result is cancelled, so the new extract-first error is never shown in that scenario; process the non-cancelled failures before the cancellation shortcut, or avoid combining these result types.
Useful? React with 👍 / 👎.
binaricat
left a comment
binaricat
left a comment
There was a problem hiding this comment.
Audit of 94fa2fe3a6c4e618e1bf65b357788340344e9bb7: changes requested. Kept this PR as a draft and replaced the automatic issue-closing statement with a related-issue reference.
P2 — preserve readable empty files (domain/sftpVirtualDrop.ts:43-48, lib/uploadService.ts:272-278). I compared the unmodified production uploadFromDataTransfer path against its merge base using real filesystem staging and destination writes. new File([], 'README') uploads a zero-byte file at the base; this PR returns netcatty:virtual-folder-upload before any staging. An explicit file entry (isFile: true, isDirectory: false) is also rejected. A path-backed empty file and a pathless file with content both still upload. The same filter covers local destinations too. Neither zero length nor absence of an extension is directory evidence. Please remove the ambiguous rejection or use a demonstrated source/directory signal, and preserve supported empty-file upload.
P2 — mixed skipped items hide the failure (lib/uploadService.ts:288-290). A rejected root plus notes.txt whose conflict is skipped returns both the sentinel failure and {cancelled:true}. Passing that actual result to reportSftpUploadResults yields only sftp.upload.cancelled; the promised actionable error is hidden. The paste reporter has the same early return. This confirms the current-head remote finding; it is not resolved merely because the CI is green.
Original-report coverage remains unproven. A controlled file with content and path: 'web/same.txt' bypasses localPath at domain/sftpVirtualDrop.ts:38, reaches the filesystem read and still returns ENOENT at both base and PR head. Yesterday's actual Electron/UI controls exhibited this relative-path failure; they were not native WinRAR metadata. The reporter's video covers both archive folder and TXT failures, so a guessed empty-folder rejection cannot establish full coverage.
The PR's former statement that WinRAR/7-Zip always supply only FILEDESCRIPTOR/FILECONTENTS, rendering a pathless zero-size extensionless pseudo-file, is not established by the sources. Electron's API also returns an empty path for JavaScript-created Files; Windows documents multiple shell formats and valid zero-length FILEDESCRIPTOR files. Chromium current Windows source separately handles virtual names and materialized temporary files. None determines WinRAR 5.40's actual metadata in this report.
49 focused existing tests pass; two independent full-diff reviews confirm the empty-file regression. No further guessed classifier or drag/drop framework was added. Native WinRAR metadata is still the least missing fact; the existing diagnostic/evidence comment already requests precisely that, and the reporter need not resend versions, ZIP or video. Please leave #3057 open and this proposal draft until the native data distinguishes the actual failing path and a bounded fix can be verified.
Summary
Draft proposal to reject standalone, pathless, zero-byte, extensionless upload entries and show an extract-first message. Not ready to merge: this condition also matches legitimate empty files, and the actual WinRAR 5.40 drop metadata in #3057 is still unconfirmed.
Type of Change
Related Issue (optional)
Related to #3057. This proposal does not establish that the reported folder and TXT failures are fixed; the issue must remain open.
Changes Made
Known blockers / evidence (2026-10-16)
At head
94fa2fe3a6c4e618e1bf65b357788340344e9bb7, a comparison through the unmodified production capture/upload functions confirms:READMEwith no disk path uploads at the merge base, but this PR rejects it as a folder. The same happens when the drag item explicitly suppliesisFile: true. A path-backed empty file and a readable pathless file with content still upload. File size and name do not establish directory identity.web/same.txtbypasses the filter, reaches the local filesystem read, and still fails with ENOENT. This is a controlled comparison matching the earlier relative-path failure, not proof of the native WinRAR payload.The reporter already supplied Netcatty 1.1.83, WinRAR 5.40 and a video covering archive folders and TXT files. No repeat request for those materials is needed. The remaining discriminating fact is the actual Windows drag metadata, as described in the existing issue evidence comment.
The former assertion that WinRAR/7-Zip archive drags always expose only FILEDESCRIPTOR/FILECONTENTS as a pathless, empty, extensionless File was unsupported. Electron documents empty paths for JavaScript-created files, and its implementation returns the underlying blob path. Windows supports several shell transfer formats, and explicitly permits zero-length files in FILEDESCRIPTOR. These sources do not identify what WinRAR 5.40 supplied in this report. Chromium's current Windows implementation has separate virtual-file and temporary-file handling; its current source is not version-specific acceptance for Netcatty 1.1.83.
Screenshots / Demo
No native Windows WinRAR acceptance completed. Prior full-app controls validated ordinary extracted-folder Merge and readable pathless-file upload; those controls do not prove this proposed filter fixes archive drag/drop.
Testing
Current remote CI passes, but it does not cover the reproduced regressions. The current-head remote review has active findings; its earlier “no major issues” summary does not override them. No source changes or new review requests were made during the disposition audit.
Checklist
Keep this draft pending evidence. Do not merge on the present diagnosis. Issue #3057 must remain open.