fix: Use dynamic filename for "Download image" context menu#8408
fix: Use dynamic filename for "Download image" context menu#8408mscolnick merged 2 commits intomarimo-team:mainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Im wondering if its possible to avoid the fetch, just infer from the src string. This way we let the browser handle the download. Can also add tests for this function
claude-assisted
const IMAGE_EXT: Record<string, string> = {
png: "png",
jpg: "jpg",
jpeg: "jpg",
gif: "gif",
webp: "webp",
avif: "avif",
bmp: "bmp",
tiff: "tiff",
svg: "svg",
"svg+xml": "svg",
};
function getImageExtension(src: string): string {
const dataUriMatch = src.match(/^data:image\/([^,;]+)/);
if (dataUriMatch) {
return IMAGE_EXT[dataUriMatch[1]] ?? "png";
}
try {
const url = new URL(src, window.location.href);
const ext = url.pathname.split(".").pop()?.toLowerCase() ?? "";
return IMAGE_EXT[ext] ?? "png";
} catch {
return "png";
}
}
const ext = getImageExtension(imageRightClicked.src);f89f991 to
2c989f5
Compare
|
Thank you for your feedback. I updated the image download functionality to dynamically infer file extensions from the |
There was a problem hiding this comment.
Pull request overview
This pull request fixes an issue where the "Download image" context menu option always saved images with a hardcoded image.png filename, regardless of the actual image format. The fix introduces dynamic filename generation based on the image's MIME type or file extension.
Changes:
- Added
getImageExtension()utility function to infer file extensions from image sources (data URIs or URLs) - Updated the "Download image" handler to use dynamic extensions instead of hardcoded
.png - Added comprehensive test coverage for the new utility function
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| frontend/src/utils/filenames.ts | Added getImageExtension() function and IMAGE_EXTENSIONS mapping to extract file extensions from image sources |
| frontend/src/utils/tests/filenames.test.ts | Added comprehensive tests for getImageExtension() covering URLs, data URIs, and edge cases |
| frontend/src/components/editor/cell/cell-context-menu.tsx | Updated "Download image" handler to use dynamic extension based on image source |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| handle: () => { | ||
| if (imageRightClicked) { | ||
| const link = document.createElement("a"); | ||
| link.download = "image.png"; | ||
| link.href = imageRightClicked.src; | ||
| const ext = getImageExtension(imageRightClicked.src) || "png"; | ||
| link.download = `image.${ext}`; | ||
| link.click(); | ||
| } | ||
| }, |
There was a problem hiding this comment.
The PR description states the solution should "Fetch the image source and create a blob" and "Inspect the blob.type property" to determine the extension. However, the implementation uses URL/data URI parsing instead, without fetching.
While this is a discrepancy from the described approach, the current implementation may actually be more appropriate because:
- Fetching adds latency and memory overhead
- It could fail for CORS-restricted images
- The browser can download directly from the href without fetching
However, this approach has a limitation: URLs without file extensions (e.g., /api/image/123) will fall back to .png even if the actual content type is different. Consider whether such URLs are expected in marimo. If they are, the blob.type approach would be more reliable. If not, the current implementation is acceptable and more efficient.
| const IMAGE_EXTENSIONS: Record<string, string> = { | ||
| png: "png", | ||
| jpg: "jpg", | ||
| jpeg: "jpeg", |
There was a problem hiding this comment.
The IMAGE_EXTENSIONS mapping has separate entries for "jpg" and "jpeg", both mapping to their respective values. However, this means a MIME type of "image/jpeg" will map to extension "jpeg", while a URL with ".jpg" will map to extension "jpg". This creates inconsistency in the downloaded filenames.
Consider normalizing both "jpg" and "jpeg" to the same extension (preferably "jpg" as it's more commonly used) to ensure consistent behavior regardless of whether the extension comes from a data URI MIME type or a URL file extension.
| jpeg: "jpeg", | |
| jpeg: "jpg", |
| it("should return correct extensions for common image URLs", () => { | ||
| expect(getImageExtension("https://example.com/image.png")).toBe("png"); | ||
| expect(getImageExtension("https://example.com/image.jpeg")).toBe("jpeg"); | ||
| expect(getImageExtension("https://example.com/image.JPEG?param=1")).toBe( | ||
| "jpeg", | ||
| ); | ||
| expect(getImageExtension("https://example.com/image.gif")).toBe("gif"); | ||
| expect(getImageExtension("https://example.com/image.svg")).toBe("svg"); | ||
| expect(getImageExtension("https://example.com/image.webp")).toBe("webp"); | ||
| expect(getImageExtension("https://example.com/image.avif")).toBe("avif"); | ||
| expect(getImageExtension("https://example.com/image.bmp")).toBe("bmp"); | ||
| expect(getImageExtension("https://example.com/image.tiff")).toBe("tiff"); | ||
| }); |
There was a problem hiding this comment.
The tests are missing coverage for URLs with ".jpg" extension. While "jpeg" is tested for both URLs and data URIs, "jpg" is defined in IMAGE_EXTENSIONS but not validated in tests. Consider adding a test case for URLs with ".jpg" extension to ensure this mapping works correctly.
📝 Summary
This pull request addresses an issue where the "Download image" option in the cell output context menu would always save the file with the hardcoded name
image.png. This behavior ignored the actual MIME type of the image, causing formats like SVG, GIF, or JPEG to be saved with an incorrect.pngextension.Closes #8405
🔍 Description of Changes
The
handlefunction for the "Download image" action infrontend/src/components/editor/cell/cell-context-menu.tsxhas been updated to:blob.blob.typeproperty.downloadattribute of the anchor tag toimage.[correct_extension].📋 Checklist