Chore: Simplify Settings-Handling#667
Open
JW-CH wants to merge 2 commits into
Open
Conversation
…safe, change construction of account strategy
|
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 (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThis PR splits settings contracts, updates the config DTO/controller and Swagger wiring, refactors account-selection construction and tracking, adds tests, and removes an unused collection helper. ChangesSettings interface split and DTO/controller wiring
Account selection strategy and tracker refactor
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 (2)
ImmichFrame.WebApi/Helpers/NoReadOnlySchemaFilter.cs (1)
14-20: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider scoping the filter instead of applying it globally.
This filter strips
ReadOnlyfrom every schema property app-wide, not just the get-only DTOs (likeClientSettingsDto) it was designed for. If a future response/request type legitimately needsreadOnly(e.g., a server-assigned ID field), this filter will silently suppress that signal in the generated OpenAPI doc/client.♻️ Optional: scope to specific types
public void Apply(OpenApiSchema schema, SchemaFilterContext context) { + if (context.Type != typeof(Models.ClientSettingsDto)) + return; + foreach (var property in schema.Properties.Values) { property.ReadOnly = false; } }🤖 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.WebApi/Helpers/NoReadOnlySchemaFilter.cs` around lines 14 - 20, The NoReadOnlySchemaFilter is currently clearing ReadOnly on every OpenApiSchema property, which makes it too broad for its intended use. Update Apply in NoReadOnlySchemaFilter to only strip ReadOnly for the specific DTO types it was meant to handle, such as ClientSettingsDto or other get-only response models, by checking SchemaFilterContext before mutating schema.Properties. Keep the filter’s behavior limited to those targeted types so legitimate readOnly fields in other schemas remain intact.ImmichFrame.Core.Tests/Logic/AccountSelection/TotalAccountImagesSelectionStrategyTests.cs (1)
42-52: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider adding a zero-accounts test case.
Existing coverage handles accounts with zero total assets, but not a fully empty account list (
CreateStrategy()with no args), which is the scenario that still throws inGetAssets()per the related comment inTotalAccountImagesSelectionStrategy.cs.🤖 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.Tests/Logic/AccountSelection/TotalAccountImagesSelectionStrategyTests.cs` around lines 42 - 52, Add a test for the fully empty account list scenario in TotalAccountImagesSelectionStrategyTests, since GetAssets() still fails when CreateStrategy() is called with no accounts. Extend the existing coverage around GetAssets and CreateStrategy to verify that an empty strategy returns an empty result without throwing or producing NaN, alongside the current zero-total-assets case.
🤖 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/Logic/AccountSelection/BloomFilterAssetAccountTracker.cs`:
- Around line 13-18: The filter cache in
BloomFilterAssetAccountTracker.RecordAssetLocation is retaining failed
Lazy<Task<IBloomFilter>> entries from NewFilter(account), causing later calls to
reuse the same faulted task. Update the logic around logicToFilter.GetOrAdd so
that if the filter creation task fails, the entry for that
IAccountImmichFrameLogic is removed from logicToFilter before the exception is
propagated, allowing a fresh filter build on the next RecordAssetLocation or
ForAsset attempt.
In
`@ImmichFrame.Core/Logic/AccountSelection/TotalAccountImagesSelectionStrategy.cs`:
- Around line 34-45: GetProportions() still allows GetAssets() to call Max() on
an empty sequence when accounts is empty, causing InvalidOperationException.
Update TotalAccountImagesSelectionStrategy.GetAssets() to short-circuit before
calling Max() and return Enumerable.Empty<(IAccountImmichFrameLogic,
AssetResponseDto)>() when there are no accounts, using the existing
GetProportions() and GetWeights() flow only for non-empty account lists.
---
Nitpick comments:
In
`@ImmichFrame.Core.Tests/Logic/AccountSelection/TotalAccountImagesSelectionStrategyTests.cs`:
- Around line 42-52: Add a test for the fully empty account list scenario in
TotalAccountImagesSelectionStrategyTests, since GetAssets() still fails when
CreateStrategy() is called with no accounts. Extend the existing coverage around
GetAssets and CreateStrategy to verify that an empty strategy returns an empty
result without throwing or producing NaN, alongside the current
zero-total-assets case.
In `@ImmichFrame.WebApi/Helpers/NoReadOnlySchemaFilter.cs`:
- Around line 14-20: The NoReadOnlySchemaFilter is currently clearing ReadOnly
on every OpenApiSchema property, which makes it too broad for its intended use.
Update Apply in NoReadOnlySchemaFilter to only strip ReadOnly for the specific
DTO types it was meant to handle, such as ClientSettingsDto or other get-only
response models, by checking SchemaFilterContext before mutating
schema.Properties. Keep the filter’s behavior limited to those targeted types so
legitimate readOnly fields in other schemas remain intact.
🪄 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: deb2ae9e-aee6-418b-8848-7ef770140c27
📒 Files selected for processing (14)
ImmichFrame.Core.Tests/Logic/AccountSelection/TotalAccountImagesSelectionStrategyTests.csImmichFrame.Core/Helpers/CollectionExtensionMethods.csImmichFrame.Core/Interfaces/IClientSettings.csImmichFrame.Core/Interfaces/IImmichFrameLogic.csImmichFrame.Core/Interfaces/IServerBehaviorSettings.csImmichFrame.Core/Interfaces/IServerSettings.csImmichFrame.Core/Logic/AccountSelection/BloomFilterAssetAccountTracker.csImmichFrame.Core/Logic/AccountSelection/TotalAccountImagesSelectionStrategy.csImmichFrame.Core/Logic/MultiImmichFrameLogicDelegate.csImmichFrame.WebApi.Tests/Controllers/ConfigControllerTests.csImmichFrame.WebApi/Controllers/ConfigController.csImmichFrame.WebApi/Helpers/NoReadOnlySchemaFilter.csImmichFrame.WebApi/Models/ClientSettingsDto.csImmichFrame.WebApi/Program.cs
💤 Files with no reviewable changes (2)
- ImmichFrame.Core/Interfaces/IImmichFrameLogic.cs
- ImmichFrame.Core/Helpers/CollectionExtensionMethods.cs
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
New Features
Bug Fixes
Tests