Uh oh!
There was an error while loading. Please reload this page.
feat(file-manager):folder downloads - #753
Conversation
📝 WalkthroughWalkthroughThis change expands selection from files to items (files + folders), adds recursive folder traversal to collect nested files, updates the download/batch ZIP flow to handle mixed selections, and introduces filename/path sanitization and collision-resolution for ZIP entries. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI as "Client UI\n(renders items, triggers download)"
participant Store as "FolderStore\n(getFolderContents)"
participant Service as "FileService\n(fetch file blobs)"
participant Zip as "ZipBuilder\n(sanitize & pack)"
UI->>Store: request folder contents (for each selected folder)
Store-->>UI: return file + nested folder list (with paths)
UI->>Service: request file blob (for each file in allFilesToDownload)
Service-->>UI: return file blob
UI->>Zip: add file blob with sanitized unique path
Zip-->>UI: update progress / report errors
UI->>UI: finalize & trigger ZIP download
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
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)
1537-1556:⚠️ Potential issue | 🟡 MinorSelection info bar text should say "items" instead of "files".
The selection count displays "X files selected" even when folders are included in the selection. For consistency with the new folder selection feature, consider updating the text.
✏️ Proposed fix
<span class="text-sm text-blue-700"> {selectedFileIds.size} - {selectedFileIds.size === 1 ? "file" : "files"} selected+ {selectedFileIds.size === 1 ? "item" : "items"} selected </span>
🤖 Fix all issues with AI agents
In `@platforms/file-manager/src/routes/`(protected)/files/+page.svelte:
- Around line 969-1003: The recursive getAllFilesFromFolder has no depth limit
and silently swallows errors; modify getAllFilesFromFolder to accept an optional
depth and visited set (e.g., add parameters maxDepth:number = 100,
currentDepth:number = 0, visited:Set<string> = new Set()), check currentDepth
against maxDepth and return or throw a clear error when exceeded, skip recursion
into subfolders already present in visited to avoid cycles (add subfolder.id to
visited before recursing), and propagate errors instead of only console.error so
callers can show a user-facing failure (rethrow or return a rejected Promise
with context that includes folderId/folderName and depth).
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Actionable comments posted: 0
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)
1600-1618:⚠️ Potential issue | 🟡 MinorUI text says "files" but selection can include folders.
Since the selection now supports both files and folders, the text should say "items" for accuracy. Currently, selecting 2 folders would display "2 files selected."
✏️ Suggested fix
<span class="text-sm text-blue-700"> {selectedFileIds.size} - {selectedFileIds.size === 1 ? "file" : "files"} selected+ {selectedFileIds.size === 1 ? "item" : "items"} selected </span>
🧹 Nitpick comments (1)
platforms/file-manager/src/routes/(protected)/files/+page.svelte (1)
1305-1378: Consider extracting sanitization utilities to module scope.The
sanitizeFilename,sanitizePath, andgetUniqueFilePathfunctions are recreated on every download invocation. Moving them outsidedownloadSelectedFileswould avoid this overhead and improve readability.Additionally,
getUniqueFilePathuses case-sensitive comparison for collision detection. On case-insensitive filesystems (Windows, macOS), paths likeReport.pdfandreport.pdfwould be treated as distinct but collide when extracting.🔧 Suggested extraction and case-insensitive fix
+// Move outside downloadSelectedFiles, at module scope:+function sanitizeFilename(rawName: string): string {+ let name = rawName;+ name = name.replace(/^[a-zA-Z]:/, '');+ name = name.replace(/\\/g, '/');+ name = name.replace(/\.\./g, '');+ const lastSlashIndex = name.lastIndexOf('/');+ if (lastSlashIndex !== -1) {+ name = name.slice(lastSlashIndex + 1);+ }+ name = name.replace(/[/\\]/g, '');+ name = name.replace(/^[.\s]+/, '');+ name = name.trim();+ // biome-ignore lint/suspicious/noControlCharactersInRegex: Intentional removal of control chars+ name = name.replace(/[\x00-\x1f\x7f]/g, '');+ if (!name) {+ name = 'file';+ }+ return name;+}++function sanitizePath(rawPath: string): string {+ if (!rawPath) return "";+ const parts = rawPath.split('/').filter(Boolean);+ const sanitizedParts = parts.map(part => {+ let p = part;+ p = p.replace(/\.\./g, '');+ p = p.replace(/[\\:*?"<>|]/g, '');+ p = p.replace(/^[.\s]+/, '');+ p = p.trim();+ return p || 'folder';+ });+ return sanitizedParts.join('/');+}For case-insensitive collision detection:
-const usedPaths = new Set<string>();+const usedPaths = new Set<string>();+const usedPathsLower = new Set<string>(); // For case-insensitive check function getUniqueFilePath(originalPath: string, originalName: string): string { const sanitizedPath = sanitizePath(originalPath); const sanitizedName = sanitizeFilename(originalName); const fullPath = sanitizedPath ? `${sanitizedPath}/${sanitizedName}` : sanitizedName; + const fullPathLower = fullPath.toLowerCase();- if (!usedPaths.has(fullPath)) {+ if (!usedPathsLower.has(fullPathLower)) { usedPaths.add(fullPath); + usedPathsLower.add(fullPathLower); return fullPath; } // ... rest of collision handling with similar changes
Description of change
adds folder download functionality to file manager
Issue Number
n/a
Type of change
How the change has been tested
untested
Change checklist
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.