Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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.
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)
183-219:⚠️ Potential issue | 🟠 MajorSplit mode can incorrectly fall back to one image despite available unique assets.
At Line 183 and Line 185, you consume two assets first, then truncate to one when IDs collide. This drops the second split slot even if the backlog still has unique assets that could fill it.
Suggested fix (dedupe while selecting, and keep filling until target count)
- const useSplit = shouldUseSplitView(assetBacklog); - let next = assetBacklog.splice(0, useSplit ? 2 : 1); - - next = removeDuplicateFromNextAssets(next); + const useSplit = shouldUseSplitView(assetBacklog); + const next = takeNextUniqueAssets(assetBacklog, useSplit ? 2 : 1); assetBacklog = [...assetBacklog];- function removeDuplicateFromNextAssets(next: api.AssetResponseDto[]): api.AssetResponseDto[]{ - if(isDuplicate(next)){ - return next.slice(0,1); - } - return next; - } - - function isDuplicate(next: api.AssetResponseDto[]): boolean{ - const seen = new Set<string>(); - - for(const item of next){ - if(seen.has(item.id)){ - return true; - } - seen.add(item.id); - } - return false; - } + function takeNextUniqueAssets( + backlog: api.AssetResponseDto[], + count: number + ): api.AssetResponseDto[] { + const next: api.AssetResponseDto[] = []; + const seen = new Set<string>(); + + while (backlog.length > 0 && next.length < count) { + const candidate = backlog.shift()!; + if (seen.has(candidate.id)) continue; + seen.add(candidate.id); + next.push(candidate); + } + + return next; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@immichFrame.Web/src/lib/components/home-page/home-page.svelte` around lines 183 - 219, The current logic takes next = assetBacklog.splice(0, useSplit ? 2 : 1) then calls removeDuplicateFromNextAssets which simply cuts next to one if a duplicate is present, dropping the chance to fill the second split slot from later backlog items; update removeDuplicateFromNextAssets (and you can retire isDuplicate) to instead build a deduped next array of targetSize = useSplit ? 2 : 1 by iterating and consuming assetBacklog until next contains unique IDs or backlog is exhausted (push unique items into next and remove them from assetBacklog as you go), so that duplicate IDs are skipped but the split slot is attempted to be filled from subsequent backlog entries.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@immichFrame.Web/src/lib/components/home-page/home-page.svelte`:
- Around line 183-219: The current logic takes next = assetBacklog.splice(0,
useSplit ? 2 : 1) then calls removeDuplicateFromNextAssets which simply cuts
next to one if a duplicate is present, dropping the chance to fill the second
split slot from later backlog items; update removeDuplicateFromNextAssets (and
you can retire isDuplicate) to instead build a deduped next array of targetSize
= useSplit ? 2 : 1 by iterating and consuming assetBacklog until next contains
unique IDs or backlog is exhausted (push unique items into next and remove them
from assetBacklog as you go), so that duplicate IDs are skipped but the split
slot is attempted to be filled from subsequent backlog entries.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1438c70a-fa07-4031-9143-fed286ecc323
📒 Files selected for processing (1)
immichFrame.Web/src/lib/components/home-page/home-page.svelte
|
Hm, I think I'd prefer having this not client-side. The change itself looks good, but having the backend doing the work would simplify this I think. What are your thoughts on this? |
|
I thought that aswell, but it appears to me that the backend makes a list of all assets in the selection, which can contain the same asset multiple times, and sends it to the client. It does not care how the client displays the data. The client then receives that info and continuously picks two off of the list to display. because this logic is already handled client-side i thought it would be more logical to put this logic there aswell |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
immichFrame.Web/src/lib/components/home-page/home-page.svelte (1)
202-212: Avoid mutatingnextin the dedupe helper.Line 205 uses
splice, which mutates the input array. Returning a fresh array here makes the helper safer to reuse and easier to reason about.♻️ Proposed refactor (non-mutating)
function removeDuplicateFromNextAssets(next: api.AssetResponseDto[]): api.AssetResponseDto[]{ - if(isDuplicate(next)){ - const nextPortrait = tryPopNextNonDuplicatePortraitInBacklog([next[0]]); - let result = next.splice(0,1); - if(nextPortrait !== undefined) { - result.push(nextPortrait); - } - return result; - } - return next; + if (!isDuplicate(next)) { + return next; + } + + const primary = next[0]; + if (!primary) { + return []; + } + + const nextPortrait = tryPopNextNonDuplicatePortraitInBacklog([primary]); + return nextPortrait !== undefined ? [primary, nextPortrait] : [primary]; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@immichFrame.Web/src/lib/components/home-page/home-page.svelte` around lines 202 - 212, The helper removeDuplicateFromNextAssets currently mutates its input by calling next.splice(0,1); change it to return a new array instead (e.g., use slice(0,1) or create a new array [next[0]] and concat/ spread any portrait) so the original next array is not modified; ensure you still call tryPopNextNonDuplicatePortraitInBacklog([next[0]]) as before and conditionally append the portrait to the newly created result array rather than altering next.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@immichFrame.Web/src/lib/components/home-page/home-page.svelte`:
- Around line 202-212: The helper removeDuplicateFromNextAssets currently
mutates its input by calling next.splice(0,1); change it to return a new array
instead (e.g., use slice(0,1) or create a new array [next[0]] and concat/ spread
any portrait) so the original next array is not modified; ensure you still call
tryPopNextNonDuplicatePortraitInBacklog([next[0]]) as before and conditionally
append the portrait to the newly created result array rather than altering next.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 511add35-356d-4d76-9b4d-8e7af58ef6e5
📒 Files selected for processing (1)
immichFrame.Web/src/lib/components/home-page/home-page.svelte
|
hi @JW-CH I was thinking some more on your comment and I think you are right that it could be better to implement this server-side. I saw some othe pull requests that do something with ordering the queue of pictures, and I had nothing better to do so I implemented it in a new PR in a way I think can be easily expanded to include more features! Could you take a looka t it and let me know what you think? #616 |
2 participants
summary
On my immich frame instance I mainly want to show memories which there are not a whole lot of and I noticed that the side by side view sometimes shows duplicates. I don't think it should be that way, so i've implemented a small client-side function that will remove the duplicate picture based on it's id and show just a single photo.
what did I change?
Since I don't know this codebase well and this is my first time contributing to an open-source project I thought i'd keep my change as small as possible so I just added a function to the home page. Let me know if this should be implemented in some other way!
side effects
if you have only one image in the queue the ui looks the same regardless of the chosen layout in the config.
issues
I saw someone mention this in: #464
Summary by CodeRabbit