Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds ZIP archive support for batch downloads in the Image Picka extension, addressing issue #395. ZIP is more widely recognized and supported across different operating systems compared to TAR, making it a valuable addition for users, especially on mobile platforms.
Changes:
- Added client-zip dependency to enable ZIP file creation
- Implemented zip-packer.js with similar interface to tar-packer.js
- Updated options UI to include ZIP as a packer choice
- Changed default packer for Android from TAR to ZIP
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| package.json | Added client-zip@2.5.0 as a new dependency |
| package-lock.json | Lock file update for client-zip dependency |
| src/lib/zip-packer.js | New ZIP packer implementation using client-zip library and File System Access API |
| src/background.js | Refactored getPacker() to support multiple packer types via PACKERS object |
| src/options.js | Added "zip" option to packer dropdown in options UI |
| src/static/_locales/en/messages.json | Added "ZIP archive" localization string |
| src/lib/pref.js | Changed default packer for Android from "tar" to "zip" |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/lib/zip-packer.js
Outdated
| if (typeof navigator.storage?.getDirectory !== 'function') { | ||
| throw new Error('File System Access API is not supported in this browser.'); | ||
| } | ||
| const tarName = `temp-${Date.now()}-${Math.random().toString(16).slice(2)}.${EXT}`; |
There was a problem hiding this comment.
The variable name tarName is misleading in a ZIP packer implementation. Consider renaming it to something more generic like archiveName or tempFileName to avoid confusion, or use a ZIP-specific name like zipName.
| console.warn(err); | ||
| } | ||
| try { | ||
| const getPacker = PACKERS[pref.get("packer")]; |
There was a problem hiding this comment.
When pref.get("packer") returns "none" or any other value not in the PACKERS object, getPacker on line 542 will be undefined, causing line 543 to throw "undefined is not a function". The previous implementation handled this by only checking for "tar" explicitly and falling back to getRawPacker() for all other values including "none". Add a check like if (!getPacker) return getRawPacker(); after line 542 to preserve this behavior.
| const getPacker = PACKERS[pref.get("packer")]; | |
| const getPacker = PACKERS[pref.get("packer")]; | |
| if (!getPacker) { | |
| return getRawPacker(); | |
| } |
| itemReady.resolve({ | ||
| name: filename, | ||
| input: blob, |
There was a problem hiding this comment.
Unlike tar-packer.js which explicitly awaits blob.stream().pipeTo(fileStream), this implementation doesn't explicitly wait for the blob to be consumed. The synchronization relies on client-zip's stream backpressure to prevent memory issues. While this should work correctly, it's a different pattern from tar-packer and may behave differently under high load or with very large files. Consider documenting this design choice or verifying the backpressure behavior if issues arise.
| itemReady.resolve({ | |
| name: filename, | |
| input: blob, | |
| // Pass a ReadableStream to client-zip so that blob data is consumed | |
| // via the browser's streaming backpressure rather than as a whole Blob. | |
| itemReady.resolve({ | |
| name: filename, | |
| input: blob.stream(), |
| const PACKERS = { | ||
| tar: getTarPacker, | ||
| zip: getZipPacker | ||
| } |
There was a problem hiding this comment.
Avoid automated semicolon insertion (94% of all statements in the enclosing script have an explicit semicolon).
| } | |
| }; |
|
Thank you very much for adding ZIP support to Image Picka 🙌 Using ZIP instead of TAR makes batch downloads much more convenient, especially on Windows and Android. Best regards, |
Fixes #395