feat: Support saving embedded images based on their hash#19
Merged
Conversation
- We're currently testing `getHashedArtwork` out in uncommitted files, which requires using `expo-file-system` to create the image directory.
…rtwork - We want this data class to be immutable, so in order to generate the `saveUri` from a hash, we need to expose a function to return a new `ArtworkOptions` instance.
- We do the argument validation in the exposed JS function for `saveHashedArtwork`.
… doing everything at once in a `Promise.allSettled` - Went from taking `~83s` to `~30s` for getting all artwork + saving the images to the device. - Weirdly enough, if we save in WEBP, both methods take the same amount of time (~84s).
- This only works with the for-loop method. - It now takes `~23s` for the 1st save. - On successive checks, it takes `~20s` (with the hashed images already existing).
- Dropped the config options as it's pretty useless - if the user wants to change the max image size, they can just patch the package.
📝 WalkthroughWalkthroughRemoves ChangesHash-based artwork saving
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@android/src/main/java/com/cyanchill/missingcore/metadataretriever/models/ArtworkOptions.kt`:
- Line 49: The ArtworkOptions parsing currently accepts the raw compress value
from the options bundle without enforcing the native 0.0–1.0 contract. Update
the ArtworkOptions constructor/parsing logic where compress is read from
optionsBundle to validate it before storing it, either by clamping it into range
or rejecting invalid values so downstream image-quality conversion stays
predictable.
In
`@android/src/main/java/com/cyanchill/missingcore/metadataretriever/modules/ArtworkParser.kt`:
- Around line 57-60: The fallback artwork extraction in ArtworkParser’s use of
MediaMetadataRetriever can leak the retriever if setDataSource or
embeddedPicture throws before release() runs. Update the fallback read block
around the mmrMetadata instance to use try/finally so release() always executes,
and keep the existing coverImage assignment and Normalization.getSafeUri(uri)
flow inside the guarded section.
- Line 91: The data URI returned by ArtworkParser’s base64 encoder is using
Base64.DEFAULT, which can insert line breaks and break consumers. Update the
encoding in the ArtworkParser return path to use Base64.NO_WRAP so the payload
stays on one line and the generated data URI remains valid.
In
`@android/src/main/java/com/cyanchill/missingcore/metadataretriever/modules/MetadataRetrieverModule.kt`:
- Around line 199-206: The cache check in MetadataRetrieverModule’s
safeExecuteOnURI path only looks at artworkOptions.knownHashes, so it can return
a URI for a file that does not exist when the format-specific file is missing.
Update the logic around hashedArtworkOptions and artwork.asFile to verify the
target file exists at the generated saveUri before skipping the write, and if it
is missing, re-save the artwork or fall back to the existing null resolution
path.
In `@example/src/data/useTracksWithSavedArtwork.ts`:
- Around line 54-63: The try/catch in useTracksWithSavedArtwork currently wraps
both saveHashedArtwork and tracksMetadata.push, so a save failure can drop the
whole track from results. Refactor the logic around saveHashedArtwork and
tracksMetadata.push so only artwork saving is guarded, and always push the track
metadata using artworkData: null when saveHashedArtwork fails; use the
identifiers saveHashedArtwork, savedHashedImages, and tracksMetadata to locate
the affected block.
- Around line 40-43: The directory enumeration in useTracksWithSavedArtwork
should not rely on the internal listAsRecords helper. Update the
savedHashedImages logic to use the public getImageDirectory().list() API
instead, then derive the hash values from the returned entries in the same
place. Keep the behavior of filtering out undefined values, but make the change
within the existing useTracksWithSavedArtwork flow so it remains compatible with
expo-file-system SDK updates.
In `@src/index.ts`:
- Around line 74-85: Guard the `options` parameter at the start of
`saveHashedArtwork` before accessing `options.saveDirectory` or
`options.knownHashes`, since plain JS callers may pass nothing and trigger a
property read error. Add an explicit validation for `options` in
`saveHashedArtwork` so the function throws a controlled custom error instead of
`Cannot read properties of undefined`, then keep the existing required-field
checks for `saveDirectory` and `knownHashes`.
In `@src/types/ArtworkOptions.ts`:
- Around line 31-38: `HashedArtworkOptions` currently keys cached artwork only
by the raw-artwork hash, so different `format` or `compress` settings can
incorrectly reuse a stale file. Update the hashed cache identity in
`HashedArtworkOptions` and the code that computes/looks up entries for
`saveDirectory`/`knownHashes` so the serialization settings are part of the key,
or otherwise enforce a single fixed output profile per hashed directory. Make
sure the lookup and persistence logic stay consistent with the chosen identity
in the artwork-saving flow.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 049e34d5-0fff-4313-935f-27d3416051d1
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (16)
CHANGELOG.mdREADME.mdandroid/src/main/java/com/cyanchill/missingcore/metadataretriever/models/ArtworkOptions.ktandroid/src/main/java/com/cyanchill/missingcore/metadataretriever/modules/APIConfigs.ktandroid/src/main/java/com/cyanchill/missingcore/metadataretriever/modules/ArtworkParser.ktandroid/src/main/java/com/cyanchill/missingcore/metadataretriever/modules/MetadataReader.ktandroid/src/main/java/com/cyanchill/missingcore/metadataretriever/modules/MetadataRetrieverModule.ktandroid/src/main/java/com/cyanchill/missingcore/metadataretriever/utils/Exceptions.ktexample/package.jsonexample/src/data/useTracksWithBase64Artwork.tsexample/src/data/useTracksWithSavedArtwork.tsexample/src/utils/fs.tssrc/NativeMetadataRetriever.tssrc/index.tssrc/types/ArtworkOptions.tssrc/types/ConfigOptions.ts
💤 Files with no reviewable changes (3)
- src/types/ConfigOptions.ts
- android/src/main/java/com/cyanchill/missingcore/metadataretriever/modules/APIConfigs.kt
- example/src/data/useTracksWithBase64Artwork.ts
- Clamp `compress` option in Kotlin. - Ensure `MediaMetadataRetriever()` is released. - Use `NO_WRAP` instead of `DEFAULT` when converting ByteArray to base64.
- Although, there shouldn't be any directories in our image folder.
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
android/src/main/java/com/cyanchill/missingcore/metadataretriever/modules/ArtworkParser.kt (1)
97-114: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winCheck
Bitmap.compress()before returning the URI.Bitmap.compress(...)can returnfalsewithout throwing, so this path can hand back a URI for a broken/partial file. Returnnull(and delete the temp file) when compression fails.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@android/src/main/java/com/cyanchill/missingcore/metadataretriever/modules/ArtworkParser.kt` around lines 97 - 114, The asFile function in ArtworkParser should not return a URI when Bitmap.compress fails silently. Check the boolean result of bitmap.compress before flushing/returning, and if it is false, clean up by deleting the partially written file and return null instead. Keep the fix within asFile, using the existing imgUri, FileOutputStream, and formatUri flow.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@android/src/main/java/com/cyanchill/missingcore/metadataretriever/modules/ArtworkParser.kt`:
- Around line 97-114: The asFile function in ArtworkParser should not return a
URI when Bitmap.compress fails silently. Check the boolean result of
bitmap.compress before flushing/returning, and if it is false, clean up by
deleting the partially written file and return null instead. Keep the fix within
asFile, using the existing imgUri, FileOutputStream, and formatUri flow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0781aeea-0a0a-4cfd-8d48-91a31e496662
📒 Files selected for processing (8)
CHANGELOG.mdREADME.mdandroid/src/main/java/com/cyanchill/missingcore/metadataretriever/models/ArtworkOptions.ktandroid/src/main/java/com/cyanchill/missingcore/metadataretriever/modules/ArtworkParser.ktandroid/src/main/java/com/cyanchill/missingcore/metadataretriever/modules/MetadataRetrieverModule.ktexample/src/data/useTracksWithSavedArtwork.tssrc/NativeMetadataRetriever.tssrc/types/ArtworkOptions.ts
✅ Files skipped from review due to trivial changes (2)
- CHANGELOG.md
- README.md
🚧 Files skipped from review as they are similar to previous changes (5)
- src/types/ArtworkOptions.ts
- example/src/data/useTracksWithSavedArtwork.ts
- src/NativeMetadataRetriever.ts
- android/src/main/java/com/cyanchill/missingcore/metadataretriever/models/ArtworkOptions.kt
- android/src/main/java/com/cyanchill/missingcore/metadataretriever/modules/MetadataRetrieverModule.kt
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Important
This will probably result in a
v4.0.0release.This PR adds the ability to save embedded artwork by their hashes. This has an advantage over
saveArtworkdue to enabling the user to skip saving a file if the embedded artwork was seen elsewhere previously (via theknownHashesoption).Note
This really only works based on a "social contract", where the hashes passed to
knownHashesare stored insaveDirectory& has the sameformat.Other Changes
updateConfigs().ArtworkParser.ktfile instead.Summary by CodeRabbit
saveHashedArtworkto persist embedded artwork by hash and return{ hash, uri }(ornull).