Fix/file manager download button #746
Merged
Conversation
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
Contributor
📝 WalkthroughWalkthroughAdds multi-file selection and batched downloads to the file manager: checkbox selection UI, batched fetch + per-file and overall progress, JSZip-based ZIP assembly and browser-triggered ZIP download; also adds Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant FileManager as File Manager
participant JSZip
participant Browser as Browser
User->>FileManager: Select multiple files
User->>FileManager: Click "Download Selected"
FileManager->>FileManager: Open Progress Modal (preparing)
loop For each batch
FileManager->>FileManager: Start parallel fetches for batch
FileManager->>FileManager: Update per-file progress/status
end
FileManager->>JSZip: Add downloaded files to ZIP
JSZip-->>FileManager: Return ZIP blob
FileManager->>Browser: Trigger ZIP download (saveAs)
Browser->>User: Deliver ZIP file
User->>FileManager: Close modal
FileManager->>FileManager: Clear selection & cleanup
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@platforms/file-manager/src/routes/`(protected)/files/+page.svelte:
- Around line 1055-1058: The loop adding files to the JSZip instance overwrites
entries when filenames collide; update the block that iterates over
downloadedBlobs and calls zip.file(name, blob) to track used filenames (e.g., a
Set or Map) and, when a name is already present, generate a unique variant by
appending a numeric suffix like " (1)", " (2)" to the base name before calling
zip.file; ensure the uniqueness logic preserves file extensions and increments
the suffix until an unused filename is found so no downloadedBlob is lost.
- Around line 1013-1044: The current use of Promise.all on batchPromises will
abort the entire batch if any single file fails; change the logic to use
Promise.allSettled on batchPromises, then iterate the settled results to push
only fulfilled items into downloadedBlobs and mark corresponding entries in
downloadedFiles/downloadProgress as "done" for fulfilled and "error" for
rejected (update the downloadedFiles status union to include "error"). Ensure
you still update currentFile/currentFileIndex/overallProgress for each file
processed and surface/log per-file errors from the rejected results rather than
throwing to abort the whole operation.
🧹 Nitpick comments (1)
platforms/file-manager/src/routes/(protected)/files/+page.svelte (1)
1109-1112: Cancel does not abort in-flight downloads.The
cancelDownloadfunction hides the modal and resets state, but any in-flightfetchrequests continue running in the background. This could waste bandwidth and potentially cause unexpected behavior if the user starts a new download.Consider using
AbortControllerto properly cancel pending fetch requests.💡 Sketch of AbortController approach
// Add at module level let downloadAbortController: AbortController | null = null; async function downloadSelectedFiles() { // ... existing setup ... downloadAbortController = new AbortController(); // In the fetch call: const response = await fetch(url, { signal: downloadAbortController.signal }); // ... } function cancelDownload() { downloadAbortController?.abort(); downloadAbortController = null; showDownloadModal = false; resetDownloadProgress(); }
platforms/file-manager/src/routes/(protected)/files/+page.svelte
Outdated
Show resolved
Hide resolved
platforms/file-manager/src/routes/(protected)/files/+page.svelte
Outdated
Show resolved
Hide resolved
Contributor
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)
platforms/file-manager/src/routes/(protected)/files/+page.svelte (1)
1443-1516:⚠️ Potential issue | 🟡 MinorAdd accessible labels to the new checkboxes.
Without labels, screen readers won’t announce what each checkbox does. Addingaria-labelkeeps the selection UX accessible.💡 Proposed fix
<input type="checkbox" checked={allFilesSelected && selectableFiles.length > 0} disabled={selectableFiles.length === 0} onclick={toggleSelectAll} class="w-4 h-4 text-blue-600 bg-gray-100 border-gray-300 rounded focus:ring-blue-500 cursor-pointer disabled:cursor-not-allowed disabled:opacity-50" title={allFilesSelected ? "Deselect all" : "Select all files"} + aria-label="Select all files" /><input type="checkbox" checked={selectedFileIds.has( item.id, )} onclick={(e) => toggleFileSelection(item.id, e)} class="w-4 h-4 text-blue-600 bg-gray-100 border-gray-300 rounded focus:ring-blue-500 cursor-pointer" title="Select for download" + aria-label={`Select ${item.displayName || item.name} for download`} />
🤖 Fix all issues with AI agents
In `@platforms/file-manager/src/routes/`(protected)/files/+page.svelte:
- Around line 985-1004: The selection can become stale: before proceeding in the
download block (where selectableFiles, selectedFileIds and filesToDownload are
used), check if filesToDownload.length === 0 and if so call clearSelection(),
show a short info toast (e.g., "No selected files found"), and return early to
avoid the misleading "All 0 file(s) failed" flow; add this guard immediately
before the single-file and zip-download logic.
- Around line 1194-1204: The immediate call to URL.revokeObjectURL(zipUrl) after
link.click() can cancel downloads in some browsers; delay revocation and cleanup
to ensure the download starts first. Change the flow around the zipUrl/link
handling in the download block so after appending link, calling link.click(),
and removing it, you call URL.revokeObjectURL(zipUrl) inside a short setTimeout
(e.g., 100–500ms) or listen for the link’s click/navigation completion before
revoking; update the code that creates zipUrl, link, link.click(), and
URL.revokeObjectURL to use the delayed revocation approach.
platforms/file-manager/src/routes/(protected)/files/+page.svelte
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@platforms/file-manager/src/routes/`(protected)/files/+page.svelte:
- Around line 1148-1177: The ZIP entry names are used verbatim which allows path
traversal or invalid paths; before de-duplicating and adding to zip, sanitize
the incoming filename in the getUniqueFilename flow: in the getUniqueFilename
function (and/or before calling it for each entry from downloadedBlobs)
normalize the name by stripping any directory components and drive letters,
remove or replace path separators and any ".." segments, trim leading
dots/slashes, and if the result is empty produce a safe fallback name (e.g.,
"file"). Then run your uniqueness logic (usedFilenames, getUniqueFilename) and
call zip.file(uniqueName, blob) with the sanitized, validated uniqueName to
prevent embedding unintended paths.
Comment on lines
+1148
to
+1177
| // Add all successfully downloaded files to zip with unique filenames | ||
| const usedFilenames = new Set<string>(); | ||
|
|
||
| function getUniqueFilename(originalName: string): string { | ||
| if (!usedFilenames.has(originalName)) { | ||
| usedFilenames.add(originalName); | ||
| return originalName; | ||
| } | ||
|
|
||
| // Split name into base and extension | ||
| const lastDotIndex = originalName.lastIndexOf('.'); | ||
| const hasExtension = lastDotIndex > 0 && lastDotIndex < originalName.length - 1; | ||
| const baseName = hasExtension ? originalName.slice(0, lastDotIndex) : originalName; | ||
| const extension = hasExtension ? originalName.slice(lastDotIndex) : ''; | ||
|
|
||
| // Find a unique name by incrementing suffix | ||
| let counter = 1; | ||
| let uniqueName = `${baseName} (${counter})${extension}`; | ||
| while (usedFilenames.has(uniqueName)) { | ||
| counter++; | ||
| uniqueName = `${baseName} (${counter})${extension}`; | ||
| } | ||
|
|
||
| usedFilenames.add(uniqueName); | ||
| return uniqueName; | ||
| } | ||
|
|
||
| for (const { name, blob } of downloadedBlobs) { | ||
| const uniqueName = getUniqueFilename(name); | ||
| zip.file(uniqueName, blob); |
Contributor
There was a problem hiding this comment.
Sanitize ZIP entry names to prevent path traversal or invalid paths.
File names are used verbatim when creating ZIP entries. If a shared file name contains path separators or .., the resulting ZIP can embed unintended paths. Sanitize before de-duplication.
🔧 Suggested fix
// Add all successfully downloaded files to zip with unique filenames
const usedFilenames = new Set<string>();
+ function sanitizeFilename(originalName: string): string {
+ const sanitized = originalName
+ .replace(/[\\/]/g, "_")
+ .replace(/\.\.+/g, ".")
+ .trim();
+ return sanitized || "file";
+ }
+
function getUniqueFilename(originalName: string): string {
if (!usedFilenames.has(originalName)) {
usedFilenames.add(originalName);
return originalName;
@@
}
for (const { name, blob } of downloadedBlobs) {
- const uniqueName = getUniqueFilename(name);
+ const safeName = sanitizeFilename(name);
+ const uniqueName = getUniqueFilename(safeName);
zip.file(uniqueName, blob);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Add all successfully downloaded files to zip with unique filenames | |
| const usedFilenames = new Set<string>(); | |
| function getUniqueFilename(originalName: string): string { | |
| if (!usedFilenames.has(originalName)) { | |
| usedFilenames.add(originalName); | |
| return originalName; | |
| } | |
| // Split name into base and extension | |
| const lastDotIndex = originalName.lastIndexOf('.'); | |
| const hasExtension = lastDotIndex > 0 && lastDotIndex < originalName.length - 1; | |
| const baseName = hasExtension ? originalName.slice(0, lastDotIndex) : originalName; | |
| const extension = hasExtension ? originalName.slice(lastDotIndex) : ''; | |
| // Find a unique name by incrementing suffix | |
| let counter = 1; | |
| let uniqueName = `${baseName} (${counter})${extension}`; | |
| while (usedFilenames.has(uniqueName)) { | |
| counter++; | |
| uniqueName = `${baseName} (${counter})${extension}`; | |
| } | |
| usedFilenames.add(uniqueName); | |
| return uniqueName; | |
| } | |
| for (const { name, blob } of downloadedBlobs) { | |
| const uniqueName = getUniqueFilename(name); | |
| zip.file(uniqueName, blob); | |
| // Add all successfully downloaded files to zip with unique filenames | |
| const usedFilenames = new Set<string>(); | |
| function sanitizeFilename(originalName: string): string { | |
| const sanitized = originalName | |
| .replace(/[\\/]/g, "_") | |
| .replace(/\.\.+/g, ".") | |
| .trim(); | |
| return sanitized || "file"; | |
| } | |
| function getUniqueFilename(originalName: string): string { | |
| if (!usedFilenames.has(originalName)) { | |
| usedFilenames.add(originalName); | |
| return originalName; | |
| } | |
| // Split name into base and extension | |
| const lastDotIndex = originalName.lastIndexOf('.'); | |
| const hasExtension = lastDotIndex > 0 && lastDotIndex < originalName.length - 1; | |
| const baseName = hasExtension ? originalName.slice(0, lastDotIndex) : originalName; | |
| const extension = hasExtension ? originalName.slice(lastDotIndex) : ''; | |
| // Find a unique name by incrementing suffix | |
| let counter = 1; | |
| let uniqueName = `${baseName} (${counter})${extension}`; | |
| while (usedFilenames.has(uniqueName)) { | |
| counter++; | |
| uniqueName = `${baseName} (${counter})${extension}`; | |
| } | |
| usedFilenames.add(uniqueName); | |
| return uniqueName; | |
| } | |
| for (const { name, blob } of downloadedBlobs) { | |
| const safeName = sanitizeFilename(name); | |
| const uniqueName = getUniqueFilename(safeName); | |
| zip.file(uniqueName, blob); | |
| } |
🤖 Prompt for AI Agents
In `@platforms/file-manager/src/routes/`(protected)/files/+page.svelte around
lines 1148 - 1177, The ZIP entry names are used verbatim which allows path
traversal or invalid paths; before de-duplicating and adding to zip, sanitize
the incoming filename in the getUniqueFilename flow: in the getUniqueFilename
function (and/or before calling it for each entry from downloadedBlobs)
normalize the name by stripping any directory components and drive letters,
remove or replace path separators and any ".." segments, trim leading
dots/slashes, and if the result is empty produce a safe fallback name (e.g.,
"file"). Then run your uniqueness logic (usedFilenames, getUniqueFilename) and
call zip.file(uniqueName, blob) with the sanitized, validated uniqueName to
prevent embedding unintended paths.
coodos
approved these changes
Jan 30, 2026
This was referenced Jan 30, 2026
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description of change
Fixes download button on file manager and introduces client side zipped bulk downloads
Issue Number
n/a
Type of change
How the change has been tested
manual
Change checklist
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.