Add Immich album selection for shared trip photosFeature/immich share photo albums - #3269
Conversation
📝 WalkthroughWalkthroughThe change adds Immich shared-link retrieval and selection to shared-link forms, scopes photo searches by album and source, generates public shared-link asset URLs, and updates the shared-trip lightbox. It also updates development-container dependencies and adds German repository documentation. ChangesRepository foundation
Shared Immich photos
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
🧪 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: 8
🧹 Nitpick comments (6)
app/views/shared/links/_trip.html.erb (1)
109-146: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueModal lacks focus trap on open.
The overlay is
role="dialog"/aria-modal="true"but nothing moves focus into it or constrains Tab within it when opened, so keyboard users can tab into the hidden background content behind the modal. Escape and click-based close still work, so this doesn't block completion, but a focus trap (e.g., focus the close button on open, restore focus on close) would improve compliance.🤖 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 109 - 146, Update the shared photo overlay behavior associated with data-shared-photo-overlay to move focus to data-shared-photo-close when opening, trap Tab and Shift+Tab within the dialog’s focusable controls, and restore focus to the element that triggered the overlay when closing. Preserve the existing Escape and click-to-close behavior.app/views/shared_links/_photo_scope_fields.html.erb (1)
5-86: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftExtract album option-building logic out of the view.
This block builds album option data (id/name/date-range extraction, trip-window filtering, sorting) entirely in ERB-embedded Ruby, shared across three create-form partials. Moving it into a plain Ruby object (e.g.,
SharedLinks::AlbumOptions.new(albums, resource).call) would make it unit-testable and avoid re-implementing the same fallback logic if this partial is duplicated or extended later.🤖 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 5 - 86, Extract the album option-building logic from the ERB partial into a plain Ruby object such as SharedLinks::AlbumOptions with a call method accepting albums and resource. Move trip boundary extraction, album id/name/date and asset-count fallback handling, date-range filtering, invalid-date recovery, and case-insensitive sorting into that object, then have the partial obtain album_options through it while preserving the existing option structure and filtering behavior.app/views/shared_links/_photo_scope_fields.html.erb.bak (1)
1-91: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the committed backup template.
This stale implementation can be accidentally restored or edited instead of the active partial; Git already retains its history.
Based on learnings: “Parallele oder doppelte Implementierungen vermeiden und bestehendes Verhalten erhalten, sofern keine ausdrückliche Verhaltensänderung verlangt wird.”
🤖 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.bak` around lines 1 - 91, Remove the committed backup template _photo_scope_fields.html.erb.bak entirely; do not modify or duplicate the active photo-scope partial, and preserve the existing implementation and behavior there.Source: Learnings
spec/requests/api/v1/shared/photos_spec.rb (1)
137-144: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid asserting
Photos::Searchconstructor wiring.This locks the request spec to internal keywords instead of observable API behavior. Exercise the endpoint with an external-boundary stub and assert the returned, range-filtered photos instead.
As per coding guidelines: “Don't test wiring without outcomes. Verify the returned data or state change instead of just asserting that a method was called.”
🤖 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 `@spec/requests/api/v1/shared/photos_spec.rb` around lines 137 - 144, Update the example around the Photos::Search setup to stop asserting the constructor’s keyword arguments or call wiring. Stub the external photo-search boundary with the needed range-filtered result, exercise the endpoint normally, and assert that the response returns the expected photos.Source: Coding guidelines
spec/services/photos/search_spec.rb (1)
113-147: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest source-specific results, not requester construction.
The
not_to receive(:new)assertions test internal wiring. The distinct serialized fixtures and exact returned arrays already verify the selected-source behavior; remove the constructor expectations.As per coding guidelines: “Don't test wiring without outcomes. Verify the returned data or state change instead of just asserting that a method was called.”
🤖 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 `@spec/services/photos/search_spec.rb` around lines 113 - 147, Remove the Immich::RequestPhotos and Photoprism::RequestPhotos constructor expectations from the source-selection examples in the search spec. Keep the distinct serialized fixture assertions and exact returned arrays in the PhotoPrism-only, Immich-only, and empty-source cases as the outcome-based verification.Source: Coding guidelines
spec/requests/shared/links_spec.rb (1)
127-130: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftAvoid mocking internal photo services in these behavior specs. Both tests bypass production filtering and couple to internal delegation, so scope/search integration regressions can still pass.
spec/requests/shared/links_spec.rb#L127-L130: stub the Immich HTTP boundary or use a production-shaped fixture, then exercise the realSharedLinks::TripPhotospath.spec/services/shared_links/trip_photos_spec.rb#L55-L62: assertTripPhotos#calloutput with an external-boundary stub; move request-payload assertions to thePhotos::Searchintegration spec.As per coding guidelines, “Mock only at external boundaries … not internal collaborators.”
🤖 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 `@spec/requests/shared/links_spec.rb` around lines 127 - 130, Remove the internal SharedLinks::TripPhotos mock in spec/requests/shared/links_spec.rb lines 127-130; stub the Immich HTTP boundary or use a production-shaped fixture and exercise the real TripPhotos path. In spec/services/shared_links/trip_photos_spec.rb lines 55-62, test TripPhotos#call output with an external-boundary stub and move request-payload assertions to the Photos::Search integration spec.Source: Coding guidelines
🤖 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 `@AI/DEVELOPMENT.md`:
- Around line 112-114: Update the testvscode documentation in AI/DEVELOPMENT.md
to reflect its dependency on devcontainer: it may build the devcontainer image
when the DEV_CONTAINER is not already running, while avoiding image builds only
when that container is already available. Preserve the existing statements about
running specs in dawarich_dev and not creating or deleting volumes.
In `@app/controllers/concerns/share_links/managable.rb`:
- Around line 136-150: Preserve the selected photo scope during extraction by
permitting and validating photo_scope in the shared-link parameters handled by
the relevant concern. In app/services/shared_links/trip_photos.rb lines 36-45,
resolve SharedLinks::PhotoScope#tag_ids, return an empty result when resolution
fails, and pass the resolved IDs as tag_ids to Photos::Search.cached; both sites
require changes.
In `@app/services/immich/albums.rb`:
- Around line 15-16: Preserve successful empty cache responses by changing the
cached-value checks in app/services/immich/albums.rb lines 15-16 and
app/services/immich/tags.rb lines 15-16 to return the cached value whenever it
is not nil, rather than using present?.
In `@app/services/photos/mappable.rb`:
- Line 4: Reduce Photos::Mappable’s MAX_PHOTOS constant from 2000 back to the
existing 100-photo cap used for public shared links/maps, preserving the current
first(`@max`) response behavior and avoiding broader pagination changes.
In `@app/views/shared/links/_trip.html.erb`:
- Around line 130-138: Replace the inline SVG markup in the trip view with an
asset-based render using inline_svg_tag. Save the Immich logo as an SVG under
app/assets/svg/icons and reference that asset by name, preserving the existing
sizing, accessibility attributes, and styling.
- Around line 148-192: Move the overlay, preview, and immich-link querySelector
calls out of the once-per-page initialization and re-query those elements inside
openPhoto and closePhoto. Keep each handler’s existing behavior and guards
intact so it operates on the current Turbo-rendered DOM.
In `@Makefile`:
- Around line 22-26: Add spec/requests/shared/links_spec.rb and
spec/services/shared_links/photo_album_spec.rb to the RSPEC_FILES list so both
testdawarich and dawarich execute the complete shared photo behavior suite.
In `@spec/requests/shared/links_spec.rb`:
- Around line 141-147: Update the all-photos example in the shared-link spec to
configure an Immich source fixture with a returned photo asset and URL, ensuring
the overlay link is genuinely rendered. Keep the no-album-selected scenario, and
assert that the response omits both the Immich album control and the Immich
photo deep link.
---
Nitpick comments:
In `@app/views/shared_links/_photo_scope_fields.html.erb`:
- Around line 5-86: Extract the album option-building logic from the ERB partial
into a plain Ruby object such as SharedLinks::AlbumOptions with a call method
accepting albums and resource. Move trip boundary extraction, album id/name/date
and asset-count fallback handling, date-range filtering, invalid-date recovery,
and case-insensitive sorting into that object, then have the partial obtain
album_options through it while preserving the existing option structure and
filtering behavior.
In `@app/views/shared_links/_photo_scope_fields.html.erb.bak`:
- Around line 1-91: Remove the committed backup template
_photo_scope_fields.html.erb.bak entirely; do not modify or duplicate the active
photo-scope partial, and preserve the existing implementation and behavior
there.
In `@app/views/shared/links/_trip.html.erb`:
- Around line 109-146: Update the shared photo overlay behavior associated with
data-shared-photo-overlay to move focus to data-shared-photo-close when opening,
trap Tab and Shift+Tab within the dialog’s focusable controls, and restore focus
to the element that triggered the overlay when closing. Preserve the existing
Escape and click-to-close behavior.
In `@spec/requests/api/v1/shared/photos_spec.rb`:
- Around line 137-144: Update the example around the Photos::Search setup to
stop asserting the constructor’s keyword arguments or call wiring. Stub the
external photo-search boundary with the needed range-filtered result, exercise
the endpoint normally, and assert that the response returns the expected photos.
In `@spec/requests/shared/links_spec.rb`:
- Around line 127-130: Remove the internal SharedLinks::TripPhotos mock in
spec/requests/shared/links_spec.rb lines 127-130; stub the Immich HTTP boundary
or use a production-shaped fixture and exercise the real TripPhotos path. In
spec/services/shared_links/trip_photos_spec.rb lines 55-62, test TripPhotos#call
output with an external-boundary stub and move request-payload assertions to the
Photos::Search integration spec.
In `@spec/services/photos/search_spec.rb`:
- Around line 113-147: Remove the Immich::RequestPhotos and
Photoprism::RequestPhotos constructor expectations from the source-selection
examples in the search spec. Keep the distinct serialized fixture assertions and
exact returned arrays in the PhotoPrism-only, Immich-only, and empty-source
cases as the outcome-based verification.
🪄 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: 040f28ac-cf43-446c-a94c-66993f5e7c81
⛔ Files ignored due to path filters (2)
Gemfile.lockis excluded by!**/*.lockdawarich-immich-tags-ai-step1.tar.gzis excluded by!**/*.gz
📒 Files selected for processing (35)
.devcontainer/Dockerfile.gitignoreAGENTS.mdAI/ARCHITECTURE.mdAI/DECISIONS.mdAI/DEVELOPMENT.mdAI/FILES.mdAI/PROJECT.mdAI/README.mdMakefileapp/controllers/api/v1/shared/photos_controller.rbapp/controllers/concerns/share_links/managable.rbapp/models/shared_link.rbapp/services/immich/albums.rbapp/services/immich/request_photos.rbapp/services/immich/tags.rbapp/services/photos/mappable.rbapp/services/photos/search.rbapp/services/shared_links/photo_album.rbapp/services/shared_links/photo_scope.rbapp/services/shared_links/photo_sources.rbapp/services/shared_links/trip_photos.rbapp/services/shared_links/trip_presenter.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.erbapp/views/shared_links/_photo_scope_fields.html.erb.bakspec/requests/api/v1/shared/photos_spec.rbspec/requests/shared/links_spec.rbspec/services/photos/search_spec.rbspec/services/shared_links/photo_album_spec.rbspec/services/shared_links/photo_sources_spec.rbspec/services/shared_links/trip_photos_spec.rb
💤 Files with no reviewable changes (1)
- .gitignore
| permitted = | ||
| if raw.respond_to?(:permit) | ||
| raw.permit( | ||
| *boolean_keys, | ||
| :photo_album_id, | ||
| :photo_album_name | ||
| ) | ||
| else | ||
| raw.slice( | ||
| *( | ||
| boolean_keys.map(&:to_s) + | ||
| %w[photo_album_id photo_album_name] | ||
| ) | ||
| ) | ||
| end |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Wire photo scope through settings and the search. photo_scope is discarded during extraction, and trip searches never resolve or pass PhotoScope#tag_ids. Consequently Photos::Search receives tag_ids: nil and can return all matching Immich assets rather than the public/family-tagged subset.
app/controllers/concerns/share_links/managable.rb#L136-L150: permit and validatephoto_scopeso the selected scope persists on the shared link.app/services/shared_links/trip_photos.rb#L36-L45: resolveSharedLinks::PhotoScope#tag_ids, return an empty result when resolution fails, and pass the resolved IDs toPhotos::Search.cached.
📍 Affects 2 files
app/controllers/concerns/share_links/managable.rb#L136-L150(this comment)app/services/shared_links/trip_photos.rb#L36-L45
🤖 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 136 - 150,
Preserve the selected photo scope during extraction by permitting and validating
photo_scope in the shared-link parameters handled by the relevant concern. In
app/services/shared_links/trip_photos.rb lines 36-45, resolve
SharedLinks::PhotoScope#tag_ids, return an empty result when resolution fails,
and pass the resolved IDs as tag_ids to Photos::Search.cached; both sites
require changes.
|
|
||
| class Photos::Mappable | ||
| MAX_PHOTOS = 100 | ||
| MAX_PHOTOS = 2000 |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Locate target and related files:"
fd -a 'mappable\.rb|shared.*photos.*controller|trip.*photos|_trip\.html\.erb' . | sed 's#^\./##' | head -100
echo
echo "Search constants/usages:"
rg -n "MAX_PHOTOS|mappable_photos|capped_geotagged|max:" . --glob '*.rb' --glob '*.erb' --glob '*.yml' --glob '*.yaml' --glob '*.md' | head -200Repository: Freika/dawarich
Length of output: 5519
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Target file:"
cat -n app/services/photos/mappable.rb
echo
echo "Shared photos controller:"
cat -n app/controllers/api/v1/shared/photos_controller.rb
echo
echo "Shared trip photos service:"
cat -n app/services/shared_links/trip_photos.rb
echo
echo "Shared links trip view:"
cat -n app/views/shared/links/_trip.html.erb
echo
echo "Regular trips trip view (for comparison):"
cat -n app/views/trips/_trip.html.erbRepository: Freika/dawarich
Length of output: 22462
Keep the shared-link photo cap conservative.
MAX_PHOTOS is now 2000, and it caps the public shared-map JSON as Photos::Mappable.new(...).first(@max). The serialized response contains a proxied thumbnail_url per photo, so this increases worst-case payload size and upstream thumbnail fan-out for any visitor. Keep the existing 100-photo cap for shared links/maps unless there is a tested production-scale justification, or add per-day pagination to avoid flood behavior.
🤖 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/mappable.rb` at line 4, Reduce Photos::Mappable’s
MAX_PHOTOS constant from 2000 back to the existing 100-photo cap used for public
shared links/maps, preserving the current first(`@max`) response behavior and
avoiding broader pagination changes.
| <svg viewBox="0 0 48 48" role="img" aria-hidden="true" class="h-full w-full drop-shadow-sm"> | ||
| <path fill="#FA2921" d="M24 3c5.2 0 7.1 5.2 5.1 10.2L24 24l-5.1-10.8C16.9 8.2 18.8 3 24 3Z"/> | ||
| <path fill="#ED79B5" d="M42.2 13.5c2.6 4.5-1 8.7-6.3 9.4L24 24l6.8-9.7c3.1-4.4 8.8-5.3 11.4-.8Z"/> | ||
| <path fill="#FFB400" d="M42.2 34.5c-2.6 4.5-8.3 3.6-11.4-.8L24 24l11.9 1.1c5.3.7 8.9 4.9 6.3 9.4Z"/> | ||
| <path fill="#1E83F7" d="M24 45c-5.2 0-7.1-5.2-5.1-10.2L24 24l5.1 10.8C31.1 39.8 29.2 45 24 45Z"/> | ||
| <path fill="#18C249" d="M5.8 34.5c-2.6-4.5 1-8.7 6.3-9.4L24 24l-6.8 9.7c-3.1 4.4-8.8 5.3-11.4.8Z"/> | ||
| <path fill="#8E59FF" d="M5.8 13.5c2.6-4.5 8.3-3.6 11.4.8L24 24l-11.9-1.1c-5.3-.7-8.9-4.9-6.3-9.4Z"/> | ||
| <circle cx="24" cy="24" r="5.5" fill="white"/> | ||
| </svg> |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Inline SVG violates view guidelines.
The Immich logo is inlined directly in the ERB template. As per coding guidelines, app/views/**/*.erb: "Never inline SVG markup in views. Instead, save SVGs to app/assets/svg/icons and use inline_svg_tag \"name.svg\" to render 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/_trip.html.erb` around lines 130 - 138, Replace the
inline SVG markup in the trip view with an asset-based render using
inline_svg_tag. Save the Immich logo as an SVG under app/assets/svg/icons and
reference that asset by name, preserving the existing sizing, accessibility
attributes, and styling.
Source: Coding guidelines
| (() => { | ||
| if (window.sharedTripPhotoLoaderInstalled) return; | ||
| window.sharedTripPhotoLoaderInstalled = true; | ||
|
|
||
| const overlay = document.querySelector( | ||
| "[data-shared-photo-overlay]" | ||
| ); | ||
|
|
||
| const preview = overlay?.querySelector( | ||
| "[data-shared-photo-preview]" | ||
| ); | ||
|
|
||
| const immichLink = overlay?.querySelector( | ||
| "[data-shared-photo-immich]" | ||
| ); | ||
|
|
||
| const closePhoto = () => { | ||
| if (!overlay || !preview) return; | ||
|
|
||
| overlay.classList.add("hidden"); | ||
| overlay.classList.remove("flex"); | ||
| preview.removeAttribute("src"); | ||
| immichLink?.classList.add("hidden"); | ||
| immichLink?.classList.remove("flex"); | ||
| document.body.style.overflow = ""; | ||
| }; | ||
|
|
||
| const openPhoto = (image) => { | ||
| if (!overlay || !preview || !image?.src) return; | ||
|
|
||
| preview.src = image.src; | ||
| preview.alt = image.alt || "Vergrößertes Foto"; | ||
|
|
||
| if (immichLink) { | ||
| const immichUrl = image.dataset.immichUrl; | ||
| immichLink.href = immichUrl || "<%= j trip.immich_album_url.to_s %>"; | ||
| immichLink.classList.toggle("hidden", !immichLink.href); | ||
| immichLink.classList.toggle("flex", Boolean(immichLink.href)); | ||
| } | ||
|
|
||
| overlay.classList.remove("hidden"); | ||
| overlay.classList.add("flex"); | ||
| document.body.style.overflow = "hidden"; | ||
| }; | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether Turbo Drive is enabled app-wide (default navigation behavior)
rg -n "turbo-rails|turbo_include_tags|data-turbo" --type=erb --type=ruby -g '!spec/**'Repository: Freika/dawarich
Length of output: 153
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== candidate files =="
fd -a '_trip\.erb$|_photo_scope_fields\.erb$|Gemfile$|.*\\.rb$|.*\\.js$|.*\\.erb$' . | sed 's#^\./##' | head -200
echo
echo "== Gemfile entries involving rails/turbo =="
if [ -f Gemfile ]; then
sed -n '1,220p' Gemfile
fi
if [ -f Gemfile.lock ]; then
rg -n "rails|turbo-rails|stimulus-rails|importmap|sprockets" Gemfile.lock | head -120
fi
echo
echo "== view script snippets =="
for f in app/views/shared/links/_trip.html.erb app/views/shared/links/_photo_scope_fields.html.erb; do
if [ -f "$f" ]; then
echo "--- $f ($(wc -l < "$f") lines) ---"
sed -n '1,240p' "$f"
fi
done
echo
echo "== references to shared photo data attributes / partial in layouts/controllers =="
rg -n "shared-photo|shared-photo-overlay|shared-photo-preview|shared-photo-immich|/shared/links/_trip|shared_trip" app config lib --type=erb --type=ruby --type=js --type=ts 2>/dev/null || true
echo
echo "== app views outline (focus shared links) =="
for f in $(fd '_trip\.erb$|_photo_scope_fields\.erb$' app/views || true); do
echo "--- outline: $f ---"
ast-grep outline "$f" || true
doneRepository: Freika/dawarich
Length of output: 15315
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== link/trip/shared partial usages =="
rg -n "render \"shared/links/_trip\"|render \"shared/links/trip\"|render partial:.*shared/links|SharedLinks|shared_links|shared_trip|_photo_scope_fields" app config lib --type=erb --type=ruby --type=js --type=ts 2>/dev/null || true
echo
echo "== relevant controllers/services =="
for f in app/controllers/shared_link_controller.rb app/controllers/shared_links_controller.rb app/controllers/shared/trip_controller.rb app/controllers/shared/trips_controller.rb; do
if [ -f "$f" ]; then
echo "--- $f ---"
sed -n '1,220p' "$f"
fi
done
echo
echo "== view rendering helpers =="
for f in app/helpers/*.rb app/views/shared/links/*.erb app/views/trips/*.erb; do
[ -f "$f" ] || continue
if rg -n "shared/links/_trip|_trip.html.erb|render \"shared/links|render partial:.*trip|shared_trip|shared links|SharedLinks::TripPresenter" "$f" >/dev/null 2>&1; then
echo "--- $f ---"
sed -n '1,220p' "$f"
fi
doneRepository: Freika/dawarich
Length of output: 15120
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== shared links controller/model =="
for f in app/controllers/shared_link_controller.rb app/controllers/shared_links_controller.rb app/models/shared_link.rb app/views/shared/links/show.html.erb app/views/shared/links/_link.html.erb; do
if [ -f "$f" ]; then
echo "--- $f ---"
sed -n '1,220p' "$f"
fi
done
echo
echo "== Turbo page/link/action declarations in non-spec ERB/JS/CSS =="
rg -n "data-turbo-(false|permiss?ive)|turbo_frame|turbo_stream|<%=.*turbo|turbo_include_tags|action \".*\":.*turbo\\b|turbo_drive|VisitMode|view_transition" app config db -g '*.erb' -g '*.rb' -g '*.js' -g '*.ts' -g '*.css' 2>/dev/null || trueRepository: Freika/dawarich
Length of output: 35071
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== shared link routes =="
for f in config/routes.rb config/routes/*.rb; do
if [ -f "$f" ]; then
echo "--- $f ---"
rg -n -C 4 "shared_links?|shared_link|links/show|shared/links" "$f" || true
fi
done
echo
echo "== all link_to and form_with navigation data in shared_link controller paths/views =="
for f in app/controllers/shared_link_controller.rb app/helpers/shared_links_helper.rb $(fd _.*\.erb$ app/views/shared app/views/shared_links 2>/dev/null); do
[ -f "$f" ] || continue
if rg -n "data-turbo|turbo_frame|turbo_stream|link_to|form_with|shared_link_path|links_path|_trip|_track|timelines|lives" "$f" >/dev/null 2>&1; then
echo "--- $f ---"
sed -n '1,220p' "$f"
fi
doneRepository: Freika/dawarich
Length of output: 46831
Re-query the lightbox DOM inside the handlers.
The overlay/preview/immich selector initialization is once per page load behind window.sharedTripPhotoLoaderInstalled, and Turbo Drive defaults to DOM swapping in this Rails version. Move the querySelector calls into openPhoto/closePhoto so Turbo-refreshed shared trip pages can still close or open the lightbox.
🤖 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 148 - 192, Move the
overlay, preview, and immich-link querySelector calls out of the once-per-page
initialization and re-query those elements inside openPhoto and closePhoto. Keep
each handler’s existing behavior and guards intact so it operates on the current
Turbo-rendered DOM.
There was a problem hiding this comment.
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)
spec/requests/shared/links_spec.rb (1)
142-154: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winStub a real Immich photo so this test actually exercises the "no shared link selected" path.
Without a
SharedLinks::TripPhotosstub returning an immich-sourced asset,photos_by_dayis empty and no photo/overlay markup can render regardless ofphoto_album_id/immich_shared_link_id. The assertion that[data-shared-photo-immich]is absent passes trivially, not because the overlay logic correctly omits it when no shared link is selected. This mirrors a previously flagged concern about the "all photos" scenario not exercising real rendering.🧪 Suggested fix
it 'omits the Immich overlay link when no Immich shared link was selected' do link = create(:shared_link, user: owner, resource_type: :trip, resource_id: trip.id, settings: { 'show_days' => true, 'show_photos' => true, 'show_immich' => true, 'photo_album_id' => 'legacy-album' }) + allow(SharedLinks::TripPhotos).to receive(:new).and_return( + instance_double(SharedLinks::TripPhotos, + call: { Date.new(2026, 4, 1) => [{ id: 'asset-9', source: 'immich' }] }) + ) get "/s/#{link.id}" expect(Nokogiri::HTML(response.body).at_css('[data-shared-photo-immich]')).to be_nil end🤖 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 `@spec/requests/shared/links_spec.rb` around lines 142 - 154, Update the test example for the missing Immich shared link to stub SharedLinks::TripPhotos with a real Immich-sourced asset, while leaving the Immich shared-link selection unset. Ensure the request renders photo markup before asserting the [data-shared-photo-immich] element is absent, so the expectation exercises the overlay omission logic rather than an empty photos_by_day result.
🧹 Nitpick comments (2)
app/controllers/concerns/share_links/managable.rb (1)
169-179: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winConsider validating the selected Immich shared link/album against the live list.
photo_album_id,immich_shared_link_id, andimmich_shared_link_slugare trusted as submitted, with no check that they still match an entry fromImmich::SharedLinks.new(current_user).call. A stale or hand-crafted combination would silently persist and produce a broken/mismatched public photo URL later. Cross-checking the selectedimmich_shared_link_idagainst the fetched list (and deriving the slug/album fields from that trusted match rather than the client-submitted values) would close this gap.🤖 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 169 - 179, Validate the submitted Immich selection in the settings update flow against the live entries returned by Immich::SharedLinks.new(current_user).call. Match immich_shared_link_id to a trusted entry, persist its derived slug and album values instead of client-submitted photo_album_id, photo_album_name, and immich_shared_link_slug, and avoid persisting the selection when no valid match exists.spec/services/immich/shared_links_spec.rb (1)
44-61: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd cache-hit regression coverage once the caching fix lands.
This spec doesn't exercise
Rails.cacheread/write behavior, so it wouldn't catch the empty-result caching bug flagged inapp/services/immich/shared_links.rb(cached_links.present?vsunless cached_links.nil?). Once that's fixed, add a case asserting a secondcallwith an empty usable-links result does not re-invokeHTTParty.get.🤖 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 `@spec/services/immich/shared_links_spec.rb` around lines 44 - 61, Add regression coverage in the shared-links service specs for an empty usable-links result: stub the initial HTTParty.get response, invoke service.call twice, and assert HTTParty.get is called only once, verifying the second call uses the Rails.cache entry. Keep the existing configured API-key and filtering examples unchanged.
🤖 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/immich/shared_links.rb`:
- Around line 12-22: Update the cache-hit check in the call method to return
cached_links whenever the cache read is not nil, including legitimately cached
empty arrays. Match the nil-based pattern used by Immich::Albums and
Immich::Tags, while leaving the fetch_shared_links and cache-writing behavior
unchanged.
---
Outside diff comments:
In `@spec/requests/shared/links_spec.rb`:
- Around line 142-154: Update the test example for the missing Immich shared
link to stub SharedLinks::TripPhotos with a real Immich-sourced asset, while
leaving the Immich shared-link selection unset. Ensure the request renders photo
markup before asserting the [data-shared-photo-immich] element is absent, so the
expectation exercises the overlay omission logic rather than an empty
photos_by_day result.
---
Nitpick comments:
In `@app/controllers/concerns/share_links/managable.rb`:
- Around line 169-179: Validate the submitted Immich selection in the settings
update flow against the live entries returned by
Immich::SharedLinks.new(current_user).call. Match immich_shared_link_id to a
trusted entry, persist its derived slug and album values instead of
client-submitted photo_album_id, photo_album_name, and immich_shared_link_slug,
and avoid persisting the selection when no valid match exists.
In `@spec/services/immich/shared_links_spec.rb`:
- Around line 44-61: Add regression coverage in the shared-links service specs
for an empty usable-links result: stub the initial HTTParty.get response, invoke
service.call twice, and assert HTTParty.get is called only once, verifying the
second call uses the Rails.cache entry. Keep the existing configured API-key and
filtering examples 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6306259e-c87f-4ad7-a7fe-83cde77f6e4d
📒 Files selected for processing (19)
AI/DECISIONS.mdAI/DEVELOPMENT.mdMakefileapp/controllers/concerns/share_links/managable.rbapp/services/immich/albums.rbapp/services/immich/shared_links.rbapp/services/immich/tags.rbapp/services/shared_links/photo_album.rbapp/services/shared_links/trip_presenter.rbapp/views/settings/integrations/index.html.erbapp/views/shared/links/_trip.html.erbapp/views/shared_links/_immich_shared_link_fields.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.erbspec/requests/share_links/timelines_spec.rbspec/requests/shared/links_spec.rbspec/services/immich/shared_links_spec.rbspec/services/shared_links/photo_album_spec.rb
🚧 Files skipped from review as they are similar to previous changes (2)
- AI/DECISIONS.md
- AI/DEVELOPMENT.md
| def call | ||
| cached_links = Rails.cache.read(cache_key) | ||
| return cached_links if cached_links.present? | ||
|
|
||
| result = fetch_shared_links | ||
| return [] unless result[:success] | ||
|
|
||
| links = Array(result[:data]).select { |link| usable?(link) } | ||
| Rails.cache.write(cache_key, links, expires_in: CACHE_TTL) | ||
| links | ||
| end |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
Apply the same cache-hit fix used in Immich::Albums/Immich::Tags.
cached_links.present? treats a legitimately cached empty array (users with no album-based shared links) as a cache miss, causing a live Immich API call on every page load for those users. The sibling services in this PR (Immich::Albums, Immich::Tags) were already fixed to check unless cached_albums.nil? / unless cached_tags.nil? — this new service should follow the same pattern.
⚡ Proposed fix
def call
cached_links = Rails.cache.read(cache_key)
- return cached_links if cached_links.present?
+ return cached_links unless cached_links.nil?📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def call | |
| cached_links = Rails.cache.read(cache_key) | |
| return cached_links if cached_links.present? | |
| result = fetch_shared_links | |
| return [] unless result[:success] | |
| links = Array(result[:data]).select { |link| usable?(link) } | |
| Rails.cache.write(cache_key, links, expires_in: CACHE_TTL) | |
| links | |
| end | |
| def call | |
| cached_links = Rails.cache.read(cache_key) | |
| return cached_links unless cached_links.nil? | |
| result = fetch_shared_links | |
| return [] unless result[:success] | |
| links = Array(result[:data]).select { |link| usable?(link) } | |
| Rails.cache.write(cache_key, links, expires_in: CACHE_TTL) | |
| links | |
| end |
🤖 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/immich/shared_links.rb` around lines 12 - 22, Update the
cache-hit check in the call method to return cached_links whenever the cache
read is not nil, including legitimately cached empty arrays. Match the nil-based
pattern used by Immich::Albums and Immich::Tags, while leaving the
fetch_shared_links and cache-writing behavior unchanged.
There was a problem hiding this comment.
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 (2)
spec/requests/share_links/timelines_spec.rb (1)
71-94: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert that this POST creates the record under test.
Line 86 reads the last timeline share without asserting that the request created one. Wrap the POST in
change { user.shared_links.where(resource_type: :timeline).count }.by(1)before checking its settings.As per coding guidelines, request specs should verify observable state changes rather than only wiring or incidental record lookup.
🤖 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 `@spec/requests/share_links/timelines_spec.rb` around lines 71 - 94, Update the “stores the selected Immich shared link and its album filter” example to wrap the POST request in an expectation that the timeline shared-link count changes by one, then retain the existing settings assertions against the created record.Source: Coding guidelines
Makefile (1)
164-170: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winPromote the tested image to production
testdawarichanddawarichboth build$(IMAGE)separately, butdawarichonly depends onreview, nottestdawarich. Since both paths target the same image name,dawarichcan overwrite/rebuild the image after the test container was started or beforeproductiondeploys with a different build. Makedawarichdepend ontestdawarichand skip the production rebuild/deploy from that already-built image.🤖 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 `@Makefile` around lines 164 - 170, Update the dawarich target to depend on testdawarich, reuse the image built and validated there, and remove its redundant production image rebuild/deploy steps. Preserve the existing production deployment behavior using the already-tested $(IMAGE), while keeping testdawarich responsible for building and updating the test instance.Source: Learnings
🤖 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 `@AI/DEVELOPMENT.md`:
- Around line 124-126: Überarbeite den Abschnitt zum Commit-Workflow in
AI/DEVELOPMENT.md, sodass die empfohlene Reihenfolge mit make review vor make
commit erhalten bleibt, zugleich aber klar beschrieben wird, dass make commit
den Review-Status nicht erzwingt und weiterhin policy-bewusste Arbeitsstände
ohne erfolgreichen Review erstellen kann. Beziehe dabei die bestehenden
Prüfungen auf unerwünschte Dateien und leere Commits ein.
---
Outside diff comments:
In `@Makefile`:
- Around line 164-170: Update the dawarich target to depend on testdawarich,
reuse the image built and validated there, and remove its redundant production
image rebuild/deploy steps. Preserve the existing production deployment behavior
using the already-tested $(IMAGE), while keeping testdawarich responsible for
building and updating the test instance.
In `@spec/requests/share_links/timelines_spec.rb`:
- Around line 71-94: Update the “stores the selected Immich shared link and its
album filter” example to wrap the POST request in an expectation that the
timeline shared-link count changes by one, then retain the existing settings
assertions against the created record.
🪄 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: 31d1f225-0af9-46e3-b132-486ad0d4b699
📒 Files selected for processing (5)
AI/DEVELOPMENT.mdMakefileapp/views/shared_links/_immich_shared_link_fields.html.erbapp/views/shared_links/_modal_trip_create_form.html.erbspec/requests/share_links/timelines_spec.rb
💤 Files with no reviewable changes (1)
- app/views/shared_links/_immich_shared_link_fields.html.erb
| - `make commit MESSAGE="..."` erstellt unabhängig vom Review-Status einen | ||
| lokalen Zwischenstand und behält dabei seine Prüfungen auf unerwünschte | ||
| Dateien und leere Commits bei. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and inspect the relevant documentation and Makefile references.
if [ -f AI/DEVELOPMENT.md ]; then
echo "== AI/DEVELOPMENT.md relevant lines =="
nl -ba AI/DEVELOPMENT.md | sed -n '110,160p'
fi
echo "== Makefile names =="
fd -a '^Makefile$|makefile$' . | sed 's#^\./##'
echo "== Relevant make targets and wording =="
for f in $(fd '^Makefile$|makefile$' .); do
echo "--- $f ---"
rg -n '^(commit|review|pre-commit|post-commit|deny|whitespace|blank|empty|status|PR|review):|make review|make commit|Review|review' "$f" -A 8 -B 4 || true
done
echo "== Codebase references to make commit/review =="
rg -n 'make (commit|review)|commit MESSAGE|Review|review-before|review-before-commit|immediate pull|Zwischenstand|Review-Status' . --glob '!node_modules' --glob '!dist' --glob '!build' || trueRepository: Freika/dawarich
Length of output: 232
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and inspect the relevant documentation and Makefile references.
if [ -f AI/DEVELOPMENT.md ]; then
echo "== AI/DEVELOPMENT.md relevant lines =="
nl -ba AI/DEVELOPMENT.md | sed -n '110,160p'
fi
echo "== Makefile names =="
fd -a '^Makefile$|makefile$' . | sed 's#^\./##'
echo "== Relevant make targets and wording =="
for f in $(fd '^Makefile$|makefile$' .); do
echo "--- $f ---"
rg -n '^(commit|review|pre-commit|post-commit|deny|whitespace|blank|empty|status|PR|review):|make review|make commit|Review|review' "$f" -A 8 -B 4 || true
done
echo "== Codebase references to ma ke commit/review =="
rg -n 'make (commit|review)|commit MESSAGE|Review|review-before|review-before-commit|immediate pull|Zwi sch enstand|Review-Status' . --glob '!node_modules' --glob '!dist' --glob '!build' || trueRepository: Freika/dawarich
Length of output: 232
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Python version =="
python3 --version || true
python3 - <<'PY'
from pathlib import Path
import re
doc = Path("AI/DEVELOPMENT.md")
if not doc.exists():
print("AI/DEVELOPMENT.md not found")
raise SystemExit(0)
lines = doc.read_text().splitlines()
for n in [124, 125, 126, 136, 137, 138, 139, 140, 141, 142, 143, 144, 145, 146, 147, 148, 149]:
print(f"{n:4d}: {lines[n-1]}")
print("\n== Makefile candidates ==")
for p in Path(".").rglob("Makefile"):
if ".git" in str(p):
continue
print(p)
print("\n== Makefile relevant snippets ==")
for p in Path(".").rglob("Makefile"):
if ".git" in str(p):
continue
txt = p.read_text(errors="replace").splitlines()
found = False
for i, line in enumerate(txt, 1):
if re.search(r'^(commit|review|pre-commit|post-commit|deny|whitespace|blank|empty|status|pr|pull):|make review|make commit|Review|review|empty commit', line, re.I):
found = True
lo=max(1,i-5); hi=min(len(txt), i+12)
print(f"--- {p}:{i} ---")
for j in range(lo, hi+1):
print(f"{j:4d}: {txt[j-1]}")
if not found:
print(f"--- {p} no relevant matches ---")
print("\n== Repository references ==")
matches = []
for p in Path(".").rglob("*"):
if p.is_file() and ".git" not in p.parts and p.suffix not in {".png", ".jpg", ".svg", ".zip"}:
try:
s = p.read_text(errors="ignore")
except Exception:
continue
for q in ["make commit", "make review", "commit MESSAGE", "Review-Status", "review-before-commit", "Review-Status", "Review-Zwischenstand", "Reparaturschritte"]:
if q in s:
matches.append((str(p), q))
for m in matches:
print(m)
PYRepository: Freika/dawarich
Length of output: 11739
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
makefile = Path("Makefile").read_text().splitlines()
commit_section = []
in_commit = False
for i, line in enumerate(makefile):
if re.match(r'^commit:', line):
in_commit = True
if in_commit:
commit_section.append((i+1, line))
if i+1 < len(makefile) and makefile[i+1].startswith("dawarich:") and not line.startswith("\t") and re.match(r'^[a-zA-Z0-9_-]+:', line):
break
print("== Makefile commit target ==")
for n, line in commit_section:
print(f"{n:4d}: {line}")
print("\n== Does commit target contain review dependency/enforcement? ==")
texts = "\n".join(line.strip() for _, line in commit_section)
print("has prereq-like dependency:", bool(re.search(r'^commit:\s*review\b|^\s+@\$\(MAKE\).*review\b', texts, re.I | re.M)))
print("has success marker/output:", bool(re.search(r'REVIEW ERFOLGREICH|REVIEW\s+.*|erfolgreich.*REVIEW', texts, re.I | re.M)))
print("has no-message guard:", "MESSAGE fehlt" in texts)
print("has file hygiene checks:", all(s in texts for s in [".env|.env.*", "log/*", "tmp/*", "storage/*"]))
print("has empty commits checks:", all(s in texts for s in ["empty commits", "leere commits", "git status --short"]))
PYRepository: Freika/dawarich
Length of output: 3216
Make the commit workflow wording consistent.
AI/DEVELOPMENT.md describes the official sequence as make review before make commit, while Makefile commit does not enforce that dependency. Clarify that make commit is policy-aware but can still create work-in-progress checkpoints without a successful review.
Suggested wording
-- `make commit MESSAGE="..."` erstellt unabhängig vom Review-Status einen
- lokalen Zwischenstand und behält dabei seine Prüfungen auf unerwünschte
- Dateien und leere Commits bei.
+- `make commit MESSAGE="..."` erzwingt den Review-Status nicht. Im verbindlichen
+ Workflow darf das Target jedoch erst nach einem erfolgreichen `make review`
+ verwendet werden; die Prüfungen auf unerwünschte Dateien und leere Commits
+ bleiben bestehen.[mention_maintainability_and_code_quality]
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - `make commit MESSAGE="..."` erstellt unabhängig vom Review-Status einen | |
| lokalen Zwischenstand und behält dabei seine Prüfungen auf unerwünschte | |
| Dateien und leere Commits bei. | |
| - `make commit MESSAGE="..."` erzwingt den Review-Status nicht. Im verbindlichen | |
| Workflow darf das Target jedoch erst nach einem erfolgreichen `make review` | |
| verwendet werden; die Prüfungen auf unerwünschte Dateien und leere Commits | |
| bleiben bestehen. |
🤖 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 `@AI/DEVELOPMENT.md` around lines 124 - 126, Überarbeite den Abschnitt zum
Commit-Workflow in AI/DEVELOPMENT.md, sodass die empfohlene Reihenfolge mit make
review vor make commit erhalten bleibt, zugleich aber klar beschrieben wird,
dass make commit den Review-Status nicht erzwingt und weiterhin policy-bewusste
Arbeitsstände ohne erfolgreichen Review erstellen kann. Beziehe dabei die
bestehenden Prüfungen auf unerwünschte Dateien und leere Commits ein.
1 participant
Summary
Adds optional Immich album selection for shared trips.
Features
Tested
Summary by CodeRabbit
New Features
Bug Fixes
Documentation