0.36.0 #1952
0.36.0 #1952
Conversation
Feature/kml
Feature/omniauth
|
Caution Review failedThe pull request is closed. WalkthroughBumps app version to 0.36.0 and introduces OmniAuth (GitHub/Google/OIDC), KML import support with a Kml::Importer, tagging/privacy-zone models and UI, Places API and frontend (creation, filters, layers), Redis config adjustments, new migrations, many tests/fixtures, and extensive map UI/frontend additions. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser
participant Provider as OAuth Provider
participant App as App (/users/auth/:provider)
participant OMCC as Users::OmniauthCallbacksController
participant UserModel as Omniauthable.from_omniauth
participant Devise
Browser->>Provider: authenticate & consent
Provider-->>Browser: redirect to /users/auth/:provider/callback
Browser->>OMCC: GET /users/auth/:provider/callback (auth_hash)
OMCC->>UserModel: from_omniauth(auth_hash)
alt provider+uid exists
UserModel-->>OMCC: return existing user
else email matches existing user
UserModel-->>OMCC: link provider/uid to user
else create new user (if allowed)
UserModel-->>OMCC: return new user (persisted or not)
end
OMCC->>Devise: sign_in_and_redirect(user) or redirect_to registration with errors
Devise-->>Browser: set session / redirect
sequenceDiagram
participant Uploader as Client
participant ImportSvc as Imports::Create
participant Detector as SourceDetector
participant KmlImp as Kml::Importer
participant DB as Database
Uploader->>ImportSvc: upload file & start import
ImportSvc->>Detector: detect_source(file)
Detector-->>ImportSvc: :kml
ImportSvc->>KmlImp: instantiate(import, user_id, file_path)
KmlImp->>KmlImp: parse Placemarks, gx:Track, TimeStamp, ExtendedData
KmlImp->>DB: bulk upsert points (batched)
KmlImp-->>ImportSvc: broadcast progress / completion
ImportSvc-->>Uploader: notify finished / error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45–75 minutes Areas needing focused review:
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
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: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
app/javascript/controllers/maps_controller.js (3)
1962-1976: Critical: Modal z-index too low.The gradient editor modal has
zIndex: "100"(line 1975), but the settings panel hasz-index: 1000(fromapplication.cssline 36). The modal will appear behind the settings panel, making it unusable.Apply this fix:
Object.assign(modal.style, { position: "fixed", top: "0", left: "0", right: "0", bottom: "0", backgroundColor: "rgba(0, 0, 0, 0.5)", display: "flex", justifyContent: "center", alignItems: "center", - zIndex: "100", + zIndex: "10000", });
2003-2036: Add validation for gradient color stops.The gradient editor allows users to enter speed and color values without validation, which could result in invalid gradient configurations (e.g., negative speeds, duplicate speeds, unsorted stops).
Consider adding validation:
const createRow = (stop = { speed: 0, color: "#000000" }) => { const row = document.createElement("div"); // ... existing styling ... const speedInput = document.createElement("input"); speedInput.type = "number"; speedInput.value = stop.speed; speedInput.min = "0"; // Add minimum constraint speedInput.step = "1"; // Add step speedInput.style.width = "70px"; const colorInput = document.createElement("input"); colorInput.type = "color"; colorInput.value = stop.color; colorInput.style.width = "70px"; // ... rest of the function };Also validate on save that speeds are sorted in ascending order.
2074-2092: Validate and sort gradient stops before saving.When the user clicks "Save", ensure the gradient stops are valid and sorted by speed in ascending order. This prevents runtime errors from malformed gradient configurations.
Apply this enhancement:
const saveBtn = document.createElement("button"); saveBtn.textContent = "Save"; saveBtn.addEventListener("click", () => { const newStops = []; gradientContainer.querySelectorAll("div").forEach(row => { const inputs = row.querySelectorAll("input"); const speed = Number(inputs[0].value); const color = inputs[1].value; + if (isNaN(speed) || speed < 0) { + showFlashMessage('error', 'Speed values must be non-negative numbers.'); + return; + } newStops.push({ speed, color }); }); + // Sort by speed ascending + newStops.sort((a, b) => a.speed - b.speed); + + // Check for duplicates + const speeds = newStops.map(s => s.speed); + if (new Set(speeds).size !== speeds.length) { + showFlashMessage('error', 'Duplicate speed values are not allowed.'); + return; + } + const newGradient = colorFormatEncode(newStops); this.speedColorScale = newGradient; const speedColorScaleInput = document.getElementById("speed_color_scale"); if (speedColorScaleInput) { speedColorScaleInput.value = newGradient; } document.body.removeChild(modal); });config/initializers/sidekiq.rb (1)
27-27: Align client Redis configuration with server approach.The client configuration still embeds the database index in the URL path (old style), while the server configuration uses the separated hash approach (new style). This inconsistency may cause confusion.
Apply this diff to align the client configuration:
Sidekiq.configure_client do |config| - config.redis = { url: "#{ENV['REDIS_URL']}/#{ENV.fetch('RAILS_JOB_QUEUE_DB', 1)}" } + config.redis = { url: ENV['REDIS_URL'], db: ENV.fetch('RAILS_JOB_QUEUE_DB', 1) } end
🧹 Nitpick comments (15)
app/javascript/controllers/maps_controller.js (1)
1978-2002: Consider using CSS classes instead of inline styles.The modal content uses extensive inline styling. For better maintainability and consistency with the rest of the application, consider defining these styles in a CSS class.
Create a CSS class in
application.css:.gradient-editor-modal { position: fixed; top: 0; left: 0; right: 0; bottom: 0; background-color: rgba(0, 0, 0, 0.5); display: flex; justify-content: center; align-items: center; z-index: 10000; } .gradient-editor-content { background-color: white; padding: 20px; border-radius: 5px; min-width: 300px; max-height: 80vh; display: flex; flex-direction: column; }Then simplify the JavaScript:
const modal = document.createElement("div"); modal.className = "gradient-editor-modal"; const content = document.createElement("div"); content.className = "gradient-editor-content";CHANGELOG.md (1)
7-18: Add a language to the OIDC env vars code blockMarkdownlint points out the fenced block starting at Line 13 lacks a language. Consider annotating it for better tooling support and syntax highlighting:
-``` +```bash OIDC_CLIENT_ID=client_id_example OIDC_CLIENT_SECRET=client_secret_example OIDC_ISSUER=https://authentik.yourdomain.com/application/o/dawarich/ OIDC_REDIRECT_URI=https://your-dawarich-url.com/users/auth/openid_connect/callback</blockquote></details> <details> <summary>config/initializers/devise.rb (1)</summary><blockquote> `269-324`: **Tighten OIDC logging to avoid noisy warnings in cloud mode** The OmniAuth wiring itself looks good, but the OIDC warning in Lines 286–324 will log even when OIDC is intentionally disabled (cloud deployments): - In non-self-hosted environments, `SELF_HOSTED && ...` is false, so the `else` branch at Lines 322–324 executes, logging `OIDC: Not configured` on every boot, which is misleading. You can make the warning conditional on self-hosted mode: ```diff - if SELF_HOSTED && ENV['OIDC_CLIENT_ID'].present? && ENV['OIDC_CLIENT_SECRET'].present? + if SELF_HOSTED && ENV['OIDC_CLIENT_ID'].present? && ENV['OIDC_CLIENT_SECRET'].present? # ... - else - Rails.logger.warn 'OIDC: Not configured (missing OIDC_CLIENT_ID or OIDC_CLIENT_SECRET)' - end + elsif SELF_HOSTED + Rails.logger.warn 'OIDC: Not configured (missing OIDC_CLIENT_ID or OIDC_CLIENT_SECRET)' + endOptionally, you could also base the manual-mode log on the same defaulted scheme you use in
client_optionsto avoid logging a blank scheme whenOIDC_SCHEMEisn’t set.app/services/imports/source_detector.rb (1)
120-134: KML detection implementation is correct and follows existing patterns.The
kml_file?method appropriately mirrors thegpx_file?logic with KML-specific extension checks (.kml, .kmz) and XML markers. While RuboCop flags complexity metrics, the method is straightforward and readable.Optional: Consider extracting the common XML file detection pattern shared between
gpx_file?andkml_file?into a helper method to reduce duplication:def xml_file_matches?(extensions, xml_tag) return false unless filename&.downcase&.end_with?(*extensions) content_to_check = read_file_header(1024) ( content_to_check.strip.start_with?('<?xml') || content_to_check.strip.start_with?("<#{xml_tag}") ) && content_to_check.include?("<#{xml_tag}") end def kml_file? xml_file_matches?(['.kml', '.kmz'], 'kml') end def gpx_file? xml_file_matches?(['.gpx'], 'gpx') end def read_file_header(bytes) if file_path && File.exist?(file_path) File.open(file_path, 'rb') { |f| f.read(bytes) } else file_content end endThis refactor can be deferred to a future cleanup task.
db/migrate/20251028130433_add_omniauth_to_users.rb (1)
3-4: Consider using bulk: true for combining ALTER queries.RuboCop suggests using
change_tablewithbulk: trueto combine multiple column additions into a single ALTER TABLE statement, which is more efficient for the database.Apply this diff:
- def change - add_column :users, :provider, :string - add_column :users, :uid, :string - end + def change + change_table :users, bulk: true do |t| + t.string :provider + t.string :uid + end + endYou can also add the frozen string literal comment at the top of the file while you're at it:
+# frozen_string_literal: true + class AddOmniauthToUsers < ActiveRecord::Migration[8.0]spec/models/user_spec.rb (1)
340-431: LGTM! Comprehensive test coverage for OAuth user creation.The tests thoroughly cover all critical scenarios:
- Existing user lookup and linking
- New user creation with generated passwords
- Provider-specific flows (GitHub, Google)
- Validation failure cases (nil/blank email)
The test suite properly validates the
from_omniauthflow and edge cases.Optional: Fix hash indentation for consistency.
RuboCop flags minor indentation inconsistencies. You can apply this diff if you'd like to align with project style:
let(:auth_hash) do OmniAuth::AuthHash.new({ - provider: 'github', - uid: '123545', - info: { - email: email, - name: 'Test User' - } + provider: 'github', + uid: '123545', + info: { + email: email, + name: 'Test User' + } }) endapp/models/concerns/omniauthable.rb (1)
26-35: Optional: Simplify by directly returning the created user.The assignment before return is redundant and can be simplified.
Apply this diff:
# Create new user if not found - user = create( + create( email: data['email'], password: Devise.friendly_token[0, 20], provider: provider, uid: uid ) - - userapp/controllers/users/omniauth_callbacks_controller.rb (2)
44-53: Add exception handling for OAuth user creation.If
User.from_omniauthencounters an unexpected error (e.g., database connectivity issues, transaction failures), the exception will bubble up unhandled, potentially exposing stack traces to users.Consider wrapping the OAuth flow in exception handling:
def handle_auth(provider) - @user = User.from_omniauth(request.env['omniauth.auth']) + begin + @user = User.from_omniauth(request.env['omniauth.auth']) + rescue ActiveRecord::RecordInvalid, ActiveRecord::RecordNotSaved => e + redirect_to new_user_registration_url, alert: "Authentication failed: #{e.message}" + return + end if @user.persisted? flash[:notice] = I18n.t 'devise.omniauth_callbacks.success', kind: provider sign_in_and_redirect @user, event: :authentication else redirect_to new_user_registration_url, alert: @user.errors.full_messages.join("\n") end end
51-51: Line breaks in flash messages may not render correctly.Using
\nto join error messages in thealertflash may not render as line breaks in the browser, depending on how your views display flash messages.Consider using one of these alternatives:
- Join with
', 'for a comma-separated list- Use
<br>tags if your flash partial useshtml_safe- Display only the first error message
- redirect_to new_user_registration_url, alert: @user.errors.full_messages.join("\n") + redirect_to new_user_registration_url, alert: @user.errors.full_messages.join(', ')spec/requests/users/omniauth_callbacks_spec.rb (2)
12-12: Optional: Remove unused block parameter.The
provider_nameparameter in the shared example is not used in the test body.Apply this diff to follow Ruby conventions:
- shared_examples 'successful OAuth authentication' do |provider, provider_name| + shared_examples 'successful OAuth authentication' do |provider, _provider_name|Or remove it entirely if not needed:
- shared_examples 'successful OAuth authentication' do |provider, provider_name| + shared_examples 'successful OAuth authentication' do |provider|
1-96: Consider adding tests for the failure action.The test suite comprehensively covers successful OAuth flows but doesn't test the
failureaction in the controller. Given the complexity of the error handling logic (lines 16-40 in the controller), it would be beneficial to verify the various error scenarios.Consider adding a test context like:
describe 'GET /users/auth/failure' do it 'handles invalid credentials error' do OmniAuth.config.mock_auth[:openid_connect] = :invalid_credentials get '/users/auth/openid_connect/callback' expect(response).to redirect_to(root_path) expect(flash[:alert]).to include('Invalid credentials') end it 'handles timeout error' do OmniAuth.config.mock_auth[:openid_connect] = :timeout get '/users/auth/openid_connect/callback' expect(response).to redirect_to(root_path) expect(flash[:alert]).to include('timeout') end # Additional error scenarios... endspec/services/kml/importer_spec.rb (1)
30-34: Prefer expectations onKml::Importerinstances instead of theImports::BroadcastermoduleThe progress specs use:
expect_any_instance_of(Imports::Broadcaster).to receive(:broadcast_import_progress)
expect_any_instance_ofis designed to operate on classes whose instances receive the message, not on included modules.(rspec.toolboxforweb.xyz) Stubbing the module directly is brittle and may not behave as intended.Given
Imports::Broadcasteris included intoKml::Importer, it’s safer and clearer to target the importer instances:expect_any_instance_of(described_class) .to receive(:broadcast_import_progress).at_least(:once) parser(or switch to
expect_next_instance_ofif you want to avoidany_instanceentirely.)Also applies to: 44-48, 67-71, 81-85
spec/support/omniauth.rb (2)
15-18: Use zone‑aware helpers instead ofTime.nowin test helpersIn the three
expires_atfields you’re usingTime.now + .... That tripsRails/TimeZoneand is inconsistent with the rest of a Rails app’s time handling.Suggest updating to something like:
expires_at: 1.week.from_now # ... expires_at: 1.hour.from_now(or
Time.current + 1.week, etc.), which keeps tests aligned with zone‑aware time semantics.Also applies to: 39-43, 66-70
5-85: Long helper methods and hash indentation are purely stylistic issuesRuboCop is flagging method length and hash indentation in the three
mock_*_authhelpers. Functionally they’re fine; if you want a cleaner RuboCop run, options are:
- Extract a small helper that builds a common auth hash and override provider‑specific bits.
- Let RuboCop autocorrect the hash indentation.
This is optional and can be deferred.
app/services/kml/importer.rb (1)
45-74: Complex parsing methods are fine for now, but good candidates to split later
parse_placemark,parse_gx_track, andextract_extended_dataare doing a lot of work (multiple geometry types, timestamp handling, error handling, and data shaping), which is why RuboCop is flagging ABC size and method length.Functionally they look solid and well‑guarded against bad data; if you want to appease RuboCop or make future extensions easier, some low‑risk refactors would be:
- Extract small helpers per geometry type in
parse_placemark(point/linestring/multigeometry).- Move
gx:Trackparsing into a dedicated PORO or service.- In
extract_extended_data, split name/description extraction from the ExtendedData loop.These are non‑blocking and can be deferred.
Also applies to: 76-117, 185-207
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Gemfile.lockis excluded by!**/*.lock
📒 Files selected for processing (46)
.app_version(1 hunks)CHANGELOG.md(1 hunks)Gemfile(2 hunks)app/assets/stylesheets/application.css(1 hunks)app/controllers/users/omniauth_callbacks_controller.rb(1 hunks)app/javascript/controllers/maps_controller.js(1 hunks)app/models/concerns/omniauthable.rb(1 hunks)app/models/import.rb(1 hunks)app/models/user.rb(1 hunks)app/services/imports/create.rb(1 hunks)app/services/imports/source_detector.rb(2 hunks)app/services/kml/importer.rb(1 hunks)app/views/devise/registrations/edit.html.erb(1 hunks)app/views/devise/registrations/new.html.erb(1 hunks)app/views/devise/sessions/new.html.erb(1 hunks)app/views/devise/shared/_links.html.erb(1 hunks)app/views/imports/_form.html.erb(1 hunks)app/views/map/_settings_modals.html.erb(1 hunks)app/views/settings/index.html.erb(1 hunks)config/cable.yml(1 hunks)config/environments/development.rb(1 hunks)config/environments/production.rb(1 hunks)config/initializers/01_constants.rb(1 hunks)config/initializers/devise.rb(1 hunks)config/initializers/geocoder.rb(1 hunks)config/initializers/sidekiq.rb(1 hunks)config/routes.rb(1 hunks)db/migrate/20251028130433_add_omniauth_to_users.rb(1 hunks)docker/.env.example(1 hunks)docker/docker-compose.yml(1 hunks)postgresql.conf.example(0 hunks)spec/fixtures/files/kml/extended_data.kml(1 hunks)spec/fixtures/files/kml/gx_track.kml(1 hunks)spec/fixtures/files/kml/invalid_coordinates.kml(1 hunks)spec/fixtures/files/kml/large_track.kml(1 hunks)spec/fixtures/files/kml/linestring_track.kml(1 hunks)spec/fixtures/files/kml/multigeometry.kml(1 hunks)spec/fixtures/files/kml/nested_folders.kml(1 hunks)spec/fixtures/files/kml/points_with_timestamps.kml(1 hunks)spec/fixtures/files/kml/timespan.kml(1 hunks)spec/models/import_spec.rb(1 hunks)spec/models/user_spec.rb(1 hunks)spec/requests/users/omniauth_callbacks_spec.rb(1 hunks)spec/services/imports/source_detector_spec.rb(3 hunks)spec/services/kml/importer_spec.rb(1 hunks)spec/support/omniauth.rb(1 hunks)
💤 Files with no reviewable changes (1)
- postgresql.conf.example
🧰 Additional context used
📓 Path-based instructions (2)
spec/**/*.rb
📄 CodeRabbit inference engine (CLAUDE.md)
spec/**/*.rb: Use RSpec as the test framework
Use WebMock for external HTTP request mocking in tests
Files:
spec/models/user_spec.rbspec/models/import_spec.rbspec/services/imports/source_detector_spec.rbspec/services/kml/importer_spec.rbspec/requests/users/omniauth_callbacks_spec.rbspec/support/omniauth.rb
db/migrate/**/*.rb
📄 CodeRabbit inference engine (CLAUDE.md)
Use Strong Migrations practices for safe database schema changes
Files:
db/migrate/20251028130433_add_omniauth_to_users.rb
🧠 Learnings (5)
📚 Learning: 2025-09-29T21:15:25.496Z
Learnt from: CR
Repo: Freika/dawarich PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-09-29T21:15:25.496Z
Learning: Applies to db/migrate/**/*.rb : Use Strong Migrations practices for safe database schema changes
Applied to files:
db/migrate/20251028130433_add_omniauth_to_users.rb
📚 Learning: 2025-09-29T21:15:25.496Z
Learnt from: CR
Repo: Freika/dawarich PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-09-29T21:15:25.496Z
Learning: Applies to app/controllers/api/**/*.rb : Protect API endpoints with Bearer API keys, except explicitly public-share endpoints which must verify UUID access
Applied to files:
config/routes.rb
📚 Learning: 2025-09-29T21:15:25.496Z
Learnt from: CR
Repo: Freika/dawarich PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-09-29T21:15:25.496Z
Learning: Applies to spec/**/*.rb : Use WebMock for external HTTP request mocking in tests
Applied to files:
spec/support/omniauth.rb
📚 Learning: 2025-09-29T21:15:25.496Z
Learnt from: CR
Repo: Freika/dawarich PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-09-29T21:15:25.496Z
Learning: Applies to spec/factories/**/*.rb : Define test data with FactoryBot factories
Applied to files:
spec/support/omniauth.rb
📚 Learning: 2025-09-29T21:15:25.496Z
Learnt from: CR
Repo: Freika/dawarich PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-09-29T21:15:25.496Z
Learning: Applies to app/jobs/**/*.rb : Implement long-running or asynchronous work as Sidekiq background jobs
Applied to files:
config/initializers/sidekiq.rb
🧬 Code graph analysis (10)
spec/models/user_spec.rb (1)
app/models/concerns/omniauthable.rb (1)
from_omniauth(7-35)
spec/services/imports/source_detector_spec.rb (1)
app/services/imports/source_detector.rb (2)
detect_source(72-87)new_from_file_header(63-70)
app/models/user.rb (2)
app/controllers/users/registrations_controller.rb (1)
include(3-99)app/controllers/application_controller.rb (1)
include(3-90)
spec/services/kml/importer_spec.rb (2)
app/services/imports/create.rb (2)
call(13-48)include(3-120)app/services/kml/importer.rb (2)
call(17-41)include(5-234)
app/controllers/users/omniauth_callbacks_controller.rb (1)
app/models/concerns/omniauthable.rb (1)
from_omniauth(7-35)
spec/requests/users/omniauth_callbacks_spec.rb (1)
spec/support/omniauth.rb (1)
mock_openid_connect_auth(57-85)
spec/support/omniauth.rb (1)
app/models/user.rb (1)
include(3-188)
app/services/kml/importer.rb (2)
app/services/imports/file_loader.rb (1)
load_file_content(18-24)app/services/imports/broadcaster.rb (1)
broadcast_import_progress(4-16)
app/javascript/controllers/maps_controller.js (1)
app/javascript/maps/theme_utils.js (1)
applyThemeToPanel(74-78)
config/initializers/sidekiq.rb (1)
config/application.rb (1)
config(12-41)
🪛 markdownlint-cli2 (0.18.1)
CHANGELOG.md
13-13: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 RuboCop (1.81.7)
spec/models/user_spec.rb
[convention] 343-343: Use 2 spaces for indentation in a hash, relative to the first position after the preceding left parenthesis.
(Layout/FirstHashElementIndentation)
[convention] 349-349: Indent the right brace the same as the first position after the preceding left parenthesis.
(Layout/FirstHashElementIndentation)
[convention] 396-396: Use 2 spaces for indentation in a hash, relative to the first position after the preceding left parenthesis.
(Layout/FirstHashElementIndentation)
[convention] 402-402: Indent the right brace the same as the first position after the preceding left parenthesis.
(Layout/FirstHashElementIndentation)
app/services/imports/source_detector.rb
[convention] 120-134: Assignment Branch Condition size for kml_file? is too high. [<2, 15, 8> 17.12/17]
(Metrics/AbcSize)
[convention] 120-134: Cyclomatic complexity for kml_file? is too high. [8/7]
(Metrics/CyclomaticComplexity)
[convention] 120-134: Method has too many lines. [11/10]
(Metrics/MethodLength)
[convention] 120-134: Perceived complexity for kml_file? is too high. [9/8]
(Metrics/PerceivedComplexity)
db/migrate/20251028130433_add_omniauth_to_users.rb
[convention] 1-1: Missing frozen string literal comment.
(Style/FrozenStringLiteralComment)
[convention] 3-3: You can use change_table :users, bulk: true to combine alter queries.
(Rails/BulkChangeTable)
config/initializers/devise.rb
[convention] 270-281: Favor unless over if for negative conditions.
(Style/NegatedIf)
[convention] 320-320: Line is too long. [127/120]
(Layout/LineLength)
app/controllers/users/omniauth_callbacks_controller.rb
[convention] 16-40: Assignment Branch Condition size for failure is too high. [<3, 15, 11> 18.84/17]
(Metrics/AbcSize)
[convention] 16-40: Cyclomatic complexity for failure is too high. [11/7]
(Metrics/CyclomaticComplexity)
[convention] 16-40: Method has too many lines. [20/10]
(Metrics/MethodLength)
[convention] 16-40: Perceived complexity for failure is too high. [11/8]
(Metrics/PerceivedComplexity)
spec/requests/users/omniauth_callbacks_spec.rb
[warning] 12-12: Unused block argument - provider_name. If it's necessary, use _ or _provider_name as an argument name to indicate that it won't be used.
(Lint/UnusedBlockArgument)
app/models/concerns/omniauthable.rb
[convention] 7-35: Method has too many lines. [17/10]
(Metrics/MethodLength)
[convention] 27-32: Redundant assignment before returning detected.
(Style/RedundantAssignment)
spec/support/omniauth.rb
[convention] 6-28: Method has too many lines. [21/10]
(Metrics/MethodLength)
[convention] 8-8: Use 2 spaces for indentation in a hash, relative to the first position after the preceding left parenthesis.
(Layout/FirstHashElementIndentation)
[convention] 17-17: Do not use Time.now without zone. Use one of Time.zone.now, Time.current, Time.now.in_time_zone, Time.now.utc, Time.now.getlocal, Time.now.xmlschema, Time.now.iso8601, Time.now.jisx0301, Time.now.rfc3339, Time.now.httpdate, Time.now.to_i, Time.now.to_f instead.
(Rails/TimeZone)
[convention] 27-27: Indent the right brace the same as the first position after the preceding left parenthesis.
(Layout/FirstHashElementIndentation)
[convention] 30-55: Method has too many lines. [24/10]
(Metrics/MethodLength)
[convention] 32-32: Use 2 spaces for indentation in a hash, relative to the first position after the preceding left parenthesis.
(Layout/FirstHashElementIndentation)
[convention] 42-42: Do not use Time.now without zone. Use one of Time.zone.now, Time.current, Time.now.in_time_zone, Time.now.utc, Time.now.getlocal, Time.now.xmlschema, Time.now.iso8601, Time.now.jisx0301, Time.now.rfc3339, Time.now.httpdate, Time.now.to_i, Time.now.to_f instead.
(Rails/TimeZone)
[convention] 54-54: Indent the right brace the same as the first position after the preceding left parenthesis.
(Layout/FirstHashElementIndentation)
[convention] 57-85: Method has too many lines. [27/10]
(Metrics/MethodLength)
[warning] 57-57: Unused method argument - provider_name.
(Lint/UnusedMethodArgument)
[convention] 59-59: Use 2 spaces for indentation in a hash, relative to the first position after the preceding left parenthesis.
(Layout/FirstHashElementIndentation)
[convention] 69-69: Do not use Time.now without zone. Use one of Time.zone.now, Time.current, Time.now.in_time_zone, Time.now.utc, Time.now.getlocal, Time.now.xmlschema, Time.now.iso8601, Time.now.jisx0301, Time.now.rfc3339, Time.now.httpdate, Time.now.to_i, Time.now.to_f instead.
(Rails/TimeZone)
[convention] 84-84: Indent the right brace the same as the first position after the preceding left parenthesis.
(Layout/FirstHashElementIndentation)
app/services/kml/importer.rb
[convention] 5-234: Class has too many lines. [165/100]
(Metrics/ClassLength)
[convention] 17-41: Method has too many lines. [14/10]
(Metrics/MethodLength)
[convention] 45-74: Assignment Branch Condition size for parse_placemark is too high. [<10, 21, 6> 24.02/17]
(Metrics/AbcSize)
[convention] 45-74: Method has too many lines. [21/10]
(Metrics/MethodLength)
[convention] 76-117: Assignment Branch Condition size for parse_gx_track is too high. [<12, 29, 8> 32.39/17]
(Metrics/AbcSize)
[convention] 76-117: Cyclomatic complexity for parse_gx_track is too high. [8/7]
(Metrics/CyclomaticComplexity)
[convention] 76-117: Method has too many lines. [32/10]
(Metrics/MethodLength)
[convention] 92-92: Redundant begin block detected.
(Style/RedundantBegin)
[convention] 136-154: Assignment Branch Condition size for extract_timestamp is too high. [<4, 21, 4> 21.75/17]
(Metrics/AbcSize)
[convention] 156-170: Method has too many lines. [12/10]
(Metrics/MethodLength)
[convention] 185-207: Assignment Branch Condition size for extract_extended_data is too high. [<10, 17, 6> 20.62/17]
(Metrics/AbcSize)
[convention] 185-207: Method has too many lines. [14/10]
(Metrics/MethodLength)
[warning] 212-212: Unnecessary disabling of Rails/SkipsModelValidations.
(Lint/RedundantCopDisableDirective)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-and-push-docker
🔇 Additional comments (33)
app/views/devise/sessions/new.html.erb (1)
52-56: LGTM! Clean structural improvement.The form closing tag is now explicitly placed before the shared links partial, properly separating the email/password form from OAuth provider links. This aligns well with the OmniAuth integration work in this PR and maintains the existing invitation flow logic.
app/views/map/_settings_modals.html.erb (1)
187-187: LGTM!The label update from "Edit Scale" to "Edit Colors" appropriately reflects the gradient editor UI enhancement.
app/javascript/controllers/maps_controller.js (1)
958-1080: LGTM!The refactored settings form uses a clean, structured layout with appropriate Tailwind utility classes. The horizontal input groups with info buttons improve UX, and the smaller sizing (
input-sm,btn-sm) makes efficient use of the constrained panel width.app/views/devise/shared/_links.html.erb (1)
1-1: LGTM: Intentional spacing adjustment.The change from
my-5(margin top and bottom) tomt-5(margin top only) removes the bottom margin. This appears to be an intentional UI refinement consistent with the layout adjustments across Devise views.docker/docker-compose.yml (1)
23-23: LGTM: Helpful ARM architecture documentation.The comment provides clear guidance for ARM users to switch to the compatible PostGIS image. This is a helpful addition for multi-architecture deployments.
spec/fixtures/files/kml/multigeometry.kml (1)
1-28: LGTM: Well-structured MultiGeometry test fixture.This fixture provides good test coverage for KML files containing multiple geometry types (Points and LineString) within a single Placemark. The structure follows the KML 2.2 specification correctly, with a shared TimeStamp at the Placemark level.
app/views/devise/registrations/edit.html.erb (1)
69-70: LGTM: Prevents nested forms with OmniAuth buttons.Moving the shared links partial outside the form is correct. Since the partial includes OmniAuth
button_toelements (which generate their own forms), this change prevents invalid nested form structures in the HTML.config/environments/development.rb (1)
29-32: LGTM: Improved Redis configuration structure.The refactor to separate
urlanddbparameters is cleaner and more maintainable. This change aligns with the broader Redis configuration standardization across the codebase mentioned in the PR summary.spec/fixtures/files/kml/large_track.kml (1)
1-46: LGTM: Good fixture for batch processing tests.This fixture appropriately tests the KML importer's handling of multiple Placemarks with distinct timestamps and coordinate sequences. The 20 total points across two tracks provide reasonable coverage for batch insertion logic without being excessive.
spec/fixtures/files/kml/gx_track.kml (1)
1-19: KML fixture structure looks correct and usefulThe gx:Track fixture is well-formed and uses realistic timestamps and lon/lat/alt coords; good coverage for gx:Track parsing tests.
app/views/devise/registrations/new.html.erb (1)
77-81: Rendering Devise links outside the form is a good layout fixClosing the form before rendering
devise/shared/links(Line 77 onwards) is a sensible change and should avoid nested-form issues while matching the edit view behavior.app/models/import.rb (1)
22-26: KML source enum addition is consistent and safeExtending
sourcewithkml: 9at the end of the enum (Line 25) preserves existing mappings and cleanly supports the new import type.spec/fixtures/files/kml/extended_data.kml (1)
1-27: LGTM! Well-structured KML fixture.The fixture is well-formed and includes ExtendedData elements that will effectively test the KML importer's ability to parse metadata fields like speed, accuracy, and battery alongside basic point data.
spec/fixtures/files/kml/timespan.kml (1)
1-16: LGTM! Appropriate TimeSpan fixture.The fixture correctly demonstrates a Placemark with a TimeSpan duration (begin to end) rather than a single timestamp, which will validate proper handling of time ranges in the KML importer.
config/cable.yml (1)
3-4: LGTM! Cleaner Redis configuration approach.Separating the Redis database index into a distinct
dbattribute rather than embedding it in the URL path improves clarity and aligns with the broader Redis configuration refactor across the codebase.app/models/user.rb (1)
5-9: LGTM! Proper OmniAuth integration.The Omniauthable concern is correctly included and Devise is properly configured with
:omniauthableand the providers list from::OMNIAUTH_PROVIDERS. This cleanly integrates third-party authentication support.config/initializers/sidekiq.rb (1)
4-4: LGTM! Updated to separated Redis configuration.The server configuration now uses the cleaner separated approach with distinct
urlanddbkeys, consistent with the broader Redis configuration refactor.spec/fixtures/files/kml/invalid_coordinates.kml (1)
1-24: LGTM! Effective error-handling fixture.The fixture correctly demonstrates invalid coordinate scenarios (empty coordinates and incomplete longitude-only data) to validate that the KML importer properly handles and reports validation errors.
app/services/imports/source_detector.rb (1)
74-74: LGTM! KML detection properly integrated.KML detection is correctly added to the detection chain before falling back to OwnTracks format, consistent with the existing GPX detection pattern.
app/services/imports/create.rb (1)
61-61: LGTM!The KML importer integration follows the established pattern for other import sources.
config/initializers/geocoder.rb (1)
7-7: LGTM!Separating the Redis database index from the URL is cleaner and aligns with Redis client best practices.
spec/fixtures/files/kml/points_with_timestamps.kml (1)
1-33: LGTM!Valid KML fixture with properly structured placemarks, timestamps, and coordinates.
config/environments/production.rb (1)
76-79: LGTM!The Redis cache store configuration now uses a cleaner hash structure with separate
urlanddbparameters, consistent with other Redis configuration updates in this PR.spec/fixtures/files/kml/nested_folders.kml (1)
1-51: LGTM!Valid KML fixture demonstrating nested folder hierarchies with properly structured placemarks.
spec/fixtures/files/kml/linestring_track.kml (1)
1-21: LGTM!Valid KML fixture with LineString geometry for testing track parsing.
spec/models/import_spec.rb (1)
118-119: LGTM!The KML enum addition follows the established pattern and maintains proper sequencing.
spec/services/imports/source_detector_spec.rb (1)
77-84: LGTM! Comprehensive KML detection test coverage.The new KML detection tests appropriately cover:
- Content-based detection with .kml extension
- File header-based detection via
new_from_file_header- Multiple KML format variations (points with timestamps, linestring tracks, gx:Track, and multigeometry)
The tests follow existing patterns and integrate well with the new KML import functionality.
Also applies to: 158-165, 192-196
config/routes.rb (1)
104-108: LGTM! Correct OmniAuth callback routing.The addition of the
omniauth_callbackscontroller mapping properly wires up the OmniAuth authentication flow through Devise conventions.docker/.env.example (1)
142-187: LGTM! Excellent OIDC configuration documentation.The added OIDC configuration examples are comprehensive and well-structured:
- Clear distinction between discovery-based and manual endpoint configuration
- Provider-specific examples (Authelia, Authentik, Keycloak) offer practical guidance
- All values are properly documented as examples/placeholders
This will significantly help users configure OpenID Connect authentication.
Gemfile (2)
8-8: Verify the exact version constraint is intentional.The change from
~> 11.0to11.0prevents automatic patch updates (e.g., 11.0.1, 11.0.2). This is more restrictive and means you'll need to manually update for bug fixes.Was this intentional due to known compatibility issues with later 11.0.x versions?
27-30: LGTM! Appropriate OmniAuth gems added.The OmniAuth gem additions are well-chosen:
omniauth-githubandomniauth-google-oauth2for popular OAuth providersomniauth_openid_connectfor self-hosted OIDC providersomniauth-rails_csrf_protectionfor securityAll are standard, well-maintained gems for OmniAuth integration.
app/controllers/users/omniauth_callbacks_controller.rb (1)
29-37: The suggested refactor uses non-existent exception classes and would not work.The exception classes mentioned in the review (
OmniAuth::OpenIDConnect::DiscoveryError,OmniAuth::OpenIDConnect::ConfigurationError) do not appear to exist in the omniauth-openid-connect gem. Actual exceptions are nested underOpenIDConnect::ResponseObject::IdToken::(e.g.,InvalidIssuerfor issuer mismatch) orJSON::*namespaces.Additionally, the current code is already well-structured: it handles known error types via symbols (
:invalid_credentials,:timeout,:csrf_detected), and only falls back to string matching for unknown error types. The string patterns ("Issuer mismatch", "Discovery") likely correspond to wrapped exceptions from OmniAuth's request environment, making this approach reasonable given how OmniAuth surfaces errors.Likely an incorrect or invalid review comment.
app/services/kml/importer.rb (1)
221-221: Confirm thatunique_batch.sizeis the intended value forbroadcast_import_progress
broadcast_import_progress(import, unique_batch.size)passes the batch size as theindexargument, whichImports::Broadcasterexposes aspoints_countin the payload.If other importers pass a cumulative counter (e.g. total points processed so far) rather than per‑batch sizes, this will make KML progress look inconsistent in the UI.
If the intent is cumulative progress, track a running total in
calland pass that instead; otherwise, this is fine but worth double‑checking for consistency with the existing importers.
| div.style.width = '220px'; | ||
| div.style.maxHeight = 'calc(60vh - 20px)'; | ||
| div.style.overflowY = 'auto'; | ||
|
|
||
| // Prevent map interactions when interacting with the form | ||
| L.DomEvent.disableClickPropagation(div); | ||
| L.DomEvent.disableScrollPropagation(div); |
There was a problem hiding this comment.
Consider responsive panel width for smaller screens.
The fixed 220px width combined with the CSS left: 60px positioning could cause the panel to overflow on narrow viewports (e.g., mobile devices in portrait mode).
Consider a responsive approach:
applyThemeToPanel(div, this.userTheme);
div.style.padding = '10px';
- div.style.width = '220px';
+ div.style.width = window.innerWidth < 640 ? 'calc(100vw - 80px)' : '220px';
+ div.style.maxWidth = '220px';
div.style.maxHeight = 'calc(60vh - 20px)';
div.style.overflowY = 'auto';Alternatively, handle this in the CSS with media queries as suggested for the stylesheet.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In app/javascript/controllers/maps_controller.js around lines 1086 to 1092 the
popup panel is forced to a fixed width of 220px which can overflow on small
viewports; change the inline sizing to use responsive constraints (e.g. set
div.style.width to a relative value or remove fixed width and use
div.style.maxWidth = 'calc(100vw - 80px)' and keep div.style.maxHeight and
overflowY) or conditionally apply a smaller width for narrow screens via
window.matchMedia, and alternatively move the responsive rules into the
stylesheet with media queries so the panel adapts on mobile without overflowing;
ensure propagation disabling remains.
| def bulk_insert_points(batch) | ||
| unique_batch = batch.uniq { |record| [record[:lonlat], record[:timestamp], record[:user_id]] } | ||
|
|
||
| # rubocop:disable Rails/SkipsModelValidations | ||
| Point.upsert_all( | ||
| unique_batch, | ||
| unique_by: %i[lonlat timestamp user_id], | ||
| returning: false, | ||
| on_duplicate: :skip | ||
| ) | ||
| # rubocop:enable Rails/SkipsModelValidations | ||
|
|
||
| broadcast_import_progress(import, unique_batch.size) | ||
| rescue StandardError => e | ||
| create_notification("Failed to process KML file: #{e.message}") | ||
| end |
There was a problem hiding this comment.
🧩 Analysis chain
Don’t swallow upsert_all errors – let the main import flow fail the job
bulk_insert_points currently rescues all StandardErrors from Point.upsert_all, creates a notification, and then silently returns:
Point.upsert_all(...)
broadcast_import_progress(...)
rescue StandardError => e
create_notification("Failed to process KML file: #{e.message}")
endBecause Kml::Importer#call doesn’t re‑raise, Imports::Create#call will treat the import as successful, schedule stats and visit jobs, and eventually mark the import as completed, even if every batch write failed. The user gets an error notification but a “successful” import state, and downstream jobs run on missing/partial data.
Consider re‑raising after creating the notification so the outer import workflow can mark the import as failed and avoid scheduling follow‑up jobs, e.g.:
def bulk_insert_points(batch)
unique_batch = batch.uniq { |record| [record[:lonlat], record[:timestamp], record[:user_id]] }
# rubocop:disable Rails/SkipsModelValidations
Point.upsert_all(
unique_batch,
unique_by: %i[lonlat timestamp user_id],
returning: false,
on_duplicate: :skip
)
# rubocop:enable Rails/SkipsModelValidations
broadcast_import_progress(import, unique_batch.size)
rescue StandardError => e
create_notification("Failed to process KML file: #{e.message}")
+ raise
endYou’d then need to adjust the failing‑import spec to expect the raised error at the Imports::Create level rather than treating the importer as fully responsible for error handling.
---
🏁 Script executed:
```shell
#!/bin/bash
# Find the Kml::Importer class and its call method
fd -t f "importer\.rb" --search-path app/services/kml/
Length of output: 28
🏁 Script executed:
#!/bin/bash
# Find Imports::Create class
fd -t f -e rb --search-path app/services | xargs rg -l "class.*Imports.*Create|module.*Imports"Length of output: 102
🏁 Script executed:
#!/bin/bash
# Find test files related to KML importer
fd -t f -e rb --search-path spec | xargs rg -l "kml.*importer|Kml::Importer" -iLength of output: 34
🏁 Script executed:
#!/bin/bash
# Look at the full Kml::Importer implementation to see the call method
cat app/services/kml/importer.rbLength of output: 6747
🏁 Script executed:
#!/bin/bash
# Search for Imports::Create to understand the error handling flow
fd -t f -e rb -path "*/services/*" | xargs rg -l "class Create" | head -5Length of output: 415
---
🏁 Script executed:
```shell
#!/bin/bash
# Get the Imports::Create class to see how it handles importer errors
cat app/services/imports/create.rb
Length of output: 3500
🏁 Script executed:
#!/bin/bash
# Get the KML importer spec to see error handling tests
cat spec/services/kml/importer_spec.rbLength of output: 5222
🏁 Script executed:
#!/bin/bash
# Get the broadcaster to understand broadcast_import_progress
cat app/services/imports/broadcaster.rbLength of output: 560
---
🏁 Script executed:
```shell
#!/bin/bash
# Check other importers for error handling patterns
fd -t f "importer\.rb" --search-path app/services | grep -v kml | head -5 | xargs -I {} sh -c 'echo "=== {} ===" && tail -20 {}'
Length of output: 2816
🏁 Script executed:
#!/bin/bash
# Search for other rescue blocks in importers to compare error handling
rg -A5 "rescue StandardError" app/services/*/importer.rb --type rbLength of output: 26
Don't swallow upsert_all errors – let the main import flow fail the job
bulk_insert_points currently rescues all StandardErrors from Point.upsert_all, creates a notification, and then silently returns:
Point.upsert_all(...)
broadcast_import_progress(...)
rescue StandardError => e
create_notification("Failed to process KML file: #{e.message}")
endBecause Kml::Importer#call doesn't re-raise, Imports::Create#call will treat the import as successful, schedule stats and visit suggestion jobs, and eventually mark the import as completed in its ensure block—even if every batch write failed. The user gets an error notification but a "successful" import state, and downstream jobs run on missing or partial data.
Consider re-raising after creating the notification so the outer import workflow can mark the import as failed and avoid scheduling follow-up jobs:
def bulk_insert_points(batch)
unique_batch = batch.uniq { |record| [record[:lonlat], record[:timestamp], record[:user_id]] }
# rubocop:disable Rails/SkipsModelValidations
Point.upsert_all(
unique_batch,
unique_by: %i[lonlat timestamp user_id],
returning: false,
on_duplicate: :skip
)
# rubocop:enable Rails/SkipsModelValidations
broadcast_import_progress(import, unique_batch.size)
rescue StandardError => e
create_notification("Failed to process KML file: #{e.message}")
+ raise
endYou'd then need to adjust the error-handling spec to expect the exception to be raised at the Imports::Create level.
🧰 Tools
🪛 RuboCop (1.81.7)
[warning] 212-212: Unnecessary disabling of Rails/SkipsModelValidations.
(Lint/RedundantCopDisableDirective)
🤖 Prompt for AI Agents
In app/services/kml/importer.rb around lines 209 to 224, the rescue in
bulk_insert_points swallows all StandardError from Point.upsert_all and returns
after creating a notification, causing the outer import flow to treat the import
as successful; change the handler to create the notification and then re-raise
the original error (or raise a new wrapped error preserving cause) so the
exception bubbles to the caller and allows the import workflow to mark the
import as failed and avoid scheduling follow-up jobs; update the corresponding
specs to expect the exception to propagate to Imports::Create level.
| context 'when file has ExtendedData with speed' do | ||
| let(:file_path) { Rails.root.join('spec/fixtures/files/kml/extended_data.kml').to_s } | ||
|
|
||
| it 'creates points with velocity from ExtendedData' do | ||
| parser | ||
|
|
||
| point = user.points.first | ||
|
|
||
| expect(point.velocity).to eq('5.5') | ||
| end | ||
|
|
||
| it 'stores extended data in raw_data' do | ||
| parser | ||
|
|
||
| point = user.points.first | ||
|
|
||
| expect(point.raw_data['name']).to be_present | ||
| expect(point.raw_data['description']).to be_present | ||
| end | ||
| end |
There was a problem hiding this comment.
velocity expectation should be numeric, not a string
extract_velocity returns a Float and bulk_insert_points passes it directly into velocity, so point.velocity will be numeric. As long as the velocity column is numeric (float/decimal), expect(point.velocity).to eq('5.5') will fail strict equality.
Recommend asserting on a numeric value, e.g.:
expect(point.velocity).to eq(5.5)
# or, if you want tolerance:
# expect(point.velocity).to be_within(0.1).of(5.5)🤖 Prompt for AI Agents
In spec/services/kml/importer_spec.rb around lines 88 to 107, the test asserts
point.velocity as a string ('5.5') but extract_velocity returns a Float and the
velocity column is numeric; update the expectation to assert a numeric value
(e.g. 5.5) or use a tolerance matcher like be_within(...) to compare floating
point values.
| def mock_openid_connect_auth(email: '[email protected]', provider_name: 'Authelia') | ||
| OmniAuth.config.mock_auth[:openid_connect] = OmniAuth::AuthHash.new({ | ||
| provider: 'openid_connect', | ||
| uid: '123545', | ||
| info: { | ||
| email: email, | ||
| name: 'Test User', | ||
| image: 'https://example.com/avatar.jpg' | ||
| }, | ||
| credentials: { | ||
| token: 'mock_token', | ||
| refresh_token: 'mock_refresh_token', | ||
| expires_at: Time.now + 1.hour, | ||
| id_token: 'mock_id_token' | ||
| }, | ||
| extra: { | ||
| raw_info: { | ||
| sub: '123545', | ||
| email: email, | ||
| email_verified: true, | ||
| name: 'Test User', | ||
| preferred_username: 'testuser', | ||
| given_name: 'Test', | ||
| family_name: 'User', | ||
| picture: 'https://example.com/avatar.jpg' | ||
| } | ||
| } | ||
| }) | ||
| end |
There was a problem hiding this comment.
Handle or underscore the unused provider_name argument
mock_openid_connect_auth takes provider_name: but never uses it, which RuboCop correctly flags.
Either use it (for richer fixtures), e.g.:
extra: {
raw_info: {
preferred_username: 'testuser',
provider: provider_name,
# ...
}
}or mark it intentionally unused:
def mock_openid_connect_auth(email: '[email protected]', provider_name: 'Authelia')
_provider_name = provider_name
# ...
endThe former is usually more useful in tests.
🧰 Tools
🪛 RuboCop (1.81.7)
[convention] 57-85: Method has too many lines. [27/10]
(Metrics/MethodLength)
[warning] 57-57: Unused method argument - provider_name.
(Lint/UnusedMethodArgument)
[convention] 59-59: Use 2 spaces for indentation in a hash, relative to the first position after the preceding left parenthesis.
(Layout/FirstHashElementIndentation)
[convention] 69-69: Do not use Time.now without zone. Use one of Time.zone.now, Time.current, Time.now.in_time_zone, Time.now.utc, Time.now.getlocal, Time.now.xmlschema, Time.now.iso8601, Time.now.jisx0301, Time.now.rfc3339, Time.now.httpdate, Time.now.to_i, Time.now.to_f instead.
(Rails/TimeZone)
[convention] 84-84: Indent the right brace the same as the first position after the preceding left parenthesis.
(Layout/FirstHashElementIndentation)
🤖 Prompt for AI Agents
In spec/support/omniauth.rb around lines 57 to 85, the method argument
provider_name is never used; either use it in the fixture or explicitly mark it
unused. Fix by wiring provider_name into the mock auth hash: set the top-level
provider field to provider_name (or provider_name.downcase as appropriate) and
add a provider key inside extra[:raw_info] with provider_name; optionally derive
preferred_username from provider_name for richer fixtures. Alternatively, if you
truly want it unused, assign it to a prefixed variable (e.g., _provider_name =
provider_name) to silence RuboCop.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
CHANGELOG.md (1)
13-18: Minor: Add language identifier to code fence for better rendering.The fenced code block on line 13 is missing a language identifier, which can affect syntax highlighting and rendering in various markdown viewers.
Apply this diff to specify the language:
-``` +```bash OIDC_CLIENT_ID=client_id_example OIDC_CLIENT_SECRET=client_secret_example OIDC_ISSUER=https://authentik.yourdomain.com/application/o/dawarich/ OIDC_REDIRECT_URI=https://your-dawarich-url.com/users/auth/openid_connect/callback</blockquote></details> <details> <summary>spec/requests/api/v1/families/locations_spec.rb (1)</summary><blockquote> `68-69`: **Minor: Remove extra empty line at block end.** RuboCop has flagged an unnecessary empty line at the end of the describe block. Apply this diff: ```diff end end - endapp/controllers/family/location_sharing_controller.rb (1)
8-16: Consider adding strong parameters for input validation.While the service delegation pattern is good, the controller passes
params[:enabled]andparams[:duration]directly without defining strong parameters. Adding explicit parameter filtering would provide an additional layer of input validation at the controller level.Consider adding strong parameters:
def update result = Families::UpdateLocationSharing.new( user: current_user, enabled: location_sharing_params[:enabled], duration: location_sharing_params[:duration] ).call render json: result.payload, status: result.status end private def location_sharing_params params.permit(:enabled, :duration) end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
CHANGELOG.md(1 hunks)app/controllers/api/v1/families/locations_controller.rb(2 hunks)app/controllers/families_controller.rb(1 hunks)app/controllers/family/location_sharing_controller.rb(1 hunks)app/javascript/controllers/location_sharing_toggle_controller.js(1 hunks)config/routes.rb(3 hunks)spec/requests/api/v1/families/locations_spec.rb(1 hunks)spec/requests/family/location_sharing_spec.rb(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
app/controllers/api/**/*.rb
📄 CodeRabbit inference engine (CLAUDE.md)
Protect API endpoints with Bearer API keys, except explicitly public-share endpoints which must verify UUID access
Files:
app/controllers/api/v1/families/locations_controller.rb
spec/**/*.rb
📄 CodeRabbit inference engine (CLAUDE.md)
spec/**/*.rb: Use RSpec as the test framework
Use WebMock for external HTTP request mocking in tests
Files:
spec/requests/family/location_sharing_spec.rbspec/requests/api/v1/families/locations_spec.rb
🧠 Learnings (2)
📚 Learning: 2025-09-29T21:15:25.496Z
Learnt from: CR
Repo: Freika/dawarich PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-09-29T21:15:25.496Z
Learning: Applies to app/controllers/api/**/*.rb : Protect API endpoints with Bearer API keys, except explicitly public-share endpoints which must verify UUID access
Applied to files:
config/routes.rbspec/requests/api/v1/families/locations_spec.rb
📚 Learning: 2025-09-29T21:15:25.496Z
Learnt from: CR
Repo: Freika/dawarich PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-09-29T21:15:25.496Z
Learning: Applies to spec/factories/**/*.rb : Define test data with FactoryBot factories
Applied to files:
spec/requests/api/v1/families/locations_spec.rb
🧬 Code graph analysis (5)
app/controllers/api/v1/families/locations_controller.rb (3)
app/controllers/family/location_sharing_controller.rb (1)
before_action(3-25)app/controllers/api_controller.rb (1)
current_api_user(29-31)app/models/concerns/user_family.rb (1)
in_family?(16-18)
app/controllers/family/location_sharing_controller.rb (2)
app/controllers/api/v1/families/locations_controller.rb (2)
before_action(3-24)ensure_user_in_family!(19-23)app/models/concerns/user_family.rb (1)
in_family?(16-18)
app/controllers/families_controller.rb (1)
app/controllers/api/v1/families/locations_controller.rb (1)
before_action(3-24)
spec/requests/family/location_sharing_spec.rb (2)
app/javascript/controllers/location_sharing_toggle_controller.js (1)
response(65-76)app/models/concerns/user_family.rb (1)
update_family_location_sharing!(42-71)
spec/requests/api/v1/families/locations_spec.rb (2)
app/models/concerns/user_family.rb (1)
update_family_location_sharing!(42-71)app/controllers/api_controller.rb (1)
api_key(33-35)
🪛 markdownlint-cli2 (0.18.1)
CHANGELOG.md
13-13: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 RuboCop (1.81.7)
spec/requests/api/v1/families/locations_spec.rb
[convention] 68-69: Extra empty line detected at block body end.
(Layout/EmptyLinesAroundBlockBody)
🔇 Additional comments (10)
app/javascript/controllers/location_sharing_toggle_controller.js (1)
65-65: LGTM! Endpoint update aligns with backend refactoring.The endpoint change from
/family/update_location_sharingto/family/location_sharingcorrectly reflects the routing changes inconfig/routes.rbwhere the location sharing logic was moved to a dedicated controller (Family::LocationSharingController#update).app/controllers/families_controller.rb (1)
6-6: LGTM! Clean refactoring to dedicated controller.Removing
update_location_sharingfrom the before_action list is consistent with the extraction of location sharing functionality to the dedicatedFamily::LocationSharingController. This improves separation of concerns while keeping RESTful actions inFamiliesController.app/controllers/api/v1/families/locations_controller.rb (3)
3-3: LGTM! Controller namespace improves API organization.The rename from
Api::V1::FamiliesControllertoApi::V1::Families::LocationsControllerbetter reflects the dedicated responsibility for family location endpoints and aligns with RESTful conventions.
7-7: LGTM! Action rename follows REST conventions.Renaming the action from
locationstoindexfollows RESTful conventions for resource collections and aligns with the route changes inconfig/routes.rb.
20-20: LGTM! Safe navigation prevents potential NPE.The safe navigation operator
current_api_user&.in_family?correctly handles the case wherecurrent_api_usermight be nil (though authentication should prevent this). This defensive coding is good practice.app/controllers/family/location_sharing_controller.rb (1)
4-6: LGTM! Proper authentication and authorization guards.The before_action callbacks correctly enforce authentication (
authenticate_user!), feature availability (ensure_family_feature_enabled!), and authorization (ensure_user_in_family!) before allowing access to the update action.spec/requests/family/location_sharing_spec.rb (1)
1-117: LGTM! Excellent test coverage for location sharing feature.The test suite comprehensively covers:
- Enabling location sharing with time-limited and permanent durations
- Disabling location sharing
- Proper time handling using
travel_tofor deterministic assertions- Authorization checks (forbidden when not in family)
- Authentication requirements (unauthorized when not signed in)
- Error handling (internal server error on update failure)
The assertions correctly validate both the HTTP status codes and JSON response payloads.
config/routes.rb (3)
66-66: LGTM! Route correctly maps to dedicated location sharing controller.The new
patch 'location_sharing'route properly targets theFamily::LocationSharingController#updateaction with an appropriate route helper name. This aligns with the refactoring that extracted location sharing functionality fromFamiliesController.
107-107: LGTM! OmniAuth callbacks support for OIDC authentication.Adding
omniauth_callbacks: 'users/omniauth_callbacks'to the Devise configuration enables the OAuth/OIDC authentication flow described in the CHANGELOG. This is part of the authentication enhancements for GitHub, Google, and OpenID Connect.
174-176: LGTM! Namespace improves API route organization.Restructuring the families locations route from an inline collection route to a dedicated namespace with
resources :locations, only: [:index]improves API organization and aligns with the controller rename toApi::V1::Families::LocationsController.
…ategorize and label places with custom tags.
Feature/places management
There was a problem hiding this comment.
Actionable comments posted: 21
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
e2e/README.md (1)
60-60: Update line 60 and verify line 66 test count against actual listed tests.The inconsistency is confirmed. Line 60 references "40 tests total" (outdated) while line 66 states "81 tests." However, summing all the listed test files yields 93 tests total, not 81. Update line 60 to reflect the current count, and correct line 66 to match the actual sum of listed test files (93 tests).
spec/services/users/export_data_spec.rb (1)
192-192: Potential inconsistency: Mock still usesplacesinstead ofvisited_places.While lines 50 and 379 were updated to mock
visited_places, this line in the error context still mocksplaces. For consistency with the service implementation that usesuser.visited_places.count, consider updating this to:- allow(user).to receive(:places).and_return(double(count: 20)) + allow(user).to receive(:visited_places).and_return(double(count: 20))
♻️ Duplicate comments (1)
app/javascript/controllers/maps_controller.js (1)
1125-1169: Settings panel: new controls are useful, but width still fixed (mobile UX)The expanded settings form (fog-of-war thresholds, route segmentation, live map, speed colors) looks comprehensive and matches the existing
updateSettingswiring.However, the panel still uses a fixed width of
220px(Lines 1258-1259) plus margins, which was previously flagged as potentially overflowing on narrow screens. A more responsive approach would avoid horizontal clipping on small viewports, e.g.:- div.style.width = '220px'; - div.style.maxHeight = 'calc(60vh - 20px)'; + div.style.width = window.innerWidth < 640 ? 'calc(100vw - 80px)' : '220px'; + div.style.maxWidth = '220px'; + div.style.maxHeight = 'calc(60vh - 20px)';or move this logic into CSS with media queries.
Also applies to: 1181-1247, 1255-1265
🧹 Nitpick comments (43)
e2e/map/map-selection-tool.spec.js (1)
123-135: Consider usingtest.skip()or explicit assertions instead of silent early return.The guard logic silently skips the test when no points are found, which could mask test infrastructure issues. If data is expected on October 13, 2024, the absence of data might indicate a setup problem rather than an acceptable state.
Consider these alternatives:
Option 1: Use Playwright's conditional skip (recommended for flaky data scenarios):
- if (!hasPoints) { - console.log('No points found - skipping selection tool test'); - return; - } + test.skip(!hasPoints, 'No points found for selection test');Option 2: Explicitly assert data exists (recommended if data should be guaranteed):
- if (!hasPoints) { - console.log('No points found - skipping selection tool test'); - return; - } + expect(hasPoints).toBe(true); // Fails test if no data, revealing setup issuesOption 3: Ensure test data in setup (best for reliability):
Consider seeding test data in
beforeEachor a test fixture to guarantee points exist, eliminating the need for guards.spec/support/github_api_stubs.rb (2)
3-11: Consider limiting the scope of this global stub.This stub runs before every test in the entire suite, which may affect test isolation and could mask unexpected GitHub API calls. Tests that need different responses or want to verify that no GitHub calls are made will need to override or work around this global stub.
Consider these alternatives:
- Use a shared context that tests explicitly include when they need the stub
- Apply the stub only to tests tagged with a specific marker (e.g.,
:github_api)- Use a helper method that tests can call to set up the stub
Example with shared context:
# frozen_string_literal: true RSpec.shared_context 'with stubbed github tags' do before do stub_request(:get, 'https://api.github.com/repos/Freika/dawarich/tags') .to_return( status: 200, body: '[{"name": "1.0.0"}]', headers: {} ) end end RSpec.configure do |config| config.include_context 'with stubbed github tags', :github_api endThen tag specific tests with
:github_apito opt into the stub:RSpec.describe 'SomeFeature', :github_api do # This test suite will have the GitHub stub end
5-5: Use single-quoted string for the URL.The URL string doesn't require interpolation, so single quotes are preferred per Ruby conventions.
Apply this diff:
- stub_request(:get, "https://api.github.com/repos/Freika/dawarich/tags") + stub_request(:get, 'https://api.github.com/repos/Freika/dawarich/tags')Based on learnings (RuboCop hint).
e2e/map/map-side-panel.spec.js (1)
56-58: Consider using a static import and review the timeout duration.The dynamic
import()for the helper function is unusual here. Unless there's a specific reason (circular dependency, lazy loading), helpers are typically imported statically at the top of the file alongside other imports.Additionally, the
enableLayerhelper already includes a 1000ms wait after checking the checkbox (see line 38 ine2e/helpers/map.js), and line 58 adds another 1000ms wait, resulting in a total of 2000ms. While this may be intentionally defensive for test stability, consider whether the additional timeout is necessary.Apply this diff to use a static import:
import { test, expect } from '@playwright/test'; import { closeOnboardingModal, navigateToDate } from '../helpers/navigation.js'; import { drawSelectionRectangle } from '../helpers/selection.js'; +import { enableLayer } from '../helpers/map.js';Then update the function to remove the dynamic import:
async function selectAreaWithVisits(page) { // First, enable Suggested Visits layer to ensure visits are loaded - const { enableLayer } = await import('../helpers/map.js'); await enableLayer(page, 'Suggested'); - await page.waitForTimeout(1000); + await page.waitForTimeout(500); // enableLayer already waits 1000ms internallydb/seeds.rb (1)
45-50: Remove trailing comma.Remove the trailing comma after
'Travel Plans'on line 49 to align with Ruby style conventions.default_tags = [ { name: 'Home', color: '#FF5733', icon: '🏡' }, { name: 'Work', color: '#33FF57', icon: '💼' }, { name: 'Favorite', color: '#3357FF', icon: '⭐' }, - { name: 'Travel Plans', color: '#F1C40F', icon: '🗺️' }, + { name: 'Travel Plans', color: '#F1C40F', icon: '🗺️' } ]db/migrate/20251116184506_add_user_id_to_places.rb (1)
1-1: Add frozen string literal comment.Add the frozen string literal comment at the top of the file to align with project conventions.
+# frozen_string_literal: true + class AddUserIdToPlaces < ActiveRecord::Migration[8.0]config/importmap.rb (1)
17-17: Maintain consistent quote style.Use single quotes for string literals to maintain consistency with the rest of the file and align with Ruby style conventions.
-pin "leaflet" # @1.9.4 +pin 'leaflet' # @1.9.4 pin 'leaflet-providers' # @2.0.0 pin 'chartkick', to: 'chartkick.js' pin 'Chart.bundle', to: 'Chart.bundle.js' pin 'leaflet.heat' # @0.2.0 pin 'leaflet-draw' # @1.0.4 pin 'notifications_channel', to: 'channels/notifications_channel.js' pin 'points_channel', to: 'channels/points_channel.js' pin 'imports_channel', to: 'channels/imports_channel.js' pin 'family_locations_channel', to: 'channels/family_locations_channel.js' pin 'trix' pin '@rails/actiontext', to: 'actiontext.esm.js' -pin "leaflet.control.layers.tree" # @1.2.0 -pin "emoji-mart" # @5.6.0 +pin 'leaflet.control.layers.tree' # @1.2.0 +pin 'emoji-mart' # @5.6.0Also applies to: 29-30
app/services/reverse_geocoding/places/fetch_data.rb (1)
84-86: Fix indentation alignment.The
.globalscope addition appropriately filters places by the global scope. However, the indentation should be aligned with the preceding method call.Apply this diff to fix the indentation:
def find_existing_places(osm_ids) Place.where("geodata->'properties'->>'osm_id' IN (?)", osm_ids) - .global + .global .index_by { |p| p.geodata.dig('properties', 'osm_id').to_s } .compact endspec/services/reverse_geocoding/places/fetch_data_spec.rb (1)
594-616: Consider line length, but logic is sound.The test correctly validates that existing coordinates are preserved when updating geodata. The explicit latitude and longitude attributes on the factory call ensure coordinates are already set before the update.
Note: Line 594 exceeds the 120-character limit. Consider extracting the factory attributes to improve readability:
- let!(:existing_place) { create(:place, :with_geodata, lonlat: 'POINT(10.0 50.0)', latitude: 50.0, longitude: 10.0) } + let!(:existing_place) do + create(:place, :with_geodata, lonlat: 'POINT(10.0 50.0)', + latitude: 50.0, longitude: 10.0) + enddb/migrate/20251116184520_create_taggings.rb (1)
1-12: Add frozen string literal comment and apply style improvements.The migration structure is correct and follows Rails conventions. The polymorphic association and unique composite index are appropriate for a tagging system.
Apply these style improvements:
+# frozen_string_literal: true + class CreateTaggings < ActiveRecord::Migration[8.0] def change create_table :taggings do |t| t.references :taggable, polymorphic: true, null: false, index: true t.references :tag, null: false, foreign_key: true, index: true t.timestamps end - add_index :taggings, [:taggable_type, :taggable_id, :tag_id], unique: true, name: 'index_taggings_on_taggable_and_tag' + add_index :taggings, %i[taggable_type taggable_id tag_id], + unique: true, name: 'index_taggings_on_taggable_and_tag' end endapp/models/concerns/taggable.rb (2)
7-8: Add inverse_of for better performance.The associations are correctly structured for a polymorphic tagging system. However, adding
inverse_ofcan improve performance by avoiding extra queries.Apply this diff:
- has_many :taggings, -> { order(created_at: :asc) }, as: :taggable, dependent: :destroy + has_many :taggings, -> { order(created_at: :asc) }, as: :taggable, dependent: :destroy, inverse_of: :taggable has_many :tags, through: :taggings
10-14: Consider lambda syntax for multiline scope.The scopes implement useful filtering capabilities. The
tagged_withscope correctly filters by both tag name and user, which is important for multi-tenant scenarios.For the multiline lambda on lines 12-14, consider using the
lambdamethod for consistency:scope :with_tags, ->(tag_ids) { joins(:taggings).where(taggings: { tag_id: tag_ids }).distinct } scope :without_tags, -> { left_joins(:taggings).where(taggings: { id: nil }) } - scope :tagged_with, ->(tag_name, user) { + scope :tagged_with, lambda { |tag_name, user| joins(:tags).where(tags: { name: tag_name, user: user }).distinct }app/services/places/nearby_search.rb (2)
40-60: Fix indentation and trailing whitespace.The
format_resultsmethod correctly extracts and formats geocoder data. The coordinate fallback on line 43 is a good defensive practice.Apply this diff to fix style issues:
def format_results(results) results.map do |result| properties = result.data['properties'] || {} coordinates = result.data.dig('geometry', 'coordinates') || [longitude, latitude] { name: extract_name(result.data), latitude: coordinates[1], longitude: coordinates[0], osm_id: properties['osm_id'], osm_type: properties['osm_type'], osm_key: properties['osm_key'], osm_value: properties['osm_value'], city: properties['city'], country: properties['country'], street: properties['street'], housenumber: properties['housenumber'], postcode: properties['postcode'] } end end
62-69: Fix indentation in name fallback chain.The name extraction logic is well-structured with appropriate fallbacks. The chaining provides sensible defaults when primary data is missing.
Apply this diff to fix indentation:
def extract_name(data) properties = data['properties'] || {} - + properties['name'] || - [properties['street'], properties['housenumber']].compact.join(' ').presence || - properties['city'] || - 'Unknown Place' + [properties['street'], properties['housenumber']].compact.join(' ').presence || + properties['city'] || + 'Unknown Place' endapp/javascript/maps/places_control.js (1)
68-121: Consider extracting inline event handlers.The panel content generation works correctly, but the inline
onmouseover/onmouseouthandlers could be replaced with event delegation for better maintainability.While functional, consider using event delegation instead of inline handlers:
// In buildPanelContent, remove inline handlers: <label style="..." class="places-control-item"> <!-- Remove onmouseover and onmouseout --> </label> // Then add in onAdd or after buildPanelContent: this.panel.addEventListener('mouseover', (e) => { const item = e.target.closest('.places-control-item'); if (item) item.style.backgroundColor = 'rgba(128,128,128,0.2)'; }); this.panel.addEventListener('mouseout', (e) => { const item = e.target.closest('.places-control-item'); if (item) item.style.backgroundColor = 'transparent'; });spec/services/visits/place_finder_spec.rb (2)
23-23: Address RuboCop formatting violations.The line exceeds the 120-character limit, and the lonlat assignment makes it longer. Consider extracting the POINT string to a local variable or helper method for improved readability.
Apply this diff to improve readability:
- let!(:existing_place) { create(:place, latitude: latitude, longitude: longitude, lonlat: "POINT(#{longitude} #{latitude})") } + let!(:existing_place) do + create(:place, latitude: latitude, longitude: longitude, lonlat: "POINT(#{longitude} #{latitude})") + end
40-45: Fix indentation of closing parenthesis.RuboCop flags misaligned closing parenthesis and brace placement.
Apply this diff to fix the formatting:
it 'finds an existing place by name within search radius' do similar_named_place = create(:place, name: 'Test Place', latitude: latitude + 0.0001, longitude: longitude + 0.0001, - lonlat: "POINT(#{longitude + 0.0001} #{latitude + 0.0001})" - ) + lonlat: "POINT(#{longitude + 0.0001} #{latitude + 0.0001})")spec/models/tagging_spec.rb (1)
14-15: Fix trailing whitespace and symbol array style.RuboCop flags trailing whitespace on lines 14 and 21, and suggests using
%ifor the symbol array on line 15.Apply this diff to address the formatting issues:
describe 'uniqueness' do subject { create(:tagging) } - - it { is_expected.to validate_uniqueness_of(:tag_id).scoped_to([:taggable_type, :taggable_id]) } + + it { is_expected.to validate_uniqueness_of(:tag_id).scoped_to(%i[taggable_type taggable_id]) } end it 'prevents duplicate taggings' do tagging = create(:tagging) duplicate = build(:tagging, taggable: tagging.taggable, tag: tagging.tag) - + expect(duplicate).not_to be_valid endAlso applies to: 21-21
spec/models/user_spec.rb (1)
342-433: Excellent test coverage for OmniAuth, but fix indentation.The
.from_omniauthtests comprehensively cover:
- Existing user lookup by email
- New user creation
- Multiple OAuth providers (GitHub, Google)
- Edge cases (nil/blank email)
However, fix the hash indentation in the
auth_hashdefinitions for consistency.Apply this diff to fix the indentation:
let(:auth_hash) do OmniAuth::AuthHash.new({ - provider: 'github', - uid: '123545', - info: { - email: email, - name: 'Test User' - } - }) + provider: 'github', + uid: '123545', + info: { + email: email, + name: 'Test User' + } + }) endApply similar fixes to the Google OAuth auth_hash at lines 396-405.
spec/serializers/tag_serializer_spec.rb (1)
1-32: LGTM! Comprehensive serializer test with minor style issue.The test correctly validates the TagSerializer JSON structure, including all tag attributes and nested places data. The expected structure matches the implementation.
For consistency, fix the hash indentation per RuboCop conventions (lines 17 and 30).
db/migrate/20251118210506_add_note_to_places.rb (1)
1-5: LGTM! Safe migration, but add frozen string literal comment.The migration safely adds a nullable text column to the
placestable without requiring a table rewrite.For consistency with project conventions, add the frozen string literal comment:
+# frozen_string_literal: true + class AddNoteToPlaces < ActiveRecord::Migration[8.0] def change add_column :places, :note, :text end endspec/services/visits/creator_spec.rb (1)
27-50: User‑scoped visit assertions look good; place factories are slightly redundant
- Switching to
user.visits.reload.countmakes the specs safer in multi‑user scenarios and better reflects the behavior under test.- Adding
latitude/longitudeto theplacefactories aligns them with the model callbacks that buildlonlat.As a small optional cleanup, you could rely solely on
latitude/longitudein these specs and let the callback populatelonlat, which would also help shorten the longcreate(:place, lonlat: 'POINT(...)', ...)lines RuboCop flags.Also applies to: 63-94
app/javascript/controllers/privacy_radius_controller.js (1)
1-30: Stimulus controller is sound; consider explicit sync for prefilled valuesThe toggle/slider/label wiring is clear and should work well. One minor UX improvement: when the toggle turns on and
fieldTargetalready has a non‑blank value (e.g., editing an existing tag), you might want to also setsliderTarget.valueandlabelTarget.textContentfrom that value, instead of relying solely on the default‑value branch.CHANGELOG.md (1)
7-20: Annotate fenced OIDC env block with a languageThe OIDC env var example uses a bare triple‑backtick fence, which markdownlint flags. Consider changing it to something like:
-``` +```bash OIDC_CLIENT_ID=client_id_example ... -``` +```(or
shell/env) to satisfy MD040 and get syntax highlighting.spec/models/tag_spec.rb (1)
5-38: Good coverage of Tag associations, validations, and scopesThese specs exercise the key relationships, numeric radius bounds, uniqueness per user, color format, and the
.for_user/.orderedscopes appropriately. Only nit is stylistic: the long chainedvalidate_numericality_ofline exceeds RuboCop’s 120‑char limit; you could break it across lines if you want to keepLayout/LineLengthclean.spec/models/concerns/taggable_spec.rb (1)
108-121: Tightenchangematcher blocks to avoid RuboCop ambiguityThe tests exercise
add_tag/remove_tagcorrectly, but RuboCop’sLint/AmbiguousBlockAssociationand block-delimiter cops are flagging the multi-lineexpect { ... }.to/not_to change { ... }usages. You can clean this up without changing behavior by switching todo...endand/or parenthesizing thechangematcher, e.g.:expect do place.add_tag(tag1) end.to change { place.tags.count }.by(1) expect do place.remove_tag(tag3) end.not_to(change { place.tags.count })This keeps the intent clear and should satisfy the cops.
Also applies to: 139-155
spec/requests/api/v1/tags_spec.rb (1)
15-19: Solid privacy_zones request spec; consider small auth/style tweaksThe happy-path and unauthorized scenarios look good and match the serializer structure. Two optional improvements:
- Style: change
user.create_api_key unless user.api_key.present?touser.create_api_key if user.api_key.blank?to align with Rails/RuboCop conventions.- Auth path: since
ApiController#api_keyalso acceptsAuthorization: Bearer <key>, you may want a second authenticated context that passes the API key via the header to lock in that behavior.Based on learnings.
Also applies to: 25-42, 45-50
app/views/tags/index.html.erb (1)
20-44: Avoid potential N+1 ontag.places.countThe index view is clear and usable. To reduce DB chatter when many tags are listed, consider avoiding a per-row
COUNT(*):
- If the controller preloads
places, usetag.places.sizeinstead ofcount.- For larger datasets, a
places_countcounter cache ontags(and rendering that) would be even cheaper.app/serializers/tag_serializer.rb (1)
23-32: Consider if explicit float conversion is necessary.The
to_fconversion on lines 28-29 ensures numeric output, which is good for API consistency. However, iflatitudeandlongitudeare already numeric database columns (decimal/float), the conversion may be redundant. The current implementation is safe and works correctly.e2e/map/map-places-creation.spec.js (4)
11-28: Consider using more robust wait strategies.The test uses
page.waitForTimeout(300)on line 19, which introduces a fixed delay. While this works, it can make tests slower and potentially flaky if the UI takes longer than expected. Consider using Playwright's built-in waits likewaitForFunction()or checking for specific UI state changes instead of arbitrary timeouts.
170-179: Success message verification is somewhat generic.The success message check (lines 171-177) looks for elements with various patterns (
includes('success')orclassList.contains('alert-success')). While this is flexible, it might match unrelated messages. If possible, checking for more specific text like "Place created" or using a data attribute would make the assertion more precise.
210-222: Consider abstracting Leaflet internal access.The test directly accesses Leaflet's internal
_layersproperty (lines 215-219), which is a private implementation detail (indicated by the underscore prefix). While this works for testing, it couples the test to Leaflet's internal structure. If feasible, consider exposing a public method on the maps controller to check for place markers, which would make tests more maintainable.
266-333: Consider simplifying the test logic.The test uses a programmatic approach (lines 294-318) to simulate clicking at a different location because the modal interferes with UI clicks. While this works, the nested promise inside
page.evaluate()adds complexity. Consider if there's a simpler way to test coordinate updates, such as:
- Closing the modal between clicks
- Using the controller's public API more directly
- Extracting the coordinate update logic to a helper function
app/models/tagging.rb (1)
7-9: Consider using %i syntax for symbol array.The validations are correct. RuboCop suggests using
%isyntax for the symbol array on line 9 for consistency with Ruby style guidelines:- validates :tag_id, uniqueness: { scope: [:taggable_type, :taggable_id] } + validates :tag_id, uniqueness: { scope: %i[taggable_type taggable_id] }app/javascript/controllers/places_filter_controller.js (1)
8-26: Consider reducing coupling to global window object.The controller accesses
window.mapsControllerdirectly (line 10), which creates tight coupling between controllers. While this works, consider these alternatives for better encapsulation:
- Use Stimulus outlets to connect controllers
- Dispatch custom events that the maps controller listens for
- Use a shared service or state management approach
The current approach works but may make testing and maintenance more difficult.
app/policies/place_policy.rb (1)
4-8: Consider using consistent scope syntax.The scope uses
where(user_id: user.id)(line 6), whileTagPolicyuseswhere(user: user)(app/policies/tag_policy.rb:4). Both work correctly, but usingwhere(user: user)is more Rails-idiomatic and consistent with the codebase.db/migrate/20251116184514_create_tags.rb (1)
1-13: Migration is structurally sound and Strong Migrations‑friendlyCreating a new
tagstable with the FK and unique composite index in the same migration is safe here since the table is empty when the index/constraint are created. This looks good from a Strong Migrations perspective.If you want to align with RuboCop/style guidelines, you could optionally:
# frozen_string_literal: true add_index :tags, %i[user_id name], unique: truebut that’s purely cosmetic.
app/controllers/tags_controller.rb (1)
3-61: Controller wiring, scoping, and Pundit usage look correctAuthentication, ownership scoping (
current_user.tags.find) and Pundit calls (policy_scope,authorize) are consistent and aligned with the Tag model/policy. Strong params also match the intended attributes, includingprivacy_radius_meters.If you want to appease RuboCop,
before_action :set_tag, only: %i[edit update destroy]would satisfy the%istyle hint, but that’s optional.spec/requests/api/v1/places_spec.rb (1)
5-202: Good functional coverage for Places API (tags, auth, nearby, ownership)These request specs line up well with the controller behavior: per‑user scoping, tag assignment/filtering, ownership checks for show/update/destroy, nearby search, and API‑key authentication are all exercised.
Only minor style nits from RuboCop (trailing whitespace and
{ ... }for multi‑lineexpect { ... }.to changeblocks at e.g. Lines 77–80, 137–140, 148–152). Those can be cleaned up with RuboCop/autocorrect and don’t affect behavior.e2e/map/map-places-layers.spec.js (1)
250-276: Test name is misleading.The test "should maintain Places layer state across page reloads" (line 250) doesn't actually verify state persistence. Line 275 only checks that
isEnabledis a boolean, not that the state is maintained.Consider either:
- Renaming the test to reflect what it actually tests: "should query Places layer state without error after reload"
- Or implementing actual persistence verification:
- // Verify Places layer state after reload - isEnabled = await getPlacesLayerVisible(page); - // Note: State persistence depends on localStorage or other persistence mechanism - // If not implemented, this might be false, which is expected behavior - // For now, we just check the layer can be queried without error - expect(typeof isEnabled).toBe('boolean'); + // Verify Places layer state is maintained after reload + isEnabled = await getPlacesLayerVisible(page); + expect(isEnabled).toBe(true); // Should still be enabled if persistence is implementedapp/javascript/controllers/place_creation_controller.js (1)
171-214: Good error propagation on place creation, minor robustness noteThe submit handler correctly builds a JSON payload, surfaces server-side validation messages, and emits
place:createdfor other map components.One minor robustness tweak would be to guard against missing/invalid coordinates before calling
parseFloat, to avoid sendingNaN:- latitude: parseFloat(formData.get('latitude')), - longitude: parseFloat(formData.get('longitude')), + latitude: Number(formData.get('latitude')), + longitude: Number(formData.get('longitude')),and bail early or show a notification if either is
Number.isNaN(...).app/javascript/controllers/maps_controller.js (2)
637-693: Visits layers fetch logic may call the API multiple timesFor the
Suggested/Confirmedvisits layers:
overlayaddnow callsthis.visitsManager.fetchAndDisplayVisits()whenever either layer is enabled (Lines 673-679).initializeLayersFromSettingsalso callsfetchAndDisplayVisits()when restoringSuggestedorConfirmedfrom saved settings (Lines 1750-1753).Depending on how
VisitsManageris implemented, this can result in multiple identical API calls when toggling both layers or when restoring state plus user interaction.If
fetchAndDisplayVisits()already handles caching/idempotency, this is fine. Otherwise, consider centralizing the fetch so that enabling either layer just toggles visibility of already-fetched data.Also applies to: 698-733, 1704-1751
735-743: Place creation mode wiring is clean; add small guardrails
togglePlaceCreationModeanddisablePlaceCreationModecorrectly delegate toPlacesManager.enable/disableCreationMode, update the button styling viasetCreatePlaceButtonActive/Inactive, and listen forplace:created/place:create:cancelledto auto-exit creation mode (Lines 735-743, 2410-2450). This keeps the button state and internal mode in sync.Two small robustness ideas:
- In
togglePlaceCreationMode, guard against a missing#create-place-btnearlier and log once to avoid silent no-ops if the DOM structure changes.- Consider removing the
place:created/place:create:cancelledlisteners inremoveEventListeners()ordisconnect()to avoid stacking handlers if the controller is ever re-instantiated.Also applies to: 2410-2450
| setupEventListeners() { | ||
| document.addEventListener('place:create', (e) => { | ||
| this.open(e.detail.latitude, e.detail.longitude) | ||
| }) | ||
| } |
There was a problem hiding this comment.
Detach global place:create listener on disconnect to avoid leaks
setupEventListeners registers a document-level place:create listener but there is no corresponding cleanup. If the controller is ever disconnected/reconnected (e.g., Turbo navigation), you can accumulate multiple handlers.
Consider adding a disconnect() that removes the listener, storing the bound handler on this:
connect() {
this.setupEventListeners()
this.currentRadius = 0.5
this.maxRadius = 1.5
this.setupTagListeners()
}
setupEventListeners() {
- document.addEventListener('place:create', (e) => {
- this.open(e.detail.latitude, e.detail.longitude)
- })
+ this._handlePlaceCreate = (e) => {
+ this.open(e.detail.latitude, e.detail.longitude)
+ }
+ document.addEventListener('place:create', this._handlePlaceCreate)
}
+ disconnect() {
+ if (this._handlePlaceCreate) {
+ document.removeEventListener('place:create', this._handlePlaceCreate)
+ }
+ }📝 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.
| setupEventListeners() { | |
| document.addEventListener('place:create', (e) => { | |
| this.open(e.detail.latitude, e.detail.longitude) | |
| }) | |
| } | |
| connect() { | |
| this.setupEventListeners() | |
| this.currentRadius = 0.5 | |
| this.maxRadius = 1.5 | |
| this.setupTagListeners() | |
| } | |
| setupEventListeners() { | |
| this._handlePlaceCreate = (e) => { | |
| this.open(e.detail.latitude, e.detail.longitude) | |
| } | |
| document.addEventListener('place:create', this._handlePlaceCreate) | |
| } | |
| disconnect() { | |
| if (this._handlePlaceCreate) { | |
| document.removeEventListener('place:create', this._handlePlaceCreate) | |
| } | |
| } |
🤖 Prompt for AI Agents
In app/javascript/controllers/place_creation_controller.js around lines 17 to
21, the controller attaches a document-level 'place:create' listener in
setupEventListeners but never removes it on disconnect, which can cause handler
accumulation on Turbo navigation; store the bound listener on this (e.g.,
this.handlePlaceCreate = (e) => this.open(e.detail.latitude,
e.detail.longitude)) when adding it with document.addEventListener and implement
a disconnect() that calls document.removeEventListener('place:create',
this.handlePlaceCreate) to properly clean up the listener.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
db/migrate/20251118204141_add_privacy_radius_to_tags.rb (1)
6-20: Consider adding idempotency guards (optional).Following the pattern in
AddUserIdToPlaces.rb, you could add guards to make the migration more forgiving if rerun:def up + return if column_exists?(:tags, :privacy_radius_meters) + add_column :tags, :privacy_radius_meters, :integer add_index :tags, :privacy_radius_meters, where: 'privacy_radius_meters IS NOT NULL', algorithm: :concurrently end def down + return unless column_exists?(:tags, :privacy_radius_meters) + remove_index :tags, column: :privacy_radius_meters, where: 'privacy_radius_meters IS NOT NULL', algorithm: :concurrently remove_column :tags, :privacy_radius_meters endThis is purely defensive and follows the pattern used elsewhere in the codebase.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
CHANGELOG.md(1 hunks)app/javascript/controllers/color_picker_controller.js(1 hunks)app/views/imports/_form.html.erb(1 hunks)app/views/settings/index.html.erb(1 hunks)app/views/tags/_form.html.erb(1 hunks)db/migrate/20251028130433_add_omniauth_to_users.rb(1 hunks)db/migrate/20251116184506_add_user_id_to_places.rb(1 hunks)db/migrate/20251118204141_add_privacy_radius_to_tags.rb(1 hunks)db/migrate/20251118210506_add_note_to_places.rb(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- app/javascript/controllers/color_picker_controller.js
- app/views/imports/_form.html.erb
🧰 Additional context used
📓 Path-based instructions (1)
db/migrate/**/*.rb
📄 CodeRabbit inference engine (CLAUDE.md)
Use Strong Migrations practices for safe database schema changes
Files:
db/migrate/20251118204141_add_privacy_radius_to_tags.rbdb/migrate/20251028130433_add_omniauth_to_users.rbdb/migrate/20251116184506_add_user_id_to_places.rbdb/migrate/20251118210506_add_note_to_places.rb
🧠 Learnings (2)
📚 Learning: 2025-09-29T21:15:25.496Z
Learnt from: CR
Repo: Freika/dawarich PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-09-29T21:15:25.496Z
Learning: Applies to db/migrate/**/*.rb : Use Strong Migrations practices for safe database schema changes
Applied to files:
db/migrate/20251118204141_add_privacy_radius_to_tags.rbdb/migrate/20251028130433_add_omniauth_to_users.rbdb/migrate/20251116184506_add_user_id_to_places.rbdb/migrate/20251118210506_add_note_to_places.rb
📚 Learning: 2025-09-29T21:15:25.496Z
Learnt from: CR
Repo: Freika/dawarich PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-09-29T21:15:25.496Z
Learning: Add appropriate database indexes for geographic queries to ensure performance
Applied to files:
db/migrate/20251028130433_add_omniauth_to_users.rb
🧬 Code graph analysis (4)
db/migrate/20251118204141_add_privacy_radius_to_tags.rb (1)
db/migrate/20251116184506_add_user_id_to_places.rb (3)
disable_ddl_transaction!(1-14)up(4-9)down(11-13)
db/migrate/20251028130433_add_omniauth_to_users.rb (1)
db/migrate/20251118210506_add_note_to_places.rb (2)
change(1-5)change(2-4)
db/migrate/20251116184506_add_user_id_to_places.rb (1)
db/migrate/20251118204141_add_privacy_radius_to_tags.rb (3)
disable_ddl_transaction!(3-21)up(6-12)down(14-20)
db/migrate/20251118210506_add_note_to_places.rb (1)
db/migrate/20251028130433_add_omniauth_to_users.rb (2)
change(1-7)change(2-6)
🪛 markdownlint-cli2 (0.18.1)
CHANGELOG.md
25-25: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 RuboCop (1.81.7)
db/migrate/20251028130433_add_omniauth_to_users.rb
[convention] 1-1: Missing frozen string literal comment.
(Style/FrozenStringLiteralComment)
[convention] 5-5: Use %i or %I for an array of symbols.
(Style/SymbolArray)
db/migrate/20251116184506_add_user_id_to_places.rb
[convention] 1-1: Missing frozen string literal comment.
(Style/FrozenStringLiteralComment)
[convention] 6-6: Use a guard clause (return if column_exists?(:places, :user_id)) instead of wrapping the code inside a conditional expression.
(Style/GuardClause)
db/migrate/20251118210506_add_note_to_places.rb
[convention] 1-1: Missing frozen string literal comment.
(Style/FrozenStringLiteralComment)
🔇 Additional comments (6)
CHANGELOG.md (1)
32-51: Added, Fixed, and Changed sections look good.The documented features (KML uploads, tagging, privacy zones, home place logic), bug fixes, and infrastructure changes align well with the PR objectives. Entries are clear and appropriately attributed to issues where applicable.
app/views/settings/index.html.erb (1)
72-84: Connected Accounts guard and display look correctThe updated condition safely skips the block when self‑hosted or when
current_user.provideris blank, so.capitalizewill not raise and the UI shows only when there is a real OAuth provider. Implementation is consistent with the rest of the view.db/migrate/20251118204141_add_privacy_radius_to_tags.rb (1)
3-21: LGTM! Strong Migrations best practices followed.The migration correctly implements the pattern suggested in the previous review. The explicit
up/downmethods withdisable_ddl_transaction!andalgorithm: :concurrentlyensure safe, reversible schema changes without locking thetagstable.Based on coding guidelines.
db/migrate/20251118210506_add_note_to_places.rb (1)
1-5: LGTM! Migration follows Strong Migrations practices.The migration safely adds a nullable text column with proper idempotency guards. Adding a text column without a default value doesn't require a table rewrite, making this safe for production.
The frozen string literal comment flagged by RuboCop is a style convention that can be addressed if the project enforces it, but it doesn't affect the migration's safety or functionality.
Based on coding guidelines.
db/migrate/20251116184506_add_user_id_to_places.rb (1)
1-14: LGTM! Past review concerns have been addressed.The migration now correctly follows Strong Migrations practices:
- ✅ Uses
column_exists?(:places, :user_id)guard in both up and down methods (previously flagged issue withforeign_key_exists?has been fixed)- ✅ Properly uses
disable_ddl_transaction!with concurrent index creation- ✅ Column is nullable for backward compatibility
- ✅ Guards ensure idempotent execution
The RuboCop suggestions (guard clause style, frozen string literal) are minor style preferences that don't impact the migration's safety or correctness.
Based on coding guidelines and past review comments.
app/views/tags/_form.html.erb (1)
74-78: Past duplicate color field issue has been resolved.The previous review raised concerns about duplicate color form fields. The current implementation correctly uses a plain HTML
<input type="color">(lines 74-78) without anameattribute for the UI interaction, while the hidden field (line 93) is the sole submission field. This matches the recommended solution from the past review and eliminates the form submission conflict.Also applies to: 93-93
…layers, overwriting with incomplete data
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (7)
CHANGELOG.md (2)
9-19: Remove internal QA checklist from public changelog (duplicate issue).The "TEST" section with incomplete checklist items (
[ ]and[x]markers) remains in the public changelog. This is internal development tracking that should not be exposed to users. Remove this entire block before release.
25-30: Add language identifier to code block (duplicate issue).The fenced code block is missing a language identifier for proper syntax highlighting. Update it to declare
bashas the language.-``` +```bash OIDC_CLIENT_ID=client_id_example OIDC_CLIENT_SECRET=client_secret_example OIDC_ISSUER=https://authentik.yourdomain.com/application/o/dawarich/ OIDC_REDIRECT_URI=https://your-dawarich-url.com/users/auth/openid_connect/callback</blockquote></details> <details> <summary>app/javascript/maps/places.js (4)</summary><blockquote> `58-68`: **Align `tag_ids` format and auth between `loadPlaces` and `loadPlacesIntoLayer`** There are two different ways of calling `/api/v1/places`: - `loadPlaces` uses `URL` + `tag_ids[]` params and an `Authorization: Bearer` header. - `loadPlacesIntoLayer` uses a query `api_key=...` and a comma‑joined `tag_ids=1,2,3` string with no Authorization header. This can lead to subtle bugs if the backend expects the array format (e.g. `params[:tag_ids]` as array) or only one auth mechanism. Consider normalizing to the same style used in `loadPlaces`: ```diff - console.log(`[PlacesManager] loadPlacesIntoLayer called with tagIds:`, tagIds); - let url = `/api/v1/places?api_key=${this.apiKey}`; - - if (Array.isArray(tagIds) && tagIds.length > 0) { - // Specific tags requested - url += `&tag_ids=${tagIds.join(',')}`; - } else if (Array.isArray(tagIds) && tagIds.length === 0) { - // Empty array means untagged places only - url += '&untagged=true'; - } - - console.log(`[PlacesManager] Fetching from URL:`, url); - const response = await fetch(url); + console.log(`[PlacesManager] loadPlacesIntoLayer called with tagIds:`, tagIds); + const url = new URL('/api/v1/places', window.location.origin); + url.searchParams.append('api_key', this.apiKey); // or drop in favor of Authorization if not needed + + if (Array.isArray(tagIds) && tagIds.length > 0) { + tagIds.forEach(id => url.searchParams.append('tag_ids[]', id)); + } else if (Array.isArray(tagIds) && tagIds.length === 0) { + url.searchParams.append('untagged', 'true'); + } + + console.log(`[PlacesManager] Fetching from URL:`, url.toString()); + const response = await fetch(url.toString(), { + headers: { 'Authorization': `Bearer ${this.apiKey}` } + }); + if (!response.ok) throw new Error('Failed to load places into layer');Please also confirm which auth style (
api_keyvs Bearer) the backend officially supports here and keep both methods consistent.Also applies to: 292-307
93-103: Guard against 0‑coordinates and skip null markers in layer population
createPlaceMarkercurrently does a truthiness check on coordinates andloadPlacesIntoLayerunconditionally adds the returned marker:if (!place.latitude || !place.longitude) return null; // ... const marker = this.createPlaceMarker(place); layer.addLayer(marker);Issues:
0latitude/longitude is valid but falsy, so such places are silently dropped.- If
createPlaceMarkerever returnsnull,layer.addLayer(null)can throw.Use explicit
null/undefinedchecks and guard when adding to layers:- createPlaceMarker(place) { - if (!place.latitude || !place.longitude) return null; + createPlaceMarker(place) { + if (place.latitude == null || place.longitude == null) return null; @@ - data.forEach(place => { - const marker = this.createPlaceMarker(place); - layer.addLayer(marker); - }); + data.forEach(place => { + const marker = this.createPlaceMarker(place); + if (marker) { + layer.addLayer(marker); + } + });Also applies to: 292-317
105-107: Fixplace.tagsaccess increatePlaceIconto avoid crashes on synthetic places
createPlaceIconassumesplace.tagsexists:const emoji = place.icon || place.tags[0]?.icon || '📍'; const color = place.color || place.tags[0]?.color || '#4CAF50';But
handleMapClickcalls it with{ icon: '📍', color: '#FF9800' }, soplace.tagsisundefinedandplace.tags[0]throws before?.iconis evaluated.Use optional chaining on
tagsitself:- createPlaceIcon(place) { - const emoji = place.icon || place.tags[0]?.icon || '📍'; - const color = place.color || place.tags[0]?.color || '#4CAF50'; + createPlaceIcon(place) { + const emoji = place.icon || place.tags?.[0]?.icon || '📍'; + const color = place.color || place.tags?.[0]?.color || '#4CAF50';This keeps behavior for API‑returned places while making
handleMapClicksafe.Also applies to: 185-197
135-155: Escape tag/name values (and ideally place name) in popup HTML to prevent XSSPopup content is built with raw interpolation:
const tags = place.tags.map(tag => `<span class="badge badge-sm" style="background-color: ${tag.color}"> ${tag.icon} #${tag.name} </span>` ).join(' '); return ` <div class="place-popup"> <h3 ...>${place.name}</h3> ${tags ? `<div class="mb-2">${tags}</div>` : ''} ... </div> `;If
tag.icon,tag.name, orplace.nameever contain attacker‑controlled HTML, this can execute scripts because the string is assigned toinnerHTML.You already have
escapeHtml; reuse it here (and optionally constrain/validate colors):- const tags = place.tags.map(tag => - `<span class="badge badge-sm" style="background-color: ${tag.color}"> - ${tag.icon} #${tag.name} - </span>` - ).join(' '); + const tags = (place.tags || []).map(tag => + `<span class="badge badge-sm" style="background-color: ${tag.color}"> + ${this.escapeHtml(tag.icon || '')} #${this.escapeHtml(tag.name || '')} + </span>` + ).join(' '); @@ - <h3 class="font-bold text-lg mb-2">${place.name}</h3> + <h3 class="font-bold text-lg mb-2">${this.escapeHtml(place.name || '')}</h3>app/javascript/controllers/maps_controller.js (1)
1138-1269: Settings panel width: still fixed at 220px, consider responsive sizingThe settings panel is rendered with a hardcoded width:
div.style.width = '220px'; div.style.maxHeight = 'calc(60vh - 20px)'; div.style.overflowY = 'auto';On narrow viewports,
220pxplus padding/positioning can easily overflow or crowd other controls (this was flagged in an earlier review as well).Consider making the width responsive, e.g.:
- div.style.width = '220px'; - div.style.maxHeight = 'calc(60vh - 20px)'; + div.style.width = window.innerWidth < 640 ? 'calc(100vw - 80px)' : '220px'; + div.style.maxWidth = '220px'; + div.style.maxHeight = 'calc(60vh - 20px)';or moving these rules into a CSS class with media queries.
🧹 Nitpick comments (3)
app/javascript/controllers/maps_controller.js (3)
1694-1824: Tree checkbox sync: skip group headers like “Places”/“Visits” when updating
updateTreeControlCheckboxeswalks all checkboxes and drives theircheckedstate fromenabledLayersandplacesFilteredLayers:const layerName = label.textContent.trim(); // ... let shouldBeEnabled = enabledLayers.includes(layerName); // ... if (!shouldBeEnabled && this.placesFilteredLayers) { const placeLayer = this.placesFilteredLayers[layerName]; if (placeLayer && placeLayer._placeTagId !== undefined) { shouldBeEnabled = enabledTagIds.has(placeLayer._placeTagId); } } // Skip only 'Map Styles' / 'Layers' headers if (layerName && !layerName.includes('Map Styles') && !layerName.includes('Layers')) { input.checked = shouldBeEnabled; }Because “Places” and “Visits” groups also use
selectAllCheckbox: true, they have their own checkboxes but:
- They’re not part of
enabled_map_layers.- There’s no corresponding entry in
placesFilteredLayers.The result is that
shouldBeEnabledis alwaysfalsefor those headers, so their checkboxes get forcibly unchecked even when some children are enabled, which can be confusing.At minimum, treat these group headers like “Map Styles”/“Layers” and skip them:
- if (layerName && !layerName.includes('Map Styles') && !layerName.includes('Layers')) { + if ( + layerName && + !['Map Styles', 'Layers', 'Places', 'Visits'].includes(layerName) + ) {If the plugin exposes a better way to detect group vs leaf nodes (e.g. a data attribute), keying off that would be more robust than matching literal strings.
1457-1477: Re-creating tree control after settings update: consider reusing existing overlay objectsIn
updateMapWithNewSettings, you:
- Snapshot which standard layers are visible into
layerStates.- Remove the existing layer control.
- Recreate it via
this.layerControl = this.createTreeLayerControl(); this.map.addControl(this.layerControl);- Re-add/remove layers based on
layerStates.Because
createTreeLayerControlcreates fresh layerGroup instances for scratch and all place-tag overlays, the new control is wired to new overlay objects, whilelayerStatesand the map may still hold references to the old ones. For the standard overlays you handle inlayerStatesthis is fine, but for scratch/places/visits you can easily end up with:
- Old overlays still on the map but no longer controlled.
- New overlays in the tree that start disabled and empty.
This ties into the earlier comment about reusing
placesFilteredLayers; a similar principle applies here: prefer to build the tree around existing overlay instances (this.scratchLayerManager.getLayer(), existing filtered layers, etc.) rather than constructing new ones on every control rebuild.
228-229: Global exposure of managers (window.mapsController, visitsManager, placesManager)You’re exposing the main controller and
VisitsManagerglobally:this.visitsManager = new VisitsManager(...); window.visitsManager = this.visitsManager; window.mapsController = this;Given these are now used by other controllers (places filter, family, etc.), this is pragmatic, but it also couples modules via globals. For future cleanup, consider:
- Using custom events (
dispatchEvent/addEventListener) for cross‑controller coordination instead of globals.- Or a small registry module that exports getters/setters instead of writing to
window.Not a blocker, just something to keep in mind as the map surface grows.
Also applies to: 243-245, 295-299
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
CHANGELOG.md(1 hunks)app/helpers/tags_helper.rb(1 hunks)app/javascript/controllers/maps_controller.js(24 hunks)app/javascript/maps/places.js(1 hunks)app/views/tags/_form.html.erb(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
app/javascript/controllers/maps_controller.js (4)
app/javascript/controllers/place_creation_controller.js (5)
error(199-199)event(67-67)event(208-208)event(217-220)div(226-226)app/javascript/maps/privacy_zones.js (1)
PrivacyZoneManager(7-173)app/javascript/maps/places.js (1)
PlacesManager(7-392)app/javascript/maps/map_controls.js (5)
button(16-16)button(101-101)button(137-137)setCreatePlaceButtonInactive(244-251)setCreatePlaceButtonActive(229-237)
app/javascript/maps/places.js (4)
app/javascript/controllers/place_creation_controller.js (9)
event(67-67)event(208-208)event(217-220)place(203-203)tagIds(175-176)response(84-87)response(189-196)div(226-226)data(91-91)app/javascript/maps/helpers.js (1)
showFlashMessage(122-192)app/javascript/maps/places_control.js (2)
tagIds(179-179)div(212-212)app/javascript/controllers/maps_controller.js (6)
response(2013-2013)response(2174-2179)data(2185-2185)layerControl(1827-1827)inputs(1643-1643)inputs(1843-1843)
🪛 markdownlint-cli2 (0.18.1)
CHANGELOG.md
25-25: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (7)
CHANGELOG.md (1)
32-52: Changelog content is well-structured and comprehensive.The new Added, Fixed, and Changed sections clearly document the substantial features introduced in this release (OAuth support, KML import, tagging, privacy zones, Places API, Redis configuration changes). The entries are appropriately detailed with references to related issues where applicable.
app/helpers/tags_helper.rb (1)
1-20: LGTM! Clean helper implementation.The module correctly provides a frozen emoji set for tag iconography and a helper to select random emojis. The constant is properly immutable and the method is straightforward.
app/javascript/controllers/maps_controller.js (5)
182-188: Privacy zones: good initialization order; consider future live-map filteringLoading privacy zones and filtering
this.markers/this.tracksDatabefore creating layers ensures routes/points respect privacy zones on initial render. The integration withPrivacyZoneManager.filterPoints/filterTrackslooks sound.One thing to keep in mind for later: live-updated points/tracks (via
LiveMapHandler/ channels) are not currently run throughPrivacyZoneManager, so they may bypass privacy filtering until the next full reload. If that matters for your threat model, you might want a follow-up to hook privacy filtering into the incremental update path.Also applies to: 2468-2486
735-743: Place creation toggle flow looks coherentThe place‑creation toggle is wired through:
- Top‑right button handler (
onCreatePlace: () => this.togglePlaceCreationMode()).togglePlaceCreationModecheckingthis.placesManager.creationModeand updating the button viasetCreatePlaceButtonActive/Inactive.- Global listeners on
place:createdandplace:create:cancelledboth callingdisablePlaceCreationMode()to reset UI + cursor and clean up the temporary marker.This keeps map state, button styling, and creation mode in sync, and guards against missing
placesManager.Also applies to: 2426-2466
768-810: Layer preference persistence: good coverage, just ensure place-tag format is stableThe updated
saveEnabledLayers/initializeLayersFromSettingspair now:
- Persist standard overlay visibility by name (“Points”, “Routes”, “Heatmap”, “Suggested”, “Confirmed”, etc.).
- Persist per‑tag places layers as
place_tag:<id>andplace_tag:untagged.- Restore place-tag layers with
isRestoringLayersset to avoid spurious saves, and then reconcile tree checkboxes viaupdateTreeControlCheckboxes.This is a solid approach; just make sure the server doesn’t rely on any particular ordering for the
enabled_map_layersarray and that theplace_tag:prefix is considered stable API going forward (it’s now part of what gets stored in settings).Also applies to: 1698-1715, 1787-1817
228-236: Visits/Places integration in tree control looks consistentThe new overlays subtree:
- Adds a “Visits” group with “Suggested” and “Confirmed” layers wired to
VisitsManagergetters.- Adds a “Places” group with “Untagged” and per‑tag filtered layers created via
PlacesManager.createFilteredLayer.Combined with the overlayadd/overlayremove handlers that fetch visits and manage scratch/fog, this matches the intended UX for visits and tag‑filtered places. No obvious logic errors here.
Also applies to: 471-487, 577-585
228-236: Right panel sizing and scroll behavior are reasonableThe right panel for calendar/months and visited cities uses
maxHeight: '80vh'andoverflowY: 'auto', withdisableScrollPropagationapplied to the visited‑cities list. This should keep map zoom/scroll from being hijacked while the user scrolls the panel and prevent it from exceeding viewport height.No changes needed here.
Also applies to: 1969-1974
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
app/controllers/api/v1/places_controller.rb (1)
50-63: Strengthen parameter validation in nearby action.The current validation only checks for presence, but
to_fon invalid strings returns0.0, which could lead to unintended behavior (searches at 0°N, 0°E). Additionally, there's no validation that latitude/longitude are within valid geographic ranges or that radius/limit are positive values.Add robust numeric validation:
def nearby - unless params[:latitude].present? && params[:longitude].present? - return render json: { error: 'latitude and longitude are required' }, status: :bad_request + begin + lat = Float(params[:latitude]) + lng = Float(params[:longitude]) + rescue ArgumentError, TypeError + return render json: { error: 'valid latitude and longitude are required' }, status: :bad_request + end + + unless lat.between?(-90, 90) && lng.between?(-180, 180) + return render json: { error: 'latitude must be between -90 and 90, longitude between -180 and 180' }, status: :bad_request end + + radius = [params[:radius]&.to_f || 0.5, 0.1].max + limit = [[params[:limit]&.to_i || 10, 1].max, 100].min results = Places::NearbySearch.new( - latitude: params[:latitude].to_f, - longitude: params[:longitude].to_f, - radius: params[:radius]&.to_f || 0.5, - limit: params[:limit]&.to_i || 10 + latitude: lat, + longitude: lng, + radius: radius, + limit: limit ).call render json: { places: results } end
🧹 Nitpick comments (3)
app/controllers/api/v1/places_controller.rb (2)
6-6: Consider using%isyntax for symbol arrays.The before_action array can use the more idiomatic
%iliteral syntax.Apply this diff:
- before_action :set_place, only: [:show, :update, :destroy] + before_action :set_place, only: %i[show update destroy]
93-115: N+1 concerns addressed; consider extracting serializer.The N+1 query issues flagged in previous reviews have been resolved by reloading with associations in the
create(line 25) andupdate(line 36) actions. However, as this serialization logic grows (21 lines), consider extracting it to a dedicated serializer class (e.g.,Api::PlaceSerializer) for better separation of concerns and reusability.Example extraction to
app/serializers/api/place_with_tags_serializer.rb:module Api class PlaceWithTagsSerializer def initialize(place) @place = place end def call { id: place.id, name: place.name, latitude: place.lat, longitude: place.lon, source: place.source, note: place.note, icon: place.tags.first&.icon, color: place.tags.first&.color, visits_count: place.visits.size, created_at: place.created_at, tags: place.tags.map { |tag| serialize_tag(tag) } } end private attr_reader :place def serialize_tag(tag) { id: tag.id, name: tag.name, icon: tag.icon, color: tag.color, privacy_radius_meters: tag.privacy_radius_meters } end end endThen replace
serialize_place(@place)withApi::PlaceWithTagsSerializer.new(@place).call.db/migrate/20251028130433_add_omniauth_to_users.rb (1)
1-15: LGTM! Past critical issues have been properly addressed.The migration now correctly follows Strong Migrations practices with
disable_ddl_transaction!, explicitup/downmethods, concurrent index operations, and proper guards for idempotency.Optional style consistency: Add the frozen string literal comment at the top for consistency with other files:
+# frozen_string_literal: true + class AddOmniauthToUsers < ActiveRecord::Migration[8.0]Based on learnings.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/controllers/api/v1/places_controller.rb(1 hunks)app/models/concerns/omniauthable.rb(1 hunks)config/initializers/01_constants.rb(1 hunks)db/migrate/20251028130433_add_omniauth_to_users.rb(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
db/migrate/**/*.rb
📄 CodeRabbit inference engine (CLAUDE.md)
Use Strong Migrations practices for safe database schema changes
Files:
db/migrate/20251028130433_add_omniauth_to_users.rb
app/controllers/api/**/*.rb
📄 CodeRabbit inference engine (CLAUDE.md)
Protect API endpoints with Bearer API keys, except explicitly public-share endpoints which must verify UUID access
Files:
app/controllers/api/v1/places_controller.rb
🧠 Learnings (2)
📚 Learning: 2025-09-29T21:15:25.496Z
Learnt from: CR
Repo: Freika/dawarich PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-09-29T21:15:25.496Z
Learning: Applies to db/migrate/**/*.rb : Use Strong Migrations practices for safe database schema changes
Applied to files:
db/migrate/20251028130433_add_omniauth_to_users.rb
📚 Learning: 2025-09-29T21:15:25.496Z
Learnt from: CR
Repo: Freika/dawarich PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-09-29T21:15:25.496Z
Learning: Applies to app/controllers/api/**/*.rb : Protect API endpoints with Bearer API keys, except explicitly public-share endpoints which must verify UUID access
Applied to files:
app/controllers/api/v1/places_controller.rb
🧬 Code graph analysis (2)
db/migrate/20251028130433_add_omniauth_to_users.rb (2)
db/migrate/20251116184506_add_user_id_to_places.rb (3)
disable_ddl_transaction!(1-14)up(4-9)down(11-13)db/migrate/20251118204141_add_privacy_radius_to_tags.rb (3)
disable_ddl_transaction!(3-21)up(6-12)down(14-20)
app/controllers/api/v1/places_controller.rb (4)
app/controllers/api_controller.rb (1)
current_api_user(35-37)app/serializers/api/place_serializer.rb (1)
call(8-22)app/services/places/nearby_search.rb (1)
call(15-30)app/models/place.rb (2)
lat(30-32)lon(26-28)
🪛 RuboCop (1.81.7)
db/migrate/20251028130433_add_omniauth_to_users.rb
[convention] 1-1: Missing frozen string literal comment.
(Style/FrozenStringLiteralComment)
[convention] 7-7: Use %i or %I for an array of symbols.
(Style/SymbolArray)
[convention] 11-11: Use %i or %I for an array of symbols.
(Style/SymbolArray)
app/controllers/api/v1/places_controller.rb
[convention] 6-6: Use %i or %I for an array of symbols.
(Style/SymbolArray)
[convention] 20-31: Assignment Branch Condition size for create is too high. [<2, 19, 3> 19.34/17]
(Metrics/AbcSize)
[convention] 50-63: Assignment Branch Condition size for nearby is too high. [<1, 22, 6> 22.83/17]
(Metrics/AbcSize)
[convention] 93-115: Assignment Branch Condition size for serialize_place is too high. [<1, 22, 3> 22.23/17]
(Metrics/AbcSize)
[convention] 93-115: Method has too many lines. [21/10]
(Metrics/MethodLength)
app/models/concerns/omniauthable.rb
[convention] 7-35: Method has too many lines. [17/10]
(Metrics/MethodLength)
[convention] 27-32: Redundant assignment before returning detected.
(Style/RedundantAssignment)
config/initializers/01_constants.rb
[convention] 50-50: Favor modifier if usage when having a single-line body. Another good alternative is the usage of control flow &&/||.
(Style/IfUnlessModifier)
[convention] 54-54: Favor modifier if usage when having a single-line body. Another good alternative is the usage of control flow &&/||.
(Style/IfUnlessModifier)
🔇 Additional comments (5)
app/controllers/api/v1/places_controller.rb (5)
8-14: LGTM!The index action properly eager-loads associations with
includes(:tags, :visits), preventing N+1 queries. The scoping tocurrent_api_user.placesensures proper authorization, and the filtering logic is well-implemented.
20-31: LGTM!The create action properly addresses the previous N+1 query concern by reloading the place with eager-loaded associations at line 25 before serialization. Authorization is correctly scoped to
current_api_user.
33-42: LGTM!The update action properly addresses the previous N+1 query concern by reloading the place with eager-loaded associations at line 36 before serialization. Authorization is correctly maintained through the scoped
@place.
44-48: LGTM!The destroy action is correctly implemented with proper authorization through the scoped
@placeand returns the appropriate 204 No Content status.
71-91: LGTM!The parameter handling and tag management methods are well-implemented. Importantly, both
add_tagsandset_tagsscope tag lookups tocurrent_api_user.tags, ensuring users cannot associate tags from other users with their places.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (7)
app/controllers/api/v1/places_controller.rb (1)
50-63: Validate numeric parameters to prevent unexpected behavior.The current validation only checks presence, but
to_fon invalid strings returns0.0, causing searches at 0°N, 0°E (Gulf of Guinea). Additionally,radiusandlimitshould be validated for sensible bounds.Apply this diff to add robust numeric validation:
def nearby - unless params[:latitude].present? && params[:longitude].present? - return render json: { error: 'latitude and longitude are required' }, status: :bad_request + begin + lat = Float(params[:latitude]) + lng = Float(params[:longitude]) + radius = params[:radius] ? Float(params[:radius]) : 0.5 + limit = params[:limit] ? Integer(params[:limit]) : 10 + rescue ArgumentError, TypeError + return render json: { error: 'valid numeric latitude and longitude are required' }, status: :bad_request end + + unless lat.between?(-90, 90) && lng.between?(-180, 180) + return render json: { error: 'latitude must be between -90 and 90, longitude between -180 and 180' }, status: :bad_request + end + + unless radius.positive? && limit.positive? && limit <= 100 + return render json: { error: 'radius must be positive, limit must be between 1 and 100' }, status: :bad_request + end results = Places::NearbySearch.new( - latitude: params[:latitude].to_f, - longitude: params[:longitude].to_f, - radius: params[:radius]&.to_f || 0.5, - limit: params[:limit]&.to_i || 10 + latitude: lat, + longitude: lng, + radius: radius, + limit: limit ).call render json: { places: results } endNote: Using
Float()andInteger()(kernel methods) will raiseArgumentErrorfor invalid input, unliketo_f/to_iwhich silently convert to 0.app/javascript/controllers/maps_controller.js (2)
1279-1284: Consider responsive panel width for mobileLine 1281 still uses a fixed
220pxwidth. On narrow viewports (e.g., mobile portrait mode), combined with theleft: 60pxpositioning (implied by topleft position), this could cause overflow or make the panel too wide.Consider:
- div.style.width = '220px'; + div.style.width = window.innerWidth < 640 ? 'calc(100vw - 80px)' : '220px'; + div.style.maxWidth = '220px';Or handle with CSS media queries in the stylesheet.
471-607: Major: Tree control still drops external layers in some flowsThe guard at line 488 (
if (!this.placesFilteredLayers)) prevents re-creating place-tag layers within the same instance, which is an improvement. However, whencreateTreeLayerControl()is called from:
- Line 899 (
deletePoint)- Line 1472 (
updateMapWithNewSettings)...these calls don't pass
additionalLayers, so the "Family Members" overlay is dropped from the tree control even if it's still on the map. OnlyupdateLayerControl(line 2436) correctly accepts and forwardsadditionalLayers.Apply this fix:
// After deletePoint, line 897-901: if (this.layerControl) { this.map.removeControl(this.layerControl); - this.layerControl = this.createTreeLayerControl(); + const additionalLayers = {}; + if (window.familyMembersController?.familyMarkersLayer) { + additionalLayers['Family Members'] = window.familyMembersController.familyMarkersLayer; + } + this.layerControl = this.createTreeLayerControl(additionalLayers); this.map.addControl(this.layerControl); } // Similarly in updateMapWithNewSettings, line 1472-1473app/javascript/maps/places.js (4)
97-98: Critical: Coordinate validation still excludes valid zero valuesThe truthiness check on line 98 will incorrectly filter out places at exactly
0latitude or longitude (e.g., locations on the equator or prime meridian). This was previously flagged but remains unaddressed.Apply this fix:
- if (!place.latitude || !place.longitude) return null; + if (place.latitude == null || place.longitude == null) return null;
109-112: Critical: Missing optional chaining onplace.tagscauses runtime errorsLines 110-111 still access
place.tags[0]without optional chaining ontagsitself. WhenhandleMapClick(line 199) callscreatePlaceIcon({ icon: '📍', color: '#FF9800' }), this will throw becauseplace.tagsis undefined.Apply this fix:
- const emoji = place.icon || place.tags[0]?.icon || '📍'; - const color = place.color || place.tags[0]?.color || '#4CAF50'; + const emoji = place.icon || place.tags?.[0]?.icon || '📍'; + const color = place.color || place.tags?.[0]?.color || '#4CAF50';
139-159: Critical: Multiple XSS vulnerabilities in popup contentLines 140-148 inject user-controlled data into HTML without escaping:
- Line 142:
tag.iconandtag.nameinserted directly into badge HTML- Line 148:
place.nameinserted directly into heading- Line 141:
tag.colorin style attribute (lower risk if server validates)Only
place.note(line 150) is properly escaped. This was previously flagged for tags but also affectsplace.name.Apply this fix:
createPopupContent(place) { const tags = place.tags.map(tag => - `<span class="badge badge-sm" style="background-color: ${tag.color}"> - ${tag.icon} #${tag.name} + `<span class="badge badge-sm" style="background-color: ${this.escapeHtml(tag.color || '')}"> + ${this.escapeHtml(tag.icon || '')} #${this.escapeHtml(tag.name || '')} </span>` ).join(' '); return ` <div class="place-popup" style="min-width: 200px;"> - <h3 class="font-bold text-lg mb-2">${place.name}</h3> + <h3 class="font-bold text-lg mb-2">${this.escapeHtml(place.name || '')}</h3>
314-318: Major: Guard against null markers when adding to layersLine 317 adds markers without checking if
createPlaceMarkerreturnsnull(which it does when coordinates are missing, line 98). This will cause runtime errors.Apply this fix:
data.forEach(place => { const marker = this.createPlaceMarker(place); - layer.addLayer(marker); + if (marker) { + layer.addLayer(marker); + } });
🧹 Nitpick comments (4)
config/initializers/01_constants.rb (1)
45-45: Consider minor style improvements per RuboCop.RuboCop suggests:
- Line 45: Omit parentheses around the ternary condition
- Lines 50-52 and 54-56: Use modifier
iffor single-line conditionalsThese are optional style preferences that improve consistency:
- (ENV['OIDC_CLIENT_ID'].present? && ENV['OIDC_CLIENT_SECRET'].present?) ? %i[openid_connect] : [] + ENV['OIDC_CLIENT_ID'].present? && ENV['OIDC_CLIENT_SECRET'].present? ? %i[openid_connect] : []- if ENV['GITHUB_OAUTH_CLIENT_ID'].present? && ENV['GITHUB_OAUTH_CLIENT_SECRET'].present? - providers << :github - end + providers << :github if ENV['GITHUB_OAUTH_CLIENT_ID'].present? && ENV['GITHUB_OAUTH_CLIENT_SECRET'].present?- if ENV['GOOGLE_OAUTH_CLIENT_ID'].present? && ENV['GOOGLE_OAUTH_CLIENT_SECRET'].present? - providers << :google_oauth2 - end + providers << :google_oauth2 if ENV['GOOGLE_OAUTH_CLIENT_ID'].present? && ENV['GOOGLE_OAUTH_CLIENT_SECRET'].present?Also applies to: 50-52, 54-56
app/controllers/api/v1/places_controller.rb (1)
93-115: Serialization logic is correct.While RuboCop flags complexity metrics, the serialization works correctly with preloaded associations. Extracting to a dedicated serializer class could improve maintainability but isn't essential.
If you'd like to reduce complexity, consider extracting to a serializer:
# app/serializers/place_serializer.rb class PlaceSerializer def self.call(place) { id: place.id, name: place.name, latitude: place.lat, longitude: place.lon, source: place.source, note: place.note, icon: place.tags.first&.icon, color: place.tags.first&.color, visits_count: place.visits.count, created_at: place.created_at, tags: place.tags.map { |tag| TagSerializer.call(tag) } } end endapp/javascript/maps/places.js (1)
329-367: Consider more robust layer visibility coordinationThe
ensurePlacesLayerVisiblemethod relies on:
- Direct DOM queries for
.leaflet-control-layersand checkbox text matching (line 347)- Multiple nested
setTimeoutcalls (lines 343, 358) creating potential race conditions- Global state mutation (
window.mapsController.isRestoringLayers)While functional, this coupling makes the code fragile. Consider a direct API on the maps controller for programmatic layer enabling, or use a more reliable layer reference instead of text matching.
app/javascript/controllers/maps_controller.js (1)
1802-1827: Consider more robust layer restoration approachThe layer restoration logic relies on:
- Multiple nested
setTimeoutcalls (lines 1816, 1820) with arbitrary delays (200ms, 2000ms)- Simulating clicks on checkboxes (line 1873) to trigger tree control events
- Tracking restoration state with
this.restoredPlaceTagLayersSet and countWhile functional, this approach is fragile and race-condition prone. Consider a more direct API for programmatic layer addition/removal, or use event-driven layer state synchronization instead of timeouts and click simulation.
Also applies to: 1870-1874
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
app/controllers/api/v1/places_controller.rb(1 hunks)app/javascript/controllers/maps_controller.js(25 hunks)app/javascript/maps/places.js(1 hunks)app/javascript/maps/places_control.js(1 hunks)app/models/concerns/omniauthable.rb(1 hunks)config/initializers/01_constants.rb(1 hunks)spec/requests/shared/stats_spec.rb(0 hunks)
💤 Files with no reviewable changes (1)
- spec/requests/shared/stats_spec.rb
🧰 Additional context used
📓 Path-based instructions (1)
app/controllers/api/**/*.rb
📄 CodeRabbit inference engine (CLAUDE.md)
Protect API endpoints with Bearer API keys, except explicitly public-share endpoints which must verify UUID access
Files:
app/controllers/api/v1/places_controller.rb
🧠 Learnings (1)
📚 Learning: 2025-09-29T21:15:25.496Z
Learnt from: CR
Repo: Freika/dawarich PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-09-29T21:15:25.496Z
Learning: Applies to app/controllers/api/**/*.rb : Protect API endpoints with Bearer API keys, except explicitly public-share endpoints which must verify UUID access
Applied to files:
app/controllers/api/v1/places_controller.rb
🧬 Code graph analysis (3)
app/controllers/api/v1/places_controller.rb (3)
app/controllers/api_controller.rb (1)
current_api_user(35-37)app/services/places/nearby_search.rb (1)
call(15-30)app/models/place.rb (2)
lat(30-32)lon(26-28)
app/javascript/maps/places_control.js (1)
app/javascript/maps/theme_utils.js (1)
applyThemeToPanel(74-78)
app/javascript/controllers/maps_controller.js (3)
app/javascript/maps/privacy_zones.js (1)
PrivacyZoneManager(7-173)app/javascript/maps/places.js (1)
PlacesManager(7-388)app/javascript/maps/map_controls.js (5)
button(16-16)button(101-101)button(137-137)setCreatePlaceButtonInactive(244-251)setCreatePlaceButtonActive(229-237)
🪛 RuboCop (1.81.7)
config/initializers/01_constants.rb
[convention] 45-45: Omit parentheses for ternary conditions.
(Style/TernaryParentheses)
[convention] 50-50: Favor modifier if usage when having a single-line body. Another good alternative is the usage of control flow &&/||.
(Style/IfUnlessModifier)
[convention] 54-54: Favor modifier if usage when having a single-line body. Another good alternative is the usage of control flow &&/||.
(Style/IfUnlessModifier)
app/controllers/api/v1/places_controller.rb
[convention] 6-6: Use %i or %I for an array of symbols.
(Style/SymbolArray)
[convention] 20-31: Assignment Branch Condition size for create is too high. [<2, 19, 3> 19.34/17]
(Metrics/AbcSize)
[convention] 50-63: Assignment Branch Condition size for nearby is too high. [<1, 22, 6> 22.83/17]
(Metrics/AbcSize)
[convention] 93-115: Assignment Branch Condition size for serialize_place is too high. [<1, 22, 3> 22.23/17]
(Metrics/AbcSize)
[convention] 93-115: Method has too many lines. [21/10]
(Metrics/MethodLength)
app/models/concerns/omniauthable.rb
[convention] 7-35: Method has too many lines. [17/10]
(Metrics/MethodLength)
[convention] 27-27: Use if data['email'].blank? instead of unless data['email'].present?.
(Rails/Blank)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-and-push-docker
🔇 Additional comments (17)
app/models/concerns/omniauthable.rb (1)
7-35: LGTM! All previous concerns have been addressed.The implementation correctly handles OmniAuth account linking:
- Line 18 guards against nil/blank emails before lookup
- Line 22 uses
update!to ensure linking succeeds or raises- Line 27 returns
nilwhen email is missing, establishing a clear contractThe logic flow is sound: find by provider/uid → link by email if found → create new user if needed.
Minor optional note: RuboCop suggests using
.blank?instead ofunless .present?on line 27, but this is purely stylistic.app/controllers/api/v1/places_controller.rb (8)
8-14: LGTM! Index action properly prevents N+1 queries.The association preloading at line 9 ensures efficient serialization, and the filtering options are correctly implemented.
16-18: Show action looks good.The N+1 query concern from previous reviews has been addressed since
set_placenow preloads associations at line 68.
20-31: Create action correctly handles N+1 prevention.The reload with associations at line 25 (added based on previous feedback) ensures efficient serialization.
33-42: Update action properly prevents N+1 queries.The association reload at line 36 addresses previous performance concerns.
44-48: Destroy action is correctly implemented.Using
destroy!is appropriate for an API context where exceptions should propagate.
67-69: Set_place correctly preloads associations.This addresses the N+1 query concern from previous reviews.
71-91: Parameter handling and tag management are well-implemented.The strong parameters and tag methods correctly scope to
current_api_user, preventing unauthorized tag access.
1-118: Code review verified—no changes needed.ApiController correctly enforces Bearer token authentication via the
before_action :authenticate_api_keyhook, which returns 401 Unauthorized ifcurrent_api_useris nil. Theapi_keymethod properly extracts Bearer tokens from theAuthorizationheader usingrequest.headers['Authorization']&.split(' ')&.last. All PlacesController endpoints inherit this protection and properly scope operations tocurrent_api_user. The implementation meets the API security requirements.app/javascript/maps/places.js (1)
294-304: Approved: Query parameter format now consistent with APIThe
tag_idsquery parameter now correctly uses array format (tag_ids[]=1&tag_ids[]=2) viaURLSearchParams.append, matching the API expectations. This addresses the previous concern about comma-joined strings.app/javascript/maps/places_control.js (1)
171-186: Approved: Untagged filter now works correctlyThe
applyCurrentFiltersmethod now properly distinguishes between "show all places" and "show only untagged places" by using the seconduntaggedOnlyparameter (line 179:filterByTags(null, true)). This addresses the previous issue where both cases resulted in the same behavior.The PlacesManager API was correctly updated to accept
filterByTags(tagIds, untaggedOnly = false)(see places.js line 268).app/javascript/controllers/maps_controller.js (6)
120-135: Approved: Sensible default center prioritizationThe new logic for determining the map center follows a clear priority: Home place coordinates → last marker → Berlin. The error handling for parsing home coordinates is appropriate, and the fallback chain ensures the map always has a valid center.
182-186: Approved: Privacy filtering before layer creationInitializing and applying privacy zone filtering before creating map layers (line 188) ensures sensitive location data is never rendered. The async/await pattern is correctly applied.
688-707: Approved: Overlay event names align with tree structureThe event handling for
'Suggested'and'Confirmed'(lines 688, 731, 733) now correctly matches the tree control labels (lines 530, 534), ensuring consistent behavior when toggling visit layers.Also applies to: 731-747
815-824: Approved: Place-tag layer state persistenceThe addition of place-tag layer persistence (lines 815-824) using the
"place_tag:ID"format is clear and consistent. The code correctly iterates filtered layers and saves their state based on_placeTagId.
2440-2480: Approved: Place creation mode toggle logicThe
togglePlaceCreationModeanddisablePlaceCreationModemethods correctly coordinate between the PlacesManager state, button visual updates, and tooltip text. The separation of concerns (disable only when in creation mode, line 2471) prevents unnecessary state changes.
2482-2500: Approved: Privacy zone initialization and filteringThe
initializePrivacyZonesmethod correctly:
- Awaits the async
loadPrivacyZones()call (line 2484)- Filters both markers (line 2490) and tracks (line 2494) before rendering
- Includes appropriate error handling and debug logging
This ensures privacy-sensitive data is filtered early in the initialization flow.
| // Initialize the places manager | ||
| this.placesManager = new PlacesManager(this.map, this.apiKey); | ||
| this.placesManager.initialize(); |
There was a problem hiding this comment.
Major: Missing await on async initialize() call
Line 245 calls this.placesManager.initialize() without await, but PlacesManager.initialize() is async (places.js line 20). This could cause race conditions where the tree control (line 287) or layer restoration (line 291) runs before places are fully loaded.
Apply this fix:
// Initialize the places manager
this.placesManager = new PlacesManager(this.map, this.apiKey);
- this.placesManager.initialize();
+ await this.placesManager.initialize();🤖 Prompt for AI Agents
In app/javascript/controllers/maps_controller.js around lines 243-245, the call
to this.placesManager.initialize() is missing an await which can cause
downstream code (tree control at ~287 and layer restoration at ~291) to run
before places finish loading; change the call to await
this.placesManager.initialize() and ensure the enclosing function is declared
async (or handle the returned Promise with .then()) so subsequent logic runs
only after initialization completes; optionally wrap in try/catch to surface
initialization errors.
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (8)
app/javascript/controllers/place_creation_controller.js (1)
19-27: Memory leak from missing disconnect() cleanupAs noted in the previous review, these document-level event listeners are never removed, which can accumulate handlers on Turbo navigation.
app/javascript/maps/places.js (5)
168-168: Guard against missing coordinates using explicit null checksThe truthiness check
!place.latitude || !place.longitudewill incorrectly reject valid places at 0° latitude (equator) or 0° longitude (prime meridian).Apply this fix:
- if (!place.latitude || !place.longitude) return null; + if (place.latitude == null || place.longitude == null) return null;
180-181: Fix optional chaining on place.tagsAccessing
place.tags[0]before applying optional chaining will throw whenplace.tagsis undefined (e.g., when called with a synthetic place object inhandleMapClickat line 283).Apply this fix:
- const emoji = place.icon || place.tags[0]?.icon || '📍'; - const color = place.color || place.tags[0]?.color || '#4CAF50'; + const emoji = place.icon || place.tags?.[0]?.icon || '📍'; + const color = place.color || place.tags?.[0]?.color || '#4CAF50';
180-181: Escape emoji and validate color to prevent injectionThe
emojivalue is injected directly into HTML (line 196) without escaping, creating an XSS vulnerability. Thecolorvalue is injected into an inline style (line 185) without validation, allowing style attribute breakout.Apply this fix:
createPlaceIcon(place) { - const emoji = place.icon || place.tags?.[0]?.icon || '📍'; - const color = place.color || place.tags?.[0]?.color || '#4CAF50'; + const emoji = this.escapeHtml(place.icon || place.tags?.[0]?.icon || '📍'); + const color = this.validateColor(place.color || place.tags?.[0]?.color || '#4CAF50');Add a color validation helper:
validateColor(color) { // Only allow hex colors if (/^#([0-9a-fA-F]{3}|[0-9a-fA-F]{6})$/.test(color)) { return color; } return '#4CAF50'; // fallback }Also applies to: 185-185, 196-196
210-214: Escape tag properties to prevent XSSThe
tag.icon,tag.name, andtag.colorvalues are inserted into popup HTML without escaping. If these contain HTML/script tags, this creates an XSS vulnerability.Apply this fix:
const tags = place.tags.map(tag => - `<span class="badge badge-sm" style="background-color: ${tag.color}"> - ${tag.icon} #${tag.name} + `<span class="badge badge-sm" style="background-color: ${this.escapeHtml(tag.color)}"> + ${this.escapeHtml(tag.icon)} #${this.escapeHtml(tag.name)} </span>` ).join(' ');
413-416: Check for null markers before adding to layer
createPlaceMarkerreturnsnullwhen coordinates are missing (line 168). Adding a null marker to the layer will cause an error.Apply this fix:
data.forEach(place => { const marker = this.createPlaceMarker(place); - layer.addLayer(marker); + if (marker) { + layer.addLayer(marker); + } });docker/.env.example (1)
13-21: Specify language identifier for code block.The fenced code block lacks a language identifier, which affects syntax highlighting and markdown rendering.
Apply this diff:
-``` +```bash OIDC_CLIENT_ID=client_id_example OIDC_CLIENT_SECRET=client_secret_example OIDC_ISSUER=https://authentik.yourdomain.com/application/o/dawarich/ OIDC_REDIRECT_URI=https://your-dawarich-url.com/users/auth/openid_connect/callback OIDC_AUTO_REGISTER=true # optional, default is false OIDC_PROVIDER_NAME=YourProviderName # optional, default is OpenID Connect ALLOW_EMAIL_PASSWORD_REGISTRATION=false # optional, default is true -``` +```CHANGELOG.md (1)
13-21: Specify language identifier for code block.The fenced code block should declare
bashorshellas the language for proper syntax highlighting.
🧹 Nitpick comments (9)
app/javascript/maps/places.js (1)
427-464: Consider simplifying the layer visibility sync logicThe
ensurePlacesLayerVisiblemethod has complex logic to sync the checkbox state with the layer visibility, including setting flags onwindow.mapsControllerand using multiple timeouts. While functional, this tight coupling makes the code harder to maintain.Consider:
- Emitting a custom event instead of directly accessing
window.mapsController- Consolidating the timeout logic or using a more predictable sync mechanism
- Documenting why the 100ms and 50ms delays are necessary
This would improve maintainability without changing behavior.
app/controllers/users/registrations_controller.rb (1)
101-106: Consider extracting duplicated logic to a shared location.The
email_password_registration_allowed?method here duplicates logic fromApplicationHelper#email_password_registration_enabled?. Both methods:
- Check if self-hosted mode is enabled
- Read and cast
ENV['ALLOW_EMAIL_PASSWORD_REGISTRATION']- Return false as default
Consider reusing the helper method or extracting to a shared concern to maintain DRY principles:
+ include ApplicationHelper + def email_password_registration_allowed? - env_value = ENV['ALLOW_EMAIL_PASSWORD_REGISTRATION'] - return false if env_value.nil? - - ActiveModel::Type::Boolean.new.cast(env_value) + email_password_registration_enabled? endOr alternatively, if you prefer to keep controller logic separate, extract to a shared concern or service object.
spec/views/devise/shared/_links.html.erb_spec.rb (1)
48-50: Remove extra empty line at block end.RuboCop flagged an extra empty line at the end of the block.
Apply this diff:
end end - endapp/controllers/users/omniauth_callbacks_controller.rb (1)
64-69: Consider reusing oidc_auto_register_enabled? from Omniauthable concern.Based on the AI summary, the Omniauthable concern (app/models/concerns/omniauthable.rb) also provides
oidc_auto_register_enabled?with similar logic. Consider reusing that implementation to avoid duplication.If the Omniauthable concern's method is accessible here, you could:
def oidc_auto_register_enabled? - env_value = ENV['OIDC_AUTO_REGISTER'] - return true if env_value.nil? - - ActiveModel::Type::Boolean.new.cast(env_value) + User.oidc_auto_register_enabled? # or include the concern if needed endAlternatively, extract this logic to a shared service or configuration class that both locations can reference.
spec/requests/users/omniauth_callbacks_spec.rb (1)
12-57: Remove or utilize the unusedprovider_nameparameter.The
provider_nameparameter is passed to the shared examples but never used. You can either remove it entirely or incorporate it into test descriptions for better clarity.Apply this diff to remove the unused parameter:
-shared_examples 'successful OAuth authentication' do |provider, provider_name| +shared_examples 'successful OAuth authentication' do |provider| context "when user doesn't exist" doAnd update the usage:
-include_examples 'successful OAuth authentication', :openid_connect, 'OpenID Connect' +include_examples 'successful OAuth authentication', :openid_connectAlternatively, to utilize it for more descriptive tests, you could incorporate it into context descriptions if desired.
Based on static analysis hints from RuboCop.
app/models/concerns/omniauthable.rb (2)
27-37: Consider adding explicit email validation before user creation.While the comment on line 30 acknowledges that validation will fail if email is blank, the method proceeds to call
createregardless. This relies on the User model's validations and the controller to handle the error.For better clarity and to fail fast, consider adding an explicit guard:
# Check if auto-registration is allowed for OIDC return nil if provider == 'openid_connect' && !oidc_auto_register_enabled? + # Require email for user creation + return nil if data['email'].blank? + # Attempt to create user (will fail validation if email is blank) create(This makes the email requirement explicit and avoids unnecessary database calls for invalid data.
Note on method length: RuboCop flags this method as too long (17 lines vs 10 max), but given the sequential nature of the authentication logic (lookup → link → create), the current structure is clear and reasonable.
39-48: Consider extracting duplicated OIDC configuration logic.This method is identical to
oidc_auto_register_enabled?inapp/controllers/users/omniauth_callbacks_controller.rb(lines 63-68). While the duplication is currently manageable, consolidating this logic would improve maintainability.Consider extracting to a shared configuration class:
# config/initializers/omniauth_config.rb or similar module OmniauthConfig def self.oidc_auto_register_enabled? env_value = ENV['OIDC_AUTO_REGISTER'] return true if env_value.nil? ActiveModel::Type::Boolean.new.cast(env_value) end endThen reference it in both locations:
def oidc_auto_register_enabled? OmniauthConfig.oidc_auto_register_enabled? endThis is optional since the logic is simple and unlikely to change, but it eliminates the maintenance burden of keeping both methods synchronized.
app/services/places/visits/create.rb (2)
16-18: Useeachinstead ofmapsince the return value isn’t used
callcurrently builds an array ofnilfromplace_visitsbut never uses it. Usingeachmakes the intent (side-effects only) clearer and avoids an unnecessary array:def call - places.map { place_visits(_1) } + places.each { place_visits(_1) } end
63-80: Guard against empty visit point sets and simplifycreate_or_update_visitIf
Visits::Groupever returns an emptyvisit_pointscollection,visit_points.first.timestampwill raise. You can both guard against that and trim down the method a bit:def create_or_update_visit(place, time_range, visit_points) - Rails.logger.info("Visit from #{time_range}, Points: #{visit_points.size}") - - ActiveRecord::Base.transaction do - visit = find_or_initialize_visit(place.id, visit_points.first.timestamp) - - visit.tap do |v| - v.name = "#{place.name}, #{time_range}" - v.ended_at = Time.zone.at(visit_points.last.timestamp) - v.duration = (visit_points.last.timestamp - visit_points.first.timestamp) / 60 - v.status = :suggested - end - - visit.save! - - visit_points.each { _1.update!(visit_id: visit.id) } - end + return if visit_points.blank? + + Rails.logger.info("Visit from #{time_range}, Points: #{visit_points.size}") + + first_point = visit_points.first + last_point = visit_points.last + + ActiveRecord::Base.transaction do + visit = find_or_initialize_visit(place.id, first_point.timestamp) + + visit.assign_attributes( + name: "#{place.name}, #{time_range}", + ended_at: Time.zone.at(last_point.timestamp), + duration: (last_point.timestamp - first_point.timestamp) / 60, + status: :suggested + ) + + visit.save! + + visit_points.each { _1.update!(visit_id: visit.id) } + end endThis both improves safety and should satisfy the RuboCop ABC/MethodLength complaints without changing behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
CHANGELOG.md(1 hunks)app/controllers/users/omniauth_callbacks_controller.rb(1 hunks)app/controllers/users/registrations_controller.rb(2 hunks)app/helpers/application_helper.rb(1 hunks)app/javascript/controllers/place_creation_controller.js(1 hunks)app/javascript/maps/places.js(1 hunks)app/jobs/area_visits_calculation_scheduling_job.rb(1 hunks)app/jobs/place_visits_calculating_job.rb(1 hunks)app/models/concerns/omniauthable.rb(1 hunks)app/services/places/visits/create.rb(1 hunks)app/views/devise/shared/_links.html.erb(2 hunks)app/views/shared/_place_creation_modal.html.erb(1 hunks)config/initializers/01_constants.rb(1 hunks)docker/.env.example(1 hunks)spec/helpers/application_helper_spec.rb(1 hunks)spec/requests/users/omniauth_callbacks_spec.rb(1 hunks)spec/requests/users/registrations_spec.rb(2 hunks)spec/views/devise/shared/_links.html.erb_spec.rb(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- app/views/devise/shared/_links.html.erb
- app/views/shared/_place_creation_modal.html.erb
- config/initializers/01_constants.rb
🧰 Additional context used
📓 Path-based instructions (2)
spec/**/*.rb
📄 CodeRabbit inference engine (CLAUDE.md)
spec/**/*.rb: Use RSpec as the test framework
Use WebMock for external HTTP request mocking in tests
Files:
spec/views/devise/shared/_links.html.erb_spec.rbspec/helpers/application_helper_spec.rbspec/requests/users/registrations_spec.rbspec/requests/users/omniauth_callbacks_spec.rb
app/jobs/**/*.rb
📄 CodeRabbit inference engine (CLAUDE.md)
Implement long-running or asynchronous work as Sidekiq background jobs
Files:
app/jobs/place_visits_calculating_job.rbapp/jobs/area_visits_calculation_scheduling_job.rb
🧠 Learnings (3)
📚 Learning: 2025-09-29T21:15:25.496Z
Learnt from: CR
Repo: Freika/dawarich PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-09-29T21:15:25.496Z
Learning: Applies to spec/{spec_helper,rails_helper}.rb : Enable code coverage reporting with SimpleCov in the test setup
Applied to files:
spec/helpers/application_helper_spec.rb
📚 Learning: 2025-09-29T21:15:25.496Z
Learnt from: CR
Repo: Freika/dawarich PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-09-29T21:15:25.496Z
Learning: Applies to spec/**/*.rb : Use RSpec as the test framework
Applied to files:
spec/helpers/application_helper_spec.rb
📚 Learning: 2025-09-29T21:15:25.496Z
Learnt from: CR
Repo: Freika/dawarich PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-09-29T21:15:25.496Z
Learning: Applies to app/jobs/**/*.rb : Implement long-running or asynchronous work as Sidekiq background jobs
Applied to files:
app/jobs/area_visits_calculation_scheduling_job.rb
🧬 Code graph analysis (7)
app/javascript/maps/places.js (4)
app/javascript/maps/marker_factory.js (1)
map(157-157)app/javascript/maps/helpers.js (1)
showFlashMessage(122-192)app/javascript/maps/places_control.js (1)
div(209-209)app/controllers/api/v1/places_controller.rb (1)
index(8-14)
app/jobs/place_visits_calculating_job.rb (2)
app/jobs/area_visits_calculation_scheduling_job.rb (2)
queue_as(3-13)perform(7-12)app/services/places/visits/create.rb (1)
call(16-18)
spec/helpers/application_helper_spec.rb (1)
app/helpers/application_helper.rb (2)
oauth_provider_name(134-138)email_password_registration_enabled?(140-147)
spec/requests/users/omniauth_callbacks_spec.rb (1)
spec/support/omniauth.rb (1)
mock_openid_connect_auth(57-85)
app/services/places/visits/create.rb (3)
app/models/user.rb (1)
safe_settings(40-42)app/services/users/safe_settings.rb (2)
merge_threshold_minutes(85-87)distance_unit(117-119)app/models/concerns/nearable.rb (1)
near(11-31)
app/javascript/controllers/place_creation_controller.js (4)
app/javascript/controllers/visits_map_controller.js (1)
badge(60-60)app/javascript/maps/areas.js (1)
radius(81-81)app/javascript/controllers/stat_page_controller.js (2)
response(89-95)data(102-102)app/serializers/tag_serializer.rb (1)
places(23-32)
app/models/concerns/omniauthable.rb (1)
app/controllers/users/omniauth_callbacks_controller.rb (1)
oidc_auto_register_enabled?(64-69)
🪛 markdownlint-cli2 (0.18.1)
CHANGELOG.md
13-13: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 RuboCop (1.81.7)
spec/views/devise/shared/_links.html.erb_spec.rb
[convention] 49-50: Extra empty line detected at block body end.
(Layout/EmptyLinesAroundBlockBody)
spec/requests/users/omniauth_callbacks_spec.rb
[warning] 12-12: Unused block argument - provider_name. If it's necessary, use _ or _provider_name as an argument name to indicate that it won't be used.
(Lint/UnusedBlockArgument)
app/services/places/visits/create.rb
[convention] 35-48: Assignment Branch Condition size for place_points is too high. [<3, 26, 4> 26.48/17]
(Metrics/AbcSize)
[convention] 63-80: Assignment Branch Condition size for create_or_update_visit is too high. [<6, 28, 0> 28.64/17]
(Metrics/AbcSize)
[convention] 63-80: Method has too many lines. [12/10]
(Metrics/MethodLength)
app/models/concerns/omniauthable.rb
[convention] 7-37: Method has too many lines. [17/10]
(Metrics/MethodLength)
app/controllers/users/omniauth_callbacks_controller.rb
[convention] 16-40: Assignment Branch Condition size for failure is too high. [<3, 15, 11> 18.84/17]
(Metrics/AbcSize)
[convention] 16-40: Cyclomatic complexity for failure is too high. [11/7]
(Metrics/CyclomaticComplexity)
[convention] 16-40: Method has too many lines. [20/10]
(Metrics/MethodLength)
[convention] 16-40: Perceived complexity for failure is too high. [11/8]
(Metrics/PerceivedComplexity)
[convention] 44-62: Assignment Branch Condition size for handle_auth is too high. [<3, 19, 8> 20.83/17]
(Metrics/AbcSize)
[convention] 44-62: Method has too many lines. [15/10]
(Metrics/MethodLength)
🔇 Additional comments (25)
app/javascript/controllers/place_creation_controller.js (4)
164-199: LGTM! Well-implemented rendering with XSS protectionThe method properly escapes all user-provided strings via
escapeHtml(), correctly handles both initial render and pagination, and maintains accurate indexing when appending results.
226-275: LGTM! Robust form submission handlingThe method correctly handles both creation and update flows, uses appropriate HTTP methods, provides good error handling with user feedback, and dispatches events for UI coordination.
285-290: LGTM! Standard and safe HTML escapingThe escaping implementation is correct and follows the standard pattern of using textContent assignment to sanitize untrusted strings.
11-17: LGTM! Well-structured controller with clean method implementationsThe controller methods demonstrate solid practices:
- Proper state initialization and cleanup
- Efficient event delegation for tag styling
- Clear separation between create/edit modes
- Incremental radius expansion with appropriate bounds
- Clean form population from nearby place selection
Also applies to: 29-53, 55-73, 114-124, 201-217, 219-224, 277-283
app/javascript/maps/places.js (8)
7-31: LGTM!The constructor and initialization logic is well-structured. The lazy-loading pattern (reloading places when the layer is added to the map) is appropriate.
127-151: LGTM!The URL construction using
URLSearchParamswith array notation (tag_ids[]) properly aligns with Rails parameter handling conventions.
153-165: LGTM!Proper null checking before adding markers to the layer (line 160). This is the correct pattern.
234-238: LGTM!Standard DOM-based HTML escaping implementation is correct and safe.
240-310: LGTM!Event delegation pattern for popup buttons is appropriate. The synthetic place object passed to
createPlaceIconat line 283 highlights the importance of the optional chaining fix flagged earlier.
312-348: LGTM!Proper confirmation dialog, comprehensive marker cleanup across all layers, and appropriate error handling.
375-387: LGTM!The filtered layer pattern with lazy loading on 'add' event is well-designed. The URL construction properly uses array notation for Rails parameter handling.
Also applies to: 392-420
466-484: LGTM!Simple show/hide methods and proper notification event dispatching.
app/helpers/application_helper.rb (1)
134-147: LGTM! Helper methods are well-implemented.Both
oauth_provider_nameandemail_password_registration_enabled?are cleanly implemented with proper defaults and clear logic. The OIDC provider name customization and registration control logic follow Rails conventions.spec/requests/users/registrations_spec.rb (1)
143-200: Excellent test coverage for registration controls.The tests comprehensively cover the email/password registration feature:
- Self-hosted mode with registration disabled (default behavior)
- Self-hosted mode with registration explicitly enabled
- Proper ENV isolation using around hooks
- Both GET and POST endpoints tested
The test structure follows RSpec best practices and ensures the feature works correctly in different configurations.
CHANGELOG.md (1)
7-48: Comprehensive release documentation.The unreleased section clearly documents:
- OIDC configuration with step-by-step guidance
- New features (KML support, Places, Tags, Privacy zones)
- Breaking changes and migration notes
- Bug fixes and improvements
The narrative tone and practical examples make the changelog user-friendly and informative.
spec/helpers/application_helper_spec.rb (1)
1-88: Excellent test coverage for helper methods.The tests thoroughly cover both new helper methods:
oauth_provider_name: Tests OIDC custom/default names and camelization for other providersemail_password_registration_enabled?: Tests cloud mode, self-hosted with ENV enabled/disabled/unsetThe test structure is exemplary:
- Proper use of
aroundhooks for ENV isolation- Clear context descriptions
- Comprehensive branch coverage
- Follows RSpec best practices
app/controllers/users/omniauth_callbacks_controller.rb (2)
16-40: Comprehensive error handling with user-friendly messages.The
failureaction provides excellent error handling with specific, actionable messages for different failure scenarios:
- Invalid credentials
- Timeouts
- CSRF detection
- Discovery issues
- Issuer mismatches
While RuboCop flags this as complex, the user experience benefit justifies the current structure. The error messages are clear and helpful for troubleshooting.
Note: If complexity becomes a maintenance concern in the future, consider extracting error message generation to a service object or concern.
44-62: Solid authentication flow with proper error handling.The
handle_authmethod correctly:
- Calls
User.from_omniauthto find or create users- Signs in persisted users with success flash
- Handles OIDC auto-register disabled scenario with clear messaging
- Redirects with validation errors when user creation fails
The logic properly distinguishes between user creation rejection (nil) and validation failures (unpersisted user object).
spec/requests/users/omniauth_callbacks_spec.rb (4)
1-10: LGTM!Standard RSpec setup with proper Devise configuration for OmniAuth testing.
60-105: LGTM!Comprehensive test coverage for OpenID Connect authentication scenarios including:
- Auto-registration enabled/disabled flows
- Account linking for existing users
- Proper environment variable handling with cleanup
The use of the
aroundblock for environment variable management is the correct pattern.
107-123: LGTM!Good end-to-end integration test for the full OAuth flow. Using a distinct email address helps differentiate this test scenario.
125-134: LGTM!Important security test ensuring CSRF protection doesn't interfere with the OAuth callback flow.
app/models/concerns/omniauthable.rb (2)
1-5: LGTM!Standard ActiveSupport::Concern structure following Rails conventions.
7-26: LGTM!The lookup and account linking logic is well-structured:
- Prioritizes provider/uid lookup for linked accounts
- Guards against nil email before attempting email-based linking
- Uses
update!to ensure linking succeeds or surfaces errorsapp/jobs/area_visits_calculation_scheduling_job.rb (1)
7-11: Dual enqueuing of area and place visit jobs looks solidThe scheduler now dispatches both
AreaVisitsCalculatingJobandPlaceVisitsCalculatingJobper user viaUser.find_each, which fits the Sidekiq-backed pattern we use for long-running work inapp/jobs. No issues from a correctness or queuing perspective.Based on learnings, this aligns with the guidance to run long-running work as Sidekiq jobs.
| // Check the appropriate tag checkboxes | ||
| const tagCheckboxes = this.formTarget.querySelectorAll('input[name="tag_ids[]"]') | ||
| tagCheckboxes.forEach(checkbox => { | ||
| const isSelected = place.tags.some(tag => tag.id === parseInt(checkbox.value)) | ||
| checkbox.checked = isSelected | ||
|
|
||
| // Trigger change event to update badge styling | ||
| const event = new Event('change', { bubbles: true }) | ||
| checkbox.dispatchEvent(event) | ||
| }) |
There was a problem hiding this comment.
Add defensive check for place.tags
Line 99 assumes place.tags is always defined. If the API returns a place object without tags or with tags as null, this will throw a runtime error.
Apply this diff to add defensive handling:
- const isSelected = place.tags.some(tag => tag.id === parseInt(checkbox.value))
+ const isSelected = place.tags?.some(tag => tag.id === parseInt(checkbox.value)) ?? false📝 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.
| // Check the appropriate tag checkboxes | |
| const tagCheckboxes = this.formTarget.querySelectorAll('input[name="tag_ids[]"]') | |
| tagCheckboxes.forEach(checkbox => { | |
| const isSelected = place.tags.some(tag => tag.id === parseInt(checkbox.value)) | |
| checkbox.checked = isSelected | |
| // Trigger change event to update badge styling | |
| const event = new Event('change', { bubbles: true }) | |
| checkbox.dispatchEvent(event) | |
| }) | |
| // Check the appropriate tag checkboxes | |
| const tagCheckboxes = this.formTarget.querySelectorAll('input[name="tag_ids[]"]') | |
| tagCheckboxes.forEach(checkbox => { | |
| const isSelected = place.tags?.some(tag => tag.id === parseInt(checkbox.value)) ?? false | |
| checkbox.checked = isSelected | |
| // Trigger change event to update badge styling | |
| const event = new Event('change', { bubbles: true }) | |
| checkbox.dispatchEvent(event) | |
| }) |
🤖 Prompt for AI Agents
In app/javascript/controllers/place_creation_controller.js around lines 96 to
105, the code assumes place.tags exists and calls .some on it; add a defensive
check by normalizing tags to an array before using it (e.g., const tags =
Array.isArray(place.tags) ? place.tags : []), then use tags.some(...) when
determining isSelected and dispatch the change events as before so the code
won’t throw if tags is null/undefined.
| this.map.eachLayer((layer) => { | ||
| if (layer._tagIds !== undefined) { | ||
| // Check if this place's tags match this filtered layer | ||
| const placeTagIds = place.tags.map(tag => tag.id); |
There was a problem hiding this comment.
Add defensive checks for place.tags
While the API includes tags and should always return an array, accessing place.tags.map() without a null check could crash if the API response changes or is malformed.
Apply this pattern for defensive coding:
- const placeTagIds = place.tags.map(tag => tag.id);
+ const placeTagIds = place.tags?.map(tag => tag.id) || [];Apply the same pattern at both locations (lines 58 and 109).
Also applies to: 109-109
🤖 Prompt for AI Agents
In app/javascript/maps/places.js around lines 58 and 109, the code calls
place.tags.map(...) without validating place.tags; add a defensive check that
ensures tags is an array before mapping (e.g., use Array.isArray(place.tags) ?
place.tags.map(...) : [] or coerce via (Array.isArray(place.tags) ? place.tags :
[]).map(...)) so the expression never throws if tags is null/undefined or not an
array; apply this change at both locations (line 58 and line 109).
| def perform(user_id) | ||
| user = User.find(user_id) | ||
| places = user.places # Only user-owned places (with user_id) | ||
|
|
||
| Places::Visits::Create.new(user, places).call | ||
| end |
There was a problem hiding this comment.
Handle deleted/missing users in perform to avoid hard failures
If the user is deleted between scheduling and execution, User.find(user_id) will raise and the job will fail (with retry: false, it’s then dropped). Consider failing gracefully:
def perform(user_id)
- user = User.find(user_id)
- places = user.places # Only user-owned places (with user_id)
-
- Places::Visits::Create.new(user, places).call
+ user = User.find_by(id: user_id)
+ return unless user
+
+ places = user.places # Only user-owned places (with user_id)
+ Places::Visits::Create.new(user, places).call
endThis keeps the job idempotent and more robust to deletions.
📝 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 perform(user_id) | |
| user = User.find(user_id) | |
| places = user.places # Only user-owned places (with user_id) | |
| Places::Visits::Create.new(user, places).call | |
| end | |
| def perform(user_id) | |
| user = User.find_by(id: user_id) | |
| return unless user | |
| places = user.places # Only user-owned places (with user_id) | |
| Places::Visits::Create.new(user, places).call | |
| end |
🤖 Prompt for AI Agents
In app/jobs/place_visits_calculating_job.rb around lines 7 to 12, the job
currently calls User.find(user_id) which will raise ActiveRecord::RecordNotFound
if the user was deleted between scheduling and execution; change this to handle
missing users gracefully by either using User.find_by(id: user_id) and returning
early if nil, or rescue ActiveRecord::RecordNotFound around the find call, log a
concise message, and exit the job without raising so the job is idempotent and
won’t hard-fail.
| def initialize(user, places) | ||
| @user = user | ||
| @places = places | ||
| @time_threshold_minutes = 30 || user.safe_settings.time_threshold_minutes | ||
| @merge_threshold_minutes = 15 || user.safe_settings.merge_threshold_minutes | ||
| end |
There was a problem hiding this comment.
🧩 Analysis chain
Fix inverted default vs. user settings for time/merge thresholds
@time_threshold_minutes = 30 || user.safe_settings.time_threshold_minutes (and the merge line) will always choose the literal (30/15), ignoring per-user settings. The user setting needs to be on the left:
def initialize(user, places)
@user = user
@places = places
- @time_threshold_minutes = 30 || user.safe_settings.time_threshold_minutes
- @merge_threshold_minutes = 15 || user.safe_settings.merge_threshold_minutes
+ @time_threshold_minutes = user.safe_settings.time_threshold_minutes || 30
+ @merge_threshold_minutes = user.safe_settings.merge_threshold_minutes || 15
endThis preserves custom thresholds while still falling back to sensible defaults.
🏁 Script executed:
#!/bin/bash
# Verify the exact code in the initialize method
echo "=== File: app/services/places/visits/create.rb ==="
cat app/services/places/visits/create.rb | head -30
echo -e "\n=== Search for test files related to create.rb ==="
fd -e rb -g '**/*test*.rb' -o -g '**/*spec*.rb' | grep -i 'visit\|place' | head -10
echo -e "\n=== Search for safe_settings usage ==="
rg -n 'safe_settings' --type=rb -A2 -B2 | head -40Length of output: 1138
🏁 Script executed:
#!/bin/bash
echo "=== Checking Ruby || behavior and user.safe_settings ==="
# Look for safe_settings definition
rg -n 'safe_settings' --type rb -B2 -A2 | head -60
echo -e "\n=== Looking for Places::Visits::Create instantiations ==="
rg -n 'Places::Visits::Create' --type rb -B2 -A2
echo -e "\n=== Looking for test/spec files ==="
find . -name '*spec*' -o -name '*test*' | grep -i visit | head -10Length of output: 659
🏁 Script executed:
#!/bin/bash
echo "=== Looking for safe_settings definition ==="
rg -n 'safe_settings' -B2 -A2 | head -80
echo -e "\n=== Checking test for Places::Visits::Create ==="
find . -path '*/spec/*' -name '*create*visit*.rb' -o -path '*/spec/*' -name '*visit*spec.rb' 2>/dev/null | head -5
echo -e "\n=== Content of place_visit_spec.rb ==="
cat ./git/spec/models/place_visit_spec.rb 2>/dev/null | head -50
echo -e "\n=== Search for initialization tests or threshold tests ==="
rg -n 'time_threshold\|merge_threshold' -B1 -A1
echo -e "\n=== Look at where Places::Visits::Create is called ==="
rg -n 'Places::Visits::Create\.new' -B2 -A2Length of output: 7136
Fix inverted default vs. user settings for time/merge thresholds
The bug is confirmed. In app/services/places/visits/create.rb lines 12-13, the logic is inverted:
@time_threshold_minutes = 30 || user.safe_settings.time_threshold_minutes
@merge_threshold_minutes = 15 || user.safe_settings.merge_threshold_minutesSince 30 and 15 are always truthy, the right operands (user settings) are never evaluated. User-customized thresholds are ignored. Reverse the operands to fix:
def initialize(user, places)
@user = user
@places = places
- @time_threshold_minutes = 30 || user.safe_settings.time_threshold_minutes
- @merge_threshold_minutes = 15 || user.safe_settings.merge_threshold_minutes
+ @time_threshold_minutes = user.safe_settings.time_threshold_minutes || 30
+ @merge_threshold_minutes = user.safe_settings.merge_threshold_minutes || 15
end🤖 Prompt for AI Agents
In app/services/places/visits/create.rb around lines 9 to 14, the default and
user settings are inverted so the literal defaults (30 and 15) always win;
change the assignments to prefer the user's configured values and fall back to
the literals, e.g. use user.safe_settings.time_threshold_minutes || 30 and
user.safe_settings.merge_threshold_minutes || 15 (or use safe navigation like
user&.safe_settings&.time_threshold_minutes || 30) so user-customized thresholds
are respected.
| def place_points(place) | ||
| place_radius = | ||
| if user.safe_settings.distance_unit == :km | ||
| DEFAULT_PLACE_RADIUS / ::DISTANCE_UNITS[:km] | ||
| else | ||
| DEFAULT_PLACE_RADIUS / ::DISTANCE_UNITS[user.safe_settings.distance_unit.to_sym] | ||
| end | ||
|
|
||
| points = Point.where(user_id: user.id) | ||
| .near([place.latitude, place.longitude], place_radius, user.safe_settings.distance_unit) | ||
| .order(timestamp: :asc) | ||
|
|
||
| points.group_by { |point| Time.zone.at(point.timestamp).strftime('%Y-%m') } | ||
| end |
There was a problem hiding this comment.
Normalize distance_unit and guard against missing/invalid values in place_points
Right now user.safe_settings.distance_unit is read multiple times, compared to :km and converted with to_sym later. If it’s nil or unexpected, you’ll either divide by nil or pass an unsupported unit into .near, causing runtime errors. You can normalize once, provide a default, and simplify the logic:
def place_points(place)
- place_radius =
- if user.safe_settings.distance_unit == :km
- DEFAULT_PLACE_RADIUS / ::DISTANCE_UNITS[:km]
- else
- DEFAULT_PLACE_RADIUS / ::DISTANCE_UNITS[user.safe_settings.distance_unit.to_sym]
- end
-
- points = Point.where(user_id: user.id)
- .near([place.latitude, place.longitude], place_radius, user.safe_settings.distance_unit)
- .order(timestamp: :asc)
+ settings = user.safe_settings
+ distance_unit = settings.distance_unit
+ unit = (distance_unit || 'km').to_sym
+ place_radius = DEFAULT_PLACE_RADIUS / ::DISTANCE_UNITS.fetch(unit)
+
+ points = Point.where(user_id: user.id)
+ .near([place.latitude, place.longitude], place_radius, unit)
+ .order(timestamp: :asc)
- points.group_by { |point| Time.zone.at(point.timestamp).strftime('%Y-%m') }
+ points.group_by { |point| Time.zone.at(point.timestamp).strftime('%Y-%m') }
endThis removes the redundant km branch, reduces repeated settings calls, and makes behavior well-defined when distance settings are missing.
📝 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 place_points(place) | |
| place_radius = | |
| if user.safe_settings.distance_unit == :km | |
| DEFAULT_PLACE_RADIUS / ::DISTANCE_UNITS[:km] | |
| else | |
| DEFAULT_PLACE_RADIUS / ::DISTANCE_UNITS[user.safe_settings.distance_unit.to_sym] | |
| end | |
| points = Point.where(user_id: user.id) | |
| .near([place.latitude, place.longitude], place_radius, user.safe_settings.distance_unit) | |
| .order(timestamp: :asc) | |
| points.group_by { |point| Time.zone.at(point.timestamp).strftime('%Y-%m') } | |
| end | |
| def place_points(place) | |
| settings = user.safe_settings | |
| distance_unit = settings.distance_unit | |
| unit = (distance_unit || 'km').to_sym | |
| place_radius = DEFAULT_PLACE_RADIUS / ::DISTANCE_UNITS.fetch(unit) | |
| points = Point.where(user_id: user.id) | |
| .near([place.latitude, place.longitude], place_radius, unit) | |
| .order(timestamp: :asc) | |
| points.group_by { |point| Time.zone.at(point.timestamp).strftime('%Y-%m') } | |
| end |
🧰 Tools
🪛 RuboCop (1.81.7)
[convention] 35-48: Assignment Branch Condition size for place_points is too high. [<3, 26, 4> 26.48/17]
(Metrics/AbcSize)
🤖 Prompt for AI Agents
In app/services/places/visits/create.rb around lines 35–48, normalize
user.safe_settings.distance_unit once (convert to a symbol and fallback to a
safe default such as :km if nil or unrecognized), use that normalized_unit to
compute place_radius by looking up ::DISTANCE_UNITS[normalized_unit] with a
fallback to ::DISTANCE_UNITS[:km], and pass normalized_unit into .near; this
removes the special-case branch for :km, avoids repeated calls to
user.safe_settings, and guards against nil/invalid units so we never divide by
nil or call .near with an unsupported unit.
| # OIDC Auto-Registration | ||
| # Controls whether new users are automatically created when signing in with OIDC | ||
| # Set to 'false' to require administrators to pre-create user accounts | ||
| # When disabled, OIDC users must have an existing account (matching email) to sign in | ||
| # Default: true (automatically create new users) | ||
| OIDC_AUTO_REGISTER=true |
There was a problem hiding this comment.
Verify default value for OIDC_AUTO_REGISTER in documentation.
The comment at line 18 states "default is false", but line 167 states "Default: true (automatically create new users)". This inconsistency could confuse users during setup.
Please clarify which default value is correct, or update the documentation to be consistent. Based on security best practices, defaulting to false (requiring admin pre-creation) would be more secure for self-hosted instances.
🤖 Prompt for AI Agents
In docker/.env.example around lines 163 to 168 (and the conflicting comment at
line 18), the documented default for OIDC_AUTO_REGISTER is inconsistent; update
the file so the default is consistent and secure: change the inline "Default:
true" note and the OIDC_AUTO_REGISTER value to "false" (or if policy requires
true, change the earlier comment at line 18 to match) and ensure both the brief
description and the example value reflect the chosen default; keep the
explanatory sentence that clarifies what true/false do and prefer setting the
example to false for a secure default for self-hosted instances.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.