Feat: Video support #499
Feat: Video support #499
Conversation
📝 WalkthroughWalkthroughAdds first-class asset support (images, videos, optional audio): new helpers for exclusions and filtering, asset-aware pools and logic with GetAsset streaming, ShowVideos/PlayAudio settings across server/client, frontend asset components and playback, OpenAPI/TS client updates, and expanded tests/docs. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Frontend (Home)
participant Logic as PooledImmichFrameLogic
participant Pool as AssetPool
participant API as ImmichApi
participant Filter as AssetExtensionMethods
UI->>Logic: GetAssets(clientIdentifier?)
Logic->>Pool: GetAssets(accountSettings, ct)
Pool->>API: Search / SearchRandom (Type=IMAGE if !ShowVideos else unspecified)
API-->>Pool: Assets list
Pool->>API: GetExcludedAlbumAssets(...) (via AssetHelper, cached)
API-->>Pool: Excluded album assets
Pool->>Filter: ApplyAccountFilters(assets, accountSettings, excluded)
Filter-->>Pool: Filtered assets
Pool-->>Logic: Return filtered assets
Logic-->>UI: Return list
rect rgb(240,255,240)
UI->>Logic: GetAsset(id, assetType?)
Logic->>API: GET /api/Asset/{id}/Asset?assetType=...
API-->>Logic: Binary stream + metadata
Logic-->>UI: FileResult / blob (with Content-Type)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches🧪 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 |
|
If you want to give this a try, use the |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
|
@kdy1968 the setting is under |
|
This is mostly working really well, nice job! I do see some occasional hangs on Frameo (when first launched or often when switching images it just hangs on last image). This is likely due to it's limited CPU and WebView but hopefully there is something we can do to improve this. I also think a setting like video_display_length to only play the first XX seconds would be useful. |
|
seem to be running for 3-5 minute but eventually stops , refreshing browser will resume again , not seeing anything in the log file |
Ooh just found this - fantastic. I tried it, the only slight issue I have noticed so far is that if there is an image displayed next after a video clip, it seems to be shown for a much shorter time than in the settings. (My interval currently set to 3 seconds). |
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
immichFrame.Web/src/lib/components/home-page/home-page.svelte (1)
144-152: GuardhandleDoneagainst double invocation from ProgressBar and videoended
handleDoneis invoked both by theProgressBar(onDone={handleDone}) and byImageComponent’sendedevent:
- Line 144–152:
handleDoneperforms navigation + restart logic.- Line 429–431:
on:ended={() => handleDone(false, false)}onImageComponent.- Line 482–483:
onDone={handleDone}onProgressBar.For videos, both events can fire near-simultaneously at the end of playback, which means
handleDonecan run twice in parallel. That can cause behavior like:
- Skipping one asset ahead (two calls to
getNextAssets()).- The asset immediately after a video appearing only briefly, since a second transition is triggered almost immediately — this matches feedback about “post-video images” showing shorter than the configured interval.
A simple re-entrancy guard around
handleDonewill prevent this:- let progressBarStatus: ProgressBarStatus = $state(ProgressBarStatus.Playing); - let progressBar: ProgressBar = $state() as ProgressBar; - let imageComponent: ImageComponentInstance = $state() as ImageComponentInstance; - let currentDuration: number = $state($configStore.interval ?? 20); + let progressBarStatus: ProgressBarStatus = $state(ProgressBarStatus.Playing); + let progressBar: ProgressBar = $state() as ProgressBar; + let imageComponent: ImageComponentInstance = $state() as ImageComponentInstance; + let currentDuration: number = $state($configStore.interval ?? 20); + let isTransitioning: boolean = $state(false); - const handleDone = async (previous: boolean = false, instant: boolean = false) => { - progressBar.restart(false); - $instantTransition = instant; - if (previous) await getPreviousAssets(); - else await getNextAssets(); - await tick(); - await imageComponent?.play?.(); - await progressBar.play(); - }; + const handleDone = async (previous: boolean = false, instant: boolean = false) => { + if (isTransitioning) { + return; + } + + isTransitioning = true; + try { + progressBar.restart(false); + $instantTransition = instant; + if (previous) { + await getPreviousAssets(); + } else { + await getNextAssets(); + } + await tick(); + await imageComponent?.play?.(); + await progressBar.play(); + } finally { + isTransitioning = false; + } + };This keeps the existing UX while ensuring only one transition is processed per completion.
Also applies to: 429-431, 482-483
ImmichFrame.Core/Logic/PooledImmichFrameLogic.cs (1)
99-154: Fix malformed MIME type when serving cached imagesIn the cached‑image branch you build the content type as:
var ex = Path.GetExtension(file); return (Path.GetFileName(file), $"image/{ex}", fs);
Path.GetExtensionincludes the leading dot (e.g.,.jpeg), so this yieldsimage/.jpeg, which is not a valid MIME type and may confuse some clients. A minimal fix:- var ex = Path.GetExtension(file); - return (Path.GetFileName(file), $"image/{ex}", fs); + var ext = Path.GetExtension(file).TrimStart('.').ToLowerInvariant(); + var contentType = ext == "webp" ? "image/webp" : "image/jpeg"; + return (Path.GetFileName(file), contentType, fs);This also keeps cached images consistent with the non‑cached branch.
♻️ Duplicate comments (1)
ImmichFrame.Core/Logic/Pool/CachingApiAssetsPool.cs (1)
3-3: Same dependency concern as AllAssetsPool.This file also depends on
ImmichFrame.WebApi.Helpers. See the comment on AllAssetsPool.cs regarding the Core → WebApi.Helpers dependency direction.
🧹 Nitpick comments (11)
immichFrame.Web/src/lib/constants/asset-type.ts (1)
1-11: Confirm AssetType numeric mapping matches backend AssetTypeEnumThese helpers are clean and convenient, but their correctness depends on
IMAGE = 0,VIDEO = 1, etc. staying in sync with the backendAssetTypeEnumvalues from Immich’s OpenAPI schema.It’s worth double‑checking the enum mapping in the generated client/spec and adding a small test (e.g., asserting that at least one known image and one known video asset report the expected
.typevalue) so any future backend change is caught quickly.immichFrame.Web/src/lib/components/home-page/home-page.svelte (4)
95-121:updateAssetPromisesdoesn’t need to beasync
updateAssetPromisesdoesn’t contain anyawaitand only performs synchronous bookkeeping onassetPromisesDict. Marking itasyncjust wraps the return value in a resolvedPromise<void>and can be misleading about its behavior.You can simplify it to a plain function:
- async function updateAssetPromises() { + function updateAssetPromises() { for (let asset of displayingAssets) { if (!(asset.id in assetPromisesDict)) { assetPromisesDict[asset.id] = loadAsset(asset); } } for (let i = 0; i < PRELOAD_ASSETS; i++) { if (i >= assetBacklog.length) { break; } if (!(assetBacklog[i].id in assetPromisesDict)) { assetPromisesDict[assetBacklog[i].id] = loadAsset(assetBacklog[i]); } } for (let key in assetPromisesDict) { if ( !( displayingAssets.find((item) => item.id == key) || assetBacklog.find((item) => item.id == key) ) ) { delete assetPromisesDict[key]; } } }Call sites already ignore the returned promise, so this is a safe cleanup.
135-138: Confirm Immichdurationformat and hardenparseAssetDuration
getAssetDurationSeconds/parseAssetDurationcurrently assume a colon‑separated format (hh:mm[:ss[.fff]]) and return0for anything they can’t parse, which then falls back to$configStore.interval:
- Lines 257–263:
updateCurrentDurationderivescurrentDurationfrom parsed durations.- Lines 265–272:
getAssetDurationSecondsspecial‑cases videos, otherwise uses the configured interval.- Lines 274–298:
parseAssetDurationsplits on:and multiplies by 60 each step, returning0on NaN.Immich’s OpenAPI only documents
durationas astring, without constraining the format. If some assets have a plain numeric string (seconds) or another representation, this parser will silently treat them as “invalid” and revert to the fallback interval, which can make progress bar and actual video length diverge.To make this more robust, you can add a fast path for simple numeric strings before the colon‑based logic:
- function parseAssetDuration(duration?: string | null) { - if (!duration) { - return 0; - } - const parts = duration.split(':').map((value) => value.trim()); - if (!parts.length) { - return 0; - } + function parseAssetDuration(duration?: string | null) { + if (!duration) { + return 0; + } + + // Handle plain seconds (e.g. "37" or "37.5") before colon-based formats. + if (!duration.includes(':')) { + const numeric = parseFloat(duration.replace(',', '.')); + return Number.isNaN(numeric) ? 0 : numeric; + } + + const parts = duration.split(':').map((value) => value.trim()); + if (!parts.length) { + return 0; + } let total = 0; let multiplier = 1; while (parts.length) { const value = parts.pop(); if (!value) { continue; } const normalized = value.replace(',', '.'); const numeric = parseFloat(normalized); if (Number.isNaN(numeric)) { return 0; } total += numeric * multiplier; multiplier *= 60; } return total; }I’d also recommend adding a couple of unit tests around
parseAssetDurationthat use realdurationstrings from your Immich instance (short clip, longer clip, and a failure case) so regressions are caught early.Also applies to: 257-272, 274-298
135-139: Minor readability: reuseisSupportedAssetin backlog filterIn
loadAssets, you manually repeat the image/video checks:assetBacklog = assetRequest.data.filter( (asset) => isImageAsset(asset) || isVideoAsset(asset) );Given you already have
isSupportedAsset, you could make this a bit clearer and more future‑proof:- assetBacklog = assetRequest.data.filter( - (asset) => isImageAsset(asset) || isVideoAsset(asset) - ); + assetBacklog = assetRequest.data.filter(isSupportedAsset);This keeps all knowledge of “what we can render” in a single place.
327-362: Consider revoking object URLs when assets are evicted
loadAssetcreates an object URL for each asset blob:return [getObjectUrl(req.data), assetResponse, album];and
getObjectUrlwrapsURL.createObjectURL. When assets fall out ofdisplayingAssetsandassetBacklog,updateAssetPromisesdeletes their promises fromassetPromisesDict, but the corresponding object URLs are never revoked.This was already a minor leak for images; with video blobs added, the memory impact can grow over long runtimes.
Not urgent for this PR, but as a follow‑up you may want to:
- Track the URL per asset id (e.g. in a separate
assetUrlByIdmap or by resolving the promise before deletion), and- Call
URL.revokeObjectURL(url)insideupdateAssetPromiseswhen you remove an entry that’s no longer in history/backlog.That will keep the frame process footprint more stable during long-running slideshows.
Also applies to: 111-119
ImmichFrame.Core/Logic/Pool/MemoryAssetsPool.cs (1)
20-23: Video gating in MemoryAssetsPool is correct but could be centralizedFiltering
assetstoAssetTypeEnum.IMAGEwhenShowVideosis false is logically correct and avoids extra per-asset API calls for videos. If you’re standardizing on shared helpers likeApplyAccountFilterselsewhere, consider using the same helper here for consistency so account-level rules (videos, excluded albums/people, ratings) all live in one place.ImmichFrame.Core.Tests/Logic/Pool/FavoriteAssetsPoolTests.cs (1)
42-42: Leverage the newtypeparameter to cover ShowVideos=true behaviorThe extended
CreateAssethelper is good, but the tests in this file still only exercise the image-only path (implicitShowVideos == false). Consider adding at least one test that sets_mockAccountSettings.Setup(a => a.ShowVideos).Returns(true);and verifies that:
MetadataSearchDto.Typeis not forced toIMAGE, and- video assets (created via
CreateAsset(id, AssetTypeEnum.VIDEO)) are returned as expected.That will lock in the new behavior and guard against regressions.
ImmichFrame.Core/Logic/PooledImmichFrameLogic.cs (1)
156-177: Align video file extension with actual content type (optional)
GetVideoAssetalways names the file{id}.mp4even if theContent-Typeheader is something else (e.g.,video/webm). That’s probably harmless for streaming but can be misleading if the filename is ever surfaced to clients. Consider deriving the extension fromcontentType(falling back to.mp4when unknown) so the name and MIME type match.ImmichFrame.Core.Tests/Logic/Pool/AlbumAssetsPoolTests.cs (1)
26-26: Consider parameterizing the hardcoded limit.The test helper now calls
GetAssets(25, ct)with a hardcoded limit of 25. While this is acceptable for tests, consider whether this value should be a constant or parameter to make test intent clearer.- public async Task<IEnumerable<AssetResponseDto>> TestLoadAssets(CancellationToken ct = default) => await base.GetAssets(25, ct); + private const int TestAssetLimit = 25; + public async Task<IEnumerable<AssetResponseDto>> TestLoadAssets(CancellationToken ct = default) => await base.GetAssets(TestAssetLimit, ct);ImmichFrame.Core.Tests/Logic/Pool/MemoryAssetsPoolTests.cs (1)
168-198: Simplify test setup by removing unused video memories.Lines 171, 177, and the videoMemoriesCount variable create video memories that are never used because
ShowVideosdefaults to false. These video assets are filtered out byApplyAccountFilters, making the test setup misleading.Apply this diff to remove the unnecessary video memory creation:
[Test] public async Task LoadAssets_AggregatesAssetsFromMultipleMemories() { - var imageMemoriesCount = 2; - var videoMemoriesCount = 2; + var memoryCount = 2; var assetsPerMemory = 2; // Arrange var memoryYear = DateTime.Now.Year - 3; - var memories = CreateSampleImageMemories(imageMemoriesCount, assetsPerMemory, true, memoryYear); // 2 memories, 2 assets each - memories.AddRange(CreateSampleVideoMemories(videoMemoriesCount, assetsPerMemory, true, memoryYear)); // 2 video memories, 2 assets each + var memories = CreateSampleImageMemories(memoryCount, assetsPerMemory, true, memoryYear); // 2 memories, 2 assets each _mockImmichApi.Setup(x => x.SearchMemoriesAsync(It.IsAny<DateTimeOffset>(), null, null, null, It.IsAny<CancellationToken>())) .ReturnsAsync(memories).Verifiable(Times.Once);openApi/swagger.json (1)
211-274: LGTM! New unified asset endpoint properly supports video playback.The new
/api/Asset/{id}/Assetendpoint consolidates image and video retrieval with proper content type support (image/jpeg, image/webp, video/mp4, video/quicktime) and an optionalassetTypeparameter for type-specific retrieval.The path
/api/Asset/{id}/Assethas a slightly redundant naming pattern. Consider/api/Asset/{id}or/api/Asset/{id}/Contentfor a cleaner API surface in a future refactor. However, the current pattern maintains consistency with existing endpoints like/api/Asset/{id}/Imageand/api/Asset/{id}/AssetInfo.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (32)
ImmichFrame.Core.Tests/Logic/Pool/AlbumAssetsPoolTests.cs(2 hunks)ImmichFrame.Core.Tests/Logic/Pool/AllAssetsPoolTests.cs(4 hunks)ImmichFrame.Core.Tests/Logic/Pool/CachingApiAssetsPoolTests.cs(11 hunks)ImmichFrame.Core.Tests/Logic/Pool/FavoriteAssetsPoolTests.cs(1 hunks)ImmichFrame.Core.Tests/Logic/Pool/MemoryAssetsPoolTests.cs(7 hunks)ImmichFrame.Core.Tests/Logic/Pool/PersonAssetsPoolTests.cs(4 hunks)ImmichFrame.Core/Helpers/AssetExtensionMethods.cs(1 hunks)ImmichFrame.Core/Interfaces/IImmichFrameLogic.cs(2 hunks)ImmichFrame.Core/Interfaces/IServerSettings.cs(1 hunks)ImmichFrame.Core/Logic/MultiImmichFrameLogicDelegate.cs(1 hunks)ImmichFrame.Core/Logic/Pool/AlbumAssetsPool.cs(1 hunks)ImmichFrame.Core/Logic/Pool/AllAssetsPool.cs(2 hunks)ImmichFrame.Core/Logic/Pool/CachingApiAssetsPool.cs(1 hunks)ImmichFrame.Core/Logic/Pool/FavoriteAssetsPool.cs(1 hunks)ImmichFrame.Core/Logic/Pool/MemoryAssetsPool.cs(2 hunks)ImmichFrame.Core/Logic/Pool/PeopleAssetsPool.cs(1 hunks)ImmichFrame.Core/Logic/Pool/QueuingAssetPool.cs(2 hunks)ImmichFrame.Core/Logic/PooledImmichFrameLogic.cs(2 hunks)ImmichFrame.WebApi.Tests/Controllers/AssetControllerTests.cs(2 hunks)ImmichFrame.WebApi.Tests/Resources/TestV1.json(1 hunks)ImmichFrame.WebApi.Tests/Resources/TestV2.json(2 hunks)ImmichFrame.WebApi.Tests/Resources/TestV2.yml(2 hunks)ImmichFrame.WebApi.Tests/Resources/TestV2_NoGeneral.json(1 hunks)ImmichFrame.WebApi/Controllers/AssetController.cs(3 hunks)ImmichFrame.WebApi/Helpers/Config/ServerSettingsV1.cs(2 hunks)ImmichFrame.WebApi/Models/ServerSettings.cs(1 hunks)immichFrame.Web/src/lib/components/elements/image-component.svelte(5 hunks)immichFrame.Web/src/lib/components/elements/image.svelte(6 hunks)immichFrame.Web/src/lib/components/home-page/home-page.svelte(18 hunks)immichFrame.Web/src/lib/constants/asset-type.ts(1 hunks)immichFrame.Web/src/lib/immichFrameApi.ts(2 hunks)openApi/swagger.json(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-10-09T16:12:49.488Z
Learnt from: JoeRu
Repo: immichFrame/ImmichFrame PR: 481
File: ImmichFrame.Core.Tests/Logic/Pool/ChronologicalAssetsPoolWrapperTests.cs:306-306
Timestamp: 2025-10-09T16:12:49.488Z
Learning: When testing the ChronologicalAssetsPoolWrapper in ImmichFrame.Core.Tests, use `Is.SupersetOf` rather than `Is.EquivalentTo` or `Is.EqualTo` assertions because the wrapper uses Fisher-Yates shuffle to randomize set order, making output non-deterministic between runs. The wrapper also uses a 10x fetch multiplier (capped at 1000) that may return more assets than requested, which is legitimate behavior.
Applied to files:
ImmichFrame.Core/Logic/Pool/MemoryAssetsPool.csImmichFrame.Core.Tests/Logic/Pool/CachingApiAssetsPoolTests.csImmichFrame.Core/Logic/Pool/FavoriteAssetsPool.csImmichFrame.Core/Logic/PooledImmichFrameLogic.csImmichFrame.WebApi.Tests/Controllers/AssetControllerTests.csImmichFrame.Core.Tests/Logic/Pool/PersonAssetsPoolTests.csImmichFrame.Core.Tests/Logic/Pool/FavoriteAssetsPoolTests.csImmichFrame.Core.Tests/Logic/Pool/MemoryAssetsPoolTests.csImmichFrame.Core/Logic/Pool/AlbumAssetsPool.csImmichFrame.Core/Logic/Pool/PeopleAssetsPool.csImmichFrame.Core/Logic/Pool/CachingApiAssetsPool.csImmichFrame.Core/Helpers/AssetExtensionMethods.csImmichFrame.Core/Logic/Pool/AllAssetsPool.csImmichFrame.Core/Logic/Pool/QueuingAssetPool.csImmichFrame.Core.Tests/Logic/Pool/AllAssetsPoolTests.csImmichFrame.Core.Tests/Logic/Pool/AlbumAssetsPoolTests.cs
📚 Learning: 2025-10-09T16:12:49.488Z
Learnt from: JoeRu
Repo: immichFrame/ImmichFrame PR: 481
File: ImmichFrame.Core.Tests/Logic/Pool/ChronologicalAssetsPoolWrapperTests.cs:306-306
Timestamp: 2025-10-09T16:12:49.488Z
Learning: When testing algorithms with randomization (like Fisher-Yates shuffle), use order-independent assertions such as `Is.SupersetOf` to validate core functionality while maintaining test stability. Order-dependent assertions like `Is.EqualTo` or exact-count assertions like `Is.EquivalentTo` will cause flaky tests when the implementation legitimately produces non-deterministic output.
Applied to files:
ImmichFrame.Core.Tests/Logic/Pool/CachingApiAssetsPoolTests.cs
🔇 Additional comments (47)
ImmichFrame.WebApi.Tests/Resources/TestV2_NoGeneral.json (1)
30-30: Remove unverified claim about test coverage.The original review comment incorrectly asserts that tests exercise the ShowVideos configuration difference. However,
TestLoadConfigV2Json_NoGeneral()only validatesGeneralSettingsand does not callVerifyAccounts()orVerifyConfig(). The asymmetric ShowVideos configuration (Account1 without it, Account2 with it) has no test coverage in this test case.The test file itself is appropriate—the asymmetric configuration is intentional for testing different scenarios. However, the review comment's request to verify test coverage cannot be fulfilled because the relevant test does not exercise the Accounts configuration.
Likely an incorrect or invalid review comment.
ImmichFrame.Core/Logic/Pool/QueuingAssetPool.cs (1)
50-51: No issues found—QueuingAssetPool is confirmed to be unused in production.Verification confirms the TODO comment is accurate:
QueuingAssetPoolis never instantiated in production code. TheBuildPoolmethod only instantiatesAllAssetsPool,FavoriteAssetsPool,MemoryAssetsPool,AlbumAssetsPool,PersonAssetsPool, andMultiAssetPool. No DI registration or production-code references toQueuingAssetPoolexist outside the test suite. The missingShowVideosfilter is not a functional concern for unused code.ImmichFrame.WebApi.Tests/Resources/TestV2.json (1)
47-68: ShowVideos flag correctly wired into JSON test dataThe new
ShowVideosfields for both accounts align with the C#ServerAccountSettings.ShowVideosproperty and give you explicit coverage of the video-enabled path in v2 JSON config tests. No issues from a structure or naming perspective.ImmichFrame.WebApi/Models/ServerSettings.cs (1)
65-81: ServerAccountSettings.ShowVideos added with safe default
ShowVideosis added with a default offalse, which is a good backward-compatible choice for existing configs that don’t include the field. It also matches the casing used in your JSON/YAML resources.ImmichFrame.WebApi.Tests/Resources/TestV2.yml (1)
44-60: YAML test fixture kept consistent with JSON for ShowVideosAdding
ShowVideos: trueto both account entries mirrors the JSON resource and ensures YAML-based tests exercise the video-enabled configuration too. Looks consistent and correct.ImmichFrame.WebApi/Helpers/Config/ServerSettingsV1.cs (1)
15-15: V1 settings and adapter correctly extended with ShowVideosExtending
ServerSettingsV1andAccountSettingsV1AdapterwithShowVideoskeeps the v1 configuration path feature-complete with v2 while preserving a safe default offalsefor older configs. The adapter projection is straightforward and consistent with the other account flags.Also applies to: 68-83
ImmichFrame.Core/Interfaces/IImmichFrameLogic.cs (1)
8-15: GetImage → GetAsset interface change looks soundSwitching to
GetAsset(Guid id, AssetTypeEnum? assetType = null)is a sensible generalization for video support and retains the existing stream return shape. Just ensure all implementations and callers have been updated to the new name/signature (the compiler should catch any stragglers).immichFrame.Web/src/lib/immichFrameApi.ts (1)
224-235: getAssets/getAsset client wiring aligns with new API surfaceThe generated
getAssetsandgetAssethelpers correctly reflect the new/api/Assetlist and/api/Asset/{id}/Assetbinary endpoints, including the optionalassetTypequery parameter. This matches how the front-end now requests image/video blobs.Also applies to: 272-285
ImmichFrame.WebApi.Tests/Resources/TestV1.json (1)
15-15: ShowVideos test flag looks consistentAdding
ShowVideos: truealongside other boolean flags keeps the test config aligned with the new account setting; no issues here.ImmichFrame.Core/Interfaces/IServerSettings.cs (1)
16-16: Interface change requires all implementations to expose ShowVideosAdding
ShowVideostoIAccountSettingsis appropriate next to the otherShow*flags, but it does require every implementation (and mocks) ofIAccountSettingsto be updated. Please confirm all concrete settings classes (and configuration mappings) now populate this property with the intended default.ImmichFrame.Core/Logic/Pool/MemoryAssetsPool.cs (1)
47-49: DailyApiCache expiration at next midnight is fineSetting
AbsoluteExpirationtoDateTimeOffset.Now.Date.AddDays(1)keeps the cache aligned to calendar days; the behavior looks intentional and safe.ImmichFrame.Core/Logic/Pool/FavoriteAssetsPool.cs (1)
26-29: Conditional favorite-type filter matches ShowVideos semanticsOnly forcing
metadataBody.Type = AssetTypeEnum.IMAGEwhenShowVideosis false looks correct and aligns favorites with the per-account video toggle. Please just double‑check that the ImmichSearchAssetsAsyncAPI indeed treats a nullTypeas “all asset types”; if that ever changes, video inclusion here would break silently.ImmichFrame.Core/Logic/Pool/AlbumAssetsPool.cs (1)
18-18: Album pool now relies entirely on upstream filteringReturning
albumAssetsdirectly keeps this pool simple. Given the previous excluded‑album handling, please confirm that album‑level exclusions (fromExcludedAlbums) are now enforced centrally (e.g., via a sharedApplyAccountFiltersstep inCachingApiAssetsPoolor similar) so behavior doesn’t regress for users relying on exclusions.ImmichFrame.Core/Logic/Pool/PeopleAssetsPool.cs (1)
28-31: Person-assets video gating matches the intended semanticsConditionally constraining
metadataBody.TypetoIMAGEwhenShowVideosis false is consistent with the other pools and keeps per-person results aligned with the account’s video preference. Looks good.ImmichFrame.Core/Logic/PooledImmichFrameLogic.cs (1)
76-97: New GetAsset router cleanly separates image and video handlingThe
GetAssetentrypoint looks good: resolvingassetTypeviaGetAssetInfoAsyncwhen missing, then delegating to image/video helpers and guarding unsupported types with a clear exception. Please just ensure that all external callers passing a non‑nullassetTypeare using the real asset type from Immich (not UI guesses), otherwise you can end up calling the wrong handler and surfacing confusing 404s.ImmichFrame.WebApi.Tests/Controllers/AssetControllerTests.cs (1)
193-264: Commented-out test needs attention.The TODO comment indicates this video streaming test requires fixes before activation. Consider tracking this as a follow-up task to ensure video asset retrieval is properly tested.
Would you like me to open a new issue to track completing this test, or is this already covered by issue #502?
ImmichFrame.Core.Tests/Logic/Pool/PersonAssetsPoolTests.cs (1)
44-44: LGTM! Type-aware asset filtering properly tested.The addition of the optional
typeparameter with a sensible default (IMAGE) and the consistent verification of type filtering in mock setups aligns well with the video support feature.Also applies to: 61-84, 103-109
ImmichFrame.Core.Tests/Logic/Pool/AlbumAssetsPoolTests.cs (1)
34-38: LGTM! Proper cache mock setup.The mock setup correctly handles the async factory pattern for
GetOrAddAsync, ensuring the cache behavior is properly exercised in tests.ImmichFrame.Core/Logic/Pool/AllAssetsPool.cs (2)
11-21: LGTM! Video counting logic is correct.The conditional logic properly includes video assets in the count when
ShowVideosis enabled, falling back to images-only otherwise.
32-35: LGTM! Asset type filtering correctly respects ShowVideos setting.The logic appropriately sets
Type = IMAGEonly when videos are disabled, allowing mixed asset types whenShowVideosis true. The use ofApplyAccountFiltersconsolidates filtering logic.Also applies to: 64-66
ImmichFrame.Core.Tests/Logic/Pool/CachingApiAssetsPoolTests.cs (2)
62-62: LGTM! Comprehensive video filtering test coverage.The new tests properly validate video asset filtering behavior under different
ShowVideossettings. The test data includes a video asset, and assertions correctly verify filtering outcomes.Also applies to: 85-100, 134-148, 205-220
164-177: Good improvement: Dictionary-based cache store.Replacing the local cached-value pattern with a dictionary provides more robust simulation of actual caching behavior and better reflects how the cache operates in production.
ImmichFrame.Core/Logic/Pool/CachingApiAssetsPool.cs (2)
23-26: LGTM! Sound two-step caching pattern.Caching excluded album assets separately from the final filtered assets is a good design that allows cache invalidation at the appropriate granularity. The cache keys are properly namespaced.
28-40: LGTM! Excluded assets aggregation is correct.The method properly iterates over excluded albums with null-safe handling and aggregates their assets. The null-coalescing operator on line 32 prevents null reference exceptions.
ImmichFrame.Core/Logic/MultiImmichFrameLogicDelegate.cs (1)
45-46: LGTM! API properly generalized for multi-asset-type support.Renaming
GetImagetoGetAssetwith an optionalassetTypeparameter appropriately reflects the expanded scope to support videos. The delegation correctly passes both parameters through.immichFrame.Web/src/lib/components/elements/image-component.svelte (2)
56-73: LGTM! Clean event handling and control API.The event dispatcher is properly typed, and the exported
pause/playmethods provide a clean API for external control. Null-safe optional chaining ensures the methods work correctly even if components aren't yet mounted.
115-116: LGTM! Proper component binding and event propagation.The
bind:thisdirectives correctly capture component references, and theon:endedhandlers properly propagate media-end events. The pattern handles both single and split-screen modes consistently.Also applies to: 132-133, 151-152
ImmichFrame.Core.Tests/Logic/Pool/MemoryAssetsPoolTests.cs (3)
30-47: LGTM! Asset type parameterization is well implemented.The helper now correctly sets file extensions and asset types based on the provided
AssetTypeEnum, enabling test scenarios for both images and videos.
49-57: LGTM! Type-specific helper methods improve test readability.The specialized helpers provide clear intent when creating image-only or video-only test data.
200-231: LGTM! Video aggregation test correctly validates mixed-asset behavior.The test properly sets
ShowVideos = trueand verifies that both image and video memories are aggregated, expecting the correct total count of 8 assets.ImmichFrame.Core.Tests/Logic/Pool/AllAssetsPoolTests.cs (5)
46-61: LGTM! Test helpers cleanly support multi-type and rating-aware asset creation.The helpers provide clear, type-safe wrappers for creating test assets with specific types and ratings, supporting comprehensive test scenarios.
64-95: LGTM! Asset counting tests correctly validate ShowVideos behavior.The tests properly verify that asset counts include only images when
ShowVideosis false (100 assets) and include both images and videos when enabled (140 assets).
98-156: LGTM! Parameter validation tests correctly verify API request construction.Both tests properly validate that
SearchRandomAsyncis called with the correct parameters:
- When
ShowVideosis false:Type = IMAGEand size reflects only images.- When
ShowVideosis true:Type = null(all types) and size includes both images and videos.
159-171: LGTM! Date filter test correctly validates ImagesFromDays behavior.The test properly verifies that the
TakenAfterparameter is set correctly based onImagesFromDaysconfiguration.
174-197: LGTM! Excluded albums test correctly validates asset filtering.The test properly verifies that assets from excluded albums are filtered out, using the updated image-specific helper for consistency.
openApi/swagger.json (2)
13-13: LGTM! OperationId rename improves API clarity.Renaming
GetAssettoGetAssetsfor the list endpoint accurately reflects that it returns multiple assets, improving API documentation clarity.
207-209: LGTM! Proper deprecation strategy maintains backward compatibility.Marking the
/Imageendpoint as deprecated while keeping it functional allows existing clients to continue working while signaling the need to migrate to the new/Assetendpoint.ImmichFrame.Core/Helpers/AssetExtensionMethods.cs (3)
9-12: LGTM! Supported asset type filtering is appropriate.The method correctly identifies IMAGE and VIDEO as supported asset types, which aligns with the PR's goal of adding video playback support. Other asset types (if any exist in
AssetTypeEnum) are intentionally filtered out.
14-17: LGTM! Async overload provides convenient filtering pipeline support.The async extension method allows seamless integration with async asset retrieval operations.
19-50: Date filtering excludes assets without EXIF DateTimeOriginal; consider whether fallback dates are intended.The filtering logic in lines 33 and 39 uses only
ExifInfo?.DateTimeOriginalwhen applying date range constraints. Verification confirms thatAssetResponseDtohas alternative date properties available—fileCreatedAt,fileModifiedAt, andlocalDateTime—but no fallback logic uses these fields. Assets without EXIF date information will be completely excluded from date-range filtering.This filtering behavior may be intentional (restricting to EXIF-sourced dates only) or may represent an oversight where date filters should fallback to file or computed timestamps. Verify this aligns with intended user behavior before merging.
immichFrame.Web/src/lib/components/elements/image.svelte (4)
2-47: LGTM! Video handling setup is properly structured.The component correctly imports necessary utilities, creates a typed event dispatcher for the
endedevent, derives video status, and maintains a reference to the video element for programmatic control.
52-53: LGTM! Zoom and pan are appropriately disabled for videos.Disabling these visual effects for videos prevents interference with video playback and maintains expected user experience.
137-151: LGTM! Video control methods are well-guarded against edge cases.Both
pause()andplay()properly check for video element existence before calling methods. The try-catch inplay()correctly handles autoplay blocking and other expected failures.
190-211: LGTM! Conditional rendering properly handles video and image assets.The video element is configured correctly for automatic slideshow playback:
autoplayandmutedenable automatic playbackplaysinlineprevents unwanted fullscreen on mobileposterprovides visual continuity while loadingon:endedevent enables progression to the next assetImmichFrame.WebApi/Controllers/AssetController.cs (3)
36-42: LGTM! Endpoint rename improves semantic clarity.Renaming to
GetAssets(plural) accurately reflects that the endpoint returns a list of assets, improving API consistency.
62-84: LGTM! Backward-compatible refactor with proper deprecation.The approach maintains backward compatibility while introducing video support:
- Old
GetImageendpoint delegates to newGetAssetwithIMAGEtype hint- Properly marked with
[Obsolete]attribute- New
GetAssetaccepts optionalassetTypeparameter for performance optimization- Logging includes type hint for debugging
95-95: LGTM! Random image endpoint updated to use new GetAsset method.The call correctly specifies
AssetTypeEnum.IMAGEsince this endpoint is specifically for image retrieval, as indicated by its name.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
ImmichFrame.Core/Logic/Pool/CachingApiAssetsPool.cs (1)
28-40: Same null reference issue and duplication as in AllAssetsPool.cs.Line 36 has the same potential
ArgumentNullExceptionifalbumInfo.Assetsis null. This method is an exact duplicate of the implementation inAllAssetsPool.cs(lines 69-81).Apply the same fix as suggested for
AllAssetsPool.cs:- excludedAlbumAssets.AddRange(albumInfo.Assets); + if (albumInfo.Assets != null) + { + excludedAlbumAssets.AddRange(albumInfo.Assets); + }Consider extracting this duplicated logic to a shared helper as suggested in the review for
AllAssetsPool.cs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
ImmichFrame.Core.Tests/Logic/Pool/AlbumAssetsPoolTests.cs(5 hunks)ImmichFrame.Core/Helpers/AssetExtensionMethods.cs(1 hunks)ImmichFrame.Core/Logic/Pool/AllAssetsPool.cs(3 hunks)ImmichFrame.Core/Logic/Pool/CachingApiAssetsPool.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- ImmichFrame.Core/Helpers/AssetExtensionMethods.cs
- ImmichFrame.Core.Tests/Logic/Pool/AlbumAssetsPoolTests.cs
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-09T16:12:49.488Z
Learnt from: JoeRu
Repo: immichFrame/ImmichFrame PR: 481
File: ImmichFrame.Core.Tests/Logic/Pool/ChronologicalAssetsPoolWrapperTests.cs:306-306
Timestamp: 2025-10-09T16:12:49.488Z
Learning: When testing the ChronologicalAssetsPoolWrapper in ImmichFrame.Core.Tests, use `Is.SupersetOf` rather than `Is.EquivalentTo` or `Is.EqualTo` assertions because the wrapper uses Fisher-Yates shuffle to randomize set order, making output non-deterministic between runs. The wrapper also uses a 10x fetch multiplier (capped at 1000) that may return more assets than requested, which is legitimate behavior.
Applied to files:
ImmichFrame.Core/Logic/Pool/AllAssetsPool.csImmichFrame.Core/Logic/Pool/CachingApiAssetsPool.cs
🧬 Code graph analysis (2)
ImmichFrame.Core/Logic/Pool/AllAssetsPool.cs (2)
ImmichFrame.Core/Helpers/AssetExtensionMethods.cs (2)
Task(13-16)IEnumerable(18-49)ImmichFrame.Core/Helpers/CollectionExtensionMethods.cs (4)
IEnumerable(7-14)IEnumerable(16-17)IEnumerable(19-20)IEnumerable(45-45)
ImmichFrame.Core/Logic/Pool/CachingApiAssetsPool.cs (3)
ImmichFrame.Core/Helpers/AssetExtensionMethods.cs (2)
Task(13-16)IEnumerable(18-49)ImmichFrame.Core/Logic/Pool/AllAssetsPool.cs (3)
Task(9-21)Task(23-67)Task(69-81)ImmichFrame.Core/Helpers/CollectionExtensionMethods.cs (4)
IEnumerable(7-14)IEnumerable(16-17)IEnumerable(19-20)IEnumerable(45-45)
🔇 Additional comments (3)
ImmichFrame.Core/Logic/Pool/AllAssetsPool.cs (2)
9-21: LGTM! Video support in asset counting implemented correctly.The conditional logic properly includes videos in the count when
ShowVideosis enabled, aligning with the PR objectives.
23-67: LGTM! Asset retrieval with centralized filtering looks good.The refactored approach using
ApplyAccountFilterscentralizes the filtering logic and properly handles the video support feature. The conditional type restriction (lines 32-35) correctly allows both images and videos whenShowVideosis enabled.ImmichFrame.Core/Logic/Pool/CachingApiAssetsPool.cs (1)
21-26: LGTM! Smart two-level caching strategy.The approach of caching excluded album assets separately (line 23) and then caching the filtered results (line 25) is efficient. This avoids re-fetching excluded albums on every cache miss for the main asset list.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
ImmichFrame.Core/Logic/Pool/CachingApiAssetsPool.cs (1)
28-43: Extract duplicated method to shared helper.
GetExcludedAlbumAssetsis duplicated inAllAssetsPool.cs(lines 69-84). Extract this to a shared static helper inImmichFrame.Core.Helpers(e.g.,AssetHelper.GetExcludedAlbumAssets) to eliminate duplication and improve maintainability.Apply this refactor:
1. Create a new helper class:
// ImmichFrame.Core/Helpers/AssetHelper.cs namespace ImmichFrame.Core.Helpers; public static class AssetHelper { public static async Task<IEnumerable<AssetResponseDto>> GetExcludedAlbumAssets( ImmichApi immichApi, IAccountSettings accountSettings, CancellationToken ct = default) { var excludedAlbumAssets = new List<AssetResponseDto>(); foreach (var albumId in accountSettings?.ExcludedAlbums ?? new()) { var albumInfo = await immichApi.GetAlbumInfoAsync(albumId, null, null, ct); if (albumInfo.Assets != null) { excludedAlbumAssets.AddRange(albumInfo.Assets); } } return excludedAlbumAssets; } }2. Replace in CachingApiAssetsPool.cs:
- private async Task<IEnumerable<AssetResponseDto>> GetExcludedAlbumAssets(CancellationToken ct = default) - { - var excludedAlbumAssets = new List<AssetResponseDto>(); - - foreach (var albumId in accountSettings?.ExcludedAlbums ?? new()) - { - var albumInfo = await immichApi.GetAlbumInfoAsync(albumId, null, null, ct); - - if (albumInfo.Assets != null) - { - excludedAlbumAssets.AddRange(albumInfo.Assets); - } - } - - return excludedAlbumAssets; - } + private Task<IEnumerable<AssetResponseDto>> GetExcludedAlbumAssets(CancellationToken ct = default) + => AssetHelper.GetExcludedAlbumAssets(immichApi, accountSettings, ct);3. Replace in AllAssetsPool.cs similarly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ImmichFrame.Core/Logic/Pool/AllAssetsPool.cs(3 hunks)ImmichFrame.Core/Logic/Pool/CachingApiAssetsPool.cs(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-09T16:12:49.488Z
Learnt from: JoeRu
Repo: immichFrame/ImmichFrame PR: 481
File: ImmichFrame.Core.Tests/Logic/Pool/ChronologicalAssetsPoolWrapperTests.cs:306-306
Timestamp: 2025-10-09T16:12:49.488Z
Learning: When testing the ChronologicalAssetsPoolWrapper in ImmichFrame.Core.Tests, use `Is.SupersetOf` rather than `Is.EquivalentTo` or `Is.EqualTo` assertions because the wrapper uses Fisher-Yates shuffle to randomize set order, making output non-deterministic between runs. The wrapper also uses a 10x fetch multiplier (capped at 1000) that may return more assets than requested, which is legitimate behavior.
Applied to files:
ImmichFrame.Core/Logic/Pool/CachingApiAssetsPool.csImmichFrame.Core/Logic/Pool/AllAssetsPool.cs
🧬 Code graph analysis (2)
ImmichFrame.Core/Logic/Pool/CachingApiAssetsPool.cs (4)
ImmichFrame.Core/Logic/Pool/AllAssetsPool.cs (3)
Task(9-21)Task(23-67)Task(69-84)ImmichFrame.Core/Helpers/AssetExtensionMethods.cs (2)
Task(13-16)IEnumerable(18-49)ImmichFrame.Core/Logic/Pool/AlbumAssetsPool.cs (2)
Task(8-19)AlbumAssetsPool(7-29)ImmichFrame.Core/Helpers/CollectionExtensionMethods.cs (4)
IEnumerable(7-14)IEnumerable(16-17)IEnumerable(19-20)IEnumerable(45-45)
ImmichFrame.Core/Logic/Pool/AllAssetsPool.cs (3)
ImmichFrame.Core/Logic/Pool/CachingApiAssetsPool.cs (5)
Task(11-14)Task(16-19)Task(21-26)Task(28-43)Task(45-45)ImmichFrame.Core/Helpers/AssetExtensionMethods.cs (2)
Task(13-16)IEnumerable(18-49)ImmichFrame.Core/Helpers/CollectionExtensionMethods.cs (4)
IEnumerable(7-14)IEnumerable(16-17)IEnumerable(19-20)IEnumerable(45-45)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (csharp)
🔇 Additional comments (5)
ImmichFrame.Core/Logic/Pool/CachingApiAssetsPool.cs (2)
2-2: LGTM: Correct dependency direction.The using directive correctly imports from
ImmichFrame.Core.Helpers, resolving the previously flagged upward dependency issue.
21-26: LGTM: Sound two-tier caching strategy.The separate cache keys for excluded album assets and main assets enable independent invalidation, improving cache efficiency when excluded albums change.
ImmichFrame.Core/Logic/Pool/AllAssetsPool.cs (3)
2-2: LGTM!Required for
ApplyAccountFiltersextension method.
32-35: LGTM: Correct conditional video handling.The logic correctly restricts to
IMAGEtype only whenShowVideosis false, allowing both images and videos when enabled.
64-66: LGTM: Consistent filtering flow.The filtering pattern correctly retrieves excluded album assets and applies consolidated account filters, consistent with
CachingApiAssetsPool.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
ImmichFrame.Core/Logic/Pool/AllAssetsPool.cs (1)
64-64: Consider caching excluded album assets.
GetExcludedAlbumAssetsis called on everyGetAssetsinvocation, which may be inefficient if the excluded albums list is large or API responses are slow.CachingApiAssetsPoolcaches this result (line 23 of that file), butAllAssetsPooldoes not.If
GetAssetsis called frequently, consider cachingexcludedAlbumAssetsusing the existingapiCacheinstance.Example approach:
- var excludedAlbumAssets = await AssetHelper.GetExcludedAlbumAssets(immichApi, accountSettings, ct); + var excludedAlbumAssets = await apiCache.GetOrAddAsync( + $"{nameof(AllAssetsPool)}_ExcludedAlbums", + () => AssetHelper.GetExcludedAlbumAssets(immichApi, accountSettings, ct));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
ImmichFrame.Core/Helpers/AssetHelper.cs(1 hunks)ImmichFrame.Core/Logic/Pool/AllAssetsPool.cs(3 hunks)ImmichFrame.Core/Logic/Pool/CachingApiAssetsPool.cs(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-09T16:12:49.488Z
Learnt from: JoeRu
Repo: immichFrame/ImmichFrame PR: 481
File: ImmichFrame.Core.Tests/Logic/Pool/ChronologicalAssetsPoolWrapperTests.cs:306-306
Timestamp: 2025-10-09T16:12:49.488Z
Learning: When testing the ChronologicalAssetsPoolWrapper in ImmichFrame.Core.Tests, use `Is.SupersetOf` rather than `Is.EquivalentTo` or `Is.EqualTo` assertions because the wrapper uses Fisher-Yates shuffle to randomize set order, making output non-deterministic between runs. The wrapper also uses a 10x fetch multiplier (capped at 1000) that may return more assets than requested, which is legitimate behavior.
Applied to files:
ImmichFrame.Core/Logic/Pool/CachingApiAssetsPool.csImmichFrame.Core/Logic/Pool/AllAssetsPool.cs
🧬 Code graph analysis (3)
ImmichFrame.Core/Helpers/AssetHelper.cs (3)
ImmichFrame.Core/Logic/Pool/AllAssetsPool.cs (2)
Task(9-21)Task(23-67)ImmichFrame.Core/Logic/Pool/CachingApiAssetsPool.cs (4)
Task(11-14)Task(16-19)Task(21-26)Task(28-28)ImmichFrame.Core/Logic/Pool/AlbumAssetsPool.cs (2)
AlbumAssetsPool(7-29)Task(9-28)
ImmichFrame.Core/Logic/Pool/CachingApiAssetsPool.cs (4)
ImmichFrame.Core/Helpers/AssetHelper.cs (2)
Task(9-23)AssetHelper(7-24)ImmichFrame.Core/Logic/Pool/AllAssetsPool.cs (2)
Task(9-21)Task(23-67)ImmichFrame.Core/Helpers/AssetExtensionMethods.cs (2)
Task(13-16)IEnumerable(18-49)ImmichFrame.Core/Helpers/CollectionExtensionMethods.cs (4)
IEnumerable(7-14)IEnumerable(16-17)IEnumerable(19-20)IEnumerable(45-45)
ImmichFrame.Core/Logic/Pool/AllAssetsPool.cs (4)
ImmichFrame.Core/Helpers/AssetHelper.cs (2)
Task(9-23)AssetHelper(7-24)ImmichFrame.Core/Logic/Pool/CachingApiAssetsPool.cs (4)
Task(11-14)Task(16-19)Task(21-26)Task(28-28)ImmichFrame.Core/Helpers/AssetExtensionMethods.cs (2)
Task(13-16)IEnumerable(18-49)ImmichFrame.Core/Helpers/CollectionExtensionMethods.cs (4)
IEnumerable(7-14)IEnumerable(16-17)IEnumerable(19-20)IEnumerable(45-45)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (csharp)
🔇 Additional comments (4)
ImmichFrame.Core/Helpers/AssetHelper.cs (1)
7-24: LGTM! Clean extraction of excluded album logic.The helper properly centralizes excluded album asset retrieval, handles null
accountSettingsandAssetscollections safely, and supports cancellation. This eliminates the duplication flagged in previous reviews.ImmichFrame.Core/Logic/Pool/AllAssetsPool.cs (2)
11-20: GetAssetCount now respects the ShowVideos setting.The method correctly returns combined image and video counts when
ShowVideosis enabled, and image-only counts otherwise. Note that other filters (excluded albums, date ranges, rating) are not yet applied, as acknowledged by the TODO comment.
32-35: Video filtering and exclusion logic is correct.The implementation properly restricts
searchDto.TypetoIMAGEwhenShowVideosis false and correctly applies excluded album filters viaAssetHelperandApplyAccountFilters.Also applies to: 64-66
ImmichFrame.Core/Logic/Pool/CachingApiAssetsPool.cs (1)
21-26: LGTM! Efficient caching strategy for excluded albums.Caching
excludedAlbumAssetsseparately under a dedicated key is a smart approach, as it allows independent cache invalidation. The filtering flow is clean and properly integrates the excluded assets intoApplyAccountFilters.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
immichFrame.Web/src/lib/components/home-page/home-page.svelte (1)
144-152: GuardhandleDoneagainst overlapping invocations from progress bar, videoended, and controls
handleDoneis now the central transition handler, called from:
ProgressBarviaonDone={handleDone},ImageComponentviaon:ended={() => handleDone(false, false)},OverlayControlsnext/backactions.Because
handleDoneisasyncand mutatesassetBacklog,assetHistory,displayingAssets, and restarts bothimageComponentandprogressBar, concurrent or rapid-fire triggers (e.g., progress bar finishing while a video firesended, or user clicking “next” at the same time) can cause overlapping transitions, skipped assets, or inconsistent progress/video sync.Consider adding a simple reentrancy guard so only one transition runs at a time:
- const handleDone = async (previous: boolean = false, instant: boolean = false) => { + let isHandlingAssetTransition = false; + + const handleDone = async (previous: boolean = false, instant: boolean = false) => { + if (isHandlingAssetTransition) { + return; + } + isHandlingAssetTransition = true; + try { progressBar.restart(false); $instantTransition = instant; if (previous) await getPreviousAssets(); else await getNextAssets(); await tick(); await imageComponent?.play?.(); await progressBar.play(); - }; + } finally { + isHandlingAssetTransition = false; + } + };This keeps your nice wiring (restart/stop subscriptions,
OverlayControlspause/info toggles,on:ended, and progress baronDone) while preventing race conditions during navigation and playback.Also applies to: 379-382, 386-389, 429-432, 441-458, 460-468, 476-484
♻️ Duplicate comments (1)
ImmichFrame.Core/Logic/Pool/AllAssetsPool.cs (1)
9-21: Filter mismatch between GetAssetCount and GetAssets remains unfixed in AllAssetsPool.GetAssetCount (line 9-21) uses raw API statistics filtered only by ShowVideos, while GetAssets (line 24-69) applies ShowArchived, date ranges, and rating filters. This causes GetAssetCount to return inflated counts that don't match the actual filtered assets returned by GetAssets.
Example: With ShowArchived=false, GetAssetCount includes archived assets in raw stats, but GetAssets excludes them—counts don't match.
The inline TODO comment acknowledges this is temporary, but the discrepancy remains unresolved and can mislead callers like PooledImmichFrameLogic.GetTotalAssets().
🧹 Nitpick comments (7)
immichFrame.Web/src/lib/components/elements/image-component.svelte (1)
67-75: Consider error handling and caller feedback for playback controls.The exported
pause()andplay()functions use optional chaining, which gracefully handles unmounted components but silently fails without feedback to the caller. Additionally, errors from child component methods will propagate uncaught.Consider whether:
- Callers need to know if playback control succeeded
- Errors from child components should be caught and logged or re-thrown with context
Example with error handling:
export const pause = async () => { - await primaryImageComponent?.pause?.(); - await secondaryImageComponent?.pause?.(); + try { + await primaryImageComponent?.pause?.(); + await secondaryImageComponent?.pause?.(); + } catch (error) { + console.error('Failed to pause media:', error); + throw error; + } };ImmichFrame.Core/Logic/Pool/AllAssetsPool.cs (1)
32-68: Consider eliminating redundant filter checks.Filters are applied both server-side (via
searchDtoproperties at lines 32-61) and client-side (viaApplyAccountFiltersat line 68). Specifically:
ShowVideos: filtered viasearchDto.Type(line 34) and re-checked inApplyAccountFiltersShowArchived: filtered viasearchDto.Visibility(lines 37-44) and re-checked inApplyAccountFilters- Date ranges: filtered via
searchDto.TakenBefore/TakenAfter(lines 46-56) and re-checked inApplyAccountFiltersRating: filtered viasearchDto.Rating(lines 58-61) and re-checked inApplyAccountFiltersThe only unique client-side operations are
IsSupportedAsset()and excluded-album filtering. If the server-side API is trusted, consider creating a lighterApplyAccountFiltersvariant that skips redundant checks.ImmichFrame.WebApi/Helpers/Config/ServerSettingsV1.cs (1)
10-57: Legacy V1 settings correctly expose ShowVideos and PlayAudio via adaptersThe additions of
ShowVideosandPlayAudiotoServerSettingsV1, and their exposure through bothAccountSettingsV1AdapterandGeneralSettingsV1Adapter, preserve backward compatibility for the flat config while satisfying the newer interfaces. If you ever need per-accountPlayAudiodistinct from the general value in V1, that would likely warrant a new settings version, but the current approach is reasonable for legacy support.Also applies to: 69-85, 87-123
openApi/swagger.json (1)
211-275: New /api/Asset/{id}/Asset endpoint cleanly models binary image/video retrievalThe
GET /api/Asset/{id}/Assetdefinition—withassetTypereferencingAssetTypeEnumand multiple binary content types for images and videos—matches the intended asset-type-aware retrieval and the generated TSgetAssetfunction. Consider adding a short description clarifying howassetTypeis used (e.g., when it’s required vs inferred) for future readers of the API docs.immichFrame.Web/src/lib/components/home-page/home-page.svelte (3)
95-101: MakeupdateAssetPromisessynchronous (noasync) for clarity
updateAssetPromisescontains noawaitand is always called without being awaited, so marking itasyncis misleading and may confuse future maintainers about whether its completion needs to be sequenced with other work.You can simplify it as:
- async function updateAssetPromises() { + function updateAssetPromises() { for (let asset of displayingAssets) { if (!(asset.id in assetPromisesDict)) { assetPromisesDict[asset.id] = loadAsset(asset); } } // ... for (let key in assetPromisesDict) { if ( !( displayingAssets.find((item) => item.id == key) || assetBacklog.find((item) => item.id == key) ) ) { delete assetPromisesDict[key]; } } }Also applies to: 105-106, 111-120
257-263: Duration parsing andcurrentDurationderivation are reasonable; consider adding testsThe combination of:
parseAssetDurationhandlinghh:mm:ss-style strings (with,or.decimals),getAssetDurationSecondsfalling back to$configStore.intervalwhen the API duration is missing/invalid,updateCurrentDurationtaking the max per-asset duration (and falling back to interval when needed),- binding
ProgressBar’sdurationtocurrentDuration,is a solid approach for keeping the progress bar in sync with either video length or the configured image interval.
Given the subtle parsing and fallback rules, it would be valuable to add unit tests around
parseAssetDuration/getAssetDurationSecondsfor typical cases ('12','1:30','01:02:03.5', malformed values) to lock in behavior and avoid regressions if the backend duration format changes.Also applies to: 265-272, 274-298, 303-305, 312-313, 316-316, 478-478
327-331: Revoke object URLs when assets are evicted to avoid memory leaks
loadAssetcreates an object URL viaURL.createObjectURL(req.data)and stores only the string inassetPromisesDict. When entries are deleted in the cleanup loop, the URLs themselves are never revoked, so a long-running slideshow can accumulate a growing number of unreclaimed blobs.You can track URLs per asset ID and revoke them when removing from
assetPromisesDict:let assetPromisesDict: Record< string, Promise<[string, api.AssetResponseDto, api.AlbumResponseDto[]]> > = {}; +const objectUrls: Record<string, string> = {}; // ... for (let key in assetPromisesDict) { if ( !( displayingAssets.find((item) => item.id == key) || assetBacklog.find((item) => item.id == key) ) ) { - delete assetPromisesDict[key]; + const url = objectUrls[key]; + if (url) { + URL.revokeObjectURL(url); + delete objectUrls[key]; + } + delete assetPromisesDict[key]; } } // ... - return [getObjectUrl(req.data), assetResponse, album] as [ + const objectUrl = getObjectUrl(req.data); + objectUrls[assetResponse.id] = objectUrl; + return [objectUrl, assetResponse, album] as [ string, api.AssetResponseDto, api.AlbumResponseDto[] ];Optionally, you can also revoke any remaining URLs in
onDestroyto be extra safe.Also applies to: 353-357, 360-362, 111-120
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
ImmichFrame.Core/Interfaces/IServerSettings.cs(2 hunks)ImmichFrame.Core/Logic/Pool/AllAssetsPool.cs(3 hunks)ImmichFrame.WebApi.Tests/Resources/TestV1.json(1 hunks)ImmichFrame.WebApi.Tests/Resources/TestV2.json(3 hunks)ImmichFrame.WebApi.Tests/Resources/TestV2.yml(3 hunks)ImmichFrame.WebApi/Helpers/Config/ServerSettingsV1.cs(4 hunks)ImmichFrame.WebApi/Models/ClientSettingsDto.cs(2 hunks)ImmichFrame.WebApi/Models/ServerSettings.cs(2 hunks)immichFrame.Web/src/lib/components/elements/image-component.svelte(6 hunks)immichFrame.Web/src/lib/components/elements/image.svelte(7 hunks)immichFrame.Web/src/lib/components/home-page/home-page.svelte(18 hunks)immichFrame.Web/src/lib/immichFrameApi.ts(3 hunks)openApi/swagger.json(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- immichFrame.Web/src/lib/components/elements/image.svelte
- ImmichFrame.WebApi.Tests/Resources/TestV2.json
- ImmichFrame.Core/Interfaces/IServerSettings.cs
- ImmichFrame.WebApi.Tests/Resources/TestV1.json
- ImmichFrame.WebApi.Tests/Resources/TestV2.yml
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-09T16:12:49.488Z
Learnt from: JoeRu
Repo: immichFrame/ImmichFrame PR: 481
File: ImmichFrame.Core.Tests/Logic/Pool/ChronologicalAssetsPoolWrapperTests.cs:306-306
Timestamp: 2025-10-09T16:12:49.488Z
Learning: When testing the ChronologicalAssetsPoolWrapper in ImmichFrame.Core.Tests, use `Is.SupersetOf` rather than `Is.EquivalentTo` or `Is.EqualTo` assertions because the wrapper uses Fisher-Yates shuffle to randomize set order, making output non-deterministic between runs. The wrapper also uses a 10x fetch multiplier (capped at 1000) that may return more assets than requested, which is legitimate behavior.
Applied to files:
ImmichFrame.Core/Logic/Pool/AllAssetsPool.cs
🧬 Code graph analysis (4)
immichFrame.Web/src/lib/immichFrameApi.ts (2)
ImmichFrame.WebApi/Controllers/AssetController.cs (5)
ApiController(20-121)ImmichFrame(9-122)HttpGet(53-60)HttpGet(77-120)HttpGet(62-75)ImmichFrame.Core/Interfaces/IImmichFrameLogic.cs (1)
IImmichFrameLogic(6-15)
immichFrame.Web/src/lib/components/home-page/home-page.svelte (1)
ImmichFrame.Core/Interfaces/IImmichFrameLogic.cs (1)
IImmichFrameLogic(6-15)
ImmichFrame.Core/Logic/Pool/AllAssetsPool.cs (3)
ImmichFrame.Core/Helpers/AssetHelper.cs (2)
Task(9-23)AssetHelper(7-24)ImmichFrame.Core/Helpers/AssetExtensionMethods.cs (2)
Task(13-16)IEnumerable(18-49)ImmichFrame.Core/Helpers/CollectionExtensionMethods.cs (4)
IEnumerable(7-14)IEnumerable(16-17)IEnumerable(19-20)IEnumerable(45-45)
openApi/swagger.json (2)
ImmichFrame.WebApi/Controllers/AssetController.cs (7)
ApiController(20-121)ImmichFrame(9-122)HttpGet(53-60)HttpGet(62-75)HttpGet(77-120)HttpGet(36-42)HttpGet(44-51)ImmichFrame.Core/Interfaces/IImmichFrameLogic.cs (1)
IImmichFrameLogic(6-15)
🔇 Additional comments (16)
immichFrame.Web/src/lib/components/elements/image-component.svelte (3)
63-65: LGTM!The
handleMediaEndedfunction correctly forwards the ended event from child components to parent consumers, enabling slideshow auto-advance behavior.
32-32: LGTM! Consistent prop and event wiring across all Image instances.The
playAudioprop (defaulting tofalse) and event wiring (bind:this,on:ended) are consistently applied to all Image components in both split and default modes, enabling uniform playback control and event propagation.Also applies to: 51-51, 117-119, 135-137, 155-157
7-7: No issues found. Image component interface verified.The Image component correctly exports
pauseandplayas async functions, dispatches theendedevent with proper typing, and accepts theplayAudioprop. The type import and component wiring inimage-component.svelteare correct.ImmichFrame.Core/Logic/Pool/AllAssetsPool.cs (2)
2-2: LGTM! Dependency direction corrected.The using statement now correctly references
ImmichFrame.Core.Helpersinstead of the previously flaggedImmichFrame.WebApi.Helpers, maintaining proper layering.
64-66: LGTM! Effective caching of excluded album assets.The caching strategy appropriately reduces repeated API calls for excluded albums, and the cache key is sufficiently unique. The null-safety concern from previous reviews has been addressed in
AssetHelper.GetExcludedAlbumAssets.immichFrame.Web/src/lib/immichFrameApi.ts (2)
188-216: ClientSettingsDto.playAudio wiring looks correctThe new
playAudio?: booleanfield is consistent with the server-sideClientSettingsDto.PlayAudioand the OpenAPI schema, and its placement alongsideimageFill/layoutkeeps the client settings payload coherent.
225-236: New asset endpoints mapping (getAssets / getAsset) aligns with OpenAPI
getAssetscorrectly targetsGET /api/Assetand returnsAssetResponseDto[], and the newgetAsset(id, { clientIdentifier, assetType })usesfetchBlobagainstGET /api/Asset/{id}/Assetwith the optionalassetType: AssetTypeEnum, matching the swagger definitions and expected binary responses for images/videos. Just ensure all call sites that previously used the old list-stylegetAssethave been updated to the newgetAssetsname.Also applies to: 273-286
ImmichFrame.WebApi/Models/ClientSettingsDto.cs (1)
31-34: PlayAudio added and mapped cleanly from general settingsThe
PlayAudioproperty and its assignment inFromGeneralSettingsare consistent with the rest of the DTO mapping and with the new client-sideplayAudioflag; no issues here.Also applies to: 35-66
ImmichFrame.WebApi/Models/ServerSettings.cs (1)
54-64: New PlayAudio and ShowVideos settings are well-integratedAdding
PlayAudiotoGeneralSettingsandShowVideostoServerAccountSettings, both defaulting tofalse, fits the existing configuration model and provides a clear opt-in path for audio/video features without changing existing behavior.Also applies to: 66-82
openApi/swagger.json (3)
8-56: Renaming operationId to GetAssets avoids conflict and matches client namingChanging the
GET /api/AssetoperationId toGetAssetsclarifies its plural nature and aligns with the regenerated TypeScriptgetAssetshelper without affecting the on-the-wire API.
164-210: Deprecation of /api/Asset/{id}/Image is properly signaledMarking
GET /api/Asset/{id}/Imageas"deprecated": truewhile keeping its responses unchanged is a clean way to steer new clients toward the asset-aware endpoint without breaking existing consumers.
845-945: ClientSettingsDto schema now correctly exposes playAudioThe
playAudioboolean property in theClientSettingsDtoschema reflects the new server/client capability and aligns with both the C# DTO and the generated TypeScript type; this keeps the contract in sync across layers.immichFrame.Web/src/lib/components/home-page/home-page.svelte (4)
6-9: New imports and playback/preload state look consistentThe additions of
tick, theImageComponentInstancetype,PRELOAD_ASSETS,imageComponent,currentDuration, andassetPromisesDictintegrate cleanly with the existing state pattern and are typed appropriately for the upcoming video support and duration handling.Also applies to: 30-30, 41-42, 55-58
125-125: Asset loading and type filtering align with new asset modelUsing
api.getAssets()and then filtering toisImageAsset(asset) || isVideoAsset(asset)ensures the slideshow only handles supported visual assets while keeping the backend API general. This matches the new asset-type-aware model and should keep unsupported asset types out of the UI.Also applies to: 136-138
169-171: Split view and orientation guards correctly exclude videosUsing
isImageAsset/isVideoAssetingetNextAssets/getPreviousAssetsand short-circuitingisHorizontalfor videos ensures:
- split view only ever shows two images (never videos),
- video assets don’t go through EXIF-based orientation heuristics,
imagesState.splitis true only for pure image pairs.This is a good safety measure for layout, especially with mixed asset types.
Also applies to: 191-192, 204-205, 221-222, 226-228, 312-313
379-382: Playback wiring between stores,ImageComponent, andplayAudiolooks coherentSubscribing to
restartProgress/stopProgressto callimageComponent.play()/.pause(), bindingimageComponentviabind:this, forwardingon:ended, and passingplayAudio={$configStore.playAudio}intoImageComponentall line up well with the new video/audio features. Once the reentrancy guard aroundhandleDoneis in place, this should give predictable synchronized control over both visuals and audio.Also applies to: 386-389, 429-432, 431-431
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docker/Settings.example.yml (1)
37-52: Add ShowVideos to the Accounts section template.Per the PR objectives and your clarification in the comments, the correct configuration is
Accounts: ShowVideos: true. However, this setting is not present in the example configuration template. SinceShowVideosis a per-account setting (unlike the globalPlayAudio), it should be added to the Accounts section alongside other per-account toggles likeShowMemories,ShowFavorites, andShowArchived.Please add
ShowVideos: falseto each account entry in the Accounts section to ensure users have a complete example configuration:Accounts: - ImmichServerUrl: REQUIRED ApiKey: REQUIRED ImagesFromDate: null ShowMemories: false ShowFavorites: false ShowArchived: false + ShowVideos: false ImagesFromDays: null
🧹 Nitpick comments (9)
ImmichFrame.Core.Tests/Logic/Pool/AllAssetsPoolTests.cs (1)
103-131: Consider clarifying variable naming in test setup.The test logic is correct, but the variable
requestedVideoCountat line 108 may be misleading since videos are not actually requested whenShowVideosis false. The variable is used only to populate test data, possibly to verify that videos are properly filtered out even when present. Consider renaming tounusedVideoCountor adding a comment explaining that videos in test data verify filtering behavior.Example:
- var requestedVideoCount = 8; + var unusedVideoCount = 8; // Videos in test data verify they're properly filtered when ShowVideos=false var rating = 3; _mockAccountSettings.SetupGet(s => s.ShowArchived).Returns(true); _mockAccountSettings.SetupGet(s => s.Rating).Returns(3); var returnedAssets = CreateSampleImageAssets(requestedImageCount, rating: rating); - returnedAssets.AddRange(CreateSampleVideoAssets(requestedVideoCount, rating: rating)); + returnedAssets.AddRange(CreateSampleVideoAssets(unusedVideoCount, rating: rating));immichFrame.Web/src/lib/components/elements/image.svelte (3)
26-27: AlignplayAudioprop typing with parent usage and provide a default
image.svelterequiresplayAudio: booleaninPropsand destructures it without a default, whileimage-component.sveltetreatsplayAudioas optional withplayAudio = false. This mismatch can force call sites that useImagedirectly to always provideplayAudio, even if they don’t care about audio.Consider relaxing the prop and adding an explicit default for clarity and resilience:
interface Props { // ... - playAudio: boolean; + playAudio?: boolean; } let { // ... - playAudio + playAudio = false }: Props = $props();This keeps behavior unchanged for callers that already pass
playAudio, while avoiding stricter typing for any existing direct usages ofImage.Also applies to: 40-42
45-48: Video/image branching and zoom/pan disabling look correct; consider minor robustness tweaksThe
isVideo/videoElementwiring andenableZoom/enablePangating ensure that videos render via<video>without zoom/pan animations, while images keep the existing zoom/pan behavior. This is a good separation and avoids awkward Ken Burns–style transforms on video.Two small robustness/maintenance suggestions:
Guard thumbhash decoding and reuse the computed poster URL
You now compute
thumbHashToDataURL(decodeBase64(image[1].thumbhash ?? ''))both for the<video>posterand for the background<img>at the bottom of the file. Ifthumbhashis missing or malformed, this can throw in two places and is harder to adjust centrally.You can centralize and guard the computation:
const thumbhashUrl = $derived(() => { const hash = image[1].thumbhash; if (!hash) return ''; try { return thumbHashToDataURL(decodeBase64(hash)); } catch { return ''; } });Then use
thumbhashUrlin bothposter={thumbhashUrl}and the background<img src={thumbhashUrl}>.Clarify audio/autoplay expectations
muted={!playAudio}is a sensible default, but whenplayAudioistrue, autoplay may be blocked by browsers; theplay()helper already swallows the resulting promise rejection. It may be worth adding a short comment near themuted={!playAudio}line explaining that enabling audio can disable autoplay on some platforms so future maintainers understand this trade‑off.These are optional quality-of-life improvements; the current logic is functionally sound.
Also applies to: 52-53, 161-171, 190-201
137-151: Pause/play exports are safely guarded but could be generalized slightlyThe exported
pause/playhelpers correctly no-op when the current asset is not a video or thevideoElementref is not yet bound, which makes their use from parent components safe.If you expect to support other media types (e.g., audio-only assets) in future, you might consider:
- Relaxing the
isVideocheck to a more generic “has playable media element” abstraction, or- Adding a comment that these helpers are video-specific today to avoid confusion when new asset types are added.
No functional issue, just a small clarity consideration.
immichFrame.Web/src/lib/components/elements/image-component.svelte (1)
59-70: Pause/play delegation across primary/secondary images is sound; consider centralizing shared propsThe introduction of
primaryImageComponent/secondaryImageComponentand the exportedpause/playfunctions cleanly coordinate playback across split and non-split layouts. The use of optional chaining (?.pause?.(),?.play?.()) makes this safe regardless of which layout is active.One small maintainability suggestion: the
<Image>invocations repeat the same long prop list three times, now including{playAudio}andbind:showInfo. You could reduce duplication and the risk of future drift by extracting a small helper component or using a spread object for the shared props, e.g.:<!-- pseudo-code idea --> <Image {...baseImageProps} image={images[0]} bind:this={primaryImageComponent} bind:showInfo />Not required for correctness, but it will make future changes (like adding new shared props) less error-prone.
Also applies to: 101-115, 118-132, 137-151
immichFrame.Web/src/lib/components/home-page/home-page.svelte (4)
30-31: Asset preloading viaassetPromisesDictis well-structured; consider lifecycle cleanupThe new
PRELOAD_ASSETSconstant andassetPromisesDictlogic inupdateAssetPromises()do a good job of:
- Ensuring currently displayed assets and a small backlog are prefetched.
- Avoiding duplicate requests by reusing promises keyed by
asset.id.- Cleaning up entries whose assets are no longer in
displayingAssetsorassetBacklog.One improvement to consider is explicit lifecycle cleanup for the promise dictionary when the component is destroyed to avoid any lingering references:
onDestroy(() => { // existing unsubscribe logic... assetPromisesDict = {}; });This is minor in practice (since the component is long-lived), but it documents the intended lifecycle and prevents surprises if more state is added to the dictionary later.
Also applies to: 55-59, 95-121, 200-201, 231-231
125-139: Filtering to image/video assets is clear; verify behavior for any future asset types
loadAssets()now filtersassetBacklogtoisImageAsset(asset) || isVideoAsset(asset), which is correct for this PR’s video-focused scope and ensures non-displayable asset types are ignored.If the backend introduces additional playable asset types (e.g., audio-only) and you intend to support them in the slideshow, remember to extend this filter accordingly; otherwise those assets will silently never be shown.
144-161: Transition/playback coordination viahandleDoneand subscriptions is solidThe introduction of
isHandlingAssetTransitionplus thehandleDone()flow (restart progress, update assets, awaittick(), then kickimageComponent.play()andprogressBar.play()) provides a clear, serialized transition path and guards against concurrent navigation calls from the progress bar and overlay controls.Similarly, wiring the
restartProgress/stopProgresssubscriptions and the OverlayControlspause/showInfoactions toimageComponent.play()/pause()ensures the video element stays in sync with the progress bar and UI state.The only very minor tweak you might consider is awaiting the
imageComponent?.play?.()call insidehandleDonefor consistency with the overlay callbacks:await imageComponent?.play?.();Functionally this is not required, but it makes the behavior uniform across the different entry points.
Also applies to: 388-399, 449-477
266-307: Dynamic duration handling for videos is good; consider edge cases and documentationThe new
updateCurrentDuration,getAssetDurationSeconds, andparseAssetDurationfunctions, together withduration={currentDuration}onProgressBar, nicely align the progress bar duration with asset metadata:
- For videos, valid
asset.durationstrings (e.g.,HH:MM:SS,MM:SS, with optional fractional seconds and,decimals) are parsed; invalid or non-positive values fall back to$configStore.interval.- For images, you consistently use
$configStore.interval.- For split view, you take the max of the durations for the two assets, which keeps the pair on screen long enough for the longest member.
Two optional hardening/documentation suggestions:
Guard against extremely large or tiny durations
If misconfigured or malformed durations were ever to be very large (hours) or very small (< 1 second), the UI might behave oddly. You could clamp
currentDurationwithin reasonable bounds (e.g., min 1s, max some configured ceiling).Document expected duration format
A brief comment on
parseAssetDuration()describing the accepted formats (HH:MM:SS[.fraction],MM:SS[.fraction],SS[.fraction], allowing,as decimal separator) will help keep backend and frontend in sync if new formats are introduced.The core logic is solid; these are just safety/documentation improvements.
Also applies to: 312-327, 484-487
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
ImmichFrame.Core.Tests/Logic/Pool/AllAssetsPoolTests.cs(4 hunks)docker/Settings.example.json(1 hunks)docker/Settings.example.yml(1 hunks)immichFrame.Web/src/lib/components/elements/image-component.svelte(6 hunks)immichFrame.Web/src/lib/components/elements/image.svelte(7 hunks)immichFrame.Web/src/lib/components/home-page/home-page.svelte(17 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-09T16:12:49.488Z
Learnt from: JoeRu
Repo: immichFrame/ImmichFrame PR: 481
File: ImmichFrame.Core.Tests/Logic/Pool/ChronologicalAssetsPoolWrapperTests.cs:306-306
Timestamp: 2025-10-09T16:12:49.488Z
Learning: When testing the ChronologicalAssetsPoolWrapper in ImmichFrame.Core.Tests, use `Is.SupersetOf` rather than `Is.EquivalentTo` or `Is.EqualTo` assertions because the wrapper uses Fisher-Yates shuffle to randomize set order, making output non-deterministic between runs. The wrapper also uses a 10x fetch multiplier (capped at 1000) that may return more assets than requested, which is legitimate behavior.
Applied to files:
ImmichFrame.Core.Tests/Logic/Pool/AllAssetsPoolTests.cs
🧬 Code graph analysis (2)
docker/Settings.example.json (2)
ImmichFrame.Core/Models/Weather.cs (2)
Weather(5-13)ImmichFrame(3-14)ImmichFrame.Core/Helpers/WeatherExtensions.cs (1)
ImmichFrame(5-24)
ImmichFrame.Core.Tests/Logic/Pool/AllAssetsPoolTests.cs (2)
ImmichFrame.Core/Logic/Pool/AllAssetsPool.cs (4)
Task(9-21)Task(23-69)AllAssetsPool(7-71)AllAssetsPool(6-78)ImmichFrame.Core/Logic/Pool/CachingApiAssetsPool.cs (4)
Task(11-14)Task(16-19)Task(21-26)Task(28-28)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: publish_images
- GitHub Check: Analyze (csharp)
🔇 Additional comments (14)
docker/Settings.example.yml (1)
35-35: ✓ PlayAudio setting correctly added to General section.The addition follows the established conventions: appropriate placement, consistent naming with existing toggles, and safe default value (false).
docker/Settings.example.json (3)
33-33: Formatting consistency fix applied.The WeatherIconUrl value is now properly formatted and aligns with its usage in
WeatherExtensions.cs, whereIconIdis interpolated into the URL template.
40-60: Verify ShowVideos setting is present in Accounts section.Per the PR objectives and user clarification in comments, the configuration key
ShowVideosshould be documented underAccounts(e.g.,Accounts: ShowVideos: true). The provided excerpt shows only a partial Accounts structure; please confirm thatShowVideosis included in the full Accounts configuration example.If
ShowVideosis not currently in the Accounts section ofSettings.example.json, it should be added alongside other account-level filters (ShowMemories, ShowFavorites, ShowArchived) for completeness and user discoverability.
37-37: All PlayAudio integration verified and complete.The setting is properly wired end-to-end:
- Server settings — Defined in IServerSettings interface, ServerSettings model, and ServerSettingsV1 adapter with default
false- ClientSettingsDto mapping — PlayAudio properly mapped at line 62:
dto.PlayAudio = generalSettings.PlayAudio- Frontend consumption — Wired through all layers:
- Config API endpoint (ConfigController.GetConfig) returns mapped ClientSettingsDto
- Frontend config.store wraps ClientSettingsDto
- home-page.svelte passes
$configStore.playAudioto image component- image.svelte uses it to control audio:
muted={!playAudio}The feature flows from docker/Settings.example.json → backend models → API → frontend store → UI component where it actively controls audio playback muting.
ImmichFrame.Core.Tests/Logic/Pool/AllAssetsPoolTests.cs (6)
45-49: LGTM! Cache mock properly supports async asset enumeration.The new
GetOrAddAsyncsetup forIEnumerable<AssetResponseDto>correctly handles the factory pattern and aligns with the production code's use ofAssetHelper.GetExcludedAlbumAssets.
52-67: LGTM! Helper methods properly support asset type and rating.The helper methods correctly create typed assets with optional ratings, enabling comprehensive testing of the new video support and rating filters. The structure with a base method and type-specific wrappers promotes code reuse.
69-101: LGTM! Asset counting tests properly verify video support toggle.Both tests correctly validate that
GetAssetCountreturns only images whenShowVideosis false (default) and includes videos when enabled, matching the production implementation.
133-162: LGTM! Comprehensive test for mixed image and video retrieval.The test correctly verifies that when
ShowVideosis enabled, the search DTO hasType=null(no type filter) and returns both images and videos. The assertions properly validate the total count and search parameters.
164-177: LGTM! Date filter test correctly validates ImagesFromDays.The test properly verifies that
ImagesFromDaysis converted to aTakenAfterdate filter with the correct calculation.
179-203: LGTM! Excluded albums test properly validates filtering logic.The test correctly verifies that assets from excluded albums are filtered out. Good use of
CreateSampleImageAssetsat line 183 to ensure consistent asset typing.immichFrame.Web/src/lib/components/elements/image.svelte (1)
252-271: Updated pan/zoom keyframes look consistent with new scale variablesThe changes to the
panandzoom-pankeyframes to includescale(var(--start-scale))/scale(var(--end-scale))keep the scaling behavior consistent with the standalonezoomanimation, and they respect theenableZoom/enablePanclass gating you added above.No issues spotted here; the transforms compose correctly and should preserve the intended visual effects.
immichFrame.Web/src/lib/components/elements/image-component.svelte (1)
32-33:playAudiodefaulting and wiring through toImageare consistent
playAudiois optional here with a default offalse, and it is threaded through to allImageusages. This ensures the top-level config can control video audio behavior uniformly, without breaking existing callers.Looks good as-is; no changes needed.
Also applies to: 50-52
immichFrame.Web/src/lib/components/home-page/home-page.svelte (2)
174-187: Split-view and orientation logic correctly exclude videos from split layoutUsing
isImageAssetchecks in bothgetNextAssets()andgetPreviousAssets()to decide whether to take two assets for split-view, and treatingisVideoAsset(asset)as immediately non-horizontal inisHorizontal(), ensures:
- Split layout is only used when both assets are images.
- Videos are presented in a single-column layout, which avoids awkward side-by-side video/image combinations.
This is a sensible constraint for a first iteration of video support and matches typical expectations for a slideshow frame.
Also applies to: 209-232, 234-247
41-43: BindingimageComponentand forwardingplayAudiofrom config is coherentThe new
imageComponentref (bind:this={imageComponent}) combined withplayAudio={$configStore.playAudio}cleanly propagates the configuration down to the media layer and enables programmatic control from the home page.This wiring is consistent with the new API surface introduced in
image-component.svelteandimage.svelte. No issues here.Also applies to: 421-440
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
immichFrame.Web/src/lib/components/home-page/home-page.svelte (1)
84-87: AlignprovideClose()with overlayshowInfotoggling to resume video playback
OverlayControls.showInfoand thepausehandler both pause/resume both theimageComponentand theProgressBar. In contrast,provideClose()(used viasetContext('close', provideClose)) only resumes theProgressBar:async function provideClose() { infoVisible = false; await progressBar.play(); }If an info overlay triggers
provideClose()after having paused media, the bar will resume but the video (and audio) may remain paused, which is a subtle UX inconsistency.You can make this consistent with the other paths by also resuming the
imageComponent:async function provideClose() { infoVisible = false; - await progressBar.play(); + await imageComponent?.play?.(); + await progressBar.play(); }This keeps all “close/hide info” flows aligned: whenever the info UI is dismissed, both progress and media playback resume together.
Also applies to: 452-455, 474-492
🧹 Nitpick comments (1)
immichFrame.Web/src/lib/components/home-page/home-page.svelte (1)
95-122: Object URL lifecycle is now handled; consider hardening against rejected asset promisesYou’ve correctly addressed the earlier memory‑leak concern by:
- Ensuring every displayed/preloaded asset has an entry in
assetPromisesDict.- Revoking object URLs when entries fall out of both
displayingAssetsandassetBackloginupdateAssetPromises().- Revoking remaining URLs in
onDestroyand clearingassetPromisesDict.One edge case: if
loadAsset()ever rejects (network/transport error), the stored promise will reject. BothupdateAssetPromises()andonDestroy()currentlyawaitthose promises without atry/catch, which means:
- A single rejected promise can abort the cleanup loop.
Promise.all(revokes)inonDestroycan surface as an unhandled rejection.You can make cleanup more robust by swallowing those rejections while still deleting the entry, e.g.:
- for (let key in assetPromisesDict) { + for (let key in assetPromisesDict) { if ( !( displayingAssets.find((item) => item.id == key) || assetBacklog.find((item) => item.id == key) ) ) { - revokeObjectUrl((await assetPromisesDict[key])[0]); - delete assetPromisesDict[key]; + try { + const [url] = await assetPromisesDict[key]; + revokeObjectUrl(url); + } catch (err) { + console.warn('Failed to resolve asset during cleanup:', err); + } finally { + delete assetPromisesDict[key]; + } } }And similarly for destroy:
- const revokes = Object.values(assetPromisesDict).map(async (p) => - revokeObjectUrl((await p)[0]) - ); - await Promise.all(revokes); + const revokes = Object.values(assetPromisesDict).map(async (p) => { + try { + const [url] = await p; + revokeObjectUrl(url); + } catch (err) { + console.warn('Failed to resolve asset during destroy cleanup:', err); + } + }); + await Promise.allSettled(revokes); assetPromisesDict = {};This keeps the new URL lifecycle behavior while avoiding cleanup being derailed by a transient fetch failure.
Also applies to: 337-343, 350-363, 370-380, 419-433
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
immichFrame.Web/src/lib/components/home-page/home-page.svelte(18 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (csharp)
🔇 Additional comments (8)
immichFrame.Web/src/lib/components/home-page/home-page.svelte (8)
6-6: Imports, type-safeimageComponentbinding, and asset state wiring look consistentUsing
tickandsetContextalongsideonMount/onDestroy, the separateImageComponentInstancetype forbind:this, and theassetPromisesDict/currentDurationstate wiring all line up with how the rest of the file uses them (play/pause calls, duration updates, and promise lookups). I don’t see correctness issues here; this sets up the later logic cleanly.Also applies to: 9-9, 17-18, 30-30, 41-43, 55-58
126-127: Type-aware filtering inloadAssetsis aligned with image/video supportUsing
getAssets()once and then filtering toisImageAsset(asset) || isVideoAsset(asset)before populatingassetBacklogmatches the rest of the code’s expectations (splitview and orientation logic assume only images, while video handling is done viaisVideoAsset). This is a straightforward and correct way to scope what the front-end rotates through.Also applies to: 137-139
145-161: Re-entrancy guard around asset transitions is solid and avoids racey navigationThe
isHandlingAssetTransitionflag combined with thetry/finallyinhandleDone()is a good pattern here. It prevents overlapping calls from the ProgressBaronDoneand the overlay controls (next/back) from interleavinggetNextAssets()/getPreviousAssets()andupdateAssetPromises(), while still guaranteeing the flag is reset even if something throws. Theawait tick()beforeimageComponent?.play?.()also ensures the new asset DOM is ready before resuming playback and progress.Also applies to: 465-472, 499-507
175-185: Restricting splitview to images and treating videos as non-horizontal is coherentThe combination of:
isImageAsset(assetBacklog[0/1])andisImageAsset(assetHistory[...])checks before entering the splitview branch, andsplit: assets.length == 2 && assets.every(isImageAsset)inloadImages, andisHorizontal()early-returningfalseforisVideoAsset(asset)ensures videos never participate in splitview layout or orientation heuristics. That avoids trying to fit videos into layouts designed around EXIF dimensions, which seems like the right tradeoff for this PR.
Also applies to: 200-203, 210-233, 235-247, 322-323
267-308: Duration helpers provide a robust basis for progress timing across images and videos
updateCurrentDuration()+getAssetDurationSeconds()andparseAssetDuration()give you:
- Per-asset durations for videos based on
asset.duration, with a safe fallback to$configStore.interval.- Uniform interval-based durations for images.
- A max-of-durations policy for multi-asset displays.
- A clean reset to the configured fallback when
loadImagesfails.Binding
currentDurationinto<ProgressBar duration={currentDuration} />and updating it on every transition should address the “image after video displays too briefly” behavior without complicating the rest of the flow. The parsing logic (colon-separated or single-value with comma/decimal) also looks defensive enough for typical metadata formats.Also applies to: 313-314, 326-327, 499-502
337-343:loadAssetintegration withassetTypeand object URLs matches the new APIPassing
assetType: assetResponse.typeintoapi.getAssetand wrapping the blob withgetObjectUrl(req.data)aligns this component with the asset-type-aware backend changes. The album/people enrichment and the[url, assetResponse, album]tuple structure are consistent with howloadImagesconsumes these promises. No issues spotted here beyond the general cleanup concerns already mentioned for object URLs.Also applies to: 350-363, 370-371
397-408: Restart/stop subscriptions correctly keep playback and progress in syncThe
restartProgressandstopProgresssubscriptions now not only manipulate theProgressBarbut also callimageComponent?.play?.()/pause?.(). That keeps video (and any timed image animations) aligned with the bar state. Unsubscribing in the destroy hook completes the lifecycle; this looks well-wired.Also applies to: 400-401, 407-408
452-455:playAudioprop threading intoImageComponentPassing
playAudio={$configStore.playAudio}down toImageComponentis the right place to hook audio-enable config into the media layer; it keeps this component focused on layout/selection while the child handles the actual playback semantics.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
immichFrame.Web/src/lib/components/home-page/home-page.svelte (1)
344-375: Empty URL from failed API requests bypasses error handling, causing silent failures.The
loadAssetfunction returns['', assetResponse, []]when API requests fail (line 357) without throwing an exception. This causes:
loadImagescatch block never executes (no exception thrown)imagesState.errorremainsfalseandimagesState.loadedremainstrueImageComponentrenders withsrc=""instead of showing the error state- User sees blank images with no indication of failure
Both split view and normal modes are affected since both destructure
image[0]as the src attribute.Recommended fix: Throw an error in
loadAssetwhen the request fails:if (req.status != 200 || ($configStore.showAlbumName && album == null)) { - return ['', assetResponse, []] as [string, api.AssetResponseDto, api.AlbumResponseDto[]]; + throw new Error(`Failed to load asset ${assetResponse.id}: status ${req.status}`); }This ensures the exception propagates to
loadImagescatch block (line 334), which correctly setserror: trueand displays error feedback to the user.
🧹 Nitpick comments (2)
immichFrame.Web/src/lib/components/elements/image.svelte (1)
148-162: Consider narrowing the error handling in play().The
play()function catches all errors with a generic catch block and silently ignores them. While this is appropriate for handling autoplay blocking, it could also mask legitimate errors such as network failures or corrupted video files. Consider checking the error type or logging it for debugging.export const play = async () => { if (isVideo && videoElement) { try { await videoElement.play(); - } catch { - // Autoplay might be blocked; ignore. + } catch (error) { + // Autoplay might be blocked by browser policy; ignore NotAllowedError + if (error instanceof DOMException && error.name === 'NotAllowedError') { + // Expected when autoplay is blocked + } else { + console.warn('Video playback failed:', error); + } } } };immichFrame.Web/src/lib/components/home-page/home-page.svelte (1)
242-254: Confusing function name:isHorizontalchecks for portrait orientation.The function name suggests it checks if an image is horizontally oriented (landscape), but the logic
imageHeight > imageWidth(line 253) actually checks for portrait orientation (taller than wide).While the logic appears correct for split view (which displays portrait images side by side), the misleading name reduces code clarity.
Consider renaming to better reflect the actual check:
-function isHorizontal(asset: api.AssetResponseDto) { +function isPortraitOrientation(asset: api.AssetResponseDto) { if (isVideoAsset(asset)) { return false; } const isFlipped = (orientation: number) => [5, 6, 7, 8].includes(orientation); let imageHeight = asset.exifInfo?.exifImageHeight ?? 0; let imageWidth = asset.exifInfo?.exifImageWidth ?? 0; if (isFlipped(Number(asset.exifInfo?.orientation ?? 0))) { [imageHeight, imageWidth] = [imageWidth, imageHeight]; } - return imageHeight > imageWidth; // or imageHeight > imageWidth * 1.25; + return imageHeight > imageWidth; // Portrait: taller than wide }Then update all call sites (lines 188, 189, 223, 224).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
immichFrame.Web/src/lib/components/elements/image.svelte(8 hunks)immichFrame.Web/src/lib/components/home-page/home-page.svelte(19 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (csharp)
🔇 Additional comments (14)
immichFrame.Web/src/lib/components/elements/image.svelte (4)
175-176: LGTM: Correct conditional origin calculation.The origin calculations correctly exclude video assets from face-based centering, since zoom and pan effects are disabled for videos. This avoids unnecessary computation and maintains consistent behavior.
262-268: LGTM: Improved transform readability.The keyframe transform declarations have been reformatted for better readability by splitting onto multiple lines. No functional changes.
Also applies to: 273-279
44-44: LGTM: Correct reactive derivations.The
isVideo,enableZoom, andenablePanvalues correctly use$derivedto reactively compute based on the asset type and configuration props. The logic to disable zoom and pan for video assets is appropriate.Also applies to: 51-52
7-7: Given the web search results about Svelte 5 runes, I now have enough information to provide a comprehensive rewritten review comment. The search confirms that $derived should be used to compute values that depend on reactive state and automatically updates whenever its dependencies change.However, I need to verify the actual implementation in the file to be certain about the specific issue. Let me generate a final rewritten comment based on what we've verified so far.
Ensure the
isVideoAssetimport resolves correctly and is used consistently.The import of
isVideoAssetfrom'$lib/constants/asset-type'is correctly implemented. The function checks if the asset type is VIDEO and is properly exported. The usage patterns in the codebase are consistent.However, verify that the overall implementation with the video element follows best practices. Modern browsers require the muted attribute together with the autoplay option to allow autoplay video. Ensure that the
mutedattribute binding is properly connected to theplayAudioprop and that the video element has appropriate error handling and cleanup logic.immichFrame.Web/src/lib/components/home-page/home-page.svelte (10)
6-6: LGTM! Import additions support video playback integration.The new imports (
tick,ImageComponentInstance, asset type guards) and thePRELOAD_ASSETSrename correctly reflect the shift from image-only to multi-asset-type handling.Also applies to: 9-9, 17-17, 30-30
41-42: LGTM! State variables correctly support video playback.The
imageComponentbinding andcurrentDurationstate enable lifecycle coordination, and theassetPromisesDictrename accurately reflects the expanded scope.Also applies to: 55-58
96-129: LGTM! Object URL cleanup correctly implemented.The
updateAssetPromisesfunction now properly revokes object URLs before deleting entries fromassetPromisesDict. The try-catch-finally pattern ensures cleanup happens even when promise resolution fails.
152-169: LGTM! Transition guard prevents race conditions.The
isHandlingAssetTransitionflag with try-finally ensures only one transition happens at a time and the guard is always reset. Thetick()call correctly synchronizes DOM updates before playing.
171-240: LGTM! Split view correctly restricted to image pairs.The
isImageAssetguards in bothgetNextAssetsandgetPreviousAssetsensure that split view is only activated for pairs of images, correctly excluding videos.
274-315: LGTM! Duration parsing handles edge cases correctly.The
parseAssetDurationfunction correctly:
- Parses HH:MM:SS format by processing parts right-to-left
- Handles European decimal notation (comma → dot)
- Returns 0 for invalid input (empty strings, non-numeric values, missing parts)
The
updateCurrentDurationlogic usingMath.maxensures all displayed assets share the longest duration, which is appropriate for synchronized playback.
377-387: LGTM! Object URL helpers correctly implemented.The
getObjectUrlandrevokeObjectUrlhelpers appropriately wrap the browser APIs with error handling, ensuring graceful degradation if revocation fails.
389-445: LGTM! Lifecycle hooks correctly manage resources.The
onMountsubscriptions coordinateimageComponentplayback with progress bar state, and theonDestroycleanup comprehensively revokes all object URLs usingPromise.allSettled, ensuring no memory leaks even on errors.
448-522: LGTM! Template bindings correctly integrate video playback.The template correctly:
- Binds
imageComponentfor lifecycle management- Passes
playAudioconfiguration- Coordinates play/pause across overlay controls
- Uses dynamic
currentDurationfor the progress bar
131-150: Code structure is correct; no issues found.The
getAssets()API call is properly implemented with optional parameters, the return type correctly providesAssetResponseDto[], and the filter functions are correctly implemented:
isImageAssetchecksasset.type === 0(AssetType.IMAGE)isVideoAssetchecksasset.type === 1(AssetType.VIDEO)The filtering logic in
home-page.sveltecorrectly restricts to images and videos. Whether the backend returns video assets depends on backend configuration, not the client code—which is properly structured to handle them.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
immichFrame.Web/src/lib/components/elements/asset-component.svelte (1)
3-3: Remove unused import.The
createEventDispatcherimport is not used anywhere in this component.Apply this diff to remove the unused import:
- import { createEventDispatcher } from 'svelte';immichFrame.Web/src/lib/components/elements/asset.svelte (1)
211-224: Consider video looping and verify autoplay with audio behavior.A few observations:
Autoplay with audio: When
playAudio={true}, the video is unmuted. Most browsers block autoplay with audio unless the user has interacted with the page. The try-catch in theplay()method handles this, but users may experience videos not autoplaying when audio is enabled.Missing loop attribute: Videos currently play once and stop. Consider adding a
loopattribute if continuous playback is desired during the display interval.Poster fallback: When
thumbhashUrlis empty, the poster attribute will be set to an empty string, which is safe but means no placeholder will show while the video loads.Based on the PR comments, user "kdy1968" reported "videos run ~3–5 minutes then stop." This aligns with the missing
loopattribute. User "3rob3" suggested a configuration option to limit playback duration. Consider:
- Adding a
loopattribute to the video element for continuous playback- Or adding a configuration option to control loop behavior and playback duration limits
Do you want me to open an issue to track these enhancements?
immichFrame.Web/src/lib/components/home-page/home-page.svelte (1)
291-315: Consider simplifying duration parsing logic.The duration parsing works correctly but could be more readable. The while loop with
pop()and multiplier accumulation is a bit indirect.Consider this alternative for clarity:
function parseAssetDuration(duration?: string | null) { if (!duration) { return 0; } - const parts = duration.split(':').map((value) => value.trim()); - if (!parts.length) { + const parts = duration.split(':').map((value) => value.trim().replace(',', '.')); + if (parts.length === 0 || parts.length > 3) { return 0; } - let total = 0; - let multiplier = 1; - while (parts.length) { - const value = parts.pop(); - if (!value) { - continue; - } - const normalized = value.replace(',', '.'); - const numeric = parseFloat(normalized); - if (Number.isNaN(numeric)) { - return 0; - } - total += numeric * multiplier; - multiplier *= 60; + // Parse as [hours:]minutes:seconds + const multipliers = [3600, 60, 1]; // hours, minutes, seconds + const offset = multipliers.length - parts.length; + + let total = 0; + for (let i = 0; i < parts.length; i++) { + const numeric = parseFloat(parts[i]); + if (Number.isNaN(numeric)) { + return 0; + } + total += numeric * multipliers[offset + i]; } return total; }This makes the parsing direction explicit and avoids the
pop()loop.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
immichFrame.Web/src/lib/components/elements/asset-component.svelte(9 hunks)immichFrame.Web/src/lib/components/elements/asset.svelte(7 hunks)immichFrame.Web/src/lib/components/home-page/home-page.svelte(19 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (csharp)
🔇 Additional comments (9)
immichFrame.Web/src/lib/components/elements/asset-component.svelte (1)
59-70: LGTM! Playback control delegation is well-structured.The pause/play methods properly delegate to child asset components with safe optional chaining, enabling coordinated playback control across the component hierarchy.
immichFrame.Web/src/lib/components/elements/asset.svelte (3)
46-56: LGTM! Proper video cleanup on unmount.The effect correctly pauses the video and clears the src to release resources when the asset changes or the component unmounts.
64-74: LGTM! Safe thumbhash decoding with fallback.The try-catch properly handles invalid or missing thumbhash data, returning an empty string as a safe fallback.
158-172: LGTM! Autoplay restriction handling is correct.The play method correctly wraps
videoElement.play()in a try-catch to handle cases where autoplay might be blocked by the browser, preventing unhandled promise rejections.immichFrame.Web/src/lib/components/home-page/home-page.svelte (5)
119-126: ✅ Object URL leak fixed!This properly addresses the past review comment about object URL leaks. The cleanup now correctly awaits the promise, extracts the URL, and revokes it before deleting the dictionary entry.
152-169: LGTM! Race condition guard prevents concurrent transitions.The
isHandlingAssetTransitionflag properly serializes asset transitions, and the try-finally ensures the flag is always reset even if an error occurs. The use oftick()correctly waits for the UI to update before starting playback.
426-445: ✅ Object URL cleanup in onDestroy properly implemented!This correctly addresses the past review comment. The
onDestroyhandler now properly revokes all remaining object URLs usingPromise.allSettledto ensure all cleanup attempts complete, even if some fail. The error handling is appropriate.
274-289: Duration calculation correctly handles videos and images.The logic properly:
- Extracts duration from video metadata
- Falls back to configured interval for images or when video duration is unavailable
- Takes the maximum duration when multiple assets are displayed
344-359: Batch-level error handling prevents partial failure recovery; error details are lost.Verification confirms the error handling paths exist but have design limitations:
Partial failures block the batch: If any asset fails in
pickAssets, the entire catch block triggers (line 333), returning{error: true, assets: [], ...}. All successfully loaded assets are discarded, not displayed.Error details are lost:
loadAssetthrows descriptive errors with asset ID and status (line 358), butpickAssets' generic catch swallows these details. Users only see a generic error state; they don't know which asset failed or why.The code functions without crashing, but consider:
- Collecting per-asset results instead of batch-level catch to allow partial display
- Propagating specific error details to the UI error handler
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
immichFrame.Web/src/lib/components/elements/asset-component.svelte (2)
5-7: Prefer plainletforbind:thiscomponent refs instead of$state
primaryAssetComponentandsecondaryAssetComponentare only used asbind:thisrefs and for calling instance exports (pause/play). In Svelte 5 runes mode,bind:thisis intended to target a normal variable (the docs showlet componentInstance: MyComponent; <MyComponent bind:this={componentInstance} />), and all instance exports are then available on that object. Wrapping these refs in$stateadds an unnecessary proxy layer and can make the typing/behavior of the component instance less clear, while not providing any reactive benefit here. Based on Svelte docs.Consider simplifying to something like:
-import type AssetComponent from './asset.svelte'; +import type AssetComponent from './asset.svelte'; - let primaryAssetComponent = $state<AssetComponent | undefined>(undefined); - let secondaryAssetComponent = $state<AssetComponent | undefined>(undefined); + let primaryAssetComponent: AssetComponent | undefined; + let secondaryAssetComponent: AssetComponent | undefined; export const pause = async () => { await primaryAssetComponent?.pause?.(); await secondaryAssetComponent?.pause?.(); }; export const play = async () => { await primaryAssetComponent?.play?.(); await secondaryAssetComponent?.play?.(); };The rest of the script changes (asset tuple typing,
playAudioprop/default, and thepause/playdelegation) look consistent with the new asset/video API.Also applies to: 16-17, 31-32, 35-36, 49-51, 58-69
91-154: Keying and split-mode assumptions aroundassetscould be tightenedThe template wiring to
<Asset>(includingasset={assets[0|1]}, forwardingplayAudio, and bindingshowInfoandbind:this) looks coherent, but there are a couple of details worth tightening:
{#key assets}relies on the array’s reference identity. If upstream logic ever mutates the array in place rather than replacing it, the key won’t change and transitions may not rerun as expected. Keying on something stable like the primary asset’s ID (or a slideshow sequence ID) would be more robust.- In the
splitbranch,assets[1]is used without a length check. If the calling code ever passes fewer than two items whilesplitis true, this will renderundefinedinto<Asset>. If that contract isn’t guaranteed elsewhere, consider guarding or asserting on the array length here.- When
splitis true,playAudiois forwarded to both<Asset>instances. If it’s possible for both sides to be videos and you don’t want dual audio, you may want to gate audio to a single side (e.g., only the primary) or by asset type.None of these are blockers, but clarifying these assumptions now will make the slideshow behavior easier to reason about as video support evolves.
immichFrame.Web/src/lib/components/home-page/home-page.svelte (4)
19-21: Align album tuple typing with possiblenullvalues
loadAsset()initializesalbumasapi.AlbumResponseDto[] | nulland returns it (via a cast) asapi.AlbumResponseDto[], whileAssetsState.assetsandassetPromisesDictalso declare the third tuple element asapi.AlbumResponseDto[]. At runtime this value isnullwhen$configStore.showAlbumNameis false, so the current types don’t match actual values and may hide bugs in consumers that assume a non‑null array.Consider either:
- Allowing
nullin the tuple type, e.g.api.AlbumResponseDto[] | null, or- Always returning an empty array
[]when album names are not requested.This would remove the need for the
as [...]cast and keep the types honest.Also applies to: 55-58, 342-347, 355-372
152-169: Transition guard and play/pause coordination are sensible; verify ProgressBar semanticsThe new
isHandlingAssetTransitionguard inhandleDone()plus the sequencing:
progressBar.restart(false)await getNextAssets()/getPreviousAssets()(which updatescurrentDurationviapickAssets)await tick()await assetComponent?.play?.();progressBar.play();should prevent overlapping transitions and keep playback roughly in sync. The restart in step 1 assumes
ProgressBarreads the effectivedurationonplay()rather than atrestart(), which seems intended but depends on its implementation.If
ProgressBarinstead latchesdurationinsiderestart(), you may want to move therestart(false)call to aftergetNextAssets()/getPreviousAssets()so it sees the updatedcurrentDuration.Also applies to: 402-407, 409-414, 474-483
243-245: ConfirmisHorizontalcondition vs. naming
isHorizontal()returnsfalsefor videos (good for splitview) but then treats an asset as “horizontal” whenassetHeight > assetWidth, which is more characteristic of a portrait/vertical image. Given it’s used to gate splitview layout, this naming/condition pair is a bit surprising.Please double‑check whether the intent is:
- “portrait‑friendly” assets (height > width), in which case renaming to something like
isPortraitwould be clearer, or- truly horizontal/landscape assets, in which case the condition likely wants
assetWidth > assetHeight(or the commented 1.25 ratio).Marking as a semantic check rather than a required change.
Also applies to: 248-249, 251-251, 253-253
274-280: Duration handling is sensible; clarify intended semantics for long videos
updateCurrentDuration()/getAssetDurationSeconds()/parseAssetDuration()together:
- Use parsed video durations when available.
- Fall back to
$configStore.interval ?? 20for images and invalid/absent video durations.- Take the max duration across assets in splitview.
- Bind
currentDurationinto<ProgressBar duration={currentDuration} />.This is a good, predictable strategy. One UX detail to confirm: currently, videos longer than the configured interval will play for their full duration. If the intended behavior is “interval applies only to images”, this is perfect; if the expectation is “videos should not exceed the interval” or “should be capped at some max video duration”, you might want to clamp in
getAssetDurationSeconds()(e.g.,Math.min(parsed, fallback)or a dedicatedmaxVideoDurationconfig).No functional issue, just ensuring the behavior matches the product intent.
Also applies to: 283-289, 291-313, 315-321, 324-324, 327-327, 331-331, 333-333, 511-511
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
immichFrame.Web/src/lib/components/elements/asset-component.svelte(9 hunks)immichFrame.Web/src/lib/components/home-page/home-page.svelte(19 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (csharp)
🔇 Additional comments (1)
immichFrame.Web/src/lib/components/home-page/home-page.svelte (1)
96-102: Object URL lifecycle & cleanup look robustThe combination of
assetPromisesDictpreloading, cleanup inupdateAssetPromises()(revoking URLs for assets no longer indisplayingAssets/assetBacklog), and the final sweep inonDestroycloses the previously noted object URL leak and should keep long‑running frames from accumulating Blob URLs. The try/catch around both cleanup paths also avoids unhandled rejections whenloadAsset()fails.No changes requested here; this is a solid improvement.
Also applies to: 106-109, 112-129, 368-385, 424-443
JW-CH
force-pushed
the
video_playback
branch
from
0df0743 to
8502161
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ImmichFrame.WebApi/Controllers/AssetController.cs (1)
36-42:ToList()never returns null; the null-coalescing throw is unreachable.On line 41,
ToList()will return an empty list if there are no assets, but it will never returnnull. The?? throwis dead code.If the intent is to throw when no assets are found, check for an empty list instead.
🔎 Suggested fix
- return (await _logic.GetAssets()).ToList() ?? throw new AssetNotFoundException("No asset was found"); + var assets = (await _logic.GetAssets()).ToList(); + if (assets.Count == 0) + throw new AssetNotFoundException("No asset was found"); + return assets;
🧹 Nitpick comments (10)
ImmichFrame.Core.Tests/Logic/Pool/MemoryAssetsPoolTests.cs (1)
226-229: Consider adding assertion to verify both asset types are present.The test validates the count is 8, but doesn't verify that both IMAGE and VIDEO types are included in the result. Adding this assertion would make the test more robust by confirming videos are actually returned.
🔎 Suggested improvement:
// Assert Assert.That(loadedAssets, Is.Not.Null); Assert.That(loadedAssets.Count(), Is.EqualTo(8)); // 4 memories * 2 assets + Assert.That(loadedAssets.Any(x => x.Type == AssetTypeEnum.IMAGE), Is.True, "Expected IMAGE assets"); + Assert.That(loadedAssets.Any(x => x.Type == AssetTypeEnum.VIDEO), Is.True, "Expected VIDEO assets"); _mockImmichApi.VerifyAll();ImmichFrame.Core/Helpers/AssetHelper.cs (1)
9-23: Consider parallel fetching and resilient error handling for excluded albums.The current implementation fetches albums sequentially, which could be slow if there are many excluded albums. Additionally, if one album fetch fails, the entire operation fails.
For resilience, consider catching exceptions per album to allow partial success, or use
Task.WhenAllfor parallel fetching if performance becomes a concern.🔎 Optional: Parallel fetch with error resilience
public static async Task<IEnumerable<AssetResponseDto>> GetExcludedAlbumAssets(ImmichApi immichApi, IAccountSettings accountSettings, CancellationToken ct = default) { - var excludedAlbumAssets = new List<AssetResponseDto>(); - - foreach (var albumId in accountSettings?.ExcludedAlbums ?? new()) - { - var albumInfo = await immichApi.GetAlbumInfoAsync(albumId, null, null, ct); - if (albumInfo.Assets != null) - { - excludedAlbumAssets.AddRange(albumInfo.Assets); - } - } - - return excludedAlbumAssets; + var albumIds = accountSettings?.ExcludedAlbums ?? Enumerable.Empty<Guid>(); + + var tasks = albumIds.Select(async albumId => + { + try + { + var albumInfo = await immichApi.GetAlbumInfoAsync(albumId, null, null, ct); + return albumInfo?.Assets ?? Enumerable.Empty<AssetResponseDto>(); + } + catch + { + return Enumerable.Empty<AssetResponseDto>(); + } + }); + + var results = await Task.WhenAll(tasks); + return results.SelectMany(x => x); }ImmichFrame.WebApi.Tests/Controllers/AssetControllerTests.cs (2)
70-80: Consider addingShowVideosto test settings.The
ServerAccountSettingsconfiguration is missing theShowVideosproperty that was introduced as part of this PR for video support. While the current test focuses on image retrieval, adding this property would make the test fixture more complete and ready for video-related tests.🔎 Suggested addition
var accountSettings = new ServerAccountSettings { ImmichServerUrl = "http://mock-immich-server.com", ApiKey = "test-api-key", ShowMemories = false, ShowFavorites = true, ShowArchived = false, + ShowVideos = false, Albums = new List<Guid>(), ExcludedAlbums = new List<Guid>(), People = new List<Guid>() };
193-264: Commented test for video asset streaming.The test scaffolding for video asset retrieval is present but disabled. This is appropriate for work-in-progress functionality.
Would you like help enabling this test once the video streaming path is fully implemented? I can help identify what dependencies or mocking changes might be needed.
ImmichFrame.Core/Logic/PooledImmichFrameLogic.cs (1)
156-177: File extension should match the actual content type.The filename is hardcoded to
.mp4on line 174, but thecontentTypecould bevideo/quicktime(.mov),video/webm, or other formats. This mismatch could cause issues with clients that rely on file extensions for format detection.🔎 Derive extension from content type
if (string.IsNullOrWhiteSpace(contentType)) { contentType = "video/mp4"; } - var fileName = $"{id}.mp4"; + var ext = contentType.ToLower() switch + { + "video/quicktime" => "mov", + "video/webm" => "webm", + "video/x-msvideo" => "avi", + _ => "mp4" + }; + var fileName = $"{id}.{ext}"; return (fileName, contentType, videoResponse.Stream);immichFrame.Web/src/lib/components/elements/asset.svelte (1)
48-56: Video cleanup may not trigger on asset change.This
$effectruns once on mount and returns a cleanup for unmount. However, when theassetprop changes (e.g., slideshow advances to a new video), the component may re-render with a newasset[0]URL, but the old video might not be explicitly cleaned up if the component instance persists.Consider tracking
asset[0]to ensure cleanup runs when the video source changes:🔎 Suggested approach:
$effect(() => { - // Cleanup when image changes or component unmounts + // Track asset URL to cleanup when it changes + const currentUrl = asset[0]; return () => { if (videoElement) { videoElement.pause(); videoElement.src = ''; } }; });openApi/swagger.json (3)
207-209: Add deprecation guidance for deprecated endpoint.The deprecated
/api/Asset/{id}/Imageendpoint should include a description field indicating that clients should migrate to/api/Asset/{id}/Assetand explaining the migration path.🔎 Apply this diff to add deprecation guidance:
"responses": { "200": { "description": "OK", "content": { "image/jpeg": { "schema": { "type": "string", "format": "binary" } }, "image/webp": { "schema": { "type": "string", "format": "binary" } } } } }, - "deprecated": true + "deprecated": true, + "description": "Deprecated: Use GET /api/Asset/{id}/Asset instead. This endpoint will be removed in a future version." }
211-211: Redundant endpoint path.The path
/api/Asset/{id}/Assetis redundant. Consider simplifying to/api/Asset/{id}since the context (retrieving an asset by ID) is already clear from the path structure.
932-934: Add description for the playAudio property.The new
playAudioproperty lacks a description field. Consider adding documentation to clarify its purpose (e.g., "Controls whether audio should be played for video assets" or "Enables audio playback for media with audio tracks").🔎 View suggested addition:
"playAudio": { "type": "boolean" + "description": "Controls whether audio should be played during video playback" },ImmichFrame.Core.Tests/Logic/Pool/CachingApiAssetsPoolTests.cs (1)
115-116: Strengthen the assertion to verify video filtering.The assertion checks for
IMAGE || VIDEOtypes, but sinceShowVideosis not set (defaults to false), onlyIMAGEassets should be returned. The current assertion is too permissive and doesn't verify that videos are actually filtered out.🔎 Apply this diff to strengthen the assertion:
- // All returned assets should be supported media types (image/video) - Assert.That(result.All(a => a.Type == AssetTypeEnum.IMAGE || a.Type == AssetTypeEnum.VIDEO)); + // All returned assets should be images only (videos filtered out when ShowVideos is false) + Assert.That(result.All(a => a.Type == AssetTypeEnum.IMAGE));
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (37)
ImmichFrame.Core.Tests/Logic/Pool/AlbumAssetsPoolTests.cs(5 hunks)ImmichFrame.Core.Tests/Logic/Pool/AllAssetsPoolTests.cs(4 hunks)ImmichFrame.Core.Tests/Logic/Pool/CachingApiAssetsPoolTests.cs(11 hunks)ImmichFrame.Core.Tests/Logic/Pool/FavoriteAssetsPoolTests.cs(1 hunks)ImmichFrame.Core.Tests/Logic/Pool/MemoryAssetsPoolTests.cs(7 hunks)ImmichFrame.Core.Tests/Logic/Pool/PersonAssetsPoolTests.cs(4 hunks)ImmichFrame.Core/Helpers/AssetExtensionMethods.cs(1 hunks)ImmichFrame.Core/Helpers/AssetHelper.cs(1 hunks)ImmichFrame.Core/Interfaces/IImmichFrameLogic.cs(2 hunks)ImmichFrame.Core/Interfaces/IServerSettings.cs(2 hunks)ImmichFrame.Core/Logic/MultiImmichFrameLogicDelegate.cs(1 hunks)ImmichFrame.Core/Logic/Pool/AlbumAssetsPool.cs(1 hunks)ImmichFrame.Core/Logic/Pool/AllAssetsPool.cs(3 hunks)ImmichFrame.Core/Logic/Pool/CachingApiAssetsPool.cs(1 hunks)ImmichFrame.Core/Logic/Pool/FavoriteAssetsPool.cs(1 hunks)ImmichFrame.Core/Logic/Pool/MemoryAssetsPool.cs(2 hunks)ImmichFrame.Core/Logic/Pool/PeopleAssetsPool.cs(1 hunks)ImmichFrame.Core/Logic/Pool/QueuingAssetPool.cs(2 hunks)ImmichFrame.Core/Logic/PooledImmichFrameLogic.cs(2 hunks)ImmichFrame.WebApi.Tests/Controllers/AssetControllerTests.cs(2 hunks)ImmichFrame.WebApi.Tests/Resources/TestV1.json(1 hunks)ImmichFrame.WebApi.Tests/Resources/TestV2.json(3 hunks)ImmichFrame.WebApi.Tests/Resources/TestV2.yml(3 hunks)ImmichFrame.WebApi.Tests/Resources/TestV2_NoGeneral.json(1 hunks)ImmichFrame.WebApi/Controllers/AssetController.cs(3 hunks)ImmichFrame.WebApi/Helpers/Config/ServerSettingsV1.cs(4 hunks)ImmichFrame.WebApi/Models/ClientSettingsDto.cs(2 hunks)ImmichFrame.WebApi/Models/ServerSettings.cs(2 hunks)docker/Settings.example.json(2 hunks)docker/Settings.example.yml(2 hunks)docker/example.env(1 hunks)immichFrame.Web/src/lib/components/elements/asset-component.svelte(9 hunks)immichFrame.Web/src/lib/components/elements/asset.svelte(7 hunks)immichFrame.Web/src/lib/components/home-page/home-page.svelte(19 hunks)immichFrame.Web/src/lib/constants/asset-type.ts(1 hunks)immichFrame.Web/src/lib/immichFrameApi.ts(3 hunks)openApi/swagger.json(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (23)
- ImmichFrame.WebApi.Tests/Resources/TestV2_NoGeneral.json
- immichFrame.Web/src/lib/constants/asset-type.ts
- docker/example.env
- ImmichFrame.WebApi/Models/ClientSettingsDto.cs
- ImmichFrame.WebApi.Tests/Resources/TestV2.yml
- ImmichFrame.Core.Tests/Logic/Pool/AlbumAssetsPoolTests.cs
- ImmichFrame.Core/Interfaces/IServerSettings.cs
- ImmichFrame.Core/Logic/Pool/CachingApiAssetsPool.cs
- ImmichFrame.Core/Helpers/AssetExtensionMethods.cs
- ImmichFrame.WebApi/Models/ServerSettings.cs
- docker/Settings.example.json
- ImmichFrame.Core/Logic/Pool/AllAssetsPool.cs
- ImmichFrame.WebApi.Tests/Resources/TestV2.json
- ImmichFrame.Core/Logic/Pool/MemoryAssetsPool.cs
- ImmichFrame.Core/Logic/Pool/PeopleAssetsPool.cs
- ImmichFrame.Core.Tests/Logic/Pool/PersonAssetsPoolTests.cs
- ImmichFrame.Core.Tests/Logic/Pool/FavoriteAssetsPoolTests.cs
- ImmichFrame.Core.Tests/Logic/Pool/AllAssetsPoolTests.cs
- ImmichFrame.Core/Logic/Pool/AlbumAssetsPool.cs
- ImmichFrame.Core/Logic/Pool/QueuingAssetPool.cs
- docker/Settings.example.yml
- ImmichFrame.Core/Interfaces/IImmichFrameLogic.cs
- ImmichFrame.WebApi.Tests/Resources/TestV1.json
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-10-09T16:12:49.488Z
Learnt from: JoeRu
Repo: immichFrame/ImmichFrame PR: 481
File: ImmichFrame.Core.Tests/Logic/Pool/ChronologicalAssetsPoolWrapperTests.cs:306-306
Timestamp: 2025-10-09T16:12:49.488Z
Learning: When testing the ChronologicalAssetsPoolWrapper in ImmichFrame.Core.Tests, use `Is.SupersetOf` rather than `Is.EquivalentTo` or `Is.EqualTo` assertions because the wrapper uses Fisher-Yates shuffle to randomize set order, making output non-deterministic between runs. The wrapper also uses a 10x fetch multiplier (capped at 1000) that may return more assets than requested, which is legitimate behavior.
Applied to files:
ImmichFrame.Core/Logic/Pool/FavoriteAssetsPool.csImmichFrame.Core.Tests/Logic/Pool/CachingApiAssetsPoolTests.csImmichFrame.WebApi/Controllers/AssetController.csImmichFrame.Core.Tests/Logic/Pool/MemoryAssetsPoolTests.csImmichFrame.WebApi.Tests/Controllers/AssetControllerTests.csImmichFrame.Core/Logic/PooledImmichFrameLogic.cs
📚 Learning: 2025-10-09T16:12:49.488Z
Learnt from: JoeRu
Repo: immichFrame/ImmichFrame PR: 481
File: ImmichFrame.Core.Tests/Logic/Pool/ChronologicalAssetsPoolWrapperTests.cs:306-306
Timestamp: 2025-10-09T16:12:49.488Z
Learning: When testing algorithms with randomization (like Fisher-Yates shuffle), use order-independent assertions such as `Is.SupersetOf` to validate core functionality while maintaining test stability. Order-dependent assertions like `Is.EqualTo` or exact-count assertions like `Is.EquivalentTo` will cause flaky tests when the implementation legitimately produces non-deterministic output.
Applied to files:
ImmichFrame.Core.Tests/Logic/Pool/CachingApiAssetsPoolTests.cs
🧬 Code graph analysis (4)
ImmichFrame.Core.Tests/Logic/Pool/CachingApiAssetsPoolTests.cs (2)
ImmichFrame.Core/Logic/Pool/AllAssetsPool.cs (2)
Task(9-21)Task(23-69)ImmichFrame.Core/Logic/Pool/CachingApiAssetsPool.cs (4)
Task(11-14)Task(16-19)Task(21-26)Task(28-28)
ImmichFrame.WebApi/Controllers/AssetController.cs (4)
ImmichFrame.Core/Logic/PooledImmichFrameLogic.cs (7)
Task(60-63)Task(65-68)Task(70-70)Task(72-72)Task(74-74)Task(76-97)Task(99-154)ImmichFrame.Core/Interfaces/IImmichFrameLogic.cs (9)
Task(8-8)Task(9-9)Task(10-10)Task(11-11)Task(12-12)Task(13-13)Task(14-14)Task(26-26)Task(27-27)ImmichFrame.Core/Api/AssetResponseDto.cs (1)
AssetResponseDto(7-23)ImmichFrame.Core/Helpers/ImmichFrameExtensionMethods.cs (1)
SanitizeString(7-28)
ImmichFrame.Core/Helpers/AssetHelper.cs (2)
ImmichFrame.Core/Logic/Pool/AllAssetsPool.cs (2)
Task(9-21)Task(23-69)ImmichFrame.Core/Logic/Pool/CachingApiAssetsPool.cs (4)
Task(11-14)Task(16-19)Task(21-26)Task(28-28)
ImmichFrame.WebApi.Tests/Controllers/AssetControllerTests.cs (1)
ImmichFrame.WebApi.Tests/Mocks/MockHttpMessageHandler.cs (1)
ImmichFrame(6-29)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (csharp)
🔇 Additional comments (40)
ImmichFrame.Core/Logic/Pool/FavoriteAssetsPool.cs (1)
26-29: LGTM! Clean conditional filtering for video support.The logic correctly restricts favorite assets to images when
ShowVideosis false, while allowing all asset types (including videos) when enabled. This implementation is straightforward and aligns with the PR objective to add video playback support.ImmichFrame.Core.Tests/Logic/Pool/MemoryAssetsPoolTests.cs (3)
30-47: LGTM!The asset type differentiation is well-implemented with appropriate file extensions and type assignments.
49-57: LGTM!Good refactoring with typed helper methods that improve test readability and follow DRY principles.
Minor: Consider adding a blank line after
CreateSampleVideoMemories(line 57) for consistency with the other method separations.
167-198: LGTM!Good test coverage for verifying that video assets are filtered out when
ShowVideosis not enabled. The assertions correctly validate that only IMAGE assets are returned.ImmichFrame.Core/Logic/MultiImmichFrameLogicDelegate.cs (1)
45-46: LGTM!The method correctly delegates asset retrieval with the optional
assetTypeparameter to the underlying logic, maintaining a clean delegation pattern and backward compatibility.ImmichFrame.Core/Logic/PooledImmichFrameLogic.cs (1)
76-97: LGTM!The
GetAssetmethod cleanly routes retrieval based on asset type, with proper null handling and type detection whenassetTypeis not provided. The separation into dedicated handlers for images and videos is a good design choice.ImmichFrame.WebApi/Controllers/AssetController.cs (3)
71-84: LGTM!The new
GetAssetendpoint is well-designed with:
- Proper
Producesattribute covering both image and video MIME types- Optional
assetTypeparameter for explicit type hints- Fire-and-forget webhook notification (appropriate for non-blocking behavior)
- Clean logging with type hint information
62-69: Good backward compatibility strategy.The
[Obsolete]attribute properly signals deprecation while maintaining backward compatibility by delegating to the newGetAssetmethod withAssetTypeEnum.IMAGE.
86-129: LGTM!The
GetRandomImageAndInfoendpoint correctly usesAssetTypeEnum.IMAGEsince theImageResponseformat is designed for base64-encoded images. The implementation is consistent with the response contract.immichFrame.Web/src/lib/immichFrameApi.ts (1)
1-286: Auto-generated API client changes look correct.The file is correctly marked as auto-generated by oazapfts. The new
getAssetfunction properly handles URI encoding and optional query parameters forclientIdentifierandassetType. TheplayAudioaddition toClientSettingsDtoaligns with the video support feature.immichFrame.Web/src/lib/components/elements/asset.svelte (3)
211-224: Video element setup is appropriate for slideshow use.Good handling of:
playsinlinefor mobile compatibilityposterwith thumbhash for visual placeholder during loadmuted={!playAudio}respects user preference- Error logging for debugging
The comment on line 212 correctly notes that autoplay may be blocked when
playAudiois enabled (unmuted). Theplay()export at lines 164-172 handles this gracefully.
158-172: Playback control functions are well-implemented.The
pause()andplay()exports correctly:
- Check both
isVideoandvideoElementbefore acting- Use
asyncappropriately sincevideoElement.play()returns a Promise- Gracefully handle autoplay blocking in
play()with a try/catch
66-74: Good defensive handling for thumbhash decoding.The try/catch ensures malformed or missing thumbhash data doesn't crash the component, falling back to an empty string which is handled gracefully downstream.
immichFrame.Web/src/lib/components/elements/asset-component.svelte (3)
57-69: Playback delegation is correctly implemented.The optional chaining handles both undefined component refs and potentially undefined methods. The async functions properly await both primary and secondary assets, supporting split-view playback control.
91-154: Asset keying and transition handling looks correct.Using
{#key assets}ensures the component tree re-renders on asset changes, which is appropriate for slideshow behavior. The fade transitions provide smooth visual changes between assets.
97-132:andImmichFrame.WebApi/Helpers/Config/ServerSettingsV1.cs (4)
15-15: LGTM: ShowVideos property added correctly.The property is well-placed among other display filter settings with an appropriate default value (false) for this new opt-in feature.
55-55: LGTM: PlayAudio property added correctly.The property addition is syntactically correct with an appropriate default value.
131-131: PlayAudio exposure in GeneralSettings noted.This property exposure is part of the asymmetric pattern already flagged in the review comment for lines 85-86.
85-86: Remove PlayAudio from AccountSettingsV1Adapter.
PlayAudiois exposed inAccountSettingsV1Adapter(line 86) but is not declared in theIAccountSettingsinterface. This breaks the interface contract.PlayAudiois correctly declared only inIGeneralSettingsand should only be exposed viaGeneralSettingsV1Adapter, since it's a general setting rather than an account-specific setting. Remove thePlayAudioproperty fromAccountSettingsV1Adapter.Likely an incorrect or invalid review comment.
openApi/swagger.json (1)
13-13: Breaking change: operationId renamed fromGetAssettoGetAssetson GET /api/Asset endpoint.Renaming an operationId is a breaking change that will rename the corresponding method in the SDK, forcing users to update their code. The endpoint has been properly marked as deprecated, which provides a graceful deprecation path. Ensure all generated clients are updated accordingly.
Likely an incorrect or invalid review comment.
ImmichFrame.Core.Tests/Logic/Pool/CachingApiAssetsPoolTests.cs (9)
62-62: LGTM! Video asset added for comprehensive test coverage.The video asset is appropriately included in the sample data to test video filtering and inclusion scenarios.
85-100: LGTM! Comprehensive test for video inclusion.This test correctly verifies that when
ShowVideosis true, video assets are included in the count alongside images, excluding only archived assets.
134-148: LGTM! Proper test coverage for video inclusion.This test correctly verifies that when
ShowVideosis true, video assets are included in the result set, demonstrating that the filtering logic properly handles the video support feature.
164-177: LGTM! Dictionary-based cache store improves test accuracy.The refactor from a simple cached-value pattern to a dictionary-based cache store better simulates actual caching behavior across multiple keys, ensuring that the test accurately verifies cache functionality.
205-220: LGTM! Comprehensive test for video inclusion with archive filtering.This test correctly verifies that when
ShowVideosis true, video assets are included while archived assets remain filtered out, demonstrating proper interaction between multiple filtering criteria.
239-239: LGTM! Null-conditional operator improves test robustness.Using
ExifInfo?.DateTimeOriginalis a good defensive practice that handles potential null values, making the test more resilient to edge cases.
263-263: LGTM! Consistent use of null-conditional operator.The null-conditional operator usage is consistent across date filtering tests and provides proper null safety.
287-287: LGTM! Proper null safety in date filtering.The null-conditional operator usage maintains consistency and safety across the test suite.
308-308: LGTM! Null-conditional operator handles missing ratings.The null-conditional operator is particularly important here since not all assets (such as videos) may have ratings, making this defensive approach necessary.
immichFrame.Web/src/lib/components/home-page/home-page.svelte (10)
1-58: LGTM! Clean refactoring to asset-centric model.The imports, interface changes, and state declarations properly support the transition from image-only to asset (image + video) handling. The type definitions are consistent and the use of optional chaining for
assetComponentmethods is appropriate.
96-129: Excellent fix for object URL lifecycle management!The addition of object URL revocation (lines 119-127) properly addresses the memory leak concern from the previous review. The try-catch-finally pattern ensures cleanup even when asset resolution fails.
131-150: LGTM! Proper asset filtering for video support.The asset loading correctly filters for both image and video types, and error handling is preserved.
152-169: Excellent race condition prevention!The
isHandlingAssetTransitionguard effectively prevents concurrent transition handling, and the use oftick()ensures the DOM is updated before triggering playback. The finally block guarantees proper flag cleanup.
171-240: Correct splitview asset type validation.The addition of
isImageAssetchecks (lines 186-187, 221-222) properly restricts splitview to images only, which is appropriate since videos need full-screen playback.
274-313: LGTM! Duration parsing logic is correct.The time parsing handles various formats (HH:MM:SS, MM:SS, SS) correctly using appropriate multipliers. Taking the maximum duration across assets ensures sufficient time for all content to display, which is sensible for both single and split views.
315-373: LGTM! Asset loading properly handles mixed content types.The
pickAssetsfunction correctly validates that splitview only occurs with image assets (line 327), andloadAssetappropriately passes the asset type to the API (line 345).
375-385: Good defensive error handling in URL revocation.The
revokeObjectUrlhelper includes appropriate error handling (lines 380-384) to prevent crashes if revocation fails, with useful console warnings for debugging.
387-443: Exemplary cleanup implementation inonDestroy!The lifecycle management properly coordinates asset playback with progress bar state, and the
onDestroyhook (lines 433-442) comprehensively revokes all remaining object URLs usingPromise.allSettledfor robust parallel cleanup. This fully resolves the memory leak concern from the previous review.
446-521: LGTM! Template properly integrates asset playback.The component bindings (line 462), state spreading (line 458), and playback coordination (lines 486, 489, 496, 500) correctly synchronize asset playback with UI controls. The
currentDurationbinding (line 511) properly reflects video lengths in the progress bar.
| // Expected: Asset "1", "2", "4", "5" (Asset "2" is Video, Asset "3" is archived) | ||
| Assert.That(count, Is.EqualTo(3)); |
There was a problem hiding this comment.
Clarify the comment to reflect video filtering.
The comment states "Expected: Asset '1', '2', '4', '5'" but asset "2" is a video and will be filtered out when ShowVideos is false (the default). The actual expected set is assets "1", "4", "5" (3 assets), which matches the assertion.
🔎 Apply this diff to clarify the comment:
- // Expected: Asset "1", "2", "4", "5" (Asset "2" is Video, Asset "3" is archived)
+ // Expected: Asset "1", "4", "5" (Asset "2" is Video and filtered out by default, Asset "3" is archived)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Expected: Asset "1", "2", "4", "5" (Asset "2" is Video, Asset "3" is archived) | |
| Assert.That(count, Is.EqualTo(3)); | |
| // Expected: Asset "1", "4", "5" (Asset "2" is Video and filtered out by default, Asset "3" is archived) | |
| Assert.That(count, Is.EqualTo(3)); |
🤖 Prompt for AI Agents
In ImmichFrame.Core.Tests/Logic/Pool/CachingApiAssetsPoolTests.cs around lines
81 to 82, the inline comment incorrectly lists expected assets as "1", "2", "4",
"5" despite videos being filtered out by default (ShowVideos == false); update
the comment to state the correct expected assets "1", "4", "5" (3 assets) so it
matches the Assert.That(count, Is.EqualTo(3)); keep wording concise and mention
that asset "2" is a video and asset "3" is archived.
| "/api/Asset/{id}/Asset": { | ||
| "get": { | ||
| "tags": [ | ||
| "Asset" | ||
| ], | ||
| "operationId": "GetAsset", | ||
| "parameters": [ | ||
| { | ||
| "name": "id", | ||
| "in": "path", | ||
| "required": true, | ||
| "schema": { | ||
| "type": "string", | ||
| "format": "uuid" | ||
| } | ||
| }, | ||
| { | ||
| "name": "clientIdentifier", | ||
| "in": "query", | ||
| "schema": { | ||
| "type": "string", | ||
| "default": "" | ||
| } | ||
| }, | ||
| { | ||
| "name": "assetType", | ||
| "in": "query", | ||
| "schema": { | ||
| "$ref": "#/components/schemas/AssetTypeEnum" | ||
| } | ||
| } | ||
| ], | ||
| "responses": { | ||
| "200": { | ||
| "description": "OK", | ||
| "content": { | ||
| "image/jpeg": { | ||
| "schema": { | ||
| "type": "string", | ||
| "format": "binary" | ||
| } | ||
| }, | ||
| "image/webp": { | ||
| "schema": { | ||
| "type": "string", | ||
| "format": "binary" | ||
| } | ||
| }, | ||
| "video/mp4": { | ||
| "schema": { | ||
| "type": "string", | ||
| "format": "binary" | ||
| } | ||
| }, | ||
| "video/quicktime": { | ||
| "schema": { | ||
| "type": "string", | ||
| "format": "binary" | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for GetAsset method in controller files
find . -type f \( -name "*.cs" -o -name "*.java" -o -name "*.ts" -o -name "*.js" \) 2>/dev/null | head -20Repository: immichFrame/ImmichFrame
Length of output: 1152
🏁 Script executed:
#!/bin/bash
# Look for controller files that might contain GetAsset
rg -l "GetAsset|Asset.*Controller" --max-count=20 -t cs 2>/dev/null || echo "No C# files found or pattern not matched"
rg -l "GetAsset" 2>/dev/null | head -20 || echo "Pattern not found in repository"Repository: immichFrame/ImmichFrame
Length of output: 2366
🏁 Script executed:
cat -n ImmichFrame.WebApi/Controllers/AssetController.cs | head -150Repository: immichFrame/ImmichFrame
Length of output: 7185
Document assetType parameter role and server-side content determination.
The assetType query parameter is optional but its purpose needs clarification:
assetTypeis a selection hint to filter which asset variant to retrieve (image vs video), not a format negotiator- The server determines the actual response Content-Type based on the stored asset's type, not the Accept header
- Unclear what happens if
assetTypeconflicts with the stored asset type (e.g., assetType=IMAGE for a video asset)
Add:
- A description for
assetType: explain it filters asset type, not response format - Clarify that ContentType is server-determined based on actual asset type
- Document behavior when
assetTypehint doesn't match the stored asset - Examples:
GET /api/Asset/{id}/Asset?assetType=IMAGEreturns the image version; without the parameter, the server returns the stored asset as-is
🤖 Prompt for AI Agents
In openApi/swagger.json around lines 211 to 274, the assetType query parameter
lacks a descriptive purpose and server behavior; update the parameter's
description to state it is an optional selection hint used to filter which asset
variant to retrieve (e.g., IMAGE vs VIDEO) and not a format negotiator,
explicitly state that the server will determine the actual response Content-Type
from the stored asset type (ignoring the Accept header for format negotiation),
and document the behavior when the provided assetType does not match the stored
asset (e.g., return the stored asset with a 200 and its actual Content-Type, or
return a 4xx if the hint is incompatible depending on server policy), then add
brief examples such as GET /api/Asset/{id}/Asset?assetType=IMAGE returns the
image variant when available and GET /api/Asset/{id}/Asset with no assetType
returns the stored asset as-is.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
immichFrame.Web/src/lib/index.ts (1)
24-30: LGTM! Clean URL builder implementation.The function correctly constructs the asset streaming URL with proper encoding and query parameter handling. The use of
!== undefinedforassetTypeis correct, allowing0as a valid value.Optional: Consider adding JSDoc for API documentation
Adding documentation would help future maintainers understand the function's purpose and parameters:
+/** + * Constructs a URL for streaming an asset (image or video). + * @param id - The asset identifier + * @param clientIdentifier - Optional client identifier for tracking + * @param assetType - Optional asset type (0 for image, 1 for video, etc.) + * @returns The constructed streaming URL + */ export const getAssetStreamUrl = (id: string, clientIdentifier?: string, assetType?: number) => {ImmichFrame.WebApi/Models/ImageRequestedNotification.cs (1)
12-18: Rename constructor parameter for consistency.The constructor parameter is still named
imageIdbut should be renamed toassetIdto align with the asset-centric refactor and theRequestedAssetIdproperty it initializes.🔎 Proposed fix
- public AssetRequestedNotification(Guid imageId, string clientIdentifier) + public AssetRequestedNotification(Guid assetId, string clientIdentifier) { Name = nameof(AssetRequestedNotification); ClientIdentifier = clientIdentifier; DateTime = DateTime.Now; - RequestedAssetId = imageId; + RequestedAssetId = assetId; }immichFrame.Web/src/lib/components/elements/asset.svelte (1)
159-173: Consider adding defensive error handling to pause().While
pause()is less likely to throw errors thanplay(), wrapping it in a try/catch would provide more robust error handling for edge cases where the video element might be in an invalid state.🔎 Proposed fix
export const pause = async () => { if (isVideo && videoElement) { - videoElement.pause(); + try { + videoElement.pause(); + } catch { + // Ignore any errors during pause + } } };ImmichFrame.WebApi/Controllers/AssetController.cs (1)
106-109: Consider async read for thumbhash stream.The thumbhash stream is read synchronously, which could potentially block. Consider using
ReadAsyncfor consistency with the async pattern used elsewhere.🔎 Proposed fix
randomAsset.ThumbhashImage!.Position = 0; byte[] byteArray = new byte[randomAsset.ThumbhashImage.Length]; -randomAsset.ThumbhashImage.Read(byteArray, 0, byteArray.Length); +await randomAsset.ThumbhashImage.ReadAsync(byteArray, 0, byteArray.Length); string thumbHashBase64 = Convert.ToBase64String(byteArray);immichFrame.Web/src/lib/components/home-page/home-page.svelte (1)
277-299: Consider using global comma replacement for robustness.The duration parsing logic at line 281 uses
.replace(',', '.')which only replaces the first comma in each part. While valid duration formats typically have at most one decimal comma per component (e.g.,"1:23,45"), using a global replace would be more defensive against malformed input.🔎 Proposed refinement
- const parts = duration.split(':').map((value) => value.trim().replace(',', '.')); + const parts = duration.split(':').map((value) => value.trim().replace(/,/g, '.'));
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
ImmichFrame.WebApi/Controllers/AssetController.csImmichFrame.WebApi/Models/ImageRequestedNotification.csdocs/docs/getting-started/configuration.mddocs/docs/getting-started/configurationV1.mdimmichFrame.Web/src/lib/components/elements/asset.svelteimmichFrame.Web/src/lib/components/home-page/home-page.svelteimmichFrame.Web/src/lib/index.ts
✅ Files skipped from review due to trivial changes (1)
- docs/docs/getting-started/configuration.md
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-09T16:12:49.488Z
Learnt from: JoeRu
Repo: immichFrame/ImmichFrame PR: 481
File: ImmichFrame.Core.Tests/Logic/Pool/ChronologicalAssetsPoolWrapperTests.cs:306-306
Timestamp: 2025-10-09T16:12:49.488Z
Learning: When testing the ChronologicalAssetsPoolWrapper in ImmichFrame.Core.Tests, use `Is.SupersetOf` rather than `Is.EquivalentTo` or `Is.EqualTo` assertions because the wrapper uses Fisher-Yates shuffle to randomize set order, making output non-deterministic between runs. The wrapper also uses a 10x fetch multiplier (capped at 1000) that may return more assets than requested, which is legitimate behavior.
Applied to files:
ImmichFrame.WebApi/Controllers/AssetController.cs
🪛 LanguageTool
docs/docs/getting-started/configurationV1.md
[grammar] ~34-~34: Use a hyphen to join words.
Context: ... | 2 digit ISO code, sets the language of the...
(QB_NEW_EN_HYPHEN)
[grammar] ~41-~41: Ensure spelling is correct
Context: ... | A list of webcalendar URIs in the .ics format. e.g. https://c...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~43-~43: Use a hyphen to join words.
Context: ... | Displays a comma separated list of names of all the peopl...
(QB_NEW_EN_HYPHEN)
[style] ~43-~43: Consider using “who” when you are referring to people instead of objects.
Context: ...parated list of names of all the people that are assigned in immich. ...
(THAT_WHO)
[grammar] ~44-~44: Use a hyphen to join words.
Context: ... | Displays a comma separated list of names of all the album...
(QB_NEW_EN_HYPHEN)
🪛 markdownlint-cli2 (0.18.1)
docs/docs/getting-started/configurationV1.md
37-37: Bare URL used
(MD034, no-bare-urls)
41-41: Bare URL used
(MD034, no-bare-urls)
48-48: Link text should be descriptive
(MD059, descriptive-link-text)
54-54: Bare URL used
(MD034, no-bare-urls)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (csharp)
🔇 Additional comments (10)
immichFrame.Web/src/lib/components/elements/asset.svelte (1)
48-57: LGTM: Video cleanup prevents memory leaks.The cleanup effect correctly pauses the video and clears the source when the asset changes or the component unmounts, preventing potential memory leaks from video elements.
ImmichFrame.WebApi/Controllers/AssetController.cs (1)
71-84: LGTM: Range processing enabled for video streaming.The endpoint correctly enables range processing (
enableRangeProcessing: true), which is essential for video streaming support via HTTP range requests. The async notification pattern is also well-implemented.immichFrame.Web/src/lib/components/home-page/home-page.svelte (8)
1-25: LGTM! Clean refactor to asset-centric types and imports.The imports and type definitions correctly reflect the shift from image-only to multi-asset support (images + videos). The addition of
tickfor UI synchronization andAssetComponentInstancefor component binding are appropriate.
260-275: Good duration calculation strategy.The
updateCurrentDurationfunction correctly computes the display duration by taking the maximum of all asset durations. This allows videos to play to completion while respecting the configured interval for images—a sensible approach for mixed content.
328-374: Excellent architectural decision: streaming videos vs. preloading images.The differentiation between streaming videos directly (lines 331-337) and preloading images as blobs (lines 339-348) is the right approach. This prevents memory issues with large video files while maintaining instant display for images.
380-388: Well-designed URL revocation with proper guards.The
blob:prefix check (line 382) is critical and correct—it ensures that only blob URLs created for preloaded images are revoked, while video streaming URLs remain untouched. This prevents potential issues with video playback.
152-169: Excellent use of transition guard to prevent race conditions.The
isHandlingAssetTransitionflag with afinallyblock ensures that concurrent transition requests are properly serialized and the guard is always released—even if an error occurs during the transition. The addition ofawait tick()at line 163 ensures the DOM is fully updated before playing the next asset.
231-240: Splitview correctly restricted to portrait images only.The
shouldUseSplitViewfunction appropriately excludes videos from splitview mode (lines 235-236). This makes sense because videos require full-screen display for proper playback controls and user experience. The additional portrait orientation check ensures optimal layout.
454-468: Component bindings correctly implement asset playback.The
AssetComponentbindings properly pass through the dynamically calculatedcurrentDuration(line 456) and the newplayAudioconfiguration (line 467). The component instance binding (line 465) enables coordinated play/pause control across the UI.
477-510: Proper synchronization between progress bar and asset playback.The overlay control handlers correctly coordinate play/pause state between the
ProgressBarandAssetComponent(lines 489-493, 499-504). The consistent use of optional chaining (assetComponent?.play?.()) safely handles cases where the component reference may not yet be established.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
immichFrame.Web/src/lib/components/home-page/home-page.svelte (1)
280-302: Duration parsing could handle edge cases more defensively.The
parseAssetDurationfunction handles most malformed inputs gracefully by returning 0 (which triggers fallback to the configured interval). However, it doesn't reject negative values:const numeric = parseFloat(parts[i]); if (Number.isNaN(numeric)) { return 0; } // Missing: if (numeric < 0) return 0;For video durations, negative values are nonsensical and should be treated as invalid.
🔎 Optional: Add validation for negative values
const numeric = parseFloat(parts[i]); -if (Number.isNaN(numeric)) { +if (Number.isNaN(numeric) || numeric < 0) { return 0; }immichFrame.Web/src/lib/components/elements/asset.svelte (1)
52-61: Effect cleanup works but could be simplified.The cleanup correctly pauses the video and clears the src when the asset changes. However, clearing
videoElement.src = ''(line 58) might be unnecessary since the template will immediately overwrite it with the newasset[0]. Simply pausing the video should suffice for stopping playback and releasing resources.🔎 Simplified cleanup
$effect(() => { // Track asset URL to cleanup when it changes asset[0]; return () => { if (videoElement) { videoElement.pause(); - videoElement.src = ''; } }; });
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
immichFrame.Web/src/lib/components/elements/asset-component.svelteimmichFrame.Web/src/lib/components/elements/asset.svelteimmichFrame.Web/src/lib/components/home-page/home-page.svelte
🚧 Files skipped from review as they are similar to previous changes (1)
- immichFrame.Web/src/lib/components/elements/asset-component.svelte
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (csharp)
🔇 Additional comments (18)
immichFrame.Web/src/lib/components/home-page/home-page.svelte (11)
6-9: LGTM: Clean transition to asset-centric imports.The addition of
tickfor UI synchronization,AssetComponentfor unified rendering, and asset-type utilities appropriately supports the video playback feature.Also applies to: 17-17
41-43: LGTM: State variables support video playback coordination.The
assetComponentbinding, dynamiccurrentDuration, anduserPausedflag enable proper coordination between video playback, progress bar, and user interactions.
98-131: LGTM: Object URL cleanup properly implemented.The cleanup logic correctly revokes object URLs before removing entries from
assetPromisesDict, with proper error handling to ensure dictionary cleanup even if promise resolution fails.
146-148: LGTM: Asset filtering includes videos.The filter correctly includes both image and video assets using the new asset-type utilities.
154-172: LGTM: Robust transition handling with proper synchronization.The
isHandlingAssetTransitionguard prevents race conditions during rapid navigation, andawait tick()ensures the UI is ready before resuming playback. TheuserPausedreset correctly differentiates manual pause from auto-advance.
220-243: LGTM: Split view logic correctly excludes videos.The
isPortraitfunction (renamed from the confusingisHorizontalper previous review) andshouldUseSplitViewcorrectly restrict split view to portrait image pairs, appropriately excluding videos.
334-351: LGTM: Efficient asset loading strategy.Videos use streaming URLs to avoid memory overhead, while images are preloaded as blobs for instant display. This is the correct approach for handling mixed media types.
383-391: LGTM: Safe object URL revocation.The
revokeObjectUrlfunction correctly checks for blob URLs before revoking (to avoid attempting to revoke streaming URLs) and includes error handling for safety.
430-449: LGTM: Comprehensive cleanup in onDestroy.The cleanup logic properly revokes all remaining object URLs using
Promise.allSettledto ensure all cleanup attempts complete regardless of individual failures, then clears the dictionary.
471-478: LGTM: Video event handlers respect user pause state.The
onVideoWaitingandonVideoPlayinghandlers correctly manage the progress bar during video buffering, with properuserPausedchecks to prevent auto-resume after manual pause.
488-525: LGTM: Overlay controls properly coordinate playback state.The pause/play and info toggle handlers consistently manage the
userPausedflag and coordinate bothassetComponentandprogressBarstate, ensuring manual pauses are respected during video buffering.immichFrame.Web/src/lib/components/elements/asset.svelte (7)
1-28: LGTM: Props refactored to support video assets.The props interface correctly transitions from image-centric to asset-centric data, adds
playAudiofor audio control, and includes video lifecycle callbacks (onVideoWaiting,onVideoPlaying) for parent coordination.
66-67: LGTM: Zoom and pan correctly disabled for videos.Disabling zoom/pan effects for video assets prevents CSS animation interference with video playback.
71-79: LGTM: Thumbhash decoding with safe error handling.The try/catch ensures decode failures gracefully fall back to an empty string, preventing component crashes.
163-177: LGTM: Public playback API with autoplay-block handling.The exported
pause()andplay()methods correctly guard against non-video assets and include try/catch for browser autoplay blocking.
216-241: LGTM: Conditional asset rendering structure.The conditional rendering cleanly separates video and image elements while maintaining consistent styling through shared class logic.
190-191: LGTM: Face-based origin positioning correctly disabled for videos.Consistent with the
enableZoomandenablePanlogic, face-based transform origins are disabled for video assets.
253-253: LGTM: Thumbhash provides loading state.The background thumbhash image improves perceived performance during asset loading.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
docs/docs/getting-started/configurationV1.md (1)
84-84: Fix event description grammar and align with asset-aware naming.The event name was correctly updated to
AssetRequestedNotification, but the description still has grammar issues and semantic inconsistency. Update the description to properly use "asset" instead of "image" and fix the grammar.🔎 Proposed fix
-| AssetRequestedNotification | Notifies, when a Image requested. | `{"Name":"AssetRequestedNotification", "ClientIdentifier": "Frame_Kitchen", "DateTime":"2024-11-16T21:37:19.4933981+01:00", "RequestedAssetId":"UUID"}` | +| AssetRequestedNotification | Notifies when an asset is requested. | `{"Name":"AssetRequestedNotification", "ClientIdentifier": "Frame_Kitchen", "DateTime":"2024-11-16T21:37:19.4933981+01:00", "RequestedAssetId":"UUID"}` |
🧹 Nitpick comments (1)
immichFrame.Web/src/lib/components/home-page/home-page.svelte (1)
280-302: Consider adding format documentation.The
parseAssetDurationfunction handles multiple time formats (H:M:S, M:S, or S) with reasonable error handling, replacing commas with periods for decimal support and returning 0 on invalid input.📝 Optional: Add JSDoc to clarify expected formats
+ /** + * Parse asset duration string into seconds. + * Supports formats: "H:M:S", "M:S", or "S" (with optional decimal using '.' or ',') + * @returns Duration in seconds, or 0 if parsing fails + */ function parseAssetDuration(duration?: string | null) {This would help future maintainers understand the expected input formats without reading the implementation.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
docs/docs/getting-started/configuration.mddocs/docs/getting-started/configurationV1.mdimmichFrame.Web/src/lib/components/elements/asset.svelteimmichFrame.Web/src/lib/components/home-page/home-page.svelte
🧰 Additional context used
🪛 LanguageTool
docs/docs/getting-started/configurationV1.md
[grammar] ~34-~34: Use a hyphen to join words.
Context: ... | 2 digit ISO code, sets the language of the...
(QB_NEW_EN_HYPHEN)
[grammar] ~41-~41: Ensure spelling is correct
Context: ... | A list of webcalendar URIs in the .ics format. e.g. https://c...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~43-~43: Use a hyphen to join words.
Context: ... | Displays a comma separated list of names of all the peopl...
(QB_NEW_EN_HYPHEN)
[style] ~43-~43: Consider using “who” when you are referring to people instead of objects.
Context: ...parated list of names of all the people that are assigned in immich. ...
(THAT_WHO)
[grammar] ~44-~44: Use a hyphen to join words.
Context: ... | Displays a comma separated list of names of all the album...
(QB_NEW_EN_HYPHEN)
🪛 markdownlint-cli2 (0.18.1)
docs/docs/getting-started/configurationV1.md
37-37: Bare URL used
(MD034, no-bare-urls)
41-41: Bare URL used
(MD034, no-bare-urls)
48-48: Link text should be descriptive
(MD059, descriptive-link-text)
54-54: Bare URL used
(MD034, no-bare-urls)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (csharp)
🔇 Additional comments (7)
immichFrame.Web/src/lib/components/elements/asset.svelte (1)
1-262: Excellent video support implementation!The refactoring from image-centric to asset-centric props is clean, and the video playback implementation properly addresses browser autoplay restrictions. Key strengths:
- Proper cleanup: The effect at lines 52-61 ensures video resources are released when assets change.
- Conditional autoplay: Lines 224-225 correctly use
autoplay={!playAudio}andmuted={!playAudio}, with manualplay()fallback in theonloadstarthandler (lines 228-236) when audio is enabled.- Graceful error handling: Both the exported
play()function (lines 169-177) and the inlineonloadstarthandler use try-catch to handle autoplay blocking without breaking the UI.- Video-specific logic: Zoom and pan are correctly disabled for videos (lines 66-67), and transform origin repositioning is skipped (lines 190-191).
- Event coordination: The
onVideoWaitingandonVideoPlayingcallbacks enable proper progress bar synchronization in the parent component.The thumbhash poster implementation (lines 69-79) and error logging (line 238) provide good UX fallbacks.
immichFrame.Web/src/lib/components/home-page/home-page.svelte (6)
98-131: Object URL cleanup properly implemented!The refactored
updateAssetPromisescorrectly addresses the previous memory leak concern. The cleanup logic at lines 121-128 properly awaits the promise, revokes the blob URL, and removes the dictionary entry. The try-catch ensures the dictionary is cleaned up even if URL resolution fails.This pairs well with the
revokeObjectUrlimplementation (lines 380-388) that checks for theblob:prefix, ensuring only object URLs are revoked and not streaming URLs used for videos.
154-172: Race condition guard is well-implemented.The
isHandlingAssetTransitionguard prevents overlapping transitions when users rapidly navigate, and the try-finally block ensures the guard is always released. Thetick()call at line 166 properly awaits the DOM update before coordinating playback, following Svelte best practices for asynchronous UI updates.The coordination of
userPaused,progressBar, andassetComponent.play()ensures smooth transitions.
220-243: Naming and logic clarification confirmed.The function rename from
isHorizontaltoisPortrait(line 220) correctly addresses the previous review comment. The logic accurately identifies portrait-oriented images (height > width after accounting for EXIF rotation), and videos are excluded from split view (line 221-223).The
shouldUseSplitViewfunction (lines 234-243) properly combines all constraints: layout config, sufficient assets, both must be images, and both must be portrait.
331-374: Asset loading correctly handles streaming vs preloading.The conditional logic at lines 334-351 properly uses direct streaming URLs for videos (avoiding preload memory overhead) while preloading images as blobs for smooth transitions. The blob URLs are correctly created via
getObjectUrl(line 350), which are later revoked during cleanup.The graceful degradation for album info (line 358 using
?? []) addresses the previous review concern, ensuring assets display even if album metadata fails to load.
468-475: Video event handlers coordinate playback state correctly.The
onVideoWaitinghandler pauses the progress bar when the video buffers, andonVideoPlayingresumes it—but only when!userPaused(line 472). This prevents the progress bar from auto-resuming after a user manually pauses, which is the correct UX behavior.
427-446: Cleanup in onDestroy properly revokes all remaining URLs.The
onDestroyhandler (lines 436-445) iterates over all pending asset promises, resolves them, revokes their object URLs, and clears the dictionary. UsingPromise.allSettledensures cleanup completes even if individual promises reject, and the try-catch within each revoke prevents one failure from blocking others.This completes the object URL lifecycle management alongside the cleanup in
updateAssetPromises.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
immichFrame.Web/src/lib/index.ts (1)
44-50: Add input validation for theidparameter.The function doesn't validate that
idis non-empty, which could result in malformed URLs like/api/Asset//Asset. Consider adding a guard:🔎 Suggested validation
export const getAssetStreamUrl = (id: string, clientIdentifier?: string, assetType?: number) => { + if (!id) throw new Error('Asset id is required'); const params = new URLSearchParams(); if (clientIdentifier) params.set('clientIdentifier', clientIdentifier); if (assetType !== undefined) params.set('assetType', String(assetType)); const query = params.toString(); return `/api/Asset/${encodeURIComponent(id)}/Asset${query ? '?' + query : ''}`; };immichFrame.Web/static/pwa-service-worker.js (1)
54-56: Add error handling for the modified fetch request.The modified fetch request at line 54 has no error handling. If the request fails (network error, 401/403, etc.), the service worker will not provide any fallback, and the video will fail to load silently.
Consider adding error handling or logging:
🔎 Suggested improvement
- event.respondWith(fetch(modifiedRequest)); + event.respondWith( + fetch(modifiedRequest).catch((error) => { + console.error('Asset streaming request failed:', error); + return new Response('Failed to load asset', { status: 500 }); + }) + ); return;
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
immichFrame.Web/src/lib/index.tsimmichFrame.Web/static/pwa-service-worker.js
🧰 Additional context used
🧬 Code graph analysis (2)
immichFrame.Web/static/pwa-service-worker.js (1)
ImmichFrame.WebApi/Helpers/ImmichFrameAuthenticationHandler.cs (1)
Task(23-63)
immichFrame.Web/src/lib/index.ts (1)
immichFrame.Web/src/lib/stores/persist.store.ts (1)
authSecretStore(26-26)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: publish_images
- GitHub Check: Analyze (csharp)
🔇 Additional comments (3)
immichFrame.Web/src/lib/index.ts (1)
11-11: LGTM!The integration of service worker auth setup into the initialization flow is appropriate.
immichFrame.Web/static/pwa-service-worker.js (2)
4-5: LGTM!Module-scoped storage for the auth secret is the standard pattern for service workers.
47-52: The mode and credentials configuration is acceptable as written.The combination of
mode: 'cors'withcredentials: 'same-origin'will function correctly for same-origin requests—the Authorization header will be properly attached and the request will succeed. Whilemode: 'same-origin'would be semantically more appropriate for a same-origin endpoint, the current configuration is valid and does not cause issues. No changes are required unless you prefer the additional clarity thatmode: 'same-origin'provides.
| export const sendAuthSecretToServiceWorker = () => { | ||
| if (!('serviceWorker' in navigator)) return; | ||
|
|
||
| const sendMessage = () => { | ||
| if (navigator.serviceWorker.controller) { | ||
| navigator.serviceWorker.controller.postMessage({ | ||
| type: 'SET_AUTH_SECRET', | ||
| authSecret: get(authSecretStore) | ||
| }); | ||
| } | ||
| }; | ||
|
|
||
| // Send immediately if controller is ready | ||
| sendMessage(); | ||
|
|
||
| // Also send when service worker becomes ready (for initial page load) | ||
| navigator.serviceWorker.ready.then(sendMessage); | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's understand the repository structure
echo "=== Repository structure ==="
find immichFrame.Web/src -type f -name "*.ts" -o -name "*.svelte" | head -30
echo -e "\n=== Looking for the index.ts file ==="
cat immichFrame.Web/src/lib/index.tsRepository: immichFrame/ImmichFrame
Length of output: 3213
🏁 Script executed:
#!/bin/bash
# Check if there's any gating mechanism that prevents asset requests before auth is ready
rg -nP --type=ts --type=js -C3 'getAssetStreamUrl|<video|<img.*Asset' immichFrame.Web/src/ || echo "No matches found or directory doesn't exist"
# Also check for authSecretStore and its initialization
echo -e "\n=== Searching for authSecretStore ==="
rg -nP 'authSecretStore' immichFrame.Web/src/
# Look for video/image loading patterns
echo -e "\n=== Searching for asset loading patterns ==="
rg -nP 'fetch|axios|http.*Asset|loadImage|loadVideo' immichFrame.Web/src/lib/ immichFrame.Web/src/routes/ 2>/dev/null | head -30Repository: immichFrame/ImmichFrame
Length of output: 2719
🏁 Script executed:
#!/bin/bash
# First check repository structure
echo "=== Files in immichFrame.Web/src/lib/ ==="
ls -la immichFrame.Web/src/lib/ 2>/dev/null || echo "Directory not found. Checking for similar paths..."
find . -name "index.ts" -o -name "*service*worker*" 2>/dev/null | grep -v node_modules | head -10Repository: immichFrame/ImmichFrame
Length of output: 833
🏁 Script executed:
#!/bin/bash
# Find where init() is called
echo "=== Where is init() called? ==="
rg -nP 'init\(' immichFrame.Web/src/ --type=ts --type=svelte -C2 | grep -v node_modules | head -40
# Check the layout/page that loads first
echo -e "\n=== Checking +layout.ts (runs before any page) ==="
cat immichFrame.Web/src/routes/+layout.ts
# Check the main page
echo -e "\n=== Checking +page.svelte ==="
head -50 immichFrame.Web/src/routes/+page.svelte
# Check the home-page component which seems to load assets
echo -e "\n=== Checking home-page.svelte for asset loading ==="
grep -n 'asset\|Asset\|fetch\|import' immichFrame.Web/src/lib/components/home-page/home-page.svelte | head -20Repository: immichFrame/ImmichFrame
Length of output: 1772
🏁 Script executed:
#!/bin/bash
# Search for init() calls more broadly
echo "=== Searching for init() invocations ==="
grep -r "init()" immichFrame.Web/src/ --include="*.ts" --include="*.svelte" -n
# Check +layout.svelte which runs on app startup
echo -e "\n=== Checking +layout.svelte ==="
cat immichFrame.Web/src/routes/+layout.svelte
# Look at the asset-component which renders assets
echo -e "\n=== Checking asset-component.svelte ==="
cat immichFrame.Web/src/lib/components/elements/asset-component.svelte
# Check service worker for how it handles auth
echo -e "\n=== Checking service worker ==="
head -100 immichFrame.Web/src/static/pwa-service-worker.jsRepository: immichFrame/ImmichFrame
Length of output: 5332
🏁 Script executed:
#!/bin/bash
# Find the service worker
echo "=== Finding service worker ==="
find immichFrame.Web -name "*service*worker*" -o -name "*.js" | grep -i service
# Check home-page more thoroughly to see when assets load
echo -e "\n=== Full home-page.svelte ==="
cat immichFrame.Web/src/lib/components/home-page/home-page.svelte | head -200
# Check asset.svelte to see how it renders images/videos
echo -e "\n=== Checking asset.svelte ==="
cat immichFrame.Web/src/lib/components/elements/asset.svelte | head -100Repository: immichFrame/ImmichFrame
Length of output: 8321
🏁 Script executed:
#!/bin/bash
# Check service worker implementation
echo "=== Service worker implementation ==="
cat immichFrame.Web/static/pwa-service-worker.js
# See how asset URL is generated and used
echo -e "\n=== How is asset URL used in rendering? ==="
grep -A10 -B5 'asset\[0\]' immichFrame.Web/src/lib/components/elements/asset.svelte | head -50
# Check the loadAsset function and asset promise handling
echo -e "\n=== Looking for loadAsset and pickAssets functions ==="
grep -n 'loadAsset\|pickAssets' immichFrame.Web/src/lib/components/home-page/home-page.svelte | head -20
# Get the full loadAsset and pickAssets implementations
tail -100 immichFrame.Web/src/lib/components/home-page/home-page.svelteRepository: immichFrame/ImmichFrame
Length of output: 6255
🏁 Script executed:
#!/bin/bash
# Let's see the pickAssets and loadAsset functions
echo "=== pickAssets function ==="
sed -n '304,330p' immichFrame.Web/src/lib/components/home-page/home-page.svelte
echo -e "\n=== loadAsset function ==="
sed -n '331,360p' immichFrame.Web/src/lib/components/home-page/home-page.svelte
# Check onMount to see when first asset load happens
echo -e "\n=== onMount lifecycle ==="
grep -n "onMount" immichFrame.Web/src/lib/components/home-page/home-page.svelte
# Get the onMount implementation
grep -A30 "onMount\(" immichFrame.Web/src/lib/components/home-page/home-page.svelte | head -40Repository: immichFrame/ImmichFrame
Length of output: 1718
🏁 Script executed:
#!/bin/bash
# Get the onMount implementation
echo "=== onMount implementation ==="
sed -n '390,420p' immichFrame.Web/src/lib/components/home-page/home-page.svelte
# Get full loadAsset function
echo -e "\n=== Full loadAsset function ==="
sed -n '331,380p' immichFrame.Web/src/lib/components/home-page/home-page.svelteRepository: immichFrame/ImmichFrame
Length of output: 2417
🏁 Script executed:
#!/bin/bash
# Check persist.store.ts to understand authSecretStore initialization
echo "=== persist.store.ts ==="
cat immichFrame.Web/src/lib/stores/persist.store.ts
# Check when the app starts and when stores are initialized
echo -e "\n=== app.d.ts ==="
cat immichFrame.Web/src/app.d.ts
# Check the persist store implementation
echo -e "\n=== Looking for persistStore function ==="
grep -n "persistStore\|export.*authSecretStore" immichFrame.Web/src/lib/stores/persist.store.tsRepository: immichFrame/ImmichFrame
Length of output: 1455
Race condition: Asset requests may be sent before service worker receives auth secret.
On initial page load, authSecretStore defaults to null from persistStore. The init() function runs at module initialization (before onMount) and calls setBearer() synchronously (potentially with a null value) while sendAuthSecretToServiceWorker() sends the auth secret asynchronously via postMessage. Asset loading starts immediately in onMount(), which may execute before the service worker receives and stores the secret. For videos, this means fetch requests could occur without the Authorization header; for images, the header could be "Bearer null" if the secret wasn't persisted from a previous session.
The dual-send strategy (immediate + on ready) narrows the window but doesn't eliminate it entirely.
Consider:
- Deferring video/image element rendering until
sendAuthSecretToServiceWorker()completes (requires tracking the promise). - Ensuring
authSecretStoreis populated before asset loading begins (e.g., via URL params or initialization logic). - Adding error handling in the service worker to retry failed requests if the auth secret hasn't been set yet.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
immichFrame.Web/static/pwa-service-worker.js
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (csharp)
🔇 Additional comments (3)
immichFrame.Web/static/pwa-service-worker.js (3)
4-5: LGTM! Module-scoped auth secret storage is appropriate.Storing the auth secret in module scope is the correct pattern for service workers since they run in an isolated context.
57-60: LGTM! Standard network-first caching strategy.The fallback to cache on network failure is appropriate for PWA offline support.
38-55: No changes needed. The CORS mode and credentials configuration is correct for this same-origin GET endpoint. Themode: 'cors'withcredentials: 'same-origin'combination properly handles adding the Authorization header for same-origin requests without triggering unnecessary CORS preflight checks. Additionally, the endpoint only accepts GET requests, so not copying a request body is appropriate.
| // Listen for auth secret updates from the main app | ||
| self.addEventListener('message', (event) => { | ||
| if (event.data && event.data.type === 'SET_AUTH_SECRET') { | ||
| authSecret = event.data.authSecret; | ||
| } | ||
| }); |
There was a problem hiding this comment.
Validate message source to prevent auth secret hijacking.
The previous console.log issue has been resolved. However, the message handler doesn't validate the source of incoming messages. Any page could potentially send a SET_AUTH_SECRET message to this service worker, allowing an attacker to set a malicious auth secret and intercept video streaming requests.
🔎 Suggested fix: Add origin validation
// Listen for auth secret updates from the main app
self.addEventListener('message', (event) => {
+ // Validate message source
+ const clientUrl = new URL(event.source.url);
+ const expectedOrigin = self.location.origin;
+ if (clientUrl.origin !== expectedOrigin) {
+ console.warn('Service worker: rejecting message from untrusted origin');
+ return;
+ }
+
if (event.data && event.data.type === 'SET_AUTH_SECRET') {
authSecret = event.data.authSecret;
}
});🤖 Prompt for AI Agents
In immichFrame.Web/static/pwa-service-worker.js around lines 27-32, the message
handler blindly accepts SET_AUTH_SECRET messages; restrict this by validating
the message source origin and payload before setting authSecret. Update the
handler to ensure event.source exists and has a URL, verify new
URL(event.source.url).origin matches the service worker's expected origin (e.g.,
self.location.origin or an allowlist), verify event.data.authSecret is present
and of the expected type/format, and only then assign authSecret; otherwise
ignore the message (and optionally log a warning).
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
immichFrame.Web/src/lib/components/elements/asset.svelte
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (csharp)
🔇 Additional comments (2)
immichFrame.Web/src/lib/components/elements/asset.svelte (2)
52-61: LGTM: Proper video cleanup on asset change.The effect correctly tracks the asset URL and performs cleanup by pausing and clearing the video source when the asset changes, preventing resource leaks.
218-243: LGTM: Video element implementation handles autoplay restrictions correctly.The video element properly implements conditional autoplay with muted playback when audio is disabled, and manual play() with error handling when audio is enabled. The
playsinlineattribute prevents unwanted fullscreen behavior on iOS, and event callbacks are correctly forwarded.
| src={thumbHashToDataURL(decodeBase64(image[1].thumbhash ?? ''))} | ||
| alt="data" | ||
| /> | ||
| <img class="absolute flex w-full h-full z-[-1]" src={thumbhashUrl} alt="data" /> |
There was a problem hiding this comment.
Conditionally render the background image to avoid unnecessary requests.
When thumbhashUrl is an empty string (because the asset has no thumbhash or decoding failed), the <img> element with src="" will cause the browser to make an HTTP request to the current page URL.
🔎 Proposed fix to conditionally render
-<img class="absolute flex w-full h-full z-[-1]" src={thumbhashUrl} alt="data" />
+{#if thumbhashUrl}
+ <img class="absolute flex w-full h-full z-[-1]" src={thumbhashUrl} alt="data" />
+{/if}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <img class="absolute flex w-full h-full z-[-1]" src={thumbhashUrl} alt="data" /> | |
| {#if thumbhashUrl} | |
| <img class="absolute flex w-full h-full z-[-1]" src={thumbhashUrl} alt="data" /> | |
| {/if} |
🤖 Prompt for AI Agents
In immichFrame.Web/src/lib/components/elements/asset.svelte around line 264 the
<img> with src={thumbhashUrl} is always rendered, causing the browser to request
the current page when thumbhashUrl is an empty string; change this to
conditionally render the image only when thumbhashUrl is non-empty (e.g. wrap
the <img> in a Svelte {#if thumbhashUrl} ... {/if} block) so no <img> with an
empty src is emitted and unnecessary requests are avoided.
Closes #183
Summary by CodeRabbit
New Features
Refactor
Web UI
Tests
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.