implemented "avoid duplicates in split view" server-side#616
Open
Qoen1 wants to merge 3 commits intoimmichFrame:mainfrom
Open
implemented "avoid duplicates in split view" server-side#616Qoen1 wants to merge 3 commits intoimmichFrame:mainfrom
Qoen1 wants to merge 3 commits intoimmichFrame:mainfrom
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)
📝 WalkthroughWalkthroughAdds a queue-mutator pipeline (shuffle + duplicate-avoidance), EXIF-aware image helpers, DI registration for the mutator chain, tests for duplicate-avoidance behavior, and applies the mutator in MultiImmichFrameLogicDelegate asset retrieval. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant MIFD as MultiImmichFrameLogicDelegate
participant RQM as RandomQueueMutator
participant DAQM as DuplicateAvoidingQueueMutator
participant IH as ImageHelper
participant Result as Result
Client->>MIFD: GetAssets()
MIFD->>MIFD: accountSelectionStrategy.GetAssets()\nSelect(it => it.ToAsset())
MIFD->>RQM: Mutate(assets)
RQM->>RQM: Shuffle
RQM->>DAQM: Mutate(shuffled)
loop per asset
DAQM->>IH: IsLandscape(width,height,orientation)
IH-->>DAQM: bool landscape
alt portrait & ID already in batch
DAQM->>DAQM: Re-enqueue asset (avoid adjacent duplicate)
else
DAQM->>DAQM: Add to batch
alt batch full
DAQM->>Result: Flush batch
end
end
end
DAQM->>Result: Flush remaining
DAQM-->>RQM: mutated sequence
RQM-->>MIFD: final sequence
MIFD-->>Client: assets
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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.
Actionable comments posted: 3
🧹 Nitpick comments (5)
ImmichFrame.Core/Logic/QueueMutator/DuplicateAvoidingQueueMutator.cs (2)
12-17: Consider validatingloggerparameter.The
batchSizeis validated, butloggeris used without null check. Ifnullis passed,LogWarningcalls will throwNullReferenceException.Suggested fix
public DuplicateAvoidingQueueMutator(int batchSize, ILogger<DuplicateAvoidingQueueMutator> logger) { if(batchSize <= 0) throw new ArgumentException("Batch size must be greater than 0", nameof(batchSize)); _batchSize = batchSize; - _logger = logger; + _logger = logger ?? throw new ArgumentNullException(nameof(logger)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ImmichFrame.Core/Logic/QueueMutator/DuplicateAvoidingQueueMutator.cs` around lines 12 - 17, The DuplicateAvoidingQueueMutator constructor validates batchSize but not logger; add a null check for the ILogger<DuplicateAvoidingQueueMutator> parameter in the DuplicateAvoidingQueueMutator(int batchSize, ILogger<DuplicateAvoidingQueueMutator> logger) constructor and throw an ArgumentNullException(nameof(logger)) (or guard and assign a NullLogger) so that subsequent calls to _logger.LogWarning / LogInformation cannot cause a NullReferenceException; update assignment to _logger after the null-check.
30-39: Broadcatchblock hides exception types.Catching all exceptions masks both expected cases (null EXIF data) and unexpected programming errors. Consider catching specific expected exceptions or at least logging the exception type/message for debugging.
Suggested improvement
try { - landscape = ImageHelper.IsLandscape(currentItem.ExifInfo.ExifImageWidth!.Value, currentItem.ExifInfo.ExifImageHeight!.Value, currentItem.ExifInfo.Orientation); } - catch + catch (Exception ex) when (ex is NullReferenceException or ArgumentException) { - _logger.LogWarning("Failed to determine orientation for asset {AssetId}, defaulting to portrait", currentItem.Id); + _logger.LogWarning(ex, "Failed to determine orientation for asset {AssetId}, defaulting to portrait", currentItem.Id); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ImmichFrame.Core/Logic/QueueMutator/DuplicateAvoidingQueueMutator.cs` around lines 30 - 39, The catch-all around ImageHelper.IsLandscape should be narrowed and the exception logged; replace the broad catch with specific exceptions you expect (e.g., ArgumentNullException, InvalidOperationException, NullReferenceException) when calling ImageHelper.IsLandscape(currentItem.ExifInfo.ExifImageWidth!.Value, currentItem.ExifInfo.ExifImageHeight!.Value, currentItem.ExifInfo.Orientation) and if you still need a fallback catch, log the caught Exception object with _logger.LogWarning(exception, "Failed to determine orientation for asset {AssetId}, defaulting to portrait", currentItem.Id) so the exception type/message is recorded while preserving the existing portrait default behavior in DuplicateAvoidingQueueMutator (use the same currentItem and ImageHelper identifiers).ImmichFrame.Core/Logic/MultiImmichFrameLogicDelegate.cs (1)
21-25: Consider adding null validation forqueueMutatorparameter.The
loggerparameter is validated with?? throw new ArgumentNullException(...), butqueueMutatoris not. For consistency and defensive coding:Suggested fix
public MultiImmichFrameLogicDelegate(IServerSettings serverSettings, Func<IAccountSettings, IAccountImmichFrameLogic> logicFactory, ILogger<MultiImmichFrameLogicDelegate> logger, IAccountSelectionStrategy accountSelectionStrategy, IQueueMutator<AssetResponseDto> queueMutator) { _logger = logger ?? throw new ArgumentNullException(nameof(logger)); _accountSelectionStrategy = accountSelectionStrategy; - _queueMutator = queueMutator; + _queueMutator = queueMutator ?? throw new ArgumentNullException(nameof(queueMutator)); _serverSettings = serverSettings;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ImmichFrame.Core/Logic/MultiImmichFrameLogicDelegate.cs` around lines 21 - 25, The constructor of MultiImmichFrameLogicDelegate assigns _queueMutator without null validation; update the constructor to validate the queueMutator parameter like logger does by throwing an ArgumentNullException when null and then assign to _queueMutator (use queueMutator ?? throw new ArgumentNullException(nameof(queueMutator)) pattern), referencing the constructor and the _queueMutator field to locate the change.ImmichFrame.Core/Helpers/ImageHelper.cs (1)
26-51: Consider handlingnullor empty orientation strings gracefully.The method throws
ArgumentExceptionfor any orientation value outside "1"-"8", includingnullor empty strings. Given thatDuplicateAvoidingQueueMutatorcatches all exceptions and defaults to portrait, this works but hides potentially common cases (missing EXIF data) behind exception throwing.A design choice: you could return a default (e.g., assume orientation "1") for null/empty orientation values to avoid exception overhead for assets without EXIF orientation data.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ImmichFrame.Core/Helpers/ImageHelper.cs` around lines 26 - 51, IsLandscape currently throws ArgumentException for any exifOrientation not in "1".."8", including null/empty; change IsLandscape to treat null/empty (or whitespace) as orientation "1" instead of throwing, by adding a pre-check (e.g., if string.IsNullOrWhiteSpace(exifOrientation) then exifOrientation = "1") and then proceed with the existing orientation handling and call to GetAspectRatioFloat; remove the throw so missing EXIF orientation defaults to the normal (unrotated) logic.ImmichFrame.Core.Tests/Logic/QueueMutator/DuplicateAvoidingQueueMutatorTests.cs (1)
19-26: Redundant initialization before assignment.The
resultvariable is initialized with[new()]and then immediately overwritten. This pattern repeats in other tests.Suggested simplification
[Test] public void Mutate_NoItems_Succeeds() { - IEnumerable<AssetResponseDto> result = [new()]; - - Assert.DoesNotThrow(() => result = _queueMutator.Mutate([])); + IEnumerable<AssetResponseDto>? result = null; + Assert.DoesNotThrow(() => result = _queueMutator.Mutate([])); Assert.That(result, Is.Empty); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ImmichFrame.Core.Tests/Logic/QueueMutator/DuplicateAvoidingQueueMutatorTests.cs` around lines 19 - 26, The test Mutate_NoItems_Succeeds contains a redundant initialization of the local variable result (assigned [new()] then immediately overwritten); remove the unnecessary initial assignment and instead declare result without initialization (or assign default) and keep the Assert.DoesNotThrow invoking _queueMutator.Mutate with an explicit empty input collection; apply the same simplification pattern to the other tests that pre-initialize result before assigning from _queueMutator.Mutate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@ImmichFrame.Core.Tests/Logic/QueueMutator/DuplicateAvoidingQueueMutatorTests.cs`:
- Around line 62-77: Rename the test method
Mutate_AdjacentDuplicates_ShouldSeperate to
Mutate_AdjacentDuplicates_ShouldSeparate in the
DuplicateAvoidingQueueMutatorTests class to fix the spelling; update the method
declaration and any references (e.g., test runner attributes or calls) to use
the new name so the test still executes correctly.
In `@ImmichFrame.Core/Helpers/ImageHelper.cs`:
- Around line 18-24: Update GetAspectRatioFloat to use an accurate exception
message and remove the redundant cast: change the ArgumentException text to say
"Width and height must be positive numbers." (or similar) so it matches the
double parameters, and remove the unnecessary (double) cast in the return
statement in the GetAspectRatioFloat method in ImageHelper.
In `@ImmichFrame.WebApi/Program.cs`:
- Around line 79-84: The current issue is a captive dependency: the singleton
MultiImmichFrameLogicDelegate captures a transient
IQueueMutator<AssetResponseDto>, defeating transient semantics; to fix, either
register the mutator as a singleton or change the injection so the singleton
resolves a fresh instance per use (inject Func<IQueueMutator<AssetResponseDto>>
or IServiceProvider and call
GetRequiredService<IQueueMutator<AssetResponseDto>>() where used). Locate the
registration of IQueueMutator<AssetResponseDto> (the AddTransient lambda that
constructs RandomQueueMutator and sets next to DuplicateAvoidingQueueMutator)
and update it to AddSingleton with the same construction, or change the consumer
(MultiImmichFrameLogicDelegate) constructor to accept
Func<IQueueMutator<AssetResponseDto>> (or IServiceProvider) and call the factory
inside methods when a new mutator is needed.
---
Nitpick comments:
In
`@ImmichFrame.Core.Tests/Logic/QueueMutator/DuplicateAvoidingQueueMutatorTests.cs`:
- Around line 19-26: The test Mutate_NoItems_Succeeds contains a redundant
initialization of the local variable result (assigned [new()] then immediately
overwritten); remove the unnecessary initial assignment and instead declare
result without initialization (or assign default) and keep the
Assert.DoesNotThrow invoking _queueMutator.Mutate with an explicit empty input
collection; apply the same simplification pattern to the other tests that
pre-initialize result before assigning from _queueMutator.Mutate.
In `@ImmichFrame.Core/Helpers/ImageHelper.cs`:
- Around line 26-51: IsLandscape currently throws ArgumentException for any
exifOrientation not in "1".."8", including null/empty; change IsLandscape to
treat null/empty (or whitespace) as orientation "1" instead of throwing, by
adding a pre-check (e.g., if string.IsNullOrWhiteSpace(exifOrientation) then
exifOrientation = "1") and then proceed with the existing orientation handling
and call to GetAspectRatioFloat; remove the throw so missing EXIF orientation
defaults to the normal (unrotated) logic.
In `@ImmichFrame.Core/Logic/MultiImmichFrameLogicDelegate.cs`:
- Around line 21-25: The constructor of MultiImmichFrameLogicDelegate assigns
_queueMutator without null validation; update the constructor to validate the
queueMutator parameter like logger does by throwing an ArgumentNullException
when null and then assign to _queueMutator (use queueMutator ?? throw new
ArgumentNullException(nameof(queueMutator)) pattern), referencing the
constructor and the _queueMutator field to locate the change.
In `@ImmichFrame.Core/Logic/QueueMutator/DuplicateAvoidingQueueMutator.cs`:
- Around line 12-17: The DuplicateAvoidingQueueMutator constructor validates
batchSize but not logger; add a null check for the
ILogger<DuplicateAvoidingQueueMutator> parameter in the
DuplicateAvoidingQueueMutator(int batchSize,
ILogger<DuplicateAvoidingQueueMutator> logger) constructor and throw an
ArgumentNullException(nameof(logger)) (or guard and assign a NullLogger) so that
subsequent calls to _logger.LogWarning / LogInformation cannot cause a
NullReferenceException; update assignment to _logger after the null-check.
- Around line 30-39: The catch-all around ImageHelper.IsLandscape should be
narrowed and the exception logged; replace the broad catch with specific
exceptions you expect (e.g., ArgumentNullException, InvalidOperationException,
NullReferenceException) when calling
ImageHelper.IsLandscape(currentItem.ExifInfo.ExifImageWidth!.Value,
currentItem.ExifInfo.ExifImageHeight!.Value, currentItem.ExifInfo.Orientation)
and if you still need a fallback catch, log the caught Exception object with
_logger.LogWarning(exception, "Failed to determine orientation for asset
{AssetId}, defaulting to portrait", currentItem.Id) so the exception
type/message is recorded while preserving the existing portrait default behavior
in DuplicateAvoidingQueueMutator (use the same currentItem and ImageHelper
identifiers).
🪄 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: 6704bd4f-b3ce-47de-a52c-773d4166d957
📒 Files selected for processing (8)
ImmichFrame.Core.Tests/Logic/QueueMutator/DuplicateAvoidingQueueMutatorTests.csImmichFrame.Core/Helpers/ImageHelper.csImmichFrame.Core/Logic/MultiImmichFrameLogicDelegate.csImmichFrame.Core/Logic/QueueMutator/DuplicateAvoidingQueueMutator.csImmichFrame.Core/Logic/QueueMutator/IQueueMutator.csImmichFrame.Core/Logic/QueueMutator/QueueMutatorBase.csImmichFrame.Core/Logic/QueueMutator/RandomQueueMutator.csImmichFrame.WebApi/Program.cs
ImmichFrame.Core.Tests/Logic/QueueMutator/DuplicateAvoidingQueueMutatorTests.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@ImmichFrame.Core.Tests/Logic/QueueMutator/DuplicateAvoidingQueueMutatorTests.cs`:
- Around line 40-42: The NoChange tests currently use order-insensitive
assertions which can miss unintended reordering; update the assertions in the
DuplicateAvoidingQueueMutatorTests that check no-change behavior (the asserts
that call _queueMutator.Mutate(input) and compare result to original) to use
order-sensitive comparison by replacing Is.EquivalentTo with Is.EqualTo for
those NoChange test cases (also update the similar assertion referenced at the
other occurrence around lines 58-60) so the test fails if item order is changed.
- Around line 75-77: The current assertions in
DuplicateAvoidingQueueMutatorTests only check that the result differs from
original; update the test for _queueMutator.Mutate to (1) assert the multiset of
items in result equals original (preserve counts) and (2) assert no two adjacent
items in result are equal (no adjacent duplicates). Locate the test method that
calls _queueMutator.Mutate (in DuplicateAvoidingQueueMutatorTests) and replace
the Assert.DoesNotThrow/Is.Not.EqualTo checks with assertions that verify item
counts match (e.g., compare grouped counts or sort+sequence-equal) and loop or
use a sequence assertion to ensure result[i] != result[i+1] for all adjacent
indices.
🪄 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: 8f04040c-8e68-4dc3-8ada-9ffe652a1b85
📒 Files selected for processing (2)
ImmichFrame.Core.Tests/Logic/QueueMutator/DuplicateAvoidingQueueMutatorTests.csImmichFrame.Core/Helpers/ImageHelper.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- ImmichFrame.Core/Helpers/ImageHelper.cs
ImmichFrame.Core.Tests/Logic/QueueMutator/DuplicateAvoidingQueueMutatorTests.cs
Show resolved
Hide resolved
ImmichFrame.Core.Tests/Logic/QueueMutator/DuplicateAvoidingQueueMutatorTests.cs
Show resolved
Hide resolved
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.
server-side alternative to: #612
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 server-side chain class that can manipulate queues (in this case for removing duplicates) and can be expanded to fit other que manipulation usecases.
side effects
the amount of times only one portrait image is show was higher during testing because duplicates are moved to the back of the queue. I think this can best be fixed by adding another implementation of QueueManipulator that tries to optimize for this but perhaps it is best to leave that for another time.
issues
I saw someone mention this in: #464
Summary by CodeRabbit
New Features
Tests