Feat/missing metadata#168
Merged
Merged
Conversation
📝 WalkthroughWalkthroughThis PR refactors the import queue from single-type items to multi-type items, and changes track metadata defaulting and validation from global settings to per-field configuration. Queue items now store a Types slice with helper methods HasType and PrimaryType; download jobs defer metadata processing to import; and all consumers (import service, views, handlers, templates) adapt to use the new model. ChangesQueue item type model and track metadata configuration
Download job, directory import, and import service changes
View models, handlers, and UI template updates
Telegram, lyrics, and tag writing updates
Feature specification and user documentation
🎯 4 (Complex) | ⏱️ ~75 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 `@config.example.yaml`:
- Around line 42-47: The sample config in config.example.yaml has
allow_missing_metadata.year and allow_missing_metadata.genre set to false which
conflicts with the runtime defaults in src/features/config/default.go (where
year and genre are true); update the example file so allow_missing_metadata.year
and allow_missing_metadata.genre are set to true to match the runtime defaults
and ensure consistent import behavior between generated defaults and the example
config.
In `@src/features/config/config.go`:
- Around line 35-47: The config loader fails when users set
import.allow_missing_metadata as a scalar boolean because AllowMissingMetadata
is a struct of per-field booleans; add a compatibility Unmarshal hook by
implementing AllowMissingMetadata.UnmarshalYAML (or UnmarshalJSON) that accepts
either a bool or a mapping and sets all fields to the bool when a scalar is
provided (true → all fields true, false → all fields false), so
Manager.loadConfig's rootNode.Decode(&cfg) succeeds for legacy scalar configs;
update the AllowMissingMetadata type to include this method and ensure it
handles mapping decoding as the existing behavior.
In `@src/features/downloading/download_job.go`:
- Around line 124-125: executeTrackDownload, executeAlbumDownload, and
executeArtistDownload assume track.Artists[0].Artist.Name exists and will panic
when a downloader returns a track with no artist metadata; update those
functions to defensively access artist data by checking len(track.Artists) > 0
and that track.Artists[0] and track.Artists[0].Artist are non-nil before reading
.Name, falling back to an empty string (or a defined helper like
safeArtistName(track)) where name is used (same fix for the occurrences
referenced around lines ~118, ~212-213, ~218, ~327-328, ~332). Ensure any
downstream uses (logging, filenames, tags) handle the empty/fallback value
safely instead of assuming a non-empty artist name.
In `@src/features/importing/directory_job.go`:
- Line 271: The call that queues a FailedImport uses err.Error() even on the
zero-byte-file path where err can be nil; update the logic around
addTrackToQueue (the call using nullTrackForQueue and QueueItemType
FailedImport) to avoid calling err.Error() when err is nil—e.g. compute an error
string only if err != nil (or pass nil/omit the "error" map entry when err is
nil) and pass that safe value into addTrackToQueue so no panic occurs when tag
reading succeeded but file size is zero.
In `@src/features/importing/handlers.go`:
- Around line 365-369: The grouped "Replace Duplicates" flag is being set too
broadly: change the condition that sets hasDuplicates so it only becomes true
when the view both is a duplicate and can actually be replaced; i.e., replace
the check using view.IsDuplicate with a check that the view is duplicate AND
replaceable (e.g., view.IsDuplicate && view.ReplaceEnabled or, if the view
represents a group, check the group's items for any item.ReplaceEnabled == true)
so the bulk replace button only appears when at least one item in the group has
ReplaceEnabled == true while keeping the existing hasImportable logic using
view.ImportEnabled.
In `@src/features/importing/service.go`:
- Around line 334-336: Call EnsureMetadataDefaults(...) on existingTrack using
s.config.Get().Import.AllowMissingMetadata before you compute the
destination/library path and before performing the move/copy so fallback
artist/album/title/year/genre values are applied to path resolution; update both
spots (the current block around EnsureMetadataDefaults at lines ~334-336 and the
other occurrence around ~429-431) to run EnsureMetadataDefaults first, then
build the destination path and perform the file move/copy operations.
- Around line 298-304: The loop that calls ProcessQueueItem can return success
even when nothing was actually replaced because Duplicate+MissingMetadata items
are routed into the "replace" branch but then rejected by ProcessQueueItem; fix
by tracking whether any items were actually processed and surface an error when
none were replaced. In the loop around ProcessQueueItem (the block that checks
action == "replace"/"import"), add a processedCount (and optional failedCount)
increment when ProcessQueueItem returns nil, and after the loop return an error
(or non-2xx result) if processedCount == 0; alternatively pre-filter items by
checking item.HasType(music.MissingMetadata) combined with
item.HasType(music.Duplicate) before calling ProcessQueueItem so you only call
ProcessQueueItem for items it will accept. Ensure references to ProcessQueueItem
and the action selection logic are used to locate and update the code.
In `@src/music/track.go`:
- Line 237: Normalize missing-metadata checks by introducing and using a
whitespace-aware helper (e.g., isBlank(s string) bool { return
strings.TrimSpace(s) == "" }) and an accessor that finds the first non-empty
artist (e.g., firstValidArtist(t *Track) *ArtistEntry) instead of inspecting
only t.Artists[0]; replace all raw == "" checks on fields like Artist.Name,
Album.Name, Track.Title with isBlank(...) and replace index-only checks
(t.Artists[0].Artist == nil / t.Artists[0].Artist.Name) with a loop that returns
the first Artist != nil and !isBlank(Artist.Name) before defaulting/validation
so you don’t overwrite valid later entries and treats whitespace-only values as
missing.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 362e6415-279c-45a0-bf97-ed44a165f969
📒 Files selected for processing (22)
FEATURE_SPEC_KIT.mdconfig.example.yamldocs/importing.mdsrc/features/config/config.gosrc/features/config/default.gosrc/features/config/handlers.gosrc/features/downloading/download_job.gosrc/features/importing/directory_job.gosrc/features/importing/handlers.gosrc/features/importing/queue.gosrc/features/importing/service.gosrc/features/importing/telegram.gosrc/features/lyrics/handlers.gosrc/features/lyrics/lyrics_job.gosrc/features/lyrics/service.gosrc/infra/tag/tag_writer.gosrc/music/queue.gosrc/music/track.goviews/config/config_form.htmlviews/importing/queue_items.htmlviews/importing/queue_items_grouped_album.htmlviews/importing/queue_items_grouped_artist.html
Comment on lines
+365
to
369
| if view.IsDuplicate { | ||
| hasDuplicates = true | ||
| } else if view.Type != "failed_import" && view.Type != "missing_metadata" { | ||
| } | ||
| if view.ImportEnabled { | ||
| hasImportable = true |
There was a problem hiding this comment.
HasDuplicates is too broad for the grouped “Replace Duplicates” action.
These lines mark a group as replaceable whenever any item is duplicate, even if every duplicate has ReplaceEnabled == false because of missing metadata. The grouped templates will still render a bulk replace button for a group that cannot successfully replace anything.
🤖 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 `@src/features/importing/handlers.go` around lines 365 - 369, The grouped
"Replace Duplicates" flag is being set too broadly: change the condition that
sets hasDuplicates so it only becomes true when the view both is a duplicate and
can actually be replaced; i.e., replace the check using view.IsDuplicate with a
check that the view is duplicate AND replaceable (e.g., view.IsDuplicate &&
view.ReplaceEnabled or, if the view represents a group, check the group's items
for any item.ReplaceEnabled == true) so the bulk replace button only appears
when at least one item in the group has ReplaceEnabled == true while keeping the
existing hasImportable logic using view.ImportEnabled.
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.
PR Improves the importing queue, making it possible to have multiple reasons for why a track was queued in a single queue item. It also expands the
missing_metadataqueue type and makes it configurable so that the user can decide weather it wants to let tracks to it's library with/withtoutTitle,Artist,Album,Genreand/orYear.Defaults for those type have been defined in the domains and added to the configuration feature.
Additionally this PR removes the defaults value for the download job.
Summary by CodeRabbit
Release Notes
New Features
Improvements
Documentation