Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAsset identifiers move to ChangesAsset identity and face-data wiring
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
ImmichFrame.Core/Logic/PooledImmichFrameLogic.cs (1)
78-80: Use named arguments for ImmichApi calls with multiple null parameters.These calls pass several positional
nullarguments to generated client methods with optional parameters (key,slug,isOwned,isShared,name,edited,size). Using named arguments would make the intent explicit and guard against parameter order changes during client regeneration.Affected calls:
GetAssetInfoAsync(assetId, null, null)→GetAssetInfoAsync(id: assetId, key: null, slug: null)(lines 78, 88)GetAllAlbumsAsync(assetId, null, null, null, null)→GetAllAlbumsAsync(assetId: assetId, id: null, isOwned: null, isShared: null, name: null)(line 80)ViewAssetAsync(null, id, string.Empty, AssetMediaSize.Preview, null)→ViewAssetAsync(edited: null, id: id, key: string.Empty, size: AssetMediaSize.Preview, slug: null)(line 143)PlayAssetVideoAsync(id, null, null)→PlayAssetVideoAsync(id: id, key: null, slug: null)(line 176)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ImmichFrame.Core/Logic/PooledImmichFrameLogic.cs` around lines 78 - 80, Replace positional null arguments with named arguments in ImmichApi method calls to improve clarity and protect against parameter order changes. Update the GetAssetInfoAsync call on line 78 to use named arguments for key and slug parameters, the GetAllAlbumsAsync call on line 80 to use named arguments for id, isOwned, isShared, and name parameters, the ViewAssetAsync call on line 143 to use named arguments for edited, id, key, size, and slug parameters, and the PlayAssetVideoAsync call on line 176 to use named arguments for id, key, and slug parameters. Each call should explicitly name all parameters being passed instead of relying on positional argument order.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ImmichFrame.Core/Helpers/AssetHelper.cs`:
- Around line 15-20: The GetExcludedAlbumAssets method in AssetHelper.cs and
LoadAssets method in AlbumAssetsPool.cs only fetch the first page of search
results, causing assets from later pages to be missed. Implement pagination in
both methods by wrapping the search call in a loop that uses Page and Size
parameters set to a batch size of 1000, accumulating results across multiple
pages until a page returns fewer items than the batch size. Follow the same
pagination pattern already implemented in FavoriteAssetsPool, PeopleAssetsPool,
and TagAssetsPool to ensure consistency across the codebase.
In `@ImmichFrame.Core/Logic/Pool/AlbumAssetsPool.cs`:
- Around line 17-19: The albumAssets query in AlbumAssetsPool is not paginated
and only retrieves the first page of results, causing large albums to have
truncated assets. Implement pagination similar to the FavoriteAssetsPool,
PeopleAssetsPool, and TagAssetsPool implementations by wrapping the
immichApi.SearchAssetsAsync call in a loop that increments the Page parameter on
MetadataSearchDto until all pages are retrieved. Apply the same pagination fix
to AssetHelper.cs where the identical issue exists.
---
Nitpick comments:
In `@ImmichFrame.Core/Logic/PooledImmichFrameLogic.cs`:
- Around line 78-80: Replace positional null arguments with named arguments in
ImmichApi method calls to improve clarity and protect against parameter order
changes. Update the GetAssetInfoAsync call on line 78 to use named arguments for
key and slug parameters, the GetAllAlbumsAsync call on line 80 to use named
arguments for id, isOwned, isShared, and name parameters, the ViewAssetAsync
call on line 143 to use named arguments for edited, id, key, size, and slug
parameters, and the PlayAssetVideoAsync call on line 176 to use named arguments
for id, key, and slug parameters. Each call should explicitly name all
parameters being passed instead of relying on positional argument order.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 021382d4-dd64-49fa-b97b-f3c51b252447
📒 Files selected for processing (24)
ImmichFrame.Core.Tests/Logic/Pool/AggregatingAssetPoolTests.csImmichFrame.Core.Tests/Logic/Pool/AlbumAssetsPoolTests.csImmichFrame.Core.Tests/Logic/Pool/AllAssetsPoolTests.csImmichFrame.Core.Tests/Logic/Pool/CachingApiAssetsPoolTests.csImmichFrame.Core.Tests/Logic/Pool/FavoriteAssetsPoolTests.csImmichFrame.Core.Tests/Logic/Pool/FixtureHelpers.csImmichFrame.Core.Tests/Logic/Pool/MemoryAssetsPoolTests.csImmichFrame.Core.Tests/Logic/Pool/MultiAssetPoolTests.csImmichFrame.Core.Tests/Logic/Pool/PersonAssetsPoolTests.csImmichFrame.Core.Tests/Logic/Pool/QueuingAssetPoolTests.csImmichFrame.Core.Tests/Logic/Pool/TagAssetsPoolTests.csImmichFrame.Core/Helpers/AssetHelper.csImmichFrame.Core/Logic/AccountSelection/BloomFilterAssetAccountTracker.csImmichFrame.Core/Logic/AccountSelection/IAssetAccountTracker.csImmichFrame.Core/Logic/AccountSelection/TotalAccountImagesSelectionStrategy.csImmichFrame.Core/Logic/Pool/AlbumAssetsPool.csImmichFrame.Core/Logic/Pool/FavoriteAssetsPool.csImmichFrame.Core/Logic/Pool/MemoryAssetsPool.csImmichFrame.Core/Logic/Pool/PeopleAssetsPool.csImmichFrame.Core/Logic/Pool/TagAssetsPool.csImmichFrame.Core/Logic/PooledImmichFrameLogic.csImmichFrame.Core/OpenAPIs/immich-openapi-specs.jsonImmichFrame.WebApi.Tests/Controllers/AssetControllerTests.csImmichFrame.WebApi/Controllers/AssetController.cs
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@openApi/swagger.json`:
- Around line 537-540: The OpenAPI schema is over-restricting Guid fields by
enforcing UUIDv4 only. Update the affected schema definitions in swagger.json
for the listed DTOs (such as AssetResponseDto, AlbumResponseDto,
UserResponseDto, PersonResponseDto, TagResponseDto, AssetStackResponseDto, and
ContributorCountResponseDto) so that their uuid-format string fields no longer
use the UUIDv4-only pattern. Remove the pattern property entirely or relax it to
accept any valid Guid/UUID shape while keeping format: uuid and nullable where
applicable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e02ff92d-96c8-4a89-95c9-532f2897c09d
📒 Files selected for processing (2)
immichFrame.Web/src/lib/immichFrameApi.tsopenApi/swagger.json
✅ Files skipped from review due to trivial changes (1)
- immichFrame.Web/src/lib/immichFrameApi.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@immichFrame.Web/src/lib/components/home-page/home-page.svelte`:
- Around line 398-407: The face-loading logic in home-page.svelte is incorrectly
gated by assetResponse.people, which can be missing when showPeopleDesc is off,
causing zoom/pan face focus to skip loading faces. Update the condition around
getAssetFaces() to rely on the zoom/pan flags directly, or ensure people is
hydrated whenever face focus is enabled, so the facesRequest path always runs
for zoom/pan assets.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 71ad4c73-2c43-474c-8d71-47b559882fb6
📒 Files selected for processing (10)
ImmichFrame.Core/Interfaces/IImmichFrameLogic.csImmichFrame.Core/Logic/MultiImmichFrameLogicDelegate.csImmichFrame.Core/Logic/Pool/AlbumAssetsPool.csImmichFrame.Core/Logic/PooledImmichFrameLogic.csImmichFrame.WebApi/Controllers/AssetController.csimmichFrame.Web/src/lib/components/elements/asset-component.svelteimmichFrame.Web/src/lib/components/elements/asset.svelteimmichFrame.Web/src/lib/components/home-page/home-page.svelteimmichFrame.Web/src/lib/immichFrameApi.tsopenApi/swagger.json
🚧 Files skipped from review as they are similar to previous changes (3)
- ImmichFrame.Core/Logic/Pool/AlbumAssetsPool.cs
- ImmichFrame.WebApi/Controllers/AssetController.cs
- ImmichFrame.Core/Logic/PooledImmichFrameLogic.cs
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
immichFrame.Web/src/lib/components/home-page/home-page.svelte (1)
398-411: 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick winSkip face requests for video assets
asset.svelteonly usesfacesfor the image zoom/pan overlay, and those paths are disabled for videos. Gateapi.getAssetFaces(...)on!isVideoAsset(assetResponse)so video loads don’t pay for an unused request.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@immichFrame.Web/src/lib/components/home-page/home-page.svelte` around lines 398 - 411, The face lookup in the home page asset loading flow should be skipped for video assets, since the returned faces are only used by the image zoom/pan overlay. Update the logic around api.getAssetFaces in home-page.svelte to also check !isVideoAsset(assetResponse) before making the request, keeping the existing configStore zoom/pan gating intact so images still load faces when needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@immichFrame.Web/src/lib/components/home-page/home-page.svelte`:
- Around line 398-411: The face lookup in the home page asset loading flow
should be skipped for video assets, since the returned faces are only used by
the image zoom/pan overlay. Update the logic around api.getAssetFaces in
home-page.svelte to also check !isVideoAsset(assetResponse) before making the
request, keeping the existing configStore zoom/pan gating intact so images still
load faces when needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c64235d2-5fbe-48b7-b51c-fbbd0783e0c7
📒 Files selected for processing (1)
immichFrame.Web/src/lib/components/home-page/home-page.svelte
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit