Skip to content

feat(pool): exhaustive shuffle — show each asset once before repeating (resolves #438)#651

Open
frslvr wants to merge 2 commits into
immichFrame:mainfrom
frslvr:exhaustive-shuffle
Open

feat(pool): exhaustive shuffle — show each asset once before repeating (resolves #438)#651
frslvr wants to merge 2 commits into
immichFrame:mainfrom
frslvr:exhaustive-shuffle

Conversation

@frslvr
Copy link
Copy Markdown

@frslvr frslvr commented May 30, 2026

What

Adds an opt-in ExhaustiveShuffle account 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 (AllAssetsPool uses Immich's random search; CachingApiAssetsPool reshuffles and takes per call), so some photos repeat many times before others appear at all — the behaviour reported in #438 (and #555).

How

  • New ShuffleBagAssetPool : AggregatingAssetPool decorates 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.
    • Dedupes by id, so multi-source aggregates don't double-serve.
    • Bag capped at 1000 to match Immich's random-search page limit; libraries larger than that become a rolling no-repeat window rather than the entire library per cycle.
  • ExhaustiveShuffle (bool, default false) added to IAccountSettings, ServerAccountSettings, and the V1 settings + adapter, and wired into PooledImmichFrameLogic.BuildPool. Opt-in, so there is no behaviour change unless it is enabled.

Scope / notes

  • Single-client semantics: the bag lives in the (singleton) pool instance and is shared by all clients of an account. For a single frame this is exactly right; with several independent clients they share one rotation. This keeps the change small and matches the existing shared-state model — happy to extend to per-client bags if preferred.
  • Exhaustive for single-source pools (all-assets, a single album, a person…); for multi-source aggregates it dedupes and is best-effort per cycle.

Tests

  • 5 new unit tests: no-repeat within a cycle, full-set coverage each cycle, dedup, empty pool, count delegation.
  • Existing pool tests still pass.

Summary by CodeRabbit

  • New Features

    • Added an "Exhaustive Shuffle" option that, when enabled, shows each asset once in a randomized order before any repeats.
  • Tests

    • Added tests validating exhaustive-shuffle behavior: full-cycle coverage, reshuffling between cycles, deduplication, empty-source handling, and max-bag capping.
  • Documentation

    • Updated configuration docs and example settings to document the new Exhaustive Shuffle setting.

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.
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 30, 2026

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 0b59cc56-ae5a-43f9-82f0-a4dc607c5710

📥 Commits

Reviewing files that changed from the base of the PR and between 7e2af5e and 429b8e0.

📒 Files selected for processing (1)
  • ImmichFrame.Core.Tests/Logic/Pool/ShuffleBagAssetPoolTests.cs

📝 Walkthrough

Walkthrough

This 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 ExhaustiveShuffle setting wired through config and adapters, integration into pool construction, tests, and docs.

Changes

Exhaustive Shuffle Asset Pool Feature

Layer / File(s) Summary
Setting Contract and Models
ImmichFrame.Core/Interfaces/IServerSettings.cs, ImmichFrame.WebApi/Models/ServerSettings.cs, ImmichFrame.WebApi/Helpers/Config/ServerSettingsV1.cs
Adds a new boolean ExhaustiveShuffle property to account settings and exposes it through V1 config adapter (default false).
ShuffleBagAssetPool Algorithm
ImmichFrame.Core/Logic/Pool/ShuffleBagAssetPool.cs
New ShuffleBagAssetPool wraps an IAssetPool, refills a capped bag from the inner pool, deduplicates by asset.Id, shuffles with Fisher–Yates, serves each asset once per cycle, and serializes access via SemaphoreSlim.
ShuffleBagAssetPool Tests
ImmichFrame.Core.Tests/Logic/Pool/ShuffleBagAssetPoolTests.cs
Fixture and six tests verify per-cycle uniqueness, reshuffling across cycles, inner-duplicate deduplication, empty-pool behavior, MaxBagSize capping, and asset count delegation to the inner pool.
Integration with PooledImmichFrameLogic
ImmichFrame.Core/Logic/PooledImmichFrameLogic.cs
Adds WithExhaustiveShuffle helper to wrap the source pool with ShuffleBagAssetPool when account settings enable exhaustive shuffle.
Configuration Examples and Documentation
docker/Settings.example.json, docker/Settings.example.yml, docs/docs/getting-started/configurationV1.md
Adds ExhaustiveShuffle to example configs and documents the behavior (boolean, default false) in the configuration guide.

Estimated Code Review Effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

Suggested labels

enhancement

Suggested Reviewers

  • JW-CH

Poem

A rabbit shuffles through the bag,
Each photo shown, no one left lag,
One pass, then mix—no repeats in sight,
Soft hops of randomness through the night. 🐇✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 10.53% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'feat(pool): exhaustive shuffle — show each asset once before repeating' clearly summarizes the main feature: implementing exhaustive shuffle functionality where assets are shown once before repeating.
Linked Issues check ✅ Passed The PR implementation fully addresses issue #438 requirements: adds ExhaustiveShuffle setting, implements Fisher-Yates shuffle with no-repeat guarantee per cycle, deduplicates assets, caps at reasonable window size, and allows fresh draws on reshuffle.
Out of Scope Changes check ✅ Passed All changes are directly related to implementing the exhaustive shuffle feature. Configuration updates, test additions, and implementation of ShuffleBagAssetPool are all in-scope and necessary for the feature.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
ImmichFrame.Core.Tests/Logic/Pool/ShuffleBagAssetPoolTests.cs (1)

25-85: 💤 Low value

Optional: add coverage for the MaxBagSize cap 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 MaxBagSize distinct 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

📥 Commits

Reviewing files that changed from the base of the PR and between 8e94c69 and 7e2af5e.

📒 Files selected for processing (9)
  • ImmichFrame.Core.Tests/Logic/Pool/ShuffleBagAssetPoolTests.cs
  • ImmichFrame.Core/Interfaces/IServerSettings.cs
  • ImmichFrame.Core/Logic/Pool/ShuffleBagAssetPool.cs
  • ImmichFrame.Core/Logic/PooledImmichFrameLogic.cs
  • ImmichFrame.WebApi/Helpers/Config/ServerSettingsV1.cs
  • ImmichFrame.WebApi/Models/ServerSettings.cs
  • docker/Settings.example.json
  • docker/Settings.example.yml
  • docs/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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add exhaustive shuffle behavior so all images in an album are shown before reshuffling

1 participant