Skip to content

Conversation

@Zaida-3dO
Copy link

@Zaida-3dO Zaida-3dO commented Jan 18, 2026

Summary

Add AssetBatchSize config option to control how many assets are fetched per batch (default: 25)

Rationale: Currently this is hardcoded to 25 but that means after 25 images you get a new batch of random assets where there could be collisions, making this configurable allows the user to determine how long they want to go before possible seeing duplicate assets.

Add ClientPersistAssetQueue to persist current and upcoming assets in localStorage across browser restarts

Add ClientPersistAssetHistory to persist asset history in localStorage for back button functionality after refresh

Rationale: Currently if the client restarts (e.g. due to a power cycle on the TV or the app gets unintentionally closed, all viewed items and the current queue gets reset. This means

  1. You can go back to view history anymore
  2. You get a fresh batch of assets that might again, be duplicates of those you already viewed recently
    This makes it configurable to choose for all clients to persist the history and queue so you don't loose your place just because the app got closed.

Open questions/considerations

  • This should maybe be a client side config - Ideally I actually think this should be a config on the client side especially for the android app, but I don't have much experience with android dev so this was the next best thing
  • Should these be the same config - I can see arguments for both my current solution and for this being one config option instead of 2, but I figured it's never a bad idea to have more configurability. I'm happy to change this if maintainers think otherwise.
  • Assets stored might become stale (i.e. no longer supposed to be server by the server) - This is valid and so i added a warning in the config to flag this, setting the config to false will also clear the state for any user who runs into this.

Summary by CodeRabbit

  • New Features

    • Configurable asset batch size (default 25) for asset loading.
    • Optional client-side persistence for asset queue and viewing history; restored on load and resumes viewing when available.
    • Server session ID exposed so clients can detect server restarts; asset requests include a client identifier.
  • Documentation

    • Configuration guide updated with the new settings and notes about persistence behavior.

✏️ Tip: You can customize this high-level summary in your review settings.

Zaida-3dO and others added 2 commits January 18, 2026 18:39
- Add AssetBatchSize config to control batch size for asset loading (default: 25)
- Add ClientPersistAssetQueue to persist current and upcoming assets in localStorage
- Add ClientPersistAssetHistory to persist asset history in localStorage
- Extract showAssets() function to centralize asset display logic
- Update documentation with new config options and stale asset warnings

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Add AssetBatchSize, ClientPersistAssetQueue, and ClientPersistAssetHistory
defaults to the ServerSettingsV1Adapter for backwards compatibility.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@coderabbitai
Copy link

coderabbitai bot commented Jan 18, 2026

📝 Walkthrough

Walkthrough

Added three new general settings (AssetBatchSize, ClientPersistAssetQueue, ClientPersistAssetHistory), exposed them through backend interfaces, models, DTOs, and API; GetAssets uses AssetBatchSize; introduced ServerSession; added frontend persisted stores and restoration/persistence logic for backlog, displaying assets, and history.

Changes

Cohort / File(s) Summary
Backend Interface & Models
ImmichFrame.Core/Interfaces/IServerSettings.cs, ImmichFrame.WebApi/Models/ServerSettings.cs, ImmichFrame.WebApi/Helpers/Config/ServerSettingsV1.cs
Added IGeneralSettings properties: AssetBatchSize (int), ClientPersistAssetQueue (bool), ClientPersistAssetHistory (bool). Defaults added in ServerSettings and adapter; Validate enforces AssetBatchSize >= 1.
Backend Logic
ImmichFrame.Core/Logic/PooledImmichFrameLogic.cs
Replaced hardcoded batch size with _generalSettings.AssetBatchSize when requesting asset batches.
Backend → Frontend DTO & Controller
ImmichFrame.WebApi/Models/ClientSettingsDto.cs, ImmichFrame.WebApi/Controllers/ConfigController.cs
Added DTO properties AssetBatchSize, ClientPersistAssetQueue, ClientPersistAssetHistory, and ServerSessionId; updated FromGeneralSettings signature and GetConfig to include server session ID.
Server Session & DI
ImmichFrame.WebApi/Helpers/ServerSession.cs, ImmichFrame.WebApi/Program.cs
Added ServerSession class exposing a GUID SessionId and registered it as a singleton.
Frontend API Types
immichFrame.Web/src/lib/immichFrameApi.ts
Extended client settings type with assetBatchSize?: number, clientPersistAssetQueue?: boolean, clientPersistAssetHistory?: boolean.
Frontend Persisted Stores
immichFrame.Web/src/lib/stores/persist.store.ts
Added persistArrayStore<T> and clearPersistedStore; exported serverSessionIdStore, assetBacklogStore, assetHistoryStore, displayingAssetsStore persisted to localStorage.
Frontend Component Logic
immichFrame.Web/src/lib/components/home-page/home-page.svelte
Added persistence helpers and showAssets flow; restore/persist backlog, displayingAssets, and history based on server session and persist flags; include clientIdentifier in fetches and integrate persistence into navigation/load flows.
Documentation
docs/docs/getting-started/configuration.md
Documented three new general configuration keys with defaults and notes about persistence behavior across restarts.

Sequence Diagram

sequenceDiagram
    participant Browser as Client (Browser)
    participant API as Server (API)
    participant LS as LocalStorage
    participant Pool as Asset Pool

    Browser->>API: GET /client-settings
    API-->>Browser: ClientSettings (assetBatchSize, persist flags, serverSessionId)

    alt persistence enabled
        Browser->>LS: load `serverSessionId`, `assetBacklog`, `displayingAssets`, `assetHistory`
        alt persisted session matches
            LS-->>Browser: return persisted arrays
            Browser->>Pool: render persisted displayingAssets
            Browser->>Browser: restore image promises
        else no valid persisted data
            Browser->>API: Fetch assets (include clientIdentifier, batchSize)
            API-->>Browser: return asset batch
            Browser->>Pool: render fetched assets
            Browser->>LS: persist backlog & displayingAssets
        end
    else persistence disabled
        Browser->>LS: clear persisted keys
        Browser->>API: Fetch assets (include clientIdentifier, batchSize)
        API-->>Browser: return asset batch
        Browser->>Pool: render fetched assets
    end

    loop user navigates
        Browser->>Browser: compute next/previous sets
        Browser->>LS: persist backlog & history
        Browser->>Pool: render updated displayingAssets
    end
Loading

Estimated Code Review Effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Poem

🐰 I nibble configs under moonlit light,
I count the batches and tuck queues tight,
Backlogs remembered, history saved fair,
A hop, a blink — your gallery's there! 🥕✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 7.69% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and specifically describes the main changes: adding a configurable AssetBatchSize and client persistence options for asset queue and history.

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


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

@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.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
immichFrame.Web/src/lib/components/home-page/home-page.svelte (1)

344-387: Fix persistence key scoping to prevent cross-client data leakage.

Persistence keys (assetBacklog, assetHistory, displayingAssets) are hardcoded without client identifier. Switching ?client= will restore the previous client's queue and history. Include the client identifier in the persistence keys or clear these stores when the client parameter changes.

🤖 Fix all issues with AI agents
In `@ImmichFrame.WebApi/Models/ServerSettings.cs`:
- Around line 73-75: ServerSettings.AssetBatchSize can be zero/negative and must
be validated in ServerSettings.Validate(); update Validate() to clamp or enforce
a minimum (e.g., if AssetBatchSize < 1 then set AssetBatchSize = 1 or throw) so
downstream logic never sees non-positive batch sizes—locate the ServerSettings
class and its Validate() method and add this check (use Math.Max(1,
AssetBatchSize) or equivalent).
🧹 Nitpick comments (1)
ImmichFrame.WebApi/Helpers/Config/ServerSettingsV1.cs (1)

129-131: Avoid default drift between V1 adapter and GeneralSettings.

These hard-coded defaults can diverge from GeneralSettings over time. Consider referencing a shared constant/default source to keep them in sync.

Zaida-3dO and others added 4 commits January 18, 2026 19:47
Ensure AssetBatchSize is at least 1 to prevent issues with
zero or negative batch sizes.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
When the server restarts, the in-memory BloomFilter loses track of which
assets belong to which account. This caused errors when clients tried to
display persisted assets from a previous server session.

- Add ServerSession service that generates a unique ID at startup
- Include serverSessionId in ClientSettingsDto
- Client checks if server session changed and clears persisted asset
  stores (backlog, history, displayingAssets) on mismatch

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@Zaida-3dO
Copy link
Author

Hey @JW-CH so sorry for the direct ping, I just have a question about process here

I believe this PR and the other I put up here are ready for review, but I'm not sure if there's anything else I need to do to get this added to your review backlog.

No pressure on getting to the actual review, I just want to make sure there's nothing left on my end blocking review of these PRs

There's some comments from code rabbit I have addressed and a conflict but I want to wait till the review process is started to fix the conflict

@JW-CH
Copy link
Collaborator

JW-CH commented Feb 2, 2026

Hey @Zaida-3dO, thank you for your message. I've been going back and forth on whether to review your PRs — I should have just reached out earlier.
To be honest, I'm hesitant to review PRs that are primarily AI-generated. Code review takes considerable time and effort, and when it feels like the review requires more investment than went into creating the code itself, something feels off to me.
I do use tools like Coderabbit to assist with reviews, but that's in addition to a human review — not a replacement for thoughtful, human-driven contributions.
I hope you understand where I'm coming from.

@Zaida-3dO
Copy link
Author

Zaida-3dO commented Feb 2, 2026

Hey @JW-CH

I completely understand where you're coming from, but I can assure you I did put in a lot of time and effort to thinking through the changes here, claude was mainly used for execution. Also I have a fork of this running locally which I have been using for the last 2 weeks and haven't run into any major issues so far.

I do use tools like Coderabbit to assist with reviews, but that's in addition to a human review — not a replacement for thoughtful, human-driven contributions.

Yeah, I feel the same, I used claude mainly as a tool for writing the code, but not as a replacement for thinking through the changes, how best to implement it and reviewing and testing that the code actually works.

I tried to show that I had really thought through this change in the PR description where I left my rationale for each decision made on the PR, Let me know if in the future there's a better way to demonstrate that the PR has had someone actually think through it beyond just sending prompts 😅

To be honest, I'm hesitant to review PRs that are primarily AI-generated. Code review takes considerable time and effort

I should have maybe mentioned this in the PR description, but honestly before even going too deep into the code I would appreciate an initial high level review of the changes suggested, if this is something you would even be happy to merge in.

I think it would be very reasonable if you conclude that the ClientPersistAsset... changes are solving such a niche edge case that it's not worth merging (although I do believe other users might find value in this)

I don't see any downside to making AssetBatchSize configurable though, so maybe I split that into a separate PR?

@Zaida-3dO
Copy link
Author

Zaida-3dO commented Feb 2, 2026

Also I have a fork that includes this change running locally which I have been using for the last 2 weeks and haven't run into any major issues so far.

This isn't actually 100% true, there's an issue where every time my TV restarts it restores the current image but the timer resets 😞, I have the timer set to 24 hours, so this means if the TV doesn't stay on for 24 hours straight, I just get the same image forever.

This should be relatively easy to fix as a FLUP

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.

2 participants