Experiment/filter newest#733
Merged
Merged
Conversation
Contributor
|
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:
WalkthroughRenamed config field Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Request Handler
participant FA as fetchAssets()
participant Builder as Request Builder
participant Cache as Cache Layer
participant API as Immich API
Client->>FA: fetchAssets(requestID, deviceID, requestBody)
FA->>Builder: apply FilterDate, apply FilterNewest (truncate)
Builder->>Builder: marshal body, build query/hash -> apiURL
FA->>Cache: withImmichAPICache(apiURL, body)
alt Cache Hit
Cache-->>FA: (body, apiURL, true, nil)
else Cache Miss
Cache->>API: POST /search/random
API-->>Cache: response bytes
Cache-->>FA: (body, apiURL, false, nil)
FA->>FA: unmarshal, derive assets, shuffle
end
FA-->>Client: (assets, apiURL, error)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/immich/immich_helpers.go (1)
48-99:⚠️ Potential issue | 🟡 MinorKeep cache-hit state local to each wrapped call.
usingCacheis captured outside the returned function, so a reused wrapper can return a staletruecache-hit flag after a previous hit. Also pass through variadic headers on cache misses to preserve the wrapped API call contract.Proposed fix
- usingCache := false - return func(ctx context.Context, method, apiURL string, body []byte, headers ...map[string]string) ([]byte, string, bool, error) { + usingCache := false if !requestConfig.Kiosk.Cache { return immichAPICall(ctx, method, apiURL, body, headers...) } @@ - apiBody, contentType, _, err := immichAPICall(ctx, method, apiURL, body) + apiBody, contentType, _, err := immichAPICall(ctx, method, apiURL, body, headers...)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/immich/immich_helpers.go` around lines 48 - 99, The wrapper captures usingCache outside the returned function causing stale true values across calls and it also drops variadic headers on the cache-miss API call; fix by moving the usingCache := false declaration inside the returned anonymous func so each invocation starts with a fresh flag, and ensure all immichAPICall invocations inside the wrapper (notably the call that unmarshals and caches the response) forward the variadic headers parameter (headers...) so the wrapped function preserves its contract; keep the rest of the cache logic (apiCacheKey, cache.Get, cache.Set, json unmarshalling) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/config/config_validation.go`:
- Around line 683-693: The new filter_newest option is validated by
Config.checkFilterNewest but not exposed to env or docs; add the mapping
{"filter_newest", "KIOSK_FILTER_NEWEST"} into the bindVars list in config.go
(the env bindings block around bindVars) so KIOSK_FILTER_NEWEST will populate
Config.FilterNewest, and then add an example entry for filter_newest (with a
short comment describing it) to config.example.yaml, demo.config.yaml and
README.md so users see how to configure it; ensure the key name matches the
struct tag and the example uses the same numeric semantics as enforced by
checkFilterNewest.
In `@internal/config/config.go`:
- Around line 344-348: The FilterNewest field can be set from request query/form
and currently only clamped by checkFilterNewest() during Load(), so requests
like ?filter_newest=50000 bypass the 0–1000 limit; fix by enforcing the clamp
immediately after request binding (where e.Bind(c) is called) or inside the
request binder/validation path so every request runs checkFilterNewest() on the
bound config; specifically, ensure checkFilterNewest() is invoked (or its clamp
logic applied) on the FilterNewest field (0..1000) right after e.Bind(c) or in
the same request-handling path that consumes FilterNewest to prevent oversized
searches.
In `@internal/immich/immich_helpers.go`:
- Around line 272-302: When filterNewest is true, cached responses are
SearchMetadataResponse but the current logic only unmarshals to
SearchMetadataResponse when not usingCache; update the parsing after
immichAPICall (the block that handles apiBody, usingCache, err from
immichAPICall) to branch on filterNewest (not on usingCache) so that when
filterNewest is true you always json.Unmarshal apiBody into
SearchMetadataResponse and set immichAssets =
searchMetadataResponse.Assets.Items (then shuffle), otherwise unmarshal into
[]Asset; adjust the error handling calls to immichAPIFail accordingly (keep
references to immichAPICall, filterNewest, usingCache, SearchMetadataResponse,
immichAssets).
In `@internal/immich/immich_random.go`:
- Line 55: The cache key collision happens because FilterNewest caches a
SearchMetadataResponse under the API URL key, but fetchAssets later marshals and
writes []Asset under the same key; to fix, ensure a single payload shape per
cache key — either modify fetchAssets to transform the SearchMetadataResponse
into the same SearchMetadataResponse representation before caching (i.e.,
perform the conversion inside fetchAssets so it marshals and writes the
SearchMetadataResponse form rather than raw []Asset) or use a distinct cache key
for the []Asset payload (e.g., append a suffix like ":assets" when storing the
marshalled []Asset). Update the code paths in fetchAssets and the FilterNewest
wrapper so they consistently read/write the same payload type (reference
functions/methods FilterNewest and fetchAssets and the API URL cache key) and
remove any places that overwrite the existing cache key with a different JSON
shape.
In `@internal/immich/immich_rating.go`:
- Around line 74-77: The code calls immichAPIFail again after fetchAssets fails,
which masks the original response and adds a spurious second failure; instead,
when fetchAssets returns an error (in the block around immichAssets, apiURL, err
:= a.fetchAssets(...)), propagate that err directly from the current function
(e.g., return nil, err or return err per the current signature) and remove the
immichAPIFail(...) call there so the original fetchAssets error and its context
are preserved; keep immichAPIFail only where fetchAssets itself constructs and
returns those API-failure responses.
In `@internal/immich/immich_tag.go`:
- Around line 135-138: The code is re-wrapping an error from fetchAssets with
immichAPIFail (losing the original body context); instead propagate the original
error instead of re-processing it: in the block that checks the result of
fetchAssets (the variables immichAssets, apiURL, err returned by fetchAssets),
remove the call to immichAPIFail(immichAssets, err, nil, apiURL.String()) and
simply return the original error (propagate err) or the original return values
from the caller function so the fetchAssets-produced immichAPIFail/logging
remains authoritative; update the return in that if err != nil branch
accordingly using the function's existing return signature.
In `@internal/immich/immich.go`:
- Line 375: The wrapper withImmichAPICache currently stores usingCache as an
outer variable which makes the cache-hit boolean stateful across calls; change
withImmichAPICache in internal/immich/immich_helpers.go so that the returned
apiCall closure declares a fresh local usingCache (e.g., `usingCache := false`)
at the start of each invocation and sets/returns that value per call instead of
updating an outer-scope variable, ensuring the bool returned by the apiCall
accurately reflects the cache status for that single request (affecting apiCall
type, withImmichAPICache, and callers like fetchAssets).
In `@internal/routes/routes_asset_helpers.go`:
- Around line 249-264: In gatherRatedAssets, the rating bucket always uses the
full AssetsWithRatingCount which ignores requestConfig.FilterNewest; change the
count query to use the same "newest" filtering used for people/albums/tags/dates
so the weight respects d.requestConfig.FilterNewest and avoids the full-count
query. Concretely, update the call to d.immichAsset.AssetsWithRatingCount(...)
(and the resulting ratedAssetsCount) to use the filtered/newest-count variant or
pass the FilterNewest/NewestLimit parameters from d.requestConfig so the
returned count reflects the newest filter, and then use that filtered count when
appending the AssetWithWeighting.
---
Outside diff comments:
In `@internal/immich/immich_helpers.go`:
- Around line 48-99: The wrapper captures usingCache outside the returned
function causing stale true values across calls and it also drops variadic
headers on the cache-miss API call; fix by moving the usingCache := false
declaration inside the returned anonymous func so each invocation starts with a
fresh flag, and ensure all immichAPICall invocations inside the wrapper (notably
the call that unmarshals and caches the response) forward the variadic headers
parameter (headers...) so the wrapped function preserves its contract; keep the
rest of the cache logic (apiCacheKey, cache.Get, cache.Set, json unmarshalling)
unchanged.
🪄 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: 37217d19-a36f-41a7-bd23-0c06708785df
📒 Files selected for processing (20)
internal/config/config.gointernal/config/config_validation.gointernal/immich/immich.gointernal/immich/immich_album.gointernal/immich/immich_date.gointernal/immich/immich_faces.gointernal/immich/immich_favourites.gointernal/immich/immich_filter.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_helpers.gotaskfile.yml
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
config.schema.json (1)
157-159: Consider mirroring the Go validation bounds onfilter_newest.The runtime validation in
internal/config/config_validation.goclampsFilterNewestto[0, 1000], but the schema accepts any integer. Addingminimum/maximumhere would surface invalid configs at schema-validation time rather than silently clamping later.♻️ Proposed change
"filter_newest": { - "type": "integer" + "type": "integer", + "minimum": 0, + "maximum": 1000 },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config.schema.json` around lines 157 - 159, The JSON schema entry for "filter_newest" currently allows any integer while runtime validation in internal/config/config_validation.go clamps FilterNewest to [0,1000]; update the "filter_newest" schema definition to mirror that by adding "minimum": 0 and "maximum": 1000 alongside "type": "integer" so invalid configs are rejected at schema-validation time rather than silently clamped at runtime.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@config.schema.json`:
- Around line 157-159: The JSON schema entry for "filter_newest" currently
allows any integer while runtime validation in
internal/config/config_validation.go clamps FilterNewest to [0,1000]; update the
"filter_newest" schema definition to mirror that by adding "minimum": 0 and
"maximum": 1000 alongside "type": "integer" so invalid configs are rejected at
schema-validation time rather than silently clamped at runtime.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/routes/routes_asset_helpers.go`:
- Around line 102-106: The call to gatherRatedAssets inside the rating branch
currently logs and swallows errors (log.Error(err)) which hides user-facing
errors; change the behavior so that when requestConfig.Rating > -1 and
gatherRatedAssets(&d) returns an error you propagate/return that error (or wrap
it with context) instead of logging and continuing, ensuring the caller receives
the error and the rating-only request fails fast; update the enclosing
function's return path to return the propagated error from gatherRatedAssets
(reference gatherRatedAssets, requestConfig.Rating, and variable d) so
rating-bucket errors are surfaced to the client.
- Around line 109-112: The memories branch currently uses unfiltered counts
which skews selection in FilterNewest mode; update the memories weight
calculation and calls to getMemoriesAssetsCount to apply the same FilterNewest
filtering used by other buckets (i.e., compute counts using FilterNewest instead
of TotalAssetCount unless requestConfig.MemoriesOnly is true) so the injected
random/fallback weighting uses the filtered memory count; also apply the same
change in the other occurrences around the 277-294 region to ensure consistent
filtered counts for memories vs random mixing.
🪄 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: 0d4ebee4-51c2-4e3e-a56a-d32890df5ca5
📒 Files selected for processing (2)
internal/config/config.gointernal/routes/routes_asset_helpers.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/config/config.go
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
internal/immich/immich_helpers.go (1)
278-301:⚠️ Potential issue | 🔴 CriticalCache hits with
FilterNeweststill unmarshal the wrong shape.
withImmichAPICachere-marshals theSearchMetadataResponsejsonShape into the cache on a miss, so cache hits replay{"assets":{"items":[...]}}bytes. The current branch only decodes asSearchMetadataResponsewhen!usingCache; on any subsequent hit execution falls through tojson.Unmarshal(apiBody, &immichAssets)at line 296 and fails with a JSON shape mismatch, meaning every cachedFilterNewestrequest will error out until the cache expires. Branch onfilterNewestalone and keep the shuffle so repeated cached fetches still vary in order.🔧 Proposed fix
- if filterNewest && !usingCache { + _ = usingCache + if filterNewest { var searchMetadataResponse SearchMetadataResponse if err = json.Unmarshal(apiBody, &searchMetadataResponse); err != nil { log.Error("failed Unmarshal", "err", err) _, _, err = immichAPIFail(searchMetadataResponse, err, apiBody, apiURL.String()) return nil, url.URL{}, err } immichAssets = searchMetadataResponse.Assets.Items rand.Shuffle(len(immichAssets), func(i, j int) { immichAssets[i], immichAssets[j] = immichAssets[j], immichAssets[i] }) } else { if err = json.Unmarshal(apiBody, &immichAssets); err != nil { log.Error("failed Unmarshal", "err", err) _, _, err = immichAPIFail(immichAssets, err, apiBody, apiURL.String()) return nil, url.URL{}, err } }If the
usingCachesignal is no longer needed by any caller, you can also drop it fromimmichAPICall/withImmichAPICacheto simplify the API surface.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/immich/immich_helpers.go` around lines 278 - 301, The cached responses for FilterNewest are shaped as SearchMetadataResponse but the current logic only unmarshals into SearchMetadataResponse when !usingCache, causing cache hits to fail; modify the conditional to branch on filterNewest alone (not filterNewest && !usingCache): if filterNewest { unmarshal apiBody into SearchMetadataResponse, set immichAssets = searchMetadataResponse.Assets.Items and keep the rand.Shuffle block } else { unmarshal apiBody into immichAssets } — this change should be applied around the immichAPICall result handling in immich_helpers.go (symbols: immichAPICall, filterNewest, usingCache, apiBody, SearchMetadataResponse, immichAssets, immichAPIFail).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@internal/immich/immich_helpers.go`:
- Around line 278-301: The cached responses for FilterNewest are shaped as
SearchMetadataResponse but the current logic only unmarshals into
SearchMetadataResponse when !usingCache, causing cache hits to fail; modify the
conditional to branch on filterNewest alone (not filterNewest && !usingCache):
if filterNewest { unmarshal apiBody into SearchMetadataResponse, set
immichAssets = searchMetadataResponse.Assets.Items and keep the rand.Shuffle
block } else { unmarshal apiBody into immichAssets } — this change should be
applied around the immichAPICall result handling in immich_helpers.go (symbols:
immichAPICall, filterNewest, usingCache, apiBody, SearchMetadataResponse,
immichAssets, immichAPIFail).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 64adc5c6-84f9-49a5-b058-96adb2e65a9a
📒 Files selected for processing (2)
frontend/src/css/error.cssinternal/immich/immich_helpers.go
✅ Files skipped from review due to trivial changes (1)
- frontend/src/css/error.css
…-memories Feature/filter date applied memories
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Chores
Refactor
Style