Support selecting an Immich album for shared trip photos - #3259
cleniemeyer-collab wants to merge 4 commits into
Conversation
📝 WalkthroughWalkthroughShared-link photo settings now support public and family scopes, resolve corresponding Immich tags, apply tag-aware cached searches, and lazy-load trip photos when day sections open. ChangesShared-link photo scoping
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant SharedLink as Shared link
participant PhotoScope as SharedLinks::PhotoScope
participant Tags as Immich::Tags
participant Search as Photos::Search
participant Immich as Immich::RequestPhotos
SharedLink->>PhotoScope: Resolve configured photo scope
PhotoScope->>Tags: Fetch cached Immich tags
PhotoScope-->>Search: Provide resolved tag IDs
Search->>Immich: Search with tagIds
Immich-->>Search: Return tagged assets
Search-->>SharedLink: Render scoped photos
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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: 3
🧹 Nitpick comments (4)
app/controllers/concerns/share_links/managable.rb (1)
144-145: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated scope whitelist — single source of truth suggested.
%w[public family]duplicates the valid-scopes list already encapsulated inSharedLinks::PhotoScope(VALID_SCOPES, per the referenced snippet inapp/services/shared_links/photo_scope.rb). If a new scope is added there, this literal must be updated too or requests will silently fall back topublic.♻️ Proposed fix referencing the shared constant
- scope = values['photo_scope'].to_s - settings['photo_scope'] = %w[public family].include?(scope) ? scope : 'public' + scope = values['photo_scope'].to_s + settings['photo_scope'] = + SharedLinks::PhotoScope::VALID_SCOPES.include?(scope) ? scope : 'public'🤖 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 `@app/controllers/concerns/share_links/managable.rb` around lines 144 - 145, Update the photo_scope validation in the settings assignment to use SharedLinks::PhotoScope::VALID_SCOPES instead of the duplicated %w[public family] literal, preserving the existing fallback to "public".app/views/shared/links/_trip.html.erb (1)
105-137: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a Stimulus controller instead of a raw inline
<script>.The logic itself is correct (capture-phase listener properly handles the non-bubbling
toggleevent; content is cloned once and cleared on close). However, the rest of this file and the surrounding Hotwire app rely on Stimulus controllers/data-actionbindings rather than ad-hoc global listeners. Wrapping this in a small Stimulus controller (e.g.trip-day-photos_controller.jswith atoggleaction) would be more consistent with the codebase's conventions, easier to test, and avoids the manual "install once onwindow" guard.🤖 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 `@app/views/shared/links/_trip.html.erb` around lines 105 - 137, Replace the inline script’s global toggle listener and window installation guard with a Stimulus controller, such as trip-day-photos_controller.js, and bind its toggle action through the existing data-action conventions. Move the open/close behavior into the controller while preserving the HTMLDetailsElement check, lazy template cloning, and clearing the container when closed.app/services/photos/search.rb (2)
53-79: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding spec coverage for the new multi-tag aggregation path.
request_immich/request_immich_for_multiple_tagsintroduce non-trivial branching (single vs. multi-tag, dedupe-by-id, nil short-circuiting on partial failure) flagged as high complexity. A few unit tests covering: single tag, multiple tags with overlap, and one tag request failing (should abort the whole search) would guard this logic going forward.🤖 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 `@app/services/photos/search.rb` around lines 53 - 79, Add unit specs for the Immich request flow covering single-tag requests, multi-tag aggregation with overlapping assets deduplicated by ID, and a multi-tag request where one tag returns nil and the overall search aborts. Anchor the tests to request_immich and request_immich_for_multiple_tags, preserving the existing single-tag behavior and nil short-circuit.
53-79: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRun the per-tag Immich lookups concurrently.
request_immich_for_multiple_tagssends one blockingHTTParty.postrequest per tag, each with a 10s timeout. For 2+ tags, a cache-miss Immich request can take 20s+ before the per-route 30-minute cache is written. Bound the worst-case to a single request timeout by firing the per-tag searches concurrently.Don’t change the union strategy to a multi-value
tagIdsrequest: Immich’s search metadata filter requires assets to match all supplied tag IDs, so Union-or behavior only works with separate per-tag requests.🤖 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 `@app/services/photos/search.rb` around lines 53 - 79, Update request_immich_for_multiple_tags to execute each request_immich_assets([tag_id]) lookup concurrently while preserving the existing union behavior by concatenating results from separate per-tag searches. Propagate a nil result as failure, wait for all lookups, and return the combined assets without changing the single-tag path.
🤖 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 `@app/services/photos/mappable.rb`:
- Line 4: Revert the MAX_PHOTOS increase in Photos::Mappable and decouple the
map-marker limit from the gallery limit. Preserve the existing capped_geotagged
default used by the shared photos controller, while allowing the gallery call
site to use its larger explicit limit through a separate constant or parameter.
In `@app/services/shared_links/trip_photos.rb`:
- Around line 36-44: Update app/services/shared_links/trip_photos.rb lines 36-44
and app/controllers/api/v1/shared/photos_controller.rb lines 71-80 so
PhotoPrism-only configurations still resolve shared photos through
Photos::Search when SharedLinks::PhotoScope#tag_ids returns nil. Replace the
unconditional empty-result path with the appropriate PhotoPrism fallback, while
preserving the existing Immich tag-filtered behavior.
In `@app/views/shared_links/_photo_scope_fields.html.erb`:
- Around line 6-42: Update the photo scope fields partial around the hidden
fields and photo selection radio buttons to accept the current shared-link
settings and use them when rendering. Initialize show_photos, photo_scope, and
the checked radio choice from those settings, preserving the existing defaults
only when no prior values are available so validation rerenders retain public or
family selections.
---
Nitpick comments:
In `@app/controllers/concerns/share_links/managable.rb`:
- Around line 144-145: Update the photo_scope validation in the settings
assignment to use SharedLinks::PhotoScope::VALID_SCOPES instead of the
duplicated %w[public family] literal, preserving the existing fallback to
"public".
In `@app/services/photos/search.rb`:
- Around line 53-79: Add unit specs for the Immich request flow covering
single-tag requests, multi-tag aggregation with overlapping assets deduplicated
by ID, and a multi-tag request where one tag returns nil and the overall search
aborts. Anchor the tests to request_immich and request_immich_for_multiple_tags,
preserving the existing single-tag behavior and nil short-circuit.
- Around line 53-79: Update request_immich_for_multiple_tags to execute each
request_immich_assets([tag_id]) lookup concurrently while preserving the
existing union behavior by concatenating results from separate per-tag searches.
Propagate a nil result as failure, wait for all lookups, and return the combined
assets without changing the single-tag path.
In `@app/views/shared/links/_trip.html.erb`:
- Around line 105-137: Replace the inline script’s global toggle listener and
window installation guard with a Stimulus controller, such as
trip-day-photos_controller.js, and bind its toggle action through the existing
data-action conventions. Move the open/close behavior into the controller while
preserving the HTMLDetailsElement check, lazy template cloning, and clearing the
container when closed.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8ee01dfc-1083-4453-b46d-4fe98ebabeed
📒 Files selected for processing (14)
app/controllers/api/v1/shared/photos_controller.rbapp/controllers/concerns/share_links/managable.rbapp/models/shared_link.rbapp/services/immich/request_photos.rbapp/services/immich/tags.rbapp/services/photos/mappable.rbapp/services/photos/search.rbapp/services/shared_links/photo_scope.rbapp/services/shared_links/trip_photos.rbapp/views/shared/links/_trip.html.erbapp/views/shared_links/_modal_timeline_create_form.html.erbapp/views/shared_links/_modal_track_create_form.html.erbapp/views/shared_links/_modal_trip_create_form.html.erbapp/views/shared_links/_photo_scope_fields.html.erb
| tag_ids = SharedLinks::PhotoScope.new(@link).tag_ids | ||
| return [] if tag_ids.nil? | ||
|
|
||
| Photos::Search.cached( | ||
| @link.user, | ||
| start_date: trip.started_at.iso8601, | ||
| end_date: trip.ended_at.iso8601, | ||
| tag_ids: tag_ids | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== candidate files =="
git ls-files | rg '(^app/services/shared_links/trip_photos\.rb$|^app/controllers/api/v1/shared/photos_controller\.rb$|SharedLinks|Immich|Photos::Search|PhotoScope)' || true
echo "== trip_photos outline =="
ast-grep outline app/services/shared_links/trip_photos.rb || true
echo
echo "== photos_controller outline =="
ast-grep outline app/controllers/api/v1/shared/photos_controller.rb || true
echo
echo "== relevant lines =="
cat -n app/services/shared_links/trip_photos.rb | sed -n '1,120p'
echo "---------- photos_controller ----------"
cat -n app/controllers/api/v1/shared/photos_controller.rb | sed -n '1,130p'Repository: Freika/dawarich
Length of output: 7346
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== broad searches =="
rg -n "class PhotoScope|module PhotoScope|SharedLinks::PhotoScope|def tag_ids|tag_ids =|Immich::Tags|Photos::Search\.cached|configurate|configured|immich|photo_prism|PhotoPrism" app lib config test spec || true
echo "== locate immich/photo services/controllers =="
git ls-files app lib | rg -i 'immich|photo_prism|photos|search' || trueRepository: Freika/dawarich
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== photo_scope.rb =="
cat -n app/services/shared_links/photo_scope.rb | sed -n '1,80p'
echo
echo "== photos/search.rb relevant =="
cat -n app/services/photos/search.rb | sed -n '1,120p'
echo
echo "== trip_photos_spec relevant =="
cat -n spec/services/shared_links/trip_photos_spec.rb | sed -n '1,120p'
echo
echo "== photo_scope tests if any =="
rg -n "SharedLinks::PhotoScope|SharedLinks\\:\\:PhotoScope|class SharedLinks/PhotoScope|module SharedLinks/PhotoScope|shared_links.*photo_scope|PhotoScope" spec app lib 2>/dev/null | head -200 || true
echo "== focused test files mentioning missing immich in shared/trip =="
grep -RIn "photo_prism\\|photoprism\\|only.*photo\\|no.*immich\\|Immich integration" spec app services/controllers -i 2>/dev/null | rg -i "shared_links|trip_photos|shared/photos|photoprism|unique" | head -150 || trueRepository: Freika/dawarich
Length of output: 25791
Handle PhotoPrism-only sharing before returning an empty result. SharedLinks::PhotoScope#tag_ids resolves tags only through Immich::Tags, falls back to nil on missing Immich config, and both shared photo paths return [] in that case. Since Photos::Search supports PhotoPrism directly, PhotoPrism-only users will see no shared trip/map photos unless Immich is configured; add a PhotoPrism-only fallback or ensure the empty state is intentional and communicated.
app/services/shared_links/trip_photos.rb#L36-L44app/controllers/api/v1/shared/photos_controller.rb#L71-L80
📍 Affects 2 files
app/services/shared_links/trip_photos.rb#L36-L44(this comment)app/controllers/api/v1/shared/photos_controller.rb#L71-L80
🤖 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 `@app/services/shared_links/trip_photos.rb` around lines 36 - 44, Update
app/services/shared_links/trip_photos.rb lines 36-44 and
app/controllers/api/v1/shared/photos_controller.rb lines 71-80 so
PhotoPrism-only configurations still resolve shared photos through
Photos::Search when SharedLinks::PhotoScope#tag_ids returns nil. Replace the
unconditional empty-result path with the appropriate PhotoPrism fallback, while
preserving the existing Immich tag-filtered behavior.
| <%= hidden_field_tag 'shared_link[settings][show_photos]', | ||
| '0', | ||
| data: { photo_scope_show_photos: true } %> | ||
|
|
||
| <%= hidden_field_tag 'shared_link[settings][photo_scope]', | ||
| 'public', | ||
| data: { photo_scope_value: true } %> | ||
|
|
||
| <label class="label cursor-pointer justify-start gap-3"> | ||
| <%= radio_button_tag "#{id_prefix}_photo_selection", | ||
| 'none', | ||
| true, | ||
| id: "#{id_prefix}_photo_selection_none", | ||
| class: 'radio', | ||
| data: { photo_scope_choice: true } %> | ||
| <span class="label-text">No photos</span> | ||
| </label> | ||
|
|
||
| <label class="label cursor-pointer justify-start gap-3"> | ||
| <%= radio_button_tag "#{id_prefix}_photo_selection", | ||
| 'public', | ||
| false, | ||
| id: "#{id_prefix}_photo_selection_public", | ||
| class: 'radio', | ||
| data: { photo_scope_choice: true } %> | ||
| <span class="label-text">Public photos only</span> | ||
| </label> | ||
|
|
||
| <label class="label cursor-pointer justify-start gap-3"> | ||
| <%= radio_button_tag "#{id_prefix}_photo_selection", | ||
| 'family', | ||
| false, | ||
| id: "#{id_prefix}_photo_selection_family", | ||
| class: 'radio', | ||
| data: { photo_scope_choice: true } %> | ||
| <span class="label-text">Public + family photos</span> | ||
| </label> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Preserve the selected photo scope on validation rerenders.
This always submits show_photos=0/photo_scope=public and selects “No photos.” The timeline form re-renders invalid @shared_link state, so a previously selected family/public scope is silently reset on retry. Pass the current settings into this partial and initialize both hidden values and the checked radio from them.
🤖 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 `@app/views/shared_links/_photo_scope_fields.html.erb` around lines 6 - 42,
Update the photo scope fields partial around the hidden fields and photo
selection radio buttons to accept the current shared-link settings and use them
when rendering. Initialize show_photos, photo_scope, and the checked radio
choice from those settings, preserving the existing defaults only when no prior
values are available so validation rerenders retain public or family selections.
1 participant
Summary
This PR replaces the previous tag-based photo filtering with optional Immich album selection for shared trips.
Features
Notes
The album filtering is currently implemented in the view to keep the change small and easy to review. It could later be moved into the Immich::Albums service if preferred.
Summary by CodeRabbit