feat(media): collapse singleton media containers#860
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR implements singleton container fallback resolution and equivalent-media metadata merging: single-child directories can resolve to their sole descendant, and media handlers merge or select metadata across equivalent media IDs (child and validated parent aliases). DB, resolver, browse, image/meta handlers, tests, and docs were added/updated. ChangesSingleton Media Container and Alias Resolution
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ 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)
Comment |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/api/methods/media_alias.go (1)
30-132:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd direct tests for the new alias helper behaviors.
This file introduces new resolution/merge logic, but I don’t see helper-focused tests in the provided changes for: nil-guard behavior, parent↔child equivalence cases, and de-dupe precedence in
mergeMediaTags/mergeMediaProperties. Please add tests for these paths before merge.As per coding guidelines "Write tests for all new code — see TESTING.md and pkg/testing/README.md".
🤖 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 `@pkg/api/methods/media_alias.go` around lines 30 - 132, Add unit tests covering the new helper behaviors: for equivalentMediaIDs add nil-guard tests (call equivalentMediaIDs with env == nil, row == nil, env.Database == nil and env.Database.MediaDB == nil and assert it returns non-error nil slice), parent↔child equivalence tests (mock/stub MediaDB.FindSingleDescendantMedia and FindMediaBySystemAndPath so child is returned, onlyChild equals row.DBID for parentPath and parent is returned; assert returned IDs include parent and child appropriately), and edge-case where parentPath == "" or parentPath == row.Path returns only the original id; for mergeMediaTags and mergeMediaProperties add tests verifying de-duplication and precedence (when duplicates exist ensure primary items are kept first and aliases do not overwrite them, and that duplicate Type+Tag or TypeTag entries are removed). Use the functions equivalentMediaIDs, mergeMediaTags and mergeMediaProperties and mock the MediaDB methods (FindSingleDescendantMedia, FindMediaBySystemAndPath) to control scenarios.
🧹 Nitpick comments (3)
pkg/api/methods/media_resolve_test.go (1)
139-140: ⚡ Quick winUse
filepath.Join(with slash normalization) instead of manual"/"concatenation in test paths.The new setup uses
containerPath + "/Game.nes"andcontainerPath + "/"; convert these to join-based construction for consistency and portability.As per coding guidelines "Use
filepath.Joinfor path construction everywhere, including test files — never hardcode POSIX-style paths like "/roms/snes/game.sfc" as string literals".🤖 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 `@pkg/api/methods/media_resolve_test.go` around lines 139 - 140, Replace manual string concatenation for paths: construct childPath with filepath.Join(containerPath, "Game.nes") and set Media.ParentDir by normalizing the containerPath with filepath.ToSlash(containerPath) and appending a single "/" (to preserve the trailing-dir semantics). Add the appropriate imports (path/filepath and the one providing ToSlash if needed) and update the media := database.Media{DBID: 20, Path: childPath, ParentDir: ...} initializer to use these join/normalization helpers instead of containerPath + "/".pkg/database/mediadb/sql_scraper_test.go (1)
124-133: ⚡ Quick winReplace slash-concatenated test paths with
filepath.Join-based construction.The new tests build paths via
parent+"/..."andparent+"/"in multiple places; switch these tofilepath.Join(+filepath.ToSlashwhere needed) to keep path handling consistent and portable.As per coding guidelines "Use
filepath.Joinfor path construction everywhere, including test files — never hardcode POSIX-style paths like "/roms/snes/game.sfc" as string literals".Also applies to: 146-160
🤖 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 `@pkg/database/mediadb/sql_scraper_test.go` around lines 124 - 133, The test constructs paths by concatenating parent with string literals (e.g. parent+"/one.nes" and parent+"/") inside the mediaDB.sql.ExecContext call; replace those concatenations with filepath.Join(parent, "one.nes") and filepath.Join(parent, "") or filepath.Join(parent, ""); if the SQL needs POSIX slashes, wrap the joined paths with filepath.ToSlash. Update all similar occurrences in this file (including the other instances around the 146-160 area) so every test path uses filepath.Join (and filepath.ToSlash only where the DB/input expects forward slashes) instead of string concatenation.pkg/api/methods/media_meta.go (1)
102-117: 🏗️ Heavy liftPrecompute alias metadata once for the whole batch.
The batch loop recomputes
equivalentMediaIDs()and thenmergedMediaMeta()issues freshGetMediaTagsByMediaDBIDs/GetMediaPropertyMetadataByMediaDBIDscalls for every aliased row. On alias-heavy batches this falls back to an N+1 query pattern and bypasses the maps already loaded at Lines 58-66. Consider collecting the union of equivalent IDs up front and merging from one prefetched result set inside the loop.Also applies to: 136-173
🤖 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 `@pkg/api/methods/media_meta.go` around lines 102 - 117, The loop currently calls equivalentMediaIDs() and mergedMediaMeta() per item causing N+1 DB calls (GetMediaTagsByMediaDBIDs / GetMediaPropertyMetadataByMediaDBIDs) and bypasses the preloaded maps (mediaTagsByID, mediaPropsByID); instead, before the loop call: collect the union of all equivalentMediaIDs for the batch (using equivalentMediaIDs on each Row once), fetch merged tag/property results once (via the same underlying GetMediaTagsByMediaDBIDs / GetMediaPropertyMetadataByMediaDBIDs used by mergedMediaMeta), merge those results into the existing maps (mediaTagsByID, mediaPropsByID), and then inside the loop use those maps to buildMediaMetaResponse without calling mergedMediaMeta again (apply same change to the second loop region lines 136-173).
🤖 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 `@pkg/api/methods/media_browse.go`:
- Around line 659-660: The code calls annotateSingletonDirectoryEntry
per-directory which triggers up to five serial MediaDB lookups for each
singleton directory; instead batch these lookups by first collecting all
singleton directories discovered by BrowseDirectories, issuing a single MediaDB
query to fetch the descendant/asset metadata for all involved SystemIDs at once,
then map results back to each directory and annotate entries in a loop.
Concretely: change the flow around BrowseDirectories to accumulate dir.SystemIDs
for singleton dirs, add a batched lookup function (or extend the existing
MediaDB query) that returns metadata keyed by SystemID, and replace per-entry
calls to annotateSingletonDirectoryEntry with a batched annotator that consumes
the pre-fetched metadata (or modify annotateSingletonDirectoryEntry to accept a
metadata map).
- Around line 651-654: Replace the hardcoded path concat that sets dirPath :=
path + "/" + dir.Name with filepath.Join to correctly build filesystem paths
(use filepath.Join(path, dir.Name)); update the import to include
"path/filepath" if absent and ensure the models.BrowseEntry{ Name: dir.Name,
Path: dirPath } uses the joined path; locate this change around the code that
constructs BrowseEntry objects in media_browse.go (the dirPath variable and the
models.BrowseEntry initialization) to avoid generating incorrect paths like
//child or C://child.
In `@pkg/api/methods/media_image.go`:
- Around line 143-155: The current change uses metadata-only queries
(GetMediaTitlePropertyMetadata and mediaImagePropSources) but
loadMediaImageProperty expects full property fields (prop.Binary, prop.BlobDBID,
prop.Text), causing inline-binary images to be dropped; revert to or replace
these metadata calls with the full-property getters (the counterparts to
GetMediaTitlePropertyMetadata/GetMediaPropertyMetadata used elsewhere) when
building mediaPropSources and titleProps passed into
selectMediaImageFromSources, or implement a new batched full-property query that
returns the complete property rows for the same IDs and use that in
mediaImagePropSources (affecting mediaImagePropSources,
db.GetMediaTitlePropertyMetadata usage, and the selectMediaImageFromSources
invocation so loadMediaImageProperty sees prop.Binary/prop.BlobDBID/prop.Text).
---
Outside diff comments:
In `@pkg/api/methods/media_alias.go`:
- Around line 30-132: Add unit tests covering the new helper behaviors: for
equivalentMediaIDs add nil-guard tests (call equivalentMediaIDs with env == nil,
row == nil, env.Database == nil and env.Database.MediaDB == nil and assert it
returns non-error nil slice), parent↔child equivalence tests (mock/stub
MediaDB.FindSingleDescendantMedia and FindMediaBySystemAndPath so child is
returned, onlyChild equals row.DBID for parentPath and parent is returned;
assert returned IDs include parent and child appropriately), and edge-case where
parentPath == "" or parentPath == row.Path returns only the original id; for
mergeMediaTags and mergeMediaProperties add tests verifying de-duplication and
precedence (when duplicates exist ensure primary items are kept first and
aliases do not overwrite them, and that duplicate Type+Tag or TypeTag entries
are removed). Use the functions equivalentMediaIDs, mergeMediaTags and
mergeMediaProperties and mock the MediaDB methods (FindSingleDescendantMedia,
FindMediaBySystemAndPath) to control scenarios.
---
Nitpick comments:
In `@pkg/api/methods/media_meta.go`:
- Around line 102-117: The loop currently calls equivalentMediaIDs() and
mergedMediaMeta() per item causing N+1 DB calls (GetMediaTagsByMediaDBIDs /
GetMediaPropertyMetadataByMediaDBIDs) and bypasses the preloaded maps
(mediaTagsByID, mediaPropsByID); instead, before the loop call: collect the
union of all equivalentMediaIDs for the batch (using equivalentMediaIDs on each
Row once), fetch merged tag/property results once (via the same underlying
GetMediaTagsByMediaDBIDs / GetMediaPropertyMetadataByMediaDBIDs used by
mergedMediaMeta), merge those results into the existing maps (mediaTagsByID,
mediaPropsByID), and then inside the loop use those maps to
buildMediaMetaResponse without calling mergedMediaMeta again (apply same change
to the second loop region lines 136-173).
In `@pkg/api/methods/media_resolve_test.go`:
- Around line 139-140: Replace manual string concatenation for paths: construct
childPath with filepath.Join(containerPath, "Game.nes") and set Media.ParentDir
by normalizing the containerPath with filepath.ToSlash(containerPath) and
appending a single "/" (to preserve the trailing-dir semantics). Add the
appropriate imports (path/filepath and the one providing ToSlash if needed) and
update the media := database.Media{DBID: 20, Path: childPath, ParentDir: ...}
initializer to use these join/normalization helpers instead of containerPath +
"/".
In `@pkg/database/mediadb/sql_scraper_test.go`:
- Around line 124-133: The test constructs paths by concatenating parent with
string literals (e.g. parent+"/one.nes" and parent+"/") inside the
mediaDB.sql.ExecContext call; replace those concatenations with
filepath.Join(parent, "one.nes") and filepath.Join(parent, "") or
filepath.Join(parent, ""); if the SQL needs POSIX slashes, wrap the joined paths
with filepath.ToSlash. Update all similar occurrences in this file (including
the other instances around the 146-160 area) so every test path uses
filepath.Join (and filepath.ToSlash only where the DB/input expects forward
slashes) instead of string concatenation.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 08d569c9-f6f8-4457-b63a-4ece4c8d0a97
📒 Files selected for processing (13)
docs/api/methods.mddocs/scraper.mdpkg/api/methods/media_alias.gopkg/api/methods/media_batch.gopkg/api/methods/media_browse.gopkg/api/methods/media_image.gopkg/api/methods/media_meta.gopkg/api/methods/media_resolve.gopkg/api/methods/media_resolve_test.gopkg/database/database.gopkg/database/mediadb/sql_scraper.gopkg/database/mediadb/sql_scraper_test.gopkg/testing/helpers/db_mocks.go
| if dir.FileCount == 1 { | ||
| annotateSingletonDirectoryEntry(env, &entry, dirPath, dir.SystemIDs, rootDirs) |
There was a problem hiding this comment.
Batch singleton-directory metadata lookups instead of querying per entry.
Lines 659-660 call a helper that does up to five serial MediaDB lookups per singleton directory. Because BrowseDirectories is not capped by maxResults, a directory-heavy first page can turn one browse request into dozens of SQLite round-trips and noticeably worse latency. Please batch this enrichment or extend the browse query to return the descendant metadata in one pass.
Also applies to: 742-781
🤖 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 `@pkg/api/methods/media_browse.go` around lines 659 - 660, The code calls
annotateSingletonDirectoryEntry per-directory which triggers up to five serial
MediaDB lookups for each singleton directory; instead batch these lookups by
first collecting all singleton directories discovered by BrowseDirectories,
issuing a single MediaDB query to fetch the descendant/asset metadata for all
involved SystemIDs at once, then map results back to each directory and annotate
entries in a loop. Concretely: change the flow around BrowseDirectories to
accumulate dir.SystemIDs for singleton dirs, add a batched lookup function (or
extend the existing MediaDB query) that returns metadata keyed by SystemID, and
replace per-entry calls to annotateSingletonDirectoryEntry with a batched
annotator that consumes the pre-fetched metadata (or modify
annotateSingletonDirectoryEntry to accept a metadata map).
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 (2)
pkg/api/methods/media_alias.go (1)
50-142: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd direct tests for alias ID and merge semantics.
This new helper logic adds parent/child alias branching plus tag/property dedup rules, but the supplied tests only exercise resolver fallback. Please add table-driven coverage for
equivalentMediaIDs,mergeMediaTags, andmergeMediaPropertiesso these behaviors are locked in before merge.As per coding guidelines,
**/*.go: Write tests for all new code — see TESTING.md and pkg/testing/README.md🤖 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 `@pkg/api/methods/media_alias.go` around lines 50 - 142, Add table-driven unit tests for equivalentMediaIDs, mergeMediaTags, and mergeMediaProperties: write tests covering nil inputs, singleton aliases disabled, child-only, parent-child branching, and deduplication semantics. For equivalentMediaIDs, create a RequestEnv with a fake MediaDB that implements FindSingleDescendantMedia and FindMediaBySystemAndPath to return controlled media rows (including nils) and assert returned ID slices for DBID ordering and uniqueness; also include cases where singletonMediaAliasesEnabled(env) is false. For mergeMediaTags, add cases that check duplicate suppression using the key tag.Type + "\x00" + tag.Tag and order preservation (primary first, then aliases); for mergeMediaProperties, add cases asserting uniqueness by prop.TypeTag and order behavior. Use table-driven tests in a _test.go file with clear test names and assertions.pkg/api/methods/media_resolve_test.go (1)
147-159:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAssert the platform mock expectations (
pl.AssertExpectations(t)).These tests stub
pl.On("Settings", ...)but never callpl.AssertExpectations(t), so they won’t fail ifresolveMediaBySystemAndPathstops consulting platform settings.Suggested fix
assert.Equal(t, childPath, result.Path) mockDB.AssertExpectations(t) + pl.AssertExpectations(t) @@ assert.Contains(t, err.Error(), "media not found") mockDB.AssertExpectations(t) + pl.AssertExpectations(t) @@ assert.Contains(t, err.Error(), "media not found") mockDB.AssertNotCalled(t, "FindSingleDescendantMedia", mock.Anything, system.DBID, containerPath) mockDB.AssertExpectations(t) + pl.AssertExpectations(t) @@ assert.Contains(t, err.Error(), "media not found") mockDB.AssertExpectations(t) pl.AssertNotCalled(t, "RootDirs", mock.Anything) + pl.AssertExpectations(t) @@ assert.Contains(t, err.Error(), "media not found") mockDB.AssertExpectations(t) pl.AssertNotCalled(t, "RootDirs", mock.Anything) + pl.AssertExpectations(t) @@ assert.Contains(t, err.Error(), "media not found") mockDB.AssertExpectations(t) pl.AssertNotCalled(t, "RootDirs", mock.Anything) + pl.AssertExpectations(t)Also applies to: 171-182, 194-204, 219-229, 241-251, 266-276
🤖 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 `@pkg/api/methods/media_resolve_test.go` around lines 147 - 159, The platform mock expectations are never asserted in these tests, so add pl.AssertExpectations(t) after mockDB.AssertExpectations(t) (or immediately after the test call) in each test that sets pl.On("Settings"...), e.g., in the test exercising resolveMediaBySystemAndPath ensure you call pl.AssertExpectations(t) to verify resolveMediaBySystemAndPath consulted platform settings; apply the same addition to the other similar test blocks referenced (the tests around the other ranges that stub pl.Settings).
🤖 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 `@pkg/api/methods/media_alias.go`:
- Around line 50-142: Add table-driven unit tests for equivalentMediaIDs,
mergeMediaTags, and mergeMediaProperties: write tests covering nil inputs,
singleton aliases disabled, child-only, parent-child branching, and
deduplication semantics. For equivalentMediaIDs, create a RequestEnv with a fake
MediaDB that implements FindSingleDescendantMedia and FindMediaBySystemAndPath
to return controlled media rows (including nils) and assert returned ID slices
for DBID ordering and uniqueness; also include cases where
singletonMediaAliasesEnabled(env) is false. For mergeMediaTags, add cases that
check duplicate suppression using the key tag.Type + "\x00" + tag.Tag and order
preservation (primary first, then aliases); for mergeMediaProperties, add cases
asserting uniqueness by prop.TypeTag and order behavior. Use table-driven tests
in a _test.go file with clear test names and assertions.
In `@pkg/api/methods/media_resolve_test.go`:
- Around line 147-159: The platform mock expectations are never asserted in
these tests, so add pl.AssertExpectations(t) after mockDB.AssertExpectations(t)
(or immediately after the test call) in each test that sets
pl.On("Settings"...), e.g., in the test exercising resolveMediaBySystemAndPath
ensure you call pl.AssertExpectations(t) to verify resolveMediaBySystemAndPath
consulted platform settings; apply the same addition to the other similar test
blocks referenced (the tests around the other ranges that stub pl.Settings).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 95cfd430-2f2e-466a-8711-6610ba3ca7a9
📒 Files selected for processing (5)
docs/api/methods.mddocs/scraper.mdpkg/api/methods/media_alias.gopkg/api/methods/media_browse.gopkg/api/methods/media_resolve_test.go
✅ Files skipped from review due to trivial changes (2)
- docs/scraper.md
- docs/api/methods.md
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/api/methods/media_browse.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@pkg/api/methods/media_alias_test.go`:
- Around line 68-69: Update the test literals in
pkg/api/methods/media_alias_test.go to build paths with filepath.Join instead of
hardcoded slash strings: replace occurrences used for Path (e.g.,
"roms/Game.zip/Game.nes"), ParentDir (e.g., "roms/Game.zip/"), and archive base
("roms/Game.zip") with filepath.Join calls so path separators are correct on all
platforms; for ParentDir preserve the trailing-separator semantics by appending
os.PathSeparator (or using filepath.Join and adding string(os.PathSeparator)) as
needed when constructing the ParentDir field; update all mentioned instances at
the Path/ParentDir assignments and other comparisons so they use the new
filepath.Join-based values.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8c99cd40-2b38-4cfd-a4d1-f11191e4719a
📒 Files selected for processing (5)
pkg/api/methods/media_alias_test.gopkg/api/methods/media_browse.gopkg/api/methods/media_image.gopkg/api/methods/media_resolve_test.gopkg/database/mediadb/sql_scraper_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/api/methods/media_image.go
- pkg/database/mediadb/sql_scraper_test.go
- pkg/api/methods/media_browse.go
- pkg/api/methods/media_resolve_test.go
| Path: "roms/Game.zip/Game.nes", | ||
| ParentDir: "roms/Game.zip/", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify slash-delimited hardcoded paths in Go tests under methods.
rg -nP --type=go '"[^"]*/[^"]*"' pkg/api/methods/media_alias_test.goRepository: ZaparooProject/zaparoo-core
Length of output: 1022
Use filepath.Join for test path literals in pkg/api/methods/media_alias_test.go
Replace the hardcoded slash-delimited strings (e.g., "roms/Game.zip/Game.nes", "roms/Game.zip/", "roms/Game.zip") at lines 68-69, 73, 77-79, 107, and 111 with filepath.Join-constructed paths, keeping ParentDir’s trailing-separator semantics consistent with the existing expectations.
🤖 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 `@pkg/api/methods/media_alias_test.go` around lines 68 - 69, Update the test
literals in pkg/api/methods/media_alias_test.go to build paths with
filepath.Join instead of hardcoded slash strings: replace occurrences used for
Path (e.g., "roms/Game.zip/Game.nes"), ParentDir (e.g., "roms/Game.zip/"), and
archive base ("roms/Game.zip") with filepath.Join calls so path separators are
correct on all platforms; for ParentDir preserve the trailing-separator
semantics by appending os.PathSeparator (or using filepath.Join and adding
string(os.PathSeparator)) as needed when constructing the ParentDir field;
update all mentioned instances at the Path/ParentDir assignments and other
comparisons so they use the new filepath.Join-based values.
Summary
Verification
Summary by CodeRabbit
New Features
Documentation
Tests