0.30.11 #1676
0.30.11 #1676
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughMake Import.source nullable and remove its default; controller now ensures imported filenames are unique by appending timestamps on collisions; source detection is conditional and persisted by the create service with expanded importer mappings; several services and serializers handle nil sources; UI classes, tests, fixtures, schema, and changelog updated. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Controller as ImportsController
participant Storage as ActiveStorage
participant DB as DB (imports)
participant Service as Imports::Create
participant Detector as SourceDetector
participant Importer as Importer (various)
User->>Controller: upload signed_id
Controller->>Storage: find blob by signed_id
Storage-->>Controller: blob (filename)
Controller->>Controller: generate_unique_import_name(filename)
Controller->>DB: create Import (name, source may be nil)
Controller-->>User: respond (accepted)
Service->>DB: load Import, download blob -> temp_file
Service->>Service: should_detect_source?
alt detection needed (source nil or allowed)
Service->>Detector: detect_source_from_file(temp_file)
Detector-->>Service: detected_source
Service->>DB: update import.source = detected_source
else API sources (immich_api/photoprism_api)
Service->>Service: keep existing source
end
Service->>Service: importer = lookup_by(source) (raise if nil/unsupported)
Service->>Importer: import(temp_file)
Importer-->>Service: result
Service-->>User: complete/queued
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (2)
✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![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 (19)
app/controllers/settings/background_jobs_controller.rb (3)
14-15: flash.now is discarded on redirect; avoid duplicate notices.You both set
flash.now[:notice]and passnotice:toredirect_to. Theflash.nowmessage won’t survive the redirect, and the user will only see the redirect notice. Drop theflash.nowassignment to avoid confusion.Apply this minimal diff:
- flash.now[:notice] = 'Job was successfully created.' ... - redirect_to redirect_path, notice: 'Job was successfully created.' + redirect_to redirect_path, notice: 'Job was successfully created.'Optional: centralize the message via I18n (
t('background_jobs.create.success')) to keep copy consistent.Also applies to: 24-24
5-7: Deduplicate the “import jobs” special-casing via a constant; reduces drift across auth and redirect.You special-case the same two job names in both the
before_actionand the redirect logic. Extracting them to a single constant keeps behavior aligned if/when the set changes.Proposed refactor:
class Settings::BackgroundJobsController < ApplicationController + IMPORT_JOBS = %w[start_immich_import start_photoprism_import].freeze before_action :authenticate_self_hosted! - before_action :authenticate_admin!, unless: lambda { - %w[start_immich_import start_photoprism_import].include?(params[:job_name]) - } + before_action :authenticate_admin!, unless: -> { IMPORT_JOBS.include?(params[:job_name]) } ... - redirect_path = - case params[:job_name] - when 'start_immich_import', 'start_photoprism_import' - imports_path - else - settings_background_jobs_path - end + redirect_path = IMPORT_JOBS.include?(params[:job_name]) ? imports_path : settings_background_jobs_pathFollow-up thought: if more job-specific redirects appear, consider a small map (JOB_REDIRECTS) for O(1) lookups rather than branching.
Also applies to: 16-22
11-13: Validate job_name against a known registry to prevent enqueuing unknown/unsupported jobs.Right now any
params[:job_name]is accepted. IfEnqueueBackgroundJobdispatches dynamically, this could enqueue unexpected work or fail noisily. Guard with an allowlist/registry (possibly the same sourceEnqueueBackgroundJobuses) and return 422/400 when invalid.If you want, I can scan the repo to locate where valid job names are defined and wire a shared registry. For now, here’s a minimal controller-side guard you can adapt:
def create - EnqueueBackgroundJob.perform_later(params[:job_name], current_user.id) + job_name = params[:job_name].to_s + unless job_name.present? + return redirect_back fallback_location: settings_background_jobs_path, alert: 'Missing job name.' + end + # TODO: replace ALLOWED_JOB_NAMES with a shared registry + if defined?(ALLOWED_JOB_NAMES) && !ALLOWED_JOB_NAMES.include?(job_name) + return redirect_back fallback_location: settings_background_jobs_path, alert: 'Unsupported job.' + end + EnqueueBackgroundJob.perform_later(job_name, current_user.id)Would you like me to open a follow-up to introduce a shared job registry and specs?
CHANGELOG.md (2)
11-11: Tighten wording and clarify behavior for duplicate import namesCurrent text is grammatically off and a bit vague. Consider rephrasing and giving an example of the timestamp format for predictability.
Apply:
-- If user already have import with the same name, it will be appended with timestamp during the import process. +- If a user already has an import with the same name, Dawarich will append a timestamp to make it unique during the import process (e.g., `my_import_2025-08-23T14-55-53Z`).
9-9: Call out the schema/config change: Import.source can be nil and default removedThis PR removes the default for
imports.sourceand allowsImport.sourceto benil. That’s user-facing (and potentially breaking) behavior and should be documented under “Changed” to help self-hosters/operators.Apply:
## Changed +- Import source no longer defaults to a value; `Import.source` may now be `nil` (migration included). Source auto-detection moved from controller to service and runs only when appropriate.spec/services/google_maps/phone_takeout_importer_spec.rb (2)
39-49: Avoid relying on implicit record order in expectations
Point.all[6]andPoint.lastrely on implicit ordering which isn’t guaranteed. Sort explicitly for deterministic tests.Apply:
- expect(Point.all[6].lat).to eq(27.696576) - expect(Point.all[6].lon).to eq(-97.376949) - expect(Point.all[6].timestamp).to eq(1_693_180_140) + ordered = Point.order(:timestamp) + expect(ordered[6].lat).to eq(27.696576) + expect(ordered[6].lon).to eq(-97.376949) + expect(ordered[6].timestamp).to eq(1_693_180_140) - expect(Point.last.lat).to eq(27.709617) - expect(Point.last.lon).to eq(-97.375988) - expect(Point.last.timestamp).to eq(1_693_180_320) + expect(ordered.last.lat).to eq(27.709617) + expect(ordered.last.lon).to eq(-97.375988) + expect(ordered.last.timestamp).to eq(1_693_180_320)
17-20: Ensure duplicates are dropped and remove redundant file attachmentTo prevent regressions in your phone-takeout importer spec:
- Add an explicit assertion that the importer dedupes points by the intended key (e.g. timestamp/lat/lon), not just that the total count changes.
- Remove the redundant
import.file.attachin thebeforeblock and rely on thefile:passed into the factory, preventing extra ActiveStorage blobs.Locations to update in
spec/services/google_maps/phone_takeout_importer_spec.rb:
- Around line 19: after creating/importing, assert uniqueness.
- Around lines 10–13: drop the
before do … import.file.attach(...) endblock.Suggested example diff:
@@ spec/services/google_maps/phone_takeout_importer_spec.rb - before do - import.file.attach( - io: File.open(file_path), - filename: 'phone_takeout.json', - content_type: 'application/json' - ) - end let(:file_path) { Rails.root.join('spec/fixtures/files/google/phone-takeout_w_3_duplicates.json') } let(:file) { Rack::Test::UploadedFile.new(file_path, 'application/json') } let(:import) { create(:import, user: user, name: 'phone_takeout.json', file: file) } it 'drops duplicate points' do - expect { parser }.to change { Point.count }.by(4) - parser + expect { parser }.to change { Point.count }.by(4) + parser + # ensure all remaining points are unique by the dedupe key + unique_count = Point.select(:lat, :lon, :timestamp).distinct.count + expect(unique_count).to eq(Point.count) endapp/services/imports/source_detector.rb (4)
63-70: Guard against nil header reads and minor style polish
File#readcan returnnilfor empty files; we should avoid calling methods onnilfurther down. Also, return an empty string when header is missing to keep downstream checks safe.Apply:
- def self.new_from_file_header(file_path) + def self.new_from_file_header(file_path) filename = File.basename(file_path) - - # For detection, read only first 2KB to optimize performance - header_content = File.open(file_path, 'rb') { |f| f.read(2048) } - - new(header_content, filename, file_path) + # For detection, read only first 2KB to optimize performance + header_content = File.open(file_path, 'rb') { |f| f.read(2048) }.to_s + new(header_content, filename, file_path) end
101-116: Fix potential nil dereference and address RuboCop alignment in GPX detectionIf
content_to_checkisnil,strip.start_with?will raise. Also alignsif/else/endwith RuboCop and improves readability.Apply:
def gpx_file? return false unless filename # Must have .gpx extension AND contain GPX XML structure return false unless filename.downcase.end_with?('.gpx') - - # Check content for GPX structure - content_to_check = if file_path && File.exist?(file_path) - # Read first 1KB for GPX detection - File.open(file_path, 'rb') { |f| f.read(1024) } - else - file_content - end - - content_to_check.strip.start_with?('<?xml') && content_to_check.include?('<gpx') + # Check content for GPX structure + content_to_check = + if file_path && File.exist?(file_path) + # Read first 1KB for GPX detection + File.open(file_path, 'rb') { |f| f.read(1024) } + else + file_content + end + + content = content_to_check.to_s + content.start_with?('<?xml') && content.include?('<gpx') end
118-133: OwnTracks detection: use binary-safe reads, guard nil, and align branchesSame rationale as GPX: avoid nil dereference, align
if/else, and prefer binary read to avoid encoding issues on some platforms.Apply:
def owntracks_file? return false unless filename # Check for .rec extension first (fastest check) return true if filename.downcase.end_with?('.rec') - - # Check for specific OwnTracks line format in content - content_to_check = if file_path && File.exist?(file_path) - # For OwnTracks, read first few lines only - File.open(file_path, 'r') { |f| f.read(2048) } - else - file_content - end - - content_to_check.lines.any? { |line| line.include?('"_type":"location"') } + # Check for specific OwnTracks line format in content + content_to_check = + if file_path && File.exist?(file_path) + # For OwnTracks, read first few lines only + File.open(file_path, 'rb') { |f| f.read(2048) } + else + file_content + end + + content_to_check.to_s.lines.any? { |line| line.include?('"_type":"location"') } end
72-88: Optional: avoid full-file JSON load during detection for very large inputs
Oj.load_filewill load the entire file. For large exports, consider a streaming heuristic first (e.g., read and inspect the first N KB for top-level keys likesemanticSegments,timelineObjects,locations) before falling back to full parse. This keeps source detection cheap for giant files.If you want, I can draft a streaming-first detector using
Oj.saj/sc_parseto look for known keys without loading the whole JSON.db/migrate/20250823125940_remove_default_from_imports_source.rb (1)
1-5: Add frozen string literal pragma and consider data backfillStyle: add
# frozen_string_literal: truefor consistency.Semantics: this drops the default going forward but leaves existing
source = 0rows untouched. If0previously mapped to a concrete source (e.g.,google_semantic_history) rather than “unknown”, you might want a follow-up data migration or rake task to nullify incorrect legacy defaults created under the old behavior.Apply:
+ # frozen_string_literal: true class RemoveDefaultFromImportsSource < ActiveRecord::Migration[8.0] def change change_column_default :imports, :source, from: 0, to: nil end endIf you choose to backfill, a reversible migration or a one-off rake task like:
# Example (adjust the condition to your enum mapping): # Rails.logger.warn("Nullifying legacy defaulted sources (source=0) created after 2025-08-01") # Import.where(source: 0).update_all(source: nil)Let me know if you want me to draft a safe, filterable data migration that avoids clobbering known-correct sources.
spec/factories/imports.rb (1)
7-7: Add source-specific traits; consider making the generated name source-agnostic
- To keep factories explicit and avoid coupling to a specific provider in the filename, add traits for common sources and consider a neutral default name.
Example traits (outside selected lines):
factory :import do user sequence(:name) { |n| "import_#{n}.json" } trait(:google_semantic_history) { source { 'google_semantic_history' } } trait(:owntracks) { source { 'owntracks' } } trait(:google_records) { source { 'google_records' } } trait(:google_phone_takeout) { source { 'google_phone_takeout' } } trait(:gpx) { source { 'gpx' } } trait(:geojson) { source { 'geojson' } } trait(:immich_api) { source { 'immich_api' } } trait(:photoprism_api) { source { 'photoprism_api' } } trait(:user_data_archive) { source { 'user_data_archive' } } endspec/services/users/import_data/imports_spec.rb (1)
224-225: Expectation update to nil is correct; also update the adjacent outdated inline comment
- Switching the expected source for “Missing Source Import” to
nilmatches the removal of the default andallow_nil: true.- Minor nit: the comment above still says “source defaults to first enum,” which is no longer true.
Apply:
- # Verify only the valid imports were created (name is required, source defaults to first enum) + # Verify only the valid imports were created (name is required; source can be nil)Optional: to reduce DB-specific brittleness, assert booleans as truthy/falsey instead of
1/0(not required for this PR).spec/services/imports/create_spec.rb (1)
24-28: Rename to reflect behavior; consider adding a nil→detected persistence case
- The example “updates the import source” reads like it should change the value. Here it verifies that it remains
owntracks(which is fine; the service persists a resolved/explicit source).Rename and add a complementary case:
- it 'updates the import source' do + it 'persists the resolved import source (does not overwrite explicit value)' do service.call expect(import.reload.source).to eq('owntracks') endAdditional spec (outside selected lines) to exercise the new detection + persistence path when source starts nil:
it 'detects and persists source when import.source is nil' do import = create(:import, source: nil, name: '2024-03.rec') file_path = Rails.root.join('spec/fixtures/files/owntracks/2024-03.rec') import.file.attach(io: File.open(file_path), filename: '2024-03.rec', content_type: 'application/octet-stream') described_class.new(user, import).call expect(import.reload.source).to eq('owntracks') endapp/views/imports/index.html.erb (2)
39-39: Zebra table class: minor cleanup and consistencyLooks good to switch to
table table-zebra. Minor nit: you’ve gotoverflow-x-autoboth on the wrapper div (Line 38) and on the table. The wrapper alone is sufficient and avoids redundant styling.Apply this diff to simplify:
- <table class="table table-zebra overflow-x-auto"> + <table class="table table-zebra">
60-64: Hide empty source parentheses when source is nilNow that
Import.sourcecan be nil, the UI renders empty parentheses like(). Consider conditionally rendering the source (and its surrounding parentheses) only when present.Suggested change:
- (<%= import.source %>) + <%= "(#{import.source})" if import.source.present? %>app/services/imports/create.rb (1)
19-26: Clarify detection policy: this overrides non-API sourcesThis branch forces re-detection for any non-API source (e.g.,
gpx,geojson,owntracks) even if a value is already set onimport.source. If that’s intentional, all good; if not, tighten the condition to only detect whenimport.sourceis nil or explicitly unknown.If the intent is “detect only when nil or unknown,” consider:
- source = if import.source.nil? || should_detect_source? + source = if import.source.nil? detect_source_from_file(temp_file_path) else import.source endOr rename
should_detect_source?tooverride_non_api_source?to better reflect current semantics.app/controllers/imports_controller.rb (1)
120-120: Style: trailing whitespaceRuboCop flagged trailing whitespace here. The refactor above removes it; otherwise, delete the extraneous spaces.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (15)
CHANGELOG.md(1 hunks)app/controllers/imports_controller.rb(1 hunks)app/controllers/settings/background_jobs_controller.rb(1 hunks)app/models/import.rb(1 hunks)app/services/imports/create.rb(2 hunks)app/services/imports/source_detector.rb(3 hunks)app/views/imports/index.html.erb(3 hunks)db/migrate/20250823125940_remove_default_from_imports_source.rb(1 hunks)db/schema.rb(2 hunks)spec/factories/imports.rb(1 hunks)spec/fixtures/files/google/phone-takeout_w_3_duplicates.json(0 hunks)spec/services/google_maps/phone_takeout_importer_spec.rb(1 hunks)spec/services/imports/create_spec.rb(3 hunks)spec/services/imports/source_detector_spec.rb(2 hunks)spec/services/users/import_data/imports_spec.rb(1 hunks)
💤 Files with no reviewable changes (1)
- spec/fixtures/files/google/phone-takeout_w_3_duplicates.json
🧰 Additional context used
🪛 RuboCop (1.76.1)
db/schema.rb
[convention] 13-13: Use underscores(_) as thousands separator and separate every 3 digits with them.
(Style/NumericLiterals)
[convention] 102-102: Prefer single-quoted strings when you don't need string interpolation or special symbols.
(Style/StringLiterals)
app/services/imports/source_detector.rb
[convention] 110-110: Use 2 (not 1) spaces for indentation.
(Layout/IndentationWidth)
[convention] 111-111: Align else with if.
(Layout/ElseAlignment)
[warning] 113-113: end at 113, 22 is not aligned with if at 108, 23.
(Layout/EndAlignment)
[convention] 127-127: Use 2 (not 1) spaces for indentation.
(Layout/IndentationWidth)
[convention] 128-128: Align else with if.
(Layout/ElseAlignment)
[warning] 130-130: end at 130, 22 is not aligned with if at 125, 23.
(Layout/EndAlignment)
db/migrate/20250823125940_remove_default_from_imports_source.rb
[convention] 1-1: Missing frozen string literal comment.
(Style/FrozenStringLiteralComment)
app/controllers/imports_controller.rb
[convention] 120-120: Trailing whitespace detected.
(Layout/TrailingWhitespace)
🔇 Additional comments (14)
app/controllers/settings/background_jobs_controller.rb (1)
9-9: Whitespace-only tweak in index is fine.No functional change; reads clean.
db/schema.rb (1)
13-13: Schema version bump looks consistent with the new migrationNo action needed.
spec/factories/imports.rb (1)
7-7: Factory default removal for source is correct and aligned with enum allow_nilCommenting out the default
sourcematches the migration and model change to allow nils. This will surface tests that were accidentally depending on the implicit default.spec/services/imports/create_spec.rb (2)
72-72: Fixture path and filename update are consistentSwitching to
phone-takeout_w_3_duplicates.jsonand matching the attached filename keeps the spec coherent with the new fixture.Minor consistency suggestion (outside selected lines): a few contexts define
file = Rack::Test::UploadedFile.new(...)but still attach viaFile.open. Prefer attaching thefileto avoid open file handle leaks and keep style uniform:# before import.file.attach(io: File.open(file_path), filename: '2024-03.rec', content_type: 'application/octet-stream') # after (uses the defined `file`) import.file.attach(file)Also applies to: 75-76
203-203: Whitespace-only changeNo action needed.
spec/services/imports/source_detector_spec.rb (2)
27-27: Updated fixture improves coverage without altering intentUsing
phone-takeout_w_3_duplicates.jsonkeeps detection focused while aligning with importer scenarios that handle duplicates. Looks good.
134-134: Whitespace-only changeNo action needed.
app/views/imports/index.html.erb (3)
58-59: Row hover styling: LGTMAdding
class="hover"on the<tr>pairs well withtable-zebrain DaisyUI and improves affordance without affecting behavior.
76-76: Download button: good use of Content-Disposition and filenameUsing
rails_blob_path(..., disposition: 'attachment', download: import.name)is solid and will prompt a user-friendly filename. No action needed.
78-78: Delete button: LGTMConfirmation +
turbo_method: :deletelooks correct and consistent with the rest of the app.app/services/imports/create.rb (3)
25-25: Persisting source before import: LGTMSaving the resolved source prior to invoking the importer improves observability and aligns persisted state with runtime behavior.
104-108: Source detector call: LGTMDelegation remains clean; no issues spotted.
99-103: Ignore the symbol vs. string concern – Rails enums return stringsRails’
enum :sourceaccessor always returns a string (e.g."immich_api"), so%w[immich_api photoprism_api].include?(import.source)already works as intended. No change is needed here.Likely an incorrect or invalid review comment.
app/controllers/imports_controller.rb (1)
105-107: Using a helper for unique names: good separationDeferring name generation to a dedicated helper keeps
create_import_from_signed_idtidy. Nice.
| def generate_unique_import_name(original_name) | ||
| return original_name unless current_user.imports.exists?(name: original_name) | ||
|
|
||
| Imports::SourceDetector.new_from_file_header(temp_file_path).detect_source | ||
| rescue StandardError => e | ||
| Rails.logger.warn "Failed to auto-detect import source for #{file_attachment.filename}: #{e.message}" | ||
| nil | ||
| ensure | ||
| # Cleanup temp file | ||
| if temp_file_path && File.exist?(temp_file_path) | ||
| File.unlink(temp_file_path) | ||
| end | ||
| # Extract filename and extension | ||
| basename = File.basename(original_name, File.extname(original_name)) | ||
| extension = File.extname(original_name) | ||
|
|
||
| # Add current datetime | ||
| timestamp = Time.current.strftime('%Y%m%d_%H%M%S') | ||
| "#{basename}_#{timestamp}#{extension}" | ||
| end |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Strengthen uniqueness: second-level timestamp can collide; also trim trailing whitespace
If a user uploads multiple files with the same original name within the same second, both will receive the same timestamped name, causing collisions. Use microseconds and re-check in a loop (or add a short counter suffix) to guarantee uniqueness. Also, RuboCop flagged trailing whitespace (Line 120/121).
Apply this diff to harden uniqueness and remove trailing spaces:
- def generate_unique_import_name(original_name)
- return original_name unless current_user.imports.exists?(name: original_name)
-
- # Extract filename and extension
- basename = File.basename(original_name, File.extname(original_name))
- extension = File.extname(original_name)
-
- # Add current datetime
- timestamp = Time.current.strftime('%Y%m%d_%H%M%S')
- "#{basename}_#{timestamp}#{extension}"
- end
+ def generate_unique_import_name(original_name)
+ # Fast path if unused
+ return original_name unless current_user.imports.exists?(name: original_name)
+
+ base = File.basename(original_name, File.extname(original_name))
+ ext = File.extname(original_name)
+
+ # Use microsecond precision and retry if needed to guarantee uniqueness
+ timestamp = Time.current.utc.strftime('%Y%m%d_%H%M%S%6N')
+ candidate = "#{base}_#{timestamp}#{ext}"
+ i = 1
+ while current_user.imports.exists?(name: candidate)
+ candidate = "#{base}_#{timestamp}_#{i}#{ext}"
+ i += 1
+ end
+ candidate
+ endIf there’s a unique DB index on imports.name, we’re covered at the database layer too. Check with:
🏁 Script executed:
#!/bin/bash
rg -nP --type=ruby -C2 'add_index\s+:imports\b.*\b:name\b.*\bunique:\s*true' dbLength of output: 81
🏁 Script executed:
#!/bin/bash
# Search for a unique DB index on imports.name in migrations
rg -nP --type=ruby -C2 'add_index\s+:imports\b.*\b:name\b.*unique:\s*true' db/migrate
# Search for inline t.index declarations in migrations
rg -nP --type=ruby -C2 't\.index\s+:name\b.*unique:\s*true' db/migrate
# Verify schema.rb for any index definitions on imports.name
if [ -f db/schema.rb ]; then
rg -nP 'add_index\s+"imports", \["name"\], unique: true' db/schema.rb
rg -nP 't\.index\s+:name, unique: true' db/schema.rb
fiLength of output: 313
Ensure true name uniqueness and clean up whitespace
I didn’t find a unique database index on imports.name, so name collisions could still occur if two imports are created within the same microsecond. Please:
- Harden the Ruby helper to use microsecond precision with a retry counter fallback for guaranteed uniqueness.
- Remove the RuboCop-flagged trailing whitespace.
- Add a unique index on
imports.nameat the database level (e.g. via a Rails migration) to enforce this constraint.
Apply these changes:
# app/controllers/imports_controller.rb
- def generate_unique_import_name(original_name)
- return original_name unless current_user.imports.exists?(name: original_name)
-
- # Extract filename and extension
- basename = File.basename(original_name, File.extname(original_name))
- extension = File.extname(original_name)
-
- # Add current datetime
- timestamp = Time.current.strftime('%Y%m%d_%H%M%S')
- "#{basename}_#{timestamp}#{extension}"
- end
+ def generate_unique_import_name(original_name)
+ # Fast path if the name isn’t taken
+ return original_name unless current_user.imports.exists?(name: original_name)
+
+ base = File.basename(original_name, File.extname(original_name))
+ ext = File.extname(original_name)
+ timestamp = Time.current.utc.strftime('%Y%m%d_%H%M%S%6N')
+
+ candidate = "#{base}_#{timestamp}#{ext}"
+ counter = 1
+ while current_user.imports.exists?(name: candidate)
+ candidate = "#{base}_#{timestamp}_#{counter}#{ext}"
+ counter += 1
+ end
+
+ candidate
+ endAnd add a migration such as:
class AddUniqueIndexToImportsName < ActiveRecord::Migration[6.1]
def change
add_index :imports, :name, unique: true
end
endThis combination guarantees both application-level and database-level uniqueness.
🧰 Tools
🪛 RuboCop (1.76.1)
[convention] 120-120: Trailing whitespace detected.
(Layout/TrailingWhitespace)
🤖 Prompt for AI Agents
In app/controllers/imports_controller.rb around lines 114 to 124, the helper
generate_unique_import_name can still collide and contains trailing whitespace;
update it to strip any surrounding whitespace from original_name, use
Time.current.strftime with microsecond precision (e.g. '%Y%m%d_%H%M%S_%6N') and
attempt a looped retry with an incrementing counter appended if a uniqueness
check still collides, returning a guaranteed-unique name; remove the trailing
whitespace in the file; and add a Rails migration to add a unique database index
on imports.name (e.g. add_index :imports, :name, unique: true) so uniqueness is
enforced at the DB level.
| google_phone_takeout: 3, gpx: 4, immich_api: 5, geojson: 6, photoprism_api: 7, | ||
| user_data_archive: 8 | ||
| } | ||
| }, allow_nil: true |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Enum now allows nil; verify downstream assumptions and queries
Allowing source to be nil matches the migration and tests. Please double-check:
- Any validations, scopes, or queries that previously assumed presence (e.g.,
where(source: ...)without handling nil, or UI filters) still behave correctly. - Background jobs and services that branch on
sourcetolerate nil (they should, givenshould_detect_source?gating in the service).
Run this quick scan to find potential assumptions:
🏁 Script executed:
#!/bin/bash
# Find validations or code paths that assume presence of `source`
rg -n -C2 --type ruby '\bvalidates\s+:source|\bsource\.\w+\?|where\([^)]*source:|\bgroup\(:source\)|pluck\(:source\)' -g '!**/spec/**'
# Find places that compare or branch on source without nil guards
rg -n -C2 --type ruby '\bif\s+.*source\b|\bcase\s+.*source\b'Length of output: 2961
Handle nil source in downstream code paths
The source enum now allows nil; several services, serializers, and helpers assume a non-nil value and will raise or return undesirable nils. Please address the following critical locations:
-
app/services/imports/watcher.rb (around line 73)
Callingsource.to_symonniltriggers aNoMethodError. Add a nil guard (e.g.case source&.to_sym) or early return, and define a fallback MIME type. -
app/services/imports/create.rb (
importermethod at lines 51–54)
case source.to_syields""fornil, so no branch matches and the method returnsnil. Either provide a default importer or raise a clear error for unsupported sources. -
app/services/photos/thumbnail.rb (around lines 31–33 and 49–50)
case sourceandif source == 'immich'silently skip fornil, leading tonilURLs or missing headers. Ensure there’s a sensible default path or explicit handling for anilsource. -
app/serializers/api/photo_serializer.rb (around lines 66–68)
orientationreturnsnilwhensourceisnil. Supply a default orientation (e.g.'landscape') or guard againstnil. -
app/helpers/trips_helper.rb (around lines 19–21)
photo_search_urlreturnsnilfor anilsource, breaking UI filters. Handle thenilcase with a default search URL or an error.
These code paths must be updated to either guard against nil or explicitly handle it to avoid runtime errors and ensure consistent behavior.
🤖 Prompt for AI Agents
In app/models/import.rb around line 24, allow_nil on the source enum exposes nil
downstream; update the listed callers to guard or handle nil: in
app/services/imports/watcher.rb (≈line 73) use a nil-safe check (e.g. case
source&.to_sym or early return) and provide a fallback MIME type; in
app/services/imports/create.rb (importer method lines 51–54) ensure nil maps to
a default importer or raise a clear "unsupported source" error instead of
returning nil; in app/services/photos/thumbnail.rb (≈lines 31–33 and 49–50) add
explicit handling for nil source (default thumbnail path or headers) so
URLs/headers are not nil; in app/serializers/api/photo_serializer.rb (≈lines
66–68) return a default orientation like 'landscape' when source is nil; in
app/helpers/trips_helper.rb (≈lines 19–21) make photo_search_url return a
sensible default URL or fallback filter when source is nil. Ensure each change
either short-circuits on nil with a clear fallback or raises a descriptive error
so downstream code never operates on an unexpected nil source.
| t.integer "source" | ||
| t.datetime "created_at", null: false | ||
| t.datetime "updated_at", null: false |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
imports.source default removed — confirm model/validations align
The column now has no default (nullable). Ensure:
enum :source, ...is declared withallow_nil: true(it is in the PR).- No
presence: truevalidations onsource. - UI/forms tolerate
nil.
Run:
🏁 Script executed:
#!/bin/bash
rg -nP 'enum\s+:source' -C2 app/models
rg -nP 'validates\s+:source,\s+presence:\s+true' -n app
rg -nP 'f\.select.*source' -n app/views app/helpers || trueLength of output: 774
🏁 Script executed:
#!/bin/bash
# Verify allow_nil on enum :source in import and place models
rg -nP 'enum\s+:source.*allow_nil' -n app/models || true
# Check form helpers referencing source
rg -nP 'f\.(?:select|input|collection_select).*:source' -n app/views app/helpers || trueLength of output: 147
🏁 Script executed:
#!/bin/bash
# Show the full enum declaration in Import model
sed -n '15,40p' app/models/import.rb
# Check for allow_nil usage in both models
grep -R -n "allow_nil" app/models/import.rb app/models/place.rb || true
# Show the full enum declaration in Place model
sed -n '10,25p' app/models/place.rbLength of output: 1182
Place model’s source enum needs allow_nil: true
The Import model correctly declares enum :source, …, allow_nil: true, but the Place model’s enum does not. Without allow_nil: true, assigning or persisting a nil source will raise an ArgumentError (since the DB default was removed), leading to errors when creating or updating Place records without a source value.
Please update:
-
app/models/place.rb (around line 15):
- enum :source, { manual: 0, photon: 1 } + enum :source, { manual: 0, photon: 1 }, allow_nil: true
This ensures the model aligns with the nullable column and prevents runtime exceptions when source is unset.
📝 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.
| t.integer "source" | |
| t.datetime "created_at", null: false | |
| t.datetime "updated_at", null: false | |
| # app/models/place.rb | |
| class Place < ApplicationRecord | |
| # … | |
| # Allow the source enum to accept nil, matching the nullable DB column | |
| enum :source, { manual: 0, photon: 1 }, allow_nil: true | |
| # … | |
| end |
🧰 Tools
🪛 RuboCop (1.76.1)
[convention] 102-102: Prefer single-quoted strings when you don't need string interpolation or special symbols.
(Style/StringLiterals)
[convention] 103-103: Prefer single-quoted strings when you don't need string interpolation or special symbols.
(Style/StringLiterals)
[convention] 104-104: Prefer single-quoted strings when you don't need string interpolation or special symbols.
(Style/StringLiterals)
🤖 Prompt for AI Agents
In app/models/place.rb around line 15, the Place model's enum :source is missing
allow_nil: true which causes ArgumentError when source is nil; update the enum
declaration to include allow_nil: true (e.g. enum source: { ... }, _prefix: ...,
allow_nil: true) so the model accepts nil values consistent with the nullable DB
column and prevents runtime exceptions when creating or updating Place without a
source.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
app/helpers/trips_helper.rb (1)
24-26: Remove redundant else-nil; let Ruby’s implicit nil return keep behavior and satisfy RuboCop.The explicit else returning nil is unnecessary and flagged by RuboCop (Style/EmptyElse). Dropping it keeps semantics identical and avoids the warning.
- else - nil # return nil for nil or unknown source endapp/services/photos/thumbnail.rb (1)
14-14: Avoid double-raising; call the error helper directly.unsupported_source_error already raises. Calling raise on its result is redundant and obscures the exception origin.
- raise unsupported_source_error unless SUPPORTED_SOURCES.include?(source) + unsupported_source_error unless SUPPORTED_SOURCES.include?(source)app/serializers/api/photo_serializer.rb (1)
61-63: Harden type derivation to avoid NoMethodError when type is absent.If both photo['type'] and photo['Type'] are nil, downcase will raise. Coerce to string first.
- (photo['type'] || photo['Type']).downcase + (photo['type'] || photo['Type']).to_s.downcaseapp/services/imports/create.rb (2)
51-53: Remove return raise anti-pattern.return raise is redundant; raise already exits control flow. Prefer plain raise for clarity.
- return raise ArgumentError, 'Import source cannot be nil' if source.nil? + raise ArgumentError, 'Import source cannot be nil' if source.nil?
19-26: Minor: consider normalizing source to string once.Downstream you case on source.to_s; consider normalizing when assigning to reduce repeated coercions and type ambiguity.
- source = if import.source.nil? || should_detect_source? - detect_source_from_file(temp_file_path) - else - import.source - end + source = if import.source.nil? || should_detect_source? + detect_source_from_file(temp_file_path).to_s + else + import.source.to_s + endThis also aligns with the should_detect_source? fix to consistently treat sources as strings.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
app/helpers/trips_helper.rb(1 hunks)app/serializers/api/photo_serializer.rb(1 hunks)app/services/imports/create.rb(3 hunks)app/services/imports/watcher.rb(1 hunks)app/services/photos/thumbnail.rb(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
app/services/photos/thumbnail.rb (1)
app/services/imports/watcher.rb (1)
source(54-70)
app/services/imports/create.rb (3)
app/services/imports/watcher.rb (1)
source(54-70)app/controllers/settings/users_controller.rb (1)
import(56-84)app/services/imports/source_detector.rb (1)
new_from_file_header(63-70)
🪛 RuboCop (1.76.1)
app/helpers/trips_helper.rb
[convention] 24-24: Redundant else-clause.
(Style/EmptyElse)
🔇 Additional comments (4)
app/services/photos/thumbnail.rb (1)
13-13: Good nil-guard to prevent downstream NoMethodError.Early failure with a clear message is appropriate here, given subsequent use of public_send on source.
app/serializers/api/photo_serializer.rb (1)
71-73: Sensible default orientation for unknown/nil sources.Defaulting to 'landscape' makes the API consistent and eliminates nil consumers must special-case.
app/services/imports/watcher.rb (1)
73-81: LGTM: nil-safe MIME resolution with a safe fallback.Switching to source&.to_sym and adding a nil branch prevents crashes when source is intentionally unset, while keeping strict errors for truly unsupported values.
app/services/imports/create.rb (1)
101-104: No action needed:import.sourceis always a String
ActiveRecord’s built-inenumfeature casts any symbol you assign (e.g.source: :immich_api) to its string key on read, soimport.sourcewill always return e.g."immich_api", never a:immich_apisymbol. Consequently,!%w[immich_api photoprism_api].include?(import.source)correctly handles all cases and adding
to_sis redundant (stackoverflow.com).
No changes or additional test coverage are required.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/services/imports/create.rb (1)
51-53: Good nil-guard; make the error more actionable.Include the import id in the message to speed up debugging.
- raise ArgumentError, 'Import source cannot be nil' if source.nil? + raise ArgumentError, "Import source cannot be nil for import ##{import.id}" if source.nil?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
app/services/imports/create.rb(3 hunks)app/services/photos/thumbnail.rb(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/services/photos/thumbnail.rb
🧰 Additional context used
🧬 Code graph analysis (1)
app/services/imports/create.rb (4)
app/services/imports/watcher.rb (1)
source(54-70)app/controllers/settings/users_controller.rb (1)
import(56-84)app/services/users/import_data.rb (1)
import(43-63)app/services/imports/source_detector.rb (1)
new_from_file_header(63-70)
🔇 Additional comments (2)
app/services/imports/create.rb (2)
106-110: LGTM on detector usage.Using the header-only detector keeps I/O bounded; exceptions bubble up to the existing rescue. No issues here.
101-105: Ignore the symbol/string mismatch warningThe Rails
enum :sourcedefinition returns the attribute value as a String (e.g."immich_api"), so calling.include?(import.source)against an array of strings works correctly. There is no symbol/string mismatch, and the existing guard behaves as intended.Optional: you may still choose to extract
%w[immich_api photoprism_api]into a frozenAPI_SOURCESconstant for clarity and reuse, but no change to.include?is required.Likely an incorrect or invalid review comment.
| source = if import.source.nil? || should_detect_source? | ||
| detect_source_from_file(temp_file_path) | ||
| else | ||
| import.source | ||
| end | ||
|
|
||
| import.update!(source: source) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Avoid overriding explicit sources and lazily download only when detection is needed.
- Current condition re-detects for every non-API source, potentially overriding a deliberately set
import.source. Confirm this is intended. - The file is downloaded before deciding whether detection is needed; for API sources this can be unnecessary or even fail if no file is attached.
Replace this block to defer downloading until needed and not override when detection is skipped:
- source = if import.source.nil? || should_detect_source?
- detect_source_from_file(temp_file_path)
- else
- import.source
- end
-
- import.update!(source: source)
+ source = import.source
+ temp_file_path = nil
+ if source.nil? || should_detect_source?
+ temp_file_path = Imports::SecureFileDownloader.new(import.file).download_to_temp_file
+ source = detect_source_from_file(temp_file_path)
+ end
+ import.update!(source: source)Note: With this change you should remove the unconditional download earlier in the method (Line 17) so the file is not fetched for API imports.
Please verify Photos::Importer accepts a nil temp_file_path:
🏁 Script executed:
#!/bin/bash
# Locate Photos::Importer initializer to confirm file arg is optional or unused.
rg -nP --type rb -C3 'class\s+Photos::Importer\b|def\s+initialize\(' appLength of output: 101
🏁 Script executed:
#!/bin/bash
# Locate Photos::Importer class definition
rg -n 'class Photos::Importer' -g '*.rb' app
# Inspect the initialize method signature in that file
rg -n -C3 'def initialize' -g '*.rb' appLength of output: 46105
Defer file download and preserve explicit sources
The current implementation always downloads the import file before checking if source detection is needed and may overwrite an explicitly set import.source. To avoid unnecessary downloads (especially for API imports without attachments) and to respect deliberate source assignments, update the code in app/services/imports/create.rb as follows:
• Only download when detection is required (i.e., when import.source is nil or should_detect_source? returns true).
• Preserve an existing import.source when detection is skipped.
• Remove the earlier unconditional download (around Line 17) so API imports without files won’t trigger a fetch.
• Note: Photos::Importer#initialize(import, user_id, file_path = nil) already accepts a nil file_path, so deferring download will not break its usage.
Locations to update:
- app/services/imports/create.rb (around Lines 17–25)
Proposed diff:
- # existing unconditional download at Line 17 – remove this:
- temp_file_path = Imports::SecureFileDownloader.new(import.file).download_to_temp_file
-
- source = if import.source.nil? || should_detect_source?
- detect_source_from_file(temp_file_path)
- else
- import.source
- end
-
- import.update!(source: source)
+ source = import.source
+ temp_file_path = nil
+
+ if source.nil? || should_detect_source?
+ # Only download when we need to detect
+ temp_file_path = Imports::SecureFileDownloader.new(import.file).download_to_temp_file
+ source = detect_source_from_file(temp_file_path)
+ end
+
+ import.update!(source: source)📝 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.
| source = if import.source.nil? || should_detect_source? | |
| detect_source_from_file(temp_file_path) | |
| else | |
| import.source | |
| end | |
| import.update!(source: source) | |
| source = import.source | |
| temp_file_path = nil | |
| if source.nil? || should_detect_source? | |
| # Only download when we need to detect | |
| temp_file_path = Imports::SecureFileDownloader.new(import.file).download_to_temp_file | |
| source = detect_source_from_file(temp_file_path) | |
| end | |
| import.update!(source: source) |
🤖 Prompt for AI Agents
In app/services/imports/create.rb around lines 17 to 25, remove the
unconditional file download and instead only download the file when detection is
required (i.e., when import.source.nil? || should_detect_source? is true); if
detection is required, download to temp_file_path, run
detect_source_from_file(temp_file_path) and set source from that detection,
otherwise preserve the existing import.source (do not overwrite it); ensure
import.update!(source: source) is only called with the selected source and that
Photos::Importer is invoked with file_path set to the temp path only when a file
was downloaded (pass nil otherwise).
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
app/services/photos/thumbnail.rb (4)
41-43: Harden URL joining to avoid accidental double slashesIf
source_urlends with a slash, concatenation can produce//which some proxies don’t normalize consistently. Safer join:def request_url base = source_url.to_s.chomp('/') path = source_path.to_s.sub(%r{\A/+}, '') "#{base}/#{path}" end
36-37: Guard against missing Photoprism preview tokenIf the cache returns nil, the generated URL will contain a blank token segment and likely 404. Fail fast with a clear error or attempt a refresh/fallback.
when 'photoprism' preview_token = Rails.cache.read("#{Photoprism::CachePreviewToken::TOKEN_CACHE_KEY}_#{user.id}") raise ArgumentError, 'Missing Photoprism preview token' if preview_token.blank? "/api/v1/t/#{id}/#{preview_token}/tile_500"If there’s a refresh helper (e.g.,
Photoprism::CachePreviewToken.fetch(user.id)), use it instead of raising.
23-29: Validate presence of expected settings methods to avoid NoMethodError
public_send("#{source}_url")andpublic_send("#{source}_api_key")will raise if the settings object doesn’t expose those keys for a given user/source combination. Given source is now nullable across the app, it’s worth guarding here too.def source_url key = "#{source}_url" settings = user.safe_settings raise ArgumentError, "Missing setting: #{key}" unless settings.respond_to?(key) settings.public_send(key) end def source_api_key key = "#{source}_api_key" settings = user.safe_settings return nil unless settings.respond_to?(key) # only immich uses it now settings.public_send(key) endAdd a small spec asserting a clear error when settings are misconfigured.
13-14: No symbol sources detected—normalization remains optionalI ran a project-wide search for calls like
Photos::Thumbnail.new(..., :some_source)and found no instances of symbol literals being passed as thesourceargument. While converting incoming values withsource.to_sis still a harmless future-proofing step, it isn’t strictly required based on current usage.• Verification command used:
rg -nP 'Photos::Thumbnail\.new\([^,]+,\s*:[A-Za-z_][A-Za-z0-9_]*' -C2– no matches found.
Optional refactor (unchanged from original suggestion):
- raise ArgumentError, 'Photo source cannot be nil' if source.nil? - unsupported_source_error unless SUPPORTED_SOURCES.include?(source) + raise ArgumentError, 'Photo source cannot be nil' if source.nil? + @source = source.to_s + unsupported_source_error unless SUPPORTED_SOURCES.include?(source)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
CHANGELOG.md(1 hunks)app/services/photos/thumbnail.rb(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- CHANGELOG.md
🧰 Additional context used
🧬 Code graph analysis (1)
app/services/photos/thumbnail.rb (1)
app/services/imports/watcher.rb (1)
source(54-70)
🪛 RuboCop (1.76.1)
app/services/photos/thumbnail.rb
[convention] 13-13: Add empty line after guard clause.
(Layout/EmptyLineAfterGuardClause)
🔇 Additional comments (1)
app/services/photos/thumbnail.rb (1)
13-13: RuboCop guard-clause empty-line hint looks like a false positiveThere’s already an empty line after the guard section (Line 15). With consecutive guard clauses, many teams prefer no empty line between guards. I’d keep as-is and ignore the cop here unless your style guide mandates spacing between consecutive guards.
| raise ArgumentError, 'Photo source cannot be nil' if source.nil? | ||
| unsupported_source_error unless SUPPORTED_SOURCES.include?(source) | ||
|
|
||
| HTTParty.get(request_url, headers: headers) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add timeouts and response/error handling for external HTTP call
Unbounded network calls can hang threads and degrade the app. Also, failures should surface as explicit errors. Add a sane timeout and check response.success?; log and/or raise on failure.
Here’s a targeted revision (illustrative; outside the changed lines):
def call
raise ArgumentError, 'Photo source cannot be nil' if source.nil?
@source = source.to_s
unsupported_source_error unless SUPPORTED_SOURCES.include?(source)
response = HTTParty.get(request_url, headers: headers, timeout: 5)
return response if response.success?
Rails.logger.warn("Thumbnail fetch failed for #{source} id=#{id} code=#{response.code} url=#{request_url}")
raise StandardError, "Failed to fetch thumbnail from #{source} (HTTP #{response.code})"
rescue HTTParty::Error, SocketError, Net::OpenTimeout, Net::ReadTimeout => e
Rails.logger.error("Thumbnail fetch error for #{source} id=#{id}: #{e.class} #{e.message}")
raise
end🤖 Prompt for AI Agents
In app/services/photos/thumbnail.rb around line 16, the HTTParty.get call is
unbounded and doesn't check the response; update the call to include a
reasonable timeout (e.g. timeout: 5) and then verify response.success? — if not,
log a warning with source, id, response.code and request_url and raise a
descriptive error; also rescue HTTParty::Error, SocketError, Net::OpenTimeout,
Net::ReadTimeout (and similar network errors), log the exception type/message as
an error and re-raise so failures surface instead of silently hanging.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores