Feature/persistent cache#754
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds optional disk-backed persistence for the in-memory kiosk cache, exposes Asset internals as exported fields ( ChangesPersistent Cache & Asset Field Refactor
Sequence Diagram — persistence flow sequenceDiagram
participant App as Application (main)
participant Cache as cache.Initialize
participant Ticker as PeriodicSaver
participant Save as SaveToDisk
participant Gob as gob.Encoder
App->>Cache: Initialize(ctx, persistentCache=true)
Cache->>Ticker: start background ticker goroutine
loop every interval or on ctx.Done()
Ticker->>Save: SaveToDisk()
Save->>Gob: Encode persistedCache
Gob->>Save: Write file at PersistentCacheFilePath
end
App->>Cache: RegisterPersistence(marshal, unmarshal)
App->>Cache: LoadFromDisk()
App->>Cache: cache.Wait() on shutdown
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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.
Actionable comments posted: 4
🧹 Nitpick comments (1)
internal/common/common.go (1)
107-114: ⚡ Quick winRemove the ViewDataToBytes and ViewDataFromBytes helpers or make them symmetric for potential future use.
These helpers are currently asymmetric—
ViewDataToBytesaccepts a singleViewDatawhilstViewDataFromBytesreturns a slice[]ViewData. However, the actual persistence layer uses separatepersistentCacheMarshelandpersistentCacheUnmarshalfunctions that are already symmetric (both work with[]ViewData). Since these helpers are unused in the codebase, either remove them or refactorViewDataToBytesto accept a slice for consistency if they're intended as public utilities.🤖 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 `@internal/common/common.go` around lines 107 - 114, The helpers ViewDataToBytes and ViewDataFromBytes are asymmetric (one handles ViewData, the other []ViewData) and unused; either delete both functions or make them symmetric to match the persistence helpers (persistentCacheMarshel and persistentCacheUnmarshal). To fix, choose one approach: (A) remove ViewDataToBytes and ViewDataFromBytes entirely if unused, or (B) refactor ViewDataToBytes to accept []ViewData and return json.Marshal([]ViewData) so its signature and behavior mirror ViewDataFromBytes and the persistentCache* helpers; update any callers accordingly.
🤖 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 `@config.schema.json`:
- Around line 594-596: The JSON schema property "persistant_cache" is misspelled
and must be renamed to "persistent_cache" to match the config struct/env
binding; update the property key in config.schema.json from "persistant_cache"
to "persistent_cache" so validation (with additionalProperties: false) succeeds
and the setting maps correctly to your code.
In `@internal/cache/cache.go`:
- Around line 87-98: The current logic in internal/cache/cache.go checks
os.Stat(PersistentCacheBaseDir) and returns early if it doesn't exist, which
prevents creating the persistence directories on first run; change this to
attempt to create the base directory instead of returning (e.g., if
os.IsNotExist(err) then call os.MkdirAll(PersistentCacheBaseDir, 0755) and
handle errors), then proceed to ensure PersistentCacheDir exists (remove the
early return), and update the error log calls around PersistentCacheDir creation
(the block referencing PersistentCacheBaseDir and PersistentCacheDir) to report
the actual error when MkdirAll fails so persistence is created on first run.
- Around line 323-327: FlushDisk currently removes a file from the current
working directory instead of the actual persisted cache file; update the
FlushDisk function to remove the canonical persisted cache path (use the
existing PersistantCacheFile full path or construct it via filepath.Join with
the app data/cache location) and call os.Remove on that resolved path
(optionally using filepath.Abs or a helper getter to ensure the path points to
data/cache/...), keeping the same error logging (log.Error) on failure;
reference the FlushDisk function and the PersistantCacheFile identifier when
making the change.
In `@main.go`:
- Around line 375-392: The current check treats a successfully unmarshaled empty
[]common.ViewData (vd) as a failure because of "len(vd) > 0", causing valid
empty slices to fall through to the []byte branch; change the condition to
accept any successful Unmarshal (i.e., use "err == nil" only) so empty slices
are returned as valid. Update the block that iterates vd (the loop that assigns
viewData.Config = *baseConfig and sets asset.ImmichAsset.Ctx / RequestConfig) to
still work correctly when vd is empty (it will naturally no-op), and remove
reliance on len(vd) to decide success; keep the existing fallback to
unmarshaling into raw []byte only when json.Unmarshal into vd actually errors.
This touches the vd variable, the viewData loop, and the assignments to
viewData.Config and asset.ImmichAsset.{Ctx,RequestConfig}.
---
Nitpick comments:
In `@internal/common/common.go`:
- Around line 107-114: The helpers ViewDataToBytes and ViewDataFromBytes are
asymmetric (one handles ViewData, the other []ViewData) and unused; either
delete both functions or make them symmetric to match the persistence helpers
(persistentCacheMarshel and persistentCacheUnmarshal). To fix, choose one
approach: (A) remove ViewDataToBytes and ViewDataFromBytes entirely if unused,
or (B) refactor ViewDataToBytes to accept []ViewData and return
json.Marshal([]ViewData) so its signature and behavior mirror ViewDataFromBytes
and the persistentCache* helpers; update any callers accordingly.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 2b1e2b6a-832c-4a5d-87cf-85cc9a4ba954
📒 Files selected for processing (29)
.gitignoreconfig.schema.jsondocker-compose.yamlinternal/cache/cache.gointernal/cache/cache_test.gointernal/common/common.gointernal/config/config.gointernal/immich/immich.gointernal/immich/immich_album.gointernal/immich/immich_cache.gointernal/immich/immich_date.gointernal/immich/immich_faces.gointernal/immich/immich_favourites.gointernal/immich/immich_helpers.gointernal/immich/immich_memories.gointernal/immich/immich_person.gointernal/immich/immich_random.gointernal/immich/immich_rating.gointernal/immich/immich_server.gointernal/immich/immich_statistics.gointernal/immich/immich_tag.gointernal/immich/immich_user.gointernal/immich/immich_video.gointernal/routes/routes_asset.gointernal/routes/routes_asset_helpers.gointernal/routes/routes_cache.gointernal/routes/routes_test.gomain.gotaskfile.yml
💤 Files with no reviewable changes (1)
- internal/routes/routes_asset.go
Summary by CodeRabbit
New Features
Chores