feat: client-side persistence of the slideshow queue and history#664
feat: client-side persistence of the slideshow queue and history#664Zaida-3dO wants to merge 1 commit into
Conversation
Persist the asset backlog, history, and currently-displayed assets to the browser's localStorage so a refresh (or the frame's error-reload) resumes in place instead of re-fetching a fresh batch. - Add a ClientPersistAssets general setting (V2 and legacy V1 config) that gates the persisted stores - Add a server session ID generated at startup, exposed via the client config; clients clear their persisted assets when it changes, because the server's asset-routing tracker (BloomFilter) resets on restart and can no longer resolve stale persisted assets - Weave persist/restore into the v3 slideshow engine (home-page.svelte) - Persisted reads fall back to the default when the stored value is missing or corrupt - Update config-loader test resources for the new setting
📝 WalkthroughWalkthroughAdds a ChangesClient Persist Assets feature
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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: 1
🧹 Nitpick comments (1)
ImmichFrame.WebApi/Helpers/ServerSession.cs (1)
8-11: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueSession id is per-process, not per-cluster.
SessionIdis generated once per process instance. In a horizontally-scaled deployment without sticky routing, each replica would hand out a different id, causing clients to treat every request as a "server restart" and repeatedly clear persisted state. Given ImmichFrame's typical single-instance self-hosted deployment, this is a low-probability edge case, but worth a doc note if multi-instance/HA deployments are ever supported.🤖 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/ServerSession.cs` around lines 8 - 11, The ServerSession.SessionId value is generated per process, so in multi-replica deployments it can change across requests and look like a restart. Update the ServerSession class or its usage to document that SessionId is process-scoped, not cluster-scoped, and note the limitation wherever the session identity is exposed or relied on so future HA/multi-instance support doesn’t assume a stable cluster-wide id.
🤖 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.Web/src/lib/stores/persist.store.ts`:
- Around line 36-44: Wrap the localStorage write in persistArrayStore’s
subscribe callback in a try/catch so quota failures don’t escape synchronously.
Update persistArrayStore to catch errors from localStorage.setItem, and handle
them gracefully (for example by logging or ignoring) while keeping
assetBacklogStore, assetHistoryStore, and displayingAssetsStore transitions
alive.
---
Nitpick comments:
In `@ImmichFrame.WebApi/Helpers/ServerSession.cs`:
- Around line 8-11: The ServerSession.SessionId value is generated per process,
so in multi-replica deployments it can change across requests and look like a
restart. Update the ServerSession class or its usage to document that SessionId
is process-scoped, not cluster-scoped, and note the limitation wherever the
session identity is exposed or relied on so future HA/multi-instance support
doesn’t assume a stable cluster-wide id.
🪄 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: 3fe47f3b-3cfe-4b5e-951b-81f16e58be19
📒 Files selected for processing (16)
ImmichFrame.Core/Interfaces/IServerSettings.csImmichFrame.WebApi.Tests/Resources/TestV1.jsonImmichFrame.WebApi.Tests/Resources/TestV2.jsonImmichFrame.WebApi.Tests/Resources/TestV2.ymlImmichFrame.WebApi/Controllers/ConfigController.csImmichFrame.WebApi/Helpers/Config/ServerSettingsV1.csImmichFrame.WebApi/Helpers/ServerSession.csImmichFrame.WebApi/Models/ClientSettingsDto.csImmichFrame.WebApi/Models/ServerSettings.csImmichFrame.WebApi/Program.csdocker/Settings.example.jsondocker/Settings.example.ymldocs/docs/getting-started/configuration.mdimmichFrame.Web/src/lib/components/home-page/home-page.svelteimmichFrame.Web/src/lib/immichFrameApi.tsimmichFrame.Web/src/lib/stores/persist.store.ts
| function persistArrayStore<T>(key: string, defaultValue: T[]) { | ||
| const store = writable(loadPersistedArray(key, defaultValue)); | ||
|
|
||
| store.subscribe((value) => { | ||
| localStorage?.setItem(key, JSON.stringify(value)); | ||
| }); | ||
|
|
||
| return store; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does localStorage.setItem throw QuotaExceededError when storage limit is exceeded?
💡 Result:
Yes, localStorage.setItem throws a QuotaExceededError when the storage limit is exceeded [1][2][3]. According to the HTML Standard and Web Storage API specifications, if a browser cannot set a new value—either because the storage quota has been reached or because storage has been disabled for the site—it must throw a QuotaExceededError [2][3][4]. To handle this, it is recommended to always wrap calls to localStorage.setItem in a try...catch block [1][5][6]. When implementing error handling, note the following: - In modern browsers, this error is typically represented as a DOMException with the name QuotaExceededError [7]. - Some older browser implementations or specific conditions (such as private browsing mode) may exhibit variations in how this error is surfaced or named (e.g., NS_ERROR_DOM_QUOTA_REACHED in older Firefox versions) [8][9]. - Because localStorage might be unavailable even if the feature check succeeds (e.g., due to user settings or private browsing restrictions), a robust practice is to test for storage availability by attempting to set and then remove a test item before use [10].
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/API/Storage_API/Storage_quotas_and_eviction_criteria
- 2: https://html.spec.whatwg.org/multipage/webstorage.html
- 3: https://www.w3.org/TR/2016/REC-webstorage-20160419/
- 4: https://html.spec.whatwg.org/dev/webstorage.html
- 5: https://mmazzarolo.com/blog/2022-06-25-local-storage-status/
- 6: https://thevalleyofcode.com/lesson/storage-api/storage-size-limits/
- 7: https://developer.mozilla.org/en-US/docs/Web/API/QuotaExceededError
- 8: http://crocodillon.com/blog/always-catch-localstorage-security-and-quota-exceeded-errors
- 9: https://stackoverflow.com/questions/14555347/html5-localstorage-error-with-safari-quota-exceeded-err-dom-exception-22-an
- 10: https://developer.mozilla.org/en-US/docs/Web/API/Web_Storage_API/Using_the_Web_Storage_API
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the target file and related callers/usages.
git ls-files | rg 'immichFrame\.Web/src/lib/stores/persist\.store\.ts|immichFrame\.Web/src/lib/stores|persistHistory|persistBacklog|getNextAssets|handleDone|AssetResponseDto'
echo '--- persist.store.ts ---'
cat -n immichFrame.Web/src/lib/stores/persist.store.ts
echo '--- search for persistArrayStore usage ---'
rg -n "persistArrayStore|persistHistory|persistBacklog|localStorage\.?\.setItem|AssetResponseDto" immichFrame.Web/src -A 4 -B 4
echo '--- any explicit storage quota handling in repo ---'
rg -n "QuotaExceededError|quota|localStorage" immichFrame.Web/src -A 2 -B 2Repository: immichFrame/ImmichFrame
Length of output: 31658
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- AssetResponseDto shape ---'
sed -n '90,170p' immichFrame.Web/src/lib/immichFrameApi.ts
echo '--- home-page persistence flow ---'
sed -n '140,305p' immichFrame.Web/src/lib/components/home-page/home-page.svelteRepository: immichFrame/ImmichFrame
Length of output: 6809
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "handleDone\\(" immichFrame.Web/src/lib/components/home-page/home-page.svelte -A 3 -B 3Repository: immichFrame/ImmichFrame
Length of output: 1408
Wrap the localStorage.setItem write in a try/catch.
assetBacklogStore, assetHistoryStore, and displayingAssetsStore persist full AssetResponseDto[] payloads, so a large queue can hit the localStorage quota and throw synchronously inside subscribe(). That rejection will escape persistBacklog()/persistHistory() and abort the transition flow instead of degrading gracefully.
🤖 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.Web/src/lib/stores/persist.store.ts` around lines 36 - 44, Wrap
the localStorage write in persistArrayStore’s subscribe callback in a try/catch
so quota failures don’t escape synchronously. Update persistArrayStore to catch
errors from localStorage.setItem, and handle them gracefully (for example by
logging or ignoring) while keeping assetBacklogStore, assetHistoryStore, and
displayingAssetsStore transitions alive.
1 participant
Summary
Adds an opt-in setting that lets the web client persist its slideshow state to the browser's
localStorage, so a refresh — or the frame's built-in error-reload (window.location.reload()) — resumes exactly where it left off instead of re-fetching a fresh random batch.ClientPersistAssets— persists the pending asset queue, the currently-displayed asset(s), and the "previous" history so back-navigation survives a reload.Defaults to
false(fully backward compatible).Server session ID
Persisted asset IDs are only meaningful to the server instance that handed them out — the server's asset-routing tracker (BloomFilter) is rebuilt on restart, after which it can no longer resolve those IDs. To handle this, the server now generates a session ID at startup and exposes it in the client config; when the client sees it change, it clears its persisted assets before restoring.
Alternative considered
Instead of storing the queue on the client, we could have the server keep an asset history and asset queue per connected client. This would also have the added benefit of potentially allowing the server to dedupe across asset batches if a user wanted to. However, this would require a way to uniquely identify clients across restarts and I can't think of a clean way to do that.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation