SVG support and replace-file workflow with enhanced file ops #11
Conversation
- Implement file replace functionality in FileBrowser component including UI and handler logic. - Add event handling and bulk replace support for selected files. - Extend FileContextMenu with replace file option. - Enhance appUtils to infer MIME types and add disposition to file access URLs. - Update worker getObject handler to support inline Content-Disposition and infer content-type from filename. - Improve FilePreview and MediaGallery components to use inline disposition for better media rendering. - Refactor FileOptions rename logic for consistent naming handling. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
WalkthroughThis PR introduces file replacement capability across the dashboard, enhances MIME type inference for content-type handling, and extends URL building to support content-disposition parameters. Changes span utility functions, UI components, and backend worker modules to enable inline/attachment disposition handling and improved metadata management throughout the file access pipeline. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant UI as FileBrowser.vue
participant Upload as apiHandler.uploadObjects
participant Cache as Browser Cache
participant Notify as Notification
participant Server as Backend
User->>UI: Click "Replace File..."
UI->>UI: handleReplace(item)
activate UI
UI->>UI: Set replaceTarget = item
UI->>UI: Trigger hidden file input
deactivate UI
User->>UI: Select replacement file
UI->>UI: handleReplaceFileChange(event)
activate UI
UI->>Notify: Show "Uploading..." progress
UI->>Upload: uploadObjects(file, key, bucket, callback, {httpMetadata, customMetadata})
deactivate UI
Upload->>Upload: Infer MIME type from filename
Upload->>Upload: Build HTTP metadata with contentType
Upload->>Server: POST with inferred content-type
Server->>Server: Store file with metadata
Server-->>Upload: Success
Upload->>Cache: Clear cache version
Upload-->>UI: Callback with result
UI->>Notify: Show "File replaced" success
UI->>UI: handleBulkRefresh()
UI->>UI: resetReplaceState()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![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)
packages/dashboard/src/components/preview/FilePreview.vue (1)
315-321: Fix preview size guard to use raw bytes instead of formatted size
bytesToMegabytes(file.size)is operating on the human‑readablesizestring frommapFile(e.g."10 MB"), so the> 200check will never behave correctly. Use the raw bytes field instead.Apply this change:
- if (bytesToMegabytes(file.size) > 200) { + if (bytesToMegabytes(file.sizeRaw ?? file.size) > 200) {Assuming all callers pass in objects from
mapFile,sizeRawwill be defined and the guard will work as intended.
🧹 Nitpick comments (2)
packages/dashboard/src/appUtils.js (1)
21-47: Optional: consider using audio/video specific type for.oggRight now
.oggis mapped to"application/ogg". If you primarily serve audio OGG,"audio/ogg"can be a closer match; for video,"video/ogg"would be more accurate. Not critical, but worth aligning with your dominant use case.packages/dashboard/src/components/files/FileOptions.vue (1)
134-163: Consider adding JSDoc comments to the helpers.While the helper methods are well-implemented, adding JSDoc comments would improve maintainability and help other developers understand their purpose and edge cases.
Example documentation:
+ /** + * Normalizes an item's name for comparison by trimming whitespace + * and removing trailing slashes from folders. + * @param {Object} item - The item (file or folder) to normalize + * @returns {string} The normalized name, or empty string if item is invalid + */ getComparableName(item) { // ... existing code }, + /** + * Extracts the parent directory path from an item's key. + * For folders, strips the trailing slash before finding the parent. + * @param {Object} item - The item whose parent key to extract + * @returns {string} Parent directory path with trailing slash, or empty string for root-level items + */ getParentKey(item) { // ... existing code },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/dashboard/src/appUtils.js(5 hunks)packages/dashboard/src/components/files/FileBrowser.vue(14 hunks)packages/dashboard/src/components/files/FileOptions.vue(5 hunks)packages/dashboard/src/components/preview/FilePreview.vue(7 hunks)packages/dashboard/src/components/preview/MediaGallery.vue(1 hunks)packages/dashboard/src/pages/files/FileContextMenu.vue(3 hunks)packages/worker/src/modules/buckets/getObject.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
packages/worker/src/modules/buckets/getObject.ts (1)
packages/dashboard/src/appUtils.js (6)
MIME_TYPE_MAP(21-47)DOUBLE_EXTENSIONS(49-51)ext(70-70)ext(85-85)ext(503-503)ext(508-508)
packages/dashboard/src/appUtils.js (2)
packages/dashboard/src/stores/main-store.js (3)
useMainStore(4-108)useMainStore(4-108)url(64-64)packages/dashboard/src/boot/axios.js (1)
api(14-14)
🔍 Remote MCP Cloudflare Workers Observability
Summary of Relevant Context Found
Based on comprehensive documentation searches, here are key technical considerations for reviewing this PR:
MIME Type and Content-Type Handling
When serving static assets, Workers automatically determines the MIME type of files based on their extension, and attaches a Content-Type header to the response if one is provided during asset upload. The PR's addition of MIME type inference from filenames aligns with this pattern.
Content-Disposition Header Best Practices
R2 supports HTTP header metadata including Content-Disposition through the httpMetadata field, which maps to HTTP headers. The PR's implementation of disposition query parameters for inline vs. attachment serving follows established web standards for file delivery.
R2 Object Storage and Metadata
When uploading objects to R2, metadata is handled through the Metadata Service which ensures strong consistency across the storage system. R2's httpMetadata field supports customMetadata (user-defined key-value pairs stored as x-amz-meta-* headers) and standard headers like Content-Type and Content-Disposition. The PR's approach of passing both customMetadata and httpMetadata in upload operations is consistent with R2's design.
SVG Handling Considerations
Cloudflare sanitizes SVG files using svg-hush to remove scripting, hyperlinks, and cross-origin resource references before serving them. SVG files are inherently scalable and do not require resizing. The PR's SVG MIME type support (image/svg+xml) enables proper content-type delivery for sanitized SVGs.
Query Parameter Patterns
The PR's pattern of appending disposition and cache-version as query parameters (v=... and disposition=...) aligns with how Wrangler allows setting Content-Type and Content-Disposition through optional flags during upload, supporting dynamic HTTP header control.
Double Extension Handling
The inclusion of double-extension support (e.g., log.gz) is relevant because Cloudflare caches SVG and other extensions by file extension, so accurate extension parsing is important for correct MIME type inference and cache behavior.
🔇 Additional comments (19)
packages/dashboard/src/components/preview/MediaGallery.vue (1)
187-193: Inline disposition in gallery URLs looks correctPassing
disposition: "inline"intobuildFileAccessUrlfor gallery media aligns with preview usage and avoids forcing download; no issues spotted with this change.packages/dashboard/src/pages/files/FileContextMenu.vue (1)
15-19: Replace + bulk semantics are consistent and well‑scopedRestricting “Replace file…” and “Rename” to non‑bulk states and defining bulk purely as
selectedRows.length > 1matches the intended UX, and the newreplaceObjectemitter is cleanly wired for parent handling.Also applies to: 74-75, 84-86
packages/dashboard/src/components/preview/FilePreview.vue (1)
200-205: Object URL lifecycle + MIME inference are implemented cleanlyThe new
objectUrlfield,cleanupObjectUrl()helper, and calls fromopenFile,close, andbeforeUnmountprovide a tight lifecycle for preview URLs and avoid leaks. UsinghttpMetadata.contentTypewith a fallback toinferMimeTypeFromFilename(this.file?.name)when constructing the Blob gives sensible content types for image/video/pdf/audio previews.Also applies to: 226-227, 293-298, 335-336, 368-380, 394-395, 592-594
packages/dashboard/src/appUtils.js (2)
21-72: MIME inference and URL query handling are coherent and backwards‑safeThe new
MIME_TYPE_MAP/DOUBLE_EXTENSIONSplusinferMimeTypeFromFilenamecover the key formats (including svg and.log.gz) and thebuildFileAccessUrlchanges cleanly centralizevanddispositionquery parameters while keeping existing call sites working (3rd arg remains optional). Usingdisposition: "inline"ingetThumbnailUrlmatches preview/gallery usage.Also applies to: 219-258, 515-522
364-402: uploadObjects options + contentType inference align with replace workflowAllowing
uploadObjectsto takeoptionswithcustomMetadata/httpMetadata, auto‑injecting a"cache-version"(when not skipped), and inferringcontentTypefromfile.type, filename, or existing metadata gives the replace/upload flows enough flexibility while retaining sane defaults.packages/dashboard/src/components/files/FileBrowser.vue (4)
181-193: Replace action is consistently exposed across grid, list, context, and mobile UIsHooking
@replaceObject="handleReplace"into allfile-context-menuinstances and adding a “Replace File…” entry in the mobile sheet keeps the replace capability discoverable in every interaction mode, while remaining file‑only.Also applies to: 305-316, 333-345, 388-411
513-519: Replace workflow correctly scopes target, preserves metadata, and respects read‑onlyThe combination of
replaceInput/replaceTarget,resetReplaceState,handleReplace, andhandleReplaceFileChange:
- Only allows replacing a single file (not folders or multi‑selection).
- Guards against read‑only mode both before opening the picker and on change.
- Reuses the existing key so URLs remain stable.
- Passes through existing
customMetadata/httpMetadatatouploadObjects, so attributes are preserved while cache‑version is refreshed.Overall the flow is robust and matches the PR goals.
Also applies to: 603-605, 673-682, 1087-1190
1192-1222: Bulk rename/replace helpers provide clear UX constraints
handleBulkRename/handleBulkReplaceenforcing a single selected item and surfacing explicit notifications when the selection is invalid avoids ambiguous behavior and keeps the bulk bar aligned with single‑item operations.
465-482: Bulk toolbar and keyboard F2 shortcut are well‑integratedThe new bulk “Rename”/“Replace” buttons, gated by
canRenameSelection/canReplaceSelection, plus the F2 keybinding for rename, give predictable affordances without overloading multi‑selection behavior. Wiring these through the existinghandleRename/handleReplacepaths keeps the implementation DRY.Also applies to: 673-682, 1368-1426, 1475-1551
packages/worker/src/modules/buckets/getObject.ts (4)
29-31: LGTM!The
DOUBLE_EXTENSIONSmapping is consistent with the dashboard version and correctly handles the common.log.gzcase.
33-48: LGTM!The content-type inference logic is correct: it checks double extensions first (e.g.,
.log.gz), then falls back to single extensions. The defensive check at line 42 ensures safe handling of filenames without extensions.
96-97: LGTM!The disposition query parameter extraction is straightforward and correctly handles the case where the parameter is not present (returns
null).
103-108: LGTM!The content-type inference correctly respects existing metadata by only applying inferred types when the header is missing. This preserves explicitly set content-types while providing sensible defaults.
packages/dashboard/src/components/files/FileOptions.vue (6)
134-143: LGTM: Clean normalization helper with proper null handling.The
getComparableNamehelper correctly normalizes item names by trimming whitespace and stripping trailing slashes from folders. The null/undefined check prevents runtime errors, and the regex pattern/\/+$/appropriately handles multiple consecutive trailing slashes.
144-163: LGTM: Parent key extraction handles edge cases correctly.The
getParentKeyhelper correctly derives parent directory paths for both files and folders. The logic properly handles root-level items (returning empty string) and normalizes folder keys before extracting the parent path.
184-190: LGTM: Consistent use of normalization in rename initialization.Using
getComparableName(row)ensures the rename input is initialized with the normalized name (without trailing slashes for folders), improving user experience.
209-223: LGTM: Correct key reconstruction for rename operation.The rename logic correctly constructs the new key by:
- Comparing normalized names to detect actual changes
- Extracting the parent key using the helper
- Appending the appropriate slash suffix for folders
This ensures renamed items maintain their position in the hierarchy.
506-525: LGTM: Consistent normalization in disabled state check.The rename button correctly uses
getComparableNameto determine if the name has actually changed, ensuring the normalized comparison matches the logic used elsewhere.
491-498: LGTM: Duplicate detection verified across the call chain.The verification confirms the implementation is sound:
fetchFile()properly handles pagination (lines 415-480 in appUtils.js) and returns the complete item listsortedItemsin FileBrowser.vue contains all sibling items in the current directory- FileBrowser.vue correctly passes
sortedItems.valuetorenameObject()as theavailableItemsparameter- The duplicate detection logic properly uses normalized names (
getComparableName), case-insensitive comparison, and excludes the item being renamedAll callers examined (FileBrowser.vue line 1080) correctly pass the complete list of sibling items.
| const MIME_TYPE_MAP: Record<string, string> = { | ||
| svg: "image/svg+xml", | ||
| png: "image/png", | ||
| jpg: "image/jpeg", | ||
| jpeg: "image/jpeg", | ||
| gif: "image/gif", | ||
| webp: "image/webp", | ||
| avif: "image/avif", | ||
| mp4: "video/mp4", | ||
| webm: "video/webm", | ||
| mov: "video/quicktime", | ||
| mp3: "audio/mpeg", | ||
| ogg: "application/ogg", | ||
| wav: "audio/wav", | ||
| json: "application/json", | ||
| pdf: "application/pdf", | ||
| csv: "text/csv", | ||
| html: "text/html", | ||
| txt: "text/plain", | ||
| md: "text/markdown", | ||
| log: "text/plain", | ||
| }; |
There was a problem hiding this comment.
Inconsistent MIME type coverage and code duplication.
The worker's MIME_TYPE_MAP is missing several entries present in the dashboard version (packages/dashboard/src/appUtils.js): bmp, flac, aac, htm, and gz. This inconsistency means files with these extensions won't receive proper content-type inference on the worker side.
Additionally, the MIME type maps are duplicated across packages. Consider extracting these constants to a shared package/module to ensure consistency and reduce maintenance burden.
Apply this diff to add the missing MIME types:
const MIME_TYPE_MAP: Record<string, string> = {
svg: "image/svg+xml",
png: "image/png",
jpg: "image/jpeg",
jpeg: "image/jpeg",
gif: "image/gif",
+ bmp: "image/bmp",
webp: "image/webp",
avif: "image/avif",
mp4: "video/mp4",
webm: "video/webm",
mov: "video/quicktime",
mp3: "audio/mpeg",
ogg: "application/ogg",
wav: "audio/wav",
+ flac: "audio/flac",
+ aac: "audio/aac",
json: "application/json",
pdf: "application/pdf",
csv: "text/csv",
html: "text/html",
+ htm: "text/html",
txt: "text/plain",
md: "text/markdown",
log: "text/plain",
+ gz: "application/gzip",
};📝 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.
| const MIME_TYPE_MAP: Record<string, string> = { | |
| svg: "image/svg+xml", | |
| png: "image/png", | |
| jpg: "image/jpeg", | |
| jpeg: "image/jpeg", | |
| gif: "image/gif", | |
| webp: "image/webp", | |
| avif: "image/avif", | |
| mp4: "video/mp4", | |
| webm: "video/webm", | |
| mov: "video/quicktime", | |
| mp3: "audio/mpeg", | |
| ogg: "application/ogg", | |
| wav: "audio/wav", | |
| json: "application/json", | |
| pdf: "application/pdf", | |
| csv: "text/csv", | |
| html: "text/html", | |
| txt: "text/plain", | |
| md: "text/markdown", | |
| log: "text/plain", | |
| }; | |
| const MIME_TYPE_MAP: Record<string, string> = { | |
| svg: "image/svg+xml", | |
| png: "image/png", | |
| jpg: "image/jpeg", | |
| jpeg: "image/jpeg", | |
| gif: "image/gif", | |
| bmp: "image/bmp", | |
| webp: "image/webp", | |
| avif: "image/avif", | |
| mp4: "video/mp4", | |
| webm: "video/webm", | |
| mov: "video/quicktime", | |
| mp3: "audio/mpeg", | |
| ogg: "application/ogg", | |
| wav: "audio/wav", | |
| flac: "audio/flac", | |
| aac: "audio/aac", | |
| json: "application/json", | |
| pdf: "application/pdf", | |
| csv: "text/csv", | |
| html: "text/html", | |
| htm: "text/html", | |
| txt: "text/plain", | |
| md: "text/markdown", | |
| log: "text/plain", | |
| gz: "application/gzip", | |
| }; |
| const filename = filePath.split("/").pop() || "file"; | ||
| const safeFilename = filename.replace(/"/g, '\\"') || "file"; | ||
| const disposition = | ||
| requestedDisposition === "inline" ? "inline" : "attachment"; | ||
| headers.set( | ||
| "Content-Disposition", | ||
| `attachment; filename="${filePath.split("/").pop()}"`, | ||
| `${disposition}; filename="${safeFilename}"`, | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
Verify filename encoding compliance with RFC 6266.
The current approach escapes only double quotes in filenames. RFC 6266 specifies that the Content-Disposition header should use percent-encoding (via the filename* parameter) for filenames containing non-ASCII or special characters beyond basic escaping.
While the current implementation handles common cases, please verify whether more robust encoding is needed for:
- Non-ASCII characters (e.g., Unicode filenames)
- Special characters beyond double quotes (e.g., semicolons, backslashes)
- Very long filenames
🌐 Web query:
RFC 6266 Content-Disposition filename encoding requirements for special characters
💡 Result:
Summary — RFC 6266 requirements for encoding filenames with special characters
- Use filename* (RFC 5987 encoding) when the desired name contains characters not representable in ISO‑8859‑1 (i.e., non‑ASCII / international characters). (datatracker.ietf.org)
- When sending filename*, also send a fallback filename (plain filename parameter) for legacy UAs; place filename first in the header. Recipients SHOULD prefer filename* when they understand it. (datatracker.ietf.org)
- filename (non‑star) is limited to ISO‑8859‑1; use token form only if it contains no disallowed characters (use quoted-string otherwise). Avoid non‑ASCII in filename. (datatracker.ietf.org)
- The RFC5987 form (filename*=charset'lang'%xx...) percent‑encodes octets (UTF‑8 is recommended); use UTF‑8 for filename*. (rfc-editor.org)
- Practical generation advice / pitfalls from RFC 6266 Appendix D:
- Avoid embedding literal percent‑hex sequences (e.g. %A9) in filename because some implementations treat them as escapes.
- Avoid backslash "" in quoted-string (some UAs don’t implement escaping).
- If possible, provide an ASCII-friendly fallback (e.g., transliteration) for the filename parameter. (httpwg.org)
If you want, I can show exact header examples (attachment; filename="..."; filename*=UTF-8''...) and how to percent‑encode a sample UTF‑8 name.
Citations:
- 1: https://datatracker.ietf.org/doc/html/rfc6266?utm_source=openai
- 2: https://datatracker.ietf.org/doc/html/rfc6266?utm_source=openai
- 3: https://datatracker.ietf.org/doc/html/rfc6266?utm_source=openai
- 4: https://www.rfc-editor.org/rfc/rfc5987?utm_source=openai
- 5: https://httpwg.org/specs/rfc6266.html?utm_source=openai
🏁 Script executed:
#!/bin/bash
# Search for getObject usage and understand typical filename patterns
rg -n "getObject" packages/worker/src --type=ts -C 3 | head -50Length of output: 635
🏁 Script executed:
#!/bin/bash
# Check if there are any filename/key handling tests or documentation
fd -e test.ts -e spec.ts -e docs.md | xargs rg -l "getObject|filename|Content-Disposition" 2>/dev/null | head -10Length of output: 96
🏁 Script executed:
#!/bin/bash
# Search for any existing filename encoding or sanitization utilities
rg -n "filename|Content-Disposition|RFC|6266|filename\*" packages/worker/src --type=ts -C 2 | head -40Length of output: 1310
🏁 Script executed:
#!/bin/bash
# Check getObject.ts for additional context about the function and its purpose
cat packages/worker/src/modules/buckets/getObject.ts | head -150Length of output: 2927
🏁 Script executed:
#!/bin/bash
# Check test patterns for getObject to understand expected usage
cat packages/worker/tests/integration/object.test.tsLength of output: 13418
🏁 Script executed:
#!/bin/bash
# Search for any documentation or comments about filename handling
rg -n "filePath|filename|disposition" packages/worker/src/modules/buckets/getObject.tsLength of output: 508
🏁 Script executed:
#!/bin/bash
# Check if there are any utility functions for encoding/sanitizing filenames
fd -type f -name "*.ts" packages/worker/src | xargs rg -l "encode|sanitize" | head -5Length of output: 387
Fix Content-Disposition header to comply with RFC 6266.
The current implementation has two RFC 6266 compliance issues:
-
Backslash escaping is non-compliant: RFC 6266 Appendix D explicitly warns that "backslash '' in quoted-string" should be avoided as some user agents don't implement escaping correctly. The
replace(/"/g, '\\"')on line 111 violates this. -
Missing filename parameter for non-ASCII*: RFC 6266 requires the
filename*parameter (RFC 5987 encoding) for filenames with non-ASCII characters. Without it, international filenames won't display correctly. This is relevant because email attachments (which feed into this system via receiveEmail.ts) can contain non-ASCII filenames.
Required changes:
- Implement RFC 5987 percent-encoding for the
filename*parameter - Provide an ASCII fallback in the
filenameparameter (e.g., via transliteration) - Use proper quoted-string escaping or token form (avoid backslash escaping per RFC 6266 Appendix D)
- Update tests to cover non-ASCII filename handling
|
@claude resolve these (minor) errors |
zudsniper
deleted the
terragon/support-svg-previews-and-file-ops-3k9hh1
branch
Summary
Adds SVG MIME support across the app, enhances file operations including a new replace-file workflow, and refines how files are previewed, downloaded, and renamed. Also improves server-side MIME inference and URL handling to support inline dispositions and cache busting.
Changes
Core MIME & URL handling
Backend / Worker updates
Upload API improvements
UI: File Context & Browse enhancements
Preview & Gallery improvements
Context menus & bulk ops
Testing plan
SVG handling
Replace-file workflow
Rename & duplicates
Preview/Download behavior
API & URL behavior
Migration / Compatibility
Notes
🌿 Generated by Terry
ℹ️ Tag @terragon-labs to ask questions and address PR feedback
📎 Task: https://www.terragonlabs.com/task/d3197c9f-aae9-47f6-9e2a-5036ed7067c4
Summary by CodeRabbit
New Features
Bug Fixes