feat(pool): exhaustive shuffle — show each asset once before repeating (resolves #438)#651
feat(pool): exhaustive shuffle — show each asset once before repeating (resolves #438)#651frslvr wants to merge 2 commits into
Conversation
Adds an opt-in `ExhaustiveShuffle` account setting backed by a `ShuffleBagAssetPool` decorator. Instead of drawing each image at random (which repeats some photos long before others are shown), it draws the underlying set, shuffles it (Fisher-Yates), and serves each asset once before reshuffling. - New `ShuffleBagAssetPool : AggregatingAssetPool` wraps the source pool; refills from a fresh draw on exhaustion so newly added assets appear on the next cycle. Dedupes by id; bag capped at 1000 (Immich random-search page limit) so very large libraries become a rolling no-repeat window. - `ExhaustiveShuffle` flag added to IAccountSettings, ServerAccountSettings, and the V1 settings + adapter; wired into PooledImmichFrameLogic.BuildPool (opt-in, default false — no behaviour change unless enabled). - Unit tests: no-repeat within a cycle, full-set coverage each cycle, dedup, empty pool, count delegation. - Docs + example settings updated. Resolves immichFrame#438.
|
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)
📝 WalkthroughWalkthroughThis PR adds an exhaustive-shuffle asset pool: a thread-safe ShuffleBagAssetPool that serves each asset once per cycle (Fisher–Yates shuffle, per-ID dedupe, MaxBagSize cap), a new ChangesExhaustive Shuffle Asset Pool Feature
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested Reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ImmichFrame.Core.Tests/Logic/Pool/ShuffleBagAssetPoolTests.cs (1)
25-85: 💤 Low valueOptional: add coverage for the
MaxBagSizecap path.The cap (1000) is the most nuanced documented behavior (rolling no-repeat window for large sets) and isn't exercised. A test with an inner pool of > 1000 assets asserting each cycle yields exactly
MaxBagSizedistinct ids would lock in that contract.🤖 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/Pool/ShuffleBagAssetPoolTests.cs` around lines 25 - 85, Add a test that exercises the MaxBagSize cap path by constructing an inner pool with greater than ShuffleBagAssetPool.MaxBagSize assets (use InnerReturning to supply e.g. MaxBagSize + 10 unique AssetResponseDto ids), then call pool.GetAssets(ShuffleBagAssetPool.MaxBagSize) and assert the returned list has Count equal to MaxBagSize and that all ids are distinct and drawn from the supplied inner set; place the test alongside the other ShuffleBagAssetPool tests and reference ShuffleBagAssetPool, MaxBagSize, GetAssets, and InnerReturning so the cap behavior is verified.
🤖 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.
Nitpick comments:
In `@ImmichFrame.Core.Tests/Logic/Pool/ShuffleBagAssetPoolTests.cs`:
- Around line 25-85: Add a test that exercises the MaxBagSize cap path by
constructing an inner pool with greater than ShuffleBagAssetPool.MaxBagSize
assets (use InnerReturning to supply e.g. MaxBagSize + 10 unique
AssetResponseDto ids), then call pool.GetAssets(ShuffleBagAssetPool.MaxBagSize)
and assert the returned list has Count equal to MaxBagSize and that all ids are
distinct and drawn from the supplied inner set; place the test alongside the
other ShuffleBagAssetPool tests and reference ShuffleBagAssetPool, MaxBagSize,
GetAssets, and InnerReturning so the cap behavior is verified.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c5ec9112-8d3b-45dd-9dc8-037177086fe9
📒 Files selected for processing (9)
ImmichFrame.Core.Tests/Logic/Pool/ShuffleBagAssetPoolTests.csImmichFrame.Core/Interfaces/IServerSettings.csImmichFrame.Core/Logic/Pool/ShuffleBagAssetPool.csImmichFrame.Core/Logic/PooledImmichFrameLogic.csImmichFrame.WebApi/Helpers/Config/ServerSettingsV1.csImmichFrame.WebApi/Models/ServerSettings.csdocker/Settings.example.jsondocker/Settings.example.ymldocs/docs/getting-started/configurationV1.md
Adds a size-honoring inner-pool mock and a test asserting that a library larger than MaxBagSize yields exactly MaxBagSize distinct ids per cycle (the rolling no-repeat window). Per CodeRabbit review on PR immichFrame#651. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
What
Adds an opt-in
ExhaustiveShuffleaccount setting so the slideshow shows every asset once before any repeats, instead of drawing each image independently at random. Closes #438.Why
Images are currently selected at random with replacement (
AllAssetsPooluses Immich's random search;CachingApiAssetsPoolreshuffles and takes per call), so some photos repeat many times before others appear at all — the behaviour reported in #438 (and #555).How
ShuffleBagAssetPool : AggregatingAssetPooldecorates the source pool: it loads the set, shuffles it (Fisher–Yates), and serves each asset once; when the bag empties it refills from a fresh draw, so newly-added assets are picked up on the next cycle without a restart.ExhaustiveShuffle(bool, defaultfalse) added toIAccountSettings,ServerAccountSettings, and the V1 settings + adapter, and wired intoPooledImmichFrameLogic.BuildPool. Opt-in, so there is no behaviour change unless it is enabled.Scope / notes
Tests
Summary by CodeRabbit
New Features
Tests
Documentation