Repository navigation
Conversation
m4bard
force-pushed
the
feat/embed-cover-art
branch
from
September 15, 2026 18:35
1c5e393 to
b73b858
Compare
This was referenced Oct 6, 2026
Contributor
Author
|
I've added two commits to this PR: The reason is #843. With
The new test, Readarr only writes tags on import when you turn it on ( Disclosure: drafted with Claude Code at my direction; I read the cited code at the stated commit and reviewed this before posting. |
m4bard
added a commit
to m4bard/Listenarr
that referenced
this pull request
Oct 6, 2026
…ts test The ASIN hardlink guard from below meets Listenarrs#914's widened condition (ASIN or cover art) in ManualImportController.ProcessItem.cs, and the conflict there was resolved by guarding the combined tag write: under HardlinkCopy neither the ASIN nor the artwork is written, since the destination is the source's inode. This adds the regression test for the cover-art half, and points the ASIN guard's test at WriteImportTagsAsync, the call Listenarrs#914 replaced WriteAsinTagAsync with. The code is the same as fix/914-hardlink-guard's. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> (cherry picked from commit 5d0ba95)
m4bard
marked this pull request as draft
October 29, 2026 03:30
Contributor
Author
|
I am moving this to draft. I think it should wait for the staged-copy tagging design in #842, which you scoped there and which I still owe a sketch for, since cover art would be a second payload written to the same staged copy and I do not want to rebuild it before that exists. The cover-art code and its tests look reusable to me, so I plan to bring them back on top of that change; tell me if you would rather I close this instead. Disclosure: drafted with Claude Code at my direction; I read the cited code at the stated commit and reviewed this before posting. |
EnableCoverArtDownload has been persisted since the Settings page was added and read by nothing. Listenarrs#833 asked which direction to take it. This takes the reading the *arr family supports: the flag governs embedding artwork into audio files, not whether artwork is fetched for the UI. That distinction comes from checking the lineage first. Readarr, Lidarr and Sonarr have no toggle at all for downloading cover images for the UI; MediaCoverService takes IConfigFileProvider only for UrlBase. The boolean of this exact shape in that family is Lidarr's EmbedCoverArt (ConfigService.cs:308, read at AudioTagService.cs:117), whose label is "Embed Cover Art In Audio Files". Gating Listenarr's image cache instead would also have blanked author and series artwork, because the cache chokepoint cannot tell which kind of image it is holding. A new column rather than the old one. EnableCoverArtDownload is true on every existing instance because that was the property initialiser, not because an operator chose it. Reusing it would mean upgrading straight into rewriting audio files with no opt-in step. EmbedCoverArtInAudioFiles is added with defaultValue false, so existing installs are off by construction and no data repair migration is needed, which the architecture tests forbid in a PR anyway. The dead flag is left in place so this stays a single additive EF scaffold; dropping it is a separate mechanical change and I did not want it riding along with behaviour. On the default, deliberately not matching Lidarr's literal true: Lidarr hides EmbedCoverArt behind WriteAudioTags, which defaults to No, so nothing embeds out of the box there. Listenarr has no equivalent master switch and its ASIN write is unconditional, so porting the default without the gate would be more aggressive than the setting it copies. Artwork and the ASIN are written in one open and save. An ASIN is a few bytes and a cover is commonly a few hundred kilobytes, going into an M4B through a generation-bound lease, so a second pass would rewrite a file that is often several gigabytes twice. The bytes are identified by their leading bytes rather than trusted from a URL or a Content-Type. An image host answering a request with an HTML error page would otherwise be embedded under image/jpeg, producing a file whose cover silently fails to render. Anything unrecognised is dropped and the ASIN write still happens. This also gives DownloadCoverArtAsync its first production caller; it has been dead since it was written. It fetches from the URL rather than the image cache: resolving a cached path means reproducing the temp-versus- library layout handling in ImagesController, which is worth doing but not in the same change as new behaviour. Scope: manual import only, which is where the one existing tag write lives. The automatic import path writes no tags at all today and is left alone. Nothing rewrites artwork for files already in the library. Tests: the setting gates both the write and the fetch, so a disabled instance makes no request; an unrecognised response still writes the ASIN; the byte sniffing names jpeg, png and webp and refuses an HTML body. Confirmed the gate test fails with the settings check removed. Backend 3124 passed, 0 failed, 130 skipped. Frontend 607 passed.
Two XML doc blocks landed above the wrong member. In IMetadataService the new WriteImportTagsAsync declaration was inserted between the existing summary/param/returns for DownloadCoverArtAsync and that method, so WriteImportTagsAsync ended up carrying two summary tags, a returns tag describing a byte array it does not return, and a param tag naming one of its three parameters, while DownloadCoverArtAsync ended up with no documentation at all. In ApplicationSettings the block describing EmbedCoverArtInAudioFiles sits above EnableCoverArtDownload, which then carries two summary tags while the property the text is about carries none. Both moves are comment text only. No member, signature, initialiser or column changes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ppending ApplyCoverArt makes a product decision in its docstring, that artwork replaces whatever the file already carries, and nothing asserted it. Break it four ways and the suite stays green: return false without setting Pictures, append instead of replacing, leave MimeType unset, or use the wrong PictureType. ApplyCoverArt becomes internal and is driven by a TagLib.File that owns no bytes, over an ID3v2 tag seeded with an existing back cover. The assertions are the single resulting picture, the MIME type, the picture type and the bytes. Checked that it discriminates: appending instead of replacing and dropping the MimeType assignment both fail it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The tag write was gated on the ASIN alone. That gate predates this feature and was right when the ASIN was the only payload, but a book matched outside Audible has no ASIN and does have an ImageUrl, so the new setting was silently inert for every one of them. The gate now admits either. WriteImportTagsAsync already handled a null asin downstream, so the only other change is widening the parameter from string to string? through IMetadataService, MetadataService and the test mock, which is what the call site now actually passes. Two controller tests, both on MetadataServiceMock.ImportTagWrites: a book with artwork and no ASIN records one write with a null asin and the image url, and a book with neither records none. Checked that they discriminate: narrowing the gate back to the ASIN fails the first, opening it unconditionally fails the second. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
PR 918 adds a migration too, and the two branches were rewriting the same two lines of this file: the const beneath WeakStorageVerifiedCleanupMigrationId, and the last entry of the expected post-canary list. A pairwise merge check found that pair conflicting in both orders. This branch's migration id now sits in its own declaration at the top of the class and is asserted on its own, and the shared list literal goes back to the text canary has. What is still pinned is that the branch's migration is applied, that it sorts after the consolidated history, and that the rest of the post-canary history is exactly the list canary has. Checked that it discriminates: point the id at a migration that does not exist and the test fails. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… file A pairwise merge check found this branch conflicting with PR 843 in both orders, in TagLibAudioTagWriter.cs and in its test file. Neither change is incompatible with the other: they simply landed on adjacent lines. 843 makes ApplyAsinTag internal so its own test can drive it, and this branch inserts ApplyCoverArt immediately above that declaration and appends to the end of the same test class. So ApplyCoverArt moves below ApplyAsinTag, and this branch's test and its stub file move above the existing tests and take a name of their own. No behaviour changes and no assertion changes. Both orders merge clean afterwards. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Regression test for the cover-art and ASIN write under HardlinkCopy. A hardlinked destination is the source's inode, so a tag write rewrites the file a download client may still be seeding. Copy is the control. Fails on this commit by design; the guard follows. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Under HardlinkCopy the registration lease is the hardlinked destination, the same inode as the source a download client may still be seeding, so embedding cover art or the ASIN there rewrites the source too. Measured with Listenarrs#843's read-write stream applied: Copy left the source unchanged, HardlinkCopy changed it, for both m4b and mp3. The write is now skipped for HardlinkCopy and logged at Debug; Copy and Move are tagged as before. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…ites The markerless mover now hands out leases that defer metadata writes, so the cover art and hardlink tests never reached the tagging call and could not tell a skipped write from a refused one. A lease that reports metadata writes as supported restores that, and the no-tag controls become real controls. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
m4bard
force-pushed
the
feat/embed-cover-art
branch
from
October 29, 2026 19:33
519a256 to
61d7a3e
Compare
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
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.
Closes #833, if you want the setting kept. I offered this as a branch on that issue rather than a PR because the default is a judgement call. Opening it now so it is reviewable, with that question still genuinely open.
What #833 found
EnableCoverArtDownloadhas been persisted since the Settings page was added and read by nothing. One declaration, no reader anywhere in the backend, and a checkbox saying "Download and embed cover art for audiobooks" that describes neither half of what happens.The direction, and why it is not the obvious one
Checking the family first changed my answer. Readarr, Lidarr and Sonarr have no toggle at all for downloading cover images for the UI:
MediaCoverServicetakesIConfigFileProvideronly forUrlBase. The boolean of this exact shape is Lidarr'sEmbedCoverArt(ConfigService.cs:308, read atAudioTagService.cs:117), labelled "Embed Cover Art In Audio Files". So in that family a flag like this governs embedding, not fetching.That also rules out the change I would otherwise have made. Gating the existing image cache looked cheapest, but
ImageCacheServicecannot tell an audiobook cover from an author or series image, so a checkbox reading "cover art for audiobooks" would have silently blanked author and series artwork.Off by default, deliberately not matching Lidarr
Lidarr defaults
EmbedCoverArtto true, but hides it behindWriteAudioTags, which defaults toNo, so nothing embeds out of the box there. Listenarr has no equivalent master switch and its ASIN write is unconditional, so porting the default without the gate would be more aggressive than the setting it copies.This is the part I am least sure you will agree with. It is a one-line change if you would rather it defaulted on.
Why a new column rather than wiring the old one
EnableCoverArtDownloadis true on every existing instance because that was the property initialiser, not because an operator chose it. Two approaches did not survive:MigrationProvenanceArchitectureTestsforbids outrightRenameColumnand therefore preserves the stored true, so every existing install would silently start rewriting audio files on upgradeEmbedCoverArtInAudioFilesis added withdefaultValue: false, so existing rows are off by construction. The dead flag is left in place so this stays a single additive scaffold. Dropping it is a separate mechanical change and I did not want it riding along with behaviour.Scope
Manual import only, which is where the one existing tag write lives. Artwork and the ASIN go in one open and save, since a second pass would rewrite a file that is often several gigabytes twice. The automatic import path writes no tags at all today and is untouched, and nothing rewrites artwork for files already in the library.
The bytes are identified from their leading bytes rather than trusted from a URL or a
Content-Type, because an image host answering with an HTML error page would otherwise be embedded underimage/jpegand produce a file whose cover silently fails to render.It also gives
DownloadCoverArtAsyncits first production caller. It has been dead since it was written.Conflicts to expect
This edits the pinned migration list in
SqliteMigrationSchemaTestsand the model snapshot, so it collides with any other PR adding a migration, including your #841 against our #763. It also collides with the release blocklist branch for the same reason. Whichever lands second wants a rebase; the resolution is keeping both ids in migration-timestamp order.Tests
Backend 3124 passed, frontend 607 passed. The gate has a control: the test fails with the settings check removed, which I checked rather than assumed.
Happy to change the default, split the frontend copy fix out on its own, or close this if you would rather the setting were removed instead.
Worked through with Claude Code at my direction. The suite numbers are runs on this machine against this branch, and the family citations are source-reading against Lidarr and Readarr. I reviewed this before posting.