Add Exhaustive Shuffle Support for Albums #583
Conversation
📝 WalkthroughWalkthroughAdds a request-scoped IRequestContext (AssetOffset, AssetShuffleRandom), threads it through pools, logic, and controllers, implements deterministic offset+seed paging in the caching pool, adds middleware to populate the context from query params, changes API responses to AssetListResponseDto, and updates tests and frontend types/usages. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Middleware as RequestContextMiddleware
participant Controller as AssetController
participant Logic as ImmichFrameLogic
participant Pool as AssetPool
participant Storage as AssetStorage
Client->>Middleware: GET /api/assets?assetOffset=0&assestShuffleRandom=123
Middleware->>Middleware: parse query -> set IRequestContext.AssetOffset / AssetShuffleRandom
Middleware->>Controller: invoke (IRequestContext injected)
Controller->>Logic: GetAssets(IRequestContext)
Logic->>Pool: GetAssets(requested=25, IRequestContext)
Pool->>Storage: load assets
Pool->>Pool: seeded shuffle (AssetShuffleRandom), apply AssetOffset, slice results, advance AssetOffset
Pool-->>Logic: return assets
Logic-->>Controller: build AssetListResponseDto { assetOffset, assets }
Controller-->>Client: 200 OK { assetOffset, assets[] }
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ImmichFrame.Core/Logic/Pool/QueuingAssetPool.cs (1)
43-67:⚠️ Potential issue | 🟡 MinorPotential per-client offset skew when reloading into a shared queue.
ReloadAssetsAsync uses the caller’s IRequestContext; if the delegate is offset-aware (e.g., CachingApiAssetsPool) and the queue is shared across clients, the background reload advances only that caller’s offset for assets enqueued for everyone. Consider using a pool-level offset or a dedicated context for reloads, or constraining this pool to delegates that ignore AssetOffset.
🤖 Fix all issues with AI agents
In `@ImmichFrame.Core/Api/AssetListResponseDto.cs`:
- Around line 3-6: The Assets property on AssetListResponseDto is nullable but
the frontend expects a non-null list; change the Assets declaration to a
non-nullable List<AssetResponseDto> and initialize it to an empty list by
default (e.g., inline initializer or in the class constructor) so
AssetListResponseDto.Assets always returns an empty collection instead of null.
In `@ImmichFrame.Core/Logic/Pool/AlbumAssetsPool.cs`:
- Line 23: The code always randomizes albumAssets by calling
albumAssets.Shuffle().ToList(); change this to honor the ExhaustiveAlbumShuffle
toggle: in the AlbumAssetsPool method that returns the list (where albumAssets
is used), check the ExhaustiveAlbumShuffle setting and only call Shuffle() when
that setting is true, otherwise return albumAssets.ToList() so order is
preserved; reference the ExhaustiveAlbumShuffle property, the albumAssets
variable, and the Shuffle() call when applying the conditional.
In `@ImmichFrame.Core/Logic/Pool/CachingApiAssetsPool.cs`:
- Around line 16-35: In GetAssets, advancing requestContext.AssetOffset by the
requested count can skip items when fewer assets are returned than requested;
change the offset advancement to use the actual number of items returned (e.g.,
assetsToReturn.Count()) and then wrap correctly (set to 0 or use modulo
totalCount) so you never skip or double-serve assets when crossing the end
boundary; also handle totalCount == 0 early to avoid division/invalid wrap
logic. Ensure you update the code around AllAssets, totalCount, assetsToReturn
and requestContext.AssetOffset accordingly.
In `@immichFrame.Web/src/lib/components/home-page/home-page.svelte`:
- Around line 133-136: The initial call to api.getAssets sets clientIdentifier:
undefined which bypasses per-client filtering and causes mismatched assets vs
later requests that use $clientIdentifierStore; update the getAssets call (the
assetRequest assignment) to pass the current client identifier (e.g.,
$clientIdentifierStore or the local clientIdentifier variable used elsewhere)
instead of undefined so all asset requests consistently use the same client
identifier.
In `@ImmichFrame.WebApi/Controllers/AssetController.cs`:
- Line 40: The GetAssets method declares an unused parameter assetOffset which
is redundant because RequestContextMiddleware populates
_requestContext.AssetOffset; either remove the parameter from
AssetController.GetAssets to avoid confusion (and update Swagger/contract docs
accordingly), or modify GetAssets to use the incoming assetOffset parameter
(replace references to _requestContext.AssetOffset with the method parameter) so
the parameter is actually applied; update any XML comments or swagger
annotations to reflect the chosen behavior.
In `@ImmichFrame.WebApi/Helpers/RequestContextMiddleware.cs`:
- Around line 16-23: The code in RequestContextMiddleware that reads
context.Request.Query.TryGetValue("assetOffset", out var assetOffset) uses
int.Parse which can throw on bad or out-of-range input; change it to use
int.TryParse on assetOffset.ToString() (or TryParse on the Value directly) and
only assign to requestContext.AssetOffset when TryParse returns true, leaving
the existing default/nullable value when parsing fails to avoid exceptions and
500 responses.
🧹 Nitpick comments (2)
ImmichFrame.Core/Logic/Pool/CachingApiAssetsPool.cs (1)
18-19: Multiple enumeration ofallAssetsmay be inefficient.
allAssetsis enumerated multiple times: once forCount()(line 19) and again forSkip().Take()(line 26). If the cache returns a deferredIEnumerable, this causes redundant computation.Consider materializing to a list once, or verifying the cache always returns a materialized collection.
♻️ Suggested improvement
- var allAssets = await AllAssets(ct); - var totalCount = allAssets.Count(); + var allAssets = (await AllAssets(ct)).ToList(); + var totalCount = allAssets.Count;ImmichFrame.Core.Tests/Logic/Pool/CachingApiAssetsPoolTests.cs (1)
57-59: Mock doesn't track offset mutations; consider adding offset behavior tests.The mock setup
_mockRequestContext.Setup(x => x.AssetOffset).Returns(0)returns a constant value. SinceCachingApiAssetsPool.GetAssetsmutatesAssetOffset, these tests won't detect if offset advancement is broken.Consider adding tests that:
- Use
SetupPropertyto allow the mock to track mutations- Verify offset is correctly advanced after calls
- Test wrap-around behavior when offset exceeds total count
💡 Example test for offset behavior
[Test] public async Task GetAssets_AdvancesOffset_AfterRetrieval() { // Arrange var assets = CreateSampleAssets(); _testPool.LoadAssetsFunc = () => Task.FromResult<IEnumerable<AssetResponseDto>>(assets); _mockAccountSettings.SetupGet(s => s.ShowArchived).Returns(true); int currentOffset = 0; _mockRequestContext.SetupGet(x => x.AssetOffset).Returns(() => currentOffset); _mockRequestContext.SetupSet(x => x.AssetOffset = It.IsAny<int>()) .Callback<int>(val => currentOffset = val); // Act await _testPool.GetAssets(2, _mockRequestContext.Object); // Assert Assert.That(currentOffset, Is.EqualTo(2)); }
| public partial class AssetListResponseDto | ||
| { | ||
| public int AssetOffset { get; set; } | ||
| public List<AssetResponseDto>? Assets { get; set; } |
There was a problem hiding this comment.
Avoid nullable Assets to match client expectations.
The frontend consumes assetRequest.data.assets without a null guard. If the server ever omits the list, this will throw. Prefer a non-null list with a default empty value (or ensure it is always populated).
🛠️ Suggested fix
- public List<AssetResponseDto>? Assets { get; set; }
+ public List<AssetResponseDto> Assets { get; set; } = new();📝 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.
| public partial class AssetListResponseDto | |
| { | |
| public int AssetOffset { get; set; } | |
| public List<AssetResponseDto>? Assets { get; set; } | |
| public partial class AssetListResponseDto | |
| { | |
| public int AssetOffset { get; set; } | |
| public List<AssetResponseDto> Assets { get; set; } = new(); |
🤖 Prompt for AI Agents
In `@ImmichFrame.Core/Api/AssetListResponseDto.cs` around lines 3 - 6, The Assets
property on AssetListResponseDto is nullable but the frontend expects a non-null
list; change the Assets declaration to a non-nullable List<AssetResponseDto> and
initialize it to an empty list by default (e.g., inline initializer or in the
class constructor) so AssetListResponseDto.Assets always returns an empty
collection instead of null.
|
I have a couple of concerns with this (I haven't looked at the code yet, this is just going by the description):
I was trying to think of other options, but it gets complex. For instance; each client keeps a list of shown assets, and in each fetch of 20 we remove any that are on that list. Complexity comes as you begin to exhaust not yet shown assets. Or trying to make it work for both 100 asset libraries and 30,000 asset libraries. Or trying to figure out where the list cut-off is (length or time?). |
|
While I'm not nearly as well versed in this project as you are, I'll add my thoughts here. I say this in the lightest way possible, I'm just challenging with what limited knowledge I have right now.
|
I’m still concerned with 1—people are really good at spotting patterns, and I think they’ll find the fixed order noticeable. Regarding 2, I’m a little confused on the 24-hour cache mention. The default is 12 hours, and we can pull that down to an hour (or zero) using RefreshAlbumPeopleInterval. Are you seeing a 24-hour limit somewhere else? |
This is my bad, I must be misremembering, likely the Thinking about this, we can store a random shuffle value on the client, and pass that to the server. The server can then order by the value passed from the client. This would guarantee no duplicates while still allowing each frame to be uniquely random. What do you think about that? |
… the random shuffle integer
|
I've moved back to shuffling/ordering on each request, with the client holding the randomization integer. I believe this should play nicer with automatically fetching new assets in the future. This will also ensure that multiple frames/devices all get a completely different series of assets. |
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In `@ImmichFrame.Core/Logic/Pool/CachingApiAssetsPool.cs`:
- Line 24: The current OrderBy call in CachingApiAssetsPool (the
allAssets.OrderBy(_ => requestContext.AssestShuffleRandom) that produces
assetsToReturn) is a no-op because it uses a constant key; replace it with a
deterministic per-element ordering keyed by asset ID plus the client seed:
compute a per-asset key like HashCode.Combine(asset.Id,
requestContext.AssestShuffleRandom) (or alternatively perform a Fisher–Yates
shuffle using a Random seeded from requestContext.AssestShuffleRandom) and
OrderBy that key so each client gets a stable, shuffled ordering before
Skip/Take.
In `@ImmichFrame.Core/Services/RequestContextService.cs`:
- Around line 1-9: The RequestContext class currently misspells the property
AssestShuffleRandom; rename that property to AssetShuffleRandom in the
RequestContext class and also update the IRequestContext interface to match
(keep AssetOffset as-is), then update all usages (middleware that maps query
params, API client models, and frontend consumers) to the new AssetShuffleRandom
name so the property name is consistent across the stack.
In `@immichFrame.Web/src/lib/components/home-page/home-page.svelte`:
- Around line 49-50: Rename the misspelled variable assestShuffleRandom to
assetShuffleRandom across the codebase; update the declaration in
home-page.svelte (let assestShuffleRandom → let assetShuffleRandom), and
propagate the rename to all related types and symbols including IRequestContext,
RequestContext, any middleware that references assestShuffleRandom, and the API
client methods/fields that send or receive this property so names match on
client and server; run a project-wide search/replace for "assestShuffleRandom"
and update usages and tests accordingly to ensure compilation and API
compatibility.
- Line 50: The seed assignment for assestShuffleRandom uses
Math.floor(Math.random()) which always yields 0; replace it with a proper
integer seed generator such as producing a random integer in [0, 2147483647]
(Int32.MaxValue) so each client gets a unique nonzero shuffle seed (e.g.,
compute Math.random() * 2147483647 and floor it) and keep the variable type
number as assestShuffleRandom.
In `@ImmichFrame.WebApi/Helpers/RequestContextMiddleware.cs`:
- Line 16: The inline comment in RequestContextMiddleware (comment text
"assetOffest") contains a typo; update the comment to read "assetOffset" so it
accurately reflects the intended variable/field name and improves readability in
the RequestContextMiddleware class or related methods.
- Around line 39-49: The current middleware stores
requestContext.AssestShuffleRandom but CachingApiAssetsPool.GetAssets uses
.OrderBy(_ => requestContext.AssestShuffleRandom) which is a no-op because it
orders by a constant; fix GetAssets to perform a deterministic per-request
shuffle using the stored integer as a seed by creating a single Random seeded
with requestContext.AssestShuffleRandom (or implement a seeded Fisher–Yates) and
then ordering elements by that Random.Next() or performing an in-place shuffle,
ensuring you do NOT instantiate new Random per element; locate
CachingApiAssetsPool.GetAssets and replace the constant OrderBy with a
seeded-shuffle using requestContext.AssestShuffleRandom.
- Around line 39-60: The query parameter and property misspell "asset" as
"assest"; update the HTTP query key and all related symbols to the correct
spelling: rename the query parameter "assestShuffleRandom" to
"assetShuffleRandom" in RequestContextMiddleware, rename the property
AssestShuffleRandom to AssetShuffleRandom in IRequestContext,
RequestContextService, RequestContextMiddleware and CachingApiAssetsPool, and
update all usages and tests accordingly so the public API and internal
references are consistent (preserve behavior when parsing integer values).
Ensure any serialization/contract annotations or docs referencing the old name
are updated to the new "assetShuffleRandom".
🧹 Nitpick comments (2)
ImmichFrame.WebApi/Helpers/RequestContextMiddleware.cs (1)
14-62: Extract a helper to eliminate duplicated parsing logic.The two query-parameter parsing blocks (lines 17–37 and 40–60) are structurally identical. A small helper would reduce duplication and make adding future parameters trivial.
♻️ Suggested refactor
+ private static int ParseQueryInt(HttpContext context, string paramName, int defaultValue = 0) + { + if (context.Request.Query.TryGetValue(paramName, out var raw) + && int.TryParse(raw.ToString(), out var parsed) + && parsed >= 0) + { + return parsed; + } + return defaultValue; + } + public async Task InvokeAsync(HttpContext context, IRequestContext requestContext) { - // assetOffest - if (context.Request.Query.TryGetValue("assetOffset", out var assetOffset)) - { - ... - } - - // assestShuffleRandom - if (context.Request.Query.TryGetValue("assestShuffleRandom", out var assestShuffleRandom)) - { - ... - } + requestContext.AssetOffset = ParseQueryInt(context, "assetOffset"); + requestContext.AssetShuffleRandom = ParseQueryInt(context, "assetShuffleRandom"); await _next(context); }ImmichFrame.Core/Logic/Pool/CachingApiAssetsPool.cs (1)
16-32: Multiple enumeration ofIEnumerable— materialize once.
allAssetsisIEnumerable<AssetResponseDto>. It is enumerated by.Count()(line 17), then by.OrderBy().Skip().Take()(line 24), then the result is enumerated again by.Count()(line 26). Even if the cache returns a materialized collection, this is fragile and could cause subtle bugs or performance issues if the underlying implementation changes.♻️ Proposed fix
- var allAssets = await AllAssets(ct); - var totalCount = allAssets.Count(); + var allAssets = (await AllAssets(ct)).ToList(); + var totalCount = allAssets.Count;Then
assetsToReturnshould also be materialized (.ToList()) before calling.Count()on line 26, as shown in the other comment's fix.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@ImmichFrame.Core/Logic/Pool/CachingApiAssetsPool.cs`:
- Around line 24-33: The deferred LINQ using new
Random(requestContext.AssetShuffleRandom) makes assetsToReturn stateful and
calling assetsToReturn.Count() re-enumerates with a different Random state;
materialize the ordered sequence once (e.g., evaluate the result of
allAssets.OrderBy(_ => orderValue.Next()) with ToList()) into a local like
orderedAssets, use
orderedAssets.Skip(requestContext.AssetOffset).Take(requested) or slice that
list and update requestContext.AssetOffset using the materialized list's Count,
then return the materialized/selected sequence so enumeration happens only once;
update references to assetsToReturn, orderValue, requestContext.AssetOffset,
requested, allAssets.OrderBy and totalCount accordingly.
- Around line 16-17: AllAssets() currently returns a lazy IEnumerable from
LoadAssets().ApplyAccountFilters(...), causing repeated re-evaluation whenever
callers call Count(), OrderBy(), etc.; change AllAssets() to materialize the
filtered sequence (e.g., call .ToList()) before caching/returning so the result
is a concrete collection and the filter chain is not re-run multiple times;
locate AllAssets() (and the call sites using AllAssets() like where totalCount =
allAssets.Count()) and ensure the cached value originates from
LoadAssets().ApplyAccountFilters(...).ToList() so subsequent Count/OrderBy
operations operate on the in-memory list.
🧹 Nitpick comments (1)
ImmichFrame.WebApi/Helpers/RequestContextMiddleware.cs (1)
14-60: Extract a helper to eliminate the duplicated parse-or-default pattern.The two blocks (lines 17–36 and 40–60) are structurally identical. A small helper would reduce duplication and make adding future query parameters trivial.
♻️ Suggested refactor
+ private static int ParseIntQueryParam(HttpContext context, string paramName) + { + if (context.Request.Query.TryGetValue(paramName, out var raw) + && int.TryParse(raw.ToString(), out var value)) + { + return value; + } + return 0; + } + public async Task InvokeAsync(HttpContext context, IRequestContext requestContext) { - // assetOffest - if (context.Request.Query.TryGetValue("assetOffset", out var assetOffset)) - { - string value = assetOffset.ToString(); - - if (value != null && value.Length > 0) - { - bool success = int.TryParse(value, out int number); - if (success) - { - requestContext.AssetOffset = number; - } - else - { - requestContext.AssetOffset = 0; - } - } - else - { - requestContext.AssetOffset = 0; - } - } - - // assetShuffleRandom - if (context.Request.Query.TryGetValue("assestShuffleRandom", out var assetShuffleRandom)) - { - string value = assetShuffleRandom.ToString(); - - if (value != null && value.Length > 0) - { - bool success = int.TryParse(value, out int number); - if (success) - { - requestContext.AssetShuffleRandom = number; - } - else - { - requestContext.AssetShuffleRandom = 0; - } - } - else - { - requestContext.AssetShuffleRandom = 0; - } - } + requestContext.AssetOffset = ParseIntQueryParam(context, "assetOffset"); + requestContext.AssetShuffleRandom = ParseIntQueryParam(context, "assetShuffleRandom"); await _next(context); }
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@ImmichFrame.Core/Logic/Pool/CachingApiAssetsPool.cs`:
- Around line 19-22: The code only resets requestContext.AssetOffset when it
exceeds totalCount but doesn't guard against negative values; update the logic
around the existing check that references requestContext.AssetOffset and
totalCount (in CachingApiAssetsPool.cs) to clamp AssetOffset to the valid range:
if AssetOffset is less than 0 set it to 0, if AssetOffset is greater than or
equal to totalCount set it to 0 (or totalCount-1 if you prefer business logic),
and ensure this clamping occurs before any Skip/Take operations so negative
offsets cannot influence LINQ.Skip behavior.
🧹 Nitpick comments (1)
ImmichFrame.Core/Logic/Pool/CachingApiAssetsPool.cs (1)
24-25: FullOrderByon every request is O(n log n); consider caching the sorted list.
allAssets.OrderBy(_ => orderValue.Next())re-sorts the entire cached collection on everyGetAssetscall. For the ~600-asset test case this is fine, but for larger libraries it becomes expensive. Since the ordering is deterministic for a given seed, you could cache the sorted result keyed byAssetShuffleRandom(with an appropriate eviction policy) so repeated pages from the same client don't re-sort.
2 participants
This PR is a potential solution to the exhaustive shuffle (#438) issue.
The main changes here are that we now shuffle the assets once and cache them (so we'll fetch/shuffle daily, without restarts), and the web client keeps track of an offset position, which is used by the API to determine which section/slice of the assets to return to the client. The API returns a new offset with each response, so the web client can stay ignorant of the assets outside of the 25 that are currently returned with each request. The offset correctly rolls over to 0 if it exceeds the length of the assets.
One caveat here is that every client will get the images in exactly the same order. We could add a random offset to the first asset fetch (say if it's a negative number), so multiple clients don't show the exact same images if they connect at the same time.
Another is that we now pass a
RequestContextaround, which is how we know about the offset from each client. I believe in the future this might prove useful if there is ever a desire for client-side configurations, or other similar behaviour.I have added an
ExhaustiveAlbumShufflesetting, but I have not used it, as I'm not sure if this needs to be behind a setting, or if this new behaviour as the default is acceptable. Also, using settings inside of the Pools was potentially not an easy lift and I didn't want this PR to get any bigger than it already is without getting some feedback.I have been testing on an album of almost 600 assets, mixed pictures and videos, and it's been good.
Please let me know your thoughts, as I am very much looking forward to something like this to prevent seeing the same images multiple times, and some almost never.
Summary by CodeRabbit
New Features
Tests